fix(review): keep the blocker in a COMMENT body every softening path reaches

packages/cli/src/commands/review/compose-review.test.ts

@@ -478,6 +478,17 @@ function blindPlan(): string {
   return plan();
 }
 
+function findingsFile(content: string): string {
+  const f = join(dir, 'qwen-review-findings.md');
+  writeFileSync(f, content);
+  return f;
+}
+
+const TAGGED =
+  '- **File:** src/pay.ts:42\n' +
+  '- **Issue:** off-by-one in the retry cap\n' +
+  '- **Severity:** Critical — [unverified]\n';
+
 const FOOTER = `_— ${MODEL} via Qwen Code /review (vunknown)_`;
 
 function base(overrides: Partial<ComposeReviewInput>): ComposeReviewInput {
@@ -1553,6 +1564,28 @@ describe('composeReview — presubmit downgrades', () => {
     );
   });
 
+  it('keeps the presubmit downgrade reasons when a cap softens the Approve first', () => {
+    // The APPROVE→COMMENT cap runs before the presubmit arms, so a capped
+    // zero-finding Approve reached arm 1 as COMMENT — its `event === 'APPROVE'`
+    // gate failed, arm 2's REQUEST_CHANGES gate failed too, and the presubmit
+    // reasons vanished from the body and the verdict line while the identical
+    // uncapped run rendered both. The gate is derived from `baseEvent` — the
+    // row before every cap — exactly like its RC sibling.
+    const r = composeReview(
+      base({
+        contextUnavailable: true,
+        presubmit: { downgradeApprove: true, downgradeReasons: ['self-PR'] },
+      }),
+    );
+    expect(r.baseEvent).toBe('APPROVE');
+    expect(r.event).toBe('COMMENT');
+    expect(r.cappedBy).toContain('context-unavailable');
+    expect(r.downgraded).toBe(true);
+    expect(r.downgradedFrom).toBe('Approve');
+    expect(r.body).toContain('⚠️ Downgraded from Approve to Comment: self-PR.');
+    expect(verdictLine(r)).toContain('a presubmit check failed');
+  });
+
   it('a downgraded Approve never certifies "no blockers" in the same body (the downgrade names failing CI two clauses earlier)', () => {
     const r = composeReview(
       base({
@@ -3548,6 +3581,32 @@ describe('the Step 4/5 gate — verify and reverse audit must have run (high eff
     expect(verdictLine(r)).toContain('its blockers were never verified');
   });
 
+  it('keeps the presubmit downgrade reasons when the findings-tag cap also holds', () => {
+    // The tag cap softens the event while setting NEITHER legacy flag, so the
+    // recovery arm's enumeration missed it: the presubmit reasons vanished
+    // from the body — the silent loss the arm's own comment forbids. Verdict
+    // keeps the tag sentence; the body's downgrade clause carries the reasons.
+    const r = composeReview({
+      criticalsInline: 1,
+      planPath: coveredPlan(['verify', 'reverse-audit']),
+      env: ENV,
+      findingsPath: findingsFile(TAGGED),
+      presubmit: {
+        downgradeRequestChanges: true,
+        downgradeReasons: ['self-PR'],
+      },
+      modelId: MODEL,
+    });
+    expect(r.event).toBe('COMMENT');
+    expect(r.cappedBy).toContain('findings-unverified-at-compose');
+    expect(r.body).toContain(
+      'Downgraded from Request changes to Comment: self-PR',
+    );
+    expect(verdictLine(r)).toContain(
+      'findings were still unverified when the loop ended',
+    );
+  });
+
   it('verify on record with the reverse audit absent still blocks — softening gates on verify alone', () => {
     const r = composeReview({
       criticalsInline: 1,
@@ -3574,6 +3633,31 @@ 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('keeps the body Criticals when the FINDINGS-TAG cap softens the event', () => {
+    // The third softening path, and the one the enumerated condition missed:
+    // coverage is proven and the verifier ran, so `criticalsUnverified` is
+    // false and no presubmit downgrade fired — the cap comes from the
+    // findings file still carrying `— [unverified]` at compose time. The
+    // posted body was 239 characters of opener and disclosure with the
+    // blocker — its only copy — nowhere in it.
+    const r = composeReview({
+      planPath: coveredPlan(['verify', 'reverse-audit']),
+      env: ENV,
+      modelId: MODEL,
+      criticalsInline: 0,
+      suggestionsInline: 0,
+      bodyCriticals: ['whole-PR blocker X'],
+      findingsPath: findingsFile(TAGGED),
+    });
+    expect(r.baseEvent).toBe('REQUEST_CHANGES');
+    expect(r.event).toBe('COMMENT');
+    // Neither flag the old condition listed is set on this path.
+    expect(r.downgradedFrom).toBeNull();
+    expect(r.cappedBy).toContain('findings-unverified-at-compose');
+    expect(r.cappedBy).not.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
@@ -6701,9 +6785,9 @@ describe("composeReview — the composed body fits GitHub's limit", () => {
     // that qualify the verdict before it spent a single blocker.
     //
     // The cap here is the absent verifier, which still POSTS the blockers
-    // (clause 7 rides on `criticalsUnverified`); the findings-file tag route
-    // caps without that flag, so its body carries no blocker and cannot
-    // reach the cut at all.
+    // (clause 7 rides on the softened RC→COMMENT transition); the
+    // findings-file tag route softens the same way, so its body carries the
+    // blocker too and can reach the cut.
     const r = composeReview({
       planPath: coveredPlan(['reverse-audit']),
       env: ENV,
@@ -6959,16 +7043,6 @@ describe('composeReview — the findings file tag check', () => {
   // round's findings digest — so compose-review reads the cumulative
   // findings file itself and caps on any surviving tag.
 
-  function findingsFile(content: string): string {
-    const f = join(dir, 'qwen-review-findings.md');
-    writeFileSync(f, content);
-    return f;
-  }
-
-  const TAGGED =
-    '- **File:** src/pay.ts:42\n' +
-    '- **Issue:** off-by-one in the retry cap\n' +
-    '- **Severity:** Critical — [unverified]\n';
   const CLEAN =
     '- **File:** src/pay.ts:42\n' +
     '- **Issue:** off-by-one in the retry cap\n' +

packages/cli/src/commands/review/compose-review.ts

@@ -2143,20 +2143,24 @@ function composeReviewBody(
     event = 'COMMENT';
   }
 
-  // Presubmit downgrades apply after the caps and only when the verdict
-  // they name is the one on the table.
+  // Presubmit downgrades apply after the caps and only when the verdict they
+  // name was the one on the table — `baseEvent` is the row before every cap,
+  // so a softening cap that ran first cannot erase the presubmit's reasons.
   let downgraded = false;
   let downgradedFrom: 'Approve' | 'Request changes' | null = null;
-  if (event === 'APPROVE' && downgradeApprove) {
+  if (
+    (event === 'APPROVE' || (baseEvent === 'APPROVE' && event === 'COMMENT')) &&
+    downgradeApprove
+  ) {
     event = 'COMMENT';
     downgraded = true;
     downgradedFrom = 'Approve';
   } else if (
     (event === 'REQUEST_CHANGES' ||
-      (baseEvent === 'REQUEST_CHANGES' && criticalsUnverified)) &&
+      (baseEvent === 'REQUEST_CHANGES' && event === 'COMMENT')) &&
     downgradeRequestChanges
   ) {
-    // The unverified-blockers cap softened the event first, but the presubmit
+    // A softening cap moved 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
@@ -3289,10 +3293,20 @@ function composeReviewBody(
   clauses.push(...continuityBlock);
 
   // 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
+  //    would have been. The body copy is the ONLY copy of an unanchorable
   //    blocker, and softening the event must never erase it.
-  if (downgradedFrom === 'Request changes' || criticalsUnverified) {
+  //
+  //    DERIVED, not enumerated. The condition was a list of the two
+  //    softening flags known when it was written — the presubmit carve-out
+  //    and `criticalsUnverified` — and a third path shipped past it: the
+  //    findings-file `— [unverified]` tag softens a Request changes at the
+  //    event line above while setting NEITHER flag, so a run whose coverage
+  //    was proven posted a 239-character body carrying the opener and the
+  //    tag disclosure and no blocker at all. `baseEvent` is the row before
+  //    every cap and downgrade, so this comparison asks the question the
+  //    clause is actually about, and answers it for softening paths that do
+  //    not exist yet.
+  if (baseEvent === 'REQUEST_CHANGES' && event === 'COMMENT') {
     clauses.push(...bodyCriticalBlock);
   }