fix(providers): refresh unavailable providers on request#357
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds request-time provider model refresh across gateway, router, and registry so resolution can refresh provider inventories and retry when selectors fail, registries are empty, or models are unsupported. ChangesRequest-Time Provider Model Refresh
Sequence Diagram(s)sequenceDiagram
participant Client as Client Request
participant Gateway as Gateway
participant Registry as ModelRegistry
participant Router as Router
participant Provider as Provider
Client->>Gateway: ResolveRequestModelWithAuthorizer(requested)
Gateway->>Registry: ResolveExecutionSelector(requested)
alt selector fails / empty / unsupported
Registry-->>Gateway: error or unsupported
Gateway->>Registry: RefreshProviderModels(ctx, providerSelector)
Registry->>Provider: CheckAvailability/ListModels
Provider-->>Registry: models or error
Registry->>Registry: applyFetchedInventory -> rebuild maps
Registry-->>Gateway: refreshed count or error
Gateway->>Registry: ResolveExecutionSelector(requested) (retry)
end
Registry-->>Gateway: resolved model
Gateway->>Router: routeResolvedModelCall(ctx, resolved)
Router->>Registry: ResolveExecutionSelector(resolved) or RefreshProviderModels if missing
Router->>Provider: ChatCompletion
Provider-->>Router: response
Router-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR adds request-time provider inventory refresh when model resolution cannot find a requested provider model. It changes:
Confidence Score: 4/5This is close, but the batch path should be fixed before merging.
internal/gateway/batch_selection.go Important Files Changed
Reviews (2): Last reviewed commit: "fix(gateway): surface refresh target res..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/gateway/request_model_resolution_test.go`:
- Around line 167-222: Add a new unit test (e.g.,
TestResolveRequestModelRefreshTargetErrorHandling) that configures the request
refresh provider to make ResolveRefreshTarget return an error (use the existing
newRequestRefreshProvider or set the provider flag that triggers a resolve
error), call ResolveRequestModelWithAuthorizer with that provider and a selector
that triggers the refresh-target flow, and assert that
ResolveRequestModelWithAuthorizer returns a non-nil error which matches the
underlying ResolveRefreshTarget error; reference
requestRefreshTargetResolver/ResolveRefreshTarget and
ResolveRequestModelWithAuthorizer in the test to locate where to inject the
failure and the assertion.
In `@internal/gateway/request_model_resolution.go`:
- Around line 165-168: The code currently calls ResolveRefreshTarget on
targetResolver (type modelRefreshTargetResolver) and discards any error, which
masks real configuration/alias faults; change the logic in the block where
ResolveRefreshTarget(requested) is invoked so that when err != nil you return or
propagate that error (or wrap it with context) instead of silently ignoring it,
otherwise continue only when err == nil and ok == true and set providerSelector
accordingly; update the containing function's error flow to propagate the error
from ResolveRefreshTarget.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee0e151c-0e67-4ff2-ba6c-c05f78f833d4
📒 Files selected for processing (6)
internal/aliases/service.gointernal/aliases/service_test.gointernal/gateway/request_model_resolution.gointernal/gateway/request_model_resolution_test.gointernal/providers/registry_provider_refresh.gointernal/providers/registry_test.go
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
2 participants
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.
Summary
Tests
Summary by CodeRabbit
New Features
Bug Fixes
Tests