feat(review): prove Step 4 (verify) and Step 5 (reverse audit) actually ran

packages/cli/src/commands/review/check-coverage.test.ts

@@ -29,6 +29,7 @@ import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import {
   coverageFromTranscripts,
+  verificationGaps,
   TranscriptsUnavailableError,
 } from './lib/coverage.js';
 import { promptRecordDir, briefPath } from './lib/prompt-record.js';
@@ -775,6 +776,22 @@ describe('the roster — who should have been here', () => {
     expect(r.ok).toBe(false);
   });
 
+  it('does not credit a brief opened as a `.bak` sibling', () => {
+    // The brief-open check matches the whole quoted path, not a bare substring, so
+    // an agent that opened `<brief>.bak` — a real path with the brief as a strict
+    // prefix — is not credited with opening the brief. A bare `includes(brief)`
+    // would have counted it and cleared the gap.
+    const p = plan3a();
+    const brief = briefPath(p, '2'); // Agent 2 (Security), a roster whole-diff role
+    const prompt = readFileSync(join(promptRecordDir(p), '2.txt'), 'utf8');
+    // Relaunch it opening the `.bak` sibling instead of the brief itself.
+    transcript('r-2', prompt, { calls: 2, opens: [`${brief}.bak`] });
+
+    const r = coverageFromTranscripts(p, ENV);
+    expect(r.unreadBriefs.some((s) => s.includes('Security'))).toBe(true);
+    expect(r.ok).toBe(false);
+  });
+
   it('does not demand a build-and-test agent from a diff with no tree to build', () => {
     // A cross-repo lightweight review has the diff and nothing else. Requiring
     // Agent 7 or the cross-file tracer of it would fail every such review for not
@@ -931,3 +948,113 @@ describe('an agent that paged its chunk still read it', () => {
     expect(r.missingChunks).toEqual([]);
   });
 });
