Repository navigation
feat(mcp): group sessions by ChatGPT, Codex and VS Code conversation ids - #1193
Open
lucasheriques wants to merge 8 commits into
Open
lucasheriques wants to merge 8 commits into
lucasheriques wants to merge 8 commits into
Conversation
Read ChatGPT's `_meta["openai/session"]` conversation id as a session source for calls with no conversation handle and no transport session, and stamp `$mcp_session_source` (conversation, transport, client_metadata, generated) on every event whose session the SDK resolves. Why: on per-request stateless servers with `enable_conversation_id=False` (common for ChatGPT/Claude directory submissions) every call got its own `$session_id` (PostHog/posthog#119784). ChatGPT already sends a per-conversation id, so grouping needs no schema or result change. Order is handle, transport (token or mcp id), client id, generated, so it only groups calls that fragment today. A call carrying it mints no handle and gets no prompt-back. Same rule in `PostHogMCP.prepare_tool_call` via `request_meta`, after an explicit `session_id`. Twin of posthog-js 3eb19d9 and ADR-0014; the derivation `ses_35e4cc45ea3e5e1282c3f091de632132` is a cross-SDK contract. Internals: `resolve_session_id_with_source` / `prepare_request` return a `ResolvedSession(session_id, source)`, which the record_* functions take, so the source reaches every event. `token`/`mcp` map to `transport` at the property boundary. `handshake_session_id` becomes `handshake_session`. A client-conversation session never stands in for an unobserved `initialize`, like a conversation one. Tested: `pytest posthog/test/mcp` -> 699 passed, 10 skipped on mcp 1.30.0 and 557 passed, 42 skipped on mcp 2.3.0 (incl. new parameterized test_client_conversation_session.py plus a v1 FastMCP and a v2 wire-level fresh-instance test); `ruff format --check`, `ruff check`, `mypy`, `make public_api_check` clean.
Pre-PR gate findings on the TypeScript twin (posthog-js 26d720abd), applied here: - tools/list, resources/list|read and initialize resolved sessions without the client conversation id, which the spec's session order covers for every event. Each call site now passes it (`params.meta` on request handlers, `_meta` of the initialize params through `handshake_session`). Still per-request, never written to shared state, and it still does not stand in for an unobserved initialize. - Custom events from `McpAnalytics.capture()` now carry `$mcp_session_source`, the instance's own source (`generated`, or `transport` for a token or mcp session id). - `prepare_tool_call(session_id="")` already treated an empty id as absent; a regression case now pins it. Tested: `pytest posthog/test/mcp` -> 714 passed, 11 skipped on mcp 1.30.0 and 570 passed, 45 skipped on mcp 2.3.0 (fastmcp uninstalled); new parameterized cases failed before the fix (12 on mcp 2.3.0); `ruff format --check`, `ruff check`, `mypy`, `make public_api_check` clean.
…ernal tools/list What - ToolCallLifecycle.prime_session now passes the request's client conversation to resolve_session_id, so a call carrying only openai/session returns at the client_metadata step before the memory branch. It no longer refreshes data.last_activity or rotates data.session_id, so ChatGPT calls cannot keep the instance's generated session alive past its inactivity window. - The v2 tools/list wrapper skips analytics when the SDK calls the handler itself. On the 2026-07-28 streamable-HTTP path, mcp 2.3.0 runs tools/list during every tools/call (Mcp-Param-* validation) on a server without get_tool_input_schema (the low-level Server; MCPServer provides it). That call has no _meta, resolved to a fresh generated session, and emitted a phantom $mcp_tools_list. It is detected by the shared HTTP request's required Mcp-Method header naming a different method than the handler's own. Why Pre-PR gate findings on the client-metadata session work; the spec limits events to requests the client sent. Tested - pytest posthog/test/mcp: mcp 1.30.0 716 passed, 11 skipped; mcp 2.3.0 576 passed, 45 skipped (fastmcp uninstalled) - ruff format --check, ruff check, mypy | mypy-baseline filter, make public_api_check: clean - New tests failed before the fixes (instance state moved; phantom event emitted). A real tools/list still emits on both MCPServer and low-level Server.
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 208707d: 1 Must fix.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
Contributor
posthog-python Compliance ReportDate: 2026-11-12T15:10:45.278797+00:00 ✅ All Tests Passed!270/270 tests passed Capture_V1 Tests✅ 95/95 tests passed View Details
Capture_Ai Tests✅ 5/5 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
Feature_Flags_V4 Tests✅ 149/149 tests passed View Details
Feature_Flags_Local_Evaluation Tests✅ 4/4 tests passed View Details
|
Contributor
SDK performance: posthog-pythonBenchmarks completed. No changes flagged for review. Flags require at least a 5% change in the median and all six pairs agreeing on direction. This is a noise filter, not proof of a performance change or a CI gate. View all results
Values are medians of paired measurements. Not flagged does not mean unchanged. Unavailable means a measurement could not be provided, not zero. Estimated allocation measurements retain their instrumentation uncertainty. RSS measures the whole process, not just SDK allocations; observed peaks are sampled checkpoints. Paired ranges in the diagnostics describe variability, not confidence intervals. Each shard uses its own runner; compare Before PR against With PR within a row, not across shards. Compared 102 metric rows; 102 rows shown in all results. Omitted or shortened data remain in |
dustinbyrne
reviewed
Nov 5, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
Reproduced a custom-event attribution regression: after a token-bearing request, a metadata-only caller’s in-tool capture can inherit the previous caller’s session and person. The same public-entry test passes on base and fails on this head.
The metadata policy also needs reconciliation with the current canonical MCP specification; the linked spec proposal remains open. Details and executed evidence are inline.
AI-assisted review.
…x, VS Code) Rework the openai/session change to the merged sdk-specs rules (PostHog/sdk-specs 2026-11-04-mcp-client-conversation-ids and -codex-vscode-conversation-ids), matching posthog-js#5577: - _client_conversation.CLIENT_CONVERSATION_ROWS mirrors the spec table: openai/session (1-128 printable ASCII, handle-free), Codex sessionId and VS Code vscode.conversationId (UUID, lowercased, non-nil, handle kept). - A key is read only from a request its row recognizes by User-Agent or client name (_meta clientInfo first, then the adapter's resolved name). - The client id is session source 1, ahead of the handle and transport, on tool calls only (tools/list, resources and initialize no longer read it). A handle echoed with it is stripped but ignored. - A handle-free client's client-facing listing has no conversation_id or _mcp_instructions, and it is minted no handle. The SDK's internal cache pass still injects, so validation stays client-agnostic. - Client- and handle-grouped calls count as connection activity, so a stdio session does not rotate early (gate finding from the JS port). - PostHogMCP: prepare_tool_list/prepare_tool_call take headers (+client_info), capture_tool_call takes request_meta. Tested: pytest posthog/test/mcp 743 passed on mcp 1.30.0, 601 passed on mcp 2.3.0; the new tests cover every spec scenario and vector, including real HTTP fleets over official, fastmcp and lowlevel servers. ruff, mypy (CI mode with baseline) and the public API snapshot pass.
Open
7 tasks done
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
- Count a client-grouped call as connection activity only on a one-client connection (no HTTP request, no token, no transport id). On a shared stateless HTTP pod, ChatGPT calls kept the instance session alive and merged two unrelated clients into one $session_id. - PostHogMCP.prepare_tool_list for a handle-free client still records conversation_id ownership, so prepare_tool_call strips an echoed handle like the automatic path does. Tested: both regressions have tests that fail on 02a4815 and pass now; pytest posthog/test/mcp 745 passed on mcp 1.30.0 and 603 on mcp 2.3.0; ruff, mypy (CI mode) and the public API check pass.
lucasheriques
changed the title
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 3cbcd29: 1 Must fix, 2 Should fix. This pull request is large, so the review ran in 4 parts with less depth than usual.
This is review round 3 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
dustinbyrne
reviewed
Nov 5, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
The earlier recognition, bounds and precedence feedback is addressed. The custom-event attribution regression remains and now also reproduces when the second caller supplies its own token.
I also reproduced session/identity merging for handle-free HTTP calls without usable metadata, output-validation failures after mixed-client FastMCP listings, and schema-invalid results after mixed manual listings. The corresponding base controls pass; details are inline.
AI-assisted review.
Resolve conflicts with main's bounded intent scans, unknown-tool event and new_session_id: keep lists_conversation_id and the client-conversation imports, and thread the resolved session through record_unknown_tool so $mcp_unknown_tool carries $mcp_session_source like every other event. Code matches the merged spec plus sdk-specs#185. Tested: uv run pytest posthog/test/mcp (776 passed, 11 skipped); ruff format and check, mypy-baseline filter, public API check, import under -W error all clean. Full suite: 6326 passed, 12 failed outside posthog/mcp (langchain, metrics autocapture fork).
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 08774e9: 1 Must fix. This pull request is large, so the review ran in 4 parts with less depth than usual.
This is review round 4 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-adding the stamphog label gives the same result unless the changed files change.
Stamphog can't auto-review this pull request because two gates refused it. The deny-list gate flagged it for matching the auth and public_api categories, which fits the session and identity changes in posthog/mcp/session.py, posthog/mcp/_instrumentation.py and posthog/mcp/posthog_mcp.py, and the signature changes recorded in references/public_api_snapshot.txt. The tier gate classified it as T2-never because it is a cross-cutting feature spanning 22 files and 1508 lines. The size gate passed, so size is not the blocker.
Please ask a human reviewer to look at it. Splitting it, for example into the client-conversation parsing module, the session resolution changes, and the PostHogMCP API changes, may make that review easier, but the sensitive areas will still need a human.
- posthog[bot] reviewed the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: auth, public_api |
| size | ✓ | 722L, 13F substantive, 1508L/22F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (1508L, 22F, cross-cutting, feat) |
| stamphog 2.7.0 | .stamphog/policy.yml @ unknown · reviewed head 08774e9 |
dustinbyrne
reviewed
Nov 9, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
Complete read-only follow-up against current base 133d7d12c3f15b04287b6d8389b0412fb7dfe6de.
The recognition table, validation, namespaced derivation and metadata precedence now match the canonical MCP contract, including automatic session-source reporting. All four required regressions from my prior review still reproduce against this updated base: custom capture inherits the previous caller's identity; missing/empty handle-free HTTP metadata merges sessions and person properties; mixed-client official FastMCP listings turn successful strict output into errors; and manual listing/preparation returns a schema-incompatible result.
I independently reproduced the existing MCP 2.x internal-list ownership report: base passes all four cases; head fails fresh raw HTTP/no-resolver with JSON-RPC -32603, while warm-listing and resolver controls pass. The adversarial pass also identified a tracked public-import removal; the old import/string-return smoke test passes on base and raises ImportError on head.
These are five P1 corrections plus one P2 compatibility correction, so I cannot recommend approval yet. Current CI is green (26 successes, one neutral), but it does not cover these sequences. The exact regressions pass on the current base and fail on this head. Full local suites also have nine identical standalone FastMCP dependency/import failures on both revisions; those are environmental, not additional PR regressions. No live PostHog delivery, provider integration, deployed profile mutation or UI verification was performed.
AI-assisted review.
…s consistent Address the 11-09 review of #1193, checked against sdk-specs main (openspec/specs/mcp-analytics/spec.md): - Priming no longer short-circuits on a client conversation id, so an in-tool McpAnalytics.capture() never reads the previous caller's session or identity. Over HTTP such a call, and a handle-free (ChatGPT) call without a usable id, primes a session reserved for that one request ("request" internally, "generated" on the wire). The metadata-derived id is never stored on shared state, and a one-client connection (stdio, SSE) keeps the process session. - Two handle-free HTTP calls without openai/session no longer share the process session and its cached person properties (spec rule 4). - A listing that declares no _mcp_instructions turns mirroring off, because mcp 1.x validates the next result against the latest listing. Ordinary list, ChatGPT list, ordinary call no longer fails an extra="forbid" result. - PostHogMCP records real ownership on a suppressed listing, so a later ordinary listing declares _mcp_instructions whenever its results carry it. - MCP 2.x internal tools/list dispatch still prepares schemas and caches ownership. It only skips the $mcp_tools_list event. - A finished call grouped by a client id refreshes stdio connection activity. Rotation happens on arrival only. Virtual tools now prime on arrival too. - Restore posthog.mcp.asgi.handshake_session_id as an async str wrapper. Tested, one red-first test per finding: - pytest posthog/test/mcp: mcp 1.30 791 passed / 11 skipped; mcp 2.3 + fastmcp 4.1.8 677 passed / 28 skipped; mcp 2.3 bare 626 passed / 60 skipped. - ruff format --check, ruff check, mypy | mypy-baseline filter, public API check, python -W error -c "import posthog": clean. Reviewer notes: rule 4 is applied only where a handle used to isolate the call. The stateless-route-without-middleware fallback for other clients is unchanged (test_a_route_without_the_middleware_keeps_the_fallback). Separately, an ordinary stdio call longer than 30 minutes is stamped with a rotated session at completion; that predates this PR and is left for its own fix.
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-adding the stamphog label gives the same result unless the changed files change.
stamphog can't approve this pull request automatically. The deny-list gate refused it because it touches sensitive areas, matching both auth and public_api. This shows up in the session and identity changes under posthog/mcp/ and in the references/public_api_snapshot.txt update. The tier gate also classified it as T2-never: a cross-cutting feature spanning 1875 lines across 23 files, which is never eligible for automated approval.
Please ask a human reviewer to take this one. It would also help to split it up, for example the client-conversation recognition module, the PostHogMCP signature changes, and the $mcp_session_source event property as separate pull requests.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: auth, public_api |
| size | ✓ | 813L, 13F substantive, 1875L/23F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (1875L, 23F, cross-cutting, feat) |
| stamphog 2.8.0 | .stamphog/policy.yml @ unknown · reviewed head 5417daa |
This branch has not been deployed
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
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.
Problem
Servers that need tool schemas and results left unchanged turn conversation IDs off. On a stateless server that builds a fresh instance per request, every tool call then gets its own
$session_id(PostHog/posthog#119784).Some clients already name the conversation in each tool call's
_meta. sdk-specs main defines which keys the SDKs read, from which clients, and in what order: "Session identity", from PostHog/sdk-specs#175 and #176. This PR implements those rules exactly. It is the twin of PostHog/posthog-js#5577.Changes
_client_conversation.CLIENT_CONVERSATION_ROWSmirrors the spec table:openai/session: 1 to 128 printable ASCII characters. Handle-free.sessionId: a UUID. Keeps the handle as a fallback.vscode.conversationId: a UUID. Keeps the handle as a fallback.UUIDs are lowercased and the nil UUID is rejected.
Recognition. A key is read only from a request whose
User-Agentor client name matches its row. The client name comes from the request's_metaclientInfofirst, then the name the adapter resolved.The client's id is session source 1 on tool calls, ahead of the handle and the transport.
tools/list, resource requests, andinitializeno longer read it. A handle echoed alongside it is stripped but ignored.Handle-free (ChatGPT). Its client-facing
tools/listhas noconversation_idor_mcp_instructions, and it is minted no handle. The SDK's internal tool-cache pass still injects, so validation stays the same for every client.Connection activity. On a one-client connection (stdio), a client-grouped call counts as activity, so the session doesn't rotate early.
$mcp_session_sourceon everyinstrument()event. The spec addition for it is docs(mcp-analytics): record which source grouped each session as $mcp_session_source sdk-specs#185.PostHogMCP.prepare_tool_listandprepare_tool_calltakeheaders, andprepare_tool_callalso takesclient_info.capture_tool_calltakesrequest_meta. A ChatGPTprepare_tool_liststill recordsconversation_idownership.On mcp 2.x low-level servers, the SDK's internal
tools/listdispatch (Mcp-Param header validation) emits no event. That part is unchanged from the earlier version of this PR.Not verified against real clients
openai/sessionstay the same for a whole ChatGPT conversation? Not verified. I have no ChatGPT connector session to log from. One way to confirm is to log_metaacross a multi-turn chat.sessionId? Partly verified, from source only. Codex adds it to tool-call_metastarting atrust-v0.156.0(2026-09-22). Older installs fall back to the handle.How I tested
pytest posthog/test/mcp: 745 passed on mcp 1.30.0 and 603 passed on mcp 2.3.0.test_client_conversation_session.pycovers every spec scenario and vector, and recognition examples.ruff,mypy(CI mode with baseline), and the public API snapshot pass.prepare_tool_listskipped recording ownership, soprepare_tool_callpassed an echoed handle to the handler.>. It predates this PR and matches posthog-js, so it's a follow-up.structuredContent. Mirroring is a MAY and the handle is still delivered.$mcp_session_sourceonly for a recognized client id (spec MAY, follow-up).🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code (pstack poteto-mode) at Lucas's direction. An earlier version ranked
openai/sessionbelow the transport session, with no recognition rules. Lucas decided the merged sdk-specs rules win, so this version implements them as written.