From 73da98c7c1cd53afc9f79866ae73e9bebc01d2e9 Mon Sep 17 00:00:00 2001
From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Date: Fri, 23 Oct 2026 21:25:51 -0700
Subject: [PATCH] honor not-ready review verdicts with advisory findings

The classifier now honors the last recognized not-ready verdict
regardless of parsed severity, returning findings and exit code 3 while
retaining the highest severity in the summary. Advisory findings with a
ready verdict or no verdict keep their existing behavior. Regression
coverage and troubleshooting documentation describe this behavior across
the execution, structured, and review gates.

Fixes #3329
---
 docs/troubleshooting.md                       |  6 +-
 lib/outside-review-result.ts                  |  3 +-
 test/outside-review-unparsed-severity.test.ts | 76 ++++++++++++++++++-
 3 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md
index 027d071eb..b509350c0 100644
--- a/docs/troubleshooting.md
+++ b/docs/troubleshooting.md
@@ -170,8 +170,10 @@ the readiness dashboard and the PR body, and never count it as a pass.
   findings.
 - **Every review gate.** These rules apply to every gate that summarises a
   review, including the `execution` gate behind `/codex` consult and challenge.
-  A reply whose last `Verdict:` line says `not ready` but carries no readable
-  severity prints `FINDINGS: unspecified` and exits 3 like a blocking finding.
+  A reply whose last `Verdict:` line says `not ready` is `findings` and exits
+  3 even when readable advisory severities (P2 or P3) were parsed; `FINDINGS:`
+  names the highest one. A not-ready reply with no readable severity still
+  prints `FINDINGS: unspecified` and exits 3.
 - **No findings.** A review with no tag and no label is `clean` only when its
   conclusion says so: the last sentence (or its last ", and" clause), or the
   `because` clause of its `Recommendation:` line, opens with `NO_FINDINGS`,
diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts
index 8409c73b2..ad76233ab 100644
--- a/lib/outside-review-result.ts
+++ b/lib/outside-review-result.ts
@@ -263,7 +263,8 @@ export function classifyOutsideReview(input: OutsideReviewInput): OutsideReviewC
   }
   if (!reviewed) return result(blocking ? 'findings' : 'clean');
   const unparsed = firstUnparsed(code);
