Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds ADR and code to deterministically parse provider-qualified model selectors, updates registry/router to expose provider-prefixed public models and to prefer provider-qualified resolution when the selector prefix matches a configured provider, and removes the repository PR template. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router
participant Registry as ModelRegistry
participant Provider as Provider
Client->>Router: request ListModels / request model "selector"
Router->>Registry: resolve selector / ListPublicModels?
alt selector contains "/"
Registry->>Registry: parse prefix and modelID
alt prefix matches configured provider
Registry->>Provider: lookup model by modelID (provider-scoped)
Provider-->>Registry: model found / not found
alt found
Registry-->>Router: provider-qualified model
else not found
Registry-->>Router: return not-found (no fallback)
end
else prefix not a configured provider
Registry->>Registry: treat selector as raw model ID (global lookup)
Registry->>Provider: route to matching provider by model ID
Provider-->>Registry: model found
Registry-->>Router: raw model result
end
else no "/"
Registry->>Registry: unqualified lookup (first-registered-provider-wins)
Registry->>Provider: lookup
Provider-->>Registry: model found
Registry-->>Router: model result
end
Router-->>Client: response (models list or model response)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/providers/registry_test.go`:
- Around line 621-677: Add a variant to
TestListPublicModels_UsesConfiguredProviderNamesAndIncludesDuplicates that
includes a model ID containing a slash (e.g., "google/gemini-xyz") in one
provider's modelsResponse.Data (suggest using the openRouter mock), then extend
the expected want slice to include the double-qualified ID
"openrouter/google/gemini-xyz" with OwnedBy "openrouter" and update assertions
accordingly; modify the registryMockProvider for openRouter (and the want
ordering) so ListPublicModels produces "openrouter/google/gemini-xyz" to verify
slash-containing IDs are prefixed with the configured provider name by the
ListPublicModels logic.
In `@internal/providers/router.go`:
- Around line 313-316: Avoid calling r.lookup.ListModels() unconditionally and
discarding it when r.lookup implements publicModelLister; instead perform the
type assertion first (check if r.lookup satisfies publicModelLister) and call
ListPublicModels() in that branch, otherwise call r.lookup.ListModels() to
populate models; update the code around the models variable and the type
assertion using the publicModelLister interface, referencing
r.lookup.ListModels(), publicModelLister, and ListPublicModels() so the
unnecessary initial call is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48bb34ef-0be7-46c0-b8be-38a2eacf897c
📒 Files selected for processing (6)
.github/pull_request_template.mddocs/adr/0005-provider-qualified-model-selectors.mdinternal/providers/registry.gointernal/providers/registry_test.gointernal/providers/router.gointernal/providers/router_test.go
💤 Files with no reviewable changes (1)
- .github/pull_request_template.md
Comment on lines
+621
to
+677
| func TestListPublicModels_UsesConfiguredProviderNamesAndIncludesDuplicates(t *testing.T) { | ||
| registry := NewModelRegistry() | ||
|
|
||
| openAI := ®istryMockProvider{ | ||
| name: "provider-openai", | ||
| modelsResponse: &core.ModelsResponse{ | ||
| Object: "list", | ||
| Data: []core.Model{ | ||
| {ID: "gpt-4o", Object: "model", OwnedBy: "openai"}, | ||
| }, | ||
| }, | ||
| } | ||
| openRouter := ®istryMockProvider{ | ||
| name: "provider-openrouter", | ||
| modelsResponse: &core.ModelsResponse{ | ||
| Object: "list", | ||
| Data: []core.Model{ | ||
| {ID: "gpt-4o", Object: "model", OwnedBy: "openai"}, | ||
| }, | ||
| }, | ||
| } | ||
| azure := ®istryMockProvider{ | ||
| name: "provider-azure", | ||
| modelsResponse: &core.ModelsResponse{ | ||
| Object: "list", | ||
| Data: []core.Model{ | ||
| {ID: "gpt-4o", Object: "model", OwnedBy: "openai"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| registry.RegisterProviderWithNameAndType(openAI, "openai", "openai") | ||
| registry.RegisterProviderWithNameAndType(openRouter, "openrouter", "openai") | ||
| registry.RegisterProviderWithNameAndType(azure, "azure-openai", "openai") | ||
| if err := registry.Initialize(context.Background()); err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| models := registry.ListPublicModels() | ||
| if len(models) != 3 { | ||
| t.Fatalf("expected 3 public models, got %d", len(models)) | ||
| } | ||
|
|
||
| want := []core.Model{ | ||
| {ID: "azure-openai/gpt-4o", OwnedBy: "azure-openai"}, | ||
| {ID: "openai/gpt-4o", OwnedBy: "openai"}, | ||
| {ID: "openrouter/gpt-4o", OwnedBy: "openrouter"}, | ||
| } | ||
| for i, model := range want { | ||
| if models[i].ID != model.ID { | ||
| t.Fatalf("models[%d].ID = %q, want %q", i, models[i].ID, model.ID) | ||
| } | ||
| if models[i].OwnedBy != model.OwnedBy { | ||
| t.Fatalf("models[%d].OwnedBy = %q, want %q", i, models[i].OwnedBy, model.OwnedBy) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good coverage for ListPublicModels with configured provider names.
The test verifies that:
- Public model IDs use the configured provider name (not provider type)
- Duplicate models from multiple providers are all included
OwnedByis set to the configured provider name
Consider adding a test case with slash-containing model IDs (e.g., "google/gemini-xyz" from openrouter) to verify the double-qualification produces "openrouter/google/gemini-xyz".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/registry_test.go` around lines 621 - 677, Add a variant to
TestListPublicModels_UsesConfiguredProviderNamesAndIncludesDuplicates that
includes a model ID containing a slash (e.g., "google/gemini-xyz") in one
provider's modelsResponse.Data (suggest using the openRouter mock), then extend
the expected want slice to include the double-qualified ID
"openrouter/google/gemini-xyz" with OwnedBy "openrouter" and update assertions
accordingly; modify the registryMockProvider for openRouter (and the want
ordering) so ListPublicModels produces "openrouter/google/gemini-xyz" to verify
slash-containing IDs are prefixed with the configured provider name by the
ListPublicModels logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/providers/router_test.go`:
- Around line 61-64: The test suite lacks coverage for the fallback path in
Router.ListModels when the lookup does not implement publicModelLister; add a
new minimal mock type (e.g., mockBasicLookup) that implements core.ModelLookup
but NOT ListPublicModels, instantiate it with a single model (e.g., "gpt-4o"),
create the router via NewRouter(mockBasicLookup), call router.ListModels(ctx)
and assert that the response uses lookup.ListModels() (check resp.Data length
and resp.Data[0].ID equals "gpt-4o"); name the test
TestRouterListModels_FallbackToListModels and keep assertions for no error and
expected model ID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9ef9d3d-788e-4b4b-a298-11db031f87e6
📒 Files selected for processing (3)
docs/adr/0005-provider-qualified-model-selectors.mdinternal/providers/router.gointernal/providers/router_test.go
Comment on lines
+61
to
+64
| func (m *mockModelLookup) ListPublicModels() []core.Model { | ||
| m.publicCalls++ | ||
| return append([]core.Model(nil), m.publicModels...) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for the fallback path when publicModelLister is not implemented.
Since mockModelLookup always implements ListPublicModels(), the fallback path in Router.ListModels() (calling r.lookup.ListModels()) is not covered. Consider adding a separate mock type without ListPublicModels() to test that the router falls back correctly.
💡 Example: Add a minimal mock for fallback testing
// mockBasicLookup implements only core.ModelLookup without publicModelLister
type mockBasicLookup struct {
models map[string]core.Provider
modelList []core.Model
}
func (m *mockBasicLookup) Supports(model string) bool { _, ok := m.models[model]; return ok }
func (m *mockBasicLookup) GetProvider(model string) core.Provider { return m.models[model] }
func (m *mockBasicLookup) GetProviderType(_ string) string { return "" }
func (m *mockBasicLookup) ListModels() []core.Model { return m.modelList }
func (m *mockBasicLookup) ModelCount() int { return len(m.models) }
func TestRouterListModels_FallbackToListModels(t *testing.T) {
lookup := &mockBasicLookup{
models: map[string]core.Provider{"gpt-4o": &mockProvider{}},
modelList: []core.Model{{ID: "gpt-4o", Object: "model"}},
}
router, _ := NewRouter(lookup)
resp, err := router.ListModels(context.Background())
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(resp.Data) != 1 || resp.Data[0].ID != "gpt-4o" {
t.Errorf("expected fallback to ListModels(), got %+v", resp.Data)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/router_test.go` around lines 61 - 64, The test suite lacks
coverage for the fallback path in Router.ListModels when the lookup does not
implement publicModelLister; add a new minimal mock type (e.g., mockBasicLookup)
that implements core.ModelLookup but NOT ListPublicModels, instantiate it with a
single model (e.g., "gpt-4o"), create the router via NewRouter(mockBasicLookup),
call router.ListModels(ctx) and assert that the response uses
lookup.ListModels() (check resp.Data length and resp.Data[0].ID equals
"gpt-4o"); name the test TestRouterListModels_FallbackToListModels and keep
assertions for no error and expected model ID.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI model names are now prefixed with {provider} when you enlist them by calling
/modelsendpoint.Summary by CodeRabbit
New Features
Documentation