fix(ally-review): do not read a NEGATED fix-before-merge line as a finding (BLO-33880)

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

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. */