refactor: standardize empty array conventions for VK Provider & MCP Configs, and makes Provider Config weight optional for routing#1932
Merged
Pratham-Mishra04 merged 1 commit intov1.5.0from Mar 18, 2026
Conversation
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
18 tasks
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughEmpty Virtual Key provider or MCP configs are now deny-by-default; provider weights are nullable and excluded from weighted routing when null. Migration backfills explicit VK configs and recomputes hashes. MCP include-client/tool context keys moved to core/schemas. Docs, schemas, transports, UI, and tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant MCP
participant Provider
rect rgba(200,230,250,0.5)
Client->>Gateway: Request with Virtual Key
Gateway->>Gateway: Check VK.provider_configs (deny-by-default if empty)
alt VK has MCP configs
Gateway->>MCP: Forward with `x-bf-mcp-include-tools` (may list tools/wildcards)
else VK has no MCP configs
Gateway->>MCP: Forward with `x-bf-mcp-include-tools` = ""
end
Gateway->>Gateway: Build allowed provider list (empty = none)
alt Weighted providers (non-nil weights) exist
Gateway->>Gateway: Perform weighted selection among non-nil weights
else No weighted providers
Gateway->>Gateway: Use non-weighted selection or block if none allowed
end
Gateway->>Provider: Forward request to chosen Provider
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/app/workspace/logs/sheets/logDetailsSheet.tsx (1)
841-975: 🛠️ Refactor suggestion | 🟠 MajorDead code:
normalizeObjectForCopyand externalcopyRequestBodyare never used.These functions appear to be orphaned. The component defines and uses its own internal
copyRequestBodyfunction (line 79) that's called at line 248. The external version with improved normalization is never invoked.Options:
- Remove dead code if the internal implementation is sufficient.
- Refactor to use the external function (recommended) since it has better normalization logic for handling dotted backend names like
chat.completionandaudio.speech.♻️ Recommended approach — use the external function
Keep the external functions and refactor the component to use them:
// Inside the component, replace the internal copyRequestBody with: + const handleCopyRequestBody = async () => { + await copyRequestBody(log); + }; - const copyRequestBody = async () => { ... entire internal function ... };Update the click handler:
-<DropdownMenuItem onClick={copyRequestBody}> +<DropdownMenuItem onClick={handleCopyRequestBody} data-testid="log-copy-request-body-btn">This eliminates ~120 lines of duplicate code and ensures the normalization logic handles all edge cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/logs/sheets/logDetailsSheet.tsx` around lines 841 - 975, The file contains an unused external normalizeObjectForCopy and copyRequestBody while the component defines a duplicate internal copyRequestBody; remove the dead external code or (recommended) refactor the component to call the external helpers: replace the component's internal copyRequestBody logic with a call to the top-level copyRequestBody and switch any normalization calls to use normalizeObjectForCopy, remove the duplicate internal helper(s), and ensure any references (click handlers that trigger copying) now invoke the external copyRequestBody; run type checks and verify clipboard/toast behavior remains unchanged.transports/config.schema.json (1)
461-467:⚠️ Potential issue | 🟠 MajorSchema still rejects nullable provider weight despite nullable runtime semantics.
While Line 463 updates deny-by-default wording, the same schema object still models
weightas non-null number. Ifnullis a valid opt-out value for weighted routing, config validation will diverge from runtime behavior.🔧 Suggested schema adjustment
"weight": { - "type": "number", - "description": "Weight for load balancing", - "default": 1.0 + "type": ["number", "null"], + "description": "Weight for load balancing (null excludes this provider from weighted routing)" },As per coding guidelines,
transports/config.schema.jsonis the authoritative source of truth for config fields and must stay aligned with handler/runtime semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/config.schema.json` around lines 461 - 467, The JSON Schema defines provider configs under "provider_configs" referencing "$defs/virtual_key_provider_config", but that referenced object currently models the "weight" property as a non-null number while runtime accepts null to opt out of weighted routing; update the "$defs/virtual_key_provider_config" definition so the "weight" schema allows null (e.g., type: ["number","null"] or an anyOf with number and null) and ensure any required/nullable flags reflect that change so schema validation matches the handler/runtime semantics for "weight".
🧹 Nitpick comments (4)
ui/app/workspace/logs/sheets/logDetailsSheet.tsx (2)
29-29: Unused imports:DollarSign,FileText,Timer.These icons are imported but never used in the file. Remove to avoid lint warnings and keep imports clean.
🧹 Proposed fix
-import { Clipboard, DollarSign, FileText, MoreVertical, Timer, Trash2 } from "lucide-react"; +import { Clipboard, MoreVertical, Trash2 } from "lucide-react";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/logs/sheets/logDetailsSheet.tsx` at line 29, The import line in logDetailsSheet.tsx brings in unused icons DollarSign, FileText, and Timer; edit the import from lucide-react used by the LogDetailsSheet to remove those unused symbols so only needed icons (e.g., Clipboard, MoreVertical, Trash2) are imported, eliminating lint warnings — update the import statement that currently references Clipboard, DollarSign, FileText, MoreVertical, Timer, Trash2 to only include the actually used icons.
248-251: Missingdata-testidattribute on interactive element.Per coding guidelines, all interactive elements in workspace pages require
data-testidattributes for E2E testing.✅ Proposed fix
-<DropdownMenuItem onClick={copyRequestBody}> +<DropdownMenuItem onClick={copyRequestBody} data-testid="log-copy-request-body-btn"> <Clipboard className="h-4 w-4" /> Copy request body </DropdownMenuItem>As per coding guidelines:
ui/app/workspace/**/*.tsx: UI workspace pages must include data-testid attributes on all interactive elements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/logs/sheets/logDetailsSheet.tsx` around lines 248 - 251, The DropdownMenuItem interactive element in logDetailsSheet (the element that calls copyRequestBody) is missing a data-testid; add a unique data-testid attribute (e.g., data-testid="log-details-copy-request-body" or similar consistent naming) to the DropdownMenuItem that triggers copyRequestBody so E2E tests can reliably select it; ensure the attribute is added to the DropdownMenuItem JSX and matches workspace test naming conventions.docs/providers/provider-routing.mdx (1)
1163-1163: Split this setup bullet into two explicit cases for clarity.Line 1163 currently combines “No Virtual Key” with “Virtual Key without
provider_configs,” but only the no-VK path matches the LB-only flow described below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/providers/provider-routing.mdx` at line 1163, Split the combined bullet that currently reads "No Virtual Key, or Virtual Key without `provider_configs` (note: empty `provider_configs` blocks all providers)" into two separate bullets: one titled "No Virtual Key" and a second titled "Virtual Key without `provider_configs` (note: empty `provider_configs` blocks all providers)"; in the "No Virtual Key" bullet explicitly state that this is the LB-only flow, and in the "Virtual Key without `provider_configs`" bullet clarify it does NOT map to the LB-only flow so readers don’t conflate the two.framework/configstore/migrations.go (1)
3622-3643: Consider batching inserts for large deployments.The current per-VK counting and per-row inserts can make startup migrations slow on large datasets. Batch insert + conflict-safe writes would reduce query volume and lock duration.
Also applies to: 3653-3673
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 3622 - 3643, The migration loops over allVKs and creates TableVirtualKeyProviderConfig rows one-by-one (using tx.Create) after counting per VK, which is slow for large datasets; change to batch upserts: for example collect providerConfig structs for each VK that has zero provider configs (identify via a single query that returns VK IDs with existing provider configs or use a left join to find VKs lacking configs) and then perform a single bulk insert/upsert for TableVirtualKeyProviderConfig with the tx (using the ORM's bulk insert and conflict-resolution/upsert option) instead of per-row tx.Create; keep references to TableVirtualKeyProviderConfig, tx, allVKs, allProviders, vk.ID and provider.Name to locate the code and ensure the bulk operation is conflict-safe (do nothing or update as appropriate) and logs the total backfilled count.
🤖 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 3616-3621: The migration currently proceeds and marks itself
complete even when source tables are empty; modify the backfill in migrations.go
to fail fast when expected source data is missing by checking the result slice
lengths after tx.Find (e.g., the allProviders slice retrieved in the block using
tx.Find(&allProviders)) and returning a non-nil error if no rows were found (and
similarly for the other fetches referenced in the comment such as the MCP/source
table fetches), so the migration aborts instead of silently no-op and can be
re-run when data exists.
In `@transports/bifrost-http/handlers/mcpserver.go`:
- Around line 342-345: Create a single package-level typed context key (e.g.,
mcpIncludeToolsKey := schemas.BifrostContextKey("mcp-include-tools")) and
replace the inline usages of schemas.BifrostContextKey("mcp-include-tools") with
that variable; update both places that set/get the context (the ctx =
context.WithValue(...) site where executeOnlyTools is stored and the earlier
location around line 227) so they use mcpIncludeToolsKey, keeping the key
unexported and typed to schemas.BifrostContextKey to follow the context-key
guideline.
In `@ui/app/workspace/logs/sheets/logDetailsSheet.tsx`:
- Around line 79-88: The copyRequestBody function uses a naive normalization
(const object = log.object?.toLowerCase() || "") which misses dotted/back-end
variants; replace that logic by calling the existing normalizeObjectForCopy
helper (or delegate normalization to it) inside copyRequestBody, remove the
local toLowerCase handling, then base the
isChat/isResponses/isSpeech/isTextCompletion/isEmbedding/isTranscription checks
on the normalized value returned by normalizeObjectForCopy (ensure
normalizeObjectForCopy is imported/accessible in this module and used wherever
the component previously relied on the simple normalization).
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Line 595: Update the misleading placeholder text in the input within
virtualKeySheet.tsx: replace the existing "Empty = not in routing" placeholder
with wording that accurately reflects runtime behavior (for example, "Empty =
excluded from weighted routing" or similar) so it conveys exclusion from
weighted routing rather than global routing blockage; locate the placeholder
attribute in the VirtualKeySheet component's JSX and update the string
accordingly.
- Around line 311-313: The current normalization for the weight property in
virtualKeySheet.tsx uses "parseFloat(config.weight) || null", which treats 0 as
falsy and converts valid zero weights to null; replace that expression so zeros
are preserved by using parseFloat and checking Number.isNaN instead (e.g.,
compute parsed = parseFloat(config.weight) and set weight to parsed if
!Number.isNaN(parsed) otherwise null), keeping the existing branches for
non-string values (i.e., leave the ternary for typeof config.weight === "string"
and the fallback to config.weight).
- Line 257: The default provider config in VirtualKeySheet sets weight: null
which violates the form schema that disallows null; update the provider config
initialization (where the provider object / initial form values are created in
the VirtualKeySheet component) to either omit the weight property or set it to
undefined (not null) so new/unchanged configs validate; ensure any TypeScript
type for that config accepts optional weight (e.g., make weight?: number) or
handle undefined in the form logic.
---
Outside diff comments:
In `@transports/config.schema.json`:
- Around line 461-467: The JSON Schema defines provider configs under
"provider_configs" referencing "$defs/virtual_key_provider_config", but that
referenced object currently models the "weight" property as a non-null number
while runtime accepts null to opt out of weighted routing; update the
"$defs/virtual_key_provider_config" definition so the "weight" schema allows
null (e.g., type: ["number","null"] or an anyOf with number and null) and ensure
any required/nullable flags reflect that change so schema validation matches the
handler/runtime semantics for "weight".
In `@ui/app/workspace/logs/sheets/logDetailsSheet.tsx`:
- Around line 841-975: The file contains an unused external
normalizeObjectForCopy and copyRequestBody while the component defines a
duplicate internal copyRequestBody; remove the dead external code or
(recommended) refactor the component to call the external helpers: replace the
component's internal copyRequestBody logic with a call to the top-level
copyRequestBody and switch any normalization calls to use
normalizeObjectForCopy, remove the duplicate internal helper(s), and ensure any
references (click handlers that trigger copying) now invoke the external
copyRequestBody; run type checks and verify clipboard/toast behavior remains
unchanged.
---
Nitpick comments:
In `@docs/providers/provider-routing.mdx`:
- Line 1163: Split the combined bullet that currently reads "No Virtual Key, or
Virtual Key without `provider_configs` (note: empty `provider_configs` blocks
all providers)" into two separate bullets: one titled "No Virtual Key" and a
second titled "Virtual Key without `provider_configs` (note: empty
`provider_configs` blocks all providers)"; in the "No Virtual Key" bullet
explicitly state that this is the LB-only flow, and in the "Virtual Key without
`provider_configs`" bullet clarify it does NOT map to the LB-only flow so
readers don’t conflate the two.
In `@framework/configstore/migrations.go`:
- Around line 3622-3643: The migration loops over allVKs and creates
TableVirtualKeyProviderConfig rows one-by-one (using tx.Create) after counting
per VK, which is slow for large datasets; change to batch upserts: for example
collect providerConfig structs for each VK that has zero provider configs
(identify via a single query that returns VK IDs with existing provider configs
or use a left join to find VKs lacking configs) and then perform a single bulk
insert/upsert for TableVirtualKeyProviderConfig with the tx (using the ORM's
bulk insert and conflict-resolution/upsert option) instead of per-row tx.Create;
keep references to TableVirtualKeyProviderConfig, tx, allVKs, allProviders,
vk.ID and provider.Name to locate the code and ensure the bulk operation is
conflict-safe (do nothing or update as appropriate) and logs the total
backfilled count.
In `@ui/app/workspace/logs/sheets/logDetailsSheet.tsx`:
- Line 29: The import line in logDetailsSheet.tsx brings in unused icons
DollarSign, FileText, and Timer; edit the import from lucide-react used by the
LogDetailsSheet to remove those unused symbols so only needed icons (e.g.,
Clipboard, MoreVertical, Trash2) are imported, eliminating lint warnings —
update the import statement that currently references Clipboard, DollarSign,
FileText, MoreVertical, Timer, Trash2 to only include the actually used icons.
- Around line 248-251: The DropdownMenuItem interactive element in
logDetailsSheet (the element that calls copyRequestBody) is missing a
data-testid; add a unique data-testid attribute (e.g.,
data-testid="log-details-copy-request-body" or similar consistent naming) to the
DropdownMenuItem that triggers copyRequestBody so E2E tests can reliably select
it; ensure the attribute is added to the DropdownMenuItem JSX and matches
workspace test naming conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4b05eea-e29f-4284-bc11-ace4b26e95f4
📒 Files selected for processing (18)
docs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/governance.ts
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
9ee898b to
8789ae3
Compare
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (2)
597-622:⚠️ Potential issue | 🟠 MajorAdd
data-testidto the updated weight input.This interactive input was modified but still has no test selector, which makes E2E coverage brittle for provider weight behavior.
🔧 Proposed fix
<Input + data-testid={`vk-provider-weight-input-${index}`} placeholder="Exclude from routing" className="h-10 w-full"As per coding guidelines:
ui/app/workspace/**/*.tsxmust includedata-testidon all interactive elements, andui/app/**/*.{tsx,ts}requires the<entity>-<element>-<qualifier>convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 597 - 622, The weight Input inside the provider config block is missing a test selector; add a data-testid prop to the Input component (near the placeholder/className/value/onChange/onBlur props) using the project convention <entity>-<element>-<qualifier>, e.g. "provider-weight-input-<index>" (or "provider-weight-input-<providerId>" if a stable provider id is available) so tests can reliably target the element; leave all existing handlers (handleUpdateProviderConfig, value: config.weight, onChange, onBlur) unchanged.
928-969:⚠️ Potential issue | 🟠 MajorAdd
data-testidfor MCP tools selector interaction.The MCP tools selector behavior changed materially (“Allow All Tools” wildcard flow), but the interactive control has no dedicated selector for E2E assertions.
🔧 Proposed fix
<MultiSelect + data-testid={`vk-mcp-tools-select-${index}`} options={[As per coding guidelines:
ui/app/workspace/**/*.tsxmust includedata-testidon all interactive elements, andui/app/**/*.{tsx,ts}requires the<entity>-<element>-<qualifier>convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 928 - 969, Add a data-testid prop to the MultiSelect used for MCP tools so E2E can target it: in virtualKeySheet.tsx on the MultiSelect instance (symbol: MultiSelect) add data-testid="virtual-key-sheet-mcp-tools-selector" (following the <entity>-<element>-<qualifier> convention). Ensure the prop is passed alongside existing props (defaultValue, onValueChange, placeholder, etc.) and use that exact test id for assertions.
♻️ Duplicate comments (1)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
326-330:⚠️ Potential issue | 🟠 MajorPreserve explicit
0rate limits during normalization.Line 326 and Line 329 use
parseInt(...) || undefined, which converts valid"0"intoundefinedand silently drops user intent.🔧 Proposed fix
- token_max_limit: config.rate_limit.token_max_limit ? parseInt(config.rate_limit.token_max_limit) || undefined : undefined, + token_max_limit: config.rate_limit.token_max_limit + ? (() => { + const parsed = Number.parseInt(config.rate_limit.token_max_limit, 10); + return Number.isNaN(parsed) ? undefined : parsed; + })() + : undefined, token_reset_duration: config.rate_limit.token_reset_duration || "1h", request_max_limit: config.rate_limit.request_max_limit - ? parseInt(config.rate_limit.request_max_limit) || undefined + ? (() => { + const parsed = Number.parseInt(config.rate_limit.request_max_limit, 10); + return Number.isNaN(parsed) ? undefined : parsed; + })() : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 326 - 330, The normalization currently uses parseInt(... ) || undefined for token_max_limit and request_max_limit in the virtualKeySheet component, which turns a valid "0" into undefined; update the logic in the token_max_limit and request_max_limit assignment (using config.rate_limit) to parse the value once (e.g., const v = parseInt(...)) and set the field to v if !isNaN(v) (so 0 is preserved) otherwise undefined, ensuring you keep existing token_reset_duration handling unchanged.
🧹 Nitpick comments (1)
plugins/governance/main.go (1)
792-825: Extract MCP tool-filter building into a shared helper.This logic now exists in both
plugins/governance/main.goandtransports/bifrost-http/handlers/mcpserver.go. A shared helper will reduce drift risk as deny-by-default semantics evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 792 - 825, Extract the MCP tool-filter construction into a shared helper (e.g., BuildMCPIncludeToolsHeader) that accepts the virtual key's MCPConfigs (or the virtualKey object) and returns the string value for the "x-bf-mcp-include-tools" header; the helper must implement the same deny-by-default behavior (return empty string when len(MCPConfigs)==0), skip vkMcpConfig entries with no ToolsToExecute, map a "*" entry to fmt.Sprintf("%s-*", vkMcpConfig.MCPClient.Name), append non-empty tools as fmt.Sprintf("%s-%s", vkMcpConfig.MCPClient.Name, tool), and join results with commas; replace the duplicated blocks in plugins/governance/main.go (the executeOnlyTools/header logic) and transports/bifrost-http/handlers/mcpserver.go to call this new helper and set headers["x-bf-mcp-include-tools"] to the helper's return value.
🤖 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/governance/mcp-tools.mdx`:
- Around line 18-19: The doc contains contradictory statements: one says "no MCP
tools are available (deny-by-default)" and elsewhere states "leaving tools blank
allows all tools for a client"; make them consistent by changing the latter to
assert deny-by-default (e.g., replace the phrase "leaving tools blank allows all
tools for a client" with "leaving tools blank denies all tools for a client" and
add a short clarifier that MCP client configurations must be explicitly provided
to permit specific tools), and update any related example or sentence that
implies implicit allow (search for occurrences of "leave tools blank" or "blank
allows" and update to match the deny-by-default behavior).
In `@framework/configstore/migrations.go`:
- Around line 3632-3636: The provider backfill is leaving Weight nil which
excludes providers from weighted routing; in the providerConfig construction
(tables.TableVirtualKeyProviderConfig with VirtualKeyID vk.ID and Provider
provider.Name) set Weight to a non-nil pointer (e.g. default weight 1) using the
project helper bifrost.Ptr(...) instead of leaving it nil so backfilled
providers participate in weighted routing.
In `@plugins/governance/main.go`:
- Around line 563-583: The weighted-selection block using
weightedConfigs/getWeight currently proceeds even when totalWeight <= 0 which
can yield unexpected picks; before generating randomValue, check totalWeight and
if it's <= 0, skip the weighted draw and set selectedProvider to a safe fallback
(e.g., schemas.ModelProvider(weightedConfigs[0].Provider) or run the unweighted
selection path) and return/continue accordingly; ensure you still handle the
FP-guard case after the loop and reference weightedConfigs, getWeight,
selectedProvider and rand.Float64 when making the change.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 157-158: Remove the debug console.log that prints providersData
from virtualKeySheet.tsx: locate the statement console.log(providersData); and
delete it (or replace with a non-sensitive debug toggle or sanitized logging if
you need runtime visibility). Ensure no other direct logging of the full
providersData payload remains in functions or components within the same file
(e.g., any usage near the VirtualKeySheet component or its render logic).
---
Outside diff comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 597-622: The weight Input inside the provider config block is
missing a test selector; add a data-testid prop to the Input component (near the
placeholder/className/value/onChange/onBlur props) using the project convention
<entity>-<element>-<qualifier>, e.g. "provider-weight-input-<index>" (or
"provider-weight-input-<providerId>" if a stable provider id is available) so
tests can reliably target the element; leave all existing handlers
(handleUpdateProviderConfig, value: config.weight, onChange, onBlur) unchanged.
- Around line 928-969: Add a data-testid prop to the MultiSelect used for MCP
tools so E2E can target it: in virtualKeySheet.tsx on the MultiSelect instance
(symbol: MultiSelect) add data-testid="virtual-key-sheet-mcp-tools-selector"
(following the <entity>-<element>-<qualifier> convention). Ensure the prop is
passed alongside existing props (defaultValue, onValueChange, placeholder, etc.)
and use that exact test id for assertions.
---
Duplicate comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 326-330: The normalization currently uses parseInt(... ) ||
undefined for token_max_limit and request_max_limit in the virtualKeySheet
component, which turns a valid "0" into undefined; update the logic in the
token_max_limit and request_max_limit assignment (using config.rate_limit) to
parse the value once (e.g., const v = parseInt(...)) and set the field to v if
!isNaN(v) (so 0 is preserved) otherwise undefined, ensuring you keep existing
token_reset_duration handling unchanged.
---
Nitpick comments:
In `@plugins/governance/main.go`:
- Around line 792-825: Extract the MCP tool-filter construction into a shared
helper (e.g., BuildMCPIncludeToolsHeader) that accepts the virtual key's
MCPConfigs (or the virtualKey object) and returns the string value for the
"x-bf-mcp-include-tools" header; the helper must implement the same
deny-by-default behavior (return empty string when len(MCPConfigs)==0), skip
vkMcpConfig entries with no ToolsToExecute, map a "*" entry to
fmt.Sprintf("%s-*", vkMcpConfig.MCPClient.Name), append non-empty tools as
fmt.Sprintf("%s-%s", vkMcpConfig.MCPClient.Name, tool), and join results with
commas; replace the duplicated blocks in plugins/governance/main.go (the
executeOnlyTools/header logic) and transports/bifrost-http/handlers/mcpserver.go
to call this new helper and set headers["x-bf-mcp-include-tools"] to the
helper's return value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d9e6176-c4a0-46e3-aa04-9853a85375fa
📒 Files selected for processing (27)
core/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/mcp.gocore/mcp/utils.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/numberAndSelect.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
✅ Files skipped from review due to trivial changes (1)
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- docs/features/governance/routing.mdx
- ui/lib/types/governance.ts
- framework/configstore/tables/virtualkey.go
- docs/providers/provider-routing.mdx
- plugins/governance/resolver_test.go
- plugins/governance/resolver.go
- docs/openapi/schemas/management/governance.yaml
- transports/config.schema.json
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
2 times, most recently
from
ea84c03 to
2d9d2e9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
transports/config.schema.json (1)
461-470:⚠️ Potential issue | 🟠 MajorSchema contract is only partially updated for the new VK semantics.
provider_configsdescription was updated, but the same schema still omits deny-by-default language formcp_configsand keepsvirtual_key_provider_config.weightnon-null with default1.0, which conflicts with the nullable weight behavior implemented in Go/UI.🔧 Suggested schema alignment patch
"mcp_configs": { "type": "array", - "description": "MCP configurations for this virtual key", + "description": "MCP configurations for this virtual key (empty means no MCP tools allowed, deny-by-default)", "items": { "$ref": "#/$defs/virtual_key_mcp_config" } }"weight": { - "type": "number", - "description": "Weight for load balancing", - "default": 1.0 + "type": ["number", "null"], + "description": "Weight for load balancing. null (or omitted) excludes this provider from weighted routing." },As per coding guidelines, `transports/config.schema.json (~2700 lines) is the authoritative source of truth for all config.json fields. Documentation examples must match the schema. Update schema first before updating handlers or docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/config.schema.json` around lines 461 - 470, Update the JSON schema to match the new virtual key semantics: change the "mcp_configs" description to include the "deny-by-default (empty means no MCPs allowed)" language analogous to "provider_configs", and modify the "$defs/virtual_key_provider_config" entry so that the "weight" property is nullable (type ["number","null"]) and its default is null (remove default 1.0) to match Go/UI behavior; ensure the symbol names referenced are "mcp_configs", "provider_configs", and "virtual_key_provider_config.weight" so the authoritative transports/config.schema.json reflects the implemented semantics before changing handlers or docs.ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
595-620:⚠️ Potential issue | 🟠 MajorAdd
data-testidto changed interactive controls in workspace page.The modified Weight input and MCP tools multiselect are interactive controls but currently have no
data-testid. Please add IDs following<entity>-<element>-<qualifier>(for example,vk-provider-weight-inputandvk-mcp-tools-select).As per coding guidelines, "UI workspace pages are located in ui/app/workspace// and must include data-testid attributes on all interactive elements. Check existing data-testid values before modifying UI elements." and "
ui/app/**/*.{tsx,ts}: E2E test data-testid attributes must follow the convention 'data-testid="--"'."Also applies to: 926-967
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 595 - 620, Add E2E data-testid attributes to the interactive controls: add data-testid="vk-provider-weight-input" to the Input component used for provider weight (the Input with value={config.weight ?? ""} and onChange/onBlur handlers that call handleUpdateProviderConfig) and add data-testid="vk-mcp-tools-select" to the MCP tools multiselect component referenced in the later block (the multiselect in the 926-967 change). Ensure the attributes follow the <entity>-<element>-<qualifier> format and are placed directly on the rendered interactive elements so tests can target them.
♻️ Duplicate comments (1)
plugins/governance/main.go (1)
553-583:⚠️ Potential issue | 🟠 MajorGuard non-positive total weight before weighted draw.
If all configured weights are
<= 0, the draw at Line 570 can still select a provider (especially with<=at Line 575), which breaks intended weighted behavior.Suggested hardening
- if len(weightedConfigs) > 0 { + if len(weightedConfigs) > 0 { // Weighted random selection from providers that have weight set - totalWeight := 0.0 - for _, config := range weightedConfigs { - totalWeight += getWeight(config.Weight) - } - // Generate random number between 0 and totalWeight - randomValue := rand.Float64() * totalWeight - // Select provider based on weighted random selection - currentWeight := 0.0 - for _, config := range weightedConfigs { - currentWeight += getWeight(config.Weight) - if randomValue <= currentWeight { - selectedProvider = schemas.ModelProvider(config.Provider) - break - } - } - // Fallback: if no provider was selected (shouldn't happen but guard against FP issues) - if selectedProvider == "" { - selectedProvider = schemas.ModelProvider(weightedConfigs[0].Provider) - } + totalWeight := 0.0 + normalized := make([]configstoreTables.TableVirtualKeyProviderConfig, 0, len(weightedConfigs)) + for _, config := range weightedConfigs { + w := getWeight(config.Weight) + if w <= 0 { + continue + } + totalWeight += w + normalized = append(normalized, config) + } + if totalWeight <= 0 || len(normalized) == 0 { + selectedProvider = schemas.ModelProvider(allowedProviderConfigs[0].Provider) + } else { + randomValue := rand.Float64() * totalWeight + currentWeight := 0.0 + for _, config := range normalized { + currentWeight += getWeight(config.Weight) + if randomValue < currentWeight { + selectedProvider = schemas.ModelProvider(config.Provider) + break + } + } + } } else { // No providers have weight set - select first allowed provider without load balancing selectedProvider = schemas.ModelProvider(allowedProviderConfigs[0].Provider) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 553 - 583, Guard against non-positive total weight before doing the weighted draw: after computing totalWeight (from weightedConfigs using getWeight), check if totalWeight <= 0 and in that case skip the weighted random selection and set selectedProvider to a deterministic fallback (e.g., schemas.ModelProvider(weightedConfigs[0].Provider) or handle as no-selection) instead of proceeding to rand.Float64; ensure this check is placed before generating randomValue and before iterating weightedConfigs so getWeight/isWeightSet logic cannot produce a draw when all weights are non-positive.
🧹 Nitpick comments (1)
framework/configstore/migrations.go (1)
3622-3673: Consider set-based backfill to avoid migration-time N+1 overhead.Current logic does per-VK
COUNTchecks and per-rowCreate, which can make startup migrations slow on large datasets. A set-based approach (query VK IDs missing configs once, then bulk insert with conflict-ignore) will be more resilient operationally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 3622 - 3673, The migration currently does per-VK COUNTs and per-row tx.Create which causes N+1 overhead; instead query once for VK IDs that lack provider configs and once for VK IDs that lack MCP configs (use tx.Select/Where with a subquery against tables.TableVirtualKeyProviderConfig and tables.TableVirtualKeyMCPConfig to get missing virtual_key_id values from allVKs), then build slices of tables.TableVirtualKeyProviderConfig and tables.TableVirtualKeyMCPConfig for those VK IDs using allProviders and allMCPClients respectively and perform a bulk insert via tx.Create(&slice) (or tx.Clauses(clause.OnConflict{DoNothing: true}).Create(&slice)) to insert many rows at once while ignoring conflicts; reference symbols: allVKs, allProviders, allMCPClients, tx, tables.TableVirtualKeyProviderConfig, tables.TableVirtualKeyMCPConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/logs/sheets/logDetailsSheet.tsx`:
- Line 126: The DropdownMenuItem invoking copyRequestBody(log) in the
LogDetailsSheet component needs a stable data-testid added (e.g.,
data-testid="log-copy-request-body" or similar) so E2E tests can target it;
update the DropdownMenuItem JSX that calls onClick={() => copyRequestBody(log)}
to include the data-testid attribute, and then search tests/e2e/ for any
references to the old test id (rename/update or re-add usages) to keep tests
consistent; ensure the chosen id follows the project's naming convention and is
unique within ui/app/workspace.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 56-57: The weight field currently allows arbitrary strings and is
coerced later with parseFloat (in the normalization around the code that
converts weight via parseFloat at lines ~312–315), which lets out-of-range
values bypass validation; change the zod schema for weight (the schema
referenced as weight: z.union([...]) ) to preprocess strings into numbers (e.g.,
z.preprocess to parseFloat) and then validate with z.number().min(0).max(1) so
all inputs are checked before normalization. Also add data-testid attributes to
the new interactive elements: the weight input (e.g.,
data-testid="vk-weight-input"), the model multiselect (e.g.,
data-testid="vk-models-multiselect"), each delete button (e.g.,
data-testid="vk-delete-button-<id>"), and the MCP tools selector (e.g.,
data-testid="vk-mcp-tools-selector") to follow the vk-<element>-<qualifier>
convention so tests can reliably target these controls.
---
Outside diff comments:
In `@transports/config.schema.json`:
- Around line 461-470: Update the JSON schema to match the new virtual key
semantics: change the "mcp_configs" description to include the "deny-by-default
(empty means no MCPs allowed)" language analogous to "provider_configs", and
modify the "$defs/virtual_key_provider_config" entry so that the "weight"
property is nullable (type ["number","null"]) and its default is null (remove
default 1.0) to match Go/UI behavior; ensure the symbol names referenced are
"mcp_configs", "provider_configs", and "virtual_key_provider_config.weight" so
the authoritative transports/config.schema.json reflects the implemented
semantics before changing handlers or docs.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 595-620: Add E2E data-testid attributes to the interactive
controls: add data-testid="vk-provider-weight-input" to the Input component used
for provider weight (the Input with value={config.weight ?? ""} and
onChange/onBlur handlers that call handleUpdateProviderConfig) and add
data-testid="vk-mcp-tools-select" to the MCP tools multiselect component
referenced in the later block (the multiselect in the 926-967 change). Ensure
the attributes follow the <entity>-<element>-<qualifier> format and are placed
directly on the rendered interactive elements so tests can target them.
---
Duplicate comments:
In `@plugins/governance/main.go`:
- Around line 553-583: Guard against non-positive total weight before doing the
weighted draw: after computing totalWeight (from weightedConfigs using
getWeight), check if totalWeight <= 0 and in that case skip the weighted random
selection and set selectedProvider to a deterministic fallback (e.g.,
schemas.ModelProvider(weightedConfigs[0].Provider) or handle as no-selection)
instead of proceeding to rand.Float64; ensure this check is placed before
generating randomValue and before iterating weightedConfigs so
getWeight/isWeightSet logic cannot produce a draw when all weights are
non-positive.
---
Nitpick comments:
In `@framework/configstore/migrations.go`:
- Around line 3622-3673: The migration currently does per-VK COUNTs and per-row
tx.Create which causes N+1 overhead; instead query once for VK IDs that lack
provider configs and once for VK IDs that lack MCP configs (use tx.Select/Where
with a subquery against tables.TableVirtualKeyProviderConfig and
tables.TableVirtualKeyMCPConfig to get missing virtual_key_id values from
allVKs), then build slices of tables.TableVirtualKeyProviderConfig and
tables.TableVirtualKeyMCPConfig for those VK IDs using allProviders and
allMCPClients respectively and perform a bulk insert via tx.Create(&slice) (or
tx.Clauses(clause.OnConflict{DoNothing: true}).Create(&slice)) to insert many
rows at once while ignoring conflicts; reference symbols: allVKs, allProviders,
allMCPClients, tx, tables.TableVirtualKeyProviderConfig,
tables.TableVirtualKeyMCPConfig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 824afbd0-423b-4170-90a0-6b74f9bc9bca
📒 Files selected for processing (27)
core/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/mcp.gocore/mcp/utils.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/numberAndSelect.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
✅ Files skipped from review due to trivial changes (2)
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- docs/mcp/filtering.mdx
🚧 Files skipped from review as they are similar to previous changes (9)
- core/internal/mcptests/fixtures.go
- framework/configstore/clientconfig.go
- plugins/governance/resolver_test.go
- transports/bifrost-http/handlers/mcpserver.go
- docs/providers/provider-routing.mdx
- core/internal/mcptests/codemode_stdio_test.go
- plugins/governance/resolver.go
- core/internal/mcptests/agent_test_helpers.go
- docs/features/governance/mcp-tools.mdx
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
2d9d2e9 to
c0dfed9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugins/governance/main.go (1)
563-583:⚠️ Potential issue | 🟠 MajorGuard non-positive aggregate weight before random selection.
When all set weights are
0(or sum to non-positive), the current path can still select a provider (e.g.,0 <= 0at Line 575). This violates expected weighted behavior.🔧 Suggested hardening
if len(weightedConfigs) > 0 { - // Weighted random selection from providers that have weight set - totalWeight := 0.0 - for _, config := range weightedConfigs { - totalWeight += getWeight(config.Weight) - } - // Generate random number between 0 and totalWeight - randomValue := rand.Float64() * totalWeight - // Select provider based on weighted random selection - currentWeight := 0.0 - for _, config := range weightedConfigs { - currentWeight += getWeight(config.Weight) - if randomValue <= currentWeight { - selectedProvider = schemas.ModelProvider(config.Provider) - break - } - } - // Fallback: if no provider was selected (shouldn't happen but guard against FP issues) - if selectedProvider == "" { - selectedProvider = schemas.ModelProvider(weightedConfigs[0].Provider) - } + totalWeight := 0.0 + normalized := make([]configstoreTables.TableVirtualKeyProviderConfig, 0, len(weightedConfigs)) + for _, config := range weightedConfigs { + w := getWeight(config.Weight) + if w <= 0 { + continue + } + totalWeight += w + normalized = append(normalized, config) + } + + if totalWeight <= 0 || len(normalized) == 0 { + selectedProvider = schemas.ModelProvider(allowedProviderConfigs[0].Provider) + } else { + randomValue := rand.Float64() * totalWeight + currentWeight := 0.0 + for _, config := range normalized { + currentWeight += getWeight(config.Weight) + if randomValue < currentWeight { + selectedProvider = schemas.ModelProvider(config.Provider) + break + } + } + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 563 - 583, The weighted selection logic using weightedConfigs and getWeight can proceed when totalWeight is 0 (or negative) leading to incorrect selection; before generating randomValue with rand.Float64(), guard against non-positive totalWeight by detecting totalWeight <= 0 and handling it as a fallback (e.g., choose a deterministic fallback like schemas.ModelProvider(weightedConfigs[0].Provider) or perform uniform random selection across weightedConfigs) and only run the weighted random loop when totalWeight > 0, ensuring selectedProvider is set correctly in both branches.ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
606-606:⚠️ Potential issue | 🟡 MinorMake the weight placeholder semantically precise.
“Exclude from routing” implies full blocking, but empty weight currently means excluded from weighted routing.
✏️ Suggested wording
- placeholder="Exclude from routing" + placeholder="Exclude from weighted routing"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` at line 606, The placeholder text "Exclude from routing" is misleading for the weight input in VirtualKeySheet; update the placeholder on the weight input (the element with placeholder currently set in virtualKeySheet.tsx, related to the weight field in the VirtualKeySheet component) to a semantically precise message such as "Leave empty to exclude from weighted routing" or "No weight — excluded from weighted routing" so it accurately indicates that an empty weight excludes the item from weighted routing rather than fully blocking it.
🧹 Nitpick comments (1)
framework/configstore/clientconfig.go (1)
562-562: Make VK provider-config sort nil-aware now that hash preserves pointer semantics.Line 655 now hashes
Weightas*float64, but ordering still usesgetWeight(...)(Line 637), which treatsniland1.0as equal. This can produce unstable hash ordering when both states exist.♻️ Suggested fix
sort.Slice(sortedProviderConfigs, func(i, j int) bool { if sortedProviderConfigs[i].Provider != sortedProviderConfigs[j].Provider { return sortedProviderConfigs[i].Provider < sortedProviderConfigs[j].Provider } @@ - return getWeight(sortedProviderConfigs[i].Weight) < getWeight(sortedProviderConfigs[j].Weight) + wiSet := sortedProviderConfigs[i].Weight != nil + wjSet := sortedProviderConfigs[j].Weight != nil + if wiSet != wjSet { + // keep explicit weight ordered before implicit default + return wiSet + } + return getWeight(sortedProviderConfigs[i].Weight) < getWeight(sortedProviderConfigs[j].Weight) })Also applies to: 655-655
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/clientconfig.go` at line 562, The sort used for VK provider-config must treat Weight pointer semantics consistently with the hashing change: modify the ordering logic that currently calls getWeight(...) (which normalizes nil to 1.0) to compare the Weight pointer values directly (preserving nil vs non-nil and comparing *Weight when both non-nil) so ordering matches the hash behavior that now hashes Weight as *float64; locate the comparator/sort implementation that references getWeight and update it to handle nil-aware pointer comparisons for the Weight field used by the VK provider-config.
🤖 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/providers/provider-routing.mdx`:
- Line 1163: Split the mixed statement "No Virtual Key, or Virtual Key without
`provider_configs` (note: empty `provider_configs` blocks all providers)" into
two distinct cases: state that "No Virtual Key" is the Load Balancing–only
setup, and separately state that a "Virtual Key with empty `provider_configs`"
blocks all providers and therefore is NOT an LB-only setup; update the wording
to clearly separate these two outcomes so readers don't conflate them.
---
Duplicate comments:
In `@plugins/governance/main.go`:
- Around line 563-583: The weighted selection logic using weightedConfigs and
getWeight can proceed when totalWeight is 0 (or negative) leading to incorrect
selection; before generating randomValue with rand.Float64(), guard against
non-positive totalWeight by detecting totalWeight <= 0 and handling it as a
fallback (e.g., choose a deterministic fallback like
schemas.ModelProvider(weightedConfigs[0].Provider) or perform uniform random
selection across weightedConfigs) and only run the weighted random loop when
totalWeight > 0, ensuring selectedProvider is set correctly in both branches.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Line 606: The placeholder text "Exclude from routing" is misleading for the
weight input in VirtualKeySheet; update the placeholder on the weight input (the
element with placeholder currently set in virtualKeySheet.tsx, related to the
weight field in the VirtualKeySheet component) to a semantically precise message
such as "Leave empty to exclude from weighted routing" or "No weight — excluded
from weighted routing" so it accurately indicates that an empty weight excludes
the item from weighted routing rather than fully blocking it.
---
Nitpick comments:
In `@framework/configstore/clientconfig.go`:
- Line 562: The sort used for VK provider-config must treat Weight pointer
semantics consistently with the hashing change: modify the ordering logic that
currently calls getWeight(...) (which normalizes nil to 1.0) to compare the
Weight pointer values directly (preserving nil vs non-nil and comparing *Weight
when both non-nil) so ordering matches the hash behavior that now hashes Weight
as *float64; locate the comparator/sort implementation that references getWeight
and update it to handle nil-aware pointer comparisons for the Weight field used
by the VK provider-config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69435b52-801e-44dd-931a-3d45c8faf747
📒 Files selected for processing (27)
core/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/mcp.gocore/mcp/utils.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/numberAndSelect.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
🚧 Files skipped from review as they are similar to previous changes (13)
- core/internal/mcptests/fixtures.go
- core/internal/mcptests/concurrency_advanced_test.go
- ui/components/ui/numberAndSelect.tsx
- docs/features/governance/routing.mdx
- plugins/governance/resolver.go
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- docs/features/governance/mcp-tools.mdx
- transports/config.schema.json
- ui/lib/types/governance.ts
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- core/mcp/utils.go
- transports/bifrost-http/handlers/governance.go
- docs/mcp/filtering.mdx
|
|
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
c0dfed9 to
5fbfacf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/governance/resolver.go (1)
325-344:⚠️ Potential issue | 🟠 MajorFail closed when
allowed_modelsdepends on a missing model catalog.This function still turns
allowed_models: []into allow-all whenr.modelCatalogisnil. That means a missing/uninitialized catalog silently reopens every model for the provider, which undercuts the stricter routing semantics this PR is introducing.🩹 Suggested fix
- // Fallback when model catalog is not available: simple string matching - if len(pc.AllowedModels) == 0 { - return true - } - return slices.Contains(pc.AllowedModels, model) + // Fail closed when catalog-backed validation is unavailable. + if len(pc.AllowedModels) == 0 { + return false + } + return slices.Contains(pc.AllowedModels, model)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/resolver.go` around lines 325 - 344, The isModelAllowed method in BudgetResolver currently treats pc.AllowedModels == [] as allow-all when r.modelCatalog is nil, which reopens models if the catalog is missing; change the fallback behavior inside isModelAllowed so that when r.modelCatalog == nil an empty pc.AllowedModels denies (return false) instead of returning true: keep using slices.Contains(pc.AllowedModels, model) when AllowedModels is non-empty, but if AllowedModels is empty and modelCatalog is nil, return false. Refer to BudgetResolver.isModelAllowed, r.modelCatalog, pc.AllowedModels, modelCatalog.IsModelAllowedForProvider and slices.Contains to locate and update the logic.ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
584-600:⚠️ Potential issue | 🟠 MajorMove the provider delete action out of
AccordionTrigger.The clickable
Trash2is nested inside the accordion trigger button, so it is not a standalone keyboard-accessible control and its click will also toggle the accordion while deleting the row.♿ Suggested structure
- <AccordionTrigger className="flex h-12 items-center gap-0 px-1"> - <div className="flex w-full items-center justify-between"> + <div className="flex items-center gap-2"> + <AccordionTrigger className="flex h-12 flex-1 items-center gap-0 px-1"> + <div className="flex w-full items-center"> <div className="flex w-fit items-center gap-2"> ... </div> - <div className="hover:bg-accent/50 cursor-pointer rounded-sm p-2"> - <Trash2 onClick={() => handleRemoveProvider(index)} className="h-4 w-4 opacity-75" data-testid={`vk-delete-provider-${index}`} /> - </div> </div> - </AccordionTrigger> + </AccordionTrigger> + <Button + type="button" + variant="ghost" + size="icon" + aria-label={`Remove ${config.provider} provider`} + data-testid={`vk-delete-provider-${index}`} + onClick={() => handleRemoveProvider(index)} + > + <Trash2 className="h-4 w-4 opacity-75" /> + </Button> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 584 - 600, The Trash2 delete icon is inside AccordionTrigger which makes it toggle the accordion and not keyboard-accessible as a standalone control; move the delete control out of AccordionTrigger so it is its own focusable button element (e.g., render a <button> or IconButton adjacent to the accordion trigger), keep the onClick handler use handleRemoveProvider(index), preserve data-testid (`vk-delete-provider-${index}`) and styling, add an accessible aria-label, and ensure the button's onClick calls event.stopPropagation() to prevent the accordion from toggling when deleting.
♻️ Duplicate comments (2)
plugins/governance/main.go (1)
620-650:⚠️ Potential issue | 🟠 MajorGuard non-positive total weight before the weighted draw.
If every configured weight is
<= 0, this branch still runs the random selection and can pick a provider unexpectedly. Filter non-positive weights out before summing, and fall back when nothing usable remains.🔧 Suggested hardening
if len(weightedConfigs) > 0 { // Weighted random selection from providers that have weight set totalWeight := 0.0 - for _, config := range weightedConfigs { - totalWeight += getWeight(config.Weight) + normalized := make([]configstoreTables.TableVirtualKeyProviderConfig, 0, len(weightedConfigs)) + for _, config := range weightedConfigs { + w := getWeight(config.Weight) + if w <= 0 { + continue + } + totalWeight += w + normalized = append(normalized, config) } - // Generate random number between 0 and totalWeight - randomValue := rand.Float64() * totalWeight - // Select provider based on weighted random selection - currentWeight := 0.0 - for _, config := range weightedConfigs { - currentWeight += getWeight(config.Weight) - if randomValue <= currentWeight { - selectedProvider = schemas.ModelProvider(config.Provider) - break + + if totalWeight <= 0 || len(normalized) == 0 { + selectedProvider = schemas.ModelProvider(allowedProviderConfigs[0].Provider) + } else { + randomValue := rand.Float64() * totalWeight + currentWeight := 0.0 + for _, config := range normalized { + currentWeight += getWeight(config.Weight) + if randomValue < currentWeight { + selectedProvider = schemas.ModelProvider(config.Provider) + break + } } } // Fallback: if no provider was selected (shouldn't happen but guard against FP issues) if selectedProvider == "" { selectedProvider = schemas.ModelProvider(weightedConfigs[0].Provider)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 620 - 650, The weighted-selection block needs to guard against non-positive weights: when building weightedConfigs or before the draw, filter out entries where getWeight(config.Weight) <= 0 so they don't contribute to totalWeight; compute totalWeight from only positive weights, and if totalWeight <= 0 then skip the weighted draw and set selectedProvider to a sensible fallback (e.g., schemas.ModelProvider(weightedConfigs[0].Provider) or pick from allowedProviderConfigs) instead of running the random selection; ensure you reference and update weightedConfigs, totalWeight, getWeight, randomValue and selectedProvider accordingly.ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
940-981:⚠️ Potential issue | 🟡 MinorAdd a stable test selector to the MCP tools picker.
This changed
MultiSelectis still missing adata-testid, so the new allow-all/specific-tools flow is hard to target reliably in E2E. Add the selector on the trigger or wrapper, and thread it throughMultiSelectif needed.As per coding guidelines,
ui/app/workspace/**/*.tsx: UI workspace pages are located in ui/app/workspace// and must include data-testid attributes on all interactive elements. Check existing data-testid values before modifying UI elements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 940 - 981, Add a stable data-testid to the MCP tools picker by passing a test id prop into the MultiSelect wrapper/trigger in virtualKeySheet.tsx: add something like data-testid="mcp-tools-picker" on the element that renders the trigger/wrapper for the MultiSelect (or pass a prop through the MultiSelect component if it currently swallows extra attributes), ensuring the identifier is unique and follows existing workspace test id conventions; update the MultiSelect implementation to accept and forward this data-testid to its trigger DOM element if required so tests can reliably target the allow-all/specific-tools flow (references: MultiSelect, selectedTools, onValueChange, handleUpdateMCPConfig).
🧹 Nitpick comments (2)
framework/configstore/tables/virtualkey.go (1)
194-195: Comment update correctly reflects new deny-by-default semantics.The updated inline comment on
ProviderConfigsaccurately documents the behavior change. Consider adding a similar comment toMCPConfigsfor consistency, since the PR objectives indicate both fields now follow deny-by-default when empty.,
📝 Suggested comment addition for consistency
- ProviderConfigs []TableVirtualKeyProviderConfig `gorm:"foreignKey:VirtualKeyID;constraint:OnDelete:CASCADE" json:"provider_configs"` // Empty means no providers allowed (deny-by-default) - MCPConfigs []TableVirtualKeyMCPConfig `gorm:"foreignKey:VirtualKeyID;constraint:OnDelete:CASCADE" json:"mcp_configs"` + ProviderConfigs []TableVirtualKeyProviderConfig `gorm:"foreignKey:VirtualKeyID;constraint:OnDelete:CASCADE" json:"provider_configs"` // Empty means no providers allowed (deny-by-default) + MCPConfigs []TableVirtualKeyMCPConfig `gorm:"foreignKey:VirtualKeyID;constraint:OnDelete:CASCADE" json:"mcp_configs"` // Empty means no MCP clients allowed (deny-by-default)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/virtualkey.go` around lines 194 - 195, The comment on ProviderConfigs was updated to note deny-by-default when empty; mirror that clarification on the MCPConfigs field by updating the struct comment for MCPConfigs (TableVirtualKeyMCPConfig: MCPConfigs) to state that an empty slice means no MCPs are allowed (deny-by-default) and that the GORM constraint remains OnDelete:CASCADE; place the brief comment inline next to the MCPConfigs field to match the style used for ProviderConfigs.core/internal/mcptests/codemode_stdio_test.go (1)
59-59: Derive these client aliases instead of hardcoding them.These
config.Nameassignments are all the same kebab-case → camelCase transformation thattoCamelCasealready implements. Reusing the helper here would keep the code-mode identifiers, include-client filters, and future server additions from drifting out of sync.Also applies to: 64-64, 69-69, 74-74, 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/internal/mcptests/codemode_stdio_test.go` at line 59, Replace the hardcoded camelCase client alias assignments (e.g., config.Name = "temperature") with a derived value using the existing toCamelCase helper: set config.Name = toCamelCase(<kebabCaseIdentifier>) where <kebabCaseIdentifier> is the original kebab-case identifier you're converting in that test; do this for all similar sites that currently hardcode the camelCase string (the other occurrences noted in the comment). This ensures the client alias derives from the same toCamelCase logic used elsewhere and prevents drift between code-mode identifiers and include-client filters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/internal/mcptests/agent_test_helpers.go`:
- Around line 133-140: The helpers (SetupAgentTest and
SetupAgentTestWithClients) currently use len(config.ClientFiltering) > 0 and
len(config.ToolFiltering) > 0 which drops explicit empty slices; instead check
for nil to preserve the empty-vs-nil distinction used by AgentTestConfig and
CreateTestContextWithMCPFilter. Replace those len(...) > 0 conditions with
config.ClientFiltering != nil and config.ToolFiltering != nil where the context
values are set (keys: schemas.MCPContextKeyIncludeClients and
schemas.MCPContextKeyIncludeTools) so NewBifrostContext receives the intended
nil vs empty slice semantics; apply the same change in the other occurrence
mentioned (around the 193–201 region).
In `@docs/features/governance/routing.mdx`:
- Around line 31-32: Update the "With Provider Configs" description to state
that providers with weight: null are excluded from weighted routing;
specifically modify the second bullet (the line that starts "With Provider
Configs") to note that configured providers participate in weighted load
balancing only if their weight is set to a numeric value while providers with
weight: null remain configured but are opted out of weighted selection. Mention
"weight: null" explicitly so readers know how to keep a provider configured
without including it in weighted routing.
In `@framework/configstore/migrations.go`:
- Around line 259-261: migrationBackfillEmptyVirtualKeyConfigs currently inserts
ProviderConfigs/MCPConfigs but does not update
governance_virtual_keys.config_hash for VKs it mutates; update the migration to
recompute and persist the config_hash for every virtual key touched by the
backfill. Specifically, collect the set of virtual_key IDs modified by
migrationBackfillEmptyVirtualKeyConfigs (e.g., by returning or recording them),
then for each ID compute the canonical config payload using the same hashing
routine used elsewhere (the function used previously to generate virtual-key
hashes) and perform an UPDATE on governance_virtual_keys SET config_hash =
<new_hash> WHERE id = <virtual_key_id>; ensure the update runs in the same
migration transaction/DB connection and include both ProviderConfigs and
MCPConfigs changes so subsequent config-sync/diff logic sees the new hash.
In `@plugins/governance/main.go`:
- Around line 561-563: The current denial for an empty provider_configs returns
a plain error (fmt.Errorf) and causes callers to treat it as a transport
failure; replace that plain error return with the hook's transport
short-circuit/4xx response for deny-by-default so the hook boundary maps this to
a client-facing 4xx short-circuit. Keep the ctx.AppendRoutingEngineLog call, and
instead of returning (body, fmt.Errorf(...)), construct and return the hook
short-circuit/transport result (e.g., a 403/4xx short-circuit payload or the
existing shortCircuit/Transport* return value used by the hook pipeline) with a
clear message referencing virtualKey.Name and modelStr so callers receive a
proper deny response rather than a bare error. Ensure the function signature and
callers that expect (body, error) or a special short-circuit type are used
consistently (use the same short-circuit/4xx helper or type the hook framework
uses).
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 850-854: The tooltip text in TooltipContent misleadingly implies
adding an MCP client alone permits tools, but handleAddMCPClient initializes
tools_to_execute to [] so tools remain blocked; update the copy in
virtualKeySheet.tsx (TooltipContent) to state that after adding an MCP client
you must also select specific tools or choose "Allow All Tools" (or the
equivalent UI action) to grant tool access, and reference the behavior in
handleAddMCPClient and any UI label for "Allow All Tools" so users know the
required next step.
---
Outside diff comments:
In `@plugins/governance/resolver.go`:
- Around line 325-344: The isModelAllowed method in BudgetResolver currently
treats pc.AllowedModels == [] as allow-all when r.modelCatalog is nil, which
reopens models if the catalog is missing; change the fallback behavior inside
isModelAllowed so that when r.modelCatalog == nil an empty pc.AllowedModels
denies (return false) instead of returning true: keep using
slices.Contains(pc.AllowedModels, model) when AllowedModels is non-empty, but if
AllowedModels is empty and modelCatalog is nil, return false. Refer to
BudgetResolver.isModelAllowed, r.modelCatalog, pc.AllowedModels,
modelCatalog.IsModelAllowedForProvider and slices.Contains to locate and update
the logic.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 584-600: The Trash2 delete icon is inside AccordionTrigger which
makes it toggle the accordion and not keyboard-accessible as a standalone
control; move the delete control out of AccordionTrigger so it is its own
focusable button element (e.g., render a <button> or IconButton adjacent to the
accordion trigger), keep the onClick handler use handleRemoveProvider(index),
preserve data-testid (`vk-delete-provider-${index}`) and styling, add an
accessible aria-label, and ensure the button's onClick calls
event.stopPropagation() to prevent the accordion from toggling when deleting.
---
Duplicate comments:
In `@plugins/governance/main.go`:
- Around line 620-650: The weighted-selection block needs to guard against
non-positive weights: when building weightedConfigs or before the draw, filter
out entries where getWeight(config.Weight) <= 0 so they don't contribute to
totalWeight; compute totalWeight from only positive weights, and if totalWeight
<= 0 then skip the weighted draw and set selectedProvider to a sensible fallback
(e.g., schemas.ModelProvider(weightedConfigs[0].Provider) or pick from
allowedProviderConfigs) instead of running the random selection; ensure you
reference and update weightedConfigs, totalWeight, getWeight, randomValue and
selectedProvider accordingly.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 940-981: Add a stable data-testid to the MCP tools picker by
passing a test id prop into the MultiSelect wrapper/trigger in
virtualKeySheet.tsx: add something like data-testid="mcp-tools-picker" on the
element that renders the trigger/wrapper for the MultiSelect (or pass a prop
through the MultiSelect component if it currently swallows extra attributes),
ensuring the identifier is unique and follows existing workspace test id
conventions; update the MultiSelect implementation to accept and forward this
data-testid to its trigger DOM element if required so tests can reliably target
the allow-all/specific-tools flow (references: MultiSelect, selectedTools,
onValueChange, handleUpdateMCPConfig).
---
Nitpick comments:
In `@core/internal/mcptests/codemode_stdio_test.go`:
- Line 59: Replace the hardcoded camelCase client alias assignments (e.g.,
config.Name = "temperature") with a derived value using the existing toCamelCase
helper: set config.Name = toCamelCase(<kebabCaseIdentifier>) where
<kebabCaseIdentifier> is the original kebab-case identifier you're converting in
that test; do this for all similar sites that currently hardcode the camelCase
string (the other occurrences noted in the comment). This ensures the client
alias derives from the same toCamelCase logic used elsewhere and prevents drift
between code-mode identifiers and include-client filters.
In `@framework/configstore/tables/virtualkey.go`:
- Around line 194-195: The comment on ProviderConfigs was updated to note
deny-by-default when empty; mirror that clarification on the MCPConfigs field by
updating the struct comment for MCPConfigs (TableVirtualKeyMCPConfig:
MCPConfigs) to state that an empty slice means no MCPs are allowed
(deny-by-default) and that the GORM constraint remains OnDelete:CASCADE; place
the brief comment inline next to the MCPConfigs field to match the style used
for ProviderConfigs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db5d16ea-2295-4e6a-b10f-1003a1168f78
📒 Files selected for processing (27)
core/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/mcp.gocore/mcp/utils.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/numberAndSelect.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
✅ Files skipped from review due to trivial changes (1)
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- core/mcp/utils.go
- core/schemas/bifrost.go
- ui/lib/types/governance.ts
- docs/openapi/schemas/management/governance.yaml
- plugins/governance/utils.go
- plugins/governance/resolver_test.go
- docs/mcp/filtering.mdx
- ui/components/ui/numberAndSelect.tsx
- transports/config.schema.json
- docs/features/governance/mcp-tools.mdx
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
18 tasks
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
5fbfacf to
c76b1ee
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/openapi/schemas/management/governance.yaml (1)
253-265:⚠️ Potential issue | 🟡 MinorMirror the deny-by-default descriptions on
UpdateVirtualKeyRequest.
CreateVirtualKeyRequestnow documents that emptyprovider_configsandmcp_configsblock access, but the update schema still omits that. Generated API docs will makePUTlook like it has different semantics even though the backend change applies there too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/schemas/management/governance.yaml` around lines 253 - 265, The UpdateVirtualKeyRequest schema is missing the "deny-by-default" descriptions present on CreateVirtualKeyRequest: update the provider_configs and mcp_configs properties inside UpdateVirtualKeyRequest to mirror the CreateVirtualKeyRequest wording by adding a description that an empty provider_configs and an empty mcp_configs will block access (deny-by-default) and document any nullable/empty semantics used in CreateVirtualKeyRequest so PUT docs match backend behavior; locate the provider_configs and mcp_configs property definitions in the UpdateVirtualKeyRequest schema and copy the exact deny-by-default description text from CreateVirtualKeyRequest.ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
861-899:⚠️ Potential issue | 🟡 MinorAdd
data-testidhooks to the new MCP controls.The MCP client picker and tools selector are new interactive elements in a workspace page, but neither exposes a
data-testid. That makes the new flow harder to cover intests/e2e/and is inconsistent with the rest of this sheet.As per coding guidelines,
ui/app/workspace/**/*.tsx: "UI workspace pages are located in ui/app/workspace// and must include data-testid attributes on all interactive elements. Check existing data-testid values before modifying UI elements."Also applies to: 941-982
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 861 - 899, The MCP client picker and its options lack data-testid attributes; add test IDs to the interactive Select components and items used in this block (the Select, SelectTrigger, SelectValue, SelectContent, and SelectItem elements) and to the related tools selector components referenced later so e2e tests can target them. Specifically, add data-testid attributes like data-testid="mcp-client-select" on the Select or SelectTrigger, data-testid="mcp-client-placeholder" on the SelectValue, data-testid="mcp-client-option-{client.config.name}" on each SelectItem (use a safe encoded name), and analogous IDs for the tools selector (e.g., data-testid="mcp-tools-select" and per-tool items) ensuring names match existing workspace test-id conventions.plugins/governance/main.go (1)
638-642:⚠️ Potential issue | 🟠 MajorDo not continue once every configured provider has been filtered out.
If all provider configs are excluded here—especially by provider-level budget or rate-limit checks—the function leaves the request unmodified and downstream provider selection can still proceed. That turns a governance deny into a pass-through instead of returning a governed 4xx/402/429 response.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 638 - 642, The code currently returns (body, nil, nil) when allowedProviderConfigs is empty, which lets requests continue instead of failing governance; replace this early-return with a definitive governed error response: use the same ctx.AppendRoutingEngineLog call and then return an error/result that signals a governance denial (e.g., a 4xx/402/429-style error) so downstream selection stops; update the branch that checks len(allowedProviderConfigs) == 0 to return that governance error rather than the original body and nils (referencing allowedProviderConfigs, ctx.AppendRoutingEngineLog and the existing return site).
♻️ Duplicate comments (3)
plugins/governance/main.go (1)
654-674:⚠️ Potential issue | 🟠 MajorExclude non-positive weights from the draw.
This still lets
weight <= 0participate in selection. With all-zero weights,totalWeightbecomes0and the loop picks the first provider; with mixed weights, a zero-weight provider can still win whenrandomValue == 0. Filter out non-positive weights before the draw and only run weighted selection when the remaining total is positive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 654 - 674, The weighted-selection loop currently includes non-positive weights which can let zero/negative entries affect the draw; before computing totalWeight and running the rand-based selection, filter weightedConfigs to a new slice containing only entries where getWeight(config.Weight) > 0, recompute totalWeight from that filtered slice, and run the selection over the filtered slice; only perform the weighted selection when totalWeight > 0 (otherwise fall back to the existing non-weighted/fallback path such as using weightedConfigs[0] or the unweighted selection), updating uses of weightedConfigs in that block and ensuring selectedProvider (schemas.ModelProvider) is set from the filtered list when chosen.ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
607-610:⚠️ Potential issue | 🟡 MinorClarify that an empty weight only skips weighted selection.
weight: nullkeeps the provider configured for explicit routing, so this placeholder still overstates the effect. Wording like “Exclude from weighted routing” would match the backend behavior more closely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 607 - 610, Update the placeholder text for the weight input to accurately reflect backend behavior: change the Input placeholder from "Exclude from routing" to something like "Exclude from weighted routing" in virtualKeySheet.tsx (the Input with data-testid `vk-weight-input-${index}` inside the VirtualKeySheet/weight editor) so users understand that leaving weight empty only skips weighted selection and does not disable explicit routing (weight: null still keeps the provider configured for explicit routing).docs/features/governance/mcp-tools.mdx (1)
17-19:⚠️ Potential issue | 🟡 MinorAlign the Web UI steps with the new deny-by-default behavior.
These new bullets now say empty MCP config blocks access, but Line 43 still tells users that leaving the tool list blank allows all tools. That contradiction will lead to misconfigured Virtual Keys if someone follows the UI walkthrough instead of the overview.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/governance/mcp-tools.mdx` around lines 17 - 19, Update the Web UI walkthrough text so it matches the new deny-by-default rule described in the "No MCP Configuration on Virtual Key (Default)" section: replace any guidance that says "leaving the tool list blank allows all tools" with instructions that an empty MCP tool list denies all tools and that you must explicitly add MCP client configurations to permit tools; ensure all references to the Virtual Key creation steps, the "tool list" field, and any example screenshots or copy (the UI walkthrough block) are revised for consistency with the deny-by-default behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/openapi/schemas/management/governance.yaml`:
- Around line 253-265: The UpdateVirtualKeyRequest schema is missing the
"deny-by-default" descriptions present on CreateVirtualKeyRequest: update the
provider_configs and mcp_configs properties inside UpdateVirtualKeyRequest to
mirror the CreateVirtualKeyRequest wording by adding a description that an empty
provider_configs and an empty mcp_configs will block access (deny-by-default)
and document any nullable/empty semantics used in CreateVirtualKeyRequest so PUT
docs match backend behavior; locate the provider_configs and mcp_configs
property definitions in the UpdateVirtualKeyRequest schema and copy the exact
deny-by-default description text from CreateVirtualKeyRequest.
In `@plugins/governance/main.go`:
- Around line 638-642: The code currently returns (body, nil, nil) when
allowedProviderConfigs is empty, which lets requests continue instead of failing
governance; replace this early-return with a definitive governed error response:
use the same ctx.AppendRoutingEngineLog call and then return an error/result
that signals a governance denial (e.g., a 4xx/402/429-style error) so downstream
selection stops; update the branch that checks len(allowedProviderConfigs) == 0
to return that governance error rather than the original body and nils
(referencing allowedProviderConfigs, ctx.AppendRoutingEngineLog and the existing
return site).
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 861-899: The MCP client picker and its options lack data-testid
attributes; add test IDs to the interactive Select components and items used in
this block (the Select, SelectTrigger, SelectValue, SelectContent, and
SelectItem elements) and to the related tools selector components referenced
later so e2e tests can target them. Specifically, add data-testid attributes
like data-testid="mcp-client-select" on the Select or SelectTrigger,
data-testid="mcp-client-placeholder" on the SelectValue,
data-testid="mcp-client-option-{client.config.name}" on each SelectItem (use a
safe encoded name), and analogous IDs for the tools selector (e.g.,
data-testid="mcp-tools-select" and per-tool items) ensuring names match existing
workspace test-id conventions.
---
Duplicate comments:
In `@docs/features/governance/mcp-tools.mdx`:
- Around line 17-19: Update the Web UI walkthrough text so it matches the new
deny-by-default rule described in the "No MCP Configuration on Virtual Key
(Default)" section: replace any guidance that says "leaving the tool list blank
allows all tools" with instructions that an empty MCP tool list denies all tools
and that you must explicitly add MCP client configurations to permit tools;
ensure all references to the Virtual Key creation steps, the "tool list" field,
and any example screenshots or copy (the UI walkthrough block) are revised for
consistency with the deny-by-default behavior.
In `@plugins/governance/main.go`:
- Around line 654-674: The weighted-selection loop currently includes
non-positive weights which can let zero/negative entries affect the draw; before
computing totalWeight and running the rand-based selection, filter
weightedConfigs to a new slice containing only entries where
getWeight(config.Weight) > 0, recompute totalWeight from that filtered slice,
and run the selection over the filtered slice; only perform the weighted
selection when totalWeight > 0 (otherwise fall back to the existing
non-weighted/fallback path such as using weightedConfigs[0] or the unweighted
selection), updating uses of weightedConfigs in that block and ensuring
selectedProvider (schemas.ModelProvider) is set from the filtered list when
chosen.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 607-610: Update the placeholder text for the weight input to
accurately reflect backend behavior: change the Input placeholder from "Exclude
from routing" to something like "Exclude from weighted routing" in
virtualKeySheet.tsx (the Input with data-testid `vk-weight-input-${index}`
inside the VirtualKeySheet/weight editor) so users understand that leaving
weight empty only skips weighted selection and does not disable explicit routing
(weight: null still keeps the provider configured for explicit routing).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2dda15be-ef2a-43cc-a47e-dc0f5259291c
📒 Files selected for processing (27)
core/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/mcp.gocore/mcp/utils.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/numberAndSelect.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
✅ Files skipped from review due to trivial changes (1)
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- ui/lib/types/governance.ts
- core/mcp/utils.go
- core/internal/mcptests/fixtures.go
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- ui/components/ui/numberAndSelect.tsx
- core/schemas/bifrost.go
- docs/providers/provider-routing.mdx
- framework/configstore/tables/virtualkey.go
- transports/bifrost-http/handlers/governance.go
- plugins/governance/resolver.go
- core/internal/mcptests/concurrency_advanced_test.go
- plugins/governance/utils.go
- core/internal/mcptests/codemode_stdio_test.go
- transports/config.schema.json
12 tasks
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
c76b1ee to
585627c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ui/lib/types/governance.ts (1)
87-95:⚠️ Potential issue | 🟠 MajorAllow
nullin the request types too.This updates the returned provider config shape, but the create/update request interfaces below still type
weightasnumber. The UI still can’t sendweight: nullto opt a provider out of weighted routing without casting around the types.🧩 Suggested fix
export interface VirtualKeyProviderConfigRequest { provider: string; - weight?: number; + weight?: number | null; allowed_models?: string[]; budget?: CreateBudgetRequest; rate_limit?: CreateRateLimitRequest; key_ids?: string[]; // List of DBKey UUIDs to associate with this provider config } export interface VirtualKeyProviderConfigUpdateRequest { id?: number; provider: string; - weight?: number; + weight?: number | null; allowed_models?: string[]; budget?: UpdateBudgetRequest; rate_limit?: UpdateRateLimitRequest; key_ids?: string[]; // List of DBKey UUIDs to associate with this provider config }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/governance.ts` around lines 87 - 95, The request interfaces for creating/updating providers still type weight as number only, preventing sending weight: null; update the create/update request types (e.g., CreateVirtualKeyProviderRequest and UpdateVirtualKeyProviderRequest or whatever the request interfaces are named below VirtualKeyProviderConfig) to use weight: number | null to match VirtualKeyProviderConfig so the UI can submit null to opt out of weighted routing; ensure any related helper types or form serialization logic that casts/validates weight are updated accordingly.transports/bifrost-http/handlers/mcpserver.go (1)
315-345:⚠️ Potential issue | 🟠 MajorHonor
x-goog-api-keywhen selecting the VK-specific MCP server.This deny-by-default MCP filtering only works after the request is mapped to the correct virtual key, but
getVKFromRequestbelow still ignoresx-goog-api-key.plugins/governance/utils.goalready treats that header as a valid VK carrier, so MCP requests authenticated that way will miss the VK-specific server and its tool filter entirely.🔧 Suggested fix
func getVKFromRequest(ctx *fasthttp.RequestCtx) string { if value := strings.TrimSpace(string(ctx.Request.Header.Peek(string(schemas.BifrostContextKeyVirtualKey)))); value != "" { return value } @@ if apiKey := strings.TrimSpace(string(ctx.Request.Header.Peek("x-api-key"))); apiKey != "" { if strings.HasPrefix(strings.ToLower(apiKey), governance.VirtualKeyPrefix) { return apiKey } } + + if googleAPIKey := strings.TrimSpace(string(ctx.Request.Header.Peek("x-goog-api-key"))); googleAPIKey != "" { + if strings.HasPrefix(strings.ToLower(googleAPIKey), governance.VirtualKeyPrefix) { + return googleAPIKey + } + } return "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcpserver.go` around lines 315 - 345, The request-to-virtual-key mapping ignores the x-goog-api-key header so MCP requests authenticated via that header never get the VK-specific MCP server and its tool filter; update the VK resolution logic used in getVKFromRequest to honor x-goog-api-key (or reuse the existing carrier parsing in plugins/governance/utils.go that treats x-goog-api-key as a valid VK carrier) so vk.MCPConfigs are applied and the MCPContextKeyIncludeTools filter (executeOnlyTools) is set for those requests; ensure getVKFromRequest (or the caller that selects the VK server) reads x-goog-api-key from the incoming request headers and returns the correct virtual key.plugins/governance/main.go (1)
638-643:⚠️ Potential issue | 🟠 MajorShort-circuit when filtering leaves zero eligible providers.
Line 642 currently lets the request continue unchanged after model/budget/rate-limit filtering removes every configured provider. On an unprefixed
model, that reopens a downstream fallback/default-provider path and weakens the deny-by-default contract this PR is introducing. This branch should deny instead of passing the bare model through.🔧 Minimal hardening
if len(allowedProviderConfigs) == 0 { ctx.AppendRoutingEngineLog(schemas.RoutingEngineGovernance, fmt.Sprintf("No eligible providers remaining after filtering for model %s, skipping load balancing", modelStr)) - // TODO: Send proper error if (overall VK budget/rate limit) or (all provider budgets/rate limits) are violated - // No allowed provider configs, continue without modification - return body, nil, nil + errorBody, err := sonic.Marshal(map[string]any{ + "error": map[string]any{ + "message": fmt.Sprintf("no eligible providers configured for virtual key '%s' for model '%s'", virtualKey.Name, modelStr), + }, + }) + if err != nil { + p.logger.Warn("failed to marshal error body: %v", err) + return body, nil, err + } + return body, &schemas.HTTPResponse{ + StatusCode: 403, + Headers: map[string]string{ + "Content-Type": "application/json", + }, + Body: errorBody, + }, nil }As per coding guidelines, pre-hooks should short-circuit policy denials rather than let the request continue as normal flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 638 - 643, The code currently returns (body, nil, nil) when allowedProviderConfigs is empty, which lets the request continue; change this to short-circuit with a denial by returning an explicit error/denial response: after logging with ctx.AppendRoutingEngineLog(schemas.RoutingEngineGovernance, ...), return nil, nil, fmt.Errorf("request denied: no eligible providers for model %s after governance filtering", modelStr) (or construct/use the project’s PolicyDenied error type if one exists) so callers of this pre-hook can stop processing instead of falling back to defaults; update the branch containing allowedProviderConfigs and the return to produce that error.
♻️ Duplicate comments (2)
docs/features/governance/mcp-tools.mdx (1)
18-19:⚠️ Potential issue | 🟡 MinorKeep the “empty tools” guidance consistent across the page.
These new lines correctly describe deny-by-default, but Line 43 still says leaving the tool list blank allows all tools for that client. That contradiction will lead to the exact opposite configuration.
✏️ Suggested doc edit
-4. Add the tools you want to restrict the VK to, or leave it blank to allow all tools for this client +4. Add the tools you want to allow for this client. Use `*` to allow all tools for this client; leave it blank to block all tools for this client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/governance/mcp-tools.mdx` around lines 18 - 19, The page contains a contradiction: the sentence "If a Virtual Key has no specific MCP configurations, **no MCP tools are available** (deny-by-default)" must be kept consistent with the later statement that currently reads "leaving the tool list blank allows all tools for that client." Update the later wording (the phrase about "leaving the tool list blank") to reflect deny-by-default semantics—e.g., state that leaving the tool list blank grants no tools for that client and that tools must be explicitly listed in the MCP client configuration—so both the earlier line and the later guidance convey the same deny-by-default behavior.ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
941-982:⚠️ Potential issue | 🟡 MinorAdd a stable test id to the MCP tools selector.
This is one of the changed interactive controls in the MCP section, but unlike the surrounding VK inputs/buttons it still can't be targeted via the workspace E2E selector contract. Please expose a
data-testidon theMultiSelectitself or on a stable wrapper around it.As per coding guidelines
UI data-testid attributes are load-bearing — E2E tests depend on them. Convention: data-testid='<entity>-<element>-<qualifier>'. When renaming/removing, search tests/e2e/ for references. Add data-testid to new interactive elements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 941 - 982, Add a stable data-testid to the MCP tools selector by adding a data-testid prop to the MultiSelect (or a stable wrapper element) so E2E tests can target it; update the MultiSelect invocation in virtualKeySheet.tsx (the MultiSelect that uses options/defaultValue/onValueChange/placeholder) to include data-testid="mcp-tools-selector" (or follow the convention data-testid="virtualkey-mcp-tools-selector") and ensure the prop is passed through by the MultiSelect component if it doesn’t already accept arbitrary DOM attributes.
🧹 Nitpick comments (1)
framework/configstore/migrations.go (1)
3615-3696: Precompute missing VK configs instead of counting per row.This backfill loops over every VK twice, does a
COUNT(*)per VK for provider configs and MCP configs, and then reloads every modified VK again for hashing while startup is serialized behind the migration lock. On larger installs that can make upgrades noticeably slower. Consider collecting the VK IDs missing configs with grouped subqueries/left joins and batching the inserts/hash updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 3615 - 3696, The migration currently does per-VK COUNT queries and per-VK inserts which is slow; instead, query for missing configs with grouped subqueries/left joins to collect slices of VK IDs that lack ProviderConfigs and that lack MCPConfigs (use tables.TableVirtualKey left-join tables.TableVirtualKeyProviderConfig and left-join tables.TableVirtualKeyMCPConfig or use NOT EXISTS subqueries) and then perform batched inserts of tables.TableVirtualKeyProviderConfig and tables.TableVirtualKeyMCPConfig for those VK IDs (create records for allProviders and allMCPClients respectively), and finally recompute config_hash by loading the modified VKs in a single query (Preload("ProviderConfigs").Preload("ProviderConfigs.Keys").Preload("MCPConfigs") with WHERE id IN (...)) and update them in a batch instead of reloading one VK per loop; update code paths around modifiedVKIDs, the provider backfill loop, MCP backfill loop, and the final recompute loop to use these batched operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 598-600: The delete affordance is currently an SVG with an onClick
handler (Trash2) which is not keyboard-focusable or labeled; replace it with a
real button element (e.g., a <button type="button">) separate from the accordion
header content and attach the existing handleRemoveProvider(index) to the
button's onClick, add an accessible label (aria-label or visually-hidden text
like "Remove provider") and optional title, ensure the button stops event
propagation so it doesn't toggle the accordion, and keep the data-testid
(vk-delete-provider-{index}) on the new button for tests.
- Around line 186-192: The code treats token_max_limit and request_max_limit
with truthiness (e.g., token_max_limit: config.rate_limit.token_max_limit ?
String(...) : undefined), which causes explicit 0 to be lost; update the
default-value mapping in the virtualKeySheet component to check for
null/undefined instead of falsiness (e.g., use config.rate_limit.token_max_limit
!== null && config.rate_limit.token_max_limit !== undefined to decide to
String(...) ), apply the same change for request_max_limit, and update the
submit normalization logic that currently uses truthy checks so it preserves
numeric 0 (replace any `if (value)` or `value ? ... : undefined` patterns with
explicit null/undefined checks or Number.isFinite where appropriate).
---
Outside diff comments:
In `@plugins/governance/main.go`:
- Around line 638-643: The code currently returns (body, nil, nil) when
allowedProviderConfigs is empty, which lets the request continue; change this to
short-circuit with a denial by returning an explicit error/denial response:
after logging with ctx.AppendRoutingEngineLog(schemas.RoutingEngineGovernance,
...), return nil, nil, fmt.Errorf("request denied: no eligible providers for
model %s after governance filtering", modelStr) (or construct/use the project’s
PolicyDenied error type if one exists) so callers of this pre-hook can stop
processing instead of falling back to defaults; update the branch containing
allowedProviderConfigs and the return to produce that error.
In `@transports/bifrost-http/handlers/mcpserver.go`:
- Around line 315-345: The request-to-virtual-key mapping ignores the
x-goog-api-key header so MCP requests authenticated via that header never get
the VK-specific MCP server and its tool filter; update the VK resolution logic
used in getVKFromRequest to honor x-goog-api-key (or reuse the existing carrier
parsing in plugins/governance/utils.go that treats x-goog-api-key as a valid VK
carrier) so vk.MCPConfigs are applied and the MCPContextKeyIncludeTools filter
(executeOnlyTools) is set for those requests; ensure getVKFromRequest (or the
caller that selects the VK server) reads x-goog-api-key from the incoming
request headers and returns the correct virtual key.
In `@ui/lib/types/governance.ts`:
- Around line 87-95: The request interfaces for creating/updating providers
still type weight as number only, preventing sending weight: null; update the
create/update request types (e.g., CreateVirtualKeyProviderRequest and
UpdateVirtualKeyProviderRequest or whatever the request interfaces are named
below VirtualKeyProviderConfig) to use weight: number | null to match
VirtualKeyProviderConfig so the UI can submit null to opt out of weighted
routing; ensure any related helper types or form serialization logic that
casts/validates weight are updated accordingly.
---
Duplicate comments:
In `@docs/features/governance/mcp-tools.mdx`:
- Around line 18-19: The page contains a contradiction: the sentence "If a
Virtual Key has no specific MCP configurations, **no MCP tools are available**
(deny-by-default)" must be kept consistent with the later statement that
currently reads "leaving the tool list blank allows all tools for that client."
Update the later wording (the phrase about "leaving the tool list blank") to
reflect deny-by-default semantics—e.g., state that leaving the tool list blank
grants no tools for that client and that tools must be explicitly listed in the
MCP client configuration—so both the earlier line and the later guidance convey
the same deny-by-default behavior.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 941-982: Add a stable data-testid to the MCP tools selector by
adding a data-testid prop to the MultiSelect (or a stable wrapper element) so
E2E tests can target it; update the MultiSelect invocation in
virtualKeySheet.tsx (the MultiSelect that uses
options/defaultValue/onValueChange/placeholder) to include
data-testid="mcp-tools-selector" (or follow the convention
data-testid="virtualkey-mcp-tools-selector") and ensure the prop is passed
through by the MultiSelect component if it doesn’t already accept arbitrary DOM
attributes.
---
Nitpick comments:
In `@framework/configstore/migrations.go`:
- Around line 3615-3696: The migration currently does per-VK COUNT queries and
per-VK inserts which is slow; instead, query for missing configs with grouped
subqueries/left joins to collect slices of VK IDs that lack ProviderConfigs and
that lack MCPConfigs (use tables.TableVirtualKey left-join
tables.TableVirtualKeyProviderConfig and left-join
tables.TableVirtualKeyMCPConfig or use NOT EXISTS subqueries) and then perform
batched inserts of tables.TableVirtualKeyProviderConfig and
tables.TableVirtualKeyMCPConfig for those VK IDs (create records for
allProviders and allMCPClients respectively), and finally recompute config_hash
by loading the modified VKs in a single query
(Preload("ProviderConfigs").Preload("ProviderConfigs.Keys").Preload("MCPConfigs")
with WHERE id IN (...)) and update them in a batch instead of reloading one VK
per loop; update code paths around modifiedVKIDs, the provider backfill loop,
MCP backfill loop, and the final recompute loop to use these batched operations.
🪄 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: bdecccdd-1d16-41e0-8694-1d0cc25951bd
📒 Files selected for processing (27)
core/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/mcp.gocore/mcp/utils.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/numberAndSelect.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
✅ Files skipped from review due to trivial changes (1)
- framework/configstore/tables/virtualkey.go
🚧 Files skipped from review as they are similar to previous changes (12)
- core/internal/mcptests/concurrency_advanced_test.go
- transports/config.schema.json
- transports/bifrost-http/handlers/governance.go
- core/schemas/bifrost.go
- core/internal/mcptests/agent_test_helpers.go
- core/internal/mcptests/fixtures.go
- core/mcp/utils.go
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- docs/providers/provider-routing.mdx
- docs/mcp/filtering.mdx
- plugins/governance/resolver.go
- docs/openapi/schemas/management/governance.yaml
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
585627c to
89bd338
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/governance/main.go (1)
552-559:⚠️ Potential issue | 🟠 MajorPrefixed-model early return bypasses VK provider enforcement in this path.
Line 558 returns before the virtual-key provider checks run (including the deny-by-default branch at Line 569). For provider-prefixed models, this skips governance filtering in
loadBalanceProviderentirely.At minimum, validate that the requested prefixed provider/model is allowed by
virtualKey.ProviderConfigsbefore returning early.🔧 Fix direction
if strings.Contains(modelStr, "/") { - provider, _ := schemas.ParseModelString(modelStr, "") + requestedProvider, requestedModel := schemas.ParseModelString(modelStr, "") if p.inMemoryStore != nil { - if _, ok := p.inMemoryStore.GetConfiguredProviders()[provider]; ok { - return body, nil, nil + if _, ok := p.inMemoryStore.GetConfiguredProviders()[requestedProvider]; ok { + // Do not return yet; continue and enforce VK provider/model policy. + // After allowedProviderConfigs is built: + // - if requestedProvider/model is allowed -> return body unchanged + // - else -> return deny short-circuit (403) + modelStr = requestedModel } } else { return body, nil, nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 552 - 559, The early-return for provider-prefixed models in the block that checks modelStr and calls schemas.ParseModelString bypasses virtual-key provider enforcement in loadBalanceProvider; before returning from that branch (where p.inMemoryStore != nil and provider found in p.inMemoryStore.GetConfiguredProviders()), validate that the parsed provider is permitted by p.virtualKey.ProviderConfigs (or via the same validation helper used by loadBalanceProvider) and only return body,nil,nil if the provider is allowed—otherwise proceed to run the normal virtual-key/provider enforcement logic.
♻️ Duplicate comments (1)
docs/features/governance/mcp-tools.mdx (1)
18-19:⚠️ Potential issue | 🟡 MinorThis page still has contradictory empty-tool behavior.
The deny-by-default statement here conflicts with Line 43 (“leave it blank to allow all tools for this client”). Please align Line 43 to “leave it blank to block all tools for this client.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/governance/mcp-tools.mdx` around lines 18 - 19, The page contains contradictory statements about empty MCP client-tool behavior: the top says "deny-by-default" while Line 43 currently reads "leave it blank to allow all tools for this client"; update the text on Line 43 to read "leave it blank to block all tools for this client" (or equivalent phrasing) so it matches the deny-by-default rule and removes the contradiction.
🧹 Nitpick comments (1)
framework/configstore/migrations.go (1)
3640-3677: Consider removing per-VK count queries to avoid startup-time N+1 load.Line 3643 and Line 3675 run
Countper virtual key, twice. On large datasets this can significantly slow startup while the migration lock is held. Precomputing VK IDs that already have provider/MCP configs (single grouped query each) would make this migration scale better.♻️ Refactor sketch (batch existence checks)
- for _, vk := range allVKs { - var providerConfigCount int64 - if err := tx.Model(&tables.TableVirtualKeyProviderConfig{}).Where("virtual_key_id = ?", vk.ID).Count(&providerConfigCount).Error; err != nil { - return fmt.Errorf("failed to count provider configs for VK %s: %w", vk.ID, err) - } - if providerConfigCount == 0 && len(allProviders) > 0 { ... } - } + providerConfigExists := map[string]struct{}{} + var providerRows []struct{ VirtualKeyID string } + if err := tx.Model(&tables.TableVirtualKeyProviderConfig{}). + Select("DISTINCT virtual_key_id"). + Find(&providerRows).Error; err != nil { + return fmt.Errorf("failed to query existing provider configs: %w", err) + } + for _, r := range providerRows { + providerConfigExists[r.VirtualKeyID] = struct{}{} + } + for _, vk := range allVKs { + if _, exists := providerConfigExists[vk.ID]; !exists && len(allProviders) > 0 { ... } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 3640 - 3677, The per-VK Count queries inside the loop (using tx.Model(&tables.TableVirtualKeyProviderConfig{})... Count => providerConfigCount and tx.Model(&tables.TableVirtualKeyMCPConfig{})... Count => mcpConfigCount) cause an N+1; replace them by precomputing sets of VK IDs that already have provider configs and MCP configs with two grouped queries (SELECT DISTINCT virtual_key_id FROM TableVirtualKeyProviderConfig WHERE virtual_key_id IN (...allVKs IDs... ) and similarly for TableVirtualKeyMCPConfig), build maps of existing IDs, then iterate allVKs and check map membership instead of running Count per vk; keep the rest of the logic (creating provider configs, modifiedVKIDs, logging, tx.Create, error handling) unchanged and use the same symbols (allVKs, tx, modifiedVKIDs, tables.TableVirtualKeyProviderConfig, tables.TableVirtualKeyMCPConfig) to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugins/governance/main.go`:
- Around line 552-559: The early-return for provider-prefixed models in the
block that checks modelStr and calls schemas.ParseModelString bypasses
virtual-key provider enforcement in loadBalanceProvider; before returning from
that branch (where p.inMemoryStore != nil and provider found in
p.inMemoryStore.GetConfiguredProviders()), validate that the parsed provider is
permitted by p.virtualKey.ProviderConfigs (or via the same validation helper
used by loadBalanceProvider) and only return body,nil,nil if the provider is
allowed—otherwise proceed to run the normal virtual-key/provider enforcement
logic.
---
Duplicate comments:
In `@docs/features/governance/mcp-tools.mdx`:
- Around line 18-19: The page contains contradictory statements about empty MCP
client-tool behavior: the top says "deny-by-default" while Line 43 currently
reads "leave it blank to allow all tools for this client"; update the text on
Line 43 to read "leave it blank to block all tools for this client" (or
equivalent phrasing) so it matches the deny-by-default rule and removes the
contradiction.
---
Nitpick comments:
In `@framework/configstore/migrations.go`:
- Around line 3640-3677: The per-VK Count queries inside the loop (using
tx.Model(&tables.TableVirtualKeyProviderConfig{})... Count =>
providerConfigCount and tx.Model(&tables.TableVirtualKeyMCPConfig{})... Count =>
mcpConfigCount) cause an N+1; replace them by precomputing sets of VK IDs that
already have provider configs and MCP configs with two grouped queries (SELECT
DISTINCT virtual_key_id FROM TableVirtualKeyProviderConfig WHERE virtual_key_id
IN (...allVKs IDs... ) and similarly for TableVirtualKeyMCPConfig), build maps
of existing IDs, then iterate allVKs and check map membership instead of running
Count per vk; keep the rest of the logic (creating provider configs,
modifiedVKIDs, logging, tx.Create, error handling) unchanged and use the same
symbols (allVKs, tx, modifiedVKIDs, tables.TableVirtualKeyProviderConfig,
tables.TableVirtualKeyMCPConfig) to locate and update the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ab9ff4e-17e1-464b-9703-8e3ca057f7f5
📒 Files selected for processing (19)
core/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/mcp.gocore/mcp/utils.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.go
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
✅ Files skipped from review due to trivial changes (1)
- framework/configstore/tables/virtualkey.go
🚧 Files skipped from review as they are similar to previous changes (6)
- core/internal/mcptests/agent_test_helpers.go
- core/schemas/bifrost.go
- plugins/governance/resolver_test.go
- docs/openapi/schemas/management/governance.yaml
- docs/providers/provider-routing.mdx
- docs/features/governance/routing.mdx
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
89bd338 to
883605d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
1004-1045:⚠️ Potential issue | 🟡 MinorMissing
data-testidon MCP tools MultiSelect.The MCP tools
MultiSelectcomponent lacks adata-testidattribute. Per coding guidelines, UI interactive elements must havedata-testidattributes following the pattern<entity>-<element>-<qualifier>.🔧 Suggested fix
<MultiSelect + data-testid={`vk-mcp-tools-multiselect-${index}`} options={[ { label: "Allow All Tools",As per coding guidelines:
ui/**/*.{ts,tsx}: UI interactive elements must have data-testid attributes following the pattern 'data-testid="--"'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 1004 - 1045, The MultiSelect instance rendering the MCP tools (the component with props options, defaultValue={selectedTools}, onValueChange calling handleUpdateMCPConfig, etc.) is missing a data-testid; add a data-testid following the project's pattern (e.g., data-testid="mcp-tools-multiselect" or similar '<entity>-<element>-<qualifier>') to that MultiSelect JSX element so tests can reliably target it without changing any behavior in onValueChange or other props.
♻️ Duplicate comments (3)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (2)
186-192:⚠️ Potential issue | 🟠 MajorExplicit
0rate limits are still lost due to truthiness check.Lines 188 and 190 use
config.rate_limit.token_max_limit ? String(...) : undefined, which treats0as falsy and converts it toundefined. An existing rate limit of0will render as blank and be lost on save.🔧 Suggested fix
rate_limit: config.rate_limit ? { - token_max_limit: config.rate_limit.token_max_limit ? String(config.rate_limit.token_max_limit) : undefined, + token_max_limit: config.rate_limit.token_max_limit !== undefined && config.rate_limit.token_max_limit !== null + ? String(config.rate_limit.token_max_limit) + : undefined, token_reset_duration: config.rate_limit.token_reset_duration, - request_max_limit: config.rate_limit.request_max_limit ? String(config.rate_limit.request_max_limit) : undefined, + request_max_limit: config.rate_limit.request_max_limit !== undefined && config.rate_limit.request_max_limit !== null + ? String(config.rate_limit.request_max_limit) + : undefined, request_reset_duration: config.rate_limit.request_reset_duration, } : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 186 - 192, The mapping for rate_limit in virtualKeySheet.tsx incorrectly treats numeric zero as falsy, so token_max_limit and request_max_limit become undefined when they are 0; change the checks to explicitly test for null/undefined (e.g., use config.rate_limit.token_max_limit != null or typeof ... !== 'undefined') before converting to String, and apply the same fix for request_max_limit so explicit 0 values are preserved when constructing rate_limit.
658-660:⚠️ Potential issue | 🟠 MajorProvider delete is still an inaccessible SVG, not a keyboard-navigable button.
The
Trash2icon has anonClickhandler anddata-testid, but it's not wrapped in a<button>element. This makes it:
- Not keyboard-focusable (can't Tab to it)
- Missing accessible name for screen readers
Compare with the MCP delete at line 1048 which correctly uses
<Button>.🔧 Suggested fix
<div className="hover:bg-accent/50 cursor-pointer rounded-sm p-2"> - <Trash2 onClick={() => handleRemoveProvider(index)} className="h-4 w-4 opacity-75" data-testid={`vk-delete-provider-${index}`} /> + <button + type="button" + onClick={(e) => { + e.stopPropagation(); + handleRemoveProvider(index); + }} + className="hover:bg-accent/50 cursor-pointer rounded-sm p-2" + aria-label={`Remove ${ProviderLabels[config.provider as ProviderName] || config.provider} provider`} + data-testid={`vk-delete-provider-${index}`} + > + <Trash2 className="h-4 w-4 opacity-75" /> + </button> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 658 - 660, The Trash2 icon is used as a clickable element but isn't keyboard-focusable or labelled; wrap the icon in a real interactive element (use the same Button component or a native <button>) and move the onClick from the SVG to that button, add an accessible name via aria-label (e.g., aria-label={`Delete provider ${index}`}) and keep the data-testid on the button (data-testid={`vk-delete-provider-${index}`}) so handleRemoveProvider(index) remains invoked; update the element around Trash2 in virtualKeySheet.tsx accordingly to mirror the MCP delete implementation.plugins/governance/main.go (1)
621-651:⚠️ Potential issue | 🟠 MajorGuard non-positive aggregate weight before weighted draw.
If all set weights are
<= 0, the current path can still pick a provider unexpectedly. Filter to strictly positive weights and skip weighted draw when aggregate weight is non-positive.🔧 Suggested hardening
if len(weightedConfigs) > 0 { - // Weighted random selection from providers that have weight set - totalWeight := 0.0 - for _, config := range weightedConfigs { - totalWeight += getWeight(config.Weight) - } - // Generate random number between 0 and totalWeight - randomValue := rand.Float64() * totalWeight - // Select provider based on weighted random selection - currentWeight := 0.0 - for _, config := range weightedConfigs { - currentWeight += getWeight(config.Weight) - if randomValue <= currentWeight { - selectedProvider = schemas.ModelProvider(config.Provider) - break - } - } - // Fallback: if no provider was selected (shouldn't happen but guard against FP issues) - if selectedProvider == "" { - selectedProvider = schemas.ModelProvider(weightedConfigs[0].Provider) - } + // Weighted random selection from providers that have strictly positive weights + positiveWeightedConfigs := make([]configstoreTables.TableVirtualKeyProviderConfig, 0, len(weightedConfigs)) + totalWeight := 0.0 + for _, config := range weightedConfigs { + w := getWeight(config.Weight) + if w <= 0 { + continue + } + totalWeight += w + positiveWeightedConfigs = append(positiveWeightedConfigs, config) + } + if totalWeight > 0 && len(positiveWeightedConfigs) > 0 { + randomValue := rand.Float64() * totalWeight + currentWeight := 0.0 + for _, config := range positiveWeightedConfigs { + currentWeight += getWeight(config.Weight) + if randomValue <= currentWeight { + selectedProvider = schemas.ModelProvider(config.Provider) + break + } + } + } + if selectedProvider == "" { + selectedProvider = schemas.ModelProvider(allowedProviderConfigs[0].Provider) + } } else { // No providers have weight set - select first allowed provider without load balancing selectedProvider = schemas.ModelProvider(allowedProviderConfigs[0].Provider) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 621 - 651, The weighted-selection logic currently sums all getWeight(config.Weight) values from weightedConfigs and proceeds even if the aggregate is <= 0; change the flow in the block that builds weightedConfigs/does the draw so that you first filter/collect only entries with strictly positive getWeight(config.Weight) (e.g., build a positiveWeighted slice), compute totalWeight from that slice, and only perform the rand.Float64-based selection when totalWeight > 0; if totalWeight <= 0 (or the positive slice is empty) set selectedProvider to a safe fallback (for example schemas.ModelProvider(weightedConfigs[0].Provider) or another explicit default) instead of performing the weighted draw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/changelog.md`:
- Around line 1-3: The changelog in core/changelog.md currently contains
unrelated entries; replace them with entries that match this PR's objectives:
add a breaking-change entry stating Virtual Key provider_configs and mcp_configs
now deny-by-default when empty, add an entry that provider weight is now
optional/nullable, and add an entry documenting the automatic migration
migrationBackfillEmptyVirtualKeyConfigs which preserves existing behavior; also
verify and remove or move any entries that belong to other PRs (`#2008`, `#2006`,
`#1940`, `#1933`) so the changelog reflects only the changes implemented in this PR.
In `@plugins/governance/main.go`:
- Around line 501-503: The doc comment for loadBalanceProvider is stale: it
mentions returning *schemas.HTTPResponse but the function signature for
loadBalanceProvider(ctx *schemas.BifrostContext, req *schemas.HTTPRequest, body
map[string]any, virtualKey *configstoreTables.TableVirtualKey) actually returns
(map[string]any, error); update the comment to describe these returns
(map[string]any for payload and error for processing failures) and remove or
replace the *schemas.HTTPResponse bullet, or if the intended behavior was to
return *schemas.HTTPResponse, change the function signature accordingly; ensure
the comment text references the function name loadBalanceProvider and the actual
return types so callers/readers are accurate.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 205-209: The bulk tri-state "Auto-execute All" handler should not
collapse the enabled-tools state to the wildcard ["*"] while row toggles now
preserve explicit lists; update the bulk-control code that currently writes
["*"] to instead write the explicit list of tool names (use allToolNames) so it
sets newAutoExecute to allToolNames (or toggles back to the explicit subset) and
keep the same isAllToolsMode check to detect full enablement; locate the handler
that produces ["*"] and replace that assignment with an explicit allToolNames
assignment (and ensure any save/emit path uses newAutoExecute consistently).
---
Outside diff comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 1004-1045: The MultiSelect instance rendering the MCP tools (the
component with props options, defaultValue={selectedTools}, onValueChange
calling handleUpdateMCPConfig, etc.) is missing a data-testid; add a data-testid
following the project's pattern (e.g., data-testid="mcp-tools-multiselect" or
similar '<entity>-<element>-<qualifier>') to that MultiSelect JSX element so
tests can reliably target it without changing any behavior in onValueChange or
other props.
---
Duplicate comments:
In `@plugins/governance/main.go`:
- Around line 621-651: The weighted-selection logic currently sums all
getWeight(config.Weight) values from weightedConfigs and proceeds even if the
aggregate is <= 0; change the flow in the block that builds weightedConfigs/does
the draw so that you first filter/collect only entries with strictly positive
getWeight(config.Weight) (e.g., build a positiveWeighted slice), compute
totalWeight from that slice, and only perform the rand.Float64-based selection
when totalWeight > 0; if totalWeight <= 0 (or the positive slice is empty) set
selectedProvider to a safe fallback (for example
schemas.ModelProvider(weightedConfigs[0].Provider) or another explicit default)
instead of performing the weighted draw.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 186-192: The mapping for rate_limit in virtualKeySheet.tsx
incorrectly treats numeric zero as falsy, so token_max_limit and
request_max_limit become undefined when they are 0; change the checks to
explicitly test for null/undefined (e.g., use config.rate_limit.token_max_limit
!= null or typeof ... !== 'undefined') before converting to String, and apply
the same fix for request_max_limit so explicit 0 values are preserved when
constructing rate_limit.
- Around line 658-660: The Trash2 icon is used as a clickable element but isn't
keyboard-focusable or labelled; wrap the icon in a real interactive element (use
the same Button component or a native <button>) and move the onClick from the
SVG to that button, add an accessible name via aria-label (e.g.,
aria-label={`Delete provider ${index}`}) and keep the data-testid on the button
(data-testid={`vk-delete-provider-${index}`}) so handleRemoveProvider(index)
remains invoked; update the element around Trash2 in virtualKeySheet.tsx
accordingly to mirror the MCP delete implementation.
🪄 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: 0cf89843-7d33-4030-b732-926b81363a87
⛔ Files ignored due to path filters (1)
core/go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
core/changelog.mdcore/go.modcore/internal/llmtests/tests.gocore/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/agent_test.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/mcp/utils.gocore/providers/gemini/chat.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/numberAndSelect.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (2)
- core/mcp/mcp.go
- core/internal/llmtests/tests.go
🚧 Files skipped from review as they are similar to previous changes (15)
- docs/features/governance/mcp-tools.mdx
- framework/configstore/tables/virtualkey.go
- ui/lib/types/governance.ts
- docs/mcp/filtering.mdx
- core/internal/mcptests/codemode_stdio_test.go
- core/schemas/bifrost.go
- plugins/governance/resolver.go
- docs/openapi/schemas/management/governance.yaml
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- transports/bifrost-http/handlers/governance.go
- docs/features/governance/routing.mdx
- core/internal/mcptests/concurrency_advanced_test.go
- transports/config.schema.json
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- framework/configstore/clientconfig.go
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
883605d to
3b26787
Compare
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
3b26787 to
344b075
Compare
This was referenced Mar 17, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (2)
1004-1045:⚠️ Potential issue | 🟡 MinorAdd a
data-testidto the MCP tools selector.The changed interactive
MultiSelecthere still lacks adata-testid, which makes this control harder to target reliably in tests.As per coding guidelines, "UI interactive elements must have data-testid attributes following the pattern 'data-testid="--"'".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 1004 - 1045, The MultiSelect used for MCP tool selection is missing a data-testid which prevents reliable test targeting; add a data-testid prop to this MultiSelect (e.g., data-testid="mcp-tools-selector" or follow the project pattern like "mcp-tools-selector-mobile/desktop" if qualifier needed) so tests can query it; update the JSX for the MultiSelect component (the instance that uses selectedTools and onValueChange which calls handleUpdateMCPConfig) to include the new data-testid attribute.
658-660:⚠️ Potential issue | 🟠 MajorUse a real button for provider removal (current icon click target is not accessible).
At Line 659,
Trash2is directly clickable, so it is not keyboard-focusable or properly labeled. This should be a<button type="button">witharia-label, and click should stop propagation to avoid toggling the accordion.Proposed fix
-<div className="hover:bg-accent/50 cursor-pointer rounded-sm p-2"> - <Trash2 onClick={() => handleRemoveProvider(index)} className="h-4 w-4 opacity-75" data-testid={`vk-delete-provider-${index}`} /> -</div> +<button + type="button" + className="hover:bg-accent/50 rounded-sm p-2" + aria-label={`remove provider ${config.provider}`} + data-testid={`vk-delete-provider-${index}`} + onClick={(e) => { + e.stopPropagation(); + handleRemoveProvider(index); + }} +> + <Trash2 className="h-4 w-4 opacity-75" /> +</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 658 - 660, Replace the clickable icon with a real button element to make provider removal accessible: wrap/replace the Trash2 icon inside a <button type="button"> (instead of relying on the icon's onClick), add a clear aria-label like "Remove provider" and ensure the onClick handler calls handleRemoveProvider(index) and stops event propagation (e.stopPropagation()) so the accordion doesn't toggle; update any existing onClick on Trash2 to the button and keep the className styling/aria-testids (e.g., data-testid={`vk-delete-provider-${index}`}) on the button for tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/migrations.go`:
- Around line 3697-3808: The migration for "backfill_empty_virtual_key_configs"
uses migrator.New with migrator.DefaultOptions but does not opt into a
transactional boundary; change the migrator options passed to migrator.New so
UseTransaction: true (instead of DefaultOptions) for the migration with ID
"backfill_empty_virtual_key_configs" to ensure the entire Migrate block (the DB
reads, tx.Create calls, and later config_hash recompute) runs atomically and
will roll back on failure; keep the same Migrate and Rollback functions and only
alter the options struct passed into migrator.New to enable UseTransaction.
In `@plugins/governance/main.go`:
- Around line 620-654: The weighted-selection fallback must use the same
candidate set used for sampling: replace any fallback that references
allowedProviderConfigs[0] when selection logic iterates weightedConfigs with
weightedConfigs[0] so a provider with nil Weight is not accidentally chosen;
update the selection/fallback logic that uses variables weightedConfigs,
allowedProviderConfigs, selectedProvider, isWeightSet and getWeight accordingly,
and apply the same fix to the similar block around the 682-709 range so both
initial and retry paths use the weighted candidate set consistently.
---
Duplicate comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 1004-1045: The MultiSelect used for MCP tool selection is missing
a data-testid which prevents reliable test targeting; add a data-testid prop to
this MultiSelect (e.g., data-testid="mcp-tools-selector" or follow the project
pattern like "mcp-tools-selector-mobile/desktop" if qualifier needed) so tests
can query it; update the JSX for the MultiSelect component (the instance that
uses selectedTools and onValueChange which calls handleUpdateMCPConfig) to
include the new data-testid attribute.
- Around line 658-660: Replace the clickable icon with a real button element to
make provider removal accessible: wrap/replace the Trash2 icon inside a <button
type="button"> (instead of relying on the icon's onClick), add a clear
aria-label like "Remove provider" and ensure the onClick handler calls
handleRemoveProvider(index) and stops event propagation (e.stopPropagation()) so
the accordion doesn't toggle; update any existing onClick on Trash2 to the
button and keep the className styling/aria-testids (e.g.,
data-testid={`vk-delete-provider-${index}`}) on the button for tests.
🪄 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: 38723a05-1989-4a4e-b037-0dab359c9866
📒 Files selected for processing (28)
core/go.modcore/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/mcp.gocore/mcp/utils.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/numberAndSelect.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
🚧 Files skipped from review as they are similar to previous changes (12)
- ui/components/ui/numberAndSelect.tsx
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- core/go.mod
- core/internal/mcptests/agent_test_helpers.go
- plugins/governance/resolver.go
- docs/features/governance/mcp-tools.mdx
- core/mcp/utils.go
- core/schemas/bifrost.go
- docs/openapi/schemas/management/governance.yaml
- docs/features/governance/routing.mdx
- core/internal/mcptests/fixtures.go
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
344b075 to
c97cc50
Compare
12 tasks
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
c97cc50 to
00efab7
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
plugins/governance/main.go (1)
620-649:⚠️ Potential issue | 🟠 MajorSkip non-positive weights before sampling.
0is still a valid first-party input, but this branch adds it intototalWeight. If every weighted config is<= 0, the draw collapses andweightedConfigs[0]is selected anyway, so a zero-weight provider can still receive traffic. Filter non-positive weights out of the candidate set and treat an empty/non-positive total as “no weighted route”; the later fallback block should use the same filtered slice.🔧 Suggested hardening
- totalWeight := 0.0 - for _, config := range weightedConfigs { - totalWeight += getWeight(config.Weight) - } + totalWeight := 0.0 + positiveConfigs := make([]configstoreTables.TableVirtualKeyProviderConfig, 0, len(weightedConfigs)) + for _, config := range weightedConfigs { + w := getWeight(config.Weight) + if w <= 0 { + continue + } + totalWeight += w + positiveConfigs = append(positiveConfigs, config) + } + if totalWeight <= 0 || len(positiveConfigs) == 0 { + return body, nil + } // Generate random number between 0 and totalWeight randomValue := rand.Float64() * totalWeight // Select provider based on weighted random selection currentWeight := 0.0 - for _, config := range weightedConfigs { + for _, config := range positiveConfigs { currentWeight += getWeight(config.Weight) if randomValue <= currentWeight { selectedProvider = schemas.ModelProvider(config.Provider) break } } // Fallback: if no provider was selected (shouldn't happen but guard against FP issues) if selectedProvider == "" { - selectedProvider = schemas.ModelProvider(weightedConfigs[0].Provider) + selectedProvider = schemas.ModelProvider(positiveConfigs[0].Provider) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 620 - 649, Filter out configs whose getWeight(config.Weight) <= 0 before doing the weighted selection: build a new slice (e.g., positiveWeightedConfigs) from weightedConfigs by keeping only entries with strictly positive weight, compute totalWeight from that slice, run the random draw over positiveWeightedConfigs, and use that same filtered slice when applying the fallback instead of weightedConfigs; if positiveWeightedConfigs is empty, skip the weighted branch entirely so the later non-weighted routing logic executes.ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (3)
659-660:⚠️ Potential issue | 🟠 MajorMake the remove actions accessible buttons with labels.
The provider delete affordance is still an SVG click target inside the accordion header, so it isn't keyboard-focusable and it will also toggle the accordion when clicked. The MCP delete action is focusable now, but it's still an unlabeled icon-only button. Use real buttons with
aria-labels, and stop propagation on the provider action.🔧 Suggested fix
- <div className="hover:bg-accent/50 cursor-pointer rounded-sm p-2"> - <Trash2 onClick={() => handleRemoveProvider(index)} className="h-4 w-4 opacity-75" data-testid={`vk-delete-provider-${index}`} /> - </div> + <Button + type="button" + variant="ghost" + size="sm" + aria-label={`Remove provider ${config.provider}`} + data-testid={`vk-delete-provider-${index}`} + onClick={(e) => { + e.stopPropagation(); + handleRemoveProvider(index); + }} + > + <Trash2 className="h-4 w-4 opacity-75" /> + </Button> @@ - <Button type="button" variant="ghost" size="sm" onClick={() => handleRemoveMCPClient(index)} data-testid={`vk-delete-mcp-${index}`}> + <Button + type="button" + variant="ghost" + size="sm" + aria-label={`Remove MCP client ${config.mcp_client_name}`} + onClick={() => handleRemoveMCPClient(index)} + data-testid={`vk-delete-mcp-${index}`} + > <Trash2 className="h-4 w-4" /> </Button>Also applies to: 1049-1050
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 659 - 660, The provider delete icon (Trash2) inside the accordion header must be converted to a real <button> element with an explicit aria-label and keyboard focusability, call the existing handler handleRemoveProvider(index), and stop event propagation (e.stopPropagation()) in the click handler so the accordion does not toggle; likewise replace the MCP delete icon-only button with a labeled button (aria-label) to provide screen reader text. Locate the Trash2 usage and the MCP delete action in virtualKeySheet.tsx and ensure both use semantic <button> elements, include aria-labels, and call event.stopPropagation() before invoking handleRemoveProvider or the MCP remove handler.
171-212:⚠️ Potential issue | 🟠 MajorPreserve explicit
0rate limits when hydrating the edit form.These truthy checks still collapse
0toundefined, so an existing zero-valued limit renders blank and gets cleared on the next save. Use explicitnull/undefinedchecks for both provider-level and virtual-key-level rate limits.🔧 Suggested fix
- token_max_limit: config.rate_limit.token_max_limit ? String(config.rate_limit.token_max_limit) : undefined, + token_max_limit: + config.rate_limit.token_max_limit === undefined || config.rate_limit.token_max_limit === null + ? undefined + : String(config.rate_limit.token_max_limit), token_reset_duration: config.rate_limit.token_reset_duration, - request_max_limit: config.rate_limit.request_max_limit ? String(config.rate_limit.request_max_limit) : undefined, + request_max_limit: + config.rate_limit.request_max_limit === undefined || config.rate_limit.request_max_limit === null + ? undefined + : String(config.rate_limit.request_max_limit), request_reset_duration: config.rate_limit.request_reset_duration, } : undefined, @@ - tokenMaxLimit: virtualKey?.rate_limit?.token_max_limit ? String(virtualKey.rate_limit.token_max_limit) : "", + tokenMaxLimit: + virtualKey?.rate_limit?.token_max_limit === undefined || virtualKey?.rate_limit?.token_max_limit === null + ? "" + : String(virtualKey.rate_limit.token_max_limit), tokenResetDuration: virtualKey?.rate_limit?.token_reset_duration || "1h", - requestMaxLimit: virtualKey?.rate_limit?.request_max_limit ? String(virtualKey.rate_limit.request_max_limit) : "", + requestMaxLimit: + virtualKey?.rate_limit?.request_max_limit === undefined || virtualKey?.rate_limit?.request_max_limit === null + ? "" + : String(virtualKey.rate_limit.request_max_limit), requestResetDuration: virtualKey?.rate_limit?.request_reset_duration || "1h",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 171 - 212, The form hydration collapses zero-valued limits because it uses truthy checks (e.g., config.rate_limit ? ... and config.rate_limit.token_max_limit ? String(...) : undefined, and top-level tokenMaxLimit/requestMaxLimit checks), which turn 0 into undefined; update the checks in the providerConfigs mapping and the top-level virtual-key fields (the resolver call that builds providerConfigs, rate_limit/token_max_limit/token_reset_duration/request_max_limit/request_reset_duration, and the top-level tokenMaxLimit/requestMaxLimit/budgetMaxLimit) to test explicitly for null/undefined (e.g., rate_limit != null and token_max_limit != null) instead of truthiness so zero values are preserved when stringified and rendered in the edit form.
1005-1046:⚠️ Potential issue | 🟡 MinorAdd a stable test hook to the MCP tools selector.
This is the only new interactive control in the MCP row without a
data-testid, so it can't be targeted consistently alongside the weight input and delete button. IfMultiSelectdoesn't forward arbitrary props, expose the hook on a wrapper or extend the component API.As per coding guidelines
ui/**/*.{ts,tsx}: UI interactive elements must have data-testid attributes following the patterndata-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/virtual-keys/views/virtualKeySheet.tsx` around lines 1005 - 1046, The MultiSelect control for MCP tools needs a stable test hook; add a data-testid following the pattern (e.g., data-testid="mcp-tools-selector-row") so tests can target it like the weight input and delete button. If the MultiSelect component forwards arbitrary props, pass data-testid directly to the MultiSelect invocation in virtualKeySheet.tsx; if it does not, wrap the MultiSelect in a simple container element (div or span) and set the data-testid on that wrapper, ensuring the wrapper includes the same className/behavior as needed and does not change existing event handling tied to selectedTools or handleUpdateMCPConfig.
🧹 Nitpick comments (2)
transports/bifrost-http/handlers/governance.go (1)
510-516: Consider adding weight validation for non-nil values.The direct assignment
Weight: pc.Weightcorrectly preserves nil vs value semantics. However, there's no validation that a non-nil weight is positive. Negative or zero weights could cause unexpected behavior in weighted routing.🔧 Optional: Add weight validation
+ // Validate weight if provided + if pc.Weight != nil && *pc.Weight <= 0 { + return fmt.Errorf("provider config weight must be positive: %.2f", *pc.Weight) + } + providerConfig := &configstoreTables.TableVirtualKeyProviderConfig{ VirtualKeyID: vk.ID, Provider: pc.Provider, Weight: pc.Weight,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 510 - 516, The code constructs a TableVirtualKeyProviderConfig with Weight: pc.Weight but does not validate that a non-nil pc.Weight is positive; add a validation step before creating providerConfig (e.g., in the surrounding function handling provider entries) that checks if pc.Weight != nil and *pc.Weight > 0, otherwise return or propagate an error (or set a default positive weight) to prevent zero/negative weights from being stored; reference the pc.Weight field, the providerConfig variable, and the TableVirtualKeyProviderConfig construction and enforce the check there.framework/configstore/migrations.go (1)
3722-3759: Consider removing per-VK COUNT queries to avoid migration N+1 overhead.Line 3725 and Line 3757 execute one COUNT per VK each pass. This can slow startup noticeably on larger datasets. Precompute configured VK IDs once per table and do in-memory membership checks.
♻️ Refactor sketch
- for _, vk := range allVKs { - var providerConfigCount int64 - if err := tx.Model(&tables.TableVirtualKeyProviderConfig{}).Where("virtual_key_id = ?", vk.ID).Count(&providerConfigCount).Error; err != nil { - return fmt.Errorf("failed to count provider configs for VK %s: %w", vk.ID, err) - } - if providerConfigCount == 0 && len(allProviders) > 0 { + var vkIDsWithProviderConfigs []string + if err := tx.Model(&tables.TableVirtualKeyProviderConfig{}). + Distinct("virtual_key_id"). + Pluck("virtual_key_id", &vkIDsWithProviderConfigs).Error; err != nil { + return fmt.Errorf("failed to fetch VKs with provider configs: %w", err) + } + hasProviderConfigs := make(map[string]struct{}, len(vkIDsWithProviderConfigs)) + for _, id := range vkIDsWithProviderConfigs { + hasProviderConfigs[id] = struct{}{} + } + + for _, vk := range allVKs { + if _, exists := hasProviderConfigs[vk.ID]; !exists && len(allProviders) > 0 { // backfill... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 3722 - 3759, The migration currently runs per-VK COUNT queries (tx.Model(&tables.TableVirtualKeyProviderConfig{}).Where(...).Count(...) and tx.Model(&tables.TableVirtualKeyMCPConfig{}).Where(...).Count(...)), causing an N+1; replace those with two bulk queries to precompute membership: query all TableVirtualKeyProviderConfig rows for the relevant virtual_key_ids and build a map/set of VK IDs that already have provider configs, and likewise query all TableVirtualKeyMCPConfig rows and build a map/set of VK IDs that have MCP configs; then iterate allVKs and use in-memory lookups on those sets to decide whether to create backfill entries (still using tx.Create for TableVirtualKeyProviderConfig and TableVirtualKeyMCPConfig), preserving modifiedVKIDs updates and the existing log.Printf calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugins/governance/main.go`:
- Around line 620-649: Filter out configs whose getWeight(config.Weight) <= 0
before doing the weighted selection: build a new slice (e.g.,
positiveWeightedConfigs) from weightedConfigs by keeping only entries with
strictly positive weight, compute totalWeight from that slice, run the random
draw over positiveWeightedConfigs, and use that same filtered slice when
applying the fallback instead of weightedConfigs; if positiveWeightedConfigs is
empty, skip the weighted branch entirely so the later non-weighted routing logic
executes.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 659-660: The provider delete icon (Trash2) inside the accordion
header must be converted to a real <button> element with an explicit aria-label
and keyboard focusability, call the existing handler
handleRemoveProvider(index), and stop event propagation (e.stopPropagation()) in
the click handler so the accordion does not toggle; likewise replace the MCP
delete icon-only button with a labeled button (aria-label) to provide screen
reader text. Locate the Trash2 usage and the MCP delete action in
virtualKeySheet.tsx and ensure both use semantic <button> elements, include
aria-labels, and call event.stopPropagation() before invoking
handleRemoveProvider or the MCP remove handler.
- Around line 171-212: The form hydration collapses zero-valued limits because
it uses truthy checks (e.g., config.rate_limit ? ... and
config.rate_limit.token_max_limit ? String(...) : undefined, and top-level
tokenMaxLimit/requestMaxLimit checks), which turn 0 into undefined; update the
checks in the providerConfigs mapping and the top-level virtual-key fields (the
resolver call that builds providerConfigs,
rate_limit/token_max_limit/token_reset_duration/request_max_limit/request_reset_duration,
and the top-level tokenMaxLimit/requestMaxLimit/budgetMaxLimit) to test
explicitly for null/undefined (e.g., rate_limit != null and token_max_limit !=
null) instead of truthiness so zero values are preserved when stringified and
rendered in the edit form.
- Around line 1005-1046: The MultiSelect control for MCP tools needs a stable
test hook; add a data-testid following the pattern (e.g.,
data-testid="mcp-tools-selector-row") so tests can target it like the weight
input and delete button. If the MultiSelect component forwards arbitrary props,
pass data-testid directly to the MultiSelect invocation in virtualKeySheet.tsx;
if it does not, wrap the MultiSelect in a simple container element (div or span)
and set the data-testid on that wrapper, ensuring the wrapper includes the same
className/behavior as needed and does not change existing event handling tied to
selectedTools or handleUpdateMCPConfig.
---
Nitpick comments:
In `@framework/configstore/migrations.go`:
- Around line 3722-3759: The migration currently runs per-VK COUNT queries
(tx.Model(&tables.TableVirtualKeyProviderConfig{}).Where(...).Count(...) and
tx.Model(&tables.TableVirtualKeyMCPConfig{}).Where(...).Count(...)), causing an
N+1; replace those with two bulk queries to precompute membership: query all
TableVirtualKeyProviderConfig rows for the relevant virtual_key_ids and build a
map/set of VK IDs that already have provider configs, and likewise query all
TableVirtualKeyMCPConfig rows and build a map/set of VK IDs that have MCP
configs; then iterate allVKs and use in-memory lookups on those sets to decide
whether to create backfill entries (still using tx.Create for
TableVirtualKeyProviderConfig and TableVirtualKeyMCPConfig), preserving
modifiedVKIDs updates and the existing log.Printf calls.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 510-516: The code constructs a TableVirtualKeyProviderConfig with
Weight: pc.Weight but does not validate that a non-nil pc.Weight is positive;
add a validation step before creating providerConfig (e.g., in the surrounding
function handling provider entries) that checks if pc.Weight != nil and
*pc.Weight > 0, otherwise return or propagate an error (or set a default
positive weight) to prevent zero/negative weights from being stored; reference
the pc.Weight field, the providerConfig variable, and the
TableVirtualKeyProviderConfig construction and enforce the check there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9b920c9-b065-454a-9126-c402ac1899e0
📒 Files selected for processing (28)
core/go.modcore/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/mcp.gocore/mcp/utils.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/numberAndSelect.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
🚧 Files skipped from review as they are similar to previous changes (10)
- core/internal/mcptests/fixtures.go
- docs/features/governance/mcp-tools.mdx
- docs/mcp/filtering.mdx
- transports/bifrost-http/handlers/mcpserver.go
- plugins/governance/resolver_test.go
- core/schemas/bifrost.go
- framework/configstore/tables/virtualkey.go
- docs/features/governance/routing.mdx
- transports/config.schema.json
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
00efab7 to
eae8f63
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
framework/configstore/clientconfig.go (1)
626-690:⚠️ Potential issue | 🟠 MajorAdd a final deterministic tie-breaker before hashing provider configs.
After provider, budget, rate limit, and weight compare equal, the sorter returns
falseeven whenAllowedModelsorKeyIDsdiffer. Becausesort.Sliceis not stable, the same logical virtual key can still hash differently depending on input order, which will churn reconciliation and migrations when those arrays are the only distinguishing fields.💡 Suggested fix
// Hash ProviderConfigs if len(vk.ProviderConfigs) > 0 { - // Copy and sort provider configs for deterministic hashing - sortedProviderConfigs := make([]tables.TableVirtualKeyProviderConfig, len(vk.ProviderConfigs)) - copy(sortedProviderConfigs, vk.ProviderConfigs) - sort.Slice(sortedProviderConfigs, func(i, j int) bool { - if sortedProviderConfigs[i].Provider != sortedProviderConfigs[j].Provider { - return sortedProviderConfigs[i].Provider < sortedProviderConfigs[j].Provider - } - bi, bj := "", "" - if sortedProviderConfigs[i].BudgetID != nil { - bi = *sortedProviderConfigs[i].BudgetID - } - if sortedProviderConfigs[j].BudgetID != nil { - bj = *sortedProviderConfigs[j].BudgetID - } - if bi != bj { - return bi < bj - } - ri, rj := "", "" - if sortedProviderConfigs[i].RateLimitID != nil { - ri = *sortedProviderConfigs[i].RateLimitID - } - if sortedProviderConfigs[j].RateLimitID != nil { - rj = *sortedProviderConfigs[j].RateLimitID - } - if ri != rj { - return ri < rj - } - wi, wj := sortedProviderConfigs[i].Weight, sortedProviderConfigs[j].Weight - if (wi == nil) != (wj == nil) { - return wi == nil - } - if wi != nil && wj != nil && *wi != *wj { - return *wi < *wj - } - return false - }) - // Filter out provider configs that are not available - providerConfigsForHash := make([]VirtualKeyProviderConfigHashInput, len(sortedProviderConfigs)) - for i, pc := range sortedProviderConfigs { + type sortableProviderConfig struct { + payload VirtualKeyProviderConfigHashInput + sortKey string + } + + sortableProviderConfigs := make([]sortableProviderConfig, len(vk.ProviderConfigs)) + for i, pc := range vk.ProviderConfigs { // Sort key IDs for deterministic hashing keyIDs := make([]string, len(pc.Keys)) for j, k := range pc.Keys { keyIDs[j] = k.KeyID } sort.Strings(keyIDs) @@ // Sort allowed models for deterministic hashing sortedAllowedModels := make([]string, len(pc.AllowedModels)) copy(sortedAllowedModels, pc.AllowedModels) sort.Strings(sortedAllowedModels) - providerConfigsForHash[i] = VirtualKeyProviderConfigHashInput{ + payload := VirtualKeyProviderConfigHashInput{ Provider: pc.Provider, Weight: pc.Weight, AllowedModels: sortedAllowedModels, BudgetID: pc.BudgetID, RateLimitID: pc.RateLimitID, KeyIDs: keyIDs, } + sortKey, err := sonic.Marshal(payload) + if err != nil { + return "", err + } + sortableProviderConfigs[i] = sortableProviderConfig{ + payload: payload, + sortKey: string(sortKey), + } } + + sort.Slice(sortableProviderConfigs, func(i, j int) bool { + return sortableProviderConfigs[i].sortKey < sortableProviderConfigs[j].sortKey + }) + + providerConfigsForHash := make([]VirtualKeyProviderConfigHashInput, len(sortableProviderConfigs)) + for i, item := range sortableProviderConfigs { + providerConfigsForHash[i] = item.payload + } + data, err := sonic.Marshal(providerConfigsForHash) if err != nil { return "", err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/clientconfig.go` around lines 626 - 690, The comparator used when sorting sortedProviderConfigs currently returns false when Provider, BudgetID, RateLimitID and Weight are equal, which can leave ordering non-deterministic; update the comparison function (the anonymous func passed to sort.Slice that compares sortedProviderConfigs[i] and [j]) to add a final deterministic tie-breaker that compares sorted AllowedModels (lexicographically: length then element-by-element) and if those are equal compares sorted KeyIDs (length then element-by-element); this ensures the order is stable before building providerConfigsForHash/VirtualKeyProviderConfigHashInput and prevents spurious hash churn.docs/features/governance/mcp-tools.mdx (1)
18-19:⚠️ Potential issue | 🟡 MinorResolve the remaining contradiction about empty tool lists.
Line 18-19 says deny-by-default, but Line 43 still says leaving tools blank allows all tools. Please align Line 43 with Line 29 and the implemented behavior.
✏️ Suggested doc fix
-4. Add the tools you want to restrict the VK to, or leave it blank to allow all tools for this client +4. Add the tools you want to allow for this client. Use `*` to allow all tools for this client; leave it blank to block all tools for this client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/governance/mcp-tools.mdx` around lines 18 - 19, The documentation contains a contradiction about empty MCP tool lists: the earlier statement "If a Virtual Key has no specific MCP configurations, no MCP tools are available (deny-by-default)" must be the source of truth; update the section that currently says "leaving tools blank allows all tools" to instead state that leaving the MCP tools list blank results in deny-by-default (no tools allowed) to match the implemented behavior and the earlier "Virtual Key" guidance; ensure any examples or the phrase "leave tools blank" and the heading that references MCP client configurations are changed to reflect deny-by-default consistently.
🤖 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/config.schema.json`:
- Around line 466-468: Update the transport JSON schema to match the new virtual
key contract: change the mcp_configs paragraph (symbol mcp_configs) to document
deny-by-default semantics the same as provider_configs (empty array blocks
access), and update the virtual_key_provider_config.weight schema (symbol
virtual_key_provider_config.weight) to allow null as well as number (make the
type accept null/number or set nullable true) so provider weights can opt out of
weighted routing. Ensure descriptions and allowed types mirror provider_configs
and the VK contract.
---
Duplicate comments:
In `@docs/features/governance/mcp-tools.mdx`:
- Around line 18-19: The documentation contains a contradiction about empty MCP
tool lists: the earlier statement "If a Virtual Key has no specific MCP
configurations, no MCP tools are available (deny-by-default)" must be the source
of truth; update the section that currently says "leaving tools blank allows all
tools" to instead state that leaving the MCP tools list blank results in
deny-by-default (no tools allowed) to match the implemented behavior and the
earlier "Virtual Key" guidance; ensure any examples or the phrase "leave tools
blank" and the heading that references MCP client configurations are changed to
reflect deny-by-default consistently.
In `@framework/configstore/clientconfig.go`:
- Around line 626-690: The comparator used when sorting sortedProviderConfigs
currently returns false when Provider, BudgetID, RateLimitID and Weight are
equal, which can leave ordering non-deterministic; update the comparison
function (the anonymous func passed to sort.Slice that compares
sortedProviderConfigs[i] and [j]) to add a final deterministic tie-breaker that
compares sorted AllowedModels (lexicographically: length then
element-by-element) and if those are equal compares sorted KeyIDs (length then
element-by-element); this ensures the order is stable before building
providerConfigsForHash/VirtualKeyProviderConfigHashInput and prevents spurious
hash churn.
🪄 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: 05cacf20-3f67-4ded-b8c1-815a2b9cc84c
📒 Files selected for processing (28)
core/go.modcore/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/mcp.gocore/mcp/utils.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/numberAndSelect.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
✅ Files skipped from review due to trivial changes (1)
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
- core/internal/mcptests/concurrency_advanced_test.go
- core/schemas/bifrost.go
- core/internal/mcptests/agent_test_helpers.go
- plugins/governance/resolver.go
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- docs/mcp/filtering.mdx
- ui/components/ui/numberAndSelect.tsx
- core/go.mod
- framework/configstore/tables/virtualkey.go
- docs/openapi/schemas/management/governance.yaml
- core/mcp/utils.go
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
eae8f63 to
9d2be84
Compare
9 tasks
Collaborator
Author
|
@coderabbitai full-review |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx (2)
94-96:⚠️ Potential issue | 🟠 MajorUpdate empty state text to reflect deny-by-default behavior.
The text "All providers allowed with default settings" contradicts the new deny-by-default semantics introduced in this PR. According to the PR summary, empty
provider_configsnow blocks access unless explicitly configured. This misleading text could confuse users about their virtual key's actual security posture.📝 Suggested fix to align with deny-by-default behavior
{!virtualKey.provider_configs || virtualKey.provider_configs.length === 0 ? ( - <span className="text-muted-foreground text-sm">All providers allowed with default settings</span> + <span className="text-muted-foreground text-sm">No providers configured (access denied)</span> ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx` around lines 94 - 96, The empty-state copy in virtualKeyDetailsSheet.tsx is misleading given the new deny-by-default semantics: change the text rendered when virtualKey.provider_configs is empty (the ternary branch checking virtualKey.provider_configs || length === 0) to clearly state that no providers are allowed unless explicitly configured (e.g., “No providers allowed — access is blocked unless provider_configs are added”), update the span text accordingly to reflect deny-by-default behavior, and ensure any accessible tooltip or help text used alongside virtualKey.provider_configs references the same deny-by-default wording so UI and logic remain consistent.
297-299:⚠️ Potential issue | 🟠 MajorUpdate MCP empty state text to reflect deny-by-default behavior.
Similar to the provider configs issue, the text "All MCP clients allowed with default settings" is inconsistent with the deny-by-default behavior. Empty
mcp_configsnow denies access rather than allowing all clients.📝 Suggested fix to align with deny-by-default behavior
{!virtualKey.mcp_configs || virtualKey.mcp_configs.length === 0 ? ( - <span className="text-muted-foreground text-sm">All MCP clients allowed with default settings</span> + <span className="text-muted-foreground text-sm">No MCP clients configured (access denied)</span> ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx` around lines 297 - 299, The empty-state message in virtualKeyDetailsSheet.tsx is incorrect for deny-by-default behavior; update the JSX branch that checks virtualKey.mcp_configs (the condition "!virtualKey.mcp_configs || virtualKey.mcp_configs.length === 0") to show a deny-by-default message (e.g., "No MCP clients allowed — access denied by default") instead of "All MCP clients allowed with default settings" so the UI reflects that empty mcp_configs denies access.
♻️ Duplicate comments (2)
core/go.mod (1)
20-20: 🛠️ Refactor suggestion | 🟠 MajorUnused dependency should be moved to the PR that imports it.
The
gorilla/websocketdependency added here remains unused in this PR's scope. As previously flagged, searches across the codebase show zero imports ofgithub.com/gorilla/websocket, whilefasthttp/websocket(line 18) is the active library.Per the coding guidelines: since this PR is part of a stack (
#2006,#2008,#2113,#1940,#1933,#1932), if this dependency is required for another PR in the stack, add it in the PR that actually introduces the import—not preemptively. This maintains clear dependency hygiene and makes each PR's changes self-contained.If the dependency is genuinely needed for this PR's virtual key/MCP configuration changes, please provide the usage context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/go.mod` at line 20, The go.mod change adds an unused dependency (github.com/gorilla/websocket) while the project currently uses fasthttp/websocket; remove the gorilla/websocket line from core/go.mod (or revert this addition) so the PR only contains dependencies it actually imports, and instead add that dependency in the specific PR that introduces imports for github.com/gorilla/websocket; ensure go.sum is updated by running `go mod tidy` after removing the entry.docs/features/governance/mcp-tools.mdx (1)
18-19:⚠️ Potential issue | 🟡 MinorResolve remaining contradiction with Line 43 guidance.
Lines 18–19 correctly state deny-by-default, but Line 43 still says leaving tools blank allows all tools. Please make that instruction consistent.
Suggested doc fix
-4. Add the tools you want to restrict the VK to, or leave it blank to allow all tools for this client +4. Add the tools you want to allow for this client. Use `*` to allow all tools for this client; leave it blank to block all tools for this client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/governance/mcp-tools.mdx` around lines 18 - 19, The doc currently contradicts itself: the "Virtual Key" deny-by-default statement (no MCP tools available unless client configurations exist) conflicts with the later guidance that "leaving tools blank allows all tools." Update the later guidance text to match deny-by-default by replacing the sentence that says "leaving tools blank allows all tools" with language that clarifies an empty/omitted tools list results in no tools being enabled (i.e., explicit MCP client configurations are required to allow tools); reference the terms "Virtual Key", "MCP configurations", and "MCP client configurations" when making the change so readers see the consistent policy across the section.
🧹 Nitpick comments (1)
ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx (1)
107-107: Handle null weight values in the display.Since the PR makes provider
weightoptional (nullable), this display should handle the null case. A null weight means the provider is excluded from weighted routing, but currently the UI would show "Weight: " with no value.🎨 Suggested enhancement for null weight handling
<Badge variant="outline" className="font-mono text-xs"> - Weight: {config.weight} + Weight: {config.weight ?? "N/A"} </Badge>Alternatively, to be more explicit about the routing exclusion:
<Badge variant="outline" className="font-mono text-xs"> - Weight: {config.weight} + {config.weight != null ? `Weight: ${config.weight}` : "Not used in weighted routing"} </Badge>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx` at line 107, In VirtualKeyDetailsSheet (where the JSX renders "Weight: {config.weight}"), guard against null/undefined config.weight and render a clear fallback (e.g., "n/a" or "Excluded from weighted routing") instead of an empty value; update the display logic around config.weight to use a conditional expression that shows the numeric weight when present and the chosen fallback string when it's nullish.
🤖 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 259-261: migrationBackfillEmptyVirtualKeyConfigs is running
full-model GORM queries (tx.Find(&allVKs), tx.Find(&allProviders),
tx.Find(&allMCPClients), Preload("ProviderConfigs.Keys")) which reference struct
fields added later in this file (e.g., EncryptionStatus, ValueHash,
PricingOverridesJSON, StoreRawRequestResponse, VLLMUrl, VLLMModelName,
BedrockRoleARN) and will fail on older schemas; fix by either moving the call to
migrationBackfillEmptyVirtualKeyConfigs to execute after
migrationAddStoreRawRequestResponseColumn, or change the migration's
read/preload queries to use migration-local lightweight structs or explicit
Select(...) clauses that only list existing columns (update the code inside
migrationBackfillEmptyVirtualKeyConfigs where tx.Find and
Preload("ProviderConfigs.Keys") are used to avoid selecting non-existent
fields).
In `@transports/bifrost-http/handlers/governance.go`:
- Line 902: The handler currently updates in DB (e.g., assigns existing.Weight =
pc.Weight) but ignores errors from GovernanceManager.ReloadVirtualKey, returning
success even if the in-memory reload fails; change the control flow so that
after the DB update you call GovernanceManager.ReloadVirtualKey (or whichever
ReloadVirtualKey method is used) and if it returns an error you propagate that
by writing an HTTP 500 response (and do not return success), ensuring the HTTP
handler returns failure when ReloadVirtualKey fails so DB and in-memory state
stay consistent; update the code paths around the existing.Weight = pc.Weight /
ReloadVirtualKey call sites (lines handling ReloadVirtualKey) to check the error
and respond 500 instead of continuing.
---
Outside diff comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx`:
- Around line 94-96: The empty-state copy in virtualKeyDetailsSheet.tsx is
misleading given the new deny-by-default semantics: change the text rendered
when virtualKey.provider_configs is empty (the ternary branch checking
virtualKey.provider_configs || length === 0) to clearly state that no providers
are allowed unless explicitly configured (e.g., “No providers allowed — access
is blocked unless provider_configs are added”), update the span text accordingly
to reflect deny-by-default behavior, and ensure any accessible tooltip or help
text used alongside virtualKey.provider_configs references the same
deny-by-default wording so UI and logic remain consistent.
- Around line 297-299: The empty-state message in virtualKeyDetailsSheet.tsx is
incorrect for deny-by-default behavior; update the JSX branch that checks
virtualKey.mcp_configs (the condition "!virtualKey.mcp_configs ||
virtualKey.mcp_configs.length === 0") to show a deny-by-default message (e.g.,
"No MCP clients allowed — access denied by default") instead of "All MCP clients
allowed with default settings" so the UI reflects that empty mcp_configs denies
access.
---
Duplicate comments:
In `@core/go.mod`:
- Line 20: The go.mod change adds an unused dependency
(github.com/gorilla/websocket) while the project currently uses
fasthttp/websocket; remove the gorilla/websocket line from core/go.mod (or
revert this addition) so the PR only contains dependencies it actually imports,
and instead add that dependency in the specific PR that introduces imports for
github.com/gorilla/websocket; ensure go.sum is updated by running `go mod tidy`
after removing the entry.
In `@docs/features/governance/mcp-tools.mdx`:
- Around line 18-19: The doc currently contradicts itself: the "Virtual Key"
deny-by-default statement (no MCP tools available unless client configurations
exist) conflicts with the later guidance that "leaving tools blank allows all
tools." Update the later guidance text to match deny-by-default by replacing the
sentence that says "leaving tools blank allows all tools" with language that
clarifies an empty/omitted tools list results in no tools being enabled (i.e.,
explicit MCP client configurations are required to allow tools); reference the
terms "Virtual Key", "MCP configurations", and "MCP client configurations" when
making the change so readers see the consistent policy across the section.
---
Nitpick comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx`:
- Line 107: In VirtualKeyDetailsSheet (where the JSX renders "Weight:
{config.weight}"), guard against null/undefined config.weight and render a clear
fallback (e.g., "n/a" or "Excluded from weighted routing") instead of an empty
value; update the display logic around config.weight to use a conditional
expression that shows the numeric weight when present and the chosen fallback
string when it's nullish.
🪄 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: be7a5428-0023-4afe-a8cd-40617b680915
📒 Files selected for processing (28)
core/go.modcore/internal/mcptests/agent_test_helpers.gocore/internal/mcptests/codemode_stdio_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/fixtures.gocore/mcp/mcp.gocore/mcp/utils.gocore/schemas/bifrost.godocs/features/governance/mcp-tools.mdxdocs/features/governance/routing.mdxdocs/mcp/filtering.mdxdocs/openapi/schemas/management/governance.yamldocs/providers/provider-routing.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcpserver.gotransports/config.schema.jsonui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/numberAndSelect.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (1)
- core/mcp/mcp.go
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
9d2be84 to
b4b716b
Compare
18 tasks
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
b4b716b to
387e9e5
Compare
Collaborator
Author
Merge activity
|
Pratham-Mishra04
deleted the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
18 tasks
akshaydeo
added a commit
that referenced
this pull request
Apr 18, 2026
* refactor: standardize empty array conventions for VK Provider & MCP Configs, and makes Provider Config weight optional for routing (#1932)
## Summary
Changes Virtual Key provider and MCP configurations from "allow-all by default" to "deny-by-default" security model. Virtual Keys now require explicit provider and MCP client configurations to allow access, improving security posture.
## Changes
- **Provider Configs**: Empty `provider_configs` now blocks all providers instead of allowing all
- **MCP Configs**: Empty `mcp_configs` now blocks all MCP tools instead of allowing all
- **Weight Field**: Changed provider `weight` from required `float64` to optional `*float64` - null weight excludes provider from weighted routing
- **Migration**: Added automatic backfill migration to preserve existing Virtual Key behavior by adding all available providers/MCP clients to VKs with empty configs
- **Documentation**: Updated all references to reflect new deny-by-default behavior
- **UI Updates**: Modified Virtual Key creation/editing interface to reflect new behavior and weight handling
## Type of change
- [x] Feature
- [x] Refactor
- [x] Documentation
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
Test Virtual Key creation and provider/MCP access:
```sh
# Core/Transports
go version
go test ./...
# Test Virtual Key with no provider configs blocks requests
curl -X POST http://localhost:8080/v1/chat/completions \
-H "Authorization: Bearer sk-bf-empty-vk" \
-H "Content-Type: application/json" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Should return error about no providers configured
# Test Virtual Key with provider configs allows requests
curl -X POST http://localhost:8080/v1/chat/completions \
-H "Authorization: Bearer sk-bf-configured-vk" \
-H "Content-Type: application/json" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Should work normally
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
## Breaking changes
- [x] Yes
**Impact**: Existing Virtual Keys with empty `provider_configs` or `mcp_configs` would be blocked after this change.
**Migration**: Automatic migration `migrationBackfillEmptyVirtualKeyConfigs` runs on startup to backfill existing Virtual Keys with all available providers/MCP clients, preserving current behavior. New Virtual Keys created after this change will use deny-by-default.
## Security considerations
This change significantly improves security posture by requiring explicit configuration of allowed providers and MCP tools for Virtual Keys. The automatic migration ensures no disruption to existing deployments while new Virtual Keys benefit from the more secure default behavior.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add MCP auto tool injection toggle (#1933)
## Summary
Adds a new configuration option `DisableAutoToolInject` to the MCP (Model Context Protocol) system that allows disabling automatic tool injection into requests. When enabled, MCP tools are only included when explicitly requested via context headers or filters, providing more granular control over tool availability.
## Changes
- Added `DisableAutoToolInject` field to `MCPToolManagerConfig` schema with runtime update support
- Implemented atomic boolean storage in `ToolsManager` to safely handle concurrent access
- Added logic in `ParseAndAddToolsToRequest` to respect the disable flag and only inject tools when explicit context filters are present
- Extended configuration management with database migration, UI controls, and API endpoints
- Added hot-reload capability through `UpdateMCPDisableAutoToolInject` methods across the stack
- Updated UI with a toggle switch and clear documentation about the feature's behavior
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Validate the new MCP auto tool injection toggle:
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Test the feature:
1. Configure MCP clients and tools
2. Enable "Disable Auto Tool Injection" in the MCP configuration UI
3. Make requests without explicit tool headers - tools should not be injected
4. Make requests with `x-bf-mcp-include-tools` header - tools should be injected
5. Verify hot-reload works by toggling the setting without server restart
## Screenshots/Recordings
UI changes include a new toggle switch in the MCP configuration view with descriptive text explaining when tools are injected based on explicit headers.
## Breaking changes
- [ ] Yes
- [x] No
This is a backward-compatible addition with a default value of `false` (auto injection enabled).
## Related issues
This addresses the need for more granular control over MCP tool injection behavior in request processing.
## Security considerations
The feature provides better control over tool exposure by allowing administrators to require explicit opt-in for tool injection, potentially reducing unintended tool access.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: VK MCP config now works as an AllowList (#1940)
## Summary
This PR implements MCP tool governance by enforcing virtual key MCP configurations as an execution-time allow-list. When virtual keys have empty MCPConfigs, all MCP tools are denied. When non-empty, each tool is validated against the configured allow-list at both inference time and MCP tool execution.
## Changes
- **Context parameter updates**: Changed MCP-related functions to use `*schemas.BifrostContext` instead of `context.Context` to enable tool tracking
- **Tool tracking**: Added `BifrostContextKeyMCPAddedTools` context key to track which MCP tools are added to requests
- **Governance enforcement**: Virtual key MCP configurations now act as execution-time allow-lists with validation in both `PreMCPHook` and `evaluateGovernanceRequest`
- **Auto-injection control**: Added `DisableAutoToolInject` configuration option that respects the toggle and skips auto-injection when headers are already set by callers
- **Decision type**: Added `DecisionMCPToolBlocked` for MCP tool governance violations
- **UI improvements**: Updated MCP view description and sidebar item naming for better clarity
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Test MCP tool governance with virtual keys:
```sh
# Core/Transports
go version
go test ./...
# Test with virtual key having empty MCPConfigs (should deny all MCP tools)
curl -X POST /v1/chat/completions \
-H "x-bf-virtual-key: test-vk-empty-mcp" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Test with virtual key having specific MCP tool allowlist
curl -X POST /v1/chat/completions \
-H "x-bf-virtual-key: test-vk-with-mcp" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Test disable auto tool inject configuration
curl -X PUT /v1/config/mcp/disable-auto-tool-inject \
-d '{"disable": true}'
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
New configuration options:
- `disable_auto_tool_inject`: Boolean flag to disable automatic MCP tool injection
- Virtual key `MCPConfigs`: Array of MCP client configurations that act as allow-lists
## Screenshots/Recordings
UI changes include updated MCP configuration view with clearer descriptions for the disable auto tool injection toggle and improved sidebar navigation labels.
## Breaking changes
- [x] Yes
- [ ] No
**Impact**: MCP-related function signatures now require `*schemas.BifrostContext` instead of `context.Context`. Virtual keys with empty MCPConfigs will now deny all MCP tools by default.
**Migration**: Update any custom MCP integrations to use the new context parameter type. Configure MCPConfigs on virtual keys that need MCP tool access.
## Related issues
Implements MCP tool governance and execution-time validation for virtual key configurations.
## Security considerations
- **Access control**: Virtual key MCP configurations now enforce strict allow-lists for tool execution
- **Context isolation**: Tool tracking is isolated per request context to prevent cross-request leakage
- **Validation**: Both pre-execution and execution-time validation prevent unauthorized tool access
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: standardize empty array conventions for VK Provider Config Allowed Keys (#2006)
## Summary
Migrates VK provider config allowed keys from implicit allow-all semantics to explicit deny-by-default behavior. Adds `AllowAllKeys` boolean field to enable granular key access control while maintaining backward compatibility.
## Changes
- Added `AllowAllKeys` boolean field to `TableVirtualKeyProviderConfig` with database migration
- Backfilled existing configs with `allow_all_keys=true` to preserve current behavior
- Updated key resolution logic: empty keys now denies all access, `["*"]` wildcard allows all keys
- Modified governance resolver to set empty `includeOnlyKeys` slice when no keys are configured
- Enhanced HTTP handlers to recognize `["*"]` wildcard and set `AllowAllKeys` flag appropriately
- Updated UI to display "Allow All Keys" option and show deny-by-default messaging
- Added JSON unmarshaling support for `["*"]` wildcard in config files
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Validate the migration and new key access control behavior:
```sh
# Core/Transports
go version
go test ./...
# Test migration runs successfully
go run main.go migrate
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Test scenarios:
1. Create VK with empty `key_ids` - should deny all keys
2. Create VK with `key_ids: ["*"]` - should allow all keys
3. Create VK with specific key IDs - should allow only those keys
4. Verify existing VKs maintain their current behavior after migration
## Screenshots/Recordings
UI now shows:
- "Allow All Keys" option in key selection dropdown
- "No keys allowed" vs "All keys allowed" status indicators
- "No providers configured (deny-by-default)" messaging
## Breaking changes
- [ ] Yes
- [x] No
The migration preserves existing behavior by setting `allow_all_keys=true` for configs that previously had no keys specified.
## Related issues
Part of VK access control enhancement initiative.
## Security considerations
Improves security posture by implementing deny-by-default semantics for key access. Existing deployments maintain current access patterns through automatic backfill migration.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: standardize empty array conventions for allowed models (#2113)
## Summary
Standardizes empty array conventions across Bifrost to implement deny-by-default security semantics. Previously, empty arrays for `allowed_models` and `Models` fields meant "allow all", creating potential security gaps. Now `["*"]` explicitly means "allow all" while empty arrays mean "deny all".
## Changes
- **Core Logic**: Updated model filtering in `bifrost.go` and `selectKeyFromProviderForModel` to treat empty `Models` arrays as deny-all and `["*"]` as allow-all
- **Database Migration**: Added `migrationBackfillAllowedModelsWildcard` to convert existing empty arrays to `["*"]` preserving current behavior for existing records
- **Model Catalog**: Updated `IsModelAllowedForProvider` to use wildcard semantics with deny-by-default fallback
- **Schema Defaults**: Changed default `Models` value from `[]` to `["*"]` in table definitions and form schemas
- **UI Components**: Enhanced `ModelMultiselect` with `allowAllOption` prop and updated virtual key forms to handle wildcard selection
- **Documentation**: Updated JSON schemas, comments, and tooltips to reflect new conventions
- **Governance**: Updated provider config filtering logic to use new wildcard semantics
- **Server Bootstrap**: Added wildcard filtering when loading models to prevent literal "*" from appearing as a model name
## Type of change
- [x] Refactor
- [ ] Bug fix
- [ ] Feature
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
Validate the migration and new semantics:
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Test scenarios:
1. Create new virtual keys - should default to `["*"]` for allowed models
2. Create new provider keys - should default to `["*"]` for models
3. Verify existing keys with empty arrays are migrated to `["*"]`
4. Test that empty arrays now deny all models/keys as expected
5. Verify UI shows "All models allowed" for wildcard and "No models (deny all)" for empty arrays
## Screenshots/Recordings
UI changes include:
- Model multiselect now shows "Allow All Models" option
- Virtual key details display "All Models" badge for wildcard vs "No models (deny all)" for empty
- Form placeholders updated to reflect new semantics
## Breaking changes
- [x] Yes
- [ ] No
**Migration Impact**: The database migration automatically converts existing empty `allowed_models` and `models_json` arrays to `["*"]`, preserving current behavior. However, any new configurations with empty arrays will now deny access instead of allowing all. Applications relying on "empty = allow all" semantics must be updated to use `["*"]` explicitly.
## Related issues
Part of security hardening initiative to implement explicit allow-lists and deny-by-default semantics across Bifrost configuration.
## Security considerations
This change significantly improves security posture by:
- Eliminating ambiguous "empty means allow all" semantics
- Implementing explicit deny-by-default for new configurations
- Requiring intentional wildcard usage via `["*"]` for broad access
- Maintaining backward compatibility through automatic migration
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: replace string slices with WhiteList for allowlist fields (#2125)
## Summary
Introduces a new `WhiteList` type to standardize whitelist behavior across the codebase, replacing manual slice operations and string comparisons with semantic methods for handling allow/deny lists.
## Changes
- Added `WhiteList` type with methods `IsAllowed()`, `IsUnrestricted()`, `IsEmpty()`, `Contains()`, and `Validate()`
- Replaced `[]string` fields with `WhiteList` for model restrictions, tool filtering, and key access controls
- Updated all whitelist logic to use semantic methods instead of manual `slices.Contains()` checks
- Added validation to ensure wildcards ("*") aren't mixed with specific values and prevent duplicates
- Improved case-insensitive matching for whitelist comparisons
## Type of change
- [x] Refactor
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Plugins
## How to test
Verify that whitelist behavior remains consistent across all affected components:
```sh
# Core/Transports
go version
go test ./...
# Test specific whitelist scenarios:
# - Empty lists deny all access
# - ["*"] allows all access
# - Specific lists only allow listed items
# - Mixed wildcards and specific items are rejected
# - Duplicate entries are rejected
```
Test key model filtering, MCP tool execution, and virtual key configurations to ensure whitelist logic works correctly.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
The `WhiteList` type maintains the same JSON serialization format as `[]string`, so existing configurations remain compatible.
## Related issues
N/A
## Security considerations
Improves security by standardizing deny-by-default behavior and adding validation to prevent misconfigured whitelists that could inadvertently grant excessive permissions.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add request-level extra headers support for MCP tool execution (#2126)
## Summary
This PR adds support for request-level extra headers in MCP tool execution, allowing callers to forward specific headers to MCP servers at runtime based on a per-client allowlist configuration.
## Changes
- Added `AllowedExtraHeaders` field to MCP client configuration with allowlist semantics (empty array = deny all, `["*"]` = allow all)
- Introduced `BifrostContextKeyMCPExtraHeaders` context key to track headers forwarded to MCP tools
- Created `core/mcp/utils` package with `GetHeadersForToolExecution` function to merge static and dynamic headers
- Updated MCP tool execution in both regular tool manager and Starlark code mode to use the new header forwarding system
- Added database migration for `allowed_extra_headers_json` column in MCP client table
- Updated UI to include allowed extra headers configuration in MCP client management
- Enhanced auth demo server example to demonstrate tool-execution level authentication patterns
## Type of change
- [x] Feature
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] UI (Next.js)
## How to test
1. Configure an MCP client with allowed extra headers:
```json
{
"name": "test-client",
"connection_string": "http://localhost:3002/",
"auth_type": "headers",
"headers": {
"X-API-Key": "connection-secret"
},
"allowed_extra_headers": ["X-Tool-Token"],
"tools_to_execute": ["*"]
}
```
2. Make requests with extra headers that should be forwarded:
```bash
curl -X POST http://localhost:8080/v1/chat/completions \
-H "Authorization: Bearer your-key" \
-H "X-Tool-Token: tool-execution-secret" \
-d '{
"model": "gpt-4",
"messages": [{"role": "user", "content": "Use the secret_data tool"}],
"tools": [{"type": "function", "function": {"name": "secret_data"}}]
}'
```
3. Test the auth demo server:
```bash
cd examples/mcps/auth-demo-server
go run main.go
# Server demonstrates two-tier auth: connection-level (X-API-Key) and tool-level (X-Tool-Token)
```
4. Run tests:
```sh
go test ./core/mcp/...
go test ./transports/bifrost-http/...
cd ui
pnpm test
pnpm build
```
## Breaking changes
- [ ] Yes
- [x] No
This is a backward-compatible addition. Existing MCP clients will have empty `allowed_extra_headers` (deny all extra headers) which maintains current behavior.
## Security considerations
- Extra headers are filtered through a strict allowlist per MCP client
- Security denylist prevents auth header overrides via extra headers
- Two-tier authentication pattern demonstrated: connection-level + tool-execution level
- Headers are only forwarded to MCP servers that explicitly allow them
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: apply MCP tool filtering headers to tools/list response when using bifrost as MCP gateway (#2127)
## Summary
Adds support for `x-bf-mcp-include-clients` and `x-bf-mcp-include-tools` request headers to filter MCP tools/list response when using Bifrost as an MCP gateway. This ensures that tool filtering is respected at the MCP protocol level, not just during inference.
## Changes
- Implemented dynamic tool filtering in MCP server handlers that respects per-request include headers
- Added `makeIncludeClientsFilter()` function that filters tools based on request context values
- Registered the tool filter on both global and virtual key MCP servers during initialization
- Updated documentation to clarify that `mcp-include-tools` requires `clientName-toolName` format
- Enhanced examples in documentation to show proper tool naming format
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [x] Docs
## How to test
Test MCP gateway functionality with tool filtering:
```sh
# Test tools/list filtering with include-tools header
curl --location 'http://localhost:8080/mcp/tools/list' \
--header 'x-bf-mcp-include-tools: gmail-send_email,filesystem-read_file' \
--header 'Authorization: Bearer your-vk-here'
# Test tools/list filtering with include-clients header
curl --location 'http://localhost:8080/mcp/tools/list' \
--header 'x-bf-mcp-include-clients: gmail,filesystem' \
--header 'Authorization: Bearer your-vk-here'
# Verify chat completions still respect the same headers
curl --location 'http://localhost:8080/v1/chat/completions' \
--header 'x-bf-mcp-include-tools: gmail-send_email' \
--header 'Content-Type: application/json' \
--data '{
"model": "openai/gpt-4o-mini",
"messages": [{"role": "user", "content": "What tools are available?"}]
}'
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
The tool filtering mechanism ensures that virtual key restrictions are properly enforced at the MCP protocol level, preventing unauthorized access to tools that should be filtered out based on request headers.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: parallelize model listing for providers to speed up startup time (#2151)
## Summary
Parallelizes model listing operations for providers during server startup and provider reloading to significantly reduce initialization time. Previously, model listing was performed sequentially for each provider, causing slower startup times especially when multiple providers were configured.
## Changes
- Added concurrent execution using goroutines and sync.WaitGroup for model listing operations in three key functions: `ReloadProvider`, `ForceReloadPricing`, and `Bootstrap`
- In `ReloadProvider`, both filtered and unfiltered model listing requests now run concurrently for the same provider
- In `ForceReloadPricing` and `Bootstrap`, model listing for different providers now runs in parallel instead of sequentially
- Moved provider key retrieval earlier in `ReloadProvider` to ensure it happens before concurrent model listing
- Added proper context cancellation with defer statements for bifrost contexts
## Type of change
- [x] Refactor
## Affected areas
- [x] Transports (HTTP)
## How to test
Test server startup time with multiple providers configured to verify the performance improvement:
```sh
# Core/Transports
go version
go test ./...
# Test with multiple providers configured
# Measure startup time before and after the change
time go run main.go
```
Configure multiple providers in your bifrost configuration and observe faster startup times, especially noticeable when providers have high latency or many models.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications. The change maintains the same authentication and authorization patterns while improving performance through parallelization.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: reorder migrations and set AllowAllKeys to true for virtual key provider configs (#2158)
## Summary
Fixes database migration ordering issue and ensures virtual key configurations are properly initialized with the AllowAllKeys field set to true.
## Changes
- Reordered database migrations to execute `migrationAddAllowAllKeysToProviderConfig` before `migrationBackfillEmptyVirtualKeyConfigs` to ensure the AllowAllKeys column exists before backfilling
- Added `AllowAllKeys: true` to provider configurations created during virtual key backfill migration to enable unrestricted key access by default
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that database migrations run successfully and virtual key configurations are created with proper defaults:
```sh
# Core/Transports
go version
go test ./...
```
Test migration ordering by running against a fresh database to ensure no column reference errors occur.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
This change enables unrestricted key access by default for virtual key configurations, which may have security implications depending on the intended access control model.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: implement scoped pricing override
* refactor: custom pricing refactor
* fix: resolve merge conflicts in config loading and governance functions (#2230)
## Summary
Resolves Git merge conflicts in the bifrost-http configuration loading code by cleaning up duplicate function definitions and consolidating the configuration initialization flow.
## Changes
- Removed Git merge conflict markers and duplicate code blocks from `LoadConfig` function
- Consolidated governance configuration loading by keeping both `loadGovernanceConfigFromFile` and `loadGovernanceConfig` functions with distinct purposes
- Removed duplicate `convertSchemasMCPClientConfigToTable` function definition
- Moved pricing overrides initialization logic to `initFrameworkConfig` function for better organization
- Cleaned up extensive duplicate default configuration loading code that was causing merge conflicts
- Changed error handling for pricing overrides from returning error to logging warning
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that configuration loading works correctly without merge conflicts:
```sh
# Core/Transports
go version
go test ./...
go build ./transports/bifrost-http/...
```
Test configuration loading with various scenarios:
- Config file present
- Config file absent (default loading)
- Store-based configuration
- Governance and MCP configuration loading
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this is a merge conflict resolution that maintains existing functionality.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add Stability AI model support for Bedrock image generation (#2180)
## Summary
Adds support for Stability AI image generation models (stability.stable-image-*) to the Bedrock provider, enabling text-to-image generation with models like stability.stable-image-core-v1:1 and stability.stable-image-ultra-v1:1.
## Changes
- Added `isStabilityAIModel()` function to detect Stability AI models by "stability." prefix
- Created `ToStabilityAIImageGenerationRequest()` to convert Bifrost requests to Stability AI's flat request format
- Implemented `StabilityAIImageGenerationRequest` type with support for prompt, mode, aspect_ratio, output_format, seed, and negative_prompt parameters
- Added conditional routing in `ImageGeneration()` to use Stability AI request format when appropriate
- Extended known fields for image generation parameters to include "aspect_ratio" and "input_images"
- Updated documentation comment to reflect Stability AI model support
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test Stability AI image generation through the Bedrock provider:
```sh
# Core/Transports
go version
go test ./...
# Test with a Stability AI model
curl -X POST http://localhost:8080/v1/images/generations \
-H "Content-Type: application/json" \
-H "Authorization: Bearer your-key" \
-d '{
"model": "stability.stable-image-core-v1:1",
"prompt": "A beautiful sunset over mountains",
"aspect_ratio": "16:9",
"output_format": "PNG"
}'
```
Ensure AWS credentials are configured for Bedrock access and the Stability AI models are available in your region.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No additional security implications beyond existing Bedrock provider authentication and AWS credential handling.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add Stability AI image edit models support to Bedrock provider (#2225)
## Summary
Adds support for Stability AI image editing models in the Bedrock provider, expanding image editing capabilities beyond the existing Titan and Nova Canvas models.
## Changes
- Added `getStabilityAIEditTaskType()` function to infer edit task types from Stability AI model names (inpaint, outpaint, recolor, search-replace, erase-object, remove-bg, control-sketch, control-structure, style-guide, style-transfer, upscale-creative, upscale-conservative, upscale-fast)
- Created `ToStabilityAIImageEditRequest()` function to convert Bifrost requests to Stability AI's flat JSON format, with task-specific field validation
- Added `StabilityAIImageEditRequest` struct with comprehensive field support for all Stability AI edit operations
- Enhanced `BedrockImageGenerationResponse` with Seeds and FinishReasons fields for Stability AI compatibility
- Modified `ImageEdit()` method to route requests to appropriate conversion function based on model type
- Updated documentation to reflect expanded model support
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test with various Stability AI edit models through the Bedrock provider:
```sh
# Core/Transports
go version
go test ./...
# Test image editing with Stability AI models
# Example: stable-image-inpaint, stable-outpaint, stable-creative-upscale, etc.
```
Verify that task-specific parameters are correctly mapped and invalid fields are filtered out based on the detected task type.
## Screenshots/Recordings
N/A - Backend functionality only
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
Image data is handled as base64-encoded strings. Mask and image parameters are properly validated before processing.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: send back accumulated usage in MCP agent mode (#2246)
## Summary
This PR fixes token usage tracking in MCP agent mode by accumulating usage across all LLM calls in the agent loop and returning the total usage in the final response.
## Changes
- Added usage accumulation logic in the MCP agent execution loop to track token consumption across multiple LLM calls
- Implemented `mergeUsage` function to combine token counts and costs from multiple `BifrostLLMUsage` values, handling all detail sub-fields including prompt tokens, completion tokens, and cost breakdowns
- Extended agent API adapters with `extractUsage` and `applyUsage` methods to handle usage extraction and application for both Chat API and Responses API
- Applied accumulated usage to the final response before returning it to the client
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test MCP agent mode with multiple tool calls to verify usage accumulation:
```sh
# Core/Transports
go version
go test ./...
# Test MCP agent mode with multiple LLM calls
# Verify that the returned usage reflects the sum of all calls in the agent loop
# Check that both token counts and cost details are properly accumulated
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this change only affects usage tracking and reporting.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* [codemode]: fixing string escape corruption, enable top-level control flow in starlark, refining the prompt of executecode tool (#2206)
## Changes
- **Enhanced Starlark dialect configuration**: Enabled top-level control flow statements (if/for/while), while loops, set() builtin, global variable reassignment, and recursive functions for a more Python-like experience
- **Improved string escape handling**: Removed automatic `\n` to newline conversion, allowing Starlark's native string escape processing to handle `\n`, `\t`, and other escape sequences correctly
- **Updated tool description**: Streamlined the executeToolCode tool description with clearer syntax notes, explicit documentation of Starlark differences from Python (no try/except, no classes, no imports, no f-strings), and emphasis on fresh isolated scope per execution
- **Enhanced error hints**: Added specific error messages for unsupported Python features like try/except/finally/raise, with guidance on alternative approaches and scope persistence warnings
- **Comprehensive test coverage**: Added tests for dialect options, string escape preservation, unsupported feature detection, and end-to-end JSON deserialization scenarios
## Type of change
- [ ] Feature
- [ ] Bug fix
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go) - Starlark CodeMode improvements
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test the enhanced Starlark features with MCP CodeMode:
```sh
# Test dialect options (top-level control flow, while loops, etc.)
make test-mcp TESTCASE=TestStarlarkDialectOptions
# Test string escape handling
make test-mcp PATTERN=TestStarlarkStringEscape
# Test unsupported feature detection
make test-mcp PATTERN=TestStarlarkUnsupportedFeatures
```
## Breaking changes
- [ ] Yes
- [x] No
The Starlark changes are additive and maintain backward compatibility while enabling more Python-like syntax.
## Security considerations
Starlark CodeMode maintains its existing sandboxing with no additional network or filesystem access. The dialect enhancements only affect language features within the existing security boundary.
* logging in plugins (#2215)
## Summary
Reorders middleware initialization in the Bifrost HTTP server to ensure tracing middleware is added before transport interceptor middleware in the inference pipeline.
## Changes
- Moved tracing middleware initialization and setup earlier in the bootstrap process
- Reordered middleware registration so tracing middleware is prepended before transport interceptor middleware
- Updated comments to clarify the middleware ordering logic and rationale
The change ensures that tracing context and trace IDs are properly established before other middleware components process requests.
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that tracing middleware executes before transport interceptor middleware by checking trace logs and middleware execution order.
```sh
# Core/Transports
go version
go test ./...
```
Test with tracing enabled to ensure trace IDs are properly set in context before subsequent middleware processing.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this is a middleware ordering change that affects observability components.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* fix: handling text, vtt, srt response format for transcriptions (#2102)
* feat: add virtual key access management for MCP clients (#2255)
## Summary
Adds virtual key access management to MCP client configuration, allowing administrators to control which virtual keys can access specific MCP servers and which tools they can execute on a per-VK basis.
## Changes
- Added `vk_configs` field to MCP client update API that accepts an array of virtual key configurations
- Each VK config specifies a virtual key ID and the tools it's allowed to execute on that MCP server
- When `vk_configs` is provided, it atomically replaces all existing VK assignments for the MCP client
- Added database method `GetVirtualKeyMCPConfigsByMCPClientID` to retrieve VK configs by MCP client
- Updated OpenAPI documentation to describe the new VK configuration functionality
- Enhanced UI with virtual key access management section in the MCP client sheet
- Added Go SDK context keys for MCP tool filtering: `MCPContextKeyIncludeClients`, `MCPContextKeyIncludeTools`, and `BifrostContextKeyMCPExtraHeaders`
- Updated context keys documentation with comprehensive MCP configuration examples
## Type of change
- [x] Feature
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] UI (Next.js)
- [x] Docs
## How to test
1. Create an MCP client with tools available
2. Create virtual keys in the system
3. Update the MCP client with VK configurations:
```sh
curl -X PUT /api/mcp/client/{id} \
-H "Content-Type: application/json" \
-d '{
"name": "test-client",
"vk_configs": [
{
"virtual_key_id": "vk-123",
"tools_to_execute": ["*"]
},
{
"virtual_key_id": "vk-456",
"tools_to_execute": ["read_file", "write_file"]
}
]
}'
```
4. Verify VK assignments are created/updated in the database
5. Test the UI by opening an MCP client sheet and managing virtual key access
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
## Screenshots/Recordings
The UI now includes a "Virtual Key Access" section in the MCP client configuration sheet where administrators can:
- Add virtual keys to grant access to the MCP server
- Configure which specific tools each virtual key can execute
- Remove virtual key access entirely
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
This enables fine-grained access control for MCP servers at the virtual key level, complementing the existing governance and budgeting features.
## Security considerations
- VK access controls are enforced through the governance plugin during MCP tool execution
- The atomic replacement of VK assignments prevents partial updates that could leave the system in an inconsistent state
- Tool-level restrictions allow principle of least privilege by limiting which MCP tools each virtual key can access
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: adds option to allow MCP clients to run on all virtual keys (#2258)
## Summary
Adds a new `AllowOnAllVirtualKeys` configuration option for MCP clients that enables them to be accessible to all virtual keys without requiring explicit per-key assignment. When enabled, all tools from the MCP client are available to every virtual key.
## Changes
- Added `AllowOnAllVirtualKeys` boolean field to `MCPClientConfig` schema and database table
- Updated MCP client manager to handle the new field during client updates
- Modified governance plugin to check for clients with `AllowOnAllVirtualKeys` enabled and automatically include their tools for all virtual keys
- Added database migration to add the new column to `TableMCPClient`
- Updated UI to include a toggle for the new setting with tooltip explanation
- Added OpenAPI documentation for the new field
- Updated configuration store methods to persist and retrieve the new field
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
1. Create or update an MCP client with `allow_on_all_virtual_keys: true`
2. Verify that the client's tools are available to all virtual keys without explicit assignment
3. Test that the governance plugin correctly allows tools from such clients
4. Verify the UI toggle works correctly in the MCP client edit sheet
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
The new configuration field `allow_on_all_virtual_keys` defaults to `false` to maintain backward compatibility.
## Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
## Breaking changes
- [ ] Yes
- [x] No
This is a backward-compatible addition with the new field defaulting to `false`.
## Related issues
Link related issues and discussions. Example: Closes #123
## Security considerations
This feature reduces access control granularity by allowing MCP clients to bypass virtual key restrictions when enabled. Administrators should carefully consider which MCP clients should have this permission as it grants broad access across all virtual keys.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add provider keys CRUD to configstore and in-memory store (#2159)
## Summary
Adds dedicated CRUD operations for individual provider keys at the data layer
(configstore interface + RDB implementation) and in-memory store. This enables
key-level operations without replacing the entire provider key set, which is
required for the new `/api/providers/{provider}/keys/*` endpoints.
## Changes
- Added `GetProviderKeys`, `GetProviderKey`, `CreateProviderKey`,
`UpdateProviderKey`, `DeleteProviderKey` to `ConfigStore` interface
- Implemented all five methods in `RDBConfigStore` with proper GORM queries,
error handling, and `ErrNotFound` propagation
- Extracted `schemaKeyFromTableKey` and `tableKeyFromSchemaKey` helpers to
deduplicate key conversion logic (previously inlined in `GetProvidersConfig`
and `GetProviderConfig`)
- Added `AddProviderKey`, `UpdateProviderKey`, `RemoveProviderKey` to in-memory
`Config` with mutex locking, DB persistence, and rollback on client update
failure
- Added `GetProviderKeysRaw`, `GetProviderKeysRedacted`, `GetProviderKeyRaw`,
`GetProviderKeyRedacted` read methods
- Added `TestProviderKeyCRUD` and `TestProviderKeyCRUD_ProviderMustExist`
integration tests
- Updated `MockConfigStore` with all five new interface methods
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
# Run configstore tests
go test ./framework/configstore/... -v -run TestProviderKeyCRUD
# Run config tests (mock store)
go test ./transports/bifrost-http/lib/... -v
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
Key values are handled through existing redaction infrastructure. No new secret
exposure paths introduced.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: add provider keys HTTP handlers and refactor optional keys (#2160)
## Summary
Adds HTTP handlers for the dedicated provider keys CRUD endpoints and removes
`keys` from provider API responses and payloads. Keys are now exclusively
managed via `/api/providers/{provider}/keys/*`. Also fixes a context timeout bug
in `ReloadProvider` where model discovery could exhaust the shared context
budget, causing subsequent DB calls to fail.
## Changes
### Provider keys handlers (`provider_keys.go`)
- New file with five handlers: `listProviderKeys`, `getProviderKey`,
`createProviderKey`, `updateProviderKey`, `deleteProviderKey`
- Includes `mergeUpdatedKey` (redacted value preservation logic used by
`updateProviderKey`)
- Key handlers enforce keyless provider validation and trigger model discovery
after mutations
### Provider handlers cleanup (`providers.go`)
- Registered new key routes: `GET/POST /api/providers/{provider}/keys`,
`GET/PUT/DELETE /api/providers/{provider}/keys/{key_id}`
- Extracted inline anonymous structs into named `providerCreatePayload` and
`providerUpdatePayload` types (without `Keys` field)
- Removed `Keys` field from `ProviderResponse`
- Switched `addProvider` from `json.Unmarshal` to `sonic.Unmarshal`
- Removed `oldConfigRedacted` fetch and the entire key merge block
(`mergeKeys`, `hasKeys`, `slices` usage) from `updateProvider`
- Removed `Keys` from `getProviderResponseFromConfig` response builder
- Removed unused `encoding/json` import
### Context timeout fix (`server.go`)
- Split shared `bfCtx` in `ReloadProvider` into separate contexts:
`filteredBfCtx` (15s) for filtered `ListModelsRequest` and `unfilteredBfCtx`
(fresh 15s) for unfiltered `ListModelsRequest`, each cancelled after use
- Changed `GetKeysByProvider` to use `context.Background()` since it's a local
DB call that shouldn't be gated by model discovery timeouts
- Added `hasNoKeys` check to emit warn-level logs instead of errors when model
discovery fails because no keys are configured
- Read in-memory key count via `GetProviderKeysRaw` for the `hasNoKeys` check
### Tests (`providers_test.go`)
- Cleared file (contained only tests for removed inline struct decoding)
## Type of change
- [x] Feature
- [x] Bug fix
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
# Build
go build ./transports/bifrost-http/...
# Manual: start Bifrost, then test key CRUD
curl -X POST localhost:8080/api/providers/openai/keys -d '{"name":"test-key","value":"sk-test"}'
curl localhost:8080/api/providers/openai/keys
curl -X PUT localhost:8080/api/providers/openai/keys/{key_id} -d '{"name":"updated","value":"sk-new"}'
curl -X DELETE localhost:8080/api/providers/openai/keys/{key_id}
# Verify provider endpoints no longer return keys
curl localhost:8080/api/providers/openai | jq 'has("keys")' # should be false
```
## Screenshots/Recordings
N/A
## Breaking changes
- [x] Yes
- [ ] No
Provider API responses no longer include `keys` field. Provider create/update
payloads no longer accept `keys`. Clients must use the dedicated
`/api/providers/{provider}/keys/*` endpoints for key management.
## Related issues
N/A
## Security considerations
- Key handlers use existing redaction infrastructure (`GetProviderKeyRedacted`)
before returning responses
- Keyless provider validation prevents key creation on providers that don't
support keys
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: migrate frontend to dedicated provider keys API (#2161)
## Summary
Migrates the frontend from reading provider keys via `provider.keys` (removed
from provider API response in PR #2160) to the dedicated `getProviderKeys`
query and `/api/keys` endpoint. Removes `keys` from all provider TypeScript
types. Key mutations patch caches from authoritative server responses; provider
updates invalidate the `ProviderKeys` tag to refresh key statuses after model
discovery. Also adds a read-only routing rule info sheet.
## Changes
### Types (`config.ts`, `schemas.ts`)
- Removed `keys` field from `ModelProviderConfig`, `AddProviderRequest`, and
`UpdateProviderRequest`
- Added `CreateProviderKeyRequest`, `UpdateProviderKeyRequest`,
`ListProviderKeysResponse` types
### Store (`providersApi.ts`, `baseApi.ts`)
- Added `ProviderKeys` tag type to `baseApi`
- Changed `getProviderKeys`/`getProviderKey` from `Providers` tag to
`ProviderKeys` tag (avoids invalidating provider cache on key changes)
- Added `invalidatesTags: [ProviderKeys, DBKeys]` on `updateProvider` mutation
(refreshes key statuses after model discovery)
- Removed `getProvider`/`getProviders` cache patches from `createProviderKey`,
`updateProviderKey`, `deleteProviderKey` (providers no longer carry keys)
- Added duplicate-check guards on `createProviderKey` cache patches to prevent
ghost keys
- Each key mutation patches `getProviderKeys` and `getAllKeys` caches from
authoritative server response
### Components
- **`modelProviderKeysTableView.tsx`**: Already uses `useGetProviderKeysQuery`;
formatting/indentation fixes
- **`page.tsx`**: Removed `keys: []` from fallback provider object and
`createProvider` call; simplified `KeyDiscoveryFailedBadge` to only check
provider-level status (removed per-key status check since keys are no longer
on provider)
- **`routingRuleSheet.tsx`**: `TargetRow` now receives `allKeys` prop (from
`useGetAllKeysQuery`) instead of `providersData` with `.keys`; filters keys
by target provider
- **`routingRuleInfoSheet.tsx`**: New read-only sheet component that displays
routing rule details (conditions, targets with provider icons and weight bars,
fallback chain, scope, priority, timestamps)
- **`settingsPanel.tsx`**: Uses `useGetAllKeysQuery` to determine configured
providers (replaces `p.keys.length > 0` check) and derive
`providerKeyConfigs` per provider
### Other frontend changes (from prior commit, unchanged)
- Added `getProviderKeys`, `getProviderKey` RTK Query endpoints
- Added `createProviderKey`, `updateProviderKey`, `deleteProviderKey` mutations
- Added `buildProviderUpdatePayload` utility for key-free provider updates
- Migrated `providerKeyForm.tsx` to separate create/update mutations
- Updated `addNewKeySheet.tsx` props from `keyIndex` to `keyId`
- Updated all 6 provider form fragments to use `buildProviderUpdatePayload`
- Removed dead `selectedProvider.keys` sync matchers from `providerSlice.ts`
## Type of change
- [x] Feature
- [x] Refactor
- [ ] Bug fix
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
```sh
cd ui
npm run build
npm run lint
```
Manual testing:
1. Navigate to Providers page, select a provider with keys
2. Verify keys table loads correctly from dedicated API
3. Create a new key — verify it appears immediately (no ghost/duplicate)
4. Toggle enable/disable — verify switch updates immediately
5. Edit a key — verify form pre-populates, save works
6. Delete a key — verify it disappears immediately
7. Update provider settings — verify key statuses refresh after save
8. Check sidebar badge shows provider-level discovery failures
9. Open Playground settings — verify provider/key dropdowns work
10. Open Routing Rules — verify target key selector works
11. Click a routing rule row — verify info sheet opens with correct details
(conditions, targets, fallbacks, scope, priority)
## Screenshots/Recordings
N/A — no visual changes to existing features; routing rule info sheet is new.
## Breaking changes
- [ ] Yes
- [x] No
Frontend-only changes consuming the new API shape from PR #2160.
## Related issues
N/A
## Security considerations
No new security considerations. Key values continue to be handled through
existing redaction on the backend.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* refactor: replace string slice with WhiteList type for model restrictions (#2282)
## Summary
Refactored model access control logic by replacing string slice with a dedicated `WhiteList` type for the `Models` field in `TableKey`. This change introduces a more structured approach to handling wildcard permissions and improves code readability.
## Changes
- Changed `Models` field type from `[]string` to `schemas.WhiteList` in `TableKey` struct
- Replaced manual wildcard checking (`model == "*"`) with `IsUnrestricted()` method calls across multiple functions
- Added missing mock method `GetVirtualKeyMCPConfigsByMCPClientIDs` to test configuration store
- Applied the refactoring consistently in `ReloadProvider`, `ForceReloadPricing`, and `Bootstrap` methods
## Type of change
- [x] Refactor
- [ ] Bug fix
- [ ] Feature
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that model access control continues to work correctly with both wildcard and specific model permissions:
```sh
# Core/Transports
go version
go test ./...
# Test specific areas affected by the changes
go test ./framework/configstore/tables/...
go test ./transports/bifrost-http/...
```
Test scenarios should include:
- Keys with wildcard permissions (`["*"]`)
- Keys with specific model restrictions
- Keys with empty model lists (deny-by-default behavior)
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
This refactoring maintains the existing security model for API key permissions. The deny-by-default behavior and wildcard functionality remain unchanged, just implemented through a more structured type system.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: add Plus icon and responsive text to pricing override create button (#2285)
## Summary
Improves the visual design and mobile responsiveness of the pricing overrides section by adding a Plus icon to the create button and optimizing the button text for different screen sizes.
## Changes
- Added Plus icon import from lucide-react
- Enhanced the "Create Override" button with a Plus icon and responsive text that shows "New Override" on larger screens and hides text on mobile
- Adjusted container spacing by removing top margin and changing flex alignment from `items-start` to `items-center` for better visual balance
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Navigate to the custom pricing overrides page and verify:
1. The "New Override" button displays with a Plus icon
2. On mobile screens, only the Plus icon is visible
3. On larger screens (sm and above), both icon and "New Override" text are visible
4. The button functionality remains unchanged when clicked
```sh
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
## Screenshots/Recordings
Before/after screenshots showing the button design changes and responsive behavior would be helpful.
## Breaking changes
- [x] Yes
- [ ] No
## Related issues
## Security considerations
No security implications - this is a purely visual enhancement.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* refactor: blacklist models on new convention (#2305)
## Summary
Implements comprehensive blacklist support for model filtering across all providers. This adds the ability to explicitly deny access to specific models at the key level, with blacklist rules taking precedence over allowlist rules.
## Changes
- Added `BlackList` type with semantic validation (supports wildcard "*" for block-all)
- Updated key selection logic to check both allowlist and blacklist constraints
- Modified all provider model listing functions to filter out blacklisted models
- Enhanced UI to support blacklist configuration with improved UX for wildcard selection
- Added blacklist filtering to model catalog and provider handlers
- Updated test cases to verify blacklist functionality
Key design decisions:
- Blacklist always wins over allowlist when conflicts occur
- Wildcard "*" in blacklist blocks all models for that key
- Empty blacklist blocks nothing (permissive default)
- Consistent filtering logic across all providers (Anthropic, Azure, Bedrock, Cohere, etc.)
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
Test blacklist functionality with provider keys:
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Example configuration to test:
```json
{
"keys": [{
"id": "test-key",
"models": ["*"],
"blacklisted_models": ["gpt-4", "claude-3"]
}]
}
```
Verify that blacklisted models are excluded from model listings and key selection.
## Screenshots/Recordings
UI now shows "Blocked Models" field with improved tooltips and wildcard handling for denying access to specific models.
## Breaking changes
- [ ] Yes
- [x] No
The `blacklisted_models` field was already present in the schema but not fully implemented. This change makes it functional without breaking existing configurations.
## Related issues
Enhances model access control capabilities for fine-grained permission management.
## Security considerations
Improves security by allowing explicit denial of access to sensitive or expensive models at the key level. Blacklist rules cannot be bypassed by allowlist configurations.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* minor fix add blacklisted model field in tableKeyFromSchemaKey (#2324)
## Summary
This PR adds support for the `BlacklistedModels` field when converting schema keys to table keys in the configuration store's RDB implementation.
## Changes
- Added `BlacklistedModels: key.BlacklistedModels` field mapping in the `tableKeyFromSchemaKey` function
- Ensures that blacklisted model information is properly preserved when converting between schema and table representations
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that configuration keys with blacklisted models are properly stored and retrieved from the RDB configstore.
```sh
# Core/Transports
go version
go test ./...
```
Test creating configuration entries with `BlacklistedModels` specified and ensure they persist correctly through the RDB layer.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
None - this change only adds field mapping for existing blacklisted models functionality.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: add image edit input view on logs (#2321)
## Summary
Adds support for logging image edit and image variation requests by introducing new database columns and UI components to track and display these image manipulation operations alongside existing image generation functionality.
## Changes
- Added `image_edit_input` and `image_variation_input` columns to the logs table with corresponding database migrations
- Extended the Log struct with new fields for storing and parsing image edit/variation input data
- Updated logging plugin to capture image edit and variation request data with large payload threshold handling
- Enhanced UI to display input images and prompts for image edit operations and input images for variation operations
- Added image MIME type detection for proper display of base64-encoded images in the UI
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [ ] Do…
dwjwlxs
added a commit
to dwjwlxs/bifrost
that referenced
this pull request
Apr 20, 2026
* fix: delete fallbacks from anthropic req (#2754)
## Summary
Remove the `fallbacks` field from Anthropic provider request bodies to ensure compatibility with the Anthropic API specification.
## Changes
- Added logic to delete the `fallbacks` field from JSON request bodies in the Anthropic provider's `getRequestBodyForResponses` function
- Implemented proper error handling for the field deletion operation with appropriate Bifrost error wrapping
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test Anthropic provider requests to ensure the `fallbacks` field is properly removed and requests succeed:
```sh
# Core/Transports
go version
go test ./...
# Test specific Anthropic provider functionality
go test ./core/providers/anthropic/...
```
Verify that requests to the Anthropic API no longer include the `fallbacks` field and complete successfully.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this change only removes an unsupported field from API requests.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* fix: preserve context values in async requests (#2703)
## Summary
Refactors async job execution to pass the full BifrostContext instead of just the virtual key value, enabling proper context preservation for background operations including virtual keys, tracing headers, and other request metadata.
## Changes
- Modified `AsyncJobExecutor.SubmitJob()` to accept `*schemas.BifrostContext` instead of `*string` for virtual key
- Updated `executeJob()` to restore all original request context values in the background goroutine
- Added `getVirtualKeyFromContext()` helper function to extract virtual key from BifrostContext
- Updated all async handler methods to pass BifrostContext directly to `SubmitJob()`
- Removed redundant virtual key extraction logic from HTTP handlers
## Type of change
- [x] Refactor
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
## How to test
Verify async job execution preserves request context properly:
```sh
# Core/Transports
go version
go test ./...
# Test async endpoints with virtual keys and tracing headers
curl -X POST http://localhost:8080/v1/async/chat/completions \
-H "Authorization: Bearer vk_test_key" \
-H "X-Trace-Id: test-trace-123" \
-d '{"model": "gpt-3.5-turbo", "messages": [{"role": "user", "content": "Hello"}]}'
# Verify job execution maintains context
curl http://localhost:8080/v1/async/jobs/{job_id}
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
Improves security by ensuring proper context isolation and virtual key handling in async operations.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* [fix]: Gemini provider - handle content block tool outputs in Responses API path (#2692)
When function_call_output messages arrive via the Anthropic Responses API
format, their output is an array of content blocks (ResponsesFunctionToolCallOutputBlocks),
not a plain string (ResponsesToolCallOutputStr). The Gemini provider's
convertResponsesMessagesToGeminiContents only checked the string case,
silently dropping all tool result content and sending empty {} responses
to Gemini. This caused the model to loop endlessly retrying tool calls
it never saw results for.
Other providers (Bedrock, OpenAI, Cohere) already handle both output
formats. This aligns the Gemini provider with them.
Affected packages:
- core/providers/gemini/responses.go - Add ResponsesFunctionToolCallOutputBlocks handling
- core/providers/gemini/gemini_test.go - Add test for content block outputs
Co-authored-by: tom <tom@asteroid.ai>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Akshay Deo <akshay@akshaydeo.com>
* fix: gemini thinking level and finish reason round-trip preservation (#2697)
## Summary
Fixes two critical regressions in the Gemini provider's GenAI integration: preserves `thinkingLevel` parameters during round-trip conversions and ensures `MAX_TOKENS` finish reasons survive Bifrost transformations.
## Changes
- **Fixed thinking level preservation**: Modified `convertGenerationConfigToResponsesParameters()` to only set effort from `thinkingLevel` without deriving a `thinkingBudget`, preventing unwanted behavior changes in Gemini 3.x models
- **Enhanced finish reason handling**: Added bidirectional conversion between Gemini and Bifrost finish reasons, prioritizing `StopReason` over `IncompleteDetails` to preserve `MAX_TOKENS` finish reasons
- **Expanded finish reason support**: Added new Gemini finish reason constants for image generation, tool calls, and malformed responses
- **Improved response conversion**: Updated response conversion logic to properly handle error finish reasons and set appropriate status/error fields
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate the thinking level and finish reason preservation:
```sh
# Run Gemini provider tests
go test ./core/providers/gemini/... -v
# Specifically test the regression fixes
go test ./core/providers/gemini/... -run "TestGenAIThinkingLevel_RoundTripPreservesLevelNotBudget|TestGenAIFinishReasonMaxTokens_PersistsThroughBifrostRoundTrip" -v
```
Test with actual Gemini API calls using thinking levels and verify that:
- `thinkingLevel` parameters are preserved without generating unwanted `thinkingBudget` values
- Responses with `MAX_TOKENS` finish reason maintain that status through the conversion pipeline
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
Addresses regressions in GenAI path where thinking configuration and finish reasons were being incorrectly transformed during Bifrost conversions.
## Security considerations
No security implications - this change only affects internal data structure conversions and doesn't modify authentication, secrets handling, or data exposure.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: remove cc user agent guard from streaming in anthropic (#2706)
## Summary
Fixes WebSearch tool argument handling for all clients by removing the Claude Code user agent restriction. Previously, only Claude Code clients received proper WebSearch query arguments in the streaming response, while other clients lost the query data due to skipped argument deltas.
## Changes
- Removed the `IsClaudeCodeRequest(ctx)` check that was restricting WebSearch argument sanitization and synthetic delta generation to only Claude Code clients
- WebSearch tool arguments are now sanitized and synthetic `input_json_delta` events are generated for all clients during `output_item.done` events
- Added comprehensive test coverage for the WebSearch tool flow including argument delta skipping, synthetic delta generation, and full end-to-end streaming scenarios
- Enhanced code comments to clarify the WebSearch tool handling logic
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate the WebSearch tool behavior with the new test suite:
```sh
# Run the new WebSearch tests
go test ./core/providers/anthropic -run TestWebSearch -v
# Run all provider tests to ensure no regressions
go test ./core/providers/anthropic/...
# Full test suite
go test ./...
```
Test with different user agents to verify WebSearch queries are properly streamed to all clients, not just Claude Code.
## Screenshots/Recordings
N/A - This is a backend streaming API fix.
## Breaking changes
- [ ] Yes
- [x] No
This change expands functionality to previously broken clients without affecting existing working behavior.
## Related issues
Fixes WebSearch tool argument streaming for non-Claude Code clients.
## Security considerations
The change maintains existing argument sanitization for WebSearch tools while expanding it to all clients, preserving the same security posture.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* remove unnecessary marshalling of payload (#2770)
## Summary
Optimized JSON parsing in the Anthropic integration by replacing full JSON unmarshaling with targeted field extraction using gjson for retrieving the "type" field from streaming responses.
## Changes
- Replaced `sonic.Unmarshal()` with `gjson.Get()` to extract only the "type" field from Anthropic stream events
- Eliminated the need to unmarshal the entire JSON response into an `AnthropicStreamEvent` struct
- Improved performance by avoiding unnecessary JSON parsing overhead
## Type of change
- [x] Refactor
- [ ] Bug fix
- [ ] Feature
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test streaming responses from the Anthropic integration to ensure the type field is correctly extracted:
```sh
# Core/Transports
go version
go test ./...
# Test specifically the Anthropic integration
go test ./transports/bifrost-http/integrations/
```
## Screenshots/Recordings
N/A
## Breaking changes
- [x] No
- [ ] Yes
## Related issues
N/A
## Security considerations
No security implications - this is a performance optimization that maintains the same functionality.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: claude opus 4.7 compatibility (#2773)
## Summary
Adds support for Claude Opus 4.7 model with specific parameter handling and reasoning configuration changes. Opus 4.7 rejects temperature, top_p, and top_k parameters and only supports adaptive thinking mode without budget tokens.
## Changes
- Added `IsOpus47()` function to detect Claude Opus 4.7 models
- Modified parameter handling to skip temperature, top_p, and top_k for Opus 4.7 models
- Updated reasoning configuration to use adaptive thinking only for Opus 4.7 (no budget_tokens)
- Added support for `display` parameter in thinking configuration to control output visibility
- Extended adaptive thinking support to include Sonnet 4.6 models
- Added task budget support with new beta header `task-budgets-2026-03-13`
- Updated effort mapping to handle Opus 4.7's "xhigh" effort level
- Added comprehensive test coverage for Opus 4.7 specific behaviors
- Fixed OpenAI responses to filter out Anthropic-specific summary:"none" parameter
## Type of change
- [x] Feature
- [x] Bug fix
## Affected areas
- [x] Core (Go)
- [x] Providers/Integrations
## How to test
Validate the changes with the following tests:
```sh
# Core/Transports
go version
go test ./core/providers/anthropic/...
# Specific test cases for Opus 4.7
go test -run TestToAnthropicChatRequest_Opus47 ./core/providers/anthropic/
go test -run TestSupportsAdaptiveThinking ./core/providers/anthropic/
go test -run TestAddMissingBetaHeadersToContext_TaskBudgets ./core/providers/anthropic/
```
Test with Claude Opus 4.7 model requests to ensure:
- Temperature, top_p, top_k parameters are stripped
- Reasoning uses adaptive thinking without budget_tokens
- Task budget beta headers are properly added
## Breaking changes
- [ ] Yes
- [x] No
The changes maintain backward compatibility while adding new model support.
## Security considerations
No security implications. Changes only affect parameter handling and model-specific configurations for Anthropic's Claude models.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* docs: restructure helm guide into comprehensive multi-page reference (#2776)
* docs: restructure helm guide into comprehensive multi-page reference (#2771)
## Summary
Restructures the Helm deployment documentation into a comprehensive multi-page guide with dedicated sections for each configuration area. The main Helm page now provides quickstart instructions for both OSS and Enterprise deployments, while detailed configuration is split into focused sub-pages.
## Changes
- **Restructured main Helm page**: Condensed from 740+ lines to 103 lines with clear quickstart tabs for OSS vs Enterprise
- **Added 8 new dedicated configuration pages**:
- `values.mdx` - Complete values reference with examples and common patterns
- `client.mdx` - Client configuration (pool size, logging, CORS, auth, compat shims)
- `providers.mdx` - Provider setup for all 23+ supported LLM providers with cloud-native auth
- `storage.mdx` - Storage backends (SQLite, PostgreSQL, object storage, vector stores)
- `plugins.mdx` - Plugin configuration (telemetry, logging, semantic cache, OTel, Datadog)
- `governance.mdx` - Governance setup (budgets, rate limits, virtual keys, routing rules)
- `cluster.mdx` - Multi-replica HA with gossip-based peer discovery
- `troubleshooting.mdx` - Common issues and diagnostic commands
- **Updated chart version**: Bumped from 1.5.0 to 2.1.0
- **Enhanced navigation**: Added nested Helm section in docs.json with proper icons and organization
## Type of change
- [x] Documentation
## Affected areas
- [x] Docs
## How to test
Navigate through the new Helm documentation structure:
1. Visit the main Helm page for quickstart instructions
2. Follow the quickstart for either OSS or Enterprise deployment
3. Use the sub-pages for detailed configuration of specific areas
4. Verify all internal links work correctly
5. Test the troubleshooting commands on a real deployment
The documentation now provides both quick-start paths and comprehensive reference material for production deployments.
## Screenshots/Recordings
N/A - Documentation changes only
## Breaking changes
- [ ] Yes
- [x] No
This is purely a documentation restructure with no functional changes to the Helm chart itself.
## Related issues
Improves Helm documentation organization and usability for both new users and production deployments.
## Security considerations
The new documentation emphasizes security best practices:
- Kubernetes Secrets for all sensitive values
- Cloud-native authentication (IRSA, Workload Identity, Managed Identity)
- Proper RBAC setup for cluster mode
- Compliance considerations (HIPAA, PCI) for content logging
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* docs: update guardrails docs
* v1.4.23 cut (#2778)
## Summary
Release version 1.4.20-1.4.23 with bug fixes for provider integrations, streaming error handling, and migration test improvements. This release addresses critical issues in Gemini, Bedrock, and Anthropic providers while adding support for Claude Opus 4.7.
## Changes
- **Provider Fixes**: Fixed Gemini tool outputs handling, Bedrock streaming events, and image content preservation in tool results
- **Streaming Improvements**: Added proper error capture for Responses streaming API to prevent silent failures
- **Migration Tests**: Added support for v1.4.22 governance model pricing flex tier columns in both PostgreSQL and SQLite migration tests
- **Anthropic Enhancements**: Removed fallback fields from outgoing requests and added Claude Opus 4.7 compatibility
- **Framework Fixes**: Improved async context propagation and custom provider model validation
- **Plugin Updates**: Enhanced OTEL metrics and configuration defaults
## Type of change
- [x] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate the migration test changes and provider fixes:
```sh
# Test migration scripts
./.github/workflows/scripts/run-migration-tests.sh
# Core/Transports
go version
go test ./...
# Test provider integrations
go test ./transports/...
go test ./plugins/...
```
Test the new governance model pricing columns are properly handled in migration scenarios.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
Addresses multiple provider integration issues and streaming API error handling improvements.
## Security considerations
No security implications - changes are focused on bug fixes and migration test improvements.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* validator fix (#2780)
## Summary
Enhanced GitHub Actions security by transitioning from audit-only to strict network egress control using step-security/harden-runner. This change blocks all outbound network traffic by default and explicitly allows only required endpoints for each workflow.
## Changes
- Changed `egress-policy` from `audit` to `block` across all GitHub Actions workflows
- Added comprehensive `allowed-endpoints` lists for each job, specifying only the necessary external services
- Updated step names from "Harden the runner (Audit all outbound calls)" to "Harden Runner" for consistency
- Fixed schema validation script to use correct JSON paths for concurrency and SCIM configuration validation
- Reformatted JSON schema file for improved readability (whitespace and formatting changes only)
## Type of change
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
Verify that all GitHub Actions workflows continue to function properly with the new network restrictions:
```sh
# Trigger workflows by pushing to a branch or creating a PR
git push origin feature-branch
# Monitor workflow runs in GitHub Actions tab to ensure:
# - All jobs complete successfully
# - No network connectivity errors occur
# - All required external services remain accessible
```
Key endpoints that should remain accessible include:
- GitHub API and release assets
- Package registries (npm, PyPI, Go modules)
- Docker registries
- Cloud storage services
- External APIs used by tests and integrations
## Screenshots/Recordings
N/A - Infrastructure/CI changes only
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
This change significantly improves security posture by:
- Preventing unauthorized outbound network connections from CI runners
- Creating an explicit allowlist of required external services
- Reducing attack surface for supply chain attacks
- Providing better visibility into network dependencies
The transition from audit to block mode ensures that any new network dependencies must be explicitly approved and documented.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: token usage for vllm --skip-pipeline (#2784)
## Summary
Fixed token usage attribution for vLLM by treating empty-string content the same as nil content in streaming responses. vLLM sends `delta.content=""` (instead of `delta: null`) in finish_reason chunks, which was being forwarded and causing the synthesis chunk to lose its finish_reason, breaking usage attribution in logs and UI.
## Changes
- Modified streaming content handling to check for both nil and empty string content before processing chunks
- This prevents empty content deltas from being forwarded, ensuring finish_reason is preserved for proper token usage tracking
- Removed extraneous whitespace and formatting inconsistencies throughout the OpenAI provider code
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test with vLLM provider to ensure token usage is properly attributed:
```sh
# Core/Transports
go version
go test ./...
# Test streaming chat completion with vLLM
# Verify that finish_reason is preserved in final chunks
# Check that token usage appears correctly in logs/UI
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
Fixes token usage tracking issues with vLLM provider.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* [fix]: OpenAI provider - flatten array-form tool_result output for Responses API (#2781) --skip-pipeline
When Anthropic tool_result blocks arrive with array-form content (the
standard shape for multi-turn tool exchanges), the OpenAI provider's
MarshalJSON emitted the output as a JSON array on the wire. The OpenAI
Responses API defines function_call_output.output as a string — strict
upstreams (Ollama Cloud, openai-go typed models) reject the array form
with HTTP 400.
Fix: before marshaling, collapse text-only
ResponsesFunctionToolCallOutputBlocks into a newline-joined string.
Non-text blocks (images, files) are left as-is. The schema type is
unchanged; the transformation lives in the OpenAI provider's outbound
marshaler only.
Closes #2779
Affected packages:
- core/providers/openai/types.go - Flatten text-only output blocks to string
- core/providers/openai/responses_marshal_test.go - Three regression tests
- core/changelog.md - Changelog entry
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: prevent send on closed channel panic in provider queue shutdown --skip-pipeline (#2725)
## Summary
Fixes a race condition in provider queue shutdown that caused "send on closed channel" panics in production. The issue occurred when producers passed the `isClosing()` check but then attempted to send to a queue that was closed before they reached the select statement.
## Changes
- **Removed queue channel closure**: Queue channels are never closed to prevent "send on closed channel" panics
- **Updated worker exit mechanism**: Workers now exit via the `done` channel signal instead of waiting for queue closure
- **Enhanced shutdown handling**: Workers drain remaining buffered requests and send shutdown errors when `done` is signaled
- **Added producer re-routing**: Stale producers can transparently re-route to new queues during `UpdateProvider`
- **Improved error handling**: Added rollback logic for failed provider updates with proper cleanup
- **Enhanced transfer logic**: Buffered requests are transferred before signaling shutdown to ensure they reach new workers
- **Added comprehensive tests**: Race condition demonstration and validation of the fix across multiple scenarios
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Run the new race condition test to verify the fix:
```sh
go test -run TestProviderQueue_SendOnClosedChannel_Race ./core -v
```
Run the comprehensive provider lifecycle tests:
```sh
go test -run TestProviderQueue ./core -v
go test -run TestUpdateProvider ./core -v
go test -run TestRemoveProvider ./core -v
```
Run the full test suite to ensure no regressions:
```sh
go test ./...
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
Fixes production panics related to concurrent provider queue operations during shutdown/updates.
## Security considerations
None - this is an internal concurrency fix that doesn't affect external interfaces or data handling.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: preserve MCP tool annotations in bidirectional conversion --skip-pipeline (#2746)
## Summary
Adds support for preserving MCP tool annotations when converting between MCP tools and Bifrost schemas. This enables MCP servers to provide behavioral hints (read-only, destructive, idempotent, open-world) that help agents make better reasoning decisions about tool usage.
## Changes
- Added `MCPToolAnnotations` struct to capture MCP spec hints including title, read-only, destructive, idempotent, and open-world indicators
- Modified `convertMCPToolToBifrostSchema` to preserve MCP tool annotations when converting from MCP tools to Bifrost chat tools
- Updated `ChatToolFunction` to include optional annotations field
- Enhanced MCP server sync logic to map Bifrost annotations back to MCP tool annotations for bidirectional compatibility
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test with an MCP server that provides tool annotations to verify they are preserved through the conversion process:
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Verify that MCP tools with annotations maintain their behavioral hints when converted to Bifrost schemas and back.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this change only preserves metadata hints that help with tool behavior classification.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* fix: add support for Anthropic structured output and response format (#1972)
* fix: add support for Anthropic structured output and response format conversion
* fix: refactor output configuration setting in ToBedrockResponsesRequest
* run go fmt on responses.go
* fix: streamline response format conversion for Anthropic models
* fix: enhance merging of additional model request fields and output configuration
* fix: remove koanf/maps dependency and replace its usage with internal merge function
* preserve order in output_config
* update type casting
* add non-anthropic test-case
* check for output_config first
* diversify anthropic output formats
* move bifrost ctx update
* guard tested field
* guard format.jsonschema
* test fixes --skip-pipeline (#2782)
## Summary
Updates test configurations to align with current API specifications and replaces deprecated utility function usage.
## Changes
- Replaced `schemas.Ptr("test")` with `new("test")` in Anthropic chat test for string pointer creation
- Updated MCP client configuration tests to use `sse` connection type instead of `websocket` with simplified `connection_string` field
- Modified HTTP MCP client config to use `connection_string` instead of nested `http_config` object
- Changed OpenTelemetry plugin tests to use `genai_extension` trace type instead of `otel`
## Type of change
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
## How to test
Validate that all tests pass with the updated configurations:
```sh
# Core/Transports
go version
go test ./...
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - these are test configuration updates only.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* anthropic container changes --skip-pipeline (#2783)
## Summary
Briefly explain the purpose of this PR and the problem it solves.
## Changes
- What was changed and why
- Any notable design decisions or trade-offs
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Describe the steps to validate this change. Include commands and expected outcomes.
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
If adding new configs or environment variables, document them here.
## Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
## Breaking changes
- [ ] Yes
- [ ] No
If yes, describe impact and migration instructions.
## Related issues
Link related issues and discussions. Example: Closes #123
## Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* core schema changes --skip-pipeline (#2787)
## Summary
Promotes Anthropic-native parameters to the neutral ChatParameters layer, enabling direct access to advanced Anthropic features like containers, MCP servers, task budgets, and enhanced tool configurations without requiring ExtraParams.
## Changes
- Added neutral fields to `ChatParameters` for Anthropic-specific features: `TopK`, `Speed`, `InferenceGeo`, `MCPServers`, `Container`, `CacheControl`, `TaskBudget`, and `ContextManagement`
- Enhanced `ChatTool` with Anthropic tool flags: `DeferLoading`, `AllowedCallers`, `InputExamples`, and `EagerInputStreaming`
- Added `Display` field to `ChatReasoning` for Anthropic adaptive thinking control
- Implemented `StripUnsupportedAnthropicFields` function to remove unsupported features based on provider capabilities
- Updated parameter mapping logic to prefer neutral fields over ExtraParams with fallback support
- Added comprehensive JSON marshaling/unmarshaling for union types like `ChatContainer`
The design maintains backward compatibility by falling back to ExtraParams when neutral fields are not set, while providing type-safe access to advanced Anthropic features.
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate the new parameter handling and provider feature gating:
```sh
# Core/Transports
go version
go test ./...
# Test Anthropic provider parameter mapping
go test ./core/providers/anthropic/...
# Verify schema validation
go test ./core/schemas/...
```
Test with requests containing the new neutral fields to ensure proper mapping to Anthropic API format and appropriate stripping for unsupported providers.
## Screenshots/Recordings
N/A - Backend API changes only.
## Breaking changes
- [ ] Yes
- [x] No
This change is fully backward compatible. Existing ExtraParams usage continues to work, while new neutral fields provide enhanced type safety.
## Related issues
N/A
## Security considerations
The new MCP server configuration includes authorization tokens. Ensure proper handling of sensitive credentials in the `ChatMCPServer.AuthorizationToken` field.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* dependabot fixes --skip-pipeline (#2788)
## Summary
This PR adds the Hono web framework as a direct dependency to all MCP server examples and updates various dependencies across the project to their latest versions.
## Changes
- Added `hono@^4.12.14` as a direct dependency to all MCP server examples (edge-case-server, error-test-server, parallel-test-server, temperature, test-tools-server)
- Upgraded Hono from version 4.11.4 to 4.12.14 and changed it from a peer dependency to a direct dependency
- Updated Python dependencies including authlib (1.6.6 → 1.6.11), langchain-core (1.2.28 → 1.2.31), langchain-openai (1.1.4 → 1.1.14), langchain-text-splitters (1.1.0 → 1.1.2), langsmith (0.5.0 → 0.7.32), openai (2.13.0 → 2.32.0), and python-multipart (0.0.20 → 0.0.26)
- Updated TypeScript dependencies including langsmith (0.5.18 → 0.5.19) and added it as a direct dependency
- Added `github.com/tidwall/gjson v1.18.0` as a direct dependency in Go transports module
- Updated UI dependencies including dompurify (3.3.3 → 3.4.0) and follow-redirects (1.15.11 → 1.16.0) via package overrides
## Type of change
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] UI (Next.js)
## How to test
Validate that all dependencies are properly installed and examples still function:
```sh
# Test MCP server examples
cd examples/mcps/temperature
npm install
npm run build
# Test Go transports
cd transports
go mod tidy
go test ./...
# Test Python integrations
cd tests/integrations/python
uv sync
uv run python -m pytest
# Test TypeScript integrations
cd tests/integrations/typescript
npm install
npm test
# Test UI
cd ui
pnpm install
pnpm test
pnpm build
```
## Screenshots/Recordings
N/A - dependency updates only
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
The dependency updates include security patches, particularly for dompurify and follow-redirects which are explicitly overridden in the UI package.json for security reasons.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* move back go to 1.26.1 (#2792)
## Summary
Downgrade Go version from 1.26.2 to 1.26.1 across all GitHub Actions workflows, Go modules, and Docker images to address compatibility issues.
## Changes
- Downgraded Go version from 1.26.2 to 1.26.1 in all GitHub Actions workflows (e2e-tests, pr-tests, release-cli, release-pipeline, snyk)
- Updated go.mod files for core, CLI, examples, and test modules to use Go 1.26.1
- Updated Docker base images in transports/Dockerfile and transports/Dockerfile.local to use golang:1.26.1-alpine3.23
- Added stream cancellation safety improvements with guarded channel sends and finalizer protection to prevent goroutine leaks when clients disconnect
- Enhanced stream error checking with context cancellation support to properly drain upstream channels
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate the Go version downgrade and streaming improvements:
```sh
# Verify Go version
go version
# Core/Transports
go test ./...
# Test streaming endpoints with client disconnection scenarios
# to verify proper cleanup and no goroutine leaks
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
The streaming improvements enhance resource cleanup and prevent potential goroutine leaks when clients disconnect unexpectedly, improving overall system stability.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* temp gotoolchain auto (#2809)
* temp hack for tests (#2810)
## Summary
The Go workspace setup script was not specifying a `go` directive or toolchain version, which caused `GOTOOLCHAIN=auto` to select a Go version lower than what `core@v1.4.19` requires. This adds an explicit `go 1.26.2` and `toolchain go1.26.2` directive to the workspace so the correct toolchain is used automatically.
## Changes
- Added `go work edit -go=1.26.2 -toolchain=go1.26.2` to `setup-go-workspace.sh` so that `GOTOOLCHAIN=auto` selects Go >= 1.26.2, satisfying the minimum version required by the published `core@v1.4.19` module referenced in `transports/go.mod`.
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
# Verify the workspace is initialized with the correct Go version
bash .github/workflows/scripts/setup-go-workspace.sh
grep -E "^go |^toolchain" go.work
# Expected output:
# go 1.26.2
# toolchain go1.26.2
go test ./...
```
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
## Security considerations
None.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* temp block docker build (#2811)
## Summary
Temporarily disables the `test-docker-image-amd64` and `test-docker-image-arm64` CI jobs in the release pipeline by commenting them out.
## Changes
- Both Docker image test jobs (`test-docker-image-amd64` and `test-docker-image-arm64`) have been commented out rather than removed, preserving the full job definitions for easy re-enablement later.
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
No functional code changes. Verify the release pipeline runs without executing the Docker image test jobs.
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* removed docker build steps (#2812)
## Summary
The `test-docker-image-amd64` and `test-docker-image-arm64` CI jobs have been removed from the release pipeline. These jobs were already commented out and non-functional, and all references to them as dependencies and gate conditions in downstream release jobs have been cleaned up.
## Changes
- Deleted the commented-out `test-docker-image-amd64` and `test-docker-image-arm64` job definitions from the release pipeline.
- Removed `test-docker-image-amd64` and `test-docker-image-arm64` from the `needs` arrays of `core-release`, `framework-release`, `plugins-release`, `bifrost-http-release`, the Docker build/push jobs, the manifest job, and the final notification job.
- Removed the corresponding result checks for those two jobs from all `if` conditions in the affected release jobs.
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Trigger the release pipeline and confirm that all release jobs proceed without waiting on or referencing the removed Docker image test jobs.
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
## Security considerations
None.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* moves tests to 1.26.2 and 1.26.1 (#2813)
## Summary
Bumps the Go version used across all release pipeline jobs from `1.26.1` to `1.26.2` to keep the CI environment on the latest patch release.
## Changes
- Updated Go version from `1.26.1` to `1.26.2` in all `setup-go` steps within the release pipeline workflow.
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
The release pipeline will use the updated Go version on the next run. No additional manual steps are required beyond verifying the CI pipeline passes.
```sh
go version
go test ./...
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
Patch releases often include security and bug fixes. Staying on the latest patch version reduces exposure to known vulnerabilities in the Go toolchain.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* ocr test fixes (#2814)
## Summary
Adds an operation-allowed check for OCR requests before they are dispatched to a provider, and fixes the Mistral provider to return its custom provider name when one is configured.
## Changes
- Added a `CheckOperationAllowed` guard for `OCRRequest` in `handleProviderRequest`, consistent with how other request types are gated. If the operation is not permitted, a `BifrostError` is returned with the provider key, request type, and requested model populated.
- Updated `MistralProvider.GetProviderKey()` to use `providerUtils.GetProviderName` so that custom provider configurations are respected, rather than always returning the hardcoded `schemas.Mistral` value.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
go version
go test ./...
```
- Configure a custom provider wrapping Mistral and verify that `GetProviderKey()` returns the custom provider name rather than `mistral`.
- Attempt an OCR request against a provider where the operation is not allowed and confirm a `BifrostError` is returned with the correct `Provider`, `RequestType`, and `ModelRequested` fields set.
- Attempt an OCR request against a provider where the operation is allowed and confirm the request proceeds normally.
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
## Security considerations
None.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* revert to old schema (#2815)
## Summary
This PR simplifies and consolidates the `config.schema.json` by removing several features, collapsing provider-specific schema variants, and restructuring key configuration definitions to reduce complexity and align with updated runtime semantics.
## Changes
- Removed the top-level `version` field that controlled allow-list semantics for empty arrays
- Removed the `compat` plugin configuration block (including `convert_text_to_chat`, `convert_chat_to_responses`, `should_drop_params`, `should_convert_params`)
- Replaced `compat` with a simpler `enable_litellm_fallbacks` boolean for Groq text completion fallbacks
- Removed `mcp_disable_auto_tool_inject` and `routing_chain_max_depth` from server config
- Collapsed `provider_with_ollama_config`, `provider_with_sgl_config`, and `provider_with_replicate_config` into the generic `provider` definition; removed their corresponding key types (`ollama_key`, `sgl_key`, `replicate_key`) and `network_config_without_base_url`
- Removed providers `nebius`, `xai`, and `runway` from the providers block
- Moved `calendar_aligned` from `virtual_key` to the `budget` object; removed `virtual_key_id` and `provider_config_id` from budget in favor of a standalone `budget_id` reference on virtual keys
- Removed `chain_rule` from routing rules and relaxed the `scope_id` conditional requirement
- Simplified `virtual_key_provider_config` to inline key definitions with full provider-specific key configs (Azure, Vertex, Bedrock, VLLM), replacing the separate `key_ids` and `keys` split
- Removed `mcp_client_name` and `allow_on_all_virtual_keys` from MCP configs; removed `allowed_extra_headers` and `disable_auto_tool_inject` from MCP client config
- Added `websocket` as a supported MCP connection type with a dedicated `websocket_config` block; removed `inprocess` connection type
- Removed `per_user_oauth` as an MCP auth type and dropped the conditional `oauth_config_id` requirement
- Renamed `concurrency_and_buffer_size` to `concurrency_config`; renamed `retry_backoff_initial`/`retry_backoff_max` to `retry_backoff_initial_ms`/`retry_backoff_max_ms`; removed `enforce_http2` and `openai_config` from network config
- Moved `pricing_overrides` from the top-level config into individual provider definitions
- Simplified `provider_pricing_override` schema, removing scoped fields (`scope_kind`, `virtual_key_id`, `provider_id`, `provider_key_id`) and replacing `pattern` with `model_pattern`; added `regex` as a valid `match_type`; expanded supported `request_types`
- Renamed `scim_config` to `saml_config` in the top-level schema
- Removed `apiToken` from Okta config and made `clientSecret` optional; updated required fields to only `issuerUrl` and `clientId`
- Removed `object_storage` and `retention_days` from the logs store config
- Removed `id` and `description` fields from provider config entries in the `provider_configs` array
- Removed `websocket_responses` and `realtime` from `custom_provider_config` allowed requests; removed the enum constraint on `base_provider_type`
- Removed `disable_auto_tool_inject` from `mcp_client_config` VFS settings
- Added `deployments` mapping to `azure_key_config` and `vertex_key_config`
- Updated `otel` plugin `trace_type` to only accept `"otel"` (removed `genai_extension`, `vercel`, `open_inference`)
- Removed `prompts` from the built-in plugin name list
- Removed `builtin` as a valid plugin `placement` value
- Changed `cluster_config` discovery `dial_timeout` from a Go duration string to an integer (nanoseconds)
- Reformatted many inline `required` arrays to multi-line style for readability
## Type of change
- [ ] Bug fix
- [ ] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate existing configs against the updated schema to confirm they parse correctly. Verify that configs using removed fields (`version`, `compat`, `mcp_disable_auto_tool_inject`, `chain_rule`, etc.) are rejected by the schema validator.
```sh
go test ./...
```
Confirm that provider configs for Ollama, SGL, and Replicate continue to work using the generic `provider` definition. Confirm MCP clients using `websocket` connection type validate correctly with a `websocket_config` block.
## Breaking changes
- [x] Yes
- [ ] No
The following fields have been removed and configs using them will fail schema validation:
- `version` (top-level)
- `compat` block under server config
- `mcp_disable_auto_tool_inject` and `routing_chain_max_depth` under server config
- `chain_rule` on routing rules
- `calendar_aligned` on virtual keys (now on budgets)
- `virtual_key_id` / `provider_config_id` on budgets
- `apiToken` on Okta config (now optional `clientSecret` only)
- `object_storage` and `retention_days` on logs store
- `id`, `description` on provider config entries
- `allow_on_all_virtual_keys`, `allowed_extra_headers`, `disable_auto_tool_inject` on MCP client config
- `inprocess` MCP connection type and `per_user_oauth` auth type
- `enforce_http2` and `openai_config` from network config
- `builtin` plugin placement value; `prompts` built-in plugin name
- `nebius`, `xai`, `runway` provider entries
Migrate by removing or replacing these fields according to the updated schema definitions.
## Related issues
## Security considerations
Removal of `per_user_oauth` as an MCP auth type should be reviewed to ensure no active integrations depend on it. The relaxed `scope_id` requirement on routing rules should be validated to confirm it does not inadvertently broaden access scope.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* reduced release pipeline for this cut for go downgrade (#2816)
## Summary
This PR removes all test jobs from the release pipeline and decouples them from the release gate conditions, allowing releases to proceed without waiting for (often flaky) provider API test results. It also significantly expands and restructures `config.schema.json` to reflect new features, provider support, and breaking semantic changes introduced in v1.5.0.
## Changes
- **Release pipeline**: Removed `test-core`, `approve-flaky-test-core`, `test-framework`, `test-plugins`, `test-bifrost-http`, `test-migrations`, and `test-e2e-ui` jobs entirely from `release-pipeline.yml`. All release jobs (`core-release`, `framework-release`, `plugins-release`, `bifrost-http-prep`, `docker-build-amd64`, `docker-build-arm64`, `push-mintlify-changelog`) now depend only on change detection and upstream release jobs, not on test outcomes.
- **Schema: deny-by-default semantics (v1.5.0)**: Empty arrays in `provider_configs`, `mcp_configs`, `allowed_models`, `key_ids`, and `tools_to_execute` now mean "deny all" rather than "allow all". Use `["*"]` to allow all. A top-level `version` field (enum `1` or `2`, default `2`) controls which semantic applies, with `1` restoring v1.4.x behavior.
- **Schema: new providers**: Added `nebius`, `xai`, and `runway` as first-class provider entries.
- **Schema: provider key restructuring**: Replaced the inline key object definition in `virtual_key_provider_config` with a flat `key_ids` string array. Introduced dedicated key types `ollama_key`, `sgl_key`, and `replicate_key` with their own `_key_config` blocks. Removed `deployments` from `azure_key_config` and `vertex_key_config` (replaced by `aliases` on `base_key`). Added `aliases` to `base_key` for model-to-deployment/inference-profile mappings.
- **Schema: provider variants**: `ollama` and `sgl` now reference `provider_with_ollama_config` and `provider_with_sgl_config` respectively, which use `network_config_without_base_url` (URL is per-key). `replicate` references `provider_with_replicate_config`. Added `openai_config` def with `disable_store` for the Responses API. Renamed `concurrency_config` to `concurrency_and_buffer_size`.
- **Schema: network config**: Split `network_config` into `network_config` (with `base_url`) and `network_config_without_base_url`. Added `enforce_http2`, `stream_idle_timeout_in_seconds`, `max_conns_per_host`, `beta_header_overrides`, and `ca_cert_pem` fields. Renamed `retry_backoff_initial_ms`/`retry_backoff_max_ms` to `retry_backoff_initial`/`retry_backoff_max`.
- **Schema: MCP changes**: Removed `websocket` connection type; added `inprocess`. Added `per_user_oauth` auth type. Added `mcp_client_name` for config-file resolution. Added `allowed_extra_headers` and `allow_on_all_virtual_keys` to `mcp_client_config`. Added `disable_auto_tool_inject` to MCP plugin config. Added global `mcp_disable_auto_tool_inject` and `routing_chain_max_depth` to server config.
- **Schema: routing rules**: Added `chain_rule` boolean to `routing_rule`. Made `scope_id` required (non-null string) when `scope` is `team`, `customer`, or `virtual_key`.
- **Schema: budgets**: Moved `calendar_aligned` from the budget object to the virtual key level. Replaced `budget_id` on virtual key with `virtual_key_id`/`provider_config_id` on the budget object itself. Removed `budget_id` from `virtual_key_provider_config`.
- **Schema: logs store**: Added `object_storage` (S3/GCS) and `retention_days` to the logs store config.
- **Schema: pricing overrides**: Moved `pricing_overrides` from per-provider to a top-level array with scoped `provider_pricing_override` objects supporting `scope_kind`, `virtual_key_id`, `provider_id`, `provider_key_id`, `match_type`, `pattern`, `request_types`, and `pricing_patch`.
- **Schema: compat plugin**: Replaced `enable_litellm_fallbacks` with a structured `compat` object supporting `convert_text_to_chat`, `convert_chat_to_responses`, `should_drop_params`, and `should_convert_params`.
- **Schema: OTEL plugin**: Expanded `trace_type` enum to `genai_extension`, `vercel`, `open_inference` (was only `otel`).
- **Schema: SCIM**: Renamed `saml_config` to `scim_config`. Added `apiToken` to `okta_config` and made `clientSecret` and `apiToken` required. Changed cluster `dial_timeout` from integer (nanoseconds) to Go duration string.
- **Schema: misc**: Added `prompts` and `builtin` to plugin name/placement enums. Added `provider_configs` fields `id`, `description`, `network_config`, `proxy_config`, `custom_provider_config`, `concurrency_and_buffer_size`, and `openai_config`. Added `scim_config` top-level ref. Normalized multi-item `required` arrays to single-line format throughout.
## Type of change
- [ ] Bug fix
- [x] Feature
- [x] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
# Validate schema against existing configs
npx ajv validate -s transports/config.schema.json -d your-config.json
# Verify release pipeline runs without test gate
# Push a tagged commit and confirm release jobs trigger directly after detect-changes
```
If upgrading from v1.4.x, set `"version": 1` in your config to preserve allow-all semantics for empty arrays, or migrate empty arrays to `["*"]` and adopt v2 deny-by-default semantics.
## Breaking changes
- [x] Yes
- [ ] No
**Empty arrays in `allowed_models`, `key_ids`, `tools_to_execute`, `provider_configs`, and `mcp_configs` now deny all access by default (v2 semantics).** To allow all, use `["*"]`. To restore v1.4.x behavior, set `"version": 1` at the top level of your config.
`enable_litellm_fallbacks` has been removed; replace with the `compat` object. `saml_config` has been renamed to `scim_config`. `budget_id` has been removed from virtual keys and `virtual_key_provider_config`. `calendar_aligned` has moved from the budget object to the virtual key. `deployments` has been removed from `azure_key_config` and `vertex_key_config`; use `aliases` on the key instead. `retry_backoff_initial_ms`/`retry_backoff_max_ms` renamed to `retry_backoff_initial`/`retry_backoff_max`. The `websocket` MCP connection type has been removed; use `http` or `sse`. Okta SCIM config now requires `clientSecret` and `apiToken`.
## Related issues
N/A
## Security considerations
The `insecure_skip_verify` and `ca_cert_pem` fields on `network_config` expose TLS bypass options; these should only be used in controlled environments. The `per_user_oauth` auth type for MCP introduces per-user credential flows that require careful OAuth config management. Removal of test gates from the release pipeline means regressions from flaky provider APIs will no longer block releases, but also means real failures could ship if not caught by other means.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* force verstion back to go 1.26.1 (#2817)
## Summary
Bumps `core` to v1.4.21 and updates `transports` to depend on `core` v1.4.20, while removing a now-unnecessary workspace Go directive workaround that was previously required to satisfy the toolchain constraint introduced by `core` v1.4.19.
## Changes
- Incremented `core` version from `1.4.20` to `1.4.21`
- Updated `transports/go.mod` to reference `core` v1.4.20 (previously v1.4.19)
- Removed the `go work edit -go=1.26.2 -toolchain=go1.26.2` workaround from the workspace setup script, which was only needed to satisfy the toolchain requirement imposed by the published `core` v1.4.19
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
go work sync
go test ./...
```
Verify the workspace initializes without the explicit Go/toolchain directive and that all modules resolve correctly.
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
## Security considerations
None.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* revert everything to go1.26.1 (#2818)
## Summary
Bumps the core version to `1.4.22` and rolls back dependency versions across the framework, plugins, and transports to align with a prior stable set of releases. This resolves a version inconsistency introduced by forward-referencing newer module versions that were not yet intended to be consumed by downstream packages.
## Changes
- Incremented `core/version` from `1.4.21` to `1.4.22`
- Downgraded `bifrost/core` from `v1.4.19` → `v1.4.17` across `framework`, `governance`, `jsonparser`, `litellmcompat`, `logging`, `maxim`, `mocker`, `otel`, `semanticcache`, and `telemetry` plugins
- Downgraded `bifrost/framework` from `v1.2.38` → `v1.2.36` (or `v1.2.35` for `semanticcache`) across all dependent plugins
- Downgraded `bifrost/core` in `transports` from `v1.4.20` → `v1.4.19`
- Downgraded all plugin versions referenced in `transports` (governance, litellmcompat, logging, maxim, otel, semanticcache, telemetry) to their corresponding prior releases
- Downgraded `go.opentelemetry.io/otel/sdk` and `go.opentelemetry.io/otel/sdk/metric` from `v1.43.0` → `v1.40.0` in affected plugins
- Bumped Go toolchain version in `transports/go.mod` from `1.26.1` to `1.26.2`
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
go test ./...
```
Verify that all modules resolve correctly with the pinned dependency versions and that no import errors occur during build.
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
None. These are internal module version adjustments with no changes to auth, secrets, or data handling.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* bumped up hello-world dep (#2819)
## Summary
Pins the `bifrost/core` dependency in the example plugin modules to a consistent released version (`v1.4.17`), removing a local `replace` directive that was pointing to the local `core` module path.
## Changes
- Replaced the local `replace` directive in `hello-world-wasm-go/go.mod` with a direct reference to `github.com/maximhq/bifrost/core v1.4.17`
- Downgraded `hello-world/go.mod` from `v1.4.19` to `v1.4.17` to align both example plugins on the same released version
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
cd examples/plugins/hello-world-wasm-go
go mod tidy
go build ./...
cd examples/plugins/hello-world
go mod tidy
go build ./...
```
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
## Security considerations
No security implications. This change only affects dependency resolution for example plugin modules.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* framework: bump core to v1.4.22 --skip-pipeline
* plugins/governance: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/jsonparser: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/litellmcompat: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/logging: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/maxim: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/mocker: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/otel: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/semanticcache: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/telemetry: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* enforce go 1.26.1 (#2820)
* transports: update dependencies --skip-pipeline
* Adds changelog for v1.4.23 --skip-pipeline
* V1.5.0 (#2245)
* refactor: standardize empty array conventions for VK Provider & MCP Configs, and makes Provider Config weight optional for routing (#1932)
## Summary
Changes Virtual Key provider and MCP configurations from "allow-all by default" to "deny-by-default" security model. Virtual Keys now require explicit provider and MCP client configurations to allow access, improving security posture.
## Changes
- **Provider Configs**: Empty `provider_configs` now blocks all providers instead of allowing all
- **MCP Configs**: Empty `mcp_configs` now blocks all MCP tools instead of allowing all
- **Weight Field**: Changed provider `weight` from required `float64` to optional `*float64` - null weight excludes provider from weighted routin…
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
Changes Virtual Key provider and MCP configurations from "allow-all by default" to "deny-by-default" security model. Virtual Keys now require explicit provider and MCP client configurations to allow access, improving security posture.
Changes
provider_configsnow blocks all providers instead of allowing allmcp_configsnow blocks all MCP tools instead of allowing allweightfrom requiredfloat64to optional*float64- null weight excludes provider from weighted routingType of change
Affected areas
How to test
Test Virtual Key creation and provider/MCP access:
Breaking changes
Impact: Existing Virtual Keys with empty
provider_configsormcp_configswould be blocked after this change.Migration: Automatic migration
migrationBackfillEmptyVirtualKeyConfigsruns on startup to backfill existing Virtual Keys with all available providers/MCP clients, preserving current behavior. New Virtual Keys created after this change will use deny-by-default.Security considerations
This change significantly improves security posture by requiring explicit configuration of allowed providers and MCP tools for Virtual Keys. The automatic migration ensures no disruption to existing deployments while new Virtual Keys benefit from the more secure default behavior.
Checklist
docs/contributing/README.mdand followed the guidelines