Skip to content

Commit a520028

Browse files
authored
Merge pull request #41 from hotdata-dev/fix/skip-review-on-fork-prs
fix(review): skip pull requests from forks
2 parents d7e5fc9 + a903abf commit a520028

2 files changed

Lines changed: 48 additions & 1 deletion

File tree

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,16 @@ concurrency:
3939

4040
jobs:
4141
review:
42-
if: github.event.pull_request.draft == false
42+
# Fork pull requests run without secrets, so the App token step would fail on an empty
43+
# private key before any review. Skip them rather than fail every outside contribution.
44+
# Compared by name, not head.repo.fork, which is also true for same-repo branches in a repo
45+
# that is itself a fork. A non-pull_request event is the smoke run on push to main, which
46+
# must still run; keyed on the event name because a fork's payload is documented as empty,
47+
# so testing for an absent payload would let forks through.
48+
if: >-
49+
github.event.pull_request.draft == false &&
50+
(github.event_name != 'pull_request' ||
51+
github.event.pull_request.head.repo.full_name == github.repository)
4352
runs-on: ubuntu-latest
4453
timeout-minutes: 15
4554
permissions:

‎tests/workflow-lint-test.sh‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,44 @@ else
216216
echo "ok the review step runs only when the context step succeeded"
217217
fi
218218

219+
# Actions withholds secrets from pull_request runs whose head is a fork, so the App token step
220+
# gets an empty private key and the job fails before any review happens -- a red check on every
221+
# outside contribution in every consumer repo, for a review that could never have run. The job
222+
# has to skip those instead. Two ways to get the guard wrong, both checked:
223+
# - head.repo.fork is true for any head repo that is itself a fork, including a same-repo
224+
# branch in a hotdata repo forked from upstream, so it would skip our own pull requests.
225+
# Comparing the head repo's full name to this repo is the question actually being asked.
226+
# - tests.yml runs this workflow on push to main, where there is no pull request and the head
227+
# repo expands to null. A bare comparison is then false and the smoke job silently skips on
228+
# main, so the guard must let a run without a pull request through -- keyed on the event
229+
# name, not on the payload being absent, because GitHub documents the pull_request payload
230+
# as empty for fork pull requests, and `!github.event.pull_request` would wave those in.
231+
job_gate=$(awk '/^ review:$/ { found = 1; next }
232+
found && /^ if:/ { print; in_if = 1; next }
233+
in_if && /^ / { print; next }
234+
in_if { exit }' "$WORKFLOW_FILE")
235+
if [ -z "$job_gate" ]; then
236+
echo "FAIL could not find the review job's if: in $WORKFLOW_FILE; this check proves nothing"
237+
failures=$((failures + 1))
238+
elif ! printf '%s\n' "$job_gate" | grep -qF 'github.event.pull_request.head.repo.full_name == github.repository'; then
239+
echo "FAIL the review job does not skip pull requests from forks, which run without secrets"
240+
echo " and fail at the App token step:"
241+
printf '%s\n' "$job_gate" | sed 's/^/ /'
242+
failures=$((failures + 1))
243+
elif printf '%s\n' "$job_gate" | grep -qF '!github.event.pull_request'; then
244+
echo "FAIL the review job's no-PR bypass tests for an absent payload, which is also true of a"
245+
echo " fork pull request's documented empty payload, so forks reach the App token step:"
246+
printf '%s\n' "$job_gate" | sed 's/^/ /'
247+
failures=$((failures + 1))
248+
elif ! printf '%s\n' "$job_gate" | grep -qF "github.event_name != 'pull_request' ||"; then
249+
echo "FAIL the review job's fork guard is not bypassed when there is no pull request, so the"
250+
echo " smoke run on push to main skips instead of proving the workflow starts:"
251+
printf '%s\n' "$job_gate" | sed 's/^/ /'
252+
failures=$((failures + 1))
253+
else
254+
echo "ok the review job skips fork pull requests and still runs without one"
255+
fi
256+
219257
# The table above forces a new API call in the context step to declare its permission on the
220258
# review job. That does nothing for the smoke job in tests.yml, which calls the review workflow
221259
# and has to grant the same set by hand: a caller cannot give a reusable workflow more than it

0 commit comments

Comments
 (0)