fix(mcp): inject virtual tools once, on the first tools/list page

[gesh]

💡 Motivation and Context

The SDK adds two virtual tools to tools/list: get_more_tools (report_missing) and send_feedback (collect_feedback). A client joins every page into one list. Each tool must therefore appear on exactly one page, and a host tool with the same name must win. Neither rule held.

Ports PostHog/posthog-js#4953 and PostHog/posthog-js#4967 to Python.

Changes

Area	Before	After
get_more_tools page rule	every page	first page
send_feedback page rule	last page, so clients that ignore nextCursor never saw it	first page
Host tool with the same name, first page	swallowed, and the agent got PostHog's canned reply	the host tool runs, and a warning names the rename option
Ownership check cannot answer	the SDK intercepted the call	the SDK gives the call to the host
Host reuses one tools/list result object	the append changed that object, so both tools stopped being advertised	the append copies it
Custom tool names	matched literally, so a renamed tool got a stray conversation_id	resolved in one place

"First page" means a request with no cursor. An empty string is a valid opaque cursor, so cursor: "" is a continuation page.

One resolver now decides both tools. The enable switch, the name and the warning text live in one place per tool, so the two cannot drift apart. Three warnings (blocked, shadowed, duplicate) each name the option that renames PostHog's tool. Each warning fires once, and reaches the posthog.mcp logger as well as the logger option.

The public API does not change.

One behavior to flag

PostHog shadows a host tool that appears only on a later page. Page one cannot see page two, so PostHog's tool already holds the name by then, and calls to it reach PostHog. @posthog/mcp behaves the same way. The remedy is the rename option, which the warning names.

Scope

This PR covers where the SDK advertises the virtual tools, and what happens when a name collides. Event attribution for a host tool with the same name ($mcp_intent, conversation id) is the stacked follow-up #963. Attribution does not regress here. Before this PR the SDK swallowed such a tool, so it emitted no $mcp_tool_call to attribute.

💚 How did you test it?

• pytest posthog/test/mcp on both MCP SDK majors: 515 pass on v1, 440 pass and 19 skip on v2.
• Full suite except posthog/test/ai: 2040 pass.
• ruff format, ruff check, mypy, and make public_api_check.
• New tests cover pagination, the three collision cases, both ownership paths, delegation, custom names and reused result objects.

📝 Checklist

• I reviewed the submitted code.
• I added tests to verify the changes.
• I updated the docs if needed.
• No breaking change or entry added to the changelog.

If releasing new changes

• Ran sampo add to generate a changeset file

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Claude Opus 5 in PostHog Desktop wrote the code and the tests. Four reviewers (greptile, Veria, QA Swarm and an advisory review) found the bugs that the later commits fix. Each fix carries a regression test, which is most of the growth in this diff. Two claims in earlier review replies were wrong, and later commits correct them: a dead observed_listing flag that gated nothing, and a parity claim about @posthog/mcp that was the opposite of the truth.

The last code commit collapses duplication the review left behind. Four appenders become one, three adapter call sites become one, and the two registry adapters get the same tri-state ownership check as the low-level one.

Two choices are worth a reviewer's attention. Later-page shadowing inverts the old sticky-flag behavior, which the JS SDK also does, so a change here needs a matching change there. An ownership check that cannot answer gives the call to the host. That loses a PostHog event, but it never breaks a host tool.

---

Created with PostHog Desktop

[github-actions]

posthog-python Compliance Report

Date: 2026-09-17 08:32:26 UTC
Duration: 256383ms

✅ All Tests Passed!

111/111 tests passed

---

Capture_V1 Tests

✅ 94/94 tests passed

View Details

