Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions core/bifrost.go
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,7 @@ func (bifrost *Bifrost) ChatCompletionRequest(ctx *schemas.BifrostContext, req *
req,
response,
bifrost.makeChatCompletionRequest,
bifrost.executeMCPToolWithHooks,
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

Expand Down Expand Up @@ -777,6 +778,7 @@ func (bifrost *Bifrost) ResponsesRequest(ctx *schemas.BifrostContext, req *schem
req,
response,
bifrost.makeResponsesRequest,
bifrost.executeMCPToolWithHooks,
)
}

Expand Down Expand Up @@ -3776,6 +3778,38 @@ func (bifrost *Bifrost) handleMCPToolExecution(ctx *schemas.BifrostContext, mcpR
return finalResp, nil
}

// executeMCPToolWithHooks is a wrapper around handleMCPToolExecution that matches the signature
// expected by the agent's executeToolFunc parameter. It runs MCP plugin hooks before and after
// tool execution to enable logging, telemetry, and other plugin functionality.
func (bifrost *Bifrost) executeMCPToolWithHooks(ctx *schemas.BifrostContext, request *schemas.BifrostMCPRequest) (*schemas.BifrostMCPResponse, error) {
// Defensive check: context must be non-nil to prevent panics in plugin hooks
if ctx == nil {
return nil, fmt.Errorf("context cannot be nil")
}

if request == nil {
return nil, fmt.Errorf("request cannot be nil")
}

// Determine request type from the MCP request - explicitly handle all known types
var requestType schemas.RequestType
switch request.RequestType {
case schemas.MCPRequestTypeChatToolCall:
requestType = schemas.ChatCompletionRequest
case schemas.MCPRequestTypeResponsesToolCall:
requestType = schemas.ResponsesRequest
default:
// Return error for unknown/unsupported request types instead of silently defaulting
return nil, fmt.Errorf("unsupported MCP request type: %s", request.RequestType)
}

resp, bifrostErr := bifrost.handleMCPToolExecution(ctx, request, requestType)
if bifrostErr != nil {
return nil, fmt.Errorf("%s", GetErrorMessage(bifrostErr))
}
return resp, nil
}
Comment on lines +3781 to +3811
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Harden wrapper error/response semantics (avoid empty error strings; avoid nil, nil).

Two edge cases to guard:

  • GetErrorMessage(bifrostErr) can be "" → returns an unhelpful empty error.
  • If post-hooks return (nil, nil) you currently return (nil, nil) which is ambiguous for callers.
