Conversation
Collaborator
Author
|
|
This was referenced Apr 26, 2026
feat: add opt-in per-request overrides for content logging and raw request/response visibility
#3066
Merged
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds model-catalog provider-resolution and logging, threads config into async request preparation, centralizes JSON prepare logic with provider/model resolution, extends context metadata for model-catalog resolution and disable-content-logging header, and minor router/import cleanups and changelog entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant FastHTTP as FastHTTP Handler
participant Prepare as prepareRequest / resolveModelAndProvider
participant Catalog as ModelCatalog (via Config)
participant Context as ConvertToBifrostContext / Router
Client->>FastHTTP: POST inference request (possibly model without provider)
FastHTTP->>Prepare: Unmarshal JSON / collect unknown fields
Prepare->>Catalog: GetProvidersForModel(model)
Catalog-->>Prepare: provider options / resolved provider
Prepare->>Context: attach ModelCatalogResolution to RequestCtx
Context->>Context: append RoutingEngineModelCatalog routing log
FastHTTP->>Context: submit job / respond
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Confidence Score: 4/5Safe to merge after addressing open thread concerns; no new critical issues found in this review pass. No new P0/P1 issues found in this review. The generic
Important Files Changed
Reviews (8): Last reviewed commit: "feat: added default provider selection o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/inference.go (1)
1149-1158:⚠️ Potential issue | 🟡 MinorAvoid trimming rerank inputs in the transport layer.
strings.TrimSpacehere rejects whitespace-only queries/documents before the provider sees them, which is stricter than the passthrough policy used in this package. Check only for empty strings and let the provider reject unsupported content.Suggested fix
- if strings.TrimSpace(req.Query) == "" { + if req.Query == "" { return nil, nil, fmt.Errorf("query is required for rerank") } @@ - if strings.TrimSpace(doc.Text) == "" { + if doc.Text == "" { return nil, nil, fmt.Errorf("document text is required for rerank at index %d", i) } }Based on learnings, "In the Bifrost HTTP handlers under transports/bifrost-http/handlers, implement minimal input validation for user-provided fields (e.g., prompts) and avoid trimming whitespace. Treat Bifrost as a passthrough gateway: forward inputs as-is to providers if accepted, and return provider rejections to the caller."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/inference.go` around lines 1149 - 1158, The current validation in the rerank handler uses strings.TrimSpace on req.Query and doc.Text which rejects whitespace-only inputs; change these checks to only test for empty string (e.g., req.Query == "" and doc.Text == "") so inputs are forwarded verbatim to providers; update the three validations that reference strings.TrimSpace(req.Query), len(req.Documents) (keep this check), and strings.TrimSpace(doc.Text) to use plain empty-string checks and keep the same error messages and returned fmt.Errorf values so provider rejections are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 957-971: The code currently only consumes and deletes the legacy
"max_tokens" alias when req.ChatParameters.MaxCompletionTokens is nil, leaving
the alias in base.ExtraParams if the canonical field is already set; change the
logic in the block around base.ExtraParams handling so you always detect and
delete "max_tokens" from base.ExtraParams (using delete(base.ExtraParams,
"max_tokens")) regardless of whether req.ChatParameters.MaxCompletionTokens is
already set, and add an explicit conflict check in that same block comparing the
parsed max_tokens value to *req.ChatParameters.MaxCompletionTokens and
return/reject or log an error if they differ; ensure
req.ChatParameters.ExtraParams is still assigned to base.ExtraParams after this
removal.
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 191-204: The block in ConvertToBifrostContext reads
FastHTTPUserValueModelCatalogResolution from the fasthttp context and
unconditionally appends logs/markers, causing duplicates on repeated
conversions; after you append the routing log via
bifrostCtx.AppendRoutingEngineLog and add the marker via
schemas.AppendToContextList (and only if res != nil), clear the stored
resolution from the fasthttp context (e.g., call
ctx.SetUserValue(FastHTTPUserValueModelCatalogResolution, nil) or otherwise
remove it) so subsequent ConvertToBifrostContext calls won't reprocess the same
ModelCatalogResolution and won't create duplicate entries; alternatively, you
can guard by checking BifrostContextKeyRoutingEnginesUsed in bifrostCtx before
appending, but prefer clearing FastHTTPUserValueModelCatalogResolution
immediately after use to keep behavior local to ModelCatalogResolution handling.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 1149-1158: The current validation in the rerank handler uses
strings.TrimSpace on req.Query and doc.Text which rejects whitespace-only
inputs; change these checks to only test for empty string (e.g., req.Query == ""
and doc.Text == "") so inputs are forwarded verbatim to providers; update the
three validations that reference strings.TrimSpace(req.Query),
len(req.Documents) (keep this check), and strings.TrimSpace(doc.Text) to use
plain empty-string checks and keep the same error messages and returned
fmt.Errorf values so provider rejections are preserved.
🪄 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: 566d65bf-9ed2-4310-a0ce-9db336474502
📒 Files selected for processing (6)
core/schemas/bifrost.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.gotransports/changelog.md
Pratham-Mishra04
force-pushed
the
04-26-feat_add_request_level_support_for_disable_content_storage_in_loggin_plugin
branch
from
5bf2a03 to
da454c3
Compare
Pratham-Mishra04
force-pushed
the
04-26-feat_added_default_provider_selection_on_inference_routes
branch
from
bad7b10 to
75a20e2
Compare
Merged
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/inference.go (1)
1362-1421:⚠️ Potential issue | 🟠 MajorMultipart transcription requests lost
fallbackshandling.This function now resolves the primary model through
resolveModelAndProvider, but it never readsform.Value["fallbacks"]or propagates parsed fallbacks into the returned transcription request. JSON routes still preserve fallbacks viaprepareRequest, and the other multipart image paths keep their fallback parsing, so transcriptions now silently lose failover behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/inference.go` around lines 1362 - 1421, prepareTranscriptionRequest is parsing multipart form data but never reads or propagates form.Value["fallbacks"], so transcriptions lose configured failovers; update prepareTranscriptionRequest to read the "fallbacks" form value (if present), JSON-unmarshal it into the same fallback type used elsewhere (the Bifrost request's fallback type), and assign it to the returned *schemas.BifrostTranscriptionRequest (the Fallbacks field) before returning; reuse the same parsing/validation logic as prepareRequest/other multipart handlers and ensure any unmarshal error is returned (reference: prepareTranscriptionRequest, resolveModelAndProvider, and schemas.BifrostTranscriptionRequest.Fallbacks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 68-82: The current auto-resolution selects providers[0] from
mc.GetProvidersForModel(modelName) which can pick providers not enabled in this
deployment; fix by filtering that providers slice against the deployment's
enabled providers from config.GetAvailableProviders() before choosing a match.
In the block using config.ModelCatalog and mc.GetProvidersForModel, call
config.GetAvailableProviders(), compute the intersection (keep only providers
present/allowed by GetAvailableProviders()), replace AllProviders with the
filtered list, set ResolvedProvider and provider to the first element of the
filtered list, and return the same "no providers found" error if the filtered
list is empty.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 1362-1421: prepareTranscriptionRequest is parsing multipart form
data but never reads or propagates form.Value["fallbacks"], so transcriptions
lose configured failovers; update prepareTranscriptionRequest to read the
"fallbacks" form value (if present), JSON-unmarshal it into the same fallback
type used elsewhere (the Bifrost request's fallback type), and assign it to the
returned *schemas.BifrostTranscriptionRequest (the Fallbacks field) before
returning; reuse the same parsing/validation logic as prepareRequest/other
multipart handlers and ensure any unmarshal error is returned (reference:
prepareTranscriptionRequest, resolveModelAndProvider, and
schemas.BifrostTranscriptionRequest.Fallbacks).
🪄 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: 07eae189-fe4f-4d89-bc58-a3cdb2965456
📒 Files selected for processing (6)
core/schemas/bifrost.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.gotransports/changelog.md
✅ Files skipped from review due to trivial changes (4)
- core/schemas/bifrost.go
- transports/bifrost-http/integrations/router.go
- transports/changelog.md
- transports/bifrost-http/handlers/asyncinference.go
Pratham-Mishra04
force-pushed
the
04-26-feat_add_request_level_support_for_disable_content_storage_in_loggin_plugin
branch
from
da454c3 to
c2b6cc9
Compare
Pratham-Mishra04
force-pushed
the
04-26-feat_added_default_provider_selection_on_inference_routes
branch
from
75a20e2 to
ab521e0
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
transports/bifrost-http/lib/ctx.go (1)
491-496: Remove the duplicatedx-bf-disable-content-loggingbranch.The same header handler already exists at Lines 511-516, so this copy is dead once the first match returns. Keeping both branches makes later changes easy to miss.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 491 - 496, Remove the duplicated header branch that checks keyStr == "x-bf-disable-content-logging" by deleting the earlier block (the one that calls strconv.ParseBool(string(value)) and then bifrostCtx.SetValue(schemas.BifrostContextKeyDisableContentLogging, b) and returns true), leaving only the single canonical handler (the later branch at lines ~511-516) to avoid dead code and future maintenance mistakes; ensure you keep the remaining handler that performs strconv.ParseBool and sets schemas.BifrostContextKeyDisableContentLogging via bifrostCtx.SetValue.transports/bifrost-http/handlers/inference.go (1)
72-81: Make bare-model auto-resolution choose a provider explicitly.Picking
providers[0]turns the catalog's returned slice order into routing policy. If that order ever changes, identical bare-model requests can silently switch providers. Please normalize the list first here—stable sort or config-backed priority—before persistingResolvedProviderand logging “selecting first”.Based on learnings:
(*ModelCatalog).GetProvidersForModel(model)uses themodelPoolmap it iterates over.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/inference.go` around lines 72 - 81, The current bare-model auto-resolution in inference.go uses the catalog-returned slice order (providers := mc.GetProvidersForModel(modelName)) and then persists providers[0] into ModelCatalogResolution and provider, which can silently change routing; fix this by normalizing/sorting the providers slice deterministically before selecting a winner (e.g., stable sort by a unique provider field such as Provider.Name or ID or apply a config-backed priority mapping), then set ctx.SetUserValue(lib.FastHTTPUserValueModelCatalogResolution, &lib.ModelCatalogResolution{ResolvedProvider: providers[0], AllProviders: providers}) and log the selected provider explicitly so selection is reproducible and auditable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 72-81: The current bare-model auto-resolution in inference.go uses
the catalog-returned slice order (providers :=
mc.GetProvidersForModel(modelName)) and then persists providers[0] into
ModelCatalogResolution and provider, which can silently change routing; fix this
by normalizing/sorting the providers slice deterministically before selecting a
winner (e.g., stable sort by a unique provider field such as Provider.Name or ID
or apply a config-backed priority mapping), then set
ctx.SetUserValue(lib.FastHTTPUserValueModelCatalogResolution,
&lib.ModelCatalogResolution{ResolvedProvider: providers[0], AllProviders:
providers}) and log the selected provider explicitly so selection is
reproducible and auditable.
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 491-496: Remove the duplicated header branch that checks keyStr ==
"x-bf-disable-content-logging" by deleting the earlier block (the one that calls
strconv.ParseBool(string(value)) and then
bifrostCtx.SetValue(schemas.BifrostContextKeyDisableContentLogging, b) and
returns true), leaving only the single canonical handler (the later branch at
lines ~511-516) to avoid dead code and future maintenance mistakes; ensure you
keep the remaining handler that performs strconv.ParseBool and sets
schemas.BifrostContextKeyDisableContentLogging via bifrostCtx.SetValue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58e63db4-c8a1-4e2e-a721-62a7f2432dcc
📒 Files selected for processing (6)
core/schemas/bifrost.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.gotransports/changelog.md
✅ Files skipped from review due to trivial changes (4)
- transports/bifrost-http/integrations/router.go
- core/schemas/bifrost.go
- transports/changelog.md
- transports/bifrost-http/handlers/asyncinference.go
Pratham-Mishra04
force-pushed
the
04-26-feat_add_request_level_support_for_disable_content_storage_in_loggin_plugin
branch
from
c2b6cc9 to
6e1fb4a
Compare
Pratham-Mishra04
force-pushed
the
04-26-feat_added_default_provider_selection_on_inference_routes
branch
from
ab521e0 to
25b640f
Compare
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
transports/bifrost-http/lib/ctx.go (1)
491-496: Remove the duplicatex-bf-disable-content-loggingbranch.This block duplicates the existing handler at Line 511, so the later branch is now unreachable. Keeping a single branch here avoids drift the next time this header logic changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 491 - 496, The duplicate header-handling branch for "x-bf-disable-content-logging" should be removed: locate the branch that checks if keyStr == "x-bf-disable-content-logging" (the block that parses strconv.ParseBool(value) and calls bifrostCtx.SetValue with schemas.BifrostContextKeyDisableContentLogging) and delete this earlier/duplicate occurrence so only the remaining handler (the one at the later location) remains; ensure no other logic in that branch is needed before deletion and run tests to confirm behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 959-968: The code currently truncates fractional JSON numbers for
the legacy "max_tokens" alias; update the logic in extractExtraParams handling
base.ExtraParams["max_tokens"] so that if the value is a float64 you verify it
has no fractional part (e.g., value == math.Trunc(value) or value%1 == 0) and
return a validation error instead of converting/truncating when it does; for
integer types keep the existing conversion to set
req.ChatParameters.MaxCompletionTokens and still delete the "max_tokens" key
from base.ExtraParams. Ensure the function returns an appropriate error on
fractional values rather than silently assigning a truncated int (references:
base.ExtraParams, "max_tokens", req.ChatParameters.MaxCompletionTokens,
extractExtraParams).
- Around line 72-81: GetProvidersForModel returns providers from a map-backed
pool so choosing providers[0] is non-deterministic; before setting
ctx.ModelCatalogResolution.ResolvedProvider and local variable provider in
inference.go, sort or deterministically rank the providers slice (e.g., by
provider identifier string or Provider.Name) so the same candidate is always
chosen; update the resolution code that calls GetProvidersForModel (and sets
ResolvedProvider, AllProviders, and provider) to pick the first element after
applying that stable sort/ranking, referencing GetProvidersForModel,
ModelCatalog, ModelCatalogResolution, and the local provider variable.
---
Nitpick comments:
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 491-496: The duplicate header-handling branch for
"x-bf-disable-content-logging" should be removed: locate the branch that checks
if keyStr == "x-bf-disable-content-logging" (the block that parses
strconv.ParseBool(value) and calls bifrostCtx.SetValue with
schemas.BifrostContextKeyDisableContentLogging) and delete this
earlier/duplicate occurrence so only the remaining handler (the one at the later
location) remains; ensure no other logic in that branch is needed before
deletion and run tests to confirm behavior unchanged.
🪄 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: 84752e29-793f-4749-9ba7-b11431106c74
📒 Files selected for processing (6)
core/schemas/bifrost.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.gotransports/changelog.md
✅ Files skipped from review due to trivial changes (3)
- core/schemas/bifrost.go
- transports/changelog.md
- transports/bifrost-http/integrations/router.go
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/bifrost-http/handlers/asyncinference.go
Pratham-Mishra04
force-pushed
the
04-26-feat_added_default_provider_selection_on_inference_routes
branch
from
25b640f to
d9c76a7
Compare
Pratham-Mishra04
force-pushed
the
04-26-feat_add_request_level_support_for_disable_content_storage_in_loggin_plugin
branch
from
6e1fb4a to
cb62c54
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
transports/bifrost-http/handlers/inference.go (2)
955-964:⚠️ Potential issue | 🟡 MinorSilent truncation of fractional
max_tokensvalues.The legacy
max_tokensalias handling convertsfloat64values tointvia truncation (line 960:int(maxTokensFloat)). A fractional value like1.5silently becomes1. Sincemax_tokensshould only accept whole numbers, consider validating that the value has no fractional part and returning an error for invalid inputs.🔧 Suggested validation
if req.ChatParameters.MaxCompletionTokens == nil { if maxTokensFloat, ok := maxTokensVal.(float64); ok { + if maxTokensFloat != float64(int(maxTokensFloat)) { + return nil, nil, fmt.Errorf("max_tokens must be a whole number, got %v", maxTokensFloat) + } maxTokens := int(maxTokensFloat) req.ChatParameters.MaxCompletionTokens = &maxTokens } else if maxTokensInt, ok := maxTokensVal.(int); ok {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/inference.go` around lines 955 - 964, The handler currently silently truncates fractional max_tokens when reading base.ExtraParams["max_tokens"]; update the logic in the block handling base.ExtraParams -> max_tokens to validate that numeric values are whole integers: if the value is a float64, check that math.Modf or equivalent shows zero fractional part and return a clear error (e.g., "max_tokens must be an integer") if not; only then convert to int and assign to req.ChatParameters.MaxCompletionTokens (also preserve existing int handling and still delete the key from base.ExtraParams).
67-78:⚠️ Potential issue | 🟡 MinorNon-deterministic provider selection from map-backed catalog.
GetProvidersForModeliterates over a map-backedmodelPool, soproviders[0]is not stable across requests or process restarts. The same bare model can resolve to different providers, causing flapping in routing logs and potentially inconsistent request routing.Consider sorting or deterministically ranking candidates before selecting the first element.
🔧 Suggested fix
if provider == "" { providers := config.GetProvidersForModel(modelName) if len(providers) == 0 { return "", "", fmt.Errorf("provider is required in model field (format: provider/model) — no providers found for model %q in model catalog", modelName) } + // Sort providers for deterministic selection + sort.Slice(providers, func(i, j int) bool { + return string(providers[i]) < string(providers[j]) + }) ctx.SetUserValue(lib.FastHTTPUserValueModelCatalogResolution, &lib.ModelCatalogResolution{ Model: modelName, ResolvedProvider: providers[0], AllProviders: providers, }) provider = providers[0] }Note: You'll need to add
"sort"to imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/inference.go` around lines 67 - 78, GetProvidersForModel currently returns a slice built from a map-backed modelPool so providers[0] is non-deterministic; change the selection to a deterministic one by sorting or applying a stable ranking before assigning provider. Specifically, in the block that calls GetProvidersForModel(modelName) and uses providers[0] (and sets ctx.SetUserValue(lib.FastHTTPUserValueModelCatalogResolution, &lib.ModelCatalogResolution{...})), sort the providers slice (or apply a deterministic comparator) and then use the first element as the resolved provider and for provider = providers[0]; update imports to include "sort" (or the comparator helper) accordingly.
🧹 Nitpick comments (1)
transports/bifrost-http/lib/ctx.go (1)
491-496: Remove duplicatedx-bf-disable-content-logginghandling block.Line [491]-[496] already handles this header and returns early, so the second branch at Line [511]-[516] is unreachable and should be removed.
♻️ Proposed cleanup
@@ - if keyStr == "x-bf-disable-content-logging" { - if b, err := strconv.ParseBool(string(value)); err == nil { - bifrostCtx.SetValue(schemas.BifrostContextKeyDisableContentLogging, b) - } - return true - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 491 - 496, A duplicate branch handling the "x-bf-disable-content-logging" header exists — remove the redundant second block that parses the header and sets schemas.BifrostContextKeyDisableContentLogging (the branch that checks keyStr == "x-bf-disable-content-logging", parses with strconv.ParseBool and calls bifrostCtx.SetValue) so only the original early-return handling remains; after removal ensure the surrounding header-processing flow and return behavior unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 955-964: The handler currently silently truncates fractional
max_tokens when reading base.ExtraParams["max_tokens"]; update the logic in the
block handling base.ExtraParams -> max_tokens to validate that numeric values
are whole integers: if the value is a float64, check that math.Modf or
equivalent shows zero fractional part and return a clear error (e.g.,
"max_tokens must be an integer") if not; only then convert to int and assign to
req.ChatParameters.MaxCompletionTokens (also preserve existing int handling and
still delete the key from base.ExtraParams).
- Around line 67-78: GetProvidersForModel currently returns a slice built from a
map-backed modelPool so providers[0] is non-deterministic; change the selection
to a deterministic one by sorting or applying a stable ranking before assigning
provider. Specifically, in the block that calls GetProvidersForModel(modelName)
and uses providers[0] (and sets
ctx.SetUserValue(lib.FastHTTPUserValueModelCatalogResolution,
&lib.ModelCatalogResolution{...})), sort the providers slice (or apply a
deterministic comparator) and then use the first element as the resolved
provider and for provider = providers[0]; update imports to include "sort" (or
the comparator helper) accordingly.
---
Nitpick comments:
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 491-496: A duplicate branch handling the
"x-bf-disable-content-logging" header exists — remove the redundant second block
that parses the header and sets schemas.BifrostContextKeyDisableContentLogging
(the branch that checks keyStr == "x-bf-disable-content-logging", parses with
strconv.ParseBool and calls bifrostCtx.SetValue) so only the original
early-return handling remains; after removal ensure the surrounding
header-processing flow and return behavior unaffected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4cf7af02-3735-4a1c-b55c-b1144d44a72d
📒 Files selected for processing (7)
core/schemas/bifrost.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/changelog.md
✅ Files skipped from review due to trivial changes (3)
- core/schemas/bifrost.go
- transports/bifrost-http/integrations/router.go
- transports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/bifrost-http/handlers/asyncinference.go
This was referenced Apr 27, 2026
Contributor
Merge activity
|
akshaydeo
changed the base branch from
04-26-feat_add_request_level_support_for_disable_content_storage_in_loggin_plugin
to
graphite-base/3067
Pratham-Mishra04
force-pushed
the
04-26-feat_added_default_provider_selection_on_inference_routes
branch
from
d9c76a7 to
960f72c
Compare
Pratham-Mishra04
force-pushed
the
graphite-base/3067
branch
from
cb62c54 to
3cf0d10
Compare
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
akshaydeo
force-pushed
the
04-26-feat_added_default_provider_selection_on_inference_routes
branch
from
960f72c to
35e9e2c
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
transports/bifrost-http/handlers/inference.go (2)
67-77:⚠️ Potential issue | 🟠 MajorMake bare-model auto-resolution deterministic.
Picking
providers[0]here makes the resolved provider depend on whatever orderGetProvidersForModelreturns. If that slice is assembled from map iteration, the selected provider—and themodel-catalogrouting log—can flap between equivalent candidates.#!/bin/bash set -euo pipefail # Verify how GetProvidersForModel builds its result and whether map iteration # influences the returned provider order. rg -n -C3 'func .*GetProvidersForModel|for .*range .*modelPool|modelPool' framework/modelcatalogExpected result: if the returned slice is populated from a
rangeover a map-backed structure, sort or explicitly rankprovidersbefore storingResolvedProviderand assigningprovider = providers[0].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/inference.go` around lines 67 - 77, GetProvidersForModel can return providers in nondeterministic order, so before setting ModelCatalogResolution.ResolvedProvider and assigning provider = providers[0] you must deterministically choose a provider: sort or apply an explicit ranking to the returned providers slice (the one assigned to the local variable providers from GetProvidersForModel) and then set ctx.SetUserValue(lib.FastHTTPUserValueModelCatalogResolution, &lib.ModelCatalogResolution{Model: modelName, ResolvedProvider: providers[0], AllProviders: providers}) and provider = providers[0] using that sorted/ranked slice; ensure the sorting/ranking is deterministic (e.g., alphabetical or configured priority) so ResolvedProvider and the routing log are stable.
955-961:⚠️ Potential issue | 🟡 MinorReject fractional
max_tokensaliases instead of truncating them.Line 960 silently turns values like
1.5into1. The legacy alias should only accept whole numbers and return a validation error otherwise.Suggested fix
+import "math" @@ delete(base.ExtraParams, "max_tokens") if req.ChatParameters.MaxCompletionTokens == nil { if maxTokensFloat, ok := maxTokensVal.(float64); ok { + if maxTokensFloat != math.Trunc(maxTokensFloat) { + return nil, nil, fmt.Errorf("max_tokens must be an integer") + } maxTokens := int(maxTokensFloat) req.ChatParameters.MaxCompletionTokens = &maxTokens } else if maxTokensInt, ok := maxTokensVal.(int); ok {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/inference.go` around lines 955 - 961, The code handling the legacy alias base.ExtraParams["max_tokens"] currently truncates fractional values (e.g., 1.5 -> 1); update this in the inference handler so fractional values are rejected with a validation error instead of being silently truncated. Locate the block that inspects maxTokensVal (inference handler around base.ExtraParams, maxTokensVal and req.ChatParameters.MaxCompletionTokens), and change the logic to: only accept whole integers (int types or float64 whose fractional part is zero), and if maxTokensVal is a float64 with non-zero fractional part (or any non-integer type), return/propagate a validation error indicating max_tokens must be a whole number; do not delete the alias from base.ExtraParams or set MaxCompletionTokens when the value is invalid. Ensure the error uses the same validation/error flow as other parameter checks in this handler.
🧹 Nitpick comments (1)
transports/bifrost-http/lib/ctx.go (1)
505-510: Remove the secondx-bf-disable-content-loggingbranch.This adds a new handler for the header, but the same key is still handled again at Line 525. The lower branch is now unreachable, so future edits can diverge unless there is a single parse path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 505 - 510, There is a duplicate header handler for "x-bf-disable-content-logging"; remove the second branch so the header is parsed in exactly one place. Locate the duplicated conditional that checks keyStr == "x-bf-disable-content-logging" (the branch that calls strconv.ParseBool on value and invokes bifrostCtx.SetValue with schemas.BifrostContextKeyDisableContentLogging) and delete the redundant block, keeping the original parse-and-set logic and its return true behavior intact so only one code path handles that header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 67-77: GetProvidersForModel can return providers in
nondeterministic order, so before setting
ModelCatalogResolution.ResolvedProvider and assigning provider = providers[0]
you must deterministically choose a provider: sort or apply an explicit ranking
to the returned providers slice (the one assigned to the local variable
providers from GetProvidersForModel) and then set
ctx.SetUserValue(lib.FastHTTPUserValueModelCatalogResolution,
&lib.ModelCatalogResolution{Model: modelName, ResolvedProvider: providers[0],
AllProviders: providers}) and provider = providers[0] using that sorted/ranked
slice; ensure the sorting/ranking is deterministic (e.g., alphabetical or
configured priority) so ResolvedProvider and the routing log are stable.
- Around line 955-961: The code handling the legacy alias
base.ExtraParams["max_tokens"] currently truncates fractional values (e.g., 1.5
-> 1); update this in the inference handler so fractional values are rejected
with a validation error instead of being silently truncated. Locate the block
that inspects maxTokensVal (inference handler around base.ExtraParams,
maxTokensVal and req.ChatParameters.MaxCompletionTokens), and change the logic
to: only accept whole integers (int types or float64 whose fractional part is
zero), and if maxTokensVal is a float64 with non-zero fractional part (or any
non-integer type), return/propagate a validation error indicating max_tokens
must be a whole number; do not delete the alias from base.ExtraParams or set
MaxCompletionTokens when the value is invalid. Ensure the error uses the same
validation/error flow as other parameter checks in this handler.
---
Nitpick comments:
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 505-510: There is a duplicate header handler for
"x-bf-disable-content-logging"; remove the second branch so the header is parsed
in exactly one place. Locate the duplicated conditional that checks keyStr ==
"x-bf-disable-content-logging" (the branch that calls strconv.ParseBool on value
and invokes bifrostCtx.SetValue with
schemas.BifrostContextKeyDisableContentLogging) and delete the redundant block,
keeping the original parse-and-set logic and its return true behavior intact so
only one code path handles that header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9c78543-0872-4d17-8487-81e1f75b1642
📒 Files selected for processing (7)
core/schemas/bifrost.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/changelog.md
✅ Files skipped from review due to trivial changes (5)
- core/schemas/bifrost.go
- transports/bifrost-http/integrations/router.go
- transports/changelog.md
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/handlers/asyncinference.go
akshaydeo
deleted the
04-26-feat_added_default_provider_selection_on_inference_routes
branch
18 tasks
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 introduces automatic provider resolution via the model catalog when a request's
modelfield contains no provider prefix (e.g.,gpt-4oinstead ofopenai/gpt-4o). It also adds a newx-bf-disable-content-loggingper-request header, fixes a response extra fields type corruption issue under high concurrency streaming, and consolidates duplicated request preparation logic across all inference endpoints into a single genericprepareRequestfunction.Changes
provider/modelprefix),resolveModelAndProviderqueries the model catalog for matching providers. The first matching provider is selected and the resolution is stored on the fasthttp context.ConvertToBifrostContextpicks this up centrally and emits amodel-catalogrouting engine log entry, including all available providers and the one selected. A newRoutingEngineModelCatalog = "model-catalog"constant is added to the schema.prepareRequest[T]function: All per-endpointprepare*Requestfunctions previously duplicated JSON unmarshaling, model/provider parsing, fallback parsing, and extra param extraction. These are now consolidated into a single generic function, with each endpoint only handling its own validation logic.x-bf-disable-content-loggingheader support:ConvertToBifrostContextnow recognizes this header and setsBifrostContextKeyDisableContentLoggingon the bifrost context, enabling per-request content logging suppression.ModelCatalogResolutionstruct andFastHTTPUserValueModelCatalogResolutioncontext key: Introduced to pass resolution metadata from request preparation through to context conversion without coupling the two layers directly.handlerStorefield fromCompletionHandler; all calls now go directly throughh.config.Type of change
Affected areas
How to test
go test ./...Model catalog auto-resolution: Configure a model catalog with at least one model entry. Send a chat completion request with
"model": "gpt-4o"(no provider prefix). The request should succeed, routing to the first provider listed for that model in the catalog. The routing engine log should include amodel-catalogentry showing the available providers and the selected one.Per-request content logging toggle: Send any inference request with the header
x-bf-disable-content-logging: true. Verify that content is excluded from logs for that request.No model catalog configured: Send a request with a bare model name when no model catalog is configured. Expect a
400error:provider is required in model field (format: provider/model) and model catalog is not available.No providers found: Send a request with a bare model name that has no matching entry in the catalog. Expect a
400error indicating no providers were found for that model.Breaking changes
Security considerations
Provider resolution from the model catalog uses only the first matching provider. Callers without a provider prefix will have their provider selected automatically, which should be considered when configuring model catalog entries to avoid unintended provider routing.
Checklist
docs/contributing/README.mdand followed the guidelines