Test	Status	Duration
Endpoint And Method.Targets V1 Endpoint	✅	516ms
Endpoint And Method.Does Not Use Legacy Endpoints	✅	511ms
Required Headers.Has Authorization Bearer Header	✅	511ms
Required Headers.Has Content Type Json	✅	511ms
Required Headers.Has Posthog Sdk Info Format	✅	511ms
Required Headers.Has Posthog Attempt Header	✅	510ms
Required Headers.Has Posthog Request Id	✅	511ms
Required Headers.Has Posthog Request Timestamp	✅	511ms
Required Headers.Has User Agent	✅	511ms
Body Format.Body Has Created At And Batch	✅	511ms
Body Format.No Api Key In Body	✅	512ms
Body Format.No Sent At In Body	✅	511ms
Event Format.Event Has Required Root Fields	✅	510ms
Event Format.Event Uuid Is Valid	✅	511ms
Event Format.Event Timestamp Is Rfc3339	✅	510ms
Event Format.Distinct Id Is String	✅	511ms
Event Format.Distinct Id At Root Not Properties	✅	511ms
Event Format.Custom Properties Preserved	✅	511ms
Event Format.Set Properties Preserved	✅	510ms
Event Format.Set Once Properties Preserved	✅	511ms
Event Format.Groups Properties Preserved	✅	511ms
Event Format.Sdk Generates Uuid If Not Provided	✅	511ms
Event Format.Event Has Required Root Fields Batch	✅	514ms
Event Format.Event Uuid Is Valid Batch	✅	513ms
Event Format.Event Timestamp Is Rfc3339 Batch	✅	514ms
Event Format.Distinct Id Is String Batch	✅	514ms
Event Format.Distinct Id At Root Not Properties Batch	✅	513ms
Event Format.Custom Properties Preserved Batch	✅	515ms
Event Format.Set Properties Preserved Batch	✅	514ms
Event Format.Set Once Properties Preserved Batch	✅	514ms
Event Format.Groups Properties Preserved Batch	✅	514ms
Event Format.Sdk Generates Uuid If Not Provided Batch	✅	514ms
Batch Behavior.Multiple Events In Single Batch	✅	519ms
Batch Behavior.Batch Envelope Smoke	✅	515ms
Batch Behavior.Flush With No Events Sends Nothing	✅	507ms
Batch Behavior.Flush At Triggers Batch	✅	1012ms
Batch Behavior.Created At Reflects Batch Creation Time	✅	511ms
Deduplication.Generates Unique Uuids	✅	518ms
Deduplication.Different Events Same Content Different Uuids	✅	512ms
Deduplication.Preserves Uuid On Retry	✅	6516ms
Deduplication.Preserves Timestamp On Retry	✅	6521ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry	✅	6520ms
Deduplication.No Duplicate Events In Batch	✅	519ms
Header Behavior On Retry.Attempt Header Starts At One	✅	510ms
Header Behavior On Retry.Attempt Header Increments On Retry	✅	13523ms
Header Behavior On Retry.Request Id Preserved On Retry	✅	6520ms
Header Behavior On Retry.Different Requests Have Different Request Ids	✅	3021ms
Header Behavior On Retry.Request Timestamp Changes On Retry	✅	6520ms
Response Format Validation.Success Response Has Uuid Keyed Results	✅	511ms
Response Format Validation.Success Response Has Ok For Each Event	✅	515ms
Response Format Validation.Success No Retry After When All Ok	✅	513ms
Response Format Validation.Success Retry After Present When Retry Events	✅	1515ms
Response Format Validation.Success No Retry After When Drop Only	✅	513ms
Response Format Validation.Response Echoes Request Id	✅	511ms
Retry Behavior.Retries On 408	✅	6519ms
Retry Behavior.Retries On 500	✅	6517ms
Retry Behavior.Retries On 503	✅	8523ms
Retry Behavior.Retries On 504	✅	6517ms
Retry Behavior.Retryable Errors Have Retry After	✅	3517ms
Retry Behavior.Respects Retry After On Retryable Error	✅	11517ms
Retry Behavior.Does Not Retry On 400	✅	2515ms
Retry Behavior.Does Not Retry On 401	✅	2513ms
Retry Behavior.Does Not Retry On 402	✅	2514ms
Retry Behavior.Does Not Retry On 413	✅	2515ms
Retry Behavior.Does Not Retry On 415	✅	2512ms
Retry Behavior.Non Retryable Errors Have No Retry After	✅	2514ms
Retry Behavior.Implements Backoff	✅	22523ms
Retry Behavior.Max Retries Respected	✅	22537ms
Partial Batch Handling.Handles 200 Full Success	✅	2514ms
Partial Batch Handling.Handles 200 With All Ok	✅	3515ms
Partial Batch Handling.Does Not Retry Dropped Events	✅	3517ms
Partial Batch Handling.Does Not Retry Limited Events	✅	3516ms
Partial Batch Handling.Prunes Ok Events On Partial Retry	✅	6523ms
Partial Batch Handling.Prunes Dropped Events On Partial Retry	✅	6523ms
Partial Batch Handling.Retries Only Retry Events From Partial	✅	6523ms
Partial Batch Handling.Partial Retry Preserves Uuids	✅	6521ms
Partial Batch Handling.Partial Retry Attempt Header Increments	✅	6522ms
Partial Batch Handling.Partial Retry Request Id Preserved	✅	6517ms
Partial Batch Handling.Respects Retry After On Partial	✅	8521ms
Partial Batch Handling.Unknown Result Treated As Terminal	✅	3513ms
Partial Batch Handling.Mixed Ok Drop Limited No Retry	✅	3517ms
Compression.Sends Gzip Content Encoding	✅	511ms
Compression.No Content Encoding When Disabled	✅	511ms
Compression.Compressed Body Is Decompressible	✅	510ms
Error Handling.Does Not Retry On Unknown 4Xx	✅	2512ms
Event Options.Cookieless Mode Override	✅	511ms
Event Options.Disable Skew Correction Override	✅	511ms
Event Options.Process Person Profile Override	✅	510ms
Event Options.Product Tour Id Override	✅	510ms
Event Options.Unset Options Omitted	✅	510ms
Event Options.Options Override In Batch	✅	513ms
Geoip And Historical Migration.Geoip Disable Injected Into Properties	✅	511ms
Geoip And Historical Migration.Historical Migration Set In Body	✅	510ms
Geoip And Historical Migration.Historical Migration Absent By Default	✅	510ms

Feature_Flags Tests

✅ 17/17 tests passed

View Details

Test	Status	Duration
Request Payload.Request With Person Properties Device Id	✅	12ms
Request Payload.Flags Request Uses V2 Query Param	✅	9ms
Request Payload.Flags Request Hits Flags Path Not Decide	✅	9ms
Request Payload.Flags Request Omits Authorization Header	✅	8ms
Request Payload.Token In Flags Body Matches Init	✅	9ms
Request Payload.Groups Round Trip	✅	9ms
Request Payload.Groups Default To Empty Object	✅	9ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False	✅	8ms
Request Payload.Disable Geoip Omitted Defaults To False	✅	9ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key	✅	9ms
Request Lifecycle.No Flags Request On Init Alone	✅	3ms
Request Lifecycle.No Flags Request On Normal Capture	✅	508ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests	✅	15ms
Request Lifecycle.Mock Response Value Is Returned To Caller	✅	9ms
Retry Behavior.Retries Flags On 502	✅	313ms
Retry Behavior.Retries Flags On 504	✅	313ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event	✅	511ms

[greptile-apps]

Comments Outside Diff (1)

1. posthog/mcp/_instrumentation.py, line 571-583 (link)

   Ownership resolves too late

   The lifecycle filters virtual-tool names through server-wide collision state before the adapters perform their request-local ownership checks. If caller A records a collision, caller B's virtual-tool call is treated as a normal host call and the live probe is skipped, so the call can be misrouted or fail as an unknown tool. Conversely, a real colliding tool called before any listing is dispatched normally, but its intent and conversation handling still use virtual-tool policy. Resolve ownership before constructing the lifecycle, then use that result consistently for interception, intent, and conversation handling.

   Knowledge Base Used: MCP framework instrumentation

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: posthog/mcp/_instrumentation.py
   Line: 571-583
   
   Comment:
   **Ownership resolves too late**
   
   The lifecycle filters virtual-tool names through server-wide collision state before the adapters perform their request-local ownership checks. If caller A records a collision, caller B's virtual-tool call is treated as a normal host call and the live probe is skipped, so the call can be misrouted or fail as an unknown tool. Conversely, a real colliding tool called before any listing is dispatched normally, but its intent and conversation handling still use virtual-tool policy. Resolve ownership before constructing the lifecycle, then use that result consistently for interception, intent, and conversation handling.
   
   **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.

