diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 87f176cd70e..92a64500415 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -3424,6 +3424,153 @@ describe('per-chunk retirement — cold territories stop costing a round', () => expect(out).not.toContain('retirement:'); }); + it('huge cap: a chunk dry in rounds 1 and 2 retires with a final certificate', () => { + // Under the reduced 3-round cap, chunk 13's next cold check (round 4) is + // past the cap, so the retirement note must read `certificate final`, not + // `next cold check round 4` — the same builder's admission gate refuses a + // round-4 build. Pins the plan-cap comparison (`nextColdCheck > + // planRoundCap`) at cap 3; the only other cap-3 test keeps every chunk + // yielding, so nothing retires there. + writeFileSync( + plan, + JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 3 } }), + ); + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + answerRound(1, { 13: DRY, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: DRY, 14: YIELD, 15: YIELD }); + const out = runRound(3); + + expect(process.exitCode).toBeUndefined(); + expect(out).toContain('2 auditors required this round'); + expect(out).toContain('chunk 13 — retired: dry in rounds 1 and 2'); + expect(out).toContain('certificate final'); + expect(out).not.toContain('next cold check round 4'); + }); + + it('huge cap: a non-converging loop is refused past the reduced 3-round cap', () => { + // A huge diff caps at 3 rounds. Rounds 1-3 never converge (every chunk + // keeps yielding), so round 4 is refused at the cap: exit 4, nothing + // built, and — the robustness half — a marker compose-review caps on, + // so the verdict is capped whether or not the orchestrator relays. + writeFileSync( + plan, + JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 3 } }), + ); + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + answerRound(1, { 13: YIELD, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: YIELD, 14: YIELD, 15: YIELD }); + answerRound(3, { 13: YIELD, 14: YIELD, 15: YIELD }); + const out = runRound(4); + + expect(process.exitCode).toBe(4); + expect(out).toBe(''); + expect(keysOf(4)).toHaveLength(0); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('ROUND CAP'); + expect(msg).toContain('round cap is 3'); + // The marker is on disk so compose-review caps without the relay. + expect(readBudgetStop(plan)?.cause).toBe('round-cap'); + expect(readBudgetStop(plan)?.cap).toBe(3); + }); + + it('huge cap: a --chunk build past the cap is refused too — the per-chunk gate', () => { + // The round-cap gate must fire on the per-chunk call site, not only + // through --all-chunks: a huge-diff review whose rounds are built or + // repaired per chunk would otherwise admit round 4+ against the cap and + // run ~90-minute rounds in the exact timeout band this cap sheds. Rounds + // 1-3 are built (non-converging), then a `--chunk 13 --round 4` build — + // an unadmitted round, so its first chunk build IS the round's admission + // — must be refused at the cap, writing the round-cap marker. + writeFileSync( + plan, + JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 3 } }), + ); + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + answerRound(1, { 13: YIELD, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: YIELD, 14: YIELD, 15: YIELD }); + answerRound(3, { 13: YIELD, 14: YIELD, 15: YIELD }); + + (writeStdoutLine as unknown as Mock).mockClear(); + (writeStderrLine as unknown as Mock).mockClear(); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + findings, + chunk: 13, + round: 4, + }); + + expect(process.exitCode).toBe(4); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); + expect(keysOf(4)).toHaveLength(0); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('ROUND CAP'); + expect(msg).toContain('round cap is 3'); + expect(readBudgetStop(plan)?.cause).toBe('round-cap'); + expect(readBudgetStop(plan)?.cap).toBe(3); + // The refusal precedes admission — no round-4 stamp is left behind. + expect(readRoundStamps(plan).some((s) => s.round === 4)).toBe(false); + }); + + it('huge cap: a chunkless single build past the cap is refused too — the 3A gate', () => { + // The chunkless whole-diff gate (Step 5's 3A single auditor) is the third + // call site the cap passes through. No history is needed — round 4 > cap + // 3 alone refuses it, exit 4 with the round-cap marker. + writeFileSync( + plan, + JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 3 } }), + ); + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + + (writeStdoutLine as unknown as Mock).mockClear(); + (writeStderrLine as unknown as Mock).mockClear(); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + findings, + round: 4, + }); + + expect(process.exitCode).toBe(4); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); + expect(readRecordedPrompts(plan).size).toBe(0); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('ROUND CAP'); + expect(msg).toContain('round cap is 3'); + expect(readBudgetStop(plan)?.cause).toBe('round-cap'); + expect(readBudgetStop(plan)?.cap).toBe(3); + }); + + it('the default 5-round cap is enforced by the builder, not just prose', () => { + // Pins the general ROUND CAP enforcement: the mutation `round > cap` + // → `round > cap && cap === 1` (a sixth round builds) fails here. + answerRound(1, { 13: YIELD, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: YIELD, 14: YIELD, 15: YIELD }); + answerRound(3, { 13: YIELD, 14: YIELD, 15: YIELD }); + answerRound(4, { 13: YIELD, 14: YIELD, 15: YIELD }); + answerRound(5, { 13: YIELD, 14: YIELD, 15: YIELD }); + const out = runRound(6); + + expect(process.exitCode).toBe(4); + expect(out).toBe(''); + expect(keysOf(6)).toHaveLength(0); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('ROUND CAP'); + expect(msg).toContain('round cap is 5'); + }); + it('all retired and none due: exit 5, CONVERGED, nothing built, nothing stamped', () => { answerRound(1, { 13: DRY, 14: DRY, 15: DRY }); answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index dc4c1d214b9..75ad5f14b91 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -42,16 +42,18 @@ import { createHash } from 'node:crypto'; import { readFileSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; -import { launchToolBudget } from './lib/budget.js'; +import { launchToolBudget, reverseAuditRoundCap } from './lib/budget.js'; import { expectedRoundSeconds, readRoundStamps, reverseAuditBudgetExhausted, reverseAuditBudgetMessage, + roundCapStopEntry, stampRound, verifyBudgetExhausted, verifyBudgetMessage, writeBudgetStop, + writeRoundCapStop, } from './lib/deadline.js'; import { READ_FILE_CHAR_CAP, @@ -64,7 +66,6 @@ import { writeFindingsFile, } from './lib/prompt-record.js'; import { - REVERSE_AUDIT_MAX_ROUNDS, scheduleReverseAuditRound, type RoundSchedule, } from './lib/retirement.js'; @@ -1865,7 +1866,31 @@ function requireAuditableChunks(report: PlanReport): DiffChunk[] { function admitReverseAuditRound( planPath: string, round: number | undefined, + cap: number, ): boolean { + // The plan's round cap first: deterministic, and cheaper than the + // deadline arithmetic. The full cap normally; a reduced cap for a huge + // diff, where a single reverse-audit round is ~90 minutes and the full + // loop cannot finish (measured: the 6-hour CI reviews that posted nothing + // were 4,000-5,300-line PRs). A round past the cap writes a marker so + // `compose-review` caps the verdict whether or not the orchestrator + // relays the entry — the round cap runs its full allotted rounds, so a + // non-converged stop here is real unaudited scope, exactly as the budget + // stop is. + if (typeof round === 'number' && round > cap) { + writeRoundCapStop(planPath, cap, round); + writeStderrLine( + `ROUND CAP: the plan's reverse-audit round cap is ${cap}, and round ` + + `${round} is past it. This is a termination rule, not an error — do ` + + `not rebuild or retry. A marker has been recorded and compose-review ` + + `will cap the verdict; still add \`${roundCapStopEntry(cap)}\` to ` + + `unreviewedDimensions so the terminal report agrees. If the cap ` + + `round reported findings whose verdicts have not landed, launch ` + + `their verifiers alone and wait, then proceed to Step 6.`, + ); + process.exitCode = 4; + return false; + } const spent = reverseAuditBudgetExhausted( process.env, expectedRoundSeconds(planPath, round), @@ -1915,7 +1940,14 @@ function runAllChunks( // three yielded in most: the loop earns its keep in the hot territories, // and the cold ones were a third of its bill. let schedule: RoundSchedule | null = null; - if (role === 'reverse-audit' && round !== undefined && round >= 3) { + // Retirement needs two consecutive dry audits, so nothing retires before + // round 3 (the scheduler's own guard says the same). + const retirementReadsFrom = 3; + if ( + role === 'reverse-audit' && + round !== undefined && + round >= retirementReadsFrom + ) { try { schedule = scheduleReverseAuditRound( planPath, @@ -1950,7 +1982,14 @@ function runAllChunks( // that builds is an admission and stamps like any other; the converged // round above built nothing and stamps nothing; a build that throws // leaves no stamp for the next round's gate to misprice. - if (role === 'reverse-audit' && !admitReverseAuditRound(planPath, round)) { + if ( + role === 'reverse-audit' && + !admitReverseAuditRound( + planPath, + round, + reverseAuditRoundCap(report.budget), + ) + ) { return; } @@ -1999,6 +2038,7 @@ function runAllChunks( : `one per chunk still under audit (${skipped.length} retired ` + `chunk(s) skipped; the retirement note after the end-of-round line ` + `says which — relay it to the terminal)`; + const planRoundCap = reverseAuditRoundCap(report.budget); const retirementNote = skipped.length === 0 ? [] @@ -2011,10 +2051,10 @@ function runAllChunks( .map( (s) => `chunk ${s.chunkId} — retired: dry in rounds ` + - `${s.dryRounds[0]} and ${s.dryRounds[1]}, ` + - (s.nextColdCheck > REVERSE_AUDIT_MAX_ROUNDS + `${s.dryRounds.join(' and ')}, ` + + (s.nextColdCheck > planRoundCap ? `certificate final — the ` + - `${REVERSE_AUDIT_MAX_ROUNDS}-round hard cap leaves ` + + `${planRoundCap}-round cap leaves ` + `the loop no round for a cold check` : `next cold check round ${s.nextColdCheck}`), ) @@ -2356,7 +2396,11 @@ function runAgentPrompt(args: AgentPromptArgs): void { args.role === 'reverse-audit' && !hasChunk && !args.allChunks && - !admitReverseAuditRound(args.plan, args.round) + !admitReverseAuditRound( + args.plan, + args.round, + reverseAuditRoundCap(report.budget), + ) ) { return; } @@ -2430,7 +2474,14 @@ function runAgentPrompt(args: AgentPromptArgs): void { return; } } - if (!admitReverseAuditRound(args.plan, args.round)) return; + if ( + !admitReverseAuditRound( + args.plan, + args.round, + reverseAuditRoundCap(report.budget), + ) + ) + return; } if (args.allChunks && args.role && findingsContent !== undefined) { @@ -2526,10 +2577,11 @@ export const agentPromptCommand: CommandModule = { "Build a review agent's launch prompt from the plan (the diff path, its line " + "ranges and the agent's own brief are welded in, not left to the caller to " + 'remember). Exit codes: 0 built; 4 a build was refused — the review time ' + - 'budget refused another reverse-audit round (BUDGET line on stderr), or ' + - 'the compose floor refused a verifier so compose/submit still fit ' + - '(VERIFY BUDGET line) — both termination rules, not errors: stop and ' + - 'compose, do not retry; 5 the reverse audit CONVERGED — every chunk holds two ' + + 'budget refused another reverse-audit round (BUDGET line on stderr), the ' + + "plan's round cap refused one (ROUND CAP line), or the compose floor " + + 'refused a verifier so compose/submit still fit (VERIFY BUDGET line) — ' + + 'all termination rules, not errors: stop and compose, do not retry; 5 ' + + 'the reverse audit CONVERGED — every chunk holds two ' + 'consecutive substantive dry audits and none is due a cold check, so stop ' + 'the loop and proceed to Step 6 (also a termination rule, and a clean one: ' + 'no disclosure is owed); anything else is a bad call or a broken plan.', diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 4461c3522ee..5497a0314dd 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -17,7 +17,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { createHash } from 'node:crypto'; import { promptRecordDir, briefPath } from './lib/prompt-record.js'; -import { writeBudgetStop } from './lib/deadline.js'; +import { writeBudgetStop, writeRoundCapStop } from './lib/deadline.js'; import { getGhHost, setGhHost } from './lib/gh.js'; import { parseLedger } from './lib/ledger.js'; import { countInlineFindings } from './lib/inline-counts.js'; @@ -695,6 +695,29 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve expect(r.body).not.toContain('no blockers'); }); + it('a round-cap marker caps the verdict and dedups against the relayed entry', () => { + // A huge diff's reverse audit ran its full 3 rounds without converging; + // the builder refused round 4 and wrote a round-cap marker. compose-review + // caps on it whether or not the orchestrator relays — and says it once + // when the orchestrator does relay. + const plan = coveredPlan(); + writeRoundCapStop(plan, 3, 4); + const r = composeReview(base({ planPath: plan })); + expect(r.event).toBe('COMMENT'); + expect(r.body).toContain('reverse-audit round cap of 3'); + expect(r.body).not.toContain('LGTM'); + + const r2 = composeReview( + base({ + planPath: plan, + unreviewedDimensions: [ + 'reverse audit — did not converge within the reverse-audit round cap of 3', + ], + }), + ); + expect(r2.body.split('reverse-audit round cap').length - 1).toBe(1); + }); + it('a budget-stop marker caps APPROVE at COMMENT with nothing relayed by the caller', () => { // The round builder refused a round and recorded the refusal; the // disclosure that caps the verdict is synthesized from that marker, not diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 3b3b25b5f55..7ec573d4047 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -32,9 +32,12 @@ import { } from './lib/coverage.js'; import { BUDGET_STOP_PHRASE, + ROUND_CAP_PHRASE, budgetStopDisclosure, + roundCapStopDisclosure, readBudgetStop, } from './lib/deadline.js'; +import { MAX_REVERSE_AUDIT_ROUNDS } from './lib/budget.js'; import { shellQuotePath } from './lib/shell-quote.js'; import { gh, setGhHost } from './lib/gh.js'; import { @@ -420,12 +423,22 @@ function composeReviewBody( if (input.planPath) { const stop = readBudgetStop(input.planPath); if (stop !== null) { + // A round-cap stop and a time-budget stop both cap the verdict, but + // read differently and dedup against a different relayed phrase. The + // marker's `cause` picks which; an absent cause is a time stop, for + // markers written before the cause field existed. + const isRoundCap = stop.cause === 'round-cap'; + const phrase = isRoundCap ? ROUND_CAP_PHRASE : BUDGET_STOP_PHRASE; for (let i = unreviewed.length - 1; i >= 0; i--) { - if (unreviewed[i].includes(BUDGET_STOP_PHRASE)) { + if (unreviewed[i].includes(phrase)) { unreviewed.splice(i, 1); } } - budgetEntry = budgetStopDisclosure(stop.round ?? undefined); + budgetEntry = isRoundCap + ? roundCapStopDisclosure( + typeof stop.cap === 'number' ? stop.cap : MAX_REVERSE_AUDIT_ROUNDS, + ) + : budgetStopDisclosure(stop.round ?? undefined); coverageEntries.push(budgetEntry); } } diff --git a/packages/cli/src/commands/review/lib/budget.test.ts b/packages/cli/src/commands/review/lib/budget.test.ts index eea0f756fb0..fe24f52fe35 100644 --- a/packages/cli/src/commands/review/lib/budget.test.ts +++ b/packages/cli/src/commands/review/lib/budget.test.ts @@ -12,6 +12,7 @@ import { budgetGapDisclosures, stripBudgetGapLines, launchToolBudget, + reverseAuditRoundCap, reviewBudget, } from './budget.js'; @@ -83,12 +84,30 @@ describe('reviewBudget — domain specialists', () => { it('are capped at two once the diff is big enough for dominance to mean something', () => { expect(budget(80).specialistCap).toBe(2); - expect(budget(10_000).specialistCap).toBe(2); + expect(budget(2999).specialistCap).toBe(2); + }); + + it('shed to zero on a huge diff — the marginal pass that tips it into a timeout', () => { + // At/above the huge floor an Agent 8 whole-diff pass on top of the base + // fan-out is what a too-big-to-finish review can least afford. + expect(budget(3000).specialistCap).toBe(0); + expect(budget(10_000).specialistCap).toBe(0); }); it('read source lines only — a test-heavy diff does not unlock them', () => { expect(budget(20, 3000).specialistCap).toBe(0); }); + + it('shed on a huge non-source diff — the gate keys on effective, not src', () => { + // A docs/lockfile-dominated diff (small src, enormous total) is huge by the + // effective measure, so Agent 8 sheds even though src alone clears the 80 + // floor. Pins `effective < HUGE_DIFF_FLOOR` against a slip back to `src`, + // which would restore specialistCap: 2 in exactly the timeout band this + // gate exists to shed it from. + expect( + reviewBudget({ srcDiffLines: 100, diffLines: 30_000 }).specialistCap, + ).toBe(0); + }); }); describe('reviewBudget — the verify shard is flat', () => { @@ -370,6 +389,69 @@ describe('stripBudgetGapLines — the receipt judged without its disclosures', ( }); }); +describe('reviewBudget — the reverse-audit round cap', () => { + it('runs the full five rounds below the huge floor, three at it and above', () => { + // A reverse-audit round re-reads the whole diff against a growing + // findings list (~90 min on a 4,000-line PR); five rounds cannot finish + // the huge PRs that timed out to zero, so a huge diff caps at three — + // 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). + expect( + reviewBudget({ srcDiffLines: 100, diffLines: 100 }).reverseAuditRounds, + ).toBe(5); + expect( + reviewBudget({ srcDiffLines: 2999, diffLines: 2999 }).reverseAuditRounds, + ).toBe(5); + expect( + reviewBudget({ srcDiffLines: 3000, diffLines: 3000 }).reverseAuditRounds, + ).toBe(3); + expect( + reviewBudget({ srcDiffLines: 10_000, diffLines: 12_000 }) + .reverseAuditRounds, + ).toBe(3); + }); + + it('keys on effective lines, not raw source — a huge lockfile diff caps too', () => { + // effective = max(src, floor(total/8)); a mostly-generated 30,000-line + // diff with little source still costs a huge reverse audit to re-read. + // Pins the `effective`-vs-`src` dependence the mutation `effective` → + // `src` would otherwise survive. + expect( + reviewBudget({ srcDiffLines: 100, diffLines: 30_000 }).reverseAuditRounds, + ).toBe(3); + expect(reviewBudget({ srcDiffLines: 100, diffLines: 30_000 }).sweep).toBe( + true, + ); + }); + + it('never drops below the convergence minimum', () => { + for (const n of [0, 1, 50, 3000, 100_000]) { + expect( + reviewBudget({ srcDiffLines: n, diffLines: n }).reverseAuditRounds, + ).toBeGreaterThanOrEqual(3); + } + }); +}); + +describe('reverseAuditRoundCap — the one reader of the plan field', () => { + it('passes a valid cap through and defaults everything else to the max', () => { + expect(reverseAuditRoundCap({ reverseAuditRounds: 3 })).toBe(3); + expect(reverseAuditRoundCap({ reverseAuditRounds: 5 })).toBe(5); + // Absent, out-of-band and garbled all read as the full cap: an old or + // hand-edited plan errs toward more auditing, never less. The range is + // floored at HUGE_REVERSE_AUDIT_ROUNDS (3) — the smallest cap the CLI + // writes — so 1 and 2 read as the full cap, not as themselves. + expect(reverseAuditRoundCap(undefined)).toBe(5); + expect(reverseAuditRoundCap({})).toBe(5); + expect(reverseAuditRoundCap({ reverseAuditRounds: 0 })).toBe(5); + expect(reverseAuditRoundCap({ reverseAuditRounds: 1 })).toBe(5); + expect(reverseAuditRoundCap({ reverseAuditRounds: 2 })).toBe(5); + expect(reverseAuditRoundCap({ reverseAuditRounds: 6 })).toBe(5); + expect(reverseAuditRoundCap({ reverseAuditRounds: 2.5 })).toBe(5); + expect(reverseAuditRoundCap({ reverseAuditRounds: '1' })).toBe(5); + }); +}); + describe('reviewBudget — the budget survives the trip through the plan', () => { it('agentToolBudget is an enumerable field of the returned object', () => { // The plan is written with JSON.stringify(report); a field that were a @@ -378,6 +460,7 @@ describe('reviewBudget — the budget survives the trip through the plan', () => // not just the type. const b = reviewBudget({ srcDiffLines: 10, diffLines: 10 }); expect(Object.keys(b)).toContain('agentToolBudget'); + expect(Object.keys(b)).toContain('reverseAuditRounds'); expect( (JSON.parse(JSON.stringify(b)) as Record)[ 'agentToolBudget' diff --git a/packages/cli/src/commands/review/lib/budget.ts b/packages/cli/src/commands/review/lib/budget.ts index fdd16fa9688..2106c034077 100644 --- a/packages/cli/src/commands/review/lib/budget.ts +++ b/packages/cli/src/commands/review/lib/budget.ts @@ -97,6 +97,28 @@ export interface ReviewBudget { * crawl only feeds the wall clock. */ agentToolBudget: number; + /** + * The reverse-audit loop's round cap: the full `MAX_REVERSE_AUDIT_ROUNDS` + * normally, or a reduced `HUGE_REVERSE_AUDIT_ROUNDS` for a diff large + * enough that the full loop cannot finish inside any budget. + * + * A reverse-audit round re-reads the whole diff against a growing + * findings list, so its cost scales with the diff — measured at ~90 + * minutes a round on a 4,000-line PR, where the full five rounds alone + * (450 min) exceed the six-hour CI ceiling before the fan-out and tail + * are even counted. In a time-budgeted CI run the deadline gate already + * refuses a round that will not fit; this static cap is the belt it works + * under and the ONLY bound a local run (no deadline) has. Reduced to + * three, not two — not because two cannot converge (the all-dry + * rounds-1-and-2 shape reaches CONVERGED at the round-3 build under any + * cap of two or more, since the convergence check runs before the cap + * gate) but to buy hot chunks one extra audit round before the cap. + * + * The budget tunes how many rounds the loop runs, never whether it runs: + * the reverse audit is a dimension of the high-effort contract. The CLI + * only ever writes three or five here. + */ + reverseAuditRounds: number; } /** @@ -105,6 +127,35 @@ export interface ReviewBudget { */ const SWEEP_FLOOR = 25; +/** + * The reverse-audit loop's full round cap (SKILL.md Step 5's "stop at the + * plan's `reverseAuditRounds` cap"). The normal value; a huge diff gets + * `HUGE_REVERSE_AUDIT_ROUNDS` instead. `retirement.ts` re-exports it. + */ +export const MAX_REVERSE_AUDIT_ROUNDS = 5; + +/** + * The reduced cap for a huge diff — three, one audit round above the + * convergence floor of two, spent on hot chunks before the cap stops the + * loop. Not a convergability minimum: the all-dry rounds-1-and-2 shape + * reaches CONVERGED under any cap of two or more, because the reverse + * audit's convergence check runs before the round-cap gate. + */ +export const HUGE_REVERSE_AUDIT_ROUNDS = 3; + +/** + * The effective-line threshold above which a diff is "huge": its reverse + * audit is capped and its Agent 8 specialists are shed. Set from the + * timeout survey — the 6-hour CI reviews that ran to zero posted output + * were 4,000-5,300 line PRs (a single reverse-audit round already ~90 min); + * 3,000 triggers with margin below that band while leaving the full loop + * for the common case. `effective` (the plan's source-weighted line-span + * measure) slightly over-counts against source body lines, which only ever + * makes this fire a little EARLIER — the safe direction for a + * finishability gate. + */ +const HUGE_DIFF_FLOOR = 3000; + /** Below this, "one domain dominates the diff" is not a finding about the diff. */ const SPECIALIST_FLOOR = 80; @@ -158,16 +209,51 @@ export function reviewBudget(input: BudgetInput): ReviewBudget { return { inlineAngles, sweep: effective >= SWEEP_FLOOR, - specialistCap: src >= SPECIALIST_FLOOR ? 2 : 0, + // Agent 8 sheds in the huge zone. A specialist is a whole-diff pass on + // top of the base fan-out, and on a diff too big to finish that extra + // pass is the marginal cost that guarantees zero posted output — while + // the per-chunk fan-out already covers the ground. Finishability over + // an added depth pass, in exactly the band where the review otherwise + // posts nothing. + specialistCap: + src >= SPECIALIST_FLOOR && effective < HUGE_DIFF_FLOOR ? 2 : 0, verifyShard: VERIFY_SHARD, agentToolBudget: clamp( MIN_AGENT_TOOL_BUDGET + Math.floor(effective / LINES_PER_TOOL_CALL), MIN_AGENT_TOOL_BUDGET, MAX_AGENT_TOOL_BUDGET, ), + reverseAuditRounds: + effective >= HUGE_DIFF_FLOOR + ? HUGE_REVERSE_AUDIT_ROUNDS + : MAX_REVERSE_AUDIT_ROUNDS, }; } +/** + * The reverse-audit round cap a plan's budget carries, for every reader + * that enforces or narrates it (the admission gate, the retirement + * scheduler, the cold-check note). A plan without the field — an older + * CLI — or a garbled value reads as the full cap: an old plan errs toward + * more auditing, never less, exactly like every other budget fallback. + * + * The accepted range is floored at `HUGE_REVERSE_AUDIT_ROUNDS`, the + * smallest cap the CLI ever writes. A value of one or two is out of band + * (a hand-edited plan): honouring it would force a non-converged round-cap + * stop where the full loop would have kept auditing, so it too falls back + * to the full cap — never less. + */ +export function reverseAuditRoundCap(budget: unknown): number { + const v = (budget as { reverseAuditRounds?: unknown } | undefined) + ?.reverseAuditRounds; + return typeof v === 'number' && + Number.isInteger(v) && + v >= HUGE_REVERSE_AUDIT_ROUNDS && + v <= MAX_REVERSE_AUDIT_ROUNDS + ? v + : MAX_REVERSE_AUDIT_ROUNDS; +} + /** * The per-launch tool ceiling: the exploration allowance for this launch, * PLUS the launch's mandatory reads. diff --git a/packages/cli/src/commands/review/lib/deadline.test.ts b/packages/cli/src/commands/review/lib/deadline.test.ts index efe1ccfa79c..8777ca869ea 100644 --- a/packages/cli/src/commands/review/lib/deadline.test.ts +++ b/packages/cli/src/commands/review/lib/deadline.test.ts @@ -30,6 +30,11 @@ import { readRoundStamps, reverseAuditBudgetExhausted, reverseAuditBudgetMessage, + ROUND_CAP_PHRASE, + roundCapStopDisclosure, + roundCapStopEntry, + roundCapStopEntryZh, + writeRoundCapStop, stampRound, verifyBudgetExhausted, verifyBudgetMessage, @@ -442,6 +447,50 @@ describe('reverseAuditBudgetMessage', () => { }); }); +describe('writeRoundCapStop — the round-cap marker', () => { + it('round-trips through readBudgetStop with cause and cap', () => { + const dir = mkdtempSync(join(tmpdir(), 'rc-stop-')); + try { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, '{}'); + backdatePlan(plan); + writeRoundCapStop(plan, 3, 4, NOW_MS); + const stop = readBudgetStop(plan); + expect(stop?.cause).toBe('round-cap'); + expect(stop?.cap).toBe(3); + expect(stop?.entry).toBe(roundCapStopEntry(3)); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('disclosure names the cap in both languages', () => { + expect(roundCapStopDisclosure(3).reason).toContain(ROUND_CAP_PHRASE); + expect(roundCapStopDisclosure(3).reason).toContain('of 3'); + expect(roundCapStopEntryZh(3)).toContain('3'); + }); + + it('writes round as an explicit null when the caller passes undefined', () => { + // The chunkless call site (agent-prompt.ts) passes `round: undefined`; the + // `?? null` fallback must keep the key PRESENT with a null value, not let + // JSON.stringify drop it — a consumer that distinguishes null from an + // absent key would otherwise misread the marker. + const dir = mkdtempSync(join(tmpdir(), 'rc-stop-')); + try { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, '{}'); + backdatePlan(plan); + writeRoundCapStop(plan, 3, undefined, NOW_MS); + const stop = readBudgetStop(plan); + expect(stop).not.toBeNull(); + expect(stop && 'round' in stop).toBe(true); + expect(stop?.round).toBeNull(); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + describe('verifyBudgetExhausted — the compose floor the verifier answers to', () => { it('fails OPEN on a missing or malformed deadline — every local run', () => { expect(verifyBudgetExhausted({}, NOW_MS)).toBeNull(); diff --git a/packages/cli/src/commands/review/lib/deadline.ts b/packages/cli/src/commands/review/lib/deadline.ts index 1a8f05d0d38..c2ab90b4063 100644 --- a/packages/cli/src/commands/review/lib/deadline.ts +++ b/packages/cli/src/commands/review/lib/deadline.ts @@ -9,7 +9,8 @@ // The iterative reverse audit (Step 5) is the one stage of a review whose cost // is open-ended: each round is a fan-out (one auditor per chunk on a 3B plan), // each round's findings go back through verification, and the loop runs until -// two consecutive dry rounds or the 5-round cap. On a PR where every round +// two consecutive dry rounds or the plan's round cap (5, or 3 for a huge +// diff). On a PR where every round // finds something, that is the whole budget. Measured on a real CI run // (#8368, +1699 lines): the audit loop ran to the 5-round cap, consumed 3.5 of // the job's 4 budgeted hours, and the outer GNU-timeout kill arrived while @@ -397,6 +398,16 @@ export function verifyBudgetMessage(spent: ComposeFloorExhausted): string { } export interface BudgetStop { + /** + * Which termination wrote this marker: the time budget (the reverse-audit + * loop ran out of clock) or the round cap (it ran its full allotted + * rounds without converging). `compose-review` picks the disclosure text + * by this; an absent value reads as `time-budget` for back-compat. + */ + cause?: 'time-budget' | 'round-cap'; + /** The round cap, when `cause` is `round-cap` — what `compose-review` + * re-derives the disclosure from, the way it uses `round` for a time stop. */ + cap?: number; /** The exact `unreviewedDimensions` entry, composed here so the text that * caps the verdict is this module's in both channels. */ entry: string; @@ -449,6 +460,77 @@ export function budgetStopEntryZh(round: number | undefined): string { return `${d.subjectZh}——${d.reasonZh}`; } +/** + * The phrase identifying a ROUND-CAP disclosure wherever it is relayed — + * the cap analogue of `BUDGET_STOP_PHRASE`, so `compose-review` dedups the + * orchestrator's relayed copy against the marker's by shared text. + */ +export const ROUND_CAP_PHRASE = 'reverse-audit round cap'; + +/** + * The round-cap disclosure as structural parts, both languages — the + * analogue of `budgetStopDisclosure` for a loop that ran its full allotted + * rounds without converging. + */ +export function roundCapStopDisclosure(cap: number): { + subject: string; + reason: string; + subjectZh: string; + reasonZh: string; +} { + return { + subject: 'reverse audit', + reason: `did not converge within the ${ROUND_CAP_PHRASE} of ${cap}`, + subjectZh: '反向审计', + reasonZh: `在 ${cap} 轮的反审轮数上限内未收敛`, + }; +} + +/** The round-cap entry, spelled once for the marker AND the stderr message. */ +export function roundCapStopEntry(cap: number): string { + const d = roundCapStopDisclosure(cap); + return `${d.subject} — ${d.reason}`; +} + +/** The Chinese pair of `roundCapStopEntry`. */ +export function roundCapStopEntryZh(cap: number): string { + const d = roundCapStopDisclosure(cap); + return `${d.subjectZh}——${d.reasonZh}`; +} + +/** + * Persist a round-cap refusal beside the prompt records, so + * `compose-review` caps the verdict on a loop that ran its full rounds + * without converging — without depending on the orchestrator to relay the + * entry. Same marker file and same swallow-on-write-error discipline as + * `writeBudgetStop`; only one stop fires per run, whichever refusal comes + * first. + */ +export function writeRoundCapStop( + planPath: string, + cap: number, + round: number | undefined, + nowMs: number = Date.now(), +): void { + try { + const dir = promptRecordDir(planPath); + mkdirSync(dir, { recursive: true }); + const stop: BudgetStop = { + cause: 'round-cap', + cap, + entry: roundCapStopEntry(cap), + entryZh: roundCapStopEntryZh(cap), + round: round ?? null, + remainingSeconds: 0, + reserveSeconds: 0, + atMs: nowMs, + }; + writeFileSync(join(dir, STOP_FILE), JSON.stringify(stop, null, 2)); + } catch { + // Refusing is the load-bearing half; the stderr entry still carries it. + } +} + /** * Persist the refusal beside the prompt records, where `compose-review` * reads it back and synthesizes the verdict-capping disclosure without diff --git a/packages/cli/src/commands/review/lib/retirement.ts b/packages/cli/src/commands/review/lib/retirement.ts index 4915e38f651..95dc08dc101 100644 --- a/packages/cli/src/commands/review/lib/retirement.ts +++ b/packages/cli/src/commands/review/lib/retirement.ts @@ -73,15 +73,6 @@ export interface RoundSchedule { converged: boolean; } -/** - * The loop's hard cap, mirroring SKILL.md's Step 5 ("Stop after 5 rounds - * regardless"). Enforcing it is the orchestrator's — the builder will build - * a sixth round if asked — but the retirement note is the orchestrator's - * only word about a skipped chunk, and it must not promise a cold check the - * cap has already forbidden. - */ -export const REVERSE_AUDIT_MAX_ROUNDS = 5; - /** * The round part of a per-chunk reverse-audit record key, as `runAllChunks` * and the single-chunk rebuild path both spell it. The digest tail is matched @@ -415,7 +406,8 @@ export function scheduleReverseAuditRound( env: NodeJS.ProcessEnv = process.env, diffPath?: string, ): RoundSchedule { - // Rounds 1 and 2 establish the record; there is nothing to retire on. + // Rounds 1 and 2 establish each chunk's record; retirement needs two + // consecutive dry audits, so nothing can retire before round 3. if (round < 3) { return { due: [...chunkIds], @@ -567,8 +559,9 @@ export function scheduleReverseAuditRound( dryRounds: [lastTwo[0].round, lastTwo[1].round], // The next even round — this branch only runs on odd rounds, so // that is always round + 1. Whether the cap allows it is the note - // composer's question, not the schedule's (see - // REVERSE_AUDIT_MAX_ROUNDS). + // composer's question, not the schedule's: the plan's cap + // (`reverseAuditRoundCap` in budget.ts, floored at the huge-diff + // tier's 3) is what the admission gate enforces. nextColdCheck: round + 1, }); } diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index c1b776c472d..262fed58f17 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -937,6 +937,12 @@ Measured across the same six CI reviews: the post-verdict bookkeeping — Markdo Two measured runs of the same 14-agent Step 3A fan-out, on diffs of comparable size, took 11.7 and 41 minutes — and the wave's wall clock is its slowest agent, so the whole review inherited the difference. The slow wave's tail was not review depth: individual agents spent 40-100 model calls exploring the tree (the pattern a sibling PR had already named as the next optimization target), while healthy agents on the same class of diff settle at 25-45 calls with indistinguishable findings. The budget that answers this is soft on purpose: a hard cap would convert the pathology into silent truncation, so the brief tells the agent to stop exploring at the ceiling, file what it holds, and disclose the checks it did not get to — the disclosure lands in the same receipt machinery that already judges whiffs. +### The six-hour timeouts + +A survey of one recent 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 wall clock is model inference, not code execution: at the orchestrator level 82-88% of it is spent inside subagents, and inside a subagent ~81% is model turns (reading the diff, reasoning) — the one shell-heavy agent is Build & Test. So the timeout driver on a huge PR is the sheer volume of model work: dozens of finder and chunk agents reading a 4,000-5,300-line diff, then a reverse-audit loop whose every round re-reads that diff against a growing findings list (~90 min a round). Five rounds alone (450 min) exceed the six-hour ceiling before the fan-out is counted. + +The elastic budget answers this at the size band where the review otherwise posts nothing. `reverseAuditRounds` drops from five to three for a huge diff (effective ≥ 3000 lines) — one audit round above the convergence floor of two, since the all-dry rounds-1-and-2 shape reaches CONVERGED under any cap of two or more (the convergence check runs before the round-cap gate); the extra round buys hot chunks one more pass before the cap — and `specialistCap` sheds Agent 8 to zero there, because an Agent 8 whole-diff 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. Neither drops a required dimension: the reverse audit still runs, and Agent 8 was never a required agent. In a time-budgeted CI run the deadline gate already refuses a round that will not fit; this static cap is the belt it works under and the only bound a local run has. The cap refusal writes a marker `compose-review` caps the verdict on, so a non-converged stop at the cap discloses like a budget stop rather than resting on the orchestrator's relay. + ### The killed-before-compose tail (PR #8687) A 4,269-line, 21-file cross-worktree git guard — a security PR whose adversarial surface is near-unbounded — ran its reverse audit to a **correct** budget stop: the deadline gate refused round 3 with ~110 minutes and the whole reserve in hand, exactly as designed. Then the run died anyway, and posted nothing, holding ~20 E2E-confirmed Critical bypasses. The tail after the stop was the killer: a single hand-rolled verification agent re-running a 15-family shell/git bypass battery — each family spun up a temp git repo and executed real payloads — consumed the entire remaining budget, and the outer wall hit mid-verification, before compose-review ever ran. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 6a57630f1f0..b824baabbda 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -211,7 +211,7 @@ Read from it: - `diffLines`, `diffChars`, and `srcDiffLines` / `testDiffLines` / `docsDiffLines` / `generatedDiffLines` - `chunks[]` — contiguous, non-overlapping line ranges tiling the whole diff. Each entry has `id`, `startLine`, `endLine` (1-based, inclusive), `lines`, `chars`, an `oversized` flag, and `files[]` naming the source files and new-side line ranges it covers. A chunk with `oversized: true` may exceed what one `read_file` call returns. - `files[]` — per-file `kind` (`source` / `test` / `generated`), `hunks[]` new-side ranges (Step 7 validates comment anchors against these), `addedRanges[]` and `diffRange` (present only on `heavy` files — the exact lines the PR wrote, and where that file's own diff lives, so an invariant agent can see what was deleted), change counts, and the `heavy` flag -- `budget` — how much walking the **size-elastic** parts of this run owe, derived from `srcDiffLines` the same way the topology gate is, and recorded here rather than passed as a flag so every reader sees one number. `inlineAngles` and `sweep` scope Step 3C's low pass; `specialistCap` is the Agent 8 ceiling (**0** below 80 source lines — "one domain dominates the diff" is a judgement, and a judgement made about forty lines finds a dominant domain every time, because forty lines are usually all one thing); `verifyShard` is Step 4's findings-per-verifier; `agentToolBudget` is the base rate of the soft tool-call ceiling `agent-prompt` bakes into every finder and auditor brief — not the verifier's, not Agent 7's, and not Agent 0's, whose mandatory work scales with the linked issues rather than the diff. The ceiling is per **launch**: a scoped agent (a chunk, a heavy file) gets an allowance derived from its own territory — never above the plan's recorded allowance, which is clamped into the budget's own band in both directions, so the plan stays the one number every launch answers to — and every launch's assigned reads ride on top of the allowance rather than inside it, so a huge diff's mandatory chunk reads can never exhaust the exploration a whole-diff role owes — because a wave's wall clock is its slowest agent and the slowest agent is reliably one that kept exploring past any recall gain: the same 14-agent fan-out has measured 11.7 and 41 minutes on comparable diffs, the difference being individual agents spending 40-100 calls walking the tree (measured; DESIGN.md — The forty-one minute wave). The ceiling is soft and the briefs restate the recall rule beside it: at the budget an agent stops **exploring**, never reporting — findings in hand are filed, and each stopped check is disclosed on its own line in the fixed form `Budget gap: `, which `check-coverage` parses out of the transcripts (its report's `budgetGaps`) — see Step 3D for the ruling each gap is owed. **It never scales a dimension away** — which agents a review owes is the roster's answer and the roster reads `effort`, so a size input cannot become a back door into shrinking coverage. Nothing here is yours to override: a budget the caller can inflate is a budget that gets inflated. **A plan with no `budget` field** (written by an older CLI — the version-skew this skill has already measured once) falls back to the pre-budget flat behaviour, which errs toward more coverage, never less: walk all six angles, run the sweep, cap Agent 8 at 2, shard verification at 8. +- `budget` — how much walking the **size-elastic** parts of this run owe, sized from `srcDiffLines` except that an all-non-source diff (docs, lockfiles) counts its total lines at an eighth rate, so the size these tiers read is `effective = max(srcDiffLines, floor(diffLines / 8))`; recorded here rather than passed as a flag so every reader sees one number. `inlineAngles` and `sweep` scope Step 3C's low pass; `specialistCap` is the Agent 8 ceiling (**0** below 80 source lines — "one domain dominates the diff" is a judgement, and a judgement made about forty lines finds a dominant domain every time, because forty lines are usually all one thing — **and 0 again for a huge diff (effective ≥ 3000)**, where an Agent 8 whole-diff pass on top of the base fan-out is the marginal cost that tips a review too big to finish into posting nothing); `verifyShard` is Step 4's findings-per-verifier; `reverseAuditRounds` is the reverse-audit loop's round cap — **5** normally, **3 for a huge diff** (effective ≥ 3000 lines). A reverse-audit round re-reads the whole diff against a growing findings list, so it costs ~90 minutes on a 4,000-line PR, where five rounds (450 min) alone exceed the six-hour ceiling before the fan-out and tail are counted — the 6-hour timeouts that posted nothing were 4,000-5,300-line PRs (measured; DESIGN.md — The six-hour timeouts). 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; the extra round buys hot chunks one more pass. The `agent-prompt` builder enforces the cap itself (a `ROUND CAP:` refusal, exit 4, that writes a marker `compose-review` caps on — same contract as the deadline gate below), so you never count rounds yourself. `agentToolBudget` is the base rate of the soft tool-call ceiling `agent-prompt` bakes into every finder and auditor brief — not the verifier's, not Agent 7's, and not Agent 0's, whose mandatory work scales with the linked issues rather than the diff. The ceiling is per **launch**: a scoped agent (a chunk, a heavy file) gets an allowance derived from its own territory — never above the plan's recorded allowance, which is clamped into the budget's own band in both directions, so the plan stays the one number every launch answers to — and every launch's assigned reads ride on top of the allowance rather than inside it, so a huge diff's mandatory chunk reads can never exhaust the exploration a whole-diff role owes — because a wave's wall clock is its slowest agent and the slowest agent is reliably one that kept exploring past any recall gain: the same 14-agent fan-out has measured 11.7 and 41 minutes on comparable diffs, the difference being individual agents spending 40-100 calls walking the tree (measured; DESIGN.md — The forty-one minute wave). The ceiling is soft and the briefs restate the recall rule beside it: at the budget an agent stops **exploring**, never reporting — findings in hand are filed, and each stopped check is disclosed on its own line in the fixed form `Budget gap: `, which `check-coverage` parses out of the transcripts (its report's `budgetGaps`) — see Step 3D for the ruling each gap is owed. **It never scales a dimension away** — which agents a review owes is the roster's answer and the roster reads `effort`, so a size input cannot become a back door into shrinking coverage. Nothing here is yours to override: a budget the caller can inflate is a budget that gets inflated. **A plan with no `budget` field** (written by an older CLI — the version-skew this skill has already measured once) falls back to the pre-budget flat behaviour, which errs toward more coverage, never less: walk all six angles, run the sweep, cap Agent 8 at 2, shard verification at 8. A chunk is read with `read_file(file_path=diffPathAbsolute, offset=startLine - 1, limit=endLine - startLine + 1)` — `offset` is 0-based. @@ -623,7 +623,7 @@ After deduplication, run reverse audit **iteratively** — the first launch ride - **Small diffs (Step 3A path):** one reverse audit agent per round, reading the whole diff — except rounds 1 and 2, which are **the convergence pair** and launch together (below). - **Large diffs (Step 3B path):** one reverse audit agent **per chunk** per round, launched together in a single response. A single agent asked to re-read a 5 800-line diff with a growing finding list appended is the most context-starved agent in the pipeline — precisely on the PRs where the reverse audit matters most. Each per-chunk auditor gets the same territory as its Step 3B counterpart, plus the cumulative finding list for the **whole** diff (so it knows what is already covered elsewhere). -- **The builder schedules the 3B fan-out; you do not.** Rounds 1 and 2 audit every chunk — they are what establishes each territory's record. From round 3 on, `--all-chunks` reads the harness transcripts and **retires** any chunk whose own last two audits were substantively dry (the receipt named what it examined AND the transcript shows the diff was opened): a retired chunk is cold-checked on alternating rounds instead of every round, and a cold check that yields anything returns it to every-round auditing. The savings land on the odd rounds — every retired chunk cold-checks together on the even ones, so an even round's fan-out is unchanged; expect rounds 3 and 5 to shrink, not round 4. The blocks it prints are the round; the `retirement:` note after the `end of round` line names each skipped chunk and its certificate — relay that note in your narration, and do not hand-build an auditor for a chunk the builder skipped. Why, measured: on a real 6-chunk run, two chunks were dry in **all five rounds** — a third of the loop's auditors re-certifying territories that had already converged, while the three hot chunks were where every finding came from. Attention follows evidence; the certificate a retired chunk holds (two consecutive substantive dry audits) is exactly the one the whole loop used to end on. +- **The builder schedules the 3B fan-out; you do not.** Rounds 1 and 2 audit every chunk — they are what establishes each territory's record. From round 3 on, `--all-chunks` reads the harness transcripts and **retires** any chunk whose own last two audits were substantively dry (the receipt named what it examined AND the transcript shows the diff was opened): a retired chunk is cold-checked on alternating rounds instead of every round, and a cold check that yields anything returns it to every-round auditing. The savings land on the odd rounds — every retired chunk cold-checks together on the even ones, so an even round's fan-out is unchanged; expect the odd rounds to shrink, not the even ones (under the 3-round huge-diff cap only round 3 can shrink — the cap ends the loop before round 5). The blocks it prints are the round; the `retirement:` note after the `end of round` line names each skipped chunk and its certificate — relay that note in your narration, and do not hand-build an auditor for a chunk the builder skipped. Why, measured: on a real 6-chunk run, two chunks were dry in **all five rounds** — a third of the loop's auditors re-certifying territories that had already converged, while the three hot chunks were where every finding came from. Attention follows evidence; the certificate a retired chunk holds (two consecutive substantive dry audits) is exactly the one the whole loop used to end on. **The convergence pair (3A only).** Rounds 1 and 2 launch **in one response** — together with Step 4's verifier shards (Step 4 names this) — each built by its own `agent-prompt` call: `--round 1` and `--round 2`, the **same** `--findings` file. This is not a loosened criterion; it is the serial shape's own arithmetic made concurrent: a dry round leaves the cumulative list unchanged, so round 2's launch input was already substantively identical to round 1's — the same entries, at most with verification tags the merge had cleared in between — an independent rerun that the serial shape bought with a full round of wall clock, and that one budget-gated run could no longer afford at all, shipping a capped verdict for want of a second dry audit it had time to run in parallel but not in series (measured; DESIGN.md — The serial convergence pair). What the two-consecutive-dry criterion demands is unchanged: two independent, substantively-dry audits of the whole diff. The one delta the pair does introduce is the same one-round suppression window the pipelined loop already accepts (the merge bullet in the termination rules): the round-2 member audits with entries a verifier may be rejecting mid-flight still on its do-not-re-report list. @@ -667,10 +667,10 @@ The brief holds what the auditor is for: hunt only the **gaps** no prior agent c - A round is **dry** only when _every_ agent in it returned zero new findings **with** the evidence-bearing receipt (`No issues found — `). A round containing a twice-whiffed agent is **not dry** — silence is not convergence evidence — so the loop continues (the hard cap below still bounds it). - **When the loop ends with any scope still outstanding** (by cap, or by dry rounds elsewhere), terminal prose is not enough: add one self-explained entry per scope to `unreviewedDimensions` — e.g. `reverse audit of chunk 3 — the auditor returned nothing substantive twice` — so compose-review serializes it and caps a would-be Approve at `COMMENT`. The primary Step 3 pass did read that scope (its receipt stands), but this run's contract includes the reverse audit, and a verdict must not silently claim an audit that never ran. - Stop after **two consecutive dry rounds** (the 3A criterion — one auditor, so round-dry and territory-dry are the same thing). One dry round is not evidence of convergence: on PR #6457 the review returned "no blockers" twice and the very next round surfaced five Criticals, three of them in code that had been in the diff since the first commit. A single lazy agent must not be able to end the loop. A dry convergence pair satisfies this rule in one launch — its two members are exactly the two independent audits the rule demands; what the pair removes is the wall clock between them, not either audit. When the loop ends on this rule, the last reporting round's verifiers are already in flight (they launched with the next round's auditors) — wait for their verdicts and apply them in the final merge before Step 6. -- **On the 3B path the builder is also the convergence ledger**: when every chunk holds two consecutive substantive dry audits and none is due a cold check, `--all-chunks` builds nothing, prints a `CONVERGED` explanation to stderr and exits **5**. Stop the loop and proceed to Step 6 — this is a **clean** convergence, not a gap: no `unreviewedDimensions` entry is owed, because each chunk holds the two-dry rule's evidence chunk by chunk — two consecutive dry **audits**, though not necessarily in consecutive rounds (a chunk dry in rounds 1 and 2 skips round 3 and cold-checks dry in round 4, holding rounds 2 and 4). Exit 5 is mainly the CLI enforcing the stop the two-dry-rounds rule above used to leave to orchestrator discretion; the new savings are the odd-round skips and a round-5 convergence. (It cannot owe a verification launch: a reporting round makes its chunk hot, so every verifier launched with a later round that did run.) -- Stop after **5 rounds** regardless (hard cap), and say so in the output rather than implying convergence. If round 5 reported findings, its verifiers have NOT launched — that launch rides the next round's build, which the cap forbids — so launch them alone before Step 6 and wait for their verdicts; the tag backstop below (and `compose-review`'s machine-read of it) is what catches a miss. -- Findings **reported** by each round are merged into the cumulative list **before** the next round begins, so each round sees an updated baseline. **The merge runs unconditionally — before every round build and before Step 6, whether or not the previous round reported findings**: under the pipelined loop below, round _k_'s verdicts land during round _k+1_, and every termination mode (two dry rounds, CONVERGED, budget stop, the 5-round cap) can arrive with the final rounds dry — a merge keyed to "some round reported something" would never apply the last verdicts that landed. Each merge applies every Step 4 verdict that has landed: confirmed removes the tag, rejected removes the entry. Verification status does not gate the merge — the list exists so auditors do not re-report what is already filed, and an unverified entry serves that purpose exactly as well as a confirmed one. The trade, named: an entry a verifier later rejects will have suppressed one round of rediscovery in its neighbourhood — the window is one round in one location, and the 5-round cap still bounds the loop. The tag is what keeps this mechanical rather than remembered: an entry enters the list tagged `— [unverified]`; the merge after its Step 4 verdict removes the tag (confirmed) or the entry (rejected). Step 6's confirmed-only read then has something to key on — anything still tagged is left out of the confirmed set — instead of a memory of which round each entry arrived in. The tag rides inside the findings file, which is hashed into the record key and copied to the digest-named list file each block points at — so a launch that drops the pointer matches no record, and the delivery floor counts the agent's read of that file exactly as it counts the brief's. -- **A reporting round whose every finding the verifier rejected is retroactively dry.** The merge already removes a rejected entry from the cumulative list; from the merge that applies the last of a round's rejections, the round also stops counting as a reporting round, and the two-consecutive-dry rule reads rounds' **effective** status. Rejected means rejected — an entry confirmed at low confidence keeps its round a reporting round. Under the pipelined loop a round's verdicts land while the next round runs, so the upgrade usually arrives one round late, and that is still one round saved: a measured run held round 2 dry, watched round 3's sole finding be rejected, and then ran rounds 4 **and 5** — round 4's dry return plus the rejection already in hand was the two-dry evidence, and the fifth round audited nothing the loop had not already answered (measured; DESIGN.md — The rounds a rejected finding bought (PR #8353)). The rule leans on the rejection bar the verifier's brief already enforces — a rejection claims direct counter-evidence, never mere unverifiability — so a round retired by rejections is retired on evidence, not on doubt. **It pairs forward only, and is consulted when a round returns**: on round _k_'s dry return, first apply every verdict that has landed (the unconditional merge — the retirement takes effect at this application, not at some earlier moment), then end the loop if round _k−1_ was dry or is now retired. Round _k−1_ counts **launches, not labels**: the convergence pair is one round here — a pair member is never round _k−1_ on its own (the pair bullet's not-carried-forward rule stands), and a reporting pair retires only when every finding from **both** members is rejected. The upgrade never ends the loop by itself — a preceding dry round plus a freshly-retired round stops nothing while the next round is already in flight: that round was launched, and its return is taken whatever it says, because a launched auditor can be carrying a real Critical. This is the measured shape (round 4's return is where the loop closes under this rule — the measured run, which predates it, ran a fifth round) and the only pairing licensed here. It softens nothing else: a whiffed scope stays not-audited whatever the verdicts say, and on 3B the retirement ledger's per-chunk certificates are untouched — this rule reads at the level the round counter reads. +- **On the 3B path the builder is also the convergence ledger**: when every chunk holds two consecutive substantive dry audits and none is due a cold check, `--all-chunks` builds nothing, prints a `CONVERGED` explanation to stderr and exits **5**. Stop the loop and proceed to Step 6 — this is a **clean** convergence, not a gap: no `unreviewedDimensions` entry is owed, because each chunk holds the two-dry rule's evidence chunk by chunk — two consecutive dry **audits**, though not necessarily in consecutive rounds (a chunk dry in rounds 1 and 2 skips round 3 and cold-checks dry in round 4, holding rounds 2 and 4). Exit 5 is mainly the CLI enforcing the stop the two-dry-rounds rule above used to leave to orchestrator discretion; the new savings are the odd-round skips and a convergence at the cap round (round 5 normally, round 3 under the huge-diff cap). (It cannot owe a verification launch: a reporting round makes its chunk hot, so every verifier launched with a later round that did run.) +- Stop at the plan's **`reverseAuditRounds` cap** — 5, or 3 for a huge diff (effective ≥ 3000 lines) — and say so in the output rather than implying convergence. The builder enforces this itself: a round past the cap gets a `ROUND CAP:` refusal on stderr and exit **4**, and — like the time-budget gate — writes a marker `compose-review` caps the verdict on whether or not you relay anything; still add the entry the message names to `unreviewedDimensions` so the terminal report agrees. If the cap round reported findings, its verifiers have NOT launched — that launch rides the next round's build, which the cap forbids — so launch them alone before Step 6 and wait for their verdicts; the tag backstop below (and `compose-review`'s machine-read of it) is what catches a miss. +- Findings **reported** by each round are merged into the cumulative list **before** the next round begins, so each round sees an updated baseline. **The merge runs unconditionally — before every round build and before Step 6, whether or not the previous round reported findings**: under the pipelined loop below, round _k_'s verdicts land during round _k+1_, and every termination mode (two dry rounds, CONVERGED, budget stop, the round cap) can arrive with the final rounds dry — a merge keyed to "some round reported something" would never apply the last verdicts that landed. Each merge applies every Step 4 verdict that has landed: confirmed removes the tag, rejected removes the entry. Verification status does not gate the merge — the list exists so auditors do not re-report what is already filed, and an unverified entry serves that purpose exactly as well as a confirmed one. The trade, named: an entry a verifier later rejects will have suppressed one round of rediscovery in its neighbourhood — the window is one round in one location, and the plan's round cap still bounds the loop. The tag is what keeps this mechanical rather than remembered: an entry enters the list tagged `— [unverified]`; the merge after its Step 4 verdict removes the tag (confirmed) or the entry (rejected). Step 6's confirmed-only read then has something to key on — anything still tagged is left out of the confirmed set — instead of a memory of which round each entry arrived in. The tag rides inside the findings file, which is hashed into the record key and copied to the digest-named list file each block points at — so a launch that drops the pointer matches no record, and the delivery floor counts the agent's read of that file exactly as it counts the brief's. +- **A reporting round whose every finding the verifier rejected is retroactively dry.** The merge already removes a rejected entry from the cumulative list; from the merge that applies the last of a round's rejections, the round also stops counting as a reporting round, and the two-consecutive-dry rule reads rounds' **effective** status. Rejected means rejected — an entry confirmed at low confidence keeps its round a reporting round. Under the pipelined loop a round's verdicts land while the next round runs, so the upgrade usually arrives one round late, and that is still one round saved: a measured run held round 2 dry, watched round 3's sole finding be rejected, and then ran rounds 4 **and 5** — round 4's dry return plus the rejection already in hand was the two-dry evidence, and the fifth round audited nothing the loop had not already answered (measured; DESIGN.md — The rounds a rejected finding bought (PR #8353)). The rule leans on the rejection bar the verifier's brief already enforces — a rejection claims direct counter-evidence, never mere unverifiability — so a round retired by rejections is retired on evidence, not on doubt. **It pairs forward only, and is consulted when a round returns**: on round _k_'s dry return, first apply every verdict that has landed (the unconditional merge — the retirement takes effect at this application, not at some earlier moment), then end the loop if round _k−1_ was dry or is now retired. Round _k−1_ counts **launches, not labels**: the convergence pair is one round here — a pair member is never round _k−1_ on its own (the pair bullet's not-carried-forward rule stands), and a reporting pair retires only when every finding from **both** members is rejected. The upgrade never ends the loop by itself — a preceding dry round plus a freshly-retired round stops nothing while the next round is already in flight: that round was launched, and its return is taken whatever it says, because a launched auditor can be carrying a real Critical. This is the measured shape (round 4's return is where the loop closes under this rule — the measured run, which predates it, ran a fifth round; a cap-5 shape — under the 3-round huge-diff tier the upgrade can only ever retire rounds 1–2, since the cap round's verdicts land during its solo verification, after the loop has already ended) and the only pairing licensed here. It softens nothing else: a whiffed scope stays not-audited whatever the verdicts say, and on 3B the retirement ledger's per-chunk certificates are untouched — this rule reads at the level the round counter reads. - **Verification rides alongside the next round, not ahead of it.** When round _k_ returns with new findings, one response launches BOTH round _k_'s verifiers (Step 4, `--role verify --round k` with that round's new findings) AND round _k+1_'s auditors — build the two prompt sets first, then fire every agent together, exactly as Step 3 fans out. (Step 4's initial verification is the k=0 case of the same rule: its shards ride with the first reverse-audit launch — the convergence pair on 3A, round 1's fan-out on 3B.) The serial shape (audit → wait for verification → next round) spent 5-8 minutes per round waiting for verifiers whose results the next round's auditors never needed. Two orderings still hold: the **last** round's verification must complete before Step 6 (that ordering is what keeps unverified entries out of the report and the PR, backed by the tag backstop at the end of this step — which `compose-review` machine-checks from `findingsPath`, Step 6), and a rejected finding leaves the cumulative list at the next merge. - **The round builder is also the loop's clock.** In a time-budgeted run (CI exports `QWEN_REVIEW_DEADLINE_EPOCH`; a local run normally has no deadline and is untouched), `agent-prompt --role reverse-audit` refuses to build a round that no longer fits: the remaining time must cover **the round itself** (estimated from the costliest round's measured cost so far — a repair relaunch can make one round the expensive one, and the gate prices the worst case the run has proved, not the newest dip — or a conservative constant for round 1) **plus** the reserve kept for its verification, compose-review and submission. On refusal it prints a `BUDGET:` line to stderr and exits **4**. That refusal is a termination rule, not an error — do not rebuild the round, do not relaunch auditors, and do not retry the command. The builder also records a budget-stop marker that `compose-review` reads directly, so the verdict is capped whether or not you relay anything; still add the exact entry the message names (`reverse audit — stopped before round by the review time budget`) to `unreviewedDimensions` so the terminal report and the body agree, and proceed to Step 6. **The tail after a budget stop is bounded, and its order is load-bearing.** Verify the last round's findings — the ones whose verifiers would have ridden the round the gate just refused — **only through `agent-prompt --role verify`, never a hand-rolled `agent`**: that builder is gated on a **compose floor** and prints a `VERIFY BUDGET:` refusal (exit 4) once too little time remains, at which point you stop verifying and compose **immediately** — findings still carrying `— [unverified]` keep the tag, and `compose-review` caps the verdict on it and never treats an unverified finding as a confirmed blocker; everything earlier rounds confirmed still posts. **Bound the wait, not just the launch:** the builder gate stops a verifier from being _built_ below the floor, but a verifier admitted _above_ it can still run a real filesystem/git E2E workload past the floor while you wait on its batch — and `agent-prompt` builds prompts, it cannot cancel a running agent. So when the deadline is within the compose floor and a verifier batch has not returned, **stop waiting on it yourself**: take the findings in hand at their current tag and compose. A verifier you stopped waiting on leaves its findings `— [unverified]`, which caps the verdict exactly as a refused build would. Do **not** re-verify findings already confirmed in earlier rounds, and do **not** invent a fresh re-verification pass — that is the unbounded work a wall runs into. Compose and submit are non-negotiable; they always run. Why this exists, measured twice: a +1699-line PR's CI review ran the audit loop to the 5-round cap and was killed while round 5's findings were still being verified (#8368); and a 4,269-line cross-worktree git guard stopped the audit correctly with ~110 minutes left, then a single hand-rolled agent re-running a 15-family shell/git bypass battery with real filesystem E2E consumed all of it — the wall hit mid-verification, compose never ran, and ~20 E2E-confirmed Critical bypasses were never posted (measured; DESIGN.md — The killed-before-compose tail (PR #8687)). A review that stops on the budget still reports everything it proved; one that runs past it reports nothing.