From ce9d5282b8b8953f3a0ab01c17e50c2cbf12f85c Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 19 Jul 2026 08:09:33 +0800 Subject: [PATCH 1/3] fix(review): an unverified Critical must not become a public blocker -- soften the Request changes it rides The module's header has stated the principle since the false "leaks tokens" Critical: an unverified finding must not become a public blocker. The mechanics only enforced it on the Approve row -- a verify gap capped an Approve to Comment -- while the Request-changes row stood on "a confirmed Critical earned that", with nothing checking CONFIRMED. A real bot review shipped through the gap: a CHANGES_REQUESTED posted onto an external contributor's PR (#7166) over one Critical that the review's own body disclosed as unverified, from a run whose verifier was built and never launched. verificationGaps now returns the structured bit (unverifiedFindings) instead of leaving the verdict to parse gap prose, and composeReview softens a Request changes to Comment when the non-deterministic Criticals it would post cannot be shown verified -- verifier missing, transcripts unreadable, or no plan to check against; fail closed at every exit, because "could not show" and "was not" read the same to the person the blocker is posted at. Deterministic [build]/[test] findings stay pre-confirmed and keep their Request changes. The findings still post either way -- the body copy of an unanchorable blocker survives the softening exactly as it survives the presubmit carve-out -- and the verdict line says why a Comment sits over visible Critical comments. Manipulation check, both directions: a run that wants an Approve gains nothing (the same gap already caps Approve), and a run that wants to block without verifying now cannot. --- .../commands/review/compose-review.test.ts | 123 ++++++++++++++++-- .../cli/src/commands/review/compose-review.ts | 73 +++++++++-- .../cli/src/commands/review/lib/coverage.ts | 13 +- .../cli/src/commands/review/submit.test.ts | 120 ++++++++++++++++- .../core/src/skills/bundled/review/SKILL.md | 4 +- 5 files changed, 301 insertions(+), 32 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index f9e3de7b3b8..97a29e70cc2 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -660,12 +660,18 @@ describe('composeReview — not-reviewed entries that carry their own reason', ( }); describe('composeReview — input validation (the producer is a model that omits inapplicable fields)', () => { - it('a body-Critical-only input with every count omitted is REQUEST_CHANGES (undefined + 1 = NaN once meant APPROVE)', () => { + it('a body-Critical-only input with every count omitted lands on the REQUEST_CHANGES row (undefined + 1 = NaN once meant APPROVE)', () => { + // The NaN property pins on `baseEvent`: the arithmetic put the blocker on + // the Request-changes row. The EVENT is then softened — no plan means the + // blocker cannot be shown verified — and the blocker's body copy survives + // the softening. const r = composeReview({ bodyCriticals: ['the only blocker'], modelId: MODEL, }); - expect(r.event).toBe('REQUEST_CHANGES'); + expect(r.baseEvent).toBe('REQUEST_CHANGES'); + expect(r.event).toBe('COMMENT'); + expect(r.cappedBy).toContain('criticals-unverified'); expect(r.body).toContain('**[Critical]** the only blocker'); }); @@ -823,10 +829,16 @@ describe('composeReviewCommand handler (the CLI glue)', () => { }); const written = JSON.parse(readFileSync(outPath, 'utf8')) as { event: string; + baseEvent: string; verdictLine: string; }; - expect(written.event).toBe('REQUEST_CHANGES'); - expect(written.verdictLine).toContain('Request changes'); + // The derived count reached the Request-changes row — that is the hole + // this test pins. With no plan beside it the blocker cannot be shown + // verified, so the EVENT softens and the verdict line says why. + expect(written.baseEvent).toBe('REQUEST_CHANGES'); + expect(written.verdictLine).toContain( + 'a Request changes was NOT available', + ); } finally { rmSync(dir, { recursive: true, force: true }); } @@ -853,7 +865,8 @@ describe('composeReviewCommand handler (the CLI glue)', () => { out: outPath, }); expect( - (JSON.parse(readFileSync(outPath, 'utf8')) as { event: string }).event, + (JSON.parse(readFileSync(outPath, 'utf8')) as { baseEvent: string }) + .baseEvent, ).toBe('REQUEST_CHANGES'); } finally { rmSync(dir, { recursive: true, force: true }); @@ -1496,9 +1509,14 @@ describe('the Step 4/5 gate — verify and reverse audit must have run (high eff ); }); - 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. + it('softens an unverified Request changes to Comment — no verifier, no blocker', () => { + // This test used to pin the opposite: "a confirmed Critical still blocks — + // a cap never softens a REQUEST_CHANGES". The never-soften rule presumes + // CONFIRMED, and when Step 4 never ran, nothing confirmed anything: a real + // bot review shipped a CHANGES_REQUESTED onto an external contributor's PR + // (#7166) whose one Critical its own body disclosed as unverified. The + // module's stated principle — an unverified finding must not become a + // public blocker — now has the mechanics on the Request-changes row too. const r = composeReview({ criticalsInline: 1, suggestionsInline: 0, @@ -1506,8 +1524,84 @@ describe('the Step 4/5 gate — verify and reverse audit must have run (high eff env: ENV, modelId: MODEL, }); - expect(r.event).toBe('REQUEST_CHANGES'); + expect(r.event).toBe('COMMENT'); + expect(r.baseEvent).toBe('REQUEST_CHANGES'); + expect(r.cappedBy).toContain('criticals-unverified'); expect(r.body).toMatch(/verification — the review posts findings/); + // The opener must not certify anything over an unverified blocker. + expect(r.body).not.toContain('no blockers'); + // The verdict line names what a reader would otherwise chase: a Comment + // over visible Critical comments reads as a contradiction until it says why. + expect(verdictLine(r)).toBe( + 'Verdict: Comment — a Request changes was NOT available: its blockers ' + + 'were never verified (they are posted, disclosed as unverified)', + ); + }); + + it('keeps the body Criticals when the unverified cap softens the event — the only copy survives', () => { + // The presubmit RC→Comment carve-out learned this the hard way: a softened + // event must never erase the body copy of an unanchorable blocker. + const r = composeReview({ + criticalsInline: 0, + bodyCriticals: ['whole-PR blocker X'], + planPath: coveredPlan(['reverse-audit']), // verifier absent + env: ENV, + modelId: MODEL, + }); + expect(r.event).toBe('COMMENT'); + expect(r.cappedBy).toContain('criticals-unverified'); + expect(r.body).toContain('**[Critical]** whole-PR blocker X'); + }); + + it('a deterministic-only Request changes stands without a verifier — pre-confirmed by design', () => { + // [build]/[test] findings are deterministic: CI ran them, nothing a + // verifier rules on. A review whose only blocker is one must not be + // softened for skipping a verification it never owed. + const r = composeReview({ + criticalsInline: 0, + bodyCriticals: ['[build] tsc fails on main merge'], + planPath: coveredPlan(['reverse-audit']), // verifier absent, none owed + env: ENV, + modelId: MODEL, + }); + expect(r.event).toBe('REQUEST_CHANGES'); + expect(r.cappedBy).not.toContain('criticals-unverified'); + }); + + it('a verified Request changes still blocks — the cap binds only when Step 4 is missing', () => { + const r = composeReview({ + criticalsInline: 1, + planPath: coveredPlan(), // verify AND reverse audit ran + env: ENV, + modelId: MODEL, + }); + expect(r.event).toBe('REQUEST_CHANGES'); + expect(r.cappedBy).not.toContain('criticals-unverified'); + }); + + it('fails closed when there is no plan to check verification against', () => { + // "Could not show the blockers were verified" and "they were not" read + // the same to the person the blocker would be posted at. + const r = composeReview({ + criticalsInline: 1, + modelId: MODEL, + }); + expect(r.event).toBe('COMMENT'); + expect(r.cappedBy).toContain('criticals-unverified'); + }); + + it('fails closed when the transcripts cannot be read at all', () => { + const r = composeReview({ + criticalsInline: 1, + planPath: coveredPlan(), + env: { + QWEN_CODE_PROJECT_DIR: join(dir, 'nowhere'), + QWEN_CODE_SESSION_ID: 'S1', + }, + modelId: MODEL, + }); + expect(r.event).toBe('COMMENT'); + expect(r.cappedBy).toContain('criticals-unverified'); }); it('does not require a verifier on a review that confirmed nothing', () => { @@ -1536,16 +1630,21 @@ describe('the Step 4/5 gate — verify and reverse audit must have run (high eff 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. + // body) and still had to be verified — so a missing verifier is disclosed, + // the event is softened (an unverified finding must not become a public + // blocker), and the body copy survives the softening. 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.event).toBe('COMMENT'); + expect(r.cappedBy).toContain('criticals-unverified'); expect(r.body).toMatch(/verification — the review posts findings/); + expect(r.body).toContain( + '**[Critical]** a real blocker that could not be anchored', + ); }); it('does not require a verifier for a deterministic [build]/[test] body Critical', () => { diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 92479da368c..cfa380c8fff 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -232,6 +232,23 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { // the author a false cause. const missingReceipts: number[] = []; + // The Criticals a verifier must have ruled on before this review may post + // them as blockers. Deterministic `[build]`/`[test]` body findings are + // pre-confirmed and skip verification by design; every other Critical — + // anchored or body — is a claim, and a claim is confirmed by Step 4 or it + // is not confirmed at all. + const nonDeterministicBodyCriticals = bodyCriticals.filter( + (x) => !/\[(?:build|test)\]/i.test(x), + ).length; + const criticalsNeedingVerify = + criticalsInline + nonDeterministicBodyCriticals; + // Fail closed at every exit: this flag softens a Request changes below, and + // it must end up true whenever the review posts non-deterministic Criticals + // and CANNOT SHOW they were verified — verifier absent, transcripts + // unreadable, or no plan to check against. "Could not show" and "was not" + // read the same to the person the blocker would be posted at. + let criticalsUnverified = false; + // Coverage is NOT taken from the input. It is recomputed here, from the // harness's own per-agent transcripts. // @@ -251,6 +268,7 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { 'no plan was given, so this run cannot show that any of the diff ' + 'was read', }); + criticalsUnverified = criticalsNeedingVerify >= 1; } else { try { const cov = coverageFromTranscripts(input.planPath, input.env); @@ -395,9 +413,7 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { // 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; + criticalsInline + suggestionsInline + nonDeterministicBodyCriticals; const verification = verificationGaps( input.planPath, { postsFindings: findingsToVerify > 0 }, @@ -417,6 +433,8 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { ); } remediation.push(...verification.remediation); + criticalsUnverified = + verification.unverifiedFindings && criticalsNeedingVerify >= 1; } catch (err) { coverageEntries.push({ subject: 'verification', @@ -424,6 +442,9 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { `could not check that Step 4 and Step 5 ran ` + `(${(err as Error).message})`, }); + // Fail closed: a verification that cannot be CHECKED is not a + // verification that happened. + criticalsUnverified = criticalsNeedingVerify >= 1; } } const contextUnavailable = toBool( @@ -478,9 +499,24 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { cappedBy.push('unreviewed-dimension'); } if (contextUnavailable) cappedBy.push('context-unavailable'); + if (criticalsUnverified) cappedBy.push('criticals-unverified'); let event: ReviewEvent = baseEvent; if (event === 'APPROVE' && cappedBy.length > 0) event = 'COMMENT'; + // The ONE cap that reaches a Request changes — because it removes the + // premise the never-soften rule stands on. "A REQUEST_CHANGES earned by a + // confirmed Critical is never softened" presumes CONFIRMED, and this flag + // is precisely the statement that no verifier ever ruled on the blockers. + // The header's own principle — an unverified finding must not become a + // public blocker (the false "leaks tokens" Critical is the exact harm) — + // was mechanics for the Approve row only, and a real bot review shipped + // through the gap: a CHANGES_REQUESTED on an external contributor's PR + // (#7166) whose one Critical the body itself disclosed as unverified. + // The findings still post, disclosed; the review just may not BLOCK on a + // claim nobody confirmed. Manipulation check: a run that wants an Approve + // gains nothing here (the same flag caps Approve via `unreviewed`), and a + // run that wants to block without verifying now cannot. + if (event === 'REQUEST_CHANGES' && criticalsUnverified) event = 'COMMENT'; // Presubmit downgrades apply after the caps and only when the verdict // they name is the one on the table. @@ -707,10 +743,11 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { // 6. Not-reviewed disclosure. clauses.push(...notReviewedParts); - // 7. Body Criticals — only on a COMMENT downgraded from REQUEST_CHANGES - // (the carve-out); on a plain COMMENT there is no RC to have carried - // them. - if (downgradedFrom === 'Request changes') { + // 7. Body Criticals — on a COMMENT that stands where a REQUEST_CHANGES + // would have been: the presubmit carve-out, and the unverified-blockers + // cap. Either way the body copy is the ONLY copy of an unanchorable + // blocker, and softening the event must never erase it. + if (downgradedFrom === 'Request changes' || criticalsUnverified) { clauses.push(...bodyCriticalBlock); } @@ -905,11 +942,23 @@ export function verdictLine(r: ComposeReviewResult): string { // a dangling colon over nothing. Collect the reasons first, and say the clause // only if there is a reason to say it. // - // A cap never softens a Request changes — a confirmed blocker earned that, and - // naming a constraint that did not bind would send the reader looking for an - // effect that is not there — so this clause is gated on the base having been an - // Approve at all. - if (r.baseEvent === 'APPROVE' && r.event !== 'APPROVE') { + // A coverage cap never softens a Request changes — a confirmed blocker earned + // that, and naming a constraint that did not bind would send the reader + // looking for an effect that is not there — so the Approve clause is gated on + // the base having been an Approve at all. The unverified-blockers cap is the + // one exception, because it says the confirmation never happened, and its + // sentence must name what the reader would otherwise chase: a Comment posted + // over visible **[Critical]** comments reads as a contradiction until the + // line says why. + if ( + r.baseEvent === 'REQUEST_CHANGES' && + r.event === 'COMMENT' && + r.cappedBy.includes('criticals-unverified') + ) { + line += + ' — a Request changes was NOT available: its blockers were never ' + + 'verified (they are posted, disclosed as unverified)'; + } else if (r.baseEvent === 'APPROVE' && r.event !== 'APPROVE') { const reasons = r.cappedBy.map((c) => why[c] ?? c); if (r.downgraded) reasons.push('a presubmit check failed'); line += ` — an Approve was NOT available: ${reasons.join('; ')}`; diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts index 2c70549d40d..0a21d91f378 100644 --- a/packages/cli/src/commands/review/lib/coverage.ts +++ b/packages/cli/src/commands/review/lib/coverage.ts @@ -833,6 +833,15 @@ export interface VerificationReport { * orchestrator reads. Never rendered into the body. */ remediation: string[]; + /** + * True when this review posts findings and NO verifier's delivery came back + * clean — the structured form of the `verification — …` gap line, for the + * verdict computation. A Request changes is "earned by a confirmed + * Critical", and this is the bit that says the confirmation never happened; + * parsing the gap text for it would put the verdict at the mercy of a + * wording change. + */ + unverifiedFindings: boolean; } /** @@ -950,6 +959,7 @@ export function verificationGaps( // 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. + let unverifiedFindings = false; if (opts.postsFindings) { // The whole key family: `verify--` per shard (the record now folds // the findings in, so a launch that dropped them matches nothing), plus the @@ -959,6 +969,7 @@ export function verificationGaps( ); const verify = bestDelivery(verifyKeys); if (verify !== 'ok') { + unverifiedFindings = true; gaps.push(`verification — ${VERIFY_GAP[verify].gap}`); remediation.push( `verification: ${VERIFY_GAP[verify].fix.replace( @@ -971,7 +982,7 @@ export function verificationGaps( } } - return { ok: gaps.length === 0, gaps, remediation }; + return { ok: gaps.length === 0, gaps, remediation, unverifiedFindings }; } export { TranscriptsUnavailableError }; diff --git a/packages/cli/src/commands/review/submit.test.ts b/packages/cli/src/commands/review/submit.test.ts index c4146455f05..d69c3892ab6 100644 --- a/packages/cli/src/commands/review/submit.test.ts +++ b/packages/cli/src/commands/review/submit.test.ts @@ -10,9 +10,16 @@ // says otherwise. import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { + mkdtempSync, + mkdirSync, + rmSync, + utimesSync, + writeFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import { promptRecordDir, briefPath } from './lib/prompt-record.js'; const ghMock = vi.hoisted(() => vi.fn((_payload: string, ..._rest: string[]) => ''), @@ -250,6 +257,105 @@ describe('payload consistency — refuse before GitHub sees it', () => { /** What was actually sent to GitHub. */ const posted = () => JSON.parse(ghMock.mock.calls[0][0] as string); + /** + * A plan whose Step 4 verification is provably delivered — recorded prompt, + * brief, and a transcript that ran it verbatim and opened the brief. + * + * The tests below post Criticals, and a Critical nobody verified no longer + * blocks: composeReview softens the Request changes and says so. These + * tests are about OTHER properties of a blocking submission (count + * derivation, body escaping, unanchorable carriage), so they carry the + * verification that keeps the Request changes standing. + */ + function verifiedPlan(): string { + const diffPath = join(dir, 'verified-diff.txt'); + writeFileSync(diffPath, 'diff'); + const plan = join(dir, 'verified-plan.json'); + writeFileSync( + plan, + JSON.stringify({ + diffPathAbsolute: diffPath, + srcDiffLines: 10, + diffLines: 10, + files: [], + chunks: [{ id: 1, startLine: 1, endLine: 1 }], + }), + ); + const d = promptRecordDir(plan); + mkdirSync(d, { recursive: true }); + const brief = briefPath(plan, 'verify'); + writeFileSync(brief, 'The verify brief.'); + const launch = + `You are review agent \`verify\`.\n` + `read_file(file_path="${brief}")`; + writeFileSync(join(d, 'verify.txt'), launch); + // Transcripts newer than the plan, as in a real run. + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + const sub = join(dir, 'subagents', 'SUBV'); + mkdirSync(sub, { recursive: true }); + const base = { + agentId: 'v1', + agentName: 'general-purpose', + sessionId: 'SUBV', + }; + writeFileSync( + join(sub, 'agent-v1.jsonl'), + [ + { + ...base, + type: 'user', + message: { role: 'user', parts: [{ text: launch }] }, + }, + { + ...base, + type: 'assistant', + message: { + role: 'model', + parts: [ + { + functionCall: { name: 'read_file', args: { file_path: brief } }, + }, + ], + }, + }, + { + ...base, + type: 'tool_result', + message: { + role: 'user', + parts: [ + { + functionResponse: { + name: 'read_file', + response: { output: 'ok' }, + }, + }, + ], + }, + }, + ] + .map((x) => JSON.stringify(x)) + .join('\n') + '\n', + ); + return plan; + } + + /** Run with the transcript env the stripped-`env` compose path reads. */ + function withVerifyEnv(fn: () => void): void { + const prevDir = process.env['QWEN_CODE_PROJECT_DIR']; + const prevSession = process.env['QWEN_CODE_SESSION_ID']; + process.env['QWEN_CODE_PROJECT_DIR'] = dir; + process.env['QWEN_CODE_SESSION_ID'] = 'SUBV'; + try { + fn(); + } finally { + if (prevDir === undefined) delete process.env['QWEN_CODE_PROJECT_DIR']; + else process.env['QWEN_CODE_PROJECT_DIR'] = prevDir; + if (prevSession === undefined) delete process.env['QWEN_CODE_SESSION_ID']; + else process.env['QWEN_CODE_SESSION_ID'] = prevSession; + } + } + it("refuses a payload that carries a verdict — that is not the caller's to write", () => { // The failure this replaces. Dogfooded, a run read the coverage check's // refusal, decided "the agents clearly did their job", skipped @@ -286,16 +392,18 @@ describe('payload consistency — refuse before GitHub sees it', () => { it('counts the blockers it is actually carrying, not the ones it was told about', () => { // A Critical attached inline is a Critical, whatever the state says. There is // no `criticalsInline` field to under-report it with — and one supplied - // anyway is refused. + // anyway is refused. Verification is on record, so the Request changes the + // count earns actually stands. const review = file('c1.json', { ...REVIEW, + state: { ...REVIEW.state, planPath: verifiedPlan() }, comments: [ { path: 'a.ts', line: 12, body: '**[Critical]** boom' }, { path: 'b.ts', line: 3, body: '**[Suggestion]** tidy' }, ], }); - runSubmit(authorized({ review })); + withVerifyEnv(() => runSubmit(authorized({ review }))); expect(posted().event).toBe('REQUEST_CHANGES'); }); @@ -342,6 +450,7 @@ describe('payload consistency — refuse before GitHub sees it', () => { ...REVIEW, state: { ...REVIEW.state, + planPath: verifiedPlan(), bodyCriticals: [ 'the splitter uses `/\\n/` where the input is CRLF, so every line ' + 'keeps a trailing `\\r`', @@ -349,7 +458,7 @@ describe('payload consistency — refuse before GitHub sees it', () => { }, }); - runSubmit(authorized({ review })); + withVerifyEnv(() => runSubmit(authorized({ review }))); expect(posted().event).toBe('REQUEST_CHANGES'); expect(posted().body).toContain('`/\\n/`'); // Real newlines, not the two characters. @@ -424,12 +533,13 @@ describe('payload consistency — refuse before GitHub sees it', () => { ...REVIEW, state: { ...REVIEW.state, + planPath: verifiedPlan(), bodyCriticals: ['the inline cache is stale after a rebase'], }, comments: [], }); - runSubmit(authorized({ review })); + withVerifyEnv(() => runSubmit(authorized({ review }))); expect(ghMock).toHaveBeenCalledOnce(); const sent = JSON.parse(ghMock.mock.calls[0][0] as string); expect(sent.event).toBe('REQUEST_CHANGES'); diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 836e772353d..4391a508c89 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -642,8 +642,8 @@ The rules it applies — so you can read the line it gives you, not so you can a - 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, 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. +- **Request changes** — one or more high-confidence Criticals, anchored or in the body, **whose verification is on record** (a deterministic `[build]`/`[test]` finding is pre-confirmed and needs none). +- **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**, 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. **Or a Request changes whose blockers were never verified**: the findings still post, disclosed as unverified, but an unverified finding must not become a public blocker — a run whose verifier never launched posted a CHANGES_REQUESTED onto an external contributor's PR over a Critical its own body disclosed as unverified, and this row is what stops the next one. **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. From 70e86604cc40cba2ef1bd9757e5f35a92e5fcdd8 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 19 Jul 2026 11:36:15 +0800 Subject: [PATCH 2/3] fix(review): keep presubmit reasons beside the unverified cap, sync the Step 7 restatement Review follow-through: the Step 7 skill text still said a cap never softens a REQUEST_CHANGES -- the exact one-restatement-behind drift the module header warns about; it now carries the coverage qualifier and the one exception. When the unverified cap and presubmit downgradeRequestChanges both held, the softening ran first and the presubmit reasons silently vanished from the body; the downgrade arm now also fires on a softened Request-changes base, so the body's downgrade clause carries the reasons while the verdict line keeps the unverified sentence. Plus a pin that verify on record with the reverse audit absent still blocks -- the softening gates on verify alone. --- .../commands/review/compose-review.test.ts | 32 +++++++++++++++++++ .../cli/src/commands/review/compose-review.ts | 11 ++++++- .../core/src/skills/bundled/review/SKILL.md | 2 +- 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 97a29e70cc2..bb718913763 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -1538,6 +1538,38 @@ describe('the Step 4/5 gate — verify and reverse audit must have run (high eff ); }); + it('keeps the presubmit downgrade reasons when the unverified cap also holds', () => { + // The softening runs first, so without the widened downgrade arm the + // presubmit reasons silently vanished whenever both held. Verdict keeps + // the unverified sentence; the body downgrade clause carries the reasons. + const r = composeReview({ + criticalsInline: 1, + planPath: coveredPlan(['reverse-audit']), + env: ENV, + presubmit: { + downgradeRequestChanges: true, + downgradeReasons: ['self-PR'], + }, + modelId: MODEL, + }); + expect(r.event).toBe('COMMENT'); + expect(r.body).toContain( + 'Downgraded from Request changes to Comment: self-PR', + ); + expect(verdictLine(r)).toContain('its blockers were never verified'); + }); + + it('verify on record with the reverse audit absent still blocks — softening gates on verify alone', () => { + const r = composeReview({ + criticalsInline: 1, + planPath: coveredPlan(['verify']), + env: ENV, + modelId: MODEL, + }); + expect(r.event).toBe('REQUEST_CHANGES'); + expect(r.cappedBy).not.toContain('criticals-unverified'); + }); + it('keeps the body Criticals when the unverified cap softens the event — the only copy survives', () => { // The presubmit RC→Comment carve-out learned this the hard way: a softened // event must never erase the body copy of an unanchorable blocker. diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index cfa380c8fff..b6b2e843884 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -526,7 +526,16 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { event = 'COMMENT'; downgraded = true; downgradedFrom = 'Approve'; - } else if (event === 'REQUEST_CHANGES' && downgradeRequestChanges) { + } else if ( + (event === 'REQUEST_CHANGES' || + (baseEvent === 'REQUEST_CHANGES' && criticalsUnverified)) && + downgradeRequestChanges + ) { + // The unverified-blockers cap softened the event first, but the presubmit + // still ruled: without this arm its reasons (self-PR, failing CI) would + // silently vanish from the body whenever both held. The verdict line + // keeps the unverified sentence — the more fundamental defect — and the + // body's downgrade clause carries the presubmit reasons. event = 'COMMENT'; downgraded = true; downgradedFrom = 'Request changes'; diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 4391a508c89..91e6c1ec233 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -823,7 +823,7 @@ Rationale: an inline comment is the only place GitHub renders a ` ```suggestion - `presubmit` — `downgradeApprove` / `downgradeRequestChanges` / `downgradeReasons` from the presubmit report. Do not apply a downgrade by hand; hand it over and let `submit` own the semantics (a Suggestion-only review is already `COMMENT`, so nothing is downgraded and no "downgraded from Approve" sentence is emitted). - `modelId` — for the footer. -The verdict is a computed fact and this is the second place it must not be re-derived: Step 6 printed it from this same `state`, and `submit` will post it from this same `state`. What the machine guarantees (its tests pin all of it): `REQUEST_CHANGES` whenever any Critical is confirmed, inline or body-only; `COMMENT` for a Suggestion-only run and for every capped or downgraded outcome; `APPROVE` only for a clean, uncapped, undowngraded, zero-finding run whose coverage the transcripts confirm. A cap state forbids `APPROVE` but never softens a `REQUEST_CHANGES`; body Criticals count toward `C`; the "no blockers" opener appears only when the review can certify it. Two live failures this replaces: a review that filed three Suggestions and then publicly `APPROVE`d the PR (#6584), and a Suggestion that would not anchor becoming a second paragraph of the public body (#6631) — both impossible now, because the caller no longer writes the event or the body. +The verdict is a computed fact and this is the second place it must not be re-derived: Step 6 printed it from this same `state`, and `submit` will post it from this same `state`. What the machine guarantees (its tests pin all of it): `REQUEST_CHANGES` whenever any Critical is confirmed, inline or body-only; `COMMENT` for a Suggestion-only run and for every capped or downgraded outcome; `APPROVE` only for a clean, uncapped, undowngraded, zero-finding run whose coverage the transcripts confirm. A **coverage** cap forbids `APPROVE` but never softens a `REQUEST_CHANGES`; the one exception is the unverified-blockers cap, which softens it to `COMMENT` (findings still posted, disclosed as unverified); body Criticals count toward `C`; the "no blockers" opener appears only when the review can certify it. Two live failures this replaces: a review that filed three Suggestions and then publicly `APPROVE`d the PR (#6584), and a Suggestion that would not anchor becoming a second paragraph of the public body (#6631) — both impossible now, because the caller no longer writes the event or the body. - `comments`: high-confidence **Critical and Suggestion** findings. Skip Nice to have and low-confidence. Each must reference a line in the diff — the `line` `resolve-anchors` computed, never one you derived. - **Multi-line anchors get a `start_line` — and both `side` fields with it.** When a finding's resolution has `startLine !== line`, GitHub can highlight the whole construct instead of just its last line — the `if` and its condition, the three lines of a broken guard — which is something a bare line number could not express, and it is free: the resolver already computed both ends. But GitHub requires **`side` and `start_side` on any multi-line comment**, and rejects the whole review with a 422 without them. Emit all four together, or none: From 71a9498424746a9f2bc5500e746fa442e5412b06 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 19 Jul 2026 14:03:46 +0800 Subject: [PATCH 3/3] fix(review): a deterministic Critical keeps its Request changes beside an unverified sibling Review catch: the unverified-blockers softening dropped the whole REQUEST_CHANGES even when the review also carried a pre-confirmed [build]/[test] body Critical -- un-blocking a confirmed build failure because its sibling lacked a verifier. The softening now requires zero deterministic body Criticals; the unverified ones stay disclosed either way, and the mixed case is pinned. --- .../commands/review/compose-review.test.ts | 19 +++++++++++++++++++ .../cli/src/commands/review/compose-review.ts | 15 ++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index bb718913763..e33f95b2aa4 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -1585,6 +1585,25 @@ describe('the Step 4/5 gate — verify and reverse audit must have run (high eff expect(r.body).toContain('**[Critical]** whole-PR blocker X'); }); + it('a mixed review keeps its Request changes — the deterministic blocker is confirmed with or without a verifier', () => { + // One [build] Critical (pre-confirmed) beside one non-deterministic + // Critical with the verifier absent: softening the whole event would + // un-block a confirmed build failure. The unverified sibling stays + // disclosed; the Request changes stands on the deterministic one. + const r = composeReview({ + bodyCriticals: [ + '[build] tsc fails on the merge commit', + '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.cappedBy).toContain('criticals-unverified'); + expect(r.body).toMatch(/verification — the review posts findings/); + }); + it('a deterministic-only Request changes stands without a verifier — pre-confirmed by design', () => { // [build]/[test] findings are deterministic: CI ran them, nothing a // verifier rules on. A review whose only blocker is one must not be diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index b6b2e843884..1ced52d693b 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -516,7 +516,20 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { // claim nobody confirmed. Manipulation check: a run that wants an Approve // gains nothing here (the same flag caps Approve via `unreviewed`), and a // run that wants to block without verifying now cannot. - if (event === 'REQUEST_CHANGES' && criticalsUnverified) event = 'COMMENT'; + // …unless a DETERMINISTIC Critical also rides the review: a `[build]`/ + // `[test]` finding is pre-confirmed, its Request changes is earned with or + // without a verifier, and softening it alongside its unverified sibling + // would un-block a confirmed build failure. The unverified ones stay + // disclosed either way. + const deterministicBodyCriticals = + bodyCriticals.length - nonDeterministicBodyCriticals; + if ( + event === 'REQUEST_CHANGES' && + criticalsUnverified && + deterministicBodyCriticals === 0 + ) { + event = 'COMMENT'; + } // Presubmit downgrades apply after the caps and only when the verdict // they name is the one on the table.