[yiliang114]

What this PR does

Pins process liveness in the one orphan-reap case of the review cleanup suite that trusted a hardcoded pid to be absent from the host's process table. The case now stubs the liveness probe for the duration of the sweep, using the same helper as two sibling orphan cases in the same block, so the fixture's "this launcher is dead" premise is a fact of the test instead of an assumption about the machine it runs on.

Why it's needed

Main CI failed: Qwen Code CI — src/commands/review/cleanup.test.ts > … > keeps the run alive when the post-kill UNLINK fails (#15737, run 40547925273) is not a defect in the unlink guard it is meant to pin. The case builds an orphan socket named for pid 626262 and never stubs process.kill, so when a busy runner happens to have a process at that pid the sweep correctly treats the socket as alive, skips it, and reports Nothing to clean for target "local" — an output that reads exactly like the guard under test having been removed. Giving the same fixture a pid that is certainly resident reproduces that CI output on an otherwise green tree, and the pinned fixture passes under the same condition.

Reviewer Test Plan

How to verify

Run the suite (cd packages/cli && npx vitest run src/commands/review/cleanup.test.ts) and confirm 109 tests pass. To see the failure this prevents, rename the fixture in that one case to a pid you know is alive (for example process.pid) and drop the new wrapper: the case then fails with "Nothing to clean for target \"local\"." instead of the assertion about the reaped line. Restoring the wrapper makes it pass with the same resident pid.

Evidence (Before & After)

Before — old assertion, fixture pid made resident:

× runCleanup > orphaned capture-tui servers > keeps the run alive when the post-kill UNLINK fails 9ms
+   "Nothing to clean for target \"local\".",
      Tests  1 failed | 108 skipped (109)

After — pinned probe, same resident fixture pid:

✓ src/commands/review/cleanup.test.ts (109 tests | 108 skipped) 4ms
      Tests  1 passed | 108 skipped (109)

Full file after the change: 109 passed.

Tested on

OS	Status
🍏 macOS	✅
🪟 Windows	⚠️
🐧 Linux	⚠️

Environment (optional)

A local worktree of main at 9acb3a5a1d68a, installed with corepack pnpm install --frozen-lockfile and built with npm run build; unit tests only.

Risk & Scope

• Main risk or tradeoff: none for product code; the change is confined to one test case and only replaces an assumption about the host's pid space with an explicit stub.
• Not validated / out of scope: the Windows and Linux lanes were not run locally. The enclosing block is describe.skipIf(process.platform === 'win32'), so only the POSIX lanes execute this case.
• Breaking changes / migration notes: none.

Linked Issues

Closes #15737

中文说明

这个 PR 做了什么

给 review cleanup 测试里唯一一个依赖"写死的 pid 在宿主机上不存在"的孤儿进程用例补上存活探针的桩。该用例现在在一次扫描期间固定 process.kill 的返回，复用同一代码块里另外两个孤儿用例使用的 helper，于是"这个启动器已经死了"从对运行机器的假设变成测试自己给出的事实。

为什么需要

Main CI failed: Qwen Code CI — src/commands/review/cleanup.test.ts > … > keeps the run alive when the post-kill UNLINK fails（#15737，run 40547925273）并不是它想要守住的那段 unlink 兜底逻辑的缺陷。该用例构造了一个以 626262 命名的孤儿 socket，却从未桩掉 process.kill；当繁忙的 runner 上恰好存在这个 pid 时，扫描会正确地把它判为存活并跳过，最后打印 Nothing to clean for target "local"——这个输出看起来和"兜底被删掉"一模一样。把这个用例的固定 pid 换成一个必然存活的 pid，就能在其它测试全绿的工作区里复现出同一行 CI 输出；而在同样的条件下，固定存活探针之后用例就能通过。

评审方式

运行该测试文件（cd packages/cli && npx vitest run src/commands/review/cleanup.test.ts），确认 109 个用例全部通过。如果想看到它防止的失败，把该用例的固定 pid 换成一个你确定存活的 pid（例如 process.pid）并去掉新加的包装：用例会以 "Nothing to clean for target \"local\"." 失败，而不是关于 reap 输出的断言失败；把包装加回来后，在同一个存活 pid 下就会通过。

证据（修改前后）

修改前——旧断言，固定 pid 恰好存活：

× runCleanup > orphaned capture-tui servers > keeps the run alive when the post-kill UNLINK fails 9ms
+   "Nothing to clean for target \"local\".",
      Tests  1 failed | 108 skipped (109)

修改后——固定存活探针，同一个存活的固定 pid：

✓ src/commands/review/cleanup.test.ts (109 tests | 108 skipped) 4ms
      Tests  1 passed | 108 skipped (109)

修改后整份文件：109 个用例通过。

本地验证范围

本地在 main（9acb3a5a1d68a）的独立工作区里用 corepack pnpm install --frozen-lockfile 安装、npm run build 构建，只跑了单元测试。Windows 与 Linux 两条 lane 未在本地运行；该代码块是 describe.skipIf(process.platform === 'win32')，只有 POSIX lane 会执行这个用例。

风险与范围

只改一个测试用例，不涉及产品代码；把对宿主机 pid 空间的假设换成显式桩，没有其它影响。

关联 issue

Closes #15737

[qwen-code-review-bot]

⏳ Approval still deferred — 1 PR CI workflow run(s) still in progress for 9aadf13; the finalize job approves automatically once every run lands green. finalize run

⏳ 审批仍在延迟中 —— 9aadf13 还有 1 个 PR CI workflow 在运行，全部通过后 finalize 任务会自动提交审批。查看 finalize 运行

[qwen-code-review-bot]

Thanks for the PR!

Template looks good ✓ — all nine required headings present, with a real before/after and the OS matrix filled in.

Problem: observed, not theoretical. Test (ubuntu-latest, Node 22.x) on run 40547925273 failed this exact case on all three attempts (retry x2), and the job log carries precisely the assertion diff the PR quotes:

-   "Reaped orphaned capture server: qwen-review-capture-626262-aaa",
+   "Nothing to clean for target \"local\".",
 ❯ src/commands/review/cleanup.test.ts:1562:41

I checked the diagnosis against the production code rather than taking it on trust. cleanup.ts probes with process.kill(pid, 0) and sets alive = code !== 'ESRCH', then if (alive) continue; — so a resident pid 626262 makes the sweep skip the socket before it ever reaches the unlink guard. That also explains why the failure landed on the second assertion: expect(...).not.toThrow() still passed, so nothing here contradicts the guard the case exists to pin. The runner was self-hosted and loaded (load[6.76 15.45 18.25], hosttests[17]), which is the condition the hypothesis needs.

Direction: aligned — CI flake removal on a suite this repo already treats as load-bearing. No CHANGELOG reference (test-only), and none expected.

One note on the linked issue, because the duplicate gate looks at it: #15737 is closed as completed but not by a merged PR — the closer resolves to null. You closed it yourself eight minutes before opening this PR, on the rationale that the signature "has not recurred since". I verified the change is genuinely absent from main at 9acb3a5a1d68a (that case still calls runCleanup('local') bare, and withDeadPids has only its two pre-existing call sites), so this is not a duplicate of a landed fix and I have not closed it. Flagging the manual close only because the gate cannot distinguish "flake stopped by luck" from "flake fixed" — and this PR is arguing the former is worth hardening anyway, which I agree with: a non-recurrence is not a fix, and the next busy runner can land on 626262 again.

Size: not applicable — no core paths (single file under packages/cli/src/commands/review/), and 0 production logic lines since the only changed file is *.test.ts. No Tier 1/Tier 2 concern, no large-PR advisory.

Approach: minimal, and it matches the neighborhood. Of the literal-pid cases in this block, 717171/727272 and 838383 already wrap in withDeadPids, while 515151 and 424242 stub process.kill inline because they need the opposite probe answer — so 626262 really was the only literal-pid case with no liveness control. I had considered the other available route, reusing the block's deadPid (a spawned-then-reaped child pid that eight earlier cases already rely on); it is a smaller diff but a weaker one — still a host assumption about pid reuse rather than a fact of the test — and it would churn the fixture name in the assertion. Your version is the better call.

Small description nit, not a code issue: "exactly as the five sibling orphan cases in the same block already do" overstates the precedent — two siblings use withDeadPids, and two more stub the probe to the opposite effect. Worth a tweak so the next reader is not sent looking for five.

Risk: no elevated risk signals — no high-risk path matched (and the file is excluded as a test regardless).

Moving on to code review. 🔍

中文说明

感谢贡献！

模板完整 ✓ —— 九个必需标题齐全，附了真实的 before/after，OS 矩阵也填了。

问题： 是已观测到的，不是理论性加固。run 40547925273 的 Test (ubuntu-latest, Node 22.x) 在三次尝试（retry x2）里都失败在这个用例上，job 日志里的断言 diff 与 PR 引用的完全一致：期望 Reaped orphaned capture server: qwen-review-capture-626262-aaa，实际得到 Nothing to clean for target "local".，位置 cleanup.test.ts:1562。

我没有直接采信这个归因，而是对着产品代码核了一遍。cleanup.ts 用 process.kill(pid, 0) 探活，alive = code !== 'ESRCH'，随后 if (alive) continue; —— 所以只要宿主机上 pid 626262 恰好存活，扫描就会在触及 unlink 兜底之前跳过这个 socket。这也解释了为什么失败落在第二个断言上：expect(...).not.toThrow() 依然通过，因此这里没有任何证据与它要守的那段兜底逻辑相矛盾。当时的 runner 是自托管且负载很高的（load[6.76 15.45 18.25]、hosttests[17]），正是这个假设需要的条件。

方向： 对齐 —— 这是在本仓库已视为关键路径的测试套件上消除 CI 抖动。无 CHANGELOG 关联（纯测试改动），也不需要。

关于关联 issue 需要说明一点，因为查重环节会看它：#15737 状态是 closed as completed，但不是由已合并的 PR 关闭的 —— closer 解析结果为 null。是你在开这个 PR 前八分钟自己关的，理由是"此后未再复现"。我已确认该改动确实不在 main（9acb3a5a1d68a）上：那个用例仍然是裸调 runCleanup('local')，withDeadPids 也只有原有的两个调用点。所以它不是已落地修复的重复 PR，我没有关闭它。之所以要把这个手动关闭点出来，是因为该环节无法区分"抖动碰巧没再出现"和"抖动已被修好"——而本 PR 主张前者同样值得加固，这一点我认同：没有复现不等于修复，下一台繁忙的 runner 仍可能撞上 626262。

规模： 不适用 —— 未触及核心路径（只有 packages/cli/src/commands/review/ 下一个文件），且生产逻辑行数为 0，因为唯一改动的文件是 *.test.ts。不涉及 Tier 1/Tier 2，也不触发大 PR 提示。

方案： 改动很小，且与相邻用例的做法一致。该代码块里使用字面量 pid 的用例中，717171/727272 与 838383 已经用 withDeadPids 包了，而 515151 与 424242 是内联桩掉 process.kill，因为它们需要相反的探活结果 —— 所以 626262 确实是唯一一个没有任何存活控制的字面量 pid 用例。我也考虑过另一条路：复用该块已有的 deadPid（先前八个用例依赖的、spawn 后已回收的子进程 pid）。那样 diff 更小，但更弱 —— 它仍然是对宿主机 pid 复用的假设，而不是测试自己给出的事实 —— 而且会改动断言里的 fixture 名字。你的做法更好。

一个描述上的小问题，不是代码问题："和同一代码块里其它五个孤儿用例的做法一致"把先例说过头了 —— 用 withDeadPids 的是两个，另外两个是把探针桩成相反效果。建议改一下措辞，免得后来的读者去找那"五个"。

风险： 无升级风险信号 —— 未命中高风险路径（且该文件本身按测试文件被排除）。

进入代码审查 🔍

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

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

[qwen-code-review-bot]

Code review

I read the diff against main at 9acb3a5a1d68a and against the production sweep it exercises. No critical blockers. What I checked, since a one-line test change is easy to wave through:

• The stub is correctly scoped. process.kill appears exactly once in the production sweep — cleanup.ts:679, process.kill(Number(m[1]), 0), the liveness probe. So pinning it cannot alter any other half of runCleanup; the reap itself goes through the mocked execFileSync.
• .not.toThrow() still pins what it pinned. An exception out of runCleanup propagates through withDeadPids's try/finally (whose finally only restores process.kill) and out to the expect, so the case still asserts that the EBUSY unlink is swallowed by the guard's catch {}. Wrapping did not soften the assertion.
• The leak guard is intact. The case's own finally { mocks.rmSync.mockReset(); } is untouched, so the throwing rmSync still does not bleed into the six orphan cases that follow.
• Scope resolution is fine. withDeadPids is a hoisted function declaration inside the same describe.skipIf(process.platform === 'win32') callback (declared at 1456, called at 1566), so the new call site sees it — and win32 skips the block entirely, matching the PR's own note.
• The new comment's factual claim holds. Of the six literal-pid fixtures in the block (717171, 727272, 838383, 515151, 626262, 424242), 626262 was the only one with no liveness control: two wrap in withDeadPids, two stub process.kill inline for the opposite (alive) answer, and process.pid is deliberately live. The wording scopes itself to "hardcoded pid", so it does not misdescribe the eight deadPid-based fixtures either.

One optional suggestion, not a blocker. The added comment (1560-1564) restates the helper's own docstring at 1451-1455, which already says a literal pid can be alive on a busy host and the test would then "fail for a reason that has nothing to do with what it pins". The case also already carries its mutant note above (the catch {} explanation), so this is now nine comment lines over a two-line body, and the two pre-existing withDeadPids call sites (1479, 1509) carry no equivalent. The half that does earn its place is the misdiagnosis risk — that a liveness flake here reads exactly like the unlink guard having been removed. If you want it tighter, keep that sentence and drop the rest; equally fine to leave as is.

Independent-proposal comparison, for the record: my alternative was to swap 626262 for the block's existing deadPid (a spawned-then-reaped child pid, which eight earlier cases already trust), needing no wrapper at all. It is a smaller diff but a weaker guarantee — still a host assumption about pid reuse rather than a fact of the test — and it churns the fixture name inside the assertion. Your approach beats mine and matches the nearer neighbors.

Testing evidence

This is an unattended CI run, so I did not build or execute anything from this PR; the evidence below is the PR's own CI, read through the API for commit 9aadf13efcea13b051c3dfc2fd6cd34c97705ef9.

The honest headline: the one lane that matters is still running. Test (ubuntu-latest, Node 22.x) is in_progress, and it is the only Test job that will execute this case — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) are both skipped on this commit. That is also the lane that produced the original flake. Lint & Static (ubuntu-latest, Node 22.x) passed, which covers typecheck and lint for the changed file.

