-
Notifications
You must be signed in to change notification settings - Fork 75
Fix tool error handling to follow MCP spec #165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix tool error handling to follow MCP spec #165
Conversation
Tool execution errors now return successful responses with isError: true instead of JSON-RPC protocol errors, per the MCP specification. This allows clients to distinguish between protocol-level errors and tool execution errors, providing better error context. Fixes modelcontextprotocol#159
|
||
begin | ||
call_tool_with_args(tool, arguments) | ||
rescue => e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a step in the right direction, but I think we can take this a few more steps further:
This rescue should be at the call_tool
level: Any mistake made by the caller that falls out of valid tool contracts (e.g. invalid tool name, invalid args) should be non-exception tool results with isError: true
. Only JSON-RPC-level protocol errors should be allowed to be returned as JSON-RPC errors with error codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atesgoral makes sense. This is how it should work now:
Original (Everything was a Protocol Error)
{
"jsonrpc": "2.0",
"id": 1,
"error": {
"code": -32603,
"message": "Internal error",
"data": "Tool not found unknown_tool"
}
}
Now - Protocol Error
{
"jsonrpc": "2.0",
"id": 1,
"error": {
"code": -32601,
"message": "Method not found",
"data": "unknown_method"
}
}
Now - Tool error:
{
"jsonrpc": "2.0",
"id": 1,
"result": {
"content": [{
"type": "text",
"text": "Tool not found: unknown_tool"
}],
"isError": true
}
}
|
||
def call_tool(request) | ||
tool_name = request[:name] | ||
arguments = request[:arguments] || {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems better to keep it at its original position on line 267, since there's no need to process it until it becomes necessary.
I've left one small comment, but overall it looks good to me. Can you address it and squash the commits in to one? |
Tool execution errors now return successful responses with isError: true
instead of JSON-RPC protocol errors, per the MCP specification.
Motivation and Context
This allows clients to distinguish between protocol-level errors and
tool execution errors, providing better error context.
Fixes #159
How Has This Been Tested?
Updated unit tests to support new error behavior. All existing tests passing.
Breaking Changes
If client libs are parsing the error message for the specific tool call error, then they will likely get a parser error due to the change of the error structure.
Current behavior:
Updated behavior (matches spec and readme):
Types of changes
Checklist