fix(review): an unverified Critical must not become a public blocker — soften the Request changes it rides

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,20 +1509,152 @@ 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,
       planPath: coveredPlan(['reverse-audit']), // verifier absent
       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', () => {

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,13 +433,18 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult {
         );
       }
       remediation.push(...verification.remediation);
+      criticalsUnverified =
+        verification.unverifiedFindings && criticalsNeedingVerify >= 1;
     } catch (err) {
       coverageEntries.push({
         subject: 'verification',
         reason:
           `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('; ')}`;