diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 17e1b01c598..3a9f71a1ad3 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -484,6 +484,74 @@ describe('agent-prompt (command boundary)', () => { } }); + it('takes the round cap from the plan topology at the --chunk gate too', () => { + // The fourth of the four cap call sites, and the only one with no tier-10 + // coverage: a 3A-sized plan can carry chunks (the chunk budget is 400 + // lines while the 3A gate admits 3200 total), so a round rebuilt or + // repaired one --chunk at a time on a small plan reaches THIS gate. A + // regression touching only it would stay green suite-wide. + const dir = mkdtempSync(join(tmpdir(), 'ap-chunk-tier-')); + try { + const findings = join(dir, 'f.md'); + writeFileSync(findings, '- x'); + const handler = agentPromptCommand.handler as (a: unknown) => void; + delete process.env[DEADLINE_ENV]; + const stderr = () => + (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + + const small = join(dir, 'small.json'); + writeFileSync( + small, + JSON.stringify({ ...PLAN, srcDiffLines: 100, diffLines: 100 }), + ); + process.exitCode = undefined; + (writeStderrLine as unknown as Mock).mockClear(); + handler({ + plan: small, + role: 'reverse-audit', + chunk: 14, + findings, + round: 6, + }); + expect(process.exitCode).toBeUndefined(); + expect(readRecordedPrompts(small).size).toBe(1); + + (writeStderrLine as unknown as Mock).mockClear(); + handler({ + plan: small, + role: 'reverse-audit', + chunk: 14, + findings, + round: 11, + }); + expect(process.exitCode).toBe(4); + expect(stderr()).toContain('round cap is 10'); + + const large = join(dir, 'large.json'); + writeFileSync( + large, + JSON.stringify({ ...PLAN, srcDiffLines: 900, diffLines: 900 }), + ); + process.exitCode = undefined; + (writeStderrLine as unknown as Mock).mockClear(); + handler({ + plan: large, + role: 'reverse-audit', + chunk: 14, + findings, + round: 6, + }); + expect(process.exitCode).toBe(4); + expect(stderr()).toContain('round cap is 5'); + expect(readRecordedPrompts(large).size).toBe(0); + } finally { + process.exitCode = undefined; + rmSync(dir, { recursive: true, force: true }); + } + }); + it('lets --role reverse-audit --chunk N through and keys the record by its chunk', () => { // The unit tests build the launch prompt directly, bypassing the guard and the // key derivation. This drives the real handler: the guard must let the one legal @@ -960,6 +1028,55 @@ describe('--round — the CLI bakes the round into the identity line and the key } }); + it('takes the round cap from the plan’s topology on the chunkless path', () => { + // 3A is the topology that actually runs this path — one auditor a round, + // the whole diff — and it is the one the tier raises. Both arms use the + // same round 6 off the same builder: admitted under the 3A tier, refused + // under the 3B one. A flat cap cannot produce both. + const dir = mkdtempSync(join(tmpdir(), 'ap-cap-tier-')); + try { + const findings = join(dir, 'f.md'); + writeFileSync(findings, '- x'); + const handler = agentPromptCommand.handler as (a: unknown) => void; + delete process.env[DEADLINE_ENV]; + const stderr = () => + (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + + const small = join(dir, 'small.json'); + writeFileSync( + small, + JSON.stringify({ ...PLAN, srcDiffLines: 100, diffLines: 100 }), + ); + process.exitCode = undefined; + (writeStderrLine as unknown as Mock).mockClear(); + handler({ plan: small, role: 'reverse-audit', findings, round: 6 }); + expect(process.exitCode).toBeUndefined(); + expect(readRecordedPrompts(small).size).toBe(1); + + (writeStderrLine as unknown as Mock).mockClear(); + handler({ plan: small, role: 'reverse-audit', findings, round: 11 }); + expect(process.exitCode).toBe(4); + expect(stderr()).toContain('round cap is 10'); + + const large = join(dir, 'large.json'); + writeFileSync( + large, + JSON.stringify({ ...PLAN, srcDiffLines: 900, diffLines: 900 }), + ); + process.exitCode = undefined; + (writeStderrLine as unknown as Mock).mockClear(); + handler({ plan: large, role: 'reverse-audit', findings, round: 6 }); + expect(process.exitCode).toBe(4); + expect(stderr()).toContain('round cap is 5'); + expect(readRecordedPrompts(large).size).toBe(0); + } finally { + process.exitCode = undefined; + rmSync(dir, { recursive: true, force: true }); + } + }); + it('carries the round through --all-chunks: every key and every identity line', () => { const dir = mkdtempSync(join(tmpdir(), 'ap-round-batch-')); try { @@ -3744,6 +3861,30 @@ describe('per-chunk retirement — cold territories stop costing a round', () => expect(out).not.toContain('next cold check round 6'); }); + it('the cap in the retirement note is the plan’s tier, not a constant', () => { + // The third of the four cap call sites. Same history as the cap-5 test + // above, on a 3A-sized plan: round 5's retirement schedules its cold check + // for round 6, which the 3A tier ALLOWS — so the note must promise that + // check rather than close the certificate. The two tests are the same + // scenario with opposite outcomes, which is what makes this site's read of + // the plan observable at all. + writeFileSync( + plan, + JSON.stringify({ ...PLAN, srcDiffLines: 100, diffLines: 100 }), + ); + 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: DRY, 14: YIELD, 15: YIELD }); + answerRound(4, { 13: DRY, 14: YIELD, 15: YIELD }); + + const out = runRound(5); + expect(out).toContain('chunk 13 — retired: dry in rounds 3 and 4'); + expect(out).toContain('next cold check round 6'); + expect(out).not.toContain('certificate final'); + }); + it('the cold check comes due on parity — the retired chunk is built again', () => { answerRound(1, { 13: DRY, 14: YIELD, 15: YIELD }); answerRound(2, { 13: DRY, 14: YIELD, 15: YIELD }); @@ -3932,6 +4073,10 @@ describe('per-chunk retirement — cold territories stop costing a round', () => 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. + // + // Five because `PLAN` carries no `srcDiffLines`/`diffLines`, so the tier + // read is the unsized fallback — deliberately the large tier, which is + // what every plan got before tiering. The sized 3A case is the next test. answerRound(1, { 13: YIELD, 14: YIELD, 15: YIELD }); answerRound(2, { 13: YIELD, 14: YIELD, 15: YIELD }); answerRound(3, { 13: YIELD, 14: YIELD, 15: YIELD }); @@ -3949,6 +4094,36 @@ describe('per-chunk retirement — cold territories stop costing a round', () => expect(msg).toContain('round cap is 5'); }); + it('a 3A-sized plan runs to ten rounds, not five', () => { + // The gate reads the plan's topology tier, so a small diff — where a + // round is one auditor, not one per chunk — keeps auditing where the 3B + // number would have stopped it. Round 6 is the whole change: it is + // refused in the test above and admitted here off the same builder, so a + // revert to a single flat cap fails on the admission, not just on the + // number in the refusal text. + writeFileSync( + plan, + JSON.stringify({ ...PLAN, srcDiffLines: 100, diffLines: 100 }), + ); + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + for (const r of [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]) { + answerRound(r, { 13: YIELD, 14: YIELD, 15: YIELD }); + expect(process.exitCode).toBeUndefined(); + } + expect(keysOf(6)).not.toHaveLength(0); + + const out = runRound(11); + expect(process.exitCode).toBe(4); + expect(out).toBe(''); + expect(keysOf(11)).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 10'); + }); + 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 d486b81c6a3..66331528642 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -139,13 +139,22 @@ interface PlanReport { mergeBaseSha?: unknown; host?: unknown; repositoryContext?: unknown; - budget?: { agentToolBudget?: unknown }; - // The two size fields the topology gate reads (#9242). Declared so the - // per-chunk build paths can notice a fan-out the plan's own numbers never - // asked for; `isTerritoryFanOut` already tolerates the `unknown` via the - // `RosterPlan` cast, same bridge `runRoster` uses. + /** + * The two size fields the topology gate reads (#9242) and the ones + * `reverseAuditRoundCap` derives this plan's round-cap tier from — the same + * pair, read by two callers for two reasons, which is why one declaration + * serves both. Declared even though those functions take `unknown` (they + * parse a file, so they validate at runtime whatever the type says) because + * the declaration is what makes the coupling visible: without it a rename on + * the writing side compiles clean, the per-chunk paths stop noticing a + * fan-out the plan never asked for, and every cap here silently collapses to + * the fallback tier — a quieter failure than a wrong number. + * `isTerritoryFanOut` tolerates the `unknown` via the `RosterPlan` cast, the + * same bridge `runRoster` uses. + */ srcDiffLines?: unknown; diffLines?: unknown; + budget?: { agentToolBudget?: unknown; reverseAuditRounds?: unknown }; } /** A heavy file's entry, which is the only kind an invariant agent can be built from. */ @@ -1993,7 +2002,9 @@ function admitReverseAuditRound( fanOutWidth: 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 + // deadline arithmetic. One value per topology (`reverseAuditRoundTier`) — + // ten on a 3A diff, where a round is one auditor; five on a 3B one, where + // it is one per non-retired chunk; a reduced three 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 @@ -2166,7 +2177,7 @@ function runAllChunks( !admitReverseAuditRound( planPath, round, - reverseAuditRoundCap(report.budget), + reverseAuditRoundCap(report), chunks.length, ) ) { @@ -2222,7 +2233,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 planRoundCap = reverseAuditRoundCap(report); const retirementNote = skipped.length === 0 ? [] @@ -2585,7 +2596,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { !admitReverseAuditRound( args.plan, args.round, - reverseAuditRoundCap(report.budget), + reverseAuditRoundCap(report), 1, ) ) { @@ -2668,7 +2679,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { !admitReverseAuditRound( args.plan, args.round, - reverseAuditRoundCap(report.budget), + reverseAuditRoundCap(report), planChunkIds.length, ) ) diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index a53fcb253ec..e97fa8969e9 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -44,7 +44,7 @@ import { roundCapStopDisclosure, readBudgetStop, } from './lib/deadline.js'; -import { MAX_REVERSE_AUDIT_ROUNDS } from './lib/budget.js'; +import { LARGE_REVERSE_AUDIT_ROUNDS } from './lib/budget.js'; import { shellQuotePath } from './lib/shell-quote.js'; import { HOSTNAME_RE, @@ -973,7 +973,9 @@ function composeReviewBody( } budgetEntry = isRoundCap ? roundCapStopDisclosure( - typeof stop.cap === 'number' ? stop.cap : MAX_REVERSE_AUDIT_ROUNDS, + typeof stop.cap === 'number' + ? stop.cap + : LARGE_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 f633fa6221d..b44bec544c7 100644 --- a/packages/cli/src/commands/review/lib/budget.test.ts +++ b/packages/cli/src/commands/review/lib/budget.test.ts @@ -546,15 +546,18 @@ 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 — + it('gives each topology its own cap: ten on 3A, five on 3B, three when huge', () => { + // The cap prices a ROUND, and a round costs two orders of magnitude more + // in one topology than another: one auditor on 3A, one per non-retired + // chunk on 3B, ~90 min on a huge PR (five of which cannot finish the PRs + // that timed out to zero). Hence three tiers rather than one number — + // ten on 3A because the marginal round there is a single agent on a diff + // small enough to hold in one context, three when huge because that 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). expect( reviewBudget({ srcDiffLines: 100, diffLines: 100 }).reverseAuditRounds, - ).toBe(5); + ).toBe(10); expect( reviewBudget({ srcDiffLines: 2999, diffLines: 2999 }).reverseAuditRounds, ).toBe(5); @@ -567,6 +570,27 @@ describe('reviewBudget — the reverse-audit round cap', () => { ).toBe(3); }); + it('switches tiers on the topology gate, not on a second set of numbers', () => { + // The 3A/3B boundary is `isTerritoryFanOut`'s — src ≤ 500 AND total ≤ + // 3200 — and this is what pins that the cap reads that predicate rather + // than a copy of its constants that could drift from it. Both clauses: + // either one crossing moves the plan to the 3B cap. + expect( + reviewBudget({ srcDiffLines: 500, diffLines: 3200 }).reverseAuditRounds, + ).toBe(10); + expect( + reviewBudget({ srcDiffLines: 501, diffLines: 3200 }).reverseAuditRounds, + ).toBe(5); + expect( + reviewBudget({ srcDiffLines: 500, diffLines: 3201 }).reverseAuditRounds, + ).toBe(5); + // …and the huge floor wins over the topology gate: a 3B diff at 3000 + // effective lines caps at three, not five. + expect( + reviewBudget({ srcDiffLines: 3000, diffLines: 3200 }).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. @@ -590,21 +614,175 @@ describe('reviewBudget — the reverse-audit round cap', () => { }); 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. + const SMALL = { srcDiffLines: 100, diffLines: 100 }; + const LARGE = { srcDiffLines: 600, diffLines: 1000 }; + const HUGE = { srcDiffLines: 5000, diffLines: 5000 }; + + it('passes a value the plan owns through, at every tier', () => { + expect( + reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 10 } }), + ).toBe(10); + expect( + reverseAuditRoundCap({ ...LARGE, budget: { reverseAuditRounds: 5 } }), + ).toBe(5); + expect( + reverseAuditRoundCap({ ...HUGE, budget: { reverseAuditRounds: 3 } }), + ).toBe(3); + // Below its tier but in band is honoured — nothing here inflates a cap + // the plan itself wrote smaller. + expect( + reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 5 } }), + ).toBe(5); + }); + + it('honours a stored value in the INTERIOR of the tier-relative band', () => { + // Every other case here sits on a boundary — at the floor, at the tier, or + // outside — and a boundary-only suite cannot tell a tier-relative bound + // from a constant one: mutating `v <= tier` to `v <= LARGE_…ROUNDS` passed + // all 57 tests before these three. The interior is where the two differ, + // and it is reachable in production, not just by hand: the operator round + // ceiling writes exactly such a value into the plan. + expect( + reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 7 } }), + ).toBe(7); + expect( + reverseAuditRoundCap({ ...LARGE, budget: { reverseAuditRounds: 4 } }), + ).toBe(4); + // The same mutation read from the other side: a constant bound of five + // would hand a HUGE plan the four rounds it stores, past the finishability + // tier that is the whole reason that tier exists. + expect( + reverseAuditRoundCap({ ...HUGE, budget: { reverseAuditRounds: 4 } }), + ).toBe(3); + }); + + it('clamps to the plan’s own tier, so a hand edit cannot cross topologies', () => { + // The field is CLI-written and nothing here is the caller's to override. + // A single global upper bound of ten would have honoured all three of + // these; the tier is what makes them buy nothing. + expect( + reverseAuditRoundCap({ ...HUGE, budget: { reverseAuditRounds: 10 } }), + ).toBe(3); + expect( + reverseAuditRoundCap({ ...LARGE, budget: { reverseAuditRounds: 10 } }), + ).toBe(5); + expect( + reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 11 } }), + ).toBe(10); + }); + + it('reads absent, out-of-band and garbled values as the tier', () => { + // The range is floored at HUGE_REVERSE_AUDIT_ROUNDS (3) — the smallest + // cap the CLI writes — so 1 and 2 read as the tier, not as themselves: + // honouring them would force a non-converged round-cap stop where the + // full loop would have kept auditing. + for (const bad of [0, 1, 2, 2.5, '1', null] as unknown[]) { + expect( + reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: bad } }), + ).toBe(10); + expect( + reverseAuditRoundCap({ ...HUGE, budget: { reverseAuditRounds: bad } }), + ).toBe(3); + } + // A plan with no `reverseAuditRounds` at all reads as its tier. A plan + // that HAS one in band keeps it — see the legacy-value test below. + expect(reverseAuditRoundCap(SMALL)).toBe(10); + expect(reverseAuditRoundCap({ ...SMALL, budget: {} })).toBe(10); + expect(reverseAuditRoundCap(HUGE)).toBe(3); + }); + + it('rejects an in-band NON-INTEGER — the only guard that can catch it', () => { + // 2.5 above never reaches `Number.isInteger`: the `>= 3` floor rejects it + // first, so deleting the integer guard leaves the whole suite green and + // `reverseAuditRounds: 3.5` becomes a cap of 3.5. These values sit inside + // every tier's band, so the integer check is the ONLY thing between them + // and a fractional round cap. + expect( + reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 3.5 } }), + ).toBe(10); + expect( + reverseAuditRoundCap({ ...LARGE, budget: { reverseAuditRounds: 4.5 } }), + ).toBe(5); + }); + + it('honours a legacy in-band value instead of migrating it to the tier', () => { + // A CLI that predates tiering wrote 5, and 5 is inside a 3A plan's [3, 10] + // band. The plan states a number; a reader of a CLI-written field does not + // get to override it. Only an absent or out-of-band value reaches the tier + // — pinning the claim the doc comment used to get wrong in the other + // direction ("a legacy 3A plan gets the ten rounds its topology earns"). + expect( + reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 5 } }), + ).toBe(5); + expect( + reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 3 } }), + ).toBe(3); + }); + + it('treats a COERCIBLE garbage size as unsized, not as a zero-line diff', () => { + // `Number()` turns each of these into a finite 0, so a coerce-then- + // isFinite check calls them usable and hands the plan the SMALL tier — + // ten rounds, the most expensive cap — for a plan whose size is not + // known. `JSON.stringify` writes a NaN line count as `null`, so this is + // the shape the corrupted plan actually arrives in. + for (const bad of [null, '', false, [], -5, '1', '3000'] as unknown[]) { + expect(reverseAuditRoundCap({ srcDiffLines: bad, diffLines: bad })).toBe( + 5, + ); + // …including when only ONE of the pair is garbage. + expect(reverseAuditRoundCap({ srcDiffLines: 100, diffLines: bad })).toBe( + 5, + ); + expect(reverseAuditRoundCap({ srcDiffLines: bad, diffLines: 100 })).toBe( + 5, + ); + } + // A numeric-string size must not buy a tier a hand edit could not: "1" + // would otherwise coerce a 5,800-line plan into the SMALL tier through + // the very clamp that exists to stop it. + expect( + reverseAuditRoundCap({ + srcDiffLines: '1', + diffLines: '1', + budget: { reverseAuditRounds: 10 }, + }), + ).toBe(5); + // Zero is a real size — an empty diff is a small diff, not an unsized one. + expect(reverseAuditRoundCap({ srcDiffLines: 0, diffLines: 0 })).toBe(10); + }); + + it('does not let reviewBudget RECORD a tier for a size it never received', () => { + // The write path is the other half: `sane()` launders garbage into 0, and + // 0 is a perfectly good small diff, so sizing the tier from the saned pair + // persisted ten rounds into the plan where the flat cap persisted five. + for (const bad of [Number.NaN, -5, Number.POSITIVE_INFINITY]) { + expect( + reviewBudget({ srcDiffLines: bad, diffLines: bad }).reverseAuditRounds, + ).toBe(5); + } + expect(reviewBudget({} as never).reverseAuditRounds).toBe(5); + // …while a genuinely empty diff still records the small tier. + expect( + reviewBudget({ srcDiffLines: 0, diffLines: 0 }).reverseAuditRounds, + ).toBe(10); + }); + + it('reads a plan with no usable size as the large tier — today’s value', () => { + // The skew case cannot be sized, and an unsized plan could be the + // 5,800-line one. Falling back to the large tier means such a plan is + // never handed MORE rounds than every plan already runs with. 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); + expect(reverseAuditRoundCap({ budget: { reverseAuditRounds: 10 } })).toBe( + 5, + ); + expect( + reverseAuditRoundCap({ + srcDiffLines: 'x', + diffLines: 10, + budget: { reverseAuditRounds: 10 }, + }), + ).toBe(5); }); }); diff --git a/packages/cli/src/commands/review/lib/budget.ts b/packages/cli/src/commands/review/lib/budget.ts index 3eb70bdc60f..cb984d0129a 100644 --- a/packages/cli/src/commands/review/lib/budget.ts +++ b/packages/cli/src/commands/review/lib/budget.ts @@ -98,25 +98,29 @@ export interface ReviewBudget { */ 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. + * The reverse-audit loop's round cap, **one value per topology** + * (`reverseAuditRoundTier`): `SMALL_REVERSE_AUDIT_ROUNDS` on a 3A diff, + * `LARGE_REVERSE_AUDIT_ROUNDS` on a 3B one, and 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 + * A reverse-audit round re-reads the diff against a growing findings + * list, so its cost scales with the diff — one auditor on 3A, one per + * non-retired chunk on 3B, and ~90 minutes a round on a 4,000-line PR, + * where five rounds alone (450 min) exceed the six-hour CI ceiling before + * the fan-out and tail are even counted. That spread is why this is not + * one number: the same cap cannot price a single agent and a 19-way + * fan-out. 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. The huge tier is 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. + * only ever writes one of the three tier values here. */ reverseAuditRounds: number; } @@ -128,11 +132,28 @@ 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. `compose-review` imports it directly. + * The reverse-audit round cap for a **3A** diff (SKILL.md Step 5's "stop at + * the plan's `reverseAuditRounds` cap"). + * + * Ten, because on 3A a round is **one auditor reading the whole diff** — the + * marginal round is a single agent on a diff small enough to hold in one + * context, against a whole review of 17-28 calls (17-23 before this tier, so + * the five extra rounds are five calls). Five was never a 3A price: + * it is the 3B arithmetic (`rounds × chunks`) applied to a topology where + * that arithmetic does not hold, and it stopped loops that were still + * confirming Criticals for a saving of ~5 calls. The loop's real terminator + * is two consecutive dry rounds; every cap here is the belt under it. */ -export const MAX_REVERSE_AUDIT_ROUNDS = 5; +export const SMALL_REVERSE_AUDIT_ROUNDS = 10; + +/** + * The reverse-audit round cap for a **3B** diff — the historical value, and + * still the right one where a round costs one auditor per non-retired chunk + * (`19 × 5 = 95` on PR #6457's shape, before retirement trims the odd + * rounds). `compose-review` imports it directly as the cap it names when a + * stop marker arrives without one. + */ +export const LARGE_REVERSE_AUDIT_ROUNDS = 5; /** * The reduced cap for a huge diff — three, one audit round above the @@ -156,6 +177,113 @@ export const HUGE_REVERSE_AUDIT_ROUNDS = 3; */ const HUGE_DIFF_FLOOR = 3000; +/** + * The topology gate's two numbers — the same pair the skill's prose turns on + * (SKILL.md Step 3: "`srcDiffLines` ≤ 500 and `diffLines` ≤ 3200 — use the + * dimension fan-out in Step 3A"). + */ +const FAN_OUT_SRC_FLOOR = 500; +const FAN_OUT_TOTAL_FLOOR = 3200; + +/** A plan, as far as a size-derived decision needs it. */ +export interface DiffSize { + srcDiffLines?: unknown; + diffLines?: unknown; +} + +/** + * The topology gate, in code. + * + * The same two numbers the skill's prose turns on. It is here so the roster, + * the reader and the round cap cannot disagree about which fan-out was owed — + * a disagreement that would show up as a review being told it forgot eleven + * agents it was never supposed to launch. It lives in `budget.ts` rather than + * in `roster.ts` because it is a *size* ruling and this module is where size + * rulings live; `roster.ts` imports it back (this module has no imports of its + * own, so the direction cannot cycle). + */ +export function isTerritoryFanOut(plan: DiffSize): boolean { + const src = Number(plan?.srcDiffLines ?? 0); + const total = Number(plan?.diffLines ?? 0); + return !(src <= FAN_OUT_SRC_FLOOR && total <= FAN_OUT_TOTAL_FLOOR); +} + +/** + * The reverse-audit round cap this diff's **topology** earns. + * + * One number per topology, because the thing being capped costs two orders of + * magnitude more in one than in another: a 3A round is one auditor (minutes), + * a 3B round is one auditor per non-retired chunk, and a huge-diff round is + * ~90 minutes. A single cap is therefore either useless at one end or + * crippling at the other, and five was both — too loose to bound the huge + * case (the 6-hour CI reviews that posted nothing) and tight enough on 3A to + * stop loops that were still confirming Criticals. + * + * The huge tier is checked first and wins: it is a *finishability* ruling, and + * a huge diff is territory-fanned-out by construction anyway. + * + * A plan carrying no usable size — an older CLI's, or a garbled one — reads as + * the LARGE tier, which is what every plan gets today. The skew case is + * therefore never handed more rounds than it already runs with, which is the + * safe direction for a bound (the rest of this module's fallbacks err toward + * more *coverage*; this one errs toward less *cost*, because an unsized plan + * could be the 5,800-line one). + * + * **Usability is judged before coercion, not after.** `Number()` turns `null`, + * `''`, `false` and `[]` into a finite `0`, so a coerce-then-`isFinite` check + * calls them usable and hands a plan whose sizes are unknowable the SMALL + * tier — the most expensive one — while the sibling `{}` correctly falls back. + * That shape is not hypothetical: `JSON.stringify` writes a `NaN` line count + * as `null`, so the corrupted plan this fallback exists for arrives looking + * exactly like a zero-line diff. A numeric-string size (`"1"`) coerces too, + * which would have let a hand-edited huge plan reach the SMALL tier through + * the very clamp `reverseAuditRoundCap` adds to prevent it. + */ +export function reverseAuditRoundTier(size: DiffSize): number { + const src = size?.srcDiffLines; + const total = size?.diffLines; + if (!usableLineCount(src) || !usableLineCount(total)) { + return LARGE_REVERSE_AUDIT_ROUNDS; + } + const effective = effectiveLines(src, total); + if (effective >= HUGE_DIFF_FLOOR) return HUGE_REVERSE_AUDIT_ROUNDS; + // The validated pair, not `size` again. Re-reading the raw object here would + // give the huge gate and the topology gate two independent derivations of + // the same two numbers inside one function — which is exactly the shape of + // the defect this function was just repaired for, where one derivation + // laundered garbage the other rejected. + return isTerritoryFanOut({ srcDiffLines: src, diffLines: total }) + ? LARGE_REVERSE_AUDIT_ROUNDS + : SMALL_REVERSE_AUDIT_ROUNDS; +} + +/** + * A line count this module is willing to size a plan from: a real, finite, + * non-negative `number`. Everything else — absent, `null`, a numeric string, a + * boolean, `NaN`, `Infinity`, a negative — is a plan whose size is not known, + * which is a different fact from a plan whose size is zero. + * + * Deliberately NOT shared with `sane()` below, which answers the opposite + * question. `sane()` launders garbage into `0` because its readers (angles, + * the sweep, the tool budget) have a safe floor to land on; the tier has no + * such floor — landing on `0` there means "small diff", the costliest cap. + */ +function usableLineCount(v: unknown): v is number { + return typeof v === 'number' && Number.isFinite(v) && v >= 0; +} + +/** + * The plan's source-weighted line-span measure, in one place. + * + * A diff that is *all* non-source (docs, a lockfile) still has lines somebody + * has to read, so a large non-source diff counts at a coarser eighth rate. + * Both size tiers in this module read it, and two copies of it are two size + * measures that can drift apart inside one budget object. + */ +function effectiveLines(src: number, total: number): number { + return Math.max(src, Math.floor(total / 8)); +} + /** Below this, "one domain dominates the diff" is not a finding about the diff. */ const SPECIALIST_FLOOR = 80; @@ -191,13 +319,7 @@ export function reviewBudget(input: BudgetInput): ReviewBudget { const src = sane(input.srcDiffLines); const total = sane(input.diffLines); - // Size is read from source lines, with one exception: a diff that is *all* - // non-source (a docs-only or lockfile-only change) still has lines somebody - // has to read, and reading them with three angles when there are two thousand - // of them is the dilution this budget exists to avoid. So a large non-source - // diff earns angles too, at a much coarser rate — prose carries less that a - // reviewer can get wrong, not none. - const effective = Math.max(src, Math.floor(total / 8)); + const effective = effectiveLines(src, total); const extraAngles = Math.floor(effective / LINES_PER_ANGLE); const inlineAngles = clamp( @@ -223,38 +345,63 @@ export function reviewBudget(input: BudgetInput): ReviewBudget { MIN_AGENT_TOOL_BUDGET, MAX_AGENT_TOOL_BUDGET, ), - reverseAuditRounds: - effective >= HUGE_DIFF_FLOOR - ? HUGE_REVERSE_AUDIT_ROUNDS - : MAX_REVERSE_AUDIT_ROUNDS, + // The RAW input, not the `sane()`d pair above. `sane()` launders a garbled + // count into `0`, and `0` is a perfectly good small diff — so sizing the + // tier from it would record the SMALL tier's ten rounds for a plan whose + // size failed to arrive, where the flat cap recorded five. The tier does + // its own usability check precisely so this call can hand it the truth. + reverseAuditRounds: reverseAuditRoundTier(input), }; } /** - * The reverse-audit round cap a plan's budget carries, for every reader - * that enforces or narrates it (the admission gate and the cold-check - * note, both in `agent-prompt`; the retirement scheduler deliberately - * ignores the cap — whether a scheduled cold check is allowed is the note - * composer's question, not the schedule's). 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 reverse-audit round cap a **plan** carries, for every reader that + * enforces or narrates it (the admission gate and the cold-check note, both + * in `agent-prompt`; the retirement scheduler deliberately ignores the cap — + * whether a scheduled cold check is allowed is the note composer's question, + * not the schedule's). + * + * It takes the whole plan, not `plan.budget`, because the accepted range is + * now the plan's **own topology tier** rather than a global band. What that + * buys, stated as what actually happens rather than as a slogan: + * + * - **A hand-edited plan cannot cross tiers.** The field is CLI-written and + * nothing here is the caller's to override; clamping to the tier means a + * `reverseAuditRounds: 10` typed into a 5,800-line plan buys nothing, + * which a single upper bound of ten would have honoured. + * - **A plan with no `reverseAuditRounds` at all** — a pre-budget CLI's — + * reads as its topology's tier instead of one flat number. + * + * Two things this does NOT do, both of which an earlier draft of this comment + * claimed and the code never did: + * + * - It does **not** upgrade a legacy small plan to ten. A CLI that predates + * tiering wrote `reverseAuditRounds: 5`, and 5 is inside a 3A plan's + * `[3, 10]` band, so it is honoured as 5. Only an absent or out-of-band + * value ever reaches the tier. Migrating in-band values would mean + * overriding a number the plan states, which is the one thing a reader of + * a CLI-written field must not do. + * - It does **not** always err toward more auditing. A field-less **huge** + * plan now reads 3 where the flat fallback read 5 — deliberately less: the + * huge tier is a finishability ruling, and the reviews it exists for are + * the ones that ran six hours and posted nothing. * - * 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. + * The range stays 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 tier — + * never less. */ -export function reverseAuditRoundCap(budget: unknown): number { - const v = (budget as { reverseAuditRounds?: unknown } | undefined) - ?.reverseAuditRounds; +export function reverseAuditRoundCap(plan: unknown): number { + const tier = reverseAuditRoundTier((plan ?? {}) as DiffSize); + const v = (plan as { budget?: { reverseAuditRounds?: unknown } } | undefined) + ?.budget?.reverseAuditRounds; return typeof v === 'number' && Number.isInteger(v) && v >= HUGE_REVERSE_AUDIT_ROUNDS && - v <= MAX_REVERSE_AUDIT_ROUNDS + v <= tier ? v - : MAX_REVERSE_AUDIT_ROUNDS; + : tier; } /** diff --git a/packages/cli/src/commands/review/lib/deadline.ts b/packages/cli/src/commands/review/lib/deadline.ts index 8215f65a297..b9368a197b3 100644 --- a/packages/cli/src/commands/review/lib/deadline.ts +++ b/packages/cli/src/commands/review/lib/deadline.ts @@ -9,8 +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 plan's round cap (5, or 3 for a huge -// diff). On a PR where every round +// two consecutive dry rounds or the plan's round cap (one value per topology: +// 10 on a 3A diff, 5 on a 3B one, 3 when huge). 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 diff --git a/packages/cli/src/commands/review/lib/roster.ts b/packages/cli/src/commands/review/lib/roster.ts index 3e78adb5794..fba18f84897 100644 --- a/packages/cli/src/commands/review/lib/roster.ts +++ b/packages/cli/src/commands/review/lib/roster.ts @@ -27,6 +27,11 @@ import type { RepositoryContextRoleId, RoleId } from './agent-briefs.js'; import { repositoryContextOf } from './repository-context.js'; import { pathTool } from '../script-lint.js'; +// The topology gate lives in `budget.ts` — it is a size ruling, and the round +// cap needs the same one. Re-exported here because this file was its home and +// the roster is where a reader looks for "which fan-out was owed". +export { isTerritoryFanOut } from './budget.js'; +import { isTerritoryFanOut } from './budget.js'; /** * How this review's diff was captured — which decides what can be asked of it. @@ -96,20 +101,6 @@ export function reviewMode(plan: RosterPlan): ReviewMode { return 'diff-only'; } -/** - * The topology gate, in code. - * - * The same two numbers the skill's prose turns on. It is here so the roster and - * the reader cannot disagree about which fan-out was owed — a disagreement that - * would show up as a review being told it forgot eleven agents it was never - * supposed to launch. - */ -export function isTerritoryFanOut(plan: RosterPlan): boolean { - const src = Number(plan.srcDiffLines ?? 0); - const total = Number(plan.diffLines ?? 0); - return !(src <= 500 && total <= 3200); -} - /** Does the diff remove or replace anything? If not, 1b has nothing to audit. */ function hasDeletions(plan: RosterPlan): boolean { const files = Array.isArray(plan.files) ? plan.files : []; diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index c6a00638ce3..a0089122169 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -93,7 +93,21 @@ A single reverse audit pass leaves whatever the reverse audit agent itself misse One dry round was the original rule, and PR #6457 shows why it is unsound. The per-round Critical yield across its eight review rounds was `2, 2, 7, 0, 0, 5, 3, 1`. The review returned "no blockers" **twice**, and the next round surfaced five Criticals — three of them in code that had been in the diff since the first commit. A yield of zero is evidence about one round's agents, not about the code. -Requiring two consecutive dry rounds makes a single lazy or context-starved agent unable to end the loop. The hard cap moves from 3 rounds to 5, and when the cap is what stopped the loop the output must say so rather than implying convergence. +Requiring two consecutive dry rounds makes a single lazy or context-starved agent unable to end the loop. The hard cap moves from 3 rounds to 5, and when the cap is what stopped the loop the output must say so rather than implying convergence. (The cap has since become one value per topology — see "Why the round cap is per topology" below.) + +### Why the round cap is per topology + +Five was one number standing in for three prices. What the cap bounds is a **round**, and a round costs one auditor on 3A, one auditor per non-retired chunk on 3B, and ~90 minutes on a huge diff. That is two orders of magnitude across the topologies a single cap had to serve, so it was necessarily wrong at one end: too loose to bound the huge case — which is why `HUGE_REVERSE_AUDIT_ROUNDS` had to be carved out of it — and, at the other end, tight enough on 3A to stop loops that were still confirming Criticals, for a saving of about five calls out of a review that cost 17-23 of them before this change. + +The asymmetry that decides it is the one this whole design is built on: a missed issue costs another `/review` iteration, and per-run cost is the cheaper side of that trade (see "Competitors" below). On 3A the marginal round is a single agent; on huge it is an hour and a half of a six-hour ceiling. The cap should say so. + +So the cap is read from the plan's topology tier (`reverseAuditRoundTier`): **10** on 3A, **5** on 3B, **3** when huge. Three consequences worth naming: + +- **The huge tier is checked first and wins.** It is a finishability ruling, and a huge diff is territory-fanned-out by construction anyway. A 3A diff can never be huge — `effective = max(src, floor(total/8))` is at most `max(500, 400)` under the 3A gate — so the two tiers cannot contend. +- **One predicate, not two sets of numbers.** The tier reads `isTerritoryFanOut`, the same gate the roster turns on, which is why that function moved into `budget.ts`: a second copy of `500`/`3200` would eventually disagree with the roster about which fan-out a review owed. +- **The cap is a belt, not the terminator.** The loop still ends on two consecutive dry rounds, and in a time-budgeted run the deadline gate — which prices the round it is admitting plus the reserve — is the operative bound. The static cap is what a local run (no deadline) has instead, which is exactly why it should not be a number borrowed from another topology's arithmetic. + +What this does **not** change: a cap stop is still a non-converged stop. It writes the marker, caps the verdict, and owes its `unreviewedDimensions` entry, at ten exactly as at five. ### Why the reverse audit fans out per chunk @@ -593,14 +607,14 @@ The countermeasure is cheap and needs no new machinery: before Step 4, sanity-ch ## LLM call budget -**Small diffs (≤ 500 source lines AND ≤ 3200 total diff lines, Step 3A, high effort) — 17-23 calls (typically 17-19):** +**Small diffs (≤ 500 source lines AND ≤ 3200 total diff lines, Step 3A, high effort) — 17-28 calls (typically 17-19):** | Stage | Calls | Why | | ----------------------- | ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Review agents | 14 (+0-2) | issue fidelity + 3 procedural correctness walks (1a/1b/1c) + security + 3 quality slices (3a/3b/3c) + perf/tests + 3 undirected personas + build&test, plus 0-2 diff-specialized finders; cross-repo skips Agents 7 and 1c (12), non-PR skips Agent 0 (13) | | Sharded verification | `ceil(F/8)` | F = findings; typically 1-2; keeps each verifier's job small on high-finding reviews | -| Iterative reverse audit | 2-5 (2-3 huge) | loop ends after two consecutive dry rounds; 5-round hard cap — 3 for a huge diff (effective ≥ 3000 lines) | -| **Total** | **~17-23 (~15-22)** | Row maxima do not co-occur on typical runs (~17-19 is common), but the honest sum of ranges is 17-23 same-repo, 15-22 cross-repo/local. **Low effort: 0 subagent calls** — the angle rotation runs in the orchestrator's own context; medium launches its reduced fan-out | +| Iterative reverse audit | 2-10 | loop ends after two consecutive dry rounds; 10-round hard cap — one auditor a round on 3A, so the marginal round is one call. A 3A diff is never "huge": `effective = max(src, total/8) ≤ max(500, 400)`, so the huge tier cannot reach this table | +| **Total** | **~17-28 (~15-27)** | Row maxima do not co-occur on typical runs (~17-19 is common), but the honest sum of ranges is 17-28 same-repo, 15-27 cross-repo/local. **Low effort: 0 subagent calls** — the angle rotation runs in the orchestrator's own context; medium launches its reduced fan-out | **Large diffs (> 500 source lines OR > 3200 total diff lines, Step 3B, high effort) — `ceil(diffLines / 400)` chunk agents + `5..7` whole-diff agents + `3H` invariant agents (H = heavy files) + `ceil(F/8)` verify (F = findings) + `rounds × chunks` reverse audit.** The reverse audit dominates: it fans out one auditor per chunk per round, and the stop rule needs two consecutive dry rounds (hard cap 5 — **3 for a huge diff, effective ≥ 3000 lines**, which narrows the per-chunk multiplier to `2..3`). PR #6457 (5801 diff lines, 19 chunks, 1 heavy file) costs ~27-29 first-wave calls, then `19 × (2..5) = 38-95` reverse auditors — ~66-126 calls total depending on how long the audit keeps finding (a huge-diff cap-3 run of the same shape narrows this to `19 × (2..3) = 38-57`); ~70 is the clean-run floor, and the count scales with chunks and findings, not a fixed ceiling. @@ -688,22 +702,22 @@ The convergence concern that motivated the summary is real but narrower than it For a PR with 15 findings: -| Approach | LLM calls | Notes | -| --------------------------------------------------- | ------------------ | ----------------------------------------------------------------------------------------------------------------------- | -| Copilot (1 agent) | 1 | Lowest cost, lowest coverage | -| Gemini (2 LLM tasks) | 2 | Good cost, medium coverage | -| Our design (5 agents, N verify) | 21 | 5+15+1 — too expensive | -| Our design (5 agents, batch verify, single reverse) | 7 | 5+1+1 — original design | -| Our design (9 agents, iterative reverse) | 11-13 | 9+1+(1-3) — +50% cost for meaningfully higher recall | -| Our design (10 agents) | 12-14 | 10+1+(1-3) — adds issue-fidelity/root-cause gate | -| Our design (14 agents + effort levels, current) | 17-23 high / 0 low | 14(+0-2)+ceil(F/8)+(2-5) under 3A; low runs inline with no subagents, 3-6 angles by diff size — cost scales with intent | -| Claude /ultrareview | 5-20 | Cloud-hosted, cost on Anthropic | +| Approach | LLM calls | Notes | +| --------------------------------------------------- | ------------------ | ------------------------------------------------------------------------------------------------------------------------ | +| Copilot (1 agent) | 1 | Lowest cost, lowest coverage | +| Gemini (2 LLM tasks) | 2 | Good cost, medium coverage | +| Our design (5 agents, N verify) | 21 | 5+15+1 — too expensive | +| Our design (5 agents, batch verify, single reverse) | 7 | 5+1+1 — original design | +| Our design (9 agents, iterative reverse) | 11-13 | 9+1+(1-3) — +50% cost for meaningfully higher recall | +| Our design (10 agents) | 12-14 | 10+1+(1-3) — adds issue-fidelity/root-cause gate | +| Our design (14 agents + effort levels, current) | 17-28 high / 0 low | 14(+0-2)+ceil(F/8)+(2-10) under 3A; low runs inline with no subagents, 3-6 angles by diff size — cost scales with intent | +| Claude /ultrareview | 5-20 | Cloud-hosted, cost on Anthropic | ## Future optimization: Fork Subagent > Dependency: [Fork Subagent proposal](https://github.com/wenshao/codeagents/blob/main/docs/comparison/qwen-code-improvement-report-p0-p1-core.md#2-fork-subagentp0) -**Current problem:** Each of the ~17-23 LLM calls (14-16 review + sharded verify + 2-5 reverse audit rounds) creates a new subagent from scratch. At ~52K per agent (50K system + 2K task), that is ~880K-1.2M input tokens with massive redundancy. The cost grew along with the agent count — Fork Subagent matters even more under the current 14-agent design than under the original 5-agent design. (Effort levels bound the cost from the other side: low runs spawn no subagents at all, and medium spawns the reduced fan-out.) +**Current problem:** Each of the ~17-28 LLM calls (14-16 review + sharded verify + 2-10 reverse audit rounds) creates a new subagent from scratch. At ~52K per agent (50K system + 2K task), that is ~880K-1.5M input tokens with massive redundancy. The cost grew along with the agent count — Fork Subagent matters even more under the current 14-agent design than under the original 5-agent design. (Effort levels bound the cost from the other side: low runs spawn no subagents at all, and medium spawns the reduced fan-out.) **Fork Subagent solution:** Instead of creating independent subagents, fork the current conversation. All forks inherit the parent's full context (system prompt, conversation history, Step 1/1.1/1.5 results) and share a prompt cache prefix. The API caches the common prefix once; each fork only pays for its unique delta (~2K per agent). @@ -711,13 +725,13 @@ For a PR with 15 findings: Current (independent subagents): Agent 1: [50K system] + [2K task] = 52K Agent 2: [50K system] + [2K task] = 52K - ...× 17-23 agents = ~880K-1.2M total input tokens + ...× 17-28 agents = ~880K-1.5M total input tokens With Fork + prompt cache sharing: Cached prefix: [50K system + conversation history] (cached once) Fork 1: [cache hit] + [2K delta] = ~2K effective Fork 2: [cache hit] + [2K delta] = ~2K effective - ...× 17-23 forks = ~50K cached + ~34-46K delta = ~84-96K total + ...× 17-28 forks = ~50K cached + ~34-56K delta = ~84-106K total ``` **Additional benefits for /review:** @@ -727,7 +741,7 @@ With Fork + prompt cache sharing: - Verification and reverse audit agents inherit all prior findings naturally - Agent 6 personas can fork from a shared diff-loaded base, paying only the persona-framing delta -**Estimated savings:** ~88-92% token reduction (~780K-1.1M → ~80-92K) with zero quality impact. The savings ratio is now even more compelling than under the 5-agent design. +**Estimated savings:** ~90-93% token reduction (~880K-1.5M → ~84-106K) with zero quality impact. The savings ratio is now even more compelling than under the 5-agent design. **Why not implemented now:** Fork Subagent requires changes to the Qwen Code core (`AgentTool`, `forkSubagent.ts`, `CacheSafeParams`). This is a platform-level feature (~400 lines, ~5 days), not a /review-specific change. When available, /review should be updated to use fork instead of independent subagents. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index a3194407d1f..666795c76dc 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -204,7 +204,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, 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. +- `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, **one value per topology**: **10** on a Step 3A diff, **5** on a Step 3B one, **3 for a huge diff** (effective ≥ 3000 lines). One number cannot price all three, because what is being capped is a _round_ and a round costs one auditor on 3A, one auditor per non-retired chunk on 3B, and ~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, and the 6-hour timeouts that posted nothing were 4,000-5,300-line PRs (measured; DESIGN.md — The six-hour timeouts). Ten on 3A because the marginal round there is a single agent against a whole review of 17-28 calls: five was the 3B arithmetic applied where it does not hold, and it stopped loops that were still confirming Criticals to save ~5 calls. Three when huge 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: walk all six angles, run the sweep, cap Agent 8 at 2, shard verification at 8. Those four err toward more coverage, never less. The round cap is the one exception and is worth naming rather than lumping in: a field-less **huge** plan reads 3 where the flat fallback read 5 — deliberately _less_, because that tier is a finishability ruling and the reviews it exists for are the ones that ran six hours and posted nothing. A chunk is read with `read_file(file_path=diffPathAbsolute, offset=startLine - 1, limit=endLine - startLine + 1)` — `offset` is 0-based. @@ -684,8 +684,8 @@ 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). If an earlier round-cap or budget refusal told you to add its stop entry to `unreviewedDimensions`, remove it now — this convergence supersedes that stop (the marker on disk is cleared the same way). 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 verify them before Step 6 through `agent-prompt --role verify` **only** (never a hand-rolled agent), under the same bounded tail as the budget stop below: that builder is gated on the compose floor and refuses once too little time remains, and when the deadline is within the floor you stop waiting on any verifier batch still out and compose with the tags in hand — no fresh re-verification pass, and nothing already confirmed re-verified. This matters most on exactly the huge diffs the cap targets: a time-budgeted CI run that stops at the cap with ~30-90 minutes left must not spend it on an unbounded tail and die before compose. The tag backstop below (and `compose-review`'s machine-read of it) is what catches a miss. +- **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). If an earlier round-cap or budget refusal told you to add its stop entry to `unreviewedDimensions`, remove it now — this convergence supersedes that stop (the marker on disk is cleared the same way). 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 on a 3B diff, round 3 under the huge-diff cap — this ledger is 3B's, so the 3A tier's ten never applies here). (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** — 10 on a 3A diff, 5 on a 3B one, 3 for a huge diff (effective ≥ 3000 lines) — and say so in the output rather than implying convergence. The cap is per topology because it prices a round, and a 3A round is one auditor where a huge-diff round is ~90 minutes; you never work this out yourself, the builder reads the plan's tier. 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 verify them before Step 6 through `agent-prompt --role verify` **only** (never a hand-rolled agent), under the same bounded tail as the budget stop below: that builder is gated on the compose floor and refuses once too little time remains, and when the deadline is within the floor you stop waiting on any verifier batch still out and compose with the tags in hand — no fresh re-verification pass, and nothing already confirmed re-verified. This matters most on exactly the huge diffs the cap targets: a time-budgeted CI run that stops at the cap with ~30-90 minutes left must not spend it on an unbounded tail and die before compose. 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, whole-diff on 3A and per-chunk rounds 1 and 2 on 3B. The convergence pair is the one exception on the LAUNCH side: a pair member's return never triggers this rule per member — round 2's auditors are already in flight — and the pair bullets above define the one transition; the pair's findings still verify as the k=2 case, riding round 3.) 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.