Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces the legacy lite-llm plugin with a new built-in compat plugin that marks per-request conversion targets, drops or converts unsupported params, adds request/stream conversion glue in core, extends model-catalog indexes, migrates a single boolean to a structured compat config across DB/API/UI/docs/CI, and adds deep-cloning helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Core as BifrostCore
participant Compat as CompatPlugin
participant Catalog as ModelCatalog
participant Provider as Provider
Client->>Core: Send BifrostRequest (with ctx / x-bf-compat)
Core->>Compat: PreLLMHook(ctx, req)
Compat->>Catalog: IsRequestTypeSupported / GetSupportedParameters
alt conversion or drops needed
Compat-->>Core: return cloned+modified request, set ctx change-request-type & dropped params
else
Compat-->>Core: return original request
end
Core->>Provider: Invoke provider (text/chat/responses or stream)
Provider-->>Core: Response / stream chunks
Core->>Core: If ctx marked, convert responses or wrap stream post-hook runner
Core->>Compat: PostLLMHook(ctx, result, bifrostErr)
Compat-->>Client: Final response with extras (converted_request_type, dropped_compat_plugin_params)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
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: 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 4959-4967: The current branch in shouldConvertTextToChat uses
ToBifrostChatRequest → provider.ChatCompletion → ToBifrostTextCompletionResponse
and then unconditionally breaks treating it as success; instead validate each
step: after calling ToBifrostChatRequest ensure chatRequest != nil (already
checked), then after provider.ChatCompletion verify chatCompletionResponse !=
nil and bifrostError == nil, and after calling
chatCompletionResponse.ToBifrostTextCompletionResponse() ensure the returned
TextCompletionResponse is non-nil/mappable — if any of these validations fail,
do NOT break/return a successful empty response; either continue to the next
provider/branch or return a descriptive error. Update the code paths around
shouldConvertTextToChat, ToBifrostChatRequest, provider.ChatCompletion, and
ToBifrostTextCompletionResponse to perform these nil checks and handle failures
explicitly.
- Around line 4934-4944: wrapTextToChatStreamPostHookRunner currently converts
only the incoming result.ChatResponse before invoking postHookRunner, but
post-hook can replace/recover the response with a ChatResponse; modify the
wrapper to call postHookRunner(ctx, result, bifrostErr) first, capture its
returned (result, bifrostErr), then if returned result != nil and
result.ChatResponse != nil convert it via ToBifrostTextCompletionResponse and
return a new BifrostResponse with TextCompletionResponse set (preserving the
returned bifrostErr); keep the function name wrapTextToChatStreamPostHookRunner
and the postHookRunner invocation as the integration points to locate the
change.
In `@core/schemas/mux.go`:
- Around line 1988-1997: The conversion is rebuilding BifrostResponseExtraFields
from a subset and dropping fields on cr.ExtraFields (e.g., ModelDeployment,
RawRequest, ParseErrors, LiteLLMCompat and ProviderResponseHeaders); instead
preserve the full payload by using the original cr.ExtraFields when building the
converted response (or deep-copy all fields from cr.ExtraFields into
BifrostResponseExtraFields) in the conversion branches where
BifrostResponseExtraFields is constructed (references:
BifrostResponseExtraFields and cr.ExtraFields) so no metadata is lost for
converted /v1/completions responses or in the fallback branch.
🪄 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: 3978c01a-744b-4698-93ad-9558e3ddbf52
📒 Files selected for processing (7)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/chatcompletions.gocore/schemas/mux.goplugins/litellmcompat/context.goplugins/litellmcompat/main.goplugins/litellmcompat/texttochat.go
💤 Files with no reviewable changes (1)
- plugins/litellmcompat/context.go
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
d2c1fb3 to
1bc1946
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/bifrost.go (2)
4959-4967:⚠️ Potential issue | 🟠 MajorDon't treat failed chat→text conversion as a successful completion.
Line 4966 assigns the converted response and
breaks without checking eitherchatCompletionResponseor the conversion result. That can turn a provider bug or an unmappable chat payload intonil, nilat the public API boundary.Suggested fix
if shouldConvertTextToChat(req.Context, req.RequestType, req.BifrostRequest.TextCompletionRequest) { chatRequest := req.BifrostRequest.TextCompletionRequest.ToBifrostChatRequest() if chatRequest != nil { chatCompletionResponse, bifrostError := provider.ChatCompletion(req.Context, key, chatRequest) if bifrostError != nil { return nil, bifrostError } - response.TextCompletionResponse = chatCompletionResponse.ToBifrostTextCompletionResponse() + if chatCompletionResponse == nil { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{ + Message: "provider returned nil chat completion response", + }, + ExtraFields: schemas.BifrostErrorExtraFields{ + RequestType: req.RequestType, + Provider: provider.GetProviderKey(), + ModelRequested: req.BifrostRequest.TextCompletionRequest.Model, + }, + } + } + convertedResponse := chatCompletionResponse.ToBifrostTextCompletionResponse() + if convertedResponse == nil { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{ + Message: "failed to convert chat completion response to text completion response", + }, + ExtraFields: schemas.BifrostErrorExtraFields{ + RequestType: req.RequestType, + Provider: provider.GetProviderKey(), + ModelRequested: req.BifrostRequest.TextCompletionRequest.Model, + }, + } + } + response.TextCompletionResponse = convertedResponse break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4959 - 4967, The branch that converts text->chat and then back assumes provider.ChatCompletion and ToBifrostTextCompletionResponse always succeed; update the shouldConvertTextToChat branch (use symbols shouldConvertTextToChat, ToBifrostChatRequest, provider.ChatCompletion, ToBifrostTextCompletionResponse, response.TextCompletionResponse) to validate both chatCompletionResponse and the conversion result before assigning and breaking: if ToBifrostChatRequest returns nil skip conversion, if provider.ChatCompletion returns a nil response or an error propagate a proper bifrost error (do not return nil, nil), and if ToBifrostTextCompletionResponse returns nil treat it as a conversion failure and handle it (return an error or fall through to other handlers) so we never assign a nil TextCompletionResponse and exit silently.
4934-4944:⚠️ Potential issue | 🟠 MajorNormalize the wrapped stream after post-hooks too.
This wrapper only converts the inbound
ChatResponse. If a post-hook recovers or replaces the chunk with a chat-shaped response,/v1/completionscan still emit chat chunks, and a failed conversion currently falls through as success.Suggested fix
func wrapTextToChatStreamPostHookRunner(postHookRunner schemas.PostHookRunner) schemas.PostHookRunner { + normalize := func(result *schemas.BifrostResponse) (*schemas.BifrostResponse, *schemas.BifrostError) { + if result == nil || result.ChatResponse == nil { + return result, nil + } + + converted := result.ChatResponse.ToBifrostTextCompletionResponse() + if converted == nil { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{ + Message: "failed to convert chat stream response to text completion response", + }, + ExtraFields: schemas.BifrostErrorExtraFields{ + RequestType: schemas.TextCompletionStreamRequest, + }, + } + } + + return &schemas.BifrostResponse{ + TextCompletionResponse: converted, + }, nil + } + return func(ctx *schemas.BifrostContext, result *schemas.BifrostResponse, bifrostErr *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError) { - if result != nil && result.ChatResponse != nil { - if convertedResponse := result.ChatResponse.ToBifrostTextCompletionResponse(); convertedResponse != nil { - result = &schemas.BifrostResponse{ - TextCompletionResponse: convertedResponse, - } - } + normalizedResult, conversionErr := normalize(result) + if conversionErr != nil { + return nil, conversionErr } - return postHookRunner(ctx, result, bifrostErr) + + processedResult, processedErr := postHookRunner(ctx, normalizedResult, bifrostErr) + if processedErr != nil { + return processedResult, processedErr + } + + return normalize(processedResult) } }As per coding guidelines, "Post-hooks receive both response and error (either can be nil) and can recover or invalidate responses".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4934 - 4944, The wrapper wrapTextToChatStreamPostHookRunner currently only converts ChatResponse before calling postHookRunner; after postHookRunner returns you must re-normalize the possibly modified response: call result.ChatResponse.ToBifrostTextCompletionResponse() on the returned result and, if it returns non-nil, replace result with a BifrostResponse containing TextCompletionResponse; if conversion returns nil, clear ChatResponse/TextCompletionResponse on result (or set result to nil) so chat-shaped responses cannot leak through to /v1/completions. Ensure this normalization happens just before returning from wrapTextToChatStreamPostHookRunner and reference postHookRunner, ChatResponse, ToBifrostTextCompletionResponse, and TextCompletionResponse to locate the code.
🧹 Nitpick comments (1)
core/bifrost.go (1)
4402-4402: Remove the immediate-release TODO from the streaming loop.
outputStream <- streamResponsehands these response pointers to downstream readers asynchronously. ReleasingprocessedResponsehere would introduce the same pooled-object race this codebase has already hit in other streaming paths; either drop the TODO or rewrite it to document that ownership stays with the consumer/GC.Suggested fix
- // TODO: Release the processed response immediately after use + // Do not release processedResponse here; downstream stream consumers may + // still be reading the pointers carried in streamResponse.Based on learnings, "In Go streaming handlers that reuse pooled response objects ... do not release them back to their pools while asynchronous readers (PostLLMHook goroutines) may still access them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` at line 4402, Remove the misleading TODO that suggests releasing processedResponse immediately in the streaming loop; because outputStream <- streamResponse hands pointers to downstream readers (e.g., PostLLMHook goroutines), do not release or return processedResponse to any pool there—either delete the TODO or replace it with a comment that ownership of the response remains with the consumer/GC until downstream readers are done (reference outputStream, streamResponse, processedResponse, and PostLLMHook to make intent clear).
🤖 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/main.go`:
- Around line 81-84: PreLLMHook and transformTextToChatRequest currently leave
conversion state/metadata set from prior requests; ensure you always reset
BifrostContextKeyShouldConvertTextToChat=false and remove
OriginalRequestTypeContextKey and OriginalModelContextKey whenever conversion is
not applicable. Concretely: in PreLLMHook, before any early return for
non-text-completion requests, explicitly set the context marker to false and
clear the two Original* context keys; also update the code path in
transformTextToChatRequest where a text-completion model is supported (the
branch that sets marker=false) to also clear OriginalRequestTypeContextKey and
OriginalModelContextKey so metadata never lingers when conversion is declined.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 4959-4967: The branch that converts text->chat and then back
assumes provider.ChatCompletion and ToBifrostTextCompletionResponse always
succeed; update the shouldConvertTextToChat branch (use symbols
shouldConvertTextToChat, ToBifrostChatRequest, provider.ChatCompletion,
ToBifrostTextCompletionResponse, response.TextCompletionResponse) to validate
both chatCompletionResponse and the conversion result before assigning and
breaking: if ToBifrostChatRequest returns nil skip conversion, if
provider.ChatCompletion returns a nil response or an error propagate a proper
bifrost error (do not return nil, nil), and if ToBifrostTextCompletionResponse
returns nil treat it as a conversion failure and handle it (return an error or
fall through to other handlers) so we never assign a nil TextCompletionResponse
and exit silently.
- Around line 4934-4944: The wrapper wrapTextToChatStreamPostHookRunner
currently only converts ChatResponse before calling postHookRunner; after
postHookRunner returns you must re-normalize the possibly modified response:
call result.ChatResponse.ToBifrostTextCompletionResponse() on the returned
result and, if it returns non-nil, replace result with a BifrostResponse
containing TextCompletionResponse; if conversion returns nil, clear
ChatResponse/TextCompletionResponse on result (or set result to nil) so
chat-shaped responses cannot leak through to /v1/completions. Ensure this
normalization happens just before returning from
wrapTextToChatStreamPostHookRunner and reference postHookRunner, ChatResponse,
ToBifrostTextCompletionResponse, and TextCompletionResponse to locate the code.
---
Nitpick comments:
In `@core/bifrost.go`:
- Line 4402: Remove the misleading TODO that suggests releasing
processedResponse immediately in the streaming loop; because outputStream <-
streamResponse hands pointers to downstream readers (e.g., PostLLMHook
goroutines), do not release or return processedResponse to any pool there—either
delete the TODO or replace it with a comment that ownership of the response
remains with the consumer/GC until downstream readers are done (reference
outputStream, streamResponse, processedResponse, and PostLLMHook to make intent
clear).
🪄 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: 453d4574-6fe9-4e46-928a-12c9104bc18a
📒 Files selected for processing (7)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/chatcompletions.gocore/schemas/mux.goplugins/litellmcompat/context.goplugins/litellmcompat/main.goplugins/litellmcompat/texttochat.go
💤 Files with no reviewable changes (1)
- plugins/litellmcompat/context.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core/schemas/bifrost.go
- core/schemas/mux.go
18 tasks
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
1bc1946 to
3db6c20
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/schemas/mux.go (1)
1988-1997:⚠️ Potential issue | 🟠 MajorPreserve full
ExtraFieldsduring chat→text conversion.At Line 1988, Line 2021, Line 2057, and Line 2077,
ExtraFieldsis rebuilt from a subset, which drops existing metadata already present oncr.ExtraFields(e.g.,ModelDeployment,RawRequest,ParseErrors,LiteLLMCompat, and in one branchProviderResponseHeaders). Copycr.ExtraFieldsand only overrideRequestType.♻️ Suggested fix
func (cr *BifrostChatResponse) ToBifrostTextCompletionResponse() *BifrostTextCompletionResponse { if cr == nil { return nil } + toTextExtraFields := func() BifrostResponseExtraFields { + extra := cr.ExtraFields + extra.RequestType = TextCompletionRequest + return extra + } if len(cr.Choices) == 0 { return &BifrostTextCompletionResponse{ ID: cr.ID, Model: cr.Model, Object: "text_completion", SystemFingerprint: cr.SystemFingerprint, Usage: cr.Usage, - ExtraFields: BifrostResponseExtraFields{ - RequestType: TextCompletionRequest, - ChunkIndex: cr.ExtraFields.ChunkIndex, - Provider: cr.ExtraFields.Provider, - ModelRequested: cr.ExtraFields.ModelRequested, - Latency: cr.ExtraFields.Latency, - RawResponse: cr.ExtraFields.RawResponse, - CacheDebug: cr.ExtraFields.CacheDebug, - ProviderResponseHeaders: cr.ExtraFields.ProviderResponseHeaders, - }, + ExtraFields: toTextExtraFields(), } } @@ - ExtraFields: BifrostResponseExtraFields{ - RequestType: TextCompletionRequest, - ChunkIndex: cr.ExtraFields.ChunkIndex, - Provider: cr.ExtraFields.Provider, - ModelRequested: cr.ExtraFields.ModelRequested, - Latency: cr.ExtraFields.Latency, - RawResponse: cr.ExtraFields.RawResponse, - CacheDebug: cr.ExtraFields.CacheDebug, - ProviderResponseHeaders: cr.ExtraFields.ProviderResponseHeaders, - }, + ExtraFields: toTextExtraFields(), } } @@ - ExtraFields: BifrostResponseExtraFields{ - RequestType: TextCompletionRequest, - ChunkIndex: cr.ExtraFields.ChunkIndex, - Provider: cr.ExtraFields.Provider, - ModelRequested: cr.ExtraFields.ModelRequested, - Latency: cr.ExtraFields.Latency, - RawResponse: cr.ExtraFields.RawResponse, - CacheDebug: cr.ExtraFields.CacheDebug, - ProviderResponseHeaders: cr.ExtraFields.ProviderResponseHeaders, - }, + ExtraFields: toTextExtraFields(), } } @@ - ExtraFields: BifrostResponseExtraFields{ - RequestType: TextCompletionRequest, - ChunkIndex: cr.ExtraFields.ChunkIndex, - Provider: cr.ExtraFields.Provider, - ModelRequested: cr.ExtraFields.ModelRequested, - Latency: cr.ExtraFields.Latency, - RawResponse: cr.ExtraFields.RawResponse, - CacheDebug: cr.ExtraFields.CacheDebug, - }, + ExtraFields: toTextExtraFields(), } }Also applies to: 2021-2030, 2057-2066, 2077-2085
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 1988 - 1997, The current conversion rebuilds ExtraFields by constructing a new BifrostResponseExtraFields and copies only a subset of fields, dropping metadata on cr.ExtraFields (e.g., ModelDeployment, RawRequest, ParseErrors, LiteLLMCompat, ProviderResponseHeaders). Fix by copying cr.ExtraFields wholesale and then overriding RequestType to TextCompletionRequest (or the appropriate request type) instead of reconstructing a partial struct; update the code paths that build ExtraFields (the assignments that create a new BifrostResponseExtraFields near cr.ExtraFields usage) to use a shallow copy of cr.ExtraFields and set RequestType so all existing metadata is preserved.
🤖 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 1988-1997: The current conversion rebuilds ExtraFields by
constructing a new BifrostResponseExtraFields and copies only a subset of
fields, dropping metadata on cr.ExtraFields (e.g., ModelDeployment, RawRequest,
ParseErrors, LiteLLMCompat, ProviderResponseHeaders). Fix by copying
cr.ExtraFields wholesale and then overriding RequestType to
TextCompletionRequest (or the appropriate request type) instead of
reconstructing a partial struct; update the code paths that build ExtraFields
(the assignments that create a new BifrostResponseExtraFields near
cr.ExtraFields usage) to use a shallow copy of cr.ExtraFields and set
RequestType so all existing metadata is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d68108d6-491c-412a-94cf-dbafa701bb94
📒 Files selected for processing (7)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/chatcompletions.gocore/schemas/mux.goplugins/litellmcompat/context.goplugins/litellmcompat/main.goplugins/litellmcompat/texttochat.go
💤 Files with no reviewable changes (1)
- plugins/litellmcompat/context.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/schemas/bifrost.go
- plugins/litellmcompat/main.go
- core/bifrost.go
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
3db6c20 to
f40f8c1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
core/bifrost.go (2)
5114-5124:⚠️ Potential issue | 🟠 MajorNormalize the wrapped stream after post-hooks too.
Line 5116 only normalizes the inbound chunk. A post-hook can still recover or replace
resultwith aChatResponse, so/v1/completionscan emit chat chunks on the way out. Normalize both before and afterpostHookRunner, and fail closed if conversion returnsnil.Suggested fix
func wrapTextToChatStreamPostHookRunner(postHookRunner schemas.PostHookRunner) schemas.PostHookRunner { + normalize := func(result *schemas.BifrostResponse) (*schemas.BifrostResponse, *schemas.BifrostError) { + if result == nil || result.ChatResponse == nil { + return result, nil + } + + converted := result.ChatResponse.ToBifrostTextCompletionResponse() + if converted == nil { + return nil, newBifrostErrorFromMsg("failed to convert chat stream response to text completion response") + } + + return &schemas.BifrostResponse{ + TextCompletionResponse: converted, + }, nil + } + return func(ctx *schemas.BifrostContext, result *schemas.BifrostResponse, bifrostErr *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError) { - if result != nil && result.ChatResponse != nil { - if convertedResponse := result.ChatResponse.ToBifrostTextCompletionResponse(); convertedResponse != nil { - result = &schemas.BifrostResponse{ - TextCompletionResponse: convertedResponse, - } - } - } - return postHookRunner(ctx, result, bifrostErr) + normalized, conversionErr := normalize(result) + if conversionErr != nil { + return nil, conversionErr + } + + processed, processedErr := postHookRunner(ctx, normalized, bifrostErr) + if processedErr != nil { + return processed, processedErr + } + + return normalize(processed) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5114 - 5124, The wrapper wrapTextToChatStreamPostHookRunner currently converts result.ChatResponse to TextCompletionResponse only before calling postHookRunner; update it to also normalize the result after postHookRunner returns by checking result.ChatResponse and calling ChatResponse.ToBifrostTextCompletionResponse() again, replacing result.TextCompletionResponse with the converted value; if conversion after postHookRunner returns nil (meaning a ChatResponse was produced but cannot be converted), return a failing bifrost error (fail closed) instead of letting a chat stream leak through. Ensure you reference the existing types and methods (wrapTextToChatStreamPostHookRunner, schemas.PostHookRunner, schemas.BifrostResponse, ChatResponse.ToBifrostTextCompletionResponse) when making the change.
5139-5148:⚠️ Potential issue | 🟠 MajorDon't treat a failed chat→text conversion as success.
If Line 5146 returns
nil, this branch still breaks out of the switch and the caller gets a successful response withTextCompletionResponse == nil. Return a conversion error instead of falling through as success.Suggested fix
if shouldConvertTextToChat(req.Context, req.RequestType, req.BifrostRequest.TextCompletionRequest) { chatRequest := req.BifrostRequest.TextCompletionRequest.ToBifrostChatRequest() if chatRequest != nil { chatCompletionResponse, bifrostError := provider.ChatCompletion(req.Context, key, chatRequest) if bifrostError != nil { return nil, bifrostError } - response.TextCompletionResponse = chatCompletionResponse.ToBifrostTextCompletionResponse() + convertedResponse := chatCompletionResponse.ToBifrostTextCompletionResponse() + if convertedResponse == nil { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{ + Message: "failed to convert chat completion response to text completion response", + }, + ExtraFields: schemas.BifrostErrorExtraFields{ + RequestType: req.RequestType, + Provider: provider.GetProviderKey(), + ModelRequested: req.BifrostRequest.TextCompletionRequest.Model, + }, + } + } + response.TextCompletionResponse = convertedResponse break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5139 - 5148, The code treats a nil result from TextCompletionRequest.ToBifrostChatRequest as success because it breaks out of the switch; instead, detect when chatRequest == nil and return an explicit conversion error. In the branch where shouldConvertTextToChat(...) is true, replace the current nil-handling (the if chatRequest != nil { ... } else break) with logic that constructs and returns a descriptive error (e.g., fmt.Errorf or the package's error type) indicating chat conversion failed, so ChatCompletion is only called on non-nil chatRequest and response.TextCompletionResponse is never left nil on success; update the block around shouldConvertTextToChat, ToBifrostChatRequest, provider.ChatCompletion, and ToBifrostTextCompletionResponse accordingly.
🤖 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/texttochat.go`:
- Around line 31-45: The build fails because this plugin references
schemas.BifrostContextKeyShouldConvertTextToChat (and uses
OriginalRequestTypeContextKey / OriginalModelContextKey) but the corresponding
constants were not added to the core schemas; either add the new context key
constants to the core package (e.g., define
BifrostContextKeyShouldConvertTextToChat and any Original*ContextKey constants
in core/schemas/bifrost.go) and export them from the schemas package so plugins
can compile, or retarget/merge the PR to include the stacked change that
introduces these constants before merging this plugin change; update usages in
plugins/litellmcompat/texttochat.go to use the newly exported symbols.
- Around line 97-110: The guard that checks err.ExtraFields.RequestType against
ChatCompletionRequest/ChatCompletionStreamRequest should be removed so converted
errors still get normalized; in the block that currently reads
ctx.Value(schemas.BifrostContextKeyShouldConvertTextToChat).(bool) and returns
early, delete the requestType check and early return, and always run
getOriginalTextRequestMetadata(ctx) and set err.ExtraFields.RequestType,
err.ExtraFields.ModelRequested and err.ExtraFields.LiteLLMCompat = true (refer
to the variables ctx.Value(schemas.BifrostContextKeyShouldConvertTextToChat),
getOriginalTextRequestMetadata, and err.ExtraFields.RequestType/LiteLLMCompat to
locate the code).
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 5114-5124: The wrapper wrapTextToChatStreamPostHookRunner
currently converts result.ChatResponse to TextCompletionResponse only before
calling postHookRunner; update it to also normalize the result after
postHookRunner returns by checking result.ChatResponse and calling
ChatResponse.ToBifrostTextCompletionResponse() again, replacing
result.TextCompletionResponse with the converted value; if conversion after
postHookRunner returns nil (meaning a ChatResponse was produced but cannot be
converted), return a failing bifrost error (fail closed) instead of letting a
chat stream leak through. Ensure you reference the existing types and methods
(wrapTextToChatStreamPostHookRunner, schemas.PostHookRunner,
schemas.BifrostResponse, ChatResponse.ToBifrostTextCompletionResponse) when
making the change.
- Around line 5139-5148: The code treats a nil result from
TextCompletionRequest.ToBifrostChatRequest as success because it breaks out of
the switch; instead, detect when chatRequest == nil and return an explicit
conversion error. In the branch where shouldConvertTextToChat(...) is true,
replace the current nil-handling (the if chatRequest != nil { ... } else break)
with logic that constructs and returns a descriptive error (e.g., fmt.Errorf or
the package's error type) indicating chat conversion failed, so ChatCompletion
is only called on non-nil chatRequest and response.TextCompletionResponse is
never left nil on success; update the block around shouldConvertTextToChat,
ToBifrostChatRequest, provider.ChatCompletion, and
ToBifrostTextCompletionResponse 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: 7d06a24f-ea77-4fcb-8527-ce603a9de20d
📒 Files selected for processing (7)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/chatcompletions.gocore/schemas/mux.goplugins/litellmcompat/context.goplugins/litellmcompat/main.goplugins/litellmcompat/texttochat.go
💤 Files with no reviewable changes (1)
- plugins/litellmcompat/context.go
✅ Files skipped from review due to trivial changes (1)
- core/schemas/bifrost.go
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/litellmcompat/main.go
- core/schemas/mux.go
- core/schemas/chatcompletions.go
11 tasks
2 tasks
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
f40f8c1 to
83b22ac
Compare
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
83b22ac to
cf54793
Compare
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/schemas/mux.go (1)
2034-2043: 🛠️ Refactor suggestion | 🟠 MajorExtraFields are still manually reconstructed, dropping several fields.
The current implementation manually copies specific fields from
cr.ExtraFields, which dropsModelDeployment,RawRequest,ParseErrors, andLiteLLMCompat. The fallback branch (lines 2123-2131) additionally dropsProviderResponseHeaders.The previous review suggested using a struct value copy to preserve all fields:
♻️ Suggested approach to preserve all ExtraFields
- ExtraFields: BifrostResponseExtraFields{ - RequestType: TextCompletionRequest, - ChunkIndex: cr.ExtraFields.ChunkIndex, - Provider: cr.ExtraFields.Provider, - ModelRequested: cr.ExtraFields.ModelRequested, - Latency: cr.ExtraFields.Latency, - RawResponse: cr.ExtraFields.RawResponse, - CacheDebug: cr.ExtraFields.CacheDebug, - ProviderResponseHeaders: cr.ExtraFields.ProviderResponseHeaders, - }, + ExtraFields: func() BifrostResponseExtraFields { + extra := cr.ExtraFields + extra.RequestType = TextCompletionRequest + return extra + }(),Apply this pattern to all four branches (lines 2034-2043, 2067-2076, 2103-2112, 2123-2131) to ensure no metadata is lost during text-to-chat conversion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2034 - 2043, The code manually reconstructs BifrostResponseExtraFields (e.g., setting RequestType, ChunkIndex, Provider, ModelRequested, Latency, RawResponse, CacheDebug, ProviderResponseHeaders) which drops fields like ModelDeployment, RawRequest, ParseErrors, LiteLLMCompat; instead, copy the entire extra-fields struct and only override the RequestType (and any field that must change). Replace the manual field lists with a value copy from cr.ExtraFields (e.g., ExtraFields: cr.ExtraFields) and then set ExtraFields.RequestType = TextCompletionRequest (or assign a shallow copy then modify RequestType) in each of the four branches that currently rebuild the struct (the branches that use BifrostResponseExtraFields and reference cr.ExtraFields) so all metadata is preserved across the text-to-chat conversion.
🧹 Nitpick comments (1)
core/schemas/mux.go (1)
2017-2020: Duplicate section header within the same file.There's already a "RESPONSE CONVERSION METHODS" section header at line 1142. Consider either:
- Removing this duplicate header since the method belongs to the same logical section
- Using a more specific header like "TEXT COMPLETION CONVERSION METHODS"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2017 - 2020, Remove the duplicate section header "RESPONSE CONVERSION METHODS" in core/schemas/mux.go or rename this second occurrence to a more specific header (e.g., "TEXT COMPLETION CONVERSION METHODS") so sections remain unique; locate the repeated header string "RESPONSE CONVERSION METHODS" and either delete the duplicate block header or replace it with the specific header, leaving all following conversion functions (those in the same logical group) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/mux.go`:
- Around line 2080-2114: The non-streaming branch that handles
choice.ChatNonStreamResponseChoice only reads msg.Content.ContentStr and ignores
msg.Content.ContentBlocks, causing textContent to be nil for providers that
populate ContentBlocks; update the logic around msg (in the non-streaming
response conversion that returns a BifrostTextCompletionResponse and populates
TextCompletionResponseChoice.Text) to: if msg != nil && msg.Content != nil then
prefer ContentStr if present, otherwise iterate msg.Content.ContentBlocks,
concatenate each block.Text (if non-nil) using a strings.Builder, and set
textContent to the concatenated string pointer if any text was accumulated;
remember to add the strings import if missing.
---
Duplicate comments:
In `@core/schemas/mux.go`:
- Around line 2034-2043: The code manually reconstructs
BifrostResponseExtraFields (e.g., setting RequestType, ChunkIndex, Provider,
ModelRequested, Latency, RawResponse, CacheDebug, ProviderResponseHeaders) which
drops fields like ModelDeployment, RawRequest, ParseErrors, LiteLLMCompat;
instead, copy the entire extra-fields struct and only override the RequestType
(and any field that must change). Replace the manual field lists with a value
copy from cr.ExtraFields (e.g., ExtraFields: cr.ExtraFields) and then set
ExtraFields.RequestType = TextCompletionRequest (or assign a shallow copy then
modify RequestType) in each of the four branches that currently rebuild the
struct (the branches that use BifrostResponseExtraFields and reference
cr.ExtraFields) so all metadata is preserved across the text-to-chat conversion.
---
Nitpick comments:
In `@core/schemas/mux.go`:
- Around line 2017-2020: Remove the duplicate section header "RESPONSE
CONVERSION METHODS" in core/schemas/mux.go or rename this second occurrence to a
more specific header (e.g., "TEXT COMPLETION CONVERSION METHODS") so sections
remain unique; locate the repeated header string "RESPONSE CONVERSION METHODS"
and either delete the duplicate block header or replace it with the specific
header, leaving all following conversion functions (those in the same logical
group) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 164c52bb-718e-4997-ba96-7c6be819112d
📒 Files selected for processing (7)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/chatcompletions.gocore/schemas/mux.goplugins/litellmcompat/context.goplugins/litellmcompat/main.goplugins/litellmcompat/texttochat.go
💤 Files with no reviewable changes (1)
- plugins/litellmcompat/context.go
✅ Files skipped from review due to trivial changes (1)
- plugins/litellmcompat/main.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/schemas/bifrost.go
- plugins/litellmcompat/texttochat.go
- core/bifrost.go
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
cf54793 to
bd8ff55
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
core/bifrost.go (2)
5142-5150:⚠️ Potential issue | 🟠 MajorDo not treat chat→text conversion failure as success.
If
ToBifrostTextCompletionResponse()returnsnil, this branch currently stillbreaks and returns a successful envelope with an empty text payload.Proposed fix
if shouldConvertTextToChat(req.Context, req.RequestType, req.BifrostRequest.TextCompletionRequest) { chatRequest := req.BifrostRequest.TextCompletionRequest.ToBifrostChatRequest() if chatRequest != nil { chatCompletionResponse, bifrostError := provider.ChatCompletion(req.Context, key, chatRequest) if bifrostError != nil { return nil, bifrostError } - response.TextCompletionResponse = chatCompletionResponse.ToBifrostTextCompletionResponse() + converted := chatCompletionResponse.ToBifrostTextCompletionResponse() + if converted == nil { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{ + Message: "failed to convert chat completion response to text completion response", + }, + ExtraFields: schemas.BifrostErrorExtraFields{ + RequestType: req.RequestType, + Provider: provider.GetProviderKey(), + ModelRequested: req.BifrostRequest.TextCompletionRequest.Model, + }, + } + } + response.TextCompletionResponse = converted break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5142 - 5150, The branch that converts a text request to chat treats a nil conversion back to text as success; update the flow in the shouldConvertTextToChat -> ToBifrostChatRequest -> provider.ChatCompletion path to check the result of chatCompletionResponse.ToBifrostTextCompletionResponse() and if it returns nil do not break/return a successful envelope — instead return a proper error (or the existing bifrostError wrapper) so callers know conversion failed; adjust logic around ToBifrostTextCompletionResponse, provider.ChatCompletion, response.TextCompletionResponse and the surrounding control flow to only set response.TextCompletionResponse and break when the conversion is non-nil.
5117-5128:⚠️ Potential issue | 🟠 MajorNormalize stream output after post-hooks as well.
wrapTextToChatStreamPostHookRunnernormalizes only the inbound chunk. If a post-hook returns aChatResponse,/v1/completionscan still emit chat-shaped chunks. Re-normalize the returned result too, and fail conversion explicitly when mapping returnsnil.Proposed fix
func wrapTextToChatStreamPostHookRunner(postHookRunner schemas.PostHookRunner) schemas.PostHookRunner { + normalize := func(result *schemas.BifrostResponse) (*schemas.BifrostResponse, *schemas.BifrostError) { + if result == nil || result.ChatResponse == nil { + return result, nil + } + converted := result.ChatResponse.ToBifrostTextCompletionResponse() + if converted == nil { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{ + Message: "failed to convert chat stream response to text completion response", + }, + ExtraFields: schemas.BifrostErrorExtraFields{ + RequestType: schemas.TextCompletionStreamRequest, + }, + } + } + return &schemas.BifrostResponse{TextCompletionResponse: converted}, nil + } + return func(ctx *schemas.BifrostContext, result *schemas.BifrostResponse, bifrostErr *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError) { - if result != nil && result.ChatResponse != nil { - if convertedResponse := result.ChatResponse.ToBifrostTextCompletionResponse(); convertedResponse != nil { - result = &schemas.BifrostResponse{ - TextCompletionResponse: convertedResponse, - } - } - } - return postHookRunner(ctx, result, bifrostErr) + normalized, convErr := normalize(result) + if convErr != nil { + return nil, convErr + } + processed, processedErr := postHookRunner(ctx, normalized, bifrostErr) + if processedErr != nil { + return processed, processedErr + } + return normalize(processed) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5117 - 5128, The wrapper wrapTextToChatStreamPostHookRunner currently normalizes only the incoming result before calling postHookRunner, but must also normalize the value returned by postHookRunner: call postHookRunner(ctx, result, bifrostErr), examine its returned result's ChatResponse and attempt conversion with ToBifrostTextCompletionResponse again, replacing the returned BifrostResponse.TextCompletionResponse when conversion succeeds; if conversion returns nil while a ChatResponse is present, fail explicitly by returning a clear schemas.BifrostError (or set bifrostErr) to avoid emitting chat-shaped chunks from /v1/completions. Ensure you reference wrapTextToChatStreamPostHookRunner, PostHookRunner, ChatResponse, and ToBifrostTextCompletionResponse when making the changes.
🤖 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 5142-5150: The branch that converts a text request to chat treats
a nil conversion back to text as success; update the flow in the
shouldConvertTextToChat -> ToBifrostChatRequest -> provider.ChatCompletion path
to check the result of chatCompletionResponse.ToBifrostTextCompletionResponse()
and if it returns nil do not break/return a successful envelope — instead return
a proper error (or the existing bifrostError wrapper) so callers know conversion
failed; adjust logic around ToBifrostTextCompletionResponse,
provider.ChatCompletion, response.TextCompletionResponse and the surrounding
control flow to only set response.TextCompletionResponse and break when the
conversion is non-nil.
- Around line 5117-5128: The wrapper wrapTextToChatStreamPostHookRunner
currently normalizes only the incoming result before calling postHookRunner, but
must also normalize the value returned by postHookRunner: call
postHookRunner(ctx, result, bifrostErr), examine its returned result's
ChatResponse and attempt conversion with ToBifrostTextCompletionResponse again,
replacing the returned BifrostResponse.TextCompletionResponse when conversion
succeeds; if conversion returns nil while a ChatResponse is present, fail
explicitly by returning a clear schemas.BifrostError (or set bifrostErr) to
avoid emitting chat-shaped chunks from /v1/completions. Ensure you reference
wrapTextToChatStreamPostHookRunner, PostHookRunner, ChatResponse, and
ToBifrostTextCompletionResponse when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46dfe39f-3bc7-4850-a3a8-d26d2706903d
📒 Files selected for processing (7)
core/bifrost.gocore/schemas/bifrost.gocore/schemas/chatcompletions.gocore/schemas/mux.goplugins/litellmcompat/context.goplugins/litellmcompat/main.goplugins/litellmcompat/texttochat.go
💤 Files with no reviewable changes (1)
- plugins/litellmcompat/context.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/schemas/chatcompletions.go
- core/schemas/bifrost.go
- plugins/litellmcompat/main.go
- core/schemas/mux.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/modelcatalog/main.go (1)
206-216:⚠️ Potential issue | 🟠 MajorPopulate these new capability caches before serving requests.
supportedOutputsandsupportedParamsstart empty here, but they’re only rebuilt later viasyncModelParameters(). In theconfigStore == nilpath that never runs, and in the DB-backed path it happens asynchronously after startup. Until then,IsChatCompletionSupported/IsResponsesSupportedwill reportfalse, so litellmcompat can force the wrong fallback on cold start — or forever in in-memory deployments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 206 - 216, The supportedOutputs and supportedParams maps are left empty at construction causing IsChatCompletionSupported/IsResponsesSupported to return false until syncModelParameters runs asynchronously; after creating the ModelCatalog instance (mc) call syncModelParameters synchronously (or otherwise ensure it runs to completion) to populate supportedOutputs and supportedParams before returning/serving requests, e.g., invoke mc.syncModelParameters() (or the appropriate synchronous wrapper) in the constructor path that creates mc and handle the configStore==nil branch similarly so both in-memory and DB-backed startups have these capability caches populated.
♻️ Duplicate comments (2)
core/schemas/mux.go (2)
2342-2344:⚠️ Potential issue | 🟠 MajorSupport
ContentBlocksfallback in non-stream text extraction.Only reading
msg.Content.ContentStrdrops output for providers that populateContentBlocksinstead. Add block-text fallback before returning the converted text completion.♻️ Proposed fix
msg := choice.ChatNonStreamResponseChoice.Message var textContent *string - if msg != nil && msg.Content != nil && msg.Content.ContentStr != nil { - textContent = msg.Content.ContentStr + if msg != nil && msg.Content != nil { + if msg.Content.ContentStr != nil { + textContent = msg.Content.ContentStr + } else if len(msg.Content.ContentBlocks) > 0 { + var sb strings.Builder + for _, block := range msg.Content.ContentBlocks { + if block.Text != nil { + sb.WriteString(*block.Text) + } + } + if sb.Len() > 0 { + text := sb.String() + textContent = &text + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2342 - 2344, The current non-stream text extraction only reads msg.Content.ContentStr and drops output when providers populate msg.Content.ContentBlocks; update the extraction logic around textContent so that if msg.Content.ContentStr is nil or empty you fall back to msg.Content.ContentBlocks, iterating the blocks and concatenating their textual payloads (e.g., block.Text or block.ContentStr fields) into textContent before returning the converted text completion; ensure you reference msg.Content.ContentBlocks and the existing textContent variable so this fallback is used only when ContentStr is absent.
2292-2301:⚠️ Potential issue | 🟠 MajorPreserve full
ExtraFieldsduring chat→text conversion.Each branch rebuilds
BifrostResponseExtraFieldsfrom a subset, which drops existing metadata. This regresses observability/compat fields when conversion is now in provider dispatch.♻️ Proposed fix
func (cr *BifrostChatResponse) ToBifrostTextCompletionResponse() *BifrostTextCompletionResponse { if cr == nil { return nil } + + buildTextExtraFields := func() BifrostResponseExtraFields { + extra := cr.ExtraFields + extra.RequestType = TextCompletionRequest + return extra + } if len(cr.Choices) == 0 { return &BifrostTextCompletionResponse{ @@ - ExtraFields: BifrostResponseExtraFields{ - RequestType: TextCompletionRequest, - ChunkIndex: cr.ExtraFields.ChunkIndex, - Provider: cr.ExtraFields.Provider, - ModelRequested: cr.ExtraFields.ModelRequested, - Latency: cr.ExtraFields.Latency, - RawResponse: cr.ExtraFields.RawResponse, - CacheDebug: cr.ExtraFields.CacheDebug, - ProviderResponseHeaders: cr.ExtraFields.ProviderResponseHeaders, - }, + ExtraFields: buildTextExtraFields(), } } @@ - ExtraFields: BifrostResponseExtraFields{ ... }, + ExtraFields: buildTextExtraFields(), @@ - ExtraFields: BifrostResponseExtraFields{ ... }, + ExtraFields: buildTextExtraFields(), @@ - ExtraFields: BifrostResponseExtraFields{ ... }, + ExtraFields: buildTextExtraFields(), } }Also applies to: 2325-2334, 2361-2370, 2381-2389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2292 - 2301, The conversion rebuilds BifrostResponseExtraFields from a subset and drops metadata; instead preserve the full ExtraFields by copying cr.ExtraFields (or assigning ExtraFields: cr.ExtraFields) when converting chat→text so all observability/compat fields remain intact; update the conversion sites that reconstruct BifrostResponseExtraFields (the blocks creating ExtraFields at the locations around where BifrostResponseExtraFields is assembled in core/schemas/mux.go — e.g., the blocks currently setting RequestType, ChunkIndex, Provider, ModelRequested, Latency, RawResponse, CacheDebug, ProviderResponseHeaders) to reuse the existing cr.ExtraFields rather than rebuilding them, and only override RequestType when necessary.
🤖 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 1998-2024: The loop that iterates over state.ToolArgumentBuffers
and appends finalized function-call items to response.Output (the for ... range
using toolCallID, args and building fcMsg) duplicates entries already added via
allOutput; remove this entire loop (the block that constructs fcMsg and appends
to response.Output) so finalized function-call items are only included once in
response.Output, leaving the earlier allOutput-based population intact; locate
the loop by the symbols state.ToolArgumentBuffers,
ResponsesMessageTypeFunctionCall, and fcMsg to find and delete it.
In `@framework/modelcatalog/main.go`:
- Around line 897-927: The lookups in IsChatCompletionSupported,
IsResponsesSupported, and GetSupportedParameters must normalize the incoming
model key the same way the catalog does for provider-prefixed IDs and aliases;
before accessing mc.supportedOutputs or mc.supportedParams, transform the
incoming model string using the catalog's canonicalization helper (e.g., call
the existing normalize/canonicalize function used elsewhere—replace model with
the normalized value) so entries like "openai/gpt-4o" or provider-prefixed
aliases resolve to the stored canonical model key.
In `@plugins/litellmcompat/chattoresponses.go`:
- Around line 27-50: The code marks chat→responses conversion based only on
mc.IsChatCompletionSupported; change the logic to gate conversion on responses
support as well: if mc != nil check both mc.IsChatCompletionSupported(model,
provider) and mc.IsResponsesSupported(model, provider) and only set
schemas.BifrostContextKeyShouldConvertChatToResponses = true (and set
ChatToResponsesOriginalRequestTypeContextKey and
ChatToResponsesOriginalModelContextKey) when chat is not supported but responses
is supported; if neither is supported do not set the conversion flag. Also
update the logger.Debug message to reflect the combined capability check and
keep all references to mc, ctx, logger, req.ChatRequest.Model and
req.ChatRequest.Provider to locate the changes.
In `@plugins/litellmcompat/dropparams.go`:
- Around line 27-175: The unsupported-param helpers (unsupportedChatParams,
unsupportedResponsesParams, unsupportedTextCompletionParams) miss several fields
(e.g., user, store, stream_options, web_search_options, instructions,
background, suffix, best_of) so dropUnsupportedParams() cannot remove them;
update each helper to check those missing fields on the corresponding param
struct and append the correct canonical keys (matching what the catalog uses,
e.g., "user", "store", "stream_options", "web_search_options", "instructions",
"background", "suffix", "best_of") to the dropped slice when the field is
non-nil or non-empty and not present in the supported list, keeping the same
pattern and naming style used across the file to ensure dropUnsupportedParams()
can remove them.
In `@plugins/litellmcompat/main.go`:
- Around line 80-99: Previous dropped-params can persist across attempts; ensure
BifrostContextKeyLiteLLMCompatDroppedParams is cleared before/when recomputing.
Modify the block around computeUnsupportedParams/getModelFromRequest so that you
always call ctx.SetValue(schemas.BifrostContextKeyLiteLLMCompatDroppedParams,
nil) (or an empty slice) before or when model=="" or when len(droppedParams)==0;
keep the existing ctx.SetValue only for non-empty droppedParams, but ensure the
key is explicitly cleared otherwise. Target functions/identifiers:
getModelFromRequest, computeUnsupportedParams, ctx.SetValue, and the context key
BifrostContextKeyLiteLLMCompatDroppedParams.
---
Outside diff comments:
In `@framework/modelcatalog/main.go`:
- Around line 206-216: The supportedOutputs and supportedParams maps are left
empty at construction causing IsChatCompletionSupported/IsResponsesSupported to
return false until syncModelParameters runs asynchronously; after creating the
ModelCatalog instance (mc) call syncModelParameters synchronously (or otherwise
ensure it runs to completion) to populate supportedOutputs and supportedParams
before returning/serving requests, e.g., invoke mc.syncModelParameters() (or the
appropriate synchronous wrapper) in the constructor path that creates mc and
handle the configStore==nil branch similarly so both in-memory and DB-backed
startups have these capability caches populated.
---
Duplicate comments:
In `@core/schemas/mux.go`:
- Around line 2342-2344: The current non-stream text extraction only reads
msg.Content.ContentStr and drops output when providers populate
msg.Content.ContentBlocks; update the extraction logic around textContent so
that if msg.Content.ContentStr is nil or empty you fall back to
msg.Content.ContentBlocks, iterating the blocks and concatenating their textual
payloads (e.g., block.Text or block.ContentStr fields) into textContent before
returning the converted text completion; ensure you reference
msg.Content.ContentBlocks and the existing textContent variable so this fallback
is used only when ContentStr is absent.
- Around line 2292-2301: The conversion rebuilds BifrostResponseExtraFields from
a subset and drops metadata; instead preserve the full ExtraFields by copying
cr.ExtraFields (or assigning ExtraFields: cr.ExtraFields) when converting
chat→text so all observability/compat fields remain intact; update the
conversion sites that reconstruct BifrostResponseExtraFields (the blocks
creating ExtraFields at the locations around where BifrostResponseExtraFields is
assembled in core/schemas/mux.go — e.g., the blocks currently setting
RequestType, ChunkIndex, Provider, ModelRequested, Latency, RawResponse,
CacheDebug, ProviderResponseHeaders) to reuse the existing cr.ExtraFields rather
than rebuilding them, and only override RequestType when necessary.
🪄 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: a9fc2360-b891-441c-ac01-1300bb49c39b
📒 Files selected for processing (13)
core/bifrost.gocore/providers/utils/utils.gocore/schemas/bifrost.gocore/schemas/mux.gocore/utils.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.goplugins/litellmcompat/chattoresponses.goplugins/litellmcompat/dropparams.goplugins/litellmcompat/main.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.go
✅ Files skipped from review due to trivial changes (2)
- transports/bifrost-http/server/server.go
- transports/bifrost-http/server/plugins.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core/schemas/bifrost.go
- core/bifrost.go
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
2 times, most recently
from
70a2ce3 to
ff71e29
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)
core/providers/vertex/vertex.go (1)
458-476:⚠️ Potential issue | 🟠 MajorPropagate
ParamMappingsin all Vertex request-body wrappers.The Anthropic and Gemini chat branches correctly assign
paramMappings = reqBody.GetParameterMappings()before returning the wrappedVertexRawRequestBody, but the OpenAI chat branch (lines 458–476) and both image helpers (lines 1896–1933, 2159–2197) declare the variable without assigning it. This causes provider-parameter mappings to be silently dropped on these routes.Suggested fix
Add
paramMappings = reqBody.GetParameterMappings()after eachextraParams = reqBody.GetExtraParams()assignment in:
- OpenAI chat branch (line 464)
- Gemini image generation branch (line 1917)
- Imagen image generation branch (line 1931)
- Gemini image edit branch (line 2175)
- Imagen image edit branch (line 2189)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/vertex.go` around lines 458 - 476, The OpenAI chat and image helper branches fail to populate ParamMappings before returning VertexRawRequestBody; after each place where you set extraParams = reqBody.GetExtraParams() (notably in the OpenAI chat branch around reqBody.Model assignment and in the Gemini/Imagen image generation and edit branches), add paramMappings = reqBody.GetParameterMappings() so the returned &VertexRawRequestBody{... ParamMappings: paramMappings} carries the provider parameter mappings; update the spots referenced in the review (OpenAI chat, Gemini image gen, Imagen image gen, Gemini image edit, Imagen image edit) to call GetParameterMappings() immediately after GetExtraParams().
♻️ Duplicate comments (4)
core/bifrost.go (1)
5119-5128:⚠️ Potential issue | 🟠 MajorReturn an error when the conversion helper can’t map the provider response.
Both converted branches break even if
ToBifrostTextCompletionResponse()/ToBifrostChatResponse()returnsnil. That produces a successfulBifrostResponsewith the expected field unset, and the public request method can bubble upnil, nilinstead of surfacing a conversion failure.Suggested fix
case schemas.TextCompletionRequest: changeType, _ := req.Context.Value(schemas.BifrostContextKeyChangeRequestType).(schemas.RequestType) if changeType == schemas.ChatCompletionRequest { chatRequest := req.BifrostRequest.TextCompletionRequest.ToBifrostChatRequest() if chatRequest != nil { chatCompletionResponse, bifrostError := provider.ChatCompletion(req.Context, key, chatRequest) if bifrostError != nil { return nil, bifrostError } - response.TextCompletionResponse = chatCompletionResponse.ToBifrostTextCompletionResponse() + convertedResponse := chatCompletionResponse.ToBifrostTextCompletionResponse() + if convertedResponse == nil { + return nil, newBifrostErrorFromMsg("failed to convert chat completion response to text completion response") + } + response.TextCompletionResponse = convertedResponse break } } @@ case schemas.ChatCompletionRequest: changeType, _ := req.Context.Value(schemas.BifrostContextKeyChangeRequestType).(schemas.RequestType) if changeType == schemas.ResponsesRequest { responsesRequest := req.BifrostRequest.ChatRequest.ToResponsesRequest() if responsesRequest != nil { responsesResponse, bifrostError := provider.Responses(req.Context, key, responsesRequest) if bifrostError != nil { return nil, bifrostError } - response.ChatResponse = responsesResponse.ToBifrostChatResponse() + convertedResponse := responsesResponse.ToBifrostChatResponse() + if convertedResponse == nil { + return nil, newBifrostErrorFromMsg("failed to convert responses response to chat response") + } + response.ChatResponse = convertedResponse break } }Also applies to: 5137-5146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5119 - 5128, The conversion helpers ToBifrostTextCompletionResponse and ToBifrostChatResponse may return nil, causing a silent success with missing fields; update the handling in the ChatCompletion branch (where you call provider.ChatCompletion and then chatCompletionResponse.ToBifrostTextCompletionResponse) and the parallel branch that calls ToBifrostChatResponse to check the converted value for nil and return a descriptive error (e.g., fmt.Errorf("failed to convert provider response to Bifrost response")) instead of breaking/continuing, so the caller receives an explicit error when conversion fails.framework/modelcatalog/main.go (2)
885-889:⚠️ Potential issue | 🟠 MajorCanonicalize the model key before reading these indexes.
Both methods index
supportedResponseTypes/supportedParamswith the caller’s rawmodelstring and never useprovider. Provider-prefixed IDs and aliases will miss the cached entry and be treated as unsupported, which can skip provider-level conversion and miscompute dropped params for models that are actually in the catalog. Use the same model canonicalization helper the catalog already uses elsewhere before reading these maps.Also applies to: 894-904
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 885 - 889, IsRequestTypeSupported (and the similar method reading supportedParams) currently uses the raw model string to index supportedResponseTypes/supportedParams so provider-prefixed IDs or aliases miss cached entries; fix by canonicalizing the incoming model key with the catalog's existing model canonicalization helper (e.g., canonicalizeModelID or the catalog's canonicalizer) before accessing mc.supportedResponseTypes and mc.supportedParams, so lookups use the same canonical model key used when entries were stored and provider/alias variants resolve correctly.
931-942:⚠️ Potential issue | 🟠 MajorTranslate datasheet parameter IDs into request field aliases.
extractSupportedParamsstill emits raw IDs likemax_tokensandstop_sequences, but the compat/dropUnsupportedParams flow later matches actual request field names such asmax_completion_tokens,max_output_tokens, andstop. That makes supported max-token/stop controls look unsupported and strips them from chat/responses requests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 931 - 942, extractSupportedParams currently adds raw datasheet IDs (in the parsed.ModelParameters loop) like "max_tokens" and "stop_sequences" which later don't match compat/dropUnsupportedParams request field names; update the switch/logic in the parsed.ModelParameters loop inside extractSupportedParams to map datasheet IDs to their request-field aliases before calling addParam (e.g., map "max_tokens" to "max_completion_tokens" and "max_output_tokens" as appropriate, map "stop_sequences" to "stop", and any other known alias pairs), keeping existing special cases (reasoning, web_search, promptTools, image_detail, stream) intact so compat/dropUnsupportedParams will correctly recognize supported parameters.core/schemas/mux.go (1)
2257-2274:⚠️ Potential issue | 🟠 MajorPreserve
cr.ExtraFieldsand set the stream request type per branch.Every branch rebuilds
BifrostResponseExtraFieldsfrom a subset, so converted/v1/completionsresponses drop metadata already present oncr.ExtraFields(ModelDeployment,RawRequest,ParseErrors,DroppedCompatPluginParams, etc.). The streaming branch also hardcodesTextCompletionRequest, so downstream hooks still see a non-stream request after converting aChatStreamResponseChoice.♻️ Safer pattern
+ buildExtra := func(rt RequestType) BifrostResponseExtraFields { + extra := cr.ExtraFields + extra.RequestType = rt + return extra + } + if choice.ChatStreamResponseChoice != nil && choice.ChatStreamResponseChoice.Delta != nil { return &BifrostTextCompletionResponse{ ... - ExtraFields: BifrostResponseExtraFields{ - RequestType: TextCompletionRequest, - ... - }, + ExtraFields: buildExtra(TextCompletionStreamRequest), } } ... - ExtraFields: BifrostResponseExtraFields{ - RequestType: TextCompletionRequest, - ... - }, + ExtraFields: buildExtra(TextCompletionRequest),Also applies to: 2279-2306, 2330-2374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2257 - 2274, The code reconstructs BifrostResponseExtraFields from a subset and overwrites cr.ExtraFields, causing loss of metadata (e.g., ModelDeployment, RawRequest, ParseErrors) and incorrectly hardcoding RequestType; instead, preserve all existing cr.ExtraFields by shallow-copying it into the response and only override the RequestType (e.g., set to TextCompletionRequest or Stream/Text as appropriate) and any branch-specific fields (ChunkIndex, ProviderResponseHeaders, RawResponse, Latency, CacheDebug) rather than rebuilding the struct from scratch; update each branch that builds BifrostTextCompletionResponse or similar responses (referencing BifrostTextCompletionResponse, BifrostResponseExtraFields, and cr.ExtraFields) to start with extra := cr.ExtraFields, mutate extra.RequestType = <correct type> and then assign ExtraFields: extra.
🧹 Nitpick comments (1)
core/providers/gemini/types.go (1)
2697-2697: Consider preserving trailing newline.Most editors and Go style guides expect files to end with a newline character. This is a minor nit that doesn't affect functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/types.go` at line 2697, The file core/providers/gemini/types.go currently ends with a closing brace but lacks a trailing newline; please add a single newline character at the end of types.go (after the final "}" in that file) so the file ends with a newline to conform with Go style/editor expectations.
🤖 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/gemini/types.go`:
- Around line 114-115: GetParameterMappings() has the "logprobs" and
"top_logprobs" JSON path mappings inverted relative to the conversion logic in
utils.go, which causes dropUnsupportedParams() to remove the wrong fields;
update the map in GetParameterMappings() so that the key "logprobs" maps to
"generationConfig.responseLogprobs" and the key "top_logprobs" maps to
"generationConfig.logprobs" to match how params.LogProbs →
config.ResponseLogprobs and params.TopLogProbs → config.Logprobs are handled in
utils.go.
In `@core/providers/openai/types.go`:
- Line 574: The GetParameterMappings method currently returns nil causing
dropUnsupportedParams to remove new chat keys instead of legacy ones; implement
OpenAIResponsesRequest.GetParameterMappings to return a map that maps old chat
parameter names to the new responses parameter names (e.g.
"max_completion_tokens" -> "max_output_tokens", "verbosity" -> "text.verbosity",
and any other fields rewritten by BifrostChatRequest.ToResponsesRequest) so
compat will drop the legacy chat keys and preserve the rewritten responses keys
when serializing the provider request.
In `@core/schemas/mux.go`:
- Around line 2021-2023: The ToBifrostChatResponse method is currently creating
synthetic chat.completion.chunk payloads with empty deltas for
non-chat-equivalent BifrostResponsesStreamResponse events; instead, update the
conditional branches inside ToBifrostChatResponse (and the similar branches
referenced around the other ranges) to return nil for
lifecycle/metadata/event-only responses rather than constructing empty
BifrostChatResponse objects, ensuring only true chat-equivalent events produce a
BifrostChatResponse; locate the branches that build empty "delta" content and
replace those constructions with an early return nil so no SSE chat chunk is
emitted for non-chat events.
In `@plugins/compat/main.go`:
- Around line 98-118: The BifrostContextKeyCompatPluginDroppedParams value must
be cleared before recomputing dropped params to avoid leaking stale state; add a
call to ctx.ClearValue(schemas.BifrostContextKeyCompatPluginDroppedParams)
alongside the existing
ctx.ClearValue(schemas.BifrostContextKeyChangeRequestType) at the start of the
hook (so it runs before the modelCatalog/getModelFromRequest ->
computeUnsupportedParams flow) to ensure ctx.SetValue only contains the newly
computed droppedParams for the current provider/model.
---
Outside diff comments:
In `@core/providers/vertex/vertex.go`:
- Around line 458-476: The OpenAI chat and image helper branches fail to
populate ParamMappings before returning VertexRawRequestBody; after each place
where you set extraParams = reqBody.GetExtraParams() (notably in the OpenAI chat
branch around reqBody.Model assignment and in the Gemini/Imagen image generation
and edit branches), add paramMappings = reqBody.GetParameterMappings() so the
returned &VertexRawRequestBody{... ParamMappings: paramMappings} carries the
provider parameter mappings; update the spots referenced in the review (OpenAI
chat, Gemini image gen, Imagen image gen, Gemini image edit, Imagen image edit)
to call GetParameterMappings() immediately after GetExtraParams().
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 5119-5128: The conversion helpers ToBifrostTextCompletionResponse
and ToBifrostChatResponse may return nil, causing a silent success with missing
fields; update the handling in the ChatCompletion branch (where you call
provider.ChatCompletion and then
chatCompletionResponse.ToBifrostTextCompletionResponse) and the parallel branch
that calls ToBifrostChatResponse to check the converted value for nil and return
a descriptive error (e.g., fmt.Errorf("failed to convert provider response to
Bifrost response")) instead of breaking/continuing, so the caller receives an
explicit error when conversion fails.
In `@core/schemas/mux.go`:
- Around line 2257-2274: The code reconstructs BifrostResponseExtraFields from a
subset and overwrites cr.ExtraFields, causing loss of metadata (e.g.,
ModelDeployment, RawRequest, ParseErrors) and incorrectly hardcoding
RequestType; instead, preserve all existing cr.ExtraFields by shallow-copying it
into the response and only override the RequestType (e.g., set to
TextCompletionRequest or Stream/Text as appropriate) and any branch-specific
fields (ChunkIndex, ProviderResponseHeaders, RawResponse, Latency, CacheDebug)
rather than rebuilding the struct from scratch; update each branch that builds
BifrostTextCompletionResponse or similar responses (referencing
BifrostTextCompletionResponse, BifrostResponseExtraFields, and cr.ExtraFields)
to start with extra := cr.ExtraFields, mutate extra.RequestType = <correct type>
and then assign ExtraFields: extra.
In `@framework/modelcatalog/main.go`:
- Around line 885-889: IsRequestTypeSupported (and the similar method reading
supportedParams) currently uses the raw model string to index
supportedResponseTypes/supportedParams so provider-prefixed IDs or aliases miss
cached entries; fix by canonicalizing the incoming model key with the catalog's
existing model canonicalization helper (e.g., canonicalizeModelID or the
catalog's canonicalizer) before accessing mc.supportedResponseTypes and
mc.supportedParams, so lookups use the same canonical model key used when
entries were stored and provider/alias variants resolve correctly.
- Around line 931-942: extractSupportedParams currently adds raw datasheet IDs
(in the parsed.ModelParameters loop) like "max_tokens" and "stop_sequences"
which later don't match compat/dropUnsupportedParams request field names; update
the switch/logic in the parsed.ModelParameters loop inside
extractSupportedParams to map datasheet IDs to their request-field aliases
before calling addParam (e.g., map "max_tokens" to "max_completion_tokens" and
"max_output_tokens" as appropriate, map "stop_sequences" to "stop", and any
other known alias pairs), keeping existing special cases (reasoning, web_search,
promptTools, image_detail, stream) intact so compat/dropUnsupportedParams will
correctly recognize supported parameters.
---
Nitpick comments:
In `@core/providers/gemini/types.go`:
- Line 2697: The file core/providers/gemini/types.go currently ends with a
closing brace but lacks a trailing newline; please add a single newline
character at the end of types.go (after the final "}" in that file) so the file
ends with a newline to conform with Go style/editor expectations.
🪄 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: c861d03d-5027-4c9c-ae32-cc30f6b9ae24
⛔ Files ignored due to path filters (2)
plugins/compat/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
AGENTS.mdcore/bifrost.gocore/providers/anthropic/types.gocore/providers/bedrock/models.gocore/providers/bedrock/types.gocore/providers/cohere/models.gocore/providers/cohere/types.gocore/providers/elevenlabs/types.gocore/providers/gemini/types.gocore/providers/huggingface/types.gocore/providers/nebius/types.gocore/providers/openai/types.gocore/providers/perplexity/types.gocore/providers/replicate/types.gocore/providers/runway/types.gocore/providers/utils/utils.gocore/providers/vertex/models.gocore/providers/vertex/types.gocore/providers/vertex/vertex.gocore/providers/vllm/models.gocore/schemas/bifrost.gocore/schemas/chatcompletions.gocore/schemas/mux.gocore/utils.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.gonix/packages/bifrost-http.nixplugins/compat/changelog.mdplugins/compat/dropparams.goplugins/compat/go.modplugins/compat/main.goplugins/compat/versionplugins/litellmcompat/context.goplugins/litellmcompat/main.goplugins/litellmcompat/texttochat.gotransports/Dockerfile.localtransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.gotransports/go.mod
💤 Files with no reviewable changes (3)
- plugins/litellmcompat/context.go
- plugins/litellmcompat/texttochat.go
- plugins/litellmcompat/main.go
✅ Files skipped from review due to trivial changes (3)
- transports/bifrost-http/server/server.go
- core/providers/replicate/types.go
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (3)
- transports/bifrost-http/server/plugins.go
- framework/modelcatalog/sync.go
- core/providers/utils/utils.go
👮 Files not reviewed due to content moderation or server errors (6)
- core/providers/anthropic/types.go
- plugins/compat/dropparams.go
- core/providers/vertex/types.go
- core/providers/cohere/types.go
- core/schemas/bifrost.go
- core/providers/bedrock/types.go
sammaji
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/utils.go (2)
504-512:⚠️ Potential issue | 🔴 CriticalFix the misspelled provider key identifier.
Line 511 references
provierKey, which is undefined and currently breaks compilation.🐛 Proposed fix
- return "session:" + string(provierKey) + ":" + hashedSessionID + ":" + hashSHA256(discriminator) + return "session:" + string(providerKey) + ":" + hashedSessionID + ":" + hashSHA256(discriminator)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/utils.go` around lines 504 - 512, In buildSessionKey replace the misspelled identifier `provierKey` with the correct `providerKey` (the function parameter of type schemas.ModelProvider) so the return expression uses string(providerKey); ensure the concatenation still casts providerKey to string as originally intended to fix the undefined identifier and allow the code to compile.
194-199:⚠️ Potential issue | 🟠 MajorClear dropped compat params before fallback dispatch.
schemas.BifrostContextKeyCompatPluginDroppedParamsis also provider-specific state. Leaving it on the context lets a fallback attempt inherit the primary model's drop list and strip parameters that the fallback model actually supports.♻️ Proposed fix
func clearCtxForFallback(ctx *schemas.BifrostContext) { ctx.ClearValue(schemas.BifrostContextKeyAPIKeyID) ctx.ClearValue(schemas.BifrostContextKeyAPIKeyName) ctx.ClearValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys) ctx.ClearValue(schemas.BifrostContextKeyChangeRequestType) + ctx.ClearValue(schemas.BifrostContextKeyCompatPluginDroppedParams) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/utils.go` around lines 194 - 199, The clearCtxForFallback function omits clearing provider-specific compat state, causing fallback models to inherit dropped-param lists; update clearCtxForFallback to also call ctx.ClearValue for schemas.BifrostContextKeyCompatPluginDroppedParams so the compat-plugin dropped params are removed before dispatching to a fallback model (i.e., add ctx.ClearValue(schemas.BifrostContextKeyCompatPluginDroppedParams) alongside the other ClearValue calls in clearCtxForFallback).
♻️ Duplicate comments (6)
core/providers/gemini/types.go (1)
114-115:⚠️ Potential issue | 🟠 MajorLogprobs parameter mappings appear inverted.
This issue was previously flagged: based on the conversion logic in
utils.go, the mappings should be:
"logprobs"→"generationConfig.responseLogprobs"(OpenAI'slogprobsbool maps to Gemini'sresponseLogprobsbool)"top_logprobs"→"generationConfig.logprobs"(OpenAI'stop_logprobsint maps to Gemini'slogprobsint32)The current mappings are inverted, which would cause
dropUnsupportedParams()to delete incorrect JSON paths when the compat plugin marks these parameters as unsupported.🐛 Proposed fix
- "logprobs": "generationConfig.logprobs", - "top_logprobs": "generationConfig.responseLogprobs", + "logprobs": "generationConfig.responseLogprobs", + "top_logprobs": "generationConfig.logprobs",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/types.go` around lines 114 - 115, The mapping for OpenAI params is inverted: in core/providers/gemini/types.go swap the values for the keys "logprobs" and "top_logprobs" so that "logprobs" maps to "generationConfig.responseLogprobs" and "top_logprobs" maps to "generationConfig.logprobs"; update the entries where those keys are defined (refer to the map containing "logprobs" and "top_logprobs") so dropUnsupportedParams() and the compat conversion logic in utils.go use the correct JSON paths.framework/modelcatalog/main.go (2)
883-905:⚠️ Potential issue | 🟠 MajorModel key normalization missing before index lookup.
This issue was previously flagged: lookups use the raw
modelstring directly, butModelCatalogelsewhere normalizes provider-prefixed IDs (e.g., "openai/gpt-4o") and aliases to canonical model names.Requests with prefixed models will miss the cached capability/parameter rows even when the datasheet entry exists under the canonical model name.
Consider normalizing the model key using the existing
getBaseModelNameUnsafepattern orParseModelStringbefore accessingsupportedResponseTypesandsupportedParams.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 883 - 905, IsRequestTypeSupported and GetSupportedParameters use the raw model string for map lookups, so provider-prefixed IDs or aliases miss cached entries; normalize the model key (e.g., call getBaseModelNameUnsafe or ParseModelString to obtain the canonical base name) before accessing mc.supportedResponseTypes and mc.supportedParams in IsRequestTypeSupported and GetSupportedParameters, keeping the same RLock/RUnlock pattern and still returning a copy of params to avoid external mutation.
931-943:⚠️ Potential issue | 🟠 MajorParameter ID mapping may be incomplete for downstream consumers.
This issue was previously flagged: datasheet parameter IDs like
max_tokensandstop_sequencesfall through unchanged, but downstream parameter-dropping logic may check formax_completion_tokens,max_output_tokens, orstop. This mismatch can make supported parameters appear unsupported and get incorrectly dropped.Consider adding explicit mappings for common variations:
max_tokens→ also addmax_completion_tokens,max_output_tokensstop_sequences→ also addstop🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 931 - 943, The loop over parsed.ModelParameters (in the block that switches on mp.ID) misses common downstream aliases so supported params get dropped; update the switch in that loop (where addParam is called for mp.ID) to explicitly map aliases: when mp.ID == "max_tokens" call addParam("max_tokens") and also addParam("max_completion_tokens") and addParam("max_output_tokens"); when mp.ID == "stop_sequences" call addParam("stop_sequences") and also addParam("stop"); keep existing mappings for "reasoning_effort", "reasoning_summary", "web_search", "promptTools", "image_detail", and "stream" unchanged.core/schemas/mux.go (2)
2258-2375:⚠️ Potential issue | 🟠 MajorPreserve the full
ExtraFieldspayload during chat→text conversion.This helper now hand-copies metadata in multiple branches, and the fallback already drops
ProviderResponseHeaders. Since this conversion moved into the provider path, that changes the metadata surface seen by hooks/logging for converted/v1/completionsresponses.♻️ Suggested fix
- ExtraFields: BifrostResponseExtraFields{ - RequestType: TextCompletionRequest, - ChunkIndex: cr.ExtraFields.ChunkIndex, - Provider: cr.ExtraFields.Provider, - ModelRequested: cr.ExtraFields.ModelRequested, - Latency: cr.ExtraFields.Latency, - RawResponse: cr.ExtraFields.RawResponse, - CacheDebug: cr.ExtraFields.CacheDebug, - ProviderResponseHeaders: cr.ExtraFields.ProviderResponseHeaders, - }, + ExtraFields: func() BifrostResponseExtraFields { + extra := cr.ExtraFields + extra.RequestType = TextCompletionRequest + return extra + }(),Apply the same pattern to each return site in this helper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2258 - 2375, The helper converting chat→text (building BifrostTextCompletionResponse) is hand-copying ExtraFields in multiple branches and the fallback omits ProviderResponseHeaders, losing metadata; update every return site that constructs ExtraFields (including the initial non-choice return, the streaming branch that builds Choices, the non-streaming branch that builds Choices, and the final fallback) to copy the entire cr.ExtraFields (or at least include ProviderResponseHeaders and any other fields present on cr.ExtraFields) instead of hand-copying a subset so the full metadata from cr.ExtraFields is preserved in the BifrostTextCompletionResponse.
2087-2243:⚠️ Potential issue | 🟠 MajorReturn
nilfor non-chat-equivalent stream events.The docstring says these branches should be dropped, but they still synthesize empty
chat.completion.chunkpayloads. That adds extra stream events the chat API never emits and can confuse downstream consumers now that conversion happens in the provider path.♻️ Suggested fix
case ResponsesStreamResponseTypeOutputItemAdded: if rsr.Item == nil || rsr.Item.Type == nil { - resp.Choices = []BifrostResponseChoice{ - { - Index: 0, - ChatStreamResponseChoice: &ChatStreamResponseChoice{ - Delta: &ChatStreamResponseChoiceDelta{}, - }, - }, - } - return resp + return nil }Apply the same pattern to the other malformed/lifecycle-only branches that currently return an empty delta chunk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2087 - 2243, Change branches that synthesize empty chat chunks into returning nil for non-chat-equivalent/lifecycle-only events: for the ResponsesStreamResponseTypeOutputItemAdded branch, return nil when rsr.Item == nil || rsr.Item.Type == nil and also return nil (instead of creating an empty BifrostResponseChoice) when rsr.Item.ResponsesToolMessage == nil or when the inner switch default (non-message/non-function types) is hit; for ResponsesStreamResponseTypeFunctionCallArgumentsDelta return nil when rsr.Delta == nil instead of emitting an empty delta; similarly replace the outer default branch that currently builds an empty ChatStreamResponseChoice with returning nil. Locate these changes around the switch handling ResponsesStreamResponseTypeOutputItemAdded, ResponsesStreamResponseTypeFunctionCallArgumentsDelta, and the outer default so you remove the empty BifrostResponseChoice/ChatStreamResponseChoice synthesis and return nil for lifecycle-only events.core/bifrost.go (1)
5127-5136:⚠️ Potential issue | 🟠 MajorReturn an error when chat→text mapping fails.
If
ToBifrostTextCompletionResponse()returnsnil, Line 610 still returnsnil, nil, so this converted path looks successful even though no payload was produced. Only exit this branch after a successful conversion.Suggested fix
if changeType == schemas.ChatCompletionRequest { chatRequest := req.BifrostRequest.TextCompletionRequest.ToBifrostChatRequest() if chatRequest != nil { chatCompletionResponse, bifrostError := provider.ChatCompletion(req.Context, key, chatRequest) if bifrostError != nil { return nil, bifrostError } - response.TextCompletionResponse = chatCompletionResponse.ToBifrostTextCompletionResponse() + convertedResponse := chatCompletionResponse.ToBifrostTextCompletionResponse() + if convertedResponse == nil { + return nil, newBifrostErrorFromMsg("failed to convert chat completion response to text completion response") + } + response.TextCompletionResponse = convertedResponse break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5127 - 5136, The branch handling schemas.ChatCompletionRequest must not return nil,nil when the chat->text conversion fails: after calling req.BifrostRequest.TextCompletionRequest.ToBifrostChatRequest() and provider.ChatCompletion(... ) you must check that chatCompletionResponse is non-nil and that chatCompletionResponse.ToBifrostTextCompletionResponse() returns a non-nil value before assigning response.TextCompletionResponse and breaking out; if the conversion returns nil, return a descriptive error (not nil, nil). Update the logic around changeType, ToBifrostChatRequest, provider.ChatCompletion, chatCompletionResponse, and ToBifrostTextCompletionResponse so the branch only exits on a successful non-nil conversion.
🤖 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 5145-5154: When handling ResponsesRequest in the changeType
branch, guard the conversion from ResponsesResponse to a Bifrost chat before
breaking: after calling provider.Responses (in the block using responsesRequest
:= req.BifrostRequest.ChatRequest.ToResponsesRequest() and responsesResponse,
bifrostError := provider.Responses(...)), validate that
responsesResponse.ToBifrostChatResponse() is non-nil before assigning it to
response.ChatResponse and executing the break; if the conversion returns nil,
return an appropriate error (or handle it similarly to bifrostError) instead of
proceeding to break and later passing a nil into agent mode.
In `@core/providers/replicate/types.go`:
- Line 33: Implement ReplicatePredictionRequest.GetParameterMappings to return a
map of Bifrost/OpenAI-compatible parameter names to the provider JSON paths
under "input" so dropUnsupportedParams can remove them for this provider;
specifically, in GetParameterMappings (type ReplicatePredictionRequest) map keys
like "temperature", "top_p", "max_tokens" (and other OpenAI-style knobs you
support such as "presence_penalty", "frequency_penalty", "stop", "n", etc.) to
"input.temperature", "input.top_p", "input.max_tokens", etc., ensuring
dropUnsupportedParams sees the provider JSON paths rather than Bifrost field
names.
In `@core/utils.go`:
- Around line 528-553: The switch in wrapConvertedStreamPostHookRunner is
checking the wrong request families (using schemas.ChatCompletionRequest and
schemas.ResponsesRequest), so post-hooks can see provider-native chunks; update
the cases to check the caller's original request family instead: use
schemas.TextCompletionRequest for the branch that converts
ChatResponse→TextCompletionResponse and use schemas.ChatCompletionRequest for
the branch that converts ResponsesStreamResponse→ChatResponse, leaving the
conversion logic on result.ChatResponse and result.ResponsesStreamResponse
unchanged so postHookRunner receives the public-type BifrostResponse.
In `@framework/modelcatalog/sync.go`:
- Around line 334-360: The rebuild reads mc.pricingData without synchronization
(at the lookup using makeKey, normalizeProvider, normalizeRequestType and
schemas.TextCompletionRequest) while other paths replace that map under mc.mu;
fix by taking a locked snapshot of the map once before iterating paramsData:
acquire mc.mu, copy or assign mc.pricingData to a local variable (e.g.
pricingSnapshot := mc.pricingData), then release mc.mu and use pricingSnapshot
for all lookups inside the loop (replace direct mc.pricingData references with
the local variable) so the rebuild is consistent and race-free.
In `@transports/bifrost-http/server/plugins.go`:
- Around line 104-109: The code currently only handles the renamed plugin key
compat.PluginName and will fail to load persisted configs still named
"litellmcompat"; update the plugin selection to accept both names by adding a
case for the old string "litellmcompat" (or check for either value before
calling MarshalPluginConfig[compat.Config]) and then proceed to call
MarshalPluginConfig[compat.Config] and compat.Init with the same arguments
(logger, bifrostConfig.ModelCatalog); apply the same change to the other
occurrence referenced (the block around the other case at lines 205-212) so both
places accept the legacy alias and continue to initialize via compat.Init.
---
Outside diff comments:
In `@core/utils.go`:
- Around line 504-512: In buildSessionKey replace the misspelled identifier
`provierKey` with the correct `providerKey` (the function parameter of type
schemas.ModelProvider) so the return expression uses string(providerKey); ensure
the concatenation still casts providerKey to string as originally intended to
fix the undefined identifier and allow the code to compile.
- Around line 194-199: The clearCtxForFallback function omits clearing
provider-specific compat state, causing fallback models to inherit dropped-param
lists; update clearCtxForFallback to also call ctx.ClearValue for
schemas.BifrostContextKeyCompatPluginDroppedParams so the compat-plugin dropped
params are removed before dispatching to a fallback model (i.e., add
ctx.ClearValue(schemas.BifrostContextKeyCompatPluginDroppedParams) alongside the
other ClearValue calls in clearCtxForFallback).
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 5127-5136: The branch handling schemas.ChatCompletionRequest must
not return nil,nil when the chat->text conversion fails: after calling
req.BifrostRequest.TextCompletionRequest.ToBifrostChatRequest() and
provider.ChatCompletion(... ) you must check that chatCompletionResponse is
non-nil and that chatCompletionResponse.ToBifrostTextCompletionResponse()
returns a non-nil value before assigning response.TextCompletionResponse and
breaking out; if the conversion returns nil, return a descriptive error (not
nil, nil). Update the logic around changeType, ToBifrostChatRequest,
provider.ChatCompletion, chatCompletionResponse, and
ToBifrostTextCompletionResponse so the branch only exits on a successful non-nil
conversion.
In `@core/providers/gemini/types.go`:
- Around line 114-115: The mapping for OpenAI params is inverted: in
core/providers/gemini/types.go swap the values for the keys "logprobs" and
"top_logprobs" so that "logprobs" maps to "generationConfig.responseLogprobs"
and "top_logprobs" maps to "generationConfig.logprobs"; update the entries where
those keys are defined (refer to the map containing "logprobs" and
"top_logprobs") so dropUnsupportedParams() and the compat conversion logic in
utils.go use the correct JSON paths.
In `@core/schemas/mux.go`:
- Around line 2258-2375: The helper converting chat→text (building
BifrostTextCompletionResponse) is hand-copying ExtraFields in multiple branches
and the fallback omits ProviderResponseHeaders, losing metadata; update every
return site that constructs ExtraFields (including the initial non-choice
return, the streaming branch that builds Choices, the non-streaming branch that
builds Choices, and the final fallback) to copy the entire cr.ExtraFields (or at
least include ProviderResponseHeaders and any other fields present on
cr.ExtraFields) instead of hand-copying a subset so the full metadata from
cr.ExtraFields is preserved in the BifrostTextCompletionResponse.
- Around line 2087-2243: Change branches that synthesize empty chat chunks into
returning nil for non-chat-equivalent/lifecycle-only events: for the
ResponsesStreamResponseTypeOutputItemAdded branch, return nil when rsr.Item ==
nil || rsr.Item.Type == nil and also return nil (instead of creating an empty
BifrostResponseChoice) when rsr.Item.ResponsesToolMessage == nil or when the
inner switch default (non-message/non-function types) is hit; for
ResponsesStreamResponseTypeFunctionCallArgumentsDelta return nil when rsr.Delta
== nil instead of emitting an empty delta; similarly replace the outer default
branch that currently builds an empty ChatStreamResponseChoice with returning
nil. Locate these changes around the switch handling
ResponsesStreamResponseTypeOutputItemAdded,
ResponsesStreamResponseTypeFunctionCallArgumentsDelta, and the outer default so
you remove the empty BifrostResponseChoice/ChatStreamResponseChoice synthesis
and return nil for lifecycle-only events.
In `@framework/modelcatalog/main.go`:
- Around line 883-905: IsRequestTypeSupported and GetSupportedParameters use the
raw model string for map lookups, so provider-prefixed IDs or aliases miss
cached entries; normalize the model key (e.g., call getBaseModelNameUnsafe or
ParseModelString to obtain the canonical base name) before accessing
mc.supportedResponseTypes and mc.supportedParams in IsRequestTypeSupported and
GetSupportedParameters, keeping the same RLock/RUnlock pattern and still
returning a copy of params to avoid external mutation.
- Around line 931-943: The loop over parsed.ModelParameters (in the block that
switches on mp.ID) misses common downstream aliases so supported params get
dropped; update the switch in that loop (where addParam is called for mp.ID) to
explicitly map aliases: when mp.ID == "max_tokens" call addParam("max_tokens")
and also addParam("max_completion_tokens") and addParam("max_output_tokens");
when mp.ID == "stop_sequences" call addParam("stop_sequences") and also
addParam("stop"); keep existing mappings for "reasoning_effort",
"reasoning_summary", "web_search", "promptTools", "image_detail", and "stream"
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3b9ba51-206a-4e16-a541-b4558e698170
⛔ Files ignored due to path filters (2)
plugins/compat/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
AGENTS.mdcore/bifrost.gocore/providers/anthropic/types.gocore/providers/bedrock/models.gocore/providers/bedrock/types.gocore/providers/cohere/models.gocore/providers/cohere/types.gocore/providers/elevenlabs/types.gocore/providers/gemini/types.gocore/providers/huggingface/types.gocore/providers/nebius/types.gocore/providers/openai/types.gocore/providers/perplexity/types.gocore/providers/replicate/types.gocore/providers/runway/types.gocore/providers/utils/utils.gocore/providers/vertex/models.gocore/providers/vertex/types.gocore/providers/vertex/vertex.gocore/providers/vllm/models.gocore/schemas/bifrost.gocore/schemas/chatcompletions.gocore/schemas/mux.gocore/utils.goframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.gonix/packages/bifrost-http.nixplugins/compat/changelog.mdplugins/compat/dropparams.goplugins/compat/go.modplugins/compat/main.goplugins/compat/versionplugins/litellmcompat/context.goplugins/litellmcompat/main.goplugins/litellmcompat/texttochat.gotransports/Dockerfile.localtransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.gotransports/go.mod
💤 Files with no reviewable changes (3)
- plugins/litellmcompat/context.go
- plugins/litellmcompat/texttochat.go
- plugins/litellmcompat/main.go
✅ Files skipped from review due to trivial changes (7)
- transports/bifrost-http/server/server.go
- nix/packages/bifrost-http.nix
- AGENTS.md
- plugins/compat/go.mod
- core/schemas/chatcompletions.go
- plugins/compat/dropparams.go
- core/providers/cohere/types.go
🚧 Files skipped from review as they are similar to previous changes (19)
- core/providers/vertex/models.go
- core/providers/cohere/models.go
- core/providers/nebius/types.go
- core/providers/runway/types.go
- core/providers/elevenlabs/types.go
- core/providers/perplexity/types.go
- transports/bifrost-http/lib/config.go
- framework/modelcatalog/utils.go
- core/providers/bedrock/models.go
- core/providers/vertex/types.go
- core/providers/vertex/vertex.go
- transports/bifrost-http/handlers/config.go
- core/providers/anthropic/types.go
- core/providers/bedrock/types.go
- core/providers/vllm/models.go
- plugins/compat/main.go
- core/providers/openai/types.go
- core/providers/huggingface/types.go
- transports/Dockerfile.local
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
ff71e29 to
ecdd5cb
Compare
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
323a630 to
257be14
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/compat/go.mod (1)
19-37:⚠️ Potential issue | 🔴 CriticalUpgrade AWS SDK S3 service to v1.97.3+ across the codebase to resolve GHSA-xmrv-pmrh-hhx2.
This DoS vulnerability in the EventStream header decoder affects
github.com/aws/aws-sdk-go-v2/service/s3versions below 1.97.3 and is currently blocking Dependency Review. The vulnerable v1.94.0 is present in 13 go.mod files across the repository (core, framework, transports, and 11 plugins). Additionally,tests/scripts/1millogs/go.modhas v1.93.2, which is also below the patched version. Coordinate an upgrade to v1.97.3+ across all affected modules before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/go.mod` around lines 19 - 37, The S3 SDK dependency is pinned to the vulnerable version "github.com/aws/aws-sdk-go-v2/service/s3 v1.94.0 // indirect" (and similar older versions in other go.mod files); update all modules that reference github.com/aws/aws-sdk-go-v2/service/s3 (including plugins/compat/go.mod and the other 12 go.mod files noted) to v1.97.3 or later, e.g. run go get github.com/aws/aws-sdk-go-v2/service/s3@v1.97.3 in each module, then run go mod tidy (and re-run tests/build) so indirect entries are refreshed; repeat for tests/scripts/1millogs/go.mod (currently v1.93.2) to ensure every go.mod uses >= v1.97.3 and remove any remaining older indirect s3 entries.
♻️ Duplicate comments (9)
framework/modelcatalog/main.go (1)
325-331:⚠️ Potential issue | 🟠 MajorNormalize stream request types before this lookup.
framework/modelcatalog/sync.go:301-369storessupportedResponseTypesin normalized base form, but this still compares the rawrequestType.text_completion_stream,chat_completion_stream, andresponses_streamwill miss even when the underlying capability exists, which can send streaming requests down the wrong compat path.🛠️ Proposed fix
func (mc *ModelCatalog) IsRequestTypeSupported(model string, provider schemas.ModelProvider, requestType schemas.RequestType) bool { + normalizedRequestType := normalizeRequestType(requestType) mc.mu.RLock() outputs, ok := mc.supportedResponseTypes[model] mc.mu.RUnlock() - return ok && slices.Contains(outputs, string(requestType)) + return ok && slices.Contains(outputs, normalizedRequestType) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 325 - 331, The lookup in IsRequestTypeSupported currently compares the raw schemas.RequestType to entries in supportedResponseTypes (stored in normalized base form); update IsRequestTypeSupported to first normalize stream variants to their base forms (e.g., map "text_completion_stream" -> "text_completion", "chat_completion_stream" -> "chat_completion", "responses_stream" -> "responses" or apply whatever normalization logic is used when populating supportedResponseTypes in sync.go) and then perform the slices.Contains check; reference the IsRequestTypeSupported method, the supportedResponseTypes map, and schemas.RequestType when implementing this normalization so streaming request types match the stored keys.core/bifrost.go (2)
5273-5278:⚠️ Potential issue | 🟠 MajorTreat an unmappable chat→text response as an error, not a nil success.
ToBifrostTextCompletionResponse()can still return nil even whenprovider.ChatCompletion()succeeded. Right now that leavesresponse.TextCompletionResponseunset andhandleProviderRequest()returns a successful-but-empty response. Please surface a descriptiveBifrostErrorwhen the conversion fails instead ofbreaking out as success.Suggested fix
chatCompletionResponse, bifrostError := provider.ChatCompletion(req.Context, key, chatRequest) if bifrostError != nil { return nil, bifrostError } - response.TextCompletionResponse = chatCompletionResponse.ToBifrostTextCompletionResponse() + converted := chatCompletionResponse.ToBifrostTextCompletionResponse() + if converted == nil { + return nil, newBifrostErrorFromMsg("failed to convert chat completion response to text completion response") + } + response.TextCompletionResponse = converted break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5273 - 5278, When converting a successful chat result to a text response, handle a nil conversion as an error: in the block that calls provider.ChatCompletion and then chatCompletionResponse.ToBifrostTextCompletionResponse(), detect if the conversion returned nil and return a descriptive BifrostError from handleProviderRequest (instead of setting nothing and breaking as a success). Update the branch that currently does "response.TextCompletionResponse = ...; break" so that if ToBifrostTextCompletionResponse() == nil you construct and return a BifrostError (include context like provider name/key and that chat→text mapping failed) rather than treating it as a successful empty response.
5270-5280:⚠️ Potential issue | 🟠 MajorDon't fall back to
TextCompletion*after compat explicitly selected chat dispatch.In large-payload passthrough mode,
TextCompletionRequestcan be valid withInput == nil, butToBifrostChatRequest()can still return nil for that shape. These branches then silently drop back toprovider.TextCompletion*, so/v1/completionsstill fails on chat-only providers even though compat asked forChatCompletion*. Return a conversion error here, or build a passthrough-safe chat request shell instead of calling the original text endpoint.Suggested fix
case schemas.TextCompletionRequest: if changeType, ok := req.Context.Value(schemas.BifrostContextKeyChangeRequestType).(schemas.RequestType); ok && changeType == schemas.ChatCompletionRequest { chatRequest := req.BifrostRequest.TextCompletionRequest.ToBifrostChatRequest() - if chatRequest != nil { - chatCompletionResponse, bifrostError := provider.ChatCompletion(req.Context, key, chatRequest) - if bifrostError != nil { - return nil, bifrostError - } - response.TextCompletionResponse = chatCompletionResponse.ToBifrostTextCompletionResponse() - break + if chatRequest == nil { + return nil, newBifrostErrorFromMsg("failed to convert text completion request to chat completion request") + } + chatCompletionResponse, bifrostError := provider.ChatCompletion(req.Context, key, chatRequest) + if bifrostError != nil { + return nil, bifrostError } + converted := chatCompletionResponse.ToBifrostTextCompletionResponse() + if converted == nil { + return nil, newBifrostErrorFromMsg("failed to convert chat completion response to text completion response") + } + response.TextCompletionResponse = converted + break }case schemas.TextCompletionStreamRequest: if changeType, ok := req.Context.Value(schemas.BifrostContextKeyChangeRequestType).(schemas.RequestType); ok && changeType == schemas.ChatCompletionRequest { chatRequest := req.BifrostRequest.TextCompletionRequest.ToBifrostChatRequest() - if chatRequest != nil { - return provider.ChatCompletionStream(req.Context, wrapConvertedStreamPostHookRunner(postHookRunner, schemas.ChatCompletionRequest), key, chatRequest) + if chatRequest == nil { + return nil, newBifrostErrorFromMsg("failed to convert text completion stream request to chat completion stream request") } + return provider.ChatCompletionStream(req.Context, wrapConvertedStreamPostHookRunner(postHookRunner, schemas.ChatCompletionRequest), key, chatRequest) }Based on learnings, Groq, Cerebras, Ollama, Perplexity, OpenRouter, Parasail, Nebius, xAI, and SGL providers delegate to
openai.HandleOpenAI*functions - changes cascade.Also applies to: 5547-5552
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5270 - 5280, The branch that checks req.Context.Value(schemas.BifrostContextKeyChangeRequestType) for schemas.ChatCompletionRequest must not silently fall back to text endpoints when TextCompletionRequest.ToBifrostChatRequest() returns nil; instead, detect nil from TextCompletionRequest.ToBifrostChatRequest() and return a clear conversion error (or construct a passthrough-safe chat request shell) so we call provider.ChatCompletion only with a valid chat request rather than letting execution drop into provider.TextCompletion* and set response.TextCompletionResponse via ToBifrostTextCompletionResponse(); update the logic around ToBifrostChatRequest(), provider.ChatCompletion, and response.TextCompletionResponse to enforce this explicit error or safe-shell behavior.framework/configstore/migrations.go (2)
6119-6131:⚠️ Potential issue | 🔴 CriticalRefresh
config_hashafter the compat backfill.This migration updates persisted
compat_*fields onconfig_clientbut never recomputesconfig_hash. Existing rows will keep stale hashes and be treated as dirty on the next reconciliation pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6119 - 6131, The migration updates compat_* columns but doesn't recompute config_hash, leaving rows with stale hashes; after the compat backfill (the tx.Exec calls that update compat_should_convert_params and compat_convert_text_to_chat and after the optional mig.DropColumn on tables.TableClientConfig), recompute and persist the new config_hash for all affected config_client rows (run an UPDATE to set config_hash = <recompute function/result> or call the existing hash computation routine) so that reconciler no longer treats them as dirty; ensure this runs within the same transaction (tx) and uses the same identifying rows updated by the compat backfill logic.
6135-6157:⚠️ Potential issue | 🔴 CriticalFix the rollback order for
enable_litellm_fallbacks.The rollback branch only runs when
enable_litellm_fallbacksalready exists, then drops it before trying to populate it. In the normal post-migration state the legacy column is absent, so rollback never recreates or restores it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6135 - 6157, The rollback branch currently checks for enable_litellm_fallbacks and drops it before attempting to populate it, so when the legacy column is absent the rollback never recreates/restores it; change the order in the Rollback function to first ensure the legacy column enable_litellm_fallbacks exists (use mig.HasColumn and mig.AddColumn as needed), then populate it from compat_convert_text_to_chat with tx.Exec("UPDATE config_client SET enable_litellm_fallbacks = compat_convert_text_to_chat"), and only after that drop the compat_* columns (using mig.DropColumn for compat_convert_text_to_chat, compat_convert_chat_to_responses, compat_should_drop_params, compat_should_convert_params) so the data restoration occurs before removal of the source columns.core/schemas/mux.go (3)
2258-2274:⚠️ Potential issue | 🟠 MajorPreserve full
ExtraFieldsduring chat→text conversion.These branches rebuild
BifrostResponseExtraFieldsfrom a subset, which can drop metadata (including conversion/debug fields) already present oncr.ExtraFields.♻️ Suggested fix
func (cr *BifrostChatResponse) ToBifrostTextCompletionResponse() *BifrostTextCompletionResponse { if cr == nil { return nil } + extra := cr.ExtraFields + extra.RequestType = TextCompletionRequest if len(cr.Choices) == 0 { return &BifrostTextCompletionResponse{ // ... - ExtraFields: BifrostResponseExtraFields{ - RequestType: TextCompletionRequest, - ChunkIndex: cr.ExtraFields.ChunkIndex, - Provider: cr.ExtraFields.Provider, - OriginalModelRequested: cr.ExtraFields.OriginalModelRequested, - Latency: cr.ExtraFields.Latency, - RawResponse: cr.ExtraFields.RawResponse, - CacheDebug: cr.ExtraFields.CacheDebug, - ProviderResponseHeaders: cr.ExtraFields.ProviderResponseHeaders, - }, + ExtraFields: extra, } } // Apply same `ExtraFields: extra` replacement in all return branches below.Also applies to: 2298-2307, 2347-2356, 2367-2376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2258 - 2274, The code paths that convert chat responses to BifrostTextCompletionResponse are reconstructing BifrostResponseExtraFields from a subset and thus dropping metadata; instead assign the entire existing cr.ExtraFields when building the response (e.g., set ExtraFields: cr.ExtraFields) in the BifrostTextCompletionResponse construction for the branch handling len(cr.Choices) == 0 (and similarly update the other similar branches around the BifrostTextCompletionResponse creation at the other noted locations), ensuring any conversion/debug fields are preserved.
2086-2111:⚠️ Potential issue | 🟠 MajorAvoid emitting synthetic empty chat chunks for non-chat-equivalent events.
These branches fabricate empty
chat.completion.chunkdeltas for lifecycle/unsupported events. That introduces ghost SSE chunks for chat clients.♻️ Suggested fix
case ResponsesStreamResponseTypeOutputItemAdded: if rsr.Item == nil || rsr.Item.Type == nil { - resp.Choices = []BifrostResponseChoice{{ ... empty delta ... }} - return resp + return nil } switch *rsr.Item.Type { case ResponsesMessageTypeFunctionCall: if rsr.Item.ResponsesToolMessage == nil { - resp.Choices = []BifrostResponseChoice{{ ... empty delta ... }} - return resp + return nil } // ... default: - resp.Choices = []BifrostResponseChoice{{ ... empty delta ... }} - return resp + return nil } case ResponsesStreamResponseTypeFunctionCallArgumentsDelta: if rsr.Delta == nil { - resp.Choices = []BifrostResponseChoice{{ ... empty delta ... }} - return resp + return nil } // ... default: - resp.Choices = []BifrostResponseChoice{{ ... empty delta ... }} - return resp + return nilAlso applies to: 2152-2164, 2167-2177, 2233-2244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2086 - 2111, The code is emitting synthetic empty ChatStreamResponseChoice deltas for non-chat-equivalent events (e.g. in the ResponsesStreamResponseTypeOutputItemAdded branch when rsr.Item == nil || rsr.Item.Type == nil and when *rsr.Item.Type == ResponsesMessageTypeFunctionCall but rsr.Item.ResponsesToolMessage == nil); remove the construction of those fake empty deltas and instead skip emitting a chat completion chunk for these cases (i.e., do not set resp.Choices/return a chat chunk) so the SSE stream does not produce ghost chat chunks; update the corresponding branches (the nil-type guard, the ResponsesMessageTypeFunctionCall branch, and the similar cases at the other noted branches) to simply no-op/continue or return nil/non-chat response as appropriate rather than fabricating an empty ChatStreamResponseChoice.
2281-2295:⚠️ Potential issue | 🟠 MajorSkip text-completion stream chunks when there is no text delta and no terminal reason.
Current conversion emits empty chunks for role/tool-only chat deltas.
♻️ Suggested fix
// Handle streaming response choice if choice.ChatStreamResponseChoice != nil && choice.ChatStreamResponseChoice.Delta != nil { + deltaContent := choice.ChatStreamResponseChoice.Delta.Content + if (deltaContent == nil || *deltaContent == "") && choice.FinishReason == nil { + return nil + } + return &BifrostTextCompletionResponse{ // ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mux.go` around lines 2281 - 2295, The conversion currently emits empty text-completion chunks when choice.ChatStreamResponseChoice.Delta.Content is empty and there is no terminal reason; update the converter in mux.go to short-circuit/skip returning a BifrostTextCompletionResponse when choice.ChatStreamResponseChoice != nil && choice.ChatStreamResponseChoice.Delta != nil and both choice.ChatStreamResponseChoice.Delta.Content is empty (or only whitespace) AND choice.FinishReason is empty/nil — i.e., add a guard before constructing the BifrostTextCompletionResponse that returns nil (or continues) for non-terminal, content-less deltas so role/tool-only chat deltas aren’t emitted as empty chunks.plugins/compat/main.go (1)
33-37:⚠️ Potential issue | 🔴 CriticalRequest-scoped dropped-param state is shared across requests and can leak/race.
p.droppedParamsis mutable plugin-level state, but it carries per-request data. Under concurrency (and fallback re-entry), this can leak metadata between requests and attach stale values inPostLLMHook. Also, dropped params are not attached on error-only paths.♻️ Suggested fix
type CompatPlugin struct { config Config logger schemas.Logger modelCatalog *modelcatalog.ModelCatalog - droppedParams []string } func (p *CompatPlugin) PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.LLMPluginShortCircuit, error) { if ctx == nil || req == nil { return req, nil, nil } modifiedReq := cloneBifrostReq(req) - p.droppedParams = nil + ctx.ClearValue(schemas.BifrostContextKeyChangeRequestType) + ctx.ClearValue(schemas.BifrostContextKeyCompatPluginDroppedParams) // ... if p.config.ShouldDropParams && p.modelCatalog != nil { _, model, _ := modifiedReq.GetRequestFields() if model != "" { if supportedParams := p.modelCatalog.GetSupportedParameters(model); supportedParams != nil { droppedParams := dropUnsupportedParams(modifiedReq, supportedParams) if len(droppedParams) > 0 { - p.droppedParams = droppedParams + ctx.SetValue(schemas.BifrostContextKeyCompatPluginDroppedParams, droppedParams) } } } } // ... } func (p *CompatPlugin) PostLLMHook(ctx *schemas.BifrostContext, result *schemas.BifrostResponse, bifrostErr *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError, error) { if ctx == nil { return result, bifrostErr, nil } + if droppedParams, ok := ctx.Value(schemas.BifrostContextKeyCompatPluginDroppedParams).([]string); ok && len(droppedParams) > 0 { + if result != nil { + if extraFields := result.GetExtraFields(); extraFields != nil { + extraFields.DroppedCompatPluginParams = droppedParams + } + } + if bifrostErr != nil { + bifrostErr.ExtraFields.DroppedCompatPluginParams = droppedParams + } + } - - if result != nil { - if extraFields := result.GetExtraFields(); extraFields != nil { - extraFields.DroppedCompatPluginParams = p.droppedParams - } - }Based on learnings: Fallbacks re-execute the full plugin pipeline from scratch, so request-scoped plugin metadata must be recomputed from clean context state each attempt.
Also applies to: 78-103, 133-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/main.go` around lines 33 - 37, p.droppedParams is shared mutable plugin-level state but carries per-request data causing leaks/races; remove the plugin field and make droppedParams request-scoped by computing it fresh inside the request lifecycle (e.g., in the PreLLMHook/OnLLMStart handler that currently derives it) and pass it via the hook context or return value so PostLLMHook and error paths can read the per-request value; update PostLLMHook (and any error-handling hooks) to consume the per-request droppedParams from the provided context/state rather than p.droppedParams, ensuring it is recomputed for each fallback retry and not retained across requests.
🧹 Nitpick comments (3)
examples/dockers/data/config.json (1)
30-32: Consider enabling the conversion flag that preserves the old fallback behavior.This example turns on only
should_convert_params. Intransports/bifrost-http/server/plugins.go:290-303, the compat plugin checks each flag independently, so/v1/completions→ chat fallback is still off unlessconvert_text_to_chatis also enabled. If this file is meant to replace the previousenable_litellm_fallbacks: trueexample, it no longer demonstrates equivalent behavior.💡 Possible adjustment
"compat": { + "convert_text_to_chat": true, "should_convert_params": true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/dockers/data/config.json` around lines 30 - 32, The current config enables only the compat flag should_convert_params which leaves the old chat fallback off; update the compat block to also enable convert_text_to_chat (and any other legacy fallback flags used by the compat plugin) so that the behavior matches the previous enable_litellm_fallbacks: true example—look for the compat plugin logic (transports/bifrost-http/server/plugins.go checks flags like should_convert_params and convert_text_to_chat) and set both flags in the JSON so /v1/completions → chat fallback is preserved..github/workflows/scripts/test-docker-image.sh (1)
208-215: This smoke config no longer exercises compat wiring.With no
client.compat.*flags set here,transports/bifrost-http/server/plugins.go:290-303disables the built-in compat plugin entirely. Since this PR changes compat behavior, consider turning on one representative compat flag in this fixture so the docker-image smoke path at least loads that code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/scripts/test-docker-image.sh around lines 208 - 215, The smoke test config currently omits any client.compat.* flags so the built-in compat plugin is disabled; update the JSON under the "client" block in the test-docker-image.sh fixture to enable one representative compat flag (for example add a client.compat.* entry such as "compat": { "enable_legacy_headers": true } or "compat": { "allow_legacy_auth": true }) so the compat plugin registration code path (the built-in compat plugin) is exercised during the docker-image smoke run; ensure the new key uses the existing client.compat namespace and valid boolean value so the plugin loader sees a compat flag and loads the compat plugin.plugins/compat/requestcopy.go (1)
339-350: Avoid JSON round-tripping eachResponsesToolin the clone path.
MarshalSorted+Unmarshalmakes request copying pay a per-tool serialization cost on the compat hot path, and thereturn toolfallback silently drops back to shared state if the round-trip fails. Prefer a typed copier for the concreteResponsesToolvariants instead of serialize/deserialize cloning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/requestcopy.go` around lines 339 - 350, The cloneResponsesTool function currently deep-clones a schemas.ResponsesTool by calling schemas.MarshalSorted and schemas.Unmarshal, which is expensive and falls back to returning the original tool on error (leaving shared mutable state); replace this JSON round-trip with a typed copier: implement a switch on the concrete variants of schemas.ResponsesTool (match the concrete types used in your codebase), allocate a new instance for each variant, and copy all fields (slices/maps must be deep-copied) so you always return an independent cloned value from cloneResponsesTool; remove the MarshalSorted/Unmarshal usage and ensure there is no silent fallback to returning the input tool on copy failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openapi/schemas/management/config.yaml`:
- Around line 47-63: The ClientConfig.compat schema in
docs/openapi/schemas/management/config.yaml is missing the default for
should_convert_params and doesn't forbid extra keys; update the compat object to
match the authoritative transports/config.schema.json by adding default: true to
the should_convert_params property and adding additionalProperties: false under
compat so no unexpected keys are allowed, ensuring the schema for
ClientConfig.compat stays in parity with the canonical config.
In `@ui/app/workspace/config/views/clientSettingsView.tsx`:
- Line 366: Fix the typo on the four Switch components in clientSettingsView.tsx
where the attribute is incorrectly spelled "datatest-id"; change each to the
correct HTML data attribute "data-testid" following the project's convention
(e.g., data-testid="<entity>-<element>-<qualifier>"). Update the occurrences
referenced around the Switch uses (the lines near the
"compat-convert-text-to-chat" Switch and the other three instances) so E2E tests
can locate them.
---
Outside diff comments:
In `@plugins/compat/go.mod`:
- Around line 19-37: The S3 SDK dependency is pinned to the vulnerable version
"github.com/aws/aws-sdk-go-v2/service/s3 v1.94.0 // indirect" (and similar older
versions in other go.mod files); update all modules that reference
github.com/aws/aws-sdk-go-v2/service/s3 (including plugins/compat/go.mod and the
other 12 go.mod files noted) to v1.97.3 or later, e.g. run go get
github.com/aws/aws-sdk-go-v2/service/s3@v1.97.3 in each module, then run go mod
tidy (and re-run tests/build) so indirect entries are refreshed; repeat for
tests/scripts/1millogs/go.mod (currently v1.93.2) to ensure every go.mod uses >=
v1.97.3 and remove any remaining older indirect s3 entries.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 5273-5278: When converting a successful chat result to a text
response, handle a nil conversion as an error: in the block that calls
provider.ChatCompletion and then
chatCompletionResponse.ToBifrostTextCompletionResponse(), detect if the
conversion returned nil and return a descriptive BifrostError from
handleProviderRequest (instead of setting nothing and breaking as a success).
Update the branch that currently does "response.TextCompletionResponse = ...;
break" so that if ToBifrostTextCompletionResponse() == nil you construct and
return a BifrostError (include context like provider name/key and that chat→text
mapping failed) rather than treating it as a successful empty response.
- Around line 5270-5280: The branch that checks
req.Context.Value(schemas.BifrostContextKeyChangeRequestType) for
schemas.ChatCompletionRequest must not silently fall back to text endpoints when
TextCompletionRequest.ToBifrostChatRequest() returns nil; instead, detect nil
from TextCompletionRequest.ToBifrostChatRequest() and return a clear conversion
error (or construct a passthrough-safe chat request shell) so we call
provider.ChatCompletion only with a valid chat request rather than letting
execution drop into provider.TextCompletion* and set
response.TextCompletionResponse via ToBifrostTextCompletionResponse(); update
the logic around ToBifrostChatRequest(), provider.ChatCompletion, and
response.TextCompletionResponse to enforce this explicit error or safe-shell
behavior.
In `@core/schemas/mux.go`:
- Around line 2258-2274: The code paths that convert chat responses to
BifrostTextCompletionResponse are reconstructing BifrostResponseExtraFields from
a subset and thus dropping metadata; instead assign the entire existing
cr.ExtraFields when building the response (e.g., set ExtraFields:
cr.ExtraFields) in the BifrostTextCompletionResponse construction for the branch
handling len(cr.Choices) == 0 (and similarly update the other similar branches
around the BifrostTextCompletionResponse creation at the other noted locations),
ensuring any conversion/debug fields are preserved.
- Around line 2086-2111: The code is emitting synthetic empty
ChatStreamResponseChoice deltas for non-chat-equivalent events (e.g. in the
ResponsesStreamResponseTypeOutputItemAdded branch when rsr.Item == nil ||
rsr.Item.Type == nil and when *rsr.Item.Type == ResponsesMessageTypeFunctionCall
but rsr.Item.ResponsesToolMessage == nil); remove the construction of those fake
empty deltas and instead skip emitting a chat completion chunk for these cases
(i.e., do not set resp.Choices/return a chat chunk) so the SSE stream does not
produce ghost chat chunks; update the corresponding branches (the nil-type
guard, the ResponsesMessageTypeFunctionCall branch, and the similar cases at the
other noted branches) to simply no-op/continue or return nil/non-chat response
as appropriate rather than fabricating an empty ChatStreamResponseChoice.
- Around line 2281-2295: The conversion currently emits empty text-completion
chunks when choice.ChatStreamResponseChoice.Delta.Content is empty and there is
no terminal reason; update the converter in mux.go to short-circuit/skip
returning a BifrostTextCompletionResponse when choice.ChatStreamResponseChoice
!= nil && choice.ChatStreamResponseChoice.Delta != nil and both
choice.ChatStreamResponseChoice.Delta.Content is empty (or only whitespace) AND
choice.FinishReason is empty/nil — i.e., add a guard before constructing the
BifrostTextCompletionResponse that returns nil (or continues) for non-terminal,
content-less deltas so role/tool-only chat deltas aren’t emitted as empty
chunks.
In `@framework/configstore/migrations.go`:
- Around line 6119-6131: The migration updates compat_* columns but doesn't
recompute config_hash, leaving rows with stale hashes; after the compat backfill
(the tx.Exec calls that update compat_should_convert_params and
compat_convert_text_to_chat and after the optional mig.DropColumn on
tables.TableClientConfig), recompute and persist the new config_hash for all
affected config_client rows (run an UPDATE to set config_hash = <recompute
function/result> or call the existing hash computation routine) so that
reconciler no longer treats them as dirty; ensure this runs within the same
transaction (tx) and uses the same identifying rows updated by the compat
backfill logic.
- Around line 6135-6157: The rollback branch currently checks for
enable_litellm_fallbacks and drops it before attempting to populate it, so when
the legacy column is absent the rollback never recreates/restores it; change the
order in the Rollback function to first ensure the legacy column
enable_litellm_fallbacks exists (use mig.HasColumn and mig.AddColumn as needed),
then populate it from compat_convert_text_to_chat with tx.Exec("UPDATE
config_client SET enable_litellm_fallbacks = compat_convert_text_to_chat"), and
only after that drop the compat_* columns (using mig.DropColumn for
compat_convert_text_to_chat, compat_convert_chat_to_responses,
compat_should_drop_params, compat_should_convert_params) so the data restoration
occurs before removal of the source columns.
In `@framework/modelcatalog/main.go`:
- Around line 325-331: The lookup in IsRequestTypeSupported currently compares
the raw schemas.RequestType to entries in supportedResponseTypes (stored in
normalized base form); update IsRequestTypeSupported to first normalize stream
variants to their base forms (e.g., map "text_completion_stream" ->
"text_completion", "chat_completion_stream" -> "chat_completion",
"responses_stream" -> "responses" or apply whatever normalization logic is used
when populating supportedResponseTypes in sync.go) and then perform the
slices.Contains check; reference the IsRequestTypeSupported method, the
supportedResponseTypes map, and schemas.RequestType when implementing this
normalization so streaming request types match the stored keys.
In `@plugins/compat/main.go`:
- Around line 33-37: p.droppedParams is shared mutable plugin-level state but
carries per-request data causing leaks/races; remove the plugin field and make
droppedParams request-scoped by computing it fresh inside the request lifecycle
(e.g., in the PreLLMHook/OnLLMStart handler that currently derives it) and pass
it via the hook context or return value so PostLLMHook and error paths can read
the per-request value; update PostLLMHook (and any error-handling hooks) to
consume the per-request droppedParams from the provided context/state rather
than p.droppedParams, ensuring it is recomputed for each fallback retry and not
retained across requests.
---
Nitpick comments:
In @.github/workflows/scripts/test-docker-image.sh:
- Around line 208-215: The smoke test config currently omits any client.compat.*
flags so the built-in compat plugin is disabled; update the JSON under the
"client" block in the test-docker-image.sh fixture to enable one representative
compat flag (for example add a client.compat.* entry such as "compat": {
"enable_legacy_headers": true } or "compat": { "allow_legacy_auth": true }) so
the compat plugin registration code path (the built-in compat plugin) is
exercised during the docker-image smoke run; ensure the new key uses the
existing client.compat namespace and valid boolean value so the plugin loader
sees a compat flag and loads the compat plugin.
In `@examples/dockers/data/config.json`:
- Around line 30-32: The current config enables only the compat flag
should_convert_params which leaves the old chat fallback off; update the compat
block to also enable convert_text_to_chat (and any other legacy fallback flags
used by the compat plugin) so that the behavior matches the previous
enable_litellm_fallbacks: true example—look for the compat plugin logic
(transports/bifrost-http/server/plugins.go checks flags like
should_convert_params and convert_text_to_chat) and set both flags in the JSON
so /v1/completions → chat fallback is preserved.
In `@plugins/compat/requestcopy.go`:
- Around line 339-350: The cloneResponsesTool function currently deep-clones a
schemas.ResponsesTool by calling schemas.MarshalSorted and schemas.Unmarshal,
which is expensive and falls back to returning the original tool on error
(leaving shared mutable state); replace this JSON round-trip with a typed
copier: implement a switch on the concrete variants of schemas.ResponsesTool
(match the concrete types used in your codebase), allocate a new instance for
each variant, and copy all fields (slices/maps must be deep-copied) so you
always return an independent cloned value from cloneResponsesTool; remove the
MarshalSorted/Unmarshal usage and ensure there is no silent fallback to
returning the input tool on copy failure.
🪄 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: 13ad093c-ce0a-474a-b092-9d0858c4db21
⛔ Files ignored due to path filters (2)
plugins/compat/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (65)
.github/workflows/configs/withpostgresmcpclientsinconfig/config.json.github/workflows/scripts/run-migration-tests.sh.github/workflows/scripts/test-docker-image.sh.github/workflows/scripts/validate-helm-config-fields.shAGENTS.mdcore/bifrost.gocore/providers/anthropic/types.gocore/providers/bedrock/images.gocore/providers/bedrock/models.gocore/providers/cohere/types.gocore/providers/gemini/types.gocore/providers/openai/types.gocore/providers/perplexity/types.gocore/providers/replicate/types.gocore/providers/utils/utils.gocore/providers/vertex/models.gocore/providers/vertex/types.gocore/providers/vertex/vertex.gocore/schemas/bifrost.gocore/schemas/chatcompletions.gocore/schemas/mux.gocore/utils.godocs/features/litellm-compat.mdxdocs/openapi/openapi.jsondocs/openapi/schemas/management/config.yamldocs/providers/supported-providers/overview.mdxexamples/configs/withpostgresmcpclientsinconfig/config.jsonexamples/configs/withprompushgateway/config.jsonexamples/configs/withvirtualkeys/config.jsonexamples/dockers/data/config.jsonframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/go.modframework/modelcatalog/main.goframework/modelcatalog/sync.goframework/modelcatalog/utils.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlnix/packages/bifrost-http.nixplugins/compat/changelog.mdplugins/compat/conversion.goplugins/compat/dropparams.goplugins/compat/go.modplugins/compat/main.goplugins/compat/requestcopy.goplugins/compat/versionplugins/litellmcompat/context.goplugins/litellmcompat/main.goplugins/litellmcompat/texttochat.gotests/governance/config.jsontests/integrations/python/config.jsontests/integrations/typescript/config.jsontransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.gotransports/config.schema.jsontransports/go.modui/app/workspace/config/views/clientSettingsView.tsxui/components/ui/accordion.tsxui/lib/types/config.ts
💤 Files with no reviewable changes (7)
- examples/configs/withpostgresmcpclientsinconfig/config.json
- core/providers/bedrock/images.go
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
- examples/configs/withvirtualkeys/config.json
- plugins/litellmcompat/context.go
- plugins/litellmcompat/texttochat.go
- plugins/litellmcompat/main.go
✅ Files skipped from review due to trivial changes (24)
- plugins/compat/version
- core/providers/perplexity/types.go
- ui/components/ui/accordion.tsx
- transports/bifrost-http/server/server.go
- core/providers/vertex/types.go
- plugins/compat/changelog.md
- docs/providers/supported-providers/overview.mdx
- nix/packages/bifrost-http.nix
- examples/configs/withprompushgateway/config.json
- tests/integrations/typescript/config.json
- core/providers/anthropic/types.go
- tests/governance/config.json
- core/providers/bedrock/models.go
- core/providers/openai/types.go
- core/providers/replicate/types.go
- core/providers/cohere/types.go
- core/providers/vertex/vertex.go
- framework/go.mod
- core/providers/utils/utils.go
- core/providers/vertex/models.go
- tests/integrations/python/config.json
- plugins/compat/conversion.go
- plugins/compat/dropparams.go
- framework/modelcatalog/utils.go
🚧 Files skipped from review as they are similar to previous changes (14)
- AGENTS.md
- transports/go.mod
- helm-charts/bifrost/templates/_helpers.tpl
- transports/bifrost-http/lib/config.go
- docs/features/litellm-compat.mdx
- helm-charts/bifrost/values.yaml
- core/providers/gemini/types.go
- ui/lib/types/config.ts
- framework/configstore/tables/clientconfig.go
- core/utils.go
- framework/configstore/rdb.go
- core/schemas/bifrost.go
- framework/modelcatalog/sync.go
- transports/bifrost-http/lib/config_test.go
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
257be14 to
9c3d16c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
transports/go.mod (1)
188-188: Consider restoring the trailing newline.The trailing newline at the end of the file was removed. While not required, it's conventional in POSIX environments and many tools expect it. Running
go mod tidyin thetransportsdirectory should automatically restore it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/go.mod` at line 188, Restore the missing trailing newline at the end of the file (i.e., add a newline after the final ')' token) and then run go mod tidy in the module to let Go tooling normalize the file and re-add any canonical EOF newline.docs/openapi/openapi.json (1)
133224-133232: Hardencompatschema by disallowing unknown keys.
compatcurrently accepts arbitrary properties, so typos can silently pass validation. Add"additionalProperties": false(in the source schema that generates this file) to make config validation strict.Proposed schema patch
"compat": { "type": "object", "description": "Compat plugin configuration", + "additionalProperties": false, "properties": { "convert_text_to_chat": { "type": "boolean", "description": "Convert text completion requests to chat" }, "convert_chat_to_responses": { "type": "boolean", "description": "Convert chat completion requests to responses" }, "should_drop_params": { "type": "boolean", "description": "Drop unsupported parameters based on model catalog" }, "should_convert_params": { "type": "boolean", "description": "Converts model parameter values that are not supported by the model.", "default": true } } }Based on learnings: In
docs/openapi/openapi.json, keep consistency with the generated-spec pattern and align with the ref-based schema structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 133224 - 133232, The compat schema currently allows arbitrary keys; update the source schema that generates the OpenAPI spec so the "compat" object schema includes "additionalProperties": false to forbid unknown properties (locate the schema block named "compat" in the OpenAPI/source schema and add additionalProperties: false alongside its existing "type": "object" and "properties" definitions); ensure this change is propagated so docs/openapi/openapi.json is regenerated and the compat block (with properties like convert_text_to_chat, convert_chat_to_responses, should_drop_params, should_convert_params) now rejects unknown keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/go.mod`:
- Around line 17-19: Remove the duplicate module line
"github.com/maximhq/bifrost/plugins/compat v0.1.0" from the require block (leave
a single entry for github.com/maximhq/bifrost/plugins/compat v0.1.0) and then
run go mod tidy in the transports module to update go.sum and ensure the module
file is consistent.
- Line 18: Remove the duplicate module line for
github.com/maximhq/bifrost/plugins/compat in transports/go.mod and update or
confirm plugin versions: bump github.com/maximhq/bifrost/plugins/governance,
/logging, /semanticcache, and /telemetry from v1.5.0 to v1.5.1, bump
github.com/maximhq/bifrost/plugins/maxim from v1.6.0 to v1.6.1, and bump
github.com/maximhq/bifrost/plugins/otel from v1.2.0 to v1.2.1 unless the older
versions are intentionally pinned for the litellmcompat→compat migration—if they
are pinned, add a brief comment explaining the intentional pinning and the
migration status.
---
Nitpick comments:
In `@docs/openapi/openapi.json`:
- Around line 133224-133232: The compat schema currently allows arbitrary keys;
update the source schema that generates the OpenAPI spec so the "compat" object
schema includes "additionalProperties": false to forbid unknown properties
(locate the schema block named "compat" in the OpenAPI/source schema and add
additionalProperties: false alongside its existing "type": "object" and
"properties" definitions); ensure this change is propagated so
docs/openapi/openapi.json is regenerated and the compat block (with properties
like convert_text_to_chat, convert_chat_to_responses, should_drop_params,
should_convert_params) now rejects unknown keys.
In `@transports/go.mod`:
- Line 188: Restore the missing trailing newline at the end of the file (i.e.,
add a newline after the final ')' token) and then run go mod tidy in the module
to let Go tooling normalize the file and re-add any canonical EOF newline.
🪄 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: 7175f9d3-2bf2-4486-850c-bcc17d017cca
⛔ Files ignored due to path filters (1)
transports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
.github/workflows/configs/withpostgresmcpclientsinconfig/config.json.github/workflows/scripts/run-migration-tests.sh.github/workflows/scripts/test-docker-image.sh.github/workflows/scripts/validate-helm-config-fields.shdocs/features/litellm-compat.mdxdocs/openapi/openapi.jsondocs/openapi/schemas/management/config.yamldocs/providers/supported-providers/overview.mdxexamples/configs/withpostgresmcpclientsinconfig/config.jsonexamples/configs/withprompushgateway/config.jsonexamples/configs/withvirtualkeys/config.jsonexamples/dockers/data/config.jsonframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/modelcatalog/main.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/conversion.goplugins/compat/main.gotests/governance/config.jsontests/integrations/python/config.jsontests/integrations/typescript/config.jsontransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/plugins.gotransports/config.schema.jsontransports/go.modui/app/workspace/config/views/clientSettingsView.tsxui/components/ui/accordion.tsxui/lib/types/config.ts
💤 Files with no reviewable changes (3)
- examples/configs/withpostgresmcpclientsinconfig/config.json
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
- examples/configs/withvirtualkeys/config.json
✅ Files skipped from review due to trivial changes (8)
- tests/integrations/typescript/config.json
- .github/workflows/scripts/test-docker-image.sh
- docs/providers/supported-providers/overview.mdx
- tests/integrations/python/config.json
- plugins/compat/conversion.go
- transports/bifrost-http/server/plugins.go
- framework/configstore/migrations.go
- transports/bifrost-http/lib/config_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- examples/dockers/data/config.json
- tests/governance/config.json
- examples/configs/withprompushgateway/config.json
- ui/components/ui/accordion.tsx
- docs/features/litellm-compat.mdx
- ui/lib/types/config.ts
- framework/configstore/tables/clientconfig.go
- transports/config.schema.json
- transports/bifrost-http/handlers/config.go
- framework/configstore/rdb.go
- ui/app/workspace/config/views/clientSettingsView.tsx
👮 Files not reviewed due to content moderation or server errors (5)
- docs/openapi/schemas/management/config.yaml
- helm-charts/bifrost/values.schema.json
- framework/configstore/clientconfig.go
- framework/modelcatalog/main.go
- plugins/compat/main.go
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
9c3d16c to
a9eac97
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/features/litellm-compat.mdx (1)
98-115:⚠️ Potential issue | 🟡 MinorDocument the fourth compat flag here too.
The implementation exposes
should_convert_params, but this page only describes three compat toggles. That makes one live behavior undiscoverable and leaves the JSON example incomplete.📝 Suggested doc update
3. Expand **LiteLLM Compat** and enable the features you need: - **Convert Text to Chat** — converts text completion requests to chat for models that only support chat - **Convert Chat to Responses** — converts chat completion requests to responses for models that only support responses - **Drop Unsupported Params** — drops unsupported parameters based on model catalog allowlist + - **Convert Params** — normalizes request fields for provider compatibility ... "compat": { "convert_text_to_chat": true, "convert_chat_to_responses": true, - "should_drop_params": true + "should_drop_params": true, + "should_convert_params": true }As per coding guidelines:
docs/**/*.mdx: Documentation uses Mintlify MDX format with Web UI / API / config.json tabs; examples must match config.schema.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/litellm-compat.mdx` around lines 98 - 115, The docs page omits the fourth compat toggle that the code exposes (should_convert_params); update the feature list and the JSON example to include and describe "Convert/Convert Params" (referencing should_convert_params) alongside convert_text_to_chat, convert_chat_to_responses, and should_drop_params, and ensure the example JSON matches the schema in config.schema.json by adding "should_convert_params": true with a brief one-line description of its behavior.
♻️ Duplicate comments (2)
.github/workflows/scripts/run-migration-tests.sh (1)
545-546:⚠️ Potential issue | 🔴 CriticalFix
config_clientINSERT column/value misalignment.Line 546 has one extra value, which shifts subsequent assignments and produces invalid type mapping (e.g.,
max_request_body_size_mbgetstrue,mcp_tool_sync_intervalgets'server'). This can break faker insertion forconfig_client.♻️ Proposed fix
-VALUES (1, false, '["provider", "model"]', '["*"]', '["Authorization"]', '{}', 300, true, false, false, 365, true, false, true, 100, 10, 30, 'server', 10, false, false, false, true, 'client-config-hash-001', $now, $now) +VALUES (1, false, '["provider", "model"]', '["*"]', '["Authorization"]', '{}', 300, true, false, false, 365, true, false, 100, 10, 30, 'server', 10, false, false, false, true, 'client-config-hash-001', $now, $now)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/scripts/run-migration-tests.sh around lines 545 - 546, The INSERT into config_client has one extra value in the VALUES tuple causing column/value misalignment; update the VALUES row so it has the exact same number of entries as the column list for table config_client (the VALUES clause shown) by removing the extraneous boolean (or otherwise correcting the misplaced item) so that fields like max_request_body_size_mb, mcp_tool_sync_interval and config_hash map to the correct types/positions; after editing, re-verify the order of values matches the listed columns exactly.plugins/compat/main.go (1)
136-140:⚠️ Potential issue | 🟠 MajorPreserve dropped-param metadata on failures too.
dropUnsupportedParams(...)already mutates the request before the provider call, but this block only copies the metadata onto successful results. Error-only responses lose that signal.♻️ Minimal fix
- if result != nil { - if extraFields := result.GetExtraFields(); extraFields != nil { - extraFields.DroppedCompatPluginParams = p.droppedParams - } - } + if droppedParams, ok := ctx.Value(schemas.BifrostContextKeyCompatPluginDroppedParams).([]string); ok && len(droppedParams) > 0 { + if result != nil { + if extraFields := result.GetExtraFields(); extraFields != nil { + extraFields.DroppedCompatPluginParams = droppedParams + } + } + if bifrostErr != nil { + bifrostErr.ExtraFields.DroppedCompatPluginParams = droppedParams + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/main.go` around lines 136 - 140, The code only copies dropped-params metadata into successful results (using result.GetExtraFields().DroppedCompatPluginParams) and therefore loses that signal for error-only responses; update the post-call logic (around dropUnsupportedParams and the provider call) to always attach p.droppedParams to the response's extra fields whether the call returned a non-nil result or only an error—i.e., if you can obtain extra fields from the response object (or from any error-wrapped response struct) set ExtraFields.DroppedCompatPluginParams = p.droppedParams even when result == nil so the metadata is preserved on failures as well.
🧹 Nitpick comments (1)
docs/openapi/openapi.json (1)
133224-133233: Consider centralizingcompatschema in the source OpenAPI components to reduce drift.The same schema is repeated in four places; extracting it once in the ref-based source spec and regenerating would reduce maintenance risk.
Based on learnings, the bundled
docs/openapi/openapi.jsonshould stay aligned with the ref-basedopenapi.yamlgenerated-spec pattern.Also applies to: 133546-133555, 205799-205808, 206020-206029
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 133224 - 133233, The compat schema is duplicated; extract the object under "compat" (with properties convert_text_to_chat, convert_chat_to_responses, should_drop_params, should_convert_params) into a single reusable component (e.g., components.schemas.CompatPluginConfig) in the source OpenAPI spec and replace each inline occurrence with a $ref to that component; update the ref-based generator/template so the generated/openapi.json uses $ref to components.schemas.CompatPluginConfig instead of repeating the inline schema in each location (ensure the default for should_convert_params remains true and property descriptions are preserved).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm-charts/bifrost/values.schema.json`:
- Around line 296-304: The schema change removed the legacy
enableLitellmFallbacks key which breaks upgrades when
bifrost.client.additionalProperties is false; restore backwards compatibility by
adding "enableLitellmFallbacks": { "type": "boolean", "default": false } into
the "compat" properties block in helm-charts/bifrost/values.schema.json (next to
convertTextToChat, convertChatToResponses, shouldDropParams,
shouldConvertParams) so old values files validate, and add a TODO comment to
remove the legacy key in the following release window.
In `@plugins/compat/main.go`:
- Around line 32-37: The field CompatPlugin.droppedParams is request-scoped and
must not live on the shared CompatPlugin instance; instead, capture dropped
params inside the request context in PreLLMHook and read them in PostLLMHook
using the Bifrost context API (use (*BifrostContext).SetValue(key, value) rather
than context.WithValue). Remove or stop mutating p.droppedParams; in PreLLMHook
compute the dropped list and call SetValue("dropped_compat_plugin_params",
droppedList) on the request BifrostContext, and in PostLLMHook retrieve that
value from the same BifrostContext to emit metadata. Update any helper logic
that referenced CompatPlugin.droppedParams (e.g., where droppedParams was set or
cleared) to use the context key instead.
- Around line 73-83: The hook leaves a stale BifrostContextKeyChangeRequestType
on the shared *schemas.BifrostContext which can force conversion later; in
PreLLMHook clear/reset that context key before recomputing conversion intent
(e.g., call ctx.SetValue(BifrostContextKeyChangeRequestType, nil) or equivalent)
so each invocation (in PreLLMHook) recomputes from scratch; update PreLLMHook
(which references cloneBifrostReq and p.config.ShouldConvertParams /
p.config.ShouldDropParams) to clear the key at the start or right before you
decide to set it again.
---
Outside diff comments:
In `@docs/features/litellm-compat.mdx`:
- Around line 98-115: The docs page omits the fourth compat toggle that the code
exposes (should_convert_params); update the feature list and the JSON example to
include and describe "Convert/Convert Params" (referencing
should_convert_params) alongside convert_text_to_chat,
convert_chat_to_responses, and should_drop_params, and ensure the example JSON
matches the schema in config.schema.json by adding "should_convert_params": true
with a brief one-line description of its behavior.
---
Duplicate comments:
In @.github/workflows/scripts/run-migration-tests.sh:
- Around line 545-546: The INSERT into config_client has one extra value in the
VALUES tuple causing column/value misalignment; update the VALUES row so it has
the exact same number of entries as the column list for table config_client (the
VALUES clause shown) by removing the extraneous boolean (or otherwise correcting
the misplaced item) so that fields like max_request_body_size_mb,
mcp_tool_sync_interval and config_hash map to the correct types/positions; after
editing, re-verify the order of values matches the listed columns exactly.
In `@plugins/compat/main.go`:
- Around line 136-140: The code only copies dropped-params metadata into
successful results (using result.GetExtraFields().DroppedCompatPluginParams) and
therefore loses that signal for error-only responses; update the post-call logic
(around dropUnsupportedParams and the provider call) to always attach
p.droppedParams to the response's extra fields whether the call returned a
non-nil result or only an error—i.e., if you can obtain extra fields from the
response object (or from any error-wrapped response struct) set
ExtraFields.DroppedCompatPluginParams = p.droppedParams even when result == nil
so the metadata is preserved on failures as well.
---
Nitpick comments:
In `@docs/openapi/openapi.json`:
- Around line 133224-133233: The compat schema is duplicated; extract the object
under "compat" (with properties convert_text_to_chat, convert_chat_to_responses,
should_drop_params, should_convert_params) into a single reusable component
(e.g., components.schemas.CompatPluginConfig) in the source OpenAPI spec and
replace each inline occurrence with a $ref to that component; update the
ref-based generator/template so the generated/openapi.json uses $ref to
components.schemas.CompatPluginConfig instead of repeating the inline schema in
each location (ensure the default for should_convert_params remains true and
property descriptions are preserved).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0eb8cb5b-c175-475b-a781-67e97cf1e9e5
⛔ Files ignored due to path filters (3)
framework/go.sumis excluded by!**/*.sumplugins/compat/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
.github/workflows/configs/withpostgresmcpclientsinconfig/config.json.github/workflows/scripts/run-migration-tests.sh.github/workflows/scripts/test-docker-image.sh.github/workflows/scripts/validate-helm-config-fields.shdocs/features/litellm-compat.mdxdocs/openapi/openapi.jsondocs/openapi/schemas/management/config.yamldocs/providers/supported-providers/overview.mdxexamples/configs/withpostgresmcpclientsinconfig/config.jsonexamples/configs/withprompushgateway/config.jsonexamples/configs/withvirtualkeys/config.jsonexamples/dockers/data/config.jsonframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/modelcatalog/main.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/conversion.goplugins/compat/main.gotests/governance/config.jsontests/integrations/python/config.jsontests/integrations/typescript/config.jsontransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/plugins.gotransports/config.schema.jsontransports/go.modui/app/workspace/config/views/clientSettingsView.tsxui/components/ui/accordion.tsxui/lib/types/config.ts
💤 Files with no reviewable changes (3)
- examples/configs/withpostgresmcpclientsinconfig/config.json
- examples/configs/withvirtualkeys/config.json
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
✅ Files skipped from review due to trivial changes (6)
- tests/governance/config.json
- tests/integrations/typescript/config.json
- docs/providers/supported-providers/overview.mdx
- tests/integrations/python/config.json
- plugins/compat/conversion.go
- transports/bifrost-http/server/plugins.go
🚧 Files skipped from review as they are similar to previous changes (18)
- examples/configs/withprompushgateway/config.json
- transports/go.mod
- examples/dockers/data/config.json
- transports/bifrost-http/lib/config.go
- helm-charts/bifrost/values.yaml
- .github/workflows/scripts/validate-helm-config-fields.sh
- framework/configstore/rdb.go
- transports/config.schema.json
- ui/app/workspace/config/views/clientSettingsView.tsx
- ui/components/ui/accordion.tsx
- framework/modelcatalog/main.go
- docs/openapi/schemas/management/config.yaml
- ui/lib/types/config.ts
- helm-charts/bifrost/templates/_helpers.tpl
- transports/bifrost-http/handlers/config.go
- framework/configstore/clientconfig.go
- framework/configstore/migrations.go
- transports/bifrost-http/lib/config_test.go
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
2 times, most recently
from
85affa9 to
23da649
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)
ui/app/workspace/config/views/clientSettingsView.tsx (1)
92-99:⚠️ Potential issue | 🟡 MinorAdd fallback default for
compatto prevent partial state.The
useEffectprovides a fallback forheader_filter_config(line 96) but not forcompat. If the server config has an undefinedcompatfield,handleCompatChangewill spreadundefined, resulting in a partial object with only the changed field. This could cause unexpected behavior when saving.🛡️ Proposed fix
useEffect(() => { if (config) { setLocalConfig({ ...config, header_filter_config: config.header_filter_config || DefaultGlobalHeaderFilterConfig, + compat: config.compat || DefaultCoreConfig.compat, }); } }, [config]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/config/views/clientSettingsView.tsx` around lines 92 - 99, The useEffect that calls setLocalConfig spreads config but only provides a fallback for header_filter_config, so if config.compat is undefined then handleCompatChange may spread undefined and produce a partial compat object; update the useEffect to also provide a fallback for compat (e.g. compat: config.compat || DefaultCompatConfigOrEmptyObject) when building the localConfig so setLocalConfig receives a full compat object; reference the useEffect, setLocalConfig, header_filter_config, compat and handleCompatChange when making the change.
♻️ Duplicate comments (7)
ui/app/workspace/config/views/clientSettingsView.tsx (2)
336-353:⚠️ Potential issue | 🟠 MajorMove Link outside AccordionTrigger to fix nested interactive elements.
AccordionTriggerrenders as a<button>, so nesting a<Link>(anchor) inside it creates invalid HTML. This breaks keyboard navigation and screen reader behavior. WhileonClick={(e) => e.stopPropagation()}prevents accordion toggling, the DOM structure is still invalid.Also, add
data-testidto theAccordionTrigger.♿ Proposed fix
- <AccordionTrigger className="py-0 hover:no-underline"> - <div className="space-y-0.5 text-left"> - <span className="text-sm font-medium">LiteLLM Compat</span> - <p className="text-muted-foreground text-sm font-normal"> - Request type conversion and parameter dropping.{" "} - <Link - className="text-primary cursor-pointer underline" - href="https://docs.getbifrost.ai/features/litellm-compat" - target="_blank" - rel="noopener noreferrer" - data-testid="litellm-docs-link" - onClick={(e) => e.stopPropagation()} - > - Learn more - </Link> - </p> - </div> - </AccordionTrigger> + <div className="flex items-center justify-between"> + <AccordionTrigger className="py-0 hover:no-underline" data-testid="compat-settings-trigger"> + <div className="space-y-0.5 text-left"> + <span className="text-sm font-medium">LiteLLM Compat</span> + <p className="text-muted-foreground text-sm font-normal"> + Request type conversion and parameter dropping. + </p> + </div> + </AccordionTrigger> + <Link + className="text-primary shrink-0 text-sm underline" + href="https://docs.getbifrost.ai/features/litellm-compat" + target="_blank" + rel="noopener noreferrer" + data-testid="litellm-docs-link" + > + Learn more + </Link> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/config/views/clientSettingsView.tsx` around lines 336 - 353, The AccordionTrigger currently nests a Link inside it (AccordionTrigger and Link), creating invalid HTML because AccordionTrigger renders as a button; move the Link out of AccordionTrigger so the anchor is a sibling (e.g., render the "Learn more" Link after the AccordionTrigger block) and remove the onClick stopPropagation hack; also add a data-testid (e.g., data-testid="litellm-accordion-trigger") to the AccordionTrigger element to support tests and accessibility.
364-371:⚠️ Potential issue | 🔴 CriticalFix
datatest-idtypo →data-testidon all Switch components.The attribute
datatest-idis misspelled (missing hyphen). This is not a valid HTML data attribute, so E2E tests will fail to locate these elements. The correct attribute isdata-testid.This affects all four switches at lines 366, 384, 400, and 416.
🐛 Proposed fix
<Switch id="compat-convert-text-to-chat" - datatest-id="compat-convert-text-to-chat" + data-testid="compat-convert-text-to-chat-switch" size="md" ... />Apply the same pattern to the other three switches:
data-testid="compat-convert-chat-to-responses-switch"(line 384)data-testid="compat-should-drop-params-switch"(line 400)data-testid="compat-should-convert-params-switch"(line 416)As per coding guidelines, interactive UI elements require
data-testid="<entity>-<element>-<qualifier>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/config/views/clientSettingsView.tsx` around lines 364 - 371, Replace the incorrect attribute name `datatest-id` with the correct `data-testid` on the Switch components so E2E tests can locate them; update the Switch with id "compat-convert-text-to-chat" and the other three Switches (the ones using handleCompatChange for "convert_chat_to_responses", "should_drop_params", and "should_convert_params") to use data-testid values following the pattern: data-testid="compat-convert-text-to-chat", data-testid="compat-convert-chat-to-responses-switch", data-testid="compat-should-drop-params-switch", and data-testid="compat-should-convert-params-switch" respectively, keeping all other props (id, checked, onCheckedChange, disabled) unchanged.framework/configstore/migrations.go (2)
6119-6133:⚠️ Potential issue | 🟠 MajorRefresh
config_hashafter compat backfill to avoid stale hashes.Line 6119-Line 6126 mutates compat fields that participate in client config hashing, but this migration does not recompute
config_hash. That leaves rows falsely “dirty” on reconciliation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6119 - 6133, The migration updates compat fields (compat_should_convert_params and compat_convert_text_to_chat based on enable_litellm_fallbacks via tx.Exec and then mig.DropColumn) but never recomputes/persists config_hash, leaving rows with stale hashes; after performing the UPDATEs (and before/after dropping enable_litellm_fallbacks) add a step to recompute and persist config_hash for the affected config_client rows (e.g., via an additional tx.Exec that calls the same DB-side hashing function or by invoking the repo/helper that recalculates and writes config_hash for clients) so reconciliation sees the new values as clean.
6138-6145:⚠️ Potential issue | 🔴 CriticalRollback column recreation check is inverted and breaks downgrade.
The rollback currently checks
HasColumn(..., "enable_litellm_fallbacks")and then tries toADD COLUMN, which is the opposite of the required condition. In normal post-migration state, the column is absent and never recreated before the UPDATE.🛠️ Minimal fix
- if tx.Migrator().HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") { + if !mig.HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") { if err := tx.Exec("ALTER TABLE config_client ADD COLUMN enable_litellm_fallbacks BOOLEAN DEFAULT FALSE").Error; err != nil { return err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6138 - 6145, The rollback logic in the migration inverts the column-existence checks: change the ADD COLUMN branch to run only when the column does NOT exist and ensure the UPDATE runs only when the column DOES exist; specifically, replace the current tx.Migrator().HasColumn(..., "enable_litellm_fallbacks") check around the ALTER TABLE ADD with a negated check (e.g., !tx.Migrator().HasColumn(...)) so the column is created when missing, use the same migrator helper (tx.Migrator()) consistently instead of mig.HasColumn, and keep the UPDATE statement (UPDATE config_client SET enable_litellm_fallbacks = COALESCE(compat_convert_text_to_chat, FALSE)) guarded by a positive HasColumn(...) check so the UPDATE only runs after the column exists.framework/modelcatalog/main.go (1)
325-327:⚠️ Potential issue | 🟡 MinorUpdate
IsRequestTypeSupporteddoc comment to match actual behavior.The comment currently says “supports chat completion,” but the function checks support for the provided
requestType.📝 Suggested doc fix
-// IsRequestTypeSupported checks if a model supports chat completion. -// It checks the supportedResponseTypes index. +// IsRequestTypeSupported checks if a model supports the given request type. +// It checks the supportedResponseTypes index. func (mc *ModelCatalog) IsRequestTypeSupported(model string, provider schemas.ModelProvider, requestType schemas.RequestType) bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 325 - 327, The doc comment for IsRequestTypeSupported incorrectly says it “supports chat completion”; update it to accurately describe behavior: state that IsRequestTypeSupported(model string, provider schemas.ModelProvider, requestType schemas.RequestType) checks whether the given model (for the given provider) supports the provided requestType by examining the supportedResponseTypes/index, and returns a boolean. Keep the parameter names (model, provider, requestType) in the comment to make intent clear.plugins/compat/main.go (2)
73-76:⚠️ Potential issue | 🟠 MajorClear stale conversion intent before recomputing it.
Line 73 enters
PreLLMHookwithout clearingBifrostContextKeyChangeRequestType; if no conversion is needed on a later pass, the old value can still force conversion.Based on learnings: `BifrostContext` is mutable shared context and fallback attempts rerun plugin hooks.🛠️ Minimal fix
func (p *CompatPlugin) PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.LLMPluginShortCircuit, error) { if ctx == nil || req == nil { return req, nil, nil } + ctx.ClearValue(schemas.BifrostContextKeyChangeRequestType)Also applies to: 84-96, 161-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/main.go` around lines 73 - 76, Clear any previous conversion intent stored under BifrostContextKeyChangeRequestType at the start of PreLLMHook so stale values can't force conversion on subsequent passes; specifically, remove or reset that key from the mutable ctx before you recompute whether conversion is needed inside PreLLMHook (function PreLLMHook). Apply the same clearing/reset pattern in the other plugin hook methods that recompute conversion intent (the blocks referenced around lines 84-96 and 161-163) so each invocation starts with a clean BifrostContext conversion state.
32-37:⚠️ Potential issue | 🟠 MajorAvoid request-scoped mutable state on the shared plugin instance.
p.droppedParamsis mutated inPreLLMHookand read inPostLLMHookon a singleton plugin, so concurrent requests can overwrite each other’s metadata.Based on learnings: fallbacks re-execute the full plugin pipeline from scratch, and `(*BifrostContext).SetValue` mutates shared request context in place.♻️ Proposed fix (store per-request dropped params in context)
type CompatPlugin struct { config Config logger schemas.Logger modelCatalog *modelcatalog.ModelCatalog - droppedParams []string } @@ - p.droppedParams = nil + ctx.ClearValue(schemas.BifrostContextKeyCompatPluginDroppedParams) @@ droppedParams := dropUnsupportedParams(modifiedReq, supportedParams) if len(droppedParams) > 0 { - p.droppedParams = droppedParams + ctx.SetValue(schemas.BifrostContextKeyCompatPluginDroppedParams, droppedParams) } @@ if result != nil { if extraFields := result.GetExtraFields(); extraFields != nil { - extraFields.DroppedCompatPluginParams = p.droppedParams + if droppedParams, ok := ctx.Value(schemas.BifrostContextKeyCompatPluginDroppedParams).([]string); ok { + extraFields.DroppedCompatPluginParams = droppedParams + } } } + if bifrostErr != nil { + if droppedParams, ok := ctx.Value(schemas.BifrostContextKeyCompatPluginDroppedParams).([]string); ok { + bifrostErr.ExtraFields.DroppedCompatPluginParams = droppedParams + } + }Also applies to: 82-83, 99-106, 136-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/main.go` around lines 32 - 37, The CompatPlugin currently stores request-scoped mutable state in the singleton field droppedParams which is written in PreLLMHook and read in PostLLMHook, causing race conditions; update PreLLMHook to record dropped params into the per-request context (e.g., via BifrostContext or a request-scoped map) instead of assigning to CompatPlugin.droppedParams, and change PostLLMHook to read those dropped params from that same request context; ensure you do not mutate shared plugin fields and avoid using (*BifrostContext).SetValue on shared state—use a request-local key or struct passed through the request context so concurrent requests stay isolated.
🧹 Nitpick comments (1)
transports/bifrost-http/lib/config_test.go (1)
13517-13522: Consider extracting duplicatedCompatreconstruction into a helper.The same mapping literal appears twice; a small helper will reduce drift risk for future compat fields.
♻️ Suggested refactor
+ buildCompat := func(cc tables.TableClientConfig) configstore.CompatConfig { + return configstore.CompatConfig{ + ConvertTextToChat: cc.CompatConvertTextToChat, + ConvertChatToResponses: cc.CompatConvertChatToResponses, + ShouldDropParams: cc.CompatShouldDropParams, + ShouldConvertParams: cc.CompatShouldConvertParams, + } + } + clientConfig := configstore.ClientConfig{ DropExcessRequests: ccToSave.DropExcessRequests, InitialPoolSize: ccToSave.InitialPoolSize, PrometheusLabels: ccToSave.PrometheusLabels, EnableLogging: ccToSave.EnableLogging, @@ - Compat: configstore.CompatConfig{ - ConvertTextToChat: ccToSave.CompatConvertTextToChat, - ConvertChatToResponses: ccToSave.CompatConvertChatToResponses, - ShouldDropParams: ccToSave.CompatShouldDropParams, - ShouldConvertParams: ccToSave.CompatShouldConvertParams, - }, + Compat: buildCompat(ccToSave), } @@ clientConfigFromDB := configstore.ClientConfig{ DropExcessRequests: ccFromDB.DropExcessRequests, InitialPoolSize: ccFromDB.InitialPoolSize, PrometheusLabels: ccFromDB.PrometheusLabels, EnableLogging: ccFromDB.EnableLogging, @@ - Compat: configstore.CompatConfig{ - ConvertTextToChat: ccFromDB.CompatConvertTextToChat, - ConvertChatToResponses: ccFromDB.CompatConvertChatToResponses, - ShouldDropParams: ccFromDB.CompatShouldDropParams, - ShouldConvertParams: ccFromDB.CompatShouldConvertParams, - }, + Compat: buildCompat(ccFromDB), }Also applies to: 13541-13546
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config_test.go` around lines 13517 - 13522, Extract the duplicated Compat mapping literal into a small helper function (e.g., compatFromSaved) that accepts the saved struct (the variable ccToSave) and returns a configstore.CompatConfig built from ccToSave. Replace both inline literals where Compat: configstore.CompatConfig{...} is reconstructed (the occurrences using ccToSave.CompatConvertTextToChat, CompatConvertChatToResponses, CompatShouldDropParams, CompatShouldConvertParams) with a call to this helper to avoid duplication and drift.
🤖 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 `@ui/app/workspace/config/views/clientSettingsView.tsx`:
- Around line 92-99: The useEffect that calls setLocalConfig spreads config but
only provides a fallback for header_filter_config, so if config.compat is
undefined then handleCompatChange may spread undefined and produce a partial
compat object; update the useEffect to also provide a fallback for compat (e.g.
compat: config.compat || DefaultCompatConfigOrEmptyObject) when building the
localConfig so setLocalConfig receives a full compat object; reference the
useEffect, setLocalConfig, header_filter_config, compat and handleCompatChange
when making the change.
---
Duplicate comments:
In `@framework/configstore/migrations.go`:
- Around line 6119-6133: The migration updates compat fields
(compat_should_convert_params and compat_convert_text_to_chat based on
enable_litellm_fallbacks via tx.Exec and then mig.DropColumn) but never
recomputes/persists config_hash, leaving rows with stale hashes; after
performing the UPDATEs (and before/after dropping enable_litellm_fallbacks) add
a step to recompute and persist config_hash for the affected config_client rows
(e.g., via an additional tx.Exec that calls the same DB-side hashing function or
by invoking the repo/helper that recalculates and writes config_hash for
clients) so reconciliation sees the new values as clean.
- Around line 6138-6145: The rollback logic in the migration inverts the
column-existence checks: change the ADD COLUMN branch to run only when the
column does NOT exist and ensure the UPDATE runs only when the column DOES
exist; specifically, replace the current tx.Migrator().HasColumn(...,
"enable_litellm_fallbacks") check around the ALTER TABLE ADD with a negated
check (e.g., !tx.Migrator().HasColumn(...)) so the column is created when
missing, use the same migrator helper (tx.Migrator()) consistently instead of
mig.HasColumn, and keep the UPDATE statement (UPDATE config_client SET
enable_litellm_fallbacks = COALESCE(compat_convert_text_to_chat, FALSE)) guarded
by a positive HasColumn(...) check so the UPDATE only runs after the column
exists.
In `@framework/modelcatalog/main.go`:
- Around line 325-327: The doc comment for IsRequestTypeSupported incorrectly
says it “supports chat completion”; update it to accurately describe behavior:
state that IsRequestTypeSupported(model string, provider schemas.ModelProvider,
requestType schemas.RequestType) checks whether the given model (for the given
provider) supports the provided requestType by examining the
supportedResponseTypes/index, and returns a boolean. Keep the parameter names
(model, provider, requestType) in the comment to make intent clear.
In `@plugins/compat/main.go`:
- Around line 73-76: Clear any previous conversion intent stored under
BifrostContextKeyChangeRequestType at the start of PreLLMHook so stale values
can't force conversion on subsequent passes; specifically, remove or reset that
key from the mutable ctx before you recompute whether conversion is needed
inside PreLLMHook (function PreLLMHook). Apply the same clearing/reset pattern
in the other plugin hook methods that recompute conversion intent (the blocks
referenced around lines 84-96 and 161-163) so each invocation starts with a
clean BifrostContext conversion state.
- Around line 32-37: The CompatPlugin currently stores request-scoped mutable
state in the singleton field droppedParams which is written in PreLLMHook and
read in PostLLMHook, causing race conditions; update PreLLMHook to record
dropped params into the per-request context (e.g., via BifrostContext or a
request-scoped map) instead of assigning to CompatPlugin.droppedParams, and
change PostLLMHook to read those dropped params from that same request context;
ensure you do not mutate shared plugin fields and avoid using
(*BifrostContext).SetValue on shared state—use a request-local key or struct
passed through the request context so concurrent requests stay isolated.
In `@ui/app/workspace/config/views/clientSettingsView.tsx`:
- Around line 336-353: The AccordionTrigger currently nests a Link inside it
(AccordionTrigger and Link), creating invalid HTML because AccordionTrigger
renders as a button; move the Link out of AccordionTrigger so the anchor is a
sibling (e.g., render the "Learn more" Link after the AccordionTrigger block)
and remove the onClick stopPropagation hack; also add a data-testid (e.g.,
data-testid="litellm-accordion-trigger") to the AccordionTrigger element to
support tests and accessibility.
- Around line 364-371: Replace the incorrect attribute name `datatest-id` with
the correct `data-testid` on the Switch components so E2E tests can locate them;
update the Switch with id "compat-convert-text-to-chat" and the other three
Switches (the ones using handleCompatChange for "convert_chat_to_responses",
"should_drop_params", and "should_convert_params") to use data-testid values
following the pattern: data-testid="compat-convert-text-to-chat",
data-testid="compat-convert-chat-to-responses-switch",
data-testid="compat-should-drop-params-switch", and
data-testid="compat-should-convert-params-switch" respectively, keeping all
other props (id, checked, onCheckedChange, disabled) unchanged.
---
Nitpick comments:
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 13517-13522: Extract the duplicated Compat mapping literal into a
small helper function (e.g., compatFromSaved) that accepts the saved struct (the
variable ccToSave) and returns a configstore.CompatConfig built from ccToSave.
Replace both inline literals where Compat: configstore.CompatConfig{...} is
reconstructed (the occurrences using ccToSave.CompatConvertTextToChat,
CompatConvertChatToResponses, CompatShouldDropParams, CompatShouldConvertParams)
with a call to this helper to avoid duplication and drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a475fb7a-885c-4f01-b8a2-775ad50c6c9e
⛔ Files ignored due to path filters (3)
framework/go.sumis excluded by!**/*.sumplugins/compat/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
.github/workflows/configs/withpostgresmcpclientsinconfig/config.json.github/workflows/scripts/run-migration-tests.sh.github/workflows/scripts/test-docker-image.sh.github/workflows/scripts/validate-helm-config-fields.shdocs/features/litellm-compat.mdxdocs/openapi/openapi.jsondocs/openapi/schemas/management/config.yamldocs/providers/supported-providers/overview.mdxexamples/configs/withpostgresmcpclientsinconfig/config.jsonexamples/configs/withprompushgateway/config.jsonexamples/configs/withvirtualkeys/config.jsonexamples/dockers/data/config.jsonframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/modelcatalog/main.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/conversion.goplugins/compat/main.goplugins/compat/requestcopy.gotests/governance/config.jsontests/integrations/python/config.jsontests/integrations/typescript/config.jsontransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/plugins.gotransports/config.schema.jsontransports/go.modui/app/workspace/config/views/clientSettingsView.tsxui/components/ui/accordion.tsxui/lib/types/config.ts
💤 Files with no reviewable changes (3)
- examples/configs/withvirtualkeys/config.json
- examples/configs/withpostgresmcpclientsinconfig/config.json
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
✅ Files skipped from review due to trivial changes (7)
- docs/providers/supported-providers/overview.mdx
- tests/integrations/typescript/config.json
- examples/configs/withprompushgateway/config.json
- .github/workflows/scripts/test-docker-image.sh
- tests/integrations/python/config.json
- tests/governance/config.json
- plugins/compat/requestcopy.go
🚧 Files skipped from review as they are similar to previous changes (16)
- ui/components/ui/accordion.tsx
- examples/dockers/data/config.json
- plugins/compat/conversion.go
- helm-charts/bifrost/templates/_helpers.tpl
- docs/features/litellm-compat.mdx
- helm-charts/bifrost/values.yaml
- transports/config.schema.json
- .github/workflows/scripts/run-migration-tests.sh
- transports/bifrost-http/handlers/config.go
- helm-charts/bifrost/values.schema.json
- framework/configstore/tables/clientconfig.go
- transports/bifrost-http/server/plugins.go
- ui/lib/types/config.ts
- framework/configstore/clientconfig.go
- docs/openapi/schemas/management/config.yaml
- transports/bifrost-http/lib/config.go
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
23da649 to
9b39c4d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
ui/app/workspace/config/views/clientSettingsView.tsx (1)
336-350:⚠️ Potential issue | 🟠 MajorKeep the accordion trigger as a single interactive control.
Line 341 still nests a
LinkinsideAccordionTrigger, which renders a button-like control. That is invalid interactive nesting and will break keyboard/focus behavior for this section. When you move the docs link alongside the trigger, please also tag the trigger itself with adata-testidso the new control is targetable in E2E.♿ Suggested structure
- <AccordionTrigger className="py-0 hover:no-underline"> - <div className="space-y-0.5 text-left"> - <span className="text-sm font-medium">LiteLLM Compat</span> - <p className="text-muted-foreground text-sm font-normal"> - Request type conversion and parameter dropping.{" "} - <Link - className="text-primary cursor-pointer underline" - href="https://docs.getbifrost.ai/features/litellm-compat" - target="_blank" - rel="noopener noreferrer" - data-testid="litellm-docs-link" - onClick={(e) => e.stopPropagation()} - > - Learn more - </Link> - </p> - </div> - </AccordionTrigger> + <div className="flex items-start justify-between gap-3"> + <AccordionTrigger className="py-0 hover:no-underline" data-testid="compat-settings-trigger"> + <div className="space-y-0.5 text-left"> + <span className="text-sm font-medium">LiteLLM Compat</span> + <p className="text-muted-foreground text-sm font-normal"> + Request type conversion and parameter dropping. + </p> + </div> + </AccordionTrigger> + <Link + className="text-primary shrink-0 underline" + href="https://docs.getbifrost.ai/features/litellm-compat" + target="_blank" + rel="noopener noreferrer" + data-testid="litellm-docs-link" + > + Learn more + </Link> + </div>As per coding guidelines,
ui/**/*.{ts,tsx}: Adddata-testidattributes to new interactive elements using the convention<entity>-<element>-<qualifier>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/config/views/clientSettingsView.tsx` around lines 336 - 350, The AccordionTrigger currently contains a nested Link which creates invalid interactive nesting and breaks keyboard/focus; move the docs Link out of the AccordionTrigger so the trigger remains a single interactive control (place the Link alongside the trigger content, not inside AccordionTrigger), add a data-testid to the AccordionTrigger (use the project convention, e.g. "litellm-toggle-trigger" or similar) so E2E can target it, and keep the Link's attributes (href, target, rel, data-testid for the link itself like "litellm-docs-link") while ensuring any onClick stopPropagation logic is applied to the Link now that it is external to AccordionTrigger..github/workflows/scripts/run-migration-tests.sh (1)
545-546:⚠️ Potential issue | 🔴 CriticalKeep the base
config_clientseed compatible with pre-migration schemas.This static
INSERTruns before the new migration on older tagged schemas, so moving the base fixture tocompat_*columns makes the statement fail on versions that still have the legacy shape. On top of that, Line 546 has one extratrue, so the values no longer match the declared columns even on schemas that do have the compat fields.♻️ Suggested direction
-INSERT INTO config_client (id, drop_excess_requests, prometheus_labels_json, allowed_origins_json, allowed_headers_json, header_filter_config_json, initial_pool_size, enable_logging, disable_content_logging, disable_db_pings_in_health, log_retention_days, enforce_governance_header, allow_direct_keys, max_request_body_size_mb, mcp_agent_depth, mcp_tool_execution_timeout, mcp_code_mode_binding_level, mcp_tool_sync_interval, compat_convert_text_to_chat, compat_convert_chat_to_responses, compat_should_drop_params, compat_should_convert_params, config_hash, created_at, updated_at) -VALUES (1, false, '["provider", "model"]', '["*"]', '["Authorization"]', '{}', 300, true, false, false, 365, true, false, true, 100, 10, 30, 'server', 10, false, false, false, true, 'client-config-hash-001', $now, $now) +INSERT INTO config_client (id, drop_excess_requests, prometheus_labels_json, allowed_origins_json, allowed_headers_json, header_filter_config_json, initial_pool_size, enable_logging, disable_content_logging, disable_db_pings_in_health, log_retention_days, enforce_governance_header, allow_direct_keys, max_request_body_size_mb, mcp_agent_depth, mcp_tool_execution_timeout, mcp_code_mode_binding_level, mcp_tool_sync_interval, enable_litellm_fallbacks, config_hash, created_at, updated_at) +VALUES (1, false, '["provider", "model"]', '["*"]', '["Authorization"]', '{}', 300, true, false, false, 365, true, false, 100, 10, 30, 'server', 10, false, 'client-config-hash-001', $now, $now)Then add version-gated
UPDATE config_client SET compat_* = ...coverage inappend_dynamic_columns_postgres()/append_dynamic_columns_sqlite()when those columns exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/scripts/run-migration-tests.sh around lines 545 - 546, The INSERT into config_client must remain compatible with pre-migration schemas: remove any references to compat_* columns from the base VALUES and fix the mismatched boolean (remove the extra true so the number of VALUES matches the declared columns) in the INSERT statement; then add version-gated logic in append_dynamic_columns_postgres() and append_dynamic_columns_sqlite() to run an UPDATE config_client SET compat_* = ... only when those compat_* columns exist (detect column presence before updating) so newer compat fields are populated post-migration without breaking older schemas.framework/configstore/migrations.go (2)
6141-6148:⚠️ Potential issue | 🔴 CriticalFix the inverted rollback guard for
enable_litellm_fallbacks.The condition at Line 6141 is still backwards. In the normal post-migration state the legacy column is absent, so this branch skips recreating it and the next
UPDATEtargets a missing column. If the column already exists, this branch tries to add it again and errors.Suggested fix
- if tx.Migrator().HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") { + if !mig.HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") { if err := tx.Exec("ALTER TABLE config_client ADD COLUMN enable_litellm_fallbacks BOOLEAN DEFAULT FALSE").Error; err != nil { return err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6141 - 6148, The guard around adding the enable_litellm_fallbacks column is inverted: in migrations.go you should only ALTER TABLE to add enable_litellm_fallbacks when that column does NOT already exist. Replace the current check using tx.Migrator().HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") with the correct presence test (use mig.HasColumn or negate the check) so the ALTER TABLE runs only when enable_litellm_fallbacks is missing and the subsequent UPDATE (which relies on compat_convert_text_to_chat) targets an existing column; update the conditional around the ALTER to !mig.HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") (or equivalent) to fix the rollback guard inversion.
6172-6193:⚠️ Potential issue | 🟠 MajorRecompute
config_hashin the follow-up compat migration too.This migration exists specifically for databases where
replace_enable_litellm_with_compat_columnsis already marked applied. It updatescompat_should_convert_params, but never refreshesconfig_hash, so those client-config rows can remain permanently “dirty” on later reconciliation/sync passes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6172 - 6193, The migrationDefaultCompatShouldConvertParamsFalse migration updates compat_should_convert_params but does not refresh the derived config_hash, leaving rows "dirty"; after the UPDATE of compat_should_convert_params in that migration, recompute and persist config_hash for the affected config_client rows (tables.TableClientConfig / config_client) using the same hashing/serialization routine the app uses elsewhere (call the existing helper that refreshes client config hashes, or, if none exists, compute the hash from the canonical config columns and UPDATE config_hash) so the follow-up compat migration leaves hashes consistent.
🧹 Nitpick comments (2)
docs/openapi/openapi.json (1)
133224-133232: Add explicit"default": falseto allcompatboolean flags for consistency.In the OpenAPI spec,
should_convert_paramsdeclares"default": false, butconvert_text_to_chat,convert_chat_to_responses, andshould_drop_paramsdo not. This inconsistency appears across four schema definitions (GET/api/configresponse, PUT/api/configrequest,GetConfigResponse, andUpdateConfigRequest). Add explicit defaults to all three flags to ensure generated documentation and SDKs handle these fields consistently.Proposed patch pattern (apply to each compat block)
"compat": { "type": "object", "description": "Compat plugin configuration", "properties": { - "convert_text_to_chat": { "type": "boolean", "description": "Convert text completion requests to chat" }, - "convert_chat_to_responses": { "type": "boolean", "description": "Convert chat completion requests to responses" }, - "should_drop_params": { "type": "boolean", "description": "Drop unsupported parameters based on model catalog" }, + "convert_text_to_chat": { "type": "boolean", "description": "Convert text completion requests to chat", "default": false }, + "convert_chat_to_responses": { "type": "boolean", "description": "Convert chat completion requests to responses", "default": false }, + "should_drop_params": { "type": "boolean", "description": "Drop unsupported parameters based on model catalog", "default": false }, "should_convert_params": { "type": "boolean", "description": "Converts model parameter values that are not supported by the model.", "default": false } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 133224 - 133232, Add explicit "default": false to the three boolean fields in every compat schema: convert_text_to_chat, convert_chat_to_responses, and should_drop_params so they match should_convert_params; update each compat object found in the OpenAPI schemas for the GET /api/config response, PUT /api/config request, and the GetConfigResponse and UpdateConfigRequest schema definitions to include "default": false for those properties.ui/lib/types/config.ts (1)
457-479: Modelcompatas optional on the wire, or normalize it before putting it in state.
clientSettingsView.tsxalready treatsconfig.compatas possibly missing, so makingCoreConfig.compatrequired here makes the API type stricter than the actual payload shape. That hides upgrade-path cases from TypeScript and makes partial compat objects look fully initialized. Either markcompatoptional in the transport type, or always merge inDefaultCoreConfig.compatwhen hydrating server data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/config.ts` around lines 457 - 479, The CoreConfig type currently requires compat, but the server payload can omit it; update the model by making compat optional on CoreConfig (change compat: CompatConfig to compat?: CompatConfig) so types match the wire, or alternatively ensure the server-to-state hydration code (where server config is normalized before setting state—e.g., the code path that merges into DefaultCoreConfig or the logic used by clientSettingsView.tsx) always merges DefaultCoreConfig.compat into the incoming config so stored state never contains a partially initialized compat; reference CompatConfig, CoreConfig, DefaultCoreConfig.compat and clientSettingsView.tsx when applying the fix.
🤖 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/configstore/migrations.go`:
- Around line 6205-6209: The rollback currently re-applies the new default
(FALSE) for column compat_should_convert_params on table config_client; change
the Postgres rollback branch (the case handling tx.Dialector.Name() ==
"postgres" in this migration in migrations.go) to restore the pre-migration
state instead of setting FALSE — either DROP DEFAULT if the column previously
had no default, or SET DEFAULT to the prior value (inspect the corresponding up
migration to see the previous default) by running the appropriate ALTER TABLE
... ALTER COLUMN compat_should_convert_params DROP DEFAULT or ALTER TABLE ...
ALTER COLUMN compat_should_convert_params SET DEFAULT <previous_value> via
tx.Exec so downgrades truly revert the schema change.
In `@transports/bifrost-http/handlers/config.go`:
- Around line 345-371: The compat plugin is being reloaded/removed immediately
(h.configManager.ReloadPlugin / RemovePlugin) before the durable config write
(UpdateClientConfig / ReloadClientConfigFromConfigStore) succeeds, causing live
state to drift on later failure; instead, postpone calling ReloadPlugin or
RemovePlugin until after the persistence call succeeds, or implement a rollback:
perform the persistence first (call UpdateClientConfig /
ReloadClientConfigFromConfigStore using updatedConfig), and only if that returns
nil invoke h.configManager.ReloadPlugin with compatCfg or RemovePlugin (using
PluginDisabledKey as needed); if persistence fails after you already changed
runtime, call h.configManager.ReloadPlugin with the previous oldCompat values to
restore the prior runtime state.
---
Duplicate comments:
In @.github/workflows/scripts/run-migration-tests.sh:
- Around line 545-546: The INSERT into config_client must remain compatible with
pre-migration schemas: remove any references to compat_* columns from the base
VALUES and fix the mismatched boolean (remove the extra true so the number of
VALUES matches the declared columns) in the INSERT statement; then add
version-gated logic in append_dynamic_columns_postgres() and
append_dynamic_columns_sqlite() to run an UPDATE config_client SET compat_* =
... only when those compat_* columns exist (detect column presence before
updating) so newer compat fields are populated post-migration without breaking
older schemas.
In `@framework/configstore/migrations.go`:
- Around line 6141-6148: The guard around adding the enable_litellm_fallbacks
column is inverted: in migrations.go you should only ALTER TABLE to add
enable_litellm_fallbacks when that column does NOT already exist. Replace the
current check using tx.Migrator().HasColumn(&tables.TableClientConfig{},
"enable_litellm_fallbacks") with the correct presence test (use mig.HasColumn or
negate the check) so the ALTER TABLE runs only when enable_litellm_fallbacks is
missing and the subsequent UPDATE (which relies on compat_convert_text_to_chat)
targets an existing column; update the conditional around the ALTER to
!mig.HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") (or
equivalent) to fix the rollback guard inversion.
- Around line 6172-6193: The migrationDefaultCompatShouldConvertParamsFalse
migration updates compat_should_convert_params but does not refresh the derived
config_hash, leaving rows "dirty"; after the UPDATE of
compat_should_convert_params in that migration, recompute and persist
config_hash for the affected config_client rows (tables.TableClientConfig /
config_client) using the same hashing/serialization routine the app uses
elsewhere (call the existing helper that refreshes client config hashes, or, if
none exists, compute the hash from the canonical config columns and UPDATE
config_hash) so the follow-up compat migration leaves hashes consistent.
In `@ui/app/workspace/config/views/clientSettingsView.tsx`:
- Around line 336-350: The AccordionTrigger currently contains a nested Link
which creates invalid interactive nesting and breaks keyboard/focus; move the
docs Link out of the AccordionTrigger so the trigger remains a single
interactive control (place the Link alongside the trigger content, not inside
AccordionTrigger), add a data-testid to the AccordionTrigger (use the project
convention, e.g. "litellm-toggle-trigger" or similar) so E2E can target it, and
keep the Link's attributes (href, target, rel, data-testid for the link itself
like "litellm-docs-link") while ensuring any onClick stopPropagation logic is
applied to the Link now that it is external to AccordionTrigger.
---
Nitpick comments:
In `@docs/openapi/openapi.json`:
- Around line 133224-133232: Add explicit "default": false to the three boolean
fields in every compat schema: convert_text_to_chat, convert_chat_to_responses,
and should_drop_params so they match should_convert_params; update each compat
object found in the OpenAPI schemas for the GET /api/config response, PUT
/api/config request, and the GetConfigResponse and UpdateConfigRequest schema
definitions to include "default": false for those properties.
In `@ui/lib/types/config.ts`:
- Around line 457-479: The CoreConfig type currently requires compat, but the
server payload can omit it; update the model by making compat optional on
CoreConfig (change compat: CompatConfig to compat?: CompatConfig) so types match
the wire, or alternatively ensure the server-to-state hydration code (where
server config is normalized before setting state—e.g., the code path that merges
into DefaultCoreConfig or the logic used by clientSettingsView.tsx) always
merges DefaultCoreConfig.compat into the incoming config so stored state never
contains a partially initialized compat; reference CompatConfig, CoreConfig,
DefaultCoreConfig.compat and clientSettingsView.tsx when applying the fix.
🪄 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: 6c329aea-50d1-417d-8013-4b6786b13a8d
⛔ Files ignored due to path filters (3)
framework/go.sumis excluded by!**/*.sumplugins/compat/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
.github/workflows/configs/withpostgresmcpclientsinconfig/config.json.github/workflows/scripts/run-migration-tests.sh.github/workflows/scripts/test-docker-image.sh.github/workflows/scripts/validate-helm-config-fields.shdocs/features/litellm-compat.mdxdocs/openapi/openapi.jsondocs/openapi/schemas/management/config.yamldocs/providers/supported-providers/overview.mdxexamples/configs/withpostgresmcpclientsinconfig/config.jsonexamples/configs/withprompushgateway/config.jsonexamples/configs/withvirtualkeys/config.jsonexamples/dockers/data/config.jsonframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/modelcatalog/main.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/conversion.goplugins/compat/main.goplugins/compat/requestcopy.gotests/governance/config.jsontests/integrations/python/config.jsontests/integrations/typescript/config.jsontransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/plugins.gotransports/config.schema.jsontransports/go.modui/app/workspace/config/views/clientSettingsView.tsxui/components/ui/accordion.tsxui/lib/types/config.ts
💤 Files with no reviewable changes (3)
- examples/configs/withvirtualkeys/config.json
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
- examples/configs/withpostgresmcpclientsinconfig/config.json
✅ Files skipped from review due to trivial changes (8)
- docs/providers/supported-providers/overview.mdx
- .github/workflows/scripts/test-docker-image.sh
- ui/components/ui/accordion.tsx
- tests/governance/config.json
- examples/configs/withprompushgateway/config.json
- tests/integrations/typescript/config.json
- examples/dockers/data/config.json
- .github/workflows/scripts/validate-helm-config-fields.sh
🚧 Files skipped from review as they are similar to previous changes (13)
- transports/go.mod
- tests/integrations/python/config.json
- helm-charts/bifrost/values.yaml
- docs/openapi/schemas/management/config.yaml
- plugins/compat/conversion.go
- helm-charts/bifrost/templates/_helpers.tpl
- transports/config.schema.json
- framework/configstore/clientconfig.go
- framework/modelcatalog/main.go
- transports/bifrost-http/server/plugins.go
- plugins/compat/requestcopy.go
- docs/features/litellm-compat.mdx
- plugins/compat/main.go
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
9b39c4d to
1543199
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
plugins/compat/main.go (1)
32-37:⚠️ Potential issue | 🟠 MajorDon’t keep request-scoped dropped-param state on the shared plugin instance.
CompatPluginis reused across requests, soPreLLMHookcan overwritep.droppedParamsbefore another request reachesPostLLMHook. That leaks the wrongdropped_compat_plugin_paramsmetadata across requests.♻️ Minimal fix
type CompatPlugin struct { config Config logger schemas.Logger modelCatalog *modelcatalog.ModelCatalog - droppedParams []string }- p.droppedParams = nil + ctx.ClearValue(schemas.BifrostContextKeyCompatPluginDroppedParams)droppedParams := dropUnsupportedParams(modifiedReq, supportedParams) if len(droppedParams) > 0 { - p.droppedParams = droppedParams + ctx.SetValue(schemas.BifrostContextKeyCompatPluginDroppedParams, droppedParams) }if result != nil { if extraFields := result.GetExtraFields(); extraFields != nil { - extraFields.DroppedCompatPluginParams = p.droppedParams + if droppedParams, ok := ctx.Value(schemas.BifrostContextKeyCompatPluginDroppedParams).([]string); ok { + extraFields.DroppedCompatPluginParams = droppedParams + } } }Based on learnings: in maximhq/bifrost, Bifrost context key/value assignments are done with
(*BifrostContext).SetValue(key, value), not withcontext.WithValue.Also applies to: 82-83, 103-111, 141-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/main.go` around lines 32 - 37, CompatPlugin currently stores request-scoped droppedParams on the shared CompatPlugin struct which can leak between requests; remove the droppedParams field from CompatPlugin and instead store/retrieve the per-request slice in the Bifrost request context using the BifrostContext SetValue API (use the key "dropped_compat_plugin_params") in PreLLMHook and PostLLMHook (and any other places that currently use context.WithValue or access p.droppedParams); update PreLLMHook to set the dropped params via bctx.SetValue("dropped_compat_plugin_params", droppedParams) and update PostLLMHook to load and clear them via bctx.Value/SetValue, eliminating any use of context.WithValue and ensuring no shared state on the CompatPlugin instance.
🧹 Nitpick comments (2)
docs/openapi/openapi.json (1)
133224-133232: Prefer a single shared schema source forcompatto avoid drift.The same
compatobject is duplicated in four places. In the source OpenAPI YAML, define one shared schema/component and reference it, then regenerateopenapi.json.Based on learnings: In the bifrost repository, the bundled OpenAPI spec (
docs/openapi/openapi.json) is inlined and follows the existing generated-spec pattern. In reviews, verify that the inlined JSON aligns with the ref-based openapi.yaml structure (paths/schemas) and that docs/openapi/openapi.json remains consistent with the generated-spec pattern across the docs directory.Also applies to: 133546-133554, 205799-205807, 206020-206028
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 133224 - 133232, The duplicated "compat" schema is inlined in multiple places in the generated OpenAPI JSON; replace those duplicate object blocks with a single shared component/schema and reference it from each location. Update the source OpenAPI YAML to define a single components.schemas.Compat (or a uniquely named schema) with the properties convert_text_to_chat, convert_chat_to_responses, should_drop_params, should_convert_params, then regenerate docs/openapi/openapi.json so each occurrence uses $ref to that component (ensure the schema name matches the generated spec pattern and update any paths that referenced the inlined "compat" object to point to the shared schema).transports/bifrost-http/server/plugins.go (1)
218-218: Update the slot-7 comment to match behavior.Line 218 says compat loads only when features are enabled, but the plugin is always registered.
♻️ Proposed fix
- // 7. Compat (if any compat feature is enabled in ClientConfig) + // 7. Compat (always registered; behavior is driven by ClientConfig.Compat flags)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/server/plugins.go` at line 218, Update the slot-7 inline comment in plugins.go to reflect actual behavior: although the comment states "Compat (if any compat feature is enabled in ClientConfig)" the compat plugin is registered unconditionally; change the comment to indicate the compat plugin is always registered (or mention it is a no-op unless features are enabled). Locate the slot-7 comment near the plugin registration code (the slot-7 marker/section in plugins.go that handles compat) and adjust its wording to accurately describe that registration always occurs regardless of ClientConfig feature flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/scripts/run-migration-tests.sh:
- Around line 545-546: The INSERT into table config_client has 25 columns but 26
VALUES, causing a misalignment (extra boolean placed after allow_direct_keys);
fix by making the columns and values count match—either remove the stray boolean
value after allow_direct_keys in the VALUES list or, if that boolean is
intended, add the corresponding column name to the column list so the ordering
matches (refer to the INSERT INTO config_client column list and the VALUES
sequence around allow_direct_keys to align them).
In `@plugins/compat/main.go`:
- Around line 78-81: The code only clones req when the static flags
p.config.ShouldConvertParams or p.config.ShouldDropParams are set, which allows
per-request context flags (BifrostContextKeyCompatShouldDropParams /
BifrostContextKeyCompatShouldConvertParams) to cause in-place mutation by
dropUnsupportedParams or applyParameterConversion; always make a defensive clone
before any mutating operations by using cloneBifrostReq whenever modifiedReq
will be passed to dropUnsupportedParams or applyParameterConversion (i.e.,
compute a bool that ORs the static config flags with the presence/value of the
two context keys and if true set modifiedReq = cloneBifrostReq(req) so all
subsequent calls operate on the clone).
In `@ui/app/workspace/config/views/compatibilityView.tsx`:
- Around line 152-154: The Save button rendered in the CompatibilityView (Button
with onClick={handleSave} and disabled tied to
hasChanges/isLoading/hasSettingsUpdateAccess) needs a data-testid per
guidelines; add a data-testid prop to that Button using the convention
data-testid="workspace-save-button" (or similar entity-element-qualifier) so
tests can target it, ensuring you don't change existing props like
onClick/disabled or text logic (isLoading ? "Saving..." : "Save Changes").
- Line 43: The toast error message in compatibilityView.tsx uses title-case and
punctuation; update the toast.error call (the toast.error(...) invocation in
this file/component) to use a lowercase, no-trailing-punctuation string per repo
convention — e.g., replace "Configuration not loaded" with "configuration not
loaded". Ensure the only change is the message text in the toast.error call.
---
Duplicate comments:
In `@plugins/compat/main.go`:
- Around line 32-37: CompatPlugin currently stores request-scoped droppedParams
on the shared CompatPlugin struct which can leak between requests; remove the
droppedParams field from CompatPlugin and instead store/retrieve the per-request
slice in the Bifrost request context using the BifrostContext SetValue API (use
the key "dropped_compat_plugin_params") in PreLLMHook and PostLLMHook (and any
other places that currently use context.WithValue or access p.droppedParams);
update PreLLMHook to set the dropped params via
bctx.SetValue("dropped_compat_plugin_params", droppedParams) and update
PostLLMHook to load and clear them via bctx.Value/SetValue, eliminating any use
of context.WithValue and ensuring no shared state on the CompatPlugin instance.
---
Nitpick comments:
In `@docs/openapi/openapi.json`:
- Around line 133224-133232: The duplicated "compat" schema is inlined in
multiple places in the generated OpenAPI JSON; replace those duplicate object
blocks with a single shared component/schema and reference it from each
location. Update the source OpenAPI YAML to define a single
components.schemas.Compat (or a uniquely named schema) with the properties
convert_text_to_chat, convert_chat_to_responses, should_drop_params,
should_convert_params, then regenerate docs/openapi/openapi.json so each
occurrence uses $ref to that component (ensure the schema name matches the
generated spec pattern and update any paths that referenced the inlined "compat"
object to point to the shared schema).
In `@transports/bifrost-http/server/plugins.go`:
- Line 218: Update the slot-7 inline comment in plugins.go to reflect actual
behavior: although the comment states "Compat (if any compat feature is enabled
in ClientConfig)" the compat plugin is registered unconditionally; change the
comment to indicate the compat plugin is always registered (or mention it is a
no-op unless features are enabled). Locate the slot-7 comment near the plugin
registration code (the slot-7 marker/section in plugins.go that handles compat)
and adjust its wording to accurately describe that registration always occurs
regardless of ClientConfig feature flags.
🪄 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: ad9d2b81-1768-46da-b1f6-94d34bd5a62c
⛔ Files ignored due to path filters (3)
framework/go.sumis excluded by!**/*.sumplugins/compat/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
.github/workflows/configs/withpostgresmcpclientsinconfig/config.json.github/workflows/scripts/run-migration-tests.sh.github/workflows/scripts/test-docker-image.sh.github/workflows/scripts/validate-helm-config-fields.shcore/schemas/bifrost.godocs/features/litellm-compat.mdxdocs/openapi/openapi.jsondocs/openapi/schemas/management/config.yamldocs/providers/supported-providers/overview.mdxexamples/configs/withpostgresmcpclientsinconfig/config.jsonexamples/configs/withprompushgateway/config.jsonexamples/configs/withvirtualkeys/config.jsonexamples/dockers/data/config.jsonframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/modelcatalog/main.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/conversion.goplugins/compat/main.goplugins/compat/requestcopy.gotests/governance/config.jsontests/integrations/python/config.jsontests/integrations/typescript/config.jsontransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/plugins.gotransports/config.schema.jsontransports/go.modui/app/workspace/config/compatibility/page.tsxui/app/workspace/config/views/clientSettingsView.tsxui/app/workspace/config/views/compatibilityView.tsxui/components/sidebar.tsxui/components/ui/accordion.tsxui/lib/types/config.ts
💤 Files with no reviewable changes (3)
- examples/configs/withvirtualkeys/config.json
- examples/configs/withpostgresmcpclientsinconfig/config.json
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
✅ Files skipped from review due to trivial changes (9)
- .github/workflows/scripts/test-docker-image.sh
- docs/providers/supported-providers/overview.mdx
- tests/integrations/typescript/config.json
- tests/governance/config.json
- ui/components/ui/accordion.tsx
- plugins/compat/conversion.go
- framework/configstore/migrations.go
- framework/configstore/clientconfig.go
- plugins/compat/requestcopy.go
🚧 Files skipped from review as they are similar to previous changes (10)
- examples/configs/withprompushgateway/config.json
- transports/go.mod
- examples/dockers/data/config.json
- tests/integrations/python/config.json
- transports/bifrost-http/lib/config.go
- transports/config.schema.json
- helm-charts/bifrost/values.schema.json
- framework/modelcatalog/main.go
- transports/bifrost-http/handlers/config.go
- docs/features/litellm-compat.mdx
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
1543199 to
5fe51c6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
plugins/compat/main.go (3)
83-85:⚠️ Potential issue | 🟠 MajorFix the clone-guard typo for per-request param conversion.
The second override branch checks
shouldDropParamsOverrideinstead ofshouldConvertParamsOverride. When only the convert-params override is enabled,applyParameterConversion(modifiedReq)runs on the original request and mutates caller state in place.🐛 Minimal fix
- if (shouldDropParamsOverrideEnabled && shouldDropParamsOverride) || (shouldConvertParamsOverrideEnabled && shouldDropParamsOverride) || p.config.ShouldConvertParams || p.config.ShouldDropParams { + if (shouldDropParamsOverrideEnabled && shouldDropParamsOverride) || (shouldConvertParamsOverrideEnabled && shouldConvertParamsOverride) || p.config.ShouldConvertParams || p.config.ShouldDropParams { modifiedReq = cloneBifrostReq(req) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/main.go` around lines 83 - 85, The conditional that decides whether to clone the request has a typo: the second branch uses shouldDropParamsOverride instead of shouldConvertParamsOverride, causing applyParameterConversion(modifiedReq) to run on the original request when only the convert-params override is set; update the condition in the if that assigns modifiedReq (the clause currently reading "(shouldConvertParamsOverrideEnabled && shouldDropParamsOverride)") to use shouldConvertParamsOverride so that cloneBifrostReq is invoked whenever shouldConvertParamsOverrideEnabled or shouldDropParamsOverrideEnabled or the config flags (p.config.ShouldConvertParams / p.config.ShouldDropParams) are true, ensuring applyParameterConversion(modifiedReq) mutates the cloned request.
32-37:⚠️ Potential issue | 🟠 MajorMove dropped-param state off the shared plugin instance.
droppedParamsis request-scoped, butCompatPluginis shared across requests.PreLLMHookresets/writes it andPostLLMHooklater reads it, so concurrent requests can overwrite each other and emit the wrongdropped_compat_plugin_params.♻️ Minimal refactor
type CompatPlugin struct { config Config logger schemas.Logger modelCatalog *modelcatalog.ModelCatalog - droppedParams []string } @@ - p.droppedParams = nil + ctx.ClearValue(schemas.BifrostContextKeyCompatPluginDroppedParams) @@ - if len(droppedParams) > 0 { - p.droppedParams = droppedParams - } + if len(droppedParams) > 0 { + ctx.SetValue(schemas.BifrostContextKeyCompatPluginDroppedParams, droppedParams) + } @@ - if result != nil { - if extraFields := result.GetExtraFields(); extraFields != nil { - extraFields.DroppedCompatPluginParams = p.droppedParams - } - } + if result != nil { + if extraFields := result.GetExtraFields(); extraFields != nil { + if droppedParams, ok := ctx.Value(schemas.BifrostContextKeyCompatPluginDroppedParams).([]string); ok { + extraFields.DroppedCompatPluginParams = droppedParams + } + } + }Based on learnings: In maximhq/bifrost, Bifrost context key/value assignments are done with
(*BifrostContext).SetValue(key, value), not withcontext.WithValue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/main.go` around lines 32 - 37, droppedParams is request-scoped but stored on the shared CompatPlugin instance, causing race conditions between requests; remove the droppedParams field from CompatPlugin and instead store the slice in the request context inside PreLLMHook using the BifrostContext SetValue API (not context.WithValue), and retrieve it in PostLLMHook via the same BifrostContext key; update PreLLMHook to set the dropped params value and PostLLMHook to read and emit dropped_compat_plugin_params from the context, keeping CompatPlugin, PreLLMHook, and PostLLMHook as the only touchpoints.
141-145:⚠️ Potential issue | 🟠 MajorAttach dropped-param metadata to error responses too.
BifrostErrorExtraFieldsexposesdropped_compat_plugin_params, and the docs now promise it on failures, but this hook only writes the metadata onto successful results. Any provider error after param dropping loses that context.♻️ Minimal follow-up
- if result != nil { - if extraFields := result.GetExtraFields(); extraFields != nil { - extraFields.DroppedCompatPluginParams = p.droppedParams - } - } + if droppedParams, ok := ctx.Value(schemas.BifrostContextKeyCompatPluginDroppedParams).([]string); ok { + if result != nil { + if extraFields := result.GetExtraFields(); extraFields != nil { + extraFields.DroppedCompatPluginParams = droppedParams + } + } + if bifrostErr != nil { + bifrostErr.ExtraFields.DroppedCompatPluginParams = droppedParams + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/compat/main.go` around lines 141 - 145, The current code only writes p.droppedParams into result.GetExtraFields() for successful results; update the handler so that when a provider error occurs you also attach p.droppedParams to the error's extra fields (the BifrostErrorExtraFields/dropped_compat_plugin_params). Concretely, after the provider call where you currently check if result != nil and call result.GetExtraFields(), add logic to locate or create the error's extra fields (use the same GetExtraFields() accessor on the error response or the BifrostErrorExtraFields structure) and set DroppedCompatPluginParams = p.droppedParams so failures carry the same metadata as successes.framework/configstore/migrations.go (2)
6138-6148:⚠️ Potential issue | 🔴 CriticalRollback still skips recreating
enable_litellm_fallbacks.Line 6141 checks
HasColumn(...)before runningADD COLUMN, so the normal post-migration state never recreates the legacy column on downgrade. The condition needs to be inverted before the backfill fromcompat_convert_text_to_chat.🛠️ Minimal fix
- if tx.Migrator().HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") { + if !mig.HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") { if err := tx.Exec("ALTER TABLE config_client ADD COLUMN enable_litellm_fallbacks BOOLEAN DEFAULT FALSE").Error; err != nil { return err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6138 - 6148, The rollback currently only ADDs enable_litellm_fallbacks when HasColumn(...) is true, so the legacy column is never recreated; inside the Rollback function update the conditional around tx.Migrator().HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") to check for NOT HasColumn (i.e. only run ALTER TABLE ADD COLUMN when the column is missing), then keep the subsequent backfill from compat_convert_text_to_chat (UPDATE config_client SET enable_litellm_fallbacks = COALESCE(compat_convert_text_to_chat, FALSE)) so the column is created first and then populated.
6122-6134:⚠️ Potential issue | 🟠 MajorRecompute
config_hashafter the compat backfill.This migration updates
compat_*fields but never refreshesconfig_hash, so existing client-config rows will keep stale hashes and be falsely treated as dirty on the next reconciliation pass. Mirror the existing client-config hash backfill pattern in this file before returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6122 - 6134, After updating compat_* fields and dropping enable_litellm_fallbacks, recompute and persist the config_hash for affected rows in config_client so stale hashes aren’t left behind; insert the same client-config hash backfill pattern used elsewhere in this migrations.go file (i.e., the UPDATE that recalculates config_hash from the canonical config representation) immediately after the mig.DropColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") block and before returning, using the same SQL or helper routine used elsewhere in this file so config_client.config_hash is refreshed for the modified rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features/litellm-compat.mdx`:
- Around line 9-15: The opening sentence incorrectly states "two
transformations" while the list contains three items; update the sentence in
docs/features/litellm-compat.mdx (the line starting "The LiteLLM compatibility
plugin provides two transformations:") to reflect the correct count or rephrase
(e.g., "provides three transformations:" or "provides the following
transformations:") so the header matches the listed items and keep the rest of
the text about converted_request_type and extra_fields unchanged.
In `@plugins/compat/main.go`:
- Around line 157-164: The function currently leaves shouldConvert false when
p.modelCatalog is nil, contradicting Init/file docs that say conversion should
still occur without a catalog; update the logic in the block referencing
p.modelCatalog, IsRequestTypeSupported, shouldConvert, model, provider,
currentType, and targetType so that when p.modelCatalog == nil you set
shouldConvert = true (and adjust the debug log to reflect that conversion will
proceed without a catalog), while preserving the existing conditional that
checks IsRequestTypeSupported when a catalog exists.
In `@ui/app/workspace/config/views/compatibilityView.tsx`:
- Around line 14-16: The save button can be clicked before the initial core
config is loaded; update the component to also read the loading state from
useGetCoreConfigQuery (e.g., capture its isLoading/isFetching alongside data:
bifrostConfig) and include that in the button disable condition so the save
action (triggered via updateCoreConfig from useUpdateCoreConfigMutation) is
disabled while the query is still loading or when config
(bifrostConfig?.client_config?.compat) is undefined; ensure the existing
mutation isLoading is still part of the OR condition so the button is disabled
when either the query or the mutation is in flight.
---
Duplicate comments:
In `@framework/configstore/migrations.go`:
- Around line 6138-6148: The rollback currently only ADDs
enable_litellm_fallbacks when HasColumn(...) is true, so the legacy column is
never recreated; inside the Rollback function update the conditional around
tx.Migrator().HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks")
to check for NOT HasColumn (i.e. only run ALTER TABLE ADD COLUMN when the column
is missing), then keep the subsequent backfill from compat_convert_text_to_chat
(UPDATE config_client SET enable_litellm_fallbacks =
COALESCE(compat_convert_text_to_chat, FALSE)) so the column is created first and
then populated.
- Around line 6122-6134: After updating compat_* fields and dropping
enable_litellm_fallbacks, recompute and persist the config_hash for affected
rows in config_client so stale hashes aren’t left behind; insert the same
client-config hash backfill pattern used elsewhere in this migrations.go file
(i.e., the UPDATE that recalculates config_hash from the canonical config
representation) immediately after the
mig.DropColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") block
and before returning, using the same SQL or helper routine used elsewhere in
this file so config_client.config_hash is refreshed for the modified rows.
In `@plugins/compat/main.go`:
- Around line 83-85: The conditional that decides whether to clone the request
has a typo: the second branch uses shouldDropParamsOverride instead of
shouldConvertParamsOverride, causing applyParameterConversion(modifiedReq) to
run on the original request when only the convert-params override is set; update
the condition in the if that assigns modifiedReq (the clause currently reading
"(shouldConvertParamsOverrideEnabled && shouldDropParamsOverride)") to use
shouldConvertParamsOverride so that cloneBifrostReq is invoked whenever
shouldConvertParamsOverrideEnabled or shouldDropParamsOverrideEnabled or the
config flags (p.config.ShouldConvertParams / p.config.ShouldDropParams) are
true, ensuring applyParameterConversion(modifiedReq) mutates the cloned request.
- Around line 32-37: droppedParams is request-scoped but stored on the shared
CompatPlugin instance, causing race conditions between requests; remove the
droppedParams field from CompatPlugin and instead store the slice in the request
context inside PreLLMHook using the BifrostContext SetValue API (not
context.WithValue), and retrieve it in PostLLMHook via the same BifrostContext
key; update PreLLMHook to set the dropped params value and PostLLMHook to read
and emit dropped_compat_plugin_params from the context, keeping CompatPlugin,
PreLLMHook, and PostLLMHook as the only touchpoints.
- Around line 141-145: The current code only writes p.droppedParams into
result.GetExtraFields() for successful results; update the handler so that when
a provider error occurs you also attach p.droppedParams to the error's extra
fields (the BifrostErrorExtraFields/dropped_compat_plugin_params). Concretely,
after the provider call where you currently check if result != nil and call
result.GetExtraFields(), add logic to locate or create the error's extra fields
(use the same GetExtraFields() accessor on the error response or the
BifrostErrorExtraFields structure) and set DroppedCompatPluginParams =
p.droppedParams so failures carry the same metadata as successes.
🪄 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: bd65317e-5431-40a2-9273-1c533182fa1e
⛔ Files ignored due to path filters (3)
framework/go.sumis excluded by!**/*.sumplugins/compat/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
.github/workflows/configs/withpostgresmcpclientsinconfig/config.json.github/workflows/scripts/run-migration-tests.sh.github/workflows/scripts/test-docker-image.sh.github/workflows/scripts/validate-helm-config-fields.shcore/schemas/bifrost.godocs/features/litellm-compat.mdxdocs/openapi/openapi.jsondocs/openapi/schemas/management/config.yamldocs/providers/supported-providers/overview.mdxexamples/configs/withpostgresmcpclientsinconfig/config.jsonexamples/configs/withprompushgateway/config.jsonexamples/configs/withvirtualkeys/config.jsonexamples/dockers/data/config.jsonframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/modelcatalog/main.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/conversion.goplugins/compat/main.goplugins/compat/requestcopy.gotests/governance/config.jsontests/integrations/python/config.jsontests/integrations/typescript/config.jsontransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/plugins.gotransports/config.schema.jsontransports/go.modui/app/workspace/config/compatibility/page.tsxui/app/workspace/config/views/clientSettingsView.tsxui/app/workspace/config/views/compatibilityView.tsxui/components/sidebar.tsxui/components/ui/accordion.tsxui/lib/types/config.ts
💤 Files with no reviewable changes (3)
- examples/configs/withpostgresmcpclientsinconfig/config.json
- examples/configs/withvirtualkeys/config.json
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
✅ Files skipped from review due to trivial changes (12)
- ui/components/ui/accordion.tsx
- docs/providers/supported-providers/overview.mdx
- transports/go.mod
- tests/integrations/typescript/config.json
- tests/governance/config.json
- transports/bifrost-http/lib/ctx.go
- plugins/compat/conversion.go
- examples/configs/withprompushgateway/config.json
- tests/integrations/python/config.json
- ui/components/sidebar.tsx
- transports/bifrost-http/lib/config_test.go
- plugins/compat/requestcopy.go
🚧 Files skipped from review as they are similar to previous changes (12)
- .github/workflows/scripts/test-docker-image.sh
- ui/app/workspace/config/compatibility/page.tsx
- examples/dockers/data/config.json
- helm-charts/bifrost/templates/_helpers.tpl
- helm-charts/bifrost/values.yaml
- transports/config.schema.json
- helm-charts/bifrost/values.schema.json
- framework/modelcatalog/main.go
- framework/configstore/clientconfig.go
- core/schemas/bifrost.go
- transports/bifrost-http/handlers/config.go
- transports/bifrost-http/server/plugins.go
Contributor
…o_happen_at_the_provider_level
|
|
akshaydeo
deleted the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
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
5 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
Move the conversion of TextCompletion to ChatCompletion from plugin level (litellmcompat) to provider level. The plugin (litellmcompat) only sets a context key -
BifrostContextKeyShouldConvertTextToChatThis enables downstream plugins to see the correct request / response type.
Changes
shouldConvertTextToChat()function andwrapTextToChatStreamPostHookRunner()to handle text-to-chat conversion at provider dispatch timeBifrostContextKeyShouldConvertTextToChatcontext key instead of performing transformationsToBifrostTextCompletionResponse()method fromchatcompletions.gotomux.gounder a "RESPONSE CONVERSION METHODS" sectionBifrostContextKeyShouldConvertTextToChatfor plugins to signal conversion intent to coreType of change
Affected areas
How to test
Test text completion requests on models that only support chat completion to ensure they still work correctly:
Verify that the response format matches text completion format even though the underlying provider uses chat completion.
Breaking changes
This is an internal refactor that maintains the same external API behavior.
Security considerations
No security implications - this is an internal architectural change that doesn't affect authentication, secrets, or data handling.
Checklist
docs/contributing/README.mdand followed the guidelines