Skip to content

fix(cli): keep /model switches session-scoped #4332

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

Open
qqqys wants to merge 12 commits into
base: main
Choose a base branch
from
Open

fix(cli): keep /model switches session-scoped #4332

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a101ca8
fix(cli): keep model switches session-scoped
qqqys May 19, 2026
412c8d6
fix(cli): add model command locale coverage
qqqys May 19, 2026
ef5e575
test(cli): cover model default review paths
qqqys May 19, 2026
0779951
fix(cli): harden model default shortcut
qqqys May 19, 2026
eab2ece
fix(cli): address model default review feedback
qqqys May 19, 2026
fd46f47
fix(cli): harden model default persistence
qqqys May 20, 2026
f832609
fix(cli): polish model default review feedback
qqqys May 20, 2026
1faa612
fix(cli): clarify model session default feedback
qqqys May 20, 2026
346da6e
fix(cli): handle model default persistence errors
qqqys May 20, 2026
ce79a71
fix(cli): complete model flag suggestions
qqqys May 20, 2026
0b5a1ff
fix(cli): serialize model default switching
qqqys May 20, 2026
063fef6
fix(cli): handle fast model persistence failures
qqqys May 20, 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
68 changes: 49 additions & 19 deletions packages/cli/src/ui/commands/modelCommand.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@ describe('modelCommand', () => {
it('should have the correct name and description', () => {
expect(modelCommand.name).toBe('model');
expect(modelCommand.description).toBe(
'Switch the model for this session (--fast for suggestion model, [model-id] to switch immediately).',
'Switch the model for this session (--default to persist, --fast for suggestion model).',
);
expect(modelCommand.argumentHint).toBe('[--default|--fast] [<model-id>]');
});

it('should return error when config is not available', async () => {
Expand Down Expand Up @@ -151,7 +152,7 @@ describe('modelCommand', () => {
});
});

it('should switch the main model directly in interactive mode when args are provided', async () => {
it('should switch the main model for the current session without persisting when args are provided', async () => {
const setValue = vi.fn();
const switchModel = vi.fn().mockResolvedValue(undefined);
mockContext = createMockCommandContext({
Expand All @@ -173,6 +174,48 @@ describe('modelCommand', () => {

const result = await modelCommand.action!(mockContext, 'qwen-max');

expect(switchModel).toHaveBeenCalledWith(
AuthType.QWEN_OAUTH,
'qwen-max',
undefined,
);
expect(setValue).not.toHaveBeenCalled();
expect(result).toEqual({
type: 'message',
messageType: 'info',
content: 'Model: qwen-max',
});
});

it('should persist the main model only when --default is provided', async () => {
const setValue = vi.fn();
const switchModel = vi.fn().mockResolvedValue(undefined);
mockContext = createMockCommandContext({
invocation: {
raw: '/model --default qwen-max',
name: 'model',
args: '--default qwen-max',
},
services: {
config: {
getContentGeneratorConfig: vi.fn().mockReturnValue({
model: 'qwen-plus',
authType: AuthType.QWEN_OAUTH,
}),
getAvailableModelsForAuthType: vi
.fn()
.mockReturnValue([{ id: 'qwen-max', label: 'Qwen Max' }]),
switchModel,
},
settings: createMockSettings(setValue),
},
});

const result = await modelCommand.action!(
mockContext,
'--default qwen-max',
);

expect(switchModel).toHaveBeenCalledWith(
AuthType.QWEN_OAUTH,
'qwen-max',
Expand All @@ -186,7 +229,7 @@ describe('modelCommand', () => {
expect(result).toEqual({
type: 'message',
messageType: 'info',
content: 'Model: qwen-max',
content: 'Default model: qwen-max',
});
});

Expand Down Expand Up @@ -343,7 +386,7 @@ describe('modelCommand', () => {
});
});

it('should switch provider-qualified models through switchModel', async () => {
it('should switch provider-qualified models for the current session without persisting', async () => {
const setValue = vi.fn();
const switchModel = vi.fn().mockResolvedValue(undefined);
mockContext = createMockCommandContext({
Expand Down Expand Up @@ -378,16 +421,7 @@ describe('modelCommand', () => {
'gpt-4',
undefined,
);
expect(setValue).toHaveBeenCalledWith(
expect.any(String),
'security.auth.selectedType',
AuthType.USE_OPENAI,
);
expect(setValue).toHaveBeenCalledWith(
expect.any(String),
'model.name',
'gpt-4',
);
expect(setValue).not.toHaveBeenCalled();
expect(result).toEqual({
type: 'message',
messageType: 'info',
Expand Down Expand Up @@ -572,11 +606,7 @@ describe('modelCommand', () => {
'--fast-model',
undefined,
);
expect(setValue).toHaveBeenCalledWith(
expect.any(String),
'model.name',
'--fast-model',
);
expect(setValue).not.toHaveBeenCalled();
expect(result).toEqual({
type: 'message',
messageType: 'info',
Expand Down
82 changes: 63 additions & 19 deletions packages/cli/src/ui/commands/modelCommand.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ function persistSetting(

async function switchMainModel(
config: Config,
settings: LoadedSettings,
currentAuthType: AuthType,
modelArg: string,
): Promise<string> {
Expand All @@ -47,12 +46,26 @@ async function switchMainModel(
? { requireCachedCredentials: true }
: undefined,
);
return parsed.modelId;
}

await config.switchModel(currentAuthType, modelArg, undefined);
return modelArg;
}

function persistMainModelDefault(
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 19, 2026

Choose a reason for hiding this comment

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

[Suggestion] Several new code paths introduced by this PR lack test coverage:

  1. persistMainModelDefault with provider-qualified model ID — the if (parsed.authType) branch (line 62) is never exercised. The only --default test uses the unqualified qwen-max, which hits the else branch.
  2. --default with null settings — the if (isDefaultModelCommand && !settings) guard (line 291) is a new conditional with no test.
  3. d key error path — the catch block in ModelDialog.tsx (line 432) when config.switchModel throws is untested.
  4. d key in fast model mode — no test renders <ModelDialog isFastModelMode={true} /> and presses d to verify it's a no-op.

Consider adding tests for these branches to prevent regression.

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

settings: LoadedSettings,
currentAuthType: AuthType,
modelArg: string,
): string {
const parsed = parseAcpModelOption(modelArg);
if (parsed.authType) {
persistSetting(settings, 'security.auth.selectedType', parsed.authType);
persistSetting(settings, 'model.name', parsed.modelId);
return parsed.modelId;
}

await config.switchModel(currentAuthType, modelArg, undefined);
persistSetting(settings, 'security.auth.selectedType', currentAuthType);
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 20, 2026

Choose a reason for hiding this comment

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

[Suggestion] The currentAuthType parameter (line 56) is only used in this fallback branch for unqualified model IDs. For qualified IDs like gpt-4(USE_OPENAI), it is silently ignored in favor of parsed.authType. The name implies it always represents the current auth type, but its actual role is a fallback.

Renaming to fallbackAuthType would make the contract self-documenting and prevent future maintainers from passing the wrong value.

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

persistSetting(settings, 'model.name', modelArg);
return modelArg;
}
Expand Down Expand Up @@ -114,26 +127,40 @@ export const modelCommand: SlashCommand = {
completionPriority: 100,
get description() {
return t(
'Switch the model for this session (--fast for suggestion model, [model-id] to switch immediately).',
'Switch the model for this session (--default to persist, --fast for suggestion model).',
);
},
argumentHint: '[--fast] [<model-id>]',
argumentHint: '[--default|--fast] [<model-id>]',
kind: CommandKind.BUILT_IN,
supportedModes: ['interactive', 'non_interactive', 'acp'] as const,
completion: async (context, partialArg) => {
if (partialArg && '--fast'.startsWith(partialArg)) {
return [
{
value: '--fast',
description: t(
'Set a lighter model for prompt suggestions and speculative execution',
),
},
];
} else if (partialArg.trim()) {
const trimmedPartialArg = partialArg.trim();
if (trimmedPartialArg.startsWith('--default ')) {
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 20, 2026

Choose a reason for hiding this comment

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

[Suggestion] The completion function has a --default <partial> branch that strips the flag and completes model IDs, but no analogous branch for --fast <partial>. When the user types /model --fast gpt<Tab>, the input falls through to the bare model-id branch which filters on "--fast gpt" — matching nothing.

Suggested change
if (trimmedPartialArg.startsWith('--default ')) {
if (trimmedPartialArg.startsWith('--default ')) {
const modelPartial = trimmedPartialArg.slice('--default '.length).trim();
return getAvailableModelIds(context)
.filter((id) => id.startsWith(modelPartial))
.map((id) => `--default ${id}`);
}
if (trimmedPartialArg.startsWith('--fast ')) {
const modelPartial = trimmedPartialArg.slice('--fast '.length).trim();
return getAvailableModelIds(context)
.filter((id) => id.startsWith(modelPartial))
.map((id) => `--fast ${id}`);
}

Also: the completion logic (3 new branches + the --fast gap above) has zero test coverage. Consider adding a describe('completion') block covering empty string, partial flags, and model-id completion for both --default and --fast.

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

const modelPartial = trimmedPartialArg.replace('--default', '').trim();
return getAvailableModelIds(context)
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 20, 2026

Choose a reason for hiding this comment

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

[Suggestion] getAvailableModelIds(context) calls config.getAvailableModels() which returns models for the current auth type only (modelRegistry.getModelsForAuthType(this.currentAuthType)). But the action handler accepts provider-qualified models from any auth type (e.g., --default gpt-4(USE_OPENAI) while current auth is QWEN_OAUTH).

Tab-completion will never surface cross-provider models, so users can't discover the cross-provider --default capability. Consider using config.getAllConfiguredModels() here and emitting ACP-qualified IDs for non-current-provider models.

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

.filter((id) => id.startsWith(modelPartial))
.map((id) => `--default ${id}`);
}

const flagCompletions = [
{
value: '--default',
description: t('Persist the selected model as the default'),
},
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 20, 2026

Choose a reason for hiding this comment

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

[Suggestion] Four new t()-wrapped strings have no corresponding entries in locale files (including en.js): 'Default model', 'Persist the selected model as the default', 'Usage: /model --default <model-id>', and 'Enter to select, d to set default, ↑↓ to navigate, Esc to close'. English works via fallback, but non-English users will see untranslated English text.

Add these entries to en.js, zh.js, zh-TW.js (and other locales as appropriate).

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

{
value: '--fast',
description: t(
'Set a lighter model for prompt suggestions and speculative execution',
),
},
].filter((completion) => completion.value.startsWith(trimmedPartialArg));

if (flagCompletions.length > 0) {
return flagCompletions;
} else if (trimmedPartialArg) {
// Include model IDs matching the partial argument
return getAvailableModelIds(context).filter((id) =>
id.startsWith(partialArg.trim()),
id.startsWith(trimmedPartialArg),
);
} else {
return null;
Expand All @@ -156,6 +183,8 @@ export const modelCommand: SlashCommand = {

// Handle --fast flag: /model --fast <modelName>
const args = context.invocation?.args?.trim() || actionArgs.trim();
const isDefaultModelCommand =
args === '--default' || args.startsWith('--default ');
const isFastModelCommand = args === '--fast' || args.startsWith('--fast ');
if (isFastModelCommand) {
const modelName = args.replace('--fast', '').trim();
Expand Down Expand Up @@ -256,9 +285,11 @@ export const modelCommand: SlashCommand = {
};
}

const modelName = args.trim().split(/\s+/)[0] ?? '';
const modelName = isDefaultModelCommand
? args.replace('--default', '').trim().split(/\s+/)[0]
: (args.trim().split(/\s+/)[0] ?? '');
if (modelName) {
if (!settings) {
if (isDefaultModelCommand && !settings) {
return {
type: 'message',
messageType: 'error',
Expand All @@ -283,14 +314,27 @@ export const modelCommand: SlashCommand = {
}
const effectiveModelName = await switchMainModel(
config,
settings,
authType,
modelName,
);
if (isDefaultModelCommand) {
persistMainModelDefault(settings, authType, modelName);
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 19, 2026

Choose a reason for hiding this comment

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

[Critical] The dialog's d handler explicitly blocks AuthType.QWEN_OAUTH models before persisting, but the /model --default command path has no equivalent guard. If QWEN_OAUTH models still appear in the available models list (e.g., for legacy users), /model --default <qwen-oauth-model> will:

  1. Call switchMainModel (which may succeed with cached credentials)
  2. Call persistMainModelDefault, which writes security.auth.selectedType = QWEN_OAUTH to settings

On next startup, the app tries the discontinued auth path and fails. Consider adding a QWEN_OAUTH guard mirroring the dialog:

if (isDefaultModelCommand && authType === AuthType.QWEN_OAUTH) {
  return {
    type: 'message',
    messageType: 'error',
    content: t('Qwen OAuth free tier was discontinued on 2026-04-15. Please select a model from another provider.'),
  };
}

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 20, 2026

Choose a reason for hiding this comment

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

[Suggestion] persistMainModelDefault() is called without try/catch. If settings.setValue throws (read-only disk, permission error), the session model has already been switched by switchMainModel on the preceding line, but the user sees a raw propagated error with no indication the session switch succeeded.

The dialog d-key handler (ModelDialog.tsx:447-457) handles this exact scenario gracefully with a separate try/catch and the message "Switched to 'X' for this session, but failed to persist as default." The command path should match.

Suggested change
persistMainModelDefault(settings, authType, modelName);
if (isDefaultModelCommand) {
try {
persistMainModelDefault(settings, authType, modelName);
} catch (e) {
const msg = e instanceof Error ? e.message : String(e);
return {
type: 'message',
messageType: 'error',
content: `Switched to '${effectiveModelName}' for this session, but failed to persist as default.\n\n${msg}`,
};
}
}

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

}
return {
type: 'message',
messageType: 'info',
content: t('Model') + ': ' + effectiveModelName,
content:
(isDefaultModelCommand ? t('Default model') : t('Model')) +
Copy link
Copy Markdown
Collaborator

@wenshao wenshao May 20, 2026

Choose a reason for hiding this comment

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

[Critical] The success message for session-only switches (/model gpt-4) displays Model: gpt-4 — identical to the old behavior where the selection was persisted. Users have no way to know this change is ephemeral. After restart, the model silently reverts.

The non-interactive help text at line 359 explicitly says "(session only)", but the interactive success message omits this qualifier.

Suggested change
(isDefaultModelCommand ? t('Default model') : t('Model')) +
content:
(isDefaultModelCommand ? t('Default model') : t('Model')) +
': ' +
effectiveModelName +
(isDefaultModelCommand ? '' : ` (${t('session only')})`),

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

': ' +
effectiveModelName,
};
}

if (isDefaultModelCommand) {
return {
type: 'message',
messageType: 'error',
content: t('Usage: /model --default <model-id>'),
};
}

Expand Down
41 changes: 39 additions & 2 deletions packages/cli/src/ui/components/ModelDialog.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ describe('<ModelDialog />', () => {
expect(props.onClose).not.toHaveBeenCalled();
});

it('calls config.switchModel and onClose when selecting a non-OAuth model', async () => {
it('switches the current session without persisting when selecting a non-OAuth model', async () => {
const switchModel = vi.fn().mockResolvedValue(undefined);
const getAuthType = vi.fn(() => AuthType.USE_OPENAI);
const getAvailableModelsForAuthType = vi.fn((t: AuthType) => {
Expand Down Expand Up @@ -275,6 +275,42 @@ describe('<ModelDialog />', () => {
expect(switchModel).toHaveBeenCalledWith(AuthType.USE_OPENAI, 'gpt-4', {
baseUrl: undefined,
});
expect(mockSettings.setValue).not.toHaveBeenCalled();
expect(props.onClose).toHaveBeenCalledTimes(1);
});

it('persists the highlighted model as the default when "d" is pressed', () => {
const switchModel = vi.fn().mockResolvedValue(undefined);
const getAuthType = vi.fn(() => AuthType.USE_OPENAI);

const { props, mockSettings } = renderComponent({}, {
getModel: vi.fn(() => 'gpt-4'),
getAuthType,
switchModel,
getAllConfiguredModels: vi.fn(() => [
{
id: 'gpt-4',
label: 'GPT-4',
description: 'GPT-4 model',
authType: AuthType.USE_OPENAI,
},
]),
getContentGeneratorConfig: vi.fn(() => ({
authType: AuthType.USE_OPENAI,
model: 'gpt-4',
})),
} as unknown as Partial<Config>);

const keyPressHandler = mockedUseKeypress.mock.calls[0][0];
keyPressHandler({
name: 'd',
ctrl: false,
meta: false,
shift: false,
paste: false,
sequence: 'd',
});

expect(mockSettings.setValue).toHaveBeenCalledWith(
SettingScope.User,
'model.name',
Expand All @@ -285,7 +321,8 @@ describe('<ModelDialog />', () => {
'security.auth.selectedType',
AuthType.USE_OPENAI,
);
expect(props.onClose).toHaveBeenCalledTimes(1);
expect(switchModel).not.toHaveBeenCalled();
expect(props.onClose).not.toHaveBeenCalled();
});

it('stores authType-qualified selectors in fast model mode', async () => {
Expand Down
Loading
Loading