Conversation
The SDK advertises two virtual tools into tools/list: get_more_tools (report_missing) and send_feedback (collect_feedback). A client concatenates every page into one list, so each may appear on exactly one page. get_more_tools had no page gate at all, so a cursor-following client saw it once per page. send_feedback went on the last page only, hiding it from every client that never follows nextCursor. Both now go on the first page -- the page every client reads -- matching @posthog/mcp. An empty-string cursor is a valid opaque cursor, so it reads as a continuation page rather than the first one. get_more_tools also had no collision handling anywhere: no warning, no shadow state, and unconditional interception on all four adapter paths, so a host tool named get_more_tools was silently swallowed and the agent got PostHog's canned reply. Both tools now share one kind-keyed resolver, so this cannot drift again: - A real tool owning the name on the first page wins: warn, skip injection, dispatch its calls normally. - A real tool appearing only on a later page is shadowed, since page one cannot see page two. Warn when that page is served. @posthog/mcp behaves the same way. - Ownership is also settled at call time, which covers a call reaching a process that never served a listing -- the ordinary multi-pod case. FastMCP and v2 MCPServer are asked via their tool registry; raw low-level servers have none, so the SDK asks the host's own tools/list handler, and only on a name match. - Configuring both virtual tools with one name is detected and warned about; missing-capability wins, as every call path already assumed. Warnings name the option that renames PostHog's tool and go to the posthog.mcp stdlib logger as well as the logger option, so a default-configured host actually sees them. Two places matched the missing-capability name literally rather than as configured: a renamed virtual tool picked up a conversation_id argument the default-named one never got, and a real tool named get_more_tools lost its context injection and its $mcp_intent. mutate_tool_schema now takes is_sdk_virtual_tool from its caller instead of guessing from the name. PostHogMCP.prepare_tool_call honours original_tool for the missing-capability tool as it already did for feedback. Ports PostHog/posthog-js#4953 and PostHog/posthog-js#4967. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
posthog-python Compliance ReportDate: 2026-09-16 14:57:24 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if report_missing: | ||
| name = self._missing_capability_tool_name | ||
| if any(_tool_name(t) == name for t in prepared): | ||
| self._warn_virtual_tool_collision( | ||
| VIRTUAL_TOOL_MISSING_CAPABILITY, | ||
| name, | ||
| 'PostHogMCP(missing_capability_tool_name="...")', | ||
| ) | ||
| else: | ||
| prepared.append(build_report_missing_descriptor(name)) | ||
| if collect_feedback and self._collect_feedback is not None: | ||
| name = self._feedback_tool_name | ||
| if any(_tool_name(t) == name for t in prepared): | ||
| self._warn_virtual_tool_collision( | ||
| VIRTUAL_TOOL_FEEDBACK, | ||
| name, | ||
| 'PostHogMCP(collect_feedback=CollectFeedbackOptions(tool_name="..."))', | ||
| ) | ||
| else: | ||
| prepared.append(get_feedback_tool_descriptor(self._collect_feedback)) |
There was a problem hiding this comment.
prepare_tool_list injects context before checking whether an application tool owns a virtual-tool name. _is_virtual_tool_name therefore mistakes a real get_more_tools or enabled send_feedback tool for an SDK descriptor and leaves its schema unchanged. The real tool wins dispatch, but the agent is not given the analytics context field, so its intent is lost. Determine ownership before schema mutation and skip context injection only for descriptors created by PostHog.
Knowledge Base Used: MCP framework instrumentation
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/mcp/posthog_mcp.py
Line: 398-417
Comment:
**Collisions lose context**
`prepare_tool_list` injects context before checking whether an application tool owns a virtual-tool name. `_is_virtual_tool_name` therefore mistakes a real `get_more_tools` or enabled `send_feedback` tool for an SDK descriptor and leaves its schema unchanged. The real tool wins dispatch, but the agent is not given the analytics `context` field, so its intent is lost. Determine ownership before schema mutation and skip context injection only for descriptors created by PostHog.
**Knowledge Base Used:** [MCP framework instrumentation](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-python/-/docs/mcp-framework-instrumentation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid — and pre-existing, _is_virtual_tool_name is untouched by this PR. But it's the PostHogMCP analogue of exactly the bug this PR fixes on the instrument() path, so leaving the two paths inconsistent made no sense. Fixed in 8b4b35b.
Reproduced: a host tool named get_more_tools came back with properties=[] while a sibling echo got properties=['context'], so its intent was silently dropped.
The name matched in both directions, though, which is why I didn't just drop the guard: a host may re-prepare an already-prepared list (the README says repeated preparation preserves ownership), which hands PostHog's own descriptor straight back. Removing the guard would double-inject into our descriptor and — once this PR added warnings — make us warn about ourselves.
So _is_sdk_virtual_tool(tool) now compares against the descriptor the SDK would build for that name. I tried your "descriptors declare context" suggestion first, but send_feedback doesn't — it states intent through summary/details instead, so that test injected context into our own feedback descriptor on re-prepare. Comparing the description works for both, and unlike the schema it survives the model-injection pass. The same test now also gates the collision warning in prepare_tool_list.
Tests: test_posthogmcp_real_tool_named_get_more_tools_keeps_context and test_posthogmcp_repreparing_a_prepared_list_is_not_a_collision, plus an added assertion on the existing collision test that the real tool keeps its context.
| async def probe_raw_tool_names(_ctx: Any = None) -> Optional[Set[str]]: | ||
| """The names the host's own handler advertises on its first page. Calls | ||
| ``original``, not the wrapper, so the probe never recurses into | ||
| instrumentation or appends a virtual tool. Read by | ||
| ``_name_owned_by_real_tool`` on raw low-level servers, which have no | ||
| tool registry to ask instead.""" | ||
| result = await original(mcp_types.ListToolsRequest(method="tools/list")) | ||
| return { | ||
| name | ||
| for tool in extract_tools(result) | ||
| if isinstance(name := getattr(tool, "name", None), str) | ||
| } | ||
|
|
||
| data.raw_tool_names_probe = probe_raw_tool_names |
There was a problem hiding this comment.
The raw-server ownership probe invokes the host's tools/list handler again during a matching tools/call, using synthesized first-page parameters; the v2 path does the same with the tool-call context. If that handler is stateful, permission-sensitive, expensive, or has side effects, the probe can advance state, repeat work, or fail. A failure is treated as “not owned,” causing a real colliding tool to be intercepted instead of dispatched. Use an explicit ownership registry or callback rather than executing the listing handler from the call path.
Knowledge Base Used: MCP framework instrumentation
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/mcp/_instrument_lowlevel.py
Line: 352-365
Comment:
**Tool calls rerun listings**
The raw-server ownership probe invokes the host's `tools/list` handler again during a matching `tools/call`, using synthesized first-page parameters; the v2 path does the same with the tool-call context. If that handler is stateful, permission-sensitive, expensive, or has side effects, the probe can advance state, repeat work, or fail. A failure is treated as “not owned,” causing a real colliding tool to be intercepted instead of dispatched. Use an explicit ownership registry or callback rather than executing the listing handler from the call path.
**Knowledge Base Used:** [MCP framework instrumentation](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-python/-/docs/mcp-framework-instrumentation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Partly valid, and addressed in 8b4b35b — though not by the route you suggested.
Frequency. Fair hit, and now largely gone. The probe only exists for the case where a call reaches a process that has served no listing of its own (the multi-pod case). Once a listing has been observed, virtual_tool_collisions already carries the answer, so raw_listing_owns_tool_name returns early on data.observed_listing and the host's handler is never re-entered. In practice that's at most one invocation per process rather than one per virtual-tool call. Covered by test_listing_stops_the_call_path_reprobing_the_host.
Contamination. The related half of this was a real bug, independently reported with a reproduction: the probe re-read a host result object the appends had mutated in place, so it saw PostHog's own tool as a host tool. Appends are now non-mutating — see the thread on @marandaneto's comment.
Explicit ownership registry/callback. Not taking this one. An ownership callback would be a new public option, and CONTRIBUTING requires a maintainer-agreed issue before changing the public surface. It also wouldn't help the case the probe exists for: a host that could supply a registry is a host that already has one, and those paths (FastMCP, v2 MCPServer) don't use the probe — they query the registry directly. Raw low-level servers have no registry by construction.
"A failure is treated as not owned." Accurate, and deliberate: falling through to the host's dispatcher on an undetermined answer would break an already-advertised virtual tool with an unknown-tool error. It's documented in the function and in the README, alongside the rename options. @posthog/mcp makes the same call.
There was a problem hiding this comment.
That addresses the concern. The probe is now bounded to the pre-listing window, and the test confirms that a successful listing prevents call-time re-probing. Copying the result before appending also removes the contamination risk. Given that raw low-level servers have no registry and fail-closed ownership is intentional to preserve already-advertised virtual tools, I don't have a remaining actionable issue here.
There was a problem hiding this comment.
Correcting one line in my earlier reply: I wrote that @posthog/mcp makes the same call on an undetermined probe. It does not — JS gates on isToolAdvertised(...) === false, so an undetermined answer there falls through to the host dispatcher, whereas this SDK treats it as "not owned" and intercepts. The choice here is still deliberate (falling through would hand an already-advertised virtual tool to an unknown-tool handler), but it is a divergence from JS rather than parity, and I should not have claimed otherwise.
There was a problem hiding this comment.
Follow-up: changed this to match JS in 42b58f9. The ownership check now returns a tri-state, and the call sites intercept only on a definite "host does not own it" — an unanswerable check delegates the call to the host and logs why, exactly as isToolAdvertised(...) === false does. Your original point about a failure being treated as "not owned" was right, and my defence of it was not.
There was a problem hiding this comment.
Thanks for the correction. The tri-state ownership result and intercept-only-on-definite-non-ownership behavior address the original concern: probe failures no longer cause a real host tool to be intercepted, while definite non-ownership still protects advertised virtual tools. Delegating indeterminate checks to the host and logging the reason also brings this in line with the JS behavior. I have no remaining actionable issue here.
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
Automated advisory code review.
| result = await original(mcp_types.ListToolsRequest(method="tools/list")) | ||
| return { | ||
| name | ||
| for tool in extract_tools(result) |
There was a problem hiding this comment.
blocking: Cached listings disable virtual-tool dispatch — When a raw server returns a cached ServerResult, the listing wrapper appends virtual tools directly to that object. This probe subsequently reads the same object and mistakes PostHog's injected send_feedback for a real application tool. Calls then reach the host's unknown-tool handler instead of recording feedback. Use an ownership view uncontaminated by SDK injection. Reproduction: reproduced — a focused uv run --no-sync pytest -q posthog/test/mcp/test_pr962_regression.py test returning one cached listing object, listing once, then calling send_feedback returns an unknown-tool error on this head and passes on the merge base.
There was a problem hiding this comment.
Valid, and reproduced — thanks for the bisect, that saved me a lot of time. Fixed in 8b4b35b.
Root cause was broader than the probe: the appends mutated the host's result object in place, so any later read of a reused object saw PostHog's virtual tool sitting in what looked like the host's catalogue. Your probe path hit it, and so did the low-level server's req is None cache-population pass.
Rather than teach each reader to subtract the SDK's own tools, append_virtual_tool now builds a copy and the adapters serve that. The host's object is never touched, so every read of a listing is a faithful view of what the host wrote. model_copy(update={"tools": ...}) carries nextCursor over, and handles both result shapes (1.x ServerResult root model, 2.x bare ListToolsResult).
I first tried excluding SDK-appended names from the readers, but that was too coarse — it also suppressed the legitimate later-page collision warning, since a real tool on page 2 shares the name with the one we appended to page 1. Copying has no such ambiguity.
Regression tests: test_cached_result_object_does_not_collide_with_itself and test_cached_result_object_is_not_appended_to_twice (v1), plus test_v2_cached_result_object_does_not_collide_with_itself, which covers the unwrapped v2 result shape and asserts the host's object is left untouched. The second listing was also affected — the false collision silently stopped advertising both virtual tools from then on.
Review found two ways the SDK mistook its own virtual tool for a host tool. A host may return the SAME tools/list result object from every request -- a module-level constant, or its own cache. The appends mutated that object in place, so the next read of it showed PostHog's virtual tool sitting in what looked like the host's catalogue. The SDK reported a collision against itself, stopped intercepting, and handed the agent "Unknown tool: send_feedback" instead of recording its feedback. Appends now build a copy and the adapters serve that, so every read of a listing is a faithful view of what the host wrote. Reported by @marandaneto, with a reproduction. On the PostHogMCP path, _is_virtual_tool_name matched on the name alone, in both directions: a host's own tool called get_more_tools was skipped for context injection and silently lost its $mcp_intent, while a host re-preparing an already-prepared list had PostHog's descriptor treated as a host collision -- warning about ourselves. Both now compare against the descriptor the SDK would build for that name; the description is ours and, unlike the schema, survives the model-injection pass. Flagged by Greptile. Also stop re-invoking the host's tools/list handler from the call path once a listing has been observed: its collision state already carries the answer. The probe now runs only while this process has served no listing -- the multi-pod case it exists for -- so a stateful or expensive handler is not re-entered on every virtual-tool call. Flagged by Greptile. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| only: a real tool that appears solely on a later page is already shadowed by | ||
| the page-one injection and cannot be recovered here.""" | ||
| probe = data.raw_tool_names_probe | ||
| if probe is None or data.observed_listing: |
There was a problem hiding this comment.
Low: Repeated listing-handler execution
data.observed_listing is initialized to False but never set to True, so this guard never activates. A remote MCP client can repeatedly call get_more_tools or send_feedback and force the host's original tools/list handler to execute each time, amplifying work for hosts that perform dynamic database, filesystem, or network discovery there. Set the flag after successfully processing the first listing page.
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[convergent: router + fable] 🟠 HIGH
Confirmed by reproduction on both SDK majors. observed_listing is declared (_internal.py:86) and read here, but never assigned True anywhere. On a raw low-level server, one listing plus two virtual-tool calls run the host's tools/list handler three times (1 -> 3). This contradicts this function's docstring, the README paragraph on call-time ownership, commit 8b4b35b's message, and the reply to greptile above.
The JS SDK also probes on every call, so the behaviour itself matches @posthog/mcp. Only the text and the test (test_listing_stops_the_call_path_reprobing_the_host, see the comment there) are wrong.
Two ways to make code and text agree:
- Set the flag in
refresh_virtual_tool_collisions. Small change, but then a raw low-level server that serves different catalogues per client uses one client's listing state for another, and the README sentence that call-time checks are "the reliable signal there" becomes false. - Remove the flag, the claim, and the test, and document that the probe runs on each call to a virtual-tool name. Matches JS, keeps the README true, costs one extra host list call per call to
get_more_tools/send_feedback.
There was a problem hiding this comment.
Correct, and my earlier reply claiming the guard worked was wrong — the flag was declared, read, and never assigned. Fixed in fd42c1e by removing it and keeping the per-call probe, which is the behaviour we actually want: virtual_tool_collisions is per-server state rewritten by whichever listing ran last, so trusting it would answer one caller from another catalogue.
There was a problem hiding this comment.
Confirmed — 1 listing + 2 calls hit the host handler 3 times, and my reply to greptile above was wrong. Took your second option in fd42c1e: flag, claim and test removed, README now states the real cost (one probe per call to a virtual-tool name, none for ordinary traffic) and offers renaming as the way out — the per-call probe is also what keeps the per-caller-tool-sets sentence true.
Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
PR overviewThis pull request changes MCP tool-list instrumentation so virtual tools are injected only once, on the first One low-impact issue remains: the first-page state is not recorded, allowing a remote MCP client to repeatedly trigger the host's original tool-listing handler through follow-up tool calls. This can amplify database, filesystem, or network discovery work performed by dynamic handlers, but its practical impact depends on the host implementation; one other issue has already been addressed. Open issues (1)
Fixed/addressed: 1 · PR risk: 4/10 |
Only the missing-capability name was matched literally; the feedback name already resolved correctly, so a renamed send_feedback was never affected. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
lucasheriques
left a comment
lucasheriques
left a comment
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
QA Swarm review complete — two inline findings plus a reply in the existing observed_listing thread. Verdict and evidence in the pinned summary comment.
|
|
||
| server.request_handlers[mcp_types.ListToolsRequest] = counting_list | ||
| await _call(server, "get_more_tools", {"context": "need csv"}) | ||
| assert calls == [] |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[router] 🟡 MEDIUM
This test cannot fail. probe_raw_tool_names captured the original handler function at wrap time and calls it directly (_instrument_lowlevel.py, await original(...)). The test replaces the entry in server.request_handlers, which the probe never reads, so calls stays empty whether or not the probe fires. A version that counts inside the host's own paged_list handler shows 1 invocation after the listing and 3 after two get_more_tools calls, because observed_listing is never set (see the thread at _instrumentation.py:888).
There was a problem hiding this comment.
Right — the probe closes over the original handler, so replacing the request_handlers entry observed nothing and the test could not fail. Replaced in fd42c1e with one that counts inside the host's own handler and pins the real cost; writing it also turned up a fourth invocation that is the MCP SDK repopulating its own validation cache on a call to an unlisted name, which the test now excludes rather than miscounting as ours.
| # every downstream decision treats calls to it like any other tool's: | ||
| # interception is skipped, and conversation-id resolution stops exempting it | ||
| # as if it were the (shadowed) virtual tool. | ||
| injectable = injectable_virtual_tool_names(data) |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[convergent: router + fable] 🟠 HIGH
start_tool_call_lifecycle reads the virtual-tool names from injectable_virtual_tool_names(data), which only consults virtual_tool_collisions. Only a served tools/list writes that set. The call-time registry and raw probes added in this PR (_name_owned_by_real_tool, _name_owned_by_real_tool_v2, raw_listing_owns_tool_name) correctly block interception, but never write to it, so resolve_conversation_id (here) and resolve_tool_call_intent (at capture time) still treat the name as the virtual tool.
Reproduced on FastMCP (mcp 1.30.0), MCPServer (mcp 2.2.0) and raw low-level v1: a real tool named get_more_tools on an instance that never served a listing runs correctly and captures no $mcp_missing_capability, but its $mcp_tool_call has $mcp_intent=None and no conversation id even with enable_conversation_id=True. On FastMCP and MCPServer this holds for every call for the life of the process. On raw low-level it self-heals after the first call only because the SDK's internal req is None cache pass happens to run refresh_virtual_tool_collisions during dispatch.
The changeset says a real get_more_tools "keeps its context injection and $mcp_intent", which is only true after a first-page listing on the same instance.
Suggested fix: in each adapter, run the ownership probe before building the lifecycle (it already runs only when the name matches), and when the host owns the name add that kind to data.virtual_tool_collisions. Everything downstream then agrees with the interception decision. The three "before any listing" tests (test_registry_probe_blocks_interception_before_any_listing, test_raw_list_probe_blocks_interception_before_any_listing, test_fastmcp_collision_fails_open_before_any_listing) currently assert only the dispatched text and the absence of the missing-capability event; adding assertions on $mcp_intent and conversation id would pin this.
There was a problem hiding this comment.
Reproduced and fixed in 8daec48 — thanks, this was the right catch. The guard turned out to be dead code for its stated purpose: PostHog's virtual tools are intercepted before dispatch and captured by record_missing_capability/record_feedback, so they never reach record_tool_call (verified by spying on it), meaning it could only ever fire on a host tool sharing the name. Removed it and switched start_tool_call_lifecycle to enabled_virtual_tool_names, so the call path no longer reads listing state at all; one correction to your report, though — the missing conversation id is not name-specific (an ordinary tool shows the same in that harness), so the new tests assert parity with an ordinary tool rather than a conversation id. Changeset line corrected too.
There was a problem hiding this comment.
Scope correction on my previous reply: the fix is real and stands, but it moved to the stacked follow-up (#963) rather than landing here. This PR is deliberately limited to the JS-parity behaviour — first-page injection plus collision warnings — and attribution ($mcp_intent, conversation id) is the follow-up's subject. Worth noting this PR does not regress intent: before it, a host tool named get_more_tools was swallowed entirely and emitted no $mcp_tool_call at all, so there was nothing to attribute. I have also narrowed this PR's changeset so it no longer claims the $mcp_intent half.
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated reviewers (qa-team, paul-reviewer, xp-reviewer, security-audit as warranted) Verdict:
|
| Reviewer | Assessment |
|---|---|
| 🧭 router | Danger HIGH, confidence HIGH. Traced all four adapters on list and call paths; delegated the v2 wire shape, output-instructions bookkeeping and thread safety to a general lens, all clean. |
| 🧪 fable (escalation) | Independently reproduced the dead flag, the vacuous test, and the intent/conversation-id loss on raw low-level v1 and FastMCP v1. |
Automated by QA Swarm — not a human review
`observed_listing` was declared and read but never assigned, so the guard never activated. My reply on #962 claimed otherwise -- the claim was wrong, not the code. Keep the per-call behaviour, which is the correct one, and fix the text instead. `virtual_tool_collisions` is per-server state rewritten by whichever listing ran last, so a raw server serving different catalogues to different callers would answer one caller from another's listing. Asking per call cannot go stale that way, and `@posthog/mcp` probes per call for the same reason. Gating on a first-listing flag would also have falsified the README sentence that the call-time checks are the reliable signal for per-caller tool sets. test_listing_stops_the_call_path_reprobing_the_host could not fail: the probe closes over the original handler at wrap time, so replacing the `request_handlers` entry observed nothing. Replaced with a test that counts inside the host's own handler and pins the actual cost -- one probe per call to a virtual-tool name, none for ordinary tool traffic. Writing it turned up a fourth invocation that is the MCP SDK's own `req is None` cache repopulation on a call to an unlisted name, so the test excludes that rather than miscounting it as ours. README now states the real cost and offers renaming as the way to avoid the check. Reported by veria-ai and QA Swarm on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
With enable_conversation_id on, get_more_tools and send_feedback now advertise conversation_id, echo a minted handle back over prompt-back, and stamp $mcp_conversation_id -- under any configured name. They were exempt, in three places that had to agree: the schema pass skipped the argument, resolve_conversation_id short-circuited on their names, and the lifecycle passed None when preparing their session. So a $mcp_feedback or $mcp_missing_capability event was filed under a $session_id of its own. An agent's complaint about a tool landed in a different session than the call it was complaining about, which is most of the value of the report, and the fallback session emitted a second spurious $mcp_initialize. resolve_conversation_id loses its two tool-name parameters: with no tool exempt there is nothing to compare against, so the exemption cannot come back by accident. The virtual tools still never get the injected `context` argument -- they state their intent through their own -- so `is_sdk_virtual_tool` now gates only that. Two tests are inverted on purpose, with the reasoning in their docstrings: test_feedback_never_mints_conversation_id becomes test_feedback_joins_the_conversation, and the renamed-tool schema case now expects the argument to track the option. Stacked on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
lucasheriques
left a comment
lucasheriques
left a comment
There was a problem hiding this comment.
Approving to unblock: fd42c1e closes the flag and the test. One thread is still open (_instrumentation.py:571, intent and conversation id for a real tool owning a virtual-tool name; Veria's newer comment there shares the root cause). Please either fix it or trim the changeset sentence that claims a real get_more_tools keeps its $mcp_intent before merging, since that only holds after a first-page listing on the same instance. Happy with a follow-up PR for the code change if you'd rather ship this one now.
Capture intent for tools the host owns; skip it only for PostHog's own. The code asked that question two different ways and they disagreed. Interception asked a per-request probe, which was right. Intent and the conversation exemption asked `virtual_tool_collisions`, which only a served tools/list writes. On a process that had served none -- the ordinary multi-pod case the probe exists for -- a host tool named `get_more_tools` dispatched correctly but its $mcp_tool_call carried `$mcp_intent=None`. Permanent on FastMCP and v2 MCPServer; on raw low-level it self-healed after one call, because the MCP SDK's own `req is None` cache pass happens to refresh the state during dispatch. The intent guard turns out to be dead code for its stated purpose: PostHog's virtual tools are intercepted before dispatch and captured by record_missing_capability / record_feedback, so they never reach record_tool_call. Verified by spying on it. Everything that gets there is host-dispatched by construction, so the guard could only ever fire on a host tool sharing the name. Removed. The same applies to the conversation exemption: the virtual tools' capture paths discard the resolved handle, so exempting by name only ever cost the host's tool its handle. start_tool_call_lifecycle now reads `enabled_virtual_tool_names` instead of `injectable_`, so the call path stops reading listing state altogether. That also closes the cross-client report: the collision set is per-server and rewritten by whichever listing ran last, so reading it on the call path let one caller's catalogue decide another caller's call. Note on the reports: the missing conversation id they also cite is not name-specific -- an ordinary tool shows the same in that harness, so it is not a regression here. The new tests assert parity with an ordinary tool instead, which is the rule being fixed. The changeset claimed a real `get_more_tools` "keeps its $mcp_intent", which was only true after a listing on the same instance. Corrected. Reported by QA Swarm and veria-ai on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
With enable_conversation_id on, get_more_tools and send_feedback now advertise conversation_id, echo a minted handle back over prompt-back, and stamp $mcp_conversation_id -- under any configured name. They were exempt, in three places that had to agree: the schema pass skipped the argument, resolve_conversation_id short-circuited on their names, and the lifecycle passed None when preparing their session. So a $mcp_feedback or $mcp_missing_capability event was filed under a $session_id of its own. An agent's complaint about a tool landed in a different session than the call it was complaining about, which is most of the value of the report, and the fallback session emitted a second spurious $mcp_initialize. resolve_conversation_id loses its two tool-name parameters: with no tool exempt there is nothing to compare against, so the exemption cannot come back by accident. The virtual tools still never get the injected `context` argument -- they state their intent through their own -- so `is_sdk_virtual_tool` now gates only that. Two tests are inverted on purpose, with the reasoning in their docstrings: test_feedback_never_mints_conversation_id becomes test_feedback_joins_the_conversation, and the renamed-tool schema case now expects the argument to track the option. Stacked on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
A real tool of yours named `get_more_tools` now gets `context` injected unconditionally, but whether its value is captured as `$mcp_intent` still depends on this instance having served a tools/list. That half belongs with the attribution work in the follow-up PR, so don't claim it here. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
8daec48 to
3aac206
Compare
Capture intent for tools the host owns; skip it only for PostHog's own. The code asked that question two different ways and they disagreed. Interception asked a per-request probe, which was right. Intent and the conversation exemption asked `virtual_tool_collisions`, which only a served tools/list writes. On a process that had served none -- the ordinary multi-pod case the probe exists for -- a host tool named `get_more_tools` dispatched correctly but its $mcp_tool_call carried `$mcp_intent=None`. Permanent on FastMCP and v2 MCPServer; on raw low-level it self-healed after one call, because the MCP SDK's own `req is None` cache pass happens to refresh the state during dispatch. The intent guard turns out to be dead code for its stated purpose: PostHog's virtual tools are intercepted before dispatch and captured by record_missing_capability / record_feedback, so they never reach record_tool_call. Verified by spying on it. Everything that gets there is host-dispatched by construction, so the guard could only ever fire on a host tool sharing the name. Removed. The same applies to the conversation exemption: the virtual tools' capture paths discard the resolved handle, so exempting by name only ever cost the host's tool its handle. start_tool_call_lifecycle now reads `enabled_virtual_tool_names` instead of `injectable_`, so the call path stops reading listing state altogether. That also closes the cross-client report: the collision set is per-server and rewritten by whichever listing ran last, so reading it on the call path let one caller's catalogue decide another caller's call. Note on the reports: the missing conversation id they also cite is not name-specific -- an ordinary tool shows the same in that harness, so it is not a regression here. The new tests assert parity with an ordinary tool instead, which is the rule being fixed. The changeset claimed a real `get_more_tools` "keeps its $mcp_intent", which was only true after a listing on the same instance. Corrected. Reported by QA Swarm and veria-ai on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
With enable_conversation_id on, get_more_tools and send_feedback now advertise conversation_id, echo a minted handle back over prompt-back, and stamp $mcp_conversation_id -- under any configured name. They were exempt, in three places that had to agree: the schema pass skipped the argument, resolve_conversation_id short-circuited on their names, and the lifecycle passed None when preparing their session. So a $mcp_feedback or $mcp_missing_capability event was filed under a $session_id of its own. An agent's complaint about a tool landed in a different session than the call it was complaining about, which is most of the value of the report, and the fallback session emitted a second spurious $mcp_initialize. resolve_conversation_id loses its two tool-name parameters: with no tool exempt there is nothing to compare against, so the exemption cannot come back by accident. The virtual tools still never get the injected `context` argument -- they state their intent through their own -- so `is_sdk_virtual_tool` now gates only that. Two tests are inverted on purpose, with the reasoning in their docstrings: test_feedback_never_mints_conversation_id becomes test_feedback_joins_the_conversation, and the renamed-tool schema case now expects the argument to track the option. Stacked on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
The per-request ownership fix landed here rather than in #962, so its user-facing effects belong in this changeset, not that one. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
The call-time ownership check has three outcomes, not two: the host owns the name, the host does not, or the question could not be asked -- a raw low-level server whose own tools/list handler is failing. The third collapsed into "does not own", so the SDK intercepted. That is the wrong direction. Guessing toward interception breaks the host's tool, silently, for as long as their handler stays unwell, to protect an analytics affordance. Guessing toward delegation costs one visible failed call to a tool of PostHog's, which nothing depends on. An analytics SDK does not get to break the product it measures, which is the rule the rest of this package already follows. `raw_listing_owns_tool_name` now returns Optional[bool], and the four call sites intercept only on a definite False. Registry lookups are unchanged: "not found" there is an answer, not a failure. The host gets a warning naming the tool and the reason. Also removes a divergence from @posthog/mcp, which gates on `isToolAdvertised(...) === false` and delegates for the same reason. My earlier reply on this PR claimed we already matched it; we did not. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
Capture intent for tools the host owns; skip it only for PostHog's own. The code asked that question two different ways and they disagreed. Interception asked a per-request probe, which was right. Intent and the conversation exemption asked `virtual_tool_collisions`, which only a served tools/list writes. On a process that had served none -- the ordinary multi-pod case the probe exists for -- a host tool named `get_more_tools` dispatched correctly but its $mcp_tool_call carried `$mcp_intent=None`. Permanent on FastMCP and v2 MCPServer; on raw low-level it self-healed after one call, because the MCP SDK's own `req is None` cache pass happens to refresh the state during dispatch. The intent guard turns out to be dead code for its stated purpose: PostHog's virtual tools are intercepted before dispatch and captured by record_missing_capability / record_feedback, so they never reach record_tool_call. Verified by spying on it. Everything that gets there is host-dispatched by construction, so the guard could only ever fire on a host tool sharing the name. Removed. The same applies to the conversation exemption: the virtual tools' capture paths discard the resolved handle, so exempting by name only ever cost the host's tool its handle. start_tool_call_lifecycle now reads `enabled_virtual_tool_names` instead of `injectable_`, so the call path stops reading listing state altogether. That also closes the cross-client report: the collision set is per-server and rewritten by whichever listing ran last, so reading it on the call path let one caller's catalogue decide another caller's call. Note on the reports: the missing conversation id they also cite is not name-specific -- an ordinary tool shows the same in that harness, so it is not a regression here. The new tests assert parity with an ordinary tool instead, which is the rule being fixed. The changeset claimed a real `get_more_tools` "keeps its $mcp_intent", which was only true after a listing on the same instance. Corrected. Reported by QA Swarm and veria-ai on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
With enable_conversation_id on, get_more_tools and send_feedback now advertise conversation_id, echo a minted handle back over prompt-back, and stamp $mcp_conversation_id -- under any configured name. They were exempt, in three places that had to agree: the schema pass skipped the argument, resolve_conversation_id short-circuited on their names, and the lifecycle passed None when preparing their session. So a $mcp_feedback or $mcp_missing_capability event was filed under a $session_id of its own. An agent's complaint about a tool landed in a different session than the call it was complaining about, which is most of the value of the report, and the fallback session emitted a second spurious $mcp_initialize. resolve_conversation_id loses its two tool-name parameters: with no tool exempt there is nothing to compare against, so the exemption cannot come back by accident. The virtual tools still never get the injected `context` argument -- they state their intent through their own -- so `is_sdk_virtual_tool` now gates only that. Two tests are inverted on purpose, with the reasoning in their docstrings: test_feedback_never_mints_conversation_id becomes test_feedback_joins_the_conversation, and the renamed-tool schema case now expects the argument to track the option. Stacked on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
The per-request ownership fix landed here rather than in #962, so its user-facing effects belong in this changeset, not that one. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
The changeset listed five bullets, two of them restating internals. Cut to the three things a host upgrading would notice, plus the listed_tool_names shift for anyone charting it. The README section repeated the warning texts the code already emits and the option snippet twice. Cut to the rules and their limits. Also one real inconsistency in the code: a page's tool names were gathered two ways, once through a helper and once inline, and the helper's name shadowed the unrelated $mcp_listed_tool_names event field. Now one helper, named advertised_tool_names. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
Capture intent for tools the host owns; skip it only for PostHog's own. The code asked that question two different ways and they disagreed. Interception asked a per-request probe, which was right. Intent and the conversation exemption asked `virtual_tool_collisions`, which only a served tools/list writes. On a process that had served none -- the ordinary multi-pod case the probe exists for -- a host tool named `get_more_tools` dispatched correctly but its $mcp_tool_call carried `$mcp_intent=None`. Permanent on FastMCP and v2 MCPServer; on raw low-level it self-healed after one call, because the MCP SDK's own `req is None` cache pass happens to refresh the state during dispatch. The intent guard turns out to be dead code for its stated purpose: PostHog's virtual tools are intercepted before dispatch and captured by record_missing_capability / record_feedback, so they never reach record_tool_call. Verified by spying on it. Everything that gets there is host-dispatched by construction, so the guard could only ever fire on a host tool sharing the name. Removed. The same applies to the conversation exemption: the virtual tools' capture paths discard the resolved handle, so exempting by name only ever cost the host's tool its handle. start_tool_call_lifecycle now reads `enabled_virtual_tool_names` instead of `injectable_`, so the call path stops reading listing state altogether. That also closes the cross-client report: the collision set is per-server and rewritten by whichever listing ran last, so reading it on the call path let one caller's catalogue decide another caller's call. Note on the reports: the missing conversation id they also cite is not name-specific -- an ordinary tool shows the same in that harness, so it is not a regression here. The new tests assert parity with an ordinary tool instead, which is the rule being fixed. The changeset claimed a real `get_more_tools` "keeps its $mcp_intent", which was only true after a listing on the same instance. Corrected. Reported by QA Swarm and veria-ai on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
With enable_conversation_id on, get_more_tools and send_feedback now advertise conversation_id, echo a minted handle back over prompt-back, and stamp $mcp_conversation_id -- under any configured name. They were exempt, in three places that had to agree: the schema pass skipped the argument, resolve_conversation_id short-circuited on their names, and the lifecycle passed None when preparing their session. So a $mcp_feedback or $mcp_missing_capability event was filed under a $session_id of its own. An agent's complaint about a tool landed in a different session than the call it was complaining about, which is most of the value of the report, and the fallback session emitted a second spurious $mcp_initialize. resolve_conversation_id loses its two tool-name parameters: with no tool exempt there is nothing to compare against, so the exemption cannot come back by accident. The virtual tools still never get the injected `context` argument -- they state their intent through their own -- so `is_sdk_virtual_tool` now gates only that. Two tests are inverted on purpose, with the reasoning in their docstrings: test_feedback_never_mints_conversation_id becomes test_feedback_joins_the_conversation, and the renamed-tool schema case now expects the argument to track the option. Stacked on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
The per-request ownership fix landed here rather than in #962, so its user-facing effects belong in this changeset, not that one. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
The call-time ownership probe added in this branch runs the host's own
tools/list handler. When that handler is registered through the SDK's
`@server.list_tools()` decorator, mcp 1.x rebuilds `Server._tool_cache`
from the tools it returns on every invocation - and the probe, unlike the
wrapper's `req is None` branch, never re-ran schema injection afterwards.
That cache is what the SDK validates real tool arguments against, so one
ordinary `get_more_tools` call left every later call to a real tool
rejected with "Additional properties are not allowed ('context' was
unexpected)" for sending the argument we advertised, until the next
client-facing listing healed it. An analytics SDK breaking the host's
tools is the failure this branch exists to prevent.
The probe also closed over the handler captured at instrument() time, so
a host registering tools/list afterwards had ownership answered from a
catalogue no client ever sees - a confident wrong answer that swallowed
their real tool. `@posthog/mcp` re-captures the handler for this reason;
read the current one instead and report the question as unanswerable when
it is no longer ours, so the call is delegated.
Both paths are covered by tests that fail without the fix.
Generated-By: PostHog Desktop
Task-Id: f66265f3-22a8-4eb5-9bf1-bf174c1c0c65
…d state `virtual_tool_collisions` existed to carry a listing's answer to the call path. It was not needed for either job. At listing time the page's own tools are already in hand, so the decision is local: on a first page inject the name or warn that a real tool has it, on a later page warn that ours already shadows theirs. Four branches, no state - the shape `@posthog/mcp` uses. At call time every site already gates on a stronger signal than the set ever was: the tool registry on FastMCP and v2 MCPServer, the listing probe on raw low-level servers. Those answer correctly in a process that never served a listing, which is exactly where the set was empty and so silently wrong - the multi-pod case it was meant to cover. Names now resolve through `enabled_virtual_tool_names` everywhere, which keeps the fix that matters: with `report_missing` off, a real tool called `get_more_tools` is an ordinary tool and keeps its `context` captured as intent. Attribution for a real tool that *collides* with a virtual tool's name - `$mcp_intent` and `$mcp_conversation_id` - is dropped here and belongs to the stacked follow-up, which can read it off the ownership answer the call path already computes rather than from state that goes stale. Generated-By: PostHog Desktop Task-Id: f66265f3-22a8-4eb5-9bf1-bf174c1c0c65
Three fixes from review of the two commits before this one. `_name_owned_by_real_tool`'s standalone-fastmcp branch caught every exception from `get_tool` and answered False - "no real tool owns this name" - on the assumption that an unknown name raises. In fastmcp 3.x it does not: `get_tool` returns None for an unknown name and raises only when the lookup itself fails, and its provider chain can reach a mounted or proxied upstream over the network. So a connection blip was read as "the name is free", and a call to a host tool that shares a virtual tool's name was swallowed and answered with PostHog's canned reply and isError=False - a fabricated success over a tool that never ran. Until the commit before this one a listing-derived collision flag made interception impossible for a name the host owned, which masked this. Removing that flag left the lookup deciding alone. Catch fastmcp's not-found and disabled errors as a real answer; delegate on anything else, which both call sites already do for None. The continuation-page "shadowed" warning no longer fires for a name a first page already blocked. Nothing of ours is advertised in that case and the host's tool runs, so telling them "the real tool will not run" sent them chasing a bug that is not there. The foreign-handler guard now covers a removed handler as well as a replaced one, and says so once. The virtual tools stay advertised when another layer wraps tools/list - a chained wrapper still runs our injection - but nothing behind them is ever intercepted, and a silent stop is invisible in the captured data. The README already promised this was logged. Also corrects a `raw_listing_owns_tool_name` docstring that described an approach never taken (reading `Server._tool_cache`), which would have invited removing the re-injection that repairs it, and sweeps five comments describing the deleted collision state. Generated-By: PostHog Desktop Task-Id: f66265f3-22a8-4eb5-9bf1-bf174c1c0c65
Three review fixes. The continuation-page "shadowed" warning skipped a name a first page had blocked, but not one the other virtual tool had won under the duplicate name check. Configure both tools to one name and a host tool by that name on a later page drew two warnings, one of them for a tool that was never advertised. Skip any kind that did not make it onto the first page, whichever way it lost. The foreign-handler warning covers a removed handler as well as a replaced one, so it says so, and both branches now have a test - the removed one had none, which is why the wording went unnoticed. Also corrects the comment on the ownership lookup's except branch. It claimed a raised exception meant a mounted or proxied upstream had blipped. It cannot: fastmcp gathers its providers with `return_exceptions=True` and drops the failures, so an upstream blip reads back as a plain None, indistinguishable from "no such tool", and resolves to False without reaching that branch. What does reach it is the visibility, transform and auth work layered on top of the providers. The provider case is unchanged from before this SDK grew an ownership check - on the missing-capability path it is strictly better, since that path took no ownership check at all. Generated-By: PostHog Desktop Task-Id: f66265f3-22a8-4eb5-9bf1-bf174c1c0c65
…nt tool A `FastMCP` is an `AggregateProvider`, which gathers its providers with `return_exceptions=True` and drops the failures, so an unreachable mounted or proxied sub-server reads back as a plain None from `get_tool` - indistinguishable from "no such tool". If that sub-server owned a real tool by a virtual tool's name, we treat the name as free and intercept. Worth writing down because the obvious fix does not work: the same failure is dropped from `list_tools` too, so a listing fallback returns the same blind answer, and fastmcp 3.x exposes no error strategy to opt out of. A later reader would otherwise rediscover this and add a probe that cannot help. Narrower than it first reads, which the note also records: during the outage the host's tool is missing from tools/list as well, so it could not have been dispatched either way. Only a provider that recovers between the check and dispatch loses a call that would have worked. Behaviour unchanged. Generated-By: PostHog Desktop Task-Id: f66265f3-22a8-4eb5-9bf1-bf174c1c0c65
The kind-keyed resolver put per-tool policy in one place, but the layer below it stayed four copies. `append_get_more_tools`, `append_send_feedback` and their two v2 twins differed only by descriptor builder and `inputSchema` vs `input_schema`, and the block that called them was repeated verbatim in all three adapters. - One `append_virtual_tool_by_kind` behind one `virtual_tool_descriptor` switch, applied through one `apply_virtual_tool_injection`. The missing-capability-then-feedback order the `duplicate` rule depends on is now stated once instead of implied in three places. - Both raw probes use `advertised_tool_names` rather than a third copy of the same comprehension. - `VirtualToolInjection` wrapped a dict and two `.get`s that nothing reads any more, so the resolver returns the dict. - Dead branches out: the `missing_name or name` fallback the `is_missing_capability` property already rules out, the unreachable `options is None` guard in the feedback appenders, and an `event` looked up before a branch that never used it. One behaviour change. `_name_owned_by_real_tool` on the low-level adapter was tri-state — a lookup that raises means "could not answer", and the call is delegated — but the FastMCP and v2 registry paths still read every failure as "the name is free" and swallowed the host's own tool. All three now share the contract, with a test per adapter that makes the registry raise and asserts the real tool runs. Both fail without the change. Prose: the PR was adding roughly 0.7 lines of comment per line of code against ~0.2 in the same files. Cut the narrative, the `@posthog/mcp` commentary and the sentences that had reached three copies; kept the `_tool_cache` re-injection reason, the late-bound handler lookup, the empty-string cursor rule, the JS-parity warning and the provider note, which was stated twice and is now stated once. Tests: `_make_paged_lowlevel`, `_list_page`, `_call_request` and `_ECHO_TOOL` were defined in both virtual-tool test files; they move to `_helpers_lowlevel`, kept out of `_helpers` because that one loads under both SDK majors. Dropped one resolver test subsumed by its neighbours. 515 passed on mcp 1.x, 440 passed / 19 skipped on 2.x, mypy and the public API snapshot clean. Generated-By: PostHog Desktop Task-Id: b86d25ed-d899-4e29-a52f-cd0ab29ae38d
3 participants
Problem
The SDK advertises two virtual tools into
tools/list:get_more_tools(report_missing) andsend_feedback(collect_feedback). A client concatenates every page into one list, so each must appear on exactly one page — and a host's own tool with a colliding name must win. Neither held.Ports PostHog/posthog-js#4953 and PostHog/posthog-js#4967, which fixed the same class of bug in the TypeScript SDK.
Changes
get_more_toolspage rulesend_feedbackpage rulenextCursornever saw ittools/listresult objectconversation_id"First page" means a request with no cursor; an empty string is a valid opaque cursor, so
cursor: ""is a continuation page.Both tools now share one kind-keyed resolver — enable switch, configured name, collision state and warning text live in one place per tool, so they cannot drift apart again. Three warnings (
blocked,shadowed,duplicate) each name the option that renames PostHog's tool, fire once per(tool, name, warning), and go to theposthog.mcpstdlib logger as well as theloggeroption. The README documents the rules and their limits.No public API signature changes.
Behaviour worth flagging
A real tool that appears only on a later page is shadowed: page one cannot see page two, so PostHog's tool is already advertised by then and calls to the name reach it.
@posthog/mcpbehaves the same way; the remedy is the rename option, which the warning names. This replaces a sticky collision flag that protected such a tool forever.Scope
Limited to the parity behaviour: where the virtual tools are advertised, and what happens when a name collides. Event attribution for a host tool sharing a virtual tool's name —
$mcp_intent, conversation id — is the stacked follow-up #963, along with two reviewer findings here that share that root cause.Nothing here regresses attribution: before this PR such a tool was swallowed and emitted no
$mcp_tool_callat all, so there was nothing to attribute.Verification
pytest posthog/test/mcpon both MCP SDK majors: 511 on v1, 443 passed / 19 skipped on v2.posthog/test/ai/hitting a403 product_access_denied; this change touches no file there.ruff format --check,ruff check, filteredmypy(clean — the baseline is empty),check_public_api.py.$mcp_tool_callis captured instead of$mcp_missing_capability.New tests in
test_virtual_tools.pycover pagination, the three collision cases, both ownership paths, delegation on an undeterminable check, custom names and reused result objects;test_units.pycovers the resolver under both majors;test_v2_lowlevel.py/test_v2_mcpserver.pycover v2.Created with PostHog Desktop