feat: add virtual key access management for MCP clients#2255
Merged
Pratham-Mishra04 merged 1 commit intov1.5.0from Mar 25, 2026
Conversation
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
12 tasks
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds Virtual Key (VK) access management for MCP clients: OpenAPI/schema updates, SDK/docs context-key changes, backend query/transactional replace logic with governance reloads, new config-store query methods, UI for per-VK tool allowlists, type additions, and changelog updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend (MCP Sheet)
participant Server as Backend (MCP Handler)
participant DB as Database (Config Store)
participant Gov as Governance Manager
UI->>Server: PUT /api/mcp/client (payload includes vk_configs when edited)
Server->>DB: GetVirtualKeyMCPConfigsByMCPClientIDs([clientIDs])
DB-->>Server: current VK config rows
Server->>Server: Validate vk_configs & compute diff (create/update/delete)
Server->>DB: Begin transaction
alt apply diffs
Server->>DB: Create/Update/Delete VK config rows
DB-->>Server: op results
Server->>DB: Commit
DB-->>Server: Commit result
else failure
Server->>DB: Rollback
DB-->>Server: rollback ack
end
Server->>DB: Save MCP client row
DB-->>Server: saved
Server->>Gov: ReloadVirtualKey for affected virtual_key_id(s)
Gov-->>Server: reload ack
Server-->>UI: 200 OK + updated MCP client JSON (with vk_configs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (2)
docs/quickstart/go-sdk/context-keys.mdx (1)
381-381: Undocumented context key in reference table.
BifrostContextKeyUserAgentappears in the reference table but has no corresponding documentation section above explaining its purpose or usage, unlike all other keys in the table.Consider adding a brief section under "Response Metadata Keys" to document this key, or clarify if it was intentionally omitted from the detailed documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/quickstart/go-sdk/context-keys.mdx` at line 381, Add a short documented entry for BifrostContextKeyUserAgent under the "Response Metadata Keys" section similar to the other keys: state that BifrostContextKeyUserAgent is a read-only string containing the user agent of the incoming request, describe when/where it is populated (server side/request middleware), and include a small usage note showing how to read it from the response context (matching the style of other keys in the doc). Ensure the table row remains and add the matching explanatory subsection heading and one-paragraph description for BifrostContextKeyUserAgent.transports/bifrost-http/handlers/mcp.go (1)
636-650: Validate VirtualKeyID existence before creating VK MCP configs to prevent orphan records and ensure consistency.The code creates
TableVirtualKeyMCPConfigrecords without verifying that the providedVirtualKeyIDactually exists. Since the database schema has no foreign key constraint on this field, invalid VK IDs will be silently stored as orphan records.This pattern should match the existing validation used in governance handlers. Use
h.store.ConfigStore.GetVirtualKey(ctx, vc.VirtualKeyID)to check existence and return404 Bad Requestfor non-existent keys, providing clear feedback to the API consumer.💡 Suggested validation approach
// Index requested assignments by VK ID requestedByVKID := make(map[string]MCPVKConfigRequest, len(*req.VKConfigs)) for _, vc := range *req.VKConfigs { requestedByVKID[vc.VirtualKeyID] = vc } + // Validate that all requested VK IDs exist + for _, vc := range *req.VKConfigs { + _, err := h.store.ConfigStore.GetVirtualKey(ctx, vc.VirtualKeyID) + if err != nil { + if errors.Is(err, configstore.ErrNotFound) { + SendError(ctx, fasthttp.StatusBadRequest, fmt.Sprintf("virtual key %s does not exist", vc.VirtualKeyID)) + return + } + SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("failed to validate virtual key %s: %v", vc.VirtualKeyID, err)) + return + } + } if err := h.store.ConfigStore.ExecuteTransaction(ctx, func(tx *gorm.DB) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcp.go` around lines 636 - 650, Before calling CreateVirtualKeyMCPConfig for any vc in the loop over req.VKConfigs (and when deciding to insert vs update using currentByVKID), validate that the VirtualKey exists by calling h.store.ConfigStore.GetVirtualKey(ctx, vc.VirtualKeyID); if GetVirtualKey returns not found, return a 404-style error to the caller (propagate a clear "virtual key not found" error), otherwise proceed to CreateVirtualKeyMCPConfig; keep the existing UpdateVirtualKeyMCPConfig path unchanged but consider using GetVirtualKey as well to guard against race conditions.
🤖 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/quickstart/go-sdk/context-keys.mdx`:
- Around line 299-307: The example imports the unused package "time" which will
cause a compile error; either remove the "time" import or demonstrate its use by
referencing the BifrostContextKeyStreamIdleTimeout context key (e.g., create a
duration variable via time.Second/time.Minute and attach it to the example
context before using bifrost client calls). Update the import block and the
example body accordingly so "time" is either removed or clearly used with
BifrostContextKeyStreamIdleTimeout.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 612-665: The MCP client update is committed before VK configs are
updated so a failure in the VK config block (ExecuteTransaction) can leave a
partially-applied state; fix by moving the MCP client update into the same
transaction that runs the VK config changes (use
h.store.ConfigStore.ExecuteTransaction to include the MCP client update and the
calls to
UpdateVirtualKeyMCPConfig/CreateVirtualKeyMCPConfig/DeleteVirtualKeyMCPConfig)
so both commit or rollback together; if you cannot run them in one DB
transaction, capture the prior MCP client state (via GetMCPClientByID), perform
the VK config transaction, and on error perform a compensating revert using the
MCP client update method (e.g., UpdateMCPClient) and ensure in-memory state is
synchronized with the DB before returning the error.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 54-68: The current initialVKConfigs uses useGetVirtualKeysQuery({
limit: 200 }) and derives a replace-all vk_configs payload on save (vk_configs +
vkConfigsDirty), which will drop assignments outside the first page; change this
to source the VK assignments for the current MCP client from a complete source
instead of the first 200 rows — either call a dedicated endpoint that returns
all virtual key assignments for mcpClient.config.name (e.g.,
"getVirtualKeyAssignmentsForMcp") and build initialVKConfigs from that, or
paginate/fetch all pages of useGetVirtualKeysQuery before allowing edits;
additionally, when saving (the vk_configs replacement path), avoid blind
full-replace by merging changes or re-fetching the authoritative assignment set
for the MCP client before sending the vk_configs payload so unseen assignments
are preserved (look for initialVKConfigs, useGetVirtualKeysQuery,
vkConfigsDirty, and the vk_configs save code to update).
- Around line 793-805: Add stable `data-testid` attributes to the new
virtual-key UI controls: add a `data-testid` on the add trigger in the
SelectTrigger used with `addVKConfig` (e.g., "virtual-key-add-trigger"), add
`data-testid`s on the per-row tool selector Select (the element that renders
each row's tool choice via `vkOptions` / `SelectItem`) (e.g.,
"virtual-key-tool-selector-<rowId>" or "virtual-key-tool-selector-<index>"), and
add `data-testid` on the per-row remove button (the button that removes a VK
config) using the repo pattern `data-testid="<entity>-<element>-<qualifier>"` so
test authors can reliably target the add trigger, each row's tool selector, and
each row's remove button; update both the add control block (SelectTrigger) and
the per-row blocks (where SelectItem and the remove button are rendered)
accordingly.
---
Nitpick comments:
In `@docs/quickstart/go-sdk/context-keys.mdx`:
- Line 381: Add a short documented entry for BifrostContextKeyUserAgent under
the "Response Metadata Keys" section similar to the other keys: state that
BifrostContextKeyUserAgent is a read-only string containing the user agent of
the incoming request, describe when/where it is populated (server side/request
middleware), and include a small usage note showing how to read it from the
response context (matching the style of other keys in the doc). Ensure the table
row remains and add the matching explanatory subsection heading and
one-paragraph description for BifrostContextKeyUserAgent.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 636-650: Before calling CreateVirtualKeyMCPConfig for any vc in
the loop over req.VKConfigs (and when deciding to insert vs update using
currentByVKID), validate that the VirtualKey exists by calling
h.store.ConfigStore.GetVirtualKey(ctx, vc.VirtualKeyID); if GetVirtualKey
returns not found, return a 404-style error to the caller (propagate a clear
"virtual key not found" error), otherwise proceed to CreateVirtualKeyMCPConfig;
keep the existing UpdateVirtualKeyMCPConfig path unchanged but consider using
GetVirtualKey as well to guard against race conditions.
🪄 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: 352049c9-380f-4e29-b8ec-3a04baaed02d
📒 Files selected for processing (9)
docs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamldocs/quickstart/go-sdk/context-keys.mdxframework/configstore/rdb.goframework/configstore/store.gotransports/bifrost-http/handlers/mcp.goui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.ts
|
|
Pratham-Mishra04
force-pushed
the
03-24-feat_adds_option_to_automatically_add_new_mcp_server_to_assigned_vks
branch
from
629991f to
2fdadfb
Compare
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
docs/quickstart/go-sdk/context-keys.mdx (1)
299-303:⚠️ Potential issue | 🟡 MinorRemove the unused
timeimport from the complete example.
timeis imported here but never referenced anywhere in this example, so the copy-paste snippet still fails to compile.#!/bin/bash python - <<'PY' from pathlib import Path text = Path("docs/quickstart/go-sdk/context-keys.mdx").read_text() start = text.index("package main") end = text.index("```", start) block = text[start:end] print('"time" imported:', '"time"' in block) print('time package references:', block.count("time.")) PYExpected result: the script reports that the import is present and there are
0time.references.Also applies to: 309-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/quickstart/go-sdk/context-keys.mdx` around lines 299 - 303, Remove the unused "time" import from the example import block so the Go snippet compiles; specifically delete "time" from the import list (the line containing "time") in the import section that currently reads import ( "context" "fmt" "log" "time" ) and also check and remove any other duplicate import occurrences in the same example region (the subsequent import block around lines 309-349) so there are no unused time references left.ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx (2)
792-855:⚠️ Potential issue | 🟡 MinorAdd stable
data-testids to the new VK controls.The add trigger, per-row tool picker, and remove button are interactive, but none expose stable selectors. Please add them on the
SelectTrigger, eachMultiSelect, and the removeButton.As per coding guidelines
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes following the pattern:data-testid="<entity>-<element>-<qualifier>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 792 - 855, The new virtual key UI controls lack stable test selectors; add data-testid attributes following the pattern entity-element-qualifier to the SelectTrigger used for adding VKs, to each per-row MultiSelect (identify by vc.virtual_key_id) and to the per-row remove Button (the one calling removeVKConfig), e.g. use values that incorporate the virtual key id for row-level controls; update the JSX for SelectTrigger, MultiSelect, and the Button tied to removeVKConfig (and the Select onValueChange addVKConfig) to include appropriate data-testid attributes.
53-68:⚠️ Potential issue | 🟠 MajorDon't build a replace-all
vk_configspayload from the first VK page.
useGetVirtualKeysQuery({ limit: 200 })only loads a partial set, but Line 173 later sendsvk_configsas a full replacement. Any existing assignment outside that page gets silently removed on save. Source this form from the MCP client's authoritative assignment set, or fetch all pages before enabling edits.Also applies to: 173-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 53 - 68, The current initialVKConfigs built in the useMemo (initialVKConfigs) is derived from the partial page returned by useGetVirtualKeysQuery({ limit: 200 }) and will cause vk_configs to be treated as a full replace (removing assignments outside that page); instead, source the form state from the MCP client's authoritative assignment set (mcpClient.config?.vk_configs or mcpClient.mcp_configs) or ensure you fetch/aggregate all virtual_keys pages before constructing initialVKConfigs; update the logic around useGetVirtualKeysQuery, the initialVKConfigs useMemo, and the code that emits vk_configs on save so it uses the full authoritative set (mcpClient) or a complete paginated result rather than the single-page virtual_keys slice.transports/bifrost-http/handlers/mcp.go (1)
563-667:⚠️ Potential issue | 🟠 MajorVK assignment failures still leave the MCP client update committed.
The MCP config is persisted before this VK block runs, and the runtime update has already happened by the time these lookups and transaction steps execute. If any of them fail, the handler returns 500 for a partially-applied request. Move the MCP client update into the same transaction boundary as the VK diff, or add compensating rollback for both store and runtime state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcp.go` around lines 563 - 667, The MCP client DB update and the runtime update (h.store.ConfigStore.UpdateMCPClientConfig and h.mcpManager.UpdateMCPClient) happen before the VK assignment transaction, so VK failures leave a partially-applied state; fix by moving the MCP client DB update and the VK diff work into the same ExecuteTransaction boundary (use h.store.ConfigStore.ExecuteTransaction) and only call h.mcpManager.UpdateMCPClient after the transaction successfully commits, or alternatively perform the runtime update first but add a compensating rollback that calls h.mcpManager.UpdateMCPClient with oldDBConfig when the VK ExecuteTransaction fails; update code paths that call UpdateMCPClientConfig, UpdateMCPClient, ExecuteTransaction, UpdateVirtualKeyMCPConfig, CreateVirtualKeyMCPConfig, DeleteVirtualKeyMCPConfig and GetMCPClientByID accordingly so DB and in-memory state stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 630-643: Reject duplicate virtual_key_id values before entering
the transaction: validate req.VKConfigs for duplicate VirtualKeyID entries (the
slice used to build requestedByVKID) and return a client validation error if any
duplicates are found. Do this check prior to calling
h.store.ConfigStore.ExecuteTransaction so the create/update loop (for _, vc :=
range *req.VKConfigs) and functions like requestedByVKID and
UpdateVirtualKeyMCPConfig cannot attempt duplicate inserts and trigger a DB
unique-index error. Ensure the validation reports which VirtualKeyID is
duplicated and stops processing when duplicates exist.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 776-867: The current block is gated by the condition
mcpClient.tools && mcpClient.tools.length > 0 which hides Virtual Key Access
when no tools are loaded; remove or relax that guard so the section always
renders (e.g., change to render when mcpClient exists or at least when
vkConfigs/vkOptions exist). Ensure toolOptions and the MultiSelect in this block
handle an empty tools list by falling back to a wildcard-only or read-only set
(e.g., if toolOptions is empty show only "*" or disable selection), and keep
updateVKConfigTools, addVKConfig, removeVKConfig, vkConfigs, and vkById behavior
intact so admins can inspect/revoke VK access even when tool discovery failed.
Ensure placeholders and maxCount remain appropriate and the UI is still safe
when mcpClient.tools is [] or undefined.
---
Duplicate comments:
In `@docs/quickstart/go-sdk/context-keys.mdx`:
- Around line 299-303: Remove the unused "time" import from the example import
block so the Go snippet compiles; specifically delete "time" from the import
list (the line containing "time") in the import section that currently reads
import ( "context" "fmt" "log" "time" ) and also check and remove any other
duplicate import occurrences in the same example region (the subsequent import
block around lines 309-349) so there are no unused time references left.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 563-667: The MCP client DB update and the runtime update
(h.store.ConfigStore.UpdateMCPClientConfig and h.mcpManager.UpdateMCPClient)
happen before the VK assignment transaction, so VK failures leave a
partially-applied state; fix by moving the MCP client DB update and the VK diff
work into the same ExecuteTransaction boundary (use
h.store.ConfigStore.ExecuteTransaction) and only call
h.mcpManager.UpdateMCPClient after the transaction successfully commits, or
alternatively perform the runtime update first but add a compensating rollback
that calls h.mcpManager.UpdateMCPClient with oldDBConfig when the VK
ExecuteTransaction fails; update code paths that call UpdateMCPClientConfig,
UpdateMCPClient, ExecuteTransaction, UpdateVirtualKeyMCPConfig,
CreateVirtualKeyMCPConfig, DeleteVirtualKeyMCPConfig and GetMCPClientByID
accordingly so DB and in-memory state stay consistent.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 792-855: The new virtual key UI controls lack stable test
selectors; add data-testid attributes following the pattern
entity-element-qualifier to the SelectTrigger used for adding VKs, to each
per-row MultiSelect (identify by vc.virtual_key_id) and to the per-row remove
Button (the one calling removeVKConfig), e.g. use values that incorporate the
virtual key id for row-level controls; update the JSX for SelectTrigger,
MultiSelect, and the Button tied to removeVKConfig (and the Select onValueChange
addVKConfig) to include appropriate data-testid attributes.
- Around line 53-68: The current initialVKConfigs built in the useMemo
(initialVKConfigs) is derived from the partial page returned by
useGetVirtualKeysQuery({ limit: 200 }) and will cause vk_configs to be treated
as a full replace (removing assignments outside that page); instead, source the
form state from the MCP client's authoritative assignment set
(mcpClient.config?.vk_configs or mcpClient.mcp_configs) or ensure you
fetch/aggregate all virtual_keys pages before constructing initialVKConfigs;
update the logic around useGetVirtualKeysQuery, the initialVKConfigs useMemo,
and the code that emits vk_configs on save so it uses the full authoritative set
(mcpClient) or a complete paginated result rather than the single-page
virtual_keys slice.
🪄 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: a5ad6095-be9f-47e7-9357-896a581a992e
📒 Files selected for processing (10)
docs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamldocs/quickstart/go-sdk/context-keys.mdxframework/configstore/rdb.goframework/configstore/store.gotransports/bifrost-http/handlers/mcp.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.ts
✅ Files skipped from review due to trivial changes (3)
- transports/changelog.md
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- docs/openapi/paths/management/mcp.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/configstore/store.go
- docs/openapi/schemas/management/mcp.yaml
- framework/configstore/rdb.go
Pratham-Mishra04
force-pushed
the
03-24-feat_adds_option_to_automatically_add_new_mcp_server_to_assigned_vks
branch
from
2fdadfb to
9aa9293
Compare
Pratham-Mishra04
force-pushed
the
03-16-chore_added_puslse_as_air_alternative_for_dev
branch
from
ed44f8c to
1c89903
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx (1)
777-782:⚠️ Potential issue | 🟠 MajorVirtual Key Access section still hidden when MCP server has no tools.
The section is gated by
mcpClient.tools && mcpClient.tools.length > 0, which prevents administrators from viewing or revoking VK access when an MCP server is disconnected, in error state, or has no tools loaded. Consider rendering this section regardless of tool availability, falling back to wildcard-only tool selection when tools aren't available.Suggested approach
- {mcpClient.tools && mcpClient.tools.length > 0 && ( + {/* Always render VK Access section - tools may be unavailable for disconnected servers */} + {( <div className="space-y-4 pb-10 mt-6">Then adjust the
toolOptionslogic to handle the empty tools case gracefully (wildcard only).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 777 - 782, The Virtual Key Access block is currently hidden by the mcpClient.tools && mcpClient.tools.length > 0 guard in mcpClientSheet.tsx, which prevents admins from managing VK when a server has no tools; remove that conditional so the section always renders (keep the surrounding JSX for the "Virtual Key Access" UI) and update the code that builds toolOptions (the logic that maps mcpClient.tools into select options) to handle an empty or undefined tools array by returning a single wildcard-only option (e.g., [{ label: '*', value: '*' }]) so the UI and revoke/assign flows work when the MCP is disconnected or has no tools loaded.
🧹 Nitpick comments (1)
ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx (1)
888-888: Minor: Extra space in closing tag.There's an extra space before
>in the closingSheettag.Fix
- </Sheet > + </Sheet>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` at line 888, Remove the stray space in the closing JSX tag for the Sheet component in mcpClientSheet.tsx (the line rendering "</Sheet >"); change the closing tag to a normal JSX closing tag ("</Sheet>") so the component markup is valid and consistent with other JSX tags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 777-782: The Virtual Key Access block is currently hidden by the
mcpClient.tools && mcpClient.tools.length > 0 guard in mcpClientSheet.tsx, which
prevents admins from managing VK when a server has no tools; remove that
conditional so the section always renders (keep the surrounding JSX for the
"Virtual Key Access" UI) and update the code that builds toolOptions (the logic
that maps mcpClient.tools into select options) to handle an empty or undefined
tools array by returning a single wildcard-only option (e.g., [{ label: '*',
value: '*' }]) so the UI and revoke/assign flows work when the MCP is
disconnected or has no tools loaded.
---
Nitpick comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Line 888: Remove the stray space in the closing JSX tag for the Sheet
component in mcpClientSheet.tsx (the line rendering "</Sheet >"); change the
closing tag to a normal JSX closing tag ("</Sheet>") so the component markup is
valid and consistent with other JSX tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2188da96-9ecd-46a8-baf1-0193416c2f7f
📒 Files selected for processing (11)
docs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamldocs/quickstart/go-sdk/context-keys.mdxframework/configstore/rdb.goframework/configstore/store.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.ts
✅ Files skipped from review due to trivial changes (3)
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- transports/changelog.md
- docs/openapi/paths/management/mcp.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/configstore/rdb.go
- ui/lib/types/mcp.ts
- docs/quickstart/go-sdk/context-keys.mdx
- transports/bifrost-http/handlers/mcp.go
Pratham-Mishra04
changed the base branch from
03-16-chore_added_puslse_as_air_alternative_for_dev
to
graphite-base/2255
Pratham-Mishra04
force-pushed
the
graphite-base/2255
branch
from
1c89903 to
0e06156
Compare
Pratham-Mishra04
force-pushed
the
03-24-feat_adds_option_to_automatically_add_new_mcp_server_to_assigned_vks
branch
from
9aa9293 to
f2faf58
Compare
Contributor
Confidence Score: 3/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Pratham-Mishra04
force-pushed
the
03-24-feat_adds_option_to_automatically_add_new_mcp_server_to_assigned_vks
branch
from
f2faf58 to
e75dd87
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
docs/quickstart/go-sdk/context-keys.mdx (1)
299-307:⚠️ Potential issue | 🟡 MinorUnused
timeimport still present.The
timepackage is imported on line 303 but not used anywhere in the complete example. This would cause a compilation error if a user copies the code directly.Either remove the import or add a usage example (e.g., demonstrating
BifrostContextKeyStreamIdleTimeout).Option 1: Remove unused import
import ( "context" "fmt" "log" - "time" "github.com/maximhq/bifrost" "github.com/maximhq/bifrost/core/schemas" )Option 2: Add usage for the import
// Include raw provider response for debugging bfCtx.SetValue(schemas.BifrostContextKeySendBackRawResponse, true) + // Set streaming idle timeout + bfCtx.SetValue(schemas.BifrostContextKeyStreamIdleTimeout, 30*time.Second) + // Restrict MCP tools to a specific client bfCtx.SetValue(schemas.MCPContextKeyIncludeClients, []string{"filesystem"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/quickstart/go-sdk/context-keys.mdx` around lines 299 - 307, Remove the unused "time" import or use it; specifically, either delete "time" from the import block shown around the top of the example or add a short usage showing the constant BifrostContextKeyStreamIdleTimeout (e.g., create a context with context.WithValue(ctx, schemas.BifrostContextKeyStreamIdleTimeout, 30*time.Second)) so the time package is referenced and the example compiles.ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx (2)
803-809:⚠️ Potential issue | 🟡 MinorAdd a test id to the VK search input.
The other new VK controls now have selectors, but this searchable input still doesn't. That makes the add flow harder to automate end-to-end.
As per coding guidelines "
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes following the pattern:data-testid=\"<entity>-<element>-<qualifier>\"."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 803 - 809, The VK search Input lacks a test id; add a data-testid attribute to the Input in mcpClientSheet.tsx (the JSX element using value={vkSearch} and onChange={(e) => setVKSearch(e.target.value)}) following the project pattern; e.g. add data-testid="virtualkey-search-input" (entity-element-qualifier) so the searchable input can be targeted by end-to-end tests.
777-882:⚠️ Potential issue | 🟠 MajorDon't hide VK access when tool discovery fails.
This guard still removes the entire section whenever
mcpClient.toolsis empty/disconnected, which prevents admins from inspecting or revoking existing VK assignments on broken servers. Render the section unconditionally and fall back to wildcard-only or disabled tool choices until tools load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 777 - 882, The current top-level conditional (checking mcpClient.tools && mcpClient.tools.length > 0) hides the entire Virtual Key Access UI when tool discovery fails; remove that guard so the section always renders; inside the block, make tool selection resilient by using toolOptions (fallback to ["*"] or an empty list) and disable the MultiSelect when toolOptions.length === 0 (or set its options to the wildcard-only option) and update the placeholder text to indicate tools are unavailable (e.g., "Tools unavailable — only wildcard allowed" or "All tools allowed" when vc.tools_to_execute includes "*"); ensure addVKConfig, vkSearch, vkOptions, vkConfigs, vkNameByID, updateVKConfigTools and removeVKConfig continue to work unchanged so admins can inspect/revoke VK assignments even if mcpClient.tools is empty or disconnected.
🧹 Nitpick comments (3)
framework/configstore/rdb.go (1)
2169-2174: Prefer a batched VK lookup for the paginated clients response.This helper is being used while building
GET /api/mcp/clients, so each page becomes one client query plus one VK-config query per client. AGetVirtualKeyMCPConfigsByMCPClientIDs-style lookup grouped in memory would avoid an N+1 as the registry grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 2169 - 2174, The current GetVirtualKeyMCPConfigsByMCPClientID causes N+1 queries when building the paginated GET /api/mcp/clients response; add a batched variant GetVirtualKeyMCPConfigsByMCPClientIDs that accepts a slice of mcpClientIDs, queries once using WHERE mcp_client_id IN (?) to fetch all matching tables.TableVirtualKeyMCPConfig rows, and return them grouped by client ID (e.g., map[uint][]tables.TableVirtualKeyMCPConfig or a slice with a grouping key) so callers (the pagination builder for GET /api/mcp/clients) can fetch VK configs for a page of clients in a single query and then attach configs to each client in-memory.transports/bifrost-http/handlers/mcp.go (2)
132-144: Non-paginated path omitsVKConfigsfrom response.The non-paginated
getMCPClientspath (in-memory config) doesn't includeVKConfigsin the response, while the paginated path (lines 236-268) does. This could cause UI inconsistency depending on which path is used.If this is intentional (e.g., non-paginated is for legacy/simple use cases), consider documenting this difference. Otherwise, consider adding VK config fetch here for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcp.go` around lines 132 - 144, The non-paginated branch that builds MCPClientResponse (inside getMCPClients) currently omits VKConfigs causing inconsistency with the paginated branch; update the else and the connected branch where MCPClientResponse is appended so both include the VKConfigs field (fetch the client's VK configs the same way the paginated path does and assign them to MCPClientResponse.VKConfigs) ensuring the redactedConfig/connectedClient-based responses include the same VKConfigs payload as the paginated path.
236-248: Consider logging errors from VK config fetch.The error from
GetVirtualKeyMCPConfigsByMCPClientIDis silently swallowed. While returning partial data is reasonable for a read path, logging the error would aid debugging when VK configs unexpectedly appear empty.🔧 Suggested improvement
vkConfigs := []MCPVKConfigResponse{} if h.store.ConfigStore != nil { - if assignments, err := h.store.ConfigStore.GetVirtualKeyMCPConfigsByMCPClientID(ctx, dbClient.ID); err == nil { + assignments, fetchErr := h.store.ConfigStore.GetVirtualKeyMCPConfigsByMCPClientID(ctx, dbClient.ID) + if fetchErr != nil { + logger.Warn("failed to fetch VK configs for MCP client %d: %v", dbClient.ID, fetchErr) + } else { for _, a := range assignments { vkConfigs = append(vkConfigs, MCPVKConfigResponse{ VirtualKeyID: a.VirtualKeyID, VirtualKeyName: vkNameByID[a.VirtualKeyID], ToolsToExecute: a.ToolsToExecute, }) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcp.go` around lines 236 - 248, The call to GetVirtualKeyMCPConfigsByMCPClientID swallows errors, so update the block that builds vkConfigs (using MCPVKConfigResponse, vkNameByID and dbClient.ID) to log any non-nil error before continuing; use the handler's logger (e.g., h.logger or h.log) to record the error with context (dbClient.ID and the operation name) and proceed to return partial data as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openapi/schemas/management/mcp.yaml`:
- Around line 230-258: The OpenAPI schema only documents the request-side
MCPVKConfig and leaves MCPClient response shapes incomplete; add a response
variant (e.g., MCPVKConfigResponse) that extends the existing MCPVKConfig by
adding virtual_key_name (string) and use that in the MCPClient response
vk_configs array (or add a separate vk_configs_response property on MCPClient)
so the GET /api/mcp/clients response includes virtual_key_name; update
references of vk_configs to point to the new response schema and ensure
required/description fields reflect the response shape.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 678-708: The vc.ToolsToExecute.Validate() call should be moved out
of the DB transaction so validation failures produce a 400 instead of triggering
a transaction rollback/500; before calling
h.store.ConfigStore.ExecuteTransaction iterate over *req.VKConfigs and call
vc.ToolsToExecute.Validate() for each vc (return the client error if validation
fails), then run ExecuteTransaction to perform
CreateVirtualKeyMCPConfig/UpdateVirtualKeyMCPConfig/DeleteVirtualKeyMCPConfig
using currentByVKID and requestedByVKID without re-validating ToolsToExecute
inside the transaction.
In `@transports/bifrost-http/server/server.go`:
- Line 1041: The MCP handler is being created with callbacks passed for the
governance manager unconditionally, which prevents nil-guards in handlers/mcp.go
from running; change the second argument of handlers.NewMCPHandler to pass the
actual governance manager when loaded or nil when not (e.g., use
s.GovernanceManager or equivalent field and pass nil if it's nil) so
ReloadVirtualKey() and other governance-only logic are skipped for
non-governance deployments; update the call that currently reads
NewMCPHandler(callbacks, callbacks, s.Client, s.Config, oauthHandler) to pass
the governance manager or nil instead of the callbacks value.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 793-819: The Select retains its last value so re-selecting the
same virtual key does nothing; add a controlled value for the Select (e.g.,
const [vkSelectValue, setVKSelectValue] = useState("")), pass that value into
the Select as value={vkSelectValue}, and in the onValueChange handler call
addVKConfig(v); setVKSearch(""); setVKSelectValue("") to reset the Select after
adding; update references to vkSearch and addVKConfig accordingly (ensure
useState is imported).
---
Duplicate comments:
In `@docs/quickstart/go-sdk/context-keys.mdx`:
- Around line 299-307: Remove the unused "time" import or use it; specifically,
either delete "time" from the import block shown around the top of the example
or add a short usage showing the constant BifrostContextKeyStreamIdleTimeout
(e.g., create a context with context.WithValue(ctx,
schemas.BifrostContextKeyStreamIdleTimeout, 30*time.Second)) so the time package
is referenced and the example compiles.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 803-809: The VK search Input lacks a test id; add a data-testid
attribute to the Input in mcpClientSheet.tsx (the JSX element using
value={vkSearch} and onChange={(e) => setVKSearch(e.target.value)}) following
the project pattern; e.g. add data-testid="virtualkey-search-input"
(entity-element-qualifier) so the searchable input can be targeted by end-to-end
tests.
- Around line 777-882: The current top-level conditional (checking
mcpClient.tools && mcpClient.tools.length > 0) hides the entire Virtual Key
Access UI when tool discovery fails; remove that guard so the section always
renders; inside the block, make tool selection resilient by using toolOptions
(fallback to ["*"] or an empty list) and disable the MultiSelect when
toolOptions.length === 0 (or set its options to the wildcard-only option) and
update the placeholder text to indicate tools are unavailable (e.g., "Tools
unavailable — only wildcard allowed" or "All tools allowed" when
vc.tools_to_execute includes "*"); ensure addVKConfig, vkSearch, vkOptions,
vkConfigs, vkNameByID, updateVKConfigTools and removeVKConfig continue to work
unchanged so admins can inspect/revoke VK assignments even if mcpClient.tools is
empty or disconnected.
---
Nitpick comments:
In `@framework/configstore/rdb.go`:
- Around line 2169-2174: The current GetVirtualKeyMCPConfigsByMCPClientID causes
N+1 queries when building the paginated GET /api/mcp/clients response; add a
batched variant GetVirtualKeyMCPConfigsByMCPClientIDs that accepts a slice of
mcpClientIDs, queries once using WHERE mcp_client_id IN (?) to fetch all
matching tables.TableVirtualKeyMCPConfig rows, and return them grouped by client
ID (e.g., map[uint][]tables.TableVirtualKeyMCPConfig or a slice with a grouping
key) so callers (the pagination builder for GET /api/mcp/clients) can fetch VK
configs for a page of clients in a single query and then attach configs to each
client in-memory.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 132-144: The non-paginated branch that builds MCPClientResponse
(inside getMCPClients) currently omits VKConfigs causing inconsistency with the
paginated branch; update the else and the connected branch where
MCPClientResponse is appended so both include the VKConfigs field (fetch the
client's VK configs the same way the paginated path does and assign them to
MCPClientResponse.VKConfigs) ensuring the redactedConfig/connectedClient-based
responses include the same VKConfigs payload as the paginated path.
- Around line 236-248: The call to GetVirtualKeyMCPConfigsByMCPClientID swallows
errors, so update the block that builds vkConfigs (using MCPVKConfigResponse,
vkNameByID and dbClient.ID) to log any non-nil error before continuing; use the
handler's logger (e.g., h.logger or h.log) to record the error with context
(dbClient.ID and the operation name) and proceed to return partial data as
before.
🪄 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: 9c1942bd-e473-42a8-ab0e-68a93e1de63f
📒 Files selected for processing (11)
docs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamldocs/quickstart/go-sdk/context-keys.mdxframework/configstore/rdb.goframework/configstore/store.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.ts
✅ Files skipped from review due to trivial changes (4)
- docs/openapi/paths/management/mcp.yaml
- transports/changelog.md
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- framework/configstore/store.go
Pratham-Mishra04
force-pushed
the
03-24-feat_adds_option_to_automatically_add_new_mcp_server_to_assigned_vks
branch
from
e75dd87 to
384e98f
Compare
Pratham-Mishra04
force-pushed
the
03-24-feat_adds_option_to_automatically_add_new_mcp_server_to_assigned_vks
branch
from
384e98f to
a4c2716
Compare
Pratham-Mishra04
force-pushed
the
03-24-feat_adds_option_to_automatically_add_new_mcp_server_to_assigned_vks
branch
from
a4c2716 to
016b3a6
Compare
Collaborator
Author
Merge activity
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/mcp.go (1)
119-145:⚠️ Potential issue | 🟡 MinorPopulate
VKConfigsin the non-paginated path for API consistencyThe non-paginated path (lines 132–143) doesn't populate
VKConfigsin the response, while the paginated path does. This creates an inconsistent API response shape: the field remains empty for non-paginated requests but is populated for paginated ones.Fetch VK configs from the database in the non-paginated path as well (similar to the paginated path at line 250–256) to ensure both paths return the same response structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcp.go` around lines 119 - 145, The non-paginated branch that builds clients from configsInStore/connectedClientsMap currently never fills VKConfigs on the MCPClientResponse, causing inconsistent response shapes; update that branch to fetch the VK configs the same way the paginated path does (reuse the DB call used around lines handling VK configs) and set MCPClientResponse.VKConfigs to the fetched/redacted VK configs (using the same helper or store method you use in the paginated path) when constructing the MCPClientResponse in both the connected and error branches so VKConfigs is populated for every client.
♻️ Duplicate comments (2)
docs/quickstart/go-sdk/context-keys.mdx (1)
299-303:⚠️ Potential issue | 🟡 MinorDrop the unused
timeimport from the complete example.The example never references
time, so copy-pasting the full snippet still leaves a compile error.🧹 Minimal fix
import ( "context" "fmt" "log" - "time" "github.com/maximhq/bifrost" "github.com/maximhq/bifrost/core/schemas" )Read-only check: inspect the complete example block and confirm there is no
time.usage in that snippet. Expected result: the usage list is empty.#!/bin/bash python - <<'PY' from pathlib import Path lines = Path("docs/quickstart/go-sdk/context-keys.mdx").read_text().splitlines() start, end = 296, 350 snippet = lines[start - 1:end] print("\n".join(f"{i+start}: {line}" for i, line in enumerate(snippet))) uses = [i + start for i, line in enumerate(snippet) if "time." in line] print("\n`time.` usage lines:", uses if uses else "none") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/quickstart/go-sdk/context-keys.mdx` around lines 299 - 303, The import block in the example (`import ( "context" "fmt" "log" "time" )`) contains an unused "time" entry; remove "time" from that import list in the complete example (the context-keys example snippet) so the example compiles cleanly, then verify there are no remaining "time." usages in the snippet.ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx (1)
780-781:⚠️ Potential issue | 🟠 MajorKeep Virtual Key Access visible when no tools are loaded.
The outer guard still drops this entire section when tool discovery fails, which also hides existing assignments and blocks revocation. Render it regardless, then fall back to wildcard-only or disabled tool controls until tools are available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 780 - 781, The JSX block currently gated by "mcpClient.tools && mcpClient.tools.length > 0" hides the entire "Virtual Key Access" section when tool discovery fails; remove that outer guard so the section always renders, then inside the section update the logic to conditionally render tool-specific controls based on "mcpClient.tools" (e.g., if mcpClient.tools?.length > 0 render full tool controls) and otherwise render a fallback UI that shows existing assignments and revocation controls plus either a wildcard-only assignment or disabled tool controls until tools are available; adjust any buttons or inputs to be disabled when tools are missing and keep revocation/assignment UI visible so users can manage keys even if tools are not loaded.
🤖 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/quickstart/go-sdk/context-keys.mdx`:
- Around line 77-82: Replace the realistic-looking API key string used in the
direct-key example so secret scanners won't flag it: in the bfCtx.SetValue(...)
call that sets schemas.BifrostContextKeyDirectKey with a schemas.Key, change the
Value from "sk-direct-api-key" to an obviously synthetic placeholder (e.g.,
"DIRECT_KEY_PLACEHOLDER" or "example-direct-key-000") while leaving Models and
Weight untouched.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 220-225: The call to GetVirtualKeyMCPConfigsByMCPClientIDs
currently swallows errors; update the branch so when that call returns a non-nil
err you log the error (including context such as dbClientIDs) to the handler
logger (e.g., h.logger or the handler’s logging facility) instead of ignoring
it; keep the existing loop that populates assignmentsByClientID when err == nil
and add an else block that logs the error with a clear message referencing
GetVirtualKeyMCPConfigsByMCPClientIDs and dbClientIDs so operators can see why
vk_configs may be empty.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 805-812: The VK search Input lacks a stable test selector; update
the Input in mcpClientSheet.tsx (the one using vkSearch and setVKSearch, with
onChange and onKeyDown handlers) to include a data-testid attribute following
the project pattern (e.g., data-testid="virtualkey-search-input" or
"virtual-key-search-input" to match entity-element-qualifier), so E2E tests
target a stable selector instead of placeholder text.
- Around line 864-873: The icon-only remove Button lacks an accessible name;
update the Button in mcpClientSheet.tsx (the one calling
removeVKConfig(vc.virtual_key_id)) to include an aria-label (or aria-labelledby)
that describes the action and the target, e.g. aria-label={`Remove virtual key
${vc.virtual_key_id}`} (or use the row's visible name if available) so assistive
tech can identify which VK the button removes; keep the onClick and data-testid
unchanged.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 119-145: The non-paginated branch that builds clients from
configsInStore/connectedClientsMap currently never fills VKConfigs on the
MCPClientResponse, causing inconsistent response shapes; update that branch to
fetch the VK configs the same way the paginated path does (reuse the DB call
used around lines handling VK configs) and set MCPClientResponse.VKConfigs to
the fetched/redacted VK configs (using the same helper or store method you use
in the paginated path) when constructing the MCPClientResponse in both the
connected and error branches so VKConfigs is populated for every client.
---
Duplicate comments:
In `@docs/quickstart/go-sdk/context-keys.mdx`:
- Around line 299-303: The import block in the example (`import ( "context"
"fmt" "log" "time" )`) contains an unused "time" entry; remove "time" from that
import list in the complete example (the context-keys example snippet) so the
example compiles cleanly, then verify there are no remaining "time." usages in
the snippet.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 780-781: The JSX block currently gated by "mcpClient.tools &&
mcpClient.tools.length > 0" hides the entire "Virtual Key Access" section when
tool discovery fails; remove that outer guard so the section always renders,
then inside the section update the logic to conditionally render tool-specific
controls based on "mcpClient.tools" (e.g., if mcpClient.tools?.length > 0 render
full tool controls) and otherwise render a fallback UI that shows existing
assignments and revocation controls plus either a wildcard-only assignment or
disabled tool controls until tools are available; adjust any buttons or inputs
to be disabled when tools are missing and keep revocation/assignment UI visible
so users can manage keys even if tools are not loaded.
🪄 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: b6e7c7c6-d54c-4939-aa38-d7f43871c0bc
📒 Files selected for processing (12)
docs/openapi/openapi.jsondocs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamldocs/quickstart/go-sdk/context-keys.mdxframework/configstore/rdb.goframework/configstore/store.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.ts
✅ Files skipped from review due to trivial changes (3)
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- docs/openapi/paths/management/mcp.yaml
- transports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (3)
- transports/bifrost-http/server/server.go
- ui/lib/types/mcp.ts
- framework/configstore/store.go
Comment on lines
77
to
+82
| ```go | ||
| directKey := schemas.Key{ | ||
| bfCtx.SetValue(schemas.BifrostContextKeyDirectKey, schemas.Key{ | ||
| Value: "sk-direct-api-key", | ||
| Models: []string{"gpt-4o"}, | ||
| Weight: 1.0, | ||
| } | ||
| ctx := context.WithValue(ctx, schemas.BifrostContextKeyDirectKey, directKey) | ||
| }) |
Contributor
There was a problem hiding this comment.
Use a non-secret-looking placeholder in the direct-key example.
sk-direct-api-key is close enough to a real credential format that secret scanners will keep flagging this docs block. Use an obviously synthetic token instead.
🔐 Minimal fix
bfCtx.SetValue(schemas.BifrostContextKeyDirectKey, schemas.Key{
- Value: "sk-direct-api-key",
+ Value: "example-direct-key",
Models: []string{"gpt-4o"},
Weight: 1.0,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ```go | |
| directKey := schemas.Key{ | |
| bfCtx.SetValue(schemas.BifrostContextKeyDirectKey, schemas.Key{ | |
| Value: "sk-direct-api-key", | |
| Models: []string{"gpt-4o"}, | |
| Weight: 1.0, | |
| } | |
| ctx := context.WithValue(ctx, schemas.BifrostContextKeyDirectKey, directKey) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/quickstart/go-sdk/context-keys.mdx` around lines 77 - 82, Replace the
realistic-looking API key string used in the direct-key example so secret
scanners won't flag it: in the bfCtx.SetValue(...) call that sets
schemas.BifrostContextKeyDirectKey with a schemas.Key, change the Value from
"sk-direct-api-key" to an obviously synthetic placeholder (e.g.,
"DIRECT_KEY_PLACEHOLDER" or "example-direct-key-000") while leaving Models and
Weight untouched.
Comment on lines
+220
to
+225
| if allAssignments, err := h.store.ConfigStore.GetVirtualKeyMCPConfigsByMCPClientIDs(ctx, dbClientIDs); err == nil { | ||
| for _, a := range allAssignments { | ||
| assignmentsByClientID[a.MCPClientID] = append(assignmentsByClientID[a.MCPClientID], a) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Log the error when VK assignment batch-fetch fails.
The error from GetVirtualKeyMCPConfigsByMCPClientIDs is silently swallowed. If this fails, responses will have empty vk_configs without any indication of why. At minimum, log the error for operator visibility.
🛡️ Proposed fix
- if allAssignments, err := h.store.ConfigStore.GetVirtualKeyMCPConfigsByMCPClientIDs(ctx, dbClientIDs); err == nil {
+ allAssignments, err := h.store.ConfigStore.GetVirtualKeyMCPConfigsByMCPClientIDs(ctx, dbClientIDs)
+ if err != nil {
+ logger.Error("failed to fetch VK MCP configs for clients: %v", err)
+ } else {
for _, a := range allAssignments {
assignmentsByClientID[a.MCPClientID] = append(assignmentsByClientID[a.MCPClientID], a)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if allAssignments, err := h.store.ConfigStore.GetVirtualKeyMCPConfigsByMCPClientIDs(ctx, dbClientIDs); err == nil { | |
| for _, a := range allAssignments { | |
| assignmentsByClientID[a.MCPClientID] = append(assignmentsByClientID[a.MCPClientID], a) | |
| } | |
| } | |
| } | |
| allAssignments, err := h.store.ConfigStore.GetVirtualKeyMCPConfigsByMCPClientIDs(ctx, dbClientIDs) | |
| if err != nil { | |
| logger.Error("failed to fetch VK MCP configs for clients: %v", err) | |
| } else { | |
| for _, a := range allAssignments { | |
| assignmentsByClientID[a.MCPClientID] = append(assignmentsByClientID[a.MCPClientID], a) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/bifrost-http/handlers/mcp.go` around lines 220 - 225, The call to
GetVirtualKeyMCPConfigsByMCPClientIDs currently swallows errors; update the
branch so when that call returns a non-nil err you log the error (including
context such as dbClientIDs) to the handler logger (e.g., h.logger or the
handler’s logging facility) instead of ignoring it; keep the existing loop that
populates assignmentsByClientID when err == nil and add an else block that logs
the error with a clear message referencing GetVirtualKeyMCPConfigsByMCPClientIDs
and dbClientIDs so operators can see why vk_configs may be empty.
Comment on lines
+805
to
+812
| <div className="px-2 pb-1"> | ||
| <Input | ||
| placeholder="Search virtual keys..." | ||
| value={vkSearch} | ||
| onChange={(e) => setVKSearch(e.target.value)} | ||
| onKeyDown={(e) => e.stopPropagation()} | ||
| className="h-7 text-sm" | ||
| /> |
Contributor
There was a problem hiding this comment.
Add a stable selector to the VK search input.
The searchable add flow still has one untagged control, which makes E2E fall back to placeholder text.
🧪 Minimal fix
<Input
+ data-testid="mcpclient-virtualkey-search"
placeholder="Search virtual keys..."
value={vkSearch}
onChange={(e) => setVKSearch(e.target.value)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 805 -
812, The VK search Input lacks a stable test selector; update the Input in
mcpClientSheet.tsx (the one using vkSearch and setVKSearch, with onChange and
onKeyDown handlers) to include a data-testid attribute following the project
pattern (e.g., data-testid="virtualkey-search-input" or
"virtual-key-search-input" to match entity-element-qualifier), so E2E tests
target a stable selector instead of placeholder text.
Comment on lines
+864
to
+873
| <Button | ||
| type="button" | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => removeVKConfig(vc.virtual_key_id)} | ||
| className="text-muted-foreground hover:text-destructive" | ||
| data-testid={`mcpclient-virtualkey-remove-${vc.virtual_key_id}`} | ||
| > | ||
| <Trash2 className="h-4 w-4" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Give the remove action an accessible name.
This button is icon-only right now, so assistive tech has no label for which VK access row it removes.
♿ Minimal fix
<Button
type="button"
variant="ghost"
size="icon"
+ aria-label={`Remove ${vkNameByID[vc.virtual_key_id] ?? "virtual key"} access`}
onClick={() => removeVKConfig(vc.virtual_key_id)}
className="text-muted-foreground hover:text-destructive"
data-testid={`mcpclient-virtualkey-remove-${vc.virtual_key_id}`}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 864 -
873, The icon-only remove Button lacks an accessible name; update the Button in
mcpClientSheet.tsx (the one calling removeVKConfig(vc.virtual_key_id)) to
include an aria-label (or aria-labelledby) that describes the action and the
target, e.g. aria-label={`Remove virtual key ${vc.virtual_key_id}`} (or use the
row's visible name if available) so assistive tech can identify which VK the
button removes; keep the onClick and data-testid unchanged.
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…
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
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
vk_configsfield to MCP client update API that accepts an array of virtual key configurationsvk_configsis provided, it atomically replaces all existing VK assignments for the MCP clientGetVirtualKeyMCPConfigsByMCPClientIDto retrieve VK configs by MCP clientMCPContextKeyIncludeClients,MCPContextKeyIncludeTools, andBifrostContextKeyMCPExtraHeadersType of change
Affected areas
How to test
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"] } ] }'Screenshots/Recordings
The UI now includes a "Virtual Key Access" section in the MCP client configuration sheet where administrators can:
Breaking changes
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
Checklist
docs/contributing/README.mdand followed the guidelines