feat(review): drive — readiness polled, completion proven, cleanup guaranteed - #8349
Merged
Conversation
…aranteed The highest-yield review technique in this repo's history is the local build-and-drive verification: build the PR, run the real product, watch what it does. Across 260 of those sessions the mechanical half is the same every time, done by hand every time, and two of its three steps are done by GUESSING: 81% waited with `sleep N`; only 36% polled for readiness. A `sleep 2` that lands before the daemon binds its port makes the capture return an empty screen, and an empty screen reads as "the feature does not work". That is a false negative manufactured by the harness, and it is silent. 74% captured one screenful with no way to know the command had finished. A capture taken mid-write is a partial observation presented as a complete one. 87% cleaned up by hand with `pkill -f <a name they made up>`. What one round leaks, the next round inherits and captures. `drive` owns exactly those three - ready or not, finished or not, gone either way - and nothing else. What to drive, and what the output means, stay with the caller: the same split `build-test` and `test-delta` draw. Two bugs only a real run could show, both green under the unit tests: `pipe-pane` races the script. `new-session` starts it immediately and the pipe attaches after, so a fast drive finishes - taking its session with it - before the pipe exists. Measured: a one-second delay makes `pipe-pane` itself exit 1 and the log stay empty, which this command reported as `timed-out`. A pane is a window that closes; the script's own redirect is the record. The sentinel was a trailing `echo`, which `exit N` never reaches - and `exit N` is how a drive script reports its result. Measured: `echo failing; exit 17` came back `timed-out` with a null exit code, a run that answered in milliseconds reported as one that never finished. `set +e` has no bearing on `exit`; a `trap … EXIT` does, and covers falling off the end, an explicit exit, and a `set -e` abort alike. The test that should have caught the second asserted `set +e` was present - the call shape, not the behaviour. Replaced with tests that drive real bash through all four exit paths.
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ — the "Why it's needed" section is folded into "What this PR does" rather than having its own heading, and "Linked Issues" is absent, but the substance is all there and then some. Not worth a round-trip. Problem: observed and measured, not theoretical. The 260-session data (81% sleep-waited, 74% captured without a completion signal, 87% cleaned up by hand) is a concrete, quantified account of the manual verification workflow's failure modes. The two bugs the PR describes — the Direction: aligned. Size: no core paths touched — all four files are in Approach: the scope feels right. One new file that does exactly one thing, its test file, and three lines of wiring. No unrelated changes, no drive-by refactors. The log-based capture (script redirect instead of Risk: no elevated risk signals — none of the changed files match the high-revert-correlation paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — "Why it's needed" 的内容融入了 "What this PR does"，没有单独标题，"Linked Issues" 也缺省，但实质内容齐全，不值得为此打回。 问题：已观测且有量化数据支撑，非理论性问题。260 个会话的实测数据（81% 用 sleep 等待、74% 无完成信号即抓取、87% 手工清理）具体量化了手动验证流程的失败模式。PR 描述的两个 bug—— 方向：对齐。 规模：未触及核心路径——四个文件均在 方案：范围合理。一个新文件做一件事、对应的测试文件、三行接线代码。无无关改动，无顺手重构。基于日志的抓取（脚本重定向而非 风险：无升级风险信号——变更文件均未命中高回滚关联路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code ReviewIndependent proposal: given the measured failure modes (sleep-waiting, no completion signal, manual cleanup), I would build a tmux-based subcommand that polls a readiness probe, wraps the driven script in a Comparison with the diff: the PR's approach matches this almost exactly. The implementation is clean and each design choice is traceable to a measured failure. No critical blockers found. Specific observations:
TestingFinal CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The ubuntu unit suite — the platform the author tested on — is green. macOS and Windows tests were skipped (gated on the ubuntu result); the new code is POSIX-shell + tmux with no platform-specific path beyond the Not verified: runtime behavior of the Sandboxed verification would settle the remaining gap: 中文说明代码审查独立方案： 基于已测量的失败模式（sleep 等待、无完成信号、手工清理），我会构建一个基于 tmux 的子命令：轮询就绪探针、用 与 diff 的比较： PR 的方案几乎完全匹配。实现干净，每个设计选择都可追溯到实测失败。 未发现关键阻塞问题。具体观察：
次要说明（不阻塞）： (1) 测试
未验证： 沙箱验证可填补剩余空白： — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; the problem is measured, the solution is minimal, and the tests drive real bash through every exit path. This is what a well-scoped feature PR looks like. The 260-session data makes the need concrete — this isn't "wouldn't it be nice if" tooling, it's automating the three steps that go wrong 74–87% of the time in the repo's most productive review workflow. The implementation does exactly those three things (ready-or-not, finished-or-not, cleaned-up-regardless) and nothing else, drawing the same caller-owns-semantics boundary as My independent proposal matched the PR almost line for line — trap-based sentinel, log-file capture over The code reads like it was written by someone who has done this 260 times and is tired of doing it by hand. Comments explain why with measured data, not what. Every convention is followed. If I had to maintain this in six months, I'd thank the author. On the size flag from Stage 1: at 503 production lines this is a large CI is green and settled on 中文说明置信度：5/5 — 每个阶段均干净通过；问题有量化数据支撑，方案最小化，测试通过真实 bash 驱动了所有退出路径。 这是一个范围良好的功能 PR 的典范。260 个会话的数据使需求具体化——这不是"如果有就好了"的工具，而是将仓库中最高产的评审工作流中 74–87% 出错率的三个步骤自动化。实现恰好做这三件事（就绪与否、完成与否、无论如何都清理干净），别无其他，与 我的独立方案与 PR 几乎逐行匹配——基于 trap 的哨兵、日志文件抓取优于 代码读起来像是一个做了 260 次手工操作、厌倦了的人写的。注释用实测数据解释"为什么"，而非"做了什么"。所有约定均被遵循。如果六个月后我要维护这段代码，我会感谢作者。 关于 Stage 1 的规模提示：按行数计这是一个较大的 CI 已在 — Qwen Code · qwen3.8-max-preview Reviewed at |
Contributor
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. |
Round-1 finding on this PR, both halves reproduced before fixing. `--server '../../PWNED'` put `drive.sh` and its log at the FILESYSTEM ROOT: `join(tmpdir(), 'qwen-review-drive-' + server)` normalises the `..` away, so the name escapes the temp dir it is supposed to sit under. And a name holding `;` splits the `bash <script> > <log>` line tmux runs into further commands - the constructed line reads `bash /tmp/…-a; touch /tmp/X; b/drive.sh > …`, which is two commands and a fragment. The value is operator-supplied today, which is why this is a hardening and not a live hole. But the command exists to be called from a review that builds its arguments programmatically, and a server name derived from a branch or a PR title is one step away from here; neither of those is ours to trust. Restricted to a charset that cannot be either - letters, digits, dot, dash, underscore, 64 max - and the paths are quoted anyway. The redundancy is the point: whoever widens the charset later should not also have to notice the shell line. The quoting itself shipped wrong first: hand-escaping `'\''` through a test file produced `'''`, which bash answers with `unexpected EOF`. It is asserted by round trip through real bash now - six shapes including a bare quote, a semicolon and a backtick - rather than against a hand-written expected string, because a hand-written expected string is what got it wrong.
Round-2 finding. The readiness and sentinel loops paced themselves with `sleep 0.25`. Fractional operands are a GNU/BSD extension - POSIX specifies an integer - so on a system without it `sleep` fails, returns instantly, and both loops go tight. Measured through the exec seam, before the fix: **8,196,280 readiness probes in one second**. That is not a spinning CPU, it is the probe hammering the daemon it is waiting for at millions of requests a second and then reporting that the daemon never became ready. A false negative manufactured by the harness - which is the precise failure this command was written to remove, arriving through the wait it uses to remove it. `Atomics.wait` blocks for a real duration with no subprocess and no platform surface. Same measurement after: 5 probes/sec. Pinned by rate rather than by mechanism: the test counts probes per second through the seam and requires the figure to stay in the tens, so it fails for any wait that does not actually wait - including one that goes through the seam at all.
Round-3 finding, and the first fix for it was worse than the bug. `trimCapture` bounded the string this command returns; nothing bounded what the driven script wrote. Measured: 200k lines left a 9.9 MB log while the report stayed at 200 KB — and a drive script is whatever the reviewer wrote, so there is no ceiling at all. This repo has paid for that once already: `build-test`'s disk floors exist because an `npm ci` filled the disk 33 seconds in and then failed every agent after it. The first attempt piped the drive through `head -c`. Measured, not assumed: `head` exits at the cap, the writer takes SIGPIPE mid-loop, and the EXIT trap fires with `$?` from the last successful echo — so a script whose final statement is `exit 5` reported **rc=0**. Not a lost verdict: a fabricated one, a failing run presented as a clean pass. The comment I had written for that fix claimed the sentinel survived; running it took four seconds to disprove. So the size is watched instead, in the poll that was already running, and a drive that crosses the cap is stopped and reported as `overflowed` — its own outcome, with no exit code, because a run this command had to stop is not a run that finished and inventing a code for it is the whole failure above. The sentinel also moved to its own file. Two facts, two channels: the log may be trimmed, the verdict may not. The `head -c` behaviour is pinned by a test that asserts the fabricated `rc=0`, so the shortcut cannot be reintroduced by anyone who reasons about it instead of running it.
Round-4 finding, and the other half of round 3's measurement. The log size was bounded; the directory holding it was not removed. The default server name carries the pid, so every invocation leaves its own tree under the temp dir — measured across this PR's own end-to-end runs, six drives left five directories behind. Removed in the same `finally` that kills the server, and for the same reason: the report is fully in memory by then, so nothing the caller needs is still in there. A caller who passed `--log-path` owns that file and keeps it. All six outcome paths re-driven for real afterwards — completed rc=0, completed rc=17, a `set -e` abort, a timeout, a readiness probe that passes, and one that never does — with zero tmux servers and zero directories left behind.
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 110 passed · 0 failed · 110 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：110 通过 · 0 失败 · 110 总计 Verification reportPR #8349 Deep Verification —
|
| input (driven for real) | control: pre-fix trailing-echo wrapper |
head: shipped trap wrapper |
|---|---|---|
echo ok |
rc=0 (works) | rc=0 |
echo failing; exit 17 |
null — exit skips the trailing echo (bug #2) |
rc=17 |
set -e; false; echo unreachable |
null — abort never reaches the echo | rc=1 |
exit 255 / empty script / echo "it's"; exit 4 / 5000-line output then exit 5 |
— | 255 / 0 / 4 / 5 |
Control fails as predicted on the two exit-driven paths (the bug); head fixes both and holds on every boundary shape. 20/20 assertions in Harness A. The two-channel split is confirmed: script output goes to stdout, the verdict to its own file (a single rc= line), and a fake sentinel the script prints on stdout cannot set the reported code.
Orchestration, against real files (Harness B, 28/28). tmux is absent, so the only thing interposed is the tmux process-supervision layer: the fake new-session runs the constructed shell line in real bash (detached) and readiness probes run real bash; runDrive's real poll loop reads the real log + sentinel file. Witness: 02-orchestration-outcomes.png.
| scenario (real script) | outcome | exitCode | observed | other |
|---|---|---|---|---|
echo ok / …; exit 17 / set -e; false |
completed |
0 / 17 / 1 | true | scratch dir removed, kill-server ≥2 |
echo started; sleep 8 (timeout 2) |
timed-out |
null | false | note flags PARTIAL, early line captured |
700k-line loop then sleep 5; exit 5 |
overflowed |
null (not fabricated) | false | returned <30 s on a 60 s budget; dir removed |
| readiness marker appears after ~1.2 s | completed |
0 | true | readyAfterMs≈1.2 s, probe count small |
The overflowed cell is the round-3 fix proven end to end: a too-loud drive stops as its own outcome with no exit code, rather than the fabricated rc=0 a head -c pipe would have produced.
Corrections to the PR description
- "put
drive.shand its log at the FILESYSTEM ROOT" — magnitude is platform-dependent. Measured here:resolve(join(tmpdir(), 'qwen-review-drive-' + '../../PWNED'))=/tmp/PWNED. The first..is absorbed into the literal segmentqwen-review-drive-..; the second escapes one level, landing in the temp-dir root rather than the per-runqwen-review-drive-<name>namespace. On a deeper macOStmpdir()the same value climbs higher (the author's "root"). The substantive claim — the name escapes the directory it is supposed to sit under — holds and is fixed: the charset gate refuses it before any fs/shell action (Harness C: no tmux action attempted, nothing created). This is a correction to the description's magnitude, not a request to change the code.
Findings
No blocking findings. Three informational notes (none is a defect; none gates merge):
-
(info) The LAST-occurrence rule is pinned but redundant in production.
sentinelExitCodetakes the last match (mutation M2 turns the test red:expected +0 to be 42). ButrunDrivereads the code from the sentinel file, which the trap opens with>(truncate) and writes exactly once — so the file only ever holds one sentinel line, and FIRST vs LAST cannot differ there. The script's own sentinel-shaped stdout never reaches the file (Harness A.3). The rule is defence-in-depth left over from the pre-file design; its comment rationale ("a drive script that cats a log… emits a sentinel-shaped line") no longer describes a path the wiring admits. Harmless and still correct. -
(info) The fabricated-
rc=0test pins the measurement, not the production path. "capping the STREAM would FABRICATE an exit code" runs real bash +head -cand assertsrc=0; it does not callrunDrive, so norunDrivemutation makes it fail. The thing that would actually catch reintroducinghead -cintorunDriveis the log-cap test (mutation M5 →expected 'timed-out' to be 'overflowed', pinned). The design is therefore protected; the PR's phrasing "so the shortcut cannot be reintroduced" is carried by M5, with the fabricated-rc test documenting why. -
(info)
observeddrives the process exit code. The handler setsprocess.exitCode = 1whenever!report.observed, so only a fully-observedcompleteddrive exits 0 (verified via the real CLI:unavailable→ exit 1). This is a sensible contract for a scripting caller and is documented here so a future caller is not surprised that a cleannot-ready/unavailableis non-zero.
Mutation matrix — the tests are not vacuous (0 survivors)
The PR's headline quality claim is "two bugs only a real run could show, both green under the unit tests" and "all three fixes now fail a mutation." Verified by reverting each fix in a scratch copy and running the real drive.test.ts (Harness G, 14/14 scripted assertions; witness 05-mutation-matrix.png). Unmutated control: 24/24 green.
| mutation (fix reverted) | suite result | intended assertion that failed | pinned? |
|---|---|---|---|
M1 trap → trailing echo (bug #2) |
4 failed | 20 pass | expected null to be +0 (exit code lost) |
YES — positive control |
| M2 LAST → FIRST occurrence | 1 failed | 23 pass | expected +0 to be 42 |
YES (redundant in prod, see F1) |
| M3 charset gate disabled | 2 failed | 22 pass | expected 'timed-out' to be 'unavailable' |
YES |
M4 Atomics.wait → no-op |
1 failed | 23 pass | expected 10031556 to be less than 50 |
YES |
| M5 overflow size-watch disabled | 1 failed | 23 pass | expected 'timed-out' to be 'overflowed' |
YES |
M6 cleanup rmSync disabled |
1 failed | 23 pass | expected true to be false |
YES |
Every mutation fails the intended assertion with a behavioural expected-vs-actual message (not an import/compile break). M1 is the positive control proving the harness can make the suite fail. M4's no-op measured 10.0 M probes/s through the real seam, corroborating the PR's "8.2 M" and confirming the rate test is load-bearing; the shipped Atomics.wait measures 5.0/s (Harness E, witness 04-poll-rate-atomics.png; no-op positive control 22.3 M/s — a 7-order gap). Source restored byte-identical after the matrix (git diff empty).
Security hardening (Harness C, 37/37, witness 03-security-hardening.png)
--server '../../PWNED'→unavailable("not a name this command will own"), zero tmux actions attempted, nothing created at the resolved path.--server 'a; touch <tmp>/qwen-pwn-…; b'→unavailable; the injection target was not created (payload never ran).- Charset boundary is exact: 64-char name accepted, 65 rejected; leading
.,-,_rejected;/, space, backtick,$, empty,..rejected; real-world shapes (qr-1234,pr8349,ok-name_1.2,A) accepted. shellQuoteround-trips 10 hostile shapes (space, bare quote,;, backtick,$X\id`, newline,$(reboot), leading dash, unicode) through **real bash** unchanged; and the quoting guard alone (independent of the charset) makes a path containing; $(echo pwned)` create exactly one literal file with no command substitution.
Targeted gates (all green, all proven live)
- Review suite (
npx vitest run src/commands/review src/commands/review.test.ts): 48 files passed, 1457 passed | 3 skipped (1460), exit 0 (PR claimed ~1450). The "positive control FAILED" / "bad revision" lines in the output are fixture outputs from sibling tests that exercise error paths, not failures. Liveness: proven by the mutation matrix (6/6 caught within this suite). - Lint (
eslinton the two changed files): clean, exit 0. Liveness: a planted probe (unusedProbe,any) produced 4 errors (no-unused-vars,no-explicit-any×2, filename convention); probe removed. - Typecheck (
tsc --noEmit, cli package): clean, exit 0. Liveness: a plantedconst x: number = "string"producederror TS2322; restored clean.
A/A control — tmux absence is environmental, not a regression
This lane container has no tmux (command -v tmux empty; the only match is a terminfo entry) and I am not root (apt-get → permission denied; no sudo/nix/conda), so tmux cannot be installed. The PR's literal CLI test plan (the four qwen review drive … exit paths) is therefore structurally unreachable here — but that is exactly the gap drive is designed to report honestly:
- Head, real CLI bundle:
node dist/cli.js review drive --script 'echo ok' --cwd /tmp→{"outcome":"unavailable","observed":false,"exitCode":null,…}, note "tmux is not available… not a finding about the diff", exit 1 (witness06-cli-unavailable-aa.png). Harness F (realspawnSync, no fake): 5/5. - Base (
HEAD^1):review.tsregisters 23 subcommands and has nodriveCommand—driveis the 24th, purely additive. There is no base behaviour to regress; the environment gap is reported, not faked.
The substantive verification lives at the layer the PR's two bugs actually inhabited (the bash wrapper and the poll/cleanup orchestration), which needs no tmux and is proven above.
Not covered
- The real tmux transport, including the
new-session/redirect race (PR bug pre-release: fix ci #1). No tmux daemon exists in this container and it cannot be installed (non-root). Harness B interposes only the tmux process-supervision layer and runs everything else for real; it proves the handling (poll, timeout, overflow, cleanup, sentinel reading) but not the livepipe-pane-vs-redirect trigger the PR describes. A maintainer with tmux should run the four-line loop in the Reviewer Test Plan to close this. - Per-commit attribution. The checkout is depth 2 (merge commit + base tip + PR head only); the PR carries 6 commits, 5 of which are unreachable, so the round-by-round claims (rounds 1–4) were verified in aggregate against
HEAD^1..HEAD, not per commit. - Repo-wide gates. Only the affected workspace (cli) tests/lint/typecheck were run, per scope; no repo-wide suite was executed.
- The
outoption (writes the JSON report to a path) was exercised only indirectly; not separately asserted.
Methodology
Environment: node:22-bookworm lane container, node v22.23.2, no tmux, non-root. Working tree = refs/pull/8349/merge (HEAD 993898b15, base HEAD^1 5f8d08a0c, PR head HEAD^2 75e380ba5). drive.ts was bundled once with esbuild (--bundle --format=esm --platform=node) into harness/drive.bundle.mjs; harnesses A–G import the real exported functions from that bundle and drive them through real bash/spawnSync and real filesystem state — no stub of the code under test. Harness B interposes only the tmux process-supervision layer (the part tmux-absence forces); readiness probes and the driven scripts run real bash. The mutation matrix (G) applies each mutation by exact line number with an anchor guard, runs the real drive.test.ts under vitest, asserts the suite goes red on the intended test, and restores the source byte-identically. Evidence images were rasterised from live runs by scripts/verify-capture.mjs. Raw per-harness logs live in logs/; harness scripts in harness/.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
approved these changes
Aug 2, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
approved these changes
Aug 2, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — well-structured state machine with 5 clear outcomes, strong test coverage using real bash execution, and solid security hardening (server name validation, shell quoting round-trip tests, sentinel-in-file).
Non-blocking suggestions:
- Register SIGINT/SIGTERM handlers that call
tmux('kill-server')before exit, or soften the 'cleanup guaranteed' claim to 'cleanup on normal exit; stale servers reclaimed by next run' - Pass
cwd: args.cwdto the readiness probe's spawnSync so cwd-relative probes work correctly - Check statSync size before readFileSync in the poll loop to avoid re-reading unchanged multi-MiB logs
- Document that
--timeout 0means 'poll once and return immediately'
wenshao
added a commit
that referenced
this pull request
Aug 2, 2026
Conflict: the review subcommand demand message — main added drive (#8349), this branch adds publish-assets; keep both, in registration order. The subcommand surface test auto-merged with both entries. Note: drive.test.ts's SIGPIPE fabricated-exit-code case fails on macOS (bash 3.2 dies on SIGPIPE without running its EXIT trap, so the sentinel is absent rather than 0) and passes on CI's ubuntu leg — pre-existing on main, identical bytes on both sides of this merge, out of this PR's scope.
This was referenced Aug 2, 2026
Collaborator
|
Released in v0.21.4. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds
qwen review drive: start something, wait until it is really up, drive it, capture what it did — as facts rather than as a guess about how long to sleep.The highest-yield review technique in this repo's history is the local build-and-drive verification ("我是维护者，帮我在本地构建真实测试验证PR N"). Across 260 of those sessions the mechanical half is the same every time, done by hand every time, and two of its three steps are done by guessing:
sleep Nsleep 2landing before the daemon binds its port captures an empty screen — and an empty screen reads as "the feature does not work"pkill -f <a name they made up>)driveowns exactly those three — ready or not, finished or not, gone either way — and nothing else. What to drive, and what the output means, stay with the caller: the same splitbuild-testandtest-deltaalready draw.Every outcome is a fact about the run, never a verdict about the diff:
completed— the sentinel was reached;exitCodeis the script's own.not-ready— readiness never arrived, so nothing was driven and nothing observed is evidence either way.timed-out— driven, sentinel never appeared; the capture is reported and flagged partial, because a partial capture is not evidence that the run produced nothing.overflowed— stopped because its output crossed the log cap: no exit code, because a run this command had to stop is not one that finished, and inventing a code for it is the failure the cap was added to avoid.unavailable— no tmux, or the session would not start: an environment gap, explicitly not a finding.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review src/commands/review.test.ts— expect 48 files / 1450 tests green.Expect
completed/0,completed/17,completed/1,timed-out/null.Evidence (Before & After)
Two bugs that only a real run could show — both green under the unit tests at the time:
pipe-paneraces the script.new-sessionstarts it immediately and the pipe attaches after, so a fast drive finishes — taking its session with it — before the pipe exists. Measured: a one-second delay makespipe-paneitself exit 1 and the log stay empty, which this command then reported astimed-out. A pane is a window that closes; the script's own redirect is the record.The sentinel was a trailing
echo, whichexit Nnever reaches — andexit Nis exactly how a drive script reports its result. Measured:echo failing; exit 17came backtimed-outwith a null exit code, i.e. a run that answered in milliseconds reported as one that never finished.set +ehas no bearing onexit; atrap … EXITdoes, and covers falling off the end, an explicit exit, and aset -eabort alike.The test that should have caught the second one asserted that
set +ewas present — the call shape, not the behaviour. It is replaced by tests that drive real bash through all four exit paths. A third assertion ("reads the last sentinel") turned out to pin nothing — its rationale had evaporated when the capture moved off the pane — so it was rewritten against the reason that actually holds: the trap's sentinel is by construction the final line, so a drive script that cats a log cannot set the exit code this command reports. All three fixes now fail a mutation. Non-UI: N/A.Tested on
Risk & Scope
tmux; its absence is reported asunavailablerather than failing a review.中文说明
本 PR 做了什么
新增
qwen review drive：把东西起起来、等它真的就绪、驱动它、抓取它做了什么 —— 作为事实，而不是靠猜该 sleep 多久。这个仓库历史上产出最高的评审手法，是本地的构建并驱动验证（"我是维护者，帮我在本地构建真实测试验证PR N"）。在 260 个这类会话里，机械的那一半每次都相同、每次都手工做，而其中三步有两步是靠猜的：
sleep N等待sleep 2若落在 daemon 绑定端口之前，抓到的是空屏 —— 而空屏会被读成"这个功能不工作"pkill -f <自己编的名字>）drive只拥有这三件事 —— 就绪与否、完成与否、无论如何都清理干净 —— 别的都不管。驱动什么、差异意味着什么，仍归调用方：与build-test、test-delta的边界划法一致。每个 outcome 都是关于这次运行的事实，绝不是对 diff 的裁定：
completed抓到哨兵、退出码是脚本自己的；not-ready就绪从未到达，什么都没驱动，因此观察到的一切都不构成任一方向的证据；timed-out驱动了但哨兵未出现，capture 照常报出并标注为部分，因为部分 capture 不等于"这次运行什么也没产生"；overflowed因输出超过日志上限而被中止 —— 不给退出码，因为一个被本命令强行停下的运行不是一个完成的运行，给它编一个退出码正是加这个上限要避免的失效；unavailable没有 tmux 或会话起不来 —— 环境缺口，明确不是 finding。证据（前后对比）
两个只有真跑才会暴露的 bug，当时单测全绿：
pipe-pane与脚本竞态：new-session一启动脚本就跑，pipe-pane之后才附着，快脚本早已结束并带走会话。实测：延迟 1 秒后pipe-pane自身 exit 1、日志为空，而本命令会把它报成timed-out。pane 是会关的窗口，脚本自己的重定向才是记录。哨兵写在末尾的
echo里，exit N永远到不了 —— 而exit N正是驱动脚本报告结果的常规写法。实测：echo failing; exit 17报成timed-out+ 退出码为 null，即一个毫秒内就给出答案的运行，被报成从未完成。set +e对exit无效，trap … EXIT才有效，且同时覆盖正常结束、显式 exit、set -e中止三种路径。本该抓住第二个 bug 的那条测试，断言的是"
set +e存在"—— 调用形状而非行为。已替换为真跑 bash、覆盖四种退出路径的行为测试。第三条断言（"读最后一个哨兵"）经检验什么也没钉住——它的理由在 capture 改为脚本重定向后已不成立——于是按真正成立的理由重写：trap 的哨兵按构造一定是最后一行，因此驱动脚本 cat 出来的日志无法决定本命令报告的退出码。三处修复现均可被突变检出。风险与影响范围
tmux；缺失时报unavailable，不会让评审失败。