mirror of
https://github.com/Gitlawb/openclaude.git
synced 2026-08-24 02:34:15 -05:00
* fix(mistral): show all configured models in model picker (#1360) * fix(mistral): use correct API model names and prepend selected model * chore(mistral): fix comment and normalize model ids for consistency * fix(mistral): remove retired models, fix descriptor ids, add multi-model test * fix(mistral): preserve original delimiter and fix codestral descriptor id * fix(mistral): revert codestral descriptor to match shared model registry * fix(mistral): revert profile mutation; align with documented contract persistActiveProviderProfileModel() is now a no-op that returns the active profile. Runtime model selection is a session-level choice handled by mainLoopModelOverride (set by onChangeAppState before this helper is called); the profile's model list should only change via an explicit provider edit, not as a side-effect of /model. This addresses the Copilot and jatmn review threads that flagged the prior prepend behavior: - contradicted the comment above the function stating session-level switching is owned by mainLoopModelOverride - caused unbounded list growth on rotation (A->B->C->D on a profile starting with A; B produced D; C; B; A; B) - used a separator inferred from a single-character substring of the model field that broke on mixed-separator inputs - the catalog also used inconsistent id naming Catalog ids are normalized to the mistral-* scheme (mistral-devstral, mistral-large, mistral-small, mistral-ministral-3b, mistral-codestral) to match the existing prefix convention. apiName and modelDescriptorId are unchanged so route metadata and descriptor lookups are unaffected. New coverage: - providerProfiles.test.ts: locks the no-op contract for single- and multi-model profiles (semicolon and comma separators) and the in-list pick path that returns the active profile unchanged. Resolves #1360
This commit is contained in:
@@ -49,6 +49,10 @@ export default defineGateway({
|
||||
models: [
|
||||
{ id: 'mistral-vibe-cli', apiName: 'mistral-vibe-cli-latest', label: 'Vibe CLI Latest', modelDescriptorId: 'mistral-vibe-cli-latest' },
|
||||
{ id: 'mistral-devstral', apiName: 'devstral-latest', label: 'Devstral Latest', modelDescriptorId: 'devstral-latest' },
|
||||
{ id: 'mistral-large', apiName: 'mistral-large-latest', label: 'Mistral Large Latest', modelDescriptorId: 'mistral-large-latest' },
|
||||
{ id: 'mistral-small', apiName: 'mistral-small-latest', label: 'Mistral Small Latest', modelDescriptorId: 'mistral-small-latest' },
|
||||
{ id: 'mistral-ministral-3b', apiName: 'ministral-3b-latest', label: 'Ministral 3B Latest', modelDescriptorId: 'ministral-3b-latest' },
|
||||
{ id: 'mistral-codestral', apiName: 'codestral-latest', label: 'Codestral Latest', modelDescriptorId: 'codestral' },
|
||||
],
|
||||
},
|
||||
usage: { supported: false },
|
||||
|
||||
@@ -1025,9 +1025,13 @@ describe('applyActiveProviderProfileFromConfig', () => {
|
||||
})
|
||||
|
||||
describe('persistActiveProviderProfileModel', () => {
|
||||
test('updates active profile model and current env for profile-managed sessions', async () => {
|
||||
// The runtime active-model selection is owned by mainLoopModelOverride
|
||||
// (set by onChangeAppState before this helper is called). This helper
|
||||
// intentionally no longer mutates the profile's model list — see the
|
||||
// docstring in providerProfiles.ts. Coverage below locks the no-op
|
||||
// contract for both single- and multi-model profiles.
|
||||
test('returns the active profile unchanged for a single-model profile', async () => {
|
||||
const {
|
||||
applyProviderProfileToProcessEnv,
|
||||
getProviderProfiles,
|
||||
persistActiveProviderProfileModel,
|
||||
} = await importFreshProviderProfileModules()
|
||||
@@ -1042,13 +1046,49 @@ describe('persistActiveProviderProfileModel', () => {
|
||||
providerProfiles: [activeProfile],
|
||||
activeProviderProfileId: activeProfile.id,
|
||||
}))
|
||||
applyProviderProfileToProcessEnv(activeProfile)
|
||||
|
||||
const updated = persistActiveProviderProfileModel('minimax-m2.5:cloud')
|
||||
|
||||
expect(updated?.id).toBe(activeProfile.id)
|
||||
expect(updated?.model).toBe('minimax-m2.5:cloud')
|
||||
expect(process.env.OPENAI_MODEL).toBe('minimax-m2.5:cloud')
|
||||
expect(updated?.model).toBe('kimi-k2.5:cloud')
|
||||
const saved = getProviderProfiles().find(
|
||||
(profile: ProviderProfile) => profile.id === activeProfile.id,
|
||||
)
|
||||
expect(saved?.model).toBe('kimi-k2.5:cloud')
|
||||
})
|
||||
|
||||
test('does not mutate multi-model mistral profile when chosen model is out of list', async () => {
|
||||
// Regression for #1360: the picker must never silently rewrite a
|
||||
// provider's configured model list. The active model is a session-level
|
||||
// choice handled by mainLoopModelOverride; the profile's model list
|
||||
// only changes via an explicit provider edit. An earlier
|
||||
// implementation prepended the chosen model to the list, which
|
||||
// contradicted the documented contract and grew the list unboundedly
|
||||
// on rotation.
|
||||
const {
|
||||
applyProviderProfileToProcessEnv,
|
||||
getProviderProfiles,
|
||||
persistActiveProviderProfileModel,
|
||||
} = await importFreshProviderProfileModules()
|
||||
const activeProfile = buildMistralProfile({
|
||||
id: 'saved_mistral',
|
||||
baseUrl: 'https://api.mistral.ai/v1',
|
||||
model: 'devstral-latest; mistral-small-latest',
|
||||
})
|
||||
|
||||
saveMockGlobalConfig(current => ({
|
||||
...current,
|
||||
providerProfiles: [activeProfile],
|
||||
activeProviderProfileId: activeProfile.id,
|
||||
}))
|
||||
applyProviderProfileToProcessEnv(activeProfile)
|
||||
|
||||
const updated = persistActiveProviderProfileModel('mistral-large-latest')
|
||||
|
||||
expect(updated?.id).toBe(activeProfile.id)
|
||||
// The configured list is preserved verbatim regardless of the chosen
|
||||
// model being in or out of the list.
|
||||
expect(updated?.model).toBe('devstral-latest; mistral-small-latest')
|
||||
expect(process.env.CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED_ID).toBe(
|
||||
activeProfile.id,
|
||||
)
|
||||
@@ -1056,17 +1096,20 @@ describe('persistActiveProviderProfileModel', () => {
|
||||
const saved = getProviderProfiles().find(
|
||||
(profile: ProviderProfile) => profile.id === activeProfile.id,
|
||||
)
|
||||
expect(saved?.model).toBe('minimax-m2.5:cloud')
|
||||
expect(saved?.model).toBe('devstral-latest; mistral-small-latest')
|
||||
})
|
||||
|
||||
test('does not mutate process env when session is not profile-managed', async () => {
|
||||
test('preserves comma-separated multi-model list when chosen model is already a member', async () => {
|
||||
// Switching between models already in the list is a session-level
|
||||
// choice. The list itself must be preserved exactly as configured.
|
||||
const {
|
||||
getProviderProfiles,
|
||||
persistActiveProviderProfileModel,
|
||||
} = await importFreshProviderProfileModules()
|
||||
const activeProfile = buildProfile({
|
||||
id: 'saved_openai',
|
||||
model: 'kimi-k2.5:cloud',
|
||||
const activeProfile = buildMistralProfile({
|
||||
id: 'saved_mistral',
|
||||
baseUrl: 'https://api.mistral.ai/v1',
|
||||
model: 'devstral-latest, mistral-small-latest, mistral-large-latest',
|
||||
})
|
||||
|
||||
saveMockGlobalConfig(current => ({
|
||||
@@ -1075,18 +1118,17 @@ describe('persistActiveProviderProfileModel', () => {
|
||||
activeProviderProfileId: activeProfile.id,
|
||||
}))
|
||||
|
||||
process.env.CLAUDE_CODE_USE_OPENAI = '1'
|
||||
process.env.OPENAI_MODEL = 'cli-model'
|
||||
delete process.env.CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED
|
||||
delete process.env.CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED_ID
|
||||
const updated = persistActiveProviderProfileModel('mistral-small-latest')
|
||||
|
||||
persistActiveProviderProfileModel('minimax-m2.5:cloud')
|
||||
|
||||
expect(process.env.OPENAI_MODEL).toBe('cli-model')
|
||||
expect(updated?.model).toBe(
|
||||
'devstral-latest, mistral-small-latest, mistral-large-latest',
|
||||
)
|
||||
const saved = getProviderProfiles().find(
|
||||
(profile: ProviderProfile) => profile.id === activeProfile.id,
|
||||
)
|
||||
expect(saved?.model).toBe('minimax-m2.5:cloud')
|
||||
expect(saved?.model).toBe(
|
||||
'devstral-latest, mistral-small-latest, mistral-large-latest',
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -858,56 +858,20 @@ export function persistActiveProviderProfileModel(
|
||||
return null
|
||||
}
|
||||
|
||||
// If the model is already part of the profile's model list, don't
|
||||
// overwrite the field. This preserves comma-separated model lists like
|
||||
// "glm-4.5, glm-4.7". Switching between models in the list is a
|
||||
// session-level choice handled by mainLoopModelOverride, not a profile
|
||||
// edit — the profile's model list should only change via explicit edit.
|
||||
const existingModels = parseModelList(activeProfile.model)
|
||||
if (existingModels.includes(nextModel)) {
|
||||
return activeProfile
|
||||
}
|
||||
|
||||
saveGlobalConfig(current => {
|
||||
const currentProfiles = getProviderProfiles(current)
|
||||
const profileIndex = currentProfiles.findIndex(
|
||||
profile => profile.id === activeProfile.id,
|
||||
)
|
||||
|
||||
if (profileIndex < 0) {
|
||||
return current
|
||||
}
|
||||
|
||||
const currentProfile = currentProfiles[profileIndex]
|
||||
if (currentProfile.model === nextModel) {
|
||||
return current
|
||||
}
|
||||
|
||||
const nextProfiles = [...currentProfiles]
|
||||
nextProfiles[profileIndex] = {
|
||||
...currentProfile,
|
||||
model: nextModel,
|
||||
}
|
||||
|
||||
return {
|
||||
...current,
|
||||
providerProfiles: nextProfiles,
|
||||
}
|
||||
})
|
||||
|
||||
const resolvedProfile = getActiveProviderProfile()
|
||||
if (!resolvedProfile || resolvedProfile.id !== activeProfile.id) {
|
||||
return null
|
||||
}
|
||||
|
||||
if (
|
||||
process.env[PROFILE_ENV_APPLIED_FLAG] === '1' &&
|
||||
trimOrUndefined(process.env[PROFILE_ENV_APPLIED_ID]) === resolvedProfile.id
|
||||
) {
|
||||
applyProviderProfileToProcessEnv(resolvedProfile)
|
||||
}
|
||||
|
||||
return resolvedProfile
|
||||
// Runtime model selection is a session-level choice handled by
|
||||
// mainLoopModelOverride (see src/hooks/useMainLoopModel.ts), not a
|
||||
// profile edit. Whether the chosen model is already part of the
|
||||
// profile's list or not, do NOT mutate profile.model here:
|
||||
// - if it IS in the list, the list is already correct (no-op)
|
||||
// - if it ISN'T, the user picked an out-of-list model for the
|
||||
// session and the profile's list should only change via an
|
||||
// explicit provider edit, not by side-effect of /model.
|
||||
// An earlier implementation prepended out-of-list models to the
|
||||
// profile, which (a) contradicted this contract, (b) caused
|
||||
// unbounded list growth on rotation, and (c) used a separator
|
||||
// inferred from a single-character substring of the model field
|
||||
// that broke on mixed-separator inputs.
|
||||
return activeProfile
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user