Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 74 additions & 13 deletions scripts/check-pr-fully-clean.py
Original file line number Diff line number Diff line change
Expand Up @@ -1232,19 +1232,44 @@ def strip_cited_finding_vocab(text: str) -> str:
# `Block(?:ed|ing)?` needs lookbehinds because `\b` treats a hyphen as a
# boundary, so "non-blocking" -- how a reviewer marks a nit as NOT blocking
# -- read a Ready-for-merge review as not-clean (ai-config#2369, measured
# 2026-08-26 on #2288). Only the `non-`/`non ` compounds are exempted.
# "previously-blocking" is deliberately NOT exempted, although it produces a
# safe-direction false positive when narrating a fixed finding: "the
# previously-blocking finding remains open; do not merge" is a real
# not-clean statement, and a lexical lookbehind cannot tell it from "the
# previously-blocking error was fixed". Missing a not-clean is the dangerous
# direction, so the narration form stays an over-flag -- as does any other
# `-blocking` compound ("merge-blocking" is a real signal) and the
# emphasized form ("non-**blocking**": the char before `blocking` is `*`,
# which the lookbehind cannot see through).
# 2026-08-26 on #2288). "non-"/"non " were the only compounds exempted at
# first; "not blocking" and "no blocking findings remain" reproduced the
# identical false positive on #3468 (ai-config#3487, measured 2026-09-09),
# because a hyphen or space after "non" is not the only way a reviewer
# negates the word -- "not " and "no " immediately before it are the same
# statement in different words. Four prefixes are now guarded: `non-`,
# `non `, `not `, `no `. The hyphenated forms of the latter two --
# "not-blocking", "no-blocking" -- are deliberately NOT exempted and stay
# flagged: this is the same over-flag-rather-than-swallow direction as
# "previously-blocking" just below, not an oversight (adding them would mean
# widening word-boundary lookbehinds the same blanket way that, on the
# `NOT_CLEAN_NEGATION_PREFIX` guard a few hundred lines down, turned out to
# swallow real not-clean statements like "Not-negligible changes requested."
# -- see that guard's own comment). "previously-blocking" is deliberately NOT
# exempted either, although it produces a safe-direction false positive when
# narrating a fixed finding: "the previously-blocking finding remains open;
# do not merge" is a real not-clean statement, and a lexical lookbehind
# cannot tell it from "the previously-blocking error was fixed". Missing a
# not-clean is the dangerous direction, so the narration form stays an
# over-flag -- as does any other `-blocking` compound ("merge-blocking" is
# a real signal) and the emphasized form ("non-**blocking**": the char
# before `blocking` is `*`, which the lookbehind cannot see through). A
# negator that is NOT immediately adjacent also stays flagged, on purpose:
# "this is not a nit -- it is blocking" has "not" five words away from
# "blocking", so none of the fixed-width lookbehinds below fire on it, and
# the sentence is correctly read as a live finding.
#
# `Rejected` and `Unapproved` were checked for the same treatment
# (ai-config#3487's own question) and deliberately left unguarded: unlike
# "non-blocking", there is no established review idiom "non-rejected" or
# "not unapproved" -- a grep of this corpus's own prose and test fixtures
# turns up no such usage anywhere. Adding a speculative guard for a phrasing
# that does not occur would only buy risk (silently swallowing a genuine
# "Rejected" or "Unapproved" finding) for zero real benefit. Revisit if a
# real occurrence ever surfaces.
_BARE_REJECTION = (
r"\b(?:Rejected|Unapproved|"
r"(?<!non-)(?<!non\s)Block(?:ed|ing)?"
r"(?<!non-)(?<!non\s)(?<!not\s)(?<!no\s)Block(?:ed|ing)?"
r"|Impasse|Deadlock|Changes\s+requested|Actionable\s+findings"
r"|Partial\s+review)\b"
)
Expand Down Expand Up @@ -1842,8 +1867,29 @@ def _is_exempt_findings_heading(
# belonging to an earlier clause: the `\w+\s+` filler cannot cross punctuation,
# so `This is not done. Needs work` and `It is not ready; needs more work` both
# stay not-clean.
#
# The negator may be joined to what follows by a hyphen instead of a space
# (ai-config#3487): "no-changes requested" reads identically to "no changes
# requested", and the fixed word-boundary regex `Block(?:ed|ing)?` lookbehinds
# a few lines below hit the same gap for "non-blocking".
#
# The hyphen branch is intentionally its OWN alternative, `-$`, rather than a
# blanket `[\s-]` substituted for every `\s` in the pattern (a first attempt
# at this fix did exactly that, and an adversarial review of #3487 caught it
# immediately): letting a hyphen stand in for a space INSIDE the `\w+\s+`
# filler as well swallows a genuine not-clean statement whenever the negator
# word happens to open a hyphenated compound ADJECTIVE rather than negate the
# phrase that follows -- "Not-negligible changes requested." and
# "Never-resolved changes requested." both misclassified as no-verdict under
# that blanket version, confirmed by reverting to it and rerunning the tests
# below. "no-negligible" and "not-negligible" are unrelated words; a hyphen
# there does not mean the same thing a space would. So the hyphen is accepted
# ONLY in the single position "non-blocking"/"no-changes" actually need: right
# after the negator, with nothing else (no filler word) between it and the
# not-clean phrase. Any filler word must still be space-separated, exactly as
# before this fix.
NOT_CLEAN_NEGATION_PREFIX = re.compile(
r"\b(?:no|not|nothing|none|never)\s+(?:\w+\s+){0,2}$", re.IGNORECASE
r"\b(?:no|not|nothing|none|never)(?:-|\s+(?:\w+\s+){0,2})$", re.IGNORECASE
)
# Two alternation groups on purpose. Emphasis markers are tolerated ONLY
# before the alternatives that are unambiguous negations when they open the
Expand Down Expand Up @@ -2343,7 +2389,22 @@ def _unresolved_finding_pattern(body: str) -> Optional[str]:
if pat == r"changes\s+requested\b":
start = match.start()
pfx = scan_body[max(0, start - 25):start].lower()
if re.search(r"\bno\s+(\w+\s+)?$", pfx):
# A hyphen right after "no" is recognized the same as a
# space ("no-changes requested" reads the same as "no
# changes requested") -- the same hyphen-vs-space gap that
# let "non-blocking" through before ai-config#3487. The
# hyphen is its own alternative, not substituted into the
# `\w+\s+` filler's separator too: doing that the first
# time around let a hyphenated compound ADJECTIVE ("No-
# nonsense changes requested here.") read as a negated
# filler word instead of a self-contained word unrelated to
# negating "changes requested" -- caught by adversarial
# review before this landed. This guard is now redundant
# with the `NOT_CLEAN_NEGATION_PREFIX` fix a few hundred
# lines up (which every pattern in this loop already goes
# through first), kept only for parity with the issue's
# own sketch.
if re.search(r"\bno(?:-|\s+(?:\w+\s+)?)$", pfx):
continue
return pat
return None
Expand Down
81 changes: 81 additions & 0 deletions scripts/test_check_pr_fully_clean.py
Original file line number Diff line number Diff line change
Expand Up @@ -1364,6 +1364,10 @@ def main() -> int:
"Nothing here needs any further work.",
"No changes requested.",
"There are no changes requested on this round.",
# ai-config#3487: the guard's prefix test required a space between
# "no" and what follows, so a hyphenated negation slipped through
# even though it says the identical thing.
"No-changes requested.",
):
check(
f"classify_verdict: a NEGATED not-clean phrase is not a verdict -- {phrase!r}",
Expand Down Expand Up @@ -1854,6 +1858,83 @@ def main() -> int:
checker.classify_verdict(
"### Verdict\nNits below, plus the previously blocking crash which is NOT fixed.\n", "")
== "not-clean")

