capture resolved provider from the loadbalancer for logging - #3930
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds support for routing-resolved provider selection in the provider utility layer. A new context key stores the resolved provider from routing, and the provider selection function now checks for and prioritizes this resolved provider when it is present in the available providers list. ChangesRouting-Resolved Provider Selection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
Confidence Score: 5/5Change is safe to merge — the new early-return is correctly guarded by the existing availableProviders check and cannot bypass the allowed-provider constraint. The logic change is small and well-scoped: it only activates when BifrostContextKeyAvailableProviders is already set (non-nil, non-empty) and the resolved provider is a member of that list. All existing code paths are unchanged when the new key is absent. The two new tests cover both the resolution path and the bypass-prevention path. No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "capture resolved provider from the loadb..." | Re-trigger Greptile |
386c28e to
ed4afdc
Compare
98a3454 to
3f3b757
Compare
3f3b757 to
fe395bf
Compare
691f90c to
ed39e2d
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/utils/utils.go (1)
2946-2957:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck the resolved provider before the skip-key-selection early return.
Line 2946 still returns
defaultProviderbefore the newBifrostContextKeyResolvedProviderbranch runs. That leaves a path where a routing-selected provider is ignored and the code falls back to the route default again.Suggested fix
func CheckAndSetDefaultProvider(ctx *schemas.BifrostContext, defaultProvider schemas.ModelProvider) schemas.ModelProvider { if ctx != nil { + if ctx.Value(schemas.BifrostContextKeyAvailableProviders) != nil { + availableProviders, ok := ctx.Value(schemas.BifrostContextKeyAvailableProviders).([]schemas.ModelProvider) + if !ok || len(availableProviders) == 0 { + return "" + } + if resolvedProvider, ok := ctx.Value(schemas.BifrostContextKeyResolvedProvider).(schemas.ModelProvider); ok && slices.Contains(availableProviders, resolvedProvider) { + getLogger().Debug("[Provider] Using routing-resolved provider: %s (available: %v)", resolvedProvider, availableProviders) + return resolvedProvider + } + } if skip, ok := ctx.Value(schemas.BifrostContextKeySkipKeySelection).(bool); ok && skip { return defaultProvider } if ctx.Value(schemas.BifrostContextKeyAvailableProviders) != nil { availableProviders, ok := ctx.Value(schemas.BifrostContextKeyAvailableProviders).([]schemas.ModelProvider) if !ok || len(availableProviders) == 0 { return "" } - if resolvedProvider, ok := ctx.Value(schemas.BifrostContextKeyResolvedProvider).(schemas.ModelProvider); ok && slices.Contains(availableProviders, resolvedProvider) { - getLogger().Debug("[Provider] Using routing-resolved provider: %s (available: %v)", resolvedProvider, availableProviders) - return resolvedProvider - } getLogger().Debug("[Provider] Available providers: %v, checking %s", availableProviders, defaultProvider) if slices.Contains(availableProviders, defaultProvider) { return defaultProvider } return availableProviders[0] }🤖 Prompt for 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. In `@core/providers/utils/utils.go` around lines 2946 - 2957, The early return that checks BifrostContextKeySkipKeySelection returns defaultProvider before honoring a routing-resolved provider; modify the logic in the function that reads ctx.Value so that you check for BifrostContextKeyResolvedProvider (and verify it exists in the availableProviders using slices.Contains) before performing the skip-key-selection early return (or, alternatively, if skip is true, still inspect ctx.Value(schemas.BifrostContextKeyResolvedProvider) and return it when present). Ensure you reference and preserve defaultProvider, availableProviders (from ctx.Value(schemas.BifrostContextKeyAvailableProviders)), the resolvedProvider check (ctx.Value(schemas.BifrostContextKeyResolvedProvider)), and the getLogger().Debug call so routing-resolved providers are used when available.
🤖 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.
Outside diff comments:
In `@core/providers/utils/utils.go`:
- Around line 2946-2957: The early return that checks
BifrostContextKeySkipKeySelection returns defaultProvider before honoring a
routing-resolved provider; modify the logic in the function that reads ctx.Value
so that you check for BifrostContextKeyResolvedProvider (and verify it exists in
the availableProviders using slices.Contains) before performing the
skip-key-selection early return (or, alternatively, if skip is true, still
inspect ctx.Value(schemas.BifrostContextKeyResolvedProvider) and return it when
present). Ensure you reference and preserve defaultProvider, availableProviders
(from ctx.Value(schemas.BifrostContextKeyAvailableProviders)), the
resolvedProvider check (ctx.Value(schemas.BifrostContextKeyResolvedProvider)),
and the getLogger().Debug call so routing-resolved providers are used when
available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 247df19f-dc68-4084-83dc-7f9a32de7f98
📒 Files selected for processing (3)
core/providers/utils/utils.gocore/providers/utils/utils_test.gocore/schemas/bifrost.go
ed39e2d to
a217ac4
Compare
fe395bf to
1e39cba
Compare
Merge activity
|
## Summary When a routing layer selects a specific provider, `CheckAndSetDefaultProvider` was ignoring that selection and falling back to the route's default provider. This PR ensures that a routing-resolved provider takes precedence over the default, as long as it is still within the allowed set of available providers. ## Changes - Added a new context key `BifrostContextKeyResolvedProvider` to carry the provider selected by the routing layer. - Updated `CheckAndSetDefaultProvider` to check for a resolved provider in context and return it immediately if it is present in the available providers list, before falling back to the default provider check. - Added two tests: one verifying the resolved provider is used when allowed, and one verifying it is ignored when not in the available providers list. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/utils/... -run TestCheckAndSetDefaultProvider ``` Expected: both `TestCheckAndSetDefaultProviderUsesResolvedProvider` and `TestCheckAndSetDefaultProviderIgnoresDisallowedResolvedProvider` pass. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations The resolved provider context key is explicitly marked `DO NOT SET THIS MANUALLY` and is only populated by the routing layer. It cannot be used to bypass available-provider constraints, as the check enforces membership in the allowed list before honoring the resolved provider. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Improvements** * Enhanced provider selection logic to better utilize routing-determined providers when available. * Improved fallback behavior for provider resolution in routing scenarios. * **Tests** * Added test coverage for provider selection scenarios with and without routing-resolved providers. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary When a routing layer selects a specific provider, `CheckAndSetDefaultProvider` was ignoring that selection and falling back to the route's default provider. This PR ensures that a routing-resolved provider takes precedence over the default, as long as it is still within the allowed set of available providers. ## Changes - Added a new context key `BifrostContextKeyResolvedProvider` to carry the provider selected by the routing layer. - Updated `CheckAndSetDefaultProvider` to check for a resolved provider in context and return it immediately if it is present in the available providers list, before falling back to the default provider check. - Added two tests: one verifying the resolved provider is used when allowed, and one verifying it is ignored when not in the available providers list. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/utils/... -run TestCheckAndSetDefaultProvider ``` Expected: both `TestCheckAndSetDefaultProviderUsesResolvedProvider` and `TestCheckAndSetDefaultProviderIgnoresDisallowedResolvedProvider` pass. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations The resolved provider context key is explicitly marked `DO NOT SET THIS MANUALLY` and is only populated by the routing layer. It cannot be used to bypass available-provider constraints, as the check enforces membership in the allowed list before honoring the resolved provider. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Improvements** * Enhanced provider selection logic to better utilize routing-determined providers when available. * Improved fallback behavior for provider resolution in routing scenarios. * **Tests** * Added test coverage for provider selection scenarios with and without routing-resolved providers. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary When a routing layer selects a specific provider, `CheckAndSetDefaultProvider` was ignoring that selection and falling back to the route's default provider. This PR ensures that a routing-resolved provider takes precedence over the default, as long as it is still within the allowed set of available providers. ## Changes - Added a new context key `BifrostContextKeyResolvedProvider` to carry the provider selected by the routing layer. - Updated `CheckAndSetDefaultProvider` to check for a resolved provider in context and return it immediately if it is present in the available providers list, before falling back to the default provider check. - Added two tests: one verifying the resolved provider is used when allowed, and one verifying it is ignored when not in the available providers list. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/utils/... -run TestCheckAndSetDefaultProvider ``` Expected: both `TestCheckAndSetDefaultProviderUsesResolvedProvider` and `TestCheckAndSetDefaultProviderIgnoresDisallowedResolvedProvider` pass. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations The resolved provider context key is explicitly marked `DO NOT SET THIS MANUALLY` and is only populated by the routing layer. It cannot be used to bypass available-provider constraints, as the check enforces membership in the allowed list before honoring the resolved provider. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Improvements** * Enhanced provider selection logic to better utilize routing-determined providers when available. * Improved fallback behavior for provider resolution in routing scenarios. * **Tests** * Added test coverage for provider selection scenarios with and without routing-resolved providers. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…3930) ## Summary When a routing layer selects a specific provider, `CheckAndSetDefaultProvider` was ignoring that selection and falling back to the route's default provider. This PR ensures that a routing-resolved provider takes precedence over the default, as long as it is still within the allowed set of available providers. ## Changes - Added a new context key `BifrostContextKeyResolvedProvider` to carry the provider selected by the routing layer. - Updated `CheckAndSetDefaultProvider` to check for a resolved provider in context and return it immediately if it is present in the available providers list, before falling back to the default provider check. - Added two tests: one verifying the resolved provider is used when allowed, and one verifying it is ignored when not in the available providers list. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/utils/... -run TestCheckAndSetDefaultProvider ``` Expected: both `TestCheckAndSetDefaultProviderUsesResolvedProvider` and `TestCheckAndSetDefaultProviderIgnoresDisallowedResolvedProvider` pass. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations The resolved provider context key is explicitly marked `DO NOT SET THIS MANUALLY` and is only populated by the routing layer. It cannot be used to bypass available-provider constraints, as the check enforces membership in the allowed list before honoring the resolved provider. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Improvements** * Enhanced provider selection logic to better utilize routing-determined providers when available. * Improved fallback behavior for provider resolution in routing scenarios. * **Tests** * Added test coverage for provider selection scenarios with and without routing-resolved providers. <!-- end of auto-generated comment: release notes by coderabbit.ai -->

Summary
When a routing layer selects a specific provider,
CheckAndSetDefaultProviderwas ignoring that selection and falling back to the route's default provider. This PR ensures that a routing-resolved provider takes precedence over the default, as long as it is still within the allowed set of available providers.Changes
BifrostContextKeyResolvedProviderto carry the provider selected by the routing layer.CheckAndSetDefaultProviderto check for a resolved provider in context and return it immediately if it is present in the available providers list, before falling back to the default provider check.Type of change
Affected areas
How to test
go test ./core/providers/utils/... -run TestCheckAndSetDefaultProviderExpected: both
TestCheckAndSetDefaultProviderUsesResolvedProviderandTestCheckAndSetDefaultProviderIgnoresDisallowedResolvedProviderpass.Breaking changes
Related issues
Security considerations
The resolved provider context key is explicitly marked
DO NOT SET THIS MANUALLYand is only populated by the routing layer. It cannot be used to bypass available-provider constraints, as the check enforces membership in the allowed list before honoring the resolved provider.Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
Release Notes
Improvements
Tests