From 356622e061d39bc1393441de00f1c4b74f480a67 Mon Sep 17 00:00:00 2001 From: Bryan Woodruff Date: Sat, 29 Aug 2026 07:17:35 -0700 Subject: [PATCH 1/2] ci: don't lose an approval to a leading blank line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewer approved PR #5 on f8d8dbd — zero Critical, Major, and Minor findings, first line of its answer "VERDICT: approve" — and the review was recorded as COMMENTED. Cause is the verdict parse. `jq -r .result` can carry a leading blank line through from the model's answer, so FIRST_LINE came back empty, matched none of the exact cases, and fell to the `*)` branch that degrades to --comment. The same blank line also shifted the body: `tail -n +2` strips line one unconditionally, so the posted review kept "VERDICT: approve" as its own first line — which is the visible symptom, and how this was spotted. The consequence is bigger than a mislabelled review. The verdict is what the merge gate and the shepherd skill both read: an approval recorded as COMMENTED means Route A can never fire, and shepherding a PR the reviewer is finished with would poll until it exhausted its wait budget. Strip leading blank lines before reading the verdict, and trim trailing CR and spaces from that line too, since the cases are exact matches and would fail the same way. Verified against the exact payload that failed: it now selects --approve and the posted body starts at the summary. Caught only because the parse degrades quietly — it warns, but the run is green and the review still posts, so nothing draws attention to it. Worth treating a non-matching verdict line as louder than a warning at some point. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/claude-pr-review.yml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index a840b9b..a8c33c3 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -586,8 +586,18 @@ jobs: # posted as data via --body-file. The verdict line is matched # strictly; anything malformed degrades to a plain comment so a # confused (or manipulated) run can never accidentally approve. + # Drop leading blank lines before reading the verdict. The model can + # open its answer with one, and `jq -r .result` carries it through — + # which makes FIRST_LINE empty, matches no case, and silently + # degrades the run to --comment while `tail -n +2` leaves the + # "VERDICT: approve" line sitting at the top of the posted body. + # Observed on PR #5 f8d8dbd: the reviewer approved with zero findings + # at every severity and the review was recorded as COMMENTED, which + # also means Route A of the shepherd skill can never fire. + sed -i '/./,$!d' "$REVIEW_RAW" VERDICT_FLAG="--comment" - FIRST_LINE="$(head -n 1 "$REVIEW_RAW")" + # Trailing CR or spaces would also defeat the exact-match cases below. + FIRST_LINE="$(head -n 1 "$REVIEW_RAW" | tr -d '\r' | sed 's/[[:space:]]*$//')" case "$FIRST_LINE" in "VERDICT: approve") VERDICT_FLAG="--approve" ;; "VERDICT: request-changes") VERDICT_FLAG="--request-changes" ;; From cb1a435a55224a5ba5303896fd69449695e45f9a Mon Sep 17 00:00:00 2001 From: Bryan Woodruff Date: Sat, 29 Aug 2026 07:26:52 -0700 Subject: [PATCH 2/2] ci: adopt ai-pack's verdict parser instead of the narrower fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit stripped leading blank lines, which fixes the failure observed here but not the general one. ai-pack hit this first, from the other direction: on its PR #34 the model emitted a preamble paragraph before the verdict, which a blank-line strip does nothing for. Both failures are the same shape — a line-1-only parse reading something that is not the verdict — and both are silent where it matters, since an approve that never registers leaves reviewDecision empty. Port their parser rather than keep a fix that handles one of the two cases. It locates the line that is exactly a verdict anywhere in the output and takes everything after it as the body. Their reasoning about the injection surface came with it and is worth keeping verbatim in the comment: scanning past line 1 would, alone, widen the surface, because a verdict-shaped line quoted from the diff could win by appearing before the model's real one. The ambiguity rule closes that — a forged line necessarily produces a second match, and multiple matches can never resolve to --approve. Two or more matches take --request-changes if any of them requests changes, otherwise --comment, so the reachable outcomes are strictly safer than the line-1 parser rather than merely equivalent. Verified across six inputs: the mcp blank-line case, the ai-pack preamble case, a normal line-1 verdict, no verdict at all, and both injection shapes — a forged approve alongside a real request-changes, and alongside a real comment. Neither injection case yields an approve. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/claude-pr-review.yml | 75 +++++++++++++++++++------- 1 file changed, 57 insertions(+), 18 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index a8c33c3..a8d7c90 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -586,27 +586,66 @@ jobs: # posted as data via --body-file. The verdict line is matched # strictly; anything malformed degrades to a plain comment so a # confused (or manipulated) run can never accidentally approve. - # Drop leading blank lines before reading the verdict. The model can - # open its answer with one, and `jq -r .result` carries it through — - # which makes FIRST_LINE empty, matches no case, and silently - # degrades the run to --comment while `tail -n +2` leaves the - # "VERDICT: approve" line sitting at the top of the posted body. - # Observed on PR #5 f8d8dbd: the reviewer approved with zero findings - # at every severity and the review was recorded as COMMENTED, which - # also means Route A of the shepherd skill can never fire. - sed -i '/./,$!d' "$REVIEW_RAW" + # The verdict is REQUESTED on line 1, but the model does not reliably + # comply. Two observed failures of a line-1-only parse, both of which + # silently downgraded a clean approve to a plain comment: + # - mcp PR #5 f8d8dbd: `jq -r .result` carried a leading blank line + # through, so line 1 was empty. + # - ai-pack PR #34: a preamble paragraph preceded the verdict. + # Both are silent where it matters — an approve that never registers + # leaves reviewDecision empty, so the bot approval the merge gate + # depends on simply never happens, and shepherd-pr's Route A can + # never fire. + # + # This parser is ported from ai-pack, which hit the preamble case + # first and reasoned the rule through: locate the line that is + # exactly a verdict (surrounding whitespace tolerated) anywhere in + # the output, and treat everything after it as the body. + # + # Scanning beyond line 1 would, on its own, WIDEN the injection + # surface: under a line-1 parse a forged verdict had to occupy line + # 1, whereas a positional scan lets a verdict-shaped line quoted from + # the diff win by appearing before the model's real one. The + # ambiguity rule removes that — a forged line necessarily produces a + # SECOND match, and multiple matches can never resolve to --approve: + # 0 matches -> --comment, whole output posted + # 1 match -> that verdict, body is everything after it + # 2+ matches -> --request-changes if ANY match requests changes + # (never lose a block), else --comment (never gain an + # approve). Whole output posted so a human can see + # what made it ambiguous. + # A legitimate review that quotes a verdict line is downgraded to a + # comment. That is the intended direction of failure. + VERDICT_RE='^[[:space:]]*VERDICT:[[:space:]]*(approve|comment|request-changes)[[:space:]]*$' VERDICT_FLAG="--comment" - # Trailing CR or spaces would also defeat the exact-match cases below. - FIRST_LINE="$(head -n 1 "$REVIEW_RAW" | tr -d '\r' | sed 's/[[:space:]]*$//')" - case "$FIRST_LINE" in - "VERDICT: approve") VERDICT_FLAG="--approve" ;; - "VERDICT: request-changes") VERDICT_FLAG="--request-changes" ;; - "VERDICT: comment") VERDICT_FLAG="--comment" ;; - *) echo "::warning title=Unrecognized verdict line::'${FIRST_LINE}' — posting as plain comment." ;; - esac + BODY_START=1 + # grep -c prints 0 and exits 1 on no match; keep the count, drop the status. + MATCH_COUNT="$(grep -c -E "$VERDICT_RE" "$REVIEW_RAW" || true)" + MATCH_COUNT="${MATCH_COUNT:-0}" + VERDICT_LINE_NO="$(grep -n -m1 -E "$VERDICT_RE" "$REVIEW_RAW" | cut -d: -f1 || true)" + if [ -z "$VERDICT_LINE_NO" ]; then + echo "::warning title=No verdict line::output contains no 'VERDICT: ' line — posting the whole output as a plain comment." + elif [ "$MATCH_COUNT" -gt 1 ]; then + if grep -q -E '^[[:space:]]*VERDICT:[[:space:]]*request-changes[[:space:]]*$' "$REVIEW_RAW"; then + VERDICT_FLAG="--request-changes" + fi + echo "::warning title=Ambiguous verdict::${MATCH_COUNT} verdict-shaped lines in the output (quoted diff content, or a confused run). Refusing to approve on ambiguity — posting as ${VERDICT_FLAG#--} with the full output as the body." + else + # Whitespace-stripped so the match is exact regardless of spacing; + # grep already constrained the value to the three legal verdicts. + case "$(sed -n "${VERDICT_LINE_NO}p" "$REVIEW_RAW" | tr -d '[:space:]')" in + "VERDICT:approve") VERDICT_FLAG="--approve" ;; + "VERDICT:request-changes") VERDICT_FLAG="--request-changes" ;; + "VERDICT:comment") VERDICT_FLAG="--comment" ;; + esac + BODY_START=$((VERDICT_LINE_NO + 1)) + if [ "$VERDICT_LINE_NO" -ne 1 ]; then + echo "::warning title=Verdict not on first line::model emitted preamble; verdict found on line ${VERDICT_LINE_NO} and parsed. The preamble is dropped from the posted body." + fi + fi REVIEW_BODY="$(mktemp)" - tail -n +2 "$REVIEW_RAW" | sed '/./,$!d' > "$REVIEW_BODY" + tail -n +"$BODY_START" "$REVIEW_RAW" | sed '/./,$!d' > "$REVIEW_BODY" if [ ! -s "$REVIEW_BODY" ]; then echo "::warning title=Empty review body::model produced no review text; posting raw output as comment." cp "$REVIEW_RAW" "$REVIEW_BODY"