Log MCPServer handler exceptions by kind and keep crash details off the wire - #3314
Log MCPServer handler exceptions by kind and keep crash details off the wire#3314maxisbey 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.
📚 Documentation preview
|
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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/mcp/server/mcpserver/resources/types.py— FileResource.read (and DirectoryResource.read at line 247) now raise UnexpectedResourceError but their docstrings were not given theRaises:section that this same PR added to FunctionResource.read and ResourceTemplate.create_resource.Extended reasoning...
AGENTS.md (Code Quality) requires: "When a public API raises exceptions a caller would reasonably catch, document them in a
Raises:section." A caller reading FileResource.read's docstring ("Read the file content.") has no way to know a missing/unreadable file now surfaces as UnexpectedResourceError with the FileNotFoundError/PermissionError in cause, inconsistent with the sibling resource types updated in the same change.Verification: nit — The claim is factually true. This PR changed FileResource.read (src/mcp/server/mcpserver/resources/types.py:198-199:
raise UnexpectedResourceError(f"Error reading resource {self.uri}") from exc) and DirectoryResource.read (lines 252-253, same raise) to raise UnexpectedResourceError, but their docstrings remain "Read the file content." (line 193) and "Read the directory listing." (lin
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
There was a problem hiding this comment.
Beyond the inline findings, this run also checked the new validation ladder from 6fd5e05 for a deliberate ToolError raised inside a custom argument validator or default_factory: pydantic propagates it raw, so it lands in the except Exception arm and is classified as a crash — but the wire result (prefix, message, is_error=True) is identical to main, so the only effect is the ERROR-vs-INFO log level for a pattern the docstrings don't sanction, and it was ruled out as a finding.
Extended reasoning...
The commits pushed since the prior review (6fd5e05 in particular) address the earlier red-flag finding at tools/base.py:151 — non-ValidationError exceptions from validators are now wrapped as UnexpectedToolError instead of escaping raw. This run's two remaining findings (a Raises-docstring inaccuracy in server.py and a dead arguments argument at the call_fn_with_arg_validation call site) are posted inline and are quality-level, not correctness blockers. The one new candidate investigated this run — a deliberate ToolError from a custom validator being reclassified as a crash — was verified against both HEAD and the base commit: the client-visible text and is_error result are byte-identical, so the difference is confined to the new log-level taxonomy for an out-of-contract raising site. The hunt exited at max_rounds, so approval is off the table regardless; this note only records what else was examined.
| UnexpectedToolError: If the tool (or a resolver) raises anything other than | ||
| `ToolError` or `MCPError`, or its return value fails output conversion. |
There was a problem hiding this comment.
🟡 nit: MCPServer.call_tool's new Raises docstring says UnexpectedToolError is raised when "the tool (or a resolver) raises anything other than ToolError or MCPError", but Tool.run's ladder (src/mcp/server/mcpserver/tools/base.py:208-210) deliberately re-raises a plain ResourceError/ResourceNotFoundError (e.g. propagated from ctx.read_resource()) as a plain anticipated ToolError, never UnexpectedToolError. The ToolError arm of the same docstring (lines 518-519) also omits this ResourceError case. Tool.run's own docstring (tools/base.py:140-144) states the contract correctly, so the two public docstrings in the same PR… [also at: src/mcp/server/mcpserver/exceptions.py:51 - nit: ToolError docstring says "A ResourceError that escapes the tool ... counts as anticipated too", but…]
Extended reasoning...
A programmatic caller of MCPServer.call_tool() whose tool lets ResourceNotFoundError from ctx.read_resource() propagate reads this Raises section (AGENTS.md requires Raises sections to document catchable exceptions accurately) and writes except UnexpectedToolError expecting to catch that case as documented, or conversely treats any non-Unexpected ToolError as covering only the three listed causes. At runtime the failure arrives as a plain ToolError (proved by this PR's own test test_resource_error_escaping_a_tool_is_anticipated asserting type(exc.value) is ToolError), so code keyed to the documented UnexpectedToolError classification silently takes the wrong branch — e.g. crash-alerting logic that re-reports UnexpectedToolError never fires, or the author adds a needless try/except inside the tool to "fix" a misclassification that never happens.
Verification: nit — the docstring is factually inaccurate about the raised type, though runtime behavior is intentional. server.py:520-521 says "UnexpectedToolError: If the tool (or a resolver) raises anything other than ToolError or MCPError", but tools/base.py:208-210 has except (ToolError, ResourceError) as exc: raise ToolError(...) from exc — a plain ResourceError/ResourceNotFoundError (a sibling of
| self.is_async, | ||
| arguments, | ||
| pass_directly or None, | ||
| pre_validated=pre_validated, | ||
| pre_validated=validated, | ||
| ) |
There was a problem hiding this comment.
🟡 After hoisting validation, Tool.run always passes pre_validated=validated to call_fn_with_arg_validation, so the raw arguments argument at this call site is dead and the helper's internal validate-fallback branch (func_metadata.py:118-120) is no longer exercised by any SDK code path — the call could simply invoke the function with the merged validated kwargs.
Extended reasoning...
Concrete cost: misleading data flow and retained duplicate logic. Before this change the fallback self.validate_arguments(arguments_to_validate) inside call_fn_with_arg_validation (src/mcp/server/mcpserver/utilities/func_metadata.py:118-120) was the live validation path for every tool without Resolve() params; now Tool.run validates up front (tools/base.py:147) and this remains the helper's only SDK call site, so a reader of tools/base.py:174-180 sees arguments passed as if it might be re-validated when it is always ignored, and the helper keeps a second validation path that only third-party callers could reach. Simpler form: merge validated | (pass_directly or {}) and dispatch to the function directly (or pass an empty dict with a comment), keeping one obvious validation site.
Verification: nit — The claim is factually true. tools/base.py:146-156 hoists validated = self.fn_metadata.validate_arguments(arguments) ahead of the body try, and line 179 unconditionally passes pre_validated=validated, so the raw arguments positional at line 177 is dead at this call site. func_metadata.py:118-120 only reads arguments_to_validate in the pre_validated is None fallback, and grep shows
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.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
I re-reviewed the latest push and found no new bugs; the points raised in earlier rounds (the prompts/base.py syntax error, the tools/base.py validation ladder, and the two doc inconsistencies) are addressed in the current code, but given this PR reworks the error-handling taxonomy across tools, resources, and prompts, a maintainer review is still worthwhile.
What was reviewed this run:
- The hoisted argument-validation ladder in tools/base.py, including a ToolError/ResourceError raised from a field validator or default_factory (now lands on the explicit crash rung, judged acceptable).
- A ResourceNotFoundError escaping a nested ctx.read_resource() inside a resource handler being forwarded as the outer read's -32602 — pre-existing behavior, not introduced here.
- The corrected uri-templates tip and troubleshooting log-level wording against the code paths they describe.
Extended reasoning...
Overview
This PR reworks the anticipated-vs-crash error taxonomy in the mcpserver layer: src/mcp/server/mcpserver/tools/base.py (argument validation hoisted out of the execution try with a four-rung ladder: ValidationError becomes an anticipated ToolError, MCPError re-raises, everything else becomes UnexpectedToolError), resources/templates.py and resources/types.py (UnexpectedResourceError wrapping), prompts/base.py, exceptions.py, server.py handler logging, plus matching docs (docs/servers/handling-errors.md, docs/servers/uri-templates.md, docs/troubleshooting.md, docs/handlers/logging.md) and extensive test updates in tests/interaction/ and tests/server/mcpserver/.
Security risks
No injection, auth, or crypto surface is touched. The change is security-relevant in one narrow sense: it decides which exception messages reach clients verbatim versus being sanitized to a generic -32603. The current code keeps deliberate ToolError/ResourceError messages flowing to clients (as before) and sanitizes unexpected crashes, which is the safer direction. I found no path where a previously sanitized message becomes exposed.
Level of scrutiny
High. This is a behavioral contract change on a released 2.x line (per AGENTS.md, observable-behavior changes are explicit maintainer decisions), and it drew three prior review rounds from this system with red findings — including a genuine SyntaxError in prompts/base.py:213 on an earlier push. I verified from the current checkout that the syntax error is fixed, the validation ladder now has the MCPError and catch-all rungs my earlier comment said were missing, and both doc pages I flagged now state the log-level and -32603 behavior consistently with the code. One earlier finding (template-parameter ValidationError from validate_call classified as UnexpectedResourceError in templates.py:247-248) remains as-is in code; the author resolved that thread, which reads as a deliberate classification choice rather than an oversight, and I am not re-litigating it — but it is exactly the kind of judgment call a maintainer should confirm.
Other factors
The bug hunt exited on dry_streak with zero findings on this version, and the three candidates investigated this run were all ruled out (two were refinements of the already-addressed validation-ladder concern; the third is pre-existing behavior). Test coverage is thorough — the interaction suite and server tests assert the exact log levels, messages, and error codes for each rung. A maintainer (Kludex) has already engaged on the PR. Approval is not appropriate given the breaking-behavior scope and the author-self-resolved design threads, so a defer acknowledging the fixes and recording what was checked is the right outcome.
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.
maxisbey
changed the title
There was a problem hiding this comment.
8 issues found across 28 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/troubleshooting.md">
<violation number="1" location="docs/troubleshooting.md:95">
P2: When a tool raises `MCPError`, `_handle_call_tool` re-raises it as a JSON-RPC error, so the client never receives the bare `Error executing tool ...` result. Describe this as an unexpected exception other than `ToolError` or `MCPError`, including output-conversion failures, to avoid misdirecting users to the crash-log diagnosis.</violation>
</file>
<file name="src/mcp/server/mcpserver/exceptions.py">
<violation number="1" location="src/mcp/server/mcpserver/exceptions.py:48">
P2: This sentence includes `MCPError`, but protocol errors are re-raised rather than treated as crashes or logged. Describe unanticipated exceptions instead, while leaving `MCPError` as the protocol-error path.</violation>
<violation number="2" location="src/mcp/server/mcpserver/exceptions.py:64">
P2: `UnexpectedToolError` does not always have only the outer tool prefix. Nested wrappers append their generic message, so clients receive `Error executing tool outer: Error executing tool inner`; describe the message as generic, not exact.</violation>
</file>
<file name="docs/servers/handling-errors.md">
<violation number="1" location="docs/servers/handling-errors.md:128">
P2: The resource section omits the `INFO`/no-traceback behavior for anticipated `ResourceError` and incorrectly groups `MCPError` with crashes. State that `ResourceError` returns `-32603` and logs once at `INFO`, that `ResourceNotFoundError` remains `-32602`, and that `MCPError` passes through unchanged.
(Based on your team's feedback about documenting resource error outcomes.)</violation>
<violation number="2" location="docs/servers/handling-errors.md:139">
P2: This info box now says every statement on the page is client-visible, but the page also documents server-only `ERROR` records and tracebacks. Limit the sentence to client-facing behavior so the logging guidance is not presented as a client observation.</violation>
<violation number="3" location="docs/servers/handling-errors.md:153">
P2: The page now introduces `ResourceError` as the anticipated non-not-found resource failure, but the only import guidance omits it. Add `ResourceError` to the documented exception import so the guidance is usable.</violation>
</file>
<file name="src/mcp/server/mcpserver/server.py">
<violation number="1" location="src/mcp/server/mcpserver/server.py:434">
P2: When a tool validates a mapping argument, a caller-controlled key can contain a newline and forge additional log lines. Escape each location before joining it, while preserving the current output for ordinary field names.</violation>
</file>
<file name="src/mcp/server/mcpserver/resolve.py">
<violation number="1" location="src/mcp/server/mcpserver/resolve.py:580">
P2: When a legacy client returns a malformed `ElicitResult`, `session.elicit_form()` raises `ValidationError`, which this catch stringifies into the tool result. Handle `ValidationError` separately with a stable message, then retain the sanitized `ValueError` messages from `elicit_with_validation`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| The fix is in your client: **check `result.is_error`**. A `try/except` around `call_tool` catches none of these, because there is nothing to catch. This is deliberate, and it is the single most useful thing on this page to internalise: the *model* chose the call, so the model gets the message and a chance to try again. **[Handling errors](servers/handling-errors.md)** is the whole story, including the `MCPError` path that *does* raise. | ||
|
|
||
| The bare form, `Error executing tool <name>` with no message, means the tool **crashed**: it raised something other than `ToolError`, and the exception's text is kept off the wire. The traceback is in the **server's log** at `ERROR`, as `Tool '<name>' raised an unexpected exception`. |
There was a problem hiding this comment.
P2: When a tool raises MCPError, _handle_call_tool re-raises it as a JSON-RPC error, so the client never receives the bare Error executing tool ... result. Describe this as an unexpected exception other than ToolError or MCPError, including output-conversion failures, to avoid misdirecting users to the crash-log diagnosis.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/troubleshooting.md, line 95:
<comment>When a tool raises `MCPError`, `_handle_call_tool` re-raises it as a JSON-RPC error, so the client never receives the bare `Error executing tool ...` result. Describe this as an unexpected exception other than `ToolError` or `MCPError`, including output-conversion failures, to avoid misdirecting users to the crash-log diagnosis.</comment>
<file context>
@@ -92,7 +92,7 @@ result.structured_content # None
The fix is in your client: **check `result.is_error`**. A `try/except` around `call_tool` catches none of these, because there is nothing to catch. This is deliberate, and it is the single most useful thing on this page to internalise: the *model* chose the call, so the model gets the message and a chance to try again. **[Handling errors](servers/handling-errors.md)** is the whole story, including the `MCPError` path that *does* raise.
-If `<message>` alone doesn't tell you what broke and the tool crashed (rather than raising `ToolError`, being unknown, or rejecting an argument), the traceback is in the **server's log** at `ERROR`, as `Tool '<name>' raised an unexpected exception`.
+The bare form, `Error executing tool <name>` with no message, means the tool **crashed**: it raised something other than `ToolError`, and the exception's text is kept off the wire. The traceback is in the **server's log** at `ERROR`, as `Tool '<name>' raised an unexpected exception`.
## `TypeError: The @tool decorator was used incorrectly. Did you forget to call it? Use @tool() instead of @tool`
</file context>
| The bare form, `Error executing tool <name>` with no message, means the tool **crashed**: it raised something other than `ToolError`, and the exception's text is kept off the wire. The traceback is in the **server's log** at `ERROR`, as `Tool '<name>' raised an unexpected exception`. | |
| The bare form, `Error executing tool <name>` with no message, means an unexpected exception occurred while running or converting the tool result: it was neither `ToolError` nor `MCPError`, and its text is kept off the wire. The traceback is in the **server's log** at `ERROR`, as `Tool '<name>' raised an unexpected exception`. |
|
|
||
| Raise this from a tool (or a resolver) for a failure you saw coming: the | ||
| call returns `is_error=True` with your message in `content` for the model to | ||
| read, and the server logs it at INFO without a traceback. Any other exception |
There was a problem hiding this comment.
P2: This sentence includes MCPError, but protocol errors are re-raised rather than treated as crashes or logged. Describe unanticipated exceptions instead, while leaving MCPError as the protocol-error path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/mcpserver/exceptions.py, line 48:
<comment>This sentence includes `MCPError`, but protocol errors are re-raised rather than treated as crashes or logged. Describe unanticipated exceptions instead, while leaving `MCPError` as the protocol-error path.</comment>
<file context>
@@ -44,11 +44,11 @@ class ToolError(MCPServerError):
- A `ResourceError` that escapes the tool (say from `ctx.read_resource()`) counts
- as anticipated too.
+ call returns `is_error=True` with your message in `content` for the model to
+ read, and the server logs it at INFO without a traceback. Any other exception
+ is treated as a crash: the model sees only `Error executing tool <name>`, and
+ the server logs the traceback at ERROR. A `ResourceError` that escapes the tool
</file context>
| only `Error executing tool <name>`, so nothing from the original reaches the | ||
| client. `__cause__` is the original exception, which the server logs with its | ||
| traceback before returning the `is_error=True` result. Catch it around |
There was a problem hiding this comment.
P2: UnexpectedToolError does not always have only the outer tool prefix. Nested wrappers append their generic message, so clients receive Error executing tool outer: Error executing tool inner; describe the message as generic, not exact.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/mcpserver/exceptions.py, line 64:
<comment>`UnexpectedToolError` does not always have only the outer tool prefix. Nested wrappers append their generic message, so clients receive `Error executing tool outer: Error executing tool inner`; describe the message as generic, not exact.</comment>
<file context>
@@ -60,9 +60,10 @@ class UnexpectedToolError(ToolError):
- the original exception, which the server logs with its traceback before
- returning the usual `is_error=True` result. Catch it around
+ return value that fails output conversion. You never raise it. The message is
+ only `Error executing tool <name>`, so nothing from the original reaches the
+ client. `__cause__` is the original exception, which the server logs with its
+ traceback before returning the `is_error=True` result. Catch it around
</file context>
| only `Error executing tool <name>`, so nothing from the original reaches the | |
| client. `__cause__` is the original exception, which the server logs with its | |
| traceback before returning the `is_error=True` result. Catch it around | |
| a generic `Error executing tool <name>`; nested wrappers may append context, so | |
| the original exception text is withheld from the client. `__cause__` is the | |
| original exception, which the server logs with its traceback before returning the | |
| `is_error=True` result. Catch it around |
| @@ -116,18 +137,20 @@ It means a whole class of `raise` statements you don't write: don't re-validate | |||
|
|
|||
| !!! info | |||
| Everything on this page is what a **client** sees, and the in-memory `Client` you'll write | |||
There was a problem hiding this comment.
P2: This info box now says every statement on the page is client-visible, but the page also documents server-only ERROR records and tracebacks. Limit the sentence to client-facing behavior so the logging guidance is not presented as a client observation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/servers/handling-errors.md, line 139:
<comment>This info box now says every statement on the page is client-visible, but the page also documents server-only `ERROR` records and tracebacks. Limit the sentence to client-facing behavior so the logging guidance is not presented as a client observation.</comment>
<file context>
@@ -115,36 +136,21 @@ Send `get_author` a `title` that isn't a string and the SDK rejects it against t
-`ToolError` comes from `mcp.server.mcpserver.exceptions`. The model reads exactly what it read before. The difference is in your log, where a `ToolError` is a single `INFO` line with no traceback, so a production log at `WARNING` stays quiet until something is actually broken. Bad arguments and unknown tool names are logged at `INFO` too, because those are the caller's mistakes rather than yours.
-
-Resources work the same way. A crashing resource handler is logged at `ERROR` with its traceback, which matters more here because the `-32603` the client receives names only the URI. `ResourceNotFoundError` and `ResourceError` are the anticipated kind and are logged at `INFO`.
+ Everything on this page is what a **client** sees, and the in-memory `Client` you'll write
+ tests with sees exactly the same thing. Even `raise_exceptions=True` doesn't hand a failing
+ tool's exception back to the caller: by the time that flag could act, your exception is already
</file context>
| Everything on this page is what a **client** sees, and the in-memory `Client` you'll write | |
| The **client-facing behavior** described on this page is what a **client** sees, and the in-memory `Client` you'll write |
| * `ResourceNotFoundError` from a resource handler -> the protocol's `-32602`, with the URI in `data`. | ||
| * Bad arguments are rejected against the schema before your function runs; you don't `raise` for those. | ||
| * `from mcp import MCPError`; the error-code constants come from `mcp.types`. | ||
| * Imports: `from mcp import MCPError`, `from mcp.server.mcpserver.exceptions import ToolError, ResourceNotFoundError`, and the error-code constants from `mcp.types`. |
There was a problem hiding this comment.
P2: The page now introduces ResourceError as the anticipated non-not-found resource failure, but the only import guidance omits it. Add ResourceError to the documented exception import so the guidance is usable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/servers/handling-errors.md, line 153:
<comment>The page now introduces `ResourceError` as the anticipated non-not-found resource failure, but the only import guidance omits it. Add `ResourceError` to the documented exception import so the guidance is usable.</comment>
<file context>
@@ -115,36 +136,21 @@ Send `get_author` a `title` that isn't a string and the SDK rejects it against t
* `ResourceNotFoundError` from a resource handler -> the protocol's `-32602`, with the URI in `data`.
* Bad arguments are rejected against the schema before your function runs; you don't `raise` for those.
-* `from mcp import MCPError`; the error-code constants come from `mcp.types`.
+* Imports: `from mcp import MCPError`, `from mcp.server.mcpserver.exceptions import ToolError, ResourceNotFoundError`, and the error-code constants from `mcp.types`.
Errors handled. That is everything a server *exposes*. What every handler can read, and do back to the client while it runs, is the next section: **[Inside your handler](../handlers/index.md)**.
</file context>
| * Imports: `from mcp import MCPError`, `from mcp.server.mcpserver.exceptions import ToolError, ResourceNotFoundError`, and the error-code constants from `mcp.types`. | |
| * Imports: `from mcp import MCPError`, `from mcp.server.mcpserver.exceptions import ResourceError, ResourceNotFoundError, ToolError`, and the error-code constants from `mcp.types`. |
| ``` | ||
|
|
||
| Notice there is no `is_error=True` half-result here. A resource read either returns contents or fails: resources have only the protocol path. Templates and everything else about resources live in **[Resources](resources.md)**. | ||
| Notice there is no `is_error=True` half-result here. A resource read either returns contents or fails: resources have only the protocol path. `ResourceError` is the same thing for a failure that isn't "not found" (`-32603`, your message). Any other exception is a crash: the client gets `-32603` naming only the URI, and the traceback goes to your log at `ERROR`. Templates and everything else about resources live in **[Resources](resources.md)**. |
There was a problem hiding this comment.
P2: The resource section omits the INFO/no-traceback behavior for anticipated ResourceError and incorrectly groups MCPError with crashes. State that ResourceError returns -32603 and logs once at INFO, that ResourceNotFoundError remains -32602, and that MCPError passes through unchanged.
(Based on your team's feedback about documenting resource error outcomes.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/servers/handling-errors.md, line 128:
<comment>The resource section omits the `INFO`/no-traceback behavior for anticipated `ResourceError` and incorrectly groups `MCPError` with crashes. State that `ResourceError` returns `-32603` and logs once at `INFO`, that `ResourceNotFoundError` remains `-32602`, and that `MCPError` passes through unchanged.
(Based on your team's feedback about documenting resource error outcomes.) </comment>
<file context>
@@ -104,7 +125,7 @@ When it can't, raise `ResourceNotFoundError`. The SDK turns it into the protocol
-Notice there is no is_error=True half-result here. A resource read either returns contents or fails: resources have only the protocol path. Templates and everything else about resources live in Resources.
+Notice there is no is_error=True half-result here. A resource read either returns contents or fails: resources have only the protocol path. ResourceError is the same thing for a failure that isn't "not found" (-32603, your message). Any other exception is a crash: the client gets -32603 naming only the URI, and the traceback goes to your log at ERROR. Templates and everything else about resources live in Resources.
Errors you never raise
</file context>
</details>
```suggestion
Notice there is no `is_error=True` half-result here. A resource read either returns contents or fails: resources have only the protocol path. `ResourceError` is the same thing for a failure that isn't "not found" (`-32603`, your message) and logs one `INFO` line without a traceback. `ResourceNotFoundError` remains `-32602`, while `MCPError` passes through unchanged. Any other exception is a crash: the client gets `-32603` naming only the URI, and the traceback goes to your log at `ERROR`. Templates and everything else about resources live in **[Resources](resources.md)**.
| 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()}) | ||
| logger.info("Tool %r rejected arguments: %s", params.name, ", ".join(fields)) |
There was a problem hiding this comment.
P2: When a tool validates a mapping argument, a caller-controlled key can contain a newline and forge additional log lines. Escape each location before joining it, while preserving the current output for ordinary field names.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/mcpserver/server.py, line 434:
<comment>When a tool validates a mapping argument, a caller-controlled key can contain a newline and forge additional log lines. Escape each location before joining it, while preserving the current output for ordinary field names.</comment>
<file context>
@@ -427,9 +427,14 @@ async def _handle_call_tool(
+ 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()})
+ logger.info("Tool %r rejected arguments: %s", params.name, ", ".join(fields))
+ else:
+ # %r keeps peer-supplied text on one line.
</file context>
| logger.info("Tool %r rejected arguments: %s", params.name, ", ".join(fields)) | |
| logger.info("Tool %r rejected arguments: %s", params.name, ", ".join(repr(field)[1:-1] for field in fields)) |
| return await res.context.elicit(marker.message, marker.schema) | ||
| try: | ||
| return await res.context.elicit(marker.message, marker.schema) | ||
| except ValueError as e: |
There was a problem hiding this comment.
P2: When a legacy client returns a malformed ElicitResult, session.elicit_form() raises ValidationError, which this catch stringifies into the tool result. Handle ValidationError separately with a stable message, then retain the sanitized ValueError messages from elicit_with_validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/mcpserver/resolve.py, line 580:
<comment>When a legacy client returns a malformed `ElicitResult`, `session.elicit_form()` raises `ValidationError`, which this catch stringifies into the tool result. Handle `ValidationError` separately with a stable message, then retain the sanitized `ValueError` messages from `elicit_with_validation`.</comment>
<file context>
@@ -575,7 +575,12 @@ async def _fulfil(marker: _Marker, key: str, res: _Resolution) -> ElicitationRes
- return await res.context.elicit(marker.message, marker.schema)
+ try:
+ return await res.context.elicit(marker.message, marker.schema)
+ except ValueError as e:
+ # Accepted with no content, or content that fails the schema: the same
+ # client mistake the input_required path below reports as a ToolError.
</file context>
| except ValueError as e: | |
| except ValidationError as e: | |
| raise ToolError(f"Resolver {key!r} received an invalid elicitation response") from e | |
| except ValueError as e: |
| return CompleteResult( | ||
| completion=result if result is not None else Completion(values=[], total=None, has_more=None), | ||
| ) |
There was a problem hiding this comment.
🔴 The completion decorator's new crash classification wraps only await func(...) (lines 741-749); the CompleteResult(completion=result ...) construction at lines 750-752 is outside the try, so a completion handler that returns a wrong-typed value raises pydantic ValidationError there, which every dispatch boundary maps to -32602 "Invalid request parameters" with no server-side log (jsonrpc_dispatcher.py:98-101 returns ErrorData without logging; direct_dispatcher.py:269-272 does the same; runner.py on_request re-raises ValidationError to the same ladder). Pre-existing shape (the construction was always unprotected), but it now directly contradicts the contract this rewritten handler establishes: unexpected completion failures are supposed to be logged as "Completion for argument %r…
Extended reasoning...
A server author's @ mcp.completion() handler returns the wrong type — e.g. return ["bold", "italic"] or a dict instead of a Completion — a plausible authoring slip since the decorator's own docstring shows returning Completion(...) or None. CompleteResult(completion=["bold", "italic"], ...) raises pydantic ValidationError after the except ladder, so the client receives JSON-RPC -32602 "Invalid request parameters", telling it its completion/complete request was malformed when the request was fine, and the server log records nothing at all (the dispatcher maps ValidationError to the wire error without logging). The author sees clients failing with an invalid-params error, greps the log for the documented "Completion for argument ... raised an unexpected exception" ERROR record, finds nothing, and has no way to discover the real cause — exactly the silent-failure class this PR set out to eliminate, whereas raising the same bad value from inside func() would be logged with its traceback and returned as INTERNAL_ERROR "Error completing argument ".
Verification: normal — the failure is real and reachable through code this PR rewrote. src/mcp/server/mcpserver/server.py:741-752 (new in this diff) wraps only result = await func(params.ref, params.argument, params.context) in the crash-classifying try/except; the return CompleteResult(completion=result if result is not None else Completion(...)) at 750-752 is outside it. CompleteResult.completion is a s
| # 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()}) | ||
| logger.info("Tool %r rejected arguments: %s", params.name, ", ".join(fields)) |
There was a problem hiding this comment.
🔴 The new argument-rejection INFO log formats caller-controlled text with %s: fields is built from pydantic err["loc"] parts, and loc is not limited to schema-defined field names — for a dict[str, X] tool argument or a nested model with extra="forbid", loc contains the caller-supplied dictionary key verbatim. The sibling branch two lines down deliberately uses %r ("%r keeps peer-supplied text on one line"), but this branch joins the loc parts raw and passes them through %s, so peer-supplied keys containing newlines are written into the log unescaped. Fix: log the joined field list with %r (or repr each part), matching the other branch.
Extended reasoning...
A server exposes a tool like def record(scores: dict[str, int]). A malicious or buggy client calls it with {"scores": {"x\n2026-08-19 ERROR forged.line Something fake": "oops"}}. Validation fails with loc ('scores', 'x\n2026-08-19 ERROR ...'); _handle_call_tool takes the ValidationError branch at src/mcp/server/mcpserver/server.py:431-434 and emits logger.info("Tool %r rejected arguments: %s", ...), writing the caller's key — newline included — verbatim into the server log. The attacker thereby injects arbitrary forged log records (or corrupts line-oriented log parsing/alerting) at will, on every request, defeating the one-line guarantee the adjacent %r branch was written to provide. Same applies to extra_forbidden errors on nested models, where the alien caller-chosen key name lands in loc.
Verification: normal — src/mcp/server/mcpserver/server.py:433-434 (added by this diff) builds fields from pydantic err["loc"] parts and logs them via %s: logger.info("Tool %r rejected arguments: %s", params.name, ", ".join(fields)). The ValidationError reaching this branch is the caller-argument validation error from tools/base.py:150-153 (raise ToolError(...) from exc), and pydantic v2 loc tupl
|
|
||
| ```text | ||
| Error executing tool old_log: The logging capability is deprecated as of 2026-07-28 (SEP-2577). | ||
| mcp.MCPDeprecationWarning: The logging capability is deprecated as of 2026-07-28 (SEP-2577). |
There was a problem hiding this comment.
🟡 nit: The quoted server-log line mcp.MCPDeprecationWarning: The logging capability is deprecated... is not what the log will contain — a Python traceback renders the exception's real __module__.__qualname__, and MCPDeprecationWarning is defined in mcp.shared.exceptions with no __module__ override, so the captured log's final line reads mcp.shared.exceptions.MCPDeprecationWarning: ....
Extended reasoning...
A user follows the tip, runs pytest with error::mcp.MCPDeprecationWarning, and greps the captured server log for the documented string mcp.MCPDeprecationWarning: (or asserts on it in a test) — it never matches, because the ERROR record's traceback ends with mcp.shared.exceptions.MCPDeprecationWarning: The logging capability is deprecated as of 2026-07-28 (SEP-2577).; the accompanying test (tests/docs_src/test_deprecated.py) only checks type(...).__name__ and str(...), so the doc drift is not caught.
Verification: nit. docs/deprecated.md:123-126 (added by this diff) presents the block as literal captured server-log content — "the captured server log names the culprit:" followed by mcp.MCPDeprecationWarning: The logging capability is deprecated as of 2026-07-28 (SEP-2577). — but that string never appears in the log. MCPDeprecationWarning is defined at src/mcp/shared/exceptions.py:8 with no __module__
| result = await res.context.session.send_request( | ||
| _render_request(marker), | ||
| _result_type(marker), |
There was a problem hiding this comment.
🟣 The elicitation-mistake fix in _fulfil covers only the Elicit marker: on the legacy (non-input_required) path, a Sample/ListRoots resolver's session.send_request(_render_request(marker), _result_type(marker)) raises pydantic ValidationError when the client's response doesn't match the result type (documented in ServerSession.send_request, session.py:107), and no clause in _fulfil catches it, so Tool.run's final except Exception (tools/base.py:214-216) classifies this client mistake as an UnexpectedToolError crash — while the modern input_required path wraps the identical mistake as an anticipated ToolError at… [also at: src/mcp/server/mcpserver/server.py:1332 - Pre-existing (in code this diff touches): the prompt path was left out of the PR's anticipated-vs-crash classification.…]
Extended reasoning...
A tool declares a dependency Annotated[CreateMessageResult, Resolve(Sample(...))] and is called by a legacy-protocol client (no input_required flow) that advertises sampling. The client answers the sampling/createMessage request with a payload missing a required field (e.g. no model or malformed content — common for LLM-driven clients). send_request raises ValidationError; Tool.run turns it into UnexpectedToolError, so the server's production log records an ERROR with full traceback (Tool '<name>' raised an unexpected exception) — paging operators for a peer's bad response, defeating the PR's "log stays quiet at WARNING unless something is actually broken" goal — and the model receives only the generic Error executing tool <name> instead of the anticipated "Resolver 'x' received a response of the wrong kind" message the modern path returns for the exact same client mistake, so it cannot tell its own client-side answer was malformed.
Verification: pre_existing. src/mcp/server/session.py:107 documents "pydantic.ValidationError: The peer's result does not match result_type" (raised at line 122 result_type.model_validate(result, by_name=False)). On the legacy path in _fulfil, src/mcp/server/mcpserver/resolve.py:584-589 calls res.context.session.send_request(_render_request(marker), _result_type(marker), ...) with no handler — the PR's
2 participants
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 #3266. Fixes #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 (#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 (#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 (#3267 / #3271 / #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#4036, #4029, #4392 once deliberateToolErrors and model typos flooded error monitoring. #2422 and #2346 are the same signal here.Why withhold crash text: it's the call already made for resources (#1957) and prompts, it's what #698 / #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 #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 #3267, #3271, and #2198 (thank you all — the diagnoses were right; this moves the fix to where all three primitives share it). Related: #2153, #2386, #2422.
AI Disclaimer