Skip to content

refactor(router): inline 3 core resolver methods into ModelLookup#327

Merged
SantiagoDePolonia merged 1 commit into
mainfrom
refactor/router-lookup-interface
May 12, 2026
Merged

refactor(router): inline 3 core resolver methods into ModelLookup#327
SantiagoDePolonia merged 1 commit into
mainfrom
refactor/router-lookup-interface

Conversation

@SantiagoDePolonia
Copy link
Copy Markdown
Contributor

@SantiagoDePolonia SantiagoDePolonia commented May 12, 2026

Summary

Router.go ran three runtime type-assertions on r.lookup against the optional interfaces core.ProviderNameResolver, core.ProviderTypeNameResolver, and core.ProviderNameTypeResolver. *ModelRegistry already implements all three; the assertions existed because the test mockModelLookup deliberately omitted them to keep its surface small.

That made the optional-protocol pattern act as feature detection rather than real interface segregation: every production call site assumed the assertion would succeed. This PR widens core.ModelLookup with the three methods and drops the indirection — r.lookup.GetProviderName(...) instead of the if ok ceremony — and the mock gains three trivial stub methods returning empty string, matching how its missing data was already surfaced.

What does NOT change

The three core.*Resolver interface types stay defined. They are still used in 5 other places for provider-side type assertions on core.Provider (in internal/server/, internal/aliases/, internal/gateway/). Only the lookup-side assertions are removed.

One side-effect cleanup

Router.GetProviderTypeForName had a modelWithProviderLister iteration fallback for the case where the lookup didn't implement core.ProviderNameTypeResolver. With the direct method always available, that fallback became unreachable and is removed. Behavior is strictly more complete than the fallback: *ModelRegistry.GetProviderTypeForName consults the providerNames/providerTypes maps directly, which include registered providers with zero discovered models that the model-iteration fallback would have missed.

Risk

  • API surface widening: core.ModelLookup gains 3 methods. Inside this repo it's a non-event — only impl is *ModelRegistry (already has them) and the test mock (now has stubs). Anyone outside the repo who implemented core.ModelLookup for their own purposes would need to add the 3 methods.
  • Mock stubs return "": mockModelLookup returns empty string from all three new methods, matching the prior degraded behavior (the type-assertion path used to fail for the mock and fall through to the same "" outcome).
  • No semantic loss: All 25 tests using the mock pass under -race, plus the full ./... suite (56 packages).

Test plan

  • make test-race, make lint, go mod tidy — all green via pre-commit hooks.
  • Full go test ./... clean across 56 packages.
  • *ModelRegistry satisfies the widened interface (compile-time assertion at registry_test.go:1865 still holds).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Refactor
    • Improved internal provider lookup mechanisms to streamline routing efficiency.

Review Change Stack

Router.go ran three runtime type-assertions on `r.lookup` against the
optional interfaces `core.ProviderNameResolver`, `core.ProviderTypeNameResolver`,
and `core.ProviderNameTypeResolver`. *ModelRegistry already implements all
three; the assertions existed because the test `mockModelLookup` deliberately
omitted them to keep its surface small.

The optional-protocol pattern was being used as feature detection rather than
as real interface segregation: every production call site assumed the assertion
would succeed. Widening `core.ModelLookup` with the three methods removes the
indirection — `r.lookup.GetProviderName(...)` instead of
`if named, ok := r.lookup.(core.ProviderNameResolver); ok { named.GetProviderName(...) }`
— and the mock gains three trivial stub methods returning empty string,
matching how its missing data was already surfaced.

The three `core.*Resolver` interface types stay defined; they are still used
in 5 other places for provider-side type assertions on `core.Provider` (in
internal/server, internal/aliases, internal/gateway). Only the lookup-side
assertions are removed.

One side-effect cleanup in `Router.GetProviderTypeForName`: the
`modelWithProviderLister` iteration fallback became unreachable now that the
direct method is always available, and is removed. Behavior is strictly more
complete than the fallback — *ModelRegistry's direct method consults the
provider name/type maps, which include registered providers with zero
discovered models that the model-iteration fallback would have missed.

Test plan:
- All 25-ish tests using mockModelLookup compile and pass under -race.
- Full `go test ./...` clean across 56 packages.
- `*ModelRegistry` satisfies the widened interface (compile-time assertion at
  registry_test.go:1865 still holds).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c6542439-2f28-44b9-9b06-afa41bb45774

📥 Commits

Reviewing files that changed from the base of the PR and between 2f13f68 and 0988140.

📒 Files selected for processing (3)
  • internal/core/interfaces.go
  • internal/providers/router.go
  • internal/providers/router_test.go

📝 Walkthrough

Walkthrough

The PR centralizes provider lookup through the ModelLookup interface by adding three new methods: GetProviderName, GetProviderNameForType, and GetProviderTypeForName. The router is refactored to delegate directly to these methods instead of using optional resolver interfaces and fallback logic.

Changes

Provider Lookup Centralization

Layer / File(s) Summary
ModelLookup interface extension
internal/core/interfaces.go
ModelLookup gains three new provider/model mapping methods (GetProviderName, GetProviderNameForType, GetProviderTypeForName) with documented empty-string semantics for "no mapping".
Router delegation refactoring
internal/providers/router.go
resolveUnqualifiedSelector, GetProviderName, GetProviderNameForType, and GetProviderTypeForName now directly delegate to the corresponding ModelLookup methods, removing prior conditional logic based on optional resolver interface availability and Supports() checking.
Test mock implementation
internal/providers/router_test.go
mockModelLookup implements the three new provider-lookup methods returning empty strings, with a note that real provider-name routing tests should use the actual ModelRegistry.

Possibly Related PRs

  • ENTERPILOT/GoModel#160: Both PRs touch provider-name/type model lookup and evolve router.go to rely on the lookup/registry for provider-qualified model resolution.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 A rabbit hops through lookups bright,
Centralizing paths in sight,
No moreResolver's tangled maze—
ModelLookup lights the way!
Provider names now find their place, 🔍

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding three resolver methods to the ModelLookup interface and using them directly in router instead of runtime type assertions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/router-lookup-interface

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 12, 2026

Greptile Summary

This PR widens the core.ModelLookup interface with three methods (GetProviderName, GetProviderNameForType, GetProviderTypeForName) that were previously reached via optional runtime type-assertions in router.go, and removes those assertions in favour of direct method calls. The test mockModelLookup gains three stub methods that return "", matching the prior degraded behaviour when the assertions failed.

  • internal/core/interfaces.go: Three methods promoted from optional Provider*Resolver interfaces into the mandatory ModelLookup contract; the optional interfaces themselves are retained for provider-side assertions elsewhere.
  • internal/providers/router.go: Six type-assertion blocks collapsed into direct method calls; GetProviderTypeForName's modelWithProviderLister iteration fallback removed as it was unreachable in production (the real ModelRegistry always satisfied ProviderNameTypeResolver).
  • internal/providers/router_test.go: mockModelLookup gains three stub methods to satisfy the widened interface.

Confidence Score: 5/5

Safe to merge. The change eliminates optional-interface indirection that was already assumed to succeed in every production call site, replacing it with direct method calls on a widened contract that the sole real implementor already satisfies.

All three removed type-assertion blocks delegated to *ModelRegistry in production — the assertions never failed at runtime. The fallback iteration in GetProviderTypeForName was only reachable via the test mock, and *ModelRegistry.GetProviderTypeForName is strictly more complete (map lookup covers providers with zero discovered models). Mock stubs correctly reproduce the prior empty-string outcome. The change is internally consistent, all 56 packages pass under -race, and the compile-time interface assertion in registry_test.go confirms *ModelRegistry still satisfies the widened ModelLookup.

No files require special attention. External codebases that implement core.ModelLookup directly will need to add the three new methods, but no such case exists inside this repository.

Important Files Changed

Filename Overview
internal/core/interfaces.go Adds three required methods to ModelLookup; optional resolver interfaces are preserved for provider-side assertions. Breaking change for external implementors, intentional and documented.
internal/providers/router.go Six type-assertion blocks replaced with direct interface calls; GetProviderTypeForName fallback iteration removed. Logic is equivalent or strictly better than before.
internal/providers/router_test.go Three stub methods added to mockModelLookup, each returning "" to match the prior fall-through behaviour. No test logic changed.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Router method called] --> B{Before PR}
    B --> C[Type-assert r.lookup against optional interface]
    C --> D{Assertion OK?}
    D -- Yes --> E[Call method on lookup]
    D -- No --> F[Return empty string or iterate models fallback]
    A --> G{After PR}
    G --> H[Call r.lookup method directly]
    H --> I[ModelLookup contract guarantees method exists]
    I --> J[Return result or empty string]
    style F fill:#f9c,stroke:#c66
    style J fill:#cfc,stroke:#6a6
Loading

Reviews (1): Last reviewed commit: "refactor(router): inline 3 core resolver..." | Re-trigger Greptile

@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/providers/router.go 50.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@SantiagoDePolonia SantiagoDePolonia merged commit cc5ca70 into main May 12, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants