-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Skip fork PRs in Grumpy PR Reviewer instead of treating them as failures #16702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,31 +1,33 @@ | ||
| --- | ||
| description: > | ||
| Grumpy senior code reviewer that automatically reviews the changed lines of | ||
| every pull request (including community/fork PRs) and leaves grumpy-but- | ||
| constructive inline review comments. Tone and reviewing standards are | ||
| delegated to the imported grumpy-reviewer agent. | ||
| every same-repository pull request and leaves grumpy-but-constructive inline | ||
| review comments. Tone and reviewing standards are delegated to the imported | ||
| grumpy-reviewer agent. | ||
| on: | ||
| pull_request_target: | ||
| types: | ||
| - opened | ||
| - reopened | ||
| - synchronize | ||
| workflow_dispatch: | ||
| # Allow any contributor's pull request — including those from forks — to be | ||
| # reviewed. `pull_request_target` runs in the base-repository context, so the | ||
| # Copilot engine credentials are available even for fork PRs (a plain | ||
| # `pull_request` trigger would not expose them). The default role gate | ||
| # ([admin, maintainer, write]) would otherwise skip external contributors, so | ||
| # `all` is required to actually review community PRs. This is safe because the | ||
| # agent runs read-only via the safe-outputs pattern: it can only emit | ||
| # structured review comments that separate, permission-scoped jobs apply, so | ||
| # untrusted PR content can never gain write access to the repository. | ||
| roles: all | ||
| # Only run on the upstream repository. Forks don't have the Copilot engine | ||
| # credentials configured, so runs on forks would otherwise fail loudly and spam | ||
| # fork maintainers. This condition short-circuits every job cleanly (workflow | ||
| # shows as skipped, no error) on any repo other than ohcnetwork/care_fe. | ||
| if: ${{ github.repository == 'ohcnetwork/care_fe' }} | ||
| # Only run on the upstream repository, and only for pull requests whose branch | ||
| # lives in that same repository (i.e. NOT from a fork). Two reasons: | ||
| # 1. Forks don't have the Copilot engine credentials configured, so runs on a | ||
| # fork repo would fail loudly and spam fork maintainers. | ||
| # 2. Fork PRs carry untrusted content: the GitHub MCP server's integrity | ||
| # policy refuses to hand a fork PR's diff to the agent ("lower integrity | ||
| # than agent requires"), so the review can't be produced anyway. Skipping | ||
| # such PRs here — rather than starting the engine and letting it fail — is a | ||
| # clean no-op (the workflow shows as skipped, no billed run, no error | ||
| # issue) instead of a spurious "missing data" failure. | ||
| # `pull_request_target` runs in the base-repository context, so a same-repo PR's | ||
| # author already has write access; the default role gate ([admin, maintainer, | ||
| # write]) covers them without needing `roles: all`. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropping |
||
| if: > | ||
| ${{ github.repository == 'ohcnetwork/care_fe' && | ||
| (github.event.pull_request == null || | ||
| github.event.pull_request.head.repo.full_name == github.repository) }} | ||
| # The Copilot engine authenticates inference with the COPILOT_GITHUB_TOKEN repo | ||
| # secret — a fine-grained PAT whose owner has a Copilot license and only the | ||
| # "Copilot Requests: Read" account permission (no repo scopes). Reading the PR | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the
rolesrevert safe for bot-authored same-repo PRs?The comment asserts a same-repo PR author "already has write access", but that is only reliably true for humans. Same-repo branches are also pushed by app/bot actors — the Copilot coding agent (which authored this very PR, head
copilot/feat14322home-page-redesign) and Renovate. The recompiled lock adds apre_activationjob that gates oncheck_membership.cjswithadmin,maintainer,write, and a GitHub App bot actor does not generally resolve to a repository collaborator permission level.If that check resolves false, the reviewer silently skips exactly the PRs that most benefit from review — and the skip is invisible (no failure, no comment), which is the same failure mode this PR is fixing, just quieter.
The fork gate alone fixes #16669; dropping
roles: allis a separate change riding along. Worth either keepingroles: all(the fork gate now makes it harmless, since untrusted heads never activate) or confirming a bot-authored same-repo PR still passescheck_membershipbefore merging.