feat(review): capture-tui — rendering claims get pixels, not prose (Phase 2) - #8388
feat(review): capture-tui — rendering claims get pixels, not prose (Phase 2)#8388wenshao wants to merge 68 commits into
Conversation
…hase 2) Phase 1 (#8351) gave findings a place to carry image evidence and a sanctioned way to publish it. This is the producer: when a claim is about what the terminal RENDERS — "the panel clips at 80 columns" — the verifier has, until now, read the layout code and imagined a terminal, which is where rendering verdicts go wrong. New `qwen review capture-tui` makes the terminal real and the evidence a file. Mechanics and the properties that make it safe and honest: - PRIVATE tmux server (-L, pid+nonce-named): the capture is structurally unable to see, resize or kill the user's own tmux sessions — the measured failure mode of desktop-automation verification. kill-server reaps every process the capture started; the socket file is unlinked (tmux leaves it behind). - Evidence ladder, explicit: .ans (pane bytes, always) → .png (freeze, when available) → refusal (no tmux), manifested with WHY the ladder stopped, because a verifier must say whether its verdict stands on pixels, bytes, or prose. - freeze runs with stdin: 'ignore', and the comment records the measured reason: freeze treats a pipe stdin — Node's spawnSync default — as "the input is stdin" and ignores the positional file; a promptly-EOF'd pipe produces `ERROR No input` (exit 1), an open pipe hangs it indefinitely. Both modes were reproduced on one machine in one evening, and the historical "freeze hangs" incidents on this repo's workflows are this exact shape. The .ans is written BEFORE freeze runs, so a hang costs the image, never the text evidence. - capture-pane WITHOUT -J: joining wraps re-flows the pane into logical lines, erasing the wrap structure a layout claim is about — measured on this change's own smoke capture, where -J turned a wrapped 100-char line back into one long line. With -e and -N, the .ans is the pane exactly as rendered: 80+40 across two lines at 80 columns. - --until polls the pane for a settle marker (capturing anyway on timeout, recorded as degraded); --keys sends tokens one per send-keys, verbatim. The verify brief gains the corresponding evidence block: capture at the claimed width AND a control width (one image cannot distinguish "clips at 80" from "clips everywhere"); the evidence rung is part of the verdict; attach only what this verification launched — going around the private server with bare tmux or OS screenshots is the leak it exists to prevent. Findings carry the image via assetFiles into Phase 1's publish flow. Tests: pure plan shapes (every tmux call -L-scoped, kill-server not kill-session, no -J) pinned without tmux; the command boundary driven against REAL tmux where present (rendering captured with escapes, ladder honesty, no server left behind, until-match and until-timeout settles, keys verbatim, geometry and empty-command refusals) and skipped where not. 49 files / 1506 assertions; tsc --build and eslint clean.
…v hygiene, evidence honesty - Validate --until regex up front: an invalid pattern refused (exit 3) before any process starts, instead of an uncaught throw from inside a running capture; tmux failing mid-run (old tmux, dead server) now also lands on the refusal contract instead of a stack trace. - End option parsing with -- in new-session and send-keys: without it tmux silently eats dash-leading tokens as flags (measured: send-keys -l exits 0 and types nothing — silent evidence corruption). - Keep the frame that MATCHED --until: re-capturing after the match could store a later frame that no longer shows the marker the manifest claims settled the capture. - Skip freeze on a zero-byte capture: freeze fails empty input with a misleading bounds error, and an empty image is evidence-shaped noise; the manifest now says the pane captured empty. - send-keys argv moved into the pure tmuxPlan so all four shapes are pinned by unit tests; socket cleanup now honors TMUX_TMPDIR (what tmux actually uses) instead of TMPDIR. - Tests: settle on content (--until) instead of fixed delay — a fixed delay races shell startup under CI load (measured once: blank pane → empty .ans → freeze bounds error → flaky png assertion); new probes pin process death (not just socket removal), invalid-regex refusal, fixed-delay manifest honesty, and dash-leading keys.
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. |
…-server timeout, manifest contract
- Refuse NaN/negative/oversized --settle-ms and --timeout-ms up front:
yargs coerces a non-numeric value to NaN, Atomics.wait treats a NaN
timeout as INFINITY (a capture that never returns), and a NaN
deadline makes the --until poll loop unexpirable.
- Give every tmux control call a 15s timeout so a wedged server turns
into a refusal instead of hanging the review agent behind it.
- degradedBecause now honors its own contract ("why the ladder stopped
— freeze missing, timeout, …"): an --until timeout is recorded there
alongside any freeze degradation, joined when both are true.
- CaptureManifest.evidence excludes 'none': a refused capture writes no
manifest at all — 'none' is the rung a verdict stands on when there
is no manifest to cite, not a value a manifest can carry.
A host without `which` (some minimal images) would misdiagnose an installed tmux as missing. `tmux -V` / `freeze --version` answer the only question that matters — the binary responds — with a timeout so a wedged probe refuses instead of hanging.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
— qwen3.7-max via Qwen Code /review
The refusal fires exactly where describe.skipIf(!hasTmux) skips the real-tmux suite, so a probe refactor (e.g. condition inversion) could silently break it with no test noticing. The probes now sit behind an exported seam; a suite that runs on every host drives the seam to pin the refusal contract (exit 3, no artifacts, no throw), and the freeze-less rung is likewise pinned on hosts that have freeze.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
…verage, probe fidelity - An unwritable --out (EACCES/EROFS/ENOSPC) now lands on the refusal contract like every other caller/environment mistake, instead of a raw stack trace with exit 1. - The blank-capture guard trims: a pane of nothing but whitespace has no pixels worth rendering either, and the branch is now deterministically testable (sleep + settle 0). - New tests pin the two untested ladder rungs: the empty-pane branch, and the freeze-CRASH branch via a fake freeze binary that fails loudly — real exec pins the errTail composition (exit status + stderr tail), with the probe seam forced open. - The socket-cleanup probe resolves the socket dir the way production (and tmux) does — TMUX_TMPDIR, else /tmp — closing its vacuity on hosts where TMUX_TMPDIR is set. - Geometry lower bounds (20x5) pinned as ACCEPTED; start plan pinned to carry new-session and '-s cap' (the join key -t targets).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
…ching, signal reap, async capture R2-1 (Critical): wrap the command in an sh -c holder so the pane outlives it — tmux's remain-on-exit off destroyed the session the moment a one-shot command exited (measured 0/10 capturable), exactly the render-and-exit fixture shape the verify brief prescribes. The holder survives exotic default-shells, single-quotes are escaped, and kill-server reaps it with everything else. R2-2: --until now matches a LOGICAL view (capture-pane -p -J, no -e): a marker spanning a wrap boundary or an SGR attribute change could never match the physical frame. The .ans stays physical. R2-3: the regex test itself is time-budgeted via vm (500ms): a backtracking-prone pattern could spin one test() call past any deadline, orphaning the server inside the try. R2-4: the capture is now async (waits are setTimeout, the event loop is idle during them) with SIGINT/SIGTERM handlers that reap the private server and re-raise — finally alone is skipped on signal-driven termination, orphaning exactly what the command exists to make impossible. In the old fully-sync shape a JS signal handler could never have run at all. R2-5: the brief's --out template gains a -<cols> discriminator so the pair rule's second capture cannot silently overwrite the first. R2-6: probe freeze with --help, not --version — freeze <=0.1.6 (the 2024 release line) has no --version and an installed freeze was misdiagnosed as absent (measured on v0.1.6). R2-7: the .ans and manifest writes land on the refusal contract when the target turns unwritable mid-capture (ENOSPC during a long settle). R2-8/R2-9: every guard and refusal branch is now pinned — duration bounds (negative/over-max), the tmux-mid-failure catch (fake tmux binary: -V passes, everything else fails), unwritable --out, the --rows upper bound, the up-front regex validation (stderr reason pinned, distinguishing it from the mid-capture path), true server death (pid probed while alive, asserted dead after), and the no-tmux refusal reason.
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 reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
…idence truthfulness, argv shapes
- Hold on its own line (R3-2): a trailing ';' made ';;' (pane died
instantly on a valid command), a trailing '#' comment swallowed the
keep-alive; a trailing backslash is refused (line continuation would
fold the hold into the command).
- Refuse empty --until (R3-1): new RegExp('') matches a blank pane and
produced a false until-match settle. Refuse empty --out (R3-10):
resolve('') is the cwd — artifacts would clobber <cwd>.ans. Probe the
write target up front (R3-10): mkdirSync{recursive} does no
permission check on an existing dir, so an unwritable --out ran the
full capture and lost the pane text at the last write. Refuse a
--cwd that is not a directory (R3-7): tmux -c silently falls back to
the launching cwd — evidence from the wrong directory. Refuse
non-string argv shapes (R3-16): duplicated options parse to arrays
and --no-X to booleans; both threw uncaught or corrupted the settle.
- Evidence truthfulness: the png rung requires the file to exist, not
just exit 0 (R3-6); a freeze that could not spawn is named 'spawn
failed: …' instead of 'exit null' (R3-14); the manifest records cwd,
keys, until and the active duration (R3-17) — the capture's only
record must name every rendering-affecting input.
- Reap: unlink the socket only when the server is known dead (R3-20) —
unlinking after a failed kill made a live server unreachable forever.
- The freeze render is a seam ({bin, timeoutMs}, R3-19): the hang belt
is now provable (fake freeze sleeping past a shortened belt) and
fake binaries are addressed absolutely (a non-executable PATH shim
is skipped by execvp).
- Tests: SIGTERM mid-poll reaps the child's private server (tsx child
driver, R3-4); yargs handler mapping drives the real handler (R3-3);
probe flag pinned to --help via a fake accepting only it (R3-5);
per-token keys pinned (R3-8); keys in fixed-delay mode (R3-13);
duration maxima accepted (R3-12); ReDoS bound tightened to 8s
(R3-11); trailing spaces preserved (-N, R3-18); bash/pgrep absence
loud or gated (R3-15). CI's Linux test lane installs tmux (R3-9) so
the real-tmux suite actually executes there.
Review:
|
Dogfooded against a live PR's provider-dialog change and the fire-at- start --keys straddled the UI's mount: a Down was consumed and the Enter behind it lost, so key-driven captures of anything that takes a moment to render were unreliable. --ready <regex> polls the logical view (same budgeted matching as --until, one shared deadline so the two phases cannot stack to 2x --timeout-ms) and only then types; when it never matches the keys are WITHHELD — typing into a screen that never reached the expected state drives an unknown UI — and the manifest records both the miss and the withholding (keysSent: false). Tests pin the gate with a fixture that drains its input before printing the marker (exactly how a mounting TUI eats early keys): the keys-at-start mutant loses its keystrokes to the drain and turns red. The brief gains the two measured lessons: gate keys on --ready, and pick pair markers unique to the claim — a substring present in both arms (a provider name inside another entry's description) settles the control arm falsely.
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
…mux version gate, refusal reasons - A refused re-run to the same --out left the previous run's manifest/png behind while its .ans was truncated by the writability probe — a stale manifest claiming evidence from a different capture (the reviewer's merge blocker). The probe now uses a unique sibling, and all prior artifacts are cleared before anything starts: every exit path leaves this run's artifacts or nothing. - Refuse tmux older than 3.0a up front, naming the version: capture-pane -N landed in 3.0a, and an older tmux otherwise died mid-capture on the unknown flag with the blame misattributed. - The refusal JSON on stdout now carries reason and evidence:'none' so an agent consumer need not scrape stderr to tell why the ladder stopped. - Budget-overrun marker matches are recorded in degradations instead of folding into 'never matched' — the marker may be present, its match cut off. - ci.yml: the tmux install is advisory — an apt hiccup must not fail the job for a describe.skipIf-gated suite. - Nits: orphaned probes JSDoc moved back onto probes; probe doc drift fixed (available() replaced by probeOutput()); CaptureManifest.ansPath tightened to string; the shape guard refuses undefined required options (runCaptureTui is exported); the signal-handler comment names the double-delivery trade-off; prettier on the four review files.
A FIFO at the manifest path blocked the clear phase's readFileSync FOREVER. Not a throw: no refusal was printed, no reap handler existed yet, and NO in-process timeout could interrupt it, because the read is synchronous on the main thread — measured, a regression wedged a whole vitest run past its own 10s test timeout until the runner killed it. Only a regular file can be a capture manifest, so the read is stat-guarded and the collision gate then refuses by name. Its pin runs from a child with an external kill deadline, so a future regression turns red instead of wedging CI. The probe called a binary that RAN and failed "not installed" — an installed freeze whose --help exits non-zero, or dies on a signal, spawned perfectly well. That false claim persisted into the manifest and sends an operator to install something already present. Only ENOENT is absence now. reap() printed a false orphan WARNING when the socket path exceeded sun_path: tmux answers `File name too long` to both new-session and kill-server, so nothing was ever created and nothing needed killing. Dropped the per-path append probe: the collision gate refuses on any survivor at the two mandatory paths, so every shape that could fail the final write — a directory, a read-only file, an append-only one — is already refused before the capture window opens. The probe was dead code that also read as a promise it no longer kept. Two comments corrected rather than left to mislead: REAP_SIGNALS does not cover every terminating signal (SIGUSR2, SIGALRM and friends still kill the process past the reap — the holder's watchdog is what bounds those), and the tmux 3.0 refusal is a scope choice, not the crash prediction it claimed, since the ladder would run the same trimmed rung there. Test-hardening findings from this round are deferred as follow-ups per the new criterion: production defects block, coverage gaps do not.
Review criterion for the remaining roundsSixteen review rounds in, the finding count is not converging — 21, 26, 29, 19, 30 across the last five — while the diff grew past 6k lines with roughly 69% of it tests. Three of the last nine Criticals were defects introduced by the previous round's own fix. A loop where each fix enlarges the reviewed surface does not reach zero, so the exit criterion changes:
The CI tooling step and its 400-line workflow pin moved to #8792 in Deferred follow-ups from round 16, all coverage rather than behaviour: the realpath-keyed base de-dup and the EPERM liveness arm are unexercised by the sweep fixtures; the live-server negation matcher is loose enough to admit a reduced-options kill; the seeded bounds loop has no 中文说明十六轮下来发现数没有收敛（近五轮 21/26/29/19/30），而 diff 已超过 6000 行、约 69% 是测试；最近九个 Critical 里有三个是上一轮修复自己引入的。每次修复都在扩大被审面积的循环不会归零，因此改判据：
CI 工具链步骤及其 400 行钉子已拆到 #8792。在 #8792 合并前，本 PR 的真实 tmux 套件在 CI 上按原样跳过（与该步骤存在之前一致），本地和自带 tmux 的 lane 仍然实跑。 |
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: test-efficacy — harness control never ran (harnessValidated: null); mutant/hunk coverage unprobed (19 mutants + 3 hunks skipped for cap; no probe file was green in the unmutated baseline).
Not explored to full depth (tool budget reached): chunk 1: did not execute review.test.ts / agent-prompt.test.ts under vitest — the worktree has no node_modules and installing the monorepo's dependencies was out o….
Not reviewed: reverse audit — stopped before round 2 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：test-efficacy — harness control never ran (harnessValidated: null); mutant/hunk coverage unprobed (19 mutants + 3 hunks skipped for cap; no probe file was green in the unmutated baseline)。
未探索到全部深度（达到工具调用预算）：chunk 1：did not execute review.test.ts / agent-prompt.test.ts under vitest — the worktree has no node_modules and installing the monorepo's dependencies was out o…。
未审查：反向审计——评审时间预算不足，未能开始第 2 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
…out base existsSync and statSync follow links, so a pre-existing DANGLING symlink at an artifact path read as "nothing here" at every gate. The collision gate never fired, and writeFileSync's O_CREAT then created the pane text and the manifest at the links' targets — anywhere the user can write, outside the --out base — while the run reported success and the manifest named <out>.json. Probe-verified end to end against real tmux: the base held only the two symlinks, and the bytes landed in a sibling directory. The png path had the same shape one rung down: a stat-based stamp read the dangling link as absent, so the render proceeded THROUGH it and the image landed outside the base with `evidence: 'png'` recorded — measured, one file at the link target and the rung claimed. Occupancy is an lstat question everywhere it is asked: the stamp, the collision gate, the change test, and the manifest-read guard. A link at a mandatory path now hits the named refusal; at the png path it degrades to the text rung like any other foreign occupant. The link itself survives — it was never ours. A live-target link was already handled correctly; only the dangling shape slipped, which is why the existing symlink coverage did not catch it. Both lstat sites are pinned and both mutants turn red: the gate's via the mandatory-path test, the stamp's via the png one.
refuse() wrote the human line and the machine-readable JSON inside ONE try, so a synchronous throw from the stderr write — a file-backed stderr under ENOSPC throws sync in Node — skipped the stdout JSON entirely and left a HEALTHY stdout with nothing (measured: exit 3, empty stdout). The consumer is an agent that must tell an environment refusal from a caller mistake without scraping stderr, so the two writes are now independent. The probe also claimed "this host could not spawn it" for binaries that spawned perfectly well and then failed — a non-zero --help exit, a signal death. The result carries whether it RAN, and each message says which: installed-but-unusable, or unstartable here. A false environment claim persisted into the manifest is exactly what this command exists not to do. Test hygiene, same round: the signal test now kills its child on a thrown assertion instead of orphaning a node process (and its tmux server) inside a 60s capture on precisely the failure it polices, and the artifact-path test's title and comment no longer describe an append-mode probe that was removed two commits ago — what pins it now is the collision gate, asserted by content rather than existence.
|
@qwen-code /review |
Thirteen findings on this PR, all against the pin rather than the step, and two of them were the pin being WRONG rather than weak — those matter most, because a pin that reds a legitimate edit is worse than one that misses. Wrong: the redirect check matched `>` anywhere in an echo statement, including inside the quoted message, so a semantics-preserving reword was rejected — exactly what this file says it must not pin. And the sudo requirement demanded literal `sudo apt-get` adjacency, which rejects `sudo -n apt-get`, the sibling workflow's own convention. Both now ask the question at the right level: quoted spans are blanked before any question about shell syntax, and "runs through sudo" is asked of the raw statement. Weak, each mutation-verified against ci.yml: `&>` was split as a separator, so `echo '::warning::…' &> /dev/null` tore into a clean echo and an unchecked redirect; apt's `-o APT::Get::Simulate=true` reached simulate mode through a door the token blacklist never saw; the package pin matched the whole logical line, so a package dropped from the install survived if its token appeared in the guard's message; `apt-get update` was required by nothing; `-y` and sudo were checked only on the FIRST install statement, and nothing kept a foreign command off the install's chain to short-circuit it; the step's `shell:` key — the one key that decides which interpreter parses the entire block — was unpinned; the closing `fi` was unpinned, so deleting it passed 5/5 while bash rejects the block and the step runs nothing; and the else branch's annotation was a substring check unanchored to any emitting command, so `: '::warning::…'` muted it. The ~180 lines of bash-semantics helpers are the ORACLE every pin reasons through, and they had no tests of their own. They do now — a table per rule, each rule traceable to a mutation that escaped an earlier version. Writing them corrected one of my own assumptions: `echo a \` + newline + `b` joins to `echo a b`, not `echo ab`, because the space before the backslash survives. Also corrected the rationale in both files: they asserted in the present tense that a real-tmux suite exists in this tree. It does not — it arrives with #8388, and the tmux half of this step is deliberately pre-landed for it. Saying so is the honest version.
A test I added this round faked the tmux version and never put it back, and the real-tmux describe restored nothing — so every capture after it believed tmux was 3.9 and the plan sent `-T`. On a runner whose tmux is 3.2a that is `unknown flag -T`, and 21 tests failed as refusals. It was invisible here because this machine's tmux is 3.7b and accepts the flag. Reproduced and A/B'd on a tmux 3.3a host, which lacks -T the same way: without the hook, 21 failed with that exact reason; with it, 107 passed and 4 skipped. That is the CI failure, matched test-for-test. The fix is the hook rather than the one test: `probes.tmux` and `probes.freeze` are restored in afterEach, so no test in that describe can leak a fake environment into the next one. A hook cannot be forgotten; the per-test restore I skipped can be, and was.
The previous commit shipped the constants and the comment but not the two assignments — a container A/B stripped them from the working tree and my re-insertion check matched a DIFFERENT occurrence of the same string, so it reported the hook intact when it was not. tsc said 'declared but never read' in the same run and I pushed anyway. The hook is in now: tsc and eslint clean, 111/111 locally, and the tmux 3.3a A/B stands — without the assignments 21 tests fail with 'unknown flag -T', with them 107 pass and 4 skip.
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. 4 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Unresolved, please confirm: [Critical] comment 3733696855 (capture-tui.test.ts, R10-1 render-window signal race) — blocker body truncated in the context rendering; not readable in full within this run's time budget, status at the reviewed commit undetermined [Critical] comment 3725964251 (capture-tui.ts:116, sync-belt SIGTERM-immune child) — body truncated; could not be read in full, status undetermined [Critical] comment 3708601056 (capture-tui.ts, R4-1 shape-guard vs clear ordering) — body truncated; could not be read in full, status undetermined [Critical] comment 3722808202 (capture-tui.ts, R6-3 existing-directory refusal ordering) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696879 (capture-tui.ts, R10-32 clearArtifact recursive fallback) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696862 (capture-tui.ts, R10-12 EMFILE shaped bypass) — body truncated; could not be read in full, status undetermined [Critical] comment 3728158253 (capture-tui.ts, recursive cleanup site 1/4) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696877 (capture-tui.ts, R10-23 sentinel unlink ordering) — body truncated; could not be read in full, status undetermined [Critical] comment 3741921096 (capture-tui.ts:460, R12-1 png clear authorization) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696859 (capture-tui.ts:813, R10-11 WARNING write throw) — body truncated; could not be read in full, status undetermined [Critical] comment 3716324144 (capture-tui.ts, R5-2 0-byte png rung) — body truncated; could not be read in full, status undetermined [Critical] comment 3728158257 (capture-tui.ts, recursive cleanup site 2/4) — body truncated; could not be read in full, status undetermined [Critical] comment 3728158264 (capture-tui.ts, recursive cleanup site 3/4) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696885 (capture-tui.ts:1188, R10-33 unconditional png unlink) — body truncated; could not be read in full, status undetermined [Critical] comment 3728158266 (capture-tui.ts, recursive cleanup site 4/4) — body truncated; could not be read in full, status undetermined [Critical] comment 3741216572 (capture-tui.ts, png rung crediting a spared file) — body truncated; could not be read in full, status undetermined [Critical] comment 3741216575 (capture-tui.ts, write-failure cleanups unconditional) — body truncated; could not be read in full, status undetermined [Critical] comment 3741921098 (capture-tui.ts, R12-2 stdio failure flipping exit disposition) — body truncated; could not be read in full, status undetermined [Critical] comment 3742772795 (capture-tui.ts:1227, R13-35 success-path truncation of spared occupants) — body truncated; could not be read in full, status undetermined [Critical] comment 3741216577 (tui-capture.test.ts:409, -N phantom padding / -T gate) — body truncated; could not be read in full, status undetermined [Critical] comment 3741921094 (tui-capture.ts:95, R11-3 tmux 3.1/3.2 padding window) — body truncated; could not be read in full, status undetermined [Critical] comment 3700766697 (tui-capture.ts:342, R2-1 one-shot command holder) — body truncated; could not be read in full, status undetermined [Critical] comment 3716324151 (tui-capture.ts, R5-3 holder SIGINT survival) — body truncated; could not be read in full, status undetermined [Critical] comment 3722808170 (tui-capture.ts, R6-1 readyFile escaping) — body truncated; could not be read in full, status undetermined [Critical] comment 3722808212 (tui-capture.ts, R6-4 post-exit C-c holder survival) — body truncated; could not be read in full, status undetermined [Critical] comment 3741216578 (tui-capture.ts, escapeTrailingSemicolon backslash pass-through) — body truncated; could not be read in full, status undetermined [Critical] comment 3741216582 (tui-capture.ts, new-session -c format expansion) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696871 (tui-capture.ts:402, R10-14 trailing-semicolon command splitting) — body truncated; could not be read in full, status undetermined [Critical] comment 3742772789 (tui-capture.ts, R13-1 watchdog subshell signal immunity) — body truncated; could not be read in full, status undetermined [Critical] issue-level comment 5199834809 (@wenshao review at head fa23ef4) — body truncated; could not be read in full, status undetermined [Critical] issue-level comment 5200984052 (@qwen-code-dev-bot round summary) — body truncated; could not be read in full, status undetermined Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally. Not reviewed: test-efficacy — harness control never ran (harnessValidated: null); mutant/hunk coverage unprobed. Not explored to full depth (tool budget reached): chunk 2: (none — all planned checks completed); chunk 1: could not execute the test files (no node_modules in the review worktree) — all checks above are static source traces instead.; chunk 9: executing tui-capture.test.ts under vitest — neither the review worktree nor the parent checkout has node_modules , and a full monorepo install exceeded this…. Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
已审查。 建议见行内评论。 4 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未决，请确认：[Critical] comment 3733696855 (capture-tui.test.ts, R10-1 render-window signal race) — blocker body truncated in the context rendering; not readable in full within this run's time budget, status at the reviewed commit undetermined [Critical] comment 3725964251 (capture-tui.ts:116, sync-belt SIGTERM-immune child) — body truncated; could not be read in full, status undetermined [Critical] comment 3708601056 (capture-tui.ts, R4-1 shape-guard vs clear ordering) — body truncated; could not be read in full, status undetermined [Critical] comment 3722808202 (capture-tui.ts, R6-3 existing-directory refusal ordering) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696879 (capture-tui.ts, R10-32 clearArtifact recursive fallback) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696862 (capture-tui.ts, R10-12 EMFILE shaped bypass) — body truncated; could not be read in full, status undetermined [Critical] comment 3728158253 (capture-tui.ts, recursive cleanup site 1/4) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696877 (capture-tui.ts, R10-23 sentinel unlink ordering) — body truncated; could not be read in full, status undetermined [Critical] comment 3741921096 (capture-tui.ts:460, R12-1 png clear authorization) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696859 (capture-tui.ts:813, R10-11 WARNING write throw) — body truncated; could not be read in full, status undetermined [Critical] comment 3716324144 (capture-tui.ts, R5-2 0-byte png rung) — body truncated; could not be read in full, status undetermined [Critical] comment 3728158257 (capture-tui.ts, recursive cleanup site 2/4) — body truncated; could not be read in full, status undetermined [Critical] comment 3728158264 (capture-tui.ts, recursive cleanup site 3/4) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696885 (capture-tui.ts:1188, R10-33 unconditional png unlink) — body truncated; could not be read in full, status undetermined [Critical] comment 3728158266 (capture-tui.ts, recursive cleanup site 4/4) — body truncated; could not be read in full, status undetermined [Critical] comment 3741216572 (capture-tui.ts, png rung crediting a spared file) — body truncated; could not be read in full, status undetermined [Critical] comment 3741216575 (capture-tui.ts, write-failure cleanups unconditional) — body truncated; could not be read in full, status undetermined [Critical] comment 3741921098 (capture-tui.ts, R12-2 stdio failure flipping exit disposition) — body truncated; could not be read in full, status undetermined [Critical] comment 3742772795 (capture-tui.ts:1227, R13-35 success-path truncation of spared occupants) — body truncated; could not be read in full, status undetermined [Critical] comment 3741216577 (tui-capture.test.ts:409, -N phantom padding / -T gate) — body truncated; could not be read in full, status undetermined [Critical] comment 3741921094 (tui-capture.ts:95, R11-3 tmux 3.1/3.2 padding window) — body truncated; could not be read in full, status undetermined [Critical] comment 3700766697 (tui-capture.ts:342, R2-1 one-shot command holder) — body truncated; could not be read in full, status undetermined [Critical] comment 3716324151 (tui-capture.ts, R5-3 holder SIGINT survival) — body truncated; could not be read in full, status undetermined [Critical] comment 3722808170 (tui-capture.ts, R6-1 readyFile escaping) — body truncated; could not be read in full, status undetermined [Critical] comment 3722808212 (tui-capture.ts, R6-4 post-exit C-c holder survival) — body truncated; could not be read in full, status undetermined [Critical] comment 3741216578 (tui-capture.ts, escapeTrailingSemicolon backslash pass-through) — body truncated; could not be read in full, status undetermined [Critical] comment 3741216582 (tui-capture.ts, new-session -c format expansion) — body truncated; could not be read in full, status undetermined [Critical] comment 3733696871 (tui-capture.ts:402, R10-14 trailing-semicolon command splitting) — body truncated; could not be read in full, status undetermined [Critical] comment 3742772789 (tui-capture.ts, R13-1 watchdog subshell signal immunity) — body truncated; could not be read in full, status undetermined [Critical] issue-level comment 5199834809 (@wenshao review at head fa23ef4) — body truncated; could not be read in full, status undetermined [Critical] issue-level comment 5200984052 (@qwen-code-dev-bot round summary) — body truncated; could not be read in full, status undetermined 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。 未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。 未审查：test-efficacy — harness control never ran (harnessValidated: null); mutant/hunk coverage unprobed。 未探索到全部深度（达到工具调用预算）：chunk 2：(none — all planned checks completed)；chunk 1：could not execute the test files (no node_modules in the review worktree) — all checks above are static source traces instead.；chunk 9：executing tui-capture.test.ts under vitest — neither the review worktree nor the parent checkout has node_modules , and a full monorepo install exceeded this…。 未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
…t lied Three production defects, one comment. The manifest-write-failure cleanup ran all three removals in ONE try, so a throw on the .ans or .png removal skipped the manifest one — leaving exactly the partial manifest the block calls worse than none. Per path now, like the clear phase's own clearArtifact, which already had it right. The clear-phase manifest read was unbounded: readFileSync + JSON.parse on any regular file at <out>.json, so a large one killed the process with a heap OOM before any refusal could print. lstat already holds the size, so capping costs nothing — past the cap the file is simply not a manifest of ours, and the collision gate refuses it by name. The `untouched` sentinel promised, in a comment, that anything taken before the stamps exist "can neither claim an evidence rung nor authorize a delete". Its runtime semantics were the opposite: `existed: true` with an impossible inode made changed() return true for every real file. It is fail-safe by construction now — changed() answers false for an unstamped sentinel — rather than by assertion. No test can discriminate this today, because every path assigns its stamps before the cleanups read them; that is precisely what made the wrong comment dangerous rather than harmless. Also removed a duplicated FIFO-hazard comment: two near-identical blocks sat back to back, one of them already missing the measurement the other carried. The oversize pin is discriminating: it seeds a VALID, manifest-shaped file above the cap, so without the cap the clear phase parses it and deletes the siblings, and with the cap nothing is touched. A garbage payload would have passed either way.
Every one of these was a pin that passed for the wrong reason — the failure mode this PR keeps rediscovering, this time entirely in tests I wrote. The shape-bounds family asserted exit 3 and cleared artifacts but never the REASON, so deleting a gate outright shipped green: the run sailed on and refused for some other reason, which is still exit 3 with the artifacts cleared. Each case now names the gate it pins, and deleting the empty-command guard turns it red. 'keeps a pre-existing .ans when its write fails at OPEN' can no longer reach its named seam at all: the collision gate added later refuses any occupant before the capture runs, so the EMFILE-at-write path is unreachable for a pre-existing file. Retired rather than reworded — the behaviour it cared about is enforced earlier now, and pinned there. Three coverage gaps closed: a failed reap must not print the success line and a live socket must never be announced as reaped (every stdout negative in that suite named the orphan, so a hoisted success line escaped all of them — mutation-verified), and geometry must PROPAGATE, since every plan call in the unit file used the same 80x24 and a hardcoded pair would have passed the entire suite. The reason assertion rides inside the compared value rather than as a second expect() argument, which vitest/valid-expect bans — the same lint I tripped over once before in this file.
The write-probe test exhausted the whole process's descriptor table to drive a real EMFILE. vitest runs test files in worker THREADS that share one process, so that starved whatever else happened to be running in the same worker: the full review suite failed here and timed out an unrelated hadolint isolation test in the same run, while this file alone passed 3/3. My test was destabilising its neighbours. It runs from a child now — the same shape the EPIPE, ENOSPC and FIFO pins use — so exhaustion is contained to a process that exists only for it, and real exhaustion is still what drives the EMFILE. Full review suite twice in a row: back to the single pre-existing stale-bundle failure that comes from main. Also dropped two imports the move left unused, which tsc and eslint both named — the same signal I pushed past two commits ago.
|
@qwen-code /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 reviewed: test-efficacy — harness control never ran (harnessValidated: null); mutant/hunk coverage unprobed (reverting the diff breaks the new tests' imports; 7 revert probes inconclusive). Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally. Not explored to full depth (tool budget reached): chunk 7: could not execute npx vitest run src/commands/review/cleanup.test.ts — the worktree has no node_modules and no vitest binary exists on this machine; a full mo…; chunk 7: did not trace every internal branch of auditPrWrites exercised by the runCleanup('pr-8388') test — verified structurally (whole body wrapped in try/catch wi…; chunk 8: I did not run the new test file (no node_modules in the review worktree; I didn't do a full dependency install just for one file) — instead I verified by cross-…; chunk 8: did not execute tui-capture.test.ts (review worktree has no node_modules ; skipped full dependency install) — assertions were instead verified by direct comp…. Not reviewed: reverse audit — stopped before round 5 by the review time budget.
中文说明
已审查。 建议见行内评论。 未审查：test-efficacy — harness control never ran (harnessValidated: null); mutant/hunk coverage unprobed (reverting the diff breaks the new tests' imports; 7 revert probes inconclusive)。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。 未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。 未探索到全部深度（达到工具调用预算）：chunk 7：could not execute npx vitest run src/commands/review/cleanup.test.ts — the worktree has no node_modules and no vitest binary exists on this machine; a full mo…；chunk 7：did not trace every internal branch of auditPrWrites exercised by the runCleanup('pr-8388') test — verified structurally (whole body wrapped in try/catch wi…；chunk 8：I did not run the new test file (no node_modules in the review worktree; I didn't do a full dependency install just for one file) — instead I verified by cross-…；chunk 8：did not execute tui-capture.test.ts (review worktree has no node_modules ; skipped full dependency install) — assertions were instead verified by direct comp…。 未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| ...(args.ready !== undefined ? { ready: args.ready } : {}), | ||
| ...(args.until !== undefined ? { until: args.until } : {}), |
There was a problem hiding this comment.
[Suggestion] The manifest records the --until/--ready pattern and settledBy: 'until-match' but not that matching ran on the LOGICAL view (captureText: -p -J, wraps joined, escapes stripped) while the .ans saves the PHYSICAL frame (-e -N, wraps unjoined) — a legally settled marker can be provably absent from the evidence file with nothing recording the view split. — Failure scenario: probe-reproduced on tmux 3.4 through the unmodified command (3-arm, control-proven): an SGR-split marker settles until-match while the .ans holds the escape-split bytes, and a 60-char marker spanning a wrap settles with the marker split across two .ans lines — grepping the evidence for the recorded until value finds nothing in both arms, while a plain control marker IS found; a reproducer following the manifest concludes the settle was fabricated and discards valid evidence. CaptureManifest's own contract: the manifest is the capture's ONLY record, and a reproducer that does not know its inputs judges honest evidence unreproducible. — Suggested fix: record the match view when a marker governed the settle (type + builder, two files):
// CaptureManifest (lib/tui-capture.ts):
matchedOn?: string;
// manifest builder here, after the until/ready spreads:
...(args.until !== undefined || args.ready !== undefined
? {
matchedOn:
'marker matched on the logical view (wraps joined, escapes stripped); the .ans is the physical frame',
}
: {}),中文说明
[Suggestion] manifest 记录了 --until/--ready 模式与 settledBy: 'until-match'，却没有记录匹配发生在逻辑视图（captureText：-p -J，折行已合并、转义已剥离）上，而 .ans 保存的是物理帧（-e -N，折行未合并）——一个合法 settle 的标记可能在证据文件中可证明地缺失，而 manifest 没有任何字段说明两个视图的差异。— 失败场景：已在 tmux 3.4 上通过未修改的命令实测复现（三臂、带对照）：SGR 拆分的标记 settle 为 until-match，但 .ans 中是转义拆分后的字节；60 字符跨折行的标记 settle 后在 .ans 中被拆成两行——两臂中对 .ans 按记录的 until 值 grep 均为空，而纯文本对照标记可以找到；按 manifest 复现的人会得出「settle 系伪造」的结论并丢弃有效证据。CaptureManifest 自身的契约：manifest 是捕获的唯一记录，不知道其输入的复现者会把诚实的证据判为不可复现。— 建议修复：当标记主导了 settle 时记录匹配视图（见上方代码：CaptureManifest 增加 matchedOn? 字段，manifest 构造处在 until/ready 展开后补入说明）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| for (const path of [ansPath, manifestPath]) { | ||
| if (!occupied(path)) continue; | ||
| refuse( |
There was a problem hiding this comment.
[Suggestion] A run SIGKILL'd/OOM'd between the .ans write and the manifest write strands an undescribed .ans that permanently refuses every later capture against the same --out: the clear phase only removes artifacts beside a parseable capture manifest, so with no manifest nothing is cleared and this collision gate then refuses the documented same---out reuse shape until manual deletion. — Failure scenario: probe-reproduced at head — SIGKILL during the freeze render window leaves ans=true manifest=false; the same---out retry exits 3 with --out collides with a file this capture did not write: <out>.ans … Pick another --out, self-perpetuating (nothing clears the .ans without a manifest; the brief's --out template is deterministic per finding-id/cols, and the Step-9 prefix sweep reclaims it only at review end). Trigger refinement (probe-verified): catchable signals (TERM/INT/HUP/QUIT) cannot strand artifacts — the synchronous tail drains them after the manifest write — the live triggers are SIGKILL/OOM and TERM→grace→KILL harness escalation against a process blocked up to 30s in the freeze render. Fail-closed (blocks producing evidence, never produces wrong evidence), hence Suggestion. — Suggested fix: write a provisional signature-bearing manifest immediately after the .ans write, rewritten with the final ladder after the render:
// right after writeFileSync(ansPath, …):
writeFileSync(
manifestPath,
`${JSON.stringify({ ansPath, pngPath, evidence: 'ans-only', settledBy: 'fixed-delay' }, null, 2)}\n`,
'utf8',
);
// …the final manifest write below then overwrites it with the full ladder.Flip-verified: with the provisional manifest the identical SIGKILL probe's retry succeeds (exit 0, evidence png) instead of refusing; pre-declaring pngPath also covers a kill after the png write (otherwise the png strands but only degrades the retry to ans-only, since the png is not in this gate).
中文说明
[Suggestion] 在 .ans 写出与 manifest 写出之间被 SIGKILL/OOM 杀死的运行会残留一个无描述的 .ans，并让之后所有针对同一 --out 的捕获被永久拒绝：清理阶段只在存在可解析的捕获 manifest 时才删除产物，没有 manifest 就什么都不清理，于是本冲突门拒绝文档中「复用同一 --out」的用法，直到手工删除。— 失败场景：已在被审 head 实测复现——freeze 渲染窗口内 SIGKILL 后 ans=true manifest=false；同一 --out 重试以 3 退出并报 --out collides with a file this capture did not write: <out>.ans … Pick another --out，且自我延续（无 manifest 则无人清理该 .ans；brief 的 --out 模板按 finding-id/cols 确定性生成，Step-9 前缀清扫只在评审收尾运行）。触发条件 refinement（已实测）：可捕获信号（TERM/INT/HUP/QUIT）不会造成残留——同步尾部会在 manifest 写出后才排空它们——真正的触发是 SIGKILL/OOM，以及 harness 对阻塞在 freeze 渲染中（最长 30s）的进程做 TERM→宽限→KILL 升级。失败关闭（只阻断产生证据，绝不产生错误证据），故为 Suggestion。— 建议修复：在 .ans 写出后立即写一个带签名的临时 manifest，渲染结束后用完整阶梯覆写（见上方代码；预先声明 pngPath 还能覆盖 png 写出后被杀的情形——否则残留的 png 只会把重试降级为 ans-only，因为 png 不在本门内）。翻转已验证：加上临时 manifest 后同一 SIGKILL 探针的重试成功（exit 0，evidence png）而非拒绝。
— qwen3.8-max via Qwen Code /review (v0.21.8)
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): chunk 7: could not execute npx vitest run src/commands/review/cleanup.test.ts — node_modules absent in worktree and parent checkout; a monorepo npm install was not r…; chunk 12: ran the new test (node_modules absent in the review worktree) — verified against implementation code instead. Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
已审查。 建议见行内评论。 未探索到全部深度（达到工具调用预算）：chunk 7：could not execute npx vitest run src/commands/review/cleanup.test.ts — node_modules absent in worktree and parent checkout; a monorepo npm install was not r…；chunk 12：ran the new test (node_modules absent in the review worktree) — verified against implementation code instead。 未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| ['settle bound', { settleMs: -1 }, '--settle-ms'], | ||
| ] as ReadonlyArray<readonly [string, Record<string, unknown>, string]>) { |
There was a problem hiding this comment.
[Suggestion] R16-2: The clear-first family loop's comment claims to pin "the settle/timeout bounds", but the loop exercises only --settle-ms; the --timeout-ms bounds gate has no seeded-artifact ordering pin. — Failure scenario: a refactor hoisting the --timeout-ms check above the clear block leaves a previous run's "evidence":"png" manifest beside the refusal JSON on a reused --out with a typo'd timeout — the exact wrong-evidence outcome this suite's clear-first contract exists to prevent. Probe-verified: the hoist ships the full suite 106/106 green; a seeded pin flips it.
| ['settle bound', { settleMs: -1 }, '--settle-ms'], | |
| ] as ReadonlyArray<readonly [string, Record<string, unknown>, string]>) { | |
| ['settle bound', { settleMs: -1 }, '--settle-ms'], | |
| ['timeout bound', { timeoutMs: 3_600_001 }, '--timeout-ms'], | |
| ] as ReadonlyArray<readonly [string, Record<string, unknown>, string]>) { |
中文说明
[Suggestion] clear-first 族循环的注释声称钉住「settle/timeout 边界」，但循环只演练了 --settle-ms；--timeout-ms 边界门没有带预置产物的排序钉。— 失败场景：把 --timeout-ms 检查提升到 clear 块之上的重构，会在复用 --out 且 timeout 拼错时，让上一次运行的 "evidence":"png" manifest 残留在拒绝 JSON 旁——正是本套件 clear-first 契约要防止的错误证据结局。已实测：该提升下全套 106/106 仍绿；加上预置钉即翻转。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| * tests iterate the REAL list. */ | ||
| /** A capture manifest is a few hundred bytes; anything past this is not | ||
| * one, and reading it would cost more than refusing does. */ | ||
| const MAX_MANIFEST_BYTES = 1024 * 1024; |
There was a problem hiding this comment.
[Suggestion] R16-3: The doc comment for REAP_SIGNALS is orphaned: MAX_MANIFEST_BYTES and its own doc block were inserted between the comment and the declaration, so REAP_SIGNALS ships with no doc comment and the signals rationale attaches to the wrong symbol; the same block's SIGHUP clause reads as spliced mid-sentence at the colon. — Failure scenario: IDE quick-info/typedoc render the rationale on the wrong symbol or drop it, and a maintainer editing the signals set — the constant whose comment says a dropped entry must fail a test — finds no rationale attached. Move MAX_MANIFEST_BYTES together with its own doc comment above the signals doc block so the signals block is immediately adjacent to export const REAP_SIGNALS, and restore the spliced sentence.
中文说明
[Suggestion] REAP_SIGNALS 的文档注释被孤立：MAX_MANIFEST_BYTES 及其自身的文档块被插在该注释与声明之间，导致 REAP_SIGNALS 没有任何文档注释，信号设计理由被挂到错误的符号上；同一块中关于 SIGHUP 从句读起来像是在冒号处被中途拼接。— 失败场景：IDE quick-info/typedoc 会把该理由渲染到错误符号上或直接丢弃；而维护者修改信号集合时（该常量的注释明确说漏项必须让测试失败）找不到附着的理由。修复：把 MAX_MANIFEST_BYTES 连同其自身文档注释移到信号文档块之上，使信号块紧邻 export const REAP_SIGNALS，并恢复被拼接的句子。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| degradations.push( | ||
| `--until never matched within ${untilPolledMs}ms of the ` + | ||
| `${args.timeoutMs}ms budget — late frame captured`, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R16-8: The actual-polled-window number (untilPolledMs) is never asserted; every test touching this degradation only checks toContain('--until never matched'). — Failure scenario: probe-verified — replacing ${untilPolledMs} with ${args.timeoutMs} ships the full suite 106/106 green; a numeric probe on the shared-deadline shape fails on the mutant (expected 2500 to be less than 2000) and passes pristine. The mutant manifest claims the marker was searched for the full budget when the ready gate consumed most of it — the exact overstatement the production comment above says it avoids. Fix: in the shared-deadline test, parse the number out of degradedBecause and assert it is materially less than timeoutMs.
中文说明
[Suggestion] 实际轮询窗口数值（untilPolledMs）从未被断言；所有触及该降级串的测试只检查 toContain('--until never matched')。— 失败场景：已实测——把 ${untilPolledMs} 替换为 ${args.timeoutMs} 后全套 106/106 仍绿；对共享期限形态的数值探针在变异体上失败（expected 2500 to be less than 2000），原始代码通过。变异后的 manifest 会声称标记被搜索了整个预算时长——而 ready 门其实消耗了大部分预算，正是上方生产注释明确要避免的夸大。修复：在共享期限测试中从 degradedBecause 解析出该数值，断言它显著小于 timeoutMs。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (code && code !== 'ENOENT') { | ||
| return { status: 'hung', code, spawned: false }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R16-9: probeOutput's hung-with-code variants (non-ENOENT spawn error; non-zero exit; signal death — spawned true/false) have no test pairing, and neither do the corresponding could not be probed (...) refusal/degradation messages. — Failure scenario: probe-verified — collapsing all three branches into return { status: 'absent' } ships 106/106 green; PATH-shim probes (exit-2, self-kill, non-executable) go 4/4 red on the mutant and green pristine, with the refusal literally printing "not installed" for an installed binary that merely exited 2 — the measured fd-exhaustion misreport the comment names. Fix: drive the seams with richer probe results and assert the reason distinguishes "could not spawn" from "ran and failed" and never contains "not installed".
中文说明
[Suggestion] probeOutput 的带 code 的 hung 变体（非 ENOENT 的 spawn 错误；非零退出；被信号杀死——spawned 为 true/false）没有任何测试配对，对应的 could not be probed (...) 拒绝/降级消息同样没有。— 失败场景：已实测——把三个分支全部折叠为 return { status: 'absent' } 后 106/106 仍绿；用 PATH shim 探针（exit 2、自杀、不可执行）在变异体上 4/4 变红、原始代码全绿，且拒绝消息会对一个仅仅退出码为 2 的已安装二进制打印 "not installed"——正是注释点名的实测 fd 耗尽误报。修复：用更丰富的探针结果驱动接缝，断言理由能区分 "could not spawn" 与 "ran and failed" 且从不含 "not installed"。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // rethrowing it exited 1 with the .ans and manifest both on disk. Before | ||
| // that point a stdio fault still propagates: it can mean the refusal | ||
| // itself never reached anyone. | ||
| if (err.code !== 'EPIPE' && !artifactsComplete) throw err; |
There was a problem hiding this comment.
[Suggestion] R16-10: The artifactsComplete half of the broken-pipe guard (non-EPIPE stdio errors swallowed after evidence is complete) has no test; only the EPIPE path is pinned (by the child-process EPIPE test). — Failure scenario: probe-verified — deleting && !artifactsComplete ships the full suite 106/106 green; a probe emitting a non-EPIPE ENOSPC 'error' event after a fully successful capture throws on the mutant (exit 1 — a successful capture reported as failed to the machine-reading consumer) and passes pristine. The guard's own comment documents this exact shape as measured. Fix: add a test that completes a real capture while a stdio spy throws a non-EPIPE ENOSPC on the summary write, and assert process.exitCode stays undefined and the artifacts remain described.
中文说明
[Suggestion] 断管守卫的 artifactsComplete 半边（证据完成后吞掉非 EPIPE 的 stdio 错误）没有测试；只有 EPIPE 路径被子进程 EPIPE 测试钉住。— 失败场景：已实测——删除 && !artifactsComplete 后全套 106/106 仍绿；在完全成功的捕获之后发出非 EPIPE 的 ENOSPC 'error' 事件的探针在变异体上抛错（exit 1——成功的捕获被当作失败报给机器读取方），原始代码通过。守卫自身的注释明确记载该形态是实测所得。修复：新增测试，在真实捕获完成的同时让 stdio 探针在摘要写出时抛出非 EPIPE 的 ENOSPC，断言 process.exitCode 保持 undefined 且产物仍被完整描述。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| * successful capture as a failed command. */ | ||
| let artifactsComplete = false; |
There was a problem hiding this comment.
[Suggestion] R16-13: Second instance of the R16-3 doc-splice pattern: the guardBrokenPipes rationale block ("The contract writes … Swallow EPIPE only; anything else stays loud") is orphaned from its function by the artifactsComplete doc + declaration inserted between them — both JSDoc blocks sit adjacent directly above let artifactsComplete = false;. — Failure scenario: tooling attaches the swallow-policy rationale to the boolean flag or leaves it floating, while guardBrokenPipes() — the function implementing that EPIPE contract — carries no doc; a maintainer changing the swallow policy (e.g. removing the !artifactsComplete half that R16-10 shows is unpinned) finds no rationale attached. Fixing R16-3's site does not fix this one. Fix: move "The contract writes …" block down to directly precede function guardBrokenPipes(): void {.
中文说明
[Suggestion] R16-3 文档错位模式的第二处：guardBrokenPipes 的理由块（"The contract writes … Swallow EPIPE only; anything else stays loud"）被插在中间的 artifactsComplete 文档+声明从其函数上孤立——两个 JSDoc 块紧邻地悬在 let artifactsComplete = false; 上方。— 失败场景：工具会把吞错策略的理由挂到布尔标志上或悬空，而实现该 EPIPE 契约的 guardBrokenPipes() 没有任何文档；维护者修改吞错策略（例如删除 R16-10 显示未被钉住的 !artifactsComplete 半边）时找不到附着的理由。修复 R16-3 的位置不会修复此处。修复：把 "The contract writes …" 块下移至紧邻 function guardBrokenPipes(): void { 之前。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // Something WAS cleaned, so the nothing-to-clean claim must not print. | ||
| expect(mocks.writeStdoutLine).not.toHaveBeenCalledWith( | ||
| expect.stringContaining('Nothing to clean'), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R16-14: The suite pins the ABSENCE of the "Nothing to clean" claim four different ways but never its EMISSION — no test anywhere asserts the clean-run output at cleanup.ts:661 is produced. — Failure scenario: a refactor deleting or narrowing the final if (!removedAny && !failedAny && !sweepFailed) writeStdoutLine(…) block ships green: every orphan-block test asserts only the negative, and every outer runCleanup('pr-…') test sets removedAny or failedAny, so no test reaches the emission. The common clean-host run then exits with no stdout at all, silently dropping the contract this block's own comment invokes ("stdout is the one a script reads"). Fix: add one test where no sockets exist and assert expect(mocks.writeStdoutLine).toHaveBeenCalledWith('Nothing to clean for target "local".').
中文说明
[Suggestion] 本套件用四种不同方式钉住了 "Nothing to clean" 声明的「不出现」，却从未钉住它的「出现」——全部测试中没有任何一处断言 cleanup.ts:661 的干净运行输出被产生。— 失败场景：删除或收窄最后那个 if (!removedAny && !failedAny && !sweepFailed) writeStdoutLine(…) 块的重构可以绿着通过：孤儿块的测试只断言否定面，外层 runCleanup('pr-…') 测试都会置位 removedAny 或 failedAny，没有任何测试能走到该输出。常见的干净主机运行将零 stdout 退出，悄悄丢掉本块注释自己引用的契约（"stdout is the one a script reads"）。修复：新增一个无 socket 的测试，断言 expect(mocks.writeStdoutLine).toHaveBeenCalledWith('Nothing to clean for target "local".')。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| it('captures anyway on --until timeout and records the degraded settle', async () => { | ||
| await run({ until: 'NEVER-APPEARS', timeoutMs: 1500, settleMs: 0 }); | ||
| expect(process.exitCode).toBeUndefined(); |
There was a problem hiding this comment.
[Suggestion] R16-15: The --until poll's actual waiting on the timeout shape has no wall-clock lower-bound pin anywhere in the suite — every sibling wait has one (fixed-delay floors at ~2087/~2119 with a comment naming the exact mutant class), so a cap that ends the poll early ships green. — Failure scenario: probe-verified — an iteration-cap mutant (break after 4 unmatched polls) passes all 14 until/deadline/ready-poll/backtracking tests; adding a sibling-style floor flips it (expected 1916.77 to be greater than or equal to 2900) while pristine passes. In production, --until MARKER against a TUI rendering its marker at t≈2s settles at ≈1s as settledBy:'timeout' with a pre-marker frame — the false-settle shape --until exists to prevent. Distinct from R16-8 (the unasserted number in the message): here the poll never runs the window the message will claim. Fix: in this test, time run() and assert elapsed ≥ ~1400 for the 1500ms budget.
中文说明
[Suggestion] --until 轮询在超时形态下的真实等待没有任何墙钟下界钉——套件中每个兄弟等待都有（固定延迟在约 2087/2119 行有下界，注释点名了确切的变异类），因此提前结束轮询的上限可以绿着通过。— 失败场景：已实测——迭代上限变异体（4 次未匹配即 break）通过全部 14 个 until/deadline/ready 轮询/回溯测试；加上兄弟风格的下界即翻转（expected 1916.77 to be greater than or equal to 2900），原始代码通过。生产环境中，--until MARKER 面对 t≈2 秒才渲染标记的 TUI 会在约 1 秒就以 settledBy:'timeout' 落定并携带标记出现前的帧——正是 --until 要防止的假落定形态。与 R16-8（消息中数值未被断言）不同：此处轮询根本没有运行消息将要声称的那个窗口。修复：在本测试中给 run() 计时，对 1500ms 预算断言 elapsed ≥ 约 1400。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| manifestHadPng = | ||
| shaped && (m.evidence === 'png' || typeof m.pngPath === 'string'); |
There was a problem hiding this comment.
[Suggestion] R16-16: The sparing half of the png-clear rule — a shaped ans-only manifest does NOT authorize clearing <out>.png — has no test pairing; every seeded-manifest test in the suite uses {"evidence":"png"}. — Failure scenario: probe-verified both arms — the mutant manifestHadPng = shaped ships the full suite 106/106 green, while a dedicated probe goes red: run 1 ans-only spares the user's occupied <out>.png; run 2 against the same --out parses the ans-only manifest, and the mutant's clear phase unlinks the user's spared png with no degradation recording it — the destruction this rule's own comment names as measured. Fix: add a clear-phase test seeding '{"evidence":"ans-only","pngPath":null}' at cap.json plus a user file at cap.png, asserting cap.png survives byte-for-byte while cap.ans/cap.json are cleared.
中文说明
[Suggestion] png 清除规则的「保留半边」——形态完整的 ans-only manifest 不授权清除 <out>.png——没有测试配对；套件中所有预置 manifest 的测试都用 {"evidence":"png"}。— 失败场景：双臂实测——变异体 manifestHadPng = shaped 下全套 106/106 仍绿，而专用探针变红：第 1 次运行 ans-only 保留了用户占据的 <out>.png；第 2 次运行复用同一 --out 解析该 ans-only manifest，变异体的 clear 阶段会删掉被保留的用户 png 且没有任何降级记录——正是本规则注释记载为实测过的破坏。修复：新增 clear 阶段测试，预置 '{"evidence":"ans-only","pngPath":null}' 于 cap.json 并在 cap.png 放一个用户文件，断言 cap.png 逐字节幸存而 cap.ans/cap.json 被清除。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| `--out collides with a file this capture did not write: ${path}. ` + | ||
| "A previous capture's own artifacts are cleared automatically; " + | ||
| 'this one is not ours to replace. Pick another --out.', |
There was a problem hiding this comment.
[Suggestion] R16-17: The collision gate's message states as fact that any occupant is foreign ("this capture did not write … not ours to replace"), but a previous run of this same tool SIGKILL'd in the window between the .ans write and the manifest write (up to ~40s: freeze probe 10s belt + render 30s belt; SIGKILL'd runs are inside this file's own named threat model) leaves its OWN .ans at the path with no manifest beside it — and the preamble comment's "A previous capture's OWN artifacts never reach here" is false for this state. — Failure scenario: run 2 against the same --out — the documented reuse shape — refuses asserting a non-ownership the tool cannot have established (false in this canonical shape), claims auto-clearing that did not happen, and points the consuming agent at a new --out instead of the real remedy (removing a stranded artifact of a killed run); the same---out flow stays wedged until manual deletion. Keep the conservative spare-and-refuse behavior; fix the message to name the epistemic state:
| `--out collides with a file this capture did not write: ${path}. ` + | |
| "A previous capture's own artifacts are cleared automatically; " + | |
| 'this one is not ours to replace. Pick another --out.', | |
| `--out collides with an occupant this capture cannot verify as its own: ${path}. ` + | |
| 'No manifest beside it proves it is a previous capture\'s artifact, and ' + | |
| 'unverified occupants are never deleted. Remove it to reuse this --out, or pick another --out.', |
中文说明
[Suggestion] 碰撞门消息把任何占用者都当作外来文件断言（"this capture did not write … not ours to replace"），但本工具自身的上一次运行若在 .ans 写出与 manifest 写出之间的窗口（最长约 40 秒：freeze 探针 10 秒保险丝 + 渲染 30 秒保险丝；SIGKILL 的运行在本文件的威胁模型内）被 SIGKILL，会在该路径留下自己的 .ans 且旁边没有 manifest——而前言注释「上一次捕获自己的产物永远不会到达这里」对该状态是假的。— 失败场景：对同一 --out 的第 2 次运行（文档化的复用形态）拒绝时断言了工具无法确认的非所有权（该典型形态下为假），声称并未发生的自动清理，并把消费方 agent 指向新的 --out 而非真正的补救（移除被杀运行遗留的产物）；同一 --out 流程会卡死直到手动删除。保留保守的「保留并拒绝」行为；把消息改为描述认知状态（见上方建议代码）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
* fix(review): make the posted review body readable The posted body on a capped COMMENT joined every clause with spaces: on QwenLM#8388 that ran 31 unresolved existing Criticals and seven disclosures into one paragraph, each entry restating the same truncation reason, every comment id a bare number, and the Chinese fold duplicating the whole untranslated wall. - Render the COMMENT body as paragraphs: the short verdict sentences stay one opener paragraph, every disclosure gets its own (matching the REQUEST_CHANGES branch). - Render the unresolved-Critical block as a Markdown list, grouping entries that share the exact reason after their first " — " so the reason is stated once over a sub-list of subjects. Nothing is dropped: every subject and every distinct reason still renders. - Linkify "comment <id>" / "issue-level comment <id>" in those entries to their #discussion_r / #issuecomment anchors when the plan names the PR. - The Chinese fold no longer duplicates the untranslated English list; it carries the count and points at the English half. - budgetGapDisclosures: a parenthesized placeholder ("(none — all planned checks completed)") now reads as a placeholder — the leading "(" defeated the leading-token match, and QwenLM#8388's body disclosed it as a self-contradictory "Not explored to full depth" entry. * fix(review): harden the readable review body per review feedback - budget disclosures: the paren strip now drops only the bare placeholder and the completion idiom, so a real gap that merely starts with a placeholder token ("(none of the chunk-2 checks ran — the runner died)") survives instead of being silently dropped before the coverage gate; duplicate folding keys on the paren-stripped text so a gap restated with and without parens discloses once - compose-review: one shared plan-identity rule for the bilingual recovery and the comment anchors (the two copies had diverged); anchors follow the plan's recorded GHE host with a relaxed id floor off github.com; cannot-tell entries flatten embedded newlines and a dangling " — " reads as reasonless instead of an empty group key - tests pin all of the above, including the previously untested already-linked passthrough, the reasonless shape, and the bilingual fold count beyond N=1 * fix(review): keep genuine budget gaps and harden comment anchors - budget disclosures: the completion idiom now requires an actual completion predicate the text ends with — a head word alone ("none — all 5 Windows checks failed to start") no longer reads as done — and the template/dash alternatives are end-anchored, so an inner text merely starting with "<…>" or a dash run keeps. One strict classifier judges the paren-stripped text, bare and wrapped alike, ending the divergent two-parser treatment of identical content ("none of the chunk-2 checks ran — the runner died" was kept wrapped, dropped bare). - budget parse linearity: the placeholder classifier, the gap-line regex, and the trailing-punctuation strips no longer carry overlapping quantifiers — a token followed by a long whitespace run was measured quadratic (seconds at 40k spaces); every shape is now linear by construction and pinned by the linearity test. - comment anchors: the effective host is normalized (case and the implicit :443) before the github.com short-id floor comparison; "issue-level" reads case-insensitively, matching the pipeline's own capitalized label; a plan ownerRepo carrying "."/".." segments is rejected before it rides into a posted URL. - tests: the plan fixture carries the PR identity instead of four pasted read/modify/write backdate dances; the anchor suite is hermetic against an exported GH_HOST and now pins the host-fallback, host-rejection, ownerRepo-rejection, routed-host, prefixed-cannot- tell, and cased-Issue-level branches; the Chinese fold assertion covers the pointer, not just the count. * fix(review): address round-3 review findings on gap classification and anchors - End-anchor the stayed-under-budget idiom in PLACEHOLDER_GAP_RE: text continuing past "budget" discloses skipped work and must keep (R3-1). - Require the completion word to be affirmed — a negated tail ("none completed", "nothing finished") is a failure report, not completion (R3-2) — and temper the completion spans so an exception quantifier ("all but X completed") keeps the gap (R3-3). - Close the drop-side gaps: "everything" as a completion head, the found / to-report non-answer idioms, and inner paren padding (R3-5); pin the whole drop vocabulary and the new keep shapes (R3-6). - Collapse multi-line cannot-tell entries by split/join and guard stripReviewFooter on the footer marker: both regex paths scanned quadratically on long whitespace runs in model-written entries with no length cap (R3-4; the footer strip measured ~20 s at 80k characters). - Decide the anchor family per entry, not per match: an issue-level entry echoing pr-context's own header shape carries its id apart from the phrase, and adjacency-routing minted #discussion_r anchors in the wrong id space; link text now keeps the entry's casing (R3-8). - Normalize www./trailing-dot/zero-padded-port variants of github.com before the short-id floor comparison (R3-9). - Lift the dot-segment-aware owner/repo rule into lib/gh.ts as isOwnerRepo — submit's --repo check and the plan identity now share one copy (R3-10) — and revert cleanup.ts's OWNER_REPO_RE export. - Pin the BUDGET_GAP_LINE_RE rewrite's linearity and bullet handling (R3-7), the corrupt-plan degradation (R3-11), and the numeric prNumber arm of the plan identity (R3-12). * test(review): pin the gap-line linearity gate with a 160k-space probe --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
|
Closing in favour of #8894 — the same change, recreated to collapse 373 commits into a single one and rebase onto current Nothing was dropped: the 14 changed files are byte-identical to this branch's head for every file This thread stays open for reference rather than being deleted: nineteen review rounds happened here, and the reasoning behind several decisions — why 中文说明以 #8894 取代本 PR：同一改动，压缩为单个 commit 并 rebase 到当前 没有丢失任何内容：14 个改动文件中，凡 本 PR 保留存档而非删除：这里进行了十九轮评审，若干决定的推理过程——为什么 |
…wenLM#8792) * ci: install tmux and zip tooling on the Linux test lane, and pin it Two suites go quiet on a runner without their tooling. capture-tui's real-tmux block is `describe.skipIf(!hasTmux)`-gated, so on the hosted ubuntu image — which ships no tmux — every real-tmux behavior skips inside a green required check. The install-script packaging suite needs zip and unzip the same way. This installs both where apt exists, bounded (timeout-minutes) and advisory (continue-on-error absorbs the bound), so a stalled mirror or dpkg lock neither hangs the job toward its 60-minute cap nor reds the check. A step that silently stops installing is the same outage as no step at all, so it comes with a pin that reads the workflow the way bash does rather than the way it looks. The parser is quote-aware (a `#` inside a message is literal; a `;` inside one does not split statements), continuation-aware (only an odd run of trailing backslashes continues a line, and a comment never continues), and wrapper-aware (an apt-get behind sudo/env/nice/ timeout/time/nohup, with or without their options, or inside a subshell, is still an apt-get). On top of that it pins what actually keeps the step working: the if-condition whole (a one-character `&&`→`||` took the already-installed branch on a lane with zip but no tmux), the elif whole (a near-miss falsifier like `false2 &&` killed the install branch), the else fallback (deleting it removed the only signal on a lane with neither tmux nor sudo), exact package tokens (`powerline-tmux` is not tmux), the flags that make it work unattended (-y, sudo) and the ones that make it a no-op (-s, --download-only and friends exit 0 having installed nothing), that nothing may hard-fail the step in any errexit spelling or through a subshell exit, that every apt-get REACHES a `|| echo` guard (an AND-list is transparent, a pipeline or a `;` is not), and that every annotation reaches stdout unredirected with `::warning::` at line start — a workflow command the runner cannot see is just log noise. Split out of QwenLM#8388, whose capture-tui command is what made the tmux half load-bearing. Each pin was mutation-verified against this workflow file. * test(ci): close the pin's own escapes, and give its bash model tests Thirteen findings on this PR, all against the pin rather than the step, and two of them were the pin being WRONG rather than weak — those matter most, because a pin that reds a legitimate edit is worse than one that misses. Wrong: the redirect check matched `>` anywhere in an echo statement, including inside the quoted message, so a semantics-preserving reword was rejected — exactly what this file says it must not pin. And the sudo requirement demanded literal `sudo apt-get` adjacency, which rejects `sudo -n apt-get`, the sibling workflow's own convention. Both now ask the question at the right level: quoted spans are blanked before any question about shell syntax, and "runs through sudo" is asked of the raw statement. Weak, each mutation-verified against ci.yml: `&>` was split as a separator, so `echo '::warning::…' &> /dev/null` tore into a clean echo and an unchecked redirect; apt's `-o APT::Get::Simulate=true` reached simulate mode through a door the token blacklist never saw; the package pin matched the whole logical line, so a package dropped from the install survived if its token appeared in the guard's message; `apt-get update` was required by nothing; `-y` and sudo were checked only on the FIRST install statement, and nothing kept a foreign command off the install's chain to short-circuit it; the step's `shell:` key — the one key that decides which interpreter parses the entire block — was unpinned; the closing `fi` was unpinned, so deleting it passed 5/5 while bash rejects the block and the step runs nothing; and the else branch's annotation was a substring check unanchored to any emitting command, so `: '::warning::…'` muted it. The ~180 lines of bash-semantics helpers are the ORACLE every pin reasons through, and they had no tests of their own. They do now — a table per rule, each rule traceable to a mutation that escaped an earlier version. Writing them corrected one of my own assumptions: `echo a \` + newline + `b` joins to `echo a b`, not `echo ab`, because the space before the backslash survives. Also corrected the rationale in both files: they asserted in the present tense that a real-tmux suite exists in this tree. It does not — it arrives with QwenLM#8388, and the tmux half of this step is deliberately pre-landed for it. Saying so is the honest version. * test(ci): close the pin's remaining escapes, and finish its bash-model fixtures Four gaps survived the first pass, each probe-verified against the suite before the fix: - an always-failing `apt-get install` prefixed in the same && chain short-circuited the real install with every pin green, because every per-statement pin binds to a statement on the line and none sees the skip — now the step must carry exactly one install statement; - `apt-get update` could be deleted or moved behind the install — now it must exist AND precede the install in statement order; - `return` and `exec` were missing from the hard-fail blacklist, the same family as `exit`/`false` under the runner's `bash -e`: either leaves the required check green with nothing installed and not even the else-branch warning emitted (sandboxed verification's F1) — now blacklisted, with an end-to-end splice fixture pinning the axis; - the guard walker's operator alphabet still split on the `&` of `&>` although statementsOf no longer does, reddening a guarded `apt-get … &> /dev/null || echo` — both now share the exemption. Also finished the bash-model fixtures round one asked for: a quote spanning a newline, and `timeout -k 1 5 sudo …` wrapper stripping. Writing them exposed one oracle bug the round's own failure scenario had measured: `set -- -e` assigns positional parameters, it does not enable errexit — `--` now ends option parsing for the check. Mutation battery against ci.yml: the five escape mutants (prefixed install, return 0, exec true, update deleted, update reordered) all flip red; the five tolerated forms (sudo -n sibling, dropped --no-install-recommends, wrapper-wrapped update, &> redirect with the guard intact, reworded warning) all stay green. * test(ci): close the escapes a real edit would take, and stop there Seven of sixteen findings, chosen by one question: would an honest edit hit this? Each is mutation-verified against ci.yml. - apt's long option spellings reached simulate mode past a `-o`-anchored regex (`--option APT::Get::Simulate=true`, `--simulate=yes`), as did `--version`/`--help`, which print and exit having installed nothing. - The chain allowlist checked only a statement's leading word, so `&& sudo apt-get remove -y tmux` appended after the install passed while undoing it. It checks the subcommand now. - `apt-get update` was pinned as load-bearing but never required to run through sudo, though that is the whole reason the install is. - A quoted redirect target (`>'/dev/null'`) muted an annotation: blanking quoted spans leaves the operator, so the pin matches the operator itself. - The already-installed branch's BODY was unpinned — emptying it to `:` left a broken-but-installed tmux undetected on the lane that takes it. - The step was pinned before the test step but not AFTER the `ci_profile` step its `if:` reads; above it the condition is always false and the step silently never runs. - The separator alphabet was encoded three times and the copies had already drifted. One definition now, used by all three consumers: a model of bash that depends on which function you ask is not a model. The nine I did not take are recorded on their threads with reasons. Most are adversarial rewrites — swapping branch bodies, wrapping the block in a never-executing loop, nesting a conditional inside a pinned branch, hiding the chain in a heredoc — and this file cannot win that game: anyone who can restructure the step can also delete it. It guards accidental drift, which is the failure that actually happens. One asks for a composite action shared with qwen-autofix.yml; that is a repo-wide refactor and does not belong in this PR. * ci: make the install fail fast, bound its apt calls, and verify all three tools Four review findings, all about the step rather than its pin, and all four taken — they are the shapes a real runner produces. `sudo` becomes `sudo -n`, the convention nine other steps in this file and the sibling workflow already use: without it, a runner without passwordless sudo gets a password PROMPT on stdin and the step hangs to its timeout instead of falling through to the warning. The `elif` tests sudo's USABILITY (`sudo -n true`) rather than its presence: `command -v sudo` passes on a runner where sudo exists and cannot be used, which takes the install branch straight into that hang. The apt calls carry their own `timeout 280` inside the step's 5-minute bound. When the STEP timeout fires there is no `|| echo` — the lane loses its tooling and says nothing. A shorter command-level bound lets the guard run and the annotation reach the check UI. The already-installed branch verifies zip and unzip too, not just tmux: it is taken when all three are present, so a broken-but-present zip failed the packaging suite with no warning explaining why. The pins moved with the step: "runs through sudo" now walks the wrapper prefix (a bounded install is `timeout 280 sudo -n apt-get …`, and requiring sudo FIRST would have redded it), the elif equality carries the new condition, and all three tool probes are pinned to exist AND to be guarded. That last pin needed a second pass: matching by substring reported the zip probe present after deletion, because `unzip -v` contains `zip -v` — it is word-anchored now, and each of the three deletions turns it red. * fix(ci): install the tooling on root lanes too, where sudo does not exist The Test check reds on a lane that ships no zip: install-script.test.js throws at module load on a CI host missing zip/unzip — loudly, by design — and this step could not prevent it on a root-container lane, because its only install branch required `sudo -n true`, and root lanes have no sudo (and need none). The step fell through to the advisory warning, nothing installed, and the packaging suite threw inside the required check. Add a root branch ahead of the sudo branch: uid 0 runs the same bounded, guarded chain as-is. Lane probes under the runner's exact bash flags: a root lane installs without sudo, a sudo lane installs through sudo -n, a bare lane warns and exits 0, and an apt failure still reaches the guard. The pins moved with the step: each branch must carry its own update-then-install chain with exactly one install, and the branch decides the sudo shape — the root chain must NOT reach for sudo (on a lane with no sudo binary that exits 127, the guard fires, nothing installs), the sudo chain must. Six mutants red, pristine green. * fix(ci): fit the apt bounds inside the step cap, and pin what still escaped * test(ci): close the pin's round-4 escapes, and stop reddening its rewordings Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(ci): close the pin's round-5 escapes with a branch-body allowlist --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
A verifier ruling on "the panel clips at 80 columns" reads the layout code and imagines a terminal; measured on this repo, the imagining is where rendering verdicts go wrong. `qwen review capture-tui` makes the terminal real and the evidence a file: it drives a command in a throwaway tmux and hands back what actually rendered — `.ans` pane text always, a `.png` when freeze is available, and a manifest saying which rung it reached and why. Isolation is structural, not disciplined. Every tmux call is scoped to a per-run private server (`-L`), so a capture cannot see — let alone resize or kill — the user's own sessions; the measured failure mode of desktop-automation verification was exactly "drives the user's own windows". `kill-server` reaps everything the capture started, a signal handler reaps before re-raising, and the pane holder carries its own watchdog so an unreaped server still self-terminates. Evidence is never invented. The manifest records the rung and the reason it stopped there, so a verdict can say what it stands on. A png is credited only when this run's render demonstrably produced it; artifact paths are stamped by identity, so nothing this run did not write is ever deleted or claimed; and a path already occupied by a foreign file is refused up front rather than overwritten. The trailing-space fidelity that column claims depend on follows the tmux version — `-N` where it means real spaces, `-T` where padding must be trimmed, neither where `-N` would fabricate them. Refusals are a contract: exit 3, a reason on stderr and the same reason as JSON on stdout, and no stale evidence left beside it. An environment that cannot produce evidence says so instead of producing something weaker and calling it the same thing. Recreated from #8388 to collapse 373 commits and rebase onto current main. The CI tooling step and its workflow pin are #8792; the shell-registry deflake is #8795.
4 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 (49 files / 1506 assertions locally; macOS with tmux 3.7b + freeze 0.2.2). 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.capture-tuiregistered in the pinned subcommand surface test.Linked Issues
None. Phase 2 of #8351; uses the
assetFiles/publish-assetsflow it introduced.中文说明
这个 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流程。