feat: add RoutingInfo to response/error extra fields with fallback and key alias signals - #4020
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:
📝 WalkthroughWalkthroughThis PR centralizes request routing metadata by adding a RoutingInfo schema and BuildRoutingInfo helper, and propagates per-attempt and primary-attempt routing metadata through fallback and retry flows for streaming and non-streaming requests. ChangesRequest Routing Metadata Observability
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 unit tests (beta)
Comment |
|
|
Confidence Score: 3/5The new RoutingInfo feature has incomplete coverage: multiple pre-queue error paths and the non-streaming post-hook path never call PopulateRoutingInfo, so consumers who have adopted the new API will silently receive empty routing data on those paths. Two distinct gaps produce incorrect data for consumers who have migrated to RoutingInfo: early-exit errors (queue/tracer/context failures, short-circuit plugin responses) always return empty RoutingInfo while deprecated fields are correctly populated, and the non-streaming tryRequest path does not re-stamp RoutingInfo after RunPostLLMHooks, unlike the streaming postHookRunner. Both gaps are on the primary non-streaming request path. core/bifrost.go — the pre-queue error returns and the post-RunPostLLMHooks branches in tryRequest need PopulateRoutingInfo calls to match the streaming postHookRunner pattern. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
7f83e5f to
7795c41
Compare
a01b109 to
2992112
Compare
live model cache store and port keyconfig regression tests for alias/model isolation
#4034
2992112 to
ddb8535
Compare
7795c41 to
53ed5e5
Compare
ddb8535 to
f8f177d
Compare
53ed5e5 to
6fe1303
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 6095-6100: The variable attemptRoutingInfo (type
schemas.RoutingInfo) must be initialized before the retry loop so post-retry
population doesn't overwrite existing ExtraFields with a zero-value when the
per-attempt closure never ran; set attemptRoutingInfo to the current routing
snapshot before retries begin (e.g., copy the resolved/current routing info used
to call PopulateExtraFields), keep updating it inside the per-attempt closure as
already implemented, and when applying it after retries (where
PopulateExtraFields/ExtraFields are touched) only overwrite routing-derived
ExtraFields if attemptRoutingInfo is non-zero/has meaningful fields —
alternatively check for a zero-value and skip the overwrite. Ensure changes
reference attemptRoutingInfo, PopulateExtraFields, and the post-retry apply
logic so the initial non-nil routing metadata is preserved when attempts never
run.
🪄 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: 25c0075d-ca6c-445d-92e1-db6cfe02ab42
📒 Files selected for processing (4)
core/bifrost.gocore/schemas/account.gocore/schemas/bifrost.goframework/streaming/types.go
f8f177d to
bea64ee
Compare
6fe1303 to
3d70a3f
Compare
bea64ee to
768db39
Compare
3d70a3f to
139c90d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 4851-4858: The success path for fallback streaming doesn't mark
returned chunks as fallback: after calling bifrost.tryStreamRequest(ctx,
fallbackReq) you must apply SetFallbackRoutingInfo to the successful stream
result as well as to fallbackErr so that returned chunk messages carry
is_fallback=true and primary_* fields are nil; locate the call sites around
tryStreamRequest/ result / fallbackErr and invoke the same
SetFallbackRoutingInfo(provider, model) (or a helper that wraps the result
channel to annotate each emitted chunk) for the successful channel return path
(also repeat the same change at the other occurrence referenced near the
tryStreamRequest usage around the 6201-6219 region).
- Around line 4735-4740: The current code stamps fallback routing info using the
outer-scope variables `provider` and `model` immediately after calling
`tryRequest`/`tryStreamRequest`, which can be stale if a `PreLLMHook` rerouted
the request; update the logic in the `tryRequest`/`tryStreamRequest` handling
(where `result.SetFallbackRoutingInfo(provider, model)` and
`fallbackErr.SetFallbackRoutingInfo(provider, model)` are called) to first read
the completed primary attempt's `RoutingInfo` (from the returned `result` or
`fallbackErr`) for `primary_provider`/`primary_model` and use those values when
present, falling back to the outer `provider`/`model` only if the RoutingInfo
fields are empty; follow the same change for the equivalent `tryStreamRequest`
block and rely on how `BifrostRequest.UpdateProvider` mutates the inner request
(accessible via `preReq.GetRequestFields()`) so rerouted provider/model are
preserved in the completed response before stamping fallback metadata.
- Around line 939-950: The validation currently treats a non-nil
req.PreviousResponseID as present even if it's empty/whitespace, allowing empty
compaction requests through; update the condition that returns the "input not
provided" BifrostError to consider PreviousResponseID empty when it's nil OR
when strings.TrimSpace(*req.PreviousResponseID) == "" (i.e., treat
blank/whitespace previous_response_id as missing), and ensure you import/use the
strings package and preserve the existing isLargePayloadPassthrough(ctx) check.
In `@framework/streaming/types.go`:
- Line 247: The ProcessedStreamResponse struct's RoutingInfo field is never
copied into the outgoing extra_fields.routing_info during conversion; update the
ToBifrostResponse (and any other stream-to-response conversion helpers handling
ProcessedStreamResponse) to set the outgoing response's ExtraFields.RoutingInfo
(or ExtraFields["routing_info"] equivalent) from
ProcessedStreamResponse.RoutingInfo so fallback/alias routing metadata is
preserved; locate the conversion function named ToBifrostResponse and any
similar converters that build the final response and add a single assignment to
copy RoutingInfo into ExtraFields before returning the response.
🪄 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: 20d4ab02-785e-4b1a-a992-8335591664c4
📒 Files selected for processing (4)
core/bifrost.gocore/schemas/account.gocore/schemas/bifrost.goframework/streaming/types.go
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
🤖 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 4851-4858: The success path for fallback streaming doesn't mark
returned chunks as fallback: after calling bifrost.tryStreamRequest(ctx,
fallbackReq) you must apply SetFallbackRoutingInfo to the successful stream
result as well as to fallbackErr so that returned chunk messages carry
is_fallback=true and primary_* fields are nil; locate the call sites around
tryStreamRequest/ result / fallbackErr and invoke the same
SetFallbackRoutingInfo(provider, model) (or a helper that wraps the result
channel to annotate each emitted chunk) for the successful channel return path
(also repeat the same change at the other occurrence referenced near the
tryStreamRequest usage around the 6201-6219 region).
- Around line 4735-4740: The current code stamps fallback routing info using the
outer-scope variables `provider` and `model` immediately after calling
`tryRequest`/`tryStreamRequest`, which can be stale if a `PreLLMHook` rerouted
the request; update the logic in the `tryRequest`/`tryStreamRequest` handling
(where `result.SetFallbackRoutingInfo(provider, model)` and
`fallbackErr.SetFallbackRoutingInfo(provider, model)` are called) to first read
the completed primary attempt's `RoutingInfo` (from the returned `result` or
`fallbackErr`) for `primary_provider`/`primary_model` and use those values when
present, falling back to the outer `provider`/`model` only if the RoutingInfo
fields are empty; follow the same change for the equivalent `tryStreamRequest`
block and rely on how `BifrostRequest.UpdateProvider` mutates the inner request
(accessible via `preReq.GetRequestFields()`) so rerouted provider/model are
preserved in the completed response before stamping fallback metadata.
- Around line 939-950: The validation currently treats a non-nil
req.PreviousResponseID as present even if it's empty/whitespace, allowing empty
compaction requests through; update the condition that returns the "input not
provided" BifrostError to consider PreviousResponseID empty when it's nil OR
when strings.TrimSpace(*req.PreviousResponseID) == "" (i.e., treat
blank/whitespace previous_response_id as missing), and ensure you import/use the
strings package and preserve the existing isLargePayloadPassthrough(ctx) check.
In `@framework/streaming/types.go`:
- Line 247: The ProcessedStreamResponse struct's RoutingInfo field is never
copied into the outgoing extra_fields.routing_info during conversion; update the
ToBifrostResponse (and any other stream-to-response conversion helpers handling
ProcessedStreamResponse) to set the outgoing response's ExtraFields.RoutingInfo
(or ExtraFields["routing_info"] equivalent) from
ProcessedStreamResponse.RoutingInfo so fallback/alias routing metadata is
preserved; locate the conversion function named ToBifrostResponse and any
similar converters that build the final response and add a single assignment to
copy RoutingInfo into ExtraFields before returning the response.
🪄 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: 20d4ab02-785e-4b1a-a992-8335591664c4
📒 Files selected for processing (4)
core/bifrost.gocore/schemas/account.gocore/schemas/bifrost.goframework/streaming/types.go
🛑 Comments failed to post (4)
core/bifrost.go (3)
939-950:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat blank
previous_response_idas missing input.
req.PreviousResponseID != nillets""or whitespace bypass validation whenInputis empty, so an invalid compaction request is forwarded upstream instead of failing fast.💡 Proposed fix
- if len(req.Input) == 0 && req.PreviousResponseID == nil && !isLargePayloadPassthrough(ctx) { + hasPreviousResponseID := req.PreviousResponseID != nil && strings.TrimSpace(*req.PreviousResponseID) != "" + if len(req.Input) == 0 && !hasPreviousResponseID && !isLargePayloadPassthrough(ctx) {🤖 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/bifrost.go` around lines 939 - 950, The validation currently treats a non-nil req.PreviousResponseID as present even if it's empty/whitespace, allowing empty compaction requests through; update the condition that returns the "input not provided" BifrostError to consider PreviousResponseID empty when it's nil OR when strings.TrimSpace(*req.PreviousResponseID) == "" (i.e., treat blank/whitespace previous_response_id as missing), and ensure you import/use the strings package and preserve the existing isLargePayloadPassthrough(ctx) check.
4735-4740:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winStamp fallback metadata from the actual primary attempt.
provider/modelhere are captured beforetryRequest/tryStreamRequest, butPreLLMHookcan still reroute the primary attempt. When that happens, fallback outcomes report the wrongrouting_info.primary_provider/primary_model. Read those values from the completed primary response/errorRoutingInfoand only fall back to the outer variables when that metadata is empty.Based on learnings, "provider switching from a PreLLMHook plugin is performed by calling BifrostRequest.UpdateProvider(provider), which mutates the inner request ... getProviderQueue reads the updated provider via preReq.GetRequestFields()."
Also applies to: 4851-4857
🤖 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/bifrost.go` around lines 4735 - 4740, The current code stamps fallback routing info using the outer-scope variables `provider` and `model` immediately after calling `tryRequest`/`tryStreamRequest`, which can be stale if a `PreLLMHook` rerouted the request; update the logic in the `tryRequest`/`tryStreamRequest` handling (where `result.SetFallbackRoutingInfo(provider, model)` and `fallbackErr.SetFallbackRoutingInfo(provider, model)` are called) to first read the completed primary attempt's `RoutingInfo` (from the returned `result` or `fallbackErr`) for `primary_provider`/`primary_model` and use those values when present, falling back to the outer `provider`/`model` only if the RoutingInfo fields are empty; follow the same change for the equivalent `tryStreamRequest` block and rely on how `BifrostRequest.UpdateProvider` mutates the inner request (accessible via `preReq.GetRequestFields()`) so rerouted provider/model are preserved in the completed response before stamping fallback metadata.Source: Learnings
4851-4858:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSuccessful fallback streams never advertise that they are fallbacks.
Only
fallbackErrgetsSetFallbackRoutingInfo(...). On the success path the stream channel is returned unchanged, and the chunk post-hook runner only appliesperAttemptRoutingInfo, so chunks from a fallback stream still carryis_fallback=falsewith nilprimary_*. That breaks the new streaming routing-info contract for consumers and observability.Also applies to: 6201-6219
🤖 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/bifrost.go` around lines 4851 - 4858, The success path for fallback streaming doesn't mark returned chunks as fallback: after calling bifrost.tryStreamRequest(ctx, fallbackReq) you must apply SetFallbackRoutingInfo to the successful stream result as well as to fallbackErr so that returned chunk messages carry is_fallback=true and primary_* fields are nil; locate the call sites around tryStreamRequest/ result / fallbackErr and invoke the same SetFallbackRoutingInfo(provider, model) (or a helper that wraps the result channel to annotate each emitted chunk) for the successful channel return path (also repeat the same change at the other occurrence referenced near the tryStreamRequest usage around the 6201-6219 region).framework/streaming/types.go (1)
247-247:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPopulate
RoutingInfoin final stream-to-response conversion.Line 247 introduces
ProcessedStreamResponse.RoutingInfo, butToBifrostResponsenever writes it intoextra_fields.routing_info. Final aggregated streaming responses therefore lose fallback/alias routing metadata even when it was captured upstream.💡 Proposed fix
func (p *ProcessedStreamResponse) ToBifrostResponse() *schemas.BifrostResponse { if p.Data == nil { return nil } @@ switch p.StreamType { @@ } + resp.PopulateRoutingInfo(p.RoutingInfo) return resp }Also applies to: 253-459
🤖 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 `@framework/streaming/types.go` at line 247, The ProcessedStreamResponse struct's RoutingInfo field is never copied into the outgoing extra_fields.routing_info during conversion; update the ToBifrostResponse (and any other stream-to-response conversion helpers handling ProcessedStreamResponse) to set the outgoing response's ExtraFields.RoutingInfo (or ExtraFields["routing_info"] equivalent) from ProcessedStreamResponse.RoutingInfo so fallback/alias routing metadata is preserved; locate the conversion function named ToBifrostResponse and any similar converters that build the final response and add a single assignment to copy RoutingInfo into ExtraFields before returning the response.
139c90d to
decab23
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 4851-4858: The fallback success path only calls
SetFallbackRoutingInfo on fallbackErr, leaving successful streaming results (the
result chan returned from tryStreamRequest) without primary/fallback routing
metadata; to fix, when tryStreamRequest returns a non-nil result channel (the
result variable) wrap that channel with a forwarding goroutine that annotates
each emitted chunk with the same fallback/primary routing info (the metadata you
set via SetFallbackRoutingInfo) before sending it downstream, or alternatively
propagate the primary attempt data into postHookRunner so each chunk emitted
carries routing_info.is_fallback, primary_provider, and primary_model; update
the code around tryStreamRequest, the result handling, and any use of
postHookRunner/SetFallbackRoutingInfo to ensure both error and success streaming
paths attach the fallback routing metadata.
- Around line 939-951: The validation currently treats a non-nil
req.PreviousResponseID as present even when it is an empty or whitespace string;
update the compaction request check to treat blank/whitespace PreviousResponseID
as missing by using strings.TrimSpace on *req.PreviousResponseID (e.g. change
the if to require that PreviousResponseID is non-nil AND
strings.TrimSpace(*req.PreviousResponseID) != ""), and add the strings import if
missing; keep the existing BifrostError return logic (schemas.BifrostError /
schemas.ErrorField) when the input is effectively absent.
In `@core/schemas/account.go`:
- Around line 205-226: VLLMAliasCfg is defined but never included in
AliasConfig, so alias-level vLLM overrides cannot deserialize; add a pointer
field *VLLMAliasCfg to the AliasConfig struct (alongside *AzureAliasCfg,
*VertexAliasCfg, etc.), then update isLegacyShape() and Validate() to consider
this new field (accept its presence in legacy-shape checks and validate its
contents the same way other provider-specific alias cfgs are validated), and
ensure any JSON (un)marshaling or switch logic that handles alias overrides
includes VLLMAliasCfg as well.
In `@core/schemas/bifrost.go`:
- Around line 1098-1197: Add deterministic table-driven unit tests that exercise
syncDeprecatedFromRoutingInfo via the public helpers PopulateRoutingInfo and
SetFallbackRoutingInfo for both BifrostResponse and BifrostError: include cases
for non-fallback vs fallback (set PrimaryModel/PrimaryProvider) and alias vs
non-alias (ResolvedKeyAlias with ModelID vs no alias), asserting
ExtraFields.Provider, ExtraFields.OriginalModelRequested and
ExtraFields.ResolvedModelUsed match the documented derivation rules; cover
nil/empty pointers, and ensure tests call PopulateRoutingInfo before/after
SetFallbackRoutingInfo to validate both code paths. Use subtests or a table with
inputs (RoutingInfo variants) and expected deprecated-field outputs and
reference the functions syncDeprecatedFromRoutingInfo, PopulateRoutingInfo,
SetFallbackRoutingInfo, and the ExtraFields fields to locate code under test.
🪄 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: c43398ea-23f3-4898-b8c4-2b918261cc20
📒 Files selected for processing (4)
core/bifrost.gocore/schemas/account.gocore/schemas/bifrost.goframework/streaming/types.go
💤 Files with no reviewable changes (1)
- framework/streaming/types.go
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
🤖 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 4851-4858: The fallback success path only calls
SetFallbackRoutingInfo on fallbackErr, leaving successful streaming results (the
result chan returned from tryStreamRequest) without primary/fallback routing
metadata; to fix, when tryStreamRequest returns a non-nil result channel (the
result variable) wrap that channel with a forwarding goroutine that annotates
each emitted chunk with the same fallback/primary routing info (the metadata you
set via SetFallbackRoutingInfo) before sending it downstream, or alternatively
propagate the primary attempt data into postHookRunner so each chunk emitted
carries routing_info.is_fallback, primary_provider, and primary_model; update
the code around tryStreamRequest, the result handling, and any use of
postHookRunner/SetFallbackRoutingInfo to ensure both error and success streaming
paths attach the fallback routing metadata.
- Around line 939-951: The validation currently treats a non-nil
req.PreviousResponseID as present even when it is an empty or whitespace string;
update the compaction request check to treat blank/whitespace PreviousResponseID
as missing by using strings.TrimSpace on *req.PreviousResponseID (e.g. change
the if to require that PreviousResponseID is non-nil AND
strings.TrimSpace(*req.PreviousResponseID) != ""), and add the strings import if
missing; keep the existing BifrostError return logic (schemas.BifrostError /
schemas.ErrorField) when the input is effectively absent.
In `@core/schemas/account.go`:
- Around line 205-226: VLLMAliasCfg is defined but never included in
AliasConfig, so alias-level vLLM overrides cannot deserialize; add a pointer
field *VLLMAliasCfg to the AliasConfig struct (alongside *AzureAliasCfg,
*VertexAliasCfg, etc.), then update isLegacyShape() and Validate() to consider
this new field (accept its presence in legacy-shape checks and validate its
contents the same way other provider-specific alias cfgs are validated), and
ensure any JSON (un)marshaling or switch logic that handles alias overrides
includes VLLMAliasCfg as well.
In `@core/schemas/bifrost.go`:
- Around line 1098-1197: Add deterministic table-driven unit tests that exercise
syncDeprecatedFromRoutingInfo via the public helpers PopulateRoutingInfo and
SetFallbackRoutingInfo for both BifrostResponse and BifrostError: include cases
for non-fallback vs fallback (set PrimaryModel/PrimaryProvider) and alias vs
non-alias (ResolvedKeyAlias with ModelID vs no alias), asserting
ExtraFields.Provider, ExtraFields.OriginalModelRequested and
ExtraFields.ResolvedModelUsed match the documented derivation rules; cover
nil/empty pointers, and ensure tests call PopulateRoutingInfo before/after
SetFallbackRoutingInfo to validate both code paths. Use subtests or a table with
inputs (RoutingInfo variants) and expected deprecated-field outputs and
reference the functions syncDeprecatedFromRoutingInfo, PopulateRoutingInfo,
SetFallbackRoutingInfo, and the ExtraFields fields to locate code under test.
🪄 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: c43398ea-23f3-4898-b8c4-2b918261cc20
📒 Files selected for processing (4)
core/bifrost.gocore/schemas/account.gocore/schemas/bifrost.goframework/streaming/types.go
💤 Files with no reviewable changes (1)
- framework/streaming/types.go
🛑 Comments failed to post (4)
core/bifrost.go (2)
939-951:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject blank
previous_response_idin compaction validation.
PreviousResponseID != nilcurrently counts as “present” even when it points to"", so a request with no input and an empty ID skips local validation and fails later at the provider boundary. Treat blank/whitespace IDs as missing here.💡 Proposed fix
- if len(req.Input) == 0 && req.PreviousResponseID == nil && !isLargePayloadPassthrough(ctx) { + hasPreviousResponseID := req.PreviousResponseID != nil && strings.TrimSpace(*req.PreviousResponseID) != "" + if len(req.Input) == 0 && !hasPreviousResponseID && !isLargePayloadPassthrough(ctx) {As per coding guidelines, validate all untrusted input before provider calls.
🤖 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/bifrost.go` around lines 939 - 951, The validation currently treats a non-nil req.PreviousResponseID as present even when it is an empty or whitespace string; update the compaction request check to treat blank/whitespace PreviousResponseID as missing by using strings.TrimSpace on *req.PreviousResponseID (e.g. change the if to require that PreviousResponseID is non-nil AND strings.TrimSpace(*req.PreviousResponseID) != ""), and add the strings import if missing; keep the existing BifrostError return logic (schemas.BifrostError / schemas.ErrorField) when the input is effectively absent.Source: Coding guidelines
4851-4858:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSuccessful fallback streams never get fallback routing metadata.
This only annotates
fallbackErr. WhentryStreamRequestsucceeds, the returned channel is passed through unchanged, and the worker-sidepostHookRunneronly stampsperAttemptRoutingInfo. The emitted chunks therefore missrouting_info.is_fallback,primary_provider, andprimary_modelfor successful fallback streams, so the new streaming contract is incomplete.Thread the primary attempt metadata into the streaming chunk path as well, or wrap the returned channel and annotate each emitted chunk before returning it.
🤖 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/bifrost.go` around lines 4851 - 4858, The fallback success path only calls SetFallbackRoutingInfo on fallbackErr, leaving successful streaming results (the result chan returned from tryStreamRequest) without primary/fallback routing metadata; to fix, when tryStreamRequest returns a non-nil result channel (the result variable) wrap that channel with a forwarding goroutine that annotates each emitted chunk with the same fallback/primary routing info (the metadata you set via SetFallbackRoutingInfo) before sending it downstream, or alternatively propagate the primary attempt data into postHookRunner so each chunk emitted carries routing_info.is_fallback, primary_provider, and primary_model; update the code around tryStreamRequest, the result handling, and any use of postHookRunner/SetFallbackRoutingInfo to ensure both error and success streaming paths attach the fallback routing metadata.core/schemas/account.go (1)
205-226:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
VLLMAliasCfgis declared but never reachable fromAliasConfig.
VLLMAliasCfgis added here, butAliasConfighas no field for it, and neitherisLegacyShape()norValidate()account for it. Any alias-level vLLM override therefore has nowhere to deserialize, so configs meant to steer one vLLM key across multiple served models will silently behave as if the override was never set.Possible direction
type AliasConfig struct { ModelID string `json:"model_id"` ModelName *string `json:"model_name,omitempty"` ModelFamily *ModelFamily `json:"model_family,omitempty"` Description string `json:"description,omitempty"` Region *EnvVar `json:"region,omitempty"` *AzureAliasCfg *VertexAliasCfg *BedrockAliasCfg *ReplicateAliasCfg + VLLM *VLLMAliasCfg `json:"vllm,omitempty"` } func (ac AliasConfig) isLegacyShape() bool { return ac.ModelID != "" && ac.ModelName == nil && ac.ModelFamily == nil && ac.Description == "" && ac.Region == nil && ac.AzureAliasCfg == nil && ac.VertexAliasCfg == nil && ac.BedrockAliasCfg == nil && - ac.ReplicateAliasCfg == nil + ac.ReplicateAliasCfg == nil && + ac.VLLM == nil } func (ka KeyAliases) Validate(providerKey ModelProvider) error { // ... + if ac.VLLM != nil && providerKey != VLLM { + return fmt.Errorf("alias %q: vllm sub-config is only valid on VLLM keys (got provider %q)", from, providerKey) + } // ... }Also applies to: 231-240, 276-315
🤖 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/account.go` around lines 205 - 226, VLLMAliasCfg is defined but never included in AliasConfig, so alias-level vLLM overrides cannot deserialize; add a pointer field *VLLMAliasCfg to the AliasConfig struct (alongside *AzureAliasCfg, *VertexAliasCfg, etc.), then update isLegacyShape() and Validate() to consider this new field (accept its presence in legacy-shape checks and validate its contents the same way other provider-specific alias cfgs are validated), and ensure any JSON (un)marshaling or switch logic that handles alias overrides includes VLLMAliasCfg as well.core/schemas/bifrost.go (1)
1098-1197: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add table-driven coverage for routing/deprecated-field sync.
This block now defines the public
routing_infocontract and the deprecated compatibility fields for every response/error path, but the change set ships without schema-level tests for primary vs fallback and alias vs non-alias cases. A regression here will silently relabel responses across providers.As per coding guidelines, “Apply standard Go review practices: … deterministic tests, and table-driven coverage for behavior changes.”
🤖 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 1098 - 1197, Add deterministic table-driven unit tests that exercise syncDeprecatedFromRoutingInfo via the public helpers PopulateRoutingInfo and SetFallbackRoutingInfo for both BifrostResponse and BifrostError: include cases for non-fallback vs fallback (set PrimaryModel/PrimaryProvider) and alias vs non-alias (ResolvedKeyAlias with ModelID vs no alias), asserting ExtraFields.Provider, ExtraFields.OriginalModelRequested and ExtraFields.ResolvedModelUsed match the documented derivation rules; cover nil/empty pointers, and ensure tests call PopulateRoutingInfo before/after SetFallbackRoutingInfo to validate both code paths. Use subtests or a table with inputs (RoutingInfo variants) and expected deprecated-field outputs and reference the functions syncDeprecatedFromRoutingInfo, PopulateRoutingInfo, SetFallbackRoutingInfo, and the ExtraFields fields to locate code under test.Source: Coding guidelines
46cb342 to
a9be3a8
Compare
decab23 to
0f1ec91
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/streaming/types.go (1)
253-459:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTransfer RoutingInfo to BifrostResponse in all stream-type branches.
ToBifrostResponseaddsProcessedStreamResponse.RoutingInfoat line 247 but never transfers it to the resultingBifrostResponse. All six stream-type branches (text, chat, responses, audio, transcription, image) createExtraFieldsstruct literals that omitRoutingInfo, so the routing metadata is lost when converting fromProcessedStreamResponsetoBifrostResponse. This breaks the feature: clients won't seerouting_info(attempt-level provider/model/key, fallback signals, primary-attempt attribution) in streaming responses.Per the
PopulateRoutingInfomethod (context snippet 2), the recommended pattern is to callresp.XxxResponse.PopulateRoutingInfo(p.RoutingInfo)after settingExtraFields, which both stampsRoutingInfoand syncs the deprecatedProvider/OriginalModelRequested/ResolvedModelUsedfields automatically viasyncDeprecatedFromRoutingInfo. This eliminates the need to manually set the deprecated fields in the struct literal.🔧 Proposed fix to transfer RoutingInfo in all branches
Apply this pattern to all six stream-type branches (text, chat, responses, audio, transcription, image). Example for
StreamTypeChat(lines 336-351):resp.ChatResponse = chatResp resp.ChatResponse.ExtraFields = schemas.BifrostResponseExtraFields{ RequestType: schemas.ChatCompletionRequest, - Provider: p.Provider, - OriginalModelRequested: p.RequestedModel, - ResolvedModelUsed: p.ResolvedModel, Latency: p.Data.Latency, } + resp.ChatResponse.PopulateRoutingInfo(p.RoutingInfo) if p.RawRequest != nil {Repeat the same transformation for
StreamTypeText(lines 284–299),StreamTypeResponses(lines 361–377),StreamTypeAudio(lines 384–399),StreamTypeTranscription(lines 406–421), andStreamTypeImage(lines 440–456): remove the deprecated-field assignments (Provider,OriginalModelRequested,ResolvedModelUsed) from the struct literal and addresp.XxxResponse.PopulateRoutingInfo(p.RoutingInfo)immediately after theExtraFieldsassignment.🤖 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 `@framework/streaming/types.go` around lines 253 - 459, ToBifrostResponse is not transferring ProcessedStreamResponse.RoutingInfo into the created BifrostResponse, so routing metadata is lost; for each branch inside ToBifrostResponse (StreamTypeText, StreamTypeChat, StreamTypeResponses, StreamTypeAudio, StreamTypeTranscription, StreamTypeImage) remove setting the deprecated Provider/OriginalModelRequested/ResolvedModelUsed fields in the ExtraFields literal and instead call resp.<Xxx>Response.PopulateRoutingInfo(p.RoutingInfo) immediately after assigning ExtraFields (using the existing PopulateRoutingInfo helper to stamp RoutingInfo and sync deprecated fields), e.g., after resp.ChatResponse.ExtraFields = ... call resp.ChatResponse.PopulateRoutingInfo(p.RoutingInfo); repeat for Text, Responses, Speech, Transcription, and Image branches within the same ToBifrostResponse function.
🤖 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 4851-4858: The fallback stream path currently only calls
SetFallbackRoutingInfo on fallbackErr and returns the raw result channel from
tryStreamRequest, so successful streamed chunks never receive
routing_info.is_fallback/primary_provider/primary_model; fix by wrapping the
returned channel from tryStreamRequest (the variable result) inside a new
goroutine/chan reader in bifrost.tryStreamRequest caller code that reads each
emitted chunk or error and calls SetFallbackRoutingInfo(provider, model) before
forwarding it, or alternatively ensure the per-attempt BuildRoutingInfo used by
the streaming post-hook includes the primary provider/model so emitted chunks
already contain the fallback fields; reference tryStreamRequest, result (the
returned chan), and SetFallbackRoutingInfo to locate and implement the change.
In `@core/schemas/account.go`:
- Around line 205-226: AliasConfig currently never includes VLLMAliasCfg and
both VLLMAliasCfg.ModelName and AliasConfig.ModelName share the same json tag,
making the vLLM override unreachable; fix by adding a nested vLLM field to
AliasConfig (e.g., add a pointer field like VLLM *VLLMAliasCfg
`json:"vllm,omitempty"`), leaving AliasConfig.ModelName as the canonical
top-level name, and keep VLLMAliasCfg.ModelName as-is so its JSON becomes
vllm.model_name (no tag rename required), ensuring callers can express the
vLLM-specific override separately from AliasConfig.ModelName.
In `@core/schemas/bifrost.go`:
- Around line 1103-1125: syncDeprecatedFromRoutingInfo currently only overwrites
non-empty branches and can leave prior values in pooled outputs; first clear the
outputs then re-derive them from info: at the top of
syncDeprecatedFromRoutingInfo, if provider != nil set *provider to the zero
value (empty), if originalModelRequested != nil set *originalModelRequested to
"", and if resolvedModelUsed != nil set *resolved_model_used (resolvedModelUsed)
to ""; then run the existing logic that sets provider, originalModelRequested,
and resolvedModelUsed from info so partial RoutingInfo cannot leak prior-request
data.
---
Outside diff comments:
In `@framework/streaming/types.go`:
- Around line 253-459: ToBifrostResponse is not transferring
ProcessedStreamResponse.RoutingInfo into the created BifrostResponse, so routing
metadata is lost; for each branch inside ToBifrostResponse (StreamTypeText,
StreamTypeChat, StreamTypeResponses, StreamTypeAudio, StreamTypeTranscription,
StreamTypeImage) remove setting the deprecated
Provider/OriginalModelRequested/ResolvedModelUsed fields in the ExtraFields
literal and instead call resp.<Xxx>Response.PopulateRoutingInfo(p.RoutingInfo)
immediately after assigning ExtraFields (using the existing PopulateRoutingInfo
helper to stamp RoutingInfo and sync deprecated fields), e.g., after
resp.ChatResponse.ExtraFields = ... call
resp.ChatResponse.PopulateRoutingInfo(p.RoutingInfo); repeat for Text,
Responses, Speech, Transcription, and Image branches within the same
ToBifrostResponse function.
🪄 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: 2e2a89b2-03f2-4b1b-ac60-df226003e56b
📒 Files selected for processing (4)
core/bifrost.gocore/schemas/account.gocore/schemas/bifrost.goframework/streaming/types.go
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/streaming/types.go (1)
253-459:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTransfer RoutingInfo to BifrostResponse in all stream-type branches.
ToBifrostResponseaddsProcessedStreamResponse.RoutingInfoat line 247 but never transfers it to the resultingBifrostResponse. All six stream-type branches (text, chat, responses, audio, transcription, image) createExtraFieldsstruct literals that omitRoutingInfo, so the routing metadata is lost when converting fromProcessedStreamResponsetoBifrostResponse. This breaks the feature: clients won't seerouting_info(attempt-level provider/model/key, fallback signals, primary-attempt attribution) in streaming responses.Per the
PopulateRoutingInfomethod (context snippet 2), the recommended pattern is to callresp.XxxResponse.PopulateRoutingInfo(p.RoutingInfo)after settingExtraFields, which both stampsRoutingInfoand syncs the deprecatedProvider/OriginalModelRequested/ResolvedModelUsedfields automatically viasyncDeprecatedFromRoutingInfo. This eliminates the need to manually set the deprecated fields in the struct literal.🔧 Proposed fix to transfer RoutingInfo in all branches
Apply this pattern to all six stream-type branches (text, chat, responses, audio, transcription, image). Example for
StreamTypeChat(lines 336-351):resp.ChatResponse = chatResp resp.ChatResponse.ExtraFields = schemas.BifrostResponseExtraFields{ RequestType: schemas.ChatCompletionRequest, - Provider: p.Provider, - OriginalModelRequested: p.RequestedModel, - ResolvedModelUsed: p.ResolvedModel, Latency: p.Data.Latency, } + resp.ChatResponse.PopulateRoutingInfo(p.RoutingInfo) if p.RawRequest != nil {Repeat the same transformation for
StreamTypeText(lines 284–299),StreamTypeResponses(lines 361–377),StreamTypeAudio(lines 384–399),StreamTypeTranscription(lines 406–421), andStreamTypeImage(lines 440–456): remove the deprecated-field assignments (Provider,OriginalModelRequested,ResolvedModelUsed) from the struct literal and addresp.XxxResponse.PopulateRoutingInfo(p.RoutingInfo)immediately after theExtraFieldsassignment.🤖 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 `@framework/streaming/types.go` around lines 253 - 459, ToBifrostResponse is not transferring ProcessedStreamResponse.RoutingInfo into the created BifrostResponse, so routing metadata is lost; for each branch inside ToBifrostResponse (StreamTypeText, StreamTypeChat, StreamTypeResponses, StreamTypeAudio, StreamTypeTranscription, StreamTypeImage) remove setting the deprecated Provider/OriginalModelRequested/ResolvedModelUsed fields in the ExtraFields literal and instead call resp.<Xxx>Response.PopulateRoutingInfo(p.RoutingInfo) immediately after assigning ExtraFields (using the existing PopulateRoutingInfo helper to stamp RoutingInfo and sync deprecated fields), e.g., after resp.ChatResponse.ExtraFields = ... call resp.ChatResponse.PopulateRoutingInfo(p.RoutingInfo); repeat for Text, Responses, Speech, Transcription, and Image branches within the same ToBifrostResponse function.
🤖 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 4851-4858: The fallback stream path currently only calls
SetFallbackRoutingInfo on fallbackErr and returns the raw result channel from
tryStreamRequest, so successful streamed chunks never receive
routing_info.is_fallback/primary_provider/primary_model; fix by wrapping the
returned channel from tryStreamRequest (the variable result) inside a new
goroutine/chan reader in bifrost.tryStreamRequest caller code that reads each
emitted chunk or error and calls SetFallbackRoutingInfo(provider, model) before
forwarding it, or alternatively ensure the per-attempt BuildRoutingInfo used by
the streaming post-hook includes the primary provider/model so emitted chunks
already contain the fallback fields; reference tryStreamRequest, result (the
returned chan), and SetFallbackRoutingInfo to locate and implement the change.
In `@core/schemas/account.go`:
- Around line 205-226: AliasConfig currently never includes VLLMAliasCfg and
both VLLMAliasCfg.ModelName and AliasConfig.ModelName share the same json tag,
making the vLLM override unreachable; fix by adding a nested vLLM field to
AliasConfig (e.g., add a pointer field like VLLM *VLLMAliasCfg
`json:"vllm,omitempty"`), leaving AliasConfig.ModelName as the canonical
top-level name, and keep VLLMAliasCfg.ModelName as-is so its JSON becomes
vllm.model_name (no tag rename required), ensuring callers can express the
vLLM-specific override separately from AliasConfig.ModelName.
In `@core/schemas/bifrost.go`:
- Around line 1103-1125: syncDeprecatedFromRoutingInfo currently only overwrites
non-empty branches and can leave prior values in pooled outputs; first clear the
outputs then re-derive them from info: at the top of
syncDeprecatedFromRoutingInfo, if provider != nil set *provider to the zero
value (empty), if originalModelRequested != nil set *originalModelRequested to
"", and if resolvedModelUsed != nil set *resolved_model_used (resolvedModelUsed)
to ""; then run the existing logic that sets provider, originalModelRequested,
and resolvedModelUsed from info so partial RoutingInfo cannot leak prior-request
data.
---
Outside diff comments:
In `@framework/streaming/types.go`:
- Around line 253-459: ToBifrostResponse is not transferring
ProcessedStreamResponse.RoutingInfo into the created BifrostResponse, so routing
metadata is lost; for each branch inside ToBifrostResponse (StreamTypeText,
StreamTypeChat, StreamTypeResponses, StreamTypeAudio, StreamTypeTranscription,
StreamTypeImage) remove setting the deprecated
Provider/OriginalModelRequested/ResolvedModelUsed fields in the ExtraFields
literal and instead call resp.<Xxx>Response.PopulateRoutingInfo(p.RoutingInfo)
immediately after assigning ExtraFields (using the existing PopulateRoutingInfo
helper to stamp RoutingInfo and sync deprecated fields), e.g., after
resp.ChatResponse.ExtraFields = ... call
resp.ChatResponse.PopulateRoutingInfo(p.RoutingInfo); repeat for Text,
Responses, Speech, Transcription, and Image branches within the same
ToBifrostResponse function.
🪄 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: 2e2a89b2-03f2-4b1b-ac60-df226003e56b
📒 Files selected for processing (4)
core/bifrost.gocore/schemas/account.gocore/schemas/bifrost.goframework/streaming/types.go
🛑 Comments failed to post (3)
core/bifrost.go (1)
4851-4858:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPropagate fallback routing info onto successful stream chunks.
Line 4857 only annotates
fallbackErr. When the fallback stream succeeds, the returned channel is forwarded unchanged, so chunk metadata never getsrouting_info.is_fallback,primary_provider, orprimary_model. The later chunk path only uses attempt-scopedBuildRoutingInfosnapshots (for example at Line 6186 and Line 6203), so it cannot recover the primary-attempt attribution afterward. That leaves streaming fallback responses short of the same contract the non-streaming path now provides.Please either wrap the returned stream here and stamp each emitted response/error chunk with
SetFallbackRoutingInfo(provider, model), or thread the primary provider/model into the per-attempt routing snapshot used by the streaming post-hook path so chunk-level serialization can emit the fallback fields directly.🤖 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/bifrost.go` around lines 4851 - 4858, The fallback stream path currently only calls SetFallbackRoutingInfo on fallbackErr and returns the raw result channel from tryStreamRequest, so successful streamed chunks never receive routing_info.is_fallback/primary_provider/primary_model; fix by wrapping the returned channel from tryStreamRequest (the variable result) inside a new goroutine/chan reader in bifrost.tryStreamRequest caller code that reads each emitted chunk or error and calls SetFallbackRoutingInfo(provider, model) before forwarding it, or alternatively ensure the per-attempt BuildRoutingInfo used by the streaming post-hook includes the primary provider/model so emitted chunks already contain the fallback fields; reference tryStreamRequest, result (the returned chan), and SetFallbackRoutingInfo to locate and implement the change.core/schemas/account.go (1)
205-226:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
VLLMAliasCfgis unreachable in the current schema.
AliasConfignever storesVLLMAliasCfg, and both this type andAliasConfig.ModelNamewant the samejson:"model_name"slot. So a config author cannot represent “canonical model name for pricing/logs” separately from “vLLM backend model override”; the latter is silently impossible to express or validate. Give the vLLM override its own nested field/tag, or rename one of the concepts before shipping this schema.🤖 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/account.go` around lines 205 - 226, AliasConfig currently never includes VLLMAliasCfg and both VLLMAliasCfg.ModelName and AliasConfig.ModelName share the same json tag, making the vLLM override unreachable; fix by adding a nested vLLM field to AliasConfig (e.g., add a pointer field like VLLM *VLLMAliasCfg `json:"vllm,omitempty"`), leaving AliasConfig.ModelName as the canonical top-level name, and keep VLLMAliasCfg.ModelName as-is so its JSON becomes vllm.model_name (no tag rename required), ensuring callers can express the vLLM-specific override separately from AliasConfig.ModelName.core/schemas/bifrost.go (1)
1103-1125:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the deprecated fields before re-deriving them.
syncDeprecatedFromRoutingInfoonly overwrites non-empty branches. If the currentRoutingInfois partial,provider,original_model_requested, andresolved_model_usedkeep their previous values, so a reused response/error can emit stale routing metadata. Zero these outputs first, then derive them frominfo.Suggested fix
func syncDeprecatedFromRoutingInfo(info RoutingInfo, provider *ModelProvider, originalModelRequested, resolvedModelUsed *string) { - if provider != nil && info.Provider != "" { - *provider = info.Provider + if provider != nil { + *provider = "" + if info.Provider != "" { + *provider = info.Provider + } } @@ if originalModelRequested != nil { + *originalModelRequested = "" if info.IsFallback && info.PrimaryModel != nil && *info.PrimaryModel != "" { *originalModelRequested = *info.PrimaryModel } else if info.Model != "" { @@ if resolvedModelUsed != nil { + *resolvedModelUsed = "" if info.ResolvedKeyAlias != nil && info.ResolvedKeyAlias.ModelID != "" { *resolvedModelUsed = info.ResolvedKeyAlias.ModelID } else if info.Model != "" {As per coding guidelines, pooled objects must have every field reset before reuse so prior-request data cannot leak forward.
📝 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 syncDeprecatedFromRoutingInfo(info RoutingInfo, provider *ModelProvider, originalModelRequested, resolvedModelUsed *string) { if provider != nil { *provider = "" if info.Provider != "" { *provider = info.Provider } } // OriginalModelRequested: collapses to the caller-sent model. On a fallback // attempt that's the primary's model (the user never asked for the fallback's); // otherwise it's this attempt's model. if originalModelRequested != nil { *originalModelRequested = "" if info.IsFallback && info.PrimaryModel != nil && *info.PrimaryModel != "" { *originalModelRequested = *info.PrimaryModel } else if info.Model != "" { *originalModelRequested = info.Model } } // ResolvedModelUsed: the wire model. Alias's ModelID when an alias matched, // otherwise the attempt's Model. if resolvedModelUsed != nil { *resolvedModelUsed = "" if info.ResolvedKeyAlias != nil && info.ResolvedKeyAlias.ModelID != "" { *resolvedModelUsed = info.ResolvedKeyAlias.ModelID } else if info.Model != "" { *resolvedModelUsed = info.Model } } }🤖 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 1103 - 1125, syncDeprecatedFromRoutingInfo currently only overwrites non-empty branches and can leave prior values in pooled outputs; first clear the outputs then re-derive them from info: at the top of syncDeprecatedFromRoutingInfo, if provider != nil set *provider to the zero value (empty), if originalModelRequested != nil set *originalModelRequested to "", and if resolvedModelUsed != nil set *resolved_model_used (resolvedModelUsed) to ""; then run the existing logic that sets provider, originalModelRequested, and resolvedModelUsed from info so partial RoutingInfo cannot leak prior-request data.Source: Coding guidelines
0f1ec91 to
dd0fa43
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 4871-4878: The fallback stream returned from tryStreamRequest is
not being annotated with fallback/primary routing metadata: wrap the returned
stream channel from tryStreamRequest (the variable result) with a forwarding
goroutine that reads each emitted chunk/error, calls
SetFallbackRoutingInfo(provider, model) on the chunk/error (and preserves
existing per-attempt BuildRoutingInfo data), then sends the annotated item to
the client channel; ensure the wrapper also closes the output channel when the
input channel is closed and propagates cancellation; make the same change where
BuildRoutingInfo per-attempt stamping occurs (the block around
BuildRoutingInfo(...) at lines 6206-6239) so any successful fallback streams are
similarly wrapped and annotated before being returned.
In `@core/schemas/account.go`:
- Around line 537-576: Add deterministic, table-driven tests that exercise
KeyAliases.UnmarshalJSON (and the AliasConfig promotion) covering at minimum:
legacy string value (e.g. {"k":"model"} -> AliasConfig{ModelID:"model"}), object
value (e.g. {"k":{"model_id":"model"}}), null input, invalid-type (e.g.
{"k":123} expecting error), and empty-value (e.g. {"k":""} or empty raw message
expecting error). Implement a test function using testing.T with a slice of
cases {name, inputJSON, want KeyAliases or wantErr boolean/substring} and
iterate with t.Run to assert successful unmarshalling yields the expected
KeyAliases and failure cases return errors containing the descriptive text
produced by KeyAliases.UnmarshalJSON (like "empty value" or "value must be a
string"). Ensure you construct expected AliasConfig values for comparisons and
use reflection or cmp to compare results.
In `@core/schemas/bifrost.go`:
- Around line 1103-1197: The problem is that early failures can have the
deprecated provider/original_model_requested/resolved_model_used set while
ef.RoutingInfo stays zero, making routing_info unreliable; fix by ensuring
RoutingInfo is populated when PopulateRoutingInfo runs: in
BifrostResponse.PopulateRoutingInfo and BifrostError.PopulateRoutingInfo (and
when serializing), if the incoming info is zero-valued but the deprecated
triplet (ef.Provider, ef.OriginalModelRequested, ef.ResolvedModelUsed) is set,
synthesize a RoutingInfo from those fields (populate
Provider/Model/ResolvedKeyAlias.ModelID or PrimaryModel/PrimaryProvider as
appropriate) before assigning and calling syncDeprecatedFromRoutingInfo, or
alternatively seed attempt-scoped RoutingInfo earlier in
PopulateExtraFields/request-worker seeding so PopulateRoutingInfo always
receives a non-empty RoutingInfo.
🪄 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: 831bfb8c-2c53-40ac-9363-b5e5021e5ac4
📒 Files selected for processing (4)
core/bifrost.gocore/schemas/account.gocore/schemas/bifrost.goframework/streaming/types.go
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: 3
🤖 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 4871-4878: The fallback stream returned from tryStreamRequest is
not being annotated with fallback/primary routing metadata: wrap the returned
stream channel from tryStreamRequest (the variable result) with a forwarding
goroutine that reads each emitted chunk/error, calls
SetFallbackRoutingInfo(provider, model) on the chunk/error (and preserves
existing per-attempt BuildRoutingInfo data), then sends the annotated item to
the client channel; ensure the wrapper also closes the output channel when the
input channel is closed and propagates cancellation; make the same change where
BuildRoutingInfo per-attempt stamping occurs (the block around
BuildRoutingInfo(...) at lines 6206-6239) so any successful fallback streams are
similarly wrapped and annotated before being returned.
In `@core/schemas/account.go`:
- Around line 537-576: Add deterministic, table-driven tests that exercise
KeyAliases.UnmarshalJSON (and the AliasConfig promotion) covering at minimum:
legacy string value (e.g. {"k":"model"} -> AliasConfig{ModelID:"model"}), object
value (e.g. {"k":{"model_id":"model"}}), null input, invalid-type (e.g.
{"k":123} expecting error), and empty-value (e.g. {"k":""} or empty raw message
expecting error). Implement a test function using testing.T with a slice of
cases {name, inputJSON, want KeyAliases or wantErr boolean/substring} and
iterate with t.Run to assert successful unmarshalling yields the expected
KeyAliases and failure cases return errors containing the descriptive text
produced by KeyAliases.UnmarshalJSON (like "empty value" or "value must be a
string"). Ensure you construct expected AliasConfig values for comparisons and
use reflection or cmp to compare results.
In `@core/schemas/bifrost.go`:
- Around line 1103-1197: The problem is that early failures can have the
deprecated provider/original_model_requested/resolved_model_used set while
ef.RoutingInfo stays zero, making routing_info unreliable; fix by ensuring
RoutingInfo is populated when PopulateRoutingInfo runs: in
BifrostResponse.PopulateRoutingInfo and BifrostError.PopulateRoutingInfo (and
when serializing), if the incoming info is zero-valued but the deprecated
triplet (ef.Provider, ef.OriginalModelRequested, ef.ResolvedModelUsed) is set,
synthesize a RoutingInfo from those fields (populate
Provider/Model/ResolvedKeyAlias.ModelID or PrimaryModel/PrimaryProvider as
appropriate) before assigning and calling syncDeprecatedFromRoutingInfo, or
alternatively seed attempt-scoped RoutingInfo earlier in
PopulateExtraFields/request-worker seeding so PopulateRoutingInfo always
receives a non-empty RoutingInfo.
🪄 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: 831bfb8c-2c53-40ac-9363-b5e5021e5ac4
📒 Files selected for processing (4)
core/bifrost.gocore/schemas/account.gocore/schemas/bifrost.goframework/streaming/types.go
🛑 Comments failed to post (3)
core/bifrost.go (1)
4871-4878:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSuccessful fallback streams still miss
is_fallback/primary_*routing metadata.Line 4878 returns the fallback stream untouched, while Lines 6206-6239 only stamp per-attempt data from
BuildRoutingInfo(...). That means a stream that succeeds on a fallback emits chunks withprovider/model/key, but never getsrouting_info.is_fallback,primary_provider, orprimary_model, so the streaming path does not meet this PR’s fallback observability contract. You need to wrap the returned stream or otherwise injectSetFallbackRoutingInfo(...)into each emitted chunk/error before it reaches the client.Also applies to: 6206-6239
🤖 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/bifrost.go` around lines 4871 - 4878, The fallback stream returned from tryStreamRequest is not being annotated with fallback/primary routing metadata: wrap the returned stream channel from tryStreamRequest (the variable result) with a forwarding goroutine that reads each emitted chunk/error, calls SetFallbackRoutingInfo(provider, model) on the chunk/error (and preserves existing per-attempt BuildRoutingInfo data), then sends the annotated item to the client channel; ensure the wrapper also closes the output channel when the input channel is closed and propagates cancellation; make the same change where BuildRoutingInfo per-attempt stamping occurs (the block around BuildRoutingInfo(...) at lines 6206-6239) so any successful fallback streams are similarly wrapped and annotated before being returned.core/schemas/account.go (1)
537-576: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add table-driven tests for the dual-shape alias parser.
This method is now the backward-compat gate for both legacy string aliases and the new object form. Please cover at least string/object/null/invalid-type/empty-value cases before merge.
As per coding guidelines, Go behavior changes should have deterministic, table-driven coverage.
🤖 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/account.go` around lines 537 - 576, Add deterministic, table-driven tests that exercise KeyAliases.UnmarshalJSON (and the AliasConfig promotion) covering at minimum: legacy string value (e.g. {"k":"model"} -> AliasConfig{ModelID:"model"}), object value (e.g. {"k":{"model_id":"model"}}), null input, invalid-type (e.g. {"k":123} expecting error), and empty-value (e.g. {"k":""} or empty raw message expecting error). Implement a test function using testing.T with a slice of cases {name, inputJSON, want KeyAliases or wantErr boolean/substring} and iterate with t.Run to assert successful unmarshalling yields the expected KeyAliases and failure cases return errors containing the descriptive text produced by KeyAliases.UnmarshalJSON (like "empty value" or "value must be a string"). Ensure you construct expected AliasConfig values for comparisons and use reflection or cmp to compare results.Source: Coding guidelines
core/schemas/bifrost.go (1)
1103-1197:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't publish an empty
routing_infoon early retry/fallback failures.These helpers only backfill the deprecated triplet from
RoutingInfo. On the current error path, core already callsPopulateExtraFields(...)beforePopulateRoutingInfo(attemptRoutingInfo), so an early failure can still haveprovider/original_model_requested/resolved_model_usedpopulated while the new canonicalrouting_infostays zeroed. That makesrouting_infounreliable for exactly the early-attempt failures this feature is trying to surface.Please land the request-worker seeding of attempt-scoped routing info alongside this schema change, or synthesize the missing
RoutingInfobefore serializing it.Based on the fallback/error call sites in
core/bifrost.go,PopulateRoutingInfo(attemptRoutingInfo)runs afterPopulateExtraFields(...), and the stack context already identifies a separate seed step for early failures.🤖 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 1103 - 1197, The problem is that early failures can have the deprecated provider/original_model_requested/resolved_model_used set while ef.RoutingInfo stays zero, making routing_info unreliable; fix by ensuring RoutingInfo is populated when PopulateRoutingInfo runs: in BifrostResponse.PopulateRoutingInfo and BifrostError.PopulateRoutingInfo (and when serializing), if the incoming info is zero-valued but the deprecated triplet (ef.Provider, ef.OriginalModelRequested, ef.ResolvedModelUsed) is set, synthesize a RoutingInfo from those fields (populate Provider/Model/ResolvedKeyAlias.ModelID or PrimaryModel/PrimaryProvider as appropriate) before assigning and calling syncDeprecatedFromRoutingInfo, or alternatively seed attempt-scoped RoutingInfo earlier in PopulateExtraFields/request-worker seeding so PopulateRoutingInfo always receives a non-empty RoutingInfo.
0645d6f to
e0ae82e
Compare
48605b5 to
d355ea9
Compare
d355ea9 to
bdb85c8
Compare
42a7143 to
4715dbd
Compare
Merge activity
|
3 participants
Summary
Introduces a structured
RoutingInfofield on bothBifrostResponseExtraFieldsandBifrostErrorExtraFieldsthat exposes per-attempt routing details (provider, model, key name, resolved key alias) alongside fallback signals (IsFallback,PrimaryProvider,PrimaryModel). Previously, callers had no way to determine which provider/model actually handled a request after fallback resolution, or whether a response came from a fallback path at all.Changes
RoutingInfoandResolvedKeyAliasschema types capturing the provider, model, key name, resolved alias metadata, fallback flag, and primary provider/model for fallback attempts.BuildRoutingInfohelper onBifrostContextthat constructs per-attemptRoutingInfofrom the chosen provider, model, key, and resolved alias stashed in context.PopulateRoutingInfomethods onBifrostResponseandBifrostErrorto stampRoutingInfoonto responses and errors at the same call sites asPopulateExtraFields.SetFallbackRoutingInfomethods onBifrostResponseandBifrostError, called by the orchestrator (handleRequest/handleStreamRequest) after a fallback attempt succeeds or fails, to layer onIsFallback,PrimaryProvider, andPrimaryModel. These signals belong to the orchestrator scope and are intentionally not set by per-attempt code.RoutingInfois snapshotted per-attempt into a local variable (perAttemptRoutingInfo) before the asyncpostHookRunnerclosure captures it, preventing a later retry's routing info from bleeding into an earlier attempt's chunks.RoutingInfofield toProcessedStreamResponseto carry routing context through the streaming pipeline.Provider,OriginalModelRequested, andResolvedModelUsedfields on both extra-fields structs in favor ofRoutingInfo, while keeping them populated for backward compatibility.Type of change
Affected areas
How to test
go version go test ./...Verify that responses and errors from both standard and streaming requests include a populated
routing_infoobject in their extra fields. For fallback scenarios, confirmis_fallbackistrueandprimary_provider/primary_modelreflect the originally requested provider and model.Screenshots/Recordings
N/A
Breaking changes
The previously flat
Provider,OriginalModelRequested, andResolvedModelUsedfields remain populated. The newrouting_infoobject is additive.Related issues
N/A
Security considerations
No auth, secrets, PII, or sandboxing implications.
RoutingInfosurfaces key names (not key values) already present in other extra fields.Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit