fix(mcp): route FastMCP ValidationError through the validation error handler (#42578) - #42738
Conversation
#42578) fastmcp.exceptions.ValidationError (raised for malformed tool arguments) is not a subclass of pydantic.ValidationError, the only type GlobalErrorHandlerMiddleware._handle_error checks for, so a client-side argument error falls through to the generic "Internal error... contact support" branch instead of the self-correcting "Validation error in <tool>: ..." message. Red today, confirming the bug as reported. PR #41921 touches this same file extensively but does not change this dispatch line -- the bug remains live despite a bot comment on the issue claiming otherwise.
Code Review Agent Run #56010eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42738 +/- ##
==========================================
+ Coverage 65.57% 65.59% +0.01%
==========================================
Files 2818 2819 +1
Lines 160038 160168 +130
Branches 36557 36570 +13
==========================================
+ Hits 104942 105055 +113
- Misses 53051 53064 +13
- Partials 2045 2049 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…handler (#42578) GlobalErrorHandlerMiddleware._handle_error only matched pydantic's ValidationError, not FastMCP's own distinct fastmcp.exceptions.ValidationError (raised on malformed/missing tool arguments), so those calls fell through to the generic "Internal error... contact support" response instead of a client-recoverable validation message. FastMCP's ValidationError has no .errors() API, so it gets its own message-formatting branch rather than reusing pydantic's field-by-field details.
rusackas
changed the title
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #83ea3aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Verified the routing fix end to end; approving.
Routing / class match — _handle_error now catches fastmcp.exceptions.ValidationError (aliased FastMCPValidationError), which is the class FastMCP's _call_tool documents it raises on tool-argument validation failure (server.py docstring + the "fastmcp's own ValidationError is a FastMCPError" comment). It is FastMCPError → Exception in the installed fastmcp — not a subclass of ToolError nor of pydantic's ValidationError, both of which are checked earlier, so the new elif is reachable and not shadowed. Confirmed against fastmcp 3.4.2. Not a no-op.
No swallow / no double-log — the branch only selects the client-facing ToolError message; the single event_logger.log(action="mcp_tool_error") sits above the if/elif and fires exactly once, unchanged from every other error type. A genuine non-validation error still falls through to the generic else ("Internal error… Error ID") untouched.
Failure classification (#42736) — the validation error is raised as ToolError, not returned as a result payload, so LoggingMiddleware.on_call_tool sets success = False via its except clause; _is_error_response is never consulted on this path. No risk of a validation failure being audited as a success.
Logging consistency (#42730) — this PR adds no new event_logger.log call; it reuses the existing _handle_error site, so it does not introduce a 4th missing-required-args instance. Audit message is sanitized via _sanitize_error_for_logging (new arm → "Request validation failed"), and _USER_ERROR_TYPES correctly reclassifies it as a WARNING-level user error. The client-facing {error} echo is symmetric with the existing pydantic/ValueError branches and describes the caller's own malformed arguments. No function-body import added.
Tests — the new test pins the handler identity (ToolError matching Validation error in execute_sql, asserts Internal error absent). Verified Rule 26: reverting only the prod elif makes it fail (falls to the generic branch). The non-validation anti-regression is covered by the existing test_unexpected_error_logs_error. Full middleware suite: 87 passed.
|
@aminghadersohi appreciate the thorough pass, especially digging into whether |
3 participants
SUMMARY
Fixes #42578:
GlobalErrorHandlerMiddleware._handle_errorinsuperset/mcp_service/middleware.pyonly matched pydantic'sValidationErrorviaisinstance(error, ValidationError). FastMCP raises its own distinctfastmcp.exceptions.ValidationErrorfor malformed/missing tool arguments — not a subclass of pydantic's — so those calls fell through to the generic "Internal error... contact support" response instead of a proper, client-recoverable validation message.Root cause. Verified independently that
fastmcp.exceptions.ValidationError's MRO does not include pydantic'sValidationError, and that it has no.errors()method (a plainExceptionsubclass with just a message), so it can't reuse pydantic's field-by-field error formatting.Fix. Added FastMCP's
ValidationErrorto the error classification (_USER_ERROR_TYPES,_sanitize_error_for_logging) and gave it its own dispatch branch in_handle_errorthat formats the message directly fromstr(error)instead of calling.errors().Note: a bot comment on the issue claimed PR #41921 already fixes this. I checked that PR's diff directly — it never touches the
isinstance(error, ValidationError)dispatch logic inmiddleware.py. This PR is the actual fix.TESTING INSTRUCTIONS
The existing regression test (
test_fastmcp_validation_error_routes_to_validation_branch) now passes.ADDITIONAL INFORMATION