CI results for 9aadf13 — this table auto-updates as CI workflows complete:

Check	Conclusion
Installed npm Node26 private Bun acceptance	⏳ running
TUI parity snapshots (ink vs opentui)	❌ failure
Check TUI impact	✅ success
Classify PR	✅ success
Integration Tests (no-AK, No Sandbox)	✅ success
Lint & Static (ubuntu-latest, Node 22.x)	✅ success
Managed Agent gate	✅ success
OpenTUI no-flicker gate	✅ success
Select the Managed Agent tier	✅ success
Test (ubuntu-latest, Node 22.x)	✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x)	✅ success

_{One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。}

triage and review-pr are also in flight on this commit; those are bot orchestration jobs, not this PR's CI, so they should not be read as product signal either way. No check is red, so there is no failing-job excerpt to quote. I fetched once and did not poll — this repo's unit suite runs long, and the finalize workflow rewrites the table above in place once CI settles.

Not verified: that the flake is actually gone. Worth being blunt about why this matters more than it looks. A green Test (ubuntu-latest…) run does not substantiate this fix, because the failure only manifests when the host happens to have pid 626262 resident. On a quiet runner the case passes identically with and without the diff — so green CI is consistent with both "fixed" and "got lucky again", and cannot distinguish them. That is precisely the shape #15737 was closed on ("has not recurred since").

The before/after in the PR description — renaming the fixture to a resident pid, dropping the wrapper, seeing "Nothing to clean for target \"local\".", then restoring it — is the right experiment and would settle this. But it is the author's reported local result on macOS, not something I re-ran, and I am not presenting it as evidence. What I did independently confirm is the historical half: the archived log for job 122343675142 shows the failure reproducing on all three attempts with exactly that assertion diff at cleanup.test.ts:1562, on a self-hosted runner reporting load[6.76 15.45 18.25] and hosttests[17].

Sandboxed verification would settle the remaining half: @qwen-code /verify — the load-bearing claim is that pinning the probe (not a quiet runner) is what makes this case deterministic, and that is exactly what an A/B against the base build with the fixture pid forced resident would prove. A green suite cannot, since it passes with the wrapper removed whenever 626262 is free. There is no TUI surface here, so /tmux has nothing to add.

中文说明

代码审查

我把 diff 对着 main（9acb3a5a1d68a）以及它所测试的生产扫描逻辑一起读了。没有阻断性问题。 一行的测试改动很容易被顺手放过，所以我逐条核了下面这些：

• 桩的作用范围是对的。 生产扫描里 process.kill 只出现一次 —— cleanup.ts:679 的 process.kill(Number(m[1]), 0)，即存活探针。因此固定它不会影响 runCleanup 的任何其它环节；真正的 reap 走的是已被 mock 的 execFileSync。
• .not.toThrow() 守的东西没有变弱。 runCleanup 抛出的异常会穿过 withDeadPids 的 try/finally（其 finally 只负责还原 process.kill）传到 expect，所以该用例依然在断言 EBUSY 的 unlink 被兜底的 catch {} 吞掉。包一层并没有放松断言。
• 防泄漏逻辑完好。 用例自己的 finally { mocks.rmSync.mockReset(); } 未被改动，因此会抛异常的 rmSync 仍然不会渗到后面的六个孤儿用例里。
• 作用域没问题。 withDeadPids 是同一个 describe.skipIf(process.platform === 'win32') 回调里被提升的函数声明（1456 行声明，1566 行调用），新调用点能看到它；win32 会整块跳过，与 PR 自己的说明一致。
• 新注释里的事实性说法成立。 该块六个字面量 pid fixture（717171、727272、838383、515151、626262、424242）中，只有 626262 没有任何存活控制：两个用 withDeadPids 包，两个内联桩掉 process.kill 以得到相反的（存活）结果，process.pid 则是故意存活的。注释把范围限定在"写死的 pid"，因此也没有错述那八个基于 deadPid 的 fixture。

一个可选建议，不是阻断项。 新增注释（1560-1564 行）重复了 1451-1455 行 helper 自己的 docstring —— 后者已经说明字面量 pid 在繁忙宿主机上可能存活，于是测试会"因为与它要守的东西无关的原因失败"。该用例上方也已经有自己的变异体注释（解释 catch {}），所以现在是九行注释压着两行代码，而两个既有的 withDeadPids 调用点（1479、1509 行）都没有类似注释。真正有价值的那一半是"误诊风险"—— 这里的存活抖动看起来和 unlink 兜底被删掉一模一样。如果想更紧凑，保留那一句、去掉其余即可；原样保留也没问题。

记录一下我的独立方案对比：我原本想的是把 626262 换成该块已有的 deadPid（spawn 后已回收的子进程 pid，前面八个用例已经依赖它），这样完全不需要 wrapper。diff 更小，但保证更弱 —— 它仍然是对宿主机 pid 复用的假设，而不是测试自己给出的事实 —— 而且会改动断言里的 fixture 名字。你的做法比我的更好，也和更近的邻居一致。

测试证据

这是一次无人值守的 CI 运行，因此我没有构建或执行本 PR 的任何代码；下面的证据是通过 API 读取的、本 PR 自己在 commit 9aadf13efcea13b051c3dfc2fd6cd34c97705ef9 上的 CI 结果。

