fix(outside-review): no-findings counts only as the review's conclusion; add regressions/defects (#3294)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

CHANGELOG.md

@@ -1,5 +1,16 @@
 # Changelog
 
+## [Unreleased]
+
+**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 the `because` clause of a review's `Recommendation:` line) and carry no contrast or exception ("but", "however", "except", "other than"). 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.
+- **`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, eleven 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.
+
 ## [1.91.148.0] - 2026-10-19
 
 **Weekly seed PRs now get the credential scan as well as the free suites.** A pull request the recorder opens with the workflow token never starts its own `pull_request` checks. GitHub holds them as "action required". So the recorder dispatches its checks on the bot branch instead.

docs/troubleshooting.md

@@ -172,9 +172,15 @@ the readiness dashboard and the PR body, and never count it as a pass.
   review, including the `execution` gate behind `/codex` consult and challenge.
   A reply whose last `Verdict:` line says `not ready` but carries no readable
   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 it
-  says so explicitly (`NO_FINDINGS`, "no issues", "did not find any bugs").
-  Otherwise it is `unverified` (see below), never `clean`. A `/codex` consult
+- **No findings.** A review with no tag and no label is `clean` only when its
+  conclusion says so: the last sentence, 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" or "other than". 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
+  `unverified` (see below), never `clean`. A `/codex` consult
   answer with no severity labels at all is not a review summary and stays
   `clean`.
 - **Design proposals** (the design-direction voices in /design-consultation

lib/outside-review-result.ts

@@ -135,8 +135,45 @@ const SEVERITY_LABELS = [
 function severityWords(text: string): Severity[] {
   return SEVERITY_LABELS.flatMap(re => [...text.matchAll(re)].map(m => SEVERITY_WORDS[m[1]!.toLowerCase()]!));
 }
-/** An explicit conclusion that the review found nothing to report. */
-const NO_FINDINGS = /\bNO_FINDINGS\b|\bno (?:actionable |significant |new |concrete )?(?:bugs?|issues?|findings?|problems?)\b|\b(?:did not|didn't|cannot|can't|could not|couldn't) (?:find|identify) any (?:actionable |new |concrete )?(?:bugs|issues|findings|problems)\b/i;
+/**
+ * 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 ...").
+ */
+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');
+/** 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)\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;
+
+/**
+ * The review's conclusion says it found nothing: its last prose sentence
+ * (wrapped lines joined) or the because-clause of its Recommendation line is an
+ * unqualified no-findings statement. When other prose precedes it, a
+ * conclusion scoped to one place ("No issues in the parser.") says nothing
+ * about the rest, so its scope must name the change as a whole.
+ */
+function concludesNoFindings(code: string): boolean {
+  let prose = '', because: string | undefined;
+  for (const line of code.split(/\r?\n/)) {
+    const text = plainReview(line).trim();
+    if (!text || /^(?:Recommendation|Verdict)[\t ]*:/i.test(text)) {
+      because = text.match(/^Recommendation[\t ]*:.*?\bbecause\b[\t ]*(\S.*)$/i)?.[1] ?? because;
+      prose += '\n';
+    } else if (/^[\t ]*#{1,6}[\t ]/.test(line)) prose += `\n${text}\n`;
+    else prose += /^[\t ]*[-+*][\t ]/.test(line) || prose.endsWith('\n') || /[.!?;:]$/.test(prose) ? `\n${text}` : ` ${text}`;
+  }
+  const sentences = prose.split(/(?<=[.!?;])\s+|\n/).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 rest = conclusion.slice(statement.length);
+    return total === 1 || !SCOPED.test(rest) || WHOLE_CHANGE.test(rest);
+  });
+}
 
 /** Formatting the requested marker in bold, inline code, or a list does not invalidate a completed review. */
 function plainReview(text: string): string {
@@ -159,7 +196,7 @@ function withoutCode(text: string): string {
     }
     if (marker) { fence = marker; return ''; }
     return line.replace(/`([^`]+)`/g, (span, inner: string, at: number) =>
-      /^\[?(?: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)) ? inner : ' ');
   }).join('\n');
 }
 
