feat: filter ListAllModels provider fan-out by virtual key's allowed providers to suppress noisy governance errors - #3796
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements context-driven provider filtering for the model listing endpoint. Virtual-key information from HTTP requests populates a context key that the core ChangesVK-Based Provider Filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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)
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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
8c3e42e to
b95e8e7
Compare
ListAllModels provider fan-out by virtual key's allowed providers to suppress noisy governance errors
Confidence Score: 5/5Safe to merge; the filtering logic is correct, all edge cases are tested, and the change is narrowly scoped to the ListAllModels fan-out path. The core filtering logic in filterProvidersByContext is sound — nil context, absent key, malformed type-assertion, and empty allow-list are all handled correctly. The IsActiveValue() usage in list_models_vk.go correctly treats a nil IsActive pointer as active, closing the gap flagged in the prior review thread. The only open item is a documentation discrepancy: the schema comment for BifrostContextKeyAvailableProviders was not updated despite being mentioned in the PR description, but this does not affect runtime behavior. core/schemas/bifrost.go — the BifrostContextKeyAvailableProviders comment was not updated despite being mentioned in the PR description. Important Files Changed
Reviews (4): Last reviewed commit: "fix: isActive pointer references" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@transports/bifrost-http/handlers/list_models_vk.go`:
- Line 39: The SendError call currently includes the raw error
(fmt.Sprintf("Failed to resolve virtual key: %v", err)) which may leak DB
internals; change it to send a generic client-facing message like "Failed to
resolve virtual key" via SendError(ctx, fasthttp.StatusInternalServerError,
"Failed to resolve virtual key") and log the full error separately using the
request/context logger (e.g., processLogger.Errorf or ctx-logger) so the
detailed error is recorded for operators while the client only receives
non-sensitive text; update the call in list_models_vk.go where SendError is
invoked and add a separate log statement that includes err.
🪄 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: CHILL
Plan: Pro
Run ID: ee9557c3-fb14-4f1a-b578-9eec7355bdb1
📒 Files selected for processing (6)
core/bifrost.gocore/bifrost_test.gocore/schemas/bifrost.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/list_models_vk.gotransports/bifrost-http/handlers/list_models_vk_test.go
a841dd4 to
185115f
Compare
185115f to
17872e6
Compare
17872e6 to
aef283d
Compare
Merge activity
|
…d providers to suppress noisy governance errors (#3796) ## Summary When `GET /v1/models` is called with a virtual key, `ListAllModels` previously fanned out to every configured provider. Governance would then reject providers outside the virtual key's allowed set, producing noisy, expected errors in request logs. This PR scopes the provider fan-out to only the providers permitted by the virtual key. ## Changes - Added `filterProvidersByContext` in `core/bifrost.go` that reads `BifrostContextKeyAvailableProviders` from the request context and filters the provider list before fan-out. If the context value is malformed, it fails closed and returns an empty list. - Added `applyListModelsVirtualKeyProviderFilter` in a new `list_models_vk.go` handler file. When a virtual key is present on a `GET /v1/models` request with no explicit provider, it resolves the virtual key, extracts its allowed providers, and sets `BifrostContextKeyAvailableProviders` on the Bifrost context before the fan-out occurs. Inactive or missing virtual keys are silently skipped; lookup failures return an error response. - The `listModels` handler in `inference.go` now calls `applyListModelsVirtualKeyProviderFilter` before dispatching to `ListAllModels` when no specific provider is requested. - Updated the `BifrostContextKeyAvailableProviders` comment to clarify it is set by internal bifrost components rather than bifrost alone. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/... ./transports/bifrost-http/handlers/... ``` - Make a `GET /v1/models` request with a virtual key scoped to a subset of providers and confirm only those providers' models are returned with no governance rejection errors in the logs. - Make a `GET /v1/models` request without a virtual key and confirm all configured providers are still queried. - Make a `GET /v1/models` request with an inactive or unknown virtual key and confirm all providers are still queried without errors. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations The virtual key lookup fails closed on malformed context values and returns a 503 if the config store is unavailable, preventing unintended provider exposure. An unrecognized or inactive virtual key falls through without restricting providers, preserving existing behavior. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] 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 * **New Features** * Model listing now supports provider filtering through virtual key configurations, enabling restricted access to specific providers based on authentication context. * **Tests** * Added comprehensive test coverage for virtual key-based provider filtering and context handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->

Summary
When
GET /v1/modelsis called with a virtual key,ListAllModelspreviously fanned out to every configured provider. Governance would then reject providers outside the virtual key's allowed set, producing noisy, expected errors in request logs. This PR scopes the provider fan-out to only the providers permitted by the virtual key.Changes
filterProvidersByContextincore/bifrost.gothat readsBifrostContextKeyAvailableProvidersfrom the request context and filters the provider list before fan-out. If the context value is malformed, it fails closed and returns an empty list.applyListModelsVirtualKeyProviderFilterin a newlist_models_vk.gohandler file. When a virtual key is present on aGET /v1/modelsrequest with no explicit provider, it resolves the virtual key, extracts its allowed providers, and setsBifrostContextKeyAvailableProviderson the Bifrost context before the fan-out occurs. Inactive or missing virtual keys are silently skipped; lookup failures return an error response.listModelshandler ininference.gonow callsapplyListModelsVirtualKeyProviderFilterbefore dispatching toListAllModelswhen no specific provider is requested.BifrostContextKeyAvailableProviderscomment to clarify it is set by internal bifrost components rather than bifrost alone.Type of change
Affected areas
How to test
go test ./core/... ./transports/bifrost-http/handlers/...GET /v1/modelsrequest with a virtual key scoped to a subset of providers and confirm only those providers' models are returned with no governance rejection errors in the logs.GET /v1/modelsrequest without a virtual key and confirm all configured providers are still queried.GET /v1/modelsrequest with an inactive or unknown virtual key and confirm all providers are still queried without errors.Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
The virtual key lookup fails closed on malformed context values and returns a 503 if the config store is unavailable, preventing unintended provider exposure. An unrecognized or inactive virtual key falls through without restricting providers, preserving existing behavior.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
New Features
Tests