diff --git a/internal/cli/app.go b/internal/cli/app.go index 24002236a..69cc6fc4e 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -58,7 +58,11 @@ type appDeps struct { // config.SetActiveProviderEnv, set in defaultAppDeps — deliberately NOT filled // by fillAppDeps, so tests never mutate the process environment unless they // inject it). nil ⇒ no export. - exportActiveProvider func(providerName string) + exportActiveProvider func(providerName string) + // getenv reads a process environment variable (production: os.Getenv, set in + // defaultAppDeps — deliberately NOT filled by fillAppDeps, so tests are hermetic + // against ambient vars like ZERO_PROVIDER unless they inject it). nil ⇒ empty. + getenv func(string) string probeProviderHealth func(context.Context, providerhealth.Options) providerhealth.Result discoverProviderModels func(context.Context, config.ProviderProfile) ([]providermodeldiscovery.Model, error) detectLocalRuntimes func(context.Context, provideronboarding.LocalDetectOptions) []provideronboarding.DetectedLocalRuntime @@ -118,6 +122,7 @@ func defaultAppDeps() appDeps { stdin: os.Stdin, userConfigPath: config.DefaultUserConfigPath, exportActiveProvider: config.SetActiveProviderEnv, + getenv: os.Getenv, resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { options, err := config.DefaultResolveOptions(workspaceRoot) if err != nil { diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index b581cc8cd..08650c4a8 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -74,11 +74,18 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app return writeAppError(stderr, err.Error(), exitCrash) } + override := activeProviderEnvOverride(deps.getenv, cfg.ActiveProvider) if options.json { - if err := writePrettyJSON(stdout, map[string]any{ + payload := map[string]any{ "activeProvider": cfg.ActiveProvider, "configPath": configPath, - }); err != nil { + } + if override != "" { + // A JSON consumer must not read this as an effective switch either. + payload["effectiveProvider"] = override + payload["overriddenByEnv"] = config.ActiveProviderEnv + } + if err := writePrettyJSON(stdout, payload); err != nil { return exitCrash } return exitSuccess @@ -86,9 +93,32 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app if _, err := fmt.Fprintf(stdout, "Active provider set to %s\nnext: %s\n", cfg.ActiveProvider, providerCheckCommand(cfg.ActiveProvider, false)); err != nil { return exitCrash } + if override != "" { + if _, err := fmt.Fprintf(stderr, "Note: %s=%s is set and overrides config.json, so %s stays the active provider until you unset %s.\n", config.ActiveProviderEnv, override, override, config.ActiveProviderEnv); err != nil { + return exitCrash + } + } return exitSuccess } +// activeProviderEnvOverride returns the ZERO_PROVIDER value when it is set and +// names a DIFFERENT provider than the one just selected, meaning the saved +// `providers use` selection will NOT be the effective active provider until the +// env var is unset. applyEnv (resolver.go) makes ZERO_PROVIDER win over +// config.json unconditionally, so reporting the write as a plain success reads as +// a switch that silently has no effect (issue #721). Empty when nothing overrides +// (including when getenv is nil, e.g. a test that did not inject the environment). +func activeProviderEnvOverride(getenv func(string) string, selected string) string { + if getenv == nil { + return "" + } + override := strings.TrimSpace(getenv(config.ActiveProviderEnv)) + if override == "" || strings.EqualFold(override, strings.TrimSpace(selected)) { + return "" + } + return override +} + func runProvidersSetup(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { options, help, err := parseProviderSetupArgs(args) if err != nil { diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index d354ced85..6118939d8 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -75,6 +75,102 @@ func TestRunProvidersUseJSONIncludesActiveProviderAndConfigPath(t *testing.T) { } } +func providersUseOverrideConfig(t *testing.T) string { + t.Helper() + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{ + ActiveProvider: "work", + Providers: []config.ProviderProfile{ + {Name: "work", ProviderKind: config.ProviderKindOpenAI, BaseURL: config.OpenAIBaseURL, Model: "gpt-4.1"}, + {Name: "fast", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://api.groq.com/openai/v1", Model: "llama-3.3-70b-versatile"}, + }, + }) + return configPath +} + +// The write to config.json still succeeds, but when ZERO_PROVIDER names a +// different provider the saved selection is NOT effective, so the command must +// warn instead of reporting a silent success (issue #721). +func TestRunProvidersUseWarnsWhenEnvOverrides(t *testing.T) { + var stdout, stderr bytes.Buffer + configPath := providersUseOverrideConfig(t) + deps := providerSetupDeps(configPath) + deps.getenv = func(key string) string { + if key == config.ActiveProviderEnv { + return "work" // ZERO_PROVIDER=work overrides the switch to fast + } + return "" + } + + if code := runWithDeps([]string{"providers", "use", "fast"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + if cfg := readFileConfig(t, configPath); cfg.ActiveProvider != "fast" { + t.Fatalf("ActiveProvider = %q, want fast (the write still lands)", cfg.ActiveProvider) + } + if !strings.Contains(stdout.String(), "Active provider set to fast") { + t.Fatalf("stdout missing the success line: %q", stdout.String()) + } + note := stderr.String() + for _, want := range []string{config.ActiveProviderEnv, "overrides config.json", "work"} { + if !strings.Contains(note, want) { + t.Fatalf("override note missing %q, got %q", want, note) + } + } +} + +func TestRunProvidersUseJSONFlagsEnvOverride(t *testing.T) { + var stdout, stderr bytes.Buffer + deps := providerSetupDeps(providersUseOverrideConfig(t)) + deps.getenv = func(key string) string { + if key == config.ActiveProviderEnv { + return "work" + } + return "" + } + + if code := runWithDeps([]string{"providers", "use", "fast", "--json"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + var payload struct { + ActiveProvider string `json:"activeProvider"` + EffectiveProvider string `json:"effectiveProvider"` + OverriddenByEnv string `json:"overriddenByEnv"` + } + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatalf("JSON did not decode: %v\n%s", err, stdout.String()) + } + if payload.ActiveProvider != "fast" || payload.EffectiveProvider != "work" || payload.OverriddenByEnv != config.ActiveProviderEnv { + t.Fatalf("JSON must flag the env override, got %#v", payload) + } +} + +// No override note when ZERO_PROVIDER is unset or already names the selection. +func TestRunProvidersUseNoWarnWithoutEnvOverride(t *testing.T) { + cases := map[string]func(string) string{ + "env unset": func(string) string { return "" }, + "env matches fast": func(key string) string { + if key == config.ActiveProviderEnv { + return "fast" + } + return "" + }, + } + for name, getenv := range cases { + t.Run(name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + deps := providerSetupDeps(providersUseOverrideConfig(t)) + deps.getenv = getenv + if code := runWithDeps([]string{"providers", "use", "fast"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("exit = %d: %s", code, stderr.String()) + } + if stderr.Len() != 0 { + t.Fatalf("expected no override note, got %q", stderr.String()) + } + }) + } +} + func TestRunProvidersUseSurfacesMalformedConfig(t *testing.T) { var stdout, stderr bytes.Buffer configPath := filepath.Join(t.TempDir(), "config.json")