Add provider onboarding UX#161
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds CLI provider onboarding (use/setup) and advice builders; enhances providers check with computed next-action guidance and JSON; refactors providers catalog output; expands provider-model catalog, remote fetching, and live discovery merge; adds a TUI provider wizard with discovery and many tests. ChangesProvider Onboarding Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/tui/command_views.go`:
- Around line 173-183: The fallback switch in providerCredentialRequired
incorrectly omits compatible variants, so when providerCatalogDescriptor returns
no descriptor credential warnings are suppressed; update the switch inside
providerCredentialRequired to include the compatible kinds used elsewhere (match
the variants handled by providerCredentialEnvName), e.g., add
config.ProviderKindOpenAICompatible and config.ProviderKindAnthropicCompat (or
the project's exact compatible constant names) alongside ProviderKindOpenAI and
ProviderKindAnthropic so compatible providers are treated as requiring
credentials.
🪄 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: ea2ebd3c-b85c-4a41-ae45-56749f2c6712
📒 Files selected for processing (12)
internal/cli/command_center.gointernal/cli/provider_catalog_onboarding_test.gointernal/cli/provider_check_guidance_test.gointernal/cli/provider_onboarding.gointernal/cli/provider_onboarding_test.gointernal/cli/provider_setup.gointernal/config/writer.gointernal/config/writer_test.gointernal/provideronboarding/advice.gointernal/provideronboarding/advice_test.gointernal/tui/command_views.gointernal/tui/provider_onboarding_test.go
There was a problem hiding this comment.
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/tui/provider_wizard_test.go`:
- Line 91: Update the test's negative assertion that verifies the local-provider
credential step to match the new copy: replace the expected string "Add API key"
with "Paste API key" in the local-provider guard assertion so the test aligns
with the changed UI text (search for the assertion comparing the credential step
label in provider_wizard_test.go and update the expected value).
🪄 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: 040faa0f-2531-49f9-a686-362762c42609
📒 Files selected for processing (3)
internal/tui/model.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui/provider_wizard.go
- internal/tui/model.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/tui/provider_wizard_test.go (1)
186-188:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign local-provider negative assertion with updated credential copy.
Line 139 in
TestProviderWizardAdvancesProviderAPIKeyAndModelStepsuses"Paste API key", but this test still checks for"Add API key". Update to match the current UI text.Suggested fix
- if strings.Contains(view, "Add API key") { + if strings.Contains(view, "Paste API key") { t.Fatalf("local provider should skip API key step, got view:\n%s", view) }🤖 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/tui/provider_wizard_test.go` around lines 186 - 188, The negative assertion in provider_wizard_test.go is out of date: in TestProviderWizardAdvancesProviderAPIKeyAndModelSteps the UI text changed from "Add API key" to "Paste API key", so update the check that inspects the view (the if block that currently looks for "Add API key") to instead look for "Paste API key" so the local-provider test correctly asserts the API key step is skipped.
🤖 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.
Duplicate comments:
In `@internal/tui/provider_wizard_test.go`:
- Around line 186-188: The negative assertion in provider_wizard_test.go is out
of date: in TestProviderWizardAdvancesProviderAPIKeyAndModelSteps the UI text
changed from "Add API key" to "Paste API key", so update the check that inspects
the view (the if block that currently looks for "Add API key") to instead look
for "Paste API key" so the local-provider test correctly asserts the API key
step is skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9935fa98-519f-4e16-8414-b356478cd63c
📒 Files selected for processing (3)
internal/tui/provider_wizard.gointernal/tui/provider_wizard_models.gointernal/tui/provider_wizard_test.go
💤 Files with no reviewable changes (1)
- internal/tui/provider_wizard.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/tui/provider_wizard_test.go (1)
188-189:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale local-provider credential-step assertion copy.
Line 188 still checks
"Add API key", but the wizard copy is"Paste API key". This can let a credential-step regression slip through.Suggested fix
- if strings.Contains(view, "Add API key") { + if strings.Contains(view, "Paste API key") { t.Fatalf("local provider should skip API key step, got view:\n%s", view) }🤖 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/tui/provider_wizard_test.go` around lines 188 - 189, The assertion in internal/tui/provider_wizard_test.go is checking for the outdated string "Add API key" in the view variable; update that check to expect the current wizard copy "Paste API key" so the local-provider credential-step test fails correctly on regressions—locate the if check that reads if strings.Contains(view, "Add API key") and change the literal to "Paste API key" (keeping the same t.Fatalf message structure that prints view).
🤖 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.
Duplicate comments:
In `@internal/tui/provider_wizard_test.go`:
- Around line 188-189: The assertion in internal/tui/provider_wizard_test.go is
checking for the outdated string "Add API key" in the view variable; update that
check to expect the current wizard copy "Paste API key" so the local-provider
credential-step test fails correctly on regressions—locate the if check that
reads if strings.Contains(view, "Add API key") and change the literal to "Paste
API key" (keeping the same t.Fatalf message structure that prints view).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ebf1da9a-6de3-45f0-8bf3-c3e58bde5f0a
📒 Files selected for processing (7)
internal/providermodeldiscovery/discovery.gointernal/providermodeldiscovery/discovery_test.gointernal/tui/model.gointernal/tui/options.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_discovery.gointernal/tui/provider_wizard_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui/model.go
- internal/tui/provider_wizard.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/provider_wizard_discovery.go (1)
69-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedaction misses env-sourced API keys in discovery errors.
At Line 70, only
wizard.apiKeyis redacted. But Line 119–Line 121 can sourceprofile.APIKeyfromAPIKeyEnv, so an error containing that key can leak.🔐 Proposed fix
func (m model) applyProviderModelsDiscovered(msg providerModelsDiscoveredMsg) model { wizard := m.providerWizard if wizard == nil || wizard.currentProvider().ID != msg.providerID { return m } wizard.modelLoading = false if msg.err != nil { - wizard.modelLoadError = redaction.RedactString(msg.err.Error(), redaction.Options{ExtraSecretValues: []string{wizard.apiKey}}) + secrets := make([]string, 0, 2) + if key := strings.TrimSpace(wizard.apiKey); key != "" { + secrets = append(secrets, key) + } + if key := strings.TrimSpace(providerWizardDiscoveryProfile(wizard.currentProvider(), wizard.apiKey).APIKey); key != "" { + secrets = append(secrets, key) + } + wizard.modelLoadError = redaction.RedactString(msg.err.Error(), redaction.Options{ExtraSecretValues: secrets}) wizard.modelSource = "fallback" wizard.refreshModels() return m }Also applies to: 117-121
🤖 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/tui/provider_wizard_discovery.go` around lines 69 - 71, The redaction call that sets wizard.modelLoadError only includes wizard.apiKey, so API keys sourced into profile.APIKey (including from APIKeyEnv) can leak in discovery errors; update the redaction to include all possible API key values (wizard.apiKey and profile.APIKey or any env-derived API key) when calling redaction.RedactString (the code paths around wizard.modelLoadError, wizard.apiKey, profile.APIKey and APIKeyEnv) so the error string is scanned for both values before assigning wizard.modelLoadError and modelSource.
🤖 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.
Outside diff comments:
In `@internal/tui/provider_wizard_discovery.go`:
- Around line 69-71: The redaction call that sets wizard.modelLoadError only
includes wizard.apiKey, so API keys sourced into profile.APIKey (including from
APIKeyEnv) can leak in discovery errors; update the redaction to include all
possible API key values (wizard.apiKey and profile.APIKey or any env-derived API
key) when calling redaction.RedactString (the code paths around
wizard.modelLoadError, wizard.apiKey, profile.APIKey and APIKeyEnv) so the error
string is scanned for both values before assigning wizard.modelLoadError and
modelSource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b91a52e5-174b-4642-a227-90503451d200
📒 Files selected for processing (9)
internal/providermodelcatalog/catalog.gointernal/providermodelcatalog/remote.gointernal/providermodelcatalog/remote_test.gointernal/providermodeldiscovery/discovery.gointernal/providermodeldiscovery/discovery_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_discovery.gointernal/tui/provider_wizard_models.gointernal/tui/provider_wizard_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui/provider_wizard_test.go
- internal/tui/provider_wizard.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tui/provider_wizard.go`:
- Around line 439-446: In renderModelSearch, the cursor glyph is currently the
garbled literal "▌"; change the trailing cursor in the input string to the
proper block cursor used elsewhere (e.g., "▌") so the search box shows the
correct cursor; update the string concatenation that builds input in
providerWizardState.renderModelSearch to use the proper glyph (matching the
credential step's cursor) so zeroTheme renders the cursor correctly.
- Around line 94-101: The currentModel path returns provider.DefaultModel when
filteredModels() is empty, allowing Enter to advance with an unseen model;
change the empty-filter branch (after refreshModels()/filteredModels()) to
return a sentinel providerWizardModel with an empty ID and a Description like
"no matching models" (and set wizard.selectedModel to -1 or 0 consistently), and
then update the wizard advance/Enter handler to check currentModel().ID == ""
and block progressing to the done step when empty; make the same change in the
other occurrences (the analogous blocks referenced around lines 148-150 and
423-425) so Enter is refused whenever filteredModels() yields zero results.
🪄 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: 0bbe92eb-2987-469b-a404-f2bd02b9d34a
📒 Files selected for processing (3)
internal/providermodelcatalog/remote.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/providermodelcatalog/remote.go
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Finding:
[P1] Blank-key onboarding activates a provider with no API key. In internal/tui/provider_wizard.go:293, the wizard builds the profile and passes it directly into m.newProvider(profile). When the user leaves the credential step blank, providerWizardProfile stores only APIKeyEnv (internal/tui/provider_wizard.go:583-587). The provider factory expects an already-resolved profile and passes only profile.APIKey to the runtime provider (internal/providers/factory.go:32-33, same pattern for Anthropic/Gemini), so the current TUI session switches to a provider with an empty key. The saved config will work after restart because config resolution fills the env value, but the wizard says it will save and use the provider now. Fix by using a runtime copy of the profile that resolves APIKeyEnv from the process env before calling m.newProvider, while still persisting the env-var reference instead of the secret, and add a test for the blank-key/env-var path.
Validation:
GOCACHE=/tmp/zero-go-cache-pr161 go test ./...passedGOCACHE=/tmp/zero-go-cache-pr161 go vet ./...passedgit diff --check main...HEADpassed
Coordination note: this overlaps with #163 in provider catalog/TUI setup files, so merge or rebase order still matters, but that is separate from the blocker above.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
No blocking issues found on re-review.
The new Resolve provider wizard env credentials commit addresses my previous blocker: the wizard now builds a runtime-only profile with APIKeyEnv resolved from the process env before calling m.newProvider, while keeping the persisted/session profile on the env-var reference instead of storing the secret. The added regression test covers that exact blank-key/env-var path.
Validation:
GOCACHE=/tmp/zero-go-cache-pr161-rereview go test ./...passedGOCACHE=/tmp/zero-go-cache-pr161-rereview-vet go vet ./...passedgit diff --check main...HEADpassed
Still worth coordinating merge/rebase order with #163 because both touch provider catalog/TUI setup files, but that is not a blocker for this PR behavior.
2 participants
Summary
zero providers useandzero providers setuponboarding commands/providerguidance for missing profiles and missing credentialsValidation
gofmt -l internal\cli internal\config internal\provideronboarding internal\tuigit diff --checkgo test ./...go vet ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokego run ./cmd/zero providers setup groq --name fast --set-activego run ./cmd/zero providers catalog --transport openai-compatiblego run ./cmd/zero providers --helpSummary by CodeRabbit
New Features
providers useandproviders setup(human/JSON plans, optional --set-active) and richerproviders checkoutput with actionable “next” guidance.Tests