ci: parse the reviewer's verdict robustly (port ai-pack's parser)

[bawoodruff]

This started as a lost commit — pushed to ci/reviewer-turn-cap after #11 had already been squash-merged, so it never reached main. While re-landing it I found ai-pack had independently fixed a superset of the same bug, so this PR ports their parser instead.

The failure

The reviewer approved #5 on f8d8dbd — zero Critical, zero Major, zero Minor — and the review was recorded as COMMENTED:

state=COMMENTED  body starts: VERDICT: approve

jq -r .result carried a leading blank line through, so line 1 was empty, matched none of the exact cases, and fell to the *) branch that degrades to --comment. The same blank line shifted the body, which is the visible symptom.

Why the narrow fix wasn't enough

My first fix stripped leading blank lines. ai-pack hit the same class from the other direction on their PR #34: the model emitted a preamble paragraph before the verdict, which a blank-line strip does nothing for. Both are a line-1-only parse reading something that isn't the verdict.

So this ports ai-pack's parser: find the line that is exactly a verdict anywhere in the output, take everything after it as the body.

The ambiguity rule matters

Scanning past line 1 would, on its own, widen the injection surface — a verdict-shaped line quoted from a diff could win by appearing before the model's real one. The ambiguity rule closes it: a forged line necessarily produces a second match, and multiple matches can never resolve to --approve.

matches	outcome
0	--comment, whole output posted
1	that verdict; body is everything after it
2+	--request-changes if any match blocks (never lose a block), else --comment (never gain an approve)

Verified

