Skip to content

check-pr-fully-clean.py scores non-blocking as a finding #3487

Description

@d-morrison

Problem

check-pr-fully-clean.py scores non-blocking as a finding, because the Block(?:ed|ing)? alternative in the findings pattern has no negation guard. \bblocking\b matches inside non-blocking — the hyphen ends the preceding word, so \b sits right before blocking.

The guard that exists (line ~1008) is scoped to changes\s+requested only, and its prefix test is \bno\s+(\w+\s+)?$, which requires a space after no. non-blocking has a hyphen, so it would not be caught even if the guard applied.

Why it matters

"Non-blocking" is the single most common way a reviewer marks a finding as explicitly not blocking. So the instrument reports NOT CLEAN precisely when a reviewer has gone out of their way to say the opposite — and it is one-directional toward false-not-clean, which is the safe direction but wastes a review round every time.

It also degrades the instrument's authority in the way that matters most: the whole argument for check-pr-fully-clean.py over a hand-rolled parse is that phrase-matching misreads in both directions. This is the instrument committing the error it exists to prevent.

Reproduction

Live, on #3468:

$ python3 scripts/check-pr-fully-clean.py 3468 -R Morrison-Lab/ai-config
  verdict scan: examined 3 dated automated review item(s), 2 bore a verdict, latest = clean
❌ PR is NOT fully clean:
  - Review comment for SHA 654301d1 contains findings (matched pattern
    '\b(?:Rejected|Unapproved|Block(?:ed|ing)?|Impasse|Deadlock|Changes\s+requested|Actionable\s+findings)\b')

Both reviews on that PR say Ready for merge, and the verdict scan agrees (latest = clean). Every one of the nine matches is inside the string non-blocking, in sentences like:

Ready for merge. One non-blocking suggestion (correct the stale table in the closed-by issue #3467) — does not block this PR's own content

Sketch of a fix

Give the Block… alternative a prefix guard, and widen the existing one so a hyphen counts:

  • skip a match preceded by non-?\s*, not\s+, or no\s+
  • apply it to the Block… alternative, not only to changes requested

Worth checking whether the other alternatives need it too — Rejected and Unapproved can appear in the same "no X" shapes.

Guard against fixing it too loosely

A blanket "skip any match with a negation within N characters" would swallow a real finding in a sentence like "this is not a nit — it is blocking". Pin whatever lands with tests in both directions: one that fails if non-blocking starts matching again, and one that fails if a genuine blocking finding stops being detected.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions