Infer GitHub App permissions and events from resolved package workflows in add-wizard bootstrap#53766
Conversation
|
Hey Since this PR is currently a draft with no code changes yet, here are the next milestones to move it forward:
When you are ready to transition from draft to ready for review, make sure the PR:
You are on the right track with the detailed issue breakdown. Looking forward to seeing this come together! 🚀
|
…n add-wizard bootstrap Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add a cli argument to disable this behavior |
There was a problem hiding this comment.
Pull request overview
Adds automatic GitHub App requirement inference for workflows installed through add-wizard.
Changes:
- Infers and merges workflow permissions and events.
- Applies inferred requirements to GitHub App bootstrap actions.
- Adds unit tests for inference and merging.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/bootstrap_profile_runner.go |
Integrates inference into bootstrap execution. |
pkg/cli/bootstrap_profile_inference.go |
Implements permission and event inference. |
pkg/cli/bootstrap_profile_inference_test.go |
Tests inference and merge behavior. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Balanced
| if err != nil || frontmatter == nil { | ||
| continue | ||
| } | ||
| mergeBootstrapPermissionsFromFrontmatter(permissions, frontmatter.Frontmatter["permissions"]) |
There was a problem hiding this comment.
Fixed in b371aeb: inference now derives from workflow.ComputeGitHubAppManifestPermissions, which merges safe-outputs-derived permissions (via SafeOutputsConfigFromKeys/ComputePermissionsForSafeOutputs) with the raw top-level permissions block, so a handler like create-issue yields issues: write even when the workflow only declares issues: read.
| if !ok { | ||
| continue | ||
| } | ||
| merged[resource] = mergeBootstrapPermissionLevel(merged[resource], strings.TrimSpace(level)) |
There was a problem hiding this comment.
Fixed in b371aeb: ComputeGitHubAppManifestPermissions normalizes Actions permission keys (e.g. pull-requests → pull_requests, security-events → security_events) to App manifest keys and drops scopes with no App equivalent (id-token, attestations, models, copilot-requests) and none entries.
| filtered := make([]string, 0, len(names)) | ||
| for _, name := range names { | ||
| name = strings.TrimSpace(name) | ||
| if name == "" || eventsExcludedFromGitHubAppInference[name] { | ||
| continue | ||
| } | ||
| filtered = append(filtered, name) |
There was a problem hiding this comment.
Fixed in b371aeb: events are now derived via workflow.NormalizeGitHubAppWebhookEvents, which expands compiler-only triggers (slash_command, label_command, reaction, status-comment, command shorthands) to their underlying webhook events, maps pull_request_target to pull_request, and filters against valid App webhook subscriptions instead of copying raw on: keys verbatim.
| var inferredPermissions map[string]string | ||
| var inferredEvents []string | ||
| if hasBootstrapGitHubAppAction(config.Profile.Profile.Config) { | ||
| inferredPermissions, inferredEvents, err = inferBootstrapGitHubAppRequirements(ctx, config.Sources) |
There was a problem hiding this comment.
Fixed in 50e4672: executeBootstrapProfile now resolves inference sources from config.Profile.Source (the package the profile belongs to) instead of the full config.Sources, falling back to config.Sources only when Source is empty for backward compatibility. Added TestExecuteBootstrapProfile_InferenceScopedToProfileSourceOnly to cover it.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added a |
|
@copilot add extensive integration test cases |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added extensive tests covering the inference pipeline end-to-end: edge cases for |
Unified in b371aeb: added |
|
@copilot A recent author update landed on this PR, but there is still no maintainer-visible resolution on the thread. Please review the latest feedback and updates, make any remaining adjustments if needed, refresh the branch if necessary, and then run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot A recent author update landed on this PR, but there is still no maintainer-visible resolution on the thread. Please review the latest feedback and updates, refresh the branch if needed, and then run the Current maintainer-visible next step:
|
Confirmed on the current head (50e4672): the unified config path holds. Both |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
Triage: category= Highest-value PR in this run: reduces manual GitHub App permission setup friction (1228+/22-, 11 files). Non-draft, active review discussion (14 comments/8 review comments). Recommend expedited human review to land this feature.
|
|
@copilot Please refresh this PR for maintainer review and then run the Current maintainer-visible next steps:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Refreshed the branch against |
|
@copilot Please address the latest maintainer-visible blockers on this PR and then run the Current maintainer-visible blockers:
A branch refresh was attempted by sous-chef as well.
|
…trap Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Added and linked the ADR at docs/adr/53766-infer-github-app-permissions-and-events-for-bootstrap.md (commit 6e something — see latest push) covering the inference decision, alternatives, and consequences. |
|
@copilot Please refresh this PR for maintainer review and then run the Current maintainer-visible next steps:
|
Confirmed on current head (04a4442): the unified config path holds — |
gh aw add-wizardrequiredaw.ymlpackage manifests to manually declaregithub-appconfigpermissions/events, duplicating what's already expressed in the workflows themselves. Manifests that omitted these fields silently produced a GitHub App with onlymetadata: readand no subscribed events.Changes
pkg/cli/bootstrap_profile_inference.go): resolves every workflow reachable from anaw.ymlpackage and derives the minimal GitHub App requirements from their frontmatter:write>read>none) across all workflows.on:triggers, excluding non-webhook triggers (schedule,workflow_dispatch,repository_dispatch) that an App has no business subscribing to.permissions/eventsstill declared inaw.yml, so manifests can supplement but no longer must duplicate.pkg/cli/bootstrap_profile_runner.go):executeBootstrapProfilenow computes inferred requirements once per run and merges them into eachgithub-appaction before the App manifest is built.Example
A package with two workflows — one needing
issues: writeonissues, another needingcontents: writeonpull_request(plus ascheduletrigger) — now produces a single App requiring:without either workflow's
aw.ymlentry declaringpermissions/eventsexplicitly.Remaining scope
This issue also calls for verifying App identity/installation/permissions post-setup, distinguishing secret presence from valid configuration, blocking wizard success on incomplete setup, and summarizing reused/created/pending requirements — these are left as follow-up work.