From a37ccc016fd9e6dd2f4612472246896155843241 Mon Sep 17 00:00:00 2001 From: CTO Date: Mon, 14 Sep 2026 22:40:40 +0000 Subject: [PATCH 1/2] fix(ally-review): do not read a NEGATED fix-before-merge line as a finding (BLO-33880) The recommended-action heuristic matched one span from the 'Recommended Action' heading through 'fix' to 'before merge'. A clean review's action list routinely reads 'No Critical issues to fix before merge', which that span matches, so gate/ally-comment-findings published failure over a 0/0/0 review (onprem-k8s#3490, head fb72b062). Route the directive through hasNonNegatedMatch, which this file already uses for the two 'changes requested' phrasings. The negation lookback has to be anchored at 'fix' rather than at the heading: the lookback walks back only to the start of the local sentence, so checking from the heading looks past the clause that does the negating. hasNonNegatedMatch scans every match and blocks on the first non-negated one, so a negated line cannot mask a real directive later in the list -- pinned by a regression test, since that is the fail-open direction. --- server/src/__tests__/github-webhook.test.ts | 29 ++++++++++++++++++++ server/src/services/ally-review-detection.ts | 23 +++++++++++++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/server/src/__tests__/github-webhook.test.ts b/server/src/__tests__/github-webhook.test.ts index e24599165ef9..0d5e8e6fb37d 100644 --- a/server/src/__tests__/github-webhook.test.ts +++ b/server/src/__tests__/github-webhook.test.ts @@ -9211,6 +9211,35 @@ 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); + }); + 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..836526e7a13a 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,23 @@ 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); + 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. */ From 0113eab87838e03e4df482110ef17d5967679b06 Mon Sep 17 00:00:00 2001 From: CTO Date: Wed, 16 Sep 2026 21:43:32 +0000 Subject: [PATCH 2/2] fix(ally-review): make the fix-before-merge terminator a LOOKAHEAD (BLO-33880) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ally's review of the parent commit found a fail-open this diff introduced: the span `\bfix\b[\s\S]{0,400}?\bbefore\s+merg...` CONSUMES its terminator, so a negated `fix` with no inline terminator reaches forward and eats the NEXT line's `before merge`. regex.lastIndex then lands past the genuine directive, which is never tested. "1. No Critical issues to fix. 2. Fix the auth bypass before merge." -> master: blocks, this head: green That is a strict regression against master and the one direction this module must not fail in. The counted-bucket check backstops Ally's own structured reviews, but this predicate exists for the freeform/third-party reviews that carry no counted buckets, where the backstop is absent. Moving the terminator into a lookahead makes the match extent just `fix`, so lastIndex advances only past it and every later `fix` in the section is still examined. Behaviour is otherwise identical: the reach, the heading anchor and the sentence-scoped negation lookback are all unchanged. The existing guard test did not cover this — its negated line ends in `to fix before merge.`, so the match terminates on that same line and the second `Fix` is reached for unrelated reasons; it passes against the broken implementation too. Added the adversarial body (negated `fix` with NO inline terminator) as the actual regression test: verified failing at the parent commit and passing here. Co-Authored-By: Claude --- server/src/__tests__/github-webhook.test.ts | 14 ++++++++++++++ server/src/services/ally-review-detection.ts | 6 +++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/server/src/__tests__/github-webhook.test.ts b/server/src/__tests__/github-webhook.test.ts index 0d5e8e6fb37d..84a22f615f02 100644 --- a/server/src/__tests__/github-webhook.test.ts +++ b/server/src/__tests__/github-webhook.test.ts @@ -9240,6 +9240,20 @@ describe("hasActionablePrReviewFeedback — reviewer taxonomy", () => { 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 836526e7a13a..64b3bff70ced 100644 --- a/server/src/services/ally-review-detection.ts +++ b/server/src/services/ally-review-detection.ts @@ -383,7 +383,11 @@ function carriesBlockingFeedback(text: string): boolean { 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); - if (hasNonNegatedMatch(section, /\bfix\b[\s\S]{0,400}?\bbefore\s+merg(?:e|es|ed|ing)\b/i)) { + // 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; } }