Skip to content

fix(core): preserve the selected model when re-applying a provider install plan #5835

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 13 commits into from
Jun 28, 2026
Merged

fix(core): preserve the selected model when re-applying a provider install plan #5835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d125484
fix(providers): preserve user-selected model on re-install/re-auth (#…
lcheng321 Jun 24, 2026
b7f1948
test(cli): add getValue stub to settings mocks broken by install-plan…
lcheng321 Jun 24, 2026
fa08544
test(cli): add getValue stub to settings mocks broken by install-plan…
lcheng321 Jun 24, 2026
794f07c
fix(providers): address review feedback on preserve-model PR
lcheng321 Jun 25, 2026
901c6fa
fix(providers): address review feedback on preserve-model PR
lcheng321 Jun 25, 2026
0cebdec
style: fix indentation in run-qwen-serve.ts installAuthProvider
lcheng321 Jun 25, 2026
82522ea
fix(providers): use isSameModelIdentity for model retention check and…
lcheng321 Jun 25, 2026
7fc1bbf
fix: remove duplicate baseUrl in acpAgent providers/connect response
lcheng321 Jun 25, 2026
dd6818a
fix(test): preserve settings reference in acpAgent createLoadedSettin…
lcheng321 Jun 25, 2026
383a6e2
test: add coverage for planOffersCurrentModel and effectiveModelId
lcheng321 Jun 25, 2026
35ed5e4
fix(test): fix TypeScript cast in acpAgent preserved model test
lcheng321 Jun 25, 2026
2410e73
fix(serve): read effective model/baseUrl from adapter in installAuthP…
lcheng321 Jun 26, 2026
51de03d
style: fix prettier formatting
lcheng321 Jun 27, 2026
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
44 changes: 43 additions & 1 deletion packages/cli/src/acp-integration/acpAgent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,10 @@ vi.mock('../config/settings.js', () => ({
reloadEnvironment: vi.fn(() => ({ updatedKeys: [], removedKeys: [] })),
}));
vi.mock('../config/loadedSettingsAdapter.js', () => ({
createLoadedSettingsAdapter: vi.fn((settings: unknown) => settings),
createLoadedSettingsAdapter: vi.fn((settings: unknown) => {
(settings as Record<string, unknown>)['getValue'] = vi.fn();
return settings;
}),
}));
vi.mock('../config/config.js', () => ({
loadCliConfig: vi.fn(),
Expand Down Expand Up @@ -592,6 +595,7 @@ import {
MAX_PERMISSION_RULES_COUNT,
} from '../config/permission-settings.js';
import { loadCliConfig } from '../config/config.js';
import { createLoadedSettingsAdapter } from '../config/loadedSettingsAdapter.js';
import { Session, buildAvailableCommandsSnapshot } from './session/Session.js';
import {
SERVE_STATUS_EXT_METHODS,
Expand Down Expand Up @@ -3993,6 +3997,44 @@ describe('QwenAgent MCP SSE/HTTP support', () => {
await agentPromise;
});

it('qwen/providers/connect returns preserved model when adapter getValue returns a non-empty string', async () => {
vi.mocked(createLoadedSettingsAdapter).mockImplementationOnce(
(settings: unknown) => {
(settings as Record<string, unknown>)['getValue'] = vi.fn(
(key: string) =>
key === 'model.name' ? 'deepseek-flash' : undefined,
);
return settings as unknown as ReturnType<
typeof createLoadedSettingsAdapter
>;
},
);

const settings = makeSessionSettings();
const agentPromise = runAcpAgent(mockConfig, settings, mockArgv);
await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined());

const agent = capturedAgentFactory!({
get closed() {
return mockConnectionState.promise;
},
}) as AgentLike;

await expect(
agent.extMethod('qwen/providers/connect', {
providerId: 'deepseek',
apiKey: 'sk-test',
modelIds: ['deepseek-chat'],
}),
).resolves.toMatchObject({
success: true,

@doudouOUC doudouOUC Jun 28, 2026

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] This new test asserts modelId readback but not baseUrl. The production code introduces a parallel effectiveBaseUrl = adapter.getValue('model.baseUrl') ?? plan.modelSelection?.baseUrl computation that is not exercised here.

Consider extending this test (or adding a companion case) to also configure getValue to return a baseUrl and assert it appears in the response:

).resolves.toMatchObject({
  success: true,
  modelId: 'deepseek-flash',
  baseUrl: 'https://api.deepseek.com/v1',
});

— qwen3.7-max via Qwen Code /review

modelId: 'deepseek-flash',
});

mockConnectionState.resolve();
await agentPromise;
});

it('qwen/providers/list includes existing provider settings', async () => {
const settings = {
...makeSessionSettings(),
Expand Down
19 changes: 13 additions & 6 deletions packages/cli/src/acp-integration/acpAgent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4872,8 +4872,12 @@ class QwenAgent implements Agent {
);
const persistScope = readProviderConnectScope(params['scope']);
const plan = buildInstallPlan(providerConfig, inputs);
const adapter = createLoadedSettingsAdapter(
this.settings,
persistScope,
);
await applyProviderInstallPlan(plan, {
settings: createLoadedSettingsAdapter(this.settings, persistScope),
settings: adapter,
reloadModelProviders: (modelProviders) =>
this.config.reloadModelProvidersConfig(modelProviders),
syncAuthState: (authType, modelId, baseUrl) =>
Expand All @@ -4882,16 +4886,19 @@ class QwenAgent implements Agent {
.syncAfterAuthRefresh(authType, modelId, baseUrl),
refreshAuth: (authType) => this.config.refreshAuth(authType),
});

const effectiveModelId =
(adapter.getValue('model.name') as string | undefined) ??

@wenshao wenshao Jun 25, 2026

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.

[Critical] The new adapter.getValue('model.name') call here breaks 3 tests in acpAgent.test.ts (lines 3963, 4471, 4514). The mock for createLoadedSettingsAdapter returns the raw settings object via vi.fn((settings: unknown) => settings), which has no getValue method — all three qwen/providers/connect tests fail with TypeError: adapter.getValue is not a function.

Verified: base branch passes 155/155, this PR fails 3/155.

Suggested change
(adapter.getValue('model.name') as string | undefined) ??
const effectiveModelId =
((adapter as any).getValue?.('model.name') as string | undefined) ??
plan.modelSelection?.modelId;

Or better: update the mock in acpAgent.test.ts to include a getValue stub on the adapter returned by createLoadedSettingsAdapter:

createLoadedSettingsAdapter: vi.fn((settings: unknown) => ({
  ...settings,
  getValue: vi.fn(),
})),

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot Jun 25, 2026

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.

[Critical] The new adapter.getValue('model.name') call here breaks 3 tests in acpAgent.test.ts with TypeError: adapter.getValue is not a function. This is likely the cause of the failing Test (ubuntu-latest, Node 22.x) CI check.

The PR fixed two sibling test files (useAuth.test.ts, useProviderUpdates.test.ts) by switching their vi.mock('../../utils/settingsUtils.js') to the importOriginal pattern, but acpAgent.test.ts was missed. In that file the createLoadedSettingsAdapter mock is effectively vi.fn((settings: unknown) => settings), and makeSessionSettings() returns an object with no getValue method — so the identity-mock adapter has no getValue to call.

Failing tests:

  • qwen/providers extension methods list and connect model providers
  • qwen/providers/connect reuses the stored apiKey when the client omits it
  • qwen/providers/connect reuses the custom apiKey for the requested baseUrl only

Either upgrade makeSessionSettings() (and siblings like makeMemorySettings) with a getValue: vi.fn() stub, or change the createLoadedSettingsAdapter mock to return a proper adapter-shaped object:

Suggested change
(adapter.getValue('model.name') as string | undefined) ??
vi.mock('../../utils/settingsUtils.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../utils/settingsUtils.js')>();
return {
...actual,
// ... existing overrides ...
};
});

matching the pattern used in the two fixed test files.

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot Jun 25, 2026

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.

[Critical] The effectiveModelId computation is untested. In acpAgent.test.ts, the createLoadedSettingsAdapter mock returns { getValue: vi.fn() } (always undefined), so the ?? plan.modelSelection?.modelId fallback always fires. Existing qwen/providers/connect tests assert modelId from the plan's default — no test verifies that a preserved model appears in the ACP response instead.

Add a test where adapter.getValue('model.name') returns a non-empty string and verify the ACP response includes that model ID rather than the plan's default.

— qwen3.7-max via Qwen Code /review

plan.modelSelection?.modelId;
const effectiveBaseUrl =
(adapter.getValue('model.baseUrl') as string | undefined) ??
plan.modelSelection?.baseUrl;
return {
success: true,
providerId: providerConfig.id,
providerLabel: providerConfig.label,
authType: plan.authType,
modelId: plan.modelSelection?.modelId,
...(plan.modelSelection?.baseUrl
? { baseUrl: plan.modelSelection.baseUrl }
: {}),
...(effectiveModelId ? { modelId: effectiveModelId } : {}),
...(effectiveBaseUrl ? { baseUrl: effectiveBaseUrl } : {}),
};
}
case 'qwen/skills/install': {
Expand Down
22 changes: 14 additions & 8 deletions packages/cli/src/serve/run-qwen-serve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1777,26 +1777,32 @@ export async function runQwenServe(
});
const plan = core.buildInstallPlan(provider, inputs);
const fresh = settingsRuntime.settings.loadSettings(boundWorkspace);
const adapter =
settingsRuntime.loadedSettingsAdapter.createLoadedSettingsAdapter(
fresh,
);
await core.applyProviderInstallPlan(plan, {
settings:
settingsRuntime.loadedSettingsAdapter.createLoadedSettingsAdapter(
fresh,
),
settings: adapter,
doRefreshAuth: false,
});
core.emitDaemonLog('Auth provider installed.', {
'qwen-code.daemon.auth.provider_id': provider.id,
'qwen-code.daemon.auth.auth_type': plan.authType,
});
const effectiveModelId =
(adapter.getValue('model.name') as string | undefined) ??
plan.modelSelection?.modelId;
const effectiveBaseUrl =
(adapter.getValue('model.baseUrl') as string | undefined) ??
plan.modelSelection?.baseUrl ??
inputs.baseUrl;
return {
v: 1,
providerId: provider.id,
providerLabel: provider.label,
authType: plan.authType,
...(plan.modelSelection?.modelId
? { modelId: plan.modelSelection.modelId }
: {}),
...(inputs.baseUrl ? { baseUrl: inputs.baseUrl } : {}),
...(effectiveModelId ? { modelId: effectiveModelId } : {}),
...(effectiveBaseUrl ? { baseUrl: effectiveBaseUrl } : {}),
message: `Successfully configured ${provider.label}. Use /model to switch models.`,
};
},
Expand Down
15 changes: 10 additions & 5 deletions packages/cli/src/ui/auth/useAuth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,16 @@ vi.mock('../hooks/useQwenAuth.js', () => ({
})),
}));

