diff --git a/packages/cli/src/commands/review/check-coverage.test.ts b/packages/cli/src/commands/review/check-coverage.test.ts index f756d9a1f6a..cf4973330ec 100644 --- a/packages/cli/src/commands/review/check-coverage.test.ts +++ b/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 `.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 `), 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/); + }); +}); diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index d3d64170f0a..18e5790c965 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/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', () => { diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index f5dde203210..3dc35620460 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/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, diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts index b51a5924459..a866e24dc87 100644 --- a/packages/cli/src/commands/review/lib/coverage.ts +++ b/packages/cli/src/commands/review/lib/coverage.ts @@ -403,7 +403,12 @@ export function coverageFromTranscripts( // Every role, territory agents included. Their brief is where the severity // definitions, the paging rule, the uncoverable rule and the project rules live. const brief = briefPath(planPath, req.key); - const opened = agent.successfulCallArgs.some((a) => a.includes(brief)); + // The brief as a whole JSON string value (`successfulCallArgs` are already + // serialized args): a bare substring would credit `${brief}.bak` for the brief, + // the same trap `parseTranscript` avoids for the diff path. + const opened = agent.successfulCallArgs.some((a) => + a.includes(JSON.stringify(brief)), + ); if (!opened) { unreadBriefs.push( `${roleLabel(req)} — never opened its brief (${brief}), so it reviewed ` + @@ -443,4 +448,113 @@ export function coverageFromTranscripts( }; } +export interface VerificationReport { + /** True when every required Step 4/5 agent ran and read its brief. */ + ok: boolean; + /** + * Self-explanatory gap lines, shaped to drop straight into + * `unreviewedDimensions` — each carries its own ` — ` reason, so + * `compose-review` renders it verbatim rather than appending the whiff sentence. + */ + gaps: string[]; +} + +/** + * Did Step 4 (verify) and Step 5 (reverse audit) actually run, and read their + * briefs? + * + * `check-coverage` proves Step 3 was done — but it runs at Step 3D, *before* these + * two, so its roster (`requiredAgents`) cannot reach them. And their count is not + * in the plan: verify shards on the finding count (`ceil(N/8)`), reverse audit + * loops until it goes dry. So this is not an exact roster — it is a floor, and it + * is asked only by `compose-review`, which runs only at high effort. A low/medium + * quick pass has no verify and no reverse audit, and never reaches here (it emits + * no verdict, so it calls no `compose-review`). + * + * The floor is deliberately one agent per step, for the failure it exists to catch: + * the step skipped **wholesale**, or run with agents that never opened their brief — + * the same silent omission the rest of this file is a response to. Per-chunk + * completeness of a Step 3B reverse audit is the orchestrator's Step 5 loop + * contract, disclosed through `unreviewedDimensions` when a scope is left + * outstanding; this does not re-litigate it. + * + * Like everything here, nothing is supplied by the caller but the plan path. The + * proof is the intersection of two artifacts with different authors: the prompt the + * CLI recorded building (`reverse-audit` / `reverse-audit--chunk-N` / `verify`) and + * the harness's transcript of an agent launched with it that opened its brief. + */ +export function verificationGaps( + planPath: string, + opts: { postsFindings: boolean }, + env: NodeJS.ProcessEnv = process.env, +): VerificationReport { + const { plan, mtimeMs } = readPlan(planPath); + const records = readTranscripts(mtimeMs, env, plan.diffPathAbsolute); + const built = readRecordedPrompts(planPath); + const gaps: string[] = []; + + // A role whose prompt the CLI recorded, and which an agent was then launched with + // verbatim AND opened the brief it points at. The same two-author proof the + // roster check uses — a delivered launch prompt and a successful call naming the + // brief file — asked of a key rather than a plan-derived role. + const ranAndReadBrief = (key: string): boolean => { + const b = built.get(key); + if (b === undefined || b.trim() === '') return false; + const brief = briefPath(planPath, key); + // Match the brief as a whole JSON string value, quotes included — the same + // lesson `parseTranscript` learned for the diff path: a bare substring credits + // `…/x.brief.md.bak` for `…/x.brief.md`. `successfulCallArgs` are already + // `JSON.stringify(args)`, so the quoted path is what a real read of the brief + // leaves in them. + const needle = JSON.stringify(brief); + return records.some( + (r) => + wasDeliveredVerbatim(r.launchPrompt, b) && + r.successfulCallArgs.some((a) => a.includes(needle)), + ); + }; + + // Step 5: reverse audit. Required on EVERY high-effort review — it is the pass + // that hunts what Step 3 missed, and a verdict that never ran it cannot certify + // the diff complete, least of all a clean one (a zero-finding review is exactly + // when a second look matters most). 3A records it under `reverse-audit`; 3B under + // `reverse-audit--chunk-N`, one per chunk. The floor is one: at least one auditor + // ran and read its brief. Matched on the role name and the universal `--` key + // separator rather than the exact `--chunk-` shape, so a change to how the + // chunk suffix is spelled does not silently drop every per-chunk key here. + const reverseKeys = [...built.keys()].filter( + (k) => k === 'reverse-audit' || k.startsWith('reverse-audit--'), + ); + if (!reverseKeys.some(ranAndReadBrief)) { + gaps.push( + reverseKeys.length === 0 + ? 'reverse audit — no auditor ran (Step 5 builds its prompt with ' + + '`agent-prompt --role reverse-audit`; none was recorded, so the pass ' + + 'that looks for what Step 3 missed was skipped)' + : 'reverse audit — its prompt was built, but no agent was launched with ' + + 'it that opened its brief, so the reverse-audit pass did not run', + ); + } + + // Step 4: verify. Required when the review posts a finding a verifier rules on — + // an unverified finding must not become a public blocker (the false "this PR now + // leaks tokens" Critical is the exact harm). Whether it does is `opts.postsFindings`, + // decided by the caller: `compose-review` counts the anchored findings and the + // non-deterministic body Criticals, and excludes deterministic `[build]`/`[test]` + // findings, which are pre-confirmed and skip verification by design. A review that + // confirmed nothing has nothing to verify. + if (opts.postsFindings && !ranAndReadBrief('verify')) { + gaps.push( + built.has('verify') + ? 'verification — its prompt was built, but no agent was launched with it ' + + 'that opened its brief, so the posted findings were not verified' + : 'verification — the review posts findings, but no verifier ran (Step 4 ' + + 'builds its prompt with `agent-prompt --role verify`; none was ' + + 'recorded, so the findings were not verified)', + ); + } + + return { ok: gaps.length === 0, gaps }; +} + export { TranscriptsUnavailableError }; diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index d8ec5c9ca41..7cf6b6fccbe 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -620,12 +620,14 @@ qwen review compose-review --input .qwen/tmp/qwen-review-{target}-compose.json \ It prints a `Verdict:` line to stderr. **That line is the verdict — print it, and nothing else.** It writes nothing, posts nothing, and needs no authorisation, so run it on every high-effort review, whether or not you are going to post. The state file is the same one Step 7 uses (see there for every field): your findings and the states you established — the body Criticals, the discarded suggestions, the `cannot tell` blockers, the unreviewed dimensions, the `planPath`, the presubmit flags, the model id. It does **not** take the coverage or the inline counts. It derives coverage from the harness's transcripts, and Step 7 derives the inline counts from the comments you actually attach. +**It also proves Step 4 and Step 5 ran — the way `check-coverage` proves Step 3.** `check-coverage` runs at Step 3D, before verify and reverse audit exist, so its roster cannot reach them; and their count is not in the plan (verify shards on the finding count, the reverse audit loops until it goes dry), so there is no exact roster to check. What there is is a floor, and `compose-review` — which runs only at high effort, where both steps are part of the contract — checks it from the same transcripts: at least one **reverse auditor** ran and opened its brief (on every high-effort review), and at least one **verifier** did (whenever the review posts findings). A step skipped wholesale, or run with agents that never opened their brief, is named in `unreviewedDimensions` and caps the verdict, exactly like a dimension nobody reviewed. You do not pass a flag for this and cannot turn it off: the proof is the intersection of the prompt the CLI recorded building (`--role verify` / `--role reverse-audit`) and the harness's transcript of an agent that ran it. So a run cannot approve a diff by skipping the pass that looks for what Step 3 missed — the highest-value catch here is a clean, zero-finding review that never ran its reverse audit. + The rules it applies — so you can read the line it gives you, not so you can apply them yourself: - Only **high-confidence** findings count. Low-confidence ones are terminal-only, under "Needs Human Review". - **Approve** — no high-confidence Critical, and no cap state. - **Request changes** — one or more high-confidence Criticals, anchored or in the body. -- **Comment** — suggestions but no blockers, **or** an Approve that a cap took away: an uncoverable chunk, a chunk nobody read, a dimension nobody reviewed, an existing blocker you could not rule on, a PR whose discussion you could not read. A review that did not read part of the diff cannot certify it. +- **Comment** — suggestions but no blockers, **or** an Approve that a cap took away: an uncoverable chunk, a chunk nobody read, a dimension nobody reviewed, a **reverse audit that never ran** (or a **verifier** that never ran on a review with findings), an existing blocker you could not rule on, a PR whose discussion you could not read. A review that did not read part of the diff — or never looked for what it missed — cannot certify it. **Why this is a command and not a paragraph.** It was a paragraph, and the paragraph was skipped. Dogfooded, a run read the coverage check's refusal, concluded that "the agents clearly did their job", never called `compose-review` at all, and printed **`Review complete — Approve`** — a verdict it had composed itself, from prose, on a review whose gate had just refused. There is now one place a verdict exists. Skipping the command does not get you a different one; it gets you none.