From 07c3913eaa2f7fa26b8c9fc00bb6868343df6ca0 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 15:44:21 +0000 Subject: [PATCH] fix(outside-review): read verdicts from a view that unwraps inline code, reject a verdict with a question mark (#3925 fix-check) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/outside-review-result.ts | 13 ++++++------- test/outside-review-unparsed-severity.test.ts | 3 ++- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts index 26ef23a22..55ca2570d 100644 --- a/lib/outside-review-result.ts +++ b/lib/outside-review-result.ts @@ -200,11 +200,11 @@ function plainReview(text: string): string { * blocking signals use that view, so an unclosed fence cannot hide a later `[P1]` * or `Verdict: not ready`, while the evidence for clean keeps it blanked. One pass: * an opener closes only when a later marker of its kind is at least as long. + * `keepSpans` unwraps every inline span: the verdict is read from that view, so + * `Verdict: \`ready\` (tests \`not run\`)` is read whole. */ const CODE_LABEL_BEFORE = /^[\t ]*(?:>[\t ]*)?(?:#{1,6}[\t ]+|[-+*][\t ]+|\(?\d{1,3}[.)][\t ]+)?(?:\*\*|__)?$|\b(?:severity|priority)[\t ]*[:=][\t ]*(?:\*\*|__)?$|\|[\t ]*(?:\*\*)?$/i; -/** A verdict line keeps every inline span: `Verdict: \`ready\` (tests \`not run\`)` is read whole. */ -const VERDICT_LINE = /^[\t ]*(?:>[\t ]*)*(?:#{1,6}[\t ]+|[-+*][\t ]+)?(?:\*\*|__)?(?:(?:overall|final)[\t ]+)?verdict\b/i; -function withoutCode(text: string, readUnclosed = false): string { +function withoutCode(text: string, readUnclosed = false, keepSpans = false): string { const lines = text.split(/\r?\n/); const markers = lines.map(line => /^[\t ]*(`{3,}|~{3,})/.exec(line)?.[1]); const closes = markers.map(() => true); @@ -226,8 +226,7 @@ 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)) - || VERDICT_LINE.test(line) ? inner : ' '); + keepSpans || inner.trim() === 'NO_FINDINGS' || /^\[?(?:P[0-3]|critical|high|medium|low)\]?$/i.test(inner.trim()) && CODE_LABEL_BEFORE.test(line.slice(0, at)) ? inner : ' '); }).join('\n'); } @@ -283,7 +282,7 @@ export function classifyOutsideReview(input: OutsideReviewInput): OutsideReviewC const levelsOf = (c: string, p: string) => [...[...p.matchAll(/\[(P[0-3])\]|^(P[0-3]):/gm)].map(m => (m[1] ?? m[2]) as Severity), ...severityTokens(c), ...(reviewed ? severityWords(c) : [])]; const levels = levelsOf(read, plainRead); - const verdicts = [lastVerdict(plainCode), lastVerdict(plainRead)]; + const verdicts = [lastVerdict(plainReview(withoutCode(input.text, false, true))), lastVerdict(plainReview(withoutCode(input.text, true, true)))]; 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 } : {}) }; @@ -309,7 +308,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 && !/^NO_?FINDINGS$/.test(v) && (!READY_VERDICT.test(v) || UNVERIFIED_CLAIM.test(v))); + const unready = verdicts.find(v => v !== undefined && !/^NO_?FINDINGS$/.test(v) && (!READY_VERDICT.test(v) || v.includes('?') || 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 2cb98f766..f7d00f166 100644 --- a/test/outside-review-unparsed-severity.test.ts +++ b/test/outside-review-unparsed-severity.test.ts @@ -229,6 +229,7 @@ describe('#3925 an unrecognized verdict is not clean; numbered findings never pr '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.', + '## Verdict\nready (tests `not run`)', '**Verdict**\nready (suites `skipped`)', 'Verdict: Ship it? No.', 'Verdict: Ready ? No.', ]) expect({ gate, verdictLine, ...see(`No issues found.\n${verdictLine}`, gate) }).toEqual({ gate, verdictLine, verdict: 'unverified', reason: 'unverified_verdict', unlabeled: 0 }); }); @@ -243,7 +244,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`', 'Verdict: NO_FINDINGS', 'Verdict: `NO_FINDINGS`']) + for (const verdictLine of ['Verdict: Ready to merge.', 'Verdict: LGTM', 'Verdict: Approve', 'Verdict: Ship it', '**Verdict:** `ready`', 'Verdict: NO_FINDINGS', 'Verdict: `NO_FINDINGS`', 'Verdict: ready. The `[P1]` from round 1 is fixed.']) 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 }); }