From 8381316ab3e36a5e9c668394dc4b6705d0e90253 Mon Sep 17 00:00:00 2001 From: ComplexSimply Date: Mon, 10 Aug 2026 21:07:45 +0800 Subject: [PATCH 1/3] test(cli): pin the model selection across multi-provider template updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The worst case reported in #8863 was the batch path: with several providers updating in one confirmation, each executeUpdate in the loop rewrote model.name in turn and the last provider in registry order won. The #8868 gate protects it, but no test failed if that regressed — its regression tests cover a single updating provider. Pin the batch scenario: two stale providers, a current model owned by neither, one 'Update all' confirmation, and no model.name/model.baseUrl writes from either pass. Reverting the gate to the pre-#8868 form now fails this test alongside the two single-provider ones. --- .../src/ui/hooks/useProviderUpdates.test.ts | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/packages/cli/src/ui/hooks/useProviderUpdates.test.ts b/packages/cli/src/ui/hooks/useProviderUpdates.test.ts index 92cb22a7f4d..9e92d1d0734 100644 --- a/packages/cli/src/ui/hooks/useProviderUpdates.test.ts +++ b/packages/cli/src/ui/hooks/useProviderUpdates.test.ts @@ -560,6 +560,73 @@ describe('useProviderUpdates', () => { ); }); + it('leaves the model selection alone across a multi-provider batch update', async () => { + // The worst case reported in #8863: several providers update in one + // confirmation, and each executeUpdate in the loop used to rewrite + // model.name in turn — the last provider in registry order won, + // regardless of the user's intent. Neither provider owns the current + // model here, so the whole batch must leave the selection untouched. + const foreignModel = { + id: 'my-own-model', + baseUrl: 'https://my-own-gateway.example.com/v1', + envKey: 'MY_OWN_KEY', + name: '[Mine] my-own-model', + }; + mockConfig.getModel.mockReturnValue('my-own-model'); + mockConfig.getContentGeneratorConfig.mockReturnValue({ + authType: AuthType.USE_OPENAI, + baseUrl: foreignModel.baseUrl, + apiKeyEnvKey: foreignModel.envKey, + }); + 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]: [foreignModel, ...chinaTemplate, ...tokenTemplate], + }; + 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(); + }); + + expect(mockSettings.setValue).not.toHaveBeenCalledWith( + expect.anything(), + 'model.name', + expect.anything(), + ); + expect(mockSettings.setValue).not.toHaveBeenCalledWith( + expect.anything(), + 'model.baseUrl', + expect.anything(), + ); + expect(mockModelsConfig.syncAfterAuthRefresh).not.toHaveBeenCalled(); + }); + it('dismisses without persisting when user chooses "later"', async () => { (mockSettings.merged[PROVIDER_METADATA_NS] as Record)[ METADATA_KEY From c6da0275e25f97269569e174f37f5834984b11bd Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Mon, 10 Aug 2026 15:34:18 +0000 Subject: [PATCH 2/3] test(cli): verify both batch entries execute and pin the mixed-batch gate Co-authored-by: Qwen-Coder --- .../src/ui/hooks/useProviderUpdates.test.ts | 76 ++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/ui/hooks/useProviderUpdates.test.ts b/packages/cli/src/ui/hooks/useProviderUpdates.test.ts index a7f8d993891..448483d770d 100644 --- a/packages/cli/src/ui/hooks/useProviderUpdates.test.ts +++ b/packages/cli/src/ui/hooks/useProviderUpdates.test.ts @@ -612,7 +612,16 @@ describe('useProviderUpdates', () => { await result.current.providerUpdateRequest!.onConfirm('update'); await waitFor(() => { - expect(mockSettings.setValue).toHaveBeenCalled(); + expect(mockSettings.setValue).toHaveBeenCalledWith( + expect.anything(), + `${PROVIDER_METADATA_NS}.${METADATA_KEY}.version`, + chinaVersion, + ); + expect(mockSettings.setValue).toHaveBeenCalledWith( + expect.anything(), + `${PROVIDER_METADATA_NS}.${TOKEN_METADATA_KEY}.version`, + tokenVersion, + ); }); expect(mockSettings.setValue).not.toHaveBeenCalledWith( @@ -628,6 +637,71 @@ describe('useProviderUpdates', () => { expect(mockModelsConfig.syncAfterAuthRefresh).not.toHaveBeenCalled(); }); + it('migrates the model only for the active provider in a mixed batch', async () => { + // The model-selection gate must run per provider inside executeUpdate. + // If it were hoisted into the batch loop, the inactive Token Plan entry + // would keep its modelSelection and rewrite model.name a second time + // after the active Coding Plan entry migrated the selection — the exact + // #8863 symptom. + 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], + }; + // Default mock credentials point at Coding Plan, so only the first entry + // is the active provider. The current model exists in neither template, + // so the active entry must migrate the selection to its fallback model. + mockConfig.getModel.mockReturnValue('removed-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'); + + // Both entries ran to completion, regardless of provider order. + await waitFor(() => { + expect(mockConfig.reloadModelProvidersConfig).toHaveBeenCalledTimes(2); + }); + + const modelNameWrites = mockSettings.setValue.mock.calls.filter( + (call: unknown[]) => call[1] === 'model.name', + ); + expect(modelNameWrites).toEqual([ + [expect.anything(), 'model.name', chinaTemplate[0]!.id], + ]); + const modelBaseUrlWrites = mockSettings.setValue.mock.calls.filter( + (call: unknown[]) => call[1] === 'model.baseUrl', + ); + expect(modelBaseUrlWrites).toHaveLength(1); + expect(mockModelsConfig.syncAfterAuthRefresh).toHaveBeenCalledTimes(1); + expect(mockModelsConfig.syncAfterAuthRefresh).toHaveBeenCalledWith( + AuthType.USE_OPENAI, + chinaTemplate[0]!.id, + undefined, + ); + }); + it('persists a cooldown (not a full update) when user chooses "later"', async () => { (mockSettings.merged[PROVIDER_METADATA_NS] as Record)[ METADATA_KEY From f36e9e044f325a09eebfb262bd47a2d973934a03 Mon Sep 17 00:00:00 2001 From: ComplexSimply Date: Wed, 12 Aug 2026 11:14:02 +0800 Subject: [PATCH 3/3] test(cli): pin no-migration in the mixed batch after #8889 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #8889 made the template update path drop the plan's model selection unconditionally, so the mixed-batch test that expected the active provider to migrate now pins removed behavior. Invert it: a batch mixing the active provider (current model gone from its plan) with an inactive one must leave model.name and model.baseUrl untouched while both updates still run to completion — the batch-loop side of the invariant whose single-provider side #8889 pins itself. --- .../src/ui/hooks/useProviderUpdates.test.ts | 40 +++++++++---------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/packages/cli/src/ui/hooks/useProviderUpdates.test.ts b/packages/cli/src/ui/hooks/useProviderUpdates.test.ts index ada1d1a630c..d5ec09c1722 100644 --- a/packages/cli/src/ui/hooks/useProviderUpdates.test.ts +++ b/packages/cli/src/ui/hooks/useProviderUpdates.test.ts @@ -750,12 +750,14 @@ describe('useProviderUpdates', () => { expect(mockModelsConfig.syncAfterAuthRefresh).not.toHaveBeenCalled(); }); - it('migrates the model only for the active provider in a mixed batch', async () => { - // The model-selection gate must run per provider inside executeUpdate. - // If it were hoisted into the batch loop, the inactive Token Plan entry - // would keep its modelSelection and rewrite model.name a second time - // after the active Coding Plan entry migrated the selection — the exact - // #8863 symptom. + it('leaves the selection alone even for the active provider in a mixed batch', async () => { + // A batch mixing the ACTIVE provider (whose plan no longer offers the + // current model) with an inactive one: since #8889 a template update + // never applies the plan's model selection, so neither entry may touch + // model.name — while both updates still run to completion. This pins + // the batch-loop side of that invariant; the single-provider side is + // pinned by 'leaves the model selection alone when the previous model + // is gone' above. const metadataNs = mockSettings.merged[PROVIDER_METADATA_NS] as Record< string, unknown @@ -772,8 +774,7 @@ describe('useProviderUpdates', () => { [AuthType.USE_OPENAI]: [...chinaTemplate, ...tokenTemplate], }; // Default mock credentials point at Coding Plan, so only the first entry - // is the active provider. The current model exists in neither template, - // so the active entry must migrate the selection to its fallback model. + // is the active provider. The current model exists in neither template. mockConfig.getModel.mockReturnValue('removed-model'); mockConfig.refreshAuth.mockResolvedValue(undefined); @@ -797,22 +798,17 @@ describe('useProviderUpdates', () => { expect(mockConfig.reloadModelProvidersConfig).toHaveBeenCalledTimes(2); }); - const modelNameWrites = mockSettings.setValue.mock.calls.filter( - (call: unknown[]) => call[1] === 'model.name', - ); - expect(modelNameWrites).toEqual([ - [expect.anything(), 'model.name', chinaTemplate[0]!.id], - ]); - const modelBaseUrlWrites = mockSettings.setValue.mock.calls.filter( - (call: unknown[]) => call[1] === 'model.baseUrl', + expect(mockSettings.setValue).not.toHaveBeenCalledWith( + expect.anything(), + 'model.name', + expect.anything(), ); - expect(modelBaseUrlWrites).toHaveLength(1); - expect(mockModelsConfig.syncAfterAuthRefresh).toHaveBeenCalledTimes(1); - expect(mockModelsConfig.syncAfterAuthRefresh).toHaveBeenCalledWith( - AuthType.USE_OPENAI, - chinaTemplate[0]!.id, - undefined, + expect(mockSettings.setValue).not.toHaveBeenCalledWith( + expect.anything(), + 'model.baseUrl', + expect.anything(), ); + expect(mockModelsConfig.syncAfterAuthRefresh).not.toHaveBeenCalled(); }); it('persists a cooldown (not a full update) when user chooses "later"', async () => {