-
Notifications
You must be signed in to change notification settings - Fork 432
Derive omitted GitHub App owners from effective checkout target #37976
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
Changes from all commits
7377388
4d146d6
93c85e7
e7b5ed8
16f19c6
7ac4821
2be8e74
4b3501b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| # ADR-37976: Derive Omitted GitHub App Owner from `checkout.repository` | ||
|
|
||
| **Date**: 2026-06-09 | ||
| **Status**: Draft | ||
|
|
||
| ## Context | ||
|
|
||
| When an agentic workflow authenticates with a GitHub App (`actions/create-github-app-token`), the minted token must be scoped to an installation owner. Until now, the `github-app.owner` field could only be a static literal; when omitted, the framework always defaulted to `${{ github.repository_owner }}` (the repository running the workflow). This broke multi-org workflows that target `owner/repo` pairs in *other* organizations at dispatch time — the token was minted against the wrong owner, so app-token minting failed. Authors worked around this by passing a redundant `trigger_org` input duplicating information already present in `checkout.repository`. | ||
|
|
||
| ## Decision | ||
|
|
||
| We will derive the GitHub App installation owner from the single effective `checkout.repository` value when `github-app.owner` is omitted. For literal `owner/repo` values, the owner is extracted at compile time and emitted directly. For expression-based repositories (e.g. `${{ github.event.inputs.trigger_ref }}`), the compiler emits a generated pre-step that extracts the owner at runtime via `GITHUB_OUTPUT` and feeds it into the token-minting step. An explicit `github-app.owner` always takes precedence, and when no checkout repository is derivable the behavior falls back unchanged to `${{ github.repository_owner }}`. Derivation only applies when exactly one distinct repository is configured across checkouts; multiple distinct repositories yield no derivation. This applies to `checkout[*].github-app`, top-level `safe-outputs.github-app`, and `tools.github.github-app`. | ||
|
|
||
| ## Alternatives Considered | ||
|
|
||
| ### Alternative 1: Require an explicit `owner:` or `trigger_org` input | ||
| We could keep the status quo and require authors to always specify the owner explicitly (via `github-app.owner` or a `trigger_org` workflow input). This was rejected because it duplicates information already present in `checkout.repository`, is error-prone (the two values can drift), and adds boilerplate to every multi-org workflow. | ||
|
|
||
| ### Alternative 2: Always default to `github.repository_owner` | ||
| We could leave the default as the current repository owner and treat multi-org targeting as unsupported. This was rejected because it makes cross-org dispatch workflows impossible without manual overrides, which is the exact use case this change enables. | ||
|
|
||
| ### Alternative 3: Derive from any/all checkout repositories | ||
| We could attempt to derive an owner even when multiple distinct repositories are checked out (e.g. pick the first). This was rejected as ambiguous — minting a single token against one of several owners would silently choose the wrong installation. Restricting derivation to a single distinct repository keeps the inference unambiguous. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
| - Multi-org workflows can mint correctly-scoped App tokens without a redundant `trigger_org` input. | ||
| - The derived owner stays in sync with `checkout.repository`, eliminating drift between two manually-maintained values. | ||
| - Token-minting step generation now uses stable step IDs directly (`checkout-app-token-N`, `github-mcp-app-token`, `safe-outputs-app-token`) instead of string-replacing generic safe-output step metadata, making the generator easier to reason about. | ||
|
|
||
| ### Negative | ||
| - Expression-based repositories now emit an additional generated runtime pre-step (`Derive GitHub App owner`), increasing the size and complexity of generated workflow YAML. | ||
| - Owner derivation is silently skipped when multiple distinct repositories are configured; authors targeting several orgs in one workflow must still set `owner:` explicitly and may not realize derivation did not apply. | ||
|
|
||
| ### Neutral | ||
| - A new internal helper file (`github_app_owner_derivation.go`) centralizes owner inference, literal extraction, and runtime-step generation. | ||
| - The `buildGitHubAppTokenMintStep*` helper family gains an `ownerSourceRepository` parameter threaded through checkout, GitHub MCP, and safe-outputs token generation; existing call sites pass `""` to preserve prior behavior. | ||
| - Activation / pre-activation token owner behavior is intentionally left unchanged. | ||
|
|
||
| --- | ||
|
|
||
| *This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27177149721) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| package workflow | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/github/gh-aw/pkg/logger" | ||
| ) | ||
|
|
||
| var githubAppOwnerDerivationLog = logger.New("workflow:github_app_owner_derivation") | ||
|
|
||
| // inferSingleCheckoutRepositoryForGitHubAppOwner returns the single explicit checkout.repository | ||
| // value when the workflow targets exactly one distinct repository. It ignores the default checkout | ||
| // of the current repository and returns an empty string when multiple distinct repositories are | ||
| // configured. | ||
| func inferSingleCheckoutRepositoryForGitHubAppOwner(data *WorkflowData) string { | ||
| if data == nil { | ||
| return "" | ||
| } | ||
|
|
||
| checkoutMgr := NewCheckoutManager(data.CheckoutConfigs) | ||
| var repository string | ||
| for _, entry := range checkoutMgr.ordered { | ||
| if entry.key.repository == "" { | ||
| continue | ||
| } | ||
| if repository == "" { | ||
| githubAppOwnerDerivationLog.Printf("Using checkout.repository %q as GitHub App owner source candidate", entry.key.repository) | ||
| repository = entry.key.repository | ||
| continue | ||
| } | ||
| if entry.key.repository != repository { | ||
| githubAppOwnerDerivationLog.Printf("Cannot infer a single GitHub App owner source: found multiple checkout repositories (%q, %q)", repository, entry.key.repository) | ||
| return "" | ||
| } | ||
| } | ||
|
|
||
| if repository == "" && hasWorkflowCallTrigger(data.On) { | ||
| githubAppOwnerDerivationLog.Print("No explicit checkout.repository found for workflow_call; using needs.activation.outputs.target_repo as owner source") | ||
| return "${{ needs.activation.outputs.target_repo }}" | ||
| } | ||
|
|
||
| if repository == "" { | ||
| githubAppOwnerDerivationLog.Print("No explicit checkout.repository found; GitHub App owner will fall back to github.repository_owner") | ||
| } | ||
|
|
||
| return repository | ||
| } | ||
|
|
||
| func buildGitHubAppOwnerResolutionSteps(repositoryExpr, stepName, stepID string) (string, []string) { | ||
| if owner, ok := deriveLiteralGitHubAppOwner(repositoryExpr); ok { | ||
| githubAppOwnerDerivationLog.Printf("Resolved GitHub App owner %q from literal repository %q", owner, repositoryExpr) | ||
| return owner, nil | ||
| } | ||
| if strings.TrimSpace(repositoryExpr) == "" { | ||
| githubAppOwnerDerivationLog.Print("No repository expression provided for GitHub App owner derivation; using github.repository_owner") | ||
| return "${{ github.repository_owner }}", nil | ||
| } | ||
|
|
||
| ownerStepID := stepID + "-owner" | ||
| ownerStepName := strings.Replace(stepName, "Generate GitHub App token", "Derive GitHub App owner", 1) | ||
|
Contributor
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. [/grill-with-docs] The owner-step name is derived by replacing a hard-coded magic string, creating a hidden coupling to the calling step's name format. 💡 SuggestionownerStepName := strings.Replace(stepName, "Generate GitHub App token", "Derive GitHub App owner", 1)
if ownerStepName == stepName {
ownerStepName = "Derive GitHub App owner"
}This silently falls back to a generic name whenever the parent step name doesn't contain the magic string — making it fragile to caller-side renames. Consider accepting a |
||
| if ownerStepName == stepName { | ||
| ownerStepName = "Derive GitHub App owner" | ||
| } | ||
|
|
||
| var steps []string | ||
| steps = append(steps, fmt.Sprintf(" - name: %s\n", ownerStepName)) | ||
| steps = append(steps, fmt.Sprintf(" id: %s\n", ownerStepID)) | ||
| steps = append(steps, " env:\n") | ||
| steps = append(steps, fmt.Sprintf(" GH_AW_TARGET_REPOSITORY: %s\n", githubExpressionWhitespaceReplacer.Replace(repositoryExpr))) | ||
| steps = append(steps, " shell: bash\n") | ||
| steps = append(steps, " run: |\n") | ||
| steps = append(steps, " set -euo pipefail\n") | ||
| steps = append(steps, " echo \"[gh-aw] Deriving GitHub App owner from GH_AW_TARGET_REPOSITORY\"\n") | ||
| steps = append(steps, " repo=\"${GH_AW_TARGET_REPOSITORY%.wiki}\"\n") | ||
|
Contributor
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. [/tdd] The 💡 Suggested test caseAdd a table entry in {
name: "wiki repository expression emits pre-step with wiki stripped",
app: &GitHubAppConfig{
AppID: "${{ vars.APP_ID }}",
PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}",
},
ownerSourceRepo: "${{ inputs.wiki_repo }}",
expectedContains: "GH_AW_TARGET_REPOSITORY%.wiki",
},Alternatively, test the bash step content directly for the presence of the |
||
| steps = append(steps, " echo \"[gh-aw] Normalized repository candidate: $repo\"\n") | ||
| steps = append(steps, " owner=\"${repo%%/*}\"\n") | ||
| steps = append(steps, " if [[ -z \"$repo\" || -z \"$owner\" || \"$owner\" == \"$repo\" ]]; then\n") | ||
| steps = append(steps, " echo \"[gh-aw] Unable to derive GitHub App owner from repository: $GH_AW_TARGET_REPOSITORY\" >&2\n") | ||
| steps = append(steps, " exit 1\n") | ||
| steps = append(steps, " fi\n") | ||
| steps = append(steps, " echo \"[gh-aw] Derived GitHub App owner: $owner\"\n") | ||
| steps = append(steps, " echo \"owner=$owner\" >> \"$GITHUB_OUTPUT\"\n") | ||
|
|
||
| return "${{ steps." + ownerStepID + ".outputs.owner }}", steps | ||
| } | ||
|
|
||
| func resolveGitHubAppOwner(app *GitHubAppConfig, ownerSourceRepository, stepName, stepID string) (string, []string) { | ||
| if app != nil && strings.TrimSpace(app.Owner) != "" { | ||
| return app.Owner, nil | ||
| } | ||
| return buildGitHubAppOwnerResolutionSteps(ownerSourceRepository, stepName, stepID) | ||
| } | ||
|
|
||
| func deriveLiteralGitHubAppOwner(repository string) (string, bool) { | ||
| repository = strings.TrimSpace(repository) | ||
| if repository == "" { | ||
| return "", false | ||
| } | ||
|
|
||
| parts := strings.SplitN(repository, "/", 2) | ||
| if len(parts) != 2 { | ||
| return "", false | ||
| } | ||
|
|
||
| owner := strings.TrimSpace(parts[0]) | ||
| if owner == "" || strings.Contains(owner, "${{") { | ||
| return "", false | ||
| } | ||
| return owner, true | ||
| } | ||
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.
[/zoom-out] Silent fallback on multi-repo ambiguity may produce the wrong token owner — the multi-checkout cross-org case this PR enables.
💡 Analysis
inferSingleCheckoutRepositoryForGitHubAppOwnerreturns""for two distinct situations:${{ github.repository_owner }}is correct."", silently using the caller's repo owner.Case 2 is the crux:
checkout: [{repository: org-a/repo}, {repository: org-b/repo}]mints a token for the calling repo's org instead of either target. Consider returning a distinct sentinel or emitting a compile-time diagnostic so this ambiguity is surfaced rather than silently swallowed.