feat(review): capture-tui — rendering claims get pixels, not prose - #8894
feat(review): capture-tui — rendering claims get pixels, not prose#8894wenshao wants to merge 14 commits into
Conversation
|
Re-run at the author's request. The head moved since the last pass (
Moving on to code review. 🔍 中文说明应作者请求 re-run。自上次审查后 head 已移动（
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review — re-run at the new headRe-read the full diff at The three behaviour fixes are real, minimal, and pinned ( The six pin repairs do what they say ( The nine deferred round-1 items are coverage/documentation, deferred under the criterion this PR's predecessor adopted (behaviour defects block, coverage gaps don't), with the reason on the record. Acceptable. Round 2 found thirteen Suggestions, no Criticals. I spot-checked the three closest to behaviour: the missing test for the What the last pass said about the bulk of the diff still holds — that code is unchanged: the isolation property is structural and pinned (every tmux call Files changed (14 of 14)
Test evidence — this PR's own CIPer the gate rules this review is static: no PR code was built or executed here. Evidence is the PR's own CI on the reviewed commit, quoted below — and it is now fully settled green, including the lane that was still running on the last pass. The decisive upgrade over the previous pass: the ubuntu lane installs tmux (the CI tooling step from #8792 is on
Not verified: the author's specific live-smoke artifacts (the macOS 80-column wrap pair, the #8368 dogfood frames) remain the author's claim — they illustrate the feature against another PR's content, which CI cannot replay; they are not load-bearing for this PR's own contract. Windows behaviour remains out of scope per the PR body (tmux semantics; the command refuses with an explanation where tmux is absent). 中文说明代码审查（新 head 重跑）在 三项行为修复真实、最小且有钉（ 六处钉子修复如实（ round-1 延后的九项均为覆盖/文档，按前身 PR 确立的判据（行为缺陷阻塞、覆盖盲区不阻塞）延后，理由在案，可接受。 Round 2 共十三条 Suggestion、无 Critical。抽查了三条最接近行为的： 上次对 diff 主体的评价仍然成立——那部分代码未变：隔离性质是结构性且被钉住的（每个 tmux 调用带 测试证据（本 PR 自己的 CI）按门禁规则，本审查为静态：未构建或执行任何 PR 代码。证据为被审 commit 上本 PR 自己的 CI——现已完全落绿，包括上次仍在运行的 lane。相对上次的关键升级：ubuntu lane 安装 tmux（#8792 的 CI 工具步骤已在 未验证：作者的具体实机冒烟产物（macOS 80 列折行对、#8368 dogfood 帧）仍是作者的主张——它们以另一 PR 的内容演示本功能，CI 无法重放；对本 PR 自身契约不承重。Windows 行为按 PR 正文在范围外（tmux 语义；无 tmux 时命令明确拒绝并说明）。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean re-run: round 1's three behaviour defects are fixed and pinned, its six vacuous pins repaired, round 2 returned only Suggestions, and CI is fully green including the real-tmux leg. What keeps it at 4 rather than 5 is the standing follow-up track (the missing Stepping back: the two new commits are exactly what a post-review push should look like — no scope creep, each hunk answering one named finding, and the fix for the completion-flag bug rebuilt the pin that would have caught it (which the author disclosed was accidentally deleted in the old PR's round 18; that kind of plain-spoken accounting is what a maintainer wants to read). The deferred items follow the criterion this PR's predecessor adopted — behaviour defects block, coverage gaps don't — and the nine deferrals are genuinely coverage/documentation, spot-checked. The question that mattered most on the last pass — does the behavioural claim rest on the author's word? — is now settled by the PR's own CI: the ubuntu lane installs tmux, the real-tmux leg ran 109/109 against actual tmux servers, and the three new pins from this delta are in that count. The evidence-ladder honesty remains the feature's core property, and it survived two adversarial review rounds intact. Verdict: approve. CI is settled green on 中文说明回顾全局：两个新 commit 正是评审后推送应有的样子——无范围蔓延，每个 hunk 回应一条点名的发现；完成标志缺陷的修复重建了本应拦住它的钉子（作者披露该钉子曾在旧 PR round-18 被误删——这种如实说明正是维护者想看到的）。延后项遵循前身 PR 确立的判据——行为缺陷阻塞、覆盖盲区不阻塞——九项延后经抽查确为覆盖/文档类。 上次最关键的问题——行为主张是否只凭作者一面之词——现已由本 PR 自己的 CI 裁决：ubuntu lane 安装 tmux，real-tmux 腿在真实 tmux server 上跑出 109/109，本次 delta 的三枚新钉在列。证据阶梯的诚实性是该功能的核心性质，经两轮对抗性评审仍完好。 裁决：批准。CI 已在 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed; I did not run the suite live (no evidence need arose from the static trace).; chunk 9: none (vitest itself was not runnable for lack of installed deps; substituted by executing the identical assertions against the real module).; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget., and 6 more.
Test Plan (not a blocker): 2486 passed — this review observed 19570, 1200, 1502, 481, 3069, 473 passed.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks I started were completed; I did not run the suite live (no evidence need arose from the static trace).；chunk 9：none (vitest itself was not runnable for lack of installed deps; substituted by executing the identical assertions against the real module).；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.，另有 6 条。
Test Plan（非阻断）：2486 passed — this review observed 19570, 1200, 1502, 481, 3069, 473 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
Eighteen findings, no Criticals. Responses consolidated into one comment — replying per thread hit GitHub's secondary rate limit earlier in this work, and one comment carries the same information. Threads resolved individually. Three were behaviour, not coverage — fixed in
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): This PR (#8894, QwenLM/qwen-code) adds qwen review captu...: none — all planned checks completed within budget.; This PR (#8894, QwenLM/qwen-code) adds qwen review captu...: none — all planned checks completed within budget.; This PR (#8894, QwenLM/qwen-code) adds qwen review captu...: none — finished within budget.; This PR (#8894, QwenLM/qwen-code) adds qwen review captu...: none — the full hunk and all mandated reads completed within budget.; This PR (#8894, QwenLM/qwen-code) adds qwen review captu...: none — all planned checks completed within budget.`, and 2 more.
Test Plan (not a blocker): 2486 passed — this review observed 19046, 19570, 1200, 1502, 481, 3069, 473 passed.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：This PR (#8894, QwenLM/qwen-code) adds qwen review captu...：none — all planned checks completed within budget.；This PR (#8894, QwenLM/qwen-code) adds qwen review captu...：none — all planned checks completed within budget.；This PR (#8894, QwenLM/qwen-code) adds qwen review captu...：none — finished within budget.；This PR (#8894, QwenLM/qwen-code) adds qwen review captu...：none — the full hunk and all mandated reads completed within budget.；This PR (#8894, QwenLM/qwen-code) adds qwen review captu...：none — all planned checks completed within budget.`，另有 2 条。
Test Plan（非阻断）：2486 passed — this review observed 19046, 19570, 1200, 1502, 481, 3069, 473 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 89 passed · 1 failed · 90 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：89 通过 · 1 失败 · 90 总计 Verification reportPR #8894 deep verification —
|
| # | Cell | Environment / oracle | Result |
|---|---|---|---|
| F1 | head fidelity (PR smoke, 113-char line @ 80 cols) | .ans line lengths; manifest rung/settle |
exit 0; .ans = 80 + 33; evidence:"ans-only"; settledBy:"fixed-delay"; named freeze degradation; socket reaped |
| F2 | -J control, same pane |
capture-pane -p -J vs -e -N -T |
-J → one 113-char line (wrap erased); no--J → two lines 80+33 |
| F3 | isolation — standing user server | tmux -L user ls before/after; socket census |
user session identical before/after; no private socket left; pane shows only own output |
| F4 | SIGTERM mid-capture | exit code; socket census | one server existed mid-run; exit 143; 0 orphan sockets; no manifest |
| F5 | documented --out reuse |
.ans/manifest content across two runs |
run-1 evidence replaced by run-2 cleanly; manifest describes run 2 |
| F6 | settle contract | settledBy + degradedBecause |
until-match / timeout-late-frame / ready-withheld all correct, incl. "keys were NOT sent" |
| F7 | base control (HEAD^1) |
review capture-tui + review --help |
base rejects it as unknown; --help lists capture-local but not capture-tui |
| F8 | semantic gates (tmux present) | exit-3 + reason per gate | bad regex / blank-pane regex / cols 0 / NaN settle / empty cmd / bad cwd / timeout ceiling all refuse; --keys=-l typed into the shell verbatim |
| F9 | cleanup sweep | stdout + socket census | dead-pid orphan reaped & unlinked; live-pid socket spared |
| F10 | PR's literal smoke on dash |
rendered line | renders == PANEL: X == (brace un-expanded) — see Finding 2 |
Witnesses: 02-capture-ab-real-tmux.png (49/49 A/B run), 01-refusal-contract-no-tmux.png
(22/22 no-tmux refusal contract), 05-cleanup-sweep-storm.png (sweep reaps dead, spares live).
The load-bearing comparison is F2: with -J the very clipping/wrap a layout claim is about is
re-flowed away; the PR's physical -e -N -T capture keeps it. That is the "5/9 broken→fixed"-shaped
delta for this PR — identical input, opposite evidentiary value, decided solely by the no--J rule.
Findings
1. (Low) Pane holder survives a clean reap when the captured command is an interactive shell
After a successful capture whose reap reports clean (exit 0, private socket removed, server
dead), the pane holder process can remain alive when the captured command is an interactive
shell that becomes its own process-group leader. The description's "kill-server reaps the holder
along with everything else" does not hold on this boundary.
Reproduce (tmux present):
node dist/cli.js review capture-tui --command 'sh' --cols 60 --rows 8 --settle-ms 1000 --out /tmp/lk
# exit 0, /tmp/lk.ans written, socket gone, then:
ps -eo pid,ppid,args | grep 'lk.holder-ready' | grep -v grep
# -> outer `sh -c 'trap : INT QUIT ...'` still alive (reparented to pid 1)Measured (04-holder-leak-interactive-pane.png): --command 'sh' → 1 surviving holder;
control --command 'sleep 30' → 0. Mechanism: teardown SIGHUP goes to the pane's foreground
process group, which the interactive sh has taken over; the outer holder sits in a different
group, is not HUP'd, and falls into its bounded while … sleep 60 loop.
Mitigations already in the design: the hold loop is bounded (180 × 60 s) and the watchdog
(sleep 10800; kill -9 -$$) self-terminates the group at ~3 h; the tmux server and socket are
correctly reaped, and nothing about the survivor can reach the user's own tmux. So this is
resource hygiene on an edge input, not an isolation break. Severity kept low for that reason, but
the "reaps everything" wording is overstated for this case.
Suggested direction (not applied)
Have the holder also trap 'kill -9 -$$ 2>/dev/null' HUP (in addition to trap : INT QUIT), or
setsid-isolate and explicitly kill the holder group in reap(). Either should be re-verified with
the L1/L2 repro above so a non-interactive capture stays byte-identical.
2. (Suggestion) The Reviewer Test Plan's literal smoke is bash-dependent — it does not render the documented wrap on Linux lanes
Walking the plan's How to verify step by step: the smoke command uses {1..100} brace
expansion, but the holder pins default-shell /bin/sh, which is dash on Debian/Ubuntu lanes —
no brace expansion. Measured on this lane (F10, tmux 3.7b): the literal smoke renders
== PANEL: X == — one 14-char line — where the plan promises "TWO lines (80 + 40)". The
capture itself is faithful (it recorded exactly what rendered) and the step does not fail, but a
Linux reviewer following the plan sees the wrong shape and may distrust the tool. The portable
form $(seq 100) (used by my F1 cell) renders the wrap as documented. Related description nit:
the smoke line is 113 chars, so its wrap at 80 columns is 80 + 33, not 80 + 40.
Reproduce:
node dist/cli.js review capture-tui \
--command 'printf "%s\n" "== PANEL: $(printf "X%.0s" {1..100}) =="; sleep 20' \
--cols 80 --rows 10 --settle-ms 1200 --out /tmp/cap80
head -1 /tmp/cap80.ans # == PANEL: X == (dash lane)3. (Note, not a defect) No-tmux refusal wording on this lane
This lane has no tmux/freeze and its first PATH entry is a root-only directory, so libuv answers
EACCES (not ENOENT) for a missing binary. The refusal therefore reads
tmux could not be probed (EACCES) — … rather than tmux is not installed. This is the PR's probe
deliberately distinguishing "could not spawn" from "absent" — a defensive path working as designed
(assertions R1, R6, R7 all green). Reviewers on a host with a genuinely missing tmux will see the
plain "not installed" wording.
Targeted gates
packages/clireview suite with real tmux on PATH:npx vitest run src/commands/review src/commands/review.test.ts→ 68 files, 2526 passed, 0 failed, 4 skipped (the skips are the
documenteduid===0/platform guards). Includescapture-tui.test.ts(109 tests, 36.8 s of
real-tmux cells) andreview.test.ts(subcommand surface now pinscapture-tui).packages/coreglob.test.ts→ 60/60.- Vacuity check on the ride-along glob fix: reverting only the
fs.writeFileseed while keeping
the newexpect(...).toContain('Found 1 file(s)')makes the test fail with exactly
expected 'No files found…' to contain 'Found 1 file(s)'; reverting to the base shape makes it
pass vacuously. The new assertion is load-bearing. - Attributing the PR's claimed pre-existing failure: the body names one —
stale-bundle.test.ts
— as reproducing on pristinemain. On this lane that file passes (28/28 inside the suite
above), so there is no failure left to attribute to either side here; the claim neither helped nor
hurt this round.
Mutation matrix (guards the PR introduces)
Suite under mutation: tui-capture.test.ts + capture-tui.test.ts + cleanup.test.ts
(170 tests). Control unmutated: 170 passed. Witness 03-mutation-matrix.png.
| Mutant | Intended pin | Outcome |
|---|---|---|
M0 DEFAULT_COLS 80→81 (positive control) |
geometry default | 1 failed / 169 |
M1 capture argv gains -J |
no-wrap-join rule | 2 failed / 168 |
M2 kill loses -L scope |
isolation/reap scope | 5 failed / 165 |
M3 escapeTrailingSemicolon→identity |
;-splitting guard |
1 failed / 169 |
| M4 server prefix renamed | producer↔sweep prefix match | 3 failed / 167 |
M5 capture argv loses -N |
trailing-space fidelity | 4 failed / 166 |
Every introduced guard is pinned (no survivors); the positive control confirms the harness can make
the suite fail, and the target was restored byte-identical afterwards. Running the mutants under
real tmux made the stakes concrete: the M2 cell (kill without -L scope) turned nearly every
real-tmux capture into an orphan — the kill hit the default socket (nothing there), was read as
"nothing to kill", and the private socket was then unlinked with the server still alive —
precisely the state reap()'s own comment forbids. Census at peak during the matrix: 61 live
orphan servers and 182 holder-related processes, all under dead launcher pids; only one still had
a socket (an M4 mutant-prefixed one, which the sweep's orphan regex correctly ignores as
non-canonical). Attribution was then confirmed deterministically, not inferred: re-running only
the M2 mutant against capture-tui.test.ts leaked 58 live servers + 174 holders + 0 sockets in
a single suite run (logs/m2-attribution-check.log). This is proof the guards are load-bearing,
not a PR behavior: pristine code never produced any of these (the gate suite's 109 real-tmux tests,
the unmutated control cell, and ~15 harness captures left zero orphaned servers).
Not covered
--keyskey-sequence fidelity against a real multi-widget TUI (I verified tokens reach a shell
verbatim, incl. the dash-leading-lform; not a full widget walkthrough).- tmux version branches 3.1–3.3 (
-Npads / no-T) and the "too old (<3.1)" live path — only
3.7b was available; the version gate logic is covered by unit tests + my fake-shim refusal cells. freeze-rendered PNG content (no freeze on this lane): thepngrung, the identity-stamp
"did this run write it" credit check, and ENOSPC partial-write handling were exercised only via
theans-onlydegradation path.- The real GitHub
assetFiles→publish-assetsembed path (needs credentials this sandbox lacks). - Windows (declared N/A by the PR) and the bundled-skill/SKILL.md prose changes (documentation only).
Methodology
Environment: the lane's own node:22-bookworm container, Node v22.23.2, uid 1000, no root, and —
measured, not assumed — no tmux, no freeze, no zstd on the image. To reach the real-tmux branch
I built tmux 3.7b (the exact version the author tested) from the upstream release tarball into
/__w/_temp/local and prepended it to PATH for those legs; the refusal legs ran with it absent,
which is also the runtime this lane's own verify/tmux lanes execute. Two repro notes: the CI-provided
root bundle dist/cli.js predated the merge (zero capture-tui hits), so I re-ran npm run bundle
at HEAD before driving it; and the base control was a scratch git worktree at HEAD^1 with
packages/cli compiled standalone — its @qwen-code/qwen-code-core import resolves through the
root node_modules symlink to the head tree, verified acceptable because
git diff HEAD^1..HEAD -- packages/core/src shows no runtime core changes (only glob.test.ts
and the bundled SKILL.md). Harnesses are pure Node scripts that spawn the built CLI (no mocking of
the unit under test), assert on both stdout JSON and stderr, and encode expected control failures as
assertions; raw logs and the five evidence PNGs live under this directory (logs/, evidence/,
harness/). The PR's own cleanup sweep was verified against planted canonical orphans (storm and
demo cells: every dead-pid server reaped, the live-pid control spared, a non-canonical mutant-named
socket correctly ignored); against the mutation-produced field it had nothing discoverable to reap,
because the M2 failure mode unlinks the socket — the sweep's honest limit, named in Findings'
mutation paragraph.
Assertion enumeration (90): refusal-contract.mjs 22 · capture-ab.mjs 49 · holder-leak repro
L2-control pass + L1 fail (2) · glob vacuity G1–G3 (3) · mutation matrix: control + M0–M5 killed +
restore check (8) · gates: review suite + glob suite (2) · sweep cells: storm-reap, storm-spare,
demo-reap, non-canonical-ignored (4). Total 89 pass + 1 fail.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Local real-environment verification of
|
| 80 cols (wraps at column 80) | 120 cols (control, no wrap) |
|---|---|
 |
 |
Isolation — measured with live hostages
The safety property got the strongest test I could give it: during verification another agent session on this machine was actively using the default tmux server (creating/killing its own sessions). I additionally planted a canary session, then ran captures:
- During a capture:
/tmp/tmux-<uid>/showsdefault+qwen-review-capture-<pid>-<nonce>, and the private server holds only itscapsession. After: the private socket is gone (kill-server + socket unlink verified),defaultuntouched. - Canary + the concurrent session's real session both survived every capture (
isolation.txt). - SIGTERM mid-capture: exit 143, private server reaped immediately. One cosmetic note below.
- Orphan sweep: fabricated a dead-pid capture server (
qwen-review-capture-<dead-pid>-feedbeefholding a livesleep 300), ranreview cleanup local→Reaped orphaned capture server, socket removed, the user's default server untouched.
Refusal contract
All probed shapes refuse with exit 3 + machine JSON on stdout + human stderr, and no manifest: --cols 5, empty --command, blank-matching --until '\s', invalid regex --until '(', duplicated --command, tmux absent from PATH. The collision gate refused --out colliding with a pre-existing user file and left the file intact; re-running against the same --out after a successful capture cleared only the capture's own artifacts and succeeded.
Marker gating
--until MARKERsettles in ~1.5 s wall (settledBy: "until-match") instead of waiting out the budget.--readythat never matches: keys withheld (keysSent: false), late frame captured, degradation string names both facts. Honest manifests all the way down.--keys C-cagainst a runningsleep: the holder survives the INTR,^Cvisible in the captured pane, capture lands (exit 0).
The freeze root cause — reproduced, all three modes
The PR's measured claims about freeze + pipe stdin check out exactly on freeze v0.2.2:
| stdin shape | result |
|---|---|
| empty pipe, immediate EOF (spawnSync default) | ERROR No input, exit 1 |
| pipe with content | exit 0 — renders the pipe, silently ignoring the positional file (evidence substitution, the worst mode) |
| open pipe, no EOF | hangs indefinitely (confirmed >5 s, killed) |
stdin: 'ignore' structurally avoids all three. Bonus fidelity check: a capture full of ✓ … — └── ├── + CJK wide chars rendered to PNG in 1.5 s with no degradation — this machine's historical "freeze hangs on visual workflows" incidents are fully explained by the pipe mode, which this PR eliminates.
Real-product capture — the PR photographing the product with itself
Built CLI's onboarding dialog in an isolated QWEN_HOME, settled by --until 'Connect a Provider', both arms settledBy: "until-match", evidence: "png". At 80 columns the ModelStudio description wraps mid-phrase ("…Standard API / Key"); at 120 it holds — the claimed-width + control-width pair the verify brief prescribes, produced by the command under review:
| 80 columns (claimed width) | 120 columns (control width) |
|---|---|
 |
 |
Notes (non-blocking)
- Signal path leaves the
.holder-readysentinel behind (<out>.holder-readysurvived the SIGTERM run). The next run against the same--outclears it unconditionally before the ready gate, so it is litter, not a correctness hazard — but the code comment "removed on EVERY exit path" slightly overstates; the signal path is the exception. Not worth blocking on. - Suite totals differ from the description (2489 vs 2486 passed) — drift from
main, not from this PR.
Not verified here
Model-driven use of the new verify-brief block (prompt efficacy), Windows (N/A by design — the command refuses without tmux), Linux tmux versions (3.0a/3.2a version-gate logic is unit-pinned; not exercised live on this macOS host).
Verdict: LGTM from local verification. Every measured claim I probed reproduced as written; the isolation property held under concurrent real-world tmux use; refusals, degradations and manifests are honest under adversarial inputs.
中文版本（点击展开）
capture-tui 本地真实环境验证（PR head 66a7b3e）
环境: macOS (Darwin 25.6.0)、Node v24.18.1、tmux 3.7b、freeze v0.2.2。PR head 检出到隔离 worktree,npm ci && npm run build && npm run bundle,以下全部通过构建产物 dist/cli.js 驱动。证据图存于 commit-pinned 的 pr-assets/8894-verify @ c4dbde1,与 capture manifest 同目录。
测试套件
- 测试计划命令:2489 通过,1 失败 —— 唯一失败是
stale-bundle.test.ts。我没有直接采信 PR 的"既有失败"声称,而是实测:同一测试在 pristinemain(675dd4588)上以完全相同的形态失败(expected 6 lines, got 5)——与本 PR 无关。 - 三个 capture 相关测试文件显式运行(排除 commands/ 层级 glob 静默漏跑的可能):
capture-tui.test.ts+lib/tui-capture.test.ts+cleanup.test.ts= 170/170 通过(真实 tmux 3.7b)。
换行冒烟对照
113 字符长行,80 列:cap80.ans 持有两条物理行(80 + 33)——换行结构如终端所绘(无 -J 属性在产物上直接可见);120 列对照捕获单条 113 字符行。两份 manifest 均 "evidence": "png",exit 0。(图见上方英文版)
隔离性——带真实"人质"实测
验证期间本机另一个 agent 会话正在活跃使用默认 tmux server(建/杀自己的会话),我另外种了一个 canary 会话再跑捕获:
- 捕获期间
/tmp/tmux-<uid>/出现qwen-review-capture-<pid>-<nonce>私有 socket,私有 server 上只有cap会话;结束后私有 socket 连带清除(kill-server + socket unlink 均验证),default原样。 - Canary 与并发会话的真实会话在所有捕获中全程存活(
isolation.txt)。 - 捕获中途 SIGTERM:exit 143,私有 server 立即收割(一个外观小节见下)。
- 孤儿清扫:伪造死 pid 的捕获 server(内含存活的
sleep 300),review cleanup local→Reaped orphaned capture server,socket 移除,用户默认 server 未动。
拒绝契约
全部探测形态均 exit 3 + stdout 机读 JSON + stderr 人读说明,且不写 manifest:--cols 5、空 --command、匹配空白面板的 --until '\s'、非法正则、重复 --command、PATH 无 tmux。碰撞门拒绝与既有用户文件冲突的 --out 并保文件完好;成功捕获后复用同一 --out 只清自己的产物并成功。
Marker 门控
--until命中即定桩(约 1.5 秒,settledBy: "until-match"),不空耗预算。--ready永不匹配:按键扣留(keysSent: false),捕获迟到帧,降级字符串两个事实都写明。--keys C-c打向运行中的sleep:holder 扛住 INTR,捕获面板可见^C,捕获落地(exit 0)。
freeze 根因——三种形态全部复现
freeze v0.2.2 实测与 PR 声称逐条吻合:空管道立即 EOF → ERROR No input exit 1;管道带内容 → 渲染管道内容、静默无视位置参数文件(证据被偷换,最恶劣的形态);开放管道 → 无限挂起。stdin: 'ignore' 结构性规避全部三种。附加保真度检查:含 ✓ … — └── ├── 及中文宽字符的捕获 1.5 秒即渲染成 PNG 无降级——本机历史上"freeze 在视觉工作流上挂起"的悬案由管道形态完全解释,本 PR 将其消除。
真实产品捕获——PR 用自己给产品拍照
隔离 QWEN_HOME 驱动构建版 CLI 的 onboarding 对话框,--until 'Connect a Provider' 定桩,两侧均 settledBy: "until-match"、evidence: "png"。80 列下 ModelStudio 描述断行("…Standard API / Key"),120 列完整——正是 verify brief 规定的声称宽度 + 对照宽度成对。(图见上方英文版)
备注(不阻塞)
- 信号路径留下
.holder-ready哨兵文件(SIGTERM 那次运行后残留)。下次同--out运行会在 ready 门前无条件清除,属外观垃圾而非正确性风险——但代码注释"removed on EVERY exit path"略有夸张,信号路径是例外。不值得阻塞。 - 套件总数与描述有差(2489 vs 2486 通过)——来自
main漂移,非本 PR。
未验证范围
verify brief 新文案的模型实跑效果(prompt 效力)、Windows(设计上 N/A——无 tmux 明确拒绝)、Linux 各 tmux 版本(3.0a/3.2a 版本门逻辑有单测钉住;本 macOS 主机未实跑)。
结论:本地验证 LGTM。 我探测的每一条"measured"声称均如实复现;隔离性在并发真实 tmux 使用下成立;对抗性输入下拒绝、降级与 manifest 全部诚实。
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the full 14-file diff at 66a7b3e5796ab9a7eabe0d88ec2e70492adee0ce. Requesting changes for three independently reproduced safety failures: two paths can delete files that this capture does not own before a refused run, and kill-server leaves detached descendants alive despite the no-orphan contract. Details and repros are inline.
已在精确 commit 66a7b3e5796ab9a7eabe0d88ec2e70492adee0ce 完整审查 14 个文件。请求修改三项已独立复现的安全问题：两条路径会在命令拒绝前删除本次捕获并不拥有的用户文件，另有 detached 子进程在 kill-server 后仍存活，违反无孤儿进程契约。复现与建议见行内评论。
|
Round processed — one comment rather than 22 replies (per-thread replies tripped GitHub's secondary rate limit in an earlier round). Fixed in
Already fixed before this round (verified against HEAD, not just claimed):
Deferred to #9044 — R1-24, R1-13 (both threads), R1-14, R1-25, R1-26, R1-9, R1-12, R1-11, R1-28, R1-8, R1-20, R1-10. These are all "the pin does not cover what it claims", not defects in what the command does. Under the exit criterion on this PR, behaviour findings block and pin gaps become follow-ups — otherwise each round of new pins generates the next round of findings about those pins, which is what kept this PR open. Every one is listed in #9044 with its reproduction. Also in this branch since the last round, from the two verified Criticals:
113 tests pass. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): chunk 5: none — I completed every check I planned.; "This PR adds a new qwen review capture-tui subcommand that…": none — all planned checks completed within budget.; "This PR adds a new qwen review capture-tui subcommand that…": none — I completed all checks I set out to do. I did not run the entire core / review test suite, or capture-tui.test.ts / run.test.ts (which spawn real tmux/…; chunk 3: did not execute the real-tmux suite itself (review-only scope; CI covers it).; chunk 3: could not verify tmux 3.2a behavior locally for the -N / -T version-conditional tests (only tmux 3.4 installed here); those tests are gated by skipIf(tmuxPad….
Not reviewed: "This PR adds a new qwen review capture-tui subcommand that…" — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）：chunk 5：none — I completed every check I planned.；"This PR adds a new qwen review capture-tui subcommand that…"：none — all planned checks completed within budget.；"This PR adds a new qwen review capture-tui subcommand that…"：none — I completed all checks I set out to do. I did not run the entire core / review test suite, or capture-tui.test.ts / run.test.ts (which spawn real tmux/…；chunk 3：did not execute the real-tmux suite itself (review-only scope; CI covers it).；chunk 3：could not verify tmux 3.2a behavior locally for the -N / -T version-conditional tests (only tmux 3.4 installed here); those tests are gated by skipIf(tmuxPad…。
未审查："This PR adds a new qwen review capture-tui subcommand that…"——启动 prompt 为它指定了 diff 中的行，但它从未打开：有工具调用，却没有一次读取 diff。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
All six processed in Critical — detached descendants escape
Unguarded Render-window signal test without a kill guard. Correct, and the same class as R2-2 last round. It now attaches the exit listener before the wait (so a child dying during the render window settles the promise instead of hanging), guards with SIGTERM — thank you for the note that SIGKILL leaves the private server standing, since the child's own handler is what reaps it — and carries a 60s budget. This also covers the R1-25 item I had deferred to #9044.
117 tests pass. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": newline/trailing- ; client-split behavior not probed on tmux 3.1/3.2 (only 3.4 is installed here); the implementation's comment asserts cmd_parse_from_argumen…; "You are review agent reverse-audit — Reverse audit agent…": (none — all checks above completed within budget); "This PR adds qwen review capture-tui , a subcommand that…": none — all planned checks completed. I paged the diff's new-file regions (capture-tui.test.ts, capture-tui.ts, tui-capture.ts) only to their boundaries plus the…; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget., and 12 more.
Test Plan (not a blocker): 2486 passed — this review observed 19055, 19570, 1200, 1502, 481, 3069, 473 passed.
中文说明
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：newline/trailing- ; client-split behavior not probed on tmux 3.1/3.2 (only 3.4 is installed here); the implementation's comment asserts cmd_parse_from_argumen…；"You are review agent reverse-audit — Reverse audit agent…"：(none — all checks above completed within budget)；"This PR adds qwen review capture-tui , a subcommand that…"：none — all planned checks completed. I paged the diff's new-file regions (capture-tui.test.ts, capture-tui.ts, tui-capture.ts) only to their boundaries plus the…；"You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.，另有 12 条。
Test Plan（非阻断）：2486 passed — this review observed 19055, 19570, 1200, 1502, 481, 3069, 473 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
All twelve processed in R4-13 / R4-15 (Critical) — the capture window is an ownership hole. Both confirmed. They are one hole with two exploits: the collision gate runs before the window, and the window lasts up to One thing the fix had to get right that neither report mentioned: the refusal must not run its own cleanup on the occupant. Honest scope on the mutation evidence: dropping the mid-window check reddens both tests. Dropping R4-19 — R4-6 — ENOBUFS misattributed to the render belt. Right: both shapes kill with SIGKILL and set R4-14 — ENOENT from a broken interpreter. Confirmed; nothing in the spawn result separates "absent" from "present but unexecable". Rather than invent a discriminator, the refusal stops asserting what it cannot know: "tmux is not installed, or is installed but cannot be executed (a broken interpreter line answers the same ENOENT)". R4-20 — the sentinel's directory was unprobed. A direct consequence of moving the sentinel out of R4-1 ×3 — child tests with no external kill deadline. All three now match the FIFO sibling, and the ENOSPC/stdio one also drains its pipes (you were right that the unread pipe is a second route to the same hang). R4-5 / R4-17 — the orphan guards. Both correct, and R4-5 is a defect I introduced last round. The guards now send SIGTERM (R4-17: SIGKILL cannot run the child's reap, so a guard meant to prevent an orphan would create one) and set a R4-4 — the 121 tests pass. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks above were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget; the production regions beyond diff line 3687 (rest of capture-tui.ts) belong to another chunk, and I read only …; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; chunk 5: none (finished all checks within budget)., and 3 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): 2486 passed — this review observed 19059, 19570, 1200, 1502, 481, 3069, 473 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks above were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget; the production regions beyond diff line 3687 (rest of capture-tui.ts) belong to another chunk, and I read only …；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；chunk 5：none (finished all checks within budget).，另有 3 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：2486 passed — this review observed 19059, 19570, 1200, 1502, 481, 3069, 473 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
…honest until note Three of eighteen review findings were behaviour, not coverage. `process.exitCode` was per-process where everything else in a run is per run: `runCaptureTui` is exported and driven repeatedly in-process, so a refusal left exit 3 standing and the NEXT successful capture reported failure with its artifacts on disk. Probe-observed on the unmodified branch: refuse → 3, then a clean capture → still 3. It resets beside the completion flag now. `artifactsComplete` was armed one drain too late. The drain is the first event-loop turn after a long synchronous stretch, so an async stdio 'error' queued during that stretch — the reap WARNING is written there — dispatches inside the drain, found the flag false, and the broken-pipe guard rethrew: a completed capture exiting 1 with .ans and manifest both written. Armed before the drain now. When `--ready` times out the `--until` poll never runs, but the manifest recorded `until` and `settledBy: 'timeout'`, which reads as "searched and not found". Measured with the marker present in the pane for the entire run. It now says the search never happened. Also restored a test I deleted by accident: the stdio-failure pin went out in a round-18 bulk edit whose slice spanned it (present at 824d64e, gone at a1b5bed), which is why nothing caught the flag's arming point until this review. It is back, and rebuilt so it measures that point rather than the guard's mere existence — it queues the error on the reap warning, before the drain, and arming the flag after the drain turns it red.
Not more coverage — these six already passed while testing something other than what they claim, which is the failure mode that has hidden real defects in this work repeatedly. Two collision-gate tests carried a root skip left over from the write-probe design they replaced. The gate is occupancy — a pure lstat — so mode bits never enter it, and on a root lane (Docker images commonly are) both tests silently did not run. The FIFO pin returned green when `mkfifo` was absent: spawnSync does not throw for a missing binary, it hands back an ENOENT error object, so the bare `return` reported PASSED. It fails loudly now — the lane should say it cannot run this pin rather than drop it quietly. The stdin-shape fake asserted "some character device" where the property is /dev/null specifically: a pty is a character device and hangs freeze, a regular file sends it into file mode, and both satisfied the old check. It compares against a non-tty character device now, and piping stdin in production turns it red. The pre-capture stale-artifact test asserted exit 3 and cleared artifacts but never the reason, so deleting the marker-compile gate left it green — the run simply refused elsewhere. It names its gate now. And the shape-bounds family exercised only `--settle-ms` while its comment promised the timeout bound too; the missing case is there.
…ut of --out Two ways this command could destroy files it did not write, both probe-reproduced against the previous revision: The clear phase accepted any JSON at `<out>.json` whose `evidence` field read "png" or "ans-only" as proof that a previous run of this command wrote it, then deleted the .json, .ans and .png beside it. `evidence` is a field any report-shaped JSON can plausibly hold, so a user's own file authorized deleting their capture inputs — before the collision gate the run was already headed for could refuse. Ownership now requires the full signature a previous run actually writes: the evidence rung, an ansPath resolving to THIS run's .ans, and a settledBy from the closed set. Each rung is mutation-verified to redden the suite on its own. The holder's ready sentinel lived at `<out>.holder-ready`, inside the user's namespace and derived from their argument. It was unlinked unconditionally on every run; a directory at that name threw EISDIR and refused the capture outright. It now lives under the system temp dir, per-pid and random, so nothing at the old name is read, written or unlinked — the pre-unlink stays as a belt but is no longer load-bearing, and the three hazards its comment cited are gone at the source. Tests: the ordering pin whose forcing mechanism the move eliminated is retargeted at the invariant that replaced it (a user DIRECTORY at that name survives a refusal untouched), the fake tmux stubs recover the sentinel from the argv they are handed rather than guessing it from --out, and the assertions that checked `<out>.holder-ready` was absent — now vacuously true — assert instead that no sentinel leaks where it really lives. 112 pass; the one red file in this suite (lib/stale-bundle.test.ts) fails identically on origin/main.
…nd three test defects R2-3: an ans-only manifest — what a run that degraded its freeze rung writes — records no png of its own, so nothing at `<out>.png` is the next run's to clear. The `manifestHadPng` guard already said so; nothing tested it. Mutation-probed: dropping the condition shipped the whole suite green while a re-run against the same --out silently deleted a user's file at the png name. The new test seeds that exact shape and asserts the file survives byte-for-byte while the run degrades and says so; it reddens on the mutant. R2-7: the too-old-tmux refusal's defining property is that it refuses UP FRONT, before paying for a server start. Exit 3, the wording and the absent .ans are all location-invariant — a mutant moving the gate below plan.start stayed green while a real new-session ran the user's command first. The test now carries the same call-log shim the marker-gate test uses; against that mutant it reddens on the call-log assertion. R2-2: the signal test's 90s orphan guard was cleared immediately BEFORE `await disposition` — the one window it exists for. Against the dropped-re-raise mutant the test policies, the child never exits, vitest fails the test, and the already-cleared guard left a node process and its private tmux server running out a 60s capture on every red run. The guard now spans every wait, is cleared in a finally, and sits at 20s — below the test's own budget, or it can never fire. R2-8: that test was the only child-spawning test in the file without an explicit budget, running four sequential cold `node --import tsx` lifecycles under the default 15s. Measured at 18.4s under load; it now has 60s like its siblings. R1-5: two doc-comment blocks had drifted off their declarations — REAP_SIGNALS' rationale sat above MAX_MANIFEST_BYTES, and guardBrokenPipes' above `let artifactsComplete`. Reattached; no behaviour change.
…re ownership corners
The no-orphan wording was false for one class of command. Reproduced with
a discriminating control arm: a captured command running
`spawn('sleep', …, {detached: true}).unref()` left its grandchild alive
after a fully successful capture, while an attached child in the same
shape was reaped with the server. `kill-server` reaps the session, not a
process tree, and a daemonized descendant is in its own session with init
as its parent before the reap even runs — nothing portable reaches it,
and nothing links it back to the capture afterwards, so guessing at it
would kill the wrong pid. Rather than pretend otherwise, the four places
that claimed everything is reaped (the file header, the finally comment,
the plan's kill comment, the agent brief) now state the boundary and what
it means for the operator, and a two-arm test measures it in both
directions — the guarantee and the limit.
Four ownership/robustness corners, each probe-reproduced upstream and
each mutation-verified here except where noted:
- The signature compared `resolve(m.ansPath)`, so a foreign manifest with
a RELATIVE ansPath that resolved to this run's .ans passed and took all
three of the user's files. This tool always records the already-resolved
absolute path, so the relative form can only come from elsewhere: the
comparison is now strict.
- `manifestHadPng`'s `typeof m.pngPath === 'string'` disjunct was
unreachable for anything this writer produces (ans-only is paired with
`pngPath: null`) and, on an inconsistent manifest, cleared a foreign
`<out>.png` on an ans-only rung — against the guard's own invariant. The
evidence rung alone decides now.
- `statSync(pngPath)` was the only unguarded throwable fs call in the run;
a png vanishing mid-check escaped as an uncaught ENOENT (exit 1, no
contract JSON, a stack trace, both artifacts orphaned with no manifest).
Read defensively. The added test pins the degradation branch the guard
creates, NOT the TOCTOU itself — that needs fs fault injection, and a
faithful revert leaves the test green. Said so in the test.
- The render-window signal test spawned a 30s capture child with no kill
guard, so any assertion failing before its `child.kill()` orphaned the
node process and its private tmux server together. It now attaches the
exit listener before the wait, guards with SIGTERM (SIGKILL leaves the
server standing — the child's own handler is what reaps it), and carries
a 60s budget.
Also: two comments claimed `expect(value, message)` is banned by
vitest/valid-expect. It is not — the rule carves out a string message
argument, and this file uses the two-arg form elsewhere. Reworded both.
…maller defects Two ways a file could be destroyed or a write redirected AFTER the collision gate had already passed — the gate runs before the window, and the window lasts up to --timeout-ms (70 minutes at the cap): - A captured command that writes its own `<out>.json` had that file silently replaced, and the run reported success. Occupancy is now decided AGAIN at write time, against the same pre-window stamp. - A symlink planted at `<out>.ans` during the window redirected this run's bytes out of the --out base — the escape the lstat-based gate closes at check time, re-opened through the window. The artifact opens now carry O_NOFOLLOW. The refusal also had to stop short of its own cleanup: the occupant is precisely what the run refused to replace, so removing it would be the data loss the refusal exists to prevent. A distinct ArtifactCollision carries that decision to both catch blocks. Both tests redden when the mid-window check is dropped. O_NOFOLLOW alone does not: the check catches a planted symlink first, and the residual TOCTOU between check and open needs fault injection to reach — it is defence in depth, not something these tests prove. Smaller, each probe-reported and reproduced: - `tmux()` ran execFileSync with no stdio option, so Node teed every tmux child's stderr into ours through a raw, unguarded process.stderr.write — outside the broken-pipe guard and interleaved with contract output. Explicit stdio, like every sibling spawn. No dedicated pin: observing the tee needs a child driver with a closed stderr reader. - The freeze spawn took Node's silent 1 MiB maxBuffer, and an overrun kills with SIGKILL + ENOBUFS — recorded as "signal SIGKILL after the 30000ms render belt", a hang that never happened. The cap is explicit now and the attribution reads the error CODE. - execve answers ENOENT for a present-but-unexecable binary too (broken shebang), which the refusal asserted as "tmux is not installed". Nothing in the spawn result separates them, so the wording no longer claims the one it cannot know. - The ready sentinel's directory was the one path this run writes that no gate looked at — a consequence of moving it out of --out's namespace. An unusable TMPDIR burned the whole capture window waiting for a holder that could never signal ready. Probed up front, named in the refusal. - Three child-driven tests awaited exit with no external kill deadline (one also never drained its piped stdio, so a spewing regression blocks on a full pipe). They now match the FIFO sibling. - Both orphan guards killed with a signal that could not be told from the expected one, so a guard rescue passed green; the mid-poll guard also used SIGKILL, which cannot run the child's reap and would orphan the very server it guards. Both send SIGTERM and record that they fired. - The two --keys refusal gates ran against an empty dir, so their "no artifacts" assertions passed with or without a clear. Seeded, and they redden when the clear is skipped.
…more
The .ans/.json paths were re-checked after the capture window last round;
the png was not, and it is the one an external binary writes.
- A symlink planted at `<out>.png` during the window sent freeze's bytes
outside the --out base while this run attested `evidence: 'png'` for
them, because the pre-window stamp said the path was empty.
- When the render then failed, the torn-png cleanup asked that same stale
stamp and deleted whatever the captured command had put there.
One fix closes both: the png is re-stamped after the window, so every
question below it — render or degrade, credit or not, clean up or leave
alone — is decided against a closed window with only the render able to
have changed the path. Both new tests redden when the re-stamp is dropped.
Windows lanes: the TMPDIR test drove a variable `os.tmpdir()` never reads
there, and the EPIPE test pinned a path that does not exist there (a
broken pipe surfaces as UV_EOF/UV_EAGAIN, and the guard rethrows every
non-EPIPE code). Both gated, like their siblings.
Found by fixing a vacuous assertion rather than reported: the
leftover-socket test never asserted the capture succeeded, and under a
custom TMUX_TMPDIR it did not — a unix socket path is capped by
sockaddr_un (104 bytes on macOS), and this suite's mkdtemp base blew past
it. tmux answers `error connecting to … (File name too long)` AFTER the
start succeeds: a mid-capture refusal blaming tmux for a path this
command chose. Gated up front, measuring the base tmux will actually use
(the first USABLE one — measuring an unusable TMUX_TMPDIR refused runs
that were about to succeed under /tmp, which is how the first version of
this gate was caught being wrong).
Also:
- An empty `--cwd` was the one string argument whose empty form was not
refused: `resolve('')` is the launcher's cwd, so the enterability gate
always passed and the capture ran somewhere the caller never named,
with the manifest recording it as if asked for. Empty `--until`/`--ready`
refuse now too.
- The `--out` catch-all attributed fd exhaustion and a full disk to
`--out is not writable` — a machine-read reason telling an agent to fix
an argument that was fine. It names the host state instead.
- The freeze `absent` degradation asserted "not installed", the same
claim the tmux side was corrected for two commits ago.
- The kill-server retry repeated instantly, so it could not recover from
the client-spawn failure its own comment names; it pauses between
attempts and the WARNING separates "tmux said it failed" from "we could
not run tmux at all".
- The orphan sweep skipped a base silently when an ANCESTOR was
untraversable (existsSync swallows EACCES), past the catch written to
be loud about it; and its pid probe treated every non-EPERM throw as a
dead pid. Both pinned, both mutation-verified.
- Three tests were disarmed by last round's occupancy gate — two write-
failure tests could no longer reach the write, two protected-png tests
never spawned their fake freeze. Retargeted at what they now exercise,
with a marker proving the fixture that used to look load-bearing is not.
- One SIGKILL catch e9dbb43 missed, and a stale comment describing the
old sentinel design directly above the assertion pinning the opposite.
Two check-then-use races, both closed by moving the decision into the call the kernel makes atomically: - The clear phase verified `<out>.json` with lstat (regular file, size capped) and then re-resolved the path in readFileSync, so a swap between the two re-opened both classes those checks close: a FIFO that blocks the synchronous read with no refusal printed and no timeout able to interrupt it, and an arbitrarily large file that dies on the heap limit. One descriptor now answers both — opened O_NOFOLLOW|O_NONBLOCK, fstat'd, and read from the fd. - writeArtifact's occupancy check closed only the symlink half of its race with the open; a REGULAR file planted in that window was truncated by O_TRUNC and replaced, with the run reporting success. The path is always absent by then (cleared, or refused at the collision gate), so the open says exactly that: O_CREAT|O_EXCL, and EEXIST is a collision. Both are defence in depth against a racer, and I am not claiming test proof of the race itself: a faithful revert of either leaves the suite green, because a single-threaded test cannot occupy the window. The new FIFO test pins the branch, not the swap. Also fixed: - Every signal death leaked a sentinel: the re-raise terminates without unwinding, so the finally holding the only rmSync never ran. A harness reaping stuck captures produced one file per run in the system temp dir. - A RELATIVE TMPDIR resolved against the LAUNCHER's cwd for our probe and polling, but against the PANE's cwd inside the holder — the sentinel landed where nothing looked, and the precise early refusal became a dead ready-gate wait. The path is resolved once, up front. - The TMUX_TMPDIR usability check had no directoryness test, so a regular file there was measured as the socket base tmux would use and produced a machine-read refusal naming the wrong problem. - EDQUOT joined the host-state attribution instead of falling through to "--out is not writable". - isNothingToKill missed two tmux directory-level refusals (`directory … has unsafe permissions`, `… is not a directory`), each of which printed a false orphan WARNING naming a server that cannot exist. - The sweep treated ENOTDIR/ELOOP as scan failures, setting sweepFailed and suppressing "Nothing to clean" on a host where there was nothing to clean; only EACCES can be hiding an orphan. - `--timeout-ms`'s own description claimed it bounds a ready+keys capture; the settle runs after it, so wall time is timeout-ms + settle-ms. - Two doc blocks documented the declaration after the one they belong to. Tests: the no-orphan boundary test keyed pgrep/pkill to a bare `sleep 41`, which matches any concurrent run of this suite on a shared host — the finally then reached into other runs. Scoped to this pid, carried as a trailing `: <tag>` so sh cannot exec away the argv. The TMPDIR gate got the call-log and seeded-artifact pins its sibling gate families have (both mutation-verified). And three cleanup pins: the non-EPERM liveness arm, the post-kill unlink guard, and the EPERM fixture now built with the producer like every other one in that block.
…nd eight more Last round I widened `isNothingToKill` with two tmux wordings on the strength of "they also appear when no server was ever created". That is true and beside the point: both are refusals the CLIENT makes before it looks at anything, so they establish nothing about the server — and "nothing to kill" is what authorizes unlinking the socket. A live orphan behind such a socket read as reaped at both call sites: the sweep printed "Reaped", capture-tui's own reap went silent (no WARNING, exit 0), and both unlinked the socket of a running server, which makes it unreachable forever. They get their own state now: never reaped, always surfaced, the socket left alone, and the warning says which of the two things went wrong. The predicates were untested; they have a table now. The manifest size cap had the same shape as the round-6 TOCTOU it came from: fstat measured the file, then the read went to the LIVE end of the pinned inode, so an appender defeated the cap and reproduced the heap-limit death it was measured against. The read is bounded at cap+1 bytes, which also makes "it grew between the fstat and the read" observable rather than silent. And the residual identity half of the round-5 blocker: the signature authenticates the MANIFEST, and said nothing about the files beside it — a genuine previous manifest authorized unlinking whatever had since taken the .ans name. The manifest now records the identity of the artifacts it actually wrote, and the clear phase requires a match. A replaced artifact is not the one it describes, so the clear is skipped and the collision gate refuses: fail-closed. Also: tmuxPadsWithCaptureN answered true for 3.0.x, contradicting its own documented range and its sibling predicate (no shipped path reaches it — the version gate refuses those hosts first — but it is exported); and the four host-state attribution arms moved into an exported `hostStateFor` so each is pinned directly, since three of them need a fault injector to reach through the real syscalls (the R6-6 gap I had deferred). Tests: two wall bounds used Date.now() against this suite's own declared monotonic-clock rule; a throwing rmSync implementation leaked into every later test in cleanup.test.ts (clearAllMocks keeps implementations, so six later reap tests were silently exercising the failure branch); and the no-orphan boundary test tagged only the WRAPPER shell — sh forks an untagged sleep and waits, so its pkill killed the wrapper and left the sleep reparented to init. One orphan per run, from the test that exists to pin orphans. The tag rides in argv[0] now, via a per-run symlink to the real sleep. Neither the bounded read nor the O_EXCL/O_NOFOLLOW opens are proven by these tests: a faithful revert of any of them leaves the suite green, because a single-threaded test cannot occupy the window a racer occupies. The new tests pin the branches; the races are argued from the syscalls.
5b0e4b3 to
051637e
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the round cap of 5 without two consecutive dry rounds (round 5 reported findings; its verdicts landed in the cap-round tail).
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": none — nothing was cut short..
Test Plan (not a blocker): 2486 passed — this review observed 20350, 20038, 1460, 1570 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the round cap of 5 without two consecutive dry rounds (round 5 reported findings; its verdicts landed in the cap-round tail)。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 3)"：none — nothing was cut short.。
Test Plan（非阻断）：2486 passed — this review observed 20350, 20038, 1460, 1570 passed。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const r = spawnSync(freezeRender.bin, freezePlan(ansPath, pngPath), { | ||
| encoding: 'utf8', |
There was a problem hiding this comment.
[Critical] R9-1: Every artifact operation resolves the --out paths BY NAME against a filesystem tree the captured command (same uid) can mutate — the stamps, collision gates, O_NOFOLLOW and the post-window png re-stamp all argue only the FINAL path component and the capture window. This round's live-tmux reproduction opens the next entrance: swapping --out's PARENT directory for a symlink mid-window redirects all three artifacts out of the base while the manifest attests the original paths. Open Criticals R8-1 (freeze writes pngPath by name during the probe+render window after the re-stamp) and R8-5 (.ans never re-verified after the write — the manifest stamps whatever occupies the path at manifest time) are siblings of this surface and are superseded by this finding: entrance-by-entrance fixes cannot close a surface whose entrances (artifact path × occupant type × timing window × ancestor directory depth) do not enumerate. — Failure scenario: captured command runs cd .. && mv dir dir.stolen && ln -s /victim dir during the window (legal: same uid) → .ans/.json/.png are created under /victim, the run reports success, and the manifest attests <dir>/cap.* — evidence bytes leave the --out base while the manifest lies about where they live.
witness (probe, live tmux 3.3a):
exitCode: 0
{"captured":true,"evidence":"ans-only","manifest":".../capdir/cap.json"}
dir is symlink: true
victim contents: ["cap.ans","cap.json"]
victim/cap.ans bytes: "SWAP-MARK\n…"
manifest.ansPath attested ".../capdir/cap.ans"
Suggested fix: close the surface structurally — render .ans/.png to per-run uniquely-named files under resolve(tmpdir()) and rename() them onto the --out paths at credit time (rename(2) replaces a destination symlink itself rather than following it); re-verify .ans identity against the bytes writeArtifact wrote before the manifest stamps it; state any residual ancestor risk the design accepts in the header contract.
中文说明
〔Critical〕 R9-1：所有产物操作都按名称解析 --out 路径，而所在的文件系统树可被捕获命令（同 uid）改写——stamp、碰撞门、O_NOFOLLOW 与窗口后的 png 重 stamp 都只论证最后一个路径分量与捕获窗口本身。本轮的实机 tmux 复现打开了下一个入口：在窗口期间把 --out 的父目录换成符号链接，三个产物全部被重定向到 base 之外，而 manifest 仍为原路径作证。未决 Critical R8-1（freeze 在重 stamp 之后的 probe+render 窗口内按名称写 pngPath）与 R8-5（.ans 写入后从不复核——manifest 记录的是 manifest 时刻占用该路径的东西）都是这一表面的兄弟入口，被本发现取代：逐入口修补无法封闭一个入口不可枚举（产物路径 × 占用类型 × 时间窗口 × 祖先目录深度）的表面。— 失败场景：捕获命令在窗口内执行 cd .. && mv dir dir.stolen && ln -s /victim dir（同 uid，合法）→ .ans/.json/.png 被创建在 /victim 下，运行报告成功，manifest 却为 <dir>/cap.* 作证——证据字节离开 --out base，而 manifest 关于其位置说谎。
建议修复：结构性封闭——把 .ans/.png 渲染到 resolve(tmpdir()) 下每次运行唯一命名的文件，在授信时刻用 rename() 落到 --out 路径（rename(2) 替换目标符号链接本身而不是跟随它）；manifest 记录 .ans 之前，用 writeArtifact 实际写出的字节复核其身份；在头部契约中写明设计所接受的残余祖先风险。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| ); | ||
| }); | ||
|
|
||
| it('quote-escapes a user-derived readyFile in the holder script', () => { |
There was a problem hiding this comment.
[Suggestion] R9-2: The test title ("user-derived readyFile") and rationale describe a data flow this PR removed: readyFile is no longer built from --out — capture-tui.ts:422 builds it as join(resolve(tmpdir()), 'qwen-capture-ready-<pid>-<hex>'), and the production comment at lib/tui-capture.ts:298-301 was updated to match ("even now that the caller derives it from the system temp dir rather than from --out"). The test comment was not, leaving two contradicting rationales in the same PR. — Failure scenario: a maintainer auditing holder-script inputs reads the stale rationale, concludes esc() lost its reason when the --out link was severed, and drops it — a TMPDIR parent containing an apostrophe then breaks the holder quoting and burns the full sentinel deadline (the exact measured failure the comment cites).
| it('quote-escapes a user-derived readyFile in the holder script', () => { | |
| it('quote-escapes the readyFile path in the holder script', () => { |
(and restate the rationale: the path lives under resolve(tmpdir()), whose parents are not guaranteed apostrophe-free, and the holder shell re-parses the sentinel line)
中文说明
〔Suggestion〕 R9-2：测试标题（"user-derived readyFile"）与理由注释描述了本 PR 已经移除的数据流：readyFile 不再由 --out 构造——capture-tui.ts:422 将其构造为 join(resolve(tmpdir()), 'qwen-capture-ready-<pid>-<hex>')，且 lib/tui-capture.ts:298-301 的生产代码注释已同步更新（"即便调用方现在从系统临时目录而非 --out 派生……"）。测试注释未更新，同一 PR 中出现两份相互矛盾的理由。— 失败场景：维护者审查 holder 脚本输入时读到陈旧理由，认为 --out 关联已断、esc() 失去存在理由并将其删除——含撇号的 TMPDIR 父目录随后破坏 holder 引号，烧掉整个 sentinel 期限（正是注释引用的实测故障）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| } as never); | ||
| } | ||
|
|
||
| it('refuses an OVERSIZE <out>.json without reading it into memory', async () => { |
There was a problem hiding this comment.
[Suggestion] R9-3: The only two tests of the MAX_MANIFEST_BYTES heap guard (here and its twin at :2929) sit inside describe.skipIf(!hasTmux), but the behavior needs no tmux — the cap + bounded read run at capture-tui.ts:626-635 and the collision gate refuses at :769, before probes.tmux() first executes at :873. On any tmux-less lane (every Windows lane by definition) both tests are skipped and the guard has zero coverage. Distinct from open R8-10, which reports these same tests as vacuous WHEN RUN; the fixes are orthogonal. — Failure scenario: a future edit deleting the size cap or the bounded readSync ships green on every tmux-less lane; the first run against a reused --out where a large foreign file holds <out>.json then dies at the heap limit (FATAL ERROR: Reached heap limit) before any refusal can print — the exact measured death the cap exists to prevent.
witness (probe): tmux-less PATH lane → Tests 37 passed | 97 skipped (134) with both targets listed ↓ skipped; an un-gated probe form of the same fixtures passes there with tmuxCalls === 0.
Suggested fix: move/duplicate one oversize test into the probe-seam suite (it only needs withStdio + seeded files), like its sibling foreign-manifest tests that run on every platform.
中文说明
〔Suggestion〕 R9-3：MAX_MANIFEST_BYTES 堆守卫仅有的两个测试（此处与 :2929 的孪生测试）位于 describe.skipIf(!hasTmux) 内，但该行为不需要 tmux——上限与受限读取在 capture-tui.ts:626-635 执行，碰撞门在 :769 拒绝，均早于 probes.tmux() 在 :873 的首次执行。在任何无 tmux 的 lane（按定义即所有 Windows lane）上两个测试都被跳过，守卫零覆盖。与未决的 R8-10 不同（R8-10 报告这两个测试运行时是空钉）；两种修复正交。— 失败场景：未来删除大小上限或受限 readSync 的改动在所有无 tmux lane 上绿灯通过；随后首次复用 --out 且大型外部文件占用 <out>.json 的运行会在任何拒绝输出前死于堆上限（FATAL ERROR: Reached heap limit）——正是该上限要防止的实测死亡。
建议修复：把一个超量测试移入/复制到 probe-seam 套件（只需 withStdio + 预置文件），如同其兄弟外部 manifest 测试那样在每个平台运行。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| expect(manifest.cols).toBe(80); | ||
| expect(manifest.rows).toBe(24); |
There was a problem hiding this comment.
[Suggestion] R9-4: The only two captureTuiCommand.handler call sites in the repo (:3537, :3576) both pass cols: 80, rows: 24 — byte-equal to the yargs defaults. Every non-default-geometry behavior test drives runCaptureTui directly, bypassing the handler, so a mutant handler hardcoding DEFAULT_COLS/DEFAULT_ROWS ships green. — Failure scenario: a user running qwen review capture-tui --cols 120 gets a capture silently taken at 80 columns — wrong-width evidence for a feature whose purpose is width-specific layout claims.
witness (probe): mutant hardcoding defaults → Tests 2 passed | 132 skipped; flipping one test to 132×43 → AssertionError: expected 80 to be 132, and the run's own summary showed the capture executed at 80x24 while argv asked 132.
Suggested fix: in one handler-driven invocation pass non-default geometry (e.g. cols: 77, rows: 23 — both inside validGeometry) and assert manifest.cols/manifest.rows against those values.
中文说明
〔Suggestion〕 R9-4：仓库中仅有的两处 captureTuiCommand.handler 调用（:3537、:3576）都传 cols: 80, rows: 24——与 yargs 默认值逐字节相同。所有非默认几何的行为测试都直接驱动 runCaptureTui，绕过 handler，因此硬编码 DEFAULT_COLS/DEFAULT_ROWS 的 mutant handler 可以绿灯通过。— 失败场景：用户运行 qwen review capture-tui --cols 120 时被静默按 80 列捕获——对一个以"宽度相关的布局主张"为目的的功能，这是错误宽度的证据。
建议修复：在一处 handler 驱动的调用中传非默认几何（如 cols: 77, rows: 23——均在 validGeometry 范围内），并对这些值断言 manifest.cols/manifest.rows。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if (capturePads) { | ||
| degradations.push( |
There was a problem hiding this comment.
[Suggestion] R9-5: The padding-tmux degradation prefixes tmux onto tmuxVersion, but tmuxVersion is already the full tmux -V line ("tmux 3.2a") — the manifest records degradedBecause: "tmux tmux 3.2a pads capture-pane -N…". No test pins the string (the only pads reference in capture-tui.test.ts is the skipIf guard). — Failure scenario: every capture on a padding tmux (3.1–3.2.x — Ubuntu 22.04 ships 3.2a, the window this branch exists for) records the garbled string in the manifest, which verifiers quote when ruling on rung caveats.
witness (probe driving the real code path with probes.tmux answering 'tmux 3.2a'): degradedBecause: "tmux tmux 3.2a pads capture-pane -N to the grid allocation and has no -T — …"; dropping the prefix re-ran to "tmux 3.2a pads…".
| if (capturePads) { | |
| degradations.push( | |
| if (capturePads) { | |
| degradations.push( | |
| `${tmuxVersion} pads capture-pane -N to the grid allocation and ` + |
中文说明
〔Suggestion〕 R9-5：padding-tmux 降级文案在 tmuxVersion 前又拼了 tmux 前缀，但 tmuxVersion 本身就是完整的 tmux -V 行（"tmux 3.2a"）——manifest 记录 degradedBecause: "tmux tmux 3.2a pads capture-pane -N…"。没有测试钉住该字符串（capture-tui.test.ts 中唯一与 pads 相关的引用是 skipIf 门）。— 失败场景：在 padding tmux（3.1–3.2.x——Ubuntu 22.04 出厂 3.2a，正是该分支存在的窗口）上的每次捕获都会把这个错乱的字符串写进 manifest，而验证者裁决层级注意事项时会引用它。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| it('captures anyway on --until timeout and records the degraded settle', async () => { | ||
| await run({ until: 'NEVER-APPEARS', timeoutMs: 1500, settleMs: 0 }); |
There was a problem hiding this comment.
[Suggestion] R9-9: No test pins that the --until poll spends its budget: every until-match fixture's marker appears within ~100ms, and no until-timeout fixture asserts a wall-clock floor or references untilPolledMs/'never matched within', so a mutant curtailing the poll (bailing early while recording settledBy: 'timeout') ships the whole suite green. Distinct from open R1-12: untilPolledMs is computed BEFORE the poll as the granted remainder (capture-tui.ts:1340), so a truncated poll reports the identical number and R1-12's fix cannot detect curtailment. — Failure scenario: a curtailed poll captures a pre-marker frame and records '--until never matched within ms' — a false verdict an agent consumer deciding "the marker never appears" relies on.
witness (probe): truncated-poll mutant → full suite Test Files 1 passed (1) / Tests 134 passed (134).
Suggested fix: add a delayed-marker fixture (sleep 1.2; printf "LATE-MARK\n", until: 'LATE-MARK', timeoutMs: 3000, expecting settledBy 'until-match') and/or an elapsed floor on the until-timeout test, pinning the reported window by value.
中文说明
〔Suggestion〕 R9-9：没有测试钉住 --until 轮询真的花掉其预算：所有 until 匹配 fixture 的标记都在约 100ms 内出现，且没有 until 超时 fixture 断言墙钟下限或引用 untilPolledMs/'never matched within'，因此截断轮询（提前退出但仍记录 settledBy: 'timeout'）的 mutant 可以让整套测试绿灯通过。与未决的 R1-12 不同：untilPolledMs 在轮询之前按"授予的剩余量"计算（capture-tui.ts:1340），截断的轮询报告完全相同的数字，R1-12 的修复无法检测轮询截断。— 失败场景：被截断的轮询捕获到标记出现前的帧并记录 '--until never matched within <完整预算>ms'——一个代理消费者据此判定"标记从不出现"的错误裁决。
建议修复：增加延迟标记 fixture（sleep 1.2; printf "LATE-MARK\n"，until: 'LATE-MARK'，timeoutMs: 3000，期望 settledBy 'until-match'），及/或在 until 超时测试上增加耗时下限，并按值钉住报告的窗口。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| }), | ||
| ); | ||
| expect(process.exitCode).toBe(3); | ||
| expect(performance.now() - started).toBeLessThan(30_000); |
There was a problem hiding this comment.
[Suggestion] R9-10: This 30s hang bound is structurally dead: it runs only when the call already completed (adding nothing over the refusal assertions beside it); under the named regression — a blocking synchronous FIFO read — the event loop is blocked, the assertion never executes, and vitest's testTimeout (15s, packages/cli/vitest.config.ts; this test carries no larger per-test budget) kills the test before 30s anyway. The hazard class IS pinned, but by the child-spawned probe-seam FIFO test with its external 20s SIGKILL killer (~:1230), not by this assertion. — Failure scenario: a maintainer reading the bound believes the hang case has a dedicated contract assertion; the assertion passes exactly when it adds nothing and never fires when the hang recurs.
Suggested fix: either give the test an explicit budget above the bound (e.g. }, 45_000);) so the bound can discriminate, or delete the dead assertion and let the comment name the harness kill as the hang detector.
中文说明
〔Suggestion〕 R9-10：这个 30s 挂死上界是结构性死钉：它只在调用已完成时运行（相对旁边的拒绝断言毫无增益）；在其点名的回归——阻塞式同步 FIFO 读取——下事件循环被阻塞，该断言永远不会执行，而 vitest 的 testTimeout（15s，packages/cli/vitest.config.ts；本测试没有更大的单测预算）反正会在 30s 前杀掉测试。该危害类别有钉，但钉它的是带外部 20s SIGKILL 杀手的子进程 probe-seam FIFO 测试（~:1230），不是这条断言。— 失败场景：维护者读到该上界，以为挂死场景有专门的契约断言；这条断言恰好在其毫无作用时通过、在挂死重现时永不触发。
建议修复：要么给测试一个高于上界的显式预算（如 }, 45_000);）使上界可判别，要么删除死断言并让注释指明 harness kill 才是挂死探测者。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const nothingToKill = [ | ||
| 'no server running on /tmp/tmux-501/qwen-review-capture-1-a', |
There was a problem hiding this comment.
[Suggestion] R9-11: The bare 'no such file or directory' wording — carried by its own regex branch at tui-capture.ts:43 — has no dedicated test string: this array's one ENOENT entry satisfies the error connecting branch AND the bare branch simultaneously, so neither is individually pinned. Deleting the bare branch ships both suites green. Distinct from open R8-3 (branch 2 is dead code subsumed by the bare branch — the production-side fact); this is its converse test-side fact, verified independently. — Failure scenario: on a host whose tmux answers kill-server with the bare ENOENT wording (a documented shape per the function's own comment), isNothingToKill would return false → false 'could not reap' note + skipped unlink, reopening the false-orphan class the extra wordings were added to eliminate.
witness (probe): mutant (bare branch deleted) → Tests 8 passed (classification) and Tests 44 passed (cleanup); flip: isNothingToKill('no such file or directory') true under PR code, false under the mutant.
Suggested fix: add the bare form to the array, e.g. 'no such file or directory' — it matches only the bare branch, pinning it individually.
中文说明
〔Suggestion〕 R9-11：裸 'no such file or directory' 措辞——由 tui-capture.ts:43 自己的正则分支承载——没有专属测试字符串：数组中唯一的 ENOENT 条目同时满足 error connecting 分支与裸分支，因此两者都未被单独钉住。删除裸分支可让两个套件绿灯通过。与未决的 R8-3 不同（R8-3 是分支 2 为裸分支包含的死代码——生产侧事实）；本条是其反向的测试侧事实，已独立验证。— 失败场景：在 tmux 以裸 ENOENT 措辞回答 kill-server 的主机（函数注释自己点名的形状）上，isNothingToKill 将返回 false → 错误的 'could not reap' 注释 + 跳过 unlink，重新打开这些额外措辞本要消灭的假孤儿类别。
建议修复：向数组添加裸形式，如 'no such file or directory'——它只匹配裸分支，从而单独钉住该分支。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| return readdirSync(tmpdir()).filter((f) => | ||
| f.startsWith(`qwen-capture-ready-${process.pid}-`), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R9-12: leakedSentinels() filters by the vitest worker's process.pid, but the two signal-death tests (REAP_SIGNALS loop ~:3745, render-window test ~:3848) spawn CHILD processes whose sentinels carry the child's pid — so the rmSync(holderReadyPath) in onSignal (capture-tui.ts:1260, the sole signal-death cleanup; the finally never runs because the re-raise terminates without unwinding) is pinned by nothing. All seven leakedSentinels() call sites sit in in-process tests. — Failure scenario: a mutant deleting that rmSync leaks one sentinel per signal-killed capture and the suite ships green — the accumulation the production comment names ("a harness reaping stuck captures produces one per run").
witness (probe): mutant → Tests 1 passed | 133 skipped + 4 leaked /tmp/qwen-capture-ready-<childPid>-* files (one per REAP_SIGNALS child); restored → 0 new sentinels.
Suggested fix: in the REAP_SIGNALS loop, after disposition settles, assert readdirSync(tmpdir()).filter((f) => f.startsWith(qwen-capture-ready-${child.pid}-)) is [] (child.pid is already in scope); capture child.pid in the render-window test and do the same.
中文说明
〔Suggestion〕 R9-12：leakedSentinels() 按 vitest worker 的 process.pid 过滤，但两个信号死亡测试（REAP_SIGNALS 循环 ~:3745、render-window 测试 ~:3848）派生子进程，其 sentinel 携带子进程的 pid——因此 onSignal 中的 rmSync(holderReadyPath)（capture-tui.ts:1260，信号死亡路径上唯一的清理；由于重新抛出信号会不经 unwind 终止进程，finally 永不执行）没有任何钉。全部七处 leakedSentinels() 调用都在进程内测试中。— 失败场景：删除该 rmSync 的 mutant 会让每次被信号杀死的捕获泄漏一个 sentinel，而套件绿灯通过——正是生产注释点名的累积（"harness 反复回收卡住的捕获时每次运行产生一个"）。
建议修复：在 REAP_SIGNALS 循环中，处置落定后断言 readdirSync(tmpdir()).filter((f) => f.startsWith(qwen-capture-ready-${child.pid}-)) 为 []（child.pid 已在作用域内）；在 render-window 测试中捕获 child.pid 并做同样断言。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| expect(tmuxPadsWithCaptureN('tmux 3.2a')).toBe(true); | ||
| expect(tmuxPadsWithCaptureN('tmux 3.1')).toBe(true); |
There was a problem hiding this comment.
[Suggestion] R9-13: tmuxPadsWithCaptureN's TRUE branch (the dangerous side — it drops -N and triggers the degradation caveat) pins only 'tmux 3.2a' and bare 'tmux 3.1'; the lettered 3.1-line releases (3.1a/3.1b/3.1c — Debian 11 ships 3.1c) are unpinned, while the false branch bothers to pin letters (3.0a/3.0b/3.3a). The predicate's own history includes a lettered-minor regression (3.0.x once answered true). Distinct from open comment 3785684554 (the 3.0.x production bug, since fixed and pinned false). — Failure scenario: a regression mishandling 3.1 letters (e.g. matching 3.1$ only) ships green; on such a host -N stays on and fabricates phantom trailing spaces in the .ans while capturePads=false records no degradation caveat — a verdict then judges invented trailing-space evidence as real layout.
witness (probe): letter-exact mutant → Tests 29 passed (29) with tmuxPadsWithCaptureN('tmux 3.1c') → false, '3.1b' → false, '3.2' → false; pristine: all three → true.
| expect(tmuxPadsWithCaptureN('tmux 3.2a')).toBe(true); | |
| expect(tmuxPadsWithCaptureN('tmux 3.1')).toBe(true); | |
| expect(tmuxPadsWithCaptureN('tmux 3.2a')).toBe(true); | |
| for (const v of ['tmux 3.1', 'tmux 3.1a', 'tmux 3.1b', 'tmux 3.1c']) | |
| expect(tmuxPadsWithCaptureN(v)).toBe(true); |
中文说明
〔Suggestion〕 R9-13：tmuxPadsWithCaptureN 的 TRUE 分支（危险侧——它会丢弃 -N 并触发降级注意事项）只钉了 'tmux 3.2a' 与裸 'tmux 3.1'；带字母的 3.1 系列发布（3.1a/3.1b/3.1c——Debian 11 出厂 3.1c）未被钉住，而 false 分支却费心钉了字母（3.0a/3.0b/3.3a）。该谓词自身历史包含一次带字母小版本的回归（3.0.x 曾回答 true）。与未决评论 3785684554 不同（那是 3.0.x 生产缺陷，已修复并被钉为 false）。— 失败场景：错误处理 3.1 字母的回归（如只匹配 3.1$）可绿灯通过；在此类主机上 -N 保留、在 .ans 中捏造幻影尾随空格，而 capturePads=false 不记录任何降级注意事项——裁决随后把捏造的尾随空格证据当作真实布局。
— qwen3.8-max via Qwen Code /review (v0.21.12)
The reported reproduction is real: every artifact path resolves BY NAME, so a captured command running as the same uid could `mv dir dir.stolen && ln -s /victim dir` mid-window and send all three artifacts out of the --out base while the manifest went on attesting the original paths. The final component was guarded; the directory holding it was not. The suggested remedy — render elsewhere and rename onto the --out paths — does not close this one: rename(2) replaces a destination symlink rather than following it, which the O_NOFOLLOW/O_EXCL opens already handle, but its destination is still resolved by name, so a swapped ancestor takes the rename with it. What Node makes possible is identity: dev+ino of the directory, sampled up front and re-checked immediately before every artifact operation. That turns the reproduced attack — a swap anywhere in a capture window that runs up to 70 minutes — into a refusal, and leaves only the interval between the check and the syscall. Node exposes no *at() syscalls, so that residue cannot be closed from here; it is stated in the code rather than papered over. Two siblings the report names as superseded are closed with it: - The png was credited on `changed()`, which compares lstat identity — a symlink planted between the post-window re-stamp and freeze's write differs from "absent", so freeze wrote through it and the run credited the result. Only a regular file is a rendering this run produced. - The .ans identity now comes from the descriptor that wrote it (fstat before close) and is re-verified before the manifest describes it. The render window sits between those two points, and a manifest attesting bytes this run never produced is the wrong-evidence outcome the whole command exists to prevent. Also fixed: the padding-tmux degradation recorded "tmux tmux 3.2a" — tmuxVersion is already the `tmux -V` line, and every capture on a padding host (3.1-3.2.x, which is what Ubuntu 22.04 ships) carried it. Tests: the two MAX_MANIFEST_BYTES tests needed no tmux but sat behind skipIf(!hasTmux), so the heap guard was unpinned on every tmux-less lane — every Windows lane by definition. The foreign-manifest fixtures omitted the new artifacts rung, which masked whichever earlier rung a mutant restored; filled in with real identities, all four rungs redden individually (one needed a new fixture to isolate the evidence rung). Both handler call sites passed the yargs defaults byte-for-byte, so a handler hardcoding DEFAULT_COLS/DEFAULT_ROWS shipped green. leakedSentinels filtered by the vitest worker's pid while the signal-death tests spawn children, so the sentinel cleanup in onSignal — the sole cleanup on that path — was pinned by nothing. The FIFO test's wall bound could only run once the call had returned, which is exactly when the regression it names cannot happen; it carries a per-test budget instead. And the until-timeout test asserts a floor: everything else there is satisfied by a poll that bails on the first miss and records `settledBy: 'timeout'` anyway. Plus: the second reaped socket's unlink, the isSocketDirUnusable wiring (no fixture used either wording — including the regression I shipped last round), the bare 'no such file or directory' branch, the lettered 3.1 releases on the dangerous side of tmuxPadsWithCaptureN, and a test title describing a data flow this PR removed.
|
All 13 processed in R9-1 (Critical) — reproduced and closed as far as Node allows. The attack works exactly as witnessed, and I took the framing: entrance-by-entrance patching of the final path component cannot close a surface whose entrances include the ancestors. One correction to the suggested fix, because it matters for what actually got built: rendering to What Node makes available is identity. The directory holding the artifacts is stamped (dev+ino) up front and re-checked immediately before every artifact operation; a mismatch refuses. That converts the reproduction — a swap anywhere in a window that runs up to 70 minutes — into a refusal, and leaves only the interval between the check and the next syscall. Both superseded siblings are closed with it. R8-1: the png was credited on R9-5 is a real user-visible bug — every capture on a padding host (3.1–3.2.x, i.e. Ubuntu 22.04) recorded R9-3 is the one with the widest blast radius of the Suggestions: both heap-guard tests needed no tmux yet sat behind R9-8 was masking three of my own pins. With the Also fixed: 137 tests here, 46 in cleanup, 30 in tui-capture. On the force-push reminder from the bot: that was me rebasing to resolve the |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the round cap of 5 without two consecutive dry rounds (round 5 reported findings; its verdicts landed in the cap-round tail).
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": none — full chunk read (5 pages, no truncation left un-paged), no check cut short..
Test Plan (not a blocker): 2486 passed — this review observed 20038, 1460, 1570, 494, 3609, 498 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the round cap of 5 without two consecutive dry rounds (round 5 reported findings; its verdicts landed in the cap-round tail)。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：none — full chunk read (5 pages, no truncation left un-paged), no check cut short.。
Test Plan（非阻断）：2486 passed — this review observed 20038, 1460, 1570, 494, 3609, 498 passed。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // occupant is someone else's, and removing it here would be the | ||
| // data loss the refusal exists to prevent. The other two are still | ||
| // this run's to clean up. | ||
| if (e instanceof ArtifactCollision && path === manifestPath) continue; |
There was a problem hiding this comment.
[Critical] The manifest-write-failure cleanup assumes an ArtifactCollision can only name manifestPath as foreign — the carve-out skips only manifestPath — but the same catch also receives collisions naming ansPath (from the sameFile(ansWritten, ansPath) throw at ~1755) and collisions from assertSameOutDir(). In both shapes the loop evaluates changed(ansPath/pngPath, stamp) against the pre-window stamps, which answer true for ANY mid-window occupant, and rmSync deletes a file this run never wrote — contradicting the invariant this very comment states ("the occupant is someone else's"). — Failure scenario: a capture's .ans write succeeds; during the render window (freeze probe + render, seconds or more) a concurrent same-uid actor replaces <out>.ans. sameFile(ansWritten, ansPath) throws ArtifactCollision and the run refuses — but cleanup then rmSynces the actor's file, because the stamp says absent and the path is now occupied. The run's own bytes are already gone from that path, so the unlink is never restorative — it only destroys a foreign file. The directory-swap shape reaches the same loop via assertSameOutDir() and additionally unlinks foreign occupants through the swapped directory.
Witness (probe, real tmux; a watcher replaced cap.ans 1s into the render window):
PR as-is: exitCode 3 — "cannot write capture manifest: …cap.ans was replaced after this capture wrote it…" — ansExists: false (foreign file DELETED)
sameFile-gated cleanup: identical refusal — ansExists: true, ansContent "FOREIGN-CONTENT" (foreign file survives); DIR-BLOCK2 test still passes
Suggested fix: on ArtifactCollision, only remove a path whose current identity still proves it is this run's file — gate the .ans removal on sameFile(ansWritten, ansPath) and the png on the manifest's recorded artifact identity; keep skipping manifestPath. changed(path, stamp) cannot discriminate "we wrote it" from "someone replaced it after we did".
中文说明
〔Critical〕 manifest 写入失败的清理逻辑假定 ArtifactCollision 只可能指向 manifestPath 为外部文件——豁免分支只跳过 manifestPath——但同一个 catch 也会收到指向 ansPath 的碰撞（来自 ~1755 处 sameFile(ansWritten, ansPath) 的抛出）以及来自 assertSameOutDir() 的碰撞。这两种形态下，循环会用窗口前的 stamp 去评估 changed(ansPath/pngPath, stamp)，对任何窗口期间的占用者都返回 true，rmSync 会删除本次运行从未写过的文件——与本注释自己声明的不变量（"占用者属于别人"）直接矛盾。— 失败场景：捕获的 .ans 写入成功；渲染窗口期间（freeze 探针 + 渲染，可达数秒以上），同 uid 并发行为者替换了 <out>.ans。sameFile(ansWritten, ansPath) 抛出 ArtifactCollision、运行拒绝——但清理随即 rmSync 掉行为者的文件（stamp 记录为不存在、路径现被占用）。本次运行的字节早已不在该路径上，这次 unlink 毫无恢复意义——只摧毁外部文件。目录替换形态经 assertSameOutDir() 进入同一循环，还会透过被换掉的目录删除外部占用者。
证据（探针，真实 tmux；观察者在渲染窗口开始 1 秒时替换 cap.ans）：现行代码 exitCode 3、ansExists: false（外部文件被删）；加上 sameFile 门控后拒绝相同、ansExists: true 且内容为 FOREIGN-CONTENT（外部文件存活），DIR-BLOCK2 测试在修复下仍通过。
建议修复：遇到 ArtifactCollision 时，仅删除当前身份仍能证明属于本次运行的路径——.ans 的删除以 sameFile(ansWritten, ansPath) 为门、png 以 manifest 记录的产物身份为门；继续跳过 manifestPath。changed(path, stamp) 无法区分"我们写的"与"我们写完之后被人替换的"。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| return 'gone'; | ||
| } | ||
| }; | ||
| const outDirId = dirIdOf(); |
There was a problem hiding this comment.
[Critical] outDirId = dirIdOf() samples the identity of dirname(outBase) BEFORE the mkdirSync(dirname(outBase), { recursive: true }) at ~790 that exists to create that directory. When --out's parent does not yet exist, dirIdOf() catches the ENOENT and baselines 'gone'; the mkdir then creates the directory; and assertSameOutDir() inside writeArtifact compares the live dev:ino against 'gone' and throws a false collision after the entire capture window. Grep: outDirId is written only here and read only at ~439 — no re-baseline after the mkdir. All 47 test fixtures use an existing mkdtemp dir, so this ships green. — Failure scenario: capture-tui --out fresh-dir/nested/cap --command … where fresh-dir does not exist — the exact shape mkdirSync recursive exists for, and reachable via the briefed --out template whenever the plan directory has not been created yet. Every gate passes, the full capture window runs (up to the timeout-ms cap), and only then the first writeArtifact refuses with "the directory holding --out was replaced during the capture" — factually false (this run created the directory) — and the pane text is lost.
Witness (probe):
BASE: exit 3 — "the directory holding --out was replaced during the capture: …/fresh-dir/nested is no longer the one this run started with", ans/json absent
re-sample after mkdir: captured:true, evidence "ans-only", ans+json present
Suggested fix:
| const outDirId = dirIdOf(); | |
| let outDirId = dirIdOf(); |
(and reassign outDirId = dirIdOf(); immediately after the mkdirSync at ~790 — a directory this run created is definitionally this run's). The security property is unchanged: the capture window opens only after the mkdir + write probe.
中文说明
〔Critical〕 outDirId = dirIdOf() 在 ~790 处那个专门用于创建该目录的 mkdirSync(dirname(outBase), { recursive: true }) 之前采样 dirname(outBase) 的身份。当 --out 的父目录尚不存在时，dirIdOf() 捕获 ENOENT 并以 'gone' 为基线；mkdir 随后创建目录；writeArtifact 内的 assertSameOutDir() 把实时的 dev:ino 与 'gone' 比较，在整个捕获窗口结束之后抛出虚假碰撞。Grep：outDirId 只在此处写入、只在 ~439 读取——mkdir 之后没有重新采样。全部 47 个测试夹具都使用已存在的 mkdtemp 目录，因此绿着上线。— 失败场景：--out fresh-dir/nested/cap 且 fresh-dir 不存在——正是 mkdirSync recursive 存在的意义，也是 brief 模板在计划目录尚未创建时的形态。所有门通过、完整捕获窗口跑完（最长到 timeout-ms 上限），第一个 writeArtifact 才以"the directory holding --out was replaced during the capture"拒绝——与事实相反（目录是本次运行创建的）——面板文本丢失。
证据（探针）：BASE exit 3、ans/json 缺失；mkdir 后重采样 → captured:true、ans+json 落盘。
建议修复：改为 let outDirId = dirIdOf(); 并在 ~790 的 mkdirSync 之后立即重新赋值（本次运行创建的目录按定义属于本次运行）。安全性质不变：捕获窗口只在 mkdir + 写探针之后开启。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // either shipped green. | ||
| 'no such file or directory', | ||
| "can't create directory /tmp/tmux-501: Permission denied", | ||
| "couldn't create directory /tmp/tmux-501 (Permission denied)", |
There was a problem hiding this comment.
[Suggestion] The create directory branch of isNothingToKill (tui-capture.ts:37) alternates (couldn't|could not|can't|cannot), but the fixtures pin only can't and couldn't; deleting either untested alternate ships green, against this file's own stated mutation-coverage standard. — Failure scenario: a future edit narrows the regex to the two wordings the tests witness; CI green. On a host where tmux reports cannot create directory /tmp/tmux-501 (Read-only file system) (both untested wordings are live branches of the shipped regex), isNothingToKill returns false, so the reap/sweep reads a server that never existed as a live orphan and prints the false-orphan WARNING — the regression this branch was added to fix.
Witness (probe): pristine seam {"cannot":true,"couldNot":true} — both untested wordings match the shipped regex; mutant narrowed to (couldn't|can't) → full suite 30 passed (ships green).
Suggested fix: add 'cannot create directory /tmp/tmux-501 (Read-only file system)' and 'could not create directory /tmp/tmux-501' to the nothingToKill fixture list above.
中文说明
〔Suggestion〕 isNothingToKill 的 create directory 分支（tui-capture.ts:37）有四个备选措辞 (couldn't|could not|can't|cannot)，但夹具只钉住 can't 与 couldn't；删除任一未测措辞都能绿着上线，违背本文件自己声明的变异覆盖标准。— 失败场景：未来编辑把正则收窄到测试见证的两种措辞，CI 全绿；在 tmux 报告 cannot create directory /tmp/tmux-501 (Read-only file system) 的宿主上（两个未测措辞都是现行正则的活跃分支），isNothingToKill 返回 false，收尾/清扫把从未存在的服务器读作存活孤儿并打印虚假孤儿 WARNING——正是该分支要修的回归。
证据（探针）：未测措辞 cannot/could not 均匹配现行正则；收窄变异体下整套 30 测试通过（绿着上线）。
建议修复：在上方 nothingToKill 夹具列表中补入 'cannot create directory /tmp/tmux-501 (Read-only file system)' 与 'could not create directory /tmp/tmux-501'。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| function reapOrphanedCaptureServers(): { reaped: boolean; failed: boolean } { | ||
| const uid = process.getuid?.(); | ||
| // tmux is POSIX-only, and so is the socket dir layout below. | ||
| if (uid === undefined) return { reaped: false, failed: false }; |
There was a problem hiding this comment.
[Suggestion] The win32 early-return arm of reapOrphanedCaptureServers returns { reaped: false, failed: false }, and no test on any lane asserts that contract: the dedicated describe is skipIf(process.platform === 'win32') — win32 being the only platform where process.getuid is undefined — and no test stubs getuid for this file. (The arm IS executed on win32 lanes via the non-skipped runCleanup tests; what is missing is the assertion, not the execution.) — Failure scenario: a mutant returning { reaped: false, failed: true } ships green on every lane and, on every Windows cleanup run with nothing to remove, permanently suppresses the Nothing to clean for target "<t>". line scripts read from stdout (the wiring at ~681-692 maps sweep.failed → sweepFailed and gates that line on !sweepFailed); the symmetric { reaped: true } mutant suppresses it via removedAny.
Witness: trace at this head — no getuid stub exists for cleanup.ts; the whole orphan describe is skipIf(win32); the wiring confirms both mutants suppress the line.
Suggested fix: add one win32-runnable test that temporarily stubs process.getuid to undefined (restoring in finally, as the EPERM/EINVAL tests do with process.kill) and pins the contract: the sweep contributes nothing, the lease still clears, and Nothing to clean still prints.
中文说明
〔Suggestion〕 reapOrphanedCaptureServers 的 win32 早退分支返回 { reaped: false, failed: false }，但没有任何 lane 上的测试断言该契约：专属 describe 是 skipIf(process.platform === 'win32')——而 win32 恰是唯一 process.getuid 为 undefined 的平台——也没有测试为本文件 stub getuid。（该分支在 win32 lane 上会经未跳过的 runCleanup 测试执行；缺的是断言，不是执行。）— 失败场景：返回 { reaped: false, failed: true } 的变异体在每个 lane 都绿着上线，并在每次"无可清理"的 Windows cleanup 中永久吞掉脚本赖以读取的 Nothing to clean for target "<t>". 行（~681-692 的接线把 sweep.failed → sweepFailed 并以 !sweepFailed 门控该行）；对称的 { reaped: true } 变异体经 removedAny 达到同样效果。
证据：当前 head 静态追踪——cleanup.ts 无 getuid stub；整个孤儿 describe 为 skipIf(win32)；接线确认两种变异体都会吞掉该行。
建议修复：新增一个可在 win32 运行的测试，临时把 process.getuid stub 为 undefined（在 finally 中恢复，参照 EPERM/EINVAL 测试对 process.kill 的做法），钉住契约：清扫无贡献、lease 仍被清除、Nothing to clean 仍打印。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| } as never); | ||
| } | ||
|
|
||
| it('a GENUINE manifest does not authorize clearing a REPLACED artifact', async () => { |
There was a problem hiding this comment.
[Suggestion] The only test pinning the sameFile artifact-identity rung of the clear phase — a genuine manifest whose recorded identities deliberately mismatch the files on disk — sits inside describe.skipIf(!hasTmux) despite needing no tmux: it exercises only the clear/collision phase (capture-tui.ts:739-741), which runs before the tmux probe, with no shim, probe override, or spawn. Every other manifest fixture fills artifacts with real idOfPath identities by design, so this is the rung's only pin. — Failure scenario: on every tmux-less lane (every Windows lane by definition) the test is skipped; deleting both sameFile rungs ships green there, resurrecting the measured harm this PR fixes — a genuine previous manifest authorizing deletion of a user file that has since taken the .ans name (user data loss). The oversized-manifest siblings were moved out of this same describe for exactly this reason, per their own comment; this one was left in.
Witness (probe):
tmux-less PATH: this test status:"skipped" (suite 40 passed / 97 skipped)
sameFile-rung-deleted mutant, tmux-less: 40 passed / 97 skipped, failed:[] — ships green
sameFile-rung-deleted mutant, tmux present: this test FAILS (the mutant deleted the user file and captured successfully)
Suggested fix: move the test into the probe-seam describe above (it already uses only dir, withStdio, and staleManifest).
中文说明
〔Suggestion〕 钉住清理阶段 sameFile 产物身份档的唯一测试——真实 manifest 但其记录的身份与磁盘上的文件故意不匹配——位于 describe.skipIf(!hasTmux) 内，尽管它不需要 tmux：它只运行清理/碰撞阶段（capture-tui.ts:739-741），该阶段先于 tmux 探针执行，无 shim、无探针覆盖、无 spawn。其余所有 manifest 夹具都按设计填入真实 idOfPath 身份，因此这是该档唯一的钉。— 失败场景：在无 tmux 的 lane（按定义即所有 Windows lane）上该测试被跳过；删除两个 sameFile 档在那里绿着上线，复活本 PR 修复的实测危害——真实的上次 manifest 授权删除此后占用 .ans 名的用户文件（用户数据丢失）。超大 manifest 兄弟测试正是出于这个原因（按其自己的注释）被移出了同一 describe；这个被留下了。
证据（探针）：无 tmux PATH 下该测试 skipped（40 通过 / 97 跳过）；删档变异体在无 tmux lane 40 通过 / 0 失败（绿着上线），在有 tmux lane 该测试失败（变异体删除了用户文件并成功捕获）。
建议修复：把该测试移入上方 probe-seam describe（它只用到 dir、withStdio 与 staleManifest）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // (probe-reproduced: exit 0, success, wrong cwd). An empty --until is a | ||
| // pattern matching everything, settling on the first frame. | ||
| probes.tmux = () => ({ status: 'ok', out: 'tmux 3.9' }) as const; | ||
| for (const flag of ['cwd', 'until', 'ready'] as const) { |
There was a problem hiding this comment.
[Suggestion] The --out must not be empty gate (capture-tui.ts:398-403) has no effective pin: this seam family pins the empty forms of cwd/until/ready but omits out, and the sole empty---out test (~3549) sits inside describe.skipIf(!hasTmux) and asserts only process.exitCode === 3 — which still holds with the gate deleted, because resolve('') is the launcher's cwd and the directory-shaped---out gate refuses it with exit 3. — Failure scenario: delete the gate and every test stays green on every lane: (a) out: '' now refuses with the wrong machine-read reason ("must not name an existing directory: " instead of "must not be empty"); (b) out: ' ' no longer refuses at all — resolve(' ') is <cwd>/ , not an existing directory, so the capture runs to completion and writes <cwd>/ .ans/.json into the launcher's cwd with exit-0 success — the broken-template shape the gate's own comment says it exists to stop.
Witness (probe):
gate-deleted mutant: full suite 136 passed | 1 skipped — byte-identical to baseline
pristine, out:' ': EXIT=3 {"reason":"--out must not be empty."}, zero files
mutant, out:' ': EXIT=0 {"captured":true,"manifest":"/tmp/r1016-work/ .json"} — artifacts literally named ' .ans' / ' .json'
Suggested fix: add 'out' to this family (it needs no tmux — the gate fires before any probe), asserting the refusal message contains --out must not be empty.
中文说明
〔Suggestion〕 --out must not be empty 门（capture-tui.ts:398-403）没有有效钉：此 seam 族钉了 cwd/until/ready 的空形态却漏掉 out，而唯一的空 --out 测试（~3549）位于 describe.skipIf(!hasTmux) 内且只断言 process.exitCode === 3——删掉门后该断言依然成立，因为 resolve('') 就是启动者 cwd，会被目录形态的 --out 门以 exit 3 拒绝。— 失败场景：删除该门，所有 lane 全绿：(a) out: '' 以错误的机器可读理由拒绝（"must not name an existing directory: " 而非 "must not be empty"）；(b) out: ' ' 完全不再拒绝——resolve(' ') 是 <cwd>/ ，不是已存在目录，捕获跑完全程并以 exit 0 把 <cwd>/ .ans/.json 写进启动者 cwd——正是门注释声称要拦截的坏模板形态。
证据（探针）：删门变异体全套 136 通过 | 1 跳过（与基线逐字节一致）；原代码 out:' ' EXIT=3 且零文件；变异体 out:' ' EXIT=0、manifest 落在 /tmp/r1016-work/ .json，产物字面名为 ' .ans' / ' .json'。
建议修复：把 'out' 加入本族（无需 tmux——门在任何探针之前触发），断言拒绝信息包含 --out must not be empty。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // Unusable — tmux falls back to /tmp, and so does this measurement. | ||
| } | ||
| } | ||
| const socketPath = join( |
There was a problem hiding this comment.
[Suggestion] The socket-length gate measures a RELATIVE TMUX_TMPDIR verbatim (join(socketBase, …) without resolve), under-measuring the real socket path by the length of the cwd. The usability check resolves envBase against the cwd and gets the right verdict, but the length check measures the relative form. The file met this exact hazard one gate over: holderReadyPath uses resolve(tmpdir()) because os.tmpdir() hands back a relative TMPDIR verbatim (measured on Node 22). — Failure scenario: TMUX_TMPDIR=./scratch (relative) with a long cwd — the mkdtemp/CI scratch shape the gate's own comment names as "not an exotic shape at all". The gate measures e.g. 56 bytes and passes; tmux resolves against the client cwd to >108 bytes; plan.start pays for a server start and the first control call fails "File name too long" → mid-capture refusal blaming tmux for a path this command chose — the exact outcome the gate exists to prevent up front with an actionable message.
Witness (probe):
raw tmux oracle: relative TMUX_TMPDIR=./scratch resolves against the client cwd; at 126 absolute bytes tmux answers "error connecting to … (File name too long)"
PR E2E: {"reason":"tmux failed mid-capture: error connecting to … (File name too long)"}
fix (join(resolve(socketBase), …)): up-front refusal "the tmux socket path this capture would use is too long for a unix socket (126 bytes) … Point TMUX_TMPDIR at a shorter directory."
Suggested fix:
| const socketPath = join( | |
| const socketPath = join(resolve(socketBase), |
(the reap's unlink loop can stay as-is — it resolves consistently with the tmux client).
中文说明
〔Suggestion〕 socket 长度门对相对的 TMUX_TMPDIR 逐字测量（join(socketBase, …) 未 resolve），比真实 socket 路径少算了一个 cwd 的长度。可用性检查会把 envBase 对 cwd 解析、结论正确，但长度检查测的是相对形态。本文件在隔壁门里刚处理过同一危害：holderReadyPath 使用 resolve(tmpdir())，因为 os.tmpdir() 会原样返回相对 TMPDIR（Node 22 实测）。— 失败场景：TMUX_TMPDIR=./scratch（相对）+ 较长 cwd——门注释自己点名的 mkdtemp/CI 草稿目录形态（"not an exotic shape at all"）。门测得如 56 字节而放行；tmux 对客户端 cwd 解析后超过 108 字节；plan.start 付出一次服务器启动，第一个控制调用以 "File name too long" 失败 → 捕获中途拒绝，把本命令自己选的路径归咎于 tmux——正是该门存在的意义所要提前拦下的结果。
证据（探针）：裸 tmux 神谕——相对 TMUX_TMPDIR 对客户端 cwd 解析，126 绝对字节时回答 "error connecting to … (File name too long)"；PR 端到端复现为 mid-capture 拒绝；修复（join(resolve(socketBase), …)）翻转为事前拒绝并给出可操作的 TMUX_TMPDIR 指引。
建议修复：const socketPath = join(resolve(socketBase),（收尾的 unlink 循环无需改动——它与 tmux 客户端解析一致）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // print — measured, a 479MB dense JSON at <out>.json hit | ||
| // `FATAL ERROR: Reached heap limit` — and lstat already has the size, | ||
| // so the cap costs nothing. Too big to be ours: treat as unverified. | ||
| if (st.size > MAX_MANIFEST_BYTES) throw new Error('too large'); |
There was a problem hiding this comment.
[Suggestion] Writer and reader disagree about what a legitimate manifest can be: the manifest embeds --command, every --keys token, --ready and --until verbatim via JSON.stringify(manifest, null, 2) with no size bound, while the clear-phase reader caps at MAX_MANIFEST_BYTES (1 MiB). Several --keys tokens sum past 1 MiB within ARG_MAX (each token ≪ MAX_ARG_STRLEN; tmux itself rejects single send-keys tokens only ≥ ~50KB), and the exported runCaptureTui — which the tests drive directly — has no argv bound at all. — Failure scenario: a run succeeds and writes a 1.3 MB manifest. The next run against the same --out (the documented reuse shape): fstat > cap → 'too large' → shaped=false → nothing cleared → the occupancy gate refuses --out collides with a file this capture did not write — permanently, until manual deletion, for artifacts this very tool wrote and described. The refusal even claims the artifacts "are cleared automatically" while they are not.
Witness (probe, E2E through real tmux):
run 1, 130×10KB --keys tokens: {"captured":true}, manifest 1,301,564 bytes (cap 1,048,576)
run 2, same --out: {"reason":"--out collides with a file this capture did not write: …/cap.ans. A previous capture's own artifacts are cleared automatically; …"}
flip: signature-complete manifest padded to 1,229,145 bytes — refused identically under the 1 MiB cap; cleared+accepted when the cap is raised to 4 MiB. Size is the sole discriminator.
Suggested fix: bound the inputs the manifest records (refuse oversized --command/--keys at the gates), or size MAX_MANIFEST_BYTES above the largest manifest the writer can produce — either way, the writer's max ≤ the reader's cap.
中文说明
〔Suggestion〕 写者与读者对"合法 manifest 可以多大"认知不一致：manifest 经 JSON.stringify(manifest, null, 2) 逐字嵌入 --command、每个 --keys token、--ready 与 --until，无任何尺寸界限；而清理阶段的读者以 MAX_MANIFEST_BYTES（1 MiB）为上限。若干 --keys token 在 ARG_MAX 之内即可合计超过 1 MiB（单 token ≪ MAX_ARG_STRLEN；tmux 自身只在单 send-keys token ≥ ~50KB 时才拒绝），而测试直接驱动的导出函数 runCaptureTui 根本没有任何 argv 界限。— 失败场景：一次运行成功写出 1.3 MB manifest；对同一 --out 的下一次运行（文档声明的复用形态）：fstat 超上限 → 'too large' → shaped=false → 什么都不清 → 占用门拒绝 --out collides with a file this capture did not write——对本工具自己写出并描述的产物永久拒绝，直到手动删除。拒绝措辞甚至声称产物"会被自动清理"，而事实并非如此。
证据（探针，真实 tmux 端到端）：run 1（130×10KB keys）captured:true、manifest 1,301,564 字节（上限 1,048,576）；run 2 同一 --out 永久碰撞拒绝；翻转臂：签名完整的 1,229,145 字节 manifest 在 1 MiB 上限下被同样拒绝、上限提到 4 MiB 后被清理并接受——尺寸是唯一判别量。
建议修复：给 manifest 记录的输入设界（在门处拒绝超大 --command/--keys），或把 MAX_MANIFEST_BYTES 定在写者能产出的最大 manifest 之上——无论如何，写者上限 ≤ 读者上限。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // The bare wording, matched by its own regex branch: the entry above | ||
| // satisfies the `error connecting` branch AND this one, so deleting | ||
| // either shipped green. | ||
| 'no such file or directory', |
There was a problem hiding this comment.
[Suggestion] The only fixture pinning the bare /no such file or directory/i branch of isNothingToKill (tui-capture.ts:43) is all-lowercase, so dropping the /i flag from that branch ships green even though real strerror-derived wording is capital-N. The sibling file name too long branch IS pinned because its fixture carries capital F. — Failure scenario: mutant: remove /i from the bare branch. Every test passes — the bare fixture is lowercase, and the capital-N compound fixture is still caught by the error connecting branch, which keeps /i. But tmux's strerror text is No such file or directory (capital N, measured on this host), so a real bare ENOENT answer is no longer classified: serverDead stays false in reap/sweep, the run prints the false orphan WARNING and skips the socket unlink — the regression the multi-wording match was written to fix.
Witness (probe):
/i-drop mutant: tui-capture.test.ts 30 passed (ships green)
mutant: isNothingToKill('No such file or directory') -> false; lowercase -> true; compound -> true (error-connecting branch keeps /i)
restored: capital-N -> true
control mutant (/i-drop on the file-name-too-long branch): FAILS 1 test — confirming the asymmetry
real tmux 3.4 measured embedding 'No such file or directory' verbatim
Suggested fix:
| 'no such file or directory', | |
| 'no such file or directory', | |
| 'No such file or directory', |
中文说明
〔Suggestion〕 钉住 isNothingToKill 裸 /no such file or directory/i 分支（tui-capture.ts:43）的唯一夹具是全小写，因此从该分支删掉 /i 也能绿着上线——而真实 strerror 措辞是大写 N 开头。兄弟分支 file name too long 之所以钉得住，是因为其夹具带大写 F。— 失败场景：变异体删除裸分支的 /i，全部测试通过——裸夹具是小写，大写 N 的复合夹具仍被保留 /i 的 error connecting 分支接住。但 tmux 的 strerror 文本是 No such file or directory（大写 N，本机实测），真实裸 ENOENT 回答将不再被归类：reap/清扫中 serverDead 保持 false，运行打印虚假孤儿 WARNING 并跳过 socket unlink——正是多措辞匹配要修的回归。
证据（探针）：删 /i 变异体 30 测试全过（绿着上线）；变异体下大写 N → false、小写 → true、复合 → true；恢复后大写 N → true；对照变异体（对 file-name-too-long 分支删 /i）失败 1 测试，证实不对称；tmux 3.4 实测逐字嵌入 'No such file or directory'。
建议修复：在夹具中补入大写形态 'No such file or directory'。
— qwen3.8-max via Qwen Code /review (v0.21.12)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x) and Integration Tests (CLI, No Sandbox) were skipped in this PR's CI; the suites they run exercised the diff only on the Linux lane (green there for every file this PR touches).
Not reviewed: reverse audit — stopped at the round cap of 5 without two consecutive dry rounds (round 5 reported findings; their verdicts landed in the bounded tail verification).
Not explored to full depth (tool budget reached): "agent reverse-audit (round 5)": none — no check was cut short (probe scratch under /tmp/sweep-probe-* was removed after the experiment)..
Test Plan (not a blocker): 2486 passed — this review observed 20355, 20038, 1460, 1570, 494 passed.
[Critical] R10-1 (Critical, probe-verified): the post-window cleanup paths decide deletion with name-based changed()/occupied() — no directory-identity re-check, no ownership credit. Four probe-verified entrances: the manifest-catch carve-out skips only manifestPath so the sameFile(ansWritten, ansPath) collision deletes the replacer's .ans; the failed-render png cleanup deletes a victim .png through a swapped parent directory; the loop decides png ownership via changed() instead of the render credit (png === null), deleting a replaced pre-window occupant on a run that rendered nothing; and a directory-identity collision makes the loop delete /victim/cap.ans + /victim/cap.png through the swapped directory. Already filed as the existing inline thread at capture-tui.ts:1792 (comment 3790827573) on this head — not re-posted. ／ 窗口后清理按名称解析路径决定删除、无目录身份复查与归属授信，四个探针验证的入口；已在本 head capture-tui.ts:1792 的既有行内线程（comment 3790827573）中报告，不重复发布。
[Critical] R10-2 (Critical, probe-verified): outDirId is baselined as 'gone' when the --out parent does not exist; the mkdirSync(dirname(outBase), {recursive:true}) at :790 then creates it, so assertSameOutDir() in every writeArtifact refuses the first artifact write after the full capture window with a false 'directory was replaced' machine-read reason. Already filed as the existing inline thread at capture-tui.ts:435 (comment 3790827575) on this head — not re-posted. ／ --out 父目录不存在时 outDirId 基线为 'gone'，随后 mkdir 创建该目录，导致整个窗口后的首次产物写入被虚假的'目录被替换'理由拒绝；已在本 head capture-tui.ts:435 的既有行内线程（comment 3790827575）中报告，不重复发布。
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x) and Integration Tests (CLI, No Sandbox) were skipped in this PR's CI; the suites they run exercised the diff only on the Linux lane (green there for every file this PR touches)。
未审查：reverse audit — stopped at the round cap of 5 without two consecutive dry rounds (round 5 reported findings; their verdicts landed in the bounded tail verification)。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 5)"：none — no check was cut short (probe scratch under /tmp/sweep-probe-* was removed after the experiment).。
Test Plan（非阻断）：2486 passed — this review observed 20355, 20038, 1460, 1570, 494 passed。
[Critical] R10-1 (Critical, probe-verified): the post-window cleanup paths decide deletion with name-based changed()/occupied() — no directory-identity re-check, no ownership credit. Four probe-verified entrances: the manifest-catch carve-out skips only manifestPath so the sameFile(ansWritten, ansPath) collision deletes the replacer's .ans; the failed-render png cleanup deletes a victim .png through a swapped parent directory; the loop decides png ownership via changed() instead of the render credit (png === null), deleting a replaced pre-window occupant on a run that rendered nothing; and a directory-identity collision makes the loop delete /victim/cap.ans + /victim/cap.png through the swapped directory. Already filed as the existing inline thread at capture-tui.ts:1792 (comment 3790827573) on this head — not re-posted. ／ 窗口后清理按名称解析路径决定删除、无目录身份复查与归属授信，四个探针验证的入口；已在本 head capture-tui.ts:1792 的既有行内线程（comment 3790827573）中报告，不重复发布。
[Critical] R10-2 (Critical, probe-verified): outDirId is baselined as 'gone' when the --out parent does not exist; the mkdirSync(dirname(outBase), {recursive:true}) at :790 then creates it, so assertSameOutDir() in every writeArtifact refuses the first artifact write after the full capture window with a false 'directory was replaced' machine-read reason. Already filed as the existing inline thread at capture-tui.ts:435 (comment 3790827575) on this head — not re-posted. ／ --out 父目录不存在时 outDirId 基线为 'gone'，随后 mkdir 创建该目录，导致整个窗口后的首次产物写入被虚假的'目录被替换'理由拒绝；已在本 head capture-tui.ts:435 的既有行内线程（comment 3790827575）中报告，不重复发布。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| let captureTuiTs = join( | ||
| process.cwd(), | ||
| 'src/commands/review/capture-tui.ts', | ||
| ); | ||
| if (!existsSync(captureTuiTs)) { |
There was a problem hiding this comment.
[Suggestion] R10-3: This ~10-line block resolving capture-tui.ts (probe join(process.cwd(), 'src/commands/review/capture-tui.ts'), fallback candidate, no loud failure when neither exists) is copy-pasted verbatim six times in this file (lines 1121, 1497, 1575, 3083, 3845, 3974); five of the six sites carry no existence assertion after the block. — Concrete cost: if capture-tui.ts moves or the vitest cwd layout changes, six identical blocks must be edited in lockstep; when neither candidate exists the block silently passes the second non-existent path into the spawned tsx child and the test fails with an ERR_MODULE_NOT_FOUND stack from inside the child instead of an assertion naming the actual problem. The suite polices this failure class elsewhere (the mkfifo test's 'Fail loudly' comment), which makes the inconsistency concrete.
Suggested fix: extract one helper that probes both candidates and fails loudly (naming both probed paths) when neither exists, and call it from all six driver tests.
中文说明
〔Suggestion〕 R10-3：这个解析 capture-tui.ts 的约 10 行块（探测 join(process.cwd(), 'src/commands/review/capture-tui.ts')、回退候选、两者皆不存在时不响亮失败）在本文件中逐字复制粘贴了六次（1121、1497、1575、3083、3845、3974 行）；其中五处在块后没有存在性断言。— 具体代价：若 capture-tui.ts 移动或 vitest cwd 布局变化，六个相同的块必须同步修改；当两个候选都不存在时，块会静默地把第二个不存在的路径传给 spawn 的 tsx 子进程，测试以子进程内部的 ERR_MODULE_NOT_FOUND 堆栈失败，而不是一个点名真实问题的断言。套件在别处对这一故障类设了防（mkfifo 测试的 'Fail loudly' 注释），使此处不一致尤为明显。
建议修复：提取一个辅助函数探测两个候选、在两者皆不存在时响亮失败（点名两个被探测路径），六处驱动测试统一调用。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const o1 = captureServerName(717171, 'aaa'); | ||
| const o2 = captureServerName(727272, 'bbb'); |
There was a problem hiding this comment.
[Suggestion] R10-4: 'unlinks EVERY reaped socket' (pids 717171/727272, here) and 'never unlinks on a CLIENT-side refusal' (pid 838383, :415) probe hand-picked literal pids with the REAL process.kill, assuming they are dead — without stubbing it, unlike the EPERM/EINVAL tests (424242/515151). (The third literal-pid test, 626262 at :466, is left to the open thread at :467.) — Failure scenario: measured on this runner: pid_max = 4194304, 695 live processes with pids above 500000, highest live pid 4193927 — the literals are allocatable. When one belongs to an unrelated live process at probe time, the probe answers alive (success or cross-user EPERM — both treated as alive), the sweep skips the kill, and the positive assertions fail — a spurious red against healthy code. Direction is spurious-red only; the sweep behaves correctly toward a live pid.
Witness (A/B probe):
live-pid arm: expected "Reaped orphaned capture server: qwen-review-capture-2499240-aaa",
received "Nothing to clean for target \"local\"." — assertion failed
suite arm (literals currently dead): Tests 46 passed (46)
Suggested fix: derive dead pids the suite's own way — spawnSync(process.execPath, ['-e', '']) and use its pid after exit, as deadPid/deadPid2 are derived at the top of this describe — or stub process.kill to throw ESRCH for the target pid.
中文说明
〔Suggestion〕 R10-4：'unlinks EVERY reaped socket'（pid 717171/727272，此处）与 'never unlinks on a CLIENT-side refusal'（pid 838383，:415）用真实 process.kill 探测手写的字面 pid，假定其已死——且不像 EPERM/EINVAL 测试（424242/515151）那样打桩。— 失败场景：本 runner 实测 pid_max = 4194304、695 个 pid 大于 500000 的存活进程、最高存活 pid 4193927——这些字面值可被分配。若探测时恰属于某个无关存活进程，探测回答存活（成功或跨用户 EPERM——均视为存活），sweep 跳过 kill，正向断言失败——健康代码下的假红。方向仅为假红；sweep 对存活 pid 的行为是正确的。
建议修复：用套件自己的方式派生死 pid——spawnSync(process.execPath, ['-e', '']) 退出后取其 pid（与本 describe 顶部 deadPid/deadPid2 的派生方式一致），或为目\u6807 pid 桩 process.kill 抛 ESRCH。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| captureFailed = true; | ||
| refuse(`tmux failed mid-capture: ${detail}`); |
There was a problem hiding this comment.
[Suggestion] R10-5: The ready-gate timeout ('the pane never initialized — its holder wrote no ready marker', :1346-1348) is a pure existsSync polling loop after a successful plan.start; it is routed through this catch, which prefixes tmux failed mid-capture: even though no tmux invocation failed. The refusal reason is machine-read, and the catch's own comment lists tmux-side causes (ancient tmux, a command tmux refuses, a server that died) — a holder that never reached its sentinel write (broken /bin/sh — the plan pins default-shell /bin/sh; pane death; sentinel write failure after the pre-gate TMPDIR probe) is none of those. This is the misattribution class hostStateFor exists to avoid. — Failure scenario (probe, fake tmux answering every call but never writing the sentinel): exitCode=3; stdout JSON reason tmux failed mid-capture: the pane never initialized — its holder wrote no ready marker — an agent consumer goes to fix or restart tmux when the fault is the host shell, the pane, or the disk. Mitigation worth stating in the fix: the detail suffix already names the true cause, so the remedy is re-prefixing (or a distinct refusal type), not re-diagnosing.
Suggested fix: throw the ready-gate error as a distinct type (or with a marker the catch can test) and refuse it without the tmux prefix, keeping the prefix for errors actually thrown by tmux(...) calls.
中文说明
〔Suggestion〕 R10-5：ready 门超时（'the pane never initialized — its holder wrote no ready marker'，:1346-1348）是 plan.start 成功之后的纯 existsSync 轮询；它被路由到这个 catch，而该 catch 以 tmux failed mid-capture: 为前缀——尽管没有任何 tmux 调用失败。拒绝理由是机器可读的，catch 自己的注释列出的是 tmux 侧原因（过老的 tmux、tmux 拒绝的命令、死掉的 server）——holder 未能写下 sentinel（/bin/sh 损坏——plan 钉住 default-shell /bin/sh；pane 死亡；TMPDIR 预探测通过后 sentinel 写入失败）不属于其中任何一类。这正是 hostStateFor 存在所要避免的误归属类。— 失败场景（探针：假 tmux 应答一切调用但从不写 sentinel）：exitCode=3；stdout JSON 理由 tmux failed mid-capture: …——agent 消费者去修复/重启 tmux，而真实故障在宿主 shell、pane 或磁盘。
建议修复：把 ready 门错误以独立类型（或 catch 可判定的标记）抛出，拒绝时不带 tmux 前缀；前缀保留给真正由 tmux(...) 调用抛出的错误。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // CREATED, not just named: tmux only uses a base it can use, and an | ||
| // unusable one falls back to /tmp — where the path is short and the | ||
| // capture would have been fine. | ||
| const longBase = join(dir, 'x'.repeat(120)); |
There was a problem hiding this comment.
[Suggestion] R10-6: The socket-length gate's only pin exercises a ~198-200-byte gross overflow (measured with production's exact construction on this host), leaving the gate's defining constant 103 (capture-tui.ts:986; macOS sun_path is 104 bytes including the NUL) undiscriminated — any mutant bound in roughly [104, 197] still refuses this fixture and ships green — and there is no accept-side fixture at 103 bytes. grep confirms 'too long for a unix socket' is asserted at exactly one site in packages/. — Failure scenario: on macOS a TMUX_TMPDIR landing the real socket path at 104-107 bytes sails through the gate, the server starts, and the first control call dies 'File name too long' as a mid-capture refusal blaming tmux — the measured harm the gate's own comment exists to prevent. This file's pins elsewhere explicitly kill gate-moving and gate-deleting mutants; this one leaves the constant's whole wrong-value band alive.
Suggested fix: add boundary fixtures computed at runtime (the path embeds the variable-length pid): build the fixed suffix length via captureServerName(process.pid, 'deadbeef') + /tmux-${process.getuid()}/, choose longBase so the total socket path is exactly 103 bytes (assert NOT refused for the socket reason) and exactly 104 bytes (assert the socket refusal), instead of only the 120-char overflow.
中文说明
〔Suggestion〕 R10-6：socket 长度门的唯一钉只演练了约 198-200 字节的粗略超限（按生产代码的构造在本机实测），使门的定义常数 103（capture-tui.ts:986；macOS sun_path 含 NUL 共 104 字节）不被判别——约 [104, 197] 区间内的任何变异界限仍会拒绝此 fixture 并通过——且没有 103 字节的接受侧 fixture。grep 确认 packages/ 中仅一处断言 'too long for a unix socket'。— 失败场景：macOS 上使真实 socket 路径落在 104-107 字节的 TMUX_TMPDIR 会穿过该门，server 启动，第一个控制调用以 'File name too long' 死在捕获中途、归咎 tmux——正是门的注释所要防止的实测危害。
建议修复：增加运行时计算的边界 fixture（路径内嵌变长 pid）：用 captureServerName(process.pid, 'deadbeef') + /tmux-${process.getuid()}/ 推导固定后缀长度，选择 longBase 使 socket 路径总长恰好 103 字节（断言不因 socket 原因被拒）与恰好 104 字节（断言 socket 拒绝）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const renderStarted = join(dir, 'render-started'); | ||
| writeFileSync( | ||
| slowFreeze, | ||
| `#!/bin/sh\n: > "${renderStarted}"\n/bin/sleep 4\nprintf x > "$5"\n`, |
There was a problem hiding this comment.
[Suggestion] R10-7: The slow-freeze fixture hardcodes /bin/sleep 4; on hosts without /bin/sleep (NixOS store paths, minimal rootfs) the shim writes the render-started sentinel, then fails with command-not-found and sh continues immediately (no set -e) — the 4s render window this test depends on collapses to ~0ms. — Failure scenario: the parent kills on the sentinel, so SIGTERM lands on an already-exited child — expect(signal ?? 'code:' + code).toBe('SIGTERM') reds against healthy production code — or lands in the drain's await, and the test passes while no longer testing what it names: a queued signal swallowed into an exit-0 + success-JSON completion. Probe (exact script shape both ways): missing-sleep arm 0.00s elapsed, exit 0, png written; /bin/sleep arm 4.02s. Same portability class as the orphan test's /bin/sleep symlink (:3792), separate site and failure shape.
| `#!/bin/sh\n: > "${renderStarted}"\n/bin/sleep 4\nprintf x > "$5"\n`, | |
| `#!/bin/sh\n: > "${renderStarted}"\nsleep 4\nprintf x > "$5"\n`, |
(The child inherits the unmutated parent env in this test — a PATH-resolved sleep matches every other fixture in the file.)
中文说明
〔Suggestion〕 R10-7：slow-freeze fixture 硬编码 /bin/sleep 4；在没有 /bin/sleep 的主机（NixOS store 路径、精简 rootfs）上，shim 写下 render-started sentinel 后因 command-not-found 失败、sh 立即继续（无 set -e）——本测试依赖的 4s 渲染窗口坍缩为约 0ms。— 失败场景：父进程以 sentinel 为时机 kill，SIGTERM 落在已退出的子进程上——expect(...).toBe('SIGTERM') 在健康生产代码下假红——或落在 drain 的 await 上，测试通过却不再演练其点名的回归（排队的信号被吞进 exit-0 + 成功 JSON 的完成）。探针（同一脚本形状双向实测）：无 sleep 臂 0.00s elapsed, exit 0, png written；/bin/sleep 臂 4.02s。
建议修复（一键 suggestion）：改用 PATH 解析的 sleep——本测试的子进程继承未被改动的父环境，与文件内其他 fixture 一致。
— qwen3.8-max via Qwen Code /review (v0.21.12)
…rteen more Both Criticals this round are consequences of last round's fixes, and both came with a witness: - The directory-identity baseline was sampled BEFORE the `mkdirSync` that exists to create that directory, so an `--out` whose parent does not exist yet baselined 'gone' and the first write refused with "the directory holding --out was replaced" — after paying the whole capture window, and factually false: this run created it. Re-baselined after the mkdir; the capture window opens later, so the guard is unchanged. - The collision cleanup skipped only `manifestPath`, but that catch now also receives collisions naming the `.ans` (the identity re-check) and the directory (assertSameOutDir). In those shapes it asked `changed()` against the PRE-window stamps — true for any occupant — and deleted the file the refusal existed to protect, while this run's own bytes were already gone from that path. A path is removed only while its CURRENT identity still proves it is this run's. Both reproduce and both fixes redden their tests; the second is driven by a fake freeze that replaces the .ans from inside the render window, which is where the reported racer sat. Also fixed, all reported: - The socket-length gate measured a RELATIVE TMUX_TMPDIR verbatim, under-counting by the whole cwd — the same split-resolution hazard this file met one gate earlier with TMPDIR. - Writer and reader disagreed about a legitimate manifest: the writer embedded --command/--keys/--ready/--until with no bound while the reader caps at 1 MiB, so a run could write a manifest its own next run cannot verify. Measured from the arguments, before the window opens. - The ready-gate timeout is a pure existsSync poll after a start that SUCCEEDED, but it was routed through the catch that prefixes "tmux failed mid-capture" — a false causal claim in a machine-read reason. Tests: the identity-rung test needed no tmux yet sat behind skipIf(!hasTmux) (I swept the file — it was the only one left of that class); `--out must not be empty` had no effective pin, and it is the one gate that refuses WITHOUT clearing, so its assertions are the opposite of its siblings'; two of the four `create directory` alternates and the /i flag on the bare ENOENT branch were unpinned; the ~10-line capture-tui.ts resolver was copy-pasted six times, five of them failing silently when neither candidate existed; two cleanup tests probed hand-picked literal pids with the REAL process.kill; the win32 arm of the sweep had its contract asserted on no lane at all; the socket-length fixture overflowed by ~100 bytes so any mutant bound in [104, 197] passed it; and the slow-freeze shim hardcoded /bin/sleep, which on a host without it collapsed the 4s render window to nothing without failing.
|
Both rounds processed in Both Criticals are mine, from last round, and both witnesses reproduce. The The collision cleanup: you are right that the carve-out stopped being sufficient the moment I added two new throw sites. Worth stating plainly, since it is the second round in a row: these two are not incidental. Both come from adding a mechanism (a new throw, a new baseline) without re-asking what that mechanism newly enables — the same failure mode as the Production fixes: the socket-length gate measured a relative On the The rest: 139 tests here, 47 in cleanup, 32 in tui-capture. Separately: the round before this one never posted — its |
…LM#9255) * fix(ci): keep a fallback comment when the PR review runner dies A review job that dies abnormally never reaches its in-job fallback comment step: the runner worker crash in FinalizeJob on the PR QwenLM#8894 run (EACCES creating under the runner home directory) left the PR with no review and no explanation. - Probe write access to $HOME, $RUNNER_TEMP and the runner root at job start, repair single-directory ownership with the existing sudo pattern, and fail fast with a clear message when repair is impossible instead of burning the review budget to die at finalize. - Add a fallback-comment job on an ephemeral hosted runner that posts the retry guidance whenever review-pr fails. It derives the PR number from the event payload (dead job outputs do not survive a crash) and dedupes on a qwen-review-fallback comment marker plus this run's URL, so the in-job step, the ack comment, and re-runs never double-post. * fix(ci): harden the PR review fallback comment (QwenLM#9255) Review-round fixes for the fallback-comment defenses: - Probe the actual runner root (three levels above the workspace, not two) and the _diag subdirectory FinalizeJob writes in; a writable parent does not prove an existing subdirectory writable. - Open the fallback gate on authorize/review-config failures too — the incident's trigger can kill those earlier self-hosted jobs first, and a failed dependency marks review-pr 'skipped', which the old gate never matched. Guarded against resolve dispatch runs, which skip review-pr by design. - Author-scope the dedup lookup (resolved dynamically like upsert-bot-comment.sh) so a planted marker cannot suppress the fallback, and fail closed with bounded retry when the lookup or the state check fails instead of fail-open toward duplicates or a green job that never posted. - Skip the stale fallback when the PR head moved, but only on pull_request_target events where the run head is comparable — comment/review runs report main's tip, and posting wins over silence when the comparison is unavailable. - Define the marker once in a workflow-level env and pin all of the above in the workflow test suite, executing the fallback step's real bash against a stubbed gh. * fix(ci): close the fallback-comment gate gaps from round-2 review (QwenLM#9255) - Exclude comment-driven /resolve runs from the fallback gate: authorize runs on `@qwen-code /resolve` issue comments where github.event.inputs is empty, so the dispatch-only exclusion never fired there and a failed resolve run was misdiagnosed as a dead review recommending the wrong command. - Enumerate precheck-pr and delay-automatic-review failures in the gate: either failure marks review-pr 'skipped' (a transient API 5xx in delay's re-check step, or the fork-PR chain root dying before it posts anything), which the old gate never matched — silence, against its own "a skipped review is as unexplained as a dead one" norm. Both are 'skipped' where they do not apply, so the gate stays closed there. - Anchor the cross-job dedup on the run URL's closing paren: run ids grow digits over time, so the unanchored substring let a later run's fallback comment (id 123450) suppress an earlier run's (id 12345) re-run comment; every marker body renders the URL as [workflow logs](...runs/<id>), so the id is always followed by ')'. - Pin the previously surviving mutants in the workflow suite: the _diag probe guard polarity, the workflow_dispatch disjunction, the ephemeral-hosted-runner placement, and the fallback body's marker-link shape the anchor relies on; add executed coverage for each head-lookup partial failure and for a distinct run's fallback not suppressing this run's comment. * fix(ci): close round-3 review gaps in the fallback-comment defenses (QwenLM#9255) --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
npm run typecheck did not flag it; CI runs tsc --build, which does. The delete needs no suppression once the property is narrowed.
…ui-v2 # Conflicts: # packages/cli/src/commands/review/cleanup.test.ts # packages/cli/src/commands/review/cleanup.ts
|
Closing this in favour of two smaller PRs — this thread accumulated ten review rounds and stopped being readable.
Nothing is lost: #9274's tree is byte-identical to this PR's head plus the two threat-model headers. Every finding from all ten rounds here is fixed in one of the two, and the pin gaps that were deferred remain tracked in #9044. The reason for splitting rather than continuing: 26% of the diff here was hardening added round by round in response to an escalating threat model that was never scoped at the start, and 6 of the 17 Criticals in this thread were introduced by my own previous round's fix. Separating the two halves gives each one a contract a reviewer can hold it to. |
Pull request was closed
…LM#9255) * fix(ci): keep a fallback comment when the PR review runner dies A review job that dies abnormally never reaches its in-job fallback comment step: the runner worker crash in FinalizeJob on the PR QwenLM#8894 run (EACCES creating under the runner home directory) left the PR with no review and no explanation. - Probe write access to $HOME, $RUNNER_TEMP and the runner root at job start, repair single-directory ownership with the existing sudo pattern, and fail fast with a clear message when repair is impossible instead of burning the review budget to die at finalize. - Add a fallback-comment job on an ephemeral hosted runner that posts the retry guidance whenever review-pr fails. It derives the PR number from the event payload (dead job outputs do not survive a crash) and dedupes on a qwen-review-fallback comment marker plus this run's URL, so the in-job step, the ack comment, and re-runs never double-post. * fix(ci): harden the PR review fallback comment (QwenLM#9255) Review-round fixes for the fallback-comment defenses: - Probe the actual runner root (three levels above the workspace, not two) and the _diag subdirectory FinalizeJob writes in; a writable parent does not prove an existing subdirectory writable. - Open the fallback gate on authorize/review-config failures too — the incident's trigger can kill those earlier self-hosted jobs first, and a failed dependency marks review-pr 'skipped', which the old gate never matched. Guarded against resolve dispatch runs, which skip review-pr by design. - Author-scope the dedup lookup (resolved dynamically like upsert-bot-comment.sh) so a planted marker cannot suppress the fallback, and fail closed with bounded retry when the lookup or the state check fails instead of fail-open toward duplicates or a green job that never posted. - Skip the stale fallback when the PR head moved, but only on pull_request_target events where the run head is comparable — comment/review runs report main's tip, and posting wins over silence when the comparison is unavailable. - Define the marker once in a workflow-level env and pin all of the above in the workflow test suite, executing the fallback step's real bash against a stubbed gh. * fix(ci): close the fallback-comment gate gaps from round-2 review (QwenLM#9255) - Exclude comment-driven /resolve runs from the fallback gate: authorize runs on `@qwen-code /resolve` issue comments where github.event.inputs is empty, so the dispatch-only exclusion never fired there and a failed resolve run was misdiagnosed as a dead review recommending the wrong command. - Enumerate precheck-pr and delay-automatic-review failures in the gate: either failure marks review-pr 'skipped' (a transient API 5xx in delay's re-check step, or the fork-PR chain root dying before it posts anything), which the old gate never matched — silence, against its own "a skipped review is as unexplained as a dead one" norm. Both are 'skipped' where they do not apply, so the gate stays closed there. - Anchor the cross-job dedup on the run URL's closing paren: run ids grow digits over time, so the unanchored substring let a later run's fallback comment (id 123450) suppress an earlier run's (id 12345) re-run comment; every marker body renders the URL as [workflow logs](...runs/<id>), so the id is always followed by ')'. - Pin the previously surviving mutants in the workflow suite: the _diag probe guard polarity, the workflow_dispatch disjunction, the ephemeral-hosted-runner placement, and the fallback body's marker-link shape the anchor relies on; add executed coverage for each head-lookup partial failure and for a distinct run's fallback not suppressing this run's comment. * fix(ci): close round-3 review gaps in the fallback-comment defenses (QwenLM#9255) --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
3 participants
What this PR does
Phase 2 of evidence images:
qwen review capture-tui, the producer. When a finding's claim is about what the terminal renders — "the panel clips at 80 columns" — the verifier can now drive the code under review in a private tmux server, capture the pane exactly as rendered (.ansalways;.pngviafreezewhen installed), and attach the image to the finding via Phase 1'sassetFiles→publish-assetsflow. The verify brief gains the corresponding evidence block (capture at the claimed width and a control width; the evidence rung — pixels / bytes / prose — is part of the verdict; attach only what this verification launched).Why it's needed
A verifier ruling on a rendering claim without this reads the layout code and imagines a terminal; the imagining is where rendering verdicts go wrong, and a comment that embeds the clipped screenshot outranks one that describes it. The safety property is structural, not behavioral: every tmux call is scoped to a per-run private server socket (
-L, pid+nonce-named), so the capture cannot see, resize, or kill the user's own tmux sessions — the measured failure mode of desktop-automation verification — andkill-serverreaps everything the capture started.Two measured fidelity findings shaped the implementation, both caught by this PR's own smoke run:
freeze+ pipe stdin: freeze treats a pipe stdin (NodespawnSync's default) as "the input is stdin" and ignores the positional file — a promptly-EOF'd pipe yieldsERROR No input(exit 1), an open pipe hangs it indefinitely. Both modes reproduced locally in one evening; the historical "freeze hangs" incidents on this repo's own visual workflows are this exact shape. The command runs freeze withstdin: 'ignore'and writes the.ansbefore rendering, so a hang can cost the image but never the text evidence.capture-pane -Jdestroys layout evidence: joining wrapped lines re-flows the pane into logical lines — the smoke capture of a 100-char line in an 80-column pane came back as one long line, erasing the very wrap it was capturing. The capture uses-e -Nwithout-J; the same smoke now yields two lines (80 + 40), and the rendered PNG shows the wrap as the terminal drew it.Reviewer Test Plan
How to verify
Expected: all green — 2486 passed locally on this branch (macOS, tmux 3.7b, freeze 0.2.2). One pre-existing failure,
stale-bundle.test.ts, reproduces on pristinemainand is not from this change. The pure plan shapes (every tmux call-L-scoped,kill-servernotkill-session, no-J) are pinned without tmux; the command boundary runs against real tmux where present anddescribe.skipIf-skips where not.Live smoke (tmux required, freeze optional):
Evidence (Before & After)
Before: no producer — rendering claims argued from code reading. After: the smoke PNG above (80-column pane, 100-char line wrapped at exactly column 80), produced by the built CLI during this PR's verification; the
-Jvariant of the same capture (one un-wrapped line) is what motivated the no--Jrule and is preserved in the plan test's comment.Real-scenario capture — this PR photographing the product with itself. The built CLI's own onboarding TUI, driven by this PR's
capture-tuiin an isolatedQWEN_HOME, settled by--until 'Connect a Provider', rendered byfreeze— the exact claimed-width + control-width pair the verify brief prescribes. At 80 columns the provider description wraps mid-phrase ("…Standard API / Key") and the dialog borders shift; at 120 columns the same line holds. One image cannot distinguish "wraps at 80" from "wraps everywhere" — the pair can:Both manifests read
"evidence": "png", "settledBy": "until-match"; images live on the commit-pinnedpr-assets/capture-tui-8388branch, so the embedded evidence cannot change from under this description.Dogfooded against a live PR. #8368 adds Kimi / Xiaomi MiMo to the provider dialog — a rendering claim. Its branch was built in an isolated clone and driven by this PR's
capture-tui, gating the keystrokes on a rendered marker (--ready 'Third-party Providers' --keys Down Enter, the flag this dogfood run motivated: keys fired at start straddled the dialog's mount — a Down consumed, the Enter behind it lost) and settling on a marker unique to the new entry (--until 'Choose Kimi Code'— the obvious markerKimialso matches another entry's description in BOTH arms, and settled the control arm falsely until tightened):settledBy: "until-match")settledBy: "timeout")The after arm shows the new
Kimi — Choose Kimi Code or a regional Kimi API endpointentry rendered in the submenu; the before arm's manifest records--until never matched within 20000ms — late frame capturedover the full pre-change provider list. The manifests are the verdict; the frames are the pixels behind it.Tested on
Environment (optional)
Unit tests + built-CLI smoke, macOS, Node v24, tmux 3.7b, freeze v0.2.2.
Risk & Scope
.ans-only (bytes, not pixels, and the manifest says so). The brief's rules (claimed-width + control-width pair; rung named in the verdict) are prompt text and need dogfooding like any prompt change.packages/core/src/tools/glob.test.ts. test(core): deflake glob external-path test — dedicated empty dir, not /tmp #8604 gave that test a dedicated directory; this seeds a real file in it, because with an empty dir the assertions were vacuous — "found nothing at all" passed them. It rode along from the original branch and is called out here rather than left for a reviewer to trip over.capture-tuiregistered in the pinned subcommand surface test.Linked Issues
Supersedes #8388 — same change, recreated to collapse 373 commits into one and rebase onto current
main; that PR's review history (nineteen rounds) stays there for anyone tracing a decision.Phase 2 of #8351; uses the
assetFiles/publish-assetsflow it introduced. Two pieces were split out of the original and are not in this diff: the CI tooling step and its workflow pin (#8792), and the shell-registry deflake this PR's CI failures uncovered (#8795). Neither is a dependency for merging this one — until #8792 lands, the real-tmux suite skips on lanes without tmux exactly as it did before that step existed.中文说明
这个 PR 做了什么
证据图片的 Phase 2：
qwen review capture-tui，生产者。当 finding 的 claim 关于终端渲染——"面板在 80 列被裁切"——验证者现在可以在私有 tmux server 中驱动被审代码，按实际渲染捕获面板（.ans始终产出；装有freeze时渲染.png），并通过 Phase 1 的assetFiles→publish-assets流程把图片附到 finding 上。verify brief 增加对应的证据块（在 claim 声称的宽度和一个对照宽度各捕获一次；证据层级——像素/字节/散文——是裁决的一部分；只允许附上本次验证自己启动的东西）。为什么需要
没有它，验证者对渲染 claim 只能读布局代码然后想象终端；想象正是渲染裁决出错的地方，而嵌入裁切截图的评论强于描述它的评论。安全性质是结构性的而非行为性的：每个 tmux 调用都限定在每次运行的私有 server socket（
-L，pid+随机数命名），捕获不可能看到、更不可能调整或杀掉用户自己的 tmux 会话——这正是桌面自动化验证被实测过的失败模式——收尾的kill-server回收捕获启动的一切。两个实测保真度发现塑造了实现，都是本 PR 自己的冒烟跑抓出来的：
freeze+ pipe stdin：freeze 把 pipe stdin（NodespawnSync默认）当作"输入在 stdin"而无视位置参数文件——很快 EOF 的 pipe 报ERROR No input（exit 1），保持打开的 pipe 让它无限挂起。两种模式一晚内在本机全部复现；本仓库视觉工作流历史上的"freeze 挂起"事件正是这个形状。命令以stdin: 'ignore'运行 freeze，且先写.ans再渲染——挂起最多损失图片，绝不损失文本证据。capture-pane -J摧毁布局证据：合并折行把面板重排为逻辑行——80 列面板里 100 字符的行被冒烟捕获成一根长行，恰好抹掉了要捕获的折行。捕获改用-e -N不带-J；同一冒烟现在产出两行（80 + 40），渲染的 PNG 按终端实际绘制显示折行。Reviewer 测试计划
如何验证
预期：全绿（本地 49 文件 / 1506 断言；macOS，tmux 3.7b + freeze 0.2.2）。纯计划形状（每个 tmux 调用带
-L、kill-server而非kill-session、无-J）无需 tmux 即钉住；命令边界在有 tmux 的机器上驱动真实 tmux，无 tmux 时describe.skipIf跳过。实机冒烟（需 tmux，freeze 可选）：
证据（Before & After）
Before：无生产者——渲染 claim 靠读代码论证。After：上述冒烟 PNG（80 列面板，100 字符行恰在第 80 列折行），由本 PR 验证期间的构建版 CLI 产出；同一捕获的
-J版本（一根未折行的长行）正是 no--J规则的动机，保存在计划测试的注释里。真实场景捕获——本 PR 用自己给产品拍照。 构建版 CLI 自己的 onboarding TUI，由本 PR 的
capture-tui在隔离QWEN_HOME中驱动，--until 'Connect a Provider'定桩，freeze渲染——正是 verify brief 规定的「声称宽度 + 对照宽度」证据对。80 列下 provider 描述在短语中间折行（"…Standard API / Key"）、对话框边框位移；120 列下同一行完整。单张图无法区分"80 列折行"与"处处折行"——成对可以：两份 manifest 均为
"evidence": "png", "settledBy": "until-match"；图片存于 commit-pinned 的pr-assets/capture-tui-8388分支，嵌入的证据不可能在描述之下被偷换。对在评真实 PR 的 dogfood。 #8368 往 provider 对话框新增 Kimi / Xiaomi MiMo——一个渲染主张。在隔离 clone 中构建其分支，用本 PR 的
capture-tui驱动：按键以渲染标记门控（--ready 'Third-party Providers' --keys Down Enter——这个 flag 正是本次 dogfood 催生的：启动即发的按键横跨对话框挂载，Down 被消费、其后的 Enter 丢失），并以新条目独有的标记定桩（--until 'Choose Kimi Code'——直觉的Kimi在两个构建里都能匹配到另一条目描述中的 "Kimi K2.6"，曾让对照组假 settle，收紧后才干净）：settledBy: "until-match"）settledBy: "timeout"）after 侧显示子菜单中渲染出了新条目
Kimi — Choose Kimi Code or a regional Kimi API endpoint；before 侧的 manifest 记录--until never matched within 20000ms — late frame captured，帧为改动前的完整 provider 列表。manifest 即裁决，帧是其背后的像素。测试平台
环境（可选）
单元测试 + 构建版 CLI 冒烟，macOS，Node v24，tmux 3.7b，freeze v0.2.2。
风险与范围
.ans（字节而非像素，manifest 会说明）。brief 规则（声称宽度 + 对照宽度成对；裁决中点名层级）是 prompt 文本，和任何 prompt 改动一样需要 dogfooding。capture-tui已登记进钉住的子命令清单测试。关联 Issue
无。#8351 的 Phase 2；使用其引入的
assetFiles/publish-assets流程。