From 4b9f597fa850a64bc8525431d449b2f6e978f6eb Mon Sep 17 00:00:00 2001 From: jinye Date: Thu, 18 Jun 2026 10:18:41 +0800 Subject: [PATCH] fix(model): remember selected provider when multiple share a model id (#5173) (#5179) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(model): disambiguate providers sharing a model id by baseUrl (#5173) When multiple modelProviders.openai entries share the same model id but have different baseUrl values, selecting one in the model picker was not remembered across restarts — startup always resolved to the first id match. The gap was at the persistence -> startup-resolution boundary: - persistModelSelection only saved model.name, dropping the baseUrl that distinguishes the providers. - resolveCliGenerationConfig matched providers by id alone (first hit). Fix (CLI only; runtime switching and ModelRegistry composite keys were already correct): - Add optional model.baseUrl setting paired with model.name. - ModelDialog persists the selected provider's baseUrl, and clears it when the selection has none (prevents a stale disambiguator). - /model is an id-only switch, so it clears model.baseUrl too. - resolveCliGenerationConfig disambiguates by the persisted baseUrl when the model comes from settings, falling back to the first id match when the paired provider was edited/removed or no baseUrl was saved. This keeps the persistence layer consistent with ModelRegistry's existing id+baseUrl composite key and syncAfterAuthRefresh's exact-match path. * fix(model): clear model.baseUrl on ACP setModel and provider install Round-1 review follow-up: extend the "any writer of model.name must set or clear model.baseUrl" invariant to the remaining id-only persistence paths so a stale disambiguator from a prior model-picker selection can't restore the wrong provider on next launch. - ACP Session.setModel clears model.baseUrl alongside model.name. - Provider install plan (core) clears model.baseUrl when selecting a model id. * fix(auth): disambiguate provider by baseUrl in pre-flight auth validation Round-2 review follow-up: validateAuthMethod / hasApiKeyForAuth looked up the provider by model id only, so for duplicate-id configs the auth and Anthropic baseUrl checks could validate the first provider's credentials instead of the selected one. - findModelConfig now prefers the exact id+baseUrl match and falls back to the first id match (mirrors resolveCliGenerationConfig). - New resolveSelectedModel helper pairs the resolved/persisted model id with its baseUrl (runtime generationConfig first, then settings.model.{name,baseUrl}). - Thread the baseUrl into both the API-key and Anthropic-baseUrl lookups. * fix(auth): pair model with its own baseUrl; document cross-scope assumption Round-3 review follow-up: - resolveSelectedModel no longer falls back to settings.model.baseUrl when a live Config is present, so a runtime-selected model is never paired with a stale persisted baseUrl from a previous selection. - Document the merged-settings assumption at the resolver disambiguation site: every writer pairs model.name + model.baseUrl in the same scope, and the id-only fallback bounds the blast radius if a hand-edited config desyncs them across scopes. * fix(model): use empty-string tombstone to clear model.baseUrl across scopes Round-4 review follow-up: clearing model.baseUrl by writing undefined drops the key from JSON, so a higher-priority scope's clear could not override a stale model.baseUrl left in a lower-priority scope — the stale disambiguator would resurface on merge and re-select the wrong duplicate provider after restart (e.g. pick a provider in user scope, then /model in a workspace that has its own modelProviders). All clear sites (model picker, /model, ACP setModel, provider install) now write an empty-string tombstone, which is a present value that overrides lower scopes on merge. The resolver and auth lookup already treat '' as "no disambiguator". * fix(acp): clear model.baseUrl when setCoreValue sets model.name Round-5 review follow-up: the generic qwen/settings/setCoreValue RPC (ACP/serve) can change model.name directly without touching model.baseUrl, leaving a stale disambiguator from a prior model-picker selection that would re-select the wrong duplicate provider on next launch. Clear it with the same empty-string tombstone used by the other id-only model.name writers. * fix(model): warn on silent provider fallback; pair baseUrl with resolved config Address review comments: - Emit a warning when the persisted model.baseUrl no longer matches any provider and resolution falls back to the first id match (thread 1 [Critical]). - Source effectiveBaseUrl from the resolved config (after?.baseUrl) rather than the picker entry alone, so the persisted (model.name, model.baseUrl) pair stays consistent even if switchModel transforms the id (thread 2 [Critical]). * test(model): cover effectiveBaseUrl fallback to picker entry baseUrl Add a ModelDialog test where switchModel succeeds but getContentGeneratorConfig returns a config without baseUrl, exercising the 'after?.baseUrl ?? selectedEntry?.model.baseUrl' fallback. Without coverage, a regression there would silently write an empty-string tombstone and resolve the wrong same-id provider on next launch. --- .../cli/src/acp-integration/acpAgent.test.ts | 22 +++ packages/cli/src/acp-integration/acpAgent.ts | 7 + .../acp-integration/session/Session.test.ts | 6 + .../src/acp-integration/session/Session.ts | 6 + packages/cli/src/config/auth.test.ts | 63 ++++++++ packages/cli/src/config/auth.ts | 70 +++++++-- packages/cli/src/config/settingsSchema.ts | 10 ++ .../cli/src/ui/commands/modelCommand.test.ts | 8 + packages/cli/src/ui/commands/modelCommand.ts | 7 + .../src/ui/components/ModelDialog.test.tsx | 121 +++++++++++++++ .../cli/src/ui/components/ModelDialog.tsx | 20 ++- .../cli/src/utils/modelConfigUtils.test.ts | 145 ++++++++++++++++++ packages/cli/src/utils/modelConfigUtils.ts | 41 ++++- .../src/providers/__tests__/install.test.ts | 3 + packages/core/src/providers/install.ts | 6 + .../schemas/settings.schema.json | 4 + 16 files changed, 525 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 4be9504afc..3a9c2bfffc 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -2520,6 +2520,28 @@ describe('QwenAgent MCP SSE/HTTP support', () => { await agentPromise; }); + it('qwen/settings/setCoreValue clears model.baseUrl when setting model.name', async () => { + const settings = makeCoreSettings(); + const { agent, agentPromise } = await bootCoreSettingsAgent(settings); + + await agent.extMethod('qwen/settings/setCoreValue', { + scope: 'user', + key: 'model.name', + value: 'qwen3.7-max', + }); + + expect(settings.setValue).toHaveBeenCalledWith( + 'User', + 'model.name', + 'qwen3.7-max', + ); + // Id-only selection must clear the paired baseUrl disambiguator (tombstone). + expect(settings.setValue).toHaveBeenCalledWith('User', 'model.baseUrl', ''); + + mockConnectionState.resolve(); + await agentPromise; + }); + it('qwen/settings/getCore excludes untrusted workspace integrations from merged view', async () => { const settings = makeCoreSettings(); (settings as { isTrusted: boolean }).isTrusted = false; diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 7a1b5e813c..6322bcf7f3 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -6335,6 +6335,13 @@ class QwenAgent implements Agent { ); const scope = toSettingsScope(params['scope']); settings.setValue(scope, key, normalizedValue); + if (settingKey === 'model.name') { + // Selecting a model by id here can't disambiguate providers that + // share that id, so clear the paired baseUrl disambiguator left by a + // previous model-picker selection. Empty-string tombstone overrides a + // lower-scope value on merge (undefined would be dropped from JSON). + settings.setValue(scope, 'model.baseUrl', ''); + } if ( settingKey === 'general.outputLanguage' && typeof normalizedValue === 'string' && diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index fcf34e0fff..c093c05f12 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -719,6 +719,12 @@ describe('Session', () => { 'model.name', 'qwen3-coder-plus', ); + // Id-only switch must clear any stale baseUrl disambiguator (tombstone). + expect(mockSettings.setValue).toHaveBeenCalledWith( + SettingScope.User, + 'model.baseUrl', + '', + ); expect(mockSettings.setValue).toHaveBeenCalledWith( SettingScope.User, 'security.auth.selectedType', diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 0b6f4f125b..8927ee6c10 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -2987,6 +2987,12 @@ export class Session implements SessionContext { if (options.persistDefault ?? true) { const persistScope = getPersistScopeForModelSelection(this.settings); this.settings.setValue(persistScope, 'model.name', parsed.modelId); + // Id-only switch: clear any baseUrl disambiguator left by a previous + // model-picker selection so the next launch resolves to this provider, + // not a stale one sharing the same model id. Empty-string tombstone so + // the clear overrides a lower-scope value on merge (undefined is dropped + // from JSON and would not override). + this.settings.setValue(persistScope, 'model.baseUrl', ''); this.settings.setValue( persistScope, 'security.auth.selectedType', diff --git a/packages/cli/src/config/auth.test.ts b/packages/cli/src/config/auth.test.ts index 32a9401fab..ccaa00bc26 100644 --- a/packages/cli/src/config/auth.test.ts +++ b/packages/cli/src/config/auth.test.ts @@ -34,6 +34,8 @@ describe('validateAuthMethod', () => { delete process.env['ANTHROPIC_API_KEY']; delete process.env['ANTHROPIC_BASE_URL']; delete process.env['GOOGLE_API_KEY']; + delete process.env['IDEALAB_KEY']; + delete process.env['TOKEN_PLAN_KEY']; }); it('should return null for USE_OPENAI with default env key', () => { @@ -75,6 +77,67 @@ describe('validateAuthMethod', () => { expect(validateAuthMethod(AuthType.USE_OPENAI)).toBeNull(); }); + it('disambiguates by settings.model.baseUrl when providers share a model id', () => { + // Two providers with the same id; the persisted baseUrl selects the second. + // Only the second provider's env key is set, so validation passes only if + // the lookup honors baseUrl rather than matching the first id entry. + vi.mocked(settings.loadSettings).mockReturnValue({ + merged: { + model: { + name: 'qwen3.7-max', + baseUrl: 'https://idealab.example.com/v1', + }, + modelProviders: { + openai: [ + { + id: 'qwen3.7-max', + baseUrl: 'https://token-plan.example.com/v1', + envKey: 'TOKEN_PLAN_KEY', + }, + { + id: 'qwen3.7-max', + baseUrl: 'https://idealab.example.com/v1', + envKey: 'IDEALAB_KEY', + }, + ], + }, + }, + } as unknown as ReturnType); + process.env['IDEALAB_KEY'] = 'idealab-key'; + + expect(validateAuthMethod(AuthType.USE_OPENAI)).toBeNull(); + }); + + it('reports the selected provider env key when providers share a model id', () => { + vi.mocked(settings.loadSettings).mockReturnValue({ + merged: { + model: { + name: 'qwen3.7-max', + baseUrl: 'https://idealab.example.com/v1', + }, + modelProviders: { + openai: [ + { + id: 'qwen3.7-max', + baseUrl: 'https://token-plan.example.com/v1', + envKey: 'TOKEN_PLAN_KEY', + }, + { + id: 'qwen3.7-max', + baseUrl: 'https://idealab.example.com/v1', + envKey: 'IDEALAB_KEY', + }, + ], + }, + }, + } as unknown as ReturnType); + + // No env keys set → error must name the selected (IdeaLab) provider's key. + const result = validateAuthMethod(AuthType.USE_OPENAI); + expect(result).toContain('IDEALAB_KEY'); + expect(result).not.toContain('TOKEN_PLAN_KEY'); + }); + it('should return error with custom envKey hint when modelProviders envKey is set but env var is missing', () => { vi.mocked(settings.loadSettings).mockReturnValue({ merged: { diff --git a/packages/cli/src/config/auth.ts b/packages/cli/src/config/auth.ts index f52334e322..1c121dd7bc 100644 --- a/packages/cli/src/config/auth.ts +++ b/packages/cli/src/config/auth.ts @@ -25,8 +25,11 @@ const DEFAULT_ENV_KEYS: Record = { /** * Find model configuration from modelProviders by authType and modelId. - * When multiple models share the same id (different baseUrls), returns the - * first match. Callers that need an exact match should also compare baseUrl. + * When a baseUrl is given, prefers the exact id+baseUrl match (disambiguating + * providers that share a model id) and falls back to the first id match if the + * paired provider was edited/removed. When no baseUrl is given, returns the + * first id match. Mirrors resolveCliGenerationConfig so pre-flight auth + * validation checks the same provider that startup resolution selects. */ function findModelConfig( modelProviders: ModelProvidersConfig | undefined, @@ -44,11 +47,42 @@ function findModelConfig( } if (baseUrl) { - return models.find((m) => m.id === modelId && m.baseUrl === baseUrl); + return ( + models.find((m) => m.id === modelId && m.baseUrl === baseUrl) ?? + models.find((m) => m.id === modelId) + ); } return models.find((m) => m.id === modelId); } +/** + * Resolve the selected model id and its paired baseUrl for provider lookup. + * Prefers the runtime-resolved generation config (which folds in CLI args, env + * vars, settings, and the selected provider), falling back to the persisted + * settings.model.{name,baseUrl} when no Config is available yet (pre-flight). + */ +function resolveSelectedModel( + settings: Settings, + config?: Config, +): { modelId: string | undefined; baseUrl: string | undefined } { + const modelsConfig = config?.getModelsConfig(); + if (modelsConfig) { + // A live Config is the source of truth: pair its model with its own + // resolved baseUrl. Do NOT fall back to settings.model.baseUrl here — that + // could pair the runtime-selected model with a stale persisted baseUrl from + // a previous selection and validate a different duplicate-id provider. + return { + modelId: modelsConfig.getModel(), + baseUrl: modelsConfig.getGenerationConfig()?.baseUrl, + }; + } + // Pre-flight (no Config yet): use the persisted selection as a paired unit. + return { + modelId: settings.model?.name, + baseUrl: settings.model?.baseUrl, + }; +} + function hasEnvValue(settings: Settings, envKey: string | undefined): boolean { if (!envKey) { return false; @@ -80,12 +114,19 @@ function hasApiKeyForAuth( | ModelProvidersConfig | undefined; - // Use config.getModelsConfig().getModel() if available for accurate model ID resolution - // that accounts for CLI args, env vars, and settings. Fall back to settings.model.name. - const modelId = config?.getModelsConfig().getModel() ?? settings.model?.name; + // Use config.getModelsConfig() if available for accurate model resolution + // that accounts for CLI args, env vars, and settings. Fall back to the + // persisted settings.model.{name,baseUrl}. + const { modelId, baseUrl } = resolveSelectedModel(settings, config); - // Try to find model-specific envKey from modelProviders - const modelConfig = findModelConfig(modelProviders, authType, modelId); + // Try to find model-specific envKey from modelProviders, disambiguating by + // baseUrl so duplicate-id providers resolve to the selected one. + const modelConfig = findModelConfig( + modelProviders, + authType, + modelId, + baseUrl, + ); // If a Config is available, prefer the API key already resolved into the // generation config. The unified resolver folds CLI flags (e.g. @@ -226,10 +267,15 @@ export function validateAuthMethod( const modelProviders = settings.merged.modelProviders as | ModelProvidersConfig | undefined; - // Use config.getModelsConfig().getModel() if available for accurate model ID - const modelId = - config?.getModelsConfig().getModel() ?? settings.merged.model?.name; - const modelConfig = findModelConfig(modelProviders, authMethod, modelId); + // Resolve the selected model + baseUrl so duplicate-id providers validate + // the Anthropic baseUrl of the selected provider, not the first id match. + const { modelId, baseUrl } = resolveSelectedModel(settings.merged, config); + const modelConfig = findModelConfig( + modelProviders, + authMethod, + modelId, + baseUrl, + ); if (modelConfig && !modelConfig.baseUrl) { return t( diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 4414488622..17cb08b544 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -1121,6 +1121,16 @@ const SETTINGS_SCHEMA = { description: 'The model to use for conversations.', showInDialog: false, }, + baseUrl: { + type: 'string', + label: 'Model Base URL', + category: 'Model', + requiresRestart: false, + default: undefined as string | undefined, + description: + 'Base URL paired with model.name; disambiguates which provider to use when multiple modelProviders entries share the same model id.', + showInDialog: false, + }, maxSessionTurns: { type: 'number', label: 'Max Session Turns', diff --git a/packages/cli/src/ui/commands/modelCommand.test.ts b/packages/cli/src/ui/commands/modelCommand.test.ts index 25eb181a88..82b6ac6899 100644 --- a/packages/cli/src/ui/commands/modelCommand.test.ts +++ b/packages/cli/src/ui/commands/modelCommand.test.ts @@ -183,6 +183,14 @@ describe('modelCommand', () => { 'model.name', 'qwen-max', ); + // `/model ` is an id-only switch, so any baseUrl disambiguator left by + // a previous model-picker selection must be cleared (empty-string tombstone) + // to avoid resolving to a different provider on next launch. + expect(setValue).toHaveBeenCalledWith( + expect.any(String), + 'model.baseUrl', + '', + ); expect(result).toEqual({ type: 'message', messageType: 'info', diff --git a/packages/cli/src/ui/commands/modelCommand.ts b/packages/cli/src/ui/commands/modelCommand.ts index 213c8b4048..a911d23b01 100644 --- a/packages/cli/src/ui/commands/modelCommand.ts +++ b/packages/cli/src/ui/commands/modelCommand.ts @@ -49,11 +49,18 @@ async function switchMainModel( ); persistSetting(settings, 'security.auth.selectedType', parsed.authType); persistSetting(settings, 'model.name', parsed.modelId); + // `/model ` selects by id only, so clear any baseUrl disambiguator left + // by a previous model-picker selection — otherwise next launch would + // resolve to a different provider than this switch just chose. Use an + // empty-string tombstone so the clear overrides a lower-scope value (an + // undefined write is dropped from JSON and would not override on merge). + persistSetting(settings, 'model.baseUrl', ''); return parsed.modelId; } await config.switchModel(currentAuthType, modelArg, undefined); persistSetting(settings, 'model.name', modelArg); + persistSetting(settings, 'model.baseUrl', ''); return modelArg; } diff --git a/packages/cli/src/ui/components/ModelDialog.test.tsx b/packages/cli/src/ui/components/ModelDialog.test.tsx index 0a1d192228..47369bfb98 100644 --- a/packages/cli/src/ui/components/ModelDialog.test.tsx +++ b/packages/cli/src/ui/components/ModelDialog.test.tsx @@ -308,6 +308,13 @@ describe('', () => { 'model.name', 'gpt-4', ); + // The selected provider has no baseUrl, so the disambiguator must be + // cleared with an empty-string tombstone (overrides any lower-scope value). + expect(mockSettings.setValue).toHaveBeenCalledWith( + SettingScope.User, + 'model.baseUrl', + '', + ); expect(mockSettings.setValue).toHaveBeenCalledWith( SettingScope.User, 'security.auth.selectedType', @@ -316,6 +323,120 @@ describe('', () => { expect(props.onClose).toHaveBeenCalledTimes(1); }); + it('persists model.baseUrl alongside model.name when the selected provider has a baseUrl', async () => { + const switchModel = vi.fn().mockResolvedValue(undefined); + const { props, mockSettings } = renderComponent({}, { + getModel: vi.fn(() => 'qwen3.7-max'), + getAuthType: vi.fn(() => AuthType.USE_OPENAI), + switchModel, + getAllConfiguredModels: vi.fn(() => [ + { + id: 'qwen3.7-max', + label: '[Token Plan] qwen3.7-max', + description: '', + authType: AuthType.USE_OPENAI, + baseUrl: 'https://token-plan.example.com/v1', + envKey: 'TOKEN_PLAN_KEY', + }, + { + id: 'qwen3.7-max', + label: '[IdeaLab] qwen3.7-max', + description: '', + authType: AuthType.USE_OPENAI, + baseUrl: 'https://idealab.example.com/v1', + envKey: 'IDEALAB_KEY', + }, + ]), + getContentGeneratorConfig: vi.fn(() => ({ + authType: AuthType.USE_OPENAI, + model: 'qwen3.7-max', + baseUrl: 'https://idealab.example.com/v1', + })), + } as unknown as Partial); + + const childOnSelect = mockedSelect.mock.calls[0][0].onSelect; + // Select the IdeaLab entry (second provider with the same id). + await childOnSelect( + `${AuthType.USE_OPENAI}::qwen3.7-max\0https://idealab.example.com/v1`, + ); + + expect(switchModel).toHaveBeenCalledWith( + AuthType.USE_OPENAI, + 'qwen3.7-max', + { + baseUrl: 'https://idealab.example.com/v1', + }, + ); + expect(mockSettings.setValue).toHaveBeenCalledWith( + SettingScope.User, + 'model.name', + 'qwen3.7-max', + ); + expect(mockSettings.setValue).toHaveBeenCalledWith( + SettingScope.User, + 'model.baseUrl', + 'https://idealab.example.com/v1', + ); + expect(props.onClose).toHaveBeenCalledTimes(1); + }); + + it('falls back to the picker entry baseUrl when switchModel does not propagate it', async () => { + // Regression guard for the `after?.baseUrl ?? selectedEntry?.model.baseUrl` + // fallback: if switchModel succeeds but getContentGeneratorConfig returns a + // config WITHOUT baseUrl, the disambiguator must still be persisted from the + // selected picker entry's baseUrl — otherwise an empty-string tombstone would + // be written and the wrong same-id provider would resolve on next launch. + const switchModel = vi.fn().mockResolvedValue(undefined); + const { props, mockSettings } = renderComponent({}, { + getModel: vi.fn(() => 'qwen3.7-max'), + getAuthType: vi.fn(() => AuthType.USE_OPENAI), + switchModel, + getAllConfiguredModels: vi.fn(() => [ + { + id: 'qwen3.7-max', + label: '[Token Plan] qwen3.7-max', + description: '', + authType: AuthType.USE_OPENAI, + baseUrl: 'https://token-plan.example.com/v1', + envKey: 'TOKEN_PLAN_KEY', + }, + { + id: 'qwen3.7-max', + label: '[IdeaLab] qwen3.7-max', + description: '', + authType: AuthType.USE_OPENAI, + baseUrl: 'https://idealab.example.com/v1', + envKey: 'IDEALAB_KEY', + }, + ]), + // Resolved config has NO baseUrl, so `after?.baseUrl` is undefined and the + // `?? selectedEntry?.model.baseUrl` fallback must supply the disambiguator. + getContentGeneratorConfig: vi.fn(() => ({ + authType: AuthType.USE_OPENAI, + model: 'qwen3.7-max', + })), + } as unknown as Partial); + + const childOnSelect = mockedSelect.mock.calls[0][0].onSelect; + // Select the IdeaLab entry (second provider with the same id). + await childOnSelect( + `${AuthType.USE_OPENAI}::qwen3.7-max\0https://idealab.example.com/v1`, + ); + + expect(mockSettings.setValue).toHaveBeenCalledWith( + SettingScope.User, + 'model.name', + 'qwen3.7-max', + ); + // baseUrl comes from the picker entry, not the (baseUrl-less) resolved config. + expect(mockSettings.setValue).toHaveBeenCalledWith( + SettingScope.User, + 'model.baseUrl', + 'https://idealab.example.com/v1', + ); + expect(props.onClose).toHaveBeenCalledTimes(1); + }); + it('shows MiniMax-M3 image + video modality and 1M context details', () => { const { getByText } = renderComponent({}, { getModel: vi.fn(() => 'MiniMax-M3'), diff --git a/packages/cli/src/ui/components/ModelDialog.tsx b/packages/cli/src/ui/components/ModelDialog.tsx index df53f7861d..3560c14a03 100644 --- a/packages/cli/src/ui/components/ModelDialog.tsx +++ b/packages/cli/src/ui/components/ModelDialog.tsx @@ -95,9 +95,16 @@ function maskApiKey(apiKey: string | undefined): string { function persistModelSelection( settings: ReturnType, modelId: string, + baseUrl?: string, ): void { const scope = getPersistScopeForModelSelection(settings); settings.setValue(scope, 'model.name', modelId); + // Persist the paired baseUrl so the correct provider is restored on next + // launch when multiple providers share the same model id. When the selection + // has no baseUrl, write an empty-string tombstone (not undefined): undefined + // is dropped from JSON, so it would not override a stale model.baseUrl left + // in a lower-priority scope, whereas '' is a present value that does. + settings.setValue(scope, 'model.baseUrl', baseUrl ?? ''); } function persistAuthTypeSelection( @@ -132,6 +139,7 @@ interface HandleModelSwitchSuccessParams { after: ContentGeneratorConfig | undefined; effectiveAuthType: AuthType | undefined; effectiveModelId: string; + effectiveBaseUrl: string | undefined; isRuntime: boolean; } @@ -141,9 +149,10 @@ function handleModelSwitchSuccess({ after, effectiveAuthType, effectiveModelId, + effectiveBaseUrl, isRuntime, }: HandleModelSwitchSuccessParams): void { - persistModelSelection(settings, effectiveModelId); + persistModelSelection(settings, effectiveModelId, effectiveBaseUrl); if (effectiveAuthType) { persistAuthTypeSelection(settings, effectiveAuthType); } @@ -553,6 +562,15 @@ export function ModelDialog({ after, effectiveAuthType, effectiveModelId, + // Persist the selected provider's baseUrl so the right provider is + // restored next launch when several share the same id. Pair it with the + // same resolved config that effectiveModelId comes from (`after`) so the + // persisted (model.name, model.baseUrl) stays consistent even if + // switchModel transforms the id; fall back to the picker entry's + // baseUrl. Runtime models are keyed by snapshot id, so no disambiguator. + effectiveBaseUrl: isRuntime + ? undefined + : (after?.baseUrl ?? selectedEntry?.model.baseUrl), isRuntime, }); onClose(); diff --git a/packages/cli/src/utils/modelConfigUtils.test.ts b/packages/cli/src/utils/modelConfigUtils.test.ts index f05d2c2da2..a8d5337cff 100644 --- a/packages/cli/src/utils/modelConfigUtils.test.ts +++ b/packages/cli/src/utils/modelConfigUtils.test.ts @@ -444,6 +444,151 @@ describe('modelConfigUtils', () => { ); }); + describe('provider disambiguation by settings.model.baseUrl', () => { + const tokenPlan: ProviderModelConfig = { + id: 'qwen3.7-max', + name: '[Token Plan] qwen3.7-max', + baseUrl: 'https://token-plan.example.com/v1', + envKey: 'TOKEN_PLAN_KEY', + }; + const ideaLab: ProviderModelConfig = { + id: 'qwen3.7-max', + name: '[IdeaLab] qwen3.7-max', + baseUrl: 'https://idealab.example.com/v1', + envKey: 'IDEALAB_KEY', + }; + + function mockResolved() { + vi.mocked(resolveModelConfig).mockReturnValue({ + config: { model: 'qwen3.7-max', apiKey: '', baseUrl: '' }, + sources: {}, + warnings: [], + }); + } + + it('selects the provider matching the persisted baseUrl, not the first id match', () => { + mockResolved(); + const settings = makeMockSettings({ + model: { + name: 'qwen3.7-max', + baseUrl: 'https://idealab.example.com/v1', + }, + modelProviders: { + [AuthType.USE_OPENAI]: [tokenPlan, ideaLab], + }, + }); + + resolveCliGenerationConfig({ + argv: {}, + settings, + selectedAuthType: AuthType.USE_OPENAI, + }); + + // The IdeaLab provider (not the first-listed Token Plan) must be passed + // to resolveModelConfig, which is what makes sources.baseUrl.kind become + // 'modelProviders' and lets syncAfterAuthRefresh keep it after refresh. + expect(vi.mocked(resolveModelConfig)).toHaveBeenCalledWith( + expect.objectContaining({ modelProvider: ideaLab }), + ); + }); + + it('falls back to the first id match when no baseUrl is persisted (backward compat)', () => { + mockResolved(); + const settings = makeMockSettings({ + model: { name: 'qwen3.7-max' }, + modelProviders: { + [AuthType.USE_OPENAI]: [tokenPlan, ideaLab], + }, + }); + + resolveCliGenerationConfig({ + argv: {}, + settings, + selectedAuthType: AuthType.USE_OPENAI, + }); + + expect(vi.mocked(resolveModelConfig)).toHaveBeenCalledWith( + expect.objectContaining({ modelProvider: tokenPlan }), + ); + }); + + it('falls back to the first id match and emits a warning when the persisted baseUrl no longer matches any provider', () => { + mockResolved(); + const settings = makeMockSettings({ + model: { + name: 'qwen3.7-max', + baseUrl: 'https://removed.example.com/v1', + }, + modelProviders: { + [AuthType.USE_OPENAI]: [tokenPlan, ideaLab], + }, + }); + + const result = resolveCliGenerationConfig({ + argv: {}, + settings, + selectedAuthType: AuthType.USE_OPENAI, + }); + + expect(vi.mocked(resolveModelConfig)).toHaveBeenCalledWith( + expect.objectContaining({ modelProvider: tokenPlan }), + ); + expect(result.warnings).toEqual( + expect.arrayContaining([ + expect.stringContaining('https://removed.example.com/v1'), + ]), + ); + }); + + it('treats an empty-string tombstone as no disambiguator (first id match)', () => { + // A cleared selection persists model.baseUrl: '' so it can override a + // stale lower-scope value on merge; the resolver must treat '' as "no + // baseUrl" and fall back to the first id match rather than matching a + // provider whose baseUrl is literally empty. + mockResolved(); + const settings = makeMockSettings({ + model: { name: 'qwen3.7-max', baseUrl: '' }, + modelProviders: { + [AuthType.USE_OPENAI]: [tokenPlan, ideaLab], + }, + }); + + resolveCliGenerationConfig({ + argv: {}, + settings, + selectedAuthType: AuthType.USE_OPENAI, + }); + + expect(vi.mocked(resolveModelConfig)).toHaveBeenCalledWith( + expect.objectContaining({ modelProvider: tokenPlan }), + ); + }); + + it('ignores the persisted baseUrl when the model comes from argv.model, not settings', () => { + mockResolved(); + const settings = makeMockSettings({ + model: { + name: 'something-else', + baseUrl: 'https://idealab.example.com/v1', + }, + modelProviders: { + [AuthType.USE_OPENAI]: [tokenPlan, ideaLab], + }, + }); + + resolveCliGenerationConfig({ + argv: { model: 'qwen3.7-max' }, + settings, + selectedAuthType: AuthType.USE_OPENAI, + }); + + // argv-resolved model uses id-only lookup → first match. + expect(vi.mocked(resolveModelConfig)).toHaveBeenCalledWith( + expect.objectContaining({ modelProvider: tokenPlan }), + ); + }); + }); + it('strips a runtime snapshot prefix from settings.model.name (read-side self-heal)', () => { const modelProvider: ProviderModelConfig = { id: 'gpt-4o', diff --git a/packages/cli/src/utils/modelConfigUtils.ts b/packages/cli/src/utils/modelConfigUtils.ts index 0a57dfa836..3f6b52903e 100644 --- a/packages/cli/src/utils/modelConfigUtils.ts +++ b/packages/cli/src/utils/modelConfigUtils.ts @@ -157,11 +157,15 @@ export function resolveCliGenerationConfig( // Env vars are ONLY considered when neither argv.model nor settings.model.name is set. let resolvedModel: string | undefined; let sourceEnvVar: string | undefined; + // Whether the model came from settings.model.name (vs argv/env). The persisted + // settings.model.baseUrl disambiguator only applies to this case. + let resolvedFromSettings = false; if (argv.model) { resolvedModel = argv.model; } else if (settings.model?.name) { // Self-heal configs already corrupted by older builds. resolvedModel = stripRuntimeSnapshotPrefix(settings.model.name); + resolvedFromSettings = true; } else if (authType && AUTH_ENV_MODEL_VARS[authType]) { // Only check env vars for the current auth type for (const envVar of AUTH_ENV_MODEL_VARS[authType]) { @@ -178,10 +182,44 @@ export function resolveCliGenerationConfig( // so the resolver correctly uses the settings-selected model (no override occurs). // The old candidate-loop code that fell through to OPENAI_MODEL is gone. let modelProvider: ProviderModelConfig | undefined; + let disambiguationWarning: string | undefined; if (resolvedModel && authType && settings.modelProviders) { const providers = settings.modelProviders[authType]; if (providers && Array.isArray(providers)) { - modelProvider = providers.find((p) => p.id === resolvedModel); + // When multiple providers share the same id, disambiguate by the + // persisted settings.model.baseUrl (written by the model picker). This + // only applies when the model itself came from settings.model.name. + // Fall back to the first id match if the paired provider was edited or + // removed (and for the legacy id-only case where no baseUrl was saved), + // mirroring auth.ts:findModelConfig. + // + // Note: `settings` is already merged across user/workspace/system scopes. + // Every writer of model.name (the picker, /model, ACP, provider install) + // also writes model.baseUrl in the SAME scope — a real URL, or an empty + // string tombstone when there is none. The tombstone matters because an + // omitted key cannot override a stale model.baseUrl in a lower-priority + // scope on merge, but '' (a present value) can. Empty string is treated + // as "no disambiguator" here. The only remaining desync is a hand-edited + // config that sets model.name in a higher scope with no baseUrl key at + // all; the id-only fallback bounds the blast radius to a same-id provider. + const persistedBaseUrl = settings.model?.baseUrl; + if (resolvedFromSettings && persistedBaseUrl) { + const exactMatch = providers.find( + (p) => p.id === resolvedModel && p.baseUrl === persistedBaseUrl, + ); + modelProvider = + exactMatch ?? providers.find((p) => p.id === resolvedModel); + // Surface the silent fallback: the paired provider was removed or its + // baseUrl changed, so traffic now routes to a different same-id provider. + if (!exactMatch && modelProvider) { + disambiguationWarning = + `Persisted model.baseUrl '${persistedBaseUrl}' no longer matches any provider ` + + `for model '${resolvedModel}' (authType '${authType}'); using the first id match ` + + `('${modelProvider.baseUrl ?? '(default baseUrl)'}'). Re-select the model to update it.`; + } + } else { + modelProvider = providers.find((p) => p.id === resolvedModel); + } } } @@ -270,6 +308,7 @@ export function resolveCliGenerationConfig( sources: resolved.sources, warnings: [ ...resolved.warnings, + ...(disambiguationWarning ? [disambiguationWarning] : []), ...(ignoredGenerationConfigWarning ? [ignoredGenerationConfigWarning] : []), diff --git a/packages/core/src/providers/__tests__/install.test.ts b/packages/core/src/providers/__tests__/install.test.ts index 96e9557281..0eaad09281 100644 --- a/packages/core/src/providers/__tests__/install.test.ts +++ b/packages/core/src/providers/__tests__/install.test.ts @@ -153,6 +153,9 @@ describe('applyProviderInstallPlan', () => { AuthType.USE_OPENAI, ); expect(adapter.setValue).toHaveBeenCalledWith('model.name', 'new-model'); + // Id-only model selection must clear any stale baseUrl disambiguator + // (empty-string tombstone overrides a lower-scope value on merge). + expect(adapter.setValue).toHaveBeenCalledWith('model.baseUrl', ''); expect(adapter.persist).toHaveBeenCalled(); expect(reloadModelProviders).toHaveBeenCalledWith({ [AuthType.USE_OPENAI]: [ diff --git a/packages/core/src/providers/install.ts b/packages/core/src/providers/install.ts index f1e86da348..37a0148705 100644 --- a/packages/core/src/providers/install.ts +++ b/packages/core/src/providers/install.ts @@ -210,6 +210,12 @@ export async function applyProviderInstallPlan( currentStep = 'modelSelection'; if (plan.modelSelection?.modelId) { settings.setValue('model.name', plan.modelSelection.modelId); + // The plan selects by model id only, so clear any baseUrl disambiguator + // left by a previous model-picker selection — otherwise the next launch + // could resolve to a stale provider sharing this model id. Empty-string + // tombstone so the clear overrides a lower-scope value on merge (an + // undefined write is dropped from JSON and would not override). + settings.setValue('model.baseUrl', ''); } // Provider state metadata diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index 4955eb5437..8c07f93780 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -474,6 +474,10 @@ "description": "The model to use for conversations.", "type": "string" }, + "baseUrl": { + "description": "Base URL paired with model.name; disambiguates which provider to use when multiple modelProviders entries share the same model id.", + "type": "string" + }, "maxSessionTurns": { "description": "Maximum number of user/model/tool turns to keep in a session. -1 means unlimited.", "type": "number",