Enforce a per-run cost ceiling as a runaway guard

CHANGELOG.md

@@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 
 ### Added
 
+- A per-review cost ceiling guards against a runaway review: $8 plus $4 for each part of the diff, overridable with `CODE_REVIEW_MAX_RUN_USD`. Reviewer calls not started once it is reached are skipped and shown as `skipped: run budget` in the check run, synthesis still runs on what was found, and the summary warns that the review is incomplete ([#50](https://github.com/NCodeGroup/code-review/issues/50)).
 - A verification stage, off by default: with `CODE_REVIEW_VERIFICATION=on`, a Sonnet call with the checkout proves or refutes each merged blocker and should-fix, a file's findings at a time at up to $0.50 a call. Confirmed findings get the code path in their evidence, refuted ones are dropped and listed with the reason under "Refuted by verification" in the check run and in the trace, and ones it can't settle become questions one severity lower. The summary names the refuted findings, and verification turns a request for changes with nothing serious left into a comment, never an approval. Nits aren't verified, and the calls show as `verification` in the cost table. It stays off until the eval shows a gain, and the eval report now counts posted blockers and should-fixes and refuted findings ([#41](https://github.com/NCodeGroup/code-review/issues/41)).
 
 ### Changed

README.md

@@ -16,6 +16,8 @@ repository under review. Results and the trace are scanned for secrets before th
 
 Besides the reviewers, **rule checks** go through the rules the context stage picked for the pull request, twelve at a time, on Sonnet: each call checks its rules one by one against every part of the diff, searching the changed files for what the rule's check names. They report as `rule-check` and run on every review, outside the reviewer roster, so selection can't skip them. In the evals they caught about one more expected finding in 55, at about 70% more cost. `CODE_REVIEW_RULE_CHECKS=off` turns them off.
 
+Each model call has its own cost cap, and a whole review has a **cost ceiling** that guards against a runaway review only: $8 plus $4 for each part of the diff, so $12 for most pull requests and $32 for one reviewed in six parts, far above the $0.60 to $2.10 a normal review costs and the $6 to $7 of a 2 MB pull request. Once the calls so far reach it, the reviewer and rule-check calls that haven't started are skipped, each such reviewer shows `skipped: run budget` in the check run, synthesis, and verification when it's on, still run on what was found, and the summary warns that the review is incomplete and doesn't approve. To change it, set `CODE_REVIEW_MAX_RUN_USD` to a positive number of dollars in the calling job's `env`; any other value warns and keeps the default.
+
 **Verification** is off by default. `CODE_REVIEW_VERIFICATION=on` adds a stage after synthesis: each blocker and should-fix it merged goes to a Sonnet call with Read, Grep and Glob in the checkout, behind the same path guard as the reviewers, with its diff hunk and its reviewers' charters, a file's findings per call at up to $0.50 each. The call proves or refutes the finding with a concrete code path. A confirmed finding stays, with the path added to its evidence; a refuted one is dropped and listed in the check run under "Refuted by verification", with the reason; one it can neither prove nor refute stays as a question, one severity lower. Nits aren't verified. The summary, which synthesis wrote before verification, gets a sentence naming the findings verification refuted and counting the ones it made questions. Verification never makes a review an approval: a request for changes with no blocker or should-fix left becomes a comment, since a verifier misled by the change's own code can refute a real finding. The calls show as `verification` in the check run's cost table and the trace records each one. It stays off until an eval shows it catches as much at fewer blockers and should-fixes, for what it costs ([#41](https://github.com/NCodeGroup/code-review/issues/41)).
 
 ## Using it

src/config.ts

@@ -34,7 +34,7 @@ export function reviewerTiers(override = process.env['CODE_REVIEW_TIERS'] ?? '')
   return tiers;
 }
 
-// Per-run cost caps in USD, checked by the Agent SDK against its own estimate, which can overshoot a little.
+// Per-call cost caps in USD, checked by the Agent SDK against its own estimate, which can overshoot a little.
 export const budgets = {
   selection: 0.25,
   // Per reviewer, by model: Opus costs twice what Sonnet does for the same work, and a reviewer cut off at its cap
@@ -43,9 +43,28 @@ export const budgets = {
   synthesis: 0.75,
   // Per verification call, on Sonnet.
   verification: 0.5,
-  run: 4,
 };
 
+// The ceiling on one review's total cost: a guard against a runaway review only, far above normal spend (about $0.60 to
+// $2.10 for a normal pull request, $6 to $7 for a 2 MB one in six parts). Once the calls so far reach it, the reviewer and
+// rule-check calls not yet started are skipped, and synthesis still runs on what was found.
+// CODE_REVIEW_MAX_RUN_USD overrides it with a positive number of dollars; anything else warns and keeps the default.
+export const runCeilingBase = 8;
+export const runCeilingPerPart = 4;
+
+export function runCeiling(
+  parts: number,
+  override = process.env['CODE_REVIEW_MAX_RUN_USD'] ?? '',
+  warn: (message: string) => void = () => {},
+): number {
+  const fallback = runCeilingBase + runCeilingPerPart * Math.max(1, parts);
+  const value = override.trim();
+  if (!value) return fallback;
+  if (/^\d+(\.\d+)?$/.test(value) && Number(value) > 0) return Number(value);
+  warn(`CODE_REVIEW_MAX_RUN_USD isn't a positive number of dollars (${JSON.stringify(value.slice(0, 40))}), so the run's cost ceiling stays at $${fallback.toFixed(2)}.`);
+  return fallback;
+}
+
 export const maxTurns = { specialist: 30, verification: 20 };
 
 // Verification: after synthesis, Sonnet calls with the checkout try to prove or refute each blocker and should-fix, a

src/context.ts

@@ -167,6 +167,7 @@ export async function selectContext(
   rules: IndexedRule[],
   decisions: Decision[],
   describe: (gathered: Gathered) => string,
+  agent: typeof runAgent = runAgent,
 ): Promise<{ context: Context; result: AgentResult<{ rules: string[]; decisions: string[] }> | undefined }> {
   // The records the pull request adds or changes are part of the change, so every reviewer gets them without a pick.
   const proposed = decisions.filter((d) => d.proposed);
@@ -177,7 +178,7 @@ export async function selectContext(
     ...rules.map((r) => `${r.id}: ${r.summary}`),
     ...accepted.map((d) => `${d.id}: ${d.title}`),
   ].join('\n');
-  const result = await runAgent<{ rules: string[]; decisions: string[] }>(
+  const result = await agent<{ rules: string[]; decisions: string[] }>(
     { provider: run.provider, proxyUrl: run.proxyUrl },
     {
       role: 'context',

src/results.ts

@@ -7,7 +7,7 @@ import type { RefutedFinding } from './verify.ts';
 
 export interface ReviewerResult {
   name: string;
-  status: 'ran' | 'failed';
+  status: 'ran' | 'failed' | 'skipped';
   model: string;
   findingCount: number;
   topSeverity?: Severity;
@@ -103,8 +103,8 @@ export function summaryComment(results: Results, checkUrl: string, interactUrl:
     lines.push(`| synthesis | ${shortModel(results.synthesisModel)} | ${count} | ${severityDot[topSeverity(results.findings)!]} |`);
   }
   for (const r of [...results.reviewers].sort((a, b) => a.name.localeCompare(b.name))) {
-    const top = r.status === 'failed' ? '⚠️' : r.topSeverity ? severityDot[r.topSeverity] : '🟢';
-    lines.push(`| ${cell(r.name)} | ${shortModel(r.model)} | ${r.status === 'failed' ? 'failed' : r.findingCount} | ${top} |`);
+    const top = r.status === 'failed' ? '⚠️' : r.status === 'skipped' ? '⏭️' : r.topSeverity ? severityDot[r.topSeverity] : '🟢';
+    lines.push(`| ${cell(r.name)} | ${shortModel(r.model) || '—'} | ${r.status === 'ran' ? r.findingCount : r.status} | ${top} |`);
   }
   lines.push('');
   if (results.skipped.length) lines.push(`*Skipped: ${results.skipped.join(', ')}*`, '');

src/review.ts

@@ -5,7 +5,7 @@ import { tmpdir } from 'node:os';
 import path from 'node:path';
 import { runAgent, type AgentRun, type CallRecord } from './agent.ts';
 import { mapWithConcurrency } from './concurrency.ts';
-import { budgets, findingsPerExtraChunk, maxFindings, maxFindingsCap, maxTurns, minConfidence, reviewerTiers, ruleChecksOn, rulesPerCheck, verificationOn, type ModelTier } from './config.ts';
+import { budgets, findingsPerExtraChunk, maxFindings, maxFindingsCap, maxTurns, minConfidence, reviewerTiers, ruleChecksOn, rulesPerCheck, runCeiling, verificationOn, type ModelTier } from './config.ts';
 import { guidanceRules, repositoryRules, selectContext, type Decision, type IndexedRule } from './context.ts';
 import { splitDiff, type ChangedFile, type Gathered } from './gather.ts';
 import { verifyReview, type RefutedFinding, type VerificationCall } from './verify.ts';
@@ -33,7 +33,8 @@ export interface ReviewedFinding extends Finding {
 
 export interface ReviewerOutcome {
   name: string;
-  status: 'ran' | 'failed';
+  // Skipped: the run's cost ceiling was reached before any of the reviewer's calls started.
+  status: 'ran' | 'failed' | 'skipped';
   findings: Finding[];
   error?: string;
 }
@@ -269,14 +270,29 @@ export function changedLines(diff: string): Map<string, Set<number>> {
   return lines;
 }
 
-export async function review(run: AgentRun, gathered: Gathered, charters: Charters, log: (line: string) => void): Promise<Review> {
+// Why a reviewer call that never started was skipped, in the check run.
+export const runBudgetReason = 'run budget';
+
+export async function review(
+  run: AgentRun,
+  gathered: Gathered,
+  charters: Charters,
+  log: (line: string) => void,
+  agent: typeof runAgent = runAgent,
+): Promise<Review> {
   const calls: CallRecord[] = [];
+  const spent = () => calls.reduce((sum, c) => sum + c.costUsd, 0);
   const roster = [...charters.reviewers.keys()];
 
   // Context: the rules and decisions that bear on this change.
   const catalog = [...guidanceRules(gathered.guidance), ...repositoryRules(gathered.reviewRules)];
-  const { context, result: contextResult } = await selectContext(run, gathered, catalog, gathered.decisions, (g) =>
-    `${pullRequestBlock(g)}\n\nChanged files:\n${fileList(g)}\n\n<diff>\n${g.diff.slice(0, 80_000)}\n</diff>`,
+  const { context, result: contextResult } = await selectContext(
+    run,
+    gathered,
+    catalog,
+    gathered.decisions,
+    (g) => `${pullRequestBlock(g)}\n\nChanged files:\n${fileList(g)}\n\n<diff>\n${g.diff.slice(0, 80_000)}\n</diff>`,
+    agent,
   );
   if (contextResult) calls.push(contextResult.record);
   const picked = [...new Set([...context.rulesFor.values()].flat())].length;
@@ -286,7 +302,7 @@ export async function review(run: AgentRun, gathered: Gathered, charters: Charte
   const hasRules = (context.rulesFor.get('conventions')?.length ?? 0) > 0;
   const floor = selectionFloor(gathered.files, Boolean(gathered.pullRequest.body), hasRules && charters.reviewers.has('conventions'));
   const selected = new Set(floor);
-  const selection = await runAgent<{ add: { reviewer: string; reason: string }[] }>(
+  const selection = await agent<{ add: { reviewer: string; reason: string }[] }>(
     { provider: run.provider, proxyUrl: run.proxyUrl },
     {
       role: 'selection',
@@ -332,8 +348,15 @@ export async function review(run: AgentRun, gathered: Gathered, charters: Charte
     ];
   });
   const first = jobs.filter((job, i) => jobs.findIndex((other) => other.tier === job.tier && other.part === job.part) === i);
+  // A runaway guard only: calls already running finish, and each call's own cap bounds how far they go past it.
+  const ceiling = runCeiling(parts.length, undefined, (message) => log(`::warning::${message}`));
   const reviewOne = async ({ name, part, label, tier, prompt }: (typeof jobs)[number]): Promise<ReviewerOutcome> => {
-    const result = await runAgent<{ findings: Finding[] }>(reviewRun, {
+    const where = [parts.length > 1 ? `part ${part + 1}` : '', label].filter(Boolean).join(', ');
+    if (spent() >= ceiling) {
+      log(`  ${name}${where ? ` (${where})` : ''}: skipped (${runBudgetReason})`);
+      return { name, status: 'skipped', findings: [], error: runBudgetReason };
+    }
+    const result = await agent<{ findings: Finding[] }>(reviewRun, {
       role: `reviewer:${name}`,
       tier,
       systemPrompt: charters.specialistPrompt,
@@ -345,7 +368,6 @@ export async function review(run: AgentRun, gathered: Gathered, charters: Charte
     calls.push(result.record);
     const reported = result.output?.findings ?? [];
     const kept = reported.filter((f) => f.confidence >= minConfidence);
-    const where = [parts.length > 1 ? `part ${part + 1}` : '', label].filter(Boolean).join(', ');
     log(`  ${name}${where ? ` (${where})` : ''}: ${result.error ? `failed (${result.error})` : `${kept.length} findings${reported.length > kept.length ? ` (${reported.length - kept.length} below ${minConfidence} confidence dropped)` : ''}`} $${result.record.costUsd.toFixed(3)}`);
     return result.error ? { name, status: 'failed', findings: [], error: result.error } : { name, status: 'ran', findings: kept };
   };
@@ -354,17 +376,28 @@ export async function review(run: AgentRun, gathered: Gathered, charters: Charte
     ...(await mapWithConcurrency(first, concurrency, reviewOne)),
     ...(await mapWithConcurrency(jobs.filter((job) => !first.includes(job)), concurrency, reviewOne)),
   ];
-  // One outcome per reviewer across the parts and rule batches: it ran if any of its calls ran.
+  // One outcome per reviewer across the parts and rule batches: it ran if any of its calls ran, and was skipped only if
+  // all of them were.
   const outcomes: ReviewerOutcome[] = [...selected, ...(batches.length ? [ruleCheckName] : [])].map((name) => {
     const mine = perJob.filter((o) => o.name === name);
-    const errors = mine.flatMap((o) => (o.error ? [o.error] : []));
+    const status = mine.some((o) => o.status === 'ran') ? 'ran' : mine.some((o) => o.status === 'failed') ? 'failed' : 'skipped';
+    const errors = mine.flatMap((o) => (o.status === 'failed' && o.error ? [o.error] : []));
+    const skippedCalls = mine.filter((o) => o.status === 'skipped').length;
+    if (skippedCalls) errors.push(status === 'skipped' ? runBudgetReason : `${skippedCalls} of ${mine.length} calls skipped: ${runBudgetReason}`);
     return {
       name,
-      status: mine.some((o) => o.status === 'ran') ? 'ran' : 'failed',
+      status,
       findings: mine.flatMap((o) => o.findings),
       ...(errors.length ? { error: errors.join('; ') } : {}),
     };
   });
+  const skippedJobs = perJob.filter((o) => o.status === 'skipped').length;
+  let ceilingWarning = '';
+  if (skippedJobs) {
+    const names = [...new Set(perJob.filter((o) => o.status === 'skipped').map((o) => o.name))].join(', ');
+    ceilingWarning = `This review reached its $${ceiling.toFixed(2)} cost ceiling ($${spent().toFixed(2)} spent) and skipped ${skippedJobs} of ${jobs.length} reviewer calls (${names}), so it is incomplete.`;
+    log(`::warning::${ceilingWarning}`);
+  }
 
   // Synthesis.
   const limit = findingLimit(parts.length);
@@ -374,7 +407,7 @@ export async function review(run: AgentRun, gathered: Gathered, charters: Charte
   let findings: ReviewedFinding[] = [];
   if (raw.length) {
     const diff = synthesisDiff(gathered, raw.map((f) => f.file), 120_000);
-    const synthesis = await runAgent<{ summary: string; verdict: Verdict; findings: (Finding & { sources: string[] })[] }>(
+    const synthesis = await agent<{ summary: string; verdict: Verdict; findings: (Finding & { sources: string[] })[] }>(
       { provider: run.provider, proxyUrl: run.proxyUrl },
       {
         role: 'synthesis',
@@ -410,11 +443,17 @@ export async function review(run: AgentRun, gathered: Gathered, charters: Charte
     charters: new Map([...charters.reviewers, [ruleCheckName, charters.ruleCheckPrompt]]),
     log,
     enabled: verificationOn(),
+    agent,
   });
   calls.push(...verified.calls);
   ({ summary, verdict, findings } = verified);
 
   const skipped = roster.filter((name) => !selected.has(name));
   verdict ??= findings.some((f) => f.severity === 'blocker') ? 'request-changes' : findings.length ? 'comment' : 'approve';
+  if (ceilingWarning) {
+    summary = `${summary}\n\n⚠️ ${ceilingWarning}`;
+    // A review cut short hasn't seen enough of the change to approve it.
+    if (verdict === 'approve') verdict = 'comment';
+  }
   return { reviewers: outcomes, summary, verdict, findings, refuted: verified.refuted, verification: verified.trace, calls, skipped };
 }

test/engine.test.ts

@@ -269,6 +269,7 @@ test('summaryComment shows reviewers, models, counts and the worst severity', as
         { name: 'tests', status: 'ran', model: 'global.anthropic.claude-haiku-5-5', findingCount: 1, topSeverity: 'should-fix' },
         { name: 'correctness', status: 'ran', model: 'global.anthropic.claude-sonnet-5-5', findingCount: 0 },
         { name: 'security', status: 'failed', model: 'global.anthropic.claude-sonnet-5-5', findingCount: 0, error: 'x' },
+        { name: 'architecture', status: 'skipped', model: '', findingCount: 0, error: 'run budget' },
       ],
       skipped: ['performance', 'data-model'],
       synthesisModel: 'global.anthropic.claude-opus-5-5',
@@ -287,6 +288,7 @@ test('summaryComment shows reviewers, models, counts and the worst severity', as
   assert.match(body, /\| synthesis \| opus-5-5 \| 1 \| 🟠 \|/);
   assert.match(body, /\| correctness \| sonnet-5-5 \| 0 \| 🟢 \|/);
   assert.match(body, /\| security \| sonnet-5-5 \| failed \| ⚠️ \|/);
+  assert.match(body, /\| architecture \| — \| skipped \| ⏭️ \|/);
   assert.match(body, /\*Skipped: performance, data-model\*/);
   assert.match(body, /Leaks \\| a pipe/);
   assert.match(body, /\$0\.69 on Amazon Bedrock, 71s, at abcdef1/);