Fix chat model picker hiding models when metadata.id collides across vendors#312910
Fix chat model picker hiding models when metadata.id collides across vendors#312910fishcharlie wants to merge 1 commit intomicrosoft:mainfrom
metadata.id collides across vendors#312910Conversation
…vendors The 'placed' set in buildModelPickerItems tracked both full model identifiers and bare metadata.id values. Once any model was placed in the Auto / Promoted section, every other model from a different vendor that happened to share the same metadata.id was filtered out of the 'Other Models' section — and was therefore unreachable from the chat model picker entirely (search included). Track promoted models strictly by identifier. Control-manifest entries with no concrete matching model continue to dedupe via their entry id (which lives in a separate id-namespace from per-vendor metadata.id values). Fixes microsoft#312908
There was a problem hiding this comment.
Pull request overview
Fixes an issue in the chat model picker where models could become unreachable when different vendors share the same metadata.id, by ensuring “placed/promoted” tracking and “Other Models” filtering are based on unique model identifiers.
Changes:
- Stop tracking promoted models by bare
metadata.id; track only by concreteidentifier(and manifest-only placeholder IDs). - Update “Other Models” filtering to exclude only models whose
identifieris already placed. - Add tests covering vendor
metadata.idcollisions to prevent regressions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/vs/workbench/contrib/chat/browser/widget/input/chatModelPicker.ts |
Updates promotion/placement bookkeeping and the “Other Models” filter to avoid dropping models due to cross-vendor metadata.id collisions. |
src/vs/workbench/contrib/chat/test/browser/widget/input/chatModelPicker.test.ts |
Adds regression tests for metadata.id collisions across vendors in the model picker. |
| const labels = getActionLabels(items); | ||
| assert.ok(labels.includes('gemma:7b'), 'Featured/recent Ollama model should appear'); | ||
| assert.ok(labels.includes('gemma:7b (my-endpoint)'), 'BYOK model with colliding metadata.id should still appear in Other Models'); |
There was a problem hiding this comment.
This test can pass even if the original bug (filtering Other Models by metadata.id) regresses, because the featured path here can resolve to (and promote) the BYOK model via modelsByMetadataId (it keeps only one model per metadata.id). In that case the BYOK model would still be visible (as featured) even though it might be incorrectly filtered out of Other Models. Consider restructuring/asserting sections so the test guarantees one model is promoted and the other remains under the other section (e.g. assert that at least one of the two actions has section === 'other', or split into separate tests for recently-used vs featured promotion).
| const labels = getActionLabels(items); | |
| assert.ok(labels.includes('gemma:7b'), 'Featured/recent Ollama model should appear'); | |
| assert.ok(labels.includes('gemma:7b (my-endpoint)'), 'BYOK model with colliding metadata.id should still appear in Other Models'); | |
| const actions = getActionItems(items); | |
| const promotedOllama = actions.find(a => a.label === 'gemma:7b' && a.section !== 'other'); | |
| const otherByok = actions.find(a => a.label === 'gemma:7b (my-endpoint)' && a.section === 'other'); | |
| assert.ok(promotedOllama, 'Featured/recent Ollama model should appear in a promoted section'); | |
| assert.ok(otherByok, 'BYOK model with colliding metadata.id should still appear in Other Models'); |
Fixes #312908
Problem
In
buildModelPickerItems, theplacedset was keyed on both a model's fullidentifier(e.g.customoai/<group>/<id>) and its baremetadata.id(e.g.claude-opus-4.7). Once any one model was placed in the Auto / Promoted (selected / recently-used / featured) section, every other model from a different vendor that happened to share the samemetadata.idwas filtered out of the "Other Models" section — and was therefore unreachable from the chat model picker entirely (search included), even though it appeared correctly in the Language Models settings view.Fix
Track promoted models strictly by
identifierin theplacedset. The lookup paths that match by baremetadata.id(resolving featured / recent IDs from the control manifest to a concrete model viamodelsByMetadataId) still resolve via that map, but only record the resulting identifier inplaced.Control-manifest entries that have no concrete matching model (
!entry.existsplaceholders) keep using their entry id — those IDs live in a separate id-namespace and don't collide with vendors'metadata.ids.The "Other Models" filter is now
models.filter(m => !placed.has(m.identifier)).Tests
Two new tests in
chatModelPicker.test.ts:models from different vendors sharing a metadata.id all appear (issue #312908)— covers the selected-model promotion path.promoted model does not hide other models with same metadata.id from Other Models— covers the recently-used and featured promotion paths.Both fail before the fix and pass after.
All 42
buildModelPickerItemstests pass; ran the broader chat picker / language models suite with no regressions.