refactor: simplify load balancing skip logic to use parsed provider field directly - #4279
Merged
Pratham-Mishra04 merged 1 commit intoJun 11, 2026
Conversation
This was referenced Jun 11, 2026
|
|
This was referenced Jun 11, 2026
Collaborator
Author
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSimplifies governance plugin provider-selection by exiting early when request ChangesGovernance & Dependency Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Confidence Score: 4/5The governance logic change is not safe to merge as-is: the large-payload routing path builds a synthetic request without a Provider field, so the new guard silently lets provider-prefixed model strings pass through load balancing instead of being left unchanged. The simplified guard is correct for the normal HTTP path but breaks the large-payload plugins/governance/main.go — specifically the interaction between Important Files Changed
Reviews (2): Last reviewed commit: "fix: governance provider load balancing ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+461
to
469
| provider, modelStr, existingFallbacks := req.GetRequestFields() | ||
| if modelStr == "" { | ||
| return nil | ||
| } | ||
|
|
||
| // Model already has provider prefix pointing to a configured provider → leave it alone. | ||
| if strings.Contains(modelStr, "/") { | ||
| provider, _ := schemas.ParseModelString(modelStr, "") | ||
| // Checking valid provider when store is available; if store is nil, | ||
| // assume the prefixed model should be left unchanged. | ||
| if p.inMemoryStore != nil { | ||
| if _, ok := p.inMemoryStore.GetConfiguredProviders()[provider]; ok { | ||
| ctx.AppendRoutingEngineLog(schemas.RoutingEngineGovernance, schemas.LogLevelInfo, fmt.Sprintf("Skipping load balancing for model %s: already prefixed with configured provider %s", modelStr, provider)) | ||
| return nil | ||
| } | ||
| } else { | ||
| ctx.AppendRoutingEngineLog(schemas.RoutingEngineGovernance, schemas.LogLevelWarn, fmt.Sprintf("Skipping load balancing for model %s: provider-prefixed and no in-memory store to validate against", modelStr)) | ||
| return nil | ||
| } | ||
| if provider != "" { | ||
| ctx.AppendRoutingEngineLog(schemas.RoutingEngineGovernance, schemas.LogLevelInfo, fmt.Sprintf("Skipping load balancing for model %s: provider %s already set", modelStr, provider)) | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
loadBalanceProvider skip behavior
There are no existing tests that exercise the "skip load balancing when provider is already set" path, nor the large-payload synthetic-request path through runPreRequestRouting. The PR description suggests testing go test ./plugins/governance/..., but the test files don't include a case that sends a request with a non-empty provider (or a provider-prefixed model string) to loadBalanceProvider and verifies the function returns immediately without mutating the request. Given the behavioral change introduced here, a targeted table-driven test covering both the provider != "" early exit and the synthetic-request / raw-model-string case would help prevent future regressions.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Collaborator
Author
Merge activity
|
Pratham-Mishra04
changed the base branch from
06-10-docs_semantic_cache_doc_updates
to
graphite-base/4279
June 11, 2026 07:36
Pratham-Mishra04
force-pushed
the
06-11-fix_governance_provider_load_balancing_fixes
branch
from
June 11, 2026 07:38
046594b to
14d6864
Compare
Pratham-Mishra04
deleted the
06-11-fix_governance_provider_load_balancing_fixes
branch
June 11, 2026 07:39
18 tasks
akshaydeo
pushed a commit
that referenced
this pull request
Jun 12, 2026
…ield directly (#4279) ## Summary Simplifies the load balancing skip logic in the governance plugin by leveraging the provider value already parsed from `GetRequestFields()`, and bumps several indirect dependencies in the model catalog resolver module. ## Changes - The `loadBalanceProvider` function previously discarded the provider returned by `GetRequestFields()` and re-parsed it manually from the model string when a `/` was detected, with additional branching to check the in-memory store for configured providers. This logic is replaced by a straightforward check: if `provider` is already non-empty (as returned directly by `GetRequestFields()`), skip load balancing immediately. This removes the dependency on `inMemoryStore` for this check and eliminates the `strings.Contains` slash-detection path. - Bumped `cloud.google.com/go/iam` from `v1.5.3` to `v1.7.0`. - Bumped `github.com/aws/aws-sdk-go-v2` from `v1.41.7` to `v1.41.12`. - Bumped `github.com/aws/aws-sdk-go-v2/internal/configsources` from `v1.4.23` to `v1.4.28`. - Bumped `github.com/aws/aws-sdk-go-v2/internal/endpoints/v2` from `v2.7.23` to `v2.7.28`. - Bumped `github.com/aws/smithy-go` from `v1.25.1` to `v1.27.1`. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./plugins/governance/... ``` Verify that requests with a provider already set on the request are not load balanced, and that requests without a provider still proceed through load balancing as expected. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] 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 * **Improvements** * Optimized load balancing logic for more efficient provider selection * **Chores** * Updated dependencies including Google Cloud IAM and AWS SDK modules to latest stable versions <!-- end of auto-generated comment: release notes by coderabbit.ai -->
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
…ield directly (maximhq#4279) ## Summary Simplifies the load balancing skip logic in the governance plugin by leveraging the provider value already parsed from `GetRequestFields()`, and bumps several indirect dependencies in the model catalog resolver module. ## Changes - The `loadBalanceProvider` function previously discarded the provider returned by `GetRequestFields()` and re-parsed it manually from the model string when a `/` was detected, with additional branching to check the in-memory store for configured providers. This logic is replaced by a straightforward check: if `provider` is already non-empty (as returned directly by `GetRequestFields()`), skip load balancing immediately. This removes the dependency on `inMemoryStore` for this check and eliminates the `strings.Contains` slash-detection path. - Bumped `cloud.google.com/go/iam` from `v1.5.3` to `v1.7.0`. - Bumped `github.com/aws/aws-sdk-go-v2` from `v1.41.7` to `v1.41.12`. - Bumped `github.com/aws/aws-sdk-go-v2/internal/configsources` from `v1.4.23` to `v1.4.28`. - Bumped `github.com/aws/aws-sdk-go-v2/internal/endpoints/v2` from `v2.7.23` to `v2.7.28`. - Bumped `github.com/aws/smithy-go` from `v1.25.1` to `v1.27.1`. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./plugins/governance/... ``` Verify that requests with a provider already set on the request are not load balanced, and that requests without a provider still proceed through load balancing as expected. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] 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 * **Improvements** * Optimized load balancing logic for more efficient provider selection * **Chores** * Updated dependencies including Google Cloud IAM and AWS SDK modules to latest stable versions <!-- end of auto-generated comment: release notes by coderabbit.ai -->
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
…ield directly (maximhq#4279) ## Summary Simplifies the load balancing skip logic in the governance plugin by leveraging the provider value already parsed from `GetRequestFields()`, and bumps several indirect dependencies in the model catalog resolver module. ## Changes - The `loadBalanceProvider` function previously discarded the provider returned by `GetRequestFields()` and re-parsed it manually from the model string when a `/` was detected, with additional branching to check the in-memory store for configured providers. This logic is replaced by a straightforward check: if `provider` is already non-empty (as returned directly by `GetRequestFields()`), skip load balancing immediately. This removes the dependency on `inMemoryStore` for this check and eliminates the `strings.Contains` slash-detection path. - Bumped `cloud.google.com/go/iam` from `v1.5.3` to `v1.7.0`. - Bumped `github.com/aws/aws-sdk-go-v2` from `v1.41.7` to `v1.41.12`. - Bumped `github.com/aws/aws-sdk-go-v2/internal/configsources` from `v1.4.23` to `v1.4.28`. - Bumped `github.com/aws/aws-sdk-go-v2/internal/endpoints/v2` from `v2.7.23` to `v2.7.28`. - Bumped `github.com/aws/smithy-go` from `v1.25.1` to `v1.27.1`. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./plugins/governance/... ``` Verify that requests with a provider already set on the request are not load balanced, and that requests without a provider still proceed through load balancing as expected. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] 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 * **Improvements** * Optimized load balancing logic for more efficient provider selection * **Chores** * Updated dependencies including Google Cloud IAM and AWS SDK modules to latest stable versions <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
Simplifies the load balancing skip logic in the governance plugin by leveraging the provider value already parsed from
GetRequestFields(), and bumps several indirect dependencies in the model catalog resolver module.Changes
loadBalanceProviderfunction previously discarded the provider returned byGetRequestFields()and re-parsed it manually from the model string when a/was detected, with additional branching to check the in-memory store for configured providers. This logic is replaced by a straightforward check: ifprovideris already non-empty (as returned directly byGetRequestFields()), skip load balancing immediately. This removes the dependency oninMemoryStorefor this check and eliminates thestrings.Containsslash-detection path.cloud.google.com/go/iamfromv1.5.3tov1.7.0.github.com/aws/aws-sdk-go-v2fromv1.41.7tov1.41.12.github.com/aws/aws-sdk-go-v2/internal/configsourcesfromv1.4.23tov1.4.28.github.com/aws/aws-sdk-go-v2/internal/endpoints/v2fromv2.7.23tov2.7.28.github.com/aws/smithy-gofromv1.25.1tov1.27.1.Type of change
Affected areas
How to test
go test ./plugins/governance/...Verify that requests with a provider already set on the request are not load balanced, and that requests without a provider still proceed through load balancing as expected.
Breaking changes
Related issues
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
Improvements
Chores