vi.mock('../../utils/settingsUtils.js', () => ({
backupSettingsFile: vi.fn(),
restoreSettingsFromBackup: vi.fn(),
cleanupSettingsBackup: vi.fn(),
}));
vi.mock('../../utils/settingsUtils.js', async (importOriginal) => {
const actual =
await importOriginal<typeof import('../../utils/settingsUtils.js')>();
return {
...actual,
backupSettingsFile: vi.fn(),
restoreSettingsFromBackup: vi.fn(),
cleanupSettingsBackup: vi.fn(),
};
});

vi.mock('../../config/modelProvidersScope.js', () => ({
getPersistScopeForModelSelection: vi.fn(() => 'user'),
Expand Down
15 changes: 10 additions & 5 deletions packages/cli/src/ui/hooks/useProviderUpdates.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,16 @@ import {
} from '@qwen-code/qwen-code-core';
import { useProviderUpdates } from './useProviderUpdates.js';

vi.mock('../../utils/settingsUtils.js', () => ({
backupSettingsFile: vi.fn(),
restoreSettingsFromBackup: vi.fn(),
cleanupSettingsBackup: vi.fn(),
}));
vi.mock('../../utils/settingsUtils.js', async (importOriginal) => {
const actual =
await importOriginal<typeof import('../../utils/settingsUtils.js')>();
return {
...actual,
backupSettingsFile: vi.fn(),
restoreSettingsFromBackup: vi.fn(),
cleanupSettingsBackup: vi.fn(),
};
});

const chinaTemplate = buildProviderTemplate(
codingPlanProvider,
Expand Down
41 changes: 34 additions & 7 deletions packages/core/src/providers/install.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,38 @@ export async function applyProviderInstallPlan(
}

// Model selection
// Re-applying a plan (manual /auth, ACP reconnect, token refresh, or an

@qwen-code-ci-bot qwen-code-ci-bot Jun 25, 2026

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.

[Critical] The planOffersCurrentModel branch introduced here has no test coverage. In install.test.ts, createAdapter() uses getValue: vi.fn() which returns undefined, so typeof currentModelId === 'string' is always false and the preserve path is never exercised. No existing test asserts that model.name is preserved or that syncAuthState is skipped.

The core fix for #5819 has no safety net — a regression would silently re-introduce the bug and no test would catch it. The PR description references a drop-in regression test but intentionally excludes it from the diff.

Add at least two tests to install.test.ts:

  1. getValue('model.name') returns a model ID present in modelProviders (with matching baseUrl) → assert setValue NOT called for model.name and syncAuthState NOT called
  2. getValue('model.name') returns a model ID absent from modelProviders → assert setValue IS called with the plan's default model

— qwen3.7-max via Qwen Code /review

// upgrade that reordered the model list) must not silently move the user
// off a model they chose. If the plan still offers the current model, keep
// it; a genuine first-time setup still adopts the provider default. (#5819)
currentStep = 'modelSelection';
if (plan.modelSelection?.modelId) {
settings.setValue('model.name', plan.modelSelection.modelId);
if (plan.modelSelection.baseUrl) {
settings.setValue('model.baseUrl', plan.modelSelection.baseUrl);
let effectiveModelSelection = plan.modelSelection;
if (effectiveModelSelection?.modelId) {
const currentModelId = settings.getValue('model.name');

@wenshao wenshao Jun 25, 2026

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.

[Critical] This only preserves a model that was persisted in settings.model.name, but the active model can also come from runtime inputs such as OPENAI_MODEL/QWEN_MODEL (and CLI resolution only considers those env vars when settings has no model). In that case re-applying a provider plan sees no current model here, writes plan.modelSelection.modelId, and syncAuthState/refreshAuth moves the live session to the provider default even if the env-selected model is still offered. It also persists that default into settings, so later launches ignore the env-selected model because settings wins over env. Please thread the runtime-selected model/baseUrl into this decision, or have callers suppress the default selection when config.getModel() is still present in the new provider model list.

— gpt-5 via Qwen Code /review

const currentBaseUrl = settings.getValue('model.baseUrl') as
| string
| undefined;
const planOffersCurrentModel =

@qwen-code-ci-bot qwen-code-ci-bot Jun 25, 2026

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] This new planOffersCurrentModel branch is a significant behavioral change, but no tests in packages/core/src/providers/__tests__/install.test.ts exercise it. The existing test adapter's getValue is vi.fn() which returns undefined by default, so typeof currentModelId === 'string' is always false and the preservation path is never reached.

The PR description includes a comprehensive drop-in regression test (the install-preserve-user-model.test.ts block) that covers all five scenarios — consider including it in the diff, or adding equivalent cases to the existing install.test.ts.

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot Jun 25, 2026

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 new planOffersCurrentModel branch is the core behavioral change of this PR, but no test in install.test.ts exercises it. The existing test adapter's getValue is vi.fn() (returns undefined), so typeof currentModelId === 'string' is always false and the preservation path is never hit. A future refactor that inverts or removes this guard would go undetected.

Consider adding at least two test cases:

  1. getValue('model.name') returns a model ID present in plan.modelProviders — assert setValue('model.name', ...) is NOT called and syncAuthState is NOT called.
  2. getValue('model.name') returns a model ID NOT in plan.modelProviders — assert setValue('model.name', plan.modelSelection.modelId) IS called.

The PR description includes a drop-in regression test (in the details block) that could be committed alongside this change.

— qwen3.7-max via Qwen Code /review

@lcheng321 lcheng321 Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The drop-in test in the PR description covers all 5 scenarios and can be run against this branch. Committing it will be a follow-up PR to keep this fix minimal.

typeof currentModelId === 'string' &&
currentModelId.length > 0 &&
(plan.modelProviders ?? []).some((patch) =>

@wenshao wenshao Jun 25, 2026

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] planOffersCurrentModel matches by model.id alone, ignoring baseUrl. The file already has isSameModelIdentity() (line 39) that checks both id and baseUrl — using it here would be more robust.

With ID-only matching, a cross-provider model ID collision (e.g., two custom providers both offering "gpt-4" with different endpoints) produces a false-positive preservation: the model name is kept but the baseUrl may be cleared or overwritten, silently misrouting requests.

Suggested change
(plan.modelProviders ?? []).some((patch) =>
(plan.modelProviders ?? []).some((patch) =>
patch.models.some((model) =>
isSameModelIdentity(
{ id: currentModelId, baseUrl: settings.getValue('model.baseUrl') as string | undefined },
model,
),
),
);

— qwen3.7-max via Qwen Code /review

patch.models.some((model) =>
currentBaseUrl === '' || currentBaseUrl === undefined
? model.id === currentModelId
: isSameModelIdentity(
{ id: currentModelId, baseUrl: currentBaseUrl },
model,
),
),
);
if (planOffersCurrentModel) {

@qwen-code-ci-bot qwen-code-ci-bot Jun 25, 2026

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] When planOffersCurrentModel is true, effectiveModelSelection is set to undefined, which skips the entire model write block below — including the model.baseUrl clearing (the empty-string tombstone on the else branch). If a user has a stale baseUrl from a previous provider setup and re-authenticates with a provider that offers the same model ID, the old baseUrl survives and could route API traffic to a stale or unintended endpoint.

Consider still applying the plan's baseUrl logic (write or clear) when preserving the model — only the model.name write should be skipped:

if (planOffersCurrentModel) {
  // Preserve the model name, but still apply the plan's baseUrl decision
  if (plan.modelSelection?.baseUrl) {
    settings.setValue('model.baseUrl', plan.modelSelection.baseUrl);
  } else {
    settings.setValue('model.baseUrl', '');
  }
  effectiveModelSelection = undefined;
}

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot Jun 25, 2026

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] There is no log or diagnostic signal at this decision point. When investigating "why did the user's model change" or "why was the model NOT changed" in production, there is no trace indicating whether planOffersCurrentModel was true or false, what currentModelId was, or which model entry matched.

Consider adding a debug log:

log?.debug?.('applyProviderInstallPlan: model retention check', {
  currentModelId,
  planOffersCurrentModel,
  planDefaultModelId: plan.modelSelection?.modelId,
});

— qwen3.7-max via Qwen Code /review

@lcheng321 lcheng321 Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reasonable suggestion, out of scope for this fix. Will handle in a follow-up.

effectiveModelSelection = undefined;

@qwen-code-ci-bot qwen-code-ci-bot Jun 25, 2026

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] When effectiveModelSelection is set to undefined here, the function's return value (ApplyProviderInstallPlanResult) still only carries updatedModelProviders — it does not expose which model was effectively selected. Callers like acpAgent.ts (~line 4856) and run-qwen-serve.ts (~line 1710) read plan.modelSelection?.modelId from the original (never-mutated) plan object, so they report the plan's default model in their protocol responses even though the user's previous model was actually kept.

Consider extending ApplyProviderInstallPlanResult with an effectiveModelId?: string (and effectiveBaseUrl?: string) field, set to the current model when preservation fires or the plan's model when applied. Have callers use this field instead of reading from the original plan.

— qwen3.7-max via Qwen Code /review

@lcheng321 lcheng321 Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Already fixed: both acpAgent.ts and run-qwen-serve.ts now read adapter.getValue('model.name') instead of plan.modelSelection?.modelId.

}
}
if (effectiveModelSelection?.modelId) {
settings.setValue('model.name', effectiveModelSelection.modelId);
if (effectiveModelSelection.baseUrl) {
settings.setValue('model.baseUrl', effectiveModelSelection.baseUrl);
} else {
// The plan selects by model id only, so clear any baseUrl disambiguator
// left by a previous model-picker selection — otherwise the next launch
Expand All @@ -241,12 +268,12 @@ export async function applyProviderInstallPlan(
// Reload runtime config
currentStep = 'reloadModelProviders';
reloadModelProviders?.(updatedModelProviders);
if (plan.modelSelection?.modelId) {
if (effectiveModelSelection?.modelId) {
currentStep = 'syncAuthState';
syncAuthState?.(
plan.authType,
plan.modelSelection.modelId,
plan.modelSelection.baseUrl,
effectiveModelSelection.modelId,
effectiveModelSelection.baseUrl,
);
}
if (doRefreshAuth && refreshAuth) {
Expand Down
Loading