From 5071ce1863c54b48a03ff87765c782f2bd43ad00 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 5 Jun 2026 05:53:53 +0000 Subject: [PATCH 1/5] Initial plan From cdbaf26a26cfb0d918fb7bdd2961f11bc4ac5a91 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 5 Jun 2026 06:05:11 +0000 Subject: [PATCH 2/5] Fix safe-outputs GitHub App insertion when OTLP masking is enabled Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_safe_outputs_job.go | 14 +++++++ .../compiler_safe_outputs_job_test.go | 42 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/pkg/workflow/compiler_safe_outputs_job.go b/pkg/workflow/compiler_safe_outputs_job.go index 2f5c9beeba2..4ed371c762f 100644 --- a/pkg/workflow/compiler_safe_outputs_job.go +++ b/pkg/workflow/compiler_safe_outputs_job.go @@ -445,6 +445,14 @@ 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 (single string entry each). + if isOTLPHeadersPresent(data) { + insertIndex++ + } + if isOTLPAttributesPresent(data) { + insertIndex++ + } // Add artifact download steps count insertIndex += len(buildAgentOutputDownloadSteps(agentArtifactPrefix, c.getActionPin)) @@ -469,6 +477,12 @@ 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], " - name: ") { + insertIndex++ + } // 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..6600ce3f852 100644 --- a/pkg/workflow/compiler_safe_outputs_job_test.go +++ b/pkg/workflow/compiler_safe_outputs_job_test.go @@ -813,6 +813,48 @@ func TestGitHubAppWithPushToPRBranch(t *testing.T) { assert.Contains(t, stepsContent, "id: safe-outputs-app-token") } +// 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") + assert.NotContains(t, stepsContent, "with:\n echo \"GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json\" >> \"$GITHUB_OUTPUT\"") +} + // TestJobWithGitHubAppWorkflowCallUsesTargetRepoNameFallback is a regression test verifying that // 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 From 1ecc9f896e60173ed513e462604966098a1681ad Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 5 Jun 2026 06:06:16 +0000 Subject: [PATCH 3/5] Harden safe-outputs token insertion boundary logic Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_safe_outputs_job.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/workflow/compiler_safe_outputs_job.go b/pkg/workflow/compiler_safe_outputs_job.go index 4ed371c762f..e20870bf324 100644 --- a/pkg/workflow/compiler_safe_outputs_job.go +++ b/pkg/workflow/compiler_safe_outputs_job.go @@ -13,6 +13,8 @@ import ( var consolidatedSafeOutputsJobLog = logger.New("workflow:compiler_safe_outputs_job") +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. @@ -446,7 +448,8 @@ func (c *Compiler) buildSafeOutputsJobFromParts( 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 (single string entry each). + // and artifact downloads. Each mask helper currently appends one string entry to + // the steps slice (containing the full two-line YAML step). if isOTLPHeadersPresent(data) { insertIndex++ } @@ -480,7 +483,7 @@ func (c *Compiler) buildSafeOutputsJobFromParts( // // 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], " - name: ") { + for insertIndex < len(steps) && !strings.HasPrefix(steps[insertIndex], stepNameLinePrefix) { insertIndex++ } From 7cd1c8484051df6be2bae242947d435d152c560f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 5 Jun 2026 06:07:25 +0000 Subject: [PATCH 4/5] Clarify safe-outputs insertion regression guards Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_safe_outputs_job.go | 2 ++ pkg/workflow/compiler_safe_outputs_job_test.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/pkg/workflow/compiler_safe_outputs_job.go b/pkg/workflow/compiler_safe_outputs_job.go index e20870bf324..595eac7af44 100644 --- a/pkg/workflow/compiler_safe_outputs_job.go +++ b/pkg/workflow/compiler_safe_outputs_job.go @@ -13,6 +13,8 @@ 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 diff --git a/pkg/workflow/compiler_safe_outputs_job_test.go b/pkg/workflow/compiler_safe_outputs_job_test.go index 6600ce3f852..5057df29f72 100644 --- a/pkg/workflow/compiler_safe_outputs_job_test.go +++ b/pkg/workflow/compiler_safe_outputs_job_test.go @@ -852,6 +852,8 @@ func TestGitHubAppTokenStepWithOTLPHeaders(t *testing.T) { 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\"") } From 4f9fa258be111e26336d81870d22abd79a9d7a92 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 5 Jun 2026 11:52:48 +0000 Subject: [PATCH 5/5] Address review feedback: add tests, warning log, and dynamic offset counting Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_safe_outputs_job.go | 14 ++- .../compiler_safe_outputs_job_test.go | 94 ++++++++++++++++++- 2 files changed, 102 insertions(+), 6 deletions(-) diff --git a/pkg/workflow/compiler_safe_outputs_job.go b/pkg/workflow/compiler_safe_outputs_job.go index 595eac7af44..e20b68404b6 100644 --- a/pkg/workflow/compiler_safe_outputs_job.go +++ b/pkg/workflow/compiler_safe_outputs_job.go @@ -450,13 +450,13 @@ func (c *Compiler) buildSafeOutputsJobFromParts( 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. Each mask helper currently appends one string entry to - // the steps slice (containing the full two-line YAML step). + // 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++ + insertIndex += strings.Count(generateOTLPHeadersMaskStep(), stepNameLinePrefix) } if isOTLPAttributesPresent(data) { - insertIndex++ + insertIndex += strings.Count(generateOTLPAttributesMaskStep(), stepNameLinePrefix) } // Add artifact download steps count @@ -488,6 +488,12 @@ func (c *Compiler) buildSafeOutputsJobFromParts( 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 5057df29f72..02df1ec3336 100644 --- a/pkg/workflow/compiler_safe_outputs_job_test.go +++ b/pkg/workflow/compiler_safe_outputs_job_test.go @@ -854,10 +854,100 @@ func TestGitHubAppTokenStepWithOTLPHeaders(t *testing.T) { 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\"") + 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:") } -// TestJobWithGitHubAppWorkflowCallUsesTargetRepoNameFallback is a regression test verifying that +// 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.