直说结论：唯一关键的那条 lane 还在跑。 Test (ubuntu-latest, Node 22.x) 状态是 in_progress，而它是唯一会执行该用例的 Test job —— 本 commit 上 Test (macos-latest, Node 22.x) 与 Test (windows-latest, Node 22.x) 都是 skipped。这也正是当初产生抖动的那条 lane。Lint & Static (ubuntu-latest, Node 22.x) 已通过，覆盖了改动文件的类型检查与 lint。

（CI 结果表见上方英文部分的标记区域，由 finalize 工作流在 CI 跑完后原地更新。）

triage 与 review-pr 在该 commit 上也还在跑；它们是 bot 编排 job，不是本 PR 的 CI，无论结果如何都不应当作产品信号。目前没有红色 check，所以没有失败日志片段可引。我只取了一次结果，没有轮询 —— 本仓库的单测套件耗时很长，表格会由 finalize 工作流在 CI 稳定后就地重写。

未验证：抖动是否真的消失了。 这一点值得讲透，因为它比看上去更重要。Test (ubuntu-latest…) 变绿并不能证明这个修复有效，因为该失败只在宿主机恰好有 pid 626262 存活时才出现。在安静的 runner 上，有没有这个 diff 用例都一样通过 —— 所以绿色 CI 同时兼容"修好了"和"这次又走运了"，无法区分二者。而这恰恰就是 #15737 被关闭时所依据的形态（"此后未再复现"）。

PR 描述里的 before/after —— 把 fixture 换成一个必然存活的 pid、去掉 wrapper、看到 "Nothing to clean for target \"local\"."、再加回来 —— 是正确的实验，也确实能定论。但那是作者在 macOS 上自报的本地结果，不是我重跑的，我不把它当作证据呈现。我独立确认的是历史性的一半：job 122343675142 的归档日志显示该失败在三次尝试中全部复现，断言 diff 与描述一致，位置 cleanup.test.ts:1562，且当时是一台自托管 runner，报告 load[6.76 15.45 18.25]、hosttests[17]。

沙箱验证可以定下剩下的一半：@qwen-code /verify —— 真正吃重的说法是"让该用例变得确定性的是固定探针，而不是 runner 恰好安静"，而这正是拿 base 构建做 A/B、并把 fixture pid 强制设为存活所能证明的。绿色套件证明不了，因为只要 626262 空闲，去掉 wrapper 也照样通过。这里没有 TUI 界面，所以 /tmux 帮不上忙。

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

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

[qwen-code-review-bot]

Confidence: 4/5 — the change is correct, minimal, and removes a real host assumption; the only nits are a redundant comment block and an overstated "five siblings" in the description, neither of which is worth holding the PR for.

Stepping back: this is a six-line test-only change that makes one case deterministic, and it does it the way the neighboring cases in the same block already do it. I went looking for a reason to say no and did not find one that survives contact with the evidence.

The thing I want to be explicit about, because it is the only real judgment call here, is whether this PR is needed at all. #15737 was closed on the grounds that the signature had not recurred, and the product code was never wrong — the sweep correctly declined to reap a socket whose pid looked alive. So one could argue this is hygiene. I don't think that reading holds. The failure did land on main CI, it failed all three retries, and its output (Nothing to clean for target "local") is actively misleading: it reads exactly like the unlink guard having been deleted, which is the one thing the case exists to pin. A test that can red-line main and point the investigator at the wrong code is a defect in the suite, not a cosmetic preference — and the fix costs nothing in product risk. Non-recurrence over a few pushes is luck on a quiet runner, not a fix.

On my own independent proposal: I'd have reached for the block's existing deadPid instead of the wrapper. Having read the code closely, the PR's choice is better than mine — deadPid still leans on the host not reusing a pid, whereas stubbing the probe makes "this launcher is dead" a fact of the test. I'm recording that I changed my mind rather than quietly adopting the PR's framing.

What I would not claim: that the flake is now provably gone. Green CI cannot show it, since the case passes with the wrapper removed whenever 626262 happens to be free — the suite is green either way on a quiet runner. That is a property of flake fixes generally, not a defect in this one, and the change is strictly monotonic: it can only remove a failure mode, never add one. The /verify A/B named in my Stage 2 comment is the way to close that gap if anyone wants it closed; I don't think it should block the merge.

In six months I would thank the author for this, not curse them: the next person to see this case fail will now be looking at the unlink guard, which is the correct place to look.

Approval deferred until CI lands green on 9aadf13efcea13b051c3dfc2fd6cd34c97705ef9. Two of this PR's own workflow runs are still in flight — Qwen Code CI (which carries Test (ubuntu-latest, Node 22.x), the only lane that executes this case) and tui-parity. Approving now would attest to a result that does not exist yet, so I am posting no review in this run; the finalize workflow will place the same commit-pinned approval once every check on that commit completes green, and will withhold it if anything lands red or the head moves.

中文说明

Confidence: 4/5 —— 改动正确、极小，并且真的去掉了一个对宿主机的假设；仅有的两个小问题是注释冗余、以及描述里"五个同级用例"说过了头，都不值得为此卡住这个 PR。

退一步看整体：这是一个六行的纯测试改动，让一个用例变得确定性，而且用的就是同一代码块里相邻用例已有的做法。我特意去找否决它的理由，没有找到任何经得起证据检验的。

我想把这里唯一的判断难点讲清楚，就是这个 PR 到底需不需要。#15737 是以"该签名此后未再复现"为由关闭的，而且产品代码从头到尾没有错 —— 扫描正确地拒绝去 reap 一个 pid 看起来还存活的 socket。所以可以说这只是代码卫生。我不认同这个读法。该失败确实落在了 main CI 上，三次重试全失败，而且它的输出（Nothing to clean for target "local"）有实质误导性：它看起来和 unlink 兜底被删掉一模一样，而那正是该用例要守的唯一东西。一个能把 main 弄红、并把排查者指向错误代码的测试，是套件本身的缺陷，不是审美偏好 —— 而这个修复在产品风险上的代价是零。几次 push 内没有复现，只是安静 runner 上的运气，不是修复。

关于我自己的独立方案：我原本会去用该块已有的 deadPid，而不是这个 wrapper。仔细读完代码后，我认为 PR 的选择比我的更好 —— deadPid 仍然依赖宿主机不复用该 pid，而桩掉探针则让"这个启动器已经死了"成为测试自己给出的事实。我把"我改变了看法"记录下来，而不是悄悄采纳 PR 的框架当作自己的结论。

我不会主张的是：抖动现在已被证明消失。绿色 CI 证明不了这一点，因为只要 626262 恰好空闲，去掉 wrapper 用例也一样通过 —— 在安静的 runner 上两种写法都是绿的。这是所有抖动修复的共性，不是这个修复的缺陷，而且该改动是严格单调的：它只可能移除一种失败模式，不可能新增。如果确实要把这个缺口补上，我在 Stage 2 评论里点名的 /verify A/B 就是办法；但我认为它不应该阻塞合并。

六个月后我会感谢作者，而不是骂他：下一个看到这个用例失败的人，会去查 unlink 兜底 —— 而那正是应该查的地方。

批准将延迟到 CI 在 9aadf13efcea13b051c3dfc2fd6cd34c97705ef9 上全绿之后。 本 PR 自己还有两个 workflow run 在跑 —— Qwen Code CI（其中包含 Test (ubuntu-latest, Node 22.x)，即唯一会执行该用例的 lane）和 tui-parity。此刻批准等于为一个尚不存在的结果背书，所以本次运行不提交任何 review；finalize 工作流会在该 commit 上所有 check 全绿后落下同一个绑定 commit 的批准，若出现红色或 head 发生移动则不予批准（延迟批准标记已附在本评论中）。

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

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

[yiliang114]

Updated both language versions of the description: two sibling cases use the helper, not five. The patch remains at 9aadf13efcea13b051c3dfc2fd6cd34c97705ef9; no code change was needed for the current feedback. The existing comment explains the fixture's non-obvious liveness premise, so I left that optional wording suggestion unchanged. #15737 is reopened because this fix has not merged.

The current Ubuntu unit, lint/static, no-AK integration and web-shell smoke jobs passed. The TUI parity job remains red, and installed-npm acceptance and automated review were queued in this snapshot. This is not an all-green or merge-ready report.

I inspected the failed TUI job and its tui-migration-9aadf13efcea13b051c3dfc2fd6cd34c97705ef9-attempt-1 artifact. In shell-focus-bun-opentui-K78JJ2, the assertion at line 564 saw the byte stream without the final 09, but CHILD-EVENTS.json records that byte at 1793156790084, 431 ms after the Shift+Tab entry in KEYS.json, before the child exited normally. The test waits for the footer's approval-mode update and then immediately reads the child log; the footer update does not acknowledge child receipt. This supports delayed observation for this failed run, rather than a permanently dropped byte. The parity job runs a separate integration suite and CLI process; it does not execute the cleanup unit case or its temporary process.kill stub.

