From c4489619bde2bb1f49eabd7690d74bdc4ebe0a5e Mon Sep 17 00:00:00 2001 From: wsp Date: Wed, 16 Sep 2026 16:23:29 +0800 Subject: [PATCH] fix(chat): resolve model selections by configuration ID Match configured model IDs exclusively in the selector and pre-send synchronization to prevent same-name models from switching accounts. Preserve primary/fast selectors and block sending when a pinned model ID is unavailable or ambiguous. Add regression coverage for model identity collisions, unavailable selections, context windows, and remote routing parameters. Fixes #3007 --- .../flow_chat/components/ModelSelector.tsx | 5 +- .../ModelSelectorProviderLevels.test.tsx | 23 +++++++ .../flow-chat-manager/MessageModule.test.ts | 48 ++++++++++++++ .../flow_chat/utils/modelResolution.test.ts | 64 ++++++++++++++++++- .../src/flow_chat/utils/modelResolution.ts | 16 +++-- src/web-ui/src/flow_chat/utils/modelSync.ts | 12 ++-- 6 files changed, 154 insertions(+), 14 deletions(-) diff --git a/src/web-ui/src/flow_chat/components/ModelSelector.tsx b/src/web-ui/src/flow_chat/components/ModelSelector.tsx index ba66cb2ecf..b736d514e3 100644 --- a/src/web-ui/src/flow_chat/components/ModelSelector.tsx +++ b/src/web-ui/src/flow_chat/components/ModelSelector.tsx @@ -841,6 +841,8 @@ export const ModelSelector: React.FC = ({ status = 'unconfigured'; } else if (availableModels.length === 0) { status = 'no-enabled-chat-model'; + } else if (!nativeModelResolution.model) { + status = 'target-model-unavailable'; } else if (catalogLoadState === 'error') { status = 'catalog-unavailable'; } else if (nativeModelResolution.recovered) { @@ -851,13 +853,14 @@ export const ModelSelector: React.FC = ({ return { status, - canSend: configLoadState === 'ready' && availableModels.length > 0, + canSend: configLoadState === 'ready' && nativeModelResolution.model !== null, }; }, [ allModels.length, availableModels.length, catalogLoadState, configLoadState, + nativeModelResolution.model, nativeModelResolution.recovered, ]); diff --git a/src/web-ui/src/flow_chat/components/ModelSelectorProviderLevels.test.tsx b/src/web-ui/src/flow_chat/components/ModelSelectorProviderLevels.test.tsx index 1f301aa392..22bee4ed85 100644 --- a/src/web-ui/src/flow_chat/components/ModelSelectorProviderLevels.test.tsx +++ b/src/web-ui/src/flow_chat/components/ModelSelectorProviderLevels.test.tsx @@ -517,6 +517,29 @@ describe('ModelSelector provider levels', () => { )).not.toBeNull(); }); + it('blocks sending with a missing pinned ID while keeping replacement models selectable', async () => { + const onAvailabilityChange = vi.fn(); + flowChatStoreMocks.sessions.set('missing-model-session', { + config: { agentType: 'Standard', modelName: 'removed-id' }, + }); + vi.mocked(configManager.getConfigs).mockResolvedValue({ + 'ai.models': CATALOG_MODELS, + 'ai.default_models': { primary: 'acme-fast' }, + 'ai.agent_model_defaults': { mode: 'primary' }, + }); + await act(async () => { + root.render(); + await Promise.resolve(); + }); + expect(onAvailabilityChange).toHaveBeenLastCalledWith({ + status: 'target-model-unavailable', canSend: false, + }); + await openMenu(); + await openProvider('provider-acme'); + expect(modelOption('acme-fast')).not.toBeNull(); + }); + it('distinguishes configured models from enabled chat models', async () => { const onAvailabilityChange = vi.fn(); vi.mocked(configManager.getConfigs).mockResolvedValue({ diff --git a/src/web-ui/src/flow_chat/services/flow-chat-manager/MessageModule.test.ts b/src/web-ui/src/flow_chat/services/flow-chat-manager/MessageModule.test.ts index 3a479a22ad..d924667e7b 100644 --- a/src/web-ui/src/flow_chat/services/flow-chat-manager/MessageModule.test.ts +++ b/src/web-ui/src/flow_chat/services/flow-chat-manager/MessageModule.test.ts @@ -980,6 +980,29 @@ describe('MessageModule detached dispatch', () => { }); describe('MessageModule model synchronization', () => { + function modelSyncContext(modelName: string) { + const session = { + sessionId: 'same-name-session', + config: { modelName, workspacePath: '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/remote/repo' }, + remoteConnectionId: 'ssh-1', remoteSshHost: 'example.test', + maxContextTokens: 32000, + }; + const context: any = { + flowChatStore: { + getSurfaceGeneration: () => 0, + getState: () => ({ sessions: new Map([[session.sessionId, session]]) }), + updateSessionModelName: vi.fn(), + updateSessionMaxContextTokens: vi.fn(), + }, + }; + return { context, session }; + } + + const sameNameModels = [ + { id: 'model-first', name: 'MOCK-8000', model_name: 'asdf', enabled: true, context_window: 32000 }, + { id: 'asdf', name: 'MOCK-8000-2', model_name: 'asdf', enabled: true, context_window: 64000 }, + ]; + beforeEach(() => { vi.clearAllMocks(); mockGetConfigs.mockResolvedValue({ @@ -993,6 +1016,31 @@ describe('MessageModule model synchronization', () => { mockUpdateSessionModel.mockResolvedValue(undefined); }); + it('sends the exact selected account ID with the original remote routing', async () => { + mockGetConfigs.mockResolvedValue({ + 'ai.models': sameNameModels, 'ai.default_models': { primary: 'model-first' }, + }); + const { context, session } = modelSyncContext('asdf'); + await syncSessionModelSelection(context, session.sessionId, 'Standard'); + expect(context.flowChatStore.updateSessionModelName).not.toHaveBeenCalled(); + expect(context.flowChatStore.updateSessionMaxContextTokens).toHaveBeenCalledWith(session.sessionId, 64000); + expect(mockUpdateSessionModel).toHaveBeenCalledWith(expect.objectContaining({ + modelName: 'asdf', remoteConnectionId: 'ssh-1', remoteSshHost: 'example.test', + workspacePath: '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/remote/repo', + })); + }); + + it.each(['MOCK-8000', 'asdf', 'removed-model'])('does not replace unavailable ID %s with a name match or Primary', async modelName => { + mockGetConfigs.mockResolvedValue({ + 'ai.models': [sameNameModels[0]], 'ai.default_models': { primary: 'model-first' }, + }); + const { context, session } = modelSyncContext(modelName); + await expect(syncSessionModelSelection(context, session.sessionId, 'Standard')) + .rejects.toThrow('model configuration ID'); + expect(context.flowChatStore.updateSessionModelName).not.toHaveBeenCalled(); + expect(mockUpdateSessionModel).not.toHaveBeenCalled(); + }); + it('keeps an explicit primary selector when synchronizing before send', async () => { const session = { sessionId: 'session-primary', diff --git a/src/web-ui/src/flow_chat/utils/modelResolution.test.ts b/src/web-ui/src/flow_chat/utils/modelResolution.test.ts index b85e228a35..ab181951ac 100644 --- a/src/web-ui/src/flow_chat/utils/modelResolution.test.ts +++ b/src/web-ui/src/flow_chat/utils/modelResolution.test.ts @@ -6,7 +6,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { aiApi } from '@/infrastructure/api/service-api/AIApi'; import { configManager } from '@/infrastructure/config/services/ConfigManager'; import { setRecentReasoningPreset } from './reasoningPresets'; -import { resolveReasoningPresetForSessionCreation } from './modelResolution'; +import { + getModelMaxTokens, + resolveModelReference, + resolveModelSelection, + resolveReasoningPresetForSessionCreation, +} from './modelResolution'; +import type { AIModelConfig } from '@/infrastructure/config/types'; vi.mock('@/infrastructure/api/service-api/AIApi', () => ({ aiApi: { getModelCatalog: vi.fn() }, @@ -16,6 +22,62 @@ vi.mock('@/infrastructure/config/services/ConfigManager', () => ({ configManager: { getConfigs: vi.fn() }, })); +describe('configured model identity', () => { + const first: AIModelConfig = { + id: 'model-first', name: 'MOCK-8000', model_name: 'asdf', + provider: 'openai', base_url: 'https://example.test', enabled: true, + category: 'general_chat', capabilities: ['text_chat'], context_window: 32000, + metadata: { provider_instance_id: 'account-1' }, + }; + const selected: AIModelConfig = { + ...first, id: 'asdf', name: 'MOCK-8000-2', context_window: 64000, + metadata: { provider_instance_id: 'account-2' }, + }; + const models = [first, selected]; + + it.each([models, [selected, first]])('preserves the selected account regardless of catalog order: %j', (...ordered) => { + const result = resolveModelSelection({ models: ordered, sessionModelId: 'asdf' }); + expect(result).toMatchObject({ + model: selected, selectorId: 'asdf', concreteModelId: 'asdf', + source: 'session', recovered: false, + }); + }); + + it('does not match a display name even when it equals another config ID', () => { + expect(resolveModelReference([{ ...first, name: 'asdf' }, selected], 'asdf')).toBe(selected); + expect(resolveModelReference(models, 'MOCK-8000-2')).toBeNull(); + expect(resolveModelReference([first], 'asdf')).toBeNull(); + }); + + it.each(['primary', 'fast'])('resolves the %s alias through the exact configured ID', selector => { + expect(resolveModelSelection({ + models, sessionModelId: selector, defaultModels: { primary: 'asdf', fast: 'asdf' }, + })).toMatchObject({ model: selected, selectorId: selector, concreteModelId: 'asdf' }); + }); + + it('keeps Fast fallback to the configured Primary ID', () => { + expect(resolveModelReference(models, 'fast', { fast: 'missing', primary: 'asdf' })).toBe(selected); + }); + + it.each(['missing', 'MOCK-8000-2'])('marks an unavailable pinned reference %s without choosing another account', sessionModelId => { + expect(resolveModelSelection({ models, sessionModelId, defaultModels: { primary: 'model-first' } })) + .toEqual({ model: null, source: 'session', recovered: true }); + }); + + it('rejects disabled, missing-ID and duplicate-ID entries', () => { + expect(resolveModelReference([first, { ...selected, enabled: false }], 'asdf')).toBeNull(); + expect(resolveModelSelection({ models: [{ ...selected, id: undefined }] }).model).toBeNull(); + expect(resolveModelReference([selected, { ...first, id: 'asdf' }], 'asdf')).toBeNull(); + }); + + it('uses the selected account context window', async () => { + vi.mocked(configManager.getConfigs).mockResolvedValue({ + 'ai.models': models, 'ai.default_models': { primary: 'model-first' }, + }); + await expect(getModelMaxTokens('asdf')).resolves.toBe(64000); + }); +}); + describe('reasoning preset session creation resolution', () => { beforeEach(() => { const storage = new Map(); diff --git a/src/web-ui/src/flow_chat/utils/modelResolution.ts b/src/web-ui/src/flow_chat/utils/modelResolution.ts index 60391bbe07..ad10ffeba8 100644 --- a/src/web-ui/src/flow_chat/utils/modelResolution.ts +++ b/src/web-ui/src/flow_chat/utils/modelResolution.ts @@ -50,9 +50,9 @@ export interface ModelSelectionResolution { function findSelectableModel(models: readonly AIModelConfig[], modelRef: string | null | undefined): AIModelConfig | null { const value = modelRef?.trim(); if (!value) return null; - return models.find(model => isSelectableTextChatModel(model) - && (model.id === value || model.name === value || model.model_name === value) - ) ?? null; + // Config IDs identify credentials; display and upstream names do not. + const matches = models.filter(model => isSelectableTextChatModel(model) && model.id === value); + return matches.length === 1 ? matches[0] : null; } function resolveModelForContextWindow( @@ -135,20 +135,24 @@ export function resolveModelSelection({ if (model) { const selectorId = ref === 'primary' || ref === 'fast' ? ref - : model.id?.trim() || model.model_name.trim(); + : model.id; return { model, selectorId, - concreteModelId: model.id?.trim() || model.model_name.trim(), + concreteModelId: model.id, source: candidate.source, recovered, }; } + // A pinned session must not silently switch accounts when its ID is unavailable. + if (candidate.source === 'session') { + return { model: null, source: 'session', recovered: true }; + } recovered = true; } const fallback = selectableModels[0]; - const concreteModelId = fallback.id?.trim() || fallback.model_name.trim(); + const concreteModelId = fallback.id; return { model: fallback, selectorId: concreteModelId, diff --git a/src/web-ui/src/flow_chat/utils/modelSync.ts b/src/web-ui/src/flow_chat/utils/modelSync.ts index c9adcf4f6f..07f4f01ebf 100644 --- a/src/web-ui/src/flow_chat/utils/modelSync.ts +++ b/src/web-ui/src/flow_chat/utils/modelSync.ts @@ -10,7 +10,7 @@ import { configManager } from '@/infrastructure/config/services/ConfigManager'; import type { AIModelConfig, AgentModelDefaultsConfig, DefaultModelsConfig } from '@/infrastructure/config/types'; import { createLogger } from '@/shared/utils/logger'; import type { FlowChatContext } from '../services/flow-chat-manager/types'; -import { getModelMaxTokens } from './modelResolution'; +import { getModelMaxTokens, resolveModelReference } from './modelResolution'; import { sessionProjectWorkspacePath } from './sessionWorkspace'; import { getActiveSurfaceScope, @@ -34,11 +34,11 @@ function normalizeModelSelection( return matchedModel ? value : 'primary'; } - const matchedModel = models.find(model => - model.enabled !== false - && (model.id === value || model.name === value || model.model_name === value), - ); - return matchedModel?.id || 'primary'; + const matchedModel = resolveModelReference(models, value); + if (!matchedModel?.id) { + throw new Error(`Unknown, disabled, or ambiguous model configuration ID: ${value}`); + } + return matchedModel.id; } export async function syncSessionModelSelection(