Prompt To Fix All With AI

### Issue 1
posthog/mcp/_instrumentation.py:571-583
**Ownership resolves too late**

The lifecycle filters virtual-tool names through server-wide collision state before the adapters perform their request-local ownership checks. If caller A records a collision, caller B's virtual-tool call is treated as a normal host call and the live probe is skipped, so the call can be misrouted or fail as an unknown tool. Conversely, a real colliding tool called before any listing is dispatched normally, but its intent and conversation handling still use virtual-tool policy. Resolve ownership before constructing the lifecycle, then use that result consistently for interception, intent, and conversation handling.

### Issue 2
posthog/mcp/posthog_mcp.py:398-417
**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.

### Issue 3
posthog/mcp/_instrument_lowlevel.py:352-365
**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.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{Reviews (1): Last reviewed commit: "fix(mcp): inject virtual tools once, on ..." | Re-trigger Greptile}

[marandaneto]

Automated advisory code review.

[veria-ai]

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 2 · PR risk: 0/10

[lucasheriques]

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.

[lucasheriques]

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: ⚠️ REQUEST CHANGES _{(round 1 @ f07e28a)}

The pagination and collision design (kind-keyed resolver, copy-on-append, first-page-only injection, custom names, same-name precedence) is sound and verified live on both SDK majors. Two claims about call-time ownership do not hold, and the test meant to cover one of them cannot fail. Both defects are silent: no crash and no swallowed call, but the analytics data this SDK exists to produce degrades.

Key findings

• 🟠 HIGH — observed_listing is never set (_instrumentation.py:888). The probe re-runs the host's tools/list handler on every virtual-tool call on raw low-level servers, contradicting the docstring, README, commit 8b4b35b and the greptile reply. Behaviour matches JS; text does not. Decision needed: set the flag, or drop the flag and the claim.
• 🟠 HIGH — real tool owning a virtual-tool name loses $mcp_intent and conversation id (_instrumentation.py:571). Call-time probes gate interception but never update virtual_tool_collisions, so conversation-id and intent resolution still treat the name as the virtual tool. Permanent on FastMCP/MCPServer until a listing is served.
• 🟡 MEDIUM — vacuous regression test (test_virtual_tools.py:407). Counts on the registry entry while the probe calls the captured original function.

Convergence

Both HIGH findings were reached independently by the router (Sonnet) and by the escalation tier (Fable), each with its own reproduction. The router additionally reproduced both on mcp 2.2.0.

Verified clean

v2 handler params are validated PaginatedRequestParams | None models; append_virtual_tool preserves nextCursor on both majors and all four append helpers route through it; the new mutable sets are safe under FastMCP's worker-thread offload; PostHogMCP descriptor comparison handles re-prepared lists. Tests: 510 passed on mcp 1.30.0, 63 v2 tests passed on mcp 2.2.0.

Reviewer summaries

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

[lucasheriques]

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.

[marandaneto]

Automated advisory code review.

[marandaneto]

