fix(cli): keep /model switches session-scoped#4332
Conversation
📋 Review SummaryThis PR addresses issue #4331 by changing the 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] The non-interactive/ACP help text at modelCommand.ts:348 doesn't mention the new --default flag:
Use "/model <model-id>" to switch models or "/model --fast <model-id>" to set the fast model.
Users in non-interactive mode cannot discover that persistence now requires --default. Suggest updating to:
Use "/model <model-id>" to switch models (session only), "/model --default <model-id>" to persist, or "/model --fast <model-id>" to set the fast model.
[Suggestion] Missing test coverage for 2 new code paths:
modelCommand.ts:327-332—/model --defaultwithout model ID error path has no testModelDialog.tsx:403-411—dkey Qwen OAuth guard branch is not tested (the Enter handler equivalent IS tested)
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| } | ||
|
|
||
| if ( | ||
| key.name === 'd' && |
There was a problem hiding this comment.
[Critical] The d key handler blocks AuthType.QWEN_OAUTH but does not check highlightedEntry.isRuntime. For a non-OAuth runtime model (e.g., isRuntime: true, authType: USE_OPENAI), two problems occur:
config.switchModel()is called with the barehighlightedEntry.model.id, whereas the Enter handler (handleSelect) correctly passes the full$runtime|authType|modelIdsnapshot ID for runtime models. This meansdand Enter switch to different targets for the same highlighted entry.persistDefaultModelSelection()writes the bare model ID to settings. Runtime models are ephemeral — the persisted value may not resolve on next startup, leaving the user with a broken default.
The Enter handler handles this correctly via selected.startsWith('$runtime|') logic. The d handler has no equivalent.
| key.name === 'd' && | |
| key.name === 'd' && | |
| !key.ctrl && | |
| !key.meta && | |
| !isFastModelMode && | |
| highlightedEntry && | |
| !highlightedEntry.isRuntime |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| modelName, | ||
| ); | ||
| if (isDefaultModelCommand) { | ||
| persistMainModelDefault(settings, authType, modelName); |
There was a problem hiding this comment.
[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:
- Call
switchMainModel(which may succeed with cached credentials) - Call
persistMainModelDefault, which writessecurity.auth.selectedType = QWEN_OAUTHto 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
| return modelArg; | ||
| } | ||
|
|
||
| function persistMainModelDefault( |
There was a problem hiding this comment.
[Suggestion] Several new code paths introduced by this PR lack test coverage:
persistMainModelDefaultwith provider-qualified model ID — theif (parsed.authType)branch (line 62) is never exercised. The only--defaulttest uses the unqualifiedqwen-max, which hits the else branch.--defaultwith nullsettings— theif (isDefaultModelCommand && !settings)guard (line 291) is a new conditional with no test.dkey error path — thecatchblock in ModelDialog.tsx (line 432) whenconfig.switchModelthrows is untested.dkey in fast model mode — no test renders<ModelDialog isFastModelMode={true} />and pressesdto 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
| ); | ||
| }, [highlightedValue, preferredKey, availableModelEntries]); | ||
|
|
||
| useKeypress( |
There was a problem hiding this comment.
[Suggestion] The d key handler (lines 396–455) is a hand-rolled copy of the Enter handler's switchModel call that diverges in at least two ways:
- No runtime model ID resolution — passes bare
highlightedEntry.model.idinstead of the$runtime|...snapshot ID that Enter uses for runtime models - No
ModelSlashCommandEventtelemetry — the Enter handler logs this event for non-runtime models, but thedhandler omits it
Any future fix to the Enter handler's model-switching logic must be manually mirrored here with no compiler or lint safety net. Consider extracting a shared performModelSwitch helper that handles the runtime-model branching, credential guards, and telemetry logging — both handlers would call it, with the d handler additionally calling persistDefaultModelSelection on success.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| return; | ||
| } | ||
|
|
||
| persistDefaultModelSelection({ |
There was a problem hiding this comment.
[Suggestion] persistDefaultModelSelection is called after the try/catch block. If the settings write fails (disk I/O error, permission error), the exception escapes as an unhandled promise rejection — the session model is already switched but the default is not persisted, with no user-visible error.
Consider wrapping persistDefaultModelSelection in the existing try/catch or adding a separate one:
try {
await config.switchModel(...);
logModelSlashCommand(...);
persistDefaultModelSelection({...});
} catch (e) {
setErrorMessage(...);
}— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| if (highlightedEntry.authType === AuthType.QWEN_OAUTH) { | ||
| setErrorMessage( | ||
| t( | ||
| 'Qwen OAuth free tier was discontinued on 2026-04-15. Please select a model from another provider or run /auth to switch.', |
There was a problem hiding this comment.
[Suggestion] This hardcoded string differs from qwenOAuthDiscontinuedMessage() in modelCommand.ts — the dialog version appends "or run /auth to switch." while the command version does not. Users get different guidance depending on the path.
Consider reusing qwenOAuthDiscontinuedMessage() or extracting a shared constant to keep the messages in sync.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| { | ||
| value: '--default', | ||
| description: t('Persist the selected model as the default'), | ||
| }, |
There was a problem hiding this comment.
[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
|
|
||
| await config.switchModel(currentAuthType, modelArg, undefined); | ||
| // Unqualified model ids belong to the currently active auth provider. | ||
| persistSetting(settings, 'security.auth.selectedType', currentAuthType); |
There was a problem hiding this comment.
[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
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Nice to have] 5 new i18n keys (Default model, Persist the selected model as the default, Usage: /model --default <model-id>, Enter to select, d to set default, ↑↓ to navigate, Esc to close, and the description update) are added to en.js, zh.js, and zh-TW.js but missing from ca.js, de.js, fr.js, ja.js, pt.js, and ru.js. The t() fallback shows English keys verbatim — no runtime error, but non-English users see untranslated text.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| } catch (e) { | ||
| const baseErrorMessage = e instanceof Error ? e.message : String(e); | ||
| setErrorMessage( | ||
| `Failed to set default model to '${highlightedEntry.model.id}'.\n\n${baseErrorMessage}`, |
There was a problem hiding this comment.
[Suggestion] When config.switchModel() succeeds but persistDefaultModelSelection() fails, this error message says "Failed to set default model" — implying the entire operation failed. In reality, the session model already switched successfully. Users may restart expecting the old model, see the old default, and file "model switching is broken" bugs.
Consider clarifying partial success:
| `Failed to set default model to '${highlightedEntry.model.id}'.\n\n${baseErrorMessage}`, | |
| `Switched to '${highlightedEntry.model.id}' for this session, but failed to persist as default.\n\n${baseErrorMessage}`, |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| messageType: 'info', | ||
| content: t('Model') + ': ' + effectiveModelName, | ||
| content: | ||
| (isDefaultModelCommand ? t('Default model') : t('Model')) + |
There was a problem hiding this comment.
[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.
| (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
| 'model.name', | ||
| 'qwen-max', | ||
| 'gpt-4', | ||
| ); |
There was a problem hiding this comment.
[Critical] This test for unqualified --default gpt-4 only asserts model.name but not security.auth.selectedType. The persistMainModelDefault function (line 70) also writes security.auth.selectedType for unqualified IDs. If that line were accidentally deleted, this test would still pass.
The provider-qualified test below correctly asserts both settings.
| ); | |
| expect(setValue).toHaveBeenCalledWith( | |
| expect.any(String), | |
| 'model.name', | |
| 'gpt-4', | |
| ); | |
| expect(setValue).toHaveBeenCalledWith( | |
| expect.any(String), | |
| 'security.auth.selectedType', | |
| AuthType.USE_OPENAI, | |
| ); |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| id: runtimeSnapshotId, | ||
| })), | ||
| getAllConfiguredModels: vi.fn(() => [ | ||
| { |
There was a problem hiding this comment.
[Suggestion] The d key handler checks !key.meta (ModelDialog.tsx line 407), but only the Ctrl+D guard is tested. On macOS, Cmd+D has system-level meanings. Add a test mirroring this one with { name: 'd', ctrl: false, meta: true, ... } to verify the Meta+D guard.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| }); | ||
| } catch (e) { | ||
| const baseErrorMessage = e instanceof Error ? e.message : String(e); | ||
| setErrorMessage( |
There was a problem hiding this comment.
[Suggestion] After a successful d key press, persistDefaultModelSelection adds a history item to historyManager, but the dialog stays open with no visible in-dialog confirmation. The history item renders in the terminal scrollback above the dialog — invisible while the dialog is open.
Users pressing d may think nothing happened and press Enter, inadvertently overriding the default they just set. Consider setting a transient success message or closing the dialog on d (matching Enter behavior).
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| authType: AuthType; | ||
| modelId: string; | ||
| }): void { | ||
| persistModelSelection(settings, modelId); |
There was a problem hiding this comment.
[Suggestion] These two calls each trigger a separate settings.setValue() → computeMergedSettings() → saveSettings() cycle — two synchronous disk writes for what is logically a single atomic update. Same pattern in persistMainModelDefault at modelCommand.ts:64-65 and 70-71.
Consider batching both writes into a single saveSettings() call to halve disk I/O and eliminate the brief inconsistent state window between writes.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
|
|
||
| await config.switchModel(currentAuthType, modelArg, undefined); | ||
| // Unqualified model ids belong to the currently active auth provider. | ||
| persistSetting(settings, 'security.auth.selectedType', fallbackAuthType); |
There was a problem hiding this comment.
[Suggestion] persistMainModelDefault unconditionally writes security.auth.selectedType for unqualified model IDs. The old code only persisted selectedType for qualified models (those with explicit authType(modelId) syntax). This is a new side effect: /model --default gpt-4 now locks the auth type to settings, potentially overriding an env-var-driven resolution chain on next startup.
Consider only persisting selectedType when it differs from what settings already have, or document this behavior in the success message.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| } | ||
|
|
||
| function persistDefaultModelSelection({ | ||
| settings, |
There was a problem hiding this comment.
[Suggestion] After persistence logic was removed from handleModelSwitchSuccess (above), that function now only logs a history item — but its name implies it handles the full success workflow. A future engineer adding a new model-switch path might call it assuming it covers persistence, silently losing that behavior.
Consider renaming handleModelSwitchSuccess to logModelSwitchResult and adding a JSDoc noting that persistence is the caller's responsibility.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| content: `Current model: ${currentModel}\nUse "/model <model-id>" to switch models or "/model --fast <model-id>" to set the fast model.`, | ||
| content: | ||
| `Current model: ${currentModel}\n` + | ||
| 'Use "/model <model-id>" to switch models (session only), ' + |
There was a problem hiding this comment.
[Suggestion] These user-facing help strings are plain string literals not wrapped in t(), while nearby strings like t('Usage: /model --default <model-id>') correctly use i18n. Non-English locales will display this text in English.
Wrap the full string in t() and add corresponding entries to locale files.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| modelName, | ||
| ); | ||
| if (isDefaultModelCommand) { | ||
| persistMainModelDefault(settings, authType, modelName); |
There was a problem hiding this comment.
[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.
| 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
|
|
||
| setErrorMessage(null); | ||
| void (async () => { | ||
| if (!config) { |
There was a problem hiding this comment.
[Suggestion] When config is null, the d-key handler silently returns — no onClose(), no error message. The dialog stays open with zero feedback.
Compare with the Enter handler (handleSelect, line ~516) which calls onClose() when config is null. Consider adding an error message for consistency:
| if (!config) { | |
| if (!config) { | |
| setErrorMessage('Configuration not available.'); | |
| return; | |
| } |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| ); | ||
| }, [highlightedValue, preferredKey, availableModelEntries]); | ||
|
|
||
| useKeypress( |
There was a problem hiding this comment.
[Suggestion] This useKeypress callback is an inline arrow function that captures highlightedEntry (a useMemo derived from highlightedValue state). Every navigation keypress (↑/↓) mutates highlightedValue → produces a new highlightedEntry → new closure reference → useKeypress's useEffect tears down and re-subscribes.
Consider stabilizing with a ref pattern:
const highlightedEntryRef = useRef(highlightedEntry);
highlightedEntryRef.current = highlightedEntry;
const handleKeypress = useCallback(
(key: { name: string; ctrl: boolean; meta: boolean; shift: boolean }) => {
const entry = highlightedEntryRef.current;
// ... use `entry` instead of `highlightedEntry`
},
[isFastModelMode, onClose, config, settings, uiState],
);
useKeypress(handleKeypress, { isActive: true });— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| } | ||
|
|
||
| try { | ||
| persistDefaultModelSelection({ |
There was a problem hiding this comment.
[Suggestion] The d-key handler never calls logModelSwitchResult after a successful switch+persist. The Enter handler (line ~575) logs detailed model info to history (authType, base URL, masked API key). The d-key only logs a terse "Default model: <id>" via persistDefaultModelSelection.
Consider adding a logModelSwitchResult call after the persist succeeds for consistency with the Enter path:
const after = config.getContentGeneratorConfig?.() as ContentGeneratorConfig | undefined;
logModelSwitchResult({
uiState,
after,
effectiveAuthType: highlightedEntry.authType,
effectiveModelId: highlightedEntry.model.id,
isRuntime: false,
});— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| 'Esc to close': 'Esc to close', | ||
| 'Enter to select, ↑↓ to navigate, Esc to close': | ||
| 'Enter to select, ↑↓ to navigate, Esc to close', | ||
| 'Enter to select, d to set default, ↑↓ to navigate, Esc to close': |
There was a problem hiding this comment.
[Suggestion] The i18n key 'Enter to select, d to set default, ↑↓ to navigate, Esc to close' was added to en.js, zh.js, and zh-TW.js but is missing from 6 other locales: ca.js, de.js, fr.js, ja.js, pt.js, ru.js. All other new keys from this PR (Default model, session only, Persist the selected model..., Usage: /model --default..., Current model: {{model}}...) are present in all locales.
Non-CJK users will see the English fallback text in the dialog footer instead of a translated version.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| ]; | ||
| } else if (partialArg.trim()) { | ||
| const trimmedPartialArg = partialArg.trim(); | ||
| if (trimmedPartialArg.startsWith('--default ')) { |
There was a problem hiding this comment.
[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.
| 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 trimmedPartialArg = partialArg.trim(); | ||
| if (trimmedPartialArg.startsWith('--default ')) { | ||
| const modelPartial = trimmedPartialArg.replace('--default', '').trim(); | ||
| return getAvailableModelIds(context) |
There was a problem hiding this comment.
[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
| } | ||
|
|
||
| setErrorMessage(null); | ||
| void (async () => { |
There was a problem hiding this comment.
[Suggestion] The d-key handler launches an async IIFE with no reentrancy guard. Rapid double-press of d, or pressing d then Enter while config.switchModel() is in flight, can cause concurrent switch+persist operations. The Enter handler (handleSelect) is independently async with no shared lock.
Consider adding a useRef guard shared between both handlers:
const isSwitchingRef = useRef(false);
// In d-key branch:
if (isSwitchingRef.current) return;
isSwitchingRef.current = true;
void (async () => {
try { /* ... */ } finally { isSwitchingRef.current = false; }
})();Additionally, after config.switchModel() succeeds, the d-key path persists highlightedEntry.model.id and highlightedEntry.authType directly without reading back the effective config (unlike the Enter handler which calls config.getContentGeneratorConfig() to get effectiveModelId/effectiveAuthType). If switchModel resolves to a different effective model (aliasing, credential fallback), the persisted default may not match the active session model.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
LGTM ✅ The latest two commits address the major concerns from prior rounds: reentrancy guard via isSwitchingRef with proper finally cleanup, effective model persistence using config readback after switch, --fast completion support, QWEN_OAUTH guard on --default path, and i18n keys backfilled to all 9 locales. All 255 tests pass, tsc/eslint clean. Remaining observations (all low confidence, terminal-only): defense-in-depth around config readback vs user selection in d-key persist, unhandled promise rejection risk in fast-model setValue path, and minor completion/test gaps. — qwen-latest-series-invite-beta-v34 via Qwen Code /review
2 participants
Summary
/model <model-id>and normal model dialog selections now switch only the current session model instead of writingmodel.name/security.auth.selectedTypeto settings. Added/model --default <model-id>and adshortcut in the dialog to explicitly persist the highlighted model as the default.Validation
/model qwen-max,/model --default qwen-max, provider-qualified model switches, fast-model switches, normal dialog selection, and dialogddefault persistence.settings.setValue; explicit default paths still write settings; fast-model behavior remains persisted.npm run build && npm run typecheckalso passed. The existing vscode companion lint warning insrc/utils/editorGroupUtils.tswas still emitted during build, but it did not fail the command.Scope / Risk
/model <id>to permanently update the default now need/model --default <id>or the dialogdshortcut.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Fixes #4331