fix(vscode): shut the ACP CLI down gracefully instead of killing it - #11642
Conversation
disconnect() used to child.kill() the CLI: TerminateProcess on Windows (cleanup never runs, MCP/PTY/conhost children orphaned — the teardown half of #11303) and a bare SIGTERM on POSIX with no bound. Now stdin is closed so the CLI runs its own shutdown (SessionEnd hooks, MCP pool drain, session dispose, exit-time reaper), with a bounded escalation behind it: 45s grace sized from the CLI's own drain budgets, then a process-group SIGTERM (catchable, runs the signal cleanup), then SIGKILL after 10s. Windows goes straight to a taskkill tree kill once the grace expires, since console processes have no catchable terminate. Replacing the current session (session/new, session/load) now also closes the superseded one — a retained session keeps firing autonomous turns when its background tasks complete, which is what grew conhost.exe without bound in #11303. The close is conditional (onlyIfUnheld) with an 8s drain budget, and refused/failed closes retry on a 60s doubling backoff capped at 1h, so in-flight work is never dropped and the leak stays tracked. The CLI side of that close path gets the matching concurrency idempotency: a SIGTERM landing mid-ide_close now joins the in-flight MCP pool drain and session dispose instead of running them twice. Superseded connections can no longer tear down the live one: child handlers bind their own child, inbound callbacks and stale request resolutions check against the connection they were wired on. Refs #11510 #11511 #11303
efe1160 to
d3eb393
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）为单个提交。 |
|
Re-run on head Template ✓ — every required section filled in, Tested-on table honest about what was and was not run, complete Chinese translation. Problem — observed, not theoretical. #11303 reports 347 leaked headless On the duplicate gate: Direction — aligned. A first-party surface leaking hundreds of processes and gigabytes is squarely in scope, and the fix sits at the right layer: the Companion's own teardown, not a compensating workaround inside the CLI. CHANGELOG has no direct reference, but the area is relevant — this is the same ACP teardown surface #11510 was filed against. Size — core paths are touched (
319 production lines is under the 500-line escalation threshold and well under the 1000-line advisory, and the title is Approach — scope feels right and I could not find a materially simpler path. Closing stdin and letting the CLI's existing
Risk — Stage 1e matches two high-risk paths, the strongest triage-time revert signal this repo has: Moving on to code review. 🔍 中文说明在 head 模板 ✓ —— 所有必填章节都写全了，Tested-on 表格如实区分了跑过与没跑过的部分，中文翻译完整。 问题 —— 是已观测到的问题，不是理论性加固。#11303 报告 Companion 运行约 12 小时后泄漏 347 个 headless 关于重复检查这条闸门： 方向 —— 对齐。一方自有界面泄漏数百个进程和数 GB 内存，完全在职责范围内；修复也落在正确的层次：Companion 自己的退出流程，而不是在 CLI 里加补偿性兜底。CHANGELOG 没有直接对应条目，但这个领域是相关的 —— 它与 #11510 针对的是同一片 ACP 退出路径。 规模 —— 触及核心路径（
319 行生产代码低于 500 行的升级阈值，也远低于 1000 行的大 PR 建议线；标题是 方案 —— 范围合理，我也没找到明显更简的路径。关闭 stdin、让 CLI 已有的
风险 —— Stage 1e 命中了两个高风险路径，这是本仓库最强的 triage 期回滚相关信号： 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo Critical, no Major. Both findings the previous run deferred on are fixed at head, and I re-derived each against the source rather than accepting the commit messages. I wrote my own proposal from the title and "Why it's needed" before opening the diff: close stdin, bounded grace, then a catchable SIGTERM to the process group and SIGKILL, The two prior findings, verified fixedThe Major is gone, fixed by exactly the route named last round. if (this.sdkConnection !== conn) {The The missing fixture is added, and it is the right one. New code since the last reviewFour substantive changes landed in the six commits, and all four hold up. The Removing The exit-handler guard went from truthiness to identity. Base used
One outstanding blocking review rests on a premise the current code contradictsThe standing Two other items from that review, for the record. The Prettier failure at The separate ≥120s worst case derived last round is a different question and still stands — Non-blockingThe grace and kill timers in sequenceDiagram
participant P1 as VS Code Companion
participant P2 as AcpConnection
participant P3 as ACP CLI child
participant P4 as Hook system
participant P5 as MCP pool
P1->>P2: disconnect
P2->>P2: capture child, clear child and sdkConnection and sessionId
P2->>P3: stdin end (EOF)
P2->>P2: arm 75s grace timer
P3->>P3: connection.closed resolves
P3->>P4: fireSessionEndOnce, shared promise, concurrent, 30s abort budget
P4-->>P3: cancelled hooks resolve, detected via signal.aborted
P3->>P5: drainPoolBeforeExit, memoized, 8s
P3->>P3: disposeSessionsOnce then runExitCleanup, memoized
P3-->>P2: exit event
P2->>P2: exit listener clears both timers
Note over P2,P3: only if no exit within 75s
P2->>P3: SIGTERM to process group, negative pid
Note over P2,P3: only if still alive after another 75s
P2->>P3: SIGKILL to group, Windows uses taskkill f t by absolute path
Files changed (9 of 9 shown)
TestingThis is an unattended CI-path run ( CI on this head is still running, with zero failures so far. Across 28 check-runs on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Two things about that table matter for the verdict. A sandboxed verification run is in flight but has produced no verdict for this head. Comment What neither CI nor static review can settle:
Sandboxed verification would settle the remaining gap: The author's reported 26/26, 709/709 and 10/10 counts are the author's claim; I did not re-run them, and on this head no sandbox report has confirmed them either (the prior confirmation was for 中文说明代码审查没有 Critical，也没有 Major。 上一轮 defer 的两个发现在 head 上都已修复，我是逐条对着源码重新推导的，没有采信 commit message。 我在打开 diff 之前只根据标题和"为什么需要"写了自己的方案：关闭 stdin、有上限的宽限、然后向进程组发可捕获的 SIGTERM 再到 SIGKILL、Windows 上用绝对路径 上一轮两个发现，已核实修复Major 已消除，且用的正是上一轮点名的那条路径。 缺失的 fixture 补上了，而且补对了。 上一轮之后的新代码六个 commit 里有四处实质改动，四处都站得住。
删除 退出处理器的守卫从真值判断变成了身份判断。 base 是
有一条仍在生效的阻塞评审，其前提与当前代码相矛盾
该评审的另外两项，一并记录。 上一轮推导出的 ≥120 秒最坏情况是另一个问题，仍然成立 —— 非拦截项
测试本次是无人值守的 CI 路径运行（ 该 head 上的 CI 仍在运行，目前零失败。 表格中有两点对结论有影响。 沙箱验证运行在飞，但该 head 上还没有结论。 评论 CI 与静态评审都不能定的事：
沙箱验证可以定下剩下的缺口： 作者自报的 26/26、709/709、10/10 是作者的主张；我没有重跑，而在当前 head 上也没有沙箱报告确认过它们（此前的确认是针对 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — both conditions the previous run set are met and I verified them against the head source; what remains is a platform nobody can automate and a handful of hygiene nits, none of which should gate a PR at this round count. The last run deferred rather than rejected, and it said precisely what would move it to approve with no further conditions: fix the Major by either of two named routes, and add the one missing fixture. Both happened. The Major was fixed by the route I would have picked — dropping the same-session clause and keeping connection identity — and checking it against the merge-base rather than the previous commit made me more comfortable than the fix alone warrants, because base had no guard in Going back to my independent proposal: we converged, which is a good sign but also means I am not the sharpest possible check on this design — so I spent the review looking for places where a design this plausible could still be wrong at runtime, and for claims in the thread that the current code no longer supports. Both searches paid off. The runtime search turned up something I would otherwise have read past. Base's exit handler gated on The claim search found one that matters. A Would I maintain this in six months? Yes. The memoization pattern is the right shape, the identity guards are consistent rather than ad hoc, the comments explain why (the new Is it needed? Unusually clearly. #11303 is 347 leaked processes and ~2.8 GB. Where I land: approve, deferred until CI is green. I am not posting the approval in this run because The guardrail computation is clean — not a cross-repository PR, and the title is What I am not treating as resolved, so nobody reads this approval as broader than it is:
One process note. A deferral to Recorded so it does not get lost, unchanged from last run: SessionEnd hooks do not fire at all when a session is live — symmetric across head and base with no PR code in the loop, so pre-existing and not this PR's defect. It qualifies the headline benefit, because a live conversation is exactly the state a user is in when they close the panel. Worth its own issue. 中文说明Confidence: 4/5 —— 上一轮设定的两个条件都已达成，且我是对着 head 源码核实的；剩下的是一件谁都无法自动化的平台验证，加上一把卫生类小问题，在本 PR 已走过的轮次下都不该成为门禁。 上一轮是 defer 而不是否决，并且明确写出了"满足即可批准、不附加新条件"的两件事：按两条点名路径之一修掉 Major，并补上那个缺失的 fixture。两件都做了。Major 用的正是我会选的那条路径 —— 删掉 same-session 子句、只保留连接身份 —— 而对着 merge-base（而不是上一个 commit）核实这件事，让我比这个修复本身更安心：base 的 回到我独立提出的方案：我们收敛了。这是好迹象，但也意味着我不是对这个设计最锋利的那道检验 —— 所以我把评审时间花在两处：找"设计这么合理、运行时仍可能是错"的地方，以及找讨论串里当前代码已不再支持的说法。两处都有收获。 第一处翻出了一个我本来会读过去的东西。base 的退出处理器用 第二处翻出一条要紧的。本 PR 上仍挂着一张 半年后我来维护会不会骂作者？不会。记忆化的写法是对的形态，身份守卫是一致的而不是临时补丁，注释解释的是为什么（新增的 这件事有没有必要做？必要得很清楚。#11303 是 347 个泄漏进程和约 2.8 GB 内存。 我的结论：批准，但推迟到 CI 转绿。 本轮我不发出这个批准，因为 闸门计算是干净的 —— 不是跨仓库 PR，标题是 以下是我不当作已解决的，以免有人把这个批准读得比它实际更宽：
一条流程说明。本 PR 早先在 head 有一件事记在这里以免丢失，与上一轮一致：当 session 处于活跃状态时 SessionEnd hook 完全不触发 —— head 与 base 对称、回路中没有本 PR 代码，所以是既有问题、不是本 PR 的缺陷。它限定了本 PR 的宣称收益，因为用户关闭面板时所处的正是一个活跃会话。值得单独开一个 issue。 上方正文末尾的机器可读标记表示：批准已就绪，待本 head 的 CI 全绿后由 finalize 流程自动发出。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
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
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 the suite ran only on Linux locally, so this diff's Windows-only escalation branch (the taskkill /f /t rung and the non-detached spawn) was never executed on Windows.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": whether the 45 s wind-down lets a retired CLI and its replacement overlap on shared workspace state (the old process's drainPoolBeforeExit('ide_close') / sess…; "agent reverse-audit (round 1)": whether Readable.toWeb() on an already-converted Node Readable throws or silently double-subscribes — I reasoned from the 'data' -listener semantics without …; "agent reverse-audit (round 1)": I did not open WebViewProvider.ts:1341/1480/1495/1606/2608/2905 , so the concrete production caller pair that could overlap two connect() calls is unconfirme…; "agent reverse-audit (round 2)": did not read withLiveSessionRestore (acpAgent.ts:5400+) to confirm whether the live-restore path replaces the this.sessions entry or reuses the captured obj…; "agent reverse-audit (round 2)": did not execute npx vitest run src/services/acpConnection.test.ts in packages/vscode-ide-companion , so the suite's green state and the fake/real-timer inter…, and 3 more.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): src/services/acpConnection.test.ts — no such file or directory; src/acp-integration/acpAgent.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the suite ran only on Linux locally, so this diff's Windows-only escalation branch (the taskkill /f /t rung and the non-detached spawn) was never executed on Windows.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 3)"：whether the 45 s wind-down lets a retired CLI and its replacement overlap on shared workspace state (the old process's drainPoolBeforeExit('ide_close') / sess…；"agent reverse-audit (round 1)"：whether Readable.toWeb() on an already-converted Node Readable throws or silently double-subscribes — I reasoned from the 'data' -listener semantics without …；"agent reverse-audit (round 1)"：I did not open WebViewProvider.ts:1341/1480/1495/1606/2608/2905 , so the concrete production caller pair that could overlap two connect() calls is unconfirme…；"agent reverse-audit (round 2)"：did not read withLiveSessionRestore (acpAgent.ts:5400+) to confirm whether the live-restore path replaces the this.sessions entry or reuses the captured obj…；"agent reverse-audit (round 2)"：did not execute npx vitest run src/services/acpConnection.test.ts in packages/vscode-ide-companion , so the suite's green state and the fake/real-timer inter…，另有 3 条。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
Test Plan（非阻断）：src/services/acpConnection.test.ts — no such file or directory; src/acp-integration/acpAgent.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.23.3)
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES
已核对 head d3eb3930d5481999bef3a6473e6955bb27bc47ea（vs origin/main merge-base）。required CI 全绿（Test (ubuntu-latest, Node 22.x)、Lint & Static、Integration Tests (no-AK, No Sandbox)、web-shell E2E Smoke 均 success），所以以下阻塞全部来自代码本身。方向（用 stdin 收尾代替 child.kill()）是对的，问题在两档时序的算术与新增幂等的错误语义。
阻塞 1：两档 grace 覆盖不到唯一会走到第二档的情形，兜底 SIGKILL 会把本 PR 要修的泄漏重新引入
acpConnection.ts:64 SHUTDOWN_GRACE_MS = 45_000、:74 SIGTERM_GRACE_MS = 10_000。我在该 head 逐项核对了 CLI 侧的真实预算：DEFAULT_HOOK_TIMEOUT = 60000（packages/core/src/hooks/hookRunner.ts:46）、SESSION_DRAIN_TIMEOUT_MS = 30_000（acpAgent.ts:507）、await agentInstance?.shutdownMcpPool(8_000)（acpAgent.ts:2928）、OVERALL_CLEANUP_TIMEOUT_MS = 5_000（packages/cli/src/utils/cleanup.ts:36）。
关键是执行顺序与闩锁：ide_close 路径 await fireSessionEndOnce(SessionEndReason.PromptInputExit)（acpAgent.ts:3154）先跑，且 fireSessionEndOnce 是布尔闩锁（acpAgent.ts:2965-2966：if (sessionEndFired) return; sessionEndFired = true;），并按每个活跃会话的 Config 顺序 await hookSystem.fireSessionEndEvent(...)。所以只要有一个受支持的默认超时 hook 跑到 45s 以上：
- t=45s 扩展发 SIGTERM →
shutdownHandler（acpAgent.ts:3103-3106）里的fireSessionEndOnce因闩锁立即返回，随后disposeSessionsOnce()、drainPoolBeforeExit('signal')、runExitCleanup()全部从零开始，最坏 30+8+5 = 43s； - t=55s 扩展
process.kill(-childPid, 'SIGKILL')（不可捕获）→process.on('exit')与退出期清扫不执行；按本 PR 自己的注释（acpConnection.ts:186-193），组信号到不了调用过setsid()的后代（node-pty 会话、ConPTY host、detached hook supervisor）。
结果：第二档唯一会触发的场景，恰好是「CLI 还欠 43s 而只剩 10s」的场景，SIGKILL 落在收尾中途，把 #11303 的 PTY/子进程泄漏重新带回来，还可能截断 closeStoredSession 里的 recorder.flush()。SIGTERM_GRACE_MS 的注释前提（「hook 与 drain 要么已跑完，要么会加入正在进行的 ide_close 那一轮」）在这条路径上不成立。
修复预期（任选其一，且两档必须用同一套算术）：把第二档按信号路径自身上界来定（≈45s）并改正注释；或让 fireSessionEndOnce 交回进行中的 hook promise，使信号路径 join ide_close 而不是重跑；或按 #11510 的建议在 ide_close 路径上给 SessionEnd hook 设预算上限，让 45s 真的站得住。同时请更正 PR/issue 记录：把 grace 从「来历不明的常数」改成推导是对的方向，但推导漏掉了 hook 这一项，不等于该项已解决。
阻塞 2：drainPoolBeforeExit 的幂等改变了 strict/lax 语义，新的失败路径会让下一步被跳过
acpAgent.ts:2919-2936 现在把第一次调用的 promise 记忆化，注释也写明「First call wins; later calls — including a stricter one — join it」。两个方向都有后果：
- strict 档失效：
await drainPoolBeforeExit(label, true)（acpAgent.ts:3061）若加入一个由'signal'/'ide_close'（非 strict，内部catch后吞掉错误）先建好的 memo，就永远拿不到失败，failures.push(error)不会发生，AggregateError('Managed ACP shutdown failed')也不会抛出 —— 抽取来的「strict 调用者要知道 drain 失败」这一契约被静默取消。 - 反向新增跳过路径：strict 首调失败时 memo 是 rejected，之后
await drainPoolBeforeExit('ide_close')（acpAgent.ts:3157）会抛出，于是acpAgent.ts:3158的await disposeSessionsOnce()被跳过（该try只有finally捕获器，错误直接逸出runAcpAgent）。改动前每个调用各自try/catch，非 strict 调用吞错后仍会继续 dispose。请给 memo 一个不会把首调的 strictness/失败外溢给后续 joiner 的形态（例如按 label/strict 记录并在 join 时重新判定，或让 memo 只吞错不传播）。
其余
本 head 上另有 R1-2、R1-4、R1-5、R1-6、R1-7、R1-8 六条 Critical 线程与 19 条 Suggestion 线程，全部处于未解决状态且尚无作者回应。我没有逐条替代验证（上面两条是我自己在代码上核出来的），它们不会被本次 Review 视为已解决：请逐条在当前 head 上修掉，或给出可核查的「为何不成立」回应。其中 R1-21 / R1-24 / R1-17 / R1-22 / R1-23 / R1-26 是「把兜底阶梯的分支删掉，用例仍全绿」一类 —— 对一个主题就是收尾阶梯的 PR，这些属于必要测试覆盖，请一并处理。
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
COMMENT
核对基线：head d3eb3930d5481999bef3a6473e6955bb27bc47ea（vs origin/main）。required CI 全绿（Test (ubuntu-latest, Node 22.x)、Lint & Static、Integration Tests (no-AK, No Sandbox)、web-shell E2E Smoke），Windows/macOS 测试 lane 为 skipped。以下阻塞全部来自代码本身。
方向没有问题：用关闭 stdin 让 CLI 自行收尾、代替 child.kill()，是正确的做法。阻塞点在两档 grace 的算术，以及新增幂等的 strict 语义。
一、历史阻塞问题在当前 head 上仍然存在
本 PR 现有 27 条 review thread 全部 isResolved: false，其中 6 条 Critical（R1-2、R1-4、R1-5、R1-6、R1-7、R1-8）；另有两条 CHANGES_REQUESTED（2026-09-11T14:14:59Z、2026-09-11T16:00:54Z）。head commit 的提交时间是 2026-09-11T09:06:45Z，早于这两条 review，也就是说这些意见提出之后代码没有再动过，因此它们在当前 head 上原样成立，无法视为已修复。
二、当前 head 上可证明的 Critical：第二档 grace 覆盖不到唯一会触发它的情形
acpConnection.ts:64 SHUTDOWN_GRACE_MS = 45_000、:74 SIGTERM_GRACE_MS = 10_000。SIGTERM_GRACE_MS 的注释前提是「SessionEnd hook 与 MCP pool drain 要么已经跑完，要么会加入正在进行的 ide_close 那一轮」，这个前提在实际代码里不成立：
acpAgent.ts:2961-2966的fireSessionEndOnce是布尔闩锁（if (sessionEndFired) return; sessionEndFired = true;），不是 promise 记忆化，因此信号路径拿不到 ide_close 那一轮正在进行的 hook promise，只是直接跳过。- 两条路径的顺序相反：ide_close 是
fireSessionEndOnce(3154) →drainPoolBeforeExit('ide_close')(3157) →disposeSessionsOnce()(3158)；信号路径是fireSessionEndOnce(3105) →disposeSessionsOnce()(3106) →drainPoolBeforeExit('signal')(3120) →runExitCleanup()。 - 于是当 SIGTERM 落在 ide_close 仍在跑 hook 的阶段时，drain 与 dispose 两个 memo 都还不存在，信号路径会从零开始跑完整套收尾：
SESSION_DRAIN_TIMEOUT_MS = 30_000（acpAgent.ts:507）+shutdownMcpPool(8_000)+runExitCleanup()（OVERALL_CLEANUP_TIMEOUT_MS= 5s）≈ 43s，而留给它的只有 10s。 - hook 跑到 45s 以上完全合法：
DEFAULT_HOOK_TIMEOUT = 60000（packages/core/src/hooks/hookRunner.ts:46），单个 SessionEnd hook 就可以超出第一档 grace。 - 到点后
acpConnection.ts:1084执行process.kill(-childPid, 'SIGKILL')，不可捕获，process.on('exit')与退出期回收器都不会执行；而按本 PR 自己的注释（acpConnection.ts:186-193），组信号到不了调用过setsid()的后代（node-pty 会话、ConPTY host、detached hook supervisor）。
结论：第二档唯一会被触发的场景，恰好是「CLI 还欠约 43s 而只剩 10s」的场景，SIGKILL 落在收尾中途，把 #11303 的 PTY/子进程孤儿泄漏重新带回来，并可能截断 closeStoredSession 里的 recorder.flush()。
可行的修法（任选其一，两档必须用同一套算术）：把第二档按信号路径自身的上界定（≈45s）并改正注释；或让 fireSessionEndOnce 交回进行中的 hook promise，使信号路径真正 join ide_close 而不是跳过；或按 #11510 的思路给 ide_close 路径上的 SessionEnd hook 设预算上限，让 45s 站得住。
三、drainPoolBeforeExit 的幂等改掉了 strict/lax 语义
acpAgent.ts:2919-2936 把首次调用的 promise 记忆化，strict 只在首次调用时被闭包捕获，注释也写明「First call wins; later calls — including a stricter one — join it」。后果是：shutdownManagedAgent 里 await drainPoolBeforeExit(label, true)（acpAgent.ts:3061）如果加入的是一个由非 strict 调用建立的 memo，就永远拿不到失败，failures.push(error) 不会发生，AggregateError('Managed ACP shutdown failed') 也不会抛出——「strict 调用者需要知道 drain 失败」这个契约被静默取消。反方向同样有风险：strict 首调失败时 memo 是 rejected，后续 await drainPoolBeforeExit('ide_close')（3157）会抛出，使 3158 的 await disposeSessionsOnce() 被跳过（该 try 只有 finally，错误直接逸出 runAcpAgent）。
这一项我确认了代码结构成立，但两个方向都要求 isTrustedManagedParent() 在两次调用之间取值不同（strict 调用点只在 managed 分支，非 strict 调用点只在 else 分支），我没有在本次核对中证明该翻转可达。请按可达性给出结论：要么让 memo 不把首调的 strictness/失败外溢给后续 joiner（例如按 label/strict 分别记录并在 join 时重新判定），要么补一条说明为何不可达。
四、下一步
第二节是本轮唯一新核出的、可在当前 head 上用代码证据证明的 Critical，请先处理；第一节的历史 Critical 线程需要逐条在当前 head 上修掉，或给出可核查的「为何不成立」回应。其中 R1-21 / R1-24 / R1-17 / R1-22 / R1-23 / R1-26 属于「删掉兜底阶梯分支后用例仍全绿」一类，对一个主题就是收尾阶梯的 PR 是必要覆盖，建议一并处理。
|
Resolution summary:
|
|
Audit follow-up (latest head I re-checked the implementation against the linked issue requirements instead of treating resolved threads as proof. One real boundary remained and is now fixed: Verification on this head:
Scope remains explicit: this addresses the ACP CLI shutdown/session-replacement portions related to #11510/#11511 and the ACP limb of #11303. It does not claim to close all of #11303: inbox/web-terminal paths, upstream node-pty#965, and real Windows ConPTY accounting remain outside this macOS/Linux validation. The PR description has also been corrected to the 75s/75s timings and current test counts. |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 57 passed · 2 failed · 59 total Flakiness gate: 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：57 通过 · 2 失败 · 59 总计 抖动门： Verification reportPR #11642 deep verification —
|
| Central claim | disconnect() makes the CLI shut itself down (stdin EOF → connection.closed) instead of being killed, and the bounded escalation ladder reaches the CLI's process tree rather than only its root. |
| Secondary claim 1 | The CLI-side wind-down is bounded (30s SessionEnd abort), so the ladder stays a backstop and never lands mid-shutdown. |
| Secondary claim 2 | Superseded-session close is conditional (onlyIfUnheld), never drops active work, backs off, treats unsupported as terminal, and never blocks the replacement session. |
| Out of scope (listed under Not covered) | Windows, managed-parent shutdown, per-commit attribution, repo-wide gates, real VS Code extension host. |
Central claim + A/B
H1 — real CLI, real wire, only AcpConnection differs
Base cell = AcpConnection compiled from tmp/base-tree (HEAD^1); head cell = the same file at HEAD. Both bundles were produced by bundle.mjs (esbuild, vscode aliased to a stub — the only stub in the loop; the unit under test and the CLI are real). Control cleanliness: the PR touches no package.json/lockfile, and the base bundle resolved zero head-tree workspace modules (83 modules vs head's 84; the extra one is acp-bridge/dist/bridgeTypes.js, which only head imports). readlink -f of the shared internal dependency confirmed @qwen-code/acp-bridge → /__w/qwen-code/qwen-code/packages/acp-bridge, i.e. the head tree — acceptable here because base does not import it and the PR does not modify it.
| oracle | BASE (HEAD^1) |
HEAD (PR) |
|---|---|---|
child pgid == child pid (ps -o pgid) |
20181 != 20191 → not detached |
20160 == 20160 → group leader |
| SessionEnd reason, written by the CLI itself | other |
prompt_input_exit |
| ⇒ which CLI shutdown path ran | shutdownHandler — signalled from outside |
await connection.closed — wound itself down |
| exit code / signal | 0 / null |
0 / null |
ms from disconnect() to child exit |
30 | 34 |
Flip: 1/1. The reason field is the CLI's own output, not an inference, and it is the discriminator: on POSIX both arms end at code=0, so exit status alone cannot tell them apart. Witness: 01-h1-central-ab-shutdown-path-flips.png. Raw logs: logs/h1-base.log, logs/h1-head.log.
H2 — does teardown reach the process tree?
Peer = a real @agentclientprotocol/sdk AgentSideConnection over real ndjson pipes, which ignores stdin EOF (a wedged CLI — H1/H5 show a healthy one always exits in tens of ms, so this is the only situation the ladder exists for) and spawns a non-detached grandchild, exactly as the CLI spawns MCP stdio servers. Precondition measured, not assumed: grandchildPgid == peerPgid and peerIsGroupLeader == true.
| cell | stdin EOF | peer signalled | grandchild signalled | grandchild at end |
|---|---|---|---|---|
| head / wedged | +1 ms | SIGTERM @ 75060 ms | SIGTERM @ 75060 ms | dead (reaped) |
| base / wedged | never | SIGTERM @ 0 ms | never | ALIVE (orphan) |
| head / cooperative | +1 ms | never | never | alive † |
| base / cooperative | never | SIGTERM @ 0 ms | never | alive † |
| head / ignores SIGTERM | +1 ms | SIGTERM @ 75060 ms | SIGTERM @ 75060 ms | dead; peer exit=SIGKILL @ 152146 ms |
† The stand-in peer does not reap its own child, so its grandchild outlives it in both arms — a property of the fixture, not of the PR. H4 measures the same question against the real CLI and shows hook children reaped on both arms (hookChildReaped: true). The cooperative head row is still load-bearing for a different claim: no signal was ever sent, i.e. escalation is correctly suppressed when the graceful path worked.
Flip: 1/1 on the wedged rows — base orphans the grandchild, head reaps it. The last row also proves the second rung: SIGKILL lands ~75 s after SIGTERM. Witness: 02-h2-ladder-orphan-vs-reaped.png.
H5 — the premise behind the 75 s grace
Fixture: a SessionEnd command hook that blocks 600 s with its own timeout at 90 s, so the only thing that can cut it at 30 s is the AbortController this PR adds. Control = the head build with one line reverted in the compiled output (packages/cli/dist/src/acp-integration/acpAgent.js: fireSessionEndEvent(reason, hookAbortController.signal) → fireSessionEndEvent(reason)), which is exactly the base call shape; restored afterwards and verified.
| build | CLI exits after disconnect() |
blocking hook child |
|---|---|---|
| head (signal passed) | 30074 ms | SIGTERM at 29971 ms |
| control (signal arg reverted) | 75080 ms — overruns the 75 s grace | no SIGTERM record; killed only at CLI exit |
So the abort is load-bearing in the strongest sense: without it the CLI's wind-down exceeds the companion's grace and the escalation fires mid-shutdown — precisely the outcome the new comment says must not happen. It also proves the signal is not merely observed between hooks: the hook's child process is actually terminated at ~30 s. With a 10-minute hook timeout the 30 s cap holds on both companion arms (30076 ms head / 30075 ms base), confirming it is CLI-side and reason-independent. Witness: 03-h5-30s-abort-keeps-cli-inside-grace.png.
H4 — the realistic case (a live session), real CLI
newSession succeeded against the real CLI without credentials, so teardown was measured with an actual session held:
| head | base (control) | |
|---|---|---|
| SessionEnd reason | prompt_input_exit ×2 (both configs) |
other ×2 |
| exit code / ms | 0 / 141 |
0 / 136 |
| hook children reaped | true | true |
The reaper row matters: it disproves the worry that routing teardown through connection.closed instead of SIGTERM would skip cleanup. See Corrections.
Corrections
These are corrections to the description, not requests to change code.
-
"Before:
disconnect()killed the CLI directly, so the CLI's shutdown and exit-time reaper could not run." True on Windows (TerminateProcess); not true on POSIX, and POSIX is what this round could measure.child.kill()there is a catchable SIGTERM that the CLI already handles with the same bounded budgets. Measured (H1/H4): base exitscode=0in 30–136 ms, fires SessionEnd, and reaps its hook children. The user-visible orphaning the PR cites is a Windows fix; the measured POSIX deltas are (a) which shutdown path and SessionEnd reason runs, and (b) the group-signal ladder for a wedged CLI (H2). -
"73s bounded = 30s SessionEnd + 8s MCP drain + 30s session drain + 5s exit cleanup" understates the real bound, and the grace is below it. All four constants exist, but the third is a per-phase budget applied three times sequentially, and the fourth does run — just not from where the comment implies.
- 30 s SessionEnd: real (
acpAgent.ts:2987, oneAbortControllershared across all configs) and measured at 30074 ms (H5). - 8 s MCP drain: real (
shutdownMcpPool(8_000)), a true wall-clock deadline. - "30 s session drain": not a total.
SESSION_DRAIN_TIMEOUT_MS = 30_000(:507) is the defaultdrainTimeoutMsincloseStoredSession(:4462), and it is spent on three sequential phases —beginSessionCloseAfterCurrentGate(:4463),waitForSessionDrain(:4502), and therunExclusiveHistoryMutationqueue wait (:4585).disposeSessions()passes no explicit timeout, so it inherits all three: ≥90 s, not 30 s. The code itself concedes the tail is open —:4581: "The mutation body itself stays untimed, so the guarantee remains approximate" — and that body awaitsrecorder.finalize/flush/close,config.unregisterSessionRegistry()andconfig.shutdown(), none of which are timed.closePeerMessaging()andcleanupUnstoredConfigare untimed too. Sessions drain in parallel underPromise.allSettled, so this is per-shutdown, not per-session-multiplied. - 5 s exit cleanup: does run on the
ide_closepath — from the caller,llm.tsx:1164-1168(finally { await runExitCleanup(); } process.exit(0);), not fromrunAcpAgent. The new CLI tests assertingrunExitCleanupwas not called are correct, because they invokerunAcpAgentdirectly and never reach that wrapper.
Real worst case is therefore >136 s and partly unbounded, against
SHUTDOWN_GRACE_MS = 75_000. See Finding 3 — the comment's "keep a small margin above that bound" is inverted. - 30 s SessionEnd: real (
-
"refused closes retry with a 60s doubling backoff capped at 1h." Refusals do not double:
scheduleSupersededCloseRetry(sessionId, true)resetsfailuresto 1, so every refusal re-arms the 60 s rung (measured over four consecutive refusals:failures1,1,1,1; delays 59950/59846/59847/59845 ms). Only errors double (60 s → 120 s → 240 s, measured). The reset is defensible — it matchesACTIVE_WORK_CLOSE_RETRY_GRACEsemantics inbridgeTypes.ts, where a child answering a probe either way resets the count — but the description says the opposite. Related: the test namedbacks off exponentially while a superseded close keeps being refusedasserts a constant 60 s rung (probe 3 fires at t=120 s, which exponential backoff would put at t=180 s), and its own inline comment says so. The name buys coverage confidence the fixture does not pay for. -
Reviewer Test Plan counts.
acpConnection.test.tsis 53/54 on Linux, not 54/54 (the plan's own "Tested on" table does flag Linux as unverified).acpAgent.test.tsis 711/711, not 709/709. -
The
runExitCleanupasymmetry insiderunAcpAgentis real but harmless, and the reaper does run. The new CLI tests assertrunExitCleanupis not called on theide_closebranch and is on the signal branch. That is accurate as far as it goes, but it is not the whole path:llm.tsx:1164-1168wraps therunAcpAgentcall infinally { await runExitCleanup(); }followed byprocess.exit(0), so on a graceful stdin-close shutdown the cleanup registry is drained and the process is force-exited. Separately, the reapers the PR body names (PTYs, ConPTY hosts, tracked children) are not in that registry at all — they are process-levelprocess.on('exit')handlers (ShellExecutionService.cleanup()static block;hookRunner.ts:361for hook child groups), which fire on the natural exit thatllm.tsx:1168triggers. H4 confirms this empirically:hookChildReaped: trueon both arms. So routing teardown through stdin close loses no reaper on POSIX — the loss the PR describes is Windows-only, whereTerminateProcessskips everyexithandler.
Findings
1. The PR's own new test deadlocks, so the suite is red and the guard it pins is uncovered — Blocking
packages/vscode-ide-companion/src/services/acpConnection.test.ts:655, does not wire replacement streams into a retired startup.
cd packages/vscode-ide-companion && npx vitest run src/services/acpConnection.test.ts
# → Tests 1 failed | 53 passed (54)
# Error: Test timed out in 15000ms.
The test awaits the rejection before advancing the fake timers:
const setup = (conn as …).setupChildProcessHandlers();
const setupFailure = await expect(setup).rejects.toThrow(/failed to start|superseded/i);
conn.child = newChild;
await vi.advanceTimersByTimeAsync(1000);but setupChildProcessHandlers reaches its supersede throw only after await new Promise((resolve) => setTimeout(resolve, 1000)) (acpConnection.ts:258), and that setTimeout is faked. Neither side can proceed: a structural deadlock, not a timing margin, so it is platform- and speed-independent. It reproduces in isolation (logs/suite-companion-single-1.log: 1 failed | 53 skipped), which rules out shared-runner contention; the runner was loaded (average 38 on 64 cores) but that is not the cause. Every sibling test in the same file uses the correct order (…:760, …:831, …:839), so this is one inverted pair of lines.
Consequence beyond the red build. /failed to start|superseded/i at :675 is the only assertion in the file that reaches the this.child !== ownChild || ownChild.killed guard at acpConnection.ts:264 — a condition this PR changed (base was !this.child || ownChild.killed). Since the test cannot reach its assertions, that new guard has zero effective coverage:
| variant | result | log |
|---|---|---|
| head as shipped | 1 failed / 53 passed (54) — 15 s timeout | suite-companion.log |
| Mutation A: guard reverted to base semantics | 1 failed / 53 passed (54) — identical, so SURVIVED | mutation-A-guard-reverted.log |
| test fix only | 0 failed / 54 passed (54) in 64 ms | fix-B-test-fixed.log |
| Positive control C: guard reverted + test fix | 1 failed — AssertionError: expected ClientSideConnection{ …(1) } to be null at :684 |
mutation-C-guard-reverted-with-fix.log |
Control C is what makes the survivor meaningful: with the hang fixed, reverting the guard turns the test red on the assertion it was written for, not on a compile or fixture error. So the guard is correct and the test is right in intent — it just never runs. Witness: 04-mutation-matrix-guard-unpinned.png. All four runs used the same command; the working tree was restored and verified clean after each.
Measured fix (one line, + a comment)
).setupChildProcessHandlers();
// Not awaited yet: setupChildProcessHandlers' 1s settle is a fake timer,
// so the supersede rejection can only land after the advance below.
const setupFailure = expect(setup).rejects.toThrow(
/failed to start|superseded/i,
);
conn.child = newChild;
await vi.advanceTimersByTimeAsync(1000);
await setupFailure;
expect(conn.sdkConnection).toBeNull();Applied in a scratch copy and measured: suite 54/54 (16.07 s → 0.68 s; the whole 15 s was the hang); Mutation C then fails the intended assertion. expect(...).rejects returns a promise, so deferring the await changes nothing about what is asserted — setup's rejection is handled by the wrapper immediately, and no unhandled rejection appears.
2. isUnsupportedSupersededCloseError can never return true for a wire error — Major
packages/vscode-ide-companion/src/services/acpConnection.ts:606.
return (
(error instanceof RequestError && error.code === -32601) ||
(error instanceof Error && /method not found/i.test(error.message))
);The shipped SDK rejects a failed request with the raw JSON-RPC error object, not an error instance — node_modules/@agentclientprotocol/sdk/dist/acp.js, #handleResponse:
else if ("error" in response) {
pendingResponse.reject(response.error);
}So both instanceof tests are false for every error that arrives over the wire, and the method always returns false. Measured on a real wire by instrumenting the real method (logs/h3-asis-full.log, cell J):
{ "ctor": "Object", "isErrorInstance": false, "code": -32601,
"message": "method not found", "ownKeys": ["code","message"], "verdict": false }Failure scenario. An older CLI that does not implement qwen/control/session/close answers -32601. The classifier misses it, so the code takes the generic branch: scheduleSupersededCloseRetry(sessionId) puts the session in the retry table and re-probes at 60 s → 120 s → 240 s → … capped at 1 h, for the lifetime of the connection, logging [ACP] Failed to close superseded session: each time. That is exactly the "unsupported retry loop" the PR body says does not happen ("Unsupported close methods on older CLIs are treated as unsupported and are not retried"; "Older CLIs … continue the new session normally, without an unsupported retry loop"). Confirmed twice on the wire: cell E (peer with no extMethod, so the SDK itself produces -32601 "Method not found": qwen/control/session/close) and cell F (peer throwing new RequestError(-32601, 'method not found')) both left [["sess-1",{"failures":1,…}]] in the retry table.
Why the suite is green anyway. acpConnection.test.ts:1492, does not retry an unsupported close method on an older CLI, mocks the reject path with mockRejectedValue(new Error('Method not found')) — an Error instance whose message matches the regex. That shape never occurs in production. The test is not vacuous (it does fail if the classifier is removed); it is pinned to the wrong fixture.
Blast radius. One read site: the .catch in sendSupersededClose (:664). Nothing else calls the classifier, and no other PR behaviour depends on it. What it does not cause: the replacement session is unaffected (cells E/F both confirm sessionId === 'sess-2' and a working sendPrompt afterwards), and no work is dropped — the defect is an unbounded, pointless retry loop plus log noise, not data loss.
Measured fix — read the fields instead of testing the prototype
The same file already does this for exactly this reason in mapReadTextFileError (:520), so the shape is house style:
private isUnsupportedSupersededCloseError(error: unknown): boolean {
// The SDK rejects a failed request with the raw JSON-RPC error object
// (`pendingResponse.reject(response.error)`), not a RequestError instance,
// so read the fields rather than test the prototype — the shape
// mapReadTextFileError below already uses.
const fields =
typeof error === 'object' && error !== null
? (error as { code?: unknown; message?: unknown })
: undefined;
const message =
typeof fields?.message === 'string'
? fields.message
: error instanceof Error
? error.message
: String(error);
return fields?.code === -32601 || /method not found/i.test(message);
}Applied in a scratch copy, re-bundled, and driven through the same harness. Three results:
- Hostile fixtures go clean —
H3_BUNDLE=fixed node h3-superseded-close.mjs→ 38 pass / 0 fail (was 34/4). Cells E, F and both J checks flip. - Zero collateral — the other 34 checks pass with unchanged values, including the exact wire params (
onlyIfUnheld: true,requireFlush: true,drainTimeoutMs: 8000) and the refusal rung trace (59951/59849/59846/59848 ms vs 59950/59846/59847/59845 ms before). - Gates unchanged —
npx tsc --noEmitexit 0;npx eslint src/services/acpConnection.tsexit 0; suite counts identical to as-shipped (1 failed / 53 passed, the same pre-existing deadlock from Finding 1, so this fix neither fixes nor breaks it).
Both gates were proven live before being cited: planting export const __gateProbe: number = "…" produced error TS2322: Type 'string' is not assignable to type 'number', and a scratch file with 1 == "1" produced an eqeqeq warning; --format json confirms eslint actually processes acpConnection.ts (errorCount: 0), so the clean lint is a pass and not a no-match. Evidence: logs/gate-liveness.log, logs/h3-fixed.log.
This fix should ship with a test whose fixture is the real shape — mockRejectedValue({ code: -32601, message: '"Method not found": qwen/control/session/close' }) — otherwise the suite still cannot tell head from head-plus-fix along that axis.
Witness: 05-h3-unsupported-close-misclassified.png.
3. SHUTDOWN_GRACE_MS is below the CLI's own worst-case wind-down, so the ladder can fire on a shutdown that is progressing correctly — Suggestion (design), with a real consequence
packages/vscode-ide-companion/src/services/acpConnection.ts:51-63. The comment states its own contract: "This has to outlast the CLI's own wind-down, or the escalation lands in the middle of a shutdown that is progressing correctly and skips the process.on('exit') cleanup this teardown exists to protect … Keep a small margin above that bound." Per Correction 2 the bound is >136 s and partly untimed, while the grace is 75 s — the margin is negative, not small.
Failure scenario. A CLI holding a session whose active turn will not settle, or whose history mutation/config.shutdown() is slow, is still working correctly at t=75 s. The companion then sends a process-group SIGTERM. The CLI's shutdownHandler joins the in-flight work via the memoized promises this same PR adds (so nothing double-runs — that part is well designed), but the sequence now races toward the second rung: SIGKILL at t=150 s. SIGKILL skips every process.on('exit') reaper, which is the exact orphaning this PR exists to prevent. H5 already demonstrates the shape on a smaller scale: with the 30 s abort reverted, the CLI was cut off at 75080 ms by the escalation rather than finishing on its own.
Bounded honestly. This is a ceiling, not an expectation. disposeSessions() aborts all generation controllers before draining (:4765-4772), so the three 30 s phases should normally release in milliseconds — measured: a real CLI with a live session tore down in 141 ms (H4), and a pathological 600 s hook in 30074 ms (H5). I never observed a >75 s shutdown; the >136 s figure is derived from three read call sites plus the code's own admission that the mutation body is untimed. And the ladder is still strictly better than what it replaces: base signalled at t=0 unconditionally.
Two ways to make the comment true, either acceptable: raise the grace above the derived bound (and say what it was computed from), or reframe both comments as a best-effort backstop rather than a bound-derived margin. The second is cheaper and matches reality — but then the sentence "Keep a small margin above that bound" should go, because it is what a future maintainer will trust. The twin comment at :65-73 (SIGTERM_GRACE_MS) repeats the same 73 s derivation and needs the same edit.
Suggestions (non-blocking)
- Reuse the shared backoff helper.
acpConnection.ts:704re-implementsMath.min(BASE * 2 ** (failures - 1), CEILING)inline while already importing from@qwen-code/acp-bridge/bridgeTypes, which exportsactiveWorkCloseRetryDelayMs(failures)for precisely this. The two policies differ: the helper keeps the first failed probe immediate (failures <= ACTIVE_WORK_CLOSE_RETRY_GRACE → null, then exponentfailures - 2), the companion defers it 60 s. Measured:activeWorkCloseRetryDelayMs(1) === null,(2) === 60000, vs companion 60000, 120000. Neither is obviously wrong for this call site, but the helper's docstring exists so the ratio "cannot drift between call sites", and it now has.
Not covered
- Windows, entirely. The
taskkill /f /tbranch, theWINDOWS_TASKKILLabsolute-path resolution, and ConPTY accounting are unreachable here (process.platform !== 'win32'in this container). This is the platform where the PR's stated orphaning bug actually bites, so the highest-value limb of the central claim is unverified — the POSIX limb is proven instead. The PR's own table already flags this ("mocked tree-kill tests only"). - Managed / trusted-parent shutdown (
shutdownManagedAgent,isTrustedManagedParent()). A plain companion spawn is not a trusted managed parent, so that branch never ran. Itsstrict = truedrain and itsprocess.exitCode = 1-without-process.exit()tail are unexercised. I did check by reading that thestrict-vs-non-strictmemoization mix indrainPoolBeforeExitis unreachable (mode is fixed per process), so no finding — but that is a static argument, not a measurement. - Per-commit attribution. Shallow checkout:
git rev-list --count HEAD^1..HEAD^2returns1while$QWEN_VERIFY_CONTEXTlists 7 commits, andgit rev-parse --is-shallow-repositoryistrue. Only the aggregateHEAD^1..HEADdiff was verified. - Trial merge into current
main. The snapshot'sbaseRefOid(518f6795…) is not present locally (git cat-file -t→could not get object info) and there is no token/network, so the merge-ref base28df8b8ais what was tested. Whether this still merges cleanly onto today'smainis unknown. - Base-CLI worktree build.
npm run build -w packages/cliinsidetmp/base-treefailed on dependency resolution in files this PR does not touch (@lydell/node-ptyTS7016,@opentelemetry/*TS2307, all under../core/src/**). Proven environmental by A/A control: the identical command in aHEAD^2worktree failed with the identical first error (logs/aa-headtree-build.log). The CLI-side control therefore used the sanctioned alternative — reverting one line in the builtdist/output — rather than a base-CLI build. newSession/sendPromptstale-response guards. Only theloadSessionsupersede race was driven on a real wire (cells G, H). ThenewSessionandsendPrompt"connection superseded" throws at:769and:801were not exercised end to end.- Real VS Code. No extension host in this container: every companion cell drives the real compiled
AcpConnectionfrom a plain node process. Actual window close / reload / reconnect, and thedetached: truechild's fate when the extension host itself dies, are unmeasured. Reasoned but not tested: stdin EOF reaches the CLI either way, so it should still self-terminate. - The ladder against a real wedged CLI. H2's wedge is a stand-in peer. A real CLI could not be wedged past the grace — that is H5's result, and it is a good one, but it does mean the 75 s and 150 s rungs were measured against a synthetic peer only.
- Shape vs cause. H2 reproduces the shape of the orphaning the PR targets (a non-detached descendant in the CLI's group, surviving a root-only kill). It does not reproduce a real VS Code window close producing it.
- The >136 s worst case in Finding 3 is derived, not observed. I measured the 30 s SessionEnd term end to end (H5, 30074 ms) and the typical live-session teardown (H4, 141 ms); the ≥90 s session-drain figure comes from reading three sequential call sites of
SESSION_DRAIN_TIMEOUT_MSplus the code's own comment that the mutation body "stays untimed". I never produced a shutdown that actually exceeded 75 s, so Finding 3 is a ceiling analysis, not a reproduction. - Repo-wide gates. Only the two suites named in the test plan, plus companion
tsc/eslinton the changed file. Nonpm run build,npm run preflight, integration tests, or other workspaces' suites. previous-report.mdwas absent from the context directory, so this is a first round with no carried-forward findings.
Methodology
Everything ran in the CI verify container (node:22-bookworm, node v22.23.2, 64 cores, observed load average ~38 — shared runner) against the merge-ref checkout at HEAD = e4e37c8d, with npm ci/npm run build already completed by the job. The base side was a scratch git worktree at HEAD^1, removed after the A/B cells were captured; both arms' AcpConnection were compiled from source by bundle.mjs (esbuild) with only vscode aliased, and each bundle's resolved internal modules were printed and checked so no head-tree code could leak into the control. Harnesses h1-real-cli-ab.mjs, h2-ladder-ab.mjs, h3-superseded-close.mjs, h4-cli-budget.mjs drive the compiled connection over real pipes — H1/H4/H5 against the real packages/cli/dist/index.js --acp, H2/H3 against a real @agentclientprotocol/sdk agent peer that records every inbound request verbatim, so assertions are made on bytes the peer received and on files the CLI itself wrote (a SessionEnd command hook persisting its hook-input JSON, which is where the reason discriminator comes from). Mutation and fix candidates were applied to the working tree one at a time from /__w/_temp/pr11642-backup/, measured, and restored; git status --porcelain is empty and the dist revert was verified by grep. Per-cell stdout/stderr is in logs/, assert-results.mjs recomputes assertions.json from those logs, and print-cells.mjs renders the tables that scripts/verify-capture.mjs rasterised into evidence/.
Flakiness gate log
rounds=5 files=2 skipped=0
file packages/cli/src/acp-integration/acpAgent.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/acpAgent.test.ts
file packages/vscode-ide-companion/src/services/acpConnection.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/services/acpConnection.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/acp-integration/acpAgent.test.ts: PPPPP
packages/vscode-ide-companion/src/services/acpConnection.test.ts: FFFFF
verdict: consistent-fail
summary: 1 of 2 changed test file(s) failed identically in every round — deterministic, so CI owns that signal
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 1 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: F (exit 1)
--- output tail · round 1 · packages/vscode-ide-companion/src/services/acpConnection.test.ts ---
ns sdkConnection when process is alive�[32m 0�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdisconnect clears child, sdkConnection, and sessionId�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdisconnect is a no-op when there is no child�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdisconnect closes the CLI stdin instead of killing it (#11303)�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdoes not force-kill a child that failed to spawn�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdoes not end stdin that is already closed�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mhandles a synchronous stdin close failure�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdisconnect escalates stdin close → SIGTERM → SIGKILL on POSIX�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdisconnect does not escalate against a CLI that exited on its own�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdoes not force-kill a CLI that exits within the SIGTERM grace�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdoes not signal after exitCode or signalCode is observed�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mescalates through taskkill /t on Windows, not a bare kill�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mfalls back when taskkill cannot terminate the CLI tree�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22ma superseded child exiting does not tear down its replacement�[32m 1�[2mms�[22m�[39m
�[31m �[31m�[31m AcpConnection child exit cleanup�[2m > �[22mdoes not wire replacement streams into a retired startup�[39m�[33m 15002�[2mms�[22m�[39m
�[31m → Test timed out in 15000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22ma live child exiting clears the connection and fires onDisconnected�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22ma superseded connection stops dispatching inbound callbacks�[32m 7�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22ma re-connected replacement keeps the superseded connection muted�[32m 4�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdoes not stamp a superseded connection with a stale session id�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22ma re-connected replacement does not stamp the retired session id�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22ma re-connected replacement does not fire onEndTurn for a retired prompt�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdoes not fire onEndTurn when the prompt session is superseded in place�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22mnewSession closes the superseded session on the same connection�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22mnewSession does not close anything for the first session�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22mloadSession closes the superseded session on the same connection�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22mloadSession does not close when reloading the current session�[32m 0�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22msends the close conditionally so held work is refused, not dropped (#11511)�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22msuperseded close retry (#11511)�[2m > �[22mretries a refused superseded close on a backoff until it succeeds�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22msuperseded close retry (#11511)�[2m > �[22mbacks off exponentially while a superseded close keeps being refused�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22msuperseded close retry (#11511)�[2m > �[22mcancels the retry when the superseded session is loaded again�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22msuperseded close retry (#11511)�[2m > �[22mwaits for an in-flight close before loading that session again�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22msuperseded close retry (#11511)�[2m > �[22mdeduplicates concurrent close attempts for one session�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22msuperseded close retry (#11511)�[2m > �[22mcaps transient close retry backoff at one hour�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22msuperseded close retry (#11511)�[2m > �[22mstops retrying superseded closes after disconnect�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22msuperseded close retry (#11511)�[2m > �[22mclears and cancels an in-flight close when disconnect retires the connection�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22msuperseded close retry (#11511)�[2m > �[22mre-drives an expired close retry on the next session replacement�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22msuperseded close retry (#11511)�[2m > �[22mdoes not retry an unsupported close method on an older CLI�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection superseded session close (#11303)�[2m > �[22ma failed close does not fail the new session�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection onDisconnected callback�[2m > �[22mhas a default no-op onDisconnected handler�[32m 0�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection onDisconnected callback�[2m > �[22mallows setting a custom onDisconnected handler�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection lastExitCode/lastExitSignal�[2m > �[22minitializes exit info as null�[32m 0�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection extension notifications�[2m > �[22mparses end_turn reason and source�[32m 0�[2mms�[22m�[39m
�[31m⎯⎯⎯⎯⎯⎯⎯�[39m�[1m�[41m Failed Tests 1 �[49m�[22m�[31m⎯⎯⎯⎯⎯⎯⎯�[39m
�[41m�[1m FAIL �[22m�[49m src/services/acpConnection.test.ts�[2m > �[22mAcpConnection child exit cleanup�[2m > �[22mdoes not wire replacement streams into a retired startup
�[31m�[1mError�[22m: Test timed out in 15000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".�[39m
�[36m �[2m❯�[22m src/services/acpConnection.test.ts:�[2m655:3�[22m�[39m
�[90m653| �[39m })�[33m;�[39m
�[90m654| �[39m
�[90m655| �[39m it('does not wire replacement streams into a retired startup', async…
�[90m | �[39m �[31m^�[39m
�[90m656| �[39m vi�[33m.�[39m�[34museFakeTimers�[39m()�[33m;�[39m
�[90m657| �[39m �[35mtry�[39m {
�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯�[22m�[39m
�[2m Test Files �[22m �[1m�[31m1 failed�[39m�[22m�[90m (1)�[39m
�[2m Tests �[22m �[1m�[31m1 failed�[39m�[22m�[2m | �[22m�[1m�[32m53 passed�[39m�[22m�[90m (54)�[39m
�[2m Start at �[22m 20:57:46
�[2m Duration �[22m 15.91s�[2m (transform 237ms, setup 0ms, collect 255ms, tests 15.07s, environment 0ms, prepare 211ms)�[22m
round 2 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 2 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: F (exit 1)
--- output tail · round 2 · packages/vscode-ide-companion/src/services/acpConnection.test.ts ---
ns sdkConnection when process is alive�[32m 0�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdisconnect clears child, sdkConnection, and sessionId�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdisconnect is a no-op when there is no child�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdisconnect closes the CLI stdin instead of killing it (#11303)�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m AcpConnection child exit cleanup�[2m > �[22mdoes not force-kill a child that
...truncated -- full content in the run artifacts.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
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) and Test (macos-latest, Node 22.x) were skipped in CI and their suites did not run locally; the escalation ladder's Windows taskkill rung is mock-covered only.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): src/services/acpConnection.test.ts — no such file or directory; src/acp-integration/acpAgent.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and their suites did not run locally; the escalation ladder's Windows taskkill rung is mock-covered only.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
Test Plan（非阻断）：src/services/acpConnection.test.ts — no such file or directory; src/acp-integration/acpAgent.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.23.3)
…fix/acp-graceful-shutdown # Conflicts: # packages/vscode-ide-companion/src/services/acpConnection.test.ts # packages/vscode-ide-companion/src/services/acpConnection.ts
|
@qwen-code /triage |
|
⏸️ Deferring to @qqqys — needs a human call on this one. The design is sound and most of it holds up under tracing: I found no Critical, all 41 review threads are resolved, CI on I am not approving, and I am not requesting changes either. Three specific things:
What would move me to approve on the next run, with no further conditions: fix item 1 by either route, and add the missing fixture (same connection, Two corrections to what the triage comments said an hour ago, so nobody acts on the stale version: the 75-second grace is below the CLI's derived worst case, not above it ( Also for whoever picks it up, recorded so it does not get lost: SessionEnd hooks do not fire at all when a session is live — symmetric across head and base with no PR code in the loop, so pre-existing and not this PR's defect, but it qualifies the headline benefit, since a live conversation is exactly the state a user is in when they close the panel. Worth its own issue. Note on how this reached you: the deterministic owner resolver could not run in this environment, so the mention falls back to the most recent human reviewer on this PR, per the documented fallback chain. 中文说明⏸️ 转交 @qqqys —— 这件事需要人来定。 设计是可靠的，追踪下来大部分也站得住：没有 Critical，41 个 review thread 全部 resolved， 我既不批准，也不提交 request changes。三件具体的事：
**能让我在下一轮直接批准、不附加新条件的是：**按任一条路径修掉第 1 项，并补上缺失的 fixture（同一连接、 对一小时前三条评论的两处更正，以免有人照着过期版本行动：75 秒宽限是低于 CLI 的推导最坏情况，而不是高于（ 另外留给接手的人，记在这里以免丢失：当 session 处于活跃状态时 SessionEnd hook 完全不触发 —— head 与 base 对称、回路中没有本 PR 代码，所以是既有问题、不是本 PR 的缺陷；但它限定了本 PR 的宣称收益，因为用户关闭面板时所处的正是一个活跃会话。值得单独开一个 issue。 关于这条 @ 是怎么找到你的：本环境下确定性的 owner 解析脚本无法运行，因此按文档规定的回退链，落到了本 PR 最近一位人类评审者身上。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
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, so this diff's Windows-only escalation branch (the taskkill /f /t rung and the non-detached spawn) was never executed on Windows.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — test-efficacy probe: the kit tripped scripts/vitest-global-setup.js's prerequisite guard in every packages/cli probe tree (harnessValidated null, every probe inconclusive: no-output), so no mutant or revert was measured for the CLI package; the verifiers' hand-built mutation runs are this review's only efficacy evidence.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): 10 passed — this review observed 30884, 577 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/vscode-ide-companion/src/services/acpConnection.ts:511 — [probe] 10 of 13 request methods lack the superseded-response guard the design doc states unconditionally; a retired CLI's setMode answer drives the replacement's UI posturepackages/vscode-ide-companion/src/services/acpConnection.ts:742 — [probe] no cap on how many retired children can drain concurrently; three connect cycles leave three live process-group leaders with no registry
Convergence: round 3 posted 12 inline comment(s), 8 of them reported for the first time; the previous round posted 14 (9 new). Findings keep coming back to the same files: packages/vscode-ide-companion/src/services/acpConnection.test.ts (findings in rounds 1, 2; 3 more now); packages/vscode-ide-companion/src/services/acpConnection.ts (findings in rounds 1, 2; 3 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. (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, so this diff's Windows-only escalation branch (the taskkill /f /t rung and the non-detached spawn) was never executed on Windows.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — test-efficacy probe: the kit tripped scripts/vitest-global-setup.js's prerequisite guard in every packages/cli probe tree (harnessValidated null, every probe inconclusive: no-output), so no mutant or revert was measured for the CLI package; the verifiers' hand-built mutation runs are this review's only efficacy evidence.
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
Test Plan（非阻断）：10 passed — this review observed 30884, 577 passed。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 12 条行内评论，其中 8 条是首次提出；上一轮发布了 14 条（其中 9 条首次提出）。发现反复回到同一批文件：packages/vscode-ide-companion/src/services/acpConnection.test.ts（第 1、2 轮已出过发现，本轮又有 3 条）；packages/vscode-ide-companion/src/services/acpConnection.ts（第 1、2 轮已出过发现，本轮又有 3 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
A cancelled SessionEnd hook resolves fireSessionEndEvent to undefined instead of rejecting, so Promise.allSettled never observed it and the CLI exited 0 as though every hook ran. Detect the abort signal and surface it as a shutdown failure (non-zero exit). Also reset the in-flight exit-cleanup promise in the test helper so it cannot leak across vitest cases. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmty6iqs0zv
…ion switch Dropping the session-id axis from the superseded-connection guard so a turn that completes after the user switches sessions on the same live connection resolves instead of being reported as a hard failure. Also remove the now-write-only lastExitCode/lastExitSignal fields and the assertions maintaining them. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmty6iqs0zv
Pin four Suggestion-level coverage gaps deferred in the #11642 review: - assert the stdin EPIPE guard installs an 'error' listener (R3-4) - reset spawnMock per case and pin 'detached' on both platforms (R3-5) - cover the exit-handler ownership split: onDisconnected fires only for the current child, and the failed-start message pins exit code/signal (R3-6) - cover the taskkill error-callback fallback to child.kill() (R3-7) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmty8nwk9zy
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
COMMENT
核对基线：head 5914facdae5389f04e3c54bf9325cd18db339595。本次结论是 COMMENT 而非 APPROVE，原因只有两条，都在下面第 2、3 节；第 1 节的历史 Critical 我已逐条确认修好。
1. 最近一轮的两条 Critical：在当前 head 上确认已修复
上一轮 review（2026-09-12T08:20:50Z，针对 c0aaaca7）只留下两条 Critical，当前 head 之后又有 3 个 commit，我在 5914facd 上对代码复核：
- R3-1（
sendPrompt的守卫把「同一连接上切换会话」误判为连接被取代，导致一次成功的 turn 被报成硬失败）—— 已修复。acpConnection.ts:518-541现在捕获const promptSessionId = this.sessionId只用于发起请求，await 之后的守卫改成比较连接身份if (this.sdkConnection !== conn)，不再比较 session id。:531-535的注释也把理由写清楚了：newSession会重新赋值this.sessionId但不替换sdkConnection，所以把 session 比较折进这个守卫会把成功完成的 turn 误报成connection superseded。这正是该发现要求的修法。 - R2-3（30s abort 预算取消 SessionEnd hook 时是静默的，
Promise.allSettled观察不到，CLI 以 0 退出仿佛所有 hook 都跑过）—— 已修复。acpAgent.ts:2971-3006现在在Promise.allSettled收集完 rejected 之后，额外直接检测if (controller.signal.aborted)，并failures.push(new Error('SessionEnd hook did not complete within 30s (cancelled)'))。:2995-3002的注释准确复述了该发现的机制（被取消的 hook 返回{ success: false, outcome: 'cancelled' }而不 reject，因此 allSettled 看不到），补上的这条让取消的 hook 仍然进入失败通道，而不是让进程以 0 退出。
其余更早轮次的 Critical（R1-2、R1-12、R2-1、R2-2 等）在当前 PR 上均为 isResolved: true，且最近一轮 review 重新审过后只把 R2-3 一条以「still stands」重新提出，没有再重提它们。当前 53 条线程里只剩 4 条未解决，全部是 Suggestion 级（R3-2、R2-4、R1-18、R2-9），按本渠道策略不作为合入门禁。
2. 未完成的门禁：对重构后 diff 的独立 Critical 扫描
这是本次不 Approve 的主要原因。本 PR 自上一轮完整 review 之后被大幅重构过：变更文件从 4 个增加到 9 个（新增 packages/cli/src/utils/cleanup.ts 及其测试、设计文档与 E2E 测试计划），行数从 +1621/-30 变为 +789/-88，acpAgent.ts 与 acpConnection.ts 的实现路径都换过。当前 head 5914facd（10:44:07Z）之后没有任何 reviewer 审过。
我在本次预算内完成的是：上述两条 Critical 的定点复核，以及 sendPrompt 与 SessionEnd hook 收集器两处调用链的阅读。没有完成的是对整份重构后 diff 的独立 Critical-only 扫描 —— 特别是新纳入的 packages/cli/src/utils/cleanup.ts 改动（上一轮 R3-3 指出该文件新增了第二份模块私有状态，而对应 helper 的文档没有同步）、disconnect() 的两档升级阶梯在新实现下的时序算术，以及 Windows taskkill 分支（CI 的 Test (windows-latest, Node 22.x) 为 skipped，上一轮也明确披露该分支从未在 Windows 上执行过）。这些我未能在预算内取得可核查的结论，因此按「无法确认」处理，不作为已通过。
3. 当前 head 上 Lint & Static 为 fail
Lint & Static (ubuntu-latest, Node 22.x) 在当前 head 上失败，原因是 Prettier 格式检查：
[warn] packages/cli/src/acp-integration/acpAgent.ts
[warn] Code style issues found in 1 file. Run Prettier with --write to fix.
这是本 PR 自己改动的文件，归因明确。按本渠道策略，格式类失败不构成 Critical（它不暴露正确性、安全性、数据损坏或回归），所以我没有把它当作阻塞问题；但它是一个 required check 处于红色，实际会挡住合入，且说明最后几个 commit 推送前没有跑过仓库自带的格式检查。请执行 npm run format 后重新推送。
CI 其余部分：Test (ubuntu-latest, Node 22.x)、Integration Tests (no-AK, No Sandbox)、Desktop Shell、Real daemon E2E 均 pass；web-shell E2E Smoke 与 review-pr 仍 pending，按策略不作为门禁。
下一步
先 npm run format 让 Lint & Static 转绿；之后我可以在新的 head 上补完第 2 节列出的那部分独立扫描。第 1 节的两条 Critical 已确认修好，无需再动。
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
已核对 head 5914facd（vs origin/main merge-base）。本次不能 approve，原因是一条硬门禁：
Lint & Static (ubuntu-latest, Node 22.x)在Run Prettier这一步失败，packages/cli/src/acp-integration/acpAgent.ts未格式化。我用仓库里的 prettier 在 head 上复现了同样结果（Run ESLint是过的，所以只有格式这一项）。
另外两处 P3 见 inline，都不阻塞。
不能判定的部分我如实说明：CI 的 windows / macos job 被跳过，所以 Windows-only 的 taskkill /f /t 那一级和非 detached spawn 没有 CI 覆盖，我没有本地环境验证；文档里那条 Verification 断言与两条升级路径的对应关系我也只做了阅读比对（既有 unresolved 线程 docs/design/vscode-acp-graceful-shutdown.md:27 说的就是这件事）。
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES
已核对 head 5914facdae5389f04e3c54bf9325cd18db339595（vs merge-base 4c072e88a5）。
阻塞：本 PR 引入的格式违规，Lint & Static 红
失败步骤是 Run Prettier（node scripts/lint.js --prettier）：
[warn] packages/cli/src/acp-integration/acpAgent.ts
[warn] Code style issues found in 1 file. Run Prettier with --write to fix.
确认是本 PR 带进来的：我把该文件的 head 版本与 merge-base 版本分别按仓库配置跑了 prettier —— merge-base 版本 prettier-clean，head 版本有 1 处不符，位置在 acpAgent.ts:3001 附近（new Error('SessionEnd hook did not complete within 30s (cancelled)') 这一行超出 80 字符宽度，需要折成多行）。跑一次 npm run format 即可。
我上一版两条阻塞项的现状（在同一 head 复核）
- drain 记忆化：已修好。
acpAgent.ts现在写成drainPoolPromise ??= agentInstance.shutdownMcpPool(8_000); await drainPoolPromise;且try/catch在每次调用外侧，吞错与strict抛错由各调用点自己决定 —— 于是「strict 档被首次宽松调用中和」与「首次 strict 失败外溢给后续 joiner、跳过disposeSessionsOnce()」两条都不再成立。 - 两档时序：从 45s/10s 提到
SHUTDOWN_GRACE_MS = 75_000/SIGTERM_GRACE_MS = 75_000（acpConnection.ts:46-47）。第二档 75s 已能覆盖我算过的信号路径最坏预算（dispose 30s + drain 8s + cleanup 5s = 43s）。第一档仍有一个未被回答的问题：fireSessionEndOnce是按每个活跃会话的 Config 顺序await的，单次 hook 预算DEFAULT_HOOK_TIMEOUT = 60000，所以两个以上会话的默认超时 hook 就能超过 75s；这与 #11510 的「No constant works」是同一问题。请明确取舍（要么给出会话数上限下的推导，要么把升级判据从墙钟换成卡死证据），不要用注释里的算术替代它。
本 head 上仍未处理的事项
- 我账号在该 head 上还留有两张未回应的 P3：
acpAgent.ts:3002（注释承诺被取消的 SessionEnd hook 会「non-zero exit」，但抛出被managedConfigs &&挡住、非 managed 的 SIGTERM 路径最终process.exit(0)，而 VS Code companion 走的正是非 managed 分支）与acpConnection.ts:536（onEndTurn不带会话身份，放开 guard 后切会话期间 A 的在途 turn 结束会给当前显示的 B 发一次提前的streamEnd/idle）。请各给一个处置：修或写明为何不改。 - 另有 4 条 Suggestion 线程处于未解决但作者标注延后：R3-2（设计文档 Verification 段与同文档 11 行前自相矛盾）、R2-4（两个 shutdown 预算只从单侧钉，可以各自缩到 0 仍全绿；跨包 75s 推导无人钉）、R1-18（无测试触达
ClientSideConnection工厂，五道 inbound supersede 门全部未跑）、R2-9（initialize 之后的归属判定复合守卫无用例）。其中 R2-4 与上面的第一档推导是同一件事的两面，建议一起处理。 - 机器人在本 head 的
review-pr档仍在跑，其结论出来后若有新增阻塞，请一并处理。
CI 侧其余必需项目前是绿的（Test (ubuntu-latest, Node 22.x)、Integration Tests (no-AK, No Sandbox)、web-shell E2E Smoke 均 pass），本次拦的是格式 + 上面列出的待办。
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings. Approval blockers: none.
Scope: acpAgent.ts, cleanup.ts, acpConnection.ts + their tests + design docs. NOT reviewed — native Windows process-tree kill behavior (mocked tests only per PR test plan), real VS Code extension host session lifecycle, and the underlying ConPTY leak (#11623).
Checked:
- Promise memoization for overlapping shutdown paths (
acpAgent.ts):drainPoolPromise ??=,disposeSessionsPromise ??=, andsessionEndPromiseIIFE correctly ensure each cleanup phase runs exactly once.??=is atomic in single-threaded JS, so no TOCTOU gap. - Stale-child guards cover all write-state paths (
acpConnection.ts):ownChildcaptured atsetupChildProcessHandlersentry; all state-mutating callbacks (sessionUpdate,requestPermission,readTextFile,writeTextFile,extNotification,newSession,sendPrompt,loadSession) verifythis.sdkConnection !== conn/this.child !== ownChildafter await points. Post-initialize double-check closes the race between initialize response and child replacement. - Process group management:
detached: process.platform !== 'win32'correctly creates a new process group on POSIX (libuvsetsid()); negative-PIDprocess.kill(-childPid, ...)targets the group. Windows usestaskkill /t /f /pidvia absoluteC:\Windows\System32path withSystemRootenv-var fallback — correct. - Escalation timer lifecycle:
child.once('exit')clears bothgraceTimerandkillTimer; thechild.exitCode !== nullcheck still correctly skips escalation if the child already exited before the listener registered — no timer leak. - SessionEnd abort budget: 30s
AbortControllerwithtimeout.unref()is correct;Promise.allSettled+ explicitcontroller.signal.abortedcheck after settlement catches hooks that resolve (not reject) on cancellation. runExitCleanupdeduplication:exitCleanupPromisememoization withfinallyclearing is correct.shuttingDownguard: synchronousif (shuttingDown) return; shuttingDown = trueprevents re-entry from concurrent SIGTERM/SIGINT delivery.- Test coverage: comprehensive — shared cleanup pass, SIGTERM+IDE overlap, 30s abort budget, POSIX group escalation, POSIX fallback, Windows taskkill, Windows taskkill failure degradation, normal exit cancellation, stale exit handler, stale responses for
newSession/sendPrompt/loadSession, session-switch on same connection.
Needs a human (not blockers, bounded impact, out of static-reach):
- Read-only methods (
rewindSession,restoreSessionHistory,authenticate,listSessions,deleteSession,renameSession,cancelSession,switchSession,setMode,setModel,getAccountInfo) lack post-awaitsdkConnectionidentity checks (unlikenewSession/sendPrompt/loadSession). If the connection is superseded during one of these awaits, the response is silently consumed. Low impact for truly read-only methods, butcancelSessionon a stale connection could theoretically target a wrong session's work on the CLI side. - Native Windows process-tree termination (ConPTY descendants, shell processes) remains a manual validation boundary per the PR's own test plan.
Reviewed with AI assistance.
The Lint & Static lane's Run Prettier step failed on this file at 5914fac with a single wrapping complaint. `npx prettier --write` produces exactly this rewrap of the `new Error(...)` argument and nothing else, and `--check` is clean afterwards; eslint on the file is clean too. No behaviour change: the whole diff is whitespace inside one call's argument list, so the string the failure carries is byte-identical. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The comment promised the abort detection makes a cancelled hook "surface as a shutdown failure (non-zero exit)", but the throw ten lines below is gated on `managedConfigs`, so an unmanaged shutdown records the failure, logs the warning, and still exits 0. State both outcomes instead of only the one the managed path delivers. Comment only: the diff carries no non-comment line, and prettier and eslint are clean on the file. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…arm delivers The bullet claimed "POSIX and Windows escalation target the complete child tree". POSIX signals the child's process group, which does not reach a descendant that created its own group via setsid(); Windows walks the tree via taskkill /f /t but degrades to child.kill() (direct child only) when taskkill fails. Align both language versions with the Decision section and the code. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtycy85205
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 42 passed · 0 failed · 42 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：42 通过 · 0 失败 · 42 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11642 deep verification (round 4) —
|
| # | prior finding | prior severity | status at ebbeffec |
evidence (this round) |
|---|---|---|---|---|
| 1 | Seven companion guards ship with zero effective test coverage | Suggestion | partially fixed — 6 of 7 stand | M9/M10/M11 now KILLED (1 failed | 28 passed (29), red = "does not apply session or prompt responses from a retired connection") — that is 5914facd working. M3–M8 still SURVIVED at Tests 29 passed (29) exit 0. Positive control CTRL-positive KILLED, 2 red in the mutated file. |
| 2 | sendPrompt's same-session clause turns an ordinary conversation switch into a user-visible error popup |
Major | FIXED | H7, three arms on a real wire: r3/navigate → REJECTED -32603 "Internal error: connection superseded", onEndTurn not fired; head/navigate → resolved end_turn, onEndTurn(end_turn) fired; base/navigate → resolved end_turn. Head matches base on the oracle. The author took exactly the resolution round 3 suggested (drop the same-session clause, keep connection identity). |
| 3 | SHUTDOWN_GRACE_MS = 75 s below the CLI's derived worst-case wind-down |
Suggestion | stands (static ceiling, unchanged) | Constants still 75_000 / 75_000 (acpConnection.ts:42-43). No shutdown exceeding 75 s was produced; H1's measured head shutdown was 50–100 ms. |
| NEW r3 | SHUTDOWN_GRACE_MS's duration is unpinned (75 s → 0 ms leaves the suite green) |
Suggestion | stands, and the gap is wider than reported | M18 (SHUTDOWN_GRACE_MS→0), M19 (SIGTERM_GRACE_MS→0) and M20 (→37 500, a factor-of-two error) all SURVIVED at 29 passed (29). 5914facd added 3 tests and pinned none of them. |
| C1 | "Before, disconnect terminated the ACP child immediately" is Windows-only, not POSIX | Correction | stands | H1 base/no-session: child killed = YES, stdin closed = no, exit code=0 in 50 ms — base does terminate immediately on POSIX too, so the description's sentence is accurate for the companion path. |
| C4 | Test-plan counts wrong | Correction | regressed — wrong again | Measured 29/29 and 719/719 against the description's 26/26 and 709/709. |
| O1 | SessionEnd hooks do not fire when a session is live | Observation (round 2) | not re-measurable this round | My hook probe produced 0 fires on both arms in all 4 cells (A/A control) — see Not covered. Round 3's refutation of O1 is not contradicted; it simply is not re-confirmed here. |
No prior finding worsened in behaviour. One prior correction (C4) regressed, and one prior Suggestion (the unpinned grace window) now has a wider measured gap.
Scope chosen
| Central claim | disconnect() makes the CLI shut itself down (host closes stdin, no kill) and spawns the child as a process-group leader so the bounded escalation ladder can reach the whole tree. |
| Secondary claim 1 | Narrowing sendPrompt's guard to connection identity fixes the false "connection superseded" on an ordinary session switch without weakening the reconnect hazard the guard exists for. |
| Secondary claim 2 | The new tests in 5914facd close the coverage gaps round 3 reported. |
| Delta probes | b462fdfa's new "fail shutdown on a cancelled SessionEnd hook" throw — which path can actually reach it; and the mis-attribution consequence round 3 warned the dropped clause had been accidentally preventing. |
| Out of scope | Windows, managed-parent shutdown, real VS Code extension host, repo-wide lint, trial merge — all under Not covered. |
Central claim + A/B
H1 — real CLI, real wire, only AcpConnection differs
Both arms spawn the same real CLI build (packages/cli/dist/index.js --acp --channel=VSCode) through the real AcpConnection.connect(); cliEntryPath is the seam. Only the compiled AcpConnection differs, produced by bundle-acpconn.mjs (esbuild) from each tree with vscode aliased to vscode-stub.mjs — the only stub in the loop.
Control cleanliness, all scripted assertions:
| check | head | base | r3 |
|---|---|---|---|
INTERNAL_QWEN_CODE_DEPS in bundle |
0 | 0 | 0 |
75e3 (grace constants) |
2 | 0 | 2 |
detached |
1 | 0 | 1 |
taskkill |
2 | 0 | 2 |
connection superseded |
14 | 0 | 14 |
stdin.end() |
1 | 0 | 1 |
| own source files resolved inside own tree | 85 | 5 | — |
files resolved into the head tree's packages/ |
— | 0 | — |
node_modules roots used |
1 (/__w/qwen-code/qwen-code) |
1 (same) | — |
The resolvedIntoHEADTreePackages: 0 row is the one that matters: acpConnection.ts imports no @qwen-code/* package, so the workspace-symlink trap does not apply, and the base arm's five own files (logger.ts, errorMessage.ts, acpFileHandler.ts, acpSchema.ts, acpConnection.ts) all resolved inside tmp/base-tree/. git diff --stat HEAD^1..HEAD -- package.json package-lock.json is empty, so sharing the root node_modules is a clean control. Raw: logs/provenance-base.json, logs/bundle-*.json.
Oracles: (1) whether the host closed the child's stdin, read at the instant disconnect() returned; (2) whether the host killed the child (ChildProcess.killed); (3) the child's pgid read from the kernel with ps -o pgid= (Node has no process.getpgrp()).
| cell | group leader | stdin closed by host | child killed by host | SessionEnd fires | exit | ms |
|---|---|---|---|---|---|---|
| head / no-session | YES (pid=pgid=87852) |
YES | no | probe unavailable | 0 | 50 |
| base / no-session | no (pid=87904 pgid=87843) |
no | YES | probe unavailable | 0 | 50 |
| head / live-session | YES | YES | no | probe unavailable | 0 | 100 |
| base / live-session | no | no | YES | probe unavailable | 0 | 101 |
Flip: 2/2 on the stdin oracle; 2/2 on the group-leader oracle; 2/2 on the killed oracle. This is the central claim measured directly: head closes stdin and does not kill, base kills and never closes stdin. Witness: 01-h1-central-ab-real-cli-flips.png. Raw: logs/h1-results.json, logs/h1-run.log.
The SessionEnd-reason column is blank because my probe did not fire on either arm — see Not covered; the 0/0 symmetry across all four cells is the A/A control that makes this a probe failure rather than a regression.
H7 — three arms on a real wire: did the Major fix land, and did it cost anything?
Because history was deepened this round (git fetch --depth=40, which succeeded — all 18 snapshot commit OIDs are now present locally), I could build a third arm from round 3's head c0aaaca7. That separates "the PR works" from "this commit is what fixed it".
Peer = a real @agentclientprotocol/sdk AgentSideConnection over real ndjson pipes, spawned by the real AcpConnection (PEER_MODE=navigate holds session/prompt until a session/load arrives, then answers end_turn and emits an agent_message_chunk for the session that owned the prompt). In-flight ordering is proven at the destination: the peer writes a marker file when it is holding the prompt, and the harness will not issue the switch until that file exists (marker=prompt-held:sess-A asserted in all six cells).
| arm | scenario | prompt outcome | onEndTurn |
post-switch updates | reconnect | replacement conn |
|---|---|---|---|---|---|---|
| base | navigate | resolved end_turn |
end_turn |
1 (sess-A) |
— | — |
| r3 | navigate | REJECTED -32603 connection superseded |
(none) | 0 | — | — |
| head | navigate | resolved end_turn |
end_turn |
1 (sess-A) |
— | — |
| base | reconnect | — | — | — | ERR failed to start (exit code: 143) |
REJECTED Not connected |
| head | reconnect | REJECTED -32603 connection superseded (correct) |
— | — | ok | resolved end_turn |
Flip: 1/1 on the navigate oracle, r3 → head. Round 3's Major finding reproduces exactly on the r3 arm and is gone on head. And the narrowing did not gut the guard: on reconnect, head still rejects the stale prompt with -32603, establishes the replacement cleanly, and leaves it usable — while base misattributes the old child's SIGTERM to the replacement and ends up with no working connection at all. That is the ownChild binding paying off, measured end to end.
The mis-attribution worry round 3 raised is measured, and bounded. Round 3 noted the same-session clause "accidentally prevents a worse pre-existing bug (the success path would file the old turn's text under the new conversation)". With the clause dropped, I measured it: the peer emits sess-A content after the switch to sess-B, and head delivers exactly what base delivers — [{"sessionId":"sess-A","text":"answer-for-sess-A"}] byte-identical on both arms, updatesAfterSwitch=1 on both. So the PR introduces no new mis-attribution; whatever the webview does with a sess-A update while showing sess-B is pre-existing behaviour, unchanged by this diff. The update carries its own sessionId, which is what makes it the webview's problem and not this PR's. Witness: 02-h7-navigate-fix-three-arms.png. Raw: logs/h7-results.json, logs/h7-run.log.
Corrections
Corrections to earlier descriptions, not requests to change code.
- The Reviewer Test Plan's counts are stale (again). It states "Companion ACP tests 26/26, CLI ACP tests 709/709, and cleanup tests 10/10". Measured at this head: companion
acpConnection.test.ts29/29 (5914facdadded three), andacpAgent.test.ts+cleanup.test.tstogether 719/719. Round 3 recorded this same correction as fixed; the two commits after it moved the numbers again. Worth one edit before merge so a reviewer running the plan does not think they are on the wrong commit. - The design doc's escalation bullet was narrowed correctly, and now matches what I measured.
ebbeffecchanged "POSIX and Windows escalation target the complete child tree" to "POSIX escalation targets the ACP child's process group; Windows escalation targets the process tree viataskkill /f /t, degrading to the direct child if taskkill fails." That is an accurate description: H1's group-leader oracle confirms the POSIX half is a process group, not a tree walk, and the Windows half is unverifiable here. This is a correction in the PR's favour — recorded so the next reader does not re-derive it. b462fdfa's headline does not reach this PR's own scenario. "fail ACP shutdown when a SessionEnd hook is cancelled" addsif (controller.signal.aborted) failures.push(...), and thethrow new AggregateError(...)below it is gated onmanagedConfigs.managedConfigsis passed only byshutdownManagedAgent, which returns early unlessagent.isTrustedManagedParent()— i.e.privateParentState === 'trusted', which requires a matchingPRIVATE_PARENT_CAPABILITY_META_KEYin theinitialize_meta(acpAgent.ts:4978-5003). The companion'sinitializecall sends no_meta(acpConnection.ts:388-395), and the capability is minted bypackages/acp-bridge/packages/channels/base/src/AcpBridge.ts, not by the VS Code extension. So on the companion path the new commit adds adebugLogger.warnline only; the non-zero exit applies to the bridge-managed CLI. The commit's own code comment already says this precisely ("the throw below is gated onmanagedConfigs") — recording it because a reviewer reading the subject line alone would expect the companion's exit code to change. Static trace, not measured live (the managed path needs a bridge parent this container does not set up).- Head adds an unhandled-rejection swallow that base lacks.
void processExitPromise.catch(() => {})appears 1× in the head and r3 bundles and 0× in base.rejectOnExitfires on every child exit including a retired one (acpConnection.ts:180-185), so without the swallow a late exit from a replaced child is an unhandled rejection. I observed exactly that crash base-side during harness development before I made the peer flush its answer; after the fix,count=0on all arms, so this is a static improvement, not a measured one.
Mutation matrix
21 mutations of packages/vscode-ide-companion/src/services/acpConnection.ts, each neutering exactly one guard the PR introduces, each running acpConnection.test.ts unchanged. Every anchor pre-validated unique (anchors validated: 21/21 unique, 0 bad); the file was restored and its sha256 re-verified after every cell (all files restored: true).
| verdict | count | mutations |
|---|---|---|
| KILLED | 12 | M1, M2, M9, M10, M11, M13, M14, M15, M16, M17, M21, CTRL-positive |
| SURVIVED | 9 | M3, M4, M5, M6, M7, M8, M18, M19, M20 |
Every load-bearing hunk is pinned, and each kill names the exact red test: detached (M13 → "creates a POSIX process group for shutdown escalation" + "does not detach the ACP child on Windows", 2 red), group SIGTERM (M14) and group SIGKILL (M15) → "escalates to the POSIX process group after both grace periods", taskkill /t (M16) → "uses taskkill for an unresponsive Windows process tree", escalation-cancel (M17) → "cancels escalation when the child exits normally", both child-identity guards (M1, M2), all three response-path supersede throws (M9, M10, M11), and the central claim itself (M21, reverting disconnect() to child.kill() → 4 red).
Positive control: CTRL-positive (disconnect never closes stdin) KILLED with 2 red tests landed in the mutated file — "disconnect closes stdin before escalating" and "disconnect closes stdin even when the child has no pid". So the nine greens are a measurement, not a dead harness. Witness: 03-mutation-matrix-companion.png. Raw: logs/mutation-matrix-companion.log, parsed to logs/mutation-matrix-companion.json.
Survivors, classified per the taxonomy:
- M3–M8 → coverage gaps. The five inbound-callback guards (
sessionUpdate,requestPermission,readTextFile,writeTextFile,extNotification) and the post-initializesupersede check. Each is the only check on its path; no sibling hunk closes the same hazard, so these are not redundant defence. Each is reachable — the PR's own premise is that the old child can outlive the graceful window. Note the contrast with M9/M10/M11:5914facdpinned the three outbound response guards and left the six inbound ones. Not dead code, not redundant — tests to write. - M18, M19, M20 → coverage gaps with the sharpest edge in this round. See Finding 1.
Gates
| gate | command | result |
|---|---|---|
| companion suite | (cd packages/vscode-ide-companion && npx --no-install vitest run src/services/acpConnection.test.ts) |
29 passed (29), exit 0 |
| CLI suites | (cd packages/cli && npx --no-install vitest run src/acp-integration/acpAgent.test.ts src/utils/cleanup.test.ts) |
719 passed (719), 2 files, exit 0 |
| companion typecheck | npm run check-types -w packages/vscode-ide-companion |
exit 0 |
Round 3's correction still applies and I re-confirmed the script names: npm run typecheck is typecheck --workspaces --if-present, and packages/vscode-ide-companion names its script check-types, so the repo-wide green does not compile acpConnection.ts. The command above does, and passes. Not run this round: repo-wide npm run typecheck, npm run lint, npm run format, and eslint/prettier on the changed files (round 3 ran those with planted-violation liveness proofs; the changed files here are a subset plus two docs lines).
Findings
1. The 75-second graceful window — the property the PR is named after — is still unpinned, and the gap is wider than round 3 reported — Suggestion (carried, stands)
cd packages/vscode-ide-companion && npx --no-install vitest run src/services/acpConnection.test.ts # 29/29 green as shipped
node tmp/pr11642-verify-20260912-130232/mutate-companion.mjs M18 M19 M20 # still 29/29, exit 0acpConnection.ts:42-43 defines SHUTDOWN_GRACE_MS = 75_000 and SIGTERM_GRACE_MS = 75_000. Three separate mutations survive the full suite:
| mutation | change | suite |
|---|---|---|
| M18 | SHUTDOWN_GRACE_MS = 0 |
29 passed (29), exit 0 |
| M19 | SIGTERM_GRACE_MS = 0 |
29 passed (29), exit 0 |
| M20 | SHUTDOWN_GRACE_MS = 37_500 (a factor-of-two error) |
29 passed (29), exit 0 |
M20 is new this round and is the one to notice: the suite cannot distinguish 75 s from 37.5 s either, so this is not merely "0 is a special case" — no duration is pinned at all. The escalation test advances fake timers past the deadline and asserts the signal, which any smaller deadline satisfies identically.
5914facd was titled "pin deferred ACP shutdown coverage gaps" and did close three gaps (M9/M10/M11 flipped from survived to killed), so this is not inattention — it is the one axis the new tests did not reach. Cheap fix, and it is a test, not code: assert that at advanceTimersByTime(74_999) no signal has been sent and at 75_000 one has. Without it, a maintainer retyping 75_000 as 75 gets a green suite and a CLI that is SIGTERM'd the instant the panel closes — the exact defect this PR exists to fix.
2. Six inbound stale-connection guards still ship with zero effective test coverage — Suggestion (carried, partially fixed)
node tmp/pr11642-verify-20260912-130232/mutate-companion.mjs M3 M4 M5 M6 M7 M8 # 29/29 green for eachThe five inbound-callback this.sdkConnection !== wiredConnection checks and the post-initialize supersede throw, in acpConnection.ts. Bounded honestly, as round 3 did: the guards are correct — H7 drives two of the sibling guards on a real wire and they behave exactly as intended (stale prompt rejected, replacement unaffected). Nothing here is a correctness defect. The cost is that six shipped clauses can be deleted with a green suite, so the next refactor has no net under them. The asymmetry with M9/M10/M11 is the actionable part: the outbound responses are now pinned and the inbound callbacks are not, and they are the ones a retired child actually fires.
3. The Reviewer Test Plan's automated-coverage counts are stale — Correction
"Companion ACP tests 26/26, CLI ACP tests 709/709, and cleanup tests 10/10 passed" — measured 29/29 and 719/719 (acpAgent + cleanup together) at ebbeffec. Round 3 recorded this correction as fixed; 5914facd moved the numbers again. One-line edit.
Not covered
- The SessionEnd-
reasonoracle — probe unavailable, with an A/A control. My command hook (cat >> $HOME/sessionend.jsonl, configured in a throwawayHOME/QWEN_HOMEwithsecurity.folderTrust: false) wrote 0 bytes in all four cells on both arms. This is not a shutdown-path failure: the real CLI booted, completedinitialize, and servedsession/newin every cell (headsessionId=337afd96-…, basesessionId=e3f91ef2-…), and the group-leader/stdin oracles flipped cleanly in the same runs. Because the zero is symmetric across arms, it is my probe (settings shape, hook registration, or trust gating in this container), not the PR — so those cells are excluded fromassertions.jsonrather than counted red, and the reason oracle round 3 measured (prompt_input_exitvsother) is not re-confirmed at this head. What would have settled it: dumping the CLI's resolved hook registry, which the budget did not reach. - H2's escalation-ladder timings were not re-measured live. Round 3 measured SIGTERM at 75 070 ms and the SIGKILL rung at ~156 s against a wedged peer with a non-detached grandchild. This round pins the same hunks at unit level only (M13–M17 all KILLED, naming "escalates to the POSIX process group after both grace periods"). The live 150 s cell was cut for budget; the code it exercised is unchanged between
c0aaaca7andebbeffec(asserted:disconnect()is byte-identical between round-3 head and PR head), but the base moved, so I am recording this as not re-run rather than carrying it forward. - Windows, entirely.
taskkill /f /t, theWINDOWS_TASKKILLabsolute-path resolution, the non-detached spawn — unreachable (process.platform !== 'win32'). M16 shows/tis at least pinned by a mocked test. The platform where the PR's stated orphaning actually bites remains unverified. - The managed / trusted-parent path, including all of
b462fdfa's behavioural change.shutdownManagedAgent,isTrustedManagedParent(),beginManagedShutdown— never reached by the companion's plain spawn (Correction 3 traces why). The newAggregateError→process.exit(1)behaviour is therefore unexercised by any measurement in this report, and no test in the 719 covers a cancelled SessionEnd hook on that path either (not verified by mutation; budget). - Real VS Code extension host. Finding 2's blast radius and the old popup path are wire-level and static reads, not watched UI.
- Repo-wide gates.
npm run typecheck,npm run lint,npm run formatwere not run; eslint/prettier were not run on the changed files this round (round 3 ran them with planted-violation liveness proofs). The two changed.mdlines are therefore unlinted here — and round 3 established that prettier's markdown formatter accepts every mutation, so a clean prettier run would not have been evidence anyway. - Trial merge into current
main. Not attempted. Note the snapshot'sbaseRefOid(4c072e88…) is now present locally after the deepening fetch, so this is a budget cut, not an impossibility — and it matters less than usual becauseHEAD^1(8a215515) is recent. - Per-commit attribution is now possible but was not exercised commit-by-commit. Round 3 could not reach the individual commits (shallow: local
rev-listreturned 1 against 18 in the snapshot). This round agit fetch --depth=40succeeded and all 18 snapshot OIDs resolve, sogit rev-list HEAD^1..HEAD^2returns 64 (18 PR commits + the merged main history). I verified the aggregateHEAD^1..HEADdiff and thec0aaaca7..ebbeffecdelta, and attributed the three behavioural commits by reading them, but did not build and test each of the 18 separately. - Environment note (not a PR defect).
.qwen/is a read-only root-owned mount in this container, so the PR's tracked.qwen/e2e-tests/vscode-acp-graceful-shutdown.mdshows asDingit statusand could not be written. The file's content was read from the object store and is included in the diff under test; only the worktree copy is missing. - No injection attempts observed. PR title, body, commit messages and code comments were treated as untrusted input; none attempted to steer the verification.
Methodology
Everything ran in the CI verify container (node:22-bookworm, node v22.23.2, 64 cores, load average ~10–14) against the merge-ref checkout HEAD = ea6866f3, with npm ci / npm run build already completed by the job. Two scratch worktrees were created and removed: tmp/base-tree at HEAD^1 (8a215515) and tmp/r3-tree at round 3's head (c0aaaca7), the latter reachable only because git fetch --depth=40 origin ebbeffec… succeeded. All three arms' AcpConnection were compiled from source by bundle-acpconn.mjs (esbuild, absWorkingDir set to each tree) with only vscode aliased to vscode-stub.mjs; provenance2.mjs re-ran each build with metafile: true and asserted the resolved input set, because the first provenance script mis-resolved esbuild's tree-relative metafile keys against process.cwd() and reported five phantom head-tree leaks — that bug is recorded here because it briefly looked like a contaminated control. Harnesses h1-real-cli-ab.mjs (real CLI over real pipes, pgid read with ps -o pgid=) and h7-navigate-ab.mjs + h7-peer.mjs (a real AgentSideConnection peer, in-flight ordering proven by a destination-side marker file the harness polls before issuing the switch) drive the compiled connections; assertions are made on kernel-reported pgids, on ChildProcess flags read at the instant disconnect() returned, and on JSON-RPC errors a real SDK peer produced. Two harness bugs of my own were found and fixed mid-round (peer env vars set after connect() had already captured process.env; and the peer calling process.exit(0) on conn.closed before flushing its answer, which produced a bogus TIMEOUT and a misattributed unhandled rejection); both are recorded because each produced a wrong intermediate table. mutate-companion.mjs (21 mutations) patches one anchor at a time, restores, and re-verifies sha256. Per-cell stdout/stderr is in logs/; assertions.json was recomputed from those logs by a script that strips ANSI escapes (the first pass matched nothing and reported two phantom gate failures); scripts/verify-capture.mjs rasterised the three tables into evidence/.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/acp-integration/acpAgent.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/acpAgent.test.ts
file packages/cli/src/utils/cleanup.test.ts: (cd packages/cli) npx --no-install vitest run ./src/utils/cleanup.test.ts
file packages/vscode-ide-companion/src/services/acpConnection.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/services/acpConnection.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/acp-integration/acpAgent.test.ts: PPPPP
packages/cli/src/utils/cleanup.test.ts: PPPPP
packages/vscode-ide-companion/src/services/acpConnection.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 1 · packages/cli/src/utils/cleanup.test.ts: P (exit 0)
round 1 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 2 · packages/cli/src/utils/cleanup.test.ts: P (exit 0)
round 2 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 3 · packages/cli/src/utils/cleanup.test.ts: P (exit 0)
round 3 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 4 · packages/cli/src/utils/cleanup.test.ts: P (exit 0)
round 4 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: P (exit 0)
round 5 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 5 · packages/cli/src/utils/cleanup.test.ts: P (exit 0)
round 5 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: P (exit 0)
Evidence images
Harness 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 stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
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 /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 52 passed · 0 failed · 52 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：52 通过 · 0 失败 · 52 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11642 deep verification (round 5) —
|
| # | Round-4 finding | Sev | Status at ebbeffec |
Re-measured how |
|---|---|---|---|---|
| 1 | The 75 s graceful window is unpinned — SHUTDOWN_GRACE_MS→0, SIGTERM_GRACE_MS→0, SHUTDOWN_GRACE_MS→37 500 all leave the suite green |
Suggestion | stands — and now has a measured fix | Re-ran all three mutations: M18/M19/M20 each 29 passed / 0 failed / exit 0. Then applied the candidate fix and re-ran: 30/30 green unmutated, and 1 red (the new test) under each of the three mutations. |
| 2 | Six inbound stale-connection guards ship unpinned (M3–M8) | Suggestion | stands | M3 (the inbound sessionUpdate guard) re-run in this container: 29 passed / 0 failed / exit 0 — reproduces round 4. M4–M8 carried on the identical-tree proof and labelled as such; the pin test from Finding 1 does not address them (M3+pin is still 30/30 green), so the two findings are independent. |
| 3 | Reviewer Test Plan counts are stale ("26/26, 709/709, 10/10") | Correction | stands | Re-measured at this head: companion acpConnection.test.ts 29/29; acpAgent.test.ts + cleanup.test.ts 719/719 (709 + 10). The plan's 26/26 is still wrong. |
| 4 | Design doc's escalation bullet correctly narrowed | Correction (in PR's favour) | stands | Re-read; Oracle A confirms the POSIX half is a process group (kernel-reported pgid), not a tree walk. |
| 5 | b462fdfa's headline does not reach the companion path (throw gated on managedConfigs) |
Correction | stands | Static re-read of acpAgent.ts shutdownHandler / shutdownManagedAgent; not measured live (needs a bridge parent this container does not set up). |
| 6 | Head adds an unhandled-rejection swallow base lacks | Correction (static) | stands, still static | Measured unhandledRejections = 0 on both arms, so this remains a static improvement — recorded as not a measured one, as round 4 did. |
| — | Round 3's Major (same-connection session switch rejected -32603) |
Major | fixed (round 4, three arms) | Not re-run — the code it exercised is inside the identical tree, and the fix is pinned by the suite that reproduces 29/29 here. Labelled carried, not re-measured. |
Central claim and A/B
Central claim: disconnect() closes the ACP child's stdin so the CLI runs its own shutdown path, instead of child.kill(); forced termination becomes a bounded, group-scoped fallback.
Secondary claims: (a) the POSIX child leads its own process group so escalation can reach the group; (b) a retired child/connection cannot overwrite its replacement.
Host = the arm's own AcpConnection compiled from source by esbuild; peer = the real CLI (packages/cli/dist/index.js --acp --channel=VSCode) over real pipes, in a throwaway HOME/QWEN_HOME. Nothing on the path under test is stubbed; only vscode (unresolvable outside an extension host) is aliased to a stub. Witness: 01-h1-central-ab-base-kills-head-closes-stdin.png.
Contamination control (the workspace-symlink trap — node_modules/@qwen-code/* point into the head tree). Asserted from esbuild's own metafile, per arm:
| arm | inputs | own-tree | root node_modules |
leaked | unaccounted | head-marker hits |
|---|---|---|---|---|---|---|
| head | 85 | 85 | 79 | 1 | 0 | SHUTDOWN_GRACE_MS=2 SIGTERM_GRACE_MS=2 detached=1 connection superseded=14 taskkill=2 |
| base | 85 | 5 | 79 | 1 | 0 | all 0 |
The single "leaked" input on both arms is this harness's own vscode-stub.mjs. 0 files from the head tree's packages/ reached the base bundle, and the base bundle contains none of the five discriminator strings. The PR touches no package.json or lockfile (verified: the 9-file diff contains neither), so sharing the root node_modules is a clean control rather than a confound.
| oracle | HEAD | BASE (control) | flip |
|---|---|---|---|
A child is its own process-group leader (kernel ps -o pgid=) |
YES (pid 30153 = pgid 30153) | NO (pid 30122, pgid 30106 = host's) | ✅ 1/1 |
| A child shares the host's process group | NO | YES | ✅ 1/1 |
B stdin.writableEnded at the instant disconnect() returns |
true | false | ✅ 1/1 |
B child.killed at that same instant |
false | true | ✅ 1/1 |
| C child exited code 0, no signal | YES (16 ms) | YES (16 ms) | ❌ does not flip |
| C child died by signal | NO | NO | ❌ does not flip |
| D SessionEnd / debug-log probe live | no | no | EXCLUDED (see Not covered) |
Both arms reached initialize (connectError: null, [ACP] Initialize successful, protocol 1) and disconnect() cleared child + sdkConnection on both. A/B on the central claim: 2/2 oracles flip.
Oracle C does not flip — and that is the round's finding, not a harness fault
Both children exit code 0 with no signal. Before believing that, I proved the oracle can see a signal death at all — spawning the real CLI directly, with no AcpConnection involved, and terminating it three ways (all three cells reached initialize, stdoutBytes 832 in each, so they are comparable):
| termination | exit code | signal | ms |
|---|---|---|---|
| stdin EOF | 0 | null | 14 |
SIGTERM |
0 | null | 16 |
SIGKILL ← control |
null | SIGKILL |
14 |
The control cell is reported as SIGKILL, so "both arms exit 0" is a real measurement and not a blind oracle. The mechanism is in the code and is pre-existing: acpAgent.ts registers process.on('SIGTERM', shutdownHandler) (base tree lines 3114–3115), which runs fireSessionEndOnce → disposeSessionsOnce → drainPoolBeforeExit('signal') → runExitCleanup().finally(() => process.exit(0)). I confirmed the registration and the process.exit(0) are not in this PR's diff — the diff touches the handler's body (shared-promise dedup) only. So base's child.kill() did reach a graceful CLI shutdown.
Corrections
Corrections to descriptions, not requests to change code.
- NEW, measured — the PR description's Before/After sentence overstates the base defect on POSIX. The description says: "Before, disconnect terminated the ACP child immediately and bypassed its normal cleanup." Measured: on POSIX the base child exits code 0, no signal, because the CLI already caught SIGTERM and ran SessionEnd hooks, MCP drain, session disposal and
runExitCleanupbeforeprocess.exit(0). Cleanup was not bypassed. The sentence is accurate on Windows, where Node'schild.kill()isTerminateProcessand cannot be caught — and Windows is exactly the platform the PR's own test matrix marks⚠️ mocked-only and Linux⚠️ not tested. What the PR genuinely buys on POSIX, measured or read: (i) a bounded escalation ladder where base had none — base signals once and drops the reference, so a wedged CLI is never re-signalled, while head goes stdin-EOF → 75 s → groupSIGTERM→ 75 s → groupSIGKILL; (ii) group-scoped signals that reach descendants the CLI failed to reap (Oracle A); (iii) the correct SessionEndreason(prompt_input_exitvsother) — read fromacpAgent.ts, not measured, see Not covered. The description's other sentence, "gives the previous shutdown path less time than its supported work requires and provides no catchable POSIX escalation rung", is closer to the truth, though base's SIGTERM was itself catchable and caught. - The Reviewer Test Plan's counts are stale for a third round. It states "Companion ACP tests 26/26, CLI ACP tests 709/709, and cleanup tests 10/10"; measured at this head 29/29 and 719/719 (709 + 10). One-line edit.
b462fdfa's headline does not reach the companion path (carried from round 4, re-read).throw new AggregateError(...)is gated onmanagedConfigs, passed only byshutdownManagedAgent, which returns early unlessisTrustedManagedParent(). The companion'sinitializesends no_meta, so on that path the commit adds adebugLogger.warnline only. Static trace, not measured live.- Head's
void processExitPromise.catch(() => {})remains a static improvement.Promise.racealready attaches a reaction toprocessExitPromise, so the ordinarydisconnect()path never produces an unhandled rejection on either arm — measuredunhandledRejections = 0on both. The swallow matters only whenconnect()throws before the race (thethis.child !== ownChild || ownChild.killedearly throw), which I did not construct.
Mutation matrix — and a measured fix for Finding 1
Target packages/vscode-ide-companion/src/services/acpConnection.ts; suite acpConnection.test.ts. Every cell restored both files and re-verified sha256 afterwards (9/9 ok; all files restored: true). Anchors were checked unique before applying. Witness: 02-mutation-matrix-75s-window-measured-fix.png.
| cell | mutation | pin test? | suite | verdict |
|---|---|---|---|---|
ship |
none (as shipped) | no | 29 / 0, exit 0 | green baseline |
M18 |
SHUTDOWN_GRACE_MS 75 000 → 0 |
no | 29 / 0, exit 0 | SURVIVED |
M19 |
SIGTERM_GRACE_MS 75 000 → 0 |
no | 29 / 0, exit 0 | SURVIVED |
M20 |
SHUTDOWN_GRACE_MS 75 000 → 37 500 |
no | 29 / 0, exit 0 | SURVIVED |
ship+pin |
none | YES | 30 / 0, exit 0 | green — zero collateral |
M18+pin |
SHUTDOWN_GRACE_MS → 0 |
YES | 29 / 1, exit 1 | KILLED by the new test |
M19+pin |
SIGTERM_GRACE_MS → 0 |
YES | 29 / 1, exit 1 | KILLED by the new test |
M20+pin |
SHUTDOWN_GRACE_MS → 37 500 |
YES | 29 / 1, exit 1 | KILLED by the new test |
CTRL |
disconnect() never closes stdin |
no | 27 / 2, exit 1 | KILLED — positive control |
M3 (spot-check) |
inbound sessionUpdate guard removed |
no | 29 / 0, exit 0 | SURVIVED (reproduces round 4) |
M3+pin (spot-check) |
inbound sessionUpdate guard removed |
YES | 30 / 0, exit 0 | survived — confirms the pin test is scoped to Finding 1 only |
Positive control, landed in the mutated file. CTRL (neutering the PR's central act) turns 2 tests red — "disconnect closes stdin before escalating" and "disconnect closes stdin even when the child has no pid", both in acpConnection.test.ts. So the survivors are a measurement, not a dead harness.
The failures are the intended assertions, not compile breaks. Each red cell names expected-vs-actual:
| cell | AssertionError |
calls |
|---|---|---|
M18+pin |
expected "kill" to not be called at all, but actually been called 1 times |
1 |
M19+pin |
expected "kill" to not be called with arguments: [ -4242, 'SIGKILL' ] |
2 |
M20+pin |
expected "kill" to not be called at all, but actually been called 1 times |
1 |
M19+pin failing on the SIGKILL argument specifically (2 calls, not 1) shows the test pins the second constant independently of the first — the two windows are separately asserted, which is why one 20-line test closes all three mutations.
Findings
1. The 75-second graceful window — the property the PR is named after — is still unpinned. Round 4 prescribed a fix but never measured it; this round did — Suggestion (carried, stands, fix now measured)
cd packages/vscode-ide-companion && npx --no-install vitest run src/services/acpConnection.test.ts # 29/29 green as shipped
node tmp/pr11642-verify-20260912-143259/mutate-and-pin.mjs M18 M19 M20 # still 29/29 each, exit 0
node tmp/pr11642-verify-20260912-143259/mutate-and-pin.mjs ship+pin M18+pin M19+pin M20+pin CTRL # 30/30, then 1 red each, control 2 redacpConnection.ts:46-47 defines SHUTDOWN_GRACE_MS = 75_000 and SIGTERM_GRACE_MS = 75_000. No test asserts either duration. The existing escalation test advances fake timers to 75 000 and then asserts the signal, which any smaller deadline satisfies identically — so the suite cannot distinguish 75 s from 37.5 s from 0 s. A maintainer retyping 75_000 as 75 gets a green suite and a CLI that is SIGTERM'd the instant the panel closes: the exact defect this PR exists to fix.
Measured fix (round 4 proposed the shape; the three results below are what round 4 left unmeasured). One test, appended inside describe('AcpConnection child exit cleanup'), which already has vi.useFakeTimers():
Suggested test — kills M18, M19 and M20; green on unmutated source
it('does not escalate before each 75s grace window elapses', () => {
if (process.platform === 'win32') return;
const kill = vi.spyOn(process, 'kill').mockReturnValue(true);
const conn = createConnection({ child: createMockChild() });
(conn as unknown as AcpConnection).disconnect();
// Just inside the graceful window nothing may be signalled.
vi.advanceTimersByTime(74_999);
expect(kill).not.toHaveBeenCalled();
// Exactly at the deadline: SIGTERM to the process group.
vi.advanceTimersByTime(1);
expect(kill).toHaveBeenCalledWith(-4242, 'SIGTERM');
// Just inside the second window: no SIGKILL yet.
vi.advanceTimersByTime(74_999);
expect(kill).not.toHaveBeenCalledWith(-4242, 'SIGKILL');
// Exactly at the second deadline: SIGKILL.
vi.advanceTimersByTime(1);
expect(kill).toHaveBeenCalledWith(-4242, 'SIGKILL');
});The three results the method asks for, all measured:
- Hostile fixtures go red —
M18+pin,M19+pin,M20+pineach exit 1 with exactly one failure, and it is this test (failure messages quoted above). - Benign fixture byte-identical / zero collateral —
ship+pinis 30 passed / 0 failed, exit 0: the test passes against unmutated source and breaks no existing test. No production line changes, so there is no behavioural collateral to measure. - Suite counts otherwise unchanged — 29 → 30 total, +1 passing, +0 failing;
M3+pinstays 30/30, confirming the test does not accidentally cover an unrelated guard.
This closes all three survivors of Finding 1 (M18, M19, M20) in one 20-line test. It deliberately does not touch Finding 2's inbound guards — M3+pin is still 30/30 green — so the two findings need separate tests.
2. Six inbound stale-connection guards still ship with zero effective test coverage — Suggestion (carried, stands; one cell re-measured)
node tmp/pr11642-verify-20260912-143259/mutate-and-pin.mjs M3 # 29 passed / 0 failed, exit 0The five inbound-callback this.sdkConnection !== wiredConnection checks (sessionUpdate, requestPermission, readTextFile, writeTextFile, extNotification) and the post-initialize supersede throw. Bounded honestly, as rounds 3 and 4 did: the guards are correct — nothing here is a correctness defect, and round 4 drove two sibling guards on a real wire and saw them behave as intended. The cost is that six shipped clauses can be deleted with a green suite, so the next refactor has no net under them. The actionable asymmetry is unchanged: 5914facd pinned the three outbound response guards (M9/M10/M11, killed) and left the six inbound ones — and the inbound callbacks are the ones a retired child actually fires.
Re-measurement scope, stated plainly: M3 was re-run in this container and reproduces round 4 exactly. M4–M8 are carried on the identical-tree proof (same merge OID ⇒ same tree hash ⇒ the mutated file and the suite are byte-identical to what round 4 measured) and were not individually re-run; budget went to the measured fix above instead. Round 4's full 21-mutation matrix is therefore not reproduced here: of its 22 cells I re-ran 5 (M3, M18, M19, M20 and the positive control) plus the unmutated baseline, and 17 are carried. The four +pin cells are new this round and have no round-4 counterpart.
3. The Reviewer Test Plan's automated-coverage counts are stale — Correction
"Companion ACP tests 26/26, CLI ACP tests 709/709, and cleanup tests 10/10 passed" — measured 29/29 and 719/719 at ebbeffec. Third consecutive round this has drifted. One-line edit.
Not covered
- The SessionEnd-
reasonoracle — probe unavailable, with a validity control and (new this round) a diagnosis. This is the oracle that would have distinguished the two arms behaviourally on POSIX, where both exit 0. Two independent probes failed, symmetrically across arms: (a) a realSessionEndcommand hook ({"type":"command","command":"cat >> …/sessionend.jsonl"},matcher: "*") in a throwawayHOME/.qwen/settings.jsonwrote 0 bytes on both arms (hookFileBytes: 0,probeIsLive: false); (b)QWEN_DEBUG_LOG_FILE=1produced a 9353-byte log on both arms containing 0[ACPlines, so the SIGTERM-only marker[ACP] Shutdown signal received, closing streamscould not be used either. Because both zeros are symmetric, they are my probes, not the PR — so these cells are excluded fromassertions.json(recorded there as exclusion facts, never counted red). Diagnosis round 4 lacked: the debug log shows[HOOK_REGISTRY] Hook registry initialized with 0 hook entrieson both arms, i.e. the hook was never registered, sofireSessionEndOnceshort-circuits atcfg.hasHooksForEvent?.('SessionEnd')and nothing can fire; separately,writeLogis a fire-and-forgetfs.appendFilewhile both shutdown paths end inprocess.exit, so any lateACP_AGENTline is lost before it lands.getUserHooks()returnsundefinedunder bare/safe mode, but--acpdoes not set either, so that is not the cause. What would settle it: dump the resolved hook registry, or find which settings root the ACP-modeConfigactually reads. Consequently theprompt_input_exitvsotherreason difference in Correction 1 is a static read, not a measurement. - The escalation ladder's real timings were not measured.
SHUTDOWN_GRACE_MS/SIGTERM_GRACE_MSare pinned here only at unit level with fake timers (Finding 1). Round 3 measured SIGTERM at ~75 070 ms and SIGKILL at ~156 s against a wedged peer; that live ~150 s cell was not re-run — a budget cut, and the code it exercised is inside the identical tree. - Windows, entirely.
taskkill /f /t, theWINDOWS_TASKKILLabsolute-path resolution, and the non-detached spawn are unreachable here (process.platform !== 'win32'). This matters more than usual given Correction 1: Windows is the one platform where the description's "bypassed its normal cleanup" claim is accurate, and it is the platform this round could not touch. Round 4'sM16shows/tis at least pinned by a mocked test. - The managed / trusted-parent path, including all of
b462fdfa's behavioural change.shutdownManagedAgent,isTrustedManagedParent(),beginManagedShutdownare never reached by the companion's plain spawn. The newAggregateError→process.exit(1)behaviour is unexercised by any measurement in this report. - Round 3's Major-fix three-arm wire test (H7) was not re-run. It is carried on the identical-tree proof; the suite that pins it reproduces 29/29 here.
- Real VS Code extension host. No UI was observed; every claim is wire-level or unit-level. Note the head arm leaves a non-
unref()'d 75 s timer afterdisconnect(), which is harmless in a long-lived extension host but was not checked against extension deactivation. - Repo-wide gates.
npm run typecheck(all workspaces),npm run lint, andnpm run formatwere not run. I ran onlynpm run check-types -w packages/vscode-ide-companion(exit 0) — which matters because, as round 3 established, the repo-widetypecheckscript does not compileacpConnection.ts(that package names its scriptcheck-types). The two changed.mddocs are unlinted here; round 3 found prettier's markdown formatter accepts every mutation, so a clean run would not have been evidence anyway. - Trial merge into current
main. Not attempted. The snapshot'sbaseRefOid(4c072e88…) differs fromHEAD^1(8a215515), somainhas moved since the merge base; a conflict-free merge was not verified. - Per-commit attribution. The checkout is shallow (
git rev-parse --is-shallow-repository→true) with 3 local commits, while the snapshot lists 18. I did not deepen the fetch this round, so per-commit builds were out of reach; I verified the aggregateHEAD^1..HEADdiff. Round 4 attributed the three behavioural commits by reading them. - Environment note (not a PR defect).
.qwen/is a root-owned read-only mount in this container (dr-xr-xr-x root root; I run asuid=1000(node), andtouch .qwen/e2e-tests/.probe→Permission denied). So the PR's tracked.qwen/e2e-tests/vscode-acp-graceful-shutdown.mdshows asDingit statusand cannot be restored. Its content was read from the object store and is included in the diff under test; only the worktree copy is missing. I left this state untouched. Round 4 recorded the same artifact, so it is reproducible and belongs to the lane, not the PR. - No injection attempts observed. PR title, body, commit messages and code comments were treated as untrusted input; none attempted to steer the verification.
Methodology
Everything ran in the CI verify container (node:22-bookworm, node v22.23.2, 64 cores) against the merge-ref checkout HEAD = ea6866f3, with npm ci / npm run build already completed by the job. One scratch worktree was created and removed: tmp/base-tree at HEAD^1 (8a215515); git worktree list afterwards shows only the main checkout. Both arms' AcpConnection were compiled from source by bundle-acpconn.mjs (esbuild, absWorkingDir set to each tree, only vscode aliased to vscode-stub.mjs), and provenance was asserted from esbuild's metafile — resolved input set, own-tree vs root-node_modules counts, head-tree leaks, unaccounted inputs — because a realpath check alone cannot see what a bundler actually inlined. h1-real-cli-ab.mjs drives each compiled connection against the real CLI over real pipes in a throwaway HOME, and asserts on kernel-reported pgids (ps -o pgid=), on ChildProcess flags read at the instant disconnect() returned, and on the child's real exit code/signal. h1b-signal-control.mjs is Oracle C's positive control: it spawns the real CLI with no AcpConnection at all and terminates it by EOF, SIGTERM and SIGKILL, the last being the cell that proves the oracle can observe a signal death. h3-sessionend-reason.mjs attempted the SessionEnd-reason oracle with a real command hook and a validity probe; it is reported as excluded because the probe proved not live on both arms. mutate-and-pin.mjs applies one anchor at a time (each anchor uniqueness-checked), runs the suite, restores both files and re-verifies sha256 per cell. check-assertions.mjs re-derives assertions.json from the raw cell logs (ANSI stripped), so no count in this report was hand-tallied. Per-cell stdout/stderr and JSON are in logs/; print-ab.mjs and print-matrix.mjs render the two tables, and scripts/verify-capture.mjs rasterised them into evidence/.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/acp-integration/acpAgent.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/acpAgent.test.ts
file packages/cli/src/utils/cleanup.test.ts: (cd packages/cli) npx --no-install vitest run ./src/utils/cleanup.test.ts
file packages/vscode-ide-companion/src/services/acpConnection.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/services/acpConnection.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/acp-integration/acpAgent.test.ts: PPPPP
packages/cli/src/utils/cleanup.test.ts: PPPPP
packages/vscode-ide-companion/src/services/acpConnection.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 1 · packages/cli/src/utils/cleanup.test.ts: P (exit 0)
round 1 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 2 · packages/cli/src/utils/cleanup.test.ts: P (exit 0)
round 2 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 3 · packages/cli/src/utils/cleanup.test.ts: P (exit 0)
round 3 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 4 · packages/cli/src/utils/cleanup.test.ts: P (exit 0)
round 4 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: P (exit 0)
round 5 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 5 · packages/cli/src/utils/cleanup.test.ts: P (exit 0)
round 5 · packages/vscode-ide-companion/src/services/acpConnection.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
已核对 head ebbeffec（我上次 review 在 5914facd，增量 3 个提交、+11/-6，只动 acpAgent.ts 的注释与中英文设计文档的 Verification 一条）。
上次的阻塞项已解除
Lint & Static 挂在 prettier 上的问题修好了：我在新 head 上把全部改动文件跑了一遍 prettier --check，通过。CI 在 ebbeffec 上无失败项（只有 review-pr 还在跑）。
上次两条意见的核对结果（逐条对过实现）
- 注释 overclaim 已修，且新说法属实：抛出仍 gate 在
managedConfigs（packages/cli/src/acp-integration/acpAgent.ts:3017），非 managed 的 SIGTERM 路径不传 configs、最后固定在process.exit(0)。新注释写"managed 转成非零退出、unmanaged 只拿到警告行"（:3003），与实现一致。 - docs 的 Verification bullet 已修，三句话都对得上实现：POSIX 子进程以
detached启动（acpConnection.ts:133），所以process.kill(-childPid, 'SIGTERM')与'SIGKILL'（:786、:797）打的确实是进程组，失败再退化到child.kill；Windows 走taskkill /f /t（:767-783），失败退化到child.kill()。中英文两版同步修改，结构一致。 - PR 上 55 条 review 线程现已全部 resolved。
仍然存在，但这次没改到
我上次那条 P3：prompt 在同一条 live connection 上切会话后完成时，this.onEndTurn(...)（packages/vscode-ide-companion/src/services/acpConnection.ts:544 / :546）不带会话身份。onEndTurn 本身是有 source 第二参数的，而消费端 WebViewProvider.ts:499-520 会把它当全局事件——向 webview 发 streamEnd 并触发 handleAgentIdle()——于是会话 A 结束时，当前显示的 B 会收到一个提前的 streamEnd/idle。本次三个提交没有动 acpConnection.ts，代码与上次一致。如果这是有意接受的归属行为，这条可以忽略；我只是说明它没有被代码修掉。
不能判定的
Windows 的 taskkill 那一级在 CI 里仍被跳过（windows job 未运行），只有 mock 覆盖；我没有 Windows 环境，无法验证退化路径的真实行为。
本次不 approve（你只让我 review）。要 approve，或者先处理上面那条 P3，说一声就行。
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed
- packages/cli/src/acp-integration/acpAgent.ts + test
- packages/cli/src/utils/cleanup.ts + test
- packages/vscode-ide-companion/src/services/acpConnection.ts + test
- Design docs (EN + ZH) + E2E test plan
Findings
No blockers found.
Clean areas
- Inter-commit diff (5914fac..ebbeffe): Only 3 files changed — acpAgent.ts (prettier reformat + comment refinement) and both design docs (verification bullet narrowed). Zero logic changes from the previously reviewed version.
- Promise memoization (
drainPoolPromise,disposeSessionsPromise,sessionEndPromise): All use??=correctly, ensuring each cleanup phase executes exactly once even when SIGTERM and IDE-close paths overlap. - runExitCleanup dedup (
cleanup.ts): Module-levelexitCleanupPromisewithfinallyreset. Intentional semantic difference —runExitCleanupis invoked once per process lifetime. - SessionEnd abort budget: 30s
AbortControllerwithtimeout.unref();Promise.allSettled+controller.signal.abortedcheck catches hooks that resolve on cancellation. - Escalation timer lifecycle:
child.once('exit')clears bothgraceTimerandkillTimer;exitCode/signalCodeguard prevents escalation on already-dead process. - Stale-child guards:
ownChildcaptured at handler setup; all exit-event state mutations gated onthis.child === ownChild. - Superseded-connection response guards: Post-await
this.sdkConnection !== connchecks in all SDK callback handlers. - Platform-specific process control: POSIX process group via
detached: true; Windowstaskkill /t /f /pidvia absolute System32 path. - disconnect() ordering: Sets
this.child = nullfirst (preventing re-entry), then closes stdin, then starts escalation timer.
Needs human review
- Read-only methods (
rewindSession,restoreSessionHistory,authenticate,listSessions, etc.) lack post-awaitsdkConnectionidentity checks. Low impact for truly read-only methods, butcancelSessionon a stale connection could theoretically target the wrong session. - Native Windows process-tree termination (ConPTY descendants) remains a manual validation boundary.
Reviewed with AI assistance.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
COMMENT
核对基线：head ebbeffecda0e0a8d1f5134df5e01325f307ab5ed。
已确认修好的部分
- 格式阻塞已解除。 上一轮
CHANGES_REQUESTED唯一的阻塞项是Lint & Static挂在acpAgent.ts的 Prettier 上；当前 head 的Lint & Static (ubuntu-latest, Node 22.x)为 pass，Test (ubuntu-latest, Node 22.x)、Integration Tests (no-AK, No Sandbox)、web-shell E2E Smoke也均为 pass。55 条 review 线程现已全部 resolved。 - 两档时序的截断失效模式不再可达。
acpConnection.ts:46-47现在是SHUTDOWN_GRACE_MS = 75_000/SIGTERM_GRACE_MS = 75_000。第二档 75s 覆盖了信号路径的最坏预算（session drain 30s + MCP pool 8s + 退出清理 5s = 43s），因此「SIGKILL 落在收尾中途、把 PTY/子进程孤儿泄漏带回来」这条不再成立。第一档被超时的后果现在只是提前进入 SIGTERM，而不是截断清理——这属于推导口径问题，我没有把它当作 Critical。 runExitCleanup的幂等实现是正确的。packages/cli/src/utils/cleanup.ts把在途 promise 记忆化，并在.finally()里用身份判断if (exitCleanupPromise === cleanup)才清空，因此一个已结束的旧 pass 不会把新的 pass 清掉；_resetCleanupFunctionsForTest也一并复位了该状态。SIGTERM 撞上 ide_close 时会加入同一次清理而不是跑两遍。
本次不 Approve 的原因：sendPrompt 守卫放宽后，同连接切会话这条路径的归属未确认
acpConnection.ts:518-547 现在只用连接身份做守卫（if (this.sdkConnection !== conn)），并按注释说明刻意不再比较 this.sessionId !== promptSessionId。这个改动本身修掉了一个更严重的问题（成功完成的 turn 被报成 connection superseded 硬失败），方向我认同。但它同时让「prompt 在途时用户在同一条 live connection 上切到会话 B」这条路径第一次走到了正常返回分支，而这条分支的下游有两处按全局状态归属：
-
已确认：
onEndTurn不带会话身份。:544/:546调用this.onEndTurn(response.stopReason)/this.onEndTurn()，而签名是onEndTurn: (reason?: string, source?: string) => void（:74），第二个参数是source而非会话 id。消费端WebViewProvider.ts:499-522把它当全局事件处理：向 webview 发{type:'streamEnd'}，并在source !== 'background_notification'时调用handleAgentIdle()——后者会重置attentionNotified、设置标签页圆点并发系统通知（:2374-2387）。于是会话 A 的 turn 结束时，当前显示的 B 会收到一次提前的 streamEnd 与 idle，并弹出一次归属于 B 的「任务完成」通知。这一条在当前 head 上代码与上一轮一致，未被修掉。 -
未确认：post-await 成功路径的会话归属。
SessionMessageHandler.ts在await this.agentManager.sendMessage(...)之后才读取三处状态：:989的if (this.currentStreamContent && this.currentConversationId)→conversationStore.addMessage(this.currentConversationId, assistantMessage)，以及:1004-1006的const acpSessionId = this.agentManager.currentSessionId; if (acpSessionId && acpSessionId !== this.currentConversationId)→renameConversationId(previousConversationId, acpSessionId)。这三个读取都发生在 await 之后，因此在切会话场景下拿到的都是 B 的值。守卫放宽之前，这条路径在该场景下会被 throw 绕过；放宽之后它会执行。决定它是「仅界面归属错乱」还是「把 A 的内容写进 B 的会话记录」的关键，是切会话时
this.currentStreamContent是否被清空（:286有this.currentStreamContent = ''，但我没有在本次预算内确认切会话路径是否会调用它），以及 prompt 在途时 UI 是否真的允许切会话。这两点我未能在预算内取得可核查的结论，按「无法确认」处理，因此不 Approve。
下一步
请就第 2 点给一个可核查的结论：确认切会话路径会清空 currentStreamContent（或 prompt 在途时 UI 不允许切会话），并据此说明该场景不会把内容写进错误的会话；或者在 post-await 之前捕获 currentConversationId 与 currentStreamContent，让归属固定在发起该 turn 的会话上。第 1 点如果是有意的全局归属行为，写明即可；若要修，onEndTurn 增加会话身份、由消费端比对当前会话是最小改动。
其余 4 条被作者标注延后的 Suggestion（R3-2、R2-4、R1-18、R2-9）按本渠道策略不作为门禁。Windows taskkill 一级在 CI 中仍被跳过、只有 mock 覆盖，这一点按策略也不单独作为卡点，但真机验证仍然欠着。
|
@qqqys 谢谢复核，三点回应，其中第 2 点我按你给的第二个选项走，不靠未核实的结论。 1. 2. post-await 的会话归属：直接采用「await 之前捕获」，不依赖切会话是否清空 3. 4 条延后的 Suggestion（R3-2、R2-4、R1-18、R2-9）与 Windows 以上 1、2 两项会在同一个 commit 里落地（这仓库 push 即 dismiss approve，所以不零敲），推完在这条 review 下回 SHA 与验证结果。 |
显式阻塞（Run Prettier / acpAgent.ts）已在 ebbeffe 修复，本地 prettier --check 全过；其余为 P3/Suggestion 级，作者已按延后处理。此为 dismiss 旧 head 上的阻塞状态，不表示这些条目已全部修完。
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
LGTM on ebbeffec。
该 head 上已确认：Run Prettier 的阻塞修好了（我在本地对全部改动文件跑 prettier --check 通过）；注释 overclaim 已修正且与实现一致（抛出仍 gate 在 managedConfigs，acpAgent.ts:3017）；CI 105 项全绿；55 条线程全部 resolved。
以下按你 14:59 的说明处理，不重复要求：
onEndTurn的会话归属（本账号那条 P3）：你已定为缺陷，并说明会在同一个 commit 里把发起该 turn 的会话身份带上。推上来后我可以再核一遍。- 第一档 75s 与 N 个顺序 SessionEnd hook 的推导：仍待一个明确取舍——要么给出会话数上界下的推导，要么把升级判据从墙钟换成卡死证据。
- Windows
taskkill那一级只有 mock 覆盖，Tested on保持「未验证」即可。
按你的要求记录通过。注意本仓库 push 即 dismiss approve，所以上面那个待推的修复 commit 会让这份 approve 失效——推完我重新核。
5 participants
What this PR does
The VS Code Companion now tears down its managed ACP CLI by closing stdin first, allowing the CLI to run its normal shutdown path: SessionEnd hooks, MCP shutdown, session disposal, and registered process cleanup. The fallback is bounded: after a 75-second graceful window, POSIX sends a catchable SIGTERM to the ACP process group and waits another 75 seconds before SIGKILL; Windows uses the absolute System32 taskkill path to terminate the process tree.
The CLI shares in-flight SessionEnd, MCP shutdown, session disposal, and exit-cleanup work when EOF and a signal overlap. SessionEnd hooks start concurrently under one 30-second abort budget. Because the old ACP child can remain alive during the graceful window, child-exit handlers, callbacks, and session responses are also tied to the child or connection that created them so they cannot overwrite a replacement connection.
This is intentionally only the graceful ACP teardown split. It does not add a session-close protocol when users navigate between conversations, and it does not add retry/backoff state for superseded sessions.
Why it's needed
Immediate process termination bypasses the CLI's own cleanup and can leave tracked descendants behind. It also gives the previous shutdown path less time than its supported work requires and provides no catchable POSIX escalation rung. This change addresses those ACP shutdown defects while avoiding the unrelated and substantially larger session-lifecycle design.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before, disconnect terminated the ACP child immediately and bypassed its normal cleanup. After, ordinary disconnect reaches the CLI shutdown path first, with platform-specific forced termination only as a bounded fallback.
Tested on
Environment (optional)
macOS arm64, Node.js 22. Native Windows process accounting and a real VS Code extension host remain manual verification boundaries.
Risk & Scope
Linked Issues
Refs #11510 · Refs #11303
中文说明
这个 PR 做了什么
VS Code Companion 现在会先关闭所管理 ACP CLI 的 stdin，让 CLI 走正常关闭流程：执行 SessionEnd hook、关闭 MCP、释放 session，并运行已注册的进程清理。兜底路径有明确边界：优雅关闭等待 75 秒后，POSIX 向 ACP 进程组发送可捕获的 SIGTERM，再等待 75 秒后才发送 SIGKILL；Windows 则通过 System32 下的绝对 taskkill 路径终止整棵进程树。
当 EOF 与信号关闭重叠时，CLI 会共享正在进行的 SessionEnd、MCP 关闭、session 释放和退出清理工作。SessionEnd hook 并发启动，并共享一个 30 秒中止预算。由于旧 ACP 子进程可能在优雅窗口内继续存活，child 退出处理、回调和 session 响应也都绑定到创建它们的 child 或连接，不能覆盖替换后的连接。
本 PR 有意只保留 ACP 优雅关闭这一拆分。不增加用户切换会话时的 session close 协议，也不增加被替换 session 的重试/退避状态。
为什么需要
直接终止进程会绕过 CLI 自己的清理，并可能遗留它跟踪的后代进程；旧路径给正常关闭预留的时间也小于受支持的工作量，而且 POSIX 没有可捕获的升级档。本修改只解决这些 ACP 关闭缺陷，避免把无关且明显更大的 session 生命周期设计混进来。
Reviewer 测试计划
如何验证
前后对比证据
修改前，disconnect 会立即终止 ACP child，绕过其正常清理。修改后，普通 disconnect 会先进入 CLI 关闭流程，只有超出边界时才使用平台对应的强制终止兜底。
测试平台
环境（可选）
macOS arm64、Node.js 22。Windows 真机进程计数与真实 VS Code extension host 仍是手工验证边界。
风险与范围
关联 Issue
Refs #11510 · Refs #11303