diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 9ad32f4bb93..07067905903 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -394,6 +394,45 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => ({ return undefined; }, ), + PROVIDER_METADATA_NS: 'providerMetadata', + resolveMetadataKey: vi.fn((provider: { id: string; models?: unknown[] }) => + provider.models ? provider.id : undefined, + ), + readRecordedBuiltinIds: vi.fn( + ( + metadataKey: string | undefined, + providerMetadata: Record | undefined, + ) => { + if (!metadataKey) return []; + const record = providerMetadata?.[metadataKey]; + const builtinIds = + record && typeof record === 'object' + ? (record as Record)['builtinIds'] + : undefined; + return Array.isArray(builtinIds) + ? builtinIds.filter((id): id is string => typeof id === 'string') + : []; + }, + ), + getStaleBuiltinIds: vi.fn( + ( + provider: { id: string; models?: Array<{ id: string }> }, + providerMetadata: Record | undefined, + ) => { + const metadataKey = provider.models ? provider.id : undefined; + if (!metadataKey) return []; + const record = providerMetadata?.[metadataKey]; + const builtinIds = + record && typeof record === 'object' + ? (record as Record)['builtinIds'] + : undefined; + const recorded = Array.isArray(builtinIds) + ? builtinIds.filter((id): id is string => typeof id === 'string') + : []; + const defaults = new Set(provider.models?.map((model) => model.id) ?? []); + return recorded.filter((id) => !defaults.has(id)); + }, + ), ExtensionManager: vi.fn().mockImplementation(() => ({ refreshCache: mockExtensionManagerState.refreshCache, getLoadedExtensions: vi.fn(() => mockExtensionManagerState.extensions), @@ -11143,6 +11182,68 @@ describe('QwenAgent MCP SSE/HTTP support', () => { await agentPromise; }); + it('qwen/providers/list excludes stale built-ins from existing modelIds', async () => { + const settings = { + ...makeSessionSettings(), + merged: { + mcpServers: {}, + env: { DEEPSEEK_API_KEY: 'sk-existing' }, + modelProviders: { + openai: [ + { + id: 'deepseek-chat', + baseUrl: 'https://api.deepseek.com/v1', + envKey: 'DEEPSEEK_API_KEY', + }, + { + id: 'deepseek-renamed-away', + baseUrl: 'https://api.deepseek.com/v1', + envKey: 'DEEPSEEK_API_KEY', + }, + { + id: 'my-custom-model', + baseUrl: 'https://api.deepseek.com/v1', + envKey: 'DEEPSEEK_API_KEY', + }, + ], + }, + providerMetadata: { + deepseek: { + version: 'old-version-hash', + baseUrl: 'https://api.deepseek.com', + builtinIds: ['deepseek-chat', 'deepseek-renamed-away'], + }, + }, + }, + } as unknown as LoadedSettings; + const agentPromise = runAcpAgent(mockConfig, settings, mockArgv); + + await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined()); + + const agent = capturedAgentFactory!({ + get closed() { + return mockConnectionState.promise; + }, + }) as AgentLike; + + const providers = await agent.extMethod('qwen/providers/list', {}); + expect(providers).toEqual({ + providers: [ + expect.objectContaining({ + id: 'deepseek', + existingConfig: expect.objectContaining({ + // The recorded built-in that is no longer a default must not be + // serialized — echoing it back on connect would reinstall it. + modelIds: ['deepseek-chat', 'my-custom-model'], + }), + }), + ], + }); + + mockConnectionState.resolve(); + await agentPromise; + }); + it('qwen/skills/install rejects http and non-GitHub source URLs', async () => { mockConfig.getSkillManager = vi.fn().mockReturnValue({ parseSkillContent: vi.fn(), diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 0401b7d5b15..f04b4d72a2b 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -24,6 +24,7 @@ import { getDefaultBaseUrlForProtocol, getDefaultModelIds, getScopedEnvContents, + getStaleBuiltinIds, QwenOAuth2Event, qwenOAuth2Events, resolveBaseUrl, @@ -44,6 +45,7 @@ import { MCPServerStatus, McpTransportPool, POOLED_TRANSPORTS_DEFAULT, + PROVIDER_METADATA_NS, INVOCATION_CONTEXT_META_KEY, PRIVATE_ACP_CAPABILITY_ENV, PRIVATE_PARENT_CAPABILITY_META_KEY, @@ -2084,13 +2086,32 @@ function readExistingProviderConfig( const advancedConfig = readExistingAdvancedConfig(firstModel); + // Same classification as the update flow (via getStaleBuiltinIds): ids + // recorded as built-in at the last install but no longer current defaults + // are stale built-ins — don't serialize them, or clients that echo modelIds + // back on connect would reinstall what the update cleans up. + const staleBuiltinIds = new Set( + getStaleBuiltinIds( + config, + (settings.merged as Record)[PROVIDER_METADATA_NS] as + | Record + | undefined, + ), + ); + return { protocol, baseUrl: sanitizeProviderBaseUrl(baseUrl), // Never serialize the raw secret over the ACP wire. Expose only whether a // key is stored; the client can omit `apiKey` on connect to keep it. ...(apiKey ? { hasApiKey: true } : {}), - ...(existing ? { modelIds: existing.models.map((model) => model.id) } : {}), + ...(existing + ? { + modelIds: existing.models + .map((model) => model.id) + .filter((id) => !staleBuiltinIds.has(id)), + } + : {}), ...(advancedConfig ? { advancedConfig } : {}), }; } diff --git a/packages/cli/src/ui/auth/AuthDialog.test.tsx b/packages/cli/src/ui/auth/AuthDialog.test.tsx index 901a178ac56..2d8c3a326e0 100644 --- a/packages/cli/src/ui/auth/AuthDialog.test.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.test.tsx @@ -1338,6 +1338,101 @@ describe('AuthDialog', { timeout: 15000 }, () => { }, ); + itWhenTuiInputReliable( + 'should not pre-fill stale built-in model IDs recorded in providerMetadata', + async () => { + // The saved model list holds a renamed-away built-in (recorded in + // providerMetadata.builtinIds but no longer a default) alongside a + // true custom model — only the custom one may be prefilled, or the + // wizard would reinstall what the update flow cleans up. + const savedSettings = { + security: { auth: { selectedType: undefined } }, + ui: { customThemes: {} }, + mcpServers: {}, + modelProviders: { + openai: [ + { + id: 'renamed-away-model', + name: '[ModelStudio Token Plan] renamed-away-model', + baseUrl: + 'https://token-plan.cn-beijing.maas.aliyuncs.com/compatible-mode/v1', + envKey: 'BAILIAN_TOKEN_PLAN_API_KEY', + }, + { + id: 'my-custom-token-model', + name: '[ModelStudio Token Plan] my-custom-token-model', + baseUrl: + 'https://token-plan.cn-beijing.maas.aliyuncs.com/compatible-mode/v1', + envKey: 'BAILIAN_TOKEN_PLAN_API_KEY', + }, + ], + }, + providerMetadata: { + 'token-plan': { + version: 'old-version-hash', + baseUrl: + 'https://token-plan.cn-beijing.maas.aliyuncs.com/compatible-mode/v1', + builtinIds: ['qwen3.7-plus', 'qwen3.7-max', 'renamed-away-model'], + }, + }, + } as unknown as Settings; + const settings: LoadedSettings = new LoadedSettings( + { + settings: { ui: { customThemes: {} }, mcpServers: {} }, + originalSettings: { ui: { customThemes: {} }, mcpServers: {} }, + path: '', + }, + { + settings: {}, + originalSettings: {}, + path: '', + }, + { + settings: savedSettings, + originalSettings: savedSettings, + path: '', + }, + { + settings: { ui: { customThemes: {} }, mcpServers: {} }, + originalSettings: { ui: { customThemes: {} }, mcpServers: {} }, + path: '', + }, + true, + new Set(), + ); + + const { stdin, lastFrame, unmount } = renderAuthDialog(settings); + + await waitForSelectedOption(lastFrame, 'Alibaba ModelStudio'); + stdin.write('\r'); + await waitForSelectedOption(lastFrame, 'Coding Plan'); + await moveDownAndWaitForSelection(stdin, lastFrame, 'Token Plan'); + await pressEnterAndWaitFor( + stdin, + lastFrame, + 'Alibaba ModelStudio · Step 1/3 · Region', + ); + await pressEnterAndWaitFor( + stdin, + lastFrame, + 'Alibaba ModelStudio · Step 2/3 · API Key', + ); + + await typeText(stdin, 'sk-token-plan'); + + await pressEnterAndWaitFor( + stdin, + lastFrame, + 'Alibaba ModelStudio · Step 3/3 · Model IDs', + ); + + expect(lastFrame()).toContain('my-custom-token-model'); + expect(lastFrame()).not.toContain('renamed-away-model'); + + unmount(); + }, + ); + itWhenTuiInputReliable( 'should return from Token Plan API key input to Token Plan selection', async () => { diff --git a/packages/cli/src/ui/auth/AuthDialog.tsx b/packages/cli/src/ui/auth/AuthDialog.tsx index f5543b22793..6726cd965c5 100644 --- a/packages/cli/src/ui/auth/AuthDialog.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.tsx @@ -21,6 +21,8 @@ import { findProviderByCredentials, findExistingProviderModels, getDefaultModelIds, + getStaleBuiltinIds, + PROVIDER_METADATA_NS, customProvider, ALIBABA_PROVIDERS, THIRD_PARTY_PROVIDERS, @@ -180,7 +182,21 @@ export function AuthDialog(): React.JSX.Element { ); if (!saved) return []; const builtinIds = new Set(getDefaultModelIds(providerConfig)); - return saved.models.map((m) => m.id).filter((id) => !builtinIds.has(id)); + // Same classification as the update flow (via getStaleBuiltinIds): ids + // recorded as built-in at the last install but no longer current defaults + // are stale built-ins, not user customs — don't prefill what the update + // cleans up. + const staleBuiltinIds = new Set( + getStaleBuiltinIds( + providerConfig, + (settings.merged as Record)[PROVIDER_METADATA_NS] as + | Record + | undefined, + ), + ); + return saved.models + .map((m) => m.id) + .filter((id) => !builtinIds.has(id) && !staleBuiltinIds.has(id)); }; const handleProviderSelect = (providerId: string) => { diff --git a/packages/cli/src/ui/hooks/useProviderUpdates.test.ts b/packages/cli/src/ui/hooks/useProviderUpdates.test.ts index 92cb22a7f4d..7694daa591d 100644 --- a/packages/cli/src/ui/hooks/useProviderUpdates.test.ts +++ b/packages/cli/src/ui/hooks/useProviderUpdates.test.ts @@ -191,6 +191,48 @@ describe('useProviderUpdates', () => { expect(entry?.diff.currentModelAffected).toBe(false); }); + it('shows stale built-in removals in the prompt diff before confirming', async () => { + const staleBuiltinId = 'renamed-away-model'; + mockConfig.getModel.mockReturnValue(staleBuiltinId); + (mockSettings.merged[PROVIDER_METADATA_NS] as Record)[ + METADATA_KEY + ] = { + baseUrl: CODING_PLAN_CHINA_BASE_URL, + version: 'old-version-hash', + builtinIds: [...chinaTemplate.map((m) => m.id), staleBuiltinId], + }; + mockSettings.merged['modelProviders'] = { + [AuthType.USE_OPENAI]: [ + ...chinaTemplate, + { + id: staleBuiltinId, + baseUrl: CODING_PLAN_CHINA_BASE_URL, + envKey: CODING_PLAN_ENV_KEY, + name: `[Coding Plan] ${staleBuiltinId}`, + }, + ], + }; + + const { result } = renderHook(() => + useProviderUpdates( + mockSettings as never, + mockConfig as never, + mockAddItem, + ), + ); + + await waitFor(() => { + expect(result.current.providerUpdateRequest).toBeDefined(); + }); + + const entry = result.current.providerUpdateRequest?.entries[0]; + // Exactly the stale built-in — user customs and current defaults must + // not leak into the removal preview. + expect(entry?.diff.removed).toEqual([staleBuiltinId]); + expect(entry?.diff.currentModelAffected).toBe(true); + expect(entry?.diff.fallbackModel).toBe(chinaTemplate[0]!.id); + }); + it('detects newly added built-in models when the template grows', async () => { // Simulate an older install that lacks the last built-in model. const olderTemplate = chinaTemplate.slice(0, -1); @@ -218,7 +260,11 @@ describe('useProviderUpdates', () => { }); const entry = result.current.providerUpdateRequest?.entries[0]; - expect(entry?.diff.added).toContain(addedModelId); + // Exactly the one new model — the already-installed current defaults must + // NOT appear as "added" (a legacy install records no builtinIds, so the + // comparison set must still include the current defaults). + expect(entry?.diff.added).toEqual([addedModelId]); + expect(entry?.diff.removed).toEqual([]); }); it('preserves user-added custom models when executing an update', async () => { @@ -265,6 +311,74 @@ describe('useProviderUpdates', () => { ); }); + it('drops stale renamed built-ins but keeps user-added custom models on update', async () => { + const customModel = { + id: 'my-custom-model', + baseUrl: CODING_PLAN_CHINA_BASE_URL, + envKey: CODING_PLAN_ENV_KEY, + name: '[Coding Plan] my-custom-model', + }; + // A model that was built-in at install time but was renamed/removed + // upstream since — it must be cleaned up, unlike true user customs. + const staleBuiltinId = 'renamed-away-model'; + (mockSettings.merged[PROVIDER_METADATA_NS] as Record)[ + METADATA_KEY + ] = { + baseUrl: CODING_PLAN_CHINA_BASE_URL, + version: 'old-version-hash', + builtinIds: [...chinaTemplate.map((m) => m.id), staleBuiltinId], + }; + mockSettings.merged['modelProviders'] = { + [AuthType.USE_OPENAI]: [ + ...chinaTemplate, + { + id: staleBuiltinId, + baseUrl: CODING_PLAN_CHINA_BASE_URL, + envKey: CODING_PLAN_ENV_KEY, + name: `[Coding Plan] ${staleBuiltinId}`, + }, + customModel, + ], + }; + 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(mockConfig.reloadModelProvidersConfig).toHaveBeenCalled(); + }); + + const reloaded = mockConfig.reloadModelProvidersConfig.mock.calls[0][0]; + const ids = reloaded[AuthType.USE_OPENAI].map((m: { id: string }) => m.id); + // Exact list, not membership: catches both the stale id surviving and + // any default being carried through as a pseudo-custom and duplicated. + expect(ids).toEqual([...chinaTemplate.map((m) => m.id), 'my-custom-model']); + expect(mockSettings.setValue).toHaveBeenCalledWith( + expect.anything(), + `${PROVIDER_METADATA_NS}.${METADATA_KEY}.version`, + chinaVersion, + ); + expect(mockAddItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'info', + text: expect.stringContaining(staleBuiltinId), + }), + expect.anything(), + ); + }); + it('executes update when user confirms with "update"', async () => { (mockSettings.merged[PROVIDER_METADATA_NS] as Record)[ METADATA_KEY diff --git a/packages/cli/src/ui/hooks/useProviderUpdates.ts b/packages/cli/src/ui/hooks/useProviderUpdates.ts index 9ed6bb16a0b..74d94dbc8e3 100644 --- a/packages/cli/src/ui/hooks/useProviderUpdates.ts +++ b/packages/cli/src/ui/hooks/useProviderUpdates.ts @@ -14,11 +14,12 @@ import { ALL_PROVIDERS, applyProviderInstallPlan, buildInstallPlan, - buildProviderTemplate, - computeModelListVersion, + computeProviderTemplateVersion, getDefaultModelIds, + getStaleBuiltinIds, PROVIDER_METADATA_NS, providerMatchesCredentials, + readRecordedBuiltinIds, resolveBaseUrl, resolveMetadataKey, resolveOwnsModel, @@ -59,6 +60,7 @@ interface ProviderMetadata { version?: string; baseUrl?: string; ignoredVersion?: string; + builtinIds?: string[]; } function getProviderMetadata( @@ -71,9 +73,13 @@ function getProviderMetadata( | undefined; if (!ns) return {}; const metadata = ns[metadataKey]; - return metadata && typeof metadata === 'object' - ? (metadata as ProviderMetadata) - : {}; + const record = + metadata && typeof metadata === 'object' + ? (metadata as ProviderMetadata) + : {}; + // Settings are hand-editable — validate builtinIds before any removal + // decision relies on it. + return { ...record, builtinIds: readRecordedBuiltinIds(metadataKey, ns) }; } // --------------------------------------------------------------------------- @@ -174,11 +180,19 @@ function readInstalledOwnedIds( function getInstalledOwnedModelIds( settings: LoadedSettings, provider: ProviderConfig, + staleBuiltinIds: string[], ): string[] { // Only compare built-in model IDs — user-added custom models should not // appear as "removed" in the diff since they were never part of the - // provider's built-in list. - const builtinIds = new Set(getDefaultModelIds(provider)); + // provider's built-in list. Stale built-ins (recorded at the last install + // but dropped since) stay in the comparison so the removals the update + // performs show up in the preview and the two never disagree. The set + // defaults ∪ stale equals defaults ∪ recorded, because every recorded id + // is either still a default or stale. + const builtinIds = new Set([ + ...getDefaultModelIds(provider), + ...staleBuiltinIds, + ]); return readInstalledOwnedIds(settings, provider).filter((id) => builtinIds.has(id), ); @@ -189,6 +203,10 @@ function findAllPendingUpdates( currentModel: string, ): PendingUpdate[] { const results: PendingUpdate[] = []; + const mergedSettings = settings.merged as Record; + const providerMetadataNs = mergedSettings[PROVIDER_METADATA_NS] as + | Record + | undefined; for (const provider of ALL_PROVIDERS) { const metadataKey = resolveMetadataKey(provider); if (!metadataKey) continue; @@ -197,13 +215,16 @@ function findAllPendingUpdates( if (!metadata.version) continue; const baseUrl = metadata.baseUrl || resolveBaseUrl(provider); - const currentTemplate = buildProviderTemplate(provider, baseUrl); - const currentVersion = computeModelListVersion(currentTemplate); + const currentVersion = computeProviderTemplateVersion(provider, baseUrl); if (metadata.version === currentVersion) continue; if (metadata.ignoredVersion === currentVersion) continue; - const existingModelIds = getInstalledOwnedModelIds(settings, provider); + const existingModelIds = getInstalledOwnedModelIds( + settings, + provider, + getStaleBuiltinIds(provider, providerMetadataNs), + ); const newModelIds = provider.models!.map((s) => s.id); const diff = computeModelDiff(existingModelIds, newModelIds, currentModel); @@ -240,9 +261,31 @@ export function useProviderUpdates( // An update only refreshes built-in models — user-added custom IDs // must be carried through so they are not deleted by the // prepend-and-remove-owned merge. + // + // Distinguish true user-custom models from stale built-ins that were + // renamed/removed upstream (e.g. deepseek-v4-flash → -0731). A model + // is "custom" only if it is not a current default AND was not part of + // the built-in list at the last install (metadata.builtinIds). Stale + // built-ins are dropped so they get cleaned up instead of persisting + // forever. Installs predating builtinIds recording fall back to [] + // and keep already-stale ids — they cannot be safely distinguished + // from user customs. That keep is permanent: this update re-records + // builtinIds as the current defaults only, so carried ids are + // classified as user customs on every future update as well. const defaultIds = getDefaultModelIds(providerCfg); - const customIds = readInstalledOwnedIds(settings, providerCfg).filter( - (id) => !defaultIds.includes(id), + const mergedSettings = settings.merged as Record; + const staleBuiltinIds = getStaleBuiltinIds( + providerCfg, + mergedSettings[PROVIDER_METADATA_NS] as + | Record + | undefined, + ); + const ownedIds = readInstalledOwnedIds(settings, providerCfg); + const customIds = ownedIds.filter( + (id) => !defaultIds.includes(id) && !staleBuiltinIds.includes(id), + ); + const droppedStaleBuiltinIds = ownedIds.filter((id) => + staleBuiltinIds.includes(id), ); const installPlan = buildInstallPlan(providerCfg, { baseUrl: resolved, @@ -315,6 +358,18 @@ export function useProviderUpdates( ); } + if (droppedStaleBuiltinIds.length > 0) { + addItem( + { + type: 'info', + text: t('Removed outdated built-in models: {{ids}}', { + ids: droppedStaleBuiltinIds.join(', '), + }), + }, + Date.now(), + ); + } + addItem( { type: 'info', diff --git a/packages/core/src/providers/__tests__/install.test.ts b/packages/core/src/providers/__tests__/install.test.ts index 7a09f6c5965..2f32240c26f 100644 --- a/packages/core/src/providers/__tests__/install.test.ts +++ b/packages/core/src/providers/__tests__/install.test.ts @@ -10,8 +10,13 @@ import type { ModelProvidersConfig } from '../../models/types.js'; import { applyProviderInstallPlan, buildInstallPlan, + buildProviderTemplate, + computeModelListVersion, customProvider, + deepseekProvider, generateCustomEnvKey, + getDefaultModelIds, + PROVIDER_METADATA_NS, ProviderInstallError, type ProviderInstallPlan, type ProviderSettingsAdapter, @@ -415,6 +420,40 @@ describe('applyProviderInstallPlan', () => { expect(adapter.setValue).toHaveBeenCalledWith('codingPlan.version', 'v1'); }); + it('writes plan-builder-recorded builtinIds/version through the install path', async () => { + // Derive the plan from the real plan builder (rather than injecting a + // providerState fixture) so this gates resolveProviderState's recording + // end-to-end through applyProviderInstallPlan: a regression that stops + // attaching builtinIds/version makes this fail. Carrying a custom model + // makes it discriminating — the stored version must come from the + // default-only template and builtinIds must exclude the custom id. + const adapter = createAdapter(); + const baseUrl = 'https://api.deepseek.com'; + const plan = buildInstallPlan(deepseekProvider, { + baseUrl, + apiKey: 'sk-deepseek', + modelIds: [...getDefaultModelIds(deepseekProvider), 'my-custom-model'], + }); + + await applyProviderInstallPlan(plan, { settings: adapter }); + + const prefix = `${PROVIDER_METADATA_NS}.${deepseekProvider.id}`; + expect(adapter.setValue).toHaveBeenCalledWith( + `${prefix}.builtinIds`, + getDefaultModelIds(deepseekProvider), + ); + expect(adapter.setValue).toHaveBeenCalledWith( + `${prefix}.version`, + computeModelListVersion(buildProviderTemplate(deepseekProvider, baseUrl)), + ); + expect(adapter.setValue).toHaveBeenCalledWith(`${prefix}.baseUrl`, baseUrl); + // The custom model is carried into the written model list… + const writtenModels = adapter.setValue.mock.calls.find( + ([key]) => key === `modelProviders.${AuthType.USE_OPENAI}`, + )?.[1] as Array<{ id: string }> | undefined; + expect(writtenModels?.map((m) => m.id)).toContain('my-custom-model'); + }); + it('appends models with append merge strategy', async () => { const adapter = createAdapter({ [AuthType.USE_OPENAI]: [ diff --git a/packages/core/src/providers/__tests__/presets/alibaba-coding-plan.test.ts b/packages/core/src/providers/__tests__/presets/alibaba-coding-plan.test.ts index 876cb1b522d..f795ba168c6 100644 --- a/packages/core/src/providers/__tests__/presets/alibaba-coding-plan.test.ts +++ b/packages/core/src/providers/__tests__/presets/alibaba-coding-plan.test.ts @@ -54,6 +54,7 @@ describe('coding plan provider', () => { 'providerMetadata.coding-plan': { baseUrl: CODING_PLAN_CHINA_BASE_URL, version, + builtinIds: getDefaultModelIds(codingPlanProvider), }, }); }); diff --git a/packages/core/src/providers/__tests__/presets/alibaba-token-plan.test.ts b/packages/core/src/providers/__tests__/presets/alibaba-token-plan.test.ts index 869c9b53545..c6d0d4562af 100644 --- a/packages/core/src/providers/__tests__/presets/alibaba-token-plan.test.ts +++ b/packages/core/src/providers/__tests__/presets/alibaba-token-plan.test.ts @@ -99,6 +99,7 @@ describe('token plan provider', () => { 'providerMetadata.token-plan': { baseUrl: TOKEN_PLAN_CHINA_BASE_URL, version, + builtinIds: getDefaultModelIds(tokenPlanProvider), }, }); }); @@ -136,6 +137,7 @@ describe('token plan provider', () => { 'providerMetadata.token-plan': { baseUrl: TOKEN_PLAN_GLOBAL_BASE_URL, version, + builtinIds: getDefaultModelIds(tokenPlanProvider), }, }); }); diff --git a/packages/core/src/providers/__tests__/provider-config.test.ts b/packages/core/src/providers/__tests__/provider-config.test.ts index 9c2ff332ec0..3001126a2ae 100644 --- a/packages/core/src/providers/__tests__/provider-config.test.ts +++ b/packages/core/src/providers/__tests__/provider-config.test.ts @@ -440,6 +440,40 @@ describe('findExistingProviderModels', () => { }); }); +describe('readRecordedBuiltinIds (via source import)', () => { + it('returns the recorded builtinIds for a known metadata key', () => { + expect( + readRecordedBuiltinIdsSrc('coding-plan', { + 'coding-plan': { version: 'v1', builtinIds: ['a', 'b'] }, + }), + ).toEqual(['a', 'b']); + }); + + it('returns [] when the key is missing or undefined', () => { + expect(readRecordedBuiltinIdsSrc(undefined, {})).toEqual([]); + expect(readRecordedBuiltinIdsSrc('coding-plan', { other: {} })).toEqual([]); + expect(readRecordedBuiltinIdsSrc('coding-plan', undefined)).toEqual([]); + }); + + it('returns [] for malformed builtinIds values', () => { + expect( + readRecordedBuiltinIdsSrc('coding-plan', { + 'coding-plan': { builtinIds: 'not-an-array' }, + }), + ).toEqual([]); + expect( + readRecordedBuiltinIdsSrc('coding-plan', { + 'coding-plan': { builtinIds: ['a', 42, null, 'b'] }, + }), + ).toEqual(['a', 'b']); + expect( + readRecordedBuiltinIdsSrc('coding-plan', { + 'coding-plan': 'scalar-record', + }), + ).toEqual([]); + }); +}); + describe('shouldShowStep', () => { it('shows protocol step only when multiple options', () => { const single = makeConfig({ @@ -676,6 +710,12 @@ import { import { resolveBaseUrl as resolveBaseUrlSrc, providerMatchesCredentials as providerMatchesCredentialsSrc, + readRecordedBuiltinIds as readRecordedBuiltinIdsSrc, + buildInstallPlan as buildInstallPlanSrc, + buildProviderTemplate as buildProviderTemplateSrc, + computeModelListVersion as computeModelListVersionSrc, + getDefaultModelIds as getDefaultModelIdsSrc, + getStaleBuiltinIds as getStaleBuiltinIdsSrc, } from '../provider-config.js'; describe('resolveBaseUrl edge cases', () => { @@ -916,3 +956,72 @@ describe('resolveMetadataKey dotted-id guard', () => { expect(() => resolveMetadataKeySrc(config)).toThrow(/must not contain/); }); }); + +// Pins the headline fix: the stored version must be computed from the +// built-in template (default models only), NOT from the final model list, so +// that carrying user-added custom models through an install/update does not +// make the stored version diverge from the launch-time recomputation. This +// only discriminates when modelIds != defaults (an editable provider with an +// extra custom id); with modelIds == defaults the pre-fix computation hashes +// to the same value. +describe('stored providerState version is template-derived (headline fix)', () => { + it('records version from the default-only template, excluding custom ids from version and builtinIds', () => { + const config = makeConfig({ modelsEditable: true }); + const baseUrl = 'https://api.test.com/v1'; + const plan = buildInstallPlanSrc(config, { + baseUrl, + apiKey: 'sk-test', + modelIds: [...getDefaultModelIdsSrc(config), 'my-custom-model'], + }); + + const templateVersion = computeModelListVersionSrc( + buildProviderTemplateSrc(config, baseUrl), + ); + const state = plan.providerState?.[`providerMetadata.${config.id}`]; + expect(state).toEqual({ + version: templateVersion, + baseUrl, + builtinIds: getDefaultModelIdsSrc(config), + }); + + // The custom model is carried into the model list… + expect(plan.modelProviders?.[0]?.models.map((m) => m.id)).toContain( + 'my-custom-model', + ); + // …but must not leak into the stored version or the built-in snapshot: + // the stored version differs from a hash over the final model list (which + // includes the custom model), and builtinIds excludes the custom id. + expect(state?.['version']).not.toBe( + computeModelListVersionSrc(plan.modelProviders?.[0]?.models ?? []), + ); + expect(state?.['builtinIds'] as string[]).not.toContain('my-custom-model'); + }); +}); + +describe('getStaleBuiltinIds', () => { + it('returns recorded ids that are no longer current defaults', () => { + const config = makeConfig({ models: [{ id: 'a' }, { id: 'b' }] }); + const metadata = { + [config.id]: { builtinIds: ['a', 'b', 'renamed-away'] }, + }; + expect(getStaleBuiltinIdsSrc(config, metadata)).toEqual(['renamed-away']); + }); + + it('returns [] when nothing is recorded', () => { + const config = makeConfig({ models: [{ id: 'a' }] }); + expect(getStaleBuiltinIdsSrc(config, undefined)).toEqual([]); + expect(getStaleBuiltinIdsSrc(config, {})).toEqual([]); + }); + + it('returns [] when all recorded ids are still current defaults', () => { + const config = makeConfig({ models: [{ id: 'a' }, { id: 'b' }] }); + const metadata = { [config.id]: { builtinIds: ['a', 'b'] } }; + expect(getStaleBuiltinIdsSrc(config, metadata)).toEqual([]); + }); + + it('returns [] for providers without static models', () => { + const config = makeConfig({ id: 'custom-like', models: undefined }); + const metadata = { 'custom-like': { builtinIds: ['x'] } }; + expect(getStaleBuiltinIdsSrc(config, metadata)).toEqual([]); + }); +}); diff --git a/packages/core/src/providers/index.ts b/packages/core/src/providers/index.ts index fbe784bae52..e87404ac62e 100644 --- a/packages/core/src/providers/index.ts +++ b/packages/core/src/providers/index.ts @@ -23,11 +23,14 @@ export { buildInstallPlan, buildProviderTemplate, computeModelListVersion, + computeProviderTemplateVersion, findExistingProviderModels, getDefaultBaseUrlForProtocol, getDefaultModelIds, + getStaleBuiltinIds, providerMatchesCredentials, PROVIDER_METADATA_NS, + readRecordedBuiltinIds, resolveBaseUrl, resolveMetadataKey, resolveOwnsModel, diff --git a/packages/core/src/providers/provider-config.ts b/packages/core/src/providers/provider-config.ts index 88577e24b77..2ee8bd31fc7 100644 --- a/packages/core/src/providers/provider-config.ts +++ b/packages/core/src/providers/provider-config.ts @@ -229,14 +229,24 @@ export const PROVIDER_METADATA_NS = 'providerMetadata'; function resolveProviderState( config: ProviderConfig, baseUrl: string, - models: ProviderModelConfig[], ): ProviderInstallState | undefined { const key = resolveMetadataKey(config); if (key) { return { [`${PROVIDER_METADATA_NS}.${key}`]: { - version: computeModelListVersion(models), + // The stored version must be computed from the same input the + // launch-time check uses — the built-in template (default models + // only) — so that user-added custom models (editable providers) + // don't make the stored version permanently diverge from the + // recomputed currentVersion. Otherwise "update all" would never + // clear the prompt for users with custom models (see #8829 + // follow-up). + version: computeProviderTemplateVersion(config, baseUrl), baseUrl, + // Record the built-in ids at install time so a later update can tell + // stale built-ins (renamed/removed upstream) apart from true + // user-custom models and clean the former up. + builtinIds: getDefaultModelIds(config), }, }; } @@ -287,7 +297,7 @@ export function buildInstallPlan( ...(ownsModel ? { ownsModel } : {}), }, ], - providerState: resolveProviderState(config, inputs.baseUrl, models), + providerState: resolveProviderState(config, inputs.baseUrl), }; } @@ -299,6 +309,23 @@ export function computeModelListVersion(models: ProviderModelConfig[]): string { return createHash('sha256').update(JSON.stringify(models)).digest('hex'); } +/** + * Version of a provider's built-in template (default models only). + * + * Single owned function for the invariant this codebase relies on: the + * version stored at install time ({@link resolveProviderState}) and the + * currentVersion recomputed at launch (useProviderUpdates) must hash the + * same input, or the "update available" prompt never clears. Call sites + * must use this instead of composing buildProviderTemplate + + * computeModelListVersion by hand. + */ +export function computeProviderTemplateVersion( + config: ProviderConfig, + baseUrl: string, +): string { + return computeModelListVersion(buildProviderTemplate(config, baseUrl)); +} + /** * Default base URLs per protocol, used as placeholder/fallback when the user * doesn't supply one for a custom provider. Kept in core so the CLI flow @@ -400,6 +427,46 @@ export function findExistingProviderModels( return undefined; } +/** + * Read the built-in model ids recorded at the last install from the raw + * `providerMetadata` namespace. Returns [] when nothing was recorded (e.g. + * installs predating the recording) or the value is malformed. + */ +export function readRecordedBuiltinIds( + metadataKey: string | undefined, + providerMetadata: Record | undefined, +): string[] { + if (!metadataKey) return []; + const record = providerMetadata?.[metadataKey]; + const builtinIds = + record && typeof record === 'object' + ? (record as Record)['builtinIds'] + : undefined; + return Array.isArray(builtinIds) + ? builtinIds.filter((id): id is string => typeof id === 'string') + : []; +} + +/** + * Single source of truth for the stale-built-in classification: ids recorded + * as built-in at the last install that are no longer current defaults + * (renamed/removed upstream). Every entry point that decides whether an + * installed id is a user custom or a cleanup target (the update-confirm + * filter, the update-preview diff, the /auth wizard prefill, the ACP + * providers/list serialization) must derive from this function so the + * classification cannot drift between copies. + */ +export function getStaleBuiltinIds( + config: ProviderConfig, + providerMetadata: Record | undefined, +): string[] { + const defaults = new Set(getDefaultModelIds(config)); + return readRecordedBuiltinIds( + resolveMetadataKey(config), + providerMetadata, + ).filter((id) => !defaults.has(id)); +} + // --------------------------------------------------------------------------- // Check if a step should be shown in the UI // --------------------------------------------------------------------------- diff --git a/packages/core/src/providers/types.ts b/packages/core/src/providers/types.ts index aac55346515..fd0d4cf3fcd 100644 --- a/packages/core/src/providers/types.ts +++ b/packages/core/src/providers/types.ts @@ -158,7 +158,10 @@ export interface ProviderModelProvidersPatch { * Arbitrary key-value metadata to persist alongside a provider install. * Each top-level key becomes a settings path prefix (e.g. `codingPlan.version`). */ -export type ProviderInstallState = Record>; +export type ProviderInstallState = Record< + string, + Record +>; export interface ProviderInstallPlan { providerId: ProviderId; diff --git a/packages/vscode-ide-companion/src/services/settingsWriter.test.ts b/packages/vscode-ide-companion/src/services/settingsWriter.test.ts index 4bb70b1f5d0..2f10a8033bf 100644 --- a/packages/vscode-ide-companion/src/services/settingsWriter.test.ts +++ b/packages/vscode-ide-companion/src/services/settingsWriter.test.ts @@ -26,7 +26,11 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { }); import { AuthType, type ProviderInstallPlan } from '@qwen-code/qwen-code-core'; -import { CODING_PLAN_ENV_KEY } from './subscriptionPlanDefinitions.js'; +import { + CODING_PLAN_ENV_KEY, + CodingPlanRegion, + getSubscriptionPlanConfig, +} from './subscriptionPlanDefinitions.js'; import { applyProviderInstallPlanToFile, clearPersistedAuth, @@ -132,6 +136,53 @@ describe('settingsWriter', () => { expect(openaiModels.some((m) => m.id === 'user-model')).toBe(true); }); + it('refreshes template-derived providerMetadata fields when re-syncing coding plan settings', () => { + // This sync replaces every Coding Plan model with the region template, so + // version/baseUrl/builtinIds must be recomputed from that template — not + // carried over from a prior install (possibly a different region). Only + // unrelated fields the CLI owns, such as ignoredVersion, are preserved. + // Seed a GLOBAL-region snapshot, then write china, so the refresh of + // baseUrl/builtinIds is observable (they differ between regions). + const globalConfig = getSubscriptionPlanConfig( + 'coding', + CodingPlanRegion.GLOBAL, + ); + const chinaConfig = getSubscriptionPlanConfig( + 'coding', + CodingPlanRegion.CHINA, + ); + fs.mkdirSync(path.dirname(settingsPath), { recursive: true }); + fs.writeFileSync( + settingsPath, + JSON.stringify({ + providerMetadata: { + 'coding-plan': { + version: globalConfig.version, + region: CodingPlanRegion.GLOBAL, + baseUrl: globalConfig.baseUrl, + builtinIds: ['model-renamed-away'], + ignoredVersion: 'skipped-version', + }, + }, + }), + 'utf-8', + ); + + writeCodingPlanConfig('china', 'coding-plan-key'); + + const settings = JSON.parse( + fs.readFileSync(settingsPath, 'utf-8'), + ) as Record; + const metadata = ( + settings.providerMetadata as Record> + )['coding-plan']; + expect(metadata.region).toBe('china'); + expect(metadata.version).toBe(chinaConfig.version); + expect(metadata.baseUrl).toBe(chinaConfig.baseUrl); + expect(metadata.builtinIds).toEqual(chinaConfig.template.map((m) => m.id)); + expect(metadata.ignoredVersion).toBe('skipped-version'); + }); + it('reads an api-key configuration after switching away from coding plan', () => { writeCodingPlanConfig('china', 'coding-plan-key'); diff --git a/packages/vscode-ide-companion/src/services/settingsWriter.ts b/packages/vscode-ide-companion/src/services/settingsWriter.ts index cf998be536f..243bf3cc793 100644 --- a/packages/vscode-ide-companion/src/services/settingsWriter.ts +++ b/packages/vscode-ide-companion/src/services/settingsWriter.ts @@ -342,11 +342,27 @@ export function writeCodingPlanConfig( providers[AuthType.USE_OPENAI] = [...planModels, ...nonCodingPlan]; // Coding Plan metadata — write to the providerMetadata namespace that - // the CLI now reads from. Remove legacy top-level key if present. + // the CLI now reads from. Remove legacy top-level key if present. Spread + // the existing entry only to keep fields this sync doesn't own (e.g. + // ignoredVersion). Every field derived from the template being written — + // version, baseUrl, builtinIds — must be recomputed from `planConfig`, + // not carried over: this function just replaced every Coding Plan model + // with `planConfig.template`, so a stale baseUrl/builtinIds snapshot from + // a previous region or template would make the CLI's next update hash the + // wrong template and could silently revert a region switch. const providerMetadata = ensureNestedObject(settings, 'providerMetadata'); + const existingMetadata = + providerMetadata['coding-plan'] && + typeof providerMetadata['coding-plan'] === 'object' && + !Array.isArray(providerMetadata['coding-plan']) + ? (providerMetadata['coding-plan'] as Record) + : {}; providerMetadata['coding-plan'] = { + ...existingMetadata, region: codingRegion, + baseUrl: planConfig.baseUrl, version: planConfig.version, + builtinIds: planConfig.template.map((m) => m.id), }; delete settings.codingPlan;