feat(review): require an executed witness on every confirmed Critical

packages/cli/src/commands/review/agent-prompt.test.ts

@@ -493,6 +493,13 @@ describe('agent-prompt (command boundary)', () => {
       // The verdict branch: Exclusion Criteria yes, finding format no.
       expect(briefText).toContain('What is NOT a finding');
       expect(briefText).not.toContain('**Anchor:**');
+      // The witness rule: a confirmed Critical returns its executed evidence
+      // or the one-line reason, and the sweep is a named witness form. These
+      // demands are what the orchestrator's low-confidence demotion sorts on,
+      // so a brief that drops them silently demotes every trace-only Critical.
+      expect(briefText).toContain('A confirmed Critical returns its witness.');
+      expect(briefText).toContain('witness: not run —');
+      expect(briefText).toContain('sweep the real population');
     } finally {
       rmSync(dir, { recursive: true, force: true });
     }

packages/cli/src/commands/review/findings.test.ts

@@ -24,6 +24,7 @@ import {
   type Finding,
   type FindingsReport,
   holdCriticalsFailingOnBase,
+  holdUnwitnessedCriticals,
   sharedFailingFilesOf,
 } from './findings.js';
 
@@ -511,6 +512,30 @@ describe('findings (command boundary)', () => {
     return out;
   }
 
+  it('demotes an unwitnessed Critical through the whole handler, and says so on stderr', () => {
+    // The unit tests pin holdUnwitnessedCriticals in isolation; this pins the
+    // WIRING — the call sits in the handler before buildReport, so removing
+    // it, or moving it after the report is built, fails here, not silently.
+    const input = join(dir, 'in.json');
+    const out = join(dir, 'findings.json');
+    writeFileSync(
+      input,
+      JSON.stringify([
+        { ...base, id: 'w1' },
+        { ...base, id: 'w2', witness: 'probe flipped: 2 calls → 1' },
+      ]),
+    );
+    const stderr = runCapturingStderr({ input, out, print: false });
+    const report = JSON.parse(readFileSync(out, 'utf8')) as FindingsReport;
+    const byId = new Map(report.findings.map((f) => [f.id, f]));
+    expect(byId.get('w1')?.confidence).toBe('low');
+    expect(byId.get('w1')?.failureScenario).toContain('witness rule');
+    expect(byId.get('w2')?.confidence).toBe('high');
+    expect(stderr).toContain('w1 filed at low confidence');
+    expect(stderr).not.toContain('w2 filed at low confidence');
+    expect(report.counts.byConfidence['low']).toBe(1);
+  });
+
   it('announces every hold, naming the finding and the measured file', () => {
     // A severity this command lowered is a change to what the review says. Left
     // unannounced it reads as the reviewer's own judgement, which is the one
@@ -790,6 +815,68 @@ describe('findings (command boundary)', () => {
   });
 });
 
+describe('holdUnwitnessedCriticals — the witness rule has a machine half', () => {
+  const critical = {
+    id: 'w1',
+    severity: 'Critical' as const,
+    confidence: 'high' as const,
+    source: 'review' as const,
+    summary: 'double-executes the shell command',
+    shortSummary: 'double execute',
+    failureScenario: 'run !git push → sendShellCommand fires twice',
+    locations: [{ file: 'src/pay.ts', line: 42 }],
+  };
+
+  it('files an unwitnessed high-confidence review Critical at low confidence, and says why', () => {
+    // The demotion the SKILL promises as mechanical: without this, the sort
+    // exists only as Step 4 prose, and an omitted `confidence` even defaults
+    // to `high` — the fail-open direction (dogfood review of the witness PR).
+    const { findings, unwitnessed } = holdUnwitnessedCriticals([critical]);
+    expect(findings[0].confidence).toBe('low');
+    expect(findings[0].severity).toBe('Critical');
+    expect(findings[0].failureScenario).toContain('witness rule');
+    // The original evidence survives — the rule is appended, not substituted.
+    expect(findings[0].failureScenario).toContain('fires twice');
+    expect(unwitnessed).toEqual(['w1']);
+  });
+
+  it('leaves a witnessed Critical alone — either form of the field counts', () => {
+    for (const witness of [
+      'BASE: 2 calls / PR: 1 call — probe flipped',
+      'not run — needs a live OAuth endpoint this harness lacks',
+    ]) {
+      const { findings, unwitnessed } = holdUnwitnessedCriticals([
+        { ...critical, witness },
+      ]);
+      expect(findings[0].confidence).toBe('high');
+      expect(unwitnessed).toEqual([]);
+    }
+  });
+
+  it('exempts deterministic sources — their witness is constitutive', () => {
+    // A [build]/[test]/[probe] finding IS a run's output; demanding a second
+    // witness would demote findings the pipeline treats as pre-confirmed.
+    for (const source of ['build', 'test', 'probe', 'lint'] as const) {
+      const { unwitnessed } = holdUnwitnessedCriticals([
+        { ...critical, source },
+      ]);
+      expect(unwitnessed).toEqual([]);
+    }
+  });
+
+  it('is idempotent — a demoted finding re-fed is not touched again', () => {
+    const once = holdUnwitnessedCriticals([critical]).findings[0];
+    const twice = holdUnwitnessedCriticals([once]).findings[0];
+    expect(twice).toEqual(once);
+    // Suggestions are never judged: the rule targets the severity that posts
+    // as a blocker.
+    expect(
+      holdUnwitnessedCriticals([{ ...critical, severity: 'Suggestion' }])
+        .unwitnessed,
+    ).toEqual([]);
+  });
+});
+
 describe('holdCriticalsFailingOnBase', () => {
   // The shape test-delta writes: workspace-relative paths, while a finding
   // names the repo-relative one.
@@ -1214,4 +1301,15 @@ describe('validateFindings — the canonical artifact round-trips', () => {
     expect(f.outcome).toBeUndefined();
     expect(f.outcomeNote).toBeUndefined();
   });
+
+  it('keeps witness, so the executed evidence survives being fed back', () => {
+    // The Step 4 witness rule attaches the evidence once; the report and the
+    // comment bodies read it back out of the artifact. Dropped here, every
+    // downstream quote becomes a fresh transcription.
+    const [f] = validateFindings([
+      { ...base, witness: 'BASE: 2 calls / PR: 1 call — probe flipped' },
+    ]);
+    expect(f.witness).toBe('BASE: 2 calls / PR: 1 call — probe flipped');
+    expect(validateFindings([{ ...base }])[0].witness).toBeUndefined();
+  });
 });

packages/cli/src/commands/review/findings.ts

@@ -83,6 +83,13 @@ export interface Finding {
   shortSummary: string;
   /** The concrete trigger and wrong outcome — the finding's evidence. */
   failureScenario: string;
+  /**
+   * The executed evidence that settled the verdict (a probe's two sides, an
+   * A/B's quoted pair, a sweep count) — or the verifier's
+   * `not run — <reason>` line. Carried as data so the report and the comment
+   * bodies quote one recorded string instead of transcribing it twice more.
+   */
+  witness?: string;
   suggestedFix?: string;
   /** Free-form kebab-case tag (`correctness`, `security`, `test-coverage`, …). */
   category?: string;
@@ -351,6 +358,11 @@ export function validateFindings(raw: unknown): Finding[] {
     const shortSummary =
       asString(o, 'shortSummary') ?? asString(o, 'short_summary');
 
+    // `witness` round-trips for the same reason `outcomeNote` does: the Step 4
+    // witness rule attaches it once, and the report and the comment bodies read
+    // it back out of the artifact instead of transcribing the evidence again.
+    const witness = asString(o, 'witness');
+
     return {
       id,
       severity,
@@ -361,6 +373,7 @@ export function validateFindings(raw: unknown): Finding[] {
         ? compressSummary(shortSummary)
         : compressSummary(summary),
       failureScenario,
+      ...(witness ? { witness } : {}),
       ...(asString(o, 'suggestedFix') || asString(o, 'suggested_fix')
         ? {
             suggestedFix: (asString(o, 'suggestedFix') ??
@@ -507,6 +520,45 @@ export function holdCriticalsFailingOnBase(
   return { findings: out, held, readjudicated };
 }
 
+/**
+ * The witness rule's machine half. Step 4 demands that a confirmed Critical
+ * carry its executed evidence — the `witness` field, holding either the
+ * observed output or the verifier's `not run — <reason>` line — and promises
+ * the demotion is mechanical. This is the mechanism, in the same place the
+ * test-delta holdback lives: a high-confidence Critical from the one
+ * non-deterministic source that arrives with no witness is filed at low
+ * confidence — terminal-only, never posted. Only `source: 'review'` is
+ * judged: a `[build]`/`[test]`/`[lint]`/`[probe]` finding IS a run's output,
+ * so its witness is constitutive, not an attachment. Nothing is deleted and
+ * nothing is raised; the appended sentence names the rule that moved it and
+ * the way back (attach the witness, or say why none could run). Idempotent by
+ * construction — a demoted finding re-fed through `--input` is already low
+ * confidence and is not touched again.
+ */
+export function holdUnwitnessedCriticals(findings: readonly Finding[]): {
+  findings: Finding[];
+  unwitnessed: string[];
+} {
+  const unwitnessed: string[] = [];
+  const out = findings.map((f) => {
+    if (
+      f.severity !== 'Critical' ||
+      f.confidence !== 'high' ||
+      f.source !== 'review' ||
+      f.witness !== undefined
+    ) {
+      return f;
+    }
+    unwitnessed.push(f.id);
+    return {
+      ...f,
+      confidence: 'low' as Confidence,
+      failureScenario: `${f.failureScenario}\n\nFiled at low confidence by the witness rule: this confirmed Critical arrived with neither a witness (the executed evidence that settled the verdict) nor a \`not run — <reason>\` line. Attach either and it stands at high confidence again.`,
+    };
+  });
+  return { findings: out, unwitnessed };
+}
+
 const WORKSPACE_IN_COMMAND_RE = /--workspace="([^"]+)"/;
 
 /**
@@ -888,6 +940,8 @@ export const findingsCommand: CommandModule = {
         shared,
       ));
     }
+    const witnessHold = holdUnwitnessedCriticals(findings);
+    findings = witnessHold.findings;
     const report = buildReport(findings);
 
     const target = resolve(out);
@@ -909,6 +963,14 @@ export const findingsCommand: CommandModule = {
         `findings: ${h.id} held back from Critical — test-delta measured ${h.file} as failing on the merge base too`,
       );
     }
+    // The witness rule's demotions get the same disclosure: a confidence this
+    // command lowered must name the finding and the rule, or the demotion
+    // reads as the reviewer's own judgement.
+    for (const id of witnessHold.unwitnessed) {
+      writeStderrLine(
+        `findings: ${id} filed at low confidence — a confirmed Critical carried neither a witness nor a 'not run' reason (Step 4's witness rule)`,
+      );
+    }
     // A hold that was weighed and reversed is a decision, and a decision this
     // command declined to overrule is exactly as reportable as one it made.
     for (const r of readjudicated) {

packages/cli/src/commands/review/lib/agent-briefs.ts

@@ -636,6 +636,8 @@ For each finding you were given:
 
 **When the fix IS a threshold, measure the threshold.** A guard built on a ratio or length cutoff makes the fix's coverage an empirical number, not a reading: hold every other variable fixed, vary the guarded quantity, and binary-search the boundary where behaviour flips. Then put that number next to what the linked issue actually reports — a live verification of a prose-ratio guard measured the minimum recovering payload at ~473 chars with the issue's own preamble held fixed, which proved the fix covered the issue's \`edit\`/\`write_file\` half and silently declined its \`run_shell_command\` half. "Fix is narrower than its claim, here is the boundary, here is the half it misses" is a finding no amount of code-reading produces.
 
+**When the defect is mechanically enumerable, sweep the real population — the count is the verdict.** For a claim about a pattern, a predicate, or a parser ("this misclassifies X", "this mishandles shape Y"), do not stop at the one reported instance: run the check over every real instance this repo holds (every workflow step body, every call site, every input the code will actually see) and report the count. "195 of 434 real \`run:\` bodies reach this path" confirms the finding, sizes its severity, and hands the author a number they can re-run rather than argue with — and a count of **zero** is the quoted contradiction that rejects it. Two rules keep a sweep evidence rather than theatre: its oracle must be an **external authority** — the real parser, the real tool, \`bash -n\` — never your own reimplementation of the logic under test, because a mirror shares the blind spots of what it mirrors and mirrored sweeps have manufactured false findings out of their own bugs; and spot-check one hit by reading it before you quote a nonzero count.
+
 **A suggested fix you did not run is a hypothesis; say which one you are giving.** When a finding's fix is cheap to apply, patch it in, re-run the same probe/harness to show it works, then revert — and state that every other number in your report comes from the unmodified PR (the contamination line is what lets a reader trust the rest). A fix too costly to verify is still worth proposing, labeled untested.
 
 **A probabilistic failure gets a RATE, not an anecdote.** For a timing/race claim, run N repetitions per arm and report the rates as the verdict; amplify with full CPU load to force the window open (a live case went from 4/11 idle to 5/5 loaded). And attribute honestly: a lower idle rate with no structural change is luck, not a fix. Fake-timer tests hardcode one ordering by construction — they cannot discriminate a race, so a green fake-timer suite is non-evidence here.
@@ -696,6 +698,8 @@ Return, for each finding, one verdict:
 - **confirmed (low confidence)** — the mechanism is real but the trigger is uncertain (timing, environment, configuration). Say what would confirm it. Carry the severity.
 - **rejected** — the code does not do what the finding claims (**quote the contradicting code**), or it matches an Exclusion Criterion (one-line reason).
 
+**A confirmed Critical returns its witness.** Alongside the verdict, include a \`witness:\` line quoting the observed output that settled it — the probe's two sides, the A/B's \`BASE:\`/\`PR:\` pair, the extracted step's run, the sweep count — trimmed to the deciding lines. When every run-capability above is genuinely inapplicable and the confirmation rests on the trace alone, write the one line \`witness: not run — <why no run could settle this claim>\` instead; writing that line is also the moment you notice when the claim was runnable after all. This is mechanical downstream — enforced in code at the findings canonicalization, not merely by the orchestrator's read of its rules: a confirmed Critical returning neither the witness nor the reason line is filed at **low confidence** — terminal-only, never posted — whatever your prose argued, because the evidence a run produced is the one part of a Critical its author can act on without re-deriving the bug.
+
 **Rejecting a Critical carries a higher bar than anything else, and it is one-way.** A rejected Critical is gone — no later stage revisits it, it vanishes from both the pull request and the terminal. To reject one you must **quote the specific code that contradicts the claim**. A passing test, a plausible-looking guard, or "I could not reproduce the reasoning" is not enough — when you cannot quote the contradiction, the floor is \`confirmed (low confidence)\`, never rejection. Downgrading is reversible; a human still sees a low-confidence finding under "Needs Human Review". Rejection is not.
 
 **For anything non-Critical, when uncertain, downgrade to low confidence rather than rejecting.** Reserve outright rejection for a finding that clearly does not match the code (it describes behaviour the code does not have) or matches an Exclusion Criterion. Low confidence is for "likely real, needs human judgement", not for "I have no idea" — a vague suspicion with no concrete evidence in the code can still be rejected.