diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 653291539709353480a81011b3012c3f7d36c36a..b6038e0d0e3fb76e4567b4c5c739eb8da952311e 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -1740,3 +1740,124 @@ func TestConfig_configureProviders_EchoEmptyHeaderDropped(t *testing.T) { require.False(t, present, "$(echo) → empty → header must be dropped") require.Equal(t, "present", pc.ExtraHeaders["X-Kept"]) } + +// TestConfig_configureProviders_UnsetAPIKeySkipsProvider pins Phase 2 +// Step 12 / design decision #15: under the lenient-nounset shell +// resolver, $UNSET_API_KEY expands to ("", nil) rather than ("", err), +// and the existing `v == "" || err != nil` skip path at load.go:331 +// still drops the provider. The slog.Warn line is emitted on the same +// path but is not asserted here — internal/config/load_test.go's +// TestMain replaces the default slog handler with an io.Discard +// writer, so capturing that log line would require mid-test handler +// swapping and a sync.Mutex dance that adds more flake surface than +// signal. The observable outcome (provider absent from the map) is +// what downstream code — model picker, agent wiring — actually reads, +// so that's what we pin. +func TestConfig_configureProviders_UnsetAPIKeySkipsProvider(t *testing.T) { + knownProviders := []catwalk.Provider{ + { + ID: "openai", + APIKey: "$SOMETHING_UNSET", + APIEndpoint: "https://api.openai.com/v1", + Models: []catwalk.Model{{ID: "test-model"}}, + }, + } + + // Existing user config for this known provider so the load.go:332 + // `if configExists` branch fires and actually calls Providers.Del. + // Without it the provider was never in the map to begin with and + // the test would pass trivially. + cfg := &Config{ + Providers: csync.NewMapFrom(map[string]ProviderConfig{ + "openai": {BaseURL: "custom-url"}, + }), + } + cfg.setDefaults("/tmp", "") + + testEnv := env.NewFromMap(map[string]string{ + "PATH": os.Getenv("PATH"), + }) + resolver := NewShellVariableResolver(testEnv) + + err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders) + require.NoError(t, err, "skip path must not surface as a load error") + + require.Equal(t, 0, cfg.Providers.Len(), "provider with unset API key must be skipped") + _, exists := cfg.Providers.Get("openai") + require.False(t, exists) +} + +// TestConfig_configureProviders_FailingAPIKeyCmdSkipsProvider pins +// that the two failure modes for APIKey — ("", nil) from an unset var +// under lenient nounset and ("", err) from a failing $(cmd) — are +// equivalent for the skip outcome at load.go:331. The `v == "" || +// err != nil` check fires on either branch; this test locks in that +// equivalence so a future refactor that splits the check into two +// paths doesn't accidentally start propagating $(false) as a load +// error while keeping unset-var as a silent skip (or vice versa). +func TestConfig_configureProviders_FailingAPIKeyCmdSkipsProvider(t *testing.T) { + knownProviders := []catwalk.Provider{ + { + ID: "openai", + APIKey: "$(false)", + APIEndpoint: "https://api.openai.com/v1", + Models: []catwalk.Model{{ID: "test-model"}}, + }, + } + + cfg := &Config{ + Providers: csync.NewMapFrom(map[string]ProviderConfig{ + "openai": {BaseURL: "custom-url"}, + }), + } + cfg.setDefaults("/tmp", "") + + testEnv := env.NewFromMap(map[string]string{ + "PATH": os.Getenv("PATH"), + }) + resolver := NewShellVariableResolver(testEnv) + + err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders) + require.NoError(t, err, "failing $(cmd) in API key must skip provider, not fail load") + + require.Equal(t, 0, cfg.Providers.Len(), "provider with failing $(cmd) API key must be skipped") + _, exists := cfg.Providers.Get("openai") + require.False(t, exists) +} + +// TestConfig_configureProviders_UnsetAzureEndpointSkipsProvider pins +// the same contract on the Azure path at load.go:287 — APIEndpoint is +// the field that gates Azure and goes through the same +// `v == "" || err != nil` skip check. Covered here so both branches +// of the shared skip pattern (APIKey default path and APIEndpoint +// Azure path) are tested; a future refactor that unifies them can +// rely on these two tests to catch drift. +func TestConfig_configureProviders_UnsetAzureEndpointSkipsProvider(t *testing.T) { + knownProviders := []catwalk.Provider{ + { + ID: catwalk.InferenceProviderAzure, + APIKey: "test-key", + APIEndpoint: "$UNSET_AZURE_ENDPOINT", + Models: []catwalk.Model{{ID: "test-model"}}, + }, + } + + cfg := &Config{ + Providers: csync.NewMapFrom(map[string]ProviderConfig{ + "azure": {BaseURL: ""}, + }), + } + cfg.setDefaults("/tmp", "") + + testEnv := env.NewFromMap(map[string]string{ + "PATH": os.Getenv("PATH"), + }) + resolver := NewShellVariableResolver(testEnv) + + err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders) + require.NoError(t, err) + + require.Equal(t, 0, cfg.Providers.Len(), "azure provider with unset endpoint must be skipped") + _, exists := cfg.Providers.Get("azure") + require.False(t, exists) +}