Smoke eval baseline gate and writer checks

.github/workflows/smoke-eval.yml

@@ -3,7 +3,8 @@
 #
 # It runs the smoke subset in evals/cases.ts with the production guidance, stops and fails past $8, and compares the run
 # with evals/baseline-smoke.json in the job summary: it fails when caught drops by more than 2 or the must-not case is
-# flagged. With no baseline yet, the run is uploaded as the proposed one and the job passes with a notice.
+# flagged. Until a baseline is first committed, the run is uploaded as the proposed one and the job passes with a notice;
+# once one has been, a run without it fails.
 #
 # It uses the organization's Bedrock key in the code-review environment, which only the default branch can use. Never
 # add a pull_request trigger: the key would then run code from any pull request.
@@ -41,6 +42,8 @@ jobs:
     steps:
       - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
         with:
+          # The full history, so the check can tell a deleted baseline from one never committed.
+          fetch-depth: 0
           persist-credentials: false
       - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0
         with:

CHANGELOG.md

@@ -28,6 +28,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 - The `api-compat` floor places each changed C# line in its member by reading the file's braces, with comments, strings and preprocessor lines left out, instead of its indentation, so unusual formatting no longer misplaces a change; a file whose braces don't balance counts as a public change ([#112](https://github.com/NCodeGroup/code-review/issues/112)).
 - A re-review posts a problem `correctness` and `security` both report, in the same file within three lines and with the same title words, once, at the higher severity and naming both reviewers, instead of two threads; its log counts these merges and the findings of the two that sit that close without matching, for the evals to measure ([#116](https://github.com/NCodeGroup/code-review/issues/116)).
 - The evals have a re-review case whose changes touch a file the selection floor gives to `security` and one whose changes touch a `sensitive:` path, which a case can now set for a repository that has none; `--subset re-review` runs the four re-review cases, whose output has each one's cost ([#117](https://github.com/NCodeGroup/code-review/issues/117)).
+- The smoke eval fails when `evals/baseline-smoke.json` is missing after it has been committed, naming the commit that deleted it, so deleting the baseline can't turn the gate off; until the first baseline is committed it still passes with a notice. Its workflow checks out the full history for this ([#93](https://github.com/NCodeGroup/code-review/issues/93)).
+- The tests check that the smoke eval workflow uploads its outputs only after the secret scan passes and fails the job when the run or the check fails, and that `evals/run.ts` writes its output before it fails a run over `--max-cost` ([#93](https://github.com/NCodeGroup/code-review/issues/93)).
+- A private organization member's comment counts as the maintainer's for the loop limit, their open issues can track a finding, and the bot answers their `Autopilot:` replies: these use the engine's writer check, the author's association or else their permission on the repository, and a lookup that fails counts as no write access. The README says how such a member can use `/review`, whose gate still reads only the association ([#89](https://github.com/NCodeGroup/code-review/issues/89)).
 
 ### Fixed
 

README.md

@@ -42,8 +42,11 @@ The reviewers see a pull request's description, its commit subjects and the issu
 their authors can write to the repository. The engine asks the repository's permissions when the event doesn't say so:
 GitHub reports an organization member whose membership is private as a contributor. The prompt says whether the pull
 request has a description, and when one was left out, so no reviewer reports a description it wasn't shown as missing.
-A `/review` comment still counts only when its `author_association` names a writer, as the workflow's `if:` below
-checks before the engine starts, so a private organization member can't start a review that way.
+The loop limit's count of maintainer comments, the open issues a finding can be tracked in and the `Autopilot:` replies
+the bot answers use the same check. A `/review` comment still counts only when its `author_association` names a writer,
+as the workflow's `if:` below checks before the engine starts: a permission lookup there would let more people start a
+run that holds the model credentials. So a private organization member can't start a review that way;
+[Commands](#commands) says what they can do.
 
 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.
 
@@ -194,6 +197,9 @@ the source above but hasn't been tried on a consumer yet.
 ## Commands
 
 Comment on a pull request, as a top-level comment, not a reply in a review thread. Only people with write access can.
+GitHub reports an organization member whose membership is private as a contributor, so their commands are ignored.
+To use them, change your membership from Private to Public on the organization's People page, or have an admin add
+you to the repository as a collaborator.
 
 | Comment | What happens |
 |---|---|
@@ -346,7 +352,9 @@ After three reviews with no comment from someone with write access in between, c
 automatic review stops before any model call and posts once "Paused: comment `/review` to resume". An acknowledged
 "Code review" check run is completed as "Review paused", so it never stays in progress. Comments the
 autopilot posts as the maintainer's account, which start with `Autopilot:`, don't count as the maintainer's. A
-`/review` from someone with write access always reviews, and counts as their comment.
+`/review` from someone with write access always reviews, and counts as their comment. A comment counts as from someone
+with write access by its author's association or, when that doesn't say so, their permission on the repository; one
+whose permission can't be read doesn't count.
 
 ### Re-reviews
 
@@ -381,8 +389,9 @@ of review converge:
   says so instead, and its verdict is never Approve;
 - it closes the loop on the bot's own threads, never anyone else's: it resolves the thread of an earlier finding it
   found fixed when the thread is outdated or its lines changed since the last review. When it finds one still open
-  and the thread's last comment is an `Autopilot:` reply, it replies once to say why it stays open, ending with a line
-  that asks the maintainer (`@<them>: needs you.`), and never a second time in that thread.
+  and the thread's last comment is an `Autopilot:` reply from someone with write access, it replies once to say why it
+  stays open, ending with a line that asks the maintainer (`@<them>: needs you.`), and never a second time in that
+  thread.
 
 A `/review` of the commit already reviewed, or a last reviewed head that can't be fetched any more (after a force-push),
 gets a full review instead. A review whose reviewers all failed doesn't count as the last review, nor does a re-review
@@ -508,7 +517,7 @@ Each case's pull request is built as the review step builds it: `author` and `au
 
 **Capping the cost.** `--max-cost <usd>` stops a run once its cost, judges included, passes that many dollars: the case that crossed it finishes, the rest don't start, the output covers the cases that ran, and the run fails. `--subset <name>` runs a named set of cases from `evals/cases.ts` instead of `--only id,id`; a name that matches no case fails the run.
 
-**Smoke eval.** The [Smoke eval](.github/workflows/smoke-eval.yml) workflow runs on every push to `main` that changes `prompts/`, `reviewers/`, `src/config.ts` or `guidance/`, and by hand (`gh workflow run smoke-eval.yml`). It runs in the `code-review` environment with the organization's Bedrock key, so it never runs on a pull request. It runs the `smoke` subset, 4 cases with the must-not case and one large diff, with the production guidance, `--precision off` and `--max-cost 8`. `node evals/smoke.ts check` then compares the run with `evals/baseline-smoke.json`, posts the comparison to the job summary, and fails when caught drops by more than 2, when the must-not case is flagged, or when the two cover different cases. `evals/out` is scanned for secrets before it's uploaded as the `smoke-eval` artifact, since artifacts on a public repository are public. With no baseline yet, the run passes with a notice, failing only on a flagged must-not case. To adopt a run as the baseline, the first time or after an intended change in what the smoke cases catch or a change to the subset, take `proposed-baseline-smoke.json` from that run's artifact, which is the run without its agent calls and raw findings, and commit it in a pull request:
+**Smoke eval.** The [Smoke eval](.github/workflows/smoke-eval.yml) workflow runs on every push to `main` that changes `prompts/`, `reviewers/`, `src/config.ts` or `guidance/`, and by hand (`gh workflow run smoke-eval.yml`). It runs in the `code-review` environment with the organization's Bedrock key, so it never runs on a pull request. It runs the `smoke` subset, 4 cases with the must-not case and one large diff, with the production guidance, `--precision off` and `--max-cost 8`. `node evals/smoke.ts check` then compares the run with `evals/baseline-smoke.json`, posts the comparison to the job summary, and fails when caught drops by more than 2, when the must-not case is flagged, or when the two cover different cases. `evals/out` is scanned for secrets before it's uploaded as the `smoke-eval` artifact, since artifacts on a public repository are public. Until `evals/baseline-smoke.json` is first committed, the run passes with a notice, failing only on a flagged must-not case. Once it has been, a run without it fails and names the commit that deleted it, so deleting the file can't turn the gate off; the workflow checks out the full history so the check can tell, and the check fails on a shallow checkout. To adopt a run as the baseline, the first time or after an intended change in what the smoke cases catch or a change to the subset, take `proposed-baseline-smoke.json` from that run's artifact, which is the run without its agent calls and raw findings, and commit it in a pull request:
 
 ```powershell
 gh run download <run id> -n smoke-eval -D evals/out/smoke

evals/run.ts

@@ -264,6 +264,16 @@ export function tallyCase(
 // How the output names a finding that raised a concern.
 const raisedLabel = (finding: Finding & { reviewers?: string[] }) => `${finding.file}:${finding.line} (${(finding.reviewers ?? []).join(', ')})`;
 
+// Writes the run's output to <dir>/<label>.json, then fails a run that passed --max-cost, so the smoke eval still checks
+// and uploads the cases that ran.
+export function writeRunOutput(dir: string, label: string, output: { summary: unknown; cases: unknown[] }, capError: string | undefined): string {
+  mkdirSync(dir, { recursive: true });
+  const file = path.join(dir, `${label}.json`);
+  writeFileSync(file, JSON.stringify(output, null, 2));
+  if (capError) throw new Error(capError);
+  return file;
+}
+
 async function main(): Promise<void> {
   const maxCost = maxCostFrom(process.argv);
   const precisionOn = parsePrecision(arg('precision'));
@@ -425,9 +435,7 @@ async function main(): Promise<void> {
   if (precision) console.log(`Precision ${percent(precision.precision)}: ${precision.matched} matched + ${precision.real} real of ${precision.posted} blockers and should-fixes; ${precision.unsure} unsure, ${precision.notReal} not real, ${precision.mustNot} must-not`);
   console.log(`Findings per case: mean ${summary.findingsPerCase.mean.toFixed(1)}, max ${summary.findingsPerCase.max}; review cost per case: mean $${summary.reviewCostPerCase.mean.toFixed(2)}, max $${summary.reviewCostPerCase.max.toFixed(2)}`);
   if (shadowNames.size) console.log(`Shadow reviewers (${[...shadowNames].join(', ')}), not posted: caught ${caughtShadow}/${expected}; must-not violations ${shadowViolations}; ${shadowFindingCount} findings`);
-  mkdirSync(path.join(root, 'evals', 'out'), { recursive: true });
-  writeFileSync(path.join(root, 'evals', 'out', `${label}.json`), JSON.stringify({ summary, cases: report }, null, 2));
-  if (cap.error) throw new Error(cap.error);
+  writeRunOutput(path.join(root, 'evals', 'out'), label, { summary, cases: report }, cap.error);
 }
 
 if (process.argv[1] && fileURLToPath(import.meta.url) === path.resolve(process.argv[1])) {

evals/smoke.ts

@@ -3,13 +3,15 @@
 //   node evals/smoke.ts check <run.json> [--baseline <file>]
 //     Compares the run with evals/baseline-smoke.json in eval:compare's table and fails when caught drops by more than
 //     2, when a must-not-flag concern was raised, or when the two cover different cases. With no baseline yet it fails
-//     only on a must-not-flag concern, with a notice. Writes the table to $GITHUB_STEP_SUMMARY (stdout outside
-//     Actions), and the run, trimmed to what compare reads, to proposed-baseline-smoke.json beside it.
+//     only on a must-not-flag concern, with a notice. Once one was committed, a missing one fails, naming the commit
+//     that deleted it. Writes the table to $GITHUB_STEP_SUMMARY (stdout outside Actions), and the run, trimmed to what
+//     compare reads, to proposed-baseline-smoke.json beside it.
 //   node evals/smoke.ts scan <dir>
 //     Fails when a file under <dir> holds one of the job's secrets or anything shaped like a credential, before the
 //     workflow uploads the folder: artifacts on a public repository are public.
 
-import { appendFileSync, readdirSync, readFileSync, writeFileSync } from 'node:fs';
+import { execFileSync } from 'node:child_process';
+import { appendFileSync, existsSync, readdirSync, readFileSync, writeFileSync } from 'node:fs';
 import path from 'node:path';
 import { fileURLToPath } from 'node:url';
 import { assertNoSecrets } from '../src/outputs.ts';
@@ -40,7 +42,7 @@ function runTable(run: EvalOutput): string {
 // Model output reaches the summary only inside a code block, which a run of backticks could otherwise close.
 const fenced = (text: string) => `\`\`\`text\n${text.replace(/`{3,}/g, (ticks) => "'".repeat(ticks.length)).trimEnd()}\n\`\`\`\n`;
 
-export function smokeCheck(run: EvalOutput, baseline: EvalOutput | undefined): SmokeVerdict {
+export function smokeCheck(run: EvalOutput, baseline: EvalOutput | undefined, deletedIn?: string): SmokeVerdict {
   const failures: string[] = [];
   const notices: string[] = [];
   for (const c of run.cases) {
@@ -56,6 +58,8 @@ export function smokeCheck(run: EvalOutput, baseline: EvalOutput | undefined): S
       const drop = baseline.summary.caught - run.summary.caught;
       if (drop > noiseLimit) failures.push(`Caught dropped by ${drop}, from ${baseline.summary.caught}/${baseline.summary.expected} to ${run.summary.caught}/${run.summary.expected}: more than the ${noiseLimit} a run varies by.`);
     }
+  } else if (deletedIn) {
+    failures.push(`${baselineFile} is missing, but commit ${deletedIn} deleted it: restore it, or adopt a new baseline from this run's ${proposedName} in a pull request.`);
   } else {
     notices.push(`There is no ${baselineFile} yet, so caught wasn't compared. This run is the proposed baseline: ${proposedName} in the smoke-eval artifact. The README's Smoke eval section says how to adopt it.`);
   }
@@ -121,20 +125,31 @@ export function baselinePathFrom(argv: readonly string[], root: string): string
 }
 
 // Reads the run and the baseline, a missing one as none, checks them, and writes the proposed baseline beside the run.
-export function checkFiles(runFile: string, baselinePath: string): SmokeVerdict {
+export function checkFiles(runFile: string, baselinePath: string, deletedIn?: string): SmokeVerdict {
   const run = readOutput(runFile);
-  const verdict = smokeCheck(run, readBaseline(baselinePath));
+  const verdict = smokeCheck(run, readBaseline(baselinePath), deletedIn);
   writeFileSync(path.join(path.dirname(runFile), proposedName), `${JSON.stringify(trimmed(run), null, 2)}\n`);
   return verdict;
 }
 
+// The last commit that changed baselineFile in root's history, or undefined when none did. A shallow checkout can't
+// tell, so it throws rather than pass a deleted baseline.
+export function lastBaselineCommit(root: string): string | undefined {
+  const git = (...args: string[]) => execFileSync('git', ['-C', root, ...args], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim();
+  if (git('rev-parse', '--is-shallow-repository') === 'true') throw new Error(`The checkout is shallow, so it can't tell whether ${baselineFile} was deleted; check out with fetch-depth: 0.`);
+  return git('log', '-1', '--format=%h', '--', baselineFile) || undefined;
+}
+
 function main(): void {
   const [command, target] = process.argv.slice(2);
   const annotate = (kind: 'notice' | 'error', message: string) =>
     console.log(process.env['GITHUB_ACTIONS'] === 'true' ? `::${kind} title=Smoke eval::${commandData(message)}` : `${kind}: ${message}`);
   if (command === 'check' && target) {
     const root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..');
-    const verdict = checkFiles(path.resolve(target), baselinePathFrom(process.argv, root));
+    const baselinePath = baselinePathFrom(process.argv, root);
+    // Only the committed baseline has a history; a --baseline file is the caller's.
+    const deletedIn = baselinePath === path.join(root, baselineFile) && !existsSync(baselinePath) ? lastBaselineCommit(root) : undefined;
+    const verdict = checkFiles(path.resolve(target), baselinePath, deletedIn);
     const summaryFile = process.env['GITHUB_STEP_SUMMARY'];
     if (summaryFile) appendFileSync(summaryFile, verdict.markdown);
     else process.stdout.write(verdict.markdown);