fix(ci): claude-review on pull_request_target + orchestrator waits - #109
Merged
Conversation
… orchestrator
claude-review.yml previously triggered on workflow_run of CI. That
choice forced a chain that kept producing regressions:
- `authorAssociation` isn't valid on `gh pr view --json` (fixed in
#105 with a REST fallback)
- `statusCheckRollup` blows up on PRs with cross-integration checks
(fixed in #108 by switching that lookup to REST)
- REST `author_association` returns `NONE` for private org members
when called by the integration GITHUB_TOKEN, so private members
were always being treated as untrusted
- workflow_run-triggered checks don't attach to the PR head commit,
so the PR had no visible "Claude is running" indicator
- orchestrator's bot-clean fires in parallel with Claude review
(both triggered by CI completion), so reviewers got pinged before
Claude's feedback arrived
Replace the workflow_run trigger with pull_request_target (+ keep the
existing issue_comment and workflow_dispatch paths). With
pull_request_target:
- The job appears in the PR's Checks panel as "Claude review".
- The webhook payload carries the PR number and head SHA directly
(no `gh pr view` round-trip on this path).
- Secrets and write tokens are available (Anthropic action requires
CLAUDE_CODE_OAUTH_TOKEN; `pull_request` would deny it on fork PRs).
- The job runs immediately on PR open/sync, independent of CI.
Trust gate moves from "PR author's REST author_association" to "HEAD
commit's author or committer has at least read permission on the
repo". This:
- Fixes the private-member NONE issue — the
collaborator/permission endpoint is accessible to the integration
token and returns the user's actual permission.
- Honours the "I pushed a fixup onto someone else's PR, trust the
new HEAD" intuition: the committer field reflects who applied
the commit, so an admin's fixup on a fork PR flips the gate
from skip to proceed.
- Treats GitHub Web UI commits correctly (web-flow as committer,
real user as author → trust via author).
The gate exits 0 with skip=true when neither candidate is trusted,
so the job still concludes success and the "Claude review" check
appears on the PR — which lets the orchestrator's bot-clean wait
work cleanly for PRs that legitimately shouldn't be auto-reviewed.
Orchestrator changes:
- workflow_run.workflows: [CI] → [CI, "Claude PR review"]
Re-fires the orchestrator when Claude completes, so a CI-first
bot-clean (Claude still pending → false) is followed by a
Claude-completion bot-clean (all green → assign reviewer).
- Bot-clean's required-checks list gains "Claude review".
- Bot-clean switches from `gh pr view --json statusCheckRollup` to
REST `check-runs`, pre-empting the same cross-integration GraphQL
failure that #108 fixed in claude-review.yml. REST enum values
are lowercase.
Claude review is *not* added to the branch ruleset's required checks
— it's advisory. The orchestrator just waits for it locally.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
github-actions
Bot
added
github_actions
EricAndrechek
changed the title
2 tasks
EricAndrechek
added a commit
that referenced
this pull request
May 12, 2026
## Summary Follow-up to #109. The Anthropic action's OIDC token exchange rejects `pull_request_target`'s OIDC claims and we hit `App token exchange failed: 401 Unauthorized - Invalid OIDC token` on the first run. This is upstream issue [anthropics/claude-code-action#713](anthropics/claude-code-action#713) (open). Their docs (`docs/solutions.md` > "Automatic PR Code Review") use `pull_request`, not `pull_request_target`. Switching to that. ## What changes - Trigger: `pull_request_target` → `pull_request`. Same event types. - `if:` and the in-script case dispatch use `pull_request` instead of `pull_request_target`. - Checkout comment updated: `pull_request` defaults to the PR's *merge* ref; we keep explicitly pinning to PR HEAD so the action sees the commit, not a merge artefact. ## Trade-off (worth knowing) `pull_request` runs from a fork PR with a **read-only** GITHUB_TOKEN and **no access to repo secrets**. Practical impact: - Internal PRs (the dominant case on this repo) — fine. Secrets accessible. - Fork PRs from drive-by contributors — trust gate still skips them cleanly, conclusion=success, orchestrator's bot-clean isn't disrupted. - Fork PRs where an admin pushes a fixup commit — HEAD-committer trust gate passes, but the action then fails to authenticate because `CLAUDE_CODE_OAUTH_TOKEN` is empty in fork-PR contexts. This case wasn't supported before either (workflow_run was the prior trigger and fork PRs don't populate `workflow_run.pull_requests`), so this is a wash, not a regression. ## Why not solve fork support some other way The upstream pattern for fork-PR review is a two-workflow split: a `pull_request` workflow that posts a "pending" status, and a `workflow_run`/`pull_request_target` follow-up that runs the review with elevated tokens. That's exactly the chain we just removed in #109 because it kept breaking. Not worth re-introducing for a case we don't actually have. ## Test plan - [ ] Next push to PR #7 (or any internal PR): `Claude review` check appears and completes successfully. - [ ] Orchestrator waits for the Claude check before assigning. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
EricAndrechek
added a commit
that referenced
this pull request
May 12, 2026
## Two related noise sources Both produce red workflow runs on PR pushes that don't reflect actual problems and were flooding the inbox. ### 1. `claude-review.yml` failing on PRs that edit Claude's own files The Anthropic action runs a self-validation step that fails when its own workflow file (`.github/workflows/claude-review.yml`) or the prompt template (`.github/prompts/pr-review.md`) is in the PR's diff. Every PR touching Claude's wiring (#105, #108, #109, #110, #111, #113, the merge commit of #115) has logged a noisy red Claude check that resolves itself once the change merges. **Fix:** in `claude-review.yml`'s gate step, query the PR's changed files; if either path is in the diff, set `skip=true` and exit 0. The check shows success, the action never runs, the change still takes effect on the next PR's review. ### 2. `dependabot-automerge.yml push failure` ghost runs on every branch push #115 added `reviewers: ${{ replace(env.ADMINS, ',', ' ') }}` to convert the comma-separated `ADMINS` env var to the space-separated form the composite expects. Problem: `replace()` isn't a GitHub Actions expression function. The valid list is `contains`, `startsWith`, `endsWith`, `format`, `join`, `toJSON`, `fromJSON`, `hashFiles`, plus status checks. An unknown function fails workflow validation, and GitHub records a failed run with the file path (not the workflow's `name:`) as the display name and no jobs — every push to any branch since #115 merged. **Fix:** replace the bad expression with a real bash step that uses parameter expansion (`${ADMINS//,/ }`) to write `ADMINS_SPACE` to `$GITHUB_ENV`. The composite's `with:` then references `env.ADMINS_SPACE`. Both steps gated on `update-type == version-update:semver-major` so they only run when actually needed. ## Test plan - [ ] This PR's own Claude check: success (skip-gate catches the self-modification). - [ ] After merge, branch pushes no longer trigger the ghost `.github/workflows/dependabot-automerge.yml push failure` runs. - [ ] Next major-version Dependabot PR: both admins assigned correctly via `ADMINS_SPACE`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the workflow_run-triggered Claude review with a
pull_request_target-triggered one, switches the trust gate from "PR author association" to "HEAD commit author/committer permission", and makes the orchestrator wait for Claude review before assigning a reviewer.Closes the chain of regressions we've been working through (#105, #108) and gives the PR a visible
Claude reviewcheck.What was broken
The previous design — trigger Claude review on
workflow_runafter CI succeeds — kept producing new failures because workflow_run runs with a different token context and the check doesn't attach to the PR:Unknown JSON field: authorAssociationgh pr view --jsonfield. Patched in #105.Resource not accessible by integration(GraphQLstatusCheckRollup)checkSuite.workflowRunacross every context; the workflow's GITHUB_TOKEN can't read that on PRs with other integrations' checks. Patched in #108.author_association: NONEfor jfwoodsGITHUB_TOKENcollapses private org membership toNONEon REST. No good workaround at that field.Claude reviewindicator on the PRWhat this PR does
claude-review.yml: rebuild aroundpull_request_targetpull_request_target(open/sync/reopen/ready_for_review) plus the existingissue_commentandworkflow_dispatchpaths.Review→Claude review— that's the check name that appears on the PR and that the orchestrator looks for. Keep them in sync.if:filters Dependabot at the cheap level. All other gating moves inside the job.HEAD commit's author OR committer must have ≥ read permission on the repo. Implementation:gh api repos/$REPO/commits/$head_sha→ pullauthor.login,committer.login.web-flowanddependabot[bot]),gh api repos/$REPO/collaborators/$candidate/permission.admin/maintain/write/triage/readwins.successso the orchestrator can move on (drive-by PRs still get a human reviewer assigned, just without AI auto-review).project-orchestrator.yml: wait for Claude reviewworkflow_run.workflows: [CI]→[CI, "Claude PR review"]. Orchestrator re-fires when Claude completes.Claude review. First firing (after CI) sees Claude pending → bot-clean false → skip. Second firing (after Claude) sees all green → assign reviewer + set board state.gh pr view --json statusCheckRollupto RESTcheck-runs, pre-empting the same GraphQLResource not accessible by integrationfailure that hitclaude-review.ymlin fix(ci): use REST check-runs for claude-review required-check gate #108. PR docs: add initial Astro-based documentation site #7 wasn't tripping it yet; PR feat(mq)!: thread context through Ack/Nak with synchronous DoubleAck #88 (with Gemini) would have.Claude reviewis not added to the branch ruleset'srequired_status_checks— it's advisory. The orchestrator just waits for it locally.Behavioural changes worth flagging
successwith a notice in the step log explaining the skip. The orchestrator treats this as "Claude weighed in" and proceeds to assign a human reviewer.Test plan
Claude reviewcheck appears on the PR; orchestrator waits for it before assigning.Claude reviewpostssuccess (skipped); orchestrator still assigns a reviewer.@claude//reviewre-trigger via comment still works.if:, no Claude check on PR, dependabot-automerge path handles it as before.🤖 Generated with Claude Code