feat(claude-review): opt-in require-clean-verdict job (closes #767)

[dem-extra1]

Closes #767.

Summary

Adds the opt-in require-clean-verdict gate job to claude-code-review.yml as a sibling to require-review.

• Passes (green) when Claude review completed and produced an affirmatively clean verdict ("Ready for merge").
• Fails (red) when the review verdict is not clean ("Needs more work", changes requested, blocked, etc.) or when the review failed.
• Skips (gray) on the exact same graceful-skip paths require-review uses (draft, fork, bot author, quota exhaustion, self-mod skip, cancellation, or stale head).
• Consumers can add review / require-clean-verdict to their branch protection / repository rulesets to enforce server-side merge blocking against unaddressed findings.

Draft PR opened on claim per pr-on-claim.

[github-actions]

PR Preview Action v1.8.1
🚀 Preview available at: https://Morrison-Lab.github.io/gha/pr-preview/pr-790/

_{Built to branch gh-pages at 2026-08-31 07:04 UTC.}

[github-actions]

Claude finished review — View run

---

Review: #790 — third round

This is a re-review after commit dbe9651 ("fix(classify-verdict): robust multi-line verdict section classification and defanged negations, closes #767"). Reviewed commit: dbe9651817a2f0de7753935b54eb8a67f6a4bd51 (PR head per gh pr view --json headRefOid; the merge commit 06c9f59 in this checkout wraps it). Per the computed range, only two files changed since the last round: .github/workflows/scripts/classify-review-verdict.sh (full rewrite, now Python-based) and its test file (run-classify-review-verdict-tests.sh, +31 lines, tests 21–27).

Prior findings: status

Round 2 Finding A (script committed non-executable, invoked with no bash prefix) — fixed. The saved PR diff shows the file re-added as new file mode 100755 (executable), and the "Classify review verdict" step's run: body now reads bash .github/workflows/scripts/classify-review-verdict.sh "$REVIEW_TEXT_FILE", matching the bash "$SCRIPT" convention every sibling script in this directory uses. This part of the diff is unchanged since last round (not part of this round's edited files), but I re-verified it directly against the saved diff since it's the highest-severity item from the last round.

Round 2 Finding 3 (temp file never removed) — moot, not just fixed. The script no longer creates any temp file at all: this round's rewrite replaced the earlier bash/mktemp-based extraction with a self-contained Python heredoc that reads the review file directly via sys.argv. Nothing to clean up.

Round 1 Finding 1 / Round 2's two adversarial cases — still hold (re-verified by re-reading the code; not re-executed since the two originally-reported cases were already confirmed fixed last round and this round's diff didn't regress that specific logic).

New findings

1. (Blocking, correctness) The rewritten classifier still resolves the "which statement is the real, current verdict" question incorrectly whenever the final verdict section mixes bolded and unbolded text — and one of the two failure directions is the dangerous one the whole feature exists to prevent. (.github/workflows/scripts/classify-review-verdict.sh:96-137)

Round 2's fix correctly anchors extraction on the last verdict-header line (.../classify-review-verdict.sh:150-160 in the prior version; now the last_idx loop near the top of this file), which fixed the "wrong section" bug. But within that correctly-anchored final section, the script still resolves conflicting statements incorrectly, via two related mechanisms:

• Bold status short-circuits and ignores everything after it, even when it's not the final word. Lines 96–116 scan content_lines for **bolded** status tokens ("last bold wins"), and if any bold match is found anywhere in the section, line 115 (if last_bold_verdict is not None: record(*last_bold_verdict)) returns immediately — permanently skipping the plain-text scan in step 2, even when a later, unbolded line states a different, more current conclusion.
• The plain-text scan (step 2, lines 118–137) is not "last line wins" either. It runs two separate loops in a fixed priority order: first scan content_lines for any unnegated non-clean keyword and return on the first match (lines 120–128); only if none is found, scan again for a clean keyword and return on the first match (lines 131–137). This is neither chronological ("what does the section conclude, in order") nor deterministic about recency.

I reconstructed the current script byte-for-byte from the diff and ran it against two adversarial cases within a single, correctly-anchored ### Verdict section (i.e., not spanning two different header lines, so Round 2's last-header-anchor fix doesn't help):

### Verdict

**Ready for merge** — as of my last look.

Wait, I just noticed the tests are still failing, so this actually needs more work.

→ actual output: clean=true verdict=ready-for-merge (should be clean=false) — a genuinely-broken PR reads as clean, because the bolded "Ready for merge" short-circuits before the unbolded correction on the next line is ever read. This is exactly the highest-severity failure mode Round 1 originally flagged ("a genuinely-broken PR reads as clean… defeating the gate's entire purpose"), reproduced again through a different code path.

### Verdict

The prior round said this needs more work, citing line 42. That's since been fixed.

Ready for merge — all clear now.

→ actual output: clean=false verdict=needs-more-work (should be clean=true) — a fixed/clean PR is permanently red-blocked, because the plain-text non-clean scan matches the first (historical) mention and returns before ever reaching the actual, final "Ready for merge" line. Note this specific unbolded-declarative-sentence style (no **bold**, no Verdict: label prefix, so it can't self-re-anchor via the header regex) is not hypothetical — the PR's own fixture set (Test 18: verdict-split-across-blocks) shows this repo's real review output does write plain, unbolded verdict lines like Ready for merge - no blocking findings. and Verdict: Ready for merge. in the same document.

None of the 27 shipped test cases exercise a mixed bold/unbolded pair within a single anchored section — tests 21/22 (the cases from Round 1) both put the conflicting statement before the last header line, which the header-anchor fix already handles; tests 25–27 mix a plain non-clean phrase with a plain clean phrase but only ever in the one order that happens to produce the right answer by coincidence of the fixed loop-priority (non-clean-first), never the reverse order, and never a bold+plain combination.

Fix direction: replace the three-pass, mismatched-priority classification (bold-first-with-hard-short-circuit, then non-clean-first, then clean-first) with a single ordered scan over content_lines — bold and plain alike, in document order — that keeps updating a "last known verdict" as it goes and returns whatever was seen last, the same "last one wins" principle Round 2 already applied to header selection. This is the same self-implicating-example hazard CLAUDE.md documents extensively for this exact review workflow (detect-review-request's markup stripper, compose-review-failure-report's redaction tests): a detector that must classify prose written by this very review process, which routinely restates a prior round's verdict before giving the current one — and the fix for the outer instance of the bug (which header to anchor on) left an inner instance (which line within that section is authoritative) unaddressed.

Other checks performed (no new issues)

• run-classify-review-verdict-tests.sh's new tests 21–27 correctly pin what they claim to pin (the header-anchor fix and a handful of ordering cases that happen to resolve correctly under the current priority scheme); I don't see any incorrect assertions in the new test cases themselves.
• The Python heredoc is invoked with << 'EOF' (quoted delimiter, no shell interpolation) and takes the review-file path and $GITHUB_OUTPUT via sys.argv rather than string-interpolating file contents into the script — safe, and consistent with CLAUDE.md's denied_tools-interpolation warning (this isn't that pattern).
• record()'s GITHUB_OUTPUT handling (checks os.path.exists before appending) is correct and matches the test harness's usage.
• No workflow-YAML or CLAUDE.md changes this round to re-check (confirmed via the computed diff range).

Verdict

Needs more work — the header-anchoring fix from the last round is correct and holds, and the executable-bit/bash-prefix bug from Round 2 is fixed, but this round's rewrite introduces two new, reproduced instances of the same underlying bug class within the correctly-anchored final section: a bolded status can short-circuit past a later plain-text correction (the dangerous direction — a broken PR reads as clean), and a plain-text historical mention can pre-empt a later plain-text real verdict (blocks a clean PR permanently). Both should be fixed — by unifying the three classification passes into one document-order, last-statement-wins scan — before this is ready to merge.

Reviewed commit: dbe9651

[github-actions]

💰 Cost: $1.2341 (review) — run