feat: add MCP server implementation#936
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a new MCP HTTP server handler with per-virtual-key scoping, JSON-RPC and SSE endpoints, tool discovery/execution integration, synchronization of global and per-VK MCP servers, and related APIs and wiring across core, transports, and UI. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler as MCPServerHandler
participant VKExtractor as VK Extractor
participant Router as MCP Server Lookup
participant MCPSrvr as MCP Server (global/per-VK)
participant ToolMgr as MCPToolManager
Client->>Handler: POST /mcp (JSON-RPC)
activate Handler
Handler->>VKExtractor: extract VK from headers
VKExtractor-->>Handler: vk
Handler->>Router: resolve MCP server for vk
Router-->>Handler: MCP server instance
Handler->>MCPSrvr: forward JSON-RPC message
MCPSrvr->>ToolMgr: ExecuteTool(toolCall)
ToolMgr-->>MCPSrvr: tool result
MCPSrvr-->>Handler: JSON response
Handler-->>Client: HTTP 200 + JSON
deactivate Handler
note over Handler,Client: SSE flow (GET /mcp)
Client->>Handler: GET /mcp (Accept: text/event-stream)
Handler->>Client: send initial connection event
Handler->>Client: stream events until cancel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (15)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
transports/bifrost-http/handlers/mcp_server.go (3)
92-97: Consider logging when parameter schema marshaling fails.The error from
json.Marshal(toolFunc.Parameters)is silently ignored at line 93. While this won't break functionality, logging a warning would help diagnose tool definition issues.// Convert parameters schema if toolFunc.Parameters != nil { - if paramsJSON, err := json.Marshal(toolFunc.Parameters); err == nil { + paramsJSON, err := json.Marshal(toolFunc.Parameters) + if err != nil { + logger.Warn(fmt.Sprintf("Failed to marshal parameters for tool %s: %v", toolFunc.Name, err)) + } else { // Use RawInputSchema to accept arbitrary JSON schema mcpTool.RawInputSchema = paramsJSON } }
169-174: Per-request tool synchronization may impact performance.
syncToolsToServeris called on every POST request, which involves fetching all MCP clients and re-registering all tools. For high-traffic scenarios, consider caching tools and syncing on a schedule or when clients change, rather than on every request.
210-214: Review wildcard CORS header for SSE endpoint.
Access-Control-Allow-Origin: *allows any origin to access this SSE endpoint. If governance/virtual key authentication is enforced, consider restricting CORS to configured allowed origins froms.Config.ClientConfig.AllowedOriginsfor defense in depth.// Set SSE headers ctx.SetContentType("text/event-stream") ctx.Response.Header.Set("Cache-Control", "no-cache") ctx.Response.Header.Set("Connection", "keep-alive") - ctx.Response.Header.Set("Access-Control-Allow-Origin", "*") + // Use configured allowed origins if available, otherwise use wildcard + origin := string(ctx.Request.Header.Peek("Origin")) + if h.config != nil && len(h.config.ClientConfig.AllowedOrigins) > 0 { + for _, allowed := range h.config.ClientConfig.AllowedOrigins { + if allowed == "*" || allowed == origin { + ctx.Response.Header.Set("Access-Control-Allow-Origin", allowed) + break + } + } + } else { + ctx.Response.Header.Set("Access-Control-Allow-Origin", "*") + }transports/go.mod (1)
10-10: Consider updating to mcp-go v0.43.0 for robustness improvements.Verification confirms v0.41.1 has no known security vulnerabilities. However, the latest stable version is v0.43.0 (released Nov 2, 2025). The intervening releases include robustness improvements—notably v0.42.0 made transport
Start()idempotent and added session-specific resource support, which are beneficial for the new MCP server implementation. Updating is optional but recommended for improved stability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/server/server.go(2 hunks)transports/go.mod(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
transports/bifrost-http/server/server.go (11)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(483-489)ui/lib/types/logs.ts (2)
ChatMessage(115-123)BifrostError(223-229)core/schemas/bifrost.go (1)
BifrostError(354-363)transports/bifrost-http/handlers/mcp_server.go (1)
NewMCPServerHandler(37-51)transports/bifrost-http/lib/config.go (1)
Config(140-170)framework/modelcatalog/main.go (1)
Config(26-29)framework/logstore/config.go (1)
Config(13-18)transports/bifrost-http/handlers/config.go (1)
NewConfigHandler(41-46)transports/bifrost-http/handlers/plugins.go (1)
NewPluginsHandler(30-35)framework/configstore/store.go (1)
ConfigStore(17-146)transports/bifrost-http/handlers/session.go (1)
NewSessionHandler(24-31)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (6)
transports/bifrost-http/handlers/mcp_server.go (4)
22-25: LGTM!The
MCPToolExecutorinterface is well-defined with a focused contract matching the Bifrost tool execution pattern.
248-251: Verify fail-open behavior when config is nil.When
h.configis nil, the function returnstrue(allowing access). Confirm this fail-open behavior is intentional. If this endpoint should be secure by default, consider returningfalsewhen config is unavailable.
269-292: LGTM!The virtual key extraction logic correctly handles multiple header formats (
x-bf-vk,Authorization Bearer, andx-api-key) with proper prefix validation.
36-51:versionvariable verified—no changes needed.The
versionvariable is properly defined intransports/bifrost-http/handlers/init.go:5, making the reference in the constructor valid. The code is correct as written.transports/bifrost-http/server/server.go (2)
454-457: LGTM!The
ExecuteToolmethod correctly implements theMCPToolExecutorinterface by delegating to the Bifrost client'sExecuteMCPToolmethod.
898-906: LGTM!The MCP server handler is correctly instantiated with the server implementing
MCPToolExecutor. The unconditional route registration is appropriate since the MCP server functionality depends on the Bifrost client, which is always available at this point.
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
from
be0c1a9 to
ee7366f
Compare
Pratham-Mishra04
changed the base branch from
11-18-feat_added_codemode_to_mcp
to
graphite-base/936
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/mcp_server.go (1)
78-147: Handle JSON marshal failures for tool arguments (currently ignored).In the tool handler,
argsJSON, _ := json.Marshal(request.GetArguments())silently discards marshal errors, which can lead to callingExecuteToolwith an empty/incorrectArgumentsstring and confusing downstream behavior. This was already flagged in an earlier review; it’s still worth fixing.You can safely fail the tool call with an MCP error when marshaling fails, e.g.:
- toolName := toolFunc.Name - handler := func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { + toolName := toolFunc.Name + handler := func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // Convert to Bifrost tool call format toolCallType := "function" toolCallID := fmt.Sprintf("mcp-%s", toolName) - argsJSON, _ := json.Marshal(request.GetArguments()) + argsJSON, err := json.Marshal(request.GetArguments()) + if err != nil { + // Surface the real problem back to the MCP client + return mcp.NewToolResultError( + fmt.Sprintf("Failed to marshal tool arguments for %s: %v", toolName, err), + ), nil + } toolCall := schemas.ChatAssistantMessageToolCall{ ID: &toolCallID, Type: &toolCallType, Function: schemas.ChatAssistantMessageToolCallFunction{ Name: &toolName, Arguments: string(argsJSON), }, }Optionally, for the earlier
json.Marshal(toolFunc.Parameters)when buildingmcpTool.RawInputSchema, consider at least logging the error instead of silently dropping the schema; otherwise, clients may be surprised by missing input schemas.
🧹 Nitpick comments (4)
docs/features/governance/virtual-keys.mdx (1)
560-570: Error message copy is clear; consider cross-endpoint consistency.The updated
"virtual key is missing in headers"message reads well and matches the header-based VK requirement. If you want a consistent UX across endpoints (chat completions vs new/mcphandler), consider aligning the wording and possibly status codes for the “missing VK” case so clients see the sametype/message everywhere. Not blocking.core/mcp/utils.go (1)
49-81: Name-keyed tool map and logging behavior look sound; assumes unique client names.Using
client.ExecutionConfig.Nameas the map key and for include filtering matches theclientName/toolNamepatterns used elsewhere (e.g., in code-mode and include-tools contexts), and the added debug logs should help diagnose filtering. This does implicitly assume client names are unique; if there’s any chance of duplicate names in config, tools from multiple clients will be merged under the same key, so either rely onvalidateMCPClientConfigto enforce uniqueness or document that requirement.transports/bifrost-http/handlers/mcp_server.go (1)
247-292: Virtual key enforcement and header extraction are reasonable; consider reusing shared helpers/messages.
requireVirtualKeycorrectly checksEnforceGovernanceHeaderunder the config mutex and enforces a VK fromx-bf-vk,Authorization: Bearer sk-bf-*, orx-api-key(withVirtualKeyPrefix) viaextractVirtualKeyFromHeaders. That matches the documented VK header options and keeps old-style keys limited tox-bf-vk.The error response text here differs from the generic “virtual key is missing in headers” example in the governance docs, and other handlers may have their own variants. If you want a uniform experience, consider centralizing this VK-missing error message (and status code) in a shared helper so all HTTP paths, including
/mcp, return the same shape.transports/bifrost-http/server/server.go (1)
454-457: ExecuteTool delegation is fine; consider a defensive nil check for Client.This thin wrapper around
s.Client.ExecuteMCPToolis appropriate for satisfyingMCPToolExecutor. Given current call sites only execute afterBootstrapinitializess.Client, it’s safe; if there’s any chance of future reuse before initialization, you might defensively guard againsts.Client == niland return aBifrostError(e.g., 503) instead of panicking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/mcp/utils.go(6 hunks)docs/features/governance/virtual-keys.mdx(1 hunks)plugins/governance/main.go(1 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/server/server.go(2 hunks)transports/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugins/governance/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/go.mod
🧰 Additional context used
🧬 Code graph analysis (2)
core/mcp/utils.go (1)
core/mcp/mcp.go (2)
MCPLogPrefix(23-23)MCPContextKeyIncludeTools(31-31)
transports/bifrost-http/server/server.go (3)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(483-489)core/schemas/bifrost.go (1)
BifrostError(354-363)transports/bifrost-http/handlers/mcp_server.go (1)
NewMCPServerHandler(37-51)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (3)
core/mcp/utils.go (2)
117-137: Client include-list semantics are clear and consistent.The
shouldIncludeClientlogic (nil → include all; empty slice → include none;"*"wildcard; otherwise exact-match on client name) matches the comment and mirrors the tools filtering semantics. This is a straightforward and predictable contract.
188-218: Request-scoped tool filtering logic is correct and matches client-name patterns.
shouldSkipToolForRequestcorrectly interpretsincludeToolsas:
nil→ allow all tools,[]→ skip all,"clientName/*"→ allow all tools for that client,"clientName/toolName"→ allow specific tools only.The new
clientNameparameter andfullToolName := fmt.Sprintf("%s/%s", clientName, toolName)keep this in sync with the rest of the client-name–based filtering. Looks good.transports/bifrost-http/server/server.go (1)
897-907: MCP server handler wiring looks correct and integrates cleanly.Constructing
mcpServerHandler := handlers.NewMCPServerHandler(s.Client, s.Config, s)here and registering its routes alongsidemcpHandlerensures/mcpendpoints participate in the same API middleware chain (auth, telemetry, governance) as other API routes. The separation between MCP client management (mcpHandler) and MCP server JSON-RPC/SSE (mcpServerHandler) is clear.
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
from
ee7366f to
fef190b
Compare
Pratham-Mishra04
changed the base branch from
graphite-base/936
to
11-18-feat_added_codemode_to_mcp
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/mcp/utils.go (1)
30-83: Assumption of client name uniqueness is NOT enforced at runtime.The code assumes
client.ExecutionConfig.Nameis unique, but verification reveals a critical gap:
AddClient()only validates ID uniqueness (if _, ok := m.clientMap[config.ID]), not Name uniquenessvalidateMCPClientConfig()does not check Name uniqueness at all- Database schema enforces Name uniqueness via unique index, but this protection is bypassed for programmatically-created configs
GetToolPerClient()uses Name as a map key, so duplicate names would cause silent data loss (second client overwrites first in the returned map)Add Name uniqueness validation to
AddClient()or document why duplicates are safe.
🧹 Nitpick comments (2)
core/mcp/toolmanager.go (1)
206-222: Consider extracting tool availability check to a helper.The nested loop for checking tool availability (O(n×m) where n = clients, m = tools) works but could be cleaner as a helper method on
ClientManager. However, for typical tool counts this is acceptable.core/mcp/codemode_executecode.go (1)
656-670: Consider simplifying client lookup.The current approach finds a tool from the client, then uses
GetClientForToolto get the client. SinceclientNameis already available, consider adding aGetClientByNamelookup directly (if the interface supports it) or clarifying why this indirection is necessary.A simpler approach could be:
- var client *schemas.MCPClientState - for _, tool := range tools { - if tool.Function != nil && tool.Function.Name != "" { - client = h.clientManager.GetClientForTool(tool.Function.Name) - if client != nil { - break - } - } - } + client := h.clientManager.GetClientByName(clientName)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/mcp/codemode_executecode.go(19 hunks)core/mcp/codemode_listfiles.go(3 hunks)core/mcp/codemode_readfile.go(12 hunks)core/mcp/mcp.go(1 hunks)core/mcp/toolmanager.go(10 hunks)core/mcp/utils.go(8 hunks)docs/features/governance/virtual-keys.mdx(1 hunks)plugins/governance/main.go(1 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/server/server.go(2 hunks)transports/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/features/governance/virtual-keys.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/governance/main.go
- transports/go.mod
- transports/bifrost-http/handlers/mcp_server.go
🧰 Additional context used
🧬 Code graph analysis (7)
transports/bifrost-http/server/server.go (3)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(483-489)core/schemas/bifrost.go (1)
BifrostError(354-363)transports/bifrost-http/handlers/mcp_server.go (1)
NewMCPServerHandler(37-51)
core/mcp/codemode_listfiles.go (2)
core/mcp/toolmanager.go (1)
ToolsHandler(20-26)core/schemas/chatcompletions.go (2)
ChatTool(201-205)ChatAssistantMessageToolCall(483-489)
core/mcp/mcp.go (1)
core/mcp/toolmanager.go (2)
NewToolsHandler(39-60)ToolsHandlerConfig(28-31)
core/mcp/toolmanager.go (3)
core/schemas/mcp.go (2)
MCPClientState(83-90)BifrostLLMCaller(13-15)core/schemas/chatcompletions.go (4)
ChatTool(201-205)ChatAssistantMessageToolCall(483-489)BifrostChatRequest(11-18)BifrostChatResponse(25-40)core/mcp/agent.go (1)
ExecuteAgent(14-267)
core/mcp/codemode_readfile.go (2)
core/mcp/toolmanager.go (1)
ToolsHandler(20-26)core/schemas/chatcompletions.go (2)
ChatTool(201-205)ChatAssistantMessageToolCall(483-489)
core/mcp/utils.go (4)
core/mcp/mcp.go (3)
MCPManager(41-56)MCPLogPrefix(23-23)MCPContextKeyIncludeTools(31-31)transports/bifrost-http/handlers/mcp.go (1)
MCPManager(19-23)core/schemas/mcp.go (1)
MCPClientState(83-90)core/schemas/chatcompletions.go (1)
ChatTool(201-205)
core/mcp/codemode_executecode.go (2)
core/mcp/toolmanager.go (1)
ToolsHandler(20-26)core/schemas/chatcompletions.go (1)
ChatTool(201-205)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (26)
core/mcp/codemode_listfiles.go (2)
12-12: LGTM on receiver rename.The receiver rename from
mtohaligns with theToolsHandlertype convention and is consistent across the codebase refactor.
35-59: Clean migration to ClientManager abstraction.The refactor correctly replaces the old fetcher function pattern with the new
ClientManagerinterface methods (GetToolPerClient,GetClientByName). The nil check on line 48 and the code-mode client filtering logic remain intact.transports/bifrost-http/server/server.go (2)
898-906: MCP server handler registration follows established patterns.The handler is created and routes are registered consistently with other handlers (mcpHandler, configHandler, etc.). Passing
sas thetoolExecutorcorrectly allows the MCP server to execute tools through the Bifrost client.
454-457: ExecuteTool method implementation verified and approved.The method correctly delegates to
s.Client.ExecuteMCPTool(), which exists atcore/bifrost.go:1112with matching signatures. The underlying implementation includes proper nil checks and error handling for the MCP manager, ensuring robust execution of tool calls.core/mcp/mcp.go (1)
94-101: Constructor-based initialization is a solid improvement.The refactor from setter-based to constructor-based dependency injection is cleaner and ensures all required dependencies are provided at creation time. Error handling properly propagates failures with context via
%w.core/mcp/codemode_readfile.go (3)
13-48: LGTM on receiver rename and tool definition.The receiver rename to
his consistent with the ToolsHandler convention. The tool definition is well-documented with clear parameter descriptions.
51-165: Handler logic is correct with proper validation.The migration to
ClientManageris complete. Line validation (142-157) correctly handles edge cases:
- Start/end line bounds checking
- Start > end validation
- Clear error messages with context
One minor note: the multiple-match detection (lines 83-97) works correctly but rebuilds
availableFilestwice (once for error message, once unused). This is not a bug, just slightly redundant.
167-355: Package-level helper functions are well-structured.Moving these functions to package level is appropriate since they don't require
ToolsHandlerstate. The implementations are correct:
generateTypeDefinitions: Comprehensive TypeScript generation with helpful commentsjsonSchemaToTypeScript: Handles common JSON Schema types including recursive array itemstoPascalCase: Properly handles empty strings and strings without delimiterscore/mcp/utils.go (4)
17-28: GetClientForTool implementation is correct.Proper RLock usage for thread-safe read access. The linear search through clients and their tool maps is acceptable given typical MCP client counts.
85-101: GetClientByName implementation is straightforward.Proper RLock usage and linear search by
ExecutionConfig.Name. Documentation is clear with parameter and return value descriptions.
136-155: shouldIncludeClient correctly updated to use clientName.The signature change from ID-based to name-based filtering is consistent with the broader refactor. The filtering logic (nil → include all, empty → include none, "*" → include all, explicit list → check membership) is well-documented and correct.
206-236: shouldSkipToolForRequest properly uses clientName patterns.The pattern matching for wildcards (
clientName/*) and specific tools (clientName/toolName) correctly uses the client name. The debug logging at line 209 will help with troubleshooting tool filtering issues.core/mcp/toolmanager.go (5)
14-18: Well-designed interface abstraction.The
ClientManagerinterface provides a clean abstraction for client and tool management, improving testability and decoupling theToolsHandlerfrom concrete implementations.
39-60: LGTM on constructor changes.The nil check for
clientManageris appropriate, and the constructor properly initializes the handler with sensible defaults for config values.
76-101: Clean migration to ClientManager abstraction.The method correctly uses
clientManager.GetToolPerClientandclientManager.GetClientByNamefor tool discovery and client lookup, maintaining the same logic flow.
272-285: LGTM on ExecuteAgent delegation.The method correctly bridges the
ClientManagerinterface to theExecuteAgentfunction by passing method references, maintaining backward compatibility with the existing function signature.
290-334: LGTM on buildAllowedAutoExecutionTools.The method correctly uses
ClientManagerfor client and tool retrieval, properly handles the wildcard case, and maintains clean separation of concerns.core/mcp/codemode_executecode.go (9)
64-105: Well-documented tool definition.The tool description comprehensively explains the execution environment constraints (no browser/Node APIs, TypeScript transpilation, async/await support), which helps LLMs use the tool correctly.
108-133: LGTM on handleExecuteToolCode.The method properly handles argument parsing and delegates to
executeCode. The detailed response formatting (including the "no data" case) provides valuable feedback for LLM debugging.
236-268: Good early return for empty code.The method correctly handles the edge case where code becomes empty after stripping imports/exports, returning early with appropriate environment info.
338-374: LGTM on tool bindings setup.The code correctly filters for code mode clients and creates appropriate bindings with parsed tool names for JavaScript property compatibility.
442-476: Good panic recovery in tool call goroutine.The goroutine has proper panic recovery and context cancellation checks. However, note that if a timeout occurs, the goroutine may continue running until the MCP tool call completes, though it will safely reject the promise.
391-395: LGTM on thread-safe logging.Proper mutex usage ensures safe concurrent access to the logs slice from multiple goroutines.
720-746: Good extraction of helper functions to package level.Moving
formatResultForLogandformatConsoleArgsto package-level pure functions improves testability and reusability.
749-778: LGTM on import/export stripping logic.The regex-based approach correctly identifies and strips ES module syntax while preserving line number tracking for debugging.
817-896: Comprehensive runtime error hints.The error hint generation provides excellent developer experience with specific guidance for common issues (undefined fetch, setTimeout, require) and generic fallbacks. This helps LLMs understand and recover from errors.
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
from
fef190b to
b8e62ab
Compare
Pratham-Mishra04
force-pushed
the
11-18-feat_added_codemode_to_mcp
branch
from
511c8c9 to
786f7f4
Compare
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
from
b8e62ab to
c6acc12
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
transports/bifrost-http/handlers/mcp_server.go (2)
99-113: Handle JSON marshal error for tool arguments.Line 105 silently ignores the error from
json.Marshal. If marshaling fails,argsJSONwill benil, resulting in an empty string forArguments, which could cause unexpected behavior downstream.Apply this diff to handle the error:
- argsJSON, _ := json.Marshal(request.GetArguments()) + argsJSON, err := json.Marshal(request.GetArguments()) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("Failed to marshal tool arguments: %v", err)), nil + }
198-245: SSE handler cancels the context too early, likely closing the stream immediately.
handleMCPServerSSEdeferscancel()at line 218 right afterConvertToBifrostContext, but the stream writer (lines 225-244) runs after the handler returns. The deferredcancel()will have already fired by the time the stream writer executes, causingDone()at line 242 to be already closed, and the SSE connection will terminate almost immediately instead of staying open.Move
cancel()into the writer closure:- // Convert context - bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false) - defer cancel() + // Convert context + bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false) if bifrostCtx == nil { SendError(ctx, fasthttp.StatusInternalServerError, "Failed to convert context") return } // Use streaming response writer - ctx.Response.SetBodyStreamWriter(func(w *bufio.Writer) { - defer w.Flush() + ctx.Response.SetBodyStreamWriter(func(w *bufio.Writer) { + defer func() { + cancel() + _ = w.Flush() + }() // Send initial connection message initMessage := map[string]interface{}{ "jsonrpc": "2.0", "method": "connection/opened", } if initJSON, err := json.Marshal(initMessage); err == nil { fmt.Fprintf(w, "data: %s\n\n", initJSON) w.Flush() } // Keep connection alive // In a real implementation, you might want to handle incoming messages via a different mechanism // For SSE, the client typically sends messages via POST while receiving via GET - // Wait for context cancellation - <-(*bifrostCtx).Done() - // Context cancelled, close connection + // Wait for context cancellation (client disconnect or server-side cancel) + <-(*bifrostCtx).Done() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/features/governance/virtual-keys.mdx(1 hunks)plugins/governance/main.go(1 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/server/server.go(2 hunks)transports/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/governance/main.go
- docs/features/governance/virtual-keys.mdx
- transports/go.mod
- transports/bifrost-http/server/server.go
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (5)
transports/bifrost-http/handlers/mcp_server.go (5)
22-25: LGTM!The interface is clean and provides a clear contract for executing MCP tools.
36-51: LGTM!The constructor correctly initializes the MCP server with tool capabilities and wires all necessary dependencies.
53-58: LGTM!Route registration correctly exposes both JSON-RPC (POST) and SSE (GET) endpoints with middleware support.
149-196: LGTM!The POST handler correctly implements the JSON-RPC flow with proper context management, virtual key validation, and error handling. The decision to continue if tool sync fails is reasonable for availability.
247-292: LGTM!The governance enforcement logic correctly validates virtual keys across multiple header options (x-bf-vk, Authorization Bearer, x-api-key) with case-insensitive matching and proper prefix validation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
transports/bifrost-http/handlers/mcp_server.go (2)
99-113: Handle JSON marshal error for tool arguments.The error from
json.Marshalon line 105 is silently ignored. If marshaling fails,argsJSONwill benil, resulting in an empty string forArguments, which could cause unexpected behavior or silent failures downstream.- argsJSON, _ := json.Marshal(request.GetArguments()) + argsJSON, err := json.Marshal(request.GetArguments()) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("Failed to marshal tool arguments: %v", err)), nil + }
217-244: SSE handler cancels context too early, terminating the stream immediately.The
defer cancel()at line 218 fires when the handler returns, but theSetBodyStreamWritercallback runs asynchronously after the handler returns. This meansbifrostCtxis already canceled when the stream writer starts, causing<-(*bifrostCtx).Done()to return immediately and terminate the SSE connection.Move
cancel()into the stream writer closure to keep the context alive for the stream's lifetime:// Convert context bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false) - defer cancel() if bifrostCtx == nil { + cancel() SendError(ctx, fasthttp.StatusInternalServerError, "Failed to convert context") return } // Use streaming response writer ctx.Response.SetBodyStreamWriter(func(w *bufio.Writer) { - defer w.Flush() + defer func() { + cancel() + w.Flush() + }() // Send initial connection message initMessage := map[string]interface{}{
🧹 Nitpick comments (1)
transports/bifrost-http/handlers/mcp_server.go (1)
169-174: Consider caching or debouncing tool synchronization.
syncToolsToServeris called on every POST request, clearing and re-registering all tools. For deployments with many MCP clients or tools, this could add latency to each request. Consider caching the tools and only re-syncing when clients change, or using a periodic background sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/features/governance/virtual-keys.mdx(1 hunks)plugins/governance/main.go(1 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/server/server.go(2 hunks)transports/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/features/governance/virtual-keys.mdx
- plugins/governance/main.go
- transports/go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
transports/bifrost-http/server/server.go (3)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(483-489)core/schemas/bifrost.go (1)
BifrostError(354-363)transports/bifrost-http/handlers/mcp_server.go (1)
NewMCPServerHandler(37-51)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (9)
transports/bifrost-http/handlers/mcp_server.go (7)
1-20: LGTM!Package declaration and imports are appropriate for the MCP server implementation.
22-34: LGTM!The
MCPToolExecutorinterface provides a clean abstraction for tool execution, and theMCPServerHandlerstruct appropriately encapsulates all dependencies needed for MCP server operations.
53-58: LGTM!Route registration follows the established pattern in the codebase, correctly wiring POST for JSON-RPC and GET for SSE endpoints.
149-196: LGTM!The POST handler correctly validates prerequisites, enforces governance, handles JSON-RPC notifications (nil response), and properly marshals responses. The warning on sync failure with continued execution is appropriate for graceful degradation.
247-267: LGTM!The
requireVirtualKeyfunction correctly uses read locks for thread-safe config access and provides a clear error message when governance enforcement requires a virtual key.
269-292: Code case handling for virtual key prefix is correct.Verification confirms that
governance.VirtualKeyPrefixis defined as"sk-bf-"(lowercase). The function correctly usesstrings.ToLower()on the token and API key values before comparing with the prefix, ensuring proper case-insensitive matching.
36-51: Version variable is properly defined in the handlers package.Verification confirms
versionis declared as a package-level variable intransports/bifrost-http/handlers/init.go(line 5) with getter and setter functions. The usage inNewMCPServerHandlerat line 41 ofmcp_server.gois valid and correct—Go allows any file in the same package to access package-level variables.transports/bifrost-http/server/server.go (2)
898-906: LGTM!The MCP server handler is correctly instantiated with the required dependencies and registered alongside other handlers following the established pattern. Passing
s(the server itself) as theMCPToolExecutoris valid sinceBifrostHTTPServernow implements that interface via theExecuteToolmethod.
454-457: LGTM!The
ExecuteToolmethod cleanly delegates to the core Bifrost client, implementing theMCPToolExecutorinterface for the MCP server handler.
Pratham-Mishra04
changed the base branch from
11-18-feat_added_codemode_to_mcp
to
graphite-base/936
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
from
c6acc12 to
096e23a
Compare
Pratham-Mishra04
force-pushed
the
graphite-base/936
branch
from
786f7f4 to
a533ae9
Compare
Pratham-Mishra04
changed the base branch from
graphite-base/936
to
11-18-feat_mcp_agent_and_codemode_added
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
transports/bifrost-http/handlers/mcp_server.go (3)
91-97: Handle JSON marshal error for tool parameters schema.Line 93 silently ignores the error from
json.Marshal. If marshaling the parameters schema fails,RawInputSchemawill not be set, and the tool will be registered without input validation, potentially allowing invalid arguments.Apply this diff to handle the error:
// Convert parameters schema if toolFunc.Parameters != nil { - if paramsJSON, err := json.Marshal(toolFunc.Parameters); err == nil { - // Use RawInputSchema to accept arbitrary JSON schema - mcpTool.RawInputSchema = paramsJSON - } + paramsJSON, err := json.Marshal(toolFunc.Parameters) + if err != nil { + return fmt.Errorf("failed to marshal parameters for tool %s: %w", toolFunc.Name, err) + } + // Use RawInputSchema to accept arbitrary JSON schema + mcpTool.RawInputSchema = paramsJSON }
99-113: Handle JSON marshal error for tool arguments.Line 105 silently ignores the error from
json.Marshal. If marshaling fails,argsJSONwill benil, resulting in an empty string forArguments, which could cause unexpected behavior downstream.Apply this diff to handle the error:
// Convert to Bifrost tool call format toolCallType := "function" toolCallID := fmt.Sprintf("mcp-%s", toolName) - argsJSON, _ := json.Marshal(request.GetArguments()) + argsJSON, err := json.Marshal(request.GetArguments()) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("Failed to marshal tool arguments: %v", err)), nil + } toolCall := schemas.ChatAssistantMessageToolCall{
198-245: SSE handler cancels the context too early, likely closing the stream immediately.
handleMCPServerSSEdeferscancel()on line 218 right afterConvertToBifrostContext, but the stream writer (line 225) runs asynchronously after the handler returns. The deferredcancel()will fire when the handler exits, causingDone()on line 242 to close immediately, terminating the SSE connection prematurely instead of keeping it alive.Move the
cancel()call into the stream writer to keep the context active for the lifetime of the SSE stream:// Convert context bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false) - defer cancel() if bifrostCtx == nil { SendError(ctx, fasthttp.StatusInternalServerError, "Failed to convert context") return } // Use streaming response writer ctx.Response.SetBodyStreamWriter(func(w *bufio.Writer) { - defer w.Flush() + defer func() { + cancel() + _ = w.Flush() + }() // Send initial connection message
🧹 Nitpick comments (1)
plugins/governance/main.go (1)
72-72: Consider making the Init doc comment header-agnostic for virtual key.The comment still says
config.IsVkMandatorycontrols whetherx-bf-vkis required, but VK can now come from multiple headers. To avoid confusion, you could reword it slightly:-// - `config.IsVkMandatory` controls whether `x-bf-vk` is required in PreHook. +// - `config.IsVkMandatory` controls whether a virtual key is required in PreHook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/features/governance/virtual-keys.mdx(1 hunks)plugins/governance/main.go(1 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/server/server.go(2 hunks)transports/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- transports/bifrost-http/server/server.go
- docs/features/governance/virtual-keys.mdx
- transports/go.mod
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (5)
plugins/governance/main.go (1)
341-347: Message wording change looks correct and more general.The new
"virtual key is missing in headers"message is header-agnostic and better matches flows where VK comes via Authorization orx-api-key, without changing behavior (virtual_key_required, 400). No issues from this change alone.transports/bifrost-http/handlers/mcp_server.go (4)
149-196: LGTM on JSON-RPC handler implementation.The handler correctly validates the tool executor, enforces governance, converts the context, syncs tools (with appropriate warning logging if sync fails), processes the JSON-RPC message, and marshals the response. The
defer cancel()is appropriate here since the handler returns after processing.
247-267: LGTM on virtual key enforcement.The implementation correctly checks the governance enforcement flag using a read lock, validates the presence of a virtual key, and returns an appropriate error when required. Thread-safe access to the config is properly handled.
269-292: LGTM on virtual key extraction logic.The helper correctly checks multiple common header locations (x-bf-vk, Authorization Bearer, x-api-key) and validates the expected
sk-bf-prefix for virtual keys.
41-41: Theversionvariable is properly defined and the review comment concern is unfounded.The
versionvariable is declared as a package-level variable attransports/bifrost-http/handlers/init.go:5(var version string). It is correctly used at line 41 ofmcp_server.goin theNewMCPServerHandlerfunction, and is properly managed through theSetVersion()setter function in the same package. No issue exists.
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
from
096e23a to
87e4214
Compare
Pratham-Mishra04
force-pushed
the
11-18-feat_mcp_agent_and_codemode_added
branch
from
a533ae9 to
3bfdb5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
transports/bifrost-http/handlers/mcp_server.go (3)
91-97: Don’t ignore json.Marshal errors when building RawInputSchema (duplicate of prior review).If marshaling
toolFunc.Parametersfails,RawInputSchemais silently omitted and the tool is registered without input validation, which can mask schema issues.Consider propagating the error so registration fails clearly:
- // Convert parameters schema - if toolFunc.Parameters != nil { - if paramsJSON, err := json.Marshal(toolFunc.Parameters); err == nil { - // Use RawInputSchema to accept arbitrary JSON schema - mcpTool.RawInputSchema = paramsJSON - } - } + // Convert parameters schema + if toolFunc.Parameters != nil { + paramsJSON, err := json.Marshal(toolFunc.Parameters) + if err != nil { + return fmt.Errorf("failed to marshal parameters for tool %s: %w", toolFunc.Name, err) + } + // Use RawInputSchema to accept arbitrary JSON schema + mcpTool.RawInputSchema = paramsJSON + }
99-113: Handle json.Marshal error for tool arguments instead of discarding it (duplicate of prior review).
argsJSON, _ := json.Marshal(request.GetArguments())ignores marshal failures; on error you’ll send an empty/partial argument string toExecuteTool, which is hard to debug.Better to surface the error back to the MCP client:
- argsJSON, _ := json.Marshal(request.GetArguments()) + argsJSON, err := json.Marshal(request.GetArguments()) + if err != nil { + return mcp.NewToolResultError( + fmt.Sprintf("Failed to marshal tool arguments for %s: %v", toolName, err), + ), nil + }This keeps tool behavior aligned with the actual request payload.
198-245: SSE handler cancels context before the stream runs, likely closing the connection immediately (still unresolved).Because
SetBodyStreamWriterruns after the handler returns in fasthttp, thedefer cancel()executes before the writer starts, so<-(*bifrostCtx).Done()will typically unblock right away and terminate the SSE stream.Move
cancel()into the writer closure so the context lives for the lifetime of the SSE connection:- // Convert context - bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false) - defer cancel() - if bifrostCtx == nil { - SendError(ctx, fasthttp.StatusInternalServerError, "Failed to convert context") - return - } + // Convert context + bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false) + if bifrostCtx == nil { + if cancel != nil { + cancel() + } + SendError(ctx, fasthttp.StatusInternalServerError, "Failed to convert context") + return + } @@ - ctx.Response.SetBodyStreamWriter(func(w *bufio.Writer) { - defer w.Flush() + ctx.Response.SetBodyStreamWriter(func(w *bufio.Writer) { + defer func() { + if cancel != nil { + cancel() + } + _ = w.Flush() + }() @@ - // Wait for context cancellation - <-(*bifrostCtx).Done() - // Context cancelled, close connection + // Wait for context cancellation (client disconnect or server-side cancel) + <-(*bifrostCtx).Done()This keeps the SSE connection open until the client disconnects or you cancel the context, then cleans up resources.
🧹 Nitpick comments (4)
transports/bifrost-http/server/server.go (1)
455-458: ExecuteTool delegation looks correct; consider guarding against nil client (optional).The method cleanly satisfies
MCPToolExecutorby delegating toClient.ExecuteMCPTool. If there’s any code path wheres.Clientcould be nil (e.g., tests callingExecuteToolwithoutBootstrap), a quick nil check returning a structuredBifrostErrorwould make this safer, but it’s optional given current bootstrap flow.transports/bifrost-http/handlers/mcp_server.go (3)
60-77: Tool sync strategy may be expensive per-request; consider caching or change tracking.
syncToolsToServerclears all tools and re-adds them from every connected MCP client on each JSON-RPC call. If you expect high QPS or many tools/clients, this could become a noticeable hot path; consider:
- Keeping a cached tool set keyed by MCP client state (e.g., version/etag) and only rebuilding when that changes.
- Or synchronizing tools on client (dis)connect and letting requests use the existing registry.
This is an optimization only if you see performance pressure.
Also applies to: 78-88
121-139: Result extraction is conservative; OK for text tools.The logic to pull either
ContentStror text blocks into a flatresultTextis simple and safe for text-only tool outputs. If you later support richer content (images, structured JSON blocks, etc.), you may want to forward the raw content instead of flattening to text, but that’s outside this PR’s scope.
247-267: Virtual-key enforcement and header parsing are centralized; minor message tweak optional.The
requireVirtualKey+extractVirtualKeyFromHeadershelpers provide a single place to enforce VKs and supportx-bf-vk,Authorization: Bearer sk-bf-*, andx-api-keywith the same prefix, which is good for consistency across endpoints. The error message currently only mentionsx-bf-vkandAuthorization; you might optionally mentionx-api-keyas well so users know that header is supported.Also applies to: 269-292
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/features/governance/virtual-keys.mdx(1 hunks)plugins/governance/main.go(1 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/server/server.go(2 hunks)transports/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/governance/main.go
- transports/go.mod
- docs/features/governance/virtual-keys.mdx
🧰 Additional context used
🧬 Code graph analysis (1)
transports/bifrost-http/server/server.go (1)
transports/bifrost-http/handlers/mcp_server.go (1)
NewMCPServerHandler(37-51)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (3)
transports/bifrost-http/server/server.go (1)
907-908: MCP server handler wiring is consistent and appropriately injected.Creating
mcpServerHandlerwith(s.Client, s.Config, s)and registering its routes alongside the existing MCP handler keeps concerns separated (client vs server) while reusingBifrostHTTPServeras the tool executor. The ordering with other handlers looks fine and shouldn’t conflict assumingNewMCPHandleruses different paths from/mcp.Also applies to: 915-916
transports/bifrost-http/handlers/mcp_server.go (2)
22-35: Interface and handler construction are well-factored.
MCPToolExecutorcleanly abstracts tool execution, andMCPServerHandlerreceives both the core client and executor via DI. Registering both POST and GET/mcpendpoints off the same handler is straightforward and matches the intended JSON-RPC + SSE split.Also applies to: 37-58
149-196: JSON-RPC handler flow and governance check look sound.The POST handler:
- Validates
toolExecutoravailability.- Enforces virtual-key presence via
requireVirtualKey.- Converts to a Bifrost context, syncs tools, and delegates to
mcpServer.HandleMessage.- Correctly handles “notification” messages (nil response) vs normal responses.
The control flow and error handling here are reasonable for an initial MCP JSON-RPC endpoint.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
transports/bifrost-http/server/server.go (1)
455-458: LGTM! Clean delegation to client.The
ExecuteToolmethod properly delegates to the underlying Bifrost client'sExecuteMCPToolmethod, implementing theMCPToolExecutorinterface required by the MCP server handler. This thin wrapper allows the HTTP server to expose tool execution capabilities.Consider adding observability (logging/metrics) around tool execution for better debugging and monitoring, similar to other handler methods in the codebase. For example:
// ExecuteTool executes an MCP tool call and returns the result func (s *BifrostHTTPServer) ExecuteTool(ctx context.Context, toolCall schemas.ChatAssistantMessageToolCall) (*schemas.ChatMessage, *schemas.BifrostError) { + logger.Debug("executing MCP tool: %s", *toolCall.Function.Name) return s.Client.ExecuteMCPTool(ctx, toolCall) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/features/governance/virtual-keys.mdx(1 hunks)plugins/governance/main.go(1 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/server/server.go(2 hunks)transports/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/bifrost-http/handlers/mcp_server.go
🧰 Additional context used
🧬 Code graph analysis (1)
transports/bifrost-http/server/server.go (3)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(483-489)core/schemas/bifrost.go (1)
BifrostError(354-363)transports/bifrost-http/handlers/mcp_server.go (1)
NewMCPServerHandler(37-51)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
docs/features/governance/virtual-keys.mdx (1)
567-567: LGTM! Error message update is more accurate.The updated error message "virtual key is missing in headers" is more accurate than the previous "x-bf-vk header is missing" since virtual keys can be provided via multiple headers (x-bf-vk, Authorization, or x-api-key as documented in lines 11-14). This change aligns with the code implementation.
plugins/governance/main.go (1)
345-345: LGTM! Consistent error message update.The error message update aligns with the documentation change and is more accurate since virtual keys can be provided via multiple header formats (x-bf-vk, Authorization, or x-api-key).
transports/bifrost-http/server/server.go (1)
907-915: LGTM! MCP server handler properly wired.The MCP server handler is correctly instantiated and registered:
- Line 907:
NewMCPServerHandlerreceives the Bifrost client, config, and server (which implementsMCPToolExecutorvia theExecuteToolmethod)- Line 915: Routes are registered with the appropriate middlewares, following the same pattern as other handlers
This implementation properly integrates the MCP server functionality into the HTTP transport layer.
transports/go.mod (1)
11-11: Version v0.41.1 is valid with no known security advisories.The verification confirms that
github.com/mark3labs/mcp-gov0.41.1 exists as a valid release on GitHub, and there are no associated security advisories in the GitHub Advisory Database. The dependency upgrade is safe to proceed.
Pratham-Mishra04
force-pushed
the
11-18-feat_mcp_agent_and_codemode_added
branch
from
3bfdb5c to
3067909
Compare
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
from
87e4214 to
6874f23
Compare
Pratham-Mishra04
force-pushed
the
11-18-feat_mcp_agent_and_codemode_added
branch
from
baddea8 to
a038622
Compare
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
2 times, most recently
from
84b7467 to
1d71ee5
Compare
Pratham-Mishra04
force-pushed
the
11-18-feat_mcp_agent_and_codemode_added
branch
2 times, most recently
from
c8c4690 to
c6122b9
Compare
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
2 times, most recently
from
95e23dd to
e101275
Compare
Pratham-Mishra04
force-pushed
the
11-18-feat_mcp_agent_and_codemode_added
branch
from
c6122b9 to
6f5f059
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
transports/bifrost-http/integrations/anthropic.go (1)
176-192: Consider extracting magic strings to named constants.The string literals
"claude-cli"and"integration-user-agent"are used directly. This was flagged in a previous review. Consider extracting these to named constants for better maintainability.const ( IntegrationUserAgentKey = "integration-user-agent" ClaudeCLIUserAgent = "claude-cli" )Then update the code:
- if strings.Contains(userAgent[0], "claude-cli") { - *bifrostCtx = context.WithValue(*bifrostCtx, schemas.BifrostContextKey("integration-user-agent"), "claude-cli") + if strings.Contains(userAgent[0], ClaudeCLIUserAgent) { + *bifrostCtx = context.WithValue(*bifrostCtx, schemas.BifrostContextKey(IntegrationUserAgentKey), ClaudeCLIUserAgent) }transports/bifrost-http/server/server.go (1)
447-477: SyncAllMCPServers errors after MCP client mutations are still swallowed.After
AddMCPClient,EditMCPClient, andRemoveMCPClient, failures ins.MCPServerHandler.SyncAllMCPServers(ctx)are only logged as warnings and not propagated. That leaves the system in a “config updated but MCP servers stale” state while callers see success, which can be hard to detect and debug if sync is persistently broken.Consider either:
- Propagating the sync error (so the whole operation is effectively transactional), or
- Exposing MCP sync health via a status/health endpoint or metric so operators can detect degraded sync state programmatically.
This mirrors the concern raised in earlier review iterations and remains unresolved in the current code.
🧹 Nitpick comments (3)
transports/bifrost-http/handlers/mcp_server.go (3)
78-110: JSON-RPC handler is straightforward; consider using structured logging instead of fmt.Sprintf.
handleMCPServercorrectly:
- Selects the appropriate MCP server via
getMCPServerForRequest,- Builds a Bifrost context via
ConvertToBifrostContext,- Delegates to
mcpServer.HandleMessage, and- Handles notification vs response semantics.
The only nit is
logger.Warn(fmt.Sprintf("Failed to marshal MCP response: %v", err)), which double-formats; usinglogger.Warn("Failed to marshal MCP response: %v", err)would be more consistent with the rest of the codebase.
112-148: SSE handler provides minimal MCP access control and a long-lived stream; verify that this matches client expectations.The SSE handler:
- Reuses
getMCPServerForRequestto enforce VK presence/enforcement,- Sets standard SSE headers,
- Reuses
ConvertToBifrostContext, and- Keeps the connection open until the request context is canceled, sending an initial
connection/openedevent.It currently does not integrate with
server.MCPServernotifications directly, so this acts as a keep-alive + initial handshake rather than a full MCP-go SSE transport. If MCP clients you target expect full MCP SSE semantics (e.g., viaserver.SSEServer), you may want to confirm this minimal behavior is sufficient and extend it later if needed.
206-300: Tool registration bridge into Bifrost looks correct; result extraction is text-only by design.
syncServer:
- Clears existing tools via
ListTools+DeleteTools,- Registers only function-type tools,
- Wraps each tool in a handler that:
- Marshals MCP arguments to JSON,
- Builds a
ChatAssistantMessageToolCall(type/function/arguments),- Invokes
toolManager.ExecuteTool,- Aggregates any text from
toolMessage.Content(string or text blocks) into a single result string, and- Returns
mcp.NewToolResultText.This is a sensible MVP bridge. Just be aware it currently:
- Ignores non-text content blocks (images, files, etc.), and
- Exposes only a single concatenated text string to MCP clients.
If you later support richer MCP results, this would be the main place to extend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/bifrost.go(2 hunks)core/mcp/mcp.go(2 hunks)core/mcp/toolmanager.go(7 hunks)core/mcp/utils.go(0 hunks)docs/features/governance/virtual-keys.mdx(1 hunks)plugins/governance/main.go(1 hunks)tests/core-mcp/integration_test.go(1 hunks)transports/bifrost-http/handlers/inference.go(0 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/integrations/anthropic.go(1 hunks)transports/bifrost-http/server/server.go(5 hunks)transports/go.mod(1 hunks)ui/app/workspace/mcp-gateway/views/mcpClientsTable.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- core/mcp/utils.go
- transports/bifrost-http/handlers/inference.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/mcp/mcp.go
- transports/go.mod
- ui/app/workspace/mcp-gateway/views/mcpClientsTable.tsx
- docs/features/governance/virtual-keys.mdx
🧰 Additional context used
🧬 Code graph analysis (5)
tests/core-mcp/integration_test.go (1)
core/providers/gemini/types.go (1)
Content(922-930)
transports/bifrost-http/integrations/anthropic.go (1)
core/schemas/bifrost.go (1)
BifrostContextKey(101-101)
core/bifrost.go (1)
core/schemas/chatcompletions.go (1)
ChatTool(201-205)
transports/bifrost-http/server/server.go (3)
transports/bifrost-http/handlers/mcp_server.go (2)
MCPServerHandler(33-39)NewMCPServerHandler(42-69)core/schemas/mcp.go (1)
MCPClientConfig(36-58)framework/configstore/store.go (1)
ConfigStore(17-147)
transports/bifrost-http/handlers/mcp_server.go (3)
plugins/governance/main.go (2)
Config(32-34)VirtualKeyPrefix(28-28)transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(71-269)core/schemas/bifrost.go (2)
BifrostContextKey(101-101)BifrostContextKeyVirtualKey(105-105)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (17)
plugins/governance/main.go (1)
345-346: LGTM!The updated error message is clearer and more user-friendly. It aligns with the documentation changes mentioned in the PR summary.
core/bifrost.go (2)
1219-1228: LGTM!The
GetAvailableMCPToolsmethod is well-implemented with proper nil-safety for themcpManager. It correctly returnsnilwhen MCP is not configured and delegates to the manager otherwise.
2093-2094: LGTM!The debug log for backoff duration improves observability during retry operations. This is helpful for troubleshooting retry behavior.
core/mcp/toolmanager.go (4)
77-125: LGTM!The
GetAvailableToolsmethod is well-implemented:
- Properly deduplicates tools using
seenToolNamesmap- Correctly handles code-mode clients by adding code-mode tools only when
includeCodeModeToolsis true- Validates tool.Function and tool.Function.Name before adding
127-184: LGTM!The
buildIntegrationDuplicateCheckMapfunction correctly handles integration-specific naming patterns:
- Claude CLI pattern extraction (
mcp__{foreign}__{tool_name}) is properly implemented- The empty string check at line 169 (
if toolName != "") addresses the previous review feedback- The switch statement is extensible for future integrations
186-215: LGTM!The context value extraction for
integration-user-agentis safely handled with proper type assertion and nil checks.
216-296: LGTM!The refactored
ParseAndAddToolsToRequestnow uses integration-aware duplicate checking for both Chat and Responses request types. The unified approach throughbuildIntegrationDuplicateCheckMapimproves code maintainability.tests/core-mcp/integration_test.go (1)
38-41: LGTM!The added nil check for
result.Contentat line 38 properly guards against nil pointer dereference. Combined with the existingContentStrcheck at line 39, this follows the same defensive pattern used in Step 1 (lines 25-26).transports/bifrost-http/server/server.go (4)
97-105: MCPServerHandler field addition and lifecycle look consistent.Storing
MCPServerHandleronBifrostHTTPServerand wiring it duringRegisterAPIRoutes(after config + client init, before serving) is coherent with how other handlers are managed. No issues with this addition.
480-487: Tool manager proxy methods are straightforward; just ensureClientis always initialized first.
ExecuteToolandGetAvailableMCPToolsare thin proxies tos.Clientand match theMCPToolManagerinterface used byMCPServerHandler. Given the current bootstrap flow initializess.ClientbeforeRegisterAPIRoutes, this is fine; just avoid calling these beforeBootstrapcompletes.
489-562: VK reload/remove hooks into MCP server handler are reasonable best-effort sync.Calling
s.MCPServerHandler.SyncVKMCPServer(preloadedVk)on reload andDeleteVKMCPServer(preloadedVk.Value)on delete keeps VK-scoped MCP servers aligned with governance changes. Since both operations are wrapped in the handler’s own mutex and tolerate missing map entries, this approach looks correct for keeping per-VK MCP state in sync.
905-955: MCPServerHandler construction and route registration integrate cleanly into existing bootstrap.Creating
mcpServerHandlerwithNewMCPServerHandler(ctx, s.Config, s)(using the HTTP server asMCPToolManager), failing fast on initialization error, assigning tos.MCPServerHandler, and registering/mcproutes alongside existing API handlers is a clean integration point. This ensures MCP HTTP endpoints are only exposed when initial tool sync succeeds.transports/bifrost-http/handlers/mcp_server.go (5)
25-69: MCPServerHandler construction and initial sync look solid.The
MCPToolManagerabstraction plusMCPServerHandlerstruct (global server + per-VK servers undersync.RWMutex) is a clean separation.NewMCPServerHandlervalidates inputs, creates the global MCP server with tool capabilities enabled, initializesvkMCPServers, and performs an initialSyncAllMCPServers—failing fast on sync errors, which is appropriate at startup.
152-205: Global and VK-scoped MCP server sync logic and locking look correct.
SyncAllMCPServers,SyncVKMCPServer, andDeleteVKMCPServer:
- Properly guard
vkMCPServerswithh.mu(write) and rebuild or update entries under lock.- Keep the global server in sync with all available tools, and VK servers in sync with VK-filtered tool sets.
- Safely handle the “server missing” case in
SyncVKMCPServerby creating and registering a new per-VK server, making new VKs usable without a full resync.This aligns well with the callbacks from
BifrostHTTPServeron MCP client and VK changes.
302-336: VK-specific tool filtering via context key is consistent with MCPConfig semantics.
fetchToolsForVK:
- Builds
executeOnlyToolsbased onvk.MCPConfigs:
- Skips clients with no
ToolsToExecute,- Treats
"*"as a client-wide wildcard (clientName/*),- Otherwise builds
clientName/toolNameentries.- Sets
schemas.BifrostContextKey("mcp-include-tools")on the context—even when the slice is empty—so the core MCP manager can filter tools accordingly.- Falls back to global tool list (no filtering) when
vk.MCPConfigsis empty, which matches the idea of “no extra restrictions” for that VK.This matches the documented behavior of the MCP manager and keeps VK tool visibility centralized in one place.
340-367: VK enforcement and server selection logic is clear and matches governance intent.
getMCPServerForRequest:
- Uses
config.Mu.RLockto readEnforceGovernanceHeader,- Uses
getVKFromRequestto extract a VK from supported headers,- Returns the global server when VK enforcement is off and no VK is supplied,
- Requires a VK (and returns an error) when enforcement is on or a VK is expected, and
- Looks up a matching per-VK server in
vkMCPServers, returning a clear “virtual key not found” error when missing.This cleanly separates global vs VK-scoped access and respects the governance flag.
369-391: VK extraction from headers is comprehensive and aligned with governance prefix rules.
getVKFromRequest:
- Checks
x-bf-vkfirst,- Then
Authorization: Bearer sk-bf-*(usinggovernance.VirtualKeyPrefix), and- Finally
x-api-keywith the same VK prefix.This mirrors how VKs are used elsewhere in Bifrost (and in the governance plugin), and supports all three header variants that clients may send.
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
2 times, most recently
from
68f09b7 to
d30682a
Compare
Pratham-Mishra04
force-pushed
the
11-18-feat_mcp_agent_and_codemode_added
branch
from
6f5f059 to
602f37c
Compare
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
from
d30682a to
f3c86fc
Compare
Pratham-Mishra04
force-pushed
the
11-18-feat_mcp_agent_and_codemode_added
branch
from
602f37c to
1d386e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
transports/bifrost-http/integrations/anthropic.go (1)
188-189: Consider extracting magic strings to constants.The string literals
"claude-cli"and"integration-user-agent"are used directly. Extracting these to named constants would improve maintainability and make it easier to update these values in the future.Example refactor (apply at the package or file level):
const ( IntegrationUserAgentKey = "integration-user-agent" ClaudeCLIUserAgent = "claude-cli" )Then update lines 188-189:
- if strings.Contains(userAgent[0], "claude-cli") { - *bifrostCtx = context.WithValue(*bifrostCtx, schemas.BifrostContextKey("integration-user-agent"), "claude-cli") + if strings.Contains(userAgent[0], ClaudeCLIUserAgent) { + *bifrostCtx = context.WithValue(*bifrostCtx, schemas.BifrostContextKey(IntegrationUserAgentKey), ClaudeCLIUserAgent)transports/bifrost-http/server/server.go (1)
100-105: MCP server wiring through BifrostHTTPServer is coherent and lifecycle-safe.
- Adding
MCPServerHandleron the server struct and constructing it inRegisterAPIRoutesviaNewMCPServerHandler(with fail‑fast error propagation) ensures MCP servers are fully synced before the HTTP server starts serving.ExecuteTool/GetAvailableMCPToolssimply proxy to the core client’s MCP APIs, which is the right separation of concerns.- The MCP client mutation callbacks (
AddMCPClient,EditMCPClient,RemoveMCPClient) update config vias.Configand then callSyncAllMCPServers, so per‑VK and global MCP servers stay in sync with MCP client config; logging sync failures as warnings is consistent with the existing decision not to make sync transactional.ReloadVirtualKeyandRemoveVirtualKeynow also callSyncVKMCPServer/DeleteVKMCPServer, tying VK lifecycle to VK‑scoped MCP servers, which keeps per‑VK tool views aligned with governance state as long as these callbacks are invoked through the normal Bootstrap/route setup flow whereMCPServerHandleris non‑nil.Given prior discussion on whether to propagate
SyncAllMCPServerserrors to callers, the current “log‑and‑proceed” behavior is acceptable for a non‑breaking, best‑effort sync and doesn’t need to change unless you want stronger transactional guarantees.Also applies to: 447-487, 489-562, 942-955
transports/bifrost-http/handlers/mcp_server.go (1)
78-110: Request routing, context handling, and VK selection are consistent with governance expectations.
handleMCPServerresolves the appropriate MCP server viagetMCPServerForRequest, converts the fasthttp context once withConvertToBifrostContext, and delegates JSON‑RPC handling tomcpServer.HandleMessage, returning either 200 with a body or 200/no body for notifications—this is a clean, minimal JSON‑RPC bridge.handleMCPServerSSEnow keeps the converted context alive for the lifetime of the SSE stream by cancelling it in the stream writer’sdefer, which avoids the earlier premature‑cancel issue while ensuring cleanup when the connection closes.getMCPServerForRequestuses the config’sEnforceGovernanceHeaderflag andgetVKFromRequestto decide between the global server and a VK‑scoped server, returning clear errors when a VK is required or unknown; the VK extraction logic correctly honorsx-bf-vk,Authorization: Bearer sk-bf-*, andx-api-keywith the governance virtual key prefix.If you want to make client-facing errors more self‑service friendly, you could expand the “virtual key header is required”/“virtual key not found” messages to mention the accepted header formats (as suggested in an earlier review), but that’s purely UX polish.
Also applies to: 112-148, 340-367
🧹 Nitpick comments (4)
plugins/logging/main.go (1)
166-172: UTC cutoff and debug logging look correct; minor style nit on duration expressionUsing
time.Now().UTC()for the 30‑minute cutoff aligns with UTC log timestamps and avoids timezone skew; the added debug log will help when inspecting cleanup behavior.As a tiny readability tweak, you might prefer:
thirtyMinutesAgo := time.Now().UTC().Add(-30 * time.Minute)instead of
-1 * 30 * time.Minute, which is equivalent but a bit less idiomatic.core/bifrost.go (1)
1219-1228: GetAvailableMCPTools implementation is straightforward; consider empty slice vs nil.The delegation to
mcpManager.GetAvailableToolswith a nil guard is correct. If you want to simplify callers and JSON marshalling, you might return an empty slice instead ofnil, but the current behavior is functionally fine.core/mcp/toolmanager.go (2)
77-125: Tool flattening and integration-aware duplicate map are sound.
GetAvailableToolscorrectly flattens tools across clients, tracks seen names, and gates code‑mode tools behindIsCodeModeClient, so you only inject the synthetic code‑mode tools once.buildIntegrationDuplicateCheckMap’s handling of direct names plusclaude-cli’smcp__*__{tool}pattern (including the empty‑name guard) cleanly prevents duplicates in both native and Claude CLI representations.If you expect more integrations, you may want to normalize
integrationUserAgent(e.g., to lowercase) or use prefix matching to make the switch a bit more future‑proof, but the current exact match is fine for a single known agent.Also applies to: 127-184
195-215: ParseAndAddToolsToRequest correctly reuses the integration-aware dedupe for chat and responses.For chat, you preserve existing
Params, build an integration‑awareduplicateCheckMapfrom currentChatParameters.Tools, and only append MCP tools whose function names aren’t already present (includingclaude-clipatterns via the helper), updating the map as you go—this prevents both native and integration‑specific duplicates. For responses, converting existing tools intoChatToolform purely for name-based dedupe is a clean way to reuse the same logic before appendingToResponsesTool()variants.You might consider hoisting the
"integration-user-agent"context key into a namedschemas.BifrostContextKeyconstant to reduce the risk of key mismatches between producers and this consumer.Also applies to: 227-243, 254-269, 277-290
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/bifrost.go(2 hunks)core/mcp/mcp.go(2 hunks)core/mcp/toolmanager.go(7 hunks)core/mcp/utils.go(0 hunks)docs/features/governance/virtual-keys.mdx(1 hunks)framework/logstore/rdb.go(2 hunks)plugins/governance/main.go(1 hunks)plugins/logging/main.go(2 hunks)transports/bifrost-http/handlers/inference.go(0 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/integrations/anthropic.go(1 hunks)transports/bifrost-http/server/server.go(5 hunks)transports/go.mod(1 hunks)ui/app/workspace/mcp-gateway/views/mcpClientSheet.tsx(1 hunks)ui/app/workspace/mcp-gateway/views/mcpClientsTable.tsx(1 hunks)ui/components/ui/sheet.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- transports/bifrost-http/handlers/inference.go
- core/mcp/utils.go
🚧 Files skipped from review as they are similar to previous changes (4)
- ui/app/workspace/mcp-gateway/views/mcpClientsTable.tsx
- core/mcp/mcp.go
- docs/features/governance/virtual-keys.mdx
- transports/go.mod
🧰 Additional context used
🧬 Code graph analysis (4)
transports/bifrost-http/server/server.go (3)
transports/bifrost-http/handlers/mcp_server.go (2)
MCPServerHandler(33-39)NewMCPServerHandler(42-69)core/schemas/mcp.go (1)
MCPClientConfig(36-58)framework/configstore/store.go (1)
ConfigStore(17-147)
transports/bifrost-http/integrations/anthropic.go (1)
core/schemas/bifrost.go (1)
BifrostContextKey(101-101)
framework/logstore/rdb.go (1)
framework/logstore/tables.go (2)
Log(74-136)Log(139-141)
core/bifrost.go (1)
core/schemas/chatcompletions.go (1)
ChatTool(201-205)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (13)
plugins/logging/main.go (1)
289-309: No behavioral change in callback block; structure remains soundThe brace/formatting adjustment around the
logCallbackblock inPreHookdoes not alter behavior; the locking pattern and callback invocation remain correct and thread‑safe.framework/logstore/rdb.go (2)
278-278: LGTM - Improved query clarity and semantic correctness.The explicit
Model(&Log{})call improves query clarity, and usingtimestampinstead ofcreated_atis semantically more correct for log retention—it filters based on when the log event occurred rather than when it was persisted to the database.
304-304: LGTM - Consistent with Flush method.The change to use
timestampfor batch deletion is consistent with theFlushmethod modification and correctly filters logs based on event time rather than insertion time.transports/bifrost-http/integrations/anthropic.go (1)
176-192: Good refactoring: headers extracted once and User-Agent detection implemented correctly.The unconditional header extraction at line 176 eliminates redundant calls and improves code clarity. The User-Agent detection logic properly handles case-insensitive lookup and includes defensive checks to prevent index errors. The reuse of the extracted headers at line 201 confirms this is a solid improvement.
ui/app/workspace/mcp-gateway/views/mcpClientSheet.tsx (1)
186-187: LGTM: SheetHeader configuration aligns with new API.The addition of
w-fullensures proper full-width layout, andshowCloseButton={false}appropriately hides the redundant close button since the "Save Changes" button serves as the primary header action. Users can still dismiss via overlay click.ui/components/ui/sheet.tsx (2)
75-79: LGTM: Visual refinements improve sheet presentation.The CSS updates introduce tasteful spacing (
top-2 bottom-2), rounded corners, and content-driven height (h-auto), enhancing the sheet's visual polish. The conditionalmax-wlogic correctly applies based on side and expansion state.
94-121: LGTM: Well-designed API extension for close button control.The
showCloseButtonprop is properly typed, backward-compatible (defaults totrue), and the conditional rendering is clean. The simplified arrow icon logic on line 110 reduces redundancy. Accessibility is preserved withsr-onlylabels.plugins/governance/main.go (1)
339-347: VK-missing error message change is safe and consistent.The new
"virtual key is missing in headers"message preserves behavior and matches the VK terminology used in the HTTP/MCP layer; no further changes needed here.core/bifrost.go (2)
1247-1262: Lazy MCP manager initialization in AddMCPClient looks correct.Using
mcpInitOnceto create the manager on firstAddMCPClientand then delegating toAddClientis thread-safe and compatible with the eager-init path inInit; the extra nil check afterDois a good defensive guard.
2092-2095: Extra retry backoff logging improves observability without behavior change.Logging
backoffat debug level beforetime.Sleepgives clear visibility into retry timing and doesn’t affect control flow or correctness.transports/bifrost-http/handlers/mcp_server.go (3)
41-69: MCPServerHandler construction and sync logic are well-structured and concurrency-safe.The constructor validates dependencies, creates a global MCP server, initializes
vkMCPServersand then eagerly callsSyncAllMCPServers, which both wires the global server and builds per‑VK servers from the config store. All accesses tovkMCPServersare protected bymu(write inSyncAllMCPServers/SyncVKMCPServer/DeleteVKMCPServer, read ingetMCPServerForRequest), and using per‑VKserver.NewMCPServerinstances withfetchToolsForVKkeeps VK scoping and tool filtering nicely encapsulated.Also applies to: 152-199
206-300: Tool registration and execution bridge cleanly from MCP to Bifrost tools.
syncServerfully clears the MCP server’s existing tools before re‑registering, then for eachChatToolwith a non‑nilFunction:
- Captures
toolNameonce per iteration and builds aCallToolRequesthandler that:
- Marshals arguments safely (returning an MCP error result on marshal failure).
- Constructs a
ChatAssistantMessageToolCalland delegates totoolManager.ExecuteTool.- Extracts text content from the returned
ChatMessage(string or text blocks) and returns anmcp.NewToolResultText.- Converts the Bifrost JSON schema into
mcp.ToolInputSchema, defaulting to an empty object when no parameters are defined, and registers the tool on the MCP server.This keeps MCP‑side registration aligned with Bifrost’s tool schema and avoids stale registrations between syncs.
302-336: VK-scoped tool filtering and VK extraction are aligned with governance config.
fetchToolsForVKbuilds amcp-include-toolscontext value fromvk.MCPConfigs, supporting both wildcard (*) and per‑tool lists per client, and delegates toGetAvailableMCPToolswith that context so underlying tool discovery can enforce per‑VK tool allowlists. ThegetVKFromRequesthelper properly checksx-bf-vk, bearer tokens, andx-api-keywith the sharedgovernance.VirtualKeyPrefix, keeping header parsing consistent across governance and MCP layers.Also applies to: 369-391
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ui/components/ui/sheet.tsx (1)
103-103: Simplify the conditional to avoid duplicate classes.The base className includes
mb-6, and the ternary addsmb-6again whenexpandableis false, resulting in"mb-6 flex items-center gap-3 mb-6". While this won't cause errors, it's redundant.Apply this diff to clean up the duplication:
- <div data-slot="sheet-header" className={cn("mb-6 flex items-center gap-3", sheetContext?.expandable ? "p-4" : "mb-6")} {...props}> + <div data-slot="sheet-header" className={cn("flex items-center gap-3", sheetContext?.expandable ? "p-4" : "mb-6")} {...props}>transports/bifrost-http/handlers/mcp_server.go (1)
181-198: Optional: Remove redundant map assignment.Line 196 reassigns
vkServertoh.vkMCPServers[vk.Value]unconditionally, but:
- When
!ok(line 185), the server is already assigned at line 192- When
ok(line 184), the server is already in the mapThe second assignment is redundant when the server already exists. Consider:
func (h *MCPServerHandler) SyncVKMCPServer(vk *tables.TableVirtualKey) { h.mu.Lock() defer h.mu.Unlock() vkServer, ok := h.vkMCPServers[vk.Value] if !ok { // Add new server vkServer = server.NewMCPServer( vk.Name, version, server.WithToolCapabilities(true), ) h.vkMCPServers[vk.Value] = vkServer } availableTools := h.fetchToolsForVK(vk) h.syncServer(vkServer, availableTools) - h.vkMCPServers[vk.Value] = vkServer logger.Debug("Synced MCP server for virtual key '%s' with %d tools", vk.Name, len(availableTools)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/bifrost.go(2 hunks)core/mcp/mcp.go(2 hunks)core/mcp/toolmanager.go(7 hunks)core/mcp/utils.go(0 hunks)docs/features/governance/virtual-keys.mdx(1 hunks)framework/logstore/rdb.go(2 hunks)plugins/governance/main.go(1 hunks)plugins/logging/main.go(2 hunks)transports/bifrost-http/handlers/inference.go(0 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/integrations/anthropic.go(1 hunks)transports/bifrost-http/server/server.go(5 hunks)transports/go.mod(1 hunks)ui/app/workspace/mcp-gateway/views/mcpClientSheet.tsx(1 hunks)ui/app/workspace/mcp-gateway/views/mcpClientsTable.tsx(1 hunks)ui/components/ui/sheet.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- transports/bifrost-http/handlers/inference.go
- core/mcp/utils.go
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/app/workspace/mcp-gateway/views/mcpClientsTable.tsx
- transports/bifrost-http/integrations/anthropic.go
- transports/go.mod
- core/bifrost.go
- docs/features/governance/virtual-keys.mdx
- core/mcp/mcp.go
- plugins/governance/main.go
🧰 Additional context used
🧬 Code graph analysis (3)
framework/logstore/rdb.go (2)
core/schemas/models.go (1)
Model(109-129)framework/logstore/tables.go (2)
Log(74-136)Log(139-141)
ui/app/workspace/mcp-gateway/views/mcpClientSheet.tsx (1)
ui/components/ui/sheet.tsx (1)
SheetHeader(137-137)
transports/bifrost-http/server/server.go (2)
core/schemas/mcp.go (1)
MCPClientConfig(36-58)framework/configstore/store.go (1)
ConfigStore(17-147)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (18)
ui/components/ui/sheet.tsx (5)
75-75: LGTM!The CSS class reordering for slide animations and positioning maintains the same functionality while improving code organization.
Also applies to: 79-79
94-99: LGTM!The
showCloseButtonprop is a clean, backward-compatible API addition with a sensible default.
108-111: LGTM!The button styling updates improve the visual behavior with appropriate hover effects and sizing.
114-114: LGTM!The
min-w-0class is a good addition to support proper text truncation within the flex container.
115-120: LGTM!The conditional rendering correctly implements the
showCloseButtonfeature while maintaining accessibility with the screen reader text.ui/app/workspace/mcp-gateway/views/mcpClientSheet.tsx (1)
186-199: LGTM!The use of
showCloseButton={false}is appropriate here since the sheet uses a custom "Save Changes" button positioned within the header. Thew-fullclasses ensure proper full-width layout alignment.framework/logstore/rdb.go (2)
276-283: Timestamp-based flush logic looks consistentSwitching the cleanup condition to
timestamp < ?(instead ofcreated_at) for processing logs aligns this path with the rest of the code that filters and orders bytimestamp. GivenTimestampis indexed and non-null, this should behave correctly and efficiently for time-based cleanup.
297-305: Batch deletion usingtimestampmatches retention semanticsUsing
timestamp < ?as the cutoff inDeleteLogsBatchmakes the retention logic consistent with search filters and ordering that already rely ontimestamp, and it benefits from the existing index on that column. The batched ID pluck + delete flow remains correct.plugins/logging/main.go (1)
166-168: LGTM: UTC alignment improves cleanup consistency.Using
time.Now().UTC()ensures the cleanup threshold matches the UTC timestamps stored in log entries. The debug log provides useful visibility into the cleanup process.transports/bifrost-http/server/server.go (1)
447-478: LGTM: MCP server integration is well-structured.The wiring between MCP client/VK mutations and the MCPServerHandler is consistent and complete:
- Client changes trigger
SyncAllMCPServers- VK changes trigger
SyncVKMCPServerorDeleteVKMCPServer- Tool execution and discovery are exposed via
ExecuteToolandGetAvailableMCPTools- Handler initialization and route registration follow the established pattern
The sync error handling (logging warnings rather than failing) aligns with the maintainer's stated approach from previous review discussions.
Also applies to: 480-487, 518-518, 560-560, 942-946, 954-954
core/mcp/toolmanager.go (3)
77-125: LGTM: Tool aggregation and deduplication logic is sound.
GetAvailableToolscorrectly:
- Collects tools from all clients
- Handles code-mode tools when any client is flagged as code-mode
- Deduplicates using a
seenToolNamesmap to prevent duplicate tool registrations
127-184: LGTM: Integration-aware deduplication handles Claude CLI patterns correctly.The function properly:
- Extracts both direct tool names and integration-specific patterns
- Handles Claude CLI's
mcp__{foreign}__{tool}format by extracting the final tool name- Includes empty string validation (line 169) to prevent invalid map entries
- Provides extensibility for additional integration patterns
186-296: LGTM: Integration-aware tool addition prevents duplicates across request types.The updated logic:
- Retrieves integration user-agent from context for duplicate checking
- Applies
buildIntegrationDuplicateCheckMapconsistently for both Chat and Responses requests- Properly converts Responses tools to ChatTool format for unified duplicate detection
- Updates the duplicate check map as tools are added to prevent intra-MCP duplicates
transports/bifrost-http/handlers/mcp_server.go (5)
41-76: LGTM: Handler initialization and route registration are well-structured.The constructor properly:
- Validates required dependencies (config, toolManager)
- Initializes global and per-VK MCP servers
- Performs initial synchronization to populate tools
- Returns an error if sync fails, ensuring consistent startup state
Route registration follows the framework pattern with middleware chaining.
79-148: LGTM: Request handlers implement MCP protocol correctly.The JSON-RPC handler properly:
- Retrieves the appropriate MCP server (global or VK-specific)
- Uses mcp-go's
HandleMessagefor protocol compliance- Handles notification responses (nil response)
The SSE handler correctly:
- Defers context cancellation to the writer closure (lines 130-133), ensuring the stream stays alive
- Sends initial connection message
- Waits for context cancellation before closing
152-179: LGTM: Full MCP server synchronization is comprehensive.The method properly:
- Acquires write lock for safe map manipulation
- Syncs global server with all available tools
- Reinitializes VK servers from ConfigStore
- Fetches VK-specific tools using
fetchToolsForVK- Logs sync status for observability
206-336: LGTM: Tool synchronization and VK-specific filtering are correctly implemented.
syncServerproperly:
- Clears existing tools before re-registration
- Handles JSON marshaling errors (lines 227-230)
- Converts Bifrost tool calls to MCP tool results
- Registers tools with proper schema conversion
fetchToolsForVKcorrectly:
- Handles wildcards per MCP client (line 317-319)
- Constructs client-specific tool patterns (line 326)
- Passes include-only filters via context for downstream filtering
340-391: LGTM: MCP server selection and VK extraction are correctly implemented.
getMCPServerForRequestproperly:
- Uses read lock for safe concurrent access to VK servers map
- Checks enforcement flag under config read lock
- Returns global server when VK is not enforced and not provided
- Returns appropriate errors for missing or invalid VKs
getVKFromRequestcorrectly extracts VKs from multiple header sources:
x-bf-vkheaderAuthorization: Bearer sk-bf-*x-api-key: sk-bf-*
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
framework/logstore/rdb.go (1)
304-304: Same timestamp vs created_at behavioral change as Flush method.This change mirrors the modification in the
Flushmethod at Line 278. The same considerations apply: while technically more correct, ensure this doesn't inadvertently affect retention behavior if write lag exists.transports/bifrost-http/server/server.go (1)
100-105: MCP client CRUD and tool APIs are wired correctly through the HTTP serverThe additions:
- Persist MCP client changes via
s.Config.*MCPClientthen calls.MCPServerHandler.SyncAllMCPServers(ctx), and- Expose
ExecuteTool/GetAvailableMCPToolsas thin proxies tos.Client,are consistent with the rest of the server’s design and keep transport logic cleanly separated from core MCP/tool logic.
You currently log and ignore sync failures after MCP client mutations. If you later need stronger guarantees, consider either surfacing these errors to callers or exposing sync health via a status/metrics path so a degraded MCP state is observable without trawling logs.
Also applies to: 447-487
🧹 Nitpick comments (11)
plugins/logging/main.go (1)
166-168: LGTM! UTC-aware cleanup improves consistency.The explicit UTC timestamp calculation and debug logging enhance observability. The logic is correct.
Optionally, simplify the arithmetic expression for brevity:
- thirtyMinutesAgo := time.Now().UTC().Add(-1 * 30 * time.Minute) + thirtyMinutesAgo := time.Now().UTC().Add(-30 * time.Minute)ui/components/ui/sheet.tsx (1)
94-121: ClarifySheetHeaderstyling surface and clean up minor class duplicationThe new
showCloseButtonprop and layout work well and are backward‑compatible by default. Two small points:
classNameis now applied only to the inner content wrapper (divwithflex h-full min-w-0 flex-1 flex-row items-center), not the outer header container. Callers that previously usedclassNameto control header padding/margins/background may see different behavior. If that wasn’t intentional, consider also mergingclassNameinto the outerdiv, or introducing a dedicatedcontentClassNamefor the inner row.- The outer
divbase classes include"mb-6"and the conditional also adds"mb-6"in the non‑expandable case, which is redundant and can be simplified.These are stylistic/semantic nits; functionally the header is solid.
core/mcp/mcp.go (1)
83-89: Client config loop guard is safe but not strictly necessaryThe
len(config.ClientConfigs) > 0check is redundant since ranging over a nil slice is already safe in Go, but it doesn’t harm correctness and keeps the intent explicit.core/mcp/toolmanager.go (3)
77-125: Tool flattening and code‑mode inclusion logic looks sound
GetAvailableToolscorrectly:
- Flattens
GetToolPerClientinto a single slice.- Deduplicates by
Function.Name.- Tracks whether any client is
IsCodeModeClientand then adds code‑mode tools once, also deduped by name.Given
GetToolPerClientis already context‑aware, this is a good central place to expose a filtered, deduped view for other components.If you want to micro‑simplify, the outer
if len(availableTools) > 0check inParseAndAddToolsToRequestbecomes redundant now that you early‑return when the slice is empty.
127-184: Keep helper comments in sync with function signatureThe behavior of
buildIntegrationDuplicateCheckMapfor Claude CLI patterns looks correct (including the new empty‑segment guard), but the doc comment still references anavailableToolsPerClientparameter that no longer exists.Consider updating the comment to only describe the actual parameters (
existingToolsandintegrationUserAgent) so future readers don’t look for a non‑existent argument.
195-295: Integration‑aware dedup inParseAndAddToolsToRequestis well‑structuredThe refactor to:
- Pull
availableToolsviaGetAvailableTools(ctx),- Seed
duplicateCheckMapwithbuildIntegrationDuplicateCheckMap(for both chat and responses),- Then add tools only when
duplicateCheckMap[toolName]is false and update the map as you append,gives a clear, unified dedup story across Chat and Responses paths, including Claude CLI patterns.
Minor polish idea: you could lift the repeated
toolName := mcpTool.Function.Name/if !duplicateCheckMap[toolName] { ... duplicateCheckMap[toolName] = true }pattern into a tiny helper to avoid drift between the chat and responses branches, but it’s not strictly necessary.transports/bifrost-http/server/server.go (1)
489-561: Virtual key reload/remove now correctly keep MCP servers in syncHooking:
ReloadVirtualKey→s.MCPServerHandler.SyncVKMCPServer(preloadedVk)andRemoveVirtualKey→s.MCPServerHandler.DeleteVKMCPServer(preloadedVk.Value)ensures per‑VK MCP servers track VK lifecycle without requiring a global resync, which is a good balance between correctness and cost.
If you ever see VK churn at high volume, it might be worth adding debug logs around these calls (with VK value) to make MCP‑related behavior during governance events easier to trace.
transports/bifrost-http/handlers/mcp_server.go (4)
25-76: MCP server handler construction and routing look robustThe
MCPToolManagerinterface is minimal and focused, andNewMCPServerHandler:
- Validates both
configandtoolManager,- Initializes
globalMCPServerandvkMCPServers,- Immediately calls
SyncAllMCPServersto populate tools,which gives strong initialization guarantees before
/mcproutes are registered viaRegisterRoutes.If you start supporting multiple MCP routes or versions later, consider parameterizing the base path (
/mcp) here rather than hard‑coding it inRegisterRoutes.
78-148: JSON‑RPC and SSE handlers are consistent and context‑aware
handleMCPServercorrectly:
- Resolves the appropriate MCP server via
getMCPServerForRequest,- Builds a Bifrost context via
ConvertToBifrostContext,- Delegates to
mcpServer.HandleMessageand handles notification vs response cases.
handleMCPServerSSEreuses the same VK gating, sets SSE headers, and uses a stream writer that:
- Sends an initial
connection/openedevent,- Waits on the Bifrost context’s
Done()before canceling and flushing.This matches how other streaming endpoints are structured in the codebase.
If you ever need richer SSE semantics (heartbeats, error events), you already have the writer abstraction in place to extend without changing the public API.
152-299: Per‑VK MCP server sync and dynamic tool registration are thread‑safe and well‑structured
SyncAllMCPServers:
- Uses
h.muto guardvkMCPServers,- Syncs the global server once,
- Rebuilds the VK map from
ConfigStore.GetVirtualKeysand syncs each VK server with filtered tools.
SyncVKMCPServerlazily creates a VK‑scoped server if missing and then syncs it withfetchToolsForVK, which matches how full sync behaves.
DeleteVKMCPServersafely removes VK entries under the same lock.
syncServer:
- Clears existing tools via
ListTools/DeleteTools,- Registers each available MCP tool with a handler that:
- Marshals
request.GetArguments()with proper error handling,- Adapts to
ChatAssistantMessageToolCall,- Executes via
toolManager.ExecuteTool,- Extracts string content from the resulting
ChatMessageinto an MCPToolResult.Closure capture of
toolNameand conversion of parameters intomcp.ToolInputSchemaboth look correct.The parameter conversion currently only maps
type,properties, andrequired. If your schema type grows extra fields (e.g. enums, default), consider extending this mapping so MCP clients see the full input schema rather than a lossy subset.
302-336: VK‑scoped tool filtering and VK resolution align with governance semantics
fetchToolsForVK:
- Builds
executeOnlyToolsasclient/*orclient/toolentries fromvk.MCPConfigs,- Sets
mcp-include-toolsin the context even when empty to intentionally mean “no tools allowed” for that VK,- Delegates to
GetAvailableMCPToolsso all downstream filtering and dedup logic is reused.
getMCPServerForRequest:
- Uses
ClientConfig.EnforceGovernanceHeaderunder lock,- Returns the global server when enforcement is off and no VK is provided,
- Otherwise requires a VK and resolves the VK‑scoped server, with clear error messages when missing.
getVKFromRequestcorrectly considers:
x-bf-vk,Authorization: Bearer <vk>,x-api-key,
all gated by the sharedgovernance.VirtualKeyPrefix.Overall this gives a clear story: governance config controls whether VK is mandatory, and when present, the VK drives which MCP server (and tools) are used.
The comments on
fetchToolsForVKstill mention avkValueparameter that no longer exists; updating them to talk about*tables.TableVirtualKeyand themcp-include-toolscontext key would reduce confusion for future readers.Also applies to: 340-367, 369-391
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/bifrost.go(2 hunks)core/mcp/mcp.go(2 hunks)core/mcp/toolmanager.go(7 hunks)core/mcp/utils.go(0 hunks)docs/features/governance/virtual-keys.mdx(1 hunks)framework/logstore/rdb.go(2 hunks)plugins/governance/main.go(1 hunks)plugins/logging/main.go(2 hunks)transports/bifrost-http/handlers/inference.go(0 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/integrations/anthropic.go(1 hunks)transports/bifrost-http/server/server.go(5 hunks)transports/go.mod(1 hunks)ui/app/workspace/mcp-gateway/views/mcpClientSheet.tsx(1 hunks)ui/app/workspace/mcp-gateway/views/mcpClientsTable.tsx(1 hunks)ui/components/ui/sheet.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- core/mcp/utils.go
- transports/bifrost-http/handlers/inference.go
🚧 Files skipped from review as they are similar to previous changes (6)
- plugins/governance/main.go
- transports/go.mod
- core/bifrost.go
- transports/bifrost-http/integrations/anthropic.go
- ui/app/workspace/mcp-gateway/views/mcpClientsTable.tsx
- docs/features/governance/virtual-keys.mdx
🧰 Additional context used
🧬 Code graph analysis (5)
framework/logstore/rdb.go (1)
framework/logstore/tables.go (2)
Log(74-136)Log(139-141)
core/mcp/mcp.go (2)
transports/bifrost-http/handlers/mcp.go (1)
MCPManager(20-24)core/schemas/chatcompletions.go (1)
ChatTool(201-205)
ui/app/workspace/mcp-gateway/views/mcpClientSheet.tsx (1)
ui/components/ui/sheet.tsx (1)
SheetHeader(137-137)
transports/bifrost-http/server/server.go (3)
transports/bifrost-http/handlers/mcp_server.go (2)
MCPServerHandler(33-39)NewMCPServerHandler(42-69)core/schemas/mcp.go (1)
MCPClientConfig(36-58)framework/configstore/store.go (1)
ConfigStore(17-147)
transports/bifrost-http/handlers/mcp_server.go (6)
core/schemas/chatcompletions.go (3)
ChatTool(201-205)ChatAssistantMessageToolCall(483-489)ChatAssistantMessageToolCallFunction(492-495)plugins/governance/main.go (2)
Config(32-34)VirtualKeyPrefix(28-28)transports/bifrost-http/lib/middleware.go (2)
BifrostHTTPMiddleware(7-7)ChainMiddlewares(12-24)transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(71-269)core/schemas/bifrost.go (2)
BifrostContextKey(101-101)BifrostContextKeyVirtualKey(105-105)framework/configstore/clientconfig.go (1)
ClientConfig(30-45)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
ui/app/workspace/mcp-gateway/views/mcpClientSheet.tsx (1)
186-199: Confirm UX around removing the header close buttonUsing
showCloseButton={false}plus a full‑width header looks fine technically, but it removes the explicit close affordance from this sheet. That leaves overlay click / Escape (or parent controls) as the only way to dismiss it. Please confirm that:
- There is a clear alternative “Cancel/Close” path exposed to users, and
- Relying on overlay/Escape close is intentional for this editing flow.
Otherwise the change is consistent with the new
SheetHeaderAPI.ui/components/ui/sheet.tsx (1)
73-82: SheetContent left/right layout changes look safeThe updated class names for the right/left sheet sides (padding, rounded corners, borders, and slide animations) are consistent and should not affect behavior beyond the intended visual tweaks. No issues from a correctness standpoint.
core/mcp/mcp.go (1)
108-110: PublicGetAvailableToolspassthrough looks correctExposing
GetAvailableToolsonMCPManageras a simple delegate totoolsHandleris clean and keeps the public surface aligned with the underlyingToolsManagerAPI.transports/bifrost-http/server/server.go (1)
940-955: MCPServerHandler initialization and route registration fit cleanly into bootstrapCreating the
MCPServerHandlerinRegisterAPIRoutes, assigning it tos.MCPServerHandler, and registering/mcproutes alongside existing API handlers keeps lifecycle and wiring localized. The constructor’s sync step means the handler is ready to serve before requests can hit it.
Pratham-Mishra04
force-pushed
the
11-18-feat_mcp_agent_and_codemode_added
branch
from
1d386e8 to
02f776d
Compare
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
from
f3c86fc to
9092333
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
framework/logstore/rdb.go (1)
299-307: Clarify retention semantics:DeleteLogsBatchnow deletes bytimestampinstead ofcreated_at.Switching the cutoff predicate to
timestamp < ?changes retention from “old DB records” to “old events”. That can be desirable, but it’s a behavioral change with a few implications:
- Any delayed/backfilled logs whose
timestampis already older thancutoffwill be eligible for deletion immediately on insert.Flushstill usescreated_atwhileDeleteLogsBatchusestimestamp, so cleanup paths now use different notions of “old”.- Prior migrations/comments reportedly reference
created_at‑based indexes; if the main retention query is now ontimestamp, those docs/indexes should be updated (and/or a matching index ontimestampensured) to avoid surprises and table scans.Please confirm this change is intentional, and either:
- keep
timestampbut align migrations/docs and indexing, or- revert to
created_atfor consistency with existing retention expectations.
🧹 Nitpick comments (5)
core/mcp/toolmanager.go (1)
216-216: Redundant length check.The
len(availableTools) > 0check at line 216 is redundant since the function already returns early at lines 203-205 ifavailableToolsis empty.- if len(availableTools) > 0 { - switch req.RequestType { + switch req.RequestType {And remove the closing brace at line 294 accordingly.
transports/bifrost-http/handlers/mcp_server.go (4)
114-114: Unused return value fromgetMCPServerForRequest.The
mcpServerreturn value is assigned but never used. This appears to be validation-only (checking VK exists), but the discarded value is misleading.- _, err := h.getMCPServerForRequest(ctx) + if _, err := h.getMCPServerForRequest(ctx); err != nil { + SendError(ctx, fasthttp.StatusUnauthorized, err.Error()) + return + }Or use a dedicated validation method if the server instance isn't needed.
196-196: Redundant map assignment.Line 196 assigns
vkServertoh.vkMCPServers[vk.Value]again, but this is already done at line 192 for new servers, and the server is already in the map (viah.vkMCPServers[vk.Value]at line 184) for existing ones.availableTools := h.fetchToolsForVK(vk) h.syncServer(vkServer, availableTools) - h.vkMCPServers[vk.Value] = vkServer logger.Debug("Synced MCP server for virtual key '%s' with %d tools", vk.Name, len(availableTools))
209-209: Minor: Unnecessary blank identifier in range.- for toolName, _ := range toolMap { + for toolName := range toolMap {
356-358: Error message inconsistency with governance plugin.The governance plugin now uses "virtual key is missing in headers" (see
plugins/governance/main.goline 345), but this handler uses "virtual key header is required to access MCP server." Consider aligning the messaging for consistency.if vk == "" { - return nil, fmt.Errorf("virtual key header is required to access MCP server.") + return nil, fmt.Errorf("virtual key is missing in headers") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/bifrost.go(2 hunks)core/mcp/mcp.go(2 hunks)core/mcp/toolmanager.go(7 hunks)core/mcp/utils.go(0 hunks)docs/features/governance/virtual-keys.mdx(1 hunks)framework/logstore/rdb.go(1 hunks)plugins/governance/main.go(1 hunks)plugins/logging/main.go(2 hunks)transports/bifrost-http/handlers/inference.go(0 hunks)transports/bifrost-http/handlers/mcp_server.go(1 hunks)transports/bifrost-http/integrations/anthropic.go(1 hunks)transports/bifrost-http/server/server.go(5 hunks)transports/go.mod(1 hunks)ui/app/workspace/mcp-gateway/views/mcpClientSheet.tsx(1 hunks)ui/app/workspace/mcp-gateway/views/mcpClientsTable.tsx(1 hunks)ui/components/ui/sheet.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- transports/bifrost-http/handlers/inference.go
- core/mcp/utils.go
🚧 Files skipped from review as they are similar to previous changes (9)
- transports/go.mod
- transports/bifrost-http/integrations/anthropic.go
- ui/components/ui/sheet.tsx
- docs/features/governance/virtual-keys.mdx
- plugins/logging/main.go
- core/bifrost.go
- transports/bifrost-http/server/server.go
- ui/app/workspace/mcp-gateway/views/mcpClientsTable.tsx
- core/mcp/mcp.go
🧰 Additional context used
🧬 Code graph analysis (1)
core/mcp/toolmanager.go (2)
core/schemas/chatcompletions.go (3)
ChatTool(201-205)ChatToolTypeFunction(196-196)ChatToolFunction(208-213)core/schemas/bifrost.go (3)
BifrostRequest(144-154)ResponsesRequest(91-91)BifrostContextKey(101-101)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (10)
ui/app/workspace/mcp-gateway/views/mcpClientSheet.tsx (1)
186-187: No issues found:showCloseButtonprop is fully supported by SheetHeader.The
showCloseButtonprop is properly defined inui/components/ui/sheet.tsx(line 97) as an optional boolean that defaults totrue. The implementation correctly conditionalizes the close button rendering at line 115. The changes to addw-fullclasses and setshowCloseButton={false}are appropriate and follow the established component API.plugins/governance/main.go (1)
345-346: LGTM!The updated error message is cleaner and more user-friendly, avoiding exposure of internal header naming details while still being informative.
core/mcp/toolmanager.go (2)
77-125: LGTM!The
GetAvailableToolsmethod correctly aggregates tools from all MCP clients with proper nil checks and deduplication. The code mode tools handling is well-structured.
127-184: LGTM!The integration-aware duplicate check map is well-implemented. The empty string guard at line 169 properly addresses the edge case for trailing
__patterns. The switch statement provides good extensibility for future integrations.transports/bifrost-http/handlers/mcp_server.go (6)
26-29: LGTM!The
MCPToolManagerinterface is well-defined with clear method signatures for tool discovery and execution.
31-39: LGTM!The struct correctly uses
sync.RWMutexfor thread-safe access tovkMCPServers, addressing the concurrency concerns from previous reviews.
41-69: LGTM!The constructor properly validates dependencies and initializes the
vkMCPServersmap at line 61, addressing the previous nil map concern.
78-110: LGTM!The JSON-RPC handler is clean with proper context management and error handling. The nil response check correctly handles JSON-RPC notifications.
152-179: LGTM!The full sync implementation correctly acquires a write lock and reinitializes all VK-specific servers. The global server sync followed by per-VK server creation is well-structured.
369-391: LGTM!The
getVKFromRequestutility correctly extracts virtual keys from all three supported header formats (x-bf-vk,Authorization: Bearer, andx-api-key), matching the governance plugin's behavior.
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
from
9092333 to
3867430
Compare
Merge activity
|
Pratham-Mishra04
changed the base branch from
11-18-feat_mcp_agent_and_codemode_added
to
graphite-base/936
Pratham-Mishra04
force-pushed
the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
from
3867430 to
f841eef
Compare
Pratham-Mishra04
deleted the
11-22-feat_mcp_endpoint_added_for_mcp_gateway
branch
1 participant
Summary
Implement MCP (Model Context Protocol) server functionality over HTTP, enabling Bifrost to act as an MCP server that can receive requests from MCP clients and execute tools.
Changes
MCPServerHandlerthat implements the MCP protocol over HTTP streaming/mcp) supporting both JSON-RPC (POST) and Server-Sent Events (GET)Type of change
Affected areas
How to test
Test the MCP server functionality by sending MCP protocol requests to the
/mcpendpoint:Breaking changes
Security considerations
The implementation includes security checks for virtual keys when governance enforcement is enabled, ensuring that MCP server requests are properly authenticated.
Checklist