test(core): pin the deferred exit-time PTY release against trailing output

[yiliang114]

test(core): pin the deferred exit-time PTY release against trailing output

What this PR does

Test-only changes to web-terminal-registry.test.ts. The production exit-time release logic already landed on main via #11572; this PR pins it with tests and fixes the test scaffolding.

Two changes:

1. Mock isolation fix: the shared onData/onExit describe-scope fake's disposeData spy is captured by value at registration time, so a stale exit-time release from one test can't contaminate another test's spy. The listener-identity check in the dispose closure prevents a stale deferred release from re-attaching and disposing the wrong session.

2. New trailing-output test: pins the one-tick defer behavior — output queued behind onExit still reaches the scrollback before the release runs. The load-bearing regression signal is the intermediate expect(disposeData).not.toHaveBeenCalled() before onData; the post-tick disposeData assertion verifies eventual cleanup.

Why it's needed

The production fix in #11572 already landed, but the trailing-output ordering and test-fake isolation still need regression coverage. This follow-up keeps the deferred-release contract observable without changing runtime code.

Reviewer Test Plan

How to verify

cd packages/core && npx vitest run src/services/web-terminal-registry.test.ts — all 33 tests pass.

The trailing-output test fails if the release is made synchronous: the intermediate no-dispose assertion catches the listener being detached before trailing output. The post-tick assertion confirms the deferred release still happens.

Evidence (Before & After)

N/A — this PR changes tests only. The reviewed head passes all 33 tests, and the added timing assertion fails if release occurs before trailing output.

Tested on

OS	Status
🐧 Linux	✅ tested

Risk & Scope

• Main risk or tradeoff: test-only change; the assertions model listener disposal and event-loop ordering, with no production behavior change.
• Not validated / out of scope: native Windows PTY execution and the production implementation already landed in fix(core): free an exited web terminal's PTY resources at exit time #11572; no user-facing behavior changes.
• Breaking changes / migration notes: none.

Linked Issues

Follow-up to #11572; issue #11353 was resolved by that production PR.

中文说明

这个 PR 做了什么

仅修改测试。退出时释放的生产代码已通过 #11572 合入 main；本 PR 为 trailing-output 顺序和测试 fake 隔离补充回归覆盖，在不改变运行时代码的前提下锁定延迟释放契约。

两处改动：

1. Mock 隔离修复：共享的 onData/onExit describe-scope fake 的 disposeData spy 在注册时按值捕获，避免一个测试的退出时释放污染另一个测试的 spy。dispose 闭包中的 listener 身份检查防止陈旧的延迟释放重新挂接并释放错误的会话。

2. 新增 trailing-output 测试：锁定一个 tick 的延迟行为——onExit 之后排队的输出在释放运行前仍能进入 scrollback。真正承重的回归信号是 onData 之前的 expect(disposeData).not.toHaveBeenCalled()；tick 之后的 disposeData 断言只用于确认最终清理仍会发生。

为什么需要

生产修复已随 #11572 合入，但 trailing-output 的时序和测试 fake 的隔离仍需要回归覆盖。本跟进 PR 让延迟释放契约可被测试观察，同时不改变运行时代码。

评审者测试计划

如何验证

cd packages/core && npx vitest run src/services/web-terminal-registry.test.ts —— 33 个测试全部通过。

如果释放改为同步，onData 之前的未调用断言会立即失败；tick 之后的断言则确认延迟释放最终仍会执行。

证据（修复前后）

N/A —— 本 PR 仅修改测试。当前 head 下 33 个测试全部通过；新增的时序断言会在释放早于尾部输出时变红。

测试平台

操作系统	状态
🐧 Linux	✅ 已测试

风险与范围

• 主要风险或权衡：仅测试变更；断言建模 listener 释放和事件循环时序，不改变生产行为。
• 未验证 / 超出范围：原生 Windows PTY 执行以及已在 fix(core): free an exited web terminal's PTY resources at exit time #11572 合入的生产实现；没有用户可见行为变化。
• 破坏性变更 / 迁移说明：无。

关联 Issue

跟进 #11572；问题 #11353 已由该生产 PR 解决。

[qwen-code-ci-bot]

✅ Qwen Triage finished — view run. See the stage comments in this thread for the result.

✅ Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

[qwen-code-ci-bot]

Thanks — re-run picked this up after the body edit, and the template gate that stopped the last pass is cleared.

Template looks good ✓ — ## Why it's needed, ### Evidence (Before & After), ## Risk & Scope and ## Linked Issues are all back, and ## Linked Issues now uses the accurate non-closing form ("Follow-up to #11572") rather than a Fixes keyword aimed at an issue #11572 already closed. ### Environment (optional) is the only absent heading and it is the only one the template marks optional.

Problem: a real, verified coverage gap — not theoretical hardening. handleExit ends in setImmediate(() => this.releasePtyResources(session)) (web-terminal-registry.ts:296), and the comment above it states the invariant outright: "setImmediate runs after the poll-phase callbacks already queued this tick, so trailing output still reaches buffer before the data listener is detached." Nothing on main pins that ordering. The three sibling tests (frees an exited session at exit time…, still replays buffered scrollback…, does not free an exited session twice…) all await setImmediate first and then assert disposeData fired once — and because releasePtyResources is idempotent per session (:541), every one of them passes identically if the release is made synchronous. So the documented defer currently has zero executable coverage. This PR is the test that was missing from #11572.

Direction: aligned. AGENTS.md asks for collocated regression tests, and this area has churned through #11303 / #11313 / #11353 — a timing invariant that three existing tests cannot distinguish is exactly the kind of thing that regresses silently.

Size: packages/core/src/services/** is a core path, so the two-tier gate ran. Breakdown: 0 production logic lines, 41 test lines (one *.test.ts file, +40/−1), 0 generated/schema. The file is excluded from the size calculation, Tier 1 does not apply (title is test, not refactor, and there is no production surface at all), and the 500/1000-line advisories are not reachable. No maintainer escalation on scope.

Approach: minimal, and it follows the file's own conventions rather than inventing new ones — same osPlatform.mockReturnValue('win32') + create + onExit + setImmediate shape and same terminal:exit-* id naming as the three siblings it sits beside. Two edits, both needed: the new test, and the fake's dispose gaining a listener detach. No drive-by refactors, no production churn, nothing to split out.

One framing nit for the description, not the code: change #1 is described as a "mock isolation fix" for a stale release contaminating another test's spy, but main's shorthand return { dispose: disposeData } already captured the spy by value at object-literal creation, so that contamination did not exist before this PR. The by-value capture is load-bearing because this PR turns dispose into a deferred arrow that would otherwise late-bind the describe-scope let. The code is right; the "fix" wording just overstates a pre-existing bug that wasn't there.

Risk: no elevated risk signals — the Stage 1e high-risk path scan matches nothing once *.test.ts is filtered out, and the PR touches no production file.

Moving on to code review. 🔍

中文说明

感谢 —— 本次 re-run 是在正文修改之后触发的，上一轮拦住 PR 的模板闸门已经解除。

模板完整 ✓ —— ## Why it's needed、### Evidence (Before & After)、## Risk & Scope、## Linked Issues 都补回来了，而且 ## Linked Issues 用的是准确的不带关闭关键字的写法（"Follow-up to #11572"），没有把 Fixes 指向一个已被 #11572 关闭的 issue。唯一缺席的 ### Environment (optional) 正是模板里唯一标注 optional 的小节。

问题：真实且已核实的覆盖缺口，不是理论性加固。handleExit 以 setImmediate(() => this.releasePtyResources(session)) 收尾（web-terminal-registry.ts:296），其上方注释把不变量写得很明白："setImmediate 在本 tick 已排队的 poll 阶段回调之后运行，因此尾部输出仍能在 data listener 被摘除前进入 buffer。" 而 main 上没有任何测试钉住这个时序。三个同级测试（frees an exited session at exit time…、still replays buffered scrollback…、does not free an exited session twice…）都是先 await setImmediate 再断言 disposeData 调用一次 —— 由于 releasePtyResources 按会话幂等（:541），把释放改成同步后它们全部照样通过。也就是说这个被写进注释的延迟目前在 main 上零可执行覆盖。本 PR 补的正是 #11572 缺的那个测试。

方向：对齐。AGENTS.md 要求测试与源码同目录，而这个区域已经过 #11303 / #11313 / #11353 的反复修改 —— 一个现有三个测试都区分不出来的时序不变量，正是最容易悄悄回归的东西。

规模：packages/core/src/services/** 属于核心路径，两级闸门已执行。明细：生产逻辑 0 行，测试 41 行（单个 *.test.ts 文件，+40/−1），生成/schema 0 行。该文件不计入规模统计，Tier 1 不适用（标题是 test 而非 refactor，且完全没有生产面），500/1000 行提示也够不着。范围上无需上报维护者。

方案：改动最小，并且沿用了文件自身的约定而不是另起一套 —— 与紧邻的三个同级测试相同的 osPlatform.mockReturnValue('win32') + create + onExit + setImmediate 结构，以及相同的 terminal:exit-* id 命名。两处编辑都是必要的：新增测试，以及 fake 的 dispose 增加 listener 摘除。没有顺手重构，没有生产代码扰动，无需拆分。

一个关于描述（而非代码）的措辞小问题：改动 #1 被描述成修复"陈旧释放污染另一个测试的 spy"的"mock 隔离修复"，但 main 上的简写 return { dispose: disposeData } 在对象字面量创建时就已经按值捕获了 spy，所以本 PR 之前并不存在这种污染。按值捕获之所以是承重代码，是因为本 PR 把 dispose 改成了延迟执行的箭头函数，否则会晚绑定 describe 作用域的 let。代码是对的，只是"fix"这个说法夸大了原本并不存在的 bug。

风险：无升级风险信号 —— 过滤掉 *.test.ts 后 Stage 1e 高风险路径扫描无任何命中，且本 PR 不触碰任何生产文件。

进入代码审查 🔍

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at b1e4f307019d011af98dfc916a219a9fc7a6174e · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Code review

No Critical findings. The one blocking item that two independent reviewers raised on earlier heads is genuinely closed here, and I re-derived it against the production code rather than taking the fix on faith.

What I would have written independently. To pin a one-tick defer you need an observation between the trigger and the tick, because anything asserted after await setImmediate is true for synchronous and deferred releases alike. So: fire onExit, assert the release has not run yet, emit the trailing byte, advance one tick, then assert the release ran and the byte is in the snapshot. That is exactly what :499 does — the PR did not miss a simpler path, and I have nothing to propose that would be smaller.

Why the added assertion actually discriminates (static trace; see the verification note below for what this is and isn't). The chain that makes :514 load-bearing:

• dataDisposable = spawned.onData(handleData) is assigned synchronously inside create() at web-terminal-registry.ts:314, and is on the session object built at :372 — so by the time the test's await registry.create(…) resolves, session.dataDisposable is the fake's disposable. This mattered: had it been attached later, a synchronous release would hit session.dataDisposable?.dispose() with undefined, no-op, and :514 would pass for the wrong reason.
• releasePtyResources (:540) disposes it at :543. Making the release synchronous therefore calls the fake's dispose inside onExit({ exitCode: 0 }), i.e. before :514 runs.
• The fake's dispose calls disposeDataSpy (:77), which is the same spy object the test reads as disposeData — beforeEach assigns it once and nothing reassigns it mid-test.

So under a synchronous release :514 fails. Under a removed release :514 passes but expect(disposeData).toHaveBeenCalledOnce() fails. Both regression directions are covered, and — the point chiga0 and the dev-bot were making — neither depends on the fake's listener detach any more.

Status of the prior findings, checked against b1e4f307 as it stands:

• R2-1 / chiga0's :515 minor / dev-bot's blocking item — fixed by this diff. All three asked for the same one-liner, expect(disposeData).not.toHaveBeenCalled() placed before the setImmediate await. It is present at :514, in the right position, against the right spy.
• R1-1 (detach lives in only one of this file's three spawn fakes) — still stands, now non-blocking. The two others at :577 and :640 still read return { dispose: disposeData }. That divergence is no longer a correctness risk precisely because of the fix above: those fakes sit inside it.skipIf(process.platform === 'win32') blocks covering killPtyTree, they never assert on defer timing, and nothing outside :499 depends on detach semantics. Worth tidying if someone is in the file anyway; not worth a round.
• The by-value capture at :77 is correct and necessary. The earlier round that called it a no-op was wrong and the later round corrected it: { dispose: disposeData } captured by value at object-literal creation, but the new arrow dispose runs deferred, so referencing disposeData inside it would late-bind the describe-scope let and let a release escaping one test increment a later test's spy. :77 preserves the original semantics. The identity guard at :82 is the matching half — it stops a stale dispose from detaching the current test's listener.

Two nits, neither blocking. The comment at :510-513 says the fake's detach "loses that tail too", which reads as a claim about the passing path when it is only true under the synchronous-release mutation; and toMatchObject here is looser than the toEqual the three sibling snapshot tests use — deliberate-looking, since exitCode and workspaceCwd are already pinned by still replays buffered scrollback after the exit-time release, but toEqual would make the buffer-exactness assertion (output is exactly 'trailing', no early-output flush leaking in) self-evident.

House style is clean: ESM, no any, collocated test, kebab-case filename unchanged, and the comment density matches what this file already carries. No AGENTS.md violations.

Test evidence

Unattended CI run (GITHUB_EVENT_NAME=issue_comment), so per the skill's rules I did not build or execute anything from this PR — no vitest, no npm, no checkout. Everything below is this PR's own CI, read off the API for the reviewed commit. 124 check-runs on b1e4f307, zero failures; the only non-terminal entry is the bot's own review-pr orchestration job.

Check	Conclusion
Test (ubuntu-latest, Node 22.x)	success
Lint & Static (ubuntu-latest, Node 22.x)	success
Integration Tests (no-AK, No Sandbox)	success
Desktop Shell (ubuntu-22.04)	success
Desktop Shell (windows-2022)	success
web-shell E2E Smoke (ubuntu-latest, Node 22.x)	success
Test (macos-latest, Node 22.x)	skipped
Test (windows-latest, Node 22.x)	skipped
Integration Tests (CLI, No Sandbox)	skipped
review-pr	in_progress

Two things about that table worth saying out loud. Test (windows-latest, …) is skipped, yet the new test forces osPlatform.mockReturnValue('win32') — that is fine and intentional, because the file mocks node:os platform (:24-30) specifically so the ConPTY release path is reachable on Linux CI, and the three sibling win32-forced tests already prove the pattern works there. And green CI here shows the suite passes on this head; it says nothing about whether the new test would fail on a regression.

Not verified: the mutation behavior — that :499 goes red when web-terminal-registry.ts:296 is made synchronous. I could not execute it in this lane, so the discriminating-power argument above is a hand trace of the production code, not a measurement. Earlier rounds of this PR did measure it, and the author reports a local mutation check; both are their claims, not my evidence.

Sandboxed verification would settle exactly that: @qwen-code /verify — the load-bearing claim here is that the added expect(disposeData).not.toHaveBeenCalled() turns the suite red under a synchronous release and under a removed release, independent of whether the fake detaches its listener. A green suite cannot distinguish that from a test that passes either way, which is the precise failure mode this PR spent two review rounds on. The author has write access, so this is a normal run rather than a sponsored one.

Real-scenario tmux testing: N/A — test-only change with no user-visible surface, and this is the CI path where live behavior is driven only by the isolated @qwen-code /tmux job.

中文说明

代码审查：无 Critical。前几轮由两位独立评审提出的唯一阻塞项，在当前 head 上确实已经关闭 —— 我是对着生产代码重新推导的，没有直接采信"已修复"这个说法。

我自己会怎么写：要钉住一个 tick 的延迟，必须在触发和该 tick 之间插入观测点，因为任何放在 await setImmediate 之后的断言，对同步释放和延迟释放都同样成立。所以顺序是：触发 onExit → 断言释放尚未发生 → 发出尾部字节 → 推进一个 tick → 断言释放已发生且字节已进入快照。:499 做的正是这件事，没有漏掉更简的路径，我也想不出更小的写法。

新增断言为什么真的有判别力（静态推导）：dataDisposable 在 create() 内同步赋值（web-terminal-registry.ts:314），并进入 :372 构造的 session —— 所以测试 await registry.create(…) 返回时它已就位。这一点很关键：如果它是稍后才挂上的，同步释放会走到 session.dataDisposable?.dispose() 时遇到 undefined 而空转，:514 就会因为错误的原因通过。releasePtyResources（:540）在 :543 释放它，因此把释放改成同步会让 fake 的 dispose 在 onExit(...) 内部就被调用，早于 :514。而 fake 的 dispose 调用的 disposeDataSpy（:77）与测试读取的 disposeData 是同一个 spy 对象。结论：同步释放 → :514 变红；完全移除释放 → :514 通过但 toHaveBeenCalledOnce() 变红。两个回归方向都被覆盖，且都不再依赖 fake 的 listener 摘除行为。

历史结论对照 b1e4f307：R2-1、chiga0 在 :515 的 minor、dev-bot 的阻塞项 —— 已被本次 diff 修复（三者要求的是同一行断言，现位于 :514，位置和 spy 都正确）。R1-1（摘除行为只存在于三个 spawn fake 中的一个）—— 仍然存在，但已不阻塞：:577 和 :640 仍是 return { dispose: disposeData }，而正因为上面那行修复，这处不一致不再是正确性风险；那两个 fake 位于 it.skipIf(process.platform === 'win32') 块内、覆盖 killPtyTree，从不断言延迟时序。顺手统一可以，不值得再走一轮。:77 的按值捕获是正确且必要的：简写形式在对象字面量创建时即按值捕获，而新的箭头 dispose 是延迟执行的，直接引用 disposeData 会晚绑定 describe 作用域的 let；:82 的身份判断是配套的另一半，防止陈旧 dispose 摘掉当前测试的 listener。

两个非阻塞小问题：:510-513 的注释说 fake 的摘除"也会丢掉那段尾部输出"，读起来像是在描述正常通过的路径，但它只在同步释放的变异下成立；此处用 toMatchObject 比三个同级快照测试用的 toEqual 更松 —— 看起来是有意的（exitCode 和 workspaceCwd 已由 still replays buffered scrollback… 钉住），但 toEqual 能让"buffer 恰好等于 'trailing'、没有 early-output 泄漏进来"这层断言更直观。

代码风格符合仓库约定：ESM、无 any、测试同目录、文件名保持 kebab-case，注释密度与该文件既有风格一致。无 AGENTS.md 违规。

测试证据：本次为无人值守 CI 运行（GITHUB_EVENT_NAME=issue_comment），按 skill 规则我没有构建或执行本 PR 的任何代码 —— 没跑 vitest、没跑 npm、没有 checkout。下面全部是通过 API 读取的本 PR 自身 CI 结果。b1e4f307 上共 124 个 check-run，零失败；唯一未结束的是 bot 自己的 review-pr 编排任务。

表格有两点需要明说：Test (windows-latest, …) 是 skipped，但新测试强制 osPlatform.mockReturnValue('win32') —— 这没问题且是刻意的，因为该文件在 :24-30 mock 了 node:os 的 platform，专门让 ConPTY 释放路径在 Linux CI 上可达，三个同级的 win32 强制测试已经证明这个写法在此有效。另外，CI 全绿只说明套件在当前 head 通过，并不能说明新测试在回归发生时会失败。

未验证：变异行为 —— 即把 web-terminal-registry.ts:296 改成同步后 :499 是否变红。 本 lane 无法执行，所以上面关于判别力的论证是对生产代码的手工推导，不是实测。本 PR 前几轮确实做过实测，作者也报告了本地变异检查；但那些是他们的说法，不是我的证据。

沙箱验证恰好能定这件事：@qwen-code /verify —— 本 PR 的承重论点是新增的 expect(disposeData).not.toHaveBeenCalled() 在同步释放和移除释放两种情况下都会让套件变红，且与 fake 是否摘除 listener 无关。绿色套件无法把这一点和"两种情况都通过的测试"区分开，而这正是本 PR 花掉两轮评审的失效模式。作者有写权限，所以这是常规运行而非赞助运行。

真实场景 tmux 测试：N/A —— 纯测试改动，无用户可见面；且本次为 CI 路径，真实行为只由隔离的 @qwen-code /tmux 任务驱动。

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at b1e4f307019d011af98dfc916a219a9fc7a6174e · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Confidence: 4/5 — the coverage gap is real and the fix is the right one-liner; the only thing keeping this off 5 is that the test's discriminating power is a hand trace here rather than a measurement.

Stepping back: this PR is 41 lines in one test file that make a documented production invariant observable for the first time. handleExit's own comment states that the release is deferred one turn so trailing PTY output still reaches buffer before the listener is detached — and on main that sentence is protected by nothing, because the three sibling tests all await the tick before asserting, and the release is idempotent per session, so they pass identically whether it is deferred or synchronous. That is the whole justification, and it holds up. Six months from now, whoever moves setImmediate(...) at web-terminal-registry.ts:296 inline gets a red test with a comment explaining why; today they get a silent regression in a subsystem that has already produced #11303, #11313 and #11353. I would thank the author for this, not curse them.

Against my own baseline it is a match — I would have written the same observation order, and there is no smaller version of it. Every line in the diff earns its place: the new test, and the fake's dispose gaining a detach plus the by-value capture that keeps the deferred arrow from late-binding a describe-scope spy. No production surface, no drive-by edits, nothing I would ask to split out.

My reservation, stated plainly so it is not mistaken for confidence I do not have: green CI proves the suite passes on this head, and my trace proves why :514 must fail under a synchronous release — dataDisposable is wired synchronously at :314, releasePtyResources disposes it at :543, and the spy the fake calls is the same object the test reads. That chain is deterministic with no environment or timing dependency, which is why I am comfortable approving on it. But it is reasoning, not a measurement, and this specific PR spent two rounds on exactly the question "does this test actually go red?" — a question that was only ever settled by running the mutation. @qwen-code /verify would close it mechanically, and I named it in Stage 2 for that reason. If a maintainer would rather see the mutation table before merging, that is a reasonable call and nothing here argues against it.

Non-blocking, for whoever lands this: the detach now lives in one of the file's three spawn fakes (:82, vs. :577 and :640), which is cosmetic now that :514 carries the signal; the :510-513 comment describes the mutation case in words that read like the passing case; and the PR body still frames the by-value capture as fixing pre-existing cross-test contamination, when main's shorthand was already by-value — the capture is required by the new arrow, not a repair.

One process note, because it changes what the PR page shows. The previous pass stopped at the template gate and left a CHANGES_REQUESTED review from this bot on this exact commit. That gate is resolved — the body was edited afterwards and now carries all four headings it was missing — so this run went through Stages 1 to 3 normally instead of re-filing the same terminal review. The approval below is the bot's latest review on b1e4f307 and supersedes that stale request-changes; no one needs to dismiss anything, and it was not a judgment about the code either time. @yiliang114 the template fix is what unblocked it.

CI on the reviewed commit is green with zero failures out of 124 check-runs, no pull_request-event run is still in flight, the approval guardrail does not apply (same-repository branch, test-type title), and Stage 0 raised no escalation — 0 production logic lines. Approving, pinned to the reviewed commit.

中文说明

信心度：4/5 —— 覆盖缺口是真实的，修复也正是那一行该加的断言；没到 5 分的唯一原因是：本测试的判别力在这里是手工推导，而非实测。

退一步看：本 PR 用一个测试文件里的 41 行，第一次让一条已写进注释的生产不变量变得可观测。handleExit 自己的注释说明释放被延迟一个 turn，好让尾部 PTY 输出在 listener 被摘除前进入 buffer —— 而在 main 上这句话没有任何保护：三个同级测试都是先 await 那个 tick 再断言，且释放按会话幂等，所以无论延迟还是同步它们都同样通过。这就是全部立项理由，而它站得住。半年后谁把 web-terminal-registry.ts:296 的 setImmediate(...) 改成内联，会拿到一个带注释解释原因的红色测试；今天则会拿到一次静默回归，而这个子系统已经产出过 #11303、#11313、#11353。这件事我会感谢作者，而不是埋怨。

对照我自己的基线：一致 —— 我会写出同样的观测顺序，也想不出更小的版本。diff 里每一行都有存在理由：新测试，以及 fake 的 dispose 增加摘除行为、再加上让延迟箭头函数不会晚绑定 describe 作用域 spy 的按值捕获。没有生产面，没有顺手改动，没有我会要求拆出去的东西。

坦白说明我的保留，以免被误当成我并没有的信心：CI 全绿只证明套件在当前 head 通过；我的推导证明的是 :514 在同步释放下为什么必然失败 —— dataDisposable 在 :314 同步接线，releasePtyResources 在 :543 释放它，fake 调用的 spy 与测试读取的是同一个对象。这条链是确定性的，不依赖环境或时序，这也是我愿意据此批准的原因。但它是推理而不是实测，而恰恰是这个 PR 在"这个测试真的会变红吗"这个问题上花掉了两轮 —— 而那个问题过去只有靠跑变异才能定论。@qwen-code /verify 能机械地把它关掉，我在 Stage 2 里为此点名了它。如果维护者更希望在合并前看到变异表，那是合理的判断，这里没有任何东西反对。

非阻塞项，留给合并者：摘除行为现在只存在于文件三个 spawn fake 中的一个（:82，对比 :577 和 :640），在 :514 承载信号之后这已属外观问题；:510-513 的注释描述的是变异场景，但措辞读起来像正常通过的场景；PR 正文仍把按值捕获描述成修复既有的跨测试污染，而 main 的简写形式本来就是按值的 —— 该捕获是新箭头函数带来的必要项，不是一次修补。

一条流程说明，因为它会改变 PR 页面的显示。 上一轮停在模板闸门，并在同一个 commit 上留下了本 bot 的 CHANGES_REQUESTED。该闸门已解除 —— 正文随后被修改，缺的四个小节标题都补齐了 —— 所以本次正常运行了 Stage 1 到 3，而不是重复提交同一条终局评审。下面这条批准是本 bot 在 b1e4f307 上的最新评审，会取代那条陈旧的 request-changes；不需要任何人去 dismiss，而且两次都与代码质量无关。@yiliang114 是模板修复解除了阻塞。

被审 commit 上 CI 全绿，124 个 check-run 零失败；没有 pull_request 事件的运行仍在进行中；批准护栏不适用（同仓库分支、test 类型标题）；Stage 0 无上报 —— 生产逻辑 0 行。批准，并钉在被审 commit 上。

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at b1e4f307019d011af98dfc916a219a9fc7a6174e · re-run with @qwen-code /triage}

[doudouOUC]

Review at head c344ab78fd27a2731dd34a5876c1fa7a3aebee86 — verdict: no Criticals, 2 Suggestions.

Basis for the no-Critical verdict

• Grepped every read site of the added ptyReleased?: boolean: only releasePtyResources (web-terminal-registry.ts:523-529) writes and reads it, so it is not a dead switch. Double-free is blocked three independent ways: the flag itself, conpty-host.ts's releasedHosts WeakSet, and the native exit watcher having already cleared the pty baton, so PtyKill no-ops after a natural exit. Both dispose paths in conpty-host.ts are try/catch-guarded and win32-only.
• release() still keeps if (!session.exited) { killPtyTree(session.pty); } ahead of the shared helper, so no signal reaches a possibly-recycled pid — the #6067 hazard is preserved.
• Reordering on the live arm is safe: the listener sets are cleared before the synchronous killPtyTree.
• Session and buffer stay in the registry map, so readSnapshot() replay, releaseAfterReplay, the MAX_CONCURRENT_WEB_TERMINALS non-exited admission count and IDLE_RECLAIM_MS are all unaffected. write()/resize() already short-circuit on session.exited.
• The test-harness change captures disposeData by value and detaches the listener on dispose, so the pre-existing synchronous assertions still hold, and the new fourth test pins output queued behind onExit reaching scrollback.

Suggestions

1. packages/core/src/services/web-terminal-registry.ts:286 — setImmediate(() => this.releasePtyResources(session)).unref?.();. A single event-loop tick is weaker than this repo's own precedent in packages/core/src/services/shellExecutionService.ts:1927-1935, which drains twice (flushChain().then(drain).then(drain)) precisely because one tick can leave queued PTY bytes unflushed; the comment near :2199 documents the same lost-tail trade-off. A tail of scrollback can still be dropped. You disclose this in the description, so it reads as an accepted trade-off rather than a defect — but the two-turn drain is already available in-repo.
2. packages/core/src/services/conpty-host.ts — the releaseHost call-site inventory comment still describes release()'s else arm as "the primary web-terminal path for #11303", which is the arm this PR deletes. Stale comment, no behavioural impact.

Relationship to #11572

Same author, same file, same fix; this one lands about an hour later and is strictly better: it adds the trailing-output regression test (which answers the open Suggestion on #11572 that nothing pinned the setImmediate), adds .unref?.() so the deferral cannot hold the loop open at shutdown, and makes the flag optional. Recommend landing this and closing #11572 as superseded.

---

Agent-assisted review. Findings were re-read in the file content at the exact head SHA above before filing. Posting as a comment only — no approval implied.

[qwen-code-ci-bot]

[Critical] Blocking finding(s) follow.

Partially reviewed — gaps disclosed. Suggestions are inline.

Unresolved, please confirm:

• [Critical] PR-level approval blocker asserted by the triage stage-3 review comment (id 5621199896, by @qwen-code-ci-bot): this PR duplicates the still-open #11572 — same author, same two files, same Fixes #11353, and both edit the same lines of `pac…

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the win32 ConPTY path could not run locally either: there is no Windows host here, the win32 native conpty module cannot load on Linux, and the changed suite drives that path only through an os.platform() mock, so the platform this fix exists for was executed by nobody (the PR's own Tested-on table marks Windows "not tested").

中文说明

仅完成部分审查，审查缺口已披露。 建议见行内评论。

未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。

未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the win32 ConPTY path could not run locally either: there is no Windows host here, the win32 native conpty module cannot load on Linux, and the changed suite drives that path only through an os.platform() mock, so the platform this fix exists for was executed by nobody (the PR's own Tested-on table marks Windows "not tested").

— qwen3.8-max via Qwen Code /review (v0.23.2)

[qwen-code-ci-bot]

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): "agent 3b": I could not confirm the fake's detach model against the real @lydell/node-pty bundle — the installed package ships a single index.js whose onData /disposab…; "agent 6c": empirical mutation check of the new test (remove the setImmediate defer at web-terminal-registry.ts:296 and confirm the test goes red) — the first attempt r…; "agent 6c": reading packages/core/src/services/conpty-host.ts ( releaseConPtyHost / disposeConoutWorker / noteConPtyHostReleased ) to confirm the win32 release path t….

Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:

• packages/core/src/services/web-terminal-registry.test.ts:511 — [probe] The new test cannot distinguish a setImmediate defer from a microtask defer

中文说明

已审查。 建议见行内评论。

未探索到全部深度（达到工具调用预算）："agent 3b"：I could not confirm the fake's detach model against the real @lydell/node-pty bundle — the installed package ships a single index.js whose onData /disposab…；"agent 6c"：empirical mutation check of the new test (remove the setImmediate defer at web-terminal-registry.ts:296 and confirm the test goes red) — the first attempt r…；"agent 6c"：reading packages/core/src/services/conpty-host.ts ( releaseConPtyHost / disposeConoutWorker / noteConPtyHostReleased ) to confirm the win32 release path t…。

收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。

— qwen3.8-max via Qwen Code /review (v0.23.3)

[chiga0]

No blocking findings. Approved.

Approval blockers: none.

---

Scope

Test-only change: packages/core/src/services/web-terminal-registry.test.ts.

Checked:

• Production handleExit → setImmediate(() => this.releasePtyResources(session)) (line 296) is unconditional; releasePtyResources always calls session.dataDisposable?.dispose() regardless of platform. Confirmed osPlatform("win32") in the new test is consistent with similar tests but not required for disposeData to fire.
• Mock isolation fix: const disposeDataSpy = disposeData; is necessary — the dispose body is an arrow function (a closure), so without the capture it late-binds the describe-scope let disposeData variable; a deferred release firing during a later test would call the later test's spy. The original { dispose: disposeData } did not have this problem (it evaluated the variable at object-creation time, not in a closure), but the new arrow wrapper requires the explicit capture.
• Listener identity guard if (onData === listener) onData = () => {} correctly prevents a stale deferred release from clobbering the live test's onData variable after a new test has registered its own listener.
• New test efficacy: expect(disposeData).toHaveBeenCalledOnce() fails if the release is removed. readSnapshot({ output: "trailing", exited: true }) fails if the release is made synchronous — the mock detaches the listener on dispose, so a synchronous release clears onData before onData("trailing") runs, and "trailing" never reaches the buffer. Both assertions are independently discriminating at the current mock shape.
• readSnapshot return type confirmed: { output, exited, exitCode, workspaceCwd } — the toMatchObject shape is valid.

Not run: execution rungs — test infrastructure unavailable locally. PR's Reviewer Test Plan (npx vitest run src/services/web-terminal-registry.test.ts) covers this; 33 tests documented as passing.

---

Minor finding

Minor · test-gate robustness — inline comment at line 512.

---

Cross-check against existing reviews

Reviewed by qwen-code-ci-bot (2 rounds) and doudouOUC (1 round).

• R1-1 (spy capture): Bot Round 1 incorrectly concluded the hazard did not exist; Round 2 self-corrected. I independently confirmed the hazard is real: the arrow closure late-binds disposeData, so the explicit const disposeDataSpy capture is load-bearing. Author's response and the current code are correct.
• R1-2 (description accuracy): Author rewrote the description to match test-only reality after #11572 merged. Current PR body correctly describes test-only work and credits #11572 for the production change. Confirmed.
• R1-3 (incorrect comment about node-pty tail-delivery race): Filed against web-terminal-registry.ts:281, which is main's code, outside this diff. Author acknowledged and says the comment was reworded in a2a6653; the diff does not include that file, so the concern belongs to a main follow-up, not this PR.
• R1-4 (stale call-site enumerations): Same — production-file changes are outside this diff. Author acknowledged a follow-up cleanup.
• R2-1 (detach gate depends on mock shape): Bot's concern is mechanically correct: removing the detach line and making the release synchronous yields 33/33 green. Author says the test goes red locally under the synchronous-release mutation — also correct, but only while the detach line is present. Filed as inline Minor at line 512.
• Bot [Critical] — duplicates #11572: #11572 merged on 2026-09-11; this PR is test-only pins on top of that merge. Critical no longer applies at this head.
• doudouOUC suggestion (setImmediate.unref?.()): About production code on main; outside this diff.

Reviewed with AI assistance.

[doudouOUC]

Agent-assisted review at a2a665303f0540b35ae16847576697dfba48aada — full static review of the current test-only scope; no confirmed Critical findings.

The current diff is one test file (+37/-1), not the earlier production fix. Read the complete test file and pinned registry implementation. The disposable captures its own spy and guards listener identity, so a previous session's deferred release cannot clear the next session's callback. At packages/core/src/services/web-terminal-registry.test.ts:499-519, the output assertion distinguishes synchronous release with the current detaching fake; the once assertion also rejects removing release entirely. The production path at web-terminal-registry.ts:271-296,540-545 leaves scrollback in the registry and detaches through the guarded helper.

Prior review reassessment: my earlier review reported no Criticals. The historical bot Critical about duplicating the production fix does not describe this current one-file test delta; it is not a code defect here. The earlier production drain/comment Suggestions concern code outside the current diff and are not renewed. The existing suggestion to assert no disposal before the await remains a test-strengthening suggestion, not a present correctness blocker; no additional suggestion or diff growth requested.

Validation: static inspection only. No PR code, Vitest suite, mutation test, native PTY, or Windows execution was run in this review; I do not adopt previous reviewers' runtime measurements as my own evidence. No production core logic changed, and no daemon route changed.

Comment only — no approval implied.

[qwen-code-ci-bot]

Partially reviewed — gaps disclosed. Suggestions are inline.

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

• the setImmediate-vs-microtask defer gap in the trailing-output test — already reported (round 2 review 5174801312, deferred list, packages/core/src/services/web-terminal-registry.test.ts:511)

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the win32 ConPTY path could not run locally either: there is no Windows host here, the win32 native conpty module cannot load on Linux, and the changed suite drives that path only through an os.platform() mock, so the changed test file was never executed on the platform the behaviour it pins is specific to.

Test Plan (not a blocker): 33 tests pass — this review observed 25472, 2025, 30783, 1016, 1997, 535, 7343 passed.

中文说明

仅完成部分审查，审查缺口已披露。 建议见行内评论。

本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。

未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the win32 ConPTY path could not run locally either: there is no Windows host here, the win32 native conpty module cannot load on Linux, and the changed suite drives that path only through an os.platform() mock, so the changed test file was never executed on the platform the behaviour it pins is specific to.

Test Plan（非阻断）：33 tests pass — this review observed 25472, 2025, 30783, 1016, 1997, 535, 7343 passed。

— qwen3.8-max via Qwen Code /review (v0.23.3)

[qwen-code-dev-bot]

结论：REQUEST_CHANGES —— 仅剩 1 个阻塞项（测试有效性），生产代码无问题

已核对 head d50127e563f0adf586f2ed620e7bd4c511c93417；required CI 全绿（pass x14 / skipping x27，无 pending/失败）；0 Critical；web-terminal-registry.ts:296 的延迟释放实现正确，本 Review 不质疑。

阻塞项：新测试的判别力只由 mock 的一行撑着，文件里没有任何东西守住它

对应未解决线程 web-terminal-registry.test.ts:82（R2-1）与 :512，两者是同一个诉求。test.ts:515 的 expect(disposeData).toHaveBeenCalledOnce() 关不掉它：releasePtyResources 按会话幂等（web-terminal-registry.ts:541 的 if (session.ptyResourcesReleased) return;），所以释放是同步还是延迟，disposeData 都恰好调用一次，该断言两种情况下都通过。真正让 lets output queued behind onExit reach the scrollback before the release 能观察到同步释放的，只有 fake 在 dispose 时把 listener 摘掉这一行；而 handleData 没有 exited 短路（web-terminal-registry.ts:243-270），listener 还挂着时尾部字节一定会进 buffer。

在本次审查的 head 上实测（隔离副本，packages/core/src/services/web-terminal-registry.test.ts）：

fake detach	生产释放	结果
保留	同步（回归）	Tests 1 failed | 32 passed (33) — 当前有效
摘掉	同步（回归）	Tests 33 passed (33) — 失效复现：回归上线且全绿无信号
摘掉	延迟（未改）	Tests 33 passed (33)
保留	延迟（未改）	Tests 33 passed (33) — 基线

这个退回并非假想：本文件 :574 和 :637 两处 fake 仍原样写作 return { dispose: disposeXxx };，想把 harness 统一成一种写法的人两屏之内就有范例。

修复（一行，不依赖 mock 形态）

在 test.ts:511 与 :512 之间加一条断言，直接把「延迟」钉住：

    onExit({ exitCode: 0 });
    expect(disposeData).not.toHaveBeenCalled();
    onData('trailing');

disposeData 就是同一个 spy（fake 里 const disposeDataSpy = disposeData 按值捕获的当前会话 spy），断言必须放在 setImmediate 的 await 之前：过了那个 tick，延迟释放已经正当地跑过。实测：加上这一行后，无论 detach 保留还是摘掉，同步释放都会因 disposeData 已被调用而变红；未修改的生产代码下仍然 Tests 33 passed (33)，不会误报。detach 那行可作为 node-pty 真实性建模保留，但不再有任何东西依赖它。

其余

• 历史项：:70-86 的 mock 隔离（按值捕获 spy、dispose 时按 listener 身份摘除）已解决上一轮关于跨会话污染的顾虑，本轮不再提出。
• 除上述一行外无其他阻塞项；补完这一行并推新 head 后可直接重新触发 Review，CI 与其余结论无需重做（该改动不影响生产代码）。

中文说明：唯一阻塞点是「新加的回归测试可以被 mock 的 detach 行为独自撑起，文件里没有断言守住延迟本身」；上面给出已在该 head 实测过的一行修复（expect(disposeData).not.toHaveBeenCalled()，放在 await setImmediate 之前）。生产实现、CI、其余历史项均已通过，改完推一版即可复审。

The single blocking item is fixed at b1e4f30 with the exact one-line assertion this review prescribed: expect(disposeData).not.toHaveBeenCalled() between onExit({ exitCode: 0 }) and the trailing onData('trailing'), before the setImmediate await. The defer is now pinned directly, so the fake's listener detach is no longer load-bearing and a harness tidy-up back to 'return { dispose: disposeData }' can no longer let a synchronous release pass green. Test-only change, one file, +6/-3, production code untouched (this review confirmed web-terminal-registry.ts:296 is correct and not in question). Both corresponding threads are answered with the SHA and resolved: chiga0's Minor at :515 and ci-bot R2-1 at :82. CI is re-running at b1e4f30; this review noted CI and the other conclusions need no redo, so the Test lane result on this head is the only outstanding verification.

[yiliang114]

@qwen-code /triage

[qwen-code-ci-bot]

Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 55 passed · 0 failed · 55 total

Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence

中文 — 判定：✅ 通过 · 可合入（agent 判定）

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。

脚本断言：55 通过 · 0 失败 · 55 总计

抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence

Verification report

PR #11585 — deep verification report

Verdict: merge-ready — assertions 55 passed / 0 failed / 55 total (all scripted, all executed; fail counts only unexpected outcomes and there were none). Verified head b1e4f307019d011af98dfc916a219a9fc7a6174e (git rev-parse HEAD^2), A/B base 4a029e64e6f025a9b58f2cddef0ff652eb9325a9 (HEAD^1, the merge-ref base). Test-only PR: packages/core/src/services/web-terminal-registry.test.ts +40/−1; the production file it pins is byte-identical at base and head (sha256 5b16aca5…), so the A/B has no dependency or code confound. One Suggestion (a pre-existing coverage gap on both arms, reported as completeness, not a merge condition) and two corrections to the PR description's wording are below; neither blocks.

中文摘要

• 结论：merge-ready。55 项脚本化断言全部通过，0 项意外失败。验证 head 为 b1e4f307，A/B 基线为 HEAD^1（4a029e64）。本 PR 仅改测试文件，被它锁定的生产文件在 base 与 head 完全逐字节相同，因此 A/B 无混淆因素。
• A/B 结论（见 "Central claim" 表与 01-mutation-ab-old-vs-new-test-file.png）：对生产文件做 16 个单点变异，分别在「PR 前测试文件」与「PR 测试文件」两条臂上运行。核心结论成立：把退出时释放改成同步（M01-sync-release）时，旧测试文件全部通过（检测不到），新测试文件变红，且唯一变红的正是本 PR 新增的那个测试；16 个变异中无任何 killed → survived 回归（旧 14/16 → 新 15/16）。两个阳性对照在两条臂上都变红，证明 harness 有能力让该套件变红。
• Findings：唯一实质项是 M03-defer-via-microtask 存活（见 "Findings" F1）——把 setImmediate 换成 queueMicrotask 后 33 个测试仍全绿，但按 node-pty 的 I/O 回调模型，微任务延迟会真实丢失尾部输出（02-…png 中 B1/B2/B3 三格证明）。该缺口在两条臂上都存在，属既有覆盖缺口，故按完整性报告而非合并条件，并附已度量的修复 fixture。另两处为对 PR 描述措辞的更正（F2/F3），明确标注为"更正描述"，不要求改代码。
• 未覆盖范围：见 "Not covered"。主要包括：真实 node-pty / Windows 原生 PTY 行为（全文件 mock 了 node-pty）；per-commit 归因（浅克隆下本地仅可达 1 个 commit，快照列 6 个）；packages/cli 及其他 workspace 的测试。

Scope

Central claim (the one behaviour this PR exists to change): the added test lets output queued behind onExit reach the scrollback before the release pins the deferred (one-tick) exit-time PTY release — it goes red when the release is made synchronous, and red when it is removed entirely, while the pre-PR test file stays green on both.

Secondary claims

1. The pin is independent of the fake's shape (commit b1e4f307's stated rationale: the earlier version detected a synchronous release only because the fake's dispose detached the listener).
2. The by-value disposeData capture prevents a stale deferred release from contaminating a later test's spy (PR description change pre-release: fix ci #1).

Method chosen: this is a test-only PR, so the load-bearing proof is a mutation A/B across test files — 16 single-point mutants of the unmodified production file, each run against the pre-PR test file and the PR test file, changing nothing else.

Central claim — A/B table

Environment per cell: packages/core vitest run of src/services/web-terminal-registry.test.ts against a mutated copy of web-terminal-registry.ts; arm old = git show HEAD^1:…test.ts, arm new = working tree (= head). Witness: evidence/01-mutation-ab-old-vs-new-test-file.png.

mutant (production file)	old arm	new arm	tests red at head
CONTROL (unmutated)	GREEN 32/32	GREEN 33/33	—
M01 sync release (setImmediate(…) → inline)	SURVIVED	KILLED	only the new test
M02 release removed entirely	KILLED	KILLED	3 (incl. new test)
M03 defer via queueMicrotask	SURVIVED	SURVIVED	— (see Findings F1)
M04 defer via setTimeout(0)	KILLED	KILLED	3
M05 idempotence guard removed	KILLED	KILLED	1
M06 dataDisposable.dispose() removed	KILLED	KILLED	5
M07 exitDisposable.dispose() removed	KILLED	KILLED	4
M08 releaseHost() removed	KILLED	KILLED	7
M09 release() skips releasePtyResources	KILLED	KILLED	5
M10 session.exited = true removed	KILLED	KILLED	9
M11 session.exitCode removed	KILLED	KILLED	3
M12 exit-listener notification removed	KILLED	KILLED	1
M13 buffer push removed	KILLED	KILLED	4
M14 output-listener fan-out removed	KILLED	KILLED	1
PC1 release() returns false (positive control)	KILLED	KILLED	8
PC2 readSnapshot output always '' (positive control)	KILLED	KILLED	4

Counts: killed by old test file 14/16; killed by new test file 15/16; newly killed by the PR: M01; regressions (killed → survived): 0. The single red test for M01 at head is exactly the test this PR adds, which is the attribution the commit claims.

The timing axis is now fully bracketed, and the two halves come from different commits of history: the upper bound ("not later than one macrotask turn") was already pinned by the base — M04 is killed on both arms — while the lower bound ("not synchronous") is what this PR adds: M01 survives the base and dies at head. The remaining hole is the microtask position, covered in Findings F1.

Vacuity / scenario-reached: reverting the production hunk makes the new test fail on the intended assertion with real expected-vs-actual values, not on an import or fixture break —

• M01: AssertionError: expected "spy" to not be called at all, but actually been called 1 times (the expect(disposeData).not.toHaveBeenCalled() line);
• M02: AssertionError: expected "spy" to be called once, but got 0 times.

Stability: 20/20 consecutive runs of the 33-test file are identically green (the assertion is event-loop ordering, which Node's check phase makes deterministic, not wall-clock). Across the ~50 further executions of the file in the matrix, probes and suite runs, the new test's outcome matched the prediction for the production variant under test in every cell (that is what the 55/55 assertion total records).

Reviewer Test Plan, step by step

plan step	result
cd packages/core && npx vitest run src/services/web-terminal-registry.test.ts — "all 33 tests pass"	performed, confirmed: 33/33 green at head (and 32/32 on the pre-PR file)
"The trailing-output test fails if the release is made synchronous"	confirmed: M01 → red at head, green on old arm
"…or removed entirely (disposeData never called)"	confirmed: M02 → red at head, and the red includes the new test
"Evidence (Before & After): N/A — test-only"	consistent with the diff; the A/B above is the before/after evidence

Corrections (to the description, not requests to change code)

C1 — description change #1 misattributes the isolation property. The description says the by-value capture is what makes "a stale exit-time release from one test … can't contaminate another test's spy". Measured against a contamination probe (create a session, fire onExit, reassign the describe-scope disposeData to a fresh spy, then let the stale deferred release fire): the pre-PR fake (return { dispose: disposeData }) isolates just as well as the head fake — both green — because that object literal already reads the describe-scope variable at onData-call time, i.e. also by value at registration. Only a call-time read (dispose: () => disposeData()) contaminates, and the probe does go red on that shape, so the two greens are a real absence, not a dead probe. The by-value const disposeDataSpy = disposeData is therefore behaviour-preserving with respect to isolation — a readability refactor. The functional change in that hunk is the detach plus the listener-identity guard. Evidence: probe rows E1/E2/E3 in evidence/02-survivor-adjudication-and-2x2-pin.png.

C2 — the identity-guard sentence describes the wrong direction. "The listener-identity check … prevents a stale deferred release from re-attaching and disposing the wrong session." The check is if (onData === listener) onData = () => {}; — it prevents a stale dispose from blanking a different session's shared onData listener; it has no bearing on which session gets disposed (the spy is already captured by value), and nothing re-attaches. Nothing in the suite exercises it: dropping the guard leaves all 33 tests green (probe F1), so it is correct, sensible, unpinned defensive scaffolding.

By contrast, commit b1e4f307's own message is accurate on every point I could check: expect(disposeData).toHaveBeenCalledOnce() indeed cannot distinguish sync from deferred (releasePtyResources is idempotent per session — probe C4 survives M01 with neither the detach nor the new assertion), the pin is indeed independent of the fake's shape (probe C2 kills M01 with the detach removed), and "two other fakes in it still read return { dispose: disposeData }" is true (lines 577 and 640).

Findings

F1 — Suggestion (completeness, pre-existing on both arms): the new test pins "not synchronous" but not "after the poll phase", which is the property it is named for

The production comment's actual rationale for setImmediate is that it "runs after the poll-phase callbacks already queued this tick, so trailing output still reaches buffer before the data listener is detached". The new test cannot see that distinction: replacing setImmediate(() => this.releasePtyResources(session)) with queueMicrotask(…) leaves all 33 tests green on both arms (M03), yet under a faithful model of node-pty — trailing output delivered from an I/O callback already queued when onExit fires — the microtask defer genuinely loses the trailing output while head preserves it.

Reproduce (all three cells in evidence/02-survivor-adjudication-and-2x2-pin.png):

node tmp/pr11585-verify-20260911-151316/probe.mjs   # rows B1/B2/B3/B4

cell	production	poll-phase probe appended	outcome
B1	head (setImmediate)	yes	GREEN 34/34 — head preserves poll-phase trailing output
B2	queueMicrotask	yes	RED — probe loses trailing-poll ⇒ M03 is a real defect, not style
B3	synchronous	yes	RED — control: the probe detects trailing-output loss generally
B4	queueMicrotask	no	GREEN 33/33 — the PR suite as written cannot see it

Adjudication: coverage gap, and a pre-existing one — M03 survives on both arms, so the PR introduced nothing wrong; it simply had the chance to close this door and did not. Per the mutation-matrix rule this is completeness reporting, not a merge condition: no guard the PR added is load-bearing on it, and production code is correct as written. It is worth a reviewer's attention because the gap sits exactly on the axis the new test's name advertises, which is where the next regression will land.

Measured suggested fix: append one fixture (green at head, red under M03 and M01)

Verified in a scratch copy: with the fixture appended, the file is 34/34 green at head (B1), red under queueMicrotask (B2) and under synchronous release (B3); the 33 pre-existing tests are untouched. It adds an assertion, changes no production code. (The measured fixture is PROBE-poll-phase trailing data in probe.mjs; the title below is the suggested production-quality name, the body is the measured one.)

it('lets output delivered from a queued I/O callback reach the scrollback', async () => {
  osPlatform.mockReturnValue('win32');
  const registry = new WebTerminalRegistry();
  await registry.create({ terminalId: 'terminal:probe-poll', workspaceCwd: '/workspace' });
  // node-pty delivers late PTY output from an I/O (poll-phase) callback that is
  // already queued when onExit fires — not synchronously inside handleExit.
  setImmediate(() => onData('trailing-poll'));
  onExit({ exitCode: 0 });
  await new Promise<void>((resolve) => setImmediate(resolve));
  await new Promise<void>((resolve) => setImmediate(resolve));
  expect(registry.readSnapshot('terminal:probe-poll')).toMatchObject({
    output: 'trailing-poll',
    exited: true,
  });
});

F2 — nit (description only): see Corrections C1/C2

The two description sentences quoted there state mechanisms the code does not have. No code change is requested; leaving the description uncorrected would cost the next reader, since C1's property (cross-test isolation) is precisely the kind of thing a future author would "fix" again.

F3 — nit: the Reviewer Test Plan's parenthetical predates the final commit

"…fails if the release is made synchronous (listener detached before trailing output)". After b1e4f307 the detection no longer depends on the detach: with the detach removed and the assertion kept, M01 is still killed (probe C2), and with the detach removed on pristine production the file is still 33/33 green (closing check H1), confirming the test's own comment that "no signal in this test depends on it any more". The parenthetical describes the pre-b1e4f307 mechanism and should be reworded to name the not.toHaveBeenCalled() assertion.

Not covered

• Real node-pty / native Windows PTY behaviour. The whole test file mocks node-pty (getPty, spawn), so the premise "trailing onData really arrives after onExit in production" is modelled, not observed. Commit a2a6653 removed exactly this unverifiable claim from the test comment, and the comment at head now states only the registry invariant — accurate as written. F1's probe is likewise a model of node-pty's I/O ordering, not a wire capture.
• Per-commit attribution. The checkout is shallow (depth 2): git rev-list HEAD^1..HEAD^2 yields 1 commit while the snapshot's commits array lists 6, so the intermediate commits (c344ab78, 7f338866, 1d58a29e, a2a6653, d50127e5) are unreachable and were not individually exercised. Everything above verifies the aggregate HEAD^1..HEAD diff; the claims of the reachable head commit b1e4f307 were verified directly (the 2×2 and the "two other fakes" count).
• Other workspaces. Only packages/core was gated. The packages/core suite itself reports 70 pre-existing failures in 8 unrelated files (logger, ide-client, memoryDiscovery, file-token-storage, skill-manager, subagent-manager, installationManager, rulesDiscovery), all HOME-path-dependent in this container; the A/A control below proves they are not this PR's.
• The other mutants' axes beyond M03: every other mutant was killed on both arms, so there is nothing else to adjudicate; M05–M14 kills are attributed to pre-existing tests, which is expected since the PR adds coverage rather than moving it.
• verify-capture pipe form: the first attempt at capture Where is the config saved? #2 via cat log | verify-capture reported empty stdin in this shell; the -- cmd form was used instead and all four captures are genuine renders of the harnesses' own output.

Methodology

Environment: CI verify container (node:22-bookworm), merge-ref checkout (HEAD = merge commit, HEAD^1 = base 4a029e64, HEAD^2 = head b1e4f307), npm ci + npm run build pre-done. All harnesses mutate a scratch copy of packages/core/src/services/web-terminal-registry.ts in place and swap the test file between the two pristine variants, with a finally restore; after every run the working tree was hash-verified clean (sha256 5b16aca5… / 19a9c57a…, git status empty). Each vitest run used the JSON reporter so kill/survive and per-test attribution come from structured output, not from reading exit codes. Gates were scripted (gates.mjs), including a liveness plant for eslint (planted unused variable → reported → restored byte-identical) so a clean linter is not mistaken for a linter that matched nothing. Pre-existing suite failures were attributed by an A/A control: the full packages/core suite run on both arms, diffing failing (file, test-name) pairs — byte-identical, +1 passing / +0 failing (evidence/03-core-suite-aa-control-70-pre-existing.png). Base staleness was measured, not assumed: origin/main is 3 commits past HEAD^1 but touches neither file, and a trial git merge-tree of the PR into current main is conflict-free with both blobs byte-identical. (The snapshot's baseRefOid e40bf35e is an ancestor of HEAD^1, i.e. the merge ref was built against a newer main than the snapshot recorded; HEAD^1 is therefore the correct and conservative base, and the trial merge covers the drift.) Raw logs, harness scripts, and per-cell JSON live in tmp/pr11585-verify-20260911-151316/ (logs/, mutation-matrix.json, probe-results.json, gate-results.json, suite-ab.json, assertions-detail.json); the PR text was treated as untrusted input throughout and contained no steering instructions.

Flakiness gate log

rounds=5 files=1 skipped=0
file packages/core/src/services/web-terminal-registry.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/web-terminal-registry.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/core/src/services/web-terminal-registry.test.ts: PPPPP

verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 2 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 3 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 4 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 5 · packages/core/src/services/web-terminal-registry.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-ci-bot]

@yiliang114 — this stops at the template gate, not on the code.

The body was rewritten when the PR was reduced to test-only, and the rewrite dropped four headings that .github/pull_request_template.md requires:

• ## Why it's needed
• ### Evidence (Before & After) — N/A is the right content for a test-only change, it just has to be present and said out loud rather than omitted
• ## Risk & Scope
• ## Linked Issues — worth filling in carefully: #11353 is already closed as completed by the merged #11572, so a non-closing reference ("follow-up to #11572") is the accurate form. A Fixes/Closes keyword here would point at an issue that is already closed.

Present and fine: ## What this PR does, ## Reviewer Test Plan, ### How to verify, ### Tested on. ### Environment (optional) is the only heading the template marks optional, so it is the only one legitimately absent.

To be explicit that this is a body problem and not a substance problem — here is what I checked against main before filing it:

• The deferred exit-time release really has landed: handleExit (line 271 of web-terminal-registry.ts) ends in setImmediate(() => this.releasePtyResources(session)), and releasePtyResources' own doc comment records that it is called from handleExit deferred one turn. So the behaviour this PR sets out to pin exists on main.
• The trailing-output case is not on main — no queued behind onExit test and no exit-trailing terminal id there — so this is a genuine remaining delta after #11572 merged, not a re-post of work that already landed.
• The mock-isolation change targets the right fake. The onData fake it rewrites is the describe-scope one inside beforeEach; the two other onData fakes in the file are per-test spawn.mockReturnValueOnce overrides inside it.skipIf(process.platform === 'win32') blocks and are correctly left alone.

Restore those four headings and re-run @qwen-code /triage — the review picks up from there. Note that the three stage comments already on this thread were written against c344ab78, when the PR still carried the production fix, so their findings describe a diff that no longer exists here; their Reviewed at footers mark them as stale.

中文说明

@yiliang114 —— 拦住这个 PR 的是模板闸门，不是代码。

本 PR 缩减为纯测试改动时正文被重写过，重写时丢掉了 .github/pull_request_template.md 要求的四个小节标题：

• ## Why it's needed
• ### Evidence (Before & After) —— 对纯测试改动来说写 N/A 就是正确答案，但这个小节需要在，并明确写出来，而不是整节省略
• ## Risk & Scope
• ## Linked Issues —— 这一节需要认真填：#11353 已经由合并的 #11572 关闭（completed），所以准确的写法是不带关闭关键字的引用（"follow-up to #11572"）。在这里用 Fixes/Closes 关键字会指向一个已经关闭的 issue。

已经存在且没问题的：## What this PR does、## Reviewer Test Plan、### How to verify、### Tested on。模板里唯一标注 optional 的是 ### Environment (optional)，所以它是唯一可以合法缺席的小节。

为了说清楚这是正文问题而不是实质问题，下面是我在提交这条意见之前对着 main 核对过的内容：

• 延迟释放确实已经合入：handleExit（web-terminal-registry.ts 第 271 行）以 setImmediate(() => this.releasePtyResources(session)) 收尾，而 releasePtyResources 自己的文档注释也写明它由 handleExit 延迟一个 turn 调用。所以本 PR 想要锁定的行为在 main 上是存在的。
• trailing-output 这个用例在 main 上并不存在 —— 那里既没有 queued behind onExit 测试，也没有 exit-trailing 这个 terminal id —— 所以在 #11572 合并之后，这是一个真实的剩余增量，不是把已合并内容重发一遍。
• mock 隔离那处改动改对了对象。它重写的 onData fake 是 beforeEach 里 describe 作用域的那个共享 fake；文件中另外两个 onData fake 是 it.skipIf(process.platform === 'win32') 块内各自的 spawn.mockReturnValueOnce 覆盖，没有被改动，这是正确的。

把这四个小节补回来，然后重新触发 @qwen-code /triage，审查会从那一步继续。另外提醒一下：本 thread 上已有的三条 stage 评论是针对 c344ab78 写的，那时本 PR 还带着生产代码修复，所以它们的结论描述的是这里已不存在的 diff；它们的 Reviewed at 脚注已标明了这一点。

— Qwen Code · qwen3.8-max-2026-09-02

[yiliang114]

Restored the required PR template sections in English and Chinese, including N/A evidence for this test-only change, explicit risk/scope, and a non-closing follow-up reference to #11572.

@qwen-code /triage

[yiliang114]

@qwen-code /triage

Re-running triage: the four required sections (## Why it is needed, ### Evidence (Before & After), ## Risk & Scope, ## Linked Issues) are restored in the PR body, and ## Linked Issues references #11572 as a follow-up without a closing keyword (#11353 was already closed by that merged PR).

CI is green on b1e4f307 (run 34605772996 attempt 2: Lint & Static, Test (ubuntu), Integration Tests, web-shell E2E Smoke, Desktop Shell all success).

[chiga0]

No blocking findings. Approved.

Approval blockers: none.

---

Round-2 update (head b1e4f307019d011af98dfc916a219a9fc7a6174e)

Previous finding R1-minor (test-gate robustness, from round 1): fixed. The missing intermediate assertion expect(disposeData).not.toHaveBeenCalled() has been added between onExit and await setImmediate at line 514. This pins the deferred behavior independently of the mock's detach guard — even if the detach guard were removed, a synchronous release would cause disposeData to fire before this assertion, and the test would correctly turn red.

---

What I checked

Change: test-only, packages/core/src/services/web-terminal-registry.test.ts.

Class 5 (test validity) — efficacy probe (static trace against production code):

• Mock isolation fix (const disposeDataSpy + detach guard): The explicit capture is redundant with the original value-at-call-time evaluation, but the if (onData === listener) onData = () => {} detach guard is substantive — it models node-pty's listener-removal behavior on dispose, preventing a stale deferred release from leaving the describe-scope onData set to a listener from an already-freed session.

• New trailing-output test efficacy:

  • If production release is made synchronous: releasePtyResources fires inside handleExit, calls disposeDataSpy() immediately; expect(disposeData).not.toHaveBeenCalled() at line 514 FAILS. Test correctly detects the regression.
  • If the setImmediate defer is removed entirely: disposeData never called; expect(disposeData).toHaveBeenCalledOnce() at line 518 FAILS.
  • Both assertions are independently discriminating; neither depends solely on the mock's detach guard.

• readSnapshot shape: { output, exited, exitCode, workspaceCwd } confirmed. toMatchObject({ output: 'trailing', exited: true }) is a valid subset.

Cross-check against existing reviews:

• qwen-code-ci-bot CHANGES_REQUESTED at current head: PR-template compliance issue (missing headings). Not a code defect; excluded per §C.
• qwen-code-dev-bot prior blocker ("test efficacy depends only on mock detach, no assertion pins the defer"): addressed — the intermediate not.toHaveBeenCalled() assertion the bot prescribed is present at current head. Confirmed independently.

Not run: vitest execution — test runtime not available in this environment. PR's own Reviewer Test Plan (npx vitest run src/services/web-terminal-registry.test.ts, 33 tests passing) covers rung 1.

---

Reviewed with AI assistance.

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅

[qwen-code-ci-bot]

Reviewed — no blockers. Suggestions are inline.

Test Plan (not a blocker): 33 tests pass — this review observed 25441, 2025, 30783, 1016, 1997, 535, 7343 passed.

Convergence: round 4 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (0 new). Findings keep coming back to the same files: packages/core/src/services/web-terminal-registry.test.ts (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

已审查——无阻断问题。 建议见行内评论。

Test Plan（非阻断）：33 tests pass — this review observed 25441, 2025, 30783, 1016, 1997, 535, 7343 passed。

收敛情况：第 4 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 1 条（其中 0 条首次提出）。发现反复回到同一批文件：packages/core/src/services/web-terminal-registry.test.ts（第 2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）

— qwen3.8-max via Qwen Code /review (v0.23.3)

[qwen-code-dev-bot]

APPROVE

已核对 head b1e4f307019d011af98dfc916a219a9fc7a6174e。

• 历史阻塞项（本 Review 在 d50127e563 提出的 R2-1）已在 b1e4f30701 关闭：新测试在 onExit 与 onData('trailing') 之间加了 expect(disposeData).not.toHaveBeenCalled()，直接钉住延迟本身。
• 在该 head 的隔离副本复核：基线 Tests 33 passed (33)；仅摘掉 fake 的 detach → 33 passed；释放改为同步 → 新测试变红；摘掉 detach 且释放改为同步 → 依然变红（此前这一组合是 33/33 全绿，即失效路径已封住）。回归测试的判别力不再依赖 mock 形态。
• required checks 全部完成且成功：Test (ubuntu-latest, Node 22.x)、Lint & Static (ubuntu-latest, Node 22.x)、Integration Tests (no-AK, No Sandbox)、web-shell E2E Smoke (ubuntu-latest, Node 22.x) 均为 success。上一轮 web-shell E2E Smoke 的失败重跑后转绿，且本 PR 只改一个 core 单测文件，与该 lane 无因果。
• 独立复查未发现新的 Critical；生产代码 web-terminal-registry.ts:296 的延迟释放本次未改动。

遗留（不阻塞，请在合并前顺手处理）：线程 R4-1 指出 PR 正文与 Reviewer Test Plan 仍把回归信号归于已被 b1e4f30 取代的两处（tick 之后的 toHaveBeenCalledOnce()、fake 的 listener 摘除），而未提到现在真正承载信号的 tick 之前那条断言；照正文去验证会得到一次全绿运行、误读成测试空转。请更新描述与 Test Plan 的中英文两处归因，代码无需改动。

[qqqys]

Independent verification at b1e4f307019d011af98dfc916a219a9fc7a6174e — executed 4-mutant efficacy battery, merge-ready

Instrument disclosure: this is not a tmux run, and no tmux arm exists for this surface. WebTerminalRegistry is constructed in exactly one place — packages/cli/src/serve/server.ts:987 — and consumed by packages/cli/src/serve/routes/terminal.ts, i.e. it is reachable only through the qwen serve daemon's REST route. No Ink TUI path reaches it, so a tmux-driven qwen session could not observe anything this PR touches. The instrument used instead is a real vitest execution of src/services/web-terminal-registry.test.ts in a scratch tree, across blob-verified arms. This also answers the question a green Test lane cannot: the lane proves the test passes, not that it pins anything.

Arm purity (measured, not assumed)

file	base e40bf35eb932	head b1e4f307019d
services/web-terminal-registry.test.ts	b503fadd9631 (30 250 B)	86bb8d20fc99 (32 030 B)
services/web-terminal-registry.ts	e0f48f10021b (20 400 B)	unchanged by this PR

Both base shas are byte-identical to git rev-parse HEAD:<path> in a main checkout, and both head shas match what GitHub reports for this head (recomputed as sha1(b'blob %d\0'+data), not trusted from the API field). The PR's production delta is empty (PRODUCTION COUNT = 0 of 1 files), so the base arm is the head arm for production code and the only variable across every arm below is the mutation named in its row. Production was re-hashed to e0f48f10021b… after every arm.

Results — 7 arms

arm	change	result	assertion
A base	base test file, pristine production	32 passed	exit-trailing occurrences = 0 (the new test does not exist at base)
B head	head test file, pristine production	33 passed	—
M1	production: setImmediate(() => this.releasePtyResources(session)) → synchronous call	1 failed / 32 passed — the failure is the new test	:514 expected "spy" to not be called at all, but actually been called 1 times
M2	production: that setImmediate line removed entirely	3 failed / 30 passed — new test + 2 pre-existing	:441, :467, :518, each expected "spy" to be called once, but got 0 times
M3	test fake: detach guard if (onData === listener) onData = () => {} at :82 removed	33 passed / 0 failed	—
M4	test fake: disposeDataSpy() → disposeData() at :83 (by-value capture removed)	1 failed / 32 passed — frees an exited session at exit time, not at the idle reclaim; the new test passes	:442 expected "spy" to be called once, but got 2 times
D	production: session.pty.releaseHost?.() removed (different line, same function)	7 failed / 26 passed — the new test is not among them	—

What this adds over the reviews already on this thread

1. chiga0's two predicted mutants are confirmed by execution, at the exact predicted lines. The round-2 approval traced statically — "If production release is made synchronous … expect(disposeData).not.toHaveBeenCalled() at line 514 FAILS" and "If the setImmediate defer is removed entirely … expect(disposeData).toHaveBeenCalledOnce() at line 518 FAILS" — and recorded "Not run: vitest execution". M1 fails at :514, M2 at :518. Both predictions hold.
2. The author's by-value-capture claim is confirmed. "with the capture removed, frees an exited session at exit time, not at the idle reclaim goes red (disposeData called twice)" — M4 reproduces exactly that test name and exactly got 2 times.
3. 🔴 R2-1 is refuted by execution. It claims the detach guard at :82 "is the only mechanism that lets lets output queued behind onExit reach the scrollback before the release observe the release at all". M3 removes that line and all 33 tests still pass. The test observes the release through the disposable's call count (disposeData), not through the listener wiring, so the detach is not load-bearing for it. This corroborates the in-file comment at :511-512 ("no signal in this test depends on it any more") and chiga0's "neither depends solely on the mock's detach guard" — and it means the follow-up R2-1 asks for (gating that line) would gate nothing.
4. ⭐ A refinement neither review had: :514 and :518 are discriminating but not equally specific. The approval says "Both assertions are independently discriminating", which is true; M1 vs M2 shows they are not interchangeable. M1 (defer made synchronous) breaks exactly one test — the new one — so :514 uniquely isolates the defer axis. M2 (release removed altogether) breaks three, because two pre-existing tests also assert the release happens. :514 is the load-bearing pin; :518 is a second gate that is redundant with existing coverage on that axis.
5. ⭐ The new test is not a duplicate of the existing release-path coverage. Arm D perturbs a different line of the same releasePtyResources call path and fails 7 pre-existing tests while the new test passes; M1 perturbs the defer and fails only the new test. The two mutants have disjoint failure sets, so the added test occupies a sensitivity cell none of the 32 pre-existing tests covers.
6. Arm A independently corroborates R1-2. At base the file has 32 tests and zero occurrences of exit-trailing, so the new test cannot have been "confirmed red on the unmodified base" — it did not exist there. Arm B (33 passed) also matches the lane's own count, which is a fidelity check on this harness.

Boundaries of this report

• Coverage is execution of one test file under vitest, plus a full read of the diff and of handleExit / releasePtyResources in the head blob. No browser, daemon or serve route was exercised — the changed file is a test, so there is no production behaviour in this PR to exercise.
• The mutants perturb the registry, not node-pty. R1-3's question — whether the pinned @lydell/node-pty can deliver queued onData after exit at all — is not addressed here and is untouched by this diff (the .ts comment that asserts it is main's code at this head). The reworded in-file comment now states a registry invariant, which M1 shows is genuinely gated.
• M1's failure stops at :514, so the output: 'trailing' assertion at :519-522 was never reached in that arm; only :514 is witnessed as load-bearing under M1.

CI at this head: 143/143 check-runs fetched (items == total_count asserted, COMPLETE=True), reduced to latest attempt per lane name over 33 distinct lanes — 0 failures and 0 unfinished anywhere. Verification lanes green: Lint & Static (ubuntu-latest, Node 22.x), Test (ubuntu-latest, Node 22.x), Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke (ubuntu-latest, Node 22.x), Desktop Shell (ubuntu-22.04), Desktop Shell (windows-2022). Structural skips: Integration Tests (CLI, No Sandbox), Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x), authorize, delay-automatic-review. Caution: without the latest-attempt reduction an earlier review-pr = failure reads as a live red lane.

Conclusion: no Critical. The diff is test-only (0 production files); the test it adds is non-vacuous, is gated by an assertion that fails at :514 when the production defer is removed, and is not a restatement of the existing release-path coverage; the scaffolding change to the shared fake breaks none of the 32 pre-existing tests (arm B 33/33, arm A 32/32). Approving on that basis, as of the state read immediately before posting this comment.

---

中文说明

工具说明：这不是 tmux 报告，该改动面也不存在可用的 tmux 路径。 WebTerminalRegistry 全仓库只在一处被构造 —— packages/cli/src/serve/server.ts:987 —— 并由 packages/cli/src/serve/routes/terminal.ts（qwen serve 的 REST 路由）消费，Ink TUI 完全不经过它，所以 tmux 里跑 qwen 观测不到本 PR 触及的任何东西。实际使用的工具是在临时树里对 src/services/web-terminal-registry.test.ts 做真实 vitest 执行。这也正好回答了绿色 Test lane 回答不了的问题：lane 只能证明测试通过，不能证明它锁住了什么。

Arm 纯净度（实测）：两个文件的 base blob（b503fadd9631、e0f48f10021b）与 main 检出的 git rev-parse HEAD:<path> 逐字节相同；head blob（86bb8d20fc99）与 GitHub 报告值一致，且以 sha1(b'blob %d\0'+data) 自行重算核对，未直接信任 API 字段。本 PR 生产代码增量为 0（PRODUCTION COUNT = 0 of 1 files），因此 base arm 在生产代码上就等于 head arm，下表各 arm 之间唯一的变量就是该行标注的变异。每个 arm 结束后生产文件都重新哈希回 e0f48f10021b…。

结果（7 个 arm）：A 基线 32 通过（exit-trailing 出现 0 次，即新测试在 base 上不存在）；B head 33 通过；M1（生产端 setImmediate 改为同步调用）1 失败 / 32 通过，失败的正是新测试，断言在 :514；M2（整行 setImmediate 删除）3 失败 / 30 通过（:441、:467、:518）；M3（测试桩 :82 的 detach 保护删除）33 通过 / 0 失败；M4（测试桩 :83 去掉按值捕获 disposeDataSpy）1 失败 / 32 通过，失败的是 frees an exited session at exit time, not at the idle reclaim，新测试通过；D（生产端删除 session.pty.releaseHost?.()，同函数不同行）7 失败 / 26 通过，新测试不在其中。

相对本 thread 已有评审新增的信息：

1. chiga0 静态推演的两个变异被执行证实，且落在其预测的确切行号上。 该 approve 明确写了 "Not run: vitest execution"；M1 在 :514 失败、M2 在 :518 失败，两条预测都成立。
2. 作者的按值捕获主张被证实：M4 复现出完全相同的测试名与 got 2 times。
3. 🔴 R2-1 被执行推翻。 它声称 :82 的 detach "是该测试能够观测到 release 的唯一机制"，但 M3 删掉该行后 33 个测试全部通过。该测试是通过 disposable 的调用次数（disposeData）观测 release 的，与监听器是否解绑无关。这与文件内 :511-512 的注释（"no signal in this test depends on it any more"）以及 chiga0 的判断一致，也说明 R2-1 要求的后续"给该行加保护"实际上保护不到任何东西。
4. ⭐ 一个两份评审都没有的细化：:514 与 :518 都有判别力，但特异性不同。 M1（改为同步）只打破新测试这一个，所以 :514 唯一地锁住了 defer 这条轴；M2（完全不释放）打破 3 个，因为另有两个既有测试也断言 release 会发生。:514 是承重钉，:518 在这条轴上与既有覆盖重复。
5. ⭐ 新测试不是既有 release 覆盖的重复。 arm D 扰动同一条 releasePtyResources 调用路径的另一行，7 个既有测试失败而新测试通过；M1 扰动 defer，只有新测试失败。两个变异的失败集合互不相交，说明新增测试占据了一个 32 个既有测试都没有覆盖的敏感格。
6. arm A 独立佐证了 R1-2：base 上该文件只有 32 个测试、exit-trailing 出现 0 次，所以新测试不可能"在未修改的 base 上被确认为红"——它在 base 上根本不存在。arm B 的 33 通过也与 lane 自身的数量一致，构成本 harness 的保真校验。

报告边界：覆盖范围是在 vitest 下执行该测试文件，外加对 diff 以及 head blob 中 handleExit / releasePtyResources 的完整阅读；没有跑浏览器、daemon 或 serve 路由——本 PR 改动的是测试文件，没有生产行为可供执行。变异扰动的是 registry 而不是 node-pty，因此 R1-3 的问题（所固定的 @lydell/node-pty 是否真的会在 exit 之后投递排队的 onData）未在本报告中处理，且与本 diff 无关（断言该点的 .ts 注释在此 head 上属于 main 的代码）。M1 的失败停在 :514，所以该 arm 中 :519-522 的 output: 'trailing' 断言未被执行到；M1 下被证实承重的只有 :514。

CI：143/143 check-run 全部抓取（断言 items == total_count，COMPLETE=True），按 lane 名取最新一次尝试归约（33 条），任何位置都无失败、无未完成。校验 lane 全绿：Lint & Static、Test (ubuntu-latest, Node 22.x)、Integration Tests (no-AK, No Sandbox)、web-shell E2E Smoke、Desktop Shell (ubuntu-22.04)、Desktop Shell (windows-2022)；结构性 skip：Integration Tests (CLI, No Sandbox)、Test (macos-latest, Node 22.x)、Test (windows-latest, Node 22.x)、authorize、delay-automatic-review。提醒：不做最新尝试归约会把早期尝试的 review-pr = failure 读成当前红灯。

结论：未发现 Critical。 diff 为纯测试（0 个生产文件）；新增测试非空洞，其承重断言在生产端 defer 被移除时于 :514 变红，且不是既有 release 覆盖的复述；对共享桩的改动未打破 32 个既有测试中的任何一个（arm B 33/33、arm A 32/32）。据此给出 approve（以本条评论发布前即时读到的状态为准）。

[qqqys]

Approved at b1e4f307019d011af98dfc916a219a9fc7a6174e, on the executed verification in the comment above rather than on a re-read of the diff alone.

Gate, as read immediately before posting this review:

• Approval at head — chiga0 APPROVED @14:41:36Z, qwen-code-ci-bot APPROVED @15:14:07Z and qwen-code-dev-bot APPROVED @15:24:21Z, all three commit_id == b1e4f307019d. The bot's earlier CHANGES_REQUESTED @14:22:28Z at this same head was a PR-template gate on the body ("this stops at the template gate, not on the code"), which the author closed at 14:33:04Z, so no code finding stands.
• CI at head — 143/143 check-runs fetched (items == total_count asserted), reduced to the latest attempt per lane name over 33 distinct lanes: 0 failures, 0 unfinished.
• No Critical from us — the diff is test-only: PRODUCTION COUNT = 0 of 1 files, the single changed file being packages/core/src/services/web-terminal-registry.test.ts.

Why a separate verification comment on a test-only PR: a green Test lane certifies that the added test passes, not that it pins anything. So it was executed against four mutations. Making the production release synchronous fails exactly one test — the new one — at :514. Removing the fake's detach guard at :82, which R2-1 calls "the only mechanism that lets" this test observe the release, leaves all 33 green, so that thread's premise does not hold. Removing releaseHost?.() fails 7 pre-existing tests while the new one passes, so the two failure sets are disjoint and the added test is not a restatement of existing release-path coverage.

Non-blocking, and not a request: :514 and :518 are both discriminating, but only :514 is specific to the defer axis — the :518 failure mode is also caught by two pre-existing tests.

This approval is scoped to head b1e4f307019d…. A further push, or a superseding verdict at a new head, voids it.