-  const notReady = !findings.highest && verdictNotReady(plainCode);
+  // A last explicit not-ready blocks even when the only parsed severities are advisory.
+  const notReady = verdictNotReady(plainCode);
   if (blocking || notReady) return { ...result('findings'), ...(unparsed ? { unparsed } : {}) };
   if (unparsed) return { ...result('unverified', 'unparsed_severity', unparsed), unparsed };
   if ((input.gate === 'structured' || input.gate === 'review') && !findings.highest && !concludesNoFindings(code)) {
diff --git a/test/outside-review-unparsed-severity.test.ts b/test/outside-review-unparsed-severity.test.ts
index bca28a2cc..82e7d0f1a 100644
--- a/test/outside-review-unparsed-severity.test.ts
+++ b/test/outside-review-unparsed-severity.test.ts
@@ -5,12 +5,14 @@
  * FINDINGS: none` for all of them. The fixtures in
  * test/fixtures/outside-review-severity/codex-3105/ are synthesized from the
  * forms the issue documents (it describes 13 replies but does not include them).
+ * #3329: that same gate then printed `clean` for a parsed advisory finding
+ * whose last `Verdict:` line says `not ready`. `bare-dash-p2.txt` is unchanged.
  */
 import { describe, expect, test } from 'bun:test';
 import { spawnSync } from 'node:child_process';
 import * as fs from 'node:fs';
 import * as path from 'node:path';
-import { classifyOutsideReview, type OutsideGate } from '../lib/outside-review-result';
+import { classifyOutsideReview, validateOutsideReview, type OutsideGate } from '../lib/outside-review-result';
 import { GATE_OUTCOMES } from '../lib/gate-outcomes';
 
 const ROOT = path.resolve(import.meta.dir, '..');
@@ -26,7 +28,7 @@ describe('#3105 Codex reply shapes under the execution gate', () => {
   for (const [name, status, stdout] of [
     ['plan-bold-dash.txt', 3, 'VERDICT: findings\nFINDINGS: P1\n'],
     ['diff-numbered-dash.txt', 3, 'VERDICT: findings\nFINDINGS: P1\n'],
-    ['bare-dash-p2.txt', 0, 'VERDICT: clean\nFINDINGS: P2\n'],
+    ['bare-dash-p2.txt', 3, 'VERDICT: findings\nFINDINGS: P2\n'],
     ['ready-one-p3.txt', 0, 'VERDICT: clean\nFINDINGS: P3\n'],
     ['table-p1.txt', 3, 'VERDICT: findings\nFINDINGS: P1\n'],
     ['not-ready-unlabeled.txt', 3, 'VERDICT: findings\nFINDINGS: unspecified\n'],
@@ -112,3 +114,73 @@ describe('#3105 severity forms are read by every gate that summarises a review',
     expect(classifyOutsideReview({ text: proposal, gate: 'proposal' }).verdict).toBe('clean');
   });
 });
+
+describe('#3329 a last not-ready Verdict blocks even with advisory findings', () => {
+  const ISSUE = '1. **P2 — The docs lag the new flag.**\n2. **P3 — Rename the helper.**\nVerdict: not ready (1 P1/P2)';
+  const P2 = '1. **P2 — The docs lag the new flag.**';
+  const textFor = (gate: OutsideGate, text: string) => gate === 'review' ? `${text}\n${REC}` : text;
+  const see = (text: string, gate: OutsideGate) => {
+    const r = classifyOutsideReview({ text: textFor(gate, text), gate });
+    return { verdict: r.verdict as string, highest: r.findings.highest as string | null, reason: (r.reason ?? null) as string | null, unparsed: r.unparsed ?? null };
+  };
+
+  test('numbered bold P2 plus P3 with not ready is findings/P2 on execution, structured and review', () => {
+    for (const gate of ['execution', 'structured', 'review'] as const)
+      expect({ gate, ...see(ISSUE, gate) }).toEqual({ gate, verdict: 'findings', highest: 'P2', reason: null, unparsed: null });
+    expect(validateOutsideReview(ISSUE, 'structured')).toEqual({ completed: true, gate: 'fail' });
+  });
+
+  test('P3-only and unlabeled not-ready replies block; P0/P1 block even when the last Verdict says ready', () => {
+    for (const gate of ['execution', 'structured', 'review'] as const) {
+      expect({ gate, ...see('1. **P3 — Rename the helper.**\nVerdict: not ready', gate) }).toEqual({ gate, verdict: 'findings', highest: 'P3', reason: null, unparsed: null });
+      expect({ gate, ...see('The retry path can double-send.\nVerdict: not ready', gate) }).toEqual({ gate, verdict: 'findings', highest: null, reason: null, unparsed: null });
+    }
+    expect(see('1. **P1 — The cap is ignored.**\nVerdict: ready', 'review')).toMatchObject({ verdict: 'findings', highest: 'P1' });
+    expect(see('**P0 — Secrets reach the log.**\nVerdict: ready', 'execution')).toMatchObject({ verdict: 'findings', highest: 'P0' });
+  });
+
+  test('advisory P2/P3 with ready or no Verdict stay clean', () => {
+    for (const text of [
+      `${P2}\n2. **P3 — Rename the helper.**\nVerdict: ready`,
+      `${P2}\n2. **P3 — Rename the helper.**`,
+      '1. **P3 — Rename the helper.**\nVerdict: ready (1 P3)',
+    ]) {
+      const r = see(text, 'execution');
+      expect({ text, verdict: r.verdict, highest: r.highest }).toEqual({ text, verdict: 'clean', highest: text.includes('P2') ? 'P2' : 'P3' });
+    }
+    expect(see(`${P2}\nVerdict: ready`, 'review').verdict).toBe('clean');
+    expect(validateOutsideReview(`${P2}\nVerdict: ready`, 'structured')).toEqual({ completed: true, gate: 'pass' });
+  });
+
+  test('the last recognized Verdict wins, including supported formatting; quotes do not', () => {
+    expect(see(`${P2}\nVerdict: ready\nVerdict: not ready (1 P1/P2)`, 'execution')).toMatchObject({ verdict: 'findings', highest: 'P2' });
+    expect(see(`${P2}\nVerdict: not ready (1 P1/P2)\nVerdict: ready`, 'execution').verdict).toBe('clean');
+    for (const verdictLine of ['**Verdict:** not ready', '- Verdict: not ready', '### Verdict: not ready'])
+      expect({ verdictLine, ...see(`${P2}\n${verdictLine}`, 'execution') }).toMatchObject({ verdict: 'findings', highest: 'P2' });
+    expect(see(`${P2}\nVerdict: not ready\n### Verdict: ready`, 'execution').verdict).toBe('clean');
+    const fence = ['```', 'Verdict: not ready (2 P2)', '```'].join('\n');
+    expect(see(`${P2}\nVerdict: ready\n${fence}`, 'execution').verdict).toBe('clean');
+    expect(see(`${fence}\n${P2}\nVerdict: not ready`, 'execution')).toMatchObject({ verdict: 'findings', highest: 'P2' });
+    expect(see(`${P2}\nVerdict: ready\nThe rubric says \`Verdict: not ready\`.`, 'execution').verdict).toBe('clean');
+    expect(see(`${P2}\nThe rubric says \`Verdict: not ready\`.`, 'execution').verdict).toBe('clean');
+    expect(see(`No issues found.\n${fence}`, 'review').verdict).toBe('clean');
+  });
+
+  test('parsed P2 plus an unparsed label and not-ready still blocks and keeps the diagnostic', () => {
+    const text = `${P2}\n## P1/P2 findings\nVerdict: not ready`;
+    expect(see(text, 'execution')).toEqual({ verdict: 'findings', highest: 'P2', reason: null, unparsed: 'line 2: "## P1/P2 findings"' });
+    expect(see(`${P2}\n## P1/P2 findings`, 'structured')).toMatchObject({ verdict: 'unverified', reason: 'unparsed_severity', unparsed: 'line 2: "## P1/P2 findings"' });
+  });
+
+  test('missing markers and failed execution stay unavailable; proposal and spec ignore the verdict', () => {
+    expect(classifyOutsideReview({ text: `${P2}\nVerdict: not ready`, gate: 'review' }).reason).toBe('missing_markers');
+    const failed = classifyOutsideReview({ text: textFor('review', `${P2}\nVerdict: not ready`), gate: 'review', exit: 1, stderr: 'fatal: provider exploded' });
+    expect(failed.verdict).toBe('unavailable');
+    expect(failed.reason).toBe('execution_failed');
+    expect(classifyOutsideReview({ text: `${P2}\nVerdict: not ready\n${REC}`, gate: 'proposal' }).verdict).toBe('clean');
+    const passing = classifyOutsideReview({ text: `SCORE: 8\nAMBIGUITIES: NONE\n${P2}\nVerdict: not ready`, gate: 'spec' });
+    expect([passing.verdict, passing.score]).toEqual(['clean', 8]);
+    const failing = classifyOutsideReview({ text: 'SCORE: 6\nAMBIGUITIES: NONE\nVerdict: ready', gate: 'spec' });
+    expect([failing.verdict, failing.score]).toEqual(['findings', 6]);
+  });
+});