Skip to content

Commit 96b68d8

Browse files
authored
Refactor Docker -e passthrough handling into shared envutil walker (#6034)
Semantic clustering flagged duplicated Docker `-e VAR` passthrough scanning in `launcher` and `envutil`. This change consolidates that traversal into one helper so expansion and logging stay aligned as Docker env-arg handling evolves. - **Shared env-arg traversal** - Added `envutil.WalkDockerEnvArgs(args, fn)` to centralize detection of Docker passthrough args in the form `-e VAR`. - The helper only visits passthrough entries; explicit assignments like `-e VAR=value` and incomplete trailing `-e` flags are ignored by construction. - **`ExpandEnvArgs` now delegates** - Reworked `internal/envutil/expand_env_args.go` to use the shared walker when building expanded `-e VAR=value` pairs. - Keeps existing behavior for unset vars and explicit `VAR=value` args while removing the duplicated loop skeleton. - **Launcher logging now delegates** - Reworked `internal/launcher/log_helpers.go` to use the same walker for passthrough logging. - Preserves sanitized logging for present values and distinguishes empty-but-present variables from missing ones. - **Focused coverage** - Added walker-specific tests covering set, empty, missing, explicit-assignment, and trailing-flag cases. - Updated launcher log helper coverage for empty passthrough values. - **Docs** - Updated `AGENTS.md` to reflect that `internal/envutil/` now owns Docker env-arg helper behavior in addition to general env utilities. ```go // envutil func WalkDockerEnvArgs(args []string, fn func(index int, varName, value string, found bool)) // launcher envutil.WalkDockerEnvArgs(args, func(_ int, varName, value string, found bool) { if !found { log.Printf("[LAUNCHER] ✗ WARNING: Env passthrough for %s requested but NOT FOUND in MCPG process", varName) return } if value != "" { log.Printf("[LAUNCHER] ✓ Env passthrough: %s=%s (from MCPG process)", varName, sanitize.TruncateSecret(value)) return } log.Printf("[LAUNCHER] ⚠️ Env passthrough for %s is empty in MCPG process", varName) }) ```
2 parents 0f16742 + 664bad2 commit 96b68d8

5 files changed

Lines changed: 105 additions & 37 deletions

File tree

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ Quick reference for AI agents working with MCP Gateway (Go-based MCP proxy serve
3232
- `validation.go`, `validation_env.go`, `validation_schema.go` - Fail-fast field, environment, and schema validation
3333
- `validation_test.go` - Comprehensive validation tests
3434
- `internal/difc/` - Decentralized Information Flow Control
35-
- `internal/envutil/` - Environment variable utilities
35+
- `internal/envutil/` - Environment variable and Docker env-arg utilities
3636
- `internal/guard/` - Security guards (NoopGuard, WasmGuard, WriteSinkGuard)
3737
- `internal/httputil/` - Shared HTTP helper utilities (server, proxy)
3838
- `internal/launcher/` - Backend process management

internal/envutil/expand_env_args.go

Lines changed: 33 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -10,32 +10,47 @@ import (
1010

1111
var logExpand = logger.New("envutil:expand")
1212

13+
// WalkDockerEnvArgs iterates Docker-style passthrough env args ("-e VAR_NAME")
14+
// and calls fn with the index of the "-e" flag, the variable name, its current
15+
// process value, and whether the variable exists in the environment.
16+
func WalkDockerEnvArgs(args []string, fn func(index int, varName, value string, found bool)) {
17+
for i := 0; i < len(args); i++ {
18+
arg := args[i]
19+
if arg == "-e" && i+1 < len(args) {
20+
nextArg := args[i+1]
21+
if nextArg != "" && !strings.Contains(nextArg, "=") {
22+
value, found := os.LookupEnv(nextArg)
23+
fn(i, nextArg, value, found)
24+
}
25+
i++ // Skip the next arg since we processed it
26+
}
27+
}
28+
}
29+
1330
// ExpandEnvArgs expands Docker -e flags that reference environment variables.
1431
// Converts "-e VAR_NAME" to "-e VAR_NAME=value" by reading from the process environment.
1532
// If the variable is not set, the flag is passed through unchanged.
1633
func ExpandEnvArgs(args []string) []string {
1734
logExpand.Printf("Expanding env args: input_count=%d", len(args))
35+
expandedValues := make(map[int]string)
36+
WalkDockerEnvArgs(args, func(index int, varName, value string, found bool) {
37+
if found {
38+
logExpand.Printf("Expanding env var: name=%s", varName)
39+
expandedValues[index] = fmt.Sprintf("%s=%s", varName, value)
40+
return
41+
}
42+
logExpand.Printf("Env var not found in process environment: name=%s", varName)
43+
})
44+
1845
result := make([]string, 0, len(args))
1946
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-
}
47+
if expandedValue, ok := expandedValues[i]; ok {
48+
result = append(result, "-e", expandedValue)
49+
i++ // Skip the next arg (variable name) since we just processed it; loop increment will advance past the current -e.
50+
continue
3751
}
38-
result = append(result, arg)
52+
53+
result = append(result, args[i])
3954
}
4055
logExpand.Printf("Env args expansion complete: output_count=%d", len(result))
4156
return result

internal/envutil/expand_env_args_test.go

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,3 +150,52 @@ func TestExpandEnvArgs_OutputIsIndependentOfInput(t *testing.T) {
150150
result[0] = "MODIFIED"
151151
assert.Equal(t, "run", args[0], "Modifying result should not affect original slice")
152152
}
153+
154+
func TestWalkDockerEnvArgs(t *testing.T) {
155+
t.Setenv("SET_VAR", "value")
156+
t.Setenv("EMPTY_VAR", "")
157+
158+
type walkedArg struct {
159+
index int
160+
varName string
161+
value string
162+
found bool
163+
}
164+
165+
var walked []walkedArg
166+
WalkDockerEnvArgs([]string{
167+
"run",
168+
"-e", "SET_VAR",
169+
"-e", "EXPLICIT=value",
170+
"-e", "EMPTY_VAR",
171+
"-e", "MISSING_VAR",
172+
"-e",
173+
}, func(index int, varName, value string, found bool) {
174+
walked = append(walked, walkedArg{
175+
index: index,
176+
varName: varName,
177+
value: value,
178+
found: found,
179+
})
180+
})
181+
182+
assert.Equal(t, []walkedArg{
183+
{index: 1, varName: "SET_VAR", value: "value", found: true},
184+
{index: 5, varName: "EMPTY_VAR", value: "", found: true},
185+
{index: 7, varName: "MISSING_VAR", value: "", found: false},
186+
}, walked)
187+
assert.Len(t, walked, 3)
188+
}
189+
190+
func TestWalkDockerEnvArgs_IgnoresExplicitAssignmentsAndTrailingFlag(t *testing.T) {
191+
calls := 0
192+
193+
WalkDockerEnvArgs([]string{
194+
"-e", "EXPLICIT=value",
195+
"-e",
196+
}, func(index int, varName, value string, found bool) {
197+
calls++
198+
})
199+
200+
assert.Zero(t, calls)
201+
}

internal/launcher/log_helpers.go

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,9 @@ package launcher
22

33
import (
44
"log"
5-
"os"
6-
"strings"
75

86
"github.com/github/gh-aw-mcpg/internal/config"
7+
"github.com/github/gh-aw-mcpg/internal/envutil"
98
"github.com/github/gh-aw-mcpg/internal/logger"
109
"github.com/github/gh-aw-mcpg/internal/logger/sanitize"
1110
"github.com/github/gh-aw-mcpg/internal/mcp"
@@ -38,23 +37,17 @@ func (l *Launcher) logLaunchStart(serverID, sessionID string, serverCfg *config.
3837

3938
// logEnvPassthrough checks and logs environment variable passthrough status
4039
func (l *Launcher) logEnvPassthrough(args []string) {
41-
for i := 0; i < len(args); i++ {
42-
arg := args[i]
43-
// If this arg is "-e", check the next argument
44-
if arg == "-e" && i+1 < len(args) {
45-
nextArg := args[i+1]
46-
// Check if it's a passthrough (no = sign) vs explicit value (has = sign)
47-
if !strings.Contains(nextArg, "=") {
48-
// This is a passthrough variable, check if it exists in our environment
49-
if val := os.Getenv(nextArg); val != "" {
50-
log.Printf("[LAUNCHER] ✓ Env passthrough: %s=%s (from MCPG process)", nextArg, sanitize.TruncateSecret(val))
51-
} else {
52-
log.Printf("[LAUNCHER] ✗ WARNING: Env passthrough for %s requested but NOT FOUND in MCPG process", nextArg)
53-
}
54-
}
55-
i++ // Skip the next arg since we just processed it
40+
envutil.WalkDockerEnvArgs(args, func(_ int, varName, value string, found bool) {
41+
if !found {
42+
log.Printf("[LAUNCHER] ✗ WARNING: Env passthrough for %s requested but NOT FOUND in MCPG process", varName)
43+
return
5644
}
57-
}
45+
if value != "" {
46+
log.Printf("[LAUNCHER] ✓ Env passthrough: %s=%s (from MCPG process)", varName, sanitize.TruncateSecret(value))
47+
return
48+
}
49+
log.Printf("[LAUNCHER] ⚠️ Env passthrough for %s is empty in MCPG process", varName)
50+
})
5851
}
5952

6053
// logLaunchError logs detailed launch failure diagnostics

internal/launcher/log_helpers_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,17 @@ func TestLauncher_LogEnvPassthrough(t *testing.T) {
187187
"NOT FOUND",
188188
},
189189
},
190+
{
191+
name: "passthrough empty variable value logs empty warning",
192+
args: []string{"run", "-e", "EMPTY_VAR"},
193+
setupEnv: func(t *testing.T) {
194+
t.Setenv("EMPTY_VAR", "")
195+
},
196+
wantInLog: []string{
197+
"empty in MCPG process",
198+
"EMPTY_VAR",
199+
},
200+
},
190201
{
191202
name: "explicit value not passthrough",
192203
args: []string{"run", "-e", "VAR=value"},

0 commit comments

Comments
 (0)