feat: log tool-call failures - #73
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
tbxark
left a comment
tbxark
left a comment
There was a problem hiding this comment.
Thanks for addressing the missing proxy-side visibility. There is one important gap before merging: MCP tool-originated failures are normally returned as a successful protocol response with CallToolResult.IsError == true and err == nil. The current wrapper only logs non-nil Go errors, so it still misses the common tool execution failure path described by this PR. Please log both cases while continuing to return the original result/error unchanged, and add coverage for protocol/transport errors, IsError results, and successful calls. Also, the repository consistently uses the structured log key err rather than error.
| toolName := tool.Name | ||
| mcpServer.AddTool(tool, func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { | ||
| result, err := c.client.CallTool(ctx, request) | ||
| if err != nil { |
There was a problem hiding this comment.
Please also handle result != nil && result.IsError. MCP specifies that errors originating from a tool should be returned inside CallToolResult with isError: true, usually leaving err == nil; checking only err therefore misses the primary failure case this PR aims to expose.
There was a problem hiding this comment.
Gentle nudge, I addressed the feedback here.
6936196 to
f122611
Compare
Wrap proxied CallTool to log transport errors and MCP tool failures (CallToolResult.IsError) at the proxy without changing returned results. Co-authored-by: Cursor <cursoragent@cursor.com>
f122611 to
b4f1370
Compare
2 participants
Problem
Only the initial connect path (
addToMCPServer) logs errors. Once a client is up,tools/callfailures are returned to the calling client but never appear in the proxy's own logs — so when a downstream server starts erroring, there's nothing on the proxy side to see it in.Change
Wrap each registered tool's handler so failures are logged server-side with the client and tool name, using the existing
slogconventions. The result and error are passed through unchanged, so behaviour toward the caller is identical.Two lines of behaviour change; no config surface.