diff --git a/server/src/__tests__/github-webhook.test.ts b/server/src/__tests__/github-webhook.test.ts index e24599165ef9..84a22f615f02 100644 --- a/server/src/__tests__/github-webhook.test.ts +++ b/server/src/__tests__/github-webhook.test.ts @@ -9211,6 +9211,49 @@ describe("hasActionablePrReviewFeedback — reviewer taxonomy", () => { expect(__test_hasActionablePrReviewFeedback(body)).toBe(true); }); + // BLO-33880 / onprem-k8s#3490: a 0/0/0 review whose Recommended Action reads + // "No Critical issues to fix before merge" was published as a red. The span + // heuristic matched from the heading, so the negation sitting immediately + // before `fix` was never consulted. + it("is not actionable when the recommended action NEGATES the fix-before-merge directive", () => { + const body = [ + "## Ally — Consolidated PR Review", + "### Critical Issues (0)", + "", + "### Important Issues (0)", + "", + "### Recommended Action", + "1. No Critical issues to fix before merge.", + "2. No Important issues to address this cycle.", + ].join("\n"); + expect(__test_hasActionablePrReviewFeedback(body)).toBe(false); + }); + + // The negation must not become a mask: a genuine directive later in the same + // list still has to block, or the false-RED fix would open a fail-open hole. + it("stays actionable when a negated line precedes a real fix-before-merge directive", () => { + const body = [ + "### Recommended Action", + "1. No Critical issues to fix before merge.", + "2. Fix the Important issue before merge.", + ].join("\n"); + expect(__test_hasActionablePrReviewFeedback(body)).toBe(true); + }); + + // The case above passes even against a consuming span, because the negated + // line carries its own `before merge` terminator. This is the real regression + // guard: the negated `fix` has NO inline terminator, so a consuming span + // reaches onto line 2, advances lastIndex past the genuine directive, and + // never tests it. Fails open on a consuming span; blocks on the lookahead. + it("stays actionable when a negated fix WITHOUT its own terminator precedes a real directive", () => { + const body = [ + "### Recommended Action", + "1. No Critical issues to fix.", + "2. Fix the auth bypass before merge.", + ].join("\n"); + expect(__test_hasActionablePrReviewFeedback(body)).toBe(true); + }); + it("does not mask a non-zero bucket that follows a zero-count bucket", () => { const body = "### Critical Issues (0)\n### Important Issues (2)\n- one\n- two"; expect(__test_hasActionablePrReviewFeedback(body)).toBe(true); diff --git a/server/src/services/ally-review-detection.ts b/server/src/services/ally-review-detection.ts index 0896eed1aecb..64b3bff70ced 100644 --- a/server/src/services/ally-review-detection.ts +++ b/server/src/services/ally-review-detection.ts @@ -159,6 +159,11 @@ const NEGATION_CUE_REGEX = /\b(?:no|not|zero|none|never|without|isn't|aren't|doesn't|didn't|won't|cannot)\b/i; const NEGATION_LOOKBACK_WORDS = 8; +// "Recommended Action" + the two 400-character hops the directive heuristic +// allows, plus the heading itself. Keeps the scan window identical to the span +// regex it replaced, so narrowing the negation anchor cannot also narrow reach. +const RECOMMENDED_ACTION_REACH = 840; + // Uncounted findings must begin a heading/list line. An unanchored pattern // would incorrectly flag prose such as "No Critical or Important issues". const UNCOUNTED_FINDINGS_HEADING_REGEX = @@ -366,7 +371,27 @@ function carriesBlockingFeedback(text: string): boolean { if (/^[ \t]*decision[ \t]*:[ \t]*changes_requested[ \t]*$/im.test(text)) return true; if (hasNonNegatedMatch(text, /\bchanges\s+requested\b/i)) return true; if (hasNonNegatedMatch(text, /\brequest(?:ed|s)?\s+changes\b/i)) return true; - return /\bRecommended\s+Action\b[\s\S]{0,400}\bfix\b[\s\S]{0,400}\bbefore\s+merg(?:e|es|ed|ing)\b/i.test(text); + // The directive form ("fix X before merge") is blocking only when it is not + // negated. A clean review's Recommended Action routinely reads "No Critical + // issues to fix before merge", and matching the whole span from the heading + // read that as a finding — publishing a red over a 0/0/0 review (BLO-33880, + // onprem-k8s#3490). The negation lookback has to be anchored at `fix`, not at + // the heading: hasNonNegatedMatch walks back only to the start of the local + // sentence, so checking from the heading looks past the clause that negates. + // It scans every `fix` in the section and blocks on the first non-negated + // one, so a negated line cannot mask a real directive later in the same list. + for (const heading of text.matchAll(/\bRecommended\s+Action\b/gi)) { + // Same reach as the span this replaces: heading + two 400-character hops. + const section = text.slice(heading.index, heading.index + RECOMMENDED_ACTION_REACH); + // The terminator sits in a LOOKAHEAD so the match extent is just `fix`. A + // consuming span would swallow a later line's `before merge`, advancing + // lastIndex past a genuine directive that then never gets tested — the one + // direction this module must not fail in. + if (hasNonNegatedMatch(section, /\bfix\b(?=[\s\S]{0,400}?\bbefore\s+merg(?:e|es|ed|ing)\b)/i)) { + return true; + } + } + return false; } /** Return whether a formal or comment-shaped review contains blocking feedback. */