fix(core): Preserve fire-and-forget hooks after exit - #10288
Conversation
|
Re-run after the round-6 fixes landed at
Moving on to code review. 🔍 中文说明第 6 轮评审意见修复(
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal for "keep fire-and-forget hook input, deadline, and tree supervision alive across parent exit" lands on the same shape this PR implements: stdin/stdout can't be parent-owned if the child must outlive the parent, so input goes through a staged temp file; the parent's The two round-6 Criticals are genuinely fixed at this head — not just claimed:
Integration reads clean: the eval'd supervisor source interpolates only internal constants — no user input reaches the eval'd text; command, args and NODE_OPTIONS travel as argv/JSON. Input staging is 0600 + Behavior change worth a changelog note (also flagged in the earlier human pass): generic Not re-raising the round-7 Suggestion-level probes (supervisor Test evidence — the PR's own CIThe macOS/Windows unit-suite skips are by design, not caused by this PR:
Sandboxed verification is already running for this triage run ( Real-scenario testingN/A — non-user-visible process lifecycle change; there is no TUI surface to drive. The evidence base is the real-process test suite running in CI, quoted above. 中文说明代码审查针对"让 fire-and-forget Hook 的输入、超时与进程树监督在父进程退出后继续有效"这个问题,我的独立方案与 PR 的实现形态一致:子进程要比父进程活得更久,stdin/stdout 就不能由父进程持有,因此输入走暂存临时文件;父进程的 第 6 轮的两个 Critical 在当前头提交上是真修复,不是口头声明:
**集成部分读下来是干净的:**被 eval 的 supervisor 源码只内插内部常量——没有任何用户输入进入被 eval 的文本;命令、参数与 NODE_OPTIONS 通过 argv/JSON 传递。输入暂存为 0600 + 值得在 changelog 提一句的行为变化(此前人工审查也提过):其他事件上的普通 不再重复第 7 轮的建议级探针(supervisor 的 测试证据 —— 本 PR 自己的 CImacOS/Windows 单测的跳过是工作流设计使然,与本 PR 无关: (CI 结论表见上方英文部分的机器可读区域,由 finalize 工作流在 CI 稳定后就地更新。) 沙箱验证已随本次 triage 运行启动( 真实场景测试N/A —— 非用户可见的进程生命周期变更,没有可驱动的 TUI 面。证据基础是 CI 中运行的真实进程测试套件(见上)。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — everything verifiable at this head is green and the two round-6 Criticals are test-pinned fixes; the residuals are tracked follow-ups and platform coverage, not doubts about the change. Stepping back: this is a follow-up done right by round 8. The problem is a real contract gap #10100 left behind, the mechanism (staged input + detached supervisor owning deadline and process group) is the minimal one that can keep those guarantees alive across parent exit, and the implementation is honest about its edges — the untrappable-SIGKILL boundaries and the Windows descendant-reclamation gap are documented rather than hand-waved. Test growth tracked the risk: every lifecycle claim — survival after explicit and natural exit, timeout enforcement after exit, abort through a stalled supervisor, root-before-descendant supervision, 5 MiB input integrity, real exit 124 preservation, NODE_OPTIONS isolation, 0600 staging and cleanup — has a real-process test behind it, green on CI. Approving because the code earned it, not because the rounds ran out. Non-blocking residuals, named so nothing is silently dropped:
Also for the maintainer's awareness: wenshao's round-6 request-changes review still stands formally against this head even though both findings are fixed — it needs a human dismiss/re-review alongside the second approving vote. 中文说明置信度:4/5 —— 当前头提交上所有可验证项均为绿色,第 6 轮的两个 Critical 已有测试锚定的修复;剩余事项是已跟踪的后续项与平台覆盖问题,不是对改动本身的疑虑。 退一步看:这是第 8 轮时一个被正确打磨出来的后续修复。问题是 #10100 遗留的真实契约缺口;机制(暂存输入 + 持有超时期限与进程组的 detached supervisor)是能让这些保证跨越父进程退出的最小机制;实现对自己的边界也是诚实的——无法捕获的 SIGKILL 边界与 Windows 后代回收缺口被明确记录在文档中,而不是回避。测试增长与风险相匹配:每一个生命周期声明——显式与自然退出后的存活、退出后的超时强制、穿过卡死 supervisor 的 abort、根进程先于后代退出时的持续监督、5 MiB 输入完整性、真实 124 退出码保留、NODE_OPTIONS 隔离、0600 暂存与清理——背后都有真实进程测试,且在 CI 上通过。批准是因为代码本身达标,而不是因为评审轮次用完了。 非阻断的剩余事项,点名列出以免被静默遗漏:
另请维护者留意:wenshao 第 6 轮的 request-changes 评审在当前头上形式上仍然有效,尽管其中两项发现均已修复——需要人工 dismiss/复审,并补上第二张赞成票。 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6a": none — I finished all checks I opened..
中文说明
已审查。 建议见行内评论。
未探索到全部深度(达到工具调用预算):"agent 6a":none — I finished all checks I opened.。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed review feedback in
Verification: Core build and typecheck passed; 324 focused tests passed; changed-file ESLint, Prettier, and diff checks passed. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM!✅
— qwen3.8-max via Qwen Code /review (v0.22.2)
chiga0
left a comment
There was a problem hiding this comment.
Standard-tier review. No blocking findings.
Core change in hookRunner.ts: survivesParentExit is now determined by event type (MessageDisplay | StopFailure | SessionDelete) rather than by isAsyncHook. Surviving hooks get stdio: ['pipe', 'ignore', 'ignore'] so stdout/stderr are parent-independent sinks — the broken-pipe failure path is closed. Generic async: true hooks are removed from the surviving set: their captured output belongs to the in-memory AsyncHookRegistry, so they correctly remain process-scoped and are reaped on parent exit.
Test coverage: Unit tests directly assert stdio: ['pipe', 'ignore', 'ignore'] for all three surviving event types (sync and async variants). A new process test row adds ['async', 'process-exit', true] to confirm async hooks ARE reaped. The surviving-hook process tests now cover StopFailure and SessionDelete in addition to MessageDisplay, and the fixture exercises explicit stdout/stderr writes after parent exit (await write(process.stdout, 'late stdout') etc.) so the no-broken-pipe guarantee is end-to-end witnessed.
CI: Test (ubuntu-latest), Desktop Shell (ubuntu-22.04), Desktop Shell (windows-2022), web-shell E2E Smoke all pass.
Reviewed with AI assistance.
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 74 passed · 0 failed · 74 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:74 通过 · 0 失败 · 74 总计 抖动门:✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10288 Deep Verification — fix(core): Preserve fire-and-forget hooks after exitVerdict: 中文摘要
Central claim and A/B proofCentral claim: output-ignored The production diff is 8+4 lines in
Every cell dispatches a real command hook through the real
Read across, the table proves each half of the fix is load-bearing and neither alone suffices: S2/S3 flip from killed-on-exit to survives-but-EPIPE between base and mid (predicate half), and from EPIPE to completed between mid and head (stdio half); S1 shows the stdio half alone is what fixes Consumer-side check (the change suppresses captured output): no code consumes stdout/stderr of these events. CorrectionsNone needed — no prior-round claims to correct (first round). FindingsNo blocking findings. Two informational notes a reviewer should have:
Not covered
MethodologyEnvironment: CI verify container ( Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review. The bot already has a review of its own on 机器人在 The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
788fc5f
|
Addressed all four Critical findings in
Validation:
|
chiga0
left a comment
There was a problem hiding this comment.
Tier: Deep(进程生命周期 + 父子依赖)— 增量 130d4a36 → 788fc5f1
已核验内容
本次增量对 fire-and-forget hooks 做了重大重构,引入 supervisor 模式 全面替换了之前的直接 spawn 方案,解决所有 4 个 Critical 问题:
R3-1(surviving hook 失去配置的 timeout)
Supervisor 在自身进程内启动 setTimeout(timeout) 并在 parent 退出后继续持有。Supervisor 被设计为独立进程(detached: true + child.unref()),不依赖 parent 的事件循环。✓
R3-2(child/timeout handle 仍被 parent 引用)
child.unref() 在 supervisor spawn 后立即调用,parent 不再持有引用,可正常退出而不等待 supervisor。✓
R3-3(root shell 退出但 background descendant 残留)
Supervisor 通过 process.kill(-hook.pid, 0) 轮询整个进程组是否存活(POSIX),使用 setInterval + groupAlive() 确保 group 完全退出才结束;SIGTERM/SIGKILL 也发送给整个进程组 (process.kill(-hook.pid, signal))。Windows 路径使用 taskkill /f /t /pid。✓
R3-4(stdin 仍是 parent-owned pipe)
createSurvivingHookInputFile 将 input 写入 mode-0600 临时文件,supervisor 用 openSync(inputPath, 'r') 打开并作为 hook 的 stdin fd 传入。spawn 后立即 closeSync(inputFd) + removeInput()(unlink)——由于 hook 子进程已继承了 fd,unlink 不影响读取,且文件不会在磁盘上长期残留。✓
其他逻辑
- Supervisor source 以
String.rawtemplate literal 嵌入 TypeScript,${SURVIVING_HOOK_TIMEOUT_EXIT_CODE}和${SURVIVING_HOOK_SUPERVISOR_GRACE_MS}在编译时插值为常量值 ✓ process.on('exit', ...)兜底:supervisor 进程退出时如果 group 仍存活,发送 SIGKILL ✓- 错误路径(spawn 失败)清理临时文件 ✓
CI
1 success(web-shell E2E Smoke),1 failure(Post Coverage Comment — CI 基础设施任务,非测试失败),1 in_progress,27 skipped。
无阻塞项。
E2E verification report — head
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally (the supervisor's Windows code paths stay unexercised; the real-process suite is POSIX-only).
Not explored to full depth (tool budget reached): "agent 1a": did not run hookRunner.test.ts / hook-runner.process.test.ts — the worktree has no node_modules /built dist , and installing plus building was too costly ….
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/core/src/hooks/hookRunner.ts:993 — [review] Async-hook exit-time fate flip under-disclosed (PR body claims no behavior change / no breaking changes; base code and the deleted test contradict it)
Convergence: round 4 posted 10 inline comment(s), 10 of them reported for the first time. Findings keep coming back to the same files: packages/core/src/hooks/hookRunner.ts (findings in round 3; 10 more now). (Evidence: the previous round was recovered from a marker this account did not post, so those rounds may not be this account's own.) A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally (the supervisor's Windows code paths stay unexercised; the real-process suite is POSIX-only)。
未探索到全部深度(达到工具调用预算):"agent 1a":did not run hookRunner.test.ts / hook-runner.process.test.ts — the worktree has no node_modules /built dist , and installing plus building was too costly …。
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 4 轮发布了 10 条行内评论,其中 10 条是首次提出。发现反复回到同一批文件:packages/core/src/hooks/hookRunner.ts(第 3 轮已出过发现,本轮又有 10 条)。(证据说明:上一轮的数据来自并非本账号发布的标记,上述轮次可能不属于本账号。)一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
38ad1ba to
3d33c33
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
|
[codex] Rebased the PR branch onto current The four PR commits are patch-identical after rebase ( |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R5-10 async-hook exit-time fate flip under-disclosed (generic async:true hooks re-scoped to process-scoped; Async Hooks docs section contradicts the shipped example; PR body claims no behavior change) — already reported (round-4 deferred li…
Convergence: round 5 posted 7 inline comment(s), 6 of them reported for the first time; the previous round posted 10 (10 new). Findings keep coming back to the same files: packages/core/src/hooks/hookRunner.ts (findings in rounds 3, 4; 6 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查——无阻断问题。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
收敛情况:第 5 轮发布了 7 条行内评论,其中 6 条是首次提出;上一轮发布了 10 条(其中 10 条首次提出)。发现反复回到同一批文件:packages/core/src/hooks/hookRunner.ts(第 3、4 轮已出过发现,本轮又有 6 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。本轮没有未决的 Critical,因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
[codex] Review round 5 reports no blockers. I independently confirmed the seven inline comments as Suggestion-level follow-ups, recorded the shared scope in #10386, replied to each thread, and resolved 7/7. No code was changed in this round: per the repository convergence rule, after roughly five review rounds this PR should accept only Critical fixes. CI remains green on |
yiliang114
left a comment
There was a problem hiding this comment.
Independent pass at 3d33c33, focused on the exit lifecycle (prior 23 threads all resolved; bot CHANGES_REQUESTED treated as stale). No substantiated findings. Verified the surviving-hook design: MessageDisplay/StopFailure/SessionDelete hooks spawn a detached, unref-ed node supervisor (NODE_OPTIONS scrubbed, stdio ignored) that opens its input file synchronously at startup (openSync before any async work) and removes it itself, so the parent-side cleanup rmSync on child close cannot starve the supervisor; the input temp file is 0600/wx/unique; timeout is enforced inside the supervisor with a distinct exit code mapped back to the standard timeout result; abort still terminates the supervisor tree with its own grace budget. Two residuals for the author, not findings: (1) survival is now decided by event type instead of isAsyncHook(hookConfig) — intentional per the updated design doc, but worth a changelog note for anyone configuring async hooks on other events; (2) a power-loss between createSurvivingHookInputFile and spawn would leak a 0600 temp file in tmpdir (harmless). 616 added test lines cover the process-level paths.
yiliang114
left a comment
There was a problem hiding this comment.
Approving. Re-verified all four standing findings at head 3d33c33 — each is fixed by the supervisor-process redesign, not papered over: (1) the surviving-hook timeout now lives in a detached supervisor (node --eval, detached:true + child.unref(), hookRunner.ts:1032-1037) with its own group-level timeout, independent of parent exit; (2) no parent-held references remain for surviving hooks (stdio:'ignore', timeoutHandle guarded by !survivesParentExit at :1148); (3) the supervisor polls process-group liveness after root close and SIGKILLs the group on its own exit; (4) stdin is delivered via a 0o600 temp file opened as fd 0 and removed immediately, so parent exit can no longer truncate large inputs. Regression tests map 1:1 to the findings (timeout-after-parent-exit, group supervision past root exit, large-input delivery). CI green on the head.
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — local dependency installation failed with ENOTEMPTY before packages/core could build or test.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit round 3 — the auditor failed to execute twice.
Not reviewed: "agent reverse-audit (round 3)" — the agent made no tool call: it read nothing.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round; 2 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/core/src/hooks/hookRunner.ts:1007 — [probe] Critical [fails-closed] [new-surface] D6-2 Sequential surviving plans lose unstarted hooks after parent exitpackages/core/src/hooks/hookRunner.ts:1031 — [probe] Critical [fails-closed] [new-surface] D6-1 Invalid hook cwd can prevent supervisor startup and leak staged input
Convergence: round 6 posted 2 inline comment(s), 2 of them reported for the first time. Findings keep coming back to the same files: packages/core/src/hooks/hookRunner.ts (findings in rounds 3, 4, 5; 2 more now). (Evidence: the previous round was recovered from a marker this account did not post and merged over this account's own entries, so some of those rounds may not be this account's own.) A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — local dependency installation failed with ENOTEMPTY before packages/core could build or test。
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查:reverse audit round 3 — the auditor failed to execute twice。
未审查:"agent reverse-audit (round 3)"——该 agent 未发起任何工具调用:它什么都没读。
收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改;其中 2 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface:未认证任何错误结果,且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中:共 2 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 6 轮发布了 2 条行内评论,其中 2 条是首次提出。发现反复回到同一批文件:packages/core/src/hooks/hookRunner.ts(第 3、4、5 轮已出过发现,本轮又有 2 条)。(证据说明:上一轮的数据来自并非本账号发布的标记,并与本账号自己的条目合并,上述轮次中的部分可能不属于本账号。)一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)
机制健康:本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff,除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上;并会一直如此,直到某一轮的标记重新带上锚点,或落地的嫁接能被运行该轮的评审使用。(仅陈述,不据此行动——这不改变本轮发布的任何内容。)
— gpt-5.6-sol via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the two round-6 Critical findings in
Verification: focused lint and core typecheck passed; HookRunner unit/process suites passed (85/85); independent process probes confirmed the stopped-supervisor hook group is gone at return and delayed handling preserves |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- Windows supervisor-branch coverage gap — already reported (R5-3/R5-4, comments 3879548650 and 3879548656; deferred to #10386)
- Supervisor poll-vs-timeout completion race — already reported (R5-5, comment 3879548663; deferred to #10386)
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/core/src/hooks/hookRunner.test.ts:1210 — [probe] unit cancellation test never exercises the supervisor-reported hook-group kill branchpackages/core/src/hooks/hookRunner.ts:1341 — [probe] supervisor 'failed' outcome is parsed but never consumed — spawn diagnostic lostpackages/core/src/hooks/hookRunner.test.ts:1143 — [probe] TMPDIR steering is inert on Windows — spawn-throw cleanup assertion vacuous
中文说明
已审查——无阻断问题。
本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
收敛姿态下延后(第 7 轮,非阻断)——已记录,本轮不要求修改:共 3 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 83 passed · 0 failed · 83 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:83 通过 · 0 失败 · 83 总计 抖动门:✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) - follow-up round at new head Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, mutation/vacuity matrix, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 83 passed · 0 failed · 83 total (70 A/B process assertions + 13 post-check assertions) Verified head: 中文 — 判定:✅ 通过 · 可合入(agent 判定,第二轮)
Previous-round finding status (follow-up round)The previous round verified head
No blocking findings existed in the previous round; none were found now. Central claim and A/B proofCentral claim: output-ignored The A/B drives each tree's own
Read across, the table proves the redesign load-bearing on every axis the PR names: survival (S1–S3 vs base SIGKILL/EPIPE), input integrity (S4), retained deadline (S5), supervision past root exit (S6), abort forwarding past parent exit (S7), the agreed async-reaping change (S8), no regression to ordinary hooks (S9, S10b), exit-code authority (S10a), supervisor isolation (S11), and staging hygiene (S12). S1n additionally shows the referenced-handle fix: on base the parent cannot even naturally exit while a surviving hook runs. CorrectionsNone — no prior-round claim was inaccurate for the head it measured. The previous round's "8+4 lines, two half-changes" description applied to the old head; the mechanism was replaced since, which is why this round re-measured everything rather than carrying cells forward. FindingsNo blocking findings. One non-blocking note:
Not covered
MethodologyEnvironment: CI verify container ( Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Local verification report (Linux) — PR #10288I built a real environment and A/B-tested this change against its merge base. The bug reproduces on Setup
A/B method. The PR touches exactly one source file, so "before" is produced by checking out
No mocks of 1. The reported failure reproduces, and the PR fixes itReal CLI. A
The SIGPIPE-trapping variant of the same hook pins the mechanism exactly: on The same A/B at the
2. Why — what the hook process actually inherits
3. Input integrity5 MiB
Reproduced end-to-end through the real CLI too (1 310 720-char cumulative text: 4. The deadline survives the parentHook ignores On 5. Everything else held
Parent killed by Three things worth an explicit sign-off(a) Generic (b) (c) Cost per fire-and-forget hook execution. Mean over 20 runs of One robustness gapIf the temp directory is missing or unwritable, It degrades gracefully (a failed Test, lint and build status
Not coveredWindows (no Job Object / descendant-tree reclamation path could be exercised); the external- 中文版本地真实环境验证报告(Linux)— PR #10288我搭建了真实环境,并与 merge base 做了 A/B 对比。问题在 环境与方法
本 PR 只改动了一个源文件,因此"改前"通过 1. 问题复现与修复真实 CLI:
带 SIGPIPE trap 的同款 Hook 精确锁定了机制: 在 2. 原因:Hook 进程实际继承了什么Hook 运行期间的 3. 输入完整性5 MiB
真实 CLI 端到端同样复现(累计文本 1 310 720 字符: 4. 超时期限在父进程退出后仍然有效Hook 忽略 5. 其余行为保持不变
Hook 运行中用 建议明确确认的三点(a) 普通 (b) 三个事件的 (c) 每次 fire-and-forget Hook 执行的成本。 20 次 一个健壮性缺口当临时目录缺失或不可写时,这三个事件的 降级是优雅的(返回失败的 测试、Lint 与构建
未覆盖Windows(无法验证 Job Object / 后代进程树回收路径);PR 已声明超出范围的外部 |
Resolves a conflict with QwenLM#10288's fire-and-forget hook supervisor, which landed before this PR's windowsVerbatimArguments fix existed. Re-threads useVerbatimArguments through both spawn paths: the regular one, and the supervisor's own args array plus a new argv position so SURVIVING_HOOK_SUPERVISOR_SOURCE's internal spawn() also gets windowsVerbatimArguments. Adds a real-cmd.exe test for the supervisor path, which previously had no per-PR win32 coverage. Also extends the round-3 comment-detection fix to PowerShell (it uses # for line comments too) and adds newline to findCmdTokenEnd's delimiter set, so a bare placeholder followed by a line break doesn't absorb the next line into its quoted region.
|
Released in v0.23.0. |












What this PR does
This PR lets the output-ignored
MessageDisplay,StopFailure, andSessionDeletecommand hooks finish after the Qwen process exits without losing their input, deadline, or process-tree supervision. Qwen stages each hook input in a mode-0600 temporary file and launches an unreferenced, detached supervisor with parent-independent standard streams. The supervisor starts the actual hook, retains the configured timeout, and on POSIX keeps supervising the owned process group even if its root process exits before a descendant.Explicit AbortSignal cancellation is forwarded through the supervisor and still terminates the complete hook process tree. Generic asynchronous hooks continue to capture output through the in-memory registry and remain process-scoped.
The regression coverage now exercises explicit and natural parent exit for all three events, timeout after parent exit, abort forwarding, root-before-descendant exit, complete 5 MiB input delivery, preservation of a real hook exit code 124 before its deadline, supervisor isolation from hook NODE_OPTIONS, mode-0600 staging, and cleanup after normal use or synchronous supervisor spawn failure. The lifecycle design documentation records the supervisor, temporary-input, and untrappable host-failure boundaries.
Why it's needed
The parent-exit cleanup introduced by #10100 intentionally allowed
MessageDisplayhooks to outlive Qwen, but those hooks still inherited Qwen-owned output pipes. A hook that wrote after Qwen exited could therefore fail with a broken pipe instead of completing.StopFailurehad the same documented fire-and-forget contract but was not exempted from parent-exit cleanup, andSessionDeleteuses the same output-ignored dispatch contract.Making only stdout and stderr parent-independent was not sufficient: a queued stdin write could be truncated when Qwen exited, Qwen-owned timeout state disappeared with the parent, referenced child handles could prevent natural shutdown, and root-process close could clear supervision while a descendant remained alive. The independent supervisor preserves those lifecycle guarantees without changing ordinary synchronous or asynchronous hook behavior.
Reviewer Test Plan
How to verify
Run the HookRunner and hooks test suites from the Core package. Confirm that
MessageDisplay,StopFailure, andSessionDeletehooks let their driver exit explicitly or naturally before the hook completes, then still create their completion marker. Confirm that a hook which ignores SIGTERM is killed by its configured timeout after the driver exits, a surviving hook whose root exits before a descendant still times out the process group, a 5 MiB input remains complete and parseable, and AbortSignal cancellation still reclaims the hook tree. Confirm that a real hook which exits 124 before its deadline remains a normal non-zero hook result, user NODE_OPTIONS cannot alter the internal supervisor but is restored for the actual hook, staged input is mode 0600 and removed after use, and synchronous supervisor spawn failure removes its staged input. Also confirm that a genericasync: truehook tree remains process-scoped and is reclaimed on parent exit.Expected result: all hooks tests pass, the three fire-and-forget events retain independent input and lifecycle supervision, and other synchronous and asynchronous hooks preserve output capture and parent-exit cleanup.
Evidence (Before & After)
N/A — non-UI process lifecycle change.
Tested on
Environment (optional)
macOS Darwin 25.4.0 arm64; Node.js v22.22.3; npm 10.9.8. Core build and typecheck passed; all 776 hooks tests passed; changed-file ESLint, Prettier, and diff checks passed.
Risk & Scope
Linked Issues
Follow-up to #10100.
中文说明
本 PR 的改动
本 PR 让忽略输出的
MessageDisplay、StopFailure和SessionDelete命令 Hook 能够在 Qwen 进程退出后继续完成,同时不丢失输入、超时期限或进程树监督。Qwen 会将每个 Hook 输入暂存到权限为 0600 的临时文件,并启动一个不引用父事件循环、采用独立标准流的 detached supervisor。supervisor 负责启动真实 Hook、保留配置的 timeout,并在 POSIX 上持续监督所属进程组,即使根进程先于后代进程退出也不会提前结束监督。显式 AbortSignal 取消会转发给 supervisor,并继续终止完整 Hook 进程树。普通异步 Hook 仍通过内存注册表捕获输出并保持进程级生命周期。
回归测试现覆盖三个事件的显式和自然父进程退出、父进程退出后的 timeout、Abort 转发、根进程先于后代退出、完整 5 MiB 输入传递、在 deadline 前保留真实 Hook 的 124 退出码、隔离 supervisor 与 Hook 的 NODE_OPTIONS、0600 暂存权限,以及正常使用或 supervisor 同步启动失败后的清理。生命周期设计文档记录了 supervisor、临时输入和不可捕获主机故障的边界。
为什么需要
#10100 引入的父进程退出清理有意允许
MessageDisplayHook 比 Qwen 存活更久,但这些 Hook 仍继承 Qwen 拥有的输出管道。因此,Hook 在 Qwen 退出后写入输出时可能因管道断开而失败,无法完成。StopFailure具有相同的 fire-and-forget 文档契约,却没有被排除在父进程退出清理之外;SessionDelete也使用相同的忽略输出分发契约。仅让 stdout 和 stderr 独立于父进程仍不充分:Qwen 退出时,排队中的 stdin 写入可能被截断;由 Qwen 持有的 timeout 状态会随父进程消失;被引用的子进程句柄可能阻止自然退出;根进程关闭也可能在后代仍存活时提前清除监督。独立 supervisor 在不改变普通同步和异步 Hook 行为的前提下保留了这些生命周期保证。
审阅者测试计划
验证方法
在 Core 包中运行 HookRunner 和 hooks 测试套件。确认
MessageDisplay、StopFailure和SessionDeleteHook 允许驱动进程在 Hook 完成前显式或自然退出,之后仍能创建完成标记。确认忽略 SIGTERM 的 Hook 会在驱动进程退出后按配置 timeout 被终止;根进程先于后代退出的 surviving Hook 仍会对整个进程组执行 timeout;5 MiB 输入保持完整且可解析;AbortSignal 取消仍会回收 Hook 进程树。确认真实 Hook 在 deadline 前以 124 退出时仍返回普通非零结果;用户 NODE_OPTIONS 不能影响内部 supervisor,但会为真实 Hook 恢复;暂存输入权限为 0600 且使用后删除;supervisor 同步启动失败也会删除暂存输入。同时确认普通async: trueHook 进程树仍是进程级的,并会在父进程退出时被回收。预期结果:所有 hooks 测试通过,三个 fire-and-forget 事件保留独立输入和生命周期监督,其他同步与异步 Hook 继续保持输出捕获和父进程退出清理语义。
证据(前后对比)
N/A — 非 UI 的进程生命周期变更。
测试平台
环境(可选)
macOS Darwin 25.4.0 arm64;Node.js v22.22.3;npm 10.9.8。Core 构建和类型检查通过;全部 776 个 hooks 测试通过;变更文件的 ESLint、Prettier 和 diff 检查通过。
风险与范围
关联事项
#10100 的后续修复。