diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index f9e3de7b3b8..e33f95b2aa4 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,10 +1524,137 @@ describe('the Step 4/5 gate — verify and reverse audit must have run (high eff env: ENV, modelId: MODEL, }); + 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 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. + 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 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 + // 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', () => { // C=0, S=0: nothing to verify. The reverse audit ran, so this approves. const r = composeReview({ @@ -1536,16 +1681,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..1ced52d693b 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,37 @@ 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. + // …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. @@ -490,7 +539,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'; @@ -707,10 +765,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 +964,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..91e6c1ec233 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. @@ -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: