fix(review-gate): stop review/ally-comment red-flagging clean reviews (BLO-31890)

[allyblockcast]

Fixes BLO-31890.

The defect

hasActionablePrReviewFeedback ended with an unguarded prose rule:

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

Ally's clean Recommended-Action template is:

### Recommended Action

1. No Critical issues — nothing to fix before merge.
2. No Important issues.

That sentence carries Recommended Action … fix … before merge — the same three tokens a blocking one carries. So the rule matched the exact prose that declares a PR mergeable, and the gate went red precisely when the review was clean.

Measured blast radius (2026-09-04/05)

Three PRs simultaneously failure on review/ally-comment, each with a review at its current head reporting Critical Issues (0) / Important Issues (0):

PR	head	review/ally-comment
#1571	e0c050d6	failure
#1644	69c6e673	failure
#1655	e9a66437	failure

One predicate, three poisoned call paths

All cured by this change:

site	symptom
pr-comment-review-gate.ts:312 → :317	"…for this head carries an unresolved finding."
headsWithUndispositionedFinding :282 → :353	a clean earlier review counted as a carried finding → "An unresolved finding from Ally's review of <sha> is still undispositioned"
github-webhook.ts:3402	clean reviews falsely woke the PR author with changes-requested framing

The second one explains the earlier message on #1571 at 22:37Z: fa550ed's review was clean, and was still counted as a live finding.

The fix

Route the rule through hasNonNegatedMatch, exactly as the two changes requested rules above it already do — this was the only prose rule that did not. The guard is applied per occurrence, not per block, so a clean first line cannot mask a real demand after it.

What I deliberately did not do

My first cut also added nothing to NEGATION_CUE_REGEX. I dropped it:

1. It is unnecessary. The clean template carries a leading No, already inside the 8-word lookback. Verified against test(agent-health): restore executable preflight census #1571's real review body with the cue removed: still clean.
2. It costs a live finding. That vocabulary is shared with the changes requested rules, so the cue makes "Nothing else to note, but changes requested for the auth path" stop blocking — fail-open, the direction this module argues against at RESOLVED_PRIOR_DISPOSITIONS (:92-97).

Guard the rule, not the vocabulary. Both choices are pinned by tests.

Verification

• reviewer taxonomy — 18 passed (14 pre-existing + 4 new)
• pr-comment-review-gate — 31 passed

Every new assertion is mutation-checked against the real module, each caught by exactly the test written for it:

mutation	result
revert to the unguarded rule	the 2 clean-review tests fail
guard per-block instead of per-occurrence	the later-fix-demand test fails
re-add the nothing cue	the fail-closed test fails

Plus a sweep asserting every fix/before merge gap the previous 400/400 window admitted is still actionable, so the widened 820-char slice cannot narrow what used to block.

Note for reviewers

This PR is itself subject to the bug it fixes: if Ally reviews it clean, review/ally-comment will go red on this PR too, for the same reason. That status is not in the master ruleset (only merge_queue), so it does not mechanically block — but do not read a red here as a finding without opening the review body.

[allyblockcast]

🔗 Paperclip issue: BLO-31890

[allyblockcast]

@ally please review at head 5574a164.

Focus, in priority order:

1. The per-occurrence guard is the load-bearing choice. hasNonNegatedMatch is applied to the anchored slice, so a clean first line must not mask a real demand after it. The test does not let a clean recommended-action line mask a later genuine fix demand is the control; mutating the guard to per-block form is the failure I am trying to stay caught by. Is there a Recommended-Action shape where the 8-word lookback still masks a genuine finding?

2. The 820-char slice. It is sized as the previous rule's 400 before fix plus 400 after, so any fix that rule could reach still leaves ≥420 chars for the lookahead. Past 400 the slice can truncate the lookahead, but only for matches the old rule never made — so it cannot lose a finding that used to block. The sweep test pins gaps up to 396. Is that reasoning right, and is the asymmetry worth keeping over restoring exact 400/400 semantics?

3. The vocabulary decision. I deliberately did not add nothing to NEGATION_CUE_REGEX, because it is shared with the changes requested rules and would make "Nothing else to note, but changes requested…" fail open. Please sanity-check that against RESOLVED_PRIOR_DISPOSITIONS' stated fail-closed asymmetry — I read it as supporting the narrower fix, but it is the judgement call here.

Note this PR is subject to the bug it fixes: if your review is clean, review/ally-comment will read failure on this PR for that reason, not because of a finding.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Closing in favour of #1657, which fixes the same defect (BLO-31446) and does it properly.

#1657 was opened 16 minutes before this PR; I missed it because my duplicate search filtered on status: todo and BLO-31446 is in_progress. Filing BLO-31890 was itself a duplicate — of an issue I had filed myself while shepherding #1618. That is on me.

More importantly, #1657's approach is strictly better and this one is incomplete. Its PR body argues that wrapping the clause in hasNonNegatedMatch — exactly what this PR does — cannot work, because the guard only inspects words preceding a match within its sentence. I checked that against this branch's committed code rather than taking it on faith, running all four real 0/0 bodies #1657 cites:

body	this PR
paperclip#1605 — "Fix Critical issues before merge. (None.)"	✗ still false-reds — the negation is trailing, invisible to a lookback guard
paperclip#1618 — "No Critical issues to fix before merge."	✓ clean
multicast#589 — "No Critical or Important issues — nothing to fix before merge."	✓ clean
paperclip#1651 — heading, then fix as a noun, then a before merging belonging to a rebase note	✗ still false-reds — no negation cue exists to find

2 of 4 survive this fix. #1657's explicit-0/0-precedence handles all four, and is backed by a census over all 68 Ally consolidated reviews on the 25 most recent PRs (7 flips, all correct, none the other way). A lexical guard was the wrong instrument: #1651 uses all three tokens in good faith, so no tuning of the cue list separates them.

Nothing here is worth salvaging into #1657 — its early return sits above the prose fallback, so the per-occurrence guard this PR adds would be dead code behind it. I have posted the independent replication of its counter-examples on #1657 as corroboration.

Branch staff/blo-31890-review-gate-negation is left in place for the record; no further work planned. BLO-31890 is cancelled as a duplicate of BLO-31446, and BLO-30936's blocker edge has been re-pointed at BLO-31446.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5574a16

The diagnosis is right and the core move — routing the last unguarded prose rule through hasNonNegatedMatch — is correct. I reproduced the predicate old-vs-new outside the repo and confirmed it cures the three measured phrasings. Two findings are about what the rewrite changed beyond adding the guard.

Critical Issues (0)

Important Issues (2)

• [gstack/review] server/src/services/ally-review-detection.ts:239 — Anchoring on the first Recommended Action occurrence narrows the rule against the old whole-text scan, in the fail-open direction. The old non-global .test() retried at every occurrence of the heading; .exec() returns only the first, so a stray prose mention more than 820 chars ahead of the real heading now swallows the rule. Reproduced against both implementations:

  ## Ally — Consolidated PR Review
  ### Critical Issues (0)
  ### Important Issues (0)
  ### Suggestions (1)
  - The Recommended Action heading is fine. <900 chars>
  ### Recommended Action
  1. Fix the null deref before merge.
  

  → old true, new false. Same result for two genuine ### Recommended Action sections >820 apart (a supplementary or quoting comment). The PR body's claim that this "cannot lose a finding that used to block" is argued only over gap distance; it does not hold over occurrence count, and no test covers it.

  Blast radius is bounded — the counted-bucket rule at :215-217 returns early for any body reporting Critical/Important Issues (N>0), so this only bites bodies with no counted bucket. That is precisely the population this prose rule exists to serve, and the module argues fail-closed for exactly this class at :99-104.

  • Scan every occurrence: for (const m of text.matchAll(/\bRecommended\s+Action\b([\s\S]{0,820})/gi)) if (hasNonNegatedMatch(m[1]!, …)) return true; return false; — restores the old occurrence semantics while keeping the guard.

• [code] server/src/services/ally-review-detection.ts:244 — The cure depends on the negation cue landing inside NEGATION_LOOKBACK_WORDS (8) of fix, and the real phrasing on #1644 sits at exactly 8. Measured against the new predicate:

  clean-review line	words before fix	actionable
  No Critical issues — nothing to fix before merge. (#1655, real)	6	false ✅
  No Critical or Important issues — nothing to fix before merge. (#1644, real)	8	false ✅
  No Critical, Important, or blocking issues — nothing to fix before merge.	9	true ❌

  One extra clause in a clean review's prose and review/ally-comment goes red again, with no test pinning the sensitivity. The bug this PR fixes was found in production, not in review, so the margin matters.

  • The load-bearing cue here is the list item's leading No, which is a property of the line, not of an 8-word window. Scope this rule's lookback to the enclosing line, or pass a per-rule lookback, rather than relying on the shared 8-word default. Declining to widen NEGATION_CUE_REGEX (correct, see Strengths) does not preclude a rule-local lookback. At minimum, add a boundary test at 8 and 9 words so the margin fails loudly.

Suggestions (2)

• [comments] server/src/services/ally-review-detection.ts:233-238 — "so this cannot lose a finding that used to block" is a universal claim the surrounding argument does not establish; it proves only the gap-distance half. Tighten it to what is proven ("any fix the previous rule could reach still leaves enough slice for the lookahead"), so the next reader does not inherit the occurrence-count blind spot as settled.
• [native-codex] server/src/services/ally-review-detection.ts:239 — The canonical Recommended-Action template (1. Fix Critical issues before merge. / 2. Address Important issues this cycle.) still reads actionable on a (0)/(0) body — verified true under the new predicate. None of the three measured PRs emit it verbatim on a clean review, so this is not a live regression, but it is the same false-positive class and would strand a clean review red. Worth a line in the comment recording that the rule is cured for the observed phrasings, not for every clean Recommended-Action shape.

Strengths

• Declining to add nothing to NEGATION_CUE_REGEX is the right call and the reasoning is exactly right: the cue set is shared with the changes requested rules, so widening the vocabulary is fail-open at a distance. Guarding the rule instead of the vocabulary is the smaller blast radius, and it is pinned by the "Nothing else to note, but changes requested" test.
• The 820 sizing argument checks out: a fix the old rule could reach starts at slice offset ≤400, leaving ≥417 chars — enough for the 400-char lookahead plus before merging. I verified the arithmetic rather than taking it on trust.
• The per-occurrence guard (rather than per-block) is the correct reading, and does not let a clean recommended-action line mask a later genuine fix demand is the test that pins it.
• Test-quality detail worth keeping: the boundary sweep asserts on an object carrying gapBefore/gapAfter, so a failure names the offending cell instead of printing expected true, got false.

Recommended Action

1. Address the two Important findings before merge — the occurrence-count narrowing at :239 is the one that can silently clear a real finding, and it is a one-line change to matchAll.
2. Consider the Suggestions opportunistically; both are comment-accuracy, not behavior.
3. Note this PR is subject to the bug it fixes only in the clean case — this review reports findings, so review/ally-comment will go red here on its merits, not on the defect.