diff --git a/.github/workflows/claude-review-external.yml b/.github/workflows/claude-review-external.yml index 4230724..20a1dc6 100644 --- a/.github/workflows/claude-review-external.yml +++ b/.github/workflows/claude-review-external.yml @@ -44,9 +44,22 @@ permissions: pull-requests: write id-token: write # the action mints an OIDC token even with token auth +# One review at a time per pull request. `cancel-in-progress` is evaluated BEFORE any job +# condition, so a run that will be skipped still enters the group and can cancel a live +# one — the same defect fixed in quorum-review.yml (#42, #53), which this file never got. +# +# It cost a real review on #57. Two labels were applied in the same second, so GitHub sent +# TWO `labeled` events: the `claude-review` one started `authorise`, the `documentation` +# one arrived two seconds later, cancelled it, and then skipped itself because +# `github.event.label.name` was not `claude-review`. Result: `authorise cancelled`, +# `authorise skipped`, and no review — with nothing on the pull request saying why. +# +# A label event never makes a running review obsolete: the head commit is unchanged. Only +# a push does. And `false` does not DISCARD the second run, it queues it — so an unrelated +# label waits its turn and then skips, costing a few seconds of a queue slot. concurrency: group: claude-review-ext-${{ github.event.pull_request.number }} - cancel-in-progress: true + cancel-in-progress: ${{ github.event.action == 'synchronize' }} jobs: # Cheap gate, separate job: nothing with secrets runs until this passes. @@ -128,6 +141,30 @@ jobs: - uses: anthropics/claude-code-action@be7b93b1907a4abad570368f3c74b6fe3807510b # v1 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + # Pass the GitHub token explicitly instead of letting the action mint one. + # + # Left out, the action exchanges a GitHub OIDC token for a Claude App + # installation token. That works on `pull_request` — claude-review.yml does it + # every run — and returned `401 Unauthorized - Invalid OIDC token` on all three + # attempts here, the first time this workflow ever reached the action at all + # (before that it died at the checkout guard, then at the concurrency bug + # above). The action version is not the difference: the 26 commits between this + # pin and the tag the internal workflow resolves touch `workflow_run` support, + # not the token exchange. + # + # `github_token` is documented as "optional if using GitHub App", and upstream + # recommends passing it for the analogous privileged context (`workflow_run` + # with a non-write actor). So this is a supported path rather than a + # workaround — and for a `pull_request_target` job it is the better one: + # GITHUB_TOKEN is scoped by this workflow's `permissions:` block + # (contents: read, pull-requests: write), whereas an App installation token + # carries whatever that App was granted across the whole repository. The + # narrower credential belongs in the context that most needs one. + # + # The cost is cosmetic — comments arrive as github-actions[bot] rather than the + # Claude app — plus one real benefit: a GITHUB_TOKEN comment does not trigger + # other workflows, which is the class of failure #42 was. + github_token: ${{ github.token }} # Surfaces the real API error instead of a bare is_error:true (the action # suppresses it by default). Keep on until the review path is proven stable. show_full_output: true diff --git a/tests/run-tests.sh b/tests/run-tests.sh index cee1554..f7711fe 100755 --- a/tests/run-tests.sh +++ b/tests/run-tests.sh @@ -1228,6 +1228,46 @@ if grep -qE 'comment\.(body|user|author_association)' <<<"$CONC"; then echo "FAIL: concurrency inspects the comment again — it must not predict the job condition"; FAIL=$((FAIL+1)); else echo "ok: concurrency does not try to predict whether the job will run"; PASS=$((PASS+1)); fi +# The SAME property on the external workflow, which never got the #42/#53 fix and lost a +# real review to it on #57: two labels applied in the same second produced two `labeled` +# events, the `documentation` one cancelled the live `claude-review` one and then skipped +# itself. Asserted separately rather than looped over both files, because the two differ — +# quorum discriminates on `github.event_name`, this one is all `pull_request_target` and +# has to key on the action — and a shared assertion would have to be loose enough to pass +# on either, which is how a guard stops guarding. +XW="$ROOT/.github/workflows/claude-review-external.yml" +# NOTE the range: this file puts `permissions:` BEFORE `concurrency:`, so the quorum +# extraction above would come back empty here and every assertion would pass on nothing. +XCONC="$(sed -n '/^concurrency:/,/^jobs:/p' "$XW")" +if [ -z "${XCONC//[$' \t\n']/}" ]; then + echo "FAIL: could not read the external workflow concurrency block"; FAIL=$((FAIL+1)); +else echo "ok: external concurrency block located"; PASS=$((PASS+1)); fi +if grep -q "cancel-in-progress: *true" <<<"$XCONC"; then + echo "FAIL: external cancel-in-progress is bare true — an unrelated label kills the review"; FAIL=$((FAIL+1)); +else echo "ok: external cancel-in-progress is an expression"; PASS=$((PASS+1)); fi +# Only a push makes a running review obsolete; a label leaves the head commit alone. +if grep -q "github.event.action == 'synchronize'" <<<"$XCONC"; then + echo "ok: external cancels only on a push"; PASS=$((PASS+1)); +else echo "FAIL: external cancel-in-progress no longer keys on synchronize alone"; FAIL=$((FAIL+1)); fi +# Same design decision as quorum: the expression must not try to predict the job's `if:`. +if grep -qE 'label\.name|labels\.\*' <<<"$XCONC"; then + echo "FAIL: external concurrency inspects the label — it must not predict the job condition"; FAIL=$((FAIL+1)); +else echo "ok: external concurrency does not inspect the label"; PASS=$((PASS+1)); fi + +# The external reviewer must not fall back to minting a Claude App installation token. +# Doing so 401ed on every attempt under pull_request_target, and even when it works it is +# the WIDER credential: an App token carries whatever that App holds across the +# repository, while GITHUB_TOKEN is bounded by this workflow's permissions block. The +# privileged context is the one place not to take the wider one. +if grep -qE '^ +github_token: \$\{\{ *github\.token *\}\}' "$XW"; then + echo "ok: external review uses the workflow-scoped GITHUB_TOKEN"; PASS=$((PASS+1)); +else echo "FAIL: external review has no explicit github_token — it will mint an App token"; FAIL=$((FAIL+1)); fi +# ...and the permissions that token is scoped BY have to actually cover posting a review. +XPERM="$(sed -n '/^permissions:/,/^concurrency:/p' "$XW")" +if grep -q 'pull-requests: write' <<<"$XPERM"; then + echo "ok: the workflow grants pull-requests: write for the review comment"; PASS=$((PASS+1)); +else echo "FAIL: GITHUB_TOKEN cannot post the review with these permissions"; FAIL=$((FAIL+1)); fi + # The fork guard runs before anything is cloned or any credential is minted. if [ "$(grep -n 'Refuse a fork' "$QW" | cut -d: -f1)" \ -lt "$(grep -n 'actions/checkout@' "$QW" | head -1 | cut -d: -f1)" ]; then