feat(review): report the address a drive's service actually bound - #9655
Conversation
A port is a request, not a fact. Handed one that is taken, `qwen serve` prints `port 8931 is in use, trying 8932...` and listens on the next. A verifier that goes on addressing the port it asked for then reads a different, stale process for the rest of the run — its readiness probe passes against whatever is squatting there, the drive completes, and every number in the witness is about the wrong daemon. Nothing in the report says so, because nothing in the report knew: `drive` had no port handling at all. Measured during a daemon verification, it cost a full cycle before the readings stopped making sense. `--capture name=<regex>` reads named facts back out of the run's own output into `captured`, the bound address first among them. Four choices in it are the ones that matter: - It reads the UNTRIMMED log. `trimCapture` keeps the tail and a service prints its address at the head, so capturing from the report's `output` would lose exactly the value this exists for, on the loudest runs — the ones most likely to need it. - A pattern that never matched is `null`, never `''`, and the note NAMES it. That is the moment a witness is about to quote a value the run never produced, and the reader has to know which one. - A malformed pattern rejects the whole set before anything starts. Silently dropping the bad entry would leave a missing key beside the good ones, which reads as "the service never printed it" — the one meaning `null` is reserved for. Finding out after a 300-second drive costs the drive. - Captures are taken on every outcome, not only `completed`: a drive that timed out still bound its port, and that address is often what explains where the rest of it went. The verify brief carries it too. A capability taught only where the verifier does not read is inert — the lesson from #9445's first review round — so the brief now says to bind ephemeral where the service allows it and quote the captured address rather than the one on the command line. Refs #9446.
|
Second re-run — four commits landed since the last pass at
Moving on to code review. 🔍 中文说明第二轮重跑——自上一轮（
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review (state at
|
| Check | Conclusion |
|---|---|
Classify PR |
✅ success |
Dependency CVE audit |
✅ success |
Desktop Shell (ubuntu-22.04) |
✅ success |
Desktop Shell (windows-2022) |
✅ success |
Post Coverage Comment (ubuntu-latest, 22.x) |
✅ success |
Remind on force-push |
✅ success |
Secret scan (TruffleHog) |
✅ success |
Test (ubuntu-latest, Node 22.x) |
✅ success |
review-pr |
✅ success |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
✅ success |
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。
The central claim is behavioural, and the prior isolated /verify A/B (at 999262a) already proved it load-bearing — a contended drive captures the bound address from a head-trimmed log, base has no captured field at all, and the mutation matrix killed 9/9. A fresh /verify run triggered by this triage comment is in flight (run 32572093115) and will re-run the A/B against this head — the two commits it adds over the verified one are exactly the recipe fix and the re-read; its report will post in the verification thread. No new trigger needed.
中文说明
代码审查（b54d07b 状态）
无阻塞项。本轮通读全部 diff，重点看 999262a 之后落地的两个 Critical 修复——都是针对已复现缺陷的真实修复，且都比各自对应的 finding 收得更窄：
- 第 5 轮 Critical 已修复，并由一个直接执行 brief 原文的测试钉住（
0eb58b9）。初版 recipe 把服务重定向到自己的文件，而--capture只读 drive 日志——结果是每一次忠实运行都得到null，miss note 却声称该值"从未被测量"。修正后的形态把输出送到它必须到达的两处：脚本grep它做就绪探测并从中推导$BASE的mktemp文件，然后在第一个请求之前cat它——捕获取首次匹配，因此服务自己的行优先于任何回显该行的响应体（测试的诱饵服务在响应体里宣称 59999 端口，断言明确拒绝它）。mktemp保持被审工作区干净，rm -f负责清理；brief 同时新增两条警告：脚本不要自带trap … EXIT（会顶替包装器的 sentinel 陷阱——运行完美却报 timed-out），以及注意块缓冲的 stdout。该测试承重之处在于：脚本体与捕获 pattern 都从BRIEFS.verify本身提取而非重新抄写，因此 brief 与测试不可能悄然漂移。它已在当前 head 的 Linux CI 套件中跑绿。 - 第 6 轮 Critical 已修复，经代码审读验证（
b54d07b）。轮询循环原本背靠背地先读日志、再读 sentinel；落在两次读取之间的最后一次写入在文件里、却不在快照里——对运行确实产出的值报告null，note 还断言 pattern 从未匹配。修复在观测到 sentinel 后重读一次日志，其 happens-before 主张成立：wrapScript用 EXIT 陷阱写 sentinel（已在 head 确认），严格晚于脚本经 shell 重定向的最后一次写入，故 sentinel 之后的读取是完整的。修复正确地只保留在completed分支——超时与溢出是"终止了运行"而非"观测到运行结束"，没有可依赖的保证。 - 一个诚实的不对称，非阻塞： 本 PR 其余每个修复都带着变异验证过的回归测试；这次重读修复没有——确定性的 FIFO 复现存在于 autofix 运行器被丢弃的提交里，从未推送。删掉重读那一行，现有套件依然全绿（fake-tmux 接缝在
new-session内同步写入日志与 sentinel，首次读取已包含两者）。这是关于套件的观察，不是关于修复的——修复本身审读正确，第 7 轮的变异探测在此处也没有登记任何问题——但若这条路径将来回归，没有任何测试会变红。 - 基线事实已在当前 head 复核：提取读未截断日志、先于
trimCapture；每种 outcome 都提取；不传--capture时报告逐字节不变（captured缺省、note 无捕获子句）；trim 调和子句仍限定于completed；CLI 接缝测试仍钉住 yargs→handler 强转。 /review循环自己对本提交（本运行前一小时）的第 7 轮未提出新发现；其余五条为收敛姿态下的 Suggestion 级延后项（docblock 措辞精确性、未钉住的上限边界、未完成 outcome 上 miss note 的原因子句），均已在评审台账中跟踪，均不阻塞。
测试证据（本 PR 自己的 CI，b54d07b）
审查提交上的两个 pull_request 事件工作流均绿——Qwen Code CI 与 Security Checks 都是 success，无 pending。Linux 单元测试（Test (ubuntu-latest, Node 22.x)）通过，其中跑了完整的 drive.test.ts，含新的 recipe 执行测试。Windows/macOS 测试任务与 Integration Tests (CLI, No Sandbox) 显示 skipped 是工作流自身 classify_pr 门控的结果（与本 PR 此前每个提交一致），并非失败。只取一次快照、不轮询。
核心主张是行为性的，而此前的隔离 /verify A/B（在 999262a 上）已证明其承重——端口竞争的 drive 能从头部裁剪后的日志里捕获实际绑定地址，base 完全没有 captured 字段，变异矩阵 9/9 全杀。由本次 triage 评论触发的新一轮 /verify 正在运行（run 32572093115），将在当前 head 上重跑 A/B——相对已验证提交新增的两个提交恰是 recipe 修复与重读修复；报告会发布在验证线程，无需再次触发。
— Qwen Code · qwen3.8-max
Reviewed at b54d07bf43ea3b18f48d8fb0defddece6f38debc · re-run with @qwen-code /triage
|
Confidence: 4/5 — two real Criticals found by the review loop, both fixed narrowly and correctly; the last fifth is an unpinneable race fix and a handful of tracked Suggestion-level deferrals. This arc is the process working as designed. The first pass approved at The two reservations, both named, neither blocking. The re-read fix is the one change in this PR without a mutation-verified regression test — the deterministic reproduction stayed in the autofix runner's discarded commit — so the suite passes with the line deleted; the fix itself reads as correct and round 7's probing found nothing, but the asymmetry is worth knowing. And five Suggestion-level items stay deferred under the convergence posture (docblock wording, unpinned boundaries, the miss note's cause clause on incomplete outcomes) — tracked in the ledger, not dropped. On the standing state: the round 5 and 6 Verdict: approve. Both 中文说明置信度：4/5 —— 评审循环发现了两个真实的 Critical，均已被收窄且正确地修复；最后这一分扣在一个无法用测试钉住的竞态修复，以及若干已跟踪的 Suggestion 级延后项。 这条弧线正是流程按设计运转的样子。第一轮在 两点保留意见，点名说明，均不阻塞。重读修复是本 PR 唯一没有变异验证回归测试的改动——确定性复现留在了 autofix 运行器被丢弃的提交里——因此删掉那一行套件依然全绿；修复本身审读正确，第 7 轮探测也未发现问题，但这个不对称值得知晓。另有五条 Suggestion 级事项在收敛姿态下延后（docblock 措辞、未钉住的边界、未完成 outcome 上 miss note 的原因子句）——台账跟踪中，不会丢。 关于当前评审状态：第 5、6 轮的 结论：批准。两个 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| for (const { name, re } of specs) { | ||
| const m = re.exec(output); |
There was a problem hiding this comment.
[Suggestion] These caller-supplied regexes run against the full untrimmed log (up to the 8 MiB file cap — the overflow path reads more before breaking) with no time bound, in a phase --timeout does not cover: extraction happens after the poll loop has exited, so the timeout has already spent itself. The 200-char cap bounds pattern length, not backtracking complexity — a nested-quantifier pattern like x=(a+)+$ (9 chars, well under the cap) meeting a near-matching run in the log backtracks exponentially, and the CLI hangs at 100% CPU with no report ever written to --out or stdout. Measured through the shipped extractCaptures at this commit: (a+)+$ on 'a'.repeat(n)+'b' — n=22: 66 ms, n=24: 260 ms, n=26: 850 ms; growth is roughly ×3.5 per +2 chars, so a ~40-char run is hours. The PR description acknowledges "a pathological regex can spend time"; what is new here is that no timeout bounds this phase, and that the verify brief this same PR adds directs LLM agents to author their own patterns — a stray nested quantifier then costs the whole verification run rather than being caught by a human at a prompt. Consider running extraction under a deadline (a worker with an Atomics.wait timeout matches this file's polling idiom; report an expired capture as null with a note), rejecting nested-quantifier shapes at parse time, or at minimum documenting the hazard beside MAX_CAPTURE_PATTERN, which does not bound it.
中文说明
这些由调用方提供的正则会在完整未裁剪的日志（上限 8 MiB；overflow 路径在退出前会读到更多）上无时间约束地运行，而且处于 --timeout 已不再覆盖的阶段：提取发生在轮询循环退出之后，超时此时已经失效。200 字符上限约束的是 pattern 的长度，不是回溯复杂度——形如 x=(a+)+$（9 个字符，远低于上限）的嵌套量词 pattern 遇到日志中一段接近匹配的文本时会指数级回溯，CLI 以 100% CPU 挂死，--out 和 stdout 都不会写出任何报告。在本 commit 上用已发布的 extractCaptures 实测：(a+)+$ 对 'a'.repeat(n)+'b'——n=22：66 ms，n=24：260 ms，n=26：850 ms；大约每 +2 字符 ×3.5，因此 ~40 字符的连续段就是小时级。PR 描述承认"病态正则会耗时"；这里新增的是：该阶段不受任何 timeout 保护，而且本 PR 新增的 verify brief 明确指示 LLM agent 自行编写 pattern——一个误写的嵌套量词会葬送整轮验证，而不是在 prompt 阶段被人发现。建议：为提取设置截止时间（worker + Atomics.wait 超时，与本文件的轮询习惯一致；超时的 capture 报 null 并附 note），或在解析期拒绝嵌套量词形态，至少也在 MAX_CAPTURE_PATTERN 旁注明这一风险——它约束不了这个问题。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| type: 'array', | ||
| string: true, | ||
| describe: | ||
| 'name=<regex> read back out of this run\'s own output, e.g. baseUrl="listening on (http://\\S+)". Repeatable. Group 1 when the pattern has one, the whole match otherwise; null when nothing matched. Use it for anything the service CHOSE rather than was told — above all the address it actually bound.', |
There was a problem hiding this comment.
[Suggestion] The example in this help text is wrong in two ways. (1) The inner double quotes become part of the regex: the spec splits on the first =, so a caller who copies the sole --help example verbatim passes "listening on (http://\S+)" as the pattern — the literal " characters never match what qwen serve prints, so every run yields captured.baseUrl = null with the note saying the value was "addressed by assumption", and the caller falls back to the assumed port: the exact stale-process failure this flag exists to prevent. Probed through the real parser: the help-example capture on the actual qwen serve line = {"baseUrl":null}; the quote-free form = {"baseUrl":"http://127.0.0.1:8932"}. (2) http:// only: run-qwen-serve.ts builds the listening URL as scheme = tlsOptions ? 'https' : 'http', so a TLS-configured daemon prints https:// and the example never matches (probe: the taught pattern on a TLS log = null; the https? variant captures). Both fixed below; the verify-brief example carries the same scheme fix in its own comment.
| 'name=<regex> read back out of this run\'s own output, e.g. baseUrl="listening on (http://\\S+)". Repeatable. Group 1 when the pattern has one, the whole match otherwise; null when nothing matched. Use it for anything the service CHOSE rather than was told — above all the address it actually bound.', | |
| 'name=<regex> read back out of this run\'s own output, e.g. baseUrl=listening on (https?://\\S+). Repeatable. Group 1 when the pattern has one, the whole match otherwise; null when nothing matched. Use it for anything the service CHOSE rather than was told — above all the address it actually bound.', |
中文说明
这条帮助文本里的示例有两处错误。（1）内层双引号会成为正则的一部分：spec 在第一个 = 处切分，调用方照抄这个唯一的 --help 示例时，传入的 pattern 是 "listening on (http://\S+)"——字面 " 字符永远匹配不上 qwen serve 的输出，于是每次运行都得到 captured.baseUrl = null，note 说该值 "addressed by assumption"，调用方退回假设的端口：恰恰是这个 flag 要消灭的陈旧进程失效模式。经真实解析器实测：帮助示例对真实 qwen serve 输出行的 capture = {"baseUrl":null}；去掉引号的形式 = {"baseUrl":"http://127.0.0.1:8932"}。（2）只匹配 http://：run-qwen-serve.ts 按 scheme = tlsOptions ? 'https' : 'http' 构造监听 URL，配置了 TLS 的 daemon 打印 https://，该示例同样永远匹配不上（实测：教给 agent 的 pattern 对 TLS 日志 = null；https? 变体可以 capture 到）。下面的建议同时修掉这两处；verify-brief 示例的 scheme 修复在它自己的评论里。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
|
||
| \`\`\`bash | ||
| "\${QWEN_CODE_CLI:-qwen}" review drive --cwd <the worktree> --script <what to run> \\ | ||
| --capture 'baseUrl=listening on (http://\\S+)' --timeout 300 --out <plan dir>/drive.json |
There was a problem hiding this comment.
[Suggestion] The pattern taught here matches only http://, but qwen serve builds its listening URL as scheme = tlsOptions ? 'https' : 'http' (run-qwen-serve.ts:6621) — a TLS-configured daemon prints listening on https://… and this example never matches it, so the verifier gets captured.baseUrl = null even though the daemon is up. The miss is loud (the note names it), so no wrong address is ever reported — but for a TLS drive the canonical guidance degrades into exactly the assumption-mode this feature exists to eliminate, and every such drive pays a retry to discover the one-character fix. Probed: the pattern as taught on a TLS log = {"baseUrl":null}; the widened variant = {"baseUrl":"https://127.0.0.1:8443"}. The --capture help text carries the same scheme fix — plus a quoting defect — in its own comment.
| --capture 'baseUrl=listening on (http://\\S+)' --timeout 300 --out <plan dir>/drive.json | |
| --capture 'baseUrl=listening on (https?://\\S+)' --timeout 300 --out <plan dir>/drive.json |
中文说明
这里教给 verifier 的 pattern 只匹配 http://，而 qwen serve 按 scheme = tlsOptions ? 'https' : 'http' 构造监听 URL（run-qwen-serve.ts:6621）——配置了 TLS 的 daemon 打印 listening on https://…，这个示例永远匹配不上，即使 daemon 已经起来，verifier 拿到的也是 captured.baseUrl = null。miss 是显式的（note 会点名），所以不会报告错误地址——但对 TLS 驱动而言，这份规范示例退化成了这个功能本要消灭的"假设寻址"模式，每次都要白跑一轮重试才能发现这个一字之差。实测：原 pattern 对 TLS 日志 = {"baseUrl":null}；放宽后 = {"baseUrl":"https://127.0.0.1:8443"}。--capture 帮助文本有同样的 scheme 修复——外加一个引号缺陷——在它自己的评论里。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // Before anything is started, like the server-name check above: a caller who | ||
| // asked for a capture wants it in the witness, and discovering the pattern | ||
| // was malformed after a 300-second drive costs the drive. | ||
| const parsed = parseCaptureSpecs(args.capture); |
There was a problem hiding this comment.
[Suggestion] This early return hand-copies the nine-field "unavailable / nothing started" report literal from the server-name check directly above — after this PR, runDrive holds four near-identical copies of the same boilerplate (~413, ~430, ~446, ~520). Every future field added to DriveReport (this very PR added captured) must then be spelled out at four identical sites, and the next pre-flight check will paste a fifth copy; the copy at ~520 already differs (it returns the real readyAfterMs/killedStale instead of the fixed null/false), so a reader comparing the four can no longer tell at a glance which fields carry meaning and which are filler. A shared factory keeps the intentional differences visible:
const unavailable = (note: string, over: Partial<DriveReport> = {}): DriveReport => ({
outcome: 'unavailable', observed: false, exitCode: null, readyAfterMs: null,
droveForMs: 0, output: '', truncated: false, killedStale: false, note, ...over,
});
// here: return unavailable(`${parsed.error} Nothing was started.`);
// at ~520: return unavailable(msg, { readyAfterMs, killedStale });中文说明
这个 early return 手工复制了上方 server-name 检查那个九字段的 "unavailable / nothing started" 报告字面量——本 PR 之后，runDrive 里已有四份几乎相同的样板（~413、~430、~446、~520）。以后每给 DriveReport 加一个字段（本 PR 就加了 captured），都要在四处同时写对，下一个预检还会贴上第五份；而 ~520 那份已经不同了（返回真实的 readyAfterMs/killedStale 而非固定的 null/false），读者对照四份时已无法一眼看出哪些字段有意义、哪些只是填充。抽一个共享工厂可以让刻意存在的差异显形：
const unavailable = (note: string, over: Partial<DriveReport> = {}): DriveReport => ({
outcome: 'unavailable', observed: false, exitCode: null, readyAfterMs: null,
droveForMs: 0, output: '', truncated: false, killedStale: false, note, ...over,
});
// 此处： return unavailable(`${parsed.error} Nothing was started.`);
// ~520： return unavailable(msg, { readyAfterMs, killedStale });— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Deferred to the next round. Verified against the code: runDrive holds four near-identical nine-field unavailable-report literals, and the copy at the session-start site already differs (readyAfterMs/killedStale). The consolidation is real and worth doing, but this round already implements eight findings and the factory touches four sites at once — it gets its own round instead of riding along.
延后到下一轮。已对照代码核实：runDrive 中有四处几乎相同的九字段 unavailable 报告字面量，且会话启动处那份已经不同（readyAfterMs/killedStale）。这个收敛是真实的、值得做，但本轮已实现 8 条发现，而工厂会同时触及四处——它值得单独一轮，而不是顺带完成。
| * Group 1 when the pattern has one, the whole match otherwise, so both | ||
| * `listening on http://\S+` and `listening on (\S+)` do what they look like. | ||
| * | ||
| * FIRST match, unlike `sentinelExitCode`'s last. The two are answering |
There was a problem hiding this comment.
[Suggestion] The FIRST-match rule documented here is pinned by no test — every log in the new suite contains each pattern exactly once, where first and last match coincide. Verified by mutation: changing extractCaptures to take the LAST match (the idiom sentinelExitCode elsewhere in this file deliberately uses, which makes a copy-paste refactor toward it plausible) passes all 38 tests, while a two-occurrence probe flips cleanly — shipped returns http://127.0.0.1:8931, the mutant http://127.0.0.1:8932. A log with two occurrences is not exotic: a driven script that restarts the service, or a wrapper echoing the startup line. If that regression ships, captured.baseUrl reports a later, different address on exactly the noisy runs this feature exists for — reintroducing the stale-address confusion, with a green suite as its paperwork. One multi-match case pins it:
expect(
extractCaptures(
'listening on http://127.0.0.1:1\nlistening on http://127.0.0.1:2\n',
parseCaptureSpecs(['baseUrl=listening on (http://\\S+)']).specs,
),
).toEqual({ baseUrl: 'http://127.0.0.1:1' });中文说明
这里文档化的"取第一个匹配"规则没有任何测试锁定——新增套件里每份日志中每个 pattern 都恰好出现一次，首匹配与末匹配重合。变异验证：把 extractCaptures 改成取最后一个匹配（本文件其他地方 sentinelExitCode 刻意使用的写法，复制粘贴式重构很容易滑向它），全部 38 个测试仍然通过；而一条两次出现的探针可以干净地翻转——线上代码返回 http://127.0.0.1:8931，变异体返回 http://127.0.0.1:8932。两次出现并不罕见：驱动脚本重启服务、wrapper 回显启动行都会造成。若该回归溜进去，captured.baseUrl 恰恰在这个功能为之存在的吵闹运行上报告一个更晚的、不同的地址——带着全绿的套件，把陈旧地址混淆重新带回来。加一条多匹配用例即可锁定：
expect(
extractCaptures(
'listening on http://127.0.0.1:1\nlistening on http://127.0.0.1:2\n',
parseCaptureSpecs(['baseUrl=listening on (http://\\S+)']).specs,
),
).toEqual({ baseUrl: 'http://127.0.0.1:1' });— qwen3.8-max via Qwen Code /review (v0.21.15)
| const out: Record<string, string | null> = {}; | ||
| for (const { name, re } of specs) { | ||
| const m = re.exec(output); | ||
| out[name] = m ? (m[1] ?? m[0]) : null; |
There was a problem hiding this comment.
[Suggestion] A captured value is the only output channel in this file with no size cap. It is extracted from the UNTRIMMED log before trimCapture, so one group can carry up to ~8 MiB into the report — which the handler writes to BOTH stdout and the --out file — and the verify brief this PR adds explicitly instructs agents to quote captured values in the witness. A service that prints one huge line (a minified-JSON dump, a base64 blob) under a pattern spanning it (data=(.*)) puts megabytes into captured.data, and an agent following the brief ingests it — probed at this commit: data=(.*) over a ~1 MB line carried the full 1,000,000 characters. Everything else here is capped precisely because unbounded reviewer-script output was measured causing damage; this field bypasses both the 200 KB pane cap and the intent of the 8 MiB log cap. Truncate each captured value to a small bound (a few KB) with an explicit marker, symmetric with trimCapture:
const CAPTURE_VALUE_MAX = 4096;
const v = m ? (m[1] ?? m[0]) : null;
out[name] =
v !== null && v.length > CAPTURE_VALUE_MAX
? `${v.slice(0, CAPTURE_VALUE_MAX)}… [truncated, ${v.length} chars total]`
: v;中文说明
captured 值是本文件中唯一没有大小上限的输出通道。它先于 trimCapture 从未裁剪日志中提取，因此一个捕获组最多可以把 ~8 MiB 带进报告——handler 会把它同时写到 stdout 和 --out 文件——而本 PR 新增的 verify brief 又明确指示 agent 在 witness 中引用 captured 的值。服务一旦打印一行超大文本（minified JSON 转储、base64 块），而 pattern 又横跨它（data=(.*)），captured.data 就是兆字节级，照 brief 行事的 agent 会把它整个吞进去——在本 commit 上实测：data=(.*) 对 ~1 MB 的行原样带回了全部 1,000,000 个字符。本文件其他通道都设了上限，正因为无界的 reviewer 脚本输出曾实测造成损害；这个字段同时绕过了 200 KB 面板上限和 8 MiB 日志上限的意图。建议把每个 captured 值截断到较小上限（几 KB）并附明确标记，与 trimCapture 对称：
const CAPTURE_VALUE_MAX = 4096;
const v = m ? (m[1] ?? m[0]) : null;
out[name] =
v !== null && v.length > CAPTURE_VALUE_MAX
? `${v.slice(0, CAPTURE_VALUE_MAX)}… [truncated, ${v.length} chars total]`
: v;— qwen3.8-max via Qwen Code /review (v0.21.15)
| // WHICH one before deciding whether the rest still stands. | ||
| const captureNote = | ||
| missed.length > 0 | ||
| ? ` — no output matched --capture ${missed.map((n) => JSON.stringify(n)).join(', ')}, so ${missed.length === 1 ? 'that value is' : 'those values are'} null rather than measured; anything addressed by ${missed.length === 1 ? 'it' : 'them'} was addressed by assumption` |
There was a problem hiding this comment.
[Suggestion] When a completed drive's log exceeded the 200 KB report cap AND a capture pattern missed, the composed note places "the capture was trimmed at the head, so early output is missing" immediately before "no output matched --capture …", without saying the miss was computed against the FULL untrimmed log. Captures do scan the full log (pinned by the 'reads the UNTRIMMED log' test), so the null is authoritative — but a consuming agent reading the two clauses back to back reasonably infers the value may have sat in the missing head, and either discounts the "addressed by assumption" warning or burns a re-run recovering output that was never lost. The one combination where the untrimmed-read design pays off — the docs' own "loudest runs are the ones most likely to need it" — is the one where the report undermines its own guarantee. Probed: a ~300 KB completed drive with a missed capture reproduces the note with no full-log scoping; appending the clause below flip-tested green (39/39).
| ? ` — no output matched --capture ${missed.map((n) => JSON.stringify(n)).join(', ')}, so ${missed.length === 1 ? 'that value is' : 'those values are'} null rather than measured; anything addressed by ${missed.length === 1 ? 'it' : 'them'} was addressed by assumption` | |
| ? ` — no output matched --capture ${missed.map((n) => JSON.stringify(n)).join(', ')} (captures are matched against the full log, not the trimmed capture above), so ${missed.length === 1 ? 'that value is' : 'those values are'} null rather than measured; anything addressed by ${missed.length === 1 ? 'it' : 'them'} was addressed by assumption` |
中文说明
当一次 completed 驱动的日志超过 200 KB 报告上限、且某个 capture 未命中时，组合出的 note 会把 "the capture was trimmed at the head, so early output is missing" 紧挨着 "no output matched --capture …" 放出，却没有说明 miss 是对完整未裁剪日志判定的。capture 确实扫的是完整日志（有 'reads the UNTRIMMED log' 测试锁定），所以这个 null 是权威的——但消费方 agent 连着读这两句，很容易推断该值可能落在被裁掉的头部，于是要么低估 "addressed by assumption" 的警告，要么白白重跑一轮去恢复从未丢失的输出。未裁剪读取设计最值钱的那种组合——文档自己说的"最吵的运行最需要它"——恰恰是报告自我拆台的场合。实测：~300 KB 的 completed 驱动 + 未命中 capture 可复现该 note，且没有 full-log 限定；追加建议中的限定子句后探针翻转、套件仍全绿（39/39）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Deferred to the next round. Verified in the note composition: when a completed drive's log is trimmed AND a capture missed, the two clauses sit back to back without saying the miss was computed against the full untrimmed log. The proposed scoping clause is the right fix; it is deferred only to keep this round's batch at eight findings, and will land next round.
延后到下一轮。已在 note 组合处核实：当一次 completed 驱动的日志被裁剪且某个 capture 未命中时，两个子句紧挨着出现，没有说明 miss 是对完整未裁剪日志判定的。建议的限定子句是正确的修复；延后只是为了把本轮批量控制在 8 条，将在下一轮实现。
| const out: Record<string, string | null> = {}; | ||
| for (const { name, re } of specs) { | ||
| const m = re.exec(output); | ||
| out[name] = m ? (m[1] ?? m[0]) : null; |
There was a problem hiding this comment.
[Suggestion] The documented "null, never ''" distinction — an empty printed value IS a measurement, preserved here by ?? — is pinned by no test. Verified by mutation: a ?? → || change, which converts an empty group-1 match into the whole match, passes the entire suite (no input anywhere in it produces a match where group 1 is the empty string), while a one-line probe flips — extractCaptures('value: ', parseCaptureSpecs(['v=value: (.*)']).specs) returns {"v":""} on shipped code and {"v":"value: "} under the mutant. A future refactor then silently turns empty-value measurements into whole-match values, collapsing exactly the semantics the DriveReport docblock promises. The probe is the test:
expect(
extractCaptures('value: ', parseCaptureSpecs(['v=value: (.*)']).specs),
).toEqual({ v: '' });中文说明
"null，绝不是 ''"这条约定——打印出空值也是一次测量，由这里的 ?? 保留——没有任何测试锁定。变异验证：把 ?? 改成 ||（空 group-1 会变成整个匹配），整套测试依然通过（套件里没有任何输入能产生 group 1 为空串的匹配）；而一行探针即可翻转——extractCaptures('value: ', parseCaptureSpecs(['v=value: (.*)']).specs) 在线上代码返回 {"v":""}，在变异体下返回 {"v":"value: "}。未来某次重构会把"空值测量"悄悄变成"整体匹配值"，恰恰坍缩 DriveReport docblock 承诺的语义。探针即测试：
expect(
extractCaptures('value: ', parseCaptureSpecs(['v=value: (.*)']).specs),
).toEqual({ v: '' });— qwen3.8-max via Qwen Code /review (v0.21.15)
| export function parseCaptureSpecs( | ||
| raw: readonly string[] | undefined, | ||
| ): { specs: CaptureSpec[] } | { error: string } { | ||
| if (!raw || raw.length === 0) return { specs: [] }; |
There was a problem hiding this comment.
[Suggestion] A bare --capture with no specs parses to [] and silently runs a capture-less drive — the one malformed shape that escapes the "rejects rather than skips" rule parseCaptureSpecs documents for every other shape. Probed against this repo's yargs (17.7.2) with this command's own builder: --capture as the last argument, --capture --out x.json, and an empty array expansion (--capture "${SPECS[@]}" with an empty array) all yield capture: [], and parseCaptureSpecs([]) is byte-identical to parseCaptureSpecs(undefined) — so the command runs the full drive (up to --timeout 300 s) and returns a report with no captured field and no error, indistinguishable from the flag never being passed. Bad name, bad pattern, duplicate name and over-cap sets are all refused pre-flight precisely so a malformed request "must not be able to disguise itself"; a bare flag disguises itself as "asked for nothing", and the caller — per the brief text this PR adds, which branches on captured presence and null — cannot tell the capture was requested and silently lost.
| if (!raw || raw.length === 0) return { specs: [] }; | |
| if (!raw) return { specs: [] }; | |
| if (raw.length === 0) { | |
| return { error: '--capture was given but holds no name=<regex> pair.' }; | |
| } |
中文说明
不带任何 spec 的裸 --capture 会解析成 []，然后静默地执行一次无 capture 的驱动——这是唯一逃脱 parseCaptureSpecs 所声明的"拒绝而非跳过"规则的畸形形态。用本仓库的 yargs（17.7.2）+ 本命令自己的 builder 实测：--capture 作为最后一个参数、--capture --out x.json、以及空数组展开（数组为空时的 --capture "${SPECS[@]}"）都得到 capture: []；parseCaptureSpecs([]) 与 parseCaptureSpecs(undefined) 逐字节相同——于是命令照常跑完整个驱动（最长 --timeout 300 秒），返回一份没有 captured 字段、也没有任何错误的报告，与从未传过该 flag 无法区分。坏名字、坏 pattern、重名、超数都在启动前被拒绝，正是为了"畸形请求不能伪装自己"；而裸 flag 伪装成了"什么都没要"，调用方——按本 PR 新增 brief 文本对 captured 在场性与 null 的分支逻辑——无法察觉 capture 是被请求了却被静默丢掉的。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| * Measured: a full verification cycle spent on a daemon that was not the one | ||
| * under test. | ||
| * | ||
| * `null` for a pattern that never matched, never `''` — a service that did |
There was a problem hiding this comment.
[Suggestion] This invariant — null for a pattern that never matched, never '' — has a hole: a pattern that can match empty yields '' when the expected content is absent. Probed on the shipped code: pattern pid=(\d*) against the log line pid=abc matches zero-width right after the =, group 1 is '', and extractCaptures returns {"pid":""} — the value this docblock, the test comment ("null, never ''") and the added verify-brief text ("null means it never matched") reserve for a service that PRINTED an empty value, while "did not print what it was expected to print" is supposed to read as null. Any */optional quantifier in capture position ((\d*), (\S*)) hits this; the taught \d+/\S+ forms don't, which is why the suite never exercises it — and the miss-note filter (v === null) skips '' too, so nothing names the miss. One resolution: reject at parse time patterns whose REPORTED part can match empty — note the check must test group 1 when the pattern has one (the whole pattern pid=(\d*) cannot match empty; its group can) — or, if empty-matching patterns are deliberate, correct this docblock to say so.
中文说明
这条不变量——pattern 从未匹配时为 null，绝不是 ''——有个漏洞：能匹配空的 pattern，在期望内容缺席时返回 ''。在线上代码实测：pattern pid=(\d*) 对日志行 pid=abc 会在 = 后零宽匹配，group 1 为 ''，extractCaptures 返回 {"pid":""}——而这个值是 docblock、测试注释（"null, never ''"）与新增 verify brief（"null means it never matched"）留给"服务打印了空值"的语义；"没有打印期望的内容"本应读作 null。任何捕获位置带 */可选量词的 pattern（(\d*)、(\S*)）都会命中；教给 agent 的 \d+/\S+ 形态不会，所以套件从未演练到——而且 miss-note 过滤器（v === null）也跳过 ''，于是什么都不点名。一种解法：在解析期拒绝上报部分能匹配空的 pattern——注意要检查的是 group 1（若 pattern 有捕获组）：整体 pattern pid=(\d*) 并不能匹配空，是它的捕获组能——或者，如果空匹配 pattern 是有意支持的，就把这段 docblock 改得与之相符。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Reproduced on the shipped code: pid=(\d*) against pid=abc returns {"pid":""} — the value the docblock reserves for a service that PRINTED an empty value. Deferred to the next round because the resolution is a semantic call: reject empty-matching patterns at parse time (the check must isolate group 1, since pid=(\d*) as a whole cannot match empty) or correct the documented invariant to say empty-matching patterns can yield ''. That decision deserves a dedicated round rather than a rushed choice inside a batch of eight.
已在线上代码复现：pid=(\d*) 对 pid=abc 返回 {"pid":""}——而 docblock 把该值保留给打印了空值的服务。延后到下一轮，因为修复是一个语义决策：在解析期拒绝可空匹配的 pattern（检查必须隔离 group 1，因为 pid=(\d*) 整体并不能匹配空），或者修正文档化的不变量、说明可空匹配 pattern 可以产生 ''。这个决策值得专门一轮，而不是在八条批量中仓促选择。
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review round 1 — summaryRound 1 triaged 11 inline Fixed this round
Deferred to the next round (replies posted on their threads)
No conflicts with the base branch ( VerificationCommands actually run this round, on the final tree (exit codes recorded):
Mutation probes — every guard/branch this round adds is witnessed by a test this round commits (mutate → focused suite must FAIL → restore → green):
Pre-fix reproduction probes (built
中文说明评审第 1 轮 — 总结第 1 轮共分诊了自动评审的 11 条行内 本轮已修复
延后到下一轮（已在各自线程回复）
与 base 分支无冲突（ 验证本轮在最终代码树上实际执行的命令（记录退出码）：
变异探针 — 本轮新增的每个守卫/分支都有本轮提交的测试作证（变异 → 聚焦套件必须失败 → 恢复 → 全绿）：
修复前的复现探针（在已提交 commit 的构建产物
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
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. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 10.
Test Plan (not a blocker): 38 passed — this review observed 22845 passed; 285 passed — this review observed 22845 passed; 2 passed — this review observed 22845 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/drive.ts:671 — [probe] D2-1 CLI seam (yargs → handler cast → DriveArgs.capture) has no test — a DriveArgs.capture rename silently disables --capture end-to-end while the suite stays green and tsc exits 0packages/cli/src/commands/review/drive.ts:615 — [probe] D2-2 captureNote guard tested only for the plural-miss case — a missed.length>0 → captured mutant survives the suite and appends a wrong miss note to every all-matched runpackages/cli/src/commands/review/drive.ts:596 — [probe] D2-3 a completed drive can report null for a value the service printed in its final flush — race probed live (2/400 rounds hit; 0/400 with a post-sentinel re-read)packages/cli/src/commands/review/drive.ts:615 — [probe] D2-4 the miss note states null in absolute terms on timed-out/overflowed drives, where it means 'not printed within the observed window', contradicting the docblock/brief 'never matche…packages/cli/src/commands/review/drive.ts:671 — [probe] D2-5 a bare --capture beside a valued one silently vanishes in yargs, escaping the rejects-rather-than-skips guard the autofix round's test comment claims is closed; nargs: 1 flips it …packages/cli/src/commands/review/drive.ts:266 — [probe] D2-6 patterns compile flagless, so ^/$ are whole-string anchors and $ does not match before a trailing newline — line-anchored patterns silently null; compiling with 'm' fixes it with …packages/cli/src/commands/review/drive.ts:240 — [probe] D2-7 --no-capture parses to [false] and parseCaptureSpecs throws a raw TypeError with no report written, instead of the designed refusal; fetch-pr.ts already guards the identical sibli…
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 10 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：38 passed — this review observed 22845 passed; 285 passed — this review observed 22845 passed; 2 passed — this review observed 22845 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| for (const { name, re } of specs) { | ||
| const m = re.exec(output); |
There was a problem hiding this comment.
[Suggestion] R1-1: These caller-supplied regexes still run against the full untrimmed log (up to the ~8 MiB file cap) with no bound on running time, in a phase --timeout does not cover — extraction happens after the poll loop exits. Round 1 filed this; the fix commit answered with documentation (the MAX_CAPTURE_PATTERN comment and the verify brief's linearity prose), and the mechanism is unchanged: a nested-quantifier pattern well under the 200-char cap hangs extraction with no report ever written. Measured in this worktree: x=(a+)+$ (14 chars) against a 28-char near-miss input spent 8,698 ms in a single re.exec; by the diff's own ~×3.5-per-+2-chars estimate a ~40-char near-miss is hours at 100% CPU. The realistic trigger is not malice — the verify brief this PR adds sends agents to author their own patterns, and a nested quantifier is a plausible accident there. One symptom is new this round: the warning lives only in the brief prose and the source comment; the --capture --help describe says nothing about linearity, so every caller the brief doesn't reach gets no warning anywhere. A worker-based deadline was declined last round as disproportionate — fair under Simplicity First — but the mitigation currently sits one consumer too shallow: at minimum put the linearity warning where every pattern author reads it (the yargs describe), or reject a quantified group whose body itself holds a quantifier.
中文说明
这些由调用方提供的正则仍然在完整未裁剪的日志（上限约 8 MiB）上运行，且运行时间没有任何约束，而提取发生轮询循环退出之后——--timeout 覆盖不到的阶段。第 1 轮已提出此问题；修复 commit 以文档回应（MAX_CAPTURE_PATTERN 注释与 verify brief 中的"保持线性"说明），机制本身未变：一个远低于 200 字符上限的嵌套量词 pattern 就能让提取挂死，且不产生任何报告。在本 worktree 实测：x=(a+)+$（14 字符）对 28 字符的近似未命中输入，单次 re.exec 耗时 8,698 ms；按 diff 自己的测量（每 +2 字符约 ×3.5），约 40 字符的近似未命中就是 100% CPU 数小时。现实触发不是恶意——本 PR 新增的 verify brief 正是让 agent 自行编写 pattern 的地方，嵌套量词在那里是可能失手写出的。本轮新增一个症状：警告只存在于 brief 文本与源码注释中；--capture 的 --help describe 对线性只字未提，brief 触达不到的调用方在任何地方都看不到警告。上一轮已以"与本命令不成比例"为由否决了 worker 截止时间机制——在 Simplicity First 下这合理——但当前缓解措施放浅了一层：至少把线性警告放到每个 pattern 作者都会读到的地方（yargs describe），或在解析期拒绝"量词包裹的组体内再含星词"的 pattern。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const parsed = parseCaptureSpecs(args.capture); | ||
| if ('error' in parsed) { |
There was a problem hiding this comment.
[Suggestion] R1-4: This early return hand-copies the nine-field "unavailable / nothing started" report literal — after this PR, runDrive holds five near-identical copies (server name, capture parse, tmux -V, not-ready, session-create failure), and the copies already drift: the not-ready and create-failed sites vary readyAfterMs/killedStale, so a reader cannot tell which fields are deliberately varied and which are stale copies. Any future change to the early-return report shape — this PR itself just added a field to DriveReport — must be applied at all five sites. Round 1 filed this and the autofix round verified and deferred the consolidation to its next round; still standing at HEAD. A small factory absorbs all copies:
function unavailableReport(note: string): DriveReport {
return {
outcome: 'unavailable',
observed: false,
exitCode: null,
readyAfterMs: null,
droveForMs: 0,
output: '',
truncated: false,
killedStale: false,
note,
};
}中文说明
这个 early return 手抄了那份九字段的 "unavailable / nothing started" 报告字面量——本 PR 之后，runDrive 中已有五处几乎相同的拷贝（server name、capture parse、tmux -V、not-ready、session-create failure），且拷贝之间已经开始漂移：not-ready 与 create-failed 两处的 readyAfterMs/killedStale 各不相同，读者无法分辨哪些字段是刻意不同、哪些是抄旧的。未来任何对 early-return 报告形状的改动——本 PR 自己就刚给 DriveReport 加了一个字段——都必须同时应用到五处。第 1 轮已提出，autofix 轮核实后明确延后到下一轮收敛；HEAD 上仍未处理。一个小工厂可以吸收全部拷贝（需要变动 readyAfterMs/killedStale 的调用处可覆盖写或以可选参数传入）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Deferred to the follow-up queue, not dropped. Verified real: the three identical unavailable early returns plus the not-ready/create-failed siblings that vary readyAfterMs/killedStale. The consolidation is a pure style refactor, and this PR's round-2 decision already recorded deferring it out of the defect-fixing rounds so it can be reviewed as a refactor on its own terms in a follow-up PR; the round-3 re-post keeps it on the record rather than requesting it in-round, so it is recorded for the follow-up queue here.
中文说明
已延后到 follow-up 队列，未丢弃。已核实属实：三处完全相同的 unavailable early return，外加各自变动 readyAfterMs/killedStale 的 not-ready / create-failed 两处。该合并是纯风格重构，本 PR 第 2 轮已明确记录将其延后到缺陷修复轮之外、以便在独立的重构 PR 中按重构本身来评审；第 3 轮的重发也是将其保留在记录上而非要求本轮处理，故在此记入 follow-up 队列。
| outcome === 'overflowed' | ||
| ? `the drive wrote more than ${Math.round(LOG_MAX_BYTES / 1024 / 1024)} MiB and was stopped — no exit code is reported because it never gave one, and a run this command had to stop is not evidence about the diff either way. Quieten the script, or have it manage its own output file.` | ||
| : outcome === 'completed' | ||
| ? `drove for ${Math.round(droveForMs / 1000)}s and reached its sentinel with exit ${exitCode}${readyAfterMs === null ? '' : ` (ready after ${Math.round(readyAfterMs / 1000)}s)`}${truncated ? '; the capture was trimmed at the head, so early output is missing' : ''}` |
There was a problem hiding this comment.
[Suggestion] R1-8: The note composition still does not reconcile capture extraction with head-trimming. When a completed drive's log exceeded the 200 KB report cap, the note says "the capture was trimmed at the head, so early output is missing" — but captures are extracted from the UNTRIMMED log before trimCapture, so beside that clause the report simultaneously presents captured values read from exactly the missing head. Two entrances now: the round-1 one (a missed pattern's "addressed by assumption" clause sits back-to-back with the trim clause without saying the miss was computed against the full untrimmed log), and the matched case observed this round — the test reads the UNTRIMMED log... ships precisely truncated: true with a populated captured and the unreconciled note. A witness quoting the report then inherits what reads as a self-contradiction: "early output is missing" next to the value that came out of the early output. Append a reconciliation clause whenever truncated is true and captures were taken, e.g. "(captured values are read from the untrimmed log and survive the trim)".
中文说明
note 的组合仍然没有把 capture 提取与头部裁剪的关系说清楚。当一次 completed 驱动的日志超过 200 KB 报告上限时，note 写着 "the capture was trimmed at the head, so early output is missing"——但 capture 是在 trimCapture 之前从未裁剪的日志里提取的，于是报告一边说头部输出缺失，一边又给出恰好来自那段缺失头部的 captured 值。现在有两个入口：第 1 轮指出的那个（未命中 pattern 的 "addressed by assumption" 子句与裁剪子句紧挨着出现，没有说明 miss 是对完整未裁剪日志判定的），以及本轮观察到的命中场景——测试 reads the UNTRIMMED log... 恰好固化了 truncated: true 且 captured 有值、note 措辞未做任何调和的组合。引用该报告的 witness 会继承一个读起来自相矛盾的报告："头部输出缺失"旁边摆着从头取出的值。建议在 truncated 为真且有 capture 时追加一句调和子句，例如 "(captured values are read from the untrimmed log and survive the trim)"。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| * | ||
| * `null` for a pattern that never matched, never `''` — a service that did | ||
| * not print what it was expected to print is a different fact from one that | ||
| * printed an empty value, and only the second is a measurement. |
There was a problem hiding this comment.
[Suggestion] R1-11: The documented "null for a pattern that never matched, never ''" invariant still has its hole: a pattern that can match empty yields '' when the expected content is absent — pid=(\d*) against a log holding pid=abc returns {"pid":""} (reproduced on the shipped code in round 1; the m[1] ?? m[0] code is unchanged at HEAD), which is the value the docblock reserves for a service that PRINTED an empty value. The resolution remains the semantic call the autofix round deferred to this one: reject empty-matching patterns at parse time (the check must isolate group 1, since pid=(\d*) as a whole cannot match empty), or correct the documented invariant to say empty-matching patterns can yield ''. Either way, the docblock and the behaviour should agree before a witness relies on the null/'' distinction.
中文说明
文档化的不变量"没匹配上是 null，绝不是 ''"仍然有漏洞：一个可以匹配空的 pattern，在期望内容缺席时会产出 ''——pid=(\d*) 对含有 pid=abc 的日志返回 {"pid":""}（第 1 轮已在 shipped 代码上复现；HEAD 上 m[1] ?? m[0] 代码未变），而这个值在 docblock 里是保留给"服务打印了空值"的。修复仍是 autofix 轮延后到本轮的那个语义决策：在解析期拒绝可空匹配的 pattern（检查必须隔离 group 1，因为 pid=(\d*) 整体并不能匹配空），或者修正文档化不变量、说明可空匹配 pattern 可以产生 ''。无论选哪边，在 witness 依赖 null/'' 区分之前，docblock 与行为应当一致。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const out: Record<string, string | null> = {}; | ||
| for (const { name, re } of specs) { | ||
| const m = re.exec(output); | ||
| const v = m ? (m[1] ?? m[0]) : null; |
There was a problem hiding this comment.
[Suggestion] A pattern whose group 1 is optional but did NOT participate in the match silently falls through ?? to the whole match — so a value the service never printed is reported as a measurement, and being non-null it also escapes the miss note. Distinct from the R1-11 hole (a group that participates with the empty string): here the group never matched at all, and the report presents the pattern's own literal prefix as the measured fact. Concrete: --capture 'port=port (\d+)?' against a log holding port pending returns {"port":"port "} — a witness quoting captured then addresses "port " as the bound port, the exact disguised-measurement outcome parseCaptureSpecs's docblock says the design exists to prevent. Probed at this commit: HEAD returns {"port":"port "} with missed: []; with the fix below, {"port":null} with missed: ["port"] (named by the note), and all 43 shipped drive tests still pass. The docblock's "Group 1 when the pattern has one, the whole match otherwise" argues the no-group state; m.length is the exec array's declared-group count plus one regardless of participation, so it cleanly separates "declared a group" from "declared none".
| const v = m ? (m[1] ?? m[0]) : null; | |
| const v = m ? (m.length > 1 ? (m[1] === undefined ? null : m[1]) : m[0]) : null; |
中文说明
当 pattern 的 group 1 是可选的、且没有参与本次匹配时，?? 会静默落到整个匹配——于是服务从未打印的值被当成测量结果上报；而且因为它非 null，还逃过了 miss note。这与 R1-11 的漏洞不同（那是 group 以空串参与匹配）；这里是 group 根本没匹配，报告却把 pattern 自己的字面前缀当作测得的事实。具体例子：--capture 'port=port (\d+)?' 对含有 port pending 的日志返回 {"port":"port "}——引用 captured 的 witness 会把 "port " 当作绑定的端口，这正是 parseCaptureSpecs docblock 声称本设计要防止的"伪装成测量的值"。在本 commit 实测：HEAD 返回 {"port":"port "} 且 missed: []；应用下方修复后返回 {"port":null} 且 missed: ["port"]（被 note 点名），全部 43 个 shipped drive 测试仍通过。docblock 的"带组取 group 1，否则取整个匹配"论证的是无组状态；m.length 是 exec 数组声明的组数加一、与组是否参与无关，可以干净地区分"声明了组"与"没声明组"。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Round 1/2 review. `m[1] ?? m[0]` reads as "group 1, or else the whole match", and for an OPTIONAL group that is a silent substitution rather than a fallback: `(?:a(x))?b` against `b` matches with group 1 absent, so the caller that asked what `x` matched received the whole match `b` under the same name, with nothing in the report saying a different question had been answered. Whether the pattern DECLARES a group and whether group 1 happened to participate are different questions; conflating them loses a value quietly, which is the one failure mode this command exists to remove. The declaration is now settled when the pattern is parsed — by matching `<source>|` against the empty string, guarded, since it builds a second pattern from the first — and a declared-but-unfilled group is `null`. A pattern with no group still yields the whole match. The docblock's `null`-versus-`''` claim was too strong beside that, and the narrowing belongs to the pattern rather than the service: `''` means the group captured zero characters, which a `*`-quantified group does wherever it is anchored (`pid=(\d*)` returns `''` against `pid=abc`). Said so, and named `+` as what a "printed nothing" test needs. Also from the two rounds: - The head-trim clause and the capture block contradicted each other in the note. `output` is trimmed at the head, `captured` is read before that trim, so a completed noisy drive said "early output is missing" directly beside a value that came out of the missing head — and beside a null the reader would reasonably blame the trim for. A scoping clause now says captures read the untrimmed log, covering the matched case (R1-8) and the missed one (D2-2) together. - Four invariants the docblocks state were pinned by nothing: the unfilled-group rule, the empty-capture-is-a-measurement rule, the first-match rule against a log holding two, and the pattern-length cap's upper bound (only its lower bound was tested). - The CLI seam has a test (D2-1). The handler casts `argv as unknown as DriveArgs`, which type-checks whatever the option is called, so a rename of either side silently disabled `--capture` end to end while every runDrive test stayed green and tsc exited 0. It now drives the real builder with a real flag string and asserts the value reaches the report the handler prints. Each of these was checked by mutation: reverting the group rule, the option name, the first-match rule or the scoping clause reds exactly its own test, and restoring them returns 50 passed.
|
Rounds 1 and 2 addressed in The one that was a real defectR1-11 / the A caller that asked what The R1-8 + D2-2 — the note contradicted itselfBoth are one issue. The four unpinned invariants, and the CLI seamAdded tests for the unfilled-group rule, empty-capture-is-a-measurement, the first-match rule against a log holding two occurrences, and the pattern-length cap's upper bound (only its lower bound was tested). D2-1 is closed properly rather than by assertion: the handler casts VerificationEvery fix was checked by mutation rather than by the suite going green — reverting each reds exactly its own test:
Two I am deliberately not doing, with reasonsR1-4 — the four R1-1 — no time bound on a caller-supplied regex. 中文说明两轮意见已在 唯一的真实缺陷是 R1-11。 该 finding 的 R1-8 与 D2-2 是同一个问题的两面。 四条无人钉住的不变量都补了测试：未填充组规则、空捕获是一次测量、对含两次出现的日志取首次匹配、以及 pattern 长度上限的上界（此前只测了下界）。D2-1 是真正关掉而非口头保证：handler 里的 验证方式是变异而不是"套件绿了"：回退分组规则 / 改选项名 / 首次改末次 / 去掉限定子句，各自打红对应用例，全部还原后 50 passed； 两条我有意不做并说明理由：
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 6b": running packages/cli vitest for drive.test.ts — the shared worktree has no node_modules and a cold monorepo install+build exceeded the budget; substituted sta….
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/drive.test.ts:653 — [probe] MAX_CAPTURES accept side untested — a > → >= mutant survives the suite; deferred under the code-age rule (code unchanged since round 2, which did not flag it)packages/cli/src/commands/review/drive.ts:303 — [probe] flagless compilation makes ^/$ whole-string anchors — still-standing re-discovery of round 2's deferred D2-6, code unchanged since it was flagged
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 6b"：running packages/cli vitest for drive.test.ts — the shared worktree has no node_modules and a cold monorepo install+build exceeded the budget; substituted sta…。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| for (const { name, re, hasGroup } of specs) { | ||
| const m = re.exec(output); |
There was a problem hiding this comment.
[Suggestion] R1-1: These caller-supplied regexes still run against the full untrimmed log (up to the ~8 MiB file cap) with no bound on running time, in a phase --timeout does not cover — extraction happens after the poll loop exits. Round 1 filed this, round 2 re-posted it, and the mechanism is unchanged at this commit: the fix answered with documentation (the MAX_CAPTURE_PATTERN comment and the brief's linearity prose), but nothing enforces linearity. A nested-quantifier pattern well under the 200-char cap passes every parseCaptureSpecs gate and can hang extraction on a near-miss log; the hang lands after tmux cleanup but before the report is built, so no report and no --out file are ever written. Measured at this commit:
v=(a+)+$ — accepted by parseCaptureSpecs (9 chars, under the 200 cap)
near-miss exec timings: 22 a's = 48 ms | 24 = 183 ms | 26 = 756 ms (~x4 per +2 chars)
The realistic trigger is not malice — the verify brief this PR adds sends agents to author their own patterns, and a nested quantifier is a plausible accident there. The caller already supplies --script and --ready as arbitrary shell, so this is not a trust boundary, only a robustness gap. The worker deadline was declined in earlier rounds as disproportionate — fairly, under Simplicity First — but the cheaper residuals stay open: the --capture --help describe still says nothing about linearity, so every caller the brief doesn't reach gets no warning anywhere. At minimum put the warning where every pattern author reads it, or reject a quantified group whose body itself holds a quantifier at parse time.
中文说明
这些由调用方提供的正则仍然在完整未裁剪的日志（上限约 8 MiB）上运行，且运行时间没有任何约束，而提取发生在轮询循环退出之后——--timeout 覆盖不到的阶段。第 1 轮已提出、第 2 轮已重发，本 commit 上机制未变：修复以文档回应（MAX_CAPTURE_PATTERN 注释与 brief 中的"保持线性"说明），但没有任何强制线性的机制。一个远低于 200 字符上限的嵌套量词 pattern 能通过 parseCaptureSpecs 的全部门槛，遇到近似未命中的日志即可让提取挂死；挂死发生在 tmux 清理之后、报告生成之前，因此报告和 --out 文件都不会被写出。在本 commit 实测：v=(a+)+$（9 字符，远低于 200 上限）可被接受；近似未命中耗时 22 个 a = 48 ms、24 个 = 183 ms、26 个 = 756 ms（每 +2 字符约 ×4）。现实触发不是恶意——本 PR 新增的 verify brief 正是让 agent 自行编写 pattern 的地方，嵌套量词是可能失手写出的。调用方本来就提供 --script 与 --ready 两段任意 shell，所以这不是新增信任边界，只是健壮性缺口。worker 截止时间机制在前几轮已以"与本命令不成比例"为由被否决——在 Simplicity First 下合理——但更便宜的缓解仍未做：--capture 的 --help describe 对线性只字未提，brief 触达不到的调用方在任何地方都看不到警告。至少把警告放到每个 pattern 作者都会读到的地方，或在解析期拒绝"量词包裹的组体内再含量词"的 pattern。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| output: '', | ||
| truncated: false, | ||
| killedStale: false, | ||
| note: `${parsed.error} Nothing was started.`, |
There was a problem hiding this comment.
[Suggestion] R1-4: This early return hand-copies the nine-field "unavailable / nothing started" report literal — runDrive now holds four near-identical copies (server name, capture parse, tmux -V, plus the not-ready/create-failure siblings that vary readyAfterMs/killedStale), and the copies already drift, so a reader cannot tell which fields are deliberately varied and which are stale copies. Any future change to the early-return report shape — this PR itself just added a field to DriveReport — must be applied at all four sites. The author explicitly declined this in-PR (a style refactor in a defect-fixing round) and proposed a follow-up refactor PR; this re-post keeps the finding on the record until the consolidation lands. A small factory absorbs all copies:
function unavailableReport(note: string): DriveReport {
return {
outcome: 'unavailable', observed: false, exitCode: null,
readyAfterMs: null, droveForMs: 0, output: '',
truncated: false, killedStale: false, note,
};
}中文说明
这个 early return 手抄了那份九字段的 "unavailable / nothing started" 报告字面量——runDrive 中现在已有四处几乎相同的拷贝（server name、capture parse、tmux -V，外加各自变动 readyAfterMs/killedStale 的 not-ready/create-failure 两处），且拷贝之间已经开始漂移，读者无法分辨哪些字段是刻意不同、哪些是抄旧的。未来任何对 early-return 报告形状的改动——本 PR 自己就刚给 DriveReport 加了一个字段——都必须同时应用到四处。作者已明确表示不在本 PR 内修（缺陷修复轮里不做风格重构），并提议作为独立的重构 PR 处理；本条重发将该 finding 保留在记录上，直到合并落地。一个小工厂可以吸收全部拷贝（需要变动字段的调用处覆盖写即可）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Same disposition as the round-2 comment: deferred to the follow-up queue together with it. The finding is verified real, the re-post itself states it exists to keep the finding on the record until the consolidation lands, and the consolidation belongs to a follow-up refactor PR rather than to this defect-fixing PR.
中文说明
与第 2 轮的评论同一处置：与其一并延后到 follow-up 队列。该 finding 已核实属实，重发本身也说明其作用是在合并落地前把 finding 保留在记录上，而合并属于后续重构 PR，不属于这个缺陷修复 PR。
| const trimScopeNote = | ||
| captured && truncated | ||
| ? ' — --capture reads the untrimmed log, so the head-trim above does not reach it and a null capture is a miss against the whole run rather than against what survived the trim' | ||
| : ''; |
There was a problem hiding this comment.
[Suggestion] The new reconciliation clause (added this round to close R1-8) is gated on captured && truncated, but both of its claims only hold on the completed outcome. The clause it points at — "the capture was trimmed at the head, so early output is missing" — is emitted only in the completed branch, so on timed-out and overflowed drives "the head-trim above" has no antecedent anywhere in the note. On overflowed the second claim is false: the run was stopped at the 8 MiB log cap, extraction only saw the first ~8 MiB, and a value printed after that point is invisible — yet the note asserts a null capture "is a miss against the whole run". Probed at this commit with a drive overflowing the log cap:
outcome: overflowed | truncated: true | captured: {"baseUrl":null}
note has head-trim antecedent: false | whole-run claim: true
— while the log on disk ends 'listening on http://127.0.0.1:9999'
A witness quoting that null as proof the service never printed the address would be wrong — the harness killed the run before it could. Gate the clause on the outcome that carries its antecedent, or reword it non-anaphorically; if the overflowed case deserves reconciliation it needs its own wording — the file cap, not the head-trim, is what bounds what the capture saw there.
| const trimScopeNote = | |
| captured && truncated | |
| ? ' — --capture reads the untrimmed log, so the head-trim above does not reach it and a null capture is a miss against the whole run rather than against what survived the trim' | |
| : ''; | |
| const trimScopeNote = | |
| captured && truncated && outcome === 'completed' | |
| ? ' — --capture reads the untrimmed log, so the head-trim above does not reach it and a null capture is a miss against the whole run rather than against what survived the trim' | |
| : ''; |
中文说明
这条新增的调和子句（本轮为关闭 R1-8 而加）以 captured && truncated 为条件，但它的两个主张只在 completed 结果下成立。它指向的那句——"the capture was trimmed at the head, so early output is missing"——只在 completed 分支输出，因此 timed-out 与 overflowed 的 note 里 "the head-trim above" 找不到任何先行词。在 overflowed 下第二个主张是假的：运行在 8 MiB 日志上限处被停止，提取只看到了前 ~8 MiB，在那之后打印的值根本不可见——而 note 却断言 null 是"对整次运行的未命中"。在本 commit 用一次撑爆日志上限的 drive 实测：outcome: overflowed | truncated: true | captured: {"baseUrl":null}，note 无 head-trim 先行词却声称 whole-run，而磁盘上的日志明明以 listening on http://127.0.0.1:9999 结尾。引用该 null 作为"服务从未打印过地址"之证据的 witness 会是错的——是 harness 在打印发生前杀掉了运行。把条件收窄到携带先行词的结果（见 suggestion），或改写为不依赖上文的自足表述；若 overflowed 也需要调和，需单独措辞——那里限制 capture 视野的是文件上限，不是头部裁剪。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| writeFileSync( | ||
| logPath, | ||
| `listening on http://127.0.0.1:8432\n${'x'.repeat(400_000)}`, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The driveWithLog helper exists precisely to fake the tmux lifecycle (new-session writes the log, then the sentinel file), but it is declared inside describe('--capture'), so this reconcile test re-implements the whole ~25-line protocol (mkdtemp / logPath / workDir / exec / runDrive / rmSync) a second time, and the seam test further down re-implements the exec factory a third. If runDrive's exec surface changes — the sentinel file name drive.rc, the sentinel line format, or a new lifecycle exec call — three sites must be updated in lockstep; a missed one keeps modelling the old protocol while still passing, so the suite certifies behaviour the real drive no longer has. The file already has the counter-pattern: the module-scope harness helper is shared across describes, including by this PR's own "no reconciliation" test. Hoist driveWithLog to module scope next to harness, and factor the fake-tmux exec out as a small factory the seam test also calls.
中文说明
driveWithLog 这个 helper 正是为了伪造 tmux 生命周期（new-session 写日志、再写 sentinel 文件）而存在的，但它被声明在 describe('--capture') 内部，于是这个 reconcile 用例第二次手抄了整套约 25 行的协议（mkdtemp / logPath / workDir / exec / runDrive / rmSync），下面的 seam 用例又第三次手抄了 exec 工厂。如果 runDrive 的 exec 面发生变化——sentinel 文件名 drive.rc、sentinel 行格式、或新增生命周期 exec 调用——三处必须同步更新；漏掉的一处会继续按旧协议建模却仍然通过，套件便为真实 drive 已不再具备的行为背书。文件里已有反例可循：模块级 harness helper 被各 describe 共享，本 PR 自己的 "no reconciliation" 用例就在用。把 driveWithLog 提升到模块级（放在 harness 旁），并把伪造 tmux 的 exec 抽成一个小工厂，让 seam 用例也调用它。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| --capture 'baseUrl=listening on (https?://\\S+)' --timeout 300 --out <plan dir>/drive.json | ||
| \`\`\` | ||
|
|
||
| Each pattern lands in the report's \`captured\` as group 1 (or the whole match), and **\`null\` means it never matched** — which is the report telling you that anything you addressed by that value, you addressed by assumption. Quote the captured address in the witness rather than the one in your command line: the first is what the run produced, the second is what you hoped. Keep patterns linear — no nested quantifiers like \`(a+)+\`: extraction runs once the drive has ended, where no \`--timeout\` reaches, and one backtracking pattern hangs the whole run with no report written. |
There was a problem hiding this comment.
[Suggestion] This sentence was falsified by this PR's own final commit: a pattern that matched while a declared capture group was left unfilled now also yields null — the distinction the DriveReport docblock added in the same commit states explicitly ("the pattern never matched, or it declares a group the match left unfilled"). Probed at this commit: extractCaptures('b', specsOf(['v=(?:a(x))?b'])) returns {"v":null} — the pattern matched and the value is still null. A verify agent who authors a pattern with an optional capturing group, gets null, and trusts this sentence concludes "the service never printed the line" — reporting a phantom service defect in the witness, or re-running the drive chasing a log line that is present. The yargs describe in drive.ts carries the same simplification (separate comment). Reword to match the docblock: "null means nothing was captured — the pattern never matched, or its declared group did not participate in the match".
中文说明
这句话被本 PR 自己的最后一个 commit 证伪了：pattern 匹配成功、但其声明的捕获组未参与匹配时，现在同样得到 null——同一 commit 新增的 DriveReport docblock 明确写出了这一区分（"the pattern never matched, or it declares a group the match left unfilled"）。在本 commit 实测：extractCaptures('b', specsOf(['v=(?:a(x))?b'])) 返回 {"v":null}——pattern 匹配了，值却仍是 null。一个编写了含可选捕获组 pattern 的 verify agent，拿到 null 后若相信这句话，就会得出"服务从未打印过该行"的结论——在 witness 里报告一个幻影般的服务缺陷，或者重跑 drive 去追一条实际存在的日志行。drive.ts 的 yargs describe 也有同样的简化（另见对应评论）。建议改为与 docblock 一致的表述："null means nothing was captured — the pattern never matched, or its declared group did not participate in the match"。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| type: 'array', | ||
| string: true, | ||
| describe: | ||
| "name=<regex> read back out of this run's own output, e.g. baseUrl=listening on (https?://\\S+). Repeatable. Group 1 when the pattern has one, the whole match otherwise; null when nothing matched. Use it for anything the service CHOSE rather than was told — above all the address it actually bound.", |
There was a problem hiding this comment.
[Suggestion] Same stale semantics as the verify brief (separate comment): "null when nothing matched". Since this PR's final commit, null is also produced when the pattern matched but its declared group did not participate in the match — the --help text is where every caller who never reads the brief learns the flag, and it now teaches the wrong boundary. Mirror the docblock's wording: "null when nothing was captured — the pattern never matched, or its declared group did not participate".
中文说明
与 verify brief 中的问题相同（另见对应评论）："null when nothing matched"。自本 PR 最后一个 commit 起，pattern 匹配成功但其声明的组未参与匹配时也会得到 null——--help 文本是不读 brief 的调用方了解该 flag 的唯一入口，它现在教的是错误的边界。请与 docblock 措辞对齐："null when nothing was captured — the pattern never matched, or its declared group did not participate"。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // WHICH one before deciding whether the rest still stands. | ||
| const captureNote = | ||
| missed.length > 0 | ||
| ? ` — no output matched --capture ${missed.map((n) => JSON.stringify(n)).join(', ')}, so ${missed.length === 1 ? 'that value is' : 'those values are'} null rather than measured; anything addressed by ${missed.length === 1 ? 'it' : 'them'} was addressed by assumption` |
There was a problem hiding this comment.
[Suggestion] The miss note says "no output matched --capture " for every null, but since this PR's final commit missed also contains names whose pattern DID match with an unfilled declared group — the prose collapses the two cases the code elsewhere carefully separates (the null-vs-whole-match fix landed this round exists precisely to keep them apart). Probed at this commit through runDrive with log b and capture v=(?:a(x))?b:
captured: {"v":null}
NOTE >>> ... — no output matched --capture "v", so that value is null rather than measured; ...
— beside the direct-exec proof that the pattern matches "b"
A maintainer (or agent) greps the log, sees the pattern text plainly present, and concludes the extraction logic is broken rather than that the group did not participate — debugging the harness instead of the pattern. Phrase the cause neutrally:
| ? ` — no output matched --capture ${missed.map((n) => JSON.stringify(n)).join(', ')}, so ${missed.length === 1 ? 'that value is' : 'those values are'} null rather than measured; anything addressed by ${missed.length === 1 ? 'it' : 'them'} was addressed by assumption` | |
| ? ` — --capture produced no value for ${missed.map((n) => JSON.stringify(n)).join(', ')} (the pattern never matched, or its group did not participate), so ${missed.length === 1 ? 'that value is' : 'those values are'} null rather than measured; anything addressed by ${missed.length === 1 ? 'it' : 'them'} was addressed by assumption` |
中文说明
未命中 note 对每个 null 都说 "no output matched --capture "，但自本 PR 最后一个 commit 起，missed 也会包含"pattern 匹配了、但声明的组未参与"的名字——运行时措辞把代码其他处处小心区分的两种情况混为一谈（本轮落地的 null-vs-整体匹配修复，存在的意义正是区分它们）。在本 commit 通过 runDrive 实测（日志为 b、capture 为 v=(?:a(x))?b）：captured: {"v":null}，NOTE 写着 "no output matched --capture "v""——而直接 exec 证明该 pattern 确实匹配 b。维护者（或 agent）grep 日志，明明看到 pattern 文本就在，会以为是提取逻辑坏了，而不是组未参与——调试的是 harness 而非 pattern。请中性地表述原因（见 suggestion）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Round summaryRound on PR #9655 (head Findings and dispositionsImplemented[rc:3832601052] The reconciliation clause fired on outcomes its claims do not hold on. Reproduced on the pre-round code with a probe: an [rc:3832601072] The miss note said "no output matched" for a pattern that DID match. Reproduced: [rc:3832601067] The verify brief's " [rc:3832601069] The [rc:3832601040] + [rc:3832060569] R1-1 residual: the linearity warning never reached the [rc:3832601062] The fake-tmux protocol was modelled three times in the tests. Hoisted Deferred to the follow-up queue[rc:3832601048] + [rc:3832060575] R1-4: four near-identical Re-verified as already resolved (commit e903274)
No action this round
Changes
No conflict notes — Verification
中文说明本轮摘要PR #9655 的一轮（HEAD Findings 与处置已实现[rc:3832601052] 调和子句在其主张不成立的结果上也触发了。 已在本轮之前的代码上用 probe 复现：一次 [rc:3832601072] 未命中 note 对一个确实匹配了的 pattern 说 "no output matched"。 已复现： [rc:3832601067] verify brief 里的 " [rc:3832601069] [rc:3832601040] + [rc:3832060569] R1-1 遗留：线性警告从未出现在 [rc:3832601062] 伪造 tmux 的协议在测试里被建模了三次。 已把 延后到 follow-up 队列[rc:3832601048] + [rc:3832060575] R1-4：四处近乎相同的 复核确认已解决（commit e903274）
本轮不处理
改动内容
无冲突说明—— 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
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. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/drive.ts:240 — [probe] declaresGroup/hasGroup redundant — exec result length already carries the answer — deferred under the code-age rule (code unchanged since round 3, which read it and did not flag it)packages/cli/src/commands/review/drive.test.ts:764 — [probe] CAPTURE_VALUE_MAX boundary unpinned — comparator and constant mutants survive — deferred under the code-age rule (code unchanged since round 3, which read it and did not flag it)packages/cli/src/commands/review/lib/agent-briefs.ts:713 — [probe] no guidance that ^/$ anchor the whole log — $-anchored patterns silently null — rediscovery of round 2's deferred D2-6, compile site unchanged since flaggedpackages/cli/src/commands/review/drive.test.ts:689 — [probe] MAX_CAPTURES acceptance boundary unpinned — off-by-one and drift mutants survive — rediscovery of round 3's deferred entry, code unchanged since flaggedpackages/cli/src/commands/review/drive.ts:639 — [probe] one-write-behind stale read — final log writes can miss captured unrecoverably — rediscovery of round 2's deferred D2-3, poll loop unchanged since flagged
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| for (const { name, re, hasGroup } of specs) { | ||
| const m = re.exec(output); |
There was a problem hiding this comment.
[Suggestion] R1-1: These caller-supplied regexes still run against the full untrimmed log (up to the ~8 MiB file cap) with no bound on running time, in a phase --timeout does not cover — extraction happens after the poll loop exits. Re-measured this round: (a+)+$ (9 chars, under the 200-char cap) takes 711 ms against 26 as + b and 2.9 s at 28 (roughly doubling per character), so a ~40-char near-miss is on the order of hours at 100% CPU with no report written and no timeout to fire; even a single large-count quantifier with no nesting — a{1000000}b, 13 chars — took 34 s against a 200 KB non-matching string. The PR documents this tradeoff in three places and prior rounds accepted the author's call to warn rather than enforce; this re-post keeps the finding on the record. If it ever bites in practice, the fixes are a hard deadline around extraction (worker thread, reporting the affected captures as null with a note) or rejecting the common exponential shapes at parse time.
witness: (a+)+$ vs 26 a's + b -> 711 ms; vs 28 a's -> 2.9 s (~x2 per character)
a{1000000}b vs 200 KB non-matching string -> 34 s (this round, same machine)
中文说明
R1-1：这些由调用方提供的正则仍然在 --timeout 覆盖不到的阶段、对完整的未裁剪日志（上限约 8 MiB）无时长上限地运行——提取发生在轮询循环退出之后。本轮重新测量：(a+)+$（9 个字符，低于 200 字符上限）对 26 个 a 加 b 耗时 711 ms，28 个 a 耗时 2.9 s（约每字符翻倍），因此一个约 40 字符的"接近匹配"就是小时级的 100% CPU——没有报告写出，超时也永不触发；即使是不带嵌套的单个大量词重复——a{1000000}b，13 个字符——对一份 200 KB 的不匹配串也耗时 34 s。本 PR 已在三处把该权衡写入文档，此前各轮也接受了作者"只警示、不强制"的决定；本次重发只为把该发现留在记录上。若它在实际中真的造成问题，修法是给提取过程加硬性时限（worker thread，把受影响的 capture 报告为 null 并附说明），或在解析期拒绝常见的指数级形状。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Deferred to the follow-up queue, keeping the finding alive past merge — not declined.
Verified the measurement against current code this round: (a+)+$ (6 chars, far under the 200-char cap) takes ~0.7–1.1 s against 24–26 repeated chars + b on this runner, roughly doubling per added character, and extractCaptures runs it after the poll loop exits, where --timeout does not reach. The hazard is real.
The disposition stays as accepted in earlier rounds, though: this is a deliberately documented tradeoff rather than an oversight — the cap's doc comment, the --capture CLI help, and the verify brief all name the nested-quantifier shape and tell pattern authors to keep patterns linear, and prior rounds accepted the author's warn-don't-enforce call. The enforcement options named here (a hard deadline around extraction via worker thread with affected captures reported as null, or rejecting common exponential shapes at parse time) are a hardening feature in their own right, outside this PR's purpose of reporting the address a drive's service actually bound — so they go to the follow-up queue rather than into round 4 of a converging PR.
中文说明
转入 follow-up 队列（延后项清单）以让该发现在合并后仍然保留——不是拒绝。
本轮在当前代码上复测确认：(a+)+$（6 个字符，远低于 200 字符上限）对 24–26 个重复字符加 b 在本运行环境耗时约 0.7–1.1 秒，约每增加一个字符翻倍，而 extractCaptures 在轮询循环退出之后才运行它，--timeout 覆盖不到。风险是真实存在的。
但处置维持此前各轮已接受的决定：这是刻意写入文档的权衡，而非疏漏——上限常量的注释、--capture 的 CLI 帮助文本、验证简报都点名了嵌套量词形状并告知模式作者保持线性，此前各轮也接受了作者"只警示、不强制"的决定。此处点名的强制手段（用 worker thread 给提取过程加硬性时限、把受影响的 capture 报告为 null，或在解析期拒绝常见的指数级形状）本身是一个独立的加固特性，超出本 PR "报告 drive 服务实际绑定地址" 的目的——因此进入 follow-up 队列，而不是在一个正在收敛的 PR 的第 4 轮里实施。
| output: '', | ||
| truncated: false, | ||
| killedStale: false, | ||
| note: `${parsed.error} Nothing was started.`, |
There was a problem hiding this comment.
[Suggestion] R1-4: This early return hand-copies the nine-field "unavailable / nothing started" report literal — after this PR, runDrive holds five near-identical copies (server name, capture parse, tmux -V, not-ready, session-create failure). Any change to the error-path report shape must be applied to every copy by hand: this PR's own new optional field captured is deliberately absent from all five early returns, but nothing enforces that — the next optional DriveReport field can silently diverge between the error paths (present on one, missing on another) without tsc flagging it, since the field is optional. Round 2's disposition deferred the consolidation to the follow-up queue; this re-post keeps the finding on the record until it lands. The fix remains a small local helper (e.g. unavailable(note, killedStale?, readyAfterMs?)) used by all the early returns.
中文说明
R1-4：这个 early return 手抄了那份九个字段、全零值的 "unavailable / nothing started" 报告字面量——本 PR 之后，runDrive 里已有五份几乎相同的拷贝（server name、capture 解析、tmux -V、not-ready、会话创建失败）。任何对错误路径报告形状的改动都必须手工同步到每一份拷贝：本 PR 自己新增的可选字段 captured 刻意缺席于全部五个 early return，但没有任何机制保证这一点——下一个加入 DriveReport 的可选字段可能在各错误路径之间悄悄分叉（这份有、那份没有），而由于字段可选，tsc 不会发现。第 2 轮已把该合并延后进 follow-up 队列；本次重发只为把该发现留在记录上，直到合并落地。修法仍然是提一个小的本地 helper（如 unavailable(note, killedStale?, readyAfterMs?)），供所有 early return 共用。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Deferral kept — refreshed under this round's comment id so it survives merge.
Verified against current code: runDrive holds five near-identical early-return report literals (server name, capture parse, tmux -V, readiness timeout, session-create failure), this PR added the capture-parse one, and its new optional captured field is absent from all five by convention only — tsc cannot flag divergence on an optional field, so the next one can silently differ between error paths.
The fix (a single local helper used by all the early returns) remains exactly as scoped in round 2, which routed it to the follow-up queue; this re-post keeps the finding on the record until it lands there, so the disposition is unchanged rather than reopened in round 4 of a converging PR.
中文说明
维持延后——以本轮的评论 id 重新登记，使其在合并后仍然保留。
已在当前代码上核实：runDrive 中有五份几乎相同的 early return 报告字面量（server name、capture 解析、tmux -V、就绪超时、会话创建失败），本 PR 新增了其中 capture 解析这一份，且其新增的可选字段 captured 仅仅出于约定而缺席于全部五份——tsc 无法对可选字段的分叉报错，因此下一个可选字段可能在各错误路径之间悄悄不一致。
修法（供所有 early return 共用的一个本地 helper）维持第 2 轮已界定的范围；第 2 轮已将其转入 follow-up 队列，本次重发只为把该发现留在记录上直到在那里落地，因此处置不变，不在一个正在收敛的 PR 的第 4 轮重新开启。
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no code changes. Round 4 feedback on PR #9655 carries no failed checks, no still-red checks, no Each finding was verified against the current code before classification:
Working tree is unchanged; no commit was made. Thread replies ( 中文说明Autofix 审查轮次：无代码变更。 PR #9655 的第 4 轮反馈中没有失败检查、没有持续失败的检查、没有 每条发现在分类前都已在当前代码上核实：
工作区未变更；未创建提交。线程回复（ Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 128 passed · 0 failed · 128 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：128 通过 · 0 失败 · 128 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportVerification report — PR #9655
|
| Cell | Oracle | Head | Base (control) |
|---|---|---|---|
C1 motivating: log holds port 8931 is in use, trying 8932... + listening on http://127.0.0.1:8932 at the head, then 400 KB of noise; --capture baseUrl=listening on (http://\S+) |
captured.baseUrl, output, note |
completed; captured.baseUrl = http://127.0.0.1:8932, value contains no 8931; output head-trimmed (no listening on line); note carries both the trim clause and the reconciliation clause |
completed; no captured key; no capture machinery in note — the bound address is unknowable |
C2 no --capture |
report keys + note | keys without captured; note unchanged |
identical key set and note (timing normalised) — byte-identical modulo the two timing fields |
C3 malformed pattern baseUrl=[unclosed |
outcome + exec-call count | unavailable, not a valid regular expression, Nothing was started., 0 exec calls (not even tmux -V) |
runs the whole drive to completed, exec calls > 0, no captured facts — the malformed ask costs the drive |
| C4 two unmatched patterns | captured + note |
both null; note names "baseUrl" and "pid", says addressed by assumption |
no captured key |
| C5 timed-out, noisy log | captured + note scoping |
captured.baseUrl present on a non-completed outcome; no untrimmed-log reconciliation clause (scoped to completed) |
no captured key |
| C10 overflowed (9 MB) | captured + note scoping |
captured.baseUrl = null; no reconciliation clause |
— |
| B5/A/A real exec, tmux absent | note | unavailable, tmux is not available… |
identical note — the gap is environmental, not a regression |
Real-CLI cells on the built bundle (node dist/cli.js review drive), witness evidence/04-cli-cells-real-bundle.png: malformed pattern → exit 1 with the JSON unavailable report in milliseconds (rejection provably precedes any drive); valid pattern without tmux → the same environmental unavailable as the base A/A.
Boundary probes on the changed expressions (name/pattern/set caps at 31|32, 200|201, 8|9; null vs ''; unfilled group; first match; value cap at 4096|4097; named groups; astral values): 33/33, witness evidence/02-unit-boundary-matrix.png.
Corrections (description vs measured code)
- Test counts in the body are stale. The "How to verify" table says
drive.test.ts 38 passed (29 before, 9 new); the measured suite at the verified head is 53 passed (53) (logs/m0-control.log). 38 matches the state after commit 1; commit 3's message says 50, and the final commit adds three more scoping tests. The code is right; the description was not refreshed. - "Captures are taken on every outcome" overstates. True for
completed/timed-out/overflowed(the outcomes where the script ran). Onnot-readyandunavailablethe early-return reports omitcapturedentirely and the note never mentions the request (repro in F1). The docblock's absence rule ("absent entirely when nothing was captured") is self-consistent with that behaviour; it is the commit-message sentence that overreaches.
Findings (non-blocking)
F1 — Suggestion: a requested capture silently vanishes on not-ready/unavailable
The PR's own philosophy is that the moment a witness would quote a value the run never produced, the report must name it. On the two early-return outcomes the request disappears without a trace:
runDrive({ ready: 'curl …', readyTimeout: 0, capture: ['baseUrl=listening on (\\S+)'], … })
→ { outcome: 'not-ready', … } // no `captured` key
has captured key: false
note mentions capture: false
(repro: runDrive through the exec seam with a failing readiness probe and capture: ['baseUrl=…'] — the printed report above is the raw output). The not-ready note does say "nothing was driven, so nothing here is evidence about the diff", which a careful reader can extend to the capture — but a caller branching on 'captured' in report cannot distinguish "asked for nothing" from "asked, but nothing could run", and the miss-note machinery built for exactly this moment stays silent. Minimal fix: on the two early returns where a valid capture set was parsed, append the same named clause, e.g. — --capture requested N pattern(s) but nothing was driven, so nothing was captured. No test pins this today; the fix should ship with a fixture (a not-ready drive that names the request in the note).
F2 — Suggestion: the new verify-brief paragraph is pinned by nothing
agent-prompt.test.ts (285) and run-skill-parity.test.ts (2) pass and exercise the brief builders, but no assertion fails if the paragraph is deleted from BRIEFS['verify'] — the capability half the PR calls "deliberate rather than incidental" is one prose edit away from silent removal. A one-line containment assertion in agent-prompt.test.ts (the verify brief contains Address what the service BOUND and the --capture example) would pin it the way the stale-bundle suite pins its notice.
Note (not a finding): the ReDoS residual is real, disclosed, and bounded as described
The ladder (evidence/05-redos-ladder.png, logs/redos-ladder.log): (a+)+$ against 'a'×N + '!' takes 19.8 / 69.5 / 284 / 1301 / 4589 ms for N = 18/20/22/24/26 — ×3.5–4.6 per +2 chars, confirming the docblock's "~×3.5" and its "hours at ~40 chars" extrapolation. Extraction runs after the poll loop, so --timeout does not reach it. This is not a new trust boundary: the only writers of --capture patterns are the verifier agent via the brief (grep-verified; no code path constructs captures from external text), and that same caller already supplies --script and --ready as arbitrary shell. The CLI describe, the docblock, and the brief all warn "keep patterns linear". Self-DoS by a trusted caller, accurately disclosed — recorded here as quantification, not as a defect.
Mutation matrix (vacuity of the new tests)
Witness: evidence/03-mutation-matrix-from-logs.png (rendered from the per-mutant vitest logs in logs/). Suite at head: 53 tests.
| Mutant | Result | Red tests |
|---|---|---|
| M0 unmutated control | 53 passed (53) | — |
M1 extract from trimCapture(output).text |
2 failed | reads the UNTRIMMED log…; says captures survive the trim… |
M2 unmatched → '' instead of null |
5 failed | names an unmatched pattern…, prefers group 1…, keeps an empty capture as ""…, +2 |
M3 validation after the tmux -V probe |
1 failed | refuses a malformed pattern before starting anything |
M4 m[1] ?? m[0] (no declared-group rule) |
2 failed | a declared group the match left unfilled is null…, phrases a miss neutrally… |
| M5 last match instead of first | 2 failed | both FIRST-match tests |
M6 drop outcome === 'completed' from the reconciliation note |
2 failed | scopes the reconciliation to completed drives, stays silent on a timed-out drive too |
M7 captured always present ({}) |
1 failed | omits the field entirely when nothing was asked for |
| M8 positive control: trim keeps the head | 2 failed | keeps the TAIL when it must trim…, reads the UNTRIMMED log… |
M9 rename yargs option capture → captureX |
1 failed | the CLI-seam test |
9/9 mutants killed, zero survivors; M8 proves the runner can fail tests in this file. The PR's three claimed mutations reproduce (with higher red counts where later commits added tests — consistent with Correction 1, not a contradiction).
Targeted gates
drive.test.ts: 53/53 (logs/m0-control.log)agent-prompt.test.ts+run-skill-parity.test.ts: 287/287 (logs/gates-agent-prompt-parity.log)tsc --noEmit -p packages/cli/tsconfig.json: 0 diagnostics (logs/gates-typecheck.log)eslinton the three changed files: clean (logs/gates-eslint.log); gate proven live — a planted unused variable was caught (logs/gates-eslint-live.log)- Brief wiring: compiled
BRIEFS['verify']carries the paragraph, the example, and the quantifier warning; base source has zero occurrences (logs/gates-brief-wiring.log)
Not covered
- Full tmux end-to-end drive. The container has no tmux and no root/sudo to install one; the A/A control (B5) shows both arms fail identically, so this is environmental, not a regression. The seam-free surface that exists without tmux (pre-probe rejection, tmux-absent reporting, CLI wiring) is covered by the real-CLI cells.
- Per-commit attribution. The checkout is depth 2: only the merge commit, base tip, and PR head are reachable, while the metadata lists 4 commits. The aggregate
HEAD^1..HEADdiff is what was verified; per-commit claims (e.g. which commit added which tests) are cited from commit messages only. - ReDoS ladder beyond N=26 (4.6 s). Higher rungs are extrapolation of the measured curve; each rung ran under
timeout 30. - Windows/macOS behaviour (the suite skips its real-bash block on win32; this round ran on Linux only).
- The other two residuals of review: residual gaps in the live-service witness arm, and a graft for coexistence claims #9446 — declared out of scope by the PR and not touched by the diff.
- The mutation matrix covers
drive.tsguards only; the brief paragraph's vacuity is reported as F2 rather than mutated. - First run of the unit matrix had 2 failures that were my harness's off-by-one fixtures (name cap is 32 total; a stray literal
^in an input); fixed and re-run green — the PR code was never at fault.
Methodology
Environment: node:22 CI container, merge-ref checkout (HEAD merge, HEAD^1 base, HEAD^2 PR head), pre-built at HEAD. The A/B compiled drive.ts from head and from a scratch HEAD^1 worktree with identical esbuild configs; the changed logic was driven through the unit's own exec seam (log text supplied faithfully), while real-exec and real-CLI cells covered the seam-free paths. Unit matrix and brief wiring ran against the compiled head bundle. Mutation runs edited packages/cli/src/commands/review/drive.ts in place, ran npx vitest run src/commands/review/drive.test.ts, and restored byte-identically (verified with cmp after every mutant; final git status clean). Raw logs live in logs/, harnesses in harness/ (rerunnable), evidence PNGs in evidence/ via scripts/verify-capture.mjs.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/cli/src/commands/review/drive.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/drive.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/drive.test.ts: PPPPP
verdict: pass
summary: 1 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/drive.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/drive.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/drive.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/drive.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/drive.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. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Found one blocking correctness issue in the new capture workflow; details inline.
| // a drive that timed out still bound its port, and the address it bound is | ||
| // often the fact that explains why the rest of it went nowhere. | ||
| const captured = | ||
| captureSpecs.length > 0 ? extractCaptures(output, captureSpecs) : undefined; |
There was a problem hiding this comment.
[P1] Post-run capture cannot prevent stale-process measurements. captured is computed only after the drive loop has ended and the finally block has killed tmux, so neither the readiness probe nor the driven script can consume captured.baseUrl. With 8931 occupied, a script can launch qwen serve --port 8931, let it fall back and print 8932, yet still send every probe/request to the stale process on 8931. This path reaches the sentinel and returns observed: true plus captured.baseUrl: ...8932 with no contradiction; the new verifier brief then tells the witness to quote 8932, misattributing readings from 8931 to the new daemon—the exact false evidence this change is meant to eliminate. The workflow needs to make the bound address available before requests are issued (or explicitly require and verify that the script parses and uses it); extracting it only for the final report is too late.
There was a problem hiding this comment.
You are right, and this was the worst kind of finding to get: the PR made the failure it set out to remove harder to see. Fixed in 8a7bb7a.
I confirmed the ordering you describe — extraction runs after the drive loop and after finally kills tmux, so captured cannot reach a request the script already made. Combined with the brief's instruction to quote the captured address in the witness, a script that talks to 8931 while the service logs 8932 returns observed: true, captured.baseUrl: …8932, no contradiction anywhere — and the witness attributes 8931's numbers to the daemon on 8932. Before this PR the same run would at least have quoted the address it actually read. That is a regression, not a shortfall.
What I did not do: make drive supply the address to the run. That means drive starting the service itself, which is the live-service command scoped in #9446 — a much larger change than this PR, and rushing it into round 5 is how the next P1 gets written.
What I did: take the authority away from the report and put the mechanism back in the script — your second alternative ("explicitly require and verify that the script parses and uses it"). The brief no longer says "capture the address". It requires the script to derive the address from the service's own output before its first request, to bind ephemeral wherever the service allows one so the fallback never fires at all, and demotes --capture to showing that the address the script used is the one the service printed. It states the trap in your terms — reading a capture as though it could reach earlier requests is worse than not capturing — and spells the recipe out rather than describing it, because the failure it replaces is one of omission:
"${QWEN_CODE_CLI:-qwen}" review drive --cwd <the worktree> --timeout 300 \
--capture 'baseUrl=listening on (https?://\S+)' --out <plan dir>/drive.json \
--script '
<start the service; --port 0 wherever it allows one> > svc.log 2>&1 &
until grep -q "listening on" svc.log; do sleep 0.2; done
BASE=$(sed -n "s|.*listening on \(https\{0,1\}://[^ ]*\).*|\1|p" svc.log | head -1)
curl -s "$BASE/<the endpoint the claim is about>"
'captured is now documented in the report type as a record of the run, never an input to it, with your scenario spelled out there too — that is where a future reader is most likely to form the wrong model.
Two things I checked by running rather than reading, since this recipe is what agents will copy:
- the
sedand the--capturepattern were run against reallistening onlines, http and https, and return the same value — the property that makes the capture corroborate the script instead of competing with it; - the brief was rendered and re-read after the edit, and again after eslint flagged an unnecessary
\$escape inside it, to confirm$BASEstill reaches the agent verbatim.
340 passed across drive / agent-prompt / run-skill-parity, tsc 0 errors, eslint clean. No behaviour change in this commit — the defect was in what the workflow authorised, so the fix is where the authorisation was.
If you would rather this PR not ship the capture guidance at all until live-service can supply the address before the requests, say so and I will cut the brief paragraph and leave --capture as a reporting-only field.
中文说明
你是对的，而且这是最糟的一类 finding：这个 PR 让它本要消灭的失败更难被看见。已在 8a7bb7a 修复。
我确认了你描述的时序——提取发生在 drive 循环结束、finally 杀掉 tmux 之后，所以 captured 够不到脚本已经发出的请求。再加上 brief 让 verifier「在 witness 里引用 captured 地址」，一个对 8931 说话、而服务日志写着 8932 的脚本，会返回 observed: true、captured.baseUrl: …8932，全程没有任何矛盾——witness 于是把 8931 的数字算到 8932 那个 daemon 头上。而在本 PR 之前，同样的运行至少引用的是它真正读过的地址。这是回归，不是不足。
我没有做的：让 drive 在请求发出前把地址交给运行。那意味着由 drive 自己启动服务，也就是 #9446 里规划的 live-service——比这个 PR 大得多，在第 5 轮硬塞进来正是下一个 P1 的写法。
我做的：把权威从报告拿走、把机制放回脚本，也就是你给的第二个选项。brief 不再说「capture 地址」，而是要求脚本在第一个请求之前从服务自己的输出里取出地址；服务允许就绑定临时端口，让回退根本不会发生；--capture 降级为「证明脚本用的地址正是服务打印的那个」。它用你的说法写明了这个陷阱，并且把 recipe 直接写出来而不是描述，因为它要替代的失败本身就是「漏做」。
captured 在报告类型里现在写明是运行的记录，绝不是运行的输入，你的场景也写在那里——那是未来读者最容易形成错误心智模型的地方。
有两件事我是跑出来而不是读出来的，因为这段 recipe 是 agent 会照抄的：sed 与 --capture 正则对真实的 listening on 行（http 与 https）返回同一个值——正是这个性质让 capture 与脚本互相印证而不是互相竞争；以及 brief 在改完之后、以及 eslint 指出其中一处多余的 \$ 转义之后，都重新渲染并通读过，确认 $BASE 仍然原样送到 agent 手里。
drive / agent-prompt / run-skill-parity 共 340 passed，tsc 0 error，eslint 干净。本次提交无行为变更——缺陷在于这套工作流授权了什么，所以修复就落在授权发生的地方。
如果你更希望在 live-service 能于请求前提供地址之前、这个 PR 干脆不要带 capture 指引，说一声，我会把 brief 那一段删掉，只保留 --capture 作为纯报告字段。
There was a problem hiding this comment.
Follow-up on 8a7bb7a, because the two checks I reported above stopped one step short: they ran the sed and the --capture pattern against the same listening on lines, but not the recipe end to end. Done now — real tmux, a real HTTP service with the fallback behaviour, a squatter on the requested port — and the recipe has one defect of its own.
The operational half holds. A verbatim run of the recipe derives $BASE from svc.log before its first request and addresses the fallback port: the service under test received the GET, the squatter received nothing during the drive, and the response body identifies the right process. The P1's mechanism is closed for a conforming script.
But captured.baseUrl comes back null on every conforming run. Extraction reads the drive's own log — the script's stdout — and the recipe redirects the service's output into svc.log, which nothing ever surfaces. listening on never reaches the log --capture scans:
{ "outcome": "completed", "exitCode": 0,
"output": "PROBE: {\"daemon\":\"REAL\",\"version\":\"2.0\",\"port\":18942}\n",
"captured": { "baseUrl": null },
"note": "… --capture produced no value for \"baseUrl\" … anything addressed by it was addressed by assumption" }So the recipe demotes exactly the runs that follow it to assumption-mode — the miss note denies the evidence the script legitimately produced, and the docblock's "what this field then adds is evidence that the address it used is the one the service printed" is false for the canonical usage. This is R1-3's degradation shape again, reintroduced by the fix. The repair is one line in the recipe — surface the log after readiness:
until grep -q "listening on" svc.log; do sleep 0.2; done
cat svc.logRe-run with that line, same trap: captured.baseUrl: "http://127.0.0.1:18952", output shows the probe landing on the same port, squatter silent — the corroborating shape the brief promises.
Worth folding into the recipe while touching it: have the script also print the address it is about to use and pin it with a second pattern —
echo "BASE_USED=$BASE"
# --capture 'usedUrl=BASE_USED=(\S+)'With both baseUrl (what the service printed) and usedUrl (what the script addressed) in captured, a non-conforming run's report contradicts itself instead of corroborating itself — which is the one thing the current design cannot do: I re-ran the pre-fix naive shape against this build and it still completes with observed: true, the fallback address in captured, and every reading from the squatter, nothing mechanical flagging it. Observed in a live agent run against the new brief: the agent invented exactly this dual-capture unprompted, so the recipe would be codifying a move the guidance already induces.
中文说明
对 8a7bb7a 的跟进——因为我上面报告的那两项"跑出来的"检查恰好停在缺陷前一步：它们把 sed 和 --capture 正则跑在同样的 listening on 行上，却没有端到端跑过 recipe 本身。现在补上了——真实 tmux、带回退行为的真实 HTTP 服务、被请求端口上蹲一个 squatter——recipe 自身有一个缺陷。
操作那一半成立。 逐字执行 recipe 的运行会在第一个请求前从 svc.log 派生 $BASE 并寻址回退端口：被测服务收到了 GET，squatter 在 drive 期间零请求，响应体自证是正确的进程。对守约的脚本，P1 的机制已经关闭。
但每个守约的运行拿到的都是 captured.baseUrl: null。 提取读的是 drive 自己的日志——脚本的 stdout——而 recipe 把服务输出重定向进了 svc.log，没有任何东西把它带回来。listening on 永远到不了 --capture 扫描的日志（证据见上方 JSON）。
于是 recipe 恰好把照它做的运行降级成假设模式——miss note 否认了脚本刚刚合法产出的证据，docblock 里"这个字段提供的是脚本所用地址正是服务打印地址的证据"对标准用法而言是假的。这正是 R1-3 说的退化形态，被修复自己重新引入。修法是 recipe 里加一行：readiness 之后 cat svc.log。加上重跑、同一陷阱：captured.baseUrl: "http://127.0.0.1:18952"，output 显示探测落在同一端口、squatter 静默——brief 承诺的互证形态。
顺手值得并入 recipe 的一项：让脚本把它即将使用的地址也打印出来并用第二个 pattern 钉住（echo "BASE_USED=$BASE" + --capture 'usedUrl=BASE_USED=(\S+)'）。captured 里同时有 baseUrl（服务打印的）和 usedUrl（脚本寻址的）之后，不守约的运行的报告会自相矛盾而不是自我印证——这是当前设计做不到的唯一一件事：我在这个构建上重跑了修复前的 naive 形态，它依然 observed: true、captured 里是回退地址、所有读数来自 squatter，没有任何机械环节报警。并且在一次针对新 brief 的真实 agent 运行里观察到：agent 无提示地自己发明了这个双 capture——recipe 写进去只是把指引已经诱导出的动作固化下来。
There was a problem hiding this comment.
The cat-before-request repair landed in 0eb58b9 and is pinned by the recipe test (mutation-verified: reverting to the prior recipe, or keeping mktemp while dropping only the cat, both fail it). The dual-capture addition (echo "BASE_USED=$BASE" + --capture 'usedUrl=BASE_USED=(\S+)') is deferred to #9710: the self-contradiction property it buys is real and worth having, but it widens a recipe that is now pinned by tests, in a round whose first obligation is repairing the gate rejection of the stale-tail fix. Recorded there, not dropped.
There was a problem hiding this comment.
Independently re-verified at b54d07b with the same trap harness that produced the original finding (squatter on the requested port, service falling back +1): the new recipe run verbatim returns completed with captured.baseUrl holding the fallback address, the probe landing on the real daemon, and the squatter receiving zero requests — the corroborating shape the brief promises, no charity needed. The three load-bearing details hold up under the harness too, and two of them close corners the finding did not name (mktemp keeping the reviewed worktree clean; cat-before-request making first-match win over a response body quoting an address). The #9710 deferral of the usedUrl dual capture is the right disposition — recorded is enough; this thread's defect is closed.
中文说明
已在 b54d07b 上用产出原 finding 的同一陷阱 harness（squatter 占被请求端口、服务 +1 回退）独立复验：新 recipe 逐字执行返回 completed，captured.baseUrl 为回退地址，探测命中真实 daemon，squatter 零请求——正是 brief 承诺的互证形态，无需任何善意解读。三个承重细节在 harness 下也都成立，其中两个还关掉了原 finding 没点名的角（mktemp 保持被审 worktree 干净；cat 前置使 first-match 压过引用地址的响应体）。usedUrl 双 capture 延后至 #9710 是正确处置——有记录即可；本线程的缺陷已闭合。
@yiliang114's P1. Captures are extracted after the drive loop has ended, so nothing in `captured` can reach a request the script already made — and the brief told the verifier to quote the captured address in the witness anyway. That combination is worse than not capturing at all: a script that talks to 8931 while the service logs its fallback to 8932 reaches its sentinel, returns `observed: true` beside `captured.baseUrl: …8932` with nothing contradicting it, and the witness then attributes 8931's readings to the daemon on 8932. Before this change the same run would at least have quoted the address it really read. The mechanism that prevents the wrong-process read lives in the script, not in the report, and the brief now says so: derive the address from the service's own output before the first request, bind ephemeral wherever the service allows one so the fallback never fires, and let `--capture` show that the address the script used is the one the service printed. `null` is named as "never measured", explicitly not as permission to fall back to the address on the command line. The recipe is spelled out rather than described, because the failure it replaces is one of omission. The report carries the same caveat where it cannot be missed: `captured` is documented as a RECORD of the run and never an input to it. Verified rather than read: the recipe's `sed` and the `--capture` pattern were run against real `listening on` lines, http and https, and agree on the same value — which is the property that makes the capture corroborate the script instead of competing with it. The rendered brief was printed and re-read after the edit, and again after eslint caught an unnecessary `\$` escape in it, to confirm `$BASE` still survives to the agent verbatim. No behaviour change: 340 passed across drive, agent-prompt and run-skill-parity; tsc 0 errors; eslint clean.
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 explored to full depth (tool budget reached): "agent 1b": none — completed well under the tool ceiling..
Test Plan (not a blocker): 38 passed — this review observed 22858 passed; 285 passed — this review observed 22858 passed; 2 passed — this review observed 22858 passed.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/drive.test.ts:95 — [review] driveExec is a third fake-tmux dispatcher — the docblock's 'lives in ONE place' claim is false as landed — deferred under the code-age rule (code unchanged since round 4, which re…packages/cli/src/commands/review/drive.test.ts:691 — [probe] MAX_CAPTURES accept side unpinned — a > to >= mutant survives the suite (probe-verified) — deferred under the code-age rule (code unchanged since round 4, which read it and did no…packages/cli/src/commands/review/drive.ts:125 — [review] captured docblock 'Absent entirely when nothing was captured' contradicts the code (absent iff nothing was asked for) — deferred under the code-age rule (code unchanged since round 4,…packages/cli/src/commands/review/drive.ts:314 — [probe] ^/$ anchor the whole log, undisclosed at every authoring surface — line-anchored patterns silently null — deferred under the code-age rule (code unchanged since round 4, which read it …
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 1b"：none — completed well under the tool ceiling.。
Test Plan（非阻断）：38 passed — this review observed 22858 passed; 285 passed — this review observed 22858 passed; 2 passed — this review observed 22858 passed。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| "\${QWEN_CODE_CLI:-qwen}" review drive --cwd <the worktree> --timeout 300 \\ | ||
| --capture 'baseUrl=listening on (https?://\\S+)' --out <plan dir>/drive.json \\ | ||
| --script ' | ||
| <start the service; --port 0 wherever it allows one> > svc.log 2>&1 & |
There was a problem hiding this comment.
[Critical] The canonical bound-address recipe routes the service's output to a file --capture never reads, so the feature this PR exists for cannot fire when the recipe is followed. runDrive runs the script as bash <script> > <logPath> 2>&1 and extractCaptures reads only that log — but this redirect sends the service's stdout/stderr to svc.log, so the listening on ... line never reaches the drive log. The script derives BASE from svc.log (the prevention half works), the drive completes, yet captured.baseUrl is null on every faithful run and the miss note claims the value "was never measured" and anything using it "was addressed by assumption" — although the service printed it. The promise below ("captured.baseUrl is the same line the script read") is structurally false: the script reads svc.log, the capture reads the drive log, and the two never intersect. This fails issue #9446 residual (1)'s acceptance criterion — "the address in the report is the one it bound" — in the pipeline's canonical usage.
Witness (real runDrive on tmux 3.4 with the built CLI, fake service binding an ephemeral port):
ARM A (recipe verbatim): "outcome": "completed", "observed": true, "captured": {"baseUrl": null}
note: --capture produced no value for "baseUrl" ... addressed by assumption
(svc.log held "listening on http://127.0.0.1:42513"; curl succeeded against it)
ARM B (one-line counterfactual — the service line also echoed to script stdout):
"captured": {"baseUrl": "http://127.0.0.1:40621"}, clean note
Make the service's line reach the drive log so the evidence producer and the evidence consumer meet in the channel --capture reads — e.g. tee the service output to both sinks (this also closes the decoy hole in the certification sentence below, because the service's startup line then precedes any response-body text under first-match):
| <start the service; --port 0 wherever it allows one> > svc.log 2>&1 & | |
| <start the service; --port 0 wherever it allows one> > >(tee svc.log) 2>&1 & |
中文说明
[Critical] 这段教学 recipe 把服务的输出引到一个 --capture 根本不会读的文件里，因此照着 recipe 走时，这个 PR 唯一存在的功能永远不会触发。runDrive 以 bash <script> > <logPath> 2>&1 运行脚本，而 extractCaptures 只读这份 drive 日志——但此处的重定向把服务的 stdout/stderr 全部送进 svc.log，于是 listening on ... 这行永远到不了 drive 日志。脚本从 svc.log 里取出 BASE（预防的那一半是有效的）、drive 正常完成，但每一次忠实运行里 captured.baseUrl 都是 null，且 miss note 声称该值"从未被测量"、一切基于它的寻址"都是按假设进行的"——尽管服务明明打印过它。下文的承诺（"captured.baseUrl 与脚本读到的是同一行"）在结构上为假：脚本读的是 svc.log，capture 读的是 drive 日志，两者永不相交。这在流水线的规范用法上直接不满足 issue #9446 残余 (1) 的验收标准——"报告里的地址就是服务实际绑定的地址"。
Witness（在 tmux 3.4 上用构建好的 CLI 跑真实 runDrive，假服务绑定临时端口）：ARM A（recipe 原样）："outcome": "completed"、"observed": true、"captured": {"baseUrl": null}，note 为"--capture produced no value for "baseUrl" ... addressed by assumption"——而 svc.log 里就有 listening on http://127.0.0.1:42513，curl 也确实访问成功。ARM B（一行反事实——把服务的那行同时 echo 到脚本 stdout）："captured": {"baseUrl": "http://127.0.0.1:40621"}，note 干净。两臂确定性翻转。
修复方向：让服务的那行输出到达 drive 日志，使证据的生产者与消费者在 --capture 读取的通道里相遇——例如用 tee 把服务输出同时送到两个槽（这同时也堵住了下文认证句的诱饵漏洞：服务启动行会先于任何响应体文本出现，first-match 会优先取到它）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| for (const { name, re, hasGroup } of specs) { | ||
| const m = re.exec(output); |
There was a problem hiding this comment.
[Suggestion] R1-1: These caller-supplied regexes still run against the full untrimmed log (up to the ~8 MiB file cap — the breaking poll iteration reads the file before the cap check, so extraction can receive even more) with no wall-clock bound, in a phase --timeout does not cover: extraction runs after the poll loop exits. MAX_CAPTURE_PATTERN caps pattern length (200), not complexity, so the only ReDoS mitigation is still prose — and the verify brief added by this same diff now tells agents to author their own patterns. Re-measured this round on the shipped code: (a+)+$ (7 chars, far under the cap) against a near-miss grows ~×4 per +2 characters — 2.72 ms (n=19) → 10.95 (21) → 43.59 (23) → 174.57 (25) → 696.45 (27) on Node v22.23.0 — so a ~40-char near-miss is hours at 100% CPU, with no report written and the calling worker lost until killed externally. Deferred to the follow-up queue last round; still standing. Recommend enforcing rather than advising: run the extraction in a worker_threads worker terminated after a wall-clock budget (reporting the capture as null with a named note), and/or reject at parse time the bounded syntactic class the risk lives in (a quantified group whose body contains a quantifier).
中文说明
[Suggestion] R1-1：这些由调用方提供的正则仍然在完整未裁剪的日志（上限约 8 MiB——触发退出判断的那轮轮询先读文件再检查上限，因此提取阶段实际可能拿到更多）上运行，且没有任何墙钟时限；提取发生在轮询循环退出之后，--timeout 已管不到这里。MAX_CAPTURE_PATTERN 只限制 pattern 的长度（200），不限制复杂度，所以唯一的 ReDoS 缓解至今仍是文字提示——而本 diff 新增的 verify brief 恰恰在让 agent 自己编写 pattern。本轮在已发布代码上重新测量：(a+)+$（7 个字符，远低于上限）对近似失配输入约每 +2 字符 ×4 增长——Node v22.23.0 上 2.72 ms (n=19) → 10.95 (21) → 43.59 (23) → 174.57 (25) → 696.45 (27)——即约 40 字符的失配就是数小时的 100% CPU：不写任何报告，调用方 worker 只能被外部杀掉。上一轮已推迟到后续队列，问题仍然存在。建议用机制代替劝告：把提取放进 worker_threads worker 并设墙钟预算（超时就以具名 note 报告该 capture 为 null），和/或在解析期拒绝风险所在的有界语法类（被量化的组、其组体内又含量化符）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Still deferred to the follow-up queue (#9710) — disposition unchanged from rounds 6–7, recorded here on the thread so it stays visible. The finding stands and was re-measured this window: the pattern-length cap does not bound backtracking, and extraction runs after the poll loop exits, where --timeout no longer reaches. A real wall-clock bound means running extraction in a worker_threads worker killed on overrun — its own design; rejecting the bounded syntactic class by inspecting the pattern source is the mirrored-oracle trap and would refuse legitimate patterns. Both reasons are written up in #9710. This round repairs the verification-gate rejection of the stale-tail Critical and deliberately does not expand.
| const parsed = parseCaptureSpecs(args.capture); | ||
| if ('error' in parsed) { | ||
| return { |
There was a problem hiding this comment.
[Suggestion] R1-4: This early return hand-copies the all-zero "unavailable / nothing started" DriveReport literal — after this PR runDrive holds three verbatim all-zero copies (server name, capture parse, tmux -V) plus two near-identical siblings (not-ready, session-create) varying readyAfterMs/killedStale. This PR demonstrates the hazard itself: the new tenth field captured compiles silently in all five only because it is optional. The next required or default-bearing field must be added in five places, and missing one yields a compile error at best, or an early-exit report that silently diverges from completed reports at worst. Still standing from round 1 (deferred to the follow-up queue last round). Suggested consolidation:
function notStartedReport(note: string, over: Partial<DriveReport> = {}): DriveReport {
return {
outcome: 'unavailable', observed: false, exitCode: null, readyAfterMs: null,
droveForMs: 0, output: '', truncated: false, killedStale: false,
note, ...over,
};
}(called from all five exits; not-ready/session-create pass their differing fields via over).
中文说明
[Suggestion] R1-4：这个 early return 手抄了一份全零的 "unavailable / nothing started" DriveReport 字面量——本 PR 之后，runDrive 里有三份逐字相同的全零拷贝（server name、capture parse、tmux -V），外加两份近乎相同的兄弟（not-ready、session-create，差异仅在 readyAfterMs/killedStale）。本 PR 本身就演示了这个隐患：新增的第十个字段 captured 之所以能在五处都静默编译通过，只因为它是可选的。下一个必选或带默认值的字段必须在五个地方同时添加，漏掉一处，最好是编译报错，最坏是 early-exit 报告与 completed 报告悄然分叉。自第 1 轮起仍然存在（上一轮已推迟到后续队列）。建议的合并方式如上（抽取一个 notStartedReport(note, over) 构造器，五处 early return 统一调用，not-ready/session-create 通过 over 传入差异字段）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Still deferred to the follow-up queue (#9710) — the finding stands (this PR demonstrates the hazard itself: the tenth field compiled in all five all-zero literals only because it is optional), but the repair is a five-return-path refactor that wants reviewing as a refactor, and this round is the rejection repair for the stale-tail Critical. Tracked beside R1-1 in #9710.
| "\${QWEN_CODE_CLI:-qwen}" review drive --cwd <the worktree> --timeout 300 \\ | ||
| --capture 'baseUrl=listening on (https?://\\S+)' --out <plan dir>/drive.json \\ | ||
| --script ' | ||
| <start the service; --port 0 wherever it allows one> > svc.log 2>&1 & |
There was a problem hiding this comment.
[Suggestion] This redirect plants svc.log inside the reviewed worktree: the script's cwd is the worktree (new-session -c args.cwd, matching the recipe's own --cwd <the worktree> line), and nothing removes the file — finally deletes only the temp qwen-review-drive-<server> dir. .gitignore does not cover svc.log, so any later capture of the same tree (resume, re-review, local-diff capture) lists it via git ls-files --others --exclude-standard and inlines up to 1 MB of service log into the diff under review as though the PR added it — and a service log is exactly the artifact most likely to contain request/response noise that reads as PR content. Every other artifact in this section goes to <plan dir>. Witness (probe): a real runDrive with --cwd = a fixture repo left svc.log in the driven cwd, and captureLocalDiff on that tree then produced a +++ b/svc.log / new file mode 100644 section containing the service output. Move the log beside the other artifacts (> <plan dir>/svc.log 2>&1 &, with the grep/sed references below pointed at the same path).
中文说明
[Suggestion] 这个重定向会把 svc.log 留在被审查的 worktree 里：脚本的 cwd 就是 worktree（new-session -c args.cwd，与 recipe 自身的 --cwd <the worktree> 一致），而没有任何逻辑会删除该文件——finally 只删除临时的 qwen-review-drive-<server> 目录。.gitignore 不覆盖 svc.log，因此之后对同一棵树的任何捕获（resume、重新审查、local-diff 捕获）都会经 git ls-files --others --exclude-standard 列出它，并把至多 1 MB 的服务日志内联进正在审查的 diff，仿佛它是 PR 新增的——而服务日志恰恰是最可能包含请求/响应噪声、且读起来像 PR 内容的产物。本节的其他所有产物都放在 <plan dir>。Witness（探针）：以 --cwd = fixture 仓库跑真实 runDrive 后，svc.log 留在了被驱动的 cwd；对该树运行 captureLocalDiff 随即产生了 +++ b/svc.log / new file mode 100644 段，内容就是服务输出。建议把日志挪到其他产物旁边（> <plan dir>/svc.log 2>&1 &，下面的 grep/sed 也指向同一路径）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| --capture 'baseUrl=listening on (https?://\\S+)' --out <plan dir>/drive.json \\ | ||
| --script ' | ||
| <start the service; --port 0 wherever it allows one> > svc.log 2>&1 & | ||
| until grep -q "listening on" svc.log; do sleep 0.2; done |
There was a problem hiding this comment.
[Suggestion] This readiness poll silently fails for a service whose stdout is block-buffered once redirected to a regular file — the standard C stdio and CPython print() behaviour: the listening on ... line sits in the userspace buffer, grep -q never matches, and the drive consumes the entire --timeout 300 budget beside a service that bound and was ready in a second — the harness-produced false negative this command's own header treats as worse than a missed finding, and the note's advice (raise --timeout) cannot fix this class at all. Witness (probe, real runDrive, Python service without flush, 10 s budget): at t=4 s the port was LISTEN while svc.log was 0 bytes; the drive reported timed-out, observed: false, output: ""; the identical service with flush=True completed in 0 s. Node services (qwen serve) write regular-file stdout synchronously, which is why the taught example works while the generic recipe ("an MCP server" — commonly Python) does not. Add a clause: if the service buffers stdout when redirected, force unbuffered/line-buffered output (python -u / PYTHONUNBUFFERED=1, stdbuf -oL, an explicit flush) — or keep readiness on an external --ready probe, which already waits portably.
中文说明
[Suggestion] 这个就绪轮询对"重定向到普通文件后 stdout 变为块缓冲"的服务会静默失效——这是 C stdio 与 CPython print() 的标准行为：listening on ... 这行停在用户态缓冲区里，grep -q 永远匹配不到，于是 drive 在一个早已绑定并就绪的服务旁边耗尽整个 --timeout 300 预算——这正是本命令文件头视为"比漏掉 finding 更糟"的那类 harness 制造的假阴性，而 note 给出的建议（调大 --timeout）对这一类完全无效。Witness（探针，真实 runDrive，未 flush 的 Python 服务，10 s 预算）：t=4 s 时端口已 LISTEN 而 svc.log 为 0 字节；drive 报告 timed-out、observed: false、output: ""；同一个服务加上 flush=True 后 0 s 完成。Node 服务（qwen serve）写普通文件是同步的，所以教学示例能跑通，而泛化 recipe（"一个 MCP server"——常见为 Python）不能。建议补充一句：若服务重定向后会缓冲 stdout，请强制非缓冲/行缓冲输出（python -u / PYTHONUNBUFFERED=1、stdbuf -oL 或显式 flush）——或者将就绪判断交回外部 --ready 探针，它本来就以可移植方式等待。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| ' | ||
| \`\`\` | ||
|
|
||
| Bind ephemeral wherever the service allows it: an OS-assigned port cannot collide, so the fallback never fires and the only address in the log is the right one. Then \`captured.baseUrl\` is the same line the script read, and quoting it in the witness is quoting what the run addressed. **\`null\` means nothing was captured** — the pattern never matched, or its declared group did not participate — which is the report saying the value was never measured, not permission to fall back to the one on your command line. Keep patterns linear — no nested quantifiers like \`(a+)+\`: extraction runs once the drive has ended, where no \`--timeout\` reaches, and one backtracking pattern hangs the whole run with no report written. |
There was a problem hiding this comment.
[Suggestion] This certification has a second hole besides the null case above: under the taught recipe a NON-null captured.baseUrl can only come from the probe response body or an agent-added echo, never from the service — the service's output goes to svc.log, while the drive log (the only channel extractCaptures reads) receives just the curl response. If the probed endpoint's body contains text matching listening on https?://\S+ (a status/log endpoint, a page quoting the startup banner, an echo service), the report presents observed: true beside a captured address sourced entirely from the response body — a false positive nothing in the report contradicts, and this sentence tells the agent that quoting it "is quoting what the run addressed". That is the decoy the sentinel's last-match design explicitly defends against on its own channel, reintroduced invisibly on the capture channel. Witness (probe, real runDrive, fake service on 127.0.0.1:18931): with a matching decoy line in the endpoint body, a faithful recipe run returned captured.baseUrl = "https://decoy.example.invalid:9999" while the script actually addressed http://127.0.0.1:18931; with no decoy, null on a fully correct run; teeing the service output into the drive log captured the real bind line, first-match picking it ahead of the body decoy. State that a non-null capture is only certified when the service's own output, not just the probe's, flows into the drive log.
中文说明
[Suggestion] 除了上面的 null 情形，这句认证还有第二个漏洞：按所教 recipe 运行时，非空的 captured.baseUrl 只可能来自探测响应的响应体、或 agent 自己加的 echo，而不可能来自服务——服务的输出进了 svc.log，而 drive 日志（extractCaptures 唯一读取的通道）里只有 curl 的响应。如果被探测端点的响应体里恰好含有匹配 listening on https?://\S+ 的文本（状态/日志端点、引用了启动横幅的页面、echo 服务），报告就会在 observed: true 旁呈现一个完全来自响应体的被捕获地址——一个报告自身无法反驳的假阳性，而这句话还在告诉 agent：引用它"就是在引用本次运行真正寻址的那个地址"。这正是 sentinel 的 last-match 设计在自己通道上明确防御的那种诱饵，如今在 capture 通道上被无形地重新引入。Witness（探针，真实 runDrive，假服务位于 127.0.0.1:18931）：端点响应体含诱饵行时，忠实按 recipe 运行返回 captured.baseUrl = "https://decoy.example.invalid:9999"，而脚本实际寻址的是 http://127.0.0.1:18931；无诱饵时，一次完全正确的运行返回 null；把服务输出 tee 进 drive 日志后，捕获到的就是真实绑定地址，first-match 优先取到它而非响应体诱饵。建议写明：只有当服务自身的输出（而不只是探测响应）流入 drive 日志时，非空的 capture 才有资格被认证。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Round 5 Critical, and it was mine: the recipe added last round sends the service to a file of its own, and `drive` runs the script as `bash <script> > <the drive log> 2>&1` while `extractCaptures` reads only that log. So the service's `listening on` line never reached the capture. Reproduced against the real `runDrive` before touching anything: a faithful run of the recipe returns `completed`, `observed: true`, and `captured.baseUrl: null`, under a note asserting the value "was never measured" — while the service had printed it all along. A recipe that cannot work is worse than no recipe. The service's output has to reach two places, and the shape that does it is a temp file the script greps plus a `cat` of that file before the first request. Three details in it are load-bearing, and each is a finding from this round: - `mktemp`, not a file beside the code. `--cwd` is the reviewed worktree, and an untracked `svc.log` left there is inlined into the next capture of that tree as though the PR added it. - `cat` BEFORE the request. Captures take the first match, so the service's own line wins over any response body containing one — a status endpoint quoting its own banner cannot forge the address. Verified with a service whose body advertises a different port. - No `trap … EXIT` of the caller's own. The wrapper writes its completion sentinel from an EXIT trap and a second one replaces it; measured, a script with its own trap comes back `timed-out` with a null exit code having run perfectly. I was about to use one for cleanup. Block-buffered stdout gets a line too — it is the one failure this shape cannot fix, and the note's advice to raise `--timeout` cannot touch it. The recipe is now pinned by a test that runs the brief's OWN text: the script body and the capture pattern are extracted from the brief rather than retyped, filled with a real service, and executed under the same redirect contract `runDrive` imposes. No tmux is involved, because what broke was the shell. Reverting the recipe to either broken form — the one shipped last round, or merely dropping the `cat` — reds it, and restoring returns 54 passed. 341 passed across drive, agent-prompt and run-skill-parity; tsc 0 errors; eslint clean, including two problems of my own the linter caught in the new test.
|
Round 5's [Critical] is correct, and it was mine — the recipe I added last round to fix @yiliang114's P1 could not work. Fixed in I reproduced it against the real The service printed its address all along; the drive log held only the The shape that works, and why each part is thereThe service's output has to reach two places — the script, to derive the address, and the drive log, because LOG=$(mktemp)
<start the service; --port 0 wherever it allows one> > "$LOG" 2>&1 &
until grep -q "listening on" "$LOG"; do sleep 0.2; done
BASE=$(sed -n "s|.*listening on \(https\{0,1\}://[^ ]*\).*|\1|p" "$LOG" | head -1)
cat "$LOG"
curl -s "$BASE/<the endpoint the claim is about>"
rm -f "$LOG"Each of the other three findings in this round is one of those details, so they are fixed together rather than patched separately:
One I found while fixing, not in any findingI was about to use
The recipe is pinned nowThe reason this Critical existed is that nothing executed the taught text. There is now a test that runs the brief's own recipe — script body and capture pattern extracted from the brief rather than retyped — filled with a real service and executed under the same redirect contract
341 passed across drive / agent-prompt / run-skill-parity, tsc 0 errors, eslint clean — including two problems of my own the linter caught inside the new test. Deferred, recorded rather than dropped: #9710Per the five-round rule, the two standing Suggestions go to a follow-up rather than widening this PR further:
中文说明第 5 轮的 [Critical] 是对的，而且是我造成的——我上一轮为修 @yiliang114 的 P1 而加的 recipe 根本跑不通。已在 动手前我先用真实 正确的形状：服务输出必须到达两个地方——脚本（用于取地址）和 drive log（因为 本轮其余三条 finding 恰好就是其中三个细节，所以一并修掉： 修的过程中自己发现的一条，不在任何 finding 里：我本来要用 recipe 现在被钉住了：这个 Critical 之所以存在，是因为没有任何东西执行过那段被教的文本。现在有一个测试运行 brief 自己的 recipe——脚本体和 capture 正则都从 brief 里抽取而非重敲——填入真实服务，并在 drive / agent-prompt / run-skill-parity 共 341 passed，tsc 0 error，eslint 干净——其中包括 linter 在新测试里抓到的、我自己的两个问题。 延后但已记录：#9710。 按五轮规则，两条仍然成立的 Suggestion 转入后续 issue 而不是继续扩大本 PR：R1-4（五处手抄的 |
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) was skipped in CI; the BSD userland would exercise the new recipe tests and its suite did not run locally.
Test Plan (not a blocker): 38 passed — this review observed 22859 passed; 285 passed — this review observed 22859 passed; 2 passed — this review observed 22859 passed.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/drive.test.ts:803 — [review] duplicate invariant pins whose mutation-justification comments are false of this same commitpackages/cli/src/commands/review/drive.ts:124 — [review] captured docblock 'Absent entirely when nothing was captured' contradicts the code (absent iff nothing was asked)packages/cli/src/commands/review/drive.test.ts:692 (+5 locations) — [probe] unpinned documented contracts — surviving mutants at 5 sites (spec-count accept side, name charset, two-group selection, value-cap boundary/direction, trimScopeNote…packages/cli/src/commands/review/lib/agent-briefs.ts:718 — [probe] recipe readiness loop has no deadline/failure branch — a dead service burns the full timeout, its only diagnostic orphaned in an anonymous temp filepackages/cli/src/commands/review/drive.ts:314 — [probe] flag-less regex compilation silently changes ^/$ anchoring and \u{}/\p{} escape semantics, undisclosed at every authoring surfacepackages/cli/src/commands/review/drive.ts:669 — [probe] the miss note's exhaustive cause enumeration is emitted on timed-out/overflowed drives, where a third cause existspackages/cli/src/commands/review/lib/agent-briefs.ts:721 — [probe] the recipe's trailing rm -f overwrites the request's exit status — a transport failure still yields completed/exitCode 0packages/cli/src/commands/review/lib/agent-briefs.ts:719 — [probe] a scheme-less 'listening on' banner passes readiness, sed derives an empty $BASE, curl fails silently, and rm -f supplies exit 0packages/cli/src/commands/review/drive.test.ts:949 — [probe] decoy port 59999 lies inside the OS ephemeral range — a ~1/28,000 false red presenting as the pinned regressionpackages/cli/src/commands/review/lib/agent-briefs.ts:718 — [probe] fractional sleep 0.2 is not POSIX — on integer-only hosts the readiness loop tight-spins error lines into the drive logpackages/cli/src/commands/review/drive.ts:361 — [probe] the value-cap cut can split a surrogate pair — a lone unpaired surrogate is emitted into both report sinkspackages/cli/src/commands/review/lib/agent-briefs.ts:720 — [probe] the one-shot cat snapshot semantics are undisclosed — values printed after readiness never reach the capture, yet the miss note asserts they were never printedpackages/cli/src/commands/review/lib/agent-briefs.ts:718 — [probe] readiness passes on a partial banner line — sed/cat freeze a truncated URL that the capture then corroborates as a measurementpackages/cli/src/commands/review/drive.ts:655 — [probe] the miss filter's v === null has no drive-level pin for an empty-string capture — the falsy mutant ships green with a self-contradicting report
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI; the BSD userland would exercise the new recipe tests and its suite did not run locally。
Test Plan（非阻断）：38 passed — this review observed 22859 passed; 285 passed — this review observed 22859 passed; 2 passed — this review observed 22859 passed。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 14 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const captured = | ||
| captureSpecs.length > 0 ? extractCaptures(output, captureSpecs) : undefined; |
There was a problem hiding this comment.
[Critical] A completed drive can report null for a value the run actually produced, and the miss note then asserts false causes over the one outcome the verify brief licenses for behavioural claims.
Extraction reads the poll loop's output snapshot, but each iteration reads the log before it checks the sentinel (lines 619–623) and breaks on completed with no re-read. The wrapper's EXIT trap writes the sentinel strictly after the script's last log write, so a write landing between those two back-to-back reads never reaches the snapshot. The loop ordering predates this PR, but the extraction and the causal notes added here turn the stale tail from a display artefact into a machine-readable measurement with false assertions attached.
Concrete trigger: a script whose last statement prints the captured fact (e.g. echo "finalmetric=7" with --capture metric=finalmetric=(\d+)) and whose final write lands in the window — one full readFileSync of a near-cap log, calibrated at ~1.5% of such drives. The report then says outcome: completed, captured.metric: null, and the note asserts "the pattern never matched, or its group did not participate" and "anything addressed by it was addressed by assumption" — both false, while the log on disk contains the value.
Witness (race probe — fake-tmux runDrive, 8.32 MiB log, deterministic pause sweep before the sentinel write):
PR arm (unmodified): 1/60 hits — outcome completed, captured {"metric": null},
log read back from disk AFTER the report contains finalmetric=7,
note: "--capture produced no value for \"metric\" ... addressed by assumption"
Fixed arm (re-read): 0/60 hits — same sweep, same harness (calibration 3.67 vs 3.83 ms/read)
Fix: re-read the log after the sentinel is observed, before extraction — every log write happens-before the sentinel write, so the post-sentinel read is complete:
exitCode = existsSync(sentinelPath)
? sentinelExitCode(readFileSync(sentinelPath, 'utf8'))
: null;
if (exitCode !== null) {
output = readFileSync(logPath, 'utf8');
outcome = 'completed';
break;
}(keeping the existing read order for the non-completed paths).
中文说明
[Critical] 一次 completed 的 drive 可能对运行实际产生过的值报告 null，而且 miss note 还会在这个唯一被 verify brief 认可作行为断言的 outcome 上给出错误的因果陈述。
提取读取的是轮询循环的 output 快照，但每一轮迭代都是先读日志、后查 sentinel（619–623 行），并在 completed 时直接 break、没有重读日志。包装脚本的 EXIT trap 严格在脚本最后一条日志写入之后才写 sentinel，因此落在两次背靠背读取之间的最后一次写入永远进不了快照。循环的先后顺序是本 PR 之前就有的，但本次新增的提取与因果 note 把这个"落后一次写入的尾巴"从显示层的小瑕疵变成了带错误断言的机器可读测量。
具体触发：脚本最后一句打印要被 capture 的事实（如 echo "finalmetric=7" 配 --capture metric=finalmetric=(\d+)），且最后这次写入恰好落在上述窗口内——窗口即一次对接近上限日志的完整 readFileSync，标定约为这类 drive 的 1.5%。此时报告会给出 outcome: completed、captured.metric: null，note 断言"pattern 从未匹配，或其捕获组未参与"、"基于它的一切都是按假设处理的"——两者都是假的，而磁盘上的日志里明明有这个值。
Witness（竞态探针——fake-tmux runDrive、8.32 MiB 日志、在写 sentinel 前做确定性暂停扫描）：PR 原样臂 1/60 命中——completed、captured {"metric": null}、报告写出后从磁盘回读日志仍含 finalmetric=7、note 为"--capture produced no value for "metric" ... addressed by assumption"；修复臂（观察到 sentinel 后重读日志）0/60 命中——同样的扫描与 harness（标定 3.67 对 3.83 ms/读）。
修复：在观察到 sentinel 之后、提取之前重读日志——所有日志写入都先于 sentinel 写入，因此 sentinel 之后的读是完整的（如上代码块；非 completed 路径保留原有读取顺序）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Confirmed and fixed in b54d07b. I reproduced it before changing anything, because a race asserted from a reading is the thing this PR keeps getting caught on.
Before — 7.9 MiB log (just under the cap, so the overflow branch does not pre-empt it), a real writer process swept across the read window, appending the final line and then writing the sentinel:
trials=70 completed=70 STALE-TAIL HITS=1 ← delay=248.72ms
outcome: completed, captured {"metric": null}
log re-read from disk after the report: contains finalmetric=7
note: --capture produced no value for "metric" ... addressed by assumption
After — identical sweep, identical harness:
trials=70 completed=70 STALE-TAIL HITS=0
Your 1-in-60 and my 1-in-70 are the same number.
Your reading of why it is this PR's defect is the part I want to agree with explicitly: the loop ordering is older than the diff, and while the only consumer was output it cost a truncated tail — a display artefact. Extracting captured from that same snapshot is what turns it into a machine-readable measurement with a false cause attached, on the one outcome the brief licenses for behavioural claims. So the escalation is the defect, even though the loop is not.
The fix is yours, with two constraints kept:
- Scoped to the
completedbranch. The happens-before argument is what makes the post-sentinel read safe, and only this exit has it — the others stopped the run rather than watching it finish. - The
existsSyncguard stays. A script that wrote nothing must still report an empty capture rather than throw; dropping the guard while fixing a correctness bug is how the next round starts.
Why no unit test, and what it would take
Reproducing this deterministically means mutating the log between two reads inside one iteration. There is no seam for that — runDrive reads through node:fs directly, and drive.test.ts states as a property that it never mocks node:fs ("Real bindings by construction"). The widened-race version hits ~1 in 70, and a test that fails one run in seventy is its own defect; I would rather leave the measurement above as the evidence than ship that.
The minimal way to pin it would be a readFile seam on DriveArgs, alongside the exec and logPath seams already there — then the race is a two-line fake. That is production surface added in round 6 for a test, so I have not done it unilaterally. Say the word and it is a small follow-up.
Recorded in #9710 alongside the other two deferrals if you would rather it went there.
344 passed across drive / agent-prompt / run-skill-parity, tsc 0 errors, eslint clean. One note on the numbers: this branch now carries a merge of main, which briefly showed 48 tsc errors — all from a stale packages/core/dist against newly merged core exports, none mentioning drive. Rebuilt, and the count above is post-rebuild.
中文说明
已确认并在 b54d07b 修复。动手前先复现——"从阅读推断出的竞态"正是这个 PR 反复栽跟头的地方。
修复前：7.9 MiB 日志（刚好在上限之下，避免 overflow 分支抢先），用真实写入进程在读取窗口上做扫描，先追加最后一行、再写 sentinel：70 次里 1 次命中（delay=248.72ms）——completed、captured {"metric": null}、报告写出后从磁盘回读日志仍含 finalmetric=7、note 声称"pattern 从未匹配…基于它的一切都是按假设处理的"。
修复后：同一份扫描、同一套 harness，70 次 0 命中。
你的 1/60 和我的 1/70 是同一个数。
关于为什么这算本 PR 的缺陷，我想明确表示同意：循环的先后顺序确实早于本 diff，但在只有 output 消费它时，代价只是被截断的尾巴，属于显示层瑕疵。把 captured 从同一个快照里提取出来，才把它变成带错误因果的机器可读测量，而且恰好落在 brief 唯一认可用于行为断言的那个 outcome 上。所以升级本身是缺陷，尽管循环不是。
修法采用你给的，并保留两条约束：只作用于 completed 分支（happens-before 才是这次读取安全的依据，只有这个出口具备它，其余出口是"我们把它停掉了"）；保留 existsSync 守卫（脚本什么都没写时应报空捕获而不是抛异常——在修正确性 bug 时顺手去掉守卫，正是下一轮的开端）。
为什么没加单测，以及要加需要什么：确定性复现需要在同一轮迭代的两次读取之间改动日志。目前没有这个缝——runDrive 直接走 node:fs，而 drive.test.ts 明确把"从不 mock node:fs"写成了自身属性。加宽版命中率约 1/70，一个七十次失败一次的测试本身就是缺陷；我宁可用上面的测量作为证据。最小的钉法是给 DriveArgs 加一个 readFile 缝（与已有的 exec、logPath 同类），那样这个竞态就是两行 fake。但那是在第 6 轮为测试增加生产面，我没有擅自做。你说一声就可以作为小后续。也可以按你意愿并入 #9710 的延后清单。
drive / agent-prompt / run-skill-parity 共 344 passed，tsc 0 error，eslint 干净。关于数字的一个说明：本分支现在带有一次 main 的合并，其间 tsc 一度报 48 个 error——全部来自 packages/core/dist 相对新合入的 core 导出陈旧，没有一条提到 drive；已重建，上面的数字是重建之后的。
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind Round summary — PR #9655One commit this round: Feedback points and dispositionsFixed this round
Why it was not pushed: Note: the base has since been auto-updated; the verdict below predates that update, and the next round's re-measurement may charge the round. bite check: changed tests pass on the pre-round tree (claimed defect does not reproduce) 中文说明🤖 AutoFix 更新了一个过期的 base —— 修复未通过验证，但本 PR 落后于 验证门的拒绝原因与日志证据见上方英文部分（gate-rejection 不翻译）。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/32565993656 🧠 Handled by Qwen Code · model/模型 |
Round 6 Critical. The poll loop reads the log and then the sentinel, and
breaks on `completed` without reading again — while the wrapper writes
the sentinel from an EXIT trap, strictly after the script's last write
to the log. A final write landing between those two back-to-back reads
is on disk and not in the snapshot, a window one `readFileSync` of a
near-cap log wide.
The ordering predates this PR, where it cost a truncated tail in
`output` and read as a display artefact. Extracting `captured` from the
same snapshot turned it into a machine-readable measurement with a false
cause attached: `outcome: completed`, `captured.<name>: null`, and a
note asserting the pattern never matched — for a value the log on disk
contains. That is the shape this command exists to prevent, so the
escalation is the defect even though the loop is older than the diff.
Reproduced before changing anything, on a 7.9 MiB log with a real
writer process swept across the read window:
before trials=70 completed=70 stale-tail hits=1 (delay 248.72 ms)
outcome completed, captured null, finalmetric=7 on disk
after trials=70 completed=70 stale-tail hits=0 same sweep
Every log write happens-before the sentinel write, so a read taken after
observing it is complete. Scoped to that branch alone: the other exits
stopped the run rather than watching it finish and have no such
guarantee to lean on, and the existing `existsSync` guard is kept so a
script that wrote nothing still reports an empty capture rather than
throwing.
No unit test: reproducing this deterministically needs the log mutated
between two reads inside one iteration, which wants a `readFile` seam
this command does not have, and drive.test.ts states that it never mocks
`node:fs`. A widened-race test hits ~1 in 70 and a flaky test is its own
defect. The measurement above stands as the evidence.
344 passed across drive, agent-prompt and run-skill-parity; tsc 0
errors; eslint clean.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI; the BSD userland would exercise the new recipe tests and its suite did not run locally.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/drive.test.ts:803 — [review] duplicate invariant pins whose mutation-justification comments are false of this same commitpackages/cli/src/commands/review/drive.test.ts:913 — [probe] briefText() reconstructs a superset of BRIEFS.verify.brief instead of using the brief accessorpackages/cli/src/commands/review/drive.ts:124 — [review] captured docblock 'Absent entirely when nothing was captured' contradicts the code (absent iff nothing was asked)packages/cli/src/commands/review/drive.test.ts:764 — [probe] CAPTURE_VALUE_MAX boundary unpinned — comparator and constant mutants survive the suitepackages/cli/src/commands/review/drive.ts:683 — [probe] the miss note's whole-log cause clause is emitted unscoped on timed-out/overflowed drives
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI; the BSD userland would exercise the new recipe tests and its suite did not run locally。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
@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: 80 passed · 0 failed · 80 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：80 通过 · 0 失败 · 80 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed (agent verdict) — 80/80 scripted assertions passed · verified head 中文 — 判定：✅ 通过（agent 判定）
Verification reportVerification report — PR #9655
|
| # | Finding (round 1) | Severity | Status at b54d07bf |
|---|---|---|---|
| F1 | Requested --capture silently vanishes on not-ready/unavailable (no captured key, note silent) |
Suggestion | stands — re-measured: both early returns still omit the key and the note (harness/ab-cells.mjs F1a/F1b, INFO lines). The docblock's absence rule ("absent entirely when nothing was captured") remains self-consistent; the author's rationale stands. Agree: non-blocking. |
| F2 | New verify-brief paragraph pinned by nothing | Suggestion | partially fixed — the fenced recipe is now pinned: deleting the whole section reds the recipe test (M12), deleting cat "$LOG" reds it (M11a), moving cat after the request reds it (M11c). The PROSE around the fence (the failure-mode explanation, the three load-bearing details, the null/ephemeral/buffering guidance) is still unpinned: deleting only the prose leaves all 344 tests green (M13). |
| C1 (correction) | Body's "38 passed (29 before, 9 new)" stale | — | still stale — measured 54 passed in drive.test.ts at this head (54 + 290 = 344 across the three suites, matching the final commit's claim). |
| C2 (correction) | "Captures on every outcome" overstates | — | superseded by code comments — the current docblock no longer claims every outcome; the in-code comment scopes it to outcomes where the script ran. F1 is the residual. |
Scope and central claim
Central claim (carried, re-measured): qwen review drive --capture name=<regex> reads named facts out of the drive's own UNTRIMMED output into captured — group 1 when the pattern declares one (declared-but-unfilled → null), whole match otherwise, null named in the note when nothing matched, field absent when nothing asked, malformed set rejected before anything starts, extraction on every outcome where the script ran.
Secondary claims (new since last round): (1) the head commit's sentinel re-read closes the stale-tail race; (2) the brief's taught bound-address recipe actually captures under the redirect contract runDrive imposes.
A/B: head vs base (core capture semantics, re-run at new head)
Control: drive.ts compiled standalone (esbuild transform, same flags) from a scratch HEAD^1 worktree and from head; the unit imports only node builtins plus two handler-only local modules, stubbed identically on both arms, so the arms differ by exactly the PR diff. Witness: evidence/01-ab-head-vs-base-cells.png.
| Cell | Oracle | Head | Base (control) |
|---|---|---|---|
C1 motivating: port 8931 is in use… + listening on …8932 at head, 400 KB noise |
captured.baseUrl, output, note |
completed; captured.baseUrl = http://127.0.0.1:8932, no 8931; output head-trimmed; reconciliation clause present |
completed; no captured key; bound address unknowable |
C2 no --capture |
report JSON | byte-equal to base modulo timing fields | identical |
| C3 malformed pattern | outcome + exec-call count | unavailable, Nothing was started., 0 exec calls |
runs the whole drive to completed (no validation) |
| C4 two unmatched patterns | captured + note |
both null, note names both |
no captured key |
| C5 timed-out, noisy | captured + note scoping |
captures on non-completed; no completed-scoped clause | no captured key |
| C6 overflowed (9 MiB) | captured + note |
overflowed, captured.baseUrl = null, miss named |
— |
Boundary matrix on the changed expressions (caps 32/33, 200/201, 8/9; split-on-first-; duplicate; empty ask; unfilled group; '' vs null; first match; value cap 4096/4097; astral; named groups): 22/22, in the same witness.
A/B: the sentinel re-read race (head commit b54d07bf)
harness/race-ab.mjs drives the REAL wrapper and poll loop: the fake new-session spawns a detached bash running the exact command line runDrive builds (bash <script> > <log> 2>&1); the script writes ~8.2 MiB of throttled noise, sweeps the delay before its final line across 0–475 ms, and the wrapper's EXIT trap writes the sentinel. Two arms: head, and head with ONLY the re-read statement removed (compiled diff verified to be exactly that line). Witness: evidence/02-race-ab-control-vs-head.png.
| arm | trials | completed | invalid | stale-tail hits |
|---|---|---|---|---|
| control (re-read removed) | 200 | 200 | 0 | 2 (captured=null, value on disk, outcome=completed) |
| head (re-read in place) | 200 | 200 | 0 | 0 |
Both arms ran the identical sweep under identical load (two parallel instances each). The two control hits are exactly the defect shape the commit describes — a full, plausible, wrong report. Head's zero is structural, not luck: every log write happens-before the sentinel write, and head re-reads after observing it. The unit suite stays green with the re-read removed (M10) — by the author's stated design (deterministic repro would need a readFile seam; drive.test.ts never mocks node:fs); the race harness is the pin.
The taught recipe, verified live
harness/recipe-check.mjs extracts the script body and capture pattern from the shipped dist brief, fills them with a real service whose response body carries a decoy listening on …:59999, and runs under the same redirect contract. 16/16, witness evidence/04-recipe-check-live.png: captured http://127.0.0.1:41845 (the service's own ephemeral port, not the decoy), the captured address answers and its body carries the decoy (same service), no stray log left in the cwd, and the brief's sed and --capture pattern agree on real http and https lines. The brief's "no trap … EXIT of your own" warning is measured, not read: a driven script with its own EXIT trap replaces the wrapper's sentinel trap (no sentinel written; control shape writes it) — harness/trap-and-redos.mjs, witness evidence/05-trap-hazard-and-redos-ladder.png.
Mutation matrix
Witness: evidence/03-mutation-matrix.png (per-mutant vitest logs in logs/mut-*.log; every mutant restored byte-identical, final git status clean). Suite at head: 54 tests in drive.test.ts, 344 across the three.
| Mutant | Result | Red tests |
|---|---|---|
| M1 extract from trimmed text | 2 failed | reads the UNTRIMMED log…; says captures survive the trim… |
M2 unmatched → '' |
5 failed | the null-semantics five |
M3 validation after tmux -V |
1 failed | refuses a malformed pattern before starting anything |
M4 m[1] ?? m[0] |
2 failed | the unfilled-group pair |
| M5 last match instead of first | 3 failed | both FIRST-match tests + the recipe test (decoy would win) |
| M6 drop completed-only scope | 2 failed | both scoping tests |
M7 captured always present |
1 failed | omits the field entirely when nothing was asked for |
| M8 positive control (trim keeps head) | 2 failed | proves the suite goes red in this file |
| M9 yargs option renamed | 1 failed | the CLI-seam test |
| M10 re-read removed | survived (54 green) | by design — pinned by the race A/B above, not the suite |
M11a recipe without cat "$LOG" |
1 failed | the recipe test |
M11b recipe LOG=$PWD/svc.log |
survived (54 green) | coverage gap — see F3 |
M11c cat after the request |
1 failed | the recipe test (decoy wins) |
| M12 delete whole brief section | 1 failed (344-run) | the recipe test |
| M13 delete prose only, keep fence | survived (344 green) | F2 residual |
13/15 killed; both survivors adjudicated below; M8/M11a/M12 are the positive controls proving the harness can red each file.
Findings (non-blocking)
F3 — Suggestion (new): the recipe's mktemp detail is unpinned
The brief's mktemp (not a file beside the code) protects the reviewed worktree on the recipe's FAILURE path — a service that never prints listening on leaves the script killed at drive timeout with its log in --cwd. The recipe test only runs the success path, where the recipe's own rm -f "$LOG" masks the choice (M11b survives). The brief text is correct as shipped; a fixture for the failure path (service that never comes up, then assert no stray file in the cwd) would pin it. Completeness reporting, not a merge condition.
F1 / F2 residual — carried from round 1, unchanged in severity
F1 stands as measured (status table); F2's unpinned half is the prose (M13). Both remain Suggestions: F1's absence rule is self-consistent and the author declined with rationale; the prose is guidance whose mechanical core (the recipe) is now pinned.
Note (not a finding): ReDoS residual unchanged, disclosed, bounded
Ladder re-run at this head through the real extractCaptures: 16.9 / 67.5 / 274.4 / 1073.3 / 4292.1 ms at N = 18/20/22/24/26 (~×3.7 per +2 chars), matching the docblock's "~×3.5" and the round-1 curve. Same trust-boundary assessment: the only pattern writers are the verifier agent and the operator, who already supply arbitrary shell via --script/--ready; brief, describe, and docblock all warn "keep patterns linear".
Targeted gates
drive.test.ts54/54;agent-prompt.test.ts+run-skill-parity.test.ts290/290 (344 total, matching the final commit's claim);tsc --noEmit -p packages/cli0 diagnostics; eslint on the three changed files clean, gate proven live (planted unused variable caught). Witness:evidence/07-targeted-gates.png.- Real CLI on the built dist: malformed pattern → exit 1 with the
unavailableJSON in milliseconds; valid pattern without tmux → the environmentalunavailable. Witness:evidence/06-cli-cells-real-dist.png.
Not covered
- Full tmux end-to-end. No tmux in the container, uid 1000 (no install path); the tmux-absent cell is environmental on both arms (A/A from round 1, re-confirmed by CLI-2). The seam-free surface (reject-before-start, environmental reporting, CLI wiring, wrapper/trap/poll-loop with a real bash standing in for tmux's process supervision) is covered.
- Per-commit attribution. Depth-2 checkout: only merge commit, base tip, PR head reachable; the aggregate
HEAD^1..HEADdiff is what was verified. - ReDoS ladder beyond N=26 (4.3 s here); higher rungs are extrapolation of the measured curve, each rung under
timeout 30. - Windows/macOS (Linux only; the suite's real-bash block is win32-skipped).
- The other two residuals of review: residual gaps in the live-service witness arm, and a graft for coexistence claims #9446 — declared out of scope by the PR, untouched by the diff.
- M10's unit coverage — absent by explicit author design; the race A/B harness is the pin, and it is a harness a future reader must re-run to re-verify.
Methodology
Environment: node:22 CI container, merge-ref checkout (HEAD merge 84005828, HEAD^1 base cef54b83, HEAD^2 PR head b54d07bf), pre-built at HEAD. Arms compiled with identical esbuild transforms from head, from a scratch base worktree (removed after capture), and from a scratch copy with only the re-read statement removed (diff verified to be exactly that line). The core cells drove runDrive through its own exec seam; the race harness replaced only tmux's process supervision with a real detached bash running the command line runDrive builds, so wrapper, log, sentinel, and poll loop are the real thing; the recipe harness ran the shipped dist brief's own text under the same redirect contract. Mutation runs edited the two source files in place, ran vitest, and restored byte-identically (cmp after every mutant; final git status clean). Raw logs in logs/, harnesses in harness/ (rerunnable), evidence PNGs via scripts/verify-capture.mjs.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/cli/src/commands/review/drive.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/drive.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/drive.test.ts: PPPPP
verdict: pass
summary: 1 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/drive.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/drive.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/drive.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/drive.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/drive.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. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Independent review at b54d07bf — both open Criticals verified fixed; approving
I checked the two unresolved Criticals against the code as it stands rather than taking the thread flags at face value, and re-derived the capture semantics from the documented invariants instead of from the test names.
The two Criticals
drive.ts stale-tail race (…333) — fixed. The completed branch now re-reads the log after the sentinel is observed, and the fix is correctly scoped to that one branch: the happens-before argument (EXIT trap writes the sentinel strictly after the script's last log write) is what makes a post-sentinel read complete, and only that exit has it. I confirmed the added read is inside the try, so it precedes the finally that removes the temp dir, and that the pre-existing uncapped read at the top of the loop already read the same file in the same iteration — so the second read adds no new size exposure, only a transient duplicate string.
agent-briefs.ts recipe never reaching the capture channel (…957) — fixed, and I verified the recipe end to end. The recipe now cats the service's log into the script's stdout before the first request, so the evidence producer and the evidence consumer finally meet in the channel --capture reads. Ran the taught shape against a real service binding an ephemeral port, as drive runs it (bash <script> > <drive log> 2>&1):
--- drive log (what --capture reads) ---
listening on http://127.0.0.1:52607
pong
SCRIPT-USED-BASE=http://127.0.0.1:52607
captured.baseUrl : "http://127.0.0.1:52607"
address script used : "http://127.0.0.1:52607"
AGREE : YES — the capture corroborates the script
That is the acceptance criterion from #9446 residual (1): the address in the report is the one it bound. Putting cat before the request also does the decoy work the brief claims for it — under first-match the service's own banner wins over any response body quoting an address.
Capture semantics — checked against real inputs, not just described
Every invariant the docblocks assert holds:
| behaviour | result |
|---|---|
motivating case: log holds both 8931 is in use and listening on …8932 |
{"baseUrl":"http://127.0.0.1:8932"} — 8931 absent |
| first match beats a later decoy line | …:40621, not the response body's evil.example:9999 |
| no group → whole match / group → group 1 | listening on http://…:8932 / http://…:8932 |
declared group left unfilled ((?:a(x))?b vs b) |
null, not a silent whole-match swap |
pid=(\d*) vs pid=abc / pid=(\d+) vs pid=abc |
"" / null — the distinction the report leans on |
| bounds: 200 vs 201 chars, 8 vs 9 patterns | accepted / rejected, rejected as a set |
flags cannot be injected (v=/LISTENING/i) |
treated as a literal pattern → null |
split on first = only (port=listening on \S+=(\d+)) |
{"port":"8932"} |
| value cap | cut at 4096 with ... [truncated, 5000 characters total] |
(a)|(b) against b also yields null — group 1 is the capture and it did not participate. That is the documented rule rather than a surprise, and the miss note names the key, which is what makes it readable.
The --capture option is not a dead switch: the handler passes argv straight into runDrive, and there is a test driving the flag through the real CLI seam to the printed report. Captures being absent on unavailable / not-ready is correct rather than an inconsistency with "taken on every outcome" — the readiness probe runs before new-session, so on those paths the script never ran and there is genuinely nothing to capture.
Earlier rounds' coverage gaps are closed at this head: the FIRST-match rule, the length cap's upper bound, and the completed-only scoping of the reconciliation clause each have a test now.
Non-blocking residuals
- The ReDoS is real, disclosed accurately, and not an escalation. I measured
(a+)+$— 9 characters, far under the 200-char cap — at 40.9 / 161.8 / 644.1 ms for 22 / 24 / 26 characters of near-match, so the "~×3.5 per +2 characters, a ~40-char near-miss is hours" comment is honest rather than understated, and extraction sits after the poll loop where--timeoutcannot reach. It is not a new trust boundary: the same caller already supplies--scriptand--readyas arbitrary shell. What it does add is a way for an agent authoring its own pattern to hang a drive with no report, which is why the warning belongs in the flag help and the brief — and it is in both. Fine as a documented tradeoff; a bounded matcher would be a separate change. - R1-4 —
runDrivenow carries several near-identical hand-copied "unavailable / nothing started" report literals. Cosmetic, but a field added toDriveReporthas to be remembered in each one. Worth a small follow-up so it is not silently dropped.
Scope of my verification
I did not run the repository's vitest suite — my worktree has no node_modules, and CI's Test (ubuntu-latest, Node 22.x) covers it green. My evidence is a verbatim transliteration of parseCaptureSpecs / declaresGroup / extractCaptures exercised against the inputs above, plus a real shell run of the taught recipe against a real ephemeral-port service. House style is clean: no any, no cross-package relative imports, tests collocated.
No blocking findings at this head.
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round 8 summary — PR #96551. The previous round's gate rejection is repaired (top priority, per the feedback)The verification gate rejected the stale-tail fix ( Why the fix shipped unwitnessed, and what unblocks it: reproducing the race deterministically needs the log mutated between the poll loop's two back-to-back reads, What this round commits (
Mutation probes (AGENTS.md requires each added guard to have its own witness; both probes ran and were restored):
Bite-check shape, measured rather than asserted: with 2. Re-verified and resolved (fixed by earlier commits, threads still open)
3. Deferred to the #9710 follow-up queue (recorded, not dropped)
4. Declined — refuted on the current head
5. Left unresolved
VerificationCommands actually run this round, with results:
中文说明第 8 轮总结 — PR #96551. 上一轮的门拒绝已修复（按反馈要求的第一优先级）验证门拒绝了 stale-tail 修复（ 为什么该修复发布时没有见证测试、以及现在靠什么解开：确定性地复现这个竞态，需要在轮询循环两次背靠背读取之间改动日志； 本轮提交（
变异探针（AGENTS.md 要求每个新增守卫有自己的见证；两个探针都已执行并还原）：
bite 检查形态，实测而非断言：把 2. 重新验证后解决（更早的提交已修复，线程仍未关闭）
3. 延后到 #9710 后续队列（已记录，不会丢）
4. 拒绝——在当前 head 上被证伪
5. 保持未解决
验证本轮实际执行的命令及结果：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
Released in v0.22.0. |
…wenLM#9708) * feat(review): add temporal-reachability and incident-replay lenses Two blind-spot fixes measured on PR QwenLM#9655's escaped P1 (a post-run --capture that could not steer the run it documents, plus brief text telling the witness to quote it as though it had): - Agent 1c: reachability gains a TIME axis. A value produced after every decision it should influence is a record, not a mechanism; when documentation or workflow guidance treats the record as a mechanism, that is the Critical, with 'produced at X, needed at Y, Y precedes X' as the whole trace. - Agent 0: a motivating incident narrated in the PR context is replayed step by step against the post-change workflow, regardless of closing-keyword formality. An unchanged outcome is a Critical even when the diff faithfully implements what its issue prescribed — an issue can prescribe a remedy that never reaches its own observed failure. An empty closing set no longer empties the replay duty. * fix(review): pin the new lenses and give the replay an enforceable contract Round-1 review feedback on this PR, all five findings addressed: - R1-1/2/3: the three added passages were unpinned — a future deletion shipped green. Weld-style pins added in agent-prompt.test.ts (the enumeration-trap precedent), covering the replay duty, its un-gating, the TIME-axis paragraph, the trace format, and the verifier clause. - R1-4: the empty-scope return now carries a fourth evidence item — the replay's outcome (the step that changes, or the reason none does), or an explicit statement that the description narrates no incident — so a skipped replay never reads identically to a performed one. - R1-5: the orchestrator contract buried the lens's product in the exact case it was written for — SKILL.md forbade falling back to the PR description and the verify brief downgraded fidelity findings lacking issue evidence to low confidence (terminal-only). Carve-outs added in critical rule 4, the Step 2 context paragraph, and the verify brief: a replay finding quotes the PR's own narrative as its evidence, judged as the PR's claim about what the change prevents, not as ground truth. * fix(review): route the no-step-changed replay outcome to a finding, never the receipt Round-2 review feedback, all four findings addressed: - R2-1 (Critical): round 1's fourth evidence item routed the replay's no-step-changed outcome INTO the scope-empty receipt while the bullet above mandates it as a Critical — two mutually exclusive return shapes, and a receipt contributes nothing to the verdict, so the mandated Critical could dissolve. The contract now routes explicitly: no step changed = a findings return; the receipt carries only the benign outcomes (the step the replay saw change, or an explicit statement that the description narrates no incident). - R2-2: four load-bearing clauses pinned — the replay's Critical severity, 1c's record-as-mechanism severity condition, the distinguishability sentence, and the verifier's no-downgrade clause. - R2-3: the orchestrator-facing copies of Agent 0's return contract (the whiff-check parenthetical and the roll-call example) updated to the new shape, so a skipped replay cannot pass as the old three-item receipt the prose told the orchestrator to accept without relaunch. - R2-4: SKILL.test.ts revert guards for both SKILL.md copies of the incident-replay carve-out, following the rule-4 guard's pattern. * fix(review): complete the receipt example and pin the last unpinned clauses Round-3 review feedback, all three findings addressed: - The roll-call example restores the 'not a bugfix' evidence item the round-2 rewrite dropped — it now models all four receipt items, so an orchestrator shaping its Step 6 line on it cannot certify scope-empty for a bugfix PR without that determination asserted. - The orchestrator-side copy of the R2-1 routing rule and the roll-call line are pinned in SKILL.test.ts's carve-out guard: reverting either restored the pre-R2-1 receipt standard while every brief-side pin stayed green. - The TIME-axis pins gain the definition clause ('a record, not a mechanism') and the two-moments method — without them the severity rule names a split nothing defines.
5 participants
What this PR does
Adds
--capture name=<regex>toqwen review drive. Each pattern is read back out of the run's own output into acapturedblock in the report — capture group 1 where the pattern has one, the whole match otherwise, andnullwhere nothing matched. The verify brief gains a paragraph telling a verifier to bind ephemeral where the service allows it, capture the address the service prints, and quote that in the witness rather than the one on its own command line.Nothing else changes. Existing drives behave exactly as before, and a drive that asks for no captures does not get an empty
capturedobject — the field is absent, because an empty result set is a claim and a drive that asked for nothing has none to make.Why it's needed
A port is a request, not a fact. Handed one that is already taken,
qwen serveprintsport 8931 is in use, trying 8932...and listens on the next one. A verifier that goes on addressing the port it asked for then reads a different, stale process for the rest of the run: the readiness probe passes against whatever is squatting there, the drive reaches its sentinel,outcomeiscompleted, and every number in the resulting witness is about the wrong daemon. Nothing in the report contradicts it, because nothing in the report knew —drivehad no port handling at all, only a comment mentioning that a daemon binds one.That failure is the specific kind this command was written to remove. Its own header comment says so: the measurements behind it are about harness-manufactured false readings — a
sleepthat lands before the daemon is up, a capture taken mid-write — and treats a silent wrong answer as worse than a missed finding. A run against the wrong process is the same class, and it is the worst-behaved member of it, because unlike an empty capture it produces a full, plausible, completely wrong set of numbers. It cost a full cycle during the daemon verification this came out of, and the run that produced the bogus readings looked exactly like a clean one.The fix is not to teach
driveabout ports. It is to let a run report what it chose rather than what it was told, so the address a witness quotes is one the run produced. That generalises past ports — a pid, a temp path, a negotiated protocol version — which is why the flag is a named pattern rather than a--portspecial case.Four decisions inside it are the ones worth reviewing:
trimCapturekeeps the tail; a service prints its address at the head. Capturing from the report'soutputwould lose exactly the value this exists for, and would lose it on the loudest runs — the ones most likely to need it.null, never'', and the note names it. That is the moment a witness is about to quote a value the run never produced, and the reader needs to know which value before deciding whether the rest still stands.nullis reserved for. Doing the check after a 300-second drive would cost the drive.completed. A drive that timed out still bound its port, and that address is often the fact that explains where the rest of it went.The brief half is deliberate rather than incidental: a capability taught only where the verifier does not read is inert. That was the first review round's finding on #9445, and repeating it here would make this flag a feature nothing uses.
Reviewer Test Plan
How to verify
The suites passing is the weaker half. The load-bearing half is that the new tests fail when the behaviour they describe is removed — three mutations against the implementation, with the PR's own tests kept:
trimCapture(output).textinstead of the untrimmed logreads the UNTRIMMED log, so a value printed at startup survives a noisy run''instead ofnullnames an unmatched pattern in the note instead of reporting a blank,prefers group 1, falls back to the whole match--capturevalidation after thetmux -Vproberefuses a malformed pattern before starting anythingThe untrimmed-log case is the one to read closely, because it is the only one whose failure would be invisible in production: it drives a 400 KB log whose
listening online is at the head, asserts the report'soutputno longer contains that line at all (the tail-trim worked), and assertscaptured.baseUrlstill holds the address. The third mutation's test proves nothing was started by asserting the exec log is empty —tmux -Vis the first thingrunDrivewould otherwise touch.The motivating scenario is a test of its own: a log holding both
port 8931 is in use, trying 8932...andlistening on http://127.0.0.1:8932must capture 8932, and the assertion explicitly requires the captured value not to contain 8931.Evidence (Before & After)
N/A — no user-visible or TUI change; the surface is a review subcommand's JSON report.
Tested on
Environment (optional)
Local
npm ci+npm run buildon Node v22.22.2, then the suites above.Risk & Scope
--scriptand--readyas arbitrary shell — and the bounds are deliberately tight (8 patterns, 200 characters each, no flags, nog), but it is the one thing this adds that can be slow rather than wrong.drivenormock-provideris inside the review's time budget, andgraftdoes not exist. Both are separate changes and neither is started here.capturedis a new optional field; every existing invocation produces a byte-identical report.Linked Issues
Refs #9446 — this closes the first of its three residuals.
中文说明
这个 PR 做了什么
给
qwen review drive增加--capture name=<regex>。每个 pattern 从本次运行自己的输出里取回一个事实，放进报告的captured块——pattern 带捕获组时取组 1，否则取整个匹配；没匹配上则为null。verify brief 相应增加一段，告诉 verifier：服务允许时就绑定临时端口，把服务打印出来的地址 capture 下来，witness 里引用那个地址，而不是自己命令行上的那个。其余一切不变。已有的 drive 行为逐字节相同；没有请求任何 capture 的 drive 不会得到一个空的
captured对象——该字段直接缺席，因为空结果集也是一种主张，而什么都没要的 drive 没有主张可作。为什么需要
端口是一个请求，不是一个事实。拿到已被占用的端口时，
qwen serve会打印port 8931 is in use, trying 8932...然后监听下一个。继续按自己请求的端口寻址的 verifier，此后整轮读到的是另一个陈旧进程：readiness 探测在那个蹲着的进程上通过、drive 抵达 sentinel、outcome是completed，而 witness 里的每一个数字都是关于错误 daemon 的。报告里没有任何东西反驳它，因为报告根本不知道——drive完全不处理端口，只有一句提到 daemon 会绑定端口的注释。这类失败正是这个命令被写出来要消灭的那一类。它自己的文件头就是这么说的：背后那些测量讲的都是由 harness 制造的假读数（
sleep落在 daemon 起来之前、写到一半就抓屏），并且把"静默的错误答案"视为比"漏掉一个 finding"更糟。跑在错误进程上属于同一类，而且是其中表现最恶劣的一个——因为它不像空抓屏，它会产出一整套完整、可信、彻头彻尾错误的数字。在本次 daemon 验证里它让我白跑一整轮，而产出那些假数字的运行看起来和干净运行一模一样。修法不是让
drive去懂端口，而是让一次运行能报告它自己选择了什么、而不只是被告知了什么，这样 witness 引用的地址就是运行产出的。这同时也推广到端口之外——pid、临时路径、协商出来的协议版本——所以这个 flag 是具名 pattern，而不是一个--port特例。其中四个决定值得评审时细看：
trimCapture保留尾部，而服务在头部打印地址。从报告的output里 capture 会恰好丢掉这个功能唯一存在的价值，而且是在最吵的运行上丢——那正是最需要它的场合。null，绝不是''，而且 note 里点名是哪一个。 那是"witness 即将引用一个本次运行从未产生的值"的时刻，读者必须知道是哪一个值，才能判断其余部分是否还站得住。null独占的含义。放到 300 秒的 drive 之后才发现，代价是整个 drive。completed。 超时的 drive 照样绑定了端口，而那个地址往往正是解释它后来为什么走不下去的事实。brief 那一半是刻意为之而非顺带：只写在 verifier 读不到的地方的能力等于不存在。那是 #9445 第一轮评审的结论，在这里重蹈会让这个 flag 变成一个没人用的功能。
Reviewer Test Plan
如何验证
套件全绿是较弱的那一半。承重的那一半是：移除新用例所描述的行为，它们会红——保留本 PR 自己的测试，对实现做三处变异：
trimCapture(output).text而非未裁剪日志 capturereads the UNTRIMMED log, so a value printed at startup survives a noisy run''而非nullnames an unmatched pattern in the note instead of reporting a blank、prefers group 1, falls back to the whole match--capture校验移到tmux -V探测之后refuses a malformed pattern before starting anything未裁剪日志那条最值得细读，因为只有它的失效在生产中是不可见的：它驱动一份 400 KB 的日志、其
listening on行位于头部，断言报告的output里已经完全不含该行（尾部裁剪确实生效了），同时断言captured.baseUrl仍然持有地址。第三条变异对应的用例靠"exec 日志为空"证明确实什么都没启动——tmux -V是runDrive否则会最先碰到的东西。引发本 PR 的场景本身也是一条用例：一份同时含有
port 8931 is in use, trying 8932...与listening on http://127.0.0.1:8932的日志必须 capture 到 8932，并且断言显式要求 capture 到的值不含 8931。Evidence (Before & After)
N/A —— 无用户可见或 TUI 变更；改动面是一个 review 子命令的 JSON 报告。
测试环境
本地在 Node v22.22.2 上
npm ci+npm run build，然后跑上述套件。风险与范围
--script与--ready这两段任意 shell——边界也刻意收得很紧（最多 8 条、每条 200 字符、无 flag、无g），但它确实是本次新增的唯一一个可能"慢"而非"错"的东西。drive与mock-provider都不在 review 的时间预算内，以及graft尚不存在。两者都是独立改动，本次都未着手。captured是新增可选字段；每一个既有调用产出的报告逐字节相同。关联 Issue
Refs #9446 —— 本 PR 关掉其三条残余中的第一条。