fix: allow Copilot bot in mattpocock-skills-reviewer; handle GitHub App "not a user" in checkBotStatus#33451
fix: allow Copilot bot in mattpocock-skills-reviewer; handle GitHub App "not a user" in checkBotStatus#33451Copilot wants to merge 6 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…b App not-a-user errors Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… remove fake error construction Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot pull_request_reviewer should automatically add the copilot bot to the allowed bots |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit |
|
@copilot merge main and recompile |
…ction-skip Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
The PR effectively solves the immediate problem of allowing Copilot to trigger the mattpocock-skills-reviewer workflow. The implementation is well-tested and the error messages are helpful.
Issues to address:
- Security/Correctness — The
isGitHubAppNotUserError()regex is too permissive and could match unrelated error messages - Assumption validity — The logic assumes all GitHub Apps in event payloads have repository access, which may not hold for all event types
What's done well:
- Comprehensive test coverage for the new detection logic
- Clear, actionable error messages guiding users to add
on.bots: - Auto-injection of Copilot for
pull_request_reviewerworkflows reduces friction - Proper deduplication handling in
mergeBots()
🔎 Code quality review by PR Code Quality Reviewer · ● 2.8M
| if (!error || typeof error !== "object") return false; | ||
| if (!("status" in error) || error.status !== 404) return false; | ||
| const msg = getErrorMessage(error); | ||
| return typeof msg === "string" && /\bis not a user\b/.test(msg); |
There was a problem hiding this comment.
Potential security issue: The regex /\bis not a user\b/ may be too permissive. If a future GitHub API change adds different error messages containing "is not a user" (e.g., "Repository is not a user workspace"), this function would incorrectly classify it as a GitHub App.
Suggestion: Make the pattern more specific to match the exact GitHub API response format:
return typeof msg === "string" && /^[\w-]+ is not a user\b/.test(msg);This ensures we're matching "(login) is not a user" and not other unrelated messages.
| // so if an app actor appears in an event payload it must already have repository access. | ||
| // Treat it as active to avoid false negatives for apps like Copilot. | ||
| if (isGitHubAppNotUserError(botError)) { | ||
| core.info(`Bot '${actor}' is a GitHub App; treating as active based on event trigger`); |
There was a problem hiding this comment.
Logic concern: This assumes that any GitHub App appearing in an event payload "must already have repository access." While this may be true for installation events, it's not necessarily true for all event types.
For example:
- A GitHub App could trigger a webhook for a repository it doesn't have access to (e.g., organization-level webhooks)
- The app's permissions could have been revoked between the event trigger and this check
Suggestion: Consider adding a comment explaining which specific event types this assumption applies to, or add a fallback verification step. At minimum, log the event name to aid debugging:
if (isGitHubAppNotUserError(botError)) {
core.info(`Bot '${actor}' is a GitHub App (event: ${github.context.eventName}); treating as active based on event trigger`);
return { isBot: true, isActive: true };
}There was a problem hiding this comment.
Pull request overview
Fixes permission-check behavior so workflows don’t get skipped when triggered by the Copilot GitHub App, and updates compiled workflows accordingly.
Changes:
- Detects GitHub App “not a user” 404s and treats such bot actors (e.g., Copilot) as active in bot-status checks; propagates
isGitHubAppthrough permission-check results. - Improves denial output for GitHub App actors and adds/propagates
GH_AW_ALLOWED_BOTSin compiled workflow lock files. - Adjusts compiler validation/wiring for
pull_request_reviewerworkflows (suppresses a warning + auto-injects Copilot into allowed bots) and adds regression tests.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_validators.go | Avoids emitting the slash_command+bots warning for pull_request_reviewer workflows. |
| pkg/workflow/compiler_orchestrator_workflow.go | Auto-injects “Copilot” into the bot allowlist for pull_request_reviewer workflows during compilation. |
| pkg/workflow/bots_test.go | Adds tests covering Copilot auto-injection/deduplication behavior for pull_request_reviewer. |
| actions/setup/js/check_permissions_utils.cjs | Adds GitHub App “not a user” detection + treats those cases as active; propagates isGitHubApp from repo permission checks. |
| actions/setup/js/check_permissions_utils.test.cjs | Updates/extends tests to validate isGitHubApp propagation and GitHub App bot-status behavior. |
| actions/setup/js/check_membership.cjs | Emits actionable denial summary when the actor is a GitHub App and isn’t authorized/allowlisted. |
| actions/setup/js/check_membership.test.cjs | Adds a test case for the new GitHub App guidance path. |
| actions/setup/js/check_admin_permissions.test.cjs | Updates expectations for the new isGitHubApp field in permission-check results. |
| .github/workflows/test-quality-sentinel.lock.yml | Adds GH_AW_ALLOWED_BOTS: "Copilot" to compiled workflow env. |
| .github/workflows/pr-code-quality-reviewer.lock.yml | Adds GH_AW_ALLOWED_BOTS: "Copilot" to compiled workflow env. |
| .github/workflows/mattpocock-skills-reviewer.lock.yml | Adds GH_AW_ALLOWED_BOTS: "Copilot" to compiled workflow env. |
| .github/workflows/design-decision-gate.lock.yml | Adds GH_AW_ALLOWED_BOTS: "Copilot" to compiled workflow env. |
| .github/workflows/smoke-otel-backends.lock.yml | Fixes Datadog header env var reference (DD_APPLICATION_KEY). |
| .github/workflows/mcp-inspector.lock.yml | Fixes Datadog header env var reference (DD_APPLICATION_KEY). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 14/14 changed files
- Comments generated: 3
| await writeDenialSummary( | ||
| errorMessage, | ||
| `Actor '${actorToValidate}' is a GitHub App, not a regular user. ` + | ||
| `To allow this app to trigger the workflow, add it to \`on.bots:\` in the workflow frontmatter. ` + | ||
| `Example: \`bots: [${actorToValidate}]\``, | ||
| ); |
| // pull_request_reviewer workflows are triggered by review-request events from Copilot. | ||
| // Automatically allow the Copilot bot so authors don't need to add bots: [Copilot] manually. | ||
| if workflowData.PullRequestReviewer { | ||
| workflowData.Bots = c.mergeBots(workflowData.Bots, []string{"Copilot"}) | ||
| } |
| expect(mockCore.setOutput).toHaveBeenCalledWith("result", "api_error"); | ||
| const errorMessageCall = mockCore.setOutput.mock.calls.find(([key]) => key === "error_message"); | ||
| expect(errorMessageCall).toBeDefined(); | ||
| expect(errorMessageCall[1]).toContain("Repository permission check failed"); |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd based on this being a bug fix with comprehensive test coverage.
Key Themes
Root Cause Analysis (✅ Excellent)
- Two compounding bugs correctly identified: missing
GH_AW_ALLOWED_BOTSconfig + GitHub App 404 misclassification - The fix addresses both the symptom (denied workflow runs) and root causes
- New
isGitHubAppNotUserError()helper precisely distinguishes GitHub App errors from permission errors
Test Coverage (✅ Strong, 1 minor gap)
- JavaScript tests cover both
[bot]and slug-form fallback paths for the new GitHub App detection - Go tests thoroughly validate auto-injection, deduplication, and workflow-type selectivity
- Minor gap: Validator change (line 340 in
compiler_validators.go) lacks a regression test to verify the!workflowData.PullRequestReviewerexemption
Error Messages (✅ Excellent)
- Actionable guidance when GitHub Apps are denied: "add it to
on.bots:" instead of generic API error - Backwards compatibility preserved for non-GitHub-App errors
Discoverability (i️ Minor suggestion)
- Auto-injection of
Copilotforpull_request_reviewerworkflows happens silently during compilation - Consider adding a debug/info log message to make this implicit behavior more discoverable (not blocking)
Positive Highlights
- ✅ Clean separation of concerns: detection logic (
isGitHubAppNotUserError) extracted into a reusable helper - ✅ Comprehensive test suite prevents regressions (especially
TestPullRequestReviewerAutoInjectsCopilotBot) - ✅ Error messages guide workflow authors to the correct fix
- ✅ PR description thoroughly documents both root causes and the fix
Verdict
This is a high-quality bug fix. The suggested improvements are minor refinements, not blockers. Approving with optional suggestions for enhanced test coverage and discoverability.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 3.7M
| assert.NotContains(t, workflowData.Bots, "Copilot", | ||
| "non-pull_request_reviewer workflows should not auto-inject Copilot") | ||
| }) | ||
| } |
There was a problem hiding this comment.
[/tdd] Excellent comprehensive test coverage for the auto-injection behavior! Three scenarios covered:
- ✅ Auto-inject when no
bots:field present - ✅ Deduplicate when
bots: [Copilot]explicitly set - ✅ No auto-inject for non-
pull_request_reviewerworkflows
This is exactly the kind of test suite that prevents regressions. Well done!
| * @returns {boolean} | ||
| */ | ||
| function isGitHubAppNotUserError(error) { | ||
| if (!error || typeof error !== "object") return false; |
There was a problem hiding this comment.
[/diagnose] The regex /\bis not a user\b/ is a good defensive boundary check, but consider whether this pattern is stable across all GitHub API error messages.
Suggestion: Add a comment linking to the GitHub API documentation URL that appears in the error message (the one in the test: https://docs.github.com/rest/collaborators/collaborators#get-repository-permissions-for-a-user). This makes it easier to verify the pattern if GitHub changes their error format in the future.
// Regex matches GitHub API error message format documented at:
// https://docs.github.com/rest/collaborators/collaborators#get-repository-permissions-for-a-user
return typeof msg === "string" && /\bis not a user\b/.test(msg);| workflowData.Bots = c.mergeBots(c.extractBots(frontmatter), importsResult.MergedBots) | ||
| // pull_request_reviewer workflows are triggered by review-request events from Copilot. | ||
| // Automatically allow the Copilot bot so authors don't need to add bots: [Copilot] manually. | ||
| if workflowData.PullRequestReviewer { |
There was a problem hiding this comment.
[/diagnose] Auto-injecting Copilot for pull_request_reviewer workflows is the right fix for this specific use case, but this creates an implicit dependency that might surprise workflow authors.
Consideration: Should this auto-injection be logged during compilation? Currently, it happens silently. Adding a debug or info message would make the behavior more discoverable:
if workflowData.PullRequestReviewer {
workflowData.Bots = c.mergeBots(workflowData.Bots, []string{"Copilot"})
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Auto-allowing Copilot bot for pull_request_reviewer workflow"))
}Not blocking — the current implementation is safe and correct, just less discoverable.
| await writeDenialSummary(errorMessage, "The permission check failed with a GitHub API error. Check the `pre_activation` job log for details."); | ||
| // When the GitHub API responds with "is not a user", the actor is a GitHub App. | ||
| // The bot allowlist was either empty or not checked — provide actionable guidance. | ||
| if (result.isGitHubApp) { |
There was a problem hiding this comment.
[/diagnose] Great actionable error message! The conditional isGitHubApp check provides workflow authors with precise guidance ("add it to on.bots:") instead of a generic API error.
Positive highlight: The fallback path (line 215) is preserved for non-GitHub-App errors, maintaining backwards compatibility.
| // App identity (not a regular user). GitHub Apps trigger events via their installation, | ||
| // so if an app actor appears in an event payload it must already have repository access. | ||
| // Treat it as active to avoid false negatives for apps like Copilot. | ||
| if (isGitHubAppNotUserError(botError)) { |
There was a problem hiding this comment.
[/tdd] Excellent regression test coverage! The new isGitHubAppNotUserError() check is tested in both the [bot] form (line 541) and slug form (line 548) paths.
Minor observation: The slug-form fallback test creates a scenario where the first lookup returns plain 404 and the second returns "is not a user". This is a good edge case, but consider adding a test where both forms return "is not a user" to ensure the early-return path is also exercised.
Not blocking — current coverage is already strong.
| if len(workflowData.Command) > 0 && len(workflowData.Bots) > 0 { | ||
| // pull_request_reviewer workflows set Command internally (to the reviewer handle) but are | ||
| // triggered by review requests, not comments, so this concern does not apply to them. | ||
| if !workflowData.PullRequestReviewer && len(workflowData.Command) > 0 && len(workflowData.Bots) > 0 { |
There was a problem hiding this comment.
[/tdd] The validator fix correctly exempts pull_request_reviewer workflows from the slash_command + bots warning, but I don't see a regression test for this specific validator change.
Suggestion: Add a test case in compiler_validators_test.go (or bots_test.go) that verifies:
- A
pull_request_reviewerworkflow withbots: [Copilot]does not emit the warning - A regular
slash_commandworkflow withbots:still does emit the warning
This ensures the conditional logic remains correct if the validator is refactored in the future.
This comment has been minimized.
This comment has been minimized.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Review Summary
This PR correctly identifies and fixes two compounding bugs preventing Copilot-authored PRs from triggering the mattpocock-skills-reviewer workflow. The approach is sound, but I found one critical correctness issue in the slug-form fallback path that needs fixing.
Critical Issue (1)
- Slug-form fallback loses
isGitHubApptracking — When the slug-form lookup returns "is not a user", the function doesn't preserve the GitHub App flag, breaking the actionable error guidance this PR adds.
Additional Observations
- The existing reviews correctly identified several concerns around normalization, test coverage, and messaging clarity
- The auto-injection logic works correctly but could benefit from bot identifier normalization (as noted in other reviews)
- Test coverage is strong overall but missing a validator regression test
What's Done Well
✅ Comprehensive test coverage for the auto-injection behavior (3 scenarios)
✅ Clear, actionable error messages when GitHub Apps are blocked
✅ Proper deduplication logic prevents "Copilot,Copilot" in allowlists
✅ Validator correctly exempts pull_request_reviewer from slash_command warning
Please address the critical fallback path issue before merging.
🔎 Code quality review by PR Code Quality Reviewer · ● 5M
| @@ -199,6 +227,11 @@ async function checkBotStatus(actor, owner, repo) { | |||
| return { isBot: true, isActive: true }; | |||
| } catch (slugError) { | |||
There was a problem hiding this comment.
Critical bug: The slug-form fallback loses isGitHubApp tracking.
When the [bot] form returns a "not a user" error (line 211), the function correctly returns early with isActive: true. However, if the [bot] form returns a plain 404, the code falls back to checking the slug form (line 220-238).
The problem: If the slug-form lookup at line 228 also encounters the "is not a user" error, the function returns { isBot: true, isActive: true } (line 233) without preserving the isGitHubApp flag. This means checkRepositoryPermission will return { hasAccess: false, error: ... } with no isGitHubApp field, and the actionable GitHub App guidance in check_membership.cjs:207 will never trigger.
Why this matters: The entire point of this PR is to provide helpful error messages when GitHub Apps are blocked. This fallback path silently loses that context.
Suggested fix: Refactor the slug-form fallback to preserve isGitHubApp detection:
} catch (slugError) {
if (isGitHubAppNotUserError(slugError)) {
core.info(`Bot '${actor}' is a GitHub App (slug form); treating as active`);
return { isBot: true, isActive: true };
}
if (slugError?.status === 404) {
core.info(`Bot '${actor}' not found in repository collaborators (neither [bot] nor slug form)`);
return { isBot: true, isActive: false };
}
// Other errors
return { isBot: false, isActive: false, error: getErrorMessage(slugError) };
}This ensures the GitHub App detection works consistently regardless of which API call triggers it.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd based on this being a bug fix with comprehensive test coverage.
Key Themes
✅ Root cause properly diagnosed: Two distinct bugs identified and fixed independently:
- Missing
GH_AW_ALLOWED_BOTSenvironment variable (workflow config) - GitHub App actors misclassified as inactive (
checkBotStatuslogic)
✅ Excellent regression test coverage: 81 new lines of Go tests + 65 new lines of JavaScript tests covering all branches and edge cases
✅ Actionable error messages: The updated denial summary now tells workflow authors exactly what to add (bots: [Copilot])
✅ Clean separation of concerns: New isGitHubAppNotUserError() helper is reusable and testable
Positive Highlights
- Documentation in code: Inline comments explain why GitHub Apps can be trusted ("event itself is proof of access")
- Auto-injection for reviewer workflows:
pull_request_reviewerworkflows automatically allow Copilot, eliminating manual config - Deduplication logic: Handles cases where
bots: [Copilot]is explicitly declared + auto-injected - False-positive suppression: Updated compiler validator to skip the
slash_command + botswarning for reviewer workflows
Minor Suggestions
Left 4 inline comments with optional improvements:
- Defensive check for docs URL in error detection
- Enhanced error message with full YAML example
- Edge case test for
bots: [OtherBot]+ auto-inject - Clarified security boundary in code comment
Verdict
Approving — this is solid diagnostic work with strong test coverage. The minor suggestions are polish, not blockers.
Matt Pocock Skills Applied: /diagnose, /tdd
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 4.1M
Comments that could not be inline-anchored
actions/setup/js/check_permissions_utils.cjs:209
[/diagnose] The reasoning in this comment is excellent — it explains why GitHub Apps appearing in event payloads must already have access. This prevents future maintainers from questioning the logic.
Nitpick: The comment could be even clearer about the security implication:
// If we get a 404 "is not a user" response, GitHub is telling us this is a GitHub
// App identity (not a regular user). GitHub Apps trigger events via their installation,
// so if an app actor appe…
</details>
<details><summary>pkg/workflow/bots_test.go:349</summary>
**[/tdd]** Strong test coverage for the auto-inject behavior! All three branches are covered:
- ✅ Auto-inject when no explicit bots
- ✅ Deduplication when explicit bots already include Copilot
- ✅ Non-reviewer workflows don't auto-inject
**Edge case to consider**: What happens if a pull_request_reviewer workflow explicitly sets `bots: [OtherBot]` without Copilot? Does Copilot still get auto-injected? Add a test case:
```go
t.Run("pull_request_reviewer_with_other_bots_still_injects_Copilot", f…
</details>
<details><summary>actions/setup/js/check_membership.cjs:131</summary>
**[/diagnose]** Excellent actionable error message! The guidance to add `bots: [${actorToValidate}]` directly addresses the root cause.
**Minor polish**: The example could benefit from showing the full frontmatter context:
```javascript
`Actor '${actorToValidate}' is a GitHub App, not a regular user. ` +
`To allow this app to trigger the workflow, add it to \`on.bots:\` in the workflow frontmatter:\n\n` +
`\`\`\`yaml\non:\n pull_request_reviewer: matt\n bots: [${actorToValidate}]\n\`\`\``,
…
</details>
<details><summary>actions/setup/js/check_permissions_utils.cjs:194</summary>
**[/diagnose]** The regex `/\bis not a user\b/` is well-chosen for matching the GitHub API error message. Word boundaries prevent false positives on partial matches.
**Suggestion**: Consider adding a defensive check for the docs URL suffix to make the detection even more robust:
```javascript
return typeof msg === "string" && (
/\bis not a user\b/.test(msg) ||
msg.includes("/collaborators/collaborators#get-repository-permissions-for-a-user")
);This guards against future GitHub API m…
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification Details
Test Quality Analysis✅ Strengths
|
The
mattpocock-skills-reviewerworkflow was being skipped for PRs authored by the Copilot GitHub App due to two compounding bugs in the permission check pipeline.Root causes
1.
GH_AW_ALLOWED_BOTSwas never emittedmattpocock-skills-reviewer.mdhad nobots:field, so the compiled lock file omittedGH_AW_ALLOWED_BOTS. The bot-allowlist fallback incheck_membership.cjsis gated onallowedBots.length > 0— with an empty list it was skipped entirely and the workflow was denied withresult=api_error.2.
checkBotStatusclassified GitHub Apps as inactiveThe collaborators API returns
404 "Copilot is not a user"for GitHub App actors — semantically different from a plain404 Not Found(no repo access).checkBotStatusdidn't distinguish between them and returned{ isBot: true, isActive: false }, so even a properly configuredGH_AW_ALLOWED_BOTSwould have still denied Copilot.Changes
mattpocock-skills-reviewer.md— addsbots: [Copilot]underon:; lock file recompiled withGH_AW_ALLOWED_BOTS: "Copilot"in thecheck_membershipstep.check_permissions_utils.cjs— addsisGitHubAppNotUserError(error)(regex/\bis not a user\b/on the 404 body);checkBotStatusreturns active when either the[bot]-form or slug-form lookup hits this error;checkRepositoryPermissionpropagatesisGitHubApp: booleanfrom the original error so callers don't reconstruct fake error objects.check_membership.cjs— whenresult.isGitHubAppis true and the bot is denied, emits an actionable denial summary pointing the workflow author toon.bots:instead of a generic API-error message.compiler_validators.go— suppresses the false-positive "slash_command + bots" warning forpull_request_reviewerworkflows. These setworkflowData.Commandinternally (to the reviewer handle) but are triggered by review requests, not comments, so the concern doesn't apply.