[NoQA] Re-run Claude PR reviewer via repository_dispatch on C+ approval#88927
[NoQA] Re-run Claude PR reviewer via repository_dispatch on C+ approval#88927mountiny wants to merge 5 commits into
Conversation
Replaces the pull_request_review trigger added in #88584. That trigger cannot work for fork PRs because GitHub does not expose secrets to a workflow when a fork PR fires pull_request_review, so OS_BOTIFY_TOKEN, ANTHROPIC_API_KEY, and pull-requests: write are all unavailable on exactly the PRs we most want C+ re-reviews for. Instead, the GitHub webhook handled by Web-Expensify checks the C+ team membership and (for an approving review on Expensify/App) fires a repository_dispatch event of type claude-review-request against this repo. repository_dispatch always runs in the base-repo context with full secrets, including for fork PRs, and is gated by Contents: write so externals/forks cannot trigger it. Hardening: - github.actor must be OSBotify or a *[bot] account - client_payload values are passed via env vars and validated with regex before reaching shell - PR metadata (head SHA, base ref, draft state, title) is re-resolved via gh pr view rather than trusted from the payload Removes the now-unused isContributorPlus composite action; the membership check lives in Web-Expensify alongside other PHP-side team lookups. Paired with Expensify/Web-Expensify#52390. Made-with: Cursor
GitHub's /dispatches permission check (Contents: write) already excludes externals and forks, but several Expensify-controlled identities also have that permission (org admins, write-collaborator PATs, other bots). The PHP webhook in Web-Expensify is the only intended caller, and it authenticates as App ID 179547 (https://github.com/apps/expensify-bot, github.actor = "expensify-bot[bot]") via EXPENSIFY_GITHUB_APP_PRIVATE_KEY. Pin the gate to that exact slug so a leaked PAT from any other code path cannot route an arbitrary client_payload through this workflow, which has access to ANTHROPIC_API_KEY and pull-requests: write. Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
melvin-bot
Bot
requested review from
ahmedGaber93
and removed request for
a team
mountiny
requested review from
a team and
Copilot
and removed request for
ahmedGaber93
|
@ahmedGaber93 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@codex review |
|
@mkhutornyi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
Pull request overview
Updates the Claude PR review workflow so it can be re-run on Contributor+ approvals for fork PRs by switching from pull_request_review to a repository_dispatch trigger, enabling the run to execute in the base-repo context with secrets available.
Changes:
- Replace
pull_request_reviewtrigger withrepository_dispatch: claude-review-requestand add aresolvePRjob to re-resolve PR metadata viagh pr view. - Update concurrency grouping and pass resolved PR number/SHA/base ref into checkout +
paths-filter. - Remove the now-unused
isContributorPluscomposite action.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/workflows/claude-review.yml |
Adds repository_dispatch trigger + resolvePR job and wires resolved PR metadata into checkout/filters/prompts. |
.github/actions/composite/isContributorPlus/action.yml |
Removes the Contributor+ membership composite action (membership check moved upstream of dispatch). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| review: | ||
| needs: [validate, checkCPlusApproval] | ||
| needs: [validate, resolvePR] | ||
| if: | | ||
| !cancelled() | ||
| && github.event.pull_request.draft != true | ||
| && !contains(github.event.pull_request.title, 'Revert') | ||
| && ( |
There was a problem hiding this comment.
review has needs: [validate, resolvePR], but validate only runs on pull_request_target and resolvePR only runs on repository_dispatch. In GitHub Actions, a job with needs will be skipped when a required dependency is skipped unless the job-level if includes always(). As written, this risks review never running for one (or both) triggers. Consider adding always() to the review.if and then explicitly gating on needs.validate.result == 'success' for pull_request_target and needs.resolvePR.result == 'success' for repository_dispatch, or split into two trigger-specific review jobs.
There was a problem hiding this comment.
The job already begins its if with !cancelled(), which is the documented override for the implicit success() requirement on needs:. Each branch of the OR explicitly checks needs.validate.outputs.IS_AUTHORIZED == 'true' for pull_request_target and needs.resolvePR.result == 'success' for repository_dispatch, so a skipped sibling cannot accidentally take the wrong branch. Leaving as-is.
| fetch-depth: 1 | ||
| # For pull_request_target the default ref is fine; for repository_dispatch we | ||
| # need to explicitly point at the PR's head SHA so paths-filter and Claude see | ||
| # the code being reviewed (not the default branch). | ||
| ref: ${{ github.event.pull_request.head.sha || needs.resolvePR.outputs.HEAD_SHA }} |
There was a problem hiding this comment.
This ref: expression will also be truthy on pull_request_target (since github.event.pull_request.head.sha exists), so it will checkout the PR's head commit. Checking out and executing repository code from an untrusted fork under pull_request_target is a security risk because subsequent steps run scripts from the checked-out ref and have access to secrets. To harden this, avoid checking out PR code on pull_request_target (checkout the base repo/default ref for trusted scripts), and if you need PR contents for Claude, fetch them via gh pr diff/view or checkout into a separate directory without executing any PR-provided scripts.
There was a problem hiding this comment.
Good catch — accepting and fixed in 4975874. The ref: parameter has been removed from actions/checkout so the review job always runs against the trusted base ref, never fork-controlled PR-head code. Claude reads PR contents through its gh pr diff / gh pr view allowed tools, not from the workspace, so we never need fork code on disk.
| uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2 | ||
| id: filter | ||
| with: | ||
| base: ${{ github.event.pull_request.base.ref || needs.resolvePR.outputs.BASE_REF }} | ||
| ref: ${{ github.event.pull_request.head.sha || needs.resolvePR.outputs.HEAD_SHA }} |
There was a problem hiding this comment.
Setting base/ref forces paths-filter into a ref-vs-ref comparison. On the repository_dispatch path, this no longer has PR context, so it will likely rely on local git refs being available; with actions/checkout using fetch-depth: 1 and checking out a detached SHA, the base branch ref may not exist locally, which can make the filter produce incorrect results or fail. Consider either (a) ensuring the required refs are fetched (e.g., fetch base branch / use fetch-depth: 0), or (b) avoiding paths-filter here and instead deriving changed paths via the PR API (e.g., gh pr view --json files) so it doesn’t depend on local git history.
There was a problem hiding this comment.
Accepting — naturally required by the pwn-request fix above. With the PR head no longer checked out, dorny/paths-filter with explicit base/ref would be unable to compare against the unfetched ref under repository_dispatch. Replaced the step in 4975874 with a small bash step that calls gh pr view --json files and sets the same code / docs outputs, so it works identically for both pull_request_target and repository_dispatch and doesn't depend on local git history.
|
Please let me know when you wanna test so I can approve. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12eb78e94f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # For pull_request_target the default ref is fine; for repository_dispatch we | ||
| # need to explicitly point at the PR's head SHA so paths-filter and Claude see | ||
| # the code being reviewed (not the default branch). | ||
| ref: ${{ github.event.pull_request.head.sha || needs.resolvePR.outputs.HEAD_SHA }} |
There was a problem hiding this comment.
Using ref: ${{ github.event.pull_request.head.sha }} here makes the pull_request_target path check out attacker-controlled fork code, but this same job then executes trusted steps (setupNode runs npm ci) with repository secrets and a writable PR token. That combination is a known privilege-escalation pattern: a malicious fork PR can modify build scripts and exfiltrate secrets or perform privileged GitHub API actions. This was safe before because pull_request_target used the default trusted ref.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 4975874. The ref: parameter was removed from actions/checkout so the review job now always runs against the trusted base ref under both pull_request_target and repository_dispatch. Claude reads PR contents only via the allowlisted gh pr diff/gh pr view tools, so we no longer need fork code on disk and the pwn-request path is closed. (As a knock-on, replaced dorny/paths-filter with a gh pr view --json files step so path detection still works without a PR-head checkout.)
|
All good |
Removes ref: from actions/checkout so the review job always runs against the trusted base ref, addressing the pwn-request pattern where running repo scripts against fork-controlled PR-head code would expose base-repo secrets. Replaces dorny/paths-filter with gh pr view --json files so path detection works the same way for both pull_request_target and repository_dispatch without needing PR-head code on disk. Made-with: Cursor
Explanation of Change
Replaces the
pull_request_reviewtrigger added in #88584 with arepository_dispatchtrigger so the Claude PR reviewer can also re-run on Contributor+ approval for fork PRs — which is most external contributors.Why the previous trigger was wrong
pull_request_reviewfrom a fork is treated by GitHub aspull_requestfrom a fork: no secrets, read-onlyGITHUB_TOKEN, andpermissions:cannot elevate it. SoOS_BOTIFY_TOKEN,ANTHROPIC_API_KEY, andpull-requests: writewere all silently unavailable on exactly the PRs we most wanted C+ re-reviews on.New flow
pull_request_review + submittedand verifies that the reviewer is a member of the Contributor+ team.repository_dispatchevent of typeclaude-review-requestagainstExpensify/Appwith{ pr_number, reviewer }.repository_dispatchand runs in the base-repo context with full secrets, regardless of fork status.Hardening on the App side
POST /repos/Expensify/App/dispatchesrequiresContents: writeon this repo, so externals/forks cannot call it.github.actormust be exactlyexpensify-bot[bot]. Any other dispatcher fails the check.client_payload.pr_numberandclient_payload.reviewerare passed via env vars and validated with regex before reaching shell.headRefOid,baseRefName,isDraft,title) is re-resolved viagh pr viewrather than trusted from the payload, so a forged payload can only redirect Claude to a real PR.Cleanup
isContributorPluscomposite action (the membership check now lives upstream of the dispatch).pull_request_targetflow (PR opened / ready_for_review) is unchanged.Fixed Issues
$ #88582
PROPOSAL:
Tests
CI-only change, exercised end-to-end through the paired backend PR (https://github.com/Expensify/Web-Expensify/pull/52390):
Expensify/App→claude-review-requestis dispatched → this workflow runs, resolves the head SHA viagh pr view, paths-filter sees the actual PR diff, Claude posts inline comments.Revertin the title →reviewjob'sif:filters it out.reviewjob'sif:filters it out.gh api repos/Expensify/App/dispatches -f event_type=claude-review-request ...from any account other thanexpensify-bot[bot]→Verify dispatcherstep fails fast.client_payload[pr_number]=NOT_A_NUMBER→Validate payloadstep fails fast.pull_request_targetflow (PR opened / ready_for_review) still works as before.Offline tests
N/A — CI workflow change.
QA Steps
N/A — CI workflow change with no user-facing impact. After the paired backend PR ships, real-world QA is a C+ approval on a fork PR against
Expensify/Appand confirming Claude re-runs against the latest commit.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari