changes in env vars

Author: taskbot

cmd/thv-proxyrunner/app/run.go

@@ -295,17 +295,11 @@ func runCmdFunc(cmd *cobra.Command, args []string) error {
 
 	var imageMetadata *registry.ImageMetadata
 
-	// Parse environment variables from slice to map only if env flag was set
-	var envVarsMap map[string]string
-	if cmd.Flags().Changed("env") {
-		var err error
-		envVarsMap, err = environment.ParseEnvironmentVariables(runFlags.runEnv)
-		if err != nil {
-			return fmt.Errorf("failed to parse environment variables: %v", err)
-		}
-	} else {
-		// Use empty map if no env flags were set
-		envVarsMap = make(map[string]string)
+	// Parse environment variables from CLI flags (always parse, even if empty)
+	// This ensures CLI env vars can override file-based env vars if provided
+	envVarsMap, err := environment.ParseEnvironmentVariables(runFlags.runEnv)
+	if err != nil {
+		return fmt.Errorf("failed to parse environment variables: %v", err)
 	}
 
 	// Start with basic options

cmd/thv-proxyrunner/app/run_test.go

@@ -8,7 +8,9 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/stacklok/toolhive/pkg/environment"
 	"github.com/stacklok/toolhive/pkg/runner"
+	"github.com/stacklok/toolhive/pkg/transport/types"
 )
 
 func TestRunCmdFlagParsing(t *testing.T) {
@@ -353,3 +355,137 @@ func TestTryLoadConfigFromFile(t *testing.T) {
 		assert.Contains(t, err.Error(), "failed to parse JSON")
 	})
 }
+
+func TestRunCmdEnvironmentVariableOverrideBehavior(t *testing.T) {
+	t.Parallel()
+
+	t.Run("no CLI env vars preserves file-based env vars", func(t *testing.T) {
+		t.Parallel()
+
+		// Create a config file with environment variables
+		tmpDir := t.TempDir()
+		configPath := tmpDir + "/runconfig.json"
+
+		configContent := `{
+			"schema_version": "v1",
+			"name": "env-test-server",
+			"image": "test:env",
+			"transport": "stdio",
+			"env_vars": {
+				"FILE_VAR": "file_value",
+				"COMMON_VAR": "from_file"
+			}
+		}`
+
+		err := os.WriteFile(configPath, []byte(configContent), 0644)
+		require.NoError(t, err, "Should be able to create config file")
+
+		// Test function that loads config from our specific path
+		testLoadConfigFromPath := func(path string) (*runner.RunConfig, error) {
+			if _, err := os.Stat(path); err != nil {
+				return nil, nil
+			}
+
+			file, err := os.Open(path) // #nosec G304 - test path
+			if err != nil {
+				return nil, fmt.Errorf("found config file at %s but failed to open: %w", path, err)
+			}
+			defer file.Close()
+
+			return runner.ReadJSON(file)
+		}
+
+		config, err := testLoadConfigFromPath(configPath)
+		require.NoError(t, err, "Should successfully load config from file")
+		require.NotNil(t, config, "Should return config when file exists")
+
+		// Verify file-based environment variables are present
+		assert.Equal(t, "file_value", config.EnvVars["FILE_VAR"])
+		assert.Equal(t, "from_file", config.EnvVars["COMMON_VAR"])
+
+		// Create a mock command with no --env flags set
+		cmd := NewRunCmd()
+		require.NotNil(t, cmd)
+		addRunFlags(cmd, &proxyRunFlags{})
+
+		// Parse empty args (no --env flags)
+		err = cmd.ParseFlags([]string{"--transport", "stdio", "--name", "test-server"})
+		require.NoError(t, err)
+
+		// Verify that env flags were not changed
+		assert.False(t, cmd.Flags().Changed("env"), "env flag should not be changed")
+	})
+
+	t.Run("CLI env vars override file-based env vars", func(t *testing.T) {
+		t.Parallel()
+
+		// Create a config file with environment variables
+		tmpDir := t.TempDir()
+		configPath := tmpDir + "/runconfig.json"
+
+		configContent := `{
+			"schema_version": "v1",
+			"name": "env-test-server",
+			"image": "test:env",
+			"transport": "stdio",
+			"env_vars": {
+				"FILE_VAR": "file_value",
+				"COMMON_VAR": "from_file"
+			}
+		}`
+
+		err := os.WriteFile(configPath, []byte(configContent), 0644)
+		require.NoError(t, err, "Should be able to create config file")
+
+		// Create a mock command with --env flags set
+		cmd := NewRunCmd()
+		require.NotNil(t, cmd)
+		flags := &proxyRunFlags{}
+		addRunFlags(cmd, flags)
+
+		// Parse args with --env flags that should override file-based env vars
+		err = cmd.ParseFlags([]string{
+			"--transport", "stdio",
+			"--name", "test-server",
+			"--env", "CLI_VAR=cli_value",
+			"--env", "COMMON_VAR=from_cli",
+		})
+		require.NoError(t, err)
+
+		// Verify that env flag was changed and values are correct
+		assert.True(t, cmd.Flags().Changed("env"), "env flag should be changed")
+		assert.Contains(t, flags.runEnv, "CLI_VAR=cli_value")
+		assert.Contains(t, flags.runEnv, "COMMON_VAR=from_cli")
+	})
+
+	t.Run("empty CLI env vars do not interfere with file-based env vars", func(t *testing.T) {
+		t.Parallel()
+
+		// Test that ParseEnvironmentVariables with empty slice returns empty map
+		envVarsMap, err := environment.ParseEnvironmentVariables([]string{})
+		require.NoError(t, err, "ParseEnvironmentVariables should handle empty slice")
+		assert.Equal(t, map[string]string{}, envVarsMap, "Empty slice should return empty map")
+		assert.Len(t, envVarsMap, 0, "Empty map should have zero length")
+
+		// Create a RunConfig with some existing env vars (simulating file-based config)
+		config := &runner.RunConfig{
+			Transport:  types.TransportTypeStdio,
+			TargetPort: 8080,
+			EnvVars: map[string]string{
+				"FILE_VAR":   "file_value",
+				"COMMON_VAR": "from_file",
+			},
+		}
+
+		// Apply empty environment variables (simulating no CLI --env flags)
+		resultConfig, err := config.WithEnvironmentVariables(envVarsMap)
+		require.NoError(t, err, "WithEnvironmentVariables should handle empty map")
+
+		// Verify that original env vars are preserved
+		assert.Equal(t, "file_value", resultConfig.EnvVars["FILE_VAR"], "File-based env var should be preserved")
+		assert.Equal(t, "from_file", resultConfig.EnvVars["COMMON_VAR"], "File-based env var should be preserved")
+
+		// Verify transport-specific env vars are also set (these are always added)
+		assert.Equal(t, "stdio", resultConfig.EnvVars["MCP_TRANSPORT"], "Transport env var should be set")
+	})
+}