+
+describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', () => {
+  // A Step 4/5 agent as a real run leaves it: the CLI's record of the prompt it
+  // built (`agent-prompt --role <role>`), the brief that prompt points at, and the
+  // harness's transcript of an agent launched with it. `launch: false` models a
+  // prompt built but never handed to an agent; `opensBrief: false` an agent that
+  // ran but never opened the brief. To model a step skipped wholesale, do not set
+  // the key up at all — there is then no record and no transcript.
+  function step45(
+    planPath: string,
+    key: string,
+    opts: { launch?: boolean; opensBrief?: boolean } = {},
+  ): void {
+    const d = promptRecordDir(planPath);
+    mkdirSync(d, { recursive: true });
+    const brief = briefPath(planPath, key);
+    writeFileSync(brief, `The ${key} brief.`);
+    const prompt =
+      `You are review agent \`${key}\`.\n` +
+      `read_file(file_path="${brief}")\n` +
+      `read_file(file_path="${DIFF}")`;
+    writeFileSync(join(d, `${encodeURIComponent(key)}.txt`), prompt);
+    if (opts.launch === false) return;
+    transcript(`v-${key.replace(/[^a-z0-9]/gi, '_')}`, prompt, {
+      calls: 2,
+      opens: opts.opensBrief === false ? [] : [brief],
+    });
+  }
+
+  it('passes when the reverse audit ran on a review with nothing to verify', () => {
+    const p = plan();
+    step45(p, 'reverse-audit');
+    const r = verificationGaps(p, { postsFindings: false }, ENV);
+    expect(r.ok).toBe(true);
+    expect(r.gaps).toEqual([]);
+  });
+
+  it('passes when both verify and reverse audit ran on a review with findings', () => {
+    const p = plan();
+    step45(p, 'reverse-audit');
+    step45(p, 'verify');
+    expect(verificationGaps(p, { postsFindings: true }, ENV).ok).toBe(true);
+  });
+
+  it('flags a review that never ran the reverse audit', () => {
+    const p = plan(); // no reverse-audit fixture: the step was skipped
+    const r = verificationGaps(p, { postsFindings: false }, ENV);
+    expect(r.ok).toBe(false);
+    expect(r.gaps.join(' ')).toMatch(/reverse audit — no auditor ran/);
+  });
+
+  it('flags a reverse audit built but whose agent never opened its brief', () => {
+    const p = plan();
+    step45(p, 'reverse-audit', { opensBrief: false });
+    const r = verificationGaps(p, { postsFindings: false }, ENV);
+    expect(r.ok).toBe(false);
+    expect(r.gaps.join(' ')).toMatch(/reverse audit — its prompt was built/);
+  });
+
+  it('flags a reverse audit whose prompt was built but never launched', () => {
+    const p = plan();
+    step45(p, 'reverse-audit', { launch: false });
+    const r = verificationGaps(p, { postsFindings: false }, ENV);
+    expect(r.ok).toBe(false);
+    expect(r.gaps.join(' ')).toMatch(/reverse audit — its prompt was built/);
+  });
+
+  it('counts a Step 3B per-chunk reverse auditor (reverse-audit--chunk-N)', () => {
+    const p = plan();
+    step45(p, 'reverse-audit--chunk-1');
+    const r = verificationGaps(p, { postsFindings: false }, ENV);
+    expect(r.gaps.join(' ')).not.toMatch(/reverse audit/);
+  });
+
+  it('requires a verifier when the review posts findings', () => {
+    const p = plan();
+    step45(p, 'reverse-audit'); // isolate the verify gap
+    const r = verificationGaps(p, { postsFindings: true }, ENV);
+    expect(r.ok).toBe(false);
+    expect(r.gaps.join(' ')).toMatch(
+      /verification — the review posts findings/,
+    );
+  });
+
+  it('does not require a verifier when the review confirmed nothing', () => {
+    const p = plan();
+    step45(p, 'reverse-audit');
+    const r = verificationGaps(p, { postsFindings: false }, ENV);
+    expect(r.gaps.join(' ')).not.toMatch(/verification/);
+  });
+
+  it('flags a verifier built but whose agent never opened its brief', () => {
+    const p = plan();
+    step45(p, 'reverse-audit');
+    step45(p, 'verify', { opensBrief: false });
+    const r = verificationGaps(p, { postsFindings: true }, ENV);
+    expect(r.gaps.join(' ')).toMatch(/verification — its prompt was built/);
+  });
+
+  it('flags a verifier whose prompt was built but never launched', () => {
+    // The other half of `ranAndReadBrief`: `built.get('verify')` returns content,
+    // but no transcript matches it. Same gap message as opensBrief:false, but it
+    // fails at the transcript-matching term, not the brief-open one.
+    const p = plan();
+    step45(p, 'reverse-audit');
+    step45(p, 'verify', { launch: false });
+    const r = verificationGaps(p, { postsFindings: true }, ENV);
+    expect(r.gaps.join(' ')).toMatch(/verification — its prompt was built/);
+  });
+});

packages/cli/src/commands/review/compose-review.test.ts

@@ -57,7 +57,7 @@ const DIFF = '/abs/diff.txt';
  * satisfies that one. A plan that requires nothing is not a plan any capture
  * command writes, and coverage now reads the roster out of it.
  */
-function plan(): string {
+function plan(opts: { step45?: boolean } = {}): string {
   const p = join(dir, 'plan.json');
   writeFileSync(
     p,
@@ -72,6 +72,11 @@ function plan(): string {
       ],
     }),
   );
+  // Every high-effort review runs Step 4 (verify) and Step 5 (reverse audit), and
+  // `composeReview` now proves they did — so a fixture meaning "a review that did
+  // everything right" includes them, exactly as it includes the roster. Pass
+  // `{ step45: false }` for a run that skipped one or both (the gap tests).
+  if (opts.step45 !== false) recordStep45(p);
   // Backdate it. The transcripts are written first and the stale-transcript
   // filter is `mtime < planMtime`; on a filesystem with millisecond granularity
   // both land in the same tick and the comparison flips at random. An explicit
@@ -81,6 +86,38 @@ function plan(): string {
   return p;
 }
 
+/**
+ * Lay down the Step 4 verifier and Step 5 reverse auditor a complete high-effort
+ * review runs: each one's recorded prompt, its brief, and the harness's transcript
+ * of an agent launched with it that opened the brief. Neither names a line range,
+ * so neither grants chunk coverage — they answer only "did the step run", which is
+ * what `verificationGaps` asks. Pass a subset of `keys` to model a skipped step.
+ */
+function recordStep45(
+  planPath: string,
+  keys: string[] = ['verify', 'reverse-audit'],
+): void {
+  const d = promptRecordDir(planPath);
+  mkdirSync(d, { recursive: true });
+  for (const key of keys) {
+    const brief = briefPath(planPath, key);
+    writeFileSync(brief, `The ${key} brief.`);
+    const launch =
+      `You are review agent \`${key}\`.\n` +
+      `read_file(file_path="${brief}")\n` +
+      `read_file(file_path="${DIFF}")`;
+    // Match production (`prompt-record.ts`): the record filename is the
+    // percent-encoded key. A no-op for `verify`/`reverse-audit`, but a future role
+    // whose name `encodeURIComponent` transforms would otherwise be written to a
+    // name the reader never looks for.
+    writeFileSync(join(d, `${encodeURIComponent(key)}.txt`), launch);
+    transcript(`v-${key.replace(/[^a-z0-9]/gi, '_')}`, launch, {
+      toolCalls: 2,
+      opens: [brief],
+    });
+  }
+}
+
 /** Write one agent transcript, as the harness would. */
 function transcript(
   id: string,
@@ -218,14 +255,22 @@ function blindPrompt(chunk: number): string {
   return `The changes are in chunk ${chunk} of 2, covering lines 1-100 of the diff.`;
 }
 
-/** Both chunks reviewed by agents that opened the diff. */
-function coveredPlan(): string {
+/**
+ * Both chunks reviewed by agents that opened the diff, and Step 4/5 ran — a
+ * complete high-effort review. Pass a subset of keys to model a run that skipped a
+ * step (what the (B) gap tests are about); `plan({ step45: false })` suppresses the
+ * default pair so this controls them exactly.
+ */
+function coveredPlan(
+  step45Keys: string[] = ['verify', 'reverse-audit'],
+): string {
   transcript('a1', goodPrompt(1), { toolCalls: 3 });
   transcript('a2', goodPrompt(2), { toolCalls: 2 });
-  const p = plan();
+  const p = plan({ step45: false });
   recordBuilt(p, 1);
   recordBuilt(p, 2);
   recordMatrix(p);
+  recordStep45(p, step45Keys);
   return p;
 }
 
@@ -905,6 +950,90 @@ describe('coverage is recomputed, never accepted', () => {
   });
 });
 
