Skip to content

Commit 467a9c2

Browse files
Copilotpelikhan
Copilot
and
pelikhan
authored
revert safe-outputs.github-app migration: keep token minting in safe_outputs job
Reverts the safe-outputs.github-app token minting migration from the activation job back to the safe_outputs/conclusion jobs. The safe-outputs app-id/private-key steps should remain in the safe_outputs job per feedback. The tools.github.github-app token minting (moved to activation in 9bfc46e) is kept as-is. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/6962f747-7aeb-41bc-9ae2-cb8917df7dfc Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
1 parent 70cfcbc commit 467a9c2

17 files changed

Lines changed: 156 additions & 121 deletions

‎pkg/workflow/checkout_step_generator.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,12 @@ func (cm *CheckoutManager) GenerateCheckoutAppTokenInvalidationSteps(c *Compiler
4040
continue
4141
}
4242
checkoutManagerLog.Printf("Generating app token invalidation step for checkout index=%d", i)
43+
rawSteps := c.buildGitHubAppTokenInvalidationStep()
4344
stepID := fmt.Sprintf("checkout-app-token-%d", i)
44-
tokenExpr := fmt.Sprintf("steps.%s.outputs.token", stepID)
45-
rawSteps := c.buildGitHubAppTokenInvalidationStep(tokenExpr)
4645
for _, step := range rawSteps {
46+
modified := strings.ReplaceAll(step, "steps.safe-outputs-app-token.outputs.token", "steps."+stepID+".outputs.token")
4747
// Update step name to indicate it's for checkout
48-
modified := strings.ReplaceAll(step, "Invalidate GitHub App token", fmt.Sprintf("Invalidate checkout app token (%d)", i))
48+
modified = strings.ReplaceAll(modified, "Invalidate GitHub App token", fmt.Sprintf("Invalidate checkout app token (%d)", i))
4949
steps = append(steps, modified)
5050
}
5151
}

‎pkg/workflow/compiler_activation_job.go‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -128,24 +128,6 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate
128128
outputs["github_mcp_app_token"] = "${{ steps.github-mcp-app-token.outputs.token }}"
129129
}
130130

131-
// Mint the safe-outputs app token in the activation job so that the safe_outputs and
132-
// conclusion jobs never receive the app-id / private-key secrets. The minted token is
133-
// exposed as a job output and consumed by downstream jobs via
134-
// needs.activation.outputs.safe_outputs_app_token.
135-
if data.SafeOutputs != nil && data.SafeOutputs.GitHubApp != nil {
136-
safeOutputsPermissions := ComputePermissionsForSafeOutputs(data.SafeOutputs)
137-
// In workflow_call scenarios, scope the token to the platform (host) repo name.
138-
// The resolve-host-repo step already ran above and its output is available.
139-
var safeOutputsFallbackRepo string
140-
if hasWorkflowCallTrigger(data.On) {
141-
safeOutputsFallbackRepo = "${{ steps.resolve-host-repo.outputs.target_repo_name }}"
142-
}
143-
safeOutputsTokenSteps := c.buildGitHubAppTokenMintStep(data.SafeOutputs.GitHubApp, safeOutputsPermissions, safeOutputsFallbackRepo)
144-
steps = append(steps, safeOutputsTokenSteps...)
145-
outputs["safe_outputs_app_token"] = "${{ steps.safe-outputs-app-token.outputs.token }}"
146-
outputs["safe_outputs_app_token_minting_failed"] = "${{ steps.safe-outputs-app-token.outcome == 'failure' }}"
147-
}
148-
149131
// Add reaction step right after generate_aw_info so it is shown to the user as fast as
150132
// possible. generate_aw_info runs first so its data is captured even if the reaction fails.
151133
// This runs in the activation job so it can use any configured github-token or github-app.

‎pkg/workflow/compiler_github_mcp_steps.go‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -152,10 +152,21 @@ func (c *Compiler) generateGitHubMCPAppTokenInvalidationStep(yaml *strings.Build
152152
githubConfigLog.Print("Generating GitHub App token invalidation step for GitHub MCP server")
153153

154154
// The token was minted in the activation job; reference it via needs.activation.outputs.
155-
steps := c.buildGitHubAppTokenInvalidationStep("needs.activation.outputs.github_mcp_app_token")
156-
for _, step := range steps {
157-
yaml.WriteString(step)
158-
}
155+
const tokenExpr = "needs.activation.outputs.github_mcp_app_token"
156+
157+
yaml.WriteString(" - name: Invalidate GitHub App token\n")
158+
fmt.Fprintf(yaml, " if: always() && %s != ''\n", tokenExpr)
159+
yaml.WriteString(" env:\n")
160+
fmt.Fprintf(yaml, " TOKEN: ${{ %s }}\n", tokenExpr)
161+
yaml.WriteString(" run: |\n")
162+
yaml.WriteString(" echo \"Revoking GitHub App installation token...\"\n")
163+
yaml.WriteString(" # GitHub CLI will auth with the token being revoked.\n")
164+
yaml.WriteString(" gh api \\\n")
165+
yaml.WriteString(" --method DELETE \\\n")
166+
yaml.WriteString(" -H \"Authorization: token $TOKEN\" \\\n")
167+
yaml.WriteString(" /installation/token || echo \"Token revoke may already be expired.\"\n")
168+
yaml.WriteString(" \n")
169+
yaml.WriteString(" echo \"Token invalidation step complete.\"\n")
159170
}
160171

161172
// generateParseGuardVarsStep generates a step that parses the blocked-users, trusted-users, and

‎pkg/workflow/compiler_safe_outputs_job.go‎

Lines changed: 50 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -308,17 +308,59 @@ func (c *Compiler) buildConsolidatedSafeOutputsJob(data *WorkflowData, mainJobNa
308308
return nil, nil, nil
309309
}
310310

311-
// Token minting for safe-outputs.github-app has been moved to the activation job
312-
// so that app-id / private-key never reach the safe_outputs job.
313-
// Track the minting outcome via the activation output (requires activation in needs).
311+
// Add GitHub App token minting step at the beginning if app is configured
314312
if data.SafeOutputs.GitHubApp != nil {
315-
outputs["app_token_minting_failed"] = fmt.Sprintf("${{ needs.%s.outputs.safe_outputs_app_token_minting_failed }}", constants.ActivationJobName)
313+
// Track whether the app token minting succeeded so the conclusion job can surface
314+
// authentication errors in the failure issue.
315+
outputs["app_token_minting_failed"] = "${{ steps.safe-outputs-app-token.outcome == 'failure' }}"
316+
317+
// For workflow_call relay workflows, scope the token to the platform repo name only
318+
// (not the full slug) because actions/create-github-app-token expects repo names
319+
// without the owner prefix when `owner` is also set.
320+
var appTokenFallbackRepo string
321+
if hasWorkflowCallTrigger(data.On) {
322+
appTokenFallbackRepo = "${{ needs.activation.outputs.target_repo_name }}"
323+
}
324+
appTokenSteps := c.buildGitHubAppTokenMintStep(data.SafeOutputs.GitHubApp, permissions, appTokenFallbackRepo)
325+
// Calculate insertion index: after setup action (if present) and artifact downloads, but before checkout and safe output steps
326+
insertIndex := 0
327+
328+
// Count setup action steps (checkout + setup if in dev mode without action-tag, or just setup)
329+
setupActionRef := c.resolveActionReference("./actions/setup", data)
330+
if setupActionRef != "" {
331+
insertIndex += len(c.generateCheckoutActionsFolder(data))
332+
insertIndex += len(c.generateSetupStep(setupActionRef, SetupActionDestination, c.hasCustomTokenSafeOutputs(data.SafeOutputs)))
333+
}
334+
335+
// Add artifact download steps count
336+
insertIndex += len(buildAgentOutputDownloadSteps(agentArtifactPrefix))
337+
338+
// Add patch download steps if present
339+
// Download from unified agent artifact (prefixed in workflow_call context)
340+
if usesPatchesAndCheckouts(data.SafeOutputs) {
341+
patchDownloadSteps := buildArtifactDownloadSteps(ArtifactDownloadConfig{
342+
ArtifactName: agentArtifactPrefix + constants.AgentArtifactName,
343+
DownloadPath: "/tmp/gh-aw/",
344+
SetupEnvStep: false,
345+
StepName: "Download patch artifact",
346+
})
347+
insertIndex += len(patchDownloadSteps)
348+
}
349+
350+
// Note: App token step must be inserted BEFORE shared checkout steps
351+
// because those steps reference steps.safe-outputs-app-token.outputs.token
352+
353+
// Insert app token steps
354+
var newSteps []string
355+
newSteps = append(newSteps, steps[:insertIndex]...)
356+
newSteps = append(newSteps, appTokenSteps...)
357+
newSteps = append(newSteps, steps[insertIndex:]...)
358+
steps = newSteps
316359
}
317360

