Implement EnvironmentManager for safe environment variable and PATH management

cmd/env.go

@@ -104,42 +104,16 @@ func outputEnvironment() error {
 		return fmt.Errorf("failed to create tool manager: %w", err)
 	}
 
-	// Get environment variables
+	// Get environment variables (includes PATH with tool directories)
 	env, err := manager.SetupEnvironment(cfg)
 	if err != nil {
 		return fmt.Errorf("failed to setup environment: %w", err)
 	}
 
-	// Build PATH from tool bin directories
+	// Extract PATH from environment
 	pathDirs := []string{}
-
-	for toolName, toolConfig := range cfg.Tools {
-		// Check if user wants to use system tool instead
-		systemEnvVar := fmt.Sprintf("MVX_USE_SYSTEM_%s", strings.ToUpper(toolName))
-		if os.Getenv(systemEnvVar) == "true" {
-			continue
-		}
-
-		tool, err := manager.GetTool(toolName)
-		if err != nil {
-			continue
-		}
-
-		// Resolve version
-		resolvedVersion, err := manager.ResolveVersion(toolName, toolConfig)
-		if err != nil {
-			continue
-		}
-
-		// Create resolved config
-		resolvedConfig := toolConfig
-		resolvedConfig.Version = resolvedVersion
-
-		// Try to get tool path
-		binPath, err := tool.GetPath(resolvedVersion, resolvedConfig)
-		if err == nil {
-			pathDirs = append(pathDirs, binPath)
-		}
+	if pathValue, exists := env["PATH"]; exists && pathValue != "" {
+		pathDirs = strings.Split(pathValue, string(os.PathListSeparator))
 	}
 
 	// Output environment in shell-specific format

cmd/root.go

@@ -214,73 +214,20 @@ func autoSetupEnvironment() error {
 
 // setupGlobalEnvironment sets up PATH and environment variables globally
 func setupGlobalEnvironment(cfg *config.Config, manager *tools.Manager) error {
-	// Get environment variables from tool manager
+	// Get environment variables from tool manager (includes PATH with tool directories)
 	env, err := manager.SetupEnvironment(cfg)
 	if err != nil {
 		return fmt.Errorf("failed to get tool environment: %w", err)
 	}
 
-	// Set up PATH with tool bin directories
-	pathDirs := []string{}
-
-	// Add all configured tools to PATH (respecting system tool bypass)
-	for toolName, toolConfig := range cfg.Tools {
-		// Check if user wants to use system tool instead
-		systemEnvVar := fmt.Sprintf("MVX_USE_SYSTEM_%s", strings.ToUpper(toolName))
-		if os.Getenv(systemEnvVar) == "true" {
-			printVerbose("Skipping %s PATH setup: %s=true (using system tool)", toolName, systemEnvVar)
-			continue
-		}
-
-		tool, err := manager.GetTool(toolName)
-		if err != nil {
-			printVerbose("Skipping tool %s: %v", toolName, err)
-			continue
-		}
-
-		// Resolve version
-		resolvedVersion, err := manager.ResolveVersion(toolName, toolConfig)
-		if err != nil {
-			printVerbose("Skipping tool %s: version resolution failed: %v", toolName, err)
-			continue
-		}
-
-		// Create resolved config
-		resolvedConfig := toolConfig
-		resolvedConfig.Version = resolvedVersion
-
-		// Try to get tool path - if successful, tool is installed and we can add it to PATH
-		binPath, err := tool.GetPath(resolvedVersion, resolvedConfig)
-		if err != nil {
-			printVerbose("Tool %s version %s is not installed or path unavailable, skipping PATH setup: %v", toolName, resolvedVersion, err)
-		} else {
-			printVerbose("Adding %s bin path to global PATH: %s", toolName, binPath)
-			pathDirs = append(pathDirs, binPath)
-		}
-	}
-
-	// Update PATH environment variable
-	if len(pathDirs) > 0 {
-		currentPath := os.Getenv("PATH")
-		newPath := strings.Join(pathDirs, string(os.PathListSeparator))
-		if currentPath != "" {
-			newPath = newPath + string(os.PathListSeparator) + currentPath
-		}
-
-		// Set PATH globally for this process and all child processes
-		if err := os.Setenv("PATH", newPath); err != nil {
-			return fmt.Errorf("failed to set PATH: %w", err)
-		}
-
-		printVerbose("Updated global PATH with %d tool directories", len(pathDirs))
-		printVerbose("New PATH: %s", newPath)
-	}
-
-	// Set other tool-specific environment variables globally
+	// Set all environment variables globally
 	for key, value := range env {
-		if key != "PATH" { // PATH is handled above
-			if err := os.Setenv(key, value); err != nil {
-				printVerbose("Failed to set environment variable %s: %v", key, err)
+		if err := os.Setenv(key, value); err != nil {
+			printVerbose("Failed to set environment variable %s: %v", key, err)
+		} else {
+			if key == "PATH" {
+				printVerbose("Set global PATH with tool directories")
+				printVerbose("New PATH: %s", value)
 			} else {
 				printVerbose("Set global environment variable: %s=%s", key, value)
 			}

pkg/executor/executor.go

@@ -125,34 +125,53 @@ func (e *Executor) GetCommandInfo(commandName string) (*config.CommandConfig, er
 
 // setupEnvironment prepares the environment for command execution
 func (e *Executor) setupEnvironment(cmdConfig config.CommandConfig) ([]string, error) {
-	// Create environment map starting with current environment
-	envVars := make(map[string]string)
+	// Create environment manager starting with current environment
+	envManager := tools.NewEnvironmentManager()
 	for _, envVar := range os.Environ() {
 		parts := strings.SplitN(envVar, "=", 2)
 		if len(parts) == 2 {
-			envVars[parts[0]] = parts[1]
+			if parts[0] == "PATH" {
+				// Parse PATH into directories
+				if parts[1] != "" {
+					for _, dir := range strings.Split(parts[1], string(os.PathListSeparator)) {
+						envManager.AddToPath(dir)
+					}
+				}
+			} else {
+				envManager.SetEnv(parts[0], parts[1])
+			}
 		}
 	}
 
-	// Add global environment variables from config (these override system ones)
+	// Add global environment variables from config (includes tool paths and environment)
 	globalEnv, err := e.toolManager.SetupEnvironment(e.config)
 	if err != nil {
 		return nil, err
 	}
 
+	// Merge global environment into our environment manager
 	for key, value := range globalEnv {
-		envVars[key] = value
+		if key == "PATH" {
+			// The global environment's PATH includes tool-specific modifications
+			// Add these directories to our existing PATH
+			if value != "" {
+				for _, dir := range strings.Split(value, string(os.PathListSeparator)) {
+					if dir != "" {
+						envManager.AddToPath(dir)
+					}
+				}
+			}
+		} else {
+			envManager.SetEnv(key, value)
+		}
 	}
 
 	// Add command-specific environment variables (these override global ones)
 	for key, value := range cmdConfig.Environment {
-		envVars[key] = value
+		envManager.SetEnv(key, value)
 	}
 
-	// Add tool paths to PATH
-	pathDirs := []string{}
-
-	// Get required tools for this command
+	// Ensure required tools are installed (auto-install if needed)
 	requiredTools := cmdConfig.Requires
 	if len(requiredTools) == 0 {
 		// If no specific requirements, use all configured tools
@@ -162,41 +181,20 @@ func (e *Executor) setupEnvironment(cmdConfig config.CommandConfig) ([]string, e
 	}
 	util.LogVerbose("Required tools for command: %v", requiredTools)
 
-	// Add tool bin directories to PATH
+	// Ensure all required tools are installed (this may trigger auto-installation)
 	for _, toolName := range requiredTools {
 		if toolConfig, exists := e.config.Tools[toolName]; exists {
-			// EnsureTool handles version resolution, installation check, auto-install, and path retrieval
-			binPath, err := e.toolManager.EnsureTool(toolName, toolConfig)
+			// EnsureTool handles version resolution, installation check, and auto-install
+			_, err := e.toolManager.EnsureTool(toolName, toolConfig)
 			if err != nil {
-				util.LogVerbose("Skipping tool %s: %v", toolName, err)
-				continue
+				util.LogVerbose("Failed to ensure tool %s: %v", toolName, err)
+				// Continue anyway - the tool might still work if it's a system tool
 			}
-
-			util.LogVerbose("Adding %s bin path to PATH: %s", toolName, binPath)
-			pathDirs = append(pathDirs, binPath)
 		}
 	}
 
-	// Prepend tool paths to existing PATH
-	if len(pathDirs) > 0 {
-		currentPath := envVars["PATH"]
-		newPath := strings.Join(pathDirs, string(os.PathListSeparator))
-		if currentPath != "" {
-			newPath = newPath + string(os.PathListSeparator) + currentPath
-		}
-		envVars["PATH"] = newPath
-		util.LogVerbose("Updated PATH with %d tool directories: %s", len(pathDirs), newPath)
-	} else {
-		util.LogVerbose("No tool directories added to PATH")
-	}
-
-	// Convert environment map back to slice format
-	var env []string
-	for key, value := range envVars {
-		env = append(env, fmt.Sprintf("%s=%s", key, value))
-	}
-
-	return env, nil
+	// Convert environment manager to slice format
+	return envManager.ToSlice(), nil
 }
 
 // processScriptString processes a script string with arguments

pkg/tools/constants.go

@@ -57,6 +57,7 @@ const (
 	EnvMvndHome  = "MVND_HOME"
 	EnvNodeHome  = "NODE_HOME"
 	EnvGoRoot    = "GOROOT"
+	EnvGoPath    = "GOPATH"
 )
 
 // File Extensions

pkg/tools/environment.go

@@ -0,0 +1,143 @@
+package tools
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+
+	"github.com/gnodet/mvx/pkg/util"
+)
+
+// EnvironmentManager provides safe environment variable management with special PATH handling
+type EnvironmentManager struct {
+	envVars  map[string]string
+	pathDirs []string
+}
+
+// NewEnvironmentManager creates a new environment manager
+func NewEnvironmentManager() *EnvironmentManager {
+	return &EnvironmentManager{
+		envVars:  make(map[string]string),
+		pathDirs: []string{},
+	}
+}
+
+// NewEnvironmentManagerFromMap creates a new environment manager from an existing map
+func NewEnvironmentManagerFromMap(envVars map[string]string) *EnvironmentManager {
+	em := NewEnvironmentManager()
+
+	for key, value := range envVars {
+		if key == "PATH" {
+			// Parse PATH into directories
+			if value != "" {
+				em.pathDirs = strings.Split(value, string(os.PathListSeparator))
+			}
+		} else {
+			em.envVars[key] = value
+		}
+	}
+
+	return em
+}
+
+// SetEnv sets an environment variable (panics if key is "PATH")
+func (em *EnvironmentManager) SetEnv(key, value string) {
+	if key == "PATH" {
+		panic("Cannot set PATH directly, use AddToPath() or AppendToPath() instead")
+	}
+	em.envVars[key] = value
+	util.LogVerbose("Set environment variable %s=%s", key, value)
+}
+
+// GetEnv gets an environment variable
+func (em *EnvironmentManager) GetEnv(key string) (string, bool) {
+	if key == "PATH" {
+		return em.GetPath(), true
+	}
+	value, exists := em.envVars[key]
+	return value, exists
+}
+
+// AddToPath prepends a directory to PATH if not already present
+func (em *EnvironmentManager) AddToPath(dir string) {
+	if dir == "" {
+		return
+	}
+
+	// Clean the path
+	dir = filepath.Clean(dir)
+
+	// Check if already present
+	for _, existing := range em.pathDirs {
+		if existing == dir {
+			util.LogVerbose("Directory %s already in PATH", dir)
+			return
+		}
+	}
+
+	// Prepend to PATH
+	em.pathDirs = append([]string{dir}, em.pathDirs...)
+	util.LogVerbose("Added directory to PATH: %s", dir)
+}
+
+// AppendToPath appends a directory to PATH if not already present
+func (em *EnvironmentManager) AppendToPath(dir string) {
+	if dir == "" {
+		return
+	}
+
+	// Clean the path
+	dir = filepath.Clean(dir)
+
+	// Check if already present
+	for _, existing := range em.pathDirs {
+		if existing == dir {
+			util.LogVerbose("Directory %s already in PATH", dir)
+			return
+		}
+	}
+
+	// Append to PATH
+	em.pathDirs = append(em.pathDirs, dir)
+	util.LogVerbose("Appended directory to PATH: %s", dir)
+}
+
+// GetPath returns the constructed PATH string
+func (em *EnvironmentManager) GetPath() string {
+	return strings.Join(em.pathDirs, string(os.PathListSeparator))
+}
+
+// ToMap converts the environment manager to a map[string]string
+func (em *EnvironmentManager) ToMap() map[string]string {
+	result := make(map[string]string)
+
+	// Copy all environment variables
+	for key, value := range em.envVars {
+		result[key] = value
+	}
+
+	// Add PATH
+	if len(em.pathDirs) > 0 {
+		result["PATH"] = em.GetPath()
+	}
+
+	return result
+}
+
+// ToSlice converts the environment manager to []string in "KEY=VALUE" format
+func (em *EnvironmentManager) ToSlice() []string {
+	var result []string
+
+	// Add all environment variables
+	for key, value := range em.envVars {
+		result = append(result, fmt.Sprintf("%s=%s", key, value))
+	}
+
+	// Add PATH
+	if len(em.pathDirs) > 0 {
+		result = append(result, fmt.Sprintf("PATH=%s", em.GetPath()))
+	}
+
+	return result
+}