From 6b57ededeb5dbd4469f4a4fb336e6dc42ae6d72c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 19 Oct 2026 13:50:36 -0700 Subject: [PATCH] v1.91.153.0 fix(outside-review): no-findings counts only as the review's conclusion; add regressions (#3294) (#3304) * 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> * fix(outside-review): a trailing ', and' clause can conclude; elsewhere/otherwise are exceptions; real codex captures Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(outside-review): whitelist the conclusion's qualifier; join wrapped Recommendation clauses; outside-review findings as controls Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(outside-review): join a wrapped line only when it continues in lowercase Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 11 ++++++ VERSION | 2 +- agents-digest/gstack-AGENTS.md | 2 +- docs/troubleshooting.md | 14 +++++-- lib/outside-review-result.ts | 56 +++++++++++++++++++++++++-- package.json | 2 +- test/outside-review-result.test.ts | 61 ++++++++++++++++++++++++++++++ 7 files changed, 138 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75d6f54f8..cfe3ed984 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## [1.91.153.0] - 2026-10-19 + +**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, 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), 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.152.0] - 2026-10-19 **`/cso` now starts and finishes on ordinary repositories that hold a nested Git repository, a long patch history, or a Docker socket on macOS 27.** Three user reports (#3255, #3248) each stopped a run on a normal repo. diff --git a/VERSION b/VERSION index 8219c59aa..eaffa97d8 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.91.152.0 +1.91.153.0 diff --git a/agents-digest/gstack-AGENTS.md b/agents-digest/gstack-AGENTS.md index 942806459..fc7e7065c 100644 --- a/agents-digest/gstack-AGENTS.md +++ b/agents-digest/gstack-AGENTS.md @@ -1,4 +1,4 @@ -# gstack digest v1.91.152.0 — regenerate/re-copy after upgrading gstack +# gstack digest v1.91.153.0 — regenerate/re-copy after upgrading gstack Behavioral rules from gstack (https://github.com/garrytan/gstack), compressed for agent hosts without a full skill install. The full skills add workflows, diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 15094f27e..1553aa67a 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -172,9 +172,17 @@ 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 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, 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`. - **Design proposals** (the design-direction voices in /design-consultation diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts index e9871427d..8409c73b2 100644 --- a/lib/outside-review-result.ts +++ b/lib/outside-review-result.ts @@ -135,8 +135,56 @@ 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_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 = /(? 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); + }); +} /** Formatting the requested marker in bold, inline code, or a list does not invalidate a completed review. */ function plainReview(text: string): string { @@ -159,7 +207,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 +266,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'); diff --git a/package.json b/package.json index f4e22657b..99f8e4a4b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.91.152", + "version": "1.91.153", "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.", "license": "MIT", "type": "module", diff --git a/test/outside-review-result.test.ts b/test/outside-review-result.test.ts index 5d3a2ab7e..572b90f30 100644 --- a/test/outside-review-result.test.ts +++ b/test/outside-review-result.test.ts @@ -65,6 +65,67 @@ 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.', + 'After reviewing the patch, I found no issues.', 'I found no issues in this read-only change.', + '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.', + 'The change validates `max` before retrying. No affected callers or tests are present in the repository, and no actionable regression is evident.']) { + 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.', + '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\`\`\``, + ]) { + 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'); + expect(verdict('Recommendation: address the migration because no regressions were found in the parser change,\nbut the migration drops users_email_idx.')).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' });