Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 50 additions & 20 deletions packages/cli/src/ui/hooks/useProviderUpdates.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
AuthType,
CODING_PLAN_CHINA_BASE_URL,
CODING_PLAN_ENV_KEY,
CODING_PLAN_GLOBAL_BASE_URL,
codingPlanProvider,
TOKEN_PLAN_BASE_URL,
TOKEN_PLAN_ENV_KEY,
Expand Down Expand Up @@ -126,6 +127,32 @@ describe('useProviderUpdates', () => {
expect(result.current.providerUpdateRequest).toBeUndefined();
});

it('uses the stored non-default base URL when versions match', () => {
const globalTemplate = buildProviderTemplate(
codingPlanProvider,
CODING_PLAN_GLOBAL_BASE_URL,
);
(mockSettings.merged[PROVIDER_METADATA_NS] as Record<string, unknown>)[
METADATA_KEY
] = {
baseUrl: CODING_PLAN_GLOBAL_BASE_URL,
version: computeModelListVersion(globalTemplate),
};
mockSettings.merged['modelProviders'] = {
[AuthType.USE_OPENAI]: globalTemplate,
};

const { result } = renderHook(() =>
useProviderUpdates(
mockSettings as never,
mockConfig as never,
mockAddItem,
),
);

expect(result.current.providerUpdateRequest).toBeUndefined();
});

it('shows update prompt with structured diff when versions differ', async () => {
(mockSettings.merged[PROVIDER_METADATA_NS] as Record<string, unknown>)[
METADATA_KEY
Expand Down Expand Up @@ -222,7 +249,7 @@ describe('useProviderUpdates', () => {
expect(entry?.diff.added).toContain(addedModelId);
});

it('preserves user-added custom models when executing an update', async () => {
it('persists the template version and preserves custom models', async () => {
const customModel = {
id: 'my-custom-model',
baseUrl: CODING_PLAN_CHINA_BASE_URL,
Expand Down Expand Up @@ -264,6 +291,11 @@ describe('useProviderUpdates', () => {
expect.objectContaining({ id: 'my-custom-model' }),
]),
);
expect(mockSettings.setValue).toHaveBeenCalledWith(
expect.anything(),
`${PROVIDER_METADATA_NS}.${METADATA_KEY}.version`,
chinaVersion,
);
});

it('executes update when user confirms with "update"', async () => {
Expand Down Expand Up @@ -303,11 +335,6 @@ describe('useProviderUpdates', () => {
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}.${METADATA_KEY}.baseUrl`,
Expand Down Expand Up @@ -444,14 +471,11 @@ 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 () => {
let activeModel = 'removed-model';
mockConfig.getModel.mockImplementation(() => activeModel);
mockModelsConfig.syncAfterAuthRefresh.mockImplementation(
(_authType, modelId) => {
activeModel = modelId;
},
);
it('leaves the model selection alone when the previous model is gone', async () => {
// Template updates do not carry a model-selection intent; even when the
// current model is absent from the refreshed list the update must not
// adopt the provider's default or touch model.name / model.baseUrl.
mockConfig.getModel.mockReturnValue('removed-model');
(mockSettings.merged[PROVIDER_METADATA_NS] as Record<string, unknown>)[
METADATA_KEY
] = {
Expand All @@ -478,18 +502,24 @@ describe('useProviderUpdates', () => {
await result.current.providerUpdateRequest!.onConfirm('update');

await waitFor(() => {
expect(mockSettings.setValue).toHaveBeenCalled();
expect(mockConfig.reloadModelProvidersConfig).toHaveBeenCalled();
});

expect(mockModelsConfig.syncAfterAuthRefresh).toHaveBeenCalledWith(
AuthType.USE_OPENAI,
'qwen3.5-plus',
undefined,
expect(mockModelsConfig.syncAfterAuthRefresh).not.toHaveBeenCalled();
expect(mockSettings.setValue).not.toHaveBeenCalledWith(
expect.anything(),
'model.name',
expect.anything(),
);
expect(mockSettings.setValue).not.toHaveBeenCalledWith(
expect.anything(),
'model.baseUrl',
expect.anything(),
);
expect(mockAddItem).toHaveBeenCalledWith(
{
type: 'info',
text: 'Coding Plan configuration updated successfully. Model switched to "qwen3.5-plus".',
text: 'Coding Plan configuration updated successfully.',
},
expect.any(Number),
);
Expand Down
25 changes: 12 additions & 13 deletions packages/cli/src/ui/hooks/useProviderUpdates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,9 @@ function findAllPendingUpdates(
if (!metadata.version) continue;

const baseUrl = metadata.baseUrl || resolveBaseUrl(provider);
const currentTemplate = buildProviderTemplate(provider, baseUrl);
const currentVersion = computeModelListVersion(currentTemplate);
const currentVersion = computeModelListVersion(
buildProviderTemplate(provider, baseUrl),
);

if (metadata.version === currentVersion) continue;
if (metadata.ignoredVersion === currentVersion) continue;
Expand Down Expand Up @@ -255,9 +256,10 @@ export function useProviderUpdates(
const migrated = useRef(false);

const executeUpdate = useCallback(
async (providerCfg: ProviderConfig, baseUrl?: string) => {
async (pending: PendingUpdate) => {
try {
const resolved = resolveBaseUrl(providerCfg, baseUrl);
const providerCfg = pending.provider;
const resolved = resolveBaseUrl(providerCfg, pending.baseUrl);
// 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.
Expand All @@ -270,7 +272,12 @@ export function useProviderUpdates(
apiKey: '',
modelIds: [...defaultIds, ...customIds],
});
installPlan.providerState![
`${PROVIDER_METADATA_NS}.${pending.metadataKey}`
]!['version'] = pending.currentVersion;
delete installPlan.env;
// Template updates never change the selected model.
delete installPlan.modelSelection;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] The prompt still promises a model switch this line prevents

Deleting modelSelection makes applyProviderInstallPlan skip the model-selection block (core/src/providers/install.ts:236-271) and never call syncAuthState (install.ts:288-295) — which 92ca1f4f now correctly asserts.

However packages/cli/src/ui/components/ProviderUpdatePrompt.tsx:99-102 still shows, before the user confirms:

Note: Your selected model is being removed. It will switch to "{{model}}" after update.

using diff.fallbackModel (newModelIds[0], computed at line 151 of this file). After this change that never happens:

  1. model.name is never rewritten, so settings keep pointing at the removed model.
  2. For the active provider only refreshAuth runs, and the model it re-resolves is not guaranteed to be fallbackModel.
  3. For an inactive provider refreshAuth is not called at all, so the stale selection survives to the next launch.

Either soften the prompt text (e.g. "Your selected model is being removed; use /model to pick a new one") or restore an explicit switch limited to the currentModelAffected case.

Note: the Model switched to "{{model}}" branch below (lines 325-336) is still reachable via refreshAuth re-resolution (see config.ts:3686-3687), so it should not be removed. But the syncAuthState callback at lines 303-306 is now unreachable on this path.


This review was generated by QoderWork AI

const previousModel = config.getModel();
const activeConfig = config.getContentGeneratorConfig();
const updatesActiveProvider =
Expand All @@ -280,14 +287,6 @@ export function useProviderUpdates(
activeConfig.baseUrl,
activeConfig.apiKeyEnvKey,
);
const newConfigs = installPlan.modelProviders?.[0]?.models ?? [];
const previousModelStillAvailable = newConfigs.some(
(cfg) => cfg.id === previousModel,
);
// Only the active provider may migrate model selection.
if (!updatesActiveProvider || previousModelStillAvailable) {
delete installPlan.modelSelection;
}
const settingsAdapter = createLoadedSettingsAdapter(settings);

await applyProviderInstallPlan(installPlan, {
Expand Down Expand Up @@ -386,7 +385,7 @@ export function useProviderUpdates(
setUpdateRequest(undefined);
if (choice === 'update') {
for (const p of pendingList) {
await executeUpdate(p.provider, p.baseUrl);
await executeUpdate(p);
}
} else if (choice === 'skip') {
const persistScope = getPersistScopeForModelSelection(settings);
Expand Down
17 changes: 13 additions & 4 deletions packages/core/src/providers/__tests__/provider-config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,14 @@ describe('buildInstallPlan', () => {
});

it('builds a plan with editable models and unknown IDs', () => {
const config = makeConfig({ modelsEditable: true });
const plan = buildInstallPlan(config, {
const config = makeConfig({
modelsEditable: true,
models: [
{ id: 'model-a', contextWindowSize: 8192, enableThinking: true },
{ id: 'model-b' },
],
});
const plan = buildInstallPlanSrc(config, {
baseUrl: 'https://api.test.com/v1',
apiKey: 'sk-test',
modelIds: ['model-a', 'unknown-model'],
Expand All @@ -77,6 +83,9 @@ describe('buildInstallPlan', () => {
name: '[Test] unknown-model',
});
expect(models?.[1]?.generationConfig).toBeUndefined();
expect(plan.providerState?.['providerMetadata.test']?.['version']).toBe(
computeModelListVersion(models ?? []),
);
});

it('applies advancedConfig to editable unknown model IDs only', () => {
Expand Down Expand Up @@ -674,7 +683,9 @@ import {
getAllProviderBaseUrls as getAllProviderBaseUrlsSrc,
} from '../all-providers.js';
import {
buildInstallPlan as buildInstallPlanSrc,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] Still unresolved from the previous review: source import placed after first use

buildInstallPlanSrc is first used at line 72 but only imported here at line 686. ESM hoisting makes this legal, so tests pass, but a reader scanning the top import block cannot tell where buildInstallPlanSrc comes from, and any future reordering or a switch to a non-hoisting transform breaks it.

Please move this source-relative import block (and the ../all-providers.js block above it) to the top of the module alongside the @qwen-code/qwen-code-core imports.


This review was generated by QoderWork AI

resolveBaseUrl as resolveBaseUrlSrc,
resolveMetadataKey as resolveMetadataKeySrc,
providerMatchesCredentials as providerMatchesCredentialsSrc,
} from '../provider-config.js';

Expand Down Expand Up @@ -795,8 +806,6 @@ describe('providerMatchesCredentials with function envKey (custom provider)', ()
});
});

import { resolveMetadataKey as resolveMetadataKeySrc } from '../provider-config.js';

describe('customHeaders in ProviderConfig', () => {
it('merges customHeaders into generationConfig for fixed models', () => {
const config = makeConfig({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => {
};
});

import { AuthType, type ProviderInstallPlan } from '@qwen-code/qwen-code-core';
import {
AuthType,
CODING_PLAN_GLOBAL_BASE_URL,
type ProviderInstallPlan,
} from '@qwen-code/qwen-code-core';
import { CODING_PLAN_ENV_KEY } from './subscriptionPlanDefinitions.js';
import {
applyProviderInstallPlanToFile,
Expand All @@ -52,6 +56,19 @@ describe('settingsWriter', () => {
fs.rmSync(tempDir, { recursive: true, force: true });
});

it('persists the selected Coding Plan region metadata', () => {
writeCodingPlanConfig('global', 'coding-plan-key');

const settings = JSON.parse(
fs.readFileSync(settingsPath, 'utf-8'),
) as Record<string, Record<string, Record<string, unknown>>>;

expect(settings.providerMetadata?.['coding-plan']).toMatchObject({
region: 'global',
baseUrl: CODING_PLAN_GLOBAL_BASE_URL,
});
});

it('clears stale coding plan metadata when writing api-key providers', () => {
writeCodingPlanConfig('china', 'coding-plan-key');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ export function writeCodingPlanConfig(
const providerMetadata = ensureNestedObject(settings, 'providerMetadata');
providerMetadata['coding-plan'] = {
region: codingRegion,
baseUrl: planConfig.baseUrl,
version: planConfig.version,
};
delete settings.codingPlan;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,41 +5,56 @@
*/

import { describe, expect, it } from 'vitest';
import {
buildProviderTemplate,
codingPlanProvider,
computeModelListVersion,
getDefaultModelIds,
tokenPlanProvider,
} from '@qwen-code/qwen-code-core';

import {
CodingPlanRegion,
getSubscriptionPlanConfig,
} from './subscriptionPlanDefinitions.js';

describe('subscription plan definitions', () => {
it('keeps Token Plan on its dedicated model list', () => {
it('keeps Token Plan on its dedicated model list, sourced from the core preset', () => {
const tokenPlan = getSubscriptionPlanConfig('token');
const codingPlan = getSubscriptionPlanConfig('coding');

expect(tokenPlan.template.map((model) => model.id)).toEqual([
'qwen3.7-plus',
'qwen3.6-plus',
'qwen3.7-max',
'qwen3.8-max-preview',
'qwen3.6-flash',
'deepseek-v4-pro',
'deepseek-v4-flash',
'deepseek-v3.2',
'kimi-k2.7-code',
'kimi-k2.6',
'kimi-k2.5',
'glm-5.2',
'glm-5.1',
'glm-5',
'MiniMax-M2.5',
]);
// Pinning ids here would let this copy drift from the preset the CLI reads
// — which is how a renamed built-in stayed in the IDE's list. Assert the
// shared source instead.
expect(tokenPlan.template.map((model) => model.id)).toEqual(
getDefaultModelIds(tokenPlanProvider),
);
expect(codingPlan.template.map((model) => model.id)).toEqual(
getDefaultModelIds(codingPlanProvider),
);
expect(tokenPlan.template.map((model) => model.id)).not.toEqual(
codingPlan.template.map((model) => model.id),
);
expect(codingPlan.template.map((model) => model.id)).not.toContain(
'qwen3.7-max',
);
expect(
tokenPlan.template.find((model) => model.id === 'deepseek-v4-pro')
?.generationConfig,
).toEqual({ contextWindowSize: 1000000 });
});

it('records a version the CLI can reproduce from the same preset', () => {
// The CLI recomputes this on every launch to detect a pending provider
// update; a version it cannot reproduce shows the update prompt right
// after signing in from the IDE.
for (const [planId, provider] of [
['token', tokenPlanProvider],
['coding', codingPlanProvider],
] as const) {
for (const region of [CodingPlanRegion.CHINA, CodingPlanRegion.GLOBAL]) {
const plan = getSubscriptionPlanConfig(planId, region);
const template = buildProviderTemplate(provider, plan.baseUrl);
expect(plan.version).toBe(computeModelListVersion(template));
expect(plan.template).toEqual(template);
}
}
});

it('defaults Token Plan to China and supports the Singapore region', () => {
Expand Down
Loading
Loading