From e0c44fe91398ca4ee224c9778ab0220b0e852ad6 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Thu, 24 Sep 2026 18:51:54 -0700 Subject: [PATCH 1/2] fix(review): skip pull requests from forks Fork PRs run without secrets, so the App token step fails on an empty private key and the check goes red before any review. Skip the job for them, keeping the smoke run on push to main, which has no pull request. --- .github/workflows/claude-pr-review.yml | 10 ++++++++- tests/workflow-lint-test.sh | 31 ++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 933b434..8654de7 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -39,7 +39,15 @@ concurrency: jobs: review: - if: github.event.pull_request.draft == false + # Fork pull requests run without secrets, so the App token step would fail on an empty + # private key before any review. Skip them rather than fail every outside contribution. + # Compared by name, not head.repo.fork, which is also true for same-repo branches in a repo + # that is itself a fork. No pull request means the smoke run on push to main, which must + # still run. + if: >- + github.event.pull_request.draft == false && + (!github.event.pull_request || + github.event.pull_request.head.repo.full_name == github.repository) runs-on: ubuntu-latest timeout-minutes: 15 permissions: diff --git a/tests/workflow-lint-test.sh b/tests/workflow-lint-test.sh index eee890e..5baefa5 100755 --- a/tests/workflow-lint-test.sh +++ b/tests/workflow-lint-test.sh @@ -216,6 +216,37 @@ else echo "ok the review step runs only when the context step succeeded" fi +# Actions withholds secrets from pull_request runs whose head is a fork, so the App token step +# gets an empty private key and the job fails before any review happens -- a red check on every +# outside contribution in every consumer repo, for a review that could never have run. The job +# has to skip those instead. Two ways to get the guard wrong, both checked: +# - head.repo.fork is true for any head repo that is itself a fork, including a same-repo +# branch in a hotdata repo forked from upstream, so it would skip our own pull requests. +# Comparing the head repo's full name to this repo is the question actually being asked. +# - tests.yml runs this workflow on push to main, where there is no pull request and the head +# repo expands to null. A bare comparison is then false and the smoke job silently skips on +# main, so the guard must let a run without a pull request through. +job_gate=$(awk '/^ review:$/ { found = 1; next } + found && /^ if:/ { print; in_if = 1; next } + in_if && /^ / { print; next } + in_if { exit }' "$WORKFLOW_FILE") +if [ -z "$job_gate" ]; then + echo "FAIL could not find the review job's if: in $WORKFLOW_FILE; this check proves nothing" + failures=$((failures + 1)) +elif ! printf '%s\n' "$job_gate" | grep -qF 'github.event.pull_request.head.repo.full_name == github.repository'; then + echo "FAIL the review job does not skip pull requests from forks, which run without secrets" + echo " and fail at the App token step:" + printf '%s\n' "$job_gate" | sed 's/^/ /' + failures=$((failures + 1)) +elif ! printf '%s\n' "$job_gate" | grep -qF '!github.event.pull_request ||'; then + echo "FAIL the review job's fork guard is not bypassed when there is no pull request, so the" + echo " smoke run on push to main skips instead of proving the workflow starts:" + printf '%s\n' "$job_gate" | sed 's/^/ /' + failures=$((failures + 1)) +else + echo "ok the review job skips fork pull requests and still runs without one" +fi + # The table above forces a new API call in the context step to declare its permission on the # review job. That does nothing for the smoke job in tests.yml, which calls the review workflow # and has to grant the same set by hand: a caller cannot give a reusable workflow more than it From a903abfacb3d98cc7481e58974760ddf92ab3f46 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 25 Sep 2026 08:45:00 -0700 Subject: [PATCH 2/2] fix(review): key fork-guard bypass on event name GitHub documents the pull_request payload as empty for fork PRs, so !github.event.pull_request also admitted forks. Bypass only for non-pull_request events, the smoke run on push to main. --- .github/workflows/claude-pr-review.yml | 7 ++++--- tests/workflow-lint-test.sh | 11 +++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 8654de7..5dd6822 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -42,11 +42,12 @@ jobs: # Fork pull requests run without secrets, so the App token step would fail on an empty # private key before any review. Skip them rather than fail every outside contribution. # Compared by name, not head.repo.fork, which is also true for same-repo branches in a repo - # that is itself a fork. No pull request means the smoke run on push to main, which must - # still run. + # that is itself a fork. A non-pull_request event is the smoke run on push to main, which + # must still run; keyed on the event name because a fork's payload is documented as empty, + # so testing for an absent payload would let forks through. if: >- github.event.pull_request.draft == false && - (!github.event.pull_request || + (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) runs-on: ubuntu-latest timeout-minutes: 15 diff --git a/tests/workflow-lint-test.sh b/tests/workflow-lint-test.sh index 5baefa5..b339ae7 100755 --- a/tests/workflow-lint-test.sh +++ b/tests/workflow-lint-test.sh @@ -225,7 +225,9 @@ fi # Comparing the head repo's full name to this repo is the question actually being asked. # - tests.yml runs this workflow on push to main, where there is no pull request and the head # repo expands to null. A bare comparison is then false and the smoke job silently skips on -# main, so the guard must let a run without a pull request through. +# main, so the guard must let a run without a pull request through -- keyed on the event +# name, not on the payload being absent, because GitHub documents the pull_request payload +# as empty for fork pull requests, and `!github.event.pull_request` would wave those in. job_gate=$(awk '/^ review:$/ { found = 1; next } found && /^ if:/ { print; in_if = 1; next } in_if && /^ / { print; next } @@ -238,7 +240,12 @@ elif ! printf '%s\n' "$job_gate" | grep -qF 'github.event.pull_request.head.repo echo " and fail at the App token step:" printf '%s\n' "$job_gate" | sed 's/^/ /' failures=$((failures + 1)) -elif ! printf '%s\n' "$job_gate" | grep -qF '!github.event.pull_request ||'; then +elif printf '%s\n' "$job_gate" | grep -qF '!github.event.pull_request'; then + echo "FAIL the review job's no-PR bypass tests for an absent payload, which is also true of a" + echo " fork pull request's documented empty payload, so forks reach the App token step:" + printf '%s\n' "$job_gate" | sed 's/^/ /' + failures=$((failures + 1)) +elif ! printf '%s\n' "$job_gate" | grep -qF "github.event_name != 'pull_request' ||"; then echo "FAIL the review job's fork guard is not bypassed when there is no pull request, so the" echo " smoke run on push to main skips instead of proving the workflow starts:" printf '%s\n' "$job_gate" | sed 's/^/ /'