Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions pkg/workflow/compiler_safe_outputs_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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]...)
Expand Down
42 changes: 42 additions & 0 deletions pkg/workflow/compiler_safe_outputs_job_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Loading