diff --git a/.sampo/changesets/mcp-error-properties.md b/.sampo/changesets/mcp-error-properties.md new file mode 100644 index 000000000..11cf149cb --- /dev/null +++ b/.sampo/changesets/mcp-error-properties.md @@ -0,0 +1,5 @@ +--- +posthog: minor +--- + +feat(mcp): emit `$mcp_error_message` and `$mcp_error_type` on failed MCP events. The reason a tool call failed previously lived only on the sibling `$exception` event, so PostHog's failures view — which reads the scalars off the primary event — showed empty error rows for every Python-backed MCP server, and switching off `enable_exception_autocapture` removed the reason entirely. Both values are read from the same `$exception_list` the sibling carries, so the two surfaces can never disagree, and the message inherits the existing 2048-character cap. `PostHogMCP.capture_tool_call()` and `capture_tools_list()` take a new optional `error_type` for custom dispatchers that want a coarse category (`"validation"`, `"timeout"`) instead of the thrown class name. Exception messages are also redacted before they leave — previously nothing sanitized the error payload, so the `$exception` sibling had been shipping them raw. Credential-looking words go through the SDK's own detector (entropy, known key formats, PEM markers), per word, so a message like `auth failed for sk-...` keeps its diagnostic text and loses only the key. Parity with `@posthog/mcp`, which sanitizes exception values the same way. diff --git a/posthog/mcp/_capture.py b/posthog/mcp/_capture.py index 26cf67fde..bba3a896f 100644 --- a/posthog/mcp/_capture.py +++ b/posthog/mcp/_capture.py @@ -63,6 +63,7 @@ def capture_event( "user_intent_source": event_input.get("user_intent_source"), "is_error": event_input.get("is_error"), "error": event_input.get("error"), + "error_type": event_input.get("error_type"), "conversation_id": event_input.get("conversation_id"), "properties": event_input.get("properties"), } diff --git a/posthog/mcp/_posthog_events.py b/posthog/mcp/_posthog_events.py index fd6f73638..309fa4040 100644 --- a/posthog/mcp/_posthog_events.py +++ b/posthog/mcp/_posthog_events.py @@ -139,6 +139,8 @@ def _add_common_properties(event: Event, properties: Dict[str, Any]) -> None: properties[_P.INTENT_SOURCE] = event["user_intent_source"] if event.get("is_error") is not None: properties[_P.IS_ERROR] = event["is_error"] + if event.get("is_error"): + _add_error_details(event, properties) if event.get("parameters") is not None: properties[_P.PARAMETERS] = event["parameters"] if event.get("response") is not None: @@ -149,6 +151,36 @@ def _add_common_properties(event: Event, properties: Dict[str, Any]) -> None: properties["$set"] = {**identify_actor_data} +def _add_error_details(event: Event, properties: Dict[str, Any]) -> None: + """Surface the failure reason on the primary event itself. + + Without these the dashboard has to join to the ``$exception`` sibling to + 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 + ``_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 + + # An explicit coarse category (e.g. "validation", "timeout") beats the + # thrown type; a custom dispatcher can pass one that means something to the + # product, where the class name rarely does. + error_type = event.get("error_type") or first.get("type") + if error_type: + properties[_P.ERROR_TYPE] = error_type + message = first.get("value") + if message: + properties[_P.ERROR_MESSAGE] = message + + def _add_custom_properties(event: Event, properties: Dict[str, Any]) -> None: custom = event.get("properties") if custom: diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 2224d5f27..fd547f418 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -39,7 +39,38 @@ def _should_redact_key(key: str) -> bool: def _sanitize_string(value: str) -> str: if len(value) >= _SIZE_GATE and _BASE64_PATTERN.match(value): return "[binary data redacted - not supported by PostHog MCP analytics]" - return _POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value) + return _redact_secret_tokens(_POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value)) + + +def _redact_secret_tokens(value: str) -> str: + """Redact credential-looking words, leaving the surrounding text intact. + + The PostHog-token pattern above only knows ``phc_``/``phx_``; a failure + message like ``auth failed for sk-proj-...`` carries someone else's key. + Rather than enumerate every vendor's format — an arms race that fails + quietly in both directions — this reuses the SDK's own detector + (``exception_utils._looks_like_secret``: entropy, known formats such as AWS + key ids, PEM markers), which the code-variables path already ships. + + Applied per whitespace-separated token, not to the whole string: redacting + an entire exception message would destroy the diagnostic value that + ``$mcp_error_message`` exists to provide, and ordinary prose is left alone + because no single word in it looks like a credential. + """ + if " " not in value: + return _REDACTED_VALUE if _is_secret(value) else value + return " ".join( + _REDACTED_VALUE if _is_secret(word) else word for word in value.split(" ") + ) + + +def _is_secret(word: str) -> bool: + try: + from posthog.exception_utils import _looks_like_secret + + return bool(word) and _looks_like_secret(word) + except Exception: # noqa: BLE001 - redaction must never break capture + return False def sanitize_captured_value(value: Any) -> Any: @@ -64,8 +95,8 @@ def sanitize_captured_value(value: Any) -> Any: def sanitize_event(event: Dict[str, Any]) -> Dict[str, Any]: - """Sanitize an event's response, parameters, and user_intent. Returns a new - shallow copy; does not mutate the input.""" + """Sanitize an event's response, parameters, user_intent and error. Returns + a new shallow copy; does not mutate the input.""" result = {**event} if result.get("response") is not None: @@ -79,9 +110,41 @@ def sanitize_event(event: Dict[str, Any]) -> Dict[str, Any]: if result.get("user_intent") is not None: result["user_intent"] = sanitize_captured_value(result["user_intent"]) + # An exception message is free text a server wrote, and it reaches PostHog + # on the $exception sibling and — since it is also surfaced as + # $mcp_error_message — on the primary event, so run it through the same + # sanitizer as every other captured value. + # + # That sanitizer redacts PostHog tokens and sensitive-looking keys; it is + # deliberately not a general credential scrubber, because enumerating every + # vendor's key format is an arms race that fails quietly in both directions. + # A host with strict requirements should gate free text in `before_send`. + # Same scope as @posthog/mcp's sanitizeCapturedValue. + if result.get("error") is not None: + result["error"] = _sanitize_exception_values(result["error"]) + return result +def _sanitize_exception_values(error: Any) -> Any: + """Redact the ``value`` of every frame in an ``$exception_list``, leaving + the rest of the error-tracking shape untouched.""" + if not isinstance(error, dict): + return error + exception_list = error.get("$exception_list") + if not isinstance(exception_list, list): + return error + return { + **error, + "$exception_list": [ + {**exception, "value": sanitize_captured_value(exception.get("value"))} + if isinstance(exception, dict) + else exception + for exception in exception_list + ], + } + + 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/mcp/_truncation.py b/posthog/mcp/_truncation.py index d0e6c2b2b..3f45e6416 100644 --- a/posthog/mcp/_truncation.py +++ b/posthog/mcp/_truncation.py @@ -41,6 +41,7 @@ ("server_version", _MAX_METADATA_LENGTH), ("client_name", _MAX_METADATA_LENGTH), ("client_version", _MAX_METADATA_LENGTH), + ("error_type", _MAX_METADATA_LENGTH), ) _NORMALIZED_FIELDS = ("parameters", "response", "identify_actor_data", "error") diff --git a/posthog/mcp/constants.py b/posthog/mcp/constants.py index eb2de16e2..3172eb1b6 100644 --- a/posthog/mcp/constants.py +++ b/posthog/mcp/constants.py @@ -60,6 +60,8 @@ class PostHogMCPAnalyticsProperty: PROTOCOL_VERSION = "$mcp_protocol_version" CONVERSATION_ID = "$mcp_conversation_id" DURATION_MS = "$mcp_duration_ms" + ERROR_MESSAGE = "$mcp_error_message" + ERROR_TYPE = "$mcp_error_type" IS_ERROR = "$mcp_is_error" INTENT = "$mcp_intent" INTENT_SOURCE = "$mcp_intent_source" diff --git a/posthog/mcp/posthog_mcp.py b/posthog/mcp/posthog_mcp.py index 487545e80..75f10b2f5 100644 --- a/posthog/mcp/posthog_mcp.py +++ b/posthog/mcp/posthog_mcp.py @@ -87,6 +87,7 @@ def capture_tool_call( duration_ms: Optional[float] = None, is_error: bool = False, error: Any = None, + error_type: Optional[str] = None, category: Optional[str] = None, tool_description: Optional[str] = None, protocol_version: Optional[str] = None, @@ -115,6 +116,7 @@ def capture_tool_call( event["response"] = response event["duration"] = duration_ms event["is_error"] = is_error + event["error_type"] = error_type _apply_intent(event, intent, intent_source) if is_error: event["error"] = capture_exception( @@ -165,6 +167,7 @@ def capture_tools_list( duration_ms: Optional[float] = None, is_error: bool = False, error: Any = None, + error_type: Optional[str] = None, protocol_version: Optional[str] = None, distinct_id: Optional[str] = None, session_id: Optional[str] = None, @@ -190,6 +193,7 @@ def capture_tools_list( event["response"] = response event["duration"] = duration_ms event["is_error"] = is_error + event["error_type"] = error_type if is_error: event["error"] = capture_exception( error if error is not None else "tools/list failed" diff --git a/posthog/mcp/types.py b/posthog/mcp/types.py index d1002d968..47dd945a1 100644 --- a/posthog/mcp/types.py +++ b/posthog/mcp/types.py @@ -40,7 +40,7 @@ # plain dict (constructed and read with ``.get()`` throughout) to mirror the TS # plain-object pipeline. Snake_case keys map to the ``$mcp_*`` wire keys in # ``posthog_events``. Known keys: client_name, client_version, conversation_id, -# duration, error, event_name, event_type, groups, id, identify_actor_data, +# duration, error, error_type, event_name, event_type, groups, id, identify_actor_data, # identify_actor_given_id, is_error, listed_tool_names, parameters, properties, # resource_name, response, server_name, server_version, session_id, timestamp, # tool_category, tool_description, user_intent, user_intent_source. diff --git a/posthog/test/mcp/test_error_properties.py b/posthog/test/mcp/test_error_properties.py new file mode 100644 index 000000000..9c978553e --- /dev/null +++ b/posthog/test/mcp/test_error_properties.py @@ -0,0 +1,209 @@ +"""``$mcp_error_message`` / ``$mcp_error_type`` on the primary event. + +Without these the failures view has nothing to show for a Python-backed +server: the reason a call failed lived only on the ``$exception`` sibling, +which ``enable_exception_autocapture=False`` switches off entirely. Parity with +``@posthog/mcp``, which reads both off the same ``$exception_list``. +Runs under both MCP SDK majors. +""" + +from posthog.mcp import PostHogMCP +from posthog.mcp.constants import PostHogMCPAnalyticsProperty as P +from posthog.test.mcp._helpers import ( + events_named as _events, + flush_background as _flush, +) + + +def make_client(**kwargs): + client = PostHogMCP("phc_test", **kwargs) + captured = [] + # Intercept the inherited Client.capture so nothing is sent over the network. + client.capture = lambda event, **kw: captured.append({"event": event, **kw}) + return client, captured + + +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")) + await _flush() + + props = _events(captured, "$mcp_tool_call")[0]["properties"] + assert props[P.IS_ERROR] is True + assert props[P.ERROR_MESSAGE] == "bad input" + assert props[P.ERROR_TYPE] == "ValueError" + + +async def test_explicit_error_type_beats_the_thrown_class(): + """A custom dispatcher can pass a coarse category that means something to + the product, where the exception class name usually doesn't.""" + client, captured = make_client() + client.capture_tool_call( + "add", is_error=True, error=ValueError("bad input"), error_type="validation" + ) + await _flush() + + props = _events(captured, "$mcp_tool_call")[0]["properties"] + assert props[P.ERROR_TYPE] == "validation" + assert props[P.ERROR_MESSAGE] == "bad input" + + +async def test_string_error_still_yields_both(): + client, captured = make_client() + client.capture_tool_call("add", is_error=True, error="upstream timed out") + await _flush() + + props = _events(captured, "$mcp_tool_call")[0]["properties"] + assert props[P.ERROR_MESSAGE] == "upstream timed out" + assert props[P.ERROR_TYPE] == "Error" + + +async def test_successful_call_carries_neither(): + client, captured = make_client() + client.capture_tool_call("add", response={"ok": True}) + await _flush() + + props = _events(captured, "$mcp_tool_call")[0]["properties"] + assert props[P.IS_ERROR] is False + assert P.ERROR_MESSAGE not in props + assert P.ERROR_TYPE not in props + + +async def test_the_exception_sibling_still_agrees(): + """Both surfaces read the same ``$exception_list``, so they can't disagree.""" + client, captured = make_client() + client.capture_tool_call("add", is_error=True, error=RuntimeError("boom")) + await _flush() + + call = _events(captured, "$mcp_tool_call")[0]["properties"] + first = _events(captured, "$exception")[0]["properties"]["$exception_list"][0] + assert call[P.ERROR_MESSAGE] == first["value"] + assert call[P.ERROR_TYPE] == first["type"] + + +async def test_message_survives_the_sibling_being_disabled(): + """The whole point: with autocapture off there is no ``$exception`` event, + so the primary event is the only place the failure reason can live.""" + client, captured = make_client(mcp_exception_autocapture=False) + client.capture_tool_call("add", is_error=True, error=ValueError("still here")) + await _flush() + + assert _events(captured, "$exception") == [] + props = _events(captured, "$mcp_tool_call")[0]["properties"] + assert props[P.ERROR_MESSAGE] == "still here" + assert props[P.ERROR_TYPE] == "ValueError" + + +async def test_long_messages_are_bounded(): + client, captured = make_client() + client.capture_tool_call("add", is_error=True, error=ValueError("x" * 5000)) + await _flush() + + props = _events(captured, "$mcp_tool_call")[0]["properties"] + message = props[P.ERROR_MESSAGE] + # Truncation runs before the mapping, so the scalar inherits the existing + # cap (2048 + the "..." marker) rather than adding an unbounded field. + assert len(message) < 5000 + assert message.endswith("...") + # ...and it is literally the same string the $exception sibling carries. + first = _events(captured, "$exception")[0]["properties"]["$exception_list"][0] + assert message == first["value"] + + +async def test_tools_list_failures_carry_them_too(): + client, captured = make_client() + client.capture_tools_list(is_error=True, error=RuntimeError("listing failed")) + await _flush() + + props = _events(captured, "$mcp_tools_list")[0]["properties"] + assert props[P.ERROR_MESSAGE] == "listing failed" + assert props[P.ERROR_TYPE] == "RuntimeError" + + +async def test_instrumented_server_failure_carries_them(): + """The same properties must land for a wrapped server, not just the manual + dispatcher — that is the path most customers are on.""" + 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("err-e2e") + + @server.tool() + def boom() -> str: + raise ValueError("explode") + + client = FakeClient() + instrument(server, client) + + try: + await server._tool_manager.call_tool("boom", {"context": "expected failure"}) + except Exception: + pass + await _flush() + + props = _events(client, "$mcp_tool_call")[0]["properties"] + assert props[P.IS_ERROR] is True + assert props[P.ERROR_TYPE] + assert "explode" in props[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 — + on the new scalar *and* on the `$exception` sibling, which had been shipping + it raw since before this property existed.""" + client, captured = make_client() + client.capture_tool_call( + "add", + is_error=True, + error=ValueError( + "auth failed for token phc_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + ), + ) + await _flush() + + message = _events(captured, "$mcp_tool_call")[0]["properties"][P.ERROR_MESSAGE] + sibling = _events(captured, "$exception")[0]["properties"]["$exception_list"][0] + + assert "phc_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" not in message + assert "phc_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" not in sibling["value"] + # the frame's other fields are untouched — this is redaction, not deletion + assert sibling["type"] == "ValueError" + + +async def test_a_non_posthog_credential_is_redacted_too(): + """The PostHog-token pattern only knows phc_/phx_. A failure message can + carry someone else's key, so credential-looking words go through the SDK's + own detector (entropy, known formats, PEM) — per word, so the diagnostic + text around them survives.""" + client, captured = make_client() + client.capture_tool_call( + "add", + is_error=True, + error=ValueError("auth failed for sk-proj-abc123XYZ789defGHI456jklMNO012pqr"), + ) + await _flush() + + message = _events(captured, "$mcp_tool_call")[0]["properties"][P.ERROR_MESSAGE] + assert "sk-proj-" not in message + assert message.startswith("auth failed for") # the useful part survives + + +async def test_ordinary_error_text_is_left_alone(): + """The redactor must not eat normal failure messages.""" + client, captured = make_client() + client.capture_tool_call( + "add", + is_error=True, + error=RuntimeError("revenue warehouse unreachable (period=q3)"), + ) + await _flush() + + props = _events(captured, "$mcp_tool_call")[0]["properties"] + assert props[P.ERROR_MESSAGE] == "revenue warehouse unreachable (period=q3)" diff --git a/references/public_api_snapshot.txt b/references/public_api_snapshot.txt index 10ad20201..707678c9d 100644 --- a/references/public_api_snapshot.txt +++ b/references/public_api_snapshot.txt @@ -714,6 +714,8 @@ attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.CLIENT_NAME = '$mcp_ attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.CLIENT_VERSION = '$mcp_client_version' attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.CONVERSATION_ID = '$mcp_conversation_id' attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.DURATION_MS = '$mcp_duration_ms' +attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.ERROR_MESSAGE = '$mcp_error_message' +attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.ERROR_TYPE = '$mcp_error_type' attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.INTENT = '$mcp_intent' attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.INTENT_SOURCE = '$mcp_intent_source' attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.IS_ERROR = '$mcp_is_error' @@ -1332,8 +1334,8 @@ method posthog.mcp.McpAnalytics.capture(event: str, properties: Optional[dict] = method posthog.mcp.McpAnalytics.flush() -> None method posthog.mcp.posthog_mcp.PostHogMCP.capture_initialize(*, client_name: Optional[str] = None, client_version: Optional[str] = None, protocol_version: Optional[str] = None, parameters: Any = None, response: Any = None, duration_ms: Optional[float] = None, distinct_id: Optional[str] = None, session_id: Optional[str] = None, set_properties: Optional[JsonRecord] = None, groups: Optional[Dict[str, str]] = None, properties: Optional[JsonRecord] = None, timestamp: Optional[datetime] = None) -> None method posthog.mcp.posthog_mcp.PostHogMCP.capture_missing_capability(*, context: Optional[str] = None, parameters: Any = None, protocol_version: Optional[str] = None, distinct_id: Optional[str] = None, session_id: Optional[str] = None, set_properties: Optional[JsonRecord] = None, groups: Optional[Dict[str, str]] = None, properties: Optional[JsonRecord] = None, timestamp: Optional[datetime] = None) -> None -method posthog.mcp.posthog_mcp.PostHogMCP.capture_tool_call(tool_name: str, *, intent: Optional[str] = None, intent_source: Optional[str] = None, parameters: Any = None, response: Any = None, duration_ms: Optional[float] = None, is_error: bool = False, error: Any = None, category: Optional[str] = None, tool_description: Optional[str] = None, protocol_version: Optional[str] = None, distinct_id: Optional[str] = None, session_id: Optional[str] = None, set_properties: Optional[JsonRecord] = None, groups: Optional[Dict[str, str]] = None, properties: Optional[JsonRecord] = None, timestamp: Optional[datetime] = None) -> None -method posthog.mcp.posthog_mcp.PostHogMCP.capture_tools_list(*, tool_names: Optional[List[str]] = None, parameters: Any = None, response: Any = None, duration_ms: Optional[float] = None, is_error: bool = False, error: Any = None, protocol_version: Optional[str] = None, distinct_id: Optional[str] = None, session_id: Optional[str] = None, set_properties: Optional[JsonRecord] = None, groups: Optional[Dict[str, str]] = None, properties: Optional[JsonRecord] = None, timestamp: Optional[datetime] = None) -> None +method posthog.mcp.posthog_mcp.PostHogMCP.capture_tool_call(tool_name: str, *, intent: Optional[str] = None, intent_source: Optional[str] = None, parameters: Any = None, response: Any = None, duration_ms: Optional[float] = None, is_error: bool = False, error: Any = None, error_type: Optional[str] = None, category: Optional[str] = None, tool_description: Optional[str] = None, protocol_version: Optional[str] = None, distinct_id: Optional[str] = None, session_id: Optional[str] = None, set_properties: Optional[JsonRecord] = None, groups: Optional[Dict[str, str]] = None, properties: Optional[JsonRecord] = None, timestamp: Optional[datetime] = None) -> None +method posthog.mcp.posthog_mcp.PostHogMCP.capture_tools_list(*, tool_names: Optional[List[str]] = None, parameters: Any = None, response: Any = None, duration_ms: Optional[float] = None, is_error: bool = False, error: Any = None, error_type: Optional[str] = None, protocol_version: Optional[str] = None, distinct_id: Optional[str] = None, session_id: Optional[str] = None, set_properties: Optional[JsonRecord] = None, groups: Optional[Dict[str, str]] = None, properties: Optional[JsonRecord] = None, timestamp: Optional[datetime] = None) -> None method posthog.mcp.posthog_mcp.PostHogMCP.flush(timeout_seconds: Optional[float] = 10) -> None method posthog.mcp.posthog_mcp.PostHogMCP.prepare_tool_call(name: str, args: Optional[JsonRecord] = None) -> PreparedToolCall method posthog.mcp.posthog_mcp.PostHogMCP.prepare_tool_list(tools: List[Any], context: Union[bool, MCPAnalyticsContextOptions] = True, report_missing: bool = False) -> List[Any]