From 6deb686f487652927ce042ff1b7f077895e98719 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 20 May 2026 03:37:24 +0000 Subject: [PATCH 1/3] Investigate and fix non-deterministic env var generation in lock files Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/smoke-otel-backends.lock.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/smoke-otel-backends.lock.yml b/.github/workflows/smoke-otel-backends.lock.yml index 6f51faa9a84..092ac357201 100644 --- a/.github/workflows/smoke-otel-backends.lock.yml +++ b/.github/workflows/smoke-otel-backends.lock.yml @@ -773,7 +773,7 @@ jobs: "url": "https://mcp.datadoghq.com/api/unstable/mcp-server/mcp?toolsets=core", "headers": { "DD_API_KEY": "\${DD_API_KEY}", - "DD_APPLICATION_KEY": "\${DD_APPLICATION_KEY}", + "DD_APPLICATION_KEY": "\${DD_APP_KEY}", "DD_SITE": "\${DD_SITE}" }, "tools": [ From dab1188e5889f768a7ae037fdd2192da5a71c392 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 20 May 2026 03:41:45 +0000 Subject: [PATCH 2/3] Fix non-deterministic secret replacement with sorted map iteration Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../workflows/smoke-otel-backends.lock.yml | 2 +- pkg/workflow/secret_extraction.go | 44 +++++++++++-- pkg/workflow/secret_extraction_test.go | 65 +++++++++++++++++++ 3 files changed, 104 insertions(+), 7 deletions(-) diff --git a/.github/workflows/smoke-otel-backends.lock.yml b/.github/workflows/smoke-otel-backends.lock.yml index 092ac357201..6f51faa9a84 100644 --- a/.github/workflows/smoke-otel-backends.lock.yml +++ b/.github/workflows/smoke-otel-backends.lock.yml @@ -773,7 +773,7 @@ jobs: "url": "https://mcp.datadoghq.com/api/unstable/mcp-server/mcp?toolsets=core", "headers": { "DD_API_KEY": "\${DD_API_KEY}", - "DD_APPLICATION_KEY": "\${DD_APP_KEY}", + "DD_APPLICATION_KEY": "\${DD_APPLICATION_KEY}", "DD_SITE": "\${DD_SITE}" }, "tools": [ diff --git a/pkg/workflow/secret_extraction.go b/pkg/workflow/secret_extraction.go index 750c4c5bb32..115921da427 100644 --- a/pkg/workflow/secret_extraction.go +++ b/pkg/workflow/secret_extraction.go @@ -3,6 +3,7 @@ package workflow import ( "maps" "regexp" + "sort" "strings" "github.com/github/gh-aw/pkg/logger" @@ -121,7 +122,18 @@ func ExtractEnvExpressionsFromMap(values map[string]string) map[string]string { // The backslash is used to escape the ${} for proper JSON rendering in Copilot configs func ReplaceSecretsWithEnvVars(value string, secrets map[string]string) string { result := value - for varName, secretExpr := range secrets { + // Sort keys for deterministic output. When multiple secret names share the same + // expression (e.g. "${{ secrets.DD_APPLICATION_KEY || secrets.DD_APP_KEY }}" maps + // to both "DD_APPLICATION_KEY" and "DD_APP_KEY"), the alphabetically first key is + // processed first and its replacement wins; subsequent keys find the expression + // already replaced and are no-ops. This ensures stable lock-file output across runs. + sortedKeys := make([]string, 0, len(secrets)) + for k := range secrets { + sortedKeys = append(sortedKeys, k) + } + sort.Strings(sortedKeys) + for _, varName := range sortedKeys { + secretExpr := secrets[varName] // Replace ${{ secrets.VAR }} with \${VAR} (backslash-escaped for copilot JSON config) result = strings.ReplaceAll(result, secretExpr, "\\${"+varName+"}") } @@ -137,7 +149,14 @@ func ReplaceSecretsWithEnvVars(value string, secrets map[string]string) string { func ReplaceSecretsWithBashVars(value string) string { result := value secrets := ExtractSecretsFromValue(value) - for varName, secretExpr := range secrets { + // Sort keys for deterministic output; see ReplaceSecretsWithEnvVars for rationale. + sortedKeys := make([]string, 0, len(secrets)) + for k := range secrets { + sortedKeys = append(sortedKeys, k) + } + sort.Strings(sortedKeys) + for _, varName := range sortedKeys { + secretExpr := secrets[varName] result = strings.ReplaceAll(result, secretExpr, "${"+varName+"}") } return result @@ -321,15 +340,28 @@ func formatInputNameAsEnvVar(inputName string) string { func ReplaceTemplateExpressionsWithEnvVars(value string) string { result := value - // Extract and replace secrets + // Extract and replace secrets — sort keys for deterministic output; see + // ReplaceSecretsWithEnvVars for rationale. secrets := ExtractSecretsFromValue(value) - for varName, secretExpr := range secrets { + secretKeys := make([]string, 0, len(secrets)) + for k := range secrets { + secretKeys = append(secretKeys, k) + } + sort.Strings(secretKeys) + for _, varName := range secretKeys { + secretExpr := secrets[varName] result = strings.ReplaceAll(result, secretExpr, "\\${"+varName+"}") } - // Extract and replace env vars + // Extract and replace env vars — sort keys for deterministic output. envVars := ExtractEnvExpressionsFromValue(value) - for varName, envExpr := range envVars { + envKeys := make([]string, 0, len(envVars)) + for k := range envVars { + envKeys = append(envKeys, k) + } + sort.Strings(envKeys) + for _, varName := range envKeys { + envExpr := envVars[varName] result = strings.ReplaceAll(result, envExpr, "\\${"+varName+"}") } diff --git a/pkg/workflow/secret_extraction_test.go b/pkg/workflow/secret_extraction_test.go index 0c20a75dd57..368b0d72b7b 100644 --- a/pkg/workflow/secret_extraction_test.go +++ b/pkg/workflow/secret_extraction_test.go @@ -333,6 +333,71 @@ func TestSharedReplaceSecretsWithEnvVars(t *testing.T) { } } +// TestReplaceSecretsWithEnvVars_FallbackExpressionDeterminism verifies that when a single +// GitHub Actions expression references two secrets (e.g. a fallback pattern like +// "${{ secrets.DD_APPLICATION_KEY || secrets.DD_APP_KEY }}"), the replacement is +// deterministic across repeated calls. Previously, Go map iteration produced either +// "\${DD_APP_KEY}" or "\${DD_APPLICATION_KEY}" non-deterministically, causing +// smoke-otel-backends.lock.yml to differ between compiler runs. +func TestReplaceSecretsWithEnvVars_FallbackExpressionDeterminism(t *testing.T) { + // Both secrets map to the same expression — this is what ExtractSecretsFromMap + // produces for "${{ secrets.DD_APPLICATION_KEY || secrets.DD_APP_KEY }}". + sharedExpr := "${{ secrets.DD_APPLICATION_KEY || secrets.DD_APP_KEY }}" + secrets := map[string]string{ + "DD_APPLICATION_KEY": sharedExpr, + "DD_APP_KEY": sharedExpr, + } + + // Run many times to surface any non-determinism from Go map iteration. + const runs = 50 + var first string + for i := range runs { + got := ReplaceSecretsWithEnvVars(sharedExpr, secrets) + if i == 0 { + first = got + continue + } + if got != first { + t.Errorf("non-deterministic output: run 0 produced %q, run %d produced %q", first, i, got) + } + } + + // The alphabetically first key ("DD_APPLICATION_KEY") should win because it is processed + // first in sorted order and performs the replacement; the second key finds nothing + // to replace. Note: '_' (ASCII 95) > 'L' (ASCII 76), so "DD_APPLICATION_KEY" sorts + // before "DD_APP_KEY" (comparison diverges at position 6: 'L' < '_'). + want := "\\${DD_APPLICATION_KEY}" + if first != want { + t.Errorf("ReplaceSecretsWithEnvVars(%q, secrets) = %q, want %q", sharedExpr, first, want) + } +} + +// TestReplaceSecretsWithBashVars_FallbackExpressionDeterminism verifies that +// ReplaceSecretsWithBashVars produces deterministic output for fallback expressions +// with two secret references. Mirrors TestReplaceSecretsWithEnvVars_FallbackExpressionDeterminism. +func TestReplaceSecretsWithBashVars_FallbackExpressionDeterminism(t *testing.T) { + value := "${{ secrets.DD_APPLICATION_KEY || secrets.DD_APP_KEY }}" + + const runs = 50 + var first string + for i := range runs { + got := ReplaceSecretsWithBashVars(value) + if i == 0 { + first = got + continue + } + if got != first { + t.Errorf("non-deterministic output: run 0 produced %q, run %d produced %q", first, i, got) + } + } + + // "DD_APPLICATION_KEY" sorts before "DD_APP_KEY" ('L' < '_'), so it wins. + want := "${DD_APPLICATION_KEY}" + if first != want { + t.Errorf("ReplaceSecretsWithBashVars(%q) = %q, want %q", value, first, want) + } +} + // TestSharedExtractSecretsFromValueEdgeCases tests edge cases for the shared ExtractSecretsFromValue utility function func TestSharedExtractSecretsFromValueEdgeCases(t *testing.T) { tests := []struct { From b1044c8cf1e75e488554d2de171ff55ee1174427 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 20 May 2026 03:44:31 +0000 Subject: [PATCH 3/3] Refactor: use existing sortedMapKeys helper in secret_extraction.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/secret_extraction.go | 29 ++++------------------------- 1 file changed, 4 insertions(+), 25 deletions(-) diff --git a/pkg/workflow/secret_extraction.go b/pkg/workflow/secret_extraction.go index 115921da427..ca651c7d649 100644 --- a/pkg/workflow/secret_extraction.go +++ b/pkg/workflow/secret_extraction.go @@ -3,7 +3,6 @@ package workflow import ( "maps" "regexp" - "sort" "strings" "github.com/github/gh-aw/pkg/logger" @@ -127,12 +126,7 @@ func ReplaceSecretsWithEnvVars(value string, secrets map[string]string) string { // to both "DD_APPLICATION_KEY" and "DD_APP_KEY"), the alphabetically first key is // processed first and its replacement wins; subsequent keys find the expression // already replaced and are no-ops. This ensures stable lock-file output across runs. - sortedKeys := make([]string, 0, len(secrets)) - for k := range secrets { - sortedKeys = append(sortedKeys, k) - } - sort.Strings(sortedKeys) - for _, varName := range sortedKeys { + for _, varName := range sortedMapKeys(secrets) { secretExpr := secrets[varName] // Replace ${{ secrets.VAR }} with \${VAR} (backslash-escaped for copilot JSON config) result = strings.ReplaceAll(result, secretExpr, "\\${"+varName+"}") @@ -150,12 +144,7 @@ func ReplaceSecretsWithBashVars(value string) string { result := value secrets := ExtractSecretsFromValue(value) // Sort keys for deterministic output; see ReplaceSecretsWithEnvVars for rationale. - sortedKeys := make([]string, 0, len(secrets)) - for k := range secrets { - sortedKeys = append(sortedKeys, k) - } - sort.Strings(sortedKeys) - for _, varName := range sortedKeys { + for _, varName := range sortedMapKeys(secrets) { secretExpr := secrets[varName] result = strings.ReplaceAll(result, secretExpr, "${"+varName+"}") } @@ -343,24 +332,14 @@ func ReplaceTemplateExpressionsWithEnvVars(value string) string { // Extract and replace secrets — sort keys for deterministic output; see // ReplaceSecretsWithEnvVars for rationale. secrets := ExtractSecretsFromValue(value) - secretKeys := make([]string, 0, len(secrets)) - for k := range secrets { - secretKeys = append(secretKeys, k) - } - sort.Strings(secretKeys) - for _, varName := range secretKeys { + for _, varName := range sortedMapKeys(secrets) { secretExpr := secrets[varName] result = strings.ReplaceAll(result, secretExpr, "\\${"+varName+"}") } // Extract and replace env vars — sort keys for deterministic output. envVars := ExtractEnvExpressionsFromValue(value) - envKeys := make([]string, 0, len(envVars)) - for k := range envVars { - envKeys = append(envKeys, k) - } - sort.Strings(envKeys) - for _, varName := range envKeys { + for _, varName := range sortedMapKeys(envVars) { envExpr := envVars[varName] result = strings.ReplaceAll(result, envExpr, "\\${"+varName+"}") }