diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 51f81e0a92d..11872a184d7 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -417,6 +417,44 @@ describe('agent-prompt (command boundary)', () => { rmSync(dir, { recursive: true, force: true }); } }); + + it('a rules change changes the key — a corrected-rules rebuild cannot inherit the old brief', () => { + // The digest keyed findings alone. A round launched without the project + // rules and rebuilt with them kept its key, so the corrected brief landed + // at the SAME path the first round's agent had already opened — and the + // delivery check credited that old transcript with reading rules it never + // saw. The key is the identity of the launch material; rules are launch + // material. + const dir = mkdtempSync(join(tmpdir(), 'ap-ruleskey-')); + try { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); + const findings = join(dir, 'f.md'); + writeFileSync(findings, '- **[Critical]** x.ts:1 — y'); + const rulesFile = join(dir, 'rules.md'); + writeFileSync(rulesFile, 'Never merge without a changeset entry.'); + const handler = agentPromptCommand.handler as (a: unknown) => void; + handler({ plan, role: 'verify', findings }); + handler({ plan, role: 'verify', findings, rules: rulesFile }); + + const recorded = readRecordedPrompts(plan); + const keys = [...recorded.keys()]; + // Two records, not one overwritten: same findings, different rules, + // different identity. + expect(keys).toHaveLength(2); + // Each launch reads its OWN brief: the rules-less brief stayed intact + // where its transcript can honestly match it, and the corrected round + // has a fresh path no old transcript has ever opened. + const briefs = keys.map((k) => readFileSync(briefPath(plan, k), 'utf8')); + const ruled = briefs.filter((b) => b.includes('## Project rules')); + const bare = briefs.filter((b) => !b.includes('## Project rules')); + expect(ruled).toHaveLength(1); + expect(bare).toHaveLength(1); + expect(ruled[0]).toContain('Never merge without a changeset entry.'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); }); // One call per review, not one per agent. The per-agent form asks for ~30 @@ -425,6 +463,301 @@ describe('agent-prompt (command boundary)', () => { // was built for any of twelve roles" in a day — the builder simply stopped being // called. The roster call and check-coverage read the same list out of the same // plan, so what gets built is exactly what gets checked. +describe('--all-chunks — every auditor of a Step 5 round, in one call', () => { + beforeEach(() => { + (writeStdoutLine as unknown as Mock).mockClear(); + }); + + it('builds one labelled block per chunk, each recorded as its exact printed prompt', () => { + // The per-chunk form asked for one build-and-capture round trip per chunk; + // a real run answered with `for i in …; do agent-prompt … | head -5; done` + // — it sampled each build, never possessed the texts, hand-reconstructed + // all ten launches, and every one was flagged rewritten. One call, blocks + // to copy, nothing to reconstruct. + const dir = mkdtempSync(join(tmpdir(), 'ap-allchunks-')); + try { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); // chunks 13, 14, 15 + const findings = join(dir, 'f.md'); + writeFileSync(findings, '- **[Critical]** x.ts:1 — y'); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + 'all-chunks': true, + findings, + }); + + const printed = (writeStdoutLine as unknown as Mock).mock + .calls[0][0] as string; + // Numbered blocks + end marker: the same truncation self-check as the + // roster, and an explicit ban on sampling the output. + expect(printed).toContain('3 auditors required this round'); + expect(printed).toContain('NEVER sample this output'); + expect(printed).toMatch(/───── auditor 1 of 3 — chunk 13 ─────/); + expect(printed).toMatch(/───── end of round — 3 auditors ─────/); + + const recorded = readRecordedPrompts(plan); + const keys = [...recorded.keys()].sort(); + expect(keys).toHaveLength(3); + for (const c of [13, 14, 15]) { + const key = keys.find((k) => + k.startsWith(`reverse-audit--chunk-${c}--`), + )!; + expect(key).toMatch(/--[0-9a-f]{12}$/); + const rec = recorded.get(key)!; + // The record IS the printed block, identity line first, findings in. + expect(printed).toContain(rec); + expect(rec.startsWith('You are review agent `reverse-audit`')).toBe( + true, + ); + expect(rec).toContain('- **[Critical]** x.ts:1 — y'); + } + // Each block reads its OWN chunk's range — asserted on two different + // chunks, because checking only the first cannot see a batch that built + // every block from the same chunk. + const rec13 = recorded.get( + keys.find((k) => k.includes('--chunk-13--'))!, + )!; + const rec14 = recorded.get( + keys.find((k) => k.includes('--chunk-14--'))!, + )!; + expect(rec13).toContain('offset=3807'); + expect(rec13).not.toContain('offset=4024'); + expect(rec14).toContain('offset=4024'); + expect(rec14).not.toContain('offset=3807'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('refuses a plan with no chunks[] at all — an empty plan is not a clean round', () => { + // The first guard in runAllChunks; the id-validation tests below all pass + // a populated chunks[], so this guard inverted or deleted would let an + // empty plan through with no test going red. + const dir = mkdtempSync(join(tmpdir(), 'ap-allchunks-none-')); + try { + const findings = join(dir, 'f.md'); + writeFileSync(findings, '- x'); + const emptied = { ...PLAN, chunks: [] }; + const missing = { ...PLAN } as Record; + delete missing['chunks']; + for (const shape of [emptied, missing]) { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(shape)); + expect(() => + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + 'all-chunks': true, + findings, + }), + ).toThrow(/no `chunks\[\]`/); + expect(readRecordedPrompts(plan).size).toBe(0); + } + expect(writeStdoutLine as unknown as Mock).not.toHaveBeenCalled(); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('refuses a plan whose every chunk id is unusable — zero auditors is not a clean round', () => { + // The filter used to swallow this: all-non-integer ids passed the has-chunks + // guard, the filter emptied the list, and the command printed "0 auditors + // required this round" with a valid end marker and recorded nothing — a + // zero-coverage round wearing a receipt. The single-chunk path throws on + // the same corruption; so does the batch now. + const dir = mkdtempSync(join(tmpdir(), 'ap-allchunks-0-')); + try { + const plan = join(dir, 'plan.json'); + writeFileSync( + plan, + JSON.stringify({ + ...PLAN, + chunks: PLAN.chunks.map((c) => ({ ...c, id: 'x' })), + }), + ); + const findings = join(dir, 'f.md'); + writeFileSync(findings, '- x'); + expect(() => + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + 'all-chunks': true, + findings, + }), + ).toThrow(/no positive integer id/); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('refuses a plan with ONE unusable or duplicated chunk id — no shrunken round, nothing written', () => { + // Filtering handled only the all-bad case: `[13, "x", 15]` still printed a + // valid-looking TWO-auditor round with one territory silently gone, and + // `[13, 13, 15]` resolved both id-13 blocks to the same chunk and the same + // record key — the second territory never audited, under an end marker + // that says the round is whole. Same corruption coverage's readPlan + // refuses; the batch must refuse it before writing anything. + const dir = mkdtempSync(join(tmpdir(), 'ap-allchunks-part-')); + try { + const findings = join(dir, 'f.md'); + writeFileSync(findings, '- x'); + const cases: Array<[unknown[], RegExp]> = [ + [ + [PLAN.chunks[0], { ...PLAN.chunks[1], id: 'x' }, PLAN.chunks[2]], + /no positive integer id/, + ], + [ + [PLAN.chunks[0], { ...PLAN.chunks[1], id: 13 }, PLAN.chunks[2]], + /duplicate chunk ids/, + ], + ]; + for (const [chunks, pattern] of cases) { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify({ ...PLAN, chunks })); + expect(() => + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + 'all-chunks': true, + findings, + }), + ).toThrow(pattern); + // Refused BEFORE any brief, record or stdout block — a partial round + // on disk would be indistinguishable from a delivered one. + expect(readRecordedPrompts(plan).size).toBe(0); + expect(writeStdoutLine as unknown as Mock).not.toHaveBeenCalled(); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('refuses --all-chunks for a role that is not per-chunk-findings, and with --chunk', () => { + const dir = mkdtempSync(join(tmpdir(), 'ap-allchunks-x-')); + try { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); + const findings = join(dir, 'f.md'); + writeFileSync(findings, '- x'); + expect(() => + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'verify', + 'all-chunks': true, + findings, + }), + ).toThrow(/does not take it/); + expect(() => + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + 'all-chunks': true, + chunk: 13, + findings, + }), + ).toThrow(/contradict/); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it.each([ + ['--roster', { roster: true }, /--roster builds every prompt/], + [ + '--whole-diff', + { 'whole-diff': true }, + /--whole-diff builds the diff-reading block alone/, + ], + ['a bare --chunk', { chunk: 13 }, /contradict/], + ['nothing else', {}, /needs --role and --findings /], + ])( + 'refuses --all-chunks combined with %s — never silently dropped', + (_, extra, pattern) => { + // The batch gate reads `allChunks && role && findings`, so every one of + // these used to pass the guards, run the OTHER mode, and exit 0 with the + // batch silently discarded — an orchestrator that asked for a round + // walked away believing it was built. Ruled on at the primary-mode + // boundary, before any mode can quietly win. + expect(() => + (agentPromptCommand.handler as (a: unknown) => void)({ + plan: '/nonexistent/plan.json', + 'all-chunks': true, + ...extra, + }), + ).toThrow(pattern as RegExp); + expect(writeStdoutLine as unknown as Mock).not.toHaveBeenCalled(); + }, + ); + + it('an empty findings file still builds one auditor per chunk, each with the early-round framing', () => { + // Step 5's first round on a clean review passes an empty file — the batch + // gate reads `findingsContent !== undefined` for exactly that reason. A + // truthiness regression turns '' falsy, falls through to the single-role + // path, and prints ONE 3A-style prompt where the round needs one auditor + // per chunk — with every other batch test green, because they all pass + // non-empty content. + const dir = mkdtempSync(join(tmpdir(), 'ap-allchunks-empty-')); + try { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); + const findings = join(dir, 'f.md'); + writeFileSync(findings, ''); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + 'all-chunks': true, + findings, + }); + const printed = (writeStdoutLine as unknown as Mock).mock + .calls[0][0] as string; + expect(printed).toContain('3 auditors required this round'); + expect(printed).toMatch(/───── end of round — 3 auditors ─────/); + // EVERY block carries the empty-list framing, not just the first — a + // batch that fell through would carry it zero times or once. + expect(printed.split('Nothing is confirmed yet')).toHaveLength(4); + const keys = [...readRecordedPrompts(plan).keys()]; + expect( + keys.filter((k) => k.startsWith('reverse-audit--chunk-')), + ).toHaveLength(3); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('--rules lands in every brief of the batch', () => { + // The batch plumbs `rules` through buildLaunch per chunk. Dropping that + // argument would leave labels, keys, records and ranges — everything the + // other tests pin — exactly as they are, while every auditor of every + // round silently runs without the project's review rules. + const dir = mkdtempSync(join(tmpdir(), 'ap-allchunks-rules-')); + try { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); + const findings = join(dir, 'f.md'); + writeFileSync(findings, '- **[Critical]** x.ts:1 — y'); + const rulesFile = join(dir, 'rules.md'); + writeFileSync(rulesFile, 'Never merge without a changeset entry.'); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + 'all-chunks': true, + findings, + rules: rulesFile, + }); + const keys = [...readRecordedPrompts(plan).keys()]; + expect(keys).toHaveLength(3); + for (const key of keys) { + const brief = readFileSync(briefPath(plan, key), 'utf8'); + expect(brief).toContain('## Project rules'); + expect(brief).toContain('Never merge without a changeset entry.'); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + describe('--roster — every prompt the plan requires, in one call', () => { beforeEach(() => { (writeStdoutLine as unknown as Mock).mockClear(); @@ -786,7 +1119,7 @@ describe('--findings — fold the list in, print one block, record EXACTLY that return { printed, plan }; } - it('a verifier gets the findings folded above, and the record IS the printed prompt', () => { + it('a verifier gets the findings folded beneath its identity line, and the record IS the printed prompt', () => { 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 @@ -802,10 +1135,19 @@ describe('--findings — fold the list in, print one block, record EXACTLY that // the delivery check passed while no verifier ever saw a finding. const recorded = recordByPrefix(plan, 'verify--'); expect(recorded).toBe(printed); - // The attack shape from the review: delivering the tail alone (the old - // findings-free block) no longer matches the record. - const tail = printed.slice(printed.indexOf('You are review agent')); - expect(wasDeliveredVerbatim(tail, recorded)).toBe(false); + // The identity line leads the output — the one spot a real run edited on a + // fully possessed prompt was the head, where it swapped the role line for + // its own context sentence; with identity first, a context wrap lands + // above it instead of replacing it. + expect(printed.startsWith('You are review agent `verify`')).toBe(true); + // The attack shape from the review: a launch that carries the block but + // DROPS the findings section still matches no record. + const identity = printed.split('\n')[0]; + const afterFindings = printed.slice( + printed.indexOf('**Your brief is a file'), + ); + const findingsFree = `${identity}\n\n${afterFindings}`; + expect(wasDeliveredVerbatim(findingsFree, recorded)).toBe(false); // The compliant launch (possibly wrapped) still does. expect(wasDeliveredVerbatim(`Context.\n${printed}\nGo.`, recorded)).toBe( true, diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index a7b7f8b0838..3960766d1b3 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -42,7 +42,11 @@ import { createHash } from 'node:crypto'; import { readFileSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; import { writeStdoutLine } from '../../utils/stdioHelpers.js'; -import { READ_FILE_CHAR_CAP, type DiffChunk } from './lib/diff-plan.js'; +import { + READ_FILE_CHAR_CAP, + chunkIdsProblem, + type DiffChunk, +} from './lib/diff-plan.js'; import { recordPrompt, writeBrief } from './lib/prompt-record.js'; import { BRIEFS, type RoleId } from './lib/agent-briefs.js'; import { pathRulesFor } from './lib/path-rules.js'; @@ -65,6 +69,8 @@ interface AgentPromptArgs { wholeDiff?: boolean; /** Build every prompt the plan's roster requires, in one call. */ roster?: boolean; + /** With --role reverse-audit: build one block PER CHUNK, in one call. */ + allChunks?: boolean; rules?: string; /** * A file of findings to fold into a verify/reverse-audit prompt, so the caller @@ -1127,6 +1133,52 @@ function buildLaunch( return { key, prompt: buildChunkLaunchPrompt(report, id, briefFile) }; } +/** + * The digest that keys a findings role's record and brief: the identity of the + * launch material the key must tell apart — the findings list AND the effective + * project rules. Findings alone left the rules out of that identity: a round + * rebuilt with corrected rules kept its key, so the rebuilt brief landed at the + * SAME path a first-round agent had already opened, and the delivery check + * credited that old transcript with reading rules it never saw. A JSON tuple, + * not concatenation — `["ab",""]` and `["a","b"]` must not collide — and + * `null` for no-rules, so a rules-less build stays distinct from an empty file. + */ +function findingsDigest(content: string, rules: string | undefined): string { + return createHash('sha256') + .update(JSON.stringify([content, rules ?? null])) + .digest('hex') + .slice(0, 12); +} + +/** + * Fold a findings section into a launch prompt, identity line FIRST. + * + * The first cut printed the findings above the whole block, which buried the + * role line mid-output — and the one hand-edit a real run made to a fully + * possessed prompt was exactly there: it dropped the identity line and wrote + * its own context sentence in that spot. With the identity at the top, a + * context wrap lands ABOVE it instead of replacing it, and the delivery check + * keeps its first anchor line. + */ +function foldFindings(role: RoleId, content: string, prompt: string): string { + const nl = prompt.indexOf('\n'); + const identity = nl === -1 ? prompt : prompt.slice(0, nl); + // The split is anchored on line one BEING the identity line — + // `buildRoleLaunchPrompt` writes it first. If a future prompt shape moves + // it, refuse here rather than fold the findings under whatever line came + // first: that would silently rebuild the buried-identity layout this + // function exists to prevent. + if (!identity.startsWith('You are review agent `')) { + throw new Error( + 'agent-prompt: foldFindings expected the launch prompt to open with ' + + `its identity line, got: "${identity.slice(0, 60)}". Keep the ` + + 'identity line first in buildRoleLaunchPrompt, or update the fold.', + ); + } + const rest = nl === -1 ? '' : prompt.slice(nl + 1); + return `${identity}\n\n${findingsSection(role, content)}\n${rest}`; +} + /** * The line above each roster block: who this launch is, in the reader's terms. * @@ -1200,6 +1252,74 @@ function runRoster(report: PlanReport, planPath: string, rules?: string): void { ); } +/** + * One block per chunk for a per-chunk findings role, in one call. + * + * The per-chunk form asked the orchestrator for one build-and-capture round + * trip per chunk per round, and a real run answered with `for i in 1..10; do + * agent-prompt … | head -5; done` — it SAMPLED each build instead of capturing + * it, never possessed the texts, and hand-reconstructed all ten launches; every + * one was flagged rewritten and the review paid a repair round. Same medicine + * as `--roster`: one call, labelled numbered blocks, an end marker, and nothing + * left to reconstruct. + */ +function runAllChunks( + report: PlanReport, + planPath: string, + role: RoleId, + findingsContent: string, + rules?: string, +): void { + if (!Array.isArray(report.chunks) || report.chunks.length === 0) { + throw new Error('agent-prompt: the plan has no `chunks[]`.'); + } + const chunks = report.chunks as DiffChunk[]; + // The same refusal coverage makes (`readPlan`), made BEFORE any brief, + // record or block is written. Filtering the unusable ids out instead shrank + // the round: `[13, "x", 15]` printed a complete-looking two-auditor round + // with one territory silently gone, and a duplicated id resolved both blocks + // to the first matching chunk and keyed them to one record — the second + // territory never audited, under an end marker that says the round is whole. + const problem = chunkIdsProblem(chunks.map((c) => c?.id)); + if (problem) { + throw new Error( + `agent-prompt: the plan has ${problem} — a round built from what ` + + 'remains would look complete while a territory goes unaudited. ' + + 'Re-run the Step 1 capture; do not hand-edit the plan.', + ); + } + const digest = findingsDigest(findingsContent, rules); + const blocks = chunks.map((c, i) => { + const key = `${role}--chunk-${c.id}--${digest}`; + const { prompt } = buildLaunch( + report, + planPath, + { role, chunk: c.id, key }, + rules, + ); + const printed = foldFindings(role, findingsContent, prompt); + recordPrompt(planPath, key, printed); + return ( + `───── auditor ${i + 1} of ${chunks.length} — chunk ${c.id} ─────\n\n` + + printed + ); + }); + writeStdoutLine( + [ + `${chunks.length} auditors required this round — one per chunk. Launch ` + + `one agent per block below, passing its block VERBATIM — copy, do not ` + + `retype, and NEVER sample this output (no \`| head\`): the text IS the ` + + `deliverable, and a launch reconstructed from a sample matches no ` + + `record. Blocks are numbered \`auditor k of ${chunks.length}\` and the ` + + `output ends with an end-of-round line — if either is missing, the ` + + `output was truncated in transit; rebuild just the missing chunks with ` + + `--chunk .`, + ...blocks, + `───── end of round — ${chunks.length} auditors ─────`, + ].join('\n\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 @@ -1218,17 +1338,25 @@ function runAgentPrompt(args: AgentPromptArgs): void { // The roster IS the selection — the plan decides who runs, which is the point. // A --roster call that also names one agent is asking for two contradictory // scopes, and honouring either would silently drop the other. - if (hasChunk || hasRole || hasFile || hasFindings || hasWhole) { + if ( + hasChunk || + hasRole || + hasFile || + hasFindings || + hasWhole || + args.allChunks + ) { bad( '--roster builds every prompt the plan requires; it takes no --chunk, ' + - '--role, --file, --findings or --whole-diff. (Step 4/5 verify and ' + - 'reverse-audit prompts are built per round, with --role and --findings.)', + '--role, --file, --findings, --whole-diff or --all-chunks. (Step 4/5 ' + + 'verify and reverse-audit prompts are built per round, with --role ' + + 'and --findings.)', ); } } else if (hasWhole) { - if (hasChunk || hasRole || hasFile || hasFindings) { + if (hasChunk || hasRole || hasFile || hasFindings || args.allChunks) { bad( - '--whole-diff builds the diff-reading block alone; it takes no --chunk, --role, --file or --findings.', + '--whole-diff builds the diff-reading block alone; it takes no --chunk, --role, --file, --findings or --all-chunks.', ); } } else if (hasRole) { @@ -1238,6 +1366,23 @@ function runAgentPrompt(args: AgentPromptArgs): void { // (`acceptsChunk`), not hardcoded here, so a new per-chunk role is a data change // in agent-briefs, not an edit to this guard — and the message names the set it // read, so it can never claim "only reverse-audit" while allowing another role. + if (args.allChunks) { + if (!BRIEFS[role]?.acceptsChunk || !BRIEFS[role]?.acceptsFindings) { + const ok = (Object.keys(BRIEFS) as RoleId[]).filter( + (r) => BRIEFS[r].acceptsChunk && BRIEFS[r].acceptsFindings, + ); + bad( + `--all-chunks builds one block per chunk for a per-chunk findings ` + + `role (${ok.join(', ')}); role "${role}" does not take it.`, + ); + } + if (hasChunk) { + bad( + '--all-chunks and --chunk contradict: one asks for every chunk, ' + + 'the other for one. Pass exactly one of them.', + ); + } + } if (hasChunk && !BRIEFS[role]?.acceptsChunk) { const chunkRoles = (Object.keys(BRIEFS) as RoleId[]).filter( (r) => BRIEFS[r].acceptsChunk, @@ -1299,6 +1444,24 @@ function runAgentPrompt(args: AgentPromptArgs): void { `${findingRoles.map((r) => `--role ${r}`).join(' / ')} prompt; ` + 'it needs one of those roles.', ); + } else if (args.allChunks) { + // --all-chunks with no role reached the batch gate as a no-op: the gate + // reads `allChunks && role && findings`, so `--chunk 13 --all-chunks` + // passed every guard, printed the single chunk block, and exited 0 with + // the batch silently dropped — an orchestrator that asked for a round + // walked away with one auditor and no error. Every mode combination is + // ruled on here, at the boundary, before any branch can quietly win. + if (hasChunk) { + bad( + '--all-chunks and --chunk contradict: one asks for every chunk, ' + + 'the other for one. Pass exactly one of them.', + ); + } + bad( + '--all-chunks builds one auditor block per chunk for a per-chunk ' + + 'findings role; it needs --role and --findings ' + + '(--role reverse-audit for a Step 5 round).', + ); } else if (!hasChunk) { bad( 'pass exactly one of --roster (every prompt the plan requires, in one ' + @@ -1384,6 +1547,17 @@ function runAgentPrompt(args: AgentPromptArgs): void { } } + if (args.allChunks && args.role && findingsContent !== undefined) { + runAllChunks( + report, + args.plan, + args.role as RoleId, + findingsContent, + rules, + ); + return; + } + let prompt: string; let key: string; if (args.wholeDiff) { @@ -1403,15 +1577,11 @@ function runAgentPrompt(args: AgentPromptArgs): void { // and keeps the documented floor of one. let keyOverride: string | undefined; if (findingsContent !== undefined && args.role) { - const digest = createHash('sha256') - .update(findingsContent) - .digest('hex') - .slice(0, 12); const base = typeof args.chunk === 'number' ? `${args.role}--chunk-${args.chunk}` : args.role; - keyOverride = `${base}--${digest}`; + keyOverride = `${base}--${findingsDigest(findingsContent, rules)}`; } ({ key, prompt } = buildLaunch( report, @@ -1432,7 +1602,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { // delivery can satisfy — the findings-free record was exactly that. const printed = findingsContent !== undefined && args.role - ? `${findingsSection(args.role as RoleId, findingsContent)}\n\n${prompt}` + ? foldFindings(args.role as RoleId, findingsContent, prompt) : prompt; recordPrompt(args.plan, key, printed); writeStdoutLine(printed); @@ -1470,6 +1640,13 @@ export const agentPromptCommand: CommandModule = { 'The heavily-rewritten file an invariant agent owns (--role ' + 'invariant-a|invariant-b|invariant-c)', }) + .option('all-chunks', { + type: 'boolean', + describe: + 'With --role reverse-audit --findings: build one block per chunk ' + + 'in one call, labelled and separated (Step 5, 3B). Never sample ' + + 'the output; each block is pasted verbatim to its own agent.', + }) .option('roster', { type: 'boolean', describe: @@ -1508,6 +1685,7 @@ export const agentPromptCommand: CommandModule = { file: argv['file'] as string | undefined, wholeDiff: argv['whole-diff'] === true, roster: argv['roster'] === true, + allChunks: argv['all-chunks'] === true, rules: argv['rules'] as string | undefined, findings: argv['findings'] as string | undefined, }); diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts index 6c662075762..fbb96d99b71 100644 --- a/packages/cli/src/commands/review/lib/coverage.ts +++ b/packages/cli/src/commands/review/lib/coverage.ts @@ -70,6 +70,7 @@ import { type RosterPlan, } from './roster.js'; import { BRIEFS } from './agent-briefs.js'; +import { chunkIdsProblem } from './diff-plan.js'; import { shellQuotePath } from './shell-quote.js'; export interface CoverageFromTranscripts { @@ -158,14 +159,9 @@ function readPlan(path: string): { plan: Plan; mtimeMs: number } { // Chunk ids are matched against what the launch prompts say and rendered into // the review body. A non-integer or duplicate id would silently never match, // and the chunk it stands for would be reported as unreviewed forever. - const ids = plan.chunks.map((c) => c?.id); - if (ids.some((id) => !Number.isSafeInteger(id) || (id as number) < 1)) { - throw new Error( - `coverage: ${path} has a chunk with no positive integer id`, - ); - } - if (new Set(ids).size !== ids.length) { - throw new Error(`coverage: ${path} has duplicate chunk ids`); + const problem = chunkIdsProblem(plan.chunks.map((c) => c?.id)); + if (problem) { + throw new Error(`coverage: ${path} has ${problem}`); } return { plan, mtimeMs: statSync(path).mtimeMs }; } diff --git a/packages/cli/src/commands/review/lib/diff-plan.ts b/packages/cli/src/commands/review/lib/diff-plan.ts index ff926c6e878..dbbf4d7baa8 100644 --- a/packages/cli/src/commands/review/lib/diff-plan.ts +++ b/packages/cli/src/commands/review/lib/diff-plan.ts @@ -141,6 +141,25 @@ export interface DiffChunk { files: Array<{ path: string; newStart: number; newEnd: number }>; } +/** + * Why these chunk ids cannot key a review — or null when they can. + * + * One definition for everything keyed by `chunk-`: coverage refuses a plan + * whose ids it could never match (`readPlan`), and the prompt builder's batch + * mode must refuse the SAME plan before writing a brief, record or block — + * filtering there instead shrank the round, so `[13, "x", 15]` printed a + * complete-looking two-auditor round with one territory silently gone. + */ +export function chunkIdsProblem(ids: readonly unknown[]): string | null { + if (ids.some((id) => !Number.isSafeInteger(id) || (id as number) < 1)) { + return 'a chunk with no positive integer id'; + } + if (new Set(ids).size !== ids.length) { + return 'duplicate chunk ids'; + } + return null; +} + export interface DiffPlan { diffLines: number; diffChars: number; diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 8151948c405..356faee66da 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -532,12 +532,15 @@ Write **the cumulative list of every confirmed finding so far** (Steps 3-4 plus --findings \ [--rules ] -# Step 3B (large diff): one auditor PER CHUNK per round, launched together. -"${QWEN_CODE_CLI:-qwen}" review agent-prompt --plan --role reverse-audit --chunk \ +# Step 3B (large diff): one auditor PER CHUNK per round — ONE call builds them all. +"${QWEN_CODE_CLI:-qwen}" review agent-prompt --plan --role reverse-audit --all-chunks \ --findings \ - [--rules ] + [--rules ] \ + > .qwen/tmp/qwen-review-{target}-ra-round.txt ``` +Redirect and `read_file` it paged, exactly as with `--roster`: one labelled block per chunk, numbered `auditor k of N`, closed by an `end of round` line — launch one agent per block, verbatim. **Never sample the builder's output** (`| head`, `| tail`, a truncated read): the text IS the deliverable, and a real run that sampled each build with `| head -5` never possessed the prompts, hand-reconstructed all ten launches, and had every one flagged rewritten — a full repair round spent recovering from a shortcut that saved nothing. To rebuild a single auditor after a gap: `--chunk ` in place of `--all-chunks`. + **`--findings` is required for this role — the command refuses without it** (an early round with nothing confirmed yet passes an empty file; the command tells the auditor so). **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). A real run skipped `--findings`, hand-wrote the auditor's launch keeping only the brief pointer, and Step 6's check capped the verdict — the auditors had run and read their brief, but not one of them got the prompt the CLI built. The command records the exact block it prints — findings included, keyed per round's findings digest — so a launch that drops the confirmed list matches no record. 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.