From 7698cba803eec8bea52f94e4af7b7086f70c4ed8 Mon Sep 17 00:00:00 2001 From: ComplexSimply Date: Mon, 10 Aug 2026 20:16:05 +0800 Subject: [PATCH] fix(cli): keep the user's model selection intact on provider template updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'Update all' in the built-in provider update dialog rewrote model.name to the provider's first built-in model and cleared model.baseUrl whenever the current model was not one of that provider's models — exactly the case where the update has no business touching it (#8863, a #5819 regression left open by the #5835 guard, which only matches models the plan itself offers). With several providers updating in one confirmation, each rewrote model.name in turn and the last one in registry order won. A template update is not first-time setup and carries no model-selection intent: executeUpdate now drops the plan's modelSelection unconditionally. Models the provider owns are already carried into the plan (including removed built-ins the user still sits on), so the previous conditional delete was a no-op in every legitimate case and live only in the buggy one. The toast now reports a switch only when the active model actually changed, instead of predicting one from the plan. --- .../src/ui/hooks/useProviderUpdates.test.ts | 177 +++++++++++++++++- .../cli/src/ui/hooks/useProviderUpdates.ts | 16 +- 2 files changed, 180 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/ui/hooks/useProviderUpdates.test.ts b/packages/cli/src/ui/hooks/useProviderUpdates.test.ts index c2a593bf602..e124b41fbe5 100644 --- a/packages/cli/src/ui/hooks/useProviderUpdates.test.ts +++ b/packages/cli/src/ui/hooks/useProviderUpdates.test.ts @@ -417,7 +417,13 @@ describe('useProviderUpdates', () => { expect(process.env[CODING_PLAN_ENV_KEY]).toBe('sk-sp-existing-key'); }); - it('switches model when previous model is no longer available', async () => { + it('does not adopt the provider default when the previous model is gone from the plan', async () => { + // Inverted from the pre-#8863 'switches model when previous model is no + // longer available': the update path never applies the plan's model + // selection, even when the current model is absent from the refreshed + // list — a removed built-in the user still sits on is carried into the + // plan as a custom entry, so an absent model here means it was never + // this provider's to migrate. mockConfig.getModel.mockReturnValue('removed-model'); (mockSettings.merged[PROVIDER_METADATA_NS] as Record)[ METADATA_KEY @@ -448,11 +454,172 @@ describe('useProviderUpdates', () => { expect(mockSettings.setValue).toHaveBeenCalled(); }); - expect(mockModelsConfig.syncAfterAuthRefresh).toHaveBeenCalledWith( - AuthType.USE_OPENAI, - 'qwen3.5-plus', - undefined, + expect(mockModelsConfig.syncAfterAuthRefresh).not.toHaveBeenCalled(); + const modelWrites = mockSettings.setValue.mock.calls.filter( + (call: unknown[]) => + call[1] === 'model.name' || call[1] === 'model.baseUrl', + ); + expect(modelWrites).toEqual([]); + }); + + it('does not rewrite the model selection when the current model belongs to another provider', async () => { + // #8863: a template update carries no model-selection intent. When the + // user's current model is not one of this provider's models (e.g. a + // self-hosted gateway entry), the update must not move them onto this + // provider's first built-in model and must not clear model.baseUrl. + mockConfig.getModel.mockReturnValue('my-own-model'); + (mockSettings.merged[PROVIDER_METADATA_NS] as Record)[ + METADATA_KEY + ] = { + baseUrl: CODING_PLAN_CHINA_BASE_URL, + version: 'old-version-hash', + }; + mockSettings.merged['modelProviders'] = { + [AuthType.USE_OPENAI]: [ + ...chinaTemplate, + { + id: 'my-own-model', + baseUrl: 'https://my-own-gateway.example.com/v1', + envKey: 'MY_OWN_KEY', + name: '[Mine] my-own-model', + }, + ], + }; + mockConfig.refreshAuth.mockResolvedValue(undefined); + + const { result } = renderHook(() => + useProviderUpdates( + mockSettings as never, + mockConfig as never, + mockAddItem, + ), + ); + + await waitFor(() => { + expect(result.current.providerUpdateRequest).toBeDefined(); + }); + + await result.current.providerUpdateRequest!.onConfirm('update'); + + await waitFor(() => { + expect(mockSettings.setValue).toHaveBeenCalled(); + }); + + const modelWrites = mockSettings.setValue.mock.calls.filter( + (call: unknown[]) => + call[1] === 'model.name' || call[1] === 'model.baseUrl', + ); + expect(modelWrites).toEqual([]); + expect(mockModelsConfig.syncAfterAuthRefresh).not.toHaveBeenCalled(); + }); + + it('does not claim a model switch in the update toast when nothing changed', async () => { + // #8863 symptom 2: the toast used to read the unchanged runtime model + // and announce 'Model switched to ""' while the overwrite + // landed on disk. With the selection untouched there is no switch to + // report. + mockConfig.getModel.mockReturnValue('my-own-model'); + (mockSettings.merged[PROVIDER_METADATA_NS] as Record)[ + METADATA_KEY + ] = { + baseUrl: CODING_PLAN_CHINA_BASE_URL, + version: 'old-version-hash', + }; + mockSettings.merged['modelProviders'] = { + [AuthType.USE_OPENAI]: [ + ...chinaTemplate, + { + id: 'my-own-model', + baseUrl: 'https://my-own-gateway.example.com/v1', + envKey: 'MY_OWN_KEY', + name: '[Mine] my-own-model', + }, + ], + }; + mockConfig.refreshAuth.mockResolvedValue(undefined); + + const { result } = renderHook(() => + useProviderUpdates( + mockSettings as never, + mockConfig as never, + mockAddItem, + ), + ); + + await waitFor(() => { + expect(result.current.providerUpdateRequest).toBeDefined(); + }); + + await result.current.providerUpdateRequest!.onConfirm('update'); + + await waitFor(() => { + expect(mockAddItem).toHaveBeenCalled(); + }); + + const switchToasts = mockAddItem.mock.calls.filter( + (call: unknown[]) => + typeof (call[0] as { text?: string })?.text === 'string' && + (call[0] as { text: string }).text.includes('Model switched'), + ); + expect(switchToasts).toEqual([]); + }); + + it('leaves the model selection alone across a multi-provider batch update', async () => { + // #8863 worst case: several providers update in one confirmation and + // each executeUpdate used to rewrite model.name in turn, the last + // provider in registry order winning. None of them may touch it. + mockConfig.getModel.mockReturnValue('my-own-model'); + const metadataNs = mockSettings.merged[PROVIDER_METADATA_NS] as Record< + string, + unknown + >; + metadataNs[METADATA_KEY] = { + baseUrl: CODING_PLAN_CHINA_BASE_URL, + version: 'old-version-hash', + }; + metadataNs[TOKEN_METADATA_KEY] = { + baseUrl: TOKEN_PLAN_BASE_URL, + version: 'old-version-hash', + }; + mockSettings.merged['modelProviders'] = { + [AuthType.USE_OPENAI]: [ + ...chinaTemplate, + ...tokenTemplate, + { + id: 'my-own-model', + baseUrl: 'https://my-own-gateway.example.com/v1', + envKey: 'MY_OWN_KEY', + name: '[Mine] my-own-model', + }, + ], + }; + mockConfig.refreshAuth.mockResolvedValue(undefined); + + const { result } = renderHook(() => + useProviderUpdates( + mockSettings as never, + mockConfig as never, + mockAddItem, + ), ); + + await waitFor(() => { + expect(result.current.providerUpdateRequest).toBeDefined(); + }); + expect(result.current.providerUpdateRequest!.entries.length).toBe(2); + + await result.current.providerUpdateRequest!.onConfirm('update'); + + await waitFor(() => { + expect(mockSettings.setValue).toHaveBeenCalled(); + }); + + const modelWrites = mockSettings.setValue.mock.calls.filter( + (call: unknown[]) => + call[1] === 'model.name' || call[1] === 'model.baseUrl', + ); + expect(modelWrites).toEqual([]); + expect(mockModelsConfig.syncAfterAuthRefresh).not.toHaveBeenCalled(); }); it('dismisses without persisting when user chooses "later"', async () => { diff --git a/packages/cli/src/ui/hooks/useProviderUpdates.ts b/packages/cli/src/ui/hooks/useProviderUpdates.ts index ec38cf8063b..e5439d9b9ce 100644 --- a/packages/cli/src/ui/hooks/useProviderUpdates.ts +++ b/packages/cli/src/ui/hooks/useProviderUpdates.ts @@ -251,13 +251,13 @@ export function useProviderUpdates( }); delete installPlan.env; const previousModel = config.getModel(); - const newConfigs = installPlan.modelProviders?.[0]?.models ?? []; - const previousModelStillAvailable = newConfigs.some( - (cfg) => cfg.id === previousModel, - ); - if (previousModelStillAvailable) { - delete installPlan.modelSelection; - } + // A template update is not first-time setup and carries no + // model-selection intent: never let the refreshed plan move the + // user off their current model or clear model.baseUrl. Models this + // provider owns are already carried into the plan above (including + // removed built-ins the user still sits on), and a model owned by + // another provider is not this update's to touch (#8863, #5819). + delete installPlan.modelSelection; const activeConfig = config.getContentGeneratorConfig(); const updatesActiveProvider = activeConfig?.authType === providerCfg.protocol && @@ -282,7 +282,7 @@ export function useProviderUpdates( const activeModel = config.getModel(); const displayName = t(providerCfg.label); - if (previousModelStillAvailable && activeModel === previousModel) { + if (activeModel === previousModel) { addItem( { type: 'info',