input	result
leading blank line (mcp #5)	--approve ✅
preamble prose (ai-pack #34)	--approve ✅
normal line-1 verdict	--approve ✅
no verdict line	--comment
forged approve + real request-changes	--request-changes ✅
forged approve + real comment	--comment ✅

Body starts at the summary in every case; neither injection shape yields an approve.

Why this is urgent

The verdict is what the merge gate and shepherd-pr both read. An approval recorded as COMMENTED means the PR never reaches an approved state, and Route A of the shepherd can never fire — so shepherding a PR the reviewer is finished with polls until it exhausts its budget. Both are currently true of #5.

🤖 Generated with Claude Code

[github-actions]

Review Summary

• Files reviewed: .github/workflows/claude-pr-review.yml (only file changed, +11/-1)
• Overall verdict: APPROVE

Analysis

This re-lands a commit that was lost when a squash-merge on #11 shipped without it. The change fixes a real bug: when jq -r .result carries a leading blank line from the model's answer through to $REVIEW_RAW, FIRST_LINE (via head -n 1) was empty, matched no case, and silently fell through to --comment even when the model wrote VERDICT: approve — while tail -n +2 then left that verdict line sitting inside the posted body instead of being consumed as the verdict.

Traced both fixes by hand against the surrounding script (lines 583–620):

1. sed -i '/./,$!d' "$REVIEW_RAW" (line 597) — standard idiom for deleting leading blank lines: the range /./,$ never activates until the first non-blank line, so !d deletes everything before it. Verified this correctly makes the true verdict line become line 1, so both FIRST_LINE (head -n 1) and the body extraction (tail -n +2, which already existed) line up correctly afterward. No regression for the already-well-formed case (verdict on line 1 unaffected — the range activates immediately and nothing is deleted).
2. tr -d '\r' | sed 's/[[:space:]]*$//' on FIRST_LINE (line 600) — correctly strips a trailing CR (CRLF payload) and trailing spaces that would otherwise defeat the exact-string case match. Confirmed the case block's exact-match arms are unaffected for the normal (already-trimmed) input.
3. sed -i with no backup-suffix argument is GNU-sed syntax; the job runs on ubuntu-latest, so this is fine (would break on BSD/macOS sed, but that's not this runner).
4. No other use of $REVIEW_RAW in the file is affected by the in-place mutation — it's populated fresh from jq -r .result immediately beforehand and only read via head/tail afterward; the earlier mktemp at line 502 for the same variable name is unrelated (overwritten before use).

No Critical or Major issues found. Two theoretical, non-blocking nits, noted only as suggestions (not blocking this fix):

Minor Issues (optional)

• .github/workflows/claude-pr-review.yml:597 — /./,$!d treats a CRLF-encoded "blank" line (bare \r after the \n split) as non-blank, since . matches \r. If the model's JSON result field ever contains CRLF line endings, a leading blank line before the verdict would not be stripped by this pass (only the verdict line's own trailing CR is handled, via the separate tr -d '\r' on FIRST_LINE). Low likelihood given Claude's JSON output is \n-delimited in practice; fix would be sed -i 's/\r$//; /./,$!d' "$REVIEW_RAW" if ever observed.
• .github/workflows/claude-pr-review.yml:600 — only trailing whitespace is trimmed from FIRST_LINE; a verdict line with leading whitespace (e.g. " VERDICT: approve", not a separate blank line) still falls through to the *) default and silently degrades to --comment. Not a regression from this diff, but could be closed in the same pass with sed 's/^[[:space:]]*//; s/[[:space:]]*$//'.

Security Findings

None.

Positive Observations

• Fix is minimal, targeted, and matches the documented failure mode exactly (verified reasoning traces cleanly through the surrounding script).
• Comments are precise and falsifiable — they describe the actual bug mechanism (empty FIRST_LINE → default case → tail -n +2 misalignment) rather than vague rationale.
• No behavior change for the already-correct case (verdict on line 1), confirmed by manual trace of the sed range idiom.

[github-actions]

Review Summary

• Files reviewed: .github/workflows/claude-pr-review.yml (only file changed, +57/-8)
• Overall verdict: APPROVE

Analysis

This replaces the line-1-only verdict parser with a full-file scan for a verdict-shaped line, with an explicit ambiguity rule (0 matches → comment, 1 match → use it, 2+ matches → request-changes if any blocks, else comment — never resolves to approve on ambiguity). I traced the new logic (lines 619–648) by hand against every scenario the PR description claims to have verified, plus a few adversarial ones it doesn't mention:

• Leading blank line, preamble prose, normal line-1 verdict — all resolve correctly; BODY_START correctly advances past the matched line and sed '/./,$!d' still strips the resulting leading blank.
• No verdict line → falls through to -z "$VERDICT_LINE_NO", degrades to --comment with the full raw output as body — correct.
• 2+ matches (forged approve + real request-changes / forged approve + real comment) → the grep -q escalation check on line 629 correctly forces --request-changes whenever any matching line reads exactly request-changes, otherwise stays at the --comment default — matches the documented truth table, and the failure direction is always "never gain an approve."
• Self-referential risk (this diff modifies the very file whose comments describe the sentinel format): checked whether the new prose/table lines in the added comment block could themselves trip VERDICT_RE — they're all prefixed with # or |/table syntax, and the case-arm literals like "VERDICT:approve") have a leading " before the anchor, so none match ^[[:space:]]*VERDICT:...$. No accidental self-trip from this diff's own content.
• Whitespace handling: [[:space:]] in the anchor regex covers a stray trailing \r (CRLF), so the CR-stripping gap flagged as a minor nit on the prior push of this file no longer needs a separate tr -d '\r' — the new regex subsumes it.
• FIRST_LINE is fully removed with no dangling references (verified via grep across the whole file).

I wasn't able to execute a standalone reproduction of the bash state machine in this sandbox (its content-safety filter blocks writing scratch scripts containing [[:space:]]/${VAR:-default} patterns as "expansion obfuscation"), so the above is a careful manual trace rather than a run reproduction — flagging that for transparency, not as a finding.

No Critical or Major issues found.

Minor Issues (optional)

• .github/workflows/claude-pr-review.yml:629 — the request-changes escalation regex (^[[:space:]]*VERDICT:[[:space:]]*request-changes[[:space:]]*$) duplicates part of VERDICT_RE (line 619) as a second literal. If the accepted-word set or anchoring in VERDICT_RE is edited later without updating this copy, the "never lose a block" guarantee can silently drift out of sync. Consider deriving it from VERDICT_RE, e.g. grep -q -E "$VERDICT_RE" ... combined with a check on the captured word, or a shared VERDICT_WORD_RE fragment reused in both places.
• No automated test (bats/shellspec/etc.) exercises this parser's branch matrix (0/1/2+ matches, escalation priority) — the PR body's verification table is manual only. Given this logic gates every PR's approve/block outcome, a small script-level test asserting the six documented scenarios would catch regressions before they reach production; not blocking since the file has no existing test harness to extend.
• The widened scan (beyond line 1) is, by design, more exposed to an accidental duplicate: if the reviewing model ever explains the sentinel format using a bare, unindented, unbacticked line that happens to read exactly VERDICT: approve (not the case in this diff's own added comments, but plausible in a model-generated review body), a legitimate single verdict silently downgrades to --comment. This is the documented safe-direction tradeoff and not a defect, just worth keeping in mind if reviews start mysteriously landing as comments instead of approvals.

Security Findings

None. The ambiguity-resolution design correctly fails toward blocking/commenting rather than approving in every adversarial case traced (forged verdict lines quoted from diff content can force a block or downgrade an approve, but can never manufacture an approve).

Positive Observations

• The ambiguity rule (2+ matches never resolves to approve) is the right fix for the injection-surface tradeoff the diff itself calls out, and holds up under hand-tracing of the forged-approve scenarios.
• Comments are precise, falsifiable, and reference concrete prior incidents (mcp #5, ai-pack #34) with the actual failure mechanism rather than vague rationale.
• ${VERDICT_FLAG#--} in the warning message is a nice small touch for readable log output.
• Old FIRST_LINE/case-statement code is fully and cleanly removed with no orphaned references.