Propagate tool call exceptions through filters #844
Conversation
FIne by me -- Just need a way to handle. My plan was to dump ex.Message in most cases. Thank you! |
| /// and does not include JSON-RPC error codes. The <see cref="Exception.Message"/> from this exception | ||
| /// will be propagated to the remote endpoint to inform the caller about the tool execution failure. | ||
| /// </remarks> | ||
| public class McpServerToolException : Exception |
There was a problem hiding this comment.
I'm not fully understanding why a second exception type is needed. We can change what we say about McpException just as we can introduce another exception type.
If we're going to introduce this one, then we should probably rename the existing one to something scarier, like McpProtocolException or something.
There was a problem hiding this comment.
Also, just by looking at the name McpServerToolException I expect that it's a subtype of McpException.
There was a problem hiding this comment.
Alright, added a new McpProtocolException specifically for places where we need to produce a JSON-RPC error despite it being a tool call like the authorization filters and for places where we need to specify a non-InternalError code.
McpException behaves exactly the same as before in that it produces a JSON-RPC error in most contexts but will produce an errored CallToolResult when thrown from an McpServerTool, CallToolHandler, or filter. It is a breaking change since McpException no longer has the ErrorCode property, but that's an easy thing to identify and fix. We could introduce the ErrorCode property and constructors back to McpException and obsolete them, but I don't think they were widely used enough to go through that effort.
Overall, I'm very happy with this change compared to before with McpServerToolException. There was a lot of existing usage of McpException in tools even in our samples, so it's nice to keep the behavior consistent.
There was a problem hiding this comment.
I think it makes sense to distinguish between an exception which should:
- Result in JSON-RPC Error - this is when the JSON-RPC Request fails
- Set IsError to true and (optionally) set a content with the error message - this is when the Tool fails
So I think these exist at different levels of abstraction in the protocol. I am not sure I think it adds clarity that McpException gives JSON-RPC errors in some contexts and tool errors in others. I know there are no other responses with an IsError, but there could be in the future.
I think McpServerToolException is cleaner because it describes a Tool failing, not the CallToolRequest.
I can see how it's a pain with the samples, but having a special type to produce the "normal" behavior is a bit unintuitive.
McpProtocolException derives from McpException which can still be used to control the JsonRpcErrorDetail set by McpSessionHandler and the CallToolResult error content set by McpServerImpl making this change non-breaking unless you were previously setting an ErrorCode
| /// endpoint; sensitive information should not be included. If sensitive details need to be included, | ||
| /// a different exception type should be used. | ||
| /// | ||
| /// This exception type can be thrown by MCP tools or tool call filters to propogate detailed error messages |
There was a problem hiding this comment.
| /// This exception type can be thrown by MCP tools or tool call filters to propogate detailed error messages | |
| /// This exception type can be thrown by MCP tools or tool call filters to propagate detailed error messages |
| return await handler(request, cancellationToken); | ||
| } | ||
| else | ||
| catch (Exception e) when (e is not OperationCanceledException and not McpProtocolException) |
There was a problem hiding this comment.
Why does this need to distinguish McpProtocolException?
5 participants
I thought I added tests that verified that tool call exceptions propagated through filters before getting converted to a CallToolResult with
IsError = truewhen I originally added the filter pipeline in #733, but I did not. If I had, I would have noticed that I put the exception catching logic immediately around the call to McpServerTool.InvokeAsync or the custom CallToolHandler without letting it propagate through the entire filter pipeline which was not my intention.This PR adds a new Exception type, McpServerToolException, because authorization filters still need the ability to throw an McpException to produce a JsonRpcError like it does in other handler types. McpServerToolException allows McpServerTool implementations to retain the ability to produce an CallToolResult with
IsError = trueand a custom message with an exception like before. However, I think even if not for the authorization filters, this change makes sense considering that McpException is "not intended to be used for application-level errors.", and it includes details irrelevant to the CallToolResult like the McpErrorCode.csharp-sdk/src/ModelContextProtocol.Core/McpException.cs
Line 8 in f286391
Technically, it isn't strictly necessary for McpServerToolException to exist considering any McpServerTool or CallToolHandler can just return a CallToolResult with
IsError = true, but that might be inconvenient.Fixes #820
This doesn't fix #830, since ArgumentException's from ReflectionAIFunctionDescriptor.GetParameterMarshaller will still get turned into generic errors without further intervention. But least after this change, developers will be able to add a tool call filter that observes the ArgumentException and turns it into a McpServerToolException (to produce a CallToolResult with IsError = true) or McpException (to produce a JsonRpcError with an McpErrorCode) depending on their needs.