feat: removes enable_litellm_fallback option for more granular settings#2260
Closed
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 (33)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces the single boolean Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Frontend UI
participant API as ConfigHandler (Server)
participant DB as ConfigStore (DB)
participant Plugin as CompatPluginManager
UI->>API: PATCH /config (payload.client.compat)
API->>API: compare payload.ClientConfig.Compat vs currentConfig.Compat
alt compat changed
API->>DB: persist updated ClientConfig.Compat
DB-->>API: ACK
API->>Plugin: if any flag true -> load/reload with compat.Config; else -> remove plugin
Plugin-->>API: success / error
API-->>UI: 200 OK or 400 error ("Failed to load/remove compat plugin")
else no compat change
API-->>UI: 200 OK (no plugin action)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
This was referenced Mar 24, 2026
Merged
Member
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
11 tasks
![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: 7
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)
9-9:⚠️ Potential issue | 🟡 MinorDocumentation inconsistency: mentions "two transformations" but three compat flags are now available.
Line 9 states "two transformations" but the new compat configuration (lines 66-69 and 79-83) exposes three flags. The document body also lacks descriptions for
convert_chat_to_responsesandshould_drop_paramstransformations beyond the brief toggle descriptions in the UI section.Consider updating the intro to reflect all three options and adding sections describing the other two transformations (or clarifying if some are not strictly "transformations").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/litellm-compat.mdx` at line 9, Intro line claims "two transformations" but the plugin exposes three compat flags; update the docs to reflect all three options and add descriptions for the missing transformations. In the LiteLLM compatibility plugin docs update the sentence to mention three transformations/flags and add short sections explaining convert_chat_to_responses and should_drop_params (in addition to the existing convert_responses_to_chat), include where these flags are shown in the UI, and clarify if any flag is not a true "transformation" vs. a toggle for behavior to remove ambiguity.
🧹 Nitpick comments (5)
.github/workflows/scripts/validate-helm-config-fields.sh (1)
206-208: Add a negative assertion for the removed Helm key.These checks prove the new
client.compat.*fields render, but they still pass if the chart accidentally emits the removedclient.enable_litellm_fallbacksalongside them. Since this rename is a breaking change, one absence check here would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/scripts/validate-helm-config-fields.sh around lines 206 - 208, Add a negative assertion that verifies the removed Helm key is not present: call the existing absence-check helper (e.g., assert_field_absent) for 'client.enable_litellm_fallbacks' with the jq path '.client.enable_litellm_fallbacks' so the test not only validates the new 'client.compat.*' fields but also fails if the old key is still emitted.helm-charts/bifrost/templates/_helpers.tpl (1)
230-242: Minor: Emptycompatobject may be set if parent key exists but no sub-keys are defined.If a user specifies
bifrost.client.compat: {}(empty object) in their Helm values, the template will still set an emptycompatobject in the generated config. This differs from other similar blocks in this file (e.g.,headerFilterConfigat lines 255-266) which check if the object has content before setting it.If an empty
compatobject is acceptable to the downstream config parser, this is fine. Otherwise, consider guarding the set:♻️ Optional: Only set compat if it has content
{{- if .Values.bifrost.client.compat }} {{- $compat := dict }} {{- if hasKey .Values.bifrost.client.compat "convertTextToChat" }} {{- $_ := set $compat "convert_text_to_chat" .Values.bifrost.client.compat.convertTextToChat }} {{- end }} {{- if hasKey .Values.bifrost.client.compat "convertChatToResponses" }} {{- $_ := set $compat "convert_chat_to_responses" .Values.bifrost.client.compat.convertChatToResponses }} {{- end }} {{- if hasKey .Values.bifrost.client.compat "shouldDropParams" }} {{- $_ := set $compat "should_drop_params" .Values.bifrost.client.compat.shouldDropParams }} {{- end }} +{{- if $compat }} {{- $_ := set $client "compat" $compat }} +{{- end }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm-charts/bifrost/templates/_helpers.tpl` around lines 230 - 242, The template currently always sets an empty $compat object when .Values.bifrost.client.compat exists even if it has no sub-keys; update the logic around $compat (the block creating $compat and the final set $client "compat" $compat) to only call set $client "compat" $compat when $compat contains at least one key (e.g., by checking length or presence of any handled sub-keys after the hasKey checks) so an empty compat object is not emitted into the generated config.docs/openapi/openapi.json (1)
132105-132112: Define explicit defaults (and close object shape) forcompatflags.At Line [132109]–Line [132111], the new booleans are optional without defaults. Adding
default: falseandadditionalProperties: falsemakes client behavior deterministic and prevents typoed keys from being silently accepted.Proposed diff
"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" } + "convert_text_to_chat": { "type": "boolean", "default": false, "description": "Convert text completion requests to chat" }, + "convert_chat_to_responses": { "type": "boolean", "default": false, "description": "Convert chat completion requests to responses" }, + "should_drop_params": { "type": "boolean", "default": false, "description": "Drop unsupported parameters based on model catalog" } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 132105 - 132112, The compat object schema currently defines optional boolean flags (convert_text_to_chat, convert_chat_to_responses, should_drop_params) without defaults or closed shape; update the compat schema (the "compat" object) to set default: false for each of these boolean properties and add "additionalProperties": false to the compat object so unrecognized keys are rejected and clients get deterministic false defaults.helm-charts/bifrost/values.yaml (1)
191-194: Consider adding an inline migration hint for chart upgraders.A short comment near Line 191 that
enableLitellmFallbackswas replaced byclient.compat.*would make manual values migration easier.Suggested comment-only patch
compat: + # Replaces deprecated client.enableLitellmFallbacks convertTextToChat: false convertChatToResponses: false shouldDropParams: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm-charts/bifrost/values.yaml` around lines 191 - 194, Add a short inline comment above the compat block explaining that the previous enableLitellmFallbacks flag was replaced by the new client.compat.* keys (convertTextToChat, convertChatToResponses, shouldDropParams) so upgraders know to map their old setting to these three booleans; update the comment near the compat stanza (adjacent to compat.convertTextToChat / convertChatToResponses / shouldDropParams) to explicitly state the migration mapping and intent.transports/bifrost-http/lib/config_test.go (1)
12425-12430: Exercise every compat flag with a non-default hash/parity case.Right now the suite only proves
ConvertTextToChatchanges the hash, and the runtime-vs-migration parity path still round-trips all three compat columns asfalse. A regression that dropsConvertChatToResponsesorShouldDropParamsfrom hashing or DB persistence would still pass. Please add table-driven cases that flip each compat flag totrueand assert both hash changes and SQLite parity.Also applies to: 13389-13389, 13408-13408
🤖 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 12425 - 12430, The test currently only flips Compat.ConvertTextToChat; update the test around cc1/cc13 and GenerateClientConfigHash to exercise every Compat flag: create a table-driven list of the compat boolean fields (ConvertTextToChat, ConvertChatToResponses, ShouldDropParams), iterate and for each make a copy of cc1, set that field to true, call GenerateClientConfigHash() and assert the hash differs from the original, then write/read via the same SQLite parity path used elsewhere in this test to assert the stored/restored Compat field round-trips correctly; reference the cc1 variable, the Compat struct fields, GenerateClientConfigHash, and the existing SQLite parity assertions when adding these cases.
🤖 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 525-526: The INSERT INTO config_client has 25 VALUES for 24
columns (column/value mismatch): in the VALUES tuple for the INSERT in
run-migration-tests.sh there's an extra boolean value inserted before the
numeric max_request_body_size_mb (the stray 'true' between allow_direct_keys and
100). Remove that extra boolean so the VALUES list lines up with the column list
(ensure the sequence matches id, drop_excess_requests, ..., allow_direct_keys,
max_request_body_size_mb, ... , config_hash, created_at, updated_at).
In `@framework/configstore/clientconfig.go`:
- Around line 116-123: migrationReplaceEnableLiteLLMWithCompatColumns currently
adds the three compat columns and copies enable_litellm_fallbacks but does not
recompute the per-row config_hash, leaving stored hashes stale; update
migrationReplaceEnableLiteLLMWithCompatColumns to iterate all rows of the client
config table after adding/migrating the compat columns, recompute the
config_hash the same way migrationAddAdditionalConfigHashColumns and the
provider/virtual key migrations do (load each row, compute the hash including
compatConvertTextToChat, compatConvertChatToResponses, compatShouldDropParams),
and persist the new config_hash values to the DB so loadClientConfig() will
match file hashes and avoid spurious reconciles.
In `@framework/configstore/migrations.go`:
- Around line 5234-5241: After copying enable_litellm_fallbacks into
compat_convert_text_to_chat, recompute and persist the config_hash for the
affected client rows before dropping the legacy column: run the project's
existing config-hash recomputation routine (or an UPDATE that recalculates
config_hash) for rows whose compat_convert_text_to_chat value changed (e.g.,
WHERE compat_convert_text_to_chat = true or matching the rows updated by the
earlier tx.Exec), and do this in the same transaction (using tx) prior to
calling mig.DropColumn on TableClientConfig so reconciliation won't remain
stale.
In `@framework/configstore/rdb.go`:
- Around line 55-57: The compat flags in updatedConfig.Compat are being
unconditionally written to the DB (via UpdateClientConfig()) even when the
request omitted the compat object, causing omitted fields to reset to false;
change the update logic so the handler only assigns/overwrites
CompatConvertTextToChat, CompatConvertChatToResponses, and
CompatShouldDropParams on updatedConfig when the incoming payload explicitly
provided them (e.g., use nullable/pointer fields or an "isSet" flag for the
compat object in the request parsing) similar to how MCPAgentDepth is guarded
(>0), or alternatively document that clients must send the full compat object on
partial updates; ensure UpdateClientConfig() receives the selectively-updated
updatedConfig.Compat values.
In `@transports/go.mod`:
- Line 17: The transports module currently pins
github.com/maximhq/bifrost/plugins/compat v0.0.1 but the published tag doesn't
exist and the local compat API changes (ConvertTextToChat,
ConvertChatToResponses, ShouldDropParams in plugins/compat/main.go) are
unreachable; fix by either restoring the local replacement for the compat module
(add the replace directive pointing to ../plugins/compat), or update the require
to a real published tag that contains those API changes, or add a go.work to
include the local plugins/compat module in the workspace so transports can
resolve ConvertTextToChat, ConvertChatToResponses and ShouldDropParams.
In `@ui/app/workspace/config/views/clientSettingsView.tsx`:
- Around line 333-399: The new LiteLLM Compat controls lack data-testid
attributes which breaks E2E targeting; add data-testid attributes following the
project's pattern to the AccordionTrigger (e.g.,
data-testid="compat-accordion-trigger") and to each Switch for
convert_text_to_chat, convert_chat_to_responses, and should_drop_params (e.g.,
data-testid="compat-convert-text-to-chat-switch",
"compat-convert-chat-to-responses-switch", "compat-should-drop-params-switch")
by adding the attributes to the AccordionTrigger JSX and the three Switch
components (referencing AccordionTrigger and the Switch elements with ids
"compat-convert-text-to-chat", "compat-convert-chat-to-responses", and
"compat-should-drop-params"); ensure naming follows
data-testid="<entity>-<element>-<qualifier>".
- Around line 333-347: The Link with data-testid="litellm-docs-link" must be
moved out of the AccordionTrigger to avoid nesting an anchor inside the trigger
button; update the JSX so AccordionTrigger only contains the title/description
(the block with "LiteLLM Compat") and render the Link as a sibling element
adjacent to AccordionTrigger (preserve its href, target, rel, className and
data-testid), remove or adjust the onClick stopPropagation if no longer needed,
and ensure the surrounding layout/spacing still matches the original design.
---
Outside diff comments:
In `@docs/features/litellm-compat.mdx`:
- Line 9: Intro line claims "two transformations" but the plugin exposes three
compat flags; update the docs to reflect all three options and add descriptions
for the missing transformations. In the LiteLLM compatibility plugin docs update
the sentence to mention three transformations/flags and add short sections
explaining convert_chat_to_responses and should_drop_params (in addition to the
existing convert_responses_to_chat), include where these flags are shown in the
UI, and clarify if any flag is not a true "transformation" vs. a toggle for
behavior to remove ambiguity.
---
Nitpick comments:
In @.github/workflows/scripts/validate-helm-config-fields.sh:
- Around line 206-208: Add a negative assertion that verifies the removed Helm
key is not present: call the existing absence-check helper (e.g.,
assert_field_absent) for 'client.enable_litellm_fallbacks' with the jq path
'.client.enable_litellm_fallbacks' so the test not only validates the new
'client.compat.*' fields but also fails if the old key is still emitted.
In `@docs/openapi/openapi.json`:
- Around line 132105-132112: The compat object schema currently defines optional
boolean flags (convert_text_to_chat, convert_chat_to_responses,
should_drop_params) without defaults or closed shape; update the compat schema
(the "compat" object) to set default: false for each of these boolean properties
and add "additionalProperties": false to the compat object so unrecognized keys
are rejected and clients get deterministic false defaults.
In `@helm-charts/bifrost/templates/_helpers.tpl`:
- Around line 230-242: The template currently always sets an empty $compat
object when .Values.bifrost.client.compat exists even if it has no sub-keys;
update the logic around $compat (the block creating $compat and the final set
$client "compat" $compat) to only call set $client "compat" $compat when $compat
contains at least one key (e.g., by checking length or presence of any handled
sub-keys after the hasKey checks) so an empty compat object is not emitted into
the generated config.
In `@helm-charts/bifrost/values.yaml`:
- Around line 191-194: Add a short inline comment above the compat block
explaining that the previous enableLitellmFallbacks flag was replaced by the new
client.compat.* keys (convertTextToChat, convertChatToResponses,
shouldDropParams) so upgraders know to map their old setting to these three
booleans; update the comment near the compat stanza (adjacent to
compat.convertTextToChat / convertChatToResponses / shouldDropParams) to
explicitly state the migration mapping and intent.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 12425-12430: The test currently only flips
Compat.ConvertTextToChat; update the test around cc1/cc13 and
GenerateClientConfigHash to exercise every Compat flag: create a table-driven
list of the compat boolean fields (ConvertTextToChat, ConvertChatToResponses,
ShouldDropParams), iterate and for each make a copy of cc1, set that field to
true, call GenerateClientConfigHash() and assert the hash differs from the
original, then write/read via the same SQLite parity path used elsewhere in this
test to assert the stored/restored Compat field round-trips correctly; reference
the cc1 variable, the Compat struct fields, GenerateClientConfigHash, and the
existing SQLite parity assertions when adding these cases.
🪄 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: cb4b3f54-f9b3-42b5-823d-c148deb2adf4
📒 Files selected for processing (33)
.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.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/main.goplugins/compat/versiontests/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
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
c12a940 to
4a9d345
Compare
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
07e3cf4 to
d286969
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
.github/workflows/scripts/run-migration-tests.sh (1)
525-526:⚠️ Potential issue | 🔴 CriticalFix
config_clientINSERT column/value mismatch (stray legacy boolean).Line 526 currently has 25 values for 24 columns. The extra
trueafterallow_direct_keysshifts subsequent fields and makes the SQL invalid.🛠️ Proposed fix
-VALUES (1, false, '["provider", "model"]', '["*"]', '["Authorization"]', '{}', 300, true, false, false, 365, true, false, true, 100, 10, 30, 'server', 10, false, false, false, '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, '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 525 - 526, The INSERT into table config_client has 24 columns but 25 VALUES (a stray legacy boolean `true` after `allow_direct_keys`) causing a column/value mismatch; fix by removing that extra boolean from the VALUES list so each value aligns with its corresponding column (ensure the sequence matches columns like allow_direct_keys, max_request_body_size_mb, mcp_agent_depth, ... and ends with config_hash, created_at, updated_at), or alternatively if that boolean is actually a needed column, add the missing column name to the column list instead of the extra value.framework/configstore/migrations.go (1)
338-340:⚠️ Potential issue | 🟠 MajorRecompute
config_hashwhen backfilling compat state.Because
triggerMigrationsschedules this migration after the earlier client-config hash backfill, rows upgraded fromenable_litellm_fallbacks = truegetcompat_convert_text_to_chat = truehere but keep a pre-compatconfig_hash. That can leave config reconciliation stale until a later write. Please refresh the hash in the same transaction before dropping the legacy column.Also applies to: 5234-5241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 338 - 340, In migrationReplaceEnableLiteLLMWithCompatColumns, after updating rows to set compat_convert_text_to_chat = true for legacy enable_litellm_fallbacks but before dropping the legacy column, recompute and update the config_hash for those affected rows within the same transaction (use the same ctx and db/tx used by migrationReplaceEnableLiteLLMWithCompatColumns) so the hashed config reflects the new compat fields; ensure the config_hash update targets the same row selection logic used for the backfill and runs prior to removing the legacy column to avoid stale reconciliation.ui/app/workspace/config/views/clientSettingsView.tsx (2)
333-399:⚠️ Potential issue | 🟡 MinorAdd
data-testidto the new compat interactive controlsAt Line 333 and Line 361-399, the new trigger and switches are interactive but missing
data-testid, which violates the UI testing selector convention for newly added controls.🔧 Suggested fix
- <AccordionTrigger className="py-0 hover:no-underline"> + <AccordionTrigger className="py-0 hover:no-underline" data-testid="compat-settings-trigger"> ... <Switch + data-testid="compat-convert-text-to-chat-switch" id="compat-convert-text-to-chat" ... <Switch + data-testid="compat-convert-chat-to-responses-switch" id="compat-convert-chat-to-responses" ... <Switch + data-testid="compat-should-drop-params-switch" id="compat-should-drop-params"As per coding guidelines,
ui/**/*.{tsx,ts}: Add new interactive UI elements with data-testid attributes following the pattern: 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 333 - 399, The new LiteLLM Compat interactive elements (AccordionTrigger and the three Switches in clientSettingsView.tsx) are missing data-testid attributes; add data-testid values following the pattern "<entity>-<element>-<qualifier>" to the AccordionTrigger and each Switch (references: AccordionTrigger element containing the "LiteLLM Compat" label, and the Switch components with ids "compat-convert-text-to-chat", "compat-convert-chat-to-responses", and "compat-should-drop-params") so the trigger and each interactive Switch have unique testids for UI tests.
333-347:⚠️ Potential issue | 🟠 MajorDon’t nest the docs link inside
AccordionTriggerAt Line 333-347,
AccordionTriggeris interactive and contains a nested interactiveLink. This breaks HTML/a11y semantics;stopPropagation()doesn’t fully fix keyboard/screen-reader behavior.🔧 Suggested 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-start justify-between gap-3"> + <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. + </p> + </div> + </AccordionTrigger> + <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" + > + 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 333 - 347, AccordionTrigger currently nests an interactive Link (the <Link ... data-testid="litellm-docs-link">) which breaks accessibility; move the docs Link out of the AccordionTrigger and render it as a sibling (for example inside the AccordionContent or immediately after AccordionTrigger) so the trigger contains only non-interactive content (title/summary). Remove the stopPropagation() handler from the Link and ensure the same href/attributes (target, rel, data-testid) are preserved when relocated; update any styling or container markup so visual layout and tests remain the same while restoring correct keyboard/screen-reader behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/config.go`:
- Around line 344-362: When applying compat changes in the handler, fail the
whole update if the plugin mutation fails instead of only logging: after calling
h.configManager.ReloadPlugin(ctx, compat.PluginName, ..., compatCfg, ..., ...)
or h.configManager.RemovePlugin(disabledCtx, compat.PluginName) check the
returned error and propagate it (return the error from the handler) so
updatedConfig.Compat is not persisted on failure; reference the
newCompat/oldCompat comparison block, the calls to ReloadPlugin and
RemovePlugin, and the updatedConfig.Compat assignment to locate where to bail
out and return the error when plugin reload/remove fails.
In `@ui/app/workspace/config/views/clientSettingsView.tsx`:
- Around line 111-113: The equality checks for compat flags
(convert_text_to_chat, convert_chat_to_responses, should_drop_params) on
localConfig vs config can treat undefined and false as different, causing
hasCoreConfigChanges to stay true; normalize these optional flags before
comparing by coalescing undefined to false (or a consistent default) when
evaluating the comparisons used to compute hasCoreConfigChanges (references:
localConfig.compat, config.compat, and the hasCoreConfigChanges calculation in
clientSettingsView.tsx) so toggling back to the implicit backend default no
longer registers as a change.
---
Duplicate comments:
In @.github/workflows/scripts/run-migration-tests.sh:
- Around line 525-526: The INSERT into table config_client has 24 columns but 25
VALUES (a stray legacy boolean `true` after `allow_direct_keys`) causing a
column/value mismatch; fix by removing that extra boolean from the VALUES list
so each value aligns with its corresponding column (ensure the sequence matches
columns like allow_direct_keys, max_request_body_size_mb, mcp_agent_depth, ...
and ends with config_hash, created_at, updated_at), or alternatively if that
boolean is actually a needed column, add the missing column name to the column
list instead of the extra value.
In `@framework/configstore/migrations.go`:
- Around line 338-340: In migrationReplaceEnableLiteLLMWithCompatColumns, after
updating rows to set compat_convert_text_to_chat = true for legacy
enable_litellm_fallbacks but before dropping the legacy column, recompute and
update the config_hash for those affected rows within the same transaction (use
the same ctx and db/tx used by migrationReplaceEnableLiteLLMWithCompatColumns)
so the hashed config reflects the new compat fields; ensure the config_hash
update targets the same row selection logic used for the backfill and runs prior
to removing the legacy column to avoid stale reconciliation.
In `@ui/app/workspace/config/views/clientSettingsView.tsx`:
- Around line 333-399: The new LiteLLM Compat interactive elements
(AccordionTrigger and the three Switches in clientSettingsView.tsx) are missing
data-testid attributes; add data-testid values following the pattern
"<entity>-<element>-<qualifier>" to the AccordionTrigger and each Switch
(references: AccordionTrigger element containing the "LiteLLM Compat" label, and
the Switch components with ids "compat-convert-text-to-chat",
"compat-convert-chat-to-responses", and "compat-should-drop-params") so the
trigger and each interactive Switch have unique testids for UI tests.
- Around line 333-347: AccordionTrigger currently nests an interactive Link (the
<Link ... data-testid="litellm-docs-link">) which breaks accessibility; move the
docs Link out of the AccordionTrigger and render it as a sibling (for example
inside the AccordionContent or immediately after AccordionTrigger) so the
trigger contains only non-interactive content (title/summary). Remove the
stopPropagation() handler from the Link and ensure the same href/attributes
(target, rel, data-testid) are preserved when relocated; update any styling or
container markup so visual layout and tests remain the same while restoring
correct keyboard/screen-reader behavior.
🪄 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: 6527bd2c-6341-4756-992c-76fd2fa432df
📒 Files selected for processing (33)
.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.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/main.goplugins/compat/versiontests/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)
- examples/configs/withprompushgateway/config.json
- plugins/compat/version
- .github/workflows/scripts/test-docker-image.sh
- docs/providers/supported-providers/overview.mdx
- ui/components/ui/accordion.tsx
- examples/dockers/data/config.json
- helm-charts/bifrost/values.schema.json
🚧 Files skipped from review as they are similar to previous changes (15)
- transports/bifrost-http/server/plugins.go
- tests/governance/config.json
- tests/integrations/typescript/config.json
- transports/go.mod
- docs/openapi/openapi.json
- docs/features/litellm-compat.mdx
- docs/openapi/schemas/management/config.yaml
- helm-charts/bifrost/values.yaml
- transports/config.schema.json
- transports/bifrost-http/lib/config.go
- framework/configstore/rdb.go
- ui/lib/types/config.ts
- tests/integrations/python/config.json
- framework/configstore/clientconfig.go
- transports/bifrost-http/lib/config_test.go
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
4a9d345 to
fd76fc6
Compare
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
d286969 to
7057a7a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/scripts/run-migration-tests.sh (1)
525-526:⚠️ Potential issue | 🔴 CriticalFix
config_clientINSERT arity/ordering mismatch (extra value).Line 526 has one extra boolean (
true) afterallow_direct_keys, producing 25 values for 24 columns and shifting all subsequent assignments.🛠️ Proposed fix
-VALUES (1, false, '["provider", "model"]', '["*"]', '["Authorization"]', '{}', 300, true, false, false, 365, true, false, true, 100, 10, 30, 'server', 10, false, false, false, '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, 'client-config-hash-001', $now, $now)#!/bin/bash set -euo pipefail FILE=".github/workflows/scripts/run-migration-tests.sh" python3 - <<'PY' import re, pathlib p = pathlib.Path(".github/workflows/scripts/run-migration-tests.sh") txt = p.read_text() m = re.search(r"INSERT INTO config_client \((.*?)\)\s*VALUES \((.*?)\)\s*ON CONFLICT DO NOTHING;", txt, re.S) if not m: raise SystemExit("config_client INSERT not found") cols_raw = m.group(1) vals_raw = m.group(2) cols = [c.strip() for c in cols_raw.split(",")] vals = [] buf = [] in_str = False i = 0 while i < len(vals_raw): ch = vals_raw[i] if ch == "'" and (i == 0 or vals_raw[i-1] != "\\"): in_str = not in_str buf.append(ch) elif ch == "," and not in_str: vals.append("".join(buf).strip()) buf = [] else: buf.append(ch) i += 1 if buf: vals.append("".join(buf).strip()) print(f"columns={len(cols)}") print(f"values={len(vals)}") for idx, (c, v) in enumerate(zip(cols, vals), 1): print(f"{idx:02d}. {c} <= {v}") if len(cols) != len(vals): print("MISMATCH: column/value count differs") raise SystemExit(1) PYAs per coding guidelines, “always check the stack if there is one for the current PR…”, this seed mismatch should be fixed to keep stacked migration validation trustworthy.
🤖 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 525 - 526, The INSERT INTO config_client statement has one extra boolean value after allow_direct_keys causing a 25-value vs 24-column mismatch; edit the VALUES tuple in the INSERT INTO config_client (...) VALUES (...) statement to remove the redundant boolean (the extra true after allow_direct_keys) so the number and ordering of values exactly matches the column list (refer to the INSERT INTO config_client and the allow_direct_keys column name to locate the row), then run the provided sanity script to confirm columns.count == values.count.framework/configstore/migrations.go (1)
5234-5241:⚠️ Potential issue | 🟠 MajorRefresh
config_hashin the compat backfill.The update at Line 5236 changes persisted client-config state, but this migration still drops the legacy column without recalculating
config_hash. Existing installs can stay on the pre-compat hash shape and keep reconciliation dirty until some later write fixes it. Recompute the hash in the same transaction before Line 5239.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 5234 - 5241, The migration updates compat_convert_text_to_chat from enable_litellm_fallbacks but never recomputes config_hash, leaving reconciler dirty; modify the transaction (within the same block where mig.HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") and after tx.Exec("UPDATE config_client SET compat_convert_text_to_chat = enable_litellm_fallbacks")) to recalculate and update config_hash for affected rows (e.g., run an UPDATE that sets config_hash = <recompute logic> or call the existing hash computation routine) before calling mig.DropColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks"); ensure you reference the same tx and table (config_client / TableClientConfig) so the drop happens only after hashes are refreshed.transports/bifrost-http/handlers/config.go (1)
344-362:⚠️ Potential issue | 🟠 MajorFail the request when the compat plugin update fails.
If
ReloadPluginorRemovePluginerrors here, the handler still falls through toupdatedConfig.Compat = newCompatand can later persist/return success. In this stack,client_config.compatis now the source of truth, so that leaves stored/UI state out of sync with the actual compat behavior.Proposed fix
if newCompat != oldCompat { newEnabled := newCompat.ConvertTextToChat || newCompat.ConvertChatToResponses || newCompat.ShouldDropParams if newEnabled { compatCfg := &compat.Config{ ConvertTextToChat: newCompat.ConvertTextToChat, ConvertChatToResponses: newCompat.ConvertChatToResponses, ShouldDropParams: newCompat.ShouldDropParams, } if err := h.configManager.ReloadPlugin(ctx, compat.PluginName, nil, compatCfg, nil, nil); err != nil { - logger.Warn(fmt.Sprintf("failed to load compat plugin: %v", err)) + logger.Warn("failed to load compat plugin: %v", err) + SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("failed to load compat plugin: %v", err)) + return } } else { disabledCtx := context.WithValue(ctx, PluginDisabledKey, true) if err := h.configManager.RemovePlugin(disabledCtx, compat.PluginName); err != nil { logger.Warn("failed to remove compat plugin: %v", err) + SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("failed to remove compat plugin: %v", err)) + return } } } updatedConfig.Compat = newCompat🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/config.go` around lines 344 - 362, The handler currently ignores errors from h.configManager.ReloadPlugin and RemovePlugin and proceeds to set updatedConfig.Compat, causing stored/UI state to diverge from runtime plugin state; change both error branches so that on error you log and return the error (or a wrapped fmt.Errorf) to abort the request instead of falling through, and only set updatedConfig.Compat = newCompat after the plugin operation succeeds; target the blocks around h.configManager.ReloadPlugin and h.configManager.RemovePlugin and ensure any context'ed RemovePlugin call also returns its error to the caller.
🤖 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 759-762: The rollback SQL uses PostgreSQL-only "ALTER TABLE ...
DROP COLUMN IF EXISTS" which breaks SQLite; update the Rollback closure in the
migration (the Rollback func using tx.WithContext(ctx)) to detect the dialect
via tx.Dialector.Name() and execute a dialect-appropriate statement: for
"postgres" use "ALTER TABLE config_client DROP COLUMN IF EXISTS
enable_litellm_fallbacks" and for "sqlite" use "ALTER TABLE config_client DROP
COLUMN enable_litellm_fallbacks"; ensure the chosen SQL is executed via tx.Exec
and errors are returned as before.
In `@helm-charts/bifrost/values.schema.json`:
- Around line 296-302: The compat object in values.schema.json currently defines
properties convertTextToChat, convertChatToResponses, and shouldDropParams but
allows unknown keys; add "additionalProperties": false to the "compat" schema so
nested unknown or snake_case keys (e.g., convert_text_to_chat) will be rejected;
update the compat object schema (the block with "properties": {
"convertTextToChat", "convertChatToResponses", "shouldDropParams" }) to include
"additionalProperties": false to enforce strict validation.
---
Duplicate comments:
In @.github/workflows/scripts/run-migration-tests.sh:
- Around line 525-526: The INSERT INTO config_client statement has one extra
boolean value after allow_direct_keys causing a 25-value vs 24-column mismatch;
edit the VALUES tuple in the INSERT INTO config_client (...) VALUES (...)
statement to remove the redundant boolean (the extra true after
allow_direct_keys) so the number and ordering of values exactly matches the
column list (refer to the INSERT INTO config_client and the allow_direct_keys
column name to locate the row), then run the provided sanity script to confirm
columns.count == values.count.
In `@framework/configstore/migrations.go`:
- Around line 5234-5241: The migration updates compat_convert_text_to_chat from
enable_litellm_fallbacks but never recomputes config_hash, leaving reconciler
dirty; modify the transaction (within the same block where
mig.HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") and after
tx.Exec("UPDATE config_client SET compat_convert_text_to_chat =
enable_litellm_fallbacks")) to recalculate and update config_hash for affected
rows (e.g., run an UPDATE that sets config_hash = <recompute logic> or call the
existing hash computation routine) before calling
mig.DropColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks"); ensure
you reference the same tx and table (config_client / TableClientConfig) so the
drop happens only after hashes are refreshed.
In `@transports/bifrost-http/handlers/config.go`:
- Around line 344-362: The handler currently ignores errors from
h.configManager.ReloadPlugin and RemovePlugin and proceeds to set
updatedConfig.Compat, causing stored/UI state to diverge from runtime plugin
state; change both error branches so that on error you log and return the error
(or a wrapped fmt.Errorf) to abort the request instead of falling through, and
only set updatedConfig.Compat = newCompat after the plugin operation succeeds;
target the blocks around h.configManager.ReloadPlugin and
h.configManager.RemovePlugin and ensure any context'ed RemovePlugin call also
returns its error to the caller.
🪄 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: df21bc17-4668-4905-85e7-73197bffc688
📒 Files selected for processing (33)
.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.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/main.goplugins/compat/versiontests/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)
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
- examples/configs/withvirtualkeys/config.json
- examples/configs/withpostgresmcpclientsinconfig/config.json
✅ Files skipped from review due to trivial changes (5)
- plugins/compat/version
- docs/providers/supported-providers/overview.mdx
- examples/configs/withprompushgateway/config.json
- docs/openapi/openapi.json
- framework/configstore/clientconfig.go
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/governance/config.json
- tests/integrations/typescript/config.json
- .github/workflows/scripts/test-docker-image.sh
- examples/dockers/data/config.json
- ui/components/ui/accordion.tsx
- transports/bifrost-http/server/plugins.go
- transports/go.mod
- docs/features/litellm-compat.mdx
- docs/openapi/schemas/management/config.yaml
- transports/bifrost-http/lib/config.go
- framework/configstore/tables/clientconfig.go
- framework/configstore/rdb.go
- transports/config.schema.json
- plugins/compat/main.go
- ui/lib/types/config.ts
- ui/app/workspace/config/views/clientSettingsView.tsx
- tests/integrations/python/config.json
- 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
7057a7a to
df3d2b2
Compare
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
fd76fc6 to
905602b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/features/litellm-compat.mdx (1)
9-13:⚠️ Potential issue | 🟡 MinorDocumentation inconsistency: claims "two transformations" but only one is listed.
Line 9 states the plugin provides "two transformations" but only one item follows (Text-to-Chat Conversion). This doesn't align with the three config flags now documented in lines 67-69 and 79-83 (
convert_text_to_chat,convert_chat_to_responses,should_drop_params).Consider updating to reflect all three features, or clarify which are "transformations" vs. other behaviors.
🤖 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 9 - 13, The doc header incorrectly says "two transformations" while the plugin actually exposes three behaviors; update the prose to enumerate/clarify all three features — convert_text_to_chat, convert_chat_to_responses, and should_drop_params — and distinguish which are true "transformations" (e.g., text-to-chat and chat-to-responses) versus other behaviors (e.g., dropping params), and mention that responses include extra_fields.litellm_compat: true when any are applied.
♻️ Duplicate comments (4)
ui/app/workspace/config/views/clientSettingsView.tsx (3)
111-113:⚠️ Potential issue | 🟡 MinorNormalize compat flags to avoid sticky dirty-state.
Comparing optional values directly means
undefined !== falseregisters as a change. When the backend baseline isundefinedbut UI state becomes explicitfalseafter toggling back,hasCoreConfigChangesstays true incorrectly.🔧 Suggested fix
- localConfig.compat?.convert_text_to_chat !== config.compat?.convert_text_to_chat || - localConfig.compat?.convert_chat_to_responses !== config.compat?.convert_chat_to_responses || - localConfig.compat?.should_drop_params !== config.compat?.should_drop_params || + (localConfig.compat?.convert_text_to_chat ?? false) !== (config.compat?.convert_text_to_chat ?? false) || + (localConfig.compat?.convert_chat_to_responses ?? false) !== (config.compat?.convert_chat_to_responses ?? false) || + (localConfig.compat?.should_drop_params ?? false) !== (config.compat?.should_drop_params ?? false) ||,
🤖 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 111 - 113, The comparison logic for compat flags (e.g., localConfig.compat?.convert_text_to_chat, convert_chat_to_responses, should_drop_params) treats undefined and false as different, causing sticky dirty state; fix by normalizing these optional flags before comparing (use a default like false via nullish-coalescing or explicit boolean cast) so the check (used by hasCoreConfigChanges or similar) compares normalized boolean values on both sides (e.g., (localConfig.compat?.convert_text_to_chat ?? false) !== (config.compat?.convert_text_to_chat ?? false)) for each compat property.
333-350:⚠️ Potential issue | 🟠 MajorMove the docs link outside the accordion trigger.
The
Linkis nested insideAccordionTrigger, which renders as a<button>. Nesting an interactive element (anchor) inside another interactive element violates HTML semantics. ThestopPropagation()only prevents click bubbling but doesn't fix keyboard navigation or screen reader accessibility.🔧 Suggested restructure
- <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 cursor-pointer underline text-sm" + 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 333 - 350, The Link (data-testid="litellm-docs-link") is nested inside AccordionTrigger (which renders a <button>), causing nested interactive elements; move the Link out of AccordionTrigger so it is not a descendant of AccordionTrigger (e.g., render the Link immediately after AccordionTrigger or inside AccordionContent), remove the now-unnecessary onClick={(e) => e.stopPropagation()} on the Link, and keep the href/target/rel/data-testid props intact so the docs link remains functional and accessible while AccordionTrigger remains a single interactive element.
361-399:⚠️ Potential issue | 🟡 MinorAdd data-testid attributes to the new compat switches.
The three new
Switchcomponents lackdata-testidattributes, making them difficult to target in E2E tests.🏷️ Suggested additions
<Switch + data-testid="compat-convert-text-to-chat-switch" id="compat-convert-text-to-chat" size="md" ... <Switch + data-testid="compat-convert-chat-to-responses-switch" id="compat-convert-chat-to-responses" size="md" ... <Switch + data-testid="compat-should-drop-params-switch" id="compat-should-drop-params" size="md"As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements with data-testid attributes following the pattern: 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 361 - 399, Add data-testid attributes to the three new Switch components in clientSettingsView.tsx so E2E tests can target them; for each Switch with id "compat-convert-text-to-chat", "compat-convert-chat-to-responses", and "compat-should-drop-params" add a data-testid following the project pattern (e.g., data-testid="compat-switch-convert-text-to-chat", "compat-switch-convert-chat-to-responses", and "compat-switch-should-drop-params") ensuring the attribute is placed on the Switch element so tests can reliably select the interactive control.framework/configstore/migrations.go (1)
5234-5241:⚠️ Potential issue | 🟠 MajorRefresh
config_hashin this migration.Earlier in this migration chain
config_hashmay already be populated, but thisUPDATEchanges the persisted client-config shape afterwards. Recompute and persist the hash for the affectedconfig_clientrows in the same transaction beforeDropColumn, otherwise reconciliation stays stale until a later write.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 5234 - 5241, The migration updates compat_convert_text_to_chat from enable_litellm_fallbacks then drops that column but doesn't refresh the derived config_hash; update the migration to, after running tx.Exec("UPDATE config_client SET compat_convert_text_to_chat = enable_litellm_fallbacks") and before calling mig.DropColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks"), recompute and persist the config_hash for the affected config_client rows within the same transaction (use the same tx and target rows in config_client), ensuring the new hash is saved so reconciliation isn't stale; reference the existing mig.HasColumn, tx.Exec, tables.TableClientConfig, compat_convert_text_to_chat, enable_litellm_fallbacks, and config_hash when locating where to add the hash recompute and update logic.
🧹 Nitpick comments (2)
plugins/compat/main.go (1)
114-124: Avoid silent no-op whenshould_drop_paramsis enabled but model catalog is nil.Right now this path quietly skips work. A debug/warn log will make misconfiguration obvious in production.
Suggested change
- // Compute unsupported parameters to drop based on model catalog allowlist - if p.config.ShouldDropParams && p.modelCatalog != nil { - _, model, _ := req.GetRequestFields() - if model != "" { - if supportedParams := p.modelCatalog.GetSupportedParameters(model); supportedParams != nil { - droppedParams := computeUnsupportedParams(req, supportedParams) - if len(droppedParams) > 0 { - ctx.SetValue(schemas.BifrostContextKeyCompatPluginDroppedParams, droppedParams) - } - } - } - } + // Compute unsupported parameters to drop based on model catalog allowlist + if p.config.ShouldDropParams { + if p.modelCatalog == nil { + p.logger.Debug("compat: should_drop_params enabled but model catalog is nil; skipping unsupported-parameter dropping") + } else { + _, model, _ := req.GetRequestFields() + if model != "" { + if supportedParams := p.modelCatalog.GetSupportedParameters(model); supportedParams != nil { + droppedParams := computeUnsupportedParams(req, supportedParams) + if len(droppedParams) > 0 { + ctx.SetValue(schemas.BifrostContextKeyCompatPluginDroppedParams, 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 114 - 124, When p.config.ShouldDropParams is true but p.modelCatalog is nil, the current branch silently does nothing; add a warning or debug log to surface this misconfiguration. Inside the existing if p.config.ShouldDropParams && p.modelCatalog != nil conditional flow, add an else branch (for when p.modelCatalog == nil) that logs a clear message via the package logger (or ctx logger) indicating ShouldDropParams is enabled but modelCatalog is missing, referencing the plugin instance and model name context as appropriate (use symbols p.config.ShouldDropParams, p.modelCatalog, computeUnsupportedParams, ctx.SetValue, schemas.BifrostContextKeyCompatPluginDroppedParams to locate the code). Ensure the log level is warning or debug so it’s visible in production without changing behavior.transports/bifrost-http/lib/config_test.go (1)
12425-12430: Expand hash coverage to all newcompatfields.Line 12427 currently validates hash sensitivity for only
Compat.ConvertTextToChat. Add checks forCompat.ConvertChatToResponsesandCompat.ShouldDropParamstoo, so each new flag is explicitly guarded.🔧 Suggested test expansion
// Different Compat should produce different hash cc13 := cc1 cc13.Compat.ConvertTextToChat = true hash13, _ := cc13.GenerateClientConfigHash() if hash1 == hash13 { t.Error("Different Compat.ConvertTextToChat should produce different hash") } + + cc13b := cc1 + cc13b.Compat.ConvertChatToResponses = true + hash13b, _ := cc13b.GenerateClientConfigHash() + if hash1 == hash13b { + t.Error("Different Compat.ConvertChatToResponses should produce different hash") + } + + cc13c := cc1 + cc13c.Compat.ShouldDropParams = true + hash13c, _ := cc13c.GenerateClientConfigHash() + if hash1 == hash13c { + t.Error("Different Compat.ShouldDropParams should produce different hash") + }🤖 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 12425 - 12430, The test only checks hash sensitivity for Compat.ConvertTextToChat; update the test around cc13/cc1 and GenerateClientConfigHash to also mutate and assert uniqueness for Compat.ConvertChatToResponses and Compat.ShouldDropParams: create separate copies (e.g., cc13, cc14, cc15) from cc1, flip each flag on Compat (ConvertTextToChat, ConvertChatToResponses, ShouldDropParams) one at a time, call GenerateClientConfigHash for each copy (hash13, hash14, hash15), and add assertions that each new hash is not equal to the original hash1 to ensure the hash changes for all new compat fields.
🤖 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 66-69: Update the docs for LiteLLM Compat to describe all three UI
features (not just Text-to-Chat): change the intro wording that currently says
"two transformations" to "three transformations", add a new "Convert Chat to
Responses" section mirroring the Text-to-Chat pattern that explains what it
does, when to use it, detailed request->response transformation behavior, and
supported provider combinations, and add a "Drop Unsupported Params" section
that explains what parameters get removed, when to enable it, exact
request/response effects, and supported provider combinations; finally, extend
the "Transformations Reference" to include entries for both Convert Chat to
Responses and Drop Unsupported Params with the same level of detail as the
existing Text-to-Chat reference.
---
Outside diff comments:
In `@docs/features/litellm-compat.mdx`:
- Around line 9-13: The doc header incorrectly says "two transformations" while
the plugin actually exposes three behaviors; update the prose to
enumerate/clarify all three features — convert_text_to_chat,
convert_chat_to_responses, and should_drop_params — and distinguish which are
true "transformations" (e.g., text-to-chat and chat-to-responses) versus other
behaviors (e.g., dropping params), and mention that responses include
extra_fields.litellm_compat: true when any are applied.
---
Duplicate comments:
In `@framework/configstore/migrations.go`:
- Around line 5234-5241: The migration updates compat_convert_text_to_chat from
enable_litellm_fallbacks then drops that column but doesn't refresh the derived
config_hash; update the migration to, after running tx.Exec("UPDATE
config_client SET compat_convert_text_to_chat = enable_litellm_fallbacks") and
before calling mig.DropColumn(&tables.TableClientConfig{},
"enable_litellm_fallbacks"), recompute and persist the config_hash for the
affected config_client rows within the same transaction (use the same tx and
target rows in config_client), ensuring the new hash is saved so reconciliation
isn't stale; reference the existing mig.HasColumn, tx.Exec,
tables.TableClientConfig, compat_convert_text_to_chat, enable_litellm_fallbacks,
and config_hash when locating where to add the hash recompute and update logic.
In `@ui/app/workspace/config/views/clientSettingsView.tsx`:
- Around line 111-113: The comparison logic for compat flags (e.g.,
localConfig.compat?.convert_text_to_chat, convert_chat_to_responses,
should_drop_params) treats undefined and false as different, causing sticky
dirty state; fix by normalizing these optional flags before comparing (use a
default like false via nullish-coalescing or explicit boolean cast) so the check
(used by hasCoreConfigChanges or similar) compares normalized boolean values on
both sides (e.g., (localConfig.compat?.convert_text_to_chat ?? false) !==
(config.compat?.convert_text_to_chat ?? false)) for each compat property.
- Around line 333-350: The Link (data-testid="litellm-docs-link") is nested
inside AccordionTrigger (which renders a <button>), causing nested interactive
elements; move the Link out of AccordionTrigger so it is not a descendant of
AccordionTrigger (e.g., render the Link immediately after AccordionTrigger or
inside AccordionContent), remove the now-unnecessary onClick={(e) =>
e.stopPropagation()} on the Link, and keep the href/target/rel/data-testid props
intact so the docs link remains functional and accessible while AccordionTrigger
remains a single interactive element.
- Around line 361-399: Add data-testid attributes to the three new Switch
components in clientSettingsView.tsx so E2E tests can target them; for each
Switch with id "compat-convert-text-to-chat",
"compat-convert-chat-to-responses", and "compat-should-drop-params" add a
data-testid following the project pattern (e.g.,
data-testid="compat-switch-convert-text-to-chat",
"compat-switch-convert-chat-to-responses", and
"compat-switch-should-drop-params") ensuring the attribute is placed on the
Switch element so tests can reliably select the interactive control.
---
Nitpick comments:
In `@plugins/compat/main.go`:
- Around line 114-124: When p.config.ShouldDropParams is true but p.modelCatalog
is nil, the current branch silently does nothing; add a warning or debug log to
surface this misconfiguration. Inside the existing if p.config.ShouldDropParams
&& p.modelCatalog != nil conditional flow, add an else branch (for when
p.modelCatalog == nil) that logs a clear message via the package logger (or ctx
logger) indicating ShouldDropParams is enabled but modelCatalog is missing,
referencing the plugin instance and model name context as appropriate (use
symbols p.config.ShouldDropParams, p.modelCatalog, computeUnsupportedParams,
ctx.SetValue, schemas.BifrostContextKeyCompatPluginDroppedParams to locate the
code). Ensure the log level is warning or debug so it’s visible in production
without changing behavior.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 12425-12430: The test only checks hash sensitivity for
Compat.ConvertTextToChat; update the test around cc13/cc1 and
GenerateClientConfigHash to also mutate and assert uniqueness for
Compat.ConvertChatToResponses and Compat.ShouldDropParams: create separate
copies (e.g., cc13, cc14, cc15) from cc1, flip each flag on Compat
(ConvertTextToChat, ConvertChatToResponses, ShouldDropParams) one at a time,
call GenerateClientConfigHash for each copy (hash13, hash14, hash15), and add
assertions that each new hash is not equal to the original hash1 to ensure the
hash changes for all new compat fields.
🪄 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: 3afd1c1f-5748-4452-885e-86a116119f06
📒 Files selected for processing (33)
.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.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/main.goplugins/compat/versiontests/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 (8)
- plugins/compat/version
- examples/dockers/data/config.json
- tests/integrations/python/config.json
- tests/integrations/typescript/config.json
- helm-charts/bifrost/values.yaml
- .github/workflows/scripts/run-migration-tests.sh
- transports/bifrost-http/handlers/config.go
- docs/openapi/openapi.json
🚧 Files skipped from review as they are similar to previous changes (15)
- .github/workflows/scripts/test-docker-image.sh
- examples/configs/withprompushgateway/config.json
- tests/governance/config.json
- transports/bifrost-http/server/plugins.go
- docs/openapi/schemas/management/config.yaml
- transports/go.mod
- ui/components/ui/accordion.tsx
- transports/config.schema.json
- framework/configstore/rdb.go
- ui/lib/types/config.ts
- helm-charts/bifrost/values.schema.json
- docs/providers/supported-providers/overview.mdx
- framework/configstore/tables/clientconfig.go
- helm-charts/bifrost/templates/_helpers.tpl
- framework/configstore/clientconfig.go
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
905602b to
79eddf4
Compare
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
df3d2b2 to
a591e66
Compare
Contributor
Confidence Score: 4/5Not safe to merge yet — the duplicate A P0
|
| Filename | Overview |
|---|---|
| framework/configstore/migrations.go | New migration adds four compat columns and drops enable_litellm_fallbacks, but unconditionally sets compat_should_convert_params = TRUE for all rows (silently activating the compat plugin on all deployments), does not recompute config_hash, and has an inverted rollback conditional. |
| framework/configstore/clientconfig.go | Hash function switched from always writing `enableLiteLLMFallbacks:{true |
| framework/configstore/tables/clientconfig.go | Adds four compat_* boolean columns with correct GORM tags and DB defaults; schema definition is clean. |
| transports/go.mod | github.com/maximhq/bifrost/plugins/compat v0.1.0 appears twice in the require block; Go treats duplicate require directives as a module graph error that breaks go build. |
| transports/bifrost-http/handlers/config.go | Compat plugin toggle logic correctly gates on newCompat != oldCompat, computes newEnabled across all four flags, and returns after both error paths; no issues found. |
| transports/bifrost-http/server/plugins.go | Startup compat plugin check includes ShouldConvertParams in the OR condition; because the migration unconditionally sets this flag to true, the plugin loads for all existing deployments. |
| plugins/compat/main.go | Plugin logic correctly branches on each individual feature flag; applyParameterConversion is scoped to Bedrock/Vertex/Gemini. |
| ui/lib/types/config.ts | Adds CompatConfig interface and updates CoreConfig; DefaultCoreConfig sets should_convert_params: true matching the new DB default. |
| ui/app/workspace/config/views/clientSettingsView.tsx | Replaces single toggle with per-flag compat controls; uses optional chaining on compat?. despite the type being non-optional. |
Reviews (17): Last reviewed commit: "feat: removes enable_litellm_fallback op..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
a591e66 to
9c75725
Compare
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
79eddf4 to
540542d
Compare
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
540542d to
4386a9d
Compare
sammaji
changed the base branch from
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
to
graphite-base/2260
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
framework/configstore/migrations.go (2)
761-764:⚠️ Potential issue | 🟡 MinorMake the rollback
DROP COLUMNdialect-aware.Line 763 uses
DROP COLUMN IF EXISTS, which SQLite does not support, so rollback will fail on SQLite.Does SQLite support `ALTER TABLE ... DROP COLUMN IF EXISTS`, and what syntax should be used for SQLite versus PostgreSQL when dropping a column conditionally?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 761 - 764, The rollback currently executes "ALTER TABLE config_client DROP COLUMN IF EXISTS enable_litellm_fallbacks" which fails on SQLite; update the Rollback func (the anonymous Rollback: func(tx *gorm.DB) error) to be dialect-aware by checking tx.Dialector.Name() and: for PostgreSQL run the DROP COLUMN ... IF EXISTS SQL via tx.Exec as before, and for SQLite perform the safe column removal sequence (create a temp table without enable_litellm_fallbacks, copy remaining columns, drop the original table, then rename the temp table) or skip the DROP if you prefer not to change schema on SQLite; ensure all tx.Exec calls inside Rollback use tx.WithContext(ctx) and return any exec errors.
5266-5273:⚠️ Potential issue | 🟠 MajorRecompute
config_hashin the same migration.This update changes persisted client-config state, but affected rows keep the pre-compat hash. That can leave config reconciliation dirty until a later write touches the row.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 5266 - 5273, After copying enable_litellm_fallbacks into compat_convert_text_to_chat, recompute and persist the config_hash for those affected client rows in the same transaction so they don’t retain the old hash; specifically, in the migration after the tx.Exec("UPDATE config_client SET compat_convert_text_to_chat = enable_litellm_fallbacks") call add a tx.Exec that recalculates and updates the config_hash column for the same rows (using the same hashing routine your codebase uses for client configs — e.g., the Compute/Generate client config hash helper used elsewhere) and do this before calling mig.DropColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") so TableClientConfig rows have an up-to-date config_hash immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@framework/configstore/migrations.go`:
- Around line 761-764: The rollback currently executes "ALTER TABLE
config_client DROP COLUMN IF EXISTS enable_litellm_fallbacks" which fails on
SQLite; update the Rollback func (the anonymous Rollback: func(tx *gorm.DB)
error) to be dialect-aware by checking tx.Dialector.Name() and: for PostgreSQL
run the DROP COLUMN ... IF EXISTS SQL via tx.Exec as before, and for SQLite
perform the safe column removal sequence (create a temp table without
enable_litellm_fallbacks, copy remaining columns, drop the original table, then
rename the temp table) or skip the DROP if you prefer not to change schema on
SQLite; ensure all tx.Exec calls inside Rollback use tx.WithContext(ctx) and
return any exec errors.
- Around line 5266-5273: After copying enable_litellm_fallbacks into
compat_convert_text_to_chat, recompute and persist the config_hash for those
affected client rows in the same transaction so they don’t retain the old hash;
specifically, in the migration after the tx.Exec("UPDATE config_client SET
compat_convert_text_to_chat = enable_litellm_fallbacks") call add a tx.Exec that
recalculates and updates the config_hash column for the same rows (using the
same hashing routine your codebase uses for client configs — e.g., the
Compute/Generate client config hash helper used elsewhere) and do this before
calling mig.DropColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks")
so TableClientConfig rows have an up-to-date config_hash immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62639205-e346-4755-8556-bff313fe5b09
⛔ Files ignored due to path filters (1)
transports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
.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.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/main.goplugins/compat/versiontests/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 (4)
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
- examples/configs/withvirtualkeys/config.json
- examples/configs/withpostgresmcpclientsinconfig/config.json
- transports/go.mod
✅ Files skipped from review due to trivial changes (9)
- examples/dockers/data/config.json
- examples/configs/withprompushgateway/config.json
- .github/workflows/scripts/test-docker-image.sh
- plugins/compat/version
- tests/integrations/typescript/config.json
- ui/components/ui/accordion.tsx
- docs/providers/supported-providers/overview.mdx
- helm-charts/bifrost/templates/_helpers.tpl
- .github/workflows/scripts/run-migration-tests.sh
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/governance/config.json
- docs/features/litellm-compat.mdx
- docs/openapi/openapi.json
- docs/openapi/schemas/management/config.yaml
- transports/bifrost-http/server/plugins.go
- transports/config.schema.json
- transports/bifrost-http/lib/config.go
- tests/integrations/python/config.json
- helm-charts/bifrost/values.yaml
- transports/bifrost-http/handlers/config.go
- plugins/compat/main.go
- framework/configstore/rdb.go
- transports/bifrost-http/lib/config_test.go
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
4386a9d to
5587929
Compare
sammaji
changed the base branch from
graphite-base/2260
to
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
sammaji
force-pushed
the
graphite-base/2260
branch
from
6c2e458 to
12a24fa
Compare
sammaji
changed the base branch from
graphite-base/2260
to
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
fe7525d to
523c153
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features/litellm-compat.mdx`:
- Around line 139-142: Update the chat→responses table row for "OpenAI (o1-pro)"
so the "Native Responses" column shows "Yes" instead of "No" to match the
narrative; locate the table where the row contains "OpenAI (o1-pro)" and change
the first data cell from "No" to "Yes" (leave the "With Fallback" cell as-is).
- Around line 184-190: The docs list incorrect error metadata keys for the
compat plugin; update the text to remove extra_fields.litellm_compat, rename
extra_fields.model_requested to extra_fields.original_model_requested, and state
that extra_fields.converted_request_type is present only in response metadata
(BifrostResponseExtraFields) not in error metadata (BifrostErrorExtraFields).
Ensure the error-path section only claims extra_fields.provider,
extra_fields.original_model_requested, and
extra_fields.dropped_compat_plugin_params are present, and adjust any lines that
imply the original request type is preserved in BifrostErrorExtraFields to
reference BifrostResponseExtraFields if 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: b8806d61-bbc9-477e-8e68-9bc8880b691e
📒 Files selected for processing (5)
.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.mdx
💤 Files with no reviewable changes (1)
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/scripts/test-docker-image.sh
- .github/workflows/scripts/run-migration-tests.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/scripts/validate-helm-config-fields.sh
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
523c153 to
bc83fba
Compare
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
12a24fa to
dabb480
Compare
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
bc83fba to
c8e2874
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
transports/bifrost-http/handlers/config.go (1)
345-370:⚠️ Potential issue | 🟠 MajorDefer compat plugin mutation until after the config write succeeds.
ReloadPlugin/RemovePluginruns beforeUpdateClientConfig()andReloadClientConfigFromConfigStore(). If either later step fails, the live compat plugin can drift from the persisted config. Move this block after the save/reload path, or roll back tooldCompaton failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/config.go` around lines 345 - 370, The compat plugin is being reloaded/removed (calls to h.configManager.ReloadPlugin and h.configManager.RemovePlugin) before the config persistence steps (UpdateClientConfig and ReloadClientConfigFromConfigStore), which can leave runtime state out of sync if persistence fails; move the entire compat mutation block (the newCompat vs oldCompat logic that sets compatCfg and uses PluginDisabledKey) to execute only after UpdateClientConfig() and ReloadClientConfigFromConfigStore() succeed, or alternatively, after performing ReloadPlugin/RemovePlugin, on any subsequent failure call the inverse operation to restore oldCompat (i.e., reload the old compatCfg or re-add the plugin with oldCompat settings) so runtime and persisted config remain consistent.docs/features/litellm-compat.mdx (2)
9-15:⚠️ Potential issue | 🟡 MinorDocument the third compat feature consistently.
The page still opens with “two transformations”, but the UI/config now expose three compat features, and
Drop Unsupported Paramsstill has no dedicated behavior/reference section. Right now users can enable it without any guidance on what gets removed or when to use it.Also applies to: 98-115, 150-173
🤖 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 9 - 15, Update the opening text to list three compat features (Text-to-Chat Conversion, Chat-to-Responses Conversion, and Drop Unsupported Params) instead of “two transformations”, and add a short dedicated section describing the third feature: explain what “Drop Unsupported Params” does, when it runs, which parameters it can remove, and that dropped keys are listed in extra_fields.dropped_compat_plugin_params; also update the other occurrences referenced (around the blocks at lines ~98-115 and ~150-173) so they consistently mention the third feature and its behavior and examples, ensuring the doc references the returned metadata field extra_fields.converted_request_type for transformed requests and extra_fields.dropped_compat_plugin_params for dropped params.
184-189:⚠️ Potential issue | 🟡 MinorKeep the error-metadata docs aligned with actual compat errors.
This section still claims transformed errors expose
extra_fields.converted_request_type/ original request type. The compat error path only guaranteesextra_fields.provider,extra_fields.original_model_requested, andextra_fields.dropped_compat_plugin_params;converted_request_typebelongs in transformed response metadata, not error metadata.🤖 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 184 - 189, Update the documentation to remove `extra_fields.converted_request_type` from the list of fields exposed on transformed request errors and instead document it under transformed response metadata; ensure the error-metadata bullet list only includes `extra_fields.provider`, `extra_fields.original_model_requested`, and `extra_fields.dropped_compat_plugin_params` (and remove the claim that the original request type is preserved via `extra_fields.converted_request_type` in error metadata).framework/configstore/migrations.go (1)
6075-6089:⚠️ Potential issue | 🟠 MajorRollback never restores the legacy column.
After a successful migrate,
enable_litellm_fallbacksis already gone, so this block is skipped and downgrade drops thecompat_*columns without recreating the old schema. Even if the condition were true, it drops the legacy column before trying to backfill it, so theUPDATEcannot work.🛠️ Suggested fix
Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) mig := tx.Migrator() - if tx.Migrator().HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") { - if err := tx.Exec("ALTER TABLE config_client DROP COLUMN enable_litellm_fallbacks").Error; err != nil { - return err - } - if err := tx.Exec("UPDATE config_client SET enable_litellm_fallbacks = compat_convert_text_to_chat").Error; err != nil { - return err - } + 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 + } + } + if mig.HasColumn(&tables.TableClientConfig{}, "compat_convert_text_to_chat") { + if err := tx.Exec("UPDATE config_client SET enable_litellm_fallbacks = compat_convert_text_to_chat").Error; err != nil { + return err + } } for _, col := range []string{ "compat_convert_text_to_chat", "compat_convert_chat_to_responses", "compat_should_drop_params",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6075 - 6089, The rollback currently drops the compat_* columns without recreating or backfilling the legacy enable_litellm_fallbacks column and even attempts UPDATE after dropping it; fix by ensuring the legacy column is present before any backfill: in the migration/downgrade block check HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") and if false run an ALTER TABLE ... ADD COLUMN enable_litellm_fallbacks BOOLEAN (or the original type) via tx.Exec to recreate it, then perform the UPDATE "UPDATE config_client SET enable_litellm_fallbacks = compat_convert_text_to_chat" to backfill, and only after successful backfill drop the compat_* columns (use mig.DropColumn on tables.TableClientConfig for the compat_* names).ui/app/workspace/config/views/clientSettingsView.tsx (2)
335-350:⚠️ Potential issue | 🟠 MajorMove the docs link outside the accordion trigger.
Line 340 places a
LinkinsideAccordionTrigger, creating nested interactive controls and accessibility/semantics issues. Keep trigger content non-interactive and render the docs link as a sibling.Suggested structure change
- <AccordionTrigger className="py-0 hover:no-underline"> + <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 ... onClick={(e) => e.stopPropagation()}>Learn more</Link> - </p> + <p className="text-muted-foreground text-sm font-normal"> + Request type conversion and parameter dropping. + </p> </div> </AccordionTrigger> + <div className="pt-2"> + <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" + > + 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 335 - 350, The Link for the docs is inside AccordionTrigger (creating nested interactive controls); move that <Link ... data-testid="litellm-docs-link"> out of the AccordionTrigger and render it as a sibling element directly after the AccordionTrigger block in clientSettingsView (so the trigger remains non-interactive), preserve the visual classes and href/target/rel/data-testid, and remove the onClick stopPropagation handler since it will no longer be inside the trigger; ensure spacing/layout matches the previous appearance.
335-401:⚠️ Potential issue | 🟡 MinorAdd
data-testidattributes to new compat interactive controls.The new
AccordionTriggerand threeSwitchcontrols are interactive and currently lack test IDs, which breaks the UI selector convention for new controls.As per coding guidelines, `ui/**/*.{tsx,ts}` must add `data-testid` to new interactive elements using the `--` pattern.Suggested test IDs
- <AccordionTrigger className="py-0 hover:no-underline"> + <AccordionTrigger + className="py-0 hover:no-underline" + data-testid="compat-accordion-trigger" + > ... <Switch + data-testid="compat-convert-text-to-chat-switch" id="compat-convert-text-to-chat" ... /> ... <Switch + data-testid="compat-convert-chat-to-responses-switch" id="compat-convert-chat-to-responses" ... /> ... <Switch + data-testid="compat-should-drop-params-switch" id="compat-should-drop-params" ... />🤖 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 335 - 401, The new interactive controls (AccordionTrigger and the three Switch components in clientSettingsView.tsx) are missing data-testid attributes; add data-testid following the <entity>-<element>-<qualifier> pattern to each interactive element so tests can reliably select them. Specifically, add data-testid="compat-accordion-trigger" to the AccordionTrigger, and add data-testid attributes to the three Switches corresponding to their ids, e.g. data-testid="compat-convert-text-to-chat-switch", data-testid="compat-convert-chat-to-responses-switch", and data-testid="compat-should-drop-params-switch" (match the existing id names like "compat-convert-text-to-chat", "compat-convert-chat-to-responses", "compat-should-drop-params") and ensure these attributes are applied alongside the existing props and disabled logic.
🧹 Nitpick comments (1)
transports/config.schema.json (1)
97-115: Schema change looks correct and aligns with the PR objectives.The new
compatobject properly replaces the singleenable_litellm_fallbacksboolean with three granular flags. The structure is valid, descriptions are clear, andadditionalProperties: falseenforces strict validation.Consider adding explicit
default: falseto each boolean property for documentation clarity and to make the default behavior explicit in the schema:💡 Optional: Add default values for clarity
"compat": { "type": "object", "description": "Compat plugin configuration for request type conversion and parameter dropping", "properties": { "convert_text_to_chat": { "type": "boolean", - "description": "Convert text completion requests to chat for models that only support chat" + "description": "Convert text completion requests to chat for models that only support chat", + "default": false }, "convert_chat_to_responses": { "type": "boolean", - "description": "Convert chat completion requests to responses for models that only support responses" + "description": "Convert chat completion requests to responses for models that only support responses", + "default": false }, "should_drop_params": { "type": "boolean", - "description": "Drop unsupported parameters based on model catalog allowlist" + "description": "Drop unsupported parameters based on model catalog allowlist", + "default": false } }, "additionalProperties": false },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/config.schema.json` around lines 97 - 115, Add explicit default: false to each boolean property inside the compat object (convert_text_to_chat, convert_chat_to_responses, should_drop_params) so the schema documents explicit defaults; update the compat object properties to include default: false for those three keys while keeping additionalProperties: false and existing descriptions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/config.go`:
- Around line 351-356: The build is failing because the core module is missing
the compat plugin dependency referenced by compat.Config and compat.PluginName
used in handlers/config.go (and the call h.configManager.ReloadPlugin). Fix by
adding the compat module to core's go.mod require section (e.g., add
github.com/maximhq/bifrost/plugins/compat v1.x.x) and run `go mod tidy` to
update go.sum so imports resolve; ensure the declared version matches the plugin
release you expect.
---
Duplicate comments:
In `@docs/features/litellm-compat.mdx`:
- Around line 9-15: Update the opening text to list three compat features
(Text-to-Chat Conversion, Chat-to-Responses Conversion, and Drop Unsupported
Params) instead of “two transformations”, and add a short dedicated section
describing the third feature: explain what “Drop Unsupported Params” does, when
it runs, which parameters it can remove, and that dropped keys are listed in
extra_fields.dropped_compat_plugin_params; also update the other occurrences
referenced (around the blocks at lines ~98-115 and ~150-173) so they
consistently mention the third feature and its behavior and examples, ensuring
the doc references the returned metadata field
extra_fields.converted_request_type for transformed requests and
extra_fields.dropped_compat_plugin_params for dropped params.
- Around line 184-189: Update the documentation to remove
`extra_fields.converted_request_type` from the list of fields exposed on
transformed request errors and instead document it under transformed response
metadata; ensure the error-metadata bullet list only includes
`extra_fields.provider`, `extra_fields.original_model_requested`, and
`extra_fields.dropped_compat_plugin_params` (and remove the claim that the
original request type is preserved via `extra_fields.converted_request_type` in
error metadata).
In `@framework/configstore/migrations.go`:
- Around line 6075-6089: The rollback currently drops the compat_* columns
without recreating or backfilling the legacy enable_litellm_fallbacks column and
even attempts UPDATE after dropping it; fix by ensuring the legacy column is
present before any backfill: in the migration/downgrade block check
HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") and if false
run an ALTER TABLE ... ADD COLUMN enable_litellm_fallbacks BOOLEAN (or the
original type) via tx.Exec to recreate it, then perform the UPDATE "UPDATE
config_client SET enable_litellm_fallbacks = compat_convert_text_to_chat" to
backfill, and only after successful backfill drop the compat_* columns (use
mig.DropColumn on tables.TableClientConfig for the compat_* names).
In `@transports/bifrost-http/handlers/config.go`:
- Around line 345-370: The compat plugin is being reloaded/removed (calls to
h.configManager.ReloadPlugin and h.configManager.RemovePlugin) before the config
persistence steps (UpdateClientConfig and ReloadClientConfigFromConfigStore),
which can leave runtime state out of sync if persistence fails; move the entire
compat mutation block (the newCompat vs oldCompat logic that sets compatCfg and
uses PluginDisabledKey) to execute only after UpdateClientConfig() and
ReloadClientConfigFromConfigStore() succeed, or alternatively, after performing
ReloadPlugin/RemovePlugin, on any subsequent failure call the inverse operation
to restore oldCompat (i.e., reload the old compatCfg or re-add the plugin with
oldCompat settings) so runtime and persisted config remain consistent.
In `@ui/app/workspace/config/views/clientSettingsView.tsx`:
- Around line 335-350: The Link for the docs is inside AccordionTrigger
(creating nested interactive controls); move that <Link ...
data-testid="litellm-docs-link"> out of the AccordionTrigger and render it as a
sibling element directly after the AccordionTrigger block in clientSettingsView
(so the trigger remains non-interactive), preserve the visual classes and
href/target/rel/data-testid, and remove the onClick stopPropagation handler
since it will no longer be inside the trigger; ensure spacing/layout matches the
previous appearance.
- Around line 335-401: The new interactive controls (AccordionTrigger and the
three Switch components in clientSettingsView.tsx) are missing data-testid
attributes; add data-testid following the <entity>-<element>-<qualifier> pattern
to each interactive element so tests can reliably select them. Specifically, add
data-testid="compat-accordion-trigger" to the AccordionTrigger, and add
data-testid attributes to the three Switches corresponding to their ids, e.g.
data-testid="compat-convert-text-to-chat-switch",
data-testid="compat-convert-chat-to-responses-switch", and
data-testid="compat-should-drop-params-switch" (match the existing id names like
"compat-convert-text-to-chat", "compat-convert-chat-to-responses",
"compat-should-drop-params") and ensure these attributes are applied alongside
the existing props and disabled logic.
---
Nitpick comments:
In `@transports/config.schema.json`:
- Around line 97-115: Add explicit default: false to each boolean property
inside the compat object (convert_text_to_chat, convert_chat_to_responses,
should_drop_params) so the schema documents explicit defaults; update the compat
object properties to include default: false for those three keys while keeping
additionalProperties: false and existing descriptions 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: 9a366a1f-1f3e-480a-846a-6449e6a5032e
📒 Files selected for processing (33)
.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.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/compat/main.goplugins/compat/versiontests/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 (9)
- tests/integrations/typescript/config.json
- plugins/compat/version
- tests/integrations/python/config.json
- docs/providers/supported-providers/overview.mdx
- .github/workflows/scripts/test-docker-image.sh
- examples/dockers/data/config.json
- tests/governance/config.json
- ui/components/ui/accordion.tsx
- docs/openapi/openapi.json
🚧 Files skipped from review as they are similar to previous changes (10)
- .github/workflows/scripts/validate-helm-config-fields.sh
- examples/configs/withprompushgateway/config.json
- .github/workflows/scripts/run-migration-tests.sh
- helm-charts/bifrost/values.yaml
- transports/bifrost-http/server/plugins.go
- transports/go.mod
- helm-charts/bifrost/values.schema.json
- framework/configstore/clientconfig.go
- transports/bifrost-http/lib/config_test.go
- framework/configstore/tables/clientconfig.go
sammaji
changed the base branch from
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
to
graphite-base/2260
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
c8e2874 to
59049a3
Compare
sammaji
force-pushed
the
graphite-base/2260
branch
from
dabb480 to
ef6fb53
Compare
sammaji
changed the base branch from
graphite-base/2260
to
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
59049a3 to
b9cc6b7
Compare
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
ef6fb53 to
a596ed4
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
framework/configstore/migrations.go (2)
6110-6132:⚠️ Potential issue | 🟠 MajorRestore the legacy column before dropping
compat_*fields.After a successful migrate,
enable_litellm_fallbacksis absent, so Lines 6113-6120 never run and rollback drops thecompat_*columns without recreating the old schema. If the column does exist, the current code drops it before copying the data back, so theUPDATEcannot succeed.🛠️ Suggested fix
Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) mig := tx.Migrator() - if tx.Migrator().HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") { - if err := tx.Exec("ALTER TABLE config_client DROP COLUMN enable_litellm_fallbacks").Error; err != nil { - return err - } - if err := tx.Exec("UPDATE config_client SET enable_litellm_fallbacks = compat_convert_text_to_chat").Error; err != nil { - return err - } + 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 + } + } + if mig.HasColumn(&tables.TableClientConfig{}, "compat_convert_text_to_chat") { + if err := tx.Exec("UPDATE config_client SET enable_litellm_fallbacks = compat_convert_text_to_chat").Error; err != nil { + return err + } } for _, col := range []string{ "compat_convert_text_to_chat", "compat_convert_chat_to_responses", "compat_should_drop_params",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6110 - 6132, The Rollback closure in Rollback should restore the legacy enable_litellm_fallbacks column before removing compat_* columns and must not drop it before copying data; modify Rollback (tx, mig, tables.TableClientConfig) to: if enable_litellm_fallbacks is missing, add/create that column first (e.g., via tx.Migrator().AddColumn or appropriate ALTER TABLE ADD COLUMN), then run an UPDATE to populate enable_litellm_fallbacks from compat_convert_text_to_chat (or other compat_* sources as intended), and only after the data is copied successfully iterate over compat_convert_text_to_chat, compat_convert_chat_to_responses, compat_should_drop_params and drop those columns via mig.DropColumn; ensure you remove the current code path that drops enable_litellm_fallbacks before the UPDATE and use tx.WithContext(ctx) for all DDL/DML operations.
800-804:⚠️ Potential issue | 🟡 MinorMake this rollback dialect-safe for SQLite.
Line 802 still uses
ALTER TABLE ... DROP COLUMN IF EXISTS. SQLite supportsDROP COLUMN column-name, but not theIF EXISTSmodifier, so this rollback still fails on SQLite. (sqlite.org)🛠️ Suggested fix
Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) - if err := tx.Exec("ALTER TABLE config_client DROP COLUMN IF EXISTS enable_litellm_fallbacks").Error; err != nil { - return err + if tx.Migrator().HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") { + stmt := "ALTER TABLE config_client DROP COLUMN IF EXISTS enable_litellm_fallbacks" + if tx.Dialector.Name() == "sqlite" { + stmt = "ALTER TABLE config_client DROP COLUMN enable_litellm_fallbacks" + } + if err := tx.Exec(stmt).Error; err != nil { + return err + } } return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 800 - 804, The rollback uses "ALTER TABLE config_client DROP COLUMN IF EXISTS enable_litellm_fallbacks" which fails on SQLite because SQLite does not support the IF EXISTS modifier; update the Rollback anonymous function to branch on the dialect (use tx.Dialector.Name() or equivalent) and execute "ALTER TABLE config_client DROP COLUMN enable_litellm_fallbacks" for sqlite while keeping the "DROP COLUMN IF EXISTS ..." statement for other dialects, ensuring you still call tx.Exec and return any error; reference the Rollback func, tx.Exec call, and the column name enable_litellm_fallbacks when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@framework/configstore/migrations.go`:
- Around line 6110-6132: The Rollback closure in Rollback should restore the
legacy enable_litellm_fallbacks column before removing compat_* columns and must
not drop it before copying data; modify Rollback (tx, mig,
tables.TableClientConfig) to: if enable_litellm_fallbacks is missing, add/create
that column first (e.g., via tx.Migrator().AddColumn or appropriate ALTER TABLE
ADD COLUMN), then run an UPDATE to populate enable_litellm_fallbacks from
compat_convert_text_to_chat (or other compat_* sources as intended), and only
after the data is copied successfully iterate over compat_convert_text_to_chat,
compat_convert_chat_to_responses, compat_should_drop_params and drop those
columns via mig.DropColumn; ensure you remove the current code path that drops
enable_litellm_fallbacks before the UPDATE and use tx.WithContext(ctx) for all
DDL/DML operations.
- Around line 800-804: The rollback uses "ALTER TABLE config_client DROP COLUMN
IF EXISTS enable_litellm_fallbacks" which fails on SQLite because SQLite does
not support the IF EXISTS modifier; update the Rollback anonymous function to
branch on the dialect (use tx.Dialector.Name() or equivalent) and execute "ALTER
TABLE config_client DROP COLUMN enable_litellm_fallbacks" for sqlite while
keeping the "DROP COLUMN IF EXISTS ..." statement for other dialects, ensuring
you still call tx.Exec and return any error; reference the Rollback func,
tx.Exec call, and the column name enable_litellm_fallbacks when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc720738-67c0-49fc-8fc6-d318ba411fc3
📒 Files selected for processing (32)
.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.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/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)
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
- examples/configs/withvirtualkeys/config.json
- examples/configs/withpostgresmcpclientsinconfig/config.json
✅ Files skipped from review due to trivial changes (9)
- examples/dockers/data/config.json
- tests/integrations/typescript/config.json
- docs/providers/supported-providers/overview.mdx
- tests/governance/config.json
- tests/integrations/python/config.json
- examples/configs/withprompushgateway/config.json
- .github/workflows/scripts/test-docker-image.sh
- docs/openapi/schemas/management/config.yaml
- ui/app/workspace/config/views/clientSettingsView.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- .github/workflows/scripts/validate-helm-config-fields.sh
- ui/components/ui/accordion.tsx
- helm-charts/bifrost/values.yaml
- docs/openapi/openapi.json
- transports/config.schema.json
- plugins/compat/main.go
- framework/configstore/clientconfig.go
- .github/workflows/scripts/run-migration-tests.sh
- ui/lib/types/config.ts
- framework/configstore/tables/clientconfig.go
- transports/bifrost-http/handlers/config.go
- transports/bifrost-http/lib/config.go
- docs/features/litellm-compat.mdx
- transports/bifrost-http/lib/config_test.go
sammaji
changed the base branch from
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
to
graphite-base/2260
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
b9cc6b7 to
2734893
Compare
sammaji
force-pushed
the
graphite-base/2260
branch
from
a596ed4 to
ef3b1d3
Compare
sammaji
changed the base branch from
graphite-base/2260
to
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
2734893 to
05c065e
Compare
sammaji
force-pushed
the
03-12-fix_conversions_for_litellm_compat_to_happen_at_the_provider_level
branch
from
ef3b1d3 to
6a6d436
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
docs/features/litellm-compat.mdx (2)
7-15:⚠️ Potential issue | 🟡 MinorThe intro still underspecifies the new compat surface.
Line 9 still says "two transformations" while the list now exposes three toggles, and
Drop Unsupported Paramsstill doesn't get the same detailed section/reference as the other two behaviors. That leaves the third switch effectively undocumented.🤖 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 7 - 15, Update the intro under "Compatibility Transformations" to correctly state there are three transformations (Text-to-Chat Conversion, Chat-to-Responses Conversion, and Drop Unsupported Params) and expand the documentation for "Drop Unsupported Params" so it mirrors the other two: describe what parameters can be dropped, when the drop occurs, the shape/location of resulting metadata (extra_fields.dropped_compat_plugin_params and how keys are listed), and include a short example of a request before/after conversion; reference the headings/labels "Text-to-Chat Conversion", "Chat-to-Responses Conversion", "Drop Unsupported Params", and the metadata keys "extra_fields.converted_request_type" and "extra_fields.dropped_compat_plugin_params" to ensure readers can locate the behavior in the doc.
182-189:⚠️ Potential issue | 🟠 MajorError metadata section is still documenting the wrong compat fields.
This section still claims
extra_fields.converted_request_typeis present on compat error payloads and implies the original request type is preserved there. As written, consumers will look for error metadata that won't exist.🤖 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 182 - 189, The docs incorrectly claim extra_fields.converted_request_type exists and that the original request type is preserved; remove any mention of extra_fields.converted_request_type and the assertion that original request type is preserved, and update the Error Handling paragraph to state that the compat layer does NOT include the original request type in error metadata; keep and verify the other fields (extra_fields.provider, extra_fields.original_model_requested, extra_fields.dropped_compat_plugin_params) are correctly described and adjust wording to explicitly say which metadata is present versus which is not (refer to the current compat error payload shape when editing).framework/configstore/migrations.go (2)
6126-6146:⚠️ Potential issue | 🟠 MajorRestore the legacy column before copying data back.
After a successful migrate,
enable_litellm_fallbacksis absent, so Line 6129 skips the restoration path and rollback only removes the newcompat_*columns. If the legacy column does exist, Lines 6130-6133 drop it before writing into it, so that branch fails immediately. Downgrades to pre-compat revisions stay broken.🛠️ Suggested fix
Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) mig := tx.Migrator() - if tx.Migrator().HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") { - if err := tx.Exec("ALTER TABLE config_client DROP COLUMN enable_litellm_fallbacks").Error; err != nil { - return err - } - if err := tx.Exec("UPDATE config_client SET enable_litellm_fallbacks = compat_convert_text_to_chat").Error; err != nil { - return err - } + 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 + } + } + if mig.HasColumn(&tables.TableClientConfig{}, "compat_convert_text_to_chat") { + if err := tx.Exec("UPDATE config_client SET enable_litellm_fallbacks = compat_convert_text_to_chat").Error; err != nil { + return err + } } for _, col := range []string{ "compat_convert_text_to_chat", "compat_convert_chat_to_responses", "compat_should_drop_params",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6126 - 6146, The rollback branch in the Rollback function currently drops or skips the legacy column and then tries to UPDATE it, breaking downgrades; change the flow in Rollback (using tx, mig, tx.Migrator(), and HasColumn checks) so that if the legacy column "enable_litellm_fallbacks" does not exist you CREATE/ADD that column first (using mig.AddColumn or ALTER TABLE ADD), and if it does exist do NOT DROP it before copying; then run the UPDATE ("UPDATE config_client SET enable_litellm_fallbacks = compat_convert_text_to_chat") to restore values, and only after the copy complete proceed to drop the new compat_* columns ("compat_convert_text_to_chat", "compat_convert_chat_to_responses", "compat_should_drop_params") via mig.DropColumn as currently implemented.
800-803:⚠️ Potential issue | 🟠 MajorMake this rollback dialect-aware.
Line 802 still uses
ALTER TABLE ... DROP COLUMN IF EXISTS, which SQLite rejects. That breaks rollback on the repo’s SQLite path even though this file already branches ontx.Dialector.Name()for other DDL differences.🛠️ Suggested fix
Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) - if err := tx.Exec("ALTER TABLE config_client DROP COLUMN IF EXISTS enable_litellm_fallbacks").Error; err != nil { - return err + if tx.Migrator().HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") { + stmt := "ALTER TABLE config_client DROP COLUMN IF EXISTS enable_litellm_fallbacks" + if tx.Dialector.Name() == "sqlite" { + stmt = "ALTER TABLE config_client DROP COLUMN enable_litellm_fallbacks" + } + if err := tx.Exec(stmt).Error; err != nil { + return err + } } return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 800 - 803, The rollback currently always runs tx.Exec("ALTER TABLE config_client DROP COLUMN IF EXISTS enable_litellm_fallbacks") which fails on SQLite; update the Rollback closure to branch on tx.Dialector.Name() (like other migrations) and run dialect-specific SQL: for Postgres/MySQL keep the ALTER TABLE ... DROP COLUMN IF EXISTS, and for sqlite either skip the DROP or perform the safe SQLite pattern (table recreate / copy without enable_litellm_fallbacks) using tx.Exec; reference the Rollback func, tx.Dialector.Name(), tx.Exec and the column name enable_litellm_fallbacks when implementing the fix.
🧹 Nitpick comments (1)
ui/lib/types/config.ts (1)
477-477: Consider a migration-safe transitional type forcompat.Line 477 makes
compatstrictly required, but this PR also supports legacy configs during migration (and UI code already handles missing compat values). A transitional optional/partial shape can prevent callers from over-trusting the type while legacy payloads still exist.♻️ Suggested transitional typing
export interface CoreConfig { drop_excess_requests: boolean; initial_pool_size: number; prometheus_labels: string[]; enable_logging: boolean; disable_content_logging: boolean; disable_db_pings_in_health: boolean; log_retention_days: number; enforce_auth_on_inference: boolean; allow_direct_keys: boolean; allowed_origins: string[]; allowed_headers: string[]; max_request_body_size_mb: number; - compat: CompatConfig; + compat?: Partial<CompatConfig>; mcp_agent_depth: number; mcp_tool_execution_timeout: number; mcp_code_mode_binding_level?: string; mcp_tool_sync_interval: number; mcp_disable_auto_tool_inject: boolean;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/config.ts` at line 477, The `compat` property is currently required (`compat: CompatConfig;`) which breaks migration support for legacy configs; update the type declaration to a migration-safe, optional/partial shape (e.g., make `compat` optional and use `Partial<CompatConfig>` or `CompatConfig | Partial<CompatConfig>`) so callers can't assume full presence while legacy payloads are handled. Locate the `compat` field in the config type definition and change its type to an optional/partial form and run typechecks to ensure existing code paths that already handle missing `compat` still compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/features/litellm-compat.mdx`:
- Around line 7-15: Update the intro under "Compatibility Transformations" to
correctly state there are three transformations (Text-to-Chat Conversion,
Chat-to-Responses Conversion, and Drop Unsupported Params) and expand the
documentation for "Drop Unsupported Params" so it mirrors the other two:
describe what parameters can be dropped, when the drop occurs, the
shape/location of resulting metadata (extra_fields.dropped_compat_plugin_params
and how keys are listed), and include a short example of a request before/after
conversion; reference the headings/labels "Text-to-Chat Conversion",
"Chat-to-Responses Conversion", "Drop Unsupported Params", and the metadata keys
"extra_fields.converted_request_type" and
"extra_fields.dropped_compat_plugin_params" to ensure readers can locate the
behavior in the doc.
- Around line 182-189: The docs incorrectly claim
extra_fields.converted_request_type exists and that the original request type is
preserved; remove any mention of extra_fields.converted_request_type and the
assertion that original request type is preserved, and update the Error Handling
paragraph to state that the compat layer does NOT include the original request
type in error metadata; keep and verify the other fields (extra_fields.provider,
extra_fields.original_model_requested,
extra_fields.dropped_compat_plugin_params) are correctly described and adjust
wording to explicitly say which metadata is present versus which is not (refer
to the current compat error payload shape when editing).
In `@framework/configstore/migrations.go`:
- Around line 6126-6146: The rollback branch in the Rollback function currently
drops or skips the legacy column and then tries to UPDATE it, breaking
downgrades; change the flow in Rollback (using tx, mig, tx.Migrator(), and
HasColumn checks) so that if the legacy column "enable_litellm_fallbacks" does
not exist you CREATE/ADD that column first (using mig.AddColumn or ALTER TABLE
ADD), and if it does exist do NOT DROP it before copying; then run the UPDATE
("UPDATE config_client SET enable_litellm_fallbacks =
compat_convert_text_to_chat") to restore values, and only after the copy
complete proceed to drop the new compat_* columns
("compat_convert_text_to_chat", "compat_convert_chat_to_responses",
"compat_should_drop_params") via mig.DropColumn as currently implemented.
- Around line 800-803: The rollback currently always runs tx.Exec("ALTER TABLE
config_client DROP COLUMN IF EXISTS enable_litellm_fallbacks") which fails on
SQLite; update the Rollback closure to branch on tx.Dialector.Name() (like other
migrations) and run dialect-specific SQL: for Postgres/MySQL keep the ALTER
TABLE ... DROP COLUMN IF EXISTS, and for sqlite either skip the DROP or perform
the safe SQLite pattern (table recreate / copy without enable_litellm_fallbacks)
using tx.Exec; reference the Rollback func, tx.Dialector.Name(), tx.Exec and the
column name enable_litellm_fallbacks when implementing the fix.
---
Nitpick comments:
In `@ui/lib/types/config.ts`:
- Line 477: The `compat` property is currently required (`compat:
CompatConfig;`) which breaks migration support for legacy configs; update the
type declaration to a migration-safe, optional/partial shape (e.g., make
`compat` optional and use `Partial<CompatConfig>` or `CompatConfig |
Partial<CompatConfig>`) so callers can't assume full presence while legacy
payloads are handled. Locate the `compat` field in the config type definition
and change its type to an optional/partial form and run typechecks to ensure
existing code paths that already handle missing `compat` still compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf3bdf6f-7a6f-43af-a02a-babdb01daa7b
📒 Files selected for processing (32)
.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.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/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)
- .github/workflows/configs/withpostgresmcpclientsinconfig/config.json
- examples/configs/withvirtualkeys/config.json
- examples/configs/withpostgresmcpclientsinconfig/config.json
✅ Files skipped from review due to trivial changes (11)
- examples/dockers/data/config.json
- .github/workflows/scripts/test-docker-image.sh
- examples/configs/withprompushgateway/config.json
- tests/governance/config.json
- docs/providers/supported-providers/overview.mdx
- transports/bifrost-http/server/plugins.go
- tests/integrations/typescript/config.json
- tests/integrations/python/config.json
- docs/openapi/schemas/management/config.yaml
- docs/openapi/openapi.json
- .github/workflows/scripts/run-migration-tests.sh
🚧 Files skipped from review as they are similar to previous changes (10)
- .github/workflows/scripts/validate-helm-config-fields.sh
- helm-charts/bifrost/values.yaml
- transports/bifrost-http/lib/config.go
- transports/go.mod
- framework/configstore/clientconfig.go
- helm-charts/bifrost/values.schema.json
- plugins/compat/main.go
- transports/bifrost-http/handlers/config.go
- ui/app/workspace/config/views/clientSettingsView.tsx
- 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
6a6d436 to
dff3960
Compare
sammaji
force-pushed
the
03-24-feat_removes_enable_litellm_fallback_option_for_more_granular_settings
branch
from
05c065e to
c4b8a62
Compare
Comment on lines
+6119
to
+6121
| if err := tx.Exec("UPDATE config_client SET compat_should_convert_params = TRUE").Error; err != nil { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
should_convert_params silently activated for all existing deployments
The unconditional UPDATE sets compat_should_convert_params = TRUE for every row in config_client — not just those where enable_litellm_fallbacks was true. Because the startup check in plugins.go line 220 is cc.ConvertTextToChat || ... || cc.ShouldConvertParams, this causes the compat plugin to be loaded for all existing deployments after the migration, even ones that never had enable_litellm_fallbacks: true.
The concrete effect: applyParameterConversion (specifically normalizeDeveloperRoleForChatRequest) will now run for every Bedrock, Vertex, and Gemini request that uses role: "developer", silently converting it to role: "system". This is an undisclosed behavior change for users who opted out of LiteLLM compat. The PR description states "the migration preserves existing behavior", which is inaccurate for this flag.
To preserve the prior opt-out semantics, the backfill should be scoped to rows that already had enable_litellm_fallbacks = true:
// Only enable ShouldConvertParams for rows that had enable_litellm_fallbacks = true,
// preserving the no-op behavior for deployments that never opted in.
if mig.HasColumn(&tables.TableClientConfig{}, "enable_litellm_fallbacks") {
if err := tx.Exec("UPDATE config_client SET compat_should_convert_params = enable_litellm_fallbacks").Error; err != nil {
return err
}
} else {
// Column was already removed (re-entrant migration run) — leave as-is.
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the single
enable_litellm_fallbacksboolean configuration with three granular compat plugin feature flags:convert_text_to_chat,convert_chat_to_responses, andshould_drop_params. This provides more fine-grained control over LiteLLM compatibility features.Changes
enable_litellm_fallbackswith acompatobject containing three boolean flagsenable_litellm_fallbacksvalues to the newcompat_convert_text_to_chatcolumnThe migration preserves existing behavior by setting
convert_text_to_chat = trueonly when the originalenable_litellm_fallbackswastrue.Type of change
Affected areas
How to test
Validate the configuration changes and UI updates:
Screenshots/Recordings
The UI now shows an expandable "LiteLLM Compat" section with three individual toggles instead of a single "Enable LiteLLM Fallbacks" switch.
Breaking changes
Migration Required: Existing configurations using
enable_litellm_fallbacksneed to be updated to use the newcompatobject structure. The database migration handles this automatically, but JSON configurations and Helm values need manual updates.Before:
{ "client_config": { "enable_litellm_fallbacks": true } }After:
{ "client_config": { "compat": { "convert_text_to_chat": true, "convert_chat_to_responses": false, "should_drop_params": false } } }Related issues
Improves granular control over LiteLLM compatibility features as requested in feature discussions.
Security considerations
No security implications - this is a configuration structure change that maintains the same plugin functionality with more granular control.
Checklist
docs/contributing/README.mdand followed the guidelines