fix(check-pr-fully-clean): stop scoring non-blocking as a finding

[d-morrison]

check-pr-fully-clean.py scored the string non-blocking as a finding.

\bblocking\b matches inside non-blocking — the hyphen ends the preceding word, so the word boundary sits right before blocking. The only negation guard in the file was scoped to changes\s+requested and tested \bno\s+…, which requires a space, so a hyphen was never going to match anyway.

Why it mattered

"Non-blocking" is the commonest way a reviewer marks a finding as explicitly not blocking, so the instrument reported NOT CLEAN precisely when a reviewer had gone out of their way to say the opposite.

That is the instrument committing the error it exists to prevent. The whole argument for check-pr-fully-clean.py over a hand-rolled phrase search — the one its own guard hook makes when it refuses a grep for ### Verdict — is that phrase searches misread in both directions.

Live before this change, on #3468:

  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 '...Block(?:ed|ing)?...')

All nine matches sat inside sentences like "Ready for merge. One non-blocking suggestion … does not block this PR's own content". After this change the same command exits 0.

The change

• _BARE_REJECTION's Block(?:ed|ing)? lookbehinds now cover not\s and no\s as well as non- and non\s.
• NOT_CLEAN_NEGATION_PREFIX accepts a hyphen after the negator through a dedicated alternation that fires only immediately after it — deliberately not a blanket [\s-] substitution.
• The parallel hyphen guard in _unresolved_finding_pattern's changes requested branch is redundant with the above and kept for parity with the issue's sketch; its comment says so, and reverting it fails no test.

Rejected and Unapproved are left unguarded. A grep for non-rejected / not rejected / no rejected and the Unapproved equivalents found no genuine idiom anywhere in the repo, so a speculative guard there would only risk swallowing a real finding for no benefit.

Why the blanket substitution was wrong

The obvious [\s-] version was written first, and review caught it swallowing real findings: "Not-negligible changes requested." classified clean. That is the dangerous direction — a false clean — which is why the fix narrows to hyphen-immediately-after-negator rather than treating hyphens and spaces alike everywhere in the separator.

Verification

797 tests pass. 20 are new, each mutation-checked:

mutation	result
not\s/no\s lookbehinds reverted	2 fail
hyphen branch reverted to bare \s	2 fail
blanket [\s-] reintroduced	4 fail, reproducing the compound-adjective false negatives
redundant changes requested guard reverted	0 fail (confirmed redundant, not silently broken)

Dangerous-direction controls, all still correctly flagged as findings: "this is not a nit — it is blocking", "no doubt this is blocking", "not-negligible changes requested", "non-trivial changes requested", "nothing here is optional: blocking". Cross-clause too: "Non-blocking suggestion. Changes requested regardless." stays not-clean, since the hyphen-adjacency window does not leak across an independent clause.

Closes #3487

🤖 Generated with Claude Code

[d-morrison]

Claimed by a Claude Code CLI session — driving this PR to a clean review and green CI.

Found while measuring a different PR: the instrument reported #3468 not-clean for three consecutive turns, and I chased that verdict rather than asking why a PR whose every review says "Ready for merge" was scoring as having findings.

Posted by Claude Code (AI agent) --- not written by a human.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[d-morrison]

Pausing here. State, so nothing is lost.

The review's finding stands, and my rebuttal was wrong

I argued the guard could not flag no-blocking while exempting no-changes requested, because it sees only the text before the matched phrase. The reviewer refuted that: the loop already has the matched pat in scope and already branches on it (if pat == _BARE_REJECTION, if pat == r"\bNeeds...", the changes\s+requested hyphen check). A pattern-scoped exemption is available; I asserted an architectural limit that is a property of the single regex I wrote, not of the pipeline.

And it found a second, worse one

NOT_CLEAN_NEGATION_PREFIX is consulted for every pattern in VERDICT_NOT_CLEAN_PATTERNS / FINDING_PATTERNS, not just Block…. So granting the hyphen to non-/no- silently swallows:

classify_verdict("### Verdict\nReady for merge. non-rejected nit noted.\n", "")   -> "clean"
classify_verdict("### Verdict\nReady for merge. no-rejected nit noted.\n", "")    -> "clean"
classify_verdict("### Verdict\nReady for merge. non-unapproved nit noted.\n", "") -> "clean"
classify_verdict("### Verdict\nReady for merge. non-impasse nit noted.\n", "")    -> "clean"
classify_verdict("### Verdict\nReady for merge. non-deadlock nit noted.\n", "")   -> "clean"

All five classify not-clean on origin/main, so it is a regression this branch introduces — in the dangerous direction the whole chain (#2369, #3487, this PR) exists to close. It also falsifies a claim in my own commit: the file says Rejected/Unapproved were "deliberately left unguarded", which is true of _BARE_REJECTION's lookbehinds and false of the shipped behaviour, because the generic guard reaches them anyway.

Why 806 tests did not catch it

The new cases test not-blocking and friends staying flagged. None tests non-rejected — the exact class the commit claims to have considered. A test asserting the documented behaviour would have failed against the code shipped beside it.

Next step, for whoever picks this up

Scope the hyphen exemption to the pattern it was written for, using the pat the loop already carries, rather than widening a guard shared by every not-clean phrase. Then pin, through classify_verdict and _unresolved_finding_pattern rather than against the bare pattern:

• non-blocking, no-changes requested — clean
• non-rejected, no-rejected, non-unapproved, non-impasse, non-deadlock — not-clean
• not-blocking, never-blocking, not-negligible changes requested — not-clean

origin/HEAD here is 50885dc1, which carries the regression. A local commit 45d1cbb8 narrows the hyphen to non-/no- and adds pipeline tests, but it does not fix the finding above and is deliberately unpushed.

The pattern worth naming

Three rounds on this PR, and each fix reintroduced the same defect class one layer out: _BARE_REJECTION's lookbehinds, then the generic prefix guard, then the generic guard's reach across every pattern. Every round the fix was correct about the case in front of it and wrong about the scope it applied at. That is the same shape as #3475's rounds 7 and 8, where narrowing and widening a PR-matching set each broke the other direction until the decision was moved to a signal that needed no heuristic at all.

Posted by Claude Code (AI agent) --- not written by a human.

[github-actions]

Claude finished review — View run

---

Summary

This round is a direct, targeted fix for the exact regression I flagged in the prior review round (commit 50885dc1): NOT_CLEAN_NEGATION_PREFIX's hyphen branch was a generic guard applied to every pattern in VERDICT_NOT_CLEAN_PATTERNS/FINDING_PATTERNS, so it silently exempted not-blocking, no-blocking, never-blocking, and not-rejected — directly contradicting the PR's own stated design intent that those forms stay flagged.

The fix removes the hyphen alternative from the shared NOT_CLEAN_NEGATION_PREFIX guard entirely (reverting it to the pre-PR space-only form, scripts/check-pr-fully-clean.py:1898-1901), and instead moves the hyphen exemption onto the specific phrase that needed it: (?<!\bno-)Changes\s+requested is now written directly into _BARE_REJECTION and into both VERDICT_NOT_CLEAN_PATTERNS/FINDING_PATTERNS' own changes\s+requested\b entries (lines 1299, 1874, 1972). The now-redundant _unresolved_finding_pattern special-case branch for changes\s+requested\b was correctly dropped as unreachable. Extensive new test coverage (lines 176-291 of the diff) exercises the fix through the full classify_verdict/_unresolved_finding_pattern pipeline rather than only at the bare-pattern level — closing the exact gap that let the regression pass 797/797 tests in the prior round.

I independently re-derived and ran the regexes rather than trusting the commentary or the new tests, per this repo's own algorithmatize-checks standard:

classify_verdict("### Verdict\nReady for merge. not-blocking nit noted.\n", "")   -> "not-clean"  ✓ (was "clean" before this fix)
classify_verdict("### Verdict\nReady for merge. no-blocking nit noted.\n", "")     -> "not-clean"  ✓
classify_verdict("### Verdict\nReady for merge. never-blocking nit noted.\n", "")  -> "not-clean"  ✓
classify_verdict("### Verdict\nReady for merge. not-rejected nit noted.\n", "")    -> "not-clean"  ✓
classify_verdict("### Verdict\nReady for merge. non-blocking nit noted.\n", "")    -> "clean"       ✓ (the real exemption, still works)
classify_verdict("### Verdict\nNo-changes requested.\n", "")                       -> ""            ✓ (the #3487 fix this PR preserves)
classify_verdict("### Verdict\nNot-negligible changes requested.\n", "")           -> "not-clean"  ✓ (compound-adjective false-negative avoided)
classify_verdict("### Verdict\nVolcano-changes requested.\n", "")                  -> "not-clean"  ✓ (word-boundary anchoring on the lookbehind holds)
classify_verdict("### Verdict\nThis is not a nit -- it is blocking.\n", "")        -> "not-clean"  ✓ (non-adjacent negator still flags)

All match expectations. I also confirmed the full local test suite passes: python3 scripts/test_check_pr_fully_clean.py → 843 passed, 0 failed, and hooks/test-no-handrolled-verdict-parse.py → 54/54 passed (unaffected by this change, checked since it shares verdict-parsing vocabulary). CI on the PR shows no failed checks (review workflow and validate were pending/in-progress at the time of this check; nothing red).

I found no other issues in this round's diff — no new patterns introduced, no scope creep, and the redundant code removal was correctly scoped (only the dead if pat == r"changes\s+requested\b" branch in _unresolved_finding_pattern, whose job is now done by the lookbehind embedded in the pattern itself).

Findings

None. The prior round's finding (NOT_CLEAN_NEGATION_PREFIX's hyphen branch silently exempting not-blocking/no-blocking/not-rejected) is fixed, verified independently, and covered by new tests that exercise the full pipeline rather than only the bare pattern — closing the exact coverage gap that let the regression through last round.

Verdict

Ready for merge — the previously-flagged regression is fixed and independently verified; no new issues found.

Structured Review Data (JSON)

{
  "schema_version": "1.1",
  "reviewer": "claude",
  "commit_sha": "348393ffcb0aa9c44f7a523a952853d453a98581",
  "verdict": "CLEAN",
  "findings": [],
  "detailed_assessment": "The NOT_CLEAN_NEGATION_PREFIX hyphen-branch regression flagged in the prior round (commit 50885dc1) is fixed by scoping the hyphen exemption onto the specific Changes-requested phrase in scripts/check-pr-fully-clean.py rather than the shared negation-prefix guard, verified independently via classify_verdict on not-blocking, no-blocking, never-blocking, and not-rejected all correctly returning not-clean.",
  "holistic_assessment": "The fix closes the exact test-coverage gap that let the regression pass 797 tests last round by adding pipeline-level tests through classify_verdict and _unresolved_finding_pattern, and the full local suite of 843 tests passes with CI showing no failed checks."
}

Reviewed commit: 163a963

[github-actions]

💰 Cost: $0.9948 (review) — run