fix(outside-review): a check-results sign-off keeps the no-findings conclusion (#3469) (#4130)

* fix(outside-review): a check-results sign-off keeps the no-findings conclusion (#3469)

A clean Codex review that ends with "Tests, lint, and build all pass." read
VERDICT: unverified, so /ship and /review reported missing outside coverage.
Closing check-results sentences are now skipped when the conclusion is read,
fail-closed for code, failures, truncation and wrapped lines. (GST-1803)

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

* fix: pre-landing review fixes

A clause after the whole-change scope ("No issues in the diff, while the change disables auth.") no longer reads as a no-findings conclusion: the scope phrase ends at a clause break and while/whereas count as contrast words.

Review-Finding: p1/specialist-batched#1 reader=red-team
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* fix: pre-landing review fixes

Any clause joined after the whole-change scope (a comma, dash, parenthesis, or and/so/since/while...) keeps the no-findings statement from concluding the review; a list of change kinds stays one scope. The scope test is linear again.

Review-Finding: p2/fix-check/fix-check#1 reader=fix-check
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* fix: pre-landing review fixes

A double-hyphen or figure dash, brackets, a spaced slash, plus and despite after the whole-change scope also start a clause the scope does not cover. Granted fix-check under review_loop_exception a-2726c5b57840.

Review-Finding: fc2/fix-check#1 reader=fix-check
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* docs(troubleshooting): name the scope-clause rules for the outside-review no-findings conclusion

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

---------

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

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)

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

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 = /(?<![\w-])(?:but|however|although|though|yet|except|excluding|unless|only|apart from|aside from|other than|besides|beyond|elsewhere|otherwise)(?![\w-])/i;
+const NO_FINDINGS_EXCEPTION = /(?<![\w-])(?:but|however|although|though|whereas|while(?!\s+(?:reviewing|reading|checking|inspecting)\b)|yet|except|excluding|unless|only|apart from|aside from|other than|besides|beyond|elsewhere|otherwise)(?![\w-])/i;
 const NO_FINDINGS_PREDICATE = /^\s*(?:(?:was|were|is|are|has been|have been)\s+)?(?:found|identified|detected|evident|apparent|introduced|remain|exist|to report)?/i;
 const WHOLE_CHANGE_SCOPE = /^(?:in|within|across|throughout|with|from|for)\s.*\b(?:chang(?:e|es|ed)|diffs?|patch(?:es)?|staged|unstaged|untracked|branch|commits?|pull request|PR)\b/i;
+/** A list of change kinds ("the staged, unstaged, or untracked changes") is one scope; any other joiner starts a clause the scope does not cover. */
+const SCOPE_LIST_JOIN = /(?:,\s*(?:(?:and|or)\s+)?|\s+(?:and|or)\s+)(?=(?:staged|unstaged|untracked|committed)\b)/gi;
+const SCOPE_CLAUSE_BREAK = /[,;()[\]{}\u2012-\u2015]|--|\s[-/]\s|\b(?:and|or|plus|so|since|because|as|while|whereas|despite|then|which|that|where|when|though|but|yet|until|unless|if)\b/i;
 
 /**
  * The review's conclusion says it found nothing: its last prose sentence or
@@ -157,9 +170,44 @@ const WHOLE_CHANGE_SCOPE = /^(?:in|within|across|throughout|with|from|for)\s.*\b
  * prose precedes it, 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: "No issues in the parser." or "No regressions were found
- * by the tests." says nothing about the rest of the reply.
+ * by the tests." says nothing about the rest of the reply. The conclusion is
+ * read from `concluding`, the reply without its closing check-results
+ * sentences, while the sentence count comes from the whole reply, so dropping
+ * a sign-off never makes a scoped claim the reply's only sentence.
  */
-function concludesNoFindings(code: string): boolean {
+function concludesNoFindings(code: string, concluding: string): boolean {
+  const all = reviewProse(code), { sentences, because } = reviewProse(concluding);
+  const total = all.sentences.length + (all.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 && !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');

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' });