feat(review): capture-tui — rendering claims get pixels, not prose - #9273
feat(review): capture-tui — rendering claims get pixels, not prose#9273wenshao wants to merge 54 commits into
Conversation
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.
…honest until note Three of eighteen review findings were behaviour, not coverage. `process.exitCode` was per-process where everything else in a run is per run: `runCaptureTui` is exported and driven repeatedly in-process, so a refusal left exit 3 standing and the NEXT successful capture reported failure with its artifacts on disk. Probe-observed on the unmodified branch: refuse → 3, then a clean capture → still 3. It resets beside the completion flag now. `artifactsComplete` was armed one drain too late. The drain is the first event-loop turn after a long synchronous stretch, so an async stdio 'error' queued during that stretch — the reap WARNING is written there — dispatches inside the drain, found the flag false, and the broken-pipe guard rethrew: a completed capture exiting 1 with .ans and manifest both written. Armed before the drain now. When `--ready` times out the `--until` poll never runs, but the manifest recorded `until` and `settledBy: 'timeout'`, which reads as "searched and not found". Measured with the marker present in the pane for the entire run. It now says the search never happened. Also restored a test I deleted by accident: the stdio-failure pin went out in a round-18 bulk edit whose slice spanned it (present at 824d64e, gone at a1b5bed), which is why nothing caught the flag's arming point until this review. It is back, and rebuilt so it measures that point rather than the guard's mere existence — it queues the error on the reap warning, before the drain, and arming the flag after the drain turns it red.
Not more coverage — these six already passed while testing something other than what they claim, which is the failure mode that has hidden real defects in this work repeatedly. Two collision-gate tests carried a root skip left over from the write-probe design they replaced. The gate is occupancy — a pure lstat — so mode bits never enter it, and on a root lane (Docker images commonly are) both tests silently did not run. The FIFO pin returned green when `mkfifo` was absent: spawnSync does not throw for a missing binary, it hands back an ENOENT error object, so the bare `return` reported PASSED. It fails loudly now — the lane should say it cannot run this pin rather than drop it quietly. The stdin-shape fake asserted "some character device" where the property is /dev/null specifically: a pty is a character device and hangs freeze, a regular file sends it into file mode, and both satisfied the old check. It compares against a non-tty character device now, and piping stdin in production turns it red. The pre-capture stale-artifact test asserted exit 3 and cleared artifacts but never the reason, so deleting the marker-compile gate left it green — the run simply refused elsewhere. It names its gate now. And the shape-bounds family exercised only `--settle-ms` while its comment promised the timeout bound too; the missing case is there.
…ut of --out Two ways this command could destroy files it did not write, both probe-reproduced against the previous revision: The clear phase accepted any JSON at `<out>.json` whose `evidence` field read "png" or "ans-only" as proof that a previous run of this command wrote it, then deleted the .json, .ans and .png beside it. `evidence` is a field any report-shaped JSON can plausibly hold, so a user's own file authorized deleting their capture inputs — before the collision gate the run was already headed for could refuse. Ownership now requires the full signature a previous run actually writes: the evidence rung, an ansPath resolving to THIS run's .ans, and a settledBy from the closed set. Each rung is mutation-verified to redden the suite on its own. The holder's ready sentinel lived at `<out>.holder-ready`, inside the user's namespace and derived from their argument. It was unlinked unconditionally on every run; a directory at that name threw EISDIR and refused the capture outright. It now lives under the system temp dir, per-pid and random, so nothing at the old name is read, written or unlinked — the pre-unlink stays as a belt but is no longer load-bearing, and the three hazards its comment cited are gone at the source. Tests: the ordering pin whose forcing mechanism the move eliminated is retargeted at the invariant that replaced it (a user DIRECTORY at that name survives a refusal untouched), the fake tmux stubs recover the sentinel from the argv they are handed rather than guessing it from --out, and the assertions that checked `<out>.holder-ready` was absent — now vacuously true — assert instead that no sentinel leaks where it really lives. 112 pass; the one red file in this suite (lib/stale-bundle.test.ts) fails identically on origin/main.
…nd three test defects R2-3: an ans-only manifest — what a run that degraded its freeze rung writes — records no png of its own, so nothing at `<out>.png` is the next run's to clear. The `manifestHadPng` guard already said so; nothing tested it. Mutation-probed: dropping the condition shipped the whole suite green while a re-run against the same --out silently deleted a user's file at the png name. The new test seeds that exact shape and asserts the file survives byte-for-byte while the run degrades and says so; it reddens on the mutant. R2-7: the too-old-tmux refusal's defining property is that it refuses UP FRONT, before paying for a server start. Exit 3, the wording and the absent .ans are all location-invariant — a mutant moving the gate below plan.start stayed green while a real new-session ran the user's command first. The test now carries the same call-log shim the marker-gate test uses; against that mutant it reddens on the call-log assertion. R2-2: the signal test's 90s orphan guard was cleared immediately BEFORE `await disposition` — the one window it exists for. Against the dropped-re-raise mutant the test policies, the child never exits, vitest fails the test, and the already-cleared guard left a node process and its private tmux server running out a 60s capture on every red run. The guard now spans every wait, is cleared in a finally, and sits at 20s — below the test's own budget, or it can never fire. R2-8: that test was the only child-spawning test in the file without an explicit budget, running four sequential cold `node --import tsx` lifecycles under the default 15s. Measured at 18.4s under load; it now has 60s like its siblings. R1-5: two doc-comment blocks had drifted off their declarations — REAP_SIGNALS' rationale sat above MAX_MANIFEST_BYTES, and guardBrokenPipes' above `let artifactsComplete`. Reattached; no behaviour change.
…re ownership corners
The no-orphan wording was false for one class of command. Reproduced with
a discriminating control arm: a captured command running
`spawn('sleep', …, {detached: true}).unref()` left its grandchild alive
after a fully successful capture, while an attached child in the same
shape was reaped with the server. `kill-server` reaps the session, not a
process tree, and a daemonized descendant is in its own session with init
as its parent before the reap even runs — nothing portable reaches it,
and nothing links it back to the capture afterwards, so guessing at it
would kill the wrong pid. Rather than pretend otherwise, the four places
that claimed everything is reaped (the file header, the finally comment,
the plan's kill comment, the agent brief) now state the boundary and what
it means for the operator, and a two-arm test measures it in both
directions — the guarantee and the limit.
Four ownership/robustness corners, each probe-reproduced upstream and
each mutation-verified here except where noted:
- The signature compared `resolve(m.ansPath)`, so a foreign manifest with
a RELATIVE ansPath that resolved to this run's .ans passed and took all
three of the user's files. This tool always records the already-resolved
absolute path, so the relative form can only come from elsewhere: the
comparison is now strict.
- `manifestHadPng`'s `typeof m.pngPath === 'string'` disjunct was
unreachable for anything this writer produces (ans-only is paired with
`pngPath: null`) and, on an inconsistent manifest, cleared a foreign
`<out>.png` on an ans-only rung — against the guard's own invariant. The
evidence rung alone decides now.
- `statSync(pngPath)` was the only unguarded throwable fs call in the run;
a png vanishing mid-check escaped as an uncaught ENOENT (exit 1, no
contract JSON, a stack trace, both artifacts orphaned with no manifest).
Read defensively. The added test pins the degradation branch the guard
creates, NOT the TOCTOU itself — that needs fs fault injection, and a
faithful revert leaves the test green. Said so in the test.
- The render-window signal test spawned a 30s capture child with no kill
guard, so any assertion failing before its `child.kill()` orphaned the
node process and its private tmux server together. It now attaches the
exit listener before the wait, guards with SIGTERM (SIGKILL leaves the
server standing — the child's own handler is what reaps it), and carries
a 60s budget.
Also: two comments claimed `expect(value, message)` is banned by
vitest/valid-expect. It is not — the rule carves out a string message
argument, and this file uses the two-arg form elsewhere. Reworded both.
…maller defects Two ways a file could be destroyed or a write redirected AFTER the collision gate had already passed — the gate runs before the window, and the window lasts up to --timeout-ms (70 minutes at the cap): - A captured command that writes its own `<out>.json` had that file silently replaced, and the run reported success. Occupancy is now decided AGAIN at write time, against the same pre-window stamp. - A symlink planted at `<out>.ans` during the window redirected this run's bytes out of the --out base — the escape the lstat-based gate closes at check time, re-opened through the window. The artifact opens now carry O_NOFOLLOW. The refusal also had to stop short of its own cleanup: the occupant is precisely what the run refused to replace, so removing it would be the data loss the refusal exists to prevent. A distinct ArtifactCollision carries that decision to both catch blocks. Both tests redden when the mid-window check is dropped. O_NOFOLLOW alone does not: the check catches a planted symlink first, and the residual TOCTOU between check and open needs fault injection to reach — it is defence in depth, not something these tests prove. Smaller, each probe-reported and reproduced: - `tmux()` ran execFileSync with no stdio option, so Node teed every tmux child's stderr into ours through a raw, unguarded process.stderr.write — outside the broken-pipe guard and interleaved with contract output. Explicit stdio, like every sibling spawn. No dedicated pin: observing the tee needs a child driver with a closed stderr reader. - The freeze spawn took Node's silent 1 MiB maxBuffer, and an overrun kills with SIGKILL + ENOBUFS — recorded as "signal SIGKILL after the 30000ms render belt", a hang that never happened. The cap is explicit now and the attribution reads the error CODE. - execve answers ENOENT for a present-but-unexecable binary too (broken shebang), which the refusal asserted as "tmux is not installed". Nothing in the spawn result separates them, so the wording no longer claims the one it cannot know. - The ready sentinel's directory was the one path this run writes that no gate looked at — a consequence of moving it out of --out's namespace. An unusable TMPDIR burned the whole capture window waiting for a holder that could never signal ready. Probed up front, named in the refusal. - Three child-driven tests awaited exit with no external kill deadline (one also never drained its piped stdio, so a spewing regression blocks on a full pipe). They now match the FIFO sibling. - Both orphan guards killed with a signal that could not be told from the expected one, so a guard rescue passed green; the mid-poll guard also used SIGKILL, which cannot run the child's reap and would orphan the very server it guards. Both send SIGTERM and record that they fired. - The two --keys refusal gates ran against an empty dir, so their "no artifacts" assertions passed with or without a clear. Seeded, and they redden when the clear is skipped.
…more
The .ans/.json paths were re-checked after the capture window last round;
the png was not, and it is the one an external binary writes.
- A symlink planted at `<out>.png` during the window sent freeze's bytes
outside the --out base while this run attested `evidence: 'png'` for
them, because the pre-window stamp said the path was empty.
- When the render then failed, the torn-png cleanup asked that same stale
stamp and deleted whatever the captured command had put there.
One fix closes both: the png is re-stamped after the window, so every
question below it — render or degrade, credit or not, clean up or leave
alone — is decided against a closed window with only the render able to
have changed the path. Both new tests redden when the re-stamp is dropped.
Windows lanes: the TMPDIR test drove a variable `os.tmpdir()` never reads
there, and the EPIPE test pinned a path that does not exist there (a
broken pipe surfaces as UV_EOF/UV_EAGAIN, and the guard rethrows every
non-EPIPE code). Both gated, like their siblings.
Found by fixing a vacuous assertion rather than reported: the
leftover-socket test never asserted the capture succeeded, and under a
custom TMUX_TMPDIR it did not — a unix socket path is capped by
sockaddr_un (104 bytes on macOS), and this suite's mkdtemp base blew past
it. tmux answers `error connecting to … (File name too long)` AFTER the
start succeeds: a mid-capture refusal blaming tmux for a path this
command chose. Gated up front, measuring the base tmux will actually use
(the first USABLE one — measuring an unusable TMUX_TMPDIR refused runs
that were about to succeed under /tmp, which is how the first version of
this gate was caught being wrong).
Also:
- An empty `--cwd` was the one string argument whose empty form was not
refused: `resolve('')` is the launcher's cwd, so the enterability gate
always passed and the capture ran somewhere the caller never named,
with the manifest recording it as if asked for. Empty `--until`/`--ready`
refuse now too.
- The `--out` catch-all attributed fd exhaustion and a full disk to
`--out is not writable` — a machine-read reason telling an agent to fix
an argument that was fine. It names the host state instead.
- The freeze `absent` degradation asserted "not installed", the same
claim the tmux side was corrected for two commits ago.
- The kill-server retry repeated instantly, so it could not recover from
the client-spawn failure its own comment names; it pauses between
attempts and the WARNING separates "tmux said it failed" from "we could
not run tmux at all".
- The orphan sweep skipped a base silently when an ANCESTOR was
untraversable (existsSync swallows EACCES), past the catch written to
be loud about it; and its pid probe treated every non-EPERM throw as a
dead pid. Both pinned, both mutation-verified.
- Three tests were disarmed by last round's occupancy gate — two write-
failure tests could no longer reach the write, two protected-png tests
never spawned their fake freeze. Retargeted at what they now exercise,
with a marker proving the fixture that used to look load-bearing is not.
- One SIGKILL catch e9dbb43 missed, and a stale comment describing the
old sentinel design directly above the assertion pinning the opposite.
Two check-then-use races, both closed by moving the decision into the call the kernel makes atomically: - The clear phase verified `<out>.json` with lstat (regular file, size capped) and then re-resolved the path in readFileSync, so a swap between the two re-opened both classes those checks close: a FIFO that blocks the synchronous read with no refusal printed and no timeout able to interrupt it, and an arbitrarily large file that dies on the heap limit. One descriptor now answers both — opened O_NOFOLLOW|O_NONBLOCK, fstat'd, and read from the fd. - writeArtifact's occupancy check closed only the symlink half of its race with the open; a REGULAR file planted in that window was truncated by O_TRUNC and replaced, with the run reporting success. The path is always absent by then (cleared, or refused at the collision gate), so the open says exactly that: O_CREAT|O_EXCL, and EEXIST is a collision. Both are defence in depth against a racer, and I am not claiming test proof of the race itself: a faithful revert of either leaves the suite green, because a single-threaded test cannot occupy the window. The new FIFO test pins the branch, not the swap. Also fixed: - Every signal death leaked a sentinel: the re-raise terminates without unwinding, so the finally holding the only rmSync never ran. A harness reaping stuck captures produced one file per run in the system temp dir. - A RELATIVE TMPDIR resolved against the LAUNCHER's cwd for our probe and polling, but against the PANE's cwd inside the holder — the sentinel landed where nothing looked, and the precise early refusal became a dead ready-gate wait. The path is resolved once, up front. - The TMUX_TMPDIR usability check had no directoryness test, so a regular file there was measured as the socket base tmux would use and produced a machine-read refusal naming the wrong problem. - EDQUOT joined the host-state attribution instead of falling through to "--out is not writable". - isNothingToKill missed two tmux directory-level refusals (`directory … has unsafe permissions`, `… is not a directory`), each of which printed a false orphan WARNING naming a server that cannot exist. - The sweep treated ENOTDIR/ELOOP as scan failures, setting sweepFailed and suppressing "Nothing to clean" on a host where there was nothing to clean; only EACCES can be hiding an orphan. - `--timeout-ms`'s own description claimed it bounds a ready+keys capture; the settle runs after it, so wall time is timeout-ms + settle-ms. - Two doc blocks documented the declaration after the one they belong to. Tests: the no-orphan boundary test keyed pgrep/pkill to a bare `sleep 41`, which matches any concurrent run of this suite on a shared host — the finally then reached into other runs. Scoped to this pid, carried as a trailing `: <tag>` so sh cannot exec away the argv. The TMPDIR gate got the call-log and seeded-artifact pins its sibling gate families have (both mutation-verified). And three cleanup pins: the non-EPERM liveness arm, the post-kill unlink guard, and the EPERM fixture now built with the producer like every other one in that block.
…nd eight more Last round I widened `isNothingToKill` with two tmux wordings on the strength of "they also appear when no server was ever created". That is true and beside the point: both are refusals the CLIENT makes before it looks at anything, so they establish nothing about the server — and "nothing to kill" is what authorizes unlinking the socket. A live orphan behind such a socket read as reaped at both call sites: the sweep printed "Reaped", capture-tui's own reap went silent (no WARNING, exit 0), and both unlinked the socket of a running server, which makes it unreachable forever. They get their own state now: never reaped, always surfaced, the socket left alone, and the warning says which of the two things went wrong. The predicates were untested; they have a table now. The manifest size cap had the same shape as the round-6 TOCTOU it came from: fstat measured the file, then the read went to the LIVE end of the pinned inode, so an appender defeated the cap and reproduced the heap-limit death it was measured against. The read is bounded at cap+1 bytes, which also makes "it grew between the fstat and the read" observable rather than silent. And the residual identity half of the round-5 blocker: the signature authenticates the MANIFEST, and said nothing about the files beside it — a genuine previous manifest authorized unlinking whatever had since taken the .ans name. The manifest now records the identity of the artifacts it actually wrote, and the clear phase requires a match. A replaced artifact is not the one it describes, so the clear is skipped and the collision gate refuses: fail-closed. Also: tmuxPadsWithCaptureN answered true for 3.0.x, contradicting its own documented range and its sibling predicate (no shipped path reaches it — the version gate refuses those hosts first — but it is exported); and the four host-state attribution arms moved into an exported `hostStateFor` so each is pinned directly, since three of them need a fault injector to reach through the real syscalls (the R6-6 gap I had deferred). Tests: two wall bounds used Date.now() against this suite's own declared monotonic-clock rule; a throwing rmSync implementation leaked into every later test in cleanup.test.ts (clearAllMocks keeps implementations, so six later reap tests were silently exercising the failure branch); and the no-orphan boundary test tagged only the WRAPPER shell — sh forks an untagged sleep and waits, so its pkill killed the wrapper and left the sleep reparented to init. One orphan per run, from the test that exists to pin orphans. The tag rides in argv[0] now, via a per-run symlink to the real sleep. Neither the bounded read nor the O_EXCL/O_NOFOLLOW opens are proven by these tests: a faithful revert of any of them leaves the suite green, because a single-threaded test cannot occupy the window a racer occupies. The new tests pin the branches; the races are argued from the syscalls.
The reported reproduction is real: every artifact path resolves BY NAME, so a captured command running as the same uid could `mv dir dir.stolen && ln -s /victim dir` mid-window and send all three artifacts out of the --out base while the manifest went on attesting the original paths. The final component was guarded; the directory holding it was not. The suggested remedy — render elsewhere and rename onto the --out paths — does not close this one: rename(2) replaces a destination symlink rather than following it, which the O_NOFOLLOW/O_EXCL opens already handle, but its destination is still resolved by name, so a swapped ancestor takes the rename with it. What Node makes possible is identity: dev+ino of the directory, sampled up front and re-checked immediately before every artifact operation. That turns the reproduced attack — a swap anywhere in a capture window that runs up to 70 minutes — into a refusal, and leaves only the interval between the check and the syscall. Node exposes no *at() syscalls, so that residue cannot be closed from here; it is stated in the code rather than papered over. Two siblings the report names as superseded are closed with it: - The png was credited on `changed()`, which compares lstat identity — a symlink planted between the post-window re-stamp and freeze's write differs from "absent", so freeze wrote through it and the run credited the result. Only a regular file is a rendering this run produced. - The .ans identity now comes from the descriptor that wrote it (fstat before close) and is re-verified before the manifest describes it. The render window sits between those two points, and a manifest attesting bytes this run never produced is the wrong-evidence outcome the whole command exists to prevent. Also fixed: the padding-tmux degradation recorded "tmux tmux 3.2a" — tmuxVersion is already the `tmux -V` line, and every capture on a padding host (3.1-3.2.x, which is what Ubuntu 22.04 ships) carried it. Tests: the two MAX_MANIFEST_BYTES tests needed no tmux but sat behind skipIf(!hasTmux), so the heap guard was unpinned on every tmux-less lane — every Windows lane by definition. The foreign-manifest fixtures omitted the new artifacts rung, which masked whichever earlier rung a mutant restored; filled in with real identities, all four rungs redden individually (one needed a new fixture to isolate the evidence rung). Both handler call sites passed the yargs defaults byte-for-byte, so a handler hardcoding DEFAULT_COLS/DEFAULT_ROWS shipped green. leakedSentinels filtered by the vitest worker's pid while the signal-death tests spawn children, so the sentinel cleanup in onSignal — the sole cleanup on that path — was pinned by nothing. The FIFO test's wall bound could only run once the call had returned, which is exactly when the regression it names cannot happen; it carries a per-test budget instead. And the until-timeout test asserts a floor: everything else there is satisfied by a poll that bails on the first miss and records `settledBy: 'timeout'` anyway. Plus: the second reaped socket's unlink, the isSocketDirUnusable wiring (no fixture used either wording — including the regression I shipped last round), the bare 'no such file or directory' branch, the lettered 3.1 releases on the dangerous side of tmuxPadsWithCaptureN, and a test title describing a data flow this PR removed.
…rteen more Both Criticals this round are consequences of last round's fixes, and both came with a witness: - The directory-identity baseline was sampled BEFORE the `mkdirSync` that exists to create that directory, so an `--out` whose parent does not exist yet baselined 'gone' and the first write refused with "the directory holding --out was replaced" — after paying the whole capture window, and factually false: this run created it. Re-baselined after the mkdir; the capture window opens later, so the guard is unchanged. - The collision cleanup skipped only `manifestPath`, but that catch now also receives collisions naming the `.ans` (the identity re-check) and the directory (assertSameOutDir). In those shapes it asked `changed()` against the PRE-window stamps — true for any occupant — and deleted the file the refusal existed to protect, while this run's own bytes were already gone from that path. A path is removed only while its CURRENT identity still proves it is this run's. Both reproduce and both fixes redden their tests; the second is driven by a fake freeze that replaces the .ans from inside the render window, which is where the reported racer sat. Also fixed, all reported: - The socket-length gate measured a RELATIVE TMUX_TMPDIR verbatim, under-counting by the whole cwd — the same split-resolution hazard this file met one gate earlier with TMPDIR. - Writer and reader disagreed about a legitimate manifest: the writer embedded --command/--keys/--ready/--until with no bound while the reader caps at 1 MiB, so a run could write a manifest its own next run cannot verify. Measured from the arguments, before the window opens. - The ready-gate timeout is a pure existsSync poll after a start that SUCCEEDED, but it was routed through the catch that prefixes "tmux failed mid-capture" — a false causal claim in a machine-read reason. Tests: the identity-rung test needed no tmux yet sat behind skipIf(!hasTmux) (I swept the file — it was the only one left of that class); `--out must not be empty` had no effective pin, and it is the one gate that refuses WITHOUT clearing, so its assertions are the opposite of its siblings'; two of the four `create directory` alternates and the /i flag on the bare ENOENT branch were unpinned; the ~10-line capture-tui.ts resolver was copy-pasted six times, five of them failing silently when neither candidate existed; two cleanup tests probed hand-picked literal pids with the REAL process.kill; the win32 arm of the sweep had its contract asserted on no lane at all; the socket-length fixture overflowed by ~100 bytes so any mutant bound in [104, 197] passed it; and the slow-freeze shim hardcoded /bin/sleep, which on a host without it collapsed the 4s render window to nothing without failing.
npm run typecheck did not flag it; CI runs tsc --build, which does. The delete needs no suppression once the property is narrowed.
…ui-v2 # Conflicts: # packages/cli/src/commands/review/cleanup.test.ts # packages/cli/src/commands/review/cleanup.ts
A verifier that says "the panel renders correctly" has argued from the code. This adds `qwen review capture-tui`: it drives a command inside a PRIVATE tmux server, captures the pane text to `<out>.ans` always, renders `<out>.png` when `freeze` is available, and writes `<out>.json` saying which evidence rung it reached and why it stopped there. The safety property is isolation, and it is structural: every tmux call is scoped to a per-run private socket, so the capture cannot see — let alone resize or kill — the user's own tmux sessions. The measured failure mode of desktop-automation verification was exactly "drives the user's own windows"; a private server makes that impossible rather than discouraged. Degradation is explicit: the manifest names the rung (`png` or `ans-only`) and why the ladder stopped, because a verdict has to say what it stands on. A refused capture writes no manifest at all — exit 3, a reason on stderr, machine-readable JSON on stdout — so a missing manifest reads as a refusal rather than as evidence. The file header now states the file guarantee explicitly, including its boundary: this command protects evidence against ordinary conditions — a re-used --out, stale artifacts, an unrelated file at one of the names, a host out of descriptors or disk — and NOT against an active same-uid adversary racing it between a check and the following syscall. Node exposes no *at() syscalls, so every path resolves by name and that interval cannot be closed from here. Hardening against that model is a separate change with its own guarantees and tests, and is deliberately not this one's claim. `qwen review cleanup` learns to sweep orphaned capture servers, since a signal or a crash can outlive the finally that reaps them.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR — the contract-in-the-header style is doing real work here. Template: the required substance is present — one non-blocking miss: the body drops the template's Problem: observed, not theoretical. The repo's own verification pipeline drives tmux on shared runners, and the measured failure mode was "drives the user's own windows" — a private per-run Direction: aligned — internal review tooling in the same family as Size: ~2,505 production lines ( Approach: scope feels right. The producer/hardening split is honest — the header states the file guarantee and its boundary (Node exposes no Risk: no elevated risk signals — no changed file matches the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献——文件头里的契约式写法在这里起了实际作用。 模板：实质内容齐全——只有一个非阻断的小缺失：正文没有写模板里的 问题：已观测到的，不是理论假设。仓库自己的验证流水线在共享 runner 上驱动 tmux，实测到的故障模式是"驱动用户自己的窗口"——每次运行一个私有 方向：对齐——与 规模：生产代码约 2,505 行（ 方案：范围合理。生产者/加固的拆分是诚实的——文件头明确写出了文件担保及其边界（Node 没有 风险：无升级风险信号——没有改动文件命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewNo critical blockers found. I wrote my independent proposal before reading the diff (private Two things I checked specifically:
The test suite is the strongest part: ~4,400 lines pinning the refusal contract wordings, every degradation rung, the signal/reap ordering, freeze's stdin trap, the trailing-semicolon and sequenceDiagram
participant P1 as verify agent
participant P2 as capture-tui
participant P3 as artifact paths
participant P4 as private tmux server
participant P5 as freeze renderer
P1->>P2: run with command and out
P2->>P3: clear stale capture artifacts, stamp occupants, write-probe
P2->>P4: start on per-run socket, holder keeps pane alive
P4-->>P2: ready sentinel
P2->>P4: optional keys, then poll marker or deadline
P2->>P4: capture-pane, then kill-server reap
P2->>P3: write ans always
P2->>P5: render ans to png when available
P5-->>P2: png, or a degraded reason
P2->>P3: write manifest naming the evidence rung
P2-->>P1: success JSON, or exit 3 refusal with no manifest
Files changed (14 of 14 shown)
Testing evidenceUnattended CI run — per the gate's rules this review is static and never executes PR code; the evidence below is the PR's own CI on the reviewed commit, fetched via API. The unit suite is still running, so this table will be updated in place by the finalize job when it lands. macOS/Windows unit jobs and the CLI integration job report Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 One caveat a green suite would not settle: the real-tmux lanes inside the unit suite are gated on a runtime Sandboxed verification would settle this: Live-behavior note for maintainers: the isolated 中文说明代码审查未发现关键阻断问题。读 diff 之前我先写了独立方案（私有 专门核查了两点：
测试套件是全 PR 最强的部分：约 4,400 行钉住拒绝契约的措辞、每一档降级、信号/reap 顺序、freeze 的 stdin 陷阱、尾随分号与 测试证据无人值守 CI 运行——按 gate 规则，本审查为静态审查，绝不执行 PR 代码；下表证据来自 API 拉取的 PR 自身 CI。单元套件仍在运行，落地后由 finalize job 原地更新表格。macOS/Windows 单元任务与 CLI 集成任务显示 一个绿套件也解决不了的隐患：单元套件里的真实 tmux 通道以运行时 沙箱验证可以定论： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean static review and an unusually rigorous test suite; the one open question is whether the real-tmux lanes actually execute in CI, which the green run plus Stepping back: this reads like work by someone who measured the failures they defend against — nearly every branch in the 1,800-line command cites a reproduced failure mode and carries a named test pinning it. My independent proposal would have produced a working capture command but gotten three things subtly wrong that this gets right: clearing stale artifacts only when ownership is provable, separating "the server is gone" from "the client could not spawn" in the reap, and the tmux-version matrix for trailing spaces. The contract/boundary split with #9274 is honest and the right scope cut: this PR guarantees evidence integrity under ordinary conditions and says plainly what it does not guarantee. The density is earned — I looked for a simpler shape and did not find one that keeps the stated guarantees. Reservations, none blocking:
CI is still running on the reviewed commit, so approval is deferred until CI lands green on 中文说明置信度：4/5 —— 静态审查干净，测试套件异常严谨；唯一悬而未决的问题是真实 tmux 通道是否真的在 CI 里执行，这由绿 CI 加 退一步看：这读起来像一个真正测量过自己所防御故障的人写的工作——1,800 行命令里几乎每个分支都引用了一个复现过的故障模式，并有具名测试钉住它。我的独立方案能产出一个可用的 capture 命令，但会在三个地方悄悄做错，而本 PR 做对了：只在所有权可证明时清理陈旧产物、reap 时区分"server 已不在"与"client 根本 spawn 不起来"、以及尾随空格的 tmux 版本矩阵。与 #9274 的契约/边界拆分是诚实的，也是正确的范围切割：本 PR 担保普通条件下的证据完整性，并明确说出不担保什么。这种密度是挣来的——我找过更简单的形态，没有找到能保住既定担保的。 保留意见，均不阻断：
CI 仍在被审 commit 上运行，因此批准推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
|
@qwen-code /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 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-dev-bot
added
the
autofix/takeover
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D15-12 probe-belt TERM-immune shim hardcodes /bin/sleep (capture-tui.test.ts:2703) — still-standing round-9 blocker already reported on the PR at the same line (comment 3832236037, by qwen-code-ci-bot); re-checked this round, mechanism unch…
Unresolved, please confirm:
- [Critical] cleanup.ts:734 — doudouOUC R14-2 (comment 3834716946): cannot rule out that the sweep's post-kill identity re-check false-WARNINGs on clean reaps if some tmux version unlinks the socket after kill-server; the premise is contradicted by thre…
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the macOS lane did not run locally; the changed suites ran on the Linux lane only.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the Windows lane did not run locally.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped at the round cap of 3; the cap round still surfaced two findings (the loop did not converge).
Deferred under the convergence posture (round 15, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/capture-tui.ts:2031 — [review] render-staging catch blames every failure on a file swap in the machine-read manifestpackages/cli/src/commands/review/cleanup.test.ts:1013 — [review] literal-pid 626262 test never stubs the pid probe (withDeadPids)packages/cli/src/commands/review/cleanup.ts:719 — [review] manual-reap note interpolates the readdir-derived socket name unquoted into the pastable commandpackages/cli/src/commands/review/cleanup.ts:561 — [review] socket-base dedup has no discriminating test; deleting the seen set ships greenpackages/cli/src/commands/review/cleanup.ts:592 — [review] no quiet scan arm is pinned — not even ENOENT; narrowing or deleting the quiet set ships greenpackages/cli/src/commands/review/capture-tui.ts:1476 — [review] reap WARNING misattributes a never-spawned tmux (resolution Error/EACCES) as a wedged serverpackages/cli/src/commands/review/cleanup.test.ts:652 — [review] single-quote escape path of the manual-reap note is untested; escape-dropping mutant ships greenpackages/cli/src/commands/review/cleanup.test.ts:846 — [review] neither lstatSync-throw catch arm of the sweep is pinned; guard-catch mutant crashes runCleanuppackages/cli/src/commands/review/cleanup.test.ts:1047 — [review] win32-arm test never asserts runCleanup proceeds after the sweep's early returnpackages/cli/src/commands/review/cleanup.ts:666 — [review] sweep retry runs back-to-back without the sibling reap's probe-verified 100ms pausepackages/cli/src/commands/review/cleanup.ts:740 — [review] sweep unlinks unconditionally even when the identity re-check said entryChangedpackages/cli/src/commands/review/capture-tui.test.ts:4711 — [review] two timed settle tests include the real freeze render in their wall boundpackages/cli/src/commands/review/capture-tui.ts:1603 — [review] unconfirmed-reap WARNING advises the bare connect-by-name the reap refusedpackages/cli/src/commands/review/cleanup.ts:636 — [review] type-guard refusal leaves failedAny unset — 'Nothing to clean' beside an unreaped notepackages/cli/src/commands/review/capture-tui.ts:1604 — [review] manual-command prose sits inside the paste boundary — the pasted command failspackages/cli/src/commands/review/lib/tui-capture.test.ts:111 — [review] verdictExaminedBase's trailing-separator guard is pinned by no fixture; startsWith mutant ships greenpackages/cli/src/commands/review/capture-tui.test.ts:1197 — [review] failed-render cleanup's occupied(pngPath) arm is pinned by no test; mutant silently replaces a foreign occupantpackages/cli/src/commands/review/capture-tui.ts:1729 — [review] --keys send loop ignores the shared deadline and never yields the event loop (95x overrun; SIGINT blocked)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the macOS lane did not run locally; the changed suites ran on the Linux lane only。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the Windows lane did not run locally。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — stopped at the round cap of 3; the cap round still surfaced two findings (the loop did not converge)。
收敛姿态下延后（第 15 轮，非阻断）——已记录，本轮不要求修改：共 18 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.21.15)
…ui-core No conflicts. Picks up the workflow-size baseline correction (#9747), so the `Test` job's step-7 gate stops failing this branch's merge ref before any dependency is installed. `npm run build` clean and `vitest run --root packages/cli src/commands/review` 4882 passed / 0 failed.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] R14-2 (comment 3834716946, cleanup.ts post-kill identity re-check, ~line 735): the premise — some tmux version unlinks the socket after a clean kill-server, making a benign reap print a false WARNING — could not be ruled on from the code: t…
Not explored to full depth (tool budget reached): chunk 13: none — no check was cut short..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
Deferred under the convergence posture (round 16, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/capture-tui.ts:1041 — [probe] argument-size gate measures the manifest cap, not the per-argv exec limit (oversized --command fails later with E2BIG blamed on tmux)packages/cli/src/commands/review/capture-tui.test.ts:5611 — [probe] /bin/sleep hardcodes go false-red on hosts without it (orphan-boundary fixture and hangprobe shim)packages/cli/src/commands/review/cleanup.ts:971 — [review] sweep ordering before the redirected-temp-dir refusal is load-bearing per the added comment but unpinned by any testpackages/cli/src/commands/review/cleanup.ts:551 — [probe] realpath-keyed base dedup has no covering test (alias of /tmp doubles kills and output)packages/cli/src/commands/review/capture-tui.ts:984 — [review] coded probe-classification arms (spawned true/false, ENOBUFS) are asserted nowhere in the test suitepackages/cli/src/commands/review/cleanup.ts:533 — [review] sweep's returned reaped field has no read site — dead contract member per the project's read-site rulepackages/cli/src/commands/review/capture-tui.ts:234 — [review] probes seam doc block orphaned above resolveOnPath — the discipline comment is invisible at the declaration it protectspackages/cli/src/commands/review/capture-tui.ts:407 — [review] hostStateFor doc claim ('three of the four asserted nowhere') is falsified by this PR's own testspackages/cli/src/commands/review/cleanup.ts:592 — [probe] sweep's quiet ENOENT/ENOTDIR/ELOOP scan branch is exercised by zero fixtures (loudness mutant ships green)packages/cli/src/commands/review/cleanup.ts:719 — [probe] manual-reap base single-quote escape is dead from the tests' perspective (no quote-carrying fixture)packages/cli/src/commands/review/cleanup.ts:737 — [probe] post-kill vanished-entry re-check arm unpinned (emptied-catch mutant prints false 'Reaped' and ships green)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）：chunk 13：none — no check was cut short.。
未审查：反向审计——在 3 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 16 轮，非阻断）——已记录，本轮不要求修改：共 11 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // (measured on 3.3a, with the same call succeeding under the base | ||
| // it was found in). Kill it where it was FOUND — `dir` is | ||
| // `<base>/tmux-<uid>`, so its parent is the base tmux wants. | ||
| env: { ...process.env, TMUX_TMPDIR: dirname(dir) }, |
There was a problem hiding this comment.
[Critical] The orphan sweep spawns a bare tmux name — execFileSync('tmux', ['-L', name, 'kill-server'], ...) — re-opening the cwd-plantation hole that resolveOnPath in capture-tui.ts (added by this same PR) exists to close: execvp honours the empty-PATH-element → cwd rule, so with a legal PATH shape containing an empty element (leading/trailing/double colon) and an executable file named tmux in the reviewed worktree, the sweep executes the plant as the reviewer's uid. The comment above this call claims "Same belt as capture-tui's own control calls" while omitting the resolution half of that belt.
Witness (probe through the PR's real runCleanup): a planted socket qwen-review-capture-1485831-aaaa (dead pid, plain socket, nlink 1) passed the type guard and pid probe; the sweep printed Reaped orphaned capture server: … and the plant executed — PLANT-EXECUTED -L qwen-review-capture-1485831-aaaa kill-server. With absolute-path-only resolution patched in, the plant was NOT executed. Atomic Node 22 probe: PATH=":/nonexistent" + cwd-planted ./tmux → execFileSync('tmux', ['-L','x','kill-server']) executed the plant.
Failure shape: a malicious PR commits an executable tmux into the worktree and plants a qwen-review-capture-<dead pid>-<nonce> socket in /tmp/tmux-<uid>/ (dead pid trivially arranged); the next qwen review cleanup run from that worktree, on a host whose PATH carries an empty element, executes attacker-supplied code with the reviewer's full environment.
Suggested fix (spans two sites, so not a one-click suggestion): move resolveOnPath into lib/tui-capture.ts (or export it from capture-tui.ts), resolve tmux once at sweep start, and when it is unresolvable on absolute PATH elements skip the kill loop and emit the existing "could not reap" note instead of spawning a bare name.
中文说明
孤儿清扫以裸名 tmux 派生 —— execFileSync('tmux', ['-L', name, 'kill-server'], ...) —— 重新打开了同一个 PR 里 resolveOnPath（capture-tui.ts 新增）专门要关闭的 cwd 植入洞：execvp 遵守“空 PATH 元素 → cwd”规则，合法但含空元素的 PATH（首/尾/连续冒号）加上被审工作树里名为 tmux 的可执行文件，清扫就会以审查者 uid 执行植入物。此调用上方的注释声称“与 capture-tui 自己的控制调用同款腰带”，却漏掉了那条腰带里“先解析”的这一半。
证据（经 PR 真实 runCleanup 的探针）：植入的 socket qwen-review-capture-1485831-aaaa（死 pid、普通 socket、nlink 1）通过了类型守卫与 pid 探测；清扫打印 Reaped orphaned capture server: … 且植入物被执行 —— PLANT-EXECUTED -L qwen-review-capture-1485831-aaaa kill-server。换成“仅绝对路径解析”的补丁后植入物未被执行。Node 22 原子探针：PATH=":/nonexistent" + cwd 植入 ./tmux → execFileSync('tmux', ['-L','x','kill-server']) 执行了植入物。
触发形态：恶意 PR 把一个可执行 tmux 提交进工作树，并在 /tmp/tmux-<uid>/ 放置 qwen-review-capture-<死pid>-<nonce> socket（死 pid 容易制造）；下一次在该工作树里运行 qwen review cleanup（宿主 PATH 含空元素时），即以审查者的完整环境执行攻击者提供的代码。
建议修复（跨两处，非一键建议）：把 resolveOnPath 移入 lib/tui-capture.ts（或从 capture-tui.ts 导出），在清扫开始时解析一次 tmux；在绝对 PATH 元素上无法解析时跳过 kill 循环并输出既有的 “could not reap” 备注，而不是以裸名派生。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // or a backtick expands at paste time and resolves the wrong | ||
| // base — the same confusion this note exists to prevent. | ||
| `(TMUX_TMPDIR='${dirname(dir).replaceAll("'", "'\\''")}' ` + | ||
| `tmux -L ${name} kill-server to reap it by hand)`, |
There was a problem hiding this comment.
[Critical] This manual-reap note shell-quotes the TMUX_TMPDIR base against paste-time expansion but interpolates ${name} — a readdir-derived string matched only by the prefix-anchored regex ^qwen-review-capture-(\d+)- with an unrestricted suffix — unquoted into the same paste-targeted command. The comment three lines above exists precisely to warn that $ or a backtick "expands at paste time"; the base half of this line was defended against exactly that and the attacker-controlled half was left open. Five independent auditors across two review rounds reported this.
Witness (end-to-end probe): a same-uid planter bound a plain unix socket named qwen-review-capture-2441321-x$(touch pwned-r167) — it passed every guard the sweep has (type guard, nlink, dead-pid probe); both kill attempts rode the belt, so the note printed with the payload intact (TMUX_TMPDIR='/tmp/r167base' tmux -L qwen-review-capture-2441321-x$(touch pwned-r167) kill-server …); pasting the suggested command — the note's stated purpose — created pwned-r167 in the paste cwd: the substitution executed.
Failure shape: the sweep's own documented planter class (a daemonized descendant, same uid) binds a socket whose suffix carries $(…)/backticks/; — a socket name cannot contain /, so payloads use relative targets or substitution forms; command execution at paste time is the harm either way.
| `tmux -L ${name} kill-server to reap it by hand)`, | |
| `tmux -L '${name.replaceAll("'", "'\\''")}' kill-server to reap it by hand)`, |
(Stronger alternative: tighten orphanRe to the producer's actual shape — ^qwen-review-capture-(\d+)-[0-9a-f]{8}$ — and skip anything else; that also protects the display-only ${name} interpolations in the WARNING and "Reaped" lines.)
中文说明
这条手动清扫备注对 TMUX_TMPDIR base 做了粘贴时展开防护（shell 单引号），却把 ${name} —— 一个来自 readdir、仅由前缀锚定正则 ^qwen-review-capture-(\d+)-（后缀不受限）匹配的字符串 —— 原样插值进同一条面向粘贴的命令。上方三行的注释正是为了警告 $ 或反引号“会在粘贴时展开”；这一行的 base 一半恰恰为此做了防护，而攻击者可控的一半被留下了。两个评审轮次里有五位独立审计者报告了此处。
证据（端到端探针）：同 uid 植入者绑定了一个名为 qwen-review-capture-2441321-x$(touch pwned-r167) 的普通 unix socket —— 它通过了清扫的全部守卫（类型守卫、nlink、死 pid 探测）；两次 kill 都走到皮带，于是备注带着完整载荷打印（TMUX_TMPDIR='/tmp/r167base' tmux -L qwen-review-capture-2441321-x$(touch pwned-r167) kill-server …）；把这条建议命令粘贴执行 —— 正是该备注的用途 —— 在粘贴目录创建了 pwned-r167：替换被执行。
触发形态：清扫自己文档点名的植入者类别（守护化后代、同 uid）绑定后缀携带 $(…)/反引号/; 的 socket —— socket 名不能含 /，载荷使用相对目标或替换形态；无论哪种，粘贴时命令执行即危害。
上方 suggestion 单引号转义 name（与 base 同款）。更彻底的替代：把 orphanRe 收紧到生产者的真实形态 —— ^qwen-review-capture-(\d+)-[0-9a-f]{8}$ —— 其余一律跳过；这也同时保护 WARNING 与 “Reaped” 行里仅展示用途的 ${name} 插值。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| fsConstants.O_TRUNC | | ||
| (fsConstants.O_NOFOLLOW ?? 0), |
There was a problem hiding this comment.
[Critical] writeArtifact's open omits the O_NONBLOCK that the manifest READ open ~100 lines above carries for exactly this hazard — a FIFO planted at <out>.ans/<out>.json in the changed()→openSync race blocks the write-side open forever: open(O_WRONLY) on a FIFO without O_NONBLOCK waits for a reader that never comes. The main thread wedges in a synchronous syscall — the refusal contract breaks (no machine-readable reason, no exit-3 disposition), and the process needs an external SIGKILL. A hang, not a refusal, from untrusted input in a tool whose contract is machine-read. (The read side has O_NONBLOCK with a comment naming this exact class, and the PR ships a FIFO-at-artifact test for the read side; the write side was left open.)
Witness: kernel arm with the PR's exact flag set — child opening a FIFO BLOCKED after 5s (state S, wchan=wait_for_partner); with O_NONBLOCK added, {result:"threw", code:"ENXIO", ms:0}. End-to-end through the real runCaptureTui under a mkfifo/unlink racer at <out>.ans: wedge observed — the run log holds only the reap WARNING, no refusal JSON; fix arm 14/14 rows returned within budget, control arm 3/3 still completed.
Failure shape: the captured command — the file's explicit adversary, untrusted code running under this uid — learns --out and sprays mkfifo/unlink during the capture window; a FIFO landing between changed()'s lstat and the open wedges the run.
| fsConstants.O_TRUNC | | |
| (fsConstants.O_NOFOLLOW ?? 0), | |
| fsConstants.O_TRUNC | | |
| (fsConstants.O_NOFOLLOW ?? 0) | | |
| (fsConstants.O_NONBLOCK ?? 0), |
(Pair with treating ENXIO alongside EEXIST/ELOOP as an ArtifactCollision in the catch below — a FIFO that failed the nonblocking open is a claimant this run did not write. Impact note: reap() runs before writeArtifact, so the harm is the indefinite wedge plus broken refusal contract, not an orphaned reap.)
中文说明
writeArtifact 的 open 缺少约 100 行上方 manifest 读取 open 为同一危害专门携带的 O_NONBLOCK —— 在 changed()→openSync 竞态窗口内被放置到 <out>.ans/<out>.json 的 FIFO 会令写入侧 open 永久阻塞：不带 O_NONBLOCK 的 open(O_WRONLY) 打开 FIFO 会等待一个永不出现的读者。主线程楔死在同步系统调用里 —— 拒绝契约被打破（无机读原因、无 exit-3 裁决），进程只能靠外部 SIGKILL 结束。一个契约就是机读的工具，因不可信输入而“挂住”而不是“拒绝”。（读取侧带 O_NONBLOCK 且注释点名了同一危害类别，PR 还为读取侧配了 FIFO 产物测试；写入侧被遗漏了。）
证据：以 PR 原样标志集做内核支路 —— 子进程打开 FIFO 5 秒后仍阻塞（状态 S、wchan=wait_for_partner）；加上 O_NONBLOCK 后 {result:"threw", code:"ENXIO", ms:0}。经真实 runCaptureTui 端到端、在 <out>.ans 上跑 mkfifo/unlink 竞速者：观察到楔死 —— 运行日志只有 reap WARNING、无拒绝 JSON；修复支路 14/14 行在预算内返回，对照支路 3/3 仍完成。
触发形态：被采集命令 —— 本文件明示的对手、以此 uid 运行的不可信代码 —— 得知 --out 并在采集窗口内喷洒 mkfifo/unlink；落在 changed() 的 lstat 与 open 之间的 FIFO 即楔死运行。
上方 suggestion 在写入 open 标志中加入 O_NONBLOCK（对常规文件无副作用）。配套：下方 catch 把 ENXIO 与 EEXIST/ELOOP 一样按 ArtifactCollision 处理 —— 非阻塞 open 失败的 FIFO 正是本运行未写入的占用者。影响说明：reap() 在 writeArtifact 之前运行，故危害是无限期楔死加拒绝契约被打破，而非 reap 被孤儿化。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // base: that base's own kill succeeds and its confirmed death | ||
| // is global, so the uncredited verdict here raises no warning | ||
| // of its own. | ||
| (isSocketPathAbsent(stderrText) && startThrew)) && |
There was a problem hiding this comment.
[Critical] The reap credits ENOENT-class kill wordings as death whenever startThrew is set — but the same file sets startThrew for belt-cut starts that threw AFTER forking a live, socket-bound server (the measured shape documented near the start call: "a start cut by the control belt throws with the server ALREADY UP"), and no socket stamp is taken in that path. The flag conflates "start never bound a socket" with "start bound, then threw" — the exact fix this line's own comment describes for the stamp-absence conflation, re-applied one level up onto startThrew.
Witness: arm A (belt-cut start: startThrew=true, no stamp) — both candidate-base kills answered the ENOENT wording and it was credited: {exitCode:3, warningPrinted:false}; arm B (startThrew=false) — the identical wording printed WARNING — kill-server failed twice; the private tmux server … may still be running. Wording lemma on real tmux 3.3a: rm the socket under a running server → kill-server answers exactly error connecting to <path> (No such file or directory) while kill -0 shows the server STILL ALIVE.
Failure shape: on a loaded runner the 15s control belt SIGKILLs new-session after the server forked; the captured command — modeled throughout this file as untrusted same-uid code — loops rm "$TMUX". The reap credits death on both bases: no WARNING, and the live server, session and pane holder are orphaned silently for up to the holder's three-hour bound, with the socket gone so neither tmux -L nor the readdir sweep can ever reach it again.
Suggested fix (semantic choice, so not a one-click suggestion): stop letting startThrew carry "never bound" — either refuse to credit isSocketPathAbsent entirely when the stamp is absent (set unconfirmed = true so the WARNING fires — this file's own rule is "Doubt stays visible", at the cost of a false-orphan warning in the true never-bound shape), or keep the credit only for the provable never-bound sub-shape (socket DIRECTORY never created under startThrew) and fail closed for path-absent, since absence of the file cannot distinguish "never bound" from "bound and removed".
中文说明
只要 startThrew 为真，reap 就把 ENOENT 类 kill 措辞采信为死亡 —— 但同一文件会对“在 fork 出活的、已绑定 socket 的服务器之后才抛错”的皮带截断启动置 startThrew（start 调用附近文档记录的实测形态：“被控制皮带截断的启动抛错时服务器已经起来了”），而该路径不会盖 socket 戳。这个标志把“启动从未绑定 socket”与“启动绑定了、随后抛错”混为一谈 —— 正是这一行的注释为“戳缺失混淆”描述的修复，在 startThrew 这一层被原样重演。
证据：支路 A（皮带截断启动：startThrew=true、无戳）—— 两个候选 base 的 kill 都给出 ENOENT 措辞且被采信：{exitCode:3, warningPrinted:false}；支路 B（startThrew=false）—— 同样的措辞打印 WARNING — kill-server failed twice; the private tmux server … may still be running。真实 tmux 3.3a 措辞引理：运行中的服务器下 rm 掉 socket → kill-server 恰好回答 error connecting to <path> (No such file or directory)，而 kill -0 显示服务器仍然活着。
触发形态：高负载跑者上 15 秒控制皮带在服务器 fork 之后 SIGKILL 掉 new-session；被采集命令 —— 本文件通篇建模为同 uid 不可信代码 —— 循环 rm "$TMUX"。reap 在两个 base 上都采信死亡：无 WARNING，活的服务器、会话与 pane 持有者被静默孤儿化，最长存活至持有者三小时上限，且 socket 已不存在，tmux -L 与 readdir 清扫都永远无法再触达它。
建议修复（语义取舍，非一键建议）：不要让 startThrew 承担“从未绑定” —— 要么在戳缺失时完全不采信 isSocketPathAbsent（置 unconfirmed = true 让 WARNING 响起 —— 本文件自己的规则是“存疑必须可见”，代价是在真正从未绑定的形态下多一次假孤儿告警），要么只在可证明的从未绑定子形态（startThrew 下 socket 目录从未被创建）保留采信，对 path-absent 失败关闭 —— 文件不存在无法区分“从未绑定”与“绑定后被移除”。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Death established GLOBALLY: the server name is unique to this | ||
| // run, so a kill that succeeded reaped this server whichever | ||
| // base answered. | ||
| confirmedDead = true; |
There was a problem hiding this comment.
[Critical] This global-death inference — "the server name is unique to this run, so a kill that succeeded reaped this server whichever base answered" — is false under the file's own threat model. A same-uid captured command that knows the socket name from $TMUX can bind a sacrificial tmux server at that exact name under the OTHER candidate base; its exit-0 kill then sets confirmedDead and suppresses the orphan WARNING for this run's still-alive server. The planted-entry identity check above is deliberately skipped off the start base ("the server binds ONE socket"), so the sacrifice passes every guard. The stamp, when present, PROVES the server bound on the start base — an exit-0 kill on any other base can never be this run's server, yet it outranks the doubt.
Witness (three probe layers): shim probe driving the real reap logic — at HEAD the run exits with no WARNING while start-base kills answer ENOENT twice (never credited, stamp present) and the /tmp kill exits 0; applying the one-line credit gate flips it to WARNING — kill-server failed twice. End-to-end, real tmux 3.3a, success shape: {exitCode:null, captured:true, hasWARNING:false, livePids:["3491991"], plantStatus:0} — decoy planted at /tmp under the same name, the run's real server alive after the reap; the refusal shape likewise exits 3 with no WARNING and the orphan alive.
Failure shape: capture runs with TMUX_TMPDIR=<scratch> (the mkdtemp CI shape this file itself measured); the captured command removes its own socket under <scratch> and starts the sacrifice at /tmp/tmux-<uid>/<identical name> — the real server and pane holder are orphaned for up to the three-hour holder bound, unreachable via -L (socket file gone) and invisible to the readdir-based orphan sweep.
Suggested fix (verified to flip all three probe layers): only let a successful kill establish global death where it can be this run's server — set confirmedDead = true only when resolve(base) === startBase || socketStampIno === undefined (the fallback shape the inference was written for); a cross-base exit-0 kill while the stamp pins the server to the start base must leave unconfirmed loud. The PR's own 7 credit-titled reap tests stay green with this gate.
中文说明
这条全局死亡推断 —— “服务器名对本次运行唯一，所以任一 base 上成功的 kill 都收割了本服务器” —— 在本文件自己的威胁模型下不成立。知道 socket 名（来自 $TMUX）的同 uid 被采集命令可以在另一个候选 base 下以完全相同的名字绑定一个替身 tmux 服务器；对它 exit 0 的 kill 会置 confirmedDead，为本次仍然活着的服务器压掉孤儿 WARNING。上方的植入入口身份检查在非起始 base 上被刻意跳过（“服务器只绑定一个 socket”），替身因此通过全部守卫。而戳记一旦存在就证明服务器绑定在起始 base —— 其他 base 上 exit 0 的 kill 绝不可能是本运行的服务器，却压过了存疑。
证据（三层探针）：驱动真实 reap 逻辑的 shim 探针 —— HEAD 上运行退出且无 WARNING：起始 base 的 kill 两次回答 ENOENT（有戳、不采信），/tmp 的 kill exit 0；加上单行采信门槛后翻转为 WARNING — kill-server failed twice。端到端、真实 tmux 3.3a、成功形态：{exitCode:null, captured:true, hasWARNING:false, livePids:["3491991"], plantStatus:0} —— 替身以同名植入 /tmp，运行真实的服务器在 reap 后仍活着；拒绝形态同样 exit 3、无 WARNING、孤儿存活。
触发形态：采集以 TMUX_TMPDIR=<scratch> 运行（本文件自己实测过的 mkdtemp CI 形态）；被采集命令移除 <scratch> 下自己的 socket 并在 /tmp/tmux-<uid>/<同名> 启动替身 —— 真实服务器与 pane 持有者被孤儿化最长三小时持有上限，-L 不可达（socket 文件已消失），基于 readdir 的孤儿清扫也看不见它。
建议修复（已验证可翻转全部三层探针）：只让“可能是本运行服务器”的成功 kill 确立全局死亡 —— 仅当 resolve(base) === startBase || socketStampIno === undefined（该推断原本为之编写的回退形态）才置 confirmedDead = true；戳记已把服务器钉在起始 base 时的跨 base exit-0 kill 必须让 unconfirmed 保持响亮。PR 自己的 7 个 credit 标题 reap 测试在该门槛下保持绿色。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🔓 Takeover auto-released: the autofix loop paused on this PR 3 day(s) ago (🤖 AutoFix stopped after 5 consecutive rounds that pushed nothing (failed rounds, timeouts, gate rejections, or stops und) and no re-arm followed, so the 中文说明🔓 已自动释放接管：autofix 循环在 3 天前暂停于此 PR（🤖 AutoFix stopped after 5 consecutive rounds that pushed nothing (failed rounds, timeouts, gate rejections, or stops und），此后无人重新武装，现移除 |
qwen-code-dev-bot
removed
the
autofix/takeover
…here, and stop inferring death Five Critical findings from the round against the previous head. Two of them are the previous round's own fixes not going far enough, which is said plainly below rather than re-argued. **The orphan sweep still spawned a bare `tmux`.** Last round taught `capture-tui.ts` to resolve every binary it executes, on the grounds that execvp honours the empty-PATH-element → cwd rule and the cwd is the reviewed worktree. `cleanup.ts`'s sweep was left out — with a comment claiming "same belt as capture-tui's own control calls" while carrying only half of it. A PR that commits an executable `tmux` and plants a dead-pid socket gets that binary executed by the next `qwen review cleanup`, with the reviewer's environment. `resolveOnPath` moves to `lib/tui-capture.ts`, which both commands already import, and the sweep resolves at the point of the kill — so a host with no orphans and no tmux stays silent, and an unresolvable tmux is a disclosed skip instead of a spawn of whatever the tree supplied. **The sweep's matcher was a prefix, and everything downstream interpolated the name.** `^qwen-review-capture-(\d+)-` left the rest of the name to whoever created the socket, and the sweep put it into a command built to be PASTED plus a stdout and a stderr line — so `…-x$(touch pwned)` bound by a same-uid planter reached an operator's shell. The producer's own anchored shape now lives beside `captureServerName` as `CAPTURE_SERVER_NAME_RE`, so the two cannot drift, and the nonce is matched by ALPHABET rather than length: a hex run cannot carry a shell metacharacter, which is the whole property needed, while pinning the count would silently stop the sweep the day someone widens `randomBytes`. The pastable name is quoted as well — second wall behind the gate. **`writeArtifact` could wedge on a FIFO.** The manifest READ open carries `O_NONBLOCK` against exactly this class and the PR ships a test for that side; the write side did not. `open(O_WRONLY)` on a FIFO waits for a reader that never comes — on the main thread, inside a synchronous syscall — so the machine-read refusal contract breaks outright and only an external SIGKILL ends it. The flags are now a named export, because the one that matters cannot be reached behaviourally: a FIFO at any moment other than the microseconds between `changed()`'s lstat and the open is caught by `changed()` as the occupant it is, and a race a test loses proves nothing. `ENXIO` joins `EEXIST`/`ELOOP` as the collision it is. **The ENOENT class credits nothing now, on any base, under any flag.** Last round moved the credit from an absent stamp to `startThrew` — which conflates the same way one level up, because this file's own start comment says a belt-cut start throws with the server already forked and its socket bound. Both proxies for "so the server never existed" were wrong, and there is no third: on a real tmux, `rm` of a live server's socket answers this exact wording while `kill -0` shows it alive, and the captured command is the thing that removes it. So the wording buys doubt, not death. The cost is stated and pinned: a start that threw now always warns, which is what the start comment says is true of it anyway. **A kill on another base no longer vouches for the start base.** "The server name is unique to this run" is not exclusivity under this file's threat model: the captured command reads the name from `$TMUX` and binds a sacrificial server under the other candidate base, whose exit-0 kill silenced the orphan WARNING for the real one. A present stamp proves the bind happened on the start base, so a success anywhere else cannot be ours; with no stamp the bind site is unknown and any base's success remains the best evidence there is. Five tests, each mutation-checked against its own fix. Two fixture repairs came out of the work: `isSocketPathAbsent` had no consumer left once the class stopped being credited, so it and its assertion are gone (the test keeps the half that matters — those wordings stay out of `isNothingToKill`); and two sweep fixtures minted non-hex nonces, which the producer never does. Second-order effects, checked rather than assumed: - A start that threw now prints an orphan WARNING beside its refusal. That is the intended cost of the ENOENT change, and honest: the file's own comment says such a start can leave a live server behind. - Gating `confirmedDead` means a stamped run whose start-base kill fails transiently while the other base succeeds now warns instead of exiting quietly. With the stamp pinning the server to the start base, the other base's success was never proof. - `cleanup.test.ts` mocks `accessSync` now, defaulting to refusal, because the sweep's PATH walk reads `node:fs` like everything else in that file — otherwise these assertions would depend on where the host installed tmux. `npm run build` clean, prettier and eslint clean, and `vitest run --root packages/cli src/commands/review` 4887 passed / 0 failed (4882 before these five tests).
…ui-core No conflicts. `npm run build` clean and `vitest run --root packages/cli src/commands/review` 4967 passed / 0 failed on the result.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] R14-2 (comment 3834716946, cleanup.ts post-kill identity re-check): the sweep may false-positive WARNING on a clean reap IF some tmux version unlinks the socket after kill-server — the premise is contradicted by measurement on tmux 3.4 (liv…
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally (this host is Linux) — this is the exact lane the confirmed R17-1 Critical fails on.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally — tmux/socket/PATH behaviour in this PR is platform-sensitive.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and did not run locally — the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites.
Deferred under the convergence posture (round 17, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/cleanup.ts:746 — [review] pasteable manual-reap note hands the operator a bare tmux name though the resolved absolute tmuxBin is in scopepackages/cli/src/commands/review/capture-tui.test.ts:2319 — [probe] pin 'does not credit a start-base verdict about a socket that is not the stamped one' cannot discriminate its guard — the fixture's swap lands after the pre-loop identity c…
Convergence: round 17 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 5 (5 new). Findings keep coming back to the same files: packages/cli/src/commands/review/capture-tui.ts (findings in round 16; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally (this host is Linux) — this is the exact lane the confirmed R17-1 Critical fails on。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally — tmux/socket/PATH behaviour in this PR is platform-sensitive。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and did not run locally — the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites。
收敛姿态下延后（第 17 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 17 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 5 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/capture-tui.ts（第 16 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(ARTIFACT_OPEN_FLAGS & fsConstants.O_NOFOLLOW).toBeTruthy(); | ||
| expect(ARTIFACT_OPEN_FLAGS & fsConstants.O_NONBLOCK).toBeTruthy(); |
There was a problem hiding this comment.
[Critical] The ARTIFACT_OPEN_FLAGS pin test ('opens artifact writes non-blocking — a FIFO must refuse, not wedge', added by the latest fix commit) asserts O_NOFOLLOW and O_NONBLOCK are set, but carries no Windows gate — and Node's fs.constants exposes neither constant on Windows ("On Windows, only O_APPEND, O_CREAT, O_EXCL, O_RDONLY, O_RDWR, O_TRUNC, O_WRONLY, and UV_FS_O_FILEMAP are available"). The repo corroborates this in five places, and this PR's own production code admits it: capture-tui.ts:274-275 builds the same constant with (fsConstants.O_NOFOLLOW ?? 0) / (fsConstants.O_NONBLOCK ?? 0). On Windows ARTIFACT_OPEN_FLAGS & undefined coerces to 0, so toBeTruthy() fails: every Windows merge-queue run (test_windows runs the full vitest suite, and packages/cli/vitest.config.ts has no exclusion for this file) goes red on these two assertions while the Linux/macOS lanes stay green. All 42 POSIX-dependent siblings in this file carry win32 gates; this one is the lone exception. Witness: driving the verbatim anchored assertions against a model of the documented win32 constants table fails with AssertionError: expected +0 to be truthy … Received: 0, while the twin arm carrying the typeof-gate fix below stays green in the same run; the real test passes on Linux (1 passed | 168 skipped).
| expect(ARTIFACT_OPEN_FLAGS & fsConstants.O_NOFOLLOW).toBeTruthy(); | |
| expect(ARTIFACT_OPEN_FLAGS & fsConstants.O_NONBLOCK).toBeTruthy(); | |
| if (typeof fsConstants.O_NOFOLLOW === 'number') { | |
| expect(ARTIFACT_OPEN_FLAGS & fsConstants.O_NOFOLLOW).toBeTruthy(); | |
| } | |
| if (typeof fsConstants.O_NONBLOCK === 'number') { | |
| expect(ARTIFACT_OPEN_FLAGS & fsConstants.O_NONBLOCK).toBeTruthy(); | |
| } |
(The typeof form mirrors production's ?? 0 degradation — on Windows the two flags legitimately contribute nothing, so gating preserves the pin where it is meaningful instead of deleting it. An it.skipIf(process.platform === 'win32') on the whole test matches the 42 siblings and works equally.)
中文说明
ARTIFACT_OPEN_FLAGS 标志位钉测试（'opens artifact writes non-blocking — a FIFO must refuse, not wedge'，由最新修复提交加入）断言 O_NOFOLLOW 与 O_NONBLOCK 已置位，但没有 Windows 门槛 —— 而 Node 的 fs.constants 在 Windows 上根本不提供这两个常量（"On Windows, only O_APPEND, O_CREAT, O_EXCL, O_RDONLY, O_RDWR, O_TRUNC, O_WRONLY, and UV_FS_O_FILEMAP are available"）。仓库里有五处独立佐证，本 PR 自己的生产代码也承认这一点：capture-tui.ts:274-275 以 (fsConstants.O_NOFOLLOW ?? 0) / (fsConstants.O_NONBLOCK ?? 0) 构造同一常量。在 Windows 上 ARTIFACT_OPEN_FLAGS & undefined 强制转换为 0，toBeTruthy() 随之失败：每一次 Windows 合并队列运行（test_windows 跑完整 vitest 套件，packages/cli/vitest.config.ts 对本文件没有任何排除）都会在这两条断言上变红，而 Linux/macOS 通道保持绿色。本文件中全部 42 个依赖 POSIX 的兄弟测试都带 win32 门槛，唯独此处遗漏。证据：以文档化 win32 常量表的模型驱动原样锚定断言，得到 AssertionError: expected +0 to be truthy … Received: 0；同一次运行中携带下方 typeof 门槛修复的对照分支保持绿色；真实测试在 Linux 上通过（1 passed | 168 skipped）。上方 suggestion 以 typeof 守卫镜像生产代码的 ?? 0 降级 —— 在 Windows 上这两个标志本就不贡献任何位，门槛只是把钉保留在它有意义的平台，而非删除；对整个测试加 it.skipIf(process.platform === 'win32') 与 42 个兄弟一致，同样可行。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| let stampedSocketAlive = false; | ||
| if (socketStampIno !== undefined && uid !== undefined) { | ||
| try { |
There was a problem hiding this comment.
[Critical] The start-base identity arm in the reap reads stampedSocketAlive computed exactly once, BEFORE the candidate-base loop — so a socket swap that lands after this computation (inside the kill window, which spans both bases' attempts plus the retry) passes the arm, and a goal-state verdict about the REPLACEMENT socket is credited on the start base inside baseDead below; the reap then unlinks an entry this capture did not write. The arm's own comment claims exactly this closure — "a base destroyed and recreated mid-window can answer about a socket this run never owned, and the live server behind the destroyed one would read as dead" — but a swap landing between the pre-loop lstat and the credited verdict slips through it, and unlike the planted-entry rename race this residue is nowhere acknowledged. The pre-loop justification ("a kill this loop credits unlinks the socket, and that removal is this reap's own") does not hold: each base's verdict is evaluated before that base's unlink, so a verdict-time re-check never misreads this reap's own removal. Failure shape: a daemonized survivor of the captured command (the file's documented same-uid attacker class; it knows the path from $TMUX) waits for the reap, renames the run's LIVE server socket away, and leaves at the path an entry that answers a creditable goal state (a sacrificial server bound there and killed answers no server running on <path>); with both candidate bases creditable, unconfirmed && !confirmedDead is false — no WARNING, exit 0, captured: true, the live server holding the pane holder up to three hours reads as reaped and is invisible to the readdir-based orphan sweep. Both stated invariants break: "A presumed-alive private server is never a silent outcome" and "Unlink the socket ONLY when the server is known dead". Witness (flip probe through the real runCaptureTui, tmux 3.4, shim performs the swap inside the kill handler, fallback base made creditable with a planted listener-less socket): unmodified PR — {"exitCode": 0, "hasWARNING": false, "swappedOccupantSurvives": false, "stolenLiveSocketSurvives": true} (silent orphan, swapped occupant unlinked); with identity re-checked at verdict time — {"hasWARNING": true, "stderr": "capture-tui: WARNING — kill-server failed twice; the private tmux server … may still be running …", "swappedOccupantSurvives": true}; the probe flips deterministically. Note the paired pin 'does not credit a start-base verdict about a socket that is not the stamped one' cannot catch a regression here: its fixture swaps inside the kill handler (after this computation), and its WARNING assertion is satisfied by the OTHER base's uncredited ENOENT wording (mutant-verified this round).
Fix (semantic, spans this site and the crediting catch below): re-establish identity at verdict time — before crediting a start-base goal-state wording, re-lstat the entry and treat "stamped inode gone or changed" as fail-closed doubt, exempting only the removal THIS base's own successful exit-0 kill just performed:
// inside the catch, before crediting a start-base goal-state wording:
let identityHolds = true;
if (onStartBase && socketStampIno !== undefined) {
try {
identityHolds =
lstatSync(join(startBase, `tmux-${uid}`, server)).ino ===
socketStampIno;
} catch {
identityHolds = false; // gone or swapped since the loop opened
}
}
// ...credit only when identityHolds; a kill THIS base just performed is
// the one exempted removal, so re-check BEFORE any unlink, not after.This narrows the race window from the whole reap (seconds–tens of seconds, plus the free second attempt) to sub-millisecond and restores fail-loud; full closure against an active racer remains #9274's documented business. Pair with a fixture that swaps between the stamp and the reap so the existing pin discriminates the arm.
中文说明
reap 中起始 base 的身份支路读取的 stampedSocketAlive 是在候选 base 循环开始之前一次性计算的 —— 因此落在该计算之后的 socket 交换（落在 kill 窗口内，该窗口横跨两个 base 的尝试与重试）会通过该支路，替换者 socket 的目标态措辞会在下方 baseDead 中被记在起始 base 名下；随后 reap 会 unlink 一个本次采集并未写入的条目。该支路自己的注释恰恰声称封闭了这一形态 —— "一个在窗口中被销毁又重建的 base 可能为一个本运行从未拥有的 socket 作答，而被销毁者背后活着的服务器会被读成已死" —— 但落在循环前 lstat 与被采信措辞之间的交换恰好穿过这一封闭，且与植入入口的 rename 竞态不同，这一残留没有在任何地方被承认为剩余。循环前计算的辩护理由（"本循环采信的 kill 会 unlink socket，那次移除是本 reap 自己的，不是窗口中被毁"）并不成立：每个 base 的措辞都在该 base 的 unlink 之前求值，所以在措辞时刻重查身份永远不会误读本 reap 自己的移除。触发形态：被采集命令的守护化幸存者（本文件明示的同 uid 攻击者类别；它从 $TMUX 知道路径）等待 reap，把本运行活着的服务器 socket 重命名走，并在原路径留下一个能给出可采信目标态的条目（在那里绑定并杀掉的替身服务器回答 no server running on <path>）；当两个候选 base 都可采信时，unconfirmed && !confirmedDead 为假 —— 无 WARNING、exit 0、captured: true，持有 pane holder（最长三小时）的活服务器被读成已收割，且对基于 readdir 的孤儿清扫不可见。两条明示不变量同时被打破："一个被推定活着的私有服务器绝不能是静默结局" 与 "仅在服务器已知死亡时 unlink socket"。证据（经真实 runCaptureTui 的翻转探针，tmux 3.4，shim 在 kill 处理器内完成交换，回退 base 以植入的无监听 socket 变为可采信）：未修改的 PR —— {"exitCode": 0, "hasWARNING": false, "swappedOccupantSurvives": false, "stolenLiveSocketSurvives": true}（静默孤儿，被交换的占用者遭 unlink）；在措辞时刻重查身份后 —— {"hasWARNING": true, "stderr": "capture-tui: WARNING — kill-server failed twice; …", "swappedOccupantSurvives": true}；探针确定性地翻转。配套的钉测试 'does not credit a start-base verdict about a socket that is not the stamped one' 无法捕获此处的回归：其 fixture 在 kill 处理器内（即本计算之后）交换，其 WARNING 断言由另一个 base 的不被采信 ENOENT 措辞满足（本轮已用突变体验证）。修复（语义性，跨本处与下方采信 catch）：在措辞时刻重建身份 —— 在采信起始 base 目标态措辞之前重新 lstat 该条目，把"被戳 inode 已消失或已变化"按失败关闭处理为存疑，仅豁免本 base 自己刚刚成功 exit-0 kill 所执行的移除（如上代码示意）。这将竞态窗口从整个 reap（数秒到数十秒，外加免费的第二次尝试）收窄到亚毫秒级并恢复"存疑必响"；对主动竞速者的完全封闭仍是 #9274 的既定工作。配套请加入一个在戳与 reap 之间交换的 fixture，使既有钉测试能够判别该支路。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…t is, and gate the flag pin Two Critical findings from the round against the previous head. One of them is a defect in a test that round added, which is said plainly rather than softened. **The reap's identity arm read a stale snapshot.** `stampedSocketAlive` was computed once before the candidate-base loop, and that loop spans both bases' attempts, a retry and a 15s belt. A daemonized survivor of the captured command — this file's documented same-uid class, and it knows the path from `$TMUX` — waits for the reap, renames the LIVE socket away, and leaves a creditable occupant at the path: the snapshot still said "alive", the replacement's goal-state verdict was credited, and the entry this capture never wrote was unlinked. Exit 0, no WARNING, the real server holding its pane holder for up to three hours and invisible to the readdir-based sweep — both of the invariants this file states out loud, broken by a read taken too early. The identity is now read where the verdict is decided. The old justification for hoisting it does not need the hoist: each base's verdict is evaluated BEFORE that base's unlink, so a read at verdict time can never misread this reap's own removal. What stays open is the rename between that read and the unlink after it — the same interval the entry guard already documents, for the same reason, and now said in this arm's comment too instead of left unmentioned. Moving the read exposed a second bug that the snapshot had been hiding, so it goes with it: the candidate bases were de-duplicated as STRINGS, while `/tmp/`, `/tmp/.` and a TMUX_TMPDIR symlinked to /tmp are different strings naming one base. Two visits were harmless against a snapshot; against a verdict-time read the first visit credits and unlinks and the second reads the socket this reap just removed as a swap, warning about an orphan it reaped itself. They are keyed on the resolved directory now — the rule `cleanup.ts`'s sweep already states for its own scan — and the mutation that restores string keys reproduces exactly that spurious warning. **The `ARTIFACT_OPEN_FLAGS` pin was the one ungated POSIX test in the file.** Node exposes neither `O_NOFOLLOW` nor `O_NONBLOCK` on Windows, which is why the production constant is built with `?? 0` — and asserting them unconditionally makes `x & undefined` fold to 0, so the merge queue's Windows lane would have gone red over two flags that legitimately contribute nothing there, while Linux and macOS stayed green. The two are gated the way production degrades rather than by skipping the whole test, so the three flags that exist everywhere keep their pin on every platform. Verified against a model of the documented win32 constant table: the two assertions drop out, the other three still hold. Three mutation checks, each reproducing its own finding: the pre-loop snapshot restores the silent orphan, string-keyed bases restore the self-inflicted warning, and the win32 model shows the ungated form failing on constants that are absent by design. `npm run build` clean, prettier and eslint clean, and `vitest run --root packages/cli src/commands/review` 4969 passed / 0 failed (4967 before these two tests).
…ui-core No conflicts. `npm run build` clean and `vitest run --root packages/cli src/commands/review` 5128 passed / 0 failed on the result.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally (this host is Linux) — flag-constant and path-shape behaviour in this PR is platform-sensitive.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally — the macOS tmpdir shape affects the long-socket fixture arithmetic.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and did not run locally — the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": none — all layers in scope were walked; the sweep-side consumption (cleanup.ts:715-779) was checked only to the extent needed to separate this finding from F7, ….
Deferred under the convergence posture (round 18, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/cleanup.test.ts:1109 — [probe] EBUSY pin probes literal pid 626262 without the withDeadPids wrapper its siblings usepackages/cli/src/commands/review/capture-tui.ts:211 — [probe] probeOutput's hung-with-code classification arms have no test (round-16 deferral)packages/cli/src/commands/review/cleanup.ts:592 — [review] sweep's quiet ENOENT/ENOTDIR/ELOOP scan branch exercised by zero fixtures (round-16 deferral)packages/cli/src/commands/review/capture-tui.ts:472 — [probe] refuse()'s two-try-block property (stdout JSON survives a throwing stderr write) untesteddocs/users/features/code-review.md:251 — [review] PR description's e2e commands reference a nonexistent bundle/gemini.js (bundle entry is dist/cli.js)packages/cli/src/commands/review/capture-tui.test.ts:3044 (+3 locations) — [probe] fixture host-binary dependencies (/bin/sleep at :3044 and :5952, yes at :5180) false-red or pin nothing on hosts lacking them (round-16 deferral)packages/cli/src/commands/review/capture-tui.ts:2290 — [review] manifest-write-failure cleanup strands THIS run's own torn png (png===null conflates foreign occupant with own landing)packages/cli/src/commands/review/cleanup.test.ts:687 — [probe] manual-reap note's single-quote escape pinned by nothing — quote-carrying TMUX_TMPDIR misparses the paste commandpackages/cli/src/commands/review/cleanup.test.ts:924 — [probe] both lstatSync-throwing arms of the sweep unexercised — deleting the guard's try/catch crashes cleanup greenpackages/cli/src/commands/review/cleanup.ts:782 — [review] reapOrphanedCaptureServers()'s reaped member has no read site — dead contract member (round-16 deferral)packages/cli/src/commands/review/lib/tui-capture.test.ts:124 — [probe] goalStatePath's space-bearing-base capture unpinned despite its load-bearing declaration commentpackages/cli/src/commands/review/capture-tui.test.ts:129 — [review] two rationale comment blocks displaced from the declarations they describepackages/cli/src/commands/review/capture-tui.test.ts:374 — [probe] long-socket fixture's 104-byte property silently fails on long-TMPDIR hosts — the off-by-one mutant escapes on darwinpackages/cli/src/commands/review/capture-tui.ts:446 — [probe] guardBrokenPipes' pre-completion rethrow branch pinned by no test — deleting the throw ships greenpackages/cli/src/commands/review/cleanup.ts:685 — [review] sweep's kill retry omits the sibling reap's probe-verified 100ms inter-attempt pausepackages/cli/src/commands/review/cleanup.test.ts:985 — [probe] sweep's realpath-keyed base dedup exercised by zero fixtures — both dedup mutants ship green (round-16 deferral)
Convergence: round 18 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/cli/src/commands/review/capture-tui.test.ts (findings in round 17; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally (this host is Linux) — flag-constant and path-shape behaviour in this PR is platform-sensitive。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally — the macOS tmpdir shape affects the long-socket fixture arithmetic。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and did not run locally — the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：none — all layers in scope were walked; the sweep-side consumption (cleanup.ts:715-779) was checked only to the extent needed to separate this finding from F7, …。
收敛姿态下延后（第 18 轮，非阻断）——已记录，本轮不要求修改：共 16 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 18 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/capture-tui.test.ts（第 17 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| p="\${TMUX_TMPDIR}/tmux-$(id -u)/$SRV" | ||
| # The swap lands DURING the kill — after any pre-loop snapshot, before | ||
| # the verdict this answer produces. | ||
| if [ "$TMUX_TMPDIR" = "${envBase}" ]; then rm -f "$p"; : > "$p"; fi |
There was a problem hiding this comment.
[Critical] The 'reads the stamped identity at VERDICT time' pin — added by the R17-2 fix commit — fails deterministically on inode-reusing filesystems, and its fixture exposes a weakness in the production identity check. The fixture lands its socket swap via rm -f "$p"; : > "$p", relying on the replacement getting a NEW inode — but production stampedSocketAlive() (capture-tui.ts:1361-1371) compares .ino only, and ext-family filesystems reuse the freed inode on immediate same-directory recreation (measured 5/5 on the review host). The identity reads "alive", the goal-state verdict is credited, no WARNING prints, and expect(stderr).toContain('WARNING') is red while tmpfs lanes stay green — consistent with Test (ubuntu-latest, Node 22.x) currently failing in CI. The same construct also defeats the production defense it pins: the documented same-uid survivor can rm the live socket and recreate a plain file at the path; wherever the inode is reused, the verdict-time check passes, the verdict is credited, and the server is orphaned silently. The sibling shim at :2488 shares the construct but stays green only because its fallback-base ENOENT arm independently forces the WARNING.
Witness (4/4 reproductions on the PR worktree, ext4 /tmp):
× reads the stamped identity at VERDICT time, not once before the loop
expected 'capture-tui: png at 80x24 (settled by…' to contain 'WARNING'
Tests 1 failed | 169 passed | 1 skipped
inode probe on the same fs: rm + recreate → ino before=7214321 after=7214321 (reused)
A/B: moving only the fixture base to /dev/shm (tmpfs) → the same test passes
| if [ "$TMUX_TMPDIR" = "${envBase}" ]; then rm -f "$p"; : > "$p"; fi | |
| if [ "$TMUX_TMPDIR" = "${envBase}" ]; then q="$p.swap"; : > "$q"; rm -f "$p"; mv -f "$q" "$p"; fi |
Landing the swap by rename of a pre-created file guarantees a new inode regardless of allocator. Apply the same at :2488, and independently consider strengthening stampedSocketAlive() beyond bare inode equality (size/mtime, the way changed() does).
中文说明
R17-2 修复提交新增的 'reads the stamped identity at VERDICT time' pin 测试在复用 inode 的文件系统上确定性失败，且其 fixture 暴露了生产代码身份检查的弱点。fixture 用 rm -f "$p"; : > "$p" 完成 socket 替换，依赖替换者获得新 inode；但生产代码 stampedSocketAlive()（capture-tui.ts:1361-1371）只比较 .ino，而 ext 系文件系统在同目录立即重建时会复用刚释放的 inode（评审主机实测 5/5）。身份检查读作"存活"，goal-state 裁决被采信，不打印 WARNING，expect(stderr).toContain('WARNING') 变红，而 tmpfs 通道保持绿色——这也与 CI 中 Test (ubuntu-latest, Node 22.x) 目前的失败相符。同一构造还击败了它所 pin 的生产防御：文档所述同 uid 幸存者可以 rm 掉存活 socket 并在原路径重建一个普通文件；凡复用 inode 之处，裁决时刻检查照样通过，裁决被采信，服务器被静默孤立。:2488 的兄弟 shim 使用同一构造却保持绿色，仅因其 fallback-base 的 ENOENT 分支独立强制了 WARNING。
证据（PR worktree 上 4/4 复现，ext4 /tmp）：测试 × reads the stamped identity at VERDICT time… 失败，expected '…' to contain 'WARNING'，Tests 1 failed | 169 passed | 1 skipped；同文件系统 inode 探针：rm + 重建 → 前后 ino 相同（被复用）；A/B：仅把 fixture 基目录移到 /dev/shm（tmpfs）→ 同一测试通过。
建议修复：用"先创建再 rename"的方式落地替换（无论分配器行为如何都保证新 inode），并对 :2488 做同样处理；独立地，考虑把 stampedSocketAlive() 加强到超越裸 inode 相等（像 changed() 那样比较 size/mtime）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if ( | ||
| entryStat.isSymbolicLink() || | ||
| !entryStat.isSocket() || | ||
| entryStat.nlink > 1 | ||
| ) { |
There was a problem hiding this comment.
[Critical] The entry guard rejects three planted entrances (symlink, hard link, non-socket) but is structurally blind to a fourth: a live foreign socket RENAMEd into a capture-shaped name before the scan — race-free, stable at scan time. A renamed socket is still a plain socket with nlink 1, so it passes all three checks; CAPTURE_SERVER_NAME_RE matches the attacker-chosen digits/hex; the attacker-chosen dead pid answers ESRCH; the pinned kill-server connects inode-addressed to the victim; and the post-kill identity re-check compares the entry against itself (the rename preceded the scan, so nothing changes mid-window) — no WARNING can fire. The sweep then unlinks and prints Reaped orphaned capture server: … with exit 0 — the exact outcome the guard's own comment claims to foreclose ("including the user's own tmux server … reports 'Reaped' while the victim is destroyed"). Unlike a check-then-syscall race, no window is raced — this is a stable state the sweep's inference deterministically misreads, inside the sweep's own stated guarantee. The planter is this PR's own documented daemonized-descendant class: rename("/tmp/tmux-<uid>/default", "/tmp/tmux-<uid>/qwen-review-capture-<deadpid>-<hex>") plus a guaranteed-dead pid is trivial. Reported independently by two auditors, then probe-verified.
Witness (live probe driving the real sweep on tmux 3.4, deterministic 4/4):
PRE: victim server pid=2258685 socket=…/ra1-victim ino=7339541 isSocket=true nlink=1
PRE: renamed entry qwen-review-capture-2258689-abcdef12 ino=7339541 (same as victim: true)
sweep printed: Reaped orphaned capture server: qwen-review-capture-2258689-abcdef12 (stderr empty)
POST: /proc/2258685/stat state="GONE" | victim alive=false | WARNING=false | exitCode=0
flip (fail-closed mutant): victim alive=true, socket intact, no "Reaped" —
note: not reaping … cannot verify this socket belongs to a capture this tool started
Establish server identity before the destructive kill: capture-tui can record the tmux server pid after start (e.g. display-message -p '#{pid}') somewhere the sweep can read, and the sweep queries the answering server's own pid and requires a match before kill-server. When a live server answers under a capture-shaped name and identity cannot be proven, refuse loudly (note: not reaping …: identity cannot be proven) instead of killing.
中文说明
该入口守卫拒绝了三种植入入口（符号链接、硬链接、非 socket），但对第四种结构性失明：一个存活的外部 socket 在扫描之前被重命名为采集形态的名字——无需竞速，扫描时状态即已稳定。被重命名的 socket 仍是 nlink 为 1 的普通 socket，通过全部三项检查；CAPTURE_SERVER_NAME_RE 匹配攻击者自选的数字/十六进制；攻击者选定的死 pid 以 ESRCH 应答；钉住环境的 kill-server 按 inode 连接到受害者；而 kill 后的身份复查是条目与自身比较（rename 先于扫描，窗口内没有任何变化）——不可能触发 WARNING。清扫随后 unlink 并以 exit 0 打印 Reaped orphaned capture server: …——这正是守卫自身注释声称要杜绝的结局（"包括用户自己的 tmux server……受害者被摧毁却报告 'Reaped'"）。与"检查到系统调用"之间的竞速不同，这里没有任何窗口被竞速——这是一个稳定状态，清扫的推理确定性地误读它，且落在清扫自身声明的担保之内。植入者正是本 PR 文档化的守护化后代类：一次 rename(...) 加一个必死 pid 即可。两位审计者独立发现，随后探针验证。
证据（真实清扫在 tmux 3.4 上的活体探针，4/4 确定性）：受害者服务器 pid=2258685、ino=7339541；重命名后的条目与其同 inode；清扫打印 "Reaped orphaned capture server: …"，stderr 为空；事后受害者进程消失、无 WARNING、exit 0。翻转（失败即关闭的突变体）：受害者存活、socket 保留、不打印 "Reaped"，改为输出 "note: not reaping … cannot verify…"。
建议修复：在破坏性 kill 之前确立服务器身份——例如 capture-tui 在启动后记录 tmux 服务器 pid（如 display-message -p '#{pid}'）于清扫可读之处，清扫查询应答服务器自身的 pid 并要求匹配后才执行 kill-server；当存活服务器以采集形态名字应答而身份无法证明时，高声拒绝而不是 kill。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…mped socket One Critical from the round against the previous head, and it lands on both sides of a test the previous round added. The reap's identity check compared `.ino` alone. An inode is not a durable name for a FILE: ext-family allocators hand the freed number straight back on an immediate same-directory recreate (measured 5/5 on the review host), so `rm` + recreate at the socket path read as "still ours", the goal-state verdict about the REPLACEMENT was credited, and the server was orphaned silently. Two of the three identity comparisons in this PR were already wider than that one — the sweep's post-kill re-check takes mode, `changed()` takes size and mtime — which left the reap's own check the narrowest of the three while guarding the most. It now compares ino, mode and mtime, through an exported `isSameSocket` so the comparison can be pinned at all: no behavioural arm can reach it, because a test cannot ask a filesystem to reuse an inode on demand and every filesystem a fixture can rely on hands out a fresh one. Reverting it to the inode alone turns that pin red. The same construct made the round's own fixture non-deterministic: it landed its swap with `rm -f "$p"; : > "$p"` and depended on getting a new inode, so it passed on APFS and tmpfs and failed on ext4 — a pin that holds only where the allocator happens to cooperate. Both fixtures that use it now swap by RENAME of a pre-created file, which gives a new inode regardless of allocator (verified on this host: before=438972309 after=438972310). `npm run build` clean, prettier and eslint clean, and `vitest run --root packages/cli src/commands/review` 5129 passed / 0 failed.
…ui-core No conflicts. main adds an MCP dependency, so a stale install fails the build with 'Cannot find module @modelcontextprotocol/client' — `npm ci` first. `npm run build` clean and `vitest run --root packages/cli src/commands/review` 5129 passed / 0 failed on the result.
…ui-core One conflict, the usual shape in this file: two independent `it`s added at the same seam — this branch's capture-tui wiring pin and main's #9789 do-not-refute pin. Both kept. Picks up the `cd-cua-driver.yml` baseline correction (#9822), so the `Test` job's step-7 gate stops failing this branch's merge ref before any dependency is installed — verified on the merged tree, the gate now answers "every workflow file is under the gate". `npm run build` clean and `vitest run --root packages/cli src/commands/review` 5130 passed / 0 failed.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] R14-2 (comment 3834716946, cleanup.ts post-kill identity re-check): the premise — some tmux version unlinks the socket after a clean kill-server, making a benign reap print a false WARNING — cannot be ruled on from the code: contradicted by…
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally (this host is Linux); tmux/socket/PATH behavior in this PR is platform-sensitive.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally (this host is Linux); flag-constant and path-shape behavior in this PR is platform-sensitive.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and did not run locally; the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites.
Not reviewed: reverse audit — reached the round cap of 3 without converging; the cap round reported two Criticals, verified after the cap.
Deferred under the convergence posture (round 19, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/capture-tui.ts:1345 — [probe] The reap's 'warning-free when the server never came up'…packages/cli/src/commands/review/cleanup.ts:765 — [probe] The sweep's post-kill identity re-check detects a swapped…
Convergence: round 19 posted 3 inline comment(s), 2 of them reported for the first time; the previous round posted 2 (2 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally (this host is Linux); tmux/socket/PATH behavior in this PR is platform-sensitive。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally (this host is Linux); flag-constant and path-shape behavior in this PR is platform-sensitive。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and did not run locally; the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites。
未审查：reverse audit — reached the round cap of 3 without converging; the cap round reported two Criticals, verified after the cap。
收敛姿态下延后（第 19 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 19 轮发布了 3 条行内评论，其中 2 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (resolve(base) === startBase || socketStamp === undefined) { | ||
| confirmedDead = true; | ||
| } |
There was a problem hiding this comment.
[Critical] The exit-0 credit here treats a successful kill pinned at the start base as proof the start base was examined, but the catch branch's own comment below documents that a base destroyed mid-window sends the tmux client to /tmp — so a fallback kill that succeeds under /tmp is credited to the start base here, silencing the orphan WARNING over a possibly-alive server. The failure-wording path defends with verdictExaminedBase; this success path has no equivalent check on where the client actually looked.
Failure scenario: TMUX_TMPDIR points at a user-writable scratch base — the shape the socket-length gate's own comment calls "not an exotic shape at all". The captured command (untrusted same-uid code that knows the unique socket name from $TMUX, per this file's own threat model) destroys the base mid-window and binds a sacrificial server at /tmp/tmux-<uid>/<that exact name>. At reap, the kill pinned with TMUX_TMPDIR=<destroyed base> falls back to /tmp, kills the sacrificial server, exits 0, and this branch sets confirmedDead because resolve(base) === startBase. This run's own server — alive behind its rm'd socket under the destroyed base, unreachable by -L and invisible to the cleanup sweep — is orphaned with no WARNING at exit 0, against this file's explicit never-a-silent-outcome policy.
Witness (E2E through the real runCaptureTui, tmux 3.4):
exitCode = undefined, stdout {"captured":true,…}, no WARNING on stderr
real server pid alive-after-reap = true | sacrificial pid dead
flip probe (re-run the start gate's usability test on `base` before crediting;
unusable → unconfirmed): stderr "WARNING — kill-server failed twice;
the private tmux server … may still be running"
Before crediting an exit-0 kill on the start base, re-run the same usability test the start gate uses on base (statSync(base).isDirectory() + accessSync(base, W_OK | X_OK)); if the base is not usable at reap time the client could not have examined it, so treat the success as unconfirmed and fall through to the WARNING.
中文说明
此处的 exit-0 采信把「钉在 start base 上且成功的 kill」当作 start base 已被检查过的证据，但下方 catch 分支自己的注释明确记录：窗口中被销毁的 base 会把 tmux 客户端送到 /tmp——因此在 /tmp 下成功的回退 kill 会在这里被记到 start base 名下，压掉本应针对可能存活服务器的孤儿 WARNING。失败措辞路径有 verdictExaminedBase 防御；这条成功路径对「客户端实际看的是哪里」没有任何等价检查。
触发场景：TMUX_TMPDIR 指向用户可写的 scratch base——socket 长度门槛自己的注释称之为「一点也不罕见的形态」。被采集命令（按本文件自己的威胁模型，是从 $TMUX 得知唯一 socket 名的同 uid 不受信代码）在窗口中销毁该 base，并在 /tmp/tmux-<uid>/<同名> 绑一个替身服务器。reap 时，钉住 TMUX_TMPDIR=<已销毁 base> 的 kill 回退到 /tmp，杀掉替身，exit 0，此分支因 resolve(base) === startBase 置 confirmedDead。本次运行自己的服务器——存活在已销毁 base 下被 rm 掉的 socket 之后，-L 够不到、清扫的 readdir 也看不见——在 exit 0 时被静默孤立、无任何 WARNING，违背本文件明确的「绝不静默收场」承诺。
证据（经真实 runCaptureTui 的 E2E，tmux 3.4）：exitCode = undefined，stdout {"captured":true,…}，stderr 无 WARNING；真实服务器 pid 在 reap 后仍存活，替身已死。翻转探针（采信前对 base 重跑启动门槛的可用性测试；不可用 → 不采信）：stderr 打印 "WARNING — kill-server failed twice; the private tmux server … may still be running"。
建议修复：在对 start base 上的 exit-0 kill 采信之前，重跑启动门槛对 base 使用的同一可用性测试；若 reap 时刻 base 不可用，则客户端不可能检查过它，应把该成功视为未确认并落入 WARNING。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const staged = lstatSync(ansStage); | ||
| if (!staged.isFile() || staged.ino !== ansWritten.ino) { | ||
| throw new Error('staged render input is not the .ans this run wrote'); |
There was a problem hiding this comment.
[Critical] The render stage pins its input by inode alone, narrower than every other identity comparison this PR writes — changed() takes ino+size+mtimeMs, and isSameSocket takes ino+mode+mtimeMs with the justification that an inode is not a durable name for a file: ext-family allocators return the freed number on an immediate same-directory recreate. The stamp→linkSync window spans the whole freeze availability probe (a 10s-belted spawnSync) plus the PATH walk, so the guarantee this staging exists for — "a swap of ansPath during the probe window cannot feed foreign bytes to the render" — fails open for the rm+recreate shape.
Failure scenario: a concurrent same-uid actor (a daemonized survivor of the captured command) rm's <out>.ans between ansWritten = stampOf(ansPath) and linkSync and recreates it with attacker bytes; the allocator hands back the freed inode (measured 30/30 on the review host), so staged.ino === ansWritten.ino passes and freeze renders the attacker's bytes into <out>.png. The run exits 0 with captured:true, evidence:'png' and a manifest naming both artifacts — a complete evidence forgery credited at the publishable rung.
Witness (real runCaptureTui, swap injected inside the freeze-probe window):
inode reused by rm+recreate = true (before=14554280, after=14554280)
stdout {"captured":true,"evidence":"png",…} rendered bytes = attacker-forged
control (symlink swap, same window): refused — evidence 'ans-only',
"cap.ans was replaced while the render was being prepared"
flip (full-stamp comparison below): evidence 'ans-only', attacker bytes not credited
| const staged = lstatSync(ansStage); | |
| if (!staged.isFile() || staged.ino !== ansWritten.ino) { | |
| throw new Error('staged render input is not the .ans this run wrote'); | |
| const staged = lstatSync(ansStage); | |
| if ( | |
| !staged.isFile() || | |
| staged.ino !== ansWritten.ino || | |
| staged.size !== ansWritten.size || | |
| staged.mtimeMs !== ansWritten.mtimeMs | |
| ) { | |
| throw new Error('staged render input is not the .ans this run wrote'); |
中文说明
渲染暂存只按 inode 钉住输入，比本 PR 写的其他所有身份比较都窄——changed() 比较 ino+size+mtimeMs，isSameSocket 比较 ino+mode+mtimeMs，其理由正是「inode 不是文件的持久名字」：ext 系分配器在同目录立即重建时会归还刚释放的编号。stamp→linkSync 的窗口横跨整个 freeze 可用性探针（10 秒腰带的 spawnSync）加 PATH 行走，因此这套暂存机制存在所要保证的「探针窗口内替换 ansPath 不能把外来字节喂给渲染」，在 rm+重建形态下是失效的。
触发场景：并发的同 uid 行为者（被采集命令守护化的后代）在 ansWritten = stampOf(ansPath) 与 linkSync 之间 rm 掉 <out>.ans 并以攻击者字节重建；分配器归还被释放的 inode（评审主机实测 30/30），staged.ino === ansWritten.ino 通过，freeze 把攻击者字节渲染进 <out>.png。运行以 exit 0、captured:true, evidence:'png' 收场，清单同时列出两个产物——在可发布档位上完成了一次彻底的证据伪造。
证据（真实 runCaptureTui，替换注入在 freeze 探针窗口内）：rm+重建复用 inode = true（前后 ino 相同）；stdout {"captured":true,"evidence":"png",…}，渲染字节为攻击者伪造；对照（同窗口符号链接替换）：拒绝——evidence 'ans-only'、"cap.ans was replaced…"；翻转（下方的完整 stamp 比较）：evidence 'ans-only'，攻击者字节不被采信。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if ( | ||
| entryStat.isSymbolicLink() || | ||
| !entryStat.isSocket() || | ||
| entryStat.nlink > 1 | ||
| ) { |
There was a problem hiding this comment.
[Critical] R18-2: The entry guard rejects three planted entrances (symlink, hard link, non-socket) but is structurally blind to a fourth: a live foreign socket RENAMEd into a capture-shaped name before the scan — race-free, stable at scan time. A renamed socket is still a plain socket with nlink 1, so it passes all three checks; CAPTURE_SERVER_NAME_RE matches the attacker-chosen digits/hex; the attacker-chosen dead pid answers ESRCH; the pinned kill-server connects inode-addressed to the victim; and the post-kill identity re-check compares the entry against itself (the rename preceded the scan, so nothing changes mid-window) — no WARNING can fire. The sweep then unlinks and prints Reaped orphaned capture server: … with exit 0 — the exact outcome the guard's own comment claims to foreclose ("including the user's own tmux server … reports 'Reaped' while the victim is destroyed"). Still standing at the reviewed commit: this file is unchanged since round 18 filed it; the planter is this PR's own documented daemonized-descendant class — one rename() plus a guaranteed-dead pid.
Witness (live probe driving the real sweep on tmux 3.4, deterministic 4/4, round 18):
PRE: victim server pid=2258685 socket ino=7339541 isSocket=true nlink=1
renamed entry qwen-review-capture-2258689-abcdef12 ino=7339541
sweep printed: Reaped orphaned capture server: qwen-review-capture-2258689-abcdef12
(stderr empty)
POST: victim alive=false | WARNING=false | exitCode=0
flip (fail-closed mutant): victim alive=true, socket intact,
"note: not reaping … cannot verify this socket belongs to a capture this tool started"
Establish server identity before the destructive kill: capture-tui can record the tmux server pid after start (e.g. display-message -p '#{pid}') somewhere the sweep can read, and the sweep queries the answering server's own pid and requires a match before kill-server. When a live server answers under a capture-shaped name and identity cannot be proven, refuse loudly (note: not reaping …: identity cannot be proven) instead of killing.
中文说明
R18-2：入口守卫拒绝了三种植入入口（符号链接、硬链接、非 socket），但对第四种结构性失明：一个存活的外部 socket 在扫描之前被重命名为采集形态的名字——无需竞速，扫描时状态即已稳定。被重命名的 socket 仍是 nlink 为 1 的普通 socket，通过全部三项检查；CAPTURE_SERVER_NAME_RE 匹配攻击者自选的数字/十六进制；攻击者选定的死 pid 以 ESRCH 应答；钉住环境的 kill-server 按 inode 连接到受害者；而 kill 后的身份复查是条目与自身比较（rename 先于扫描，窗口内没有任何变化）——不可能触发 WARNING。清扫随后 unlink 并以 exit 0 打印 Reaped orphaned capture server: …——这正是守卫自身注释声称要杜绝的结局（「包括用户自己的 tmux server……受害者被摧毁却报告 'Reaped'」）。在受审提交上仍然存在：本文件自第 18 轮提出以来未变；植入者正是本 PR 文档化的守护化后代类——一次 rename() 加一个必死 pid 即可。
证据（真实清扫在 tmux 3.4 上的活体探针，4/4 确定性，第 18 轮）：受害者服务器 pid=2258685、ino=7339541；重命名后的条目与其同 inode；清扫打印 "Reaped orphaned capture server: …"，stderr 为空；事后受害者进程消失、无 WARNING、exit 0。翻转（失败即关闭的突变体）：受害者存活、socket 保留，输出 "note: not reaping … cannot verify…"。
建议修复：在破坏性 kill 之前确立服务器身份——例如 capture-tui 在启动后记录 tmux 服务器 pid（如 display-message -p '#{pid}'）于清扫可读之处，清扫查询应答服务器自身的 pid 并要求匹配后才执行 kill-server；当存活服务器以采集形态名字应答而身份无法证明时，高声拒绝而不是 kill。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Confirmed, and the code overclaimed — that half is now fixed. But after investigating whether it is closable, the decision is to state the boundary rather than add machinery, and here is the reasoning in full.
Why no identity check closes it. The suggested fix (record the tmux server pid, match before kill) is same-uid writable — the same process that renames the socket overwrites the record. A server-shape probe (does the pane run the holder script?) is same-uid craftable — the adversary drives their own decoy, or the user's own server, to present that shape. Same uid is same authority: there is no signal a name-addressed sweep can read that this adversary cannot forge. Each proposed fix raises the bar and closes nothing, while manufacturing the false security this PR's contract is explicitly written to avoid.
Why it is out of scope, not unhandled. capture-tui's file header already states this exact boundary: it makes no guarantee against an active same-uid adversary redirecting a name, because Node exposes no *at() syscalls so every path resolves by name — and it names #9274 as the hardening with its own guarantees and tests. The rename is that adversary. What was wrong was the sweep's local comments claiming more than the header: the entry-guard implied it forecloses redirection to the user's server, and the post-kill re-check claimed to make "a won swap loud" — neither holds for a rename already in place at scan time. Both comments now state the boundary instead (c193cc3567).
Why the stakes are bounded. The sweep is an optimization: an orphaned capture server self-terminates within the holder's three-hour bound regardless. It reclaims that early; it is not the only reaper.
So the type guard closes every redirection that needs no rename (symlink, hard link, non-socket — the passive and cheap-plant shapes), which is all a name-addressed sweep can close, and the residual is the documented non-goal. Hardening it belongs in #9274, where the header already directs it.
…write keeps the inode Two Critical findings, both of them the previous round's own reasoning not carried to its sibling. **A kill pinned at the start base is not a kill that examined it.** `-L` sets the base in the client's environment; it does not bind tmux to it, and an unusable base sends the client to /tmp. The failure path already knew this — it checks `verdictExaminedBase` before believing a wording — while the success path credited any exit 0 to the base it was aimed at. So a captured command that destroys the base mid-window and binds a sacrificial server at this run's unique name under /tmp got its sacrifice killed, exit 0, and this run's own server — alive behind its removed socket, unreachable by `-L` and invisible to the readdir sweep — orphaned at exit 0 with nothing on stderr. A success from a base the client could not have examined now ends that base's attempts without establishing anything, which is what leaves the doubt the WARNING exists for; merely withholding the global credit would have been silent in both directions. The WARNING names this cause rather than saying "kill-server failed twice", which would send an operator hunting a wedged server. The two-part usability test moves into `baseIsUsable` so the reap and the start gate cannot drift — the gate now calls it too. **The render stage pinned its input by inode alone.** Last round widened `isSameSocket` on the grounds that an inode is not a durable name for a file, and left the staging check next to it comparing `.ino` and nothing else — narrower than both siblings (`changed()` takes ino+size+mtime). The window it guards spans the whole freeze availability probe, and it does not even need allocator reuse to fail: an in-place rewrite keeps the inode BY DEFINITION, so truncating and rewriting `<out>.ans` in that window put attacker bytes through `linkSync`, through freeze, and into a manifest crediting `evidence: "png"` — an evidence forgery reported as success. It compares ino, size and mtime now. Two tests, both behavioural and both mutation-checked. The in-place rewrite one asserts its own premise (the inode survived the rewrite) so it cannot quietly stop testing what it is named for. `npm run build` clean, prettier and eslint clean, and `vitest run --root packages/cli src/commands/review` 5132 passed / 0 failed. The round's third Critical — a live foreign socket RENAMEd into a capture-shaped name, which passes all three of the sweep's entrance checks — is unchanged and still open: closing it needs a design decision about recorded server identity that is the maintainer's to make, and is asked separately.
…ui-core One conflict, and not the usual both-sides-added shape: main refactored Step 7 out of SKILL.md into `references/posting.md`, deleting 385 lines this branch still carried — and this branch's one edit to that file sat inside the deleted block. Taking either side alone loses something, so main's refactor is taken whole and the branch's sentence is re-applied where its paragraph now lives: `references/posting.md` names `qwen review capture-tui` as the sanctioned producer for terminal-rendering evidence, in the paragraph about judging an image's content. `npm run build` clean, `vitest run --root packages/cli src/commands/review` 5151 passed / 0 failed, and the skill suites (`--root packages/core src/skills`) 393 passed / 0 failed.
…ed reap can promise R18-2, re-filed each round: a same-uid process renames a live foreign socket into a capture-shaped name before the sweep scans; it is a plain socket with one link and a chosen-dead pid, so it passes all three entry checks and the pinned kill-server destroys the victim under a "Reaped" line. The finding is correct that the code overclaimed, and correct that it is real — and it is exactly the active-same-uid boundary capture-tui's own header already states as a non-goal (Node exposes no *at() syscalls, so every path resolves by name and a name can be redirected). The sweep's local comments did not carry that boundary: the entry-guard comment implied it forecloses redirection "including the user's own tmux server," and the post-kill re-check claimed to make "a won swap loud" — neither is true for a rename that is already in place at scan time (a plain socket indistinguishable from a real orphan; the re-check then compares the entry against itself and is silent). Decision, after investigating whether it is closable: it is not, from here. Every identity signal a sweep could add is within the same adversary's reach — an on-disk server-pid record is same-uid writable, and the answering server's own shape is same-uid craftable — so recording a pid (the suggested fix) or probing the server's shape raises the bar without closing the model, while manufacturing the false security this PR's contract is written to avoid. The sweep is also an optimization: the orphan self-terminates within the holder's three-hour bound regardless. So the honest change is to make the comments state the boundary the header already draws, not to grow the diff with machinery that does not hold. Real hardening is #9274's, as the header says. Comment-only; no behavior change. `npm run build` clean, prettier and eslint clean, `vitest run cleanup.test.ts` 97 passed / 0 failed.
…ui-core No conflicts. npm run build clean and vitest run --root packages/cli src/commands/review 5151 passed / 0 failed.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] R14-2 (comment 3834716946, cleanup.ts post-kill identity re-check): the premise — some tmux version unlinks the socket after a clean kill-server, making a benign reap print a false WARNING — still cannot be ruled on from the code: contradic…
Not reviewed: reverse audit — stopped at the round cap of 3 without converging; the cap round still surfaced findings (the loop did not converge).
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the macOS lane did not run locally; tmux/socket/PATH behavior in this PR is platform-sensitive.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the Windows lane did not run locally; flag-constant and path-shape behavior in this PR is platform-sensitive.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites.
Not reviewed: test-efficacy — the probe harness baseline failed its build-prerequisite guard, so mutant and hunk-revert probes were skipped (harnessValidated: null — neither validated nor refuted).
Deferred under the convergence posture (round 20, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/cleanup.ts:772 — [review] paste-targeted manual-reap note embeds trailing prose inside the parenthesized command — the pasted command failspackages/cli/src/commands/review/capture-tui.test.ts:6141 — [review] orphan-reap test hardcodes /bin/sleep as the symlink target — dangling on hosts without itpackages/cli/src/commands/review/lib/tui-capture.ts:158 — [review] orphaned JSDoc block about the private server name sits above resolveOnPath's doc, attached to nothingpackages/cli/src/commands/review/cleanup.test.ts:477 — [probe] sweep fixtures mint 3-4-hex nonces while production mints 8 — a length-narrowing mutant ships green and silently no-ops the sweeppackages/cli/src/commands/review/capture-tui.ts:1019 — [review] probeOutput's coded hung-classification wordings asserted by no testpackages/cli/src/commands/review/cleanup.ts:1023 — [review] sweep's load-bearing position before the redirect refusal is pinned by no testpackages/cli/src/commands/review/cleanup.test.ts:1245 — [probe] realpath-keyed base de-dup exercised but unpinned; realpathSync unmockedpackages/cli/src/commands/review/cleanup.test.ts:507 — [probe] the sweep's quiet ENOENT scan arm is executed by zero fixturespackages/cli/src/commands/review/cleanup.test.ts:925 — [probe] post-kill re-check's entry-vanished catch arm unpinnedpackages/cli/src/commands/review/capture-tui.test.ts:375 — [probe] long-socket fixture's pad clamps on long-tmpdir hosts — the gate's 103 bound undiscriminated therepackages/cli/src/commands/review/capture-tui.test.ts:2950 — [probe] realprobe fixture pins the resolver short-circuit only; the broken-shebang arm is unexercised and the comment overclaimspackages/cli/src/commands/review/lib/tui-capture.test.ts:163 — [probe] symlink-canonicalization test pins only the base-side realpath on the Linux lanepackages/cli/src/commands/review/capture-tui.test.ts:130 — [review] two rationale comment blocks displaced from the declarations they describepackages/cli/src/commands/review/capture-tui.test.ts:4658 — [review] directory-survival fixture's comment pins a torn-png EISDIR swallow that does not exist in this diffpackages/cli/src/commands/review/capture-tui.ts:2469 — [review] --timeout-ms help understates the wall bound by >= ~75s of belted phasespackages/cli/src/commands/review/cleanup.test.ts:1109 — [review] literal-pid 626262 fixture without withDeadPids on a pid_max-4194304 lanepackages/cli/src/commands/review/cleanup.test.ts:699 — [probe] the manual-reap note's single-quote escape is pinned by no quote-carrying fixturepackages/cli/src/commands/review/cleanup.ts:772 — [review] paste advice prints a bare tmux name though the resolved absolute tmuxBin is in scopepackages/cli/src/commands/review/cleanup.ts:547 — [review] the sweep's returned reaped member has no read site; set true even in the entryChanged/WARNING arm
Convergence: round 20 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 3 (2 new). Findings keep coming back to the same files: packages/cli/src/commands/review/capture-tui.ts (findings in round 19; 3 more now); packages/cli/src/commands/review/cleanup.ts (findings in round 18; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查：reverse audit — stopped at the round cap of 3 without converging; the cap round still surfaced findings (the loop did not converge)。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the macOS lane did not run locally; tmux/socket/PATH behavior in this PR is platform-sensitive。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the Windows lane did not run locally; flag-constant and path-shape behavior in this PR is platform-sensitive。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites。
未审查：test-efficacy — the probe harness baseline failed its build-prerequisite guard, so mutant and hunk-revert probes were skipped (harnessValidated: null — neither validated nor refuted)。
收敛姿态下延后（第 20 轮，非阻断）——已记录，本轮不要求修改：共 19 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 20 轮发布了 4 条行内评论，其中 4 条是首次提出；上一轮发布了 3 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/capture-tui.ts（第 19 轮已出过发现，本轮又有 3 条）；packages/cli/src/commands/review/cleanup.ts（第 18 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| (resolve(base) === startBase && | ||
| (socketStamp === undefined || !isStampedSocket(entry))); |
There was a problem hiding this comment.
[Critical] R20-1: With a stamp present, the bind is provably on the start base (the confirmedDead rationale says exactly that), yet the candidate-base loop still connects the pinned kill-server on the OTHER bases, where the planted-entry check skips the identity arm (it is gated on resolve(base) === startBase). A plain socket renamed into the capture-shaped name passes the remaining symlink/nlink tests, so the kill destroys the foreign server — possibly the user's own.
Trigger: scratch TMUX_TMPDIR (the CI shape the length-gate comment names); start succeeds and stamps; the start-base kill succeeds first, setting confirmedDead, which silences the WARNING gate; the captured command (the file's own threat model calls it untrusted same-uid code that learns the socket name from $TMUX) renames the user's own server socket onto this run's server name under /tmp; the loop visits /tmp, lstat sees a plain socket nlink=1 → not planted → kill-server destroys the user's own server — exit 0, no WARNING, success manifest. The headline property (a capture cannot kill the user's own sessions) fails silently, and no check-vs-syscall race is needed.
Witness (live tmux 3.4, real runCaptureTui):
ARM A (PR code): USER-SERVER: DEAD after capture — exit 0, WARNING lines: 0,
manifest {"captured":true,"evidence":"png"}
ARM B (scratch tree + the one-line fix below): USER-SERVER: ALIVE — the probe flips
The fix uses only knowledge the run ALREADY has — its own stamp proves where the bind happened — not an identity signal an adversary can forge:
| (resolve(base) === startBase && | |
| (socketStamp === undefined || !isStampedSocket(entry))); | |
| (resolve(base) === startBase && | |
| (socketStamp === undefined || !isStampedSocket(entry))) || | |
| (socketStamp !== undefined && resolve(base) !== startBase); |
中文说明
[Critical] R20-1：有 stamp 时，bind 可证明发生在 start base 上（confirmedDead 的理由注释正是这么说的），但候选 base 循环仍会在其他 base 上连接钉住环境的 kill-server——那些 base 上的植入入口检查跳过了身份臂（它以 resolve(base) === startBase 为条件）。被重命名为采集形态名字的普通 socket 能通过其余的 symlink/nlink 检查，于是 kill 摧毁外部服务器——可能是用户自己的。
触发：scratch TMUX_TMPDIR（长度门槛注释称之为 CI 常见形态）；启动成功并打下 stamp；start base 上的 kill 先成功、置 confirmedDead、压掉 WARNING 门；被采集命令（本文件威胁模型自己文档化的同 uid 不受信代码，从 $TMUX 得知 socket 名）把用户自己的服务器 socket 重命名到 /tmp 下本运行的服务器名；循环访问 /tmp，lstat 看到 nlink=1 的普通 socket → 非植入 → kill-server 摧毁用户自己的服务器——exit 0、无 WARNING、成功清单。「采集不会杀掉用户自己的会话」这一头条属性静默失败，且不需要任何检查-系统调用竞速。
证据（tmux 3.4 活体，真实 runCaptureTui）：ARM A（PR 代码）：采集后 USER-SERVER: DEAD——exit 0、WARNING 0 行、清单 {"captured":true,"evidence":"png"}；ARM B（scratch 树 + 下方单行修复）：USER-SERVER: ALIVE——探针翻转。
修复只使用运行自身已有的知识——它自己的 stamp 证明 bind 发生在哪里——而不是同 uid 对手可伪造的身份信号。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| degradations.push( | ||
| `${ansPath} was replaced while the render was being prepared — ` + | ||
| '.ans text captured, no image rendered', | ||
| ); |
There was a problem hiding this comment.
[Critical] R20-2: When the render-staging identity check DETECTS that <out>.ans was swapped, the run still writes a manifest attesting evidence: 'ans-only' at ansPath with captured: true on stdout, and the degradation claims ".ans text captured" — crediting bytes the code just determined it cannot show it produced, and minting the clear-phase signature that authorizes the NEXT run to delete the foreign file.
Trigger: a swap of <out>.ans during the freeze-probe window (the window the existing tests at capture-tui.test.ts:1862/1923 simulate) throws in staging; the degradation is pushed; the manifest write then runs unconditionally. Two harms: (1) a consuming agent reads ansPath as this capture's pane text and cites the foreign bytes as evidence — the exact attribution the staging guard's own comment refuses ("degrade rather than attribute bytes this capture cannot show it produced"); (2) the next run against the same --out passes the clear-phase signature check (evidence rung + exact ansPath + closed-set settledBy) and deletes the foreign file — the data-loss loop the signature hardening was built to close, materializing on a later innocent run.
Witness (probe):
RUN 1: stdout {"captured":true,"evidence":"ans-only"}, manifest degradedBecause
"…cap.ans was replaced while the render was being prepared — .ans text captured,
no image rendered", cap.ans content: "FOREIGN-ANS-BYTES"
RUN 2 (innocent re-run): exitCode 3, cap.ans exists now: false — the clear phase
accepted RUN 1's signature and deleted the foreign file
Treat a detected pre-render swap like the .ans write-failure path: refuse (exit 3, no manifest, foreign file untouched) — or at minimum do not record the ans rung and do not claim ".ans text captured".
中文说明
[Critical] R20-2：当渲染暂存的身份检查检测到 <out>.ans 被替换时，运行仍然写出清单，宣称 ansPath 处有 evidence: 'ans-only'、stdout 报 captured: true，降级信息还声称「.ans 文本已捕获」——为该代码刚刚确定无法证明由自己产生的字节背书，并铸造了授权下一次运行删除外来文件的清场签名。
触发：<out>.ans 在 freeze 探针窗口内被替换（现有测试 capture-tui.test.ts:1862/1923 模拟的正是该窗口）→ 暂存抛错 → 推入降级 → 清单写入无条件执行。两个危害：(1) 消费方代理把 ansPath 当作本次采集的面板文本引用外来字节作为证据——正是暂存守卫注释拒绝的归属（「降级而不是归属本采集无法证明产生的字节」）；(2) 针对同一 --out 的下一次运行通过清场签名检查（证据档位 + 精确 ansPath + 封闭集 settledBy）并删除外来文件——签名加固本要关闭的数据丢失循环，在一次后来的无辜运行上重现。
证据（探针）：RUN 1：stdout {"captured":true,"evidence":"ans-only"}，清单 degradedBecause 「…cap.ans was replaced while the render was being prepared — .ans text captured, no image rendered」，cap.ans 内容："FOREIGN-ANS-BYTES"；RUN 2（无辜重跑）：exitCode 3，cap.ans 已不存在——清场阶段接受了 RUN 1 的签名并删除了外来文件。
请把检测到替换的渲染前状态按 .ans 写失败路径处理：拒绝（exit 3、无清单、外来文件不动）——至少不要记录 ans 档位、不要声称「.ans 文本已捕获」。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (png === null && path === pngPath) continue; | ||
| try { | ||
| if (changed(path, stamp)) rmSync(path, { force: true }); |
There was a problem hiding this comment.
[Critical] R20-3: The manifest-write-failure cleanup deletes whatever occupies <out>.ans when changed(path, stamp) says so — evaluated against the PRE-window stamp the collision gate guarantees had existed: false, so it reduces to occupied(): "something is there", not "this run put it there". A file swapped into <out>.ans during the render window is destroyed when the manifest write then fails (collision/ENOSPC/EMFILE — named and measured in this file). The render stage takes ansWritten precisely because "by then ansPath may already be the swap" — the cleanup never consults it. The trigger is NOT the check-vs-syscall adversary class: the captured command claiming --out paths mid-window is the documented ordinary planter class (writeArtifact's own comment), combined with an ordinary manifest-write failure.
Witness (probe): PR code → the refusal correctly spared the cap.json occupant, but cap.ans content: <DELETED> — the foreign file swapped into <out>.ans was destroyed; flip (gate the removal on lstat identity vs ansWritten, scratch tree): foreign bytes preserved, and the sanity arm (no swap) still removes the run's own .ans — the fix is sound both ways.
Gate the removal on identity with what this run wrote: for ansPath, only rmSync when the current lstat still matches ansWritten (ino+size+mtimeMs); for pngPath, keep the identity of what the rename landed and compare that, instead of changed(path, stamp) against the pre-window stamps.
中文说明
[Critical] R20-3：清单写失败的清理会在 changed(path, stamp) 为真时删除 <out>.ans 处的任何占用者——而比较所用的窗口前 stamp 被碰撞门保证 existed: false，于是退化为 occupied()：「那里有东西」，而不是「本次运行放的」。渲染窗口内被替换进 <out>.ans 的文件，会在清单写入随后失败（collision/ENOSPC/EMFILE——本文件点名并实测过）时被删除。渲染阶段拿 ansWritten 正是因为「到那时 ansPath 可能已经是替换品」——清理却从不查询它。触发不属于检查-系统调用竞速的对手类：被采集命令在窗口内认领 --out 路径是本文件文档化的普通植入类（见 writeArtifact 自己的注释），叠加一次普通的清单写失败。
证据（探针）：PR 代码 → 拒绝正确地放过了 cap.json 占用者，但 cap.ans 内容：<DELETED>——被替换进 <out>.ans 的外来文件被删除；翻转（以 lstat 身份对照 ansWritten 作为删除门槛，scratch 树）：外来字节保留，且对照分支（无替换）仍删除本运行自己的 .ans——修复两个方向都成立。
请以本次运行写入内容的身份作为删除门槛：对 ansPath，仅当当前 lstat 仍与 ansWritten（ino+size+mtimeMs）一致时才 rmSync；对 pngPath，保留 rename 落地的内容身份并与之比较，而不是拿窗口前 stamp 做 changed(path, stamp)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| try { | ||
| rmSync(join(dir, name), { force: true }); | ||
| } catch { | ||
| // Litter is cosmetic; the server itself is already gone. | ||
| } |
There was a problem hiding this comment.
[Critical] R20-4: The sweep unlinks the socket entry unconditionally — even when the post-kill identity re-check just set entryChanged because the entry at the name is NOT the plain socket the type guard inspected. That violates the function's own rule forty lines above: "unlink the socket ONLY when the server is known dead … an unlinked socket makes a live server unreachable forever". The catch comment ("the server itself is already gone") asserts precisely what is not established in the changed-entry case.
Trigger: the guard lstats the orphan socket; kill-server kills it (exit 0); in the connect→re-check window a racer renames the user's own live server socket onto the capture name; the re-check detects the different identity and prints the WARNING — and rmSync unlinks the live server's socket anyway: the server keeps running, unreachable forever (no attach, no -L control). Leaving the entry is self-healing (the next sweep re-examines it); unlinking is not.
Witness: the repo's own test, run unmodified in the scratch tree — 'warns instead of claiming "Reaped" when the entry changed under the kill' passes WITH the assertion expect(mocks.rmSync).toHaveBeenCalledWith(…) (annotated "The planted name is still unlinked"): detection exists, the WARNING fires, and the act the WARNING exists to guard against is performed anyway — pinned deliberately, with no rationale reconciling it with the unlink rule.
| try { | |
| rmSync(join(dir, name), { force: true }); | |
| } catch { | |
| // Litter is cosmetic; the server itself is already gone. | |
| } | |
| if (!entryChanged) { | |
| try { | |
| rmSync(join(dir, name), { force: true }); | |
| } catch { | |
| // Litter is cosmetic; the server itself is already gone. | |
| } | |
| } |
(and update the test to assert no unlink on a detected swap)
中文说明
[Critical] R20-4：清扫无条件 unlink socket 条目——即使 kill 后的身份复查刚刚因「该名字处的条目不是类型守卫检查过的那个普通 socket」而置了 entryChanged。这违反四十行上方函数自己的规则：「仅在服务器确定已死时 unlink socket……被 unlink 的 socket 会让存活服务器永远不可达」。catch 注释（「服务器本身已经没了」）恰恰断言了条目已变情形下并未确立的事实。
触发：守卫 lstat 孤儿 socket；kill-server 杀掉它（exit 0）；在 connect→复查窗口内，竞速者把用户自己的存活服务器 socket 重命名到采集名；复查检测到不同身份并打印 WARNING——而 rmSync 仍然 unlink 了存活服务器的 socket：服务器继续运行、永远不可达（无法 attach、无法 -L 控制）。留下条目可自愈（下次清扫会重新检查）；unlink 不可逆。
证据：在 scratch 树中原样运行仓库自己的测试——'warns instead of claiming "Reaped" when the entry changed under the kill' 带着 expect(mocks.rmSync).toHaveBeenCalledWith(…) 断言（注释「The planted name is still unlinked」）通过：检测存在、WARNING 触发、而 WARNING 本要阻止的行为照样执行——被刻意 pin 住，且没有任何理由说明它为何与 unlink 规则相容。
（并把测试改为断言检测到替换时不发生 unlink。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
…1..4)
Four Criticals, all the same shape as the last rounds: a piece of reasoning
this PR already applies at one site, not carried to its sibling. None needs a
forgeable identity signal — each uses only what the run already knows.
R20-1 — the reap's planted-entry guard applied its identity arm only on the
start base. But a present stamp PROVES the bind was on the start base (the
same fact `confirmedDead` leans on), so a socket at this run's unique name on
ANY OTHER base cannot be ours. Without this, a plain foreign socket renamed
onto the name under /tmp passed the symlink/nlink tests and took the pinned
kill — the user's own server destroyed, exit 0, no WARNING. The guard now
treats any entry on a non-start base as planted when a stamp exists.
R20-2 — a detected pre-render swap of <out>.ans still wrote a manifest
crediting `evidence: 'ans-only'` at that path and minting the clear-phase
signature that authorizes the next run to delete it. That both misattributes
foreign bytes (against the header's "never credits bytes it did not produce")
and arms a later data-loss. The manifest type already forbids `evidence:
'none'` precisely because no honest evidence means no manifest — so this now
refuses like the write-failure paths (exit 3, no manifest, foreign file
untouched) instead of degrading.
R20-3 — the manifest-write-failure cleanup deleted whatever occupied
<out>.ans, because `changed(path, ansStamp)` against the pre-window stamp
(`existed: false`) collapses to `occupied()` — "something is there", not
"this run put it there". It now removes the .ans and .png only while they
still match what this run actually wrote and landed (`ansWritten`,
`pngWritten`), by ino+size+mtime; a file swapped in is foreign and left alone.
R20-4 — the sweep unlinked the socket entry unconditionally, even when the
post-kill re-check set `entryChanged` (a racer renamed something onto the name
in the connect→re-check window). That violated the function's own rule
("unlink ONLY when the server is known dead — an unlinked socket makes a live
server unreachable forever"): a renamed-in live server's socket was unlinked
and made unreachable. It now unlinks only when the entry is unchanged;
otherwise the WARNING stands and the socket is left for the next sweep.
Tests: R20-1 pinned by a new cross-base test (a socket at the name on /tmp with
a stamp present is never connected to); R20-2 and R20-4 pinned by the two
staging tests and the sweep test updated to the new refuse/no-unlink behavior
(the finding itself asked for the R20-4 test update); each mutation-checked.
R20-3 keeps no dedicated behavioral test: R20-2 now refuses on every
staging-detected swap, so the only path left to the manifest-failure cleanup
with a foreign .ans is a swap during the freeze-render window plus a
manifest-write failure, which is not deterministically constructible in a unit
test — the identity comparison it relies on is the same ino+size+mtime the
tested siblings use.
`npm run build` clean, prettier and eslint clean, and
`vitest run --root packages/cli src/commands/review` 5152 passed / 0 failed.
…ui-core No conflicts. npm run build clean and vitest run --root packages/cli src/commands/review 5152 passed / 0 failed on the result.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] R14-2 (cleanup.ts post-kill identity re-check, ~line 787): the premise — some tmux version unlinks the socket after a clean kill-server, making a benign reap print a false WARNING and suppress 'Nothing to clean' — cannot be ruled on from th…
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the macOS lane did not run locally; tmux/socket/PATH behavior in this PR is platform-sensitive.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the Windows lane did not run locally; flag-constant and path-shape behavior in this PR is platform-sensitive.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites.
Not reviewed: test-efficacy — the probe harness could not be validated (harnessValidated: null — the control never ran; the probe runner hit vitest's build-prerequisite guard), so mutant and hunk-revert probes were skipped.
Not reviewed: reverse audit — stopped at the round cap of 3 without converging; the cap round still surfaced findings (the loop did not converge).
Not explored to full depth (tool budget reached): chunk 6: none — no check was cut short..
Deferred under the convergence posture (round 21, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/cleanup.test.ts:1112 — [probe] literal pid 626262 probed without withDeadPids on a pid_max-4194304 lanepackages/cli/src/commands/review/cleanup.ts:814 — [review] sweep's returned reaped member has no read site; set true even in the entryChanged/WARNING armpackages/cli/src/commands/review/cleanup.ts:565 — [probe] sweep-side realpath-keyed base dedup unpinned; a raw-string-key mutant ships greenpackages/cli/src/commands/review/capture-tui.ts:193 — [probe] probeOutput's ENOBUFS arm unpinned; deletion flips the refusal wording silentlypackages/cli/src/commands/review/capture-tui.ts:2171 — [probe] freeze-vanishing-before-render degradation branch unpinned; leaks the nonce'd .render stagepackages/cli/src/commands/review/capture-tui.test.ts:3332 — [probe] probe-belt shim hardcodes /bin/sleep on hosts the file defends against elsewherepackages/cli/src/commands/review/cleanup.test.ts:917 — [probe] post-kill ENOENT arm unpinned; the catch-deletion mutant strands the lease and passes 97/97packages/cli/src/commands/review/cleanup.ts:559 — [probe] relative TMUX_TMPDIR swept from the wrong cwd; orphan lives while stdout says 'Nothing to clean'packages/cli/src/commands/review/capture-tui.test.ts:6061 — [review] inclusive-maxima timeoutMs acceptance leaks a tmux server per red run under the 15s budgetpackages/cli/src/commands/review/cleanup.test.ts:1050 — [probe] client-side-refusal test throws one wording on both attempts; the reassignment mutant ships greenpackages/cli/src/commands/review/cleanup.test.ts:681 — [probe] manual-reap single-quote escape unpinned; no fixture carries an apostrophepackages/cli/src/commands/review/lib/tui-capture.test.ts:309 — [probe] plan.start under-pinned; an injected chained command ships green yet kills the session on real tmuxpackages/cli/src/commands/review/capture-tui.test.ts:375 — [probe] long-socket fixture's pad saturates on long-tmpdir hosts — the off-by-one discrimination is lost therepackages/cli/src/commands/review/capture-tui.test.ts:3052 — [probe] probeOutput's spawned:false arm unpinned; EMFILE at probe flips to 'not installed'packages/cli/src/commands/review/capture-tui.test.ts:4821 — [probe] EPIPE fault injection never asserted fired; a chunk-splitting mutant escapespackages/cli/src/commands/review/capture-tui.ts:1714 — [review] reap WARNING hands a bare kill-server command even in the planted-entry variant, without the base pinpackages/cli/src/commands/review/capture-tui.ts:2529 — [probe] --timeout-ms help's wall bound is false; key sends run after the shared deadlinepackages/cli/src/commands/review/cleanup.test.ts:997 — [probe] cross-base test pins no per-entry kill base or unlink path; a shared-base mutant ships greenpackages/core/src/tools/glob.test.ts:316 — [probe] anti-vacuity assertion pins the count only; discrimination is borrowed from an unrelated fixture
Convergence: round 21 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/cli/src/commands/review/capture-tui.ts (findings in round 20; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the macOS lane did not run locally; tmux/socket/PATH behavior in this PR is platform-sensitive。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the Windows lane did not run locally; flag-constant and path-shape behavior in this PR is platform-sensitive。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites。
未审查：test-efficacy — the probe harness could not be validated (harnessValidated: null — the control never ran; the probe runner hit vitest's build-prerequisite guard), so mutant and hunk-revert probes were skipped。
未审查：reverse audit — stopped at the round cap of 3 without converging; the cap round still surfaced findings (the loop did not converge)。
未探索到全部深度（达到工具调用预算）：chunk 6：none — no check was cut short.。
收敛姿态下延后（第 21 轮，非阻断）——已记录，本轮不要求修改：共 19 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 21 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/capture-tui.ts（第 20 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ansLost = true; | ||
| try { | ||
| rmSync(ansStage, { force: true }); |
There was a problem hiding this comment.
[Critical] R21-1: This catch conflates a swapped/gone .ans with a host-state failure of linkSync itself: ansLost = true runs unconditionally, so on any host where link() fails for the stage — exFAT/FAT mounts, WSL DrvFs, or ENOSPC/EDQUOT/EACCES hitting the directory-entry create mid-window — a fully successful text capture refuses with the factually false machine-read claim "<out>.ans was replaced during the render window", strands this run's own intact .ans with no manifest, and the next same---out run finds no signature to clear by, so the collision gate refuses "--out collides with a file this capture did not write". On such a host every capture refuses until someone deletes the file by hand, and the ladder's documented contract (png → ans-only → refused) is defeated exactly where it should degrade: the .ans rung was written and is still identity-checkable (ansWritten was stamped), only the png rung needed to drop. Note the ENOSPC/EDQUOT arms turn a transient disk-full at link time into a persistent wedge for that --out.
Witness — scoped LD_PRELOAD failing link() with EPERM only for *.render-* targets (the shape exFAT/FAT answer), driven against the unmodified PR code:
run1: {"exitCode":3,"manifestWritten":false,"ansPresent":true,"ansHasPaneText":true,
"claimsReplacedDuringRenderWindow":true,
stderrTail: "...cap.ans was replaced during the render window..."}
run2 same --out: {"exitCode":3,"collisionRefusal":true,
stderrTail: "--out collides with a file this capture did not write: ...cap.ans"}
Split the catch by cause: when linkSync itself threw, re-check lstatSync(ansPath) against ansWritten and, while the identity still matches (nothing was swapped), push a degradation naming the errno and fall through to an ans-only manifest instead of refusing. Keep ansLost for the identity-mismatch (real swap) shape. Sketch (the fix spans this whole catch, so no one-click suggestion):
} catch (stageErr) {
let swapped = true;
try {
const cur = lstatSync(ansPath);
swapped = cur.ino !== ansWritten.ino || cur.size !== ansWritten.size ||
cur.mtimeMs !== ansWritten.mtimeMs;
} catch { /* gone — lost */ }
if (swapped) {
ansLost = true;
} else {
degradations.push(
`could not stage ${ansPath} for rendering ` +
`(${(stageErr as NodeJS.ErrnoException).code ?? stageErr}) — ` +
'.ans text captured, no image rendered',
);
}
try {
rmSync(ansStage, { force: true });
} catch {}
}中文说明
[Critical] R21-1：这个 catch 把「.ans 被替换/消失」与「linkSync 本身因宿主状态失败」混为一谈：ansLost = true 无条件执行。于是在任何 link() 会对暂存文件失败的宿主上（exFAT/FAT 挂载、WSL DrvFs，或窗口内目录项创建撞上 ENOSPC/EDQUOT/EACCES），一次完全成功的文本捕获会以机器可读的虚假声明「<out>.ans was replaced during the render window」拒绝退出，把本运行自己完好的 .ans 撂在那里且没有清单；下一次同一 --out 运行找不到可验证的签名、清不了任何东西，碰撞门拒绝「--out collides with a file this capture did not write」。在这样的宿主上，每次采集都会拒绝，直到有人手工删文件；阶梯的文档化契约（png → ans-only → refused）恰在本应降级之处被击败：.ans 档已写出且身份仍可校验（ansWritten 已打下），只有 png 档需要掉落。另注意 ENOSPC/EDQUOT 分支会把 link 时刻的瞬时磁盘写满变成该 --out 的永久楔子。
证据：用作用域受限的 LD_PRELOAD 仅对 *.render-* 目标让 link() 以 EPERM 失败（即 exFAT/FAT 的应答形态），驱动未修改的 PR 代码：run1 {exitCode:3, manifestWritten:false, ansPresent:true, ansHasPaneText:true, claimsReplacedDuringRenderWindow:true}，stderr 声称「cap.ans was replaced during the render window」；run2 同一 --out：{exitCode:3, collisionRefusal:true}，「--out collides with a file this capture did not write: ...cap.ans」。
请按成因拆分该 catch：当 linkSync 自身抛出时，以 lstatSync(ansPath) 对照 ansWritten；只要身份仍一致（什么都没被替换），推入一条点名 errno 的降级并继续走 ans-only 清单，而不是拒绝。身份不符（真实替换）形态保留 ansLost。修复横跨整个 catch，故不提供一键 suggestion，代码草图见英文部分。
— qwen3.8-max via Qwen Code /review (v0.22.0)
The R20-2 fix set `ansLost` unconditionally in the render-staging catch, which conflates two causes: linkSync SUCCEEDED but staged a foreign inode (a real swap — refuse) versus linkSync ITSELF throwing (a host with no link() — exFAT/FAT/WSL DrvFs — or ENOSPC/EDQUOT/EACCES on the stage's directory entry). In the second case this run's own .ans is intact and still identity-checkable, so refusing with the factually false "replaced during the render window" stranded the intact .ans with no manifest and wedged every later capture at that --out (the collision gate then refuses the unsignatured file forever) — a link-less mount broke the ladder exactly where it should degrade png → ans-only. The catch now re-checks <out>.ans against `ansWritten`: changed or gone is a swap (keep the refusal); still ours is a stage failure that names the errno and falls through to an ans-only manifest. The swap path is unchanged — the in-place and symlink staging tests still refuse. Test: a read-only --out directory injected during the render window fails the stage linkSync with EACCES while <out>.ans stays exactly this run's bytes; the run must not emit the false swap claim (it refuses for the honest manifest-write reason instead). Reverting the split turns it red (mutation-checked). `npm run build` clean, prettier and eslint clean, and `vitest run --root packages/cli src/commands/review` 5153 passed / 0 failed.
…ui-core No conflicts. main bumped the lockfile; npm ci then npm run build clean and vitest run --root packages/cli src/commands/review 5223 passed / 0 failed.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] R14-2 (doudouOUC, comment 3834716946, cleanup.ts post-kill identity re-check ~line 786): the mechanism is real — a model probe of an unlinking tmux fired the false-WARNING arm this round — but the premise that some tmux version unlinks the …
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the macOS lane did not run locally; tmux/socket/PATH behavior in this PR is platform-sensitive.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the Windows lane did not run locally; flag-constant and path-shape behavior in this PR is platform-sensitive.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites.
Not reviewed: test-efficacy — the probe harness could not be validated (harnessValidated: null — no green unmutated baseline exists because the in-scope suites carry pre-existing failures shared with the merge base), so mutant and hunk-revert probes were skipped.
Not explored to full depth (tool budget reached): chunk 2: running capture-tui.test.ts to confirm the suite is green — the review worktree has no node_modules /built dist , and a full npm ci + build inside the sha….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
Deferred under the convergence posture (round 22, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/capture-tui.test.ts:1304 — [probe] The nosleep test's 'nothing was started' pin (absent…packages/cli/src/commands/review/capture-tui.test.ts:1462 — [review] None of the three planted-entry pins asserts…packages/cli/src/commands/review/capture-tui.test.ts:3387 — [probe] The probe-belt test's hanging-tmux shim hardcodes…packages/cli/src/commands/review/capture-tui.test.ts:4613 — [probe] The server-death probe adopts the FIRST pid pgrep -f…packages/cli/src/commands/review/capture-tui.test.ts:5219 — [probe] Five real-tmux fixtures invoke bash -c in the pane…packages/cli/src/commands/review/capture-tui.test.ts:6295 — [probe] The orphan-boundary test hardcodes /bin/sleep as its…packages/cli/src/commands/review/capture-tui.ts:1017 — [review] The hung-with-code probe branches — the tmux refusal…packages/cli/src/commands/review/capture-tui.ts:1714 — [probe] The reap-failure WARNING's hand-reap hint omits the…packages/cli/src/commands/review/capture-tui.ts:2201 — [probe] The degradation branch for freeze vanishing between the…packages/cli/src/commands/review/capture-tui.ts:2557 — [review] The --timeout-ms help text claims wall time is bounded by…packages/cli/src/commands/review/cleanup.test.ts:681 — [probe] The manual-reap quoting test pins $ and backtick survival…packages/cli/src/commands/review/cleanup.test.ts:997 — [probe] The only multi-base fixture pins the cross-base…packages/cli/src/commands/review/cleanup.test.ts:1112 — [probe] 'keeps the run alive when the post-kill UNLINK fails'…packages/cli/src/commands/review/cleanup.test.ts:1152 — [probe] The win32-arm test pins only the no-scan half of the…packages/cli/src/commands/review/cleanup.ts:605 — [review] The sweep's silent ENOENT/ENOTDIR/ELOOP readdir…packages/cli/src/commands/review/cleanup.ts:655 — [probe] Both lstat catch branches of the sweep — the guard catch…packages/cli/src/commands/review/cleanup.ts:814 — [review] reapOrphanedCaptureServers() returns a reaped member no…packages/cli/src/commands/review/lib/tui-capture.ts:157 — [review] The private-tmux-server-name rationale doc block is…packages/cli/src/commands/review/run.test.ts:511 — [probe] The sweep-isolation guard asserts nothing about itself:…
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the macOS lane did not run locally; tmux/socket/PATH behavior in this PR is platform-sensitive。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the Windows lane did not run locally; flag-constant and path-shape behavior in this PR is platform-sensitive。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; the new capture-tui subcommand's CLI integration surface was exercised only by the unit suites。
未审查：test-efficacy — the probe harness could not be validated (harnessValidated: null — no green unmutated baseline exists because the in-scope suites carry pre-existing failures shared with the merge base), so mutant and hunk-revert probes were skipped。
未探索到全部深度（达到工具调用预算）：chunk 2：running capture-tui.test.ts to confirm the suite is green — the review worktree has no node_modules /built dist , and a full npm ci + build inside the sha…。
未审查：反向审计——在 3 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 22 轮，非阻断）——已记录，本轮不要求修改：共 19 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
…ui-core Resolve three conflicts: - commands/review.ts: take main's subcommand list and re-insert capture-tui after capture-local. - review/cleanup.ts: keep both early guards, orphan capture-server sweep first (its comment requires it to precede any refusal that could strand an orphan) then main's bare-'pr' target refusal. - review/lib/agent-briefs.ts: keep both brief sections (main's ab-drive guidance, then this branch's capture-tui guidance). tsc -p packages/cli clean; review suite 5874 passed.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
11 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R23-3 dead/lying
reapedfield (cleanup.ts:815) — already recorded (round-22 deferral list at cleanup.ts:814, review 5011712216) - R23-4 orphan-boundary test hardcodes /bin/sleep (capture-tui.test.ts:6295) — already recorded (round-22 deferral list at capture-tui.test.ts:6295, review 5011712216)
- R23-5 literal pid 626262 not stubbed (cleanup.test.ts:1178) — already recorded (round-22 deferral list at cleanup.test.ts:1112, review 5011712216)
- R23-6 detached rationale doc block (tui-capture.ts:157) — already recorded (round-22 deferral list at tui-capture.ts:157, review 5011712216)
- R23-7 probeOutput classification arms untested (capture-tui.ts:1014) — already recorded (round-22 deferral list at capture-tui.ts:1017, review 5011712216)
- R23-8 freeze-vanish degradation arm untested (capture-tui.ts:2201) — already recorded (round-22 deferral list at capture-tui.ts:2201, review 5011712216)
- R23-9 belt-test shim hardcodes /bin/sleep (capture-tui.test.ts:3387) — still standing from prior-round blocker (comment 3832236037); already recorded (round-22 deferral list at capture-tui.test.ts:3387, review 5011712216)
- R23-10 ENOTDIR/ELOOP quiet arms untested (cleanup.ts:607) — already recorded (round-22 deferral list at cleanup.ts:605, review 5011712216)
- R23-11 guard-lstat race catch untested (cleanup.ts:655) — already recorded (round-22 deferral list at cleanup.ts:655, review 5011712216)
- R23-12 bash -c fixtures lack a hasBash gate (capture-tui.test.ts:5219) — already recorded (round-22 deferral list at capture-tui.test.ts:5219, review 5011712216)
- R23-13 --timeout-ms help bound is false (capture-tui.ts:2560) — already recorded (round-22 deferral list at capture-tui.ts:2557, review 5011712216)
Unresolved, please confirm:
- [Critical] R14-2 (comment 3834716946, cleanup.ts post-kill identity re-check): the ENOENT-after-successful-kill → WARNING mechanism is real in the current code, but the premise — that some tmux version unlinks the socket on a clean kill — is neither c…
- [Critical] Blocker comment 3807053171 (R9-era sibling, capture-tui reap wording family): body not fully traced within this round's budget; the create-directory wording family was separately re-audited this round (see R23-2).
- [Critical] Blocker comment 3824384524: body not fully traced within this round's budget.
- [Critical] Blocker comment 3832236026: body not fully traced within this round's budget.
- [Critical] Blocker comment 3835359665: body not fully traced within this round's budget.
- [Critical] Blocker comment 3836031102: body not fully traced within this round's budget.
- [Critical] Blocker comment 3836031277: body not fully traced within this round's budget.
- [Critical] Blocker comment 3837719402: body not fully traced within this round's budget.
- [Critical] Blocker comment 3837719405: body not fully traced within this round's budget.
- [Critical] Blocker comment 3839688217: body not fully traced within this round's budget.
- [Critical] Blocker comment 3840547572: body not fully traced within this round's budget.
- [Critical] Blocker comment 3840547584: body not fully traced within this round's budget.
- [Critical] Blocker comment 3840853011: body not fully traced within this round's budget.
Not reviewed: build-and-test — the local test phase did not run: the whole-call budget (570s) was spent building 18 workspaces; 7 suites never ran and --resume was refused twice.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x) and Integration Tests (CLI, No Sandbox) were skipped in CI and their suites did not run locally; the platform-sensitive capture-tui/cleanup suites ran only on the Linux lane, which FAILED at this commit.
Not reviewed: test-efficacy — the probe harness could not be validated (harnessValidated: null — probe worktrees lack the dist outputs the repo's vitest prerequisite guard requires); mutant and hunk-revert probes were skipped.
Not reviewed: reverse audit — stopped at the round cap of 3 without converging (round 3 reported findings; the cap forbids further rounds).
Not explored to full depth (tool budget reached): chunk 12: executing packages/cli/src/commands/review/lib/tui-capture.test.ts (worktree lacks node_modules; assertions verified statically against tui-capture.ts at HEAD).
Deferred under the convergence posture (round 23, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/cleanup.ts:566 — [probe] the sweep's realpath-keyed base de-duplication has no discriminating test; a string-key mutant ships greenpackages/cli/src/commands/review/capture-tui.test.ts:5983 — [probe] fractional sleep 1.5 gate latency is GNU-only; the two-clock mutant ships green on BSD/macOS hostspackages/cli/src/commands/review/capture-tui.ts:176 — [probe] probeOutput misreports the relative-PATH-element case as 'not installed', contradicting resolveOnPath's doc promisepackages/cli/src/commands/review/capture-tui.ts:2058 — [probe] stale pngStamp.existed disjunct degrades the ladder on a vanished occupant and persists a false 'holds a file' claim (Critical filed; downgraded — fails-closed, self-healing)packages/cli/src/commands/review/cleanup.ts:787 — [probe] the post-kill ENOENT catch has no fixture; entryChanged=false and deleted-catch mutants both ship greenpackages/cli/src/commands/review/cleanup.ts:1032 — [probe] sweep-vs-refusal ordering unpinned; a per-gate move ships green and strands orphans on refused callspackages/cli/src/commands/review/lib/agent-briefs.ts:794 — [probe] <target> slot misdefined as the artifact prefix; literal substitution mints basenames the sweep never reclaimspackages/cli/src/commands/review/lib/tui-capture.ts:195 — [probe] resolveOnPath's isFile() guard has no pinning test across all 313 tests; a directory shadow collapses the holderpackages/cli/src/commands/review/capture-tui.test.ts:375 — [probe] over-long-socket fixture pads against the lexical dir but the gate measures realpathSync; red on shortening-canonical hostspackages/cli/src/commands/review/capture-tui.ts:1580 — [probe] killSpawnFailed misses code-less resolve-failure throws; WARNING claims fd exhaustion for a server that never existedpackages/cli/src/commands/review/cleanup.test.ts:43 — [review] mock comment claims the redirect guard reads isDirectory; contradicts code and the SweepEntryStat comment abovepackages/cli/src/commands/review/cleanup.test.ts:795 — [probe] the documented 'no tmux, no orphans stays silent' invariant is unpinned; a fail-fast hoist ships greenpackages/cli/src/commands/review/lib/agent-briefs.ts:790 — [probe] the capture-tui brief omits that --until/--ready are regexes; '[OK]' settles early on a character classpackages/cli/src/commands/review/cleanup.test.ts:984 — [probe] the call-counted lstat fixture is silently order-sensitive across all lstat consumers in runCleanup's graph
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 11 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 13 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — the local test phase did not run: the whole-call budget (570s) was spent building 18 workspaces; 7 suites never ran and --resume was refused twice。
未审查：build-and-test — Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x) and Integration Tests (CLI, No Sandbox) were skipped in CI and their suites did not run locally; the platform-sensitive capture-tui/cleanup suites ran only on the Linux lane, which FAILED at this commit。
未审查：test-efficacy — the probe harness could not be validated (harnessValidated: null — probe worktrees lack the dist outputs the repo's vitest prerequisite guard requires); mutant and hunk-revert probes were skipped。
未审查：reverse audit — stopped at the round cap of 3 without converging (round 3 reported findings; the cap forbids further rounds)。
未探索到全部深度（达到工具调用预算）：chunk 12：executing packages/cli/src/commands/review/lib/tui-capture.test.ts (worktree lacks node_modules; assertions verified statically against tui-capture.ts at HEAD)。
收敛姿态下延后（第 23 轮，非阻断）——已记录，本轮不要求修改：共 14 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
| !( | ||
| isSocketDirNeverCreated(stderrText) && | ||
| onStartBase && | ||
| !startThrew | ||
| ) && |
There was a problem hiding this comment.
[Critical] R23-2: [certifies-falsely] [new-surface] The couldn't create directory kill wording is credited as death on the start base whenever startThrew is set. The file's own ENOENT-class comment documents that startThrew conflates "start failed before binding a socket" with a belt-cut start that threw with the server already forked and its socket bound — the ENOENT class is excluded from credit for exactly that conflation, but the sibling never-created wording still credits under the same flag. Trigger: a belt-cut start (the 15s control belt SIGKILLs the client mid-start on a loaded runner — the measured shape the start-call comment names) leaves startThrew=true with the server bound; the captured command — untrusted same-uid code that knows the socket path from $TMUX — destroys the socket dir and makes the base uncreatable, and the pinned kill answers couldn't create directory …, credited as death on both bases. The run exits 3 with NO orphan WARNING while the server and its pane holder live up to three hours, invisible to the readdir sweep because the socket dir is gone — a presumed-alive private server is a silent outcome, exactly what the ENOENT class was made to answer loudly. (The audit's original claim of exit 0 is corrected to exit 3 — a throwing start always propagates to the mid-capture refusal; the silent-orphan certification stands as described.)
Witness:
OBSERVE P3 exitCode=3 WARNING=false ...
stderrTail=["capture-tui: refused — tmux failed mid-capture: spawnSync /tmp/p3probe-.../fakebin/tmux ETIMEDOUT"]
(real ETIMEDOUT belt-cut via the tmuxControl seam; the shim binds the socket, then destroys the dir)
with the startBeltCut exclusion applied:
OBSERVE P3 exitCode=3 WARNING=true ...
"capture-tui: WARNING — kill-server failed twice; the private tmux server qwen-review-capture-...
may still be running (tmux -L ... kill-server to reap it by hand)"
Distinguish belt-cut from genuine start throws — the belt kill carries code === 'ETIMEDOUT', as the freeze path documents at capture-tui.ts:2292-2295. Record a startBeltCut flag in the start's catch and extend the exclusion: !(isSocketDirNeverCreated(stderrText) && onStartBase && (!startThrew || startBeltCut)). The mode-0555 measurement this file quotes ("both attempts answered couldn't create directory …", capture-tui.ts:1586-1589) binds the fix: a non-belt start throw must still credit the wording, or the false-WARNING regression the exclusion exists to prevent returns. Please add a capture-tui test that shortens the tmuxControl seam, hangs the start shim past the belt (ETIMEDOUT shape), and answers the kill shim with couldn't create directory <base>/tmux-<uid> (Permission denied) exit 1, asserting the orphan WARNING prints — removing the added exclusion must turn that test red.
中文说明
只要 startThrew 被置位，couldn't create directory 这一 kill 措辞就被当作启动 base 上服务器已死的证据。本文件 ENOENT 类的注释自己写明：startThrew 混淆了"绑定 socket 之前启动失败"与"控制带在启动中途切断、而服务器已经 fork 且 socket 已绑定"两种情形——ENOENT 类正是因为这一混淆才被排除在记功之外，但同族的 never-created 措辞却仍在同一面旗下记功。触发：控制带在负载较高的运行器上于启动中途 SIGKILL 客户端（启动调用注释命名的已测量形态），留下 startThrew=true 且服务器已绑定；被采集命令——知道 $TMUX 中 socket 路径的不受信任同 uid 代码——毁掉 socket 目录并使该 base 不可创建，钉住的 kill 回答 couldn't create directory …，两个 base 上都记功为死亡。运行以 exit 3 结束、没有任何孤儿 WARNING，而服务器与其 pane holder 继续存活至多三小时，且因 socket 目录已消失而对 readdir 清扫不可见——被推定存活的私有服务器成了静默结局，正是 ENOENT 类被造出来高声应答的情形。（审计原称 exit 0，更正为 exit 3——抛错的启动总是传播到中途捕获的拒绝；静默孤儿的定性不变。）
证据：真实 ETIMEDOUT 带切（经 tmuxControl seam；shim 绑定 socket 后毁掉目录）：exitCode=3 WARNING=false；应用 startBeltCut 排除后：exitCode=3 WARNING=true，打印 "kill-server failed twice; the private tmux server … may still be running"。
修复：区分带切与真实启动抛错——带切携带 code === 'ETIMEDOUT'（freeze 路径在 capture-tui.ts:2292-2295 已有同款文档）。在 start 的 catch 中记录 startBeltCut 标志，并把排除式扩展为 !(isSocketDirNeverCreated(stderrText) && onStartBase && (!startThrew || startBeltCut))。约束：文件引用的 mode-0555 测量（capture-tui.ts:1586-1589）约束修复——非带切的启动抛错必须仍记功该措辞，否则该排除式本要防止的假 WARNING 回归会重现。修复见证：请新增 capture-tui 测试——缩短 tmuxControl seam、让 start shim 挂过带（ETIMEDOUT 形态）、令 kill shim 以 couldn't create directory <base>/tmux-<uid> (Permission denied) exit 1 应答，断言打印孤儿 WARNING；移除新增排除式必须使该测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
5 participants
What this PR does
Adds
qwen review capture-tui, so a verifier can produce rendering evidence instead of arguing one from the code. It drives a command inside a private tmux server, captures the pane text to<out>.ansalways, renders<out>.pngwhenfreezeis available, and writes<out>.jsonrecording which evidence rung it reached and why it stopped there.qwen review cleanuplearns to sweep orphaned capture servers, since a signal or a crash can outlive thefinallythat reaps them.The file guarantee is stated explicitly in the header, boundary included: the command protects evidence against ordinary conditions — a re-used
--out, stale artifacts, an unrelated file at one of the names, a host out of descriptors or disk — and not against an active same-uid adversary racing it between a check and the syscall that follows. Node exposes no*at()syscalls, so every path resolves by name and that interval cannot be closed from here. Hardening against that model is #9274, with its own guarantees and tests.Why it's needed
The measured failure mode of desktop-automation verification was "drives the user's own windows". A private per-run tmux socket makes that structurally impossible rather than discouraged: the capture cannot see, resize or kill the user's own sessions.
And a verdict has to say what it stands on. A PNG is publishable rendering evidence, an
.ansproves bytes but not pixels, and prose is neither — so the manifest names the rung and the reason the ladder stopped, and a refused capture writes no manifest at all (exit 3, a reason on stderr, machine-readable JSON on stdout), so a missing manifest reads as a refusal rather than as evidence.This supersedes #8894, which accumulated ten review rounds in one thread and became unreadable. The change is the same work, split so each half can be reviewed on its own terms.
Reviewer Test Plan
How to verify
End to end, on a host with tmux:
Expected: exit 0,
/tmp/cap.anscarrying the SGR bytes,/tmp/cap.jsonnamingevidence(pngwhenfreezeis installed, otherwiseans-onlywithdegradedBecausesaying why), and no tmux server left behind (tmux -L qwen-review-capture-<pid>-… kill-serveranswers "no server running").Refusal contract, on any host:
… capture-tui --command 'printf hi' --out ''exits 3, prints the reason on stderr, and puts{"captured":false,"evidence":"none","reason":…}on stdout.Evidence (Before & After)
N/A — new subcommand; there is no before. The
.ans/.pngit produces are the evidence it exists to make, and the test suite pins both rungs plus every refusal wording.Tested on
Environment (optional)
macOS 15 with tmux 3.7b and freeze 0.2.2; Linux via CI. Windows lanes are covered by
skipIfgates on the POSIX-only tests.中文说明
新增
qwen review capture-tui：在私有 tmux server 里驱动命令，始终抓取 pane 文本到<out>.ans，在有freeze时渲染<out>.png，并用<out>.json记录本次达到的证据档位以及为什么停在那里。qwen review cleanup增加清扫遗留采集 server 的能力——信号或崩溃可能让finally里的回收失效。文件头明确写出了担保范围及其边界：本命令在普通情况下保护证据（复用
--out、陈旧产物、恰好占用同名的无关文件、descriptor 或磁盘耗尽），不担保对抗同 uid 的主动竞速者。Node 没有*at()系统调用，所有路径按名解析，检查与随后系统调用之间的间隙在这里关不掉。针对该模型的加固是配套的另一个 PR。本 PR 取代 #8894——那个 PR 在同一条线程里累积了十轮评审，已经无法阅读。改动内容相同，只是拆开，让两半各自按自己的标准被评审。