chore(mcp): enable model capture and conversation correlation by default - #944
Merged
Merged
Conversation
Enable both existing instrumentation features and model capture on PostHogMCP, with explicit opt-outs. Missing-capability reporting and feedback stay disabled. Resolve model argument ownership from bounded raw catalog lookups on fresh low-level instances. Do not emit synthetic tools/list events, alter application model arguments, or block dispatch when catalog resolution fails. Validation: 486 MCP v1 tests passed (1 skipped), 433 MCP v2 tests passed (21 skipped); Ruff lint/format, mypy baseline for 237 files, public API snapshot, warning-as-error import and wheel build passed. CodeScene gate passed. One feedback cursor deprecation warning remains outside the requested scope. Include a Sampo minor changeset and document changed defaults and opt-outs.
Contributor
Prompt To Fix All With AI### Issue 1
posthog/mcp/_tool_schema.py:23-25
**Ownership lookup is not cached**
The fallback returns the resolved ownership without storing it. On a cold low-level server, each direct tool call therefore invokes the original `tools/list` handler again, potentially traversing 16 pages, repeating handler side effects, and adding up to 250 ms of latency. Cache the result in `data.tool_model_parameter_injected` before returning it.
```suggestion
owns_model = await asyncio.wait_for(
_find_model_ownership(name, list_page), timeout=0.25
)
data.tool_model_parameter_injected[name] = owns_model
return owns_model
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "chore(mcp): enable model capture and con..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
posthog-python Compliance ReportDate: 2026-09-17 19:30:36 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
|
Remove the duplicate custom-dispatcher default and opt-out test. The existing parameterized prepare-and-capture test already covers both. Keep the fresh-instance regressions and bounded catalog lookup coverage. Validation: MCP v1: 485 passed, 1 skipped. MCP v2: 432 passed, 21 skipped. Ruff lint/format and CodeScene pre-commit checks passed. The existing feedback nextCursor deprecation warning remains outside this change. Runtime behavior is unchanged.
Store both confirmed ownership outcomes in the existing model cache. Return None internally for unresolved catalogs so missing tools, page limits, errors and timeouts do not become permanent negative entries. Extended existing pagination/failure tests and added late-tool recovery. Caching regressions failed before the fix. Validation: MCP v1 486 passed, 1 skipped; MCP v2 433 passed, 21 skipped. Ruff, mypy baseline (237 files), and CodeScene pre-commit checks passed. The feedback nextCursor warning remains outside this change. Addresses review discussion_r3992430558 on #944.
gesh
reviewed
Sep 15, 2026
gesh
left a comment
gesh
left a comment
Member
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
QA Swarm review complete — see inline comments. Summary posted separately.
Member
|
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 MEDIUM, confidence HIGH. Read every touched file with full context, installed both MCP SDK majors in real virtualenvs, and confirmed the pagination duck-typing, the argument stripping on cold low-level servers, and that the full suite plus the new tests pass under both majors. Found no correctness bug. Delegated one item: the rollout/semver judgment call. |
| 👤 paul-reviewer | Approves with caveats. Likes the direction — "you get the data without reading the docs" is usually right for an analytics SDK — and can live with minor, since capture_model is about a week old and context set the precedent. One real ask: split the capture_model boolean so the observing half can ship before the contract-changing half. |
Warning
qa-team was unavailable in this environment (skill not on disk). Its correctness, concurrency, and database lenses were not applied. xp-reviewer and security-audit were available but the router did not delegate to them.
Automated by QA Swarm — not a human review
Replaces the raw-catalog replay added earlier on this branch with the ownership rule posthog-js ADR-0011 applies to `context`: a fresh raw low-level instance reads the self-reported model and strips nothing, and the SDK never replays the host's tools/list handler on the call path. Standalone FastMCP reads ownership of all three injected keys from the tool signature, so it strips llm_model without a prior listing. Tested: .venv pytest posthog/test/mcp (478 passed, 1 skipped), .venv-mcp-v2 (424 passed, 21 skipped), ruff check/format, mypy baseline (no issues), public API snapshot up to date. Reviewer notes: raw low-level servers keep main's behaviour of never stripping llm_model; posthog-js strips on positive ownership. That pre-existing difference is unchanged here. Claude-Session: https://claude.ai/code/session_012YBJXRzzCizpHFGn2mZaEg
A jlowin `Tool` subclass declares its arguments in `parameters` and may have no `fn`, so the signature-based check returned nothing owned and the strip loop deleted an application-declared `llm_model`. Ownership now comes from the advertised schema first, matching the v2 standalone path, with the signature as fallback. Tested: .venv pytest posthog/test/mcp (480 passed, 1 skipped), .venv-mcp-v2 (424 passed, 21 skipped), ruff, mypy baseline. Found by `codex review --base main` on the previous commit. Claude-Session: https://claude.ai/code/session_012YBJXRzzCizpHFGn2mZaEg
The v1 and v2 standalone adapters resolved injected-argument ownership two different ways, and the v1 one stripped an application-declared `llm_model` inside a composed schema (`allOf`, `$ref`). Lift the v2 helper into `_standalone.py`, add the per-key composed-schema guard the listing-time injection already applies, and use it from both paths. Lookup failure now strips nothing on v1 too (fail closed), matching v2. Tested: .venv pytest posthog/test/mcp (482 passed, 1 skipped), .venv-mcp-v2 (424 passed, 21 skipped), ruff, mypy baseline, public API snapshot. Found by `codex review --base main` on the previous commit. Claude-Session: https://claude.ai/code/session_012YBJXRzzCizpHFGn2mZaEg
…from its schema With model capture on by default, a standalone FastMCP replica that never served the listing which advertised `llm_model` forwarded it to FastMCP's validator and the call failed. FastMCP rejects any undeclared argument, so stripping an analytics key the tool does not declare can never hurt, while stripping a declared one always does. The v1 standalone path now strips `llm_model` like `context` and `conversation_id`: unless the registered schema (or, without one, the function signature) declares it, with nothing stripped from a composed schema because nothing was injected into one. The registry is read directly, never through middleware, so rate limiters are not charged and no listing is needed. A local root `$ref` is dereferenced first, as FastMCP's built-in middleware does before the client sees the listing, and a pinned `_meta.fastmcp.version` is honoured only where FastMCP's own dispatch honours it, so ownership always follows the version that runs. Only keys the SDK injects under the current options are ever stripped, so a disabled feature leaves its key to the application, and sibling properties beside a root `$ref` count as declared, as does every node along a reference chain. For `llm_model` the effective listing is the first witness, because middleware can provide or shadow the tool the registry knows; the registry is second; with neither the argument stays and is still read. The registry is not trusted for `llm_model` while application middleware can change the listing or reroute dispatch, the server's own dereferencing setting decides whether a root `$ref` would have been injected into, and a listing that advertises two tools under one name (FastMCP 2.x with a shadowing middleware) marks the model argument as the application's. Application subclasses of FastMCP's built-in middleware count as the application's, and `$ref` segments decode JSON Pointer escapes. The model is read exactly when it was stripped; raw low-level servers keep reading it fail-open on unknown ownership (posthog-js ADR-0011). This replaces the listing-based resolver tried earlier on this branch, which five Codex rounds showed diverging from the advertised listing across FastMCP 2.5-4.0 and middleware combinations. Tested: .venv pytest posthog/test/mcp (502 passed, 1 skipped), .venv-mcp-v2 (424 passed, 21 skipped), a throwaway venv with fastmcp==2.14.5 (standalone and defaults tests: 28 passed, 5 skipped), and one with fastmcp==2.8.1, which has no middleware module (9 passed, 23 skipped; the one failure is a pre-existing test passing a constructor kwarg that release lacks), ruff, mypy baseline, public API snapshot. Claude-Session: https://claude.ai/code/session_012YBJXRzzCizpHFGn2mZaEg
Merge main, retaining both standalone schema ownership and virtual-tool collision protections. Suppress new conversation handles when the request already carries a token/transport session; echoed handles still win. Preserve host arguments when standalone FastMCP registry introspection fails transiently, while retaining the not-found middleware fallback. Adjust fixtures that deliberately test behavior with model capture off. Validation: MCP v1 549 passed, 1 skip; MCP v2 444 passed, 21 skips. Ruff lint/format, mypy baseline (239 files), and public API snapshot pass. The pre-commit CodeScene merge comparison flagged inherited main changes; a scoped PR comparison against origin/main follows before push.
Keep the conversation guard readable and remove the new complex-condition finding. Existing transport and token sessions retain the same precedence. Validation: defaults tests pass (6 passed, 1 skipped); Ruff lint and format pass. CodeScene pre-commit safeguard passes. Full MCP v1/v2 suites passed before this condition extraction.
DanielVisca
self-requested a review
September 17, 2026 17:55
DanielVisca
approved these changes
Sep 17, 2026
Skip model argument injection on standalone MCP SDK v1 servers when application middleware can alter listing or dispatch. Keep client metadata capture enabled, and invalidate earlier model ownership so a replacement tool retains its own llm_model argument. Document the middleware limitation in the README and changeset. Extend the existing shadowing test with listed, reordered, and late middleware cases, and verify a fresh replica executes advertised calls while still capturing client metadata. Validation: MCP v1 553 passed, 1 skipped; MCP v2 444 passed, 21 skipped. Ruff lint/format, mypy (239 files), public API snapshot, and CodeScene safeguard pass. CodeScene improves both runtime files; existing large-module and test-complexity warnings remain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and changes
Model capture and conversation correlation currently require separate opt-ins. Enable both by default, preserve explicit opt-outs, and leave missing-capability reporting and feedback collection disabled for Gesh's work. Both SDKs follow ADR-0013, which records the decision and compatibility tradeoffs.
Compatible schemas gain
llm_model(required on the official high-level adapters, optional elsewhere, never enforced by analytics dispatch) and optionalconversation_id. An echoed handle takes precedence, but a newly minted handle no longer replaces an existing transport session or PostHog session token. Sessionless calls can still receive a handle to echo.MCPAnalyticsOptions(capture_model=False, enable_conversation_id=False)restores the previous defaults. Model capture is also on by default onPostHogMCP; the public API snapshot records the new defaults.Fresh raw low-level instances read analytics arguments when ownership is unknown and strip only with evidence of SDK ownership, following ADR-0011. No host catalog is replayed during calls. Standalone FastMCP reads ownership from its registered schema, accounts for references/composition and middleware, and preserves application-declared keys. Transient registry lookup failures now preserve potentially host-owned arguments; actual not-found results retain middleware fallback behavior.
On MCP SDK v1, standalone FastMCP middleware that overrides listing or dispatch disables model argument injection and invalidates stale ownership. This prevents a fresh replica from rejecting an advertised analytics argument and preserves a replacement tool's own
llm_modelargument. Model capture from client metadata remains enabled; self-report injection is unavailable on these middleware installations, including pass-through logging/auth middleware. No new public option or catalog replay is introduced.Validation
mainand verified its virtual-tool collision regressions alongside the new defaults, resolving conflicts without rewriting branch history.Release
.sampo/changesets/mcp-analytics-defaults.mdrequests a minor release. Human review and the normal release workflow are still required; no package has been published from this branch.Agent context
Autonomy: Human-driven (agent-assisted), directed by Lucas Faria.
Initial implementation and latest review fixes with Codex. Ownership rework and earlier review response with Claude Code. Human review required, particularly for
_standalone_ownershipand its helpers inposthog/mcp/_instrument_lowlevel.py.https://claude.ai/code/session_012YBJXRzzCizpHFGn2mZaEg