fix(ci): external review lost to its own concurrency group, then to OIDC

.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

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