Skip to content

Commit ab8fcff

Browse files
zfarrellzfarrell
committed
fix(review): stop pinning a gh flag the runner rejects
1 parent 6f3eaa4 commit ab8fcff

5 files changed

Lines changed: 200 additions & 25 deletions

File tree

‎.github/workflows/claude-pr-review.yml‎

Lines changed: 60 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -78,12 +78,25 @@ jobs:
7878
- name: Gather review context
7979
if: github.event.pull_request.user.login != 'dependabot[bot]'
8080
id: context
81-
# Eight API reads feed the prompt now. A failure in this step *skips* the review
81+
# Nine API reads feed the prompt now. A failure in this step *skips* the review
8282
# step, and with it the notify step's failure check, so the PR would get no review
8383
# and no explanation. Every command below is guarded individually; this is the
84-
# backstop that keeps a bug in one block from costing the PR its review, at the cost
85-
# of the prompt losing whichever blocks had not been written yet.
84+
# backstop that keeps a bug in one block from costing the PR its review.
85+
#
86+
# What it costs when it fires: pr_context is appended to $GITHUB_OUTPUT once, at the
87+
# end, so an abort anywhere before that leaves the output unset and the prompt gets
88+
# an *empty* <pr_context> -- not a partial one. review_cycle and threads are written
89+
# earlier and survive. Each degraded block carries a sentence saying what is
90+
# missing, but a degraded step carries nothing, so the prompt tells the reviewer to
91+
# fetch what it needs itself when the block is empty. Without that line the prompt
92+
# would be telling it not to re-fetch context it never received.
8693
continue-on-error: true
94+
# Explicitly, because the default for a run block is `bash -e {0}` -- no pipefail.
95+
# This step is mostly `gh ... | jq` pipelines, and gh writes its error body to
96+
# stdout, so without pipefail a failed fetch feeds its own error text to jq and the
97+
# block renders whatever jq makes of it instead of the guarded fallback sentence.
98+
# tests/context-step-test.sh runs the extracted script under the same shell.
99+
shell: bash
87100
run: |
88101
PR_NUMBER=${{ github.event.pull_request.number }}
89102
REPO=${{ github.repository }}
@@ -97,6 +110,22 @@ jobs:
97110
CTX="${RUNNER_TEMP}/pr-context.md"
98111
: > "$CTX"
99112
113+
# gh refuses a raw-text body containing ANSI colour unless told to allow escape
114+
# sequences, and a diff or a job log earns an escape byte from any file holding
115+
# terminal output -- this repository's own job-log fixtures do. That refusal and
116+
# its --allow-escape-sequences opt-out arrived together in gh 2.97.0 as a security
117+
# fix; ubuntu-latest ships 2.96.0, where the flag is an unknown-flag error and the
118+
# refusal does not exist either. So every raw fetch tries the flag and falls back
119+
# to the bare call: on 2.96 the first attempt fails and the second succeeds, on
120+
# 2.97+ the first succeeds. Pinning either form breaks on the other, and the
121+
# runner image updates weekly.
122+
fetch_raw() {
123+
RAW_OUT=$1
124+
shift
125+
gh "$@" --allow-escape-sequences > "$RAW_OUT" 2>/dev/null && return 0
126+
gh "$@" > "$RAW_OUT" 2>/dev/null
127+
}
128+
100129
# Count distinct commits already reviewed, never review state: the org ruleset
101130
# sets dismiss_stale_reviews_on_push, so a push flips a prior APPROVED to
102131
# DISMISSED and a state filter stops matching it. Inline comments each create
@@ -240,10 +269,7 @@ jobs:
240269
for JOB_ID in $JOB_IDS; do
241270
JOB_LOG="${RUNNER_TEMP}/job-${JOB_ID}.log"
242271
{ echo; echo "### Failing job ${JOB_ID}"; } >> "$CTX"
243-
# --allow-escape-sequences or gh refuses the body and writes nothing at all:
244-
# job logs carry ANSI colour, and without the flag this is always "unavailable".
245-
if ! gh api --allow-escape-sequences \
246-
"repos/${REPO}/actions/jobs/${JOB_ID}/logs" > "$JOB_LOG" 2>/dev/null; then
272+
if ! fetch_raw "$JOB_LOG" api "repos/${REPO}/actions/jobs/${JOB_ID}/logs"; then
247273
echo "(log unavailable)" >> "$CTX"
248274
continue
249275
fi
@@ -276,14 +302,22 @@ jobs:
276302
SINCE_FILE="${RUNNER_TEMP}/since-last-review.diff"
277303
# The compare API, not git: the checkout is fetch-depth 1, so no base branch and
278304
# no prior commit exists locally to diff against.
279-
# --allow-escape-sequences on every call whose body is raw text rather than
280-
# JSON: gh refuses such a body outright and writes nothing. A diff earns an
281-
# escape byte from any fixture holding terminal output -- this repository's own
282-
# job-log fixtures do -- and without the flag the most important block in the
283-
# prompt goes missing behind a warning.
284-
if gh api "repos/${REPO}/compare/${LAST_SHA}...${HEAD_SHA}" \
285-
-H "Accept: application/vnd.github.diff" --allow-escape-sequences \
286-
> "$SINCE_FILE" 2>/dev/null; then
305+
#
306+
# Ask for the JSON first and only use the diff when the comparison is a clean
307+
# fast-forward. compare/A...B is three-dot, so it diffs from the *merge base* of
308+
# the two, which equals "since A" only while the branch has done nothing but gain
309+
# commits. After a rebase or a squash-and-force-push the old SHA usually stays
310+
# reachable, so this call succeeds and returns the whole PR plus anything the
311+
# rebase pulled in from upstream -- under a heading that says the opposite. A
312+
# reviewer trusting that heading re-raises issues the author already settled, and
313+
# SINCE_MAX can drop the part that genuinely is new. status is "ahead" only for
314+
# the fast-forward case; "diverged" and "behind" fall through to the message.
315+
COMPARE_STATUS_JQ='.status // "unknown"'
316+
SINCE_STATUS=$(gh api "repos/${REPO}/compare/${LAST_SHA}...${HEAD_SHA}" 2>/dev/null \
317+
| jq -r "$COMPARE_STATUS_JQ" 2>/dev/null) || SINCE_STATUS='unknown'
318+
if [ "$SINCE_STATUS" = "ahead" ] \
319+
&& fetch_raw "$SINCE_FILE" api "repos/${REPO}/compare/${LAST_SHA}...${HEAD_SHA}" \
320+
-H "Accept: application/vnd.github.diff"; then
287321
# awk, not `wc -l`: wc pads its count with spaces on BSD and the number
288322
# is interpolated into the notice below, not just compared.
289323
SINCE_LINES=$(awk 'END {print NR}' "$SINCE_FILE")
@@ -299,15 +333,17 @@ jobs:
299333
{
300334
echo
301335
echo "## Diff since your last review"
302-
echo "Unavailable: ${LAST_SHA} could not be compared to ${HEAD_SHA}."
303-
echo "The branch was probably force-pushed. Review the full diff instead."
336+
echo "Unavailable: ${LAST_SHA} does not fast-forward to ${HEAD_SHA}"
337+
echo "(comparison status: ${SINCE_STATUS})."
338+
echo "The branch was rebased or force-pushed, so there is no meaningful"
339+
echo "\"since last review\" diff. Review the full diff below instead, and"
340+
echo "read the prior review comments to see what was already raised."
304341
} >> "$CTX"
305342
fi
306343
fi
307344
308345
DIFF_FILE="${RUNNER_TEMP}/pr.diff"
309-
if gh pr diff "$PR_NUMBER" --repo "$REPO" --allow-escape-sequences \
310-
> "$DIFF_FILE" 2>/dev/null; then
346+
if fetch_raw "$DIFF_FILE" pr diff "$PR_NUMBER" --repo "$REPO"; then
311347
DIFF_LINES=$(awk 'END {print NR}' "$DIFF_FILE")
312348
{
313349
echo
@@ -420,11 +456,14 @@ jobs:
420456
# norm strips the wrappers the reviewer puts in front of a real command (timeout,
421457
# cd .. &&, env VAR=x) so they do not all collapse into "other". verb returns the
422458
# first matching label or "other" -- the output is always one of these literals.
423-
CMD_JQ='def norm: sub("^\\s+"; "") | sub("^timeout\\s+[0-9]+m?\\s+"; "") | sub("^cd\\s+[^&|;]+&&\\s*"; "") | sub("^env\\s+\\S+=\\S+\\s+"; ""); def verb: . as $c | ([[["^gh\\s+pr\\s+diff", "gh pr diff"], ["^gh\\s+pr\\s+view", "gh pr view"], ["^gh\\s+pr\\s+checks", "gh pr checks"], ["^gh\\s+pr\\s+review", "gh pr review"], ["^gh\\s+pr\\s+comment", "gh pr comment"], ["^gh\\s+api", "gh api"], ["^gh\\s", "gh other"], ["^git\\s+diff", "git diff"], ["^git\\s+log", "git log"], ["^git\\s+show", "git show"], ["^git\\s+blame", "git blame"], ["^git\\s", "git other"], ["^rg\\b", "rg"], ["^grep\\b", "grep"], ["^(fd|find)\\b", "find"], ["^(ls|tree)\\b", "ls"], ["^(sed|awk)\\b", "sed/awk"], ["^(cat|head|tail|wc)\\b", "cat/head/tail"], ["^(pytest|uv|python3?|cargo|npm|pnpm|yarn|bun|node|go|make|ruff|mypy|pyflakes)\\b", "run tests/build"]][] | select(.[0] as $re | $c | test($re))] | .[0] // ["", "other"]) | .[1]; def classify: {cmd: (norm | verb), compound: test("\\||&&|;|>")}; def toolname: if type == "string" and test("^[A-Za-z0-9_-]{1,64}$") then . else "unknown" end;'
459+
CMD_JQ='def norm: sub("^\\s+"; "") | sub("^timeout\\s+[0-9]+m?\\s+"; "") | sub("^cd\\s+[^&|;]+&&\\s*"; "") | sub("^env\\s+\\S+=\\S+\\s+"; ""); def verb: . as $c | ([[["^gh\\s+pr\\s+diff", "gh pr diff"], ["^gh\\s+pr\\s+view", "gh pr view"], ["^gh\\s+pr\\s+checks", "gh pr checks"], ["^gh\\s+pr\\s+review", "gh pr review"], ["^gh\\s+pr\\s+comment", "gh pr comment"], ["^gh\\s+api", "gh api"], ["^gh\\s", "gh other"], ["^git\\s+diff", "git diff"], ["^git\\s+log", "git log"], ["^git\\s+show", "git show"], ["^git\\s+blame", "git blame"], ["^git\\s", "git other"], ["^rg\\b", "rg"], ["^grep\\b", "grep"], ["^(fd|find)\\b", "find"], ["^(ls|tree)\\b", "ls"], ["^(sed|awk)\\b", "sed/awk"], ["^(cat|head|tail|wc)\\b", "cat/head/tail"], ["^(pytest|uv|python3?|cargo|npm|pnpm|yarn|bun|node|go|make|ruff|mypy|pyflakes)\\b", "run tests/build"]][] | select(.[0] as $re | $c | test($re))] | .[0] // ["", "other"]) | .[1]; def unquoted: gsub("\"[^\"]*\""; "") | gsub("\u0027[^\u0027]*\u0027"; ""); def classify: {cmd: (norm | verb), compound: (unquoted | test("\\||&&|;|>"))}; def toolname: if type == "string" and test("^[A-Za-z0-9_-]{1,64}$") then . else "unknown" end;'
424460
# commands and denied_commands answer two different questions: what the reviewer
425461
# spends its Bash budget on, and which of those the allowlist refuses. compound is
426462
# carried separately because an allowlisted command still gets denied when it is
427-
# piped or redirected, which no tool name or verb alone would show.
463+
# piped or redirected, which no tool name or verb alone would show -- and it is
464+
# tested against the command with quoted spans removed, because `rg -n \"a|b\"` is
465+
# one allowlisted command and counting its alternation as a pipe would inflate
466+
# exactly the number the flag exists to produce.
428467
TOOL_USAGE_JQ='{tool_calls: ([.[]? | select(.type=="assistant") | .message.content[]? | select(.type=="tool_use") | .name | toolname] | group_by(.) | map({name: .[0], n: length}) | sort_by(-.n)), commands: ([.[]? | select(.type=="assistant") | .message.content[]? | select(.type=="tool_use" and .name=="Bash") | (.input.command // "") | classify] | group_by([.cmd, .compound]) | map({cmd: .[0].cmd, compound: .[0].compound, n: length}) | sort_by(-.n)), denials: (([.[]? | select(.type=="result")] | last // {}) | (.permission_denials // []) | map(.tool_name | toolname) | group_by(.) | map({name: .[0], n: length}) | sort_by(-.n)), denied_commands: (([.[]? | select(.type=="result")] | last // {}) | (.permission_denials // []) | map(select(.tool_name == "Bash") | (.tool_input.command // "") | classify) | group_by([.cmd, .compound]) | map({cmd: .[0].cmd, compound: .[0].compound, n: length}) | sort_by(-.n)), result: (([.[]? | select(.type=="result")] | last // {}) | {subtype, is_error, num_turns, duration_ms, total_cost_usd})}'
429468
jq "$CMD_JQ $TOOL_USAGE_JQ" "$EXECUTION_FILE" > "${RUNNER_TEMP}/claude-tool-usage.json"
430469
env:

‎docs/claude-pr-review-prompt.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ This prompt includes:
99

1010
Everything in `<pr_context>` is already in front of you. Do not spend a tool call re-fetching it.
1111

12+
**Unless it is not there.** If `<pr_context>` is empty, or a block inside it says it could not be read, then that block is genuinely missing — fetch what you need yourself with `gh pr diff` or `gh pr view`, and say in your review that you reviewed without it. Never treat a missing block as evidence: an absent CI block does not mean CI is clean, and an absent diff does not mean nothing changed.
13+
1214
## Tools
1315

1416
Available: `Read`, `Grep`, `Glob`, `rg`, and `gh pr diff` / `gh pr view` / `gh pr review` / `gh pr comment`. Nothing else — every other command is refused, and each refusal costs a turn.

‎tests/context-step-test.sh‎

Lines changed: 92 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,14 +69,41 @@ fail_if_marked() {
6969
"$1") exit 1 ;;
7070
esac
7171
}
72-
# Real gh writes nothing and exits 1 when a raw-text body contains ANSI colour and the flag
73-
# is absent. Every call that asks for raw text has to pass it.
72+
# GH_VERSION picks which gh this stub imitates, because the two behave oppositely and the
73+
# workflow has to work on both:
74+
#
75+
# 2.96 -- ubuntu-latest today. No escape-sequence refusal anywhere, and
76+
# --allow-escape-sequences is an unknown flag on every subcommand.
77+
# 2.97 -- refuses a raw-text body containing ANSI colour unless the flag is passed. The
78+
# refusal and the flag arrived together, as a security fix.
79+
#
80+
# A stub that only knew 2.97 is what let `gh pr diff --allow-escape-sequences` ship green:
81+
# it *required* a flag the runner rejects, so the suite passed on the broken invocation and
82+
# would have failed on the correct one. Unknown flags are rejected here for that reason --
83+
# a stub that accepts more than the real thing can only be wrong in the direction that
84+
# hides a bug.
85+
reject_unknown_flags() {
86+
for arg in "$@"; do
87+
case "$arg" in
88+
--allow-escape-sequences)
89+
if [ "$GH_VERSION" = "2.96" ]; then
90+
echo "unknown flag: --allow-escape-sequences" >&2
91+
exit 1
92+
fi
93+
;;
94+
esac
95+
done
96+
}
97+
# Raw-text bodies only. On 2.97 the flag is mandatory; on 2.96 it cannot be passed at all,
98+
# and no refusal exists.
7499
require_escape_flag() {
100+
[ "$GH_VERSION" = "2.96" ] && return 0
75101
case "$1" in
76102
*--allow-escape-sequences*) ;;
77103
*) echo "the response contains terminal escape sequences" >&2; exit 1 ;;
78104
esac
79105
}
106+
reject_unknown_flags "$@"
80107
case "$args" in
81108
*"/reviews"*) fail_if_marked reviews; cat "$FIXTURES/reviews-straddled-round.json" ;;
82109
*"/pulls/"*"/comments"*) fail_if_marked comments; echo '[]' ;;
@@ -94,8 +121,18 @@ case "$args" in
94121
# fixtures do -- the first PR to carry them lost its entire diff block to this.
95122
*"/compare/"*)
96123
fail_if_marked compare
97-
require_escape_flag "$args"
98-
printf 'diff --git a/api/app.py b/api/app.py\n+incremental change\n'
124+
# The JSON probe and the diff body are the same endpoint, told apart by the Accept
125+
# header. COMPARE_STATUS fakes what a rebase does to it: the old SHA stays reachable, so
126+
# the call succeeds, but the comparison is no longer a fast-forward.
127+
case "$args" in
128+
*vnd.github.diff*)
129+
require_escape_flag "$args"
130+
printf 'diff --git a/api/app.py b/api/app.py\n+incremental change\n'
131+
;;
132+
*)
133+
printf '{"status":"%s","ahead_by":2,"behind_by":0}\n' "$COMPARE_STATUS"
134+
;;
135+
esac
99136
;;
100137
*"pr diff"*)
101138
fail_if_marked diff
@@ -123,6 +160,8 @@ run_step() {
123160
STUB_PR=172 \
124161
STUB_REPO=hotdata-dev/dlthubworker \
125162
STUB_JOB_LOG="${STUB_JOB_LOG:-job-log-django.txt}" \
163+
GH_VERSION="${GH_VERSION:-2.96}" \
164+
COMPARE_STATUS="${COMPARE_STATUS:-ahead}" \
126165
STUB_DIFF_LINES="${STUB_DIFF_LINES:-40}" \
127166
FAIL_ENDPOINT="${FAIL_ENDPOINT:-none}" \
128167
HEAD_SHA="${HEAD_SHA:-1d01475432236aa4fbca722aaaa2687c2b2e4947}" \
@@ -250,6 +289,55 @@ expect "$(STUB_JOB_LOG=pull-commits.json run_step)" "0" \
250289
"log with no error marker does not abort the step"
251290
expect_context 'Last 120 log lines' "log with no error marker falls back to a tail"
252291

292+
# --- Since-last-review base ---------------------------------------------------------------
293+
294+
# `compare/A...B` is three-dot, so it diffs from the *merge base* of A and B. While the
295+
# branch only gains commits that is the same thing as "since A". After a rebase or a
296+
# squash-and-force-push the old SHA usually stays reachable, so the call still succeeds and
297+
# returns everything since the old fork point -- the whole PR, plus whatever the rebase
298+
# pulled in from upstream -- under the heading "Diff since your last review". A reviewer
299+
# reading that re-raises settled issues, and the 2000-line cap can drop the part that
300+
# genuinely is new. Only a clean fast-forward earns the heading.
301+
COMPARE_STATUS=diverged run_step > "$WORK/code.txt"
302+
expect "$(cat "$WORK/code.txt")" "0" "step exits 0 when the comparison is not a fast-forward"
303+
if context | grep -q '^## Diff since your last review ('; then
304+
echo "FAIL a diverged comparison was still labelled as the diff since the last review"
305+
failures=$((failures + 1))
306+
else
307+
echo "ok diverged comparison is not labelled as the diff since the last review"
308+
fi
309+
expect_context 'force-pushed|rebased' \
310+
"diverged comparison explains why it is unavailable"
311+
if context | grep -q '^+incremental change$'; then
312+
echo "FAIL the diverged comparison's diff body was used anyway"
313+
failures=$((failures + 1))
314+
else
315+
echo "ok the diverged comparison's diff body is not used"
316+
fi
317+
318+
# "behind" is the other non-fast-forward: the reviewed SHA is ahead of the head, which
319+
# happens when a push is reverted. There is nothing new to show.
320+
COMPARE_STATUS=behind run_step > /dev/null
321+
if context | grep -q '^## Diff since your last review ('; then
322+
echo "FAIL a behind comparison was labelled as the diff since the last review"
323+
failures=$((failures + 1))
324+
else
325+
echo "ok behind comparison is not labelled as the diff since the last review"
326+
fi
327+
328+
# --- gh version robustness ---------------------------------------------------------------
329+
330+
# The three raw-text fetches have to land on both gh generations. This is the assertion the
331+
# suite was missing: it asserted the *flag*, which is a fact about one gh version, instead of
332+
# the outcome, which is the same on both -- the body reaches the prompt.
333+
for v in 2.96 2.97; do
334+
GH_VERSION=$v run_step > "$WORK/code.txt"
335+
expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on gh $v"
336+
expect_context '^\+line 1$' "full diff body reaches the context on gh $v"
337+
expect_context '^\+incremental change$' "since-last-review diff reaches the context on gh $v"
338+
expect_context 'FAILED \(failures=1' "failing job log reaches the context on gh $v"
339+
done
340+
253341
# --- Truncation --------------------------------------------------------------------------
254342

255343
expect "$(STUB_DIFF_LINES=4000 run_step)" "0" "step exits 0 on an oversized diff"

‎tests/pr-context-test.sh‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ FILES_JQ=$(extract_jq FILES_JQ)
2424
CHECKS_JQ=$(extract_jq CHECKS_JQ)
2525
FAILING_JOBS_JQ=$(extract_jq FAILING_JOBS_JQ)
2626
LAST_REVIEW_JQ=$(extract_jq LAST_REVIEW_JQ)
27+
COMPARE_STATUS_JQ=$(extract_jq COMPARE_STATUS_JQ)
2728
ISSUE_COMMENTS_JQ=$(extract_jq ISSUE_COMMENTS_JQ)
2829

2930
failures=0
@@ -165,6 +166,18 @@ expect "$(printf '[]' | jq -s -r "$LAST_REVIEW_JQ")" "" \
165166
expect "$(printf '[{"user":{"login":"claude[bot]"},"commit_id":"aaa","submitted_at":null}]' | jq -s -r "$LAST_REVIEW_JQ")" \
166167
"" "unsubmitted review is not treated as the last review"
167168

169+
# --- Comparison status --------------------------------------------------------------------
170+
171+
# Only "ahead" means the reviewed SHA fast-forwards to the head, which is the one case where
172+
# a three-dot compare really is "everything since my last review". Anything else -- and
173+
# anything unreadable -- has to be distinguishable from it by the caller.
174+
expect "$(printf '{"status":"ahead","ahead_by":2}' | jq -r "$COMPARE_STATUS_JQ")" "ahead" \
175+
"fast-forward comparison reports ahead"
176+
expect "$(printf '{"status":"diverged"}' | jq -r "$COMPARE_STATUS_JQ")" "diverged" \
177+
"rebased comparison reports diverged"
178+
expect "$(printf '{}' | jq -r "$COMPARE_STATUS_JQ")" "unknown" \
179+
"comparison with no status reports unknown, never ahead"
180+
168181
# --- PR conversation ---------------------------------------------------------------------
169182

170183
# Chronological, and every author labelled: the reviewer's own prior summary comments are in

0 commit comments

Comments
 (0)