# ai-config#3487: the #2369 fix above only exempted the "non-"/"non "
# compounds from the bare `_BARE_REJECTION` pattern itself. "not
# blocking" and "no blocking findings remain" say the exact same thing
# with a different negator immediately in front of the word, and the
# bare pattern matched both with no guard at all -- confirmed by
# reverting the `(?<!not\s)(?<!no\s)` lookbehinds and re-running these
# two checks, which then fail (match found instead of none).
#
# These two are asserted at the bare-pattern level, not through
# classify_verdict/`_unresolved_finding_pattern`: both of those already
# route every `_BARE_REJECTION` match through `NOT_CLEAN_NEGATION_PREFIX`
# (below), whose word list already includes "no"/"not" with a plain
# space -- so a classify_verdict-level check of the same two phrases
# passes identically with or without this fix and would not actually
# exercise it. The bare pattern is still worth guarding directly: other
# code (and this test file's own #2369 checks) matches `_BARE_REJECTION`
# standalone, with no downstream negation guard at all.
check("_BARE_REJECTION no longer matches inside 'not blocking'",
not _re.search(checker._BARE_REJECTION,
"this is not blocking the merge", _re.I))
check("_BARE_REJECTION no longer matches inside 'no blocking findings remain'",
not _re.search(checker._BARE_REJECTION,
"no blocking findings remain", _re.I))
# The dangerous direction stays covered: a negator that is NOT
# immediately adjacent to "blocking" must still read as a live finding,
# because the fixed-width lookbehind only ever looks at the three or
# four characters right before the word. "it is not a nit -- it is
# blocking" and "there is no doubt this is blocking" both put other
# words between the negator and "blocking", so neither is swallowed --
# this is the guard against the "blanket negation window" anti-pattern
# the issue explicitly warns against. Checked at both the bare-pattern
# level and through the full classify_verdict pipeline.
check("_BARE_REJECTION still matches 'not a nit -- it is blocking' (negator not adjacent)",
bool(_re.search(checker._BARE_REJECTION,
"this is not a nit -- it is blocking", _re.I)))
check("_BARE_REJECTION still matches 'no doubt this is blocking' (negator not adjacent)",
bool(_re.search(checker._BARE_REJECTION,
"there is no doubt this is blocking", _re.I)))
check("classify_verdict: 'not a nit -- it is blocking' stays not-clean",
checker.classify_verdict(
"### Verdict\nThis is not a nit -- it is blocking.\n", "")
== "not-clean")
check("classify_verdict: 'no doubt this is blocking' stays not-clean",
checker.classify_verdict(
"### Verdict\nThere is no doubt this is blocking merge.\n", "")
== "not-clean")

# ai-config#3487: NOT_CLEAN_NEGATION_PREFIX's negator-to-phrase gap used
# a bare `\s`, so a hyphenated negation ("No-changes requested.") was
# NOT exempted even though the word-spaced form ("No changes
# requested.") already was -- the same hyphen-vs-space gap "non-blocking"
# hit, one guard over. Mutation check: reverting the hyphen branch back
# to a bare `\s` in NOT_CLEAN_NEGATION_PREFIX makes this fail (returns
# "not-clean" instead of "").
check("classify_verdict: 'No-changes requested.' (hyphenated) is not a verdict",
checker.classify_verdict("### Verdict\nNo-changes requested.\n", "") == "")
# Round-2 adversarial review of #3487: the FIRST attempt at the check
# above widened `\s` to `[\s-]` everywhere in NOT_CLEAN_NEGATION_PREFIX,
# including inside the `\w+\s+` filler -- which let a hyphen stand in
# for a space between the negator and an intervening word, not just
# between the negator and the phrase itself. That swallowed any
# hyphenated compound ADJECTIVE that happens to open with a negator
# word, which has nothing to do with negating what follows: reverting
# to that blanket `[\s-]` version makes every check below fail (each
# returns "" or None instead of "not-clean"/a real finding).
for phrase in (
"Not-negligible changes requested.",
"No-nonsense changes requested here.",
"Not-yet-addressed changes requested below.",
"Never-resolved changes requested.",
):
check(
f"classify_verdict: hyphenated compound adjective before a real "
f"not-clean phrase still reads not-clean -- {phrase!r}",
checker.classify_verdict(f"### Verdict\n{phrase}\n", "") == "not-clean",
)
check("a previously blocking failure explicitly fixed is not an active finding",
checker._unresolved_finding_pattern(
"### Verdict\n**Ready for merge.** The previously blocking "
Expand Down
Loading