refactor: move model-catalog provider resolution into a modelcatalogresolver PreRequestHook plugin - #3934
Merged
Conversation
|
|
Contributor
|
Warning Review limit reached
More reviews will be available in 27 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (53)
📝 WalkthroughWalkthroughCentralizes provider selection in a new modelcatalogresolver PreRequestHook, removes ctx-driven defaulting from converters and router model-getters, updates transports to defer resolution to hooks, and aligns tests and schema/context keys. ChangesProvider resolution refactor
Sequence Diagram(s)sequenceDiagram
participant Router
participant PreRequestHooks
participant ModelCatalogResolver
participant ModelCatalog
Router->>PreRequestHooks: Run pre request hooks
PreRequestHooks->>ModelCatalogResolver: Resolve provider from model
ModelCatalogResolver->>ModelCatalog: Query providers for model
ModelCatalog-->>ModelCatalogResolver: Candidate providers
ModelCatalogResolver-->>PreRequestHooks: Selected provider and fallbacks
PreRequestHooks-->>Router: Continue routing with provider
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 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 unit tests (beta)
Comment |
Collaborator
Author
This was referenced May 31, 2026
Contributor
Confidence Score: 3/5The refactor is architecturally sound but ships two unresolved issues from prior reviews plus a new plugin with no unit tests, making it risky to merge as-is. The governance loadBalanceProvider no longer sets BifrostContextKeyAvailableProviders, so when all VK-scoped providers are filtered out, the modelcatalogresolver resolves from the raw catalog without any VK constraint knowledge. The checkAnthropicPassthrough change silently breaks Anthropic-prefixed model requests on catalog-less deployments. Neither issue is fixed in this diff, and the new modelcatalogresolver package ships with zero unit tests. plugins/modelcatalogresolver/main.go (no tests; explicit-empty fallbacks not distinguished from unset), transports/bifrost-http/integrations/anthropic.go (prefix-strip without catalog fallback), plugins/governance/main.go (VK provider constraint no longer enforced against catalog resolver) Important Files Changed
Reviews (12): Last reviewed commit: "feat: add model catalog router plugin" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Merged
18 tasks
Pratham-Mishra04
force-pushed
the
06-01-feat_governance_routing_moved_to_prerequesthook
branch
from
May 31, 2026 21:51
8999736 to
7b50d58
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_add_model_catalog_router_plugin
branch
from
May 31, 2026 21:51
e8352f5 to
856c996
Compare
This was referenced Jun 1, 2026
Pratham-Mishra04
force-pushed
the
06-01-feat_governance_routing_moved_to_prerequesthook
branch
from
June 1, 2026 09:53
7b50d58 to
0bc7c65
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_add_model_catalog_router_plugin
branch
from
June 1, 2026 09:53
856c996 to
ceeb945
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_governance_routing_moved_to_prerequesthook
branch
from
June 1, 2026 11:01
0bc7c65 to
af32678
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_add_model_catalog_router_plugin
branch
2 times, most recently
from
June 3, 2026 11:26
a55b42c to
da3318a
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_governance_routing_moved_to_prerequesthook
branch
from
June 3, 2026 11:26
af32678 to
71ffdd3
Compare
This was referenced Jun 3, 2026
Merged
Pratham-Mishra04
force-pushed
the
06-01-feat_governance_routing_moved_to_prerequesthook
branch
from
June 4, 2026 20:49
d255fc5 to
94645c5
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_add_model_catalog_router_plugin
branch
from
June 4, 2026 20:49
15f03b5 to
05c2bc9
Compare
18 tasks
Pratham-Mishra04
force-pushed
the
06-01-feat_governance_routing_moved_to_prerequesthook
branch
from
June 5, 2026 09:48
94645c5 to
dd5e71c
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_add_model_catalog_router_plugin
branch
from
June 5, 2026 09:48
05c2bc9 to
d3f5a32
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_governance_routing_moved_to_prerequesthook
branch
from
June 7, 2026 07:25
dd5e71c to
59a92e7
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_add_model_catalog_router_plugin
branch
from
June 7, 2026 07:25
d3f5a32 to
3010c2b
Compare
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/bedrock/rerank.go (1)
130-130: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueOptional: Consider removing unused context parameter.
The
ctx *schemas.BifrostContextparameter is no longer used after removing theCheckAndSetDefaultProvidercall. If this signature is not part of a shared interface or pattern, consider removing it in a follow-up cleanup.🤖 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/bedrock/rerank.go` at line 130, The method signature for ToBifrostRerankRequest on BedrockRerankRequest still accepts an unused ctx *schemas.BifrostContext parameter; update the signature to remove the unused parameter (change func (req *BedrockRerankRequest) ToBifrostRerankRequest(ctx *schemas.BifrostContext) to func (req *BedrockRerankRequest) ToBifrostRerankRequest()) and then update all call sites to match, or if the signature must remain for an interface, instead rename the param to _ (underscore) to mark it unused; reference the ToBifrostRerankRequest method on BedrockRerankRequest and any callers you adjust.core/schemas/bifrost.go (1)
498-750:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete the request mutators for all request envelopes.
GetRequestFields()can readFile*,Batch*,Container*, andPassthroughRequest, butSetProvider()/SetModel()still ignore many of those same variants. After provider resolution moved into sharedPreRequestHooks, that means a plugin can inspect these requests but fail to write the resolved provider or normalized model back, leaving later validation to reject them or forwarding prefixed models upstream. Based on learnings,RunPreRequestHooksis now the shared mutation point for request provider/model selection.🤖 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/schemas/bifrost.go` around lines 498 - 750, SetProvider and SetModel are incomplete: they must mirror GetRequestFields so plugins can write resolved provider/model back. In SetProvider add cases to set Provider for all File* (FileUploadRequest, FileListRequest, FileRetrieveRequest, FileDeleteRequest, FileContentRequest), Batch* (BatchListRequest, BatchRetrieveRequest, BatchCancelRequest, BatchResultsRequest, BatchDeleteRequest), Container* (ContainerCreateRequest, ContainerListRequest, ContainerRetrieveRequest, ContainerDeleteRequest, ContainerFileCreateRequest, ContainerFileListRequest, ContainerFileRetrieveRequest, ContainerFileContentRequest, ContainerFileDeleteRequest), Video* (VideoGenerationRequest, VideoRetrieveRequest, VideoDownloadRequest, VideoListRequest, VideoDeleteRequest, VideoRemixRequest) and PassthroughRequest. In SetModel add matching branches to set Model (for pointer fields, preserve the existing nil check and assign properly using new(model) or *field = model depending on the struct: e.g., if br.FileListRequest.Model != nil { *br.FileListRequest.Model = model } or br.FileUploadRequest.Model = model when non-pointer) so every variant handled by GetRequestFields is writable by SetModel.
🤖 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/providers/openai/responses.go`:
- Around line 142-163: The current normalization only converts
message.Content.ContentStr to a reasoning block; update the logic in the message
normalization section (targeting message.Content and
ResponsesMessageContentBlock handling) to also normalize any existing non-empty
message.Content.ContentBlocks into blocks with Type set to
schemas.ResponsesOutputMessageContentTypeReasoning (preserving Text via
schemas.Ptr) instead of leaving original types, and drop empty block arrays by
setting message.Content = nil; also add a regression test exercising a replayed
reasoning message that arrives with ContentBlocks (not ContentStr) to assert the
blocks are retyped to ResponsesOutputMessageContentTypeReasoning and that empty
blocks become nil.
In `@core/providers/utils/utils_test.go`:
- Around line 1794-1880: Add cookie-based credential checks to both tests: in
TestExtractProviderResponseHeaders_StripsProviderSecrets and
TestExtractPassthroughProviderResponseHeaders, set cookie/set-cookie headers on
resp (e.g., resp.Header.Set("Cookie", "...") and resp.Header.Set("Set-Cookie",
"...")) and extend the secret lists checked via the lookup function to include
"cookie" and "set-cookie" alongside "authorization", "x-goog-api-key",
"x-api-key" so the assertions validate that cookie and set-cookie are stripped
by ExtractProviderResponseHeaders and ExtractPassthroughProviderResponseHeaders.
- Around line 824-971: The tests use a fixed 200ms time.After to assert
DrainNonSSEStreamReader returns promptly, which is flaky; instead implement a
deterministic handshake between the writer and the goroutine running
DrainNonSSEStreamReader: create a started chan struct{} (or similar) and have
the goroutine that calls DrainNonSSEStreamReader close(started) immediately
after launching, then have the writer goroutine wait for <-started before
writing; remove the time.After selects in
TestDrainNonSSEStreamReader_TinyOpenSSEPrefixReturnsPromptly and
TestDrainNonSSEStreamReader_FragmentedFieldPrefixReturnsPromptly and rely on the
handshake (or, if you prefer a safety net, replace 200ms with a much larger
timeout like 5s) while keeping the existing channels result, writeErr,
firstWriteErr, and suffixWriteErr logic intact.
In `@core/providers/utils/utils.go`:
- Around line 1099-1106: peekHasPrefix incorrectly treats partial buffered data
as a match (causing false SSE detection in DrainNonSSEStreamReader); update
peekHasPrefix to require the reader have at least len(prefix) bytes buffered
before peeking and only compare when full prefix length is available (i.e.,
return false if reader.Buffered() < len(prefix), otherwise peek len(prefix) and
compare), referencing the peekHasPrefix function and its usage in
DrainNonSSEStreamReader.
---
Outside diff comments:
In `@core/providers/bedrock/rerank.go`:
- Line 130: The method signature for ToBifrostRerankRequest on
BedrockRerankRequest still accepts an unused ctx *schemas.BifrostContext
parameter; update the signature to remove the unused parameter (change func (req
*BedrockRerankRequest) ToBifrostRerankRequest(ctx *schemas.BifrostContext) to
func (req *BedrockRerankRequest) ToBifrostRerankRequest()) and then update all
call sites to match, or if the signature must remain for an interface, instead
rename the param to _ (underscore) to mark it unused; reference the
ToBifrostRerankRequest method on BedrockRerankRequest and any callers you
adjust.
In `@core/schemas/bifrost.go`:
- Around line 498-750: SetProvider and SetModel are incomplete: they must mirror
GetRequestFields so plugins can write resolved provider/model back. In
SetProvider add cases to set Provider for all File* (FileUploadRequest,
FileListRequest, FileRetrieveRequest, FileDeleteRequest, FileContentRequest),
Batch* (BatchListRequest, BatchRetrieveRequest, BatchCancelRequest,
BatchResultsRequest, BatchDeleteRequest), Container* (ContainerCreateRequest,
ContainerListRequest, ContainerRetrieveRequest, ContainerDeleteRequest,
ContainerFileCreateRequest, ContainerFileListRequest,
ContainerFileRetrieveRequest, ContainerFileContentRequest,
ContainerFileDeleteRequest), Video* (VideoGenerationRequest,
VideoRetrieveRequest, VideoDownloadRequest, VideoListRequest,
VideoDeleteRequest, VideoRemixRequest) and PassthroughRequest. In SetModel add
matching branches to set Model (for pointer fields, preserve the existing nil
check and assign properly using new(model) or *field = model depending on the
struct: e.g., if br.FileListRequest.Model != nil { *br.FileListRequest.Model =
model } or br.FileUploadRequest.Model = model when non-pointer) so every variant
handled by GetRequestFields is writable by SetModel.
🪄 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: 98e7ca17-84e4-4bc5-83e0-deab21c07263
⛔ Files ignored due to path filters (1)
plugins/modelcatalogresolver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
core/providers/anthropic/responses.gocore/providers/anthropic/text.gocore/providers/bedrock/invoke.gocore/providers/bedrock/rerank.gocore/providers/bedrock/responses.gocore/providers/bedrock/text.gocore/providers/cohere/chat.gocore/providers/cohere/count_tokens.gocore/providers/cohere/embedding.gocore/providers/cohere/rerank.gocore/providers/gemini/embedding.gocore/providers/gemini/images.gocore/providers/gemini/responses.gocore/providers/gemini/speech.gocore/providers/gemini/transcription.gocore/providers/gemini/videos.gocore/providers/openai/chat.gocore/providers/openai/embedding.gocore/providers/openai/images.gocore/providers/openai/responses.gocore/providers/openai/speech.gocore/providers/openai/text.gocore/providers/openai/transcription.gocore/providers/openai/videos.gocore/providers/utils/utils.gocore/providers/utils/utils_test.gocore/providers/vertex/rerank.gocore/schemas/bifrost.goplugins/governance/go.modplugins/governance/httptransportprehook_test.goplugins/governance/main.goplugins/modelcatalogresolver/go.modplugins/modelcatalogresolver/main.go
💤 Files with no reviewable changes (5)
- plugins/modelcatalogresolver/go.mod
- plugins/governance/httptransportprehook_test.go
- plugins/governance/main.go
- plugins/modelcatalogresolver/main.go
- plugins/governance/go.mod
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/bedrock/rerank.go (1)
130-130: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueOptional: Consider removing unused context parameter.
The
ctx *schemas.BifrostContextparameter is no longer used after removing theCheckAndSetDefaultProvidercall. If this signature is not part of a shared interface or pattern, consider removing it in a follow-up cleanup.🤖 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/bedrock/rerank.go` at line 130, The method signature for ToBifrostRerankRequest on BedrockRerankRequest still accepts an unused ctx *schemas.BifrostContext parameter; update the signature to remove the unused parameter (change func (req *BedrockRerankRequest) ToBifrostRerankRequest(ctx *schemas.BifrostContext) to func (req *BedrockRerankRequest) ToBifrostRerankRequest()) and then update all call sites to match, or if the signature must remain for an interface, instead rename the param to _ (underscore) to mark it unused; reference the ToBifrostRerankRequest method on BedrockRerankRequest and any callers you adjust.core/schemas/bifrost.go (1)
498-750:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete the request mutators for all request envelopes.
GetRequestFields()can readFile*,Batch*,Container*, andPassthroughRequest, butSetProvider()/SetModel()still ignore many of those same variants. After provider resolution moved into sharedPreRequestHooks, that means a plugin can inspect these requests but fail to write the resolved provider or normalized model back, leaving later validation to reject them or forwarding prefixed models upstream. Based on learnings,RunPreRequestHooksis now the shared mutation point for request provider/model selection.🤖 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/schemas/bifrost.go` around lines 498 - 750, SetProvider and SetModel are incomplete: they must mirror GetRequestFields so plugins can write resolved provider/model back. In SetProvider add cases to set Provider for all File* (FileUploadRequest, FileListRequest, FileRetrieveRequest, FileDeleteRequest, FileContentRequest), Batch* (BatchListRequest, BatchRetrieveRequest, BatchCancelRequest, BatchResultsRequest, BatchDeleteRequest), Container* (ContainerCreateRequest, ContainerListRequest, ContainerRetrieveRequest, ContainerDeleteRequest, ContainerFileCreateRequest, ContainerFileListRequest, ContainerFileRetrieveRequest, ContainerFileContentRequest, ContainerFileDeleteRequest), Video* (VideoGenerationRequest, VideoRetrieveRequest, VideoDownloadRequest, VideoListRequest, VideoDeleteRequest, VideoRemixRequest) and PassthroughRequest. In SetModel add matching branches to set Model (for pointer fields, preserve the existing nil check and assign properly using new(model) or *field = model depending on the struct: e.g., if br.FileListRequest.Model != nil { *br.FileListRequest.Model = model } or br.FileUploadRequest.Model = model when non-pointer) so every variant handled by GetRequestFields is writable by SetModel.
🤖 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/providers/openai/responses.go`:
- Around line 142-163: The current normalization only converts
message.Content.ContentStr to a reasoning block; update the logic in the message
normalization section (targeting message.Content and
ResponsesMessageContentBlock handling) to also normalize any existing non-empty
message.Content.ContentBlocks into blocks with Type set to
schemas.ResponsesOutputMessageContentTypeReasoning (preserving Text via
schemas.Ptr) instead of leaving original types, and drop empty block arrays by
setting message.Content = nil; also add a regression test exercising a replayed
reasoning message that arrives with ContentBlocks (not ContentStr) to assert the
blocks are retyped to ResponsesOutputMessageContentTypeReasoning and that empty
blocks become nil.
In `@core/providers/utils/utils_test.go`:
- Around line 1794-1880: Add cookie-based credential checks to both tests: in
TestExtractProviderResponseHeaders_StripsProviderSecrets and
TestExtractPassthroughProviderResponseHeaders, set cookie/set-cookie headers on
resp (e.g., resp.Header.Set("Cookie", "...") and resp.Header.Set("Set-Cookie",
"...")) and extend the secret lists checked via the lookup function to include
"cookie" and "set-cookie" alongside "authorization", "x-goog-api-key",
"x-api-key" so the assertions validate that cookie and set-cookie are stripped
by ExtractProviderResponseHeaders and ExtractPassthroughProviderResponseHeaders.
- Around line 824-971: The tests use a fixed 200ms time.After to assert
DrainNonSSEStreamReader returns promptly, which is flaky; instead implement a
deterministic handshake between the writer and the goroutine running
DrainNonSSEStreamReader: create a started chan struct{} (or similar) and have
the goroutine that calls DrainNonSSEStreamReader close(started) immediately
after launching, then have the writer goroutine wait for <-started before
writing; remove the time.After selects in
TestDrainNonSSEStreamReader_TinyOpenSSEPrefixReturnsPromptly and
TestDrainNonSSEStreamReader_FragmentedFieldPrefixReturnsPromptly and rely on the
handshake (or, if you prefer a safety net, replace 200ms with a much larger
timeout like 5s) while keeping the existing channels result, writeErr,
firstWriteErr, and suffixWriteErr logic intact.
In `@core/providers/utils/utils.go`:
- Around line 1099-1106: peekHasPrefix incorrectly treats partial buffered data
as a match (causing false SSE detection in DrainNonSSEStreamReader); update
peekHasPrefix to require the reader have at least len(prefix) bytes buffered
before peeking and only compare when full prefix length is available (i.e.,
return false if reader.Buffered() < len(prefix), otherwise peek len(prefix) and
compare), referencing the peekHasPrefix function and its usage in
DrainNonSSEStreamReader.
---
Outside diff comments:
In `@core/providers/bedrock/rerank.go`:
- Line 130: The method signature for ToBifrostRerankRequest on
BedrockRerankRequest still accepts an unused ctx *schemas.BifrostContext
parameter; update the signature to remove the unused parameter (change func (req
*BedrockRerankRequest) ToBifrostRerankRequest(ctx *schemas.BifrostContext) to
func (req *BedrockRerankRequest) ToBifrostRerankRequest()) and then update all
call sites to match, or if the signature must remain for an interface, instead
rename the param to _ (underscore) to mark it unused; reference the
ToBifrostRerankRequest method on BedrockRerankRequest and any callers you
adjust.
In `@core/schemas/bifrost.go`:
- Around line 498-750: SetProvider and SetModel are incomplete: they must mirror
GetRequestFields so plugins can write resolved provider/model back. In
SetProvider add cases to set Provider for all File* (FileUploadRequest,
FileListRequest, FileRetrieveRequest, FileDeleteRequest, FileContentRequest),
Batch* (BatchListRequest, BatchRetrieveRequest, BatchCancelRequest,
BatchResultsRequest, BatchDeleteRequest), Container* (ContainerCreateRequest,
ContainerListRequest, ContainerRetrieveRequest, ContainerDeleteRequest,
ContainerFileCreateRequest, ContainerFileListRequest,
ContainerFileRetrieveRequest, ContainerFileContentRequest,
ContainerFileDeleteRequest), Video* (VideoGenerationRequest,
VideoRetrieveRequest, VideoDownloadRequest, VideoListRequest,
VideoDeleteRequest, VideoRemixRequest) and PassthroughRequest. In SetModel add
matching branches to set Model (for pointer fields, preserve the existing nil
check and assign properly using new(model) or *field = model depending on the
struct: e.g., if br.FileListRequest.Model != nil { *br.FileListRequest.Model =
model } or br.FileUploadRequest.Model = model when non-pointer) so every variant
handled by GetRequestFields is writable by SetModel.
🪄 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: 98e7ca17-84e4-4bc5-83e0-deab21c07263
⛔ Files ignored due to path filters (1)
plugins/modelcatalogresolver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
core/providers/anthropic/responses.gocore/providers/anthropic/text.gocore/providers/bedrock/invoke.gocore/providers/bedrock/rerank.gocore/providers/bedrock/responses.gocore/providers/bedrock/text.gocore/providers/cohere/chat.gocore/providers/cohere/count_tokens.gocore/providers/cohere/embedding.gocore/providers/cohere/rerank.gocore/providers/gemini/embedding.gocore/providers/gemini/images.gocore/providers/gemini/responses.gocore/providers/gemini/speech.gocore/providers/gemini/transcription.gocore/providers/gemini/videos.gocore/providers/openai/chat.gocore/providers/openai/embedding.gocore/providers/openai/images.gocore/providers/openai/responses.gocore/providers/openai/speech.gocore/providers/openai/text.gocore/providers/openai/transcription.gocore/providers/openai/videos.gocore/providers/utils/utils.gocore/providers/utils/utils_test.gocore/providers/vertex/rerank.gocore/schemas/bifrost.goplugins/governance/go.modplugins/governance/httptransportprehook_test.goplugins/governance/main.goplugins/modelcatalogresolver/go.modplugins/modelcatalogresolver/main.go
💤 Files with no reviewable changes (5)
- plugins/modelcatalogresolver/go.mod
- plugins/governance/httptransportprehook_test.go
- plugins/governance/main.go
- plugins/modelcatalogresolver/main.go
- plugins/governance/go.mod
🛑 Comments failed to post (4)
core/providers/openai/responses.go (1)
142-163:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize existing reasoning blocks, not just string content.
Lines 147-163 only rewrite
ContentStr. A replayed reasoning item that already carriesContentBlockswill still be forwarded with whatever block types it had before, but OpenAI expects reasoning content to be encoded as reasoning blocks. That means round-tripped reasoning messages can still 400 even after this normalization. Please normalize non-emptyContentBlockshere as well, and add a regression test for that replay path. Based on learnings, "if ResponsesReasoning != nil and the response contains content blocks, all content blocks should be treated as reasoning blocks by default."🤖 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/openai/responses.go` around lines 142 - 163, The current normalization only converts message.Content.ContentStr to a reasoning block; update the logic in the message normalization section (targeting message.Content and ResponsesMessageContentBlock handling) to also normalize any existing non-empty message.Content.ContentBlocks into blocks with Type set to schemas.ResponsesOutputMessageContentTypeReasoning (preserving Text via schemas.Ptr) instead of leaving original types, and drop empty block arrays by setting message.Content = nil; also add a regression test exercising a replayed reasoning message that arrives with ContentBlocks (not ContentStr) to assert the blocks are retyped to ResponsesOutputMessageContentTypeReasoning and that empty blocks become nil.Source: Learnings
core/providers/utils/utils_test.go (2)
824-971:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid wall-clock 200ms deadlines in these stream promptness tests.
Both cases fail based on scheduler speed rather than function semantics. Under loaded CI or
-race, the pipe writer / goroutine handoff can exceed 200ms even when the implementation is correct, which makes this suite flaky. Prefer a deterministic handshake or a materially looser timeout.🤖 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_test.go` around lines 824 - 971, The tests use a fixed 200ms time.After to assert DrainNonSSEStreamReader returns promptly, which is flaky; instead implement a deterministic handshake between the writer and the goroutine running DrainNonSSEStreamReader: create a started chan struct{} (or similar) and have the goroutine that calls DrainNonSSEStreamReader close(started) immediately after launching, then have the writer goroutine wait for <-started before writing; remove the time.After selects in TestDrainNonSSEStreamReader_TinyOpenSSEPrefixReturnsPromptly and TestDrainNonSSEStreamReader_FragmentedFieldPrefixReturnsPromptly and rely on the handshake (or, if you prefer a safety net, replace 200ms with a much larger timeout like 5s) while keeping the existing channels result, writeErr, firstWriteErr, and suffixWriteErr logic intact.
1794-1880:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExtend the secret-header regressions to
cookieandset-cookie.These new tests only cover API-key / Authorization branches, but the filter contract here also strips cookie-based credentials. Without those assertions, a future regression in that path will still pass this security suite. Based on learnings,
providerResponseFilterHeadersmust exclude credentials-bearing headers includingauthorization,cookie, andset-cookie.🤖 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_test.go` around lines 1794 - 1880, Add cookie-based credential checks to both tests: in TestExtractProviderResponseHeaders_StripsProviderSecrets and TestExtractPassthroughProviderResponseHeaders, set cookie/set-cookie headers on resp (e.g., resp.Header.Set("Cookie", "...") and resp.Header.Set("Set-Cookie", "...")) and extend the secret lists checked via the lookup function to include "cookie" and "set-cookie" alongside "authorization", "x-goog-api-key", "x-api-key" so the assertions validate that cookie and set-cookie are stripped by ExtractProviderResponseHeaders and ExtractPassthroughProviderResponseHeaders.Source: Learnings
core/providers/utils/utils.go (1)
1099-1106:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid partial-prefix SSE detection false positives.
At Line 1100,
peekHasPrefixtreats partial buffered bytes as a valid match (e.g., just"d"matching"data:"). That can misclassify non-SSE payloads as SSE and skip draining inDrainNonSSEStreamReader.Suggested fix
func peekHasPrefix(reader *bufio.Reader, prefix []byte) bool { - n := min(reader.Buffered(), len(prefix)) - if n == 0 { - return false - } - peeked, err := reader.Peek(n) - return err == nil && bytes.Equal(peeked, prefix[:n]) + peeked, err := reader.Peek(len(prefix)) + if err != nil { + return false + } + return bytes.Equal(peeked, prefix) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func peekHasPrefix(reader *bufio.Reader, prefix []byte) bool { peeked, err := reader.Peek(len(prefix)) if err != nil { return false } return bytes.Equal(peeked, prefix) }🤖 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 1099 - 1106, peekHasPrefix incorrectly treats partial buffered data as a match (causing false SSE detection in DrainNonSSEStreamReader); update peekHasPrefix to require the reader have at least len(prefix) bytes buffered before peeking and only compare when full prefix length is available (i.e., return false if reader.Buffered() < len(prefix), otherwise peek len(prefix) and compare), referencing the peekHasPrefix function and its usage in DrainNonSSEStreamReader.
Pratham-Mishra04
force-pushed
the
06-01-feat_add_model_catalog_router_plugin
branch
from
June 8, 2026 06:54
3010c2b to
5ec0e17
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_governance_routing_moved_to_prerequesthook
branch
from
June 8, 2026 06:55
59a92e7 to
21ae88f
Compare
Contributor
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/providers/utils/utils.go`:
- Around line 270-316: Add unit/integration tests around the
DNS-resolution-and-dial loop that exercise the net.SplitHostPort ->
net.DefaultResolver.LookupIP -> dialing logic: verify behavior when LookupIP
returns an empty list (ensure the "no usable address resolved for %s" branch is
hit), when all resolved IPs are rejected by the filters (ensure lastErr is
returned when present), mixed IPv4/IPv6 responses, and the private-IP filtering
when allowPrivateNetwork is false vs true; target the code paths around the
resolver/dial loop (references: net.SplitHostPort, net.DefaultResolver.LookupIP,
allowPrivateNetwork, network.IsPrivateIP, network.IsLinkLocal, lastErr, and the
dial loop) and use mocked DNS resolver and net.Dialer or test hooks to simulate
each scenario.
🪄 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: 401ca876-797a-42a6-8c67-54fb179fa727
⛔ Files ignored due to path filters (1)
plugins/modelcatalogresolver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
core/providers/anthropic/responses.gocore/providers/anthropic/text.gocore/providers/bedrock/invoke.gocore/providers/bedrock/rerank.gocore/providers/bedrock/responses.gocore/providers/bedrock/text.gocore/providers/cohere/chat.gocore/providers/cohere/count_tokens.gocore/providers/cohere/embedding.gocore/providers/cohere/rerank.gocore/providers/gemini/embedding.gocore/providers/gemini/images.gocore/providers/gemini/responses.gocore/providers/gemini/speech.gocore/providers/gemini/transcription.gocore/providers/gemini/videos.gocore/providers/openai/chat.gocore/providers/openai/embedding.gocore/providers/openai/images.gocore/providers/openai/responses.gocore/providers/openai/speech.gocore/providers/openai/text.gocore/providers/openai/transcription.gocore/providers/openai/videos.gocore/providers/utils/utils.gocore/providers/utils/utils_test.gocore/providers/vertex/rerank.gocore/schemas/bifrost.goplugins/governance/go.modplugins/governance/httptransportprehook_test.goplugins/governance/main.goplugins/modelcatalogresolver/go.modplugins/modelcatalogresolver/main.go
💤 Files with no reviewable changes (5)
- plugins/modelcatalogresolver/go.mod
- plugins/governance/httptransportprehook_test.go
- plugins/governance/main.go
- plugins/modelcatalogresolver/main.go
- plugins/governance/go.mod
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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/providers/utils/utils.go`:
- Around line 270-316: Add unit/integration tests around the
DNS-resolution-and-dial loop that exercise the net.SplitHostPort ->
net.DefaultResolver.LookupIP -> dialing logic: verify behavior when LookupIP
returns an empty list (ensure the "no usable address resolved for %s" branch is
hit), when all resolved IPs are rejected by the filters (ensure lastErr is
returned when present), mixed IPv4/IPv6 responses, and the private-IP filtering
when allowPrivateNetwork is false vs true; target the code paths around the
resolver/dial loop (references: net.SplitHostPort, net.DefaultResolver.LookupIP,
allowPrivateNetwork, network.IsPrivateIP, network.IsLinkLocal, lastErr, and the
dial loop) and use mocked DNS resolver and net.Dialer or test hooks to simulate
each scenario.
🪄 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: 401ca876-797a-42a6-8c67-54fb179fa727
⛔ Files ignored due to path filters (1)
plugins/modelcatalogresolver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
core/providers/anthropic/responses.gocore/providers/anthropic/text.gocore/providers/bedrock/invoke.gocore/providers/bedrock/rerank.gocore/providers/bedrock/responses.gocore/providers/bedrock/text.gocore/providers/cohere/chat.gocore/providers/cohere/count_tokens.gocore/providers/cohere/embedding.gocore/providers/cohere/rerank.gocore/providers/gemini/embedding.gocore/providers/gemini/images.gocore/providers/gemini/responses.gocore/providers/gemini/speech.gocore/providers/gemini/transcription.gocore/providers/gemini/videos.gocore/providers/openai/chat.gocore/providers/openai/embedding.gocore/providers/openai/images.gocore/providers/openai/responses.gocore/providers/openai/speech.gocore/providers/openai/text.gocore/providers/openai/transcription.gocore/providers/openai/videos.gocore/providers/utils/utils.gocore/providers/utils/utils_test.gocore/providers/vertex/rerank.gocore/schemas/bifrost.goplugins/governance/go.modplugins/governance/httptransportprehook_test.goplugins/governance/main.goplugins/modelcatalogresolver/go.modplugins/modelcatalogresolver/main.go
💤 Files with no reviewable changes (5)
- plugins/modelcatalogresolver/go.mod
- plugins/governance/httptransportprehook_test.go
- plugins/governance/main.go
- plugins/modelcatalogresolver/main.go
- plugins/governance/go.mod
🛑 Comments failed to post (1)
core/providers/utils/utils.go (1)
270-316: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Good SSRF protection via DNS resolution and IP filtering.
The manual DNS resolution with IP filtering (unspecified, link-local, private) before dialing closes the DNS rebinding window between URL validation and connection. The logic correctly:
- Rejects dangerous IP classes before attempting connection
- Tries each resolved IP sequentially with proper error tracking
- Preserves loopback for local testing
Recommend verifying test coverage for edge cases:
- Empty IP list from DNS (line 315 path)
- All IPs rejected by filters (would return lastErr from line 313)
- Mixed IPv4/IPv6 responses
- Private IP filtering when allowPrivateNetwork=false vs true
🤖 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 270 - 316, Add unit/integration tests around the DNS-resolution-and-dial loop that exercise the net.SplitHostPort -> net.DefaultResolver.LookupIP -> dialing logic: verify behavior when LookupIP returns an empty list (ensure the "no usable address resolved for %s" branch is hit), when all resolved IPs are rejected by the filters (ensure lastErr is returned when present), mixed IPv4/IPv6 responses, and the private-IP filtering when allowPrivateNetwork is false vs true; target the code paths around the resolver/dial loop (references: net.SplitHostPort, net.DefaultResolver.LookupIP, allowPrivateNetwork, network.IsPrivateIP, network.IsLinkLocal, lastErr, and the dial loop) and use mocked DNS resolver and net.Dialer or test hooks to simulate each scenario.
Pratham-Mishra04
force-pushed
the
06-01-feat_governance_routing_moved_to_prerequesthook
branch
from
June 8, 2026 11:55
21ae88f to
4b9952a
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_add_model_catalog_router_plugin
branch
from
June 8, 2026 11:55
5ec0e17 to
13daf7a
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_governance_routing_moved_to_prerequesthook
branch
from
June 8, 2026 12:24
4b9952a to
29d2b9f
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_add_model_catalog_router_plugin
branch
from
June 8, 2026 12:24
13daf7a to
24abc35
Compare
Madhuvod
force-pushed
the
06-01-feat_governance_routing_moved_to_prerequesthook
branch
from
June 8, 2026 12:25
29d2b9f to
4b9952a
Compare
Madhuvod
force-pushed
the
06-01-feat_add_model_catalog_router_plugin
branch
from
June 8, 2026 12:25
24abc35 to
13daf7a
Compare
Pratham-Mishra04
force-pushed
the
06-01-feat_add_model_catalog_router_plugin
branch
from
June 8, 2026 12:28
13daf7a to
d803fb5
Compare
18 tasks
Contributor
Merge activity
|
akshaydeo
changed the base branch from
06-01-feat_governance_routing_moved_to_prerequesthook
to
graphite-base/3934
June 9, 2026 05:18
This was referenced Jun 9, 2026
Merged
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
3 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
This PR refactors provider resolution for unprefixed model strings out of the integration router and into a dedicated
modelcatalogresolverbuilt-in plugin. Previously, each integration route used aGetRequestModelcallback and inline model catalog logic to resolve a provider before the request reached the core pipeline. Now, that responsibility is centralized in aPreRequestHookplugin that runs as the last routing layer, after governance routing rules and load balancing plugins have had a chance to set a provider.Additionally,
PreRequestHookerrors are made non-blocking: instead of failing the request immediately on any plugin error, errors are now logged as warnings and the pipeline continues to the next plugin — matching the existing semantics ofRunLLMPreHooks.Changes
plugins/modelcatalogresolverpackage: A built-inLLMPluginthat implementsPreRequestHookto resolvereq.Providerfrom the model catalog when no provider was specified. It prefers the integration's canonical provider (viaBifrostContextKeyIntegrationType) when the catalog returns multiple candidates. Registered at position 9 in the built-in plugin order, after all other routing plugins.RunPreRequestHooksmade non-blocking: Plugin errors are now accumulated inp.preHookErrorsand logged as warnings rather than short-circuiting the pipeline and returning aBifrostError. TheBifrost.RunPreRequestHooksmethod signature changes from returning*schemas.BifrostErrortovoid.CheckAndSetDefaultProvider: TheproviderUtils.CheckAndSetDefaultProviderhelper and its associated context keys (BifrostContextKeyAvailableProviders,BifrostContextKeySkipModelCatalogProviderSelection) are removed. All call sites in provider packages (anthropic,bedrock,cohere,gemini,openai,vertex) now pass the static default provider directly toschemas.ParseModelString.GetRequestModelfromRouteConfig: The per-route model getter callbacks and the inline model catalog resolution block inGenericRouter.createHandlerare removed. TheRouteConfigTypeToProvidermap andRequestModelGettertype are also removed.resolveModelAndProviderin inference handler: The function no longer performs catalog lookups or returns errors for missing providers; it only parses the model string. Provider validation is deferred tohandleRequest/handleStreamRequest.RunPreRequestHookssince it no longer returns an error.anthropic,bedrock,cohere,genai,openai): All*ModelGetterfunctions and their references in route configs are removed. ThecheckAnthropicPassthroughfunction no longer setsBifrostContextKeySkipModelCatalogProviderSelection.Type of change
Affected areas
How to test
go test ./..."model": "claude-sonnet-4") through an OpenAI, Anthropic, GenAI, Bedrock, or Cohere integration route and verify the provider is resolved correctly via the model catalog."model": "anthropic/claude-sonnet-4") and verify the explicit provider is respected without catalog lookup.PreRequestHookplugin returning an error does not fail the request — the warning should appear in logs and the pipeline should continue.Breaking changes
Bifrost.RunPreRequestHooksno longer returns*schemas.BifrostError. Any callers outside this repo that check its return value must be updated to remove that check. TheLLMPlugin.PreRequestHookcontract changes: errors are now non-blocking warnings rather than request-terminating failures.The
BifrostContextKeyAvailableProvidersandBifrostContextKeySkipModelCatalogProviderSelectioncontext keys are removed. Any plugins or middleware that set or read these keys must be updated.Related issues
Security considerations
No new auth, secrets, or PII handling introduced. The model catalog resolver only reads from an in-memory catalog and writes to the request's provider field.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
New Features
Refactor
Tests/Chores