check-pr-fully-clean.py reports clean when only the review-dispatch notice exists

scripts/check-pr-fully-clean.py admits the dispatch acknowledgment comment as if it were a review, so a PR with no verdict at all reports clean.

The failure direction is the dangerous one: the script flips from correctly NOT-clean to falsely clean at the moment a review is dispatched, i.e. precisely when the review has not happened yet.

Reproduction, measured 2026-08-19 on this repo

PR #1711, zero comments:

$ python3 scripts/check-pr-fully-clean.py 1711 -R Morrison-Lab/ai-config; echo "rc=$?"
❌ PR is NOT fully clean:
  - No automated review comments or reviews found on PR #1711
rc=1

Correct. Then claude-review.yml was dispatched, which posts one comment:

👀 Claude Review Dispatched --- run reviewing PR #1711 at bf703fd.

With that single comment present, and the review job still in_progress:

$ python3 scripts/check-pr-fully-clean.py 1711 -R Morrison-Lab/ai-config; echo "rc=$?"
rc=0

Nothing about the PR's review state changed between those two runs except the arrival of a content-free notice.

Mechanism

check_review_comments admits a comment as a review item on author identity alone:

scripts/check-pr-fully-clean.py:527 --- if is_bot_author or is_review_header:
scripts/check-pr-fully-clean.py:145-150 --- _is_bot_author returns True for github-actions[bot], and for any login ending [bot].

The dispatch notice is posted by github-actions[bot], so is_bot_author is True and it lands in all_items. That makes the guard at line 545-547 stop firing:

if not all_items:
    issues.append(f"No automated review comments or reviews found on PR #{pr_num}")
    return False, issues

Every subsequent criterion is then evaluated over a body that contains no findings, because it contains nothing. Absence of findings is read as a clean verdict.

Why this matters more than an ordinary false positive

fully-clean.md already names the distinction this collapses:

"No verdict" is its own state, distinct from "a verdict with no findings". [...] Zero findings and zero review are indistinguishable by any count, and they call for opposite responses: one is done, the other needs a self-review and a re-run.

The script is the instrument that fragment mandates, and here it makes exactly the conflation the fragment warns against. It is also the specific instrument ardi's exit checklist requires exit 0 from, so a false 0 can carry a PR to merge with no review having occurred.

The window is narrow but systematically hit: on a workflow_dispatch-only review setup (which this repo has --- claude-review.yml carries no pull_request trigger), every PR passes through the state "dispatch notice posted, review still running", and that is exactly when an ARDI loop polls.

Suggested fix

Require an admitted item to carry evidence of being a review, rather than trusting the author. Options, roughly in order of preference:

