From 757741476181b6ed2b39cf1626c472eb069fe631 Mon Sep 17 00:00:00 2001
From: yuchou87 <yuchou87@gmail.com>
Date: Thu, 19 Feb 2026 19:45:15 +0800
Subject: [PATCH] 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.
---
 pkg/mcp/manager.go | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go
index 833755cbd..923d01f3c 100644
--- a/pkg/mcp/manager.go
+++ b/pkg/mcp/manager.go
@@ -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: