diff --git a/registry.go b/registry.go index a3cfea4..347a57f 100644 --- a/registry.go +++ b/registry.go @@ -664,10 +664,19 @@ func (m *modelRegistry) listFor(grp *group) []map[string]any { } out := make([]map[string]any, 0, len(owners)) + // listed tracks every id actually APPENDED to out below — not merely + // present in the raw, authorization-blind owners map — so the alias + // dedupe loop further down only skips an alias whose real-model entry + // this group can actually see (review fix, second pass): keying the + // dedupe on owners itself made a group-denied collision vanish the id + // from the listing entirely (neither the real entry nor the alias + // appeared), even though resolve still serves it through the alias. + listed := make(map[string]bool, len(owners)) for id, provs := range owners { winner := provs[0] if grp.allowsModel(id) && grp.allowsProvider(winner) { out = append(out, modelObject(id, winner)) + listed[id] = true } if len(provs) < 2 { continue @@ -680,6 +689,7 @@ func (m *modelRegistry) listFor(grp *group) []map[string]any { // for the equivalent client request. if (grp.allowsModel(pid) || grp.allowsModel(id)) && grp.allowsProvider(p) { out = append(out, modelObject(pid, p)) + listed[pid] = true } } } @@ -699,15 +709,15 @@ func (m *modelRegistry) listFor(grp *group) []map[string]any { // An alias id that also happens to collide with a since-discovered // model's own bare id (impossible for an EXPLICIT model — // validateModelAliases already rejects that at construction) is - // SKIPPED here (review fix), not appended a second time: owners - // already holds a real-model entry for that id from the loop above, - // and adding another with the same id would list it twice — + // SKIPPED here (review fix) only when that real model's own entry was + // actually EMITTED above (listed[alias], not merely present in + // owners) — otherwise adding it a second time would list it twice, // duplicate, order-nondeterministic-looking entries in an // OpenAI-shaped model list. resolve's own precedence (its doc // comment) still has the alias win at REQUEST time regardless of // which entry the listing shows; only the listing itself is deduped. for alias, target := range m.aliases { - if _, collides := owners[alias]; collides { + if listed[alias] { continue } _, _, canonical, err := m.resolveAliasTarget(alias, target, grp) diff --git a/registry_test.go b/registry_test.go index cb84ed0..6f6ee59 100644 --- a/registry_test.go +++ b/registry_test.go @@ -1254,6 +1254,14 @@ func TestModelRegistry_ListFor_Alias_NotAuthorized_Omitted(t *testing.T) { // and a second, duplicate one from the alias loop. resolve's own // precedence (registry.go's resolve doc comment) must still have the // alias win at request time regardless of which entry the listing shows. +// +// A second scenario (review fix, second pass) shares this fixture: when +// grp cannot reach the COLLIDING provider at all (provider-denied), the +// real model's own entry is never emitted in the first place — the +// dedupe must key off entries actually LISTED, not the raw, +// authorization-blind owners map, or the id would vanish from the +// listing entirely (neither the real entry nor the alias appears) even +// though resolve still serves it through the alias. func TestModelRegistry_ListFor_Alias_DedupedAgainstDiscoveredCollision(t *testing.T) { t.Parallel() adapters := map[string]providerAdapter{ @@ -1299,4 +1307,27 @@ func TestModelRegistry_ListFor_Alias_DedupedAgainstDiscoveredCollision(t *testin if canonical != "openai/gpt-test" { t.Errorf("canonical = %q, want %q", canonical, "openai/gpt-test") } + + // Provider-denied variant: grp can reach only "openai" — "acme", the + // colliding discovered model's own provider, is denied, so its raw + // entry is never emitted at all. The alias must still be listed + // exactly once, owned by its own target's provider ("openai") — the + // earlier owners-keyed dedupe would have wrongly skipped it too here, + // vanishing "shared-id" from the listing entirely. + narrowGrp := &group{name: "openai-only", providers: []string{"openai"}} + gotNarrow := reg.listFor(narrowGrp) + var narrowCount int + var ownedBy string + for _, entry := range gotNarrow { + if entry["id"] == "shared-id" { + narrowCount++ + ownedBy, _ = entry["owned_by"].(string) + } + } + if narrowCount != 1 { + t.Fatalf("listFor (provider-denied collision) has %d entries for id %q, want exactly 1 (the alias)", narrowCount, "shared-id") + } + if ownedBy != "openai" { + t.Errorf("owned_by = %q, want %q (the alias's own target provider)", ownedBy, "openai") + } }