That needs a separate focused test repair. The original #15712 job actually failed at line 543 because its frame lacked DRAFT_OWNED, not at the missing-tab assertion. A fix for the latter alone would not establish that #15712 is resolved. No further rerun was attempted after the earlier reported workflow-file rerun rejection.

[yiliang114]

The one red check here (TUI parity snapshots) is not this PR's.

integration-tests/interactive/tui-foreground-shell.test.ts > keeps PTY bytes and composer drafts with their actual input owner fails at :564 — expect(c.bytes()).toBe(shiftedInput) — expecting the trailing TAB (…494e4e45525f4209) and receiving the buffer one byte short (…494e4e45525f42). This PR only changes packages/cli/src/commands/review/cleanup.test.ts, and the failing test file is byte-identical to main (5d64af81e3), so nothing here produced it.

Root cause is in the test: that case has three byte assertions, and two of them (resumed input at :549, paste at :571) first wait for c.bytes().length to reach the expected length. The Shift+Tab assertion is the only one without that guard — it runs immediately after the frame shows plan mode, while the PTY write can still be in flight.

#15763 adds exactly that missing guard. Once it lands, this check should stop failing here for a reason that has nothing to do with this diff. The rest of CI on this head is green.

[qwen-code-review-bot]

APPROVE @ ff86f8885f2f344b5bf3caa50adcea2051729fc3（评审面冻结：base 78c21044，1 文件 +6/−1，纯测试）。

结论：无 P0–P2 发现。这个用例原先确实押注在宿主 pid 表上，钉死探针是正确修法。

核过的点（读 head 全文与 cleanup.ts，不是只看 hunk）：

1. 被钉的是唯一的漏网点。 这个 describe 里所有硬编码 pid 的夹具：717171/727272（:1479 用 withDeadPids）、838383（:1509 同）、515151（:1533-1540 显式 EINVAL stub）、424242（:1618-1625 显式 EPERM stub），只有 626262（:1552）此前直接跑 runCleanup —— 确实是最后一个没钉的。其余夹具用的是 spawnSync(...).pid（:866-867）这种运行期必然已死的 pid，不属同类风险。
2. stub 覆盖的是探针而不是杀人路径。 runCleanup 里 process.kill 只出现在 liveness 探测（cleanup.ts:679，ESRCH 判死、其余判活），实际清理由 execFileSync 走（本测试已 mock）。所以把 process.kill 换成恒抛 ESRCH 不会掩盖任何真实行为。
3. 还原是 finally 的：withDeadPids（:1456-1466）在 try/finally 里恢复原函数，异常路径也不会把 process.kill 泄漏给后续用例。
4. 断言面没动：not.toThrow()、Reaped orphaned capture server 那行、以及 rmSync 抛 EBUSY 的设定都原样；改的只是「这个 launcher 是死的」这一前置事实从假设变成事实。
5. 失败形态是真的读起来像被测对象坏了：夹具活着时 sweep 会输出 Nothing to clean for target "local"，与「unlink 守卫被删掉」同形 —— 正文这条归因我认同。

CI 在 head 上必须说清楚：Test (ubuntu-latest, Node 22.x) 与 web-shell E2E Smoke (ubuntu-latest, Node 22.x) 两条都是 cancelled，不是用例判红 —— 两条的注解都是 The operation was canceled.，且 07:47:46Z 起跑、08:17:43Z / 08:18:40Z 结束，正好撞上 ~30 分钟上限。这两条不携带任何指向本 diff 的信号，但也没有给出绿，合并前需要重跑它们；其余 11 success / 4 skipped，Installed npm Node26 private Bun acceptance 仍 queued。

未核：本机没有该套件的构建，109 tests passed 我没有复现；Windows lane 按 describe.skipIf(win32) 本就不跑这个用例。

[yiliang114]

Status on the trail to approval:

• The review bot APPROVED this head at 2026-10-28 08:41 after reading the whole file, not just the hunk — qwen-code-review-bot review on ff86f8885f2f344b5bf3caa50adcea2051729fc3, verdict: no P0-P2 findings, and it verified that this fixture is the only pid-table fixture in the describe that still leaks the host pid namespace.
• TUI parity no longer fails on this head, because PR fix(test): wait for focused shell tab input before asserting bytes #15763 merged the world it depended on.
• Current CI on the head: 0 failing, Lint & Static SUCCESS.
• What is missing is the second human approval — the main ruleset counts human reviewers only, so reviewDecision still reads REVIEW_REQUIRED.

Test-only change, +6/−1. If you are a human reviewer looking at this PR for the first time, the earlier review comment on this thread is your fault-check entry point for the specific reasoning.

[qqqys]

Approving at 918245eab316cb42f4657a4dae9dfcc456302c1a.

