Skip to content

Commit a5a1567

Browse files
committed
fix(review): fail loudly when the context script is missing
1 parent 4247149 commit a5a1567

4 files changed

Lines changed: 54 additions & 4 deletions

File tree

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,9 +111,25 @@ jobs:
111111
echo "$PROMPT" >> $GITHUB_OUTPUT
112112
echo "EOF" >> $GITHUB_OUTPUT
113113
114-
- name: Verify jq is available
114+
# Both preconditions for the context step, checked here because this step has no
115+
# continue-on-error and that step does. A missing script is otherwise the quietest failure
116+
# this workflow has: exit 127 swallowed into a green run, an empty context, and -- before
117+
# the review step was gated on the context outcome -- a review that read an absent history
118+
# as a clean first cycle. The prompt document has always had this protection, because
119+
# `cat` on a missing file fails the job; the script needs it spelled out.
120+
#
121+
# It guards a future break of the sparse-checkout, not a present one: a renamed path, a
122+
# pattern that stops matching in non-cone mode, a file moved without updating the checkout.
123+
- name: Verify the context step's preconditions
115124
if: github.event.pull_request.user.login != 'dependabot[bot]'
116-
run: jq --version
125+
run: |
126+
jq --version
127+
script=.github-workflows/scripts/gather-review-context.sh
128+
if [ ! -f "$script" ]; then
129+
echo "$script is missing from the cross-repo checkout." >&2
130+
echo "Check the sparse-checkout patterns against the paths in hotdata-dev/github-workflows@main." >&2
131+
exit 1
132+
fi
117133
118134
# Frontloads what a week of tool-usage artifacts showed the reviewer fetching for
119135
# itself, one denied command at a time. Over 109 runs it averaged 19.5 Bash calls and

‎scripts/gather-review-context.sh‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,13 @@
2323
# assume it.
2424
set -eo pipefail
2525

26-
: "${PR_NUMBER:?the calling step must set PR_NUMBER}"
26+
# `?` and not `:?` for PR_NUMBER, so unset is an error but empty is not. tests.yml calls the
27+
# workflow on `push: branches: [main]` too, where there is no pull request and the number is
28+
# legitimately empty; the reads below then degrade into their guarded "could not read" sentences,
29+
# which is what the inline version did and what keeps the main-push smoke run exercising the
30+
# script rather than stopping on its first line. REPO comes from github.repository and is never
31+
# legitimately empty, so it keeps `:?`.
32+
: "${PR_NUMBER?the calling step must set PR_NUMBER}"
2733
: "${REPO:?the calling step must set REPO}"
2834

2935
# Caps. The median PR reviewed across the org is 161 changed lines and the largest

‎tests/context-step-test.sh‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ run_step() {
196196
RUNNER_TEMP="$WORK/rt" \
197197
GITHUB_OUTPUT="$WORK/out.txt" \
198198
STUB_FIXTURES="$PWD/tests/fixtures" \
199-
PR_NUMBER=172 \
199+
PR_NUMBER="${PR_NUMBER-172}" \
200200
REPO=hotdata-dev/dlthubworker \
201201
STUB_JOB_LOG="${STUB_JOB_LOG:-job-log-django.txt}" \
202202
GH_VERSION="${GH_VERSION:-2.96}" \
@@ -557,6 +557,21 @@ reviews|prior reviews could not be read
557557
comments|prior inline review comments could not be read
558558
ENDPOINTS
559559

560+
# An empty PR_NUMBER is a real case, not a caller bug: tests.yml calls the workflow on
561+
# `push: branches: [main]`, where there is no pull request. The first guard in the script was
562+
# written `${PR_NUMBER:?}`, which fires on empty as well as unset, so the whole script exited on
563+
# line 26 of every main-push smoke run -- the run stayed green because the step is
564+
# continue-on-error, and the smoke test silently stopped covering anything past that line.
565+
# The reads are expected to degrade here; what matters is that the script gets to them.
566+
PR_NUMBER='' run_step "no pull request number" > "$WORK/code.txt"
567+
expect "$(cat "$WORK/code.txt")" "0" "step exits 0 when there is no pull request number"
568+
if grep -q "must set PR_NUMBER" "$WORK/step.out"; then
569+
echo "FAIL an empty PR_NUMBER aborts the script; a main-push smoke run covers nothing"
570+
failures=$((failures + 1))
571+
else
572+
echo "ok an empty PR_NUMBER degrades the reads instead of aborting the script"
573+
fi
574+
560575
if [ "$failures" -ne 0 ]; then
561576
echo "$failures test(s) failed"
562577
exit 1

‎tests/workflow-lint-test.sh‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,19 @@ check_permission "statusCheckRollup" statuses "the StatusContext half of the CI
162162
check_permission "/compare/" contents "the since-last-review comparison"
163163
check_permission "/pulls/" pull-requests "the PR reads"
164164

165+
# A missing context script has to fail loudly somewhere, and it cannot be the context step: that
166+
# one is continue-on-error, so `bash <missing file>` exits 127 into a green run. The prompt
167+
# document gets this for free -- `cat` on a missing file fails its step -- and the script needs an
168+
# explicit check to match. It must sit in a step without continue-on-error, or it proves nothing.
169+
if ! grep -q "gather-review-context.sh is missing\|! -f \"\$script\"" "$WORKFLOW_FILE"; then
170+
echo "FAIL nothing in $WORKFLOW_FILE checks that the context script arrived from the"
171+
echo " cross-repo checkout. A bad sparse-checkout pattern would exit 127 inside a"
172+
echo " continue-on-error step and leave the run green with an empty context."
173+
failures=$((failures + 1))
174+
else
175+
echo "ok a missing context script fails the job rather than emptying the context"
176+
fi
177+
165178
# The context step is continue-on-error, so everything it can fail at -- a checkout that does not
166179
# deliver the script, a bad path, a rename that stops matching the sparse pattern -- leaves the run
167180
# green with pr_context, threads and review_cycle all unset. Ungated, the review step then runs on

0 commit comments

Comments
 (0)