chore(mcp): enable model capture and conversation correlation by default - #944
lucasheriques wants to merge 7 commits into
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.
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}
posthog-python Compliance ReportDate: 2026-09-15 18:21:49 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
left a comment
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 inline comments. Summary posted separately.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| pypi/posthog: minor | |||
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[convergent: router + paul] 🟠 HIGH
Two reviewers landed on this line independently: is minor the right tier?
Verified empirically — with capture_model=True by default, the advertised inputSchema of every tool on the official high-level FastMCP / MCPServer adapters now lists llm_model in required (next to the already-default context). On a routine pip install --upgrade posthog, every existing deployment's wire-visible tool contract changes with zero code change by the user. enable_conversation_id=True adds a handle to eligible tool responses on top of that.
The dispatch path does not enforce the required flag — a call omitting context and llm_model still dispatches fine — so there is no server-side breakage. The risk is client-side: strict-schema MCP clients that validate before sending, and tooling that generates call templates from a cached schema.
Paul's read: "i can talk myself into minor (capture_model only landed in #927 six days ago, so the blast radius is genuinely small, and context already set the precedent of injecting a required arg), and i'm not going to block on the tier. but the thing i'd actually want confirmed: references/public_api_snapshot.txt changes on four lines here, and CONTRIBUTING.md now says that means 'this touches public API, agree the shape on the issue first'. i'm lazily asking rather than digging — was this one agreed somewhere?"
There was a problem hiding this comment.
Keeping minor: context shipped the same required-argument precedent as a minor, and dispatch never enforces required. On the CONTRIBUTING.md point: this repo has no "agree the shape on an issue first" rule; AGENTS.md only asks that the snapshot be regenerated (make public_api_snapshot), which it is. posthog-js does have that rule for new or changed option shapes, and this change alters no shape: same options, same types, different defaults. The decision and the rejected alternatives are now recorded in posthog-js ADR-0013, which both SDKs cite.
| # SDK-injected llm_model argument. Off by default. | ||
| capture_model: Union[bool, MCPAnalyticsModelOptions] = False | ||
| # SDK-injected llm_model argument. On by default; False disables capture. | ||
| capture_model: Union[bool, MCPAnalyticsModelOptions] = True |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[paul] 🟠 HIGH
this is the one i keep coming back to. capture_model is a single boolean doing two completely unrelated things: (a) read the model out of recognised client _meta — invisible, read-only, no wire change at all; and (b) inject llm_model into the advertised schema and stick it in required — very much a wire change. i'm allergic to flags like that generally, but here it also blocks the rollout i'd want: default-on the half that only observes, keep the half that changes the contract opt-in, and use a release or two of $mcp_llm_model_source: client_metadata data to find out how much self-reporting is even buying us. log before you limit.
MCPAnalyticsModelOptions already exists as the object form and currently only carries description, so there's a ready-made home for something like inject_argument: bool = False. that'd let this ship default-on today with no consumer-visible change, and the schema half flip once we've seen numbers. feel free to disagree — but "the schema we advertise to the world changes on a patch-ish upgrade" is exactly the class of thing i'd want a staged path for.
There was a problem hiding this comment.
Considered and rejected in posthog-js ADR-0013. Only Codex exposes model metadata today, so a default-on observe-only half yields near-zero data for every other harness and only delays what the flip exists to collect. The object form (MCPAnalyticsModelOptions) leaves room for an inject=False knob later without a breaking change, if the data says self-report is not earning its field.
| missing_capability_tool_name: Optional[str] = None, | ||
| mcp_exception_autocapture: bool = True, | ||
| capture_model: Union[bool, MCPAnalyticsModelOptions] = False, | ||
| capture_model: Union[bool, MCPAnalyticsModelOptions] = True, |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[paul] 🟢 LOW
not blocking at all, just planting a seed. the only way off this default is an argument in source, which means a code change and a redeploy — and the person upgrading is often not the person who wrote the instrument() call. an env-var override (POSTHOG_MCP_CAPTURE_MODEL=0 or similar) reading through to this default would let someone turn it off at 3am without shipping code. knight capital is my usual sermon here (https://specbranch.com/posts/knight-capital/) and it's admittedly a bit heavy for an analytics field — but "make it configurable and measurable" is cheap now and expensive later. ship as you see fit.
There was a problem hiding this comment.
Not taking this one: nothing in the package reads the environment, and the opt-out is a one-line option. Recorded as rejected in posthog-js ADR-0013.
|
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
2 participants
Motivation and changes
Model capture and conversation correlation currently require separate opt-ins. This enables both by default, preserves explicit opt-outs, and leaves missing-capability reporting and feedback collection disabled for Gesh's work. The decision and the alternatives considered are recorded in posthog-js ADR-0013, which both SDKs follow.
A fresh raw low-level instance (a new server per request) never serves a
tools/list, so it could not say who ownsllm_modeland stayed silent. It now follows the rule posthog-js ADR-0011 applies tocontext: reads fail open, strips fail closed. Nothing new runs on the call path. An earlier revision of this branch replayed the host's listing insidetools/call; that is the design ADR-0011 rejects, and it was removed after review.Standalone
fastmcp.FastMCPon MCP SDK 1.x validates arguments, so a cold replica must strip what the listing injected. It now reads ownership from the registered tool schema, never through middleware, and strips only the keys the SDK injects under the current options that the tool does not declare. The registry is trusted forllm_modelonly when no application middleware can change the listing or reroute dispatch; otherwise the argument stays and is read fail-open. Root$refchains, JSON Pointer escapes, composed schemas, the server's own dereferencing setting, pinned tool versions, and duplicate names in a listing are each handled the way the listing handles them. The v2 adapter's registry path is unchanged from main.Wire-visible effects on upgrade: compatible schemas gain
llm_model(required on the official high-level adapters, optional elsewhere, never enforced at dispatch) and an optionalconversation_id, and eligible results gain a prompt-back handle.MCPAnalyticsOptions(capture_model=False, enable_conversation_id=False)restores the previous shape.capture_modelis also on by default onPostHogMCP. No option shape changes; the public API snapshot records the new defaults.Validation
codex review --base main: fourteen rounds on the standalone path, each finding fixed with a regression test; the final round on 5f20b75 reported no actionable regressions.Release
.sampo/changesets/mcp-analytics-defaults.mdrequests a minor release. Review and merge, then approve the normal release workflow; nothing has been published from this branch.Agent context
Autonomy: Human-driven (agent-assisted), directed by Lucas Faria.
Initial implementation with Codex. Ownership rework and review response with Claude Code. Human review required, with particular attention to
_standalone_ownershipand its helpers inposthog/mcp/_instrument_lowlevel.py.https://claude.ai/code/session_012YBJXRzzCizpHFGn2mZaEg