One test file, +6/−1. For a change that stubs a system call inside a test, the only question that matters is whether the stub hides part of the behaviour the test exists to pin — a liveness stub that also masked the kill path would turn this case green while testing nothing. So I read the production call site rather than taking the wrapper's name for its scope.

The stubbed call is a pure probe, and the real work goes through paths this test still mocks and asserts on. process.kill appears exactly once in packages/cli/src/commands/review/cleanup.ts, at line 679: process.kill(Number(m[1]), 0). Signal 0 never delivers anything — it only asks whether the pid exists — and the surrounding code treats it purely as an oracle, with alive = (e as NodeJS.ErrnoException).code !== 'ESRCH' at line 689 and the comment above it spelling out that ESRCH is the dead-pid signal. The actual reaping runs through execFileSync (line 733, the tmux -L <name> kill-server invocation) and the unlink through rmSync, both of which this suite mocks and asserts against. So forcing the probe to answer ESRCH changes only the premise "this launcher is dead"; it cannot suppress a kill, an unlink or an error the case is supposed to observe.

The assertion surface is untouched, which is what keeps the case load-bearing. The diff retains not.toThrow() and the writeStdoutLine expectation on Reaped orphaned capture server: ${orphan}, and the mocks.rmSync implementation still throws EBUSY. That combination is the actual subject of the test — a post-kill unlink failure must not abort the sweep — and it still fails if the guard is removed, because the EBUSY throw would then propagate out of runCleanup. Pinning liveness does not weaken any of that; it removes an unrelated way for the same case to go red.

The helper is exception-safe. withDeadPids (lines 1456-1466) saves realKill, installs a function that always throws ESRCH, invokes fn() inside try, and restores in finally. A failing assertion therefore cannot leak the stub into later cases in the file, which is the usual hazard with monkey-patching a process global mid-suite.

This was the last fixture in the block that bet on the host's pid table. The other hardcoded-pid orphan cases already route through the same helper (lines 1479 and 1509) or install an explicit error stub for the case they are about — the EINVAL and EPERM fixtures — and the remaining fixtures derive a pid from a real spawnSync, which is dead by construction. So the change makes the block uniform rather than patching one instance of a pattern that persists elsewhere.

The head moved by two merges from main after the bot's approval, and neither touched this file. The bot's review names ff86f888 while the live head is 918245ea; compare between them reports 27 changed files with status=ahead, behind_by=0, and filtering that list for review/cleanup returns nothing. The test file is byte-identical across the two heads, so that round's reading of the whole file — including its enumeration of the sibling fixtures, which matches mine — applies to what I am approving.

On CI, and this closes the one caveat that round left open. It flagged Test (ubuntu-latest, Node 22.x) and web-shell E2E Smoke (ubuntu-latest, Node 22.x) as cancelled rather than green at the earlier head, hitting the ~30-minute ceiling, and said they needed a re-run before merge. At this head both completed and passed — Test (ubuntu-latest, Node 22.x) in 20m45s, which is the lane that actually executes this suite, and web-shell E2E Smoke in 39m39s. Lint & Static (7m7s), Integration Tests (no-AK, No Sandbox) (17m36s), TUI parity snapshots, OpenTUI no-flicker gate and Check TUI impact all passed as well. There are zero failing and zero pending checks on this head.

There were no prior reviews requesting changes and no inline threads on this PR, so there is no earlier blocking finding to re-verify. What I did not do is re-run the suite myself: the 109-test tally and the before/after reproduction — renaming the fixture to a resident pid to make the old assertion fail with Nothing to clean for target "local" — are the author's measurements. The Test (ubuntu) lane passing on this head is my evidence that the suite is green, and the code reading above is my evidence that it is green for the right reason.

[chiga0]

No blocking findings.
Approval blockers: none.

Tier: Scan — test-only change, no production code touched.

Checked:

• withDeadPids helper at cleanup.test.ts:1456-1465: saves and restores process.kill in a finally block, throws ESRCH to make all pids appear dead to the sweep. Restore is unconditional even if fn() throws.
• Two sibling orphan cases at lines 1479 and 1509 use the identical wrapper; this case was the only one in the block without it.
• The try/finally in the patched test resets mocks.rmSync; withDeadPids is called inside that try, so the pid-stub cleanup nests correctly within the mock-reset cleanup.
• No other test cases in the file use a hardcoded bare pid without a liveness stub or without process.pid as a reliably alive pid.
• Latest two commits (ff86f8885f2f, 918245eab316) are merges from main; the code change is confined to 9aadf13efcea.

Cross-check: qwen-code-review-bot approved at ff86f8885f2f with no findings. Confirmed: the code change is unchanged between that SHA and current HEAD.

Not covered: no production-code change, no persisted format or protocol touched — deeper rungs not applicable.

Reviewed with AI assistance.

[qwen-code-review-bot]

Released in v0.26.3.