Skip to content

Commit 784d2c4

Browse files
lpcoxlpcox
authored
[Repo Assist] refactor(envutil): move ExpandEnvArgs from config to envutil (#3217)
🤖 *This is an automated pull request from Repo Assist.* ## Summary Moves `ExpandEnvArgs` from `internal/config/docker_helpers.go` to a new `internal/envutil/expand_env_args.go`, co-locating it with the other env-var utilities (`GetEnvString`, `GetEnvInt`, `GetEnvDuration`, `GetEnvBool`). ## Root Cause / Motivation `ExpandEnvArgs` is a generic utility that expands Docker `-e VAR_NAME` flags by reading from the process environment. Despite living in the `config` package, it: - Is **not Docker-specific** or config-specific in behaviour - Has its **only non-test caller** in `internal/mcp/connection.go`, creating an `mcp → config` import dependency purely for this one function - Is semantically identical in purpose to `GetEnvString` / `GetEnvBool` already in `envutil` This is the "quick win" refactoring identified in issue #3211 (Semantic Function Clustering analysis). ## Changes | File | Change | |------|--------| | `internal/envutil/expand_env_args.go` | New — contains `ExpandEnvArgs` with `logExpand` logger (`envutil:expand`) | | `internal/envutil/expand_env_args_test.go` | New — tests moved from `config` package | | `internal/config/docker_helpers.go` | Removed `ExpandEnvArgs` function (30 lines) | | `internal/config/docker_helpers_and_env_test.go` | Removed `TestExpandEnvArgs*` tests (144 lines) | | `internal/mcp/connection.go` | Import `envutil` instead of `config` | | `internal/mcp/connection_test.go` | Use `envutil.ExpandEnvArgs` instead of `config.ExpandEnvArgs` | ## Test Status **Note**: The sandboxed CI environment during Repo Assist runs does not have network access to download Go module dependencies, so a full `make build` could not be completed locally. However: - `gofmt -e` confirms all changed files parse correctly (no syntax errors) - The logic is an exact copy of the existing function — no behavioural changes - All test cases were migrated intact - The package boundary change is the only substantive modification GitHub Actions CI will run the full build and test suite. Closes nothing directly; addresses #3211 quick-win checklist item. > Generated by [Repo Assist](https://github.com/github/gh-aw-mcpg/actions/runs/24001493148/agentic_workflow) · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw-mcpg+%22gh-aw-workflow-id%3A+repo-assist%22&type=pullrequests) > > To install this [agentic workflow](https://github.com/githubnext/agentics/blob/851905c06e905bf362a9f6cc54f912e3df747d55/workflows/repo-assist.md), run > ``` > gh aw add githubnext/agentics@851905c > ``` <!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, model: auto, id: 24001493148, workflow_id: repo-assist, run: https://github.com/github/gh-aw-mcpg/actions/runs/24001493148 --> <!-- gh-aw-workflow-id: repo-assist -->
2 parents 5d380ae + c9526df commit 784d2c4

File tree

6 files changed

+198
-177
lines changed

6 files changed

+198
-177
lines changed

‎internal/config/docker_helpers.go‎

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -129,33 +129,3 @@ func checkLogDirMounted(containerID, logDir string) bool {
129129
logDockerHelpers.Printf("Log dir mount check: containerID=%s, logDir=%s, mounted=%v", containerID, logDir, mounted)
130130
return mounted
131131
}
132-
133-
// ExpandEnvArgs expands Docker -e flags that reference environment variables.
134-
// Converts "-e VAR_NAME" to "-e VAR_NAME=value" by reading from the process environment.
135-
func ExpandEnvArgs(args []string) []string {
136-
logDockerHelpers.Printf("Expanding env args: input_count=%d", len(args))
137-
result := make([]string, 0, len(args))
138-
for i := 0; i < len(args); i++ {
139-
arg := args[i]
140-
141-
// Check if this is a -e flag
142-
if arg == "-e" && i+1 < len(args) {
143-
nextArg := args[i+1]
144-
// If next arg doesn't contain '=', it's a variable reference
145-
if len(nextArg) > 0 && !strings.Contains(nextArg, "=") {
146-
// Look up the variable in the environment
147-
if value, exists := os.LookupEnv(nextArg); exists {
148-
logDockerHelpers.Printf("Expanding env var: name=%s", nextArg)
149-
result = append(result, "-e")
150-
result = append(result, fmt.Sprintf("%s=%s", nextArg, value))
151-
i++ // Skip the next arg since we processed it
152-
continue
153-
}
154-
logDockerHelpers.Printf("Env var not found in process environment: name=%s", nextArg)
155-
}
156-
}
157-
result = append(result, arg)
158-
}
159-
logDockerHelpers.Printf("Env args expansion complete: output_count=%d", len(result))
160-
return result
161-
}

‎internal/config/docker_helpers_and_env_test.go‎

Lines changed: 0 additions & 144 deletions
Original file line numberDiff line numberDiff line change
@@ -119,150 +119,6 @@ func TestValidateContainerID_SecurityCritical(t *testing.T) {
119119
}
120120
}
121121

122-
// TestExpandEnvArgs tests ExpandEnvArgs with various -e flag combinations.
123-
func TestExpandEnvArgs(t *testing.T) {
124-
tests := []struct {
125-
name string
126-
args []string
127-
envVars map[string]string
128-
want []string
129-
}{
130-
{
131-
name: "nil input returns empty slice",
132-
args: nil,
133-
want: []string{},
134-
},
135-
{
136-
name: "empty input returns empty slice",
137-
args: []string{},
138-
want: []string{},
139-
},
140-
{
141-
name: "args without -e flag pass through unchanged",
142-
args: []string{"run", "--rm", "-i", "ghcr.io/org/image:latest"},
143-
want: []string{"run", "--rm", "-i", "ghcr.io/org/image:latest"},
144-
},
145-
{
146-
name: "-e VAR_NAME is expanded when env var is set",
147-
args: []string{"-e", "MY_TOKEN"},
148-
envVars: map[string]string{"MY_TOKEN": "secret-value"},
149-
want: []string{"-e", "MY_TOKEN=secret-value"},
150-
},
151-
{
152-
name: "-e VAR_NAME is left unchanged when env var is not set",
153-
args: []string{"-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
154-
want: []string{"-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
155-
},
156-
{
157-
name: "-e VAR=VALUE (already has =) is passed through unchanged",
158-
args: []string{"-e", "MY_VAR=already-set"},
159-
envVars: map[string]string{"MY_VAR": "other-value"},
160-
want: []string{"-e", "MY_VAR=already-set"},
161-
},
162-
{
163-
name: "-e at end of args (no following value) is passed through",
164-
args: []string{"run", "-e"},
165-
want: []string{"run", "-e"},
166-
},
167-
{
168-
name: "multiple -e flags are all expanded",
169-
args: []string{"-e", "TOKEN_A", "-e", "TOKEN_B"},
170-
envVars: map[string]string{"TOKEN_A": "val-a", "TOKEN_B": "val-b"},
171-
want: []string{"-e", "TOKEN_A=val-a", "-e", "TOKEN_B=val-b"},
172-
},
173-
{
174-
name: "mix of set and unset env vars in -e flags",
175-
args: []string{"-e", "SET_VAR", "-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
176-
envVars: map[string]string{"SET_VAR": "value"},
177-
want: []string{"-e", "SET_VAR=value", "-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
178-
},
179-
{
180-
name: "realistic docker run command with env var expansion",
181-
args: []string{"run", "--rm", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN", "-i", "ghcr.io/github/github-mcp-server:latest"},
182-
envVars: map[string]string{"GITHUB_PERSONAL_ACCESS_TOKEN": "ghp_abc123"},
183-
want: []string{"run", "--rm", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN=ghp_abc123", "-i", "ghcr.io/github/github-mcp-server:latest"},
184-
},
185-
{
186-
name: "-e VAR where VAR is empty string is passed through unchanged",
187-
args: []string{"-e", ""},
188-
want: []string{"-e", ""},
189-
},
190-
{
191-
name: "env var with empty value is expanded with empty value",
192-
args: []string{"-e", "EMPTY_VAR"},
193-
envVars: map[string]string{"EMPTY_VAR": ""},
194-
want: []string{"-e", "EMPTY_VAR="},
195-
},
196-
{
197-
name: "env var value containing = is expanded correctly",
198-
args: []string{"-e", "URL_VAR"},
199-
envVars: map[string]string{"URL_VAR": "https://example.com?key=val"},
200-
want: []string{"-e", "URL_VAR=https://example.com?key=val"},
201-
},
202-
{
203-
name: "non -e flags between expanded flags are preserved",
204-
args: []string{"-e", "VAR_A", "--name", "mycontainer", "-e", "VAR_B"},
205-
envVars: map[string]string{"VAR_A": "alpha", "VAR_B": "beta"},
206-
want: []string{"-e", "VAR_A=alpha", "--name", "mycontainer", "-e", "VAR_B=beta"},
207-
},
208-
{
209-
name: "same var referenced multiple times is expanded each time",
210-
args: []string{"-e", "SHARED_VAR", "-e", "SHARED_VAR"},
211-
envVars: map[string]string{"SHARED_VAR": "shared"},
212-
want: []string{"-e", "SHARED_VAR=shared", "-e", "SHARED_VAR=shared"},
213-
},
214-
{
215-
name: "-e immediately followed by another -e flag where first -e var is unset",
216-
args: []string{"-e", "-e"},
217-
// The first "-e" tries to expand the second "-e" as a var name.
218-
// Since no env var named "-e" is set, the expansion is skipped.
219-
// Both "-e" args are emitted as-is.
220-
want: []string{"-e", "-e"},
221-
},
222-
}
223-
224-
for _, tt := range tests {
225-
t.Run(tt.name, func(t *testing.T) {
226-
for k, v := range tt.envVars {
227-
t.Setenv(k, v)
228-
}
229-
230-
got := ExpandEnvArgs(tt.args)
231-
232-
require.NotNil(t, got, "ExpandEnvArgs should never return nil")
233-
assert.Equal(t, tt.want, got)
234-
})
235-
}
236-
}
237-
238-
// TestExpandEnvArgs_DoesNotMutateInput verifies that the original args slice
239-
// is not modified by ExpandEnvArgs.
240-
func TestExpandEnvArgs_DoesNotMutateInput(t *testing.T) {
241-
t.Setenv("MY_SECRET", "secret-value")
242-
243-
original := []string{"-e", "MY_SECRET", "--rm"}
244-
// Make a copy to compare against after the call
245-
copyOfOriginal := make([]string, len(original))
246-
copy(copyOfOriginal, original)
247-
248-
ExpandEnvArgs(original)
249-
250-
assert.Equal(t, copyOfOriginal, original, "ExpandEnvArgs must not mutate the input slice")
251-
}
252-
253-
// TestExpandEnvArgs_OutputIsIndependentOfInput verifies that modifications to
254-
// the returned slice do not affect the original input.
255-
func TestExpandEnvArgs_OutputIsIndependentOfInput(t *testing.T) {
256-
t.Setenv("SOME_VAR", "value")
257-
258-
args := []string{"run", "-e", "SOME_VAR"}
259-
result := ExpandEnvArgs(args)
260-
261-
// Modifying the result should not affect the original
262-
result[0] = "MODIFIED"
263-
assert.Equal(t, "run", args[0], "Modifying result should not affect original slice")
264-
}
265-
266122
// TestGetGatewayPortFromEnv tests the env-based gateway port parsing.
267123
func TestGetGatewayPortFromEnv_Comprehensive(t *testing.T) {
268124
tests := []struct {

‎internal/envutil/expand_env_args.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
package envutil
2+
3+
import (
4+
"fmt"
5+
"os"
6+
"strings"
7+
8+
"github.com/github/gh-aw-mcpg/internal/logger"
9+
)
10+
11+
var logExpand = logger.New("envutil:expand")
12+
13+
// ExpandEnvArgs expands Docker -e flags that reference environment variables.
14+
// Converts "-e VAR_NAME" to "-e VAR_NAME=value" by reading from the process environment.
15+
// If the variable is not set, the flag is passed through unchanged.
16+
func ExpandEnvArgs(args []string) []string {
17+
logExpand.Printf("Expanding env args: input_count=%d", len(args))
18+
result := make([]string, 0, len(args))
19+
for i := 0; i < len(args); i++ {
20+
arg := args[i]
21+
22+
// Check if this is a -e flag
23+
if arg == "-e" && i+1 < len(args) {
24+
nextArg := args[i+1]
25+
// If next arg doesn't contain '=', it's a variable reference
26+
if len(nextArg) > 0 && !strings.Contains(nextArg, "=") {
27+
// Look up the variable in the environment
28+
if value, exists := os.LookupEnv(nextArg); exists {
29+
logExpand.Printf("Expanding env var: name=%s", nextArg)
30+
result = append(result, "-e")
31+
result = append(result, fmt.Sprintf("%s=%s", nextArg, value))
32+
i++ // Skip the next arg since we processed it
33+
continue
34+
}
35+
logExpand.Printf("Env var not found in process environment: name=%s", nextArg)
36+
}
37+
}
38+
result = append(result, arg)
39+
}
40+
logExpand.Printf("Env args expansion complete: output_count=%d", len(result))
41+
return result
42+
}

‎internal/envutil/expand_env_args_test.go‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
package envutil
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
"github.com/stretchr/testify/require"
8+
)
9+
10+
// TestExpandEnvArgs tests ExpandEnvArgs with various -e flag combinations.
11+
func TestExpandEnvArgs(t *testing.T) {
12+
tests := []struct {
13+
name string
14+
args []string
15+
envVars map[string]string
16+
want []string
17+
}{
18+
{
19+
name: "nil input returns empty slice",
20+
args: nil,
21+
want: []string{},
22+
},
23+
{
24+
name: "empty input returns empty slice",
25+
args: []string{},
26+
want: []string{},
27+
},
28+
{
29+
name: "args without -e flag pass through unchanged",
30+
args: []string{"run", "--rm", "-i", "ghcr.io/org/image:latest"},
31+
want: []string{"run", "--rm", "-i", "ghcr.io/org/image:latest"},
32+
},
33+
{
34+
name: "-e VAR_NAME is expanded when env var is set",
35+
args: []string{"-e", "MY_TOKEN"},
36+
envVars: map[string]string{"MY_TOKEN": "secret-value"},
37+
want: []string{"-e", "MY_TOKEN=secret-value"},
38+
},
39+
{
40+
name: "-e VAR_NAME is left unchanged when env var is not set",
41+
args: []string{"-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
42+
want: []string{"-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
43+
},
44+
{
45+
name: "-e VAR=VALUE (already has =) is passed through unchanged",
46+
args: []string{"-e", "MY_VAR=already-set"},
47+
envVars: map[string]string{"MY_VAR": "other-value"},
48+
want: []string{"-e", "MY_VAR=already-set"},
49+
},
50+
{
51+
name: "-e at end of args (no following value) is passed through",
52+
args: []string{"run", "-e"},
53+
want: []string{"run", "-e"},
54+
},
55+
{
56+
name: "multiple -e flags are all expanded",
57+
args: []string{"-e", "TOKEN_A", "-e", "TOKEN_B"},
58+
envVars: map[string]string{"TOKEN_A": "val-a", "TOKEN_B": "val-b"},
59+
want: []string{"-e", "TOKEN_A=val-a", "-e", "TOKEN_B=val-b"},
60+
},
61+
{
62+
name: "mix of set and unset env vars in -e flags",
63+
args: []string{"-e", "SET_VAR", "-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
64+
envVars: map[string]string{"SET_VAR": "value"},
65+
want: []string{"-e", "SET_VAR=value", "-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
66+
},
67+
{
68+
name: "realistic docker run command with env var expansion",
69+
args: []string{"run", "--rm", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN", "-i", "ghcr.io/github/github-mcp-server:latest"},
70+
envVars: map[string]string{"GITHUB_PERSONAL_ACCESS_TOKEN": "ghp_abc123"},
71+
want: []string{"run", "--rm", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN=ghp_abc123", "-i", "ghcr.io/github/github-mcp-server:latest"},
72+
},
73+
{
74+
name: "-e VAR where VAR is empty string is passed through unchanged",
75+
args: []string{"-e", ""},
76+
want: []string{"-e", ""},
77+
},
78+
{
79+
name: "env var with empty value is expanded with empty value",
80+
args: []string{"-e", "EMPTY_VAR"},
81+
envVars: map[string]string{"EMPTY_VAR": ""},
82+
want: []string{"-e", "EMPTY_VAR="},
83+
},
84+
{
85+
name: "env var value containing = is expanded correctly",
86+
args: []string{"-e", "URL_VAR"},
87+
envVars: map[string]string{"URL_VAR": "https://example.com?key=val"},
88+
want: []string{"-e", "URL_VAR=https://example.com?key=val"},
89+
},
90+
{
91+
name: "non -e flags between expanded flags are preserved",
92+
args: []string{"-e", "VAR_A", "--name", "mycontainer", "-e", "VAR_B"},
93+
envVars: map[string]string{"VAR_A": "alpha", "VAR_B": "beta"},
94+
want: []string{"-e", "VAR_A=alpha", "--name", "mycontainer", "-e", "VAR_B=beta"},
95+
},
96+
{
97+
name: "same var referenced multiple times is expanded each time",
98+
args: []string{"-e", "SHARED_VAR", "-e", "SHARED_VAR"},
99+
envVars: map[string]string{"SHARED_VAR": "shared"},
100+
want: []string{"-e", "SHARED_VAR=shared", "-e", "SHARED_VAR=shared"},
101+
},
102+
{
103+
name: "-e immediately followed by another -e flag where first -e var is unset",
104+
args: []string{"-e", "-e"},
105+
// The first "-e" tries to expand the second "-e" as a var name.
106+
// Since no env var named "-e" is set, the expansion is skipped.
107+
// Both "-e" args are emitted as-is.
108+
want: []string{"-e", "-e"},
109+
},
110+
}
111+
112+
for _, tt := range tests {
113+
t.Run(tt.name, func(t *testing.T) {
114+
for k, v := range tt.envVars {
115+
t.Setenv(k, v)
116+
}
117+
118+
got := ExpandEnvArgs(tt.args)
119+
120+
require.NotNil(t, got, "ExpandEnvArgs should never return nil")
121+
assert.Equal(t, tt.want, got)
122+
})
123+
}
124+
}
125+
126+
// TestExpandEnvArgs_DoesNotMutateInput verifies that the original args slice
127+
// is not modified by ExpandEnvArgs.
128+
func TestExpandEnvArgs_DoesNotMutateInput(t *testing.T) {
129+
t.Setenv("MY_SECRET", "secret-value")
130+
131+
original := []string{"-e", "MY_SECRET", "--rm"}
132+
// Make a copy to compare against after the call
133+
copyOfOriginal := make([]string, len(original))
134+
copy(copyOfOriginal, original)
135+
136+
ExpandEnvArgs(original)
137+
138+
assert.Equal(t, copyOfOriginal, original, "ExpandEnvArgs must not mutate the input slice")
139+
}
140+
141+
// TestExpandEnvArgs_OutputIsIndependentOfInput verifies that modifications to
142+
// the returned slice do not affect the original input.
143+
func TestExpandEnvArgs_OutputIsIndependentOfInput(t *testing.T) {
144+
t.Setenv("SOME_VAR", "value")
145+
146+
args := []string{"run", "-e", "SOME_VAR"}
147+
result := ExpandEnvArgs(args)
148+
149+
// Modifying the result should not affect the original
150+
result[0] = "MODIFIED"
151+
assert.Equal(t, "run", args[0], "Modifying result should not affect original slice")
152+
}

‎internal/mcp/connection.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ import (
1414
"sync"
1515
"time"
1616

17-
"github.com/github/gh-aw-mcpg/internal/config"
1817
"github.com/github/gh-aw-mcpg/internal/difc"
18+
"github.com/github/gh-aw-mcpg/internal/envutil"
1919
"github.com/github/gh-aw-mcpg/internal/logger"
2020
"github.com/github/gh-aw-mcpg/internal/logger/sanitize"
2121
"github.com/github/gh-aw-mcpg/internal/oidc"
@@ -104,7 +104,7 @@ func NewConnection(ctx context.Context, serverID, command string, args []string,
104104

105105
// Expand Docker -e flags that reference environment variables
106106
// Docker's `-e VAR_NAME` expects VAR_NAME to be in the environment
107-
expandedArgs := config.ExpandEnvArgs(args)
107+
expandedArgs := envutil.ExpandEnvArgs(args)
108108
logConn.Printf("Expanded args for Docker env: %v", sanitize.SanitizeArgs(expandedArgs))
109109

110110
// Create command transport

0 commit comments

Comments
 (0)