+describe('the Step 4/5 gate — verify and reverse audit must have run (high effort)', () => {
+  it('caps a clean APPROVE to COMMENT when the reverse audit never ran', () => {
+    // The high-value catch: a zero-finding high-effort review that skipped the pass
+    // meant to find what Step 3 missed cannot certify the diff clean. compose-review
+    // runs only at high effort, so reverse audit is always owed here.
+    const r = composeReview({
+      criticalsInline: 0,
+      suggestionsInline: 0,
+      planPath: coveredPlan(['verify']), // reverse audit absent
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(r.event).toBe('COMMENT');
+    expect(r.cappedBy).toContain('unreviewed-dimension');
+    expect(r.body).toMatch(/reverse audit — no auditor ran/);
+  });
+
+  it('discloses that posted findings were not verified when Step 4 was skipped', () => {
+    // A confirmed Critical still blocks — a cap never softens a REQUEST_CHANGES —
+    // but the body says the posted findings were not verified.
+    const r = composeReview({
+      criticalsInline: 1,
+      suggestionsInline: 0,
+      planPath: coveredPlan(['reverse-audit']), // verifier absent
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(r.event).toBe('REQUEST_CHANGES');
+    expect(r.body).toMatch(/verification — the review posts findings/);
+  });
+
+  it('does not require a verifier on a review that confirmed nothing', () => {
+    // C=0, S=0: nothing to verify. The reverse audit ran, so this approves.
+    const r = composeReview({
+      criticalsInline: 0,
+      suggestionsInline: 0,
+      planPath: coveredPlan(['reverse-audit']), // verifier absent, none needed
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(r.event).toBe('APPROVE');
+    expect(r.body).not.toMatch(/verification/);
+  });
+
+  it('approves a review that ran both verify and the reverse audit', () => {
+    const r = composeReview({
+      criticalsInline: 0,
+      suggestionsInline: 0,
+      planPath: coveredPlan(), // both present
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(r.event).toBe('APPROVE');
+  });
+
+  it('requires a verifier for a body Critical that is not pre-confirmed', () => {
+    // A non-deterministic Critical that could not be anchored still posts (in the
+    // body) and still had to be verified — so a missing verifier is disclosed even
+    // with no inline findings.
+    const r = composeReview({
+      bodyCriticals: ['a real blocker that could not be anchored'],
+      planPath: coveredPlan(['reverse-audit']), // verifier absent
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(r.event).toBe('REQUEST_CHANGES');
+    expect(r.body).toMatch(/verification — the review posts findings/);
+  });
+
+  it('does not require a verifier for a deterministic [build]/[test] body Critical', () => {
+    // A `[build]`/`[test]` finding is pre-confirmed and skips verification by design,
+    // so a review whose only finding is one must not be told its findings were
+    // unverified — that would post a false disclosure on a correct review.
+    const r = composeReview({
+      bodyCriticals: ['[build] `npm run build` failed: TS2345 in x.ts'],
+      planPath: coveredPlan(['reverse-audit']), // verifier absent, none needed
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(r.event).toBe('REQUEST_CHANGES');
+    expect(r.body).not.toMatch(/verification/);
+  });
+});
+
 // `verdictLine` is what Step 6 prints — the one place a verdict exists for the
 // user. It had no test, and a review of this change found the reason to want one.
 describe('verdictLine — the terminal verdict, and its dangling colon', () => {

packages/cli/src/commands/review/compose-review.ts

@@ -25,6 +25,7 @@ import { dirname } from 'node:path';
 import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
 import {
   coverageFromTranscripts,
+  verificationGaps,
   TranscriptsUnavailableError,
 } from './lib/coverage.js';
 
@@ -282,6 +283,35 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult {
         `coverage — ${why}, so this run cannot show that any of the diff was read`,
       );
     }
+
+    // Step 4 (verify) and Step 5 (reverse audit) ran, and read their briefs?
+    // `check-coverage` proves Step 3, but it runs at Step 3D — before these exist —
+    // and their count is not in the plan, so its roster cannot reach them. This is
+    // the floor that does, and only `compose-review` asks it, which runs only at
+    // high effort — the only effort at which verify and reverse audit run at all.
+    // Reverse audit is required on every high-effort review; verify once the review
+    // has non-deterministic findings to verify. Deterministic `[build]`/`[test]`
+    // findings are pre-confirmed and skip verification by design, so they do not
+    // demand a verifier — including a body Critical that carries their source tag.
+    // Its own try, so a read failure here says so rather than wearing the coverage
+    // message, and does not undo a coverage pass a line above it.
+    try {
+      const findingsToVerify =
+        criticalsInline +
+        suggestionsInline +
+        bodyCriticals.filter((c) => !/\[(?:build|test)\]/i.test(c)).length;
+      const verification = verificationGaps(
+        input.planPath,
+        { postsFindings: findingsToVerify > 0 },
+        input.env,
+      );
+      for (const gap of verification.gaps) unreviewed.push(gap);
+    } catch (err) {
+      unreviewed.push(
+        `verification — could not check that Step 4 and Step 5 ran ` +
+          `(${(err as Error).message})`,
+      );
+    }
   }
   const contextUnavailable = toBool(
     input.contextUnavailable,