diff --git a/pkg/workflow/compiler_safe_outputs_job.go b/pkg/workflow/compiler_safe_outputs_job.go index 2babe5e572..dce10f510f 100644 --- a/pkg/workflow/compiler_safe_outputs_job.go +++ b/pkg/workflow/compiler_safe_outputs_job.go @@ -35,13 +35,8 @@ func (c *Compiler) buildConsolidatedSafeOutputsJob(data *WorkflowData, mainJobNa // Track whether threat detection job is enabled for step conditions threatDetectionEnabled := data.SafeOutputs.ThreatDetection != nil - // Add GitHub App token minting step if app is configured - if data.SafeOutputs.App != nil { - consolidatedSafeOutputsJobLog.Print("Adding GitHub App token minting step") - // Prepend GitHub App token step before other steps - appTokenSteps := c.buildGitHubAppTokenMintStep(data.SafeOutputs.App, permissions) - steps = append(steps, appTokenSteps...) - } + // Note: GitHub App token minting step is added later (after setup/downloads) + // to ensure proper step ordering. See insertion logic below. // Add setup action to copy JavaScript files setupActionRef := c.resolveActionReference("./actions/setup", data) @@ -244,7 +239,7 @@ func (c *Compiler) buildConsolidatedSafeOutputsJob(data *WorkflowData, mainJobNa // Add GitHub App token minting step at the beginning if app is configured if data.SafeOutputs.App != nil { appTokenSteps := c.buildGitHubAppTokenMintStep(data.SafeOutputs.App, permissions) - // Calculate insertion index: after setup action (if present) and artifact downloads, but before safe output steps + // Calculate insertion index: after setup action (if present) and artifact downloads, but before checkout and safe output steps insertIndex := 0 // Count setup action steps (checkout + setup if in dev mode without action-tag, or just setup) @@ -271,6 +266,9 @@ func (c *Compiler) buildConsolidatedSafeOutputsJob(data *WorkflowData, mainJobNa insertIndex += len(patchDownloadSteps) } + // Note: App token step must be inserted BEFORE shared checkout steps + // because those steps reference steps.safe-outputs-app-token.outputs.token + // Insert app token steps var newSteps []string newSteps = append(newSteps, steps[:insertIndex]...) diff --git a/pkg/workflow/compiler_safe_outputs_job_test.go b/pkg/workflow/compiler_safe_outputs_job_test.go index 355e25cf15..a51c1f3204 100644 --- a/pkg/workflow/compiler_safe_outputs_job_test.go +++ b/pkg/workflow/compiler_safe_outputs_job_test.go @@ -466,3 +466,45 @@ func TestJobDependencies(t *testing.T) { }) } } + +// TestGitHubAppWithPushToPRBranch tests that GitHub App token step is not duplicated +// when both app and push-to-pull-request-branch are configured +// Regression test for duplicate step bug reported in issue +func TestGitHubAppWithPushToPRBranch(t *testing.T) { + compiler := NewCompiler() + compiler.jobManager = NewJobManager() + + workflowData := &WorkflowData{ + Name: "Test Workflow", + SafeOutputs: &SafeOutputsConfig{ + App: &GitHubAppConfig{ + AppID: "${{ vars.ACTIONS_APP_ID }}", + PrivateKey: "${{ secrets.ACTIONS_PRIVATE_KEY }}", + }, + PushToPullRequestBranch: &PushToPullRequestBranchConfig{}, + }, + } + + job, _, err := compiler.buildConsolidatedSafeOutputsJob(workflowData, string(constants.AgentJobName), "test.md") + + require.NoError(t, err, "Should successfully build job") + require.NotNil(t, job, "Job should not be nil") + + stepsContent := strings.Join(job.Steps, "") + + // Should include app token minting step exactly once + tokenMintCount := strings.Count(stepsContent, "Generate GitHub App token") + assert.Equal(t, 1, tokenMintCount, "App token minting step should appear exactly once, found %d times", tokenMintCount) + + // Should include app token invalidation step exactly once + tokenInvalidateCount := strings.Count(stepsContent, "Invalidate GitHub App token") + assert.Equal(t, 1, tokenInvalidateCount, "App token invalidation step should appear exactly once, found %d times", tokenInvalidateCount) + + // Token step should come before checkout step (checkout references the token) + tokenIndex := strings.Index(stepsContent, "Generate GitHub App token") + checkoutIndex := strings.Index(stepsContent, "Checkout repository") + assert.Less(t, tokenIndex, checkoutIndex, "Token minting step should come before checkout step") + + // Verify step ID is set correctly + assert.Contains(t, stepsContent, "id: safe-outputs-app-token") +}