diff --git a/.sampo/changesets/mcp-unwrap-masked-tool-errors.md b/.sampo/changesets/mcp-unwrap-masked-tool-errors.md new file mode 100644 index 000000000..75a380436 --- /dev/null +++ b/.sampo/changesets/mcp-unwrap-masked-tool-errors.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +MCP tool failures now report the exception the tool actually raised on `$mcp_error_message` and `$mcp_error_type`, stepping past the SDK's dispatch `ToolError` wrapper. mcp 2.1 masks the original message out of that wrapper, which left the failures view with only `Error executing tool `. The `$exception` sibling still carries the full chain. diff --git a/posthog/mcp/_posthog_events.py b/posthog/mcp/_posthog_events.py index a7c5b79c4..c9d9d0b85 100644 --- a/posthog/mcp/_posthog_events.py +++ b/posthog/mcp/_posthog_events.py @@ -157,6 +157,52 @@ def _add_common_properties(event: Event, properties: Dict[str, Any]) -> None: properties["$set"] = {**identify_actor_data} +_TOOL_DISPATCH_WRAPPERS = ("ToolError", "UnexpectedToolError") + +# Where the SDKs define their dispatch wrappers: mcp.server.fastmcp.exceptions +# (mcp 1.x), mcp.server.mcpserver.exceptions (mcp 2.x), fastmcp.exceptions +# (standalone fastmcp). An application's own exception carries its own module, +# so a matching name alone must not unwrap it. +_SDK_MODULE_PREFIXES = ("mcp.", "fastmcp.") + + +def _is_dispatch_wrapper(entry: Any) -> bool: + if not isinstance(entry, dict): + return False + return ( + entry.get("type") in _TOOL_DISPATCH_WRAPPERS + and str(entry.get("module") or "").startswith(_SDK_MODULE_PREFIXES) + and str(entry.get("value", "")).startswith("Error executing tool") + ) + + +def _primary_exception(error: Any) -> Dict[str, Any]: + """Pick the ``$exception_list`` entry that carries the failure reason. + + The MCP SDK's tool dispatch re-raises whatever a tool raised as a + ``ToolError`` whose message starts ``Error executing tool ``, and + mcp >= 2.1 masks the original text out of that message entirely, keeping + it only on ``__cause__`` — the next entry of the chain here. The wrapper + says nothing the event's tool name does not already say, so the scalars + step past every consecutive wrapper (a tool invoking a failing tool is + wrapped once per dispatch) to the first real exception. + """ + if not isinstance(error, dict): + return {} + exception_list = error.get("$exception_list") + if not isinstance(exception_list, list) or not exception_list: + return {} + index = 0 + while ( + index + 1 < len(exception_list) + and isinstance(exception_list[index + 1], dict) + and _is_dispatch_wrapper(exception_list[index]) + ): + index += 1 + entry = exception_list[index] + return entry if isinstance(entry, dict) else {} + + def _add_error_details(event: Event, properties: Dict[str, Any]) -> None: """Surface the failure reason on the primary event itself. @@ -164,17 +210,11 @@ def _add_error_details(event: Event, properties: Dict[str, Any]) -> None: know *why* a call failed — and that sibling can be switched off with ``enable_exception_autocapture``, or never emitted when no error value was passed. Both values are read off the ``$exception_list`` the sibling would - carry, so the two always agree; the message is already bounded to + carry — the sibling keeps the full chain while the scalars carry the entry + ``_primary_exception`` picks; the message is already bounded to ``_MAX_ERROR_MESSAGE_LENGTH`` because truncation runs before this mapping. """ - first: Dict[str, Any] = {} - error = event.get("error") - if isinstance(error, dict): - exception_list = error.get("$exception_list") - if isinstance(exception_list, list) and exception_list: - candidate = exception_list[0] - if isinstance(candidate, dict): - first = candidate + first = _primary_exception(event.get("error")) # An explicit coarse category (e.g. "validation", "timeout") beats the # thrown type; a custom dispatcher can pass one that means something to the diff --git a/posthog/test/mcp/test_error_properties.py b/posthog/test/mcp/test_error_properties.py index 9c978553e..09ac2f612 100644 --- a/posthog/test/mcp/test_error_properties.py +++ b/posthog/test/mcp/test_error_properties.py @@ -23,6 +23,23 @@ def make_client(**kwargs): return client, captured +class ToolError(Exception): + """An application's own ToolError. It shares the SDK wrapper's name but not + its module, so the unwrap must leave it alone. Module-level so the recorded + type is the bare name and not a ```` path.""" + + +def _sdk_tool_error() -> type: + """The real dispatch-wrapper class for the installed SDK major.""" + from posthog.test.mcp._helpers import MCP_MAJOR + + if MCP_MAJOR >= 2: + from mcp.server.mcpserver.exceptions import ToolError as SDKToolError + else: + from mcp.server.fastmcp.exceptions import ToolError as SDKToolError + return SDKToolError + + async def test_failed_call_carries_message_and_type(): client, captured = make_client() client.capture_tool_call("add", is_error=True, error=ValueError("bad input")) @@ -153,6 +170,102 @@ def boom() -> str: assert "explode" in props[P.ERROR_MESSAGE] +async def test_the_sdk_dispatch_wrapper_is_unwrapped_to_its_cause(): + """mcp >= 2.1 masks an unexpected tool exception to ``Error executing tool + `` and keeps the original only on ``__cause__``. The scalars must + carry that original; the ``$exception`` sibling keeps the full chain.""" + client, captured = make_client() + sdk_tool_error = _sdk_tool_error() + + try: + try: + raise ValueError("explode") + except ValueError as original: + raise sdk_tool_error("Error executing tool boom") from original + except sdk_tool_error as wrapper: + client.capture_tool_call("boom", is_error=True, error=wrapper) + await _flush() + + props = _events(captured, "$mcp_tool_call")[0]["properties"] + assert props[P.ERROR_MESSAGE] == "explode" + assert props[P.ERROR_TYPE] == "ValueError" + sibling = _events(captured, "$exception")[0]["properties"]["$exception_list"] + assert sibling[0]["type"] == "ToolError" + + +async def test_a_dispatch_wrapper_without_a_cause_is_kept(): + """With no chained cause the wrapper's own message is all there is.""" + client, captured = make_client() + + client.capture_tool_call( + "boom", is_error=True, error=_sdk_tool_error()("Error executing tool boom") + ) + await _flush() + + props = _events(captured, "$mcp_tool_call")[0]["properties"] + assert props[P.ERROR_MESSAGE] == "Error executing tool boom" + assert props[P.ERROR_TYPE] == "ToolError" + + +async def test_an_application_error_sharing_the_wrapper_name_is_kept(): + """``capture_tool_call`` accepts arbitrary exceptions. The wrapper is + matched by SDK module, not just name, so an application's own ToolError + keeps the message and type the application chose to surface.""" + client, captured = make_client() + + try: + try: + raise ValueError("inner detail") + except ValueError as original: + raise ToolError("Error executing tool application task") from original + except ToolError as wrapper: + client.capture_tool_call("task", is_error=True, error=wrapper) + await _flush() + + props = _events(captured, "$mcp_tool_call")[0]["properties"] + assert props[P.ERROR_MESSAGE] == "Error executing tool application task" + assert props[P.ERROR_TYPE] == "ToolError" + + +async def test_nested_dispatch_wrappers_unwrap_to_the_root_cause(): + """An outer tool that invokes a failing inner tool gets wrapped twice — + once per dispatch — so the scalars must step past every wrapper, not just + the first, on both the inner and the outer event.""" + from posthog.test.mcp._helpers import MCP_MAJOR, FakeClient + + if MCP_MAJOR >= 2: + from mcp.server.mcpserver import MCPServer as Server + else: + from mcp.server.fastmcp import FastMCP as Server + + from posthog.mcp import instrument + + server = Server("nested-e2e") + + @server.tool() + def inner() -> str: + raise ValueError("root failure") + + @server.tool() + async def outer() -> str: + return await server._tool_manager.call_tool("inner", {}) + + client = FakeClient() + instrument(server, client) + + try: + await server._tool_manager.call_tool("outer", {}) + except Exception: + pass + await _flush() + + calls = _events(client, "$mcp_tool_call") + assert len(calls) == 2 + for call in calls: + assert call["properties"][P.IS_ERROR] is True + assert "root failure" in call["properties"][P.ERROR_MESSAGE] + + async def test_a_secret_in_the_message_is_redacted_on_both_surfaces(): """An exception message is free text a server wrote, so it can carry the credential that caused the failure. It must be redacted before it leaves —