diff --git a/pkg/workflow/compiler_safe_outputs_job.go b/pkg/workflow/compiler_safe_outputs_job.go index 2f5c9beeba2..e20b68404b6 100644 --- a/pkg/workflow/compiler_safe_outputs_job.go +++ b/pkg/workflow/compiler_safe_outputs_job.go @@ -13,6 +13,10 @@ import ( var consolidatedSafeOutputsJobLog = logger.New("workflow:compiler_safe_outputs_job") +// stepNameLinePrefix matches the canonical YAML line emitted by this compiler for +// step starts in job.Steps (6-space indent + "- name: "). +const stepNameLinePrefix = " - name: " + // buildConsolidatedSafeOutputsJob builds a single job containing all safe output operations // as separate steps within that job. This reduces the number of jobs in the workflow // while maintaining observability through distinct step names, IDs, and outputs. @@ -445,6 +449,15 @@ func (c *Compiler) buildSafeOutputsJobFromParts( countParentSpanID := setupParentSpanNeedsExpr(constants.ActivationJobName) insertIndex += len(c.generateSetupStep(data, setupActionRef, SetupActionDestination, data.SafeOutputs != nil && data.SafeOutputs.UploadArtifact != nil, countTraceID, countParentSpanID)) } + // Keep insertion index aligned with setup steps that may be injected between setup + // and artifact downloads. Count the entries each mask helper appends so the index + // stays self-consistent if either helper ever emits more than one step. + if isOTLPHeadersPresent(data) { + insertIndex += strings.Count(generateOTLPHeadersMaskStep(), stepNameLinePrefix) + } + if isOTLPAttributesPresent(data) { + insertIndex += strings.Count(generateOTLPAttributesMaskStep(), stepNameLinePrefix) + } // Add artifact download steps count insertIndex += len(buildAgentOutputDownloadSteps(agentArtifactPrefix, c.getActionPin)) @@ -469,6 +482,18 @@ func (c *Compiler) buildSafeOutputsJobFromParts( // Note: App token step must be inserted BEFORE shared checkout steps // because those steps reference steps.safe-outputs-app-token.outputs.token + // + // The insertion index is line-oriented; if it lands in the middle of a + // multi-line run/with block, move it to the next step boundary. + for insertIndex < len(steps) && !strings.HasPrefix(steps[insertIndex], stepNameLinePrefix) { + insertIndex++ + } + if insertIndex == len(steps) { + consolidatedSafeOutputsJobLog.Printf( + "WARN: app-token insertion reached end of steps slice (len=%d); step ordering may be incorrect", + len(steps), + ) + } // Insert app token steps var newSteps []string diff --git a/pkg/workflow/compiler_safe_outputs_job_test.go b/pkg/workflow/compiler_safe_outputs_job_test.go index 4bc3b50ca08..02df1ec3336 100644 --- a/pkg/workflow/compiler_safe_outputs_job_test.go +++ b/pkg/workflow/compiler_safe_outputs_job_test.go @@ -813,7 +813,141 @@ func TestGitHubAppWithPushToPRBranch(t *testing.T) { assert.Contains(t, stepsContent, "id: safe-outputs-app-token") } -// TestJobWithGitHubAppWorkflowCallUsesTargetRepoNameFallback is a regression test verifying that +// TestGitHubAppTokenStepWithOTLPHeaders ensures the app token step insertion point +// stays aligned when OTLP header masking steps are present. +func TestGitHubAppTokenStepWithOTLPHeaders(t *testing.T) { + compiler := NewCompiler() + compiler.jobManager = NewJobManager() + + workflowData := &WorkflowData{ + Name: "Test Workflow", + Env: `"env": + OTEL_EXPORTER_OTLP_HEADERS: "Authorization=******"`, + SafeOutputs: &SafeOutputsConfig{ + GitHubApp: &GitHubAppConfig{ + AppID: "${{ vars.ACTIONS_APP_ID }}", + PrivateKey: "${{ secrets.ACTIONS_PRIVATE_KEY }}", + }, + AddComments: &AddCommentsConfig{}, + AddLabels: &AddLabelsConfig{ + Allowed: []string{"bug"}, + }, + }, + } + + 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, "") + maskIndex := strings.Index(stepsContent, "Mask OTLP telemetry headers") + downloadIndex := strings.Index(stepsContent, "Download agent output artifact") + setupEnvEchoIndex := strings.Index(stepsContent, "GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json") + tokenIndex := strings.Index(stepsContent, "Generate GitHub App token") + + require.NotEqual(t, -1, maskIndex, "OTLP headers mask step should be present") + require.NotEqual(t, -1, downloadIndex, "agent output download step should be present") + require.NotEqual(t, -1, setupEnvEchoIndex, "setup-agent-output-env run command should be present") + require.NotEqual(t, -1, tokenIndex, "GitHub App token step should be present") + + assert.Less(t, maskIndex, downloadIndex, "masking should happen before artifact download") + assert.Less(t, setupEnvEchoIndex, tokenIndex, "app token step must be inserted after setup-agent-output-env run block") + // Regression guard: app token insertion must not split setup-agent-output-env and leave + // an `echo ...` run line nested under a previous `with:` block. + assert.NotContains(t, stepsContent, "with:\n echo \"GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json\" >> \"$GITHUB_OUTPUT\"", + "app token step must not split setup-agent-output-env into a nested echo under with:") +} + +// TestGitHubAppTokenStepWithOTLPAttributes ensures the app token step insertion point +// stays aligned when the OTLP attributes masking step is present (GH_AW_OTLP_ATTRIBUTES path). +func TestGitHubAppTokenStepWithOTLPAttributes(t *testing.T) { + compiler := NewCompiler() + compiler.jobManager = NewJobManager() + + workflowData := &WorkflowData{ + Name: "Test Workflow", + Env: `"env": + GH_AW_OTLP_ATTRIBUTES: "{\"service.name\":\"test\"}"`, + SafeOutputs: &SafeOutputsConfig{ + GitHubApp: &GitHubAppConfig{ + AppID: "${{ vars.ACTIONS_APP_ID }}", + PrivateKey: "${{ secrets.ACTIONS_PRIVATE_KEY }}", + }, + AddComments: &AddCommentsConfig{}, + AddLabels: &AddLabelsConfig{ + Allowed: []string{"bug"}, + }, + }, + } + + 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, "") + attrMaskIndex := strings.Index(stepsContent, "Mask OTLP custom attribute values") + downloadIndex := strings.Index(stepsContent, "Download agent output artifact") + setupEnvEchoIndex := strings.Index(stepsContent, "GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json") + tokenIndex := strings.Index(stepsContent, "Generate GitHub App token") + + require.NotEqual(t, -1, attrMaskIndex, "OTLP attributes mask step should be present") + require.NotEqual(t, -1, downloadIndex, "agent output download step should be present") + require.NotEqual(t, -1, setupEnvEchoIndex, "setup-agent-output-env run command should be present") + require.NotEqual(t, -1, tokenIndex, "GitHub App token step should be present") + + assert.Less(t, attrMaskIndex, downloadIndex, "attributes masking should happen before artifact download") + assert.Less(t, setupEnvEchoIndex, tokenIndex, "app token step must be inserted after setup-agent-output-env run block") + assert.NotContains(t, stepsContent, "with:\n echo \"GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json\" >> \"$GITHUB_OUTPUT\"", + "app token step must not split setup-agent-output-env into a nested echo under with:") +} + +// TestGitHubAppTokenStepWithOTLPHeadersAndAttributes ensures the app token step insertion +// point stays aligned when both OTLP header masking and attribute masking steps are present +// (insertIndex is incremented twice — highest-risk composition). +func TestGitHubAppTokenStepWithOTLPHeadersAndAttributes(t *testing.T) { + compiler := NewCompiler() + compiler.jobManager = NewJobManager() + + workflowData := &WorkflowData{ + Name: "Test Workflow", + Env: `"env": + OTEL_EXPORTER_OTLP_HEADERS: "Authorization=******" + GH_AW_OTLP_ATTRIBUTES: "{\"service.name\":\"test\"}"`, + SafeOutputs: &SafeOutputsConfig{ + GitHubApp: &GitHubAppConfig{ + AppID: "${{ vars.ACTIONS_APP_ID }}", + PrivateKey: "${{ secrets.ACTIONS_PRIVATE_KEY }}", + }, + AddComments: &AddCommentsConfig{}, + AddLabels: &AddLabelsConfig{ + Allowed: []string{"bug"}, + }, + }, + } + + 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, "") + headerMaskIndex := strings.Index(stepsContent, "Mask OTLP telemetry headers") + attrMaskIndex := strings.Index(stepsContent, "Mask OTLP custom attribute values") + downloadIndex := strings.Index(stepsContent, "Download agent output artifact") + setupEnvEchoIndex := strings.Index(stepsContent, "GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json") + tokenIndex := strings.Index(stepsContent, "Generate GitHub App token") + + require.NotEqual(t, -1, headerMaskIndex, "OTLP headers mask step should be present") + require.NotEqual(t, -1, attrMaskIndex, "OTLP attributes mask step should be present") + require.NotEqual(t, -1, downloadIndex, "agent output download step should be present") + require.NotEqual(t, -1, setupEnvEchoIndex, "setup-agent-output-env run command should be present") + require.NotEqual(t, -1, tokenIndex, "GitHub App token step should be present") + + assert.Less(t, headerMaskIndex, downloadIndex, "header masking should happen before artifact download") + assert.Less(t, attrMaskIndex, downloadIndex, "attributes masking should happen before artifact download") + assert.Less(t, setupEnvEchoIndex, tokenIndex, "app token step must be inserted after setup-agent-output-env run block") + assert.NotContains(t, stepsContent, "with:\n echo \"GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json\" >> \"$GITHUB_OUTPUT\"", + "app token step must not split setup-agent-output-env into a nested echo under with:") +} // a safe-output job compiled for a workflow_call trigger uses // needs.activation.outputs.target_repo_name (repo name only, no owner prefix) as the repositories // fallback for the GitHub App token mint step, instead of the full target_repo slug.