Repository navigation
feat(mcp): group sessions by ChatGPT, Codex and VS Code conversation ids - #1193
lucasheriques wants to merge 6 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}
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.
| await resolve_session_id( | ||
| data, mcp_session_id, token=token, client_conversation=client_conversation | ||
| ) |
There was a problem hiding this comment.
P1 · Isolate custom-event identity for metadata-only requests
When Alice's token-bearing request precedes Bob's metadata-only request on a shared server, client_conversation makes this resolver return without clearing Alice's shared session. An in-tool McpAnalytics.capture() then sends Bob's properties with Alice's session, cached distinct_id, and $set. Use request-local session and identity for custom capture.
posthog-python Compliance ReportDate: 2026-11-09T18:30:02.817716+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
|
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
left a comment
dustinbyrne
left a comment
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.
| await resolve_session_id(data, mcp_session_id, token=token) | ||
| await resolve_session_id( | ||
| data, mcp_session_id, token=token, client_conversation=client_conversation | ||
| ) |
There was a problem hiding this comment.
1. [P1] Metadata-only priming leaves the previous caller’s identity available to custom capture
IMPORTANT · REQUIRED · Introduced regression
Location / inline: posthog/mcp/_instrumentation.py:409–411, RIGHT; resolver branch posthog/mcp/session.py:188–194.
After Alice’s token-bearing request, Bob’s metadata-only request can send an in-tool custom event under Alice’s session and person. Expected: Bob’s event must not inherit another caller’s session, identity or cached person properties. Bob’s automatic tool-call event is correctly attributed, so the custom-event error can be missed.
A supported sequential caller is sufficient:
@server.tool()
async def probe(marker: str) -> str:
await analytics.capture("inside_tool", {"marker": marker})
return markerThe executed harness uses a shared FastMCP(stateless_http=True) instrumented with conversation anchoring disabled and an identify callback returning the request’s user. It sends actual JSON-RPC tools/call requests through httpx.ASGITransport, flushing after each:
- Alice: a valid encoded PostHog token with
session_id="ses_alice"; identify returns Alice and{"owner":"alice"}. - Bob: no session header,
_meta={"openai/session":"bob-chat"}; identify returns Bob. His tool calls publicanalytics.capture().
On head, Bob’s custom event contains:
{
"event": "inside_tool",
"distinct_id": "alice",
"properties": {
"$session_id": "ses_alice",
"$mcp_session_source": "transport",
"$set": {"owner": "alice"},
"marker": "bob"
}
}On base, the identical event has a fresh generated session, $process_person_profile: false, and no Alice $set. Removing only Bob’s metadata makes the head control pass too. No concurrency, private-state mutation or malformed input is needed; the token is a session carrier, not an authentication credential.
The new priming argument takes the metadata resolver’s early return before the old token-clearing path. McpAnalytics.capture() still reads shared data.session_id; _capture.py then retrieves Alice’s cached actor using that id. This violates the canonical session-isolation requirement and the existing preceding-caller isolation invariant tested in test_conversation_session.py:301–356.
Executed regression: regression_identity.py retains these assertions:
assert bob["properties"]["$session_id"] != "ses_alice"
assert bob["distinct_id"] != "alice"
assert bob["properties"].get("$set", {}).get("owner") != "alice"Executed regression commands (local paths represented by environment variables):
E="$REVIEW_EVIDENCE_DIR"
V="$REVIEW_VALIDATION_DIR"
PY=python3.11 # Python 3.11.13
cd "$V"
for rev in base 208707d; do
env -i HOME="$PWD" TMPDIR="$PWD" PATH=/usr/bin:/bin \
PYTHONDONTWRITEBYTECODE=1 /usr/bin/sandbox-exec -f "$E/sandbox.sb" \
"$PY" -S "$E/regression_identity.py" \
"$REVIEW_WORKTREE_ROOT/python-1193-$rev"
doneResults: base exit 0; head exit 1 at the session assertion. Printed output independently confirms the wrong actor and $set. Appending no-metadata to the head command gives exit 0. Full setup and stdout are retained in the evidence directory.
Consider restoring custom-capture isolation for metadata-only execution and adding this regression plus its negative control. An isolated anonymous fallback is sufficient; custom events need not acquire the metadata-derived session or resolve Bob’s identity earlier. Do not persist metadata globally or refresh an unrelated instance session’s inactivity window.
Impact is wrong attribution within the receiving PostHog project. This does not demonstrate an authentication bypass, cross-project disclosure, backend profile mutation or incorrect attribution of Bob’s automatic tool-call event. No fix was applied or tested.
|
|
||
| # Printable ASCII only, so every SDK hashes the same characters. | ||
| _CLIENT_CONVERSATION_ID_SHAPE = re.compile(r"[\x20-\x7e]{1,256}") | ||
|
|
There was a problem hiding this comment.
2. [P2/design] Reconcile the new metadata policy with the current canonical specification
QUESTION · REQUIRED acceptance decision · Intentional new behavior following a conflicting proposal
Suggested inline: posthog/mcp/session.py:46–60, RIGHT. Also affects resolution at 183–194 and manual preparation at posthog_mcp.py:608–629.
This implements sdk-specs#185, but that proposal is still open, at ed40d1a9bbef88393bf8279c61d629384021a278. The current canonical specification requires different behavior:
| Rule | This PR | Canonical spec at ffe38baa… |
|---|---|---|
Recognize openai/session |
Any caller carrying it | Recognized ChatGPT HTTP User-Agent only |
| Printable ASCII length | 1–256 | 1–128 |
| Precedence | Handle → token/transport → metadata | Client metadata → handle → transport |
| Client id plus echoed handle | Keep handle authoritative | Ignore handle for session and $mcp_conversation_id |
The canonical contract evolved after this PR snapshot; this is not a claim that the author misimplemented the earlier proposal. Nevertheless, the pending JS twin is not authority to override the current contract.
Executed public-path checks in test_contract.py use actual instrumented FastMCP/ASGI requests:
| Input / expected canonical behavior | Base | Head |
|---|---|---|
| Valid ChatGPT metadata groups calls | Fails: feature absent | Passes |
| Metadata wins over echoed handle | Fails current contract | Fails: handle-derived session |
example-client/1.0 carrying the key is ignored |
Passes | Fails: metadata groups it and suppresses handle |
| Identical 129-character values on fresh instances do not group | Passes | Fails: same derived session |
For the precedence case, the request carries recognized ChatGPT UA, _meta={"openai/session":"v1/conv-example-0001"} and handle 0190f0e8-7a6b-7c3d-9e4f-5a6b7c8d9e0f. Expected session is ses_2f5d35b7108d17c807766f2f769a1e8c; head instead uses handle-derived ses_6df45f0102a182bcd5e8dd5dad6c65a0.
These map to “Session identity” and its acceptance scenarios in the pinned spec. The precedence gap is not a passing-base regression: base lacks the capability. The other cases demonstrate changed input boundaries, not observed malicious production traffic. The overlength assertion checks session separation, not whether ChatGPT receives a handle.
The exact base/head command is the sandbox command above, replacing the script invocation with:
"$PY" -S "$E/run_pytest.py" "$REVIEW_WORKTREE_ROOT/python-1193-$rev" \
"$E/test_contract.py" --rootdir="$E" --confcutdir="$E" \
-o asyncio_mode=auto -q -sSet PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 as recorded in commands.sh. Base: 2 passed / 2 failed / 1 skipped; head: 1 passed / 3 failed / 1 skipped. The manual conformance sketch is explicitly skipped because the current helper lacks recognition context; no executed manual result is claimed.
Consider aligning this changed capability with the named canonical contract, or obtaining an explicit cross-SDK decision documenting the intentional divergence and its consequences. Keep Python, JS, tests, documentation and release wording consistent. This does not require expanding the PR into unrelated pre-existing Codex/VS Code support, handle-free listings or broader manual-API redesign.
…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.
| server, | ||
| data, | ||
| tools, | ||
| advertise_conversation_id=lifecycle.advertises_conversation_id, |
There was a problem hiding this comment.
P1 · Keep the shared validation cache compatible with both client listings
On MCP 1.30.0, a ChatGPT listing after another client's listing replaces the shared _tool_cache output schema without _mcp_instructions. The other client's next call still mirrors that property, so tools with additionalProperties: false return an output validation error instead of their result. Keep the shared validation schema compatible with both clients' results.
| if client is None or not client.handle_free: | ||
| prepared = self._inject_conversation(prepared, conversation_ownership) |
There was a problem hiding this comment.
P2 · Cache application ownership when suppressing conversation injection
With ChatGPT headers, prepare_tool_list() skips caching conversation ownership. A later prepare_tool_call() without original_tool, when client metadata is absent or invalid, treats an application-owned UUIDv7 conversation_id as an SDK handle and overrides the supplied session. Preserve application ownership even when conversation schema injection is suppressed.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| """ | ||
| await resolve_session_id(data, mcp_session_id, token=token) | ||
| await resolve_session_id( | ||
| data, mcp_session_id, token=token, client_conversation=client_conversation |
There was a problem hiding this comment.
Medium: Custom events inherit the previous caller's identity
On a shared server, passing client_conversation here makes the resolver return before updating data.session_id: after Alice's token-backed request, Bob's recognized metadata call leaves Alice's session in place even when Bob supplies a different token, so an in-tool McpAnalytics.capture() emits Bob's payload with Alice's distinct_id, $set, and $groups. Prime only the request's transport/memory session here, keeping client-conversation grouping on request-scoped event resolution.
| data, mcp_session_id, token=token, client_conversation=client_conversation | |
| data, mcp_session_id, token=token |
PR overviewThe PR groups stateless ChatGPT MCP calls by the OpenAI session identifier, updating how MCP instrumentation resolves conversation and session context. One issue remains open: on a shared server, a caller's custom analytics events can retain the previous caller's identity despite supplying a different token. This can attribute one user's event payload to another user's identity, properties, and groups. No issues have been addressed yet. Open issues (1)
Fixed/addressed: 0 · PR risk: 6/10 Reviews |
- 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
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.
| elif minted and ( | ||
| token is not None | ||
| or bool(mcp_session_id) | ||
| or (client is not None and client.handle_free) |
There was a problem hiding this comment.
P1 · Isolate handle-free HTTP requests without valid conversation metadata
When ChatGPT requests omit valid openai/session on a shared stateless HTTP server, this branch suppresses handles and prepare_request() reuses the process session. Separate users then share cached identity: handle_identify() merges Alice's properties into Bob's events and profile. Allocate a fresh request-local fallback session for sessionless HTTP calls.
| if not advertise: | ||
| self._conversation_ownership.update( | ||
| { | ||
| name: _ConversationOwnership(owned.conversation_id, False) | ||
| for name, owned in ownership.items() |
There was a problem hiding this comment.
P2 · Keep client-specific output suppression out of ownership state
After a ChatGPT listing, cached output_instructions=False permanently suppresses declarations on later ordinary listings through the ownership merge. Using the documented prepare_tool_call(original_tool=...) still mirrors _mcp_instructions, so results fail schemas with additionalProperties: false. Keep listing-specific suppression separate from cached ownership.
| if self.client_conversation is not None and on_one_client_connection: | ||
| await touch_connection_session(self.data) |
There was a problem hiding this comment.
P2 · Refresh connection activity when metadata-grouped calls finish
On stdio, a metadata-grouped call lasting over 30 minutes updates last_activity only at entry. Completion bypasses updates, so the next metadata-free call immediately rotates the connection session, violating the completion-based timeout contract required by AGENTS.md:21. Refresh connection activity when the call finishes.
dustinbyrne
left a comment
dustinbyrne
left a comment
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.
| await resolve_session_id(data, mcp_session_id, token=token) | ||
| await resolve_session_id( | ||
| data, mcp_session_id, token=token, client_conversation=client_conversation | ||
| ) |
There was a problem hiding this comment.
1. [P1] Metadata priming leaves the previous caller’s identity available to custom capture
IMPORTANT · REQUIRED · Introduced regression, persisting and amplified
Location: posthog/mcp/_instrumentation.py:415–417. Related resolver: session.py:153–157.
After Alice’s token-bearing request, Bob’s recognized metadata request sends an in-tool custom event under Alice’s session and person—even when Bob supplies his own valid token. Expected: Bob’s custom event must not inherit another caller’s cached identity.
A supported tool suffices:
@server.tool()
async def probe(marker: str) -> str:
await analytics.capture("inside_tool", {"marker": marker})
return markerThe executed public-entry harness uses a shared FastMCP(stateless_http=True), conversation anchoring disabled, request-based identification and actual JSON-RPC requests through ASGI:
- Alice sends encoded token
sid="ses_alice"; identification caches Alice and{"owner":"alice"}. - Bob sends recognized ChatGPT UA and
_meta={"openai/session":"bob-chat"}. - Bob’s tool invokes public
analytics.capture().
Head emits:
{
"event": "inside_tool",
"distinct_id": "alice",
"properties": {
"$session_id": "ses_alice",
"$mcp_session_source": "transport",
"$set": {"owner": "alice"},
"marker": "bob"
}
}Base/head comparison: without a Bob token, base uses an isolated anonymous generated session; with Bob’s own ses_bob token, base uses that session anonymously. Both head variants inherit Alice. Removing Bob’s metadata makes the head control pass. Bob’s automatic tool-call event is correctly attributed; this failure concerns earlier custom capture.
Passing client_conversation into priming takes the resolver’s early return before transport/memory priming and previous-token clearing. McpAnalytics.capture() still reads shared data.session_id (__init__.py:163–176); _capture.py:32–33 retrieves Alice’s cached actor under it.
This violates canonical session/person isolation and the preceding-caller invariant in test_conversation_session.py:301–356.
Executed regression: regression_identity.py, default and bob-token modes. Both pass base and fail head:
assert bob["properties"]["$session_id"] != "ses_alice"
assert bob["distinct_id"] != "alice"
assert bob["properties"].get("$set", {}).get("owner") != "alice"The first assertion fails; printed payloads independently confirm the wrong person and $set.
Consider separating custom-capture priming from request-local metadata grouping so metadata selection cannot bypass caller isolation. A safe anonymous fallback is sufficient; this need not introduce earlier identification or globally persist metadata sessions. Retain both token variants and the no-metadata control.
Impact limits: sequential valid requests suffice; no concurrency, private-state mutation or malformed input. Wrong attribution is within the receiving PostHog project. No authentication bypass, cross-project disclosure or live backend mutation was demonstrated.
Executed regression commands
Base: 5febc73aa74b38376a9d9cac895552686cefaca2; reviewed head: 3cbcd29b479ff2e47473a169c206b154b4c30378.
These are executed tests, not unexecuted sketches. Both pinned snapshots used Python 3.11.13 / MCP 1.28.1 with the same dependencies and sandbox. Local paths are represented by environment variables:
E="$REVIEW_EVIDENCE_DIR"
V="$REVIEW_VALIDATION_DIR"
PY="$REVIEW_PYTHON"
cd "$V"
for S in "$BASE_WORKTREE" "$HEAD_WORKTREE"; do
for MODE in default bob-token; do
env -i HOME="$V" TMPDIR="$V" PATH=/usr/bin:/bin PYTHONDONTWRITEBYTECODE=1 \
/usr/bin/sandbox-exec -f "$E/sandbox.sb" \
"$PY" -S "$E/regression_identity.py" "$S" "$MODE"
done
env -i HOME="$V" TMPDIR="$V" PATH=/usr/bin:/bin PYTHONDONTWRITEBYTECODE=1 \
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 /usr/bin/sandbox-exec -f "$E/sandbox.sb" \
"$PY" -S "$E/run_pytest.py" "$S" "$E/test_followup_regressions.py" \
--rootdir="$E" --confcutdir="$E" -o asyncio_mode=auto -q -s
doneIdentity variants: base exit 0, head exit 1. Follow-up harness: base 6 passed, head 4 failed / 2 passed. No fixes were applied or tested.
| elif minted and ( | ||
| token is not None | ||
| or bool(mcp_session_id) | ||
| or (client is not None and client.handle_free) |
There was a problem hiding this comment.
2. [P1] Handle-free HTTP requests without usable metadata share a session and person properties
IMPORTANT · REQUIRED · Existing fallback defect newly exposed by handle suppression
Location: posthog/mcp/_instrumentation.py:744–749. Related fallback: session.py:189–197.
On a shared stateless HTTP server, two recognized ChatGPT calls without valid openai/session receive the same process session. With request-based identification, Bob’s automatic event also receives Alice-only properties.
Minimal traffic, with default conversation anchoring enabled:
Alice: ChatGPT UA, no session header, tools/call, _meta={}
Bob: ChatGPT UA, no session header, tools/call, _meta={}
The same occurs with _meta={"openai/session":""}. Identification returns disjoint properties:
return UserIdentity(
distinct_id=who,
properties={who + "_only": "present"},
)Observed head output:
Alice: session S, distinct_id alice, $set {alice_only: present}
Bob: session S, distinct_id bob, $set {alice_only: present, bob_only: present}
Base mints distinct handles, derives separate sessions and keeps the properties separate.
The new branch suppresses handles as required, but falls back to shared process memory intended for one-client connections. Identity properties then merge under that shared session (_internal.py:158–168,201–204).
Canonical “Session identity” rule 4 requires a fresh session for sessionless HTTP; the explicit invalid handle-free-ID scenario requires different sessions. The new test checks fresh pods, masking shared-pod reuse.
Executed regression: test_handle_free_http_requests_stay_isolated[missing] and [empty] in test_followup_regressions.py; both pass base and fail head:
assert calls[0]["properties"]["$session_id"] != calls[1]["properties"]["$session_id"]
assert "alice_only" not in calls[1]["properties"].get("$set", {})Printed events independently establish copied properties after the first assertion fails.
Consider a fresh request-local generated fallback for sessionless HTTP, consistently reused throughout that request, while retaining connection-scoped fallback for stdio/in-memory transports. Do not restore ChatGPT handles or mint a different ID at every resolver invocation.
Impact limits: missing/invalid metadata is an explicit contract boundary; its live ChatGPT frequency is unknown. Normal valid metadata is not claimed to merge automatic events. Cross-user $set emission is observed; deployed person updates were not executed.
Executed regression commands
Base: 5febc73aa74b38376a9d9cac895552686cefaca2; reviewed head: 3cbcd29b479ff2e47473a169c206b154b4c30378.
These are executed tests, not unexecuted sketches. Both pinned snapshots used Python 3.11.13 / MCP 1.28.1 with the same dependencies and sandbox. Local paths are represented by environment variables:
E="$REVIEW_EVIDENCE_DIR"
V="$REVIEW_VALIDATION_DIR"
PY="$REVIEW_PYTHON"
cd "$V"
for S in "$BASE_WORKTREE" "$HEAD_WORKTREE"; do
for MODE in default bob-token; do
env -i HOME="$V" TMPDIR="$V" PATH=/usr/bin:/bin PYTHONDONTWRITEBYTECODE=1 \
/usr/bin/sandbox-exec -f "$E/sandbox.sb" \
"$PY" -S "$E/regression_identity.py" "$S" "$MODE"
done
env -i HOME="$V" TMPDIR="$V" PATH=/usr/bin:/bin PYTHONDONTWRITEBYTECODE=1 \
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 /usr/bin/sandbox-exec -f "$E/sandbox.sb" \
"$PY" -S "$E/run_pytest.py" "$S" "$E/test_followup_regressions.py" \
--rootdir="$E" --confcutdir="$E" -o asyncio_mode=auto -q -s
doneIdentity variants: base exit 0, head exit 1. Follow-up harness: base 6 passed, head 4 failed / 2 passed. No fixes were applied or tested.
| server, | ||
| data, | ||
| tools, | ||
| advertise_conversation_id=lifecycle.advertises_conversation_id, |
There was a problem hiding this comment.
3. [P1] Mixed-client listings turn successful FastMCP results into validation errors
IMPORTANT · REQUIRED · Introduced regression
Location: posthog/mcp/_instrument_fastmcp.py:296–300. Related mirroring/state: :182–185, _instrumentation.py:1270–1273.
Ordinary sequential requests on one official MCP 1.x FastMCP instance turn a successful strict-schema tool result into an error:
1. Ordinary client: tools/list
2. ChatGPT client: tools/list
3. Ordinary client: tools/call
Example tool:
class StrictResult(BaseModel):
model_config = ConfigDict(extra="forbid")
value: int
@server.tool()
def probe() -> StrictResult:
return StrictResult(value=7)Head returns:
{
"content": [{
"type": "text",
"text": "Output validation error: Additional properties are not allowed ('_mcp_instructions' was unexpected)"
}],
"isError": true
}Expected: the successful application result reaches the caller. Base does so.
The first listing sets shared tool_output_instructions["probe"]=True. The ChatGPT listing replaces the framework’s cached Tool with a strict schema lacking _mcp_instructions, but leaves that flag true. The next ordinary call mirrors the forbidden property; the outer framework validator rejects it.
Installed official MCP source confirms listings populate _tool_cache and call results are validated against it. PostHog’s FastMCP mirror executes inside that validator’s call path.
Canonical observer-only instrumentation forbids converting a successful result into failure. Optional mirroring still requires compatible declarations; official MCP requires structured results to conform to their output schema.
Executed regression: test_automatic_mixed_client_listings_keep_output_validation[other-then-chatgpt]; base passes, head fails:
assert not result.get("isError"), result
assert result["structuredContent"]["value"] == 7The head’s ChatGPT-first-only control passes. The PR’s dismissal of that simpler ordering does not cover this failure.
Consider separating client-specific advertisements from a framework validation cache compatible with every result instrumentation can write, preserving ChatGPT’s unchanged listing. Retain both ordering cases.
Impact limits: reproduced on official MCP 1.28.1, shared instance, anchoring enabled and strict structured output. No new-regression execution claim is made for MCP 2.x or standalone FastMCP. This is actual response failure, not merely loss of optional mirroring.
Executed regression commands
Base: 5febc73aa74b38376a9d9cac895552686cefaca2; reviewed head: 3cbcd29b479ff2e47473a169c206b154b4c30378.
These are executed tests, not unexecuted sketches. Both pinned snapshots used Python 3.11.13 / MCP 1.28.1 with the same dependencies and sandbox. Local paths are represented by environment variables:
E="$REVIEW_EVIDENCE_DIR"
V="$REVIEW_VALIDATION_DIR"
PY="$REVIEW_PYTHON"
cd "$V"
for S in "$BASE_WORKTREE" "$HEAD_WORKTREE"; do
for MODE in default bob-token; do
env -i HOME="$V" TMPDIR="$V" PATH=/usr/bin:/bin PYTHONDONTWRITEBYTECODE=1 \
/usr/bin/sandbox-exec -f "$E/sandbox.sb" \
"$PY" -S "$E/regression_identity.py" "$S" "$MODE"
done
env -i HOME="$V" TMPDIR="$V" PATH=/usr/bin:/bin PYTHONDONTWRITEBYTECODE=1 \
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 /usr/bin/sandbox-exec -f "$E/sandbox.sb" \
"$PY" -S "$E/run_pytest.py" "$S" "$E/test_followup_regressions.py" \
--rootdir="$E" --confcutdir="$E" -o asyncio_mode=auto -q -s
doneIdentity variants: base exit 0, head exit 1. Follow-up harness: base 6 passed, head 4 failed / 2 passed. No fixes were applied or tested.
| self._conversation_ownership.update( | ||
| { | ||
| name: _ConversationOwnership(owned.conversation_id, False) | ||
| for name, owned in ownership.items() |
There was a problem hiding this comment.
4. [P1] A ChatGPT manual listing suppresses later declarations while ordinary results still mirror them
IMPORTANT · REQUIRED · Introduced regression
Location: posthog/mcp/posthog_mcp.py:903–907. Related ownership merge: 899–901.
A shared manual dispatcher can advertise a strict schema and then return data violating it after serving ChatGPT followed by an ordinary client:
chatgpt = {"user-agent": "openai-mcp/1.0.0 (ChatGPT)"}
other = {"user-agent": "example-client/1.0"}
client.prepare_tool_list([original], context=False, headers=chatgpt)
[listed] = client.prepare_tool_list([original], context=False, headers=other)
call = client.prepare_tool_call(
"probe", {}, original_tool=original, headers=other,
)
prepared = client.prepare_tool_result(
{"content": [{"type": "text", "text": "ok"}],
"structuredContent": {"value": 7}},
call,
)For an original plain-object output schema declaring integer value and additionalProperties: false, head’s ordinary listing does not declare _mcp_instructions, but its result contains it. Expected: the schema and returned structured content remain compatible.
The new non-advertising branch stores output_instructions=False in persistent ownership. The monotonic AND merge keeps it false for later ordinary listings. However, recommended prepare_tool_call(original_tool=...) independently recognizes the extendable original schema and enables mirroring.
This follows the documented original-tool preparation path. Canonical conversation rules permit mirroring only with a declaration; official MCP requires schema-conforming structured results.
Executed regression: test_manual_client_specific_listing_does_not_poison_ownership[chatgpt-then-other]; base passes and head fails:
jsonschema.validate(
prepared.result["structuredContent"], listed["outputSchema"]
)
# ValidationError: '_mcp_instructions' was unexpectedThe head’s ordinary-only control passes. Base lacks headers, so the harness uses equivalent supported helpers without those new arguments; this is not a signature/import failure.
Consider keeping per-client suppression separate from persistent application-field ownership, so an ordinary listing advertises the declaration its ordinary result path will use. Preserve genuine application ownership and the two regression controls.
Impact limits: actual public preparation methods and returned schema/result were exercised. Strict validating clients can reject the response; no end-to-end MCP client was run for this manual case. This is a forbidden returned field, not merely an omitted optional mirror.
Executed regression commands
Base: 5febc73aa74b38376a9d9cac895552686cefaca2; reviewed head: 3cbcd29b479ff2e47473a169c206b154b4c30378.
These are executed tests, not unexecuted sketches. Both pinned snapshots used Python 3.11.13 / MCP 1.28.1 with the same dependencies and sandbox. Local paths are represented by environment variables:
E="$REVIEW_EVIDENCE_DIR"
V="$REVIEW_VALIDATION_DIR"
PY="$REVIEW_PYTHON"
cd "$V"
for S in "$BASE_WORKTREE" "$HEAD_WORKTREE"; do
for MODE in default bob-token; do
env -i HOME="$V" TMPDIR="$V" PATH=/usr/bin:/bin PYTHONDONTWRITEBYTECODE=1 \
/usr/bin/sandbox-exec -f "$E/sandbox.sb" \
"$PY" -S "$E/regression_identity.py" "$S" "$MODE"
done
env -i HOME="$V" TMPDIR="$V" PATH=/usr/bin:/bin PYTHONDONTWRITEBYTECODE=1 \
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 /usr/bin/sandbox-exec -f "$E/sandbox.sb" \
"$PY" -S "$E/run_pytest.py" "$S" "$E/test_followup_regressions.py" \
--rootdir="$E" --confcutdir="$E" -o asyncio_mode=auto -q -s
doneIdentity variants: base exit 0, head exit 1. Follow-up harness: base 6 passed, head 4 failed / 2 passed. No fixes were applied or tested.
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).
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.
| if _is_internal_dispatch(ctx): | ||
| return await original(ctx, params) |
There was a problem hiding this comment.
P1 · Preserve ownership caching on internal tool listings
On a fresh MCP 2.x low-level HTTP instance, this shortcut skips cache_listed_tool_ownership. Injected context and conversation_id then reach the application handler, causing strict dispatchers to return -32603. This breaks the injection contract required by AGENTS.md:21; preserve ownership bookkeeping while suppressing internal listing events.
![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
left a comment
dustinbyrne
left a comment
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.
| await resolve_session_id( | ||
| data, mcp_session_id, token=token, client_conversation=client_conversation | ||
| ) |
There was a problem hiding this comment.
[P1] Preserve custom-capture priming when metadata wins
On a shared official FastMCP HTTP server, Alice's token-bearing call followed by Bob's valid ChatGPT metadata call leaves Alice's shared session available to analytics.capture() inside Bob's tool. Bob's custom event contains Alice's distinct_id and $set, even when Bob supplies his own valid token. The metadata resolver returns before the previous transport/memory priming behavior, while custom capture still reads that shared state. Both sequential variants pass on base and fail on head. Please keep metadata grouping request-local while still priming or isolating custom capture before the tool body; do not persist the metadata-derived ID globally.
| elif minted and ( | ||
| token is not None | ||
| or bool(mcp_session_id) | ||
| or (client is not None and client.handle_free) | ||
| ): |
There was a problem hiding this comment.
[P1] Isolate sessionless HTTP requests when the client ID is unusable
Recognized ChatGPT calls with missing or empty openai/session, no transport/token and default anchoring now reach the shared process fallback because handle minting is suppressed. Two callers get the same $session_id, and Bob's event contains Alice-only $set properties from the shared identity cache. Both variants pass on base and fail on head. The canonical sessionless-HTTP and invalid-handle-free-ID requirements call for different sessions here. Please allocate one request-local fallback for sessionless HTTP and reuse it during that request, while retaining connection fallback for stdio. Restoring ChatGPT handles would conflict with the intended policy.
| _inject_tool_schemas( | ||
| server, | ||
| data, | ||
| tools, | ||
| advertise_conversation_id=lifecycle.advertises_conversation_id, |
There was a problem hiding this comment.
[P1] Keep framework output validation compatible across client listings
On official MCP 1.28.1, ordinary tools/list → ChatGPT tools/list → ordinary tools/call makes a successful extra="forbid" result return isError:true: _mcp_instructions is an unexpected additional property. The first listing enables shared mirroring, while the ChatGPT listing supplies a strict unextended schema to the framework's validation cache without clearing that mirroring decision. The same sequence passes on base, and the ChatGPT-only control passes on head. Please separate client-specific advertisements from the cache used to validate instrumented results. This is an actual successful-call regression, not merely optional mirroring loss.
| if not advertise: | ||
| self._conversation_ownership.update( | ||
| { | ||
| name: _ConversationOwnership(owned.conversation_id, False) | ||
| for name, owned in ownership.items() |
There was a problem hiding this comment.
[P1] Keep manual output declarations separate from client suppression
A shared PostHogMCP preparing ChatGPT's listing first stores output_instructions=False in persistent ownership; the AND merge prevents a later ordinary listing from declaring _mcp_instructions. But the documented ordinary prepare_tool_call(original_tool=original) path recomputes ownership and enables it in the result. With an original strict object output schema, the ordinary advertised schema rejects the ordinary prepared structured content. Equivalent calls pass on base and the ordinary-only head control passes. Please keep per-client suppression separate from application-field ownership so the schema and result agree; preserve genuine application-owned fields.
| if _is_internal_dispatch(ctx): | ||
| return await original(ctx, params) |
There was a problem hiding this comment.
[P1] Retain ownership bookkeeping for internal MCP 2.x listings
Corroborates existing thread #1193 (comment) . Official MCP 2.3.0 modern HTTP invokes the current tools/list before dispatch when arguments are nonempty. This early return suppresses ownership caching as well as the synthetic event. A fresh raw server with an original query-only strict schema then receives injected context, llm_model and conversation_id, returning JSON-RPC -32603 instead of success. Base strips them and passes; head fails only fresh/no optional resolver, while prior-listing and configured-resolver controls pass. Please suppress the event but retain original-schema/ownership preparation. The framework already supplies the definition; unknown-ownership fail-closed behavior does not require discarding that available proof.
| from ._internal import MCPAnalyticsData, get_server_tracking_data, tracking_key | ||
| from .logger import log, warn | ||
| from .session import handshake_session_id, new_session_id | ||
| from .session import handshake_session, new_session_id |
There was a problem hiding this comment.
[P2] Preserve the tracked handshake import and return contract
The base API snapshot explicitly exposes posthog.mcp.asgi.handshake_session_id, but this refactor removes the definition/import and replaces it with handshake_session. The old public import/string-return smoke test passes on base and raises ImportError on head. CONTRIBUTING.md treats snapshot changes as public API, and the changeset is minor. Please retain a compatibility wrapper and the old import with its asynchronous string return; directly aliasing the new tuple-returning helper would still break callers. No known external consumer was found, so this is P2 rather than an established application outage.
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.