diff --git a/changelog.d/gst-1803-outside-review-check-signoff.md b/changelog.d/gst-1803-outside-review-check-signoff.md new file mode 100644 index 000000000..70c19432f --- /dev/null +++ b/changelog.d/gst-1803-outside-review-check-signoff.md @@ -0,0 +1 @@ +- **A clean Codex outside review that signs off with a sentence about its checks now reads `VERDICT: clean`.** Since #3304 only a review's last sentence (or its Recommendation because-clause) can conclude it, so a reply such as "No actionable regressions were identified in the diff. Tests, lint, and build all pass." read `VERDICT: unverified` / `REASON: untagged_review`, and /ship and /review reported missing outside coverage for a review that ran and found nothing. Closing sentences that only report passing checks are now skipped when the conclusion is read, and "The changes introduce no definite, actionable regressions." counts as a no-findings statement held to the whole-change scope rule. It stays fail-closed: a check sentence that quotes code, fails, is cut off, continues a wrapped line or follows a colon is never skipped, skipping one never makes a scoped claim the reply's only sentence, a clause joined after the whole-change scope by a comma, dash, slash, bracket or a word such as "and", "so" or "while" ("No issues in the diff, while the change disables auth.") is never a conclusion, and a review with findings is never read as clean. See docs/troubleshooting.md#outside-review-verdict. (#3469, GST-1803; design and tests from #3641 by @vilano93) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index fb86c40eb..c71cdf3b5 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -199,12 +199,20 @@ the readiness dashboard and the PR body, and never count it as a pass. - **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`, - "No issues found", "I found no actionable regressions" or "I did not find - any bugs", with no "but", "however", "except", "other than" or "elsewhere". + "No issues found", "I found no actionable regressions", "I did not find + any bugs" or "The changes introduce no regressions", with no "but", + "however", "while", "whereas", "except", "other than" or "elsewhere". Closing sentences that + only report passing checks ("Tests, lint, and build all pass.") sign the + review off and are skipped, so the sentence before them is the conclusion; + a check sentence that quotes code, fails, is cut off, continues a wrapped + line, follows a colon or is the reply's only sentence is never skipped. When other prose comes first, anything after that statement except a predicate ("were found", "is evident") or a colon explanation must scope it to the change as a whole ("in this change", "in the staged changes"); "No - issues in the parser." does not conclude the review. A "no issues" phrase + issues in the parser." does not conclude the review, and neither does a + clause joined after that scope ("No issues in the diff, so it is safe."). + A statement with the change as its subject ("The changes introduce no + regressions") is held to this scope rule even as the reply's only sentence. A "no issues" phrase anywhere else in the reply does not count. Otherwise the review is `unverified` (see below), never `clean`. A `/codex` consult answer with no severity labels at all is not a review summary and stays diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts index e2db1b4ac..7a481673f 100644 --- a/lib/outside-review-result.ts +++ b/lib/outside-review-result.ts @@ -139,16 +139,29 @@ function severityWords(text: string): Severity[] { /** * A no-findings statement opening a sentence: NO_FINDINGS (its underscore is * gone after plainReview), "No issues found", "I found no actionable - * regressions", "I did not find any bugs". Never a phrase inside another - * clause ("renaming the flag is no problem for callers, but ..."). + * regressions", "I did not find any bugs", "The changes introduce no definite, + * actionable regressions" (the whole change as subject). Never a phrase inside + * another clause ("renaming the flag is no problem for callers, but ..."). */ -const FINDING_NOUN = '(?:(?:actionable|significant|new|concrete)\\s+)?(?:bugs?|issues?|findings?|problems?|regressions?|defects?)\\b'; +const FINDING_NOUN = '(?:(?:actionable|significant|new|concrete|definite),?\\s+){0,2}(?:bugs?|issues?|findings?|problems?|regressions?|defects?)\\b'; const NO_FINDINGS_LEAD = '(?:(?:overall|in summary|in short|summary|conclusion|result)[,:]?\\s+|(?:after|having|on|upon)\\s+(?:reviewing|reading|checking|inspecting|reviewed|read|checked|inspected)\\b[^,.;]{0,80},\\s+)?'; -const NO_FINDINGS = new RegExp(`^${NO_FINDINGS_LEAD}(?:NO_?FINDINGS\\b|(?:(?:i|we)\\s+(?:found|see|saw|identified)\\s+|there\\s+(?:are|were|is)\\s+)?no\\s+${FINDING_NOUN}|(?:i|we)\\s+(?:did not|didn't|could not|couldn't|cannot|can't)\\s+(?:find|identify|see)\\s+any\\s+${FINDING_NOUN})`, 'i'); +const CHANGE_SUBJECT = '(?:the|this|these)\\s+(?:changes?|diffs?|patch(?:es)?)\\s+(?:introduces?|adds?|contains?|ha(?:s|ve))\\s+'; +const NO_FINDINGS = new RegExp(`^${NO_FINDINGS_LEAD}(?:NO_?FINDINGS\\b|(?:(?:i|we)\\s+(?:found|see|saw|identified)\\s+|there\\s+(?:are|were|is)\\s+|${CHANGE_SUBJECT})?no\\s+${FINDING_NOUN}|(?:i|we)\\s+(?:did not|didn't|could not|couldn't|cannot|can't)\\s+(?:find|identify|see)\\s+any\\s+${FINDING_NOUN})`, 'i'); +/** The whole change as subject is held to the scope rule even as the reply's only sentence. */ +const CHANGE_SUBJECT_STATEMENT = new RegExp(`^${NO_FINDINGS_LEAD}${CHANGE_SUBJECT}`, 'i'); +/** + * A whole sentence that only reports passing checks ("Tests, lint, and build all + * pass."). Closing the reply, it signs the review off and never replaces its conclusion (#3469). + */ +const CHECK_NAME = '(?:(?:unit|existing|targeted|full)\\s+)?(?:tests?|test suite|lint(?:ing)?|build|typecheck|type[- ]check|format check)'; +const CHECK_RESULTS = new RegExp(`^(?:the\\s+|all\\s+)?${CHECK_NAME}(?:(?:,\\s*(?:and\\s+)?|\\s+and\\s+)${CHECK_NAME})*\\s+(?:all\\s+|both\\s+)?(?:pass(?:es|ed)?|succeed(?:s|ed)?|(?:is|are)\\s+green)[.!]$`, 'i'); /** A contrast or exception qualifies the claim, so the sentence does not conclude that nothing was found. */ -const NO_FINDINGS_EXCEPTION = /(? { + const statement = conclusion?.match(NO_FINDINGS)?.[0]; + if (!conclusion || !statement || NO_FINDINGS_EXCEPTION.test(conclusion)) return false; + const qualifier = conclusion.slice(statement.length).replace(NO_FINDINGS_PREDICATE, '').replace(/[\s.!;]+$/, '').trim(); + return (total === 1 && !CHANGE_SUBJECT_STATEMENT.test(conclusion)) || !qualifier || qualifier.startsWith(':') + || (WHOLE_CHANGE_SCOPE.test(qualifier) && !SCOPE_CLAUSE_BREAK.test(qualifier.replace(SCOPE_LIST_JOIN, ' '))); + }); +} + +/** + * The raw reply without the check-results sentences that close it, read before + * code is blanked so a sentence quoting code ("Tests `do not` pass.") never is + * one. A line is a sentence of its own only after a sentence end or a blank + * line, never the tail of a wrapped one ("`Not all`\ntests pass.") or a + * condition after a colon. The reply's only sentence is never dropped. + */ +function withoutClosingCheckResults(reply: string): string { + let rest = reply.trimEnd(); + for (;;) { + const line = rest.slice(rest.lastIndexOf('\n') + 1); + const last = line.split(/(?<=[.!?;])[\t ]+/).at(-1)!; + const before = rest.slice(0, rest.length - last.length); + const ownSentence = last.length < line.length || /(?:[.!?;]|\n[\t \r]*)[\t \r]*\n[\t ]*$/.test(before); + if (!ownSentence || !CHECK_RESULTS.test(last.trim()) || !before.trim()) return rest; + rest = before.trimEnd(); + } +} + +/** The reply's prose sentences, wrapped lines joined, and its Recommendation because-clause. */ +function reviewProse(code: string): { sentences: string[]; because: string | undefined } { let prose = '', because: string | undefined, recommendationOpen = false; for (const line of code.split(/\r?\n/)) { const text = plainReview(line).trim(); @@ -177,14 +225,7 @@ function concludesNoFindings(code: string): boolean { } else if (/^[\t ]*#{1,6}[\t ]/.test(line)) prose += `\n${text}\n`; else prose += listed || prose.endsWith('\n') || /[.!?;:]$/.test(prose) || !/^[a-z]/.test(text) ? `\n${text}` : ` ${text}`; } - const sentences = prose.split(/(?<=[.!?;])\s+|\n|,\s+and\s+/i).map(sentence => sentence.trim()).filter(Boolean); - const total = sentences.length + (because ? 1 : 0); - return [sentences.at(-1), because].some(conclusion => { - const statement = conclusion?.match(NO_FINDINGS)?.[0]; - if (!conclusion || !statement || NO_FINDINGS_EXCEPTION.test(conclusion)) return false; - const qualifier = conclusion.slice(statement.length).replace(NO_FINDINGS_PREDICATE, '').replace(/[\s.!]+$/, '').trim(); - return total === 1 || !qualifier || qualifier.startsWith(':') || WHOLE_CHANGE_SCOPE.test(qualifier); - }); + return { sentences: prose.split(/(?<=[.!?;])\s+|\n|,\s+and\s+/i).map(sentence => sentence.trim()).filter(Boolean), because }; } /** Formatting the requested marker in bold, inline code, or a list does not invalidate a completed review. */ @@ -276,11 +317,12 @@ export function classifyOutsideReview(input: OutsideReviewInput): OutsideReviewC const code = withoutCode(input.text), plainCode = plainReview(code); const read = withoutCode(input.text, true), plainRead = plainReview(read); const marked = recommendationHeading(code); + const concludes = concludesNoFindings(marked, recommendationHeading(withoutCode(withoutClosingCheckResults(input.text)))); 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 unlabeled = reviewed && verdicts[1] !== undefined && !concludesNoFindings(marked) + const unlabeled = reviewed && verdicts[1] !== undefined && !concludes ? [...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; @@ -307,7 +349,7 @@ export function classifyOutsideReview(input: OutsideReviewInput): OutsideReviewC if (unparsed) return { ...result('unverified', 'unparsed_severity', unparsed), unparsed }; 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(marked)) { + if ((input.gate === 'structured' || input.gate === 'review') && !cleanEvidence && !concludes) { return result('unverified', 'untagged_review', 'missing severity or explicit no-findings conclusion'); } return result('clean'); diff --git a/test/outside-review-result.test.ts b/test/outside-review-result.test.ts index 0e2d37b1e..4f1b7c83b 100644 --- a/test/outside-review-result.test.ts +++ b/test/outside-review-result.test.ts @@ -140,7 +140,7 @@ describe('a no-findings statement counts only as the reply\'s conclusion (#3294) for (const text of ['No regressions found.', 'I did not find any defects.', 'There are no actionable regressions.', 'NO_FINDINGS', '**NO_FINDINGS**', '`NO_FINDINGS`', '- NO_FINDINGS', `The change adds a retry cap and its tests.\n${REPORTER}`, 'Reviewed the retry helper.\n\nNo issues found in this change.', - 'After reviewing the patch, I found no issues.', 'I found no issues in this read-only change.', + 'After reviewing the patch, I found no issues.', 'I found no issues in this read-only change.', 'No issues found while reviewing the diff.', 'Summary of the retry helper\nNo issues found.', 'I found no actionable regressions in the staged\nchanges.', // Clean `codex review --uncommitted` replies captured from codex-cli 0.160.0 with gpt-6-sol. 'The change validates the retry limit before execution. No actionable regression was found.', @@ -185,6 +185,82 @@ describe('a no-findings statement counts only as the reply\'s conclusion (#3294) }); }); +// #3469: a clean Codex review often signs off with one sentence about the checks +// it ran. That sentence follows the no-findings conclusion; it never replaces it. +describe('a closing check-results sentence keeps the no-findings conclusion (#3469)', () => { + // The two clean tails quoted in #3469 (codex review --base main, codex-cli 0.163.0-alpha, gpt-6-luna). + const TAILS = [ + 'No actionable regressions were identified in the diff. Tests, lint, and build all pass.', + 'The changes introduce no definite, actionable regressions. Tests, lint, and build all pass.', + ]; + const FINDING = 'The fallback path now swallows ENOENT and returns an empty config.'; + const unverified = (text: string) => { + const r = classifyOutsideReview({ text, gate: 'structured' }); + expect({ text, verdict: r.verdict, reason: r.reason }).toEqual({ text, verdict: 'unverified', reason: 'untagged_review' }); + }; + + test("the reporter's clean tails read clean in-process and through the CLI /ship calls", () => { + for (const text of TAILS) { + expect({ text, verdict: classifyOutsideReview({ text, gate: 'structured' }).verdict }).toEqual({ text, verdict: 'clean' }); + const r = cli(['--label', 'Codex structured review', '--exit', '0', '--stderr', file(''), 'structured', file(`${text}\n`)]); + expect([r.status, r.stdout]).toEqual([0, 'VERDICT: clean\nFINDINGS: none\n']); + } + }); + + test('other check-results sign-offs, one or several, on one line or after a blank line', () => { + for (const text of ['No issues found. Typecheck passes.', 'No regressions found in this change. The tests pass.\nLint passes.', + 'I found no issues. Tests and build succeeded.', 'This patch introduces no regressions. All tests pass.', + 'The change validates the retry limit.\nNo actionable regressions were identified in the diff; tests, lint, and build all pass.', + `The change adds a retry cap.\n\n${TAILS[0]}`, 'No issues found.\r\n\r\nTests, lint, and build all pass.\r\n']) { + expect({ text, verdict: classifyOutsideReview({ text, gate: 'structured' }).verdict }).toEqual({ text, verdict: 'clean' }); + } + }); + + test('findings followed by a check-results sentence are never clean', () => { + for (const text of [`[P1] data loss\n${TAILS[0]}`, `P0: corrupts every row\n${TAILS[1]}`, `Severity: High. ${FINDING}\n${TAILS[0]}`]) { + expect({ text, verdict: classifyOutsideReview({ text, gate: 'structured' }).verdict }).toEqual({ text, verdict: 'findings' }); + } + for (const text of [`${FINDING} Tests, lint, and build all pass.`, `No issues found. ${FINDING} Tests pass.`, + 'No issues found. Tests pass. The fallback path now swallows ENOENT.', + 'No regressions found. Tests pass, but the fallback path now swallows ENOENT.', + 'No issues found, except that the fallback path now swallows ENOENT. Lint passes.', + `${FINDING}\nNo issues in the parser. Tests pass.`, + `${FINDING} The changes to the parser introduce no regressions. Tests pass.`, + 'The changes introduce no regressions in parsing, while missing configuration now silently disables authentication.', + 'No issues in the parser, while missing configuration now silently disables authentication. Tests pass.', + 'The changes introduce no regressions in the diff, while missing configuration now silently disables authentication.', + 'No issues in the parser, while the change silently disables authentication. Tests pass.', + 'I reviewed the parser.\nNo issues in the diff, and the fallback path now swallows ENOENT in this change.', + 'I reviewed the parser.\nNo issues in the parser whereas this change drops the cache.', + 'The changes introduce no regressions in the diff, the fallback path now swallows ENOENT.', + 'I reviewed the parser.\nNo issues in the diff and the fallback path now swallows ENOENT.', + 'I reviewed the parser.\nNo issues in the diff \u2014 the fallback path now swallows ENOENT.', + 'I reviewed the parser.\nNo issues in the diff (the fallback path now swallows ENOENT).', + 'I reviewed the parser.\nNo issues in this change so callers now lose data.', + 'I reviewed the parser.\nNo issues in the diff -- the fallback path now swallows ENOENT.', + 'I reviewed the parser.\nNo issues in the diff [the fallback path now swallows ENOENT].', + 'I reviewed the parser.\nNo issues in the diff / the fallback path now swallows ENOENT.', + 'I reviewed the parser.\nNo issues in the diff \u2012 the fallback path now swallows ENOENT.', + 'I reviewed the parser.\nNo issues in the diff plus the fallback path now swallows ENOENT.', + 'I reviewed the parser.\nNo issues in the diff despite the fallback path now swallowing ENOENT.']) unverified(text); + }); + + test('a truncated, empty or check-only reply is never clean', () => { + for (const text of ['No actionable regressions were identified in the diff. Tests, lint, and b', + 'No actionable regressions were identified in the diff. Tests, lint, and build all pass', + 'No issues found. Tests', 'Tests, lint, and build all pass.', 'No regressions found. Tests fail.']) unverified(text); + const empty = classifyOutsideReview({ text: ' \n', gate: 'structured' }); + expect([empty.verdict, empty.reason]).toEqual(['unavailable', 'empty_response']); + }); + + test('a check sentence that quotes code, continues a wrapped line or follows a condition is not a sign-off', () => { + for (const text of ['No issues found. Tests `do not` pass.', 'No issues found. `Not all` tests pass.', + 'No issues found. Tests pass `only if the regression is skipped`.', 'No issues found. `Not all`\ntests pass.', + '```\ntests pass.\n```\nNo issues found.\n`Not all` tests pass.', 'No issues found in this patch, provided that:\nTests pass.', + 'No issues found. Not all tests pass.', 'No issues found. Some tests pass.']) unverified(text); + }); +}); + describe('validateOutsideReview keeps the text-only shape for direct importers', () => { test('existing shapes are unchanged', () => { expect(validateOutsideReview('', 'review')).toEqual({ completed: false, reason: 'empty response' });