From ae2da6c51e2333cbdf290ce7fd4c227a6dffc2fc Mon Sep 17 00:00:00 2001 From: Bernat Torres Date: Tue, 1 Sep 2026 11:04:26 +0200 Subject: [PATCH 1/2] fix(mcp): report the exception a tool raised when the SDK masks it 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 $mcp_error_message and $mcp_error_type scalars read $exception_list[0], so on mcp 2.1 every unexpected tool failure reported the same masked string and the failures view lost the reason. The scalars now read the entry behind the dispatch wrapper, which exceptions_from_error_tuple already records from __cause__. The wrapper is matched by type name and message prefix because each SDK major ships its own ToolError class. A wrapper without a chained cause is kept as is, and the $exception sibling still carries the full chain. This broke CI without a repo change: the rolling exclude-newer = "7 days" quarantine made mcp 2.1.0 (published Aug 24) eligible on Aug 31 at 19:04 UTC, between two runs of the same commit. The unpinned mcp>=2,<3 leg resolved 2.0.0 in the morning and 2.1.0 in the evening. mcp 2.1.1 ages in on Sep 1; the fix is verified against 2.0.0, 2.1.0, and 2.1.1. Generated-By: PostHog Desktop Task-Id: ec0cc27f-fc40-4994-b153-2bfe74de1edf --- .../mcp-unwrap-masked-tool-errors.md | 5 +++ posthog/mcp/_posthog_events.py | 42 +++++++++++++++---- posthog/test/mcp/test_error_properties.py | 42 +++++++++++++++++++ 3 files changed, 80 insertions(+), 9 deletions(-) create mode 100644 .sampo/changesets/mcp-unwrap-masked-tool-errors.md 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..93301f618 100644 --- a/posthog/mcp/_posthog_events.py +++ b/posthog/mcp/_posthog_events.py @@ -157,6 +157,36 @@ def _add_common_properties(event: Event, properties: Dict[str, Any]) -> None: properties["$set"] = {**identify_actor_data} +_TOOL_DISPATCH_WRAPPERS = ("ToolError", "UnexpectedToolError") + + +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 + read the entry behind it. Matched by type name and message prefix because + each SDK major ships its own ``ToolError`` class. + """ + if not isinstance(error, dict): + return {} + exception_list = error.get("$exception_list") + if not isinstance(exception_list, list) or not exception_list: + return {} + first = exception_list[0] if isinstance(exception_list[0], dict) else {} + if ( + len(exception_list) > 1 + and isinstance(exception_list[1], dict) + and first.get("type") in _TOOL_DISPATCH_WRAPPERS + and str(first.get("value", "")).startswith("Error executing tool") + ): + return exception_list[1] + return first + + def _add_error_details(event: Event, properties: Dict[str, Any]) -> None: """Surface the failure reason on the primary event itself. @@ -164,17 +194,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..aa3aa78be 100644 --- a/posthog/test/mcp/test_error_properties.py +++ b/posthog/test/mcp/test_error_properties.py @@ -23,6 +23,12 @@ def make_client(**kwargs): return client, captured +class ToolError(Exception): + """Stands in for the SDK's dispatch wrapper, which is matched by type name + because each SDK major ships its own class. Module-level like the real + ones, so the recorded type is the bare name and not a ```` path.""" + + 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 +159,42 @@ 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() + + try: + try: + raise ValueError("explode") + except ValueError as original: + raise ToolError("Error executing tool boom") from original + except ToolError 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=ToolError("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_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 — From bd6512141d07f76054b917f9c9d4d55c8f94200a Mon Sep 17 00:00:00 2001 From: Bernat Torres Date: Tue, 1 Sep 2026 11:58:10 +0200 Subject: [PATCH 2/2] fix(mcp): gate the unwrap on SDK module and traverse nested wrappers Two review findings on the dispatch-wrapper unwrap, both reproduced before fixing. A tool that invokes a failing tool is wrapped once per dispatch, so stepping past a single entry landed on the still-masked inner wrapper. The unwrap now walks every consecutive wrapper to the first real exception; a nested-server test asserts both the inner and the outer event report the root cause. Matching by type name and message prefix alone also unwrapped an application's own exception that happened to be named ToolError with a matching prefix, replacing the message the application chose to surface. The match now additionally requires the entry's recorded module to come from an SDK namespace (mcp., fastmcp.), verified against mcp 1.28.1, mcp 2.0.0/2.1.0/2.1.1, and standalone fastmcp. The unit tests use the real per-major ToolError classes, and a new test pins that a same-named application exception is kept. Generated-By: PostHog Desktop Task-Id: ec0cc27f-fc40-4994-b153-2bfe74de1edf --- posthog/mcp/_posthog_events.py | 36 +++++++--- posthog/test/mcp/test_error_properties.py | 83 +++++++++++++++++++++-- 2 files changed, 103 insertions(+), 16 deletions(-) diff --git a/posthog/mcp/_posthog_events.py b/posthog/mcp/_posthog_events.py index 93301f618..c9d9d0b85 100644 --- a/posthog/mcp/_posthog_events.py +++ b/posthog/mcp/_posthog_events.py @@ -159,6 +159,22 @@ def _add_common_properties(event: Event, properties: Dict[str, Any]) -> None: _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. @@ -168,23 +184,23 @@ def _primary_exception(error: Any) -> Dict[str, Any]: 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 - read the entry behind it. Matched by type name and message prefix because - each SDK major ships its own ``ToolError`` class. + 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 {} - first = exception_list[0] if isinstance(exception_list[0], dict) else {} - if ( - len(exception_list) > 1 - and isinstance(exception_list[1], dict) - and first.get("type") in _TOOL_DISPATCH_WRAPPERS - and str(first.get("value", "")).startswith("Error executing tool") + index = 0 + while ( + index + 1 < len(exception_list) + and isinstance(exception_list[index + 1], dict) + and _is_dispatch_wrapper(exception_list[index]) ): - return exception_list[1] - return first + index += 1 + entry = exception_list[index] + return entry if isinstance(entry, dict) else {} def _add_error_details(event: Event, properties: Dict[str, Any]) -> None: diff --git a/posthog/test/mcp/test_error_properties.py b/posthog/test/mcp/test_error_properties.py index aa3aa78be..09ac2f612 100644 --- a/posthog/test/mcp/test_error_properties.py +++ b/posthog/test/mcp/test_error_properties.py @@ -24,9 +24,20 @@ def make_client(**kwargs): class ToolError(Exception): - """Stands in for the SDK's dispatch wrapper, which is matched by type name - because each SDK major ships its own class. Module-level like the real - ones, so the recorded type is the bare name and not a ```` path.""" + """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(): @@ -164,13 +175,14 @@ async def test_the_sdk_dispatch_wrapper_is_unwrapped_to_its_cause(): `` 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 ToolError("Error executing tool boom") from original - except ToolError as wrapper: + 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() @@ -186,7 +198,7 @@ async def test_a_dispatch_wrapper_without_a_cause_is_kept(): client, captured = make_client() client.capture_tool_call( - "boom", is_error=True, error=ToolError("Error executing tool boom") + "boom", is_error=True, error=_sdk_tool_error()("Error executing tool boom") ) await _flush() @@ -195,6 +207,65 @@ async def test_a_dispatch_wrapper_without_a_cause_is_kept(): 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 —