Provider loading fix#623
Conversation
d3ef7f3 to
561afc5
Compare
|
@kevincodex1 @Vasanthdev2004 @gnanam1990 @auriti @anandh8x please take a look at this pr as it is a significant fix, right now setting provider via ui is broken |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Review: Provider Loading Fix
Thanks for this PR — fixing the /provider UI is important. The overall direction (treating Gemini and Mistral as first-class providers instead of OpenAI aliases) is correct. But I found a hard bug that makes the Gemini path non-functional, plus a few other issues that need attention before merge.
🔴 Blocker 1: isGeminiMode checks the wrong env var
src/services/api/providerConfig.ts line ~363:
const isGeminiMode = isEnvTruthy(process.env.CLAUDE_CODE_USE_MISTRAL)This is a copy-paste bug — isGeminiMode is checking CLAUDE_CODE_USE_MISTRAL instead of CLAUDE_CODE_USE_GEMINI. This means:
- When
CLAUDE_CODE_USE_GEMINI=1is set,isGeminiModeis false - The Gemini model/baseUrl resolution path in
resolveProviderRequest()is completely dead - Setting
CLAUDE_CODE_USE_MISTRAL=1accidentally activates bothisMistralModeandisGeminiMode
Fix: const isGeminiMode = isEnvTruthy(process.env.CLAUDE_CODE_USE_GEMINI)
🔴 Blocker 2: Unused import will cause issues
src/utils/providerProfile.ts:
import provider from 'src/commands/provider/index.js'This import is never used anywhere in the file. It will likely trigger lint errors and adds an unnecessary circular dependency risk (providerProfile.ts → commands/provider/index.js → potentially back to providerProfile.ts).
Fix: Remove this import.
🟡 Concern: Precedence behavior change in buildStartupEnvFromProfile
The old code had an early return:
if (hasExplicitProviderSelection(processEnv) && !profileManagedEnv) {
return processEnv
}This is removed entirely. The new code only has the profileManagedEnv check and the comment says "we should still apply the persisted profile rather than returning early."
This is a behavioral breaking change that I think is the right call (the old "explicit selection bypasses profile" logic was causing the bug where /provider settings didn't stick), but it needs to be called out explicitly because:
- Users who set
CLAUDE_CODE_USE_GEMINI=1in their shell will now have the persisted profile override their env var on startup, unlessCLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED=1. - The test "buildStartupEnvFromProfile treats explicit falsey provider flags as user intent" now asserts the opposite behavior —
CLAUDE_CODE_USE_OPENAI='0'no longer means "don't use OpenAI", it gets overridden by the persisted Gemini profile.
This seems intentional but it's a subtle semantic shift. I'd recommend adding a clear comment explaining the new precedence rules so future readers understand why env vars no longer win over the profile.
🟡 Concern: applyProviderProfileToProcessEnv doesn't clear CLAUDE_CODE_USE_GEMINI for Mistral and vice versa
For the new mistral branch in applyProviderProfileToProcessEnv:
if (profile.provider === 'mistral') {
process.env.CLAUDE_CODE_USE_MISTRAL = '1'
process.env.MISTRAL_BASE_URL = profile.baseUrl
process.env.MISTRAL_MODEL = profile.model
delete process.env.OPENAI_BASE_URL
delete process.env.OPENAI_API_KEY
delete process.env.OPENAI_MODEL
return
}It deletes OpenAI env vars but doesn't delete CLAUDE_CODE_USE_GEMINI, GEMINI_API_KEY, GEMINI_MODEL, GEMINI_BASE_URL, GEMINI_AUTH_MODE, or GEMINI_ACCESS_TOKEN. The existing OpenAI branch similarly doesn't clear these. If a user switches from Gemini to Mistral via /provider, the Gemini env vars will linger and could cause both CLAUDE_CODE_USE_GEMINI=1 and CLAUDE_CODE_USE_MISTRAL=1 to be set simultaneously.
The Gemini branch has the same issue — it doesn't clear CLAUDE_CODE_USE_MISTRAL or Mistral env vars.
Fix: Both new branches should clear the competing provider's env vars, matching the pattern from the existing OpenAI/anthropic branches.
🟢 Non-blocking: Gemini context window values look correct
gemini-3.1-pro: 1,048,576 context, 65,536 max output — matches Google's official model card ✅gemini-3.1-flash-lite-preview: 1,048,576 context, 65,536 max output — Google says 66K output, 65K is a reasonable conservative default ✅
These partially overlap with PR #602 (which I requested changes on for different incorrect values). Would be good to coordinate so they don't conflict on merge.
🟢 Non-blocking: PROVIDERS array doesn't include github, bedrock, vertex, foundry
The new PROVIDERS constant is ['openai', 'anthropic', 'mistral', 'gemini'] but hasExplicitProviderSelection still checks CLAUDE_CODE_USE_GITHUB, CLAUUDE_CODE_USE_BEDROCK, etc. The new provider-switching loop in buildLaunchEnv only iterates over PROVIDERS, so GitHub/Bedrock/Vertex/Foundry users are excluded from the auto-profile detection. This seems intentional (only the 4 first-class providers get profile support), but worth documenting.
Verdict: Needs changes 🔧
Two hard blockers:
isGeminiModechecks wrong env var → Gemini routing is broken- Unused import of
commands/provider/index.js→ lint/circular-dep risk
One significant concern to address:
3. Cross-provider env var cleanup when switching providers (Gemini ↔ Mistral leaves stale env vars)
Once those are fixed, this is approve-ready. The core fix — making Gemini and Mistral first-class providers in the profile system — is the right call.
auriti
left a comment
There was a problem hiding this comment.
Thanks for the fix attempt here — the /provider loading/switching bug sounds real and worth fixing. I do see one concrete blocker in the current diff though.
-
src/services/api/providerConfig.ts
The new Gemini branch currently declares:const isGeminiMode = isEnvTruthy(process.env.CLAUDE_CODE_USE_MISTRAL)
That means the Gemini-specific request-resolution path is keyed off the Mistral flag instead of the Gemini flag. So the new
GEMINI_MODEL/GEMINI_BASE_URLlogic can activate under the wrong provider state, which makes this branch unsafe to merge as-is.I would expect that branch to read
CLAUDE_CODE_USE_GEMINIinstead.
Because this PR also changes provider/profile precedence in a few sensitive paths, I’d want that corrected and rechecked before going further.
fc69bfc to
bab8f21
Compare
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-review: PR #623 — Provider loading fix (head b3770c9)
CI green ✅. 9 files, +273/-46. Thanks for iterating on this, @lunamonke — the two previous hard blockers are fixed:
- ✅
isGeminiModenow correctly readsCLAUDE_CODE_USE_GEMINI - ✅ Unused
import providerfromcommands/provider/index.jsis gone
The core fix (making Gemini and Mistral first-class providers, writing startup profile on /provider switch) is the right direction. But there are a few remaining issues on the current head.
🔧 Blockers
1. Mistral and Gemini branches don't set API key from profile
In applyProviderProfileToProcessEnv(), the Mistral branch sets MISTRAL_BASE_URL and MISTRAL_MODEL but never sets MISTRAL_API_KEY from profile.apiKey. Same for Gemini — GEMINI_API_KEY is never set. The OpenAI and Anthropic branches both correctly set their API keys.
If a user saves an API key in their /provider profile and then switches to that profile, the key won't be applied to process.env, so subsequent API calls will fail with auth errors.
Fix:
// Mistral branch — add:
if (profile.apiKey) {
process.env.MISTRAL_API_KEY = profile.apiKey
} else {
delete process.env.MISTRAL_API_KEY
}
// Gemini branch — add:
if (profile.apiKey) {
process.env.GEMINI_API_KEY = profile.apiKey
} else {
delete process.env.GEMINI_API_KEY
}2. clearProviderProfileEnvFromProcessEnv doesn't clear Mistral/Gemini model vars
The clear function deletes all CLAUDE_CODE_USE_* flags and OpenAI/Anthropic URLs, but doesn't delete:
GEMINI_MODEL,GEMINI_BASE_URL,GEMINI_API_KEY,GEMINI_AUTH_MODE,GEMINI_ACCESS_TOKENMISTRAL_MODEL,MISTRAL_BASE_URL,MISTRAL_API_KEY
When switching providers via /provider, these stale vars remain in process.env. While they won't affect routing (because the USE_* flags are cleared correctly), they're messy and could cause confusion in debugging or if any code path reads them directly.
Fix — add to clearProviderProfileEnvFromProcessEnv:
delete processEnv.GEMINI_MODEL
delete processEnv.GEMINI_BASE_URL
delete processEnv.GEMINI_API_KEY
delete processEnv.GEMINI_AUTH_MODE
delete processEnv.GEMINI_ACCESS_TOKEN
delete processEnv.GOOGLE_API_KEY
delete processEnv.MISTRAL_MODEL
delete processEnv.MISTRAL_BASE_URL
delete processEnv.MISTRAL_API_KEY🟡 Non-blocking but important
3. buildLaunchEnv provider auto-detection — last-provider-wins if multiple flags set
The new loop:
for (let provider of PROVIDERS) {
if (provider === "anthropic") continue
const env_key_name = `CLAUDE_CODE_USE_${provider.toUpperCase()}`
if (env_key_name in processEnv && isEnvTruthy(processEnv[env_key_name])) {
options.profile = provider
}
}If both CLAUDE_CODE_USE_MISTRAL=1 and CLAUDE_CODE_USE_GEMINI=1 are set, the loop sets profile='mistral' then immediately overwrites to profile='gemini' (gemini is last in the array). This is nondeterministic behavior that depends on the PROVIDERS array order. Consider: logging a warning when multiple flags are set, or erroring, or taking the first match instead of last.
4. Behavioral change: shell env vars no longer win over persisted profile
The removal of the early return in buildStartupEnvFromProfile means users who set CLAUDE_CODE_USE_GEMINI=1 in their shell will now have their persisted profile override that on startup. This is the intended fix for the /provider settings not sticking, but it's a subtle semantic shift that could surprise users who expect shell env vars to always win.
The test name change from "treats explicit falsey provider flags as user intent" to the opposite assertion (profile wins over CLAUDE_CODE_USE_OPENAI='0') reflects this. Worth documenting the new precedence rules clearly — perhaps in a comment or in the PR description/release notes.
5. let provider should be const provider in the for-of loop
Minor: for (let provider of PROVIDERS) → for (const provider of PROVIDERS). The variable is never reassigned within the loop body (only options.profile is).
✅ What looks good
- Gemini and Mistral as first-class providers in
sanitizeProfile,isProcessEnvAlignedWithProfile,hasConflictingProviderFlagsForProfile setActiveProviderProfilenow writes the startup profile file — this is the core fix for/providersettings not persisting- New test coverage for Mistral and Gemini profile application in
providerProfiles.test.ts - Gemini context window entries for
gemini-3.1-proandgemini-3.1-flash-lite-preview hasExplicitProviderSelectionnow usesisEnvTruthyinstead of!== undefined— correctly meansCLAUDE_CODE_USE_OPENAI='0'is no longer treated as an explicit selection
Verdict: Needs changes 🔧
Two blockers: (1) Mistral and Gemini branches in applyProviderProfileToProcessEnv don't set API keys from the profile, (2) clearProviderProfileEnvFromProcessEnv doesn't clear Mistral/Gemini model vars, leaving stale env on provider switch. Both are straightforward fixes.
b3770c9 to
ad4248d
Compare
|
please fix conflicts @lunamonke |
a49bb32 to
d212d53
Compare
|
please have a look too @gnanam1990 @Vasanthdev2004 |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-review: PR #623 — Provider loading fix (head d212d53)
CI green ✅. 10 files, +385/-82. Thanks for iterating on this, @lunamonke — both previous hard blockers are now fixed:
- ✅
applyProviderProfileToProcessEnvnow setsMISTRAL_API_KEY/GEMINI_API_KEYfromprofile.apiKey(with properelse { delete }cleanup) - ✅
clearProviderProfileEnvFromProcessEnvnow clears all Gemini and Mistral env vars (GEMINI_MODEL,GEMINI_BASE_URL,GEMINI_API_KEY,GEMINI_AUTH_MODE,GEMINI_ACCESS_TOKEN,GOOGLE_API_KEY,MISTRAL_MODEL,MISTRAL_BASE_URL,MISTRAL_API_KEY)
The core fix is solid — Gemini and Mistral are now first-class providers in sanitizeProfile, isProcessEnvAlignedWithProfile, applyProviderProfileToProcessEnv, and clearProviderProfileEnvFromProcessEnv. Writing the startup profile file in setActiveProviderProfile is the right approach for persisting /provider selections across sessions.
🟡 Non-blocking suggestions
1. for (let provider of PROVIDERS) → for (const provider of PROVIDERS)
The loop variable is never reassigned; only options.profile is updated. const is more idiomatic and avoids suggesting mutation.
2. Behavioral change: shell env vars no longer win over persisted profile
The removal of the early return in buildStartupEnvFromProfile means users who set CLAUDE_CODE_USE_GEMINI=1 in their shell will now have their persisted /provider profile override that on startup. This is the intended fix, but it's a subtle semantic shift. Worth documenting in the PR description and/or release notes — users who relied on shell env vars taking priority will need to know the new precedence rules.
3. Anthropic → 'openai' mapping in setActiveProviderProfile is confusing but works
When activeProfile.provider === 'anthropic', the startup profile is saved as profile: 'openai' because buildLaunchEnv has no 'anthropic' branch. This works with the current code but is a maintenance footgun. Consider adding a comment explaining why, or adding an 'anthropic' alias in buildLaunchEnv.
4. Multiple provider flags → last-wins behavior in buildLaunchEnv
If both CLAUDE_CODE_USE_MISTRAL=1 and CLAUDE_CODE_USE_GEMINI=1 are set, the loop iterates through PROVIDERS array order and the last match wins (gemini in ['openai', 'anthropic', 'mistral', 'gemini']). This is deterministic but not obvious. Consider: warning when multiple flags are set, or taking the first match and breaking.
✅ What looks good
hasExplicitProviderSelectionnow usesisEnvTruthy()—CLAUDE_CODE_USE_OPENAI='0'is no longer treated as explicit selection. Correct.resolveProviderRequestnow falls through to Gemini model/base URL when in Gemini mode, matching the existing Mistral pattern. Consistent.ProviderManagersyncsmainLoopModelon provider switch — keeps the UI in sync. Good.- Gemini context windows for
gemini-3.1-proandgemini-3.1-flash-lite-previewadded. - New test coverage for Mistral and Gemini profile application in
providerProfiles.test.ts. RESTORED_KEYSin test now includes all Gemini/Mistral env vars for proper cleanup.- Profile file uses
mode: 0o600— API keys stored with restrictive permissions. Consistent with existing pattern.
Verdict: Approve-ready ✅
Both blockers from the previous review are fixed. The remaining items are non-blocking suggestions for maintainability and documentation. This is a meaningful improvement to provider switching.
81195ba
d212d53 to
81195ba
Compare
|
@kevincodex1 @Vasanthdev2004 @gnanam1990 @auriti if it's good to go, can we merge this? thank you for the reviews |
* add mistral and gemini provider type for profile provider field * load latest locally selected * env variables take precedence over json save * add gemini context windows and fix gemini defaulting for env * load on startup fix * fix failing tests * clarify test message * fix variable mismatches * fix failing test * delete keys and set profile.apiKey for mistral and gemini * switch model as well when switching provider * set model when adding a new model
Summary
Setting and editing provider via ui is broken (/provider). It doesn't set, and when closed and reopened, it doesn't load properly. This pr fixes that.
Before this change, after going through the initial setup screen, providers would mistakenly default to anthropic, and setting default and switching via the ui using /provider did not work properly. This change fixes that, loading and saving properly to the .openclaude-profile.json.
Also, when environment providers are set via CLAUDE_CODE_USE_GEMINI etc., now they take precedence.