Test eval review cost and explain's answer

CHANGELOG.md

@@ -21,6 +21,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 - Verification no longer drops a refuted blocker or `security` finding: it stays as a question one severity lower, with the refutation in its evidence. Any refutation must cite a line of the checkout outside the lines the pull request adds; one that rests only on the change's own lines keeps the finding as a question ([#87](https://github.com/NCodeGroup/code-review/issues/87)).
 - Verification stops at the run's cost ceiling like the reviewer calls: its calls that haven't started are skipped, their findings stay unverified, and the summary says how many were skipped ([#87](https://github.com/NCodeGroup/code-review/issues/87)).
 - A re-review also runs `security` when what changed since the last review touches a file `sensitive:` names or code the selection floor gives to `security`, such as a path with `auth`, `token` or `crypt` in it, and a change to a `sensitive:` file makes it a deep review: `correctness` and `security` run on Opus, unless `CODE_REVIEW_TIERS` names them, verification checks the findings in sensitive files on Opus, and the summary and the check run say so. Each of its reviewers checks the earlier findings, and the most cautious status counts; when one of them fails, the re-review posts no findings and the next one covers those changes again ([#86](https://github.com/NCodeGroup/code-review/issues/86)).
+- The tests check that the evals' review cost leaves out the concern judge's and precision judge's calls, which the case cost counts, so `eval:compare` never reports the judges' spend as review cost ([#107](https://github.com/NCodeGroup/code-review/issues/107)).
+- The tests check what a `/review explain` run leaves for the publish step: only `answer.json` for an unknown finding, without a model call, `trace.json` too when the call ran, the `answered` output, and never `results.json` ([#108](https://github.com/NCodeGroup/code-review/issues/108)).
 
 ### Fixed
 

evals/run.ts

@@ -240,6 +240,29 @@ export function casePullRequest(c: EvalCase, headSha: string, readIssue: (number
   return pullRequest;
 }
 
+// The tallies the run's summary is built from, one entry per case.
+export interface CaseTally {
+  precisions: CasePrecision[];
+  findingsPerCase: number[];
+  reviewCosts: number[];
+}
+
+// Adds one case to the run's tallies and returns its costs. The review cost is only the review's own calls
+// (outcome.calls), while the case cost counts every call in `calls`, the concern judge's and precision judge's included,
+// so eval:compare never reports the judges' Opus spend as review cost.
+export function tallyCase(
+  tally: CaseTally,
+  outcome: { findings: readonly unknown[]; calls: readonly CallRecord[] },
+  calls: CallRecord[],
+  precision: CasePrecision | undefined,
+): { reviewCost: number; caseCost: number } {
+  if (precision) tally.precisions.push(precision);
+  tally.findingsPerCase.push(outcome.findings.length);
+  const reviewCost = outcome.calls.reduce((sum, call) => sum + call.costUsd, 0);
+  tally.reviewCosts.push(reviewCost);
+  return { reviewCost, caseCost: totalCost(calls) };
+}
+
 // How the output names a finding that raised a concern.
 const raisedLabel = (finding: Finding & { reviewers?: string[] }) => `${finding.file}:${finding.line} (${(finding.reviewers ?? []).join(', ')})`;
 
@@ -266,9 +289,7 @@ async function main(): Promise<void> {
   const shadowNames = new Set<string>();
   const verdictFile = path.join(root, 'evals', 'verdicts.json');
   const verdictCache = loadVerdicts(verdictFile);
-  const precisions: CasePrecision[] = [];
-  const findingsPerCase: number[] = [];
-  const reviewCosts: number[] = [];
+  const tally: CaseTally = { precisions: [], findingsPerCase: [], reviewCosts: [] };
 
   let capped: Awaited<ReturnType<typeof runCapped>>;
   try {
@@ -354,15 +375,8 @@ async function main(): Promise<void> {
         cache: verdictCache,
         judge: realityJudge({ ...run, checkout: head }, { title: c.title, diffFor: (file) => diffs.get(file) }, calls),
       });
-      if (precision) {
-        saveVerdicts(verdictFile, verdictCache);
-        precisions.push(precision);
-      }
-      findingsPerCase.push(outcome.findings.length);
-      const reviewCost = outcome.calls.reduce((sum, call) => sum + call.costUsd, 0);
-      reviewCosts.push(reviewCost);
-
-      const caseCost = totalCost(calls);
+      if (precision) saveVerdicts(verdictFile, verdictCache);
+      const { reviewCost, caseCost } = tallyCase(tally, outcome, calls, precision);
       expected += verdicts.length;
       caught += verdicts.filter((v) => v.caught).length;
       caughtRaw += verdicts.filter((v) => v.caughtRaw).length;
@@ -394,13 +408,13 @@ async function main(): Promise<void> {
     await proxy.close();
   }
 
-  const precision = precisionOn ? totalPrecision(precisions) : undefined;
+  const precision = precisionOn ? totalPrecision(tally.precisions) : undefined;
   const cap = capOutcome(maxCost, capped, cases.length, label);
   const summary = {
     label, tiers: process.env['CODE_REVIEW_TIERS'] ?? '', verification: verificationOn() ? 'on' : 'off', expected, caught, caughtRaw, violations,
     findings: findingCount, blockersAndShouldFixes: seriousCount, refuted: refutedCount,
-    precision, findingsPerCase: spread(findingsPerCase), reviewCostUsd: reviewCosts.reduce((a, b) => a + b, 0),
-    reviewCostPerCase: spread(reviewCosts), costUsd: cost,
+    precision, findingsPerCase: spread(tally.findingsPerCase), reviewCostUsd: tally.reviewCosts.reduce((a, b) => a + b, 0),
+    reviewCostPerCase: spread(tally.reviewCosts), costUsd: cost,
     shadow: { reviewers: [...shadowNames], caught: caughtShadow, violations: shadowViolations, findings: shadowFindingCount },
     ...cap.record,
   };

src/explain.ts

@@ -7,7 +7,7 @@ import { runAgent, type AgentResult, type AgentRun } from './agent.ts';
 import type { FindingRef } from './command.ts';
 import { errorText, readCommentPages } from './comments.ts';
 import { budgets, maxTurns } from './config.ts';
-import { assertNoSecrets } from './outputs.ts';
+import { assertNoSecrets, writeForPublish } from './outputs.ts';
 import { botLogin } from './results.ts';
 import { markerFields } from './threads.ts';
 
@@ -211,6 +211,24 @@ export async function answerFor(
   return { body, explained: { found, result } };
 }
 
+// What a `/review explain` run leaves for the publish step: answer.json, plus trace.json when the agent was asked. An
+// unknown ref writes only the answer, without asking. It never writes results.json, so the publish step posts no review.
+export async function writeAnswer(
+  outDir: string,
+  where: { repository: string; number: number; question: string; secrets: string[] },
+  ref: FindingRef,
+  resolution: Resolution,
+  ask: (found: FoundFinding) => Promise<AgentResult<Explanation>>,
+): Promise<void> {
+  const { body, explained } = await answerFor(ref, resolution, ask);
+  const files: Record<string, unknown> = { [answerFile]: { repository: where.repository, pullRequest: where.number, body } satisfies Answer };
+  if (explained) {
+    const { found, result } = explained;
+    files['trace.json'] = { explain: { id: found.id, comment: found.comment.html_url ?? found.comment.id, question: where.question, output: result.output, calls: result.records } };
+  }
+  writeForPublish(outDir, files, where.secrets, 'answered');
+}
+
 export type GitHubApi = (method: string, url: string, body?: unknown) => Promise<any>;
 
 // Posts the answer as a top-level comment, with the app's token, after the agent has exited. It is scanned for secrets

src/main.ts

@@ -13,13 +13,13 @@ import { decideRun, type Command, type CommentEvent } from './command.ts';
 import { errorText, readIssueComments, type ListedComment } from './comments.ts';
 import { AnthropicTokenSource, secretValues, type Credentials } from './credentials.ts';
 import { required, take } from './env.ts';
-import { answerFile, answerFor, explainFinding, findFinding, githubReader, type Answer, type FoundFinding } from './explain.ts';
+import { answerFile, explainFinding, findFinding, githubReader, writeAnswer, type FoundFinding } from './explain.ts';
 import { fetchCommit, fetchHeadFor, fetchPullRequestHead } from './fetch-head.ts';
 import { closedIssues, gather, readPullRequest } from './gather.ts';
 import { fetchPullRequest, runUrl } from './github.ts';
 import { readOpenIssues } from './issues.ts';
 import { isClosedEvent, recordOutcomes, tokenApi } from './outcomes.ts';
-import { assertNoSecrets } from './outputs.ts';
+import { writeForPublish } from './outputs.ts';
 import { checkPause, pausedStatus, type Paused } from './pause.ts';
 import { chooseProvider, stoppedBy } from './provider.ts';
 import { startProxy } from './proxy.ts';
@@ -72,16 +72,6 @@ function takeCredentials(): Credentials {
   return credentials;
 }
 
-// Writes what the publish step posts, scanned for secrets, and tells the action's later steps there's something to post.
-function writeForPublish(outDir: string, files: Record<string, unknown>, secrets: string[], flag: 'reviewed' | 'answered' | 'taught', outputs: Record<string, string> = {}): void {
-  const texts = Object.entries(files).map(([name, value]) => [name, JSON.stringify(value, null, 2)] as const);
-  for (const [name, text] of texts) assertNoSecrets(text, secrets, `The ${name.replace(/\.json$/, '')}`);
-  mkdirSync(outDir, { recursive: true });
-  for (const [name, text] of texts) writeFileSync(path.join(outDir, name), text);
-  const output = process.env['GITHUB_OUTPUT'];
-  if (output) appendFileSync(output, [`${flag}=true`, ...Object.entries(outputs).map(([key, value]) => `${key}=${value}`)].map((line) => `${line}\n`).join(''));
-}
-
 // `/review teach`: words the guidance issue, reading a finding it names with the read-only token, with no model call,
 // and leaves it for the publish step, which opens it with an issues token. A rule that can't be taught gets a reply.
 async function teach(
@@ -135,21 +125,15 @@ async function answer(
       const run = { provider: choice.provider, proxyUrl: proxy.url, checkout: prPath, ...(fallback ? { fallback } : {}) };
       const result = await explainFinding(run, systemPrompt, { found, question: command.question, headSha });
       if (fallback?.reason) console.log(`Switched to Bedrock partway: ${fallback.reason}`);
+      if (result.error) console.log(`::warning::The explain call failed: ${errorText(result.error)}`);
+      const outcome = !result.output ? 'No answer' : result.output.holds ? 'Holds' : "Doesn't hold";
+      console.log(`${outcome}, $${totalCost(result.records).toFixed(2)}`);
       return result;
     } finally {
       await proxy.close();
     }
   };
-  const { body, explained } = await answerFor(command.ref, resolution, ask);
-  const files: Record<string, unknown> = { [answerFile]: { repository, pullRequest: number, body } satisfies Answer };
-  if (explained) {
-    const { found, result } = explained;
-    if (result.error) console.log(`::warning::The explain call failed: ${errorText(result.error)}`);
-    files['trace.json'] = { explain: { id: found.id, comment: found.comment.html_url ?? found.comment.id, question: command.question, output: result.output, calls: result.records } };
-    const outcome = !result.output ? 'No answer' : result.output.holds ? 'Holds' : "Doesn't hold";
-    console.log(`${outcome}, $${totalCost(result.records).toFixed(2)}`);
-  }
-  writeForPublish(outDir, files, secrets, 'answered');
+  await writeAnswer(outDir, { repository, number, question: command.question, secrets }, command.ref, resolution, ask);
 }
 
 async function main(): Promise<void> {

src/outputs.ts

@@ -1,6 +1,9 @@
 // Everything the run publishes is scanned for secrets first: comments, check run text and the trace artifact are public
 // on public repositories.
 
+import { appendFileSync, mkdirSync, writeFileSync } from 'node:fs';
+import path from 'node:path';
+
 const secretPatterns: RegExp[] = [
   /sk-ant-[A-Za-z0-9_-]{8,}/,
   /\bgh[pousr]_[A-Za-z0-9]{20,}/,
@@ -28,3 +31,13 @@ export function assertNoSecrets(text: string, knownSecrets: Iterable<string>, wh
 export function printable(text: string): string {
   return text.replace(/[\u0000-\u001f\u007f-\u009f\u2028\u2029]/g, '');
 }
+
+// Writes what the publish step posts, scanned for secrets, and tells the action's later steps there's something to post.
+export function writeForPublish(outDir: string, files: Record<string, unknown>, secrets: string[], flag: 'reviewed' | 'answered' | 'taught', outputs: Record<string, string> = {}): void {
+  const texts = Object.entries(files).map(([name, value]) => [name, JSON.stringify(value, null, 2)] as const);
+  for (const [name, text] of texts) assertNoSecrets(text, secrets, `The ${name.replace(/\.json$/, '')}`);
+  mkdirSync(outDir, { recursive: true });
+  for (const [name, text] of texts) writeFileSync(path.join(outDir, name), text);
+  const output = process.env['GITHUB_OUTPUT'];
+  if (output) appendFileSync(output, [`${flag}=true`, ...Object.entries(outputs).map(([key, value]) => `${key}=${value}`)].map((line) => `${line}\n`).join(''));
+}

test/explain.test.ts

@@ -1,5 +1,5 @@
 import assert from 'node:assert/strict';
-import { existsSync, mkdtempSync, writeFileSync } from 'node:fs';
+import { existsSync, mkdtempSync, readdirSync, readFileSync, writeFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import path from 'node:path';
 import { test } from 'node:test';
@@ -19,8 +19,10 @@ import {
   postAnswer,
   publishAnswer,
   unknownRefBody,
+  writeAnswer,
   type Explanation,
   type FoundFinding,
+  type Resolution,
   type ReviewComment,
 } from '../src/explain.ts';
 import { SecretFoundError } from '../src/outputs.ts';
@@ -220,6 +222,85 @@ test('answerFor says the answer could not be finished when the call fails', asyn
   assert.equal((await answerFor({ kind: 'id', id: 'F3' }, { found }, async () => agentResult(undefined))).body, failedBody(found, 'no answer'));
 });
 
+// Runs writeAnswer for the ref F3 into a scratch folder with its own GITHUB_OUTPUT file, restored afterwards, and returns
+// the files it wrote, the outputs it set and the findings it asked the agent about.
+async function answerRun(resolution: Resolution, result: AgentResult<Explanation>): Promise<{ outDir: string; files: string[]; outputs: string; asked: FoundFinding[] }> {
+  const dir = mkdtempSync(path.join(tmpdir(), 'answer-'));
+  const outDir = path.join(dir, 'out');
+  const outputFile = path.join(dir, 'github-output');
+  const asked: FoundFinding[] = [];
+  const saved = process.env['GITHUB_OUTPUT'];
+  process.env['GITHUB_OUTPUT'] = outputFile;
+  try {
+    const where = { repository: 'NCodeGroup/code-review', number: 7, question: 'why?', secrets: ['s'.repeat(20)] };
+    await writeAnswer(outDir, where, { kind: 'id', id: 'F3' }, resolution, async (f) => { asked.push(f); return result; });
+    return { outDir, files: readdirSync(outDir).sort(), outputs: readFileSync(outputFile, 'utf8'), asked };
+  } finally {
+    if (saved === undefined) delete process.env['GITHUB_OUTPUT'];
+    else process.env['GITHUB_OUTPUT'] = saved;
+  }
+}
+
+const readJson = (file: string): unknown => JSON.parse(readFileSync(file, 'utf8'));
+
+test('writeAnswer answers an unknown ref with answer.json alone, without asking the agent', async () => {
+  const run = await answerRun({ validIds: ['F1'] }, agentResult(undefined));
+  assert.deepEqual(run.asked, []);
+  assert.deepEqual(run.files, ['answer.json']);
+  assert.deepEqual(readJson(path.join(run.outDir, 'answer.json')), { repository: 'NCodeGroup/code-review', pullRequest: 7, body: unknownRefBody({ kind: 'id', id: 'F3' }, ['F1']) });
+});
+
+test('writeAnswer writes the trace of a call that ran beside the answer', async () => {
+  const result = agentResult({ answer: 'a', holds: true, reason: 'r' });
+  const run = await answerRun({ found }, result);
+  assert.deepEqual(run.asked, [found]);
+  assert.deepEqual(run.files, ['answer.json', 'trace.json']);
+  assert.deepEqual(readJson(path.join(run.outDir, 'trace.json')), {
+    explain: { id: 'F3', comment: found.comment.html_url ?? found.comment.id, question: 'why?', output: result.output, calls: result.records },
+  });
+});
+
+test('writeAnswer sets the answered output for the publish step', async () => {
+  assert.equal((await answerRun({ validIds: ['F1'] }, agentResult(undefined))).outputs, 'answered=true\n');
+  assert.equal((await answerRun({ found }, agentResult({ answer: 'a', holds: true, reason: 'r' }))).outputs, 'answered=true\n');
+});
+
+test('writeAnswer never writes results.json, so the publish step posts no review', async () => {
+  assert.ok(!(await answerRun({ validIds: ['F1'] }, agentResult(undefined))).files.includes('results.json'));
+  assert.ok(!(await answerRun({ found }, agentResult({ answer: 'a', holds: true, reason: 'r' }))).files.includes('results.json'));
+});
+
+// Runs writeAnswer for a found F3 whose call returns `result`, with the run's secret `secret`, expecting it to refuse
+// with a SecretFoundError whose message matches `where`, and returns whether it wrote a file or set an output anyway.
+async function refusedAnswerRun(result: AgentResult<Explanation>, secret: string, where: RegExp): Promise<{ wrote: boolean; set: boolean }> {
+  const dir = mkdtempSync(path.join(tmpdir(), 'answer-'));
+  const outDir = path.join(dir, 'out');
+  const outputFile = path.join(dir, 'github-output');
+  const saved = process.env['GITHUB_OUTPUT'];
+  process.env['GITHUB_OUTPUT'] = outputFile;
+  try {
+    const run = { repository: 'NCodeGroup/code-review', number: 7, question: 'why?', secrets: [secret] };
+    await assert.rejects(writeAnswer(outDir, run, { kind: 'id', id: 'F3' }, { found }, async () => result), (error: unknown) => error instanceof SecretFoundError && where.test(error.message));
+    return { wrote: existsSync(outDir), set: existsSync(outputFile) };
+  } finally {
+    if (saved === undefined) delete process.env['GITHUB_OUTPUT'];
+    else process.env['GITHUB_OUTPUT'] = saved;
+  }
+}
+
+const runSecret = 's'.repeat(20);
+
+test('writeAnswer throws SecretFoundError and writes nothing when the answer holds a secret', async () => {
+  const result = agentResult({ answer: `The token is ${runSecret}.`, holds: true, reason: 'r' });
+  assert.deepEqual(await refusedAnswerRun(result, runSecret, /^The answer contains one of this run's secrets\.$/), { wrote: false, set: false });
+});
+
+test('writeAnswer throws SecretFoundError and writes nothing when the trace holds a secret', async () => {
+  const clean = agentResult({ answer: 'a', holds: true, reason: 'r' });
+  const result = { ...clean, records: [{ ...clean.records[0]!, model: runSecret }] };
+  assert.deepEqual(await refusedAnswerRun(result, runSecret, /^The trace contains one of this run's secrets\.$/), { wrote: false, set: false });
+});
+
 function response(status: number, body: unknown = {}): Response {
   return new Response(JSON.stringify(body), { status, headers: { 'content-type': 'application/json' } });
 }