mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-07-09 17:19:02 +00:00
* fix(model): disambiguate providers sharing a model id by baseUrl (#5173) When multiple modelProviders.openai entries share the same model id but have different baseUrl values, selecting one in the model picker was not remembered across restarts — startup always resolved to the first id match. The gap was at the persistence -> startup-resolution boundary: - persistModelSelection only saved model.name, dropping the baseUrl that distinguishes the providers. - resolveCliGenerationConfig matched providers by id alone (first hit). Fix (CLI only; runtime switching and ModelRegistry composite keys were already correct): - Add optional model.baseUrl setting paired with model.name. - ModelDialog persists the selected provider's baseUrl, and clears it when the selection has none (prevents a stale disambiguator). - /model <id> is an id-only switch, so it clears model.baseUrl too. - resolveCliGenerationConfig disambiguates by the persisted baseUrl when the model comes from settings, falling back to the first id match when the paired provider was edited/removed or no baseUrl was saved. This keeps the persistence layer consistent with ModelRegistry's existing id+baseUrl composite key and syncAfterAuthRefresh's exact-match path. * fix(model): clear model.baseUrl on ACP setModel and provider install Round-1 review follow-up: extend the "any writer of model.name must set or clear model.baseUrl" invariant to the remaining id-only persistence paths so a stale disambiguator from a prior model-picker selection can't restore the wrong provider on next launch. - ACP Session.setModel clears model.baseUrl alongside model.name. - Provider install plan (core) clears model.baseUrl when selecting a model id. * fix(auth): disambiguate provider by baseUrl in pre-flight auth validation Round-2 review follow-up: validateAuthMethod / hasApiKeyForAuth looked up the provider by model id only, so for duplicate-id configs the auth and Anthropic baseUrl checks could validate the first provider's credentials instead of the selected one. - findModelConfig now prefers the exact id+baseUrl match and falls back to the first id match (mirrors resolveCliGenerationConfig). - New resolveSelectedModel helper pairs the resolved/persisted model id with its baseUrl (runtime generationConfig first, then settings.model.{name,baseUrl}). - Thread the baseUrl into both the API-key and Anthropic-baseUrl lookups. * fix(auth): pair model with its own baseUrl; document cross-scope assumption Round-3 review follow-up: - resolveSelectedModel no longer falls back to settings.model.baseUrl when a live Config is present, so a runtime-selected model is never paired with a stale persisted baseUrl from a previous selection. - Document the merged-settings assumption at the resolver disambiguation site: every writer pairs model.name + model.baseUrl in the same scope, and the id-only fallback bounds the blast radius if a hand-edited config desyncs them across scopes. * fix(model): use empty-string tombstone to clear model.baseUrl across scopes Round-4 review follow-up: clearing model.baseUrl by writing undefined drops the key from JSON, so a higher-priority scope's clear could not override a stale model.baseUrl left in a lower-priority scope — the stale disambiguator would resurface on merge and re-select the wrong duplicate provider after restart (e.g. pick a provider in user scope, then /model <id> in a workspace that has its own modelProviders). All clear sites (model picker, /model, ACP setModel, provider install) now write an empty-string tombstone, which is a present value that overrides lower scopes on merge. The resolver and auth lookup already treat '' as "no disambiguator". * fix(acp): clear model.baseUrl when setCoreValue sets model.name Round-5 review follow-up: the generic qwen/settings/setCoreValue RPC (ACP/serve) can change model.name directly without touching model.baseUrl, leaving a stale disambiguator from a prior model-picker selection that would re-select the wrong duplicate provider on next launch. Clear it with the same empty-string tombstone used by the other id-only model.name writers. * fix(model): warn on silent provider fallback; pair baseUrl with resolved config Address review comments: - Emit a warning when the persisted model.baseUrl no longer matches any provider and resolution falls back to the first id match (thread 1 [Critical]). - Source effectiveBaseUrl from the resolved config (after?.baseUrl) rather than the picker entry alone, so the persisted (model.name, model.baseUrl) pair stays consistent even if switchModel transforms the id (thread 2 [Critical]). * test(model): cover effectiveBaseUrl fallback to picker entry baseUrl Add a ModelDialog test where switchModel succeeds but getContentGeneratorConfig returns a config without baseUrl, exercising the 'after?.baseUrl ?? selectedEntry?.model.baseUrl' fallback. Without coverage, a regression there would silently write an empty-string tombstone and resolve the wrong same-id provider on next launch.
This commit is contained in:
jinye
GitHub
parent
c9f4b53f2e
commit
4b9f597fa8
16 changed files with 525 additions and 14 deletions
|
|
@ -2520,6 +2520,28 @@ describe('QwenAgent MCP SSE/HTTP support', () => {
|
|||
await agentPromise;
|
||||
});
|
||||
|
||||
it('qwen/settings/setCoreValue clears model.baseUrl when setting model.name', async () => {
|
||||
const settings = makeCoreSettings();
|
||||
const { agent, agentPromise } = await bootCoreSettingsAgent(settings);
|
||||
|
||||
await agent.extMethod('qwen/settings/setCoreValue', {
|
||||
scope: 'user',
|
||||
key: 'model.name',
|
||||
value: 'qwen3.7-max',
|
||||
});
|
||||
|
||||
expect(settings.setValue).toHaveBeenCalledWith(
|
||||
'User',
|
||||
'model.name',
|
||||
'qwen3.7-max',
|
||||
);
|
||||
// Id-only selection must clear the paired baseUrl disambiguator (tombstone).
|
||||
expect(settings.setValue).toHaveBeenCalledWith('User', 'model.baseUrl', '');
|
||||
|
||||
mockConnectionState.resolve();
|
||||
await agentPromise;
|
||||
});
|
||||
|
||||
it('qwen/settings/getCore excludes untrusted workspace integrations from merged view', async () => {
|
||||
const settings = makeCoreSettings();
|
||||
(settings as { isTrusted: boolean }).isTrusted = false;
|
||||
|
|
|
|||
|
|
@ -6335,6 +6335,13 @@ class QwenAgent implements Agent {
|
|||
);
|
||||
const scope = toSettingsScope(params['scope']);
|
||||
settings.setValue(scope, key, normalizedValue);
|
||||
if (settingKey === 'model.name') {
|
||||
// Selecting a model by id here can't disambiguate providers that
|
||||
// share that id, so clear the paired baseUrl disambiguator left by a
|
||||
// previous model-picker selection. Empty-string tombstone overrides a
|
||||
// lower-scope value on merge (undefined would be dropped from JSON).
|
||||
settings.setValue(scope, 'model.baseUrl', '');
|
||||
}
|
||||
if (
|
||||
settingKey === 'general.outputLanguage' &&
|
||||
typeof normalizedValue === 'string' &&
|
||||
|
|
|
|||
|
|
@ -719,6 +719,12 @@ describe('Session', () => {
|
|||
'model.name',
|
||||
'qwen3-coder-plus',
|
||||
);
|
||||
// Id-only switch must clear any stale baseUrl disambiguator (tombstone).
|
||||
expect(mockSettings.setValue).toHaveBeenCalledWith(
|
||||
SettingScope.User,
|
||||
'model.baseUrl',
|
||||
'',
|
||||
);
|
||||
expect(mockSettings.setValue).toHaveBeenCalledWith(
|
||||
SettingScope.User,
|
||||
'security.auth.selectedType',
|
||||
|
|
|
|||
|
|
@ -2987,6 +2987,12 @@ export class Session implements SessionContext {
|
|||
if (options.persistDefault ?? true) {
|
||||
const persistScope = getPersistScopeForModelSelection(this.settings);
|
||||
this.settings.setValue(persistScope, 'model.name', parsed.modelId);
|
||||
// Id-only switch: clear any baseUrl disambiguator left by a previous
|
||||
// model-picker selection so the next launch resolves to this provider,
|
||||
// not a stale one sharing the same model id. Empty-string tombstone so
|
||||
// the clear overrides a lower-scope value on merge (undefined is dropped
|
||||
// from JSON and would not override).
|
||||
this.settings.setValue(persistScope, 'model.baseUrl', '');
|
||||
this.settings.setValue(
|
||||
persistScope,
|
||||
'security.auth.selectedType',
|
||||
|
|
|
|||
|
|
@ -34,6 +34,8 @@ describe('validateAuthMethod', () => {
|
|||
delete process.env['ANTHROPIC_API_KEY'];
|
||||
delete process.env['ANTHROPIC_BASE_URL'];
|
||||
delete process.env['GOOGLE_API_KEY'];
|
||||
delete process.env['IDEALAB_KEY'];
|
||||
delete process.env['TOKEN_PLAN_KEY'];
|
||||
});
|
||||
|
||||
it('should return null for USE_OPENAI with default env key', () => {
|
||||
|
|
@ -75,6 +77,67 @@ describe('validateAuthMethod', () => {
|
|||
expect(validateAuthMethod(AuthType.USE_OPENAI)).toBeNull();
|
||||
});
|
||||
|
||||
it('disambiguates by settings.model.baseUrl when providers share a model id', () => {
|
||||
// Two providers with the same id; the persisted baseUrl selects the second.
|
||||
// Only the second provider's env key is set, so validation passes only if
|
||||
// the lookup honors baseUrl rather than matching the first id entry.
|
||||
vi.mocked(settings.loadSettings).mockReturnValue({
|
||||
merged: {
|
||||
model: {
|
||||
name: 'qwen3.7-max',
|
||||
baseUrl: 'https://idealab.example.com/v1',
|
||||
},
|
||||
modelProviders: {
|
||||
openai: [
|
||||
{
|
||||
id: 'qwen3.7-max',
|
||||
baseUrl: 'https://token-plan.example.com/v1',
|
||||
envKey: 'TOKEN_PLAN_KEY',
|
||||
},
|
||||
{
|
||||
id: 'qwen3.7-max',
|
||||
baseUrl: 'https://idealab.example.com/v1',
|
||||
envKey: 'IDEALAB_KEY',
|
||||
},
|
||||
],
|
||||
},
|
||||
},
|
||||
} as unknown as ReturnType<typeof settings.loadSettings>);
|
||||
process.env['IDEALAB_KEY'] = 'idealab-key';
|
||||
|
||||
expect(validateAuthMethod(AuthType.USE_OPENAI)).toBeNull();
|
||||
});
|
||||
|
||||
it('reports the selected provider env key when providers share a model id', () => {
|
||||
vi.mocked(settings.loadSettings).mockReturnValue({
|
||||
merged: {
|
||||
model: {
|
||||
name: 'qwen3.7-max',
|
||||
baseUrl: 'https://idealab.example.com/v1',
|
||||
},
|
||||
modelProviders: {
|
||||
openai: [
|
||||
{
|
||||
id: 'qwen3.7-max',
|
||||
baseUrl: 'https://token-plan.example.com/v1',
|
||||
envKey: 'TOKEN_PLAN_KEY',
|
||||
},
|
||||
{
|
||||
id: 'qwen3.7-max',
|
||||
baseUrl: 'https://idealab.example.com/v1',
|
||||
envKey: 'IDEALAB_KEY',
|
||||
},
|
||||
],
|
||||
},
|
||||
},
|
||||
} as unknown as ReturnType<typeof settings.loadSettings>);
|
||||
|
||||
// No env keys set → error must name the selected (IdeaLab) provider's key.
|
||||
const result = validateAuthMethod(AuthType.USE_OPENAI);
|
||||
expect(result).toContain('IDEALAB_KEY');
|
||||
expect(result).not.toContain('TOKEN_PLAN_KEY');
|
||||
});
|
||||
|
||||
it('should return error with custom envKey hint when modelProviders envKey is set but env var is missing', () => {
|
||||
vi.mocked(settings.loadSettings).mockReturnValue({
|
||||
merged: {
|
||||
|
|
|
|||
|
|
@ -25,8 +25,11 @@ const DEFAULT_ENV_KEYS: Record<string, string> = {
|
|||
|
||||
/**
|
||||
* Find model configuration from modelProviders by authType and modelId.
|
||||
* When multiple models share the same id (different baseUrls), returns the
|
||||
* first match. Callers that need an exact match should also compare baseUrl.
|
||||
* When a baseUrl is given, prefers the exact id+baseUrl match (disambiguating
|
||||
* providers that share a model id) and falls back to the first id match if the
|
||||
* paired provider was edited/removed. When no baseUrl is given, returns the
|
||||
* first id match. Mirrors resolveCliGenerationConfig so pre-flight auth
|
||||
* validation checks the same provider that startup resolution selects.
|
||||
*/
|
||||
function findModelConfig(
|
||||
modelProviders: ModelProvidersConfig | undefined,
|
||||
|
|
@ -44,11 +47,42 @@ function findModelConfig(
|
|||
}
|
||||
|
||||
if (baseUrl) {
|
||||
return models.find((m) => m.id === modelId && m.baseUrl === baseUrl);
|
||||
return (
|
||||
models.find((m) => m.id === modelId && m.baseUrl === baseUrl) ??
|
||||
models.find((m) => m.id === modelId)
|
||||
);
|
||||
}
|
||||
return models.find((m) => m.id === modelId);
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the selected model id and its paired baseUrl for provider lookup.
|
||||
* Prefers the runtime-resolved generation config (which folds in CLI args, env
|
||||
* vars, settings, and the selected provider), falling back to the persisted
|
||||
* settings.model.{name,baseUrl} when no Config is available yet (pre-flight).
|
||||
*/
|
||||
function resolveSelectedModel(
|
||||
settings: Settings,
|
||||
config?: Config,
|
||||
): { modelId: string | undefined; baseUrl: string | undefined } {
|
||||
const modelsConfig = config?.getModelsConfig();
|
||||
if (modelsConfig) {
|
||||
// A live Config is the source of truth: pair its model with its own
|
||||
// resolved baseUrl. Do NOT fall back to settings.model.baseUrl here — that
|
||||
// could pair the runtime-selected model with a stale persisted baseUrl from
|
||||
// a previous selection and validate a different duplicate-id provider.
|
||||
return {
|
||||
modelId: modelsConfig.getModel(),
|
||||
baseUrl: modelsConfig.getGenerationConfig()?.baseUrl,
|
||||
};
|
||||
}
|
||||
// Pre-flight (no Config yet): use the persisted selection as a paired unit.
|
||||
return {
|
||||
modelId: settings.model?.name,
|
||||
baseUrl: settings.model?.baseUrl,
|
||||
};
|
||||
}
|
||||
|
||||
function hasEnvValue(settings: Settings, envKey: string | undefined): boolean {
|
||||
if (!envKey) {
|
||||
return false;
|
||||
|
|
@ -80,12 +114,19 @@ function hasApiKeyForAuth(
|
|||
| ModelProvidersConfig
|
||||
| undefined;
|
||||
|
||||
// Use config.getModelsConfig().getModel() if available for accurate model ID resolution
|
||||
// that accounts for CLI args, env vars, and settings. Fall back to settings.model.name.
|
||||
const modelId = config?.getModelsConfig().getModel() ?? settings.model?.name;
|
||||
// Use config.getModelsConfig() if available for accurate model resolution
|
||||
// that accounts for CLI args, env vars, and settings. Fall back to the
|
||||
// persisted settings.model.{name,baseUrl}.
|
||||
const { modelId, baseUrl } = resolveSelectedModel(settings, config);
|
||||
|
||||
// Try to find model-specific envKey from modelProviders
|
||||
const modelConfig = findModelConfig(modelProviders, authType, modelId);
|
||||
// Try to find model-specific envKey from modelProviders, disambiguating by
|
||||
// baseUrl so duplicate-id providers resolve to the selected one.
|
||||
const modelConfig = findModelConfig(
|
||||
modelProviders,
|
||||
authType,
|
||||
modelId,
|
||||
baseUrl,
|
||||
);
|
||||
|
||||
// If a Config is available, prefer the API key already resolved into the
|
||||
// generation config. The unified resolver folds CLI flags (e.g.
|
||||
|
|
@ -226,10 +267,15 @@ export function validateAuthMethod(
|
|||
const modelProviders = settings.merged.modelProviders as
|
||||
| ModelProvidersConfig
|
||||
| undefined;
|
||||
// Use config.getModelsConfig().getModel() if available for accurate model ID
|
||||
const modelId =
|
||||
config?.getModelsConfig().getModel() ?? settings.merged.model?.name;
|
||||
const modelConfig = findModelConfig(modelProviders, authMethod, modelId);
|
||||
// Resolve the selected model + baseUrl so duplicate-id providers validate
|
||||
// the Anthropic baseUrl of the selected provider, not the first id match.
|
||||
const { modelId, baseUrl } = resolveSelectedModel(settings.merged, config);
|
||||
const modelConfig = findModelConfig(
|
||||
modelProviders,
|
||||
authMethod,
|
||||
modelId,
|
||||
baseUrl,
|
||||
);
|
||||
|
||||
if (modelConfig && !modelConfig.baseUrl) {
|
||||
return t(
|
||||
|
|
|
|||
|
|
@ -1121,6 +1121,16 @@ const SETTINGS_SCHEMA = {
|
|||
description: 'The model to use for conversations.',
|
||||
showInDialog: false,
|
||||
},
|
||||
baseUrl: {
|
||||
type: 'string',
|
||||
label: 'Model Base URL',
|
||||
category: 'Model',
|
||||
requiresRestart: false,
|
||||
default: undefined as string | undefined,
|
||||
description:
|
||||
'Base URL paired with model.name; disambiguates which provider to use when multiple modelProviders entries share the same model id.',
|
||||
showInDialog: false,
|
||||
},
|
||||
maxSessionTurns: {
|
||||
type: 'number',
|
||||
label: 'Max Session Turns',
|
||||
|
|
|
|||
|
|
@ -183,6 +183,14 @@ describe('modelCommand', () => {
|
|||
'model.name',
|
||||
'qwen-max',
|
||||
);
|
||||
// `/model <id>` is an id-only switch, so any baseUrl disambiguator left by
|
||||
// a previous model-picker selection must be cleared (empty-string tombstone)
|
||||
// to avoid resolving to a different provider on next launch.
|
||||
expect(setValue).toHaveBeenCalledWith(
|
||||
expect.any(String),
|
||||
'model.baseUrl',
|
||||
'',
|
||||
);
|
||||
expect(result).toEqual({
|
||||
type: 'message',
|
||||
messageType: 'info',
|
||||
|
|
|
|||
|
|
@ -49,11 +49,18 @@ async function switchMainModel(
|
|||
);
|
||||
persistSetting(settings, 'security.auth.selectedType', parsed.authType);
|
||||
persistSetting(settings, 'model.name', parsed.modelId);
|
||||
// `/model <id>` selects by id only, so clear any baseUrl disambiguator left
|
||||
// by a previous model-picker selection — otherwise next launch would
|
||||
// resolve to a different provider than this switch just chose. Use an
|
||||
// empty-string tombstone so the clear overrides a lower-scope value (an
|
||||
// undefined write is dropped from JSON and would not override on merge).
|
||||
persistSetting(settings, 'model.baseUrl', '');
|
||||
return parsed.modelId;
|
||||
}
|
||||
|
||||
await config.switchModel(currentAuthType, modelArg, undefined);
|
||||
persistSetting(settings, 'model.name', modelArg);
|
||||
persistSetting(settings, 'model.baseUrl', '');
|
||||
return modelArg;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -308,6 +308,13 @@ describe('<ModelDialog />', () => {
|
|||
'model.name',
|
||||
'gpt-4',
|
||||
);
|
||||
// The selected provider has no baseUrl, so the disambiguator must be
|
||||
// cleared with an empty-string tombstone (overrides any lower-scope value).
|
||||
expect(mockSettings.setValue).toHaveBeenCalledWith(
|
||||
SettingScope.User,
|
||||
'model.baseUrl',
|
||||
'',
|
||||
);
|
||||
expect(mockSettings.setValue).toHaveBeenCalledWith(
|
||||
SettingScope.User,
|
||||
'security.auth.selectedType',
|
||||
|
|
@ -316,6 +323,120 @@ describe('<ModelDialog />', () => {
|
|||
expect(props.onClose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('persists model.baseUrl alongside model.name when the selected provider has a baseUrl', async () => {
|
||||
const switchModel = vi.fn().mockResolvedValue(undefined);
|
||||
const { props, mockSettings } = renderComponent({}, {
|
||||
getModel: vi.fn(() => 'qwen3.7-max'),
|
||||
getAuthType: vi.fn(() => AuthType.USE_OPENAI),
|
||||
switchModel,
|
||||
getAllConfiguredModels: vi.fn(() => [
|
||||
{
|
||||
id: 'qwen3.7-max',
|
||||
label: '[Token Plan] qwen3.7-max',
|
||||
description: '',
|
||||
authType: AuthType.USE_OPENAI,
|
||||
baseUrl: 'https://token-plan.example.com/v1',
|
||||
envKey: 'TOKEN_PLAN_KEY',
|
||||
},
|
||||
{
|
||||
id: 'qwen3.7-max',
|
||||
label: '[IdeaLab] qwen3.7-max',
|
||||
description: '',
|
||||
authType: AuthType.USE_OPENAI,
|
||||
baseUrl: 'https://idealab.example.com/v1',
|
||||
envKey: 'IDEALAB_KEY',
|
||||
},
|
||||
]),
|
||||
getContentGeneratorConfig: vi.fn(() => ({
|
||||
authType: AuthType.USE_OPENAI,
|
||||
model: 'qwen3.7-max',
|
||||
baseUrl: 'https://idealab.example.com/v1',
|
||||
})),
|
||||
} as unknown as Partial<Config>);
|
||||
|
||||
const childOnSelect = mockedSelect.mock.calls[0][0].onSelect;
|
||||
// Select the IdeaLab entry (second provider with the same id).
|
||||
await childOnSelect(
|
||||
`${AuthType.USE_OPENAI}::qwen3.7-max\0https://idealab.example.com/v1`,
|
||||
);
|
||||
|
||||
expect(switchModel).toHaveBeenCalledWith(
|
||||
AuthType.USE_OPENAI,
|
||||
'qwen3.7-max',
|
||||
{
|
||||
baseUrl: 'https://idealab.example.com/v1',
|
||||
},
|
||||
);
|
||||
expect(mockSettings.setValue).toHaveBeenCalledWith(
|
||||
SettingScope.User,
|
||||
'model.name',
|
||||
'qwen3.7-max',
|
||||
);
|
||||
expect(mockSettings.setValue).toHaveBeenCalledWith(
|
||||
SettingScope.User,
|
||||
'model.baseUrl',
|
||||
'https://idealab.example.com/v1',
|
||||
);
|
||||
expect(props.onClose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('falls back to the picker entry baseUrl when switchModel does not propagate it', async () => {
|
||||
// Regression guard for the `after?.baseUrl ?? selectedEntry?.model.baseUrl`
|
||||
// fallback: if switchModel succeeds but getContentGeneratorConfig returns a
|
||||
// config WITHOUT baseUrl, the disambiguator must still be persisted from the
|
||||
// selected picker entry's baseUrl — otherwise an empty-string tombstone would
|
||||
// be written and the wrong same-id provider would resolve on next launch.
|
||||
const switchModel = vi.fn().mockResolvedValue(undefined);
|
||||
const { props, mockSettings } = renderComponent({}, {
|
||||
getModel: vi.fn(() => 'qwen3.7-max'),
|
||||
getAuthType: vi.fn(() => AuthType.USE_OPENAI),
|
||||
switchModel,
|
||||
getAllConfiguredModels: vi.fn(() => [
|
||||
{
|
||||
id: 'qwen3.7-max',
|
||||
label: '[Token Plan] qwen3.7-max',
|
||||
description: '',
|
||||
authType: AuthType.USE_OPENAI,
|
||||
baseUrl: 'https://token-plan.example.com/v1',
|
||||
envKey: 'TOKEN_PLAN_KEY',
|
||||
},
|
||||
{
|
||||
id: 'qwen3.7-max',
|
||||
label: '[IdeaLab] qwen3.7-max',
|
||||
description: '',
|
||||
authType: AuthType.USE_OPENAI,
|
||||
baseUrl: 'https://idealab.example.com/v1',
|
||||
envKey: 'IDEALAB_KEY',
|
||||
},
|
||||
]),
|
||||
// Resolved config has NO baseUrl, so `after?.baseUrl` is undefined and the
|
||||
// `?? selectedEntry?.model.baseUrl` fallback must supply the disambiguator.
|
||||
getContentGeneratorConfig: vi.fn(() => ({
|
||||
authType: AuthType.USE_OPENAI,
|
||||
model: 'qwen3.7-max',
|
||||
})),
|
||||
} as unknown as Partial<Config>);
|
||||
|
||||
const childOnSelect = mockedSelect.mock.calls[0][0].onSelect;
|
||||
// Select the IdeaLab entry (second provider with the same id).
|
||||
await childOnSelect(
|
||||
`${AuthType.USE_OPENAI}::qwen3.7-max\0https://idealab.example.com/v1`,
|
||||
);
|
||||
|
||||
expect(mockSettings.setValue).toHaveBeenCalledWith(
|
||||
SettingScope.User,
|
||||
'model.name',
|
||||
'qwen3.7-max',
|
||||
);
|
||||
// baseUrl comes from the picker entry, not the (baseUrl-less) resolved config.
|
||||
expect(mockSettings.setValue).toHaveBeenCalledWith(
|
||||
SettingScope.User,
|
||||
'model.baseUrl',
|
||||
'https://idealab.example.com/v1',
|
||||
);
|
||||
expect(props.onClose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('shows MiniMax-M3 image + video modality and 1M context details', () => {
|
||||
const { getByText } = renderComponent({}, {
|
||||
getModel: vi.fn(() => 'MiniMax-M3'),
|
||||
|
|
|
|||
|
|
@ -95,9 +95,16 @@ function maskApiKey(apiKey: string | undefined): string {
|
|||
function persistModelSelection(
|
||||
settings: ReturnType<typeof useSettings>,
|
||||
modelId: string,
|
||||
baseUrl?: string,
|
||||
): void {
|
||||
const scope = getPersistScopeForModelSelection(settings);
|
||||
settings.setValue(scope, 'model.name', modelId);
|
||||
// Persist the paired baseUrl so the correct provider is restored on next
|
||||
// launch when multiple providers share the same model id. When the selection
|
||||
// has no baseUrl, write an empty-string tombstone (not undefined): undefined
|
||||
// is dropped from JSON, so it would not override a stale model.baseUrl left
|
||||
// in a lower-priority scope, whereas '' is a present value that does.
|
||||
settings.setValue(scope, 'model.baseUrl', baseUrl ?? '');
|
||||
}
|
||||
|
||||
function persistAuthTypeSelection(
|
||||
|
|
@ -132,6 +139,7 @@ interface HandleModelSwitchSuccessParams {
|
|||
after: ContentGeneratorConfig | undefined;
|
||||
effectiveAuthType: AuthType | undefined;
|
||||
effectiveModelId: string;
|
||||
effectiveBaseUrl: string | undefined;
|
||||
isRuntime: boolean;
|
||||
}
|
||||
|
||||
|
|
@ -141,9 +149,10 @@ function handleModelSwitchSuccess({
|
|||
after,
|
||||
effectiveAuthType,
|
||||
effectiveModelId,
|
||||
effectiveBaseUrl,
|
||||
isRuntime,
|
||||
}: HandleModelSwitchSuccessParams): void {
|
||||
persistModelSelection(settings, effectiveModelId);
|
||||
persistModelSelection(settings, effectiveModelId, effectiveBaseUrl);
|
||||
if (effectiveAuthType) {
|
||||
persistAuthTypeSelection(settings, effectiveAuthType);
|
||||
}
|
||||
|
|
@ -553,6 +562,15 @@ export function ModelDialog({
|
|||
after,
|
||||
effectiveAuthType,
|
||||
effectiveModelId,
|
||||
// Persist the selected provider's baseUrl so the right provider is
|
||||
// restored next launch when several share the same id. Pair it with the
|
||||
// same resolved config that effectiveModelId comes from (`after`) so the
|
||||
// persisted (model.name, model.baseUrl) stays consistent even if
|
||||
// switchModel transforms the id; fall back to the picker entry's
|
||||
// baseUrl. Runtime models are keyed by snapshot id, so no disambiguator.
|
||||
effectiveBaseUrl: isRuntime
|
||||
? undefined
|
||||
: (after?.baseUrl ?? selectedEntry?.model.baseUrl),
|
||||
isRuntime,
|
||||
});
|
||||
onClose();
|
||||
|
|
|
|||
|
|
@ -444,6 +444,151 @@ describe('modelConfigUtils', () => {
|
|||
);
|
||||
});
|
||||
|
||||
describe('provider disambiguation by settings.model.baseUrl', () => {
|
||||
const tokenPlan: ProviderModelConfig = {
|
||||
id: 'qwen3.7-max',
|
||||
name: '[Token Plan] qwen3.7-max',
|
||||
baseUrl: 'https://token-plan.example.com/v1',
|
||||
envKey: 'TOKEN_PLAN_KEY',
|
||||
};
|
||||
const ideaLab: ProviderModelConfig = {
|
||||
id: 'qwen3.7-max',
|
||||
name: '[IdeaLab] qwen3.7-max',
|
||||
baseUrl: 'https://idealab.example.com/v1',
|
||||
envKey: 'IDEALAB_KEY',
|
||||
};
|
||||
|
||||
function mockResolved() {
|
||||
vi.mocked(resolveModelConfig).mockReturnValue({
|
||||
config: { model: 'qwen3.7-max', apiKey: '', baseUrl: '' },
|
||||
sources: {},
|
||||
warnings: [],
|
||||
});
|
||||
}
|
||||
|
||||
it('selects the provider matching the persisted baseUrl, not the first id match', () => {
|
||||
mockResolved();
|
||||
const settings = makeMockSettings({
|
||||
model: {
|
||||
name: 'qwen3.7-max',
|
||||
baseUrl: 'https://idealab.example.com/v1',
|
||||
},
|
||||
modelProviders: {
|
||||
[AuthType.USE_OPENAI]: [tokenPlan, ideaLab],
|
||||
},
|
||||
});
|
||||
|
||||
resolveCliGenerationConfig({
|
||||
argv: {},
|
||||
settings,
|
||||
selectedAuthType: AuthType.USE_OPENAI,
|
||||
});
|
||||
|
||||
// The IdeaLab provider (not the first-listed Token Plan) must be passed
|
||||
// to resolveModelConfig, which is what makes sources.baseUrl.kind become
|
||||
// 'modelProviders' and lets syncAfterAuthRefresh keep it after refresh.
|
||||
expect(vi.mocked(resolveModelConfig)).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ modelProvider: ideaLab }),
|
||||
);
|
||||
});
|
||||
|
||||
it('falls back to the first id match when no baseUrl is persisted (backward compat)', () => {
|
||||
mockResolved();
|
||||
const settings = makeMockSettings({
|
||||
model: { name: 'qwen3.7-max' },
|
||||
modelProviders: {
|
||||
[AuthType.USE_OPENAI]: [tokenPlan, ideaLab],
|
||||
},
|
||||
});
|
||||
|
||||
resolveCliGenerationConfig({
|
||||
argv: {},
|
||||
settings,
|
||||
selectedAuthType: AuthType.USE_OPENAI,
|
||||
});
|
||||
|
||||
expect(vi.mocked(resolveModelConfig)).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ modelProvider: tokenPlan }),
|
||||
);
|
||||
});
|
||||
|
||||
it('falls back to the first id match and emits a warning when the persisted baseUrl no longer matches any provider', () => {
|
||||
mockResolved();
|
||||
const settings = makeMockSettings({
|
||||
model: {
|
||||
name: 'qwen3.7-max',
|
||||
baseUrl: 'https://removed.example.com/v1',
|
||||
},
|
||||
modelProviders: {
|
||||
[AuthType.USE_OPENAI]: [tokenPlan, ideaLab],
|
||||
},
|
||||
});
|
||||
|
||||
const result = resolveCliGenerationConfig({
|
||||
argv: {},
|
||||
settings,
|
||||
selectedAuthType: AuthType.USE_OPENAI,
|
||||
});
|
||||
|
||||
expect(vi.mocked(resolveModelConfig)).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ modelProvider: tokenPlan }),
|
||||
);
|
||||
expect(result.warnings).toEqual(
|
||||
expect.arrayContaining([
|
||||
expect.stringContaining('https://removed.example.com/v1'),
|
||||
]),
|
||||
);
|
||||
});
|
||||
|
||||
it('treats an empty-string tombstone as no disambiguator (first id match)', () => {
|
||||
// A cleared selection persists model.baseUrl: '' so it can override a
|
||||
// stale lower-scope value on merge; the resolver must treat '' as "no
|
||||
// baseUrl" and fall back to the first id match rather than matching a
|
||||
// provider whose baseUrl is literally empty.
|
||||
mockResolved();
|
||||
const settings = makeMockSettings({
|
||||
model: { name: 'qwen3.7-max', baseUrl: '' },
|
||||
modelProviders: {
|
||||
[AuthType.USE_OPENAI]: [tokenPlan, ideaLab],
|
||||
},
|
||||
});
|
||||
|
||||
resolveCliGenerationConfig({
|
||||
argv: {},
|
||||
settings,
|
||||
selectedAuthType: AuthType.USE_OPENAI,
|
||||
});
|
||||
|
||||
expect(vi.mocked(resolveModelConfig)).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ modelProvider: tokenPlan }),
|
||||
);
|
||||
});
|
||||
|
||||
it('ignores the persisted baseUrl when the model comes from argv.model, not settings', () => {
|
||||
mockResolved();
|
||||
const settings = makeMockSettings({
|
||||
model: {
|
||||
name: 'something-else',
|
||||
baseUrl: 'https://idealab.example.com/v1',
|
||||
},
|
||||
modelProviders: {
|
||||
[AuthType.USE_OPENAI]: [tokenPlan, ideaLab],
|
||||
},
|
||||
});
|
||||
|
||||
resolveCliGenerationConfig({
|
||||
argv: { model: 'qwen3.7-max' },
|
||||
settings,
|
||||
selectedAuthType: AuthType.USE_OPENAI,
|
||||
});
|
||||
|
||||
// argv-resolved model uses id-only lookup → first match.
|
||||
expect(vi.mocked(resolveModelConfig)).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ modelProvider: tokenPlan }),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
it('strips a runtime snapshot prefix from settings.model.name (read-side self-heal)', () => {
|
||||
const modelProvider: ProviderModelConfig = {
|
||||
id: 'gpt-4o',
|
||||
|
|
|
|||
|
|
@ -157,11 +157,15 @@ export function resolveCliGenerationConfig(
|
|||
// Env vars are ONLY considered when neither argv.model nor settings.model.name is set.
|
||||
let resolvedModel: string | undefined;
|
||||
let sourceEnvVar: string | undefined;
|
||||
// Whether the model came from settings.model.name (vs argv/env). The persisted
|
||||
// settings.model.baseUrl disambiguator only applies to this case.
|
||||
let resolvedFromSettings = false;
|
||||
if (argv.model) {
|
||||
resolvedModel = argv.model;
|
||||
} else if (settings.model?.name) {
|
||||
// Self-heal configs already corrupted by older builds.
|
||||
resolvedModel = stripRuntimeSnapshotPrefix(settings.model.name);
|
||||
resolvedFromSettings = true;
|
||||
} else if (authType && AUTH_ENV_MODEL_VARS[authType]) {
|
||||
// Only check env vars for the current auth type
|
||||
for (const envVar of AUTH_ENV_MODEL_VARS[authType]) {
|
||||
|
|
@ -178,10 +182,44 @@ export function resolveCliGenerationConfig(
|
|||
// so the resolver correctly uses the settings-selected model (no override occurs).
|
||||
// The old candidate-loop code that fell through to OPENAI_MODEL is gone.
|
||||
let modelProvider: ProviderModelConfig | undefined;
|
||||
let disambiguationWarning: string | undefined;
|
||||
if (resolvedModel && authType && settings.modelProviders) {
|
||||
const providers = settings.modelProviders[authType];
|
||||
if (providers && Array.isArray(providers)) {
|
||||
modelProvider = providers.find((p) => p.id === resolvedModel);
|
||||
// When multiple providers share the same id, disambiguate by the
|
||||
// persisted settings.model.baseUrl (written by the model picker). This
|
||||
// only applies when the model itself came from settings.model.name.
|
||||
// Fall back to the first id match if the paired provider was edited or
|
||||
// removed (and for the legacy id-only case where no baseUrl was saved),
|
||||
// mirroring auth.ts:findModelConfig.
|
||||
//
|
||||
// Note: `settings` is already merged across user/workspace/system scopes.
|
||||
// Every writer of model.name (the picker, /model, ACP, provider install)
|
||||
// also writes model.baseUrl in the SAME scope — a real URL, or an empty
|
||||
// string tombstone when there is none. The tombstone matters because an
|
||||
// omitted key cannot override a stale model.baseUrl in a lower-priority
|
||||
// scope on merge, but '' (a present value) can. Empty string is treated
|
||||
// as "no disambiguator" here. The only remaining desync is a hand-edited
|
||||
// config that sets model.name in a higher scope with no baseUrl key at
|
||||
// all; the id-only fallback bounds the blast radius to a same-id provider.
|
||||
const persistedBaseUrl = settings.model?.baseUrl;
|
||||
if (resolvedFromSettings && persistedBaseUrl) {
|
||||
const exactMatch = providers.find(
|
||||
(p) => p.id === resolvedModel && p.baseUrl === persistedBaseUrl,
|
||||
);
|
||||
modelProvider =
|
||||
exactMatch ?? providers.find((p) => p.id === resolvedModel);
|
||||
// Surface the silent fallback: the paired provider was removed or its
|
||||
// baseUrl changed, so traffic now routes to a different same-id provider.
|
||||
if (!exactMatch && modelProvider) {
|
||||
disambiguationWarning =
|
||||
`Persisted model.baseUrl '${persistedBaseUrl}' no longer matches any provider ` +
|
||||
`for model '${resolvedModel}' (authType '${authType}'); using the first id match ` +
|
||||
`('${modelProvider.baseUrl ?? '(default baseUrl)'}'). Re-select the model to update it.`;
|
||||
}
|
||||
} else {
|
||||
modelProvider = providers.find((p) => p.id === resolvedModel);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -270,6 +308,7 @@ export function resolveCliGenerationConfig(
|
|||
sources: resolved.sources,
|
||||
warnings: [
|
||||
...resolved.warnings,
|
||||
...(disambiguationWarning ? [disambiguationWarning] : []),
|
||||
...(ignoredGenerationConfigWarning
|
||||
? [ignoredGenerationConfigWarning]
|
||||
: []),
|
||||
|
|
|
|||
|
|
@ -153,6 +153,9 @@ describe('applyProviderInstallPlan', () => {
|
|||
AuthType.USE_OPENAI,
|
||||
);
|
||||
expect(adapter.setValue).toHaveBeenCalledWith('model.name', 'new-model');
|
||||
// Id-only model selection must clear any stale baseUrl disambiguator
|
||||
// (empty-string tombstone overrides a lower-scope value on merge).
|
||||
expect(adapter.setValue).toHaveBeenCalledWith('model.baseUrl', '');
|
||||
expect(adapter.persist).toHaveBeenCalled();
|
||||
expect(reloadModelProviders).toHaveBeenCalledWith({
|
||||
[AuthType.USE_OPENAI]: [
|
||||
|
|
|
|||
|
|
@ -210,6 +210,12 @@ export async function applyProviderInstallPlan(
|
|||
currentStep = 'modelSelection';
|
||||
if (plan.modelSelection?.modelId) {
|
||||
settings.setValue('model.name', plan.modelSelection.modelId);
|
||||
// The plan selects by model id only, so clear any baseUrl disambiguator
|
||||
// left by a previous model-picker selection — otherwise the next launch
|
||||
// could resolve to a stale provider sharing this model id. Empty-string
|
||||
// tombstone so the clear overrides a lower-scope value on merge (an
|
||||
// undefined write is dropped from JSON and would not override).
|
||||
settings.setValue('model.baseUrl', '');
|
||||
}
|
||||
|
||||
// Provider state metadata
|
||||
|
|
|
|||
|
|
@ -474,6 +474,10 @@
|
|||
"description": "The model to use for conversations.",
|
||||
"type": "string"
|
||||
},
|
||||
"baseUrl": {
|
||||
"description": "Base URL paired with model.name; disambiguates which provider to use when multiple modelProviders entries share the same model id.",
|
||||
"type": "string"
|
||||
},
|
||||
"maxSessionTurns": {
|
||||
"description": "Maximum number of user/model/tool turns to keep in a session. -1 means unlimited.",
|
||||
"type": "number",
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue