config: remove unused environment variable resolver

Christian Rocha and Charm Crush created 1 week ago

The hand-rolled $VARNAME-only resolver (NewEnvironmentVariableResolver)
had no production callers. Tests have been switched to the shell
resolver, which is what production already uses, and the dead type
plus its tests are gone.

Co-Authored-By: Charm Crush <[e-mail]>

Change summary

internal/config/load_test.go    | 78 +++++++++++++++++-----------------
internal/config/resolve.go      | 27 -----------
internal/config/resolve_test.go | 64 ----------------------------
internal/lsp/client_test.go     |  2 
4 files changed, 41 insertions(+), 130 deletions(-)

Detailed changes

internal/config/load_test.go

@@ -77,7 +77,7 @@ func TestConfig_configureProviders(t *testing.T) {
 	env := env.NewFromMap(map[string]string{
 		"OPENAI_API_KEY": "test-key",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 	require.Equal(t, 1, cfg.Providers.Len())
@@ -120,7 +120,7 @@ func TestConfig_configureProvidersWithOverride(t *testing.T) {
 	env := env.NewFromMap(map[string]string{
 		"OPENAI_API_KEY": "test-key",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 	require.Equal(t, 1, cfg.Providers.Len())
@@ -162,7 +162,7 @@ func TestConfig_configureProvidersWithNewProvider(t *testing.T) {
 	env := env.NewFromMap(map[string]string{
 		"OPENAI_API_KEY": "test-key",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 	// Should be to because of the env variable
@@ -198,7 +198,7 @@ func TestConfig_configureProvidersBedrockWithCredentials(t *testing.T) {
 		"AWS_ACCESS_KEY_ID":     "test-key-id",
 		"AWS_SECRET_ACCESS_KEY": "test-secret-key",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 	require.Equal(t, cfg.Providers.Len(), 1)
@@ -224,7 +224,7 @@ func TestConfig_configureProvidersBedrockWithoutCredentials(t *testing.T) {
 	cfg := &Config{}
 	cfg.setDefaults("/tmp", "")
 	env := env.NewFromMap(map[string]string{})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 	// Provider should not be configured without credentials
@@ -249,7 +249,7 @@ func TestConfig_configureProvidersBedrockWithoutUnsupportedModel(t *testing.T) {
 		"AWS_ACCESS_KEY_ID":     "test-key-id",
 		"AWS_SECRET_ACCESS_KEY": "test-secret-key",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.Error(t, err)
 }
@@ -272,7 +272,7 @@ func TestConfig_configureProvidersVertexAIWithCredentials(t *testing.T) {
 		"VERTEXAI_PROJECT":  "test-project",
 		"VERTEXAI_LOCATION": "us-central1",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 	require.Equal(t, cfg.Providers.Len(), 1)
@@ -304,7 +304,7 @@ func TestConfig_configureProvidersVertexAIWithoutCredentials(t *testing.T) {
 		"GOOGLE_CLOUD_PROJECT":      "test-project",
 		"GOOGLE_CLOUD_LOCATION":     "us-central1",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 	// Provider should not be configured without proper credentials
@@ -329,7 +329,7 @@ func TestConfig_configureProvidersVertexAIMissingProject(t *testing.T) {
 		"GOOGLE_GENAI_USE_VERTEXAI": "true",
 		"GOOGLE_CLOUD_LOCATION":     "us-central1",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 	// Provider should not be configured without project
@@ -353,7 +353,7 @@ func TestConfig_configureProvidersSetProviderID(t *testing.T) {
 	env := env.NewFromMap(map[string]string{
 		"OPENAI_API_KEY": "test-key",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 	require.Equal(t, cfg.Providers.Len(), 1)
@@ -544,7 +544,7 @@ func TestConfig_configureProvidersWithDisabledProvider(t *testing.T) {
 	env := env.NewFromMap(map[string]string{
 		"OPENAI_API_KEY": "test-key",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 
@@ -572,7 +572,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
 		cfg.setDefaults("/tmp", "")
 
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
 		require.NoError(t, err)
 
@@ -595,7 +595,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
 		cfg.setDefaults("/tmp", "")
 
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
 		require.NoError(t, err)
 
@@ -617,7 +617,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
 		cfg.setDefaults("/tmp", "")
 
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
 		require.NoError(t, err)
 
@@ -642,7 +642,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
 		cfg.setDefaults("/tmp", "")
 
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
 		require.NoError(t, err)
 
@@ -667,7 +667,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
 		cfg.setDefaults("/tmp", "")
 
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
 		require.NoError(t, err)
 
@@ -695,7 +695,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
 		cfg.setDefaults("/tmp", "")
 
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
 		require.NoError(t, err)
 
@@ -725,7 +725,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
 		cfg.setDefaults("/tmp", "")
 
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
 		require.NoError(t, err)
 
@@ -760,7 +760,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {
 		env := env.NewFromMap(map[string]string{
 			"GOOGLE_GENAI_USE_VERTEXAI": "false",
 		})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -791,7 +791,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {
 		cfg.setDefaults("/tmp", "")
 
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -822,7 +822,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {
 		cfg.setDefaults("/tmp", "")
 
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -855,7 +855,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {
 		env := env.NewFromMap(map[string]string{
 			"OPENAI_API_KEY": "test-key",
 		})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -889,7 +889,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
 		cfg := &Config{}
 		cfg.setDefaults("/tmp", "")
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -925,7 +925,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
 		cfg := &Config{}
 		cfg.setDefaults("/tmp", "")
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -955,7 +955,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
 		cfg := &Config{}
 		cfg.setDefaults("/tmp", "")
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 		_, _, err = cfg.defaultModelSelection(knownProviders)
@@ -998,7 +998,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
 		}
 		cfg.setDefaults("/tmp", "")
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 		large, small, err := cfg.defaultModelSelection(knownProviders)
@@ -1042,7 +1042,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
 		}
 		cfg.setDefaults("/tmp", "")
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 		_, _, err = cfg.defaultModelSelection(knownProviders)
@@ -1084,7 +1084,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
 		}
 		cfg.setDefaults("/tmp", "")
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 		large, small, err := cfg.defaultModelSelection(knownProviders)
@@ -1129,7 +1129,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {
 		env := env.NewFromMap(map[string]string{
 			"OPENAI_API_KEY": "test-key",
 		})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.ErrorContains(t, err, "no custom providers")
 
@@ -1172,7 +1172,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {
 			"MY_API_KEY":     "test-key",
 			"OPENAI_API_KEY": "test-key",
 		})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -1226,7 +1226,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {
 			"OPENAI_API_KEY":    "test-key",
 			"ANTHROPIC_API_KEY": "test-key",
 		})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -1254,7 +1254,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {
 		cfg.setDefaults("/tmp", "")
 
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
 		require.ErrorContains(t, err, "no custom providers")
 
@@ -1278,7 +1278,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {
 		cfg.setDefaults("/tmp", "")
 
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
 		require.ErrorContains(t, err, "no custom providers")
 
@@ -1336,7 +1336,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {
 		cfg.setDefaults(dir, "")
 		store := &ConfigStore{config: cfg, globalDataPath: globalPath}
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(store, env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -1386,7 +1386,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {
 		}
 		cfg.setDefaults("/tmp", "")
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -1448,7 +1448,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {
 		}
 		cfg.setDefaults("/tmp", "")
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -1493,7 +1493,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {
 		}
 		cfg.setDefaults("/tmp", "")
 		env := env.NewFromMap(map[string]string{})
-		resolver := NewEnvironmentVariableResolver(env)
+		resolver := NewShellVariableResolver(env)
 		err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 		require.NoError(t, err)
 
@@ -1532,7 +1532,7 @@ func TestConfig_configureProviders_HyperAPIKeyFromEnv(t *testing.T) {
 	env := env.NewFromMap(map[string]string{
 		"HYPER_API_KEY": "env-api-key",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 	require.Equal(t, 1, cfg.Providers.Len())
@@ -1579,7 +1579,7 @@ func TestConfig_configureProviders_HyperAPIKeyFromConfigOverrides(t *testing.T)
 	env := env.NewFromMap(map[string]string{
 		"HYPER_API_KEY": "env-api-key",
 	})
-	resolver := NewEnvironmentVariableResolver(env)
+	resolver := NewShellVariableResolver(env)
 	err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
 	require.NoError(t, err)
 	require.Equal(t, 1, cfg.Providers.Len())

internal/config/resolve.go

@@ -10,8 +10,7 @@ import (
 )
 
 // resolveTimeout bounds how long a single ResolveValue call may spend
-// inside shell expansion (including any command substitution). Matches
-// the timeout used by the previous hand-rolled parser.
+// inside shell expansion (including any command substitution).
 const resolveTimeout = 5 * time.Minute
 
 type VariableResolver interface {
@@ -173,27 +172,3 @@ func scrubErrorMessage(s string) string {
 	}
 	return string(out)
 }
-
-type environmentVariableResolver struct {
-	env env.Env
-}
-
-func NewEnvironmentVariableResolver(env env.Env) VariableResolver {
-	return &environmentVariableResolver{
-		env: env,
-	}
-}
-
-// ResolveValue resolves environment variables from the provided env.Env.
-func (r *environmentVariableResolver) ResolveValue(value string) (string, error) {
-	if len(value) == 0 || value[0] != '$' {
-		return value, nil
-	}
-
-	varName := value[1:]
-	resolvedValue := r.env.Get(varName)
-	if resolvedValue == "" {
-		return "", fmt.Errorf("environment variable %q not set", varName)
-	}
-	return resolvedValue, nil
-}

internal/config/resolve_test.go

@@ -220,62 +220,6 @@ func TestScrubErrorMessage(t *testing.T) {
 	})
 }
 
-func TestEnvironmentVariableResolver_ResolveValue(t *testing.T) {
-	tests := []struct {
-		name        string
-		value       string
-		envVars     map[string]string
-		expected    string
-		expectError bool
-	}{
-		{
-			name:     "non-variable string returns as-is",
-			value:    "plain-string",
-			expected: "plain-string",
-		},
-		{
-			name:     "environment variable resolution",
-			value:    "$HOME",
-			envVars:  map[string]string{"HOME": "/home/user"},
-			expected: "/home/user",
-		},
-		{
-			name:     "environment variable with complex value",
-			value:    "$PATH",
-			envVars:  map[string]string{"PATH": "/usr/bin:/bin:/usr/local/bin"},
-			expected: "/usr/bin:/bin:/usr/local/bin",
-		},
-		{
-			name:        "missing environment variable returns error",
-			value:       "$MISSING_VAR",
-			envVars:     map[string]string{},
-			expectError: true,
-		},
-		{
-			name:        "empty environment variable returns error",
-			value:       "$EMPTY_VAR",
-			envVars:     map[string]string{"EMPTY_VAR": ""},
-			expectError: true,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			testEnv := env.NewFromMap(tt.envVars)
-			resolver := NewEnvironmentVariableResolver(testEnv)
-
-			result, err := resolver.ResolveValue(tt.value)
-
-			if tt.expectError {
-				require.Error(t, err)
-			} else {
-				require.NoError(t, err)
-				require.Equal(t, tt.expected, result)
-			}
-		})
-	}
-}
-
 func TestNewShellVariableResolver(t *testing.T) {
 	testEnv := env.NewFromMap(map[string]string{"TEST": "value"})
 	resolver := NewShellVariableResolver(testEnv)
@@ -283,11 +227,3 @@ func TestNewShellVariableResolver(t *testing.T) {
 	require.NotNil(t, resolver)
 	require.Implements(t, (*VariableResolver)(nil), resolver)
 }
-
-func TestNewEnvironmentVariableResolver(t *testing.T) {
-	testEnv := env.NewFromMap(map[string]string{"TEST": "value"})
-	resolver := NewEnvironmentVariableResolver(testEnv)
-
-	require.NotNil(t, resolver)
-	require.Implements(t, (*VariableResolver)(nil), resolver)
-}

internal/lsp/client_test.go

@@ -26,7 +26,7 @@ func TestClient(t *testing.T) {
 
 	// Test creating a powernap client - this will likely fail with echo
 	// but we can still test the basic structure
-	client, err := New(ctx, "test", cfg, config.NewEnvironmentVariableResolver(env.NewFromMap(map[string]string{
+	client, err := New(ctx, "test", cfg, config.NewShellVariableResolver(env.NewFromMap(map[string]string{
 		"THE_CMD": "echo",
 	})), ".", false)
 	if err != nil {