feat(review): fold the findings list into the verify/reverse-audit prompt

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

@@ -11,7 +11,15 @@
 // is in the prompt, the read call is in the prompt, and the agent is not handed a
 // sentence to recite when it finds nothing.
 
-import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest';
+import {
+  describe,
+  it,
+  expect,
+  vi,
+  beforeEach,
+  afterEach,
+  type Mock,
+} from 'vitest';
 import { mkdtempSync, rmSync, writeFileSync, readFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
@@ -26,7 +34,11 @@ import {
   buildRoleLaunchPrompt,
   agentPromptCommand,
 } from './agent-prompt.js';
-import { readRecordedPrompts, briefPath } from './lib/prompt-record.js';
+import {
+  readRecordedPrompts,
+  briefPath,
+  wasDeliveredVerbatim,
+} from './lib/prompt-record.js';
 
 const PLAN = {
   diffPathAbsolute: '/abs/.qwen/tmp/qwen-review-pr-6771-diff.txt',
@@ -397,6 +409,199 @@ describe('agent-prompt (command boundary)', () => {
   });
 });
 
+// Dogfooded on a real 3A review: the orchestrator delivered Step 3 prompts verbatim
+// but PARAPHRASED the Step 4/5 ones — added "(round 2)", inserted its own summary,
+// truncated the "nothing replaces the brief" line — because it hand-prepended the
+// findings list. `--findings` removes that assembly step: the command folds the list
+// in and prints one block. The record stays findings-free, so the shared key still
+// matches by the add-only delivery rule.
+describe('--findings — fold the list in, print one block, record the block alone', () => {
+  // Every temp dir this block makes, cleaned up after each test — the rest of the
+  // file uses try/finally; a helper-based block tracks and sweeps instead.
+  let dirs: string[] = [];
+  const tmp = (prefix: string): string => {
+    const d = mkdtempSync(join(tmpdir(), prefix));
+    dirs.push(d);
+    return d;
+  };
+  beforeEach(() => {
+    (writeStdoutLine as unknown as Mock).mockClear();
+    dirs = [];
+  });
+  afterEach(() => {
+    for (const d of dirs) rmSync(d, { recursive: true, force: true });
+  });
+
+  function run(args: Record<string, unknown>): {
+    printed: string;
+    plan: string;
+  } {
+    const dir = tmp('ap-find-');
+    const plan = join(dir, 'plan.json');
+    writeFileSync(plan, JSON.stringify(PLAN));
+    const findings = join(dir, 'findings.md');
+    writeFileSync(
+      findings,
+      '- **[Critical]** foo.ts:10 — the collision drops arguments\n' +
+        '- **[Suggestion]** bar.ts:5 — stale comment',
+    );
+    (agentPromptCommand.handler as (a: unknown) => void)({
+      plan,
+      findings,
+      ...args,
+    });
+    const printed = (writeStdoutLine as unknown as Mock).mock
+      .calls[0][0] as string;
+    return { printed, plan };
+  }
+
+  it('a verifier gets the findings folded above, and the record is findings-free', () => {
+    const { printed, plan } = run({ role: 'verify' });
+    // Printed: the findings section AND the findings themselves — and NOT the
+    // reverse auditor's framing (a branch swap in findingsSection would pass both
+    // tests if each only asserted its own heading).
+    expect(printed).toContain('## The findings you are ruling on');
+    expect(printed).not.toContain('Already confirmed');
+    expect(printed).toContain('foo.ts:10 — the collision drops arguments');
+    // and the line the orchestrator used to truncate away.
+    expect(printed).toContain('does not replace the brief; read it first');
+    // Recorded: the launch block ALONE — no findings baked in.
+    const recorded = readRecordedPrompts(plan).get('verify')!;
+    expect(recorded).not.toContain('foo.ts:10');
+    expect(recorded.startsWith('You are review agent `verify`')).toBe(true);
+    // The whole point: the delivery check still passes on the folded prompt, because
+    // the recorded block appears in order within it (findings are an add-only prefix).
+    expect(wasDeliveredVerbatim(printed, recorded)).toBe(true);
+  });
+
+  it('a reverse auditor gets the do-not-re-report framing', () => {
+    const { printed, plan } = run({ role: 'reverse-audit' });
+    expect(printed).toContain('Already confirmed — do not re-report these');
+    // and NOT the verifier's framing — the mirror of the assertion above.
+    expect(printed).not.toContain('The findings you are ruling on');
+    expect(printed).toContain('foo.ts:10 — the collision drops arguments');
+    const recorded = readRecordedPrompts(plan).get('reverse-audit')!;
+    expect(recorded).not.toContain('foo.ts:10');
+    expect(wasDeliveredVerbatim(printed, recorded)).toBe(true);
+  });
+
+  it('a Step 3B per-chunk reverse auditor takes --chunk and --findings together', () => {
+    // The one valid triple: reverse-audit declares both acceptsChunk and
+    // acceptsFindings, and Step 5 3B launches `--role reverse-audit --chunk N
+    // --findings <cumulative>` per chunk per round. The findings fold above the
+    // chunk-scoped prompt; the record is that chunk's block, findings-free, keyed by
+    // the chunk. (PLAN's chunks are 13/14/15 — chunk 14 is offset 4024, limit 176.)
+    const { printed, plan } = run({ role: 'reverse-audit', chunk: 14 });
+    expect(printed).toContain('Already confirmed — do not re-report these');
+    expect(printed).toContain('foo.ts:10 — the collision drops arguments');
+    expect(printed).toContain('offset=4024, limit=176'); // this chunk's range only
+    expect(printed).not.toContain('offset=3807'); // not chunk 13's
+    const recorded = readRecordedPrompts(plan).get('reverse-audit--chunk-14')!;
+    expect(recorded).not.toContain('foo.ts:10');
+    expect(recorded).toContain('offset=4024, limit=176');
+    expect(wasDeliveredVerbatim(printed, recorded)).toBe(true);
+  });
+
+  it('an empty findings file tells the reverse auditor nothing is confirmed yet', () => {
+    const dir = tmp('ap-find0-');
+    const plan = join(dir, 'plan.json');
+    writeFileSync(plan, JSON.stringify(PLAN));
+    const findings = join(dir, 'f.md');
+    writeFileSync(findings, '   \n  ');
+    (agentPromptCommand.handler as (a: unknown) => void)({
+      plan,
+      role: 'reverse-audit',
+      findings,
+    });
+    const printed = (writeStdoutLine as unknown as Mock).mock
+      .calls[0][0] as string;
+    expect(printed).toContain('Nothing is confirmed yet');
+    expect(printed).not.toContain('do not re-report');
+  });
+
+  it('an empty findings file tells the verifier there is nothing to verify', () => {
+    // The verify branch of findingsSection handles empty differently from the
+    // reverse auditor's (which hunts every gap) — a verifier with no findings has
+    // nothing to rule on. Asymmetric handling is exactly what regresses unnoticed.
+    const dir = tmp('ap-vf0-');
+    const plan = join(dir, 'plan.json');
+    writeFileSync(plan, JSON.stringify(PLAN));
+    const findings = join(dir, 'f.md');
+    writeFileSync(findings, '   \n  ');
+    (agentPromptCommand.handler as (a: unknown) => void)({
+      plan,
+      role: 'verify',
+      findings,
+    });
+    const printed = (writeStdoutLine as unknown as Mock).mock
+      .calls[0][0] as string;
+    expect(printed).toContain('nothing to verify');
+    expect(printed).not.toContain('Nothing is confirmed yet');
+  });
+
+  it('the record is byte-identical whether or not --findings was passed', () => {
+    // Proves the shared per-shard/round key is unaffected: two verify shards with
+    // different findings record the SAME launch block, so both match it. Same plan
+    // both times (the record embeds the plan-derived brief path), differing only in
+    // whether findings were folded into what was PRINTED.
+    const dir = tmp('ap-nof-');
+    const plan = join(dir, 'plan.json');
+    writeFileSync(plan, JSON.stringify(PLAN));
+    const findings = join(dir, 'f.md');
+    writeFileSync(findings, '- **[Critical]** foo.ts:10 — x');
+    (agentPromptCommand.handler as (a: unknown) => void)({
+      plan,
+      role: 'verify',
+      findings,
+    });
+    const withFindings = readRecordedPrompts(plan).get('verify')!;
+    (agentPromptCommand.handler as (a: unknown) => void)({
+      plan,
+      role: 'verify',
+    });
+    const withoutFindings = readRecordedPrompts(plan).get('verify')!;
+    expect(withFindings).toBe(withoutFindings);
+  });
+
+  it('cannot read the findings file — says so, does not review without them', () => {
+    const dir = tmp('ap-findbad-');
+    const plan = join(dir, 'plan.json');
+    writeFileSync(plan, JSON.stringify(PLAN));
+    expect(() =>
+      (agentPromptCommand.handler as (a: unknown) => void)({
+        plan,
+        role: 'verify',
+        findings: join(dir, 'no-such.md'),
+      }),
+    ).toThrow(/cannot read the findings/);
+  });
+
+  it.each([
+    [
+      'a dimension role',
+      { role: '2', findings: '/f' },
+      /--findings folds a findings list into the prompt, only for a role that takes one/,
+    ],
+    [
+      'no role',
+      { findings: '/f' },
+      /--findings folds a findings list into a --role verify \/ --role reverse-audit/,
+    ],
+    [
+      'whole-diff',
+      { 'whole-diff': true, findings: '/f' },
+      /--whole-diff builds the diff-reading block alone/,
+    ],
+  ])('rejects --findings with %s', (_, extra, pattern) => {
+    expect(() =>
+      (agentPromptCommand.handler as (a: unknown) => void)({
+        plan: '/nonexistent/plan.json',
+        ...extra,
+      }),
+    ).toThrow(pattern as RegExp);
+  });
+});
+
 // The half of the fan-out this command did not cover. Measured against one real
 // Step 3B run: all three whole-diff agents — cross-file tracer, test-coverage
 // matrix, build & test — were launched with a prompt that named no diff file at

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

@@ -58,6 +58,11 @@ interface AgentPromptArgs {
   /** Build only the diff-reading block (Agent 8, whose brief lives nowhere else). */
   wholeDiff?: boolean;
   rules?: string;
+  /**
+   * A file of findings to fold into a verify/reverse-audit prompt, so the caller
+   * pastes one block instead of hand-prepending the list. Printed, not recorded.
+   */
+  findings?: string;
 }
 
 /** The plan report, as far as this command needs it. */
@@ -921,6 +926,47 @@ export function buildRoleLaunchPrompt(
   return parts.join('\n');
 }
 
+/**
+ * The findings block folded above a verify / reverse-audit launch prompt, so the
+ * caller pastes one thing instead of hand-assembling it.
+ *
+ * This is what gets *printed*; it is not recorded (the record stays the findings-
+ * free launch block, so the shared per-shard/round key still matches by the add-only
+ * delivery rule). Its closing line restates that the brief is authoritative — the
+ * exact sentence the orchestrator truncated when it used to build this by hand.
+ */
+function findingsSection(role: RoleId, content: string): string {
+  const body = content.trim();
+  if (role === 'verify') {
+    return [
+      '## The findings you are ruling on',
+      '',
+      'Rule on each below — one verdict, traced through the real code, as your brief ' +
+        'defines. This list does not replace the brief; read it first.',
+      '',
+      body || '(no findings were provided — there is nothing to verify)',
+    ].join('\n');
+  }
+  // A reverse auditor: the list is what NOT to re-report. Empty is meaningful — an
+  // early round on a clean review has nothing confirmed yet, and must be told so
+  // rather than handed a bare heading.
+  return body
+    ? [
+        '## Already confirmed — do not re-report these',
+        '',
+        'These are already on the review; a gap that repeats one is not a gap. Your ' +
+          'job is what they missed. This list does not replace the brief; read it first.',
+        '',
+        body,
+      ].join('\n')
+    : [
+        '## Nothing is confirmed yet',
+        '',
+        'No prior finding to avoid — hunt every gap. This note does not replace the ' +
+          'brief; read it first.',
+      ].join('\n');
+}
+
 function runAgentPrompt(args: AgentPromptArgs): void {
   // Exactly one primary mode: a territory chunk, a named role, or the bare
   // whole-diff block. A call that named none used to fall through to the chunk
@@ -929,14 +975,16 @@ function runAgentPrompt(args: AgentPromptArgs): void {
   const hasChunk = typeof args.chunk === 'number';
   const hasRole = typeof args.role === 'string' && args.role.length > 0;
   const hasFile = typeof args.file === 'string' && args.file.length > 0;
+  const hasFindings =
+    typeof args.findings === 'string' && args.findings.length > 0;
   const hasWhole = !!args.wholeDiff;
   const bad = (msg: string): never => {
     throw new Error(`agent-prompt: ${msg}`);
   };
   if (hasWhole) {
-    if (hasChunk || hasRole || hasFile) {
+    if (hasChunk || hasRole || hasFile || hasFindings) {
       bad(
-        '--whole-diff builds the diff-reading block alone; it takes no --chunk, --role or --file.',
+        '--whole-diff builds the diff-reading block alone; it takes no --chunk, --role, --file or --findings.',
       );
     }
   } else if (hasRole) {
@@ -966,6 +1014,31 @@ function runAgentPrompt(args: AgentPromptArgs): void {
           `role "${role}" does not take --file.`,
       );
     }
+    // `--findings` folds a findings list into the printed prompt, for the two roles
+    // that take one: the verifier rules on findings, the reverse auditor avoids
+    // re-reporting them. Declared on the brief (`acceptsFindings`), like `acceptsChunk`.
+    if (hasFindings && !BRIEFS[role]?.acceptsFindings) {
+      const findingRoles = (Object.keys(BRIEFS) as RoleId[]).filter(
+        (r) => BRIEFS[r].acceptsFindings,
+      );
+      bad(
+        `--findings folds a findings list into the prompt, only for a role that ` +
+          `takes one (${findingRoles.join(', ')}); role "${role}" does not.`,
+      );
+    }
+  } else if (hasFindings) {
+    // `--findings` with no role: it has no prompt to fold into. A territory chunk
+    // agent reviews the diff, not a findings list. Name the roles it needs from the
+    // briefs, not a hardcoded pair — the wrong-role branch above already does, and a
+    // new `acceptsFindings` role must not leave this one telling a stale story.
+    const findingRoles = (Object.keys(BRIEFS) as RoleId[]).filter(
+      (r) => BRIEFS[r].acceptsFindings,
+    );
+    bad(
+      `--findings folds a findings list into a ` +
+        `${findingRoles.map((r) => `--role ${r}`).join(' / ')} prompt; ` +
+        'it needs one of those roles.',
+    );
   } else if (!hasChunk) {
     bad(
       'pass exactly one of --chunk <id> (a Step 3B territory agent), --role ' +
@@ -1052,8 +1125,31 @@ function runAgentPrompt(args: AgentPromptArgs): void {
     );
     prompt = buildChunkLaunchPrompt(report, id, briefFile);
   }
+
+  // Record the findings-FREE launch prompt, and print the one the caller pastes.
+  // For a verifier / reverse auditor given `--findings`, those differ: the printed
+  // prompt folds the findings in so there is no hand-assembly step to drift, but the
+  // record stays the launch block alone. The delivery check is add-only — the built
+  // block must appear in order in what the agent got — so a printed prompt that is
+  // `<findings>\n\n<block>` still matches the recorded `<block>`, and the per-shard
+  // (verify) / per-round (reverse-audit) key keeps working without baking a
+  // different findings list into each one's record.
+  let printed = prompt;
+  if (hasFindings && args.role) {
+    const role = args.role as RoleId;
+    let content: string;
+    try {
+      content = readFileSync(args.findings as string, 'utf8');
+    } catch (err) {
+      throw new Error(
+        `agent-prompt: cannot read the findings ${args.findings}: ` +
+          `${(err as Error).message}. Omit --findings, or pass a path that resolves.`,
+      );
+    }
+    printed = `${findingsSection(role, content)}\n\n${prompt}`;
+  }
   recordPrompt(args.plan, key, prompt);
-  writeStdoutLine(prompt);
+  writeStdoutLine(printed);
 }
 
 export const agentPromptCommand: CommandModule = {
@@ -1100,6 +1196,14 @@ export const agentPromptCommand: CommandModule = {
         describe:
           'Path to the project rules file from `load-rules` (omit when the ' +
           'review has none)',
+      })
+      .option('findings', {
+        type: 'string',
+        describe:
+          'Path to a file of findings to fold into a --role verify (the shard it ' +
+          'rules on) / --role reverse-audit (the cumulative confirmed list) prompt, ' +
+          'so you paste ONE block. The findings are printed, not recorded — paste ' +
+          'the whole output verbatim, do not add a round number or reword it.',
       }),
   handler: (argv) => {
     runAgentPrompt({
@@ -1109,6 +1213,7 @@ export const agentPromptCommand: CommandModule = {
       file: argv['file'] as string | undefined,
       wholeDiff: argv['whole-diff'] === true,
       rules: argv['rules'] as string | undefined,
+      findings: argv['findings'] as string | undefined,
     });
   },
 };