fix(outside-review): read approvals and verdict spellings, skip steps in a no-findings reply, share the FINDINGS label (#3925 review)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

bin/gstack-outside-voice

@@ -17,7 +17,7 @@
 import * as fs from 'node:fs';
 import * as path from 'node:path';
 import { EXIT, exitTableHelp } from '../lib/headless-artifacts';
-import type { OutsideGate } from '../lib/outside-review-result';
+import { findingsLabel, type OutsideGate } from '../lib/outside-review-result';
 import { RUNNER_IDS, createRunner, familyConflict, modelFamily, outsideStatusLine, runnerTable, selectRunner, type RunnerId } from '../lib/outside-voice-runner';
 import { outsideConsent } from '../lib/outside-consent';
 import { apiTimeoutMs } from '../lib/outside-voice-api';
@@ -121,7 +121,7 @@ async function main(argv: string[]): Promise<number> {
   fs.writeFileSync(`${path.resolve(outFile)}.meta.json`, JSON.stringify(result, null, 2) + '\n');
   if (json) process.stdout.write(JSON.stringify(result) + '\n');
   else {
-    if (result.verdict) process.stdout.write(`VERDICT: ${result.verdict.verdict}\nFINDINGS: ${result.verdict.findings.highest ?? 'none'}\n`);
+    if (result.verdict) process.stdout.write(`VERDICT: ${result.verdict.verdict}\nFINDINGS: ${findingsLabel(result.verdict)}\n`);
     process.stdout.write(outsideStatusLine(result) + '\n');
   }
   if (result.status === 'completed' || result.status === 'unverified') return EXIT.ok;

changelog.d/gsta259-3925-unverified-verdict.md

@@ -1,4 +1,4 @@
 ### Fixed
 
-- **An outside review that says it could not verify the change is no longer read as a pass (#3925).** `lib/outside-review-result.ts` printed `VERDICT: clean` / `FINDINGS: none` for a Codex reply ending `Verdict: Can't verify (one changed file excluded and four required suites blocked)`. A last `Verdict:` line now has to say `ready` (or `not ready`, which still blocks) for the reply to count; anything else, and a `ready` that says something could not be verified, is `VERDICT: unverified` with the new `REASON: unverified_verdict` and exit 4. See docs/troubleshooting.md#outside-review-unverified-verdict.
-- **A numbered finding without a P0-P3 tag is no longer dropped (#3925).** A reply with `1. **Nonblocking — x**`, `Verdict: ready` and `END OF REPORT: 1 findings (0 blocking)` printed `FINDINGS: none`. In a reply with a `Verdict:` line, every numbered item is now counted as a finding, and one with no readable severity makes `FINDINGS:` print `unspecified`. The verdict is unchanged (`clean` here, since the reviewer said ready). A consult answer with no `Verdict:` line, and a real clean reply, still read `clean` / `none`.
+- **An outside review that says it could not verify the change is no longer read as a pass (#3925).** `lib/outside-review-result.ts` printed `VERDICT: clean` / `FINDINGS: none` for a Codex reply ending `Verdict: Can't verify (one changed file excluded and four required suites blocked)`. Now the reply's last verdict has to approve the change (`ready`, `LGTM`, `approve`, `ship it`, `pass`, `clean`, `good to go`) or say `not ready`, which still blocks. Anything else, or an approval that says something was excluded, blocked, skipped, not run or could not be verified, is `VERDICT: unverified` with the new `REASON: unverified_verdict` and exit 4. A quoted verdict, an `Overall verdict` line, a dash separator and a `Verdict` heading with the value on the next line are all read. See docs/troubleshooting.md#outside-review-unverified-verdict.
+- **A numbered finding without a P0-P3 tag is no longer dropped (#3925).** A reply with `1. **Nonblocking — x**`, `Verdict: ready` and `END OF REPORT: 1 findings (0 blocking)` printed `FINDINGS: none`. In a reply with a verdict line that doesn't conclude it found nothing, every numbered item is now counted as a finding, and one with no readable severity makes `FINDINGS:` print `unspecified`, in the helper and in `gstack-outside-voice run`. The verdict is unchanged (`clean` here, since the reviewer approved). A consult answer with no verdict line, and a real clean reply, still read `clean` / `none`.

docs/troubleshooting.md

@@ -179,14 +179,19 @@ the readiness dashboard and the PR body, and never count it as a pass.
   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. Any other last `Verdict:`
-  (`Can't verify`, `blocked`, `revise`, an empty one), or a `ready` one that
-  says something could not be verified, is
-  [`unverified`](#outside-review-unverified-verdict), never `clean`.
-- **Numbered findings.** In a reply with a `Verdict:` line, every numbered
-  item at the start of a line (`1. x`, `1. **Nonblocking — x**`) is a finding.
-  When one has no readable severity, `FINDINGS:` prints `unspecified` instead
-  of `none`; the verdict still comes from the rules above.
+  prints `FINDINGS: unspecified` and exits 3. The verdict line may also be
+  quoted (`> Verdict:`), say `Overall verdict`, use a dash, or be a `Verdict`
+  heading with the value on the next line. A last verdict that is not an
+  approval (`ready`, `LGTM`, `approve`, `ship it`, `pass`, `clean`, `good to
+  go`), such as `Can't verify`, `blocked`, `revise` or an empty one, or an
+  approval that says something was excluded, blocked, skipped, not run or
+  could not be verified, is [`unverified`](#outside-review-unverified-verdict),
+  never `clean`.
+- **Numbered findings.** In a reply with a verdict line, every numbered item
+  at the start of a line (`1. x`, `1. **Nonblocking — x**`) is a finding,
+  unless the reply concludes that it found nothing. When one has no readable
+  severity, `FINDINGS:` prints `unspecified` instead of `none` (here and in
+  `gstack-outside-voice run`); the verdict still comes from the rules above.
 - **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`,
@@ -390,9 +395,10 @@ bun lib/outside-review-result.ts --verdict execution reply.md
 <a id="outside-review-unverified-verdict"></a>
 ### `... outside review: ran, verdict unverified (the reviewer's own verdict does not say the change is ready (Verdict: ...))` / `REASON: unverified_verdict`
 
-**Meaning.** The reply's last `Verdict:` line is neither `ready` nor
+**Meaning.** The reply's last verdict is neither an approval nor
 `not ready`, for example `Verdict: Can't verify (one changed file excluded
-...)`, or it says `ready` but also that something could not be verified. The
+...)`, or it approves but also says something was excluded, blocked, skipped,
+not run or could not be verified. The
 reviewer did not say the review finished and found the change ready, so gstack
 does not count it as `clean`. The quoted verdict is in parentheses.
 
@@ -405,7 +411,7 @@ run), then re-run the review. To re-check a saved reply, run
 gstack checkout.
 
 **Expected result.** `VERDICT: clean` or `VERDICT: findings` with a last
-`Verdict: ready` or `Verdict: not ready` line.
+verdict that approves the change or says `not ready`.
 
 <a id="codex-model-unusable"></a>
 ### `CODEX_MODE: model_unusable` / `MODEL_UNUSABLE`

lib/outside-review-result.ts

@@ -202,6 +202,8 @@ function plainReview(text: string): string {
  * an opener closes only when a later marker of its kind is at least as long.
  */
 const CODE_LABEL_BEFORE = /^[\t ]*(?:>[\t ]*)?(?:#{1,6}[\t ]+|[-+*][\t ]+|\(?\d{1,3}[.)][\t ]+)?(?:\*\*|__)?$|\b(?:severity|priority)[\t ]*[:=][\t ]*(?:\*\*|__)?$|\|[\t ]*(?:\*\*)?$/i;
+/** A verdict label before an inline span: `Verdict: \`ready\`` keeps its value. */
+const VERDICT_BEFORE = /^[\t ]*(?:>[\t ]*)*(?:#{1,6}[\t ]+|[-+*][\t ]+)?(?:\*\*|__)?(?:(?:overall|final)[\t ]+)?verdict(?:\*\*|__)?[\t ]*(?::|\u2014|\u2013|-)?[\t ]*(?:\*\*|__)?[\t ]*$/i;
 function withoutCode(text: string, readUnclosed = false): string {
   const lines = text.split(/\r?\n/);
   const markers = lines.map(line => /^[\t ]*(`{3,}|~{3,})/.exec(line)?.[1]);
@@ -224,7 +226,8 @@ function withoutCode(text: string, readUnclosed = false): string {
     }
     if (marker) { if (closes[i]) fence = marker; return ''; }
     return line.replace(/`([^`]+)`/g, (span, inner: string, at: number) =>
-      inner.trim() === 'NO_FINDINGS' || /^\[?(?:P[0-3]|critical|high|medium|low)\]?$/i.test(inner.trim()) && CODE_LABEL_BEFORE.test(line.slice(0, at)) ? inner : ' ');
+      inner.trim() === 'NO_FINDINGS' || /^\[?(?:P[0-3]|critical|high|medium|low)\]?$/i.test(inner.trim()) && CODE_LABEL_BEFORE.test(line.slice(0, at))
+        || VERDICT_BEFORE.test(line.slice(0, at)) ? inner : ' ');
   }).join('\n');
 }
 
@@ -250,12 +253,25 @@ function firstUnparsed(text: string): string | undefined {
   return at < 0 ? undefined : `line ${at + 1}: "${lines[at]!.trim().slice(0, 160)}"`;
 }
 
-/** The value of the reply's last `Verdict:` line, or undefined when it has none. */
+/**
+ * The value of the reply's last verdict line, or undefined when it has none:
+ * `Verdict: x`, `Overall verdict — x`, `> Verdict: x`, or a bare `Verdict`
+ * heading whose value is the next non-empty line.
+ */
 function lastVerdict(plain: string): string | undefined {
-  return [...plain.matchAll(/^[\t ]*Verdict[\t ]*:[\t ]*([^\r\n]*)$/gim)].at(-1)?.[1]!.trim();
+  const lines = plain.split(/\r?\n/);
+  for (let i = lines.length - 1; i >= 0; i--) {
+    const m = /^[\t ]*(?:>[\t ]*)*(?:(?:overall|final)[\t ]+)?verdict[\t ]*(?:(?::|\u2014|\u2013|-(?=[\t ]))[\t ]*(.*?))?[\t ]*$/i.exec(lines[i]!);
+    if (!m) continue;
+    if (m[1]) return m[1];
+    return lines.slice(i + 1).find(line => line.trim())?.replace(/^[\t ]*(?:>[\t ]*)*/, '').trim() ?? '';
+  }
+  return undefined;
 }
-/** A verdict that says the reviewer could not check something is never a ready one (#3925). */
-const UNVERIFIED_CLAIM = /\b(?:can['\u2019]?t|cannot|can not|couldn['\u2019]t|could not|unable to|not able to)\s+(?:be\s+)?verif|\bunverifi|\bnot (?:been )?verified\b/i;
+/** A verdict that approves the change. */
+const READY_VERDICT = /^(?:ready|lgtm|approved?|ship(?:[\t ]+it)?|pass(?:ed)?|clean|good to (?:go|merge|ship))\b/i;
+/** A verdict that says the reviewer could not check, run or see something is never a ready one (#3925). */
+const UNVERIFIED_CLAIM = /\b(?:can['\u2019]?t|cannot|can not|couldn['\u2019]t|could not|unable to|not able to|did not|didn['\u2019]t)\s+(?:be\s+)?(?:verif|run|test|check|open|read|review)|\bunverifi|\bnot (?:been )?(?:verified|run|tested|checked|reviewed)\b|\b(?:excluded|blocked|skipped)\b/i;
 /** A numbered item at the start of a line; in a reply with a `Verdict:` line it is a finding (#3925). */
 const NUMBERED_ITEM = /^(?:\*\*|__)?\(?\d{1,3}[.)][\t ]+\S.*$/gm;
 
@@ -268,7 +284,7 @@ export function classifyOutsideReview(input: OutsideReviewInput): OutsideReviewC
     ...severityTokens(c), ...(reviewed ? severityWords(c) : [])];
   const levels = levelsOf(read, plainRead);
   const verdicts = [lastVerdict(plainCode), lastVerdict(plainRead)];
-  const unlabeled = reviewed && verdicts[1] !== undefined
+  const unlabeled = reviewed && verdicts[1] !== undefined && !concludesNoFindings(code)
     ? [...read.matchAll(NUMBERED_ITEM)].filter(m => !levelsOf(m[0], plainReview(m[0])).length).length : 0;
   const findings = { highest: levels.length ? levels.sort()[0]! : null, ...(unlabeled ? { unlabeled } : {}) };
   const cleanEvidence = levelsOf(code, plainCode).length > 0;
@@ -293,7 +309,7 @@ export function classifyOutsideReview(input: OutsideReviewInput): OutsideReviewC
   const notReady = verdicts.some(v => v !== undefined && /^not[\t ]+ready\b/i.test(v));
   if (blocking || notReady) return { ...result('findings'), ...(unparsed ? { unparsed } : {}) };
   if (unparsed) return { ...result('unverified', 'unparsed_severity', unparsed), unparsed };
-  const unready = verdicts.find(v => v !== undefined && (!/^ready\b/i.test(v) || UNVERIFIED_CLAIM.test(v)));
+  const unready = verdicts.find(v => v !== undefined && (!READY_VERDICT.test(v) || UNVERIFIED_CLAIM.test(v)));
   if (unready !== undefined) return result('unverified', 'unverified_verdict', `Verdict: ${unready}`.trim().slice(0, 160));
   if ((input.gate === 'structured' || input.gate === 'review') && !cleanEvidence && !concludesNoFindings(code)) {
     return result('unverified', 'untagged_review', 'missing severity or explicit no-findings conclusion');
@@ -301,6 +317,11 @@ export function classifyOutsideReview(input: OutsideReviewInput): OutsideReviewC
   return result('clean');
 }
 
+/** The FINDINGS: value: the highest severity, `unspecified` when findings carry none, else `none`. */
+export function findingsLabel(checked: OutsideReviewClassification): string {
+  return checked.findings.highest ?? (checked.verdict === 'findings' || checked.findings.unlabeled ? 'unspecified' : 'none');
+}
+
 const LEGACY_REASONS: Partial<Record<GateReason, string>> = {
   empty_response: 'empty response',
   review_refused: 'review refused',
@@ -397,7 +418,7 @@ if (import.meta.main) {
   const checked = classifyOutsideReview({ text, gate: gate as OutsideGate, exit, stderr: await readOptional(flags['--stderr']), events });
   const label = flags['--label'] ?? 'Outside review';
   console.log(`VERDICT: ${checked.verdict}`);
-  console.log(`FINDINGS: ${checked.findings.highest ?? (checked.verdict === 'findings' || checked.findings.unlabeled ? 'unspecified' : 'none')}`);
+  console.log(`FINDINGS: ${findingsLabel(checked)}`);
   if (checked.unparsed) console.log(`UNPARSED: ${checked.unparsed}`);
   if (flags['--events'] !== undefined) console.log(formatUsageLine(usageFromEvents(events)));
   if (checked.reason) {

test/outside-review-unparsed-severity.test.ts

@@ -218,12 +218,14 @@ describe('#3925 an unrecognized verdict is not clean; numbered findings never pr
     expect(validateOutsideReview(read('no-findings.txt').replace('Verdict: ready', "Verdict: Can't verify"), 'execution').completed).toBe(false);
   });
 
-  test('every verdict other than ready, and a ready verdict that says it could not verify, is unverified on every review gate', () => {
+  test('every verdict that is not an approval, and an approval that says something was not checked, is unverified on every review gate', () => {
     for (const gate of ['execution', 'structured', 'review'] as const)
       for (const verdictLine of [
         "Verdict: Can't verify (suites blocked)", 'Verdict: cannot verify', 'Verdict: Unable to verify the Windows path',
         "**Verdict:** Can\u2019t verify", '### Verdict: blocked', 'Verdict: revise', 'Verdict:',
         'Verdict: ready, but I could not verify the migration', 'Verdict: ready (unverified)',
+        "> Verdict: Can't verify (suites blocked)", "## Verdict\nCan't verify (one changed file excluded)", "Overall verdict: can't verify",
+        "Verdict \u2014 can't verify", 'Verdict: ready, though one changed file was excluded', 'Verdict: ready (tests not run, sandbox blocked)',
       ])
         expect({ gate, verdictLine, ...see(`No issues found.\n${verdictLine}`, gate) }).toEqual({ gate, verdictLine, verdict: 'unverified', reason: 'unverified_verdict', unlabeled: 0 });
   });
@@ -238,8 +240,11 @@ describe('#3925 an unrecognized verdict is not clean; numbered findings never pr
   test('negative controls: real clean replies, consult answers, tagged findings and quoted examples stay as they were', () => {
     for (const gate of ['execution', 'structured', 'review'] as const) {
       expect({ gate, ...see(read('no-findings.txt'), gate) }).toEqual({ gate, verdict: 'clean', reason: null, unlabeled: 0 });
-      expect({ gate, ...see('No issues found.\nVerdict: Ready to merge.', gate) }).toEqual({ gate, verdict: 'clean', reason: null, unlabeled: 0 });
+      for (const verdictLine of ['Verdict: Ready to merge.', 'Verdict: LGTM', 'Verdict: Approve', 'Verdict: Ship it', '**Verdict:** `ready`'])
+        expect({ gate, verdictLine, ...see(`No issues found.\n${verdictLine}`, gate) }).toEqual({ gate, verdictLine, verdict: 'clean', reason: null, unlabeled: 0 });
+      expect({ gate, ...see('I checked:\n1. the retry path\n2. every caller\nNo issues found.\nVerdict: ready', gate) }).toEqual({ gate, verdict: 'clean', reason: null, unlabeled: 0 });
     }
+    expect(see('No issues found.\n## Verdict\nReady.', 'execution')).toEqual({ verdict: 'clean', reason: null, unlabeled: 0 });
     expect(see('1. Use a queue.\n2. Add a retry cap.', 'execution')).toEqual({ verdict: 'clean', reason: null, unlabeled: 0 });
     expect(see('1. **P2 — The docs lag.**\n2. **P3 — Rename it.**\nVerdict: ready', 'execution')).toEqual({ verdict: 'clean', reason: null, unlabeled: 0 });
     expect(see('No issues found.\n```\n1. Nonblocking — example\nVerdict: cannot verify\n```\nVerdict: ready', 'execution')).toEqual({ verdict: 'clean', reason: null, unlabeled: 0 });

test/outside-runner-contract.test.ts

@@ -112,6 +112,12 @@ describe('host-subagent runner', () => {
     expect(meta.output_sha256).toMatch(/^[0-9a-f]{64}$/);
     expect(meta.verdict.verdict).toBe('clean');
   });
+  test('prints the shared FINDINGS label: a numbered finding with no P-tag is unspecified, never none (#3925)', () => {
+    const result = path.join(ROOT, 'test', 'fixtures', 'outside-review-severity', 'codex-3925', 'nonblocking-numbered.txt');
+    const r = run(['run', '--runner', 'host-subagent', '--gate', 'structured', '--prompt', promptFile, '--out', path.join(TMP, 'host-3925.md'), '--result', result, '--model', 'openai/gpt-6-astra', '--native-model', 'claude-opus-4-7'], cleanEnv(emptyBin));
+    expect(r.status, r.stderr).toBe(0);
+    expect(r.stdout).toContain('VERDICT: unverified\nFINDINGS: unspecified\n');
+  });
   test('without --result or --model it is unavailable (exit 3, OUTSIDE_RUNNER_UNAVAILABLE)', () => {
     const r = run(['run', '--runner', 'host-subagent', '--prompt', promptFile, '--out', path.join(TMP, 'no.md'), '--model', 'gpt-5.4'], cleanEnv(emptyBin));
     expect(r.status).toBe(3);