diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 47695e518b8..998d4556c4a 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -56,6 +56,7 @@ import { findingsSection, agentPromptCommand, } from './agent-prompt.js'; +import { BRIEFS } from './lib/agent-briefs.js'; import { readRecordedPrompts, briefPath, @@ -3665,3 +3666,315 @@ describe('per-chunk retirement — cold territories stop costing a round', () => expect(msg).toContain('CONVERGED'); }); }); + +describe('the tool budget in the briefs', () => { + // The untyped literal exists so tests can spread it (`as never` cannot be + // spread); `budgetPlan` is the cast the builders take. + const budgetPlanObj = { + ...PLAN, + // Role 0 refuses to build without a PR to check issues against. + prNumber: '6771', + ownerRepo: 'QwenLM/qwen-code', + files: [ + { + path: 'big.ts', + kind: 'source', + heavy: true, + addedLines: 300, + removedLines: 100, + }, + ], + budget: { + inlineAngles: 4, + sweep: true, + specialistCap: 2, + verifyShard: 8, + agentToolBudget: 42, + }, + }; + const budgetPlan = budgetPlanObj as never; + + it('scopes a chunk agent to its own territory, not the whole plan', () => { + // Chunk 13 is 217 lines / 9,000 chars: allowance min(plan 42, 30+217/20 + // = 40) = 40, plus its reading list (brief + one diff page). Handing it + // the whole-diff number instead keeps exactly the wandering headroom the + // budget exists to cut. + expect(buildChunkAgentPrompt(budgetPlan, 13)).toContain( + 'About **42 tool calls**', + ); + // Chunk 14's 40,000 chars take two reads to page through: brief + two + // pages ride on top of its 38-call allowance. + expect(buildChunkAgentPrompt(budgetPlan, 14)).toContain( + 'About **41 tool calls**', + ); + }); + + it('an UNCOVERABLE chunk gets no budget block at all', () => { + // Chunk 15's instruction is to return the exact `Uncoverable:` line and + // stop. A budget block telling it to "write your findings from the + // evidence in hand" beside that is two contradicting masters — and an + // agent following the budget's format never matches the uncoverable + // parser, turning a disclosed gap into a hard coverage failure. + const p = buildChunkAgentPrompt(budgetPlan, 15); + expect(p).toContain('Uncoverable: chunk 15'); + expect(p).not.toContain('Tool budget'); + }); + + it('gives a whole-diff role the plan allowance plus its reading list', () => { + // 42 from the plan + its brief + every chunk's PAGES (1 + 2 + 3 = 6 + // for the fixture's 9k/40k/60k-char chunks) — an oversized chunk's + // `isTruncated` paging must not be paid out of the analysis allowance. + for (const role of ['1a', '2', '6b'] as const) { + expect(buildRoleBrief(budgetPlan, role)).toContain( + 'About **49 tool calls**', + ); + } + // The chunkless (Step 3A) reverse auditor also owes the cumulative + // findings list its brief orders read in full — same three pages the + // chunk-scoped branch counts, keyed on `acceptsFindings`. + expect(buildRoleBrief(budgetPlan, 'reverse-audit')).toContain( + 'About **52 tool calls**', + ); + }); + + it('a chunk-scoped reverse auditor gets its chunk, not the diff', () => { + // Chunk 13's 40-call allowance + brief + one diff page + the cumulative + // findings list its brief orders read in full (measured 65-82 KB). + expect( + buildRoleBrief(budgetPlan, 'reverse-audit', { chunk: 13 }), + ).toContain('About **45 tool calls**'); + }); + + it('an invariant agent budgets on its file, reads scaled by its size', () => { + // 300 added + 100 removed lines: territory allowance min(42, 30+400/20 + // = 50) = 42, plus reads max(4, 2 + ceil(300/500)) = 4. The reads floor + // at the old flat 4 and grow with the added lines a heavy rewrite pages + // through — a flat count once told a 400 KB file's agent its mandatory + // paging was already overspending. + expect( + buildRoleBrief(budgetPlan, 'invariant-a', { file: 'big.ts' }), + ).toContain('About **46 tool calls**'); + }); + + it('invariant reads scale with the file, past the floor', () => { + // The fixture sits ABOVE both thresholds it pins — a +300-line file's + // reads land on the flat-4 floor, so a mutant deleting the scaling + // term entirely stayed green. 3,200 post-change lines: reads = 2 + 7 = + // 9, territory 3000 → min(plan 60, cap 60) = 60 → 69 (a flat 4 gives + // 64). + const big = { + ...budgetPlanObj, + files: [ + { + path: 'huge.ts', + kind: 'source', + heavy: true, + addedLines: 3000, + removedLines: 0, + fileLines: 3200, + addedRanges: [{ start: 10, end: 3010 }], + diffRange: { startLine: 1, endLine: 3600 }, + }, + ], + budget: { agentToolBudget: 60 }, + } as never; + expect(buildRoleBrief(big, 'invariant-a', { file: 'huge.ts' })).toContain( + 'About **69 tool calls**', + ); + }); + + it('removed lines are territory too — a gutting rewrite is not 200 lines', () => { + // added 200 / removed 800: territory 1000 → allowance 60. An + // added-only derivation would hand this launch 40. + const gutted = { + ...budgetPlanObj, + files: [ + { + path: 'gut.ts', + kind: 'source', + heavy: true, + addedLines: 200, + removedLines: 800, + fileLines: 400, + addedRanges: [{ start: 1, end: 200 }], + diffRange: { startLine: 1, endLine: 1100 }, + }, + ], + budget: { agentToolBudget: 60 }, + } as never; + expect(buildRoleBrief(gutted, 'invariant-a', { file: 'gut.ts' })).toContain( + 'About **64 tool calls**', + ); + }); + + it('a volume-heavy file budgets its paging from fileLines, not added lines', () => { + // A file can go heavy by VOLUME: ~450 added lines in a 9,000-line + // file. The brief mandates paging the WHOLE post-change file — 18 + // pages, not the 1 the added lines suggest. reads = max(4, 2 + 18) = + // 20; territory 450 → min(60, 52) = 52 → 72. The added-only estimate + // told exactly this agent its mandatory reading was overspending (56). + const voluminous = { + ...budgetPlanObj, + files: [ + { + path: 'vol.ts', + kind: 'source', + heavy: true, + addedLines: 450, + removedLines: 0, + fileLines: 9000, + addedRanges: [{ start: 100, end: 550 }], + diffRange: { startLine: 1, endLine: 700 }, + }, + ], + budget: { agentToolBudget: 60 }, + } as never; + expect( + buildRoleBrief(voluminous, 'invariant-a', { file: 'vol.ts' }), + ).toContain('About **72 tool calls**'); + }); + + it('chunk territory is source-weighted, like the plan allowance it mirrors', () => { + // A 640-line chunk that is 80 source lines + 560 lockfile lines is + // not 640 lines of risk: weighted = 640·(80 + 560/8)/640 = 150 → + // allowance min(42, 30 + 7) = 37, reads 2 → 39. Raw-lines scaling + // handed this chunk min(42, 60) = 42 — and the inversion the finding + // measured: the generated chunk out-earning the source one. + const mixed = { + ...budgetPlanObj, + files: [ + { path: 'src/real.ts', kind: 'source' }, + { path: 'package-lock.json', kind: 'generated' }, + ], + chunks: [ + { + id: 21, + startLine: 1, + endLine: 640, + lines: 640, + chars: 20_000, + maxLineChars: 120, + files: [ + { path: 'src/real.ts', newStart: 1, newEnd: 80 }, + { path: 'package-lock.json', newStart: 1, newEnd: 560 }, + ], + }, + ], + } as never; + expect(buildChunkAgentPrompt(mixed, 21)).toContain( + 'About **39 tool calls**', + ); + }); + + it('an Agent 8 specialist is budgeted like any other whole-diff finder', () => { + // Specialists launch through buildWholeDiffBlock (its one consumer); + // without this they were the one launch class that could still wander + // unbudgeted. Its domain brief is appended inline, so its reading list + // is the diff pages alone — all six of them, per chunk size. + expect(buildWholeDiffBlock(budgetPlan)).toContain( + 'About **48 tool calls**', + ); + }); + + it('budgets every role in BRIEFS except the ones declaring budgetExempt', () => { + // Walked from the runtime roster, not a hand-copied list: a role added + // later must DECLARE its exemption at its brief, where the reason lives, + // or it gets the ceiling — it cannot silently join the exempt set, and + // the exempt set itself is pinned below. + const roles = Object.keys(BRIEFS) as Array; + const budgeted = Object.fromEntries( + roles.map((role) => { + const opts = String(role).startsWith('invariant-') + ? { file: 'big.ts' } + : {}; + return [ + role, + buildRoleBrief(budgetPlan, role, opts).includes('Tool budget'), + ]; + }), + ); + expect(budgeted).toEqual( + Object.fromEntries( + roles.map((role) => [role, !BRIEFS[role].budgetExempt]), + ), + ); + const exempt = roles.filter((r) => BRIEFS[r].budgetExempt).sort(); + expect(exempt).toEqual(['0', '7', 'verify']); + }); + + it.each([ + ['zero', 0], + ['negative', -5], + ['NaN', Number.NaN], + ['Infinity', Number.POSITIVE_INFINITY], + ['a string', '42'], + ])('a plan whose ceiling is %s gets no ceiling at all', (_name, value) => { + // The plan is parsed off disk with an unchecked cast; a garbled field + // must fall back exactly like an absent one — toward more coverage — + // not render `About **NaN tool calls**` into a brief. + const garbled = { + ...PLAN, + budget: { agentToolBudget: value }, + } as never; + expect(buildChunkAgentPrompt(garbled, 13)).not.toContain('Tool budget'); + expect(buildRoleBrief(garbled, '1a')).not.toContain('Tool budget'); + }); + + it.each([ + // A version-skewed or hand-edited plan: a positive-but-absurd value is + // clamped into the budget's own band, in both directions — 0.5 must not + // become a three-call brief, 100000 must not remove the ceiling. + ['a fraction', 0.5, 37], + ['oversized', 100_000, 67], + ])( + 'a plan whose ceiling is %s is clamped, not obeyed', + (_name, value, expected) => { + const skewed = { + ...budgetPlanObj, + budget: { agentToolBudget: value }, + } as never; + expect(buildRoleBrief(skewed, '1a')).toContain( + `About **${expected} tool calls**`, + ); + }, + ); + + it('a chunk entry missing lines and chars still renders finite numbers', () => { + // `chunkFrom` validates only startLine/endLine; the twin guard at the + // role-brief call site existed and this one did not — a malformed chunk + // must degrade to the scoped floor, never to `About **NaN tool calls**` + // and never to inheriting the whole-diff headroom. + const garbledChunk = { + ...budgetPlanObj, + chunks: [ + { + id: 16, + startLine: 1, + endLine: 2, + files: [{ path: 'a.ts', newStart: 1, newEnd: 2 }], + }, + ], + } as never; + const p = buildChunkAgentPrompt(garbledChunk, 16); + expect(p).not.toContain('NaN'); + // Floor allowance 30 + brief + one page = 32 — not the whole-diff 42. + expect(p).toContain('About **32 tool calls**'); + }); + + it('a plan without the field falls back to no ceiling — more coverage, never less', () => { + expect(buildChunkAgentPrompt(PLAN as never, 13)).not.toContain( + 'Tool budget', + ); + expect(buildRoleBrief(PLAN as never, '1a')).not.toContain('Tool budget'); + }); + + it('restates the recall rule and fixes the disclosure format', () => { + // Self-contained on purpose — a chunk brief has no RECALL section, so + // the sentence must carry the rule instead of citing it; and without + // the fixed format, check-coverage has nothing to parse. + const brief = buildRoleBrief(budgetPlan, '1a'); + expect(brief).toContain('never suppresses a finding'); + expect(brief).toContain('Budget gap: '); + expect(brief).not.toContain('as the recall rule requires'); + }); +}); diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index c1d19105b8c..dd5801607e1 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -42,6 +42,7 @@ 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 { expectedRoundSeconds, readRoundStamps, @@ -127,14 +128,20 @@ interface PlanReport { worktreePath?: unknown; mergeBaseSha?: unknown; repositoryContext?: unknown; + budget?: { agentToolBudget?: unknown }; } /** A heavy file's entry, which is the only kind an invariant agent can be built from. */ interface HeavyFile { path: string; heavy?: boolean; + kind?: string; addedRanges?: Array<{ start: number; end: number }>; diffRange?: { startLine: number; endLine: number }; + addedLines?: number; + removedLines?: number; + /** Post-change file length — `FileMetric.fileLines`, in every plan. */ + fileLines?: number; } /** @@ -276,6 +283,157 @@ function chunkFrom( return { diffPath, chunk, total: chunks.length }; } +/** + * The cumulative findings list a reverse auditor is ordered to read in + * full, in pages: measured at 65-82 KB on real runs. An estimate on + * purpose — the list grows round over round and the brief is built before + * the round runs; the ceiling is soft, so the error costs a disclosure. + */ +const FINDINGS_LIST_READS = 3; + +/** + * Lines a single `read_file` page holds, for estimating an invariant + * agent's paging through its post-change file: the read cap's worth of + * characters at a measured ~50 characters per source line. + */ +const LINES_PER_FILE_READ = 500; + +/** + * The reads a whole-diff assignment actually takes: each chunk costs its + * PAGES, not a flat one — an oversized chunk's `read_file` comes back + * `isTruncated` and the extra pages were being paid out of the analysis + * allowance. + */ +function wholeDiffReadPages(report: PlanReport): number { + return (Array.isArray(report.chunks) ? report.chunks : []).reduce( + (n: number, c) => { + const chars = (c as { chars?: number })?.chars; + return ( + n + + Math.max( + 1, + Math.ceil( + (typeof chars === 'number' && Number.isFinite(chars) ? chars : 0) / + READ_FILE_CHAR_CAP, + ), + ) + ); + }, + 0, + ); +} + +/** + * A chunk's territory in the same source-weighted units the plan-level + * budget is derived from. `reviewBudget` reads `effective = max(src, + * total/8)` because prose and generated lines carry less a reviewer can + * get wrong — a scoped allowance scaling off RAW chunk lines inverted + * that: a 600-line lockfile chunk out-earned a 200-line source chunk. The + * chunk's own file spans are weighted by `report.files[].kind` (a path the + * plan does not classify counts as source — erring toward more headroom), + * and the weight scales `chunk.lines` so the unit stays the chunk's own. + */ +function weightedTerritoryLines( + report: PlanReport, + chunk: { lines?: number; files?: unknown }, +): number { + const lines = + typeof chunk.lines === 'number' && Number.isFinite(chunk.lines) + ? Math.max(0, Math.floor(chunk.lines)) + : 0; + if (lines === 0) return 0; + const kinds = new Map( + (Array.isArray(report.files) ? (report.files as HeavyFile[]) : []) + .filter((f) => !!f && typeof f.path === 'string') + .map((f) => [f.path, f.kind]), + ); + let src = 0; + let other = 0; + for (const f of Array.isArray(chunk.files) ? chunk.files : []) { + const e = f as { path?: string; newStart?: number; newEnd?: number }; + const span = + typeof e?.newStart === 'number' && typeof e?.newEnd === 'number' + ? e.newEnd - e.newStart + 1 + : 0; + if (!(span > 0)) continue; + const kind = typeof e.path === 'string' ? kinds.get(e.path) : undefined; + if (kind === undefined || kind === 'source') src += span; + else other += span; + } + const total = src + other; + if (total === 0) return lines; + return Math.max(1, Math.round((lines * (src + other / 8)) / total)); +} + +/** + * The soft tool-call ceiling for finder/auditor briefs (see lib/budget.ts, + * `agentToolBudget` and `launchToolBudget`). Empty when the plan predates + * the budget field — an old plan fails toward more coverage, exactly like + * the pre-budget fallback the skill documents — and empty for the roles + * whose brief declares `budgetExempt` (the reason lives at each role's + * entry in agent-briefs). + * + * The ceiling is per LAUNCH, not per plan: a scoped agent's allowance is + * derived from its own territory (its chunk, its heavy file) but never + * exceeds the plan's recorded allowance, and every launch's mandatory + * reads ride on top of the allowance — a whole-diff role on a huge diff + * is assigned more chunk reads than a flat cap holds. The reads estimate + * counts the launch's whole reading list — the brief file itself, the + * diff pages, and any files the role's method mandates — and it is an + * estimate: the ceiling is soft, so roughness costs a disclosure, never a + * truncation. + * + * The wording is deliberate on three points. "Stop exploring" is aimed at + * the measured pathology — the slowest agent of a wave is reliably one + * that kept walking the tree past any recall gain (two runs of the same + * 14-agent wave: 11.7 vs 41 minutes). The recall restatement is inline + * and self-contained (a chunk brief has no RECALL section to cite) + * because a budget that reads as a reporting cap would suppress exactly + * the low-confidence candidates the pipeline's later stages exist to + * judge. And the disclosure format is FIXED (`Budget gap: `, + * one per line) because check-coverage parses those lines out of the + * transcript and reports them — a gap the orchestrator must then rule on, + * exactly as it rules on whiffs. + */ +function toolBudgetBlock( + report: PlanReport, + launch: { territoryLines?: number | null; mandatoryReads: number }, +): string[] { + const base = report.budget?.agentToolBudget; + if (typeof base !== 'number' || !Number.isFinite(base) || base <= 0) { + return []; + } + // The plan is parsed off disk with an unchecked cast, so a garbled chunk + // entry can hand this NaN — which must degrade to the floor, not render + // `About **NaN tool calls**` into a brief. + const reads = Number.isFinite(launch.mandatoryReads) + ? Math.max(0, Math.floor(launch.mandatoryReads)) + : 0; + const territory = + typeof launch.territoryLines === 'number' && + Number.isFinite(launch.territoryLines) + ? launch.territoryLines + : launch.territoryLines === null || launch.territoryLines === undefined + ? null + : 0; + const total = launchToolBudget(base, territory, reads); + return [ + '', + '## Tool budget', + '', + `About **${total} tool calls** for this whole review — reads, greps, shell, ` + + `everything — and the ~${reads} reads your launch is assigned (your ` + + 'brief, the diff pages, any files your method mandates) are already ' + + 'counted in. It is a soft ceiling. At the ceiling: stop exploring, write ' + + 'your findings from the evidence already in hand, and disclose each ' + + 'unfinished check on its own line, exactly as `Budget gap: ` — ' + + 'the coverage tool reads those lines, so the format is load-bearing. The ' + + 'budget never suppresses a finding: a candidate you can already name goes ' + + 'in your return regardless (at `Confidence: low` if the budget stopped ' + + 'you before verifying it).', + ]; +} + /** * The launch prompt for the agent that owns `chunk`. * @@ -385,6 +543,31 @@ export function buildChunkAgentPrompt( parts.push('', ...repositoryContextBlock(repositoryContext)); } + // NOT for an unreachable chunk: its instruction is to return the exact + // `Uncoverable:` line and stop, and a budget block telling it to "write your + // findings from the evidence in hand" beside that is the same two-masters + // contradiction the receipt guard below documents — an agent that follows + // the budget's disclosure format instead of the exact receipt line turns a + // disclosed uncoverable gap into a hard coverage failure. + if (!unreachable) { + parts.push( + ...toolBudgetBlock(report, { + territoryLines: weightedTerritoryLines(report, chunk), + // The launch's whole reading list: the brief file, plus the diff pages + // this chunk takes. + mandatoryReads: + 1 + + Math.max( + 1, + Math.ceil( + (Number.isFinite(chunk.chars) ? chunk.chars : 0) / + READ_FILE_CHAR_CAP, + ), + ), + }), + ); + } + // Deliberately NOT included: a sentence for the agent to recite when it finds // nothing. Every real launch handed the agent its own receipt text — `If you // find no issues, say "No issues found — reviewed chunk 13 (...)"` — and an @@ -507,6 +690,19 @@ export function buildWholeDiffBlock( if (repositoryContext) { parts.push('', ...repositoryContextBlock(repositoryContext)); } + // An Agent 8 specialist is a whole-diff finder like any other: without + // this block it was the one launch class that could still spend 40-100 + // calls wandering — recreating exactly the slowest-agent tail the budget + // exists to cut. This block is built for Agent 8 ALONE — every rostered + // role's launch comes out of `--roster`/`--role` with its reading block + // and (unless its brief declares `budgetExempt`) its own budget already + // inside, so prepending this to a role brief would double-budget it and + // hand the exempt roles the ceiling their exemption exists to withhold. + // Its domain brief is appended inline by the orchestrator, not read from + // disk, so the reading list is the diff pages alone. + parts.push( + ...toolBudgetBlock(report, { mandatoryReads: wholeDiffReadPages(report) }), + ); parts.push(...tail(rules)); return parts.join('\n'); } @@ -830,6 +1026,87 @@ export function buildRoleBrief( } parts.push('## Your dimension', '', brief.brief); + // The exemptions are declared on the briefs (`budgetExempt`), each with + // its reason at the role's entry — a hardcoded name list here is how a + // later role whose work does not scale with the diff would silently + // receive a diff-derived ceiling. + if (!brief.budgetExempt) { + const chunks = ( + Array.isArray(report.chunks) ? report.chunks : [] + ) as Array<{ id?: number; lines?: number; chars?: number }>; + if (typeof opts.chunk === 'number') { + // A chunk-scoped launch (a 3B reverse-audit chunk agent): its own + // territory, not the whole diff's. + const c = chunks.find((x) => x?.id === opts.chunk); + parts.push( + ...toolBudgetBlock(report, { + territoryLines: weightedTerritoryLines(report, c ?? {}), + // Its reading list: the brief file, the chunk's diff pages, and + // the cumulative findings list its brief orders read in full — + // measured at 65-82 KB on real runs, several pages of it. + mandatoryReads: + 1 + + Math.max( + 1, + Math.ceil( + (typeof c?.chars === 'number' && Number.isFinite(c.chars) + ? c.chars + : 0) / READ_FILE_CHAR_CAP, + ), + ) + + FINDINGS_LIST_READS, + }), + ); + } else if (role.startsWith('invariant-') && opts.file) { + // One heavy file: budget on its changed lines, with a rough page + // allowance for the post-change read plus its own diff slice — the + // ceiling is soft, so the roughness costs a disclosure, never a + // truncation. + const files = ( + Array.isArray(report.files) ? report.files : [] + ) as HeavyFile[]; + const f = files.find((x) => x?.path === opts.file); + const added = typeof f?.addedLines === 'number' ? f.addedLines : 0; + const fileLines = + typeof f?.fileLines === 'number' && Number.isFinite(f.fileLines) + ? f.fileLines + : 0; + const changed = + added + (typeof f?.removedLines === 'number' ? f.removedLines : 0); + parts.push( + ...toolBudgetBlock(report, { + territoryLines: changed, + // Its reading list: the brief, its own diff slice, and the whole + // post-change file paged to the end. The pages come from + // `fileLines` — the post-change length the plan records for + // every file — with `addedLines` as the fallback lower bound + // for an older plan without it: a file can go heavy by VOLUME + // in a large file it barely rewrote, and estimating its paging + // from added lines alone told exactly that agent its mandatory + // reading was already overspending. Floors at the old flat 4 so + // no launch gets less than before. + mandatoryReads: Math.max( + 4, + 2 + Math.ceil(Math.max(added, fileLines) / LINES_PER_FILE_READ), + ), + }), + ); + } else { + // A whole-diff role is assigned every chunk's PAGES, plus its brief — + // plus, for a findings-bearing role (the chunkless Step 3A reverse + // auditor), the cumulative findings list its brief orders read in + // full, exactly as the chunk-scoped branch counts it. Keyed on the + // brief's own `acceptsFindings` declaration, not the role name. + parts.push( + ...toolBudgetBlock(report, { + mandatoryReads: + 1 + + wholeDiffReadPages(report) + + (brief.acceptsFindings ? FINDINGS_LIST_READS : 0), + }), + ); + } + } const repositoryContext = repositoryContextOf(report); if (role === '7') { if (repositoryContext) { diff --git a/packages/cli/src/commands/review/check-coverage.test.ts b/packages/cli/src/commands/review/check-coverage.test.ts index 3a6223bb13b..a791e878c15 100644 --- a/packages/cli/src/commands/review/check-coverage.test.ts +++ b/packages/cli/src/commands/review/check-coverage.test.ts @@ -161,6 +161,13 @@ function transcript( * clears a path-shaped floor without reading the file. */ mentions?: string[]; + /** + * `[offset, limit]` for the diff reads, making them RANGED — the shape + * a compliant agent's reads take, and the only shape `diffReads` + * records. The budget-gap tests need it: a disclosing agent's chunk + * credit narrows to its ranged reads. + */ + range?: [number, number]; } = {}, ): void { const base = { agentId: id, agentName: 'general-purpose', sessionId: 'S1' }; @@ -187,7 +194,18 @@ function transcript( message: { role: 'model', parts: [ - { functionCall: { name: 'read_file', args: { file_path: DIFF } } }, + { + functionCall: { + name: 'read_file', + args: opts.range + ? { + file_path: DIFF, + offset: opts.range[0], + limit: opts.range[1], + } + : { file_path: DIFF }, + }, + }, ], }, }), @@ -724,6 +742,130 @@ describe('Step 3A — dimension agents, no territory, no receipts', () => { }); }); +describe('budget-gap disclosures — guarded, parsed, never punished', () => { + it("collects a working agent's gaps under its coverage label", () => { + transcript('a1', good(1), { + calls: 3, + range: [0, 100], + text: + 'No issues found — reviewed chunk 1 end to end.\n' + + 'Budget gap: callers of parseArgs outside packages/cli\n' + + '- Budget gap: the removed retry path in fetch-pr', + }); + transcript('a2', good(2), { calls: 2, range: [100, 100] }); + + const r = coverageFromTranscripts(plan(), ENV); + expect(r.budgetGaps).toEqual([ + { + agent: 'chunk 1', + gaps: [ + 'callers of parseArgs outside packages/cli', + 'the removed retry path in fetch-pr', + ], + }, + ]); + // The load-bearing half: this agent READ its territory (the ranged + // read), so its disclosure costs nothing — coverage stands and the gate + // passes. Failing on disclosure teaches agents not to disclose; the + // ruling on each gap belongs to the orchestrator, exactly as with + // whiffs. + expect(r.coveredChunks).toContain(1); + expect(r.ok).toBe(true); + }); + + it('a disclosure costs no coverage credit — the gate must not punish it', () => { + // An earlier draft narrowed a disclosing agent's credit to its ranged + // reads. `rangeOf` records only reads carrying a positive `limit`, so + // a compliant offset-paged or whole-file read left an honest discloser + // with zero credit and a hard gate failure — while an agent that + // stopped WITHOUT disclosing kept its full credit. The `told` + // presumption is the same for every agent; a disclosed gap changes the + // RULING (Step 3D), never the arithmetic. + transcript('sec', wholeDiff(), { + calls: 1, + text: 'Walked what I could.\nBudget gap: chunk 2 exploration depth', + }); + + const r = coverageFromTranscripts(plan3a(), ENV); + expect(r.coveredChunks).toEqual([1, 2]); + expect(r.ok).toBe(true); + expect(r.budgetGaps).toHaveLength(1); + }); + + it("a gap-free compliant relaunch silences the failed attempt's gaps", () => { + // The repair pattern: attempt 1 hits the ceiling and discloses, + // attempt 2 (same verbatim prompt) finishes clean. Reporting attempt + // 1's stale gaps beside the repair would keep the report from ever + // converging — the same rule every failure flag in this file follows. + transcript('try1', good(1), { + calls: 2, + text: 'Partial.\nBudget gap: the rest of chunk 1', + }); + transcript('try2', good(1), { calls: 3 }); + transcript('a2', good(2), { calls: 2 }); + const p = plan(); + writeFileSync(join(promptRecordDir(p), 'chunk-1.txt'), good(1)); + + expect(coverageFromTranscripts(p, ENV).budgetGaps).toEqual([]); + }); + + it('two disclosing relaunches must not supersede each other into silence', () => { + // Both attempts hit the ceiling and both disclosed. Mutual + // supersession would drop every gap — nobody rules, nothing renders, + // and a required-trace gap never caps the verdict. Suppression + // requires a GAP-FREE superseding record: a genuine repair. + transcript('try1', good(1), { + calls: 2, + text: 'Partial.\nBudget gap: the callers of the renamed export', + }); + transcript('try2', good(1), { + calls: 2, + text: 'Partial again.\nBudget gap: the callers of the renamed export', + }); + transcript('a2', good(2), { calls: 2 }); + const p = plan(); + writeFileSync(join(promptRecordDir(p), 'chunk-1.txt'), good(1)); + + const gaps = coverageFromTranscripts(p, ENV).budgetGaps; + expect(gaps.length).toBeGreaterThan(0); + expect(gaps[0].gaps).toEqual(['the callers of the renamed export']); + }); + + it('does not credit an idle agent that copied the template back', () => { + // The brief hands every agent the literal `Budget gap: ` + // format — the costume is issued with the uniform. A zero-tool-call + // agent's disclosure is the whiff wearing it. + transcript('idle1', good(1), { + calls: 0, + text: 'No issues found — thorough review.\nBudget gap: deeper caller tracing', + }); + transcript('a2', good(2), { calls: 2, range: [100, 100] }); + + const r = coverageFromTranscripts(plan(), ENV); + expect(r.idleAgents).toEqual(['chunk 1']); + expect(r.budgetGaps).toEqual([]); + }); + + it('does not credit a blind agent with a disclosed gap either', () => { + transcript('blind1', blind(1), { + calls: 2, + text: 'Reviewed.\nBudget gap: the other half of the chunk', + }); + transcript('a2', good(2), { calls: 2, range: [100, 100] }); + + const r = coverageFromTranscripts(plan(), ENV); + expect(r.blindAgents).toEqual(['chunk 1']); + expect(r.budgetGaps).toEqual([]); + }); + + it('reports none when nobody disclosed one', () => { + transcript('a1', good(1), { calls: 3 }); + transcript('a2', good(2), { calls: 2 }); + + expect(coverageFromTranscripts(plan(), ENV).budgetGaps).toEqual([]); + }); +}); + describe('worked, but not on the diff', () => { it('catches the agent that was pointed at the diff and never opened it', () => { // The old bar was one successful tool call, and a `glob` for test files is a @@ -990,6 +1132,53 @@ describe('the roster — who should have been here', () => { } }); + it('prints the budget-gap NOTE with its directives before the agent text', () => { + // stderr is the interface the orchestrator acts on, and this NOTE is + // the only channel telling it not to relaunch and how to rule each + // gap. The directive-before-disclosure ordering is deliberate — + // instructions that follow quoted material can be impersonated by it — + // and a disclosure must never move the exit code. + transcript('a1', good(1), { + calls: 3, + text: 'No issues found — walked it.\nBudget gap: the removed retry path', + }); + transcript('a2', good(2), { calls: 2 }); + const p = plan(); + + const prevDir = process.env['QWEN_CODE_PROJECT_DIR']; + const prevSession = process.env['QWEN_CODE_SESSION_ID']; + process.env['QWEN_CODE_PROJECT_DIR'] = ENV['QWEN_CODE_PROJECT_DIR']; + process.env['QWEN_CODE_SESSION_ID'] = ENV['QWEN_CODE_SESSION_ID']; + const prevExit = process.exitCode; + try { + vi.mocked(writeStderrLine).mockClear(); + (checkCoverageCommand.handler as (a: Record) => void)({ + plan: p, + out: join(dir, 'cov.json'), + }); + + const note = vi + .mocked(writeStderrLine) + .mock.calls.map((c) => String(c[0])) + .find((l) => l.includes('budget-gap disclosure(s)')); + expect(note).toBeDefined(); + expect(note).toContain( + 'NOTE: 1 budget-gap disclosure(s) from 1 agent(s)', + ); + expect(note).toContain('chunk 1: the removed retry path'); + expect(note!.indexOf('Do not relaunch over these')).toBeLessThan( + note!.indexOf('chunk 1: the removed retry path'), + ); + expect(process.exitCode).toBe(prevExit); + } finally { + process.exitCode = prevExit; + if (prevDir === undefined) delete process.env['QWEN_CODE_PROJECT_DIR']; + else process.env['QWEN_CODE_PROJECT_DIR'] = prevDir; + if (prevSession === undefined) delete process.env['QWEN_CODE_SESSION_ID']; + else process.env['QWEN_CODE_SESSION_ID'] = prevSession; + } + }); + it('formats the partial case on stderr: one role missing, the rest briefed', () => { // The all-briefless collapse has a handler test; the partial shape reached // stderr only through the pure function. A formatting regression here — a diff --git a/packages/cli/src/commands/review/check-coverage.ts b/packages/cli/src/commands/review/check-coverage.ts index 5fdc9bf7f82..06bfbb4b353 100644 --- a/packages/cli/src/commands/review/check-coverage.ts +++ b/packages/cli/src/commands/review/check-coverage.ts @@ -210,9 +210,14 @@ function runCheckCoverage(args: CheckCoverageArgs): void { writeStderrLine( 'NOTE: a chunk counts as read when an agent was pointed at its lines AND ' + 'the harness recorded that agent opening the diff. An agent handed the ' + - 'diff with no line ranges covers nothing. Build every whole-diff ' + - 'agent\'s prompt with `"${QWEN_CODE_CLI:-qwen}" review agent-prompt ' + - `--plan ${shellQuotePath(args.plan)} --whole-diff\` and paste it verbatim ahead of its brief.`, + "diff with no line ranges covers nothing. Every rostered agent's " + + 'launch block — its diff reads included — comes from ' + + `\`"\${QWEN_CODE_CLI:-qwen}" review agent-prompt --plan ${shellQuotePath(args.plan)} --roster\` ` + + '(or `--role ` for one, `--chunk ` for a chunk agent — the ' + + 'usual reader of a missing chunk, which `--role` cannot rebuild); ' + + 'pass each verbatim. `--whole-diff` builds ' + + 'the reading block for an Agent 8 specialist alone — prepending it to ' + + 'a rostered brief double-budgets the agent.', ); } if (report.idleAgents.length > 0) { @@ -238,6 +243,27 @@ function runCheckCoverage(args: CheckCoverageArgs): void { `aggregate findings over a diff that was not read.`, ); } + // A NOTE, never an error, and never a relaunch: a disclosed gap is the soft + // tool budget working as designed, and failing the gate on it would teach + // agents not to disclose. The ruling belongs to the orchestrator — a gap + // naming an incomplete REQUIRED trace joins unreviewedDimensions; optional + // depth goes to the report's "Not reviewed" section. + if (report.budgetGaps.length > 0) { + const total = report.budgetGaps.reduce((n, g) => n + g.gaps.length, 0); + // The directives come FIRST: everything after the dash is agent-authored + // text (parser-sanitized and length-capped, but still the agents'), and + // instructions that follow quoted material can be impersonated by it. + writeStderrLine( + `NOTE: ${total} budget-gap disclosure(s) from ` + + `${report.budgetGaps.length} agent(s). Do not relaunch over these; ` + + `rule on each: a gap naming an incomplete required trace goes in ` + + `unreviewedDimensions, optional depth is disclosed in the report's ` + + `"Not reviewed" section. The disclosures — ` + + report.budgetGaps + .map((g) => `${g.agent}: ${g.gaps.join('; ')}`) + .join(' | '), + ); + } if (!report.ok) { process.exitCode = 3; diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 1402ddc6dd4..4461c3522ee 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -189,7 +189,13 @@ function recordStep45( function transcript( id: string, launchPrompt: string, - opts: { toolCalls?: number; text?: string; opens?: string[] } = {}, + opts: { + toolCalls?: number; + text?: string; + opens?: string[]; + /** `[offset, limit]` making the diff reads ranged, as a compliant agent's are. */ + range?: [number, number]; + } = {}, ): void { const pointedAtBriefs = [ ...launchPrompt.matchAll(/read_file\(file_path="([^"]*\.brief\.md)"\)/g), @@ -212,7 +218,18 @@ function transcript( message: { role: 'model', parts: [ - { functionCall: { name: 'read_file', args: { file_path: DIFF } } }, + { + functionCall: { + name: 'read_file', + args: opts.range + ? { + file_path: DIFF, + offset: opts.range[0], + limit: opts.range[1], + } + : { file_path: DIFF }, + }, + }, ], }, }), @@ -1155,6 +1172,114 @@ describe('composeReview — not-reviewed entries that carry their own reason', ( }); }); +describe('composeReview — budget-gap disclosures (a channel, never a cap)', () => { + it('renders disclosed gaps in the body and still approves a clean run', () => { + // The agent read its whole territory (ranged read) and disclosed one + // optional-depth check its tool budget cut short. The disclosure must + // reach the author mechanically — whether or not the orchestrator + // relays anything — and must NOT cap the verdict: judging which gaps + // name a required trace is the orchestrator's ruling (Step 3D), and + // capping on every routine budget stop would make the soft ceiling + // hard. + transcript('a1', goodPrompt(1), { + toolCalls: 3, + range: [0, 100], + text: + 'No issues found — walked chunk 1 fully.\n' + + 'Budget gap: second-order callers of the renamed export', + }); + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + recordStep45(p, ['verify', 'reverse-audit']); + + // Not base(): its planPath DEFAULT (coveredPlan()) is evaluated on every + // call and rewrites this run's a1/a2 transcripts with clean ones. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + }); + // Attributed to its agent and wrapped as inline code — a gap carrying + // an @-mention, a #123 reference or a stray `` must reach + // the body inert. + expect(r.body).toContain( + 'Not explored to full depth (tool budget reached): ' + + 'chunk 1: `second-order callers of the renamed export`.', + ); + expect(r.event).toBe('APPROVE'); + }); + + it('drops its mechanical line for a gap the caller promoted — one register, not two', () => { + // Step 3D has the orchestrator promote a required-trace gap into + // unreviewedDimensions with the gap's own text as the scope. The + // promoted entry caps and renders verbatim; the mechanical line must + // yield, or the body says one budget stop twice in two contradicting + // framings (#7188's double-disclosure regression, reopened). + transcript('a1', goodPrompt(1), { + toolCalls: 3, + range: [0, 100], + text: + 'No issues found — walked chunk 1 fully.\n' + + 'Budget gap: second-order callers of the renamed export', + }); + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + recordStep45(p, ['verify', 'reverse-audit']); + + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + unreviewedDimensions: [ + 'second-order callers of the renamed export — stopped at the agent tool budget', + ], + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + 'Not reviewed: second-order callers of the renamed export — stopped at the agent tool budget.', + ); + expect(r.body).not.toContain('Not explored to full depth'); + expect(r.event).toBe('COMMENT'); + }); + + it('a disclosed gap denies the "no blockers" certification', () => { + // "Reviewed — no blockers." two lines above "Not explored to full + // depth" is the opener certifying what the disclosure takes back. + transcript('a1', goodPrompt(1), { + toolCalls: 3, + range: [0, 100], + text: + 'One suggestion filed.\n' + + 'Budget gap: the callers of the renamed export', + }); + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + recordStep45(p, ['verify', 'reverse-audit']); + + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 1, + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain('Not explored to full depth'); + expect(r.body).not.toContain('no blockers'); + }); +}); + describe('composeReview — input validation (the producer is a model that omits inapplicable fields)', () => { it('a body-Critical-only input with every count omitted lands on the REQUEST_CHANGES row (undefined + 1 = NaN once meant APPROVE)', () => { // The NaN property pins on `baseEvent`: the arithmetic put the blocker on diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 3cfe58f99a6..3b3b25b5f55 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -434,6 +434,19 @@ function composeReviewBody( // command that repairs it, to the orchestrator. #7012's public body was fourteen // lines of the second register posted to the first reader. const remediation: string[] = []; + // Budget-gap disclosures from the coverage report — the checks agents said + // their soft tool budget cut short. A DISCLOSURE channel, deliberately not + // a cap: these render in the body's "Not reviewed" section mechanically + // (so a disclosed gap reaches the author whether or not the orchestrator + // relays it), while judging which gaps name an incomplete REQUIRED trace — + // and so belong in `unreviewedDimensions`, which caps — stays the + // orchestrator's ruling, exactly as the skill's Step 3D writes it. Capping + // on every gap here would make the soft ceiling hard: any large diff's + // routine budget stop would forbid an Approve the review otherwise earned. + const budgetGapNotes: Array<{ agent: string; gaps: string[] }> = []; + // Sibling caps MAX_DIMENSIONS and MAX_NOTES bound their lists for the + // same reason; this bounds the one budget-gap sentence. + const MAX_BUDGET_GAP_LINES = 5; // FIX lines are commands. `` was a placeholder a reader had to notice // and fill; pasted literally it parses as a shell redirection. The run KNOWS // its plan path — substitute it, and leave only the selectors (``, ``) @@ -614,6 +627,7 @@ function composeReviewBody( 'the read is what proves the review happened', ); } + budgetGapNotes.push(...cov.budgetGaps); // The prompt was built in code and edited on the way to the agent. This caps // for the same reason the others do: what the agent was actually asked is not // what this skill's guarantees are written against. @@ -1065,6 +1079,41 @@ function composeReviewBody( zh: `未审查:${d}。`, }); } + // Budget-gap disclosures, one BOUNDED sentence for all of them. Four + // review findings shaped this: each gap rides through `mdField` (inline + // code neutralizes @-mentions, #123 cross-references, links and any + // stray `` an agent quoted — the first path by which raw + // sub-agent prose could reach a public PR body); the line is capped like + // its siblings (unbounded entries joined into one disclosure drown the + // verdict they ride on — and ~50 uncapped agents would break GitHub's + // 64 KB body limit and lose the whole POST); each gap carries its + // agent's label so N agents stopping on the same trace stay tellable + // apart; and a gap the caller already promoted into + // `unreviewedDimensions` is dropped here — the capping relay owns it, + // and the body must not say it twice in two registers. These are + // "stopped at the budget", not "nobody looked": the phrasing must not + // claim the stronger gap, and the entries do not join the capping lists. + const budgetGapItems: Array<{ agent: string; gap: string }> = []; + for (const g of budgetGapNotes) { + for (const gap of g.gaps) budgetGapItems.push({ agent: g.agent, gap }); + } + const keptBudgetGaps = budgetGapItems.filter( + (it) => !unreviewed.some((d) => d.includes(it.gap)), + ); + if (keptBudgetGaps.length > 0) { + const shown = keptBudgetGaps.slice(0, MAX_BUDGET_GAP_LINES); + const more = keptBudgetGaps.length - shown.length; + const enList = + shown.map((it) => `${it.agent}: ${mdField(it.gap)}`).join('; ') + + (more > 0 ? `, and ${more} more` : ''); + const zhList = + shown.map((it) => `${it.agent}:${mdField(it.gap)}`).join(';') + + (more > 0 ? `,另有 ${more} 条` : ''); + notReviewedParts.push({ + en: `Not explored to full depth (tool budget reached): ${enList}.`, + zh: `未探索到全部深度(达到工具调用预算):${zhList}。`, + }); + } // Same cause, one sentence: forty-three chunks launched with rewritten // prompts are one failure with forty-three subjects, not forty-three // paragraphs — a posted body on #7166 was ninety-nine clauses over four @@ -1247,16 +1296,24 @@ function composeReviewBody( } if (event === 'APPROVE') { + // `notReviewedParts` here is exactly the budget-gap disclosures: every + // other source of a not-reviewed entry also caps, and a capped run never + // reaches this branch. They render on the Approve because they are a + // disclosure, not a defect — hiding "stopped at the tool budget" behind + // an unqualified LGTM would break the one promise the disclosure channel + // makes, that it reaches the author mechanically. return { event, body: render( [ { en: 'No issues found. LGTM! ✅', zh: '未发现问题。LGTM!✅' }, + ...notReviewedParts, ...deferredBlock, ...testPlanBlock, ...repositoryContextBlock, ], - deferredBlock.length || + notReviewedParts.length || + deferredBlock.length || testPlanBlock.length || repositoryContextBlock.length ? '\n\n' @@ -1309,6 +1366,12 @@ function composeReviewBody( // them." Nothing nobody read can be certified blocker-free — and neither // can a loop that ended with findings no verifier ever ruled on. missingReceipts.length === 0 && + // A disclosed budget gap is not a blocker, but "Reviewed — no + // blockers." two lines above "Not explored to full depth" is the + // opener certifying what the disclosure takes back — the exact + // shape the comment below forbids. (A gap the caller promoted into + // `unreviewedDimensions` already denies certification above.) + keptBudgetGaps.length === 0 && !findingsUnverifiedAtCompose; // The opener may not say "Reviewed." over a disclosure set that denies it. // #7268's posted body opened exactly that way — "Reviewed. Suggestions are diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index dd190a0a2d5..ab878e618be 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -163,6 +163,19 @@ export interface Brief { * instructs exactly as it counts the brief's. */ acceptsFindings?: boolean; + /** + * This role's brief never carries the soft tool-call ceiling + * (`agentToolBudget`). + * + * Declarative for the same reason `acceptsChunk` is: the exemption used to + * be three role names hardcoded in the prompt builder, which is exactly how + * a later role whose mandatory work does not scale with the diff would + * silently receive a diff-derived ceiling. Each exemption carries its own + * reason at the role's entry; a new role decides here, next to everything + * else it declares, and the roster test walks `BRIEFS` so the exempt set + * cannot drift unpinned. + */ + budgetExempt?: boolean; /** The agent-facing text. */ brief: string; } @@ -178,6 +191,10 @@ export const REVERSE_AUDIT_EXAMPLE_RECEIPT = export const BRIEFS: Record = { '0': { + // Budget-exempt: Issue-sized mandatory work, not diff-sized: a small bugfix + // referencing many issues would exhaust a diff-derived ceiling on + // required fetches alone. + budgetExempt: true, label: 'Agent 0: Issue fidelity & root-cause ownership', publicLabel: 'the linked-issue fidelity pass', publicLabelZh: '关联 issue 一致性检查', @@ -481,6 +498,10 @@ You are undirected on purpose. Do not restrict yourself to the list.`, }, '7': { + // Budget-exempt: Deterministic build/test commands — the run costs what the + // project scripts cost, and stopping early is the one thing it must + // never do. + budgetExempt: true, label: 'Agent 7: Build & test verification', publicLabel: 'the build-and-test check', publicLabelZh: '构建与测试验证', @@ -571,6 +592,9 @@ Report a **Critical** for each violation, and give **both** locations that toget }, verify: { + // Budget-exempt: Its per-finding re-trace must not stop early; `verifyShard` + // already governs its load. + budgetExempt: true, reviewsCode: true, output: 'verdicts', acceptsFindings: true, diff --git a/packages/cli/src/commands/review/lib/budget.test.ts b/packages/cli/src/commands/review/lib/budget.test.ts index 316d678d3fb..eea0f756fb0 100644 --- a/packages/cli/src/commands/review/lib/budget.test.ts +++ b/packages/cli/src/commands/review/lib/budget.test.ts @@ -9,6 +9,9 @@ import { MAX_INLINE_ANGLES, MIN_INLINE_ANGLES, VERIFY_SHARD, + budgetGapDisclosures, + stripBudgetGapLines, + launchToolBudget, reviewBudget, } from './budget.js'; @@ -130,3 +133,255 @@ describe('reviewBudget — garbled input fails toward the cheap end, never throw } }); }); + +describe('reviewBudget — the agent tool budget', () => { + it('floors at 30 on a small diff', () => { + expect( + reviewBudget({ srcDiffLines: 40, diffLines: 60 }).agentToolBudget, + ).toBe(32); + expect( + reviewBudget({ srcDiffLines: 0, diffLines: 0 }).agentToolBudget, + ).toBe(30); + }); + + it('earns a call per twenty effective lines', () => { + expect( + reviewBudget({ srcDiffLines: 300, diffLines: 400 }).agentToolBudget, + ).toBe(45); + }); + + it('caps at 60 — a wanderer must not out-earn the ceiling', () => { + expect( + reviewBudget({ srcDiffLines: 5000, diffLines: 6000 }).agentToolBudget, + ).toBe(60); + }); + + it('a large all-prose diff earns budget at the coarse effective rate', () => { + // effective = max(src, total/8): prose still has lines to walk. + expect( + reviewBudget({ srcDiffLines: 0, diffLines: 3200 }).agentToolBudget, + ).toBe(50); + }); +}); + +describe('launchToolBudget — the per-launch ceiling', () => { + it('derives a scoped allowance from the territory, same rate and clamps', () => { + expect(launchToolBudget(60, 0, 0)).toBe(30); + expect(launchToolBudget(60, 217, 0)).toBe(40); + expect(launchToolBudget(60, 5000, 0)).toBe(60); + }); + + it('never lets a territory raise a launch above the plan allowance', () => { + // The plan's recorded number is the authority every launch answers to; + // the scoped derivation may only lower it. + expect(launchToolBudget(35, 5000, 0)).toBe(35); + expect(launchToolBudget(42, 217, 0)).toBe(40); + }); + + it('a whole-diff launch (null territory) uses the plan allowance as-is', () => { + expect(launchToolBudget(42, null, 0)).toBe(42); + }); + + it('clamps the plan value in both directions', () => { + // A version-skewed or hand-edited plan carrying 0.5 or 100000 must not + // become a three-call or a hundred-thousand-call brief. + expect(launchToolBudget(0.5, null, 0)).toBe(30); + expect(launchToolBudget(100_000, null, 0)).toBe(60); + expect(launchToolBudget(100_000, 5000, 0)).toBe(60); + }); + + it('mandatory reads ride on top of the allowance, never inside it', () => { + // The finding this pins: a whole-diff role on a 25,000-line diff is + // ASSIGNED 63 chunk reads — a flat cap would be exhausted by the reading + // list before any analysis began. + expect(launchToolBudget(60, 25_000, 63)).toBe(60 + 63); + expect(launchToolBudget(60, 100, 2)).toBe(35 + 2); + }); + + it('garbled inputs fail toward the floor, never throw', () => { + expect(launchToolBudget(Number.NaN, Number.NaN, Number.NaN)).toBe(30); + expect(launchToolBudget(-5, -40, -3)).toBe(30); + expect(launchToolBudget(42, 100, Number.POSITIVE_INFINITY)).toBe(35); + }); + + it('caps the TOTAL — the reads term must not erase the clamped ceiling', () => { + // The reads come from the same unchecked-cast plan as the allowance: + // a garbled chars of 1e9 flowed through as a forty-thousand-call + // brief while the same plan's inflated allowance was dutifully + // clamped to 60. Legitimate reading lists stay untouched. + expect(launchToolBudget(60, 400, 40_004)).toBe(200); + expect(launchToolBudget(60, 25_000, 63)).toBe(123); + }); +}); + +describe('budgetGapDisclosures — the one parser of the disclosure format', () => { + it('parses plain fixed-format lines', () => { + expect( + budgetGapDisclosures( + 'No issues found — walked it all.\n' + + 'Budget gap: callers of parseArgs outside packages/cli\n' + + 'Budget gap: the removed retry path', + ), + ).toEqual([ + 'callers of parseArgs outside packages/cli', + 'the removed retry path', + ]); + }); + + it('tolerates the markdown furniture an LLM writes its own lists in', () => { + // A disclosure lost to a bullet point is unobservable: nothing + // downstream can tell "no gaps" from "gaps we failed to parse". The + // fullwidth colon is deliberate too — this skill's outputs are + // bilingual, and Chinese prose uses `:`. + for (const line of [ + '- Budget gap: the check', + '* Budget gap: the check', + '1. Budget gap: the check', + '**Budget gap:** the check', + '`Budget gap: the check`', + 'Budget gap:the check', + // A zh-narrating agent's budget stop must be as visible as an + // English one — the receipt regex next door accepts zh receipts. + '预算缺口:the check', + '预算不足: the check', + ]) { + expect(budgetGapDisclosures(line)).toEqual(['the check']); + } + }); + + it('does not read a QUOTATION of the format as a use of it', () => { + // This repo reviews its own PRs: an agent reviewing this very diff + // quotes these strings out of the brief and the skill. Blockquotes, + // fenced code and an unclosed code span are citations, not + // disclosures — the same self-reference hazard transcripts.ts guards + // for tool-call parsing. + expect( + budgetGapDisclosures('> Budget gap: the removed retry path in fetch-pr'), + ).toEqual([]); + expect( + budgetGapDisclosures( + '```\nBudget gap: inside a fence\n```\nafter the fence', + ), + ).toEqual([]); + expect( + budgetGapDisclosures( + '- `Budget gap: `, which check-coverage parses out of the transcripts', + ), + ).toEqual([]); + }); + + it('requires the gap on the SAME line as its marker', () => { + // A bare header used to capture the following line — turning an + // explicit denial into a phantom disclosure and swallowing the first + // item of a header-plus-list shape. + expect( + budgetGapDisclosures('Budget gap:\nNo further checks were cut short.'), + ).toEqual([]); + expect( + budgetGapDisclosures('**Budget gap:**\n- item one\n- item two'), + ).toEqual([]); + }); + + it('drops non-answers in any punctuation, not only bare tokens', () => { + // `Budget gap: None.` is the agent saying it has nothing to disclose; + // a phantom gap costs real rounds downstream (a chunk that never + // retires, an Approve that discloses "None." under its LGTM). + for (const line of [ + 'Budget gap: ', + 'Budget gap: none', + 'Budget gap: None.', + 'Budget gap: None (all checks completed)', + 'Budget gap: N/A - stayed under budget', + 'Budget gap: nothing skipped', + 'Budget gap: no gaps', + 'Budget gap:', + ]) { + expect(budgetGapDisclosures(line)).toEqual([]); + } + }); + + it('folds duplicate disclosures into one gap', () => { + // An agent commonly states its gap mid-return and restates it in the + // closing summary — one gap, not two, and duplicates must not consume + // the count cap either. + expect( + budgetGapDisclosures( + 'Budget gap: second-order callers\n' + + 'more prose\n' + + 'Budget gap: Second-order callers', + ), + ).toEqual(['second-order callers']); + }); + + it('sanitizes and caps what will reach a terminal and the posted body', () => { + // C1 controls, the Unicode line separators and the bidi overrides are + // as dangerous as C0 — and U+2028 must not silently truncate the gap. + const laundered = budgetGapDisclosures( + 'Budget gap: first\u2028second \u009b\u202epart', + )[0]; + expect(laundered).toBe('first second part'); + const long = budgetGapDisclosures(`Budget gap: ${'a'.repeat(500)}`)[0]; + expect([...long].length).toBeLessThanOrEqual(161); + const many = budgetGapDisclosures( + Array.from({ length: 20 }, (_, i) => `Budget gap: check ${i}`).join('\n'), + ); + expect(many).toHaveLength(8); + }); + + it('strips markdown wrappers only in pairs — never one side', () => { + // A trailing-only strip turned balanced Markdown into an orphan + // backtick that pairs with the next gap's on the joined line and + // swallows the text between them. + expect(budgetGapDisclosures('Budget gap: **trace the callers**')).toEqual([ + 'trace the callers', + ]); + expect(budgetGapDisclosures('Budget gap: callers of `parseFoo`')).toEqual([ + 'callers of `parseFoo`', + ]); + }); + + it('stays linear on pathological inputs', () => { + // The previous single multiline regex was measured at 5.8 s on 98 KB + // of newlines — quadratic backtracking from every line start. The + // line-based scan has no cross-line class to backtrack over. + const pathological = '-\n'.repeat(49_000) + ' \n> - '.repeat(20_000); + const t0 = performance.now(); + expect(budgetGapDisclosures(pathological)).toEqual([]); + expect(performance.now() - t0).toBeLessThan(1000); + }); +}); + +describe('stripBudgetGapLines — the receipt judged without its disclosures', () => { + it('removes exactly the disclosure lines and keeps everything else', () => { + expect( + stripBudgetGapLines( + 'No new issues found — re-walked the territory.\n' + + 'Budget gap: the two remaining call-site traces\n' + + 'Everything else held.', + ), + ).toBe( + 'No new issues found — re-walked the territory.\nEverything else held.', + ); + }); + + it('leaves quotations of the format in place', () => { + const text = '> Budget gap: quoted from the brief'; + expect(stripBudgetGapLines(text)).toBe(text); + }); +}); + +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 + // getter on a prototype, or added only under some inputs, would silently + // vanish from the plan every consumer reads. Assert the runtime shape, + // not just the type. + const b = reviewBudget({ srcDiffLines: 10, diffLines: 10 }); + expect(Object.keys(b)).toContain('agentToolBudget'); + expect( + (JSON.parse(JSON.stringify(b)) as Record)[ + 'agentToolBudget' + ], + ).toBe(30); + }); +}); diff --git a/packages/cli/src/commands/review/lib/budget.ts b/packages/cli/src/commands/review/lib/budget.ts index 41c0701101c..fdd16fa9688 100644 --- a/packages/cli/src/commands/review/lib/budget.ts +++ b/packages/cli/src/commands/review/lib/budget.ts @@ -81,6 +81,22 @@ export interface ReviewBudget { * its list, which is a fact about the verifier and not about the diff. */ verifyShard: number; + /** + * Soft tool-call ceiling baked into every finder/auditor brief — not the + * verifier, whose load `verifyShard` already governs, and not Build & Test, + * whose calls are deterministic commands. + * + * A fan-out wave's wall clock is its slowest agent, and the slowest agent + * is reliably a wanderer: two measured runs of the SAME 14-agent wave took + * 11.7 and 41 minutes, the difference being individual agents spending + * 40-100 model calls exploring the tree, while healthy agents on + * comparable diffs settle in the 25-45 range. The ceiling is SOFT: the + * brief tells the agent to stop exploring at the budget, write its + * findings from the evidence in hand, and disclose what it did not get to + * — a disclosed gap feeds the whiff and receipt machinery; an undisclosed + * crawl only feeds the wall clock. + */ + agentToolBudget: number; } /** @@ -99,6 +115,17 @@ export const MIN_INLINE_ANGLES = 3; export const MAX_INLINE_ANGLES = 6; export const VERIFY_SHARD = 8; +/** + * The floor is what a small diff's walk legitimately needs (brief + chunk + * reads + a handful of enclosing-function reads and greps); the ceiling sits + * above every healthy per-agent count measured on real reviews (25-45) and + * below the wandering pathology (40-100+). One extra call per twenty + * effective lines lets a larger territory earn a longer walk. + */ +export const MIN_AGENT_TOOL_BUDGET = 30; +export const MAX_AGENT_TOOL_BUDGET = 60; +const LINES_PER_TOOL_CALL = 20; + /** * The review budget for a plan. * @@ -133,9 +160,258 @@ export function reviewBudget(input: BudgetInput): ReviewBudget { sweep: effective >= SWEEP_FLOOR, specialistCap: src >= SPECIALIST_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, + ), }; } +/** + * The per-launch tool ceiling: the exploration allowance for this launch, + * PLUS the launch's mandatory reads. + * + * Review findings shaped every term here. A whole-diff role on a + * 25,000-line diff is ASSIGNED 63 chunk reads — a flat 60-call cap is + * exhausted by the reading list before any analysis begins, so mandatory + * reads ride on top of the allowance, never inside it. A scoped agent (one + * chunk, one heavy file) inheriting the whole-diff ceiling keeps exactly + * the wandering headroom the budget exists to cut, so a scoped launch's + * allowance is derived from its own territory at the same rate. And the + * plan's recorded number stays the authority for every launch — the skill + * promises "every reader sees one number", so the scoped derivation may + * only LOWER the plan's allowance, never raise it, and the plan's value is + * clamped into the same [floor, ceiling] band in both directions: a + * version-skewed or hand-edited plan carrying `0.5` or `100000` must not + * become a three-call or a hundred-thousand-call brief. + * + * `territoryLines: null` is a whole-diff launch — no territory smaller + * than the plan's, so the clamped plan allowance is used as-is. + */ +/** + * The hard ceiling on the TOTAL a brief may state. The allowance is + * clamped, but the reads term comes from the same unchecked-cast plan — + * a garbled `chars` of 1e9 flowed through as a forty-thousand-call + * brief, the exact number the clamp exists to make impossible. High + * enough that no legitimate reading list reaches it (a 63-chunk 3B + * fan-out with paged chunks and the findings list sits well under), + * low enough that a garbled plan cannot erase the ceiling. + */ +export const MAX_TOTAL_TOOL_CALLS = 200; + +export function launchToolBudget( + planBudget: number, + territoryLines: number | null, + mandatoryReads: number, +): number { + const base = clamp( + sane(planBudget) || MIN_AGENT_TOOL_BUDGET, + MIN_AGENT_TOOL_BUDGET, + MAX_AGENT_TOOL_BUDGET, + ); + const allowance = + territoryLines === null + ? base + : Math.min( + base, + clamp( + MIN_AGENT_TOOL_BUDGET + + Math.floor(sane(territoryLines) / LINES_PER_TOOL_CALL), + MIN_AGENT_TOOL_BUDGET, + MAX_AGENT_TOOL_BUDGET, + ), + ); + return Math.min( + MAX_TOTAL_TOOL_CALLS, + allowance + Math.max(0, Math.floor(sane(mandatoryReads))), + ); +} + +/** + * The disclosure an agent writes when the ceiling stopped a check, and the + * one parser every reader of that disclosure shares. The brief mandates the + * line form (`Budget gap: `); `check-coverage` reports the + * parsed gaps; the reverse-audit retirement strips these lines out of a + * receipt before judging its substance. One matcher, one home — a second + * copy is how the brief and its readers drift apart. + * + * The scan is LINE-BASED, three reviews' worth of reasons at once: + * + * - A single multiline regex whose prefix class could cross `\n` was + * measured at seconds-per-run on pathological-but-ordinary returns + * (quoted diff hunks, banner comments) — quadratic backtracking from + * every line start. Per-line matching cannot cross lines, so it cannot + * backtrack across them. + * - The gap must sit on the SAME line as its marker: a bare + * `Budget gap:` header used to swallow the following line, turning an + * explicit next-line denial into a phantom disclosure. + * - QUOTING the format must not read as USING it: lines inside fenced + * code blocks and blockquote lines are skipped — this repo reviews its + * own PRs, and a reviewer of this very diff quotes these strings. (The + * same hazard transcripts.ts guards for tool-call parsing.) Bullets and + * numbering stay tolerated: lists are how an agent writes its own + * disclosures, and a disclosure lost to a bullet is unobservable — + * nothing downstream can tell "no gaps" from "gaps we failed to parse". + */ +const BUDGET_GAP_LINE_RE = + /^[ \t]*(?:[-*+]|\d+[.)])?[ \t]*(`?)[*_~]{0,3}(?:budget gap|预算(?:缺口|不足|用尽))[*_~]{0,3}[ \t]*[::][*_~]{0,3}[ \t]*(.+?)[ \t]*$/i; + +/** A cheap pre-filter so the line walk skips returns with nothing to find. */ +const GAP_HINT_RE = /budget gap|预算(?:缺口|不足|用尽)/i; + +/** + * The disclosure marker ANYWHERE in a line — for the one consumer that + * cannot rely on the own-line format: a receipt clause with the disclosure + * appended after the separator (`No new issues found — …; Budget gap: X`) + * would otherwise absorb the gap text as its own substance. The general + * parser deliberately stays line-anchored (a mid-line mention is how the + * format is QUOTED); this is only for cutting a clause, never for minting + * gaps. + */ +export const INLINE_BUDGET_GAP_RE = + /(?:budget gap|预算(?:缺口|不足|用尽))[*_~`]{0,3}[ \t]*[::]/i; + +/** + * Templates and non-answers that must not become gaps someone rules on. + * Tested against the gap with trailing punctuation stripped, and matched on + * the LEADING token — `none.`, `None (all checks completed)` and + * `N/A - stayed under budget` are all the agent saying it has nothing to + * disclose, and a phantom gap costs real rounds downstream (a chunk that + * never retires, a body that discloses "None." on an Approve). + */ +const PLACEHOLDER_GAP_RE = + /^(?:<[^>]*>|none\b.*|n\/a\b.*|nothing\b.*|no (?:gaps?|checks?)\b.*|[-—*_~`]+)$/i; + +/** Keep an operator-facing NOTE readable; a gap names a check, not an essay. */ +const MAX_GAP_LENGTH = 160; +const MAX_GAPS_PER_AGENT = 8; + +/** + * Dangerous codepoints stripped from a gap before it can reach a terminal + * or a posted body: C0 and C1 controls (U+009B is an 8-bit CSI), DEL, the + * Unicode line separators (U+2028/29 — ECMAScript line terminators, which + * would otherwise truncate silently downstream), and the bidi embedding / + * override range U+202A-U+202E. + */ +const DANGEROUS_CHARS_RE = + // eslint-disable-next-line no-control-regex + /[\u0000-\u001f\u007f-\u009f\u2028\u2029\u202a-\u202e]/g; + +/** Markdown wrapper pairs stripped only SYMMETRICALLY — never one side. */ +const WRAPPER_PAIRS: Array<[string, string]> = [ + ['**', '**'], + ['__', '__'], + ['*', '*'], + ['_', '_'], + ['~', '~'], + ['`', '`'], +]; + +function stripWrappers(s: string): string { + let out = s; + let changed = true; + while (changed) { + changed = false; + for (const [open, close] of WRAPPER_PAIRS) { + if ( + out.length > open.length + close.length && + out.startsWith(open) && + out.endsWith(close) + ) { + out = out.slice(open.length, out.length - close.length).trim(); + changed = true; + } + } + } + return out; +} + +/** Truncate on code points — a slice through a surrogate pair is mojibake. */ +function truncateGap(s: string): string { + const points = [...s]; + return points.length > MAX_GAP_LENGTH + ? `${points.slice(0, MAX_GAP_LENGTH).join('')}…` + : s; +} + +/** + * Every budget-gap disclosure in an agent's final return, sanitized for the + * two places it lands: an operator's terminal (stderr NOTE) and the posted + * review body. Dangerous codepoints are stripped, each gap is capped in + * length (on code points) and the list in count, duplicates are folded + * (an agent that states its gap mid-return and restates it in the summary + * disclosed one gap, not two), and placeholder text (the brief's own + * `` template, `none` in any punctuation) is dropped rather + * than handed to the orchestrator as a gap to rule on. + */ +export function budgetGapDisclosures(finalText: string): string[] { + if (!GAP_HINT_RE.test(finalText)) return []; + const gaps: string[] = []; + const seen = new Set(); + let inFence = false; + for (const line of finalText.split(/\r?\n/)) { + if (/^[ \t]*(?:```|~~~)/.test(line)) { + inFence = !inFence; + continue; + } + if (inFence) continue; + // A blockquote is a quotation by definition; the agent is citing the + // format (a brief, another agent's return), not using it. + if (/^[ \t]*>/.test(line)) continue; + // Sanitized BEFORE matching: U+2028/29 are line terminators to the + // regex dot, and a gap carrying one would otherwise fail the match and + // vanish — silent loss in a channel whose promise is delivery. + const m = BUDGET_GAP_LINE_RE.exec(line.replace(DANGEROUS_CHARS_RE, ' ')); + if (!m) continue; + // A line written as a code span (`Budget gap: …`) is only taken when + // the backtick closes — and then unwrapped with its partner, so a + // symbol the gap itself names in backticks keeps both of its own. + let raw = m[2] ?? ''; + if (m[1] === '`') { + if (!raw.endsWith('`')) continue; + raw = raw.slice(0, -1); + } + raw = stripWrappers(raw.trim()).trim(); + const normalized = raw.replace(/[.!…,;:\s]+$/, '').trim(); + if (normalized.length === 0 || PLACEHOLDER_GAP_RE.test(normalized)) { + continue; + } + const key = normalized.toLowerCase(); + if (seen.has(key)) continue; + seen.add(key); + gaps.push(truncateGap(raw)); + if (gaps.length >= MAX_GAPS_PER_AGENT) break; + } + return gaps; +} + +/** + * `finalText` with its budget-gap disclosure lines removed — what the + * reverse-audit retirement judges a receipt on, so an agent's admission of + * what it skipped can neither serve as the receipt's substance nor block a + * receipt that is substantive without it. + */ +export function stripBudgetGapLines(finalText: string): string { + if (!GAP_HINT_RE.test(finalText)) return finalText; + const kept: string[] = []; + let inFence = false; + for (const line of finalText.split(/\r?\n/)) { + const fence = /^[ \t]*(?:```|~~~)/.test(line); + if (fence) inFence = !inFence; + if ( + !fence && + !inFence && + !/^[ \t]*>/.test(line) && + BUDGET_GAP_LINE_RE.test(line) + ) { + continue; + } + kept.push(line); + } + return kept.join('\n'); +} + function sane(n: unknown): number { const v = Number(n); return Number.isFinite(v) && v > 0 ? Math.floor(v) : 0; diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts index cb4280c4997..57a51dba8ee 100644 --- a/packages/cli/src/commands/review/lib/coverage.ts +++ b/packages/cli/src/commands/review/lib/coverage.ts @@ -73,6 +73,7 @@ import { import { BRIEFS } from './agent-briefs.js'; import { chunkIdsProblem } from './diff-plan.js'; import { readBudgetStop } from './deadline.js'; +import { budgetGapDisclosures } from './budget.js'; import { shellQuotePath } from './shell-quote.js'; export interface CoverageFromTranscripts { @@ -154,6 +155,17 @@ export interface CoverageFromTranscripts { missingChunks: number[]; /** Chunk ids an agent declared unreachable. */ uncoverableChunks: number[]; + /** + * `Budget gap: ` lines parsed from agent returns — the fixed + * disclosure format the tool-budget brief mandates when an agent's soft + * ceiling stopped a check it wanted. Detection is deterministic (this + * parse); the RULING stays with the orchestrator, exactly as it does for + * whiffs: a gap naming an incomplete required trace joins + * `unreviewedDimensions` and caps Approve, a gap naming optional depth is + * disclosed in the report. An empty list on a budgeted run means no agent + * hit its ceiling mid-check. + */ + budgetGaps: Array<{ agent: string; gaps: string[] }>; /** Chunk ids a working agent actually reviewed. */ coveredChunks: number[]; /** @@ -478,6 +490,67 @@ export function coverageFromTranscripts( const superseded = (rec: AgentRecord, chunk: number | null): boolean => chunk !== null ? chunkSatisfied(chunk, rec) : keySatisfied(rec); + // Parsed once per record: the gap scan also feeds the supersession check + // below, and the parse is not free on a long return. + const gapsMemo = new Map(); + const gapsOf = (rec: AgentRecord): string[] => { + let g = gapsMemo.get(rec); + if (g === undefined) { + g = budgetGapDisclosures(rec.finalText); + gapsMemo.set(rec, g); + } + return g; + }; + // A record's gaps are silenced only by a GAP-FREE superseding record — a + // genuine repair. Two relaunches that both hit the ceiling and both + // disclose would otherwise supersede each other and drop every gap. + const gapsSuperseded = (rec: AgentRecord, chunk: number | null): boolean => { + if (chunk !== null) { + const b = builtOf(`chunk-${chunk}`); + if (b === undefined) return false; + return records.some( + (r) => + r !== rec && + assignedChunk(r) === chunk && + wasDeliveredVerbatim(r.launchPrompt, b) && + r.diffToolCalls > 0 && + gapsOf(r).length === 0, + ); + } + // A whole-diff record: same shape as `keySatisfied`, plus the gap-free + // requirement on the record that would do the superseding. + for (const key of built.keys()) { + const b = builtOf(key); + if (b === undefined) continue; + if (!wasDeliveredVerbatim(rec.launchPrompt, b)) continue; + const needle = JSON.stringify(briefPath(planPath, key)); + if ( + records.some( + (r) => + r !== rec && + wasDeliveredVerbatim(r.launchPrompt, b) && + r.successfulCallArgs.some((a) => a.includes(needle)) && + gapsOf(r).length === 0, + ) + ) { + return true; + } + } + return false; + }; + + // Budget-gap disclosures (`Budget gap: ` lines, the format the + // tool-budget brief mandates and `budgetGapDisclosures` parses). Collected + // inside the walk below so every guard the `Uncoverable:` claim earns + // applies here for the same reason: the brief hands each agent the literal + // template, so a zero-tool-call or blind agent that copied it back must + // not be credited with a disclosed gap — that is the whiff wearing a + // costume. Detection is deterministic here; the RULING (which gaps cap + // Approve) stays with the orchestrator, like whiffs. Not part of `ok`: a + // disclosed gap is the budget working, and failing the gate on it would + // teach agents not to disclose. + const budgetGaps: Array<{ agent: string; gaps: string[] }> = []; + for (const rec of records) { const chunk = assignedChunk(rec); const name = label(rec, chunk); @@ -579,10 +652,34 @@ export function coverageFromTranscripts( continue; } - // What it was told to read, plus what it demonstrably read. The second term is - // what lets an agent handed the bare diff path with no territory — a - // reverse-audit pass, a verifier — be credited for exactly the lines it opened - // and for no others. + // This record has passed every credit guard: it was given the diff, it + // worked, and if it was pointed at lines it opened the file they live + // in. Only now do its budget-gap lines count as disclosures. + // + // Disclosing costs NO coverage credit, on purpose — an earlier draft + // narrowed a disclosing agent's credit to its ranged reads, and that + // punished exactly the honest agent: `rangeOf` records only reads that + // carry a positive `limit`, so a compliant offset-paged or whole-file + // read left a discloser with zero credit and a hard gate failure, + // while an agent that stopped WITHOUT disclosing kept its full `told` + // credit. An asymmetry that only ever bites the discloser teaches + // agents not to disclose. The `told` presumption is the same for every + // agent; what a disclosed gap changes is the RULING (Step 3D), not the + // arithmetic. + // + // Suppression is gap-aware: a superseding record silences this one's + // gaps only if it has none itself — a relaunch that hits the same + // ceiling and discloses again must not let two compliant records + // mutually supersede every disclosure into silence. + const gaps = gapsOf(rec); + if (gaps.length > 0 && !gapsSuperseded(rec, chunk)) { + budgetGaps.push({ agent: name, gaps }); + } + + // What it was told to read, plus what it demonstrably read. The second + // term is what lets an agent handed the bare diff path with no + // territory — a reverse-audit pass, a verifier — be credited for + // exactly the lines it opened and for no others. const ranges = merge([...told, ...rec.diffReads]); if (ranges.length === 0) continue; @@ -876,6 +973,7 @@ export function coverageFromTranscripts( unreadBriefs, missingChunks, uncoverableChunks: [...uncoverable].sort((a, b) => a - b), + budgetGaps, coveredChunks: [...covered].sort((a, b) => a - b), plannedChunks: plan.chunks.map((c) => ({ id: c.id, diff --git a/packages/cli/src/commands/review/lib/retirement.test.ts b/packages/cli/src/commands/review/lib/retirement.test.ts index 8be15528613..2aa7afe28cb 100644 --- a/packages/cli/src/commands/review/lib/retirement.test.ts +++ b/packages/cli/src/commands/review/lib/retirement.test.ts @@ -196,6 +196,61 @@ describe('scheduleReverseAuditRound — the scheduler on its own', () => { expect(schedule(2).due).toEqual([13, 14, 15]); }); + it('a disclosure cannot BE the receipt — but cannot BLOCK a real one either', () => { + // Two directions, one rule: the receipt is judged with its + // `Budget gap:` lines stripped. A return whose only substance is its + // disclosures must not retire the chunk still owing the work (the + // admission doubling as the receipt). And a receipt substantive + // without them — a proven territory walk that found nothing new — + // must still retire, or a reverse auditor whose ceiling is routinely + // met (its brief orders the whole findings list read) makes + // convergence impossible and runs every budgeted loop to the round + // cap. The gap is coverage's to report and Step 3D's to rule on. + const ONLY_GAPS = + 'No new issues found —\n' + + 'Budget gap: the reconnect state machine walk\n' + + 'Budget gap: the two remaining changed-export call-site traces'; + const DRY_WITH_GAP = + DRY + '\nBudget gap: second-order callers outside this chunk'; + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY_WITH_GAP); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), DRY_WITH_GAP); + transcript(record(1, 14, 'chunk 14 round 1 territory walk'), ONLY_GAPS); + transcript(record(2, 14, 'chunk 14 round 2 territory walk'), ONLY_GAPS); + record(1, 15, 'chunk 15 round 1 territory walk'); + record(2, 15, 'chunk 15 round 2 territory walk'); + + const r3 = schedule(3); + // 13 retires on its substantive-without-gaps receipts; 14's + // gaps-as-receipt returns keep it due. + expect(r3.due).toEqual([14, 15]); + expect(r3.skipped).toEqual([ + { chunkId: 13, dryRounds: [1, 2], nextColdCheck: 4 }, + ]); + expect(r3.converged).toBe(false); + }); + + it('an inline disclosure cannot lend the receipt its substance', () => { + // A one-line return puts the disclosure AFTER the receipt separator, + // where the line-based strip cannot see it — and the clause capture + // would absorb the gap text and pass the substance check on it. The + // clause is cut at the inline marker first: with nothing before the + // disclosure, the receipt is bare and the chunk stays due. A zh + // disclosure counts the same — the receipt regex accepts zh receipts, + // so the guard must too. + const INLINE = 'No new issues found — Budget gap: the remaining traces'; + const INLINE_ZH = '未发现新问题——预算缺口:其余调用点追踪'; + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), INLINE); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), INLINE); + transcript(record(1, 14, 'chunk 14 round 1 territory walk'), INLINE_ZH); + transcript(record(2, 14, 'chunk 14 round 2 territory walk'), INLINE_ZH); + record(1, 15, 'chunk 15 round 1 territory walk'); + record(2, 15, 'chunk 15 round 2 territory walk'); + + const r3 = schedule(3); + expect(r3.due).toEqual([13, 14, 15]); + expect(r3.skipped).toEqual([]); + }); + it('a chunk twice dry retires on the odd round and cold-checks on the even one', () => { transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); transcript(record(2, 13, 'chunk 13 round 2 territory walk'), DRY); diff --git a/packages/cli/src/commands/review/lib/retirement.ts b/packages/cli/src/commands/review/lib/retirement.ts index 02ad7623425..4915e38f651 100644 --- a/packages/cli/src/commands/review/lib/retirement.ts +++ b/packages/cli/src/commands/review/lib/retirement.ts @@ -48,6 +48,7 @@ import { promptRecordDir, readRecordedPrompts, } from './prompt-record.js'; +import { stripBudgetGapLines, INLINE_BUDGET_GAP_RE } from './budget.js'; /** What one prior audit of one chunk provably produced. */ export type AuditOutcome = 'yielded' | 'dry' | 'unknown'; @@ -319,13 +320,39 @@ function classifyReturn( return 'yielded'; } } - const receipt = DRY_RECEIPT_RE.exec(text); + // The receipt is judged WITHOUT its budget-gap disclosure lines. Two + // failure modes bound this from opposite sides. An auditor's admission of + // what its soft ceiling cut short must not double as the receipt's + // substantive clause — stripped, a return whose only substance was its + // disclosures reads `unknown` and the chunk stays under audit. But a + // receipt that is substantive WITHOUT them — a real walk of the + // territory, proven by the same tool-call and territory-read bar as + // ever, that found nothing new and separately disclosed exploration it + // did not take — still retires: an earlier draft read any gap-bearing + // return as `unknown`, and since a reverse auditor's ceiling is routinely + // met (its brief orders a 65-82 KB findings list read in full), that made + // convergence impossible and ran every budgeted loop to the round cap — + // the exact never-retire failure this module's own docstrings warn + // about. The gap itself is not lost: coverage reports it and Step 3D + // rules on it; retirement certifies the audit that DID happen, not the + // exploration that did not. + const judged = stripBudgetGapLines(text); + const receipt = DRY_RECEIPT_RE.exec(judged); + // The clause is cut at any INLINE disclosure marker before its substance + // is judged: a one-line return (`No new issues found — …; Budget gap: X`) + // slips past the line-based strip, and the `[\s\S]*` capture would + // otherwise absorb the gap text and get its substantiveness from it — + // the admission doubling as the receipt again, one line lower. + const clause = receipt?.[1] ?? ''; + const inlineGap = INLINE_BUDGET_GAP_RE.exec(clause); + const judgedClause = + inlineGap === null ? clause : clause.slice(0, inlineGap.index); if ( rec.successfulToolCalls > 0 && rec.diffToolCalls > 0 && openedTheTerritory(rec.diffReads, territory) && receipt !== null && - substantiveClause(receipt[1] ?? '') + substantiveClause(judgedClause) ) { return 'dry'; } diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 178e352f641..90fc8b651d9 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -932,3 +932,7 @@ Every one of six measured CI reviews (2026-08-05/06) spent 1.5–3 minutes at St ### The one-command-per-turn tail Measured across the same six CI reviews: the post-verdict bookkeeping — Markdown report, cost-ledger, save-artifact, `record_artifact`, the incremental-cache write, cleanup — ran one command per model turn, 4–6 minutes of wall clock after the review's outcome was already decided (and, on posting runs, already on the PR), stretching past 7 minutes when the qwen-home fumbling above joined it. Every command in the tail is cheap; the turns are not — the same arithmetic that batches the Step 1 setup calls, unapplied to the other end of the run. + +### The forty-one minute wave + +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. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index b13779cbaf2..16c3ae22da9 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -194,7 +194,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. **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, 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. A chunk is read with `read_file(file_path=diffPathAbsolute, offset=startLine - 1, limit=endLine - startLine + 1)` — `offset` is 0-based. @@ -413,6 +413,8 @@ It reads the harness's own per-agent transcripts: a record you do not author, ar Why this is a command and not a paragraph: **the review approved a pull request that no agent read.** Every prose defence against exactly this failure went unperformed in a real dogfood (measured; DESIGN.md — The Approve over an unread diff). +**The coverage report also carries `budgetGaps`** — the `Budget gap: ` lines agents disclosed when the soft tool-call ceiling stopped a check (the format is fixed so this detection is a parse, not a memory; it never fails the gate, because failing on disclosure teaches agents not to disclose). Detection is the CLI's; the ruling is yours, exactly as with whiffs: a gap that names an incomplete **required** trace — the callers of a changed export, a security path, the re-establishment of removed behaviour — joins `unreviewedDimensions`, which forbids an Approve; a gap naming only optional depth is carried into the report's "Not reviewed" section — `compose-review` renders every parsed gap there mechanically, so the disclosure reaches the author even if you relay nothing; your ruling adds only the capping entries. A budget gap is the ceiling working, not an agent failing — never relaunch an agent over one. A disclosure costs no coverage credit and never fails the gate — an arithmetic that only ever bites the discloser teaches agents not to disclose. One consequence is the CLI's, not yours: the reverse-audit retirement judges a receipt with its `Budget gap:` lines stripped, so the disclosure can neither serve as the receipt's substance (a return whose only substance is its gaps does not retire its chunk) nor block a receipt that is substantive without it (a proven territory walk that found nothing new still retires — the gap is ruled on, not re-audited). When your ruling promotes a gap into `unreviewedDimensions`, write it self-explained, with the gap's own text as the scope — ` — stopped at the agent tool budget` — the em-dash reason renders verbatim instead of under the whiffed-agent explanation, and `compose-review` drops its own mechanical line for any gap your entry echoes, so the body never says it twice. + The roll-call below is still worth writing for your own reading — but it is not what stops this any more: ```