-
Notifications
You must be signed in to change notification settings - Fork 563
Rework MCP error handling #422
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?
Rework MCP error handling #422
Conversation
BREAKING CHANGE: McpError is no longer used for validation and internal errors. Client code catching McpError should now catch specific exception types like IllegalArgumentException, IllegalStateException, McpTransportException, or McpClientInternalException. - Add McpTransportException for transport layer errors - Add McpClientInternalException for internal client failures - Use standard Java exceptions for validation errors - Implement structured error builder with JSON-RPC error codes - Change tool not found errors to be returned in CallToolResult - Update logging levels for unregistered notification handlers Signed-off-by: Christian Tzolov <[email protected]>
Signed-off-by: Christian Tzolov <[email protected]>
@@ -469,11 +471,17 @@ private McpServerSession.RequestHandler<CallToolResult> toolsCallRequestHandler( | |||
.findAny(); | |||
|
|||
if (toolSpecification.isEmpty()) { | |||
return Mono.error(new McpError("Tool not found: " + callToolRequest.name())); | |||
// Tool errors should be reported within the result object, not as MCP |
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 aligns the tool response with the spec: https://modelcontextprotocol.io/specification/2025-06-18/server/tools#error-handling
- Tool errors now return CallToolResult with isError(true) instead of throwing exceptions
- This allows LLMs to see and potentially handle tool errors rather than causing protocol failures
@@ -14,12 +15,41 @@ public McpError(JSONRPCError jsonRpcError) { | |||
this.jsonRpcError = jsonRpcError; | |||
} | |||
|
|||
public McpError(Object error) { |
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.
Now reserved for actual MCP protocol JSON-RPC errors
public JSONRPCError getJsonRpcError() { | ||
return jsonRpcError; | ||
} | ||
|
||
public static Builder builder(int errorCode) { |
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.
Added builder pattern for proper error structure: McpError.builder(errorCode).message("...").data("...").build()
@@ -221,7 +221,7 @@ private Mono<Void> handleIncomingNotification(McpSchema.JSONRPCNotification noti | |||
return Mono.defer(() -> { | |||
var handler = notificationHandlers.get(notification.method()); | |||
if (handler == null) { | |||
logger.error("No handler registered for notification method: {}", notification.method()); | |||
logger.warn("No handler registered for notification: {}", notification); |
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.
Log abnormal behavior without indicating it's an error.
Perhaps consider using debug level logging?
@@ -248,7 +248,7 @@ private Mono<Void> handleIncomingNotification(McpSchema.JSONRPCNotification noti | |||
|
|||
var handler = notificationHandlers.get(notification.method()); | |||
if (handler == null) { | |||
logger.error("No handler registered for notification method: {}", notification.method()); | |||
logger.warn("No handler registered for notification: {}", notification); |
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.
Log abnormal behavior without indicating it's an error.
Perhaps consider using debug level logging?
Refactor exception handling to introduce a proper exception hierarchy, replacing generic
McpError
usage with specific exception types for better error categorization and handling.Motivation and Context
The existing codebase was using
McpError
as a catch-all exception type for various error scenarios, making it difficult to distinguish between different types of failures. This change addresses several issues:How Has This Been Tested?
Breaking Changes
Yes, this is a breaking change for applications that catch
McpError
exceptions. Users will need to update their exception handling code to catch the new specific exception types:McpTransportException
for transport-related errorsMcpClientInternalException
for internal client errorsIllegalArgumentException
for validation errorsIllegalStateException
for state-related errorsMcpError
now only for actual MCP protocol JSON-RPC errorsTypes of changes
Checklist
Additional context
Key Changes:
New Exception Classes:
McpTransportException
: For transport layer errors (network, serialization, SSE parsing)McpClientInternalException
: For internal client errors (initialization failures, endpoint handling)McpError Refinement:
McpError.builder(errorCode).message("...").data("...").build()
Standard Java Exceptions:
IllegalArgumentException
for validation errors (null parameters, invalid arguments)IllegalStateException
for state-related errors (missing capabilities, not initialized)Tool Error Handling Improvement:
CallToolResult
withisError(true)
instead of throwing exceptionsMigration Guide:
Applications catching
McpError
should update their exception handling:This refactoring significantly improves error handling semantics and makes the codebase more maintainable while following Java exception handling best practices.