diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 7d1e07337a3..1b7775823c3 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/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'; @@ -24,9 +32,14 @@ import { buildWholeDiffBlock, buildRoleBrief, buildRoleLaunchPrompt, + findingsSection, 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 +410,209 @@ 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): { + 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 ` 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('throws for a role it has no framing for, rather than falling through', () => { + // A future role that sets acceptsFindings but has no branch in findingsSection + // must fail loudly, not inherit the reverse auditor's "do not re-report" prose. + // Called directly with a role the function does not frame — the guards never let + // a non-findings role reach it in a real run. + expect(() => findingsSection('2', 'some findings')).toThrow( + /--findings has no framing for role "2"/, + ); + }); + + 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 diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 4961bbde5ad..9a8dd49ff4e 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/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,60 @@ 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. + * + * Each `acceptsFindings` role has its own framing, and the branches are explicit: a + * future role that opts into `--findings` but has no framing here throws, rather than + * silently inheriting the reverse auditor's "do not re-report" prose — which is wrong + * for any role not hunting gaps. (Same reasoning as the no-role guard message, which + * also derives from `acceptsFindings` so a new role cannot leave it stale.) + */ +export 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'); + } + if (role === 'reverse-audit') { + // 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'); + } + throw new Error( + `agent-prompt: --findings has no framing for role "${role}". A role that sets ` + + '`acceptsFindings` needs a branch in findingsSection; do not let it inherit ' + + "another role's framing by falling through.", + ); +} + 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 +988,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 +1027,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 (a Step 3B territory agent), --role ' + @@ -1052,8 +1138,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 + // `\n\n` still matches the recorded ``, 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 +1209,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 +1226,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, }); }, }; diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 9cf017586c7..801a87c6334 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -95,6 +95,22 @@ export interface Brief { * design exists to spare it, because the brief is what the agent is told to obey. */ acceptsChunk?: boolean; + /** + * May this role be launched `--role --findings `, folding a findings + * list into the prompt the command prints? + * + * The verifier rules on findings; the reverse auditor avoids re-reporting them. + * Both used to get their findings the same way: the command printed a launch + * block and the orchestrator hand-prepended the list above it. Dogfooded, that + * hand-assembly is where the prompt got paraphrased — the model added a round + * number, inserted its own summary, and truncated the line telling it the brief + * is authoritative — so the delivery check failed even though the agent opened + * its brief. With this flag the command folds the findings in and prints one + * block to paste, and there is no assembly step left to drift. The findings are + * NOT recorded (see runAgentPrompt): the record stays the findings-free launch + * block, so the per-shard/round key still matches by the add-only delivery rule. + */ + acceptsFindings?: boolean; /** The agent-facing text. */ brief: string; } @@ -394,6 +410,7 @@ Report a **Critical** for each violation, and give **both** locations that toget verify: { reviewsCode: true, output: 'verdicts', + acceptsFindings: true, label: 'Verification agent', readsDiff: true, brief: `You are a **verification agent**. You do not look for new problems — you rule on the findings you were handed, listed in the message that launched you, each with a file, a line, an issue, and a **failure scenario**. The failure scenario is the finding's testable claim, and your verdict is the **result of tracing it through the real code**, not a plausibility vote on how the finding reads. @@ -422,6 +439,7 @@ Return, for each finding, one verdict: 'reverse-audit': { reviewsCode: true, acceptsChunk: true, + acceptsFindings: true, label: 'Reverse audit agent', readsDiff: true, brief: `You are a **reverse audit agent**. Prior agents have already reviewed this diff and their confirmed findings are listed in the message that launched you. Your job is not to re-report them — it is to find the **gaps**: the important issues no prior agent or round caught. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 7cf6b6fccbe..853534e28ea 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -472,14 +472,17 @@ Launch verification agents that between them receive **all** non-pre-confirmed f A single verifier for every finding was cheaper, but on a large review it becomes the most context-starved agent in the pipeline: it must re-read code for each of 30-60 findings inside one context window, and its quality collapses on the tail of the list. Sharding keeps each verifier's job small; the cost is still far below one-agent-per-finding. -**Do not write the verifier's prompt. Ask for it:** +**Do not write the verifier's prompt. Ask for it — and hand it the shard's findings so it prints the whole block:** + +Write this shard's findings to a file — each with its file, line, issue and failure scenario (the scenario is the claim under test); for any **Agent 0 (Issue Fidelity)** finding, include the **issue evidence it quoted** (issue body + comments), because a root-cause claim rests on linked-issue evidence the codebase does not contain and the verifier must check against it. Then: ```bash qwen review agent-prompt --plan --role verify \ + --findings \ [--rules ] ``` -Paste what it prints to each verifier **verbatim**, and add above it the one thing that changes per shard: **the findings this shard must rule on** — each with its file, line, issue and failure scenario (the scenario is the claim under test). For any **Agent 0 (Issue Fidelity)** finding in the shard, add the **issue evidence it quoted** (issue body + comments): a root-cause claim rests on linked-issue evidence the codebase does not contain, and the verifier must be handed it to check against. In worktree mode the verifier's `working_dir` is the PR worktree (same rule as Step 3), so its reads and re-checks resolve against the PR's code. +**Paste what it prints verbatim — the whole block, findings and all. Do not prepend, append, reword, or add a shard number.** `--findings` folds the list in for you precisely so there is no hand-assembly step left to drift: dogfooded, the step that used to have you prepend the list by hand is where the prompt got paraphrased — a summary inserted, the "nothing replaces the brief" line truncated — and Step 6's check caught it and capped the verdict. The command records the findings-free launch block, so every shard's record still matches (the findings are an add-only prefix). In worktree mode the verifier's `working_dir` is the PR worktree (same rule as Step 3), so its reads and re-checks resolve against the PR's code. The brief holds the method the orchestrator used to spell out here and that a paraphrase kept dropping: trace the failure scenario through the real code rather than voting on the finding's prose; engage the diff's own documented intent before calling a documented change a regression (the rule a run skipped when it auto-posted a false "leaks tokens" Critical); and the one-way, quote-the-contradiction bar on **rejecting a Critical**. Read the brief to know what a verdict means; do not re-derive it here. @@ -517,19 +520,23 @@ After aggregation, run reverse audit **iteratively**. Each round receives the cu - **Small diffs (Step 3A path):** one reverse audit agent per round, reading the whole diff. - **Large diffs (Step 3B path):** one reverse audit agent **per chunk** per round, launched together in a single response. A single agent asked to re-read a 5 800-line diff with a growing finding list appended is the most context-starved agent in the pipeline — precisely on the PRs where the reverse audit matters most. Each per-chunk auditor gets the same territory as its Step 3B counterpart, plus the cumulative finding list for the **whole** diff (so it knows what is already covered elsewhere). -**Do not write the reverse auditor's prompt. Ask for it:** +**Do not write the reverse auditor's prompt. Ask for it — and hand it the findings so far so it prints the whole block:** + +Write **the cumulative list of every confirmed finding so far** (Steps 3-4 plus all prior rounds) to a file, so the auditor hunts what is not already on it. An early round on a clean review may have nothing confirmed yet — pass the file anyway (empty is fine; the command tells the auditor so). Then: ```bash # Step 3A (small diff): one auditor per round, the whole diff. qwen review agent-prompt --plan --role reverse-audit \ + --findings \ [--rules ] # Step 3B (large diff): one auditor PER CHUNK per round, launched together. qwen review agent-prompt --plan --role reverse-audit --chunk \ + --findings \ [--rules ] ``` -Paste what it prints **verbatim**, and add above it the one thing that changes per round: **the cumulative list of every confirmed finding so far** (Steps 3-4 plus all prior rounds), so the auditor hunts what is not already on it. The command gives each auditor its diff reads — the whole plan in 3A, one chunk's range in 3B (a Step 3B auditor handed the whole 5 800-line diff is the most context-starved agent in the pipeline, on exactly the PRs where the reverse audit matters most). In worktree mode its `working_dir` is the PR worktree. +**Paste what it prints verbatim — the whole block. Do not prepend, append, reword, or add a round number** (track the round in your own notes, not in the prompt). `--findings` folds the cumulative list in so there is no hand-assembly step to drift — the same paraphrase Step 6's check caught and capped a real run on, even though the auditor had opened its brief. The command records the findings-free launch block, so every round's record still matches. It also gives each auditor its diff reads — the whole plan in 3A, one chunk's range in 3B (a Step 3B auditor handed the whole 5 800-line diff is the most context-starved agent in the pipeline, on exactly the PRs where the reverse audit matters most). In worktree mode its `working_dir` is the PR worktree. The brief holds what the auditor is for: hunt only the **gaps** no prior agent caught, report only Critical or Suggestion, apply the Exclusion Criteria, and end with a substantive receipt (`No issues found — `) — a bare "No issues found." fails the substantive-return check below and triggers the one relaunch.