blocking: Preserve conversation attribution for real feedback tools — With collect_feedback=True and enable_conversation_id=True, a real application tool named send_feedback still runs after listing but now loses its conversation ID and prompt-back. feedback_name no longer excludes host-owned tools before resolve_conversation_id() runs; the later ownership check preserves dispatch without correcting attribution. Unlike the get_more_tools attribution work deferred to #963, this send_feedback case already worked on the merge base. Reproduction: reproduced — uv run --extra dev --extra test pytest posthog/test/mcp/test_pr962_regression.py -q --timeout=30, using FastMCP call_tool(convert_result=True) after tools/list, fails on a091101 with missing $mcp_conversation_id and passes on merge base 794354c.

[gesh]

The feedback feature is still in progress. Working on a follow-up PR to add a proper conversation ID to the virtual tools.

[marandaneto]

Two non-blocking product decisions worth explicitly confirming before merging:

1. A real tool on a later page can become unreachable. For example, the host advertises search on page 1 and its own send_feedback on page 2. PostHog adds its virtual send_feedback to page 1 because it cannot see page 2 yet. The client then sees two tools with the same name. On the raw low-level path, the call-time ownership check also inspects only the first page, so calls to that name reach PostHog rather than the host's implementation. A warning is emitted when the later-page collision is seen, and renaming PostHog's tool avoids the conflict. This is documented in the PR, but is making a host tool unreachable an intentional and acceptable product tradeoff for first-page discovery and JS parity?

2. Every virtual-tool call can rerun the host's listing work. FastMCP/MCPServer can check ownership through their registry. A raw low-level server instead invokes the host's original tools/list handler once per call to an enabled virtual-tool name, checking its first page before deciding whether to intercept or delegate. Ordinary tool calls do not incur this check. This is not necessarily another HTTP request, but if the listing handler queries a database or remote service, that work adds latency/load to every such call. The benefit is fresh, caller-specific ownership rather than trusting a previous listing. Is that cost acceptable for the intended integrations?

These are product/behavior callouts, not additional blocking findings. Please explicitly confirm whether both behaviors are intended; if not, the desired discovery, collision, and ownership-check behavior should be agreed before merge.

[marandaneto]

left a last comment and 2 product questions, otherwise lgtm

[gesh]

Two non-blocking product decisions worth explicitly confirming before merging:

1. A real tool on a later page can become unreachable. For example, the host advertises search on page 1 and its own send_feedback on page 2. PostHog adds its virtual send_feedback to page 1 because it cannot see page 2 yet. The client then sees two tools with the same name. On the raw low-level path, the call-time ownership check also inspects only the first page, so calls to that name reach PostHog rather than the host's implementation. A warning is emitted when the later-page collision is seen, and renaming PostHog's tool avoids the conflict. This is documented in the PR, but is making a host tool unreachable an intentional and acceptable product tradeoff for first-page discovery and JS parity?

Intentional. Page one cannot see page two, so any injection decision there is made without knowledge of later pages, and each alternative is worse: injecting on the last page is the bug this PR fixes (clients that ignore nextCursor never saw send_feedback), walking every page before deciding turns one listing into N against a handler that may hit a database, and deferring injection until the collision is visible is per-server state that lets one caller's listing decide another caller's dispatch.
TLDR: We simply want to warn when there's conflict, allowing the users to configure our virtual tool names to prevent conflicts.

2. Every virtual-tool call can rerun the host's listing work. FastMCP/MCPServer can check ownership through their registry. A raw low-level server instead invokes the host's original tools/list handler once per call to an enabled virtual-tool name, checking its first page before deciding whether to intercept or delegate. Ordinary tool calls do not incur this check. This is not necessarily another HTTP request, but if the listing handler queries a database or remote service, that work adds latency/load to every such call. The benefit is fresh, caller-specific ownership rather than trusting a previous listing. Is that cost acceptable for the intended integrations?

These are product/behavior callouts, not additional blocking findings. Please explicitly confirm whether both behaviors are intended; if not, the desired discovery, collision, and ownership-check behavior should be agreed before merge.

Only raw low-level servers pay it, only on calls to an enabled virtual-tool name, and only for the first page — the alternative is trusting a previous or another caller's listing, which is the stateful cross-client bug this PR removes.