feat(review): make Step 4 verification execution-grade - #9740
Conversation
|
Re-run at the author's Template looks good ✓ Problem: observed, not theoretical. Three of the four pieces mechanize steps that maintainer verifications have been re-assembling by hand on every run, and the PR links the concrete incident this grew out of — the #9131 verification, whose sharpest notes were harness side effects with nowhere to go. The fourth piece closes a real hole: the witness rule gated only Criticals, so an unexecuted claim could still reach the author through the Suggestion door. Direction: aligned. This is the repo's own review pipeline gaining execution-grade evidence forms — the same direction as Size: grown again since the last triage pass — now +6,570/−176 over 21 files. The split: 2,748 production logic lines (revert-hunk 1,246 · ab-drive 987 · drive 251 · pr-context 96 · findings 74 · diff-plan 43 · stdioHelpers 24 · agent-briefs 21 · review.ts 6), 3,948 test lines, and ~50 lines of bundled-skill prose — the prose plus one formatting-only test touch being the only Approach: the four pieces interlock around one contract — the fenced, witnessed evidence — and the drive.ts hardening is shared machinery ab-drive reuses rather than forks, which is the right shape. Two things ride along that the body doesn't name as pieces: the Risk: no elevated risk signals — none of the changed files matches the revert-correlated paths. The tmux-dependent surface reports unavailable on Windows by design, same as Moving on to code review. 🔍 中文说明应作者 模板完整 ✓ 问题：已观测、非理论。四块中的三块是把维护者验证每次都要手工重搭的步骤固化为命令，PR 也关联了具体起因——#9131 验证中最锋利的几条 note 全是驱动真实栈的副产品、此前无处安放。第四块补的是真窟窿：witness 规则此前只卡 Critical，未经执行的断言仍能从 Suggestion 这扇门到达作者。 方向：对齐。这是本仓库自己的评审管线补上"可执行证据"形态——与 规模：较上次 triage 又增长——现 +6,570/−176、21 个文件。构成：2,748 行生产逻辑（revert-hunk 1,246 · ab-drive 987 · drive 251 · pr-context 96 · findings 74 · diff-plan 43 · stdioHelpers 24 · agent-briefs 21 · review.ts 6）、3,948 行测试、约 50 行 bundled-skill 文档——文档与一处纯格式化的测试改动是仅有的 方案：四块围绕同一契约（带围栏的见证证据）互相衔接；drive.ts 的加固是 ab-drive 直接复用而非另起炉灶的共享机制，形态正确。有两处 body 未点名的随行改动：pr-context.ts 中 风险：无升级风险信号——变更文件均未命中与 revert 相关的高风险路径。依赖 tmux 的表面在 Windows 上按设计报 unavailable，与 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review — no blockers found. This pass re-read the whole diff at the head carrying all 17 rounds plus the merge of
Files changed (21 of 21 shown)
Testing evidence — the PR's own CI at the reviewed commit (unattended run; this gate never executes PR code):
The ubuntu unit/integration suite is green on the exact reviewed head, and both Sandboxed verification would settle the remaining behavioural gap: this PR's central claims — that the paired A/B and the hunk revert hold up on a live stack, and that the widened witness demotion fires mechanically — are exactly what a green suite alone does not prove. The isolated 中文说明代码审查——未发现阻塞项。本遍在承载全部 17 轮 + 合并
测试证据（无人值守运行；门禁从不执行 PR 代码）：被审 commit 上 ubuntu 单元/集成套件为绿，两个 沙箱验证用于收口剩余行为缺口：本 PR 的核心主张——配对 A/B 与 hunk 还原在真实栈上成立、witness 降级机械生效——正是绿色套件本身无法证明的。隔离的 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean re-read at the fully-hardened head; no blockers, only non-blocking notes (an under-named fifth piece, one formatting drive-by, and the documented per-arm teardown residual). Stepping back: my independent proposal for this problem was exactly the shape this PR took — one paired-runner command that reuses the existing drive machinery instead of forking it, hunk mutation through git's own patch engine instead of sed ranges, and the witness gate widened at the canonicalization point where it is already enforced. The PR matches that proposal and goes past it in the places that matter: the Two things a reader of this thread should keep in mind, and neither is mine to resolve:
Verdict: approve. Approving pinned to the reviewed commit below. 中文说明置信度：4/5 —— 在完全加固后的 head 上重读，干净；无阻塞项，只有非阻塞备注（未点名的第五块、一处格式化顺手改动、以及已成文的 per-arm teardown 残余）。 退一步看：我对这个问题的独立方案正是本 PR 落成的形态——一个复用既有 drive 机制而非另起炉灶的配对执行命令、走 git 自身补丁引擎而非 sed 区段的 hunk 变异、以及在既已执行收口处（规范化）放宽的 witness 门禁。PR 与这个方案一致，并在要紧的地方走得更远： 两点提醒后续读者，都不由我裁决：
结论：批准。批准固定于下方被审 commit。 — 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. |
8267751 to
12d01f0
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
7205f9f to
7ba7370
Compare
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 — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| for (const [flag, p] of [ | ||
| ['--arm-a', args.armA], | ||
| ['--arm-b', args.armB], | ||
| ] as const) { | ||
| if (!existsSync(resolve(p))) { |
There was a problem hiding this comment.
[Critical] R1-1: The arm pre-flight only checks existsSync, so an arm path that exists but is not a directory passes the guard; tmux new-session -c <file> succeeds with a silent cwd fallback to $HOME, and the observed gate licenses a comparison in which that arm never ran in its tree. --shared-cwd has no check at all. A verifier who passes --arm-a out/base-report.json (a file — this pre-flight exists to catch such path mix-ups and its message claims "does not exist") gets: existsSync passes, the session starts, the wrapper runs the script from $HOME (the wrapper's set +e makes even a defensive cd fail open), the sentinel is written, the arm reports completed with root claiming the file path, and if arm b completes the report says observed: true — an A/B verdict where arm a measured $HOME; the arm's relative writes also land in $HOME. Only an omitted --ready would trip. Fix: check statSync(resolve(p)).isDirectory() with a fail("… is not an existing directory — nothing was started.") branch, and apply the same check to the resolved sharedCwd before start().
Witness:
[probe] tmux 3.4: new-session -d -c <existing FILE> bash -lc 'pwd' → exit 0, runs from $HOME
End-to-end through unmodified runAbDrive, armA = an existing file:
"observed": true, a.root claims the file path,
a.output = "/home/github-runner\nmarker=cat: marker.txt: No such file or directory"
FIXED (isDirectory guard): observed:false,
"--arm-a … is not an existing directory — nothing was started", suite 11/11
中文说明
arm 预检只查 existsSync：存在但不是目录的路径也能通过；tmux new-session -c <文件> 会静默回退到 $HOME 作为 cwd 并成功，observed 门随后给一个"该臂根本没在自己的树里跑过"的对比背书。--shared-cwd 则完全没有任何检查。验证者若传 --arm-a out/base-report.json（一个文件——这个预检本来就是为抓这类路径混淆而存在的，报错文案还写着 "does not exist"）：existsSync 通过、会话启动、脚本实际在 $HOME 里执行（wrapper 的 set +e 连防御性的 cd 都会静默失败）、sentinel 照写、该臂报告 completed 且 root 声称是那个文件路径，另一臂完成后报告 observed: true——一份 arm a 实际量的是 $HOME 的 A/B 裁决；相对路径写入也会落在 $HOME。只有不传 --ready 才会踩空。修复：用 statSync(resolve(p)).isDirectory() 判断并走 fail("… is not an existing directory — nothing was started.") 分支，sharedCwd 在 start() 前做同样检查。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const droveFrom = Date.now(); | ||
| const deadline = droveFrom + args.timeout * 1000; | ||
| let outcome: DriveOutcome = 'timed-out'; |
There was a problem hiding this comment.
[Critical] R1-2: Non-finite numeric options (--timeout abc → NaN through yargs type:'number', --timeout Infinity; same for --ready-timeout/--shared-ready-timeout) pass validation and disable every time bound, because Date.now() >= NaN (and >= Infinity) is never true. With --timeout abc the arm deadline becomes NaN, so the poll loop only ends on sentinel or log overflow: a script that hangs without output hangs the command forever — finally's kill-server (the only one) never runs and the tmux server and sessions leak; NaN in pollReady's deadline makes readiness polling endless. The file's SERVER_NAME_RE comment names programmatically-built arguments as the trust model, but nothing validates the numbers. Fix: in runAbDrive's pre-flight reject non-finite/non-positive values per flag — if (!Number.isFinite(v) || v <= 0) return fail(…) — matching the existing SERVER_NAME_RE pre-flight style.
Witness:
[probe] real abDriveCommand.builder, yargs 17.7.2: --timeout abc →
failCalled:false, timeoutIsNaN:true, nowGreaterEqDeadline:false (review.ts has no coerce)
CONTROL timeout=0.3 → {"outcome":"timed-out","ms":1002}
NaN timeout child → no output, killed at 6s (exit 124)
FIXED: {"ms":1,"note":"--timeout must be a positive, finite number of seconds (got NaN) — nothing was started."}
中文说明
非有限数值选项（--timeout abc 经 yargs type:'number' 得到 NaN、--timeout Infinity；--ready-timeout/--shared-ready-timeout 同理）能通过校验并禁用所有时间边界——因为 Date.now() >= NaN（以及 >= Infinity）永远不为真。--timeout abc 使 arm deadline 变成 NaN，轮询只剩 sentinel 或日志溢出两个出口：脚本若挂起且不输出，整个命令永久挂起——finally 里的 kill-server（唯一一处）永远不执行，tmux server 与会话全部泄漏；pollReady 的 deadline 为 NaN 时就绪轮询同样无限循环。本文件 SERVER_NAME_RE 的注释明确把"程序构造的参数"列为信任模型，但数值没有任何校验。修复：在 runAbDrive 预检里逐 flag 拒绝非有限/非正值，风格与 SERVER_NAME_RE 预检一致。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (exitCode !== null) { | ||
| outcome = 'completed'; | ||
| break; | ||
| } |
There was a problem hiding this comment.
[Critical] R1-3: The completed branch breaks on the log snapshot taken BEFORE the sentinel check, omitting the post-sentinel re-read that drive.ts's analogous loop explicitly added and documented for this exact race. wrapScript writes the sentinel from an EXIT trap strictly after the script's last log write; if that last write lands between this readFileSync(logPath) and the sentinel read on the same poll iteration, the arm's capture permanently loses the script's final output. drive.ts documents this window at ~1-in-70 completions on near-cap logs. Amplified here: if arm a hits the race and arm b doesn't, two byte-identical runs produce identicalOutput: false — a harness-fabricated difference of exactly the kind the module header says is "the one failure that matters", quoted by a verifier from a capture missing its last line. Fix: mirror drive.ts — inside the exitCode !== null branch, re-read the log before setting outcome; every log write happens-before the sentinel write, so a read taken after it is complete.
Witness:
[probe] deterministic fs-layer model of the documented race:
PR: arm-a capture = "early output\n" — the FINAL-TAIL-LINE present in the
file at completion is missing
With the drive.ts-style re-read: "early output\nFINAL-TAIL-LINE\n"
中文说明
completed 分支用的是 sentinel 检查之前拍的日志快照就 break 了，漏掉了 drive.ts 同类循环专门加上并写明的那个"sentinel 之后重读日志"。wrapScript 的 sentinel 由 EXIT trap 在脚本最后一次日志写入之后才写；如果最后那一次写恰好落在同一次轮询的 readFileSync(logPath) 与 sentinel 读之间，该臂的捕获就永久丢失脚本的最后一段输出。drive.ts 记录这个窗口在近上限日志上约 1/70 的完成率。在此处危害被放大：arm a 踩中而 arm b 没踩中时，两份字节级相同的运行会得出 identicalOutput: false——正是模块头部所说"唯一真正要紧的失败"那种由 harness 伪造出来的差异，验证者还会引用一份缺了最后一行的捕获。修复：照 drive.ts 做——在 exitCode !== null 分支里、置 outcome 之前重读日志；所有日志写都 happens-before sentinel 写，之后读到的一定完整。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (args.shared && mode === 'per-arm') { | ||
| tmux('kill-session', '-t', `shared-${arm}`); | ||
| sharedRc = null; | ||
| } |
There was a problem hiding this comment.
[Critical] R1-4: An arm that ends timed-out or overflowed is never killed — the per-arm teardown kills only the shared session, and the finally kill-server runs only after arm b's entire run, so the arm-${arm} session keeps executing through it. Arm a times out (or overflows) holding whatever its script holds; the same script then runs as arm b, by construction contending for the same ports/files — a script that binds a fixed port leaves arm b failing with EADDRINUSE, and arm b's capture measures arm a's zombie, a confound the report never names. On overflowed it is also a resource leak: LOG_MAX_BYTES only breaks observation, and the writer keeps growing the log unbounded through arm b's whole window (up to ~7 min by default), the disk-filling mode the cap exists to stop. Fix: after the poll loop, unconditionally tmux('kill-session', '-t', \arm-${arm}`)` — harmless when the session already exited on completion, and it bounds the timed-out/overflowed script to its own arm.
Witness:
[probe] real tmux.
Drive 1 (arm a: sleep 30, --timeout 3): a=timed-out, b=completed, observed:false;
arm b's listing of the drive's OWN server while b runs: arm-a session alive: true
— the zombie lived exactly through b's window, unnamed in the report
Drive 2 (arm a ~10MB/s writer past LOG_MAX_BYTES): a=overflowed;
arm-a.log sampled during arm b's window: t0=12582912 → t2s=31457280 (+~18MB in 2s)
中文说明
以 timed-out 或 overflowed 结束的 arm 从不被杀——每臂收尾只杀 shared 会话，finally 的 kill-server 要等 arm b 整个跑完才执行，所以 arm-${arm} 会话在 arm b 期间继续运行。arm a 超时（或溢出）时握着脚本持有的一切资源；同一脚本随后作为 arm b 再跑，按构造就会争同样的端口/文件——绑固定端口的脚本会让 arm b 以 EADDRINUSE 失败，arm b 的捕获利到的是 arm a 的僵尸，报告对此只字不提。overflowed 时还是资源泄漏：LOG_MAX_BYTES 只中断观察，写入方会在 arm b 的整个窗口（默认最长约 7 分钟）里无限增大日志——正是这个上限要防的写满磁盘。修复：轮询循环结束后无条件 tmux('kill-session', '-t', \arm-${arm}`)`——正常完成的会话杀一下无副作用，同时把超时/溢出的脚本限制在自己的臂内。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ); | ||
| if (ms === null) { | ||
| note = `the shared process never became ready${mode === 'per-arm' ? ` for arm ${arm}` : ''} within ${args.sharedReadyTimeout}s (\`${args.sharedReady}\`) — nothing was driven${mode === 'per-arm' ? ' for this arm' : ''}, so nothing here is evidence either way.`; | ||
| return mode === 'once' ? 'stop' : bail('not-ready', null); |
There was a problem hiding this comment.
[Critical] R1-5: In per-arm --shared mode the shared session is killed only on the post-loop teardown path; all three bail() returns that fire AFTER the shared instance was started (shared-ready timeout ~:341, arm --ready timeout ~:351, arm-session start failure ~:358) return without killing it, contradicting the header's "killed unconditionally" invariant. Per-arm run with --shared 'node mock.js --port 8080' where arm a's --ready probe times out: shared-a leaks holding the port; arm b starts shared-b, which cannot bind 8080 and dies at birth → the report presents arm b as having watched a dead upstream and the note tells the verifier to fix arm b's shared script — misdirected repair. If shared-b survives startup while --shared-ready probes the port held by the leaked instance, arm b is driven against arm a's state — cross-arm contamination. (observed stays false because arm a did not complete — the costs are the contaminated/wasted arm-b drive, the misdiagnosis, and a live process leak across arms.) Fix: kill the shared session on every post-start exit of runArm in per-arm mode — e.g. at the top of bail(): if (args.shared && mode === 'per-arm' && sharedRc !== null) { tmux('kill-session', '-t', \shared-${arm}`); sharedRc = null; }` (the sharedRc guard keeps pre-shared-start bail paths correct).
Witness:
[probe] harnessed exec, per-arm shared, arm-ready probe forced to fail:
PR exec sequence: [kill-server, new-session(hold), new-session(shared-a),
new-session(shared-b), kill-server] — three new-sessions, ZERO kill-sessions;
shared-b started while shared-a alive
With the bail()-kill fix: … kill-session(shared-a) before new-session(shared-b) …
中文说明
per-arm --shared 模式下 shared 会话只在轮询循环之后的收尾路径被杀；在 shared 实例已经启动之后触发的三条 bail() 返回（shared-ready 超时 ~:341、arm --ready 超时 ~:351、arm 会话启动失败 ~:358）都没有杀它就直接返回，违反文件头部"无条件击杀"的不变量。场景：--shared 'node mock.js --port 8080' 且 arm a 的 --ready 探针超时——shared-a 占着端口泄漏；arm b 启动 shared-b 时绑不上 8080、出生即死→报告把 arm b 呈现为"看着一个死掉的上游"，提示词还让验证者去修 arm b 的 shared 脚本——修错对象。若 shared-b 能活过启动而 --shared-ready 探活的是泄漏实例占着的端口，arm b 就是被 arm a 的状态驱动的——跨臂污染。（observed 因 arm a 未完成仍为 false——代价是臂 b 的驱动被污染/浪费、误诊断、以及跨臂的活进程泄漏。）修复：在 per-arm 模式下 runArm 的每个启动后出口都杀 shared 会话——例如 bail() 顶部加上带 sharedRc !== null 守卫的 kill-session（守卫保住 shared 启动前的 bail 路径）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| /** Test seam — production shells out to the real git. */ | ||
| exec?: ( | ||
| cwd: string, |
There was a problem hiding this comment.
[Suggestion] R1-23: RevertHunkArgs.exec is a dead switch: declared and read (const exec = args.exec ?? gitApply;, ~line 196) but never set by any caller — every test in the PR's own new suite and the sole production call site pass only {diff, tree, hunk} (sweep: 0 of 6 call sites set exec). AGENTS.md mandates grepping read sites of every added optional parameter; the sibling commands drive their seams constantly (exec injections: build-test ~90, drive 11, ab-drive 8, test-delta 1 — revert-hunk 0). The injected-exec branch is code no test ever executes, so a change to the seam's contract (or to how a provided exec's result is consumed at the two git call sites) survives the suite green. Meanwhile the suite pays exactly the costs the seam exists to avoid: spawning real git leaks 7 real repos into os.tmpdir() per run (R1-10) and depends on host git ≥ 2.28 for git init -b. Fix: either drive the failure/race branches (the --check refusal, the check-passed-but-apply-failed path, the spawn-error branch of R1-7) through the exec seam in the new test file, or delete the seam and the ?? gitApply fallback.
Witness:
[probe] sweep over the real caller population: 0 of 6 runRevertHunk call sites
set exec; one suite run (9/9 passed) leaked exactly 7 fresh /tmp/rh-* git repos
中文说明
RevertHunkArgs.exec 是死开关：声明并被读取（const exec = args.exec ?? gitApply;，约 196 行）但没有任何调用方设置它——本 PR 新套件的所有测试与唯一生产调用点都只传 {diff, tree, hunk}（扫描：6 个调用点中 0 个设置 exec）。AGENTS.md 要求对每个新增可选参数 grep 其读取点；兄弟命令都在持续驱动各自的 seam（exec 注入：build-test 约 90、drive 11、ab-drive 8、test-delta 1——revert-hunk 为 0）。注入 exec 的分支是从未被测试执行过的代码，对 seam 契约的改动（或两个 git 调用点消费 exec 结果的方式）可以全绿上线。同时套件正在支付 seam 本要避免的代价：真实调用 git 每次运行泄漏 7 个真实仓库到 os.tmpdir()（R1-10），且依赖宿主 git ≥ 2.28 的 git init -b。修复：要么在新测试文件里经由 exec seam 驱动失败/竞态分支（--check 拒绝、check 通过但 apply 失败、R1-7 的 spawn 错误分支），要么删掉 seam 与 ?? gitApply 回退。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const dir = join(tmpdir(), `qwen-review-revert-hunk-${process.pid}`); | ||
| mkdirSync(dir, { recursive: true }); |
There was a problem hiding this comment.
[Suggestion] R1-24: The patch staging directory is a predictable, pid-keyed path in the shared OS temp dir, created without exclusivity; writeFileSync(patchPath) follows a pre-planted symlink at that path. On a shared host (CI runners), a local peer enumerates PIDs and pre-creates /tmp/qwen-review-revert-hunk-<pid> as a symlink to a directory they choose; mkdirSync(recursive) succeeds silently against the symlinked target, and writeFileSync then creates or overwrites hunk.patch at the attacker-chosen location (a second planted symlink at <dir>/hunk.patch extends this to an arbitrary file path). Harm is bounded to a file named hunk.patch with diff-derived content; trigger requires a local peer on a shared machine. The codebase's established idiom is mkdtempSync (1289 call sites under packages/), which creates a unique 0700 directory. Fix: const dir = mkdtempSync(join(tmpdir(), 'qwen-review-revert-hunk-')); (drop the pid from the name; keep the rmSync in finally). The dir is purely internal staging — nothing outside runRevertHunk reads it — so this is a drop-in fix.
Witness:
[probe] probe replicating revert-hunk.ts:191-195, symlink pre-planted at the
pid-keyed path: mkdirSync(recursive) succeeded silently; writeFileSync followed
the chain and overwrote the attacker-chosen file ("VICTIM-EXISTING-CONTENT" →
"PATCH-CONTENT-FROM-DIFF"); after the victim's finally{rmSync} the written file
persists at the attacker location
中文说明
补丁暂存目录是共享系统临时目录里一个可预测的、以 pid 为键的路径，创建时不带排他性；writeFileSync(patchPath) 会跟随预先植入的符号链接。在共享主机（CI runner）上，本地同伴枚举 PID 并预先创建 /tmp/qwen-review-revert-hunk-<pid> 作为指向其选定目录的符号链接；mkdirSync(recursive) 对被链接的目标静默成功，随后 writeFileSync 在攻击者选定的位置创建或覆盖 hunk.patch（在 <dir>/hunk.patch 再植一个符号链接可扩展到任意文件路径）。危害限于一个名为 hunk.patch、内容为 diff 派生的文件；触发需要同机本地同伴。代码库既定写法是 mkdtempSync（packages/ 下 1289 处调用），它创建唯一的 0700 目录。修复：const dir = mkdtempSync(join(tmpdir(), 'qwen-review-revert-hunk-'));（名字里去掉 pid；保留 finally 里的 rmSync）。该目录纯属内部暂存——runRevertHunk 之外无人读取——因此是即插即用的修复。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * nothing — so downstream it counts as no witness at all. | ||
| */ | ||
| export function isEmptyNotRunWitness(witness: string): boolean { | ||
| return /^\s*(?:witness:\s*)?not run\s*[—–-]*\s*$/i.test(witness); |
There was a problem hiding this comment.
[Suggestion] R1-25: isEmptyNotRunWitness enumerates only three dash glyphs (—, –, -); a reason-less not run line carrying any other dash-like character is not recognized as empty and counts as a witness, defeating the rule this PR's own SKILL.md change states ("a reason-less not run line now counts as no witness at all"). A non-compliant verifier writes the escape hatch with the reason omitted and an unenumerated dash — not run ― (U+2015), not run − (U+2212), not run － (U+FF0D), or not run ー (U+30FC, common in the CJK output this pipeline's model produces): isEmptyNotRunWitness returns false, holdUnwitnessedFindings treats the line as a valid witness, and the unexecuted, reason-less Critical/Suggestion stands at high confidence, passes isPostable, and is posted to the PR — the exact failure the rule was built to hold back. DESIGN.md's "quality of a named reason stays with humans" argues against parsing reason substance; it does not argue for the dash-class enumeration. Fix: test for "nothing substantive follows not run" instead of enumerating dash glyphs, e.g. /^\s*(?:witness:\s*)?not run\W*$/iu — any real reason must contribute at least one word character (add U+30FC explicitly if it must also count as a dash, since it is a word character).
Witness:
[probe] real isEmptyNotRunWitness/holdUnwitnessedFindings driven through vitest:
"not run —" (U+2014) isEmpty=true; U+2013, '-' = true
"not run ―" (U+2015) = false; U+2212, U+FF0D, U+30FC = false
hold(reason-less U+2015): confidence=high, unwitnessed=[] — stands and posts
control (em dash, reason-less): confidence=low, demoted
中文说明
isEmptyNotRunWitness 只枚举三种破折号字形（—、–、-）；带其他破折号类字符的空理由 not run 行不被识别为"空"，反而被当作有效 witness，恰好击败本 PR 在 SKILL.md 里新写的规则（"空理由的 not run 行按无 witness 处理"）。不守格式的验证者写出理由省略、破折号不在枚举内的逃生句——not run ―（U+2015）、not run −（U+2212）、not run －（U+FF0D）、not run ー（U+30FC，在本管线模型的 CJK 输出中常见）：isEmptyNotRunWitness 返回 false，holdUnwitnessedFindings 把该行当作有效 witness，未经执行、无理由的 Critical/Suggestion 保持高置信、通过 isPostable、被贴上 PR——正是这条规则要挡住的失败。DESIGN.md 的"理由质量交给人类判断"反对的是解析理由内容，并不为破折号字形枚举背书。修复：改为判断"not run 之后没有实质内容"而不是枚举破折号，如 /^\s*(?:witness:\s*)?not run\W*$/iu——真实理由至少要贡献一个词字符（若 U+30FC 也要算破折号，需显式加入，因为它是词字符）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| (f.severity !== 'Critical' && f.severity !== 'Suggestion') || | ||
| f.confidence !== 'high' || | ||
| f.source !== 'review' || | ||
| f.witness !== undefined | ||
| (f.witness !== undefined && !isEmptyNotRunWitness(f.witness)) |
There was a problem hiding this comment.
[Suggestion] R1-26: Extending the witness rule to Suggestions makes it re-judge the findings that holdCriticalsFailingOnBase demoted Critical→Suggestion moments earlier in the same handler: an unwitnessed measurement-held finding is dropped to low confidence and silently loses the PR surface the hold's "Downgrade, never drop" contract promises — where before this diff it posted as a Suggestion. Trigger (the ordinary finder shape, exactly the module base fixture): a test command is red on a shared file and a review-source Critical names that file with no witness. The hold demotes it to Suggestion keeping confidence high; this extended predicate then judges that Suggestion and drops it to low; isPostable requires high, so the finding — carrying the measurement note whose own text tells the reader how to re-raise it ("say which test, quote both sides, and file it at Critical again") — never reaches Step 7's comments at all. Neither the hold's docstring ("A Suggestion stays in front of a human") nor DESIGN.md's witness-rule section acknowledges the composed state, and the measurement-hold handler test feeds precisely this shape yet asserts only severity and the appended sentence. Fix: decide the composition deliberately and pin it — if held findings should stay postable, exempt f.heldByMeasurement !== undefined here; if the double demotion is intended, add a handler test asserting the held unwitnessed finding lands at low confidence and projects zero anchor requests, and reconcile the hold's docstring with it.
Witness:
[probe] the real findingsCommand.handler on the ordinary shape: stderr prints
"R1-3 held back from Critical — test-delta measured … failing on the merge base
too" immediately followed by "R1-3 filed at low confidence — … neither a witness
nor a 'not run' reason"; final state {"severity":"Suggestion","confidence":"low",
"heldByMeasurement":{…},"isPostable":false}
中文说明
把 witness 规则扩展到 Suggestion，会让它重新裁决同一 handler 里刚被 holdCriticalsFailingOnBase 从 Critical 降为 Suggestion 的发现：无 witness 的被测保留发现被降到低置信，静默失去该保留机制"降级但绝不丢弃"契约承诺的 PR 露面——而本 diff 之前它会以 Suggestion 身份发布。触发（最普通的 finder 形态，正是模块 base 夹具的形状）：某测试命令在 shared 文件上红，一条 review 来源、无 witness 的 Critical 指向该文件。保留机制把它降为 Suggestion 并保持高置信；随后这个扩展谓词裁决该 Suggestion、降为低置信；isPostable 要求高置信，于是这条发现——携带的测量备注里还写着如何恢复（"说明是哪个测试、引用两边、按 Critical 重新提交"）——根本到不了 Step 7 的评论。保留机制的 docstring（"Suggestion 留在人类面前"）与 DESIGN.md 的 witness 规则小节都没有承认这个组合状态；测量保留的 handler 测试喂的正是这个形态，却只断言严重级与追加的句子。修复：有意识地决定这个组合并钉住它——若被保留的发现应当仍可发布，就在此豁免 f.heldByMeasurement !== undefined；若双重降级是有意为之，补一个断言"被保留且无 witness 的发现落到低置信且锚点请求为零"的测试，并让保留机制的 docstring 与之自洽。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ) { | ||
| return f; | ||
| } | ||
| unwitnessed.push(f.id); | ||
| return { | ||
| ...f, | ||
| confidence: 'low' as Confidence, | ||
| failureScenario: `${f.failureScenario}\n\nFiled at low confidence by the witness rule: this confirmed Critical arrived with neither a witness (the executed evidence that settled the verdict) nor a \`not run — <reason>\` line. Attach either and it stands at high confidence again.`, | ||
| failureScenario: `${f.failureScenario}\n\nFiled at low confidence by the witness rule: this confirmed ${f.severity} arrived with neither a witness (the executed evidence that settled the verdict) nor a \`not run — <reason>\` line naming why no run could settle it. Attach either and it stands at high confidence again.`, |
There was a problem hiding this comment.
[Suggestion] R1-27: The demotion sentence interpolates ${f.severity} (changed from hardcoded "Critical" precisely because Suggestions are now demoted), but no test pins the interpolation: the new assertions only check toContain('witness rule'), and all handler-level demotion tests use a Critical fixture. The mutation ${f.severity} → hardcoded 'Critical' ships green (verified: 120/120 pass), and every demoted Suggestion's canonical artifact and terminal rendering would then read "this confirmed Critical arrived with neither a witness…" — mislabelling the severity in exactly the sentence a human reads to understand the demotion and how to lift it. Fix: in the new 'judges Suggestions on the same terms' test, add expect(findings[0].failureScenario).toContain('this confirmed Suggestion') (and optionally the matching 'this confirmed Critical' in the existing Critical test).
Witness:
[probe] MUTATED (${f.severity} → 'Critical'): Tests 120 passed (120)
FLIP (added toContain('this confirmed Suggestion')): 1 failed | 119 passed
with "+ … this confirmed Critical arrived …"
UNMUTATED with the same added assertion: 120 passed
中文说明
降级句子插值了 ${f.severity}（从写死的 "Critical" 改过来，正是因为现在 Suggestion 也会被降级），但没有任何测试钉住这个插值：新增断言只查 toContain('witness rule')，handler 层的降级测试全部使用 Critical 夹具。变异 ${f.severity} → 写死 'Critical' 可以全绿上线（已验证：120/120 通过），届时每条被降级 Suggestion 的规范产物与终端渲染都会写成 "this confirmed Critical arrived with neither a witness…"——恰恰在人类用来理解降级与恢复方式的那句话里标错严重级。修复：在新的 'judges Suggestions on the same terms' 测试里加 expect(findings[0].failureScenario).toContain('this confirmed Suggestion')（可选地在既有 Critical 测试里加对应的 'this confirmed Critical'）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
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:
- R2-35 keeper (hold) session unpinned on no-shared tests — already reported (comment 3837030344) at ab-drive.test.ts:224
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const dir = join(tmpdir(), `qwen-review-ab-drive-${args.server}`); | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| mkdirSync(dir, { recursive: true }); |
There was a problem hiding this comment.
[Critical] R2-1: Cross-arm sentinel forgery: all four phases share ONE run directory with fixed, guessable file names (arm-a.rc, arm-b.rc), and nothing isolates the harness's sentinel channel from the observed code — code running inside arm a (the PR tree's own code, which this command exists to execute) can write arm b's sentinel and fabricate arm b's completed outcome, exit code, and therefore observed: true. Distinct from R2-14 (the run-dir symlink vector) and survives its mkdtempSync fix shape: the attacker is the driven script itself, which learns the dir at runtime (its own wrapper's $0, /tmp enumeration). Nothing removes stale .rc files: start() writes only the .sh, and the poll loop treats 'rc file exists' as the completed verdict.
A malicious PR's build/test code in arm a discovers the run dir and writes QWEN_REVIEW_DRIVE_DONE rc=0 into arm-b.rc. When arm b is driven against the base tree (where the malicious code is absent), the harness reports arm b completed with the attacker-chosen exit code, observed: true — the A/B witness is fabricated in whichever direction the PR wants. Variant: arm-a code unlinking arm-a.log between existsSync and readFileSync throws ENOENT out of runAbDrive, discarding the whole report.
Witness:
[probe] Real-tmux run: arm a forged arm-b.rc during its own window -> {"observed": true, "b": {"outcome": "completed", "exitCode": 0, "droveForMs": 0}} — arm b's verdict fabricated with zero drive time. With the unlink-at-start() fix shape: observed: false, b: timed-out.
Suggested fix: Give each phase its own mkdtempSync directory created immediately before its start(), and unlink stale .rc/.log before each start, so one arm's code cannot know — or pre-write — the other arm's sentinel paths; remove the per-phase dirs in the same finally.
中文说明
四个阶段共用同一个运行目录、文件名固定可猜（arm-a.rc、arm-b.rc），harness 的 sentinel 通道与被观察代码之间没有任何隔离——arm a 里运行的代码（PR 树自己的代码，正是本命令要执行的对象）可以写 arm b 的 sentinel，伪造其 completed 结局与退出码，从而伪造 observed: true。与 R2-14（运行目录符号链接向量）不同、且其 mkdtempSync 修复也无法关闭本项：攻击者就是被驱动脚本本身（从自身包装脚本的 $0 或枚举 /tmp 即可得知目录）。真实 tmux 探针：arm a 伪造 arm-b.rc → observed: true、arm b "零驱动时间完成"；start() 前清理 .rc/.log 后翻转为 observed: false。建议每阶段使用独立的 mkdtemp 目录、启动前清理陈旧 .rc/.log。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| .filter((l) => !FILE_LEVEL_METADATA_RE.test(l)); | ||
| if (file.renameFrom !== undefined) { |
There was a problem hiding this comment.
[Critical] R2-2: Copy-with-edits sections get their copy from/copy to lines stripped by FILE_LEVEL_METADATA_RE but never enter the path-rewrite branch (keyed solely on file.renameFrom, which parseDiff only sets from rename from), so the surviving diff --git a/old b/new / --- a/old / +++ b/new tokens make git apply -R execute a RENAME rewind — the exact 'different mutation than the report names' the comment block above the regex says this filtering exists to prevent.
A diff captured with copy detection active (plan-diff / arbitrary --diff entry points admit user-captured diffs; the pinned pipeline captures suppress copy detection — measured on git 2.43) yields a copy section; the verifier selects new:1; extractHunkPatch strips the copy lines but leaves --- a/old unrewritten; git apply -R --check passes and -R moves the file from new back to old while the report claims a content revert at new — a fabricated witness pair for the probe that runs next.
Witness:
[probe] Fixture via real `git diff -M --find-copies-harder`: patch retains old-path tokens ({'diff --git names old path': true, '--- names old path': true}); runRevertHunk -> applied: true, note 'reverted hunk new.txt:1 (@@ -1,4 +1,4 @@' while old.txt exists: true / new.txt exists: false — the file moved to the OLD path while the report names the new one.
Suggested fix: Key the rewrite on the header tokens disagreeing (the --- token, after its a/ prefix, differs from the +++ token, after b/) rather than on renameFrom — covering renames and copies alike; alternatively detect copy from in the raw header and return an explicit 'copy sections are not independently revertible' note instead of mutating.
中文说明
带编辑的 copy 段：copy from/copy to 被 FILE_LEVEL_METADATA_RE 剥掉，但不进入路径改写分支（只键于 renameFrom），存活的 diff --git a/old b/new / --- a/old 记号让 git apply -R 执行改名回退。真实 git copy 检测夹具（git diff -M --find-copies-harder）端到端复现：报告显示 "reverted hunk new.txt:1"，文件却回到了旧路径（old.txt 存在、new.txt 消失）。可达性：管线固定捕获参数抑制 copy 检测（git 2.43 实测），但 --diff 接受任意预捕获 diff（plan-diff 入口），且正则本身就预期了 copy 行。建议把改写键改为"头记号不一致"（--- 与 +++ 不同），或直接拒绝 copy 段。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Liveness is read BEFORE the per-arm teardown, so "alive at end" means | ||
| // "outlived the arm", not "survived our own kill". | ||
| const sharedAliveAtEnd = sharedRc === null ? null : !existsSync(sharedRc); |
There was a problem hiding this comment.
[Critical] R2-3: The shared-process liveness probe inverts a one-directional sentinel: 'sentinel present => exited' is sound, but the code reads 'sentinel absent => alive'. The wrapper's EXIT-trap sentinel is never written when the wrapper is SIGKILLed or when the shared script exec's its daemon (exec discards the trap) — so a dead upstream reports sharedAliveAtEnd: true and the observed gate silently loses its shared-holds conjunct. The once-mode pre-arm-b check keys on the same inverted signal.
--shared 'exec node server.js' (an idiomatic daemon shape within the option's documented contract) crashes or exits mid-arm: no sentinel ever appears, so the report says observed: true with 'the shared process outlived each arm' while both arms' captures were taken against a dead upstream — exactly the harness-fabricated evidence the observed gate exists to block; in once mode arm b is driven against the corpse.
Witness:
[probe] --shared 'true' -> observed: false, sharedAliveAtEnd: false; --shared 'exec true' (identical instant death) -> observed: true, sharedAliveAtEnd: true on both arms, note 'the shared process outlived each arm'. On-host: SIGKILL of a trap-wrapped shell -> sentinel ABSENT. Fix probe (tmux has-session liveness): the exec case flips to observed: false, control unchanged.
Suggested fix: Liveness = session exists AND no sentinel: probe tmux has-session -t shared-${arm} alongside the existsSync(sharedRc) test at both read sites (and the once-mode pre-arm-b check); treat either signal as 'exited'.
中文说明
共享进程活性探测反转了单向 sentinel："有 sentinel ⇒ 已退出"成立，代码却读成"无 sentinel ⇒ 活着"。包装脚本的 EXIT trap 在 SIGKILL 与 exec 形态（exec 丢弃 trap）下都不会写 sentinel，死掉的上游因此报 sharedAliveAtEnd: true，observed 门悄悄丢掉"shared 存活"合取项。探针：--shared 'true' → observed: false、sharedAliveAtEnd: false；--shared 'exec true'（同样的立即死亡）→ observed: true、sharedAliveAtEnd: true、注记 "outlived each arm"——两臂捕获其实都对着死上游。once 模式的 arm-b 预检键于同一反转信号。tmux has-session 修复探针可翻转。建议活性 = 会话存在 且 无 sentinel，两处读取点与 once 预检都要改。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| } catch (err) { | ||
| writeStderrLine((err as Error).message); | ||
| process.exitCode = 1; |
There was a problem hiding this comment.
[Critical] R2-4: The handler collapses the TypeError thrown by assertWritableOutPath (deliberately called pre-drive to classify an unusable --out) into exit 1 — the runtime-failure / not-observed class — instead of exit 2, the documented 'repairable invocation' class. Five sibling commands call the same helper and map TypeError -> 2 (revert-hunk.ts:382 — this PR's own sibling — scratch-tree, fetch-diff, issue-context, comment-body; meta and plan-diff document the same convention); lib/paths.ts's docstring exists precisely so such errors do not exit as a runtime failure.
A verifier invokes ab-drive with an unusable --out (existing directory, trailing-separator path, empty variable); the handler exits 1 with no report; a caller scripting the family contract reads exit 1 as 'a run happened and was not observed / a runtime failure to retry' and retries the identical broken invocation or records an A/B evidence gap, while the same mis-invocation of revert-hunk returns 2 and gets the invocation repaired. The two commands this PR adds disagree on the exit class of the same error.
Witness:
[probe] Handler driven with --out = existing directory: ab-drive exitCode: 1 / revert-hunk exitCode: 2 for the identical broken invocation shape; patching process.exitCode = err instanceof TypeError ? 2 : 1 flips ab-drive to 2.
Suggested fix: process.exitCode = err instanceof TypeError ? 2 : 1; in the handler's outer catch, matching revert-hunk.ts:382 and the six other callers.
中文说明
handler 把 assertWritableOutPath（驱动前故意调用以分类不可用 --out）抛出的 TypeError 归入退出码 1（运行时失败/未观察类），而不是 2（"可修复的调用"类）——包括本 PR 自己的 revert-hunk 在内，五个调用同一帮助函数的兄弟命令都把 TypeError 映射为 2（meta 与 plan-diff 也文档化了同一约定）；lib/paths.ts 的文档字符串正是为了这类错误不以运行时失败退出而存在。探针：同一坏调用（--out = 已存在目录），ab-drive 退出 1、revert-hunk 退出 2；按契约脚本调用的调用方会重试这个注定失败的调用。打成 err instanceof TypeError ? 2 : 1 后翻转。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const apply = exec(tree, ['apply', '-R', patchPath]); | ||
| if (apply.status !== 0) { |
There was a problem hiding this comment.
[Critical] R2-5: The apply branch folds spawn-level error/signal into the same bucket as a non-zero exit, recording a harness kill or spawn failure as 'the tree changed between the two calls' — the misclassification the sibling --check branch three statements above correctly guards (check.error !== undefined || check.signal !== undefined) and the GitApplyResult doc comment explicitly warns against. The exec-seam race test only drives {status: 1}, so the shape is unguarded by any test.
--check passes, then the real git apply -R is killed by the 60s timeout the same function installs (slow/network fs, lock stall) or spawn fails (fd exhaustion): spawnSync returns {status: null, signal/error set}; status !== 0 takes the race branch and the report's conflict field — evidence rendered verbatim to the verifier — carries a fabricated tree-race fact; a SIGTERM mid-write can also leave the target file torn, contradicting the conflict invariant 'the tree is UNCHANGED'.
Witness:
[probe] Via the real exec seam: both {status: null, signal: 'SIGTERM'} and {status: null, error: 'EMFILE'} after --check passed -> pristine records conflict 'git apply refused (no error text)' + note 'the tree changed between the two calls' (a race that did not happen); adding the same guard on the apply branch reports 'could not run git apply … a harness failure, not a fact about the hunk' with conflict unset.
Suggested fix: Mirror the check-branch guard before the status test: if (apply.error !== undefined || apply.signal !== undefined) return an applied: false harness-failure note (stating the tree may be partially modified, so reset the scratch tree), and drive that shape through the existing seam test.
中文说明
apply 分支把 spawn 层 error/signal 与状态码非零混为一谈，把 harness 被杀/spawn 失败记成"两次调用之间树发生了变化"——三条语句之上的 --check 分支已正确守护同类错误（check.error !== undefined || check.signal !== undefined），GitApplyResult 注释明确警告过这种误分类。探针经真实 exec seam 复现：--check 通过后，{status: null, signal: SIGTERM} 与 {status: null, error: EMFILE} 都得到伪造的"树竞态"注记；给 apply 结果补上同款守护后翻转为 harness 失败报告。套件只驱动过 {status: 1}。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // mutated tree as untouched. | ||
| writeStdoutLine(text); |
There was a problem hiding this comment.
[Suggestion] R2-32: The only throwing write on the success path sits between the tree mutation and the --out write: writeStdoutLine(text) after a successful reverse-apply can deliver EPIPE asynchronously when the reader is gone (stdioHelpers documents the EPIPE shape); the unhandled 'error' event crashes the process with exit 1 — the refusal/not-applied class — for a tree that was in fact mutated. The design comment directly above guards this exact harm for --out but not for the stdout write that precedes it; drive.ts/ab-drive.ts share the throwing write but mutate no tree before it. (Verification correction: EPIPE is async, so the handler's synchronous flow completes and the --out file IS written — the misreading rides entirely on the exit code.)
The verifier's script pipes the command and the reader dies before the report is written (killed/crashed reader, | true shapes): git apply -R already succeeded (tree mutated, exitCode still 0); the EPIPE crash exits 1, and the calling script — which branches on exit status per the command's own contract — concludes the tree is pristine, never resets it, and runs the intact-tree probe against a tree that is in fact reverted: the witness pair measures the wrong tree.
Witness:
[probe] End-to-end through the real handler with --out set: EPIPE arm (| true): tree first line top-old (mutated), out-exists=true, unhandled-error crash at writeStdoutLine <- handler, node-exit=1. Control arm (> /dev/null): identical observations, node-exit=0. Deterministic 1/0 split on the unmodified commit.
Suggested fix: Use writeStdoutLineSafe(text) for the report line — the exit code already carries applied's semantics per the adjacent comment — so a dead stdout cannot flip the exit class and send the caller to probe a mutated tree as pristine.
中文说明
成功路径上唯一会抛的写入位于"树已变异"与 --out 写入之间：反向 apply 成功后 writeStdoutLine(text) 在读者已离开时（stdioHelpers 文档记载的 EPIPE 形状）异步投递错误，未处理的 'error' 事件使进程以退出 1（拒绝/未应用类）崩溃——而树其实已被变异。上方注释为 --out 防了同款危害、却没防它前面的 stdout 写入；drive.ts/ab-drive.ts 同样有该写入但之前不变异树。（核查更正：EPIPE 异步送达，同步流程走完、--out 文件确实写出——误读完全骑在退出码上。）端到端探针：EPIPE 臂（| true）树已变异、退出 1；对照臂（> /dev/null）同样观测、退出 0。建议报告行改用 writeStdoutLineSafe(text)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const args = baseArgs({ ready: 'ARMPROBE' }); | ||
| const h = harness({ | ||
| server: args.server, | ||
| failProbes: [{ marker: 'ARMPROBE', arm: 'a' }], |
There was a problem hiding this comment.
[Suggestion] R2-33: The arm readiness gate is never negatively exercised on arm b — arm: 'b' does not appear anywhere in the suite's failProbes, and readyAfterMs is asserted nowhere — so arm b's pollReady branch in runArm has no negative pinning, despite the file header's claim that asymmetric fixtures exist for exactly this shape. (Filed low-confidence in round 5; the mutant run settled the survival claim.)
A mutant that skips readiness polling when arm === 'b' survives 22/22. In a real run where arm b's tree never becomes ready within --ready-timeout (e.g., the artifact the probe waits for only exists in the PR arm), arm b is driven anyway, its script can still reach its sentinel, bothCompleted goes true and observed: true licenses a comparison in which arm b measured a not-ready tree — the harness-made confound --ready exists to prevent. Distinct from R2-23 above: there the missing arm-b fixture lets a bothCompleted operand drop; here arm b can complete, so the gate that must fail is the readiness poll itself.
Witness:
[probe] Mutant if (args.ready && arm !== 'b') (skip arm-b readiness polling entirely): survived 22/22.
Suggested fix: Add the mirrored fixture: failProbes: [{ marker: 'ARMPROBE', arm: 'b' }], asserting r.b?.outcome 'not-ready', r.a?.outcome 'completed', r.observed false, and h.events() contains no 'new:arm-b'.
中文说明
臂就绪门从未在 arm b 上做负向测试——failProbes 中没有 arm: 'b'、readyAfterMs 无断言——runArm 里 arm b 的 pollReady 分支没有负向钉住。跳过 arm-b 就绪轮询的变异（if (args.ready && arm !== 'b')）22/22 全绿；真实运行中 arm b 的树未在 --ready-timeout 内就绪时（探针等待的产物只在 PR 臂存在），arm b 仍会被驱动、脚本仍可到 sentinel，observed: true 背书对未就绪树的比较——正是 --ready 存在要防的 harness 伪造。与上面的 R2-23 不同：那里缺的夹具让 bothCompleted 判据漏掉；这里 arm b 可以完成、必须失败的是就绪轮询本身。建议补镜像夹具。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| writeFileSync(join(armA, 'marker.txt'), 'CONTENT-OF-A\n'); | ||
| writeFileSync(join(armB, 'marker.txt'), 'CONTENT-OF-B\n'); | ||
| const r = runAbDrive({ | ||
| script: 'cat marker.txt; echo "arm=$AB_ARM root=$AB_ARM_ROOT"', |
There was a problem hiding this comment.
[Suggestion] R2-34: The AB_ARM_ROOT channel — one of the command's two documented 'ONLY variation between the arms' (ab-drive.ts:140, agent-briefs.ts) — is pinned only for arm a (expect(r.a?.output).toContain(armA), the sole root assertion in the suite); arm b's root is asserted nowhere, and the harness's fake exec never inspects the arm script body (it regexes only AB_ARM out of probe strings), so the whole harness suite is blind to that env value.
Any real verification script that derives paths from $AB_ARM_ROOT then reads arm a's tree inside arm b; arm b's capture becomes arm a's behavior, and the report licenses a fabricated comparison (observed: true) with nothing in the suite or the report contradicting it. The cwd channel does not save this: the contract explicitly exports the root as env for scripts that use it (the real test's own script echoes it).
Witness:
[probe] Mutant envPrefix(arm, arm === 'b' ? resolve(args.armA) : root) at the arm-start call site (hands arm b arm a's AB_ARM_ROOT while cwd stays correct): 22/22 green, including the real-tmux test whose arm-b assertions (arm=b, CONTENT-OF-B via cwd) all still pass while $AB_ARM_ROOT silently carries arm a's tree.
Suggested fix: Add the b-side mirror assertion to the real test — expect(r.b?.output).toContain(armB); (and optionally expect(r.b?.exitCode).toBe(0);) — so a root transposition on either arm fails the suite.
中文说明
AB_ARM_ROOT——文档声明的两臂间"唯一变量"之一——只对 arm a 钉住（toContain(armA) 是套件唯一的根断言）；arm b 的根无断言，且 harness 的假 exec 从不检查臂脚本体（只从探言行正则提取 AB_ARM），整个 harness 套件对该环境值失明。把 arm b 的 AB_ARM_ROOT 换成 arm a 根的变异 22/22 全绿（含真 tmux 测试——其 arm-b 断言走 cwd 通道仍然通过）。依赖 $AB_ARM_ROOT 取路径的验证脚本会在 arm b 里读到 arm a 的树，报告背书伪造的比较而无处可查。建议补 b 侧镜像断言。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| afterAll(() => { | ||
| for (const d of tmpDirs) rmSync(d, { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R2-35: This round added the command's own temp patch-dir lifecycle (mkdtempSync + finally rmSync(dir, …) in revert-hunk.ts) but no test pins it: the suite scrupulously cleans up its own fixture repos yet never asserts the command removed its qwen-review-revert-hunk-* directory after a revert — grep confirms the prefix appears nowhere in the file.
A mutant that deletes the finally rmSync block — or moves it onto the applied-only path so refused runs leak — survives all 16 tests, because nothing inspects tmpdir() after any call. Every production revert-hunk invocation then leaks one temp directory holding the extracted patch; verification selects hunks one at a time (dozens per PR) across repeated rounds on shared CI runners, where the dirs persist until reboot — the exact leak class the suite's own cleanup comment and the confirmed ab-drive teardown finding (R2-25) treat as worth pinning.
Witness:
[probe] Mutant removing the finally rmSync: 16/16 green and left 12 leaked qwen-review-revert-hunk-* directories in /tmp during the run, each holding the extracted hunk.patch — the leak observed directly, not inferred.
Suggested fix: Add a test that lists qwen-review-revert-hunk-* entries in tmpdir() before and after runRevertHunk, on BOTH the applied and the refused path, and asserts no new entries remain.
中文说明
本轮新增了命令自身的临时补丁目录生命周期（mkdtempSync + finally rmSync），但无测试钉住：套件仔细清理自己的夹具仓库、却从不断言命令删除了 qwen-review-revert-hunk-* 目录——该前缀在文件中零出现。删掉 finally rmSync（或移到仅 applied 路径、让拒绝运行泄漏）的变异 16/16 全绿；探针运行时直接观察到 12 个泄漏目录、各含 hunk.patch。验证按 hunk 逐个选择（每 PR 数十次）、跨轮重复，共享 CI 上持续累积。建议补 applied 与 refused 两路的前后列表断言。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| diff: diffPath, | ||
| list: true, | ||
| }); | ||
| const printed = vi.mocked(writeStdoutLine).mock.calls.at(-1)?.[0] as string; |
There was a problem hiding this comment.
[Suggestion] R2-36: The --list wiring test asserts only the stdout JSON and never asserts process.exitCode, so the enumeration path's exit-code contract — the very contract test 3 names as 'the branch a calling script takes' — is pinned by nothing. Mirror image of R2-31 above: there stdout is unpinned where exit codes are pinned; here the exit code is unpinned where stdout is pinned.
A future edit that leaves or sets a non-zero process.exitCode on the --list success branch keeps the entire suite green: the test reads only the mocked writeStdoutLine output, and tests 2-3 each pre-set process.exitCode = 0 before invoking the handler, absorbing any leak. A verifier script that runs qwen review revert-hunk --diff d --list to discover hunk ids then sees exit 1 beside a valid enumeration, treats enumeration as failed, and aborts hunk selection.
Witness:
[probe] Mutant setting process.exitCode = 1 on the --list success branch: 16/16 green.
Suggested fix: In the --list test set process.exitCode = 0 before the handler call, assert expect(process.exitCode).toBe(0) after the JSON assertions, and reset it to 0 at the end — mirroring the discipline of tests 2 and 3.
中文说明
--list 布线测试只断言 stdout JSON、从不断言 process.exitCode——枚举路径的退出码契约（测试 3 称之为"脚本调用方会走的分支"）无钉住，与上面的 R2-31 互为镜像。在 --list 成功分支置退出 1 的变异 16/16 全绿（本测试只读 mock 的 stdout；测试 2/3 预设 exitCode 0 吸收了泄漏）；枚举 hunk id 的验证脚本会在有效枚举旁看到退出 1、把枚举当失败而中止选择。建议按测试 2/3 的纪律补断言。
— qwen3.8-max via Qwen Code /review (v0.22.0)
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:
- R3-7 location 3: --hunk selector exit class (revert-hunk.ts:411) — already reported (comment 3837030394, by qwen-code-ci-bot) at revert-hunk.ts:411
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the unit suites ran locally on Linux only.
Not explored to full depth (tool budget reached): chunk 1: executing ab-drive.test.ts under vitest — the review worktree has no node_modules ( Cannot find package 'vitest' ), and a full npm ci (which also triggers….
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/ab-drive.ts:717 — [review] stdout-first guarantee's --out mid-write failure branch untested in both commandspackages/cli/src/commands/review/ab-drive.test.ts:582 — [review] post-sentinel log re-read unpinned and unpinnable through the harness as writtenpackages/cli/src/commands/review/ab-drive.test.ts:617 — [review] report field value mode: 'no-shared' asserted nowherepackages/cli/src/commands/review/ab-drive.ts:625 — [review] empty --script passes demandOption and licenses a vacuous observed: true verdictpackages/cli/src/commands/review/revert-hunk.test.ts:677 — [review] --list-alone path never parsed through the real yargs builderpackages/cli/src/commands/review/ab-drive.test.ts:320 — [review] success note's arm-b exit-code half unpinned (fabricated-zero class)packages/cli/src/commands/review/revert-hunk.test.ts:562 — [review] usage guard's missing---hunk direction untested (crash-class message on hunk-less invocation)packages/cli/src/commands/review/revert-hunk.test.ts:181 — [review] coupling-refusal note text unpinned; cross-wiring with the race branch ships green
Convergence: round 3 posted 26 inline comment(s), 26 of them reported for the first time; the previous round posted 37 (36 new). Findings keep coming back to the same files: packages/cli/src/commands/review/ab-drive.test.ts (findings in round 2; 9 more now); packages/cli/src/commands/review/ab-drive.ts (findings in rounds 1, 2; 7 more now); packages/cli/src/commands/review/revert-hunk.test.ts (findings in round 2; 4 more now), and 2 more file(s). 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.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the unit suites ran locally on Linux only。
未探索到全部深度（达到工具调用预算）：chunk 1：executing ab-drive.test.ts under vitest — the review worktree has no node_modules ( Cannot find package 'vitest' ), and a full npm ci (which also triggers…。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 26 条行内评论，其中 26 条是首次提出；上一轮发布了 37 条（其中 36 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/ab-drive.test.ts（第 2 轮已出过发现，本轮又有 9 条）；packages/cli/src/commands/review/ab-drive.ts（第 1、2 轮已出过发现，本轮又有 7 条）；packages/cli/src/commands/review/revert-hunk.test.ts（第 2 轮已出过发现，本轮又有 4 条），另有 2 个文件。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| output = readIfThere(a.logPath); | ||
| const rcText = readIfThere(a.rcPath); | ||
| exitCode = rcText === '' ? null : sentinelExitCode(rcText); |
There was a problem hiding this comment.
[Critical] R3-1: The harness's evidence channels are path-addressed phase files the driven code can reach, so untrusted PR code can forge the very captures observed: true licenses. Two demonstrated entrances: (1) a process that survives arm a's teardown via setsid (escaping kill-session) discovers arm b's freshly created phase dir from the run dir — known to the script via its own wrapper $0 — and plants arm-b.rc, fabricating arm b's completion and exit code with zero drive time; (2) code running INSIDE an arm's own window needs no survivor at all: the wrapper's bash <script> > <logPath> puts the log path on every child's fd 1 (readlink /proc/$$/fd/1), so the arm's own build/test overwrites the capture mid-drive — per-phase mkdtemp does not help, fd inheritance defeats path secrecy. The poll loop re-reads both files BY PATH every cycle, so the final capture and the verdict are attacker-chosen bytes.
Witness:
[probe] real tmux, this commit.
Survivor: arm a's setsid'd watcher plants arm-b.rc -> observed: true,
b.exitCode 77, b.droveForMs 0 (arm b never driven);
control (no watcher): b.exitCode 3, output 'real-arm-b-ran'.
In-window: arm process discovers the log via /proc/$$/fd/1, overwrites it,
forges the sentinel -> observed: true, b.output = planted bytes,
genuine output erased.
Gating the rc read on session death closes only (1). Pin each phase's files by handle — open with O_NOFOLLOW after new-session and read via the fd (or compare lstat dev/ino against the creation-time stat on every read, bailing with a tamper note on mismatch) — for the log, the rc sentinel, and the script path alike.
中文说明
harness 的证据通道是被驱动代码可触及的按路径寻址文件：不可信的 PR 代码可以伪造 observed: true 所背书的捕获本身。两条实弹入口：(1) 通过 setsid 逃过 kill-session 的幸存进程从运行目录（脚本经自身包装脚本 $0 即可得知）发现 arm b 新建的阶段目录并写入 arm-b.rc，以零驱动时间伪造 arm b 的完成与退出码；(2) 在臂自身窗口内运行的代码无需幸存：包装脚本的 bash <script> > <logPath> 让日志路径出现在每个子进程的 fd 1 上（readlink /proc/$$/fd/1），臂内的构建/测试可在驱动中途覆写捕获——逐阶段 mkdtemp 防不住这一点（fd 继承使路径保密失效）。轮询循环每轮都按路径重读两个文件，最终捕获与裁决因而是攻击者选定的字节。探针：真实 tmux 下两路均复现（幸存进程伪造 → observed: true、b 零驱动时间；窗口内覆写 → 真输出被擦除）。把 rc 读取门控在会话死亡上只关闭入口 (1)；请按句柄钉住各阶段文件（O_NOFOLLOW 打开后经 fd 读取，或每次读取比对 lstat dev/ino，不一致即以篡改注记退出）——日志、rc sentinel 与脚本路径都需要同样的钉法。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Classify an unusable --out BEFORE the drives: both arms can take | ||
| // minutes, and an EISDIR discovered after them throws the whole run's | ||
| // evidence away. | ||
| if (a.out) assertWritableOutPath(a.out); |
There was a problem hiding this comment.
[Critical] R3-2: The truthiness guard skips the --out fail-fast classification for an exactly-empty value: --out '' (or --out "$REPORT" with the variable unset) is falsy, so assertWritableOutPath never runs, both arms drive for up to 2×--timeout, the post-drive write guard also skips, and the process exits 0/1 with no report file and no diagnostic — instead of the documented repairable class exit 2. Every sibling caller gates on !== undefined (revert-hunk.ts:378, scratch-tree, fetch-diff, comment-body, issue-context), and the helper's own JSDoc names the empty path as its first usage-error case — the two commands this PR adds disagree on the exit class of the same broken invocation.
Witness:
[probe] same input out: '':
AB-DRIVE exitCode 0 (drove both arms, no report file, no diagnostic)
REVERT-HUNK exitCode 2 (sibling `!== undefined` classifies before acting)
| if (a.out) assertWritableOutPath(a.out); | |
| if (a.out !== undefined) assertWritableOutPath(a.out); |
The write-side guard may stay truthiness-based — exactly scratch-tree's pairing.
中文说明
真值守卫让恰好为空的 --out 绕过了失败前置分类：--out ''（或 --out "$REPORT" 而变量未设置）是 falsy，assertWritableOutPath 不会执行，两臂照常各驱动最长 --timeout，驱动后的写守卫同样跳过，进程以 0/1 退出、没有报告文件、没有任何诊断——而不是文档约定的可修复类退出码 2。其余五个调用方全部用 !== undefined 守卫（revert-hunk.ts:378、scratch-tree、fetch-diff、comment-body、issue-context），帮助函数的 JSDoc 也把空路径列为第一种用法错误——本 PR 新增的两个命令对同一种坏调用给出不同的退出类别。探针：同一输入 out: ''，ab-drive 退出 0（驱动了双臂），revert-hunk 退出 2。建议改为 !== undefined 守卫（写入侧可保留真值守卫，与 scratch-tree 的搭配一致）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const expectedRoot = arm === 'a' ? realA : realB; | ||
| if (realpathOf(root) !== expectedRoot) { |
There was a problem hiding this comment.
[Critical] R3-3: The arm-root re-check runs at the top of runArm, but the root is not consumed until start()'s tmux new-session -c root ~60 lines later — after the arm's OWN shared upstream is started and polled (--shared-ready up to 60 s, then --ready-timeout). That shared daemon is untrusted PR code running as the same user, and it gets a pre-drive window to delete the arm root (tmux then silently falls back to $HOME) or replace it with a symlink to an attacker tree; the default per-arm sharedCwd = root is never re-checked either. Arm b's script then completes normally and the report says root: <original> with observed: true — a verdict where arm b never ran in its claimed tree.
Witness:
[probe] real tmux: shared daemon re-links arm b's root during the ready window ->
PR: observed: true, b_root claims the original path, b_output 'TARGET-MARKER'
FIX (re-check duplicated immediately before start()): observed: false,
note 'arm b's root ... changed before the drive started — harness fact.'
Re-validate the arm root (and the default sharedCwd = root) immediately before the start(\arm-${arm}`, root, ...)` call, comparing against the pinned realpath and bailing with a named confound note when it changed.
中文说明
arm 根的复检发生在 runArm 顶部，但根直到约 60 行之后 start() 的 tmux new-session -c root 才被消费——而在这之前，该臂自己的共享上游已经启动并经历了就绪轮询（--shared-ready 最长 60 秒，再叠加 --ready-timeout）。该共享守护进程是与审查者同用户运行的不可信 PR 代码，拥有一个驱动前窗口：删除臂根（tmux 会静默回退到 $HOME）或替换为指向攻击者树的符号链接；默认的每臂 sharedCwd = root 同样从未复检。arm b 的脚本随后正常完成，报告仍写 root: <原路径> 且 observed: true——一份 arm b 从未在其声称的树里运行过的裁决。探针：真实 tmux 下共享守护进程在就绪窗口内重链接 arm b 的根 → 成品代码 observed: true、b 输出为 attack 树内容；在 start() 前复检后翻转为 observed: false。请把臂根（及默认 sharedCwd = root）在 start(\arm-${arm}`, ...)` 调用前立即复检，与钉住的 realpath 比对，不一致即以点名原因的注记退出。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // silent drop to terminal-only — and it is not the unexecuted claim | ||
| // this rule exists to stop, because the measurement that moved it IS a | ||
| // run's output, riding the finding as `heldByMeasurement`. | ||
| f.heldByMeasurement !== undefined || |
There was a problem hiding this comment.
[Critical] R3-4: The new heldByMeasurement exemption is broader than its rationale. The comment justifies exempting the Suggestion the test-delta hold JUST demoted, but the condition exempts ANY finding carrying the marker — including a Critical re-raised under the measurement note's own 'file it at Critical again' door — which skips the witness rule entirely and posts at high confidence with no executed evidence.
Failure shape: round N posts a Critical held to Suggestion with heldByMeasurement; round N+1 re-raises it to Critical (the deliberate act findings.ts:554 describes); the marker round-trips through --input; holdUnwitnessedFindings exempts it with witness === undefined; isPostable projects it into the resolver input and it posts as a blocking Critical — an unexecuted claim, exactly what the rule exists to stop (pre-diff holdUnwitnessedCriticals would have demoted it).
Witness:
[probe] real holdCriticalsFailingOnBase + holdUnwitnessedFindings:
re-raised Critical + marker + no witness ->
{severity:'Critical', confidence:'high', unwitnessed:[]} (exempt, postable)
control without marker -> {confidence:'low', unwitnessed:[id]}
| f.heldByMeasurement !== undefined || | |
| (f.heldByMeasurement !== undefined && f.severity === 'Suggestion') || |
中文说明
新的 heldByMeasurement 豁免比其理由更宽：注释只为"刚被 test-delta 降级成的那个 Suggestion"辩护，但条件豁免了任何携带该标记的发现——包括按测量注记自己那扇"可以重新按 Critical 提交"的门被重新提升为 Critical 的条目——使其完全绕过 witness 规则、以高置信度带着零执行证据发布。场景：第 N 轮发布了被降为 Suggestion 的 Critical（带 heldByMeasurement）；第 N+1 轮把它重新提升为 Critical（findings.ts:554 描述的刻意行为）；标记经 --input 往返；holdUnwitnessedFindings 在 witness === undefined 时豁免它；isPostable 把它投影进锚点输入并作为阻断性 Critical 发布——一条未经执行的断言，正是本规则要阻止的（改动前的 holdUnwitnessedCriticals 会将其降级）。探针证实标记是豁免的唯一原因。建议把豁免收窄为 && f.severity === 'Suggestion'，让重新提升的 Critical 仍需 witness（或 not run — <理由> 行）才能发布。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const oldPath = stripSide(aTokOld); | ||
| const newPath = stripSide(bTok); | ||
| if (oldPath !== null && newPath !== null && oldPath !== newPath) { |
There was a problem hiding this comment.
[Critical] R3-5: FILE_LEVEL_METADATA_RE strips rename/copy lines unconditionally, but the compensating ---/diff --git token rewrite only fires when both tokens carry a//b/ (or quoted) prefixes. For a rename-with-edits diff captured with --src-prefix=src/ --dst-prefix=dst/, stripSide returns null for src/old.txt / dst/new.txt, so the rewrite is skipped while the rename metadata is already gone — git apply -R --check passes and the apply moves the file back to its OLD path while the report says applied: true, 'reverted hunk dst/new.txt:1': a mutation different from the one the report names, certified as applied, and the next intact/reverted witness pair measures the wrong mutation. For no-prefix diffs, git's hardcoded -p1 refusal is recorded as 'a coupling worth reporting as a fact' — a prefix problem posing as a coupling fact. The command explicitly accepts arbitrary --diff input.
Witness:
[probe] custom-prefix rename fixture through the unmodified command:
report: applied: true, 'reverted hunk dst/new.txt:1'
TREE BEFORE [.git, custom.diff, new.txt] ->
TREE AFTER [.git, custom.diff, old.txt] (rename rewound)
FIX (prefix-independent rewrite + unsupported-prefix refusal):
content-only revert, new.txt kept, 25/25 tests green
Key the rewrite on the parsed, prefix-independent paths (parseDiff already resolves both sides — generalize for copy from/copy to) so it fires whenever the sides genuinely disagree; when a token has no slash-delimited prefix at all, refuse early with an explicit 'unsupported diff prefix' note instead of recording git's refusal as a coupling fact.
中文说明
FILE_LEVEL_METADATA_RE 无条件剥掉 rename/copy 行，但补偿性的 ---/diff --git 记号改写只在两个记号都带 a//b/（或引号）前缀时才触发。对以 --src-prefix=src/ --dst-prefix=dst/ 捕获的带编辑改名段，stripSide 对 src/old.txt/dst/new.txt 返回 null，改写被跳过而改名元数据已被剥掉——git apply -R --check 通过，apply 把文件移回旧路径，报告却写 applied: true, 'reverted hunk dst/new.txt:1'：一个与报告命名不同的变异被认证为已应用，下一组 intact/reverted 证对将测量错误的变异。对无前缀 diff，git 硬编码的 -p1 拒绝被记录成"值得作为事实报告的耦合"——前缀问题冒充耦合事实。本命令明确接受任意 --diff 输入。探针：自定义前缀改名夹具端到端复现文件回退；改为前缀无关的改写 + 显式拒绝无前缀输入后翻转为纯内容还原且 25/25 测试全绿。建议把改写键改为解析出的前缀无关路径（parseDiff 已解析两侧，推广到 copy from/copy to）；对完全没有斜杠前缀的记号提前以"不支持的 diff 前缀"注记拒绝，而不是把 git 的拒绝记成耦合事实。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(r.a?.outcome).toBe('overflowed'); | ||
| expect(r.a?.exitCode).toBeNull(); | ||
| expect(r.observed).toBe(false); |
There was a problem hiding this comment.
[Suggestion] R3-19: The overflowed test pins neither of the two post-overflow behaviors its sibling tests pin for every other early-stop outcome — that the overflowed arm session is killed, and that arm b is still driven. Two mutants ship green (verified): (1) gating the post-loop kill-session off for overflowed — the session survives into arm b's whole window, log growing unbounded past LOG_MAX_BYTES (the disk-filling mode the cap exists to stop) and contending for the same ports/files as arm b's run of the same script; (2) skipping runArm('b') after an overflow — r.b null, observed false, all three assertions pass, losing arm b's capture, the repair pointer the not-observed note sends the verifier to. Every other early-stop outcome pins 'the other arm still runs'; only overflowed does not.
Add expect(r.b?.outcome).toBe('completed'); and event-order assertions mirroring the timed-out test: ev contains kill:arm-a, and indexOf('kill:arm-a') < indexOf('new:arm-b').
Witness:
[probe] each mutant shipped 35/35 green:
kill-session gated off for overflowed; runArm('b') skipped after overflow.
中文说明
overflowed 测试没有钉住其余每种早停结局都钉住的两个溢出后行为——溢出的臂会话被杀、且 arm b 仍被驱动。两个突变体全绿（实测）：(1) 把循环后的 kill-session 对 overflowed 关掉——会话存活穿过 arm b 的整个窗口，日志越过 LOG_MAX_BYTES 无界增长（上限存在就是要防的写满磁盘模式），并与同一脚本的 arm b 运行争抢相同端口/文件；(2) 溢出后跳过 runArm('b')——r.b 为 null、observed 为 false、三条断言全过，丢掉 arm b 的捕获（未观察注记指引验证者去看的修复指针）。其他每种早停结局都钉了"另一臂继续运行"，唯独 overflowed 没有。请补 expect(r.b?.outcome).toBe('completed'); 及仿照超时测试的事件顺序断言。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const before = countStaging(); | ||
| const { dir, diffPath } = twoHunkFixture(); | ||
| runRevertHunk({ diff: diffPath, tree: dir, hunk: 'f.txt:1' }); | ||
| runRevertHunk({ diff: diffPath, tree: dir, hunk: 'f.txt:1' }); // refused | ||
| expect(countStaging()).toBe(before); |
There was a problem hiding this comment.
[Suggestion] R3-20: The staging-cleanup test pins the finally rmSync only on the applied and refused outcomes; the three failure returns inside the try (check-harness-failure, apply-harness-failure, check-passed-but-apply-failed) never count staging dirs, so replacing the unconditional finally with per-branch cleanup on the two covered branches ships green AND leaks: the mutant run left 4 orphaned qwen-review-revert-hunk-* directories in /tmp (one per failure-path invocation in the suite) — the #9207 residue class. In production each mistyped --tree (no handler pre-guard — it reaches runRevertHunk, mkdtemps, writes hunk.patch, returns the harness note without cleanup) or git killed by the 60 s hang guard orphans another directory while the suite stays green.
Reuse the existing failure invocations with the count: after the mistyped-tree call and one exec-seam kill call, assert countStaging() is unchanged.
Witness:
[probe] per-branch-cleanup mutant: 27/27 green AND 4 orphaned staging dirs
observed; the suggested assertions fail against it (expected 6 to be 4).
中文说明
暂存目录清理测试只在应用成功与拒绝两种结局上钉住 finally rmSync；try 内的三种失败返回（check-harness 失败、apply-harness 失败、check 通过但 apply 失败）从不清点暂存目录，因此把无条件 finally 换成两个已覆盖分支上的按分支清理将全绿且泄漏：突变体运行在 /tmp 留下 4 个孤儿 qwen-review-revert-hunk-* 目录（套件中每个失败路径调用一个）——#9207 残留类。生产中每次打错 --tree（handler 无前置守卫——会进入 runRevertHunk，mkdtemp、写 hunk.patch、返回 harness 注记但不清理）或被 60 秒悬挂守卫杀掉的 git 都会孤儿化一个目录，而套件全绿。请复用现有失败调用做清点：打错 --tree 的调用与一次 exec-seam kill 调用之后断言 countStaging() 不变。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(process.exitCode).toBe(2); | ||
| expect(readFileSync(join(fresh.dir, 'f.txt'), 'utf8')).toBe(before); | ||
| process.exitCode = 0; |
There was a problem hiding this comment.
[Suggestion] R3-21: The EISDIR --out case pins the exit class (2) and the non-mutation, but the crash-class stderr diagnostic is pinned nowhere: the file's only two writeStderrLineSafe assertions both target graceful-branch messages, so deleting the writeStderrLineSafe call in the handler's outer catch ships green (mutant verified 27/27) and silences every crash-class refusal — --out /some/existing/dir then exits 2 with nothing on stdout and nothing on stderr: a caller keyed on the exit contract learns it is repairable but not what to repair, losing for the crash class the very reason channel the sibling tests pin for the graceful class.
In the EISDIR half, assert the reason channel on the LAST writeStderrLineSafe call with EISDIR-specific text (e.g. toContain('names a directory')) — a bare stringContaining('revert-hunk:') does not discriminate because this suite's mocks accumulate across tests.
Witness:
[probe] mutant deleting the catch's writeStderrLineSafe line -> 27/27 green;
the last-call assertion turns red (lastErr undefined in isolation).
中文说明
EISDIR --out 用例钉住了退出类别（2）与树未被修改，但崩溃类的 stderr 诊断没有任何钉点：全文件仅有的两处 writeStderrLineSafe 断言都针对优雅分支文案，因此删掉 handler 外层 catch 里的 writeStderrLineSafe 调用全绿（突变体实测 27/27），并使所有崩溃类拒绝失声——--out /some/existing/dir 将以退出 2 结束且 stdout、stderr 皆空：按退出契约调用的脚本只知道可修复、不知道修什么，崩溃类失去了兄弟测试为优雅类钉住的那条理由通道。请在 EISDIR 半边对最后一次 writeStderrLineSafe 调用断言 EISDIR 特征文案（如 toContain('names a directory')）——裸 stringContaining('revert-hunk:') 因本套件 mock 跨测试累积而无区分度。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const r1 = runAbDrive({ ...perArm, exec: h1.exec }); | ||
| expect(r1.a?.outcome).toBe('unavailable'); | ||
| expect(r1.b?.outcome).toBe('completed'); | ||
| expect(r1.observed).toBe(false); |
There was a problem hiding this comment.
[Suggestion] R3-22: The per-arm half of the shared-start-failure test pins arm b's capture but never pins that arm b gets a FRESH shared instance after arm a's failed to start — a mutant that skips shared-b's start after shared-a's failure ships 35/35 green (verified), unlike the ready-failure twin which IS pinned. In production arm b then drives up to --timeout against a nonexistent upstream (budget wasted, measuring a harness confound), and the report emits b.sharedAliveAtEnd: null — which reads as 'no --shared configured' — instead of true/false, so a witness quoting arm b's run shape cites a wrong fact.
Add expect(h1.events()).toContain('new:shared-b'); and expect(r1.b?.sharedAliveAtEnd).toBe(true); (the harness keeps shared-b alive: no sentinel written, has-session ok).
Witness:
[probe] mutant skipping the shared retry after a start failure -> 35/35 green;
the suggested assertions fail against it (events lack new:shared-b).
中文说明
共享启动失败测试的 per-arm 半边钉住了 arm b 的捕获，却从未钉住 arm a 启动失败后 arm b 会获得一个全新的共享实例——在 shared-a 失败后跳过 shared-b 启动的突变体 35/35 全绿（实测），而就绪失败的孪生用例是有钉点的。生产中 arm b 将对一个不存在的上游驱动至 --timeout（预算浪费、量到的是 harness 混淆），且报告输出 b.sharedAliveAtEnd: null——读起来像"没有配置 --shared"——而非 true/false，引用 arm b 运行形态的 witness 因此引用错误事实。请补 expect(h1.events()).toContain('new:shared-b'); 与 expect(r1.b?.sharedAliveAtEnd).toBe(true);。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(calls).toHaveLength(2); | ||
| expect(r.applied).toBe(false); | ||
| expect(r.note).toContain('PARTIALLY modified'); |
There was a problem hiding this comment.
[Suggestion] R3-23: The mid-apply kill test leaves conflict unpinned — unlike its --check-killed twin and the spawn-failure test, which both assert r.conflict is toBeUndefined — so adding a conflict field to the harness-kill branch ships green (mutant verified 27/27) and fabricates a 'git refusal text' field for a kill: a killed git's stderr is empty, so a normalizing conflict: apply.stderr || 'git apply refused (no error text)' fires the fallback. The report interface documents conflict as git's refusal text, so a JSON-reading consumer keying on conflict !== undefined as the refusal/coupling signal then reads a SIGKILL harness kill as a refusal carrying quoted git text — the exact misclassification (harness fact recorded as hunk fact) the two sibling tests exist to prevent.
Add expect(r.conflict).toBeUndefined();, mirroring the --check kill test.
Witness:
[probe] mutant adding the conflict fallback to the kill branch -> 27/27 green;
expect(r.conflict).toBeUndefined() fails with
'expected git apply refused (no error text) to be undefined'.
中文说明
apply 中途被杀的测试没有钉住 conflict——其 --check 被杀的孪生与 spawn 失败测试都断言 r.conflict 为 toBeUndefined——因此给 harness-kill 分支加上 conflict 字段全绿（突变体实测 27/27），为一次杀进程伪造出"git 拒绝文本"字段：被杀的 git stderr 为空，归一化的 conflict: apply.stderr || 'git apply refused (no error text)' 会触发兜底文案。报告接口把 conflict 文档化为 git 的拒绝文本，按 conflict !== undefined 判断拒绝/耦合的 JSON 消费者于是把一次 SIGKILL harness 杀进程读成携带引用文本的拒绝——正是两个兄弟测试要防的误分类（harness 事实被记成 hunk 事实）。请仿照 --check kill 测试补 expect(r.conflict).toBeUndefined();。
— qwen3.8-max via Qwen Code /review (v0.22.0)
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 — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the unit suites ran locally on Linux only.
Not explored to full depth (tool budget reached): chunk 2: executing ab-drive.test.ts under vitest (worktree has no node_modules; npm ci + prerequisite builds exceed the tool budget) — all assertions were instead ve….
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/ab-drive.test.ts:134 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/ab-drive.test.ts:335 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/ab-drive.test.ts:379 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/ab-drive.test.ts:380 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/ab-drive.test.ts:384 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/ab-drive.test.ts:495 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/ab-drive.test.ts:712 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/ab-drive.ts:276 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/ab-drive.ts:317 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/ab-drive.ts:577 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/ab-drive.ts:594 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/ab-drive.ts:751 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/revert-hunk.test.ts:63 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/revert-hunk.test.ts:132 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/revert-hunk.test.ts:294 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/revert-hunk.test.ts:565 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/revert-hunk.test.ts:572 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/revert-hunk.ts:120 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/revert-hunk.ts:121 — [review] Deferred (convergence posture, round 4packages/cli/src/commands/review/revert-hunk.ts:270 — [review] Deferred (convergence posture, round 4- …and 3 more (see the run report)
Convergence: round 4 posted 25 inline comment(s), 17 of them reported for the first time; the previous round posted 26 (26 new). Findings keep coming back to the same files: packages/cli/src/commands/review/ab-drive.ts (findings in round 3; 7 more now); packages/cli/src/commands/review/revert-hunk.ts (findings in round 3; 5 more now); packages/cli/src/commands/review/revert-hunk.test.ts (findings in round 3; 3 more now), and 1 more file(s). 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.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the unit suites ran locally on Linux only。
未探索到全部深度（达到工具调用预算）：chunk 2：executing ab-drive.test.ts under vitest (worktree has no node_modules; npm ci + prerequisite builds exceed the tool budget) — all assertions were instead ve…。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 23 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 25 条行内评论，其中 17 条是首次提出；上一轮发布了 26 条（其中 26 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/ab-drive.ts（第 3 轮已出过发现，本轮又有 7 条）；packages/cli/src/commands/review/revert-hunk.ts（第 3 轮已出过发现，本轮又有 5 条）；packages/cli/src/commands/review/revert-hunk.test.ts（第 3 轮已出过发现，本轮又有 3 条），另有 1 个文件。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (r.harnessFailure) process.exitCode = 2; | ||
| else if (!r.applied) process.exitCode = 1; |
There was a problem hiding this comment.
[Critical] R4-1: Root cause — this command's exit-code taxonomy (0 applied / 1 refusal-coupling-fact / 2 repairable) leaks non-refusal failures into exit 1, the class the handler's own comment reserves for "a real refusal / coupling fact" a calling script records. Six entrances, each executed at this commit:
- Handler catch-all — staging IO failures (mkdtempSync/writeFileSync ENOSPC/EACCES inside
runRevertHunk) escape the harnessFailure classification → exit 1 with the tree untouched and git never run. Probe: broken TMPDIR → exit 1, stderrrevert-hunk: ENOENT… mkdtemp…, no report on stdout. - yargs usage failures — missing
--diff(Missing required argument: diff) and--list+--hunkconflicts die in the CLI-global.fail()(config.ts:979-983) with exit 1, while the identical repairable shape mistyped--diffexits 2. - git fatal folding — the --check refusal branch folds git FATAL exits into the conflict class: status 128
fatal: not a git repository(pruned-gitdir worktree, reproduced) and plain non-repo--treedirs — recorded as coupling facts although git never inspected the patch. - Apply-after-check race — the branch returns
conflictWITHOUTharnessFailure, contradicting its own comment ("Report it as the harness fact it is") → exit 1 although--checkone git call earlier proved independent revertibility. - Repairable-input refusals — garbage/missing selectors and the unsupported-prefix recapture note exit 1 alongside genuine coupling refusals, while the verifier brief mechanizes
applied: falseas a coupling FACT to quote. - CRLF folding — a CRLF-corrupted
--difffailsgit apply -R --checkwith status 1, indistinguishable from a genuine context mismatch, folded into the coupling class with a note asserting a false cause and a repair (reset the scratch tree) that cannot fix it. Reproduced; the identical diff with LF endings applies.
Witness:
built CLI: no --diff -> 'Missing required argument: diff' exit 1
--list + --hunk -> exit 1
control: mistyped --diff -> exit 2
runRevertHunk(pruned worktree) -> conflict 'fatal: not a git repository…', handler exit 1 (raw git STATUS=128)
CRLF --diff -> applied:false + coupling note; identical LF diff -> applied:true
In every entrance the calling verifier records a fabricated coupling/refusal fact — the outcome this command exists to prevent. The round-3 locations of this pattern (R3-7/R3-8) were fixed, and the pattern produced six new siblings this round — fix the taxonomy once: mark every non-refusal failure with a report flag (reuse harnessFailure with a widened docstring, or add inputError) on the staging-IO, unsupported-prefix, selector-validation, fatal-128, and apply-race returns; map if (r.harnessFailure || r.inputError) process.exitCode = 2; here; give the command a scoped yargs .fail() exiting 2 for usage errors; and refuse CRLF input (/\r/.test(diffText)) with a renormalize instruction before classification.
中文说明
[Critical] R4-1：根因发现——本命令的退出码分类（0 已应用 / 1 拒绝/耦合事实 / 2 可修复）把六类"非拒绝"失败泄漏进了 exit 1——即 handler 注释保留给"调用脚本会记录为真实拒绝/耦合事实"的那一类。六个入口均在本提交上实弹验证：(1) handler 兜底 catch 把 runRevertHunk 内部的暂存 IO 失败（mkdtempSync/writeFileSync 的 ENOSPC/EACCES）归为 exit 1——探针：坏 TMPDIR → exit 1、stderr 报错、无 JSON 报告；(2) yargs 用法错误（缺 --diff、--list+--hunk 冲突）死在 CLI 全局 .fail() 里退出 1，而同样的可修复形状打错 --diff 却退出 2；(3) --check 拒绝分支把 git 致命退出（状态 128 'fatal: not a git repository'——剪掉 gitdir 的 worktree，已复现；以及普通非仓库 --tree）折进 conflict 类——git 根本没检查补丁却记成耦合事实；(4) check 通过但 apply 失败的竞态分支返回 conflict 却不带 harnessFailure，与其自身注释"作为 harness 事实报告"矛盾，--check 刚证明可独立还原却退出 1；(5) 可修复输入拒绝（垃圾/不存在的 hunk 选择器、不支持前缀的"请重新抓取"注记）与真正的耦合拒绝同样退出 1，而验证者 brief 把 applied: false 机制化为"可引用的耦合事实"；(6) CRLF 损坏的 --diff 使 git apply -R --check 以状态 1 失败、与真实上下文不匹配无法区分，被折进耦合类并断言错误原因、给出无法修复的建议——已复现，同样的 LF diff 可以应用。修复：集中分类——给暂存 IO、不支持前缀、选择器校验、fatal-128、apply 竞态这些非拒绝失败统一打报告标记（复用拓宽文档的 harnessFailure 或新增 inputError），handler 映射 if (r.harnessFailure || r.inputError) process.exitCode = 2;，为命令加作用域内的 yargs .fail()（用法错误退出 2），并在分类前以 /\r/.test(diffText) 拒绝 CRLF 输入、给出重新规范化指引。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const isDir = (p: string) => { | ||
| try { | ||
| return statSync(resolve(p)).isDirectory(); |
There was a problem hiding this comment.
[Critical] R4-2: The arm-root gates validate existence (statSync().isDirectory()) and identity (realpath pin + checkRoot re-checks) but never chdir-usability: a directory whose search bit is revoked (mode 000) passes every layer, and tmux new-session -c <dir> then exits 0 and silently starts the session in a fallback cwd — observed: true licensing a verdict about a tree never driven.
Reproduced link by link on this machine: statSync(mode-000 dir).isDirectory() → true; realpathSync resolves; tmux new-session -d -c <mode-000> → exit 0 with the session's real cwd the fallback directory. End-to-end through unmodified runAbDrive (armB = mode-000 dir): all gates pass, both arms "complete", observed: true, b.output shows the fallback-cwd content (cat: marker.txt: No such file or directory) while b.root names the validated tree. Non-adversarial trigger: a mode-000 lock or restricted mount passed as an arm; adversarial: same-uid driven code revoking the search bit inside the validate→use window hits the identical missing check (checkRoot re-validates identity only).
Witness:
runAbDrive(armB=mode-000) -> observed:true, b.root=<validated path>,
b.output = fallback-cwd content
tmux -c <mode-000> -> exit 0, session cwd = fallback
fix flip (accessSync X_OK in isDir) -> observed:false, 'nothing was started', 38/38 green
Probe searchability in the same gates: fs.accessSync(resolve(p), fs.constants.X_OK) inside isDir AND inside checkRoot — the adversarial revocation lands precisely in the validate→use window the re-check exists for.
中文说明
[Critical] R4-2：臂根门卫校验了存在性（statSync().isDirectory()）与身份（realpath 钉扎 + checkRoot 复检），却从不校验 chdir 可用性：搜索位被撤销的目录（mode 000）能通过所有层，随后 tmux new-session -c <dir> 退出 0 并静默回退到后备 cwd 启动会话——observed: true 给一份"从未在该树里驱动过"的裁决背书。本机逐环节复现：statSync(mode-000).isDirectory() → true；realpathSync 可解析；tmux -c → exit 0、会话真实 cwd 为后备目录；未改动的 runAbDrive 端到端（armB=mode-000）：全部门卫通过、双臂"完成"、observed:true、b.output 是后备 cwd 的内容而 b.root 写着已校验路径。非对抗触发：mode-000 锁目录或受限挂载被当作臂传入；对抗触发：同 uid 的被驱动代码在"校验→使用"窗口内撤销搜索位——命中同一个缺失检查（checkRoot 只复检身份）。修复：在 isDir 与 checkRoot 内同时探测 fs.accessSync(resolve(p), fs.constants.X_OK)——对抗性撤销恰恰落在复检所覆盖的窗口里。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const readIfThere = (p: string): string => { | ||
| try { | ||
| return readFileSync(p, 'utf8'); |
There was a problem hiding this comment.
[Critical] R4-3: The poll loop reads the arm's log/rc files — path-addressed files the untrusted driven script knows and can replace — with blocking readFileSync. Swapping either path for a FIFO hangs the harness indefinitely: open(O_RDONLY) blocks waiting for a writer that never comes, inside the single-threaded poll loop.
The arm script is untrusted by this file's own threat model and learns its paths trivially — /proc/$PPID/cmdline of the wrapper carries the full bash '<script>' > '<log>' 2>&1 line (verified on this machine). It rms the log (or rc) and mkfifos the same path; the next poll's readFileSync then blocks in open() — verified locally: a readFileSync of a FIFO under a 2s timeout neither returned nor threw. Consequences: (1) Date.now() >= deadline is never evaluated again — --timeout defeated, the exact failure the NaN-budget guard's comment says must never happen ("a hung script would hang this command forever and the finally kill-server would never run"); (2) no JSON report is ever emitted; (3) the finally kill-server never runs, so the keeper session pins the tmux server indefinitely. Real-tmux end-to-end: the driven arm performs the swap; a harness with a 5s budget was still alive at 20s (watchdog SIGKILL, no report); tmux ls afterwards showed arm-a and hold sessions still alive. Same shape via readIfThere(a.rcPath) and the post-gate log re-read.
Witness:
real-tmux e2e: WATCHDOG: child still running after 20s -> HANG
CHILD STDOUT: SERVER-NAME only (no REPORT)
post-kill `tmux ls`: 'arm-a: 1 windows', 'hold: 1 windows'
fix flip (O_NONBLOCK open + fstat isFile): report
{observed:false, a:timed-out, b:timed-out} in ~6s
Open non-blocking and require a regular file before reading: const fd = openSync(p, fs.constants.O_RDONLY | fs.constants.O_NONBLOCK); try { if (!fstatSync(fd).isFile()) return ''; return readFileSync(fd, 'utf8'); } finally { closeSync(fd); } — O_NONBLOCK makes a writer-less FIFO open succeed and fstat exposes it (also neutralizes directory/device swaps).
中文说明
[Critical] R4-3：轮询循环用阻塞式 readFileSync 读取臂的 log/rc 文件——这些是被驱动（不可信）脚本知道且可以替换的按路径寻址文件。把任一路径换成 FIFO 会让 harness 无限挂起：open(O_RDONLY) 在单线程轮询循环里等待一个永不出现的写者。臂脚本按本文件自己的威胁模型就是不可信的，且能轻易获知路径——包装进程的 /proc/$PPID/cmdline 里就有完整的 bash '<script>' > '<log>' 2>&1 行（本机已验证）。它 rm 掉 log（或 rc）再 mkfifo 同一路径，下一次轮询的 readFileSync 就阻塞在 open()——本机验证：2 秒 timeout 下对 FIFO 的 readFileSync 既不返回也不抛。后果：(1) 截止时间再也不会被求值——--timeout 失效，正是 NaN 预算守卫注释说"绝不能发生"的失败（"挂死的脚本会挂死整个命令，finally 的 kill-server 永远不会执行"）；(2) 永远不会输出 JSON 报告；(3) finally kill-server 不执行，keeper 会话把 tmux 服务器永久钉住。真 tmux 端到端：被驱动臂完成替换后，5 秒预算的 harness 20 秒仍存活（看门狗 SIGKILL、无报告），事后 tmux ls 显示 arm-a 与 hold 会话仍在。readIfThere(a.rcPath) 与门控后的日志重读是同形入口。修复：非阻塞打开并要求常规文件——openSync(p, O_RDONLY | O_NONBLOCK) + fstatSync(fd).isFile() 检查（同时化解目录/设备替换）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * captures always use default prefixes; a non-standard one arrives only | ||
| * through arbitrary `--diff`, and refusing it is safe where mutating is not. | ||
| */ | ||
| export function renameSectionHasUnsupportedPrefix( |
There was a problem hiding this comment.
[Suggestion] R4-4: The new unsupported-prefix guard (round-3 R3-5's fix) does not fire for two input classes it exists to refuse: copy-with-edits sections captured with non-standard prefixes, and C-quoted rename paths captured with non-default prefixes.
Symptom 1 (copy): parseDiff never surfaces copy from (only rename from at diff-plan.ts:388), so the guard bails at file.renameFrom === undefined; with x//y/ prefixes stripSide returns null, the token rewrite is skipped, and git apply -R rewinds the COPY — reproduced: applied: true, note "reverted hunk y/copy.txt:1", observed copyStillExists: false (copy deleted, source restored) while the note claims a content revert. Symptom 2 (quoted): standard() accepts any quoted token, but stripSide's tok.slice(3) assumes " + 2-char prefix — reproduced with --src-prefix=src/ --dst-prefix=dst/ café→naïve rename: applied: true, note "reverted hunk dst/naïve.txt:1", tree gained the old-path file. Both are the "different mutation than the report names" class the guard exists to refuse.
Witness:
S1 (copy): applied:true + copy deleted (real git)
S2 (quoted): applied:true + naïve-GONE + old-path file restored
fix flip (guard keyed on copy-from metadata + quoted-standard prefixes):
both inputs -> applied:false with recapture note, tree unchanged, 30/30 green
Scan the section header for a copy from line inside the guard (or surface copyFrom from parseDiff) and run the same check for copies; tighten standard() to accept only quoted tokens starting "a/ or "b/.
中文说明
[Suggestion] R4-4：新增的不支持前缀守卫（第 3 轮 R3-5 的修复）对它本该拒绝的两类输入不生效：以非标准前缀抓取且含编辑的 copy 段，以及以非默认前缀抓取、路径被 C 引用的改名。症状 1（copy）：parseDiff 只从 rename from 取值、从不暴露 copy from，守卫在 file.renameFrom === undefined 处直接放行；x//y/ 前缀下 stripSide 返回 null、记号改写被跳过，git apply -R 把 COPY 回退掉——复现：applied:true、注记 "reverted hunk y/copy.txt:1"、copy 被删除而源文件恢复，注记却声称是内容还原。症状 2（引号）：standard() 接受任何带引号记号，但 stripSide 的 tok.slice(3) 假定 " + 2 字符前缀——用 --src-prefix=src/ --dst-prefix=dst/ 的 café→naïve 改名复现：applied:true、树里出现旧路径文件。两者都是守卫旨在拒绝的"与报告命名不同的变异"。修复：守卫内扫描段头的 copy from 行（或让 parseDiff 暴露 copyFrom）并对 copy 跑同样检查；收紧 standard() 只接受以 "a/ 或 "b/ 开头的引号记号。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if ( | ||
| exitCode !== null && | ||
| tmux('has-session', '-t', `arm-${arm}`).status !== 0 | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R4-5: The completion gate's anti-plant half (the has-session clause added this round) has no discriminating fixture: the harness writes the sentinel and immediately marks the session ended, so no test ever presents "sentinel exists while the session still exists".
Mutant deleting the has-session clause ships the full suite 39/39 green. With the clause gone, a driven PR tree that plants its own .rc mid-run reads completed with a fabricated exit code and observed: true licenses the comparison. Verified by probe: planted .rc with the session kept alive — intact code reports timed-out / observed: false (plant refused); the mutant reports completed / exitCode 0 / observed: true with the note "both arms completed".
Witness:
plant + alive session: intact -> {aOutcome: timed-out, observed: false}
mutant -> {aOutcome: completed, aExitCode: 0, observed: true}
mutant suite: 39/39 green
Add a harness knob (e.g. plantSentinel?: string[]) that writes the arm's .rc during onSession while keeping the session out of endedSessions, and assert the arm does NOT read completed while the session is alive.
中文说明
[Suggestion] R4-5：完成门的防植入半边（本轮新增的 has-session 子句）没有判别性夹具：harness 写下 sentinel 就立即把会话标记为已结束，因此没有任何测试能呈现"sentinel 存在而会话仍存活"的状态。删除 has-session 子句的突变体整套 39/39 全绿；失去该子句后，被驱动的 PR 树在运行中植入自己的 .rc 即可读到 completed 与伪造退出码，observed: true 随即给比较背书。探针：植入 .rc 且保持会话存活——成品代码 timed-out / observed:false（植入被拒绝）；突变体 completed / exitCode 0 / observed:true。修复：给 harness 加 plantSentinel 开关（在 onSession 中写入 .rc 但不把会话加入 endedSessions），断言会话存活期间该臂不得读到 completed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const r = runAbDrive({ ...args, exec: h.exec }); | ||
| expect(r.observed).toBe(false); | ||
| expect(r.a).toBeNull(); | ||
| expect(r.note).toContain('could not start a session'); |
There was a problem hiding this comment.
[Suggestion] R3-15: Still standing from round 3 — killedStale is asserted only on the two happy paths; the keeper-failure path here and both --shared-once stop paths return fail(note, { killedStale }) unpinned, and the false value (the normal case — no stale server to reclaim) is never exercised either.
Mutant dropping the { killedStale } partial from the keeper-failure return ships the whole suite 38/38 green (verified); every post-reclaim failure report then says killedStale: false though a stale server WAS reclaimed — a false reclaim fact in the field the suite's own comment says "is quoted into witnesses", sending a verifier to re-check staleness that was already handled. The harness answers kill-server with ok(), so the correct pinned value on these paths is true.
Add expect(r.killedStale).toBe(true) to the keeper-failure test and at least one of the once-stop tests.
中文说明
[Suggestion] R3-15：第 3 轮仍未解决——killedStale 只在两条快乐路径上被断言；此处的 keeper 失败路径与两条 --shared-once 停止路径都返回未钉扎的 fail(note, { killedStale })，且 false 值（常态——没有陈旧服务器可回收）也从未被演练。删除 keeper 失败返回值中 { killedStale } 偏量的突变体整套 38/38 全绿（已验证）；此后每次"回收成功后失败"的报告都写 killedStale: false，尽管确实回收了陈旧服务器——在套件注释自称"会被证词引用"的字段里写入假回收事实。harness 对 kill-server 应答 ok()，这些路径上应钉的正确值是 true。修复：在 keeper 失败测试与至少一条 once 停止测试中加 expect(r.killedStale).toBe(true)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(process.exitCode).toBe(2); | ||
| process.exitCode = 0; | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R3-17: Still standing from round 3 — this exit-2 wiring test asserts only process.exitCode === 2; the ordering its comment claims to pin — assertWritableOutPath fires BEFORE the arms drive — is untested.
Mutant moving assertWritableOutPath(a.out) to after runAbDrive keeps the test green (exit is still 2 via the TypeError catch): with --out /some/directory a caller waits out both arm budgets (2 × 300 s default) before receiving the same instant exit 2 the comment says must fire before any driving. Note from this round's verifier: the previously proposed stdout-count assertion does NOT discriminate this mutant (the assert still throws before writeStdoutLine in both orderings) — the pin must observe the drive itself.
Witness:
ordering mutant: wiring test passed (exitCode 2) with the arms actually driven
mutant + stdout-count assertion: also passed — proposed fix insufficient
Assert the drive never happened on the exit-2 path — e.g. that no arm session was ever started (harness event log empty) across the invocation.
中文说明
[Suggestion] R3-17：第 3 轮仍未解决——这个 exit-2 接线测试只断言 process.exitCode === 2；其注释声称钉住的顺序——assertWritableOutPath 先于双臂驱动——并未被测试。把 assertWritableOutPath(a.out) 移到 runAbDrive 之后的突变体仍绿（TypeError catch 照样退出 2）：--out /some/directory 时调用方要先熬过双臂预算（默认 2×300 秒）才得到那个本应立即出现的 exit 2。本轮验证者提示：此前提议的 stdout 计数断言无法判别该突变体（两种顺序下断言都先于 writeStdoutLine 抛出）——钉扎必须观察"驱动本身是否发生"。修复：断言 exit-2 路径上驱动从未发生（如整个调用期间没有任何臂会话启动、harness 事件日志为空）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // tested: `not runnable` and `not running —` are the brief's reason-less | ||
| // forms too, and counting their `nable`/`ning` letters as reason content | ||
| // was the same fail-open one suffix over. | ||
| const m = /^\s*(?:witness:\s*)?not run[\p{L}\p{N}]*(?<rest>[\s\S]*)$/iu.exec( |
There was a problem hiding this comment.
[Suggestion] R3-21: Still standing from round 3 (new shapes merged this round) — isEmptyNotRunWitness still fails to recognize reason-less variants of the escape phrase: both the detached-repetition shape round 3 filed (not run — not run: the repeated phrase's letters count as reason content) and whitespace variants of the phrase itself — 'not run' (double space), 'not\trun' (tab), 'not\u00A0run' (NBSP) never match the single-literal-space regex at all.
Either shape lets a high-confidence [review] Critical/Suggestion with zero executed evidence pass as witnessed and post — the exact fail-open the function exists to stop. Reproduced against the real function: all variants accepted (isEmpty: false, finding stays high); control 'not run' correctly demoted.
Witness:
real isEmptyNotRunWitness + holdUnwitnessedFindings:
'not run' / 'not\trun' / 'not\u00A0run' -> isEmpty:false, stays high
fix (not\s+run + detached-phrase handling) -> all demoted to low
reason-bearing controls unaffected
Match the separator as whitespace (/not\s+run/) and treat a remainder consisting only of dash glyphs plus repetitions of the escape phrase itself as empty; add the variant fixtures to the reason-less tests.
中文说明
[Suggestion] R3-21：第 3 轮仍未解决（本轮并入新形状）——isEmptyNotRunWitness 仍认不出免跑短语的无理由变体：既有第 3 轮提出的"分离重复"形状（'not run — not run'：重复短语的字母被算作理由内容），也有短语本身的空白变体——'not run'（双空格）、'not\trun'（制表符）、'not\u00A0run'（NBSP）完全匹配不上那个单字面空格的正则。任一形状都让零执行证据的高置信 [review] Critical/Suggestion 被当作有证词而发布——正是该函数要堵的失败开放。已对真实函数复现：所有变体均被接受（isEmpty: false、保持 high）；对照 'not run' 正确降级。修复：分隔符按空白匹配（/not\s+run/），并把"仅剩破折号字形与短语自身重复"的余文视为空；把变体夹具加入无理由测试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(r.identicalOutput).toBeNull(); | ||
| }); | ||
|
|
||
| it('an arm whose log crosses the cap ends overflowed — no verdict, gate failed', () => { |
There was a problem hiding this comment.
[Suggestion] R3-22: Still standing from round 3 — this overflowed test pins outcome/exitCode/observed but neither of the two post-overflow behaviors its sibling tests pin for every other early-stop outcome: that the overflowed arm session is killed, and that arm b is still driven.
Mutant skipping the unconditional kill-session when outcome === 'overflowed' ships 38/38 green (verified): arm a's still-running writer contends with arm b for the same ports/files through arm b's whole drive window (up to --timeout, default 300 s) and keeps growing its log unbounded — the exact cross-arm confound the kill's own comment names ("an overflowed writer keeps growing its log through arm b's whole window"). Only the final kill-server at run end stops it, long after arm b's capture was taken.
Witness:
mutant: suite 38/38 green; the timed-out test's kill-order pins do not cover the overflowed arm
Add the same event pins the timed-out test has: expect(ev).toContain('kill:arm-a') and expect(ev.indexOf('kill:arm-a')).toBeLessThan(ev.indexOf('new:arm-b')), plus that arm b still completes.
中文说明
[Suggestion] R3-22：第 3 轮仍未解决——这个溢出测试钉住了 outcome/exitCode/observed，却没钉它的兄弟测试为其他每种提前停止结局钉住的两个溢出后行为：溢出的臂会话被杀、且 arm b 仍被驱动。在 outcome === 'overflowed' 时跳过无条件 kill-session 的突变体 38/38 全绿（已验证）：arm a 仍在运行的写者会在 arm b 的整个驱动窗口（最长 --timeout，默认 300 秒）里与 arm b 争抢同样的端口/文件并无限增大日志——正是 kill 注释点名的跨臂混杂。修复：补上与超时测试相同的事件钉扎（kill:arm-a 存在且先于 new:arm-b），并断言 arm b 仍完成。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(vi.mocked(writeStderrLineSafe)).toHaveBeenCalledWith( | ||
| expect.stringContaining('revert-hunk:'), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R3-24: Still standing from round 3 in substance — the assertion added this round to pin the EISDIR crash-class stderr diagnostic is pre-satisfied by accumulated mock calls: writeStderrLineSafe is module-mocked with no clearing anywhere (no clearAllMocks/beforeEach in this file, none in vitest.config.ts or test-setup.ts), and the earlier usage-branch test already called it with a string containing 'revert-hunk:'.
Mutant deleting only the outer-catch writeStderrLineSafe call ships the suite 27/27 green (verified) — the silent-exit-2 regression the adjacent comment names ("deleting the writeStderrLineSafe there would exit 2 with nothing said") ships past the test advertising itself as the guard.
Witness:
mutant: suite 27/27 green
with toHaveBeenLastCalledWith(stringContaining('revert-hunk: --out names a directory')):
the same mutant FAILS — the failure diff shows the accumulated prior call
Assert the call THIS invocation produced: expect(vi.mocked(writeStderrLineSafe)).toHaveBeenLastCalledWith(expect.stringContaining('--out names a directory')) — or add beforeEach(() => vi.clearAllMocks()) to the suite.
中文说明
[Suggestion] R3-24：第 3 轮实质上仍未解决——本轮为钉住 EISDIR 崩溃类 stderr 诊断而加的断言被累积的 mock 调用预先满足：writeStderrLineSafe 是模块级 mock 且任何地方都不清理（本文件无 clearAllMocks/beforeEach，vitest.config.ts 与 test-setup.ts 也没有），更早的用法分支测试已经用含 'revert-hunk:' 的字符串调用过它。只删除外层 catch 中 writeStderrLineSafe 调用的突变体整套 27/27 全绿（已验证）——邻近注释点名的"删掉它就会静默 exit 2"的回归，从自称为其守卫的测试眼皮底下溜过。修复：断言本次调用产生的那次调用（toHaveBeenLastCalledWith(stringContaining('--out names a directory'))），或给套件加 beforeEach(() => vi.clearAllMocks())。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
1023c3a to
d92b20c
Compare
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.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R4-11 exists-but-unreadable --diff probe unwired — already reported (comment 3838755301, posted under the round-4 id-shifted prefix 'R4-10') at revert-hunk.ts:472
- R4-17 staging-cleanup never drives a failed patch write — already reported (comment 3838755318, posted under the round-4 id-shifted prefix 'R4-16') at revert-hunk.test.ts:595
- R5-27 planted-sentinel exitCode leak into timed-out reports — overlaps existing R3-1 thread at ab-drive.ts:591 (comment 3838133067)
- R5-33 staging-cleanup test never pins staging-dir existence — overlaps existing R3-20 thread at revert-hunk.test.ts:631 (comment 3838133111)
Unresolved, please confirm:
- [Critical] R3-1 (comment 3838133067, ab-drive.ts:591): path-addressed evidence channels forgeable by driven code — the demonstrated rc-plant entrance is closed by the sentinel-plus-session-gone completion gate at this commit, but the wider capture-for…
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the unit suites ran locally on Linux only.
Test Plan (not a blocker): packages/cli/src/utils/findings.test.ts — no such file or directory.
Convergence: round 5 posted 56 inline comment(s), 41 of them reported for the first time; the previous round posted 25 (17 new). Findings keep coming back to the same files: packages/cli/src/commands/review/ab-drive.test.ts (findings in rounds 3, 4; 13 more now); packages/cli/src/commands/review/revert-hunk.test.ts (findings in rounds 3, 4; 11 more now); packages/cli/src/commands/review/ab-drive.ts (findings in rounds 3, 4; 10 more now), and 1 more file(s). 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, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (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.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the unit suites ran locally on Linux only。
Test Plan（非阻断）：packages/cli/src/utils/findings.test.ts — no such file or directory。
收敛情况：第 5 轮发布了 56 条行内评论，其中 41 条是首次提出；上一轮发布了 25 条（其中 17 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/ab-drive.test.ts（第 3、4 轮已出过发现，本轮又有 13 条）；packages/cli/src/commands/review/revert-hunk.test.ts（第 3、4 轮已出过发现，本轮又有 11 条）；packages/cli/src/commands/review/ab-drive.ts（第 3、4 轮已出过发现，本轮又有 10 条），另有 1 个文件。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(r2.note).toContain('--shared-cwd'); | ||
| }); | ||
|
|
||
| it('refuses a directory whose search bit is revoked — tmux -c would fall back', () => { |
There was a problem hiding this comment.
[Critical] R5-7: The mode-000 refusal fixture (ab-drive.test.ts:257) is unrepresentable on Windows — W8's defect class in a second file: chmodSync(dir, 0o000) only toggles the read-only attribute there and libuv's accessSync(dir, X_OK) succeeds for any existing directory (verified against libuv src/win/fs.c fs__access), so isDir() returns true, the guard does not refuse, both harness arms drive to observed:true and expect(r.observed).toBe(false) fails — the merge-queue Windows job goes red. The file already uses the house convention (it.skipIf on the FIFO test); the repo's own scratch-tree.test.ts:814 documents this exact Windows behavior for chmod-permission tests.
Witness:
libuv v1.x src/win/fs.c fs__access: with X_OK only, the success clause is always true for an existing path; chmodSync maps to _wchmod (read-only attribute only). In-repo authority: scratch-tree.test.ts:814 and cost-ledger.test.ts:1864 state the same.
中文说明
mode-000 拒绝夹具同样无法在 Windows 上表示——与上一条同类的第二个文件：chmodSync(dir, 0o000) 在 Windows 只切换只读属性，libuv 的 accessSync(dir, X_OK) 对任何存在目录都成功（已核对 libuv src/win/fs.c 的 fs__access 实现），于是 isDir() 返回 true、守卫不拒绝、双臂驱动至 observed: true，expect(r.observed).toBe(false) 失败。仓库自己的 scratch-tree.test.ts 与 cost-ledger.test.ts 都记录了同样的 Windows 行为并跳过。修复：it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)（root 跳过同时关闭已确认的 R5-15）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const readIfThere = (p: string): string => { | ||
| let fd: number | undefined; | ||
| try { | ||
| fd = openSync(p, constants.O_RDONLY | constants.O_NONBLOCK); |
There was a problem hiding this comment.
[Critical] R5-4: The arm log-capture channel has no integrity signal: the rm/mkfifo swap that readIfThere's own docstring names is converted from a hang into a silently EMPTY capture that still reaches completed and still compares equal. The pane keeps writing into the original inode, the rc channel is untouched, both arms honestly reach the two-signal completion, readIfThere returns '' (ENOENT; FIFO fails isFile) — and because both arms run the same bytes the destruction is symmetric: observed:true, identicalOutput:true, a 'the PR changes nothing' verdict manufactured from two destroyed captures, with a note inviting the verifier to quote deciding lines that do not exist.
Witness:
Probe: both arms' log paths destroyed mid-drive, wrappers exit honestly → PR: observed:true, aOutput:'', bOutput:'', identicalOutput:true, note 'quote the deciding lines of each as the witness'; minimal integrity check at completion (absent/non-regular log → 'unavailable' + confound note) → observed:false, identicalOutput:null; fix keeps the suite green (35 passed).
中文说明
臂日志捕获通道没有完整性信号：readIfThere 文档点名的 rm/mkfifo 替换被从“挂起”变成“静默空捕获”——面板继续写原 inode、rc 通道未动、两臂诚实地走完两信号完成门，readIfThere 返回 ''，两臂对称 → observed: true、identicalOutput: true：用两份被销毁的捕获制造“PR 无任何变化”的裁决，注记还邀请验证者引用并不存在的“决定性行”。实测：驱动中销毁两臂日志路径 → PR 代码给出 observed: true + 空捕获；完成时加最小完整性检查（缺失/非常规文件 → unavailable + 混淆注记）→ observed: false；修复不破坏套件。触发只需被驱动脚本本身——完全在本文件处处声明的威胁模型之内。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * "alive" reported a crashed upstream as having outlived its arm — and | ||
| * `observed: true` then licensed captures taken against a corpse. | ||
| */ | ||
| const sharedAlive = (): boolean => |
There was a problem hiding this comment.
[Critical] R5-36: Asymmetric forgery handling: the arm completion gate rejects a planted sentinel by requiring the session to be gone, but sharedAlive (ab-drive.ts:444-447) accepts a PLANTED sentinel as proof of death with no session conjunct — death is 'sentinel present OR session gone', so the plant alone decides it. The plant path is concrete: wrapScript embeds the rc path verbatim in the wrapper's trap line, and the wrapper .sh file plus /proc/$PPID/cmdline are same-uid readable (the mkdtemp 0700 dirs do not exclude the uid the arms run under). The shared script starts the PR's own server (the canonical --shared shape); that server learns the rc path and writes a fake sentinel while the daemon is still alive → sharedAliveAtEnd:false → observed:false with the note 'the shared process died before an arm finished … Fix the shared script' — a forged confound fact misdirecting repair at the verifier's shared script; in --shared-once mode the same plant during arm a's window trips arm b's pre-check and suppresses the entire run. The code's own comments put this attacker in scope (the arm gate exists because the arm's own untrusted code can plant arm-.rc; the runDir comment names cross-phase planting).
Witness:
PR: PLANT observed=false, sharedAliveAtEnd=false, note 'the shared process died before an arm finished … Fix the shared script'; PLANT-ONCE observed=false, b=not-ready. FIX: PLANT observed=true; PLANT-ONCE observed=true; HONEST-DEATH/SIGKILL controls stay observed=false; LIVE control stays true. Landing note: the fix also requires the fake harness to end shared sessions the way it ends arm sessions (three sentinel-only death fixtures go red until updated).
中文说明
伪造处理不对称：臂完成门拒绝植入的 sentinel（要求会话已消失），但 sharedAlive（:444-447）把植入的 sentinel 当作死亡证明、没有会话合取——死亡判定变成“sentinel 存在 或 会话消失”，植入本身就能决定结果。植入路径具体：wrapScript 把 rc 路径原样嵌在包装脚本的 trap 行里，包装脚本 .sh 与 /proc/$PPID/cmdline 同 uid 可读（mkdtemp 0700 目录并不排除臂运行的 uid）。典型 --shared 形态下共享脚本启动的是 PR 自己的服务——它读到 rc 路径、在 daemon 仍活着时写入假 sentinel → sharedAliveAtEnd: false → observed: false，注记“共享进程在臂完成前死亡……修复共享脚本”——一个把修复引向验证者共享脚本的伪造混淆事实；--shared-once 模式下同样的植入（发生在 arm a 窗口）会触发 arm b 的前置检查、压制整个运行。代码注释本身就把这个攻击者纳入范围（臂门的存在正是因为“臂自己的（不可信）代码能植入 arm-.rc”；runDir 注释点名跨阶段植入）。实测：植入 → PR 代码在两种模式下都伪造混淆；镜像门（sentinel 存在时还需 has-session 失败）→ observed: true，诚实死亡/SIGKILL 对照仍判死亡。落地注意：该修复同时要求假 harness 像结束臂会话一样结束共享会话（三个仅以 sentinel 模拟死亡的夹具会翻红，需同步更新）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ); | ||
| if ( | ||
| args.sharedCwd !== undefined && | ||
| realpathOf(sharedCwd) !== realShared |
There was a problem hiding this comment.
[Critical] R5-3: The --shared-cwd mid-run re-validation (ab-drive.ts:525-528) lacks the !isDir fallback the arm-side checkRoot has: realpathOf(sharedCwd) !== realShared alone passes when the directory loses its search bit (realpath still resolves) or when both pins race to null (null !== null is false). The shared daemon then starts in an unsearchable/gone directory — tmux new-session -c silently falls back — and observed:true is licensed with the daemon serving both arms from a directory the report never names.
Witness:
Probe (chmod-000 sharedCwd after the pin, per-arm mode, no --shared-ready): PR arm starts shared-a AND shared-b with -c = the unsearchable dir, r.observed=true, note never mentions --shared-cwd; fix (|| !isDir(sharedCwd)) → zero shared sessions started, bail 'unavailable'. Preceding measurement: realpathSync(chmod-000 dir) resolves while accessSync(X_OK) throws.
中文说明
--shared-cwd 的中途复检只有 realpath 比较，缺少 arm 侧 checkRoot 拥有的 !isDir 兜底：目录被撤销搜索位时 realpath 仍可解析、守卫放行；两枚指针都为 null 时 null !== null 亦为 false。共享 daemon 随后在不可搜索/已消失的目录上启动（tmux new-session -c 静默回退），两臂对着报告从未提及的目录完成，observed: true 背书。实测（pin 后 chmod-000）：shared-a 与 shared-b 都在不可搜索目录上启动、observed: true；加上 || !isDir(sharedCwd) 后零会话启动、unavailable。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(() => statSync(join(dir, 'old.txt'))).toThrow(); | ||
| }); | ||
|
|
||
| it('reverts a mode-changed file’s content hunk WITHOUT flipping the mode', () => { |
There was a problem hiding this comment.
[Critical] R5-6: The mode-changed-file fixture (revert-hunk.test.ts:253) is unrepresentable on Windows — Git for Windows initializes repos with core.filemode=false (NTFS has no POSIX exec bit), so git never emits old mode/new mode lines, the fixture's self-check expect(diffText).toContain('old mode') fails, and the merge-queue Windows job (ci.yml test_windows runs packages/cli vitest with no platform exclusions) goes red while Linux passes. The suite carries zero platform guards; the codebase convention it.skipIf(process.platform === 'win32') is used 110x across packages/cli/src.
Witness:
Modeled probe (core.filemode=false from git init): chmod 755 + git add records no mode change (ls-tree 100644 in both commits); git diff emits 0 old-mode lines; identical fixture with filemode=true emits old mode 100644 / new mode 100755.
中文说明
mode-changed 夹具在 Windows 上无法构造：Git for Windows 以 core.filemode=false 初始化仓库（NTFS 无 POSIX 执行位），git 永远不会输出 old mode/new mode 行，夹具自检 expect(diffText).toContain('old mode') 必然失败；而合并队列的 Windows 任务（ci.yml test_windows，merge_group 触发）无任何平台排除地运行 packages/cli 套件——Linux 绿、Windows 红。仓库惯例 it.skipIf(process.platform === 'win32') 在 packages/cli/src 用了 110 次。修复：给该测试加 win32 跳过（Linux 运行继续钉住头部过滤）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| n: number; | ||
| /** The `@@ ...` header line, verbatim — enough to recognise the hunk. */ | ||
| header: string; | ||
| addedLines: number; |
There was a problem hiding this comment.
[Suggestion] R5-12: --list/the report never disclose that a section also carries a mode change: old mode/new mode lines are stripped from the extracted patch (correctly — the mutation stays content-only), but nothing tells the verifier a mode flip remains in the tree unreverted, and a mode-only section is simply absent from --list.
中文说明
--list/报告从不披露段还携带 mode 变更：old mode/new mode 行被（正确地）从提取补丁中剥掉以保持内容还原纯净，但没有任何信息告诉验证者 mode 翻转仍留在树里未还原；纯 mode 段在 --list 中直接缺席。实测：chmod +x 加一行编辑，还原 run.sh:1 → applied: true、内容回退、执行位仍在、报告对 mode 只字不提——“不 load-bearing”的裁决就此产生。给 HunkEntry 加 modeChange?: boolean（段头含 old mode/new mode 时置位），在 --list 与成功注记中呈现。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Body starts after the `@@` line. `+++`/`---` cannot open a body line | ||
| // that is not itself an add/remove (metadata only exists before the | ||
| // first hunk), so the first byte is the authority. | ||
| for (let ln = h.diffStart + 1; ln <= h.diffEnd; ln++) { |
There was a problem hiding this comment.
[Suggestion] R5-31: listHunks trusts h.diffEnd as the hunk body end, but parseDiff closes the LAST hunk of a file at the next section/EOF — so trailer lines after the final hunk (a git format-patch file's standard '-- '/'version' trailer; arbitrary --diff is explicitly in scope) are absorbed and their leading bytes counted, inflating addedLines/removedLines in --list and the report's hunk entry.
中文说明
listHunks 信任 h.diffEnd 作为 hunk 体的终点，但 parseDiff 把文件的最后一个 hunk 关到下一个 diff --git/EOF——最终 hunk 之后的尾行被吸收进体区间并按行首字节计数：--list 与报告 hunk 条目的 added/removed 虚增。任意 --diff 明确在支持范围内，git format-patch 文件的 '-- '/'2.34.1' 尾注是天然输入；'-- ' 以 '-' 开头：删一行的 hunk 记成 removed: 2（实测：带尾注 removed: 2，纯净 removed: 1；真实 format-patch -1 同样 removed: 2）。git apply 按 @@ 声明计数，还原本身不受影响——错的是验证者选 hunk 所依据的枚举。改为按 @@ 声明的计数推导体区间（走到新旧预算耗尽为止，\ No newline 不消耗预算）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| note: `could not run git in ${tree}: ${check.error ?? `killed by ${check.signal}`} — a harness failure, not a fact about the hunk. Check --tree and that git is on PATH; the tree is unchanged (nothing ran).`, | ||
| }; | ||
| } | ||
| if (check.status === 128) { |
There was a problem hiding this comment.
[Suggestion] R5-35: The exit-128 branch folds git's entire fatal class into one diagnosis — 'git could not operate on ${tree}' with repair advice targeting --tree — but 128 is also what git apply --check returns for a malformed PATCH, where the repairable argument is --diff.
中文说明
exit-128 分支把 git 的整个 fatal 类折进同一诊断——“git 无法操作 ${tree}”并给出指向 --tree 的修复建议——但 128 也是畸形补丁的返回码，此时可修复的参数是 --diff。实测：中途截断的 --diff 使 git apply -R --check 退出 128、'error: corrupt patch at line N'；经真实 runRevertHunk（有效树 + 截断 diff）：note 断言树有问题、把调用方引向 --tree，而截断的 --diff 原样留在原地。退出类正确（harnessFailure → 2）；错的是可操作方向。按 check.stderr 给 128 分类（'corrupt patch'/'unrecognized input' 指向 --diff 重抓取；其余保留树的指引）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| writeFileSync(resolve(out), `${text}\n`, 'utf8'); | ||
| } catch (err) { | ||
| writeStderrLineSafe( | ||
| `revert-hunk: the report was printed above but --out failed: ${(err as Error).message}`, |
There was a problem hiding this comment.
[Suggestion] R4-13: Still standing from round 4: the --out write-failure inner catch in revert-hunk (the 'report was printed above but --out failed' path, revert-hunk.ts:514) is unpinned — no test makes the --out write fail AFTER the report prints.
中文说明
仍立（第 4 轮起）：--out 写入失败的内层 catch（“报告已打印但 --out 失败”，:523）无钉：没有测试让 --out 在报告打印之后失败。删除内层 try/catch 全绿；一次成功还原的退出码会被 EISDIR/ENOSPC 翻转（实测：--out 父级为已存在文件时，突变退出 1 而还原已发生，完整代码退出 0 并打印失败注记）。补一个让 --out 写入在报告打印后失败、断言退出码保持 0 的夹具。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
|
||
| ## Why the witness rule grew to cover Suggestions | ||
|
|
||
| The rule shipped Critical-only, on the argument that a Critical blocks a merge and so carries the highest cost of being wrong. What that scoping missed is that the posting surface does not distinguish: a Suggestion lands on the author's screen as an inline comment on the same terms as a Critical ("Why Suggestion-level findings are posted as inline comments, like Critical", above), so an unexecuted claim rides onto the PR through the Suggestion door with nothing demanding its evidence. The extension closes that door: both postable severities owe a witness or the one-line reason, and the demotion for arriving with neither is the same — low confidence, terminal-only. `Nice to have` stays exempt because it is terminal-only by construction; there is nothing for the rule to hold back. |
There was a problem hiding this comment.
[Suggestion] R5-13: The new DESIGN.md section's cross-reference points '(above)' but the section it names — 'Why Suggestion-level findings are posted as inline comments, like Critical' — sits ~130 lines BELOW (heading at DESIGN.md:769, predating this PR); no same-titled section exists above.
中文说明
新 DESIGN.md 小节的交叉引用指向 “(above)”，但被引用的小节 'Why Suggestion-level findings are posted as inline comments, like Critical' 位于其下方约 130 行（:769，且 merge-base 确认早于本 PR 存在），上方没有同名小节——按指针向上查找一无所获，设计记录的可审计线索在此中断。改为 “below”。
— qwen3.8-max via Qwen Code /review (v0.22.0)
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 — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the unit suites ran locally on Linux only.
Not reviewed: reverse audit — stopped at the 5-round cap without converging (rounds 3, 4 and 5 each reported new findings).
Not explored to full depth (tool budget reached): "agent reverse-audit (round 5)": none — but the @@ -694,7 +696,7 @@ SKILL.md hunk (diff lines ~3956-3959) sits in the preceding chunk and was read only as truncated context; I did not walk it a….
Test Plan (not a blocker): packages/cli/src/utils/findings.test.ts — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/ab-drive.ts:556 — [probe] dead shared.rcPath field — declared, populated, never read; residue of the removed sentinel liveness checkpackages/cli/src/commands/review/ab-drive.ts:765 — [probe] dangling --shared-once/--shared-ready without --shared silently ignored; arms drive against no upstreampackages/core/src/skills/bundled/review/SKILL.md:630 (+2 locations) — [probe] witness-rule prose omits the heldByMeasurement Suggestion exemption the code implementspackages/cli/src/commands/review/ab-drive.test.ts:568 — [probe] per-arm shared-ready bail path pinned only for arm a; mutant flips to observed:truepackages/cli/src/commands/review/ab-drive.test.ts:325 — [probe] once-mode branch of the --shared-cwd use-site re-check unpinnedpackages/cli/src/commands/review/ab-drive.test.ts:524 — [probe] poll-loop completion-vs-overflow check order unpinned; completed >8MB-log arm mutant reports overflowedpackages/cli/src/commands/review/revert-hunk.test.ts:301 — [review] custom-prefix fixture never sanity-checks its capture carries the prefixespackages/core/src/skills/bundled/review/SKILL.md:630 — [review] escape-hatch examples model the old reason-only shape; the named-capability toll goes unpaidpackages/cli/src/commands/review/revert-hunk.test.ts:318 — [probe] disjoint-path concatenated format-patch passes the ambiguity guard; mail headers mis-lexed, counters inflatedpackages/cli/src/commands/review/ab-drive.test.ts:918 — [review] the only exit-2 classification pin sits inside a tmux-gated describe; skipped legs ship the regression greenpackages/cli/src/commands/review.test.ts:62 — [probe] ab-drive is a superstring of drive — the demandCommand message guard's drive entry is vacuously pinnedpackages/cli/src/commands/review/revert-hunk.test.ts:64 — [probe] git fixtures lack isolateHostGitConfig; polluted ambient config (gpgsign) reddens the whole suite deterministicallypackages/core/src/skills/bundled/review/SKILL.md:1025 — [review] the three rewritten witness posting-surface clauses have no SKILL.test.ts pins and no machine backstop
Convergence: round 6 posted 8 inline comment(s), 5 of them reported for the first time. Findings keep coming back to the same files: packages/cli/src/commands/review/revert-hunk.ts (findings in rounds 4, 5; 3 more now); packages/cli/src/commands/review/ab-drive.ts (findings in rounds 3, 4, 5; 2 more now). (Evidence: the previous round's work list was truncated to fit the marker, so the rounds named above may be an undercount, and a new finding written under an earlier round's id cannot be told from a re-post over a partial list, so the new-finding count may be understated.) 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.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the unit suites ran locally on Linux only。
未审查：reverse audit — stopped at the 5-round cap without converging (rounds 3, 4 and 5 each reported new findings)。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 5)"：none — but the @@ -694,7 +696,7 @@ SKILL.md hunk (diff lines ~3956-3959) sits in the preceding chunk and was read only as truncated context; I did not walk it a…。
Test Plan（非阻断）：packages/cli/src/utils/findings.test.ts — no such file or directory。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 13 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 8 条行内评论，其中 5 条是首次提出。发现反复回到同一批文件：packages/cli/src/commands/review/revert-hunk.ts（第 4、5 轮已出过发现，本轮又有 3 条）；packages/cli/src/commands/review/ab-drive.ts（第 3、4、5 轮已出过发现，本轮又有 2 条）。（证据说明：上一轮的工作清单为放进标记而被截断，上述轮次可能少计；在不完整的清单上，冠以早先轮次 id 的新发现无法与重发区分，首次提出的条数可能少计。）一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| for (;;) { | ||
| output = readIfThere(a.logPath); | ||
| const rcText = readIfThere(a.rcPath); | ||
| exitCode = rcText === '' ? null : sentinelExitCode(rcText); |
There was a problem hiding this comment.
[Critical] R3-1: Still standing from round 3 (the round-5 review left it unresolved) — the evidence channels remain path-addressed files the driven code can reach, and the wider capture-forgery channel is open at this commit. Demonstrated anew this round: the poll loop assigns exitCode from the rc file on EVERY iteration before the completion gate, so a timed-out/overflowed arm leaks the — possibly planted — sentinel value into the report beside outcome: 'timed-out', violating the field's documented invariant (null unless the sentinel was reached) and re-leaking the forgery the sentinel-plus-session-gone gate closes. The in-window capture overwrite (the arm's own code overwrites its log via the fd-1-inherited path) and the survivor watch-and-race on arm-b.sh between the write and bash's open are unchanged at this commit: nothing pins the phase files by handle.
A planted rc then reads as a completed exit 0 for an arm that never completed for any consumer branching on exitCode !== null; the in-window overwrite forges the capture bytes an observed: true verdict quotes.
Witness: code read at 8c78a7b18 — exitCode assigned at :600-602 every iteration before the gate (:614-630); independently rediscovered this round by two reverse auditors; no handle-pinning anywhere in readIfThere (:402-412).
Fix: only assign the outer exitCode inside the completion branch, and pin the phase files by handle (open with O_NOFOLLOW after new-session and read via the fd, or compare lstat dev/ino against the creation-time stat on every read, bailing with a tamper note on mismatch).
中文说明
第 3 轮提出、第 5 轮审查明确保留未决：证据通道仍是被驱动代码可触及的按路径寻址文件，更宽的捕获伪造通道在本提交上依然开放。本轮新实证：轮询循环在每轮完成门之前都给 exitCode 赋值，超时/溢出的臂会把（可能被植入的）sentinel 值连同 outcome: 'timed-out' 一起泄漏进报告，违反该字段"非 null 即 sentinel 已达"的文档不变量，把 sentinel+会话消失门已关闭的伪造又从第二个通道漏出。窗口内捕获覆写（臂内代码经 fd 1 继承的路径覆写自己的日志）与幸存进程对 arm-b.sh 的"写入→bash 打开"竞态在本提交上均未变化：阶段文件没有按句柄钉住。修复：只在完成分支内给外层 exitCode 赋值；按句柄钉住阶段文件（O_NOFOLLOW 打开后经 fd 读，或每次读比对 lstat dev/ino，不一致以篡改注记退出）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| header.some( | ||
| (l) => l.startsWith('copy from ') || l.startsWith('rename from '), | ||
| ); | ||
| if (!isMoveOrCopy) return false; |
There was a problem hiding this comment.
[Critical] R5-2: Still standing from round 5 — renameSectionHasUnsupportedPrefix early-returns false for every non-rename/copy section, so PLAIN edit sections captured with non-standard prefixes bypass the gate entirely and reach git apply under default -p1. Rediscovered four independent times this round with empirical reproduction.
A verifier passes an arbitrary --diff whose plain-edit section was captured with git diff --no-prefix (explicitly in scope per the module header): for src/f.txt, git strips src/ and targets root f.txt — with no root f.txt the report fabricates a coupling fact (exit 1); with a matching root f.txt the reverse-apply mutates the WRONG file while the report claims applied: true for src/f.txt:1.
Witness (reproduced this round at HEAD by two independent auditors, re-run by the verifier): scenario A — exit 1 with error: config2: No such file or directory and the coupling note; scenario B — decoy root file mutated = true, real file mutated = false, report applied: true.
Fix: run the standard() token check on the ---/+++ tokens of EVERY section that carries hunks (drop the isMoveOrCopy early return, keeping the /dev/null allowance for genuine creation/deletion sections), routing the refusal through the exit-2 harnessFailure note.
中文说明
第 5 轮提出、仍未修复：renameSectionHasUnsupportedPrefix 对所有非 rename/copy 段早退，非标准前缀捕获的普通编辑段完全绕过门卫、直接进入默认 -p1 的 git apply。本轮被四个独立审计者复现。--no-prefix 捕获的 src/f.txt 普通编辑段：git 剥掉 src/ 指向根级 f.txt——根级不存在时伪造 exit-1 耦合事实；存在同名文件时反向应用改写错误文件而报告仍写 applied: true。修复：对每个含 hunk 的段都跑 standard() 记号检查（去掉 isMoveOrCopy 早退，保留对真创建/删除段的 /dev/null 豁免），以 exit-2 harnessFailure 注记拒绝。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const sharedHeld = | ||
| !args.shared || | ||
| (a?.sharedAliveAtEnd !== false && b?.sharedAliveAtEnd !== false); | ||
| const observed = Boolean(bothCompleted && sharedHeld); |
There was a problem hiding this comment.
[Critical] R5-4: Still standing from round 5 — the arm log-capture channel has no integrity signal: the rm/mkfifo swap that readIfThere's own docstring names is converted from a hang into a silently EMPTY capture that still reaches completed and still compares equal. The pane keeps writing into the original inode, the rc channel is untouched, both arms honestly reach the two-signal completion, readIfThere returns '' — and because both arms run the same bytes the destruction is symmetric: observed: true, identicalOutput: true, a 'the PR changes nothing' verdict manufactured from two destroyed captures, with a note inviting the verifier to quote deciding lines that do not exist. Code read at this commit confirms no integrity check was added by the round-5 fix: completion (:614-630) requires only sentinel + session gone, and this observed gate has no capture-presence conjunct.
Witness: ab-drive.ts:600-630 and :684-690 at 8c78a7b18 contain no capture-integrity conjunct; readIfThere (:402-412) returns '' for absent/non-regular paths, which the completed branch accepts as a capture; the round-5 probe (symmetric log destruction → observed: true, identicalOutput: true) is uncontradicted by any change in the fix commit.
Fix: a minimal integrity check at completion — an absent or non-regular log for a completed arm reports 'unavailable' with a confound note (observed: false, identicalOutput: null).
中文说明
第 5 轮提出、仍未修复：臂日志捕获通道没有完整性信号——readIfThere 文档点名的 rm/mkfifo 替换被从"挂起"变成"静默空捕获"：面板继续写原 inode、rc 通道未动、两臂诚实走完两信号完成门、readIfThere 返回 ''；两臂对称 → observed: true、identicalOutput: true：用两份被销毁的捕获制造"PR 无任何变化"的裁决，注记还邀请验证者引用并不存在的决定性行。本提交代码确认第 5 轮修复未加完整性检查。修复：完成时做最小完整性检查——已完成臂的日志缺失/非常规即以 'unavailable' + 混淆注记报告（observed: false、identicalOutput: null）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // A quoted token is standard only when it quotes an a/ or b/ prefix — a | ||
| // C-quoted path under a CUSTOM prefix (`"x/café"`) starts with `"` too but | ||
| // the rewrite's `"a/`-assuming slice would mangle it. | ||
| const standard = (t: string) => |
There was a problem hiding this comment.
[Critical] renameSectionHasUnsupportedPrefix decides 'standard prefix' syntactically — by whether the ---/+++ tokens' leading bytes are a/ or b/ — instead of verifying the tokens against the section's own unprefixed ground truth (rename from/rename to). A rename-with-edits inside a top-level directory literally named a or b (e.g. a/config → a/config2 — single-letter fixture directories are common) captured with git diff --no-prefix -M reads tokens --- a/config / +++ a/config2 as standard: no refusal fires, the rewrite emits a/config2/a/config2, and git's default strip-1 resolves the target to root config2. The predicate also accepts crossed prefixes (--- b/… with +++ a/…) and accepts /dev/null inside a move/copy section (a hand-edited --diff with rename metadata + --- /dev/null is rewritten into a content edit that truncates the target to empty, applied: true, where the true reverse would delete it).
Witness (real git fixture at HEAD): scenario A — no root config2: exit 1, error: config2: No such file or directory, fabricated coupling note; scenario B — decoy root config2 matching the hunk's new side: applied: true, exit 0, decoy mutated, a/config2 untouched; scratch-tree semantic fix flips both (refusal, both files untouched). /dev/null entrance: variant 1 applied: true with the target truncated to empty; variant 2 fabricated token a/ev/null → exit-1 coupling note.
| const standard = (t: string) => | |
| const fromVal = header.find((l) => l.startsWith('rename from ') || l.startsWith('copy from '))?.replace(/^(rename|copy) from /, ''); | |
| const toVal = header.find((l) => l.startsWith('rename to ') || l.startsWith('copy to '))?.replace(/^(rename|copy) to /, ''); | |
| if (fromVal !== undefined || toVal !== undefined) { | |
| return !( | |
| unquoteCStylePath(tok('--- ')) === `a/${fromVal}` && | |
| unquoteCStylePath(tok('+++ ')) === `b/${toVal}` | |
| ); | |
| } |
(the semantic check compares the tokens against the section's own rename/copy metadata; keep the existing /dev/null allowance only on the non-move/copy path — adapt to the helper names in this file)
中文说明
renameSectionHasUnsupportedPrefix 以记号前缀字节（a//b/）语法化地判定"标准前缀"，而不是对照该段自身无前缀的基准（rename from/rename to）。名字恰好叫 a 或 b 的顶层目录内的带编辑改名（a/config → a/config2）经 git diff --no-prefix -M 捕获后，--- a/config/+++ a/config2 被读作标准记号：不拒绝、重写产出 a/config2/a/config2、git 默认 strip-1 把目标解析到根级 config2。同一谓词还接受交叉前缀与 move/copy 段内的 /dev/null（手编 --diff 可把创建段改写成截空目标的内容编辑，applied: true）。实证：场景 A 伪造 exit-1 耦合事实；场景 B 诱饵文件被改写、真文件未动而报告 applied: true；语义化修复后两者翻转。修复：以段自身的 rename/copy 元数据为基准比对记号（含 C 引号形式），不一致即拒绝。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| .option('diff', { | ||
| type: 'string', | ||
| demandOption: true, |
There was a problem hiding this comment.
[Critical] Usage-level yargs validation failures route through the CLI-wide .fail() (config.ts:979-983: writeStderrLine; yargs.showHelp(); process.exit(1)) BEFORE the handler runs — landing in exit 1, the class this command's contract reserves for a genuine refusal/coupling fact, while the handler's own catch maps the identical repairable class to exit 2. This location: demandOption on --diff (and the --list/--hunk conflict at :463). The sibling entrance in the same PR's ab-drive is reported at its builder.
A verifier script drops --diff (or passes both --list and --hunk): yargs prints usage help and exits 1; a caller keying on the documented taxonomy records a coupling fact about the hunk for a flag omission, burning retries on an error retry cannot fix.
Witness (built CLI at HEAD): qwen review revert-hunk → EXIT=1 Missing required argument: diff; --diff f --list --hunk f.txt:1 → EXIT=1 Arguments list and hunk are mutually exclusive; control mistyped --diff → EXIT=2 not a readable file.
Fix: drop demandOption: true and validate in the handler beside the --hunk/--tree guard (throw TypeError — already maps to exit 2); move the list/hunk exclusion into the same handler guard block.
中文说明
yargs 用法级校验失败经 CLI 全局 .fail()（config.ts:979-983，process.exit(1)）在 handler 之前退出——落入退出码 1（本命令契约保留给真实拒绝/耦合事实的类别），而 handler 自己的 catch 把同样的可修复类映射为退出码 2。此处：--diff 的 demandOption（与 :463 的 --list/--hunk 冲突）。实证（构建后 CLI）：缺 --diff → EXIT=1；--list+--hunk → EXIT=1；对照：打错 --diff → EXIT=2。修复：去掉 demandOption，在 handler 内与 --hunk/--tree 守卫并列校验（抛 TypeError 即归入退出码 2），并把 list/hunk 互斥移入同一守卫块。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| .option('script', { | ||
| type: 'string', | ||
| demandOption: true, |
There was a problem hiding this comment.
[Critical] Same entrance as the revert-hunk finding, in this new command: missing --script/--arm-a/--arm-b (all three demandOption: true) route through the CLI-wide .fail() (config.ts:979-983, process.exit(1)) before the handler runs — the run-failed class — with no JSON report and no ab-drive: note at all, while the handler's own catch declares the repairable class as exit 2 (err instanceof TypeError ? 2 : 1, matching revert-hunk so 'a calling script can tell fix the flags from a run failed').
A verifier script assembles the invocation programmatically and omits --arm-b (e.g. the base-tree path was absent and the flag line was built conditionally): yargs prints usage help and exits 1; a caller keying on the declared contract records the A/B as a failed run, burning retries on an error retry cannot fix.
Witness (built CLI at HEAD): omitting --script / --arm-a / --arm-b one at a time → exit 1, 0 bytes on stdout, no ab-drive: note — all three cases (Missing required argument: script|arm-a|arm-b).
Fix: drop the three demandOption: true and guard in the handler beside assertWritableOutPath (throw TypeError ${flag} is required — already maps to exit 2 in the catch).
中文说明
与 revert-hunk 同型入口出现在本命令：缺 --script/--arm-a/--arm-b（均为 demandOption: true）在 handler 之前死于 CLI 全局 .fail()（exit 1，运行失败类）——没有 JSON 报告、没有 ab-drive: 注记，而 handler 自己的 catch 声明可修复类为退出码 2。实证：三个标志逐一缺失均 exit 1、stdout 0 字节。修复：去掉三个 demandOption，在 handler 内与 assertWritableOutPath 并列抛 TypeError 守卫（catch 已映射为退出码 2）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| } finally { | ||
| tmux('kill-server'); | ||
| rmSync(runDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
[Critical] The finally cleanup can throw and discard the completed report: an untrusted arm script (same uid, learns its phase dir from its own wrapper $0) leaves a mode-000 directory behind (mkdir nodelete; chmod 000 nodelete); both arms complete honestly (observed: true, captures in memory), then rmSync(runDir, {recursive, force}) throws EACCES on the unreadable subdir — force only suppresses ENOENT — the throw propagates out of runAbDrive after the try body finished, the handler catch maps the non-TypeError to exit 1, and the report (both captures, the verdict) is never printed. No adversary needed: any honest fixture leaving a mode-000 directory triggers it. The module's own start() comment names exactly this shape as the failure it designs against ('the handler's generic catch would discard the already-completed capture, the evidence this command exists to preserve') — the finally rmSync simply did not get the same guard.
Witness: node-level probe on Node v22.23.0 — fs.rmSync with force: true THREW EACCES: permission denied, scandir on a chmod-000 subdir; end-to-end runAbDrive — 'THREW after both arms completed: EACCES … scandir …/arm-a-…/nodelete'; scratch-tree try/catch fix flips to 'NO THROW — report observed=true, a/b completed'.
| } finally { | |
| tmux('kill-server'); | |
| rmSync(runDir, { recursive: true, force: true }); | |
| } finally { | |
| tmux('kill-server'); | |
| try { | |
| rmSync(runDir, { recursive: true, force: true }); | |
| } catch { | |
| // An arm that made its phase dir unremovable must not throw away | |
| // the evidence its run produced. | |
| } | |
| } |
中文说明
finally 清理可能抛错并丢弃已完成的报告：不可信臂脚本（同 uid、从自身包装脚本 $0 得知阶段目录）留下 mode-000 目录；双臂诚实完成（observed: true、捕获在内存）后，rmSync 在不可读子目录上抛 EACCES（force 只豁免 ENOENT）——throw 在 try 体完成后传出 runAbDrive，handler 兜底把非 TypeError 映射为 exit 1，报告永远不打印。无需攻击者：任何留下 mode-000 目录的诚实夹具都触发。start() 注释点名的正是这类失败——finally 的 rmSync 却没有得到同样的守卫。实证：Node v22.23.0 上 fs.rmSync force:true 抛 EACCES；端到端双臂完成后抛错；try/catch 修复后翻转。修复：把 rmSync 包进 try/catch（证据优先于清理）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // away (EPIPE from `qwen … | head`) may crash the process into the | ||
| // refused class over a revert that happened. | ||
| writeStdoutLineSafe(text); |
There was a problem hiding this comment.
[Critical] The post-mutation report relies on writeStdoutLineSafe for the EPIPE guarantee its own comment claims ('neither an --out failure nor stdout's reader having gone away (EPIPE from qwen … | head) may crash the process into the refused class over a revert that happened'), but the helper's try/catch only catches SYNCHRONOUS throws while a broken stdout pipe on Node 22 delivers EPIPE as an ASYNCHRONOUS unhandled 'error' event — and revert-hunk registers no process.stdout.on('error') listener, unlike the codebase's own convention (cost-ledger.ts:643-656 documents both EPIPE arrival paths and registers the listeners; nonInteractiveCli.ts:1014 does the same). The helper (stdioHelpers.ts) is pre-existing, but this diff's new handler banks on its contract while asserting the guarantee in a comment — and this command's exit-code semantics make the crash newly harmful: exit 1 is the class a calling script records as a coupling fact.
A revert succeeds (applied: true, exitCode 0 preset) and the reader of stdout is gone — the comment's own qwen … | head shape, or an orchestrator that closes the read end early (probe: even a SMALL payload exits 1 when the reader already exited). The async EPIPE crashes the process with exit 1: the calling script records a fabricated coupling fact about a revert that actually happened, the tree is mutated, the report lost.
Witness (Node v22.23.0): size sweep — 300B exit 0 / 4KB exit 0 / 64KB exit 1 / 131KB-800KB exit 1, stderr throw er; // Unhandled 'error' event … Error: write EPIPE; reader-gone arm — small payload, node exit 1; flip arm with the convention's listener registered — exit 0, 0 stderr bytes.
Fix: make the safe writers actually EPIPE-proof — implement them with fs.writeSync(1, …)/fs.writeSync(2, …) (synchronous throws are caught), or register the stdout/stderr 'error' listeners in the handler as cost-ledger.ts does.
中文说明
变异后的报告依赖 writeStdoutLineSafe 兑现其注释声称的 EPIPE 保证（"--out 失败或 stdout 读者已离开（qwen … | head 的 EPIPE）都不能把已发生的还原推进被拒绝类"），但该帮助函数只捕获同步抛出——Node 22 上管道断裂的 EPIPE 以异步未处理 'error' 事件送达，而 revert-hunk 没有注册 process.stdout.on('error') 监听，与仓库自身约定（cost-ledger.ts:643-656 记录两种 EPIPE 到达路径并注册监听；nonInteractiveCli.ts:1014 同样）不一致。帮助函数虽是存量代码，但本 diff 的新 handler 以其契约为凭并在注释中断言该保证，且本命令的退出码语义使崩溃首次有害：exit 1 正是调用脚本记录为耦合事实的类别。实证：载荷 300B/4KB exit 0，64KB 起 exit 1（stderr Unhandled 'error' EPIPE）；读者已离开时小载荷也 exit 1；注册约定监听后翻转为 exit 0。修复：让 safe writers 真正防 EPIPE（改用 fs.writeSync，或在 handler 注册 stdout/stderr 'error' 监听）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
8c78a7b to
4f5a087
Compare
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 — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the unit suites ran locally on Linux only.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): packages/cli/src/utils/findings.test.ts — no such file or directory.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/core/src/skills/bundled/review/DESIGN.md:1188 (+2 locations) — [review] the measured incident overstates its cited #9131 evidence (N2 was a code reading; the title says 2 notes)packages/cli/src/commands/review/ab-drive.test.ts:933 (+3 locations) — [review] ignoreBrokenPipe unpinned in three places (ordering assertion, helper behaviour, revert-hunk call site)packages/cli/src/commands/review/ab-drive.ts:809 — [review] no test parses ab-drive argv through the real yargs builder; a key-rename mutant ships greenpackages/cli/src/commands/review/ab-drive.test.ts:666 — [review] the best-effort finally teardown added this commit (R6-4's fix) is pinned by no testpackages/cli/src/commands/review/revert-hunk.test.ts:708 — [review] staging-cleanup test never drives the status-128 return; a per-branch-cleanup mutant ships greenpackages/core/src/skills/bundled/review/DESIGN.md:501 — [review] 'extracts verbatim … so what runs is what the diff says' contradicts the header rewrite the same PR implements
Convergence: round 7 posted 13 inline comment(s), 10 of them reported for the first time; the previous round posted 8 (5 new). Findings keep coming back to the same files: packages/cli/src/commands/review/ab-drive.ts (findings in rounds 3, 5, 6; 4 more now); packages/cli/src/commands/review/revert-hunk.ts (findings in rounds 5, 6; 3 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.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the unit suites ran locally on Linux only。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：packages/cli/src/utils/findings.test.ts — no such file or directory。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 7 轮发布了 13 条行内评论，其中 10 条是首次提出；上一轮发布了 8 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/ab-drive.ts（第 3、5、6 轮已出过发现，本轮又有 4 条）；packages/cli/src/commands/review/revert-hunk.ts（第 5、6 轮已出过发现，本轮又有 3 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const standard = (t: string) => | ||
| t === '' || | ||
| t === '/dev/null' || | ||
| t.startsWith('a/') || |
There was a problem hiding this comment.
[Critical] R6-1: Still standing from round 6 — sectionHasUnsupportedPrefix decides 'standard prefix' syntactically (token leading bytes a//b/) instead of verifying the tokens against the section's own unprefixed ground truth (rename from/rename to). A rename-with-edits inside a top-level directory literally named a or b (e.g. a/config → a/config2) captured with git diff --no-prefix --find-renames reads as standard, so no refusal fires; the rewrite then emits a/config2/a/config2 and git's default strip-1 resolves the target to root config2. Crossed prefixes (--- b/… with +++ a/…) and /dev/null inside a move/copy section also still pass. The round-6 fix broadened the gate to every section (closing R5-2) but left the predicate syntactic.
Reproduced at this commit: scenario A (no root config2) → exit 1 with error: config2: No such file or directory and a fabricated coupling note for what is a prefix mis-handling; scenario B (a decoy root config2 matching the hunk's new side) → applied: true, the decoy mutated, the real a/config2 untouched — a wrong-file mutation reported as a content revert.
Witness:
[probe] real git fixture at 4f5a0878: --no-prefix rename in dir 'a' passes the gate;
scenario A -> applied:false, fabricated coupling note;
scenario B -> applied:true, decoy mutated, a/config2 untouched;
quote-aware semantic fix flips both to refusal
Fix: compare the tokens against the section's own rename from/rename to metadata (quote-aware — the metadata is itself C-quoted for quoted paths); keep the /dev/null allowance only on the non-move/copy path.
中文说明
第 6 轮提出、仍未修复：sectionHasUnsupportedPrefix 以记号前缀字节（a//b/）语法化地判定"标准前缀"，而不是对照该段自身无前缀的基准（rename from/rename to）。名字恰好叫 a 或 b 的顶层目录内的带编辑改名（a/config → a/config2）经 git diff --no-prefix --find-renames 捕获后被读作标准记号：不拒绝、重写产出 a/config2/a/config2、git 默认 strip-1 把目标解析到根级 config2；交叉前缀与 move/copy 段内的 /dev/null 也同样通过。第 6 轮修复把门卫扩到了每个段（关闭了 R5-2），但谓词仍是语法化的。本提交实证：场景 A 伪造 exit-1 耦合事实；场景 B 诱饵文件被改写、真文件未动而报告 applied: true。修复：以段自身的 rename/copy 元数据为基准比对记号（需处理 C 引号形式），/dev/null 豁免只保留给非 move/copy 段。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| .option('diff', { | ||
| type: 'string', | ||
| demandOption: true, |
There was a problem hiding this comment.
[Critical] R6-2: Still standing from round 6 — usage-level yargs validation failures route through the CLI-wide .fail() (config.ts:979-983: writeStderrLine; showHelp; process.exit(1)) BEFORE the handler, landing in exit 1 — the class this command's contract reserves for a genuine refusal/coupling fact — while the handler's own catch maps the identical repairable class to exit 2. This location: demandOption: true on --diff and the .conflicts('list', 'hunk') guard.
A verifier script that drops --diff (or passes both --list and --hunk) gets usage help and exit 1; a caller keying on the documented taxonomy records a coupling fact about the hunk for a flag omission, burning retries on an error retry cannot fix.
Witness:
[probe] PR-built CLI at 4f5a0878:
no --diff -> EXIT=1, stdout 0 bytes, 'Missing required argument: diff'
--list + --hunk -> EXIT=1, 'Arguments list and hunk are mutually exclusive'
--diff /no/such.diff -> EXIT=2, 'not a readable file' (same repairable class via the handler)
| .option('diff', { | |
| type: 'string', | |
| demandOption: true, | |
| .option('diff', { | |
| type: 'string', |
(drop demandOption and validate in the handler beside the --hunk/--tree guard — throw TypeError, already exit 2; move the list/hunk exclusion into the same guard block)
中文说明
第 6 轮提出、仍未修复：yargs 用法级校验失败经 CLI 全局 .fail()（config.ts:979-983，process.exit(1)）在 handler 之前退出——落入退出码 1（本命令契约保留给真实拒绝/耦合事实的类别），而 handler 自己的 catch 把同样的可修复类映射为退出码 2。此处：--diff 的 demandOption 与 --list/--hunk 冲突。实证（构建后 CLI）：缺 --diff → EXIT=1、stdout 0 字节；--list+--hunk → EXIT=1；对照：打错 --diff → EXIT=2。修复：去掉 demandOption，在 handler 内与 --hunk/--tree 守卫并列校验（抛 TypeError 即归入退出码 2），并把 list/hunk 互斥移入同一守卫块。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| .option('script', { | ||
| type: 'string', | ||
| demandOption: true, |
There was a problem hiding this comment.
[Critical] R6-3: Still standing from round 6 — same entrance as R6-2 in this command: missing --script/--arm-a/--arm-b (all three demandOption: true) route through the CLI-wide .fail() (config.ts:979-983, process.exit(1)) before the handler runs — the run-failed class — with no JSON report and no ab-drive: note, while the handler's own catch declares the repairable class as exit 2.
A verifier script that assembles the invocation programmatically and omits --arm-b (e.g. the base-tree path was absent and the flag line was built conditionally) gets usage help and exit 1; a caller keying on the declared contract records the A/B as a failed run, burning retries on an error retry cannot fix.
Witness:
[probe] PR-built CLI at 4f5a0878: omitting each of --script / --arm-a / --arm-b
-> EXIT=1, stdout 0 bytes, 'Missing required arguments: …' — no report, no ab-drive: note
Fix: drop the three demandOption: true and guard in the handler beside assertWritableOutPath (throw TypeError ${flag} is required — already maps to exit 2 in the catch).
中文说明
与 revert-hunk 同型入口（R6-2）出现在本命令，第 6 轮提出、仍未修复：缺 --script/--arm-a/--arm-b（均为 demandOption: true）在 handler 之前死于 CLI 全局 .fail()（exit 1，运行失败类）——没有 JSON 报告、没有 ab-drive: 注记，而 handler 自己的 catch 声明可修复类为退出码 2。实证：三个标志逐一缺失均 exit 1、stdout 0 字节。修复：去掉三个 demandOption，在 handler 内与 assertWritableOutPath 并列抛 TypeError 守卫（catch 已映射为退出码 2）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if ( | ||
| exitCode !== null && | ||
| tmux('has-session', '-t', `arm-${arm}`).status !== 0 | ||
| ) { |
There was a problem hiding this comment.
[Critical] R7-1: Class-level finding collapsing ledger R3-1 and R5-4 (same-uid tampering with path-addressed phase files — please file no siblings of this class). The arm log-capture channel has no integrity signal at completion: the completion gate requires only sentinel + session gone, and observed = bothCompleted && sharedHeld has no capture-presence conjunct. readIfThere ALREADY detects a non-regular file (its fstatSync(fd).isFile() FIFO defense) but silently returns '' — the detection is discarded. A driven arm that rm/mkfifo-swaps or deletes its own log reaches completed with an EMPTY capture; because both arms run the same bytes the destruction is symmetric: observed: true, identicalOutput: true — a fabricated 'the PR changes nothing' verdict, with a note inviting the verifier to quote deciding lines that do not exist. The benign trigger (the same script crashing before any output on both arms) reaches the same verdict with no adversary. Folded entrance (evidence, not a separate finding): the write→open race on arm-b's script file — a setsid survivor of arm a's teardown can rewrite arm-b.sh between writeFileSync and bash's open, falsifying the scriptSha256 same-bytes fact the report vouches (part of ledger R3-1's channel). The ACCEPTED RESIDUAL RISK comment argues PREVENTION (needs an OS boundary, true); this finding's ask is DETECTION, which the harness already does elsewhere (session-gone for planted sentinels, liveness for shared plants, realpath re-checks for root swaps).
Witness:
[probe] harnessed run through the exec seam at 4f5a0878, both arms rm their own log:
PR -> {"observed": true, "identicalOutput": true, "aOutput": "", "bOutput": ""}
fix -> observed:false, identicalOutput:null (completed arm whose log is absent/non-regular
reported 'unavailable' with a confound note)
Fix: the minimal integrity conjunct at completion — a completed arm whose log is absent or non-regular reports unavailable with a confound note (observed: false, identicalOutput: null); stop discarding the isFile() detection.
中文说明
类级发现，合并台账 R3-1 与 R5-4（同 uid 下对按路径寻址的阶段文件的篡改——请勿再报此类的兄弟条目）：臂日志捕获通道在完成时没有完整性信号——完成门只要求 sentinel+会话消失，observed 门没有"捕获存在"合取项；readIfThere 本就检测非常规文件（其 FIFO 防御 fstatSync(fd).isFile()）却静默返回 ''，把检测结果丢弃。被驱动臂删除/rm+mkfifo 自己的日志后仍以空捕获到达 completed；两臂同字节 → 破坏对称 → observed: true、identicalOutput: true：用两份被销毁的捕获制造"PR 无任何变化"的裁决，注记还邀请引用并不存在的决定性行。无需攻击者的良性触发（两臂脚本都在输出前崩溃）同样到达该裁决。合并入口（证据而非独立发现）：arm-b.sh 的"写入→打开"竞态——arm a 收尾后幸存的 setsid 进程可在 writeFileSync 与 bash 打开之间改写脚本，伪造报告背书的 scriptSha256 同字节事实（属 R3-1 通道）。残留风险注释论证的是"预防"（需 OS 边界，成立）；本条要求的是"检测"——harness 在别处已经这样做（planted sentinel 的会话消失门、shared 活性、realpath 复检）。实证：双臂各自删除日志 → observed:true、identicalOutput:true；加上最小完整性合取后翻转。修复：完成臂日志缺失/非常规即以 unavailable+混淆注记报告（observed:false、identicalOutput:null）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(h.events()).not.toContain('new:arm-b'); | ||
| }); | ||
|
|
||
| it('re-validates --shared-cwd for a revoked search bit, not only deletion', () => { |
There was a problem hiding this comment.
[Critical] R7-2: The mid-run --shared-cwd search-bit test has NO platform/uid gate, and its pre-flight sibling (:264) has it.skipIf(process.platform === 'win32') but omits the repo convention's root clause. The harnessed describe is collected by every CI job (only the real-tmux describes are gated), including the merge-queue test_windows. On Windows chmodSync(dir, 0o000) only toggles the read-only attribute and libuv's accessSync(dir, X_OK) succeeds for any existing directory, so isDir() returns true, no bail fires, and this test's own assertions fail; as root (root bypasses mode bits) the :264 test fails the same way. The repo convention for exactly this shape is it.skipIf(process.platform === 'win32' || process.getuid?.() === 0) — cost-ledger.test.ts:1864 documents why. This is the residue of round-5 R5-7, whose pre-flight entrance gained the win32 skip.
Witness:
[probe] at 4f5a0878 (Linux, non-root): baseline 79 passed;
modeled Windows/root arm (chmod hook a no-op w.r.t. X_OK):
AssertionError: expected 'completed' to be 'unavailable' — the test's own assertions fail;
under the convention's gate the test reports skipped
| it('re-validates --shared-cwd for a revoked search bit, not only deletion', () => { | |
| it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)('re-validates --shared-cwd for a revoked search bit, not only deletion', () => { |
(and add the root clause to the :264 sibling)
中文说明
中途复检 --shared-cwd 搜索位的测试没有任何平台/uid 门；其预检兄弟测试（:264）有 skipIf(win32) 但缺仓库约定的 root 子句。该 harnessed describe 被所有 CI 作业收集（只有真 tmux describe 有门），包括合并队列的 test_windows。Windows 上 chmodSync(dir, 0o000) 只切换只读属性，libuv 的 accessSync(dir, X_OK) 对任何存在目录都成功，于是 isDir() 返回 true、不触发 bail，本测试自身断言失败；root 下（root 绕过模式位）:264 测试同样失败。仓库对同型夹具的约定是 it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)（cost-ledger.test.ts:1864 有说明）。这是第 5 轮 R5-7 的残留（其预检入口已加 win32 跳过）。实证：POSIX 非 root 基线 79 通过；Windows/root 模型下本测试自身断言失败；加上约定门后变为跳过。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| .option('server', { | ||
| type: 'string', | ||
| default: `qr-ab-${process.pid}`, |
There was a problem hiding this comment.
[Critical] R7-6: The verdict gate's trust anchor is the tmux SERVER, and the driven arm owns it: $TMUX (exported into every session) hands the arm the socket path; a same-uid arm can kill-server and re-host the same socket (new-session auto-starts a replacement server on the existing socket), then answer every has-session the observed gate consumes — including exact-match = targets, so R7-4's fix alone does not close this channel. The planted-rc reachability is the one R7-1 established (the wrapper cmdline carries the path). The completion-gate comment concedes socket reachability ('tamper-proofing impossible at one uid') — that argues prevention; this finding's ask is DETECTION (identity pinning), the same pattern confirmed for R7-1 and R7-4.
Witness:
[probe] live on tmux 3.4 at 4f5a0878, race-free choreography:
CONTROL (same bytes, no attack) -> observed=false, both arms timed-out
ATTACK -> 6/6 trials forged observed:true (a=completed/exit0/~1.5s droveForMs,
honest sleep-60 body never ran; liveness answered by the re-hosted server)
#{server_pid} expands EMPTY on tmux 3.4 and the socket inode does NOT change
across re-host (neither usable as a witness); $TMUX inside a pane carries the
server pid — an identity-witness fix is implementable
Fix: capture a server-identity witness at keeper creation — one short-lived session whose command writes $TMUX (the middle comma-field is the server pid) — and re-verify it before recording sharedAliveAtEnd/completion and before driving arm b; on mismatch return a harness failure (observed: false, note naming the server replacement) instead of consuming the answers.
中文说明
裁决门的信任锚是 tmux 服务器本身，而被驱动臂拥有它：$TMUX 把 socket 路径交给臂脚本；同 uid 的臂可以 kill-server 后在同一 socket 上重新托管（new-session 会自动在原 socket 上拉起替代服务器），此后 observed 门消费的每个 has-session 都由攻击者应答——包括 = 精确匹配目标，因此仅靠 R7-4 的修复关闭不了本通道。完成门注释承认 socket 可及（"同 uid 下无法做到防篡改"）——那论证的是预防；本条要求的是检测（身份钉住），与 R7-1/R7-4 同一模式。实证（tmux 3.4）：对照 observed=false；攻击 6/6 次伪造 observed:true；#{server_pid} 在 tmux 3.4 展开为空、socket inode 在重托管后不变（均不可作证据），而面板内 $TMUX 携带服务器 pid——身份见证修复可实现。修复：keeper 创建时以短命会话写出 $TMUX（中间逗号字段即服务器 pid）作为身份见证，在记录活性/完成与驱动 arm b 之前复检，不一致即以 harness 失败返回。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // overflowed writer keeps growing its log through arm b's whole | ||
| // window. Killing a session that already exited on completion is a | ||
| // no-op, so this is unconditional. | ||
| tmux('kill-session', '-t', `arm-${arm}`); |
There was a problem hiding this comment.
[Critical] R7-7: The harness equates tmux-session lifecycle with process lifecycle: its only kill mechanisms (kill-session, kill-server) deliver SIGHUP to the pane's process group only, so a --shared script using the standard daemon idioms (setsid, nohup) escapes every teardown — with NO adversary involved (the shared script is cooperative). Demonstrated at this commit: (1) per-arm default — arm a's escaped daemon answers arm b's readiness probe after shared-b fails EADDRINUSE, so arm b is driven against arm a's upstream and BOTH captures show arm a's content while the report vouches observed: true, mode per-arm — a false difference attributed to the PR, or identicalOutput: true manufacturing 'the PR changes nothing'; (2) the escaped daemon leaks into the NEXT ab-drive run holding its port → false not-ready blaming the shared script. This contradicts the file's own header promise that the shared process is 'killed unconditionally'; the ACCEPTED RESIDUAL RISK comment argues adversarial capture-corruption, not this. (The mirror direction — nohup daemon & exit 0 reporting a factually false dead upstream — did not reproduce on this host and is not claimed.)
Witness:
[probe] live through the real command at 4f5a0878, daemonizing shared upstream:
PROBE1 observed=true, BOTH arms' outputs 'MARKER-ARM-A' (arm b served by arm a's
escaped daemon); escaped daemon alive after kill-server = true
run 2 with fresh arms -> still served 'MARKER-ARM-A' (run 1's leak)
fix (snapshot pane_pid + /proc descendants, kill the tree BEFORE kill-session):
arm b output 'MARKER-ARM-B', no leak after kill-server, run-2 arms serve their own trees
Fix: kill the process TREE, not the session — before each kill-session, resolve the pane pid (tmux display-message -p -t <s> '#{pane_pid}'), kill its descendant tree (walk /proc/<pid>/task/*/children, escalating to SIGKILL), then kill the session; state in the --shared describe that background-start shapes report as dead-upstream because liveness is the session.
中文说明
harness 把 tmux 会话生命周期等同于进程生命周期：仅有的击杀手段（kill-session、kill-server）只向面板进程组发 SIGHUP，使用标准守护写法（setsid、nohup）的 --shared 脚本能逃过所有收尾——无需攻击者（共享脚本是合作的）。本提交实证：(1) 默认 per-arm——arm a 逃逸的守护进程在 shared-b 因 EADDRINUSE 失败后应答 arm b 的就绪探测，arm b 被对着 arm a 的上游驱动，两份捕获都是 arm a 的内容而报告背书 observed:true（把虚假差异归因于 PR，或 identicalOutput:true 制造"PR 无变化"）；(2) 逃逸守护进程带着端口泄漏进下一次运行 → 虚假 not-ready 且注记责怪共享脚本。这与文件头部"无条件击杀共享进程"的承诺矛盾；残留风险注释论证的是对抗性捕获破坏，不是本条。（镜像方向——nohup daemon & exit 0 报虚假"上游已死"——在本机未复现，不作主张。）实证：修复（击杀面板进程树后再 kill-session）后各臂读到各自树的内容、无泄漏。修复：每次 kill-session 前解析 pane_pid 并击杀其后代树。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Confirmed still standing on the current head 31665a87, with a real-tmux run through the built runAbDrive. This is reachable with a cooperative daemon script, not same-uid tampering: the per-arm shared script used nohup python3 -m http.server <port> --bind 127.0.0.1 & and then kept its wrapper alive. The result was:
{
"observed": true,
"mode": "per-arm",
"a": { "output": "ARM-A\n", "sharedAliveAtEnd": true },
"b": { "output": "ARM-A\n", "sharedAliveAtEnd": true },
"identicalOutput": true
}Arm A’s detached server survived kill-session, satisfied arm B’s readiness probe after the B-side server could not bind, and served A content to both arms. The report therefore vouches for the exact false comparison that fresh-per-arm mode exists to prevent. Please either make teardown own daemonized descendants, or narrow/enforce the contract so this shape cannot produce observed: true. A regression test should drive a daemonizing shared upstream through real tmux and prove arm B cannot be marked observed from arm A’s surviving process, with no child left after teardown.
| } finally { | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
[Critical] R7-8: The staging-sweep rmSync in runRevertHunk's finally is bare; force: true suppresses only ENOENT, so an EACCES — a same-uid peer chmods the mkdtemp dir 0o500 while git apply runs (the dir name is unguessable, but dirname(patchPath) rides in git's own argv) — throws out of the finally and displaces the already-computed return, including applied: true. The handler catch maps non-TypeError to exit 1 with stderr only — the refusal/coupling class, no JSON — while the tree IS reverted. The caller records a coupling refusal and probes the wrong half of the witness pair. The same commit hardened ab-drive's teardown against this exact shape ('Neither teardown may throw out of the finally and discard the report') but left this sweep bare.
Witness:
[probe] deterministic via the code's own exec seam at 4f5a0878 (real git;
on apply success, chmodSync(dirname(patchPath), 0o500)):
PR -> tree reverted, but runRevertHunk threw EACCES (report null) — exit-1 class
fix -> try/catch around the sweep: report returned applied:true; suite 34/34 green
| } finally { | |
| rmSync(dir, { recursive: true, force: true }); | |
| } | |
| } finally { | |
| try { | |
| rmSync(dir, { recursive: true, force: true }); | |
| } catch { | |
| // a same-uid peer can make the staging dir un-removable; never let the sweep displace the report | |
| } | |
| } |
中文说明
runRevertHunk 的 finally 清场 rmSync 没有守卫：force: true 只豁免 ENOENT，EACCES（同 uid 对等在 git apply 运行期间把 mkdtemp 目录 chmod 0o500——目录名不可猜，但 dirname(patchPath) 就在 git 自己的 argv 里）会从 finally 抛出，顶掉已经算好的返回值（包括 applied: true）。handler 兜底把非 TypeError 映射为 exit 1 且只有 stderr——拒绝/耦合事实类、没有 JSON——而树确实已被还原。调用方因此记录一次"耦合拒绝"并去探测错误的半个见证对。同一提交给 ab-drive 的收尾加了同样形状的防护（"任何收尾都不得从 finally 抛出并丢弃报告"），本处清场却没有。实证：确定性探针下未修复版抛 EACCES（报告丢失），包上 try/catch 后报告正常返回且全套 34/34 绿。修复：给清场包 try/catch（证据优先于清理）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // purpose: a finding re-raised to Critical through the note's own | ||
| // "file it at Critical again" door still carries the marker, and it | ||
| // must face the witness rule like any other unexecuted Critical. | ||
| (f.heldByMeasurement !== undefined && f.severity === 'Suggestion') || |
There was a problem hiding this comment.
[Critical] R7-9: The measurement-hold exemption returns the finding unmodified, so a witness-rule confidence demotion that PREDATES the measurement demotion survives it — across rounds the two holds compose into exactly the silent drop the exemption's own comment says it exists to prevent. Round N: an unwitnessed review-source Critical is demoted to confidence low by holdUnwitnessedFindings (severity stays Critical); the artifact round-trips through --input. Round N+1: holdCriticalsFailingOnBase demotes Critical→Suggestion via a spread that never touches confidence (stays low) and sets heldByMeasurement; the exemption returns the finding unmodified at confidence low → isPostable false → no anchor, no comment — while stderr prints 'held back from Critical' and the hold's contract promises it STAYS in front of a human as a posted Suggestion whose note says how to re-raise it. The identical finding measured in the SAME invocation (still high when the measurement demotes) IS posted — PR visibility of the same evidence state depends solely on which round each hold ran in.
Witness:
[probe] real functions run in cross-round order at 4f5a0878:
BASE -> AFTER-MEASUREMENT {severity:Suggestion, confidence:low, heldByMeasurement:{…}},
FINAL confidence:low, ANCHOR-REQUESTS []
FIX -> restore confidence:high in the measurement demotion:
FINAL confidence:high, ANCHOR-REQUESTS [1 entry]
contrast: measurement in the same invocation stays high with 1 anchor request
Fix: restore confidence: 'high' when the measurement demotion lands — add it to holdCriticalsFailingOnBase's demotion return — and pin the cross-round ordering with a test (witness-demotion first, then measurement hold, then witness hold again, expecting a high-confidence posted Suggestion).
中文说明
measurement-hold 豁免按原样返回发现，于是先于 measurement 降级发生的 witness 规则置信度降级会存活下来——跨轮时两个 hold 恰好合成豁免注释声称要防止的静默丢弃。第 N 轮：无见证的 review 来源 Critical 被 holdUnwitnessedFindings 降为 confidence low（severity 仍是 Critical），产物经 --input 回传；第 N+1 轮：holdCriticalsFailingOnBase 以不触碰 confidence 的展开把它降为 Suggestion 并设置 heldByMeasurement；豁免按原样返回（confidence 仍 low）→ isPostable 为 false → 没有锚点、没有评论——而 stderr 打印了 'held back from Critical'，hold 的契约承诺它会以"带恢复说明的已发布 Suggestion"留在人面前。同一次调用内被测量的同样发现（测量降级时仍是 high）却会被发布——同一证据状态是否上 PR 只取决于两个 hold 各在哪一轮运行。实证：真实函数跨轮序运行，基线最终 confidence:low、锚点请求为空；修复（测量降级时恢复 confidence:high）后锚点请求 1 条。修复：在 holdCriticalsFailingOnBase 的降级返回中恢复 confidence: 'high'，并加跨轮顺序测试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const dir = mkdtempSync(join(tmpdir(), 'qwen-review-revert-hunk-')); | ||
| const patchPath = join(dir, 'hunk.patch'); |
There was a problem hiding this comment.
[Critical] R7-10: The extracted patch sits on disk at a FIXED name in a same-uid-writable tmpdir across two sequential git apply reads; mkdtemp closes PRE-planting only (the comment's 'nothing else can have claimed' is about the name, not post-creation access). A same-uid peer — the local-peer class this PR's own residual boundary names — enumerating qwen-review-revert-hunk-* can replace hunk.patch with a different patch that also passes --check (the named hunk plus extra edits); the swap lands before or between the two exec calls with no narrow window needed. The report returns applied: true naming hunk f.ts:1 while the tree holds a DIFFERENT mutation — the exact 'different mutation than the one the report claims' class the file header says the command exists to eliminate.
Witness:
[probe] at 4f5a0878, real git (exec seam used only to time the swap deterministically):
BASE -> report {applied:true, hunkId:'f.txt:1'} while the tree shows BOTH hunks
reverted (the decoy patch was the full diff)
FIX -> re-read patchPath after apply, compare with the in-memory patch,
harnessFailure on mismatch: {applied:false, harnessFailure:true}; suite 34/34 green
Fix: remove the file channel — feed the patch to git over stdin (spawnSync('git', ['apply', '-R', '--check', '-'], { input: patch }), same for the apply call); an in-memory string cannot be swapped. If the on-disk shape must stay, re-read patchPath after the apply and compare against the in-memory patch, reporting harnessFailure on any mismatch.
中文说明
抽取出的补丁以固定文件名落在同 uid 可写的 tmpdir 里，跨越两次串行的 git apply 读取；mkdtemp 只关闭"预植入"（注释里"没有别的东西能占用"说的是名字，不是创建后的可访问性）。同 uid 对等方——本 PR 自身残留边界点名的本地对等方——枚举 qwen-review-revert-hunk-* 即可把 hunk.patch 换成另一份同样能通过 --check 的补丁（指定 hunk 外加额外编辑），交换落在两次 exec 调用之前或之间，无需窄窗口。报告以 applied: true 指名 hunk f.ts:1，树里却是另一份变异——正是文件头部声称本命令要消灭的"与报告声称不同的变异"一类。实证：基线报告 applied:true 而树被还原了两个 hunk（诱饵是完整 diff）；修复（apply 后重读比对、不一致即 harnessFailure）后翻转，全套 34/34 绿。修复：改经 stdin 传补丁（内存字符串无法被交换），或 apply 后重读比对。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /triage |
|
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: 166 passed · 0 failed · 166 total Flakiness gate: ✅ 5 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：166 通过 · 0 失败 · 166 总计 抖动门：✅ 5 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9740 deep verification —
|
| cell | input | head (expected → actual) | base (expected → actual) |
|---|---|---|---|
| F1 | Suggestion, no witness | demoted to low + stderr line ✓ |
stays high ✓ (control red as predicted) |
| F2 | Critical, witness: "not run —" (empty reason) |
demoted ✓ | stays high ✓ (base accepts any witness string) |
| F3 | Critical, not run — no X server in container |
stays high ✓ |
stays high ✓ |
| F4 | Suggestion + heldByMeasurement, no witness |
stays high (exempt) ✓ |
stays high ✓ |
| F5 | Nice to have, no witness |
stays high (exempt) ✓ |
stays high ✓ |
| F6 | Critical, source: test, no witness |
stays high (deterministic source) ✓ |
stays high ✓ |
| F7 | Suggestion WITH a witness | stays high (no over-reach) ✓ |
stays high ✓ |
| F8 | Suggestion, witness: "not runnable" (word-continuation) |
demoted ✓ | stays high ✓ |
| F9 | head's demoted F1 re-fed into head | idempotent: stays low, no double demotion, rule sentence ×1 ✓ |
— |
A 21-shape boundary ladder on the compiled isEmptyNotRunWitness + the hold (witness-boundary.mjs, 25/25) confirmed the Unicode claims: CJK (not run — 无可用环境) and Vietnamese reasons stay witnesses; every dash species (em, U+2015, U+2212, U+FF0D, ASCII) with nothing after counts as empty; punctuation-only reasons count as empty. Scaling: 1000×20 000-char hostile inputs = 261 ms (linear, no superlinear rung).
ab-drive — live cells on the built head CLI (real tmux)
The base side of this claim is "the subcommand does not exist at base" (verified: no ab-drive.js in the base build), so load-bearing proof is live behaviour + mutation M2 rather than a two-cell diff. Witness: 02-ab-drive-live-real-tmux.png (41/41).
| cell | observable oracle | result |
|---|---|---|
| L1 per-arm shared upstream | both arms completed, observed: true, each arm saw its own tree (AAA/BBB) and a distinct upstream instance, AB_ARM/AB_ARM_ROOT exported, cwd = arm root |
✓ |
L2 --shared-once + --shared-cwd |
one upstream instance observed by both arms, both trees still distinct, observed: true |
✓ |
| L3 shared dies at birth | observed: false, sharedAliveAtEnd: false on both arms, exit 1, note names the dead upstream |
✓ |
| L4 guards (same dir both arms / empty script / empty arm path / missing dir / file-as-arm / bad server name / NaN timeout) | all refused with observed: false and a named reason, nothing started |
✓ 7/7 |
| L5 residue | tmux server gone (tmux ls fails), zero qwen-review-ab-drive-* temp dirs |
✓ |
A tuned follow-up cell (logs/once-death.mjs): in once mode with the shared process dying between the arms, the report comes back observed: false, exit 1, arm A's completed capture preserved in the report — and the early-bail path (stop) only fires before any arm is driven, so the "half an A/B still tells you where the harness needs repair" property holds (a scarier consequence I checked and could NOT confirm — no evidence-loss shape found).
revert-hunk — live cells on a fixture repo (built head CLI)
Witness: 03-revert-hunk-live-fidelity.png (55/55). Fixture: a real two-commit git repo with an 8-section diff (two-hunk file, no-trailing-newline, CRLF, latin-1 0xE9, colon filename, space filename, rename+edit), scratch worktree at the PR-head commit.
| cell | oracle | result |
|---|---|---|
R1 --list |
ids a.txt:1…space name.txt:1, @@ headers verbatim incl. git's funcname suffix, per-hunk +/− counts, tree untouched |
✓ |
R2 revert a.txt:1 |
top edit reverted, bottom edit kept, exit 0 | ✓ |
| R3 double revert | refused by --check, exit 1, conflict carries git's text, tree byte-identical (sha256 over all files) |
✓ |
| R4 tree mutated in hunk context | refusal class (exit 1, no harnessFailure), tree NOT further mutated; contrast: same hunk applies after reset |
✓ |
R5 \ No newline file |
reverted file byte-identical to the base blob | ✓ |
| R6 CRLF file | reverted file byte-identical (CRLF preserved) | ✓ |
R7 latin-1 0xE9 |
reverted file byte-identical — the 0xE9 survives diff→patch→apply, no U+FFFD | ✓ |
R8 colon:name.txt:1, space name.txt:1 |
both revert (id splits from the right) | ✓ |
| R9 rename-with-edits | content reverted at the new path; old path not resurrected (no move) | ✓ |
R10 --no-prefix / custom-prefix diffs |
refused exit 2, note names the prefixes, tree untouched | ✓ |
| R11 exit-code discipline | malformed selector / nonexistent hunk / missing --diff / missing --tree / ambiguous concatenated diff all exit 2 (never 1); --list vs --hunk conflict enforced |
✓ |
| R12 residue | zero staging dirs created by this run | ✓ |
Note on R7's journey: my first harness captured the fixture diff with encoding: 'utf8' — itself the exact mangling the command's latin1 design exists to avoid — and the revert then rightly failed. A buffer-faithful standalone probe (logs/latin-probe.mjs) proved the command correct; the fixed harness passes. Reported so the audit trail is honest.
Brief rendering + skill docs
agent-prompt's verify brief carries the ab-drive block, the revert-hunk paragraph, the extended witness sentence and the incidental-findings section — pinned by agent-prompt.test.ts (green in the gate), and M4 below proves the pin is live. SKILL.md/DESIGN.md/posting.md prose was checked against measured behaviour where testable (fresh-per-arm default, coupling facts, both postable severities) and matches.
Mutation matrix — the new tests are not vacuous
One point mutation per guard, applied in a scratch worktree at the verified head, reverted after each row; same tree unmutated as the control. Witness: 04-mutation-matrix-all-killed.png; raw vitest logs logs/m1.log–m4.log, logs/control.log.
| mutant | suite that caught it | kill evidence (quoted) |
|---|---|---|
| M1 findings: severity condition back to Critical-only | findings.test.ts |
judges Suggestions on the same terms → AssertionError: expected 'high' to be 'low' (1 failed / 11 witness tests still green) |
M2 ab-drive: sharedHeld = true |
ab-drive.test.ts |
3 tests red: a shared process that dies before its arm finishes fails the observed gate (+ SIGKILL'd-upstream and asymmetric cases) → expected true to be false |
| M3 revert-hunk: metadata filter disabled | revert-hunk.test.ts |
4 tests red: rename/mode/copy/C-quoted rename → expected '…' not to contain 'rename from' |
| M4 agent-briefs: witness sentence reverted | agent-prompt.test.ts |
expected '…' to contain 'A confirmed Critical returns its witn…' (1 failed / 298 passed) |
| control | same three files, unmutated | 195 passed, 8 skipped (tmux-gated), 0 failed |
No survivors — nothing to adjudicate. The side note worth keeping: reverting ONLY the severity clause (M1) also breaks tsc (TS2367: '"Critical"' and '"Suggestion"' have no overlap on the heldByMeasurement exemption), i.e. the type system independently pins the two-clause shape.
Targeted gates
| gate | result |
|---|---|
packages/cli review suite (npx vitest run src/commands/review) at HEAD |
5053 passed | 12 skipped | 0 failed, 104 files |
packages/core SKILL.test.ts at HEAD |
40/40 passed |
literal test-plan command npx vitest run src/commands/review src/utils/findings.test.ts |
exits 0, same 5053-pass result (see nit 2 — the second path matches nothing) |
repo-wide npm run typecheck at HEAD |
exit 0 (ran after the gates, logs/typecheck.log) |
The 8 skipped tests in ab-drive.test.ts are the two describe.skipIf(!hasTmux) "driven for real" blocks: this container ships no tmux and the verify lane installs none, so they skip in-lane. I installed tmux 3.3a into a user prefix (debs extracted with dpkg -x, no root) and covered those shapes live — L1/L2/L3 above mirror the block's three scenarios (distinct arms, per-arm shared, upstream dying at birth → observed: false).
Findings
Nit 1 — revert-hunk.test.ts leaks one staging dir into the system temp dir on every run
Repro: run cd packages/cli && npx vitest run src/commands/review/revert-hunk.test.ts, then ls -d $TMPDIR/qwen-review-revert-hunk-*.
Measured: after the gate's first run of this suite (and before any of my harnesses had invoked the command) exactly one qwen-review-revert-hunk-* dir existed, containing the twoHunkFixture patch (top-old/top-new, timestamp matching the gate run). Cause: the test returns the applied report even if the staging sweep cannot remove the dir chmods the command's staging dir 0500 mid-apply; the command's finally then cannot remove it — correctly, per its documented leak-over-lie policy — and the test's restoring line carries the comment // let afterAll clean up, but afterAll only removes the fixture dirs it tracked (tmpDirs), never the command's staging dir. The sibling cleanliness test takes its countStaging() baseline AFTER the leak happens, so it cannot see it. Impact is bounded (one 0700 dir per suite run; CI containers are discarded; only local repeated runs accumulate).
Measured candidate fix (applied in a scratch copy — not in this report's tree)
- if (stagingDir) chmodSync(stagingDir, 0o700); // let afterAll clean up
+ if (stagingDir) {
+ chmodSync(stagingDir, 0o700);
+ // afterAll only removes the FIXTURE dirs it tracked; remove the
+ // command's staging dir here, or every run leaks it into tmpdir.
+ rmSync(stagingDir, { recursive: true, force: true });
+ }Three results with the patch: suite 36 passed (36) (counts unchanged), staging dirs after the suite 0 (was 1), and no other test touches the dir after the restore (logs/fix.log).
Nit 2 — Reviewer Test Plan names a test path that does not exist
cd packages/cli && npx vitest run src/commands/review src/utils/findings.test.ts — there is no src/utils/findings.test.ts in packages/cli (the findings suite is src/commands/review/findings.test.ts, already covered by the first filter). Vitest treats the dead path as a zero-match filter, so the command still exits 0 with the full 5053-pass result — harmless, but the path as written will confuse the next reviewer who tries to target just that suite.
Not covered
- Per-commit attribution. The metadata snapshot lists 8 commits; the depth-2 merge-ref checkout makes only the aggregate
HEAD^1..HEADdiff reachable (git rev-list HEAD^1..HEAD^2returns 1 at the shallow boundary, so the mismatch was detected, not guessed). All measurements above are of the aggregate. - Base OID drift. The snapshot's
baseRefOid(717ad10…) differs from the merge-ref base tipHEAD^1(2dbe806204…);headRefOidmatchesHEAD^2exactly. The A/B uses the merge-ref base — the one the verified head actually merges against. agent-promptbrief live render. Exercised only through the pinned tests (+ M4), not driven end-to-end as a standalone CLI invocation.- ab-drive adversarial same-uid channel (FIFO/symlink-planted logs/sentinels, root-swap races). The code carries explicit hardening and a documented accepted-residual-risk statement; I did not attack it live. The
--readyper-arm probe path likewise rides on unit tests (my live cells used--shared-ready). - Windows paths (
it.skipIf(process.platform === 'win32')shapes) — this lane is linux. - Repo-wide lint not run; repo-wide typecheck passed at HEAD (exit 0,
logs/typecheck.log). - Semantic review of the docs prose (DESIGN.md's four rationale sections, the feat(web-shell): refresh composer skills incrementally after toggles #9131 incident paragraph) — checked for agreement with measured behaviour where testable, not line-edited.
verify-capture.mjs: present and used for all four evidence images; nothing uncovered here.
Methodology
Environment: the lane's own node:22-bookworm container (node v22.23.2, uid 1000, no sudo), working tree at refs/pull/9740/merge depth 2; npm ci + npm run build pre-done at HEAD. The container ships no tmux (measured: command -v tmux empty; the verify job's install step adds only ca-certificates curl git gnupg jq gh), so ab-drive's live cells first extracted tmux 3.3a + libevent + libutempter debs into /__w/_temp/tmux-prefix via dpkg -x and ran everything with that prefix on PATH/LD_LIBRARY_PATH. A/B base side: git worktree add tmp/base-tree HEAD^1, cli rebuilt there (scripts/build_package.js; the build's tsc --build reports type errors from core's source references yet emits the cli JS — verified the emitted base dist contains holdUnwitnessedCriticals and not the new symbols, and no ab-drive/revert-hunk modules). Internal-link check quoted per the contract: from the base tree, @qwen-code/qwen-code-core realpath resolves to the head tree's packages/core — clean control here because this PR's core diff is .md-only (0 .ts files), established by git diff --name-only. Harnesses drove the compiled dist/ through spawnSync (real child processes, real git, real tmux, loopback-free because no network upstream was needed — shared upstreams were local sleep/marker-file scripts, the observer shape's essence); all assertions are scripted expected/actual comparisons that exit non-zero on surprise. Mutation work happened in a separate scratch worktree (tmp/vacuity-tree), every mutant reverted after its row (tree verified clean), worktrees removed afterwards; the vitest guard's workspace-dist preconditions there were satisfied with the head tree's dists for packages this PR does not touch (verified 0 diff lines in acp-bridge/web-templates/channels/*). Raw per-cell logs and the probe scripts live in logs/; harness sources in harness/. Intermediate harness iterations existed and failed for harness-side reasons only (utf8 diff capture; a too-close two-hunk fixture; an L2 fixture that read the shared upstream's state from the wrong tree's cwd) — each was classified by a standalone probe before the fix, and only the final harness runs are counted above.
Flakiness gate log
rounds=5 files=5 skipped=0
file packages/cli/src/commands/review.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review.test.ts
file packages/cli/src/commands/review/ab-drive.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/ab-drive.test.ts
file packages/cli/src/commands/review/agent-prompt.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/agent-prompt.test.ts
file packages/cli/src/commands/review/findings.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/findings.test.ts
file packages/cli/src/commands/review/revert-hunk.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/revert-hunk.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review.test.ts: PPPPP
packages/cli/src/commands/review/ab-drive.test.ts: PPPPP
packages/cli/src/commands/review/agent-prompt.test.ts: PPPPP
packages/cli/src/commands/review/findings.test.ts: PPPPP
packages/cli/src/commands/review/revert-hunk.test.ts: PPPPP
verdict: pass
summary: 5 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/ab-drive.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/agent-prompt.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/revert-hunk.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/ab-drive.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/agent-prompt.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/revert-hunk.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/ab-drive.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/agent-prompt.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/revert-hunk.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/ab-drive.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/agent-prompt.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/revert-hunk.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/ab-drive.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/agent-prompt.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/revert-hunk.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Independent pass at head 31665a8 (not approving yet — the 26 open Critical threads need a bot re-round on this head first, and Test (ubuntu) is still pending on it). What I verified directly:
- The pairing discipline is the right shape: one script serves both arms verbatim with the SHA-256 in the report, AB_ARM/AB_ARM_ROOT as the only variation; the shared upstream is started/readiness-polled/liveness-checked per arm and killed unconditionally; the
observedgate is false unless both arms completed and the shared process outlived each arm it served. That is exactly the set of facts a hand-rolled A/B loses. - R4-3 (untrusted script vs harness observation files): addressed — the log/rc reads are O_NONBLOCK with an fstat regular-file guard, so the rm-and-mkfifo hang is closed, and the residual risk is stated honestly and once: this hardens against a hang, not against a same-uid producer corrupting its own capture (symmetric corruption reads as identicalOutput; no file-based signal prevents it because the attacker is the producer).
- R3-6 (exactly-empty --arm-a/--arm-b/--shared-cwd): refused with nothing started, checked before the tmux probe.
- The round-7 commit's unified diff-prefix gate (git-standard-form sections only, rename/copy metadata required when paths differ) closes the R5-2/R6-1/R7-3 holes in one rule per its message.
Holding approval until the bot re-round on this head clears (or re-states) the 26 open Criticals and Test (ubuntu) lands green.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the full diff and existing threads at exact head 31665a87.
One correctness blocker remains: R7-7. A current-head real-tmux reproduction with a cooperative daemonizing shared script returns observed: true in per-arm mode while both arms capture ARM-A; the detached arm-A upstream survives teardown, satisfies arm B readiness, and serves arm A content to arm B. This is normal daemon lifecycle rather than the accepted same-uid capture-tampering class, and it breaks the fresh-per-arm evidence contract.
Verification on this head: 506 targeted CLI assertions passed, including 45 ab-drive tests and a clean isolated 36-test revert-hunk real-git run; 39 bundled-review skill tests passed; changed-file ESLint, Prettier, and git diff --check passed. The full build reached unrelated pre-existing Ink type mismatches in untouched UI/selection code. Ubuntu CI is still pending at review time.
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Two new subcommands give the verifier runnable evidence forms: ab-drive runs one script against the PR and base trees (shared upstream owned end to end, fresh per arm by default, a keeper session pinning the tmux server so sequential phases cannot race its shutdown, and an observed gate refusing confounded comparisons), and revert-hunk applies a single hunk in reverse through git's own patch engine so load-bearing claims are measured, not read. The witness rule now covers both postable severities (Suggestion posts on the same terms as Critical), a reason-less 'not run' line counts as no witness, and the verify brief gains an incidental-findings channel: what a required run tripped over enters the pipeline unverified instead of being read past, bounded by zero extra budget, no self-confirmation, and verdicts-first.
ab-drive: arm and shared-cwd paths must be directories (tmux -c silently falls back to $HOME for a file); time budgets are validated finite and positive (NaN disabled every deadline and leaked the server); the completed branch re-reads the log after the sentinel, mirroring drive; a timed-out or overflowed arm session is killed before the other arm starts; a per-arm shared instance is torn down on every post-start bail path; --shared-once fast-fails arm b when the instance already exited; the shared-ready probe keeps the daemon's own AB_ARM_ROOT under --shared-cwd; the handler validates --out up front and prints the report before writing it; the exec wrapper and poll cadence are now imported from drive instead of duplicated. revert-hunk: extracted patches drop file-level rename/mode metadata (a single-hunk revert no longer rewinds a rename or flips permission bits); spawn-level git failures are reported as harness facts instead of phantom coupling; the hunk lookup keys on the parsed selector so a non-canonical number cannot throw after mutating the tree; the report reaches stdout before --out; the patch staging dir is mkdtemp. findings: the reason-less 'not run' check tests for any Unicode letter or digit instead of enumerating dash glyphs (a CJK reason is a reason; U+2015 alone is not), and a measurement-held finding is exempt from the witness demotion so the two holds cannot compose into a silent drop. Tests cover every new branch and the previously unguarded conjunctions: asymmetric observed-gate fixtures, per-session harness control, session lifecycle ordering, exit codes, the real yargs conflicts guard, rename and mode-change fixtures, and tmp-dir cleanup in both suites.
Maintainer override: the 17-round review loop is stopped; standing findings are tracked as post-merge follow-ups. Superseded by human approval.
|
@qwen-code /triage |
|
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: 203 passed · 0 failed · 203 total Flakiness gate: ✅ 9 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：203 通过 · 0 失败 · 203 总计 抖动门：✅ 9 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9740 deep verification (follow-up round) —
|
| # | previous finding | severity | status at new head |
|---|---|---|---|
| 1 | revert-hunk.test.ts leaked one qwen-review-revert-hunk-* staging dir per run (the chmod test's "let afterAll clean up" comment was false) |
nit | fixed — the test now removes the command's staging dir itself with an accurate comment; re-measured: suite 69/69 green, staging dirs before 0 / after 0 (was 1) |
| 2 | Reviewer Test Plan names src/utils/findings.test.ts, which does not exist |
nit | stands — the body still names it, the file still does not exist, and the literal command still exits 0 (vitest treats the dead path as a zero-match filter), so it remains a harmless reviewer-confuser only |
Central claim + A/B
The PR's central claim is unchanged from the previous round: Step-4 verification gains two execution-grade machines whose pairing/extracting is done by code, not by hand — ab-drive (one script, two trees, owned upstream, an observed gate) and revert-hunk (verbatim one-hunk reverse apply) — and the witness rule's machine half covers both postable severities with a reason-less not run counting as no witness. Secondary claims: the verify brief carries the new capabilities + incidental-findings channel, and the round-17 hardening (EOL/type/count grounding, quoted-path scan, budget guards) holds under live probes.
A/B — witness demotion (qwen review findings), head build vs base build
Both arms are compiled CLI builds driven through the real command boundary, no mocks. Base = HEAD^1 rebuilt in a scratch worktree (see Methodology for the purity checks and the two build repairs). Witness: 01-ab-witness-demotion-head-vs-base.png (47/47).
| cell | input | head | base (control) |
|---|---|---|---|
| F1 | Suggestion, no witness | demoted to low + stderr line ✓ |
stays high ✓ |
| F2 | Critical, witness: "not run —" (empty reason) |
demoted ✓ | stays high ✓ (any witness string accepted) |
| F3 | Critical, not run — no X server in container |
stays high ✓ |
stays high ✓ |
| F4 | Suggestion + heldByMeasurement, no witness |
stays high (exempt) ✓ |
stays high ✓ |
| F5 | Nice to have, no witness |
stays high ✓ |
stays high ✓ |
| F6 | Critical, source: test, no witness |
stays high ✓ |
stays high ✓ |
| F7 | Suggestion WITH a witness | stays high ✓ |
stays high ✓ |
| F8 | Suggestion, witness: "not runnable" |
demoted ✓ (word-continuation consumed) | stays high ✓ |
| B1–B4 | not run + dash-only tail (U+2015 / U+2212 / U+FF0D / ASCII) |
all demoted ✓ | all stay high ✓ |
| B5 | witness: not run — 无可用环境 (prefix + CJK reason) |
stays high ✓ |
stays high ✓ |
| B6 | not run — … (punctuation-only) |
demoted ✓ | stays high ✓ |
| B7 | NOT RUN — reason given |
stays high ✓ |
stays high ✓ |
| B8 | not running — |
demoted ✓ | stays high ✓ |
| B9 | Vietnamese reason | stays high ✓ |
stays high ✓ |
| F9 | head's demoted output re-fed into head | idempotent: stays low, rule sentence ×1, no new demotion lines ✓ |
— |
| scale | 20 findings × 20 000-char witnesses (half present, half empty) | all classified correctly, 785 ms wall (linear) | — |
ab-drive — live cells on the built head CLI (real tmux 3.3a)
Base side of this claim remains "the subcommand does not exist at base" (re-verified: no ab-drive.js in the base dist). Witness: 02-ab-drive-live-real-tmux.png (44/44).
| cell | observable oracle | result |
|---|---|---|
| L1 per-arm shared upstream | both arms completed, observed: true, each arm saw its own marker (AAA/BBB) and a distinct upstream instance, AB_ARM/AB_ARM_ROOT correct, cwd = arm root, sharedAliveAtEnd: true, identicalOutput: false, scriptSha256 carried |
✓ |
L2 --shared-once + --shared-cwd |
one upstream instance observed by both arms, trees still distinct, observed: true, mode once |
✓ |
| L3 shared dies at birth | observed: false, exit 1, sharedAliveAtEnd: false on both arms, note names the dead upstream |
✓ |
L4 guards ×8 (same dir / empty script / empty arm path / missing dir / file-as-arm / NaN timeout / empty --shared / bad server name) |
all refused with a fail report (observed: false + named reason), nothing started. Exit class is 1 by design here — ab-drive reserves exit 2 for the --out TypeError path; guard refusals ride the fail-report shape (documented in the handler) |
✓ 8/8 |
| L5 residue | no tmux server left, zero new qwen-review-ab-drive-* temp dirs |
✓ |
Budget guards (round-17 delta). Witness: 05-budget-guards-trimmed-vs-overflowed.png (13/13).
| shape | oracle | result |
|---|---|---|
| S1 fast flood (~9 MB completing between polls) | capture TRIMMED (... [N characters omitted from the head] ...), truncated: true, identicalOutput: null — a truncated capture is never compared as identical; stdout-to-file and --out reports agree |
✓ |
| S2 slow flood (log above the 8 MB soft cap while still running) | outcome: overflowed, observed: false, exit 1, note names the partial state |
✓ |
revert-hunk — live cells on a real fixture repo (built head CLI)
Fixture: a real two-commit git repo with a multi-section diff (two-hunk file, no-trailing-newline, CRLF, latin-1 0xE9, colon/space filenames, a detected rename-with-edits, a C-quoted rename target), diffs captured as bytes. Witness: 03-revert-hunk-live-fidelity.png (78/78).
Carried-forward cells (all re-measured at the new head): R1 --list ids/headers/counts; R2 revert top hunk, bottom kept; R3 double revert refused, tree byte-identical; R4 context-mutated tree refuses then applies after reset; R5/R6/R7 byte-identity vs git's own base blobs for no-newline/CRLF/latin-1 (0xE9 survives, no U+FFFD); R8 colon + space filenames; R9 rename-with-edits reverts content at the new path, old path not resurrected; R10 --no-prefix/custom-prefix refusals exit 2, tree untouched; R11 exit-code discipline (bad selector / nonexistent hunk / missing flags / ambiguous concatenated diff all exit 2); R12 zero staging residue.
Round-17 delta cells:
| cell | probe | result |
|---|---|---|
| D1 | --diff CRLF-normalized (\r on @@ headers) |
refused exit 2, harnessFailure, note names CRLF, tree untouched ✓ |
| D2 | real symlink-retarget section (index … 120000) |
refused exit 2, note names symlink/TYPE ✓ |
| D3 | gitlink section (Subproject commit body, mode 160000) |
refused exit 2 ✓ |
| D4 | target carrying .gitattributes eol=crlf |
refused exit 2, note names the attribute, content untouched ✓ |
| D5 | under-declared @@ counts (body continues past them) |
refused exit 2, note names the count disagreement ✓ |
| D6 | git format-patch -1 --stdout capture |
--list counts exact (+1/−1, the -- mbox trailer NOT counted as a removed line), revert applies, trailer NOT appended to the file ✓ |
| D8 | rename whose target needs C-quoting (we"ird.txt) |
id unquoted in --list, revert applies at the new quoted path, old path not resurrected ✓ |
Mutation matrix — the new tests are not vacuous
One point mutation per guard in a scratch worktree at the verified head, reverted after each row; witness 04-mutation-matrix-all-killed.png (13/13 scripted checks over the recorded logs; raw logs logs/m0-control.log–m4.log).
| mutant | kill evidence (quoted) |
|---|---|
| control (unmutated, same four files) | 546 passed / 0 failed, exit 0 |
| M1 findings: severity condition back to Critical-only | judges Suggestions on the same terms — they post to the PR too → AssertionError: expected 'high' to be 'low' |
M2 ab-drive: sharedHeld = true |
5 tests red incl. a shared process that dies before its arm finishes fails the observed gate → expected true to be false |
M3 revert-hunk: FILE_LEVEL_METADATA_RE disabled |
5 tests red incl. expected '…' not to contain 'rename from' |
| M4 agent-briefs: witness sentence reverted | drives the verify role end-to-end through the handler → expected '…' to contain 'A confirmed Critical returns its witn…' |
No survivors. Every kill landed on the intended behavioral assertion, not an import/compile break. Tree verified clean after each row (git status --short empty).
Brief rendering + targeted gates
brief-live (8/8): a live qwen review agent-prompt --plan … --role verify --findings … through the compiled CLI writes a brief carrying the extended witness sentence, the not run — form, the ab-drive block, the revert-hunk paragraph, the ### Incidental findings section, and the verdict-first demand.
| gate | result at the new head |
|---|---|
packages/cli review suite (npx vitest run src/commands/review) |
5475 passed | 17 skipped | 0 failed, 114 files |
literal test-plan command … src/commands/review src/utils/findings.test.ts |
exits 0, same counts (the second path matches nothing — see nit below) |
packages/core SKILL.test.ts |
55/55 passed |
repo-wide npm run typecheck |
exit 0 (logs/typecheck.log) |
Findings
Suggestion — a near-cap ab-drive report's stdout is truncated for PIPE consumers (pre-existing forced-exit mechanism; --out and file redirects are complete)
Repro: drive an arm whose output floods ~9 MB fast (completing between polls), consume via a pipe (e.g. Node spawnSync capture): the stdout report is cut mid-JSON at ~146 KB (Unterminated string in JSON), while --out <file> and a shell > file redirect both carry the complete, valid report (measured: 200 847 bytes, a.truncated: true, identicalOutput: null).
Mechanism and attribution (measured, not inferred): the review subcommands exit through config.ts:1128's process.exit(process.exitCode ?? 0), which drops queued async stdout writes; a plain-node control reproduces the class (process.exit after a 300 KB write delivers 81 920 of 300 004 bytes). config.ts is unchanged by this PR (0 diff lines; identical line at base), and the base's drive emits its report the same way (drive.ts:783 at base) — so the mechanism predates the PR. What the PR adds is a second command whose stdout routinely reaches this size exactly when things go wrong (the near-cap-capture path the budget guards exist for), plus a handler comment that calls stdout "the run's only evidence".
Bounding: the pipeline brief mandates --out <plan-dir>/ab-drive.json, which is always complete; exit codes and stderr are unaffected; a file-redirected consumer gets everything. A candidate fix (synchronous fd-1 write in the handler) was measured in a scratch dist copy and did NOT change the pipe-consumer outcome, so the honest fix lives at the exit path (drain-before-exit) or the consumer contract (--out), not in this handler — which is why this is filed as a Suggestion about the premise/comment rather than a code-change demand on this PR.
Nit (stands from previous round) — Reviewer Test Plan names a test path that does not exist
cd packages/cli && npx vitest run src/commands/review src/utils/findings.test.ts — there is no src/utils/findings.test.ts (the suite is src/commands/review/findings.test.ts). Re-measured at the new head: the file still does not exist, and the literal command still exits 0 with the full suite (vitest treats the dead path as a zero-match filter). Harmless, but the path as written will confuse the next reviewer targeting just that suite.
Not covered
- Per-commit attribution. The metadata snapshot lists 28 commits; the depth-2 merge-ref checkout makes only the aggregate
HEAD^1..HEADdiff reachable (git rev-list HEAD^1..HEAD^2returns 1 at the shallow boundary; the previous head31665a87is not reachable, so old-vs-new head file-level delta could not be enumerated — delta scoping used the newest commit's named areas, each probed directly instead). - Base OID drift. Snapshot
baseRefOid(983fafd1…) differs from the merge-ref base tipHEAD^1(f470b122fa…);headRefOidmatchesHEAD^2exactly. The A/B uses the merge-ref base — the one the verified head actually merges against. - ab-drive adversarial same-uid channel (FIFO/symlink-planted logs/sentinels): code carries explicit hardening plus a documented accepted-residual-risk statement; not attacked live.
--readyper-arm probe rides on unit tests (live cells used no readiness gate or--shared-ready). - Windows paths (
skipIf(process.platform === 'win32')shapes) — this lane is linux. - Repo-wide lint not run; repo-wide typecheck passed (exit 0).
- Flakiness gate is owned by the workflow lane (5 changed test files × 5 rounds); it is not re-run inside this agent's budget.
- Semantic review of docs prose (DESIGN.md rationale sections) — checked against measured behaviour where testable, not line-edited.
- Harness-side corrections this round, for audit honesty: two early fixture bugs were mine, not the command's — a 2-line rename fixture is below git's similarity threshold (the diff emitted create/delete sections; fixed with 20-line files), and decoding a lone 0xE9 byte as UTF-8 yields U+FFFD by construction (the byte-identity oracle was the real check). One early oracle mis-read ab-drive's exit contract (guard refusals are exit-1 fail reports by design, not exit 2).
Methodology
Environment: the lane's own node:22-bookworm container (node v22.23.2, git 2.39.5, uid 1000), working tree at refs/pull/9740/merge depth 2; npm ci + npm run build pre-done at HEAD. The container ships no tmux; tmux 3.3a + libevent + libutempter debs were extracted into /__w/_temp/tmux-prefix via dpkg -x and put on PATH/LD_LIBRARY_PATH for every ab-drive cell. Base arm: git worktree add tmp/base-tree HEAD^1, cli rebuilt there; two repairs were needed before the base CLI would run — npm run generate (the git-ignored generated/git-commit.ts does not exist in a fresh worktree; without it the base CLI dies with ERR_MODULE_NOT_FOUND) and the pre-existing channels/feishu tsc errors that make npm run build -w packages/cli exit 1 while still emitting the cli JS (same as the previous round; base dist verified to contain holdUnwitnessedCriticals and neither holdUnwitnessedFindings/isEmptyNotRunWitness nor any ab-drive/revert-hunk module, while head dist is the mirror image). Internal-link purity per the contract: the base tree has no node_modules; readlink -f of @qwen-code/qwen-code-core from the base tree resolves to the head tree's packages/core — a clean control because this PR's core diff is .md + one test file (0 production .ts), re-established by git diff --name-only. Harnesses drove the compiled dist/ through real child processes (real git, real tmux, byte-faithful diff capture via latin1/buffers); oracles compared against git's own base blobs where fidelity mattered. Mutation work ran in a separate scratch worktree at the verified head, with the vitest guard's workspace-dist prerequisites satisfied by symlinks to the head tree's dists for packages this PR does not touch (verified 0 diff lines in acp-bridge/web-templates/channels/*) and core/cli node_modules symlinks for ajv and cli-local deps; every mutant reverted after its row and the tree verified clean. Raw per-cell logs in logs/, harness sources in harness/ (rerunnable: node harness/<name>.mjs), evidence images in evidence/. Worktrees removed afterwards (git worktree list shows only the main tree).
Flakiness gate log
rounds=5 files=9 skipped=0
file packages/cli/src/commands/review.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review.test.ts
file packages/cli/src/commands/review/ab-drive.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/ab-drive.test.ts
file packages/cli/src/commands/review/agent-prompt.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/agent-prompt.test.ts
file packages/cli/src/commands/review/drive.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/drive.test.ts
file packages/cli/src/commands/review/findings.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/findings.test.ts
file packages/cli/src/commands/review/lib/local-anchor.integration.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/local-anchor.integration.test.ts
file packages/cli/src/commands/review/pr-context.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/pr-context.test.ts
file packages/cli/src/commands/review/revert-hunk.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/revert-hunk.test.ts
file packages/core/src/agents/team/teamHelpers.test.ts: (cd packages/core) npx --no-install vitest run ./src/agents/team/teamHelpers.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review.test.ts: PPPPP
packages/cli/src/commands/review/ab-drive.test.ts: PPPPP
packages/cli/src/commands/review/agent-prompt.test.ts: PPPPP
packages/cli/src/commands/review/drive.test.ts: PPPPP
packages/cli/src/commands/review/findings.test.ts: PPPPP
packages/cli/src/commands/review/lib/local-anchor.integration.test.ts: PPPPP
packages/cli/src/commands/review/pr-context.test.ts: PPPPP
packages/cli/src/commands/review/revert-hunk.test.ts: PPPPP
packages/core/src/agents/team/teamHelpers.test.ts: PPPPP
verdict: pass
summary: 9 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/ab-drive.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/agent-prompt.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/drive.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/local-anchor.integration.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/pr-context.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/revert-hunk.test.ts: P (exit 0)
round 1 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/ab-drive.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/agent-prompt.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/drive.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/local-anchor.integration.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/pr-context.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/revert-hunk.test.ts: P (exit 0)
round 2 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/ab-drive.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/agent-prompt.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/drive.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/local-anchor.integration.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/pr-context.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/revert-hunk.test.ts: P (exit 0)
round 3 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/ab-drive.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/agent-prompt.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/drive.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/local-anchor.integration.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/pr-context.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/revert-hunk.test.ts: P (exit 0)
round 4 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/ab-drive.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/agent-prompt.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/drive.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/local-anchor.integration.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/pr-context.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/revert-hunk.test.ts: P (exit 0)
round 5 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
🤖 Could not produce a passing fix for this feedback (round 17/100) — the verification gate rejected the attempt. This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. Round 19 — address-review summary (PR #9740)Same-run verification repair round. Per the workflow's instruction, the The rejection and its diagnosisThe gate's The throw is the diagnosis: it can only fire AFTER
Why it was not pushed: tests failed in packages/cli 中文说明🤖 未能为该反馈产生可通过验证的修复（第 17/100 轮） —— 验证门拒绝了该尝试。此项现在需要人工处理；循环保持在线，仍会拾取新反馈与 base 冲突，但不会自行重试此项。 验证门的拒绝原因与日志证据见上方英文部分（gate-rejection 不翻译）。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/33159300648 🧠 Handled by Qwen Code · model/模型 |
|
Released in v0.22.3. |
6 participants
What this PR does
Adds execution-grade evidence forms to the
/reviewskill's Step 4 verification, in four connected pieces:qwen review ab-drive— runs ONE script against two trees (the PR worktree and thebase-treepath) and reports the paired captures. Same bytes on both arms (the report carries the script's SHA-256;AB_ARM/AB_ARM_ROOTare the only variation), an optional--sharedupstream owned end to end (started, readiness-polled, liveness-checked at each arm's end, killed unconditionally — fresh instance per arm by default,--shared-oncefor observer-shaped drives where the arms only watch), and anobservedgate that goes false whenever a difference could be the harness's rather than the trees'.qwen review revert-hunk— enumerates the diff's hunks under stable<path>:<n>ids and applies exactly one in reverse in a target tree viagit apply -R --check, so "is this change load-bearing?" is answered by re-running the same probe on an intact/reverted pair instead of by reading the diff. Extraction is verbatim (file headers and\ No newlinemarkers included), and a hunk that will not revert independently is reported as a coupling fact, never forced.not run — <reason>line) on the same terms as a Critical; the machine half (holdUnwitnessedCriticals→holdUnwitnessedFindings) demotes either to low confidence (terminal-only, never posted) when it arrives with neither; and a reason-lessnot runline now counts as no witness at all — the reason must name the run capability that came closest.Nice to havestays exempt (terminal-only by construction).### Incidental findingssection instead of being read past, bounded by three hard rules: zero extra budget (only what a required run already surfaced; the hunt stays with the reverse audit), never self-confirmed (the entry enters the pipeline unverified and a later round's fresh verifier rules on it), and verdicts first. SKILL.md carries the orchestrator-side merge rules (high effort: ride the next verification round with the— [unverified]tag; medium: terminal-only under "Needs Human Review"), and DESIGN.md carries four new rationale sections plus one measured incident.Why it's needed
The highest-yield findings in this repo's review history come from building the PR and driving the real product, and Step 4 already runs things (probe,
base-tree,drive,extract-step) — but three shapes of that practice were still hand-rolled, and one class of its output had nowhere to go. The paired live-stack A/B (one upstream, two arms) was re-assembled by hand on every verification, and the pairing is exactly where a hand-rolled A/B quietly stops being evidence: two drive calls drift by a flag, a shared daemon dies between the arms, and the difference quoted is the harness's. Hunk-level mutation ("are both hunks load-bearing?") relied on hand-extracted sed ranges over multi-thousand-line diffs, where an off-by-one silently tests a different mutation than the report claims. The brief's flat "you do not look for new problems" sent live-stack observations nowhere — the maintainer verification of PR #9131 (#9131 (comment), see Linked Issues) surfaced its three sharpest notes purely as side effects of driving the stack, none reachable by reading the diff. And the witness rule gated only Criticals, so an unexecuted claim could still land on the author's screen through the Suggestion door.Reviewer Test Plan
How to verify
Unit and integration suites (all green locally, including after merging
main):ab-drivelive (real tmux, no mocks): two temp dirs as arms, a shared upstream started fresh per arm, driven through the built CLI — both arms complete, each sees its own upstream instance and its own tree content,observed: true, and no tmux server or temp dir is left behind. Theab-drive.test.ts"driven for real" describe block does the same three scenarios (distinct arms, per-arm shared, upstream dying at birth →observed: false).revert-hunklive: a two-hunk fixture repo;--listenumeratesa.txt:1/a.txt:2, reverting:1restores the top edit while keeping the bottom one, re-reverting the same hunk is refused by--checkwith the tree byte-identical (exit 1).Brief rendering:
qwen review agent-prompt --plan <fixture> --role verify --findings <file>produces a brief carrying the ab-drive block, the revert-hunk paragraph, the extended witness rule, and the incidental-findings closer — pinned inagent-prompt.test.ts.Witness demotion: feed
qwen review findingsa high-confidence[review]Suggestion without awitnessand observe the stderr demotion line plusconfidence: lowin the artifact; same for a Critical withwitness: "not run —"(empty reason).Evidence (Before & After)
N/A (no UI change). Live
ab-driverun through the built CLI, trimmed:{ "observed": true, "mode": "per-arm", "a": { "outcome": "completed", "exitCode": 0, "output": "port=7777\nAAA\narm=a\n", "sharedAliveAtEnd": true }, "b": { "outcome": "completed", "exitCode": 0, "output": "port=7777\nBBB\narm=b\n", "sharedAliveAtEnd": true }, "identicalOutput": false }Live
revert-hunkdouble-revert (refusal path, tree unchanged):{ "applied": false, "hunk": { "id": "a.txt:1", "header": "@@ -1,4 +1,4 @@" }, "conflict": "error: patch failed: a.txt:1\nerror: a.txt: patch does not apply" }Tested on
Environment (optional)
macOS 26.6, Node 24, real tmux for the live
ab-drive/drivepaths; unit suites via vitest.Risk & Scope
findingscanonicalization — a high-confidence[review]-source Suggestion arriving without a witness (or with a reason-lessnot runline) is now demoted to low confidence and stays terminal-only. This is the intended tightening; deterministic sources ([build]/[test]/[probe]/[lint]) are exempt by construction, and the demotion appends the way back into the finding text.agent-promptpins); Windows, where the tmux-dependent paths reportunavailableby design, same asdrive.holdUnwitnessedCriticalswas renamed toholdUnwitnessedFindings(internal util, single call site).ab-driveandrevert-hunkare additive; roster, topology and budgets are untouched.Linked Issues
Motivated by the maintainer verification methodology on #9131 — specifically #9131 (comment), whose harness (same-source dual-arm A/B, per-hunk mutation, incidental notes N1–N3) this PR turns into commands and brief rules. No issue to close.
中文说明
本 PR 做了什么
为
/reviewskill 的 Step 4（验证环节）补上"可执行证据"形态，四块互相衔接：qwen review ab-drive—— 用同一份脚本跑两棵树（PR worktree 与base-tree路径）并回报配对捕获。两臂字节级一致（报告携带脚本 SHA-256；AB_ARM/AB_ARM_ROOT是唯一变量）；可选的--shared上游全程托管（启动、就绪轮询、每臂结束时活性检查、无条件击杀——默认每臂新起实例，--shared-once留给两臂只做观察者的形态）；observed门在任何"差异可能来自 harness 而非两棵树"的情况下都置 false。qwen review revert-hunk—— 以稳定的<path>:<n>id 枚举 diff 的 hunk，并经git apply -R --check在目标树里精确还原其中一个，让"这处改动是否 load-bearing"由同一探针在 intact/reverted 对上重跑来回答，而不是靠读 diff。提取逐字保真（文件头与\ No newline标记都在），无法独立还原的 hunk 作为耦合事实报告，绝不强行套用。not run — <理由>行）；机器半边（holdUnwitnessedCriticals→holdUnwitnessedFindings）对二者缺失时同样降为低置信度（只进终端、绝不上 PR）；空理由的not run行现在按无 witness 处理——理由必须点名最接近能跑起来的那个能力。Nice to have维持豁免（本就只进终端）。### Incidental findings节而不是被读过去，受三条硬规则约束：零额外预算（只报必要运行已经摆到面前的东西；搜寻仍归反向审计）、绝不自证（条目以未验证状态入管线，由后续轮次的新 verifier 裁决）、verdict 优先。SKILL.md 承载编排者侧的合并规则（high effort：带— [unverified]标签搭下一轮验证；medium：只进终端的 "Needs Human Review"），DESIGN.md 新增四个 rationale 小节和一条 measured incident。为什么需要
本仓库评审史上产出最高的发现都来自"构建 PR、驱动真实产品"，Step 4 也早已具备运行能力（probe、
base-tree、drive、extract-step）——但这套实践里仍有三个形态靠手工，还有一类产出无处安放。同源双臂 A/B（一个上游、两条腿）每次验证都手工重搭，而配对恰恰是手搭 A/B 悄悄失去证据资格的地方：两次 drive 差一个 flag、共享 daemon 在两臂之间死掉，引用的差异就成了 harness 的。hunk 级变异（"两处 hunk 都是必需的吗"）靠在几千行 diff 上手工 sed 取段，差一行就在悄悄测试一个与报告声称不同的变异。brief 里一句"不找新问题"让真实栈上的观察无处可去——#9131 的维护者验证（见 Linked Issues 中的评论链接）里最锋利的三条 note 全是驱动真实栈的副产品，没有一条能靠读 diff 得到。同时 witness 规则只卡 Critical，未经执行的断言仍能从 Suggestion 这扇门贴上作者屏幕。评审验证计划
如何验证
单元与集成套件（本地全绿，合并
main后复跑仍绿）：ab-drive实跑（真 tmux、无 mock）：两个临时目录当双臂、每臂新起共享上游、走构建后的 CLI——双臂 completed，各自看到自己那个上游实例与自己树里的内容，observed: true，无 tmux server 与临时目录残留。ab-drive.test.ts的 "driven for real" 块覆盖同样三个场景（双臂内容不同、每臂共享上游、上游出生即死 →observed: false）。revert-hunk实跑：双 hunk 夹具仓库；--list枚举a.txt:1/a.txt:2，还原:1后顶部改动回退而底部保留，对同一 hunk 二次还原被--check拒绝且树字节不变（exit 1）。brief 渲染：
qwen review agent-prompt --plan <fixture> --role verify --findings <file>产出的 brief 携带 ab-drive 块、revert-hunk 段、扩展后的 witness 规则与 incidental findings 结尾段——已在agent-prompt.test.ts钉住。witness 降级：给
qwen review findings喂一条无witness的高置信[review]Suggestion，观察 stderr 的降级行与产物中的confidence: low；对携带witness: "not run —"（空理由）的 Critical 同理。证据（Before & After）
N/A（无 UI 变化）。构建后 CLI 的
ab-drive实跑（截取）与revert-hunk二次还原拒绝路径见上方英文块。已测试平台
macOS ✅；Windows / Linux 交由 CI⚠️ 。
环境（可选）
macOS 26.6、Node 24，
ab-drive/drive实跑用真 tmux；单测走 vitest。风险与范围
findings规范化的行为变化——高置信[review]来源的 Suggestion 若无 witness（或not run空理由）现在会降为低置信度、只进终端。这是刻意收紧；确定性来源（[build]/[test]/[probe]/[lint]）按构造豁免，降级文案附带恢复路径。agent-prompt钉点覆盖）；Windows 上依赖 tmux 的路径按设计报unavailable，与drive一致。holdUnwitnessedCriticals更名为holdUnwitnessedFindings（内部工具函数，单一调用点）。ab-drive与revert-hunk纯新增；roster、拓扑与预算未动。关联 Issue
动机来自 #9131 的维护者验证方法论——具体见 https://github.com/QwenLM/qwen-code/pull/9131#issuecomment-5378420743，本 PR 把该验证的 harness（同源双臂 A/B、逐 hunk 变异、N1–N3 类副产品发现）固化为命令与 brief 规则。无需关闭的 issue。