mirror of
https://github.com/sipeed/picoclaw.git
synced 2026-06-12 18:08:54 +00:00
fix(mcp): ensure proper environment variable override semantics
Use a map to merge environment variables with guaranteed override behavior. Config variables (cfg.Env) now properly override file variables (envFile), which in turn override parent process environment. Previously, simply appending to a slice could result in duplicate variables, and while most systems use the last occurrence, this behavior is not guaranteed and could lead to unexpected results. Addresses Copilot code review feedback.
This commit is contained in:
+18
-10
@@ -284,8 +284,16 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP
|
||||
// Create command with context
|
||||
cmd := exec.CommandContext(ctx, cfg.Command, cfg.Args...)
|
||||
|
||||
// Set environment variables
|
||||
env := cmd.Environ()
|
||||
// Build environment variables with proper override semantics
|
||||
// Use a map to ensure config variables override file variables
|
||||
envMap := make(map[string]string)
|
||||
|
||||
// Start with parent process environment
|
||||
for _, e := range cmd.Environ() {
|
||||
if idx := strings.Index(e, "="); idx > 0 {
|
||||
envMap[e[:idx]] = e[idx+1:]
|
||||
}
|
||||
}
|
||||
|
||||
// Load environment variables from file if specified
|
||||
if cfg.EnvFile != "" {
|
||||
@@ -294,7 +302,7 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP
|
||||
return fmt.Errorf("failed to load env file %s: %w", cfg.EnvFile, err)
|
||||
}
|
||||
for k, v := range envVars {
|
||||
env = append(env, fmt.Sprintf("%s=%s", k, v))
|
||||
envMap[k] = v
|
||||
}
|
||||
logger.DebugCF("mcp", "Loaded environment variables from file",
|
||||
map[string]interface{}{
|
||||
@@ -305,16 +313,16 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP
|
||||
}
|
||||
|
||||
// Environment variables from config override those from file
|
||||
if len(cfg.Env) > 0 {
|
||||
for k, v := range cfg.Env {
|
||||
env = append(env, fmt.Sprintf("%s=%s", k, v))
|
||||
}
|
||||
for k, v := range cfg.Env {
|
||||
envMap[k] = v
|
||||
}
|
||||
|
||||
// Set environment if we added any variables
|
||||
if len(env) > len(cmd.Environ()) {
|
||||
cmd.Env = env
|
||||
// Convert map to slice
|
||||
env := make([]string, 0, len(envMap))
|
||||
for k, v := range envMap {
|
||||
env = append(env, fmt.Sprintf("%s=%s", k, v))
|
||||
}
|
||||
cmd.Env = env
|
||||
|
||||
transport = &mcp.CommandTransport{Command: cmd}
|
||||
default:
|
||||
|
||||
Reference in New Issue
Block a user