Repository navigation
Allow Copilot coding agents through the bot allowlist without collaborator lookup - #67238
Conversation
…okup Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed for PR #67238: has_implementation_label=false and default_business_additions=0 (<=100 threshold, no custom .design-gate.yml). 3 files changed, none adding >100 lines in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review. Prepared to submit a PR review for #67238 after local analysis.
|
There was a problem hiding this comment.
🔵 Needs a closer look
The change intentionally modifies a security-sensitive authorization boundary and warrants final human review.
0 open findings
What changed in this PR
This PR fixes Copilot coding agent activation by exempting explicitly allowlisted built-in identities from collaborator lookup.
Changes:
- Bypasses collaborator checks for known Copilot identities.
- Preserves fork and confused-deputy protections.
- Adds regression tests and documents the exception.
| File | Description |
|---|---|
actions/setup/js/check_membership.cjs |
Implements the Copilot authorization exception. |
actions/setup/js/check_membership.test.cjs |
Covers activation, synchronization, and fork rejection. |
docs/src/content/docs/reference/triggers.md |
Documents Copilot’s special allowlist behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes
The Copilot allowlist shortcut is still incomplete, so a supported on.bots: [copilot] configuration can keep failing for canonical Copilot actor IDs.
Blocking theme
The compiler expands the copilot alias to multiple identities, but this patch only special-cases three literal actor strings in the membership check. As a result, some canonical Copilot actors still hit the collaborator lookup path and are rejected as inactive, which means the regression described in the PR is only partially fixed.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 58.4 AIC · ⌖ 8.25 AIC · ⊞ 19.9K
Comment /review to run again
| const botStatus = await checkBotStatus(actorToValidate, owner, repo); | ||
| // Built-in Copilot agents are not repository collaborators; the explicit allowlist | ||
| // is sufficient for their GitHub-provided actor identities. | ||
| const isBuiltInCopilot = actorToValidate === "Copilot" || actorToValidate === "copilot-swe-agent" || actorToValidate === "copilot-swe-agent[bot]"; |
There was a problem hiding this comment.
This bypass only recognizes three hard-coded actor strings, so supported Copilot aliases like copilot and @app/copilot-swe-agent still fall through to checkBotStatus() and get rejected as bot_not_active.
💡 Why this blocks the fix
The compiler already expands on.bots: [copilot] to the full Copilot family (copilot-swe-agent, Copilot, copilot, @app/copilot-swe-agent). With the current check, only Copilot, copilot-swe-agent, and copilot-swe-agent[bot] bypass the collaborator lookup, so two canonical identities that the product explicitly emits remain broken.
Please derive this exception from the same normalized Copilot alias set the compiler uses, or at least include every expanded identifier after stripping an optional [bot] suffix, so the auth path cannot drift again.
There was a problem hiding this comment.
Fixed in fb33cbd: the membership check now recognizes every compiler-supplied Copilot alias after [bot] normalization and only allows a conclusive collaborator 404. Regression coverage exercises the full alias family.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design on the core change in check_membership.cjs. Tests and docs look solid; flagging one maintainability concern worth addressing before merge.
📋 Key Themes & Highlights
Key Themes
- Duplicated identity list: the new
isBuiltInCopilotcheck hardcodes a third list of Copilot identifiers (Copilot,copilot-swe-agent,copilot-swe-agent[bot]) that overlaps but doesn't matchpkg/constants.CopilotBotNames(which also hascopilotand@app/copilot-swe-agent). Future aliases added there won't automatically get the bypass here, silently reintroducing #67157 for them. - Bypasses existing conclusive-lookup pattern: the PR could reuse the already-tested
installationCheckOptionalbypass (used forrepository_dispatch) instead of skippingcheckBotStatusentirely, keeping one code path for "App identity isn't a collaborator" rather than two. - Minor duplication: the three-literal
===chain could use the existingcanonicalizeBotIdentifierhelper already imported in this file.
Positive Highlights
- ✅ Confused-deputy protection is correctly preserved — fork-PR synchronization from an allowlisted Copilot actor is still denied before authorization (verified by the new "deny allowlisted Copilot synchronizing a fork PR" test).
- ✅ Good regression coverage:
it.eachover all three literal identities, plus a negative test (copilot[bot]) confirming the bypass doesn't over-match similar-looking bots. - ✅ Docs update in
triggers.mdclearly explains the exception alongside the existing collaborator-lookup rules.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 136.1 AIC · ⌖ 14.7 AIC · ⊞ 10.1K
Comment /matt to run again
| const botStatus = await checkBotStatus(actorToValidate, owner, repo); | ||
| // Built-in Copilot agents are not repository collaborators; the explicit allowlist | ||
| // is sufficient for their GitHub-provided actor identities. | ||
| const isBuiltInCopilot = actorToValidate === "Copilot" || actorToValidate === "copilot-swe-agent" || actorToValidate === "copilot-swe-agent[bot]"; |
There was a problem hiding this comment.
[/diagnosing-bugs] This hardcodes a third, independent list of "built-in Copilot" identities (Copilot, copilot-swe-agent, copilot-swe-agent[bot]) that must stay in sync with pkg/constants.CopilotBotNames (which also includes copilot and @app/copilot-swe-agent). It also completely skips checkBotStatus instead of reusing the already-tested "conclusively not a collaborator" bypass (installationCheckOptional) used for repository_dispatch — that path addresses the same root cause (GitHub App identities aren't collaborators) without a brand-new, unverified branch.
💡 Why this matters
The root cause of #67157 is "GitHub App/bot identities aren't repository collaborators, so the lookup 404s." That's exactly what installationCheckOptional already solves for repository_dispatch, with real verification (a 404 from the API, not an actor-name string match) and test coverage for the "lookup fails → not authorized" case. This PR instead adds a second, hand-maintained identity list that bypasses the API call entirely:
- If
pkg/constants.CopilotBotNamesgains a new alias later (it already hascopilotand@app/copilot-swe-agent), this list silently falls out of sync and those aliases regress back to the original 404 bug. - Skipping
checkBotStatusmeans zero API verification occurs for Copilot — a typo'd or spoofedactorToValidatestring match would be the only protection, whereas the existinginstallationCheckOptionalpattern still requires a conclusive 404 (not a transient error) before authorizing.
Consider either: (a) extracting a shared isBuiltInCopilotIdentity helper backed by the same canonical list the Go compiler uses (or passing it through as a generated constant), or (b) setting installationCheckOptional: true for Copilot's pull_request/pull_request_target paths and reusing the existing conclusively-not-a-collaborator logic instead of a full bypass.
@copilot please address this.
There was a problem hiding this comment.
Fixed in fb33cbd: Copilot aliases now come from constants.CopilotBotNames, are matched using canonicalizeBotIdentifier, and reuse the existing installationCheckOptional path, which still denies transient lookup errors.
| const botStatus = await checkBotStatus(actorToValidate, owner, repo); | ||
| // Built-in Copilot agents are not repository collaborators; the explicit allowlist | ||
| // is sufficient for their GitHub-provided actor identities. | ||
| const isBuiltInCopilot = actorToValidate === "Copilot" || actorToValidate === "copilot-swe-agent" || actorToValidate === "copilot-swe-agent[bot]"; |
There was a problem hiding this comment.
[/codebase-design] The three-way literal string comparison (actorToValidate === "Copilot" || actorToValidate === "copilot-swe-agent" || actorToValidate === "copilot-swe-agent[bot]") duplicates logic that canonicalizeBotIdentifier already exists to solve elsewhere in this same file's import list.
💡 Suggested simplification
const canonicalActor = canonicalizeBotIdentifier(actorToValidate);
const isBuiltInCopilot = canonicalActor === "Copilot" || canonicalActor === "copilot-swe-agent";This reduces three branches to two, matches the existing canonicalization convention used by isAllowedBot/checkBotStatus in check_permissions_utils.cjs, and automatically covers any future [bot]-suffixed variant without needing a third literal added by hand.
@copilot please address this.
There was a problem hiding this comment.
Fixed in fb33cbd: actor and configured alias names now use the existing canonicalizeBotIdentifier helper, including optional [bot] suffixes.
There was a problem hiding this comment.
Review Summary
Change type: bug_fix (correctness fix to confused-deputy / bot-allowlist authorization logic)
Impeccable modes applied: harden, audit
The fix correctly scopes the collaborator-lookup bypass to only the three canonical built-in Copilot actor identities, applied after isAllowedBot() gating — so the existing confused-deputy, cross-repo PR, and Dependabot protections are untouched. Test coverage is solid: trusted-author synchronize, fork-PR rejection, all three literal identity forms, and a negative case (copilot[bot]) confirming the bypass doesn't over-match similar-looking actors.
One non-blocking maintainability note left inline: the literal identity list duplicates constants.CopilotBotNames in Go; consider a shared source of truth or a cross-reference comment so future additions to the canonical list aren't missed here.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 124.1 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
| const botStatus = await checkBotStatus(actorToValidate, owner, repo); | ||
| // Built-in Copilot agents are not repository collaborators; the explicit allowlist | ||
| // is sufficient for their GitHub-provided actor identities. | ||
| const isBuiltInCopilot = actorToValidate === "Copilot" || actorToValidate === "copilot-swe-agent" || actorToValidate === "copilot-swe-agent[bot]"; |
There was a problem hiding this comment.
This literal identity list ("Copilot", "copilot-swe-agent", "copilot-swe-agent[bot]") duplicates knowledge already centralized in Go as constants.CopilotBotNames (copilot-swe-agent, Copilot, copilot, @app/copilot-swe-agent) and used for on.bots: [Copilot] expansion. If that canonical list ever changes (e.g. a new runtime alias is added), this JS check silently drifts out of sync and either over- or under-grants the collaborator-lookup bypass.
Consider deriving this list from a shared JS constant (mirroring bot_constants.go) or adding a code comment/test that explicitly cross-references constants.CopilotBotNames so a future update to one is caught by review of the other.
@copilot please address this.
There was a problem hiding this comment.
Fixed in fb33cbd: the compiler passes the canonical constants.CopilotBotNames list to membership validation, so the JavaScript authorization check no longer maintains a separate identity list.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: f7d2328
|
…ot-status-check Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: fb33cbd
|
…ot-status-check # Conflicts: # actions/setup/js/work_queue_compaction.test.cjs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
on.bots: [Copilot]matches the Copilot coding agent, but activation rejects it when the collaborator API returns 404. The workflow then appears successful while its review jobs are skipped.Copilotandcopilot-swe-agentidentities bypass the collaborator lookup. Other bots retain the existing installation check and confused-deputy protections.on.botsreference and add regressions for PR activation, trusted synchronization, and fork rejection.