perf(review): cap the reverse audit and shed Agent 8 on a huge diff - #8773
Merged
Conversation
The sweep's rationale, extended to the high tier's second pass: below ~25 effective lines a diff fits in one view, and a second reader of the same few hunks is the same reader. The reverse audit is that second reader run as a LOOP — to two consecutive dry audits per chunk — and on a micro diff the loop re-reads the same lines round after round. Measured on a 23-line one-file PR: three rounds, eleven minutes, and the single finding round 2 produced was verifier-rejected. The plan's budget gains reverseAuditRounds (5, or 1 below the sweep floor; never 0 — one round IS the second look, the budget must not scale a dimension away). Under a cap of 1 a single substantive dry audit — or a retroactively-dry round — is the certificate, final, with no cold check: the retirement scheduler retires on one dry audit and reads retirement from round 2, so an all-dry round 1 exits 5 CONVERGED at the round-2 build. A hot chunk at the cap gets a deterministic ROUND CAP refusal from the admission gate (exit 4, the deadline gate's termination contract, naming the unreviewedDimensions entry only when scope is outstanding) — the cap is enforced by the builder, not by the orchestrator counting rounds. An older or garbled plan reads as the full cap: more auditing, never less.
wenshao
requested review from
LaZzyMan,
doudouOUC,
tanzhenxin and
yiliang114
as code owners
August 9, 2026 01:02
Collaborator
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
|
Thanks for the PR — re-running triage on the refocused version ( Template looks good ✓ — all required sections present and substantive ( Problem: observed, and costly — a survey of one CI window found 26 review-pr jobs timing out, ~122 hours of compute, zero posted, several the same PR retried and re-timed-out. The driver is measured: a reverse-audit round re-reads the whole diff against a growing findings list (~90 min on a 4,000-line PR), so five rounds alone exceed the six-hour ceiling before the fan-out is counted. This is the opposite end of the size band from the earlier micro-diff work — the end where the timeouts actually live. Direction: aligned. Second piece of the /review latency line (#8642, #8772; the micro-diff kill switch #8774 already landed). It stays inside the review pipeline's budget machinery — no user-facing product surface, no new dependencies. Size: author is a maintainer, so the two-tier core gate does not apply. The code lives in Approach: scope feels right — and notably tighter than the first iteration. The round-1 review's two Criticals proved the micro tier was built on a path micro diffs never take; the fix was structural (drop the tier, not patch it), leaving exactly two knobs where the harm is measured: Risk: no elevated risk signals — none of the changed files match the revert-correlated path list. Moving on to code review. 🔍 中文说明感谢贡献 —— 应 模板完整 ✓ —— 所有必需章节齐全且内容充实(非 UI 改动的 问题:已观测且代价高昂 —— 对一个 CI 窗口的调查显示 26 个 review-pr 作业超时、约 122 小时算力、零发布,多个是同一 PR 反复重试再超时。驱动因素有实测:反审每轮都要对着增长的发现清单重读整个 diff(4000 行 PR 上约 90 分钟/轮),仅 5 轮就超过 6 小时上限,还没算扇出。这是与微型 diff 工作相反的尺寸带 —— 也正是超时真正发生的尺寸带。 方向:对齐。/review 延迟线的第二块(#8642、#8772;微型 diff 开关 #8774 已合并)。改动停留在评审管线的预算机制内 —— 无用户可见产品面、无新依赖。 规模:作者是维护者,两级核心门不适用。代码位于 方案:范围合理 —— 且明显比第一版收敛。首轮评审的两个 Critical 证明微型档建在微型 diff 从不走的路径上;修复是结构性的(整档移除而非打补丁),只留下两个恰好落在实测损害处的旋钮: 风险:无升级风险信号 —— 改动文件均未命中与 revert 相关的路径清单。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Code review — the approach matches what I'd have proposed independently: tier the budget on
No blocking findings, and no AGENTS.md violations — tests collocated, style consistent with the rest of the review library. CI test evidence — from the PR's own checks on the reviewed commit (this triage run does not execute PR code):
No red checks. The macOS/Windows and integration skips are the repo's standing CI design (those jobs run in the merge queue). The cancelled The suite pins every mechanism — the 2999/3000 boundary, the 中文说明代码审查 —— 方案与我独立设想的一致:按
无阻断发现,亦无 AGENTS.md 违规 —— 测试与源码同置,风格与评审库其余部分一致。 CI 测试证据 —— 取自该提交上 PR 自己的检查(本 triage 不执行 PR 代码):无红色检查;Linux 单测、web-shell E2E smoke、Desktop Shell 均为 success。macOS/Windows 与集成测试的跳过是仓库既有 CI 设计(在合并队列中运行)。该提交上被取消的重复 套件钉住了全部机制 —— 2999/3000 边界、两个旋钮上 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
requested changes
Aug 9, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): PR #8773 (QwenLM/qwen-code) adds a reverseAuditRounds c...: run the three updated test files (budget.test.ts, retirement.test.ts, agent-prompt.test.ts) to confirm they pass — node_modules absent in the worktree and paren….
Test Plan (not a blocker): 302 passed — this review observed 18611, 19546, 1124, 1470, 481, 2941, 454 passed; 2281 passed — this review observed 18611, 19546, 1124, 1470, 481, 2941, 454 passed.
中文说明
未探索到全部深度（达到工具调用预算）：PR #8773 (QwenLM/qwen-code) adds a reverseAuditRounds c...：run the three updated test files (budget.test.ts, retirement.test.ts, agent-prompt.test.ts) to confirm they pass — node_modules absent in the worktree and paren…。
Test Plan（非阻断）：302 passed — this review observed 18611, 19546, 1124, 1470, 481, 2941, 454 passed; 2281 passed — this review observed 18611, 19546, 1124, 1470, 481, 2941, 454 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
Refocuses this PR from the micro-diff cap (which the review showed was mis-built: micro diffs run 3A, but the cap-1 machinery lived on the 3B path they never take, and its threshold never fired on the motivating case) onto the huge end, which is where the six-hour timeouts actually are. A timeout survey found 26 review-pr jobs dying in one recent window — ~122 hours of compute, zero posted, several the same PR retried. The wall clock is model inference (82-88% inside subagents, ~81% of that model turns), so on a 4,000-5,300-line PR the driver is sheer volume: dozens of agents reading the diff, then a reverse-audit loop whose every round re-reads it against a growing findings list (~90 min a round; five rounds alone exceed the ceiling). The elastic budget answers this in the band where the review otherwise posts nothing: reverseAuditRounds drops 5 -> 3 for a huge diff (effective >= 3000; three is the smallest loop two-consecutive-dry still converges), and specialistCap sheds Agent 8 to 0 there (a whole-diff pass on top of the base fan-out is the marginal cost that tips a too-big review over the wall). Neither drops a required dimension. The ROUND CAP refusal now writes a marker compose-review caps the verdict on, so a non-converged stop discloses like a budget stop rather than resting on the orchestrator's relay. Removing the cap-1 tier resolves the review's two Criticals and collapses the retirement scheduler back to its clean two-dry logic; the duplicated plan-cap read is gone (agent-prompt reads the parsed report, retirement no longer needs the cap at all). Tests pin the huge/normal boundary, the effective-vs-source split, round-past-cap enforcement at both 3 and 5, and the marker round-trip.
wenshao
changed the title
- Drop the always-plural ternary in the retirement note: dryRounds is a two-tuple again, so the singular branch is dead prose. - SKILL.md's merge bullet: the loop's bound is the plan's round cap (5, or 3 for a huge diff), not a flat 5-round cap. - Re-wrap the overlong deadline.ts header line.
wenshao
added a commit
that referenced
this pull request
Aug 9, 2026
The afterAll deletes several 16k-entry fixture trees — tens of thousands of unlinks — and blew past vitest's default 10s hook timeout on a loaded CI runner (PR #8773's second Test (ubuntu-latest) run), failing a suite whose 59 tests had all passed. Same CI-load flake class as the extract test fix in this branch.
wenshao
added a commit
that referenced
this pull request
Aug 9, 2026
…ar I/O Each register/complete in the retention-cap loop tests also writes the status sidecar via atomicWriteFileSync, and loaded CI runners have been measured at ~700ms per sidecar write — the ~70 writes of the longest loop take ~50s, past vitest's 15s default, failing four tests whose assertions are pure eviction semantics (seen twice on PR #8773, on two different runners). Give them an explicit 120s timeout; no assertion changes.
pull Bot
pushed a commit
to AmirulAndalib/qwen-code
that referenced
this pull request
Aug 9, 2026
* fix(core): deflake auto-memory extract tests under CI load waitForMockCall polled ten zero-delay event-loop turns and gave up. The mock it waits for fires after real async work (index rebuilds, cursor I/O), so on a loaded CI runner the poll spun through its ten turns without waiting any wall-clock time and the two rebuild-isolation tests failed with 'Expected mock to be called' (seen on PR QwenLM#8773's Test (ubuntu-latest) job). Wait against a 2s deadline instead; the fast path still returns on the first check. * fix(cli): give the manifest-context fixture teardown a real timeout The afterAll deletes several 16k-entry fixture trees — tens of thousands of unlinks — and blew past vitest's default 10s hook timeout on a loaded CI runner (PR QwenLM#8773's second Test (ubuntu-latest) run), failing a suite whose 59 tests had all passed. Same CI-load flake class as the extract test fix in this branch. * fix(core): let the shell-registry retention tests pay for their sidecar I/O Each register/complete in the retention-cap loop tests also writes the status sidecar via atomicWriteFileSync, and loaded CI runners have been measured at ~700ms per sidecar write — the ~70 writes of the longest loop take ~50s, past vitest's 15s default, failing four tests whose assertions are pure eviction semantics (seen twice on PR QwenLM#8773, on two different runners). Give them an explicit 120s timeout; no assertion changes.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: reverse audit — stopped before round 5 by the review time budget. Test Plan (not a blocker): 2287 passed — this review observed 18683, 19547, 1124, 1480, 481, 2941, 454 passed; 5 passed — this review observed 18683, 19547, 1124, 1480, 481, 2941, 454 passed.
中文说明
已审查。 建议见行内评论。 2 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未审查：反向审计——评审时间预算不足，未能开始第 5 轮。 Test Plan（非阻断）：2287 passed — this review observed 18683, 19547, 1124, 1480, 481, 2941, 454 passed; 5 passed — this review observed 18683, 19547, 1124, 1480, 481, 2941, 454 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
Collaborator
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not explored to full depth (tool budget reached): PR #8773 refocuses the review pipeline's reverse-audit ro...: (none — all planned checks completed). Not reviewed: reverse audit — stopped before round 4 by the review time budget. Test Plan (not a blocker): 2287 passed — this review observed 18750, 19554, 1172, 1494, 481, 2974, 459 passed; 5 passed — this review observed 18750, 19554, 1172, 1494, 481, 2974, 459 passed.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未探索到全部深度（达到工具调用预算）：PR #8773 refocuses the review pipeline's reverse-audit ro...：(none — all planned checks completed)。 未审查：反向审计——评审时间预算不足，未能开始第 4 轮。 Test Plan（非阻断）：2287 passed — this review observed 18750, 19554, 1172, 1494, 481, 2974, 459 passed; 5 passed — this review observed 18750, 19554, 1172, 1494, 481, 2974, 459 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
added 2 commits
August 10, 2026 11:22
…cro-cap # Conflicts: # packages/cli/src/commands/review/agent-prompt.ts # packages/cli/src/commands/review/lib/deadline.test.ts # packages/core/src/skills/bundled/review/DESIGN.md
- Floor `reverseAuditRoundCap` at the huge-diff cap of 3: an out-of-band 1 or 2 reads as the full cap, never fewer rounds. - Delete the dead `REVERSE_AUDIT_MAX_ROUNDS` re-export (no consumers) and point the retirement cold-check comment at the plan cap. - Correct the cap-3 rationale everywhere it was mechanically wrong: three is one audit round above the convergence floor of two (the all-dry rounds-1-and-2 shape converges under any cap of two or more, since the convergence check runs before the cap gate), not the smallest converging loop — in budget.ts, budget.test.ts, DESIGN.md and SKILL.md. - Define `effective` in the SKILL.md budget bullet and make the round-5 narration cap-agnostic; scope the retroactive-dry example to the cap-5 shape. - Add tests for the cap-3 retirement certificate, the per-chunk and chunkless round-cap gates, the undefined-round marker, and the specialistCap effective-vs-src dependence.
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2421 passed · 0 failed · 2421 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2421 通过 · 0 失败 · 2421 总计 Verification reportPR #8773 Deep Verification — perf(review): cap the reverse audit and shed Agent 8 on a huge diffVerdict: 中文摘要结论:
Central claim and A/B proofCentral claim: on a huge diff (effective ≥ 3000 lines) the reverse-audit round cap drops 5 → 3 and Agent 8's Secondary claims: (1) out-of-band/absent plan caps read as the full cap — never fewer rounds; (2) the marker round-trips ( A/B cells — budget derivation (
|
| input (src, total) | effective | base specialistCap |
head specialistCap |
base reverseAuditRounds |
head reverseAuditRounds |
|---|---|---|---|---|---|
| (2999, 2999) | 2999 | 2 | 2 | absent | 5 |
| (3000, 3000) | 3000 | 2 | 0 ⬅ flip | absent | 3 ⬅ flip |
| (100, 30000) lockfile | 3750 | 2 | 0 ⬅ flip | absent | 3 ⬅ flip |
| (4000, 5300) timeout band | 4000 | 2 | 0 ⬅ flip | absent | 3 ⬅ flip |
| (100, 100) / (80, 80) / (79, 79) | < 3000 | parity | parity | absent | 5 |
Fallback matrix (head only): undefined, {}, 0, 1, 2, 6, 2.5, '1', -3, NaN, null → all read as 5; 3→3, 4→4, 5→5. Out-of-band low values 1/2 are floored up — "never less auditing". 53/53 scripted checks. Witness: 01-budget-ab-head-dist-vs-base.png.
A/B cells — admission gate (real agentPromptCommand.handler, real files, no mocks; harness harness/ab-gate.mjs)
| scenario | base | head |
|---|---|---|
huge plan, rounds 1–3 (--all-chunks) |
builds, 3 records/round | builds, 3 records/round (unchanged) |
huge plan, round 4 --all-chunks |
builds (3 records, no marker) | refused: exit 4, ROUND CAP: … round cap is 3, marker {cause:"round-cap", cap:3, round:4}, 0 records, no stamp |
huge plan, round 4 --chunk 13 |
builds (1 record) | refused: exit 4 + marker |
| huge plan, round 4 chunkless (3A) | builds (1078-B prompt) | refused: exit 4 + marker |
| normal plan, rounds 1–5 | builds | builds (unchanged) |
| normal plan, round 6 | builds | refused: exit 4, round cap is 5 + marker |
head: plan without budget, round 6 |
— | refused at fallback cap 5 |
head: hand-edited reverseAuditRounds: 1, round 2 |
— | builds (1 floored to 5) |
| head: huge plan, all-dry rounds 1+2, round-3 build | — | CONVERGED exit 5, nothing built, no marker (convergence outranks the cap) |
4 cells flip from "builds" to "refused + marker"; parity cells prove nothing below the cap moved. 24/24 head checks, 8/8 base checks. Witnesses: 02-gate-ab-head.png, 03-gate-ab-base.png. The shipped dist reproduces both refusals (dist-parity.mjs, 3/3): 05-dist-parity.png.
The marker's downstream consumer was traced: compose-review.ts dispatches on stop.cause === 'round-cap' (capped verdict + dedup pinned by its test, killed by mutant M7); the only other reader, coverage.ts:1444, uses the cause-agnostic boolean readBudgetStop(...) !== null, whose "rebuild-the-round remediation is futile" reasoning holds identically for a round-cap stop.
Corrections (to the PR description, not the code)
- "Three is the smallest loop the two-consecutive-dry convergence rule still satisfies (dry in rounds 2 and 3)" is mechanically wrong — and the PR's own final commit already corrected it everywhere in-tree. Measured with the real handler (
harness/probe-convergence-shapes.mjs, log06-convergence-shapes.log): under cap 3, all-dry rounds 1+2 reachesCONVERGEDexit 5 at the round-3 build (convergence check runs before the cap gate — the corrected in-tree rationale), but dry rounds 2+3 (round 1 yielded) reaches the round-4 build where every retired chunk is due a cold check (cold checks land on even rounds) → not converged →ROUND CAPexit 4 + "did not converge" marker. The smallest cap that shape converges under is 4 (round-4 cold check runs, round-5 build converges before the gate — both measured). The code, budget.ts comments, DESIGN.md, SKILL.md and tests carry the corrected wording ("one audit round above the convergence floor of two … not the smallest converging loop"); only the PR description and its 中文摘要 still carry the superseded sentence. Behaviorally the stop is conservative and disclosed (caps a would-be clean verdict, never silent), matching the in-tree "post what you proved" tradeoff — so this is a description correction, not a code finding.
Findings
- Nit — stale "(hard cap 5)" prose in DESIGN.md's call-budget section.
packages/core/src/skills/bundled/review/DESIGN.md:597(Large diffs paragraph): "the stop rule needs two consecutive dry rounds (hard cap 5)" — for the huge sub-band (effective ≥ 3000) the cap is now 3, and the example arithmetic19 × (2..5)auditors is cap-5-only. The small-diff table row (line 594, "5-round hard cap") is still correct (small diffs top out at effective ≤ 500 and can never reach the huge tier). DESIGN.md is background reading — the runtime brief (SKILL.md) is fully updated and the SKILL gate passes — so this is docs drift at nit severity. Suggested wording (not applied): "(the plan'sreverseAuditRoundscap — 5, or 3 for a huge diff)".
No other findings. Specifically checked and not problematic:
- The round-cap gate fires before the deadline gate inside
admitReverseAuditRound— a CI run hitting both gets the more specific ROUND CAP disclosure; both are verdict-capping stops, so no observability is lost. writeRoundCapStopandwriteBudgetStopsharebudget-stop.json(last-writer-wins). A single run can only trip one stop per admission, and both causes cap the verdict with an honest disclosure — no silent-loss variant.- The removed
REVERSE_AUDIT_MAX_ROUNDSexport fromretirement.tshas zero remaining consumers (repo-wide grep; typecheck clean). - Huge ⇒ always 3B topology (effective ≥ 3000 requires src ≥ 3000 or total ≥ 24000, both beyond 3A's ≤500-src/≤3200-total bounds) — the PR's "the huge end is on the sound path" holds.
Mutation matrix (vacuity proof for the new tests)
Unmutated control green (532/532 across the five PR test files). Every mutant applied as an exact single-point edit, suite re-run, then restored via git checkout. Witness: 04-mutation-matrix.png, log 05-mutation-matrix.log.
| mutant | guard | result (failing tests) |
|---|---|---|
M0 SWEEP_FLOOR 25→2500 |
positive control (pre-existing test) | KILLED — 2 failed |
M1 rounds keyed on src instead of effective |
effective-vs-src split | KILLED — "keys on effective lines…" |
M2 specialistCap shed keyed on src |
effective-vs-src split | KILLED — "shed on a huge non-source diff…" |
M3 boundary >= → > |
3000 is already huge | KILLED — boundary test |
M4 cap floor removed (v >= 1) |
1/2 read as full cap | KILLED — fallback-matrix test |
M5 gate condition → if (false) |
the whole cap gate | KILLED — all 4 refusal tests |
M6 cold-check note hardcodes > 5 |
plan-cap-aware certificate | KILLED — "certificate final" test |
M7 compose ignores cause |
marker → verdict capping | KILLED — round-cap marker test |
M8 marker written without cause |
marker round-trip | KILLED — 5 tests (marker + gate + compose) |
9/9 killed; no survivors. The reverted central case fails the intended assertion with the behavioral mismatch quoted: AssertionError: expected undefined to be 4 (exit code absent when the gate is disabled), then the restored tree goes green again.
Targeted gates
| gate | result |
|---|---|
Full review suite, packages/cli (vitest run src/commands/review) |
65 files, 2316 passed, 4 skipped, 0 failed at the verified head (the author's plan cites 64 files / 2287 passed as of their run; my path filter also picks up the command-level src/commands/review.test.ts that sits just outside the review/ directory — 64 directory files + that one) |
packages/core SKILL gate (SKILL.test.ts, incl. the new "The six-hour timeouts" DESIGN anchor) |
5/5 passed |
tsc --noEmit in packages/cli |
clean; liveness: 131 review files listed in the program (--listFilesOnly) |
| ESLint on the five changed source files | clean; liveness: a planted unused-variable file was flagged, then removed |
| Shipped dist freshness | dist/src/commands/review/lib/budget.js contains the new tier logic; behavioral parity via dist-parity.mjs |
Not covered
- The motivating statistics are premises, not verifiable here: "26 review-pr jobs timed out / ~122 hours / zero posted" and "~90 min a round" come from a CI survey; this container has no GitHub token or CI history. They size the threshold choice (3000 with margin below the 4000–5300 timeout band) but the code's behavior does not depend on their exact values.
- Per-commit attribution: the checkout is depth 2;
git rev-list HEAD^1..HEAD^2sees only the head commit while the metadata snapshot lists 8 — the aggregateHEAD^1..HEADdiff is what was verified. - A real huge-PR end-to-end review run (would require live model calls). The wire shape of the cap stop (exit code, stderr contract, marker, compose disclosure) is proven; the model-side orchestration that reads SKILL.md's updated cap bullet is out of reach in-container. This reproduces the shape, not the full orchestrated loop.
- Repo-wide test suites beyond the affected workspaces; Windows/macOS behavior (pure Node file/JSON logic, no platform-specific paths).
probe-convergence-shapes.mjsprints observed outcomes; it carries no scripted pass/fail of its own and is counted as evidence, not assertions.
Methodology
Environment: the CI verify container (node:22-bookworm), merge-ref checkout at depth 2; npm ci + npm run build pre-run at HEAD. A/B base side: scratch worktree at HEAD^1 (856b793c15) under tmp/, root + workspace node_modules symlinked in (PR touches no manifest, so the tree is a clean dependency control); realpath of the only internal link asserted (node_modules/@qwen-code/qwen-code-core → head packages/core, whose code is unchanged by this PR — core diff is two .md files — and none of the changed modules import core). Both arms drove real source through tsx with an identical harness; the head arm was additionally tied to the shipped artifact by running the same scenarios against packages/cli/dist and by harness 1 importing the dist budget.js directly. Gate A/B drove the real agentPromptCommand.handler with real plan/findings/transcript files in temp dirs — the only interception was capturing process.stdout/stderr.write; no code under test was stubbed. Mutants were exact string replacements verified to match once, restored with git checkout after each run (working tree confirmed clean at the end; base worktree removed). Raw logs: logs/01-ab-budget.log, 02/03-ab-gate-{head,base}.log, 04-dist-parity.log, 05-mutation-matrix.log, 06-convergence-shapes.log; harnesses rerunnable from harness/. Evidence captures re-ran each harness live under scripts/verify-capture.mjs.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Collaborator
|
Confidence: 4/5 — clean across every stage; the one thing nobody can prove before merge is the wall-clock outcome itself, and the sandboxed A/B running alongside this comment is closing what of it can be closed. Stepping back: the problem is real and expensive (122 hours of compute in one CI window posting nothing), the fix lands exactly where the harm was measured, and the PR got better under pressure — the round-1 Criticals didn't get patched, they got dissolved by deleting the mis-built tier and refocusing on the end of the size band where the timeouts live. Three rounds of adversarial review each left verifiable fixes in the final diff: I spot-checked the R3-1 and R3-3 corrections against the code (the convergence-before-gate ordering is real, and the cap-agnostic narration holds under cap 3), and every safety property degrades the right way — old or garbled plans audit more, never less; a non-converged stop discloses like a budget stop instead of silently passing. My independent proposal and the PR's shape are the same shape; where it goes beyond mine (the per-gate mutant-killing tests, the undefined-round marker pin) it earns the lines. If I were maintaining this in six months, the rationale comments — now mechanically accurate — are exactly what I'd want beside the constants. The named reservation, the reason this is a 4 and not a 5: no huge-diff review has been observed finishing under the new cap yet. The mechanism is fully pinned by tests and CI is green on the head; the in-flight 中文说明退一步看:问题真实且代价高昂(一个 CI 窗口 122 小时算力零产出),修复恰好落在实测损害处,而且这个 PR 在压力下变得更好 —— 第 1 轮的两个 Critical 没有被打补丁,而是通过删除建错的档位、聚焦到超时真正所在的尺寸带而消解。三轮对抗性评审各自在最终 diff 里留下可核对的修复:我抽查了 R3-1 与 R3-3 的修正(收敛先于门的顺序真实存在,与 cap 无关的叙述在 cap 3 下成立),每个安全性质都朝正确方向降级 —— 旧 plan 或损坏值审计更多、从不少审;非收敛停止像预算停止一样披露,而非悄悄放行。 我独立的设想与 PR 形态一致;它超出我设想之处(逐门的杀变异测试、undefined-round 标记钉)物有所值。若六个月后由我维护,常量旁这些机械上准确的理由注释正是我想要的。 保留意见,也是 4 而非 5 的原因:尚无一次巨型 diff 评审被观察到在新上限下跑完。机制已由测试完全钉住、CI 在该提交上为绿;运行中的 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
approved these changes
Aug 10, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
approved these changes
Aug 10, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Verified at head 805a37b: the band classification is sound (effective = max(src, floor(total/8)) ≥ 3000, boundary pinned at 2999→5 / 3000→3), and huge ⇒ 3B is now structurally guaranteed — effective ≥ 3000 implies srcDiffLines ≥ 3000 or diffLines ≥ 24000, either of which violates the 3A topology gate, so the round-1 R1-1 failure mode (cap machinery on a path the plan never takes) is arithmetically impossible. The cap gate is convergence-ordered on all three call sites (chunkless, --chunk, --all-chunks), the round-cap refusal writes a persistent marker that compose-review caps the verdict on like a budget stop, and neither knob drops a required dimension (reverse audit still runs; Agent 8 was never required). All 14 round-1 findings plus the 8 round-2 suggestions are genuinely resolved by the re-scope and 805a37b, verified against head sources. Only P3s remain: a stale docstring sentence claiming retirement.ts re-exports MAX_REVERSE_AUDIT_ROUNDS (the re-export was deleted), a Zh terminology nit in deadline.ts reasonZh, and two small test gaps (cap-3 all-dry CONVERGED shape; chunkless rounds ≤ cap building) — low risk since the convergence check provably precedes the cap gate and never reads the cap. CI green on this head and the bot already approved. Nothing blocks merge.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not explored to full depth (tool budget reached): 本 PR 将评审管线对巨型 diff(effective ≥ 3000 行)的反审轮数上限降为 3(常规为 5)、...: (none — all planned checks completed). Test Plan (not a blocker): 2287 passed — this review observed 19557, 1172, 1501, 481, 2983, 459 passed; 5 passed — this review observed 19557, 1172, 1501, 481, 2983, 459 passed.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未探索到全部深度（达到工具调用预算）：本 PR 将评审管线对巨型 diff(effective ≥ 3000 行)的反审轮数上限降为 3(常规为 5)、...：(none — all planned checks completed)。 Test Plan（非阻断）：2287 passed — this review observed 19557, 1172, 1501, 481, 2983, 459 passed; 5 passed — this review observed 19557, 1172, 1501, 481, 2983, 459 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
Collaborator
|
Released in v0.21.9. |
pull Bot
pushed a commit
to bit-cook/qwen-code
that referenced
this pull request
Aug 11, 2026
QwenLM#8850) * fix(review): correct the round-cap marker lifecycle and stale cap docs Follow-up to QwenLM#8773. The round-cap stop marker shared budget-stop.json with the time-budget stop but did not share its lifecycle rules, and the huge-diff cap left several docs describing the old 5-round-only world. Marker lifecycle: - First refusal wins. Both writers now skip when a same-run marker already exists, so a time-budget stop followed by a retry the cap refuses no longer overwrites the marker and posts two contradictory stop disclosures. - A CONVERGED exit clears any same-run marker. A round-cap refusal followed by a converged round (the convergence check runs before the cap gate) left a stale marker that capped a legitimately-converged verdict; the converged branch now unlinks it. - coverage.ts's reverseByDesign exemption is now cause-aware. It suppressed the not-built gap for any marker, but a round-cap stop's fix (rebuild --round 1) is admitted — so a round-cap marker keeps the gap and its rebuild remediation. Docs and tests: - Drop the stale "retirement.ts re-exports it" and "the retirement scheduler" reader claims in budget.ts; note the huge-diff 3-round cap in DESIGN.md's LLM-call-budget paragraph and table. - Mirror the time-budget stop's bounded-tail protocol into the ROUND CAP message and SKILL.md bullet (verify only via --role verify, bound the wait at the compose floor, no fresh re-verification pass). - Pin the spelled cap number in the retirement note, and pin that a converged past-cap round exits 5 rather than refusing at the cap. * test(review): pin the round-cap tail protocol and the per-chunk converged clear * fix(review): recall relayed stop entries on a converged exit and align both refusal tails Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(review): pin clearBudgetStop directly and the round-cap wait-bound clauses --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Caps the reverse-audit loop and sheds Agent 8 for a huge diff (effective ≥ 3000 lines) — the size band where a high review currently times out to zero posted output.
reverseAuditRoundsin the plan budget: 5 normally, 3 for a huge diff. Three is the smallest loop the two-consecutive-dry convergence rule still satisfies (dry in rounds 2 and 3).specialistCap: sheds Agent 8 to 0 in the same huge band — a whole-diff specialist pass on top of the base fan-out is the marginal cost that tips a too-big review over the wall, while the per-chunk fan-out already covers the ground.agent-promptbuilder enforces the cap: a round past it gets aROUND CAP:refusal (exit 4) that writes a markercompose-reviewcaps the verdict on, so a non-converged stop discloses like a budget stop rather than resting on the orchestrator's relay.Neither knob drops a required dimension: the reverse audit still runs, and Agent 8 was never a required agent.
Why the scope changed
This PR began as a micro-diff cap (1 round below the sweep floor). The round-1 review (thanks) showed that half was mis-built: micro diffs run the 3A topology, but the cap-1 machinery lived on the 3B
--all-chunkspath they never take — and worse, the 3A convergence pair's round-2 build hit the ROUND CAP gate and would have been refused before any round ran (R1-1); and the threshold, keyed on diff-plan line-spans, never fired on the motivating 23-line case (R1-2). The huge end is on the sound path (huge diffs genuinely are 3B) and is where the timeouts are, so the micro tier is removed and this PR refocuses there. Removing cap-1 resolves both Criticals and collapses the retirement scheduler back to its clean two-dry logic.Why it's needed
A survey of one recent CI window: 26 review-pr jobs timed out — ~122 hours of compute, zero posted, several the same PR retried and re-timed-out. The wall clock is model inference (82–88% inside subagents; ~81% of subagent time is model turns), so on a 4,000–5,300-line PR the driver is volume: dozens of agents reading the diff, then a reverse-audit loop whose every round re-reads it against a growing findings list (~90 min a round; five rounds alone exceed the six-hour ceiling). In CI the deadline gate already refuses a round that won't fit; this static cap is the belt it works under and the only bound a local run has.
Reviewer Test Plan
cd packages/cli && npx vitest run src/commands/review/lib/budget.test.ts src/commands/review/lib/retirement.test.ts src/commands/review/agent-prompt.test.ts src/commands/review/lib/deadline.test.ts src/commands/review/compose-review.test.ts— pins the huge/normal boundary (2999→5, 3000→3), theeffective-vs-srcsplit (a 30k-line lockfile diff caps too), Agent-8 shedding, round-past-cap enforcement at both 3 and 5, and the round-cap marker round-trip + compose-review capping on it. Full review suite: 64 files, 2287 passed.packages/coreSKILL gate: 5 passed.Tested on
Risk & Scope
Linked Issues
Follow-up to the /review latency line (#8642, #8772); the huge-diff timeouts measured in the timeout survey.
中文说明
这个 PR 做了什么
对巨型 diff(effective ≥ 3000 行)封顶反审轮数并卸载 Agent 8 —— 正是 high 评审当前超时到零产出的尺寸带。
reverseAuditRounds:常规 5,巨型 3(3 是两连干燥收敛仍能满足的最小循环);specialistCap:巨型带卸到 0(base 扇出已覆盖,额外的全 diff 专家 pass 正是把跑不完的评审推过墙的边际成本)。构建器强制上限:超过则ROUND CAP:拒绝(exit 4)并写标记供 compose-review 封顶,非收敛停止像预算停表一样披露,不依赖 orchestrator 转述。两个旋钮都不砍必需维度。范围为何变化
本 PR 起初是微型 diff 封顶(sweep 地板下 1 轮)。首轮评审证明那一半建错了路:微型 diff 走 3A,而 cap-1 机制在它们从不走的 3B 路上,且 3A 收敛对的第 2 轮会被 ROUND CAP 门在任何轮次运行前拒绝(R1-1);阈值按 diff-plan 行区间计,在动机的 23 行案例上从不触发(R1-2)。大端在正确的路上、也正是超时所在,故移除微型档、聚焦大端。移除 cap-1 使两个 Critical 消解,退休调度器回到干净的两连干燥逻辑。
为什么需要
近期一个 CI 窗口调查:26 个 review-pr 超时 —— 约 122 小时算力,0 条发布,多个是同一 PR 反复重试再超时。墙钟是模型推理(82–88% 在子 agent 内,其中 ~81% 是模型轮次),4000–5300 行 PR 的驱动是体量:几十个 agent 读 diff,反审每轮对着增长的发现清单重读整个 diff(~90 分钟/轮;仅 5 轮就超 6 小时上限)。CI 中截止门已反应式拒绝塞不下的轮次;此静态上限是它的兜底,也是本地运行唯一的界。
风险与范围