From 1a98c0358e725a76dc2109addaaa83d93c64517d Mon Sep 17 00:00:00 2001 From: Georgis Andonis Date: Thu, 10 Sep 2026 16:57:40 +0300 Subject: [PATCH 1/8] feat(mcp): capture agents feedback Add an opt-in collect_feedback option that injects a send_feedback virtual tool and captures every call as a $mcp_feedback event, porting PostHog/posthog-js#4870 to the Python MCP analytics SDK. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 --- .sampo/changesets/mcp-collect-feedback.md | 6 + posthog/mcp/README.md | 79 +++ posthog/mcp/__init__.py | 19 + posthog/mcp/_conversation_id.py | 14 +- posthog/mcp/_event_types.py | 1 + posthog/mcp/_instrument_fastmcp.py | 30 +- posthog/mcp/_instrument_lowlevel.py | 40 +- posthog/mcp/_instrument_v2.py | 60 ++ posthog/mcp/_instrumentation.py | 177 +++++- posthog/mcp/_internal.py | 5 + posthog/mcp/_posthog_events.py | 1 + posthog/mcp/constants.py | 9 + posthog/mcp/feedback.py | 324 ++++++++++ posthog/mcp/posthog_mcp.py | 121 +++- posthog/mcp/types.py | 93 ++- posthog/test/mcp/conftest.py | 1 + posthog/test/mcp/test_feedback.py | 681 ++++++++++++++++++++++ posthog/test/mcp/test_v2_lowlevel.py | 62 ++ posthog/test/mcp/test_v2_mcpserver.py | 50 ++ references/public_api_snapshot.txt | 48 +- 20 files changed, 1794 insertions(+), 27 deletions(-) create mode 100644 .sampo/changesets/mcp-collect-feedback.md create mode 100644 posthog/mcp/feedback.py create mode 100644 posthog/test/mcp/test_feedback.py diff --git a/.sampo/changesets/mcp-collect-feedback.md b/.sampo/changesets/mcp-collect-feedback.md new file mode 100644 index 000000000..a43dfb186 --- /dev/null +++ b/.sampo/changesets/mcp-collect-feedback.md @@ -0,0 +1,6 @@ +--- +pypi/posthog: minor +--- + +Add an opt-in `collect_feedback` option to MCP analytics. It injects a `send_feedback` virtual tool and captures every call as a `$mcp_feedback` event, so agents can report a missing capability, a tool problem, or praise. +The option supports a custom tool name and description, host-declared extra schema fields, and an `on_feedback` handler that routes reports to a real backend. `PostHogMCP` gains the same option plus `capture_feedback` for custom dispatchers. diff --git a/posthog/mcp/README.md b/posthog/mcp/README.md index a032629f7..af432d106 100644 --- a/posthog/mcp/README.md +++ b/posthog/mcp/README.md @@ -90,6 +90,85 @@ Model injection copies tool objects instead of changing their original schemas. Always advertise the returned list and pass the original application tool to `prepare_tool_call()`. Repeatedly preparing the original list preserves ownership. +## Collect agent feedback + +Feedback collection is off by default. Enable it to advertise a `send_feedback` +virtual tool. Agents use it to report a missing capability (the priority +category), a tool that failed or confused them, or praise: + +```python +from posthog.mcp import MCPAnalyticsOptions, instrument + +analytics = instrument( + server, + posthog, + MCPAnalyticsOptions(collect_feedback=True), +) +``` + +Each call emits one `$mcp_feedback` event (never a `$mcp_tool_call`) with +`$mcp_feedback_type`, `$mcp_feedback_summary`, `$mcp_feedback_details`, +`$mcp_feedback_friction_points`, `$mcp_feedback_suggested_improvement`, +`$mcp_feedback_tool`, `$mcp_feedback_sentiment`, and +`$mcp_feedback_task_completed`. The summary and details also map to +`$mcp_intent`. Free-text fields are sanitized, PII-redacted, and length-bounded; +the raw arguments are never captured. An invalid `feedback_type` falls back to +`other`. The agent receives an honest acknowledgement: the call records feedback +and adds no tools. + +The tool covers what `report_missing` covers (as feedback_type +`missing_capability`), so new integrations should enable only one of the two. + +If a real tool already uses the name, the SDK logs a warning, does not inject +the virtual tool, and never intercepts the real tool. + +Use the object form to rename the tool, declare host-specific fields, or route +reports to a real backend: + +```python +from posthog.mcp import CollectFeedbackOptions, MCPAnalyticsOptions + +options = MCPAnalyticsOptions( + collect_feedback=CollectFeedbackOptions( + extra_properties={ + "product_area": { + "type": "string", + "description": "The product the feedback is about.", + }, + }, + extra_required=["product_area"], + on_feedback=lambda report: feedback_backend.record(report), + ), +) +``` + +Declared extras merge into the advertised schema and are captured as +`$mcp_feedback_`. Arguments the agent invents are never captured; the +handler reads them from `report.raw`. A key that collides with a core field +raises at configuration time. `on_feedback` may be sync or async; a returned +non-blank string replaces the default reply, and a raised handler is logged and +falls back to it. The event is captured either way. + +For a custom dispatcher, use the same option on `PostHogMCP`: + +```python +from posthog.mcp import PostHogMCP, send_feedback_result + +posthog = PostHogMCP("phc_...", collect_feedback=True) + +# tools/list handler +tools = posthog.prepare_tool_list(server_tools, collect_feedback=True) + +# tools/call dispatcher +call = posthog.prepare_tool_call(tool_name, raw_args) +if call.is_feedback: + posthog.capture_feedback(report=call.feedback_report) # emits $mcp_feedback + return send_feedback_result() # replies to the agent and stops dispatch +``` + +`on_feedback` is ignored on this path — the dispatcher routes reports itself via +`call.feedback_report`. + ## Stateless / multi-pod servers A stateless MCP server issues no session id, so `$session_id` fragments across pods diff --git a/posthog/mcp/__init__.py b/posthog/mcp/__init__.py index 633459300..80a686b6e 100644 --- a/posthog/mcp/__init__.py +++ b/posthog/mcp/__init__.py @@ -69,9 +69,17 @@ get_mcp_session, ) from ._sink import McpEventSink +from .feedback import ( + SEND_FEEDBACK_TOOL_NAME, + get_feedback_tool_descriptor, + resolve_collect_feedback_options, + send_feedback_result, +) from .tools import get_more_tools_result from .types import ( CaptureEventData, + CollectFeedbackOptions, + FeedbackReport, MCPAnalyticsContextOptions, MCPAnalyticsModelOptions, MCPAnalyticsModelSource, @@ -91,8 +99,12 @@ "MCPAnalyticsModelSource", "UserIdentity", "CaptureEventData", + "CollectFeedbackOptions", + "FeedbackReport", "PreparedToolCall", "get_more_tools_result", + "send_feedback_result", + "SEND_FEEDBACK_TOOL_NAME", # Read HTTP headers inside identify / intent_fallback / # event_properties callbacks on either SDK major: the per-request context # arrives as extra["ctx"] and its shape differs between them. @@ -254,6 +266,13 @@ def instrument( ) _warn_if_unsupported_mcp_version() + # Fail fast on a `collect_feedback` config error (reserved extra key, + # undeclared extra_required) — before the try below, so it raises instead of + # degrading to the no-op handle and first surfacing at tools/list time. + feedback_options = resolve_collect_feedback_options(opts.collect_feedback) + if feedback_options is not None: + get_feedback_tool_descriptor(feedback_options) + key = _canonical_server(server) try: diff --git a/posthog/mcp/_conversation_id.py b/posthog/mcp/_conversation_id.py index 1f44f3c63..4602897ad 100644 --- a/posthog/mcp/_conversation_id.py +++ b/posthog/mcp/_conversation_id.py @@ -79,15 +79,21 @@ def resolve_conversation_id( args: Any, tool_name: Optional[str], missing_capability_tool_name: str, + feedback_tool_name: Optional[str] = None, ) -> Tuple[Optional[str], bool]: - """Return ``(conversation_id, minted)``. Disabled or get_more_tools → ``(None, False)``; - agent echoed a handle we could have minted → ``(value, False)``; anything - else (omitted, or a value the agent made up) → ``(new uuid, True)``. + """Return ``(conversation_id, minted)``. Disabled, get_more_tools, or + send_feedback → ``(None, False)``; agent echoed a handle we could have minted + → ``(value, False)``; anything else (omitted, or a value the agent made up) + → ``(new uuid, True)``. Lowercased on the way in: the shape test is case-insensitive but the hash behind ``$session_id`` is not, so an uppercased echo (some hosts normalise uuids) would land in a different session than the call that minted it.""" - if not enabled or tool_name == missing_capability_tool_name: + if ( + not enabled + or tool_name == missing_capability_tool_name + or (feedback_tool_name is not None and tool_name == feedback_tool_name) + ): return None, False supplied = extract_conversation_id(args) if supplied and _MINTED_CONVERSATION_ID.match(supplied): diff --git a/posthog/mcp/_event_types.py b/posthog/mcp/_event_types.py index 32e63576d..75778dcd8 100644 --- a/posthog/mcp/_event_types.py +++ b/posthog/mcp/_event_types.py @@ -15,6 +15,7 @@ class MCPAnalyticsEventType: IDENTIFY = "posthog:identify" CUSTOM = "posthog:custom" + MCP_FEEDBACK = "mcp:feedback" MCP_MISSING_CAPABILITY = "mcp:missing_capability" MCP_INITIALIZE = "mcp:initialize" MCP_PROMPTS_GET = "mcp:prompts/get" diff --git a/posthog/mcp/_instrument_fastmcp.py b/posthog/mcp/_instrument_fastmcp.py index f61f3c361..dedf2d946 100644 --- a/posthog/mcp/_instrument_fastmcp.py +++ b/posthog/mcp/_instrument_fastmcp.py @@ -30,9 +30,11 @@ from ._instrumentation import ( _to_jsonable, append_get_more_tools, + append_send_feedback, collect_listed_tools, extract_tools, mutate_tool_schema, + refresh_feedback_shadow, request_to_dict, resolve_session_and_client, start_tool_call_lifecycle, @@ -114,6 +116,12 @@ async def wrapped( mcp_types.TextContent(type="text", text=get_more_tools_result_text()) ] + if lifecycle.is_feedback and not _feedback_name_owned_by_real_tool( + server, name + ): + reply = await lifecycle.record_feedback() + return [mcp_types.TextContent(type="text", text=reply)] + # Strip each injected key independently. A tool can declare its own # `context` (kept) while `conversation_id` is still SDK-injected (stripped), # so coupling both to context-ownership leaked conversation_id into the tool. @@ -215,7 +223,12 @@ async def list_handler(req: Any) -> Any: # advertise and write, the SDK rejects the customer's own tool result. if req is None: result = await original(req) - _inject_tool_schemas(server, data, extract_tools(result)) + tools = extract_tools(result) + # Refresh the collision flag here too: this pass sees the real tool + # registry, so a real tool named like the feedback tool is detected + # before any client-facing listing. + refresh_feedback_shadow(data, tools) + _inject_tool_schemas(server, data, tools) return result client_name, client_version = _low_level_client_info(server) @@ -254,6 +267,7 @@ async def list_handler(req: Any) -> Any: tools = extract_tools(result) # Empty is computed before adding the virtual missing-capability tool. names, empty = collect_listed_tools(data, tools) + feedback_name = refresh_feedback_shadow(data, tools) _inject_tool_schemas(server, data, tools) @@ -263,6 +277,10 @@ async def list_handler(req: Any) -> Any: append_get_more_tools(result, missing_name, data) names.append(missing_name) + if feedback_name is not None: + append_send_feedback(result, data) + names.append(feedback_name) + await lifecycle.record_result( names=names, response=_to_jsonable(result), @@ -299,6 +317,16 @@ def _inject_prompt_back(result: Any, conversation_id: str) -> Any: return result +def _feedback_name_owned_by_real_tool(server: Any, name: str) -> bool: + """Live registry probe so a real tool by the feedback tool's name is never + shadowed even before the first listing refreshes the collision flag.""" + try: + tool_manager = getattr(server, "_tool_manager", None) + return tool_manager is not None and tool_manager.get_tool(name) is not None + except Exception: # noqa: BLE001 - unknown tool -> the name is not owned + return False + + def _tool_owns_param(server: Any, name: str, param: str) -> bool: """True when the tool's own function declares ``param`` — then it's a real tool argument we must neither inject nor strip (the agent's value belongs to the tool).""" diff --git a/posthog/mcp/_instrument_lowlevel.py b/posthog/mcp/_instrument_lowlevel.py index d16c698e0..42e1ea673 100644 --- a/posthog/mcp/_instrument_lowlevel.py +++ b/posthog/mcp/_instrument_lowlevel.py @@ -25,9 +25,11 @@ from ._instrumentation import ( _to_jsonable, append_get_more_tools, + append_send_feedback, collect_listed_tools, extract_tools, mutate_tool_schema, + refresh_feedback_shadow, request_to_dict, resolve_session_and_client, start_tool_call_lifecycle, @@ -123,6 +125,17 @@ async def handler(req: Any) -> Any: ) ) + if lifecycle.is_feedback and not await _feedback_name_owned_by_real_tool( + high_level, name + ): + reply = await lifecycle.record_feedback() + return mcp_types.ServerResult( + mcp_types.CallToolResult( + content=[mcp_types.TextContent(type="text", text=reply)], + isError=False, + ) + ) + # On raw low-level servers `context`/`conversation_id` are injected as # *optional* schema properties and left in place (a (name, arguments) # handler ignores extra keys). FastMCP 2.0 validates against the function @@ -247,9 +260,12 @@ async def handler(req: Any) -> Any: # `additionalProperties: false`. if req is None: result = await original(req) - _inject_tool_schemas( - data, extract_tools(result), context_required=context_required - ) + tools = extract_tools(result) + # Refresh the collision flag here too: this pass sees the real tool + # registry, so a real tool named like the feedback tool is detected + # before any client-facing listing. + refresh_feedback_shadow(data, tools) + _inject_tool_schemas(data, tools, context_required=context_required) return result client_name, client_version = _client_info(server) @@ -291,6 +307,7 @@ async def handler(req: Any) -> Any: # Zero advertised tools is treated as an errored tools/list before the # virtual missing-capability tool is appended. names, empty = collect_listed_tools(data, tools) + feedback_name = refresh_feedback_shadow(data, tools) _inject_tool_schemas(data, tools, context_required=context_required) @@ -300,6 +317,10 @@ async def handler(req: Any) -> Any: append_get_more_tools(result, missing_name, data) names.append(missing_name) + if feedback_name is not None: + append_send_feedback(result, data) + names.append(feedback_name) + await lifecycle.record_result( names=names, response=_to_jsonable(result), @@ -313,6 +334,19 @@ async def handler(req: Any) -> Any: handlers[mcp_types.ListToolsRequest] = handler +async def _feedback_name_owned_by_real_tool(high_level: Any, name: str) -> bool: + """Live registry probe on the standalone-fastmcp path, so a real tool by the + feedback tool's name is never shadowed even before the first listing refreshes + the collision flag. Raw low-level servers have no registry to probe; they rely + on the listing-derived flag alone.""" + if high_level is None: + return False + try: + return await high_level.get_tool(name) is not None + except Exception: # noqa: BLE001 - unknown tool -> the name is not owned + return False + + async def _tool_owned_injected_keys(high_level: Any, name: str) -> set: """Which of (``context``, ``conversation_id``) the jlowin FastMCP tool declares itself, read from its function signature. These are real tool arguments we must diff --git a/posthog/mcp/_instrument_v2.py b/posthog/mcp/_instrument_v2.py index c740ae273..96e17d31b 100644 --- a/posthog/mcp/_instrument_v2.py +++ b/posthog/mcp/_instrument_v2.py @@ -42,10 +42,12 @@ collect_listed_tools, mutate_tool_schema, params_to_request_dict, + refresh_feedback_shadow, resolve_session_and_client, start_tool_call_lifecycle, start_tools_list_lifecycle, ) +from .feedback import get_feedback_tool_descriptor, resolve_collect_feedback_options from ._internal import MCPAnalyticsData from ._model_parameters import ( can_inject_model_parameter, @@ -286,6 +288,14 @@ async def wrapped( ] ) + if lifecycle.is_feedback and not _feedback_name_owned_by_real_tool_v2( + server, name + ): + reply = await lifecycle.record_feedback() + return mcp_types.CallToolResult( + content=[mcp_types.TextContent(type="text", text=reply)] + ) + # v2 validates against the function signature and rejects unexpected # keys, so injected parameters are stripped before dispatch — but never # one the tool's own schema declares (that's a real argument). @@ -432,6 +442,14 @@ async def handler(ctx: Any, params: Any) -> Any: ] ) + # No registry to probe on a raw low-level server; interception relies on + # the listing-derived collision flag alone. + if lifecycle.is_feedback: + reply = await lifecycle.record_feedback() + return mcp_types.CallToolResult( + content=[mcp_types.TextContent(type="text", text=reply)] + ) + # Settle the shared session before the tool body runs, so an in-tool # `analytics.capture()` is attributed to this caller and not the last one. await lifecycle.prime_session() @@ -510,6 +528,7 @@ async def handler(ctx: Any, params: Any) -> Any: tools = list(getattr(result, "tools", []) or []) # Empty is computed before adding the virtual missing-capability tool. names, empty = collect_listed_tools(data, tools) + feedback_name = refresh_feedback_shadow(data, tools) for tool in tools: schema = getattr(tool, "input_schema", None) @@ -532,6 +551,10 @@ async def handler(ctx: Any, params: Any) -> Any: _append_get_more_tools_v2(result, missing_name, data) names.append(missing_name) + if feedback_name is not None: + _append_send_feedback_v2(result, data) + names.append(feedback_name) + await lifecycle.record_result( names=names, response=_to_jsonable(result), @@ -545,6 +568,43 @@ async def handler(ctx: Any, params: Any) -> Any: _replace_handler(server, _LIST_METHOD, handler, entry.params_type) +def _feedback_name_owned_by_real_tool_v2(high_level: Any, name: str) -> bool: + """Live registry probe so a real tool by the feedback tool's name is never + shadowed even before the first listing refreshes the collision flag.""" + try: + return high_level._tool_manager.get_tool(name) is not None + except Exception: # noqa: BLE001 - unknown tool -> the name is not owned + return False + + +def _append_send_feedback_v2(result: Any, data: MCPAnalyticsData) -> None: + """Append the send_feedback virtual tool to a v2 ListToolsResult. Callers gate + on :func:`refresh_feedback_shadow` returning a name.""" + options = resolve_collect_feedback_options(data.options.collect_feedback) + if options is None: + return + descriptor = get_feedback_tool_descriptor(options) + tool = mcp_types.Tool( + name=descriptor["name"], + description=descriptor["description"], + input_schema=descriptor["inputSchema"], + annotations=descriptor["annotations"], + ) + # `owns_context=True`: the tool carries its intent in its own summary / + # details arguments, so no `context` parameter is injected — but the + # capture_model pass still runs, so it advertises `llm_model` too. + mutate_tool_schema( + data, + tool, + schema_attribute="input_schema", + owns_context=True, + context_required=True, + ) + tools_list = getattr(result, "tools", None) + if isinstance(tools_list, list): + tools_list.append(tool) + + def _append_get_more_tools_v2(result: Any, name: str, data: MCPAnalyticsData) -> None: descriptor = build_report_missing_descriptor(name) tool = mcp_types.Tool( diff --git a/posthog/mcp/_instrumentation.py b/posthog/mcp/_instrumentation.py index 328fc597f..e197c495a 100644 --- a/posthog/mcp/_instrumentation.py +++ b/posthog/mcp/_instrumentation.py @@ -27,6 +27,15 @@ from ._conversation_id import add_conversation_id_to_schema, resolve_conversation_id from ._event_types import MCPAnalyticsEventType from ._exceptions import capture_exception +from .feedback import ( + build_feedback_event_properties, + build_feedback_intent, + get_feedback_tool_descriptor, + handle_feedback, + parse_feedback_report, + resolve_collect_feedback_options, + resolve_send_feedback_tool_name, +) from ._intent import resolve_tool_call_intent, set_event_intent from ._internal import MCPAnalyticsData, handle_identify, resolve_event_properties from ._model_parameters import ( @@ -43,6 +52,7 @@ from .session import resolve_session_id, resolve_session_id_with_source from .session_token import SessionTokenPayload, decode_session_id from .tools import GET_MORE_TOOLS_NAME, resolve_missing_capability_tool_name +from .types import CollectFeedbackOptions, FeedbackReport # Keep strong refs to in-flight capture tasks/futures and their lifecycle owners so # they aren't GC'd mid-flight and lifecycle drains can select only their own work. @@ -407,6 +417,8 @@ class ToolCallLifecycle: client_version: Optional[str] protocol_version: Optional[str] missing_name: str + feedback_options: Optional[CollectFeedbackOptions] + feedback_name: Optional[str] conversation_id: Optional[str] minted_conversation_id: bool @@ -414,6 +426,16 @@ class ToolCallLifecycle: def is_missing_capability(self) -> bool: return self.data.options.report_missing and self.name == self.missing_name + @property + def is_feedback(self) -> bool: + # Never intercept a name a real application tool owns (fail-open): the + # listing pass records the collision on `feedback_tool_shadowed`. + return ( + self.feedback_name is not None + and self.name == self.feedback_name + and not self.data.feedback_tool_shadowed + ) + async def prepare_session(self, conversation_id: Optional[str]) -> str: return await prepare_request( self.data, @@ -448,6 +470,26 @@ async def record_missing_capability(self) -> None: extra=self.extra, ) + async def record_feedback(self) -> str: + """Capture the ``$mcp_feedback`` event, then run the host's ``on_feedback`` + handler and return the reply text for the agent. The event is captured + whether or not the handler raises.""" + report = parse_feedback_report(self.arguments, self.feedback_options) + session_id = await self.prepare_session(None) + await record_feedback( + self.data, + session_id, + report=report, + tool_name=self.feedback_name or self.name, + arguments=self.arguments, + request_meta=self.request_meta, + client_name=self.client_name, + client_version=self.client_version, + protocol_version=self.protocol_version, + extra=self.extra, + ) + return await handle_feedback(report, self.feedback_options) + async def record_error(self, error: Any, duration_ms: float) -> None: # A freshly minted handle cannot anchor or be captured when dispatch # raised: no adapter had an opportunity to deliver it to the agent. @@ -509,8 +551,18 @@ def start_tool_call_lifecycle( ) -> ToolCallLifecycle: """Resolve adapter-independent policy for a tool call without dispatching it.""" missing_name = resolve_missing_capability_tool_name(data.options) + feedback_options = resolve_collect_feedback_options(data.options.collect_feedback) + feedback_name = ( + resolve_send_feedback_tool_name(feedback_options) + if feedback_options is not None + else None + ) conversation_id, minted = resolve_conversation_id( - data.options.enable_conversation_id, arguments, name, missing_name + data.options.enable_conversation_id, + arguments, + name, + missing_name, + feedback_name, ) return ToolCallLifecycle( data=data, @@ -526,6 +578,8 @@ def start_tool_call_lifecycle( client_version=client_version, protocol_version=protocol_version, missing_name=missing_name, + feedback_options=feedback_options, + feedback_name=feedback_name, conversation_id=conversation_id, minted_conversation_id=minted, ) @@ -631,6 +685,57 @@ def append_get_more_tools(result: Any, name: str, data: MCPAnalyticsData) -> Non tools_list.append(tool) +def refresh_feedback_shadow(data: MCPAnalyticsData, tools: list) -> Optional[str]: + """Refresh the collision flag from this listing's tools. Returns the resolved + feedback tool name when the virtual tool should be appended, ``None`` when the + feature is off or a real application tool owns the name (fail-open: the real + tool is advertised and dispatched untouched). Run before the schema-injection + pass so it reads the fresh flag.""" + options = resolve_collect_feedback_options(data.options.collect_feedback) + if options is None: + return None + name = resolve_send_feedback_tool_name(options) + shadowed = any(getattr(tool, "name", None) == name for tool in tools) + if shadowed: + log( + f'Warning: Cannot inject agent-feedback tool "{name}" because a real tool ' + "already uses that name. The real tool will not be intercepted." + ) + data.feedback_tool_shadowed = shadowed + return None if shadowed else name + + +def append_send_feedback(result: Any, data: MCPAnalyticsData) -> None: + """Append the send_feedback virtual tool to the real ListToolsResult.tools + list. Callers gate on :func:`refresh_feedback_shadow` returning a name.""" + import mcp.types as mcp_types + + options = resolve_collect_feedback_options(data.options.collect_feedback) + if options is None: + return + descriptor = get_feedback_tool_descriptor(options) + tool = mcp_types.Tool( + name=descriptor["name"], + description=descriptor["description"], + inputSchema=descriptor["inputSchema"], + annotations=descriptor["annotations"], + ) + root = getattr(result, "root", result) + tools_list = getattr(root, "tools", None) + if isinstance(tools_list, list): + # `owns_context=True`: the tool carries its intent in its own summary / + # details arguments, so no `context` parameter is injected — but the + # capture_model pass still runs, so it advertises `llm_model` too. + mutate_tool_schema( + data, + tool, + schema_attribute="inputSchema", + owns_context=True, + context_required=True, + ) + tools_list.append(tool) + + def read_tool_category(tool: Any) -> Optional[str]: """Read a tool's product category from its ``_meta.category``.""" meta = getattr(tool, "meta", None) @@ -654,6 +759,19 @@ def collect_listed_tools(data: MCPAnalyticsData, tools: list) -> tuple[List[str] return names, not tools +def _is_sdk_virtual_tool(data: MCPAnalyticsData, tool_name: Any) -> bool: + """Whether this name is one of the SDK's own virtual tools (``get_more_tools``, + ``send_feedback``) — those carry their intent in their own arguments, so they + never get ``context``/``conversation_id`` injected. A shadowed feedback name + belongs to a real application tool and keeps normal injection.""" + if tool_name == GET_MORE_TOOLS_NAME: + return True + options = resolve_collect_feedback_options(data.options.collect_feedback) + if options is None or data.feedback_tool_shadowed: + return False + return tool_name == resolve_send_feedback_tool_name(options) + + def mutate_tool_schema( data: MCPAnalyticsData, tool: Any, @@ -671,7 +789,7 @@ def mutate_tool_schema( schema = getattr(tool, schema_attribute, None) original_schema = schema if ( - tool.name != GET_MORE_TOOLS_NAME + not _is_sdk_virtual_tool(data, tool.name) and is_context_enabled(data.options.context) and not owns_context ): @@ -697,7 +815,7 @@ def mutate_tool_schema( not app_owns_model and schema_has_param(schema, "llm_model") ) if ( - tool.name != GET_MORE_TOOLS_NAME + not _is_sdk_virtual_tool(data, tool.name) and data.options.enable_conversation_id and not schema_has_param(schema, "conversation_id") ): @@ -868,6 +986,59 @@ async def record_missing_capability( log(f"record_missing_capability failed (event dropped): {err}") +async def record_feedback( + data: MCPAnalyticsData, + session_id: str, + *, + report: FeedbackReport, + tool_name: str, + arguments: Optional[Dict[str, Any]], + request_meta: Optional[Dict[str, Any]] = None, + client_name: Optional[str] = None, + client_version: Optional[str] = None, + protocol_version: Optional[str] = None, + extra: Optional[Dict[str, Any]] = None, +) -> None: + """Record a ``send_feedback`` call as ``$mcp_feedback``, with the report's + summary and details as ``$mcp_intent``. + + Deliberately no ``parameters``: the arguments are agent-narrated free text, + and the PII-redacted ``$mcp_feedback_*`` properties are the captured surface — + the raw arguments would bypass that redaction and record undeclared fields.""" + try: + request = build_tool_call_request(tool_name, arguments) + event: Dict[str, Any] = { + "event_type": MCPAnalyticsEventType.MCP_FEEDBACK, + "session_id": session_id, + "resource_name": tool_name, + "client_name": client_name, + "client_version": client_version, + "protocol_version": protocol_version, + } + intent = build_feedback_intent(report) + if intent: + event["user_intent"] = intent + event["user_intent_source"] = "context_parameter" + if is_capture_model_enabled(data.options.capture_model): + model, source = resolve_model( + request_meta, arguments, allow_self_reported=True + ) + if model: + event["llm_model"] = model + event["llm_model_source"] = source + # Merged by hand (not `_apply_event_properties`, which assigns) so the + # customer's event_properties callback can't clobber the feedback fields. + props = await resolve_event_properties(data, request, extra) + event["properties"] = { + **(props or {}), + **build_feedback_event_properties(report), + } + stamp_transport_identity(event, extra) + fire_and_forget(capture_event(data, event), data) + except Exception as err: # noqa: BLE001 - isolate analytics from the tool path + log(f"record_feedback failed (event dropped): {err}") + + async def record_tools_list( data: MCPAnalyticsData, session_id: str, diff --git a/posthog/mcp/_internal.py b/posthog/mcp/_internal.py index 34e547535..4b46d4765 100644 --- a/posthog/mcp/_internal.py +++ b/posthog/mcp/_internal.py @@ -66,6 +66,11 @@ class MCPAnalyticsData: # signature of a stateless server whose mint middleware never attached. Warned # a single time per server so the log isn't flooded on every request. warned_no_stateless_session: bool = False + # True when the last tools/list showed a real application tool using the + # feedback tool's name. Calls to that name then dispatch normally instead of + # being intercepted (fail-open), and the tool keeps its normal analytics + # schema injection. Refreshed at every listing pass. + feedback_tool_shadowed: bool = False last_activity: datetime = field(default_factory=lambda: datetime.now(timezone.utc)) identified_sessions: IdentityCache = field(default_factory=IdentityCache) tool_categories: Dict[str, str] = field(default_factory=dict) diff --git a/posthog/mcp/_posthog_events.py b/posthog/mcp/_posthog_events.py index 689746e84..6991c4a2f 100644 --- a/posthog/mcp/_posthog_events.py +++ b/posthog/mcp/_posthog_events.py @@ -21,6 +21,7 @@ _BUILT_IN_EVENT_NAME_BY_TYPE = { MCPAnalyticsEventType.CUSTOM: PostHogMCPAnalyticsEvent.CUSTOM, MCPAnalyticsEventType.IDENTIFY: PostHogMCPAnalyticsEvent.IDENTIFY, + MCPAnalyticsEventType.MCP_FEEDBACK: PostHogMCPAnalyticsEvent.FEEDBACK, MCPAnalyticsEventType.MCP_MISSING_CAPABILITY: PostHogMCPAnalyticsEvent.MISSING_CAPABILITY, MCPAnalyticsEventType.MCP_INITIALIZE: PostHogMCPAnalyticsEvent.INITIALIZE, MCPAnalyticsEventType.MCP_PROMPTS_GET: PostHogMCPAnalyticsEvent.PROMPT_GET, diff --git a/posthog/mcp/constants.py b/posthog/mcp/constants.py index bbf1a81b1..75dff21b8 100644 --- a/posthog/mcp/constants.py +++ b/posthog/mcp/constants.py @@ -49,6 +49,7 @@ class PostHogMCPAnalyticsEvent: CUSTOM = "$mcp_custom" EXCEPTION = "$exception" + FEEDBACK = "$mcp_feedback" IDENTIFY = "$identify" INITIALIZE = "$mcp_initialize" MISSING_CAPABILITY = "$mcp_missing_capability" @@ -72,6 +73,14 @@ class PostHogMCPAnalyticsProperty: DURATION_MS = "$mcp_duration_ms" ERROR_MESSAGE = "$mcp_error_message" ERROR_TYPE = "$mcp_error_type" + FEEDBACK_DETAILS = "$mcp_feedback_details" + FEEDBACK_FRICTION_POINTS = "$mcp_feedback_friction_points" + FEEDBACK_SENTIMENT = "$mcp_feedback_sentiment" + FEEDBACK_SUGGESTED_IMPROVEMENT = "$mcp_feedback_suggested_improvement" + FEEDBACK_SUMMARY = "$mcp_feedback_summary" + FEEDBACK_TASK_COMPLETED = "$mcp_feedback_task_completed" + FEEDBACK_TOOL = "$mcp_feedback_tool" + FEEDBACK_TYPE = "$mcp_feedback_type" IS_ERROR = "$mcp_is_error" INTENT = "$mcp_intent" INTENT_SOURCE = "$mcp_intent_source" diff --git a/posthog/mcp/feedback.py b/posthog/mcp/feedback.py new file mode 100644 index 000000000..2dc943de7 --- /dev/null +++ b/posthog/mcp/feedback.py @@ -0,0 +1,324 @@ +"""The ``send_feedback`` virtual tool: an opt-in tool advertised to agents so they +can send feedback about the server to its developers — a missing capability (the +priority category), a tool that failed or confused them, or praise. Calling it +emits ``$mcp_feedback`` (not ``$mcp_tool_call``).""" + +from __future__ import annotations + +import copy +import json +from typing import Any, Dict, Optional, Union + +from ._internal import _maybe_await +from ._sanitization import redact_pii, sanitize_captured_value +from .constants import PostHogMCPAnalyticsProperty +from .logger import log +from .types import CollectFeedbackOptions, FeedbackReport, JsonRecord + +__all__ = ["SEND_FEEDBACK_TOOL_NAME", "send_feedback_result"] + +SEND_FEEDBACK_TOOL_NAME = "send_feedback" + +_FEEDBACK_TYPES = ("missing_capability", "issue", "praise", "other") +_SENTIMENTS = ("positive", "neutral", "negative", "mixed") + +# Free-text fields are agent-narrated, like `$mcp_intent`; bound them the same way. +_MAX_FEEDBACK_TEXT_LENGTH = 2048 +_MAX_FEEDBACK_TOOL_NAME_LENGTH = 256 +_TRUNCATION_SUFFIX = "..." + +_DEFAULT_FEEDBACK_DESCRIPTION = ( + "Send feedback about this server to its developers. Most important: report a missing " + "capability whenever no available tool fits your task, even if you can work around it " + '(feedback_type "missing_capability"). Also welcome: a tool that failed or confused you, ' + "an unhelpful error, or something that worked well. This records the feedback; it does " + "not add or change tools. Do not include user PII or sensitive content." +) + +_SEND_FEEDBACK_RESULT_TEXT = ( + "Your feedback was recorded for the server's developers. No additional tools are " + "available - continue with the tools already listed." +) + +_CORE_FEEDBACK_SCHEMA_PROPERTIES: Dict[str, Dict[str, Any]] = { + "feedback_type": { + "type": "string", + "enum": list(_FEEDBACK_TYPES), + "description": ( + "What kind of feedback this is. Use 'missing_capability' when the tool you " + "needed does not exist in the tool list - nothing failed, the capability is " + "absent (this is the most valuable report; send it even if you found a " + "workaround). Use 'issue' when an existing tool behaved badly: it failed, " + "returned a confusing error, or its description or schema misled you. Use " + "'praise' when something worked notably well. Use 'other' for anything else." + ), + }, + "summary": { + "type": "string", + "description": ( + "One self-contained sentence. For 'missing_capability': the capability you " + "needed, e.g. 'No tool to delete multiple cohorts in one call.' For 'issue': " + "the tool and the problem, e.g. 'query-trends rejects relative date ranges " + "with an unclear error.'" + ), + }, + "details": { + "type": "string", + "description": ( + "Optional longer context: what you tried, exact parameter values, the error " + "text you saw, and any workaround you used. Omit when the summary says it all." + ), + }, + "friction_points": { + "type": "string", + "description": ( + "Optional: the specific moments that slowed you down, as short bullet-like " + "sentences, quoting exact tool names, parameters, or error text." + ), + }, + "suggested_improvement": { + "type": "string", + "description": ( + "Optional: the concrete change that would have helped, e.g. the tool to add, " + "the description to reword, or the error message to improve." + ), + }, + "tool_name": { + "type": "string", + "description": ( + "Optional: the existing tool this feedback is about (for 'issue' or 'praise'). " + "Leave empty for 'missing_capability' - the point is that no tool fits." + ), + }, + "sentiment": { + "type": "string", + "enum": list(_SENTIMENTS), + "description": "Optional: how the experience felt overall.", + }, + "task_completed": { + "type": "boolean", + "description": "Optional: whether you still completed the user's task despite the problem.", + }, +} + +# Extra-property names a host may not declare: the core fields themselves, the +# names whose `$mcp_feedback_` property would collide with a core property +# (`type` -> `$mcp_feedback_type`, `tool` -> `$mcp_feedback_tool`), and the +# SDK-injected analytics arguments, which are stripped before dispatch and so +# would never reach the report. +_RESERVED_EXTRA_PROPERTY_KEYS = frozenset(_CORE_FEEDBACK_SCHEMA_PROPERTIES) | { + "type", + "tool", + "context", + "conversation_id", + "llm_model", +} + + +def resolve_collect_feedback_options( + config: Union[bool, CollectFeedbackOptions, None], +) -> Optional[CollectFeedbackOptions]: + """``collect_feedback`` normalized to its object form; ``None`` when the + feature is off.""" + if not config: + return None + return CollectFeedbackOptions() if config is True else config + + +def resolve_send_feedback_tool_name(options: Optional[CollectFeedbackOptions]) -> str: + """The configured name of the virtual tool, falling back to the default. + Resolve through here everywhere (inject + detect) so a custom name can't drift.""" + name = options.tool_name if options is not None else None + return name or SEND_FEEDBACK_TOOL_NAME + + +def get_feedback_tool_descriptor( + options: Optional[CollectFeedbackOptions] = None, +) -> Dict[str, Any]: + """The advertised descriptor: the core feedback schema plus the host's declared + ``extra_properties`` (plain dict; adapters build the framework's Tool object + from it). Raises ``ValueError`` on a config error (a reserved extra key, or an + ``extra_required`` entry that was never declared) so a bad setup fails at + configuration time instead of silently corrupting the advertised schema.""" + options = options or CollectFeedbackOptions() + extra_properties = options.extra_properties or {} + extra_required = options.extra_required or [] + + for key in extra_properties: + if key in _RESERVED_EXTRA_PROPERTY_KEYS: + raise ValueError( + f'collect_feedback extra_properties key "{key}" collides with a core or ' + "SDK-reserved send_feedback field. Rename it." + ) + for key in extra_required: + if key not in extra_properties: + raise ValueError( + f'collect_feedback extra_required key "{key}" is not declared in extra_properties.' + ) + + # Deep copies: the host may reuse or mutate the fragments it handed us, and + # schema injection mutates the advertised descriptor. + properties: Dict[str, Any] = copy.deepcopy(_CORE_FEEDBACK_SCHEMA_PROPERTIES) + if extra_properties: + properties.update(copy.deepcopy(extra_properties)) + + return { + "name": resolve_send_feedback_tool_name(options), + "description": options.description or _DEFAULT_FEEDBACK_DESCRIPTION, + "inputSchema": { + "type": "object", + "properties": properties, + "required": ["feedback_type", "summary", *extra_required], + }, + "annotations": { + "title": "Send feedback", + "readOnlyHint": True, + # Interacts with an external entity: the report lands in analytics. + "openWorldHint": True, + # Only records the feedback, so repeat calls are harmless — and + # advertising it as idempotent makes agents more willing to call it + # proactively. + "idempotentHint": True, + "destructiveHint": False, + }, + } + + +def _read_string(value: Any) -> Optional[str]: + return value if isinstance(value, str) and value.strip() else None + + +def parse_feedback_report( + args: Optional[Dict[str, Any]], + options: Optional[CollectFeedbackOptions] = None, +) -> FeedbackReport: + """Parse the raw ``send_feedback`` arguments into a typed report. Never raises: + an invalid ``feedback_type`` falls back to ``other``, missing fields stay + ``None``, and only **declared** extras are lifted into ``extras`` — anything + the agent invented reaches the handler via ``raw`` and is never captured.""" + raw = args or {} + declared = (options.extra_properties if options is not None else None) or {} + extras = {key: raw[key] for key in declared if raw.get(key) is not None} + feedback_type = raw.get("feedback_type") + sentiment = raw.get("sentiment") + task_completed = raw.get("task_completed") + return FeedbackReport( + feedback_type=feedback_type if feedback_type in _FEEDBACK_TYPES else "other", + summary=_read_string(raw.get("summary")) or "", + sentiment=sentiment if sentiment in _SENTIMENTS else None, + friction_points=_read_string(raw.get("friction_points")), + suggested_improvement=_read_string(raw.get("suggested_improvement")), + details=_read_string(raw.get("details")), + tool_name=_read_string(raw.get("tool_name")), + task_completed=task_completed if isinstance(task_completed, bool) else None, + extras=extras, + raw=dict(raw), + ) + + +def build_feedback_intent(report: FeedbackReport) -> str: + """The report's free text, used as the event's ``$mcp_intent``.""" + return "\n\n".join(part for part in (report.summary, report.details) if part) + + +def _truncate_feedback_text(value: str, max_length: int) -> str: + return value[:max_length] + _TRUNCATION_SUFFIX if len(value) > max_length else value + + +def _capture_free_text(value: str) -> str: + """Agent-narrated free text can contain a secret the LLM read aloud or personal + data it narrated, so it gets the ``$mcp_intent`` treatment: sanitize, strip + structured PII, then bound the length. The event pipeline does not process + ``event["properties"]``, so this happens here.""" + return _truncate_feedback_text( + redact_pii(sanitize_captured_value(value)), _MAX_FEEDBACK_TEXT_LENGTH + ) + + +def _capture_extra_value(value: Any) -> Any: + """A declared extra is agent-supplied like the core free-text fields, so it + gets the same treatment. Non-scalars are JSON-stringified first so the + redaction sees the full text; unserializable values are dropped.""" + sanitized = sanitize_captured_value(value) + if isinstance(sanitized, str): + return _capture_free_text(sanitized) + if sanitized is None or isinstance(sanitized, (bool, int, float)): + return sanitized + try: + return _capture_free_text(json.dumps(sanitized)) + except Exception: # noqa: BLE001 - capture must never raise into the tool path + return None + + +def build_feedback_event_properties(report: FeedbackReport) -> JsonRecord: + """The ``$mcp_feedback_*`` event properties for one report, declared extras + included.""" + properties: JsonRecord = { + PostHogMCPAnalyticsProperty.FEEDBACK_TYPE: report.feedback_type + } + if report.summary: + properties[PostHogMCPAnalyticsProperty.FEEDBACK_SUMMARY] = _capture_free_text( + report.summary + ) + if report.sentiment: + properties[PostHogMCPAnalyticsProperty.FEEDBACK_SENTIMENT] = report.sentiment + if report.friction_points: + properties[PostHogMCPAnalyticsProperty.FEEDBACK_FRICTION_POINTS] = ( + _capture_free_text(report.friction_points) + ) + if report.suggested_improvement: + properties[PostHogMCPAnalyticsProperty.FEEDBACK_SUGGESTED_IMPROVEMENT] = ( + _capture_free_text(report.suggested_improvement) + ) + if report.details: + properties[PostHogMCPAnalyticsProperty.FEEDBACK_DETAILS] = _capture_free_text( + report.details + ) + if report.tool_name: + properties[PostHogMCPAnalyticsProperty.FEEDBACK_TOOL] = _truncate_feedback_text( + sanitize_captured_value(report.tool_name), _MAX_FEEDBACK_TOOL_NAME_LENGTH + ) + if report.task_completed is not None: + properties[PostHogMCPAnalyticsProperty.FEEDBACK_TASK_COMPLETED] = ( + report.task_completed + ) + for key, value in report.extras.items(): + captured = _capture_extra_value(value) + if captured is not None: + properties[f"$mcp_feedback_{key}"] = captured + return properties + + +def send_feedback_result() -> Dict[str, Any]: + """The canned acknowledgement returned to the agent after it calls + ``send_feedback``. Reply with this from a custom dispatcher; the + ``instrument()`` path returns it automatically, or the string your + ``on_feedback`` handler returned instead.""" + return {"content": [{"type": "text", "text": _SEND_FEEDBACK_RESULT_TEXT}]} + + +def send_feedback_result_text() -> str: + return _SEND_FEEDBACK_RESULT_TEXT + + +async def handle_feedback( + report: FeedbackReport, options: Optional[CollectFeedbackOptions] = None +) -> str: + """Run the host's ``on_feedback`` handler (when configured) and return the + reply text. A returned non-blank string replaces the default acknowledgement; + a raised handler is logged and falls back to it — feedback capture must never + break the agent's turn.""" + # Only the type: the summary is agent-narrated free text (possible PII, + # newlines for log forging, unbounded length) and does not belong in host logs. + log(f"Agent feedback reported ({report.feedback_type})") + if options is not None and options.on_feedback is not None: + try: + reply = await _maybe_await(options.on_feedback(report)) + if isinstance(reply, str) and reply.strip(): + return reply + except Exception as error: # noqa: BLE001 - never break the agent's turn + log( + "Warning: on_feedback handler threw; returning the default " + f"acknowledgement - {error}" + ) + return _SEND_FEEDBACK_RESULT_TEXT diff --git a/posthog/mcp/posthog_mcp.py b/posthog/mcp/posthog_mcp.py index 8aac5a8b8..2464413fc 100644 --- a/posthog/mcp/posthog_mcp.py +++ b/posthog/mcp/posthog_mcp.py @@ -35,8 +35,18 @@ resolve_model, ) from ._sink import McpCaptureOptions, McpEventSink +from .feedback import ( + build_feedback_event_properties, + build_feedback_intent, + get_feedback_tool_descriptor, + parse_feedback_report, + resolve_collect_feedback_options, + resolve_send_feedback_tool_name, +) from .tools import build_report_missing_descriptor from .types import ( + CollectFeedbackOptions, + FeedbackReport, JsonRecord, MCPAnalyticsContextOptions, MCPAnalyticsModelOptions, @@ -51,9 +61,9 @@ class PostHogMCP(Client): """A drop-in posthog ``Client`` with ``capture_tool_call`` / ``capture_initialize`` - / ``capture_tools_list`` / ``capture_missing_capability`` plus ``prepare_tool_list`` - and ``prepare_tool_call`` helpers. ``capture``, ``flush``, ``shutdown``, feature - flags, etc. all work unchanged.""" + / ``capture_tools_list`` / ``capture_missing_capability`` / ``capture_feedback`` + plus ``prepare_tool_list`` and ``prepare_tool_call`` helpers. ``capture``, + ``flush``, ``shutdown``, feature flags, etc. all work unchanged.""" def __init__( self, @@ -61,6 +71,7 @@ def __init__( missing_capability_tool_name: Optional[str] = None, mcp_exception_autocapture: bool = True, capture_model: Union[bool, MCPAnalyticsModelOptions] = False, + collect_feedback: Union[bool, CollectFeedbackOptions] = False, **kwargs: Any, ) -> None: super().__init__(api_key, **kwargs) @@ -69,6 +80,19 @@ def __init__( self._missing_capability_tool_name = ( missing_capability_tool_name or _GET_MORE_TOOLS_NAME ) + # `None` is the enable switch's off state: without it, prepare_tool_call + # must never claim a call named like the virtual tool — the host may have + # a real tool by that name, and flagging it would shadow the real handler. + # `on_feedback` is ignored on this path: the host dispatcher routes + # reports itself via PreparedToolCall.feedback_report. + self._collect_feedback = resolve_collect_feedback_options(collect_feedback) + self._feedback_tool_name = resolve_send_feedback_tool_name( + self._collect_feedback + ) + # Fail fast on a config error (reserved extra key, undeclared + # extra_required) instead of first surfacing it when a tools/list is served. + if self._collect_feedback is not None: + get_feedback_tool_descriptor(self._collect_feedback) # Whether a failed tool call fans out an `$exception` sibling event. Distinct # from the inherited Client.enable_exception_autocapture (global uncaught-error # hook); this mirrors instrument()'s enable_exception_autocapture, default on. @@ -268,6 +292,51 @@ def capture_missing_capability( _apply_model(event, llm_model, llm_model_source) self._emit(event) + def capture_feedback( + self, + *, + report: FeedbackReport, + llm_model: Optional[str] = None, + llm_model_source: Optional[MCPAnalyticsModelSource] = None, + protocol_version: Optional[str] = None, + distinct_id: Optional[str] = None, + session_id: Optional[str] = None, + client_user_agent: Optional[str] = None, + vendor_client: Optional[str] = None, + set_properties: Optional[JsonRecord] = None, + groups: Optional[Dict[str, str]] = None, + properties: Optional[JsonRecord] = None, + timestamp: Optional[datetime] = None, + ) -> None: + """Capture a ``send_feedback`` call as an agent-feedback report. Emits + ``$mcp_feedback`` with the report's ``$mcp_feedback_*`` properties and its + summary/details as ``$mcp_intent``. Reply to the agent with + ``send_feedback_result()`` (or a custom text) after routing the report to + your own feedback backend.""" + event = self._base_event( + MCPAnalyticsEventType.MCP_FEEDBACK, + distinct_id, + session_id, + set_properties, + groups, + properties, + timestamp, + client_user_agent, + vendor_client, + ) + event["resource_name"] = self._feedback_tool_name + event["protocol_version"] = protocol_version + # Deliberately no `parameters`: the arguments are agent-narrated free + # text, and the PII-redacted `$mcp_feedback_*` properties are the captured + # surface. Raw arguments would bypass that redaction. + event["properties"] = { + **build_feedback_event_properties(report), + **(properties or {}), + } + _apply_intent(event, build_feedback_intent(report), "context_parameter") + _apply_model(event, llm_model, llm_model_source) + self._emit(event) + # --- prepare helpers ----------------------------------------------------- def prepare_tool_list( @@ -275,12 +344,16 @@ def prepare_tool_list( tools: List[Any], context: Union[bool, MCPAnalyticsContextOptions] = True, report_missing: bool = False, + collect_feedback: bool = False, ) -> List[Any]: """Inject the ``context`` argument into every tool so agents state their intent (captured as ``$mcp_intent``), and optionally append the - ``get_more_tools`` virtual tool (``report_missing=True``). Returns a new - list; dict tools are copied, context injection mutates tool objects in - place, and model injection copies them to preserve field ownership.""" + ``get_more_tools`` virtual tool (``report_missing=True``) and the + ``send_feedback`` virtual tool (``collect_feedback=True``, which also + requires the constructor's ``collect_feedback`` option — the enable switch + that gates detection in :meth:`prepare_tool_call`). Returns a new list; + dict tools are copied, context injection mutates tool objects in place, + and model injection copies them to preserve field ownership.""" prepared = [] context_description = get_context_description(context) for tool in tools: @@ -297,6 +370,12 @@ def prepare_tool_list( prepared.append( build_report_missing_descriptor(self._missing_capability_tool_name) ) + if ( + collect_feedback + and self._collect_feedback is not None + and not any(_tool_name(t) == self._feedback_tool_name for t in prepared) + ): + prepared.append(get_feedback_tool_descriptor(self._collect_feedback)) prepared = self._inject_models(prepared) return prepared @@ -309,9 +388,11 @@ def prepare_tool_call( original_tool: Any = None, ) -> PreparedToolCall: """Pull the agent's intent off the injected ``context`` argument, strip - ``context`` from the arguments, and flag the ``get_more_tools`` virtual tool. - When model capture is enabled, resolve its value and source and strip - the SDK-owned ``llm_model`` argument before dispatch.""" + ``context`` from the arguments, and flag the ``get_more_tools`` and + ``send_feedback`` virtual tools (the latter only with the constructor's + ``collect_feedback`` opt-in, so a real tool by that name is never + shadowed). When model capture is enabled, resolve its value and source and + strip the SDK-owned ``llm_model`` argument before dispatch.""" raw_context = (args or {}).get("context") intent = ( raw_context.strip() @@ -334,6 +415,9 @@ def prepare_tool_call( prepared_args = _strip_context(args) if analytics_owns_model: prepared_args = _strip_model(prepared_args) + is_feedback = ( + self._collect_feedback is not None and name == self._feedback_tool_name + ) return PreparedToolCall( args=prepared_args, intent=intent, @@ -341,6 +425,12 @@ def prepare_tool_call( llm_model=llm_model, llm_model_source=llm_model_source, is_missing_capability=name == self._missing_capability_tool_name, + is_feedback=is_feedback, + feedback_report=( + parse_feedback_report(args, self._collect_feedback) + if is_feedback + else None + ), ) # --- internals ----------------------------------------------------------- @@ -385,10 +475,19 @@ def _emit(self, event: Dict[str, Any]) -> None: # flush()/shutdown() able to drain without blocking their own event loop's tasks. fire_and_forget(self._mcp_sink.capture(event, options), self, background=True) + def _is_virtual_tool_name(self, name: Any) -> bool: + """The SDK's own virtual tools carry their intent in their own arguments, + so they never get the ``context`` parameter injected. The feedback name + only counts with the constructor opt-in — without it a real tool by that + name is an ordinary tool.""" + if name == self._missing_capability_tool_name: + return True + return self._collect_feedback is not None and name == self._feedback_tool_name + def _inject_context(self, tool: Any, description: Optional[str]) -> Any: if isinstance(tool, dict): name = tool.get("name", "unknown") - if name == self._missing_capability_tool_name: + if self._is_virtual_tool_name(name): return tool new_schema = add_context_parameter_to_schema( tool.get("inputSchema"), name, description @@ -396,7 +495,7 @@ def _inject_context(self, tool: Any, description: Optional[str]) -> Any: return {**tool, "inputSchema": new_schema} name = getattr(tool, "name", "unknown") - if name == self._missing_capability_tool_name: + if self._is_virtual_tool_name(name): return tool new_schema = add_context_parameter_to_schema( getattr(tool, "inputSchema", None), name, description diff --git a/posthog/mcp/types.py b/posthog/mcp/types.py index c97581200..c76b52b9f 100644 --- a/posthog/mcp/types.py +++ b/posthog/mcp/types.py @@ -17,7 +17,17 @@ from dataclasses import dataclass, field from datetime import datetime -from typing import Any, Awaitable, Callable, Dict, Literal, Optional, TypedDict, Union +from typing import ( + Any, + Awaitable, + Callable, + Dict, + List, + Literal, + Optional, + TypedDict, + Union, +) from .logger import LoggerFn @@ -28,6 +38,11 @@ "MCPAnalyticsModelSource", "UserIdentity", "CaptureEventData", + "CollectFeedbackConfig", + "CollectFeedbackOptions", + "FeedbackReport", + "FeedbackSentiment", + "FeedbackType", "PreparedToolCall", ] @@ -92,6 +107,64 @@ class MCPAnalyticsModelOptions: description: Optional[str] = None +FeedbackType = Literal["missing_capability", "issue", "praise", "other"] +FeedbackSentiment = Literal["positive", "neutral", "negative", "mixed"] + +# Route each report to a real backend. Return a string (sync or async) to replace +# the default acknowledgement text; a raise is logged and falls back to it. +OnFeedbackFn = Callable[["FeedbackReport"], Any] # -> Optional[str] | awaitable + + +@dataclass +class CollectFeedbackOptions: + """Object form of the ``collect_feedback`` option (``True`` uses the defaults).""" + + # Rename the ``send_feedback`` virtual tool. Set once so the tool is + # advertised and detected under the same name. + tool_name: Optional[str] = None + # Replace the default tool description. + description: Optional[str] = None + # Host-specific fields merged into the tool's advertised input schema (plain + # JSON Schema fragments, keyed by property name). Each declared key is + # captured as a ``$mcp_feedback_`` event property through the standard + # sanitize/redact/truncate pipeline; arguments the agent invents beyond the + # schema are never captured. A key that collides with a core field or an + # SDK-injected argument raises at configuration time. + extra_properties: Optional[Dict[str, Dict[str, Any]]] = None + # Keys of ``extra_properties`` to advertise as required. + extra_required: Optional[List[str]] = None + # ``instrument()`` path only — a custom dispatcher routes reports itself via + # :attr:`PreparedToolCall.feedback_report`. The ``$mcp_feedback`` event is + # captured whether or not the handler raises. + on_feedback: Optional[OnFeedbackFn] = None + + +# The ``collect_feedback`` option: ``True``/``False`` or the object form. +CollectFeedbackConfig = Union[bool, CollectFeedbackOptions] + + +@dataclass +class FeedbackReport: + """One parsed ``send_feedback`` call, as handed to ``on_feedback`` and the + custom dispatcher.""" + + # Invalid or missing values fall back to ``other``. + feedback_type: str = "other" + # One-sentence summary; empty string when the agent omitted it. + summary: str = "" + sentiment: Optional[str] = None + friction_points: Optional[str] = None + suggested_improvement: Optional[str] = None + details: Optional[str] = None + # The existing tool the feedback is about (the ``tool_name`` argument). + tool_name: Optional[str] = None + task_completed: Optional[bool] = None + # Values of the declared ``extra_properties`` fields. + extras: JsonRecord = field(default_factory=dict) + # The full raw arguments, for the handler only — never captured. + raw: JsonRecord = field(default_factory=dict) + + # request is a JSON-RPC-shaped dict; extra carries session_id / headers. IdentifyFn = Callable[ ..., Any @@ -123,6 +196,16 @@ class MCPAnalyticsOptions: # Capture the model from recognized client metadata, falling back to an # SDK-injected llm_model argument. Off by default. capture_model: Union[bool, MCPAnalyticsModelOptions] = False + # Inject the `send_feedback` virtual tool so agents can send feedback about + # this server to its developers — a missing capability (the priority + # category), a tool that failed or confused them, or praise. Calls to it emit + # `$mcp_feedback` (never a `$mcp_tool_call`). Off by default. `True` uses the + # defaults; the object form renames the tool, replaces its description, + # declares host-specific extra_properties, or wires an on_feedback handler. + # Covers what `report_missing` covers (as feedback_type "missing_capability"), + # so new integrations should enable only one of the two. New field appended + # last: positional construction of the earlier fields must keep working. + collect_feedback: Union[bool, CollectFeedbackOptions] = False @dataclass @@ -145,6 +228,14 @@ class PreparedToolCall: is_missing_capability: bool = False llm_model: Optional[str] = None llm_model_source: Optional[MCPAnalyticsModelSource] = None + # True when the call targeted the ``send_feedback`` virtual tool AND the + # constructor's ``collect_feedback`` option is set. Always False without that + # opt-in, so a real tool that happens to use the name is never shadowed. + is_feedback: bool = False + # The parsed report, set only when ``is_feedback`` is True. Pass it to + # ``PostHogMCP.capture_feedback`` and to your own feedback backend, then + # reply with ``send_feedback_result()`` or a custom text. + feedback_report: Optional[FeedbackReport] = None @dataclass diff --git a/posthog/test/mcp/conftest.py b/posthog/test/mcp/conftest.py index 65c0e887d..c28407858 100644 --- a/posthog/test/mcp/conftest.py +++ b/posthog/test/mcp/conftest.py @@ -16,6 +16,7 @@ "test_fastmcp.py", "test_fastmcp_v2.py", "test_features_m4.py", + "test_feedback.py", "test_lowlevel.py", "test_review_fixes.py", ] diff --git a/posthog/test/mcp/test_feedback.py b/posthog/test/mcp/test_feedback.py new file mode 100644 index 000000000..a4392e3bb --- /dev/null +++ b/posthog/test/mcp/test_feedback.py @@ -0,0 +1,681 @@ +"""Tests for the ``send_feedback`` virtual tool (the ``collect_feedback`` option).""" + +import mcp.types as mcp_types +import pytest +from mcp.server.fastmcp import FastMCP +from mcp.server.lowlevel import Server + +from posthog.mcp import ( + SEND_FEEDBACK_TOOL_NAME, + CollectFeedbackOptions, + PostHogMCP, + instrument, + send_feedback_result, +) +from posthog.mcp.feedback import ( + build_feedback_event_properties, + build_feedback_intent, + get_feedback_tool_descriptor, + parse_feedback_report, + send_feedback_result_text, +) +from posthog.mcp.types import MCPAnalyticsOptions +from posthog.test.mcp._helpers import ( + FakeClient, + events_named as _events, + flush_background as _flush, +) + +_REPORT_ARGS = { + "feedback_type": "missing_capability", + "summary": "No tool to delete multiple cohorts in one call.", + "details": "Deleted 20 cohorts one by one via cohort-delete.", + "friction_points": "cohort-delete accepts a single id; no batch variant.", + "suggested_improvement": "Add a bulk delete tool.", + "sentiment": "negative", + "task_completed": True, +} + + +def make_fastmcp(): + server = FastMCP("feedback-fastmcp") + + @server.tool() + def add(a: int, b: int) -> str: + return f"sum is {a + b}" + + return server + + +def make_lowlevel(): + server = Server("feedback-lowlevel") + + @server.list_tools() + async def list_tools(): + return [ + mcp_types.Tool( + name="echo", + description="Echo", + inputSchema={ + "type": "object", + "properties": {"msg": {"type": "string"}}, + "required": ["msg"], + }, + ) + ] + + @server.call_tool() + async def call_tool(name, arguments): + return [mcp_types.TextContent(type="text", text=str(arguments.get("msg")))] + + return server + + +def _call_request(name, arguments): + return mcp_types.CallToolRequest( + method="tools/call", + params=mcp_types.CallToolRequestParams(name=name, arguments=arguments), + ) + + +async def _list_tools_lowlevel(server): + handler = server.request_handlers[mcp_types.ListToolsRequest] + return await handler(mcp_types.ListToolsRequest(method="tools/list")) + + +# --- descriptor + config validation ------------------------------------------- + + +def test_descriptor_defaults(): + descriptor = get_feedback_tool_descriptor() + assert descriptor["name"] == SEND_FEEDBACK_TOOL_NAME + assert "missing capability" in descriptor["description"] + assert descriptor["inputSchema"]["required"] == ["feedback_type", "summary"] + assert set(descriptor["inputSchema"]["properties"]) == { + "feedback_type", + "summary", + "details", + "friction_points", + "suggested_improvement", + "tool_name", + "sentiment", + "task_completed", + } + assert descriptor["annotations"] == { + "title": "Send feedback", + "readOnlyHint": True, + "openWorldHint": True, + "idempotentHint": True, + "destructiveHint": False, + } + + +def test_descriptor_merges_extras_and_custom_name(): + options = CollectFeedbackOptions( + tool_name="report_feedback", + description="Tell us.", + extra_properties={"product_area": {"type": "string", "description": "Area."}}, + extra_required=["product_area"], + ) + descriptor = get_feedback_tool_descriptor(options) + assert descriptor["name"] == "report_feedback" + assert descriptor["description"] == "Tell us." + assert descriptor["inputSchema"]["properties"]["product_area"]["type"] == "string" + assert descriptor["inputSchema"]["required"] == [ + "feedback_type", + "summary", + "product_area", + ] + + +def test_descriptor_deep_copies_host_fragments(): + fragment = {"type": "string", "enum": ["a", "b"]} + options = CollectFeedbackOptions(extra_properties={"area": fragment}) + descriptor = get_feedback_tool_descriptor(options) + fragment["enum"].append("mutated") + descriptor["inputSchema"]["properties"]["feedback_type"]["enum"].append("bogus") + assert get_feedback_tool_descriptor(options)["inputSchema"]["properties"]["area"][ + "enum" + ] == ["a", "b", "mutated"] + assert ( + "bogus" + not in get_feedback_tool_descriptor()["inputSchema"]["properties"][ + "feedback_type" + ]["enum"] + ) + + +@pytest.mark.parametrize( + "key", ["summary", "type", "tool", "context", "conversation_id", "llm_model"] +) +def test_descriptor_rejects_reserved_extra_keys(key): + options = CollectFeedbackOptions(extra_properties={key: {"type": "string"}}) + with pytest.raises(ValueError, match="collides"): + get_feedback_tool_descriptor(options) + + +def test_descriptor_rejects_undeclared_extra_required(): + options = CollectFeedbackOptions(extra_required=["ghost"]) + with pytest.raises(ValueError, match="not declared"): + get_feedback_tool_descriptor(options) + + +def test_instrument_fails_fast_on_config_error(): + server = make_fastmcp() + options = MCPAnalyticsOptions( + collect_feedback=CollectFeedbackOptions( + extra_properties={"context": {"type": "string"}} + ) + ) + with pytest.raises(ValueError, match="collides"): + instrument(server, FakeClient(), options) + + +# --- parsing ------------------------------------------------------------------- + + +def test_parse_falls_back_and_keeps_only_declared_extras(): + options = CollectFeedbackOptions(extra_properties={"area": {"type": "string"}}) + report = parse_feedback_report( + { + "feedback_type": "bogus", + "summary": " ", + "sentiment": "angry", + "task_completed": "yes", + "area": "cohorts", + "invented": "never captured", + }, + options, + ) + assert report.feedback_type == "other" + assert report.summary == "" + assert report.sentiment is None + assert report.task_completed is None + assert report.extras == {"area": "cohorts"} + assert report.raw["invented"] == "never captured" + + +def test_parse_handles_missing_arguments(): + report = parse_feedback_report(None) + assert report.feedback_type == "other" + assert report.summary == "" + assert report.extras == {} and report.raw == {} + + +def test_parse_reads_all_core_fields(): + report = parse_feedback_report({**_REPORT_ARGS, "tool_name": "cohort-delete"}) + assert report.feedback_type == "missing_capability" + assert report.summary == _REPORT_ARGS["summary"] + assert report.details == _REPORT_ARGS["details"] + assert report.friction_points == _REPORT_ARGS["friction_points"] + assert report.suggested_improvement == _REPORT_ARGS["suggested_improvement"] + assert report.tool_name == "cohort-delete" + assert report.sentiment == "negative" + assert report.task_completed is True + + +# --- event properties + intent --------------------------------------------------- + + +def test_properties_carry_all_fields_and_redact_pii(): + report = parse_feedback_report( + { + **_REPORT_ARGS, + "details": "Reach me at jane@example.com about it.", + "tool_name": "cohort-delete", + } + ) + props = build_feedback_event_properties(report) + assert props["$mcp_feedback_type"] == "missing_capability" + assert props["$mcp_feedback_summary"] == _REPORT_ARGS["summary"] + assert props["$mcp_feedback_details"] == "Reach me at [redacted] about it." + assert props["$mcp_feedback_friction_points"] == _REPORT_ARGS["friction_points"] + assert ( + props["$mcp_feedback_suggested_improvement"] + == _REPORT_ARGS["suggested_improvement"] + ) + assert props["$mcp_feedback_tool"] == "cohort-delete" + assert props["$mcp_feedback_sentiment"] == "negative" + assert props["$mcp_feedback_task_completed"] is True + + +def test_properties_bound_free_text_and_tool_name(): + report = parse_feedback_report( + {"feedback_type": "issue", "summary": "s" * 5000, "tool_name": "t" * 500} + ) + props = build_feedback_event_properties(report) + assert len(props["$mcp_feedback_summary"]) == 2048 + 3 + assert props["$mcp_feedback_summary"].endswith("...") + assert len(props["$mcp_feedback_tool"]) == 256 + 3 + + +def test_properties_capture_declared_extras_only(): + options = CollectFeedbackOptions( + extra_properties={ + "area": {"type": "string"}, + "score": {"type": "number"}, + "tags": {"type": "array"}, + } + ) + report = parse_feedback_report( + { + "feedback_type": "praise", + "summary": "Great tools.", + "area": "mail me: jane@example.com", + "score": 9, + "tags": ["a", "b"], + "invented": "nope", + }, + options, + ) + props = build_feedback_event_properties(report) + assert props["$mcp_feedback_area"] == "mail me: [redacted]" + assert props["$mcp_feedback_score"] == 9 + assert props["$mcp_feedback_tags"] == '["a", "b"]' + assert "$mcp_feedback_invented" not in props + + +def test_intent_joins_summary_and_details(): + report = parse_feedback_report(_REPORT_ARGS) + assert ( + build_feedback_intent(report) + == f"{_REPORT_ARGS['summary']}\n\n{_REPORT_ARGS['details']}" + ) + summary_only = parse_feedback_report({"summary": "Just this."}) + assert build_feedback_intent(summary_only) == "Just this." + assert build_feedback_intent(parse_feedback_report(None)) == "" + + +# --- instrument(): FastMCP ------------------------------------------------------- + + +async def test_fastmcp_advertises_and_captures_feedback(): + server = make_fastmcp() + client = FakeClient() + instrument( + server, client, MCPAnalyticsOptions(collect_feedback=True, capture_model=True) + ) + + list_handler = server._mcp_server.request_handlers[mcp_types.ListToolsRequest] + result = await list_handler(mcp_types.ListToolsRequest(method="tools/list")) + virtual = [t for t in result.root.tools if t.name == "send_feedback"] + assert virtual + schema_props = virtual[0].inputSchema["properties"] + # Its intent rides its own arguments; the model argument is still advertised. + assert "context" not in schema_props and "conversation_id" not in schema_props + assert "llm_model" in schema_props + + canned = await server._tool_manager.call_tool("send_feedback", dict(_REPORT_ARGS)) + await _flush() + + assert canned[0].text == send_feedback_result_text() + feedback = _events(client, "$mcp_feedback") + assert len(feedback) == 1 + props = feedback[0]["properties"] + assert props["$mcp_feedback_type"] == "missing_capability" + assert props["$mcp_feedback_task_completed"] is True + assert ( + props["$mcp_intent"] + == f"{_REPORT_ARGS['summary']}\n\n{_REPORT_ARGS['details']}" + ) + assert props["$mcp_intent_source"] == "context_parameter" + # The raw arguments are agent-narrated free text — never captured. + assert "$mcp_parameters" not in props + # A send_feedback call is NOT a normal tool call. + assert _events(client, "$mcp_tool_call") == [] + + +async def test_fastmcp_invalid_type_falls_back_to_other(): + server = make_fastmcp() + client = FakeClient() + instrument(server, client, MCPAnalyticsOptions(collect_feedback=True)) + + await server._tool_manager.call_tool("send_feedback", {"feedback_type": "bogus"}) + await _flush() + + feedback = _events(client, "$mcp_feedback") + assert feedback[0]["properties"]["$mcp_feedback_type"] == "other" + assert "$mcp_intent" not in feedback[0]["properties"] + + +async def test_fastmcp_collision_fails_open(): + server = make_fastmcp() + + @server.tool() + def send_feedback(note: str) -> str: + return f"real tool got {note}" + + client = FakeClient() + instrument(server, client, MCPAnalyticsOptions(collect_feedback=True)) + + list_handler = server._mcp_server.request_handlers[mcp_types.ListToolsRequest] + result = await list_handler(mcp_types.ListToolsRequest(method="tools/list")) + named = [t for t in result.root.tools if t.name == "send_feedback"] + # Advertised once — the real tool, with normal context injection. + assert len(named) == 1 + assert "context" in named[0].inputSchema["properties"] + + out = await server._tool_manager.call_tool( + "send_feedback", {"note": "hi", "context": "using the real tool"} + ) + await _flush() + + assert "real tool got hi" in str(out) + assert _events(client, "$mcp_feedback") == [] + assert _events(client, "$mcp_tool_call") + + +async def test_fastmcp_collision_fails_open_before_any_listing(): + server = make_fastmcp() + + @server.tool() + def send_feedback(note: str) -> str: + return f"real tool got {note}" + + client = FakeClient() + instrument(server, client, MCPAnalyticsOptions(collect_feedback=True)) + + # No tools/list served yet — the live registry probe must protect the tool. + out = await server._tool_manager.call_tool("send_feedback", {"note": "early"}) + await _flush() + + assert "real tool got early" in str(out) + assert _events(client, "$mcp_feedback") == [] + assert _events(client, "$mcp_tool_call") + + +async def test_fastmcp_custom_tool_name(): + server = make_fastmcp() + client = FakeClient() + instrument( + server, + client, + MCPAnalyticsOptions( + collect_feedback=CollectFeedbackOptions(tool_name="report_feedback") + ), + ) + + list_handler = server._mcp_server.request_handlers[mcp_types.ListToolsRequest] + result = await list_handler(mcp_types.ListToolsRequest(method="tools/list")) + names = [t.name for t in result.root.tools] + assert "report_feedback" in names and "send_feedback" not in names + + await server._tool_manager.call_tool( + "report_feedback", {"feedback_type": "praise", "summary": "Nice."} + ) + await _flush() + assert _events(client, "$mcp_feedback") + + +async def test_fastmcp_coexists_with_report_missing(): + server = make_fastmcp() + client = FakeClient() + instrument( + server, client, MCPAnalyticsOptions(report_missing=True, collect_feedback=True) + ) + + list_handler = server._mcp_server.request_handlers[mcp_types.ListToolsRequest] + result = await list_handler(mcp_types.ListToolsRequest(method="tools/list")) + names = [t.name for t in result.root.tools] + assert "get_more_tools" in names and "send_feedback" in names + + await server._tool_manager.call_tool("get_more_tools", {"context": "need csv"}) + await server._tool_manager.call_tool( + "send_feedback", {"feedback_type": "praise", "summary": "Nice."} + ) + await _flush() + + assert len(_events(client, "$mcp_missing_capability")) == 1 + assert len(_events(client, "$mcp_feedback")) == 1 + assert _events(client, "$mcp_tool_call") == [] + + +# --- on_feedback ----------------------------------------------------------------- + + +async def test_on_feedback_custom_reply(): + server = make_fastmcp() + client = FakeClient() + seen = [] + + def on_feedback(report): + seen.append(report) + return "Thanks - your feedback reached the team." + + instrument( + server, + client, + MCPAnalyticsOptions( + collect_feedback=CollectFeedbackOptions(on_feedback=on_feedback) + ), + ) + + canned = await server._tool_manager.call_tool("send_feedback", dict(_REPORT_ARGS)) + await _flush() + + assert canned[0].text == "Thanks - your feedback reached the team." + assert seen and seen[0].feedback_type == "missing_capability" + assert _events(client, "$mcp_feedback") + + +async def test_on_feedback_async_handler(): + server = make_fastmcp() + client = FakeClient() + + async def on_feedback(report): + return "async thanks" + + instrument( + server, + client, + MCPAnalyticsOptions( + collect_feedback=CollectFeedbackOptions(on_feedback=on_feedback) + ), + ) + + canned = await server._tool_manager.call_tool("send_feedback", dict(_REPORT_ARGS)) + await _flush() + assert canned[0].text == "async thanks" + + +async def test_on_feedback_raise_falls_back_and_still_captures(): + server = make_fastmcp() + client = FakeClient() + + def on_feedback(report): + raise RuntimeError("backend down") + + instrument( + server, + client, + MCPAnalyticsOptions( + collect_feedback=CollectFeedbackOptions(on_feedback=on_feedback) + ), + ) + + canned = await server._tool_manager.call_tool("send_feedback", dict(_REPORT_ARGS)) + await _flush() + + assert canned[0].text == send_feedback_result_text() + assert _events(client, "$mcp_feedback") + + +# --- instrument(): low-level v1 ---------------------------------------------------- + + +async def test_lowlevel_advertises_and_captures_feedback(): + server = make_lowlevel() + client = FakeClient() + instrument(server, client, MCPAnalyticsOptions(collect_feedback=True)) + + result = await _list_tools_lowlevel(server) + assert "send_feedback" in [t.name for t in result.root.tools] + + call_handler = server.request_handlers[mcp_types.CallToolRequest] + out = await call_handler(_call_request("send_feedback", dict(_REPORT_ARGS))) + await _flush() + + assert out.root.isError is False + assert out.root.content[0].text == send_feedback_result_text() + feedback = _events(client, "$mcp_feedback") + assert len(feedback) == 1 + assert "$mcp_parameters" not in feedback[0]["properties"] + assert _events(client, "$mcp_tool_call") == [] + + +async def test_lowlevel_collision_fails_open_after_listing(): + server = Server("feedback-lowlevel-collision") + + @server.list_tools() + async def list_tools(): + return [ + mcp_types.Tool( + name="send_feedback", + description="A real application tool", + inputSchema={ + "type": "object", + "properties": {"note": {"type": "string"}}, + }, + ) + ] + + @server.call_tool() + async def call_tool(name, arguments): + return [mcp_types.TextContent(type="text", text="real tool ran")] + + client = FakeClient() + instrument(server, client, MCPAnalyticsOptions(collect_feedback=True)) + + result = await _list_tools_lowlevel(server) + assert [t.name for t in result.root.tools].count("send_feedback") == 1 + + call_handler = server.request_handlers[mcp_types.CallToolRequest] + out = await call_handler(_call_request("send_feedback", {"note": "hi"})) + await _flush() + + assert out.root.content[0].text == "real tool ran" + assert _events(client, "$mcp_feedback") == [] + assert _events(client, "$mcp_tool_call") + + +async def test_feedback_never_mints_conversation_id(): + server = make_lowlevel() + client = FakeClient() + instrument( + server, + client, + MCPAnalyticsOptions(collect_feedback=True, enable_conversation_id=True), + ) + + result = await _list_tools_lowlevel(server) + virtual = [t for t in result.root.tools if t.name == "send_feedback"][0] + assert "conversation_id" not in virtual.inputSchema["properties"] + + call_handler = server.request_handlers[mcp_types.CallToolRequest] + out = await call_handler(_call_request("send_feedback", dict(_REPORT_ARGS))) + await _flush() + + feedback = _events(client, "$mcp_feedback") + assert "$mcp_conversation_id" not in feedback[0]["properties"] + # No prompt-back block appended to the acknowledgement. + assert len(out.root.content) == 1 + + +# --- PostHogMCP custom dispatcher --------------------------------------------------- + + +def make_client(**kwargs): + client = PostHogMCP("phc_test", host="https://us.i.posthog.com", **kwargs) + captured = [] + client.capture = lambda event, **kw: captured.append({"event": event, **kw}) + return client, captured + + +def test_posthogmcp_constructor_fails_fast_on_config_error(): + with pytest.raises(ValueError, match="collides"): + PostHogMCP( + "phc_test", + collect_feedback=CollectFeedbackOptions( + extra_properties={"llm_model": {"type": "string"}} + ), + ) + + +async def test_posthogmcp_prepare_tool_list_appends_descriptor(): + client, _ = make_client(collect_feedback=True) + tools = [{"name": "search", "inputSchema": {"type": "object", "properties": {}}}] + + prepared = client.prepare_tool_list(tools, collect_feedback=True) + names = [t["name"] for t in prepared] + assert names == ["search", "send_feedback"] + virtual = prepared[-1] + # The virtual tool never gets the injected context argument. + assert "context" not in virtual["inputSchema"]["properties"] + + # Not appended without the per-call flag, nor over a real tool by the name. + assert len(client.prepare_tool_list(tools)) == 1 + collided = client.prepare_tool_list( + [ + { + "name": "send_feedback", + "inputSchema": {"type": "object", "properties": {}}, + } + ], + collect_feedback=True, + ) + assert [t["name"] for t in collided] == ["send_feedback"] + + +async def test_posthogmcp_prepare_tool_list_requires_constructor_option(): + client, _ = make_client() + tools = [{"name": "search", "inputSchema": {"type": "object", "properties": {}}}] + prepared = client.prepare_tool_list(tools, collect_feedback=True) + assert [t["name"] for t in prepared] == ["search"] + + +async def test_posthogmcp_prepare_tool_call_flags_feedback(): + client, _ = make_client(collect_feedback=True) + call = client.prepare_tool_call("send_feedback", dict(_REPORT_ARGS)) + assert call.is_feedback is True + assert call.feedback_report is not None + assert call.feedback_report.feedback_type == "missing_capability" + + ordinary = client.prepare_tool_call("search", {"q": "x"}) + assert ordinary.is_feedback is False and ordinary.feedback_report is None + + +async def test_posthogmcp_prepare_tool_call_without_opt_in_never_flags(): + client, _ = make_client() + call = client.prepare_tool_call("send_feedback", dict(_REPORT_ARGS)) + assert call.is_feedback is False and call.feedback_report is None + + +async def test_posthogmcp_capture_feedback_event_shape(): + client, captured = make_client(collect_feedback=True) + call = client.prepare_tool_call("send_feedback", dict(_REPORT_ARGS)) + client.capture_feedback( + report=call.feedback_report, + distinct_id="user_1", + session_id="s1", + llm_model="claude-opus-4-8", + llm_model_source="self_reported", + properties={"host_prop": "kept"}, + ) + await _flush() + + events = _events(captured, "$mcp_feedback") + assert len(events) == 1 + props = events[0]["properties"] + assert props["$mcp_resource_name"] == "send_feedback" + assert props["$mcp_feedback_type"] == "missing_capability" + assert props["$mcp_intent"].startswith(_REPORT_ARGS["summary"]) + assert props["$mcp_llm_model"] == "claude-opus-4-8" + assert props["host_prop"] == "kept" + assert "$mcp_parameters" not in props + assert events[0]["distinct_id"] == "user_1" + + +def test_send_feedback_result_shape(): + result = send_feedback_result() + assert result["content"][0]["type"] == "text" + assert result["content"][0]["text"] == send_feedback_result_text() diff --git a/posthog/test/mcp/test_v2_lowlevel.py b/posthog/test/mcp/test_v2_lowlevel.py index eb543f86f..e7d36bade 100644 --- a/posthog/test/mcp/test_v2_lowlevel.py +++ b/posthog/test/mcp/test_v2_lowlevel.py @@ -252,6 +252,68 @@ async def test_report_missing_appends_virtual_tool(): assert missing and missing[0]["properties"]["$mcp_intent"] == "need an email tool" +async def test_collect_feedback_appends_virtual_tool(): + server = make_server() + client = FakeClient() + instrument(server, client, MCPAnalyticsOptions(collect_feedback=True)) + + result = await _list_tools(server) + assert "send_feedback" in [t.name for t in result.tools] + + call_result = await _call_tool( + server, + "send_feedback", + {"feedback_type": "issue", "summary": "add rejects floats."}, + ) + await _flush() + + assert call_result.is_error is False + feedback = _events(client, "$mcp_feedback") + assert len(feedback) == 1 + assert feedback[0]["properties"]["$mcp_feedback_type"] == "issue" + assert "$mcp_parameters" not in feedback[0]["properties"] + assert _events(client, "$mcp_tool_call") == [] + + +async def test_collect_feedback_collision_fails_open_after_listing(): + async def on_call_tool(ctx, params): + return mcp_types.CallToolResult( + content=[mcp_types.TextContent(type="text", text="real tool ran")] + ) + + async def on_list_tools(ctx, params): + return mcp_types.ListToolsResult( + tools=[ + mcp_types.Tool( + name="send_feedback", + description="A real application tool", + input_schema={ + "type": "object", + "properties": {"note": {"type": "string"}}, + }, + ) + ] + ) + + server = Server( + "low-v2-feedback-collision", + on_call_tool=on_call_tool, + on_list_tools=on_list_tools, + ) + client = FakeClient() + instrument(server, client, MCPAnalyticsOptions(collect_feedback=True)) + + result = await _list_tools(server) + assert [t.name for t in result.tools].count("send_feedback") == 1 + + out = await _call_tool(server, "send_feedback", {"note": "hi"}) + await _flush() + + assert out.content[0].text == "real tool ran" + assert _events(client, "$mcp_feedback") == [] + assert _events(client, "$mcp_tool_call") + + async def test_callbacks_can_read_headers_through_the_helper(): """The same `identify` body must work on both SDK majors: `extra["ctx"]` is the SDK's own context and `get_request_headers` normalises the read.""" diff --git a/posthog/test/mcp/test_v2_mcpserver.py b/posthog/test/mcp/test_v2_mcpserver.py index fb938fb26..e8021c787 100644 --- a/posthog/test/mcp/test_v2_mcpserver.py +++ b/posthog/test/mcp/test_v2_mcpserver.py @@ -379,6 +379,56 @@ async def test_report_missing_accepts_omitted_arguments(): assert "$mcp_intent" not in missing[0]["properties"] +async def test_collect_feedback_advertises_and_captures(): + server = make_server() + client = FakeClient() + instrument(server, client, MCPAnalyticsOptions(collect_feedback=True)) + + listed = await _list_tools(server) + assert "send_feedback" in [t.name for t in listed.tools] + + result = await _call_tool( + server, + "send_feedback", + { + "feedback_type": "missing_capability", + "summary": "No email tool.", + "details": "Wanted to notify a teammate.", + }, + ) + await _flush() + + assert result.is_error is False + feedback = _events(client, "$mcp_feedback") + assert len(feedback) == 1 + props = feedback[0]["properties"] + assert props["$mcp_feedback_type"] == "missing_capability" + assert props["$mcp_intent"] == "No email tool.\n\nWanted to notify a teammate." + assert "$mcp_parameters" not in props + # A send_feedback call is NOT a normal tool call. + assert _events(client, "$mcp_tool_call") == [] + + +async def test_collect_feedback_collision_fails_open(): + server = make_server() + + @server.tool() + def send_feedback(note: str) -> str: + return f"real tool got {note}" + + client = FakeClient() + instrument(server, client, MCPAnalyticsOptions(collect_feedback=True)) + + listed = await _list_tools(server) + assert [t.name for t in listed.tools].count("send_feedback") == 1 + + await _call_tool(server, "send_feedback", {"note": "hi", "context": "real tool"}) + await _flush() + + assert _events(client, "$mcp_feedback") == [] + assert _events(client, "$mcp_tool_call") + + async def test_instrument_is_idempotent(): server = make_server() client = FakeClient() diff --git a/references/public_api_snapshot.txt b/references/public_api_snapshot.txt index 2186387d2..6c28b7dd6 100644 --- a/references/public_api_snapshot.txt +++ b/references/public_api_snapshot.txt @@ -332,6 +332,8 @@ alias posthog.inner_tag -> posthog.contexts.tag alias posthog.integrations.django.Client -> posthog.client.Client alias posthog.integrations.django.contexts -> posthog.contexts alias posthog.mcp.CaptureEventData -> posthog.mcp.types.CaptureEventData +alias posthog.mcp.CollectFeedbackOptions -> posthog.mcp.types.CollectFeedbackOptions +alias posthog.mcp.FeedbackReport -> posthog.mcp.types.FeedbackReport alias posthog.mcp.MCPAnalyticsContextOptions -> posthog.mcp.types.MCPAnalyticsContextOptions alias posthog.mcp.MCPAnalyticsModelOptions -> posthog.mcp.types.MCPAnalyticsModelOptions alias posthog.mcp.MCPAnalyticsModelSource -> posthog.mcp.types.MCPAnalyticsModelSource @@ -343,6 +345,7 @@ alias posthog.mcp.PostHogMCPAnalyticsEvent -> posthog.mcp.constants.PostHogMCPAn alias posthog.mcp.PostHogMCPAnalyticsProperty -> posthog.mcp.constants.PostHogMCPAnalyticsProperty alias posthog.mcp.PostHogMcpStatelessSessionMiddleware -> posthog.mcp.asgi.PostHogMcpStatelessSessionMiddleware alias posthog.mcp.PreparedToolCall -> posthog.mcp.types.PreparedToolCall +alias posthog.mcp.SEND_FEEDBACK_TOOL_NAME -> posthog.mcp.feedback.SEND_FEEDBACK_TOOL_NAME alias posthog.mcp.SessionTokenPayload -> posthog.mcp.session_token.SessionTokenPayload alias posthog.mcp.UserIdentity -> posthog.mcp.types.UserIdentity alias posthog.mcp.__version__ -> posthog.mcp.version.__version__ @@ -361,6 +364,7 @@ alias posthog.mcp.encode_session_id -> posthog.mcp.session_token.encode_session_ alias posthog.mcp.get_mcp_session -> posthog.mcp.asgi.get_mcp_session alias posthog.mcp.get_more_tools_result -> posthog.mcp.tools.get_more_tools_result alias posthog.mcp.get_request_headers -> posthog.mcp.request_headers.get_request_headers +alias posthog.mcp.send_feedback_result -> posthog.mcp.feedback.send_feedback_result alias posthog.mcp.set_logger -> posthog.mcp.logger.set_logger alias posthog.metrics_capture.VERSION -> posthog.version.VERSION alias posthog.metrics_capture.remove_trailing_slash -> posthog.utils.remove_trailing_slash @@ -741,6 +745,7 @@ attribute posthog.mcp.asgi.PostHogMcpStatelessSessionMiddleware.app = app attribute posthog.mcp.constants.POSTHOG_MCP_ANALYTICS_SOURCE = 'posthog_mcp_analytics' attribute posthog.mcp.constants.PostHogMCPAnalyticsEvent.CUSTOM = '$mcp_custom' attribute posthog.mcp.constants.PostHogMCPAnalyticsEvent.EXCEPTION = '$exception' +attribute posthog.mcp.constants.PostHogMCPAnalyticsEvent.FEEDBACK = '$mcp_feedback' attribute posthog.mcp.constants.PostHogMCPAnalyticsEvent.IDENTIFY = '$identify' attribute posthog.mcp.constants.PostHogMCPAnalyticsEvent.INITIALIZE = '$mcp_initialize' attribute posthog.mcp.constants.PostHogMCPAnalyticsEvent.MISSING_CAPABILITY = '$mcp_missing_capability' @@ -757,6 +762,14 @@ attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.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.FEEDBACK_DETAILS = '$mcp_feedback_details' +attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.FEEDBACK_FRICTION_POINTS = '$mcp_feedback_friction_points' +attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.FEEDBACK_SENTIMENT = '$mcp_feedback_sentiment' +attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.FEEDBACK_SUGGESTED_IMPROVEMENT = '$mcp_feedback_suggested_improvement' +attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.FEEDBACK_SUMMARY = '$mcp_feedback_summary' +attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.FEEDBACK_TASK_COMPLETED = '$mcp_feedback_task_completed' +attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.FEEDBACK_TOOL = '$mcp_feedback_tool' +attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.FEEDBACK_TYPE = '$mcp_feedback_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' @@ -775,6 +788,7 @@ attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.TOOL_CATEGORY = '$mc attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.TOOL_DESCRIPTION = '$mcp_tool_description' attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.TOOL_NAME = '$mcp_tool_name' attribute posthog.mcp.constants.PostHogMCPAnalyticsProperty.VENDOR_CLIENT = '$mcp_vendor_client' +attribute posthog.mcp.feedback.SEND_FEEDBACK_TOOL_NAME = 'send_feedback' attribute posthog.mcp.session_token.MCP_SESSION_HEADER = 'mcp-session-id' attribute posthog.mcp.session_token.SessionTokenPayload.client_name: Optional[str] = None attribute posthog.mcp.session_token.SessionTokenPayload.client_version: Optional[str] = None @@ -782,11 +796,30 @@ attribute posthog.mcp.session_token.SessionTokenPayload.protocol_version: Option attribute posthog.mcp.session_token.SessionTokenPayload.session_id: str attribute posthog.mcp.types.CaptureEventData.event: str attribute posthog.mcp.types.CaptureEventData.properties: Optional[JsonRecord] = None +attribute posthog.mcp.types.CollectFeedbackConfig = Union[bool, CollectFeedbackOptions] +attribute posthog.mcp.types.CollectFeedbackOptions.description: Optional[str] = None +attribute posthog.mcp.types.CollectFeedbackOptions.extra_properties: Optional[Dict[str, Dict[str, Any]]] = None +attribute posthog.mcp.types.CollectFeedbackOptions.extra_required: Optional[List[str]] = None +attribute posthog.mcp.types.CollectFeedbackOptions.on_feedback: Optional[OnFeedbackFn] = None +attribute posthog.mcp.types.CollectFeedbackOptions.tool_name: Optional[str] = None +attribute posthog.mcp.types.FeedbackReport.details: Optional[str] = None +attribute posthog.mcp.types.FeedbackReport.extras: JsonRecord = field(default_factory=dict) +attribute posthog.mcp.types.FeedbackReport.feedback_type: str = 'other' +attribute posthog.mcp.types.FeedbackReport.friction_points: Optional[str] = None +attribute posthog.mcp.types.FeedbackReport.raw: JsonRecord = field(default_factory=dict) +attribute posthog.mcp.types.FeedbackReport.sentiment: Optional[str] = None +attribute posthog.mcp.types.FeedbackReport.suggested_improvement: Optional[str] = None +attribute posthog.mcp.types.FeedbackReport.summary: str = '' +attribute posthog.mcp.types.FeedbackReport.task_completed: Optional[bool] = None +attribute posthog.mcp.types.FeedbackReport.tool_name: Optional[str] = None +attribute posthog.mcp.types.FeedbackSentiment = Literal['positive', 'neutral', 'negative', 'mixed'] +attribute posthog.mcp.types.FeedbackType = Literal['missing_capability', 'issue', 'praise', 'other'] attribute posthog.mcp.types.MCPAnalyticsContextOptions.description: Optional[str] = None attribute posthog.mcp.types.MCPAnalyticsModelOptions.description: Optional[str] = None attribute posthog.mcp.types.MCPAnalyticsModelSource = Literal['client_metadata', 'self_reported'] attribute posthog.mcp.types.MCPAnalyticsOptions.before_send: Optional[BeforeSendFn] = None attribute posthog.mcp.types.MCPAnalyticsOptions.capture_model: Union[bool, MCPAnalyticsModelOptions] = False +attribute posthog.mcp.types.MCPAnalyticsOptions.collect_feedback: Union[bool, CollectFeedbackOptions] = False attribute posthog.mcp.types.MCPAnalyticsOptions.context: Union[bool, MCPAnalyticsContextOptions] = True attribute posthog.mcp.types.MCPAnalyticsOptions.enable_conversation_id: bool = False attribute posthog.mcp.types.MCPAnalyticsOptions.enable_exception_autocapture: bool = True @@ -797,8 +830,10 @@ attribute posthog.mcp.types.MCPAnalyticsOptions.logger: Optional[LoggerFn] = Non attribute posthog.mcp.types.MCPAnalyticsOptions.missing_capability_tool_name: Optional[str] = None attribute posthog.mcp.types.MCPAnalyticsOptions.report_missing: bool = False attribute posthog.mcp.types.PreparedToolCall.args: Optional[JsonRecord] = None +attribute posthog.mcp.types.PreparedToolCall.feedback_report: Optional[FeedbackReport] = None attribute posthog.mcp.types.PreparedToolCall.intent: Optional[str] = None attribute posthog.mcp.types.PreparedToolCall.intent_source: Optional[str] = None +attribute posthog.mcp.types.PreparedToolCall.is_feedback: bool = False attribute posthog.mcp.types.PreparedToolCall.is_missing_capability: bool = False attribute posthog.mcp.types.PreparedToolCall.llm_model: Optional[str] = None attribute posthog.mcp.types.PreparedToolCall.llm_model_source: Optional[MCPAnalyticsModelSource] = None @@ -986,13 +1021,15 @@ class posthog.mcp.McpAnalytics(key: Any) class posthog.mcp.asgi.PostHogMcpStatelessSessionMiddleware(app: Any) class posthog.mcp.constants.PostHogMCPAnalyticsEvent class posthog.mcp.constants.PostHogMCPAnalyticsProperty -class posthog.mcp.posthog_mcp.PostHogMCP(api_key: str, missing_capability_tool_name: Optional[str] = None, mcp_exception_autocapture: bool = True, capture_model: Union[bool, MCPAnalyticsModelOptions] = False, **kwargs: Any) +class posthog.mcp.posthog_mcp.PostHogMCP(api_key: str, missing_capability_tool_name: Optional[str] = None, mcp_exception_autocapture: bool = True, capture_model: Union[bool, MCPAnalyticsModelOptions] = False, collect_feedback: Union[bool, CollectFeedbackOptions] = False, **kwargs: Any) class posthog.mcp.session_token.SessionTokenPayload(session_id: str, client_name: Optional[str] = None, client_version: Optional[str] = None, protocol_version: Optional[str] = None) class posthog.mcp.types.CaptureEventData(event: str, properties: Optional[JsonRecord] = None) +class posthog.mcp.types.CollectFeedbackOptions(tool_name: Optional[str] = None, description: Optional[str] = None, extra_properties: Optional[Dict[str, Dict[str, Any]]] = None, extra_required: Optional[List[str]] = None, on_feedback: Optional[OnFeedbackFn] = None) +class posthog.mcp.types.FeedbackReport(feedback_type: str = 'other', summary: str = '', sentiment: Optional[str] = None, friction_points: Optional[str] = None, suggested_improvement: Optional[str] = None, details: Optional[str] = None, tool_name: Optional[str] = None, task_completed: Optional[bool] = None, extras: JsonRecord = dict(), raw: JsonRecord = dict()) class posthog.mcp.types.MCPAnalyticsContextOptions(description: Optional[str] = None) class posthog.mcp.types.MCPAnalyticsModelOptions(description: Optional[str] = None) -class posthog.mcp.types.MCPAnalyticsOptions(logger: Optional[LoggerFn] = None, report_missing: bool = False, missing_capability_tool_name: Optional[str] = None, enable_conversation_id: bool = False, enable_exception_autocapture: bool = True, context: Union[bool, MCPAnalyticsContextOptions] = True, identify: Optional[Union[IdentifyFn, UserIdentity]] = None, intent_fallback: Optional[IntentFallbackFn] = None, before_send: Optional[BeforeSendFn] = None, event_properties: Optional[EventPropertiesFn] = None, capture_model: Union[bool, MCPAnalyticsModelOptions] = False) -class posthog.mcp.types.PreparedToolCall(args: Optional[JsonRecord] = None, intent: Optional[str] = None, intent_source: Optional[str] = None, is_missing_capability: bool = False, llm_model: Optional[str] = None, llm_model_source: Optional[MCPAnalyticsModelSource] = None) +class posthog.mcp.types.MCPAnalyticsOptions(logger: Optional[LoggerFn] = None, report_missing: bool = False, missing_capability_tool_name: Optional[str] = None, enable_conversation_id: bool = False, enable_exception_autocapture: bool = True, context: Union[bool, MCPAnalyticsContextOptions] = True, identify: Optional[Union[IdentifyFn, UserIdentity]] = None, intent_fallback: Optional[IntentFallbackFn] = None, before_send: Optional[BeforeSendFn] = None, event_properties: Optional[EventPropertiesFn] = None, capture_model: Union[bool, MCPAnalyticsModelOptions] = False, collect_feedback: Union[bool, CollectFeedbackOptions] = False) +class posthog.mcp.types.PreparedToolCall(args: Optional[JsonRecord] = None, intent: Optional[str] = None, intent_source: Optional[str] = None, is_missing_capability: bool = False, llm_model: Optional[str] = None, llm_model_source: Optional[MCPAnalyticsModelSource] = None, is_feedback: bool = False, feedback_report: Optional[FeedbackReport] = None) class posthog.mcp.types.UserIdentity(distinct_id: str, properties: Optional[JsonRecord] = None, groups: Optional[Dict[str, str]] = None) class posthog.metrics_capture.PostHogMetrics(client, config: Optional[dict] = None) class posthog.poller.Poller(interval, execute, *args, **kwargs) @@ -1182,6 +1219,7 @@ function posthog.join() -> None function posthog.load_feature_flags() function posthog.mcp.asgi.autowire_stateless_mint(server: Any) -> None function posthog.mcp.asgi.get_mcp_session(request_or_scope: Any) -> Optional[SessionTokenPayload] +function posthog.mcp.feedback.send_feedback_result() -> Dict[str, Any] function posthog.mcp.instrument(server: Any, posthog_client: Optional[Client] = None, options: Optional[MCPAnalyticsOptions] = None) -> McpAnalytics function posthog.mcp.logger.set_logger(logger: Optional[LoggerFn]) -> None function posthog.mcp.request_headers.get_request_headers(extra: Any) -> Optional[RequestHeaderBag] @@ -1396,13 +1434,14 @@ method posthog.integrations.django.PosthogContextMiddleware.extract_tags(request method posthog.integrations.django.PosthogContextMiddleware.process_exception(request, exception) method posthog.mcp.McpAnalytics.capture(event: str, properties: Optional[dict] = None) -> None method posthog.mcp.McpAnalytics.flush() -> None +method posthog.mcp.posthog_mcp.PostHogMCP.capture_feedback(*, report: FeedbackReport, llm_model: Optional[str] = None, llm_model_source: Optional[MCPAnalyticsModelSource] = None, protocol_version: Optional[str] = None, distinct_id: Optional[str] = None, session_id: Optional[str] = None, client_user_agent: Optional[str] = None, vendor_client: 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_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, client_user_agent: Optional[str] = None, vendor_client: 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, llm_model: Optional[str] = None, llm_model_source: Optional[MCPAnalyticsModelSource] = None, parameters: Any = None, protocol_version: Optional[str] = None, distinct_id: Optional[str] = None, session_id: Optional[str] = None, client_user_agent: Optional[str] = None, vendor_client: 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, llm_model: Optional[str] = None, llm_model_source: Optional[MCPAnalyticsModelSource] = None, protocol_version: Optional[str] = None, distinct_id: Optional[str] = None, session_id: Optional[str] = None, client_user_agent: Optional[str] = None, vendor_client: 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, client_user_agent: Optional[str] = None, vendor_client: 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, *, request_meta: Optional[JsonRecord] = None, original_tool: Any = 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] +method posthog.mcp.posthog_mcp.PostHogMCP.prepare_tool_list(tools: List[Any], context: Union[bool, MCPAnalyticsContextOptions] = True, report_missing: bool = False, collect_feedback: bool = False) -> List[Any] method posthog.mcp.posthog_mcp.PostHogMCP.shutdown() -> None method posthog.metrics_capture.PostHogMetrics.count(name: str, value: float = 1, unit: Optional[str] = None, attributes: Optional[dict] = None) -> None method posthog.metrics_capture.PostHogMetrics.flush() -> None @@ -1490,6 +1529,7 @@ module posthog.integrations.django module posthog.mcp module posthog.mcp.asgi module posthog.mcp.constants +module posthog.mcp.feedback module posthog.mcp.logger module posthog.mcp.posthog_mcp module posthog.mcp.request_headers From 83f01b40c67898690c1ea0f9f93f87c007b048f2 Mon Sep 17 00:00:00 2001 From: Georgis Andonis Date: Thu, 10 Sep 2026 17:19:14 +0300 Subject: [PATCH 2/8] fix(mcp): address review findings ported from the TS feedback tool - redact structured PII in tool_name like the other free-text fields - enforce declared type/enum on extras: mismatches stay out of extras and the captured properties (raw keeps everything for the handler) - warn when on_feedback is set on the PostHogMCP path, where it is ignored - align capture_feedback's property precedence with the instrument() path (feedback properties win over caller-supplied ones) - correct the reserved-keys comment: SDK-injected arguments do reach raw because the report is parsed before stripping Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 --- posthog/mcp/feedback.py | 46 ++++++++++++++++++++++---- posthog/mcp/posthog_mcp.py | 12 +++++-- posthog/mcp/types.py | 5 ++- posthog/test/mcp/test_feedback.py | 54 ++++++++++++++++++++++++++++++- 4 files changed, 107 insertions(+), 10 deletions(-) diff --git a/posthog/mcp/feedback.py b/posthog/mcp/feedback.py index 2dc943de7..17f8c92dd 100644 --- a/posthog/mcp/feedback.py +++ b/posthog/mcp/feedback.py @@ -104,8 +104,9 @@ # Extra-property names a host may not declare: the core fields themselves, the # names whose `$mcp_feedback_` property would collide with a core property # (`type` -> `$mcp_feedback_type`, `tool` -> `$mcp_feedback_tool`), and the -# SDK-injected analytics arguments, which are stripped before dispatch and so -# would never reach the report. +# SDK-injected analytics arguments — the report is parsed from the raw arguments +# before those are stripped, so an extra by the same name would capture an +# SDK-owned value. _RESERVED_EXTRA_PROPERTY_KEYS = frozenset(_CORE_FEEDBACK_SCHEMA_PROPERTIES) | { "type", "tool", @@ -188,17 +189,46 @@ def _read_string(value: Any) -> Optional[str]: return value if isinstance(value, str) and value.strip() else None +def _matches_extra_schema(value: Any, schema: Dict[str, Any]) -> bool: + """True when the value conforms to the declared fragment's ``type`` and + ``enum`` — the same advisory-schema enforcement the core fields get, so + ``extras`` only ever holds schema-conforming values and a misbehaving agent + shows up as absence rather than as an unexpected shape in the host's handler.""" + if isinstance(value, list): + actual = "array" + elif value is None: + actual = "null" + elif isinstance(value, bool): + actual = "boolean" + elif isinstance(value, (int, float)): + actual = "number" + elif isinstance(value, str): + actual = "string" + else: + actual = "object" + declared = schema.get("type") + if declared != actual and not (declared == "integer" and actual == "number"): + return False + enum = schema.get("enum") + return not isinstance(enum, list) or value in enum + + def parse_feedback_report( args: Optional[Dict[str, Any]], options: Optional[CollectFeedbackOptions] = None, ) -> FeedbackReport: """Parse the raw ``send_feedback`` arguments into a typed report. Never raises: an invalid ``feedback_type`` falls back to ``other``, missing fields stay - ``None``, and only **declared** extras are lifted into ``extras`` — anything - the agent invented reaches the handler via ``raw`` and is never captured.""" + ``None``, and only **declared** extras whose values match their declared + ``type``/``enum`` are lifted into ``extras`` — mismatches and anything the + agent invented reach the handler via ``raw`` only and are never captured.""" raw = args or {} declared = (options.extra_properties if options is not None else None) or {} - extras = {key: raw[key] for key in declared if raw.get(key) is not None} + extras = { + key: raw[key] + for key, schema in declared.items() + if raw.get(key) is not None and _matches_extra_schema(raw[key], schema) + } feedback_type = raw.get("feedback_type") sentiment = raw.get("sentiment") task_completed = raw.get("task_completed") @@ -275,8 +305,12 @@ def build_feedback_event_properties(report: FeedbackReport) -> JsonRecord: report.details ) if report.tool_name: + # Nominally an identifier, but the schema can't stop an agent from + # writing prose into it — so it gets the same PII redaction as the + # other free text. properties[PostHogMCPAnalyticsProperty.FEEDBACK_TOOL] = _truncate_feedback_text( - sanitize_captured_value(report.tool_name), _MAX_FEEDBACK_TOOL_NAME_LENGTH + redact_pii(sanitize_captured_value(report.tool_name)), + _MAX_FEEDBACK_TOOL_NAME_LENGTH, ) if report.task_completed is not None: properties[PostHogMCPAnalyticsProperty.FEEDBACK_TASK_COMPLETED] = ( diff --git a/posthog/mcp/posthog_mcp.py b/posthog/mcp/posthog_mcp.py index 2464413fc..c16879097 100644 --- a/posthog/mcp/posthog_mcp.py +++ b/posthog/mcp/posthog_mcp.py @@ -26,6 +26,7 @@ from ._exceptions import capture_exception from ._instrumentation import drain_pending_sync, fire_and_forget from ._lib_identity import apply_mcp_lib_identity +from .logger import log from ._model_parameters import ( add_model_parameter_to_schema, can_inject_model_parameter, @@ -93,6 +94,12 @@ def __init__( # extra_required) instead of first surfacing it when a tools/list is served. if self._collect_feedback is not None: get_feedback_tool_descriptor(self._collect_feedback) + if self._collect_feedback.on_feedback is not None: + log( + "Warning: collect_feedback.on_feedback is ignored on the PostHogMCP " + "path - route reports from your dispatcher via " + "prepare_tool_call().feedback_report instead." + ) # Whether a failed tool call fans out an `$exception` sibling event. Distinct # from the inherited Client.enable_exception_autocapture (global uncaught-error # hook); this mirrors instrument()'s enable_exception_autocapture, default on. @@ -328,10 +335,11 @@ def capture_feedback( event["protocol_version"] = protocol_version # Deliberately no `parameters`: the arguments are agent-narrated free # text, and the PII-redacted `$mcp_feedback_*` properties are the captured - # surface. Raw arguments would bypass that redaction. + # surface. Raw arguments would bypass that redaction. Feedback properties + # win over the caller's, matching the instrument() path's merge order. event["properties"] = { - **build_feedback_event_properties(report), **(properties or {}), + **build_feedback_event_properties(report), } _apply_intent(event, build_feedback_intent(report), "context_parameter") _apply_model(event, llm_model, llm_model_source) diff --git a/posthog/mcp/types.py b/posthog/mcp/types.py index c76b52b9f..d2f3afdd3 100644 --- a/posthog/mcp/types.py +++ b/posthog/mcp/types.py @@ -159,7 +159,10 @@ class FeedbackReport: # The existing tool the feedback is about (the ``tool_name`` argument). tool_name: Optional[str] = None task_completed: Optional[bool] = None - # Values of the declared ``extra_properties`` fields. + # Values of the declared ``extra_properties`` fields that match their + # declared ``type``/``enum``. A value the agent sent with the wrong shape is + # left out (find it in ``raw`` if you need it), so these are safe to trust + # as declared. extras: JsonRecord = field(default_factory=dict) # The full raw arguments, for the handler only — never captured. raw: JsonRecord = field(default_factory=dict) diff --git a/posthog/test/mcp/test_feedback.py b/posthog/test/mcp/test_feedback.py index a4392e3bb..3fe3de837 100644 --- a/posthog/test/mcp/test_feedback.py +++ b/posthog/test/mcp/test_feedback.py @@ -275,6 +275,43 @@ def test_properties_capture_declared_extras_only(): assert "$mcp_feedback_invented" not in props +def test_extras_must_match_declared_type_and_enum(): + options = CollectFeedbackOptions( + extra_properties={ + "score": {"type": "integer"}, + "channel": {"type": "string", "enum": ["web", "app"]}, + } + ) + report = parse_feedback_report( + { + "feedback_type": "praise", + "summary": "Great tools.", + "score": "very high", + "channel": "email", + }, + options, + ) + # Mismatches stay out of extras and the captured properties; raw keeps them. + assert report.extras == {} + assert report.raw["score"] == "very high" and report.raw["channel"] == "email" + props = build_feedback_event_properties(report) + assert "$mcp_feedback_score" not in props and "$mcp_feedback_channel" not in props + + conforming = parse_feedback_report( + {"feedback_type": "praise", "summary": "s", "score": 9, "channel": "web"}, + options, + ) + assert conforming.extras == {"score": 9, "channel": "web"} + + +def test_tool_name_gets_pii_redaction(): + report = parse_feedback_report( + {"feedback_type": "issue", "summary": "s", "tool_name": "ask jane@example.com"} + ) + props = build_feedback_event_properties(report) + assert props["$mcp_feedback_tool"] == "ask [redacted]" + + def test_intent_joins_summary_and_details(): report = parse_feedback_report(_REPORT_ARGS) assert ( @@ -659,7 +696,7 @@ async def test_posthogmcp_capture_feedback_event_shape(): session_id="s1", llm_model="claude-opus-4-8", llm_model_source="self_reported", - properties={"host_prop": "kept"}, + properties={"host_prop": "kept", "$mcp_feedback_type": "spoofed"}, ) await _flush() @@ -667,6 +704,7 @@ async def test_posthogmcp_capture_feedback_event_shape(): assert len(events) == 1 props = events[0]["properties"] assert props["$mcp_resource_name"] == "send_feedback" + # Feedback properties win over the caller's, matching the instrument() path. assert props["$mcp_feedback_type"] == "missing_capability" assert props["$mcp_intent"].startswith(_REPORT_ARGS["summary"]) assert props["$mcp_llm_model"] == "claude-opus-4-8" @@ -675,6 +713,20 @@ async def test_posthogmcp_capture_feedback_event_shape(): assert events[0]["distinct_id"] == "user_1" +def test_posthogmcp_warns_when_on_feedback_is_set(): + from posthog.mcp import set_logger + + messages = [] + set_logger(messages.append) + try: + make_client( + collect_feedback=CollectFeedbackOptions(on_feedback=lambda report: None) + ) + finally: + set_logger(None) + assert any("on_feedback is ignored" in message for message in messages) + + def test_send_feedback_result_shape(): result = send_feedback_result() assert result["content"][0]["type"] == "text" From a5795b4eebd1848b985645e8ac8de187e410f996 Mon Sep 17 00:00:00 2001 From: Georgis Andonis Date: Fri, 11 Sep 2026 10:42:39 +0300 Subject: [PATCH 3/8] test(mcp): pin PII redaction of emails inside URLs in feedback text The TS SDK's sanitizer re-serializes URLs before PII redaction, which percent-encodes "@" and hides emails from the email pattern. The Python sanitizer does no URL rewriting, so the case already redacts correctly; this test keeps it that way if URL scrubbing is ever added. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 --- posthog/test/mcp/test_feedback.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/posthog/test/mcp/test_feedback.py b/posthog/test/mcp/test_feedback.py index 3fe3de837..f3d364571 100644 --- a/posthog/test/mcp/test_feedback.py +++ b/posthog/test/mcp/test_feedback.py @@ -312,6 +312,22 @@ def test_tool_name_gets_pii_redaction(): assert props["$mcp_feedback_tool"] == "ask [redacted]" +def test_pii_inside_urls_is_redacted(): + # Guards the ordering of sanitize vs redact: a sanitizer that re-serializes + # URLs (percent-encoding "@") before redaction would hide the email from the + # redaction pattern (the TS SDK's veria finding on this feature). + report = parse_feedback_report( + { + "feedback_type": "issue", + "summary": "Login fails at https://example.com/?email=jane@example.com", + } + ) + props = build_feedback_event_properties(report) + assert "jane@example.com" not in props["$mcp_feedback_summary"] + assert "jane%40example.com" not in props["$mcp_feedback_summary"] + assert "[redacted]" in props["$mcp_feedback_summary"] + + def test_intent_joins_summary_and_details(): report = parse_feedback_report(_REPORT_ARGS) assert ( From be63255d0db33ae1bc7a70de87409b8360b6713d Mon Sep 17 00:00:00 2001 From: Georgis Andonis Date: Fri, 11 Sep 2026 11:13:05 +0300 Subject: [PATCH 4/8] fix(mcp): run free-text PII redaction before URL rewriting in feedback capture Main's URL-credential sanitizer (#928) percent-encodes the @ the email pattern anchors on, so sanitize-then-redact let PII inside URLs through the feedback fields. Free text (summary, details, friction_points, suggested_improvement, tool_name) now uses the $mcp_intent pass (credentials -> PII -> URLs), and extras use a new sanitize_free_text_value walker that applies it per string leaf while keeping key-based redaction. sanitize_intent is renamed sanitize_free_text: the pass is no longer intent-specific. Ports posthog-js#4870 commits 9d3b3933, f4a69ea8, 270c4630. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 --- posthog/mcp/_sanitization.py | 32 +++++++++++++------ posthog/mcp/feedback.py | 31 +++++++++--------- posthog/test/mcp/test_feedback.py | 53 +++++++++++++++++++++++++------ 3 files changed, 82 insertions(+), 34 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 90db4ebd0..0600ab69a 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -443,9 +443,9 @@ def _redact_credentials(value: str) -> str: return _redact_secret_tokens(_POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value)) -def sanitize_intent(value: Any) -> Any: - """Sanitize the agent-narrated intent: the binary gate, the credential passes, - structured PII, then URLs. +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. Every step sits where it does for a reason. The binary gate runs first because splicing a redaction into a base64 blob stops it looking like base64, @@ -586,12 +586,23 @@ def redact_pii(value: Any) -> Any: def sanitize_captured_value(value: Any) -> Any: + return _sanitize_value_with(value, _sanitize_string) + + +def sanitize_free_text_value(value: Any) -> Any: + """:func:`sanitize_captured_value` with the free-text string pass + (:func:`sanitize_free_text`) on every string leaf, so nested agent-narrated + values get structured-PII redaction in the load-bearing order too.""" + return _sanitize_value_with(value, sanitize_free_text) + + +def _sanitize_value_with(value: Any, sanitize_string_fn: Any) -> Any: if value is None: return value if isinstance(value, str): - return _sanitize_string(value) + return sanitize_string_fn(value) if isinstance(value, list): - return [sanitize_captured_value(item) for item in value] + return [_sanitize_value_with(item, sanitize_string_fn) for item in value] # bool is an int subclass; both pass through unchanged. if not isinstance(value, dict): return value @@ -601,7 +612,7 @@ def sanitize_captured_value(value: Any) -> Any: result[key] = ( _REDACTED_VALUE if _should_redact_key(str(key)) - else sanitize_captured_value(nested) + else _sanitize_value_with(nested, sanitize_string_fn) ) return result @@ -622,12 +633,13 @@ def sanitize_event(event: Dict[str, Any]) -> Dict[str, Any]: # The intent comes straight from an agent-narrated `context` string, so it # can contain a secret the LLM read aloud or personal data it narrated about - # the user. `sanitize_intent` redacts it like any other captured value and + # the user. `sanitize_free_text` redacts it like any other captured value and # additionally strips structured PII (emails, phone numbers, IPs, cards, - # SSNs). PII redaction is scoped to the intent only — structured tool - # parameters and responses often hold the same shapes as legitimate data. + # SSNs). PII redaction is scoped to agent-narrated free text only — + # structured tool parameters and responses often hold the same shapes as + # legitimate data. if result.get("user_intent") is not None: - result["user_intent"] = sanitize_intent(result["user_intent"]) + result["user_intent"] = sanitize_free_text(result["user_intent"]) if result.get("llm_model") is not None: result["llm_model"] = sanitize_captured_value(result["llm_model"]) diff --git a/posthog/mcp/feedback.py b/posthog/mcp/feedback.py index 17f8c92dd..93297edfb 100644 --- a/posthog/mcp/feedback.py +++ b/posthog/mcp/feedback.py @@ -10,7 +10,7 @@ from typing import Any, Dict, Optional, Union from ._internal import _maybe_await -from ._sanitization import redact_pii, sanitize_captured_value +from ._sanitization import sanitize_free_text, sanitize_free_text_value from .constants import PostHogMCPAnalyticsProperty from .logger import log from .types import CollectFeedbackOptions, FeedbackReport, JsonRecord @@ -257,25 +257,26 @@ def _truncate_feedback_text(value: str, max_length: int) -> str: def _capture_free_text(value: str) -> str: """Agent-narrated free text can contain a secret the LLM read aloud or personal - data it narrated, so it gets the ``$mcp_intent`` treatment: sanitize, strip - structured PII, then bound the length. The event pipeline does not process + data it narrated, so it gets exactly the ``$mcp_intent`` pass + (``sanitize_free_text``: credentials -> structured PII -> URLs — the order is + load-bearing, the URL rewrite would percent-encode the ``@`` the email pattern + anchors on), then a length bound. The event pipeline does not process ``event["properties"]``, so this happens here.""" - return _truncate_feedback_text( - redact_pii(sanitize_captured_value(value)), _MAX_FEEDBACK_TEXT_LENGTH - ) + return _truncate_feedback_text(sanitize_free_text(value), _MAX_FEEDBACK_TEXT_LENGTH) def _capture_extra_value(value: Any) -> Any: - """A declared extra is agent-supplied like the core free-text fields, so it - gets the same treatment. Non-scalars are JSON-stringified first so the - redaction sees the full text; unserializable values are dropped.""" - sanitized = sanitize_captured_value(value) + """A declared extra is agent-supplied like the core free-text fields, so its + string leaves get the same free-text pass (with the key-based redaction + ``sanitize_free_text_value`` keeps for nested objects), then non-scalars are + JSON-stringified and everything is bounded. Unserializable values are dropped.""" + sanitized = sanitize_free_text_value(value) if isinstance(sanitized, str): - return _capture_free_text(sanitized) + return _truncate_feedback_text(sanitized, _MAX_FEEDBACK_TEXT_LENGTH) if sanitized is None or isinstance(sanitized, (bool, int, float)): return sanitized try: - return _capture_free_text(json.dumps(sanitized)) + return _truncate_feedback_text(json.dumps(sanitized), _MAX_FEEDBACK_TEXT_LENGTH) except Exception: # noqa: BLE001 - capture must never raise into the tool path return None @@ -306,10 +307,10 @@ def build_feedback_event_properties(report: FeedbackReport) -> JsonRecord: ) if report.tool_name: # Nominally an identifier, but the schema can't stop an agent from - # writing prose into it — so it gets the same PII redaction as the - # other free text. + # writing prose into it — so it gets the same free-text pass as the + # other fields. properties[PostHogMCPAnalyticsProperty.FEEDBACK_TOOL] = _truncate_feedback_text( - redact_pii(sanitize_captured_value(report.tool_name)), + sanitize_free_text(report.tool_name), _MAX_FEEDBACK_TOOL_NAME_LENGTH, ) if report.task_completed is not None: diff --git a/posthog/test/mcp/test_feedback.py b/posthog/test/mcp/test_feedback.py index f3d364571..994fb060d 100644 --- a/posthog/test/mcp/test_feedback.py +++ b/posthog/test/mcp/test_feedback.py @@ -312,20 +312,55 @@ def test_tool_name_gets_pii_redaction(): assert props["$mcp_feedback_tool"] == "ask [redacted]" -def test_pii_inside_urls_is_redacted(): - # Guards the ordering of sanitize vs redact: a sanitizer that re-serializes - # URLs (percent-encoding "@") before redaction would hide the email from the - # redaction pattern (the TS SDK's veria finding on this feature). +def test_pii_inside_urls_is_redacted_on_every_surface(): + # Guards the ordering of the free-text pass: running the URL rewrite before + # PII redaction percent-encodes the "@" the email pattern anchors on, letting + # the email through (the TS SDK's veria finding on this feature). + url = "https://example.com/?email=jane@example.com" + options = CollectFeedbackOptions(extra_properties={"area": {"type": "string"}}) report = parse_feedback_report( { "feedback_type": "issue", - "summary": "Login fails at https://example.com/?email=jane@example.com", - } + "summary": f"Login fails at {url}", + "details": f"see {url} too", + "friction_points": f"url {url} slow", + "suggested_improvement": f"fix {url}", + "tool_name": url, + "area": url, + }, + options, + ) + props = build_feedback_event_properties(report) + for key in ( + "$mcp_feedback_summary", + "$mcp_feedback_details", + "$mcp_feedback_friction_points", + "$mcp_feedback_suggested_improvement", + "$mcp_feedback_tool", + "$mcp_feedback_area", + ): + assert "jane@example.com" not in props[key], key + assert "jane%40example.com" not in props[key], key + assert "[redacted]" in props[key], key + + +def test_nested_extras_keep_key_based_redaction(): + # The feedback path walks extras with the free-text pass; nothing else + # asserts that credential-named keys inside a nested extra still redact by + # key name, so a stringify-first refactor could drop that protection silently. + options = CollectFeedbackOptions(extra_properties={"meta": {"type": "object"}}) + report = parse_feedback_report( + { + "feedback_type": "issue", + "summary": "s", + "meta": {"note": "ping jane@example.com", "password": "hunter2"}, + }, + options, ) props = build_feedback_event_properties(report) - assert "jane@example.com" not in props["$mcp_feedback_summary"] - assert "jane%40example.com" not in props["$mcp_feedback_summary"] - assert "[redacted]" in props["$mcp_feedback_summary"] + assert '"password": "[redacted]"' in props["$mcp_feedback_meta"] + assert "hunter2" not in props["$mcp_feedback_meta"] + assert '"note": "ping [redacted]"' in props["$mcp_feedback_meta"] def test_intent_joins_summary_and_details(): From 5e9ed768643f7d66b2dbf3511f862f3f0a9dd426 Mon Sep 17 00:00:00 2001 From: Georgis Andonis Date: Fri, 11 Sep 2026 12:06:41 +0300 Subject: [PATCH 5/8] refactor: apply simplify pass Compute _is_sdk_virtual_tool once per mutate_tool_schema call instead of twice per tool per tools/list dispatch. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 --- posthog/mcp/_instrumentation.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/posthog/mcp/_instrumentation.py b/posthog/mcp/_instrumentation.py index 2b4fdfb7e..31f7c22ef 100644 --- a/posthog/mcp/_instrumentation.py +++ b/posthog/mcp/_instrumentation.py @@ -787,8 +787,9 @@ def mutate_tool_schema( """ schema = getattr(tool, schema_attribute, None) original_schema = schema + is_sdk_virtual_tool = _is_sdk_virtual_tool(data, tool.name) if ( - not _is_sdk_virtual_tool(data, tool.name) + not is_sdk_virtual_tool and is_context_enabled(data.options.context) and not owns_context ): @@ -814,7 +815,7 @@ def mutate_tool_schema( not app_owns_model and schema_has_param(schema, "llm_model") ) if ( - not _is_sdk_virtual_tool(data, tool.name) + not is_sdk_virtual_tool and data.options.enable_conversation_id and not schema_has_param(schema, "conversation_id") ): From 899a219642484ef3e94b83b1859f1d07f60f8874 Mon Sep 17 00:00:00 2001 From: Georgis Andonis Date: Fri, 11 Sep 2026 12:20:42 +0300 Subject: [PATCH 6/8] fix(mcp): plug feedback-tool-shadow and error-leak gaps from review - _matches_extra_schema: a declared "integer" extra accepted any float, including fractional ones (3.5), since Python's numeric tower conflates int and float; now requires the value to be whole. - handle_feedback: an on_feedback exception was logged with str(error) verbatim, letting agent-controlled report text (PII, credentials, log-forging newlines) an error message echoes reach host logs; now logs only the exception type, matching the report log beside it. - start_tool_call_lifecycle: conversation-id resolution skipped every call named like the feedback tool regardless of the listing-derived shadow flag, so a real tool that collided with the configured feedback name never got a conversation id even once ownership was known; now mirrors ToolCallLifecycle.is_feedback's fail-open guard. Addresses greptile-apps findings on PR #939. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 --- posthog/mcp/_instrumentation.py | 6 ++- posthog/mcp/feedback.py | 19 +++++++-- posthog/test/mcp/test_feedback.py | 58 +++++++++++++++++++++++++++ posthog/test/mcp/test_v2_mcpserver.py | 27 +++++++++++++ 4 files changed, 106 insertions(+), 4 deletions(-) diff --git a/posthog/mcp/_instrumentation.py b/posthog/mcp/_instrumentation.py index 31f7c22ef..876278e6e 100644 --- a/posthog/mcp/_instrumentation.py +++ b/posthog/mcp/_instrumentation.py @@ -553,7 +553,11 @@ def start_tool_call_lifecycle( feedback_options = resolve_collect_feedback_options(data.options.collect_feedback) feedback_name = ( resolve_send_feedback_tool_name(feedback_options) - if feedback_options is not None + # Mirrors `ToolCallLifecycle.is_feedback`'s fail-open guard below: once a + # real application tool is known to own this name, conversation-id + # resolution must treat calls to it like any other tool too, not skip + # them as if they were the (shadowed) virtual feedback tool. + if feedback_options is not None and not data.feedback_tool_shadowed else None ) conversation_id, minted = resolve_conversation_id( diff --git a/posthog/mcp/feedback.py b/posthog/mcp/feedback.py index 93297edfb..fcba7fad1 100644 --- a/posthog/mcp/feedback.py +++ b/posthog/mcp/feedback.py @@ -207,7 +207,15 @@ def _matches_extra_schema(value: Any, schema: Dict[str, Any]) -> bool: else: actual = "object" declared = schema.get("type") - if declared != actual and not (declared == "integer" and actual == "number"): + if declared == "integer" and actual == "number": + # A JSON Schema `integer` also accepts a whole-valued float (`3.0`), but + # not a fractional one (`3.5`) - Python's numeric tower conflates int and + # float here, so the type name alone can't tell them apart. + if not ( + isinstance(value, int) or (isinstance(value, float) and value.is_integer()) + ): + return False + elif declared != actual: return False enum = schema.get("enum") return not isinstance(enum, list) or value in enum @@ -352,8 +360,13 @@ async def handle_feedback( if isinstance(reply, str) and reply.strip(): return reply except Exception as error: # noqa: BLE001 - never break the agent's turn + # Only the exception's type, matching the report log above: a + # handler can echo the unsanitized report (PII, credentials, + # log-forging newlines, unbounded length) into its error message, + # and that agent-controlled text does not belong in host logs + # any more than `report.summary` does. log( - "Warning: on_feedback handler threw; returning the default " - f"acknowledgement - {error}" + "Warning: on_feedback handler threw " + f"({type(error).__name__}); returning the default acknowledgement" ) return _SEND_FEEDBACK_RESULT_TEXT diff --git a/posthog/test/mcp/test_feedback.py b/posthog/test/mcp/test_feedback.py index 994fb060d..0cf32965e 100644 --- a/posthog/test/mcp/test_feedback.py +++ b/posthog/test/mcp/test_feedback.py @@ -304,6 +304,28 @@ def test_extras_must_match_declared_type_and_enum(): assert conforming.extras == {"score": 9, "channel": "web"} +def test_extras_declared_integer_rejects_fractional_float(): + # `isinstance(value, (int, float))` alone can't tell `3` from `3.5` - both + # are Python `number`s - so a declared `integer` extra must additionally + # check the value has no fractional part before it's trusted downstream. + options = CollectFeedbackOptions(extra_properties={"score": {"type": "integer"}}) + + fractional = parse_feedback_report( + {"feedback_type": "praise", "summary": "s", "score": 3.5}, options + ) + assert fractional.extras == {} + + whole_float = parse_feedback_report( + {"feedback_type": "praise", "summary": "s", "score": 3.0}, options + ) + assert whole_float.extras == {"score": 3.0} + + whole_int = parse_feedback_report( + {"feedback_type": "praise", "summary": "s", "score": 3}, options + ) + assert whole_int.extras == {"score": 3} + + def test_tool_name_gets_pii_redaction(): report = parse_feedback_report( {"feedback_type": "issue", "summary": "s", "tool_name": "ask jane@example.com"} @@ -588,6 +610,42 @@ def on_feedback(report): assert _events(client, "$mcp_feedback") +async def test_on_feedback_raise_does_not_log_agent_text(): + # A raising backend can echo the unsanitized report (PII, credentials, + # forged newlines) into its exception message; the warning log must not + # repeat it, mirroring the report log just above it that logs only the type. + from posthog.mcp import set_logger + + server = make_fastmcp() + client = FakeClient() + secret_summary = "credit card 4242-4242-4242-4242 jane@example.com" + + def on_feedback(report): + raise RuntimeError(f"backend rejected: {report.summary}") + + instrument( + server, + client, + MCPAnalyticsOptions( + collect_feedback=CollectFeedbackOptions(on_feedback=on_feedback) + ), + ) + + messages = [] + set_logger(messages.append) + try: + canned = await server._tool_manager.call_tool( + "send_feedback", {**dict(_REPORT_ARGS), "summary": secret_summary} + ) + await _flush() + finally: + set_logger(None) + + assert canned[0].text == send_feedback_result_text() + assert any("on_feedback handler threw" in message for message in messages) + assert not any(secret_summary in message for message in messages) + + # --- instrument(): low-level v1 ---------------------------------------------------- diff --git a/posthog/test/mcp/test_v2_mcpserver.py b/posthog/test/mcp/test_v2_mcpserver.py index e8021c787..80ae82ce6 100644 --- a/posthog/test/mcp/test_v2_mcpserver.py +++ b/posthog/test/mcp/test_v2_mcpserver.py @@ -429,6 +429,33 @@ def send_feedback(note: str) -> str: assert _events(client, "$mcp_tool_call") +async def test_collect_feedback_collision_keeps_conversation_id(): + # The name collision must fail open for every feature keyed off the + # feedback tool name, not just dispatch - conversation-id resolution used + # to keep skipping the real tool because it checked the configured name + # alone, ignoring the listing-derived shadow flag. + server = make_server() + + @server.tool() + def send_feedback(note: str) -> str: + return f"real tool got {note}" + + client = FakeClient() + instrument( + server, + client, + MCPAnalyticsOptions(collect_feedback=True, enable_conversation_id=True), + ) + + await _list_tools(server) + await _call_tool(server, "send_feedback", {"note": "hi", "context": "real tool"}) + await _flush() + + calls = _events(client, "$mcp_tool_call") + assert len(calls) == 1 + assert calls[0]["properties"].get("$mcp_conversation_id") + + async def test_instrument_is_idempotent(): server = make_server() client = FakeClient() From bd25a87434b190e622babf8ccf7369a5d20f8665 Mon Sep 17 00:00:00 2001 From: Georgis Andonis Date: Fri, 11 Sep 2026 12:48:48 +0300 Subject: [PATCH 7/8] fix(mcp): let original_tool disambiguate a feedback-name collision in prepare_tool_call On the custom-dispatcher path prepare_tool_list skips injecting the virtual tool when a real tool owns the feedback name, but prepare_tool_call still flagged every call by that name as feedback, so the documented dispatch flow suppressed the real tool. A host-supplied original_tool is stateless proof a real tool owns the name (the virtual tool never exists in the host's own list), so it now wins. Without original_tool the name match stands and the documented remedy is a non-colliding tool_name. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 --- posthog/mcp/posthog_mcp.py | 16 ++++++++++++++-- posthog/test/mcp/test_feedback.py | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/posthog/mcp/posthog_mcp.py b/posthog/mcp/posthog_mcp.py index c16879097..614b96157 100644 --- a/posthog/mcp/posthog_mcp.py +++ b/posthog/mcp/posthog_mcp.py @@ -400,7 +400,12 @@ def prepare_tool_call( ``send_feedback`` virtual tools (the latter only with the constructor's ``collect_feedback`` opt-in, so a real tool by that name is never shadowed). When model capture is enabled, resolve its value and source and - strip the SDK-owned ``llm_model`` argument before dispatch.""" + strip the SDK-owned ``llm_model`` argument before dispatch. + + ``original_tool`` is the application's own tool for ``name``, from the + host's un-prepared list (the virtual tools never exist there). Passing it + also disambiguates a name collision: a real tool by the feedback tool's + name is dispatched normally instead of being flagged as feedback.""" raw_context = (args or {}).get("context") intent = ( raw_context.strip() @@ -423,8 +428,15 @@ def prepare_tool_call( prepared_args = _strip_context(args) if analytics_owns_model: prepared_args = _strip_model(prepared_args) + # A supplied `original_tool` is a real application tool by this name (it + # comes from the host's own list, which never holds the virtual tool), so + # the real tool wins — the stateless twin of instrument()'s listing-derived + # shadow flag. Without it the name match stands, and the documented remedy + # for a collision is configuring a non-colliding `tool_name`. is_feedback = ( - self._collect_feedback is not None and name == self._feedback_tool_name + self._collect_feedback is not None + and name == self._feedback_tool_name + and original_tool is None ) return PreparedToolCall( args=prepared_args, diff --git a/posthog/test/mcp/test_feedback.py b/posthog/test/mcp/test_feedback.py index 0cf32965e..d3da39add 100644 --- a/posthog/test/mcp/test_feedback.py +++ b/posthog/test/mcp/test_feedback.py @@ -796,6 +796,26 @@ async def test_posthogmcp_prepare_tool_call_without_opt_in_never_flags(): assert call.is_feedback is False and call.feedback_report is None +async def test_posthogmcp_original_tool_wins_name_collision(): + # A host whose own list holds a real `send_feedback` tool passes it as + # `original_tool`; the call then dispatches as a real tool call instead of + # being swallowed as feedback — the stateless twin of instrument()'s + # listing-derived shadow flag. + client, _ = make_client(collect_feedback=True) + real_tool = { + "name": "send_feedback", + "inputSchema": {"type": "object", "properties": {"note": {"type": "string"}}}, + } + call = client.prepare_tool_call( + "send_feedback", {"note": "hi"}, original_tool=real_tool + ) + assert call.is_feedback is False and call.feedback_report is None + + # Without `original_tool` the name match stands (TS parity). + virtual = client.prepare_tool_call("send_feedback", dict(_REPORT_ARGS)) + assert virtual.is_feedback is True and virtual.feedback_report is not None + + async def test_posthogmcp_capture_feedback_event_shape(): client, captured = make_client(collect_feedback=True) call = client.prepare_tool_call("send_feedback", dict(_REPORT_ARGS)) From 052e85543de48ae3684d3b1d3b502601fe13768f Mon Sep 17 00:00:00 2001 From: Georgis Andonis Date: Fri, 11 Sep 2026 17:56:54 +0300 Subject: [PATCH 8/8] fix(mcp): keep a feedback-name collision across paginated tools/list pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each page of a paginated listing recomputed feedback_tool_shadowed from that page's tools alone, so a real send_feedback tool listed on page 1 was forgotten by page 2 — the SDK appended its virtual tool and then swallowed the real tool's calls. The flag is now sticky for the instrumentation instance's lifetime, and the virtual tool is appended only to the final page (no nextCursor), so an early page can't advertise it before a later page reveals the real tool. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 --- posthog/mcp/_instrument_fastmcp.py | 3 +- posthog/mcp/_instrument_lowlevel.py | 3 +- posthog/mcp/_instrument_v2.py | 3 +- posthog/mcp/_instrumentation.py | 37 +++++++++---- posthog/test/mcp/test_feedback.py | 80 +++++++++++++++++++++++++++++ 5 files changed, 113 insertions(+), 13 deletions(-) diff --git a/posthog/mcp/_instrument_fastmcp.py b/posthog/mcp/_instrument_fastmcp.py index 6fcce564f..61d33c27a 100644 --- a/posthog/mcp/_instrument_fastmcp.py +++ b/posthog/mcp/_instrument_fastmcp.py @@ -34,6 +34,7 @@ append_send_feedback, collect_listed_tools, extract_tools, + listing_has_next_page, mutate_tool_schema, refresh_feedback_shadow, request_to_dict, @@ -281,7 +282,7 @@ async def list_handler(req: Any) -> Any: append_get_more_tools(result, missing_name, data) names.append(missing_name) - if feedback_name is not None: + if feedback_name is not None and not listing_has_next_page(result): append_send_feedback(result, data) names.append(feedback_name) diff --git a/posthog/mcp/_instrument_lowlevel.py b/posthog/mcp/_instrument_lowlevel.py index d916ff35d..786597930 100644 --- a/posthog/mcp/_instrument_lowlevel.py +++ b/posthog/mcp/_instrument_lowlevel.py @@ -29,6 +29,7 @@ append_send_feedback, collect_listed_tools, extract_tools, + listing_has_next_page, mutate_tool_schema, prepare_request, record_resource_request, @@ -410,7 +411,7 @@ async def handler(req: Any) -> Any: append_get_more_tools(result, missing_name, data) names.append(missing_name) - if feedback_name is not None: + if feedback_name is not None and not listing_has_next_page(result): append_send_feedback(result, data) names.append(feedback_name) diff --git a/posthog/mcp/_instrument_v2.py b/posthog/mcp/_instrument_v2.py index 9f8705019..a523a8788 100644 --- a/posthog/mcp/_instrument_v2.py +++ b/posthog/mcp/_instrument_v2.py @@ -41,6 +41,7 @@ from ._instrumentation import ( _to_jsonable, collect_listed_tools, + listing_has_next_page, mutate_tool_schema, params_to_request_dict, prepare_request, @@ -633,7 +634,7 @@ async def handler(ctx: Any, params: Any) -> Any: _append_get_more_tools_v2(result, missing_name, data) names.append(missing_name) - if feedback_name is not None: + if feedback_name is not None and not listing_has_next_page(result): _append_send_feedback_v2(result, data) names.append(feedback_name) diff --git a/posthog/mcp/_instrumentation.py b/posthog/mcp/_instrumentation.py index 876278e6e..4a01bcf2c 100644 --- a/posthog/mcp/_instrumentation.py +++ b/posthog/mcp/_instrumentation.py @@ -690,22 +690,39 @@ def append_get_more_tools(result: Any, name: str, data: MCPAnalyticsData) -> Non def refresh_feedback_shadow(data: MCPAnalyticsData, tools: list) -> Optional[str]: """Refresh the collision flag from this listing's tools. Returns the resolved - feedback tool name when the virtual tool should be appended, ``None`` when the + feedback tool name when the virtual tool may be appended, ``None`` when the feature is off or a real application tool owns the name (fail-open: the real tool is advertised and dispatched untouched). Run before the schema-injection - pass so it reads the fresh flag.""" + pass so it reads the fresh flag. + + Sticky for the instrumentation instance's lifetime: a paginated ``tools/list`` + delivers one page per request, so a collision seen on an earlier page must + survive a later page that doesn't list the real tool — otherwise that page + would re-arm interception and swallow the real tool's calls. The trade-off is + deliberate: un-shadowing after the host removes the real tool requires + re-instrumentation.""" options = resolve_collect_feedback_options(data.options.collect_feedback) if options is None: return None name = resolve_send_feedback_tool_name(options) - shadowed = any(getattr(tool, "name", None) == name for tool in tools) - if shadowed: - log( - f'Warning: Cannot inject agent-feedback tool "{name}" because a real tool ' - "already uses that name. The real tool will not be intercepted." - ) - data.feedback_tool_shadowed = shadowed - return None if shadowed else name + if any(getattr(tool, "name", None) == name for tool in tools): + if not data.feedback_tool_shadowed: + log( + f'Warning: Cannot inject agent-feedback tool "{name}" because a real tool ' + "already uses that name. The real tool will not be intercepted." + ) + data.feedback_tool_shadowed = True + return None if data.feedback_tool_shadowed else name + + +def listing_has_next_page(result: Any) -> bool: + """Whether this ``tools/list`` result is a non-final page of a paginated + listing. The virtual feedback tool is only appended to the final page: an + earlier page could advertise it before a later page reveals a real tool by + the same name. Reads both SDK majors' cursor spelling (1.x models expose + ``nextCursor``, 2.x ``next_cursor``).""" + root = getattr(result, "root", result) + return bool(getattr(root, "nextCursor", None) or getattr(root, "next_cursor", None)) def append_send_feedback(result: Any, data: MCPAnalyticsData) -> None: diff --git a/posthog/test/mcp/test_feedback.py b/posthog/test/mcp/test_feedback.py index d3da39add..0e9c02ae1 100644 --- a/posthog/test/mcp/test_feedback.py +++ b/posthog/test/mcp/test_feedback.py @@ -704,6 +704,86 @@ async def call_tool(name, arguments): assert _events(client, "$mcp_tool_call") +def _make_paged_lowlevel(pages): + """A raw low-level server whose tools/list handler serves ``pages`` (a list of + tool lists) one page per request, chained by ``nextCursor``. The paged handler + is registered directly into ``request_handlers`` so the wire pagination shape + is exact; the real ``send_feedback`` handler answers ``real tool ran``.""" + server = Server("feedback-lowlevel-paged") + + @server.call_tool() + async def call_tool(name, arguments): + return [mcp_types.TextContent(type="text", text="real tool ran")] + + async def paged_list(req): + cursor = getattr(getattr(req, "params", None), "cursor", None) if req else None + index = int(cursor) if cursor else 0 + next_cursor = str(index + 1) if index + 1 < len(pages) else None + return mcp_types.ServerResult( + mcp_types.ListToolsResult(tools=list(pages[index]), nextCursor=next_cursor) + ) + + server.request_handlers[mcp_types.ListToolsRequest] = paged_list + return server + + +def _list_page(server, cursor=None): + handler = server.request_handlers[mcp_types.ListToolsRequest] + params = mcp_types.PaginatedRequestParams(cursor=cursor) if cursor else None + return handler(mcp_types.ListToolsRequest(method="tools/list", params=params)) + + +_REAL_SEND_FEEDBACK = mcp_types.Tool( + name="send_feedback", + description="A real application tool", + inputSchema={"type": "object", "properties": {"note": {"type": "string"}}}, +) +_ECHO_TOOL = mcp_types.Tool( + name="echo", + description="Echo", + inputSchema={"type": "object", "properties": {"msg": {"type": "string"}}}, +) + + +@pytest.mark.parametrize("real_tool_page", [0, 1]) +async def test_paginated_listing_keeps_collision_across_pages(real_tool_page): + # A collision seen on any page must survive the other pages: recomputing the + # flag from one page alone would re-arm interception and swallow the real + # tool's calls (and an early page must not advertise the virtual tool before + # a later page reveals the real one). + pages = [[_ECHO_TOOL], [_ECHO_TOOL]] + pages[real_tool_page] = [_REAL_SEND_FEEDBACK] + server = _make_paged_lowlevel(pages) + client = FakeClient() + instrument(server, client, MCPAnalyticsOptions(collect_feedback=True)) + + page_one = await _list_page(server) + page_two = await _list_page(server, cursor="1") + listed = [t.name for t in page_one.root.tools] + [ + t.name for t in page_two.root.tools + ] + assert listed.count("send_feedback") == 1 # the real tool only, never appended + + call_handler = server.request_handlers[mcp_types.CallToolRequest] + out = await call_handler(_call_request("send_feedback", {"note": "hi"})) + await _flush() + + assert out.root.content[0].text == "real tool ran" + assert _events(client, "$mcp_feedback") == [] + assert _events(client, "$mcp_tool_call") + + +async def test_paginated_listing_appends_virtual_tool_once_on_final_page(): + server = _make_paged_lowlevel([[_ECHO_TOOL], [_ECHO_TOOL]]) + client = FakeClient() + instrument(server, client, MCPAnalyticsOptions(collect_feedback=True)) + + page_one = await _list_page(server) + page_two = await _list_page(server, cursor="1") + assert [t.name for t in page_one.root.tools] == ["echo"] # non-final: no append + assert [t.name for t in page_two.root.tools] == ["echo", "send_feedback"] + + async def test_feedback_never_mints_conversation_id(): server = make_lowlevel() client = FakeClient()