fix(ci): external review lost to its own concurrency group, then to OIDC - #58
Conversation
Two defects, both surfaced by #57 — the first fork PR to get far enough to hit them. 1. A skippable run cancelled a live one. Two labels applied in the same second produce 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`. Run 31556100775 shows `authorise cancelled`, 31556102147 `authorise skipped`, and the PR showed no review and no reason. This is #42/#53 in quorum-review, in the one workflow that never got the fix: `cancel-in-progress` is evaluated BEFORE any job condition. A label never makes a running review obsolete — the head is unchanged. Only a push does, and `false` queues rather than discards, so an unrelated label waits its turn and then skips. 2. With that cleared, the action ran for the first time ever and the OIDC exchange returned 401 Invalid OIDC token, three attempts. Not a version skew: the 26 commits between this pin and the tag the internal workflow resolves touch workflow_run support, not the exchange. The internal workflow does the same exchange successfully on every run, so the App is installed and the difference is the event. Passing github_token explicitly skips the exchange. Upstream documents the input as "optional if using GitHub App" and recommends passing it for the analogous privileged context, so this is a supported path, not a workaround — and it is the better one here. GITHUB_TOKEN is bounded by this workflow's permissions block; an App installation token carries whatever that App holds repository-wide. The narrower credential belongs in the pull_request_target job. Six assertions, each verified by mutation. The concurrency ones are written separately from quorum's rather than shared: quorum keys on github.event_name, this file is all pull_request_target and keys on the action, and an assertion loose enough for both would stop guarding either. One of them exists only because the extraction range differs — this file puts permissions: before concurrency:, so quorum's range comes back empty here and every assertion would have passed on nothing. 260 -> 262.
Quorum reviewReviewing only what changed since
Beyond the diff, the models made 17 read-only lookup(s) into the checkout at No new issues found in this diff. Usage
Reviewed |
|
Reviewed. This PR only touches Fix 1 (concurrency): Fix 2 (github_token): passing Tests: the new assertions in No blocking issues found. One purely cosmetic nit: the |
Review nit: '"'"' survived into the comment text as literal characters when these lines were written through a python heredoc. Harmless — they are inside # comments — but they read as noise in the one place that explains why the credential choice matters.
|
Reviewed the diff ( Logic itself looks correct. The new tests are well-built against the exact pitfall this task is primed to look for (a guard/assertion that's a no-op or passes on empty extraction): the One thing that doesn't add up: the PR description's test count. It says "260 → 262" (+2), but the diff to
Static count of Not independently verified (flagging per repo convention rather than assuming): the claim that upstream |
1 participant
Both found by #57 — the first fork PR to get far enough to hit them. Neither had ever been exercised: every prior run of this workflow was
skipped.1. A run that was going to skip cancelled the one that was going to review
Two labels were applied in the same second, so GitHub sent two
labeledevents:claude-reviewdocumentationThe
documentationrun entered the concurrency group withcancel-in-progress: true, killed the live one, and then skipped itself. The PR showed no review and nothing saying why.This is #42 / #53 in quorum-review, in the one workflow that never got the fix:
cancel-in-progressis evaluated before any job condition, so a run that will be skipped still enters the group.A label never makes a running review obsolete — the head commit is unchanged. Only a push does. And
falsequeues rather than discards, so an unrelated label waits its turn and then skips.2. With that cleared, the action ran for the first time and 401'd
Checked before concluding:
claude-review.ymlresolves touchworkflow_runsupport andcontext.ts, not the exchange.App token successfully obtainedon every run.pull_request_target.Passing
github_tokenexplicitly skips the exchange. Upstream documents the input as "optional if using GitHub App" and recommends passing it for the analogous privileged context (workflow_runwith a non-write actor), so this is a supported path rather than a workaround.It is also the better credential here.
GITHUB_TOKENis bounded by this workflow'spermissions:block (contents: read,pull-requests: write); an App installation token carries whatever that App was granted across the repository. The narrower one belongs in thepull_request_targetjob. Cost is cosmetic — comments arrive asgithub-actions[bot]— plus one benefit: a GITHUB_TOKEN comment does not trigger other workflows, which is the class of failure #42 was.Tests
260 → 262, each verified by mutation (bare
true, an expression that inspects the label, a removedgithub_token,pull-requests: read).Written separately from quorum's equivalents rather than shared: quorum discriminates on
github.event_name, this file is allpull_request_targetand has to key on the action, and an assertion loose enough to pass on both would stop guarding either.One assertion exists only because the two files differ structurally — this one puts
permissions:beforeconcurrency:, so quorum's extraction range comes back empty here and every assertion would have passed on nothing. That is checked explicitly.