Require a verdict-bearing marker for bot-authored comments too. is_review_header already exists and already matches the real markers (**Claude finished, ### Verdict). Dropping the is_bot_author or shortcut for comments would exclude the notice. Formal reviews (which carry a real commit.oid) can keep author-based admission, since the existing comment at lines 536-540 explains why that is sound there.
Explicitly skip known non-review bot notices --- dispatch acknowledgments, cost lines (💰 **Cost:**), preview-deploy stickies. A denylist is more fragile than (1) but smaller.
Treat "an in-flight review run exists" as not-clean, orthogonally to comments.

Option 1 looks strictly better than 2, and the header markers are already maintained in one place. Worth a test fixture named for this case, since the existing suite has a check-pr-fully-clean test block that would catch a regression.

Found while driving rme#1082 and #1711 to clean.

Same root cause, a second trigger --- and this one is permanent where the dispatch notice is transient.

Measured 2026-08-21 on #1841, head 158a82f2:

$ python3 scripts/check-pr-fully-clean.py 1841 -R Morrison-Lab/ai-config; echo "rc=$?"
Checking ARDI / fully-clean status for Morrison-Lab/ai-config#1841...
PR #1841 (fix/check-install-worktree): state=OPEN, HEAD=158a82f2 (committed 2026-08-21T21:39:44Z)
  verdict scan: examined 6 dated automated review item(s), 0 bore a verdict, latest = NONE
✓ Found clean review comment evaluating HEAD SHA 158a82f2

✅ Morrison-Lab/ai-config#1841 is FULLY CLEAN on HEAD 158a82f2!
rc=0

The admitted item is the quota/credential skip notice, not the dispatch acknowledgment:

Warning

Claude review skipped --- API credential or quota unavailable. ... View run

It is authored by github-actions[bot], so _is_bot_author admits it to all_items; it carries no commit_oid, so the _resolve_run_head_sha branch resolves its View run link to the run's head_sha, which equals HEAD; and it contains none of finding_patterns, so has_findings stays False and it reads as a clean review.

Why this trigger is worse than the dispatch one

The dispatch notice is a transient false clean: a real verdict lands minutes later and corrects it. A skip notice is a statement that no verdict is coming for this head at all, so the false clean is the terminal state. On this PR the reviewer failed three consecutive times (see #1048), and the checker reported clean every time.

The script already has the evidence to refuse

The contradiction is printed in its own output, two lines apart:

verdict scan: examined 6 dated automated review item(s), 0 bore a verdict, latest = NONE
✓ Found clean review comment evaluating HEAD SHA 158a82f2

check_latest_verdict() computed 0 bore a verdict and latest = NONE, and the clean path never consults it. Gating the ✓ Found clean review comment branch on the matched item actually bearing a verdict would close the dispatch-notice case and this one together, and would be a positive test for the good state rather than an absence-of-findings test --- which is fail-fast.md's objection to the current shape: a non-review and a finding-free review are presently the same observable.

Worth also treating latest = NONE as blocking on its own, since "examined N items, none bore a verdict" is exactly the unreviewed state.

Not the same as #1798

#1798 is about a human-authored self-review being counted. That is not this path: all_items admits only bot authors (or a CHANGES_REQUESTED/REJECTED state from anyone), so my own self-review and ARD comments on #1841 were correctly excluded. This is a bot comment that is genuinely a bot comment and genuinely not a review.

Correcting my own comment above. I wrote that all_items "admits only bot authors (or a CHANGES_REQUESTED/REJECTED state from anyone), so my own self-review and ARD comments on #1841 were correctly excluded."

That is wrong, and I had read the formal-review loop and generalized it to comments. The comment loop admits on is_bot_author OR is_review_header, where the header markers (### verdict, verdict:, code review, ...) are matched against the body with no author check at all.

Re-derived over #1841's actual comments rather than from the source alone:

ADMIT author=github-actions   bot=True   markers=[]                            len=363
ADMIT author=github-actions   bot=True   markers=[]                            len=363
ADMIT author=github-actions   bot=True   markers=[]                            len=363
ADMIT author=github-actions   bot=True   markers=[]                            len=363
ADMIT author=d-morrison       bot=False  markers=['### verdict', 'verdict:']    len=4804
ADMIT author=github-actions   bot=True   markers=[]                            len=363

My own 4804-character ARD comment was admitted, on markers it contains only because it quotes the cross-vendor reviewer's verdict line.

The conclusion of my comment above still holds --- the false clean on #1841 came from the skip notices, not from my comment --- but for a reason I had not identified: my comment failed the later HEAD-SHA match, because it cites 6a6fd05d rather than the current head, and the _resolve_run_head_sha fallback that could otherwise rescue a body with no SHA is itself gated on _is_bot_author.

So the human-comment admission was a near miss here, not an exclusion. Had my ARD comment happened to cite the current head SHA --- which a disposition comment written after a push routinely does --- it would have entered matching_items and been scanned as a review. That is #1798's failure, live, one coincidence away on this PR.

Two consequences for whoever fixes either issue:

The two issues are closer than I claimed. Both are "an item that is not a verdict is scanned as one"; check-pr-fully-clean.py counts a human-authored self-review as an external verdict, failing toward CLEAN #1798 is the human-authored route and this one is the bot-authored non-review route. A fix that gates the clean path on the matched item actually bearing a verdict closes both, which is an argument for doing that rather than tightening the two admission rules separately.
strip_cited_finding_vocab cuts the wrong way here. It exists so a clean verdict quoting finding vocabulary is not misread as raising a finding (check-pr-fully-clean.py false-positive: finding_patterns matches finding vocabulary quoted in a clean review body #1202) --- but it also means my comment's quoted `### Verdict: Needs more work` is blanked, so an admitted disposition comment reporting a blocking verdict would scan as finding-free.

Filed as a correction rather than edited into the comment above, so the wrong claim stays visible next to what replaced it.

Working on this --- paws off until I'm done.

fix: a workflow status notice is not a review (closes #1719)

fix: a workflow status notice is not a review (#1862)

fix(review): close structured-payload fail-opens found in post-merge …

feat(review): support structured review-bot output in check-pr-fully-…

Navigation Menu

Repository navigation

check-pr-fully-clean.py reports clean when only the review-dispatch notice exists #1719

Reproduction, measured 2026-08-19 on this repo

Mechanism

Why this matters more than an ordinary false positive

Suggested fix

Why this trigger is worse than the dispatch one

The script already has the evidence to refuse

Not the same as #1798

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

check-pr-fully-clean.py reports clean when only the review-dispatch notice exists #1719

Description

Reproduction, measured 2026-08-19 on this repo

Mechanism

Why this matters more than an ordinary false positive

Suggested fix

Activity

d-morrison commented on Aug 21, 2026

Why this trigger is worse than the dispatch one

The script already has the evidence to refuse

Not the same as #1798

d-morrison commented on Aug 21, 2026

d-morrison commented on Aug 21, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions