fix(provider-wizard): allow multiple custom OpenAI-compatible providers#389
fix(provider-wizard): allow multiple custom OpenAI-compatible providers#389KunjShah95 wants to merge 3 commits into
Conversation
…rs to be configured
WalkthroughProvider wizard saved-key matching now skips CatalogID checks for generic provider IDs, and the Replace action clears more state before routing to the next wizard step. MCP OAuth login tests also allow longer time for callback and completion. ChangesProvider Wizard Fix
MCP OAuth Login Test Timing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
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.go (1)
939-952: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd coverage for the Replace → endpoint path
TestProviderWizardManageKeyReplaceAndKeepstill only hits the credential-step branch because the test buildsproviderWizardStatewithoutproviders, socurrentProvider()is zero-value andproviderWizardNeedsEndpoint(...)stays false. Add a case with acustom-openai-compatibleorcustom-anthropic-compatibleprovider so Replace exercises the new endpoint routing too.🤖 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.go` around lines 939 - 952, Add test coverage for the Replace flow’s endpoint-routing path in the provider wizard. Update TestProviderWizardManageKeyReplaceAndKeep so it doesn’t only exercise the credential-step branch: build providerWizardState with a real provider entry so currentProvider() is populated and providerWizardNeedsEndpoint(...) can become true. Add a Replace case using a custom-openai-compatible or custom-anthropic-compatible provider and assert wizardProviderStoredKey/current provider handling covers the endpoint path too.
🤖 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 964-974: The Replace branch in providerWizardUpdate now routes to
providerWizardStepEndpoint when providerWizardNeedsEndpoint(currentProvider())
is true, but the existing TestProviderWizardManageKeyReplaceAndKeep never sets
wizard.providers so currentProvider() stays zero-value and only the
credential-step path is covered. Update
TestProviderWizardManageKeyReplaceAndKeep in provider_wizard_test.go, or add a
dedicated case, to populate wizard.providers with a custom-openai-compatible
descriptor and assert that the Replace flow reaches providerWizardStepEndpoint
instead of only providerWizardStepCredential.
---
Outside diff comments:
In `@internal/tui/provider_wizard.go`:
- Around line 939-952: Add test coverage for the Replace flow’s endpoint-routing
path in the provider wizard. Update TestProviderWizardManageKeyReplaceAndKeep so
it doesn’t only exercise the credential-step branch: build providerWizardState
with a real provider entry so currentProvider() is populated and
providerWizardNeedsEndpoint(...) can become true. Add a Replace case using a
custom-openai-compatible or custom-anthropic-compatible provider and assert
wizardProviderStoredKey/current provider handling covers the endpoint path too.
🪄 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
Run ID: 2547d457-edd5-4768-a06b-40c8caf72953
📒 Files selected for processing (1)
internal/tui/provider_wizard.go
| case 1: // Replace | ||
| wizard.apiKey = "" | ||
| wizard.err = "" | ||
| wizard.step = providerWizardStepCredential | ||
| wizard.baseURL = "" | ||
| wizard.profileName = "" | ||
| if providerWizardNeedsEndpoint(wizard.currentProvider()) { | ||
| wizard.step = providerWizardStepEndpoint | ||
| } else { | ||
| wizard.step = providerWizardStepCredential | ||
| } | ||
| return m, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add test coverage for the new endpoint-step routing.
The Replace branch's conditional routing to providerWizardStepEndpoint (Lines 969-970) is the core fix enabling users to enter a new base URL for a second custom provider, but TestProviderWizardManageKeyReplaceAndKeep in provider_wizard_test.go (context snippet 4) never populates wizard.providers, so currentProvider() returns a zero-value descriptor and providerWizardNeedsEndpoint always evaluates to false there — only the else credential-step path is exercised. The behavior this PR is meant to add is currently unverified by tests.
Consider extending the test (or adding a new case) with wizard.providers set to a custom-openai-compatible-ID descriptor so the Replace path to providerWizardStepEndpoint is asserted.
🤖 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.go` around lines 964 - 974, The Replace branch
in providerWizardUpdate now routes to providerWizardStepEndpoint when
providerWizardNeedsEndpoint(currentProvider()) is true, but the existing
TestProviderWizardManageKeyReplaceAndKeep never sets wizard.providers so
currentProvider() stays zero-value and only the credential-step path is covered.
Update TestProviderWizardManageKeyReplaceAndKeep in provider_wizard_test.go, or
add a dedicated case, to populate wizard.providers with a
custom-openai-compatible descriptor and assert that the Replace flow reaches
providerWizardStepEndpoint instead of only providerWizardStepCredential.
|
fix coderabbit issues @KunjShah95 |
…routing and fix flaky MCP OAuth test
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/provider_wizard_test.go (1)
1170-1189: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen test to also verify state-clearing on Replace.
Test only checks the step transition to
providerWizardStepEndpoint, but the underlying fix (per PR summary) is that Replace now clearsapiKey,baseURL,profileName, anderrbefore routing. Pre-seeding those fields with stale values and asserting they're cleared would directly cover the behavior this fix targets — without it, a regression that stops clearing state (letting a second provider inherit the first provider's baseURL) would slip through undetected.♻️ Suggested strengthening
m.providerWizard = &providerWizardState{ step: providerWizardStepManageKey, manageProviderName: "my-custom-openai", manageKeyCursor: 1, + apiKey: "stale-key", + baseURL: "https://stale.example.com", + profileName: "stale-profile", + err: "stale error", providers: []providercatalog.Descriptor{ { ID: "custom-openai-compatible", Transport: providercatalog.TransportOpenAICompatible, }, }, selectedProvider: 0, } next, _ := m.applyManageKeyChoice() if next.providerWizard == nil || next.providerWizard.step != providerWizardStepEndpoint { t.Fatalf("replace for generic provider should route to the endpoint step, got step: %v", next.providerWizard.step) } + if next.providerWizard.apiKey != "" || next.providerWizard.baseURL != "" || next.providerWizard.profileName != "" || next.providerWizard.err != "" { + t.Fatal("replace should clear stale apiKey/baseURL/profileName/err before routing") + } }🤖 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 1170 - 1189, Strengthen TestProviderWizardManageKeyReplaceGeneric to cover the Replace state-reset behavior in applyManageKeyChoice, not just the providerWizardStepEndpoint transition. Pre-seed providerWizardState fields like apiKey, baseURL, profileName, and err with stale values before calling applyManageKeyChoice, then assert those fields are cleared in the returned state when manageKeyCursor selects Replace. Use the existing providerWizardState, providerWizardStepManageKey, and providerWizardStepEndpoint symbols to locate the test and verify the generic-provider Replace path.
🤖 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/tui/provider_wizard_test.go`:
- Around line 1170-1189: Strengthen TestProviderWizardManageKeyReplaceGeneric to
cover the Replace state-reset behavior in applyManageKeyChoice, not just the
providerWizardStepEndpoint transition. Pre-seed providerWizardState fields like
apiKey, baseURL, profileName, and err with stale values before calling
applyManageKeyChoice, then assert those fields are cleared in the returned state
when manageKeyCursor selects Replace. Use the existing providerWizardState,
providerWizardStepManageKey, and providerWizardStepEndpoint symbols to locate
the test and verify the generic-provider Replace path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ef1c56c-41c5-4e0b-9db9-989d6326ebc7
📒 Files selected for processing (2)
internal/cli/mcp_oauth_test.gointernal/tui/provider_wizard_test.go
Fixes #375
Problem: \wizardProviderStoredKey\ matched saved providers by CatalogID. Since ALL custom OpenAI-compatible providers share CatalogID \custom-openai-compatible, setting up one blocked configuring a second.
Fix:
Summary by CodeRabbit