From 2dff02287395741e33067385b87727ae91ed8c16 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Fri, 14 Aug 2026 12:57:22 +0000 Subject: [PATCH 1/8] Log MCPServer handler exceptions once, by kind 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. --- docs/handlers/logging.md | 2 + docs/migration.md | 2 +- docs/servers/handling-errors.md | 30 +- docs/servers/uri-templates.md | 9 +- docs/troubleshooting.md | 2 + docs_src/handling_errors/tutorial004.py | 14 + src/mcp/server/mcpserver/exceptions.py | 41 +- src/mcp/server/mcpserver/prompts/base.py | 4 +- .../server/mcpserver/resources/templates.py | 14 +- src/mcp/server/mcpserver/resources/types.py | 28 +- src/mcp/server/mcpserver/server.py | 68 ++- src/mcp/server/mcpserver/tools/base.py | 49 +- tests/docs_src/test_handling_errors.py | 47 +- tests/docs_src/test_troubleshooting.py | 10 + tests/interaction/_requirements.py | 23 + tests/interaction/mcpserver/test_prompts.py | 32 ++ tests/interaction/mcpserver/test_resources.py | 32 ++ tests/interaction/mcpserver/test_tools.py | 31 ++ .../resources/test_file_resources.py | 8 +- .../resources/test_function_resources.py | 20 +- tests/server/mcpserver/test_server.py | 523 +++++++++++++++++- 21 files changed, 915 insertions(+), 74 deletions(-) create mode 100644 docs_src/handling_errors/tutorial004.py diff --git a/docs/handlers/logging.md b/docs/handlers/logging.md index 6f6c839314..1c839d70b5 100644 --- a/docs/handlers/logging.md +++ b/docs/handlers/logging.md @@ -70,6 +70,8 @@ went to standard error: the terminal, not the wire. don't want log lines, you want spans. Your server already emits them: the SDK traces every message with OpenTelemetry out of the box. See **[OpenTelemetry](../run/opentelemetry.md)**. +You don't have to log your own handlers' crashes either. When a tool or resource function raises something unexpected, the SDK writes the `ERROR` record with the traceback for you, on its own `mcp.*` loggers; a failure you raised deliberately (`ToolError`, `ResourceNotFoundError`) is an `INFO` line instead. A prompt function that raises is an `ERROR` record too, whatever it raised. **[Handling errors](../servers/handling-errors.md#what-lands-in-your-log)** has the split. (In a test using `Client(mcp, raise_exceptions=True)`, a prompt failure is handed to your test as the exception rather than logged.) + ## Recap * The MCP protocol's logging capability is deprecated by the 2026-07-28 spec and not replaced. Don't build on it. diff --git a/docs/migration.md b/docs/migration.md index b094d79f84..e59b4a6ac6 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -1016,7 +1016,7 @@ except MCPError as e: ### Resource not found returns `-32602` and resource lookups raise typed exceptions (SEP-2164) -Reading a missing resource now returns JSON-RPC error code `-32602` (invalid params) with the requested URI in `error.data` (`{"uri": ...}`), per [SEP-2164](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2164). Previously the server returned code `0` with no `data`. Clients can now reliably distinguish not-found from other errors; a template handler that raises `ResourceNotFoundError` (from `mcp.server.mcpserver.exceptions`) produces this same response. +Reading a missing resource now returns JSON-RPC error code `-32602` (invalid params) with the requested URI in `error.data` (`{"uri": ...}`), per [SEP-2164](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2164). Previously the server returned code `0` with no `data`. Clients can now reliably distinguish not-found from other errors; a resource handler (static or template) that raises `ResourceNotFoundError` (from `mcp.server.mcpserver.exceptions`) produces this same response. The underlying lookups now raise typed exceptions instead of `ValueError`. `ResourceManager.get_resource()` raises `ResourceNotFoundError` when no resource or template matches the URI, and `ResourceTemplate.create_resource()` raises `ResourceError` when the template function fails. Neither subclasses `ValueError`, so callers catching `ValueError` should switch to `ResourceNotFoundError` / `ResourceError` (both importable from `mcp.server.mcpserver.exceptions`; `ResourceNotFoundError` subclasses `ResourceError`). diff --git a/docs/servers/handling-errors.md b/docs/servers/handling-errors.md index 4262f586a7..6519889312 100644 --- a/docs/servers/handling-errors.md +++ b/docs/servers/handling-errors.md @@ -115,10 +115,29 @@ Send `get_author` a `title` that isn't a string and the SDK rejects it against t It means a whole class of `raise` statements you don't write: don't re-validate your own type hints. !!! 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 turn a tool error - back into a traceback: by the time that flag could act, your exception is already the - `is_error=True` result. Assert on the result. **[Testing](../get-started/testing.md)** covers the pattern. + Everything so far 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 the + `is_error=True` result. Assert on the result; the traceback is in the server's log (next + section), which pytest's `caplog` captures. **[Testing](../get-started/testing.md)** covers the pattern. + +## What lands in your log + +Your server keeps its own record of these failures, and it draws one more line: between a failure you anticipated and one you didn't. + +`get_author` raised a plain `ValueError`. The model got the message, but the SDK can't know you *meant* that exception, so it assumes you didn't: the call is logged at `ERROR` with the full traceback. That is exactly what you want on the day the exception is a `KeyError` from three libraries down and the result text says only `'id'`. + +When the failure is one you planned for, say so with `ToolError`: + +```python title="server.py" hl_lines="2 12-13" +--8<-- "docs_src/handling_errors/tutorial004.py" +``` + +The model reads precisely what it read before. The difference is on your side: a `ToolError` is logged as one `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 `INFO` lines too; those are the caller's mistakes, not yours. + +Resources draw the same line. The `-32603` from a crashing resource handler names only the URI, so the `ERROR` record in your log is the one place the cause and its traceback exist. `ResourceNotFoundError`, including the SDK's own `Unknown resource`, is an `INFO` line. (A template parameter that fails its type annotation, `books://{id}` read with an `id` that isn't an `int`, currently counts as a crash.) + +Prompts aren't split yet: any failure in a prompt function, including an unknown name or a missing argument, is one `ERROR` record with its traceback, written by the transport layer that turns it into the JSON-RPC error. ## Recap @@ -127,7 +146,8 @@ It means a whole class of `raise` statements you don't write: don't re-validate * The deciding question: *could a smarter model have avoided this?* Yes -> exception. No -> `MCPError`. * `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`. +* In your log: an exception you didn't raise as `ToolError` is an `ERROR` record with its traceback; `ToolError`, bad tool arguments, unknown tool names, and `ResourceNotFoundError` are one `INFO` line each. +* `from mcp import MCPError`; `ToolError` and `ResourceNotFoundError` come from `mcp.server.mcpserver.exceptions`; the error-code constants come 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)**. diff --git a/docs/servers/uri-templates.md b/docs/servers/uri-templates.md index 406a8fda6a..a79889b2af 100644 --- a/docs/servers/uri-templates.md +++ b/docs/servers/uri-templates.md @@ -199,10 +199,11 @@ These checks are a heuristic pre-filter; for filesystem access, `safe_join` remains the containment boundary. !!! tip - If your handler can't fulfil the request (the file doesn't exist, - the id is unknown), raise an exception. The SDK turns it into an - error response. See **[Handling errors](handling-errors.md)** for the difference between a - protocol error and a tool error. + If your handler can't fulfil the request (the file doesn't exist, the id is unknown), raise + `ResourceNotFoundError` from `mcp.server.mcpserver.exceptions`. The client gets `-32602` with + your message and the URI, and your log gets one `INFO` line; any other exception is treated as + a crash (`-32603`, and an `ERROR` record with the traceback). See + **[Handling errors](handling-errors.md#a-resource-that-doesnt-exist)**. ## Resources on the low-level Server diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 75a6652ecc..f549dfbadd 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -92,6 +92,8 @@ 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 `` alone doesn't tell you what broke, the traceback is in the **server's log**: an exception the tool didn't raise as `ToolError` is logged there at `ERROR`, as `Tool '' raised an unexpected exception`. + ## `TypeError: The @tool decorator was used incorrectly. Did you forget to call it? Use @tool() instead of @tool` You wrote `@mcp.tool` instead of `@mcp.tool()`. `tool()` is a decorator *factory*: without the parentheses, Python hands your function to its `name=` parameter. diff --git a/docs_src/handling_errors/tutorial004.py b/docs_src/handling_errors/tutorial004.py new file mode 100644 index 0000000000..9676a10075 --- /dev/null +++ b/docs_src/handling_errors/tutorial004.py @@ -0,0 +1,14 @@ +from mcp.server import MCPServer +from mcp.server.mcpserver.exceptions import ToolError + +mcp = MCPServer("Bookshop") + +CATALOG = {"Dune": "Frank Herbert", "Neuromancer": "William Gibson"} + + +@mcp.tool() +def get_author(title: str) -> str: + """Look up the author of a book in the catalog.""" + if title not in CATALOG: + raise ToolError(f"No book titled {title!r} in the catalog.") + return CATALOG[title] diff --git a/src/mcp/server/mcpserver/exceptions.py b/src/mcp/server/mcpserver/exceptions.py index 239785e9a9..9f2415d976 100644 --- a/src/mcp/server/mcpserver/exceptions.py +++ b/src/mcp/server/mcpserver/exceptions.py @@ -6,20 +6,55 @@ class MCPServerError(Exception): class ResourceError(MCPServerError): - """Error in resource operations.""" + """Error in resource operations. + + When a resource or resource template handler raises this, its message reaches + the client as a `-32603` protocol error. + """ class ResourceNotFoundError(ResourceError): """Resource does not exist. - Raise this from a resource template handler to signal that the requested instance does not exist; + Raise this from a resource handler to signal that the requested instance does not exist; clients receive `-32602` (invalid params) per [SEP-2164](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2164). """ +class UnexpectedResourceError(ResourceError): + """A resource read failed with something other than `ResourceError` or `MCPError`. + + MCPServer raises this itself, around a crash in a resource or resource + template handler or a failed file read; you never raise it. `__cause__` is + the original exception, which the server logs with its traceback. The + message names only the URI, so the original text is withheld from the client. + """ + + class ToolError(MCPServerError): - """Error in tool operations.""" + """A tool failure the model should read. + + Raise this from a tool (or a resolver) for a failure you anticipate: the + call returns `is_error=True` with the message in `content`, and the server + logs it at INFO without a traceback. Any other exception reaches the model + the same way but is treated as a crash and logged at ERROR with its traceback. + + The SDK raises it too, for an unknown tool name and for arguments that fail + the input schema, and `UnexpectedToolError` subclasses it, so `except ToolError` + around `MCPServer.call_tool()` catches every tool failure, crash or not. + """ + + +class UnexpectedToolError(ToolError): + """A tool call failed with something other than `ToolError` or `MCPError`. + + MCPServer raises this itself, around a crash in the tool (or a resolver) or a + return value that fails output conversion; you never raise it. `__cause__` is + the original exception, which the server logs with its traceback before + returning the usual `is_error=True` result. Catch it around + `MCPServer.call_tool()` to tell a crash from a deliberate `ToolError`. + """ class InvalidSignature(Exception): diff --git a/src/mcp/server/mcpserver/prompts/base.py b/src/mcp/server/mcpserver/prompts/base.py index d30c0b3c60..7028dd8f0b 100644 --- a/src/mcp/server/mcpserver/prompts/base.py +++ b/src/mcp/server/mcpserver/prompts/base.py @@ -209,5 +209,5 @@ async def render( return messages except MCPError: raise - except Exception as e: - raise ValueError(f"Error rendering prompt {self.name}: {e}") + except Exception as exc: + raise ValueError(f"Error rendering prompt {self.name}: {exc}") from exc diff --git a/src/mcp/server/mcpserver/resources/templates.py b/src/mcp/server/mcpserver/resources/templates.py index 2ea99c19b6..54e50de4e4 100644 --- a/src/mcp/server/mcpserver/resources/templates.py +++ b/src/mcp/server/mcpserver/resources/templates.py @@ -11,18 +11,15 @@ from mcp_types import Annotations, Icon, InputRequiredResult from pydantic import BaseModel, Field, validate_call -from mcp.server.mcpserver.exceptions import ResourceError +from mcp.server.mcpserver.exceptions import ResourceError, UnexpectedResourceError from mcp.server.mcpserver.resources.types import FunctionResource, Resource from mcp.server.mcpserver.utilities.context_injection import find_context_parameter, inject_context from mcp.server.mcpserver.utilities.func_metadata import func_metadata -from mcp.server.mcpserver.utilities.logging import get_logger from mcp.shared._callable_inspection import is_async_callable from mcp.shared.exceptions import MCPError from mcp.shared.path_security import contains_path_traversal, is_absolute_path from mcp.shared.uri_template import UriTemplate -logger = get_logger(__name__) - if TYPE_CHECKING: from mcp.server.context import LifespanContextT, RequestT from mcp.server.mcpserver.context import Context @@ -217,7 +214,9 @@ async def create_resource( carrying the echoed opaque state. Raises: - ResourceError: If creating the resource fails. + ResourceError: If the template function raises `ResourceError`. + UnexpectedResourceError: If the template function raises anything other + than `ResourceError` or `MCPError`; `__cause__` is the original. """ try: # Add context to params if needed @@ -246,5 +245,6 @@ async def create_resource( except (ResourceError, MCPError): raise except Exception as exc: - logger.exception(f"Error creating resource from template {uri}") - raise ResourceError(f"Error creating resource from template {uri}") from exc + # Name only the URI: the original text is withheld from the client, and + # the server logs the traceback from `__cause__`. + raise UnexpectedResourceError(f"Error creating resource from template {uri}") from exc diff --git a/src/mcp/server/mcpserver/resources/types.py b/src/mcp/server/mcpserver/resources/types.py index 2edf342337..f3751989d7 100644 --- a/src/mcp/server/mcpserver/resources/types.py +++ b/src/mcp/server/mcpserver/resources/types.py @@ -16,6 +16,7 @@ from mcp_types import Annotations, Icon, InputRequiredResult from pydantic import Field, validate_call +from mcp.server.mcpserver.exceptions import ResourceError, UnexpectedResourceError from mcp.server.mcpserver.resources.base import Resource from mcp.shared._callable_inspection import is_async_callable from mcp.shared.exceptions import MCPError @@ -79,7 +80,12 @@ class FunctionResource(Resource): fn: Callable[[], Any] = Field(exclude=True) async def read(self) -> str | bytes: - """Read the resource by calling the wrapped function.""" + """Read the resource by calling the wrapped function. + + Raises: + UnexpectedResourceError: If the function raises anything other than + `ResourceError` or `MCPError`; `__cause__` is the original. + """ try: fn = self.fn if is_async_callable(fn): @@ -103,10 +109,12 @@ async def read(self) -> str | bytes: return result else: return pydantic_core.to_json(result, fallback=str, indent=2).decode() - except MCPError: + except (MCPError, ResourceError): raise - except Exception as e: - raise ValueError(f"Error reading resource {self.uri}: {e}") + except Exception as exc: + # Name only the URI: the original text is withheld from the client, and + # the server logs the traceback from `__cause__`. + raise UnexpectedResourceError(f"Error reading resource {self.uri}") from exc @classmethod def from_function( @@ -187,8 +195,8 @@ async def read(self) -> str | bytes: if self.encoding is None: return await anyio.to_thread.run_sync(self.path.read_bytes) return await anyio.to_thread.run_sync(partial(self.path.read_text, encoding=self.encoding)) - except Exception as e: - raise ValueError(f"Error reading file {self.path}: {e}") + except Exception as exc: + raise UnexpectedResourceError(f"Error reading resource {self.uri}") from exc class HttpResource(Resource): @@ -232,8 +240,8 @@ def list_files(self) -> list[Path]: # pragma: no cover if self.pattern: return list(self.path.glob(self.pattern)) if not self.recursive else list(self.path.rglob(self.pattern)) return list(self.path.glob("*")) if not self.recursive else list(self.path.rglob("*")) - except Exception as e: - raise ValueError(f"Error listing directory {self.path}: {e}") + except Exception as exc: + raise ValueError(f"Error listing directory {self.path}: {exc}") from exc async def read(self) -> str: # Always returns JSON string # pragma: no cover """Read the directory listing.""" @@ -241,5 +249,5 @@ async def read(self) -> str: # Always returns JSON string # pragma: no cover files = await anyio.to_thread.run_sync(self.list_files) file_list = [str(f.relative_to(self.path)) for f in files if f.is_file()] return json.dumps({"files": file_list}, indent=2) - except Exception as e: - raise ValueError(f"Error reading directory {self.path}: {e}") + except Exception as exc: + raise UnexpectedResourceError(f"Error reading resource {self.uri}") from exc diff --git a/src/mcp/server/mcpserver/server.py b/src/mcp/server/mcpserver/server.py index 70e45329c5..5bced230fe 100644 --- a/src/mcp/server/mcpserver/server.py +++ b/src/mcp/server/mcpserver/server.py @@ -71,7 +71,13 @@ from mcp.server.lowlevel.server import LifespanResultT, Server from mcp.server.lowlevel.server import lifespan as default_lifespan from mcp.server.mcpserver.context import Context -from mcp.server.mcpserver.exceptions import ResourceError, ResourceNotFoundError +from mcp.server.mcpserver.exceptions import ( + ResourceError, + ResourceNotFoundError, + ToolError, + UnexpectedResourceError, + UnexpectedToolError, +) from mcp.server.mcpserver.prompts import Prompt, PromptManager from mcp.server.mcpserver.resources import ( DEFAULT_RESOURCE_SECURITY, @@ -420,8 +426,9 @@ async def _handle_call_tool( return await self.call_tool(params.name, params.arguments or {}, context) except MCPError: raise - except Exception as e: - return CallToolResult(content=[TextContent(type="text", text=str(e))], is_error=True) + except Exception as exc: + _log_handler_exception("Tool", params.name, exc) + return CallToolResult(content=[TextContent(type="text", text=str(exc))], is_error=True) async def _handle_list_resources( self, ctx: ServerRequestContext[LifespanResultT], params: PaginatedRequestParams | None @@ -434,10 +441,10 @@ async def _handle_read_resource( context = Context(request_context=ctx, mcp_server=self, input_params=params, subscriptions=self._subscriptions) try: results = await self.read_resource(params.uri, context) - except ResourceNotFoundError as err: - raise MCPError(code=INVALID_PARAMS, message=str(err), data={"uri": str(params.uri)}) except ResourceError as err: - raise MCPError(code=INTERNAL_ERROR, message=str(err), data={"uri": str(params.uri)}) + _log_handler_exception("Resource", str(params.uri), err) + code = INVALID_PARAMS if isinstance(err, ResourceNotFoundError) else INTERNAL_ERROR + raise MCPError(code=code, message=str(err), data={"uri": str(params.uri)}) if isinstance(results, InputRequiredResult): return results contents: list[TextResourceContents | BlobResourceContents] = [] @@ -498,7 +505,15 @@ async def list_tools(self) -> list[MCPTool]: async def call_tool( self, name: str, arguments: dict[str, Any], context: Context[LifespanResultT, Any] | None = None ) -> CallToolResult | InputRequiredResult: - """Call a tool by name with arguments.""" + """Call a tool by name with arguments. + + Raises: + ToolError: If the tool is unknown, the arguments fail validation, or the + tool (or a resolver) raises `ToolError`. + UnexpectedToolError: If the tool (or a resolver) raises anything other than + `ToolError` or `MCPError`, or its return value fails output conversion; + `__cause__` is the original. + """ if context is None: context = Context(mcp_server=self, subscriptions=self._subscriptions) return await self._tool_manager.call_tool(name, arguments, context, convert_result=True) @@ -549,7 +564,10 @@ async def read_resource( Raises: ResourceNotFoundError: If no resource or template matches the URI. - ResourceError: If template creation or resource reading fails. + ResourceError: If the resource or template function raises `ResourceError`. + UnexpectedResourceError: If reading the resource (or creating it from a + template) raises anything other than `ResourceError` or `MCPError`; + `__cause__` is the original. """ if context is None: context = Context(mcp_server=self, subscriptions=self._subscriptions) @@ -560,12 +578,14 @@ async def read_resource( try: content = await resource.read() return [ReadResourceContents(content=content, mime_type=resource.mime_type, meta=resource.meta)] - except MCPError: + except (MCPError, ResourceError): + # Includes the UnexpectedResourceError the built-in resource types raise + # around a crash in the function or the file read. raise except Exception as exc: - logger.exception(f"Error getting resource {uri}") - # If an exception happens when reading the resource, we should not leak the exception to the client. - raise ResourceError(f"Error reading resource {uri}") from exc + # A custom Resource subclass whose read() raised: wrap it the same way, + # naming only the URI so the original text is withheld from the client. + raise UnexpectedResourceError(f"Error reading resource {uri}") from exc def add_tool( self, @@ -1293,10 +1313,32 @@ async def get_prompt( except MCPError: raise except Exception as e: - logger.exception(f"Error getting prompt {name}") + # Not logged here: this escapes `_handle_get_prompt` as-is, so the + # dispatcher boundary that turns it into the JSON-RPC error logs it once + # with its traceback (or, in-process with `raise_exceptions=True`, + # hands it to the caller instead). raise ValueError(str(e)) from e +def _log_handler_exception(kind: Literal["Tool", "Resource"], name: str, exc: Exception) -> None: + """Record a tool or resource handler failure; the one place MCPServer logs them. + + Called from the `except` block that turns the failure into a response. A + `ToolError` or `ResourceError` (deliberate, an unknown name, arguments that + failed validation, `ResourceNotFoundError`) is an anticipated outcome the + client already receives in full: one INFO record, no traceback, the text + repr-quoted so peer-supplied names and newlines stay on one line. Anything + else, including the `Unexpected*` wrappers whose `__cause__` is what the + handler actually raised, is a crash in user code: ERROR with the traceback. + """ + if isinstance(exc, ToolError | ResourceError) and not isinstance( + exc, UnexpectedToolError | UnexpectedResourceError + ): + logger.info("%s %r failed: %r", kind, name, str(exc)) + else: + logger.exception("%s %r raised an unexpected exception", kind, name, exc_info=exc) + + def _version_gated(method: MethodBinding) -> RequestHandler: """Wrap a method handler so a request at a disallowed protocol version is rejected. diff --git a/src/mcp/server/mcpserver/tools/base.py b/src/mcp/server/mcpserver/tools/base.py index 23248707a3..cd556e9726 100644 --- a/src/mcp/server/mcpserver/tools/base.py +++ b/src/mcp/server/mcpserver/tools/base.py @@ -5,9 +5,9 @@ from typing import TYPE_CHECKING, Any from mcp_types import Icon, InputRequiredResult, ToolAnnotations -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, ValidationError -from mcp.server.mcpserver.exceptions import InvalidSignature, ToolError +from mcp.server.mcpserver.exceptions import InvalidSignature, ToolError, UnexpectedToolError from mcp.server.mcpserver.resolve import ( build_resolver_plans, find_resolved_parameters, @@ -128,21 +128,33 @@ async def run( ) -> Any: """Run the tool with arguments. + Every failure other than `MCPError` is raised with its message prefixed + `Error executing tool : `, and `__cause__` set to what was raised. + Raises: - ToolError: If the tool function raises during execution. + ToolError: If the arguments fail validation against the input schema, or + the tool function (or a resolver) raises `ToolError`. + UnexpectedToolError: If the tool function (or a resolver) raises anything + other than `ToolError` or `MCPError`, or its return value fails output + conversion. """ + try: + validated = self.fn_metadata.validate_arguments(arguments) + except ValidationError as exc: + # The caller's arguments don't match the input schema. That is the model's + # mistake to read and correct, so it is reported like a deliberate ToolError. + raise ToolError(f"Error executing tool {self.name}: {exc}") from exc + try: pass_directly: dict[str, Any] = {} if self.context_kwarg is not None: pass_directly[self.context_kwarg] = context - # Resolvers see the same validated arguments the tool body receives: - # validate once and reuse it, so a `default_factory`/stateful validator - # can't hand a by-name resolver a different value than the body. - pre_validated: dict[str, Any] | None = None + # Resolvers see the same validated arguments the tool body receives, so a + # `default_factory`/stateful validator can't hand a by-name resolver a + # different value than the body. if self.resolved_params: - pre_validated = self.fn_metadata.validate_arguments(arguments) - resolved = await resolve_arguments(self.resolved_params, self.resolver_plans, pre_validated, context) + resolved = await resolve_arguments(self.resolved_params, self.resolver_plans, validated, context) if isinstance(resolved, InputRequiredResult): # A resolver still needs client input (>= 2026-07-28): surface the # batched questions instead of running the tool body this round. @@ -154,13 +166,14 @@ async def run( self.is_async, arguments, pass_directly or None, - pre_validated=pre_validated, + pre_validated=validated, ) # Registration rejects the annotated form of this combination; this covers - # a body that returns an InputRequiredResult without declaring it. + # a body that returns an InputRequiredResult without declaring it. It is + # an authoring bug, so it is raised as a crash rather than a ToolError. if self.resolved_params and isinstance(result, InputRequiredResult): - raise ToolError( + raise RuntimeError( "the tool returned an InputRequiredResult but its parameters use Resolve(...); " "a call has one input_required channel, so the multi-round flow is driven " "either by resolvers or by the tool body, not both" @@ -177,5 +190,13 @@ async def run( # it as a top-level JSON-RPC error rather than wrapping it as a # `CallToolResult(isError=True)` execution failure. raise - except Exception as e: - raise ToolError(f"Error executing tool {self.name}: {e}") from e + # Everything else reaches the model as an is_error result under this tool's + # name. The wrapper's type is what tells the server whether to log a crash. + except UnexpectedToolError as exc: + # A nested tool call crashed: still a crash under this tool's name. + raise UnexpectedToolError(f"Error executing tool {self.name}: {exc}") from exc + except ToolError as exc: + # Raised deliberately by the tool or a resolver: anticipated. + raise ToolError(f"Error executing tool {self.name}: {exc}") from exc + except Exception as exc: + raise UnexpectedToolError(f"Error executing tool {self.name}: {exc}") from exc diff --git a/tests/docs_src/test_handling_errors.py b/tests/docs_src/test_handling_errors.py index 0c2629169c..e2aab0e423 100644 --- a/tests/docs_src/test_handling_errors.py +++ b/tests/docs_src/test_handling_errors.py @@ -1,9 +1,11 @@ """`docs/servers/handling-errors.md`: every claim the page makes, proved against the real SDK.""" +import logging + import pytest from mcp_types import INVALID_PARAMS, ErrorData, TextContent, TextResourceContents -from docs_src.handling_errors import tutorial001, tutorial002, tutorial003 +from docs_src.handling_errors import tutorial001, tutorial002, tutorial003, tutorial004 from mcp import Client, MCPError # See test_index.py for why this is a per-module mark and not a conftest hook. @@ -68,7 +70,8 @@ async def test_resource_not_found_error_maps_to_invalid_params() -> None: async def test_raise_exceptions_does_not_turn_a_tool_error_into_a_traceback() -> None: - """The closing `!!! info`: even `raise_exceptions=True` leaves a failing tool as the `is_error=True` result.""" + """The `!!! info` before the log section: even `raise_exceptions=True` leaves a failing tool as the + `is_error=True` result.""" async with Client(tutorial001.mcp, raise_exceptions=True) as client: result = await client.call_tool("get_author", {"title": "Nothing"}) assert result.is_error @@ -84,3 +87,43 @@ async def test_a_title_the_template_knows_reads_normally() -> None: (contents,) = result.contents assert isinstance(contents, TextResourceContents) assert contents.text == "Dune by Frank Herbert" + + +async def test_a_plain_exception_is_logged_as_a_crash_with_its_traceback(caplog: pytest.LogCaptureFixture) -> None: + """tutorial001, "What lands in your log": the `ValueError` is one ERROR record carrying the traceback.""" + caplog.set_level(logging.INFO) + async with Client(tutorial001.mcp) as client: + await client.call_tool("get_author", {"title": "Nothing"}) + (record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert record.levelno == logging.ERROR + assert record.exc_info is not None + logged = record.exc_info[1] + assert logged is not None and isinstance(logged.__cause__, ValueError) + assert str(logged.__cause__) == "No book titled 'Nothing' in the catalog." + + +async def test_tool_error_reads_the_same_to_the_model_and_logs_one_info_line( + caplog: pytest.LogCaptureFixture, +) -> None: + """tutorial004: swapping in `ToolError` leaves the result byte-identical and the log at INFO, no traceback.""" + caplog.set_level(logging.INFO) + async with Client(tutorial004.mcp) as client: + result = await client.call_tool("get_author", {"title": "Nothing"}) + assert result.is_error + assert result.content == [ + TextContent(type="text", text="Error executing tool get_author: No book titled 'Nothing' in the catalog.") + ] + records = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert [(r.levelno, r.exc_info) for r in records] == [(logging.INFO, None)] + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + +async def test_a_bad_argument_is_an_info_line_not_a_crash(caplog: pytest.LogCaptureFixture) -> None: + """ "What lands in your log": schema rejection of the arguments is logged at INFO with no traceback.""" + caplog.set_level(logging.INFO) + async with Client(tutorial001.mcp) as client: + result = await client.call_tool("get_author", {"title": 42}) + assert result.is_error + records = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert [(r.levelno, r.exc_info) for r in records] == [(logging.INFO, None)] + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] diff --git a/tests/docs_src/test_troubleshooting.py b/tests/docs_src/test_troubleshooting.py index 9c94b643c1..7d79e21b5e 100644 --- a/tests/docs_src/test_troubleshooting.py +++ b/tests/docs_src/test_troubleshooting.py @@ -83,6 +83,16 @@ async def test_a_failing_tool_returns_is_error_true_instead_of_raising() -> None ] +async def test_a_failing_tool_leaves_its_traceback_in_the_server_log(caplog: pytest.LogCaptureFixture) -> None: + """The `Error executing tool` entry's pointer to the server log: the exact ERROR message it names.""" + with caplog.at_level(logging.ERROR, logger="mcp.server.mcpserver.server"): + async with Client(tutorial001.mcp) as client: + await client.call_tool("forecast", {"city": "Atlantis"}) + (record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert record.getMessage() == "Tool 'forecast' raised an unexpected exception" + assert record.exc_info is not None + + async def test_an_unknown_tool_is_the_same_kind_of_result() -> None: """`Unknown tool: ` travels the same `is_error=True` path as a failing tool.""" async with Client(tutorial001.mcp) as client: diff --git a/tests/interaction/_requirements.py b/tests/interaction/_requirements.py index 86725bcb4f..56a970c808 100644 --- a/tests/interaction/_requirements.py +++ b/tests/interaction/_requirements.py @@ -1020,6 +1020,14 @@ def __post_init__(self) -> None: "tool result with isError true and the failure text in content; it does not become a JSON-RPC error." ), ), + "mcpserver:tool:handler-throws:logged": Requirement( + source="sdk", + behavior=( + "An exception other than ToolError raised by a tool function is logged server-side exactly once, " + "at ERROR with its traceback, before the isError result is returned; the transport does not change " + "how many records are written." + ), + ), "mcpserver:tool:input-validation": Requirement( source=f"{SPEC_BASE_URL}/server/tools#error-handling", behavior=( @@ -1311,6 +1319,13 @@ def __post_init__(self) -> None: "(-32603 Internal error), with the original exception text withheld." ), ), + "mcpserver:resource:read-throws:logged": Requirement( + source="sdk", + behavior=( + "The exception withheld from the -32603 response is logged server-side exactly once, at ERROR " + "with its traceback; the transport does not change how many records are written." + ), + ), "mcpserver:resource:static": Requirement( source="sdk", behavior=( @@ -1427,6 +1442,14 @@ def __post_init__(self) -> None: source="sdk", behavior="A prompt with optional arguments can be fetched without supplying them.", ), + "mcpserver:prompt:render-throws:logged": Requirement( + source="sdk", + behavior=( + "An exception raised by a prompt function is logged server-side exactly once, at ERROR with its " + "traceback, by whichever layer turns it into the JSON-RPC error; the transport does not change how " + "many records are written." + ), + ), "mcpserver:prompt:unknown-name": Requirement( source=f"{SPEC_BASE_URL}/server/prompts#error-handling", behavior="prompts/get for a name that was never registered returns JSON-RPC error -32602 (Invalid params).", diff --git a/tests/interaction/mcpserver/test_prompts.py b/tests/interaction/mcpserver/test_prompts.py index 8409e50207..5a357592ae 100644 --- a/tests/interaction/mcpserver/test_prompts.py +++ b/tests/interaction/mcpserver/test_prompts.py @@ -1,5 +1,7 @@ """Prompt interactions against MCPServer, driven through the public Client API.""" +import logging + import pytest from inline_snapshot import snapshot from mcp_types import ( @@ -141,6 +143,36 @@ def repeat(phrase: str, count: int) -> str: assert exc_info.value.error.message.startswith("Error rendering prompt repeat: 1 validation error") +@requirement("mcpserver:prompt:render-throws:logged") +async def test_get_prompt_function_exception_is_logged_once_with_its_traceback( + connect: Connect, caplog: pytest.LogCaptureFixture +) -> None: + """An exception raised by a prompt function is logged exactly once, at ERROR, with its traceback. + + MCPServer lets the failure escape to the dispatcher boundary, which owns both the JSON-RPC error and + the log record; the owning logger therefore differs by transport, but the count must not. + """ + mcp = MCPServer("prompter") + raised = RuntimeError("template store unreachable") + + @mcp.prompt() + def briefing() -> str: + raise raised + + caplog.set_level(logging.ERROR) + async with connect(mcp) as client: + with pytest.raises(MCPError): + await client.get_prompt("briefing") + + def chains_to_raised(exc: BaseException | None) -> bool: + while exc is not None and exc is not raised: + exc = exc.__cause__ or exc.__context__ + return exc is raised + + records = [r for r in caplog.records if r.exc_info and chains_to_raised(r.exc_info[1])] + assert [r.levelname for r in records] == ["ERROR"] + + @requirement("mcpserver:prompt:optional-args") async def test_get_prompt_with_an_optional_argument_omitted_uses_the_default( connect: Connect, unstamped: Unstamp diff --git a/tests/interaction/mcpserver/test_resources.py b/tests/interaction/mcpserver/test_resources.py index eadf4794e6..162fa9a813 100644 --- a/tests/interaction/mcpserver/test_resources.py +++ b/tests/interaction/mcpserver/test_resources.py @@ -1,5 +1,7 @@ """Resource interactions against MCPServer, driven through the public Client API.""" +import logging + import pytest from inline_snapshot import snapshot from mcp_types import ( @@ -152,6 +154,36 @@ def boom() -> str: ) +@requirement("mcpserver:resource:read-throws:logged") +async def test_resource_function_exception_is_logged_once_with_its_traceback( + connect: Connect, caplog: pytest.LogCaptureFixture +) -> None: + """The exception withheld from the -32603 response is logged exactly once, at ERROR, with its traceback. + + The client sees only the URI, so this record is the operator's only route to the cause; it must be + written on every transport and never duplicated by a dispatcher boundary. + """ + mcp = MCPServer("library") + raised = RuntimeError("nope") + + @mcp.resource("res://boom") + def boom() -> str: + raise raised + + caplog.set_level(logging.ERROR) + async with connect(mcp) as client: + with pytest.raises(MCPError): + await client.read_resource("res://boom") + + def chains_to_raised(exc: BaseException | None) -> bool: + while exc is not None and exc is not raised: + exc = exc.__cause__ or exc.__context__ + return exc is raised + + records = [r for r in caplog.records if r.exc_info and chains_to_raised(r.exc_info[1])] + assert [(r.name, r.levelname) for r in records] == [("mcp.server.mcpserver.server", "ERROR")] + + @requirement("mcpserver:resource:duplicate-name") async def test_registering_a_duplicate_resource_uri_warns_and_keeps_the_first( connect: Connect, unstamped: Unstamp diff --git a/tests/interaction/mcpserver/test_tools.py b/tests/interaction/mcpserver/test_tools.py index a6418ac9c5..c275fc60b1 100644 --- a/tests/interaction/mcpserver/test_tools.py +++ b/tests/interaction/mcpserver/test_tools.py @@ -119,6 +119,37 @@ def flux() -> str: ) +@requirement("mcpserver:tool:handler-throws:logged") +async def test_call_tool_function_exception_is_logged_once_with_its_traceback( + connect: Connect, caplog: pytest.LogCaptureFixture +) -> None: + """The exception behind an is_error result is logged exactly once, at ERROR, with its traceback. + + The result text carries only `str(exc)`; the traceback exists nowhere but this record, so it must + be written on every transport and never duplicated by a dispatcher boundary. + """ + mcp = MCPServer("errors") + raised = LookupError("no such row") + + @mcp.tool() + def explode() -> str: + raise raised + + caplog.set_level(logging.ERROR) + async with connect(mcp) as client: + result = await client.call_tool("explode", {}) + + assert result.is_error is True + + def chains_to_raised(exc: BaseException | None) -> bool: + while exc is not None and exc is not raised: + exc = exc.__cause__ or exc.__context__ + return exc is raised + + records = [r for r in caplog.records if r.exc_info and chains_to_raised(r.exc_info[1])] + assert [(r.name, r.levelname) for r in records] == [("mcp.server.mcpserver.server", "ERROR")] + + @requirement("mcpserver:tool:unknown-name") async def test_call_tool_unknown_name_returns_error_result(connect: Connect, unstamped: Unstamp) -> None: """Calling a tool name that was never registered is reported as an is_error result. diff --git a/tests/server/mcpserver/resources/test_file_resources.py b/tests/server/mcpserver/resources/test_file_resources.py index db9f73e935..674b165ec0 100644 --- a/tests/server/mcpserver/resources/test_file_resources.py +++ b/tests/server/mcpserver/resources/test_file_resources.py @@ -6,6 +6,7 @@ import pytest from pydantic import ValidationError +from mcp.server.mcpserver.exceptions import UnexpectedResourceError from mcp.server.mcpserver.resources import FileResource @@ -178,8 +179,10 @@ async def test_missing_file_error(temp_file: Path): name="test", path=missing, ) - with pytest.raises(ValueError, match="Error reading file"): + with pytest.raises(UnexpectedResourceError) as exc: await resource.read() + assert str(exc.value) == "Error reading resource file:///missing.txt" + assert isinstance(exc.value.__cause__, FileNotFoundError) @pytest.mark.skipif(os.name == "nt", reason="File permissions behave differently on Windows") @@ -192,7 +195,8 @@ async def test_permission_error(temp_file: Path): # pragma: lax no cover name="test", path=temp_file, ) - with pytest.raises(ValueError, match="Error reading file"): + with pytest.raises(UnexpectedResourceError) as exc: await resource.read() + assert isinstance(exc.value.__cause__, PermissionError) finally: temp_file.chmod(0o644) # Restore permissions diff --git a/tests/server/mcpserver/resources/test_function_resources.py b/tests/server/mcpserver/resources/test_function_resources.py index 5a5c5c48dd..dc57dbc31c 100644 --- a/tests/server/mcpserver/resources/test_function_resources.py +++ b/tests/server/mcpserver/resources/test_function_resources.py @@ -7,6 +7,7 @@ from mcp_types import InputRequiredResult from pydantic import BaseModel +from mcp.server.mcpserver.exceptions import UnexpectedResourceError from mcp.server.mcpserver.resources import FunctionResource @@ -80,18 +81,22 @@ def get_data() -> dict[str, str]: @pytest.mark.anyio async def test_error_handling(self): - """Test error handling in FunctionResource.""" + """A crash in the function is wrapped as UnexpectedResourceError naming only the URI, + with the function's own exception as `__cause__`.""" + raised = ValueError("Test error") def failing_func() -> str: - raise ValueError("Test error") + raise raised resource = FunctionResource( uri="function://test", name="test", fn=failing_func, ) - with pytest.raises(ValueError, match="Error reading resource function://test"): + with pytest.raises(UnexpectedResourceError) as exc: await resource.read() + assert str(exc.value) == snapshot("Error reading resource function://test") + assert exc.value.__cause__ is raised @pytest.mark.anyio async def test_basemodel_conversion(self): @@ -255,9 +260,10 @@ def ask() -> InputRequiredResult: return InputRequiredResult(request_state="round-1") resource = FunctionResource(uri="resource://ask", name="ask", fn=ask) - with pytest.raises(ValueError) as exc: + with pytest.raises(UnexpectedResourceError) as exc: await resource.read() - assert str(exc.value) == snapshot( - "Error reading resource resource://ask: static resources cannot return " - "InputRequiredResult; only resource template functions participate in the multi-round-trip flow" + assert str(exc.value) == snapshot("Error reading resource resource://ask") + assert str(exc.value.__cause__) == snapshot( + "static resources cannot return InputRequiredResult; " + "only resource template functions participate in the multi-round-trip flow" ) diff --git a/tests/server/mcpserver/test_server.py b/tests/server/mcpserver/test_server.py index c22d0ca907..15c2e8692e 100644 --- a/tests/server/mcpserver/test_server.py +++ b/tests/server/mcpserver/test_server.py @@ -1,7 +1,8 @@ import base64 +import logging from pathlib import Path from types import SimpleNamespace -from typing import Any +from typing import Annotated, Any from unittest.mock import AsyncMock, MagicMock, patch import anyio @@ -24,6 +25,7 @@ ElicitRequestFormParams, ElicitResult, EmbeddedResource, + ErrorData, GetPromptResult, Icon, ImageContent, @@ -41,17 +43,24 @@ TextContent, TextResourceContents, ) -from pydantic import BaseModel +from pydantic import BaseModel, ValidationError from starlette.applications import Starlette from starlette.routing import Mount, Route from typing_extensions import NotRequired, TypedDict from mcp.client import Client from mcp.server.context import ServerRequestContext -from mcp.server.mcpserver import Context, MCPServer, ResourceSecurity -from mcp.server.mcpserver.exceptions import ResourceNotFoundError, ToolError +from mcp.server.mcpserver import Context, MCPServer, RequestStateSecurity, Resolve, ResourceSecurity +from mcp.server.mcpserver.exceptions import ( + ResourceError, + ResourceNotFoundError, + ToolError, + UnexpectedResourceError, + UnexpectedToolError, +) from mcp.server.mcpserver.prompts.base import Message, UserMessage from mcp.server.mcpserver.resources import FileResource, FunctionResource +from mcp.server.mcpserver.resources import Resource as MCPServerResource from mcp.server.mcpserver.utilities.types import Audio, Image from mcp.server.subscriptions import ( InMemorySubscriptionBus, @@ -2246,6 +2255,512 @@ def thing() -> str: assert exc.value.error.data == {"requiredCapabilities": ["elicitation"]} +def _cause_chain(exc: BaseException | None) -> list[BaseException]: + """`exc` and everything it chains back to, explicitly (`__cause__`) or implicitly (`__context__`).""" + chain: list[BaseException] = [] + while exc is not None: + chain.append(exc) + exc = exc.__cause__ or exc.__context__ + return chain + + +def _server_records(caplog: pytest.LogCaptureFixture) -> list[tuple[str, str, bool]]: + """(level, message, has-traceback) for every record MCPServer itself wrote.""" + return [ + (r.levelname, r.getMessage(), r.exc_info is not None) + for r in caplog.records + if r.name == "mcp.server.mcpserver.server" + ] + + +def _logged_exception(caplog: pytest.LogCaptureFixture) -> BaseException: + """The exception attached to the one MCPServer record that carries a traceback.""" + (exc_info,) = [r.exc_info for r in caplog.records if r.name == "mcp.server.mcpserver.server" and r.exc_info] + assert exc_info[1] is not None + return exc_info[1] + + +async def test_tool_raising_unexpected_exception_is_logged_once_at_error_with_its_traceback( + caplog: pytest.LogCaptureFixture, +): + """SDK-defined: a tool crash still reaches the model as is_error, and the server logs the + original exception exactly once, at ERROR, with the traceback the result text lacks.""" + mcp = MCPServer() + raised = KeyError("k") + + @mcp.tool() + def lookup() -> str: + raise raised + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("lookup", {}) + + assert result.is_error is True + assert result.content == [TextContent(type="text", text="Error executing tool lookup: 'k'")] + assert _server_records(caplog) == snapshot([("ERROR", "Tool 'lookup' raised an unexpected exception", True)]) + assert raised in _cause_chain(_logged_exception(caplog)) + assert len([r for r in caplog.records if r.levelno >= logging.WARNING]) == 1 + + +async def test_tool_raising_tool_error_is_logged_at_info_without_traceback(caplog: pytest.LogCaptureFixture): + """SDK-defined: ToolError marks an anticipated failure, so the same is_error result is + logged as one INFO record with no traceback rather than as a crash.""" + mcp = MCPServer() + + @mcp.tool() + def forecast(city: str) -> str: + raise ToolError(f"no forecast for {city}") + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("forecast", {"city": "Atlantis"}) + + assert result.is_error is True + assert result.content == [TextContent(type="text", text="Error executing tool forecast: no forecast for Atlantis")] + assert _server_records(caplog) == snapshot( + [("INFO", "Tool 'forecast' failed: 'Error executing tool forecast: no forecast for Atlantis'", False)] + ) + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + +async def test_tool_error_subclass_is_still_anticipated(caplog: pytest.LogCaptureFixture): + """SDK-defined: a user's ToolError subclass is treated like ToolError - INFO, no traceback - + and reaches a programmatic caller as a plain ToolError carrying the tool-name prefix.""" + mcp = MCPServer() + + class QuotaExceeded(ToolError): + pass + + @mcp.tool() + def spend() -> str: + raise QuotaExceeded("daily quota used up") + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("spend", {}) + with pytest.raises(ToolError) as exc: + await mcp.call_tool("spend", {}) + + assert result.is_error is True + assert type(exc.value) is ToolError + assert str(exc.value) == snapshot("Error executing tool spend: daily quota used up") + assert _server_records(caplog) == snapshot( + [("INFO", "Tool 'spend' failed: 'Error executing tool spend: daily quota used up'", False)] + ) + + +async def test_tool_argument_validation_failure_is_logged_at_info_without_traceback( + caplog: pytest.LogCaptureFixture, +): + """SDK-defined: arguments the model got wrong are the model's to correct, so the rejection + is logged as one INFO record with no traceback; the message is repr-quoted onto one line.""" + mcp = MCPServer() + + @mcp.tool() + def add(a: int, b: int) -> int: + raise NotImplementedError + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("add", {"a": "one", "b": 2}) + + assert result.is_error is True + ((level, message, has_traceback),) = _server_records(caplog) + assert (level, has_traceback) == ("INFO", False) + # pydantic owns the rest of the text; pin only the SDK's part and the single-line rendering. + assert message.startswith("Tool 'add' failed: ") and "Error executing tool add: 1 validation error" in message + assert "\n" not in message + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + +async def test_tool_argument_validation_failure_chains_directly_to_the_validation_error(): + """SDK-defined: a programmatic caller sees a plain ToolError whose `__cause__` is pydantic's + ValidationError, with no intermediate wrapper.""" + mcp = MCPServer() + + @mcp.tool() + def add(a: int, b: int) -> int: + raise NotImplementedError + + with pytest.raises(ToolError) as exc: + await mcp.call_tool("add", {"a": "one", "b": 2}) + assert type(exc.value) is ToolError + assert isinstance(exc.value.__cause__, ValidationError) + + +async def test_validation_error_raised_inside_the_tool_body_is_a_crash(caplog: pytest.LogCaptureFixture): + """SDK-defined: only the SDK's own argument validation is anticipated; a pydantic + ValidationError from the tool's code is logged as a crash with its traceback.""" + mcp = MCPServer() + + class Row(BaseModel): + n: int + + @mcp.tool() + def parse() -> str: + Row.model_validate({"n": "x"}) + raise NotImplementedError + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("parse", {}) + + assert result.is_error is True + assert _server_records(caplog) == snapshot([("ERROR", "Tool 'parse' raised an unexpected exception", True)]) + assert isinstance(_cause_chain(_logged_exception(caplog))[-1], ValidationError) + + +async def test_return_value_failing_the_output_schema_is_a_crash(caplog: pytest.LogCaptureFixture): + """SDK-defined: a return value that doesn't match the declared output schema is the tool's + bug, so it is logged as a crash even though the model still gets an is_error result.""" + mcp = MCPServer() + + class Weather(BaseModel): + temperature: float + + @mcp.tool() + def get_weather() -> Weather: + reading: Any = {"temperature": "warm"} + return reading + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("get_weather", {}) + + assert result.is_error is True + assert _server_records(caplog) == snapshot([("ERROR", "Tool 'get_weather' raised an unexpected exception", True)]) + + +async def test_unknown_tool_is_logged_at_info_without_traceback(caplog: pytest.LogCaptureFixture): + """SDK-defined: a call to a name that was never registered is the caller's mistake, logged + as one INFO record alongside the is_error result.""" + mcp = MCPServer() + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("nope", {}) + + assert result.is_error is True + assert _server_records(caplog) == snapshot([("INFO", "Tool 'nope' failed: 'Unknown tool: nope'", False)]) + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + +async def test_tool_raising_mcp_error_is_not_logged_by_mcpserver(caplog: pytest.LogCaptureFixture): + """SDK-defined: MCPError is a protocol answer the tool chose, so MCPServer writes no record for it.""" + mcp = MCPServer() + + @mcp.tool() + def gated() -> str: + raise MCPError(code=INVALID_PARAMS, message="not for you") + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + with pytest.raises(MCPError) as exc: + await client.call_tool("gated", {}) + + assert exc.value.error.code == INVALID_PARAMS + assert _server_records(caplog) == [] + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + +async def test_resolver_raising_tool_error_is_anticipated(caplog: pytest.LogCaptureFixture): + """SDK-defined: a ToolError from a Resolve() resolver is classified like one from the tool + body - INFO, no traceback.""" + mcp = MCPServer(name="resolvers", request_state_security=RequestStateSecurity.ephemeral()) + + async def current_user(ctx: Context) -> str: + raise ToolError("sign in first") + + @mcp.tool() + async def whoami(user: Annotated[str, Resolve(current_user)]) -> str: + raise NotImplementedError + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("whoami", {}) + + assert result.content == [TextContent(type="text", text="Error executing tool whoami: sign in first")] + assert _server_records(caplog) == snapshot( + [("INFO", "Tool 'whoami' failed: 'Error executing tool whoami: sign in first'", False)] + ) + + +async def test_resolver_crash_is_logged_as_the_tools_crash(caplog: pytest.LogCaptureFixture): + """SDK-defined: an unexpected exception in a Resolve() resolver is the tool's crash - ERROR + with a traceback reaching the resolver's exception.""" + mcp = MCPServer(name="resolvers", request_state_security=RequestStateSecurity.ephemeral()) + raised = ConnectionError("user directory unreachable") + + async def current_user(ctx: Context) -> str: + raise raised + + @mcp.tool() + async def whoami(user: Annotated[str, Resolve(current_user)]) -> str: + raise NotImplementedError + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("whoami", {}) + + assert result.is_error is True + assert _server_records(caplog) == snapshot([("ERROR", "Tool 'whoami' raised an unexpected exception", True)]) + assert raised in _cause_chain(_logged_exception(caplog)) + + +async def test_static_resource_raising_unexpected_exception_is_logged_once_at_error_with_its_traceback( + caplog: pytest.LogCaptureFixture, +): + """SDK-defined: the client gets a -32603 naming only the URI, and the withheld original is + logged exactly once, at ERROR, with its traceback.""" + mcp = MCPServer() + raised = RuntimeError("connection pool exhausted") + + @mcp.resource("db://stats") + def stats() -> str: + raise raised + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + with pytest.raises(MCPError) as exc: + await client.read_resource("db://stats") + + assert exc.value.error == snapshot( + ErrorData(code=INTERNAL_ERROR, message="Error reading resource db://stats", data={"uri": "db://stats"}) + ) + assert _server_records(caplog) == snapshot( + [("ERROR", "Resource 'db://stats' raised an unexpected exception", True)] + ) + assert raised in _cause_chain(_logged_exception(caplog)) + assert len([r for r in caplog.records if r.levelno >= logging.WARNING]) == 1 + + +async def test_resource_template_raising_unexpected_exception_is_logged_once_at_error_with_its_traceback( + caplog: pytest.LogCaptureFixture, +): + """SDK-defined: a template handler crash surfaces as -32603 naming only the URI, and the + withheld original is logged exactly once, at ERROR, with its traceback.""" + mcp = MCPServer() + raised = RuntimeError("connection pool exhausted") + + @mcp.resource("db://tables/{table}") + def describe(table: str) -> str: + raise raised + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + with pytest.raises(MCPError) as exc: + await client.read_resource("db://tables/users") + + assert exc.value.error == snapshot( + ErrorData( + code=INTERNAL_ERROR, + message="Error creating resource from template db://tables/users", + data={"uri": "db://tables/users"}, + ) + ) + assert _server_records(caplog) == snapshot( + [("ERROR", "Resource 'db://tables/users' raised an unexpected exception", True)] + ) + assert raised in _cause_chain(_logged_exception(caplog)) + assert len([r for r in caplog.records if r.levelno >= logging.WARNING]) == 1 + + +async def test_read_resource_wraps_a_crash_as_unexpected_resource_error_chained_to_the_original(): + """SDK-defined: for static and template resources alike, a programmatic caller gets + UnexpectedResourceError naming only the URI, with `__cause__` the handler's own exception.""" + mcp = MCPServer() + raised = RuntimeError("connection pool exhausted") + + @mcp.resource("db://stats") + def stats() -> str: + raise raised + + @mcp.resource("db://tables/{table}") + def describe(table: str) -> str: + raise raised + + with pytest.raises(UnexpectedResourceError) as static: + await mcp.read_resource("db://stats") + with pytest.raises(UnexpectedResourceError) as template: + await mcp.read_resource("db://tables/users") + + assert str(static.value) == snapshot("Error reading resource db://stats") + assert static.value.__cause__ is raised + assert str(template.value) == snapshot("Error creating resource from template db://tables/users") + assert template.value.__cause__ is raised + + +async def test_custom_resource_subclass_crash_is_wrapped_and_logged_like_a_function_resource( + caplog: pytest.LogCaptureFixture, +): + """SDK-defined: a hand-written Resource subclass whose read() raises gets the same treatment as + a decorated function - -32603 naming only the URI, one ERROR record chaining to the original.""" + raised = OSError("sensor bus offline") + + class SensorResource(MCPServerResource): + async def read(self) -> str: + raise raised + + mcp = MCPServer() + mcp.add_resource(SensorResource(uri="sensor://temp", name="temp")) + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + with pytest.raises(MCPError) as exc: + await client.read_resource("sensor://temp") + + assert exc.value.error == snapshot( + ErrorData(code=INTERNAL_ERROR, message="Error reading resource sensor://temp", data={"uri": "sensor://temp"}) + ) + assert _server_records(caplog) == snapshot( + [("ERROR", "Resource 'sensor://temp' raised an unexpected exception", True)] + ) + logged = _logged_exception(caplog) + assert isinstance(logged, UnexpectedResourceError) and logged.__cause__ is raised + + +async def test_static_resource_raising_resource_not_found_error_is_invalid_params_logged_at_info( + caplog: pytest.LogCaptureFixture, +): + """SDK-defined: ResourceNotFoundError from a static resource handler passes through as -32602 + with the handler's message, as it does from a template handler, and is logged at INFO.""" + mcp = MCPServer() + + @mcp.resource("reports://latest") + def latest() -> str: + raise ResourceNotFoundError("no report has been generated yet") + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + with pytest.raises(MCPError) as exc: + await client.read_resource("reports://latest") + + assert exc.value.error == snapshot( + ErrorData(code=INVALID_PARAMS, message="no report has been generated yet", data={"uri": "reports://latest"}) + ) + assert _server_records(caplog) == snapshot( + [("INFO", "Resource 'reports://latest' failed: 'no report has been generated yet'", False)] + ) + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + +async def test_deliberate_resource_error_passes_its_message_through_and_is_logged_at_info( + caplog: pytest.LogCaptureFixture, +): + """SDK-defined: a ResourceError the handler raised on purpose reaches the client as -32603 with + the handler's message, from a static resource as from a template, and is one INFO record each.""" + mcp = MCPServer() + + @mcp.resource("db://stats") + def stats() -> str: + raise ResourceError("stats database is in maintenance") + + @mcp.resource("db://tables/{table}") + def describe(table: str) -> str: + raise ResourceError(f"table {table} is being rebuilt") + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + with pytest.raises(MCPError) as static: + await client.read_resource("db://stats") + with pytest.raises(MCPError) as template: + await client.read_resource("db://tables/users") + + assert static.value.error == snapshot( + ErrorData(code=INTERNAL_ERROR, message="stats database is in maintenance", data={"uri": "db://stats"}) + ) + assert template.value.error == snapshot( + ErrorData(code=INTERNAL_ERROR, message="table users is being rebuilt", data={"uri": "db://tables/users"}) + ) + assert _server_records(caplog) == snapshot( + [ + ("INFO", "Resource 'db://stats' failed: 'stats database is in maintenance'", False), + ("INFO", "Resource 'db://tables/users' failed: 'table users is being rebuilt'", False), + ] + ) + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + +async def test_prompt_raising_unexpected_exception_is_logged_once(caplog: pytest.LogCaptureFixture): + """SDK-defined: a prompt crash is logged exactly once, by the dispatcher boundary that turns it + into the JSON-RPC error, and not a second time by MCPServer.""" + mcp = MCPServer() + raised = RuntimeError("template store unreachable") + + @mcp.prompt() + def briefing() -> str: + raise raised + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + with pytest.raises(MCPError) as exc: + await client.get_prompt("briefing") + + assert exc.value.error.code == INTERNAL_ERROR + assert _server_records(caplog) == [] + (record,) = [r for r in caplog.records if r.levelno >= logging.WARNING] + assert record.levelno == logging.ERROR + assert record.exc_info is not None and raised in _cause_chain(record.exc_info[1]) + + +async def test_call_tool_wraps_a_crash_as_unexpected_tool_error_chained_to_the_original(): + """SDK-defined: programmatic callers can tell a crash from a deliberate ToolError by type and + reach the original exception through `__cause__`.""" + mcp = MCPServer() + raised = RuntimeError("boom") + + @mcp.tool() + def explode() -> str: + raise raised + + with pytest.raises(UnexpectedToolError) as exc: + await mcp.call_tool("explode", {}) + assert str(exc.value) == snapshot("Error executing tool explode: boom") + assert exc.value.__cause__ is raised + + +async def test_call_tool_keeps_a_deliberate_tool_error_a_plain_tool_error(): + """SDK-defined: a ToolError raised by the tool is re-raised as a plain ToolError carrying the + tool-name prefix, never reclassified as unexpected.""" + mcp = MCPServer() + + @mcp.tool() + def refuse() -> str: + raise ToolError("not today") + + with pytest.raises(ToolError) as exc: + await mcp.call_tool("refuse", {}) + assert type(exc.value) is ToolError + assert str(exc.value) == snapshot("Error executing tool refuse: not today") + + +async def test_nested_tool_crash_stays_unexpected_through_the_outer_tool(caplog: pytest.LogCaptureFixture): + """SDK-defined: when a tool awaits another tool that crashes, the outer wrapper keeps the + UnexpectedToolError classification, so the crash is still logged once with its traceback.""" + mcp = MCPServer() + raised = ZeroDivisionError("division by zero") + + @mcp.tool() + def inner() -> str: + raise raised + + @mcp.tool() + async def outer(ctx: Context) -> str: + await ctx.mcp_server.call_tool("inner", {}) + raise NotImplementedError + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("outer", {}) + + assert result.content == [ + TextContent(type="text", text="Error executing tool outer: Error executing tool inner: division by zero") + ] + assert _server_records(caplog) == snapshot([("ERROR", "Tool 'outer' raised an unexpected exception", True)]) + assert raised in _cause_chain(_logged_exception(caplog)) + + async def test_context_exposes_client_capabilities_from_connection(): mcp = MCPServer() seen: list[ClientCapabilities | None] = [] From 7b0059cabe0e07ca5eb427fc13ba666b00c1a711 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:11:03 +0000 Subject: [PATCH 2/8] Inline handler logging and trim the docs 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. --- docs/handlers/logging.md | 2 +- docs/servers/handling-errors.md | 19 ++++---- docs/servers/uri-templates.md | 5 +-- docs/troubleshooting.md | 2 +- src/mcp/server/mcpserver/exceptions.py | 8 ++-- .../server/mcpserver/resources/templates.py | 2 +- src/mcp/server/mcpserver/resources/types.py | 2 +- src/mcp/server/mcpserver/server.py | 44 ++++++++----------- tests/docs_src/test_handling_errors.py | 5 +-- tests/interaction/_requirements.py | 23 ---------- tests/interaction/mcpserver/test_prompts.py | 32 -------------- tests/interaction/mcpserver/test_resources.py | 32 -------------- tests/interaction/mcpserver/test_tools.py | 31 ------------- 13 files changed, 39 insertions(+), 168 deletions(-) diff --git a/docs/handlers/logging.md b/docs/handlers/logging.md index 1c839d70b5..bac877a8d3 100644 --- a/docs/handlers/logging.md +++ b/docs/handlers/logging.md @@ -70,7 +70,7 @@ went to standard error: the terminal, not the wire. don't want log lines, you want spans. Your server already emits them: the SDK traces every message with OpenTelemetry out of the box. See **[OpenTelemetry](../run/opentelemetry.md)**. -You don't have to log your own handlers' crashes either. When a tool or resource function raises something unexpected, the SDK writes the `ERROR` record with the traceback for you, on its own `mcp.*` loggers; a failure you raised deliberately (`ToolError`, `ResourceNotFoundError`) is an `INFO` line instead. A prompt function that raises is an `ERROR` record too, whatever it raised. **[Handling errors](../servers/handling-errors.md#what-lands-in-your-log)** has the split. (In a test using `Client(mcp, raise_exceptions=True)`, a prompt failure is handed to your test as the exception rather than logged.) +You also don't need a `try`/`except` in every handler just to record failures. When a tool or resource function raises, the SDK logs it for you. **[Handling errors](../servers/handling-errors.md#what-the-server-logs)** explains what gets logged and at which level. ## Recap diff --git a/docs/servers/handling-errors.md b/docs/servers/handling-errors.md index 6519889312..9e7cedff2f 100644 --- a/docs/servers/handling-errors.md +++ b/docs/servers/handling-errors.md @@ -118,14 +118,14 @@ It means a whole class of `raise` statements you don't write: don't re-validate Everything so far 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 the - `is_error=True` result. Assert on the result; the traceback is in the server's log (next - section), which pytest's `caplog` captures. **[Testing](../get-started/testing.md)** covers the pattern. + `is_error=True` result. Assert on the result. If you need the traceback, it is in the server's + log (next section), and pytest's `caplog` captures it. **[Testing](../get-started/testing.md)** covers the pattern. -## What lands in your log +## What the server logs -Your server keeps its own record of these failures, and it draws one more line: between a failure you anticipated and one you didn't. +The server also logs these failures, and how it logs them depends on whether you anticipated the failure. -`get_author` raised a plain `ValueError`. The model got the message, but the SDK can't know you *meant* that exception, so it assumes you didn't: the call is logged at `ERROR` with the full traceback. That is exactly what you want on the day the exception is a `KeyError` from three libraries down and the result text says only `'id'`. +`get_author` raised a plain `ValueError`. The model got the message, but the SDK can't tell that you raised it on purpose, so it treats the call as a crash and logs it at `ERROR` with the full traceback. That is what you want on the day the exception is a `KeyError` from deep inside a library and the result text says only `'id'`. When the failure is one you planned for, say so with `ToolError`: @@ -133,11 +133,9 @@ When the failure is one you planned for, say so with `ToolError`: --8<-- "docs_src/handling_errors/tutorial004.py" ``` -The model reads precisely what it read before. The difference is on your side: a `ToolError` is logged as one `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 `INFO` lines too; those are the caller's mistakes, not yours. +`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 draw the same line. The `-32603` from a crashing resource handler names only the URI, so the `ERROR` record in your log is the one place the cause and its traceback exist. `ResourceNotFoundError`, including the SDK's own `Unknown resource`, is an `INFO` line. (A template parameter that fails its type annotation, `books://{id}` read with an `id` that isn't an `int`, currently counts as a crash.) - -Prompts aren't split yet: any failure in a prompt function, including an unknown name or a missing argument, is one `ERROR` record with its traceback, written by the transport layer that turns it into the JSON-RPC error. +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` is an `INFO` line. ## Recap @@ -146,8 +144,7 @@ Prompts aren't split yet: any failure in a prompt function, including an unknown * The deciding question: *could a smarter model have avoided this?* Yes -> exception. No -> `MCPError`. * `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. -* In your log: an exception you didn't raise as `ToolError` is an `ERROR` record with its traceback; `ToolError`, bad tool arguments, unknown tool names, and `ResourceNotFoundError` are one `INFO` line each. -* `from mcp import MCPError`; `ToolError` and `ResourceNotFoundError` come from `mcp.server.mcpserver.exceptions`; the error-code constants come from `mcp.types`. +* `from mcp import MCPError`; the error-code constants come 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)**. diff --git a/docs/servers/uri-templates.md b/docs/servers/uri-templates.md index a79889b2af..5a0d1f0575 100644 --- a/docs/servers/uri-templates.md +++ b/docs/servers/uri-templates.md @@ -201,9 +201,8 @@ These checks are a heuristic pre-filter; for filesystem access, !!! tip If your handler can't fulfil the request (the file doesn't exist, the id is unknown), raise `ResourceNotFoundError` from `mcp.server.mcpserver.exceptions`. The client gets `-32602` with - your message and the URI, and your log gets one `INFO` line; any other exception is treated as - a crash (`-32603`, and an `ERROR` record with the traceback). See - **[Handling errors](handling-errors.md#a-resource-that-doesnt-exist)**. + your message and the URI. Any other exception is treated as a crash and the client gets a + generic `-32603`. See **[Handling errors](handling-errors.md#a-resource-that-doesnt-exist)**. ## Resources on the low-level Server diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index f549dfbadd..49d972ce25 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -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 `` alone doesn't tell you what broke, the traceback is in the **server's log**: an exception the tool didn't raise as `ToolError` is logged there at `ERROR`, as `Tool '' raised an unexpected exception`. +If `` alone doesn't tell you what broke, look in the **server's log**. Unless the tool raised `ToolError`, the exception is logged there at `ERROR` with its traceback, as `Tool '' raised an unexpected exception`. ## `TypeError: The @tool decorator was used incorrectly. Did you forget to call it? Use @tool() instead of @tool` diff --git a/src/mcp/server/mcpserver/exceptions.py b/src/mcp/server/mcpserver/exceptions.py index 9f2415d976..a2cd0c1d8c 100644 --- a/src/mcp/server/mcpserver/exceptions.py +++ b/src/mcp/server/mcpserver/exceptions.py @@ -16,8 +16,8 @@ class ResourceError(MCPServerError): class ResourceNotFoundError(ResourceError): """Resource does not exist. - Raise this from a resource handler to signal that the requested instance does not exist; - clients receive `-32602` (invalid params) per + Raise this from a resource handler to signal that the requested instance does not exist. + Clients receive `-32602` (invalid params) per [SEP-2164](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2164). """ @@ -26,7 +26,7 @@ class UnexpectedResourceError(ResourceError): """A resource read failed with something other than `ResourceError` or `MCPError`. MCPServer raises this itself, around a crash in a resource or resource - template handler or a failed file read; you never raise it. `__cause__` is + template handler or a failed file read. You never raise it. `__cause__` is the original exception, which the server logs with its traceback. The message names only the URI, so the original text is withheld from the client. """ @@ -50,7 +50,7 @@ class UnexpectedToolError(ToolError): """A tool call failed with something other than `ToolError` or `MCPError`. MCPServer raises this itself, around a crash in the tool (or a resolver) or a - return value that fails output conversion; you never raise it. `__cause__` is + return value that fails output conversion. You never raise it. `__cause__` is the original exception, which the server logs with its traceback before returning the usual `is_error=True` result. Catch it around `MCPServer.call_tool()` to tell a crash from a deliberate `ToolError`. diff --git a/src/mcp/server/mcpserver/resources/templates.py b/src/mcp/server/mcpserver/resources/templates.py index 54e50de4e4..1699d9a734 100644 --- a/src/mcp/server/mcpserver/resources/templates.py +++ b/src/mcp/server/mcpserver/resources/templates.py @@ -216,7 +216,7 @@ async def create_resource( Raises: ResourceError: If the template function raises `ResourceError`. UnexpectedResourceError: If the template function raises anything other - than `ResourceError` or `MCPError`; `__cause__` is the original. + than `ResourceError` or `MCPError`. `__cause__` is the original exception. """ try: # Add context to params if needed diff --git a/src/mcp/server/mcpserver/resources/types.py b/src/mcp/server/mcpserver/resources/types.py index f3751989d7..f77b32b610 100644 --- a/src/mcp/server/mcpserver/resources/types.py +++ b/src/mcp/server/mcpserver/resources/types.py @@ -84,7 +84,7 @@ async def read(self) -> str | bytes: Raises: UnexpectedResourceError: If the function raises anything other than - `ResourceError` or `MCPError`; `__cause__` is the original. + `ResourceError` or `MCPError`. `__cause__` is the original exception. """ try: fn = self.fn diff --git a/src/mcp/server/mcpserver/server.py b/src/mcp/server/mcpserver/server.py index 5bced230fe..ae7844c0fd 100644 --- a/src/mcp/server/mcpserver/server.py +++ b/src/mcp/server/mcpserver/server.py @@ -427,7 +427,14 @@ async def _handle_call_tool( except MCPError: raise except Exception as exc: - _log_handler_exception("Tool", params.name, exc) + # A ToolError (deliberate, unknown tool, rejected arguments) is an outcome + # the model already reads in full, so it is one INFO record, repr-quoted to + # keep peer-supplied text on one line. Anything else is a crash in the + # tool: log the traceback that the result text doesn't carry. + if isinstance(exc, ToolError) and not isinstance(exc, UnexpectedToolError): + logger.info("Tool %r failed: %r", params.name, str(exc)) + else: + logger.exception("Tool %r raised an unexpected exception", params.name) return CallToolResult(content=[TextContent(type="text", text=str(exc))], is_error=True) async def _handle_list_resources( @@ -442,7 +449,13 @@ async def _handle_read_resource( try: results = await self.read_resource(params.uri, context) except ResourceError as err: - _log_handler_exception("Resource", str(params.uri), err) + # UnexpectedResourceError wraps a crash whose text is withheld from the + # client, so the traceback goes to the log. Any other ResourceError was + # raised on purpose (or is the SDK's "Unknown resource") and is one INFO record. + if isinstance(err, UnexpectedResourceError): + logger.exception("Resource %r raised an unexpected exception", str(params.uri)) + else: + logger.info("Resource %r failed: %r", str(params.uri), str(err)) code = INVALID_PARAMS if isinstance(err, ResourceNotFoundError) else INTERNAL_ERROR raise MCPError(code=code, message=str(err), data={"uri": str(params.uri)}) if isinstance(results, InputRequiredResult): @@ -511,8 +524,8 @@ async def call_tool( ToolError: If the tool is unknown, the arguments fail validation, or the tool (or a resolver) raises `ToolError`. UnexpectedToolError: If the tool (or a resolver) raises anything other than - `ToolError` or `MCPError`, or its return value fails output conversion; - `__cause__` is the original. + `ToolError` or `MCPError`, or its return value fails output conversion. + `__cause__` is the original exception. """ if context is None: context = Context(mcp_server=self, subscriptions=self._subscriptions) @@ -566,8 +579,8 @@ async def read_resource( ResourceNotFoundError: If no resource or template matches the URI. ResourceError: If the resource or template function raises `ResourceError`. UnexpectedResourceError: If reading the resource (or creating it from a - template) raises anything other than `ResourceError` or `MCPError`; - `__cause__` is the original. + template) raises anything other than `ResourceError` or `MCPError`. + `__cause__` is the original exception. """ if context is None: context = Context(mcp_server=self, subscriptions=self._subscriptions) @@ -1320,25 +1333,6 @@ async def get_prompt( raise ValueError(str(e)) from e -def _log_handler_exception(kind: Literal["Tool", "Resource"], name: str, exc: Exception) -> None: - """Record a tool or resource handler failure; the one place MCPServer logs them. - - Called from the `except` block that turns the failure into a response. A - `ToolError` or `ResourceError` (deliberate, an unknown name, arguments that - failed validation, `ResourceNotFoundError`) is an anticipated outcome the - client already receives in full: one INFO record, no traceback, the text - repr-quoted so peer-supplied names and newlines stay on one line. Anything - else, including the `Unexpected*` wrappers whose `__cause__` is what the - handler actually raised, is a crash in user code: ERROR with the traceback. - """ - if isinstance(exc, ToolError | ResourceError) and not isinstance( - exc, UnexpectedToolError | UnexpectedResourceError - ): - logger.info("%s %r failed: %r", kind, name, str(exc)) - else: - logger.exception("%s %r raised an unexpected exception", kind, name, exc_info=exc) - - def _version_gated(method: MethodBinding) -> RequestHandler: """Wrap a method handler so a request at a disallowed protocol version is rejected. diff --git a/tests/docs_src/test_handling_errors.py b/tests/docs_src/test_handling_errors.py index e2aab0e423..8872ba7b4c 100644 --- a/tests/docs_src/test_handling_errors.py +++ b/tests/docs_src/test_handling_errors.py @@ -90,7 +90,7 @@ async def test_a_title_the_template_knows_reads_normally() -> None: async def test_a_plain_exception_is_logged_as_a_crash_with_its_traceback(caplog: pytest.LogCaptureFixture) -> None: - """tutorial001, "What lands in your log": the `ValueError` is one ERROR record carrying the traceback.""" + """tutorial001, "What the server logs": the `ValueError` is one ERROR record carrying the traceback.""" caplog.set_level(logging.INFO) async with Client(tutorial001.mcp) as client: await client.call_tool("get_author", {"title": "Nothing"}) @@ -99,7 +99,6 @@ async def test_a_plain_exception_is_logged_as_a_crash_with_its_traceback(caplog: assert record.exc_info is not None logged = record.exc_info[1] assert logged is not None and isinstance(logged.__cause__, ValueError) - assert str(logged.__cause__) == "No book titled 'Nothing' in the catalog." async def test_tool_error_reads_the_same_to_the_model_and_logs_one_info_line( @@ -119,7 +118,7 @@ async def test_tool_error_reads_the_same_to_the_model_and_logs_one_info_line( async def test_a_bad_argument_is_an_info_line_not_a_crash(caplog: pytest.LogCaptureFixture) -> None: - """ "What lands in your log": schema rejection of the arguments is logged at INFO with no traceback.""" + """ "What the server logs": schema rejection of the arguments is logged at INFO with no traceback.""" caplog.set_level(logging.INFO) async with Client(tutorial001.mcp) as client: result = await client.call_tool("get_author", {"title": 42}) diff --git a/tests/interaction/_requirements.py b/tests/interaction/_requirements.py index 56a970c808..86725bcb4f 100644 --- a/tests/interaction/_requirements.py +++ b/tests/interaction/_requirements.py @@ -1020,14 +1020,6 @@ def __post_init__(self) -> None: "tool result with isError true and the failure text in content; it does not become a JSON-RPC error." ), ), - "mcpserver:tool:handler-throws:logged": Requirement( - source="sdk", - behavior=( - "An exception other than ToolError raised by a tool function is logged server-side exactly once, " - "at ERROR with its traceback, before the isError result is returned; the transport does not change " - "how many records are written." - ), - ), "mcpserver:tool:input-validation": Requirement( source=f"{SPEC_BASE_URL}/server/tools#error-handling", behavior=( @@ -1319,13 +1311,6 @@ def __post_init__(self) -> None: "(-32603 Internal error), with the original exception text withheld." ), ), - "mcpserver:resource:read-throws:logged": Requirement( - source="sdk", - behavior=( - "The exception withheld from the -32603 response is logged server-side exactly once, at ERROR " - "with its traceback; the transport does not change how many records are written." - ), - ), "mcpserver:resource:static": Requirement( source="sdk", behavior=( @@ -1442,14 +1427,6 @@ def __post_init__(self) -> None: source="sdk", behavior="A prompt with optional arguments can be fetched without supplying them.", ), - "mcpserver:prompt:render-throws:logged": Requirement( - source="sdk", - behavior=( - "An exception raised by a prompt function is logged server-side exactly once, at ERROR with its " - "traceback, by whichever layer turns it into the JSON-RPC error; the transport does not change how " - "many records are written." - ), - ), "mcpserver:prompt:unknown-name": Requirement( source=f"{SPEC_BASE_URL}/server/prompts#error-handling", behavior="prompts/get for a name that was never registered returns JSON-RPC error -32602 (Invalid params).", diff --git a/tests/interaction/mcpserver/test_prompts.py b/tests/interaction/mcpserver/test_prompts.py index 5a357592ae..8409e50207 100644 --- a/tests/interaction/mcpserver/test_prompts.py +++ b/tests/interaction/mcpserver/test_prompts.py @@ -1,7 +1,5 @@ """Prompt interactions against MCPServer, driven through the public Client API.""" -import logging - import pytest from inline_snapshot import snapshot from mcp_types import ( @@ -143,36 +141,6 @@ def repeat(phrase: str, count: int) -> str: assert exc_info.value.error.message.startswith("Error rendering prompt repeat: 1 validation error") -@requirement("mcpserver:prompt:render-throws:logged") -async def test_get_prompt_function_exception_is_logged_once_with_its_traceback( - connect: Connect, caplog: pytest.LogCaptureFixture -) -> None: - """An exception raised by a prompt function is logged exactly once, at ERROR, with its traceback. - - MCPServer lets the failure escape to the dispatcher boundary, which owns both the JSON-RPC error and - the log record; the owning logger therefore differs by transport, but the count must not. - """ - mcp = MCPServer("prompter") - raised = RuntimeError("template store unreachable") - - @mcp.prompt() - def briefing() -> str: - raise raised - - caplog.set_level(logging.ERROR) - async with connect(mcp) as client: - with pytest.raises(MCPError): - await client.get_prompt("briefing") - - def chains_to_raised(exc: BaseException | None) -> bool: - while exc is not None and exc is not raised: - exc = exc.__cause__ or exc.__context__ - return exc is raised - - records = [r for r in caplog.records if r.exc_info and chains_to_raised(r.exc_info[1])] - assert [r.levelname for r in records] == ["ERROR"] - - @requirement("mcpserver:prompt:optional-args") async def test_get_prompt_with_an_optional_argument_omitted_uses_the_default( connect: Connect, unstamped: Unstamp diff --git a/tests/interaction/mcpserver/test_resources.py b/tests/interaction/mcpserver/test_resources.py index 162fa9a813..eadf4794e6 100644 --- a/tests/interaction/mcpserver/test_resources.py +++ b/tests/interaction/mcpserver/test_resources.py @@ -1,7 +1,5 @@ """Resource interactions against MCPServer, driven through the public Client API.""" -import logging - import pytest from inline_snapshot import snapshot from mcp_types import ( @@ -154,36 +152,6 @@ def boom() -> str: ) -@requirement("mcpserver:resource:read-throws:logged") -async def test_resource_function_exception_is_logged_once_with_its_traceback( - connect: Connect, caplog: pytest.LogCaptureFixture -) -> None: - """The exception withheld from the -32603 response is logged exactly once, at ERROR, with its traceback. - - The client sees only the URI, so this record is the operator's only route to the cause; it must be - written on every transport and never duplicated by a dispatcher boundary. - """ - mcp = MCPServer("library") - raised = RuntimeError("nope") - - @mcp.resource("res://boom") - def boom() -> str: - raise raised - - caplog.set_level(logging.ERROR) - async with connect(mcp) as client: - with pytest.raises(MCPError): - await client.read_resource("res://boom") - - def chains_to_raised(exc: BaseException | None) -> bool: - while exc is not None and exc is not raised: - exc = exc.__cause__ or exc.__context__ - return exc is raised - - records = [r for r in caplog.records if r.exc_info and chains_to_raised(r.exc_info[1])] - assert [(r.name, r.levelname) for r in records] == [("mcp.server.mcpserver.server", "ERROR")] - - @requirement("mcpserver:resource:duplicate-name") async def test_registering_a_duplicate_resource_uri_warns_and_keeps_the_first( connect: Connect, unstamped: Unstamp diff --git a/tests/interaction/mcpserver/test_tools.py b/tests/interaction/mcpserver/test_tools.py index c275fc60b1..a6418ac9c5 100644 --- a/tests/interaction/mcpserver/test_tools.py +++ b/tests/interaction/mcpserver/test_tools.py @@ -119,37 +119,6 @@ def flux() -> str: ) -@requirement("mcpserver:tool:handler-throws:logged") -async def test_call_tool_function_exception_is_logged_once_with_its_traceback( - connect: Connect, caplog: pytest.LogCaptureFixture -) -> None: - """The exception behind an is_error result is logged exactly once, at ERROR, with its traceback. - - The result text carries only `str(exc)`; the traceback exists nowhere but this record, so it must - be written on every transport and never duplicated by a dispatcher boundary. - """ - mcp = MCPServer("errors") - raised = LookupError("no such row") - - @mcp.tool() - def explode() -> str: - raise raised - - caplog.set_level(logging.ERROR) - async with connect(mcp) as client: - result = await client.call_tool("explode", {}) - - assert result.is_error is True - - def chains_to_raised(exc: BaseException | None) -> bool: - while exc is not None and exc is not raised: - exc = exc.__cause__ or exc.__context__ - return exc is raised - - records = [r for r in caplog.records if r.exc_info and chains_to_raised(r.exc_info[1])] - assert [(r.name, r.levelname) for r in records] == [("mcp.server.mcpserver.server", "ERROR")] - - @requirement("mcpserver:tool:unknown-name") async def test_call_tool_unknown_name_returns_error_result(connect: Connect, unstamped: Unstamp) -> None: """Calling a tool name that was never registered is reported as an is_error result. From 4e0fc9ebcbd2a95f82567004a2f9503586cac14f Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Tue, 18 Aug 2026 14:13:05 +0000 Subject: [PATCH 3/8] Wrap crashing validators, treat ResourceError in a tool as anticipated 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). --- docs/handlers/logging.md | 4 +- docs/migration.md | 2 +- docs/servers/handling-errors.md | 4 +- docs/servers/uri-templates.md | 8 +- docs/troubleshooting.md | 2 +- docs_src/uri_templates/tutorial002.py | 6 +- src/mcp/server/mcpserver/context.py | 8 +- src/mcp/server/mcpserver/exceptions.py | 32 ++-- .../server/mcpserver/resources/templates.py | 2 - src/mcp/server/mcpserver/resources/types.py | 87 ++++------- src/mcp/server/mcpserver/server.py | 24 +-- src/mcp/server/mcpserver/tools/base.py | 34 +++-- tests/docs_src/test_uri_templates.py | 13 ++ tests/interaction/_requirements.py | 11 +- tests/interaction/mcpserver/test_resources.py | 23 +++ .../resources/test_file_resources.py | 8 +- .../resources/test_function_resources.py | 17 +-- tests/server/mcpserver/test_resolve.py | 9 +- tests/server/mcpserver/test_server.py | 137 +++++++++++++++++- 19 files changed, 296 insertions(+), 135 deletions(-) diff --git a/docs/handlers/logging.md b/docs/handlers/logging.md index bac877a8d3..2370750d82 100644 --- a/docs/handlers/logging.md +++ b/docs/handlers/logging.md @@ -49,6 +49,8 @@ The default is `"INFO"`. `logging.basicConfig()` never replaces handlers that already exist. If you configure logging yourself before creating the server, your configuration wins. +You also don't need a `try`/`except` in every handler just to record failures. When a tool or resource function raises, the SDK logs it for you. **[Handling errors](../servers/handling-errors.md#what-the-server-logs)** explains what gets logged and at which level. + ## Try it Run the server with the MCP Inspector: @@ -70,8 +72,6 @@ went to standard error: the terminal, not the wire. don't want log lines, you want spans. Your server already emits them: the SDK traces every message with OpenTelemetry out of the box. See **[OpenTelemetry](../run/opentelemetry.md)**. -You also don't need a `try`/`except` in every handler just to record failures. When a tool or resource function raises, the SDK logs it for you. **[Handling errors](../servers/handling-errors.md#what-the-server-logs)** explains what gets logged and at which level. - ## Recap * The MCP protocol's logging capability is deprecated by the 2026-07-28 spec and not replaced. Don't build on it. diff --git a/docs/migration.md b/docs/migration.md index e59b4a6ac6..ed1b2a58b5 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -1018,7 +1018,7 @@ except MCPError as e: Reading a missing resource now returns JSON-RPC error code `-32602` (invalid params) with the requested URI in `error.data` (`{"uri": ...}`), per [SEP-2164](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2164). Previously the server returned code `0` with no `data`. Clients can now reliably distinguish not-found from other errors; a resource handler (static or template) that raises `ResourceNotFoundError` (from `mcp.server.mcpserver.exceptions`) produces this same response. -The underlying lookups now raise typed exceptions instead of `ValueError`. `ResourceManager.get_resource()` raises `ResourceNotFoundError` when no resource or template matches the URI, and `ResourceTemplate.create_resource()` raises `ResourceError` when the template function fails. Neither subclasses `ValueError`, so callers catching `ValueError` should switch to `ResourceNotFoundError` / `ResourceError` (both importable from `mcp.server.mcpserver.exceptions`; `ResourceNotFoundError` subclasses `ResourceError`). +The underlying lookups now raise typed exceptions instead of `ValueError`. `ResourceManager.get_resource()` raises `ResourceNotFoundError` when no resource or template matches the URI, and `ResourceTemplate.create_resource()` raises `ResourceError` when the template function fails. Neither subclasses `ValueError`, so callers catching `ValueError` should switch to `ResourceNotFoundError` / `ResourceError` (both importable from `mcp.server.mcpserver.exceptions`; `ResourceNotFoundError` subclasses `ResourceError`). Likewise, `FunctionResource.read()` and `FileResource.read()` no longer wrap failures in `ValueError`: called directly they raise whatever the function or file read raised, and through `MCPServer.read_resource()` that arrives as `UnexpectedResourceError` (a `ResourceError`) with the original as `__cause__`. ### `Resource` classes reject unknown keyword arguments diff --git a/docs/servers/handling-errors.md b/docs/servers/handling-errors.md index 9e7cedff2f..005396ace9 100644 --- a/docs/servers/handling-errors.md +++ b/docs/servers/handling-errors.md @@ -123,7 +123,7 @@ It means a whole class of `raise` statements you don't write: don't re-validate ## What the server logs -The server also logs these failures, and how it logs them depends on whether you anticipated the failure. +The server also logs tool and resource failures, and how it logs them depends on whether you anticipated the failure. `get_author` raised a plain `ValueError`. The model got the message, but the SDK can't tell that you raised it on purpose, so it treats the call as a crash and logs it at `ERROR` with the full traceback. That is what you want on the day the exception is a `KeyError` from deep inside a library and the result text says only `'id'`. @@ -135,7 +135,7 @@ When the failure is one you planned for, say so with `ToolError`: `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` is an `INFO` line. +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`. ## Recap diff --git a/docs/servers/uri-templates.md b/docs/servers/uri-templates.md index 5a0d1f0575..1d6e13c0a0 100644 --- a/docs/servers/uri-templates.md +++ b/docs/servers/uri-templates.md @@ -159,7 +159,7 @@ The built-in checks stop the common cases but can't know your sandbox boundary. For filesystem access, use `safe_join` to resolve the path and verify it stays inside your base directory: -```python title="server.py" hl_lines="4 14" +```python title="server.py" hl_lines="5 15" --8<-- "docs_src/uri_templates/tutorial002.py" ``` @@ -200,9 +200,9 @@ These checks are a heuristic pre-filter; for filesystem access, !!! tip If your handler can't fulfil the request (the file doesn't exist, the id is unknown), raise - `ResourceNotFoundError` from `mcp.server.mcpserver.exceptions`. The client gets `-32602` with - your message and the URI. Any other exception is treated as a crash and the client gets a - generic `-32603`. See **[Handling errors](handling-errors.md#a-resource-that-doesnt-exist)**. + `ResourceNotFoundError` as `read_manual` does above. The client gets `-32602` with your message + and the URI. An unexpected exception becomes a generic `-32603` instead. See + **[Handling errors](handling-errors.md#a-resource-that-doesnt-exist)**. ## Resources on the low-level Server diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 49d972ce25..2d53123a69 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -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 `` alone doesn't tell you what broke, look in the **server's log**. Unless the tool raised `ToolError`, the exception is logged there at `ERROR` with its traceback, as `Tool '' raised an unexpected exception`. +If `` 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 '' raised an unexpected exception`. ## `TypeError: The @tool decorator was used incorrectly. Did you forget to call it? Use @tool() instead of @tool` diff --git a/docs_src/uri_templates/tutorial002.py b/docs_src/uri_templates/tutorial002.py index 3d0dc5c36b..94ca94c10d 100644 --- a/docs_src/uri_templates/tutorial002.py +++ b/docs_src/uri_templates/tutorial002.py @@ -1,6 +1,7 @@ from pathlib import Path from mcp.server import MCPServer +from mcp.server.mcpserver.exceptions import ResourceNotFoundError from mcp.shared.path_security import safe_join mcp = MCPServer("Bookshop") @@ -11,4 +12,7 @@ @mcp.resource("manuals://{+path}") def read_manual(path: str) -> str: """A staff manual page, served from a directory on disk.""" - return safe_join(DOCS_ROOT, path).read_text(encoding="utf-8") + file = safe_join(DOCS_ROOT, path) + if not file.is_file(): + raise ResourceNotFoundError(f"No manual at {path!r}.") + return file.read_text(encoding="utf-8") diff --git a/src/mcp/server/mcpserver/context.py b/src/mcp/server/mcpserver/context.py index bf4c26a248..07c4799dc1 100644 --- a/src/mcp/server/mcpserver/context.py +++ b/src/mcp/server/mcpserver/context.py @@ -169,8 +169,12 @@ async def read_resource(self, uri: str | AnyUrl) -> Iterable[ReadResourceContent The resource content as either text or bytes Raises: - ResourceNotFoundError: If no resource or template matches the URI. - ResourceError: If template creation or resource reading fails. + ResourceNotFoundError: If no resource or template matches the URI, or the + handler raised it. + ResourceError: If the resource or template function raises `ResourceError`. + UnexpectedResourceError: If the resource or template function raises anything + else. `__cause__` is the original exception. Left uncaught in a tool, this + is logged as the tool's crash, while the two above are not. RuntimeError: If the resource returned an `InputRequiredResult`. """ assert self._mcp_server is not None, "Context is not available outside of a request" diff --git a/src/mcp/server/mcpserver/exceptions.py b/src/mcp/server/mcpserver/exceptions.py index a2cd0c1d8c..0a39d61981 100644 --- a/src/mcp/server/mcpserver/exceptions.py +++ b/src/mcp/server/mcpserver/exceptions.py @@ -6,10 +6,18 @@ class MCPServerError(Exception): class ResourceError(MCPServerError): - """Error in resource operations. - - When a resource or resource template handler raises this, its message reaches - the client as a `-32603` protocol error. + """A resource failure you anticipated. + + Raise this from a resource or resource template handler for a failure you saw + coming: the client receives a `-32603` protocol error carrying your message + (`ResourceNotFoundError` below is the `-32602` variant), and the server logs it + at INFO without a traceback. Any other exception is treated as a crash: the + client gets a generic message naming only the URI, and the server logs the + traceback at ERROR. + + The SDK raises it too, and `UnexpectedResourceError` subclasses it, so + `except ResourceError` around `MCPServer.read_resource()` catches every read + failure, crash or not. """ @@ -25,20 +33,22 @@ class ResourceNotFoundError(ResourceError): class UnexpectedResourceError(ResourceError): """A resource read failed with something other than `ResourceError` or `MCPError`. - MCPServer raises this itself, around a crash in a resource or resource - template handler or a failed file read. You never raise it. `__cause__` is - the original exception, which the server logs with its traceback. The - message names only the URI, so the original text is withheld from the client. + The SDK raises this itself, around a crash in a resource or resource template + handler. You never raise it. `__cause__` is the original exception, which the + server logs with its traceback. The message names only the URI, so the + original text is withheld from the client. """ class ToolError(MCPServerError): - """A tool failure the model should read. + """A tool failure you anticipated. - Raise this from a tool (or a resolver) for a failure you anticipate: the + Raise this from a tool (or a resolver) for a failure you saw coming: the call returns `is_error=True` with the message in `content`, and the server logs it at INFO without a traceback. Any other exception reaches the model the same way but is treated as a crash and logged at ERROR with its traceback. + A `ResourceError` that escapes the tool (say from `ctx.read_resource()`) counts + as anticipated too. The SDK raises it too, for an unknown tool name and for arguments that fail the input schema, and `UnexpectedToolError` subclasses it, so `except ToolError` @@ -49,7 +59,7 @@ class ToolError(MCPServerError): class UnexpectedToolError(ToolError): """A tool call failed with something other than `ToolError` or `MCPError`. - MCPServer raises this itself, around a crash in the tool (or a resolver) or a + The SDK raises this itself, around a crash in the tool (or a resolver) or a return value that fails output conversion. You never raise it. `__cause__` is the original exception, which the server logs with its traceback before returning the usual `is_error=True` result. Catch it around diff --git a/src/mcp/server/mcpserver/resources/templates.py b/src/mcp/server/mcpserver/resources/templates.py index 1699d9a734..621b2e9448 100644 --- a/src/mcp/server/mcpserver/resources/templates.py +++ b/src/mcp/server/mcpserver/resources/templates.py @@ -245,6 +245,4 @@ async def create_resource( except (ResourceError, MCPError): raise except Exception as exc: - # Name only the URI: the original text is withheld from the client, and - # the server logs the traceback from `__cause__`. raise UnexpectedResourceError(f"Error creating resource from template {uri}") from exc diff --git a/src/mcp/server/mcpserver/resources/types.py b/src/mcp/server/mcpserver/resources/types.py index f77b32b610..c8b479bb78 100644 --- a/src/mcp/server/mcpserver/resources/types.py +++ b/src/mcp/server/mcpserver/resources/types.py @@ -16,10 +16,8 @@ from mcp_types import Annotations, Icon, InputRequiredResult from pydantic import Field, validate_call -from mcp.server.mcpserver.exceptions import ResourceError, UnexpectedResourceError from mcp.server.mcpserver.resources.base import Resource from mcp.shared._callable_inspection import is_async_callable -from mcp.shared.exceptions import MCPError # `application/*` types that are textual but predate the `+json`/`+xml` # structured-syntax suffixes, so the suffix rule below can't catch them. @@ -80,41 +78,29 @@ class FunctionResource(Resource): fn: Callable[[], Any] = Field(exclude=True) async def read(self) -> str | bytes: - """Read the resource by calling the wrapped function. - - Raises: - UnexpectedResourceError: If the function raises anything other than - `ResourceError` or `MCPError`. `__cause__` is the original exception. - """ - try: - fn = self.fn - if is_async_callable(fn): - result = await fn() - else: - result = await anyio.to_thread.run_sync(self.fn) - - if isinstance(result, InputRequiredResult): - # A static resource function can never read the retry's - # input_responses (it takes no Context), so this can only be a - # mistake — reject it instead of JSON-dumping it as content. - raise ValueError( - "static resources cannot return InputRequiredResult; only resource " - "template functions participate in the multi-round-trip flow" - ) - if isinstance(result, Resource): # pragma: no cover - return await result.read() - elif isinstance(result, bytes): - return result - elif isinstance(result, str): - return result - else: - return pydantic_core.to_json(result, fallback=str, indent=2).decode() - except (MCPError, ResourceError): - raise - except Exception as exc: - # Name only the URI: the original text is withheld from the client, and - # the server logs the traceback from `__cause__`. - raise UnexpectedResourceError(f"Error reading resource {self.uri}") from exc + """Read the resource by calling the wrapped function.""" + fn = self.fn + if is_async_callable(fn): + result = await fn() + else: + result = await anyio.to_thread.run_sync(self.fn) + + if isinstance(result, InputRequiredResult): + # A static resource function can never read the retry's + # input_responses (it takes no Context), so this can only be a + # mistake — reject it instead of JSON-dumping it as content. + raise ValueError( + "static resources cannot return InputRequiredResult; only resource " + "template functions participate in the multi-round-trip flow" + ) + if isinstance(result, Resource): # pragma: no cover + return await result.read() + elif isinstance(result, bytes): + return result + elif isinstance(result, str): + return result + else: + return pydantic_core.to_json(result, fallback=str, indent=2).decode() @classmethod def from_function( @@ -191,12 +177,9 @@ def validate_text_encoding(cls, encoding: str | None) -> str | None: async def read(self) -> str | bytes: """Read the file content.""" - try: - if self.encoding is None: - return await anyio.to_thread.run_sync(self.path.read_bytes) - return await anyio.to_thread.run_sync(partial(self.path.read_text, encoding=self.encoding)) - except Exception as exc: - raise UnexpectedResourceError(f"Error reading resource {self.uri}") from exc + if self.encoding is None: + return await anyio.to_thread.run_sync(self.path.read_bytes) + return await anyio.to_thread.run_sync(partial(self.path.read_text, encoding=self.encoding)) class HttpResource(Resource): @@ -236,18 +219,12 @@ def list_files(self) -> list[Path]: # pragma: no cover if not self.path.is_dir(): raise NotADirectoryError(f"Not a directory: {self.path}") - try: - if self.pattern: - return list(self.path.glob(self.pattern)) if not self.recursive else list(self.path.rglob(self.pattern)) - return list(self.path.glob("*")) if not self.recursive else list(self.path.rglob("*")) - except Exception as exc: - raise ValueError(f"Error listing directory {self.path}: {exc}") from exc + if self.pattern: + return list(self.path.glob(self.pattern)) if not self.recursive else list(self.path.rglob(self.pattern)) + return list(self.path.glob("*")) if not self.recursive else list(self.path.rglob("*")) async def read(self) -> str: # Always returns JSON string # pragma: no cover """Read the directory listing.""" - try: - files = await anyio.to_thread.run_sync(self.list_files) - file_list = [str(f.relative_to(self.path)) for f in files if f.is_file()] - return json.dumps({"files": file_list}, indent=2) - except Exception as exc: - raise UnexpectedResourceError(f"Error reading resource {self.uri}") from exc + files = await anyio.to_thread.run_sync(self.list_files) + file_list = [str(f.relative_to(self.path)) for f in files if f.is_file()] + return json.dumps({"files": file_list}, indent=2) diff --git a/src/mcp/server/mcpserver/server.py b/src/mcp/server/mcpserver/server.py index ae7844c0fd..ac5aebe344 100644 --- a/src/mcp/server/mcpserver/server.py +++ b/src/mcp/server/mcpserver/server.py @@ -427,10 +427,7 @@ async def _handle_call_tool( except MCPError: raise except Exception as exc: - # A ToolError (deliberate, unknown tool, rejected arguments) is an outcome - # the model already reads in full, so it is one INFO record, repr-quoted to - # keep peer-supplied text on one line. Anything else is a crash in the - # tool: log the traceback that the result text doesn't carry. + # %r keeps peer-supplied text (names, pydantic messages) on one line. if isinstance(exc, ToolError) and not isinstance(exc, UnexpectedToolError): logger.info("Tool %r failed: %r", params.name, str(exc)) else: @@ -449,9 +446,6 @@ async def _handle_read_resource( try: results = await self.read_resource(params.uri, context) except ResourceError as err: - # UnexpectedResourceError wraps a crash whose text is withheld from the - # client, so the traceback goes to the log. Any other ResourceError was - # raised on purpose (or is the SDK's "Unknown resource") and is one INFO record. if isinstance(err, UnexpectedResourceError): logger.exception("Resource %r raised an unexpected exception", str(params.uri)) else: @@ -584,20 +578,15 @@ async def read_resource( """ if context is None: context = Context(mcp_server=self, subscriptions=self._subscriptions) - resource = await self._resource_manager.get_resource(uri, context) - if isinstance(resource, InputRequiredResult): - return resource - try: + resource = await self._resource_manager.get_resource(uri, context) + if isinstance(resource, InputRequiredResult): + return resource content = await resource.read() return [ReadResourceContents(content=content, mime_type=resource.mime_type, meta=resource.meta)] except (MCPError, ResourceError): - # Includes the UnexpectedResourceError the built-in resource types raise - # around a crash in the function or the file read. raise except Exception as exc: - # A custom Resource subclass whose read() raised: wrap it the same way, - # naming only the URI so the original text is withheld from the client. raise UnexpectedResourceError(f"Error reading resource {uri}") from exc def add_tool( @@ -1326,10 +1315,7 @@ async def get_prompt( except MCPError: raise except Exception as e: - # Not logged here: this escapes `_handle_get_prompt` as-is, so the - # dispatcher boundary that turns it into the JSON-RPC error logs it once - # with its traceback (or, in-process with `raise_exceptions=True`, - # hands it to the caller instead). + # Not logged here: the dispatcher boundary logs it once. raise ValueError(str(e)) from e diff --git a/src/mcp/server/mcpserver/tools/base.py b/src/mcp/server/mcpserver/tools/base.py index cd556e9726..0768372c39 100644 --- a/src/mcp/server/mcpserver/tools/base.py +++ b/src/mcp/server/mcpserver/tools/base.py @@ -7,7 +7,13 @@ from mcp_types import Icon, InputRequiredResult, ToolAnnotations from pydantic import BaseModel, Field, ValidationError -from mcp.server.mcpserver.exceptions import InvalidSignature, ToolError, UnexpectedToolError +from mcp.server.mcpserver.exceptions import ( + InvalidSignature, + ResourceError, + ToolError, + UnexpectedResourceError, + UnexpectedToolError, +) from mcp.server.mcpserver.resolve import ( build_resolver_plans, find_resolved_parameters, @@ -133,17 +139,21 @@ async def run( Raises: ToolError: If the arguments fail validation against the input schema, or - the tool function (or a resolver) raises `ToolError`. - UnexpectedToolError: If the tool function (or a resolver) raises anything - other than `ToolError` or `MCPError`, or its return value fails output - conversion. + the tool function (or a resolver) raises `ToolError` or `ResourceError`. + UnexpectedToolError: If argument validation, the tool function, or a + resolver raises anything else, or the return value fails output conversion. """ try: validated = self.fn_metadata.validate_arguments(arguments) except ValidationError as exc: - # The caller's arguments don't match the input schema. That is the model's - # mistake to read and correct, so it is reported like a deliberate ToolError. + # The caller's arguments don't match the input schema: the model's mistake + # to read and correct, so it is reported like a deliberate ToolError. raise ToolError(f"Error executing tool {self.name}: {exc}") from exc + except MCPError: + raise + except Exception as exc: + # A custom validator or default_factory that raises is a crash. + raise UnexpectedToolError(f"Error executing tool {self.name}: {exc}") from exc try: pass_directly: dict[str, Any] = {} @@ -191,12 +201,12 @@ async def run( # `CallToolResult(isError=True)` execution failure. raise # Everything else reaches the model as an is_error result under this tool's - # name. The wrapper's type is what tells the server whether to log a crash. - except UnexpectedToolError as exc: - # A nested tool call crashed: still a crash under this tool's name. + # name, and the wrapper's type tells the server whether to log a crash. + except (UnexpectedToolError, UnexpectedResourceError) as exc: + # A nested tool call or resource read crashed: still a crash here. raise UnexpectedToolError(f"Error executing tool {self.name}: {exc}") from exc - except ToolError as exc: - # Raised deliberately by the tool or a resolver: anticipated. + except (ToolError, ResourceError) as exc: + # Raised deliberately by the tool, a resolver, or a resource it read. raise ToolError(f"Error executing tool {self.name}: {exc}") from exc except Exception as exc: raise UnexpectedToolError(f"Error executing tool {self.name}: {exc}") from exc diff --git a/tests/docs_src/test_uri_templates.py b/tests/docs_src/test_uri_templates.py index 03e12d8174..16744fd447 100644 --- a/tests/docs_src/test_uri_templates.py +++ b/tests/docs_src/test_uri_templates.py @@ -139,6 +139,19 @@ async def test_safe_join_serves_a_file_inside_the_base_directory( assert content.text == "# Printer setup" +async def test_a_missing_manual_is_resource_not_found(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """tutorial002 and the closing tip: a path with no file behind it is `-32602` with the handler's message.""" + monkeypatch.setattr(tutorial002, "DOCS_ROOT", tmp_path) + async with Client(tutorial002.mcp) as client: + with pytest.raises(MCPError) as exc: + await client.read_resource("manuals://printing/missing.md") + assert exc.value.error == ErrorData( + code=INVALID_PARAMS, + message="No manual at 'printing/missing.md'.", + data={"uri": "manuals://printing/missing.md"}, + ) + + def test_safe_join_raises_when_the_resolved_path_escapes_the_base(tmp_path: Path) -> None: """tutorial002: a path that climbs out of `DOCS_ROOT` raises `PathEscapeError`.""" with pytest.raises(PathEscapeError): diff --git a/tests/interaction/_requirements.py b/tests/interaction/_requirements.py index 86725bcb4f..38b250c505 100644 --- a/tests/interaction/_requirements.py +++ b/tests/interaction/_requirements.py @@ -1307,8 +1307,15 @@ def __post_init__(self) -> None: "mcpserver:resource:read-throws-surfaced": Requirement( source="sdk", behavior=( - "A resource function that raises is surfaced to the caller as a JSON-RPC error response " - "(-32603 Internal error), with the original exception text withheld." + "A resource function that raises an unexpected exception is surfaced to the caller as a JSON-RPC " + "error response (-32603 Internal error), with the original exception text withheld." + ), + ), + "mcpserver:resource:static-not-found": Requirement( + source="sdk", + behavior=( + "A static (fixed-URI) resource function that raises ResourceNotFoundError is surfaced as -32602 " + "with the handler's message and the URI in data, the same as from a template function." ), ), "mcpserver:resource:static": Requirement( diff --git a/tests/interaction/mcpserver/test_resources.py b/tests/interaction/mcpserver/test_resources.py index eadf4794e6..914d3cbbeb 100644 --- a/tests/interaction/mcpserver/test_resources.py +++ b/tests/interaction/mcpserver/test_resources.py @@ -14,6 +14,7 @@ from mcp import MCPError from mcp.server.mcpserver import MCPServer +from mcp.server.mcpserver.exceptions import ResourceNotFoundError from tests._stamp import Unstamp from tests.interaction._connect import Connect from tests.interaction._requirements import requirement @@ -152,6 +153,28 @@ def boom() -> str: ) +@requirement("mcpserver:resource:static-not-found") +async def test_static_resource_function_raising_not_found_is_invalid_params(connect: Connect) -> None: + """ResourceNotFoundError from a fixed-URI resource function reaches the caller as -32602 with its message. + + A static resource can still be absent (a report not generated yet, a file that comes and goes), + and the handler's message passes through exactly as it does from a template function. + """ + mcp = MCPServer("library") + + @mcp.resource("reports://latest") + def latest() -> str: + raise ResourceNotFoundError("no report has been generated yet") + + async with connect(mcp) as client: + with pytest.raises(MCPError) as exc_info: + await client.read_resource("reports://latest") + + assert exc_info.value.error == snapshot( + ErrorData(code=-32602, message="no report has been generated yet", data={"uri": "reports://latest"}) + ) + + @requirement("mcpserver:resource:duplicate-name") async def test_registering_a_duplicate_resource_uri_warns_and_keeps_the_first( connect: Connect, unstamped: Unstamp diff --git a/tests/server/mcpserver/resources/test_file_resources.py b/tests/server/mcpserver/resources/test_file_resources.py index 674b165ec0..8149f5bee0 100644 --- a/tests/server/mcpserver/resources/test_file_resources.py +++ b/tests/server/mcpserver/resources/test_file_resources.py @@ -6,7 +6,6 @@ import pytest from pydantic import ValidationError -from mcp.server.mcpserver.exceptions import UnexpectedResourceError from mcp.server.mcpserver.resources import FileResource @@ -179,10 +178,8 @@ async def test_missing_file_error(temp_file: Path): name="test", path=missing, ) - with pytest.raises(UnexpectedResourceError) as exc: + with pytest.raises(FileNotFoundError): await resource.read() - assert str(exc.value) == "Error reading resource file:///missing.txt" - assert isinstance(exc.value.__cause__, FileNotFoundError) @pytest.mark.skipif(os.name == "nt", reason="File permissions behave differently on Windows") @@ -195,8 +192,7 @@ async def test_permission_error(temp_file: Path): # pragma: lax no cover name="test", path=temp_file, ) - with pytest.raises(UnexpectedResourceError) as exc: + with pytest.raises(PermissionError): await resource.read() - assert isinstance(exc.value.__cause__, PermissionError) finally: temp_file.chmod(0o644) # Restore permissions diff --git a/tests/server/mcpserver/resources/test_function_resources.py b/tests/server/mcpserver/resources/test_function_resources.py index dc57dbc31c..d38ddd840c 100644 --- a/tests/server/mcpserver/resources/test_function_resources.py +++ b/tests/server/mcpserver/resources/test_function_resources.py @@ -7,7 +7,6 @@ from mcp_types import InputRequiredResult from pydantic import BaseModel -from mcp.server.mcpserver.exceptions import UnexpectedResourceError from mcp.server.mcpserver.resources import FunctionResource @@ -81,22 +80,19 @@ def get_data() -> dict[str, str]: @pytest.mark.anyio async def test_error_handling(self): - """A crash in the function is wrapped as UnexpectedResourceError naming only the URI, - with the function's own exception as `__cause__`.""" - raised = ValueError("Test error") + """read() lets the function's own exception propagate; MCPServer.read_resource does the wrapping.""" def failing_func() -> str: - raise raised + raise ValueError("Test error") resource = FunctionResource( uri="function://test", name="test", fn=failing_func, ) - with pytest.raises(UnexpectedResourceError) as exc: + with pytest.raises(ValueError) as exc: await resource.read() - assert str(exc.value) == snapshot("Error reading resource function://test") - assert exc.value.__cause__ is raised + assert str(exc.value) == "Test error" @pytest.mark.anyio async def test_basemodel_conversion(self): @@ -260,10 +256,9 @@ def ask() -> InputRequiredResult: return InputRequiredResult(request_state="round-1") resource = FunctionResource(uri="resource://ask", name="ask", fn=ask) - with pytest.raises(UnexpectedResourceError) as exc: + with pytest.raises(ValueError) as exc: await resource.read() - assert str(exc.value) == snapshot("Error reading resource resource://ask") - assert str(exc.value.__cause__) == snapshot( + assert str(exc.value) == snapshot( "static resources cannot return InputRequiredResult; " "only resource template functions participate in the multi-round-trip flow" ) diff --git a/tests/server/mcpserver/test_resolve.py b/tests/server/mcpserver/test_resolve.py index aa5ced266a..966bb94ed1 100644 --- a/tests/server/mcpserver/test_resolve.py +++ b/tests/server/mcpserver/test_resolve.py @@ -1,6 +1,7 @@ """Tests for resolver dependency injection (MRTR) on MCPServer tools.""" import json +import logging from collections.abc import Callable from datetime import datetime from typing import Annotated, Any, Literal, TypeVar, cast @@ -1761,10 +1762,13 @@ async def listy(login: Annotated[Login, Resolve(lookup)]) -> list[str]: @pytest.mark.anyio -async def test_tool_returning_input_required_dynamically_with_resolvers_is_an_error(): +async def test_tool_returning_input_required_dynamically_with_resolvers_is_an_error( + caplog: pytest.LogCaptureFixture, +): # The annotated form of this combination is rejected at registration; a body # that returns an InputRequiredResult without declaring it fails loudly at the # same boundary instead of silently fighting the resolvers for the channel. + # It is an authoring bug, so it is logged as a crash rather than at INFO. mcp = MCPServer(name="DynamicChannelClash", request_state_security=RequestStateSecurity.ephemeral()) async def lookup(ctx: Context) -> Login: @@ -1774,11 +1778,14 @@ async def lookup(ctx: Context) -> Login: async def sneaky(login: Annotated[Login, Resolve(lookup)]): return InputRequiredResult(input_requests={}, request_state="opaque") + caplog.set_level(logging.INFO) async with Client(mcp) as client: result = await client.call_tool("sneaky", {}) assert result.is_error assert isinstance(result.content[0], TextContent) assert "the multi-round flow is driven either by resolvers or by the tool body" in result.content[0].text + records = [(r.levelname, r.getMessage()) for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert records == [("ERROR", "Tool 'sneaky' raised an unexpected exception")] def test_question_digest_pins_the_rendered_question(): diff --git a/tests/server/mcpserver/test_server.py b/tests/server/mcpserver/test_server.py index 15c2e8692e..a97117c54f 100644 --- a/tests/server/mcpserver/test_server.py +++ b/tests/server/mcpserver/test_server.py @@ -43,7 +43,7 @@ TextContent, TextResourceContents, ) -from pydantic import BaseModel, ValidationError +from pydantic import AfterValidator, BaseModel, ValidationError from starlette.applications import Starlette from starlette.routing import Mount, Route from typing_extensions import NotRequired, TypedDict @@ -2256,11 +2256,11 @@ def thing() -> str: def _cause_chain(exc: BaseException | None) -> list[BaseException]: - """`exc` and everything it chains back to, explicitly (`__cause__`) or implicitly (`__context__`).""" + """`exc` and everything it explicitly chains back to via `__cause__` (`raise ... from ...`).""" chain: list[BaseException] = [] while exc is not None: chain.append(exc) - exc = exc.__cause__ or exc.__context__ + exc = exc.__cause__ return chain @@ -2508,6 +2508,137 @@ async def whoami(user: Annotated[str, Resolve(current_user)]) -> str: assert raised in _cause_chain(_logged_exception(caplog)) +async def test_argument_validator_that_crashes_is_the_tools_crash(caplog: pytest.LogCaptureFixture): + """SDK-defined: pydantic only turns ValueError/AssertionError into ValidationError, so a validator + raising anything else is a bug in the tool's schema and is wrapped and logged as a crash.""" + mcp = MCPServer() + raised = TypeError("codes are compared as integers") + + def check(code: str) -> str: + raise raised + + @mcp.tool() + def redeem(code: Annotated[str, AfterValidator(check)]) -> str: + raise NotImplementedError + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("redeem", {"code": "SAVE10"}) + with pytest.raises(UnexpectedToolError) as exc: + await mcp.call_tool("redeem", {"code": "SAVE10"}) + + assert result.content == [ + TextContent(type="text", text="Error executing tool redeem: codes are compared as integers") + ] + assert exc.value.__cause__ is raised + assert _server_records(caplog) == snapshot([("ERROR", "Tool 'redeem' raised an unexpected exception", True)]) + + +async def test_argument_validator_raising_mcp_error_is_a_protocol_error(caplog: pytest.LogCaptureFixture): + """SDK-defined: MCPError keeps its meaning wherever it is raised, including inside an argument + validator: the request fails with that code and MCPServer logs nothing.""" + mcp = MCPServer() + + def check(code: str) -> str: + raise MCPError(code=INVALID_PARAMS, message="codes are issued per session") + + @mcp.tool() + def redeem(code: Annotated[str, AfterValidator(check)]) -> str: + raise NotImplementedError + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + with pytest.raises(MCPError) as exc: + await client.call_tool("redeem", {"code": "SAVE10"}) + + assert exc.value.error == snapshot(ErrorData(code=INVALID_PARAMS, message="codes are issued per session")) + assert _server_records(caplog) == [] + + +async def test_resource_error_escaping_a_tool_is_anticipated(caplog: pytest.LogCaptureFixture): + """SDK-defined: a tool that lets ResourceNotFoundError from ctx.read_resource() propagate has + reported an anticipated failure, so it is INFO here just as it is for resources/read.""" + mcp = MCPServer() + + @mcp.resource("books://{title}") + def book(title: str) -> str: + raise ResourceNotFoundError(f"No book titled {title!r}.") + + @mcp.tool() + async def summarise(title: str, ctx: Context) -> str: + await ctx.read_resource(f"books://{title}") + raise NotImplementedError + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("summarise", {"title": "Nothing"}) + with pytest.raises(ToolError) as exc: + await mcp.call_tool("summarise", {"title": "Nothing"}) + + assert result.content == [ + TextContent(type="text", text="Error executing tool summarise: No book titled 'Nothing'.") + ] + assert type(exc.value) is ToolError + assert _server_records(caplog) == snapshot( + [("INFO", "Tool 'summarise' failed: \"Error executing tool summarise: No book titled 'Nothing'.\"", False)] + ) + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + +async def test_resource_crash_escaping_a_tool_is_the_tools_crash(caplog: pytest.LogCaptureFixture): + """SDK-defined: a crashing resource read inside a tool stays a crash under the tool's name, logged + once, with the traceback reaching the resource function's own exception.""" + mcp = MCPServer() + raised = ConnectionError("catalog database unreachable") + + @mcp.resource("books://{title}") + def book(title: str) -> str: + raise raised + + @mcp.tool() + async def summarise(title: str, ctx: Context) -> str: + await ctx.read_resource(f"books://{title}") + raise NotImplementedError + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("summarise", {"title": "Dune"}) + + assert result.content == [ + TextContent( + type="text", + text="Error executing tool summarise: Error creating resource from template books://Dune", + ) + ] + assert _server_records(caplog) == snapshot([("ERROR", "Tool 'summarise' raised an unexpected exception", True)]) + assert raised in _cause_chain(_logged_exception(caplog)) + + +async def test_tool_that_recovers_from_a_missing_resource_logs_nothing(caplog: pytest.LogCaptureFixture): + """SDK-defined: MCPServer.read_resource() itself writes no record, so a tool that catches + ResourceNotFoundError and carries on leaves the log clean.""" + mcp = MCPServer() + + @mcp.resource("books://{title}") + def book(title: str) -> str: + raise ResourceNotFoundError(f"No book titled {title!r}.") + + @mcp.tool() + async def summarise(title: str, ctx: Context) -> str: + try: + await ctx.read_resource(f"books://{title}") + except ResourceNotFoundError: + return "not in the catalog" + raise NotImplementedError + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + result = await client.call_tool("summarise", {"title": "Nothing"}) + + assert result.content == [TextContent(type="text", text="not in the catalog")] + assert _server_records(caplog) == [] + + async def test_static_resource_raising_unexpected_exception_is_logged_once_at_error_with_its_traceback( caplog: pytest.LogCaptureFixture, ): From 9adfe62c5126875c744a10f57759a551b441c05c Mon Sep 17 00:00:00 2001 From: Max <224885523+maxisbey@users.noreply.github.com> Date: Wed, 19 Aug 2026 11:43:45 +0100 Subject: [PATCH 4/8] remove previous exception interpolation from raised exception Co-authored-by: Marcelo Trylesinski --- src/mcp/server/mcpserver/prompts/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mcp/server/mcpserver/prompts/base.py b/src/mcp/server/mcpserver/prompts/base.py index 7028dd8f0b..a43f452df3 100644 --- a/src/mcp/server/mcpserver/prompts/base.py +++ b/src/mcp/server/mcpserver/prompts/base.py @@ -210,4 +210,4 @@ async def render( except MCPError: raise except Exception as exc: - raise ValueError(f"Error rendering prompt {self.name}: {exc}") from exc + raise ValueError(f"Error rendering prompt {self.name}) from exc From 970fa74a496b41c3a4c852e7c74b160ad4b342c9 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Wed, 19 Aug 2026 10:51:15 +0000 Subject: [PATCH 5/8] Close the f-string in Prompt.render and pin the shorter message 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 ", the legacy-path interaction test snapshots that instead of matching the pydantic prefix. --- src/mcp/server/mcpserver/prompts/base.py | 2 +- tests/interaction/mcpserver/test_prompts.py | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/mcp/server/mcpserver/prompts/base.py b/src/mcp/server/mcpserver/prompts/base.py index a43f452df3..a13b72f1aa 100644 --- a/src/mcp/server/mcpserver/prompts/base.py +++ b/src/mcp/server/mcpserver/prompts/base.py @@ -210,4 +210,4 @@ async def render( except MCPError: raise except Exception as exc: - raise ValueError(f"Error rendering prompt {self.name}) from exc + raise ValueError(f"Error rendering prompt {self.name}") from exc diff --git a/tests/interaction/mcpserver/test_prompts.py b/tests/interaction/mcpserver/test_prompts.py index 8409e50207..3872858b4b 100644 --- a/tests/interaction/mcpserver/test_prompts.py +++ b/tests/interaction/mcpserver/test_prompts.py @@ -123,8 +123,8 @@ async def test_get_prompt_with_a_wrong_type_argument_is_rejected_before_the_func The decorated function is wrapped in pydantic's validate_call, so a value that cannot be coerced to the parameter's annotation fails before the body executes. The function body - raises NotImplementedError to prove it never ran. The error is wrapped in the SDK's stable - rendering-error prefix; the body of the message is raw pydantic output and is not asserted. + raises NotImplementedError to prove it never ran. The client sees only the SDK's + rendering-error message naming the prompt, with the pydantic detail withheld. """ mcp = MCPServer("prompter") @@ -137,8 +137,7 @@ def repeat(phrase: str, count: int) -> str: with pytest.raises(MCPError) as exc_info: await client.get_prompt("repeat", {"phrase": "hi", "count": "many"}) - assert exc_info.value.error.code == 0 - assert exc_info.value.error.message.startswith("Error rendering prompt repeat: 1 validation error") + assert exc_info.value.error == snapshot(ErrorData(code=0, message="Error rendering prompt repeat")) @requirement("mcpserver:prompt:optional-args") From 9e6d1d95f3898cf3a9e176a29147fbe6df934762 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:02:19 +0000 Subject: [PATCH 6/8] Drop the migration.md addition; the guide is closed to new entries 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 --- docs/migration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/migration.md b/docs/migration.md index ed1b2a58b5..e59b4a6ac6 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -1018,7 +1018,7 @@ except MCPError as e: Reading a missing resource now returns JSON-RPC error code `-32602` (invalid params) with the requested URI in `error.data` (`{"uri": ...}`), per [SEP-2164](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2164). Previously the server returned code `0` with no `data`. Clients can now reliably distinguish not-found from other errors; a resource handler (static or template) that raises `ResourceNotFoundError` (from `mcp.server.mcpserver.exceptions`) produces this same response. -The underlying lookups now raise typed exceptions instead of `ValueError`. `ResourceManager.get_resource()` raises `ResourceNotFoundError` when no resource or template matches the URI, and `ResourceTemplate.create_resource()` raises `ResourceError` when the template function fails. Neither subclasses `ValueError`, so callers catching `ValueError` should switch to `ResourceNotFoundError` / `ResourceError` (both importable from `mcp.server.mcpserver.exceptions`; `ResourceNotFoundError` subclasses `ResourceError`). Likewise, `FunctionResource.read()` and `FileResource.read()` no longer wrap failures in `ValueError`: called directly they raise whatever the function or file read raised, and through `MCPServer.read_resource()` that arrives as `UnexpectedResourceError` (a `ResourceError`) with the original as `__cause__`. +The underlying lookups now raise typed exceptions instead of `ValueError`. `ResourceManager.get_resource()` raises `ResourceNotFoundError` when no resource or template matches the URI, and `ResourceTemplate.create_resource()` raises `ResourceError` when the template function fails. Neither subclasses `ValueError`, so callers catching `ValueError` should switch to `ResourceNotFoundError` / `ResourceError` (both importable from `mcp.server.mcpserver.exceptions`; `ResourceNotFoundError` subclasses `ResourceError`). ### `Resource` classes reject unknown keyword arguments From ab89da82ba32e96376ab7a89152ab0253a77b1aa Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Wed, 19 Aug 2026 16:05:28 +0000 Subject: [PATCH 7/8] Keep unexpected exception text out of tool results A tool that crashed used to send the exception's own text to the client as "Error executing tool : ". 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 ". 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 " 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. --- docs/client/index.md | 11 +-- docs/deprecated.md | 5 +- docs/handlers/elicitation.md | 7 +- docs/handlers/logging.md | 2 +- docs/migration.md | 2 +- docs/servers/handling-errors.md | 74 ++++++++++--------- docs/servers/structured-output.md | 13 ++-- docs/troubleshooting.md | 8 +- docs/whats-new.md | 2 +- docs_src/client/tutorial003.py | 3 +- docs_src/handling_errors/tutorial001.py | 3 +- docs_src/handling_errors/tutorial004.py | 3 - docs_src/troubleshooting/tutorial001.py | 4 +- src/mcp/server/mcpserver/exceptions.py | 17 +++-- src/mcp/server/mcpserver/resolve.py | 7 +- src/mcp/server/mcpserver/server.py | 21 +++++- src/mcp/server/mcpserver/tools/base.py | 14 ++-- tests/docs_src/test_client.py | 2 +- tests/docs_src/test_deprecated.py | 18 +++-- tests/docs_src/test_elicitation.py | 22 ++++-- tests/docs_src/test_handling_errors.py | 68 ++++++++--------- tests/docs_src/test_structured_output.py | 16 ++-- tests/docs_src/test_troubleshooting.py | 22 ++++-- tests/interaction/_requirements.py | 5 +- tests/interaction/mcpserver/test_tools.py | 28 ++++--- tests/server/mcpserver/test_resolve.py | 8 +- tests/server/mcpserver/test_server.py | 70 ++++++++++++++---- .../test_url_elicitation_error_throw.py | 2 +- 28 files changed, 277 insertions(+), 180 deletions(-) diff --git a/docs/client/index.md b/docs/client/index.md index b1a1dbc234..767f9eb06a 100644 --- a/docs/client/index.md +++ b/docs/client/index.md @@ -81,7 +81,7 @@ That schema is everything a UI needs to render an argument form, and everything `call_tool(name, arguments)` runs the tool and gives you back a `CallToolResult`. -```python title="client.py" hl_lines="26-33" +```python title="client.py" hl_lines="27-34" --8<-- "docs_src/client/tutorial003.py" ``` @@ -113,7 +113,7 @@ A tool that raises does **not** raise in your client. It comes back as an ordina !!! check Ask `lookup_book` for `"Solaris"` (a title that isn't in the catalog) and the function raises - `ValueError`. The call still returns normally: + `ToolError`. The call still returns normally: ```python result.is_error # True @@ -121,9 +121,10 @@ A tool that raises does **not** raise in your client. It comes back as an ordina result.structured_content # None ``` - The exception's message landed in `content`, where the **model** can read it and try again. That - is deliberate: a tool error is part of the conversation, not a crash. Always look at `is_error` - before you trust `structured_content`. + The `ToolError`'s message landed in `content`, where the **model** can read it and try again. That + is deliberate: a tool error is part of the conversation, not a crash. (Had the tool crashed with + some other exception, `content` would say only `Error executing tool lookup_book`.) Always look at + `is_error` before you trust `structured_content`. !!! warning `is_error=True` covers more than your own `raise`. Ask for a tool the server doesn't even have diff --git a/docs/deprecated.md b/docs/deprecated.md index 71844aa5e2..6cb2a3c82c 100644 --- a/docs/deprecated.md +++ b/docs/deprecated.md @@ -119,10 +119,11 @@ That is the whole API. There is no per-method switch, and you don't want one: th Run the filter the other way and you get a free regression test. Add `"error::mcp.MCPDeprecationWarning"` to the `filterwarnings` setting in your pytest configuration and the deprecated call **raises** instead of warning. A tool named - `old_log` that still calls `ctx.info()` stops passing and starts reporting: + `old_log` that still calls `ctx.info()` stops passing: the call comes back `is_error=True` with + `Error executing tool old_log`, and the captured server log names the culprit: ```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). ``` One line of pytest configuration, and a deprecated call can never sneak back into your diff --git a/docs/handlers/elicitation.md b/docs/handlers/elicitation.md index c9a0a4fabc..478e4824fa 100644 --- a/docs/handlers/elicitation.md +++ b/docs/handlers/elicitation.md @@ -84,7 +84,8 @@ That schema is the form. `Field(description=...)` is the label; a default pre-fi !!! warning An elicitation schema is not as expressive as a tool's input schema. Flat, primitive fields only: `str`, `int`, `float`, `bool`, or a `Literal` of strings (it becomes an `enum`). - Put a model inside the model and `ctx.elicit` raises before anything is sent to the client: + Put a model inside the model and `ctx.elicit` raises before anything is sent to the client. + The tool call fails with `Error executing tool `, and your server log has the reason: ```text TypeError: Elicitation schema field 'address' rendered as {'$ref': '#/$defs/Address'}, which is not a valid PrimitiveSchemaDefinition @@ -107,8 +108,8 @@ A refusal is not an error. The tool decides what declining means (here, no booki !!! tip The answer is validated against your model before your code sees it. A client that sends - `"maybe"` for a `bool` doesn't corrupt your booking: the call fails with a - schema-mismatch error, your `if` never runs. + `"maybe"` for a `bool` doesn't corrupt your booking: `ctx.elicit` raises `ValueError`, the call + fails, and your `if` never runs. ## Send the user to a URL diff --git a/docs/handlers/logging.md b/docs/handlers/logging.md index 2370750d82..dd5540a71b 100644 --- a/docs/handlers/logging.md +++ b/docs/handlers/logging.md @@ -49,7 +49,7 @@ The default is `"INFO"`. `logging.basicConfig()` never replaces handlers that already exist. If you configure logging yourself before creating the server, your configuration wins. -You also don't need a `try`/`except` in every handler just to record failures. When a tool or resource function raises, the SDK logs it for you. **[Handling errors](../servers/handling-errors.md#what-the-server-logs)** explains what gets logged and at which level. +You also don't need a `try`/`except` in every handler just to record failures. When a tool or resource function raises, the SDK logs it for you. **[Handling errors](../servers/handling-errors.md#any-other-exception)** explains what gets logged and at which level. ## Try it diff --git a/docs/migration.md b/docs/migration.md index e59b4a6ac6..f231710ad7 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -2737,7 +2737,7 @@ One behavioral caveat when moving progress-reporting handlers onto `Client(serve Every deprecation below is a runtime warning as well as a type-checker one: deprecated methods and helpers emit `mcp.MCPDeprecationWarning` on each call, and the deprecated `Server(...)` constructor parameters (`on_set_logging_level`, `on_roots_list_changed`, `on_progress`) emit it at construction time. The category subclasses `UserWarning`, not `DeprecationWarning`, so it is visible by default; [Deprecated features](deprecated.md) has the full list and each replacement. -Under pytest's `filterwarnings = ["error"]`, that warning becomes an exception at the first deprecated call. Inside an `@mcp.tool()` handler the exception is caught like any other and returned as `CallToolResult(is_error=True)` (`Error executing tool ...: The logging capability is deprecated as of 2026-07-28 (SEP-2577).`), which reads as a failing tool rather than a warning. Keep the warnings visible but non-fatal with: +Under pytest's `filterwarnings = ["error"]`, that warning becomes an exception at the first deprecated call. Inside an `@mcp.tool()` handler the exception is caught like any other and returned as `CallToolResult(is_error=True)` (`Error executing tool ...`, with the `MCPDeprecationWarning` traceback in the server log), which reads as a failing tool rather than a warning. Keep the warnings visible but non-fatal with: ```toml [tool.pytest.ini_options] diff --git a/docs/servers/handling-errors.md b/docs/servers/handling-errors.md index 005396ace9..a923b8c1de 100644 --- a/docs/servers/handling-errors.md +++ b/docs/servers/handling-errors.md @@ -1,8 +1,8 @@ # Handling errors -A tool can fail in two ways, and the SDK treats them very differently. +A tool can fail in three ways, and the SDK treats each differently. -Raise an ordinary exception and the **model** sees it. Raise `MCPError` and the **protocol** sees it. +Raise `ToolError` and the **model** sees your message. Raise `MCPError` and the **protocol** sees it. Raise anything else and it is a crash: the model learns only that the call failed, and your log gets the traceback. This page is about choosing. @@ -10,11 +10,11 @@ This page is about choosing. Take a tool that looks something up, and let the lookup miss: -```python title="server.py" hl_lines="11-12" +```python title="server.py" hl_lines="2 12-13" --8<-- "docs_src/handling_errors/tutorial001.py" ``` -There is nothing MCP about those two lines. `get_author` raises a plain `ValueError`, the way any Python function would. +`ToolError`, from `mcp.server.mcpserver.exceptions`, is how a tool tells the model that something went wrong. Call it with a title that isn't in the catalog and look at the result: @@ -25,13 +25,15 @@ result.structured_content # None ``` * The request **succeeded**. There is a result; nothing was raised at the caller. -* `is_error` is `True`, and your exception's message (prefixed with the tool name) is in `content`, exactly where the model reads. +* `is_error` is `True`, and your message (prefixed with the tool name) is in `content`, exactly where the model reads. * `structured_content` is `None`. A failed call has no return value to structure. -This is a **tool error**, and it is the default for *any* exception your tool raises. It is also almost always what you want. +This is a **tool error**, and it is almost always what you want. The model is the one calling your tool. It picked the arguments. So a tool error is a turn in the conversation: the model reads *"No book titled 'Nothing' in the catalog."*, realises it guessed the title wrong, and calls again with a better one. You wrote one `raise` and got a self-correcting agent. +On the server, a `ToolError` is one `INFO` line in the log, with no traceback. You saw it coming, so there is nothing to investigate. + !!! tip Never `return` an error message from a tool. A returned string has `is_error=False`, so to the model (and to every client UI) it looks like the tool worked and that string was the answer. @@ -39,7 +41,7 @@ The model is the one calling your tool. It picked the arguments. So a tool error ## An error the model cannot fix -Now swap `ValueError` for `MCPError`. +Now swap `ToolError` for `MCPError`. ```python title="server.py" hl_lines="1 3 14" --8<-- "docs_src/handling_errors/tutorial002.py" @@ -72,10 +74,10 @@ Now swap `ValueError` for `MCPError`. The two paths answer two different questions. -* **Raise any exception** for a failure of *execution*: the thing your tool tried to do didn't work. The model chose the call, so the model should see the consequence and get a chance to recover. A misspelled title, an upstream API that timed out, a row that doesn't exist: all tool errors. +* **Raise `ToolError`** for a failure of *execution*: the thing your tool tried to do didn't work. The model chose the call, so the model should see the consequence and get a chance to recover. A misspelled title, an upstream API that timed out, a row that doesn't exist: all tool errors. * **Raise `MCPError`** when the *request itself* should be rejected: the client is missing a capability your tool depends on, the server isn't in a state to serve anyone, the caller skipped a required step. No retry from the model fixes any of those, so there is nothing to gain from handing it the message. -One question decides it: **could a smarter model have avoided this?** Yes -> ordinary exception. No -> `MCPError`. +One question decides it: **could a smarter model have avoided this?** Yes -> `ToolError`. No -> `MCPError`. By that test, the second version of `get_author` made the wrong choice: a better title fixes it, so the model deserved to see the message. It's there to show you the mechanism, not to recommend it. @@ -84,6 +86,25 @@ By that test, the second version of `get_author` made the wrong choice: a better `data` payload. Whatever you put in them is what the client receives: the SDK forwards a raised `MCPError` verbatim instead of sanitising it. +## Any other exception + +Now take the check out and let the dictionary lookup fail on its own: + +```python title="server.py" hl_lines="11" +--8<-- "docs_src/handling_errors/tutorial004.py" +``` + +`CATALOG[title]` raises `KeyError`. You didn't plan for it, so the SDK treats it as a crash: + +```python +result.is_error # True +result.content # [TextContent(text="Error executing tool get_author")] +``` + +The call still returns `is_error=True`, so the model knows it failed and can move on. What it doesn't get is the exception's text: a `KeyError` from your code, or a stack of SQL from a driver three libraries down, may describe your server's internals, so it never leaves the server. + +You get it instead. The server logs the crash at `ERROR` with the full traceback, as `Tool 'get_author' raised an unexpected exception`. A production log at `WARNING` therefore stays quiet through every `ToolError` and speaks up the moment something is actually broken. + ## A resource that doesn't exist Resources draw the same line, and ship one named exception for the common case. @@ -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](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)**. ## Errors you never raise @@ -115,36 +136,21 @@ Send `get_author` a `title` that isn't a string and the SDK rejects it against t It means a whole class of `raise` statements you don't write: don't re-validate your own type hints. !!! info - Everything so far 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 the - `is_error=True` result. Assert on the result. If you need the traceback, it is in the server's - log (next section), and pytest's `caplog` captures it. **[Testing](../get-started/testing.md)** covers the pattern. - -## What the server logs - -The server also logs tool and resource failures, and how it logs them depends on whether you anticipated the failure. - -`get_author` raised a plain `ValueError`. The model got the message, but the SDK can't tell that you raised it on purpose, so it treats the call as a crash and logs it at `ERROR` with the full traceback. That is what you want on the day the exception is a `KeyError` from deep inside a library and the result text says only `'id'`. - -When the failure is one you planned for, say so with `ToolError`: - -```python title="server.py" hl_lines="2 12-13" ---8<-- "docs_src/handling_errors/tutorial004.py" -``` - -`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 + the `is_error=True` result. Assert on the result. If you need the traceback of a crash, it is in + the server's log, and pytest's `caplog` captures it. **[Testing](../get-started/testing.md)** covers the pattern. ## Recap -* Raise **any exception** in a tool -> the call returns `is_error=True` with your message in `content`. The model reads it and can retry. This is the default. +* Raise **`ToolError`** in a tool -> the call returns `is_error=True` with your message in `content`. The model reads it and can retry. * Raise **`MCPError`** -> the call itself fails with a JSON-RPC error. The model sees nothing; the host deals with it. `code`, `message`, and `data` survive intact. -* The deciding question: *could a smarter model have avoided this?* Yes -> exception. No -> `MCPError`. +* The deciding question: *could a smarter model have avoided this?* Yes -> `ToolError`. No -> `MCPError`. +* Any **other exception** is a crash -> `is_error=True` with only `Error executing tool ` for the model, and an `ERROR` record with the traceback for you. * `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)**. diff --git a/docs/servers/structured-output.md b/docs/servers/structured-output.md index 3964897cd1..792bcb0c6c 100644 --- a/docs/servers/structured-output.md +++ b/docs/servers/structured-output.md @@ -182,18 +182,19 @@ You don't notice while you build the value by hand: Pydantic already made sure y The annotation promises `WeatherData`. The upstream response stopped sending `humidity`. !!! check - Call `get_weather` and it does not quietly hand the client a half-empty object. The call fails, - and the first lines of the error name the field: + Call `get_weather` and it does not quietly hand the client a half-empty object. The call fails: + the client gets `is_error=True` with `Error executing tool get_weather`, so the model knows the + call failed instead of confidently reading weather that isn't there. The field name is for you, + in the server log at `ERROR`: ```text - Error executing tool get_weather: 1 validation error for WeatherData + Tool 'get_weather' raised an unexpected exception + ... + pydantic_core._pydantic_core.ValidationError: 1 validation error for WeatherData humidity Field required [type=missing, input_value={'temperature': 16.2, 'conditions': 'Overcast'}, input_type=dict] ``` - That text comes back as the tool result with `is_error=True`, so the model knows the call failed - instead of confidently reading weather that isn't there. - Returning a plain `dict` from a `-> WeatherData` tool is fine, by the way. That's exactly what `json.loads` produced. Validation is on the value, not on the Python type. ## Opting out diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 2d53123a69..ca6b38cee0 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -76,11 +76,11 @@ async def main() -> None: `__aexit__` is the disconnection, which is why there is no `client.close()` to forget. **[Testing](get-started/testing.md)** is built on exactly this pattern. -## `Error executing tool : ` and `Unknown tool: ` +## `Error executing tool : `, `Error executing tool `, and `Unknown tool: ` You are reading a **result**, not an exception. `call_tool` did not raise, and it never will for a failing tool. -Call `forecast` for a city the server doesn't know, and the exception it raises comes back with the request marked as *succeeded*: +Call `forecast` for a city the server doesn't know, and the `ToolError` it raises comes back with the request marked as *succeeded*: ```python result.is_error # True @@ -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 `` 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 '' raised an unexpected exception`. +The bare form, `Error executing tool ` 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 '' raised an unexpected exception`. ## `TypeError: The @tool decorator was used incorrectly. Did you forget to call it? Use @tool() instead of @tool` @@ -406,7 +406,7 @@ mcp = MCPServer("Weather", request_state_security=RequestStateSecurity(keys=[key ## Recap * `ExceptionGroup: unhandled errors in a TaskGroup` is never the error. Read the **last line**; catching `MCPError` *inside* the `async with Client(...)` block skips the wrapping entirely. -* `call_tool` does not raise for a failing tool. `Error executing tool ...` and `Unknown tool: ...` are results: check `result.is_error`. +* `call_tool` does not raise for a failing tool. `Error executing tool ...` and `Unknown tool: ...` are results: check `result.is_error`. No message after the tool name means it crashed, and the traceback is in the server log. * `Client must be used within an async context manager` -> use `async with`. `Use @tool() instead of @tool` -> add the parentheses. * `Tool already exists:` in the server log is the only sign that two same-named tools collapsed into one. * One 421, three spellings: `Server returned an error response` (the python `Client`), `421 Misdirected Request` / `Invalid Host header` (everything else), `Invalid Host header: ` (the server log). Fix: `transport_security=TransportSecuritySettings(allowed_hosts=[...])`. diff --git a/docs/whats-new.md b/docs/whats-new.md index 0a4ed4c35f..068efb26ce 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -129,7 +129,7 @@ On those types, every Python attribute is now snake_case: `result.is_error`, `to The renames announce themselves. These do not: * **Sync functions run on a worker thread.** A `def` tool (or resource, prompt, or resolver) no longer blocks the event loop; the trade is that its body no longer runs *on* the event-loop thread, which matters to thread-affine code. `async def` handlers are untouched. **[Migration Guide](migration.md#sync-handler-functions-now-run-on-a-worker-thread)**. -* **`MCPError` (v1's `McpError`) raised inside a tool is a protocol error now.** The model never sees it. Every other exception still becomes an `is_error=True` result the model can read and react to. **[Handling errors](servers/handling-errors.md)** is the split. +* **`MCPError` (v1's `McpError`) raised inside a tool is a protocol error now.** The model never sees it. Every other exception still becomes an `is_error=True` result, but only a `ToolError`'s message reaches the model: any other exception now reads `Error executing tool `, with the traceback in your server log. **[Handling errors](servers/handling-errors.md)** is the split. * **Results are validated before they leave.** A hand-built `Tool` whose `input_schema` is `{}` now fails `tools/list` (the spec requires `"type": "object"`). Servers built on `@mcp.tool()` never see this; the SDK writes their schemas. * **Your client validates what it receives.** `list_tools()` and `call_tool()` check the server's answer against the negotiated protocol version, so a not-quite-valid server that v1's lenient parse tolerated now raises `pydantic.ValidationError`. If you connect to servers you do not control, expect to be the one who finds them; the **[Migration Guide](migration.md#client-validates-inbound-traffic-against-the-protocol-schema)** has the details. * **URI templates are real RFC 6570 now.** `{+path}`, `{?query}` and friends work, matching is exact instead of regex-loose, and path traversal in extracted values is rejected by default. Stricter templates fail at decoration time, not on the first request. **[URI templates](servers/uri-templates.md)**. diff --git a/docs_src/client/tutorial003.py b/docs_src/client/tutorial003.py index bf74c46748..0831f5f752 100644 --- a/docs_src/client/tutorial003.py +++ b/docs_src/client/tutorial003.py @@ -2,6 +2,7 @@ from mcp import Client from mcp.server import MCPServer +from mcp.server.mcpserver.exceptions import ToolError from mcp.types import TextContent mcp = MCPServer("Bookshop") @@ -17,7 +18,7 @@ class Book(BaseModel): def lookup_book(title: str) -> Book: """Look up a book by its exact title.""" if title != "Dune": - raise ValueError(f"No book titled {title!r} in the catalog.") + raise ToolError(f"No book titled {title!r} in the catalog.") return Book(title="Dune", author="Frank Herbert", year=1965) diff --git a/docs_src/handling_errors/tutorial001.py b/docs_src/handling_errors/tutorial001.py index 003ea94669..9676a10075 100644 --- a/docs_src/handling_errors/tutorial001.py +++ b/docs_src/handling_errors/tutorial001.py @@ -1,4 +1,5 @@ from mcp.server import MCPServer +from mcp.server.mcpserver.exceptions import ToolError mcp = MCPServer("Bookshop") @@ -9,5 +10,5 @@ def get_author(title: str) -> str: """Look up the author of a book in the catalog.""" if title not in CATALOG: - raise ValueError(f"No book titled {title!r} in the catalog.") + raise ToolError(f"No book titled {title!r} in the catalog.") return CATALOG[title] diff --git a/docs_src/handling_errors/tutorial004.py b/docs_src/handling_errors/tutorial004.py index 9676a10075..baca11d666 100644 --- a/docs_src/handling_errors/tutorial004.py +++ b/docs_src/handling_errors/tutorial004.py @@ -1,5 +1,4 @@ from mcp.server import MCPServer -from mcp.server.mcpserver.exceptions import ToolError mcp = MCPServer("Bookshop") @@ -9,6 +8,4 @@ @mcp.tool() def get_author(title: str) -> str: """Look up the author of a book in the catalog.""" - if title not in CATALOG: - raise ToolError(f"No book titled {title!r} in the catalog.") return CATALOG[title] diff --git a/docs_src/troubleshooting/tutorial001.py b/docs_src/troubleshooting/tutorial001.py index e83a552df0..0b0f4840a7 100644 --- a/docs_src/troubleshooting/tutorial001.py +++ b/docs_src/troubleshooting/tutorial001.py @@ -1,5 +1,5 @@ from mcp.server import MCPServer -from mcp.server.mcpserver.exceptions import ResourceNotFoundError +from mcp.server.mcpserver.exceptions import ResourceNotFoundError, ToolError mcp = MCPServer("Weather") @@ -10,7 +10,7 @@ def forecast(city: str) -> str: """Today's forecast for one city.""" if city not in FORECASTS: - raise ValueError(f"No forecast for {city!r}.") + raise ToolError(f"No forecast for {city!r}.") return FORECASTS[city] diff --git a/src/mcp/server/mcpserver/exceptions.py b/src/mcp/server/mcpserver/exceptions.py index 0a39d61981..135d7352f9 100644 --- a/src/mcp/server/mcpserver/exceptions.py +++ b/src/mcp/server/mcpserver/exceptions.py @@ -44,11 +44,11 @@ class ToolError(MCPServerError): """A tool failure you anticipated. Raise this from a tool (or a resolver) for a failure you saw coming: the - call returns `is_error=True` with the message in `content`, and the server - logs it at INFO without a traceback. Any other exception reaches the model - the same way but is treated as a crash and logged at ERROR with its traceback. - 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 `, and + the server logs the traceback at ERROR. A `ResourceError` that escapes the tool + (say from `ctx.read_resource()`) counts as anticipated too. The SDK raises it too, for an unknown tool name and for arguments that fail the input schema, and `UnexpectedToolError` subclasses it, so `except ToolError` @@ -60,9 +60,10 @@ class UnexpectedToolError(ToolError): """A tool call failed with something other than `ToolError` or `MCPError`. The SDK raises this itself, around a crash in the tool (or a resolver) or a - return value that fails output conversion. You never raise it. `__cause__` is - 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 `, 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 `MCPServer.call_tool()` to tell a crash from a deliberate `ToolError`. """ diff --git a/src/mcp/server/mcpserver/resolve.py b/src/mcp/server/mcpserver/resolve.py index d4a744af37..f53713971c 100644 --- a/src/mcp/server/mcpserver/resolve.py +++ b/src/mcp/server/mcpserver/resolve.py @@ -575,7 +575,12 @@ async def _fulfil(marker: _Marker, key: str, res: _Resolution) -> ElicitationRes if res.context.session.can_send_request: _require_capability(res.context, marker, key) if isinstance(marker, Elicit): - 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. + raise ToolError(f"Resolver {key!r}: {e}") from e result = await res.context.session.send_request( _render_request(marker), _result_type(marker), diff --git a/src/mcp/server/mcpserver/server.py b/src/mcp/server/mcpserver/server.py index ac5aebe344..3b74dddb62 100644 --- a/src/mcp/server/mcpserver/server.py +++ b/src/mcp/server/mcpserver/server.py @@ -44,7 +44,7 @@ from mcp_types import Resource as MCPResource from mcp_types import ResourceTemplate as MCPResourceTemplate from mcp_types import Tool as MCPTool -from pydantic import BaseModel +from pydantic import BaseModel, ValidationError from pydantic.networks import AnyUrl from starlette.applications import Starlette from starlette.middleware import Middleware @@ -427,9 +427,14 @@ async def _handle_call_tool( except MCPError: raise except Exception as exc: - # %r keeps peer-supplied text (names, pydantic messages) on one line. if isinstance(exc, ToolError) and not isinstance(exc, UnexpectedToolError): - logger.info("Tool %r failed: %r", params.name, str(exc)) + 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. + logger.info("Tool %r failed: %r", params.name, str(exc)) else: logger.exception("Tool %r raised an unexpected exception", params.name) return CallToolResult(content=[TextContent(type="text", text=str(exc))], is_error=True) @@ -733,7 +738,15 @@ def decorator(func: _CallableT) -> _CallableT: async def handler( ctx: ServerRequestContext[LifespanResultT], params: CompleteRequestParams ) -> CompleteResult: - result = await func(params.ref, params.argument, params.context) + try: + result = await func(params.ref, params.argument, params.context) + except MCPError: + raise + except Exception as exc: + logger.exception("Completion for argument %r raised an unexpected exception", params.argument.name) + raise MCPError( + code=INTERNAL_ERROR, message=f"Error completing argument {params.argument.name}" + ) from exc return CompleteResult( completion=result if result is not None else Completion(values=[], total=None, has_more=None), ) diff --git a/src/mcp/server/mcpserver/tools/base.py b/src/mcp/server/mcpserver/tools/base.py index 0768372c39..40e9456fa0 100644 --- a/src/mcp/server/mcpserver/tools/base.py +++ b/src/mcp/server/mcpserver/tools/base.py @@ -134,8 +134,10 @@ async def run( ) -> Any: """Run the tool with arguments. - Every failure other than `MCPError` is raised with its message prefixed - `Error executing tool : `, and `__cause__` set to what was raised. + Every failure other than `MCPError` is raised as a `ToolError` whose message + starts `Error executing tool ` and whose `__cause__` is what was raised. + An anticipated failure keeps its own text after the prefix. A crash does not, + so nothing from an unexpected exception reaches the client. Raises: ToolError: If the arguments fail validation against the input schema, or @@ -153,7 +155,7 @@ async def run( raise except Exception as exc: # A custom validator or default_factory that raises is a crash. - raise UnexpectedToolError(f"Error executing tool {self.name}: {exc}") from exc + raise UnexpectedToolError(f"Error executing tool {self.name}") from exc try: pass_directly: dict[str, Any] = {} @@ -203,10 +205,12 @@ async def run( # Everything else reaches the model as an is_error result under this tool's # name, and the wrapper's type tells the server whether to log a crash. except (UnexpectedToolError, UnexpectedResourceError) as exc: - # A nested tool call or resource read crashed: still a crash here. + # A nested tool call or resource read crashed: still a crash here. Its + # message is already the generic one, so it is safe to carry along. raise UnexpectedToolError(f"Error executing tool {self.name}: {exc}") from exc except (ToolError, ResourceError) as exc: # Raised deliberately by the tool, a resolver, or a resource it read. raise ToolError(f"Error executing tool {self.name}: {exc}") from exc except Exception as exc: - raise UnexpectedToolError(f"Error executing tool {self.name}: {exc}") from exc + # A crash: the exception's own text stays on the server. + raise UnexpectedToolError(f"Error executing tool {self.name}") from exc diff --git a/tests/docs_src/test_client.py b/tests/docs_src/test_client.py index c8292d989b..d07ee4c9e4 100644 --- a/tests/docs_src/test_client.py +++ b/tests/docs_src/test_client.py @@ -85,7 +85,7 @@ async def test_call_tool_result_has_three_things_to_read() -> None: async def test_a_raising_tool_is_a_result_not_an_exception() -> None: - """tutorial003 `!!! check`: the exception's message comes back in content with is_error=True.""" + """tutorial003 `!!! check`: the ToolError's message comes back in content with is_error=True.""" async with Client(tutorial003.mcp) as client: result = await client.call_tool("lookup_book", {"title": "Solaris"}) assert result.is_error diff --git a/tests/docs_src/test_deprecated.py b/tests/docs_src/test_deprecated.py index 090ca61643..5d2afddb2e 100644 --- a/tests/docs_src/test_deprecated.py +++ b/tests/docs_src/test_deprecated.py @@ -8,6 +8,7 @@ so the prose cannot drift away from what the SDK does. """ +import logging import warnings import pytest @@ -117,20 +118,25 @@ def test_mcp_deprecation_warning_is_a_user_warning() -> None: @pytest.mark.filterwarnings("error::mcp.MCPDeprecationWarning") -async def test_error_filter_turns_the_deprecated_call_into_the_documented_tool_error() -> None: +async def test_error_filter_turns_the_deprecated_call_into_the_documented_tool_error( + caplog: pytest.LogCaptureFixture, +) -> None: """The `!!! check`: `"error::mcp.MCPDeprecationWarning"` makes `old_log` fail. - Under the error filter the warning becomes the raised exception, the tool manager - wraps it, and the result is exactly the tool error the page quotes. + Under the error filter the warning becomes the raised exception, the tool wrapper treats it as a + crash, and the result plus the logged warning are exactly what the page quotes. """ + caplog.set_level(logging.ERROR, logger="mcp.server.mcpserver.server") async with Client(mcp) as client: result = await client.call_tool("old_log", {}) assert result.is_error [content] = result.content assert isinstance(content, TextContent) - assert content.text == ( - "Error executing tool old_log: The logging capability is deprecated as of 2026-07-28 (SEP-2577)." - ) + assert content.text == "Error executing tool old_log" + (record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert record.exc_info is not None and isinstance(record.exc_info[1], BaseException) + assert str(record.exc_info[1].__cause__) == "The logging capability is deprecated as of 2026-07-28 (SEP-2577)." + assert type(record.exc_info[1].__cause__).__name__ == "MCPDeprecationWarning" async def test_filterwarnings_ignore_silences_the_whole_category() -> None: diff --git a/tests/docs_src/test_elicitation.py b/tests/docs_src/test_elicitation.py index 17933816bd..87d71571a6 100644 --- a/tests/docs_src/test_elicitation.py +++ b/tests/docs_src/test_elicitation.py @@ -1,5 +1,6 @@ """`docs/handlers/elicitation.md`: every claim the page makes, proved against the real SDK.""" +import logging from typing import Literal import pytest @@ -123,8 +124,7 @@ async def on_elicit(context: ClientRequestContext, params: ElicitRequestParams) async with Client(tutorial001.mcp, mode="legacy", elicitation_callback=on_elicit) as client: result = await client.call_tool("book_table", {"date": "2025-12-25", "party_size": 2}) assert result.is_error - assert isinstance(result.content[0], TextContent) - assert "does not match the requested schema" in result.content[0].text + assert result.content == [TextContent(type="text", text="Error executing tool book_table")] class Address(BaseModel): @@ -158,15 +158,21 @@ async def choose_seating(ctx: Context) -> str: return result.data.area -async def test_a_nested_model_is_rejected_before_anything_is_sent() -> None: - """`!!! warning`: a non-primitive field raises `TypeError` inside `ctx.elicit`, with this exact message.""" +async def test_a_nested_model_is_rejected_before_anything_is_sent(caplog: pytest.LogCaptureFixture) -> None: + """`!!! warning`: a non-primitive field raises `TypeError` inside `ctx.elicit` with this exact message, + which fails the call and lands in the server log rather than on the wire.""" + caplog.set_level(logging.ERROR, logger="mcp.server.mcpserver.server") async with Client(schema_gate_server, mode="legacy") as client: result = await client.call_tool("sign_up", {}) assert result.is_error - assert isinstance(result.content[0], TextContent) - assert result.content[0].text == ( - "Error executing tool sign_up: Elicitation schema field 'address' rendered as " - "{'$ref': '#/$defs/Address'}, which is not a valid PrimitiveSchemaDefinition" + assert result.content == [TextContent(type="text", text="Error executing tool sign_up")] + (record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert record.exc_info is not None and record.exc_info[1] is not None + cause = record.exc_info[1].__cause__ + assert isinstance(cause, TypeError) + assert str(cause) == ( + "Elicitation schema field 'address' rendered as {'$ref': '#/$defs/Address'}, " + "which is not a valid PrimitiveSchemaDefinition" ) diff --git a/tests/docs_src/test_handling_errors.py b/tests/docs_src/test_handling_errors.py index 8872ba7b4c..9824d90dc9 100644 --- a/tests/docs_src/test_handling_errors.py +++ b/tests/docs_src/test_handling_errors.py @@ -12,8 +12,8 @@ pytestmark = [pytest.mark.anyio, pytest.mark.filterwarnings("error::mcp.MCPDeprecationWarning")] -async def test_a_plain_exception_becomes_a_tool_error_the_model_reads() -> None: - """tutorial001: any non-`MCPError` exception comes back as `is_error=True` with the message in `content`.""" +async def test_tool_error_becomes_a_tool_error_the_model_reads() -> None: + """tutorial001: `ToolError` comes back as `is_error=True` with the message in `content`.""" async with Client(tutorial001.mcp) as client: result = await client.call_tool("get_author", {"title": "Nothing"}) assert result.is_error @@ -23,6 +23,16 @@ async def test_a_plain_exception_becomes_a_tool_error_the_model_reads() -> None: assert result.structured_content is None +async def test_tool_error_is_one_info_line(caplog: pytest.LogCaptureFixture) -> None: + """tutorial001: on the server a `ToolError` is one INFO record with no traceback.""" + caplog.set_level(logging.INFO) + async with Client(tutorial001.mcp) as client: + await client.call_tool("get_author", {"title": "Nothing"}) + records = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert [(r.levelno, r.exc_info) for r in records] == [(logging.INFO, None)] + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + async def test_a_title_the_catalog_knows_is_an_ordinary_result() -> None: """tutorial001: the non-raising path is a plain `is_error=False` result.""" async with Client(tutorial001.mcp) as client: @@ -57,6 +67,21 @@ async def test_mcp_error_only_fires_on_the_raising_path() -> None: assert result.structured_content == {"result": "Frank Herbert"} +async def test_any_other_exception_is_a_crash_the_model_sees_generically(caplog: pytest.LogCaptureFixture) -> None: + """tutorial004, "Any other exception": the `KeyError` text stays on the server; the model gets only the + generic line, and the log gets one ERROR record with the traceback under the documented message.""" + caplog.set_level(logging.INFO) + async with Client(tutorial004.mcp) as client: + result = await client.call_tool("get_author", {"title": "Nothing"}) + assert result.is_error + assert result.content == [TextContent(type="text", text="Error executing tool get_author")] + (record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert (record.levelno, record.getMessage()) == (logging.ERROR, "Tool 'get_author' raised an unexpected exception") + assert record.exc_info is not None + logged = record.exc_info[1] + assert logged is not None and isinstance(logged.__cause__, KeyError) + + async def test_resource_not_found_error_maps_to_invalid_params() -> None: """tutorial003: `ResourceNotFoundError` from a template handler is `-32602` with the URI in `data`.""" async with Client(tutorial003.mcp) as client: @@ -70,14 +95,11 @@ async def test_resource_not_found_error_maps_to_invalid_params() -> None: async def test_raise_exceptions_does_not_turn_a_tool_error_into_a_traceback() -> None: - """The `!!! info` before the log section: even `raise_exceptions=True` leaves a failing tool as the - `is_error=True` result.""" - async with Client(tutorial001.mcp, raise_exceptions=True) as client: + """The closing `!!! info`: even `raise_exceptions=True` leaves a failing tool as the `is_error=True` result.""" + async with Client(tutorial004.mcp, raise_exceptions=True) as client: result = await client.call_tool("get_author", {"title": "Nothing"}) assert result.is_error - assert result.content == [ - TextContent(type="text", text="Error executing tool get_author: No book titled 'Nothing' in the catalog.") - ] + assert result.content == [TextContent(type="text", text="Error executing tool get_author")] async def test_a_title_the_template_knows_reads_normally() -> None: @@ -89,36 +111,8 @@ async def test_a_title_the_template_knows_reads_normally() -> None: assert contents.text == "Dune by Frank Herbert" -async def test_a_plain_exception_is_logged_as_a_crash_with_its_traceback(caplog: pytest.LogCaptureFixture) -> None: - """tutorial001, "What the server logs": the `ValueError` is one ERROR record carrying the traceback.""" - caplog.set_level(logging.INFO) - async with Client(tutorial001.mcp) as client: - await client.call_tool("get_author", {"title": "Nothing"}) - (record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] - assert record.levelno == logging.ERROR - assert record.exc_info is not None - logged = record.exc_info[1] - assert logged is not None and isinstance(logged.__cause__, ValueError) - - -async def test_tool_error_reads_the_same_to_the_model_and_logs_one_info_line( - caplog: pytest.LogCaptureFixture, -) -> None: - """tutorial004: swapping in `ToolError` leaves the result byte-identical and the log at INFO, no traceback.""" - caplog.set_level(logging.INFO) - async with Client(tutorial004.mcp) as client: - result = await client.call_tool("get_author", {"title": "Nothing"}) - assert result.is_error - assert result.content == [ - TextContent(type="text", text="Error executing tool get_author: No book titled 'Nothing' in the catalog.") - ] - records = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] - assert [(r.levelno, r.exc_info) for r in records] == [(logging.INFO, None)] - assert not [r for r in caplog.records if r.levelno >= logging.WARNING] - - async def test_a_bad_argument_is_an_info_line_not_a_crash(caplog: pytest.LogCaptureFixture) -> None: - """ "What the server logs": schema rejection of the arguments is logged at INFO with no traceback.""" + """ "Errors you never raise": schema rejection of the arguments is logged at INFO with no traceback.""" caplog.set_level(logging.INFO) async with Client(tutorial001.mcp) as client: result = await client.call_tool("get_author", {"title": 42}) diff --git a/tests/docs_src/test_structured_output.py b/tests/docs_src/test_structured_output.py index a12e6d2e7d..dcac214378 100644 --- a/tests/docs_src/test_structured_output.py +++ b/tests/docs_src/test_structured_output.py @@ -1,5 +1,7 @@ """`docs/servers/structured-output.md`: every claim the page makes, proved against the real SDK.""" +import logging + import pytest from inline_snapshot import snapshot from mcp_types import EmbeddedResource, ImageContent, TextContent, TextResourceContents @@ -151,15 +153,19 @@ async def test_dict_str_return_is_not_wrapped() -> None: assert result.structured_content == {"London": 16.2, "Reykjavik": 4.4} -async def test_return_value_is_validated_against_the_schema() -> None: - """tutorial007: a return value that does not match the output schema is a tool error, not a result.""" +async def test_return_value_is_validated_against_the_schema(caplog: pytest.LogCaptureFixture) -> None: + """tutorial007: a return value that does not match the output schema is a tool error, not a result; + the field name goes to the server log, not the client.""" + caplog.set_level(logging.ERROR, logger="mcp.server.mcpserver.server") async with Client(tutorial007.mcp) as client: result = await client.call_tool("get_weather", {"city": "London"}) assert result.is_error assert result.structured_content is None - assert isinstance(result.content[0], TextContent) - assert result.content[0].text.startswith("Error executing tool get_weather: 1 validation error for WeatherData") - assert "humidity\n Field required" in result.content[0].text + assert result.content == [TextContent(type="text", text="Error executing tool get_weather")] + (record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert record.getMessage() == "Tool 'get_weather' raised an unexpected exception" + assert record.exc_info is not None and record.exc_info[1] is not None + assert "1 validation error for WeatherData\nhumidity\n Field required" in str(record.exc_info[1].__cause__) async def test_structured_output_false_opts_out() -> None: diff --git a/tests/docs_src/test_troubleshooting.py b/tests/docs_src/test_troubleshooting.py index 7d79e21b5e..1e1b5e15b8 100644 --- a/tests/docs_src/test_troubleshooting.py +++ b/tests/docs_src/test_troubleshooting.py @@ -83,11 +83,23 @@ async def test_a_failing_tool_returns_is_error_true_instead_of_raising() -> None ] -async def test_a_failing_tool_leaves_its_traceback_in_the_server_log(caplog: pytest.LogCaptureFixture) -> None: - """The `Error executing tool` entry's pointer to the server log: the exact ERROR message it names.""" - with caplog.at_level(logging.ERROR, logger="mcp.server.mcpserver.server"): - async with Client(tutorial001.mcp) as client: - await client.call_tool("forecast", {"city": "Atlantis"}) +async def test_a_crashing_tool_is_the_bare_form_with_its_traceback_in_the_server_log( + caplog: pytest.LogCaptureFixture, +) -> None: + """The bare `Error executing tool `: the tool raised something other than ToolError, its text + is withheld, and the server log carries the exact ERROR message the page names.""" + mcp = MCPServer("Weather") + + @mcp.tool() + def forecast(city: str) -> str: + """Today's forecast for one city. Crashes: the upstream table is missing the key.""" + forecasts: dict[str, str] = {} + return forecasts[city] + + caplog.set_level(logging.ERROR, logger="mcp.server.mcpserver.server") + async with Client(mcp) as client: + result = await client.call_tool("forecast", {"city": "Atlantis"}) + assert result.content == [TextContent(type="text", text="Error executing tool forecast")] (record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] assert record.getMessage() == "Tool 'forecast' raised an unexpected exception" assert record.exc_info is not None diff --git a/tests/interaction/_requirements.py b/tests/interaction/_requirements.py index 38b250c505..11bffba7b5 100644 --- a/tests/interaction/_requirements.py +++ b/tests/interaction/_requirements.py @@ -1016,8 +1016,9 @@ def __post_init__(self) -> None: "mcpserver:tool:handler-throws": Requirement( source="sdk", behavior=( - "An exception raised by a tool function (ToolError or otherwise) is caught and returned as a " - "tool result with isError true and the failure text in content; it does not become a JSON-RPC error." + "An exception raised by a tool function is caught and returned as a tool result with isError true, " + "never a JSON-RPC error; a ToolError carries its message in content, any other exception carries only " + "the generic 'Error executing tool '." ), ), "mcpserver:tool:input-validation": Requirement( diff --git a/tests/interaction/mcpserver/test_tools.py b/tests/interaction/mcpserver/test_tools.py index a6418ac9c5..bff5b9282d 100644 --- a/tests/interaction/mcpserver/test_tools.py +++ b/tests/interaction/mcpserver/test_tools.py @@ -84,9 +84,9 @@ def place(mode: Literal["fast", "slow"], point: Point, count: Annotated[int, Fie async def test_call_tool_function_exception_becomes_error_result(connect: Connect, unstamped: Unstamp) -> None: """An exception raised by a tool function is returned as an is_error result, not a JSON-RPC error. - The function's `-> str` annotation gives the tool a derived output schema, but the error - result is built before any schema validation runs, so no validation failure is layered on - top of the original exception. + The exception's own text ("boom") is withheld: an unexpected exception is a crash, and only + ToolError carries a message to the client. The function's `-> str` annotation gives the tool + a derived output schema, but the error result is built before any schema validation runs. """ mcp = MCPServer("errors") @@ -98,7 +98,7 @@ def explode() -> str: result = await client.call_tool("explode", {}) assert unstamped(result) == snapshot( - CallToolResult(content=[TextContent(text="Error executing tool explode: boom")], is_error=True) + CallToolResult(content=[TextContent(text="Error executing tool explode")], is_error=True) ) @@ -245,7 +245,7 @@ def add(a: int, b: int) -> str: @requirement("mcpserver:output-schema:server-validate") @requirement("mcpserver:output-schema:missing-structured") async def test_tool_with_output_schema_returning_mismatched_structured_content_is_an_error_result( - connect: Connect, + connect: Connect, unstamped: Unstamp ) -> None: """Structured content that fails the tool's own output schema is rejected on the server side. @@ -273,16 +273,14 @@ def missing() -> Annotated[CallToolResult, Weather]: mismatched_result = await client.call_tool("mismatched", {}) missing_result = await client.call_tool("missing", {}) - # The body of each message is raw pydantic ValidationError output (model name, field paths, - # an errors.pydantic.dev URL) and changes across pydantic versions, so only the SDK's stable - # prefix is asserted. - assert mismatched_result.is_error is True - assert isinstance(mismatched_result.content[0], TextContent) - assert mismatched_result.content[0].text.startswith("Error executing tool mismatched: 2 validation errors") - - assert missing_result.is_error is True - assert isinstance(missing_result.content[0], TextContent) - assert missing_result.content[0].text.startswith("Error executing tool missing: 1 validation error") + # A return value that fails its own output schema is the tool's bug, so the pydantic detail + # goes to the server log and the client gets only the generic crash text. + assert unstamped(mismatched_result) == snapshot( + CallToolResult(content=[TextContent(text="Error executing tool mismatched")], is_error=True) + ) + assert unstamped(missing_result) == snapshot( + CallToolResult(content=[TextContent(text="Error executing tool missing")], is_error=True) + ) @requirement("mcpserver:tool:duplicate-name") diff --git a/tests/server/mcpserver/test_resolve.py b/tests/server/mcpserver/test_resolve.py index 966bb94ed1..49f0f1314f 100644 --- a/tests/server/mcpserver/test_resolve.py +++ b/tests/server/mcpserver/test_resolve.py @@ -1783,9 +1783,11 @@ async def sneaky(login: Annotated[Login, Resolve(lookup)]): result = await client.call_tool("sneaky", {}) assert result.is_error assert isinstance(result.content[0], TextContent) - assert "the multi-round flow is driven either by resolvers or by the tool body" in result.content[0].text - records = [(r.levelname, r.getMessage()) for r in caplog.records if r.name == "mcp.server.mcpserver.server"] - assert records == [("ERROR", "Tool 'sneaky' raised an unexpected exception")] + assert result.content[0].text == "Error executing tool sneaky" + (record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert (record.levelname, record.getMessage()) == ("ERROR", "Tool 'sneaky' raised an unexpected exception") + assert record.exc_info is not None and record.exc_info[1] is not None + assert "the multi-round flow is driven either by resolvers or by the tool body" in str(record.exc_info[1].__cause__) def test_question_digest_pins_the_rendered_question(): diff --git a/tests/server/mcpserver/test_server.py b/tests/server/mcpserver/test_server.py index a97117c54f..65810f4ba9 100644 --- a/tests/server/mcpserver/test_server.py +++ b/tests/server/mcpserver/test_server.py @@ -298,7 +298,7 @@ async def test_tool_exception_handling(self): assert len(result.content) == 1 content = result.content[0] assert isinstance(content, TextContent) - assert "Test error" in content.text + assert content.text == "Error executing tool error_tool_fn" assert result.is_error is True async def test_tool_error_handling(self): @@ -309,7 +309,7 @@ async def test_tool_error_handling(self): assert len(result.content) == 1 content = result.content[0] assert isinstance(content, TextContent) - assert "Test error" in content.text + assert content.text == "Error executing tool error_tool_fn" assert result.is_error is True async def test_tool_error_details(self): @@ -321,7 +321,7 @@ async def test_tool_error_details(self): content = result.content[0] assert isinstance(content, TextContent) assert isinstance(content.text, str) - assert "Test error" in content.text + assert content.text == "Error executing tool error_tool_fn" assert result.is_error is True async def test_tool_return_value_conversion(self): @@ -1805,6 +1805,51 @@ async def handle_completion( assert result.completion.values == ["bold", "italic", "underline"] +async def test_completion_handler_crash_is_logged_and_reaches_the_client_generically( + caplog: pytest.LogCaptureFixture, +) -> None: + """SDK-defined: a crashing completion handler is one ERROR record with its traceback, and the client + gets -32603 naming only the argument, not the exception's text.""" + mcp = MCPServer() + raised = RuntimeError("index warmup failed on shard 3") + + @mcp.completion() + async def complete(ref: PromptReference, argument: CompletionArgument, context: CompletionContext | None): + raise raised + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + with pytest.raises(MCPError) as exc: + await client.complete( + ref=PromptReference(type="ref/prompt", name="greet"), argument={"name": "style", "value": "b"} + ) + + assert exc.value.error == snapshot(ErrorData(code=INTERNAL_ERROR, message="Error completing argument style")) + assert _server_records(caplog) == snapshot( + [("ERROR", "Completion for argument 'style' raised an unexpected exception", True)] + ) + assert raised in _cause_chain(_logged_exception(caplog)) + + +async def test_completion_handler_raising_mcp_error_passes_through(caplog: pytest.LogCaptureFixture) -> None: + """SDK-defined: MCPError from a completion handler keeps its code and message and is not logged.""" + mcp = MCPServer() + + @mcp.completion() + async def complete(ref: PromptReference, argument: CompletionArgument, context: CompletionContext | None): + raise MCPError(code=INVALID_PARAMS, message="unknown argument") + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + with pytest.raises(MCPError) as exc: + await client.complete( + ref=PromptReference(type="ref/prompt", name="greet"), argument={"name": "style", "value": "b"} + ) + + assert exc.value.error == snapshot(ErrorData(code=INVALID_PARAMS, message="unknown argument")) + assert _server_records(caplog) == [] + + def test_streamable_http_no_redirect() -> None: """Test that streamable HTTP routes are correctly configured.""" mcp = MCPServer() @@ -2297,7 +2342,7 @@ def lookup() -> str: result = await client.call_tool("lookup", {}) assert result.is_error is True - assert result.content == [TextContent(type="text", text="Error executing tool lookup: 'k'")] + assert result.content == [TextContent(type="text", text="Error executing tool lookup")] assert _server_records(caplog) == snapshot([("ERROR", "Tool 'lookup' raised an unexpected exception", True)]) assert raised in _cause_chain(_logged_exception(caplog)) assert len([r for r in caplog.records if r.levelno >= logging.WARNING]) == 1 @@ -2354,7 +2399,7 @@ async def test_tool_argument_validation_failure_is_logged_at_info_without_traceb caplog: pytest.LogCaptureFixture, ): """SDK-defined: arguments the model got wrong are the model's to correct, so the rejection - is logged as one INFO record with no traceback; the message is repr-quoted onto one line.""" + is logged as one INFO record with no traceback, naming the fields but not the values.""" mcp = MCPServer() @mcp.tool() @@ -2368,9 +2413,8 @@ def add(a: int, b: int) -> int: assert result.is_error is True ((level, message, has_traceback),) = _server_records(caplog) assert (level, has_traceback) == ("INFO", False) - # pydantic owns the rest of the text; pin only the SDK's part and the single-line rendering. - assert message.startswith("Tool 'add' failed: ") and "Error executing tool add: 1 validation error" in message - assert "\n" not in message + # Field names only: the rejected values are the caller's data and stay out of the log. + assert message == "Tool 'add' rejected arguments: a" assert not [r for r in caplog.records if r.levelno >= logging.WARNING] @@ -2527,9 +2571,7 @@ def redeem(code: Annotated[str, AfterValidator(check)]) -> str: with pytest.raises(UnexpectedToolError) as exc: await mcp.call_tool("redeem", {"code": "SAVE10"}) - assert result.content == [ - TextContent(type="text", text="Error executing tool redeem: codes are compared as integers") - ] + assert result.content == [TextContent(type="text", text="Error executing tool redeem")] assert exc.value.__cause__ is raised assert _server_records(caplog) == snapshot([("ERROR", "Tool 'redeem' raised an unexpected exception", True)]) @@ -2847,7 +2889,7 @@ def explode() -> str: with pytest.raises(UnexpectedToolError) as exc: await mcp.call_tool("explode", {}) - assert str(exc.value) == snapshot("Error executing tool explode: boom") + assert str(exc.value) == snapshot("Error executing tool explode") assert exc.value.__cause__ is raised @@ -2885,9 +2927,7 @@ async def outer(ctx: Context) -> str: async with Client(mcp) as client: result = await client.call_tool("outer", {}) - assert result.content == [ - TextContent(type="text", text="Error executing tool outer: Error executing tool inner: division by zero") - ] + assert result.content == [TextContent(type="text", text="Error executing tool outer: Error executing tool inner")] assert _server_records(caplog) == snapshot([("ERROR", "Tool 'outer' raised an unexpected exception", True)]) assert raised in _cause_chain(_logged_exception(caplog)) diff --git a/tests/server/mcpserver/test_url_elicitation_error_throw.py b/tests/server/mcpserver/test_url_elicitation_error_throw.py index 29117e6936..6d2a659345 100644 --- a/tests/server/mcpserver/test_url_elicitation_error_throw.py +++ b/tests/server/mcpserver/test_url_elicitation_error_throw.py @@ -106,4 +106,4 @@ async def failing_tool(ctx: Context) -> str: assert result.is_error is True assert len(result.content) == 1 assert isinstance(result.content[0], types.TextContent) - assert "Something went wrong" in result.content[0].text + assert result.content[0].text == "Error executing tool failing_tool" From 02a2b9268150ea7ff85af25404807ee4fdf1855a Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Thu, 20 Aug 2026 10:03:43 +0000 Subject: [PATCH 8/8] Address review: repr rejected-argument names, guard the completion result, add call_fn - Log rejected tool arguments with %r: pydantic's error locations can include caller-supplied dict keys, which must not break onto new log lines. - Build CompleteResult inside the completion adapter's try, so a handler returning the wrong type is logged as a crash and answered with the generic -32603 rather than "Invalid request parameters". - On the legacy resolver path, a malformed ElicitResult from a non-conformant client no longer has its pydantic text repeated back. - Add FuncMetadata.call_fn() for calling with already-validated arguments and use it from Tool.run; call_fn_with_arg_validation() becomes a deprecated wrapper (MCPDeprecationWarning, removal in 3.0). - Docstring and docs wording: MCPError carve-outs, nested crash message, ResourceError in the imports and resource paragraph, the exact MCPDeprecationWarning path a traceback prints. --- docs/deprecated.md | 2 +- docs/servers/handling-errors.md | 8 +- docs/troubleshooting.md | 2 +- src/mcp/server/mcpserver/exceptions.py | 12 +- src/mcp/server/mcpserver/resolve.py | 5 +- src/mcp/server/mcpserver/server.py | 16 +-- src/mcp/server/mcpserver/tools/base.py | 8 +- .../mcpserver/utilities/func_metadata.py | 46 ++++--- tests/server/mcpserver/test_func_metadata.py | 128 +++++++++++------- tests/server/mcpserver/test_server.py | 28 +++- 10 files changed, 156 insertions(+), 99 deletions(-) diff --git a/docs/deprecated.md b/docs/deprecated.md index 6cb2a3c82c..05e37903fe 100644 --- a/docs/deprecated.md +++ b/docs/deprecated.md @@ -123,7 +123,7 @@ That is the whole API. There is no per-method switch, and you don't want one: th `Error executing tool old_log`, and the captured server log names the culprit: ```text - mcp.MCPDeprecationWarning: The logging capability is deprecated as of 2026-07-28 (SEP-2577). + mcp.shared.exceptions.MCPDeprecationWarning: The logging capability is deprecated as of 2026-07-28 (SEP-2577). ``` One line of pytest configuration, and a deprecated call can never sneak back into your diff --git a/docs/servers/handling-errors.md b/docs/servers/handling-errors.md index a923b8c1de..e1f6fffba6 100644 --- a/docs/servers/handling-errors.md +++ b/docs/servers/handling-errors.md @@ -125,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. `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)**. +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 both are one `INFO` line in your log. Any other exception bar `MCPError` 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)**. ## Errors you never raise @@ -136,8 +136,8 @@ Send `get_author` a `title` that isn't a string and the SDK rejects it against t It means a whole class of `raise` statements you don't write: don't re-validate your own type hints. !!! 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 + Everything a **client** sees on this page, the in-memory `Client` you'll write tests with + sees too. 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 the `is_error=True` result. Assert on the result. If you need the traceback of a crash, it is in the server's log, and pytest's `caplog` captures it. **[Testing](../get-started/testing.md)** covers the pattern. @@ -150,7 +150,7 @@ It means a whole class of `raise` statements you don't write: don't re-validate * Any **other exception** is a crash -> `is_error=True` with only `Error executing tool ` for the model, and an `ERROR` record with the traceback for you. * `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. -* 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 ToolError, ResourceError, 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)**. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index ca6b38cee0..1e452be3ff 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -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. -The bare form, `Error executing tool ` 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 '' raised an unexpected exception`. +The bare form, `Error executing tool ` with no message, means the tool **crashed**: something other than `ToolError` was raised while running it (or its return value failed the output schema), and that exception's text is kept off the wire. The traceback is in the **server's log** at `ERROR`, as `Tool '' raised an unexpected exception`. ## `TypeError: The @tool decorator was used incorrectly. Did you forget to call it? Use @tool() instead of @tool` diff --git a/src/mcp/server/mcpserver/exceptions.py b/src/mcp/server/mcpserver/exceptions.py index 135d7352f9..22656f3781 100644 --- a/src/mcp/server/mcpserver/exceptions.py +++ b/src/mcp/server/mcpserver/exceptions.py @@ -46,9 +46,10 @@ class ToolError(MCPServerError): 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 - is treated as a crash: the model sees only `Error executing tool `, and - the server logs the traceback at ERROR. A `ResourceError` that escapes the tool - (say from `ctx.read_resource()`) counts as anticipated too. + (bar `MCPError`, which is a protocol error) is treated as a crash: the model + sees only `Error executing tool `, and the server logs the traceback at + ERROR. A `ResourceError` that escapes the tool (say from `ctx.read_resource()`) + counts as anticipated too. The SDK raises it too, for an unknown tool name and for arguments that fail the input schema, and `UnexpectedToolError` subclasses it, so `except ToolError` @@ -61,8 +62,9 @@ class UnexpectedToolError(ToolError): The SDK raises this itself, around a crash in the tool (or a resolver) or a return value that fails output conversion. You never raise it. The message is - only `Error executing tool `, so nothing from the original reaches the - client. `__cause__` is the original exception, which the server logs with its + only `Error executing tool ` (followed by the same for a nested tool or + resource that crashed), 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 `MCPServer.call_tool()` to tell a crash from a deliberate `ToolError`. """ diff --git a/src/mcp/server/mcpserver/resolve.py b/src/mcp/server/mcpserver/resolve.py index f53713971c..2afbc516e3 100644 --- a/src/mcp/server/mcpserver/resolve.py +++ b/src/mcp/server/mcpserver/resolve.py @@ -580,7 +580,10 @@ async def _fulfil(marker: _Marker, key: str, res: _Resolution) -> ElicitationRes 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. - raise ToolError(f"Resolver {key!r}: {e}") from e + # (A pydantic ValidationError here means a non-conformant client sent a + # malformed ElicitResult; its text is not repeated back.) + detail = "received an invalid elicitation response" if isinstance(e, ValidationError) else str(e) + raise ToolError(f"Resolver {key!r}: {detail}") from e result = await res.context.session.send_request( _render_request(marker), _result_type(marker), diff --git a/src/mcp/server/mcpserver/server.py b/src/mcp/server/mcpserver/server.py index 3b74dddb62..d792f9cedc 100644 --- a/src/mcp/server/mcpserver/server.py +++ b/src/mcp/server/mcpserver/server.py @@ -431,7 +431,7 @@ 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)) + logger.info("Tool %r rejected arguments: %r", params.name, fields) else: # %r keeps peer-supplied text on one line. logger.info("Tool %r failed: %r", params.name, str(exc)) @@ -521,10 +521,10 @@ async def call_tool( Raises: ToolError: If the tool is unknown, the arguments fail validation, or the - tool (or a resolver) raises `ToolError`. - UnexpectedToolError: If the tool (or a resolver) raises anything other than - `ToolError` or `MCPError`, or its return value fails output conversion. - `__cause__` is the original exception. + tool (or a resolver) raises `ToolError` or `ResourceError`. + UnexpectedToolError: If the tool (or a resolver) raises anything else, or + its return value fails output conversion. `__cause__` is the original + exception. """ if context is None: context = Context(mcp_server=self, subscriptions=self._subscriptions) @@ -740,6 +740,9 @@ async def handler( ) -> CompleteResult: try: result = await func(params.ref, params.argument, params.context) + return CompleteResult( + completion=result if result is not None else Completion(values=[], total=None, has_more=None), + ) except MCPError: raise except Exception as exc: @@ -747,9 +750,6 @@ async def handler( raise MCPError( code=INTERNAL_ERROR, message=f"Error completing argument {params.argument.name}" ) from exc - return CompleteResult( - completion=result if result is not None else Completion(values=[], total=None, has_more=None), - ) self._lowlevel_server.add_request_handler("completion/complete", CompleteRequestParams, handler) return func diff --git a/src/mcp/server/mcpserver/tools/base.py b/src/mcp/server/mcpserver/tools/base.py index 40e9456fa0..4a8bed792e 100644 --- a/src/mcp/server/mcpserver/tools/base.py +++ b/src/mcp/server/mcpserver/tools/base.py @@ -173,13 +173,7 @@ async def run( return self.fn_metadata.convert_result(resolved) if convert_result else resolved pass_directly |= resolved - result = await self.fn_metadata.call_fn_with_arg_validation( - self.fn, - self.is_async, - arguments, - pass_directly or None, - pre_validated=validated, - ) + result = await self.fn_metadata.call_fn(self.fn, self.is_async, validated, pass_directly) # Registration rejects the annotated form of this combination; this covers # a body that returns an InputRequiredResult without declaring it. It is diff --git a/src/mcp/server/mcpserver/utilities/func_metadata.py b/src/mcp/server/mcpserver/utilities/func_metadata.py index a4b7f4873e..5eab6efe59 100644 --- a/src/mcp/server/mcpserver/utilities/func_metadata.py +++ b/src/mcp/server/mcpserver/utilities/func_metadata.py @@ -23,7 +23,7 @@ ) from pydantic.fields import FieldInfo from pydantic.json_schema import GenerateJsonSchema, JsonSchemaWarningKind -from typing_extensions import NotRequired, ReadOnly, TypedDict, get_type_hints, is_typeddict +from typing_extensions import NotRequired, ReadOnly, TypedDict, deprecated, get_type_hints, is_typeddict from typing_inspection.introspection import ( UNKNOWN, AnnotationSource, @@ -35,6 +35,7 @@ from mcp.server.mcpserver.exceptions import InvalidSignature from mcp.server.mcpserver.utilities.logging import get_logger from mcp.server.mcpserver.utilities.types import Audio, Image +from mcp.shared.exceptions import MCPDeprecationWarning logger = get_logger(__name__) @@ -125,6 +126,28 @@ def validate_arguments(self, arguments_to_validate: dict[str, Any]) -> dict[str, arguments_parsed_model = self.arg_model.model_validate(arguments_pre_parsed) return arguments_parsed_model.model_dump_one_level() + async def call_fn( + self, + fn: Callable[..., Any | Awaitable[Any]], + fn_is_async: bool, + arguments: dict[str, Any], + arguments_to_pass_directly: dict[str, Any] | None = None, + ) -> Any: + """Call the function with already-validated `arguments` plus `arguments_to_pass_directly`. + + `arguments` is the output of `validate_arguments`. A sync function runs on a + worker thread. + """ + kwargs = arguments | (arguments_to_pass_directly or {}) + if fn_is_async: + return await fn(**kwargs) + return await anyio.to_thread.run_sync(functools.partial(fn, **kwargs)) + + @deprecated( + "FuncMetadata.call_fn_with_arg_validation() is deprecated and will be removed in 3.0; " + "call validate_arguments() and then call_fn() instead.", + category=MCPDeprecationWarning, + ) async def call_fn_with_arg_validation( self, fn: Callable[..., Any | Awaitable[Any]], @@ -133,25 +156,12 @@ async def call_fn_with_arg_validation( arguments_to_pass_directly: dict[str, Any] | None, pre_validated: dict[str, Any] | None = None, ) -> Any: - """Call the given function with arguments validated and injected. + """Validate `arguments_to_validate` (unless `pre_validated` is given) and call the function. - Arguments are first attempted to be parsed from JSON, then validated against - the argument model, before being passed to the function. Pass `pre_validated` - (the output of `validate_arguments`) to reuse an earlier validation pass - - validating twice can re-run `default_factory`/stateful validators and hand the - function different values than a caller already observed. + Deprecated: call `validate_arguments` and then `call_fn`. """ - # Copy so a caller-provided `pre_validated` dict is never mutated in place. - arguments_parsed_dict = dict( - pre_validated if pre_validated is not None else self.validate_arguments(arguments_to_validate) - ) - - arguments_parsed_dict |= arguments_to_pass_directly or {} - - if fn_is_async: - return await fn(**arguments_parsed_dict) - else: - return await anyio.to_thread.run_sync(functools.partial(fn, **arguments_parsed_dict)) + arguments = pre_validated if pre_validated is not None else self.validate_arguments(arguments_to_validate) + return await self.call_fn(fn, fn_is_async, arguments, arguments_to_pass_directly) def convert_result(self, result: Any) -> CallToolResult | InputRequiredResult: """Convert a function call result into a `CallToolResult`. diff --git a/tests/server/mcpserver/test_func_metadata.py b/tests/server/mcpserver/test_func_metadata.py index eff3479279..0dff88c268 100644 --- a/tests/server/mcpserver/test_func_metadata.py +++ b/tests/server/mcpserver/test_func_metadata.py @@ -14,6 +14,7 @@ from pydantic import BaseModel, Field, ValidationError from typing_extensions import NotRequired, ReadOnly, Required +from mcp import MCPDeprecationWarning from mcp.server.mcpserver import Audio, Image from mcp.server.mcpserver.exceptions import InvalidSignature from mcp.server.mcpserver.utilities.func_metadata import ArgModelBase, FuncMetadata, func_metadata @@ -102,33 +103,35 @@ async def test_complex_function_runtime_arg_validation_non_json(): meta = func_metadata(complex_arguments_fn) # Test with minimum required arguments - result = await meta.call_fn_with_arg_validation( + result = await meta.call_fn( complex_arguments_fn, fn_is_async=False, - arguments_to_validate={ - "an_int": 1, - "must_be_none": None, - "must_be_none_dumb_annotation": None, - "list_of_ints": [1, 2, 3], - "list_str_or_str": "hello", - "an_int_annotated_with_field": 42, - "an_int_annotated_with_field_and_others": 5, - "an_int_annotated_with_junk": 100, - "unannotated": "test", - "my_model_a": {}, - "my_model_a_forward_ref": {}, - "my_model_b": {"how_many_shrimp": 5, "ok": {"x": 1}, "y": None}, - }, + arguments=meta.validate_arguments( + { + "an_int": 1, + "must_be_none": None, + "must_be_none_dumb_annotation": None, + "list_of_ints": [1, 2, 3], + "list_str_or_str": "hello", + "an_int_annotated_with_field": 42, + "an_int_annotated_with_field_and_others": 5, + "an_int_annotated_with_junk": 100, + "unannotated": "test", + "my_model_a": {}, + "my_model_a_forward_ref": {}, + "my_model_b": {"how_many_shrimp": 5, "ok": {"x": 1}, "y": None}, + } + ), arguments_to_pass_directly=None, ) assert result == "ok!" # Test with invalid types with pytest.raises(ValueError): - await meta.call_fn_with_arg_validation( + await meta.call_fn( complex_arguments_fn, fn_is_async=False, - arguments_to_validate={"an_int": "not an int"}, + arguments=meta.validate_arguments({"an_int": "not an int"}), arguments_to_pass_directly=None, ) @@ -138,31 +141,33 @@ async def test_complex_function_runtime_arg_validation_with_json(): """Test that JSON string arguments are parsed and validated correctly""" meta = func_metadata(complex_arguments_fn) - result = await meta.call_fn_with_arg_validation( + result = await meta.call_fn( complex_arguments_fn, fn_is_async=False, - arguments_to_validate={ - "an_int": 1, - "must_be_none": None, - "must_be_none_dumb_annotation": None, - "list_of_ints": "[1, 2, 3]", # JSON string - "list_str_or_str": '["a", "b", "c"]', # JSON string - "an_int_annotated_with_field": 42, - "an_int_annotated_with_field_and_others": "5", # JSON string - "an_int_annotated_with_junk": 100, - "unannotated": "test", - "my_model_a": "{}", # JSON string - "my_model_a_forward_ref": "{}", # JSON string - "my_model_b": '{"how_many_shrimp": 5, "ok": {"x": 1}, "y": null}', - }, + arguments=meta.validate_arguments( + { + "an_int": 1, + "must_be_none": None, + "must_be_none_dumb_annotation": None, + "list_of_ints": "[1, 2, 3]", # JSON string + "list_str_or_str": '["a", "b", "c"]', # JSON string + "an_int_annotated_with_field": 42, + "an_int_annotated_with_field_and_others": "5", # JSON string + "an_int_annotated_with_junk": 100, + "unannotated": "test", + "my_model_a": "{}", # JSON string + "my_model_a_forward_ref": "{}", # JSON string + "my_model_b": '{"how_many_shrimp": 5, "ok": {"x": 1}, "y": null}', + } + ), arguments_to_pass_directly=None, ) assert result == "ok!" @pytest.mark.anyio -async def test_call_fn_does_not_mutate_pre_validated(): - """A caller-provided `pre_validated` dict must not be mutated by the call.""" +async def test_call_fn_does_not_mutate_the_arguments_dict(): + """The validated-arguments dict a caller passes to `call_fn` is not mutated when injected kwargs are merged.""" def fn(x: int, ctx: str) -> str: return f"{x}:{ctx}" @@ -171,17 +176,34 @@ def fn(x: int, ctx: str) -> str: pre_validated = meta.validate_arguments({"x": 1}) snapshot = dict(pre_validated) - result = await meta.call_fn_with_arg_validation( + result = await meta.call_fn( fn, fn_is_async=False, - arguments_to_validate={"x": 1}, + arguments=pre_validated, arguments_to_pass_directly={"ctx": "injected"}, - pre_validated=pre_validated, ) assert result == "1:injected" assert pre_validated == snapshot # `ctx` was not leaked into the caller's dict +@pytest.mark.anyio +async def test_call_fn_with_arg_validation_still_works_and_warns(): + """The pre-3.0 helper keeps validating-then-calling, and says it is deprecated (visible MCPDeprecationWarning).""" + + def fn(x: int, ctx: str) -> str: + return f"{x}:{ctx}" + + meta = func_metadata(fn, skip_names=["ctx"]) + with pytest.warns(MCPDeprecationWarning, match="call_fn_with_arg_validation"): + assert await meta.call_fn_with_arg_validation(fn, False, {"x": "2"}, {"ctx": "a"}) == "2:a" # pyright: ignore[reportDeprecated] + with pytest.warns(MCPDeprecationWarning): + validated = meta.validate_arguments({"x": 3}) + result = await meta.call_fn_with_arg_validation( # pyright: ignore[reportDeprecated] + fn, False, {}, {"ctx": "b"}, pre_validated=validated + ) + assert result == "3:b" + + def test_str_vs_list_str(): """Test handling of string vs list[str] type annotations. @@ -290,10 +312,10 @@ async def test_lambda_function(): } async def check_call(args): - return await meta.call_fn_with_arg_validation( + return await meta.call_fn( fn, fn_is_async=False, - arguments_to_validate=args, + arguments=meta.validate_arguments(args), arguments_to_pass_directly=None, ) @@ -555,10 +577,10 @@ def handle_json_payload(payload: str, strict_mode: bool = False) -> str: # Test with a JSON object string json_payload = '{"action": "create", "resource": "user", "data": {"name": "Test User"}}' - result = await meta.call_fn_with_arg_validation( + result = await meta.call_fn( handle_json_payload, fn_is_async=False, - arguments_to_validate={"payload": json_payload, "strict_mode": True}, + arguments=meta.validate_arguments({"payload": json_payload, "strict_mode": True}), arguments_to_pass_directly=None, ) @@ -568,10 +590,10 @@ def handle_json_payload(payload: str, strict_mode: bool = False) -> str: # Test with JSON array string json_array_payload = '["task1", "task2", "task3"]' - result = await meta.call_fn_with_arg_validation( + result = await meta.call_fn( handle_json_payload, fn_is_async=False, - arguments_to_validate={"payload": json_array_payload}, + arguments=meta.validate_arguments({"payload": json_array_payload}), arguments_to_pass_directly=None, ) @@ -1306,17 +1328,19 @@ def func_with_reserved_names( meta = func_metadata(func_with_reserved_names) # Test validation with reserved names - result = await meta.call_fn_with_arg_validation( + result = await meta.call_fn( func_with_reserved_names, fn_is_async=False, - arguments_to_validate={ - "model_dump": "test_dump", - "model_validate": 42, - "dict": ["a", "b", "c"], - "json": {"key": "value"}, - "validate": True, - "normal_param": "normal", - }, + arguments=meta.validate_arguments( + { + "model_dump": "test_dump", + "model_validate": 42, + "dict": ["a", "b", "c"], + "json": {"key": "value"}, + "validate": True, + "normal_param": "normal", + } + ), arguments_to_pass_directly=None, ) diff --git a/tests/server/mcpserver/test_server.py b/tests/server/mcpserver/test_server.py index 65810f4ba9..64b665d17f 100644 --- a/tests/server/mcpserver/test_server.py +++ b/tests/server/mcpserver/test_server.py @@ -1831,6 +1831,30 @@ async def complete(ref: PromptReference, argument: CompletionArgument, context: assert raised in _cause_chain(_logged_exception(caplog)) +async def test_completion_handler_returning_the_wrong_type_is_a_crash(caplog: pytest.LogCaptureFixture) -> None: + """SDK-defined: a completion handler whose return value isn't a Completion is the server's bug, so it is + logged as a crash and answered with the same generic -32603, not with 'Invalid request parameters'.""" + mcp = MCPServer() + + @mcp.completion() + async def complete(ref: PromptReference, argument: CompletionArgument, context: CompletionContext | None): + wrong: Any = ["bold", "italic"] + return wrong + + caplog.set_level(logging.INFO) + async with Client(mcp) as client: + with pytest.raises(MCPError) as exc: + await client.complete( + ref=PromptReference(type="ref/prompt", name="greet"), argument={"name": "style", "value": "b"} + ) + + assert exc.value.error == snapshot(ErrorData(code=INTERNAL_ERROR, message="Error completing argument style")) + assert _server_records(caplog) == snapshot( + [("ERROR", "Completion for argument 'style' raised an unexpected exception", True)] + ) + assert isinstance(_cause_chain(_logged_exception(caplog))[-1], ValidationError) + + async def test_completion_handler_raising_mcp_error_passes_through(caplog: pytest.LogCaptureFixture) -> None: """SDK-defined: MCPError from a completion handler keeps its code and message and is not logged.""" mcp = MCPServer() @@ -2413,8 +2437,8 @@ def add(a: int, b: int) -> int: assert result.is_error is True ((level, message, has_traceback),) = _server_records(caplog) assert (level, has_traceback) == ("INFO", False) - # Field names only: the rejected values are the caller's data and stay out of the log. - assert message == "Tool 'add' rejected arguments: a" + # Field names only, repr-quoted: the rejected values are the caller's data and stay out of the log. + assert message == "Tool 'add' rejected arguments: ['a']" assert not [r for r in caplog.records if r.levelno >= logging.WARNING]