Log MCPServer handler exceptions by kind and keep crash details off the wire - #1
Log MCPServer handler exceptions by kind and keep crash details off the wire#1niklasgruener wants to merge 8 commits into
Conversation
A crashing tool used to leave no server-side trace: _handle_call_tool
turned the exception into an is_error result before the dispatcher
boundary could log it, so a KeyError('id') reached the model as "'id'"
and its traceback existed nowhere. Resources logged once and prompts
twice. Tool.run also re-wrapped a deliberate ToolError, so nothing
downstream could tell an anticipated failure from a crash.
Tool.run now validates arguments first (a schema rejection is a plain
ToolError chained to the ValidationError) and runs the body under an
except ladder that keeps the distinction in the type: a deliberate
ToolError stays a ToolError, anything else becomes the new
UnexpectedToolError. Both keep the "Error executing tool X: " text, so
results are byte-identical. Resources get the matching
UnexpectedResourceError, raised by whichever layer first sees the
foreign exception so __cause__ is always the original.
_log_handler_exception in server.py is the one place tools and
resources are logged: INFO without a traceback for ToolError and
ResourceError (deliberate, unknown name, bad arguments, not found),
ERROR with the traceback for anything else. get_prompt stops logging,
leaving the dispatcher boundary's record as the only one.
ResourceError raised from a static resource now passes through to the
client as it already did from a template.
Log at the two handler sites directly instead of through a shared helper: the tool site checks for ToolError, the resource site only has to ask whether it caught an UnexpectedResourceError. Drop the three transport-matrix logging tests and their requirement ids from the interaction suite, which is for wire behaviour; the same properties are covered next to MCPServer in test_server.py. Shorten the logging docs to a pointer, reword the handling-errors section plainly, and drop the recap bullet and prompt caveats.
A custom argument validator that raises something other than ValidationError escaped Tool.run unwrapped, losing the "Error executing tool" prefix and the UnexpectedToolError type. It is now wrapped as a crash, and an MCPError raised there still passes through. A ResourceError (usually ResourceNotFoundError from ctx.read_resource) that escapes a tool body is now classified like a ToolError, since it is the same anticipated outcome resources/read logs at INFO. An UnexpectedResourceError escaping a tool stays a crash. MCPServer.read_resource is now the single place a resource crash is wrapped (plus create_resource for templates), so the built-in Resource types let the original exception propagate to direct callers. Also: trimmed raise-site comments in favour of the exception docstrings, reworded the ToolError and ResourceError docstrings, documented the FunctionResource/FileResource.read change in migration.md, corrected the uri-templates tip and example, and pinned the new cases in tests (including a wire test for ResourceNotFoundError from a static resource).
…eption-logging # Conflicts: # docs_src/uri_templates/tutorial002.py
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
The applied suggestion dropped the closing quote along with the interpolated exception text, so prompts/base.py no longer parsed. With the message now just "Error rendering prompt <name>", the legacy-path interaction test snapshots that instead of matching the pydantic prefix.
Keep the one-word correction to the SEP-2164 sentence (static resources now pass ResourceNotFoundError through too), remove the added clause about FunctionResource.read()/FileResource.read(). No-Verification-Needed: docs-only change
A tool that crashed used to send the exception's own text to the client as "Error executing tool <name>: <str(exc)>". That text can describe server internals (or, for an output-schema failure, echo the tool's return value), so a crash now reads just "Error executing tool <name>". ToolError, ResourceError, and argument-validation messages still reach the model unchanged, since those are the anticipated failures it can act on. Closes the tool half of the leak that resources already avoided and that prompts stopped doing earlier in this branch. Related tidy-ups in the same direction: - a crashing @mcp.completion() handler is logged once and answered with -32603 "Error completing argument <name>" instead of str(exc) - the legacy resolver path reports a malformed elicitation answer as a ToolError, matching what the input_required path already did - the INFO line for rejected arguments names the fields, not the values Docs now teach ToolError as the way to talk to the model and describe a plain exception as a crash the model sees generically; examples that relied on ValueError text reaching the client raise ToolError instead.
|
@contextgoblindev |
1 similar comment
|
@contextgoblindev |
|
👀 Got the request — running the review now, results will land here shortly. |
There was a problem hiding this comment.
Review Overview
Verdict:
At a glance: 2 findings — medium: Prompts don't get the INFO/ERROR classification this PR builds for tools and resources · low: Multi-field validation rejection logging is untested beyond a single bad field
Details (2 findings)
Findings (posted as inline comments)
Unplaced findings
- Architecture
src/mcp/server/mcpserver/server.py:1327-1330— Prompts don't get the INFO/ERROR classification this PR builds for tools and resources.get_promptcatchesExceptionand re-raises a bareValueError(str(e)), and_handle_get_prompthas no try/except to classify the failure. Unlike tools (ToolError/UnexpectedToolError) and resources (ResourceError/UnexpectedResourceError), prompts have no typed pair for the dispatcher to distinguish "anticipated" from "crash." Sincehandler_exception_to_error_dataonly special-casesMCPError/ValidationError, a plainValueErrorfalls through to the generic path (logger.exception("modern request handler raised")) unconditionally at ERROR. Callingget_promptwith an unknown name, or any prompt raising anything other thanMCPError, therefore always logs at ERROR with a traceback — there is no INFO tier for prompts at all, contrary to the PR's explicit stated goal of giving operators an INFO/ERROR filter for anticipated vs. crash failures. Recommendation: Give prompts the same typed pair (e.g.PromptError/UnexpectedPromptError) and classify in_handle_get_promptthe way_handle_call_tool/_handle_read_resourcedo, or explicitly document this as an intentional "not yet covered" gap. Add a regression test assertingget_prompt("nonexistent")logs at INFO without a traceback, mirroring the existing tool/resource classification tests.
| if isinstance(exc, ToolError) and not isinstance(exc, UnexpectedToolError): | ||
| if isinstance(exc.__cause__, ValidationError): | ||
| # Field names only: the rejected values are the caller's data. | ||
| fields = sorted({".".join(str(part) for part in err["loc"]) for err in exc.__cause__.errors()}) |
There was a problem hiding this comment.
Multi-field validation rejection logging is untested beyond a single bad field
The new field-extraction logic for the INFO-level "rejected arguments" log (fields = sorted({".".join(...) for err in exc.__cause__.errors()})) is only exercised by a test with a single invalid field. No test supplies two or more invalid arguments, so the sorted(...) ordering and set dedup added by this PR have no case where their removal would change observable test output.
Recommendation: Add a test calling a tool with ≥2 invalid arguments (e.g. add(a="one", b="two")) and assert the exact INFO message lists both field names, comma-separated, in sorted order.
MCPServer now treats an exception from a tool, resource, or prompt handler in one of two ways, decided by its type:
ToolError,ResourceError/ResourceNotFoundError, a schema rejection of the arguments, an unknown name): the message reaches the client as before, and the server writes oneINFOrecord with no traceback.Error executing tool <name>,Error reading resource <uri>,Error rendering prompt <name>), and the server writes oneERRORrecord with the traceback. Nothing from the exception's text goes on the wire, and nothing is logged twice.Fixes modelcontextprotocol#3266. Fixes modelcontextprotocol#698.
Motivation and Context
On
mainthe three primitives each hand-roll their ownexceptladder and each made a different choice:is_error=True,"Error executing tool X: {e}"— the raw exception textERROR+ traceback, once-32603 "Error reading resource {uri}"(text withheld)ERROR+ traceback, twicecode=0with the raw text; modern:Internal server errorTwo problems in that table. A crashing tool leaves no trace on the server (modelcontextprotocol#3266): for a
KeyError('id')the model reads'id'and the traceback exists nowhere. And a crashing tool or prompt sendsstr(exc)to the client (modelcontextprotocol#698), which can describe server internals; for an output-schema failure it echoes the tool's return value.Rather than an eighth site-specific patch (modelcontextprotocol#3267 / modelcontextprotocol#3271 / modelcontextprotocol#2198), the exception's type now carries "was this anticipated?" to one decision point per primitive:
Tool.runvalidates arguments first (a schema rejection is a plainToolErrorchained to theValidationError; a validator that raises anything else is a crash). The body then runs under an except ladder:ToolErrororResourceError(from the tool, a resolver, orctx.read_resource()) is re-raised asToolErrorwith its message; anything else becomes the newUnexpectedToolError(ToolError)whose message is onlyError executing tool <name>and whose__cause__is the original._handle_call_tool/_handle_read_resourcelog at the point the failure becomes a response:INFOfor the anticipated types (rejected arguments log field names, not values),logger.exceptionfor theUnexpected*wrappers.UnexpectedResourceError(ResourceError), raised inMCPServer.read_resource(andResourceTemplate.create_resource), so__cause__is always the original. Built-inResource.read()implementations no longer wrap.logger.exceptioninget_prompt(the dispatcher boundary's record is the only one), andPrompt.renderno longer interpolates the exception text into its message.@mcp.completion()gets the same treatment: a crash is oneERRORrecord and-32603 "Error completing argument <name>".Why level, not "log everything at ERROR": level is the one filter operators get for free and what Sentry/Datadog integrations key on. External FastMCP shipped
logger.exceptionfor every tool failure and walked it back over PrefectHQ/fastmcp item 4036, #4029, #4392 once deliberateToolErrors and model typos flooded error monitoring. modelcontextprotocol#2422 and modelcontextprotocol#2346 are the same signal here.Why withhold crash text: it's the call already made for resources (modelcontextprotocol#1957) and prompts, it's what modelcontextprotocol#698 / modelcontextprotocol#2386 ask for, and it's the default in every framework surveyed (Starlette/uvicorn, Flask, Django, gRPC-java, the C# SDK). Model self-correction is preserved because the two channels the model can act on,
ToolErrorand argument-validation text, still pass through.Client-visible changes
ToolError/ResourceError/MCPError→contentisError executing tool <name>(was…: <str(exc)>). Same for a crashing resolver, a crashing validator, and an output-schema failure.Prompt.renderfailure on the legacy path →Error rendering prompt <name>(was…: <str(exc)>).@mcp.completion()crash →-32603 Error completing argument <name>(legacy path wascode=0, str(exc)).ResourceError/ResourceNotFoundErrorfrom a static resource now pass through (-32602/ your message) as they already did from a template. Also visible one level up when a tool reads such a resource viactx.read_resource().Not in here
code=0, str(e)for any other unmapped handler exception on 2025-era transports, lowlevelServerincluded) is unchanged; it has its own TODO /protocol:error:internal-errordivergence and a wider blast radius.validate_call, which fuses validation with the call. Not a regression. Follow-up.Error executing tool X:prefix for a deliberateToolError(feat(mcpserver): let ToolError carry content for is_error results modelcontextprotocol/python-sdk#2984 territory).MCPServer's defaultRichHandlerrenders a crash as 100+ stderr lines at 80 columns under a stdio host; tool crashes now join resources/prompts there. Separate conversation.How Has This Been Tested?
tests/server/mcpserver/test_server.py: level, message, traceback identity and wire result per class — crash,ToolError,ToolErrorsubclass, bad arguments (INFO names fields;__cause__is theValidationError), validator crash vs validatorMCPError,ValidationErrorinside the body / output-schema failure (crashes), unknown tool,MCPError(no record), resolverToolErrorvs crash,ResourceNotFoundErrorvs resource crash escaping a tool, a tool that recovers from a missing resource (nothing logged), static / template / custom-subclass resource crash, staticResourceNotFoundError, deliberateResourceError, completion crash vsMCPError, prompt crash logged once, nested tool crash, and the directcall_tool()/read_resource()type and__cause__contracts.ResourceNotFoundError→-32602.tests/docs_src/*: every rewritten docs claim is exercised.log_level="WARNING"leaves only the crash records../scripts/test: 100% coverage,strict-no-cover, pyright, pre-commit clean.Breaking Changes
The client-visible changes listed above.
docs/servers/handling-errors.mdnow teachesToolErroras the way to hand the model a message; code that raised a plain exception expecting the model to read its text should switch toToolError. The new exception types subclass the existing ones, soexcept ToolError/except ResourceErrorand the documentedRaises:contracts keep working. Softer differences:FunctionResource.read()/FileResource.read()called directly raise the original exception instead of aValueError;MCPServer.read_resource()/get_prompt()no longer log by themselves; log message wording changed.Types of changes
Checklist
Additional context
Supersedes modelcontextprotocol#3267, modelcontextprotocol#3271, and modelcontextprotocol#2198 (thank you all — the diagnoses were right; this moves the fix to where all three primitives share it). Related: modelcontextprotocol#2153, modelcontextprotocol#2386, modelcontextprotocol#2422.
AI Disclaimer