diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index bd4fb9695..6aa548e8e 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -401,7 +401,13 @@ bun lib/outside-review-result.ts --verdict execution reply.md ...)`, 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. +does not count it as `clean`. The quoted verdict is in parentheses. Besides +the approvals, only the exact `NO_FINDINGS` token counts as a clean verdict. + +A `/codex` consult or challenge answer whose verdict picks an option +(`Verdict: go with B`, `Verdict: Proceed with the plan`) is `unverified` on +purpose: a consult is advice, not a pass/fail review, so there is no pass to +report. Read the answer itself; an answer with no verdict line stays `clean`. **What is kept.** The full reply is shown above. diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts index 99e7d1636..26ef23a22 100644 --- a/lib/outside-review-result.ts +++ b/lib/outside-review-result.ts @@ -268,7 +268,7 @@ function lastVerdict(plain: string): string | undefined { } return undefined; } -/** A verdict that approves the change. */ +/** A verdict that approves the change; the exact NO_FINDINGS token (its underscore is gone after plainReview) is clean too. */ 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|execut)|\bunverifi|\bnot (?:yet|until)\b|\bnot (?:been )?(?:verified|run|tested|checked|reviewed|executed)\b|\b(?:excluded|blocked|skipped)\b/i; @@ -309,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_VERDICT.test(v) || UNVERIFIED_CLAIM.test(v))); + const unready = verdicts.find(v => v !== undefined && !/^NO_?FINDINGS$/.test(v) && (!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'); diff --git a/test/outside-review-unparsed-severity.test.ts b/test/outside-review-unparsed-severity.test.ts index 589adf6e4..2cb98f766 100644 --- a/test/outside-review-unparsed-severity.test.ts +++ b/test/outside-review-unparsed-severity.test.ts @@ -228,6 +228,7 @@ describe('#3925 an unrecognized verdict is not clean; numbered findings never pr "Verdict \u2014 can't verify", 'Verdict: ready, though one changed file was excluded', 'Verdict: ready (tests not run, sandbox blocked)', 'Verdict: `ready` (tests `not run`)', 'Verdict: Ready? Not until the migration is fixed.', 'Verdict: clean-up required before merge', 'Verdict: ready, but I did not execute the suites', 'Verdict: ready (tests not executed)', + 'Verdict: no_findings', 'Verdict: NO_FINDINGS except the migration', 'Verdict: go with B.', 'Verdict: Proceed with the plan.', ]) expect({ gate, verdictLine, ...see(`No issues found.\n${verdictLine}`, gate) }).toEqual({ gate, verdictLine, verdict: 'unverified', reason: 'unverified_verdict', unlabeled: 0 }); }); @@ -242,7 +243,7 @@ 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 }); - for (const verdictLine of ['Verdict: Ready to merge.', 'Verdict: LGTM', 'Verdict: Approve', 'Verdict: Ship it', '**Verdict:** `ready`']) + for (const verdictLine of ['Verdict: Ready to merge.', 'Verdict: LGTM', 'Verdict: Approve', 'Verdict: Ship it', '**Verdict:** `ready`', 'Verdict: NO_FINDINGS', 'Verdict: `NO_FINDINGS`']) 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 }); }