fix(mcp): keep only the response envelope on $mcp_tools_list - #994
Merged
Merged
Conversation
Every tools/list result (all descriptors, input schemas, injected virtual tools) was serialized into $mcp_response, then sanitized and truncated on the request path. On large catalogues it exceeded MAX_EVENT_BYTES and re-measured through the truncation ladder. Nothing reads the list from $mcp_response, and $mcp_listed_tool_names already carries every name. Mirrors PostHog/posthog-js#5160. tools_list_envelope() dumps only the fields the handler set, minus tools (exclude_unset, exclude_none, by_alias), for the 1.x ServerResult root and the 2.x result. The lowlevel, FastMCP and v2 adapters use it. An empty envelope sets no $mcp_response. The client reply, $mcp_listed_tool_names and PostHogMCP.capture_tools_list are unchanged. Tested: pytest posthog/test/mcp on mcp 1.30.0 (561 passed, 1 skipped) and mcp 2.2.0 (451 passed, 21 skipped); ruff 0.11.12 check and format clean.
Contributor
posthog-python Compliance ReportDate: 2026-09-30T17:56:05.488488+00:00 ✅ All Tests Passed!121/121 tests passed Capture_V1 Tests✅ 95/95 tests passed View Details
Capture_Ai Tests✅ 5/5 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
Feature_Flags_Local_Evaluation Tests✅ 4/4 tests passed View Details
|
Contributor
|
[Medium risk] Changes how tool listing events capture response data. The PR appears safe to merge; the outstanding test-coverage gap is non-blocking. Reviews (2) · Last reviewed commit: "fix(mcp): build the tools/list envelope ..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review follow-up. The adapters built the envelope as a record_result argument, outside record_tools_list's try, and model_dump raised on a result that is not a model, failing the client's tools/list. ToolsListLifecycle.record_result now takes the raw result and record_tools_list shapes it inside the guard, so the rule lives in one place. tools_list_envelope returns None for non-models; the unused dict branch is gone. Renamed the tools/list duration test to match what it asserts. Tested: pytest posthog/test/mcp on mcp 1.30.0 (565 passed, 1 skipped) and 2.2.0 (455 passed, 21 skipped); ruff 0.11.12 clean. New tools_list_envelope rows for None and a bare list fail on the previous helper.
pauldambra
approved these changes
Sep 30, 2026
Review follow-up (Greptile). Adds a tools_list_envelope row asserting _meta keeps its wire alias alongside nextCursor. Tested: pytest posthog/test/mcp on mcp 1.30.0 (566 passed) and 2.2.0 (456 passed); ruff clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
$mcp_tools_listevents copied the wholetools/listresult into$mcp_response, including every tool descriptor with its full input schema. On a large catalogue that copy goes through sanitization and the truncation ladder on every list, and nothing in PostHog reads it.$mcp_listed_tool_namesalready carries every tool name.This mirrors PostHog/posthog-js#5160.
Scope
tools_list_envelope()inposthog/mcp/_instrumentation.pykeeps only the fields the handler set, minustools(nextCursor,ttlMs,cacheScope,_meta), for the 1.xServerResultroot and the 2.x result. It returnsNonefor anything that is not a model.ToolsListLifecycle.record_resulttakes the raw result, andrecord_tools_listbuilds the envelope inside its analytics guard. The lowlevel, FastMCP and v2 adapters passresult=result.$mcp_response.$mcp_listed_tool_names, and the manualPostHogMCP.capture_tools_listare unchanged.Tradeoffs
exclude_unsetdrops the 2.x SDK defaults (ttlMs: 0,cacheScope: "private",resultType: "complete") that the handler never set. Otherwise every 2.x event would carry them, and the JS SDK records only what the handler returned.Blast Radius
Only the
$mcp_responseproperty of$mcp_tools_listchanges. No PostHog query or insight reads the tool list from it. Building the envelope inside the guard means an odd handler result can no longer fail the client'stools/list.Verification
pytest posthog/test/mcpon mcp 1.30.0: 565 passed, 1 skipped.pytest posthog/test/mcpon mcp 2.2.0: 455 passed, 21 skipped.Noneand a bare list, which raised before.ruff0.11.12 check and format: clean.