From 5c5d9898506d23ff73da4a2a149474ad8889f307 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 14:55:18 +0000 Subject: [PATCH] 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 | 4 +- .../gsta259-3925-unverified-verdict.md | 4 +- docs/troubleshooting.md | 28 ++++++++------ lib/outside-review-result.ts | 37 +++++++++++++++---- test/outside-review-unparsed-severity.test.ts | 9 ++++- test/outside-runner-contract.test.ts | 6 +++ 6 files changed, 63 insertions(+), 25 deletions(-) diff --git a/bin/gstack-outside-voice b/bin/gstack-outside-voice index d2b2c56c1..381fbcd0a 100755 --- a/bin/gstack-outside-voice +++ b/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 { 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; diff --git a/changelog.d/gsta259-3925-unverified-verdict.md b/changelog.d/gsta259-3925-unverified-verdict.md index 9e70c40b5..04e160d9b 100644 --- a/changelog.d/gsta259-3925-unverified-verdict.md +++ b/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`. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 9beeefa94..9389dc014 100644 --- a/docs/troubleshooting.md +++ b/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 ### `... 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`. ### `CODEX_MODE: model_unusable` / `MODEL_UNUSABLE` diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts index 200206ee1..cb906c5a8 100644 --- a/lib/outside-review-result.ts +++ b/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> = { 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) { diff --git a/test/outside-review-unparsed-severity.test.ts b/test/outside-review-unparsed-severity.test.ts index 34cee204c..024c9cab3 100644 --- a/test/outside-review-unparsed-severity.test.ts +++ b/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 }); diff --git a/test/outside-runner-contract.test.ts b/test/outside-runner-contract.test.ts index 062f70595..f3cb95b6b 100644 --- a/test/outside-runner-contract.test.ts +++ b/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);