feat: add request-level extra headers support for MCP tool execution#2126
Merged
Pratham-Mishra04 merged 1 commit intov1.5.0from Mar 18, 2026
Conversation
18 tasks
|
|
Collaborator
Author
This was referenced Mar 17, 2026
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (30)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds request-level extra-header forwarding for MCP tool execution via an allowlist (AllowedExtraHeaders) and a BifrostContext key; threads combined allowlists into context construction, header-merging utils, DB schema/migrations, handlers, tests, UI, and changes CodeMode ExecuteTool to accept *schemas.BifrostContext. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (HTTP)
participant Handler as HTTP Handler
participant CtxBuilder as ConvertToBifrostContext
participant Manager as MCP Manager / CodeMode
participant Utils as GetHeadersForToolExecution
participant MCP as MCP Server
Client->>Handler: HTTP request with x-bf-eh-* headers + tool call
Handler->>CtxBuilder: ConvertToBifrostContext(..., combinedAllowlist)
CtxBuilder->>CtxBuilder: Collect allowed MCP extra headers
CtxBuilder-->>Handler: *schemas.BifrostContext (includes MCP extra headers)
Handler->>Manager: ExecuteTool(ctx *BifrostContext, toolCall)
Manager->>Utils: GetHeadersForToolExecution(ctx, client)
Utils->>Utils: Merge client headers + filtered MCP extra headers
Utils-->>Manager: http.Header (merged)
Manager->>MCP: Send tool request with merged headers
MCP-->>Manager: Tool response
Manager-->>Handler: Result / ChatMessage
Handler-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
📝 Coding Plan
Comment |
18 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
transports/bifrost-http/handlers/asyncinference.go (1)
111-128:⚠️ Potential issue | 🟠 MajorCarry request-scoped MCP headers into the async job context.
These handlers now build a request
bifrostCtx, butSubmitJob()only getsvirtualKeyValue/resultTTLand later invokes the callback with a freshbgCtx. That drops theschemas.BifrostContextKeyMCPExtraHeaderspopulated byConvertToBifrostContext(), so async requests that trigger MCP tools will silently lose the new request-level headers. Persist the relevant request-scoped values with the job, or copy them frombifrostCtxintobgCtxinside the callback before calling the client.Also applies to: 149-166, 187-204, 221-238, 259-276, 297-314, 335-352, 373-390, 406-423, 439-456
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/asyncinference.go` around lines 111 - 128, The async handlers build a request bifrostCtx via ConvertToBifrostContext but SubmitJob is invoked only with virtualKeyValue/resultTTL so the request-scoped MCP headers (schemas.BifrostContextKeyMCPExtraHeaders) are lost when the executor later creates bgCtx; update the SubmitJob invocation in these handlers (e.g., where getVirtualKeyFromContext, getResultTTLFromHeaderWithDefault and executor.SubmitJob are used) to either pass the MCP extra-headers into the job payload or copy the headers from bifrostCtx into the provided bgCtx inside the job callback before calling client.* (e.g., client.TextCompletionRequest), ensuring bgCtx contains the original request-level MCP headers. Ensure the same change is applied to all listed handler blocks.transports/bifrost-http/lib/ctx.go (1)
148-164:⚠️ Potential issue | 🔴 CriticalKeep reserved and denied headers out of
mcpExtraHeaders.Line 423 appends any allowlisted header without consulting the denylist from Lines 150-164, and Line 425 returns before the later
x-bf-send-back-raw-response,x-bf-parent-request-id, andx-bf-passthrough-extra-paramshandlers run. WithAllowedExtraHeaders=["*"]or an explicit reserved name, this can both swallow existing Bifrost control headers and make inbound auth headers likex-api-keyeligible for forwarding to external MCP servers.🔒 Suggested fix
- // Handle MCP extra headers - if mcpHeaderCombinedAllowlist.IsAllowed(keyStr) { - mcpExtraHeaders[keyStr] = append(mcpExtraHeaders[keyStr], string(value)) - return true - } // Send back raw response header if keyStr == "x-bf-send-back-raw-response" { if valueStr := string(value); valueStr == "true" { bifrostCtx.SetValue(schemas.BifrostContextKeySendBackRawResponse, true) } @@ if keyStr == "x-bf-passthrough-extra-params" { if valueStr := string(value); valueStr == "true" { bifrostCtx.SetValue(schemas.BifrostContextKeyPassthroughExtraParams, true) } return true } + // Handle MCP extra headers after reserved bifrost headers are processed. + if mcpHeaderCombinedAllowlist.IsAllowed(keyStr) { + if securityDenylist[keyStr] { + return true + } + mcpExtraHeaders[keyStr] = append(mcpExtraHeaders[keyStr], string(value)) + return true + } return true })Also applies to: 422-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 148 - 164, When building mcpExtraHeaders in the function that handles AllowedExtraHeaders, ensure you consult securityDenylist before appending any allowlisted header: skip/ignore headers present in securityDenylist (e.g., "x-api-key", "cookie", etc.) so they are never added to mcpExtraHeaders even when AllowedExtraHeaders contains "*" or the explicit name; also avoid the early return that happens after appending (the block around the mcpExtraHeaders append and the return on the path that handles allowlisted headers) so that subsequent handlers for x-bf-send-back-raw-response, x-bf-parent-request-id, and x-bf-passthrough-extra-params still run—modify the logic around mcpExtraHeaders, the AllowedExtraHeaders check, and the return so denylisted headers are filtered out and reserved Bifrost control headers always get their handlers executed (reference symbols: mcpExtraHeaders, AllowedExtraHeaders, securityDenylist, x-bf-send-back-raw-response, x-bf-parent-request-id, x-bf-passthrough-extra-params).
🧹 Nitpick comments (5)
framework/configstore/tables/mcp.go (1)
207-211: Normalize emptyallowed_extra_headers_jsonon read for deterministic semantics.For legacy rows where this column may be empty, consider explicitly setting
AllowedExtraHeadersto an empty whitelist to avoid nil-semantics ambiguity at runtime.Suggested edit
- if c.AllowedExtraHeadersJSON != "" { - if err := sonic.Unmarshal([]byte(c.AllowedExtraHeadersJSON), &c.AllowedExtraHeaders); err != nil { - return err - } - } + if c.AllowedExtraHeadersJSON == "" { + c.AllowedExtraHeaders = schemas.WhiteList{} + } else { + if err := sonic.Unmarshal([]byte(c.AllowedExtraHeadersJSON), &c.AllowedExtraHeaders); err != nil { + return err + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/mcp.go` around lines 207 - 211, Normalize legacy empty AllowedExtraHeadersJSON by initializing c.AllowedExtraHeaders to an explicit empty whitelist when AllowedExtraHeadersJSON == "" instead of leaving it nil: in the same block that currently checks c.AllowedExtraHeadersJSON, add logic to set c.AllowedExtraHeaders to an empty collection (matching its declared type) when the JSON string is empty; otherwise continue to sonic.Unmarshal into c.AllowedExtraHeaders and return any error. Ensure you reference the fields AllowedExtraHeadersJSON and AllowedExtraHeaders in your change so callers see deterministic, non-nil semantics.core/mcp/utils/utils.go (1)
23-46: The intermediatefilteredHeadersmap is unnecessary.The extra headers can be written directly to
headers, simplifying the code and avoiding an extra allocation.♻️ Simplified implementation
// Give priority to extra headers in the context if extraHeaders, ok := ctx.Value(schemas.BifrostContextKeyMCPExtraHeaders).(map[string][]string); ok { - filteredHeaders := make(http.Header) for key, values := range extraHeaders { if client.ExecutionConfig.AllowedExtraHeaders.IsAllowed(key) { for i, value := range values { if i == 0 { - filteredHeaders.Set(key, value) + headers.Set(key, value) } else { - filteredHeaders.Add(key, value) + headers.Add(key, value) } } } } - // Add the filtered headers to the headers - if len(filteredHeaders) > 0 { - for k, values := range filteredHeaders { - for i, v := range values { - if i == 0 { - headers.Set(k, v) - } else { - headers.Add(k, v) - } - } - } - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/mcp/utils/utils.go` around lines 23 - 46, Remove the unnecessary intermediate filteredHeaders map and write allowed extraHeaders directly into headers: iterate extraHeaders, call client.ExecutionConfig.AllowedExtraHeaders.IsAllowed(key) and when allowed write values into headers using Set for the first value and Add for subsequent values (same logic currently used), and remove the separate filteredHeaders creation and the subsequent copy/len(filteredHeaders) block; keep references to extraHeaders, headers and client.ExecutionConfig.AllowedExtraHeaders.IsAllowed to locate the change.ui/lib/types/schemas.ts (1)
926-936: Add duplicate validation forallowed_extra_headersYou already enforce wildcard exclusivity; adding uniqueness validation would prevent redundant/ambiguous config values.
♻️ Suggested refinement
allowed_extra_headers: z .array(z.string()) .optional() .refine( (headers) => { if (!headers || headers.length === 0) return true; const hasWildcard = headers.includes("*"); return !hasWildcard || headers.length === 1; }, { message: "Wildcard '*' cannot be combined with specific header names" }, - ), + ) + .refine( + (headers) => { + if (!headers) return true; + return headers.length === new Set(headers).size; + }, + { message: "Duplicate header names are not allowed" }, + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/schemas.ts` around lines 926 - 936, The existing zod schema for allowed_extra_headers (the array refine that checks wildcard exclusivity) should also enforce uniqueness to prevent duplicate header entries; update the validation on allowed_extra_headers (the .refine/.superRefine block) to check that if headers exists then new Set(headers).size === headers.length (or otherwise detect duplicates) and return a validation error like "Duplicate header names are not allowed" (or include alongside the existing wildcard message) so duplicates are rejected alongside the wildcard check.transports/bifrost-http/handlers/mcpinference.go (1)
15-25: Keep the standard handler dependency shape here.This handler now carries only
config, unlike the other HTTP handlers that inject bothhandlerStoreandconfig. Keeping the sameclient/handlerStore/configshape avoids special cases when auth or request-context behavior expands again here.As per coding guidelines "transports/bifrost-http/handlers/*.go: HTTP transport handlers must be structs with injected dependencies (client, handlerStore, config) implementing RegisterRoutes()."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcpinference.go` around lines 15 - 25, The MCPInferenceHandler currently lacks the standard injected handlerStore dependency; update the struct to include handlerStore (e.g., add a field handlerStore *handlers.Store or the same type used across other handlers), modify NewMCPInferenceHandler to accept and set that handlerStore parameter in addition to client and config, and ensure the resulting MCPInferenceHandler still implements RegisterRoutes() so it matches the client/handlerStore/config dependency shape used by other HTTP handlers (reference MCPInferenceHandler and NewMCPInferenceHandler to locate the changes).transports/bifrost-http/lib/config_test.go (1)
492-502: Consolidate MCP client mapping to a helper to prevent future field drift.The same
tables.TableMCPClient -> schemas.MCPClientConfigmapping is duplicated in two branches. A single helper will reduce maintenance misses when new fields are added (likeAllowedExtraHeaders).♻️ Proposed refactor
+func toSchemaMCPClientConfig(clientConfig *tables.TableMCPClient) *schemas.MCPClientConfig { + return &schemas.MCPClientConfig{ + ID: clientConfig.ClientID, + Name: clientConfig.Name, + IsCodeModeClient: clientConfig.IsCodeModeClient, + ConnectionType: schemas.MCPConnectionType(clientConfig.ConnectionType), + ConnectionString: clientConfig.ConnectionString, + StdioConfig: clientConfig.StdioConfig, + Headers: clientConfig.Headers, + ToolsToExecute: clientConfig.ToolsToExecute, + ToolsToAutoExecute: clientConfig.ToolsToAutoExecute, + AllowedExtraHeaders: clientConfig.AllowedExtraHeaders, + } +} + func (m *MockConfigStore) UpdateMCPClientConfig(ctx context.Context, id string, clientConfig *tables.TableMCPClient) error { @@ for i := range m.mcpConfig.ClientConfigs { if m.mcpConfig.ClientConfigs[i].ID == id { - m.mcpConfig.ClientConfigs[i] = &schemas.MCPClientConfig{ - ID: clientConfig.ClientID, - Name: clientConfig.Name, - IsCodeModeClient: clientConfig.IsCodeModeClient, - ConnectionType: schemas.MCPConnectionType(clientConfig.ConnectionType), - ConnectionString: clientConfig.ConnectionString, - StdioConfig: clientConfig.StdioConfig, - Headers: clientConfig.Headers, - ToolsToExecute: clientConfig.ToolsToExecute, - ToolsToAutoExecute: clientConfig.ToolsToAutoExecute, - AllowedExtraHeaders: clientConfig.AllowedExtraHeaders, - } + m.mcpConfig.ClientConfigs[i] = toSchemaMCPClientConfig(clientConfig) return nil } } @@ - m.mcpConfig.ClientConfigs = append(m.mcpConfig.ClientConfigs, &schemas.MCPClientConfig{ - ID: clientConfig.ClientID, - Name: clientConfig.Name, - IsCodeModeClient: clientConfig.IsCodeModeClient, - ConnectionType: schemas.MCPConnectionType(clientConfig.ConnectionType), - ConnectionString: clientConfig.ConnectionString, - StdioConfig: clientConfig.StdioConfig, - Headers: clientConfig.Headers, - ToolsToExecute: clientConfig.ToolsToExecute, - ToolsToAutoExecute: clientConfig.ToolsToAutoExecute, - AllowedExtraHeaders: clientConfig.AllowedExtraHeaders, - }) + m.mcpConfig.ClientConfigs = append(m.mcpConfig.ClientConfigs, toSchemaMCPClientConfig(clientConfig))Also applies to: 508-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config_test.go` around lines 492 - 502, Duplicate mapping from tables.TableMCPClient to schemas.MCPClientConfig exists in two places; create a single helper (e.g., mapTableMCPClientToSchema or buildMCPClientConfig) that accepts a tables.TableMCPClient and returns a schemas.MCPClientConfig with all fields (ID, Name, IsCodeModeClient, ConnectionType, ConnectionString, StdioConfig, Headers, ToolsToExecute, ToolsToAutoExecute, AllowedExtraHeaders) and replace both inline mapping blocks in the test with calls to that helper so future field additions won’t drift between branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/changelog.md`:
- Line 5: Update the changelog entry to hyphenate "request-level" so it reads:
"feat: add support for request-level extra headers in MCP tool execution using
BifrostContextKeyMCPExtraHeaders key in context." Locate the line containing the
BifrostContextKeyMCPExtraHeaders mention in core/changelog.md and replace
"request level" with "request-level" to correct the wording.
In `@core/mcp/toolmanager.go`:
- Line 556: GetHeadersForToolExecution currently appends request-level headers
onto existing connection headers which can produce duplicate singleton headers
(e.g., Authorization) in the Header field of CallToolRequest; change the merge
so request-level values replace existing keys rather than being appended. Locate
the call that sets Header: utils.GetHeadersForToolExecution(ctx, client) when
building the CallToolRequest in toolmanager.go and update the merge logic inside
GetHeadersForToolExecution (or immediately after calling it) to deduplicate by
header name: iterate connection headers into a map keyed by canonical header
name, overwrite entries with request-level headers from the incoming context,
then emit a single list of headers with the request-level values replacing
connection-level values before assigning to CallToolRequest.Header.
In `@core/mcp/utils/utils.go`:
- Around line 11-14: GetHeadersForToolExecution currently assumes ctx is non-nil
and calls ctx.Value which can panic; add a defensive nil check at the start of
GetHeadersForToolExecution (the function name) to return an empty http.Header if
ctx == nil (similar to the existing client/ExecutionConfig nil guard), or
otherwise safely handle missing context before calling ctx.Value, ensuring any
subsequent usage of ctx.Value("request-id") or other keys is only performed when
ctx is non-nil.
In `@core/schemas/mcp.go`:
- Line 90: Update the comment on the AllowedExtraHeaders field (type WhiteList)
to reflect its actual runtime behavior: state that it is an execution-time
allowlist used to forward request-level headers to MCP tool calls (independent
of any specific auth type), rather than implying it is tied to headers auth;
keep the field name AllowedExtraHeaders and type WhiteList unchanged.
In `@framework/configstore/rdb.go`:
- Line 993: The code takes the address of a non-pointer schema field
(clientConfigCopy.IsPingAvailable) which collapses the tri-state into &false and
prevents the DB default true from applying; change the schema field
IsPingAvailable from bool to *bool so tri-state is preserved, update any
constructors/initializers that populate clientConfigCopy and ensure callers set
nil when unspecified (so the DB default can apply) and only set &true/&false
when explicitly provided; alternatively, if you prefer not to change the schema
type, add explicit defaulting before constructing the table record by setting
the table's IsPingAvailable pointer to nil when the input did not specify a
value so the DB default remains usable.
In `@transports/bifrost-http/lib/config.go`:
- Around line 2529-2538: GetMCPHeaderCombinedAllowlist is reading c.MCPConfig
and iterating c.MCPConfig.ClientConfigs without a nil check or synchronization,
which can panic when MCPConfig is nil and races with writers; modify
GetMCPHeaderCombinedAllowlist to first acquire the muMCP lock (use the same
mutex guarding MCP add/update/remove), defer its release, check if c.MCPConfig
is nil and return empty schemas.WhiteList if so, then iterate
c.MCPConfig.ClientConfigs under the lock and build/return the combined allowlist
(return {"*"} early if any AllowedExtraHeaders.IsUnrestricted()).
In `@transports/changelog.md`:
- Line 5: Update the changelog entry text "feat: add support for request level
extra headers in MCP tool execution." to use the correct hyphenation for the
compound modifier by changing "request level" to "request-level" so the line
reads "feat: add support for request-level extra headers in MCP tool execution."
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 410-418: Add a stable data-testid to the new allowlist input so
tests can reliably select it: update the JSX Input component (the Input with
placeholder "*, or: authorization, x-user-id" that uses
defaultValue={headersText} and onBlur={(e) => { ... field.onChange(parsed); }})
to include data-testid="mcp-client-allowed-extra-headers-input". Ensure the
attribute is added directly on that Input element so it follows the repo
convention for UI interactive elements.
- Around line 387-427: The Input inside the FormField for allowed_extra_headers
is currently uncontrolled (uses defaultValue and manual onBlur) so form.reset()
and immediate submissions can show stale UI state; replace the uncontrolled
wiring by passing the react-hook-form binding props from render's field into the
Input: value={field.value}, name={field.name}, ref={field.ref},
onBlur={field.onBlur} and ensure you also call field.onChange on user edits
(e.g., in onChange parse comma-separated values into an array and call
field.onChange(parsed)); keep the existing parsing/trim logic but move it into
onChange or sync it with field.onChange so the form value is always up-to-date.
Also add a data-testid attribute to the Input following the pattern
"<entity>-<element>-<qualifier>" (for example use something like
"mcpclient-input-allowed-extra-headers") so tests can target this field.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/asyncinference.go`:
- Around line 111-128: The async handlers build a request bifrostCtx via
ConvertToBifrostContext but SubmitJob is invoked only with
virtualKeyValue/resultTTL so the request-scoped MCP headers
(schemas.BifrostContextKeyMCPExtraHeaders) are lost when the executor later
creates bgCtx; update the SubmitJob invocation in these handlers (e.g., where
getVirtualKeyFromContext, getResultTTLFromHeaderWithDefault and
executor.SubmitJob are used) to either pass the MCP extra-headers into the job
payload or copy the headers from bifrostCtx into the provided bgCtx inside the
job callback before calling client.* (e.g., client.TextCompletionRequest),
ensuring bgCtx contains the original request-level MCP headers. Ensure the same
change is applied to all listed handler blocks.
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 148-164: When building mcpExtraHeaders in the function that
handles AllowedExtraHeaders, ensure you consult securityDenylist before
appending any allowlisted header: skip/ignore headers present in
securityDenylist (e.g., "x-api-key", "cookie", etc.) so they are never added to
mcpExtraHeaders even when AllowedExtraHeaders contains "*" or the explicit name;
also avoid the early return that happens after appending (the block around the
mcpExtraHeaders append and the return on the path that handles allowlisted
headers) so that subsequent handlers for x-bf-send-back-raw-response,
x-bf-parent-request-id, and x-bf-passthrough-extra-params still run—modify the
logic around mcpExtraHeaders, the AllowedExtraHeaders check, and the return so
denylisted headers are filtered out and reserved Bifrost control headers always
get their handlers executed (reference symbols: mcpExtraHeaders,
AllowedExtraHeaders, securityDenylist, x-bf-send-back-raw-response,
x-bf-parent-request-id, x-bf-passthrough-extra-params).
---
Nitpick comments:
In `@core/mcp/utils/utils.go`:
- Around line 23-46: Remove the unnecessary intermediate filteredHeaders map and
write allowed extraHeaders directly into headers: iterate extraHeaders, call
client.ExecutionConfig.AllowedExtraHeaders.IsAllowed(key) and when allowed write
values into headers using Set for the first value and Add for subsequent values
(same logic currently used), and remove the separate filteredHeaders creation
and the subsequent copy/len(filteredHeaders) block; keep references to
extraHeaders, headers and client.ExecutionConfig.AllowedExtraHeaders.IsAllowed
to locate the change.
In `@framework/configstore/tables/mcp.go`:
- Around line 207-211: Normalize legacy empty AllowedExtraHeadersJSON by
initializing c.AllowedExtraHeaders to an explicit empty whitelist when
AllowedExtraHeadersJSON == "" instead of leaving it nil: in the same block that
currently checks c.AllowedExtraHeadersJSON, add logic to set
c.AllowedExtraHeaders to an empty collection (matching its declared type) when
the JSON string is empty; otherwise continue to sonic.Unmarshal into
c.AllowedExtraHeaders and return any error. Ensure you reference the fields
AllowedExtraHeadersJSON and AllowedExtraHeaders in your change so callers see
deterministic, non-nil semantics.
In `@transports/bifrost-http/handlers/mcpinference.go`:
- Around line 15-25: The MCPInferenceHandler currently lacks the standard
injected handlerStore dependency; update the struct to include handlerStore
(e.g., add a field handlerStore *handlers.Store or the same type used across
other handlers), modify NewMCPInferenceHandler to accept and set that
handlerStore parameter in addition to client and config, and ensure the
resulting MCPInferenceHandler still implements RegisterRoutes() so it matches
the client/handlerStore/config dependency shape used by other HTTP handlers
(reference MCPInferenceHandler and NewMCPInferenceHandler to locate the
changes).
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 492-502: Duplicate mapping from tables.TableMCPClient to
schemas.MCPClientConfig exists in two places; create a single helper (e.g.,
mapTableMCPClientToSchema or buildMCPClientConfig) that accepts a
tables.TableMCPClient and returns a schemas.MCPClientConfig with all fields (ID,
Name, IsCodeModeClient, ConnectionType, ConnectionString, StdioConfig, Headers,
ToolsToExecute, ToolsToAutoExecute, AllowedExtraHeaders) and replace both inline
mapping blocks in the test with calls to that helper so future field additions
won’t drift between branches.
In `@ui/lib/types/schemas.ts`:
- Around line 926-936: The existing zod schema for allowed_extra_headers (the
array refine that checks wildcard exclusivity) should also enforce uniqueness to
prevent duplicate header entries; update the validation on allowed_extra_headers
(the .refine/.superRefine block) to check that if headers exists then new
Set(headers).size === headers.length (or otherwise detect duplicates) and return
a validation error like "Duplicate header names are not allowed" (or include
alongside the existing wildcard message) so duplicates are rejected alongside
the wildcard check.
🪄 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: ba36359a-806d-4c5d-a034-2fd0746a3d03
📒 Files selected for processing (29)
core/changelog.mdcore/mcp/clientmanager.gocore/mcp/codemode.gocore/mcp/codemode/starlark/executecode.gocore/mcp/codemode/starlark/starlark.gocore/mcp/toolmanager.gocore/mcp/utils/utils.gocore/schemas/bifrost.gocore/schemas/mcp.goexamples/mcps/auth-demo-server/main.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_shared_test.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
Pratham-Mishra04
force-pushed
the
03-17-feat_added_support_for_extra_headers_in_mcp_execute_tool
branch
from
36cec44 to
e5ae59c
Compare
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
ea850ee to
189d310
Compare
12 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
transports/changelog.md (1)
5-5:⚠️ Potential issue | 🟡 MinorHyphenate “request-level” in the changelog entry.
Line 5 should use the compound modifier form: request-level.
Suggested fix
-- feat: add support for request level extra headers in MCP tool execution. +- feat: add support for request-level extra headers in MCP tool execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/changelog.md` at line 5, Update the changelog entry text that reads "add support for request level extra headers in MCP tool execution." to use the hyphenated compound modifier "request-level" so it reads "add support for request-level extra headers in MCP tool execution."; locate the exact changelog line containing that sentence and replace the phrase "request level" with "request-level".
🧹 Nitpick comments (1)
transports/bifrost-http/lib/config_test.go (1)
492-502: Copy mutable fields before storing MCP client config snapshots in mock state.
Headers,ToolsToExecute,ToolsToAutoExecute, andAllowedExtraHeadersare assigned directly. If caller-owned data is mutated later, mock state can change unexpectedly and make tests order-dependent. Prefer cloning these fields when writing tom.mcpConfig.ClientConfigs.Also applies to: 508-518
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config_test.go` around lines 492 - 502, The test is storing references to caller-owned mutable fields into m.mcpConfig.ClientConfigs (the struct created from clientConfig), causing shared-state test flakiness; update the assignment that builds the snapshot (the block using clientConfig.ClientID/Name/... and writing into m.mcpConfig.ClientConfigs) to clone mutable fields—make shallow copies of Headers and AllowedExtraHeaders (copy map entries to a new map) and copy slices for ToolsToExecute and ToolsToAutoExecute—before assigning them into the struct; apply the same cloning change to the other analogous assignment block that also writes into m.mcpConfig.ClientConfigs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/mcp/codemode/starlark/executecode.go`:
- Around line 449-461: The warning messages in the nil checks for
s.pluginPipelineProvider and pipeline are misleading because they say "calling
tool directly" while the code actually returns an error; update the logger.Warn
calls in the Execute (or surrounding) method to accurately describe the action
(e.g., "%s Plugin pipeline provider is nil, returning error" and "%s Plugin
pipeline is nil, returning error") or alternatively change the control flow to
call the tool directly if that behavior is desired; adjust the messages that
reference codemcp.CodeModeLogPrefix and the calls around
s.pluginPipelineProvider() and pipeline to remain consistent with the actual
behavior.
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 422-426: The mcp passthrough branch using
mcpHeaderCombinedAllowlist to append into mcpExtraHeaders currently allows
protected/internal headers (e.g., "authorization" and "x-bf-*" control headers)
to be captured; update the conditional in ctx.go around the block that
references mcpHeaderCombinedAllowlist, mcpExtraHeaders, keyStr and value to
first normalize keyStr to lower-case and short-circuit for protected
names/prefixes (at minimum reject "authorization" and any header with prefix
"x-bf-") before appending; implement a small helper or inline check
(isProtectedHeader(keyStr)) so wildcard allowlists cannot override these headers
and return false when protected so downstream handlers still run.
---
Duplicate comments:
In `@transports/changelog.md`:
- Line 5: Update the changelog entry text that reads "add support for request
level extra headers in MCP tool execution." to use the hyphenated compound
modifier "request-level" so it reads "add support for request-level extra
headers in MCP tool execution."; locate the exact changelog line containing that
sentence and replace the phrase "request level" with "request-level".
---
Nitpick comments:
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 492-502: The test is storing references to caller-owned mutable
fields into m.mcpConfig.ClientConfigs (the struct created from clientConfig),
causing shared-state test flakiness; update the assignment that builds the
snapshot (the block using clientConfig.ClientID/Name/... and writing into
m.mcpConfig.ClientConfigs) to clone mutable fields—make shallow copies of
Headers and AllowedExtraHeaders (copy map entries to a new map) and copy slices
for ToolsToExecute and ToolsToAutoExecute—before assigning them into the struct;
apply the same cloning change to the other analogous assignment block that also
writes into m.mcpConfig.ClientConfigs.
🪄 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: 97198e54-b219-4f1d-9529-2146809a3a6a
📒 Files selected for processing (30)
core/changelog.mdcore/mcp/clientmanager.gocore/mcp/codemode.gocore/mcp/codemode/starlark/executecode.gocore/mcp/codemode/starlark/starlark.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/mcp/utils/utils.gocore/schemas/bifrost.gocore/schemas/mcp.goexamples/mcps/auth-demo-server/main.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_shared_test.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- ui/lib/types/mcp.ts
- transports/bifrost-http/handlers/mcp.go
- ui/lib/types/schemas.ts
- transports/bifrost-http/integrations/router.go
- transports/bifrost-http/handlers/mcpserver.go
- transports/bifrost-http/handlers/asyncinference.go
- core/schemas/bifrost.go
- transports/bifrost-http/handlers/inference.go
- core/changelog.md
- framework/changelog.md
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
189d310 to
cb27490
Compare
Pratham-Mishra04
force-pushed
the
03-17-feat_added_support_for_extra_headers_in_mcp_execute_tool
branch
from
e5ae59c to
fe95fe3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/mcp.go (1)
200-215:⚠️ Potential issue | 🟠 MajorPaginated MCP list is dropping
allowed_extra_headers.The paginated path builds
schemas.MCPClientConfigwithout assigningAllowedExtraHeaders, so/api/mcp/clients?limit=...returns incomplete configs compared to the non-paginated path.💡 Suggested fix
clientConfig := &schemas.MCPClientConfig{ ID: dbClient.ClientID, Name: dbClient.Name, IsCodeModeClient: dbClient.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(dbClient.ConnectionType), ConnectionString: dbClient.ConnectionString, StdioConfig: dbClient.StdioConfig, AuthType: schemas.MCPAuthType(dbClient.AuthType), OauthConfigID: dbClient.OauthConfigID, ToolsToExecute: dbClient.ToolsToExecute, ToolsToAutoExecute: dbClient.ToolsToAutoExecute, Headers: dbClient.Headers, + AllowedExtraHeaders: dbClient.AllowedExtraHeaders, IsPingAvailable: &isPingAvailable, ToolSyncInterval: time.Duration(dbClient.ToolSyncInterval) * time.Minute, ToolPricing: dbClient.ToolPricing, }🤖 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 200 - 215, The paginated MCP client construction in mcp.go builds a schemas.MCPClientConfig but omits the AllowedExtraHeaders field; update the code that constructs clientConfig (the local variable named clientConfig of type schemas.MCPClientConfig) to set AllowedExtraHeaders: dbClient.AllowedExtraHeaders (or the equivalent source on dbClient) so the paginated response includes the same AllowedExtraHeaders as the non-paginated path.
♻️ Duplicate comments (2)
transports/changelog.md (1)
5-5:⚠️ Potential issue | 🟡 MinorFix compound modifier hyphenation on Line 5.
Use “request-level” instead of “request level” for correct changelog wording.
Suggested edit
-- feat: add support for request level extra headers in MCP tool execution. +- feat: add support for request-level extra headers in MCP tool execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/changelog.md` at line 5, Update the changelog entry that currently reads "feat: add support for request level extra headers in MCP tool execution." to use the hyphenated compound "request-level" so it reads "feat: add support for request-level extra headers in MCP tool execution."; locate the exact changelog line containing that string and replace "request level" with "request-level".transports/bifrost-http/lib/ctx.go (1)
382-386:⚠️ Potential issue | 🟠 MajorDo not let MCP passthrough consume internal
x-bf-*control headers.At Line 382, this early return can intercept internal control headers (e.g.,
x-bf-send-back-raw-response,x-bf-parent-request-id) before their dedicated handlers run later in the function.🔧 Proposed fix
- // Handle MCP extra headers - if mcpHeaderCombinedAllowlist.IsAllowed(keyStr) { + // Handle MCP extra headers. Never intercept internal x-bf-* controls. + if !strings.HasPrefix(keyStr, "x-bf-") && mcpHeaderCombinedAllowlist.IsAllowed(keyStr) { mcpExtraHeaders[keyStr] = append(mcpExtraHeaders[keyStr], string(value)) return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 382 - 386, The early return in the MCP passthrough block lets internal control headers (like x-bf-*) be consumed before their handlers run; update the block that uses mcpHeaderCombinedAllowlist, mcpExtraHeaders, keyStr and value so it only appends/returns for allowed keys that are NOT internal control headers—for example guard with a check that keyStr does not start with "x-bf-" (or use the existing internal-header predicate) before appending to mcpExtraHeaders and returning, so internal x-bf-* headers fall through to their dedicated handlers.
🧹 Nitpick comments (1)
examples/mcps/auth-demo-server/main.go (1)
195-201: Consider timing-safe comparison for educational completeness.The token comparison at line 199 uses
!=which is susceptible to timing attacks. Since this is a demo server meant to teach authentication patterns, usingcrypto/subtle.ConstantTimeCompare()would demonstrate production-grade security practices.🔐 Optional: Use constant-time comparison
+import "crypto/subtle" + // In secretDataHandler: token := headers.Get("X-Tool-Token") if token == "" { return mcp.NewToolResultError("tool auth required: missing X-Tool-Token header"), nil } - if token != toolExecToken { + if subtle.ConstantTimeCompare([]byte(token), []byte(toolExecToken)) != 1 { return mcp.NewToolResultError("tool auth failed: invalid X-Tool-Token"), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/mcps/auth-demo-server/main.go` around lines 195 - 201, Replace the direct string comparison of token and toolExecToken with a timing-safe comparison: import crypto/subtle, convert both token and toolExecToken to []byte, check lengths first and then use subtle.ConstantTimeCompare to decide if they match; if the compare result is not 1 return the existing mcp.NewToolResultError("tool auth failed: invalid X-Tool-Token") as before. This change should be applied where token is read and compared (the token and toolExecToken symbols) and keeps the same error behavior while preventing timing attacks.
🤖 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 742-746: In mergeMCPRedactedValues, preserve existing
AllowedExtraHeaders when the incoming update omits it by checking if
incoming.AllowedExtraHeaders == nil and, if so, set merged.AllowedExtraHeaders =
oldRaw.AllowedExtraHeaders (similar to the existing IsPingAvailable preservation
logic); update the merge logic that references merged, incoming, and oldRaw so
omitted AllowedExtraHeaders does not overwrite the DB value.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 492-501: The mock MCP config writer is overwriting the canonical
record ID by setting ID: clientConfig.ClientID when matching by id; change the
write logic so it preserves the existing record's canonical ID (use the matched
record.ID or leave ID unchanged) instead of replacing it with
clientConfig.ClientID, and apply the same change in the other mock write block
that mirrors lines 508-517; look for the fields ID and clientConfig.ClientID in
the test helper used for mock MCP config writes and ensure only
non-empty/intentional updates replace the stored record ID.
In `@transports/bifrost-http/lib/config.go`:
- Around line 2532-2534: The method comment for GetMCPHeaderCombinedAllowlist is
inaccurate: it claims the path is "lock-free" but the implementation uses
muMCP.RLock()/RUnlock(); update the comment on the GetMCPHeaderCombinedAllowlist
(Config) method to state that it acquires the muMCP read lock
(muMCP.RLock()/RUnlock()) to protect concurrent access and is safe for hot
paths, rather than saying it is lock-free, and keep the note that it returns a
schemas.WhiteList.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 200-215: The paginated MCP client construction in mcp.go builds a
schemas.MCPClientConfig but omits the AllowedExtraHeaders field; update the code
that constructs clientConfig (the local variable named clientConfig of type
schemas.MCPClientConfig) to set AllowedExtraHeaders:
dbClient.AllowedExtraHeaders (or the equivalent source on dbClient) so the
paginated response includes the same AllowedExtraHeaders as the non-paginated
path.
---
Duplicate comments:
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 382-386: The early return in the MCP passthrough block lets
internal control headers (like x-bf-*) be consumed before their handlers run;
update the block that uses mcpHeaderCombinedAllowlist, mcpExtraHeaders, keyStr
and value so it only appends/returns for allowed keys that are NOT internal
control headers—for example guard with a check that keyStr does not start with
"x-bf-" (or use the existing internal-header predicate) before appending to
mcpExtraHeaders and returning, so internal x-bf-* headers fall through to their
dedicated handlers.
In `@transports/changelog.md`:
- Line 5: Update the changelog entry that currently reads "feat: add support for
request level extra headers in MCP tool execution." to use the hyphenated
compound "request-level" so it reads "feat: add support for request-level extra
headers in MCP tool execution."; locate the exact changelog line containing that
string and replace "request level" with "request-level".
---
Nitpick comments:
In `@examples/mcps/auth-demo-server/main.go`:
- Around line 195-201: Replace the direct string comparison of token and
toolExecToken with a timing-safe comparison: import crypto/subtle, convert both
token and toolExecToken to []byte, check lengths first and then use
subtle.ConstantTimeCompare to decide if they match; if the compare result is not
1 return the existing mcp.NewToolResultError("tool auth failed: invalid
X-Tool-Token") as before. This change should be applied where token is read and
compared (the token and toolExecToken symbols) and keeps the same error behavior
while preventing timing attacks.
🪄 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: 0a0fb15c-26f9-4d6d-8512-1b4e846a4ff4
📒 Files selected for processing (30)
core/changelog.mdcore/mcp/clientmanager.gocore/mcp/codemode.gocore/mcp/codemode/starlark/executecode.gocore/mcp/codemode/starlark/starlark.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/mcp/utils/utils.gocore/schemas/bifrost.gocore/schemas/mcp.goexamples/mcps/auth-demo-server/main.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- transports/bifrost-http/integrations/bedrock_test.go
- transports/bifrost-http/integrations/router.go
- ui/lib/types/mcp.ts
- ui/lib/types/schemas.ts
- core/mcp/clientmanager.go
- core/mcp/codemode/starlark/starlark.go
- core/mcp/mcp.go
- transports/bifrost-http/handlers/inference.go
- core/schemas/bifrost.go
- framework/changelog.md
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
cb27490 to
61d5323
Compare
Pratham-Mishra04
force-pushed
the
03-17-feat_added_support_for_extra_headers_in_mcp_execute_tool
branch
from
fe95fe3 to
61a646a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/mcp.go (1)
200-215:⚠️ Potential issue | 🟠 MajorPaginated MCP client responses currently omit
allowed_extra_headers.
clientConfigis rebuilt fromdbClient, butAllowedExtraHeadersis not copied. This makes paginated and non-paginated responses inconsistent for the new feature.🔧 Suggested fix
clientConfig := &schemas.MCPClientConfig{ ID: dbClient.ClientID, Name: dbClient.Name, IsCodeModeClient: dbClient.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(dbClient.ConnectionType), ConnectionString: dbClient.ConnectionString, StdioConfig: dbClient.StdioConfig, AuthType: schemas.MCPAuthType(dbClient.AuthType), OauthConfigID: dbClient.OauthConfigID, ToolsToExecute: dbClient.ToolsToExecute, ToolsToAutoExecute: dbClient.ToolsToAutoExecute, Headers: dbClient.Headers, + AllowedExtraHeaders: dbClient.AllowedExtraHeaders, IsPingAvailable: &isPingAvailable, ToolSyncInterval: time.Duration(dbClient.ToolSyncInterval) * time.Minute, ToolPricing: dbClient.ToolPricing, }🤖 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 200 - 215, The paginated response builder reconstructs clientConfig from dbClient but omits the AllowedExtraHeaders field, causing inconsistency; update the construction of schemas.MCPClientConfig (clientConfig) to include AllowedExtraHeaders by copying it from dbClient (e.g., set AllowedExtraHeaders: dbClient.AllowedExtraHeaders) so both paginated and non-paginated responses include the same field.
♻️ Duplicate comments (1)
transports/bifrost-http/lib/ctx.go (1)
382-399:⚠️ Potential issue | 🟠 MajorPrevent MCP passthrough from short-circuiting internal
x-bf-*control headers.This branch can return early for allowlisted headers and bypass later internal handlers (
x-bf-send-back-raw-response,x-bf-parent-request-id,x-bf-passthrough-extra-params), especially when the allowlist is broad.🔧 Suggested fix
- // Handle MCP extra headers - if mcpHeaderCombinedAllowlist.IsAllowed(keyStr) { - mcpExtraHeaders[keyStr] = append(mcpExtraHeaders[keyStr], string(value)) - return true - } // Send back raw response header if keyStr == "x-bf-send-back-raw-response" { if valueStr := string(value); valueStr == "true" { bifrostCtx.SetValue(schemas.BifrostContextKeySendBackRawResponse, true) @@ if keyStr == "x-bf-passthrough-extra-params" { if valueStr := string(value); valueStr == "true" { bifrostCtx.SetValue(schemas.BifrostContextKeyPassthroughExtraParams, true) } return true } + + // Handle MCP extra headers after internal control headers are processed. + // Never consume internal bifrost control headers here. + if !strings.HasPrefix(keyStr, "x-bf-") && mcpHeaderCombinedAllowlist.IsAllowed(keyStr) { + mcpExtraHeaders[keyStr] = append(mcpExtraHeaders[keyStr], string(value)) + return true + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 382 - 399, The allowlist branch (mcpHeaderCombinedAllowlist.IsAllowed) currently returns early and can skip handling internal control headers like "x-bf-send-back-raw-response", "x-bf-parent-request-id" and "x-bf-passthrough-extra-params"; reorder or guard the logic so internal x-bf-* headers are processed first. Specifically, in the header handling loop check for keys with the "x-bf-" prefix (or explicitly check keyStr == "x-bf-send-back-raw-response"/"x-bf-parent-request-id"/"x-bf-passthrough-extra-params") and call bifrostCtx.SetValue with the appropriate schemas.BifrostContextKeySendBackRawResponse / schemas.BifrostMCPAgentOriginalRequestID handling before falling back to mcpHeaderCombinedAllowlist.IsAllowed and appending to mcpExtraHeaders.
🧹 Nitpick comments (1)
examples/mcps/auth-demo-server/main.go (1)
28-29: Update the demo narrative to reflect request-level extra-header forwarding.These lines still describe
X-Tool-Tokenas a staticheadersentry sent on every request, but this stack introduces request-level extra headers with per-client allowlisting. The example should demonstrateX-API-Keyas connection-level static header andX-Tool-Tokenas request-level forwarded header (allowlisted), otherwise it undercuts the new feature and least-privilege model.Suggested doc/config example adjustment
-// This means all configured headers are present on EVERY request — there is no -// separate "connection-only" vs "tool-only" header mechanism in Bifrost. To -// distinguish the two auth levels you simply use different header names, both -// configured in the same `headers` map. The server then enforces each header -// at the appropriate layer (middleware vs. handler). +// Connection headers are configured statically in MCP client config and are sent +// on MCP HTTP requests. Tool-execution-only credentials should be forwarded as +// request-level extra headers and allowlisted per client. +// The server enforces each header at the appropriate layer (middleware vs. handler). // Bifrost config example: // // { // "name": "auth_demo", // "connection_type": "http", // "connection_string": "http://localhost:3002/", // "auth_type": "headers", // "headers": { -// "X-API-Key": "super-secret-key", -// "X-Tool-Token": "tool-exec-secret" +// "X-API-Key": "super-secret-key" // }, +// "allowed_extra_headers": ["X-Tool-Token"], // "tools_to_execute": ["*"] // }Also applies to: 39-40, 86-90, 113-114, 123-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/mcps/auth-demo-server/main.go` around lines 28 - 29, Update the demo narrative and inline examples to show X-API-Key as the connection-level static header configured in the headers map and X-Tool-Token as a request-level forwarded header that is allowlisted per-client; specifically, change the comment text around the headers map and anywhere else mentioning X-Tool-Token (lines noted in the review: 28-29, 39-40, 86-90, 113-114, 123-124) to describe that X-API-Key is sent on every request from the proxy as a static header while X-Tool-Token must be forwarded per-request by the client and validated against the per-client allowlist, and ensure any example configuration and narrative references the allowlist behavior and least-privilege model.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/mcp/utils/utils.go`:
- Around line 21-47: The per-client AllowedExtraHeaders whitelist is not
validated against the securityHeaders denylist, allowing clients to permit
protected headers (e.g., authorization, x-api-key, host, content-length); update
validateAllowedExtraHeaders() to perform the same checks as
validateHeaderFilterConfig(): reject wildcard "*" and any explicit header names
that appear in securityHeaders (use the same canonicalization used by
AllowedExtraHeaders.IsAllowed), return an error on invalid entries, and ensure
callers (client creation) fail fast; reference validateAllowedExtraHeaders,
validateHeaderFilterConfig, securityHeaders, AllowedExtraHeaders, IsAllowed, and
GetHeadersForToolExecution when making the change.
In `@examples/mcps/auth-demo-server/main.go`:
- Around line 110-112: The startup logs print secret values directly
(connectionAPIKey and toolExecToken); change the log.Printf calls that reference
connectionAPIKey and toolExecToken to avoid printing secrets — instead log that
the values are set/loaded or show a redacted version (e.g., mask all but last 4
chars or log "<redacted>"), or log their presence and length (e.g., "X-API-Key:
set (length N)") so no secret bytes are emitted; update every occurrence where
connectionAPIKey and toolExecToken are interpolated (including the lines around
the current log.Printf calls and the other referenced lines) to use the
redacted/placeholder form.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 200-215: The paginated response builder reconstructs clientConfig
from dbClient but omits the AllowedExtraHeaders field, causing inconsistency;
update the construction of schemas.MCPClientConfig (clientConfig) to include
AllowedExtraHeaders by copying it from dbClient (e.g., set AllowedExtraHeaders:
dbClient.AllowedExtraHeaders) so both paginated and non-paginated responses
include the same field.
---
Duplicate comments:
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 382-399: The allowlist branch
(mcpHeaderCombinedAllowlist.IsAllowed) currently returns early and can skip
handling internal control headers like "x-bf-send-back-raw-response",
"x-bf-parent-request-id" and "x-bf-passthrough-extra-params"; reorder or guard
the logic so internal x-bf-* headers are processed first. Specifically, in the
header handling loop check for keys with the "x-bf-" prefix (or explicitly check
keyStr ==
"x-bf-send-back-raw-response"/"x-bf-parent-request-id"/"x-bf-passthrough-extra-params")
and call bifrostCtx.SetValue with the appropriate
schemas.BifrostContextKeySendBackRawResponse /
schemas.BifrostMCPAgentOriginalRequestID handling before falling back to
mcpHeaderCombinedAllowlist.IsAllowed and appending to mcpExtraHeaders.
---
Nitpick comments:
In `@examples/mcps/auth-demo-server/main.go`:
- Around line 28-29: Update the demo narrative and inline examples to show
X-API-Key as the connection-level static header configured in the headers map
and X-Tool-Token as a request-level forwarded header that is allowlisted
per-client; specifically, change the comment text around the headers map and
anywhere else mentioning X-Tool-Token (lines noted in the review: 28-29, 39-40,
86-90, 113-114, 123-124) to describe that X-API-Key is sent on every request
from the proxy as a static header while X-Tool-Token must be forwarded
per-request by the client and validated against the per-client allowlist, and
ensure any example configuration and narrative references the allowlist behavior
and least-privilege model.
🪄 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: 13e9ea2c-5e2c-46ae-83ea-9318d3499f06
📒 Files selected for processing (30)
core/changelog.mdcore/mcp/clientmanager.gocore/mcp/codemode.gocore/mcp/codemode/starlark/executecode.gocore/mcp/codemode/starlark/starlark.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/mcp/utils/utils.gocore/schemas/bifrost.gocore/schemas/mcp.goexamples/mcps/auth-demo-server/main.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- core/mcp/toolmanager.go
- transports/bifrost-http/integrations/bedrock_test.go
- core/schemas/mcp.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- ui/lib/types/schemas.ts
- transports/bifrost-http/lib/ctx_test.go
- core/mcp/codemode.go
- transports/bifrost-http/lib/config.go
- ui/lib/types/mcp.ts
- transports/bifrost-http/lib/config_test.go
- transports/bifrost-http/handlers/asyncinference.go
- transports/bifrost-http/integrations/router.go
- core/changelog.md
Pratham-Mishra04
force-pushed
the
03-17-feat_added_support_for_extra_headers_in_mcp_execute_tool
branch
from
61a646a to
05c66af
Compare
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
61d5323 to
791ecbf
Compare
9 tasks
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
791ecbf to
bede6b1
Compare
Pratham-Mishra04
force-pushed
the
03-17-feat_added_support_for_extra_headers_in_mcp_execute_tool
branch
2 times, most recently
from
3858f35 to
faac4f3
Compare
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
bede6b1 to
06b9ba6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
transports/bifrost-http/handlers/mcpinference.go (1)
63-67:⚠️ Potential issue | 🟡 MinorMove
defer cancel()after the nil guard.
defer cancel()is registered before checkingbifrostCtx == nil. Ifcancelis nil on an error path, this can panic on return.🔧 Proposed fix
- bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false, h.config.GetHeaderMatcher(), h.config.GetMCPHeaderCombinedAllowlist()) - defer cancel() // Ensure cleanup on function exit + bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false, h.config.GetHeaderMatcher(), h.config.GetMCPHeaderCombinedAllowlist()) if bifrostCtx == nil { SendError(ctx, fasthttp.StatusBadRequest, "Failed to convert context") return } + defer cancel() // Ensure cleanup on function exit- bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false, h.config.GetHeaderMatcher(), h.config.GetMCPHeaderCombinedAllowlist()) - defer cancel() // Ensure cleanup on function exit + bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false, h.config.GetHeaderMatcher(), h.config.GetMCPHeaderCombinedAllowlist()) if bifrostCtx == nil { SendError(ctx, fasthttp.StatusBadRequest, "Failed to convert context") return } + defer cancel() // Ensure cleanup on function exitAlso applies to: 96-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcpinference.go` around lines 63 - 67, The defer cancel() is registered immediately after calling lib.ConvertToBifrostContext which can return a nil cancel and cause a panic; change the pattern in the handler so you first call lib.ConvertToBifrostContext(ctx, ...) and check if bifrostCtx == nil (call SendError and return) and only after confirming bifrostCtx is non-nil assign defer cancel(); update both occurrences that call ConvertToBifrostContext (the bifrostCtx/cancel pairs around the MCP inference handler) so the nil guard runs before deferring cancel().transports/bifrost-http/handlers/mcp.go (1)
200-215:⚠️ Potential issue | 🟠 MajorInclude
AllowedExtraHeadersin paginated client config mapping.
getMCPClientsPaginatedmaps many DB fields intoschemas.MCPClientConfigbut skipsdbClient.AllowedExtraHeaders. That makes paginated reads inconsistent with the new feature and can hide persisted allowlists in UI/API responses.💡 Proposed fix
clientConfig := &schemas.MCPClientConfig{ ID: dbClient.ClientID, Name: dbClient.Name, IsCodeModeClient: dbClient.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(dbClient.ConnectionType), ConnectionString: dbClient.ConnectionString, StdioConfig: dbClient.StdioConfig, AuthType: schemas.MCPAuthType(dbClient.AuthType), OauthConfigID: dbClient.OauthConfigID, ToolsToExecute: dbClient.ToolsToExecute, ToolsToAutoExecute: dbClient.ToolsToAutoExecute, Headers: dbClient.Headers, + AllowedExtraHeaders: dbClient.AllowedExtraHeaders, IsPingAvailable: &isPingAvailable, ToolSyncInterval: time.Duration(dbClient.ToolSyncInterval) * time.Minute, ToolPricing: dbClient.ToolPricing, }🤖 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 200 - 215, getMCPClientsPaginated builds a schemas.MCPClientConfig but omits dbClient.AllowedExtraHeaders, causing paginated responses to miss the persisted allowlist; update the mapping in the clientConfig construction (the variable named clientConfig in getMCPClientsPaginated) to set AllowedExtraHeaders: dbClient.AllowedExtraHeaders so schemas.MCPClientConfig receives and returns the field consistently with other DB-to-schema mappings.
♻️ Duplicate comments (2)
transports/changelog.md (1)
5-5:⚠️ Potential issue | 🟡 MinorHyphenate the compound modifier in this changelog entry.
Line 5 should use “request-level” instead of “request level”.
Suggested fix
-- feat: add support for request level extra headers in MCP tool execution. +- feat: add support for request-level extra headers in MCP tool execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/changelog.md` at line 5, Update the changelog entry string "feat: add support for request level extra headers in MCP tool execution." to hyphenate the compound modifier so it reads "request-level" (i.e., "feat: add support for request-level extra headers in MCP tool execution."); locate and replace that exact entry in the changelog so the compound adjective is grammatically correct.transports/bifrost-http/lib/ctx.go (1)
382-386:⚠️ Potential issue | 🟠 MajorMCP allowlist short-circuits later control-header handlers.
At Line 382, returning immediately after
mcpHeaderCombinedAllowlist.IsAllowed(keyStr)prevents later explicit handlers (e.g.,x-bf-send-back-raw-response,x-bf-parent-request-id,x-bf-passthrough-extra-params) from executing when those headers are allowlisted (especially with*).🔧 Proposed fix
- // Handle MCP extra headers - if mcpHeaderCombinedAllowlist.IsAllowed(keyStr) { - mcpExtraHeaders[keyStr] = append(mcpExtraHeaders[keyStr], string(value)) - return true - } + // Handle MCP extra headers + // NOTE: do not short-circuit here; dedicated handlers below must still run. + if mcpHeaderCombinedAllowlist.IsAllowed(keyStr) { + mcpExtraHeaders[keyStr] = append(mcpExtraHeaders[keyStr], string(value)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 382 - 386, The code appends allowlisted MCP headers to mcpExtraHeaders and then returns immediately when mcpHeaderCombinedAllowlist.IsAllowed(keyStr), which short-circuits later control-header handlers (e.g., x-bf-send-back-raw-response, x-bf-parent-request-id, x-bf-passthrough-extra-params); change the behavior in the handler containing mcpHeaderCombinedAllowlist.IsAllowed(keyStr) so it still appends the header to mcpExtraHeaders but does NOT return early — remove the immediate return/short-circuit and allow the subsequent control-header checks (the logic that inspects those specific header keys) to run after appending so both allowlisting and explicit control handling occur.
🧹 Nitpick comments (2)
transports/bifrost-http/handlers/inference.go (1)
685-685: Consider a helper for context conversion to reduce future call-site driftThe same 4-argument
ConvertToBifrostContextcall is repeated many times. A small helper onCompletionHandlerwould reduce maintenance risk when this signature evolves again.♻️ Suggested refactor
+func (h *CompletionHandler) convertToBifrostContext(ctx *fasthttp.RequestCtx) (*schemas.BifrostContext, context.CancelFunc) { + return lib.ConvertToBifrostContext( + ctx, + h.handlerStore.ShouldAllowDirectKeys(), + h.config.GetHeaderMatcher(), + h.config.GetMCPHeaderCombinedAllowlist(), + ) +} ... - bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, h.handlerStore.ShouldAllowDirectKeys(), h.config.GetHeaderMatcher(), h.config.GetMCPHeaderCombinedAllowlist()) + bifrostCtx, cancel := h.convertToBifrostContext(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/inference.go` at line 685, Multiple call sites invoke lib.ConvertToBifrostContext(ctx, h.handlerStore.ShouldAllowDirectKeys(), h.config.GetHeaderMatcher(), h.config.GetMCPHeaderCombinedAllowlist()) which risks churn if the signature changes; add a small helper method on CompletionHandler (e.g., CompletionHandler.toBifrostContext(ctx) or toBifrostCtx) that encapsulates that 4-argument call and returns the bifrostCtx and cancel, then replace direct calls with h.toBifrostContext(ctx) (still using h.handlerStore and h.config inside the helper) so future signature changes are localized to the new helper.transports/bifrost-http/lib/config.go (1)
2532-2553: Hot-path allowlist merge can reduce duplicate entries and allocations
GetMCPHeaderCombinedAllowlistis called on request paths and currently rebuilds a potentially duplicated slice each time. Dedup + preallocation would lower GC pressure.⚡ Suggested optimization
func (c *Config) GetMCPHeaderCombinedAllowlist() schemas.WhiteList { c.muMCP.RLock() defer c.muMCP.RUnlock() if c.MCPConfig == nil || len(c.MCPConfig.ClientConfigs) == 0 { return schemas.WhiteList{} } - allowlist := schemas.WhiteList{} + total := 0 for _, mcpClient := range c.MCPConfig.ClientConfigs { if mcpClient == nil { continue } if mcpClient.AllowedExtraHeaders.IsUnrestricted() { return schemas.WhiteList{"*"} } - allowlist = append(allowlist, mcpClient.AllowedExtraHeaders...) + total += len(mcpClient.AllowedExtraHeaders) + } + + allowlist := make(schemas.WhiteList, 0, total) + seen := make(map[string]struct{}, total) + for _, mcpClient := range c.MCPConfig.ClientConfigs { + if mcpClient == nil { + continue + } + for _, header := range mcpClient.AllowedExtraHeaders { + if _, ok := seen[header]; ok { + continue + } + seen[header] = struct{}{} + allowlist = append(allowlist, header) + } } return allowlist }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config.go` around lines 2532 - 2553, GetMCPHeaderCombinedAllowlist currently appends headers on every call causing duplicates and extra allocations; change it to deduplicate and preallocate: under the existing muMCP read lock iterate c.MCPConfig.ClientConfigs (check for nil and AllowedExtraHeaders.IsUnrestricted() as now), compute a capacity hint by summing len(mcpClient.AllowedExtraHeaders) (or just use a small default), use a map[string]struct{} (or map[string]bool) to dedupe entries from mcpClient.AllowedExtraHeaders, and after the loop convert the map keys into a schemas.WhiteList slice with capacity equal to map size and return that; preserve the early return for unrestricted headers (return schemas.WhiteList{"*"}), and keep using the same function and field names (GetMCPHeaderCombinedAllowlist, MCPConfig.ClientConfigs, AllowedExtraHeaders, IsUnrestricted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/mcps/auth-demo-server/main.go`:
- Around line 28-29: The demo still places "X-Tool-Token" in the MCP client's
static headers map and logs text describing that model; update the code and
comments to instead demonstrate the request-level forwarding path by removing
"X-Tool-Token" from the static headers map and adding it to the MCP config's
allowed_extra_headers opt-in (use the symbol allowed_extra_headers) so
request-level forwarding permits it; update any startup log text and comments
(e.g., in main, the MCP client/config setup, and the secret_data
handler/middleware descriptions) to reflect that X-Tool-Token is forwarded via
request-level extra headers rather than static headers. Ensure references to
headers map, allowed_extra_headers, X-Tool-Token, secret_data handler, and any
startup log strings are updated consistently.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 200-215: getMCPClientsPaginated builds a schemas.MCPClientConfig
but omits dbClient.AllowedExtraHeaders, causing paginated responses to miss the
persisted allowlist; update the mapping in the clientConfig construction (the
variable named clientConfig in getMCPClientsPaginated) to set
AllowedExtraHeaders: dbClient.AllowedExtraHeaders so schemas.MCPClientConfig
receives and returns the field consistently with other DB-to-schema mappings.
In `@transports/bifrost-http/handlers/mcpinference.go`:
- Around line 63-67: The defer cancel() is registered immediately after calling
lib.ConvertToBifrostContext which can return a nil cancel and cause a panic;
change the pattern in the handler so you first call
lib.ConvertToBifrostContext(ctx, ...) and check if bifrostCtx == nil (call
SendError and return) and only after confirming bifrostCtx is non-nil assign
defer cancel(); update both occurrences that call ConvertToBifrostContext (the
bifrostCtx/cancel pairs around the MCP inference handler) so the nil guard runs
before deferring cancel().
---
Duplicate comments:
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 382-386: The code appends allowlisted MCP headers to
mcpExtraHeaders and then returns immediately when
mcpHeaderCombinedAllowlist.IsAllowed(keyStr), which short-circuits later
control-header handlers (e.g., x-bf-send-back-raw-response,
x-bf-parent-request-id, x-bf-passthrough-extra-params); change the behavior in
the handler containing mcpHeaderCombinedAllowlist.IsAllowed(keyStr) so it still
appends the header to mcpExtraHeaders but does NOT return early — remove the
immediate return/short-circuit and allow the subsequent control-header checks
(the logic that inspects those specific header keys) to run after appending so
both allowlisting and explicit control handling occur.
In `@transports/changelog.md`:
- Line 5: Update the changelog entry string "feat: add support for request level
extra headers in MCP tool execution." to hyphenate the compound modifier so it
reads "request-level" (i.e., "feat: add support for request-level extra headers
in MCP tool execution."); locate and replace that exact entry in the changelog
so the compound adjective is grammatically correct.
---
Nitpick comments:
In `@transports/bifrost-http/handlers/inference.go`:
- Line 685: Multiple call sites invoke lib.ConvertToBifrostContext(ctx,
h.handlerStore.ShouldAllowDirectKeys(), h.config.GetHeaderMatcher(),
h.config.GetMCPHeaderCombinedAllowlist()) which risks churn if the signature
changes; add a small helper method on CompletionHandler (e.g.,
CompletionHandler.toBifrostContext(ctx) or toBifrostCtx) that encapsulates that
4-argument call and returns the bifrostCtx and cancel, then replace direct calls
with h.toBifrostContext(ctx) (still using h.handlerStore and h.config inside the
helper) so future signature changes are localized to the new helper.
In `@transports/bifrost-http/lib/config.go`:
- Around line 2532-2553: GetMCPHeaderCombinedAllowlist currently appends headers
on every call causing duplicates and extra allocations; change it to deduplicate
and preallocate: under the existing muMCP read lock iterate
c.MCPConfig.ClientConfigs (check for nil and
AllowedExtraHeaders.IsUnrestricted() as now), compute a capacity hint by summing
len(mcpClient.AllowedExtraHeaders) (or just use a small default), use a
map[string]struct{} (or map[string]bool) to dedupe entries from
mcpClient.AllowedExtraHeaders, and after the loop convert the map keys into a
schemas.WhiteList slice with capacity equal to map size and return that;
preserve the early return for unrestricted headers (return
schemas.WhiteList{"*"}), and keep using the same function and field names
(GetMCPHeaderCombinedAllowlist, MCPConfig.ClientConfigs, AllowedExtraHeaders,
IsUnrestricted).
🪄 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: e539c511-c2f3-4bd1-a35d-5e1941b1a9cb
📒 Files selected for processing (30)
core/changelog.mdcore/mcp/clientmanager.gocore/mcp/codemode.gocore/mcp/codemode/starlark/executecode.gocore/mcp/codemode/starlark/starlark.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/mcp/utils/utils.gocore/schemas/bifrost.gocore/schemas/mcp.goexamples/mcps/auth-demo-server/main.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- ui/lib/types/mcp.ts
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- core/mcp/utils/utils.go
- core/mcp/toolmanager.go
- ui/lib/types/schemas.ts
- core/mcp/codemode.go
- transports/bifrost-http/integrations/bedrock_test.go
- framework/changelog.md
- transports/bifrost-http/lib/config_test.go
- core/mcp/clientmanager.go
- framework/configstore/tables/mcp.go
- core/changelog.md
- transports/bifrost-http/handlers/mcpserver.go
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
06b9ba6 to
eaf1f57
Compare
Pratham-Mishra04
force-pushed
the
03-17-feat_added_support_for_extra_headers_in_mcp_execute_tool
branch
from
faac4f3 to
6a1a4cb
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
transports/bifrost-http/lib/ctx.go (1)
337-385:⚠️ Potential issue | 🟠 MajorDon't make MCP capture mutually exclusive with provider header forwarding.
The
x-bf-eh-*and direct-allowlist branches return before the new MCP block runs. If a header is intentionally allowlisted for both provider calls and MCP tool execution, it only lands inBifrostContextKeyExtraHeaders, so the tool call never sees it.🛠️ Proposed fix
if labelName, ok := strings.CutPrefix(keyStr, "x-bf-eh-"); ok { // Skip empty header names after prefix removal if labelName == "" { return true } @@ } // Append header value (allow multiple values for the same header) extraHeaders[labelName] = append(extraHeaders[labelName], string(value)) + if mcpHeaderCombinedAllowlist.IsAllowed(labelName) { + mcpExtraHeaders[labelName] = append(mcpExtraHeaders[labelName], string(value)) + } return true } @@ if logger != nil { logger.Debug("forwarding header via allowlist: %s", keyStr) } extraHeaders[keyStr] = append(extraHeaders[keyStr], string(value)) + if mcpHeaderCombinedAllowlist.IsAllowed(keyStr) { + mcpExtraHeaders[keyStr] = append(mcpExtraHeaders[keyStr], string(value)) + } return true } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 337 - 385, The x-bf-eh-* branch and the direct-allowlist branch currently return immediately after appending to extraHeaders which prevents the subsequent mcpHeaderCombinedAllowlist.IsAllowed(keyStr) check from also receiving the header; change the control flow in the x-bf-eh-* handling (labelName branch) and in the direct-allowlist branch inside matcher.MatchesAllow(keyStr) so they do not return immediately after appending to extraHeaders — instead, after appending, also check mcpHeaderCombinedAllowlist.IsAllowed(keyStr) and if true append the same value into mcpExtraHeaders (or remove the early returns and let the existing MCP block run), preserving the existing reserved x-bf-* and securityDenylist checks and keeping logger.Debug behavior.transports/bifrost-http/handlers/mcp.go (1)
200-215:⚠️ Potential issue | 🟠 MajorMissing
AllowedExtraHeadersassignment in paginated response.The
clientConfigstruct ingetMCPClientsPaginateddoes not includeAllowedExtraHeadersfromdbClient. This causes the paginated API endpoint to return MCP clients without their allowed extra headers configuration, while the non-paginated path (viaGetMCPConfig) includes it correctly.🐛 Proposed fix
clientConfig := &schemas.MCPClientConfig{ ID: dbClient.ClientID, Name: dbClient.Name, IsCodeModeClient: dbClient.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(dbClient.ConnectionType), ConnectionString: dbClient.ConnectionString, StdioConfig: dbClient.StdioConfig, AuthType: schemas.MCPAuthType(dbClient.AuthType), OauthConfigID: dbClient.OauthConfigID, ToolsToExecute: dbClient.ToolsToExecute, ToolsToAutoExecute: dbClient.ToolsToAutoExecute, Headers: dbClient.Headers, + AllowedExtraHeaders: dbClient.AllowedExtraHeaders, IsPingAvailable: &isPingAvailable, ToolSyncInterval: time.Duration(dbClient.ToolSyncInterval) * time.Minute, ToolPricing: dbClient.ToolPricing, }🤖 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 200 - 215, getMCPClientsPaginated builds a clientConfig but omits AllowedExtraHeaders, so paginated responses miss that field; update the clientConfig initialization in getMCPClientsPaginated to set AllowedExtraHeaders: dbClient.AllowedExtraHeaders (match the field used by GetMCPConfig) so the paginated API returns the same allowed-extra-headers configuration as the non-paginated path.
🧹 Nitpick comments (1)
ui/lib/types/schemas.ts (1)
938-948: Mirror the duplicate-entry guard onallowed_extra_headers.This new whitelist accepts repeated header names even though the sibling tool lists explicitly reject duplicates. Keeping the same invariant here avoids redundant payloads and inconsistent form behavior.
♻️ Proposed refinement
allowed_extra_headers: z .array(z.string()) .optional() .refine( (headers) => { if (!headers || headers.length === 0) return true; const hasWildcard = headers.includes("*"); return !hasWildcard || headers.length === 1; }, { message: "Wildcard '*' cannot be combined with specific header names" }, - ), + ) + .refine( + (headers) => { + if (!headers) return true; + return headers.length === new Set(headers).size; + }, + { message: "Duplicate header names are not allowed" }, + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/schemas.ts` around lines 938 - 948, The allowed_extra_headers Zod schema currently only enforces the wildcard rule but allows duplicate header names; update the refine logic on allowed_extra_headers to also reject duplicates by checking uniqueness (e.g., verify headers is undefined/empty or new Set(headers).size === headers.length) while preserving the existing wildcard rule (wildcard only allowed alone). Modify the existing refine callback on allowed_extra_headers to perform both checks so the schema enforces no duplicate entries and the wildcard constraint together.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 200-215: getMCPClientsPaginated builds a clientConfig but omits
AllowedExtraHeaders, so paginated responses miss that field; update the
clientConfig initialization in getMCPClientsPaginated to set
AllowedExtraHeaders: dbClient.AllowedExtraHeaders (match the field used by
GetMCPConfig) so the paginated API returns the same allowed-extra-headers
configuration as the non-paginated path.
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 337-385: The x-bf-eh-* branch and the direct-allowlist branch
currently return immediately after appending to extraHeaders which prevents the
subsequent mcpHeaderCombinedAllowlist.IsAllowed(keyStr) check from also
receiving the header; change the control flow in the x-bf-eh-* handling
(labelName branch) and in the direct-allowlist branch inside
matcher.MatchesAllow(keyStr) so they do not return immediately after appending
to extraHeaders — instead, after appending, also check
mcpHeaderCombinedAllowlist.IsAllowed(keyStr) and if true append the same value
into mcpExtraHeaders (or remove the early returns and let the existing MCP block
run), preserving the existing reserved x-bf-* and securityDenylist checks and
keeping logger.Debug behavior.
---
Nitpick comments:
In `@ui/lib/types/schemas.ts`:
- Around line 938-948: The allowed_extra_headers Zod schema currently only
enforces the wildcard rule but allows duplicate header names; update the refine
logic on allowed_extra_headers to also reject duplicates by checking uniqueness
(e.g., verify headers is undefined/empty or new Set(headers).size ===
headers.length) while preserving the existing wildcard rule (wildcard only
allowed alone). Modify the existing refine callback on allowed_extra_headers to
perform both checks so the schema enforces no duplicate entries and the wildcard
constraint together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc4fb072-1ba6-42df-8204-ca0f84ce74db
📒 Files selected for processing (30)
core/changelog.mdcore/mcp/clientmanager.gocore/mcp/codemode.gocore/mcp/codemode/starlark/executecode.gocore/mcp/codemode/starlark/starlark.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/mcp/utils/utils.gocore/schemas/bifrost.gocore/schemas/mcp.goexamples/mcps/auth-demo-server/main.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- core/mcp/mcp.go
- transports/bifrost-http/integrations/router.go
- core/mcp/codemode/starlark/starlark.go
- transports/bifrost-http/integrations/bedrock_test.go
- core/mcp/codemode.go
- core/mcp/utils/utils.go
- ui/lib/types/mcp.ts
- framework/configstore/migrations.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- core/schemas/bifrost.go
- core/changelog.md
- transports/bifrost-http/lib/config_test.go
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
eaf1f57 to
75d59f2
Compare
Pratham-Mishra04
force-pushed
the
03-17-feat_added_support_for_extra_headers_in_mcp_execute_tool
branch
from
6a1a4cb to
b523d65
Compare
18 tasks
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/mcp.go (1)
200-215:⚠️ Potential issue | 🟠 MajorInclude
AllowedExtraHeadersin the paginated MCP response shape.This manual
schemas.MCPClientConfigrebuild drops the new field, so paginated/search-backed views returnconfig.allowed_extra_headersas missing even when it exists in the DB. The sheet now defaults a missing value to[], so editing one of those clients will silently clear the persisted allowlist on save.💡 Suggested fix
clientConfig := &schemas.MCPClientConfig{ ID: dbClient.ClientID, Name: dbClient.Name, IsCodeModeClient: dbClient.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(dbClient.ConnectionType), ConnectionString: dbClient.ConnectionString, StdioConfig: dbClient.StdioConfig, AuthType: schemas.MCPAuthType(dbClient.AuthType), OauthConfigID: dbClient.OauthConfigID, ToolsToExecute: dbClient.ToolsToExecute, ToolsToAutoExecute: dbClient.ToolsToAutoExecute, Headers: dbClient.Headers, + AllowedExtraHeaders: dbClient.AllowedExtraHeaders, IsPingAvailable: &isPingAvailable, ToolSyncInterval: time.Duration(dbClient.ToolSyncInterval) * time.Minute, ToolPricing: dbClient.ToolPricing, }🤖 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 200 - 215, The MCP paginated response is omitting the AllowedExtraHeaders field because the manual construction of schemas.MCPClientConfig in the mcp handler (clientConfig variable) doesn't include that field; update the struct literal used to build schemas.MCPClientConfig to set AllowedExtraHeaders from dbClient (e.g., AllowedExtraHeaders: dbClient.AllowedExtraHeaders) so the paginated/search-backed responses return the stored allowlist instead of dropping it and causing silent clears on save.
♻️ Duplicate comments (2)
transports/changelog.md (1)
5-5:⚠️ Potential issue | 🟡 MinorHyphenate
request-levelin the changelog entry.Use the compound modifier here for the same phrasing used elsewhere in the PR.
Suggested edit
-- feat: add support for request level extra headers in MCP tool execution. +- feat: add support for request-level extra headers in MCP tool execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/changelog.md` at line 5, Update the changelog entry string "feat: add support for request level extra headers in MCP tool execution." to hyphenate the compound modifier, changing "request level" to "request-level" so it reads "feat: add support for request-level extra headers in MCP tool execution."; locate and edit that exact line in the changelog (the entry starting with "feat: add support for") and save the file.transports/bifrost-http/lib/config_test.go (1)
492-501:⚠️ Potential issue | 🟡 MinorPreserve canonical MCP client ID in mock update paths.
Both branches overwrite
IDwithclientConfig.ClientIDeven though lookup is keyed byid. IfclientConfig.ClientIDis empty/divergent, mock state can drift and hide regressions.🔧 Suggested fix
func (m *MockConfigStore) UpdateMCPClientConfig(ctx context.Context, id string, clientConfig *tables.TableMCPClient) error { @@ + resolvedID := clientConfig.ClientID + if resolvedID == "" { + resolvedID = id + } + // Update the in-memory state to ensure GetMCPConfig returns updated data for i := range m.mcpConfig.ClientConfigs { if m.mcpConfig.ClientConfigs[i].ID == id { // Found the entry, update it with the new config m.mcpConfig.ClientConfigs[i] = &schemas.MCPClientConfig{ - ID: clientConfig.ClientID, + ID: resolvedID, Name: clientConfig.Name, @@ // If not found, create a new entry (similar to CreateMCPClientConfig behavior) m.mcpConfig.ClientConfigs = append(m.mcpConfig.ClientConfigs, &schemas.MCPClientConfig{ - ID: clientConfig.ClientID, + ID: resolvedID, Name: clientConfig.Name,Also applies to: 508-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config_test.go` around lines 492 - 501, The test mock update paths are overwriting the canonical client ID by setting the struct's ID to clientConfig.ClientID (e.g., the assignment to ID in the update object), but the lookup is keyed by the original id variable; ensure the mocked update preserves the canonical id used for lookup by assigning ID from the lookup key (id) or leaving the existing ID intact rather than overwriting it with clientConfig.ClientID; update both occurrences (the block shown and the similar block at the other occurrence) so that the update payload uses the lookup id instead of clientConfig.ClientID.
🧹 Nitpick comments (4)
core/mcp/clientmanager.go (1)
252-252: CloneIsPingAvailablevalue instead of reusing the incoming pointer.
newConfigis intended to be an immutable snapshot, but assigningupdatedConfig.IsPingAvailabledirectly keeps aliasing to caller-owned memory.♻️ Proposed patch
+ var isPingAvailable *bool + if updatedConfig.IsPingAvailable != nil { + v := *updatedConfig.IsPingAvailable + isPingAvailable = &v + } + newConfig := &schemas.MCPClientConfig{ // Immutable fields - copy from existing config @@ - IsPingAvailable: updatedConfig.IsPingAvailable, + IsPingAvailable: isPingAvailable, ToolSyncInterval: updatedConfig.ToolSyncInterval, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/mcp/clientmanager.go` at line 252, newConfig is currently aliasing the caller-owned pointer by assigning updatedConfig.IsPingAvailable directly; instead copy the boolean value into a new allocated bool for newConfig.IsPingAvailable so the snapshot is immutable. Locate the assignment to newConfig.IsPingAvailable in clientmanager.go and replace the direct pointer assignment from updatedConfig.IsPingAvailable with code that allocates a new bool, sets it to *updatedConfig.IsPingAvailable (or the value) and assigns that new pointer to newConfig.IsPingAvailable to avoid sharing the caller's memory.transports/bifrost-http/lib/ctx_test.go (1)
72-73: Add one case that exercises a non-empty MCP header allowlist.All of the updated calls still pass
schemas.WhiteList{}, so this suite only covers the signature change. Please add a small case that passes a real allowlist and assertsschemas.BifrostContextKeyMCPExtraHeadersincludes only allowed headers (and still drops denylisted ones), so the new forwarding path is locked down here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx_test.go` around lines 72 - 73, The test suite never exercises a non-empty MCP header allowlist: update ctx_test.go to add a case that calls ConvertToBifrostContext with a non-empty schemas.WhiteList (e.g., include one or two allowed header names) instead of schemas.WhiteList{} and provide incoming headers containing allowed and denylisted names; then assert that the returned context value under schemas.BifrostContextKeyMCPExtraHeaders contains only the allowed headers and does not include denylisted ones. Locate the usage of ConvertToBifrostContext and add a small subtest that constructs the whitelist, invokes ConvertToBifrostContext(ctx, false, matcher, yourWhiteList), and checks the context lookup for schemas.BifrostContextKeyMCPExtraHeaders to verify correct inclusion/exclusion.framework/configstore/migrations.go (1)
5036-5065: Consider explicit backfill to[]for data consistency.While existing rows with
NULLinallowed_extra_headers_jsonload safely as nil and are handled defensively by downstream code (seerdb.go:1081-1082,handlers/mcp.go:748-750), backfilling to an explicit empty array makes the data semantically clearer and prevents potential future issues if new code assumes non-nil values. The field is not part of MCP client hash generation, so no reconciliation concerns arise from the current approach.💡 Optional hardening patch
func migrationAddMCPClientAllowedExtraHeadersJSONColumn(ctx context.Context, db *gorm.DB) error { m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ ID: "add_mcp_client_allowed_extra_headers_json_column", Migrate: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() if !migrator.HasColumn(&tables.TableMCPClient{}, "allowed_extra_headers_json") { if err := migrator.AddColumn(&tables.TableMCPClient{}, "allowed_extra_headers_json"); err != nil { return err } + if err := tx.Exec("UPDATE config_mcp_clients SET allowed_extra_headers_json = '[]' WHERE allowed_extra_headers_json IS NULL").Error; err != nil { + return fmt.Errorf("failed to initialize allowed_extra_headers_json: %w", err) + } } return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 5036 - 5065, The migrationAddMCPClientAllowedExtraHeadersJSONColumn migration should explicitly backfill existing NULL values to an empty JSON array after adding the column: inside the Migrate function (after migrator.AddColumn) execute an UPDATE against the mcp_client table via the current tx (e.g., tx.Exec(...)) to set allowed_extra_headers_json = '[]' where allowed_extra_headers_json IS NULL and return any error; keep the Rollback as-is. Ensure the UPDATE uses the same tx.WithContext(ctx) and check/return errors so existing rows become an explicit empty array rather than NULL.framework/configstore/rdb.go (1)
1117-1117: Remove manualupdated_atassignment; rely on GORM'sautoUpdateTime.The
TableMCPClientmodel should havegorm:"autoUpdateTime"on itsUpdatedAtfield, which GORM populates automatically on updates. Manually settingupdated_athere is redundant and inconsistent with the codebase convention.♻️ Suggested fix
updates := map[string]interface{}{ "name": clientConfigCopy.Name, "is_code_mode_client": clientConfigCopy.IsCodeModeClient, "tools_to_execute_json": string(toolsToExecuteJSON), "tools_to_auto_execute_json": string(toolsToAutoExecuteJSON), "headers_json": headersJSONStr, "allowed_extra_headers_json": string(allowedExtraHeadersJSON), "tool_pricing_json": string(toolPricingJSON), "tool_sync_interval": clientConfigCopy.ToolSyncInterval, - "updated_at": time.Now(), }Based on learnings: "In this codebase using GORM, models with CreatedAt and UpdatedAt fields of type time.Time tagged with gorm:"autoUpdateTime" are populated automatically by GORM on insert/update. Do not manually set them with time.Now()."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` at line 1117, Remove the manual "updated_at": time.Now() assignment in the update/save call and let GORM populate UpdatedAt automatically via the TableMCPClient model's UpdatedAt field tagged with gorm:"autoUpdateTime"; locate the code that sets "updated_at" (string key "updated_at") in the map or values being passed to the DB update (around TableMCPClient usage) and delete that key so GORM's autoUpdateTime on TableMCPClient.UpdatedAt handles timestamp updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 200-215: The MCP paginated response is omitting the
AllowedExtraHeaders field because the manual construction of
schemas.MCPClientConfig in the mcp handler (clientConfig variable) doesn't
include that field; update the struct literal used to build
schemas.MCPClientConfig to set AllowedExtraHeaders from dbClient (e.g.,
AllowedExtraHeaders: dbClient.AllowedExtraHeaders) so the
paginated/search-backed responses return the stored allowlist instead of
dropping it and causing silent clears on save.
---
Duplicate comments:
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 492-501: The test mock update paths are overwriting the canonical
client ID by setting the struct's ID to clientConfig.ClientID (e.g., the
assignment to ID in the update object), but the lookup is keyed by the original
id variable; ensure the mocked update preserves the canonical id used for lookup
by assigning ID from the lookup key (id) or leaving the existing ID intact
rather than overwriting it with clientConfig.ClientID; update both occurrences
(the block shown and the similar block at the other occurrence) so that the
update payload uses the lookup id instead of clientConfig.ClientID.
In `@transports/changelog.md`:
- Line 5: Update the changelog entry string "feat: add support for request level
extra headers in MCP tool execution." to hyphenate the compound modifier,
changing "request level" to "request-level" so it reads "feat: add support for
request-level extra headers in MCP tool execution."; locate and edit that exact
line in the changelog (the entry starting with "feat: add support for") and save
the file.
---
Nitpick comments:
In `@core/mcp/clientmanager.go`:
- Line 252: newConfig is currently aliasing the caller-owned pointer by
assigning updatedConfig.IsPingAvailable directly; instead copy the boolean value
into a new allocated bool for newConfig.IsPingAvailable so the snapshot is
immutable. Locate the assignment to newConfig.IsPingAvailable in
clientmanager.go and replace the direct pointer assignment from
updatedConfig.IsPingAvailable with code that allocates a new bool, sets it to
*updatedConfig.IsPingAvailable (or the value) and assigns that new pointer to
newConfig.IsPingAvailable to avoid sharing the caller's memory.
In `@framework/configstore/migrations.go`:
- Around line 5036-5065: The migrationAddMCPClientAllowedExtraHeadersJSONColumn
migration should explicitly backfill existing NULL values to an empty JSON array
after adding the column: inside the Migrate function (after migrator.AddColumn)
execute an UPDATE against the mcp_client table via the current tx (e.g.,
tx.Exec(...)) to set allowed_extra_headers_json = '[]' where
allowed_extra_headers_json IS NULL and return any error; keep the Rollback
as-is. Ensure the UPDATE uses the same tx.WithContext(ctx) and check/return
errors so existing rows become an explicit empty array rather than NULL.
In `@framework/configstore/rdb.go`:
- Line 1117: Remove the manual "updated_at": time.Now() assignment in the
update/save call and let GORM populate UpdatedAt automatically via the
TableMCPClient model's UpdatedAt field tagged with gorm:"autoUpdateTime"; locate
the code that sets "updated_at" (string key "updated_at") in the map or values
being passed to the DB update (around TableMCPClient usage) and delete that key
so GORM's autoUpdateTime on TableMCPClient.UpdatedAt handles timestamp updates.
In `@transports/bifrost-http/lib/ctx_test.go`:
- Around line 72-73: The test suite never exercises a non-empty MCP header
allowlist: update ctx_test.go to add a case that calls ConvertToBifrostContext
with a non-empty schemas.WhiteList (e.g., include one or two allowed header
names) instead of schemas.WhiteList{} and provide incoming headers containing
allowed and denylisted names; then assert that the returned context value under
schemas.BifrostContextKeyMCPExtraHeaders contains only the allowed headers and
does not include denylisted ones. Locate the usage of ConvertToBifrostContext
and add a small subtest that constructs the whitelist, invokes
ConvertToBifrostContext(ctx, false, matcher, yourWhiteList), and checks the
context lookup for schemas.BifrostContextKeyMCPExtraHeaders to verify correct
inclusion/exclusion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbe93e64-decf-460a-af72-778623502d0e
📒 Files selected for processing (30)
core/changelog.mdcore/mcp/clientmanager.gocore/mcp/codemode.gocore/mcp/codemode/starlark/executecode.gocore/mcp/codemode/starlark/starlark.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/mcp/utils/utils.gocore/schemas/bifrost.gocore/schemas/mcp.goexamples/mcps/auth-demo-server/main.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- core/mcp/utils/utils.go
- ui/lib/types/mcp.ts
- core/mcp/codemode.go
- transports/bifrost-http/lib/config.go
- core/mcp/mcp.go
- transports/bifrost-http/integrations/router.go
- core/changelog.md
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/handlers/mcpserver.go
- core/schemas/bifrost.go
- ui/lib/types/schemas.ts
- framework/changelog.md
Pratham-Mishra04
force-pushed
the
03-17-feat_added_support_for_extra_headers_in_mcp_execute_tool
branch
from
b523d65 to
a38f750
Compare
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
75d59f2 to
1a620fd
Compare
Collaborator
Author
Merge activity
|
Pratham-Mishra04
changed the base branch from
03-17-refactor_added_whitelist_type
to
graphite-base/2126
Pratham-Mishra04
force-pushed
the
03-17-feat_added_support_for_extra_headers_in_mcp_execute_tool
branch
from
a38f750 to
6e9e351
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/mcp.go (1)
200-215:⚠️ Potential issue | 🔴 CriticalReturn
AllowedExtraHeadersfrom the paginated reader too.The paginated GET path drops this new field when rebuilding
schemas.MCPClientConfig. That makes the sheet loadallowed_extra_headersas missing, initialize it to[], and the next save clears the stored allowlist becausemergeMCPRedactedValues()only preserves the old value when the incoming field isnil.Suggested fix
clientConfig := &schemas.MCPClientConfig{ ID: dbClient.ClientID, Name: dbClient.Name, IsCodeModeClient: dbClient.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(dbClient.ConnectionType), ConnectionString: dbClient.ConnectionString, StdioConfig: dbClient.StdioConfig, AuthType: schemas.MCPAuthType(dbClient.AuthType), OauthConfigID: dbClient.OauthConfigID, ToolsToExecute: dbClient.ToolsToExecute, ToolsToAutoExecute: dbClient.ToolsToAutoExecute, Headers: dbClient.Headers, + AllowedExtraHeaders: dbClient.AllowedExtraHeaders, IsPingAvailable: &isPingAvailable, ToolSyncInterval: time.Duration(dbClient.ToolSyncInterval) * time.Minute, ToolPricing: dbClient.ToolPricing, }🤖 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 200 - 215, The paginated GET path reconstructs schemas.MCPClientConfig without setting AllowedExtraHeaders, causing the field to be treated as missing and later cleared by mergeMCPRedactedValues; fix by including the AllowedExtraHeaders field when building the MCPClientConfig in the paginated reader (add AllowedExtraHeaders: dbClient.AllowedExtraHeaders to the struct literal used to create schemas.MCPClientConfig) so the value is preserved across read/merge operations.
♻️ Duplicate comments (2)
transports/changelog.md (1)
5-5:⚠️ Potential issue | 🟡 MinorFix hyphenation in changelog entry.
Line 5 should use "request-level" (hyphenated) for correct compound modifier usage.
✏️ Suggested fix
-- feat: add support for request level extra headers in MCP tool execution. +- feat: add support for request-level extra headers in MCP tool execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/changelog.md` at line 5, Update the changelog entry text "feat: add support for request level extra headers in MCP tool execution." to hyphenate the compound modifier by changing "request level" to "request-level" so the line reads "feat: add support for request-level extra headers in MCP tool execution."; edit the existing string in transports/changelog.md where that exact entry appears.transports/bifrost-http/lib/ctx.go (1)
382-386:⚠️ Potential issue | 🟠 MajorMCP allowlist branch currently preempts internal control-header handling.
At Line 383, an allow-all (or broad) MCP allowlist can consume headers before later handlers run (Line 388+), which changes behavior for
x-bf-send-back-raw-response,x-bf-parent-request-id, andx-bf-passthrough-extra-params.🔧 Suggested fix
- // Handle MCP extra headers - if mcpHeaderCombinedAllowlist.IsAllowed(keyStr) { + // Handle MCP extra headers, but do not intercept internal x-bf control headers + if keyStr != "x-bf-send-back-raw-response" && + keyStr != "x-bf-parent-request-id" && + keyStr != "x-bf-passthrough-extra-params" && + mcpHeaderCombinedAllowlist.IsAllowed(keyStr) { mcpExtraHeaders[keyStr] = append(mcpExtraHeaders[keyStr], string(value)) return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 382 - 386, The MCP allowlist branch (mcpHeaderCombinedAllowlist.IsAllowed) is currently consuming headers before internal control-header handling; update the logic so the control headers ("x-bf-send-back-raw-response", "x-bf-parent-request-id", "x-bf-passthrough-extra-params") are processed first (or explicitly excluded from the allowlist branch) and only then fall back to adding to mcpExtraHeaders; locate the block referencing mcpHeaderCombinedAllowlist and mcpExtraHeaders in the same function in ctx.go and either move the allowlist check after the control-header handling or add a conditional that bypasses the allowlist for those three keys so their existing handlers run as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 413-418: The input is re-parsed on every keystroke causing the
"snap-back" when typing commas; fix it by tracking the raw CSV string separately
(e.g., const [rawCsv, setRawCsv] = useState(() => (field.value || []).join(",
"))) and use rawCsv as the Input value, update rawCsv in the Input onChange, and
only parse into an array and call field.onChange(parsed) on blur or form submit
(create an onBlur handler that does const parsed = rawCsv.trim() ?
rawCsv.split(",").map(s => s.trim()).filter(Boolean) : [] and then
field.onChange(parsed)); also add a useEffect to sync rawCsv when field.value
changes externally so editing stays consistent.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 200-215: The paginated GET path reconstructs
schemas.MCPClientConfig without setting AllowedExtraHeaders, causing the field
to be treated as missing and later cleared by mergeMCPRedactedValues; fix by
including the AllowedExtraHeaders field when building the MCPClientConfig in the
paginated reader (add AllowedExtraHeaders: dbClient.AllowedExtraHeaders to the
struct literal used to create schemas.MCPClientConfig) so the value is preserved
across read/merge operations.
---
Duplicate comments:
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 382-386: The MCP allowlist branch
(mcpHeaderCombinedAllowlist.IsAllowed) is currently consuming headers before
internal control-header handling; update the logic so the control headers
("x-bf-send-back-raw-response", "x-bf-parent-request-id",
"x-bf-passthrough-extra-params") are processed first (or explicitly excluded
from the allowlist branch) and only then fall back to adding to mcpExtraHeaders;
locate the block referencing mcpHeaderCombinedAllowlist and mcpExtraHeaders in
the same function in ctx.go and either move the allowlist check after the
control-header handling or add a conditional that bypasses the allowlist for
those three keys so their existing handlers run as intended.
In `@transports/changelog.md`:
- Line 5: Update the changelog entry text "feat: add support for request level
extra headers in MCP tool execution." to hyphenate the compound modifier by
changing "request level" to "request-level" so the line reads "feat: add support
for request-level extra headers in MCP tool execution."; edit the existing
string in transports/changelog.md where that exact entry appears.
🪄 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: 7f455cfc-cfd1-4569-abfe-3a2a1013287e
📒 Files selected for processing (30)
core/changelog.mdcore/mcp/clientmanager.gocore/mcp/codemode.gocore/mcp/codemode/starlark/executecode.gocore/mcp/codemode/starlark/starlark.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/mcp/utils/utils.gocore/schemas/bifrost.gocore/schemas/mcp.goexamples/mcps/auth-demo-server/main.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- ui/lib/types/mcp.ts
- core/mcp/mcp.go
- core/mcp/utils/utils.go
- transports/bifrost-http/integrations/bedrock_test.go
- transports/bifrost-http/handlers/inference.go
- transports/bifrost-http/lib/config_test.go
- framework/changelog.md
- core/mcp/codemode/starlark/starlark.go
Comment on lines
+413
to
+418
| value={(field.value || []).join(", ")} | ||
| onChange={(e) => { | ||
| const raw = e.target.value; | ||
| const parsed = raw.trim() ? raw.split(",").map((h) => h.trim()).filter(Boolean) : []; | ||
| field.onChange(parsed); | ||
| }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and view the file
git ls-files | grep -i mcpClientSheetRepository: maximhq/bifrost
Length of output: 113
🏁 Script executed:
# Get the file and view the specific lines mentioned
cat -n ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx | sed -n '74,77p; 89,91p; 106,108p; 387,428p'Repository: maximhq/bifrost
Length of output: 2311
🏁 Script executed:
# Check for any PR description or stack context
if [ -f .git/COMMIT_EDITMSG ]; then cat .git/COMMIT_EDITMSG; fiRepository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Also check git log for recent commits to understand context
git log --oneline -n 20Repository: maximhq/bifrost
Length of output: 125
🏁 Script executed:
# Check if there's stack context in git history or branch info
git branch -a | head -20Repository: maximhq/bifrost
Length of output: 194
🏁 Script executed:
# Check if there are related changes in recent commits
git log --oneline -n 10 -- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsxRepository: maximhq/bifrost
Length of output: 125
🏁 Script executed:
# Check Prettier formatting of this file
if command -v prettier &> /dev/null; then
prettier --check ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx 2>&1 | head -20
else
echo "Prettier not available in sandbox"
fiRepository: maximhq/bifrost
Length of output: 92
🏁 Script executed:
# Check if there are any other form fields with similar CSV patterns
rg -A 3 'split\(","\)' ui/ --type tsx --type tsRepository: maximhq/bifrost
Length of output: 86
🏁 Script executed:
# Search for CSV split patterns in TypeScript files
rg -A 3 'split\(","\)' ui/ --type tsRepository: maximhq/bifrost
Length of output: 2248
🏁 Script executed:
# Check the current branch and commit details
git statusRepository: maximhq/bifrost
Length of output: 326
🏁 Script executed:
# Look for any PR description or metadata files
find . -maxdepth 2 -name "*.md" -o -name ".github" -type d | head -10Repository: maximhq/bifrost
Length of output: 251
🏁 Script executed:
# Check commit message details
git show --no-patch --format='%B' a38f750cRepository: maximhq/bifrost
Length of output: 117
🏁 Script executed:
# Check the related branch for context
git show 03-17-refactor_added_whitelist_type --stat | head -30Repository: maximhq/bifrost
Length of output: 1712
🏁 Script executed:
# Check if the mcpClientSheet file appears in the related branch
git diff main..03-17-refactor_added_whitelist_type -- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx | head -50Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Look for the full context of the controlled input - check if field.value is properly typed as array
cat -n ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx | sed -n '1,50p'Repository: maximhq/bifrost
Length of output: 2930
Track raw CSV text separately to fix snap-back behavior.
The controlled Input snaps back to previous state when the user types a comma because the field rebuilds its display from the parsed array on every keystroke. When the user types authorization, x-user-id, after the comma the input re-renders from ["authorization"].join(", ") = "authorization", losing the unfinished entry. This breaks the primary workflow of entering headers by typing.
Instead of synchronously re-parsing the raw text into the array in onChange, maintain the raw input separately (e.g., in component state or a separate form field slice) and only parse it on blur or submission. This allows the user to type freely without the display collapsing mid-entry.
🤖 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 413 -
418, The input is re-parsed on every keystroke causing the "snap-back" when
typing commas; fix it by tracking the raw CSV string separately (e.g., const
[rawCsv, setRawCsv] = useState(() => (field.value || []).join(", "))) and use
rawCsv as the Input value, update rawCsv in the Input onChange, and only parse
into an array and call field.onChange(parsed) on blur or form submit (create an
onBlur handler that does const parsed = rawCsv.trim() ? rawCsv.split(",").map(s
=> s.trim()).filter(Boolean) : [] and then field.onChange(parsed)); also add a
useEffect to sync rawCsv when field.value changes externally so editing stays
consistent.
This was referenced Mar 23, 2026
Merged
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
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
AllowedExtraHeadersfield to MCP client configuration with allowlist semantics (empty array = deny all,["*"]= allow all)BifrostContextKeyMCPExtraHeaderscontext key to track headers forwarded to MCP toolscore/mcp/utilspackage withGetHeadersForToolExecutionfunction to merge static and dynamic headersallowed_extra_headers_jsoncolumn in MCP client tableType of change
Affected areas
How to test
{ "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": ["*"] }Breaking changes
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
Checklist
docs/contributing/README.mdand followed the guidelines