@@ -218,7 +255,7 @@ export function classifyOutsideReview(input: OutsideReviewInput): OutsideReviewC
   const notReady = !findings.highest && verdictNotReady(plainCode);
   if (blocking || notReady) return { ...result('findings'), ...(unparsed ? { unparsed } : {}) };
   if (unparsed) return { ...result('unverified', 'unparsed_severity', unparsed), unparsed };
-  if ((input.gate === 'structured' || input.gate === 'review') && !findings.highest && !NO_FINDINGS.test(input.text)) {
+  if ((input.gate === 'structured' || input.gate === 'review') && !findings.highest && !concludesNoFindings(code)) {
     return result('unverified', 'untagged_review', 'missing severity or explicit no-findings conclusion');
   }
   return result('clean');

test/outside-review-result.test.ts

@@ -65,6 +65,57 @@ describe('classifyOutsideReview: separate execution, findings and verdict', () =
   });
 });
 
+// #3294: "clean" comes only from a no-findings statement that concludes the reply,
+// never from a "no X" phrase among untagged findings anywhere in it.
+describe('a no-findings statement counts only as the reply\'s conclusion (#3294)', () => {
+  const REPORTER = 'I found no actionable regressions in the staged, unstaged, or untracked changes.';
+  const FINDING = 'The fallback path now swallows ENOENT and returns an empty config.';
+
+  test("the reporter's exact codex review reply reads clean through the reporter's CLI invocation", () => {
+    expect(classifyOutsideReview({ text: REPORTER, gate: 'structured' }).verdict).toBe('clean');
+    const r = cli(['--label', 'probe', '--exit', '0', '--stderr', file(''), 'structured', file(REPORTER)]);
+    expect([r.status, r.stdout]).toEqual([0, 'VERDICT: clean\nFINDINGS: none\n']);
+  });
+
+  test('clean conclusions, with the added regressions/defects vocabulary and the requested marker formatted', () => {
+    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.']) {
+      expect({ text, verdict: classifyOutsideReview({ text, gate: 'structured' }).verdict }).toEqual({ text, verdict: 'clean' });
+    }
+  });
+
+  test('mixed replies: untagged findings beside a "no X" phrase are unverified, never clean', () => {
+    for (const text of [
+      'The parser change has no new issues. However, db/002_drop_idx.sql drops users_email_idx, which will regress login latency under load.',
+      'Renaming the flag is no problem for callers, but the fallback path now swallows ENOENT and returns an empty config.',
+      `No issues in the parser.\n\n${FINDING}`,
+      `${FINDING}\n\nNo issues in the parser.`,
+      `${FINDING} No regressions in the existing tests.`,
+      'No regressions in the existing tests, but the fallback path now swallows ENOENT.',
+      `${FINDING}\nI found no issues other than the fallback path.`,
+      `${FINDING}\nNo other issues.`,
+      'No issues found, except that the fallback path now swallows ENOENT.',
+      'The diff adds a NO_FINDINGS check, but the regex misses lowercase input.',
+      `${FINDING}\n\`\`\`\nNo issues found.\n\`\`\``,
+    ]) {
+      for (const gate of ['structured', 'review'] as const) {
+        const r = classifyOutsideReview({ text: gate === 'review' ? `${text}\nRecommendation: ship because it looks fine.` : text, gate });
+        expect({ text, gate, verdict: r.verdict, reason: r.reason }).toEqual({ text, gate, verdict: 'unverified', reason: 'untagged_review' });
+      }
+    }
+  });
+
+  test("a review's Recommendation because-clause concludes under the same rules", () => {
+    const verdict = (text: string) => classifyOutsideReview({ text, gate: 'review' }).verdict;
+    expect(verdict('Recommendation: ship because I found no regressions in this change.')).toBe('clean');
+    expect(verdict(`The change adds a retry cap.\nRecommendation: ship because no issues remain.`)).toBe('clean');
+    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');
+  });
+});
+
 describe('validateOutsideReview keeps the text-only shape for direct importers', () => {
   test('existing shapes are unchanged', () => {
     expect(validateOutsideReview('', 'review')).toEqual({ completed: false, reason: 'empty response' });