fix(mcp): restore analytics capture for standalone FastMCP 4 - #936
Conversation
Select the adapter by handler registry so standalone FastMCP 4 captures tool calls without changing the customer's instrument() invocation. Preserve application-owned arguments, including mounted tools, and use native v2 model fields when FastMCP exposes deprecated v1 aliases. Add HTTP regression coverage for both protocol eras, late tool registration, repeat instrumentation, failed tools, unavailable capture, and mounted argument ownership. Include FastMCP 4 in the existing MCP v2 CI matrix, document adapter behavior, and add a Sampo patch changeset. Validation on current main: MCP v1 suite 307 passed; MCP v2 suite 291 passed, 13 skipped. Ruff lint/format, filtered mypy (231 source files), import warning check, and actionlint passed. Real stdio tools emitted HTTP capture batches to a local receiver under both protocol eras, two events per run. Authenticated wizard installation and hosted ingestion remain untested.
Prompt To Fix All With AI### Issue 1
posthog/mcp/__init__.py:294-296
**Instrumentation remains order-dependent**
If a standalone FastMCP 4 server's low-level server is instrumented before its wrapper, both calls use the same tracking key, so the wrapper call returns without installing FastMCP-specific argument stripping. The low-level adapter then advertises analytics parameters but forwards them unchanged, causing tools that do not declare those parameters to reject otherwise valid calls. The existing installation needs to support adding the wrapper-specific context, with coverage for this reverse order.
---
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): capture standalone FastMCP on ..." | Re-trigger Greptile |
| instrument_lowlevel_v2( | ||
| server._mcp_server, data, strip_injected_for=server | ||
| ) |
There was a problem hiding this comment.
Instrumentation remains order-dependent
If a standalone FastMCP 4 server's low-level server is instrumented before its wrapper, both calls use the same tracking key, so the wrapper call returns without installing FastMCP-specific argument stripping. The low-level adapter then advertises analytics parameters but forwards them unchanged, causing tools that do not declare those parameters to reject otherwise valid calls. The existing installation needs to support adding the wrapper-specific context, with coverage for this reverse order.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/mcp/__init__.py
Line: 294-296
Comment:
**Instrumentation remains order-dependent**
If a standalone FastMCP 4 server's low-level server is instrumented before its wrapper, both calls use the same tracking key, so the wrapper call returns without installing FastMCP-specific argument stripping. The low-level adapter then advertises analytics parameters but forwards them unchanged, causing tools that do not declare those parameters to reject otherwise valid calls. The existing installation needs to support adding the wrapper-specific context, with coverage for this reverse order.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
posthog-python Compliance ReportDate: 2026-09-11 16:43:25 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
|
Keep a weak reference to the standalone FastMCP wrapper in shared tracking state so a later wrapper install enriches existing low-level instrumentation without wrapping handlers twice. Resolve schemas for the client-requested tool version before stripping analytics-owned arguments. Consolidate adapter imports at the optional MCP dependency boundary instead of repeating imports in dispatch branches. Keep FastMCP-specific imports lazy so the official SDK remains usable without that optional package. Validation: reproduced both setup-order failures and the versioned-tool argument failure before fixing them. MCP v1 suite 307 passed; MCP v2 suite 296 passed, 13 skipped. Ruff format/lint and filtered mypy passed. Fresh-process checks passed with MCP absent and with FastMCP absent.
Decide which analytics parameters to remove before dispatch from the tools/list the process served, recorded per tool in mutate_tool_schema, instead of asking FastMCP for the tool schema on every call. The live lookup remains only for tools never listed here or when the client pins a tool version, and now logs when it fails. Tools supplied by ToolInjectionMiddleware are listed but not resolvable via get_tool(), so the per-call lookup advertised `context` and then forwarded it, failing validation. Tested: posthog/test/mcp under mcp 2.2.0 + fastmcp 4.0.3 (297 passed, 13 skipped, new middleware test red before the fix) and under mcp 1.30.0 + fastmcp 3.2.0 (307 passed); ruff, mypy-baseline clean. Reviewer note: tool_model_parameter_injected still exists alongside the new tool_injected_parameters set because four adapters read it; folding it in is a follow-up. Claude-Session: https://claude.ai/code/session_015wZCNFdfPNK9k4utwJ5bMu
gesh
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. See the inline comment.
| return None | ||
|
|
||
|
|
||
| async def _standalone_injected_parameters( |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[router] 🟡 MEDIUM
When server.get_tool() fails, or returns an object whose .parameters is not a dict, this function returns None. _wrap_v2_call_tool then skips the strip step and sends the raw arguments to the real dispatch. The arguments still contain the injected analytics keys (context, conversation_id, llm_model).
FastMCP binds tool arguments strictly. A direct call to fastmcp.FastMCP.call_tool with one unexpected keyword raises a pydantic ValidationError (verified in a fastmcp 4.0.3 environment). So this fail-open path can break dispatch — the opposite of the invariant the other except blocks in this PR protect.
The common case looks safe: FastMCP's own call_tool() resolves the tool through self.get_tool(name, version=...) at the same point, so a lookup failure usually breaks the underlying dispatch too. The residual risk is narrower: a middleware whose on_call_tool hook short-circuits before the manager stage and dispatches to a bound function directly. Dispatch then succeeds, but the PostHog lookup fails and the unstripped keys go through.
None of the new tests in test_fastmcp_v4.py build that shape, so the path is untested. This is plausible, not confirmed.
Suggested action: either add a test for a tool that dispatches but does not resolve through get_tool(), or confirm that the path is unreachable in the supported FastMCP middleware patterns and record that in the docstring.
|
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: 💬 APPROVE WITH NITS (round 1 @ 7027977)The change does what it says: it restores analytics capture for standalone FastMCP 4, it keeps the behaviour stable across the three instrumentation orders, and the new tests cover those orders. One fail-open path stays untested. Key findings🟡 MEDIUM
ConvergenceNo convergent findings — the router pass covered the diff alone. Danger was assessed MEDIUM with HIGH confidence, so no stronger model was needed. Reviewer summaries
Note: Automated by QA Swarm — not a human review |
gesh
left a comment
There was a problem hiding this comment.
Looks good! Left just a small comment
Extend FastMCP HTTP coverage to prove middleware tools dispatch when get_tool returns None. Exercise calls before listing, all advertised analytics arguments after listing, and application-owned arguments in both paths. Document why unknown arguments remain intact. Validation: MCP v1 suite 307 passed; MCP v2 suite 299 passed, 13 skipped. Ruff check and format checks passed for MCP source and tests. Repository-wide filtered mypy passed for 231 source files.
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
Remove the server-wide injected-parameter cache. Resolve effective schemas with the current request context, middleware, and requested tool version before stripping analytics arguments. Avoid using cached model ownership when schema resolution fails. Add interleaved-client regression cases for dynamic providers, middleware tools, and middleware overrides in both listing orders. The provider and middleware cases reproduced a missing application-owned llm_model argument before the fix. Document the per-call schema lookup. Validation: MCP v1 suite 307 passed; MCP v2 suite 305 passed, 13 skipped. Ruff check and format passed for MCP source and tests. Repository-wide filtered mypy passed for 231 files.
💡 Motivation and Context
Standalone FastMCP 4 servers execute tools successfully but emit no MCP analytics after instrumentation.
FastMCP 4 uses the MCP SDK v2 registry; the standalone adapter still assumes the v1 handler dictionary.
Adapter routing before and after
Before:
After:
💚 How did you test it?
The MCP suite passed locally on the PR branch:
get_tool()resolution, stripping all three advertised analytics fields, and preserving application-owned fields before and after listing.Reproduce the regression suite with
pytest posthog/test/mcp --timeout=30in each dependency environment.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Codex used the MCP Analytics debugging, CI workflow, dataclass, test, code comment, and PR description skills with shell tools and the GitHub CLI.
The fix reuses registry-based SDK detection. Adapter imports share one lazy dependency boundary; FastMCP-specific helpers load only when needed. Existing mutable tracking state holds a weak wrapper reference. Tests exercise public FastMCP HTTP entry points with synthetic tools. Human review is required.
Standalone FastMCP calls perform a schema listing per request to preserve middleware and client-specific argument ownership without a shared cache.