fix(provider-wizard): allow multiple custom OpenAI-compatible providers#389
fix(provider-wizard): allow multiple custom OpenAI-compatible providers#389KunjShah95 wants to merge 4 commits into
Conversation
…rs to be configured
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughProvider wizard saved-key matching now skips CatalogID checks for generic providers, 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
|
do check it out now @Vasanthdev2004 |
|
yea wait a min @KunjShah95 |
There was a problem hiding this comment.
Verdict: Correct fix — but #403 is the stronger duplicate of this exact change;
I checked this out, built it, ran the wizard suite (green), and reproduced the fix directly: with a saved custom-openai-compatible provider, re-selecting the generic "Custom OpenAI-compatible" catalog entry now returns matched=false from wizardProviderStoredKey, so the wizard proceeds to a fresh endpoint/name/key flow instead of getting trapped in keep/replace/remove. It does fix #375. CI is green.
But this PR (#389) and #403 fix the same bug the same way, and #403 is the cleaner implementation. Since you can only merge one, here's the honest comparison.
Where the two differ
- Locus of the fix. #389 mutates the low-level lookup
wizardProviderStoredKey(adds a!genericProviderCatalogID(...)guard around the CatalogID clause). #403 leaves the lookup's semantics intact and gates at the call site inadvanceProviderWizard— "if this provider needs an endpoint, skip ManageKey and fall through to add." Gating at the decision point is the better locus: the lookup keeps meaning "is there a stored key for this descriptor," and only the manage-vs-add decision changes. - Test coverage. #403 adds
TestAdvanceProviderWizardCustomSkipsManageKey(table over bothcustom-openai-compatibleandcustom-anthropic-compatible, asserting the realadvanceProviderWizardpath routes to the endpoint step, not ManageKey) plusTestAdvanceProviderWizardNamedShowsManageKey(a named provider still reaches ManageKey — guards against an over-broad skip). #389'sTestProviderWizardManageKeyReplaceGenericinstead exercises a manually-constructed ManageKey state that the real flow can no longer reach for a generic provider (see below), so it doesn't actually cover the add-a-second-provider scenario that #375 is about. - Scope. #389 also bundles unrelated
mcp_oauth_test.gotimeout bumps (3s→15s, 10s→20s). Harmless flake-loosening, but it doesn't belong in this fix.
One thing both PRs should acknowledge (shared limitation, not a blocker)
The wizard's provider list comes from providercatalog.All(), so custom providers only ever appear as the single generic "Custom OpenAI-compatible" entry — never as your saved instance name. wizardProviderStoredKey (via advanceProviderWizard) is the only path into keep/replace/remove. Once we skip the CatalogID match for generics, there's no longer any way to reach replace/remove for an existing custom provider through the wizard (the generic descriptor's Name is "Custom OpenAI-compatible", which won't match a user's saved name). That's an acceptable trade to unblock multi-provider setup — the old behavior could only ever manage the first CatalogID match anyway — but it means removing a stale custom key now has no wizard surface. Worth a short follow-up (e.g. list saved custom instances for management), and worth a one-line note in the PR so it's a conscious choice.
As a direct consequence, #389's applyManageKeyChoice Replace change (clear baseURL/profileName, route to endpoint for generics) is now effectively unreachable in the real flow — generic providers can't reach ManageKey anymore. It's harmless/defensive, but it's dead relative to the actual bug.
Recommendation
Both are correct. I'd consolidate on #403 (cleaner locus, tests the real flow for both custom types + a named-provider guard, no unrelated changes) and close this one — or, if you prefer this author, ask them to (1) drop the mcp_oauth timeout hunk, and (2) replace the ManageKey-replace test with a table test that drives advanceProviderWizard for both custom transports like #403 does. Either way, please capture the "can't manage an existing custom key via the wizard" follow-up.
Thanks @KunjShah95 — the diagnosis is right and the fix works; this is purely a "which of two identical fixes to keep" call.
|
ohkay no problem bhai |
|
Closing in favor of #403, which fixes the same issue (#375) with a cleaner approach — it gates at the call site in Thanks a lot @KunjShah95 — your diagnosis of #375 was spot on and your fix worked; this is purely a "two identical fixes, keep the cleaner one" call, not a knock on the work. Full comparison is in my review above. Appreciate the contribution, and look forward to the next one. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Thanks for this — the core fix (hunk 1: wizardProviderStoredKey no longer matching a generic custom descriptor by CatalogID) correctly resolves #375, and the requested Replace→endpoint test is now in.
I'm going to go with #403 for this issue, though, so I'd suggest closing this in its favor — two reasons:
- Unrelated scope creep: this also bumps the MCP-OAuth login test timeouts (
mcp_oauth_test.go) in a different subsystem — the commit message even calls it "fix flaky MCP OAuth test." That doesn't belong in a provider-wizard change. - Redundant second hunk: the
applyManageKeyChoiceReplace hunk repairs a path that hunk 1 makes unreachable for custom providers, and the added test targets that dead path rather than the actual #375 repro.
#403 does the same fix with one localized change plus tests that hit the real repro and the regression boundary, no scope creep. No hard feelings — your root-cause diagnosis was correct. Marking request-changes to reflect that this shouldn't merge as-is.
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