From a7f86838e4eeee7667bcbd9be03b52e765912816 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Tue, 10 Nov 2026 20:35:52 -0300 Subject: [PATCH 1/9] test(mcp): failing repro for FastMCP tool failures losing the raised exception Standalone FastMCP converts a raised tool exception into an isError result before our wrapped handler returns, so the failure is reported as a generic "Error" with no frames (posthog-python#1223). Fails on fastmcp 3/mcp 1 and fastmcp 4/mcp 2. --- posthog/test/mcp/test_fastmcp_tool_errors.py | 93 ++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 posthog/test/mcp/test_fastmcp_tool_errors.py diff --git a/posthog/test/mcp/test_fastmcp_tool_errors.py b/posthog/test/mcp/test_fastmcp_tool_errors.py new file mode 100644 index 000000000..b72834205 --- /dev/null +++ b/posthog/test/mcp/test_fastmcp_tool_errors.py @@ -0,0 +1,93 @@ +"""Standalone FastMCP turns a raised tool exception into an ``isError`` result +before the handler we wrap returns. The failure must still carry what was raised +(posthog-python#1223).""" + +import pytest + +pytest.importorskip("fastmcp") + +from fastmcp import Client, FastMCP # noqa: E402 +from fastmcp.exceptions import ToolError # noqa: E402 + +from posthog.mcp import MCPAnalyticsOptions, instrument # noqa: E402 +from posthog.test.mcp._helpers import FakeClient, events_named, flush_background # noqa: E402 + + +def _upstream(): + raise ConnectionError("upstream down") + + +def _chained(): + try: + _upstream() + except ConnectionError as exc: + raise RuntimeError("lookup failed") from exc + + +def _tool_error(): + raise ToolError("bad input") + + +@pytest.mark.parametrize( + "raise_in_tool, mask_error_details, error_type, message, chain", + [ + (_chained, False, "RuntimeError", "lookup failed", ["RuntimeError", "ConnectionError"]), + (_chained, True, "RuntimeError", "lookup failed", ["RuntimeError", "ConnectionError"]), + (_tool_error, False, "ToolError", "bad input", ["ToolError"]), + ], +) +async def test_failed_call_carries_the_raised_exception( + raise_in_tool, mask_error_details, error_type, message, chain +): + server = FastMCP("example-server", mask_error_details=mask_error_details) + + @server.tool + def fail() -> str: + raise_in_tool() + return "unreachable" + + sink = FakeClient() + instrument(server, sink, MCPAnalyticsOptions()) + + async with Client(server) as client: + result = await client.call_tool("fail", {}, raise_on_error=False) + await flush_background() + + assert result.is_error + [call] = events_named(sink, "$mcp_tool_call") + assert call["properties"]["$mcp_is_error"] is True + assert call["properties"]["$mcp_error_type"] == error_type + assert call["properties"]["$mcp_error_message"] == message + [exception] = events_named(sink, "$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_exception_caught_by_the_tool_is_not_reported(): + server = FastMCP("example-server") + + @server.tool + def inner() -> str: + raise RuntimeError("inner failure") + + @server.tool + async def outer() -> str: + try: + await server.call_tool("inner", {}) + except ToolError: + pass + return "recovered" + + sink = FakeClient() + instrument(server, sink, MCPAnalyticsOptions()) + + async with Client(server) as client: + result = await client.call_tool("outer", {}, raise_on_error=False) + await flush_background() + + assert not result.is_error + [call] = events_named(sink, "$mcp_tool_call") + assert call["properties"]["$mcp_is_error"] is False + assert events_named(sink, "$exception") == [] From 8bbd1fbd5f33094d51b51462cca52ffdd2e7a933 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Tue, 10 Nov 2026 20:37:55 -0300 Subject: [PATCH 2/9] fix(mcp): report the exception behind a FastMCP isError tool result Standalone FastMCP converts a raised tool exception into CallToolResult(isError=True) inside the tools/call handler we wrap, so every failure was captured from the result text: $mcp_error_type "Error", one generic $exception_list entry, no frames (posthog-python#1223). instrument() now wraps the server's public call_tool, which still raises (ToolError chained from the tool's exception). The wrapper hands the outermost call's outcome to the enclosing handler through a context variable, and the handler records it next to the result. fastmcp's "Error calling tool" ToolError joins the dispatch wrappers the scalars step past, so $mcp_error_type names the tool's own exception. Tested: - test_fastmcp_tool_errors.py fails before, passes after, on fastmcp 3.2 + mcp 1.30 and fastmcp 4.1.8 + mcp 2.3 - pytest posthog/test/mcp: 694 passed (mcp 1.x), 575 passed (mcp 2.x) - issue repro: $mcp_error_type RuntimeError, chain ToolError -> RuntimeError -> ConnectionError with frames - ruff format/check, mypy clean Not included: $mcp_error_status. The taxonomy defines it as the upstream status of PostHog's own server calls and no SDK sets it today; setting it from the SDK is a cross-SDK contract change. --- .../fastmcp-tool-error-exception.md | 5 ++ posthog/mcp/__init__.py | 7 ++- posthog/mcp/_instrument_lowlevel.py | 5 +- posthog/mcp/_instrument_v2.py | 9 ++- posthog/mcp/_instrumentation.py | 17 +++-- posthog/mcp/_posthog_events.py | 1 + posthog/mcp/_raised_tool_error.py | 62 +++++++++++++++++++ posthog/test/mcp/test_fastmcp_tool_errors.py | 16 ++++- 8 files changed, 109 insertions(+), 13 deletions(-) create mode 100644 .sampo/changesets/fastmcp-tool-error-exception.md create mode 100644 posthog/mcp/_raised_tool_error.py diff --git a/.sampo/changesets/fastmcp-tool-error-exception.md b/.sampo/changesets/fastmcp-tool-error-exception.md new file mode 100644 index 000000000..ab1520bd5 --- /dev/null +++ b/.sampo/changesets/fastmcp-tool-error-exception.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +`posthog.mcp`: a failed tool call on a standalone FastMCP server now reports the exception the tool raised, with its cause chain and stack frames, and `$mcp_error_type` names its class. Before, FastMCP converted the exception into an error result first, so the failure was a generic `Error` with no frames. diff --git a/posthog/mcp/__init__.py b/posthog/mcp/__init__.py index 98a73bede..f81e21908 100644 --- a/posthog/mcp/__init__.py +++ b/posthog/mcp/__init__.py @@ -290,6 +290,7 @@ def instrument( from ._instrument_fastmcp import instrument_fastmcp from ._instrument_lowlevel import instrument_fastmcp_v2, instrument_low_level from ._instrument_v2 import instrument_lowlevel_v2, instrument_mcpserver_v2 + from ._raised_tool_error import wrap_call_tool client = _resolve_client(posthog_client) if client is None: @@ -306,8 +307,10 @@ def instrument( server_build=server_build, ) - if is_fastmcp_v2(server) and uses_v2_handler_registry(key): - data.standalone_fastmcp = weakref.ref(server) + if is_fastmcp_v2(server): + wrap_call_tool(server) + if uses_v2_handler_registry(key): + data.standalone_fastmcp = weakref.ref(server) # A standalone FastMCP wrapper and its low-level server share one tracking # key, so instrumenting the second of the pair must still attach what only diff --git a/posthog/mcp/_instrument_lowlevel.py b/posthog/mcp/_instrument_lowlevel.py index a8ab58671..c8ad2f383 100644 --- a/posthog/mcp/_instrument_lowlevel.py +++ b/posthog/mcp/_instrument_lowlevel.py @@ -54,6 +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 .logger import log, warn from .tools import get_more_tools_result_text @@ -282,7 +283,8 @@ async def handler(req: Any) -> Any: start = time.monotonic() try: - result = await original(req) + with raised_tool_error() as raised: + result = await original(req) except Exception as error: if await lifecycle.record_if_unknown_tool( await registered_tool_exists(high_level, name) @@ -349,6 +351,7 @@ async def handler(req: Any) -> Any: call_result, duration_ms, conversation_id_delivered=delivered, + error=raised.error, ) return result diff --git a/posthog/mcp/_instrument_v2.py b/posthog/mcp/_instrument_v2.py index b7d597789..bc6768cbb 100644 --- a/posthog/mcp/_instrument_v2.py +++ b/posthog/mcp/_instrument_v2.py @@ -75,6 +75,7 @@ request_meta_from_context, ) from ._output_instructions import mirror_instructions_into_structured_content +from ._raised_tool_error import raised_tool_error from ._tool_input import get_registered_tool_input_schema from .asgi import get_mcp_session from .logger import log @@ -673,7 +674,8 @@ async def handler(ctx: Any, params: Any) -> Any: start = time.monotonic() try: - result = await original(ctx, params) + with raised_tool_error() as raised: + result = await original(ctx, params) except Exception as error: if await lifecycle.record_if_unknown_tool( await registered_tool_exists(standalone, name) @@ -702,7 +704,10 @@ async def handler(ctx: Any, params: Any) -> Any: return result await lifecycle.record_result( - result, duration_ms, conversation_id_delivered=delivered + result, + duration_ms, + conversation_id_delivered=delivered, + error=raised.error, ) return result diff --git a/posthog/mcp/_instrumentation.py b/posthog/mcp/_instrumentation.py index 9b1db44c7..023497620 100644 --- a/posthog/mcp/_instrumentation.py +++ b/posthog/mcp/_instrumentation.py @@ -665,7 +665,12 @@ async def record_error(self, error: Any, duration_ms: float) -> None: ) async def record_result( - self, result: Any, duration_ms: float, *, conversation_id_delivered: bool + self, + result: Any, + duration_ms: float, + *, + conversation_id_delivered: bool, + error: Optional[BaseException] = None, ) -> None: conversation_id = self._anchored_conversation_id(conversation_id_delivered) session_id = await self.prepare_session(conversation_id) @@ -678,6 +683,7 @@ async def record_result( allow_self_reported_model=self.allow_self_reported_model, analytics_owned_parameters=self.analytics_owned_parameters, result=result, + error=error, duration_ms=duration_ms, client_name=self.client_name, client_version=self.client_version, @@ -813,14 +819,13 @@ async def record_tool_call( event["llm_model"] = model event["llm_model_source"] = source + if result is not None: + event["response"] = _wrap_response(result) + if error is None and is_tool_result_error(result): + error = result if error is not None: event["is_error"] = True event["error"] = capture_exception(error) - elif result is not None: - event["response"] = _wrap_response(result) - if is_tool_result_error(result): - event["is_error"] = True - event["error"] = capture_exception(result) props = await resolve_event_properties(data, request, extra) input_aliases = None diff --git a/posthog/mcp/_posthog_events.py b/posthog/mcp/_posthog_events.py index f7b58f0e7..40894ff0d 100644 --- a/posthog/mcp/_posthog_events.py +++ b/posthog/mcp/_posthog_events.py @@ -178,6 +178,7 @@ def _add_common_properties(event: Event, properties: Dict[str, Any]) -> None: # its text, so reporting it loses nothing. _DISPATCH_WRAPPERS = ( (("ToolError", "UnexpectedToolError"), "Error executing tool"), + (("ToolError",), "Error calling tool"), (("MCPError", "UnexpectedResourceError"), "Error reading resource"), ) diff --git a/posthog/mcp/_raised_tool_error.py b/posthog/mcp/_raised_tool_error.py new file mode 100644 index 000000000..03972c216 --- /dev/null +++ b/posthog/mcp/_raised_tool_error.py @@ -0,0 +1,62 @@ +"""Recover the exception behind a standalone FastMCP ``isError`` tool result. + +FastMCP's ``tools/call`` handler, the one we wrap, turns whatever its public +``call_tool`` raised into ``CallToolResult(isError=True)``, keeping only the +message text. Wrapping ``call_tool`` on the server instance sees the exception +first, and hands it to the enclosing handler through a context variable. +""" + +from __future__ import annotations + +from contextlib import contextmanager +from contextvars import ContextVar +from dataclasses import dataclass +from typing import Any, Iterator, Optional + +_WRAPPED_FLAG = "__posthog_mcp_wrapped__" + + +@dataclass +class RaisedToolError: + error: Optional[BaseException] = None + + +_current: ContextVar[Optional[RaisedToolError]] = ContextVar( + "posthog_mcp_raised_tool_error", default=None +) + + +def wrap_call_tool(server: Any) -> None: + original = server.call_tool + if getattr(original, _WRAPPED_FLAG, False): + return + + async def call_tool(*args: Any, **kwargs: Any) -> Any: + # A tool calling another tool nests these calls. The outermost call + # settles last, so the slot ends holding the outcome the client sees. + try: + result = await original(*args, **kwargs) + except Exception as error: + _settle(error) + raise + _settle(None) + return result + + setattr(call_tool, _WRAPPED_FLAG, True) + server.call_tool = call_tool + + +def _settle(error: Optional[BaseException]) -> None: + raised = _current.get() + if raised is not None: + raised.error = error + + +@contextmanager +def raised_tool_error() -> Iterator[RaisedToolError]: + raised = RaisedToolError() + token = _current.set(raised) + try: + yield raised + finally: + _current.reset(token) diff --git a/posthog/test/mcp/test_fastmcp_tool_errors.py b/posthog/test/mcp/test_fastmcp_tool_errors.py index b72834205..211a081bf 100644 --- a/posthog/test/mcp/test_fastmcp_tool_errors.py +++ b/posthog/test/mcp/test_fastmcp_tool_errors.py @@ -31,8 +31,20 @@ def _tool_error(): @pytest.mark.parametrize( "raise_in_tool, mask_error_details, error_type, message, chain", [ - (_chained, False, "RuntimeError", "lookup failed", ["RuntimeError", "ConnectionError"]), - (_chained, True, "RuntimeError", "lookup failed", ["RuntimeError", "ConnectionError"]), + ( + _chained, + False, + "RuntimeError", + "lookup failed", + ["RuntimeError", "ConnectionError"], + ), + ( + _chained, + True, + "RuntimeError", + "lookup failed", + ["RuntimeError", "ConnectionError"], + ), (_tool_error, False, "ToolError", "bad input", ["ToolError"]), ], ) From 81b3170570c61346a7a2d222b3e5ec9b2388a80c Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Tue, 10 Nov 2026 20:47:26 -0300 Subject: [PATCH 3/9] fix(mcp): keep another server's handled tool failure off this call Pre-PR gate findings on the previous commit: - The error slot was shared across servers. A tool that called another instrumented FastMCP's call_tool and handled its failure got that exception on its own successful event, whenever its own server's call_tool was not wrapped (a raw low-level server, or a FastMCP instrumented only through _mcp_server). The slot now names the server whose handler opened it, and only that server's call_tool writes to it. - The "Error calling tool" wrapper match also unwrapped a tool's own ToolError worded that way. It now requires fastmcp's quoted tool name. Tested: both repros added to test_fastmcp_tool_errors.py; pytest posthog/test/mcp 697 passed (mcp 1.30 + fastmcp 3.2), 578 passed (mcp 2.3 + fastmcp 4.1.8); ruff and mypy clean. --- posthog/mcp/_instrument_lowlevel.py | 2 +- posthog/mcp/_instrument_v2.py | 2 +- posthog/mcp/_posthog_events.py | 2 +- posthog/mcp/_raised_tool_error.py | 15 ++++---- posthog/test/mcp/test_fastmcp_tool_errors.py | 36 +++++++++++++++++--- 5 files changed, 44 insertions(+), 13 deletions(-) diff --git a/posthog/mcp/_instrument_lowlevel.py b/posthog/mcp/_instrument_lowlevel.py index c8ad2f383..8d6cf4393 100644 --- a/posthog/mcp/_instrument_lowlevel.py +++ b/posthog/mcp/_instrument_lowlevel.py @@ -283,7 +283,7 @@ async def handler(req: Any) -> Any: start = time.monotonic() try: - with raised_tool_error() as raised: + with raised_tool_error(high_level) as raised: result = await original(req) except Exception as error: if await lifecycle.record_if_unknown_tool( diff --git a/posthog/mcp/_instrument_v2.py b/posthog/mcp/_instrument_v2.py index bc6768cbb..da85d2c06 100644 --- a/posthog/mcp/_instrument_v2.py +++ b/posthog/mcp/_instrument_v2.py @@ -674,7 +674,7 @@ async def handler(ctx: Any, params: Any) -> Any: start = time.monotonic() try: - with raised_tool_error() as raised: + with raised_tool_error(standalone) as raised: result = await original(ctx, params) except Exception as error: if await lifecycle.record_if_unknown_tool( diff --git a/posthog/mcp/_posthog_events.py b/posthog/mcp/_posthog_events.py index 40894ff0d..e20971aed 100644 --- a/posthog/mcp/_posthog_events.py +++ b/posthog/mcp/_posthog_events.py @@ -178,7 +178,7 @@ def _add_common_properties(event: Event, properties: Dict[str, Any]) -> None: # its text, so reporting it loses nothing. _DISPATCH_WRAPPERS = ( (("ToolError", "UnexpectedToolError"), "Error executing tool"), - (("ToolError",), "Error calling tool"), + (("ToolError",), "Error calling tool '"), (("MCPError", "UnexpectedResourceError"), "Error reading resource"), ) diff --git a/posthog/mcp/_raised_tool_error.py b/posthog/mcp/_raised_tool_error.py index 03972c216..ad6f85051 100644 --- a/posthog/mcp/_raised_tool_error.py +++ b/posthog/mcp/_raised_tool_error.py @@ -18,6 +18,7 @@ @dataclass class RaisedToolError: + server: Any error: Optional[BaseException] = None @@ -37,24 +38,26 @@ async def call_tool(*args: Any, **kwargs: Any) -> Any: try: result = await original(*args, **kwargs) except Exception as error: - _settle(error) + _settle(server, error) raise - _settle(None) + _settle(server, None) return result setattr(call_tool, _WRAPPED_FLAG, True) server.call_tool = call_tool -def _settle(error: Optional[BaseException]) -> None: +def _settle(server: Any, error: Optional[BaseException]) -> None: + # Another server's call_tool, reached from inside a tool, must not write + # its outcome onto this call. raised = _current.get() - if raised is not None: + if raised is not None and raised.server is server: raised.error = error @contextmanager -def raised_tool_error() -> Iterator[RaisedToolError]: - raised = RaisedToolError() +def raised_tool_error(server: Any) -> Iterator[RaisedToolError]: + raised = RaisedToolError(server) token = _current.set(raised) try: yield raised diff --git a/posthog/test/mcp/test_fastmcp_tool_errors.py b/posthog/test/mcp/test_fastmcp_tool_errors.py index 211a081bf..ae861d54e 100644 --- a/posthog/test/mcp/test_fastmcp_tool_errors.py +++ b/posthog/test/mcp/test_fastmcp_tool_errors.py @@ -28,6 +28,13 @@ def _tool_error(): raise ToolError("bad input") +def _tool_error_worded_like_fastmcp(): + try: + _upstream() + except ConnectionError as exc: + raise ToolError("Error calling tool: lookup failed") from exc + + @pytest.mark.parametrize( "raise_in_tool, mask_error_details, error_type, message, chain", [ @@ -46,6 +53,13 @@ def _tool_error(): ["RuntimeError", "ConnectionError"], ), (_tool_error, False, "ToolError", "bad input", ["ToolError"]), + ( + _tool_error_worded_like_fastmcp, + False, + "ToolError", + "Error calling tool: lookup failed", + ["ToolError", "ConnectionError"], + ), ], ) async def test_failed_call_carries_the_raised_exception( @@ -77,23 +91,37 @@ def fail() -> str: assert all(e["stacktrace"]["frames"] for e in raised) -async def test_exception_caught_by_the_tool_is_not_reported(): +@pytest.mark.parametrize( + "failing_server, instrumented", + [ + ("same", "wrapper"), + ("other", "wrapper"), + ("other", "low_level_only"), + ], +) +async def test_exception_caught_by_the_tool_is_not_reported( + failing_server, instrumented +): server = FastMCP("example-server") + other = FastMCP("other-server") + instrument(other, FakeClient(), MCPAnalyticsOptions()) + backend = server if failing_server == "same" else other - @server.tool + @backend.tool def inner() -> str: raise RuntimeError("inner failure") @server.tool async def outer() -> str: try: - await server.call_tool("inner", {}) + await backend.call_tool("inner", {}) except ToolError: pass return "recovered" sink = FakeClient() - instrument(server, sink, MCPAnalyticsOptions()) + target = server if instrumented == "wrapper" else server._mcp_server + instrument(target, sink, MCPAnalyticsOptions()) async with Client(server) as client: result = await client.call_tool("outer", {}, raise_on_error=False) From f5aea701a87c79385beaffe35f7f0c1af38964b0 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Wed, 11 Nov 2026 10:44:14 -0300 Subject: [PATCH 4/9] fix(mcp): keep FastMCP 2.x instrumented, and freeze the slot on exit Review on 81b3170 (PostHog Review, confirmed by ioannisj): - FastMCP 2.x has no public call_tool, so wrapping it raised AttributeError and instrument() fell back to no-op analytics: no MCP events at all on a supported stack. 2.x dispatches tools/call through _call_tool_middleware, so that is the seam wrapped there. 2.x now also reports the raised exception. - A background task holding the slot could settle it after the handler's dispatch returned. The slot now closes when the handler leaves it. - CI gains a v1-fastmcp2 leg (mcp 1.x + fastmcp 2.x), which would have caught the first point. Tested: pytest posthog/test/mcp passes on fastmcp 2.14.7 + mcp 1.30, fastmcp 3.2 + mcp 1.30, fastmcp 4.1.8 + mcp 2.3 (at 81b3170, 63 tests failed on fastmcp 2.14.7; at ae8f0bd all passed). The background race did not reproduce in a 0-40 tick sweep on 3.2 or 4.1.8, so it has no test; the close-on-exit makes it impossible by construction. --- .github/workflows/ci.yml | 10 +++++-- posthog/mcp/__init__.py | 4 +-- posthog/mcp/_raised_tool_error.py | 31 +++++++++++++------- posthog/test/mcp/test_fastmcp_tool_errors.py | 3 ++ 4 files changed, 32 insertions(+), 16 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ef0ff8a61..f23c51fdb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -156,7 +156,9 @@ jobs: # the lockfile's mcp 1.x (also exercised incidentally by the `tests` # matrix — this leg exists as an explicit, named signal); the v2 leg # (spec 2026-07-28) swaps in mcp>=2 and standalone FastMCP 4, which uses - # the v2 registry. The v2-bare leg runs mcp>=2 without FastMCP: importing + # the v2 registry. The v1-fastmcp2 leg keeps mcp 1.x and swaps in + # standalone FastMCP 2, which dispatches tools without a public + # call_tool. The v2-bare leg runs mcp>=2 without FastMCP: importing # FastMCP 4 patches camelCase properties onto mcp_types models for the # whole process, which hides reads that plain mcp 2.x rejects. # posthog/test/mcp/conftest.py splits collection by major. @@ -166,8 +168,10 @@ jobs: strategy: matrix: python-version: ['3.10', '3.14'] - mcp-major: ['v1', 'v2', 'v2-bare'] + mcp-major: ['v1', 'v1-fastmcp2', 'v2', 'v2-bare'] include: + - mcp-major: 'v1-fastmcp2' + mcp-packages: "'mcp>=1.28.1,<2' 'fastmcp>=2,<3'" - mcp-major: 'v2' mcp-packages: "'mcp>=2,<3' 'fastmcp>=4,<5'" - mcp-major: 'v2-bare' @@ -194,7 +198,7 @@ jobs: run: | UV_PROJECT_ENVIRONMENT=$pythonLocation uv sync --extra test - - name: Swap in MCP SDK v2 + - name: Swap in MCP packages if: matrix.mcp-major != 'v1' shell: bash run: | diff --git a/posthog/mcp/__init__.py b/posthog/mcp/__init__.py index f81e21908..9f9db0211 100644 --- a/posthog/mcp/__init__.py +++ b/posthog/mcp/__init__.py @@ -290,7 +290,7 @@ def instrument( from ._instrument_fastmcp import instrument_fastmcp from ._instrument_lowlevel import instrument_fastmcp_v2, instrument_low_level from ._instrument_v2 import instrument_lowlevel_v2, instrument_mcpserver_v2 - from ._raised_tool_error import wrap_call_tool + from ._raised_tool_error import wrap_tool_dispatch client = _resolve_client(posthog_client) if client is None: @@ -308,7 +308,7 @@ def instrument( ) if is_fastmcp_v2(server): - wrap_call_tool(server) + wrap_tool_dispatch(server) if uses_v2_handler_registry(key): data.standalone_fastmcp = weakref.ref(server) diff --git a/posthog/mcp/_raised_tool_error.py b/posthog/mcp/_raised_tool_error.py index ad6f85051..0d578a50b 100644 --- a/posthog/mcp/_raised_tool_error.py +++ b/posthog/mcp/_raised_tool_error.py @@ -1,8 +1,8 @@ """Recover the exception behind a standalone FastMCP ``isError`` tool result. -FastMCP's ``tools/call`` handler, the one we wrap, turns whatever its public -``call_tool`` raised into ``CallToolResult(isError=True)``, keeping only the -message text. Wrapping ``call_tool`` on the server instance sees the exception +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. """ @@ -15,11 +15,16 @@ _WRAPPED_FLAG = "__posthog_mcp_wrapped__" +# fastmcp 3+ dispatches tools/call through the public `call_tool`; 2.x has no +# `call_tool` and dispatches through `_call_tool_middleware`. +_DISPATCH_METHODS = ("call_tool", "_call_tool_middleware") + @dataclass class RaisedToolError: server: Any error: Optional[BaseException] = None + open: bool = True _current: ContextVar[Optional[RaisedToolError]] = ContextVar( @@ -27,12 +32,15 @@ class RaisedToolError: ) -def wrap_call_tool(server: Any) -> None: - original = server.call_tool +def wrap_tool_dispatch(server: Any) -> None: + method = next((m for m in _DISPATCH_METHODS if hasattr(server, m)), None) + if method is None: + return + original = getattr(server, method) if getattr(original, _WRAPPED_FLAG, False): return - async def call_tool(*args: Any, **kwargs: Any) -> Any: + async def dispatch(*args: Any, **kwargs: Any) -> Any: # A tool calling another tool nests these calls. The outermost call # settles last, so the slot ends holding the outcome the client sees. try: @@ -43,15 +51,15 @@ async def call_tool(*args: Any, **kwargs: Any) -> Any: _settle(server, None) return result - setattr(call_tool, _WRAPPED_FLAG, True) - server.call_tool = call_tool + setattr(dispatch, _WRAPPED_FLAG, True) + setattr(server, method, dispatch) def _settle(server: Any, error: Optional[BaseException]) -> None: - # Another server's call_tool, reached from inside a tool, must not write - # its outcome onto this call. + # 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. raised = _current.get() - if raised is not None and raised.server is server: + if raised is not None and raised.open and raised.server is server: raised.error = error @@ -62,4 +70,5 @@ def raised_tool_error(server: Any) -> Iterator[RaisedToolError]: try: yield raised finally: + raised.open = False _current.reset(token) diff --git a/posthog/test/mcp/test_fastmcp_tool_errors.py b/posthog/test/mcp/test_fastmcp_tool_errors.py index ae861d54e..e3dd76c30 100644 --- a/posthog/test/mcp/test_fastmcp_tool_errors.py +++ b/posthog/test/mcp/test_fastmcp_tool_errors.py @@ -102,6 +102,8 @@ def fail() -> str: async def test_exception_caught_by_the_tool_is_not_reported( failing_server, instrumented ): + if not hasattr(FastMCP, "call_tool"): + pytest.skip("fastmcp 2 has no public call_tool for a tool to call") server = FastMCP("example-server") other = FastMCP("other-server") instrument(other, FakeClient(), MCPAnalyticsOptions()) @@ -131,3 +133,4 @@ async def outer() -> str: [call] = events_named(sink, "$mcp_tool_call") assert call["properties"]["$mcp_is_error"] is False assert events_named(sink, "$exception") == [] + From 34fbaeb216a9dc564d3b478c9469f74b2bc84216 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Wed, 11 Nov 2026 10:44:14 -0300 Subject: [PATCH 5/9] fix(mcp): redact secrets in captured exception frame source lines The MCP sanitizer scrubbed each exception's value but not its frames' pre_context, context_line and post_context. A token written in the source around a raise reached $exception verbatim. This was already true on the official SDK path; #1223's fix made it reachable on standalone FastMCP too, where frames were never sent before. Those three keys now pass through the same sanitize_captured_value as the value. Tested: test_secrets_in_frame_source_lines_are_redacted fails without the sanitizer change and passes with it; full MCP suites pass on fastmcp 2.14.7, 3.2 and 4.1.8. --- posthog/mcp/_sanitization.py | 31 ++++++++++++++++++-- posthog/test/mcp/test_fastmcp_tool_errors.py | 24 +++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 426d2d29f..696dad024 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -680,14 +680,39 @@ def _sanitize_exception_values(error: Any) -> Any: return { **error, "$exception_list": [ - {**exception, "value": sanitize_captured_value(exception.get("value"))} - if isinstance(exception, dict) - else exception + _sanitize_exception(exception) if isinstance(exception, dict) else exception for exception in exception_list ], } +_FRAME_SOURCE_KEYS = ("pre_context", "context_line", "post_context") + + +def _sanitize_exception(exception: Dict[str, Any]) -> Dict[str, Any]: + sanitized = {**exception, "value": sanitize_captured_value(exception.get("value"))} + stacktrace = exception.get("stacktrace") + if isinstance(stacktrace, dict) and isinstance(stacktrace.get("frames"), list): + sanitized["stacktrace"] = { + **stacktrace, + "frames": [_sanitize_frame(frame) for frame in stacktrace["frames"]], + } + return sanitized + + +def _sanitize_frame(frame: Any) -> Any: + if not isinstance(frame, dict): + return frame + return { + **frame, + **{ + key: sanitize_captured_value(frame[key]) + for key in _FRAME_SOURCE_KEYS + if key in frame + }, + } + + def _sanitize_response(response: Any) -> Any: if response is None or not isinstance(response, (dict, list, str)): return sanitize_captured_value(response) diff --git a/posthog/test/mcp/test_fastmcp_tool_errors.py b/posthog/test/mcp/test_fastmcp_tool_errors.py index e3dd76c30..c23fc9bf7 100644 --- a/posthog/test/mcp/test_fastmcp_tool_errors.py +++ b/posthog/test/mcp/test_fastmcp_tool_errors.py @@ -134,3 +134,27 @@ async def outer() -> str: assert call["properties"]["$mcp_is_error"] is False assert events_named(sink, "$exception") == [] + +async def test_secrets_in_frame_source_lines_are_redacted(): + server = FastMCP("example-server") + + @server.tool + def fail() -> str: + dsn = "postgres://admin:hunter2@db.example.com/app" + raise RuntimeError(f"connect failed for {dsn.rsplit('@')[-1]}") + + sink = FakeClient() + instrument(server, sink, MCPAnalyticsOptions()) + + async with Client(server) as client: + await client.call_tool("fail", {}, raise_on_error=False) + await flush_background() + + [exception] = events_named(sink, "$exception") + frames = [ + frame + for entry in exception["properties"]["$exception_list"] + for frame in entry["stacktrace"]["frames"] + ] + assert "db.example.com/app" in repr(frames) + assert "hunter2" not in repr(frames) From 52fc4216becdfa410b5615d87ab3de4fc22278f8 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Wed, 11 Nov 2026 13:18:50 -0300 Subject: [PATCH 6/9] fix(mcp): don't let a lingering background task keep a tool's traceback alive PostHog Review on 34fbaeb: a background task started by a failing tool copies the request's context, and with it the slot holding the raised exception. Closing the slot left the exception attached, so its traceback and every frame's locals lived as long as the task. The slot the context can reach is now emptied on exit; the handler reads the outcome from a separate object only it holds. Tested: test_a_background_task_does_not_keep_the_failure_alive holds a weakref to a tool local while the tool's background task is still pending. It fails before and passes after on fastmcp 2.14.7, 3.2 and 4.1.8. The test turns logging off, because pytest's log capture keeps fastmcp's error records and their exc_info would retain the traceback by itself. Full MCP suites: 690 / 699 / 580 passed. --- posthog/mcp/_raised_tool_error.py | 23 +++++++--- posthog/test/mcp/test_fastmcp_tool_errors.py | 46 ++++++++++++++++++++ 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/posthog/mcp/_raised_tool_error.py b/posthog/mcp/_raised_tool_error.py index 0d578a50b..e694cf6fe 100644 --- a/posthog/mcp/_raised_tool_error.py +++ b/posthog/mcp/_raised_tool_error.py @@ -22,12 +22,17 @@ @dataclass class RaisedToolError: + error: Optional[BaseException] = None + + +@dataclass +class _Slot: server: Any error: Optional[BaseException] = None open: bool = True -_current: ContextVar[Optional[RaisedToolError]] = ContextVar( +_current: ContextVar[Optional[_Slot]] = ContextVar( "posthog_mcp_raised_tool_error", default=None ) @@ -58,17 +63,21 @@ async def dispatch(*args: Any, **kwargs: Any) -> Any: 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. - raised = _current.get() - if raised is not None and raised.open and raised.server is server: - raised.error = error + slot = _current.get() + if slot is not None and slot.open and slot.server is server: + slot.error = error @contextmanager def raised_tool_error(server: Any) -> Iterator[RaisedToolError]: - raised = RaisedToolError(server) - token = _current.set(raised) + slot = _Slot(server) + raised = RaisedToolError() + token = _current.set(slot) try: yield raised finally: - raised.open = False _current.reset(token) + # A background task started by the tool keeps a copy of this context, + # so the slot outlives the call; it must not keep the traceback alive. + raised.error, slot.error = slot.error, None + slot.open = False diff --git a/posthog/test/mcp/test_fastmcp_tool_errors.py b/posthog/test/mcp/test_fastmcp_tool_errors.py index c23fc9bf7..07a66d278 100644 --- a/posthog/test/mcp/test_fastmcp_tool_errors.py +++ b/posthog/test/mcp/test_fastmcp_tool_errors.py @@ -2,6 +2,11 @@ before the handler we wrap returns. The failure must still carry what was raised (posthog-python#1223).""" +import asyncio +import gc +import logging +import weakref + import pytest pytest.importorskip("fastmcp") @@ -158,3 +163,44 @@ def fail() -> str: ] assert "db.example.com/app" in repr(frames) assert "hunter2" not in repr(frames) + + +@pytest.fixture +def logging_off(): + # pytest's log capture keeps fastmcp's "Error calling tool" records, and + # their exc_info would keep the traceback alive on its own. + logging.disable(logging.CRITICAL) + yield + logging.disable(logging.NOTSET) + + +async def test_a_background_task_does_not_keep_the_failure_alive(logging_off): + server = FastMCP("example-server") + release = asyncio.Event() + lingering = [] + payload_refs = [] + + class Payload: + pass + + @server.tool + async def fail() -> str: + payload = Payload() + payload_refs.append(weakref.ref(payload)) + lingering.append(asyncio.get_running_loop().create_task(release.wait())) + raise RuntimeError("failed") + + sink = FakeClient() + instrument(server, sink, MCPAnalyticsOptions()) + + async with Client(server) as client: + await client.call_tool("fail", {}, raise_on_error=False) + await flush_background() + gc.collect() + + try: + assert not lingering[0].done() + assert payload_refs[0]() is None + finally: + release.set() + await lingering[0] From d8a7a552a93f6e490c546885d7f6dfa65f10b97e Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Wed, 11 Nov 2026 16:14:29 -0300 Subject: [PATCH 7/9] fix(mcp): redact sensitive JSON members inside captured strings Review on 52fc421 (PostHog Review, dustinbyrne): a failed FastMCP tool whose source has `credentials = {"password": "hunter2"}` sent the password in its frame's source context. The cause is wider than frames: the MCP Payload privacy spec requires redacting, inside any captured string, the value of a JSON member whose key is on the credential list, and posthog-python never implemented it. posthog-go and posthog-rs do. This ports their pattern into _redact_credentials, so it covers response text, error messages and frame source alike. Tested: posthog-go's TestSanitizeStringRedactsSensitiveJSONMembers table ported as a parameterized test, plus a source-line case; 13 redaction cases failed before, all 17 pass after. The frame test now also carries a JSON credential next to a benign member that must survive. Full MCP suites pass: 716 (fastmcp 3.2 + mcp 1.30), 597 (fastmcp 4.1.8 + mcp 2.3), 707 (fastmcp 2.14.7 + mcp 1.30). Ruff and mypy clean. --- posthog/mcp/_sanitization.py | 40 ++++++++++ posthog/test/mcp/test_fastmcp_tool_errors.py | 5 +- posthog/test/mcp/test_pipeline.py | 84 ++++++++++++++++++++ 3 files changed, 128 insertions(+), 1 deletion(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 696dad024..ca53860b7 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -10,6 +10,7 @@ from __future__ import annotations +import json import re from typing import Any, Dict, List, Optional, Set, Tuple from urllib.parse import SplitResult, parse_qsl, urlencode, urlsplit, urlunsplit @@ -32,6 +33,22 @@ re.IGNORECASE, ) +# A tool that returns structuredContent also returns it as JSON text, where a +# credential is a member inside a string rather than a dict key. A JSON string +# never holds a raw newline, so one cut off by truncation ends at the line or +# the text instead of swallowing prose up to the next quote. Values that nest +# objects, and JSON inside a JSON string, are out of scope. Same pattern as +# posthog-go's jsonMemberPattern and posthog-rs's JSON_MEMBER. +_JSON_STRING = r'"(?:[^"\\\n]|\\.)*(?:"|\\?(?m:$))' +_JSON_SCALAR = _JSON_STRING + r"|-?\d[\d.eE+-]*|true|false" +_JSON_MEMBER_PATTERN = re.compile( + r'("((?:[^"\\\n]|\\.)*)"\s*:\s*)(?:' + + _JSON_SCALAR + + r"|\[(?:\s*(?:" + + _JSON_SCALAR + + r"|null)\s*,?)*\s*(?:\]|$))" +) + # No leading `\b`: a word boundary needs a non-word character before the scheme, # so `resource_https://user:pw@host` (an `_` before the scheme) would match # nothing and stay unredacted. Without it the leftmost match wins — `foo.https://x` @@ -442,9 +459,32 @@ def _redact_credentials(value: str) -> str: entropy detector scans. Running the URL pass last loses nothing — it only redacts or percent-encodes, so it never exposes a credential the detectors could have matched.""" + value = _redact_json_members(value) return _redact_secret_tokens(_POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value)) +def _redact_json_members(value: str) -> str: + if '"' not in value: + return value + return _JSON_MEMBER_PATTERN.sub(_redacted_json_member, value) + + +def _redacted_json_member(member: "re.Match[str]") -> str: + if _should_redact_key(_decoded_json_key(member.group(2))): + return f'{member.group(1)}"{_REDACTED_VALUE}"' + return member.group(0) + + +def _decoded_json_key(raw: str) -> str: + if "\\" not in raw: + return raw + try: + decoded = json.loads(f'"{raw}"') + except ValueError: + return raw + return decoded if isinstance(decoded, str) else raw + + def sanitize_free_text(value: Any) -> Any: """Sanitize agent-narrated free text (the intent, the send_feedback fields): the binary gate, the credential passes, structured PII, then URLs. diff --git a/posthog/test/mcp/test_fastmcp_tool_errors.py b/posthog/test/mcp/test_fastmcp_tool_errors.py index 07a66d278..c1f1100cd 100644 --- a/posthog/test/mcp/test_fastmcp_tool_errors.py +++ b/posthog/test/mcp/test_fastmcp_tool_errors.py @@ -146,7 +146,8 @@ async def test_secrets_in_frame_source_lines_are_redacted(): @server.tool def fail() -> str: dsn = "postgres://admin:hunter2@db.example.com/app" - raise RuntimeError(f"connect failed for {dsn.rsplit('@')[-1]}") + credentials = {"password": "swordfish", "account": "reader"} + raise RuntimeError(f"connect failed for {credentials['account']}@{dsn[-6:]}") sink = FakeClient() instrument(server, sink, MCPAnalyticsOptions()) @@ -162,7 +163,9 @@ def fail() -> str: for frame in entry["stacktrace"]["frames"] ] assert "db.example.com/app" in repr(frames) + assert '"account": "reader"' in repr(frames) assert "hunter2" not in repr(frames) + assert "swordfish" not in repr(frames) @pytest.fixture diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index ea7457023..a87cc3078 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -73,6 +73,90 @@ def test_sanitize_redacts_sensitive_keys(): assert out["safe"] == "keep" +@pytest.mark.parametrize( + "value, expected", + [ + pytest.param( + '{"user":"ada","password":"hunter2"}', + '{"user":"ada","password":"[redacted]"}', + id="string", + ), + pytest.param( + '{\n "Password" : "hun\\"ter2",\n "n": 1\n}', + '{\n "Password" : "[redacted]",\n "n": 1\n}', + id="escaped quote and spacing", + ), + pytest.param('{"api_key":12345}', '{"api_key":"[redacted]"}', id="number"), + pytest.param( + '{"access-token":true}', '{"access-token":"[redacted]"}', id="boolean" + ), + pytest.param( + '{"refresh_token":"abcdefgh', + '{"refresh_token":"[redacted]"', + id="cut off at the end of the text", + ), + pytest.param( + '{"private_key":"abc\\', + '{"private_key":"[redacted]"', + id="cut off after an escape", + ), + pytest.param( + '{"Authorization":["Bearer x"]}', + '{"Authorization":"[redacted]"}', + id="array", + ), + pytest.param( + '{"cookie": [\n "a=1",\n "b=2"\n]}', + '{"cookie": "[redacted]"}', + id="indented array", + ), + pytest.param( + '{"set-cookie":["a=1","b=2', + '{"set-cookie":"[redacted]"', + id="array cut off", + ), + pytest.param( + 'retry failed: "token": "abc"', + 'retry failed: "token": "[redacted]"', + id="inside prose", + ), + pytest.param( + '"token": "abc\nnext "line"', + '"token": "[redacted]"\nnext "line"', + id="unclosed in prose stops at the line", + ), + pytest.param( + '{"pass\\u0077ord":"hunter2"}', + '{"pass\\u0077ord":"[redacted]"}', + id="escaped key", + ), + pytest.param( + ' credentials = {"password": "hunter2", "account": "reader"}', + ' credentials = {"password": "[redacted]", "account": "reader"}', + id="source line", + ), + pytest.param( + '{"password_hint":"pet"}', + '{"password_hint":"pet"}', + id="key only contains a sensitive word", + ), + pytest.param('{"token":null}', '{"token":null}', id="null"), + pytest.param( + '{"secret":{"v":"x"}}', + '{"secret":{"v":"x"}}', + id="out of scope: nested object", + ), + pytest.param( + '{"body":"{\\"password\\":\\"x\\"}"}', + '{"body":"{\\"password\\":\\"x\\"}"}', + id="out of scope: JSON inside a JSON string", + ), + ], +) +def test_sanitize_redacts_sensitive_json_members_inside_strings(value, expected): + assert sanitize_captured_value(value) == expected + + def test_sanitize_redacts_large_base64(): blob = "A" * 11000 assert sanitize_captured_value(blob).startswith("[binary data redacted") From 9c1cb4f4b4934f4d0c8cf173ab8d71de17225000 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Wed, 11 Nov 2026 16:54:40 -0300 Subject: [PATCH 8/9] fix(mcp): keep JSON-member redaction linear on malformed arrays Review on d8a7a55 (PostHog Review, veria-ai): the array part of the JSON-member pattern made every comma optional. Go and Rust run that pattern on linear-time engines; Python's re backtracks, so a caller's `{"note":[` + 40 digits + `!` split the digits into array items in exponentially many ways and blocked the event loop. Every item after the first now needs its comma, with an optional trailing one so an array cut off mid-list still matches, which leaves each input one parse. Tested: test_json_member_redaction_runs_in_linear_time_on_malformed_arrays timed out at 5 s on two of three inputs before and passes after. Ten adversarial 100k-character shapes (open arrays, lone quotes, unclosed strings, escapes, number characters) each finish under 10 ms. All JSON-member cases still pass. Full MCP suites: 719 / 600 / 710 passed on fastmcp 3.2 / 4.1.8 / 2.14.7. --- posthog/mcp/_sanitization.py | 13 ++++++++----- posthog/test/mcp/test_pipeline.py | 13 +++++++++++++ 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index ca53860b7..1403cfdd4 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -41,12 +41,15 @@ # posthog-go's jsonMemberPattern and posthog-rs's JSON_MEMBER. _JSON_STRING = r'"(?:[^"\\\n]|\\.)*(?:"|\\?(?m:$))' _JSON_SCALAR = _JSON_STRING + r"|-?\d[\d.eE+-]*|true|false" +_JSON_ITEM = r"(?:" + _JSON_SCALAR + r"|null)\s*" +# Python's `re` backtracks, unlike the RE2 and Rust engines the Go and Rust +# SDKs run this on, so every item after the first needs its comma: with an +# optional one, a run of digits splits into items in exponentially many ways. +_JSON_ARRAY = ( + r"\[\s*(?:" + _JSON_ITEM + r"(?:,\s*" + _JSON_ITEM + r")*(?:,\s*)?)?(?:\]|$)" +) _JSON_MEMBER_PATTERN = re.compile( - r'("((?:[^"\\\n]|\\.)*)"\s*:\s*)(?:' - + _JSON_SCALAR - + r"|\[(?:\s*(?:" - + _JSON_SCALAR - + r"|null)\s*,?)*\s*(?:\]|$))" + r'("((?:[^"\\\n]|\\.)*)"\s*:\s*)(?:' + _JSON_SCALAR + "|" + _JSON_ARRAY + ")" ) # No leading `\b`: a word boundary needs a non-word character before the scheme, diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index a87cc3078..d151cb86f 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -157,6 +157,19 @@ def test_sanitize_redacts_sensitive_json_members_inside_strings(value, expected) assert sanitize_captured_value(value) == expected +@pytest.mark.timeout(5) +@pytest.mark.parametrize( + "value", + [ + pytest.param('{"note":[' + "1" * 40 + "!", id="digits"), + pytest.param('{"note":[' + "1 " * 40 + "!", id="spaced digits"), + pytest.param('{"note":[' + "1," * 20_000 + "!", id="long array"), + ], +) +def test_json_member_redaction_runs_in_linear_time_on_malformed_arrays(value): + assert sanitize_captured_value(value) == value + + def test_sanitize_redacts_large_base64(): blob = "A" * 11000 assert sanitize_captured_value(blob).startswith("[binary data redacted") From f9832ccf34b33d4e5a6feb20d3e866b08afc825d Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 12 Nov 2026 10:47:37 -0300 Subject: [PATCH 9/9] fix(mcp): scan JSON members once instead of retrying the regex at every quote Review on 9c1cb4f (PostHog Review, veria-ai): with no member to match, the JSON-member regex restarted at every quote and rescanned the rest of the text, so 16,000 escaped quote pairs took about 3 s and 64,000 took 49 s on the server's event loop. This is the second performance finding in that regex: Go and Rust run it on linear-time engines, and Python's re does not. It is replaced by a scanner that tries the same key starts but caches where a string body ends and what follows each closing quote, so overlapping candidates share their work. Tested: the linear-time test gains escaped-quote and JSON-encoded-quote inputs at 32,000 pairs, which timed out at 5 s before and pass after; ten adversarial 100k-200k-character shapes each finish under 60 ms. A differential test keeps the reference pattern as an oracle and checks the scanner against it on 30,000 random texts (a one-off run of 200,000 longer ones found no mismatch). Full MCP suites: 722 / 603 / 713 passed on fastmcp 3.2 / 4.1.8 / 2.14.7; ruff and mypy clean. --- posthog/mcp/_sanitization.py | 154 +++++++++++++++++++++++++----- posthog/test/mcp/test_pipeline.py | 41 +++++++- 2 files changed, 171 insertions(+), 24 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 1403cfdd4..12b442245 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -33,24 +33,9 @@ re.IGNORECASE, ) -# A tool that returns structuredContent also returns it as JSON text, where a -# credential is a member inside a string rather than a dict key. A JSON string -# never holds a raw newline, so one cut off by truncation ends at the line or -# the text instead of swallowing prose up to the next quote. Values that nest -# objects, and JSON inside a JSON string, are out of scope. Same pattern as -# posthog-go's jsonMemberPattern and posthog-rs's JSON_MEMBER. -_JSON_STRING = r'"(?:[^"\\\n]|\\.)*(?:"|\\?(?m:$))' -_JSON_SCALAR = _JSON_STRING + r"|-?\d[\d.eE+-]*|true|false" -_JSON_ITEM = r"(?:" + _JSON_SCALAR + r"|null)\s*" -# Python's `re` backtracks, unlike the RE2 and Rust engines the Go and Rust -# SDKs run this on, so every item after the first needs its comma: with an -# optional one, a run of digits splits into items in exponentially many ways. -_JSON_ARRAY = ( - r"\[\s*(?:" + _JSON_ITEM + r"(?:,\s*" + _JSON_ITEM + r")*(?:,\s*)?)?(?:\]|$)" -) -_JSON_MEMBER_PATTERN = re.compile( - r'("((?:[^"\\\n]|\\.)*)"\s*:\s*)(?:' + _JSON_SCALAR + "|" + _JSON_ARRAY + ")" -) +_JSON_STRING_STOP = re.compile(r'["\\\n]') +_JSON_NUMBER = re.compile(r"-?\d[\d.eE+-]*") +_NON_SPACE = re.compile(r"\S") # No leading `\b`: a word boundary needs a non-word character before the scheme, # so `resource_https://user:pw@host` (an `_` before the scheme) would match @@ -469,13 +454,136 @@ def _redact_credentials(value: str) -> str: def _redact_json_members(value: str) -> str: if '"' not in value: return value - return _JSON_MEMBER_PATTERN.sub(_redacted_json_member, value) + return _JsonMemberScan(value).redacted() + +class _JsonMemberScan: + """Redacts the members posthog-go's jsonMemberPattern and posthog-rs's + JSON_MEMBER match: `"key": value`, where the value is a string, number, + boolean, or flat array of those (with every item after the first needing its + comma). A tool that returns structuredContent also returns it as JSON text, + where a credential is a member inside a string rather than a dict key. -def _redacted_json_member(member: "re.Match[str]") -> str: - if _should_redact_key(_decoded_json_key(member.group(2))): - return f'{member.group(1)}"{_REDACTED_VALUE}"' - return member.group(0) + Like those patterns, any quote may open a key, and the scan resumes after + each member. They run on linear-time engines; Python's `re` backtracks and + would rescan the rest of the text from every quote, so this caches where a + string body ends and what follows each closing quote. + + A JSON string never holds a raw newline, so one cut off by truncation ends + at the line or the text instead of swallowing prose up to the next quote. + Values that nest objects, and JSON inside a JSON string, are out of scope. + """ + + def __init__(self, text: str) -> None: + self.text = text + self.body_ends: Dict[int, int] = {} + self.members: Dict[int, Optional[Tuple[int, int]]] = {} + + def redacted(self) -> str: + text = self.text + parts: List[str] = [] + last = 0 + quote = text.find('"') + while quote != -1: + key_end = self.body_end(quote + 1) + member = ( + self.member_after(key_end + 1) + if key_end < len(text) and text[key_end] == '"' + else None + ) + if member is None: + quote = text.find('"', quote + 1) + continue + value_start, value_end = member + if _should_redact_key(_decoded_json_key(text[quote + 1 : key_end])): + parts.append(text[last:value_start]) + parts.append(f'"{_REDACTED_VALUE}"') + last = value_end + quote = text.find('"', value_end) + parts.append(text[last:]) + return "".join(parts) + + def body_end(self, start: int) -> int: + """Where a string body starting at `start` stops: its closing quote, a + newline, a backslash with nothing left to escape, or the end.""" + text = self.text + escapes = [] + pos = start + while True: + stop = _JSON_STRING_STOP.search(text, pos) + end = stop.start() if stop else len(text) + if end in self.body_ends: + end = self.body_ends[end] + break + if text[end : end + 1] != "\\" or text[end + 1 : end + 2] in ("", "\n"): + break + escapes.append(end) + pos = end + 2 + for escape in escapes: + self.body_ends[escape] = end + return end + + def member_after(self, pos: int) -> Optional[Tuple[int, int]]: + """Where the value after a key that closed just before `pos` starts and + ends, or None when no `: value` follows. Every quote inside a run of + escaped quotes closes on the same key, so this is computed once.""" + if pos not in self.members: + colon = self.skip_space(pos) + member = None + if self.text[colon : colon + 1] == ":": + value_start = self.skip_space(colon + 1) + value_end = self.value_end(value_start) + if value_end is not None: + member = (value_start, value_end) + self.members[pos] = member + return self.members[pos] + + def value_end(self, pos: int) -> Optional[int]: + if self.text[pos : pos + 1] == "[": + return self.array_end(pos + 1) + return self.scalar_end(pos) + + def scalar_end(self, pos: int) -> Optional[int]: + text = self.text + if text[pos : pos + 1] == '"': + end = self.body_end(pos + 1) + return end + 1 if text[end : end + 1] in ('"', "\\") else end + number = _JSON_NUMBER.match(text, pos) + if number: + return number.end() + for literal in ("true", "false"): + if text.startswith(literal, pos): + return pos + len(literal) + return None + + def item_end(self, pos: int) -> Optional[int]: + end = self.scalar_end(pos) + if end is None and self.text.startswith("null", pos): + end = pos + 4 + return None if end is None else self.skip_space(end) + + def array_end(self, pos: int) -> Optional[int]: + text = self.text + pos = self.skip_space(pos) + item = self.item_end(pos) + if item is not None: + pos = item + while text[pos : pos + 1] == ",": + after_comma = self.skip_space(pos + 1) + item = self.item_end(after_comma) + if item is None: + pos = after_comma + break + pos = item + if text[pos : pos + 1] == "]": + return pos + 1 + if pos == len(text) or (pos == len(text) - 1 and text[pos] == "\n"): + return pos + return None + + def skip_space(self, pos: int) -> int: + non_space = _NON_SPACE.search(self.text, pos) + return non_space.start() if non_space else len(self.text) def _decoded_json_key(raw: str) -> str: diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index d151cb86f..1e8728920 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -1,5 +1,8 @@ """Unit tests for the MCP analytics core pipeline (Milestone 1, no server).""" +import json +import re +import random from datetime import datetime, timezone import pytest @@ -9,6 +12,7 @@ PostHogMCPAnalyticsEvent, PostHogMCPAnalyticsProperty, ) +import posthog.mcp._sanitization as sanitization from posthog.mcp._event_types import MCPAnalyticsEventType from posthog.mcp._exceptions import capture_exception from posthog.mcp._ids import deterministic_prefixed_id, new_prefixed_id @@ -164,12 +168,47 @@ def test_sanitize_redacts_sensitive_json_members_inside_strings(value, expected) pytest.param('{"note":[' + "1" * 40 + "!", id="digits"), pytest.param('{"note":[' + "1 " * 40 + "!", id="spaced digits"), pytest.param('{"note":[' + "1," * 20_000 + "!", id="long array"), + pytest.param('\\"' * 32_000, id="escaped quotes"), + pytest.param(json.dumps('"' * 32_000), id="JSON-encoded quotes"), ], ) -def test_json_member_redaction_runs_in_linear_time_on_malformed_arrays(value): +def test_json_member_redaction_runs_in_linear_time_on_adversarial_text(value): assert sanitize_captured_value(value) == value +# The pattern posthog-go and posthog-rs match JSON members with (with every +# array item after the first needing its comma), run on Python's re. The SDK's +# scanner must find exactly its members, only without its quadratic retries. +_JSON_STRING_ORACLE = r'"(?:[^"\\\n]|\\.)*(?:"|\\?(?m:$))' +_JSON_SCALAR_ORACLE = _JSON_STRING_ORACLE + r"|-?\d[\d.eE+-]*|true|false" +_JSON_ITEM_ORACLE = r"(?:" + _JSON_SCALAR_ORACLE + r"|null)\s*" +_JSON_MEMBER_ORACLE = re.compile( + r'("((?:[^"\\\n]|\\.)*)"\s*:\s*)(?:' + + _JSON_SCALAR_ORACLE + + r"|\[\s*(?:" + + _JSON_ITEM_ORACLE + + r"(?:,\s*" + + _JSON_ITEM_ORACLE + + r")*(?:,\s*)?)?(?:\]|$))" +) + + +def test_json_member_scan_finds_what_the_reference_pattern_finds(monkeypatch): + monkeypatch.setattr(sanitization, "_should_redact_key", lambda key: True) + rng = random.Random(1223) + alphabet = ['"', '"', "\\", ":", ",", "[", "]", " ", "\n", "1", "-", "e", "x"] + words = ["true", "false", "null", '"k"', '"k": ', "[1, 2]", '\\"'] + texts = [ + "".join(rng.choice(alphabet + words) for _ in range(rng.randrange(16))) + for _ in range(30_000) + ] + for text in texts: + expected = _JSON_MEMBER_ORACLE.sub( + lambda member: member.group(1) + '"[redacted]"', text + ) + assert sanitization._redact_json_members(text) == expected, repr(text) + + def test_sanitize_redacts_large_base64(): blob = "A" * 11000 assert sanitize_captured_value(blob).startswith("[binary data redacted")