fix(registry): scope static model lookup by provider to prevent cross-provider ID collisions#487
Conversation
…-provider ID collisions LookupModelInfo falls back to LookupStaticModelInfo which iterates all static model catalogs and returns the first ID match regardless of provider. When multiple providers define models with the same ID but different capabilities (e.g. github-copilot "claude-opus-4.6" with levels [low,medium,high] vs the actual Claude model supporting [low, medium,high,max]), the wrong provider entry can shadow the request. Use GetStaticModelDefinitionsByChannel to search only the requesting provider catalog first. If the provider has a dedicated catalog but the model is not in it, return nil so the caller can treat it as a user-defined model. Fall back to global search only when no provider hint is given or the provider has no dedicated catalog.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the LookupModelInfo function to include a provider-specific static lookup, which helps prevent model ID collisions across different providers by searching only the relevant catalog. The review feedback points out that the lookup may fail for provider aliases and suggests mapping them to base channel names to ensure the fix is effective for all variants. Additionally, a redundant cloning operation was identified that should be removed to improve performance.
| // "claude-opus-4.6" shadowing a claude-api-key request) cannot produce a | ||
| // wrong-provider match. | ||
| if p != "" { | ||
| if models := GetStaticModelDefinitionsByChannel(p); models != nil { |
There was a problem hiding this comment.
The provider string p is used directly to look up the static catalog via GetStaticModelDefinitionsByChannel(p). However, GetStaticModelDefinitionsByChannel only recognizes a specific set of "channel" names (e.g., "claude", "gemini", "github-copilot").
If the provider string passed to this function is an alias or a specific instance name like "claude-api-key" (as mentioned in the PR description) or "openai-api-key", GetStaticModelDefinitionsByChannel will return nil. This causes the logic to fall back to the global search at line 195, where cross-provider ID collisions can still occur. You should ensure that the provider string is mapped to its corresponding base channel name before performing the static lookup to ensure the fix is effective for all provider variants.
There was a problem hiding this comment.
The provider string p comes from the executor's Identifier() method (e.g. ClaudeExecutor.Identifier() returns "claude", GeminiExecutor.Identifier() returns "gemini", etc.), not from config YAML key names like "claude-api-key". These identifiers are the canonical channel keys that GetStaticModelDefinitionsByChannel expects.
The call chain: thinking.ApplyThinking(body, model, e.Identifier(), ...) → registry.LookupModelInfo(modelID, providerKey). No executor returns "claude-api-key" or "openai-api-key" from Identifier().
Not an issue.
internal/registry/model_registry.go
Outdated
| if models := GetStaticModelDefinitionsByChannel(p); models != nil { | ||
| for _, m := range models { | ||
| if m != nil && m.ID == modelID { | ||
| return cloneModelInfo(m) |
There was a problem hiding this comment.
Redundant cloning of ModelInfo. The models slice returned by GetStaticModelDefinitionsByChannel (and its underlying calls like GetClaudeModels) already contains deep-cloned ModelInfo objects. Calling cloneModelInfo(m) again here is unnecessary and adds overhead.
| return cloneModelInfo(m) | |
| return m |
There was a problem hiding this comment.
Good catch — GetStaticModelDefinitionsByChannel returns pre-cloned objects via cloneModelInfos(). Fixed in 0893d95.
GetStaticModelDefinitionsByChannel already returns cloned ModelInfo objects (via cloneModelInfos), so the extra cloneModelInfo call was a no-op double-clone.
|
This project only accepts pull requests that relate to third-party provider support. Any pull requests unrelated to third-party provider support will be rejected. If you need to submit any non-third-party provider changes, please open them against the mainline repository. |
2 participants
Summary
LookupModelInfostatic fallback (LookupStaticModelInfo) iterates all provider catalogs and returns the first ID match regardless of provider. When multiple providers define models with the same ID but different capabilities, the wrong entry shadows the request.claude-opus-4.6exists in both GitHub Copilot (levels:low,medium,high) and is a valid upstream Claude model (supportsmax). A claude-api-key request forclaude-opus-4.6witheffort: "max"was rejected because the Copilot entry was found first.Changes
Use
GetStaticModelDefinitionsByChannel(provider)to search only the requesting provider's dedicated static catalog before falling back to the global search. If the provider has a catalog but the model isn't in it, returnnilso callers treat it as a user-defined model (passthrough). Global fallback is preserved for callers without a provider hint or providers without a dedicated catalog.Testing
go test ./internal/registry/...— passgo test ./internal/thinking/...— passgo build ./internal/registry/...— clean