feat: litellmcompat chat to responses#2050
Closed
sammaji wants to merge 1 commit intographite-base/2050from
Closed
feat: litellmcompat chat to responses#2050sammaji wants to merge 1 commit intographite-base/2050from
sammaji wants to merge 1 commit intographite-base/2050from
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughRoutes ChatCompletion requests to Responses when model/catalog indicates by adding a context flag, routing and stream wrappers, Responses↔Chat converters, a supported-outputs index and accessors in the model catalog, LiteLLMCompat request/response transforms, and plugin/transport wiring for ModelCatalog. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Core as Bifrost Core
participant ModelCat as ModelCatalog
participant Provider as Provider (Responses)
participant PostHook as Post-Hook Runner
participant Plugin as LiteLLMCompat
Client->>Core: ChatCompletionRequest / ChatCompletionStreamRequest
Core->>ModelCat: IsChatCompletionSupported(model)?
ModelCat-->>Core: false
Core->>ModelCat: IsResponsesSupported(model)?
ModelCat-->>Core: true
Core->>Core: set BifrostContextKeyShouldConvertChatToResponses
Core->>Provider: Responses / ResponsesStream (converted request)
Provider-->>Core: Responses (streaming or non-stream)
alt streaming
Core->>PostHook: wrapChatToResponsesStreamPostHookRunner(result)
PostHook->>Plugin: transformChatToResponsesResponse / transformChatToResponsesError
Plugin-->>PostHook: restored Chat metadata
PostHook-->>Core: ChatResponse chunks
else non-stream
Core->>Plugin: transformChatToResponsesResponse(result)
Plugin-->>Core: ChatResponse
end
Core-->>Client: ChatResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/modelcatalog/main.go`:
- Around line 54-56: The supportedOutputs index is currently keyed only by model
which causes provider-specific models to collide; update
buildSupportedOutputsIndex to key by a composite of provider+normalizedModel (or
use the same provider-aware normalization used by
GetProvidersForModel/RefineModelForProvider), and update
IsChatCompletionSupported and IsResponsesSupported to look up supportedOutputs
using that composite key (or perform the same provider-aware normalization
before lookup) so provider-prefixed entries (e.g., openai/..., openrouter/...,
vertex/...) are handled correctly and capabilities do not leak across providers.
🪄 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: 7ab1c125-73df-4d97-b7e8-12954a98c1c1
📒 Files selected for processing (9)
core/bifrost.gocore/schemas/bifrost.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.goplugins/litellmcompat/chattoresponses.goplugins/litellmcompat/main.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.go
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
1bc1946 to
3db6c20
Compare
sammaji
force-pushed
the
03-13-feat_litellmcompat_chat_to_responses
branch
from
c1aca52 to
4d44142
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 4960-4971: The stream post-hook wrapper
wrapChatToResponsesStreamPostHookRunner is checking the wrong field
(result.ResponsesResponse) so chat-stream payloads get skipped; change the check
to inspect result.StreamsResponse instead and, if non-nil, call its
ToBifrostChatResponse() and set result = &schemas.BifrostResponse{ChatResponse:
convertedResponse} before delegating to postHookRunner (preserving the same
behavior and return signature).
🪄 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: a0b9ca5a-eedc-48ac-b270-1f273285d358
📒 Files selected for processing (10)
core/bifrost.gocore/schemas/bifrost.gocore/utils.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.goplugins/litellmcompat/chattoresponses.goplugins/litellmcompat/main.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (5)
- transports/bifrost-http/server/plugins.go
- plugins/litellmcompat/main.go
- framework/modelcatalog/utils.go
- transports/bifrost-http/server/server.go
- plugins/litellmcompat/chattoresponses.go
sammaji
commented
Mar 13, 2026
sammaji
force-pushed
the
03-13-feat_litellmcompat_chat_to_responses
branch
from
4d44142 to
7f424b2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/bifrost.go (1)
4960-4971:⚠️ Potential issue | 🔴 CriticalStream wrapper is reading the wrong field and skips conversion.
Line 4962 checks
result.ResponsesResponseinside a stream post-hook path. For streaming, conversion input isresult.ResponsesStreamResponse, so chat-stream normalization is bypassed.🔧 Proposed fix
func wrapChatToResponsesStreamPostHookRunner(postHookRunner schemas.PostHookRunner) schemas.PostHookRunner { return func(ctx *schemas.BifrostContext, result *schemas.BifrostResponse, bifrostErr *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError) { - if result != nil && result.ResponsesResponse != nil { - if convertedResponse := result.ResponsesResponse.ToBifrostChatResponse(); convertedResponse != nil { + if result != nil && result.ResponsesStreamResponse != nil { + if convertedResponse := result.ResponsesStreamResponse.ToBifrostChatResponse(); convertedResponse != nil { result = &schemas.BifrostResponse{ ChatResponse: convertedResponse, } } } return postHookRunner(ctx, result, bifrostErr) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4960 - 4971, The stream post-hook wrapper wrapChatToResponsesStreamPostHookRunner is checking the non-stream field ResponsesResponse and therefore skips converting stream responses; update the function to check result.ResponsesStreamResponse instead (and call its ToBifrostChatResponse) and set result = &schemas.BifrostResponse{ChatResponse: convertedResponse} when conversion succeeds, then call postHookRunner(ctx, result, bifrostErr); ensure you remove or avoid using ResponsesResponse in this stream-specific wrapper so stream responses are normalized correctly.
🤖 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 1251-1252: The construction of BifrostChatResponse in
ToBifrostChatResponse dereferences responsesResp.ID without a nil guard; update
ToBifrostChatResponse to check responsesResp.ID for nil before dereferencing
(use a conditional to set ID to *responsesResp.ID when non-nil, otherwise set ID
to an empty string or appropriate zero value) so that dereferencing a nil
pointer cannot panic; ensure the change touches the BifrostChatResponse creation
and preserves existing behavior when ToBifrostResponsesResponse sets ID via
Ptr(cr.ID).
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 4960-4971: The stream post-hook wrapper
wrapChatToResponsesStreamPostHookRunner is checking the non-stream field
ResponsesResponse and therefore skips converting stream responses; update the
function to check result.ResponsesStreamResponse instead (and call its
ToBifrostChatResponse) and set result = &schemas.BifrostResponse{ChatResponse:
convertedResponse} when conversion succeeds, then call postHookRunner(ctx,
result, bifrostErr); ensure you remove or avoid using ResponsesResponse in this
stream-specific wrapper so stream responses are normalized correctly.
🪄 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: e2a163a6-30f4-43e1-9411-3828f1749865
📒 Files selected for processing (11)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/mux.gocore/utils.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.goplugins/litellmcompat/chattoresponses.goplugins/litellmcompat/main.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/utils.go
- core/schemas/bifrost.go
- transports/bifrost-http/server/server.go
sammaji
force-pushed
the
03-13-feat_litellmcompat_chat_to_responses
branch
from
7f424b2 to
42a09bc
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/server/plugins.go (1)
105-110:⚠️ Potential issue | 🟠 MajorGuard nil plugin config before dereferencing.
At Line 109,
*litellmConfigis dereferenced without a nil check. If the plugin config is omitted and marshal returns nil, plugin loading can panic.🛠️ Proposed fix
case litellmcompat.PluginName: litellmConfig, err := MarshalPluginConfig[litellmcompat.Config](pluginConfig) if err != nil { return nil, fmt.Errorf("failed to marshal litellmcompat plugin config: %w", err) } - return litellmcompat.InitWithModelCatalog(*litellmConfig, logger, bifrostConfig.ModelCatalog) + cfg := litellmcompat.Config{} + if litellmConfig != nil { + cfg = *litellmConfig + } + return litellmcompat.InitWithModelCatalog(cfg, logger, bifrostConfig.ModelCatalog)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/server/plugins.go` around lines 105 - 110, The code dereferences litellmConfig without checking for nil after calling MarshalPluginConfig[litellmcompat.Config](pluginConfig); add a nil guard: after err check, verify litellmConfig != nil and return a descriptive error if it is nil (e.g., "litellmcompat plugin config is nil"), then call litellmcompat.InitWithModelCatalog(*litellmConfig, logger, bifrostConfig.ModelCatalog); reference MarshalPluginConfig, the litellmConfig variable, and litellmcompat.InitWithModelCatalog to locate where to add the guard and error return.
♻️ Duplicate comments (1)
core/schemas/mux.go (1)
1251-1253:⚠️ Potential issue | 🟠 MajorAvoid nil dereference when mapping Responses ID to Chat ID.
At Line 1252,
responsesResp.IDis dereferenced without a nil guard, which can panic for responses constructed withoutID.🛡️ Proposed fix
chatResp := &BifrostChatResponse{ - ID: *responsesResp.ID, + ID: "", Created: responsesResp.CreatedAt, Object: "chat.completion", Model: responsesResp.Model, Citations: responsesResp.Citations, SearchResults: responsesResp.SearchResults, Videos: responsesResp.Videos, } + if responsesResp.ID != nil { + chatResp.ID = *responsesResp.ID + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 1251 - 1253, The code constructs a BifrostChatResponse and dereferences responsesResp.ID directly (in the chatResp ID assignment) which can panic if ID is nil; update the mapping in the block that builds BifrostChatResponse (where chatResp is created) to check responsesResp.ID for nil before dereferencing and assign a safe zero/empty value (or skip setting ID) when nil, ensuring no direct *responsesResp.ID dereference occurs without the nil guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@transports/bifrost-http/server/plugins.go`:
- Around line 105-110: The code dereferences litellmConfig without checking for
nil after calling MarshalPluginConfig[litellmcompat.Config](pluginConfig); add a
nil guard: after err check, verify litellmConfig != nil and return a descriptive
error if it is nil (e.g., "litellmcompat plugin config is nil"), then call
litellmcompat.InitWithModelCatalog(*litellmConfig, logger,
bifrostConfig.ModelCatalog); reference MarshalPluginConfig, the litellmConfig
variable, and litellmcompat.InitWithModelCatalog to locate where to add the
guard and error return.
---
Duplicate comments:
In `@core/schemas/mux.go`:
- Around line 1251-1253: The code constructs a BifrostChatResponse and
dereferences responsesResp.ID directly (in the chatResp ID assignment) which can
panic if ID is nil; update the mapping in the block that builds
BifrostChatResponse (where chatResp is created) to check responsesResp.ID for
nil before dereferencing and assign a safe zero/empty value (or skip setting ID)
when nil, ensuring no direct *responsesResp.ID dereference occurs without the
nil guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa015726-32e0-4a08-bc7c-cdc1a4cfd541
📒 Files selected for processing (11)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/mux.gocore/utils.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.goplugins/litellmcompat/chattoresponses.goplugins/litellmcompat/main.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.go
✅ Files skipped from review due to trivial changes (1)
- transports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/schemas/bifrost.go
- core/utils.go
- framework/modelcatalog/utils.go
- core/bifrost.go
sammaji
force-pushed
the
03-13-feat_litellmcompat_chat_to_responses
branch
from
42a09bc to
a0b8ad5
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/modelcatalog/main.go (1)
809-837: Consider logging malformed model-parameter payloads for easier debugging.
buildSupportedOutputsIndexcurrently drops JSON parse failures silently, which can hide why capability checks return false for specific models.💡 Suggested tweak
if err := json.Unmarshal(data, ¶ms); err != nil || len(params.SupportedEndpoints) == 0 { - continue + if err != nil { + mc.logger.Debug("skipping supported_endpoints parse for model %s: %v", model, err) + } + continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 809 - 837, buildSupportedOutputsIndex currently ignores json.Unmarshal errors and malformed paramsData, making it hard to trace why a model's supported_endpoints were skipped; update the function (buildSupportedOutputsIndex) to log parsing failures and include model identifier and the raw payload or error details when json.Unmarshal on paramsData[model] fails (and optionally when SupportedEndpoints is empty), using the existing logger or mc (e.g., process/global logger) so operators can see which model entries were malformed before updating mc.supportedOutputs under mc.mu; keep the de-duplication using normalizeEndpointToOutputType and slices.Contains as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/modelcatalog/main.go`:
- Around line 809-837: buildSupportedOutputsIndex currently ignores
json.Unmarshal errors and malformed paramsData, making it hard to trace why a
model's supported_endpoints were skipped; update the function
(buildSupportedOutputsIndex) to log parsing failures and include model
identifier and the raw payload or error details when json.Unmarshal on
paramsData[model] fails (and optionally when SupportedEndpoints is empty), using
the existing logger or mc (e.g., process/global logger) so operators can see
which model entries were malformed before updating mc.supportedOutputs under
mc.mu; keep the de-duplication using normalizeEndpointToOutputType and
slices.Contains as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 517f5671-9366-4b48-ab40-70dbba178533
📒 Files selected for processing (11)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/mux.gocore/utils.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.goplugins/litellmcompat/chattoresponses.goplugins/litellmcompat/main.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/litellmcompat/main.go
- core/schemas/bifrost.go
- transports/bifrost-http/server/server.go
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
3db6c20 to
f40f8c1
Compare
sammaji
force-pushed
the
03-13-feat_litellmcompat_chat_to_responses
branch
from
a0b8ad5 to
b8038bb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/litellmcompat/chattoresponses.go`:
- Around line 27-30: The early return when mc == nil prevents the new “convert
all chat→responses” fallback; instead remove that return and treat a nil model
catalog as the mode that allows conversion for all models—i.e., in
chattoresponses.go replace the mc==nil short-circuit with logic that sets a
fallback/allow-all behavior so subsequent endpoint-support checks use the
permissive path (refer to mc and req in this function) and continue processing
the request rather than returning immediately.
- Around line 104-117: The guard wrongly rejects converted errors because core
has already restored err.ExtraFields.RequestType; instead of checking
err.ExtraFields.RequestType for ResponsesRequest/ResponsesStreamRequest, rely on
the conversion context flag and restore the original chat metadata;
specifically, in the block that reads shouldConvert from ctx
(schemas.BifrostContextKeyShouldConvertChatToResponses) keep the early return if
not shouldConvert, but remove the requestType check and always call
getOriginalChatRequestMetadata(ctx) to set err.ExtraFields.RequestType,
err.ExtraFields.ModelRequested and set err.ExtraFields.LiteLLMCompat = true for
converted errors so the restored metadata and LiteLLMCompat flag are applied.
- Around line 48-53: The conversion-context flags are only set inside the true
branch, so a reused ctx can retain stale values; update the code in the
Chat->Responses conversion area to initialize/reset the keys on entry (set
schemas.BifrostContextKeyShouldConvertChatToResponses to false and clear
ChatToResponsesOriginalRequestTypeContextKey and
ChatToResponsesOriginalModelContextKey) before any early returns, then only set
them to true/actual values when you decide to convert (using ctx.SetValue with
req.RequestType and req.ChatRequest.Model); ensure the reset happens even when
ctx is nil-safe by guarding ctx != nil.
🪄 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: 98141f69-afaa-432f-9d81-9e248924fda9
📒 Files selected for processing (9)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/mux.gocore/utils.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.goplugins/litellmcompat/chattoresponses.goplugins/litellmcompat/main.go
✅ Files skipped from review due to trivial changes (1)
- core/utils.go
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/modelcatalog/sync.go
- core/schemas/bifrost.go
- framework/modelcatalog/main.go
- core/schemas/mux.go
sammaji
force-pushed
the
03-13-feat_litellmcompat_chat_to_responses
branch
from
b8038bb to
dcd8bcc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 5180-5190: The short-circuit success paths are returning raw
ResponsesResponse/ResponsesStreamResponse without normalizing them into
ChatResponse, which lets converted ChatCompletionRequest exits have
ChatResponse==nil; update the short-circuit branches in tryRequest and
tryStreamRequest to call the same normalization used after provider.Responses
(i.e., use shouldConvertChatToResponses + ToResponsesRequest and then convert
responsesResponse/streamResponse into ChatResponse via ToBifrostChatResponse
before calling RunPostLLMHooks) and reuse that normalization logic in the
snippet around provider.Responses so every ChatCompletionRequest exit path
consistently sets response.ChatResponse.
In `@core/schemas/mux.go`:
- Around line 1985-1988: The streaming chunk converter currently constructs a
BifrostChatResponse without copying provider enrichment fields, which drops
Citations, SearchResults, and Videos; update the chunk converter so the new resp
:= &BifrostChatResponse{...} also assigns Citations, SearchResults, and Videos
from the source/parent response/message (the same fields preserved by the
non-streaming converter) into the constructed BifrostChatResponse to ensure
streaming preserves those metadata fields.
- Around line 1990-1995: The code only copies resp.ID/Model when rsr.Response is
non-nil and never sets resp.Created, causing loss of chat-stream identity for
delta chunks; update the handling so you always copy a timestamp into
resp.Created when present (e.g., set resp.Created from rsr.Created or
rsr.Response.Created if available) and ensure ID/Model are propagated even when
rsr.Response is nil by reading any ID/Model fields present on rsr first and, if
later chunks arrive without those fields, maintain a tiny stream state in the
wrapper (e.g., lastResponseID/lastModel) to populate resp.ID and resp.Model for
subsequent delta events (adjust the code around the rsr/resp assignment block
that references rsr.Response, resp.ID, resp.Model, and Created).
🪄 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: ad13b389-924c-487b-8551-85eb444b1dda
📒 Files selected for processing (11)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/mux.gocore/utils.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.goplugins/litellmcompat/chattoresponses.goplugins/litellmcompat/main.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.go
✅ Files skipped from review due to trivial changes (1)
- framework/modelcatalog/utils.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/utils.go
- transports/bifrost-http/server/server.go
- core/schemas/bifrost.go
- framework/modelcatalog/main.go
- transports/bifrost-http/server/plugins.go
- plugins/litellmcompat/chattoresponses.go
- plugins/litellmcompat/main.go
11 tasks
2 tasks
sammaji
force-pushed
the
03-13-feat_litellmcompat_chat_to_responses
branch
from
dcd8bcc to
0c27d5a
Compare
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
2 times, most recently
from
83b22ac to
cf54793
Compare
sammaji
force-pushed
the
03-13-feat_litellmcompat_chat_to_responses
branch
from
0c27d5a to
d24a2b1
Compare
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/schemas/mux.go (1)
2059-2073:⚠️ Potential issue | 🟠 MajorMake the Responses→Chat stream converter stateful.
resp.ID,resp.Created, andresp.Modelare filled only fromrsr.Response, but the same-file producer does not attachResponseto normal delta events on Lines 1514-1765, and itscreated/in_progressevents on Lines 1455-1472 still omitModel. In the chat→responses→chat stack, ordinary chunks therefore go out with empty identity fields, while lifecycle-only events fall through as extra blank chat chunks. Please carry the last seenid/created/modelin stream state (or serialize them onto the Responses events) and drop lifecycle events that have no chat equivalent.Also applies to: 2260-2271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2059 - 2073, The converter in mux.go currently only sets resp.ID/resp.Created/resp.Model from rsr.Response when present, causing ordinary delta events to miss identity fields; update the Responses→Chat stream converter to be stateful by caching the last seen id, created, and model on the converter/stream state (e.g., add fields like lastID/lastCreated/lastModel to the converter struct) and use those cached values to populate BifrostChatResponse when rsr.Response is nil; additionally, suppress/drop lifecycle-only events that have no corresponding chat delta (so you don't emit empty chat chunks) and apply the same stateful logic to the analogous block around the 2260-2271 section.
🤖 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/schemas/mux.go`:
- Around line 2059-2073: The converter in mux.go currently only sets
resp.ID/resp.Created/resp.Model from rsr.Response when present, causing ordinary
delta events to miss identity fields; update the Responses→Chat stream converter
to be stateful by caching the last seen id, created, and model on the
converter/stream state (e.g., add fields like lastID/lastCreated/lastModel to
the converter struct) and use those cached values to populate
BifrostChatResponse when rsr.Response is nil; additionally, suppress/drop
lifecycle-only events that have no corresponding chat delta (so you don't emit
empty chat chunks) and apply the same stateful logic to the analogous block
around the 2260-2271 section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92543e2f-1f64-4973-bc9c-18e18a1c99ad
📒 Files selected for processing (9)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/mux.gocore/utils.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.goplugins/litellmcompat/chattoresponses.goplugins/litellmcompat/main.go
✅ Files skipped from review due to trivial changes (3)
- core/schemas/bifrost.go
- framework/modelcatalog/sync.go
- plugins/litellmcompat/chattoresponses.go
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/litellmcompat/main.go
Comment on lines
+1998
to
+2024
| // Append finalized function call items so the terminal response carries them in Output. | ||
| for toolCallID, args := range state.ToolArgumentBuffers { | ||
| if args == "" { | ||
| continue | ||
| } | ||
| statusFinal := terminalStatus | ||
| messageType := ResponsesMessageTypeFunctionCall | ||
| callName := state.ToolCallNames[toolCallID] | ||
| var callNamePtr *string | ||
| if callName != "" { | ||
| callNamePtr = &callName | ||
| } | ||
| argsValue := args | ||
| fcMsg := ResponsesMessage{ | ||
| Type: &messageType, | ||
| Status: &statusFinal, | ||
| ResponsesToolMessage: &ResponsesToolMessage{ | ||
| CallID: &toolCallID, | ||
| Name: callNamePtr, | ||
| Arguments: &argsValue, | ||
| }, | ||
| } | ||
| if itemID := state.ItemIDs[toolCallID]; itemID != "" { | ||
| fcMsg.ID = &itemID | ||
| } | ||
| response.Output = append(response.Output, fcMsg) | ||
| } |
Contributor
There was a problem hiding this comment.
Don't append terminal tool calls twice.
Lines 1955-1992 already add one ResponsesMessageTypeFunctionCall per tool call, in outputIndex order. This second map walk appends the same items again, and in random order. Because ToChatMessages() turns every function_call item into a chat tool call, the stacked chat→responses→chat fallback can ask the client to execute the same tool twice.
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
cf54793 to
bd8ff55
Compare
sammaji
force-pushed
the
03-13-feat_litellmcompat_chat_to_responses
branch
from
d24a2b1 to
a5a5854
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/schemas/mux.go (2)
2067-2073:⚠️ Potential issue | 🟠 MajorMost converted chat chunks still lose stream identity.
resp.ID,resp.Created, andresp.Modelare only copied whenrsr.Response != nil. The delta events produced in this stack (output_text.delta,function_call_arguments.delta, etc.) do not includeResponse, so ordinarychat.completion.chunkframes still serialize with empty identity unless the caller keeps separate stream state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2067 - 2073, The code only copies resp.ID/Created/Model when rsr.Response != nil, so delta events (which lack Response) lose stream identity; update the assignment to fall back to top-level fields on rsr when rsr.Response is nil — i.e. set resp.ID = first non-nil of rsr.Response.ID and rsr.ID, resp.Created = rsr.Response.CreatedAt or rsr.CreatedAt, and resp.Model = rsr.Response.Model or rsr.Model (check nil pointers appropriately) so resp retains identity for delta frames; refer to the rsr and resp variables and the fields rsr.Response.ID, rsr.ID, rsr.Response.CreatedAt, rsr.CreatedAt, rsr.Response.Model, and rsr.Model when applying the fix.
1998-2024:⚠️ Potential issue | 🟠 MajorDon't append terminal function calls twice.
Lines 1968-1992 already add one
function_callitem per tool call, ordered byoutputIndex. This second pass appends the same calls again, so the terminalresponse.completedpayload can duplicate tool calls and the chat fallback can surface duplicatetool_calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 1998 - 2024, The loop over state.ToolArgumentBuffers is re-appending function_call items already added earlier (the first pass that orders by outputIndex), causing duplicates in response.Output; to fix, avoid appending duplicates by checking whether a CallID is already present before creating fcMsg—use the toolCallID (state.ToolArgumentBuffers key) and inspect response.Output entries' ResponsesToolMessage.CallID (or maintain a local seen map keyed by toolCallID) and only append when not already seen; ensure you still set ID from state.ItemIDs and populate Name/Arguments as currently done in this loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/litellmcompat/chattoresponses.go`:
- Around line 27-50: The code sets
BifrostContextKeyShouldConvertChatToResponses=true whenever chat completions are
unsupported, but never verifies that a responses endpoint actually exists;
update the branch after the mc.IsChatCompletionSupported check to also verify
the model supports responses (e.g., call the model catalog method for responses
support such as mc.IsResponsesSupported(provider, model) or the equivalent API),
and only set the context keys ChatToResponsesOriginalRequestTypeContextKey,
ChatToResponsesOriginalModelContextKey and
BifrostContextKeyShouldConvertChatToResponses when that responses check passes;
if responses are not supported, avoid setting the conversion flag and change the
logger calls around logger.Debug("litellmcompat: marked chat completion...") to
reflect the actual capability or log a warning instead.
---
Duplicate comments:
In `@core/schemas/mux.go`:
- Around line 2067-2073: The code only copies resp.ID/Created/Model when
rsr.Response != nil, so delta events (which lack Response) lose stream identity;
update the assignment to fall back to top-level fields on rsr when rsr.Response
is nil — i.e. set resp.ID = first non-nil of rsr.Response.ID and rsr.ID,
resp.Created = rsr.Response.CreatedAt or rsr.CreatedAt, and resp.Model =
rsr.Response.Model or rsr.Model (check nil pointers appropriately) so resp
retains identity for delta frames; refer to the rsr and resp variables and the
fields rsr.Response.ID, rsr.ID, rsr.Response.CreatedAt, rsr.CreatedAt,
rsr.Response.Model, and rsr.Model when applying the fix.
- Around line 1998-2024: The loop over state.ToolArgumentBuffers is re-appending
function_call items already added earlier (the first pass that orders by
outputIndex), causing duplicates in response.Output; to fix, avoid appending
duplicates by checking whether a CallID is already present before creating
fcMsg—use the toolCallID (state.ToolArgumentBuffers key) and inspect
response.Output entries' ResponsesToolMessage.CallID (or maintain a local seen
map keyed by toolCallID) and only append when not already seen; ensure you still
set ID from state.ItemIDs and populate Name/Arguments as currently done in this
loop.
🪄 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: eafc0944-4782-47ae-8c1c-02dc5bde890c
📒 Files selected for processing (11)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/mux.gocore/utils.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.goplugins/litellmcompat/chattoresponses.goplugins/litellmcompat/main.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.go
✅ Files skipped from review due to trivial changes (2)
- core/utils.go
- transports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (6)
- core/schemas/bifrost.go
- framework/modelcatalog/sync.go
- framework/modelcatalog/utils.go
- core/bifrost.go
- framework/modelcatalog/main.go
- transports/bifrost-http/server/plugins.go
Comment on lines
+27
to
+50
| // Check if the model supports chat completion via model catalog | ||
| if mc != nil { | ||
| provider := req.ChatRequest.Provider | ||
| model := req.ChatRequest.Model | ||
| if mc.IsChatCompletionSupported(model, provider) { | ||
| if ctx != nil { | ||
| ctx.SetValue(schemas.BifrostContextKeyShouldConvertChatToResponses, false) | ||
| } | ||
| if logger != nil { | ||
| logger.Debug("litellmcompat: model %s/%s supports chat completion, skipping conversion", provider, model) | ||
| } | ||
| return req | ||
| } | ||
| } | ||
|
|
||
| // Track conversion intent. Core will do the actual conversion during provider dispatch. | ||
| if ctx != nil { | ||
| ctx.SetValue(schemas.BifrostContextKeyShouldConvertChatToResponses, true) | ||
| ctx.SetValue(ChatToResponsesOriginalRequestTypeContextKey, req.RequestType) | ||
| ctx.SetValue(ChatToResponsesOriginalModelContextKey, req.ChatRequest.Model) | ||
| } | ||
|
|
||
| if logger != nil { | ||
| logger.Debug("litellmcompat: marked chat completion for core chat->responses conversion for model %s (chat completion not supported, responses supported)", req.ChatRequest.Model) |
Contributor
There was a problem hiding this comment.
Require explicit responses capability before setting this fallback flag.
This branch only proves that chat completions are unsupported. Text-only models, unknown catalog entries, or models missing a responses endpoint will still set BifrostContextKeyShouldConvertChatToResponses=true, and core will route them into provider.Responses(...) even though the fallback target may not exist. The debug message on Line 50 already assumes “responses supported,” but that is never validated here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/litellmcompat/chattoresponses.go` around lines 27 - 50, The code sets
BifrostContextKeyShouldConvertChatToResponses=true whenever chat completions are
unsupported, but never verifies that a responses endpoint actually exists;
update the branch after the mc.IsChatCompletionSupported check to also verify
the model supports responses (e.g., call the model catalog method for responses
support such as mc.IsResponsesSupported(provider, model) or the equivalent API),
and only set the context keys ChatToResponsesOriginalRequestTypeContextKey,
ChatToResponsesOriginalModelContextKey and
BifrostContextKeyShouldConvertChatToResponses when that responses check passes;
if responses are not supported, avoid setting the conversion flag and change the
logger calls around logger.Debug("litellmcompat: marked chat completion...") to
reflect the actual capability or log a warning instead.
18 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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
litellmcompatwith model catalog.litellmcompatconverts chat completion to responses if chat completion is not supported by the model.Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines