diff --git a/.sampo/changesets/mcp-lowlevel-raised-error.md b/.sampo/changesets/mcp-lowlevel-raised-error.md new file mode 100644 index 000000000..312638b87 --- /dev/null +++ b/.sampo/changesets/mcp-lowlevel-raised-error.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +`posthog.mcp`: on mcp 1.x (1.10 and later), a tool registered with the low-level `Server`'s `@server.call_tool()` decorator that raises now reports the exception it raised, with its cause chain, stack frames and `$mcp_error_status`, and `$mcp_error_type` names its class. Before, the decorator converted the exception into an error result first, so the failure was a generic `Error` with no frames and no status. diff --git a/posthog/mcp/_instrument_lowlevel.py b/posthog/mcp/_instrument_lowlevel.py index 8d6cf4393..ace769fd3 100644 --- a/posthog/mcp/_instrument_lowlevel.py +++ b/posthog/mcp/_instrument_lowlevel.py @@ -54,7 +54,7 @@ from ._model_parameters import request_meta_from_context from ._model_parameters import can_inject_model_parameter, is_capture_model_enabled from ._output_instructions import mirror_instructions_into_structured_content -from ._raised_tool_error import raised_tool_error +from ._raised_tool_error import raised_tool_error, wrap_error_result from .logger import log, warn from .tools import get_more_tools_result_text @@ -69,6 +69,7 @@ def instrument_low_level(server: Any, data: MCPAnalyticsData) -> None: """ data.server_name = getattr(server, "name", None) data.server_version = getattr(server, "version", None) + wrap_error_result(server) _wrap_call_tool(server, data, strip_injected=False) _wrap_list_tools(server, data, context_required=False) _wrap_resource_requests(server, data) @@ -283,7 +284,9 @@ async def handler(req: Any) -> Any: start = time.monotonic() try: - with raised_tool_error(high_level) as raised: + with raised_tool_error( + server if high_level is None else high_level + ) as raised: result = await original(req) except Exception as error: if await lifecycle.record_if_unknown_tool( diff --git a/posthog/mcp/_raised_tool_error.py b/posthog/mcp/_raised_tool_error.py index e694cf6fe..6cb375f54 100644 --- a/posthog/mcp/_raised_tool_error.py +++ b/posthog/mcp/_raised_tool_error.py @@ -1,13 +1,15 @@ -"""Recover the exception behind a standalone FastMCP ``isError`` tool result. +"""Recover the exception behind an ``isError`` tool result. -FastMCP's ``tools/call`` handler, the one we wrap, turns whatever its tool -dispatch raised into ``CallToolResult(isError=True)``, keeping only the -message text. Wrapping that dispatch on the server instance sees the exception -first, and hands it to the enclosing handler through a context variable. +Standalone FastMCP's ``tools/call`` handler, and the mcp 1.x low-level +``@server.call_tool()`` decorator, turn whatever the tool raised into +``CallToolResult(isError=True)``, keeping only the message text. A wrapper on +the server instance sees the exception first, and hands it to the enclosing +handler through a context variable. """ from __future__ import annotations +import sys from contextlib import contextmanager from contextvars import ContextVar from dataclasses import dataclass @@ -19,6 +21,10 @@ # `call_tool` and dispatches through `_call_tool_middleware`. _DISPATCH_METHODS = ("call_tool", "_call_tool_middleware") +# The prefixes the mcp 1.x `@server.call_tool()` decorator puts on jsonschema +# validation failures. +_VALIDATION_PREFIXES = ("Input validation error: ", "Output validation error: ") + @dataclass class RaisedToolError: @@ -60,6 +66,38 @@ async def dispatch(*args: Any, **kwargs: Any) -> Any: setattr(server, method, dispatch) +def wrap_error_result(server: Any) -> None: + original = getattr(server, "_make_error_result", None) + if original is None or getattr(original, _WRAPPED_FLAG, False): + return + + def make_error_result(error_message: str) -> Any: + error = sys.exc_info()[1] + if isinstance(error, Exception) and _carries_bare_text(error, error_message): + _settle(server, error) + return original(error_message) + + setattr(make_error_result, _WRAPPED_FLAG, True) + setattr(server, "_make_error_result", make_error_result) + + +def _carries_bare_text(error: Exception, error_message: str) -> bool: + # Input and output validation failures also land here from inside an + # except block, with the validator's short message behind a prefix. Only + # the catch-all around the tool call passes an exception's bare text. + # Rendering a validation error walks the whole instance and schema, so + # those results are told apart before `str(error)` runs. + try: + detail = getattr(error, "message", None) + if isinstance(detail, str) and any( + error_message == prefix + detail for prefix in _VALIDATION_PREFIXES + ): + return False + return error_message == str(error) + except Exception: + return False + + def _settle(server: Any, error: Optional[BaseException]) -> None: # Another server's dispatch, reached from inside a tool, must not write # its outcome onto this call, nor may a background task that outlives it. diff --git a/posthog/test/mcp/test_lowlevel.py b/posthog/test/mcp/test_lowlevel.py index 62dbd1ed0..8cbff6413 100644 --- a/posthog/test/mcp/test_lowlevel.py +++ b/posthog/test/mcp/test_lowlevel.py @@ -2,6 +2,7 @@ import json +import httpx import pytest import mcp.types as mcp_types @@ -17,7 +18,25 @@ ) -def make_server(*, resource_error: bool = False, listing: str = "resources") -> Server: +def _boom(): + raise ValueError("boom") + + +def _upstream_503(): + request = httpx.Request("GET", "https://api.example.com/invoices/1") + httpx.Response(503, request=request).raise_for_status() + + +def _wrapped_upstream_503(): + try: + _upstream_503() + except httpx.HTTPStatusError as exc: + raise RuntimeError("invoice lookup failed") from exc + + +def make_server( + *, resource_error: bool = False, listing: str = "resources", fail=_boom +) -> Server: server = Server("test-lowlevel") @server.list_tools() @@ -38,7 +57,7 @@ async def list_tools(): async def call_tool(name, arguments): if name == "echo": return [mcp_types.TextContent(type="text", text=str(arguments.get("msg")))] - raise ValueError("boom") + fail() async def list_resources(_request): if listing == "error": @@ -344,26 +363,79 @@ async def test_tool_call_success_captures_intent(): assert "context" not in props["$mcp_parameters"]["request"]["params"]["arguments"] -async def test_tool_call_error_captured_from_is_error_result(): - server = make_server() +def _worded_like_validation(): + raise ValueError("Input validation error: quota exceeded") + + +def _nested(depth): + value = 0 + for _ in range(depth): + value = [value] + return value + + +@pytest.mark.parametrize( + "fail, name, payload, error_type, chain, status, response", + [ + (_boom, "fail", None, "ValueError", ["ValueError"], None, "boom"), + ( + _upstream_503, + "fail", + None, + "HTTPStatusError", + ["HTTPStatusError"], + 503, + "Server error '503 Service Unavailable'", + ), + ( + _wrapped_upstream_503, + "fail", + None, + "RuntimeError", + ["RuntimeError", "HTTPStatusError"], + 503, + "invoice lookup failed", + ), + ( + _worded_like_validation, + "fail", + None, + "ValueError", + ["ValueError"], + None, + "Input validation error: quota exceeded", + ), + # Missing "msg" fails the SDK's input validation before the tool runs. + (_boom, "echo", None, "Error", [], None, "Input validation error"), + (_boom, "echo", _nested(400), "Error", [], None, "Input validation error"), + ], +) +async def test_raise_converted_to_is_error_result_carries_the_raised_exception( + fail, name, payload, error_type, chain, status, response +): + server = make_server(fail=fail) client = FakeClient() instrument(server, client) + await server.request_handlers[mcp_types.ListToolsRequest](None) - handler = server.request_handlers[mcp_types.CallToolRequest] - # "fail" is unlisted -> no validation -> handler raises -> isError result - result = await handler( - _call_request("fail", {"context": "trying a tool that errors"}) + result = await server.request_handlers[mcp_types.CallToolRequest]( + _call_request( + name, {"context": "trying a tool that errors", "payload": payload} + ) ) await _flush() assert result.root.isError is True - calls = _events(client, "$mcp_tool_call") - assert calls and calls[0]["properties"]["$mcp_is_error"] is True - exceptions = _events(client, "$exception") - assert ( - exceptions - and "boom" in exceptions[0]["properties"]["$exception_list"][0]["value"] - ) + assert result.root.content[0].text.startswith(response) + [call] = _events(client, "$mcp_tool_call") + assert call["properties"]["$mcp_is_error"] is True + assert call["properties"]["$mcp_error_type"] == error_type + assert call["properties"].get("$mcp_error_status") == status + [exception] = _events(client, "$exception") + exception_list = exception["properties"]["$exception_list"] + raised = [e for e in exception_list if e["type"] in chain] + assert [e["type"] for e in raised] == chain + assert all(e["stacktrace"]["frames"] for e in raised) async def test_initialize_emitted_once():