Proposed fix
 func (bifrost *Bifrost) executeMCPToolWithHooks(ctx *schemas.BifrostContext, request *schemas.BifrostMCPRequest) (*schemas.BifrostMCPResponse, error) {
 	// Defensive check: context must be non-nil to prevent panics in plugin hooks
 	if ctx == nil {
 		return nil, fmt.Errorf("context cannot be nil")
 	}

 	if request == nil {
 		return nil, fmt.Errorf("request cannot be nil")
 	}

 	// Determine request type from the MCP request - explicitly handle all known types
 	var requestType schemas.RequestType
 	switch request.RequestType {
 	case schemas.MCPRequestTypeChatToolCall:
 		requestType = schemas.ChatCompletionRequest
 	case schemas.MCPRequestTypeResponsesToolCall:
 		requestType = schemas.ResponsesRequest
 	default:
 		// Return error for unknown/unsupported request types instead of silently defaulting
 		return nil, fmt.Errorf("unsupported MCP request type: %s", request.RequestType)
 	}

 	resp, bifrostErr := bifrost.handleMCPToolExecution(ctx, request, requestType)
 	if bifrostErr != nil {
-		return nil, fmt.Errorf("%s", GetErrorMessage(bifrostErr))
+		msg := strings.TrimSpace(GetErrorMessage(bifrostErr))
+		if msg == "" {
+			msg = "mcp tool execution failed"
+		}
+		return nil, fmt.Errorf("%s", msg)
 	}
+	if resp == nil {
+		return nil, fmt.Errorf("mcp tool execution returned nil response")
+	}
 	return resp, nil
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// executeMCPToolWithHooks is a wrapper around handleMCPToolExecution that matches the signature
// expected by the agent's executeToolFunc parameter. It runs MCP plugin hooks before and after
// tool execution to enable logging, telemetry, and other plugin functionality.
func (bifrost *Bifrost) executeMCPToolWithHooks(ctx *schemas.BifrostContext, request *schemas.BifrostMCPRequest) (*schemas.BifrostMCPResponse, error) {
// Defensive check: context must be non-nil to prevent panics in plugin hooks
if ctx == nil {
return nil, fmt.Errorf("context cannot be nil")
}
if request == nil {
return nil, fmt.Errorf("request cannot be nil")
}
// Determine request type from the MCP request - explicitly handle all known types
var requestType schemas.RequestType
switch request.RequestType {
case schemas.MCPRequestTypeChatToolCall:
requestType = schemas.ChatCompletionRequest
case schemas.MCPRequestTypeResponsesToolCall:
requestType = schemas.ResponsesRequest
default:
// Return error for unknown/unsupported request types instead of silently defaulting
return nil, fmt.Errorf("unsupported MCP request type: %s", request.RequestType)
}
resp, bifrostErr := bifrost.handleMCPToolExecution(ctx, request, requestType)
if bifrostErr != nil {
return nil, fmt.Errorf("%s", GetErrorMessage(bifrostErr))
}
return resp, nil
}
// executeMCPToolWithHooks is a wrapper around handleMCPToolExecution that matches the signature
// expected by the agent's executeToolFunc parameter. It runs MCP plugin hooks before and after
// tool execution to enable logging, telemetry, and other plugin functionality.
func (bifrost *Bifrost) executeMCPToolWithHooks(ctx *schemas.BifrostContext, request *schemas.BifrostMCPRequest) (*schemas.BifrostMCPResponse, error) {
// Defensive check: context must be non-nil to prevent panics in plugin hooks
if ctx == nil {
return nil, fmt.Errorf("context cannot be nil")
}
if request == nil {
return nil, fmt.Errorf("request cannot be nil")
}
// Determine request type from the MCP request - explicitly handle all known types
var requestType schemas.RequestType
switch request.RequestType {
case schemas.MCPRequestTypeChatToolCall:
requestType = schemas.ChatCompletionRequest
case schemas.MCPRequestTypeResponsesToolCall:
requestType = schemas.ResponsesRequest
default:
// Return error for unknown/unsupported request types instead of silently defaulting
return nil, fmt.Errorf("unsupported MCP request type: %s", request.RequestType)
}
resp, bifrostErr := bifrost.handleMCPToolExecution(ctx, request, requestType)
if bifrostErr != nil {
msg := strings.TrimSpace(GetErrorMessage(bifrostErr))
if msg == "" {
msg = "mcp tool execution failed"
}
return nil, fmt.Errorf("%s", msg)
}
if resp == nil {
return nil, fmt.Errorf("mcp tool execution returned nil response")
}
return resp, nil
}
🤖 Prompt for AI Agents
In @core/bifrost.go around lines 3662 - 3692, In executeMCPToolWithHooks,
safeguard against empty error strings and ambiguous (nil, nil) returns from
handleMCPToolExecution: after calling bifrost.handleMCPToolExecution, if
bifrostErr != nil extract msg := GetErrorMessage(bifrostErr) and if msg == ""
set a default like "unknown MCP tool execution error" then return nil,
fmt.Errorf(msg); if bifrostErr == nil but resp == nil return nil,
fmt.Errorf("MCP tool execution returned nil response without error") so callers
never get (nil, nil); keep existing nil checks and the request type switching
as-is.


// PLUGIN MANAGEMENT

// RunPreHooks executes PreHooks in order, tracks how many ran, and returns the final request, any short-circuit decision, and the count.
Expand Down
6 changes: 4 additions & 2 deletions core/mcp/mcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ func (m *MCPManager) CheckAndExecuteAgentForChatRequest(
req *schemas.BifrostChatRequest,
response *schemas.BifrostChatResponse,
makeReq func(ctx *schemas.BifrostContext, req *schemas.BifrostChatRequest) (*schemas.BifrostChatResponse, *schemas.BifrostError),
executeTool func(ctx *schemas.BifrostContext, request *schemas.BifrostMCPRequest) (*schemas.BifrostMCPResponse, error),
) (*schemas.BifrostChatResponse, *schemas.BifrostError) {
if makeReq == nil {
return nil, &schemas.BifrostError{
Expand All @@ -197,7 +198,7 @@ func (m *MCPManager) CheckAndExecuteAgentForChatRequest(
return response, nil
}
// Execute agent mode
return m.toolsManager.ExecuteAgentForChatRequest(ctx, req, response, makeReq)
return m.toolsManager.ExecuteAgentForChatRequest(ctx, req, response, makeReq, executeTool)
}

// CheckAndExecuteAgentForResponsesRequest checks if the responses response contains tool calls,
Expand Down Expand Up @@ -233,6 +234,7 @@ func (m *MCPManager) CheckAndExecuteAgentForResponsesRequest(
req *schemas.BifrostResponsesRequest,
response *schemas.BifrostResponsesResponse,
makeReq func(ctx *schemas.BifrostContext, req *schemas.BifrostResponsesRequest) (*schemas.BifrostResponsesResponse, *schemas.BifrostError),
executeTool func(ctx *schemas.BifrostContext, request *schemas.BifrostMCPRequest) (*schemas.BifrostMCPResponse, error),
) (*schemas.BifrostResponsesResponse, *schemas.BifrostError) {
if makeReq == nil {
return nil, &schemas.BifrostError{
Expand All @@ -248,7 +250,7 @@ func (m *MCPManager) CheckAndExecuteAgentForResponsesRequest(
return response, nil
}
// Execute agent mode
return m.toolsManager.ExecuteAgentForResponsesRequest(ctx, req, response, makeReq)
return m.toolsManager.ExecuteAgentForResponsesRequest(ctx, req, response, makeReq, executeTool)
}

// Cleanup performs cleanup of all MCP resources including clients and local server.
Expand Down
16 changes: 14 additions & 2 deletions core/mcp/toolmanager.go
Original file line number Diff line number Diff line change
Expand Up @@ -517,15 +517,21 @@ func (m *ToolsManager) ExecuteAgentForChatRequest(
req *schemas.BifrostChatRequest,
resp *schemas.BifrostChatResponse,
makeReq func(ctx *schemas.BifrostContext, req *schemas.BifrostChatRequest) (*schemas.BifrostChatResponse, *schemas.BifrostError),
executeTool func(ctx *schemas.BifrostContext, request *schemas.BifrostMCPRequest) (*schemas.BifrostMCPResponse, error),
) (*schemas.BifrostChatResponse, *schemas.BifrostError) {
// Use provided executeTool function, or fall back to internal ExecuteTool
executeToolFunc := executeTool
if executeToolFunc == nil {
executeToolFunc = m.ExecuteTool
}
return ExecuteAgentForChatRequest(
ctx,
int(m.maxAgentDepth.Load()),
req,
resp,
makeReq,
m.fetchNewRequestIDFunc,
m.ExecuteTool,
executeToolFunc,
m.clientManager,
)
}
Expand All @@ -548,15 +554,21 @@ func (m *ToolsManager) ExecuteAgentForResponsesRequest(
req *schemas.BifrostResponsesRequest,
resp *schemas.BifrostResponsesResponse,
makeReq func(ctx *schemas.BifrostContext, req *schemas.BifrostResponsesRequest) (*schemas.BifrostResponsesResponse, *schemas.BifrostError),
executeTool func(ctx *schemas.BifrostContext, request *schemas.BifrostMCPRequest) (*schemas.BifrostMCPResponse, error),
) (*schemas.BifrostResponsesResponse, *schemas.BifrostError) {
// Use provided executeTool function, or fall back to internal ExecuteTool
executeToolFunc := executeTool
if executeToolFunc == nil {
executeToolFunc = m.ExecuteTool
}
return ExecuteAgentForResponsesRequest(
ctx,
int(m.maxAgentDepth.Load()),
req,
resp,
makeReq,
m.fetchNewRequestIDFunc,
m.ExecuteTool,
executeToolFunc,
m.clientManager,
)
}
Expand Down
4 changes: 0 additions & 4 deletions core/mcp/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,19 +64,15 @@ func (m *MCPManager) GetToolPerClient(ctx context.Context) map[string][]schemas.
continue
}

logger.Debug(fmt.Sprintf("Checking tools for MCP client %s with tools to execute: %v", clientName, client.ExecutionConfig.ToolsToExecute))

// Add all tools from this client
for toolName, tool := range client.ToolMap {
// Check if tool should be skipped based on client configuration
if shouldSkipToolForConfig(toolName, client.ExecutionConfig) {
logger.Debug(fmt.Sprintf("%s Skipping MCP tool %s: not in tools to execute list", MCPLogPrefix, toolName))
continue
}

// Check if tool should be skipped based on request context
if shouldSkipToolForRequest(ctx, clientName, toolName) {
logger.Debug(fmt.Sprintf("%s Skipping MCP tool %s: not in include tools list", MCPLogPrefix, toolName))
continue
}

Expand Down
1 change: 1 addition & 0 deletions core/schemas/bifrost.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ const (
FileDeleteRequest RequestType = "file_delete"
FileContentRequest RequestType = "file_content"
CountTokensRequest RequestType = "count_tokens"
MCPToolExecutionRequest RequestType = "mcp_tool_execution"
UnknownRequest RequestType = "unknown"
)

Expand Down
22 changes: 22 additions & 0 deletions docs/openapi/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,14 @@ paths:
/api/logs/recalculate-cost:
$ref: './paths/management/logging.yaml#/logs-recalculate-cost'

# MCP Logs
/api/mcp-logs:
$ref: './paths/management/logging.yaml#/mcp-logs'
/api/mcp-logs/stats:
$ref: './paths/management/logging.yaml#/mcp-logs-stats'
/api/mcp-logs/filterdata:
$ref: './paths/management/logging.yaml#/mcp-logs-filterdata'

# Cache
/api/cache/clear/{requestId}:
$ref: './paths/management/cache.yaml#/clear-by-request-id'
Expand Down Expand Up @@ -832,6 +840,20 @@ components:
RecalculateCostResponse:
$ref: './schemas/management/logging.yaml#/RecalculateCostResponse'

# MCP Logs
MCPToolLogEntry:
$ref: './schemas/management/logging.yaml#/MCPToolLogEntry'
MCPToolLogSearchFilters:
$ref: './schemas/management/logging.yaml#/MCPToolLogSearchFilters'
MCPToolLogStats:
$ref: './schemas/management/logging.yaml#/MCPToolLogStats'
SearchMCPLogsResponse:
$ref: './schemas/management/logging.yaml#/SearchMCPLogsResponse'
MCPLogsFilterDataResponse:
$ref: './schemas/management/logging.yaml#/MCPLogsFilterDataResponse'
DeleteMCPLogsRequest:
$ref: './schemas/management/logging.yaml#/DeleteMCPLogsRequest'

# Cache
ClearCacheResponse:
$ref: './schemas/management/cache.yaml#/ClearCacheResponse'
Loading