Clarify saved and effective provider selection

[PierrunoYT]

Summary

• report when ZERO_PROVIDER overrides a provider saved by providers use, including resolved, deferred, unresolvable, and unrelated config-error outcomes
• expose whether provider-list entries are selectable from user config or come from the fully resolved runtime configuration
• use exact persisted names for config mutations while using the credential store's normalized identity consistently for collision, cleanup, and session decisions
• centralize OAuth/API-key credential candidates across status, refresh, logout, CLI removal, and TUI key cleanup without deleting shared catalog credentials
• require positive catalogId ownership for catalog setup/login/persistence and reject ambiguous shared catalog IDs instead of adopting by name or file order
• keep TUI live provider state and ZERO_PROVIDER synchronized for case-only renames without retargeting case-variant project providers

Validation

• make fmt-check
• go vet ./...
• go test ./...
• go run ./cmd/zero-release build
• go run ./cmd/zero-release smoke
• make lint-static (0 issues)
• make vulncheck (no vulnerabilities found)
• git diff HEAD --check

Fixes #721

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Provider listings now distinguish persisted and runtime-resolved profiles. Provider selection reports environment override resolution, while configuration mutations require exact persisted names and reject ambiguous duplicates. Authentication, setup, and TUI flows preflight configuration before persistence.

Changes

Provider configuration and selection

Layer / File(s)	Summary
Provider identity validation and mutation internal/config/writer.go, internal/config/credentials.go, internal/config/*_test.go	Persisted provider names are validated for case-insensitive duplicates, while mutation targets use trimmed exact matching.
Active provider resolution internal/config/resolver.go, internal/config/resolver_test.go	Active-provider resolution prefers exact matches, supports unique case-insensitive fallback, and rejects ambiguity.

Provider CLI behavior

Layer / File(s)	Summary
Provider listing metadata internal/cli/command_center.go, internal/cli/command_center_test.go	Provider listings expose selectable and source metadata in JSON and text output.
Provider selection override handling internal/cli/provider_onboarding.go, internal/cli/provider_onboarding_test.go, internal/config/paths.go	providers use reports resolved, unresolved, and deferred environment overrides and rejects non-selectable or mismatched identities.

Authentication and persistence preflight

Layer / File(s)	Summary
OAuth save validation internal/cli/app.go, internal/cli/auth.go, internal/cli/auth_test.go, internal/oauth/manager.go	OAuth flows support before-save configuration validation, preventing token writes when configuration becomes invalid.
Provider write and TUI flow preflights internal/cli/provider_setup.go, internal/cli/setup.go, internal/tui/*	Provider setup, wizard, and device-login flows pass config paths and preflight configuration before persistence.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ProvidersUse
  participant UserConfig
  participant Resolver
  ProvidersUse->>UserConfig: save requested provider
  ProvidersUse->>Resolver: resolve environment override
  Resolver-->>ProvidersUse: return resolved, unresolved, or deferred state
  ProvidersUse-->>ProvidersUse: report effectiveProvider and envProviderResolves

Loading

Possibly related PRs

• Gitlawb/zero#560: Overlaps provider listing rendering changes in internal/cli/command_center.go.

Suggested reviewers: gnanam1990, vasanthdev2004

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 39.13% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR adds source/selectability metadata, visual non-selectable markers, and better handling of override and identity cases required by #721.
Out of Scope Changes check	✅ Passed	The broader auth, setup, config, and TUI edits support the same provider-identity and selection behavior, so no clear unrelated scope stands out.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: distinguishing saved and effective provider selection.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

internal/cli/command_center_test.go (1)

194-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reset stderr alongside stdout for clean failure messages.

While this test passes correctly, it is a good practice to reset both output buffers before reusing them. This ensures that if the second command fails, stderr.String() only contains the relevant error from the second execution, preventing confusing t.Fatalf error messages containing leftover artifacts.

♻️ Proposed refactor

 	}
 
 	stdout.Reset()
+	stderr.Reset()
 	if code := runWithDeps([]string{"providers", "list"}, &stdout, &stderr, deps); code != exitSuccess {
 		t.Fatalf("code=%d stderr=%s", code, stderr.String())

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/command_center_test.go` around lines 194 - 229, Reset stderr
alongside stdout before the second runWithDeps invocation in
TestRunProvidersListMarksUserAndRuntimeProfiles, so failure reporting reflects
only the second command’s output.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/cli/command_center_test.go`:
- Around line 194-229: Reset stderr alongside stdout before the second
runWithDeps invocation in TestRunProvidersListMarksUserAndRuntimeProfiles, so
failure reporting reflects only the second command’s output.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 86a322da-ff12-4cef-bf1f-5f77bcde8327

📥 Commits

Reviewing files that changed from the base of the PR and between 60dc84e and 4b26069.

📒 Files selected for processing (4)

• internal/cli/command_center.go
• internal/cli/command_center_test.go
• internal/cli/provider_onboarding.go
• internal/cli/provider_onboarding_test.go

[Copilot]

Pull request overview

This PR improves clarity around provider selection by distinguishing the provider saved in user config from the provider that is actually effective at runtime (especially when ZERO_PROVIDER is set), and by enriching provider listing output so UIs can tell which entries are user-selectable vs runtime-derived.

Changes:

• Update providers use to report when ZERO_PROVIDER overrides the newly saved active provider, and include effective/override metadata in JSON output.
• Enhance providers list/current output (human + JSON) with selectable and source metadata to distinguish user-config providers from runtime-only entries.
• Add tests covering the override messaging/JSON payload and the selectable/source metadata in provider lists.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
internal/cli/provider_onboarding.go	Adds override detection for ZERO_PROVIDER to clarify saved vs effective provider; enriches JSON output and error messaging for non-selectable providers.
internal/cli/provider_onboarding_test.go	Adds coverage for override messaging/JSON and for improved error text when selecting runtime-only providers.
internal/cli/command_center.go	Adds provider list metadata (selectable, source) and updates formatting to mark runtime-only/non-selectable entries.
internal/cli/command_center_test.go	Adds coverage for new selectable/source JSON fields and for the runtime-only marker in human output.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kevincodex1]

please rebase. to main and fix conflicts

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Rebase this stale, superseded branch before merging.
  internal/cli/provider_onboarding.go
  The current head is not descended from main and has content conflicts in both this file and its test. The resulting base-to-head comparison spans 227 files rather than the PR's four-file merge-base diff. Main already contains #716's runtime-only-provider handling and #767's fix for #721; resolving these conflicts by taking this branch would remove both. Please rebase onto current main, retain the existing fixes, and submit the resolved diff for review rather than merging this head.

• [P2] Do not report an unresolvable ZERO_PROVIDER value as the effective provider.
  internal/cli/provider_onboarding.go:64
  A nonempty ZERO_PROVIDER is reported as the effective provider without checking that it resolves. For example, a stale ZERO_PROVIDER=runtime when no runtime profile exists prints Effective provider: runtime and suggests zero providers check runtime, but the next resolution fails before that check can run. Validate the override against the resolved provider list, or report it as invalid and direct the user to fix or unset it.

[Vasanthdev2004]

Heads-up @PierrunoYT: #767 merged and covers the core of this (the warning when ZERO_PROVIDER overrides a providers-use selection, #721). Your PR goes further though, and the extra parts are genuinely useful: the JSON effective-provider/override-source fields and the user-config-vs-runtime-only list marking are not in #767, so this is not fully superseded.

To carry it forward it would need a rebase onto main (post-#767) and a trim down to just the net-new parts (the JSON output, the list marking, and the runtime-only explanation), since the warning itself is now landed. Happy to review that, or if you would rather, fold those into a fresh focused PR. Let us know which you prefer.

[PierrunoYT]

Merged current main (dcf1a40) — the PR is MERGEABLE again — and addressed both findings plus the two bot comments.

[P2] Rebase this stale, superseded branch — done, and resolved away from this branch where main already had the behaviour. main's #767 covers the providers use override reporting with a cleaner design (the env read is injected through deps.getenv rather than a direct os.Getenv, so tests stay hermetic against an ambient ZERO_PROVIDER), and #716/#707 cover runtime-only provider handling. I kept all of that and dropped this branch's duplicate implementation and its two duplicate override tests, so the branch now adds only what main lacks:

• providers list / providers current: per-entry selectable + source in JSON, and a (not selectable via providers use) marker in the human output.
• providers use: an actionable error when the requested name is resolvable at runtime but cannot be saved (only providers saved in user config are selectable …).

Net diff against main is now 4 files / ~220 lines instead of the 227-file base-to-head comparison.

[P2] Do not report an unresolvable ZERO_PROVIDER value as the effective provider — fixed. New activeProviderEnvOverrideResolves checks, in order, the saved profile in the config file the command just wrote (so the check does not depend on ambient env) and then the resolver for env-synthesized profiles. When neither matches:

• the text note now reads "… but no provider named X can be resolved, so Zero cannot start until you unset ZERO_PROVIDER or point it at a saved provider" instead of calling it effective and suggesting zero providers check X;
• the JSON payload omits effectiveProvider entirely and reports overriddenByEnv, envProvider, and envProviderResolves: false.

TestRunProvidersUseFlagsUnresolvableEnvOverride covers both paths; main's resolvable-override tests still pass unchanged (their ZERO_PROVIDER=work is a saved profile, so it resolves and effectiveProvider is still reported).

@copilot: "runtime-only; not selectable/saved" over-claims for other persisted sources — valid, and it applies to the source value too, not just the prose. Project config and provider commands both contribute persisted profiles (mergeProjectConfig merges src.Providers), so those are not "runtime". The human marker is now (not selectable via providers use) — the only guaranteed claim — and the JSON source enum is user-config | resolved, documented next to the constants. The companion test comment about hard-coded wording is handled by the same rename.

@coderabbitai: reset stderr alongside stdout — already applied in fef6c5d; the second runWithDeps in TestRunProvidersListMarksUserAndRuntimeProfiles resets both buffers, and the new override test does the same.

Validation (Windows host, Go 1.26.5): go build ./..., go vet ./..., go test ./... -count=1 all pass; GOOS=linux|darwin|windows builds clean; gofmt clean on every changed file.

@jatmn @Vasanthdev2004 — the resolved diff is ready for another look; I can't use the reviewer-request button on this repo, hence the mention.

@coderabbitai review

[coderabbitai]

@PierrunoYT: Thanks for the detailed rebase and resolution notes. I’ll review the updated, reduced diff with particular attention to provider-source semantics, environment-override UX, and the new test coverage.

---

✏️ Learnings added

Learnt from: PierrunoYT
URL: https://github.com/Gitlawb/zero/pull/725

Timestamp: 2026-07-25T11:35:15.664Z
Learning: In this Go CLI, provider profiles may come from user config, project config, or provider commands; profiles resolved at runtime are not necessarily transient. For provider list/current output, `selectable` indicates whether `providers use` can persist the profile, and the documented JSON `source` values are `user-config` and `resolved`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (2)

internal/cli/provider_onboarding_test.go (1)

163-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a companion test for the resolver-fallback-resolves-true case.

This test nicely covers the fully-unresolvable override. Consider adding a sibling test where ZERO_PROVIDER names a provider that is not persisted but is resolvable via resolveCommandCenterConfig (e.g. set an ambient OPENAI_API_KEY and point the override at the env-derived provider name while switching to a different saved profile). That exercises the second branch of activeProviderEnvOverrideResolves (lines 148-156 in provider_onboarding.go), which currently only sees coverage via the persisted-check branch and the fully-unresolvable branch.

As per path instructions, "**/*_test.go: ... add regression tests for behavior changes."

🧪 Suggested additional test sketch

func TestRunProvidersUseFlagsResolvableEnvOverrideViaResolver(t *testing.T) {
	t.Setenv("OPENAI_API_KEY", "sk-env")
	configPath := providersUseOverrideConfig(t)
	deps := providerSetupDeps(configPath)
	deps.getenv = func(key string) string {
		switch key {
		case config.ActiveProviderEnv:
			return "openai"
		case "OPENAI_API_KEY":
			return "sk-env"
		default:
			return ""
		}
	}

	var stdout, stderr bytes.Buffer
	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 map[string]any
	if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil {
		t.Fatalf("decode JSON: %v\n%s", err, stdout.String())
	}
	if payload["effectiveProvider"] != "openai" {
		t.Fatalf("expected env-derived override to resolve via resolver fallback, got %#v", payload)
	}
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/provider_onboarding_test.go` around lines 163 - 213, Add a
sibling regression test next to TestRunProvidersUseFlagsUnresolvableEnvOverride
that covers an environment override naming a provider absent from persisted
profiles but resolvable by resolveCommandCenterConfig, such as “openai” with an
injected OPENAI_API_KEY. Invoke providers use with --json and assert successful
execution reports that provider as effectiveProvider, exercising the
resolver-fallback branch of activeProviderEnvOverrideResolves while retaining
the existing unresolvable test.

Source: Path instructions

internal/cli/provider_onboarding.go (1)

137-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Resolver-fallback branch of activeProviderEnvOverrideResolves is untested.

This function has two independent resolution paths: the persisted-config check (line 149) and the resolver-fallback via resolveCommandCenterConfig/providerResolvedByName (lines 152-156). Only the persisted-true case ("work") and the fully-unresolvable case ("removed-profile") are covered by tests; the case where ZERO_PROVIDER names an env-derived-but-unpersisted provider (e.g. an ambient OPENAI_API_KEY-derived profile) is not tested, leaving one of the two logical branches unverified.

See companion comment on internal/cli/provider_onboarding_test.go for the suggested regression test.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/provider_onboarding.go` around lines 137 - 158, Add a regression
test for activeProviderEnvOverrideResolves covering an env-derived provider that
is not persisted, such as one synthesized from an ambient OPENAI_API_KEY.
Configure the test so the persisted-profile check does not match,
resolveCommandCenterConfig succeeds, and providerResolvedByName finds the
override, asserting the function returns true while preserving existing
persisted and unresolvable cases.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/cli/provider_onboarding_test.go`:
- Around line 163-213: Add a sibling regression test next to
TestRunProvidersUseFlagsUnresolvableEnvOverride that covers an environment
override naming a provider absent from persisted profiles but resolvable by
resolveCommandCenterConfig, such as “openai” with an injected OPENAI_API_KEY.
Invoke providers use with --json and assert successful execution reports that
provider as effectiveProvider, exercising the resolver-fallback branch of
activeProviderEnvOverrideResolves while retaining the existing unresolvable
test.

In `@internal/cli/provider_onboarding.go`:
- Around line 137-158: Add a regression test for
activeProviderEnvOverrideResolves covering an env-derived provider that is not
persisted, such as one synthesized from an ambient OPENAI_API_KEY. Configure the
test so the persisted-profile check does not match, resolveCommandCenterConfig
succeeds, and providerResolvedByName finds the override, asserting the function
returns true while preserving existing persisted and unresolvable cases.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f848d4aa-1f56-4478-ba84-ea66efece4ee

📥 Commits

Reviewing files that changed from the base of the PR and between fef6c5d and dcf1a40.

📒 Files selected for processing (4)

• internal/cli/command_center.go
• internal/cli/command_center_test.go
• internal/cli/provider_onboarding.go
• internal/cli/provider_onboarding_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/cli/command_center_test.go
• internal/cli/command_center.go

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Determine the effective override from the resolved active profile
  internal/cli/provider_onboarding.go:149
  A persisted row, or merely a matching entry in resolved.Providers, is not proof that ZERO_PROVIDER is the provider a subsequent invocation will use. For example, a saved OpenAI-compatible profile without a model passes the persisted fast path, so providers use fast --json reports envProviderResolves: true and effectiveProvider: "broken"; the next resolution instead fails because the active broken profile requires a model. Likewise, a provider command can leave the env-named profile in the list while setting a different active provider. Run the resolver for every override and require the successfully resolved ActiveProvider to match the override before emitting the effective-provider fields/note.

• [P3] Do not case-fold provider identities when labeling individual list entries selectable
  internal/cli/command_center.go:154
  Resolution merges provider names case-sensitively, but this map collapses work and WORK. If user config has work and project config (or a provider command) contributes WORK, both resolved entries are labeled selectable: true, source: "user-config". providers use WORK only updates the user-config work row, so it cannot select the displayed WORK entry. Preserve the concrete persisted identity when deriving the metadata (or explicitly reject/handle case-only collisions) and add a regression test.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/cli/command_center_test.go`:
- Around line 271-276: Update the assertions in the provider cases of the test
to compare Source directly against the documented JSON strings "user-config" and
"resolved" instead of production constants, while preserving the existing
Selectable checks and failure messages.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: aa142389-f1f1-48db-8ae1-a5138ccf7608

📥 Commits

Reviewing files that changed from the base of the PR and between dcf1a40 and 9fd27b8.

📒 Files selected for processing (4)

• internal/cli/command_center.go
• internal/cli/command_center_test.go
• internal/cli/provider_onboarding.go
• internal/cli/provider_onboarding_test.go

🚧 Files skipped from review as they are similar to previous changes (3)

• internal/cli/provider_onboarding.go
• internal/cli/command_center.go
• internal/cli/provider_onboarding_test.go

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Avoid executing the provider command just to render the override note
  internal/cli/provider_onboarding.go:150
activeProviderEnvOverrideResolves now performs a full config resolution after providers use has already written config.json. When ZERO_PROVIDER_COMMAND is configured, that resolution runs the configured shell command (and can wait up to five seconds); its side effects or failure are then discarded and reported as an unresolved ZERO_PROVIDER. This makes a previously config-only selection unexpectedly execute an external command after committing the change. Determine the override status without loading the provider command, or surface and handle that resolution failure before claiming a successful selection.

• [P2] Resolve the case-variant provider identity ambiguity in list selectability
  internal/cli/command_center.go:160
  The new exact-case lookup correctly treats a resolved WORK profile as distinct from persisted work, but ProviderPersisted and SetActiveProvider still match case-insensitively. Copying the displayed WORK into zero providers use WORK therefore exits successfully while silently activating the different persisted work profile. Case-only persisted duplicates have the inverse problem: both are reported selectable although the mutator always selects the first match. Align the mutation and listing identity rules (or reject ambiguous case variants) so a selected list entry cannot resolve to a different provider.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Keep every provider mutator on the exact-name identity rule
  internal/config/writer.go:234
  This change makes ProviderPersisted and providers use case-sensitive, and the resolver/list now intentionally expose work and WORK as distinct profiles. However, RemoveProvider (and RenameProvider/SetProviderModel) still selects the first EqualFold match. With saved profiles ordered as work, WORK, zero providers remove WORK first passes the new exact persisted check, then deletes work; the CLI subsequently deletes the case-normalized stored credential too, leaving the requested WORK row behind without its key. Use the same exact identity for every mutator, or reject case-distinct profiles globally, and cover the remove/rename paths.

[PierrunoYT]

Split into 4 focused PRs per the review feedback above, in dependency order — each stacked on and reviewable against its predecessor:

1. Define provider identity primitives and persisted-name validation (1/4) #892 — Define provider identity primitives and persisted-name validation (1/4)
2. Resolve catalog ownership and credential candidates (2/4) #893 — Resolve catalog ownership and credential candidates (2/4), stacked on Define provider identity primitives and persisted-name validation (1/4) #892
3. Transact provider config and credential writes (3/4) #894 — Transact provider config and credential writes (3/4), stacked on Resolve catalog ownership and credential candidates (2/4) #893 — carries both P1 fixes (lock acquisition fail-closed, OpenRouter persistence joining the transaction)
4. Clarify provider selection and synchronize live TUI state (4/4) #895 — Clarify provider selection and synchronize live TUI state (4/4), stacked on Transact provider config and credential writes (3/4) #894

This PR stays open as the tracking/reference point until all four land.

[jatmn]

closing this pr now, new pr's have been updated accordingly.