classify-review-verdict: a verdict that correctly disclaims being a fully-clean claim is read as needs-more-work, failing require-clean-verdict on an approving review

[d-morrison]

What happened

review / require-clean-verdict failed on Morrison-Lab/ai-config#3361 at 3dc2ad75 (run) over a review whose stated verdict is Ready for merge and whose findings list is empty.

The reviewer did everything right. It ran check-pr-fully-clean.py, saw validate and review / claude-review still in_progress, and — following the brief's own verdict-semantics instruction, which it quotes — distinguished a content verdict on the diff from a fully-clean claim about the PR. That distinction is what the classifier reads as a rejection.

Reproduction

Fetched the exact comment body and ran the shipped classifier against it:

$ bash classify-review-verdict.sh review.txt
clean=false
verdict=needs-more-work

$ printf '### Verdict\n\n**Ready for merge**\n' > plain.txt
$ bash classify-review-verdict.sh plain.txt
clean=true
verdict=ready-for-merge

Bisected by substituting one clause at a time. Exactly one flips it, and it is inside the verdict section:

Clause substituted	Result
full body, unmodified	clean=false needs-more-work
opening prose "not yet fully clean" → "fully clean"	clean=false (irrelevant)
"Clean stopping point reached" → "Finished"	clean=false (irrelevant)
"it is not a claim that the PR is fully clean end-to-end" → "it describes the diff only"	clean=true ready-for-merge
verdict section alone	clean=false

So it is not the opening prose and not the stopping-point line. It is a negated positive inside the verdict section, which the last-match-wins scan picks over the **Ready for merge** line above it.

Why this is not simply the classifier being wrong

The classifier is behaving to its documented contract: the verdict section must state an affirmative clean conclusion and not state an unnegated rejection, and a negated positive counts. NOT_CLEAN (#827) and payload finding-vocabulary (#819) are the same mechanism. Narrowing the keyword list would re-open those.

The defect is a tension between two instructions, not a bug in either:

• The review brief tells the reviewer to qualify a verdict when checks are still in flight ("checks that are merely queued or in progress alongside you are the merge gate's business, not the verdict's").
• The classifier forbids expressing that qualification inside the verdict section.

A reviewer that follows the first faithfully fails the second. The more careful the reviewer, the likelier the false red — which is the wrong incentive gradient.

Relationship to prior issues

• Reviewer sandbox cannot read CI, so verdicts stay CI-qualified and check-pr-fully-clean.py rejects them #768 ("Reviewer sandbox cannot read CI, so verdicts stay CI-qualified") is the same family from the opposite side: there the reviewer could not see CI. Now it can, runs the instrument, and qualifies the verdict on evidence. This is that fix's downstream consequence rather than a recurrence.
• claude-code-review: the review job needs checks: read; without it check-runs return 403 and a clean review calls itself blocked #829 (review job needs checks: read) is what made the reviewer able to read CI at all.
• classify-review-verdict: triage-exemption verdict ("No action -- does not need code review") reads as unrecognized while review-data says CLEAN #845 added structured-payload precedence. This review emitted no review-data payload, so it fell through to the prose scan. Had it emitted one with "verdict": "CLEAN", classify-review-verdict: triage-exemption verdict ("No action -- does not need code review") reads as unrecognized while review-data says CLEAN #845's precedence would have classified it correctly and this would not have fired.

That last point suggests the cheapest fix.

Suggested directions, roughly in order of cost

1. Make the structured payload non-optional in the brief. classify-review-verdict: triage-exemption verdict ("No action -- does not need code review") reads as unrecognized while review-data says CLEAN #845 already gives it precedence, so a review that always emits review-data is immune to every prose-scan trap, this one included. The failing review simply omitted it. This needs no classifier change.
2. Scope the prose scan's rejection test to the verdict statement line rather than the whole verdict section, so a following sentence of scope-limiting prose cannot outrank it. Riskier: it narrows the window that classify-review-verdict: finding vocabulary inside the structured review-data payload overrides the stated verdict #819 widened for good reasons.
3. Teach the brief a fixed form for a scope-limited verdict that carries no negated positive — e.g. "Ready for merge (diff content; merge gate pending)" — so the qualification survives the scan. Cheapest to state, but it is a convention the reviewer has to remember, which is the weakest kind of mechanism.

I have no strong view between 1 and 2; 1 looks like it removes the class rather than this instance.

Impact

Every consumer running review / require-clean-verdict as a required check. A false red here blocks merge on an approving review, and the failure reads as a genuine not-clean verdict to anyone who does not open the classifier.

---

Filed by Claude Code (AI agent) --- not written by a human. Encountered while running a UMS pass on ai-config#3361; that PR is currently blocked by this.

[d-morrison]

Suggested fix 1 confirmed empirically, at no extra cost

The next round on the same PR (Morrison-Lab/ai-config#3361, run 34293392938, commit d55628cc) did emit a review-data payload:

{
  "schema_version": "1.1",
  "verdict": "CLEAN",
  "findings": []
}

and review / require-clean-verdict came back success:

review / require-clean-verdict   success
review / require-review          success

Its prose still carries scope-limiting language of the same kind that failed the previous round. The difference is only that #845's payload precedence short-circuited the prose scan.

So the two rounds are a natural A/B on one PR, one day apart, same reviewer, same repo:

Round	Payload emitted	require-clean-verdict
1 (3dc2ad75)	no	failure
2 (d55628cc)	yes	success

That is about as clean a confirmation as this is going to get without a code change: the payload is already sufficient; it is just not guaranteed. Making it non-optional in the brief removes the class rather than this instance, and needs no change to classify-review-verdict.sh — so it does not risk re-opening #819 or #827 the way narrowing the prose scan would.

The remaining question is how to make it reliable, since "the brief asks for it" is the weakest rung. A structural option, if the reviewer's output shape allows it: treat a missing payload as a review-delivery defect in its own right rather than silently falling back to the prose scan — the fallback is what makes the omission invisible.

One caveat on the payload itself, noticed while reading it: round 2's commit_sha field reads 88450ef0722edd335b354ca867292d2473d61828 while the comment's own trailer reads Reviewed commit: d55628cc8a41a8089e3be23e839bcf55cc0fe46b. The first looks like the PR's merge commit and the second like the branch head. Harmless here, but if anything downstream ever matches the payload's commit_sha against the PR head to decide staleness, it would not match. Not filing that separately until someone confirms whether anything reads that field.

---

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

---

Generated by Claude Code