Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
153 changes: 133 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,
);
Comment thread
yiliang114 marked this conversation as resolved.
});

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 All @@ -323,6 +350,89 @@ describe('useProviderUpdates', () => {
);
});

it('preserves the stored global base URL when updating', async () => {
const globalTemplate = buildProviderTemplate(
codingPlanProvider,
CODING_PLAN_GLOBAL_BASE_URL,
);
const globalVersion = computeModelListVersion(globalTemplate);
(mockSettings.merged[PROVIDER_METADATA_NS] as Record<string, unknown>)[
METADATA_KEY
] = {
baseUrl: CODING_PLAN_GLOBAL_BASE_URL,
version: 'old-version-hash',
};
mockSettings.merged['modelProviders'] = {
[AuthType.USE_OPENAI]: globalTemplate,
};

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

await waitFor(() => {
expect(result.current.providerUpdateRequest).toBeDefined();
});
await result.current.providerUpdateRequest!.onConfirm('update');

expect(mockSettings.setValue).toHaveBeenCalledWith(
expect.anything(),
`${PROVIDER_METADATA_NS}.${METADATA_KEY}.baseUrl`,
CODING_PLAN_GLOBAL_BASE_URL,
);
expect(mockSettings.setValue).toHaveBeenCalledWith(
expect.anything(),
`${PROVIDER_METADATA_NS}.${METADATA_KEY}.version`,
globalVersion,
);
});

it('updates both provider metadata keys from a batched prompt', async () => {
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],
};

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

await waitFor(() => {
expect(result.current.providerUpdateRequest?.entries).toHaveLength(2);
});
await result.current.providerUpdateRequest!.onConfirm('update');

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,
);
});

it.each([
{
name: 'on the same protocol',
Expand Down Expand Up @@ -444,14 +554,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 +585,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);
Comment thread
yiliang114 marked this conversation as resolved.
// 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;
Comment thread
yiliang114 marked this conversation as resolved.
delete installPlan.env;
// Template updates never change the selected model.
delete installPlan.modelSelection;
Comment on lines +279 to +280

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.

[Suggestion] The syncAuthState callback passed to applyProviderInstallPlan in executeUpdate (~line 303) is dead wiring left behind by this diff: modelSelection is now unconditionally deleted before the plan is applied, and applyProviderInstallPlan only invokes syncAuthState inside if (effectiveModelSelection?.modelId) (install.ts), so the callback can never fire on the update path. — Concrete cost: the call site reads as if the update flow still syncs model/auth state, contradicting the invariant the added comment states; a future change that lets a modelSelection survive this path would silently re-enable model switching through a callback everyone assumed was inert. Fix: drop the syncAuthState property from the options object passed here (it is optional in ApplyProviderInstallPlanOptions).

中文说明

[建议] executeUpdate 传给 applyProviderInstallPlansyncAuthState 回调(约第 303 行)是本 diff 留下的死代码:modelSelection 现在在应用计划前被无条件删除,而 applyProviderInstallPlan 只在 if (effectiveModelSelection?.modelId) 内调用 syncAuthState(install.ts),因此更新路径上该回调永远不可能触发。 — 具体代价:调用点看起来仍像更新流程会同步模型/认证状态,与新增注释声明的不变量矛盾;未来若有改动让 modelSelection 在此路径存活,会通过一个人人以为已失效的回调悄悄重新启用模型切换。修复:删除此处传入的 options 对象中的 syncAuthState 属性(它在 ApplyProviderInstallPlanOptions 中是可选的)。

— qwen3.8-max via Qwen Code /review (v0.21.9)

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 ?? []),
);
Comment thread
yiliang114 marked this conversation as resolved.
});

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,
Comment thread
yiliang114 marked this conversation as resolved.
} 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 @@ -17,7 +17,6 @@ export const CODING_PLAN_CHINA_BASE_URL =
export const CODING_PLAN_GLOBAL_BASE_URL =
'https://coding-intl.dashscope.aliyuncs.com/v1';

// keep in sync with packages/vscode-ide-companion/src/services/subscriptionPlanDefinitions.ts ALIBABA_SUBSCRIPTION_MODELS
const MODELSTUDIO_MODELS: ModelSpec[] = [
{
id: 'qwen3.5-plus',
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,
});
Comment on lines +66 to +69

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.

[Suggestion] No test asserts the version value writeCodingPlanConfig persists — this toMatchObject checks only region and baseUrl. — Failure scenario (probe-verified in this review): removing version: planConfig.version from writeCodingPlanConfig leaves the entire vscode-ide-companion suite green (483 passed | 5 pre-existing env failures, identical to baseline). With version missing, the CLI's findAllPendingUpdates hits if (!metadata.version) continue; and IDE-signed-in users are never offered template updates; with a locally computed hash the #8504 prompt loop returns on every launch. The suggested assertion flips the probe (fails under the mutation, passes on the clean tree); it needs getSubscriptionPlanConfig imported from ./subscriptionPlanDefinitions.js.

Suggested change
expect(settings.providerMetadata?.['coding-plan']).toMatchObject({
region: 'global',
baseUrl: CODING_PLAN_GLOBAL_BASE_URL,
});
expect(settings.providerMetadata?.['coding-plan']).toMatchObject({
region: 'global',
baseUrl: CODING_PLAN_GLOBAL_BASE_URL,
version: getSubscriptionPlanConfig('coding', 'global').version,
});
中文说明

[建议] 没有测试断言 writeCodingPlanConfig 持久化的 version 值 —— 此 toMatchObject 只检查 regionbaseUrl。 — 失败场景(本次评审已通过探针验证):从 writeCodingPlanConfig 中删除 version: planConfig.version 后,整个 vscode-ide-companion 测试套件仍全绿(483 通过 | 5 个预存环境失败,与基线完全相同)。version 缺失时,CLI 的 findAllPendingUpdates 会在 if (!metadata.version) continue; 处跳过,IDE 登录用户将永远不会收到模板更新提示;若换成本地计算的哈希,#8504 的更新提示循环会在每次启动时复现。上方 suggestion 可翻转该探针(变异后失败、干净代码上通过);需要从 ./subscriptionPlanDefinitions.js 导入 getSubscriptionPlanConfig

— qwen3.8-max via Qwen Code /review (v0.21.9)

});

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
Loading
Loading