318-
// Add GitHub App token invalidation step at the end if app is configured.
319-
// The token was minted in the activation job and is referenced via needs.activation.outputs.
361+
// Add GitHub App token invalidation step at the end if app is configured
320362
if data.SafeOutputs.GitHubApp != nil {
321-
steps = append(steps, c.buildGitHubAppTokenInvalidationStep(fmt.Sprintf("needs.%s.outputs.safe_outputs_app_token", constants.ActivationJobName))...)
363+
steps = append(steps, c.buildGitHubAppTokenInvalidationStep()...)
322364
}
323365

324366
// Upload the safe output items manifest as an artifact (non-staged mode only).
@@ -369,8 +411,7 @@ func (c *Compiler) buildConsolidatedSafeOutputsJob(data *WorkflowData, mainJobNa
369411
// - create_pull_request or push_to_pull_request_branch (need the activation artifact)
370412
// - lock-for-agent (need the activation lock)
371413
// - workflow_call trigger (need needs.activation.outputs.target_repo for cross-repo token/dispatch)
372-
// - safe-outputs github-app is configured (token was minted in activation job)
373-
if usesPatchesAndCheckouts(data.SafeOutputs) || data.LockForAgent || hasWorkflowCallTrigger(data.On) || data.SafeOutputs.GitHubApp != nil {
414+
if usesPatchesAndCheckouts(data.SafeOutputs) || data.LockForAgent || hasWorkflowCallTrigger(data.On) {
374415
needs = append(needs, string(constants.ActivationJobName))
375416
}
376417
// Add unlock job dependency if lock-for-agent is enabled

‎pkg/workflow/compiler_safe_outputs_job_test.go‎

Lines changed: 36 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -423,16 +423,11 @@ func TestJobWithGitHubApp(t *testing.T) {
423423

424424
stepsContent := strings.Join(job.Steps, "")
425425

426-
// Token minting has moved to the activation job; the safe_outputs job should NOT
427-
// contain "Generate GitHub App token" anymore.
428-
assert.NotContains(t, stepsContent, "Generate GitHub App token", "Token minting step should be in activation job, not safe_outputs job")
426+
// Should include app token minting step
427+
assert.Contains(t, stepsContent, "Generate GitHub App token")
429428

430-
// The safe_outputs job should reference the activation output token
431-
assert.Contains(t, stepsContent, "needs.activation.outputs.safe_outputs_app_token",
432-
"Safe_outputs job should reference the token minted in the activation job")
433-
434-
// Should include app token invalidation step (using the activation output)
435-
assert.Contains(t, stepsContent, "Invalidate GitHub App token", "Token invalidation step should still be present in safe_outputs job")
429+
// Should include app token invalidation step
430+
assert.Contains(t, stepsContent, "Invalidate GitHub App token")
436431
}
437432

438433
// TestAssignToAgentWithGitHubAppUsesAgentToken tests that when github-app: is configured,
@@ -462,9 +457,8 @@ func TestAssignToAgentWithGitHubAppUsesAgentToken(t *testing.T) {
462457

463458
stepsContent := strings.Join(job.Steps, "")
464459

465-
// Token minting has moved to the activation job; the safe_outputs job should NOT
466-
// contain "Generate GitHub App token" anymore.
467-
assert.NotContains(t, stepsContent, "Generate GitHub App token", "Token minting step should be in activation job, not safe_outputs job")
460+
// App token minting step should be present (github-app: is configured)
461+
assert.Contains(t, stepsContent, "Generate GitHub App token", "App token minting step should be present")
468462

469463
// Find the assign_to_agent step section
470464
assignToAgentStart := strings.Index(stepsContent, "id: assign_to_agent")
@@ -655,23 +649,27 @@ func TestGitHubAppWithPushToPRBranch(t *testing.T) {
655649

656650
stepsContent := strings.Join(job.Steps, "")
657651

658-
// Token minting has moved to the activation job; safe_outputs job should NOT contain it.
652+
// Should include app token minting step exactly once
659653
tokenMintCount := strings.Count(stepsContent, "Generate GitHub App token")
660-
assert.Equal(t, 0, tokenMintCount, "App token minting step should be in activation job, not safe_outputs job (found %d times)", tokenMintCount)
654+
assert.Equal(t, 1, tokenMintCount, "App token minting step should appear exactly once, found %d times", tokenMintCount)
661655

662-
// Should include app token invalidation step exactly once (invalidation stays in agent job)
656+
// Should include app token invalidation step exactly once
663657
tokenInvalidateCount := strings.Count(stepsContent, "Invalidate GitHub App token")
664658
assert.Equal(t, 1, tokenInvalidateCount, "App token invalidation step should appear exactly once, found %d times", tokenInvalidateCount)
665659

666-
// Invalidation step should reference the token from activation outputs
667-
assert.Contains(t, stepsContent, "needs.activation.outputs.safe_outputs_app_token",
668-
"Invalidation step should reference the activation job's safe_outputs_app_token output")
660+
// Token step should come before checkout step (checkout references the token)
661+
tokenIndex := strings.Index(stepsContent, "Generate GitHub App token")
662+
checkoutIndex := strings.Index(stepsContent, "Checkout repository")
663+
assert.Less(t, tokenIndex, checkoutIndex, "Token minting step should come before checkout step")
664+
665+
// Verify step ID is set correctly
666+
assert.Contains(t, stepsContent, "id: safe-outputs-app-token")
669667
}
670668

671669
// TestJobWithGitHubAppWorkflowCallUsesTargetRepoNameFallback is a regression test verifying that
672-
// the activation job (which now mints the safe-outputs app token) uses
673-
// steps.resolve-host-repo.outputs.target_repo_name (repo name only, no owner prefix) as the
674-
// repositories fallback for the GitHub App token mint step, instead of the full target_repo slug.
670+
// a safe-output job compiled for a workflow_call trigger uses
671+
// needs.activation.outputs.target_repo_name (repo name only, no owner prefix) as the repositories
672+
// fallback for the GitHub App token mint step, instead of the full target_repo slug.
675673
// This prevents actions/create-github-app-token from receiving an invalid owner/repo slug
676674
// in the repositories field when owner is also set.
677675
func TestJobWithGitHubAppWorkflowCallUsesTargetRepoNameFallback(t *testing.T) {
@@ -693,25 +691,24 @@ func TestJobWithGitHubAppWorkflowCallUsesTargetRepoNameFallback(t *testing.T) {
693691
},
694692
}
695693

696-
// Token minting has moved to the activation job.
697-
activationJob, err := compiler.buildActivationJob(workflowData, false, "", "test.lock.yml")
694+
job, _, err := compiler.buildConsolidatedSafeOutputsJob(workflowData, string(constants.AgentJobName), "test.md")
698695

699-
require.NoError(t, err, "Should successfully build activation job")
700-
require.NotNil(t, activationJob, "Activation job should not be nil")
696+
require.NoError(t, err, "Should successfully build job")
697+
require.NotNil(t, job, "Job should not be nil")
701698

702-
activationStepsContent := strings.Join(activationJob.Steps, "")
699+
stepsContent := strings.Join(job.Steps, "")
703700

704-
// The activation job must use the step-level output (it runs the resolve-host-repo step itself),
705-
// NOT the full slug from target_repo.
706-
assert.Contains(t, activationStepsContent, "repositories: ${{ steps.resolve-host-repo.outputs.target_repo_name }}",
707-
"Activation job GitHub App token step must use target_repo_name (repo name only) for workflow_call workflows")
708-
assert.NotContains(t, activationStepsContent, "repositories: ${{ needs.activation.outputs.target_repo_name }}",
709-
"Activation job GitHub App token step must not use needs.activation (it IS the activation job)")
701+
// Must use the repo-name-only output, NOT the full slug
702+
assert.Contains(t, stepsContent, "repositories: ${{ needs.activation.outputs.target_repo_name }}",
703+
"GitHub App token step must use target_repo_name (repo name only) for workflow_call workflows")
704+
assert.NotContains(t, stepsContent, "repositories: ${{ needs.activation.outputs.target_repo }}",
705+
"GitHub App token step must not use target_repo (full slug) for workflow_call workflows")
710706
}
711707

712708
// TestConclusionJobWithGitHubAppWorkflowCallUsesTargetRepoNameFallback is a regression test
713-
// verifying that the conclusion job no longer mints a token itself (token minting moved to
714-
// the activation job), and that the conclusion job references the activation output token.
709+
// verifying that the conclusion job compiled for a workflow_call trigger uses
710+
// needs.activation.outputs.target_repo_name (repo name only) as the repositories fallback
711+
// for the GitHub App token mint step.
715712
func TestConclusionJobWithGitHubAppWorkflowCallUsesTargetRepoNameFallback(t *testing.T) {
716713
compiler := NewCompiler()
717714
compiler.jobManager = NewJobManager()
@@ -737,12 +734,11 @@ func TestConclusionJobWithGitHubAppWorkflowCallUsesTargetRepoNameFallback(t *tes
737734

738735
stepsContent := strings.Join(job.Steps, "")
739736

740-
// Token minting moved to activation; conclusion job must NOT mint a token.
741-
assert.NotContains(t, stepsContent, "actions/create-github-app-token",
742-
"Conclusion job must not mint a GitHub App token (minting moved to activation job)")
743-
// Conclusion job should still invalidate the token via the activation output.
744-
assert.Contains(t, stepsContent, "needs.activation.outputs.safe_outputs_app_token",
745-
"Conclusion job should reference the safe_outputs_app_token from the activation job")
737+
// Must use the repo-name-only output, NOT the full slug
738+
assert.Contains(t, stepsContent, "repositories: ${{ needs.activation.outputs.target_repo_name }}",
739+
"Conclusion job GitHub App token step must use target_repo_name (repo name only) for workflow_call workflows")
740+
assert.NotContains(t, stepsContent, "repositories: ${{ needs.activation.outputs.target_repo }}",
741+
"Conclusion job GitHub App token step must not use target_repo (full slug) for workflow_call workflows")
746742
}
747743

748744
// TestCallWorkflowOnly_UsesHandlerManagerStep asserts that a workflow configured with only

‎pkg/workflow/compiler_safe_outputs_steps.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ func (c *Compiler) buildHandlerManagerStep(data *WorkflowData) []string {
276276

277277
switch ciTriggerToken {
278278
case "app":
279-
steps = append(steps, " GH_AW_CI_TRIGGER_TOKEN: ${{ needs.activation.outputs.safe_outputs_app_token || '' }}\n")
279+
steps = append(steps, " GH_AW_CI_TRIGGER_TOKEN: ${{ steps.safe-outputs-app-token.outputs.token || '' }}\n")
280280
consolidatedSafeOutputsStepsLog.Print("Extra empty commit using GitHub App token")
281281
default:
282282
// Use the magic GH_AW_CI_TRIGGER_TOKEN secret (default behavior when not explicitly configured)

‎pkg/workflow/compiler_safe_outputs_steps_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ func TestBuildSharedPRCheckoutSteps(t *testing.T) {
194194
CreatePullRequests: &CreatePullRequestsConfig{},
195195
},
196196
checkContains: []string{
197-
"token: ${{ needs.activation.outputs.safe_outputs_app_token }}",
197+
"token: ${{ steps.safe-outputs-app-token.outputs.token }}",
198198
},
199199
},
200200
{

‎pkg/workflow/create_code_scanning_alert.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,9 @@ func (c *Compiler) addUploadSARIFToken(steps *[]string, data *WorkflowData, conf
9595
safeOutputsToken = data.SafeOutputs.GitHubToken
9696
}
9797

98-
// If app is configured, use app token minted in the activation job
98+
// If app is configured, use app token
9999
if data.SafeOutputs != nil && data.SafeOutputs.GitHubApp != nil {
100-
*steps = append(*steps, " token: ${{ needs.activation.outputs.safe_outputs_app_token }}\n")
100+
*steps = append(*steps, " token: ${{ steps.safe-outputs-app-token.outputs.token }}\n")
101101
return
102102
}
103103

‎pkg/workflow/create_pull_request_ci_trigger_token_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import (
1717
// TestCreatePullRequestCITriggerToken verifies the GH_AW_CI_TRIGGER_TOKEN env var
1818
// is correctly generated in the safe_outputs job for different configurations:
1919
// - unset/empty: uses secrets.GH_AW_CI_TRIGGER_TOKEN
20-
// - "app": uses needs.activation.outputs.safe_outputs_app_token
20+
// - "app": uses steps.safe-outputs-app-token.outputs.token
2121
// - explicit token: uses the specified token value
2222
func TestCreatePullRequestCITriggerToken(t *testing.T) {
2323
tests := []struct {
@@ -35,7 +35,7 @@ func TestCreatePullRequestCITriggerToken(t *testing.T) {
3535
{
3636
name: "app config uses app token step output",
3737
tokenConfig: "app",
38-
expectedContains: "${{ needs.activation.outputs.safe_outputs_app_token || '' }}",
38+
expectedContains: "${{ steps.safe-outputs-app-token.outputs.token || '' }}",
3939
notExpected: "secrets.GH_AW_CI_TRIGGER_TOKEN",
4040
},
4141
{
@@ -135,7 +135,7 @@ func TestPushToPullRequestBranchCITriggerToken(t *testing.T) {
135135
{
136136
name: "app config uses app token step output",
137137
tokenConfig: "app",
138-
expectedContains: "${{ needs.activation.outputs.safe_outputs_app_token || '' }}",
138+
expectedContains: "${{ steps.safe-outputs-app-token.outputs.token || '' }}",
139139
},
140140
{
141141
name: "explicit token uses provided value",

0 commit comments

Comments
 (0)