Repository navigation
Conversation
Backport the platform dependency selection proposed in codewhale-hq#7139. The locked rquickjs-sys 0.12.2 archive has no bundled GNU LLVM target bindings; select the existing bindgen feature for that target. Validated with clean locked sources on native x64 Windows GNU LLVM, Rust 1.98.1 and libclang 18.1.1: default workflow-js check passed and 73 existing VM tests passed. Other architectures remain unverified. Signed-off-by: Codex <codex@openai.com>
Backport codewhale-hq#7138 to the fork engine factories. Publish normal or abnormal completion only after the actual run future is dropped; direct constructors remain NotObserved. The exec consumer now bounds its previously unbounded Shutdown send and completion wait with one two-second deadline, warns on unconfirmed normal return, and preserves the original task result. Completion does not certify durable saving or detached writer drain. Clean Rust 1.98.1 x64 Windows GNU LLVM product check passed; run-completion tests: 12 passed, 0 failed (11 new and one existing). The unchanged deadline assertion has a recorded failing renewed-budget mutation and restored positive control. Scoped independent review passed. Signed-off-by: Codex <codex@openai.com>
asto18089
left a comment
There was a problem hiding this comment.
Fresh full review at head f83e8cd (pinned). Setup: base f1eb10e == current pinvou3-clean tip, so no rebase was needed and the branch is MERGEABLE; the claimed final tree hash matches (git rev-parse f83e8cd84^{tree} = 118a6c28d6a38c498e702a68cd045922bb83a592).
Verification performed: all 12 run-completion tests pass locally (11 new + pre-existing prior_run_completion_replay_does_not_replace_active_run — matches the body's count); 240/240 forkguard_ tests pass on the head; a mechanical diff of all 200 fork-guard fingerprint pins shows every pin greps identically at base and head; two targeted mutations both fail the new tests (publish-before-drop in finish() panics the Run::drop ordering assert; splitting the shared 2s deadline into a fresh post-send budget fails run_completion_exec_shutdown_has_one_deadline_for_send_and_wait), and the tree is clean again after revert; fork-guard.sh --fast stays green on the registered parent state.
Verdict: approve direction. The PR does exactly what it claims, root-caused, with no blocker or MAJOR findings.
- Usefulness/completeness — verified TRUE, claim by claim. The hazard was real: the engine op mailbox is bounded (
ENGINE_OP_CHANNEL_CAPACITY = 32) and the old in-looplet _ = engine_handle.send(Op::Shutdown).await;(base exec_agent.rs:1084) was unbounded, so a wedged-but-alive engine could hang headless exec at exit forever. The PR bounds send+wait under one absolute 2s deadline, observes the actual run outcome, and preserves the task result (the warn is additive only). Both factories wire a per-spawn watch channel on the exact handle they return; cross-run contamination is impossible and pinned by test. The commit-1 claim checks out against the locked crate itself: rquickjs-sys 0.12.2 ships no*-windows-gnullvmbundled bindings (verified in the crate'sbindings/directory), sotarget_abi = "llvm"+ bindgen is the correct prerequisite, and Cargo.lock is untouched. - Elegance — near-verbatim upstream reuse. Diffed hunk-by-hunk against upstream codewhale-hq#7138/codewhale-hq#7139:
run_completion.rsis byte-identical except the module path (crate::utils::spawn_supervised, semantically equivalent), Cargo.toml is blob-identical to codewhale-hq#7139, and every other divergence is a forced fork adaptation (2-arg/3-arg factory signatures, flat tests.rs layout, import lists, fork-accurate README sentence — verified factually true against the base). No reinvention. The boxing change also removes the fork's pre-existing extra-async-wrapper divergence aroundengine.run()— a stack-frame hazard upstream already documents — so this PR reduces fork drift rather than adding it. - Smuggling — CLEAN. Commit 1 touches only
crates/workflow-js/Cargo.toml(byte-identical to upstream codewhale-hq#7139); commit 2 carries all six engine/exec files; every hunk attributes to a declared purpose; no lockfile/version/workflow/i18n churn, no mode changes or binaries, no secrets or private addresses in added lines or messages. DCO compliant: author = committer = sign-off (Codex <codex@openai.com>), matching the written rule and existing precedent for bot identities on this branch family. - Correctness — clean under every interleaving traced. The terminal-value invariant (run future dropped strictly before publish; sender dropped only after publish) holds for normal return, supervised panic, abort before/after first poll, and runtime drop; the
changed()-error fallback cannot produce a wrong verdict becausefinish()always publishes first. Exec exit paths enumerated: no path loses its shutdown; the only path that gains one (event-channel-closed exit) is harmless because the watch is already terminal there;Op::Shutdownis consumed between turns so it cannot abort a turn mid-flight; performance is neutral (one watch allocation per spawn, one trampoline poll layer).
Minors / nits (none blocking):
- M1 (inherited, not this PR's regression): guard pins
T3at fork-guard.sh:223-224 (run \tool_search` first to activate it; if it cannot be surfacedandactivate it via `tool_search` as well, since activating `registry_sync` does not activate it) no longer grep — the base commit #125 rewroteMCP_REGISTRY_FIRST_INSTRUCTION`. They already fail at f1eb10e, so this PR breaks nothing; the parent-side gitlink-advancing PR must refresh those two fingerprints. - M2: the exec cleanup budget (2s) equals the MCP stdio shutdown grace (
STDIO_SHUTDOWN_GRACE, mcp/stdio.rs:35), so one slow MCP stdio server can consume the entire budget on every otherwise-successful exec — up to +2s exit latency plus a warn per run. Bounded and intentional (upstream shares the budget); fine to keep, worth revisiting only if headless exit latency matters. - M3: the new
tracing::warn!is currently invisible in plain headless exec — no tracing subscriber is installed on that path (only the TUI event loop callsruntime_log::init()), so the diagnostic is latent until an embedding host installs one. Consistent with the pre-existing lifecycle-outbox warn; noting for awareness. - M4 (test gap, shared with upstream codewhale-hq#7138): nothing executes
run_exec_agentend-to-end, so warn emission, post-loop deadline placement, and result preservation on timeout are documented but unpinned. The extractedshutdown_exec_engineplus its four tests cover the extracted core well; acceptable as follow-up. - N1: the new windows-gnu-llvm section in
crates/workflow-js/Cargo.tomlis the only target section without an explanatory comment (android/freebsd/netbsd/ohos each have one). - N2: in
run_completion_exec_shutdown_has_one_deadline_for_send_and_wait,let _run = engine.observe_run_for_test(std::future::pending());is load-bearing — the underscore-prefixed binding keeps the run alive to scope end; alet _ =rewrite would publish Abnormal immediately and break the assertions. A one-line comment would prevent a future cleanup regression. - N3: the channel-closed loop exit now also performs the shutdown attempt + wait — a small behavior delta beyond the commit message's wording. Benign (completion is already terminal there) and arguably covered by the README's "after the event loop"; noting for the record.
- Info: upstream codewhale-hq#7138/codewhale-hq#7139 are still open; re-diff against their final merged form at the next baseline sync. The parent-side integration PR should carry the
docs/fork-modifications.mdregister row per the fork conventions (this diff removes divergence; no newforkguard_pins are warranted, and the existing 240 all pass).
JensenChen28
left a comment
There was a problem hiding this comment.
Reviewed head f83e8cd84dc52977e0f0cd219d65a301c44f5d6e against target f1eb10efcfe2468fde42d1724d7f5f19d54b904c.
Standards
No required findings or actionable Fowler heuristic found. The implementation reuses the existing supervisor, keeps per-run completion ownership in a small module, and documents the distinction between run termination, persistence and detached work. Independent Standards review agrees.
Spec
No required findings. Both spawn factories attach an independent observation channel to their exact run. Normal completion is published only after the owned future is dropped; abnormal destruction publishes a distinct outcome. Cloned receivers retain terminal results across cancelled waits, and direct constructors explicitly report NotObserved. The exec consumer shares one absolute two-second deadline across Shutdown delivery and completion waiting, adding only a warning without replacing the task result. GNU LLVM bindgen is scoped to the intended Windows target; dependency versions and lockfile are unchanged. Independent Spec review agrees.
Validation executed here: source and consumer review, plus git diff --check (passed). Cargo is unavailable in this review environment, so Rust compilation, native execution, the author's test/mutation counts and the prior peer's execution claims are not reported as my own results. No head check runs were observed. Early-error exits, cooperative timeout limits, detached writers and durable saving retain the explicitly documented boundaries. The existing peer discussion was read; its diagnostic/latency and end-to-end coverage observations do not establish a blocker for this scoped component.
Standards: 0 required findings. Spec: 0 required findings.
No-Issue: Backports the reusable run-completion API and Windows GNU LLVM bindings configuration proposed upstream in codewhale-hq#7138 and codewhale-hq#7139.
Both fork engine factories now expose cancel-safe completion of their exact spawned run. Normal or abnormal completion is published after the owned run future is dropped; direct constructors report NotObserved. The exec consumer shares one two-second budget between Shutdown delivery and waiting. This intentionally replaces the fork's unbounded Shutdown send and preserves the original task result.
A separate prerequisite commit enables rquickjs bindgen only for Windows GNU LLVM, whose locked crate has no bundled target bindings. No dependency version or lockfile changes are included.
Validation on base f1eb10e with Rust 1.98.1, native x64 Windows GNU LLVM, isolated libclang 18.1.1 and clean checksum-verified dependency sources:
The final tree is 118a6c28d6a38c498e702a68cd045922bb83a592. This contributes an independently usable component; completion does not certify durable saving, detached-writer drain or complete installation preparation. Existing early error exits retain their documented cleanup limits. Parent application integration, required fork CI and release qualification remain separate. This draft does not authorize advancing the protected maintenance branch or changing tags.
2026-11-02 当前合入依赖核对:父仓 Pinvou/pinvou3 的 main 及本次检查的 49 个本人开放 PR 均固定 CodeWhale gitlink 为 f1eb10e;其真实 verify-public-submodule.sh 两种现行版本都要求公开维护分支头恰好等于该 gitlink,不能仅以提交仍在历史中代替。单独推进本维护分支而不完成父引用对应安排,会使父仓下一次公共闭包校验失败。
因此本次保留 Draft 与当前维护分支,先完成父仓 gitlink、登记/指纹及实际组合验收的协调,再重新判断最终合入资格;这不是本 PR 产品代码缺陷或 CI 已修复声明,也不因本段关闭父仓业务 issue。没有移动不可变标签、放宽校验或修改父仓引用。