Skip to content

Commit f08b1ad

Browse files
Copilotpelikhan
andauthored
Sort MCP server environment variables alphabetically during workflow compilation (#1302)
* Initial plan * Sort MCP env vars alphabetically during compilation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Recompile workflows with sorted env vars Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Add test for MCP env var alphabetical sorting Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Sort MCP server environment variables alphabetically during workflow compilation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * sort envs --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Co-authored-by: Peli de Halleux <pelikhan@users.noreply.github.com>
1 parent 5355ae9 commit f08b1ad

File tree

3 files changed

+96
-9
lines changed

3 files changed

+96
-9
lines changed

.github/workflows/duplicate-code-detector.lock.yml

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/workflow/mcp-config.go

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package workflow
22

33
import (
44
"fmt"
5+
"sort"
56
"strings"
67

78
"github.com/githubnext/gh-aw/pkg/console"
@@ -183,13 +184,16 @@ func renderSharedMCPConfig(yaml *strings.Builder, toolName string, toolConfig ma
183184
case "env":
184185
if renderer.Format == "toml" {
185186
fmt.Fprintf(yaml, "%senv = { ", renderer.IndentLevel)
186-
first := true
187-
for envKey, envValue := range mcpConfig.Env {
188-
if !first {
187+
envKeys := make([]string, 0, len(mcpConfig.Env))
188+
for key := range mcpConfig.Env {
189+
envKeys = append(envKeys, key)
190+
}
191+
sort.Strings(envKeys)
192+
for i, envKey := range envKeys {
193+
if i > 0 {
189194
yaml.WriteString(", ")
190195
}
191-
fmt.Fprintf(yaml, "\"%s\" = \"%s\"", envKey, envValue)
192-
first = false
196+
fmt.Fprintf(yaml, "\"%s\" = \"%s\"", envKey, mcpConfig.Env[envKey])
193197
}
194198
yaml.WriteString(" }\n")
195199
} else {
@@ -202,6 +206,7 @@ func renderSharedMCPConfig(yaml *strings.Builder, toolName string, toolConfig ma
202206
for key := range mcpConfig.Env {
203207
envKeys = append(envKeys, key)
204208
}
209+
sort.Strings(envKeys)
205210
for envIndex, envKey := range envKeys {
206211
envComma := ","
207212
if envIndex == len(envKeys)-1 {
@@ -227,6 +232,7 @@ func renderSharedMCPConfig(yaml *strings.Builder, toolName string, toolConfig ma
227232
for key := range mcpConfig.Headers {
228233
headerKeys = append(headerKeys, key)
229234
}
235+
sort.Strings(headerKeys)
230236
for headerIndex, headerKey := range headerKeys {
231237
headerComma := ","
232238
if headerIndex == len(headerKeys)-1 {

pkg/workflow/mcp_config_compilation_test.go

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,87 @@ Please use the markitdown MCP server to convert HTML to markdown.
116116
}
117117
}
118118

119+
// TestMCPEnvVarsAlphabeticallySorted verifies that env vars in MCP configs are sorted alphabetically
120+
func TestMCPEnvVarsAlphabeticallySorted(t *testing.T) {
121+
// Create a temporary markdown file with mcp-servers configuration containing env vars
122+
workflowContent := `---
123+
on:
124+
workflow_dispatch:
125+
permissions:
126+
contents: read
127+
engine: copilot
128+
mcp-servers:
129+
test-server:
130+
container: example/test:latest
131+
env:
132+
ZEBRA_VAR: "z"
133+
ALPHA_VAR: "a"
134+
BETA_VAR: "b"
135+
---
136+
137+
# Test MCP Env Var Sorting
138+
139+
This workflow tests that MCP server env vars are sorted alphabetically.
140+
`
141+
142+
// Create temporary file
143+
tmpFile, err := os.CreateTemp("", "test-env-sort-*.md")
144+
if err != nil {
145+
t.Fatalf("Failed to create temp file: %v", err)
146+
}
147+
defer os.Remove(tmpFile.Name())
148+
149+
// Write content to file
150+
if _, err := tmpFile.WriteString(workflowContent); err != nil {
151+
t.Fatalf("Failed to write to temp file: %v", err)
152+
}
153+
tmpFile.Close()
154+
155+
// Create compiler and compile workflow
156+
compiler := NewCompiler(false, "", "test")
157+
compiler.SetSkipValidation(true)
158+
159+
// Generate YAML
160+
workflowData, err := compiler.ParseWorkflowFile(tmpFile.Name())
161+
if err != nil {
162+
t.Fatalf("Failed to parse workflow file: %v", err)
163+
}
164+
165+
yamlContent, err := compiler.generateYAML(workflowData, tmpFile.Name())
166+
if err != nil {
167+
t.Fatalf("Failed to generate YAML: %v", err)
168+
}
169+
170+
// Find the env section in the generated YAML
171+
envIndex := strings.Index(yamlContent, `"env": {`)
172+
if envIndex == -1 {
173+
t.Fatalf("Could not find env section in generated YAML")
174+
}
175+
176+
// Extract a portion of YAML starting from env section (next 300 chars should be enough)
177+
envSection := yamlContent[envIndex : envIndex+300]
178+
179+
// Verify that ALPHA_VAR appears before BETA_VAR and ZEBRA_VAR
180+
alphaIndex := strings.Index(envSection, `"ALPHA_VAR"`)
181+
betaIndex := strings.Index(envSection, `"BETA_VAR"`)
182+
zebraIndex := strings.Index(envSection, `"ZEBRA_VAR"`)
183+
184+
if alphaIndex == -1 || betaIndex == -1 || zebraIndex == -1 {
185+
t.Fatalf("Could not find all env vars in generated YAML. Section: %s", envSection)
186+
}
187+
188+
// Verify alphabetical order
189+
if alphaIndex >= betaIndex {
190+
t.Errorf("Expected ALPHA_VAR to appear before BETA_VAR, but ALPHA_VAR is at %d and BETA_VAR is at %d", alphaIndex, betaIndex)
191+
}
192+
if betaIndex >= zebraIndex {
193+
t.Errorf("Expected BETA_VAR to appear before ZEBRA_VAR, but BETA_VAR is at %d and ZEBRA_VAR is at %d", betaIndex, zebraIndex)
194+
}
195+
if alphaIndex >= zebraIndex {
196+
t.Errorf("Expected ALPHA_VAR to appear before ZEBRA_VAR, but ALPHA_VAR is at %d and ZEBRA_VAR is at %d", alphaIndex, zebraIndex)
197+
}
198+
}
199+
119200
// TestHasMCPConfigDetection verifies that hasMCPConfig properly detects MCP configurations
120201
func TestHasMCPConfigDetection(t *testing.T) {
121202
testCases := []struct {

0 commit comments

Comments
 (0)