diff --git a/CHANGELOG.md b/CHANGELOG.md index 05af592e6..c977413a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,11 +5,11 @@ **An outside review whose findings are untagged prose no longer reads `clean` because a "no issues" phrase appears somewhere in it, and a clean Codex review that says "no actionable regressions" no longer reads `unverified`.** Reported in #3294 by @SchizasNik, with the false-pass half pointed out by @xg-gh-25. `lib/outside-review-result.ts` matched its no-findings vocabulary anywhere in the reply, so "The parser change has no new issues. However, … drops users_email_idx" passed /ship's structured gate. It also lacked `regressions`, so `codex review --uncommitted`'s "I found no actionable regressions in the staged, unstaged, or untracked changes." was missing coverage. ### Fixed -- **A no-findings statement counts only as the review's conclusion.** It must open the reply's last sentence or last ", and" clause (or the `because` clause of a review's `Recommendation:` line) and carry no contrast or exception ("but", "however", "except", "other than", "elsewhere"). When other prose comes first, a conclusion scoped to one place ("No issues in the parser.") must name the change as a whole. Anything else stays `unverified` / `untagged_review`, the same reason code and troubleshooting anchor as before. +- **A no-findings statement counts only as the review's conclusion.** It must open the reply's last sentence or last ", and" clause (or the `because` clause of a review's `Recommendation:` line) and carry no contrast or exception ("but", "however", "except", "other than", "elsewhere"). When other prose comes first, anything after the statement other than a predicate ("were found", "is evident") or a colon explanation must scope it to the change as a whole, so "No issues in the parser." and "No regressions were found by the existing tests." do not conclude the review. Anything else stays `unverified` / `untagged_review`, the same reason code and troubleshooting anchor as before. - **`regressions` and `defects` join the vocabulary**, and `NO_FINDINGS` written as inline code still counts. ### Gate integrity -- The structured and review gates' no-findings check (`classifyOutsideReview`) is stricter about position and wider in vocabulary. New free regressions in `test/outside-review-result.test.ts` cover the reporter's exact reply through their CLI invocation, two clean `codex review --uncommitted` replies captured from codex-cli 0.160.0 with gpt-6-sol (both read `unverified` before), thirteen mixed replies that must stay `unverified` under both gates (the two strings from the issue, the phrase before, after and inside untagged findings, exception conclusions, a quoted `NO_FINDINGS`, a no-issues line inside a code fence), and Recommendation-clause cases. All four new tests fail on the previous classifier. Every existing negative control passes unchanged, including the stored Codex clean review that still reads `unverified` ("no actionable correctness issues"). No threshold, trial count or case kind changed. +- The structured and review gates' no-findings check (`classifyOutsideReview`) is stricter about position and wider in vocabulary. New free regressions in `test/outside-review-result.test.ts` cover the reporter's exact reply through their CLI invocation, two clean `codex review --uncommitted` replies captured from codex-cli 0.160.0 with gpt-6-sol (both read `unverified` before), fifteen mixed replies that must stay `unverified` under both gates (the two strings from the issue, the phrase before, after and inside untagged findings, exception conclusions, conclusions scoped to one place or to the tests, a quoted `NO_FINDINGS`, a no-issues line inside a code fence), and Recommendation-clause cases including a wrapped clause with "but". A GPT-6.1 Sol outside review probed the classifier; its false passes are among those controls. A known residual, unchanged from before: untagged findings followed by "No issues found in the changed parser." still read `clean`, because "changed" names the change and the parser cannot tell a finding from a summary without tags. All four new tests fail on the previous classifier. Every existing negative control passes unchanged, including the stored Codex clean review that still reads `unverified` ("no actionable correctness issues"). No threshold, trial count or case kind changed. ## [1.91.148.0] - 2026-10-19 diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index e3e69a34c..1553aa67a 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -174,12 +174,14 @@ the readiness dashboard and the PR body, and never count it as a pass. severity prints `FINDINGS: unspecified` and exits 3 like a blocking finding. - **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". When other prose comes first, a - conclusion scoped to one place ("No issues in the parser.") must name the - change as a whole ("in this change", "in the staged changes"). A "no issues" - phrase anywhere else in the reply does not count. Otherwise the review is + `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". + 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 + 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 `clean`. diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts index 59d1a1ace..e376b107b 100644 --- a/lib/outside-review-result.ts +++ b/lib/outside-review-result.ts @@ -142,36 +142,47 @@ function severityWords(text: string): Severity[] { * 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 NO_FINDINGS = new RegExp(`^(?:(?:overall|in summary|in short|summary|conclusion|result)[,:]?\\s+)?(?: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 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'); /** A contrast or exception qualifies the claim, so the sentence does not conclude that nothing was found. */ -const NO_FINDINGS_EXCEPTION = /\b(?:but|however|although|though|yet|except|excluding|unless|only|apart from|aside from|other than|besides|beyond|elsewhere|otherwise)\b/i; -const SCOPED = /^\s*(?:(?:were|was|have been|has been)\s+)?(?:found\s+|identified\s+|detected\s+)?(?:in|inside|within|for|with|across|on|from|of|to|among|throughout)\b/i; -const WHOLE_CHANGE = /\b(?:chang(?:e|es|ed)|diffs?|patch(?:es)?|staged|unstaged|untracked|branch|commits?|pull request|PR)\b/i; +const NO_FINDINGS_EXCEPTION = /(? 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 rest = conclusion.slice(statement.length); - return total === 1 || !SCOPED.test(rest) || WHOLE_CHANGE.test(rest); + const qualifier = conclusion.slice(statement.length).replace(NO_FINDINGS_PREDICATE, '').replace(/[\s.!]+$/, '').trim(); + return total === 1 || !qualifier || qualifier.startsWith(':') || WHOLE_CHANGE_SCOPE.test(qualifier); }); } diff --git a/test/outside-review-result.test.ts b/test/outside-review-result.test.ts index 7f9f42bfb..ed13f8138 100644 --- a/test/outside-review-result.test.ts +++ b/test/outside-review-result.test.ts @@ -81,6 +81,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.', // 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.', 'The change validates `max` before retrying. No affected callers or tests are present in the repository, and no actionable regression is evident.']) { @@ -100,6 +101,8 @@ describe('a no-findings statement counts only as the reply\'s conclusion (#3294) `${FINDING}\nNo other issues.`, 'No issues found, except that the fallback path now swallows ENOENT.', 'No issues in the parser, and the fallback path now swallows ENOENT.', + 'Missing configuration now silently disables authentication. No regressions were found by the existing tests.', + 'Missing configuration now silently disables authentication. No issues are apparent in the parser.', 'The fallback path now swallows ENOENT, and no issues elsewhere.', 'The diff adds a NO_FINDINGS check, but the regex misses lowercase input.', `${FINDING}\n\`\`\`\nNo issues found.\n\`\`\``, @@ -118,6 +121,7 @@ describe('a no-findings statement counts only as the reply\'s conclusion (#3294) expect(verdict('No issues in the parser.\nRecommendation: fix the fallback because it swallows ENOENT.')).toBe('unverified'); expect(verdict(`${FINDING}\nRecommendation: ship because no regressions in the existing tests.`)).toBe('unverified'); expect(verdict(`${FINDING}\nRecommendation: ship because no issues remain, but the fallback needs a follow-up.`)).toBe('unverified'); + expect(verdict('Recommendation: address the migration because no regressions were found in the parser change,\nbut the migration drops users_email_idx.')).toBe('unverified'); }); });