diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 0d60818..6dff399 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -3,9 +3,38 @@ name: Claude PR Review on: pull_request: types: [opened, synchronize, ready_for_review, reopened] - + # Callable so this repository's own CI can run this file and prove it starts. That is not a + # theoretical worry: an org ruleset injects this workflow into every repo resolved from + # `main`, so a pull request here is reviewed by main's copy and never by the copy it is + # changing. Twice in one day a file that Actions refuses to parse merged green -- an empty + # expression delimiter in a shell comment, then a `run:` block past the 21,000-character + # expression limit -- because nothing in CI had ever executed the version under review. + # + # tests.yml calls this with `uses: ./...`, which resolves from the calling commit, so the + # smoke run is the pull request's copy. A copy Actions cannot load fails the caller too, which + # is the whole point: "does it start" is exactly what both outages got wrong. + workflow_call: + inputs: + dry_run: + description: >- + Everything except the review itself. Set by tests.yml, never on the production path, + where `inputs` is empty and this reads as false. + type: boolean + default: false + +# Distinct groups for the smoke run and the live review, because a called workflow inherits the +# caller's `github` context: both would otherwise compute pr-review- on the same +# pull request, and cancel-in-progress would have each cancelling the other. A smoke test that +# kills real reviews is worse than no smoke test. +# +# The run_id fallback is for the other event. tests.yml also calls this on `push: branches: +# [main]`, where there is no pull request and the number expands to nothing, so without it every +# main-push run shares one constant group -- and cancel-in-progress on a *called* workflow does +# not just drop the smoke job, it takes the caller's whole Tests run with it. Two merges close +# together would leave the earlier commit with no test signal on main. run_id is the caller's, +# and unique per run. concurrency: - group: pr-review-${{ github.event.pull_request.number }} + group: pr-review-${{ inputs.dry_run && 'smoke' || 'live' }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true jobs: @@ -509,8 +538,19 @@ jobs: PR_TITLE: ${{ github.event.pull_request.title }} PR_BODY: ${{ github.event.pull_request.body }} + # The only step a dry run skips, and the only one it needs to: every step after this one + # is already gated on this step's outcome or its outputs, so skipping it silences the whole + # write side of the workflow without a second condition anywhere. + # + # Reduce execution log -- needs steps.review.outputs.execution_file, empty when skipped + # Upload tool usage -- needs steps.tool-usage.outcome == 'success', which is 'skipped' + # Notify on failure -- needs outcome 'failure' or 'cancelled', and this is 'skipped' + # + # So a smoke run posts no review, no comment and no artifact. Everything before this step + # still runs against the live API: the app token, the cross-repo prompt checkout, and the + # nine context reads with the job's real permissions. - uses: anthropics/claude-code-action@v1 - if: github.event.pull_request.user.login != 'dependabot[bot]' + if: github.event.pull_request.user.login != 'dependabot[bot]' && !inputs.dry_run id: review continue-on-error: true with: diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index bb80cf7..e8e8ca1 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -54,3 +54,41 @@ jobs: - name: Context step end to end run: tests/context-step-test.sh + + # Runs claude-pr-review.yml itself, from this commit, with the review step skipped. The checks + # above all read the workflow as text; this one hands it to Actions and asks whether it starts. + # + # That is the gap the last two outages went through. An org ruleset injects the review workflow + # into every repo resolved from `main`, so the review on a pull request here comes from main's + # copy -- never from the copy the pull request is changing. Both times, a file Actions refuses + # to parse merged with this suite green, and the org lost reviews until someone noticed: an + # empty expression delimiter inside a shell comment, then a `run:` block over the + # 21,000-character expression limit. Neither is visible to yaml.safe_load, to bash, or to + # actionlint, and enumerating the next limit ahead of time is a game with no end. Executing the + # file has no such gap -- if Actions will not load it, this job cannot start, and CI is red. + # + # `uses: ./` resolves from the calling commit rather than from the default branch, which is what + # makes this the pull request's copy and not main's. + smoke: + name: Review workflow starts + # The same set the called job declares. A caller cannot grant a reusable workflow more than + # it holds, and the point of this job is to exercise the real thing: the nine context reads + # each need their permission, and a missing one degrades silently into a "could not read" + # sentence rather than failing. pull-requests: write is unused on this path -- the only two + # steps that write are skipped with the review step -- but it is what production runs with, + # and a smoke test that runs with a different token is testing a different workflow. + permissions: + contents: read + pull-requests: write + id-token: write + actions: read + issues: read + checks: read + statuses: read + uses: ./.github/workflows/claude-pr-review.yml + with: + dry_run: true + # The GitHub App private key, for the token step and the cross-repo prompt checkout. Both run + # in a dry run, so a broken sparse-checkout or an expired key surfaces here rather than in the + # org. + secrets: inherit diff --git a/tests/workflow-lint-test.sh b/tests/workflow-lint-test.sh index 3ba3aaa..703dad4 100755 --- a/tests/workflow-lint-test.sh +++ b/tests/workflow-lint-test.sh @@ -25,6 +25,7 @@ cd "$(dirname "$0")/.." failures=0 WORKFLOW_FILE=.github/workflows/claude-pr-review.yml +TESTS_FILE=.github/workflows/tests.yml # The delimiter, assembled rather than written, so this file does not trip its own scan. OPEN="\${$(printf '%s' '{')" @@ -97,6 +98,30 @@ for wf in "${WORKFLOWS[@]}"; do fi done +# cancel-in-progress cancels whatever else is in the group, so the group needs a key that is +# never empty on any event the workflow accepts. It is keyed on the pull request number, and +# tests.yml calls this workflow on `push: branches: [main]` as well, where there is no pull +# request and that key expands to nothing -- collapsing every main-push run into one constant +# group. Two merges landing close together would then cancel each other, and because the +# cancellation lands on a *called* workflow it takes the caller's whole Tests run with it, so a +# commit on main silently loses its test signal. +# +# Checked as a property of the expression rather than by evaluating it: the PR number must be +# followed by a `||` fallback, so the group stays unique when there is no pull request. +group_line=$(grep -n '^ group:' "$WORKFLOW_FILE" | head -1) +if [ -z "$group_line" ]; then + echo "FAIL $WORKFLOW_FILE has no workflow-level concurrency group; this check proves nothing" + failures=$((failures + 1)) +elif ! printf '%s\n' "$group_line" | grep -q 'pull_request\.number[[:space:]]*||'; then + echo "FAIL the concurrency group keys on the pull request number with no fallback, so on a" + echo " push to main it collapses to a constant and concurrent merges cancel each other's" + echo " Tests run:" + printf '%s\n' "$group_line" | sed 's/^/ /' + failures=$((failures + 1)) +else + echo "ok the concurrency group stays unique when there is no pull request" +fi + # Every read the context step makes needs a permission declared on the job, because the job # declares `permissions:` explicitly and anything unlisted is `none`. That failure is silent # by design -- each block degrades to its "could not read" sentence -- so a missing line here @@ -142,6 +167,36 @@ check_permission "statusCheckRollup" statuses "the StatusContext half of the CI check_permission "/compare/" contents "the since-last-review comparison" check_permission "/pulls/" pull-requests "the PR reads" +# 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 +# holds, so a permission added on one side and not the other fails the smoke job with Actions' +# "is requesting 'x: read', but is only allowed 'x: none'" -- loud, but on a file that looks +# unrelated to the change that caused it. Asserting the two match keeps the claim in tests.yml's +# comment true by construction instead of by review. +# +# Name and value both, so pull-requests: write degrading to read is caught too. +perm_pairs() { + awk '/^ permissions:$/ { p = 1; next } + p && /^ [a-z-]+:[[:space:]]/ { print $1, $2 } + p && /^ [a-z]/ { exit }' "$1" | sort +} +review_perms=$(perm_pairs "$WORKFLOW_FILE") +smoke_perms=$(perm_pairs "$TESTS_FILE") +if [ -z "$review_perms" ] || [ -z "$smoke_perms" ]; then + echo "FAIL a permissions block came back empty (review: $(printf '%s' "$review_perms" | wc -l)," \ + "smoke: $(printf '%s' "$smoke_perms" | wc -l)); the parity check proves nothing" + failures=$((failures + 1)) +elif [ "$review_perms" != "$smoke_perms" ]; then + echo "FAIL the smoke job in $TESTS_FILE does not grant what the review job declares." + echo " A caller cannot grant a reusable workflow more than it holds, so the smoke job" + echo " fails until both sides agree. Difference (< review job, > smoke job):" + diff <(printf '%s\n' "$review_perms") <(printf '%s\n' "$smoke_perms") | sed 's/^/ /' + failures=$((failures + 1)) +else + echo "ok the smoke job grants exactly what the review job declares" +fi + # The scan above is a backstop for one class. actionlint checks the schema, the expression # grammar, and the shell; run it when it is on PATH. shellcheck findings are excluded because # the run blocks here intentionally use unquoted word splitting for job ids.