fix: add responses to chat fallback for custom openai providers#2599
Open
Vaibhav701161 wants to merge 1 commit intomaximhq:mainfrom
Open
fix: add responses to chat fallback for custom openai providers#2599Vaibhav701161 wants to merge 1 commit intomaximhq:mainfrom
Vaibhav701161 wants to merge 1 commit intomaximhq:mainfrom
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Responses→Chat-Completions compatibility: new provider config flag, context-backed compat state and retry policy, Pre/Post-LLM hooks to transform requests/responses/errors, runtime-detected fallback with re-execution, streaming safety guards, and tests + schema updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Bifrost as Bifrost
participant Compat as CompatPlugin
participant Provider as Provider
participant ResponsesAPI as ResponsesAPI
participant ChatAPI as ChatAPI
Client->>Bifrost: Send Responses request
Bifrost->>Compat: PreLLMHook (maybe transform / set compat state)
Compat-->>Bifrost: (original or Chat fallback request)
Bifrost->>Provider: Handle(request)
alt Native Responses attempt
Provider->>ResponsesAPI: POST /v1/responses
ResponsesAPI-->>Provider: 200 OK
Provider-->>Bifrost: native Responses result
Bifrost->>Compat: PostLLMHook (convert back if needed)
Compat-->>Bifrost: final Responses-shaped response + metadata
Bifrost-->>Client: Response
else Responses unsupported / configured fallback
Provider->>ResponsesAPI: POST /v1/responses
ResponsesAPI-->>Provider: Error (e.g., 404)
Provider-->>Bifrost: error
Bifrost->>Bifrost: activate compat state / retry policy
Bifrost->>Provider: Handle(ToChatFallbackRequest)
Provider->>ChatAPI: POST /v1/chat/completions (or SSE)
ChatAPI-->>Provider: 200 OK / SSE stream
Provider-->>Bifrost: chat response/stream
Bifrost->>Compat: PostLLMHook (transform chat -> responses)
Compat-->>Bifrost: converted Responses-shaped output + fallback metadata
Bifrost-->>Client: Response (marked as fallback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Confidence Score: 3/5Not safe to merge — multiple open P0/P1 issues from previous review rounds remain unaddressed, including nil-pointer panics for keyless providers, dropped plugin logs, and streaming chunks served in the wrong format. Multiple P0/P1 issues from previous review threads remain open: nil dereference on keyless providers in requestWorker, drainAndAttachPluginLogs silently dropped on the non-streaming path, streaming fallback emitting chat-format chunks on a responses-stream endpoint, Provider/Latency/ResolvedModelUsed dropped on fallback responses, and PopulateExtraFields removed from the WebSocket streaming path. The JSON schema also rejects null for the tri-state field, breaking the documented auto-detect mode. No evidence these have been addressed in the current HEAD. core/bifrost.go (nil-dereference panic, dropped plugin logs, WebSocket ExtraFields), plugins/compat/responsestochat.go (streaming no-op, missing ExtraFields copy), transports/config.schema.json (schema type) Important Files Changed
Reviews (18): Last reviewed commit: "fix: make responses→chat fallback produc..." | 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
🧹 Nitpick comments (2)
transports/config.schema.json (1)
1995-1998: Scopesupports_responses_apito OpenAI-based custom providers.Line 1995 currently validates this flag for any
base_provider_type, which allows silent no-op configs on non-OpenAI custom providers. Consider constraining it with anif/elserule tied tobase_provider_type: "openai".♻️ Proposed schema guard
"custom_provider_config": { "type": "object", "description": "Custom provider configuration for extending or customizing provider behavior", "properties": { @@ "supports_responses_api": { "type": "boolean", "description": "Controls native OpenAI Responses API usage for custom OpenAI-compatible providers. true forces /v1/responses, false forces an internal Chat Completions fallback, and omitting it keeps native behavior first with an automatic downgrade on obvious unsupported-endpoint failures." }, @@ }, + "allOf": [ + { + "if": { + "properties": { + "base_provider_type": { "const": "openai" } + } + }, + "else": { + "not": { + "required": ["supports_responses_api"] + } + } + } + ], "required": ["base_provider_type"], "additionalProperties": false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/config.schema.json` around lines 1995 - 1998, The schema currently allows supports_responses_api for any base_provider_type causing silent no-ops on non-OpenAI providers; add an if/then/else guard keyed on base_provider_type == "openai" so supports_responses_api is only permitted when base_provider_type is "openai". Implement an "if": {"properties": {"base_provider_type": {"const": "openai"}}} with a "then" that defines/permits the supports_responses_api boolean property (keeping the existing description/type), and an "else" that forbids that property (e.g., via "not": {"required": ["supports_responses_api"]} or by disallowing it) to ensure validation fails if non-OpenAI providers include that flag; update the schema block containing supports_responses_api accordingly.core/providers/openai/responses_fallback_test.go (1)
153-196: Add non-stream fallback context-marker assertions.You already assert the fallback marker in streaming; adding the same check in both non-stream fallback tests would protect observability behavior from regressions.
Suggested test hardening
- response, bifrostErr := provider.Responses(testOpenAIResponsesCtx(), schemas.Key{}, testOpenAIResponsesRequest()) + ctx := testOpenAIResponsesCtx() + response, bifrostErr := provider.Responses(ctx, schemas.Key{}, testOpenAIResponsesRequest()) if bifrostErr != nil { t.Fatalf("Responses returned error: %v", bifrostErr.Error) } + if ctx.Value(schemas.BifrostContextKeyIsResponsesToChatCompletionFallback) != true { + t.Fatalf("expected fallback context marker to be set, got %v", ctx.Value(schemas.BifrostContextKeyIsResponsesToChatCompletionFallback)) + } - response, bifrostErr := provider.Responses(testOpenAIResponsesCtx(), schemas.Key{}, testOpenAIResponsesRequest()) + ctx = testOpenAIResponsesCtx() + response, bifrostErr = provider.Responses(ctx, schemas.Key{}, testOpenAIResponsesRequest()) if bifrostErr != nil { t.Fatalf("Responses returned error: %v", bifrostErr.Error) } + if ctx.Value(schemas.BifrostContextKeyIsResponsesToChatCompletionFallback) != true { + t.Fatalf("expected fallback context marker to be set, got %v", ctx.Value(schemas.BifrostContextKeyIsResponsesToChatCompletionFallback)) + }Also applies to: 235-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responses_fallback_test.go` around lines 153 - 196, The non-stream fallback tests should also assert the presence of the fallback context-marker: in the non-stream tests (the one that calls provider.Responses and checks chatHits/responsesHits and inspects response), add an assertion that response.ExtraFields.ContextMarkers contains the fallback marker (same marker asserted in the streaming fallback test) to protect observability from regressions; locate the assertions around the response variable in the non-stream fallback test and insert a check that the fallback context-marker is present in response.ExtraFields.ContextMarkers (or the equivalent context marker field used elsewhere in the tests).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/openai/openai.go`:
- Around line 139-167: In responsesToChatCompletion, set the same fallback
marker used for streaming so non-stream fallbacks are observable: before
invoking HandleOpenAIChatCompletionRequest, set the context key
schemas.BifrostContextKeyIsResponsesToChatCompletionFallback on the provided ctx
(matching the streaming path's behavior) so downstream logic and telemetry can
detect the fallback; ensure you only set the marker in the non-stream fallback
path and preserve existing ctx semantics.
In `@core/providers/openai/responses_fallback_test.go`:
- Around line 124-128: The httptest server handler uses t.Fatalf/t.Fatal inside
the goroutine (in the anonymous handler created in server :=
httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request)
{ ... }))), which only aborts the handler goroutine; replace those calls with
t.Errorf(...) and then write an HTTP error status (e.g.,
w.WriteHeader(http.StatusInternalServerError)) followed by return so the test
sees the failure and the handler exits cleanly; apply this change for every
occurrence of t.Fatal/t.Fatalf in the handlers in responses_fallback_test.go
(the anonymous handler blocks used to read r.Body and other request checks).
---
Nitpick comments:
In `@core/providers/openai/responses_fallback_test.go`:
- Around line 153-196: The non-stream fallback tests should also assert the
presence of the fallback context-marker: in the non-stream tests (the one that
calls provider.Responses and checks chatHits/responsesHits and inspects
response), add an assertion that response.ExtraFields.ContextMarkers contains
the fallback marker (same marker asserted in the streaming fallback test) to
protect observability from regressions; locate the assertions around the
response variable in the non-stream fallback test and insert a check that the
fallback context-marker is present in response.ExtraFields.ContextMarkers (or
the equivalent context marker field used elsewhere in the tests).
In `@transports/config.schema.json`:
- Around line 1995-1998: The schema currently allows supports_responses_api for
any base_provider_type causing silent no-ops on non-OpenAI providers; add an
if/then/else guard keyed on base_provider_type == "openai" so
supports_responses_api is only permitted when base_provider_type is "openai".
Implement an "if": {"properties": {"base_provider_type": {"const": "openai"}}}
with a "then" that defines/permits the supports_responses_api boolean property
(keeping the existing description/type), and an "else" that forbids that
property (e.g., via "not": {"required": ["supports_responses_api"]} or by
disallowing it) to ensure validation fails if non-OpenAI providers include that
flag; update the schema block containing supports_responses_api accordingly.
🪄 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: 40ab339d-a3fd-4747-b9bf-f701e053c829
📒 Files selected for processing (4)
core/providers/openai/openai.gocore/providers/openai/responses_fallback_test.gocore/schemas/provider.gotransports/config.schema.json
|
|
Vaibhav701161
force-pushed
the
fix/responses-chat-fallback
branch
from
92adae7 to
2891adb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/providers/utils/utils.go (1)
1841-1846: Nil-guard is currently non-functional with current chunk builder.On Line 1841, the guard won’t filter anything because
BuildClientStreamChunkalways returns a non-nil empty chunk. If the goal is “send only meaningful chunks,” make the builder returnnilwhen bothprocessedResponseandprocessedErrorare nil.Suggested adjustment
func BuildClientStreamChunk(ctx context.Context, processedResponse *schemas.BifrostResponse, processedError *schemas.BifrostError) *schemas.BifrostStreamChunk { + if processedResponse == nil && processedError == nil { + return nil + } dropRaw, _ := ctx.Value(schemas.BifrostContextKeyRawRequestResponseForLogging).(bool) streamResponse := &schemas.BifrostStreamChunk{} ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/utils/utils.go` around lines 1841 - 1846, The nil-guard in the sender loop is ineffective because BuildClientStreamChunk always returns a non-nil empty chunk; change BuildClientStreamChunk so it returns nil when both processedResponse and processedError are nil (i.e., no meaningful data) so the existing check if streamResponse != nil will actually filter out empty chunks; update any callers that assume a non-nil return to handle nil accordingly (notably the code sending to responseChan using streamResponse and ctx).core/schemas/bifrost.go (1)
838-854: Potential silent data loss when response has no valid content.
response.GetExtraFields()(line 845) returns&BifrostResponseExtraFields{}(a temporary) if no response type field is set (see line 940). In this edge case, the fallback metadata written toextraFieldson lines 846-847 would be silently discarded.While callers should ensure
responsecontains valid content, the current code path doesn't guard against this. Consider returning early or logging a warning ifGetExtraFields()returns a non-addressable temporary.🔧 Optional defensive guard
func ApplyResponsesToChatCompletionFallbackMetadata(ctx context.Context, response *BifrostResponse, bifrostErr *BifrostError) { reason, ok := GetResponsesToChatCompletionFallback(ctx) if !ok { return } if response != nil { + // GetExtraFields returns a temporary if no response type is set; + // only apply metadata if there's an actual response to annotate. + if response.ListModelsResponse == nil && response.TextCompletionResponse == nil && + response.ChatResponse == nil && response.ResponsesResponse == nil && + response.ResponsesStreamResponse == nil && response.PassthroughResponse == nil { + // Consider logging this edge case for observability + } extraFields := response.GetExtraFields() extraFields.ResponsesToChatCompletionFallback = true extraFields.ResponsesToChatCompletionFallbackReason = string(reason) }Alternatively,
GetExtraFields()could return(*BifrostResponseExtraFields, bool)to signal whether it found a valid response type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/bifrost.go` around lines 838 - 854, The code may write fallback metadata into a transient extraFields value returned by GetExtraFields(), causing silent data loss; update GetExtraFields to return (*BifrostResponseExtraFields, bool) (or add a ResponseHasValidType()/IsAddressable check) and change ApplyResponsesToChatCompletionFallbackMetadata to call the new signature, early-return (or log) if the second bool is false, and only mutate and assign the extra fields when the returned bool indicates a real, addressable extra-fields object (references: ApplyResponsesToChatCompletionFallbackMetadata, GetExtraFields, BifrostResponseExtraFields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 3919-3931: The provider metadata
(BifrostContextKeyCustomProviderMetadata / SupportsResponsesAPI) is set before
running RunLLMPreHooks but prehooks can call
BifrostRequest.UpdateProvider(provider) and change routing; refresh the metadata
after prehooks complete by re-calling setProviderContextMetadataForKey(ctx,
provider) using the (possibly updated) provider from req.GetRequestFields() or
req.Provider, then clear schemas.ClearResponsesToChatCompletionFallback and
schemas.ClearResponsesToChatCompletionCompatState; apply the same post-prehook
metadata refresh pattern around the other referenced blocks that run
RunLLMPreHooks (the regions near getProviderQueue usage and the other two
spots).
---
Nitpick comments:
In `@core/providers/utils/utils.go`:
- Around line 1841-1846: The nil-guard in the sender loop is ineffective because
BuildClientStreamChunk always returns a non-nil empty chunk; change
BuildClientStreamChunk so it returns nil when both processedResponse and
processedError are nil (i.e., no meaningful data) so the existing check if
streamResponse != nil will actually filter out empty chunks; update any callers
that assume a non-nil return to handle nil accordingly (notably the code sending
to responseChan using streamResponse and ctx).
In `@core/schemas/bifrost.go`:
- Around line 838-854: The code may write fallback metadata into a transient
extraFields value returned by GetExtraFields(), causing silent data loss; update
GetExtraFields to return (*BifrostResponseExtraFields, bool) (or add a
ResponseHasValidType()/IsAddressable check) and change
ApplyResponsesToChatCompletionFallbackMetadata to call the new signature,
early-return (or log) if the second bool is false, and only mutate and assign
the extra fields when the returned bool indicates a real, addressable
extra-fields object (references: ApplyResponsesToChatCompletionFallbackMetadata,
GetExtraFields, BifrostResponseExtraFields).
🪄 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: fcdd19c9-2020-4364-9a31-5dbeb77afc2b
📒 Files selected for processing (15)
core/bifrost.gocore/custom_provider_context_test.gocore/providers/openai/openai.gocore/providers/openai/responses_fallback_test.gocore/providers/utils/utils.gocore/schemas/bifrost.gocore/schemas/mux.gocore/schemas/mux_test.gocore/schemas/provider.goplugins/litellmcompat/main.goplugins/litellmcompat/responsestochat.goplugins/litellmcompat/responsestochat_test.goplugins/litellmcompat/runtime_fallback_integration_test.gotransports/bifrost-http/server/plugins.gotransports/config.schema.json
✅ Files skipped from review due to trivial changes (4)
- plugins/litellmcompat/main.go
- plugins/litellmcompat/responsestochat_test.go
- plugins/litellmcompat/responsestochat.go
- core/schemas/mux.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/schemas/mux_test.go
- core/providers/openai/responses_fallback_test.go
- plugins/litellmcompat/runtime_fallback_integration_test.go
- core/schemas/provider.go
Vaibhav701161
added a commit
to Vaibhav701161/bifrost
that referenced
this pull request
Apr 16, 2026
Merge upstream/v1.5.0 into fix/responses-chat-fallback branch. Conflicts resolved: - core/schemas/bifrost.go: Added context keys, type definitions (ResponsesToChatCompletionFallbackReason, CustomProviderContextMetadata, ResponsesToChatCompletionRetryPolicy, ResponsesToChatCompletionCompatState), helper functions, and ExtraFields for fallback metadata on v1.5.0 base. - core/schemas/provider.go: Kept v1.5.0 base, added ProviderPricingOverride type and PricingOverrides field to ProviderConfig. - core/bifrost.go (12 regions): Kept v1.5.0's expanded RunPostLLMHooks + drainAndAttachPluginLogs + trace flush + PopulateExtraFields pattern, added ApplyResponsesToChatCompletionFallbackMetadata calls. Updated retry functions to use keyProvider pattern instead of direct key/keys params. - core/providers/openai/openai.go: Removed LiteLLMCompat field (renamed to compat in v1.5.0). - transports/bifrost-http/server/plugins.go: Took v1.5.0 (compat replaces litellmcompat). - transports/config.schema.json: Kept v1.5.0 base with supports_responses_api field already present. - plugins/litellmcompat/*.go: Removed (deleted in v1.5.0, replaced by compat).
Vaibhav701161
force-pushed
the
fix/responses-chat-fallback
branch
from
1a42512 to
dadf28b
Compare
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)
core/bifrost.go (1)
4042-4048:⚠️ Potential issue | 🟠 MajorAdd a nil request guard in
RunStreamPreHooks.Line 4047 dereferences
reqbefore validating it. A nilreqwill panic.✅ Proposed fix
func (bifrost *Bifrost) RunStreamPreHooks(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*WSStreamHooks, *schemas.BifrostError) { + if req == nil { + bifrostErr := newBifrostErrorFromMsg("stream prehook request is nil") + bifrostErr.ExtraFields.RequestType = schemas.RealtimeRequest + return nil, bifrostErr + } if ctx == nil { ctx = bifrost.ctx }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4042 - 4048, The function RunStreamPreHooks dereferences req with req.GetRequestFields() without guarding for a nil request; add an early nil-check at the top of RunStreamPreHooks (before calling req.GetRequestFields or setProviderContextMetadataForKey) that returns a nil *WSStreamHooks and an appropriate *schemas.BifrostError indicating a missing request (use the same error creation pattern used elsewhere in this file), so callers get a clear error instead of a panic; reference RunStreamPreHooks, req, GetRequestFields and setProviderContextMetadataForKey to locate where to insert the guard.
♻️ Duplicate comments (1)
core/bifrost.go (1)
4047-4059:⚠️ Potential issue | 🟠 MajorRefresh provider metadata after prehook routing rewrites.
Metadata is set before
RunLLMPreHooks, but prehooks can update provider routing. The no-op call at Line 4121 (preReq.GetRequestFields()) does not refresh context metadata, so compat/fallback decisions can run with stale provider metadata.💡 Suggested fix pattern
preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) +if preReq != nil { + updatedProvider, _, _ := preReq.GetRequestFields() + if updatedProvider != provider { + if err := bifrost.setProviderContextMetadataForKey(ctx, updatedProvider); err != nil { + bifrostErr := newBifrostError(err) + bifrostErr.ExtraFields = schemas.BifrostErrorExtraFields{ + RequestType: req.RequestType, + Provider: updatedProvider, + OriginalModelRequested: model, + } + return nil, bifrostErr + } + schemas.ClearResponsesToChatCompletionFallback(ctx) + schemas.ClearResponsesToChatCompletionCompatState(ctx) + } +}Based on learnings: provider switching from a PreLLMHook plugin is performed by calling
BifrostRequest.UpdateProvider(provider), andgetProviderQueueuses the updated provider as the single source of truth.Also applies to: 4121-4123, 4596-4607, 4789-4800
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4047 - 4059, Provider metadata is being set before RunLLMPreHooks but prehooks can call BifrostRequest.UpdateProvider(...) and change routing, so refresh the context metadata after running prehooks: after RunLLMPreHooks (and after any preReq.GetRequestFields() no-op usage) call bifrost.setProviderContextMetadataForKey(ctx, provider) again using the provider returned by req.GetRequestFields() (or preReq.GetRequestFields()) so getProviderQueue, schemas.ClearResponsesToChatCompletionFallback, and schemas.ClearResponsesToChatCompletionCompatState see the updated provider; apply the same refresh pattern around the other affected spots (the blocks around where RunLLMPreHooks is invoked).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 5557-5569: The code calls keyProvider(nil) without checking for
nil which panics in keyless flows; before invoking keyProvider(nil) in the
IsStreamRequestType branch and the else branch, guard that keyProvider != nil
and only call it when non-nil, otherwise set retryKey to the zero value used by
retryResponsesToChatCompletionStream/retryResponsesToChatCompletionRequest
(e.g., empty string or nil as expected); update the two sites around
IsStreamRequestType, retryResponsesToChatCompletionStream, and
retryResponsesToChatCompletionRequest to use the guarded retryKey.
---
Outside diff comments:
In `@core/bifrost.go`:
- Around line 4042-4048: The function RunStreamPreHooks dereferences req with
req.GetRequestFields() without guarding for a nil request; add an early
nil-check at the top of RunStreamPreHooks (before calling req.GetRequestFields
or setProviderContextMetadataForKey) that returns a nil *WSStreamHooks and an
appropriate *schemas.BifrostError indicating a missing request (use the same
error creation pattern used elsewhere in this file), so callers get a clear
error instead of a panic; reference RunStreamPreHooks, req, GetRequestFields and
setProviderContextMetadataForKey to locate where to insert the guard.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 4047-4059: Provider metadata is being set before RunLLMPreHooks
but prehooks can call BifrostRequest.UpdateProvider(...) and change routing, so
refresh the context metadata after running prehooks: after RunLLMPreHooks (and
after any preReq.GetRequestFields() no-op usage) call
bifrost.setProviderContextMetadataForKey(ctx, provider) again using the provider
returned by req.GetRequestFields() (or preReq.GetRequestFields()) so
getProviderQueue, schemas.ClearResponsesToChatCompletionFallback, and
schemas.ClearResponsesToChatCompletionCompatState see the updated provider;
apply the same refresh pattern around the other affected spots (the blocks
around where RunLLMPreHooks is invoked).
🪄 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: 5c0843b1-4d27-4fe9-b31c-dca6086ab777
📒 Files selected for processing (9)
core/bifrost.gocore/custom_provider_context_test.gocore/providers/openai/openai.gocore/providers/openai/responses_fallback_test.gocore/providers/utils/utils.gocore/schemas/bifrost.gocore/schemas/mux.gocore/schemas/mux_test.gocore/schemas/provider.go
🚧 Files skipped from review as they are similar to previous changes (5)
- core/providers/utils/utils.go
- core/schemas/provider.go
- core/providers/openai/openai.go
- core/providers/openai/responses_fallback_test.go
- core/schemas/mux.go
akshaydeo
requested changes
Apr 19, 2026
Vaibhav701161
force-pushed
the
fix/responses-chat-fallback
branch
from
dadf28b to
1be887f
Compare
Vaibhav701161
force-pushed
the
fix/responses-chat-fallback
branch
from
1be887f to
70848ad
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/providers/openai/responses_fallback_test.go (1)
217-221:⚠️ Potential issue | 🟡 MinorAvoid
t.Fatalfinsidehttptesthandler goroutines.These handlers run outside the test goroutine; use
t.Errorfplus an HTTP error response andreturnso the client path fails deterministically.Proposed fix pattern
bodyBytes, err := io.ReadAll(r.Body) if err != nil { - t.Fatalf("failed to read request body: %v", err) + t.Errorf("failed to read request body: %v", err) + http.Error(w, "failed to read request body", http.StatusInternalServerError) + return }Verify no handler closures still call
Fatal/Fatalf:#!/bin/bash # Expect: no t.Fatal/t.Fatalf calls inside httptest handler setup after the fix. rg -n -C4 'httptest\.NewServer|t\.Fatalf?\(' core/providers/openai/responses_fallback_test.goAlso applies to: 336-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responses_fallback_test.go` around lines 217 - 221, The httptest server handler closure uses t.Fatalf (e.g., after io.ReadAll) which must not be called from handler goroutines; change those calls inside the handler in responses_fallback_test.go to use t.Errorf with a clear message, send an HTTP error response (e.g., http.Error(w, ... , http.StatusInternalServerError)) and return so the client path fails deterministically; apply the same pattern to other handler closures noted (the other occurrence around lines 336-340) to eliminate any t.Fatal/t.Fatalf in httptest handlers.core/bifrost.go (1)
4099-4110:⚠️ Potential issue | 🟠 MajorRefresh custom-provider metadata after pre-hook routing changes.
These blocks set
BifrostContextKeyCustomProviderMetadata/SupportsResponsesAPIbeforeRunLLMPreHooks, but pre-hooks can rewrite the provider. Re-readpreReq.GetRequestFields()after pre-hooks and refresh metadata when the provider changes; otherwise Responses→Chat fallback decisions can use stale provider config.Suggested fix pattern
preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) +if preReq != nil { + updatedProvider, updatedModel, _ := preReq.GetRequestFields() + if updatedProvider != provider { + if err := bifrost.setProviderContextMetadataForKey(ctx, updatedProvider); err != nil { + bifrostErr := newBifrostError(err) + bifrostErr.ExtraFields = schemas.BifrostErrorExtraFields{ + RequestType: req.RequestType, + Provider: updatedProvider, + OriginalModelRequested: updatedModel, + } + return nil, bifrostErr + } + } +}Based on learnings: provider switching from a PreLLMHook plugin is performed by calling
BifrostRequest.UpdateProvider(provider), and routing reads the updated provider via request fields.Also applies to: 4648-4658, 4865-4875
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4099 - 4110, Pre-hooks can change the chosen provider, so after running RunLLMPreHooks you must re-read the request fields and refresh provider-related context metadata; specifically, call req.GetRequestFields() (or preReq.GetRequestFields()) again after RunLLMPreHooks and if the provider differs call bifrost.setProviderContextMetadataForKey(ctx, newProvider) and update SupportsResponsesAPI metadata the same way before invoking schemas.ClearResponsesToChatCompletionFallback/CompatState; also ensure code paths that call BifrostRequest.UpdateProvider(provider) (pre-hook plugins) will be reflected by this re-read and metadata refresh (apply same change at the other occurrences referenced around the 4648-4658 and 4865-4875 locations).
🧹 Nitpick comments (2)
core/providers/openai/responses_fallback_test.go (2)
271-273: Don’t assert core-owned provider attribution in a direct provider test.
ExtraFields.Provideris populated by the core response path, so this directprovider.Responses(...)unit test should either drop this assertion or route through the core layer if attribution is what you want to verify.Based on learnings, "
Provider,OriginalModelRequested, andResolvedModelUsedonBifrostResponseExtraFieldsare populated centrally by(*BifrostResponse).PopulateExtraFieldsincore/bifrost.gofor all response paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responses_fallback_test.go` around lines 271 - 273, The test is asserting core-populated attribution (ExtraFields.Provider) inside a direct provider unit test (provider.Responses), which is wrong; remove the assertion about ExtraFields.Provider (and similarly OriginalModelRequested/ResolvedModelUsed) from this test, or instead test attribution through the core path by invoking the core response flow that calls BifrostResponse.PopulateExtraFields (see BifrostResponse and BifrostResponseExtraFields in core/bifrost.go) if you intend to verify provider attribution end-to-end.
1-1: Rename this file to match the Go filename convention.
responses_fallback_test.gohas an extra underscore before the_test.gosuffix; preferresponsesfallback_test.go.As per coding guidelines, "No underscores in Go filenames except for _test.go suffix; concatenate words in lowercase for multi-word filenames."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responses_fallback_test.go` at line 1, The test filename violates Go naming rules by including an extra underscore; rename the file from responses_fallback_test.go to responsesfallback_test.go so it uses lowercase concatenated words with only the _test.go suffix, and ensure any CI scripts or references that mentioned the old filename are updated accordingly (the package name openai and test functions need no code changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 5713-5720: The fallback branch is missing per-key alias resolution
and model setting, causing stale ResolvedModelUsed; update the fallback lambda
passed into executeRequestWithRetries (the non-stream path that calls
bifrost.handleProviderRequest) to, for the selected key k, call
k.Aliases.Resolve(...) and then call SetModel(...) exactly like the primary path
does before invoking handleProviderRequest so the resolved aliases and model are
correct for the fallback attempt (ensure the same logic used by the stream
branch / primary path is reused).
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 4099-4110: Pre-hooks can change the chosen provider, so after
running RunLLMPreHooks you must re-read the request fields and refresh
provider-related context metadata; specifically, call req.GetRequestFields() (or
preReq.GetRequestFields()) again after RunLLMPreHooks and if the provider
differs call bifrost.setProviderContextMetadataForKey(ctx, newProvider) and
update SupportsResponsesAPI metadata the same way before invoking
schemas.ClearResponsesToChatCompletionFallback/CompatState; also ensure code
paths that call BifrostRequest.UpdateProvider(provider) (pre-hook plugins) will
be reflected by this re-read and metadata refresh (apply same change at the
other occurrences referenced around the 4648-4658 and 4865-4875 locations).
In `@core/providers/openai/responses_fallback_test.go`:
- Around line 217-221: The httptest server handler closure uses t.Fatalf (e.g.,
after io.ReadAll) which must not be called from handler goroutines; change those
calls inside the handler in responses_fallback_test.go to use t.Errorf with a
clear message, send an HTTP error response (e.g., http.Error(w, ... ,
http.StatusInternalServerError)) and return so the client path fails
deterministically; apply the same pattern to other handler closures noted (the
other occurrence around lines 336-340) to eliminate any t.Fatal/t.Fatalf in
httptest handlers.
---
Nitpick comments:
In `@core/providers/openai/responses_fallback_test.go`:
- Around line 271-273: The test is asserting core-populated attribution
(ExtraFields.Provider) inside a direct provider unit test (provider.Responses),
which is wrong; remove the assertion about ExtraFields.Provider (and similarly
OriginalModelRequested/ResolvedModelUsed) from this test, or instead test
attribution through the core path by invoking the core response flow that calls
BifrostResponse.PopulateExtraFields (see BifrostResponse and
BifrostResponseExtraFields in core/bifrost.go) if you intend to verify provider
attribution end-to-end.
- Line 1: The test filename violates Go naming rules by including an extra
underscore; rename the file from responses_fallback_test.go to
responsesfallback_test.go so it uses lowercase concatenated words with only the
_test.go suffix, and ensure any CI scripts or references that mentioned the old
filename are updated accordingly (the package name openai and test functions
need no code changes).
🪄 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: 5ed3f551-ef05-4fbf-935e-0631f098be10
📒 Files selected for processing (9)
core/bifrost.gocore/custom_provider_context_test.gocore/providers/openai/openai.gocore/providers/openai/responses_fallback_test.gocore/providers/utils/utils.gocore/schemas/bifrost.gocore/schemas/mux.gocore/schemas/mux_test.gocore/schemas/provider.go
✅ Files skipped from review due to trivial changes (2)
- core/schemas/mux_test.go
- core/custom_provider_context_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core/providers/utils/utils.go
- core/schemas/mux.go
Vaibhav701161
force-pushed
the
fix/responses-chat-fallback
branch
from
70848ad to
9a7d5e9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/compat/main.go (1)
22-24:⚠️ Potential issue | 🟠 MajorResponses→Chat fallback won't execute if only that feature is enabled.
The handler logic at
transports/bifrost-http/handlers/config.gogates plugin loading on the same 4 compat knobs checked inConfig.IsEnabled(), which does not include the newResponsesToChatFallback. When only that option is enabled, the compat plugin is removed from the chain, preventing the PreLLMHook at lines 120-123 from running.Either add
ResponsesToChatFallbackto theConfigstruct and include it inIsEnabled():Suggested fix
type Config struct { ConvertTextToChat bool `json:"convert_text_to_chat"` ConvertChatToResponses bool `json:"convert_chat_to_responses"` ShouldDropParams bool `json:"should_drop_params"` ShouldConvertParams bool `json:"should_convert_params"` + ResponsesToChatFallback bool `json:"responses_to_chat_fallback"` } func (c Config) IsEnabled() bool { - return c.ConvertTextToChat || c.ConvertChatToResponses || c.ShouldDropParams || c.ShouldConvertParams + return c.ConvertTextToChat || c.ConvertChatToResponses || c.ShouldDropParams || c.ShouldConvertParams || c.ResponsesToChatFallback }Or move this fallback orchestration out of the optional compat plugin if it should always be provider-config driven.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/main.go` around lines 22 - 24, The Compat Config IsEnabled method omits the new ResponsesToChatFallback flag so the compat plugin is stripped when only that feature is set; update the Config type to add a ResponsesToChatFallback boolean and include it in the Config.IsEnabled() returned expression (or alternatively move the Responses→Chat fallback orchestration out of the optional compat plugin into the always-registered code path so PreLLMHook still runs), then ensure any code referencing the old Config (e.g., places that construct or marshal Config) is updated to handle the new field.
♻️ Duplicate comments (1)
core/providers/openai/responses_fallback_test.go (1)
217-221:⚠️ Potential issue | 🟡 MinorReplace
t.Fatalfinsidehttptesthandlers.These handlers run in server goroutines;
Fatalfexits that goroutine, not the parent test. Uset.Errorf, return an HTTP error, and exit the handler.Fix pattern
bodyBytes, err := io.ReadAll(r.Body) if err != nil { - t.Fatalf("failed to read request body: %v", err) + t.Errorf("failed to read request body: %v", err) + w.WriteHeader(http.StatusInternalServerError) + return }Also applies to: 335-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responses_fallback_test.go` around lines 217 - 221, In the httptest.NewServer handler used in responses_fallback_test.go (the inline http.HandlerFunc reading request bodies), replace any t.Fatalf calls with t.Errorf, then send an appropriate HTTP error response (e.g., http.Error(w, "read body failed", http.StatusInternalServerError)) and return from the handler so the server goroutine doesn't just exit; apply the same change to the second occurrence noted (the other handler around the 335-339 region).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/bifrost.go`:
- Around line 868-880: ActivateResponsesToChatCompletionCompatState sets
ctx.SetValue(BifrostContextKeySkipOperationCheck, true) but
ClearResponsesToChatCompletionFallback does not clear this flag; update
ClearResponsesToChatCompletionFallback (and any related teardown like
GetResponsesToChatCompletionCompatState/SetResponsesToChatCompletionFallback if
used) to call ctx.SetValue(BifrostContextKeySkipOperationCheck, false) or remove
the key when clearing the fallback state so retries do not unintentionally
bypass AllowedRequests checks.
In `@core/schemas/mux.go`:
- Around line 1192-1201: The function
ResponsesToChatCompletionFallbackErrorMessage currently returns
err.Error.Message even when it is an empty string; update the guard to treat
empty messages as missing by checking err != nil && err.Error != nil &&
err.Error.Message != "" and only then return err.Error.Message, otherwise return
"unknown responses API error"; reference the BifrostError type and its Error
field to locate the check and adjust the conditional logic inside
ResponsesToChatCompletionFallbackErrorMessage.
- Around line 1134-1154: The DefaultResponsesToChatCompletionRetryPolicy
currently only flags HTTP status codes and HTML responses; update
DefaultResponsesToChatCompletionRetryPolicy to also mark JSON unmarshal failures
and empty response bodies as unsupported-response signals so the fallback
contract is honored—modify the ResponsesToChatCompletionRetryPolicy returned by
DefaultResponsesToChatCompletionRetryPolicy to include indicators for unmarshal
errors (e.g., a boolean or error-substring like "invalid character" / common
JSON parse messages) and an explicit flag or substring to detect empty bodies,
ensuring the runtime will treat these cases as unsupported and trigger the
Responses->ChatCompletion fallback.
In `@transports/config.schema.json`:
- Around line 1783-1941: The JSON schema includes duplicate $defs for
pricing_override_match_type, pricing_override_request_type,
provider_pricing_override and custom_provider_config (including
supports_responses_api) that shadow the canonical definitions later; remove this
entire duplicate block and instead merge the intended additions (add "regex" and
stream request types to the existing pricing_override_request_type enum,
add/extend fields in provider_pricing_override, and add supports_responses_api
plus any new allowed_requests/request_path_overrides entries) into the single
canonical $defs definitions already present (refer to the symbols
pricing_override_match_type, pricing_override_request_type,
provider_pricing_override, and custom_provider_config to locate where to apply
the changes).
---
Outside diff comments:
In `@plugins/compat/main.go`:
- Around line 22-24: The Compat Config IsEnabled method omits the new
ResponsesToChatFallback flag so the compat plugin is stripped when only that
feature is set; update the Config type to add a ResponsesToChatFallback boolean
and include it in the Config.IsEnabled() returned expression (or alternatively
move the Responses→Chat fallback orchestration out of the optional compat plugin
into the always-registered code path so PreLLMHook still runs), then ensure any
code referencing the old Config (e.g., places that construct or marshal Config)
is updated to handle the new field.
---
Duplicate comments:
In `@core/providers/openai/responses_fallback_test.go`:
- Around line 217-221: In the httptest.NewServer handler used in
responses_fallback_test.go (the inline http.HandlerFunc reading request bodies),
replace any t.Fatalf calls with t.Errorf, then send an appropriate HTTP error
response (e.g., http.Error(w, "read body failed",
http.StatusInternalServerError)) and return from the handler so the server
goroutine doesn't just exit; apply the same change to the second occurrence
noted (the other handler around the 335-339 region).
🪄 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: f44d53cd-0a8b-4e09-b875-bad1d8256c20
📒 Files selected for processing (15)
core/bifrost.gocore/custom_provider_context_test.gocore/providers/openai/openai.gocore/providers/openai/responses_fallback_test.gocore/providers/utils/utils.gocore/schemas/bifrost.gocore/schemas/mux.gocore/schemas/mux_test.gocore/schemas/provider.goplugins/compat/main.goplugins/compat/responsestochat.goplugins/compat/responsestochat_test.goplugins/compat/runtime_fallback_integration_test.goplugins/litellmcompat/changelog.mdtransports/config.schema.json
✅ Files skipped from review due to trivial changes (2)
- core/custom_provider_context_test.go
- plugins/compat/runtime_fallback_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/utils/utils.go
- core/schemas/mux_test.go
- core/bifrost.go
Vaibhav701161
force-pushed
the
fix/responses-chat-fallback
branch
from
9a7d5e9 to
6607831
Compare
Vaibhav701161
force-pushed
the
fix/responses-chat-fallback
branch
from
6607831 to
8d89df6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/utils/utils.go (1)
1745-1837:⚠️ Potential issue | 🟠 MajorMake the guard check for an empty chunk, not just a nil pointer.
BuildClientStreamChunkalways returns&schemas.BifrostStreamChunk{}, so Line 1873 is always true even when post-hooks return no response and no error. This can still emit empty{}stream chunks.Suggested fix
- streamResponse := BuildClientStreamChunk(ctx, processedResponse, processedError) - if streamResponse != nil { + streamResponse := BuildClientStreamChunk(ctx, processedResponse, processedError) + if hasStreamChunkPayload(streamResponse) { select { case responseChan <- streamResponse: case <-ctx.Done(): return } }Add a shared helper near
BuildClientStreamChunk:+func hasStreamChunkPayload(chunk *schemas.BifrostStreamChunk) bool { + return chunk != nil && + (chunk.BifrostTextCompletionResponse != nil || + chunk.BifrostChatResponse != nil || + chunk.BifrostResponsesStreamResponse != nil || + chunk.BifrostSpeechStreamResponse != nil || + chunk.BifrostTranscriptionStreamResponse != nil || + chunk.BifrostImageGenerationStreamResponse != nil || + chunk.BifrostPassthroughResponse != nil || + chunk.BifrostError != nil) +}Also applies to: 1872-1879
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/utils/utils.go` around lines 1745 - 1837, BuildClientStreamChunk currently treats a non-nil processedResponse as meaningful even when all its payload fields are nil, causing empty `{}` stream chunks; add a helper like IsEmptyBifrostResponse(resp *schemas.BifrostResponse) bool that returns true when resp is nil or all of resp.TextCompletionResponse, ChatResponse, ResponsesStreamResponse, SpeechStreamResponse, TranscriptionStreamResponse and ImageGenerationStreamResponse are nil, then at the top of BuildClientStreamChunk check if IsEmptyBifrostResponse(processedResponse) && processedError == nil and return nil (or an explicit nil-equivalent) to avoid emitting empty stream chunks; update callers to handle a nil return if needed.core/schemas/mux.go (1)
1013-1015:⚠️ Potential issue | 🟠 MajorKeep fallback-only normalization behind
ToChatFallbackRequest().
ToChatRequest()is the general Responses→Chat converter, but it now applies fallback-specific behavior: developer roles are normalized and tools/tool choices are sanitized. That can silently drop/alter data for non-fallback callers. Move the lossy shaping intoToChatFallbackRequest()and keepToChatRequest()as the broader conversion path.Proposed direction
// Convert Input messages using existing ToChatMessages() bcr.Input = ToChatMessages(brr.Input) - normalizeDeveloperRoleForChatFallback(bcr.Input) @@ - // Responses -> Chat fallback only supports function tools in a valid chat shape. - bcr.Params.Tools = sanitizeResponsesToolsForChatFallback(brr.Params.Tools) + if len(brr.Params.Tools) > 0 { + chatTools := make([]ChatTool, 0, len(brr.Params.Tools)) + for _, responsesTool := range brr.Params.Tools { + if chatTool := responsesTool.ToChatTool(); chatTool != nil { + chatTools = append(chatTools, *chatTool) + } + } + bcr.Params.Tools = chatTools + } // Convert ToolChoice using existing ResponsesToolChoice.ToChatToolChoice() if brr.Params.ToolChoice != nil { chatToolChoice := brr.Params.ToolChoice.ToChatToolChoice() - bcr.Params.ToolChoice = sanitizeChatToolChoiceForFallback(chatToolChoice, bcr.Params.Tools) + bcr.Params.ToolChoice = chatToolChoice } @@ func (brr *BifrostResponsesRequest) ToChatFallbackRequest() *BifrostChatRequest { - return brr.ToChatRequest() + req := brr.ToChatRequest() + if req == nil { + return nil + } + normalizeDeveloperRoleForChatFallback(req.Input) + if req.Params != nil && brr != nil && brr.Params != nil { + req.Params.Tools = sanitizeResponsesToolsForChatFallback(brr.Params.Tools) + if brr.Params.ToolChoice != nil { + req.Params.ToolChoice = sanitizeChatToolChoiceForFallback(brr.Params.ToolChoice.ToChatToolChoice(), req.Params.Tools) + } + } + return req }Also applies to: 1051-1058, 1079-1083
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 1013 - 1015, The conversion in ToChatRequest() is applying fallback-only transformations (calling normalizeDeveloperRoleForChatFallback on bcr.Input and sanitizing tools/tool choices); remove those fallback-specific calls from ToChatRequest() (including the other similar occurrences around the ToChatMessages conversion and tool-choice sanitization) and instead add them to ToChatFallbackRequest() so that ToChatRequest() remains a general Responses→Chat converter and ToChatFallbackRequest() performs the lossy fallback shaping; locate calls to ToChatMessages, normalizeDeveloperRoleForChatFallback, and any tool/tool-choice sanitization helpers and move the fallback logic into ToChatFallbackRequest() while keeping ToChatRequest() unchanged otherwise.
♻️ Duplicate comments (2)
core/providers/openai/responses_fallback_test.go (1)
217-221:⚠️ Potential issue | 🟡 MinorAvoid
Fatalfinsidehttptesthandlers.These handlers run on server goroutines;
Fatalfcan stop only that goroutine and leave the client path with a timeout/incomplete response. Uset.Errorf, write an HTTP error status, and return.Proposed fix pattern
bodyBytes, err := io.ReadAll(r.Body) if err != nil { - t.Fatalf("failed to read request body: %v", err) + t.Errorf("failed to read request body: %v", err) + w.WriteHeader(http.StatusBadRequest) + return }Apply the same replacement at both handler sites.
Also applies to: 335-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responses_fallback_test.go` around lines 217 - 221, Replace the use of t.Fatalf inside the httptest server handlers with a non-fatal error path: in the handler functions (the httptest.NewServer http.HandlerFunc closures in responses_fallback_test.go) change t.Fatalf(...) to t.Errorf(...), write an appropriate HTTP error status (e.g., w.WriteHeader(http.StatusInternalServerError)), and return immediately so the client receives a response; apply the same change to both handler sites (the one around the io.ReadAll error at the first handler and the second occurrence reported).core/bifrost.go (1)
4103-4114:⚠️ Potential issue | 🟠 MajorRefresh custom-provider metadata after pre-hook routing changes.
Metadata is set before
RunLLMPreHooks, but pre-hooks can rewrite the request provider. The currentpreReq.GetRequestFields()result is discarded, soSupportsResponsesAPI/custom-provider metadata can stay tied to the previous provider and drive the wrong compat/fallback behavior.Suggested fix pattern
preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) +if preReq != nil { + updatedProvider, updatedModel, _ := preReq.GetRequestFields() + if updatedProvider != provider { + if err := bifrost.setProviderContextMetadataForKey(ctx, updatedProvider); err != nil { + bifrostErr := newBifrostError(err) + bifrostErr.ExtraFields = schemas.BifrostErrorExtraFields{ + RequestType: req.RequestType, + Provider: updatedProvider, + OriginalModelRequested: updatedModel, + } + return nil, bifrostErr + } + provider = updatedProvider + model = updatedModel + } +}Apply the same refresh in the stream and non-stream pre-hook paths before constructing post-hook runners or enqueueing the request.
Based on learnings: provider switching from a PreLLMHook plugin is performed by calling
BifrostRequest.UpdateProvider(provider), andgetProviderQueueuses the updated provider as the single source of truth.Also applies to: 4144-4179, 4652-4662, 4685-4698, 4869-4879, 4918-4936
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4103 - 4114, Pre-hooks can change the request provider via BifrostRequest.UpdateProvider, so refresh the provider/model from req.GetRequestFields() after running RunLLMPreHooks (both stream and non-stream pre-hook paths) and before calling setProviderContextMetadataForKey and schemas.ClearResponsesToChatCompletionFallback/CompatState; update the blocks that currently call setProviderContextMetadataForKey with the original provider to instead re-fetch provider/model from req.GetRequestFields() and then call setProviderContextMetadataForKey(ctx, provider) so SupportsResponsesAPI/custom-provider metadata uses the post-hook provider (also apply the same refresh in the locations referenced by getProviderQueue usage).
🧹 Nitpick comments (1)
plugins/compat/runtime_fallback_integration_test.go (1)
29-47: Nit: redundant&on composite literal inside map.
map[K]*T{ k: &T{...} }can be written asmap[K]*T{ k: {...} }pergofmt/vet idioms, but this is purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/runtime_fallback_integration_test.go` around lines 29 - 47, The composite literal in the configs map uses an unnecessary address operator; change the entry where providerKey maps to &schemas.ProviderConfig{...} to use the pointer-typed map literal form by removing the leading '&' and writing the literal as schemas.ProviderConfig{...} (the map is of type map[schemas.ModelProvider]*schemas.ProviderConfig so the element will be a pointer); update the literal in the configs map (the providerKey -> schemas.ProviderConfig entry) to remove the redundant '&' to satisfy gofmt/vet style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 5776-5818: The postHookRunner currently only tags fallback
metadata but doesn't convert runtime fallback chat-stream chunks back into the
Responses format, so clients of ResponsesStreamRequest may receive
BifrostChatResponse chunks instead of BifrostResponsesStreamResponse; update
postHookRunner (the function passed into handleProviderStreamRequest) to, after
calling pipeline.RunPostLLMHooks and
schemas.ApplyResponsesToChatCompletionFallbackMetadata, detect when
state.OriginalRequestType indicates a ResponsesStreamRequest and the returned
streaming chunk is a chat-style chunk, then convert those chunks to the
Responses format using the existing conversion helper BuildClientStreamChunk (or
equivalent), preserving the populated response field and extra fields before
returning resp to the caller. Ensure PopulateExtraFields is still applied to the
converted response and that this conversion happens prior to finalizer/release
logic.
In `@plugins/compat/responsestochat.go`:
- Around line 77-80: The response and error conversion only sets the boolean
marker ResponsesToChatCompletionFallback but drops the actual fallback reason;
update both places where you set
responsesResponse.ExtraFields.ResponsesToChatCompletionFallback = true and the
analogous errorsResponse.ExtraFields assignment (lines around the second block)
to also propagate state.FallbackReason into a dedicated field (e.g.,
ExtraFields.FallbackReason or
ExtraFields.ResponsesToChatCompletionFallbackReason), copying
state.FallbackReason so callers and observability can see configured-vs-runtime
attribution.
In `@plugins/compat/runtime_fallback_integration_test.go`:
- Around line 184-204: The test HTTP handler uses t.Fatalf inside the server
goroutine (e.g., the httptest.NewServer handler that calls io.ReadAll and
handles "/v1/responses" and "/v1/chat/completions"), which is unsafe; replace
t.Fatalf(...) with t.Errorf(...) followed by an immediate return so the handler
logs the error and stops processing, and do the same for the other identical
io.ReadAll error sites in this file; for the http.Flusher assertion mentioned,
instead of t.Fatalf write an error response (e.g.,
w.WriteHeader(http.StatusInternalServerError) and optionally w.Write) then
return, so the test goroutine remains responsible for failing the test.
- Around line 89-92: The test's fatal error message is stale: the call is to
Init(Config{}, logger, nil) but the message says InitWithModelCatalog; update
the t.Fatalf message to accurately reference the Init call (e.g., "Init returned
error: %v" or "Init(Config, ...) returned error: %v") so failures point to the
correct function; edit the assertion in the test where Init(...) is called to
use the updated message.
---
Outside diff comments:
In `@core/providers/utils/utils.go`:
- Around line 1745-1837: BuildClientStreamChunk currently treats a non-nil
processedResponse as meaningful even when all its payload fields are nil,
causing empty `{}` stream chunks; add a helper like IsEmptyBifrostResponse(resp
*schemas.BifrostResponse) bool that returns true when resp is nil or all of
resp.TextCompletionResponse, ChatResponse, ResponsesStreamResponse,
SpeechStreamResponse, TranscriptionStreamResponse and
ImageGenerationStreamResponse are nil, then at the top of BuildClientStreamChunk
check if IsEmptyBifrostResponse(processedResponse) && processedError == nil and
return nil (or an explicit nil-equivalent) to avoid emitting empty stream
chunks; update callers to handle a nil return if needed.
In `@core/schemas/mux.go`:
- Around line 1013-1015: The conversion in ToChatRequest() is applying
fallback-only transformations (calling normalizeDeveloperRoleForChatFallback on
bcr.Input and sanitizing tools/tool choices); remove those fallback-specific
calls from ToChatRequest() (including the other similar occurrences around the
ToChatMessages conversion and tool-choice sanitization) and instead add them to
ToChatFallbackRequest() so that ToChatRequest() remains a general Responses→Chat
converter and ToChatFallbackRequest() performs the lossy fallback shaping;
locate calls to ToChatMessages, normalizeDeveloperRoleForChatFallback, and any
tool/tool-choice sanitization helpers and move the fallback logic into
ToChatFallbackRequest() while keeping ToChatRequest() unchanged otherwise.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 4103-4114: Pre-hooks can change the request provider via
BifrostRequest.UpdateProvider, so refresh the provider/model from
req.GetRequestFields() after running RunLLMPreHooks (both stream and non-stream
pre-hook paths) and before calling setProviderContextMetadataForKey and
schemas.ClearResponsesToChatCompletionFallback/CompatState; update the blocks
that currently call setProviderContextMetadataForKey with the original provider
to instead re-fetch provider/model from req.GetRequestFields() and then call
setProviderContextMetadataForKey(ctx, provider) so
SupportsResponsesAPI/custom-provider metadata uses the post-hook provider (also
apply the same refresh in the locations referenced by getProviderQueue usage).
In `@core/providers/openai/responses_fallback_test.go`:
- Around line 217-221: Replace the use of t.Fatalf inside the httptest server
handlers with a non-fatal error path: in the handler functions (the
httptest.NewServer http.HandlerFunc closures in responses_fallback_test.go)
change t.Fatalf(...) to t.Errorf(...), write an appropriate HTTP error status
(e.g., w.WriteHeader(http.StatusInternalServerError)), and return immediately so
the client receives a response; apply the same change to both handler sites (the
one around the io.ReadAll error at the first handler and the second occurrence
reported).
---
Nitpick comments:
In `@plugins/compat/runtime_fallback_integration_test.go`:
- Around line 29-47: The composite literal in the configs map uses an
unnecessary address operator; change the entry where providerKey maps to
&schemas.ProviderConfig{...} to use the pointer-typed map literal form by
removing the leading '&' and writing the literal as schemas.ProviderConfig{...}
(the map is of type map[schemas.ModelProvider]*schemas.ProviderConfig so the
element will be a pointer); update the literal in the configs map (the
providerKey -> schemas.ProviderConfig entry) to remove the redundant '&' to
satisfy gofmt/vet style.
🪄 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: d6607a40-a9bb-4c5c-bace-5ca9d03dcac4
📒 Files selected for processing (14)
core/bifrost.gocore/custom_provider_context_test.gocore/providers/openai/openai.gocore/providers/openai/responses_fallback_test.gocore/providers/utils/utils.gocore/schemas/bifrost.gocore/schemas/mux.gocore/schemas/mux_test.gocore/schemas/provider.goplugins/compat/main.goplugins/compat/responsestochat.goplugins/compat/responsestochat_test.goplugins/compat/runtime_fallback_integration_test.gotransports/config.schema.json
✅ Files skipped from review due to trivial changes (1)
- plugins/compat/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/config.schema.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
core/bifrost.go (1)
4097-4132:⚠️ Potential issue | 🟠 MajorRefresh provider metadata after prehook routing changes.
Line 4130 reads
preReq.GetRequestFields()and discards the values, so metadata remains tied to the original provider. If a prehook rewrites routing, later compat/post-hook logic can still see staleSupportsResponsesAPI/custom-provider metadata. Apply the same refresh pattern inRunStreamPreHooks,tryRequest, andtryStreamRequest; if compat prehooks must observe the new provider within the same pipeline, move this refresh into the prehook loop immediately after provider changes.Based on learnings: provider switching from a PreLLMHook plugin is performed by calling
BifrostRequest.UpdateProvider(provider), andgetProviderQueueuses the updated provider as the single source of truth.Also applies to: 4638-4665, 4871-4965
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4097 - 4132, The prehook routing change is not being picked up because preReq.GetRequestFields() is called and its results discarded, leaving metadata (e.g. SupportsResponsesAPI/custom-provider flags) tied to the original provider; update the code to refresh provider-dependent metadata after any prehook that may call BifrostRequest.UpdateProvider by re-reading request fields and updating local metadata before running compat/post-hook logic. Specifically, in RunStreamPreHooks, tryRequest, tryStreamRequest and the RunLLMPreHooks handling here, after the prehook loop (or immediately inside the prehook loop when a provider change is detected), call preReq.GetRequestFields() and use its returned values to refresh the provider-related variables used by getProviderQueue, runPostLLMHooksWithResponsesCompatMetadata, and any SupportsResponsesAPI checks so subsequent hooks see the new provider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 3998-4020: The metadata helper is reading the raw account config
key (which can be empty) instead of the routed/normalized provider key; update
setProviderContextMetadata to accept a providerKey parameter (e.g., func
setProviderContextMetadata(ctx *schemas.BifrostContext, config
*schemas.ProviderConfig, providerKey schemas.ModelProvider)) and when building
CustomProviderContextMetadata use the passed providerKey
(schemas.ModelProvider(providerKey)) for ProviderKey (falling back to
config.CustomProviderConfig.CustomProviderKey only if providerKey is empty),
then update setProviderContextMetadataForKey to pass its providerKey into
setProviderContextMetadata; this ensures custom-provider metadata uses the
normalized routed key.
- Around line 5710-5774: The fallback logic reuses the original keyProvider but
should build a new key provider for the fallback request type; before calling
executeRequestWithRetries in both the IsStreamRequestType branch and the
non-streaming branch, create a new key provider from
state.FallbackRequest.RequestType (e.g., fallbackKeyProvider :=
buildKeyProviderForType(state.FallbackRequest.RequestType, ...)) and pass that
fallbackKeyProvider into executeRequestWithRetries instead of the original
keyProvider so the chat fallback uses the correct keys and allowances; update
references around executeRequestWithRetries, keyProvider, state.FallbackRequest,
and state.FallbackRequest.RequestType to use the new fallbackKeyProvider.
In `@core/schemas/bifrost.go`:
- Around line 890-908: GetResponsesToChatCompletionFallback currently returns
("", true) when isFallback is true but the reason value has an unexpected type,
which causes callers like ApplyResponsesToChatCompletionFallbackMetadata to
treat an empty string as a valid reason; fix by either (preferred) changing the
default branch to return ("", false) so callers know the reason is absent, or
(alternative) add a clear doc comment on GetResponsesToChatCompletionFallback
stating that ok may be true with an empty reason when
BifrostContextKeyResponsesToChatCompletionFallbackReason has an unexpected type;
reference the function name GetResponsesToChatCompletionFallback and the context
keys BifrostContextKeyIsResponsesToChatCompletionFallback and
BifrostContextKeyResponsesToChatCompletionFallbackReason when making the change.
- Around line 910-926: ApplyResponsesToChatCompletionFallbackMetadata currently
mutates a throwaway BifrostResponseExtraFields when response has no active
subtype; detect the empty-envelope case after calling response.GetExtraFields()
(by checking that none of the inner subtype getters on response are non-nil)
and, if empty, persist the mutated extraFields back onto the envelope (e.g.,
assign the extra-fields into the response envelope via the generated
setter/field so the mutation is not lost); do the analogous guard for
bifrostErr.ExtraFields if necessary. Ensure you reference
ApplyResponsesToChatCompletionFallbackMetadata, response.GetExtraFields(),
BifrostResponseExtraFields and bifrostErr.ExtraFields when implementing the fix.
- Around line 1251-1253: The struct field alignment for
ResponsesToChatCompletionFallback and ResponsesToChatCompletionFallbackReason is
inconsistent with surrounding fields in BifrostResponseExtraFields and the same
misalignment exists in BifrostErrorExtraFields; fix by running gofmt -w on the
file to reformat the struct field columns so the spacing before types and tags
matches other fields (ensure the two fields align with DroppedCompatPluginParams
and ProviderResponseHeaders in both BifrostResponseExtraFields and
BifrostErrorExtraFields).
- Around line 795-805: In ToChatRequest(), when constructing FallbackRequest
(the *BifrostRequest containing a BifrostChatRequest), deep-copy mutable fields
that currently point to pooled data: copy the maps brr.Params.ExtraParams and
brr.Params.Metadata into new maps (use maps.Copy or allocate and copy entries)
and copy the byte slice brr.RawRequestBody into a new slice before assigning
them into the fallback request, so FallbackRequest.ChatParameters does not
reference the original pooled maps/slice and avoid data races.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 4097-4132: The prehook routing change is not being picked up
because preReq.GetRequestFields() is called and its results discarded, leaving
metadata (e.g. SupportsResponsesAPI/custom-provider flags) tied to the original
provider; update the code to refresh provider-dependent metadata after any
prehook that may call BifrostRequest.UpdateProvider by re-reading request fields
and updating local metadata before running compat/post-hook logic. Specifically,
in RunStreamPreHooks, tryRequest, tryStreamRequest and the RunLLMPreHooks
handling here, after the prehook loop (or immediately inside the prehook loop
when a provider change is detected), call preReq.GetRequestFields() and use its
returned values to refresh the provider-related variables used by
getProviderQueue, runPostLLMHooksWithResponsesCompatMetadata, and any
SupportsResponsesAPI checks so subsequent hooks see the new provider.
🪄 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: f8d8b7d2-271c-4871-b49b-4bda14eb52c2
📒 Files selected for processing (4)
core/bifrost.gocore/providers/openai/openai.gocore/schemas/bifrost.goplugins/compat/runtime_fallback_integration_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
core/bifrost.go (3)
4056-4068:⚠️ Potential issue | 🟠 MajorRefresh custom-provider metadata when pre-hooks rewrite routing.
These blocks seed metadata before
RunLLMPreHooks, but a pre-hook can change the provider. If that happens, later compat logic can still read the original provider’sSupportsResponsesAPI.Refresh metadata whenever the request provider changes during the pre-hook chain, ideally before subsequent pre-hooks run. Based on learnings: provider switching from a PreLLMHook plugin is performed by calling
BifrostRequest.UpdateProvider(provider), and routing uses the updated provider as the source of truth.Also applies to: 4605-4615, 4822-4832
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4056 - 4068, The code seeds provider metadata via setProviderContextMetadataForKey before running pre-hooks but does not refresh it if a pre-hook calls BifrostRequest.UpdateProvider and rewrites routing, so downstream compat logic (e.g., reading SupportsResponsesAPI) can use stale data; update the flow in the RunLLMPreHooks path (and the similar blocks around the other occurrences) to detect provider changes after each pre-hook invocation and re-call setProviderContextMetadataForKey(ctx, newProvider) when req.UpdateProvider is observed (use req.GetRequestFields() or the request's provider accessor to get the current provider), ensuring metadata is refreshed before running subsequent pre-hooks or compatibility checks.
5728-5744:⚠️ Potential issue | 🟠 MajorRebuild key selection for the fallback request type.
The fallback executes
state.FallbackRequest.RequestType, but still reuseskeyProvider/keysselected for the original Responses request. This can violate per-keyAllowedRequestsgates when falling back to Chat Completions.Build the fallback key provider using
state.FallbackRequest.RequestTypebefore these retry calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5728 - 5744, The fallback branch reuses the original keyProvider/keys even though it invokes state.FallbackRequest.RequestType, which can bypass per-key AllowedRequests checks; before calling executeRequestWithRetries for the fallback (both stream and non-stream paths where state.FallbackRequest is used), rebuild the key provider/keys for state.FallbackRequest.RequestType (e.g., create a new keyProvider and keys by calling the same key selection routine used earlier but passing state.FallbackRequest.RequestType) and pass those into executeRequestWithRetries (and makeStreamAttempt / bifrost.handleProviderRequest) instead of the original keyProvider/keys so per-key AllowedRequests are enforced for the fallback request type.
3998-4020:⚠️ Potential issue | 🟡 MinorUse the routed provider key in custom-provider metadata.
CustomProviderKeycan be empty on config objects read directly from the account, soProviderKeymay be wrong for custom providers. Pass the routed provider key intosetProviderContextMetadataand use that when populatingCustomProviderContextMetadata.Suggested patch
-func setProviderContextMetadata(ctx *schemas.BifrostContext, config *schemas.ProviderConfig) { +func setProviderContextMetadata(ctx *schemas.BifrostContext, providerKey schemas.ModelProvider, config *schemas.ProviderConfig) { isCustomProvider := config != nil && config.CustomProviderConfig != nil ctx.SetValue(schemas.BifrostContextKeyIsCustomProvider, isCustomProvider) if isCustomProvider { + if providerKey == "" { + providerKey = schemas.ModelProvider(config.CustomProviderConfig.CustomProviderKey) + } ctx.SetValue(schemas.BifrostContextKeyCustomProviderMetadata, &schemas.CustomProviderContextMetadata{ - ProviderKey: schemas.ModelProvider(config.CustomProviderConfig.CustomProviderKey), + ProviderKey: providerKey, BaseProviderType: config.CustomProviderConfig.BaseProviderType, SupportsResponsesAPI: config.CustomProviderConfig.SupportsResponsesAPI, }) @@ - setProviderContextMetadata(ctx, config) + setProviderContextMetadata(ctx, providerKey, config) return nil } @@ - setProviderContextMetadata(req.Context, config) + setProviderContextMetadata(req.Context, provider.GetProviderKey(), config)Also applies to: 5435-5435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 3998 - 4020, The custom-provider metadata currently uses config.CustomProviderConfig.CustomProviderKey which can be empty; update setProviderContextMetadata to accept the routed provider key (e.g., add a providerKey parameter) and, when isCustomProvider is true, set CustomProviderContextMetadata.ProviderKey from that routed providerKey (instead of config.CustomProviderConfig.CustomProviderKey). Also update callers such as setProviderContextMetadataForKey to pass the providerKey they resolved into setProviderContextMetadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 5728-5730: The code dereferences state.FallbackRequest when
constructing fallbackMsg without ensuring it is non-nil; update the branch that
checks schemas.GetResponsesToChatCompletionCompatState and state.ShouldRetry to
also guard that state.FallbackRequest != nil before using it. Specifically, in
bifrost.go where you call GetResponsesToChatCompletionCompatState and then
create fallbackMsg := &ChannelMessage{BifrostRequest: *state.FallbackRequest,
Context: req.Context}, add a nil-check for state.FallbackRequest and only
activate ResponsesToChatCompletionCompatState and construct fallbackMsg when the
FallbackRequest is present; if it’s nil, skip the fallback path (or handle the
error path) to avoid dereferencing a nil pointer.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 4056-4068: The code seeds provider metadata via
setProviderContextMetadataForKey before running pre-hooks but does not refresh
it if a pre-hook calls BifrostRequest.UpdateProvider and rewrites routing, so
downstream compat logic (e.g., reading SupportsResponsesAPI) can use stale data;
update the flow in the RunLLMPreHooks path (and the similar blocks around the
other occurrences) to detect provider changes after each pre-hook invocation and
re-call setProviderContextMetadataForKey(ctx, newProvider) when
req.UpdateProvider is observed (use req.GetRequestFields() or the request's
provider accessor to get the current provider), ensuring metadata is refreshed
before running subsequent pre-hooks or compatibility checks.
- Around line 5728-5744: The fallback branch reuses the original
keyProvider/keys even though it invokes state.FallbackRequest.RequestType, which
can bypass per-key AllowedRequests checks; before calling
executeRequestWithRetries for the fallback (both stream and non-stream paths
where state.FallbackRequest is used), rebuild the key provider/keys for
state.FallbackRequest.RequestType (e.g., create a new keyProvider and keys by
calling the same key selection routine used earlier but passing
state.FallbackRequest.RequestType) and pass those into executeRequestWithRetries
(and makeStreamAttempt / bifrost.handleProviderRequest) instead of the original
keyProvider/keys so per-key AllowedRequests are enforced for the fallback
request type.
- Around line 3998-4020: The custom-provider metadata currently uses
config.CustomProviderConfig.CustomProviderKey which can be empty; update
setProviderContextMetadata to accept the routed provider key (e.g., add a
providerKey parameter) and, when isCustomProvider is true, set
CustomProviderContextMetadata.ProviderKey from that routed providerKey (instead
of config.CustomProviderConfig.CustomProviderKey). Also update callers such as
setProviderContextMetadataForKey to pass the providerKey they resolved into
setProviderContextMetadata.
🪄 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: ab559b46-646f-4e31-9bbe-497edb805a28
📒 Files selected for processing (2)
core/bifrost.goplugins/compat/runtime_fallback_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/compat/runtime_fallback_integration_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
core/bifrost.go (4)
3998-4020:⚠️ Potential issue | 🟡 MinorUse the routed provider key when setting custom-provider metadata.
CustomProviderKeycan be normalized outside the raw account config, but this helper still readsconfig.CustomProviderConfig.CustomProviderKey. Pass the routedproviderKeyintosetProviderContextMetadataand use it forCustomProviderContextMetadata.ProviderKey; this avoids empty/stale provider metadata for custom OpenAI-compatible providers.Suggested patch
-func setProviderContextMetadata(ctx *schemas.BifrostContext, config *schemas.ProviderConfig) { +func setProviderContextMetadata(ctx *schemas.BifrostContext, providerKey schemas.ModelProvider, config *schemas.ProviderConfig) { isCustomProvider := config != nil && config.CustomProviderConfig != nil ctx.SetValue(schemas.BifrostContextKeyIsCustomProvider, isCustomProvider) if isCustomProvider { + metadataProviderKey := providerKey + if metadataProviderKey == "" { + metadataProviderKey = schemas.ModelProvider(config.CustomProviderConfig.CustomProviderKey) + } ctx.SetValue(schemas.BifrostContextKeyCustomProviderMetadata, &schemas.CustomProviderContextMetadata{ - ProviderKey: schemas.ModelProvider(config.CustomProviderConfig.CustomProviderKey), + ProviderKey: metadataProviderKey, BaseProviderType: config.CustomProviderConfig.BaseProviderType, SupportsResponsesAPI: config.CustomProviderConfig.SupportsResponsesAPI, }) @@ - setProviderContextMetadata(ctx, config) + setProviderContextMetadata(ctx, providerKey, config) return nil } @@ - setProviderContextMetadata(req.Context, config) + setProviderContextMetadata(req.Context, provider.GetProviderKey(), config)Also applies to: 5466-5466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 3998 - 4020, The helper setProviderContextMetadata currently reads ProviderKey from config.CustomProviderConfig.CustomProviderKey which can be stale; change setProviderContextMetadata to accept the routed providerKey (schemas.ModelProvider) from setProviderContextMetadataForKey and use that routed providerKey when constructing schemas.CustomProviderContextMetadata.ProviderKey (instead of config.CustomProviderConfig.CustomProviderKey), and update the call site in setProviderContextMetadataForKey to pass the providerKey; keep all other metadata fields sourced from config.CustomProviderConfig.
4056-4068:⚠️ Potential issue | 🟠 MajorRefresh compat metadata after pre-hooks can rewrite routing.
These blocks set custom-provider metadata before
RunLLMPreHooks, but pre-hooks can rewrite the provider. RefreshBifrostContextKeyCustomProviderMetadatafrom the provider onpreReqafter pre-hooks so the compat plugin and post-hooks don’t observe staleSupportsResponsesAPImetadata.Suggested pattern
preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) +if preReq != nil { + updatedProvider, _, _ := preReq.GetRequestFields() + if updatedProvider != provider { + if err := bifrost.setProviderContextMetadataForKey(ctx, updatedProvider); err != nil { + bifrostErr := newBifrostError(err) + bifrostErr.PopulateExtraFields(req.RequestType, updatedProvider, model, model) + return nil, bifrostErr + } + } +}Based on learnings: provider switching from a PreLLMHook plugin is performed by calling
BifrostRequest.UpdateProvider(provider), andgetProviderQueueuses the updated provider as the single source of truth.Also applies to: 4625-4635, 4846-4856
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4056 - 4068, The custom-provider metadata is set before running pre-hooks but pre-hooks can call BifrostRequest.UpdateProvider and change routing, so after RunLLMPreHooks you must refresh BifrostContextKeyCustomProviderMetadata from the updated preReq provider: retrieve provider via preReq.GetRequestFields()/preReq.GetProvider() (the same source getProviderQueue uses) and call setProviderContextMetadataForKey(ctx, provider) again so SupportsResponsesAPI and compat/post-hooks see the current provider; apply the same refresh pattern around the other locations noted (the blocks near 4625-4635 and 4846-4856).
5758-5762:⚠️ Potential issue | 🟠 MajorGuard
FallbackRequestbefore dereferencing.
ResponsesToChatCompletionCompatState.FallbackRequestis nullable, but Line 5761 dereferences it after only checkingstate.ShouldRetry(...). A partial/invalid compat state can panic the worker.Suggested patch
- if state, ok := schemas.GetResponsesToChatCompletionCompatState(req.Context); ok && state != nil && state.ShouldRetry(bifrostError) { + if state, ok := schemas.GetResponsesToChatCompletionCompatState(req.Context); ok && state != nil && state.FallbackRequest != nil && state.ShouldRetry(bifrostError) { schemas.ActivateResponsesToChatCompletionCompatState(req.Context, schemas.ResponsesToChatCompletionFallbackReasonRuntimeUnsupported) fallbackMsg := &ChannelMessage{BifrostRequest: *state.FallbackRequest, Context: req.Context}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5758 - 5762, The code dereferences ResponsesToChatCompletionCompatState.FallbackRequest when building fallbackMsg after state.ShouldRetry(...) which can be nil; update the block around state.ShouldRetry in bifrost.go to first check state.FallbackRequest != nil (e.g., if state != nil && state.FallbackRequest != nil && state.ShouldRetry(bifrostError)) and only then create fallbackMsg := &ChannelMessage{BifrostRequest: *state.FallbackRequest, Context: req.Context}; if FallbackRequest is nil, call schemas.ActivateResponsesToChatCompletionCompatState(req.Context, schemas.ResponsesToChatCompletionFallbackReasonRuntimeUnsupported) (or another appropriate fallback path) and avoid constructing the ChannelMessage or attempting to stream (IsStreamRequestType and req.RequestType) to prevent a panic.
5762-5774:⚠️ Potential issue | 🟠 MajorSelect keys using the fallback request type.
The runtime fallback executes
state.FallbackRequest.RequestTypebut still passes the originalkeyProvider, which was filtered for the Responses request type. Rebuild/select keys for the Chat fallback type soAllowedRequestsand request-type gates are evaluated correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5762 - 5774, The fallback path is still using the original keyProvider (filtered for the original request type) so AllowedRequests gating is wrong; when calling executeRequestWithRetries for the fallback (both the IsStreamRequestType branch that uses makeStreamAttempt and the non-stream branch that passes the inline func to bifrost.handleProviderRequest), rebuild or select a new keyProvider filtered for state.FallbackRequest.RequestType (e.g., create fallbackKeyProvider = keys.SelectForRequestType(state.FallbackRequest.RequestType) or the equivalent key selection helper used elsewhere) and pass that fallbackKeyProvider into executeRequestWithRetries (and ensure lastAttemptFinalizer uses the same fallbackKeyProvider context if applicable) so request-type gates are evaluated against the Chat fallback type rather than the original keyProvider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/bifrost.go`:
- Around line 3998-4020: The helper setProviderContextMetadata currently reads
ProviderKey from config.CustomProviderConfig.CustomProviderKey which can be
stale; change setProviderContextMetadata to accept the routed providerKey
(schemas.ModelProvider) from setProviderContextMetadataForKey and use that
routed providerKey when constructing
schemas.CustomProviderContextMetadata.ProviderKey (instead of
config.CustomProviderConfig.CustomProviderKey), and update the call site in
setProviderContextMetadataForKey to pass the providerKey; keep all other
metadata fields sourced from config.CustomProviderConfig.
- Around line 4056-4068: The custom-provider metadata is set before running
pre-hooks but pre-hooks can call BifrostRequest.UpdateProvider and change
routing, so after RunLLMPreHooks you must refresh
BifrostContextKeyCustomProviderMetadata from the updated preReq provider:
retrieve provider via preReq.GetRequestFields()/preReq.GetProvider() (the same
source getProviderQueue uses) and call setProviderContextMetadataForKey(ctx,
provider) again so SupportsResponsesAPI and compat/post-hooks see the current
provider; apply the same refresh pattern around the other locations noted (the
blocks near 4625-4635 and 4846-4856).
- Around line 5758-5762: The code dereferences
ResponsesToChatCompletionCompatState.FallbackRequest when building fallbackMsg
after state.ShouldRetry(...) which can be nil; update the block around
state.ShouldRetry in bifrost.go to first check state.FallbackRequest != nil
(e.g., if state != nil && state.FallbackRequest != nil &&
state.ShouldRetry(bifrostError)) and only then create fallbackMsg :=
&ChannelMessage{BifrostRequest: *state.FallbackRequest, Context: req.Context};
if FallbackRequest is nil, call
schemas.ActivateResponsesToChatCompletionCompatState(req.Context,
schemas.ResponsesToChatCompletionFallbackReasonRuntimeUnsupported) (or another
appropriate fallback path) and avoid constructing the ChannelMessage or
attempting to stream (IsStreamRequestType and req.RequestType) to prevent a
panic.
- Around line 5762-5774: The fallback path is still using the original
keyProvider (filtered for the original request type) so AllowedRequests gating
is wrong; when calling executeRequestWithRetries for the fallback (both the
IsStreamRequestType branch that uses makeStreamAttempt and the non-stream branch
that passes the inline func to bifrost.handleProviderRequest), rebuild or select
a new keyProvider filtered for state.FallbackRequest.RequestType (e.g., create
fallbackKeyProvider =
keys.SelectForRequestType(state.FallbackRequest.RequestType) or the equivalent
key selection helper used elsewhere) and pass that fallbackKeyProvider into
executeRequestWithRetries (and ensure lastAttemptFinalizer uses the same
fallbackKeyProvider context if applicable) so request-type gates are evaluated
against the Chat fallback type rather than the original keyProvider.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
core/custom_provider_context_test.go (1)
10-53: LGTM — focused coverage forsetProviderContextMetadatawith the newSupportsResponsesAPIfield.Both branches (custom-provider-set vs. empty config clears flags) are exercised, and the tests correctly assert the tri-state pointer semantics (non-nil pointer with value
false) that downstream fallback logic inplugins/compat/responsestochat.gorelies on. Context keys are accessed through the typedschemas.BifrostContextKey*constants, which complies with the repo's context-key rule.Minor optional nit: consider adding a third case for
SupportsResponsesAPI: nil(the auto-detection path), since that's the default/most-interesting tri-state branch for the feature in this PR. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/custom_provider_context_test.go` around lines 10 - 53, Add a third test case that verifies the tri-state nil behavior for SupportsResponsesAPI: create a new test (e.g., TestSetProviderContextMetadata_SupportsResponsesAPI_Nil) or extend the existing TestSetProviderContextMetadata_MarksCustomProvider to set CustomProviderConfig without the SupportsResponsesAPI field, call setProviderContextMetadata(ctx, config, schemas.ModelProvider(...)), then assert that schemas.GetCustomProviderContextMetadata(ctx) returns metadata with SupportsResponsesAPI == nil; use the existing context helpers and the BifrostContextKey* keys to locate the behavior in setProviderContextMetadata and ensure the nil (auto-detection) branch is covered.core/schemas/mux.go (2)
1104-1150: Optional: collapse the twobrr.Inputloops into one pass.The reasoning-input and developer-role checks both iterate
brr.Inputindependently and break on first match. You can merge them into a single loop with two sentinel booleans; keeps complexity O(n) either way but trims duplicated scan logic and is easier to extend when more per-input warnings are added.Proposed refactor
- for _, input := range brr.Input { - if input.Type != nil && *input.Type == ResponsesMessageTypeReasoning { - warnings = append(warnings, "reasoning input items are dropped during Responses to Chat fallback") - break - } - } - - for _, input := range brr.Input { - if input.Role != nil && *input.Role == ResponsesInputMessageRoleDeveloper { - warnings = append(warnings, "developer role is normalized to system during Responses to Chat fallback") - break - } - } + var reasoningWarned, developerWarned bool + for _, input := range brr.Input { + if !reasoningWarned && input.Type != nil && *input.Type == ResponsesMessageTypeReasoning { + warnings = append(warnings, "reasoning input items are dropped during Responses to Chat fallback") + reasoningWarned = true + } + if !developerWarned && input.Role != nil && *input.Role == ResponsesInputMessageRoleDeveloper { + warnings = append(warnings, "developer role is normalized to system during Responses to Chat fallback") + developerWarned = true + } + if reasoningWarned && developerWarned { + break + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 1104 - 1150, The two separate loops over brr.Input in ChatFallbackWarnings that check for ResponsesMessageTypeReasoning and ResponsesInputMessageRoleDeveloper should be merged into a single pass: iterate once over brr.Input, track two booleans (e.g., foundReasoning and foundDeveloper) and set them when you see input.Type == ResponsesMessageTypeReasoning and input.Role == ResponsesInputMessageRoleDeveloper respectively, append the corresponding warnings when first observed, and exit the loop early once both flags are true to preserve current short-circuit behavior.
1075-1081: Nit: preferbytes.Clonefor theRawRequestBodycopy.
bytes.Clone(stdlib, Go 1.20+) expresses intent more clearly and handles the nil/zero-length case in one call. Purely stylistic, behaviour is identical.Proposed change
- // Deep-copy RawRequestBody to prevent data races if the original request - // is returned to a pool while the FallbackRequest still holds a reference. - if brr.RawRequestBody != nil { - rawCopy := make([]byte, len(brr.RawRequestBody)) - copy(rawCopy, brr.RawRequestBody) - bcr.RawRequestBody = rawCopy - } + // Deep-copy RawRequestBody to prevent data races if the original request + // is returned to a pool while the FallbackRequest still holds a reference. + bcr.RawRequestBody = bytes.Clone(brr.RawRequestBody)Requires adding
"bytes"to the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 1075 - 1081, Replace the manual allocation+copy for deep-copying brr.RawRequestBody with bytes.Clone to simplify and handle nil/zero-length cases; update the block that sets bcr.RawRequestBody (referencing variables brr.RawRequestBody and bcr.RawRequestBody) to use bytes.Clone(brr.RawRequestBody) and add the "bytes" import to the file imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/mux.go`:
- Around line 1086-1095: The comment for cloneMetadataMap incorrectly states it
"deep-copies" but the implementation uses maps.Clone which only performs a
shallow copy; update the doc-comment for cloneMetadataMap to state it
shallow-clones the map (values like nested maps, slices, or pointers remain
shared) or implement a true deep-copy walk; reference the existing function name
cloneMetadataMap and the use of maps.Clone when making the change so reviewers
can find and update the comment or replace maps.Clone with a recursive
deep-cloning routine for nested map[string]any/slices/pointer-containing
structs.
- Around line 1038-1042: The conversion of brr.Params.Text.Format via
ConvertViaJSON currently ignores errors and leaves bcr.Params.ResponseFormat
nil; change this so conversion failures are surfaced instead of silent: call
ConvertViaJSON[interface{}](brr.Params.Text.Format) as now, and if err != nil
append a descriptive warning (including the error and the requested Format.Type)
to the fallback warnings collection (e.g., create or use bcr.FallbackWarnings
slice) or log it via the existing logger, otherwise set
bcr.Params.ResponseFormat = &responseFormat when successful; reference
brr.Params.Text.Format, ConvertViaJSON, and bcr.Params.ResponseFormat in your
changes.
---
Nitpick comments:
In `@core/custom_provider_context_test.go`:
- Around line 10-53: Add a third test case that verifies the tri-state nil
behavior for SupportsResponsesAPI: create a new test (e.g.,
TestSetProviderContextMetadata_SupportsResponsesAPI_Nil) or extend the existing
TestSetProviderContextMetadata_MarksCustomProvider to set CustomProviderConfig
without the SupportsResponsesAPI field, call setProviderContextMetadata(ctx,
config, schemas.ModelProvider(...)), then assert that
schemas.GetCustomProviderContextMetadata(ctx) returns metadata with
SupportsResponsesAPI == nil; use the existing context helpers and the
BifrostContextKey* keys to locate the behavior in setProviderContextMetadata and
ensure the nil (auto-detection) branch is covered.
In `@core/schemas/mux.go`:
- Around line 1104-1150: The two separate loops over brr.Input in
ChatFallbackWarnings that check for ResponsesMessageTypeReasoning and
ResponsesInputMessageRoleDeveloper should be merged into a single pass: iterate
once over brr.Input, track two booleans (e.g., foundReasoning and
foundDeveloper) and set them when you see input.Type ==
ResponsesMessageTypeReasoning and input.Role ==
ResponsesInputMessageRoleDeveloper respectively, append the corresponding
warnings when first observed, and exit the loop early once both flags are true
to preserve current short-circuit behavior.
- Around line 1075-1081: Replace the manual allocation+copy for deep-copying
brr.RawRequestBody with bytes.Clone to simplify and handle nil/zero-length
cases; update the block that sets bcr.RawRequestBody (referencing variables
brr.RawRequestBody and bcr.RawRequestBody) to use
bytes.Clone(brr.RawRequestBody) and add the "bytes" import to the file imports.
🪄 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: c030b4a9-563b-43d7-9005-a51918e476f5
📒 Files selected for processing (5)
core/bifrost.gocore/custom_provider_context_test.gocore/schemas/bifrost.gocore/schemas/mux.gotransports/config.schema.json
✅ Files skipped from review due to trivial changes (2)
- core/schemas/bifrost.go
- core/bifrost.go
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)
core/bifrost.go (1)
4059-4065:⚠️ Potential issue | 🟠 MajorGuard
reqbefore reading request fields.
RunStreamPreHooksnow dereferencesreqat Line 4064 before validating it. A nil request will panic instead of returning aBifrostError, unlike the other public request entrypoints.🐛 Proposed fix
func (bifrost *Bifrost) RunStreamPreHooks(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*WSStreamHooks, *schemas.BifrostError) { + if req == nil { + bifrostErr := newBifrostErrorFromMsg("stream hooks request is nil") + bifrostErr.ExtraFields.RequestType = schemas.ChatCompletionStreamRequest + return nil, bifrostErr + } if ctx == nil { ctx = bifrost.ctx }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4059 - 4065, RunStreamPreHooks currently dereferences req by calling req.GetRequestFields() before checking for nil; add an early nil check for req at the start of RunStreamPreHooks and return a suitable *schemas.BifrostError (same shape used by other public entrypoints) if req is nil, before calling req.GetRequestFields() or bifrost.setProviderContextMetadataForKey; this keeps behavior consistent with other handlers and prevents a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 5763-5808: The code currently leaves fallbackKeyProvider set to
the original keyProvider whenever
providerRequiresKey(config.CustomProviderConfig) is true but
bifrost.selectKeyFromProviderForModelWithPool returns an error or an empty list,
which causes Chat fallbacks to incorrectly reuse a Responses-only key policy;
change the logic in the bifrost.selectKey... branch so that if
fallbackKeyPoolErr != nil or len(fallbackSupportedKeys) == 0 you replace
fallbackKeyProvider with an explicit failing provider (a function matching the
signature used elsewhere that returns an error like "no fallback keys available
for provider <provider.GetProviderKey()>") rather than keeping the original
keyProvider; this ensures executeRequestWithRetries (used in both the
IsStreamRequestType branch and the non-stream branch where fallbackKeyProvider
is passed) cannot accidentally select the original provider's keys when the
fallback pool is unavailable.
In `@core/schemas/mux.go`:
- Around line 1208-1211: The substring loop in the retry policy (looping over
policy.UnsupportedErrorSubstrings in core/schemas/mux.go) must skip empty
markers because strings.Contains(message, "") is always true; before calling
strings.Contains, trim the unsupportedMarker (e.g., strings.TrimSpace) and if
the result is empty continue the loop so only non-empty markers are matched
against message.
---
Outside diff comments:
In `@core/bifrost.go`:
- Around line 4059-4065: RunStreamPreHooks currently dereferences req by calling
req.GetRequestFields() before checking for nil; add an early nil check for req
at the start of RunStreamPreHooks and return a suitable *schemas.BifrostError
(same shape used by other public entrypoints) if req is nil, before calling
req.GetRequestFields() or bifrost.setProviderContextMetadataForKey; this keeps
behavior consistent with other handlers and prevents a panic.
🪄 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: 4e572e70-9cf6-4c16-af54-ae4544e7920f
📒 Files selected for processing (3)
core/bifrost.gocore/schemas/mux.goplugins/compat/responsestochat.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/schemas/mux.go (2)
1191-1219: Consider normalizing customUnsupportedErrorSubstringsentries inShouldRetry.
DefaultResponsesToChatCompletionRetryPolicypre-lowercases its markers, so matching against the lowercasedmessageworks. However,ResponsesToChatCompletionRetryPolicyis exported and users can populateUnsupportedErrorSubstringsthemselves — any non-lowercased or whitespace-padded marker (e.g.,"HTML response"or" not implemented ") will silently fail to match despite looking correct. A cheap trim+lower inside the loop makes the public contract forgiving and matches what the default policy already does.Proposed change
message := strings.ToLower(strings.TrimSpace(err.Error.Message)) for _, unsupportedMarker := range policy.UnsupportedErrorSubstrings { - if unsupportedMarker == "" { + unsupportedMarker = strings.ToLower(strings.TrimSpace(unsupportedMarker)) + if unsupportedMarker == "" { continue } if strings.Contains(message, unsupportedMarker) { return true } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 1191 - 1219, The ShouldRetry method on ResponsesToChatCompletionRetryPolicy should normalize each entry of UnsupportedErrorSubstrings before matching: inside the loop over policy.UnsupportedErrorSubstrings (in ShouldRetry) trim whitespace and lowercase the unsupportedMarker, skip if empty, then check strings.Contains(message, normalizedMarker); this mirrors DefaultResponsesToChatCompletionRetryPolicy and makes user-supplied markers like " HTML response " or "Not Implemented" match correctly.
1098-1161:ChatFallbackWarningsre-invokesConvertViaJSONand re-walks inputs — consider consolidating.Two small inefficiencies worth considering for a follow-up:
ToChatRequestalready callsConvertViaJSON[interface{}](brr.Params.Text.Format)(Line 1039) to populateResponseFormat.ChatFallbackWarnings(Line 1155) runs the exact same conversion a second time purely to detect failure. If you return the warnings (or a single conversion error) fromToChatFallbackRequest— or cache the result on the fallback request — you avoid the duplicate marshal/unmarshal on every fallback.brr.Inputis walked twice (Lines 1114-1119 and 1121-1126) to independently detect reasoning items and developer roles. A single pass with two booleans keeps the warnings in stable order and halves the iteration.Neither is hot-path critical (fallback is already the exceptional path), so treat as nice-to-have.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 1098 - 1161, ChatFallbackWarnings redundantly re-runs ConvertViaJSON and walks brr.Input again; update ToChatRequest/ToChatFallbackRequest and ChatFallbackWarnings to avoid duplicate work by having ToChatRequest (or ToChatFallbackRequest) perform ConvertViaJSON[interface{}](brr.Params.Text.Format) once and return/cache the conversion error or the converted ResponseFormat, then have ChatFallbackWarnings read that cached result instead of re-invoking ConvertViaJSON, and merge the two loops over brr.Input into a single pass that sets booleans for "hasReasoning" and "hasDeveloperRole" to append warnings in stable order (refer to methods ToChatRequest/ToChatFallbackRequest, ChatFallbackWarnings, ConvertViaJSON, and the brr.Input iteration).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/schemas/mux.go`:
- Around line 1191-1219: The ShouldRetry method on
ResponsesToChatCompletionRetryPolicy should normalize each entry of
UnsupportedErrorSubstrings before matching: inside the loop over
policy.UnsupportedErrorSubstrings (in ShouldRetry) trim whitespace and lowercase
the unsupportedMarker, skip if empty, then check strings.Contains(message,
normalizedMarker); this mirrors DefaultResponsesToChatCompletionRetryPolicy and
makes user-supplied markers like " HTML response " or "Not Implemented" match
correctly.
- Around line 1098-1161: ChatFallbackWarnings redundantly re-runs ConvertViaJSON
and walks brr.Input again; update ToChatRequest/ToChatFallbackRequest and
ChatFallbackWarnings to avoid duplicate work by having ToChatRequest (or
ToChatFallbackRequest) perform
ConvertViaJSON[interface{}](brr.Params.Text.Format) once and return/cache the
conversion error or the converted ResponseFormat, then have ChatFallbackWarnings
read that cached result instead of re-invoking ConvertViaJSON, and merge the two
loops over brr.Input into a single pass that sets booleans for "hasReasoning"
and "hasDeveloperRole" to append warnings in stable order (refer to methods
ToChatRequest/ToChatFallbackRequest, ChatFallbackWarnings, ConvertViaJSON, and
the brr.Input iteration).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 016e04dc-95ed-4e08-a0cf-062a34246c09
📒 Files selected for processing (2)
core/bifrost.gocore/schemas/mux.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/bifrost.go
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)
core/bifrost.go (1)
4124-4130:⚠️ Potential issue | 🟠 MajorGuard
RunStreamPreHooksagainst a nil request before reading fields.Line 4129 now calls
req.GetRequestFields()before any nil check. SinceRunStreamPreHooksis exported andRunRealtimeTurnPreHooksalready handles nil requests defensively, return aBifrostErrorhere instead of risking a panic.🐛 Proposed fix
func (bifrost *Bifrost) RunStreamPreHooks(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*WSStreamHooks, *schemas.BifrostError) { + if req == nil { + bifrostErr := newBifrostErrorFromMsg("stream hook request is nil") + return nil, bifrostErr + } if ctx == nil { ctx = bifrost.ctx } provider, model, _ := req.GetRequestFields()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4124 - 4130, RunStreamPreHooks calls req.GetRequestFields() without checking req for nil, which can panic; add a nil-guard at the start of RunStreamPreHooks (similar to RunRealtimeTurnPreHooks) that returns an appropriate *schemas.BifrostError when req is nil, before calling req.GetRequestFields(), and ensure the function still uses initProviderContextMetadata and returns (*WSStreamHooks, *schemas.BifrostError) consistently.
♻️ Duplicate comments (1)
core/bifrost.go (1)
4163-4212:⚠️ Potential issue | 🟠 MajorRefresh provider metadata after prehook routing changes.
initProviderContextMetadataruns beforeRunLLMPreHooks, but these short-circuit/WS paths can run post-hooks without reaching the worker-level metadata refresh at Line 5509. If a prehook callsUpdateProvider,BifrostContextKeyCustomProviderMetadatacan stay pinned to the original provider for compat decisions and observability. Refresh frompreReq.GetRequestFields()immediately after prehooks whenpreReq != nil, before handlingshortCircuit.Based on learnings: provider switching from a PreLLMHook plugin is performed by calling
BifrostRequest.UpdateProvider(provider), andgetProviderQueueuses the updated provider as the single source of truth.Also applies to: 4715-4728, 4941-4959
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 5816-5833: The code currently converts any nil fallbackKeyProvider
into an error-returning provider, breaking the valid keyless/SkipKeySelection
path for local OpenAI-compatible providers; change the logic so that nil
fallbackKeyProvider is only turned into an error provider when
fallbackKeyPoolErr is non-nil (i.e. pool rebuild actually failed); if
fallbackKeyPoolErr is nil and buildKeyProviderFromPool returned nil, preserve
the nil value (which represents the keyless/SkipKeySelection fallback). Update
the block around selectKeyFromProviderForModelWithPool, buildKeyProviderFromPool
and the fallbackKeyProvider handling to only create the error wrapper when
fallbackKeyPoolErr != nil and keep a clarifying comment that nil ==
keyless/SkipKeySelection.
---
Outside diff comments:
In `@core/bifrost.go`:
- Around line 4124-4130: RunStreamPreHooks calls req.GetRequestFields() without
checking req for nil, which can panic; add a nil-guard at the start of
RunStreamPreHooks (similar to RunRealtimeTurnPreHooks) that returns an
appropriate *schemas.BifrostError when req is nil, before calling
req.GetRequestFields(), and ensure the function still uses
initProviderContextMetadata and returns (*WSStreamHooks, *schemas.BifrostError)
consistently.
🪄 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: b5870425-78b8-4813-b960-fe458277d786
📒 Files selected for processing (2)
core/bifrost.gocore/providers/openai/openai.go
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)
core/bifrost.go (1)
4175-4184:⚠️ Potential issue | 🟠 MajorReturn the post-hook result for recoverable short-circuit errors.
runPostLLMHooksApplyMetadataAndDraincan recover an error into a response, but this branch discardsrespand returns the originalshortCircuit.Error. That makesRunStreamPreHooksinconsistent withtryRequestand prevents post-hook recovery.🐛 Proposed fix
if shortCircuit.Error != nil { - _, bifrostErr := runPostLLMHooksApplyMetadataAndDrain(ctx, pipeline, nil, shortCircuit.Error, preCount) + resp, bifrostErr := runPostLLMHooksApplyMetadataAndDrain(ctx, pipeline, nil, shortCircuit.Error, preCount) if traceID, ok := ctx.Value(schemas.BifrostContextKeyTraceID).(string); ok && strings.TrimSpace(traceID) != "" { tracer.CompleteAndFlushTrace(strings.TrimSpace(traceID)) } cleanup() if bifrostErr != nil { return nil, bifrostErr } + if resp != nil { + return &WSStreamHooks{ + Cleanup: func() {}, + ShortCircuitResponse: resp, + }, nil + } return nil, shortCircuit.Error }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4175 - 4184, The short-circuit branch discards the response from runPostLLMHooksApplyMetadataAndDrain and always returns shortCircuit.Error; change the logic in the block handling shortCircuit.Error so you capture (resp, bifrostErr) := runPostLLMHooksApplyMetadataAndDrain(...), flush the tracer and call cleanup as before, then if bifrostErr != nil return nil, bifrostErr; else if resp != nil return resp, nil; otherwise return nil, shortCircuit.Error to preserve current behavior; reference symbols: shortCircuit, runPostLLMHooksApplyMetadataAndDrain, RunStreamPreHooks, tryRequest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 4129-4133: RunStreamPreHooks currently dereferences req via
req.GetRequestFields() before validating it; add the same nil-check and error
return used by RunRealtimeTurnPreHooks: if req == nil return a BifrostError
(matching the type/format used elsewhere) rather than proceeding. Update the
start of RunStreamPreHooks to guard req, then only call req.GetRequestFields()
and bifrost.initProviderContextMetadata(ctx, req, provider, model) after the
nil-check so the function mirrors RunRealtimeTurnPreHooks' safety behavior.
---
Outside diff comments:
In `@core/bifrost.go`:
- Around line 4175-4184: The short-circuit branch discards the response from
runPostLLMHooksApplyMetadataAndDrain and always returns shortCircuit.Error;
change the logic in the block handling shortCircuit.Error so you capture (resp,
bifrostErr) := runPostLLMHooksApplyMetadataAndDrain(...), flush the tracer and
call cleanup as before, then if bifrostErr != nil return nil, bifrostErr; else
if resp != nil return resp, nil; otherwise return nil, shortCircuit.Error to
preserve current behavior; reference symbols: shortCircuit,
runPostLLMHooksApplyMetadataAndDrain, RunStreamPreHooks, tryRequest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/bifrost.go (1)
4685-4747:⚠️ Potential issue | 🟠 MajorResolve the provider queue after pre-hook routing changes.
pqis selected beforeRunLLMPreHooks, but pre-hooks can rewrite the provider viaUpdateProvider. The rewrittenpreReqis then enqueued on the original provider’s worker queue, so provider routing and custom-provider metadata can diverge from the request that is actually executed. Move queue lookup until afterpreReqis finalized, and refresh provider metadata for the updated request without clearing compat state.Fix pattern
- provider, model, _ := req.GetRequestFields() - pq, err := bifrost.getProviderQueue(provider) - if err != nil { - bifrostErr := newBifrostError(err) - bifrostErr.PopulateExtraFields(req.RequestType, provider, model, model) - return nil, bifrostErr - } + provider, model, _ := req.GetRequestFields() if bifrostErr := bifrost.initProviderContextMetadata(ctx, req, provider, model); bifrostErr != nil { return nil, bifrostErr } resetCompatState(ctx) @@ if preReq == nil { bifrostErr := newBifrostErrorFromMsg("bifrost request after plugin hooks cannot be nil") bifrostErr.PopulateExtraFields(req.RequestType, provider, model, model) return nil, bifrostErr } + + provider, model, _ = preReq.GetRequestFields() + if bifrostErr := bifrost.initProviderContextMetadata(ctx, preReq, provider, model); bifrostErr != nil { + return nil, bifrostErr + } + pq, err := bifrost.getProviderQueue(provider) + if err != nil { + bifrostErr := newBifrostError(err) + bifrostErr.PopulateExtraFields(preReq.RequestType, provider, model, model) + return nil, bifrostErr + } msg := bifrost.getChannelMessage(*preReq)Apply the same ordering in
tryStreamRequest. Based on learnings: provider switching from a PreLLMHook plugin is performed by callingBifrostRequest.UpdateProvider(provider), andgetProviderQueuereads the updated provider viapreReq.GetRequestFields().Also applies to: 4895-5044
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4685 - 4747, The provider queue (getProviderQueue) is being fetched into pq before plugins run, but RunLLMPreHooks can change the provider via BifrostRequest.UpdateProvider, so move the getProviderQueue call to after preReq is finalized (after RunLLMPreHooks and the nil/short-circuit checks) and re-run initProviderContextMetadata/resetCompatState appropriately for the updated preReq without clearing compatibility state; apply the same ordering/fix in tryStreamRequest as well so the enqueued request uses the updated provider and metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 4087-4090: The post-LLM hook flow currently only applies
ResponsesToChatCompletionFallback metadata after plugins run, so observability
hooks may miss it; update runPostLLMHooksAndApplyMetadata to call
schemas.ApplyResponsesToChatCompletionFallbackMetadata on the current
result/bifrostErr before invoking pipeline.RunPostLLMHooks (to ensure hooks see
the fallback flags) and then keep the existing
ApplyResponsesToChatCompletionFallbackMetadata call after RunPostLLMHooks to
handle plugins that replace the response/error; refer to the
runPostLLMHooksAndApplyMetadata function, pipeline.RunPostLLMHooks, and
schemas.ApplyResponsesToChatCompletionFallbackMetadata to locate and implement
the two applications of the metadata.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 4685-4747: The provider queue (getProviderQueue) is being fetched
into pq before plugins run, but RunLLMPreHooks can change the provider via
BifrostRequest.UpdateProvider, so move the getProviderQueue call to after preReq
is finalized (after RunLLMPreHooks and the nil/short-circuit checks) and re-run
initProviderContextMetadata/resetCompatState appropriately for the updated
preReq without clearing compatibility state; apply the same ordering/fix in
tryStreamRequest as well so the enqueued request uses the updated provider and
metadata.
🪄 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: 4a591c48-8be2-461f-879b-2dceae921515
📒 Files selected for processing (2)
core/bifrost.goplugins/compat/responsestochat.go
- move fallback into unified execution flow (single executeRequestWithRetries) - remove provider-layer fallback logic (ShouldBypassOperationCheck → generic BifrostContextKeySkipOperationCheck set by compat plugin/core, not provider) - tighten retry/fallback conditions (remove ErrProviderResponseUnmarshal and ErrProviderResponseEmpty — only HTTP 404/405/410/501 + HTML response) - fix model consistency: streaming fallback uses state.OriginalRequestType for ExtraFields so observability reflects caller's original intent - fix streaming fallback metadata: apply ApplyResponsesToChatCompletionFallbackMetadata per chunk so fallback marker appears on all streamed responses - remove unused config param from tryResponsesToChatCompletionFallback - fix provider test for upstream postHookSpanFinalizer signature change - address reviewer feedback and edge cases Signed-off-by: Vaibhav mittal <vaibhavmittal929@gmail.com>
Vaibhav701161
force-pushed
the
fix/responses-chat-fallback
branch
from
c2c4d49 to
a1897c2
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (7)
core/bifrost.go (4)
4098-4104:⚠️ Potential issue | 🟠 MajorGuard
reqbefore reading request fields.
RunStreamPreHooksdereferencesreqon Line 4103 before any validation, so a nil caller panics instead of returning a structuredBifrostErrorlikeRunRealtimeTurnPreHooksdoes.🐛 Suggested fix
func (bifrost *Bifrost) RunStreamPreHooks(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*WSStreamHooks, *schemas.BifrostError) { + if req == nil { + bifrostErr := newBifrostErrorFromMsg("stream hooks request is nil") + return nil, bifrostErr + } if ctx == nil { ctx = bifrost.ctx }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4098 - 4104, RunStreamPreHooks currently dereferences req by calling req.GetRequestFields() without validating req; add a nil guard at the top of RunStreamPreHooks (similar to RunRealtimeTurnPreHooks) that returns (nil, <appropriate *schemas.BifrostError>) when req is nil, so the method never panics; place the check before calling req.GetRequestFields(), and ensure the returned error uses the same creation/formatting convention used elsewhere in Bifrost (so callers get a structured BifrostError instead of a panic); keep the rest of the logic (including calls to setProviderContextMetadataForKey and GetRequestFields) unchanged after the nil-check.
4715-4752:⚠️ Potential issue | 🟠 MajorRefresh provider metadata after prehooks rewrite routing.
setProviderContextMetadataForKey(ctx, provider)runs beforeRunLLMPreHooks, but prehooks can legitimately callBifrostRequest.UpdateProvider(...). That leavesBifrostContextKeyCustomProviderMetadata/SupportsResponsesAPIstale for later hooks in the same pipeline. The same stale-metadata pattern is repeated intryStreamRequestandRunStreamPreHooks; recompute it frompreReq.GetRequestFields()once prehooks finish successfully. Based on learnings: provider switching from a PreLLMHook plugin is performed by callingBifrostRequest.UpdateProvider(provider), and downstream routing reads the updated provider frompreReq.GetRequestFields().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4715 - 4752, Prehooks can call BifrostRequest.UpdateProvider(...) which changes routing, but setProviderContextMetadataForKey(ctx, provider) is currently called only before pipeline.RunLLMPreHooks, leaving BifrostContextKeyCustomProviderMetadata and SupportsResponsesAPI stale; after calling pipeline.RunLLMPreHooks(ctx, req) (where you receive preReq and preCount), recompute and reset provider context metadata by calling setProviderContextMetadataForKey with the provider value taken from preReq.GetRequestFields().Provider (or equivalent field) before continuing (and mirror the same fix in tryStreamRequest and RunStreamPreHooks paths) so downstream hooks and handlers see the updated provider metadata.
5908-5946:⚠️ Potential issue | 🟠 MajorConvert runtime fallback stream chunks back to Responses chunks.
This branch executes
ChatCompletionStreamRequest, but the runner shown here only tags fallback metadata.ResponsesStreamRequestcallers can still receive chat-style chunks unless this stream is wrapped with the existing responses conversion path, e.g.wrapConvertedStreamPostHookRunner(..., schemas.ResponsesRequest).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5908 - 5946, The postHookRunner passed into bifrost.handleProviderStreamRequest only tags fallback metadata and doesn't convert chat-style streaming chunks back to Responses chunks for ResponsesStreamRequest; wrap the runner with the existing conversion helper (e.g. wrapConvertedStreamPostHookRunner) when the stream's request type is a Responses request so callers receive Responses-style chunks. Locate the call that invokes bifrost.handleProviderStreamRequest (where postHookRunner and postHookSpanFinalizer are passed) and replace/post-wrap postHookRunner with wrapConvertedStreamPostHookRunner(postHookRunner, schemas.ResponsesRequest) (or the equivalent conversion call) when state.FallbackRequest.RequestType == schemas.ResponsesRequest (or when handling ResponsesStreamRequest), ensuring the wrapped runner is used everywhere that branch handles Responses streams.
5890-5958:⚠️ Potential issue | 🟠 MajorRebuild key selection for the Chat fallback request type.
The runtime fallback still passes the original
keyProviderintoexecuteRequestWithRetries. If a key is allowed forResponses*but not forChatCompletion*, this branch can retry with the wrong pool and miss valid chat-capable keys. Recreate the fallback key provider fromstate.FallbackRequest.RequestType, while preserving the nil/keyless path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5890 - 5958, The fallback path currently reuses the original keyProvider when calling executeRequestWithRetries, which can select keys incompatible with state.FallbackRequest.RequestType; rebuild a fallback-specific key provider (e.g., fallbackKeyProvider) using the same logic used to construct the original keyProvider but keyed off state.FallbackRequest.RequestType and preserving the nil/keyless behavior, then pass that fallbackKeyProvider into executeRequestWithRetries (both streaming and non-streaming branches) instead of keyProvider; update references around executeRequestWithRetries, state.FallbackRequest.RequestType, keyProvider, and lastAttemptFinalizer to use the new fallbackKeyProvider.plugins/compat/runtime_fallback_integration_test.go (2)
89-92:⚠️ Potential issue | 🟡 MinorStale fatal message references
InitWithModelCatalog.The call is
Init(Config{}, logger, nil), but the failure message still saysInitWithModelCatalog. A previous review flagged this and it was marked addressed, but the message is back/unchanged.- t.Fatalf("InitWithModelCatalog returned error: %v", err) + t.Fatalf("compat.Init returned error: %v", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/runtime_fallback_integration_test.go` around lines 89 - 92, The test's fatal message incorrectly references InitWithModelCatalog while calling Init(Config{}, logger, nil); update the t.Fatalf call in runtime_fallback_integration_test.go to reference the actual function Init (e.g., t.Fatalf("Init returned error: %v", err)) so the error message matches the invoked symbol; ensure you change the message text where plugin, err := Init(Config{}, logger, nil) is checked.
184-204:⚠️ Potential issue | 🟡 Minor
t.Fatalf/t.Fatalfrom inside the httptest handler goroutine — recurring at lines 187, 260, 311, and 324.A previous review flagged this and it was marked resolved, but
t.Fatalf(and thehttp.Flushert.Fatalat line 324) are still being called from the server's handler goroutine.t.Fatal*invokesruntime.Goexit, which Go's testing docs require be called from the test goroutine; here it would only stop the handler, leaving the test waiting on its assertions/timeouts and producing confusing failures. Replace witht.Errorf(...)+ write an HTTP error status +return.Proposed fix pattern (apply at all four sites)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - bodyBytes, err := io.ReadAll(r.Body) - if err != nil { - t.Fatalf("failed to read request body: %v", err) - } + bodyBytes, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("failed to read request body: %v", err) + w.WriteHeader(http.StatusInternalServerError) + return + }- flusher, ok := w.(http.Flusher) - if !ok { - t.Fatal("response writer does not implement http.Flusher") - } + flusher, ok := w.(http.Flusher) + if !ok { + t.Errorf("response writer does not implement http.Flusher") + w.WriteHeader(http.StatusInternalServerError) + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/runtime_fallback_integration_test.go` around lines 184 - 204, The test's httptest.NewServer handler is calling t.Fatalf/t.Fatal (seen where request bodies are read and where http.Flusher is used), which runs in the handler goroutine and must be changed: replace each t.Fatalf/t.Fatal with t.Errorf(...) to report the error, write an appropriate HTTP error status (e.g., http.StatusInternalServerError) to the ResponseWriter, and return from the handler so the goroutine exits; update all occurrences related to reading the request body and the http.Flusher path (the code paths that touch responsesHits, chatHits, responsesRequestBody, chatRequestBody and the integrationChatCompletionBody response) using this pattern.core/providers/openai/responses_fallback_test.go (1)
218-221:⚠️ Potential issue | 🟡 Minor
t.Fatalfinside httptest handler goroutines is unsafe.
t.Fatalfcallsruntime.Goexit, which per Go's testing docs must run on the goroutine that called the test function. The httptest handler runs in a separate server goroutine, so a read failure here would only kill the handler — the test would keep running and likely fail later with a misleading assertion. Same pattern at line 338. Uset.Errorf+ write an error status +returninstead.Proposed fix
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { bodyBytes, err := io.ReadAll(r.Body) if err != nil { - t.Fatalf("failed to read request body: %v", err) + t.Errorf("failed to read request body: %v", err) + w.WriteHeader(http.StatusInternalServerError) + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responses_fallback_test.go` around lines 218 - 221, The test handler is calling t.Fatalf inside the httptest server goroutine (at the io.ReadAll(r.Body) error check and the similar check around line 338), which is unsafe; change those t.Fatalf calls to t.Errorf (or t.Logf+t.Errorf), write an HTTP 500 response to the ResponseWriter (e.g., w.WriteHeader(http.StatusInternalServerError)), and return immediately from the handler to stop processing, so the test goroutine (the one running the test) remains the one to fail and the handler exits cleanly; update both occurrences that check "bodyBytes, err := io.ReadAll(r.Body)" (and the other analogous error check) accordingly.
🧹 Nitpick comments (3)
core/schemas/mux_test.go (2)
338-340: Optional: drop thecontainsWarninghelper.The helper is a one-line wrapper that doesn't add semantic value; using
strings.Contains(joined, ...)directly at the call sites (Lines 324, 327, 330, 333) keeps the test self-contained without an indirection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux_test.go` around lines 338 - 340, Remove the one-line helper containsWarning and replace its call sites with direct calls to strings.Contains(joined, "<fragment>") so the tests are self-contained; specifically delete the containsWarning function and update places that call containsWarning(joined, fragment) to use strings.Contains(joined, fragment) instead.
317-321: Tighten the warning count assertion.The test sets up four feature drops (developer role, non-function tools, tool_choice type, and truncation) and asserts presence of all four substrings below, but the count check uses
< 3. Consider assertinglen(warnings) >= 4(or== 4) so a regression that silently coalesces or drops one warning is caught even if the merged string still contains the substrings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux_test.go` around lines 317 - 321, The test currently verifies multiple compatibility warnings by calling req.ChatFallbackWarnings() and checking if len(warnings) < 3; update this to assert the expected number of warnings for the four feature drops (developer role, non-function tools, tool_choice type, and truncation) by changing the check to len(warnings) >= 4 or preferably len(warnings) == 4 and adjust the t.Fatalf message accordingly so the test fails if any warning is dropped or coalesced; locate the assertion around the variable warnings returned from req.ChatFallbackWarnings().plugins/compat/responsestochat_test.go (1)
41-199: Tests cover the three states cleanly.Forced-unsupported, runtime-eligible, and streaming-forced paths are all asserted with state inspection (
Active,RetryEligible,IsStreaming,RetryPolicy,FallbackRequest) and context markers. Thestate.ShouldRetry(&schemas.BifrostError{StatusCode: &statusCode})assertion at line 169 is a nice end-to-end check on the prepared retry policy.One optional addition: there's no unit test for
transformResponsesToChatError(the error annotation path). Consider adding a forced-fallback PostLLMHook case where the upstream returns an error, to lock inExtraFields.RequestTypeand the fallback marker on errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/responsestochat_test.go` around lines 41 - 199, Add a unit test that exercises the error-annotation path by invoking CompatPlugin.PostLLMHook with a forced responses-to-chat fallback active (set via PreLLMHook or by setting the compat state on ctx) and supplying a non-nil bifrostErr to trigger transformResponsesToChatError; assert that the returned result contains a ResponsesResponse with ExtraFields.RequestType == schemas.ResponsesRequest, ExtraFields.ResponsesToChatCompletionFallback == true, and that the responses content/error mapping reflects the original bifrostErr (referencing transformResponsesToChatError, PostLLMHook, ExtraFields.RequestType, and the ResponsesToChatCompletionFallback marker to locate code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/bifrost.go`:
- Around line 4098-4104: RunStreamPreHooks currently dereferences req by calling
req.GetRequestFields() without validating req; add a nil guard at the top of
RunStreamPreHooks (similar to RunRealtimeTurnPreHooks) that returns (nil,
<appropriate *schemas.BifrostError>) when req is nil, so the method never
panics; place the check before calling req.GetRequestFields(), and ensure the
returned error uses the same creation/formatting convention used elsewhere in
Bifrost (so callers get a structured BifrostError instead of a panic); keep the
rest of the logic (including calls to setProviderContextMetadataForKey and
GetRequestFields) unchanged after the nil-check.
- Around line 4715-4752: Prehooks can call BifrostRequest.UpdateProvider(...)
which changes routing, but setProviderContextMetadataForKey(ctx, provider) is
currently called only before pipeline.RunLLMPreHooks, leaving
BifrostContextKeyCustomProviderMetadata and SupportsResponsesAPI stale; after
calling pipeline.RunLLMPreHooks(ctx, req) (where you receive preReq and
preCount), recompute and reset provider context metadata by calling
setProviderContextMetadataForKey with the provider value taken from
preReq.GetRequestFields().Provider (or equivalent field) before continuing (and
mirror the same fix in tryStreamRequest and RunStreamPreHooks paths) so
downstream hooks and handlers see the updated provider metadata.
- Around line 5908-5946: The postHookRunner passed into
bifrost.handleProviderStreamRequest only tags fallback metadata and doesn't
convert chat-style streaming chunks back to Responses chunks for
ResponsesStreamRequest; wrap the runner with the existing conversion helper
(e.g. wrapConvertedStreamPostHookRunner) when the stream's request type is a
Responses request so callers receive Responses-style chunks. Locate the call
that invokes bifrost.handleProviderStreamRequest (where postHookRunner and
postHookSpanFinalizer are passed) and replace/post-wrap postHookRunner with
wrapConvertedStreamPostHookRunner(postHookRunner, schemas.ResponsesRequest) (or
the equivalent conversion call) when state.FallbackRequest.RequestType ==
schemas.ResponsesRequest (or when handling ResponsesStreamRequest), ensuring the
wrapped runner is used everywhere that branch handles Responses streams.
- Around line 5890-5958: The fallback path currently reuses the original
keyProvider when calling executeRequestWithRetries, which can select keys
incompatible with state.FallbackRequest.RequestType; rebuild a fallback-specific
key provider (e.g., fallbackKeyProvider) using the same logic used to construct
the original keyProvider but keyed off state.FallbackRequest.RequestType and
preserving the nil/keyless behavior, then pass that fallbackKeyProvider into
executeRequestWithRetries (both streaming and non-streaming branches) instead of
keyProvider; update references around executeRequestWithRetries,
state.FallbackRequest.RequestType, keyProvider, and lastAttemptFinalizer to use
the new fallbackKeyProvider.
In `@core/providers/openai/responses_fallback_test.go`:
- Around line 218-221: The test handler is calling t.Fatalf inside the httptest
server goroutine (at the io.ReadAll(r.Body) error check and the similar check
around line 338), which is unsafe; change those t.Fatalf calls to t.Errorf (or
t.Logf+t.Errorf), write an HTTP 500 response to the ResponseWriter (e.g.,
w.WriteHeader(http.StatusInternalServerError)), and return immediately from the
handler to stop processing, so the test goroutine (the one running the test)
remains the one to fail and the handler exits cleanly; update both occurrences
that check "bodyBytes, err := io.ReadAll(r.Body)" (and the other analogous error
check) accordingly.
In `@plugins/compat/runtime_fallback_integration_test.go`:
- Around line 89-92: The test's fatal message incorrectly references
InitWithModelCatalog while calling Init(Config{}, logger, nil); update the
t.Fatalf call in runtime_fallback_integration_test.go to reference the actual
function Init (e.g., t.Fatalf("Init returned error: %v", err)) so the error
message matches the invoked symbol; ensure you change the message text where
plugin, err := Init(Config{}, logger, nil) is checked.
- Around line 184-204: The test's httptest.NewServer handler is calling
t.Fatalf/t.Fatal (seen where request bodies are read and where http.Flusher is
used), which runs in the handler goroutine and must be changed: replace each
t.Fatalf/t.Fatal with t.Errorf(...) to report the error, write an appropriate
HTTP error status (e.g., http.StatusInternalServerError) to the ResponseWriter,
and return from the handler so the goroutine exits; update all occurrences
related to reading the request body and the http.Flusher path (the code paths
that touch responsesHits, chatHits, responsesRequestBody, chatRequestBody and
the integrationChatCompletionBody response) using this pattern.
---
Nitpick comments:
In `@core/schemas/mux_test.go`:
- Around line 338-340: Remove the one-line helper containsWarning and replace
its call sites with direct calls to strings.Contains(joined, "<fragment>") so
the tests are self-contained; specifically delete the containsWarning function
and update places that call containsWarning(joined, fragment) to use
strings.Contains(joined, fragment) instead.
- Around line 317-321: The test currently verifies multiple compatibility
warnings by calling req.ChatFallbackWarnings() and checking if len(warnings) <
3; update this to assert the expected number of warnings for the four feature
drops (developer role, non-function tools, tool_choice type, and truncation) by
changing the check to len(warnings) >= 4 or preferably len(warnings) == 4 and
adjust the t.Fatalf message accordingly so the test fails if any warning is
dropped or coalesced; locate the assertion around the variable warnings returned
from req.ChatFallbackWarnings().
In `@plugins/compat/responsestochat_test.go`:
- Around line 41-199: Add a unit test that exercises the error-annotation path
by invoking CompatPlugin.PostLLMHook with a forced responses-to-chat fallback
active (set via PreLLMHook or by setting the compat state on ctx) and supplying
a non-nil bifrostErr to trigger transformResponsesToChatError; assert that the
returned result contains a ResponsesResponse with ExtraFields.RequestType ==
schemas.ResponsesRequest, ExtraFields.ResponsesToChatCompletionFallback == true,
and that the responses content/error mapping reflects the original bifrostErr
(referencing transformResponsesToChatError, PostLLMHook,
ExtraFields.RequestType, and the ResponsesToChatCompletionFallback marker to
locate code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a88f929-6d88-43dd-8c08-974586cf401d
📒 Files selected for processing (14)
core/bifrost.gocore/custom_provider_context_test.gocore/providers/openai/openai.gocore/providers/openai/responses_fallback_test.gocore/providers/utils/utils.gocore/schemas/bifrost.gocore/schemas/mux.gocore/schemas/mux_test.gocore/schemas/provider.goplugins/compat/main.goplugins/compat/responsestochat.goplugins/compat/responsestochat_test.goplugins/compat/runtime_fallback_integration_test.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (3)
- core/schemas/provider.go
- transports/config.schema.json
- core/providers/utils/utils.go
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 adds a robust fallback mechanism for custom OpenAI-compatible providers that do not support the
/v1/responsesAPI (e.g., LMStudio, local proxies, partial implementations).Currently, Bifrost assumes Responses API support when
base_provider_type: openaiis used. This causes failures when upstream providers only support/v1/chat/completions.This change ensures that:
/v1/responsesChanges
Introduced tri-state configuration:
supports_responses_api: true→ always use/v1/responsesfalse→ always fallback to/v1/chat/completionsnil→ attempt/v1/responses, fallback on failureImplemented fallback logic in:
OpenAIProvider.Responses()OpenAIProvider.ResponsesStream()Added error-based retry mechanism:
Fallback triggers on:
Reused existing conversion utilities:
ToChatRequest()ToBifrostResponsesResponse()Added context flag to indicate fallback path for observability
Added regression tests:
Design Decisions & Trade-offs
Type of change
Affected areas
How to test
1. Start LMStudio (or any OpenAI-compatible server)
Expected:
2. Run Bifrost
cd transports/bifrost-http go run main.go -app-dir ../ -port 8080 -log-level debug3. Anthropic request (fallback path)
Expected:
4. Streaming test
Expected:
content_block_delta) received correctly5. Direct OpenAI route
Expected:
6. Observability verification
Expected:
Entries for:
responseschat_completionresponses_streamSuccessful status and provider routing
Screenshots / Recordings
Attached screenshots demonstrate:
Breaking changes
Related issues
#1321
Security considerations
Checklist