fix(config): fall back to a usable saved provider instead of forcing full re-onboarding#410
fix(config): fall back to a usable saved provider instead of forcing full re-onboarding#410PierrunoYT wants to merge 1 commit into
Conversation
…full re-onboarding When config.json has providers configured but none marked active (e.g. a blank/stale activeProvider field, or a config with 2+ providers where none was ever selected), normalizeProviders threw away the already-normalized provider list on ErrNoActiveProvider. That meant the interactive TUI's existing "fall back to an already-configured usable provider" logic (firstUsableProvider) never got a chance to run for this case — the app instead wiped resolved to a zero value and forced the user into a brand-new onboarding wizard, even though a usable, already-saved provider existed. - normalizeProviders/Resolve now return the normalized providers list alongside ErrNoActiveProvider instead of discarding it. - runInteractiveTUIWithSetup tries firstUsableProvider on that list before forcing setup; onboarding only runs when no configured provider is actually usable, matching the existing fallback already used for a different flavor of this problem (active-provider-present-but-no-credential). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
WalkthroughThe resolver now preserves the normalized provider list when resolution fails specifically due to a missing active provider ( ChangesProvider fallback fix
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as runInteractiveTUIWithSetup
participant Resolver as config.Resolve
participant TUI as runTUI
CLI->>Resolver: resolveConfig()
Resolver-->>CLI: ResolvedConfig{Providers}, ErrNoActiveProvider
CLI->>CLI: firstUsableProvider(resolved.Providers)
alt usable provider found
CLI->>CLI: set resolved.Provider, resolved.ActiveProvider
CLI->>TUI: runTUI(resolved)
else no usable provider
CLI->>CLI: clear resolved, forceSetup=true
CLI->>TUI: runTUI(forceSetup)
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/cli/app.go (2)
566-572: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
firstUsableProviderfallback pattern.The "find a usable provider and set
resolved.Provider/resolved.ActiveProvider" logic appears twice — once here (566-572) and once at 599-603 for theneedsSetupcase. Extracting a small helper (e.g.applyUsableProviderFallback(resolved *config.ResolvedConfig) bool) would remove the duplication and make it a single spot to fix if theErrProviderRequiresModelgap above is addressed later.♻️ Suggested extraction
+func applyUsableProviderFallback(resolved *config.ResolvedConfig) bool { + usable, ok := firstUsableProvider(resolved.Providers) + if !ok { + return false + } + resolved.Provider = usable + resolved.ActiveProvider = usable.Name + return true +}Also applies to: 592-604
🤖 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/app.go` around lines 566 - 572, The fallback logic that calls firstUsableProvider and sets resolved.Provider/resolved.ActiveProvider is duplicated in the config handling flow. Extract this repeated branch into a small helper such as applyUsableProviderFallback that accepts the resolved config and returns whether a usable provider was applied, then use that helper in both the current err/config.ErrNoActiveProvider path and the needsSetup path to keep the behavior in one place.
560-572: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFallback correctly gated to
ErrNoActiveProvider.Confirmed
resolved.Providersisnilfor theErrProviderRequiresModelpath (sincenormalizeProvidersWithOptionsreturnsnilthere), sofirstUsableProvidernaturally returnsok=falseand theelsebranch is taken — no functional bug from the sharedfirstUsableProvidercall preceding the&&.One asymmetry worth a follow-up: if
config.jsonhas an active provider missing itsmodel(triggeringErrProviderRequiresModel) but another fully usable profile exists, this branch still forces full re-onboarding instead of falling back — mirroring the exact UX problem issue#409fixed for the "no active marked" case. SincenormalizeProvidersdiscards the normalized list onErrProviderRequiresModel(resolver.go line 848), fixing this would require preserving providers there too. Given issue#409explicitly scopes toErrNoActiveProvider, this can be deferred to a follow-up.🤖 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/app.go` around lines 560 - 572, The fallback logic in the config resolution branch only handles `config.ErrNoActiveProvider`, so `ErrProviderRequiresModel` still wipes `resolved` and forces onboarding even when another saved provider is usable. Update the resolution flow around `firstUsableProvider`, `resolved.Providers`, and `normalizeProviders` so the normalized provider list is preserved for the `ErrProviderRequiresModel` path as well, then reuse the same fallback behavior instead of clearing `ResolvedConfig` when a usable provider exists.
🤖 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/app.go`:
- Around line 566-572: The fallback logic that calls firstUsableProvider and
sets resolved.Provider/resolved.ActiveProvider is duplicated in the config
handling flow. Extract this repeated branch into a small helper such as
applyUsableProviderFallback that accepts the resolved config and returns whether
a usable provider was applied, then use that helper in both the current
err/config.ErrNoActiveProvider path and the needsSetup path to keep the behavior
in one place.
- Around line 560-572: The fallback logic in the config resolution branch only
handles `config.ErrNoActiveProvider`, so `ErrProviderRequiresModel` still wipes
`resolved` and forces onboarding even when another saved provider is usable.
Update the resolution flow around `firstUsableProvider`, `resolved.Providers`,
and `normalizeProviders` so the normalized provider list is preserved for the
`ErrProviderRequiresModel` path as well, then reuse the same fallback behavior
instead of clearing `ResolvedConfig` when a usable provider exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64e6ff66-f8a3-4623-b3b1-934a367e000a
📒 Files selected for processing (4)
internal/cli/app.gointernal/cli/app_test.gointernal/config/resolver.gointernal/config/resolver_test.go
2 participants
Summary
When
config.jsonhas providers configured but none marked active (blank/staleactiveProvider, or 2+ providers where none was ever selected), the interactive TUI discarded the already-normalized provider list and forced a full re-onboarding wizard — even though a usable, already-authenticated provider was already saved.normalizeProviders/Resolve(internal/config/resolver.go) now return the normalized providers list alongsideErrNoActiveProviderinstead of discarding it.runInteractiveTUIWithSetup(internal/cli/app.go) tries the existingfirstUsableProviderfallback on that list before forcing setup. Onboarding only runs when no configured provider is actually usable — the same behavior already used for a different flavor of this problem (active provider present but missing a credential).Linked issue
Fixes #409
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Verification notes
go build ./.../go vet ./...clean.TestResolveKeepsNormalizedProvidersWhenNoneMarkedActive(internal/config/resolver_test.go) andTestRunNoArgsFallsBackToUsableProviderWhenNoneMarkedActive(internal/cli/app_test.go); both pass, along with the pre-existing adjacent tests (TestRunNoArgsEntersSetupWhenResolveReportsNoActiveProvider,TestResolveRejectsActiveProviderWithoutConfiguredProfiles) confirming the "genuinely nothing configured" path is unchanged.go test ./internal/config/... ./internal/cli/...has the same 8 pre-existing failures present onmainbefore this change (no active provider/API key configured in this sandbox environment) — none are new or related to this change.gofmt -lon the touched files flags them, but they were already flagged identically before this change (repo-widecore.autocrlf=trueCRLF artifact on Windows) — not introduced here.Summary by CodeRabbit