perf(review): retire dry chunks and pipeline verification in the reverse audit - #8498
Conversation
|
🔄 Qwen Triage is running — watch live progress. Stage results will post in this thread as they complete. 🔄 Qwen Triage 正在运行 —— 查看实时进度。各阶段结果完成后会更新在本线程。 |
|
Re-run over the two commits landed since the last pass (
Moving on to code review of the delta. 🔍 中文说明针对上次评审后落地的两个提交（
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review — the 23:17 ledger is closed at this headIndependent baseline first: given "retire dry chunks, pipeline verification", I would have done exactly what this PR does — derive per-chunk state from the artifacts the delivery checks already trust (the CLI's own prompt records + the harness transcripts, never orchestrator-writable), fail every classification error toward auditing, order convergence ahead of the budget gate so a clean exit never wears a truncation disclosure, and keep Every one of the 15 findings in the
My own pass beyond the ledger: the new exit-5 blast radius is contained — no workflow script or CLI module shells out to Nothing blocking found. The author's "1710 passed in src/commands/review" is their claim; the evidence below is the PR's own CI. Testing — the PR's own CI on the reviewed commitThe macOS/Windows unit legs and the no-sandbox integration leg are skipped, not failed — identical to the merged #8468's head, so this is the normal pattern for this branch, not a regression signal. The primary suite ran green.
Sandboxed verification would settle the one claim reading alone cannot: that the classifier hardening at this head keeps the A/B behavior the previous round substantiated at 中文说明代码审查 —— 23:17 的评审清单在本 head 上全部关闭先说独立基线：给定"退休干净 chunk、流水线化验证"这个目标，我会做的与这个 PR 完全一致——从交付检查本就信任的两份工件（CLI 自己的 prompt 记录 + harness transcripts，绝不读编排者可写的东西）推导每个 chunk 的状态，一切分类错误都失败向多审计，收敛检查排在预算闸之前使干净退出永不披上截断披露，
我在清单之外的核查：新 exit-5 的影响面收敛——没有任何工作流脚本或 CLI 模块直接调用 未发现阻塞项。作者"src/commands/review 1710 通过"是其自述；下面的证据来自 PR 自己的 CI。 测试 —— 已评审提交上的 CImacOS/Windows 单测与无沙箱集成为 skipped 而非失败——与已合并的 #8468 的 head 完全一致，属于该分支的常规模式，不是回归信号。主套件全绿（见上表：ubuntu 单测、web-shell E2E、Desktop Shell 均 success）。 沙箱验证将落定纯阅读无法落定的一项主张：本 head 上的分类器加固是否保持上一轮在 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — every finding from three probe-backed review rounds is closed at the head, each pinned by a test carrying the original probe's shape, and CI is green on the reviewed commit; the remaining fifth is what reading alone cannot settle — the wall-clock arithmetic, one SKILL seam still held partly by orchestrator compliance, and a verification round on this exact head still in flight. Stepping back over the whole arc: the previous pass approved the mechanism early; the review rounds since found two genuine Criticals and a cluster of real holes in the classifier, and this head closes them the way I would have closed each — and in a couple of places better than my baseline (the record fence deliberately keeps the coverage callers unfenced, with the obligation/history distinction spelled out; the three admission paths share one gate helper so they cannot drift). The fail-toward-auditing bias — the whole personality of this subsystem — survived every fix: each new guard's degradation direction is "audit more". The tests are the probes, not new prose about them, and my own walk beyond the ledger (exit-5 blast radius, the Named reservations, none blocking: the pipelined last-round ordering is now mechanical where it can be — the One state note for whoever merges: the standing Approving, pinned to the reviewed commit. 中文说明信心：4/5 —— 三轮带探针评审的每一条发现都在本 head 上关闭，每条都有携带原探针形状的测试钉死，已评审提交上的 CI 全绿；剩下的五分之一是纯阅读无法落定的部分——墙钟折算、一处仍部分依赖编排者合规的 SKILL 接缝，以及针对本 head 的验证轮仍在执行中。 退一步看整条弧线：上一轮较早地批准了机制本身；其后的评审轮找出两个真正的 Critical 和分类器上一批真实的洞，而本 head 按我会采用的方式逐条关闭——有几处比我的基线更好（记录围栏刻意让 coverage 调用方保持无围栏并写明"义务/历史"之别；三条准入路径共用一个闸 helper，不会漂移）。整个子系统的性格——失败偏向多审计——在每次修复后完整保留：每个新守卫的退化方向都是"多审计"。测试就是那些探针本身，不是关于它们的新散文；我在清单之外的核查（exit-5 影响面、 点名保留意见，均不阻塞：流水线化后的末轮顺序在能结构化的地方已经结构化—— 给合并者的状态说明：当前挂着的 批准，锚定在已评审的提交上。 — Qwen Code · qwen3.8-max Reviewed at |
Review:
|
chunk A (dry 1,2 → lastDry=2) |
chunk B (yielded r1, dry 2,3 → lastDry=3) |
|
|---|---|---|
| round 4 | cold check (due) | skipped |
| round 5 | skipped | cold check (due) |
So on any diff where one chunk yields in round 1 or 2 — the common case on a large PR, and the exact case the PR's own yield grid shows — the loop never takes the clean-convergence exit and always runs to the 5-round cap. Combined with the SKILL edit that re-scopes the two-dry-rounds rule to 3A only ("the 3A criterion — one auditor, so round-dry and territory-dry are the same thing"), the 3B path is left with no early exit at all except exit 4/the cap, and reports "hard cap" rather than "converged" on a run that had in fact converged.
The rounds are cheaper, so this is not a perf regression — but it does mean the advertised exit-5 path will rarely fire in practice. Anchoring the parity on a single per-round basis (e.g. round % 2 against a fixed epoch, or "due if it was skipped last round") would let staggered chunks re-align.
🟡 Suggestion 2 — pipelining lets a rejected finding suppress one round of rediscovery
The cumulative list now carries unverified entries, and the auditor brief tells each agent not to re-report what is on it. If round k files a false positive at x.ts:20, round k+1's auditor is told that spot is covered; the verifier then rejects it and it leaves the list at the next merge — but round k+1 is already gone. Under the old ordering a rejected entry never entered the list at all.
The window is one round in one location and the cap still bounds the loop, so I don't think this blocks the change — but "with detection semantics intact" in the PR body overstates it, and the SKILL bullet would be more honest if it named the trade (an entry a verifier later rejects will have suppressed one round of rediscovery in its neighbourhood).
🟡 Suggestion 3 — the budget gate also refuses a single-chunk repair rebuild
agent-prompt.ts:1690 gates on role === 'reverse-audit' without distinguishing "admit a new round" from "rebuild one chunk of a round already admitted". So a --chunk <id> --round k repair after a truncated read gets exit 4 and writes a marker reading stopped before round k — even though round k ran. Two effects: the truncated delivery can't be repaired (that chunk's auditor is never launched, and nothing writes an unreviewedDimensions entry for it), and the disclosure is factually wrong about which round stopped. Retirement already exempts the --chunk path as "a repair is not scheduling" (nice test, by the way) — the same reasoning applies to the budget gate.
Also: for a rebuild of round k, expectedRoundSeconds deliberately reaches past round k's own stamp to round k-1's, so the "estimate" includes all of round k's elapsed time — systematically inflated exactly on the path most likely to be refused.
🟡 Suggestion 4 — the header's truncation self-check no longer describes the output
agent-prompt.ts:1526 still tells the reader "the output ends with an end-of-round line — if either is missing, the output was truncated in transit", but with a retirement the retirement: note is now appended after it. The literal test still works ("if either is missing"), but the sentence is the thing that has to survive a compression, and it now describes a shape the builder no longer emits. Worth a clause: …ends with an end-of-round line (followed by the retirement note, when there is one).
Smaller notes
--all-chunkswith no--round:stampRound(planPath, undefined)skips the dedupe (round !== undefined && …) and appends a{round: null}stamp on every invocation, andexpectedRoundSeconds(plan, undefined)then reads the most recent one. Harmless as long as the SKILL always passes--round, but it is a fail-closed corner in a module that is otherwise uniformly fail-open.DRY_RECEIPT_RE = /No (new )?(issues|findings|gap)/iwon't match a substantive receipt phrased "Nothing further to report — re-walked…". That fails toward auditing, so it's safe; just noting the heuristic is narrower than the brief's contract implies.- Workflow: with
attempt_timeoutas low as 30s (the loop's floor beforefail), the 600s reserve floor means the audit loop is skipped outright. The comment says that's intended ("degrades to skipping the audit loop, correctly") — agreed, worth keeping the comment. - The
3600↔DEFAULT_RESERVE_SECONDS"keep the two in sync" pairing is a comment-only invariant across a YAML file and a TS file. Nothing will catch a drift. Low stakes (both are fallbacks), but a# see deadline.tson the TS side too would at least make it bidirectional.
Test coverage
Strong where it exists — driving agentPromptCommand.handler round by round with harness-shaped transcripts is much better than unit-testing scheduleReverseAuditRound directly, and the negative assertions (no records, no stamp, exit 4 outranks exit 5) are the right ones. Gaps:
- No test for a schedule with mixed parity across chunks (Suggestion 1) — the scenario the yield grid in the PR description actually shows.
- No test for a second run against the same plan path (Critical 1). The retirement path is safe there by inheritance from the transcript fence;
deadline.tsis not, and nothing covers it. - No test for one transcript matching several records (Critical 2).
.github/workflows/qwen-code-pr-review.yml's new shell arithmetic (floor/cap, per-attempt recompute) has no coverage; the existing extract-the-run-step harness pattern would cover it cheaply.
Security / performance
No new attack surface: no path comes from the model (both deadline.ts and retirement.ts derive from planPath / the CLI-exported env, consistent with the rest of the module), no new network or shell. readTranscripts is now called once more per --all-chunks round from round 3 — an O(transcripts) parse of a directory that is already read at Step 3D and Step 6, so negligible against a round that costs tens of minutes. The projected saving looks plausible to me on the grid shown; the pipelining half is the more reliable of the two, since it doesn't depend on chunks going cold.
中文说明
总评
方向正确,工程质量高:退休调度由 CLI 强制而非 prose 要求;调度依据是 orchestrator 无法伪造的两份证物(自身的 prompt 记录 × harness transcripts);所有分类路径都朝"多审计"失败;readTranscripts 的 plan-mtime 围栏被正确继承。budget-stop marker 由 compose-review 直接读取、而不依赖 orchestrator 转述,是 verdict cap 的正确形状。9 个调度用例逐轮驱动真实 handler,而非孤立单测 scheduler,这是它们真正有价值的原因。
本地复现:未跑全量 src/commands/review,只跑了本 diff 触及的两个文件 deadline.test.ts(15)+ agent-prompt.test.ts(166),181 passed / 0 failed。两个 Critical 均在干净 worktree 上以探针复现。
🔴 Critical 1:预算状态会带入 CI 同 job 的重试并污染它
budget-rounds.json / budget-stop.json 放在 promptRecordDir(planPath),仅由 plan 路径推导。与 retirement.ts:173 正确使用 plan mtime 围栏的 readTranscripts 不同,这两个文件没有任何围栏,且新一次 capture 不会清理记录目录(promptRecordDir 的三个非测试调用方都不删除)。本该清理的 Step 9 恰恰在 attempt 提前死亡时不会执行,而 workflow 的重试就在同一个 job、同一个 workspace、同一个 plan 路径上。
后果两条,均已复现:
(a) 陈旧 marker 让健康的 attempt 2 被误封顶——compose-review.ts:350 无条件读取,attempt 2 即使完整跑完所有轮次,也会披露 reverse audit — stopped before round 3 by the review time budget 并把 Approve 压到 COMMENT。
(b) stampRound 按 round 去重,导致 attempt 2 根本无法打戳,于是每个估算都是从 attempt 1 的墙钟时间量出来的:round-2 估算 2160s(真实约 1500s)→ 在还剩 54 分钟、本轮只要 25 分钟时被拒。
即:大 PR 上一次瞬时 API 错误,可能让重试完全没有反审,还附带一条它从未触发过的时间预算披露——恰好把这道闸的意图反转。本地对一个上次被中断(未走 Step 9)的目标重新 review 同理。
建议:给这两个文件加上 transcripts 已有的同一道围栏(把 planMtimeMs 写进去、不匹配即丢弃,readRoundStamps / readBudgetStop 各几行),或让 capture 步骤清理 promptRecordDir。前者更省,改动不出 deadline.ts。
🔴 Critical 2:一份 transcript 可以让所有 chunk 退休
retirement.ts:194 会把每一条 prompt 出现在该 transcript 里的记录都算上,且 wasDeliveredVerbatim 是"可加不可删"的判定——所以一个把 N 个 block 拼接起来的 launch prompt 同时匹配 N 条记录。verificationGaps 正是防这一手的,注释写得很清楚:"Injective: one transcript may satisfy ONE roster requirement…"
对真实 scheduler 复现:两个 agent(每轮一个,拿到整轮输出)让 6 个 chunk-round 全部退休,round 3 直接 converged: true → exit 5 / CONVERGED,而那段 stderr 明确宣称"这是干净收敛,不欠 unreviewedDimensions"。verificationGaps 的 floor-of-one 也照样通过(那一个 agent 确实读了 brief),下游无人能拦。
这是 orchestrator 被观测到真做过的偷懒(injectivity 规则就是因此存在),不是假想。按本模块自述的威胁模型,退休判定应当采用与 verificationGaps 相同的"一份 transcript 只能满足一条记录"的贪心分配。
🟡 建议 1:CONVERGED 只在所有 chunk 的末次干净轮同奇偶时才可能触发
converged 要求所有退休 chunk 本轮都不到期,即所有 chunk 的 (round - lastDry) 同为奇数。两个 lastDry 奇偶不同的 chunk 因此永远无法在同一轮同时被跳过(A: dry 1,2;B: r1 出货、dry 2,3 → 第 4 轮 A 冷查 B 跳过,第 5 轮 A 跳过 B 冷查)。于是只要有一个 chunk 在第 1、2 轮出货(大 PR 的常态,也正是 PR 描述里那张网格的形状),循环就永远走不到干净收敛出口,只能撞 5 轮上限并报告"硬上限"而非"已收敛"。
轮次本身变小了,所以不是性能回退;但配合 SKILL 把两干净轮规则改写为仅限 3A,3B 路径实际上除 exit 4 与硬上限外再无提前出口。把奇偶锚定改成全局(如 round % 2 配固定纪元,或"上一轮被跳过则本轮到期")可让错位的 chunk 重新对齐。
🟡 建议 2:流水线化会让被否决的发现压制一轮复查
累计清单现在带未验证条目,而 auditor brief 要求不重复报告清单内容。若第 k 轮报了一条 x.ts:20 的误报,第 k+1 轮的 auditor 被告知该处已覆盖;verifier 随后否决、下次合并时移出——但第 k+1 轮已经过去了。旧顺序下被否决条目根本不会进入清单。
窗口只有一轮一处,上限仍然兜底,我不认为这该挡住改动;但 PR 描述里"检测语义不变"说过头了,SKILL 那条 bullet 若能点名这个代价会更诚实。
🟡 建议 3:预算闸同样会拒绝单 chunk 的修复重建
agent-prompt.ts:1690 只按 role === 'reverse-audit' 判断,不区分"准入新一轮"与"重建已准入轮次的某个 chunk"。于是截断后的 --chunk <id> --round k 修复会得到 exit 4,并写下一条 stopped before round k 的 marker——而第 k 轮其实跑过了。两个后果:截断的投递无法修复(该 chunk 的 auditor 永不发射,也没有任何 unreviewedDimensions 条目),且披露文本关于"哪一轮停止"是错的。退休逻辑已经把 --chunk 路径豁免为"修复不是调度"(那个用例写得很好),同样的理由适用于预算闸。
另外:重建第 k 轮时 expectedRoundSeconds 会刻意越过第 k 轮自己的戳去读第 k-1 轮的,于是"估算"包含了第 k 轮已耗时间——恰好在最可能被拒的路径上系统性偏高。
🟡 建议 4:表头的截断自检已经不描述实际输出
agent-prompt.ts:1526 仍写着"输出以 end-of-round 行结尾",但有退休时 retirement: 注记被追加在其后。字面判据("若缺失")仍成立,但这句话是要挺过压缩的那句,现在描述的形状 builder 已不再产出。建议补一个从句。
其他小点
--all-chunks不带--round时,stampRound(planPath, undefined)跳过去重、每次调用追加一个{round: null}戳,expectedRoundSeconds(plan, undefined)随后读到最近那个。只要 SKILL 始终传--round就无害,但这是一个整体 fail-open 的模块里唯一 fail-closed 的角落。DRY_RECEIPT_RE匹配不到"Nothing further to report — re-walked…"这类实质回执。朝多审计失败,安全;只是比 brief 契约更窄。- workflow:
attempt_timeout低至 30s 时,600s 保留下限会直接跳过整个反审循环——注释说这是预期行为,同意,值得保留这段注释。 3600↔DEFAULT_RESERVE_SECONDS的"保持同步"是一条跨 YAML/TS 的纯注释不变量,无人能捕获漂移。风险低(两者都是兜底),但 TS 侧也加一句反向指引会更好。
测试覆盖
已有部分很扎实——用 harness 形状的 transcript 逐轮驱动真实 handler,远好于直接单测 scheduleReverseAuditRound;负向断言(无记录、无戳、exit 4 优先于 exit 5)选得对。缺口:
- 无 chunk 间奇偶错位的调度用例(建议 1)——正是 PR 描述那张网格的形状。
- 无同一 plan 路径第二次运行的用例(Critical 1)。退休路径靠继承 transcript 围栏是安全的,
deadline.ts不是,且无人覆盖。 - 无一份 transcript 匹配多条记录的用例(Critical 2)。
- workflow 新增的 shell 算术(下限/上限、按 attempt 重算)无覆盖;现有的"抽出 run 步骤跑 fixture"手法可以低成本补上。
安全 / 性能
无新攻击面:没有任何路径来自模型(deadline.ts 与 retirement.ts 都从 planPath / CLI 导出的环境变量推导,与模块其余部分一致),无新增网络或 shell。第 3 轮起每个 --all-chunks 轮多一次 readTranscripts——对一个已在 Step 3D 与 Step 6 读过的目录做 O(transcripts) 解析,相对于数十分钟的一轮可忽略。按所示网格折算的收益看起来可信;流水线那一半更可靠,因为它不依赖 chunk 变冷。
|
@qwen-code /takeover |
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. ✅
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #8498 (round after @wenshao's review)All feedback points from the review are addressed below. Both Criticals are Critical 1 — budget state survives into the CI in-job retry (FIXED)Both on-disk budget states —
The fence was chosen over clearing Tests: Critical 2 — one transcript can retire every chunk (FIXED)
Test: Suggestion 1 — staggered parities make CONVERGED unreachable (FIXED)Cold checks now land on ONE global parity — the even rounds — instead of each Suggestion 2 — pipelining suppresses one round of rediscovery for a later-rejected entry (ADDRESSED)The SKILL's merge bullet now names the trade explicitly: "an entry a verifier Suggestion 3 — the budget gate refuses single-chunk repair rebuilds (FIXED)
Suggestion 4 — header truncation self-check no longer describes the output (FIXED)The Smaller notes
Deferred
Failed checks noteAll checks listed as failed for this PR are Verification
中文说明处理审查意见总结 — PR #8498（@wenshao 评审后的一轮）评审中的每一条意见都在下面逐条处理。两个 Critical 均已修复，并以可复现的测试钉死；四条 Suggestion 全部实现；其他小点中两条已修、一条带证据拒绝、一条无需改动。一项测试覆盖缺口带理由推迟。 Critical 1 — 预算状态带入 CI 同 job 重试（已修复）两份磁盘预算状态 ——
选择围栏而非在 capture 时清空 测试： Critical 2 — 一份 transcript 可以让所有 chunk 退休（已修复）
测试： 建议 1 — 奇偶错位让 CONVERGED 永远不可达（已修复）冷查现在落在同一个全局奇偶上 —— 偶数轮 —— 而不再是每个 chunk 自己证书的奇偶。各自的锚永远无法重新对齐（一个 dry 于 2、3 轮的 chunk 与一个 dry 于 1、2 轮的 chunk 会在相反的轮次永远轮流冷查），于是在错位的大 PR 形状上 all-retired 的 建议 2 — 流水线化会让被否决条目压制一轮复查（已处理）SKILL 的合并 bullet 现在点名这个代价："an entry a verifier later rejects will have suppressed one round of rediscovery in its neighbourhood — the window is one round in one location, and the 5-round cap still bounds the loop。"PR 描述里"检测语义不变"的措辞不是本轮可编辑的文件；SKILL 文本（下一次运行真正会读的部分）已更正。 建议 3 — 预算闸拒绝单 chunk 的修复重建（已修复）
建议 4 — 表头截断自检已不描述实际输出（已修复）
其他小点
推迟项
关于失败检查本 PR 列出的失败检查全部是 验证
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/模型 |
Review —
|
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed — PR #8498Feedback source: @wenshao's issue-level review comment (2026-08-04). No inline 🔴 Critical — the anti-shortcut guard was inverted (retirement.ts)Fixed. The guard counted transcripts per record ( 🟡 Suggestions
🔵 Minor
Failed checks
Verification
中文说明已处理的审查反馈 — PR #8498反馈来源：@wenshao 的 issue 级审查评论（2026-08-04）。本轮没有行内评论，所有处置记录在此。未要求解决 base 冲突（ 🔴 Critical — 反捷径守卫方向反了（retirement.ts）已修复。 守卫统计的是每条 record 被几条 transcript 匹配（ 🟡 建议
🔵 小项
失败检查
验证
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/模型 |
ReviewReviewed the two commits this PR owns on top of #8468 ( Verified locally
What works well
Suggestion 1 —
|
|
🤖 AutoFix ran out of time before finishing (timeout (7200000ms)) (attempt 3/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/30897484008 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /takeover stop |
qwen-code-dev-bot
removed
the
autofix/takeover
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 |
|
All four Suggestions and the Nice-to-haves are in, across S1 — the S2 — S3 — the classifier now reads the receipt's structure: the no-issues phrase (en variants + S4 — the cumulative list tags every not-yet-verified entry NTHs — transcripts are pre-filtered on the builder's role marker and each survivor's launch prompt is flattened once ( Two more rounds of internal review on the amended head surfaced one further hazard, fixed in
— via Claude Code (claude-fable-5) |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
@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: 126 passed · 0 failed · 126 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：126 通过 · 0 失败 · 126 总计 Verification reportPR 8498 verification (round 3) —
|
| # | Round-2 finding | Severity | Status at the new head (re-measured, not diffed) |
|---|---|---|---|
| F1 | Fullwidth-colon receipts never classify as dry: [::] was two ASCII colons, contradicting its own comment |
Suggestion (bounded, fails toward auditing) | Fixed by this PR (commit 80d4d93), re-measured. Byte-level: the compiled dist separator class is 0x5b 0x3a 0xff1a 0x5d — U+FF1A present (witness 04-f1-ladder-head-bytes.png). Behavioural: the fullwidth-colon zh receipt retires its chunk through the real compiled CLI (A/B cell GzhFull: exit 5 CONVERGED on head vs 3 built on base) and through the compiled scheduler directly (6/6 separator/phrase variants retire, incl. ASCII-colon parity). The new pin a Chinese receipt separated by a full-width colon is dry kills the counter-fix mutant (M1 row: reverting to [::] turns exactly that test red) — the fix shipped with its fixture, as round 2 asked. |
| M4 | diffToolCalls > 0 clause unpinned |
Coverage gap | Now pinned. Commit c11df3b split the two tool-call guards; deleting the clause turns exactly the split test red (successful calls that never touched the diff are not dry — the two guards are independent, M4 row). Characterization, measured: the pin exercises the clause through the empty-territory fallback (synthetic records bake no read range); on production records — which always bake the chunk's read_file range — the new territory-overlap check subsumes it. The clause remains the guard for the fallback path, and is now asserted. |
| M7 (round 2: superseded — pinned in main) | Marker-vs-relayed dedupe unpinned | Coverage gap | Pin re-confirmed live. Positive control this round: disabling the BUDGET_STOP_PHRASE splice in a scratch copy turns exactly a budget-stop marker caps APPROVE at COMMENT with nothing relayed by the caller red (1/161), so the suite both passes and can fail on this axis. |
| M10 | Bare-subject vs subject — dedupe distinction unpinned (compose-review) |
Coverage gap (not a merge condition) | Stands (this round's M11 row). Removing `d === e.subject |
| C14 (round 1; fixed in main, not by this PR) | Budget-stop disclosure silently dropped when a coverage subject reverse audit collides |
Suggestion (bounded) | Still fixed at the moved base/head. Re-measured through the real compiled review compose-review on BOTH arms (witness 03-c14-collision-fixed-both-arms.png): collision / clean / marker-only cells each render the disclosure exactly once, the other reverse-audit scope (chunk 2's auditor returned nothing substantive twice) survives the collision, and the verdict stays capped (COMMENT, no APPROVE) — 20/20, identical on base and head. The first cut of this probe had an mtime bug that fenced the marker out (count=0 on the marker-only cell); fixed and noted so the witness is honest. The budget entry's own verdict cap is not isolated in these minimal states (the transcripts-missing coverage gap also caps); it is pinned by the suite test named above. |
Central claim + A/B
Central claim (this PR): from round 3, agent-prompt --all-chunks is the loop's scheduler — a chunk whose two most recent audits were substantive dry receipts (receipt names what it examined AND the transcript shows the chunk's baked territory was opened) is retired to alternating even-round cold checks, returns to every-round auditing on any yield, and an all-retired round exits 5 (CONVERGED) with nothing built, recorded, or stamped; every classification failure falls toward auditing. Secondary claims verified: convergence outranks the budget gate (exit 5, no marker); a stamped round's --chunk repair bypasses the expired gate while an unstamped --chunk round answers the same convergence-then-budget sequence; the plan-mtime fence drops a dead attempt's records and transcripts; and every classifier guard added by commit 80d4d93 holds.
Both arms drive the real compiled CLI (packages/cli/dist/index.js review agent-prompt …) over synthetic 3-chunk review states; history is established by running each arm's own builder for rounds 1–2 and delivering harness-shaped JSONL transcripts whose launch prompts are byte-for-byte copies of the CLI's own recorded blocks. Base = tmp/base-tree at 32e2741 with packages/cli rebuilt there (control-cleanness in Methodology). Table 1 — 86 assertions, all passed (base-control behaviors are asserted expectations, not failures). Witnesses: 01-ab-head-central-flips.png, 02-ab-base-controls.png.
| Cell | Scenario | base 32e2741 (control) |
head 80d4d93 |
|
|---|---|---|---|---|
| C1/C2 | rounds 1–2, any history | 3 auditors, exit 0 | identical | PASS |
| C3 | all chunks dry ×2 → r3 | 3 built, exit 0 | exit 5 CONVERGED, 0 built, 0 records, no stamp, no marker | PASS |
| C4 | chunks 1,2 dry ×2, 3 yielded → r3 | 3 built | 1 built (chunk 3); note names 1,2; records = [3]; stamped | PASS |
| C5 | same history → r4 (even) | 3, no tags | 3 built, chunks 1,2 tagged (cold check); tag never enters recorded prompts |
PASS |
| C6 | chunk 2 whiffed r2 → r3 | 3 | 2 built ([2,3]); note names only chunk 1 | PASS |
| C7 | ONE transcript carrying all three r2 records → r3 | 3 | 3 (concat attack certifies nothing) | PASS |
| C8 | transcripts dir absent → r3 | 3 | 3 (fail open, exit 0) | PASS |
| C9 | expired deadline, due round → r3 | exit 4, BUDGET line, marker, nothing built | identical (gate reorganization is behavior-preserving) | PASS |
| C10 | expired deadline + all-retired → r3 | exit 4 + marker | exit 5 (not 4), no marker — convergence outranks the gate | PASS (flip) |
| C11a | --chunk 2 --round 2 of a stamped round, expired |
exit 4 (uniform gate refuses the repair) | exit 0, prompt rebuilt (stamped-round repair bypass) | PASS (flip) |
| C11b | --chunk 2 --round 4 unstamped, expired |
exit 4 | exit 4 (unadmitted round is gated like --all-chunks) |
PASS |
| C11c | --chunk 2 --round 3 unstamped + all-retired, expired |
exit 4 | exit 5 CONVERGED, no marker — converged --chunk under deadline |
PASS (flip) |
| C12 | round-1 estimate 1800 + reserve 600, remaining 2600/2000 | exit 0 / exit 4 | identical (gate arithmetic unchanged) | PASS |
| C15 | plan re-captured after dry ×2 history → r3 | 3 | 3 (stale records + transcripts fenced, full audit) | PASS |
| GzhFull | zh receipt 未发现问题：… (fullwidth colon, F1) ×2 → r3 |
3 built | exit 5 — retired | PASS |
| GzhAscii | zh receipt with ASCII colon ×2 → r3 | 3 built | exit 5 — retired (width parity) | PASS |
| Gparrot | r2 = the brief's own example receipt, verbatim | 3 | hot ([1,2,3] built) — parrot refused | PASS |
| Gbacktick | No new issues found — all good. + stray backtick |
3 | hot — a stray backtick is not a named object | PASS |
| Gandor | No issues found — and/or cases. |
3 | hot — the conjunction is not a path | PASS |
| Gspan | No issues found — \retry-cap`.` ×2 |
3 | exit 5 — enclosed span names an object | PASS |
| Gpath | No issues found — checked src/pay.ts. ×2 |
3 | exit 5 — dotted path names an object | PASS |
| GterritoryOut | only diff read lands OUTSIDE the baked territory | 3 | chunks 1,2 hot; chunk 3 (overlap) retires | PASS |
| GterritoryOverlap | half-read overlapping the territory ×2 | 3 | exit 5 — overlap is the bar, not containment | PASS |
| GechoFileOnly | **File:** echo without any Severity + dry receipt |
3 | exit 5 — echo is not a yield | PASS |
| GechoFull | quotes a WHOLE list entry (File+Severity) present in its own launch prompt | 3 | exit 5 — quotation is not a filing | PASS |
| GrealYield | files File+Severity for a file NOT in the launch prompt | 3 | hot ([1,2,3]) — a real filing outranks the receipt | PASS |
Skipped chunk-rounds write no prompt records (C4: records = [3] only), so the coverage roster owes nothing for retired chunks — the check-coverage interplay claim holds by construction, as in rounds 1–2.
Corrections
None. (Round 2's metadata note still applies in updated form: the body says "its own two commits" while the snapshot now carries three; not load-bearing — the aggregate diff was verified.)
Findings
None new. All round-2 findings are resolved or carried as non-blocking completeness items (status table above). Two completeness notes for the author, neither a merge condition:
- Carried M10/M11 — the bare-subject dedupe branch in
compose-review.tsremains unpinned (161/161 green after deletion; suite liveness proven by the splice positive control). The file is outside this PR's diff; worth a fixture when convenient since it is the axis round-1's C14 collision rode. - M4's pin exercises the empty-territory fallback. Production records always bake the chunk's read range, where the new
openedTheTerritorycheck subsumesdiffToolCalls > 0. The clause is live defense for a prompt with no baked read; the characterization is recorded so a future refactor doesn't read the pin as covering the baked-range path.
Considered and dismissed: classifier scaling (receipts are model returns read from harness transcripts and the CLI's own records — not outsider-authored text — and the regexes are linear: bounded lazy fillers, one greedy tail).
Not covered
- Change 2 (verification pipelining) has no behavioural oracle — it is orchestrator prose in SKILL.md. Checked textually only: the load-bearing orderings survive the rewrite (the last round's verification completes before Step 6; entries still tagged
— [unverified]are structurally excluded from Step 6's confirmed-only read; the Step 5 merge runs unconditionally before every round build and before Step 6; a verifier-never-ruled entry caps a would-be Approve at COMMENT; CONVERGED exit 5 is a clean termination owing nounreviewedDimensionsentry). The projected 95→55-minute loop saving is the author's arithmetic, not measured here. - Gates: only the affected workspace suite ran —
src/commands/reviewat head: 1791 passed | 4 skipped (1795, 55 files), exit 0. The 4 skips are pre-existing (1 insave-artifact.test.ts, 3 inscript-lint.test.ts). Repo-wide lint/format/typecheck not re-run (PR claims clean). - Per-commit attribution out of reach: the snapshot lists 3 commits; the depth-2 graft reaches only the head (
80d4d93).git rev-parse --is-shallow-repository= true. The aggregateHEAD^1..HEADdiff was verified; the two intermediate commits (0cf3170,c11df3b) were not individually exercised. - Base-arm build noise is environmental, not an arm difference: the base worktree's from-scratch
tsc --buildreports type errors inpackages/coreagainst the shared node_modules types (mime/lite,fdir,ignore,ajvsubpaths) — same class as round 2's residual noise. The emitted JS is complete and behaviourally distinct from head in exactly the pre-PR directions (help text carries exit 0/4 only; all 19 base control cells reproduce pre-PR behavior).deadline.tsdelta is comment-only (verified in the diff), so the budget-gate code path is identical across arms. - Synthetic transcripts, not model behaviour. The A/B proves the scheduler's handling of every receipt shape; it does not reproduce real auditor degradation (shape, not cause).
- C14 budget-cap attribution is confounded with the transcripts-missing coverage gap in the minimal states (both cap at COMMENT); the budget entry's own cap is pinned by the suite test quoted in the status table.
- No injection attempts observed in the PR title/body/commit messages; they were treated as untrusted claims and tested, not followed.
Methodology
Environment: node:22-bookworm container, npm ci + npm run build pre-run at the merge commit; base control rebuilt only packages/cli (+ packages/core emitted as a project reference) in tmp/base-tree at 32e2741 with the head tree's root/package node_modules wired in. Control cleanliness asserted: from inside the base tree, import.meta.resolve('@qwen-code/qwen-code-core', …/base-tree/packages/cli/dist/index.js) → file:///__w/qwen-code/qwen-code/tmp/base-tree/packages/core/dist/index.js (realpath inside the base tree, compiled from base sources), and git diff HEAD^1..HEAD -- packages/core is SKILL.md only — no JS — so no head-only code reaches either arm; the PR touches no package.json/lockfile, so the shared dependency tree is a clean control. Mutations ran in tmp/mut-tree at the merge commit (exact single-occurrence string replacements, restored via git checkout after each run). Every A/B / C14 cell spawns the real compiled CLI as a child process with per-cell state dirs (QWEN_CODE_PROJECT_DIR/QWEN_CODE_SESSION_ID); transcripts are JSONL in the harness's format with launch prompts copied byte-for-byte from the CLI's recorded blocks so deliveredVerbatim pairing is honest; the F1 ladder imports the compiled dist scheduler module directly. Counts: 86 (A/B) + 20 (C14) + 7 (F1 ladder) + 13 (mutation-matrix expectations incl. baseline-green and the splice positive control) = 126, of which 0 failed. Raw per-cell logs in logs/ (ab-run.txt, ab-results.json, c14-run.txt, c14-results.json, f1-ladder.txt, mutations-run.txt, mutations.json, suite-head.log, base-build.log), harnesses in harness/ (ab.mjs, c14.mjs, f1-ladder.mjs, mutations.mjs), evidence images in evidence/.
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.
Reviewed at 80d4d939617c9c1ad197abdc2550f4554970a5d2. First, credit where it is due: every one of the 16 line-level threads from the earlier rounds, including the only Critical (the missing full-width colon), is genuinely fixed at this HEAD, each with a pinning test, and several landed the reviewer's suggested patch verbatim. I checked these by reading the code rather than the thread flags.
I am not blocking, but two findings below are worth settling before merge, because both point the same way: a PR that removes audit work should not also widen the conditions under which work is skipped.
Warning 1 — the territory check can be widened by findings prose, reopening the hole it was added to close
retirement.ts computes a record's territory with territory: bakedRanges(prompt), where prompt is the recorded prompt read back from disk. But the recorded prompt is not the bare launch prompt: agent-prompt.ts records printed = foldFindings(role, findingsContent, prompt), and for reverse-audit findingsSection inserts content.trim() raw into the "Already confirmed — do not re-report these" block.
So the cumulative findings list is inside the string that bakedRanges scans, and its regex /offset\s*[=:]\s*(\d+)\s*,\s*limit\s*[=:]\s*(\d+)/gi matches anywhere in it. Because openedTheTerritory passes when any diff read overlaps any territory range, every extra range can only ever widen the bar — never tighten it.
Concretely: a finding whose prose quotes offset=0, limit=50 injects the range [1, 50]. An auditor whose only diff read was lines 1-50 then satisfies openedTheTerritory for a chunk whose real baked territory is, say, lines 1000-1200 — so it can earn dry, retire, and contribute to CONVERGED without ever having read the territory it was accountable for. With the real baked range alone it correctly fails.
This does not need adversarial input. Any PR whose findings discuss read_file offsets produces that text — this PR's own review threads do. It is the same range-blind hole the bakedRanges / openedTheTerritory pair was introduced to close, and it points in the skipped-audit direction that the module header calls impossible. Binding the scan to a real diff read — requiring the file_path in the same read_file(...) call to name the diff, or scanning only the region below the folded findings section — would close it.
Warning 2 — a documented safety margin was consumed by a comment-only change
deadline.ts has zero non-comment changes in this diff, and DEFAULT_RESERVE_SECONDS is still 3600.
What changed is the justification. The base comment called the verification pass inside the admission-to-admission span "deliberate margin, not double-entry bookkeeping that slipped", and kept it precisely because "round costs trend UP ... so the previous round's measurement under-predicts the next in exactly the runs that end near the boundary". Pipelining moves that verification out of the measured span, and the new comment accordingly says the reserve is now its "exactly one cover" — while keeping the same under-prediction reasoning, which no longer has any margin to absorb it.
So the estimator, the reserve constant, and the workflow's scaled reserve are all unchanged, but the overlap that covered the acknowledged under-prediction is gone. The error direction is the one the same comment names as #8368: killed mid-verification, holding every confirmed finding. Pricing a round from max(observed spans) rather than the most recent one, or raising the reserve by one verification pass, would restore the asymmetry the comment still argues for.
Suggestion — nextColdCheck can print a round the cap forbids
nextColdCheck: round + 1 has no awareness of the 5-round hard cap, so a chunk retired and skipped at round 5 prints next cold check round 6. That note is the only thing the orchestrator is told about the chunk, so it asserts a future audit that cannot happen and gives the reader no reason to weigh a disclosure. Either clamp it and say so, or have the note name the certificate as final.
Suggestion — "structurally excluded" claims a guarantee the code does not implement
SKILL.md says anything still tagged — [unverified] is "structurally excluded" and that "nothing unverified reaches the report or the PR", but nothing in the codebase parses that tag. The real backstops are coarser: unverifiedFindings derives from a floor-of-one across all verify--* deliveries, so round 1's verifier record satisfies it even if the terminal round's verification never ran, and criticalsUnverified only gates on Criticals, leaving Suggestions uncapped. The unreviewedDimensions cap now in SKILL.md closes the Critical-shaped hole, so this is wording rather than a defect — but "structurally excluded" reads as a code guarantee.
For the record, the fail-open surfaces I checked and found sound: classifyReturn requires all four conditions conjunctively; a missing diff path zeroes diffToolCalls so every chunk stays hot; converged cannot coexist with a twice-whiffed unaudited territory; the plan-mtime fence invalidates all history on re-capture; Step 5's floor-of-one cannot be tripped by retirement because rounds 1 and 2 always fan out fully; and the injective guard blocks a single launch from matching many records to retire everything at once.
Review —
|
…ce the budget gate (#8498)
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Review round summary — PR #8498 (commit 49e4f1e)All four findings from @doudouOUC's review at Warning 1 — findings prose could widen the territory check → FIXEDConfirmed as described: the recorded reverse-audit prompt is the folded launch prompt — Fix: the scan is now bound to the diff's own path — it only accepts Warning 2 — the pipelined loop consumed the reserve's documented margin → FIXEDConfirmed: pipelining moved the verification pass out of the measured admission-to-admission span, so the reserve became the terminal round's only cover while the estimate still priced from the newest span, leaving the acknowledged under-prediction nothing to absorb. Implemented the reviewer's first option: Suggestion —
|
|
Perf read (this is the key review-perf change, vs #8548 which only cuts CI build redundancy): the dominant cost in multi-round reverse-audit is re-auditing already-clean chunks every round, and retiring dry chunks makes round cost shrink as the audit converges — that's the real token/wall-time win. Pipelining verification off the serial path shortens the critical path too. The perf/quality balance is sound: retirement requires two certified, diff-opening, substantively-receipted audits (fail-open on any ambiguity) plus staggered even-round cold checks as a regression backstop, so the speedup doesn't buy missed findings. One perf-related caveat to watch (also in my review): retirement makes odd rounds cheaper but expectedRoundSeconds prices from the last span, so a round admitted after a shrunk round is under-estimated — same under-reserve direction #8368 flagged; consider pricing from max(observed spans). |
Review at head
|
| round | schedule under this PR | on main |
|---|---|---|
| 4 | every chunk retired, 4 % 2 === 0 → all cold-checked, full N-auditor round |
— loop already stopped |
| 5 | all skipped → CONVERGED, exit 5 |
— |
main stops after round 3 on the two-consecutive-dry-rounds rule. This PR runs one more full fan-out first. On the 6-chunk benchmark in the PR body that is 6 extra auditors / ~20 minutes, against the ~40 minutes the change projects saving — and it lands on runs that converged early, i.e. the ones with the most to lose from an unnecessary round.
The PR's own rationale is the argument for the fix:
the per-chunk certificates being collectively the same evidence the global two-dry-rounds rule demanded
If every chunk holds the certificate, there is nothing hot left for a cold check to protect — the cold check earns its keep only while some other territory is still under audit and the loop is running anyway. Suggested change: classify every chunk first, and if all are retired return converged: true regardless of parity; apply the even-round cold check only when at least one chunk is hot. Two tests move with it — retirement.test.ts:322 and the handler's 'a cold-check-only round is still built, admitted and stamped'; note the latter only reaches round 4 by skipping round 3, which the real loop never does (round 3 exits 5 on that fixture).
Related, and worth settling in the same pass: the SKILL re-scopes "Stop after two consecutive dry rounds" to 3A with a parenthetical, but the bullet immediately above it ("A round is dry only when every agent in it returned zero new findings") still reads as the 3B rule. If the orchestrator keeps applying it on 3B, exit 5 will rarely fire; if it drops it, the extra round above bites. Say explicitly which rule owns 3B.
🟡 Suggestion 2 — the — [unverified] tag is the only thing holding Change 2 up, and nothing enforces it
Change 2's safety argument reduces to four orchestrator behaviours: tag on entry, untag on a confirmed verdict, drop the entry on a rejected one, and filter on the tag at Step 6. There is no CLI support for any of them — compose-review's unverifiedFindings is the "no verifier ran at all" floor from verificationGaps, not a per-entry notion, so a dropped tag posts an unverified finding as confirmed with nothing downstream to notice.
This subsystem's whole design premise is that the orchestrator cannot be trusted with prose (transcripts.ts' opening comment: "Evidence authored by the subject is not evidence"; prompt-record.ts' measurement of a 4 652-char prompt delivered as 2 893). Moving a hard ordering — verify, then merge — onto a tagging convention is the one place in this diff that runs against that grain. The SKILL adds a prose backstop at the end of Step 5, which is the right instinct, but it is more prose.
Cheap mechanical teeth: have compose-review refuse to place any finding whose text still carries — [unverified] in the confirmed set — emit it terminal-only and add the unreviewedDimensions entry itself, exactly as the SKILL asks the orchestrator to do by hand. Then the tag is a gate rather than a note-to-self, and the pipelining is safe even when the merge discipline slips.
Also: "with detection semantics intact" in the PR body overstates it slightly. The SKILL text is honest about the trade (an entry a verifier later rejects suppresses one round of rediscovery in its neighbourhood) — the PR body should say the same.
🟡 Suggestion 3 — the territory is re-parsed out of the prompt, and the anti-injection binding is a path a finding can legitimately carry
bakedRanges (retirement.ts:120) scans the whole record — findings included, since the record is the folded prompt — and defends against injection by accepting only reads whose file_path equals the run's diff path. That closes the generic case, and the test at retirement.test.ts:608 is a good one. The residual: a reverse-audit finding about this subsystem quoting a real builder-emitted line carries the run's own absolute diff path, and openedTheTerritory passes on any overlap — so an injected range can only ever widen the territory, i.e. fail in the one direction the module promises never to take.
Low probability, but it is avoidable rather than mitigable: the territory is already known authoritatively — it is the offset/limit buildLaunch computed from the chunk. Deriving it from the plan's chunk record instead of regexing it back out of the prompt removes the class. Failing that, foldFindings puts the findings immediately after the identity line and the builder's own text always below them, so scanning only the tail is a two-line narrowing.
🟡 Suggestion 4 — the pairing walk hoists only half the flattening
retirement.ts:409-415 flattens each candidate transcript once (good) and then calls deliveredVerbatim(c.flat, rec.prompt) per pair — but deliveredVerbatim runs lines(built) on the record side (prompt-record.ts:255), splitting and re-flattening the multi-KB record on every (record, transcript) pair. The comment at prompt-record.ts:226-234 claims the flattening is paid per transcript; it is paid per pair on the record side. Hoisting lines(rec.prompt) once per record makes it O(records + transcripts) flattens instead of O(records × transcripts) — at round 5 with 6 chunks that is ~24 flattens instead of ~576, on the critical path before the round is admitted.
Smaller notes
substantiveClauseis doing less work than its doc suggests.DRY_RECEIPT_RE's capture group is[\s\S]*— everything after the separator to end-of-text — so the 20-character floor atretirement.ts:226is met by essentially any multi-sentence return. The real teeth arediffToolCalls > 0+openedTheTerritory, which is fine; the comment just reads as if the clause bar carries more of the load than it does.- The parrot refusal is wrap-fragile.
EXAMPLE_RECEIPT_CLAUSE(retirement.ts:198) is only.trim()ed, while the candidate clause is whitespace-collapsed before theincludestest. The constant is one line today; if it is ever hard-wrapped inagent-briefs.tsthe refusal silently stops matching (fail-open, but silently). One.replace(/\s+/g, ' ')on the constant makes it wrap-proof. - Two fences, two epochs.
readRecordedPrompts' fence is the exact plan mtime (prompt-record.ts:176), whilereadRoundStamps/readBudgetStopusemtime - RUN_EPOCH_SLACK_MS. Both sides here are file mtimes, so the fractional-vs-integral skew the slack absorbs doesn't apply — correct as written, but worth half a sentence so the next reader doesn't "fix" the inconsistency in the wrong direction. expectedRoundSeconds' max-span sticks to a one-off outlier. Switching from newest-span to costliest-span is the right failure direction, but a round 1 that was slow for a reason that won't recur (cold start, a repair relaunch) now prices every remaining round for the rest of the loop — and a budget stop caps the verdict atCOMMENT. Consider the max of the last two spans, or excluding round 1, to keep the safety without pinning to an outlier. Not a blocker; just note it pushes in the opposite direction from the PR's headline goal.- The
(cold check)separator tag is new and never explained to the orchestrator. The SKILL bullet describes cold checks conceptually, but the model is separately told to translate separator labels into the Agentdescription— and it will now see a label it wasn't told about. One clause ("a block tagged(cold check)is launched exactly like any other") removes the ambiguity. - Exit 5 is documented in the CLI
describeand the SKILL only. If any doc underdocs/users/features/enumeratesagent-prompt's exit codes it should learn about 5; I didn't find one, so this is probably a no-op.
中文摘要
上一轮的两个 Critical 与 Suggestion 3/4 在本 head 已全部关闭并有测试固定；injectivity 反向成"每条 transcript 匹配几条 record"是比原建议更正确的方向。
需要在合并前定夺的一条：converged 被 parity 卡住（retirement.ts:472/497）——所有 chunk 在第 3 轮拿到证书时，第 4 轮（偶数轮）仍会全量冷抽查一整轮，而 main 在第 3 轮就已按"连续两轮干净"停止。这一形态下比现状多跑一整轮扇出（基准上约 6 个 auditor / 20 分钟），恰好发生在收敛最早、最不该多跑的运行上。建议：先分类全部 chunk，全部退休即 converged: true（与 parity 无关），冷抽查只在仍有热区时才需要。同时把 SKILL 里"连续两轮干净"到底归 3A 还是 3B 说死。
第二条：— [unverified] 标记是 Change 2 唯一的安全支点，却完全没有 CLI 兜底——compose-review 只有"是否跑过 verifier"的下限判断，没有逐条概念。整个子系统的前提就是不信任 orchestrator 的散文约定；建议让 compose-review 机械地拒绝把仍带该标记的条目计入 confirmed 集合。
其余为建议与小注：territory 从 prompt 回解析仍有同路径注入残留（建议直接取 plan 的 chunk range）、配对遍历只提升了一半的 flatten、substantiveClause 的实际强度弱于注释、parrot 拒绝对换行脆弱、两处 mtime 栅栏的 epoch 不一致值得注一句、expectedRoundSeconds 取最大跨度会被一次性离群值长期钉住、(cold check) 标签没在 SKILL 里向 orchestrator 交代。
未运行测试套件；以上均为静态审阅，每条都标了对应代码位置。
|
All 16 review threads verified against the current head ( The verification was empirical, not archival: each thread's originally-surviving mutant was re-applied to head — all 16 die to exactly the test the thread asked for — and the affected suites plus the full Worth noting for the record: — via Claude Code (claude-fable-5) |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-reviewed at 49e4f1e66d — my two Warnings from the previous round are both genuinely fixed. I verified the code rather than taking the commit title's word for it, since a fix title is a claim.
Warning 1 (findings prose widening the retirement territory) — fixed, and fixed at the right layer. bakedRanges now takes the diff path and requires the whole call shape, matching read_file(file_path="…", offset=N, limit=M and skipping any hit whose file_path is not the diff. My original injection vector was a bare offset=N, limit=M anywhere in the folded findings list, and that no longer matches anything. Binding to the diff read is the stronger of the two options I suggested — it fixes the cause rather than narrowing the scan window.
I also checked the new failure mode that a diffPath-gated territory introduces: diffPath === undefined now yields an empty territory, and openedTheTerritory returns true on an empty territory, so the territory check goes trivially true. That does not fail open, because classifyReturn requires rec.diffToolCalls > 0 as a separate conjunct on the same condition, and diffToolCalls is zero precisely when the diff path is missing. The chunk stays hot. Good that the two guards are independent.
The residual is much narrower than before but not quite zero: a finding whose prose quotes an auditor's tool call verbatim, with the run's actual diff path inside it, would still contribute a range. That needs the exact temp path in a finding's text, so I am not treating it as actionable — noting it only so the narrowing is on record as narrowing rather than elimination.
Warning 2 (the consumed budget margin) — fixed by repricing the estimator. expectedRoundSeconds no longer prices a round from the most recent span. It now filters the current round's stamps, walks every consecutive stamp pair, and returns the maximum observed span. That directly resolves the odd/even concern I raised: round 4's full fan-out is now priced against the largest span seen, which includes the always-full-fan-out rounds 1 and 2, instead of against the shrunken round 3. It also keeps the "round costs trend UP" reasoning in the comment honest, because a max is monotonic in the observations.
DEFAULT_RESERVE_SECONDS stays at 3600, which is fine — I had suggested max-pricing or a larger reserve, and pricing from the max is the better-targeted of the two since it fixes the estimate rather than padding around it.
Both changes come with tests (retirement.test.ts +50, deadline.test.ts +21, agent-prompt.test.ts +39), and CI is green on this exact SHA: run 30972342118 has head_sha=49e4f1e6.
Approving. The three still-open threads are all [Suggestion]-level test-coverage notes from the bot, with no unresolved Critical among them.
For completeness, my two lower-severity notes from last round still stand and remain non-blocking: nextColdCheck: round + 1 can still print a round the 5-round cap forbids, and SKILL.md still describes the [unverified] tag as "structurally excluded" when no code parses it.
|
@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: 177 passed · 0 failed · 177 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：177 通过 · 0 失败 · 177 总计 Verification reportPR 8498 verification (round 4) —
|
| # | Round-3 finding | Severity | Status at the new head (re-measured) |
|---|---|---|---|
| F1 | Fullwidth-colon receipts never classify as dry | Suggestion (bounded) | Fixed — stands. Byte-level: compiled dist separator class contains 0x3a 0xff1a (U5, both checks). Behavioural: fullwidth-colon zh receipt retires through the real CLI (GzhFull, exit 5 vs 3 built on base), ASCII-colon parity (GzhAscii), six separator variants all retire through the compiled scheduler (U6 ladder 6/6). Pin: reverting the class to [::] (M1) turns exactly a Chinese receipt separated by a full-width colon is dry red. |
| M4 (r3) | diffToolCalls > 0 clause unpinned |
Coverage gap | Pinned — stands. Deleting the clause (M3) turns exactly successful calls that never touched the diff are not dry — the two guards are independent red. Refinement found this round: the SIBLING clause rec.successfulToolCalls > 0 is dead code — see Findings #4. |
| M7 (r2/r3) | Marker-vs-relayed dedupe unpinned | Coverage gap | Pin re-confirmed live. Disabling the BUDGET_STOP_PHRASE splice (M7) turns exactly a budget-stop marker caps APPROVE at COMMENT with nothing relayed by the caller red (1 red), with the suite otherwise green (161 tests). |
| M10 | Bare-subject dedupe branch unpinned (compose-review) | Coverage gap (not a merge condition) | Stands. Removing d === e.subject || leaves compose-review.test.ts green (161/161); suite liveness proven by the M7 row above. File untouched by this PR's diff — carried completeness reporting. |
| C14 (r1; fixed in main) | Budget-stop disclosure dropped on reverse audit subject collision |
Suggestion (bounded) | Still fixed at the moved base/head. Re-measured through the real compiled review compose-review on BOTH arms (witness 06-c14-collision-parity-both-arms.png): collision / marker-only / relay-only cells each render the disclosure exactly once, the sibling scope chunk 2's auditor returned nothing substantive twice survives the collision, verdict capped at COMMENT — 11/11 per arm, identical. EN channel only this round (plan prDescriptionHasHan: false); see Not covered. |
Central claim + A/B
Central claim (this PR): from round 3, agent-prompt --all-chunks is the loop's scheduler — a chunk whose two most recent audits were substantive dry receipts (receipt names what it examined AND the transcript shows the chunk's baked territory was opened) is retired to alternating even-round cold checks, returns to every-round auditing on any yield, and an all-retired round exits 5 (CONVERGED) with nothing built, recorded, or stamped; every classification failure falls toward auditing. Round-4 additions verified: the baked territory is bound to reads aimed at the diff path (both the record side and the transcript side), and the budget gate prices the round from the COSTLIEST measured admission-to-admission span instead of the newest stamp.
Both arms drive the real compiled CLI (packages/cli/dist/index.js review agent-prompt …) over synthetic 3-chunk review states (territories 1001-1200 / 2001-2200 / 3001-3200 of a real 4500-line diff file); history is established by running each arm's own builder for rounds 1–2 and delivering harness-shaped JSONL transcripts whose launch prompts are byte-for-byte copies of the CLI's own recorded blocks. Base = tmp/base-tree at 8b0e8b8192 with packages/cli rebuilt there (control-cleanness in Methodology). Table 1 — head 67 + base 47 assertions, all passed (base-control behaviors are asserted expectations, not failures; each arm re-run once under the capture with identical counts). Witnesses: 01-ab-head-central-flips.png, 02-ab-base-controls.png.
| Cell | Scenario | base 8b0e8b8192 (control) |
head 49e4f1e66 |
|
|---|---|---|---|---|
| C1/C2 | rounds 1–2, any history | 3 auditors, exit 0, stamped | identical | PASS |
| C3 | all chunks dry ×2 → r3 | 3 built, exit 0 | exit 5 CONVERGED, 0 built, 0 records, no stamp, no marker | PASS |
| C4 | chunks 1,2 dry ×2, 3 yielded ×2 → r3 | 3 built | 1 built (chunk 3); note retires 1,2 (dry in rounds 1 and 2, next cold check round 4); records = [3]; stamped |
PASS |
| C5 | same history → r4 (even) | 3, no tags | 3 built; chunks 1,2 tagged (cold check) in separator only — tag never enters recorded prompts |
PASS |
| C6 | chunk 2 whiffed r2 (bare receipt, 0 calls) → r3 | 3 | 1 built ([2]); note retires 1 and 3, not 2 | PASS |
| C7 | ONE transcript carrying all three r2 records → r3 | 3 | 3 (concat attack certifies nothing) | PASS |
| C8 | transcripts dir absent → r3 | 3 | 3 (fail open, exit 0) | PASS |
| C9 | expired deadline, round still due → r3 | exit 4, BUDGET line, marker r3, nothing built/stamped | identical (gate reorganization behavior-preserving) | PASS |
| C10 | expired deadline + all-retired → r3 | exit 4 + marker | exit 5, no marker — convergence outranks the gate | PASS (flip) |
| C11a | --chunk 2 --round 2 of a stamped round, expired |
exit 4 + marker (uniform gate refuses the repair) | exit 0, prompt rebuilt, stamp count unchanged | PASS (flip) |
| C11b | --chunk 2 --round 4 unstamped, expired |
exit 4 | exit 4 (unadmitted round gated like --all-chunks) |
PASS |
| C11c | --chunk 2 --round 3 unstamped + all-retired, expired |
exit 4 + marker | exit 5 CONVERGED, no marker | PASS (flip) |
| C12a/b | round-1 constant pricing: remaining 2600 / 2000, reserve 600 | exit 0 / exit 4 | identical | PASS |
| C15 | plan re-captured after dry ×2 → r3 | 3 | 3 (stale records + transcripts fenced) | PASS |
| P1 | stamps 50/20 min old (spans 30/20), remaining 2100, reserve 600 | exit 0 (prices newest = 1200s) | exit 4 + marker (prices costliest = 1800s) | PASS (flip) |
| P2 / P2b | spans below the 600s floor / newest IS costliest | admit | admit (parity) | PASS |
| P3 | stamps 60/40/10 min old — the MIDDLE span is costliest (30 min), remaining 950, reserve 300 | exit 0 (prices newest = 600s) | exit 4 + marker (prices middle = 1800s) | PASS (flip) |
| P4a/b | single stamp: remaining 1520 / 1400 | exit 0 / exit 4 | identical | PASS |
| T1 | findings quote read_file(file_path="src/pay.ts", offset=0, limit=50); auditors read only diff 1-50 ×2 |
3 built | 3 built, no retirement note — the other-file quote does not widen the territory | PASS |
| T2 | findings quote the window aimed at the DIFF path itself (residual probe) | 3 built | exit 5 — still injects (Finding #1) | measured |
| T3 | same other-file noise, auditors read the baked territory ×2 | 3 built | exit 5 — real reads retire beside noise | PASS |
| GzhFull / GzhAscii | zh receipts, fullwidth / ASCII colon ×2 | 3 | exit 5 / exit 5 (width parity, F1 carried) | PASS |
| Gparrot | r2 = the brief's own example receipt | 3 | hot (parrot refused) | PASS |
| Gbacktick / Gandor | stray backtick / and/or clause |
3 | hot | PASS |
| Gspan / Gpath | enclosed span / dotted path clause | 3 | exit 5 | PASS |
| GterritoryOut | only diff read lands OUTSIDE the baked territory | 3 | hot | PASS |
| GterritoryOverlap | half-read overlapping the territory | 3 | exit 5 | PASS |
| GechoFileOnly / GechoFull | File echo without Severity / whole-entry quotation | 3 | exit 5 | PASS |
| GrealYield | files File+Severity for a file NOT in the launch prompt | 3 | hot | PASS |
Unit probes against the compiled modules (witness 03-repricing-costliest-span-units.png): head expectedRoundSeconds prices P1 and P3 at exactly 1800s, single-span 900s, no-stamps 1800s; base prices 1200s / 600s / 900s / 1800s (U1, 8 assertions). scheduleReverseAuditRound with diffPath === undefined: every transcript classifies unknown, nothing retires (U2); known diffPath but no baked read falls back to "opened the diff at all" (U3), and other-file-only reads stay hot on the diffToolCalls > 0 clause (U4); a read_file with no limit carries no range and opens no territory (U7).
Skipped chunk-rounds write no prompt records (C4: records = [3] only), so the coverage roster owes nothing for retired chunks — the check-coverage interplay claim holds by construction.
Corrections
None this round. Round 3's note on the drifted snapshot baseRefOid applies again in updated form (snapshot d2c7cb1, actual base 8b0e8b8192); the snapshot's commit count (4) matches the PR's real history but only the head is locally reachable.
Findings
1. T2 residual — findings prose quoting a read window aimed at the DIFF PATH ITSELF still injects the baked territory. Suggestion (bounded).
The new commit binds the territory scan to the diff path — closing other-file quotes (proven: T1 hot on head; deleting the clause flips T1 to retired, witness 05-m2-path-clause-counterfactual.png). But a folded findings entry quoting read_file(file_path="<the diff's own path>", offset=0, limit=50) still lands in bakedRanges, and any-overlap-passes then retires chunks whose territory was never opened. Reproduced end-to-end (cell T2): all three chunks exit 5 CONVERGED although every auditor only ever read diff lines 1-50 of territories 1001-1200 / 2001-2200 / 3001-3200.
Bounds: requires a finding that quotes a read_file call with the diff's exact absolute scratch path AND auditors whose only diff reads stay inside the quoted window AND two substantive receipts — and cold-check rotation still revisits retired chunks on even rounds. The failure direction is early retirement (the direction the module's header says it never fails), not fabricated evidence.
Measured candidate fix (applied in a scratch copy, rebuilt, driven through the same probes):
function bakedRanges(
prompt: string,
diffPath: string | undefined,
): Array<[number, number]> {
const out: Array<[number, number]> = [];
if (diffPath === undefined) return out;
+ // The record is the FOLDED launch prompt — the cumulative findings list
+ // rides above the builder's own text, which always opens with the brief
+ // line; a finding quoting a read aimed at the diff would otherwise inject
+ // its window into the territory. Scan the builder's section only.
+ const briefLine = prompt.lastIndexOf('**Your brief is a file.');
+ if (briefLine !== -1) prompt = prompt.slice(briefLine);
for (const m of prompt.matchAll(Results: hostile T2 fixture goes hot (code=0 blocks=3), benign fixtures byte-for-byte unaffected in behavior (T1 still hot, T3 still exit 5 — zero collateral). The suite is green both with and without the patch territory-wise (no fixture exercises this axis — see #2), so the fix should ship with a T2-shaped fixture. (First cut anchored on ## The diff and was inert — reverse-audit prompts carry no such heading; verified against a real recorded prompt before the second cut.)
2. M2 coverage gap — the path-binding clause is load-bearing but unpinned.
Deleting if (m[1] !== diffPath) continue; survives BOTH retirement.test.ts (37/37) and agent-prompt.test.ts (185/185), yet flips T1 from hot to retired in the compiled CLI (witness 05-m2-path-clause-counterfactual.png) — the clause decides real behavior no test asserts. Root cause: the PR's own fixtures cannot reach it — findings prose quoting a read window cannot widen the territory (both the unit and handler variants) uses a quote with no file_path at all (read_file(offset=0, limit=50) / bare prose used offset=0, limit=50), which the bakedRanges regex never matches with or without the clause. The pinning fixture is a decoy WITH file_path="src/pay.ts" (the T1 shape). The test names claim exactly this coverage; the inputs do not deliver it.
3. M5 coverage gap — overlap-vs-containment is unpinned.
Turning the overlap bar s <= te && ts <= e into containment s >= ts && e <= te survives retirement.test.ts green (37/37): every fixture read happens to be CONTAINED in the territory (the overlap test pages with offset=1100, limit=50 inside 1001-1200). Measured counterfactual in the compiled mutant: an overhanging read [1150,1249] over territory [1001,1200] retires on head (overlap is the bar) but stays hot on the containment mutant — a fixture with that overhang would pin the semantics the code comment promises ("an honest auditor pages an oversized chunk, and each page overlaps the territory even though no single read holds it all").
4. Dead clause — rec.successfulToolCalls > 0 in classifyReturn can decide no outcome.
diffToolCalls only increments inside the success branch that also increments successfulToolCalls, so diffToolCalls > 0 ⟹ successfulToolCalls > 0; deleting the clause survives green (37/37, M4 row) and no input can separate them. Introduced by this PR's new file — either remove it or leave it as documented belt-and-braces; either way it is not a pin-able guard. This refines, not contradicts, round 3's M4 row (which was about the sibling diffToolCalls > 0 clause — that one is live and pinned, M3 row).
5. Carried M10 — the bare-subject dedupe branch in compose-review.ts remains unpinned (161/161 green after deletion; liveness proven by the M7 splice control). Outside this PR's diff; completeness reporting, not a merge condition. It is the axis C14's collision rode, so a fixture remains worth adding when convenient.
Considered and dismissed: classifier scaling (receipts are model returns read from harness transcripts and the CLI's own records — not outsider-authored text; the regexes are linear: bounded lazy fillers, one greedy tail); the workflow-reserve sync claim in the new deadline.ts comment (verified true: qwen-code-pr-review.yml caps the scaled reserve at 3600 = DEFAULT_RESERVE_SECONDS); the budget-stop marker's missing expectedRoundSeconds field (the refusal arithmetic was pinned instead via the compiled expectedRoundSeconds probes, U1).
Not covered
- Change 2 (verification pipelining) has no behavioural oracle — it is orchestrator prose in SKILL.md. Checked textually only: the load-bearing orderings survive the rewrite (the last round's verification completes before Step 6; entries still tagged
— [unverified]are structurally excluded from Step 6's confirmed-only read; the Step 5 merge runs unconditionally before every round build and before Step 6; a verifier-never-ruled entry caps a would-be Approve at COMMENT viaunreviewedDimensions; CONVERGED exit 5 is a clean termination owing no entry; the gate prose now says "costliest round's measured cost … not the newest dip", matching the measured pricing). The projected 95→55-minute saving is the author's arithmetic, not measured here. The retirement prose in SKILL.md matches the measured cells (odd rounds shrink, even rounds cold-check all retired chunks together, the note rides after the end-of-round line). - Gates: only the affected workspace suite ran —
src/commands/reviewat head: 1890 passed | 4 skipped (1894, 56 files), exit 0. The 4 skips are pre-existing environment gates (1skipIf(!caseInsensitiveFs)insave-artifact.test.ts, 3describe.skipIf(!hasShellcheck)inscript-lint.test.ts). Repo-wide lint/format/typecheck not re-run. - Per-commit attribution out of reach: the snapshot lists 4 commits; the depth-2 graft reaches only the head (
git rev-list HEAD^1..HEAD^2= 1, shallow). The aggregateHEAD^1..HEADdiff was verified; the three earlier commits (0cf3170,c11df3b,80d4d93) were not individually exercised — round 3's measurements of80d4d93were carried forward only through re-measurement at the new head, never by trust. - Base-arm build noise is environmental, not an arm difference: the base worktree's from-scratch
tsc --buildreports the same 223-line type-error class as round 3 (mime/lite,fdir,ignore,ajvsubpaths,@testing-library/reactagainst the shared node_modules types); the emitted JS is complete and behaviorally pre-PR in exactly the expected directions (help text carries exit 0/4 only; all 47 base control cells reproduce pre-PR behavior;retirement.jsabsent from base dist,grep -c scheduleReverseAuditRound|CONVERGED0 vs 5). The mut-tree build shows the same noise class. - Synthetic transcripts, not model behaviour. The A/B proves the scheduler's handling of every receipt shape; it does not reproduce real auditor degradation (shape, not cause).
- C14 measured on the EN channel only (plan
prDescriptionHasHan: false; the zh fold needs a live PR-body fetch this sandbox cannot make). The structural entries both languages render from are the same objects the EN assertions covered. - Budget-boundary exactness: P4 cells avoid the
remaining == reserve + estimateknife-edge (±1 s of clock drift between harness and CLI would flip it); both sides of the boundary are covered with slack instead. - No injection attempts observed in the PR title/body/commit messages; they were treated as untrusted claims and tested, not followed.
Methodology
Environment: node:22-bookworm container, npm ci + npm run build pre-run at the merge commit. Base control: git worktree add tmp/base-tree HEAD^1, packages/cli rebuilt there via scripts/build_package.js (tsc project references emit packages/core too); internal workspace links re-pointed — tmp/base-tree/node_modules/@qwen-code/qwen-code-core → ../../packages/core (base tree), all other @qwen-code/* → head-tree packages (their sources are untouched by the PR — git diff HEAD^1..HEAD --name-only shows TS only under packages/cli plus SKILL.md), per-package third-party node_modules symlinked (lockfile unchanged, so byte-identical). Control cleanliness asserted: import.meta.resolve('@qwen-code/qwen-code-core', …/base-tree/packages/cli/dist/index.js) → file:///__w/qwen-code/qwen-code/tmp/base-tree/packages/core/dist/index.js (realpath inside the base tree); base CLI help text lacks the CONVERGED exit; base dist contains no retirement module. Every A/B / C14 / probe cell spawns the real compiled CLI as a child process with per-cell state dirs (QWEN_CODE_PROJECT_DIR/QWEN_CODE_SESSION_ID); transcripts are JSONL in the harness's format with launch prompts copied byte-for-byte from the CLI's recorded blocks so deliveredVerbatim pairing is honest. Unit probes import the compiled dist modules directly. Mutations ran in tmp/mut-tree at the merge commit (exact single-occurrence string replacements, occurrence count enforced, restored via git checkout and git status --porcelain verified empty after each run); dist counterfactuals (M2, M5, T2 fix) rebuilt incrementally with tsc --build. Counts: 114 (A/B: 67 head + 47 base) + 15 (unit probes incl. U7) + 22 (C14 both arms) + 16 (mutation-matrix expectations incl. 3 baseline greens and the POS/M7 positive controls) + 4 (M2 flip probes) + 2 (M5 overhang) + 3 (T2 candidate-fix probes; the residual itself is measured by the T2 cell inside the A/B count) + 1 (suite gate) = 177, of which 0 failed; the A/B arms and the matrix were each re-run once under scripts/verify-capture.mjs with identical counts (witnesses 01/02/04). Raw per-cell logs in logs/ (ab-*-results.json, ab-*-cap.json, unit-*.json, c14-*.json, mutations.json + per-mutant mut-*.log, m2-cap-*.json, t2-fixed*.json, suite-head.log, base-build.log, mut-tree-dist-build.log), harnesses in harness/ (common.mjs, ab.mjs, unit-probes.mjs, c14.mjs, mutations.mjs, t1-probe.mjs, t2-probe.mjs, overhang-probe.mjs, m2-capture.sh), evidence images in evidence/.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Confirmed — and it's already in at the current head: Pinned by test: — via Claude Code (claude-fable-5) |
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #8498 (round for @wenshao's review)All five sections of the review were addressed; nothing declined, nothing escalated. Commit: §1 — The terminal round's verification loses its structural guaranteeAddressed, both halves. Machine check (the primary ask): SKILL wording: the 5-round-cap bullet now says explicitly that round 5's verifiers have NOT launched (that launch rides the next round's build, which the cap forbids) and must be fired alone before Step 6; the two-dry-rounds bullet says the last reporting round's verifiers are already in flight and their verdicts must land in the final merge before Step 6; the CONVERGED bullet carries the parenthetical that it cannot owe a verification launch (a reporting round makes its chunk hot); the tag backstop paragraph and the pipelined bullet now name the §2 —
|
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved. This is the key review-perf change: per-chunk retirement turns full re-audit into evidence-based retirement + cold-check backstop, and pipelined verification shortens the critical path, without buying missed findings (fail-open throughout). Non-blocking follow-ups noted separately: deadline under-estimation after a shrunk round (price from max observed spans) and the prose-enforced [unverified] exclusion.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-reviewed at 802a9c6bdd. My previous approval was on 49e4f1e6 and got dismissed when this commit landed, so here is a fresh one. Both of the non-blocking notes I left last round are now addressed, and I checked the fixes rather than the commit title.
The tail reserve is repriced in lockstep across all three places it lives. DEFAULT_RESERVE_SECONDS goes 3600 → 4800, the workflow's divisor goes attempt_timeout / 4 → / 3 with its cap going 3600 → 4800, and the cross-file assertion in deadline.test.ts was updated to match the new expression. That last part is what I most wanted to see: this repo has a test that reads the workflow and pins the two sides together, so a one-sided edit would have gone red. It did not, because both sides moved. The rationale in the new comment is also honest about its own basis — the only tail ever measured was past 30 minutes and still running when the kill arrived, so the third is insurance until pipelined runs produce real tail measurements.
The — [unverified] backstop is now genuinely machine-enforced, and it fails closed. UNVERIFIED_FINDING_TAG_RE is counted against the findings file, a surviving tag pushes findings-unverified-at-compose into cappedBy, and if (event === 'APPROVE' && cappedBy.length > 0) event = 'COMMENT' gives the cap teeth. The part I specifically went looking for: an unreadable findings file sets findingsUnverifiedAtCompose = true rather than leaving it false, so a missing or unreadable file cannot be mistaken for "no tags." A malformed findingsPath throws a TypeError instead of being coerced. Both directions are right.
I also confirmed the bakedRanges diff-path binding from the previous round is untouched by this commit, so Warning 1 stays fixed.
CI is green on this exact SHA: run 30979862814 has head_sha=802a9c6b.
Approving. Two Suggestion-level observations, neither blocking:
The backstop's invocation is still prose-driven, even though the check is now machine-driven. The whole block is gated on if (findingsPath !== undefined && findingsPath !== null). Omitting it is correct at medium and low, which run no Step 5 and have no findings file — SKILL.md says so explicitly. But at high effort nothing enforces that it was passed: if the orchestrator forgets, the check silently does not run and the guarantee reverts to prose, which is exactly what this commit set out to replace. Worth noting that verification is already in scope a few dozen lines above this block, so "reverse audit rounds ran but no findingsPath was supplied" is a state compose-review can already detect and refuse. Closing that would make the backstop unbypassable rather than opt-in.
A loosening of DRY_RECEIPT_RE rode along in this commit. The phrase class gained () and a run of [*_)\]"”’] is now allowed before the separator. That makes a dry receipt easier to match, which is the fail-open direction in the retirement path — a false match skips audits, whereas a missed legitimate receipt only costs a re-audit. It is bounded, since substantiveClause still gates the clause and diffToolCalls > 0 plus openedTheTerritory remain independent conjuncts, and accepting **no new issues** — … is clearly the intent. Flagging it only because a change that widens the retire path arrived in a commit titled for the reserve and the tag check, where a reader would not look for it.
|
Released in v0.21.6. |
Summary
Stacked on #8468 (the reverse-audit budget gate) — merge that first; this PR's diff then shrinks to its own two commits.
Two independent measurements say the reverse-audit loop is where large-PR reviews spend most of their time: a +1699-line CI review ran the loop to the 5-round cap and was killed at 240 minutes mid-verification (#8368), and a +974-line local benchmark completed in 144 minutes with 95 of them (66%) in the loop — 5 rounds × 6 chunks. The round-by-chunk yield grid of that benchmark:
Chunks 3 and 6 were dry all five rounds — a third of the loop's auditors re-certifying converged territory — while the findings all came from the hot chunks. And every round waited serially for its verification before the next began.
The design intent — converge within one run, so misses do not leak into another full review cycle — is preserved by both changes; what they remove is redundancy, not depth.
Change 1: per-chunk retirement with cold rotation (CLI-enforced)
From round 3,
agent-prompt --all-chunksreads the harness transcripts (the same evidencecheck-coveragetrusts) and retires a chunk whose own last two audits were substantively dry — the receipt named what it examined AND the transcript shows the diff was opened; a whiff or a missing transcript keeps a chunk hot (fail toward auditing). A retired chunk is cold-checked on alternating rounds and returns to every-round auditing the moment a cold check yields. Rounds 1-2 always audit every chunk. Hot chunks get no less auditing than today; a late find in a retired chunk arrives one round later instead of never. (One bounded residual: a chunk retired late enough that its next cold check falls past the 5-round cap — e.g. retired after round 4 — is last audited at its retirement round; that tail is capped by the same hard limit that already ends the loop, and the chunk still holds the two-dry certificate the loop used to end on.) When every chunk holds the two-dry certificate and none is due, the builder printsCONVERGEDand exits 5 — a clean convergence: nounreviewedDimensionsentry, the per-chunk certificates being collectively the same evidence the global two-dry-rounds rule demanded. The budget gate (exit 4) runs first and is unchanged; single--chunkrebuilds bypass scheduling entirely; any classification error falls back to the full fan-out.Change 2: verification pipelines with the next round (SKILL)
Round k's verifiers and round k+1's auditors now launch in one response. The cumulative list exists so auditors do not re-report what is filed — an unverified entry serves that purpose exactly as well as a confirmed one, so the 5-8 minutes per round spent waiting for verdicts bought nothing. The orderings that matter stay: the last round's verification completes before Step 6, nothing unverified reaches the report or the PR, and an entry the verifier rejects leaves the list at the next merge.
Projected on the benchmark: the 95-minute loop drops to roughly 55 minutes (retirement −20% of auditors on that grid; pipelining −5-8 min/round), with detection semantics intact.
Test plan
9 new scheduling tests (rounds 1-2 full; round-3 skip with named note; cold-check parity; yield-returns-to-hot; whiff stays hot; all-retired → exit 5 with no records and no stamp; cold-check-only round still stamps;
--chunkbypass; transcripts-unavailable fallback; budget-gate-before-retirement pin). Fullsrc/commands/review: 1710 passed, 0 failed; eslint/prettier clean. Coverage interplay verified: skipped chunk-rounds write no records, and the Step 4/5 delivery floor is satisfied by rounds 1-2's records.中文说明
概述
叠在 #8468 之上——请先合并那个,本 PR 的 diff 随之缩为自己的两个提交。
两次独立测量都表明反审循环是大 PR 评审的最大时间去向:+1699 行的 CI 评审跑满 5 轮上限、在第 240 分钟被杀于验证中途(#8368);+974 行的本地基准 144 分钟完整跑完,其中 95 分钟(66%)在循环里——5 轮 × 6 chunk。逐轮×逐 chunk 产出网格见上(S=1 条 Suggestion,·=干净):chunk 3、6 五轮全干净——三分之一的 auditor 在反复认证早已收敛的领地;所有发现都来自热区。且每轮都串行等完验证才开下一轮。
设计初衷——在单次运行内收敛、避免漏检泄漏到下一整轮评审——两处改动都完整保留;砍掉的是冗余,不是深度。
变更 1:按 chunk 退休 + 冷轮换(CLI 强制)
第 3 轮起,
agent-prompt --all-chunks读取 harness transcripts(与check-coverage同源的证据),退休自身最近两次审计均"实质干净"的 chunk——回执点名检查对象且 transcript 显示确实打开过 diff;whiff 或缺 transcript 一律保持热区(失败方向=多审计)。退休 chunk 隔轮冷抽查,一旦出货立即回到每轮必审。第 1、2 轮永远全量。热区得到的审计不少于现状;退休区的晚期发现最多晚一轮到达,不会丢失。(一个有界残余:退休得足够晚、下次冷抽查落在 5 轮上限之外的 chunk——例如第 4 轮后退休——其最后一次审计即退休轮;这条尾巴被本就终结循环的硬上限所限,且该 chunk 仍持有循环过去赖以终止的"连续两轮干净"证书。)全部 chunk 持有"连续两轮干净"证书且本轮无人到期时,builder 打印CONVERGED并以 5 退出——干净收敛:不欠unreviewedDimensions条目,按 chunk 的证书合起来正是全局两干净轮规则所要求的同一份证据。预算闸(exit 4)先行且不变;单--chunk重建完全绕过调度;分类异常回退全量扇出。变更 2:验证与下一轮流水线化(SKILL)
第 k 轮的 verifier 与第 k+1 轮的 auditor 在同一个响应中发射。累计清单的作用是让 auditor 不重复报告——未验证条目与已确认条目在这个用途上等效,过去每轮 5~8 分钟的串行等待买不来任何东西。有意义的顺序保留:末轮验证在 Step 6 前完成,未验证内容绝不进报告或 PR,被 verifier 否决的条目在下次合并时移出清单。
按基准折算:95 分钟循环预计降至约 55 分钟(退休 −20% auditor;流水线每轮 −5~8 分钟),检测语义不变。
测试计划
9 个新调度用例(1-2 轮全量;第 3 轮跳过并点名;冷抽查奇偶;出货回热;whiff 保持热;全退休 → exit 5 且零记录零 stamp;纯冷查轮照常 stamp;
--chunk绕过;transcripts 不可用回退;预算闸先于退休)。全src/commands/review:1710 通过 0 失败;eslint/prettier 干净。覆盖率交互已核验:被跳过的 chunk-轮不写记录,Step 4/5 投递下限由 1-2 轮的记录满足。