perf(review): bake a soft tool-call budget into finder and auditor briefs - #8708
Conversation
…iefs A fan-out wave's wall clock is its slowest agent, and the slowest agent is reliably a wanderer: two measured runs of the same 14-agent wave took 11.7 and 41 minutes on comparable diffs, the difference being individual agents spending 40-100 model calls exploring the tree while healthy agents settle at 25-45 with indistinguishable findings. plan.budget gains agentToolBudget - clamp(30 + effective/20, 30, 60), computed and recorded like every other budget arm so no caller can inflate it - and agent-prompt bakes it into every finder, chunk, invariant, persona and reverse-audit brief. The verifier is exempt (verifyShard governs its load, and its per-finding re-trace must not stop early), as is Build & Test (deterministic commands). The ceiling is soft and worded against the two failure modes a budget invites: at the budget an agent stops EXPLORING, never reporting - findings in hand are filed, open checks are disclosed as "did not get to" into the same receipt machinery that judges whiffs - and the recall rule is restated beside it so it cannot read as a reporting cap. A plan without the field (older CLI) gets no ceiling: the fallback errs toward coverage, matching the documented pre-budget behaviour.
|
Thanks for the PR! Template looks good ✓ Problem: observed and measured, not theoretical. Two CI runs of the same 14-agent review wave on comparable diffs took 11.7 vs 41 minutes, with the tail traced to individual agents spending 40–100 model calls exploring the tree while healthy agents settle at 25–45 with indistinguishable findings. That's a concrete, reproduced latency pathology with numbers, and per-agent efficiency was already named as the next target in the sibling concurrency work (8631). Direction: aligned. This continues the /review latency line (follow-up to 8642, 8648) and targets exactly the item the prior work scoped out. It's internal review-tooling quality — squarely in the project's interest, and touches no auth/sandbox/model-selection/telemetry surface. Size: 156 changed lines across 6 files — 74 production logic lines ( Approach: the scope feels right and minimal. One soft ceiling computed like every other budget arm ( Risk: no elevated risk signals — none of the changed files match the repo's revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测且有实测数据，不是理论性问题。同一 14-agent review 波在可比 diff 上两次 CI 运行分别耗时 11.7 与 41 分钟，尾部延迟可追溯到个别 agent 花 40–100 次模型调用在代码树里游走，而健康 agent 25–45 次即得到无差别的发现。这是有数字支撑的、可复现的延迟问题；且单 agent 效率本就是姊妹并发工作（8631）点名的下一个目标。 方向：对齐。这是 /review 延迟工作线（8642、8648）的后续，精确针对上次工作范围外清单中的目标。属于内部 review 工具质量改进，完全符合项目利益，不涉及 auth/sandbox/模型选择/遥测等敏感面。 规模：6 个文件共 156 行变更——生产逻辑 74 行（ 方案：范围合理且最小化。一个与其他 budget 分量同样计算的软上限（ 风险：无升级风险信号——变更文件均未命中本仓库与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewOne Critical blocker: the PR does not compile. Everything else I checked out, and it checks out properly:
So the design and tests are sound; this is a one-line type omission away from a clean review, but it has to compile first. Files changed (6)
Testing evidence — this PR's own CICI is red, and the failure is caused by this PR, not infra noise: the build dies on the TS2339 above. From the failing job's log: The other two red checks are downstream of the same root cause: Post Coverage Comment fails with "Artifact not found for name: coverage-reports-22.x-ubuntu-latest" because the test job died before uploading, and the web-shell smoke hits the identical build error. The macOS/Windows test matrix and the CLI integration tests were skipped by fail-fast — they never ran, so nothing is known about them yet. The author's A/B numbers in the PR body (51.2→46.1 min wall clock, 5 agents citing the budget) remain the author's claim, not independently re-run here — unattended triage does not execute PR code.
Sandboxed verification would settle the rest once it compiles: 中文说明代码审查一个 Critical 阻塞项：PR 无法编译。 其余部分都核对过，且核对无误：
设计与测试都是健全的；只差一行类型声明就是一次干净的审查，但首先得能编译。 测试证据——本 PR 自己的 CICI 为红，且失败由本 PR 造成，不是基础设施噪音：构建死于上述 TS2339。其余两个红色检查是同根因的下游：Post Coverage Comment 因测试 job 未及上传而报"Artifact not found"；web-shell smoke 撞上同一构建错误。macOS/Windows 测试矩阵与 CLI 集成测试因 fail-fast 被跳过——从未运行，尚无任何结论。作者在 PR 正文中的 A/B 数字（墙钟 51.2→46.1 分钟、5 个 agent 引用预算）仍是作者的主张，未在此独立复跑——无人值守 triage 不执行 PR 代码。 待编译通过后可用沙箱验证收尾： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — the design is exactly right, but the PR doesn't build; one missing type declaration blocks everything. Stepping back: this is genuinely frustrating to request changes on, because the thinking here is better than what the gate usually sees. The problem is real and measured (11.7 vs 41 minutes on the same wave shape), the mechanism is the right one — a soft ceiling computed like every other budget arm, injected at the single choke point, exempt where the load is governed elsewhere, falling back toward coverage on old plans — and the wording split between "stop exploring" and "stop reporting" is the subtlety this exact change lives or dies on. My independent take on how to solve the wanderer problem landed on essentially this design, and the tests pin the right things: the clamps, the injection matrix, the exemptions, the fallback, even the recall-rule restatement. But the contract is that it compiles, and it doesn't. So: one interface field to add (with a shape, not 中文说明置信度：2/5 —— 设计完全正确，但 PR 无法构建；一处缺失的类型声明阻塞了一切。 退一步看：这次 request changes 令人遗憾，因为其中的思考高于门禁平常见到的水准。问题是真实且实测的（同一波形 11.7 对 41 分钟），机制也是对的——与其他 budget 分量同样计算的软上限、在唯一咽喉点注入、负载另有治理处豁免、旧 plan 朝覆盖回退——而"停止探索"与"停止上报"的措辞区分正是这个改动成败所系的微妙之处。我对游走问题的独立方案与此设计基本一致；测试钉住了正确的东西：clamp、注入矩阵、豁免、回退，甚至召回规则的重申。 但契约是能编译，而它不能。 所以：补一个接口字段（要有形状，不能是 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
The design is sound, but the PR doesn't compile: toolBudgetBlock reads report.budget?.agentToolBudget while the local PlanReport interface in agent-prompt.ts never declares budget — tsc --build fails with TS2339 and the ubuntu test job is red on it. Add the field (shaped, e.g. budget?: { agentToolBudget?: unknown }, so the chained access type-checks) and this should go green. Details in my notes above. 🙏
The CI build's tsc caught what vitest's transpile-only path did not: the prompt builder reads plan fields through its own unknown-typed local PlanReport interface, which had no budget member. Declared in the local house style (unknown-typed) with explicit numeric narrowing in toolBudgetBlock.
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. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
中文说明
— gpt-5.6-sol via Qwen Code /review (v0.21.7)
…re loop Round-1 review rework. The ceiling is now per LAUNCH, not per plan: a chunk or invariant agent's allowance derives from its own territory (launchToolBudget, same constants as reviewBudget), and every launch's mandatory reads ride on top of the allowance instead of inside it, so a whole-diff role on a huge diff is never exhausted by its assigned chunk reads. Agent 8's whole-diff block now carries the budget too, and Agent 0 joins the exemptions — its mandatory work is issue-sized, not diff-sized. The disclosure side becomes deterministic: briefs mandate the fixed `Budget gap: <the check>` line format, coverageFromTranscripts parses those lines into the report's budgetGaps (a NOTE, never a gate failure — punishing disclosure teaches agents not to disclose), and SKILL.md's Step 3D says how each gap is ruled: an incomplete required trace joins unreviewedDimensions, optional depth goes to "Not reviewed". Tests: launchToolBudget scaling/rides-on-top/garbled inputs, the runtime shape of reviewBudget's return, per-launch numbers for chunk, invariant, whole-diff and Agent 8 briefs, a full-roster exemption sweep, garbled agentToolBudget values falling back to no ceiling, and budgetGaps parsing with ok staying true.
…nto perf/review-agent-tool-budget
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
/code-review max — 15 findings
Reviewed at e0008ea5932ec9aecd6b826e43d5f10930eb52d7. Method: 10 finder angles + 2 verifier passes + 1 gap sweep. Every claim below was proven by running the PR's real builders and coverageFromTranscripts under vitest in a worktree, not by reading alone — the numbers quoted are measured output. The PR's own tests pass (299 in packages/cli, 4 in packages/core), so none of these are caught by CI.
Roughly by weight:
- A
Budget gap:line makes a reverse-auditor's return retire its chunk (retirement.tsnever learned about the new format). - The
budgetGapsparse has none of the whiff guards its siblingUncoverable:parse documents as load-bearing. - The budget block is pushed into uncoverable chunk briefs, turning a disclosed gap into
exitCode 3. BUDGET_GAP_REdrops every markdown-rendered disclosure (- Budget gap:→ 0 matches) and acceptsBudget gap: none.- Chunk coverage is still credited from the launch prompt, so
Budget gap: chunks 2-6still yieldsok: true. buildWholeDiffBlocktakes no role — it budgets the three exempt roles and double-budgets invariant agents.- / 8. / 10.
agentToolBudgetis unclamped on one arm and entirely ignored on the other;chunk.lines/chunk.charsrenderNaN. budgetGapsdead-ends at a stderr NOTE — unlike the TIME budget, nothing mechanically caps the verdict.- – 15. Brief-text accuracy (dangling recall-rule citation, "~N assigned reads" false for chunk agents), the invariant agent's flat
4, unsanitised gap text in an instruction-bearing NOTE, and an exemption-roster test that does not close the roster.
Two comments are anchored to the nearest diff line rather than the code they describe, because their real anchors are outside this diff — each says so on its first line (lib/retirement.ts:322, lib/coverage.ts:598).
One candidate was refuted and dropped: that the ceiling is too loose to bind (launchToolBudget(25000, 63) = 123). It fails because DESIGN.md's 41-minute wave was a Step 3A wave, which SKILL.md gates at srcDiffLines <= 500, where the ceiling is 36-63 and does bind.
…sures teeth Round-2 review rework, three threads of it. The number: launchToolBudget now takes the plan's recorded value and clamps it into the budget's own band in both directions, and a scoped launch's territory-derived allowance can only lower it, never raise it — the plan is again the one number every launch answers to, under version skew included. Reads estimates count the launch's whole reading list (brief, diff pages, the reverse auditor's findings file, an invariant agent's file paging scaled by its added lines), garbled chunk entries degrade to the scoped floor instead of NaN or whole-diff headroom, an UNCOVERABLE chunk gets no budget block beside its exact-receipt instruction, and the exemptions are declared on the briefs (budgetExempt) rather than hardcoded names — with the roster test walking BRIEFS so a new role must declare. The disclosure: one parser (budgetGapDisclosures in lib/budget.ts) shared by every reader — markdown-tolerant, placeholder-dropping, control-char-stripping, length- and count-capped. coverage collects gaps inside the guarded record walk (idle/blind/superseded agents' copied templates no longer count), narrows a disclosing agent's chunk credit to the lines the harness saw it read (told-list credit was making budget stops invisible to the gate), and retirement refuses to count a gap-bearing return as a dry audit — an agent's admission that it stopped can no longer retire the chunk that still owes the work. compose-review renders every parsed gap into the body's "Not reviewed" section mechanically — on the Approve too — as a disclosure channel that never caps; the capping ruling stays with the orchestrator per Step 3D. Also: check-coverage's missing-chunks NOTE no longer tells operators to paste --whole-diff ahead of role briefs (that double-budgets; the block is Agent 8's alone), and its budget-gap NOTE puts the directives before the agent-authored text.
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/31243036089)._ |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
/review at max effort — 15 findings
Reviewed head 8b9e0f7d2b (14 files, +995/−13) across ten finder angles plus a gap sweep. All 536 tests in the touched suites pass, so every finding below is a gap in the test suite, not a failing test. Four of the top findings carry runnable end-to-end reproductions against this branch's own code.
The load-bearing result: the disclosure loop punishes disclosure
SKILL.md:416 promises the budget-gap channel "never fails the gate". It does. An agent that walks its whole territory and writes the fully compliant line Budget gap: none. produces:
budgetGaps: [{ agent: '…', gaps: ['none.'] }]
covered: [1] missing: [2] ok: false
Three defects compose to get there:
PLACEHOLDER_GAP_REis whole-string anchored —none.,None (all checks completed),N/A - stayed under budgetall survive the filter as real gaps.- The credit narrowing in
coverage.ts:624can zero an agent's coverage —rangeOfonly records reads that carry a positive integerlimit, so anoffset-only page (the shape the brief instructs) leavesdiffReadsempty. Removing only therange: [0, 100]this PR added to its own compose-review test flips that test from APPROVE to COMMENT withcappedBy: ['chunk-nobody-read']. retirement.ts:334then keeps the chunk from ever retiring, soconvergedcan never fire and the loop runs to the 5-round cap — inverting this PR's own wall-clock goal.
The parser has two further shape bugs (a Budget gap: header line swallows the next line and turns denials into gaps; quoted and fenced text parses as authored — which matters because this repo dogfoods /review on its own PRs), and the disclosure text reaches the posted GitHub body unescaped and uncapped, where @handle and #1234 have real external side effects.
What is clearly right
The design intent is sound and the exemption mechanism is genuinely well-built: budgetExempt declared on the brief with the roster test walking Object.keys(BRIEFS) is the right shape, and I verified it cannot drift. Also verified clean, so as not to be re-litigated: --roster and single --role builds are byte-identical (both funnel through buildLaunch); BUDGET_GAP_RE's module-level /g lastIndex does not leak across calls (matchAll clones; 8/8 on repeat calls after an early break); the brief's own Budget gap: <the check> template does not self-mint (it sits mid-line); finalText is the last assistant text, not a concatenation; budgetExempt's key-order change is inert; and report.budget as a string/array/null/number degrades to no-ceiling without throwing.
Prettier, eslint, the SKILL.test.ts pointer↔heading gate, and AGENTS.md's TypeScript rules all pass; the eslint-disable-next-line no-control-regex is load-bearing and unrestricted.
Not inlined (below the 15-finding cap)
lib/budget.ts:230— the "Markdown-tolerant" class omits#and the plural:## Budget gap: X→[],Budget gaps: A; B→[]. A lost disclosure is unobservable, and under this PR it also silently leaves the agent credited for a reading list it just said it did not walk.check-coverage.ts:249— one stderr run now carries three mutually exclusive directives about the same chunks (--rosterrebuild NOTE,ERROR: … not reviewed,Do not relaunch over these) withexitCode = 3; the NOTE also has no cap on agent count (43 agents ≈ 55 KB on one line).agent-prompt.ts:283— the 30-line design comment is orphaned: TypeScript attaches both adjacent blocks toFINDINGS_LIST_READS, andtoolBudgetBlockresolves to zero JSDoc.agent-prompt.ts:2011/:2170—--whole-diff's arg-error text still calls it "the diff-reading block alone" after this PR made it emit a budget section.lib/coverage.ts:610—superseded()now runs on the success path; for a whole-diff record,keySatisfiedis O(builtKeys × records) full-prompt normalizations.lib/budget.ts:261—MAX_GAPS_PER_AGENT'sbreakonly advances on accepted gaps, so placeholder lines never bound the scan.lib/budget.ts:198—sane(planBudget) || MIN_AGENT_TOOL_BUDGETis dead given the surroundingclamp, as isMath.max(0, Math.floor(sane(x)))at line 214; andagent-prompt.ts:342's three-arm ternary is exactlylaunch.territoryLines ?? null.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — stopped before round 2 by the review time budget.
中文说明
未审查：反向审计——评审时间预算不足，未能开始第 2 轮。
— qwen3.8-max via Qwen Code /review (v0.21.7)
Local verification — real-CLI A/B against the merge baseVerified at Verdict: the mechanism does what the description says, and it is worth merging. Three defects, all in the disclosure text this PR newly posts to the PR author — none of them touches the budget arithmetic, none blocks the design. I would fix F1 and F2 before merge (both are one-line regex changes); F3 is a follow-up. Fixtures: #8708 itself (4 chunks), #8696 (21 chunks + one heavy file, so every launch class appears), #8714 re-chunked to 73 chunks, plus three local repos placed at the floor / mid-band / all-prose points of the band. 1. The ceiling appears, and only where it should
The band was checked on real captures, not by reading the formula: 16 src lines → 30 (floor), 408 → 50, 738 → 60 (cap), and an 805-line docs-only diff → 35, which is the coarse all-prose rate working ( 2. The per-launch arithmetic, against an independent oracle
I re-derived the documented formula in a separate script that imports none of this PR's code, then compared it against what the CLI actually wrote into all 29 briefs of #8696. All match, including the three cases the description makes specific claims about:
3. The disclosure loop, end to end
I also ran the full chain to a real APPROVE — nine rostered agents plus two dry reverse-audit rounds, the audit converging at exit 5 — and confirmed the gaps render into the Approve body without capping it. 4. What a disclosure now costs — merge-base vs this branch
This is the part I found most valuable, and it is a coverage-integrity fix rather than a latency one: on the merge base, an agent assigned four chunk pages that opened one is credited with all four. This is the strongest argument in the PR and the description undersells it. Findings
F1 — Suggestion — the trailing-emphasis strip unbalances the disclosure it posts
An agent naming a check almost always ends on a symbol in backticks, so this is the common case, not the edge one. It reaches the PR author: a real Approve body from … consumers of <code>budgetGapDisclosures. Not explored to full depth (tool
budget reached): the untested </code>MAX_GAPS_PER_AGENT` boundary. …Suggested fix: capture the opening emphasis run in F2 — Suggestion — the placeholder filter is whole-string anchored, so near-misses become gaps
A model writing "nothing to disclose" with a full stop costs the review an entire extra reverse-audit fan-out — the exact wall clock this PR exists to save, and a failure mode the ceiling makes more likely, since the brief now asks every agent to think about disclosure. Suggested fix: normalise before testing, e.g. F3 — Nice-to-have — the gap list is the one uncapped list in the posted body
Observation —
|
| check | result |
|---|---|
PR's own tests at 8b9e0f7 |
536 pass (5 files) |
| same tests, merge-base source overlaid | 32 fail — the tests are load-bearing, not tautological |
full review suite (packages/cli/src/commands/review) |
63 files, 2221 pass, 6 skipped |
packages/core SKILL.test.ts (pointer↔heading gate) |
4 pass |
tsc --noEmit -p packages/cli |
clean — the earlier triage blocker ("the PR does not compile") is resolved by 70ac80b |
eslint --max-warnings 0 on the changed dirs |
clean |
prettier --check on the 14 changed files |
clean |
| PR CI | green |
Not verified here
The latency claims. I have no way to reproduce a 14-agent real-model wave locally, so the 51.2 → 46.1 min and 350 → 341 call numbers stand on the PR's own single-sample A/B, which the description already flags as such. What I can say is that the compliance mechanism they depend on is deterministic and correct: the number reaching each brief is right for every launch class, the disclosure round-trips, and the two teeth (narrowed chunk credit, no dry audit on a gap) fire exactly when a gap is present and never otherwise.
中文版
本地真实环境验证 —— 与 merge base 的 A/B
在 8b9e0f7 上验证，对照 merge base 7942197，Linux，两臂各自完整构建，端到端驱动真实的 qwen review 子命令（fetch-pr → agent-prompt --roster → check-coverage → compose-review → agent-prompt --role reverse-audit）。整条链路除模型外无任何桩：launch prompt 是 CLI 自己落盘的记录，diff 是真实 PR 的 diff，harness transcript 按 lib/transcripts.ts 实际解析的 JSONL 形状构造。
结论：机制与描述一致，值得合入。 发现 3 个缺陷，全部落在本 PR 新引入、会发布给 PR 作者的披露文本上——都不涉及预算算术，都不动摇设计。建议合入前修 F1 与 F2（各是一行正则改动）；F3 可作为后续。
Fixture：#8708 自身（4 chunk）、#8696（21 chunk + 一个 heavy 文件，一次覆盖全部 launch 类别）、#8714 重切成 73 chunk，另加三个本地仓库分别落在区间下限 / 中段 / 全散文点。
1. 上限出现了，且只出现在该出现的地方
merge base 无 agentToolBudget，本分支记录了；除两个声明豁免外，每个 rostered brief 都带上了该块。从构建产物里遍历 BRIEFS，20 个角色中 budgetExempt: ["0","7","verify"]——与文档完全一致。
区间是用真实捕获验证的，不是读公式：16 源行 → 30（下限），408 → 50，738 → 60（上限），805 行纯文档 diff → 35，即粗粒度全散文费率在工作（effective = max(src, total/8) = 100）。
2. 每次 launch 的算术 vs 独立 oracle
我用一个不 import 本 PR 任何代码的脚本独立重算了文档中的公式，再与 CLI 实际写入 #8696 全部 29 份 brief 的数字对比，全部一致，包括描述中三条具体主张：
- 受限 launch 按自身领地推导且永不超过 plan（feat: support drag and drop img in web-shell #8696 的 chunk 21：84 行 → 额度 34，而非 plan 的 60）；
- 必读次数叠加在额度之上——73-chunk plan 上 whole-diff 角色得 134、Agent 8 得 133，若是平坦 60 则在分析开始前就被读取清单耗尽；
- plan 双向保持权威：
100000→ 60，5→ 30。乱值（"60"、0、-5、null、字段缺失）回退为无上限——即 plan 只能关掉预算，永远无法放大它。
3. 披露闭环，端到端
check-coverage 解析出 gap，打印指令在前、agent 文本在后的 NOTE，并且退出码 0、ok: true——披露不是门禁失败。markdown 列表形式与加粗标签形式都能解析；brief 自己的 <the check> 模板以及 none / N/A / - 被丢弃。守卫成立：零工具调用的 agent 与未拿到 diff 路径的 agent 即便把 gap 原样抄回，也分别被记为 idle/blind 且不获任何 credit。
我还把整条链跑到了真实的 APPROVE——9 个 rostered agent 加两轮全 dry 反审，round 3 exit 5 收敛——确认 gap 会渲染进 Approve body 且不会封顶。
4. 一次披露现在的代价 —— merge base vs 本分支
| merge base | 本分支 | |
|---|---|---|
| 返回中没有 gap 行 | covered [1,2,3,4] | covered [1,2,3,4] |
| 返回中有 gap 行 | covered [1,2,3,4] | covered [1]，missing [2,3,4] |
这是我认为最有价值的一段，而且它是覆盖完整性修复而非延迟优化：在 merge base 上，被指派 4 个 chunk 页却只打开了 1 个的 agent，会被记为读完了全部 4 个。这是本 PR 最强的论据，描述反而低估了它。
发现
F1 —— Suggestion —— 尾部强调符剥离破坏了它自己发布的披露文本
packages/cli/src/commands/review/lib/budget.ts:255 的 .replace(/[*_~`]+\s*$/, '') 本意是去掉整行加粗披露的收尾 **，但它无法区分这与值末尾行内代码/强调的收尾符：
"Budget gap: consumers of `budgetGapDisclosures`" -> ["consumers of `budgetGapDisclosures"]
"Budget gap: **the security path**" -> ["**the security path"]
agent 指名一项检查时几乎总以反引号包裹的符号收尾，所以这是常见路径而非边缘情况。它会到达 PR 作者：compose-review 产出的真实 Approve body 就是 … consumers of `budgetGapDisclosures.；而在 COMMENT/CHANGES 路径上所有条目合并进同一段落，GitHub 自己的渲染器（POST /markdown）会因此开启一个 code span，吞掉后面 60 个字符：
… consumers of <code>budgetGapDisclosures. Not explored to full depth (tool
budget reached): the untested </code>MAX_GAPS_PER_AGENT` boundary. …建议修法：在 BUDGET_GAP_RE 中捕获开头的强调符串，仅当尾部与之镜像时才剥离——没有开头强调符的值保留自己的 markdown。
F2 —— Suggestion —— 占位符过滤是全串锚定的，近似写法会变成真 gap
budget.ts:234 的 PLACEHOLDER_GAP_RE = /^(?:<[^>]*>|none|n\/a|nothing|-|—)$/i 会丢弃 Budget gap: none，但保留 None.、n/a.、None found、none (finished inside the ceiling)。两个后果，都用真实 CLI 实测：
-
它会被发布给作者。当某个 chunk agent 写下
Budget gap: None.时，compose-review的真实输出是：Reviewed. Not explored to full depth (tool budget reached): None.. Not reviewed: …
body 断言的内容与 agent 说的相反（句号还多了一个）。
-
retirement.ts中任何解析出的 gap 都会把返回判为unknown而非dry。用真实 round builder 跑 4 chunk、rounds 1+2 全 dry：
| 某个 auditor 的 round-2 返回 | round 3 |
|---|---|
Budget gap: none |
exit 5 —— 已收敛 |
Budget gap: None. |
exit 0 —— chunk 1 仍在审计中，3 个已退休 |
模型把"无可披露"写成带句号的形式，就要让这次 review 多付一整轮反审扇出——正是本 PR 要削减的那份墙钟；而且上限的引入让这种写法更可能出现，因为 brief 现在要求每个 agent 都去考虑披露。
建议修法：先归一化再判定，例如 /^(?:<[^>]*>|none|n\/a|na|nothing|no gaps?)(?:\s*\([^)]*\))?[.。!！]?$/i。保守很重要——用 none\b 做前缀匹配会把合法的 none of the three callers were traced 一起吞掉。
F3 —— Nice-to-have —— gap 列表是 body 中唯一不封顶的列表
compose-review.ts:1083-1090 对每个披露 agent 渲染一条且不封顶，而同一文件里其他 body 列表都封顶为 5（MAX_DIMENSIONS、MAX_NOTES）。在真实 73-chunk plan（75 个 agent）上实测：
| 每 agent gap 数 | 合成后 body |
|---|---|
| 1 | 15 030 字符 |
| 3 | 37 080 字符 |
| 8（每 agent 上限） | 92 205 字符 —— 超过 GitHub 的 65 536 限制 |
submit.ts 自己写着"422 是全有或全无，每个都会带走整次 review"，而 422 恢复路径处理的是行内评论锚点、不是 body 长度——所以极端情形会丢掉整次发布。触发它需要约 53 个 agent 都写满每 agent 上限，概率不高，但恰恰是预算所服务的大型 3B run 形态。渲染层封顶并附 and N more agents disclosed budget gaps 不会把软上限变硬：裁决封顶的位置仍在 PR 安排的地方。
观察 —— BUDGET_GAP_RE 没有中文变体
budget.ts:230 容忍全角冒号（：），说明作者已经考虑到 CJK 返回；但标签本身仅英文，而同一功能域的兄弟正则 DRY_RECEIPT_RE 恰恰带有 未发现问题 / 无新问题 等变体，正是因为 auditor 确实会用中文返回。写 预算缺口：… 的 auditor 等于披露进了黑洞。brief 强制了字面格式，因此这是一条观察而非发现。
门禁
| 检查 | 结果 |
|---|---|
PR 自身测试（8b9e0f7） |
536 通过（5 个文件） |
| 同一批测试 + merge-base 源码覆盖 | 32 失败 —— 测试是有承重的，不是同义反复 |
review 全量套件（packages/cli/src/commands/review） |
63 文件，2221 通过，6 跳过 |
packages/core SKILL.test.ts（指针↔标题门禁） |
4 通过 |
tsc --noEmit -p packages/cli |
干净 —— 早前 triage 提的"不能编译"阻塞已由 70ac80b 修复 |
改动目录 eslint --max-warnings 0 |
干净 |
14 个改动文件 prettier --check |
干净 |
| PR CI | 全绿 |
未验证的部分
延迟类主张。本地无法复现 14-agent 真实模型扇出，因此 51.2 → 46.1 分钟与 350 → 341 次调用仍只由 PR 自己的单样本 A/B 支撑——描述本身也已如此标注。我能确认的是它们所依赖的服从性机制是确定且正确的：每个 launch 类别拿到的数字都对，披露能完整往返，两颗"牙齿"（收窄 chunk credit、带 gap 不算 dry audit）恰好在存在 gap 时触发、其余情况从不触发。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.7)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
中文说明
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
…last radius Round-3 review rework — 27 findings, two of them reversals of round-2 decisions, owned as such. Reversal one: a disclosing agent's coverage credit is NOT narrowed. rangeOf records only reads carrying a positive limit, so the narrowing zeroed exactly the compliant offset-paged reader while an agent that stopped WITHOUT disclosing kept full credit — an asymmetry that only ever bites the discloser teaches agents not to disclose. The told presumption is the same for every agent; a gap changes the Step 3D ruling, never the arithmetic. This also restores the Uncoverable-claim evaluation order the narrowing had broken. Reversal two: a gap-bearing return is no longer blanket-unknown to the reverse-audit retirement. The receipt is judged with its Budget gap lines STRIPPED: a return whose only substance was its disclosures still never retires, but a receipt substantive without them does — the blanket rule made convergence impossible (a reverse auditor's ceiling is routinely met) and ran every budgeted loop to the round cap. The parser is now line-based: no cross-line prefix class (the multiline regex measured seconds on ordinary pathological returns), same-line capture only (a bare header no longer swallows the next line), fenced code and blockquotes are quotation not use, non-answers are dropped in any punctuation (`None.`), duplicates fold, the sanitizer covers C1 / U+2028/29 / bidi overrides, wrappers strip only in pairs, truncation cuts on code points. Gap supersession is gap-aware: only a GAP-FREE relaunch silences a record's disclosures. The body channel is bounded and inert: each gap rides through mdField (no @-mentions, #refs, links or stray </details> from agent prose), the sentence caps at five attributed items with an "and N more" tail, a gap the orchestrator promoted into unreviewedDimensions is dropped here so the body never says it twice, and a disclosed gap denies the "no blockers" certification. Reads estimates: invariant paging from fileLines (the plan DOES record post-change length; added-lines was a 6x undercount on volume-heavy files), the chunkless 3A reverse auditor counts its findings-list pages (keyed on acceptsFindings), and chunk territory is source-weighted like the plan allowance it mirrors — a lockfile chunk no longer out-earns a source chunk. Discriminating fixtures pin all three past their floors.
Four follow-on review findings. The whole-diff reading list now counts each chunk's PAGES, not a flat one per chunk — an oversized chunk's isTruncated paging was being paid out of the analysis allowance, and the fixture itself encoded the contradiction (40k chars = 2 reads in the chunk test, 1 in the whole-diff test). The disclosure matcher accepts the zh forms (预算缺口/不足/用尽, with either colon) — the receipt regex next door accepts zh receipts by design, so a zh-narrating auditor's budget stop was invisible to every consumer. The retirement clause is cut at an INLINE disclosure marker before its substance is judged — a one-line return put the disclosure after the receipt separator where the line-based strip cannot see it, and the clause capture absorbed the gap text as its own substance. And launchToolBudget caps the TOTAL at 200: the reads term comes from the same unchecked-cast plan as the allowance, and a garbled chars of 1e9 was rendering a forty-thousand-call brief around the clamp.
|
@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: 155 passed · 0 failed · 155 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：155 通过 · 0 失败 · 155 总计 Verification reportPR 8708 deep verification —
|
| Cell | Observable | head | base |
|---|---|---|---|
| Plan rate (600src/600tot, 100src, 3src, all-prose 4800, NaN) | reviewBudget().agentToolBudget |
60 / 35 / 30 / 60 / 30 | field absent |
| Chunk agent, 100 src lines | About **37 tool calls** in brief |
37 = min(60, 30+5) + 2 reads | no block |
| Chunk agent, lockfile chunk (source-weighting) | brief number | 32 = floor 30 + 2 (territory round(100·(100/8)/100)=13) | no block |
| Chunk agent, 500-line territory | brief number | 57 = (30+25) + 2 | no block |
| Agent 8 whole-diff block | brief number | 63 = 60 + 3 pages, no brief read | no block |
Role 1a whole-diff |
brief number | 64 = 60 + brief + 3 pages | no block |
Role 3b chunk-scoped |
brief number | 40 = 35 + (brief+page+3 findings) | no block |
Role invariant-a (fileLines 5000) |
brief number | 47 = 35 + max(4, 2+ceil(5000/500)) | no block |
| Exemption sweep over all 20 roster roles | absence set | exactly {0, 7, verify} |
n/a (no blocks at all) |
Garbled plan value (undefined, null, 0, −5, NaN, "sixty") |
block presence | no block (coverage-side fallback) | no block |
| Plan value 0.5 / 1e9 | clamp both directions | floor 30 / ceiling 60 | no block |
Garbled chunk.chars = 1e9 |
total cap | 200 (else 40,000+) | no block |
Unreachable chunk (maxLineChars > cap) |
block absence | no block beside the Uncoverable: order |
no block |
| Recall rule + fixed format restated beside ceiling | brief text | present | n/a |
End-to-end CLI (review agent-prompt --chunk 1, real entry point, written brief file) |
About **37 tool calls** |
1 block | 0 blocks |
budgetGaps on identical transcript fixture |
field | populated (see below) | absent |
25/25 head cells, 27/27 base cells — the flip is total: every one of 20 launches gains exactly one budget block at head, and none at base. The two early harness "failures" during development were my own oracle errors (Agent 8's reading list excludes the brief by design; role 0 requires PR context to build at all), corrected and re-run.
Secondary claim 1 — the parser (47/47, evidence 03-h3-parser-ladder-linear.png)
h3-parser.mjs drove the compiled parser through 41 format/sanitation fixtures plus a 5-shape × 4-rung ladder (2k/3k/5k/20k chars), each timed:
- Formats pinned green: plain, bullets
-/*/+, numbered1./12), indented, bold/italic/code wrappers, zh预算缺口/不足/用尽with both colon widths, CRLF, whole-line code span (closed unwraps, open drops), fence (```and~~~) and blockquote quotation, bare-header-no-swallow, 7 placeholder shapes, case-insensitive dedup. - Sanitization pinned green: C0 (
\u001b[31m→ ESC stripped), C1 (\u009b), DEL, U+2028 (gap still delivered — no silent loss), bidi U+202E; truncation at 160 code points +…including an astral (𩸽) boundary; 8-per-agent cap. - Ladder (input an outsider can influence — an agent's return quotes PR-authored text): worst single timing 0.58 ms (a 3 k-char rung); every 20 k-char rung ≤ 0.33 ms; 20k/2k ratios 3.4–8.3×, i.e. linear. The claim "linear on pathological inputs" holds; the historical multiline-regex quadratic class is closed by construction (line-anchored matching).
- Base arm: absence-only control, 3/3 (no parser exports).
Secondary claim 2 — the disclosure loop (24/24 head, 18/18 base, evidence 02-h4-retire-compose-flip.png)
h2-ab-disclosure.mjs (coverage level, real transcript fixtures on disk) and h4-retire-compose.mjs (real scheduleReverseAuditRound and composeReview from compiled dist):
| Cell | head | base |
|---|---|---|
| Working agent discloses (plain + bullet) | gaps collected under chunk 1, ok stays true, chunk stays covered |
no field; same gate |
| Idle agent (0 calls) copies the template | no gap credited; agent flagged idle | idle flagged |
| Blind agent (no diff in prompt) | no gap credited | blind flagged |
| Gap-free relaunch | silences the first attempt's gaps | gate unaffected |
| Two disclosing relaunches | gaps survive (no mutual supersession) | — |
| zh disclosure + fenced/blockquoted citations | only the real zh gap parsed | no field |
Real checkCoverageCommand handler (child process, unmocked stderr) |
budgetGaps in JSON report; NOTE on stderr with directives before agent text; exit code 0 — disclosure moves nothing |
no NOTE, exit 0 |
| Retirement: disclosure-only receipt, 2 rounds | chunk stays due (r1) | chunk retires — the disclosure doubles as the receipt's substance (the hole this PR closes) |
| Retirement: substantive receipt + separate disclosure | still retires, converged: true (convergence preserved) |
retires |
Retirement: inline one-liner (… — Budget gap: X, en + zh) |
clause cut at the marker; chunks stay due | chunks retire on the absorbed gap text |
| Compose: clean run + one gap | rendered in the Approve body as one attributed, inline-code sentence; event stays APPROVE | no channel |
| Compose: suggestion + gap | gap disclosed; "no blockers" certification denied | nothing disclosed |
Compose: hostile gap (</details> @victim #123 [click](http://evil.example) + ESC) |
rendered inside backticks via mdField, controls stripped, no live link |
hostile text never reaches the body (no channel) |
| Compose: 8 gaps | capped at 5 + "and 3 more" | no channel |
The retirement flips are the sharpest cells: on base, a reverse-audit chunk whose auditor wrote only No new issues found — followed by Budget gap: lines retires permanently (audited-on-paper, work never done); head keeps it under audit while still letting genuinely dry receipts converge.
Mutation matrix (vacuity, evidence 04-mutation-matrix-kills.png)
One mutant per load-bearing guard, run against the PR's own suites (vitest, from source); every revert restored and checksum-verified (sha256sum -c OK, git status clean). The unmutated control is green (below), so the kills mean something.
| Mutant | Guard removed | Suite(s) | Result |
|---|---|---|---|
| M1 | bidirectional clamp on the plan value (launchToolBudget) |
budget.test.ts | killed — exactly 1 test, intended assertion expected 200 to be 60 (a plan value of 100000 becomes a 200-call brief via the total cap, not a 60-call one) |
| M2 | MAX_TOTAL_TOOL_CALLS cap |
budget.test.ts + agent-prompt.test.ts | killed — 1 test, expected 40054 to be 200 (the garbled-chars brief of the last commit's message, reproduced) |
| M3 | zh alternatives in all three regexes | budget + retirement + check-coverage | killed — 2 tests (parser fixture; zh inline retirement). The check-coverage level did not go red — zh is pinned at parser + retirement layers; my independent h2 c6 cell covers the integration level |
| M4 | stripBudgetGapLines + inline cut in classifyReturn |
retirement.test.ts | killed — the 2 intended tests |
| M5 (positive control) | the budgetGaps collection hunk itself |
check-coverage + compose-review | killed — 6 tests across both suites, including the stderr-NOTE and body-rendering tests |
Completeness note, not a merge condition: the absence-asserting tests (budgetGaps == [] for idle/blind/superseded) survive M5 by construction — an absent producer trivially satisfies "nothing collected". The presence tests kill it, so the suite as a whole pins the feature; a future refactor of the guards should keep at least one presence test per path.
Targeted gates (all live, all green)
cd packages/cli && npx vitest run src/commands/review— 65 files, 2281 passed, 4 skipped, 0 failed (PR description's "62 files, 2200" predates the base move; measured numbers are these). Log:logs/review-suite-head.log. Suite liveness is proven by the mutation matrix above (the same instrument made it fail 6 ways).cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts— 5/5 (pointer↔heading gate covers the new DESIGN.md entry).npm run typecheck -w @qwen-code/qwen-code— clean (this is the class commit 2's message says tsc caught).- No repo-wide gate claimed; nothing re-run that CI already covers except where the A/B needed a known-clean number.
Findings
S1 (Suggestion, non-blocking) — the placeholder heuristic swallows sentence-shaped disclosures
PLACEHOLDER_GAP_RE matches on the leading token, so a gap phrased as a sentence beginning with a negative is silently dropped — measured against the shipped parser:
Budget gap: none of the migration paths were checked -> []
Budget gap: nothing beyond the tested paths was traced -> []
Budget gap: no checks beyond the diff were run -> []
Budget gap: none -> [] (by design)
The first three name real checks that were not performed; they vanish from budgetGaps, the posted body, and Step 3D's ruling — the exact "nothing downstream can tell 'no gaps' from 'gaps we failed to parse'" loss the code comment elsewhere warns about. The trigger requires an agent to deviate from the mandated noun-phrase form (the brief orders Budget gap: <the check> verbatim), so the dominant path is unaffected — hence Suggestion, not blocker. Direction, if addressed: treat leading-none/nothing/no-checks as a placeholder only when the remainder is short, or when it contains no of/beyond continuation; the fixture that would pin it is the three sentences above.
N1 (Nit) — ANSI residue after sanitization
\u001b[31m in a gap loses its ESC to the sanitizer but leaves the visible literal [31m in the rendered body (measured, h4 k3). Inert inside the inline-code span — cosmetic only; recorded so the residue is not mistaken later for an injection that got through.
Not covered
- The real-model latency A/B (350→341 calls, 51.2→46.1 min, 5 disclosing agents): requires model budget and a live review run; this environment executes neither. The PR itself labels the validated claim "compliance", which is what this round proved at the brief/parser/loop level; the latency improvement remains single-sample author evidence.
- Per-commit attribution: the checkout is depth 2 (
git rev-list HEAD^1..HEAD^2returns 1 of the 8 commits in the metadata snapshot — shallow boundary). The aggregateHEAD^1..HEADdiff is what was verified; the round-2/round-3 reversals described in commit messages are consistent with the measured behavior but were not separated commit-by-commit. - Wall-clock effect on a real fan-out wave — the motivating 11.7-vs-41-minute spread is a runtime property this harness cannot reproduce.
check-coverageload at 50+ real transcripts — fixtures exercise the guards, not scale.- zh disclosure-form completeness — the three pinned nouns (缺口/不足/用尽) are tested; other plausible phrasings (e.g. 预算超出) are not accepted. Not a defect (the brief's fixed marker is the contract and agents copy it), but a zh-narrating auditor that paraphrases is invisible to the channel.
verify-capture.mjscovered all four captures; no TUI/browser capture was needed (no TUI surface changed).
Methodology
CI verify container (node:22-bookworm), working tree at refs/pull/8708/merge with npm ci + npm run build already done at HEAD. Head arm = the prebuilt packages/cli/dist; base arm = scratch worktree at HEAD^1 with only packages/cli rebuilt (root node_modules symlinked; core resolves to the head tree, valid because the PR changes no core TS and the changed files import nothing from core — realpath check quoted above). Harnesses (h1–h4, lib-fixture.mjs, run-check-coverage.mjs) live in this artifact dir and import the compiled dist directly — no mocks of the code under test; coverage/retirement fixtures are real on-disk plans + prompt records + transcript JSONL in the shape the repo's own harness writes; the check-coverage handler ran in a child process on real stderr. Expected-base-failure cells are encoded as pass-when-red assertions. Raw logs: logs-h{1..4}-{head,base}.txt, logs/mut-M{1..5}.log, logs/review-suite-head.log, logs/skill-test-head.log, logs/typecheck-cli.log, logs/base-cli-build*.log.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Approving — reviewed at bc5222584d. C=0.
Every one of this PR's 54 review threads is marked resolved (5 Critical, 19 Suggestion, 30 untagged). A clean thread board is the easier case to get wrong than a messy one, so I did not read the flags as evidence — I re-derived the trickiest claimed fixes against the real exported functions.
The blocker is stale
reviewDecision reads CHANGES_REQUESTED, but both standing reviews are qwen-code-ci-bot at 687843f4cb (the very first commit) and 8b9e0f7d2b — and two commits have landed since (3d27977e49, bc5222584d), each of which is the fix for a finding raised against those heads. Nothing is actually outstanding; the PR just needs a fresh vote.
Critical audit — all five verified against the code, not the flags
| Thread | Verdict at bc5222584d |
|---|---|
| R1-1 budget-stop disclosures never reach coverage/verdict state | fixed — the brief mandates a one-per-line Budget gap: <check> format, coverageFromTranscripts parses it, and the gap is carried as unreviewed scope |
| R1-2 Agent 8 whole-diff prompts bypass the budget | fixed — buildWholeDiffBlock now emits the same toolBudgetBlock |
| R1-3 the 60-call cap can be below a whole-diff assignment's mandatory reads | fixed — launchToolBudget(effectiveLines, …, mandatoryReads) adds mandatory reads on top of the exploration allowance; probed below |
| R1-4 Agent 0 gets a diff-sized budget for issue-sized work | fixed — role 0 joins verify/7 in the exemption |
| R2-1 two compliant records supersede each other and drop all budget-gap disclosures | fixed — supersession is now gap-aware: gapsSuperseded silences a record's gaps only via a superseding record that is itself gap-free ("a genuine repair"), with the parse memoised per record |
What I probed rather than took on trust
The five self-review findings from 13:14/13:32 are the subtle ones, and four of them are pure functions, so I exercised them directly:
- Dedup (
budget.ts:251) —Budget gap: Xstated twice in one return now yields["X"], distinct gaps still yield both, and it is whitespace-tolerant. Fixed. - zh disclosures (
retirement.ts:336) —预算不足：…,预算缺口：…and the halfwidth-colon variant all parse to the gap. Fixed. mandatoryReadsclamp (budget.ts:214) — the reportedlaunchToolBudget(60, 400, 40_004)→ ~40,054 is gone: it now returns 200, as does1e9; negative andNaNboth floor to 50. The brief can no longer advertise an astronomical ceiling.- Mid-line disclosure (
retirement.ts:337) — fixed, via a mechanism worth naming because it is not the line collector:INLINE_BUDGET_GAP_RE(/(?:budget gap|预算(?:缺口|不足|用尽))[*_~]{0,3}[ \t]*[:：]/i) locates a gap written after a receipt separator on the same line, andretirement.ts` truncates the judged clause at that index so the gap text can no longer lend the round its substantiveness. Both colon forms, three zh phrasings, and markdown emphasis are covered.
One correction to my own work, recorded so it does not read as a finding: my first probe called budgetGapDisclosures for the mid-line case and got [], which looks like a miss. It is not — that function is the line-oriented collector by contract, and the inline case is handled by the regex above. Wrong entry point on my part, not a gap in the code.
Tests
554 pass locally at this commit: agent-prompt 209, compose-review 180, check-coverage 79, retirement 46, budget 40 — 0 failures. CI is green on this head across ubuntu Test, both Desktop Shell legs, web-shell smoke and coverage; only the bot's own review-pr run is still in progress.
Caveat on how I ran them: full-tree extraction kept stalling in my environment, so I ran the suites by overlaying this PR's complete packages/cli/src/commands/review/ tree (all 129 files, read straight out of refs/pull/8708/head) onto a working checkout of a different branch. Every affected suite passed, but that is not byte-identical to a clean checkout of bc5222584d, and CI — which is green — is the authority for the whole-repo result.
Note on shape, not a request
The soft budget now has three interlocking textual contracts — the mandated disclosure format in the brief, the parser in budget.ts, and the guard in retirement.ts — and the last two rounds were both about holes in that seam (language coverage, then line-anchoring). The mechanism is sound now and well tested, but it is worth knowing that the disclosure format is load-bearing across three files: a future edit to the brief's wording that drifts from INLINE_BUDGET_GAP_RE would silently reopen exactly this class. The regex covering three zh phrasings plus emphasis markers is the right instinct.
中文说明
批准 —— 审查提交 bc5222584d，C=0。
本 PR 全部 54 条评审线程均标记为已解决（5 Critical、19 Suggestion、30 无标签）。线程板干净其实比杂乱更容易出错，所以我没有把标记当作证据，而是针对真实导出函数重新推导了那些最微妙的修复声明。
阻塞是过期的：reviewDecision 显示 CHANGES_REQUESTED，但两条尚存的评审都来自 qwen-code-ci-bot，分别在 687843f4cb（首个提交）与 8b9e0f7d2b 上；此后已落地两个提交（3d27977e49、bc5222584d），且每个都是针对那些 head 上所提问题的修复。实际没有遗留项，只是需要一次新的投票。
5 个 Critical 全部按代码复核为已修复：R1-1 披露已进入 coverage/判定状态；R1-2 buildWholeDiffBlock 已发出同一预算块；R1-3 mandatory reads 现在叠加在探索额度之上；R1-4 role 0 已加入豁免；R2-1 supersession 已改为 gap-aware——只有本身无 gap 的记录（真正的修复）才能压制他人的 gap，且解析按记录 memo 化。
我实测而非采信的部分：去重（同一 gap 重复陈述现在只产出一条，且容忍空白差异）；zh 披露（预算不足：、预算缺口：、半角冒号变体均能解析）；mandatoryReads 钳制（原报告的 launchToolBudget(60, 400, 40_004) → 约 40,054 已消失，现返回 200，1e9 同样为 200，负数与 NaN 均落到 50）；行内披露（已修复，但机制值得点名——不是行采集器，而是 INLINE_BUDGET_GAP_RE 定位同行分隔符之后的 gap，再由 retirement.ts 在该下标处截断被判定子句，使 gap 文本无法再为该轮提供实质性）。
对我自己的一处更正（写明以免被读成发现）：我第一版探针对行内场景调用了 budgetGapDisclosures 并得到 []，看起来像漏判。并非如此——该函数按契约是面向整行的采集器，行内场景由上述正则处理。是我用错了入口，不是代码缺口。
测试：本提交下本地 554 全部通过（agent-prompt 209、compose-review 180、check-coverage 79、retirement 46、budget 40），0 失败；CI 在 ubuntu Test、两个 Desktop Shell、web-shell smoke 与覆盖率上全绿，仅机器人自身的 review-pr 仍在进行中。
关于运行方式的说明：我的环境下整树解包反复卡死，因此我把本 PR 完整的 packages/cli/src/commands/review/（全部 129 个文件，直接从 refs/pull/8708/head 读出）覆盖到另一分支的可用检出上来跑套件。所有受影响套件均通过，但这与 bc5222584d 的干净检出并非逐字节一致；全仓库结果以已绿的 CI 为准。
一点形态上的提醒（非修改请求）：软预算现在有三处相互咬合的文本契约——brief 中强制的披露格式、budget.ts 中的解析器、retirement.ts 中的守卫；最近两轮恰好都是这条缝上的漏洞（语言覆盖，随后是行锚定）。机制现已稳妥且测试充分，但值得知道该披露格式跨三个文件承重：将来若改动 brief 措辞而与 INLINE_BUDGET_GAP_RE 漂移，会静默重开这一类问题。正则同时覆盖三种中文表述与强调标记，是正确的直觉。
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Verified at head bc52225: the compile blocker and all eight round-1 ledger items are fixed — PlanReport declares budget, mutual supersession now requires a gap-free superseding record (with regression tests in both directions), the fixtures pin both scaling terms, the handler-level NOTE test exists, FINDINGS_LIST_READS is keyed on the declaration, and the placeholder filter handles punctuated non-answers. Gate safety holds end to end: budgetGaps never reaches ok, cappedBy, or the exit code, and the clamp applies server-side to whatever the plan reports, so the ceiling cannot be inflated by a garbled plan value.
One non-blocking P2 worth hardening before this channel carries more traffic: DANGEROUS_CHARS_RE (lib/budget.ts:297) strips C0/C1, U+2028/2029, and embeddings/overrides, but passes zero-widths (U+200B–200D, U+FEFF), directional marks (U+200E/F, U+061C), and the bidi isolates (U+2066–2069 — the Trojan Source vector). Gap text is the first channel where raw sub-agent prose reaches a bot-posted PR body; the impact is bounded today (mdField strips backticks/newlines, length and count are capped), but the fix is one character class: \u200b-\u200f\u061c\ufeff\u2066-\u2069.
A few smaller non-blockers: the displayed reads term at agent-prompt.ts:425 is not clamped, so a garbled plan (chars: 1e9) renders a sentence contradicting the 200 cap; two disclosing relaunches of one chunk emit the identical chunk 1: line twice in the composed body (dedup on (agent, gap) would fix it); and stripBudgetGapLines tests the raw line while the parser replaces dangerous chars first, so a U+2028/2029-bearing disclosure is collected but not stripped from the receipt. Nothing blocks merge.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x), Integration Tests (CLI, No Sandbox), build-cli were skipped in CI and their full suites and the CLI bundle build did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x), Integration Tests (CLI, No Sandbox), build-cli were skipped in CI and their full suites and the CLI bundle build did not run locally。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| for (const f of Array.isArray(chunk.files) ? chunk.files : []) { | ||
| const e = f as { path?: string; newStart?: number; newEnd?: number }; | ||
| const span = | ||
| typeof e?.newStart === 'number' && typeof e?.newEnd === 'number' | ||
| ? e.newEnd - e.newStart + 1 | ||
| : 0; |
There was a problem hiding this comment.
[Suggestion] weightedTerritoryLines weights by NEW-side line spans, blind to deletions: a pure-deletion hunk records span 1, so a deletion-heavy chunk's source share collapses toward the 1/8 prose rate, budgeting the removed-behavior audit at ~1/8 the plan rate — Failure scenario: Probe-verified with a buildable diff: a 155-line pure-deletion source hunk (newEnd === newStart, span 1) plus a 155-line lockfile hunk → weighted territory 41 vs raw 310 (~0.132, the finding's 0.127 factor), chunk allowance drops to 32 vs 37 on raw lines, while the plan-level rate (37) counts the deleted lines as source. The launch's removed-behavior audit — the dimension that exists precisely for deleted lines — is budgeted at roughly 1/8 the plan's own rate; under a soft ceiling the agent stops earlier on exactly the deletion-heavy chunks the audit cares about.
| for (const f of Array.isArray(chunk.files) ? chunk.files : []) { | |
| const e = f as { path?: string; newStart?: number; newEnd?: number }; | |
| const span = | |
| typeof e?.newStart === 'number' && typeof e?.newEnd === 'number' | |
| ? e.newEnd - e.newStart + 1 | |
| : 0; | |
| Weight by per-file added+removed diff-line counts instead of new-side spans, or treat a span of 1 on a file whose removedLines is large as source-heavy. |
中文说明
**建议：**weightedTerritoryLines weights by NEW-side line spans, blind to deletions: a pure-deletion hunk records span 1, so a deletion-heavy chunk's source share collapses toward the 1/8 prose rate, budgeting the removed-behavior audit at ~1/8 the plan rate。
**失败场景：**Probe-verified with a buildable diff: a 155-line pure-deletion source hunk (newEnd === newStart, span 1) plus a 155-line lockfile hunk → weighted territory 41 vs raw 310 (~0.132, the finding's 0.127 factor), chunk allowance drops to 32 vs 37 on raw lines, while the plan-level rate (37) counts the deleted lines as source. The launch's removed-behavior audit — the dimension that exists precisely for deleted lines — is budgeted at roughly 1/8 the plan's own rate; under a soft ceiling the agent stops earlier on exactly the deletion-heavy chunks the audit cares about.。
**建议修复：**Weight by per-file added+removed diff-line counts instead of new-side spans, or treat a span of 1 on a file whose removedLines is large as source-heavy.。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const added = typeof f?.addedLines === 'number' ? f.addedLines : 0; | ||
| const fileLines = | ||
| typeof f?.fileLines === 'number' && Number.isFinite(f.fileLines) | ||
| ? f.fileLines | ||
| : 0; |
There was a problem hiding this comment.
[Suggestion] addedLines/removedLines lack the Number.isFinite guard fileLines has; a garbled 1e999 plan value makes mandatoryReads Infinity, floored to 0 — an under-budgeted invariant launch ('About 30 tool calls … ~0 reads') — Failure scenario: Probe-verified: a plan carrying 'addedLines': 1e999 (JSON-parse → Infinity) passes the typeof guard, Math.ceil(Math.max(Infinity, fileLines)/500) = Infinity → mandatoryReads = Infinity → toolBudgetBlock's Number.isFinite guard sets reads = 0 → the brief prints 'About 30 tool calls … and the ~0 reads … are already counted in' while the launch's actual mandatory reading (brief + whole post-change file + its diff slice, floor 4) is silently omitted — in exactly the garbled-plan class this code defends against elsewhere (the same statement guards fileLines).
| const added = typeof f?.addedLines === 'number' ? f.addedLines : 0; | |
| const fileLines = | |
| typeof f?.fileLines === 'number' && Number.isFinite(f.fileLines) | |
| ? f.fileLines | |
| : 0; | |
| Add Number.isFinite to the addedLines/removedLines guards (same shape as fileLines), so changed and Math.max(added, fileLines) stay finite. |
中文说明
**建议：**addedLines/removedLines lack the Number.isFinite guard fileLines has; a garbled 1e999 plan value makes mandatoryReads Infinity, floored to 0 — an under-budgeted invariant launch ('About 30 tool calls … ~0 reads')。
**失败场景：**Probe-verified: a plan carrying 'addedLines': 1e999 (JSON-parse → Infinity) passes the typeof guard, Math.ceil(Math.max(Infinity, fileLines)/500) = Infinity → mandatoryReads = Infinity → toolBudgetBlock's Number.isFinite guard sets reads = 0 → the brief prints 'About 30 tool calls … and the ~0 reads … are already counted in' while the launch's actual mandatory reading (brief + whole post-change file + its diff slice, floor 4) is silently omitted — in exactly the garbled-plan class this code defends against elsewhere (the same statement guards fileLines).。
**建议修复：**Add Number.isFinite to the addedLines/removedLines guards (same shape as fileLines), so changed and Math.max(added, fileLines) stay finite.。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| mandatoryReads: Math.max( | ||
| 4, | ||
| 2 + Math.ceil(Math.max(added, fileLines) / LINES_PER_FILE_READ), | ||
| ), |
There was a problem hiding this comment.
[Suggestion] The invariant-agent reads estimate counts the file's diff slice as exactly one read, but a large heavy-file slice mandates many reads at the 25k-char cap — the undercount scales with the file — Failure scenario: The test's own flagship fixture huge.ts (diffRange 1-3600) mandates paging ~3600 diff lines ≈ 8-15 reads plus 7 whole-file reads plus the brief — ~16-23 reads total — while the formula states 2 + ceil(3200/500) = 9 and the block prints 'the ~9 reads … are already counted in'. The soft ceiling fires inside the mandated paging on exactly the launch class (heavily-rewritten files) the invariant agent exists for.
| mandatoryReads: Math.max( | |
| 4, | |
| 2 + Math.ceil(Math.max(added, fileLines) / LINES_PER_FILE_READ), | |
| ), | |
| Budget the slice from the file's diffRange span when present, or fold the slice into the fileLines term; add a fixture whose diffRange exceeds one page. |
中文说明
**建议：**The invariant-agent reads estimate counts the file's diff slice as exactly one read, but a large heavy-file slice mandates many reads at the 25k-char cap — the undercount scales with the file。
**失败场景：**The test's own flagship fixture huge.ts (diffRange 1-3600) mandates paging ~3600 diff lines ≈ 8-15 reads plus 7 whole-file reads plus the brief — ~16-23 reads total — while the formula states 2 + ceil(3200/500) = 9 and the block prints 'the ~9 reads … are already counted in'. The soft ceiling fires inside the mandated paging on exactly the launch class (heavily-rewritten files) the invariant agent exists for.。
**建议修复：**Budget the slice from the file's diffRange span when present, or fold the slice into the fileLines term; add a fixture whose diffRange exceeds one page.。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| transcript('try2', good(1), { | ||
| calls: 2, | ||
| text: 'Partial again.\nBudget gap: the callers of the renamed export', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The mutual-supersession test discloses the same gap text in both relaunches, so it cannot distinguish 'both survive' from 'newer suppresses older' — Failure scenario: Probe-verified: under a recency-keyed mutant of gapsSuperseded (suppress any later verbatim record), the original test still passes (identical text satisfies gaps.length > 0), while a discriminating variant with distinct gap texts fails under the mutant and passes on unmutated code. In production, attempt 1's distinct gap (a different check cut short) is silently erased with no test red.
| transcript('try2', good(1), { | |
| calls: 2, | |
| text: 'Partial again.\nBudget gap: the callers of the renamed export', | |
| }); | |
| Give the two attempts different gap texts (gap A / gap B) and assert both appear across budgetGaps. |
中文说明
**建议：**The mutual-supersession test discloses the same gap text in both relaunches, so it cannot distinguish 'both survive' from 'newer suppresses older'。
**失败场景：**Probe-verified: under a recency-keyed mutant of gapsSuperseded (suppress any later verbatim record), the original test still passes (identical text satisfies gaps.length > 0), while a discriminating variant with distinct gap texts fails under the mutant and passes on unmutated code. In production, attempt 1's distinct gap (a different check cut short) is silently erased with no test red.。
**建议修复：**Give the two attempts different gap texts (gap A / gap B) and assert both appear across budgetGaps.。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| "diff with no line ranges covers nothing. Every rostered agent's " + | ||
| 'launch block — its diff reads included — comes from ' + |
There was a problem hiding this comment.
[Suggestion] The rewritten missingChunks stderr NOTE (the operator-facing remediation with four factual claims) has zero test coverage — Failure scenario: The three handler tests cover only the blindAgents, missingRoles and unreadBriefs branches; no test asserts any of the new NOTE phrases. A future drift between agent-prompt's actual interface and this guidance (e.g. --role learning to rebuild chunks) ships unnoticed, and the operator follows stale rebuild instructions in exactly the 'nobody read those lines' scenario this command exists to remediate.
| "diff with no line ranges covers nothing. Every rostered agent's " + | |
| 'launch block — its diff reads included — comes from ' + | |
| Add a handler-level test for the missingChunks branch asserting the new NOTE text, in the style of the new budget-gap NOTE test. |
中文说明
**建议：**The rewritten missingChunks stderr NOTE (the operator-facing remediation with four factual claims) has zero test coverage。
**失败场景：**The three handler tests cover only the blindAgents, missingRoles and unreadBriefs branches; no test asserts any of the new NOTE phrases. A future drift between agent-prompt's actual interface and this guidance (e.g. --role learning to rebuild chunks) ships unnoticed, and the operator follows stale rebuild instructions in exactly the 'nobody read those lines' scenario this command exists to remediate.。
**建议修复：**Add a handler-level test for the missingChunks branch asserting the new NOTE text, in the style of the new budget-gap NOTE test.。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const PLACEHOLDER_GAP_RE = | ||
| /^(?:<[^>]*>|none\b.*|n\/a\b.*|nothing\b.*|no (?:gaps?|checks?)\b.*|[-—*_~`]+)$/i; |
There was a problem hiding this comment.
[Suggestion] PLACEHOLDER_GAP_RE token matching is over-broad (drops real gaps phrased with a denial or template token) and English-only (mints zh phantom gaps) — Failure scenario: An agent whose ceiling stopped a real check writes Budget gap: none of the callers were traced or Budget gap: <trace the callers of parseArgs>; none\b.* / <[^>]*> match any continuation and the line is dropped (verified: parses to []). The orchestrator then rules 'no gaps' and an Approve can post over the untraced required path. Conversely 预算缺口：无 mints a phantom gap that denies certification on a clean run.
| const PLACEHOLDER_GAP_RE = | |
| /^(?:<[^>]*>|none\b.*|n\/a\b.*|nothing\b.*|no (?:gaps?|checks?)\b.*|[-—*_~`]+)$/i; | |
| Match complete denials only — require the remainder after the token to be end-of-line, punctuation, or a parenthetical — and drop only the exact `<the check>` template rather than any `<...>` content; add zh non-answer tokens (无, 没有, 未发现). |
中文说明
**建议：**PLACEHOLDER_GAP_RE token matching is over-broad (drops real gaps phrased with a denial or template token) and English-only (mints zh phantom gaps)。
**失败场景：**An agent whose ceiling stopped a real check writes Budget gap: none of the callers were traced or Budget gap: <trace the callers of parseArgs>; none\b.* / <[^>]*> match any continuation and the line is dropped (verified: parses to []). The orchestrator then rules 'no gaps' and an Approve can post over the untraced required path. Conversely 预算缺口：无 mints a phantom gap that denies certification on a clean run.。
**建议修复：**Match complete denials only — require the remainder after the token to be end-of-line, punctuation, or a parenthetical — and drop only the exact <the check> template rather than any <...> content; add zh non-answer tokens (无, 没有, 未发现).。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| gaps.push(truncateGap(raw)); | ||
| if (gaps.length >= MAX_GAPS_PER_AGENT) break; |
There was a problem hiding this comment.
[Suggestion] Disclosures beyond the 8-per-agent cap are dropped while stripBudgetGapLines strips every matching line — overflow checks are invisible on every output surface — Failure scenario: An agent whose ceiling cut more than 8 checks lists them per the brief's mandate ('disclose each unfinished check on its own line' — no count limit). Probe-verified: 9 disclosures → 8 parsed, 'check 8' dropped; the stripper removes all 9 lines from the judged receipt. The 9th+ checks reach neither the NOTE nor the body nor the judged receipt — unrecoverable, exactly the 'silent loss in a channel whose promise is delivery' the module's own docstring names.
| gaps.push(truncateGap(raw)); | |
| if (gaps.length >= MAX_GAPS_PER_AGENT) break; | |
| Surface the overflow (e.g. append ', and N more' to the agent's entry) or cap stripBudgetGapLines on the same count so the receipt keeps the overflow lines as substance. |
中文说明
**建议：**Disclosures beyond the 8-per-agent cap are dropped while stripBudgetGapLines strips every matching line — overflow checks are invisible on every output surface。
**失败场景：**An agent whose ceiling cut more than 8 checks lists them per the brief's mandate ('disclose each unfinished check on its own line' — no count limit). Probe-verified: 9 disclosures → 8 parsed, 'check 8' dropped; the stripper removes all 9 lines from the judged receipt. The 9th+ checks reach neither the NOTE nor the body nor the judged receipt — unrecoverable, exactly the 'silent loss in a channel whose promise is delivery' the module's own docstring names.。
**建议修复：**Surface the overflow (e.g. append ', and N more' to the agent's entry) or cap stripBudgetGapLines on the same count so the receipt keeps the overflow lines as substance.。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| if ( | ||
| records.some( | ||
| (r) => | ||
| r !== rec && | ||
| wasDeliveredVerbatim(r.launchPrompt, b) && | ||
| r.successfulCallArgs.some((a) => a.includes(needle)) && | ||
| gapsOf(r).length === 0, | ||
| ) | ||
| ) { | ||
| return true; |
There was a problem hiding this comment.
[Suggestion] The whole-diff branch of gapsSuperseded (Step 3A) is untested — only the chunk-record shape is pinned; the guard is load-bearing (mutation + flip probe confirmed) — Failure scenario: The mutual-supersession-into-silence bug the code comment documents is guarded in the whole-diff branch by the same gapsOf(r).length === 0 requirement, but no test pins it there: a mutant removing that requirement stays green (verified), and on a real Step 3A run where a dimension agent discloses and a gap-free relaunch exists, disclosures would be dropped or kept stale with no test red.
| if ( | |
| records.some( | |
| (r) => | |
| r !== rec && | |
| wasDeliveredVerbatim(r.launchPrompt, b) && | |
| r.successfulCallArgs.some((a) => a.includes(needle)) && | |
| gapsOf(r).length === 0, | |
| ) | |
| ) { | |
| return true; | |
| Add a plan3a-style test: disclosing attempt + gap-free relaunch → gaps silenced; two disclosing attempts → gaps kept. |
中文说明
**建议：**The whole-diff branch of gapsSuperseded (Step 3A) is untested — only the chunk-record shape is pinned; the guard is load-bearing (mutation + flip probe confirmed)。
**失败场景：**The mutual-supersession-into-silence bug the code comment documents is guarded in the whole-diff branch by the same gapsOf(r).length === 0 requirement, but no test pins it there: a mutant removing that requirement stays green (verified), and on a real Step 3A run where a dimension agent discloses and a gap-free relaunch exists, disclosures would be dropped or kept stale with no test red.。
**建议修复：**Add a plan3a-style test: disclosing attempt + gap-free relaunch → gaps silenced; two disclosing attempts → gaps kept.。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const gaps = gapsOf(rec); | ||
| if (gaps.length > 0 && !gapsSuperseded(rec, chunk)) { | ||
| budgetGaps.push({ agent: name, gaps }); | ||
| } |
There was a problem hiding this comment.
[Suggestion] A bare-path agent (prompt names the diff, no ranges) that never opened the diff still contributes a budget-gap disclosure — Failure scenario: Verified by probe (plan3a fixture): a record with a bare-path prompt, one successful non-diff tool call, zero diff reads, and a 'Budget gap:' line lands in budgetGaps while covering nothing and being flagged nowhere — not idle, not blind, not unopened. Its gap then drives the stderr NOTE and the body's 'Not explored to full depth' line — a false 'working reviewer's check was cut short' signal for an agent that never read the diff. The pointed-at-lines cousin is suppressed by the unopened guard, so the same pathology is handled two ways depending on prompt shape.
| const gaps = gapsOf(rec); | |
| if (gaps.length > 0 && !gapsSuperseded(rec, chunk)) { | |
| budgetGaps.push({ agent: name, gaps }); | |
| } | |
| Collect gaps after the ranges.length === 0 guard, so a disclosure counts only for an agent that engaged the diff. |
中文说明
**建议：**A bare-path agent (prompt names the diff, no ranges) that never opened the diff still contributes a budget-gap disclosure。
**失败场景：**Verified by probe (plan3a fixture): a record with a bare-path prompt, one successful non-diff tool call, zero diff reads, and a 'Budget gap:' line lands in budgetGaps while covering nothing and being flagged nowhere — not idle, not blind, not unopened. Its gap then drives the stderr NOTE and the body's 'Not explored to full depth' line — a false 'working reviewer's check was cut short' signal for an agent that never read the diff. The pointed-at-lines cousin is suppressed by the unopened guard, so the same pathology is handled two ways depending on prompt shape.。
**建议修复：**Collect gaps after the ranges.length === 0 guard, so a disclosure counts only for an agent that engaged the diff.。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const clause = receipt?.[1] ?? ''; | ||
| const inlineGap = INLINE_BUDGET_GAP_RE.exec(clause); | ||
| const judgedClause = | ||
| inlineGap === null ? clause : clause.slice(0, inlineGap.index); |
There was a problem hiding this comment.
[Suggestion] A gap with a non-canonical marker ('Budget-gap:', 'Gap:', spaced zh) escapes both the line-strip and the inline cut, survives as the receipt's clause, passes substantiveClause, and retires the chunk — a skipped audit — Failure scenario: Probe-verified: receipts whose only clause is the disclosure, in three marker spellings — 'Budget-gap:' and 'Gap:' retire chunk 13 after two rounds; the canonical 'Budget gap:' keeps it due. The hyphenated spelling appears throughout this diff's SKILL.md/comments. BUDGET_GAP_LINE_RE / INLINE_BUDGET_GAP_RE match only the single-space literal, so the line survives the strip and the clause cut never fires; with the tool-call + territory-read bar met, substantiveClause returns true → classified dry. Two such rounds retire the chunk: a territory whose walks the ceiling cut short is certified twice-clean — a skipped audit, the module's own docstring's never-mode. Since budgetGapDisclosures shares the marker regex, the gap is also invisible to coverage — fully silent.
| const clause = receipt?.[1] ?? ''; | |
| const inlineGap = INLINE_BUDGET_GAP_RE.exec(clause); | |
| const judgedClause = | |
| inlineGap === null ? clause : clause.slice(0, inlineGap.index); | |
| Make the marker token-tolerant in both regexes (allow an optional hyphen or arbitrary whitespace run between the words — budget[ \t-]*gap) and add a regression test asserting a gap-only receipt with a hyphenated/abbreviated marker stays due. |
中文说明
**建议：**A gap with a non-canonical marker ('Budget-gap:', 'Gap:', spaced zh) escapes both the line-strip and the inline cut, survives as the receipt's clause, passes substantiveClause, and retires the chunk — a skipped audit。
**失败场景：**Probe-verified: receipts whose only clause is the disclosure, in three marker spellings — 'Budget-gap:' and 'Gap:' retire chunk 13 after two rounds; the canonical 'Budget gap:' keeps it due. The hyphenated spelling appears throughout this diff's SKILL.md/comments. BUDGET_GAP_LINE_RE / INLINE_BUDGET_GAP_RE match only the single-space literal, so the line survives the strip and the clause cut never fires; with the tool-call + territory-read bar met, substantiveClause returns true → classified dry. Two such rounds retire the chunk: a territory whose walks the ceiling cut short is certified twice-clean — a skipped audit, the module's own docstring's never-mode. Since budgetGapDisclosures shares the marker regex, the gap is also invisible to coverage — fully silent.。
**建议修复：**Make the marker token-tolerant in both regexes (allow an optional hyphen or arbitrary whitespace run between the words — budget[ \t-]*gap) and add a regression test asserting a gap-only receipt with a hyphenated/abbreviated marker stays due.。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
|
Released in v0.21.8. |
4 participants
What this PR does
Adds
agentToolBudgetto the review plan's budget and bakes a soft tool-call ceiling into every finder, chunk, invariant, persona, Agent 8 and reverse-audit brief. The plan records the base rate —clamp(30 + effective/20, 30, 60), computed and recorded like every other budget arm so no caller can inflate it — and the ceiling is scoped per launch (launchToolBudget): the plan's value is clamped into the budget's own band in both directions (and the stated total is capped so a garbled plan cannot route around the clamp through the reads term), a chunk or invariant agent derives its allowance from its own territory at the same rate but never above the plan's allowance (the plan stays the one number every launch answers to, version skew included), and every launch's mandatory reads — brief, diff pages, the files its method mandates — ride on top of the allowance rather than inside it, so a huge diff's assigned chunk reads can never exhaust a whole-diff role's exploration. The verifier is exempt (its per-finding re-trace must not stop early;verifyShardalready governs its load), as are Build & Test (deterministic commands) and Agent 0 (its mandatory work scales with the linked issues, not the diff). A plan without the field — an older CLI — gets no ceiling: the fallback errs toward coverage, matching the documented pre-budget behaviour.The wording guards the two failure modes a budget invites: at the ceiling an agent stops exploring, never reporting — findings in hand are filed, and each stopped check is disclosed as a fixed
Budget gap: <the check>line — and the recall rule is restated beside it so the ceiling cannot read as a reporting cap. The disclosure loop is deterministic end to end: one shared line-based parser (budgetGapDisclosures— markdown-tolerant incl. zh forms, quotation-aware (fenced code and blockquotes are citation, not use), placeholder-dropping in any punctuation, deduplicating, sanitized against C0/C1/bidi/U+2028, linear on pathological inputs) feedscheck-coverage'sbudgetGaps(collected behind the same blind/idle guards as theUncoverable:claim, with gap-aware supersession — only a gap-free relaunch silences a disclosure; a NOTE, never a gate failure, and never a credit change: an arithmetic that only bites the discloser teaches agents not to disclose).compose-reviewrenders the gaps into the posted body's "Not reviewed" section mechanically (the Approve body included) as ONE bounded, agent-attributed,mdField-inert sentence that never caps but does deny the "no blockers" certification, and drops itself for any gap the orchestrator promoted — SKILL.md Step 3D owes each gap that ruling, and a promoted required trace joinsunreviewedDimensions(forbids Approve). Retirement judges a reverse-audit receipt with its disclosure lines stripped (inline one-liners cut at the marker): the admission can neither serve as the receipt's substance nor block a receipt that is substantive without it, so honest disclosure and loop convergence coexist.Why it's needed
A fan-out wave's wall clock is its slowest agent, and the slowest agent is reliably a wanderer: two measured CI runs of the same 14-agent wave on comparable diffs took 11.7 and 41 minutes, the difference being individual agents spending 40–100 model calls exploring the tree while healthy agents settle at 25–45 with indistinguishable findings (the sibling concurrency fix had already named per-agent efficiency as the next target). The new DESIGN.md entry records the incident; the SKILL.md budget bullet points at it.
Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/lib/budget.test.ts src/commands/review/agent-prompt.test.ts— the scaling/clamps (floor 30, +1/20 effective lines, cap 60, coarse all-prose rate) and the per-launch numbers (chunk, invariant, whole-diff, Agent 8), a full-roster sweep pinning the exemption set to exactly{verify, 7, 0}, garbledagentToolBudgetvalues falling back to no ceiling, the runtime shape ofreviewBudget's return, the recall-rule/format restatement, andbudgetGapsparsing (withokstaying true) are all pinned. Full review suite: 62 files, 2200 passed.cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts— the pointer↔heading gate covers the new DESIGN.md incident.Evidence (Before & After)
Validated with a real-model A/B (qwen3.8-max, local medium review of the same 2,001-line uncommitted diff, same worktree, fresh QWEN_HOME per arm; the only variable is the bundle):
Compliance is the validated claim: five agents cited the budget and disclosed skipped checks; the top wanderer compressed toward the ceiling; and the finding set kept the same Critical neighborhood (the severity difference on one shared finding matches the grading variance the control arms show between themselves, not a budget suppression — the budget arm's set is the baseline arm's set modulo that wobble). The latency deltas are directionally positive but single-sample; the measured 11.7-vs-41-minute spread is the tail this exists to cut, and that shows up across runs, not within one.
Tested on
Risk & Scope
Budget gap:disclosures are parsed bycheck-coverageintobudgetGaps, and the Step 3D ruling routes required-trace gaps intounreviewedDimensionsand optional depth into the report's "Not reviewed" accounting.Confidence: lowif unverified), a test pins that restatement, and the A/B arm lost no finding neighborhood.budgetExempton theBrief(set forverify,7,0, each with its reason at the role's entry); the roster test walksObject.keys(BRIEFS)and pins the exempt set, so a later role must declare — it cannot silently join either side.Linked Issues
Follow-up to the /review latency line (8642, 8648); the per-agent efficiency target named in 8631's out-of-scope list.
中文说明
这个 PR 做了什么
在 review plan 的 budget 中新增
agentToolBudget，并将软工具调用上限烙入每个 finder、chunk、invariant、人格、Agent 8 与反审 brief。plan 记录基础费率——clamp(30 + effective/20, 30, 60)，与其他 budget 分量同样计算并记录，调用方无法放大——上限按 launch 定界（launchToolBudget）：plan 值双向 clamp 进预算自身区间、总数封顶在 200 防止损坏的 chars 绕过 clamp，chunk / invariant agent 按自己领地以同一费率推导额度但永不超过 plan 额度（plan 仍是每次 launch 都要回答的那一个数字，版本偏斜下亦然），且每次 launch 的必读次数——brief、diff 分页、方法要求的文件——叠加在额度之上而非之内，巨型 diff 的指派 chunk 读取永远耗不尽 whole-diff 角色的探索空间。verifier 豁免（逐 finding 重溯不允许提前停；其负载由verifyShard治理），Build & Test 豁免（确定性命令），Agent 0 豁免（其必做工作随关联 issue 而非 diff 规模伸缩）。旧 CLI 写的 plan 没有该字段——回退为无上限，与既有 pre-budget 回退一致，朝覆盖侧偏。措辞防住预算的两类失败形态：到达上限时 agent 停止的是探索而非上报——在手的发现照常提交，每个未完成检查以固定格式
Budget gap: <检查>逐行披露——且召回规则在上限旁重申，防止其被读成上报上限。披露闭环端到端确定性：单一共享的按行解析器（budgetGapDisclosures——容忍 markdown 与中文形式、区分引用与使用（围栏代码与 blockquote 是引用）、任意标点下丢弃占位符、去重、对 C0/C1/bidi/U+2028 消毒、病态输入线性）供给check-coverage的budgetGaps（与Uncoverable:同套 blind/idle 守卫，取代关系感知 gap——只有无 gap 的重启才压制披露；NOTE 而非门禁失败，也绝不改变 coverage credit：只咬披露者的算术会教 agent 不披露）。compose-review把 gap 机械渲染进发布 body 的 "Not reviewed"（含 Approve body）——单句、有上限、按 agent 归属、经mdField惰性化，不封顶但会否定 "no blockers" 措辞，且对 orchestrator 已晋升的 gap 自动让位——SKILL.md Step 3D 负责裁决，晋升的必做追踪进unreviewedDimensions（禁止 Approve）。retirement 以剥离披露行后的回执判断 dry（单行内联披露在标记处截断）：承认之语既不能充当回执的实质，也不能阻塞本身已有实质的回执——诚实披露与循环收敛并存。为什么需要
扇出波的墙钟等于最慢的 agent，而最慢的 agent 稳定地是游走者：同一 14-agent 波在可比 diff 上实测 11.7 与 41 分钟，差异来自个别 agent 花 40–100 次模型调用在树里游走，而健康 agent 25–45 次即得到无差别的发现集合。新增 DESIGN.md 事故条目记录该测量，SKILL.md 的 budget 条目指向它。
证据（真模型 A/B）
qwen3.8-max、同一 2001 行未提交 diff、同一 worktree、每臂全新 QWEN_HOME、唯一变量为 bundle：总调用 350→341；头部游走者 70/53/41→63/51/43；最慢 agent 23.4→20.1 分钟；墙钟 51.2→46.1 分钟；5 个 agent 在返回中引用预算并披露跳过的检查（对照臂 0 个）；发现集合保持同一 Critical 邻域（一处共享发现的严重度差异与对照臂之间自身的评级方差一致，非预算压制）。已验证的主张是服从性；延迟增量方向为正但为单样本——11.7 vs 41 分钟的散布是跨 run 呈现的尾部，正是该机制要削的对象。
风险与范围
Budget gap:披露由check-coverage解析进budgetGaps，Step 3D 裁决把必做追踪缺口路由进unreviewedDimensions，可选深度进 "Not reviewed" 记账。Confidence: low提交），测试钉住该重申，A/B 臂无发现邻域丢失。Brief上的budgetExempt（verify、7、0各自带理由声明）；roster 测试遍历Object.keys(BRIEFS)并钉住豁免集——后加的角色必须显式声明，无法静默加入任何一侧。关联
/review 延迟工作线（8642、8648）的后续；8631 范围外清单点名的单 agent 效率目标。