From f9134777c941001444eb57ecc81bfcb49c6366f5 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH] config: remove unused environment variable resolver 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 --- 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(-) diff --git a/internal/config/load_test.go b/internal/config/load_test.go index b6038e0d0e3fb76e4567b4c5c739eb8da952311e..61054e5ccbc9031702f3d1d778634bf53b625bcb 100644 --- a/internal/config/load_test.go +++ b/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()) diff --git a/internal/config/resolve.go b/internal/config/resolve.go index 8e13e16d440a31b73672ab55a467cc7ff866a1b8..a4a359be2e4ce1f6be32eba99b1b68eb3f61fe25 100644 --- a/internal/config/resolve.go +++ b/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 -} diff --git a/internal/config/resolve_test.go b/internal/config/resolve_test.go index 18b461b8a6bc88f73d6db935eebd302cc3514781..3d364e523ae4738d23888941bc429d2c00241fa2 100644 --- a/internal/config/resolve_test.go +++ b/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) -} diff --git a/internal/lsp/client_test.go b/internal/lsp/client_test.go index 0337dc8b9ba506264e519e1c53a725ab9fec35b1..583f7a8962600d711ab5087a7edf99d22eedba6b 100644 --- a/internal/lsp/client_test.go +++ b/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 {