feat: add KeyID filter to ListModelsRequest to scope calls to a single key - #4036
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional KeyID to BifrostListModelsRequest and makes requestWorker filter eligible keys to that ID; if no key matches the worker emits a BifrostError and skips provider handling. A new filterKeysByID helper and unit test validate filtering and non-mutation. ChangesSingle-key model listing scope
Sequence Diagram(s)sequenceDiagram
participant RequestWorker
participant filterKeysByID
participant Provider
RequestWorker->>filterKeysByID: filter(keys, KeyID)
filterKeysByID-->>RequestWorker: filteredKeys
alt filteredKeys empty
RequestWorker->>RequestWorker: send BifrostError on req.Err
else
RequestWorker->>Provider: call ListModels with filteredKeys
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
Comment |
8f5a92c to
e78233f
Compare
Confidence Score: 5/5Safe to merge — a small, focused change that adds an internal routing hint to ListModels with no impact on other request types. The No files require special attention. Important Files Changed
Reviews (14): Last reviewed commit: "feat: adds key param in list models" | Re-trigger Greptile |
5163c7a to
ba81f93
Compare
e78233f to
8056692
Compare
ba81f93 to
9b90058
Compare
8056692 to
5895dc2
Compare
9b90058 to
6faef35
Compare
5895dc2 to
6ce23f5
Compare
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 `@core/bifrost.go`:
- Around line 5994-6005: The error currently sent to req.Err
(schemas.BifrostError creation in the error branch) exposes the requested key id
(target) and provider.GetProviderKey() to clients; change the client-facing
Message to a generic string like "requested key not found" without embedding
target or provider, and remove or redact provider/target from any fields that
flow to HTTP responses (e.g., ExtraFields.Provider, OriginalModelRequested,
ResolvedModelUsed) while emitting the full identifiers to an internal debug log
instead (use the existing logger in scope rather than returning them in
schemas.BifrostError). Ensure req.Err still receives a schemas.BifrostError but
with non-sensitive client message and only write sensitive details to internal
logs.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7acb0f2-538d-49df-b25c-a599881ad2a8
📒 Files selected for processing (2)
core/bifrost.gocore/schemas/models.go
5f43cbc to
99af5b8
Compare
396eb1d to
ff79d23
Compare
99af5b8 to
8934e2e
Compare
ff79d23 to
435aa3d
Compare
8934e2e to
400b52e
Compare
435aa3d to
8089c10
Compare
400b52e to
e77b1f7
Compare
8089c10 to
b79deca
Compare
1f7578f to
6300061
Compare
0cee600 to
94d7bdd
Compare
4ec2730 to
b6de129
Compare
94d7bdd to
50dcfba
Compare
b6de129 to
b66eb67
Compare
50dcfba to
b53280a
Compare
b53280a to
b982660
Compare
b66eb67 to
215242f
Compare
Merge activity
|

Summary
Adds a
KeyIDfield toBifrostListModelsRequestthat allows callers to scope aListModelscall to a single configured key by ID. This enables the catalog composer (and other callers) to cache list-models responses on a per-key basis without needing an extra round-trip, and prevents the provider from aggregating results across all configured keys when only one key's models are needed.Changes
KeyID *stringtoBifrostListModelsRequest— when set, the request worker filters the available keys down to the single key matching that ID before dispatching the request. If no key matches, aBifrostErroris returned immediately.json:"-"so it is never forwarded to the provider; it is purely an internal routing hint.Type of change
Affected areas
How to test
go version go test ./...To validate manually, issue a
ListModelsrequest withKeyIDset to a valid key ID and confirm only that key's models are returned. Then setKeyIDto an unknown ID and confirm aBifrostErroris returned with a message indicating no key was found.Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
KeyIDis an internal-only field (json:"-") and is never transmitted to the provider. No secrets or PII are introduced. The error message includes the key ID string and provider key name, which should be treated as internal identifiers — ensure these are not surfaced in public-facing error responses.Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit