Close the test gaps declined in review rounds

CHANGELOG.md

@@ -43,6 +43,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 - A missing test is a should-fix only when the untested path is new behavior with a plausible failure a user would see and no test below it covers it. A wrapper over tested pieces, a test that would only pin current behavior, or a second test for a covered path is a nit. Eval concerns can name a `minSeverity`, and five cases from [#30](https://github.com/NCodeGroup/code-review/pull/30) check both sides ([#37](https://github.com/NCodeGroup/code-review/issues/37)).
 - The inline threads read the pull request's earlier review comments through the same pager as the summary comment, up to 10,000 instead of 1,000. When a pull request has more, no threads are posted, with a warning, instead of findings already posted on comments past the limit being posted again; the check run still has every finding ([#38](https://github.com/NCodeGroup/code-review/issues/38)).
 - The Release workflow fails before it tags when `CHANGELOG.md` has no dated `## [<version>] - YYYY-MM-DD` section with entries, bulleted or numbered, for the version, outside any code fence, and publishes that section as the release's notes instead of generated ones. The README says why Dependabot proposed no bump of the action to consumers, every release so far being marked as a pre-release, and how to bump them meanwhile ([#33](https://github.com/NCodeGroup/code-review/issues/33)).
+- The tests cover how a review builds each reviewer's prompt on a diff in parts (its own part's diff, rules and charter, with the other parts' files marked as in another part), run the repository's own reviewers on the tier its config gives them, and rerun the selection and context calls on Bedrock when the API credits run out ([#96](https://github.com/NCodeGroup/code-review/issues/96)).
 
 ### Fixed
 

test/fallback.test.ts

@@ -10,6 +10,7 @@ import {
   lastModel,
   runAgent,
   totalCost,
+  withoutTools,
   type AgentRequest,
   type AgentRun,
   type BedrockFallback,
@@ -56,6 +57,7 @@ function outputFor(options: Options): unknown {
   const properties = (options.outputFormat?.schema['properties'] ?? {}) as Record<string, unknown>;
   const finding = { severity: 'nit', file: 'config/greeting.json', line: 1, title: 'Typo', problem: 'p', consequence: 'c', fix: 'f', evidence: 'e', confidence: 0.9 };
   if ('add' in properties) return { add: [{ reviewer: 'tests', reason: 'the change needs tests' }] };
+  if ('rules' in properties) return { rules: ['R-review-1'], decisions: [] };
   if ('summary' in properties) return { summary: 'One typo.', verdict: 'comment', findings: [{ ...finding, sources: ['R1', 'R2'] }] };
   return { findings: [finding] };
 }
@@ -135,25 +137,22 @@ function gathered(): Gathered {
   };
 }
 
-// Reviews the typo fix, in a file that isn't documentation so the review isn't a light one, with the hygiene reviewer
-// (the floor) and the tests reviewer (added by selection), on a run that starts on the Claude API with a Bedrock key;
-// the Claude API refuses only the tests reviewer's call.
-async function reviewWithTestsRefused(bedrockFails: (prompt: string) => boolean = () => false) {
+// Reviews the change with the engine's charters through the fake Agent SDK, on a run that starts on the Claude API with
+// a Bedrock key.
+async function reviewOnFakeSdk(input: Gathered, apiRefuses: (prompt: string) => boolean, bedrockFails: (prompt: string) => boolean = () => false) {
   const previousTiers = process.env['CODE_REVIEW_TIERS'];
   delete process.env['CODE_REVIEW_TIERS'];
-  const charters = loadCharters(root);
-  const testsCharter = `Your charter:\n${charters.reviewers.get('tests')!.text}`;
   const seen: SeenCall[] = [];
   const fallback: BedrockFallback = { switched: false };
   const run: AgentRun = {
     provider: 'anthropic',
     proxyUrl: 'http://127.0.0.1:1',
     checkout: mkdtempSync(path.join(tmpdir(), 'fallback-checkout-')),
     fallback,
-    query: fakeQuery(seen, (prompt) => prompt.includes(testsCharter), (prompt) => prompt.includes(testsCharter) && bedrockFails(prompt)),
+    query: fakeQuery(seen, apiRefuses, bedrockFails),
   };
   try {
-    const outcome = await review(run, gathered(), charters, () => {});
+    const outcome = await review(run, input, loadCharters(root), () => {});
     const byRole = (role: string) => outcome.calls.filter((c) => c.role === role).map((c) => [c.provider, c.model, c.outcome]);
     return { outcome, fallback, seen, byRole };
   } finally {
@@ -162,6 +161,13 @@ async function reviewWithTestsRefused(bedrockFails: (prompt: string) => boolean
   }
 }
 
+// Reviews the typo fix, in a file that isn't documentation so the review isn't a light one, with the hygiene reviewer
+// (the floor) and the tests reviewer (added by selection); the Claude API refuses only the tests reviewer's call.
+async function reviewWithTestsRefused(bedrockFails: (prompt: string) => boolean = () => false) {
+  const testsCharter = `Your charter:\n${loadCharters(root).reviewers.get('tests')!.text}`;
+  return reviewOnFakeSdk(gathered(), (prompt) => prompt.includes(testsCharter), (prompt) => prompt.includes(testsCharter) && bedrockFails(prompt));
+}
+
 test('a credit error reruns that call on Bedrock and sends every later call there', async () => {
   const { outcome, fallback, seen, byRole } = await reviewWithTestsRefused();
 
@@ -195,6 +201,46 @@ test('a reviewer whose Bedrock rerun fails too is failed with the rerun\'s error
   assert.deepEqual(outcome.reviewers.find((r) => r.name === 'hygiene')?.status, 'ran');
 });
 
+test('a credit error on the selection call reruns it on Bedrock, and the review adds the reviewer the rerun picked', async () => {
+  const { outcome, fallback, byRole } = await reviewOnFakeSdk(gathered(), (prompt) => prompt.startsWith('Roster: '));
+
+  assert.deepEqual(fallback, { switched: true, reason: creditsRanOutReason });
+  assert.deepEqual(byRole('selection'), [
+    ['anthropic', modelIds.anthropic.sonnet, creditsRanOutReason],
+    ['bedrock', modelIds.bedrock.sonnet, 'success'],
+  ]);
+  assert.deepEqual(byRole('reviewer:tests'), [['bedrock', modelIds.bedrock.haiku, 'success']]);
+  assert.deepEqual(byRole('synthesis'), [['bedrock', modelIds.bedrock.opus, 'success']]);
+  assert.deepEqual(outcome.reviewers.map((r) => [r.name, r.status]), [['hygiene', 'ran'], ['tests', 'ran']]);
+});
+
+test('a credit error on the context call reruns it on Bedrock, and the reviewers get the rule the rerun picked', async () => {
+  const withRule: Gathered = { ...gathered(), reviewRules: [{ name: 'review.instructions.md', globs: [], text: '- Keep greetings lowercase.\n' }] };
+  const { outcome, fallback, byRole } = await reviewOnFakeSdk(withRule, (prompt) => prompt.includes('<catalog>'));
+
+  assert.deepEqual(fallback, { switched: true, reason: creditsRanOutReason });
+  assert.deepEqual(byRole('context'), [
+    ['anthropic', modelIds.anthropic.sonnet, creditsRanOutReason],
+    ['bedrock', modelIds.bedrock.sonnet, 'success'],
+  ]);
+  assert.deepEqual(byRole('selection'), [['bedrock', modelIds.bedrock.sonnet, 'success']]);
+  // The picked rule puts conventions on the floor and runs a rule check.
+  assert.deepEqual(outcome.reviewers.map((r) => [r.name, r.status]), [['hygiene', 'ran'], ['conventions', 'ran'], ['tests', 'ran'], ['rule-check', 'ran']]);
+});
+
+test('withoutTools keeps the run\'s provider, proxy, fallback and query, and drops its checkout and working folder', () => {
+  const fallback: BedrockFallback = { switched: false };
+  const query = fakeQuery([], () => false);
+  const run: AgentRun = { provider: 'anthropic', proxyUrl: 'http://127.0.0.1:1', checkout: '/checkout', workdir: '/work', fallback, query };
+  const bare = withoutTools(run);
+
+  assert.deepEqual(Object.keys(bare).sort(), ['fallback', 'provider', 'proxyUrl', 'query']);
+  assert.equal(bare.provider, 'anthropic');
+  assert.equal(bare.proxyUrl, 'http://127.0.0.1:1');
+  assert.equal(bare.fallback, fallback);
+  assert.equal(bare.query, query);
+});
+
 test('the Bedrock rerun gets the placeholder and the proxy, never a credential or a job variable', async () => {
   process.env['ACTIONS_ID_TOKEN_REQUEST_TOKEN'] = 'job-token';
   process.env['CODE_REVIEW_BEDROCK_KEY'] = 'ABSKsecret';

test/review-prompts.test.ts

@@ -0,0 +1,163 @@
+import assert from 'node:assert/strict';
+import { beforeEach, test } from 'node:test';
+import type { AgentRequest, AgentResult, AgentRun, CallRecord, runAgent } from '../src/agent.ts';
+import type { Gathered } from '../src/gather.ts';
+import { review, type Charters } from '../src/review.ts';
+
+// An agent that makes no model call: it records each request and answers from the test's table.
+function stubAgent(answer: (request: AgentRequest) => unknown) {
+  const requests: AgentRequest[] = [];
+  const agent = (async (_run: AgentRun, request: AgentRequest): Promise<AgentResult<unknown>> => {
+    requests.push(request);
+    const record: CallRecord = {
+      role: request.role, provider: 'bedrock', model: request.tier, costUsd: 0.01, inputTokens: 0, outputTokens: 0,
+      cacheReadTokens: 0, cacheWriteTokens: 0, turns: 1, durationMs: 0, outcome: 'success',
+    };
+    return { output: answer(request), records: [record] };
+  }) as typeof runAgent;
+  return { agent, requests };
+}
+
+const diffA = ['diff --git a/src/A.cs b/src/A.cs', '--- a/src/A.cs', '+++ b/src/A.cs', '@@ -1,1 +1,2 @@', ' line', '+added', ''].join('\n');
+const diffB = ['diff --git a/src/B.cs b/src/B.cs', '--- a/src/B.cs', '+++ b/src/B.cs', '@@ -1,1 +1,2 @@', ' line', '+added', ''].join('\n');
+
+// Two changed files of product code, each in a part of its own, and one learned rule for each file.
+const twoParts = (fields: Partial<Gathered> = {}): Gathered => ({
+  pullRequest: { repository: 'o/r', number: 7, title: 'Add two lines', author: 'someone', authorAssociation: 'MEMBER', authorCanWrite: true, draft: false, baseRef: 'main', baseSha: 'b'.repeat(40), headSha: 'a'.repeat(40) },
+  mergeBase: 'b'.repeat(40),
+  files: ['src/A.cs', 'src/B.cs'].map((path) => ({ path, additions: 1, deletions: 0, generated: false, shown: true })),
+  diff: `${diffA}${diffB}`,
+  chunks: [diffA, diffB],
+  conventions: '',
+  reviewRules: [],
+  guidance: [{
+    name: 'errors-and-validation.md',
+    globs: [],
+    text: ['- **Rule for A** why. *Check:* c *Applies to:* `src/A.cs`', '- **Rule for B** why. *Check:* c *Applies to:* `src/B.cs`'].join('\n'),
+  }],
+  decisions: [],
+  commits: [],
+  sensitive: [],
+  skippedByConfig: [],
+  policies: [],
+  tiers: {},
+  projectReviewers: [],
+  warnings: [],
+  ...fields,
+});
+
+const charters: Charters = {
+  specialistPrompt: 'SPECIALIST PROMPT',
+  selectionPrompt: 'SELECTION PROMPT',
+  synthesisPrompt: 'SYNTHESIS PROMPT',
+  ruleCheckPrompt: 'RULE-CHECK CHARTER',
+  verificationPrompt: 'VERIFICATION PROMPT',
+  reReviewPrompt: 'RE-REVIEW PROMPT',
+  reviewers: new Map(['correctness', 'hygiene', 'security', 'tests'].map((name) => [name, { text: `${name.toUpperCase()} CHARTER`, shadow: false }])),
+  shadowPrompts: {},
+};
+
+// The context stage picks both rules; selection adds no one; every reviewer finds nothing, so synthesis doesn't run.
+function answers(request: AgentRequest): unknown {
+  if (request.role === 'context') return { rules: ['G-errors-and-validation-1', 'G-errors-and-validation-2'], decisions: [] };
+  if (request.role === 'selection') return { add: [] };
+  return { findings: [] };
+}
+
+const run: AgentRun = { provider: 'bedrock', proxyUrl: 'http://127.0.0.1:1', checkout: '/nowhere' };
+
+// Every test runs on the default settings: clear any the runner's environment sets, so it can't change the result.
+beforeEach(() => {
+  for (const name of ['CODE_REVIEW_TIERS', 'CODE_REVIEW_RULE_CHECKS', 'CODE_REVIEW_SHADOW', 'CODE_REVIEW_SHADOW_RATE', 'CODE_REVIEW_VERIFICATION', 'CODE_REVIEW_MAX_RUN_USD']) delete process.env[name];
+});
+
+const reviewerCalls = (requests: AgentRequest[], part: number) =>
+  requests.filter((r) => r.role.startsWith('reviewer:') && String(r.prompt[0]).includes(`this is part ${part}.`));
+
+test("review() gives each part's reviewers that part's diff, and marks the other part's file as in another part", async () => {
+  const { agent, requests } = stubAgent(answers);
+  await review(run, twoParts(), charters, () => {}, agent);
+
+  for (const [part, own, other] of [[1, diffA, diffB], [2, diffB, diffA]] as const) {
+    const calls = reviewerCalls(requests, part);
+    assert.deepEqual(calls.map((r) => r.role).sort(), ['reviewer:correctness', 'reviewer:hygiene', 'reviewer:rule-check', 'reviewer:tests'], `part ${part}`);
+    for (const call of calls) {
+      const shared = String(call.prompt[0]);
+      assert.ok(shared.includes(`<diff>\n${own}\n</diff>`), `${call.role} in part ${part} has its part's diff`);
+      assert.ok(!shared.includes(other.split('\n')[0]!), `${call.role} in part ${part} has no other part's diff`);
+    }
+  }
+  assert.match(String(reviewerCalls(requests, 1)[0]!.prompt[0]), /\nsrc\/A\.cs \(\+1 -0\)\nsrc\/B\.cs \(\+1 -0\) \[diff in another part\]\n/);
+  assert.match(String(reviewerCalls(requests, 2)[0]!.prompt[0]), /\nsrc\/A\.cs \(\+1 -0\) \[diff in another part\]\nsrc\/B\.cs \(\+1 -0\)\n/);
+});
+
+test("review() gives each reviewer call its part's rules and its own charter, and the rule check the rule-check prompt", async () => {
+  const { agent, requests } = stubAgent(answers);
+  await review(run, twoParts(), charters, () => {}, agent);
+
+  for (const [part, own, other] of [[1, 'Rule for A', 'Rule for B'], [2, 'Rule for B', 'Rule for A']] as const) {
+    const calls = reviewerCalls(requests, part);
+    const charter = (role: string) => String(calls.find((r) => r.role === role)!.prompt[1]);
+    for (const role of ['reviewer:correctness', 'reviewer:rule-check']) {
+      assert.ok(charter(role).includes(own), `${role} in part ${part} has its part's rule`);
+      assert.ok(!charter(role).includes(other), `${role} in part ${part} has no other part's rule`);
+    }
+    assert.match(charter('reviewer:correctness'), /Your charter:\nCORRECTNESS CHARTER$/);
+    assert.match(charter('reviewer:rule-check'), /Your charter:\nRULE-CHECK CHARTER$/);
+    assert.match(charter('reviewer:hygiene'), /^\n\nYour charter:\nHYGIENE CHARTER$/);
+    assert.match(charter('reviewer:tests'), /^\n\nYour charter:\nTESTS CHARTER$/);
+  }
+});
+
+test('review() gives every reviewer call in each part the specialist system prompt', async () => {
+  const { agent, requests } = stubAgent(answers);
+  await review(run, twoParts(), charters, () => {}, agent);
+
+  for (const part of [1, 2]) {
+    const calls = reviewerCalls(requests, part);
+    assert.equal(calls.length, 4, `part ${part}`);
+    assert.ok(calls.every((r) => r.systemPrompt === 'SPECIALIST PROMPT'), `part ${part}`);
+  }
+});
+
+// Two project reviewers: ledger's paths match the changed file, billing's match none.
+const projectReviewers = [
+  { name: 'ledger', text: '# ledger\n\nDoes every entry balance?\n', paths: ['src/**/Ledger/**'], shadow: false },
+  { name: 'billing', text: '# billing\n\nAre amounts in cents?\n', paths: ['src/Billing/**'], shadow: false },
+];
+const ledgerChange = (): Gathered => {
+  const diff = diffA.replaceAll('src/A.cs', 'src/Server/Ledger/Entry.cs');
+  return twoParts({
+    files: [{ path: 'src/Server/Ledger/Entry.cs', additions: 1, deletions: 0, generated: false, shown: true }],
+    diff,
+    chunks: [diff],
+    guidance: [],
+    projectReviewers,
+    tiers: { 'project:ledger': 'opus' },
+  });
+};
+
+test("review() runs a project reviewer whose paths match a changed file, on the tier the repository's config gives it", async () => {
+  const { agent, requests } = stubAgent(answers);
+  const outcome = await review(run, ledgerChange(), charters, () => {}, agent);
+
+  const selection = String(requests.find((r) => r.role === 'selection')!.prompt);
+  assert.match(selection, /^Roster: correctness, hygiene, security, tests, project:ledger, project:billing\nThis repository's own reviewers:\n- project:ledger: Does every entry balance\?\n- project:billing: Are amounts in cents\?\nAlready chosen: [^\n]*\bproject:ledger\b/);
+  assert.doesNotMatch(selection, /Already chosen: [^\n]*project:billing/);
+  const ledger = requests.find((r) => r.role === 'reviewer:project:ledger')!;
+  assert.equal(ledger.tier, 'opus');
+  assert.match(String(ledger.prompt[1]), /Your charter:\n# ledger\n\nDoes every entry balance\?\n$/);
+  assert.ok(!requests.some((r) => r.role === 'reviewer:project:billing'));
+  assert.deepEqual(outcome.reviewers.find((o) => o.name === 'project:ledger')?.status, 'ran');
+  assert.ok(outcome.skipped.includes('project:billing'));
+});
+
+test('selection can add a project reviewer whose paths match no changed file, and it runs on Haiku by default', async () => {
+  const { agent, requests } = stubAgent((request) =>
+    request.role === 'selection' ? { add: [{ reviewer: 'project:billing', reason: 'it charges a fee' }] } : answers(request));
+  const outcome = await review(run, ledgerChange(), charters, () => {}, agent);
+
+  assert.equal(requests.find((r) => r.role === 'reviewer:project:billing')?.tier, 'haiku');
+  assert.deepEqual(outcome.reviewers.find((o) => o.name === 'project:billing')?.status, 'ran');
+  assert.ok(!outcome.skipped.includes('project:billing'));
+});