Skip to content

feat(mcp): emit $mcp_error_message and $mcp_error_type - #882

Merged
gesh merged 29 commits into
mainfrom
posthog-code/mcp-error-properties
Aug 21, 2026
Merged

feat(mcp): emit $mcp_error_message and $mcp_error_type#882
gesh merged 29 commits into
mainfrom
posthog-code/mcp-error-properties

Conversation

@gesh

@gesh gesh commented Aug 21, 2026

Copy link
Copy Markdown
Member

Stacked on #881 — review that first; this branch's diff against it is small.

The problem

Why an MCP tool call failed only ever reached PostHog on the sibling $exception event. The failures view reads the scalars off the primary event, so every Python-backed MCP server shows empty error rows by default — and setting enable_exception_autocapture=False drops the reason entirely.

Reported from the field by a team who built against MCP Analytics with a hand-rolled Python dispatcher:

$mcp_error_message is emitted by the TypeScript SDK only. The Python SDK never sends it, but the failures UI still expects it, so every Python-backed server shows empty error rows by default.

The change

$mcp_error_type explicit error_type if given, else the thrown class (ValueError)
$mcp_error_message the exception message
Emitted on any event with is_error$mcp_tool_call, $mcp_tools_list

Both are read from the same $exception_list the sibling event carries, so the two surfaces can never disagree, and the message inherits the existing 2048-character cap for free (truncation runs before the event mapping). Exactly how @posthog/mcp derives them.

PostHogMCP.capture_tool_call() and capture_tools_list() gain an optional error_type so a custom dispatcher can send a coarse category — "validation", "timeout" — that means something to the product, where ValueError doesn't. Also parity with the TS SDK's errorType.

Tests

9 new tests, both SDK majors: message + type on failure, explicit type beating the thrown class, string errors, no properties on success, the sibling agreeing, the message surviving with autocapture disabled (the point of the change), truncation inheritance, tools/list failures, and an end-to-end check through an instrumented server.

Suite: 197 → 206 (v1) · 182 → 191 (v2). ruff, mypy, public-API snapshot clean.


Created with PostHog Code

gesh added 14 commits August 20, 2026 14:47
…nd v2

The suite now runs twice in CI: the existing `tests` matrix stays on
mcp>=1.26,<2, and a new `tests-mcp-v2` job swaps in mcp>=2,<3 (spec
2026-07-28) and runs posthog/test/mcp. A conftest splits collection by
installed major, since each major's seams fail at import on the other.

New coverage, red until the SDK changes land:
- test_v2_mcpserver / test_v2_lowlevel: the v2 adapters driven directly,
  mirroring the v1 files (capture, errors, intent injection/stripping,
  identify, report_missing, idempotency, late registration)
- test_v2_wire_dual_era: raw JSON-RPC over the real streamable-http app in
  both protocol eras — stateless topology, envelope identity, no session
  header on 2026-07-28, conversation-anchored sessions across two fresh
  instances, and the self-encoded token surviving pods on the legacy era
- test_conversation_session: the cross-SDK session derivation contract
  (byte-for-byte vectors against posthog-js), the minted-shape guard, and
  v1 anchoring flows
- test_no_crash: instrument() degrades to a logged no-op instead of
  crashing, on both majors

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
instrument() now works on mcp 1.x and 2.x:

- New _instrument_v2 adapters: the high-level MCPServer (renamed FastMCP)
  wraps ToolManager.call_tool — the seam every dispatch routes through —
  and the low-level Server wraps the string-keyed handler registry through
  the public add_request_handler/get_request_handler API, late
  registrations included (the posthog-js#4449 lesson). Strip-vs-leave
  policy per entry point matches the 1.x pair: the high-level path strips
  injected context/conversation_id (v2 validates against the function
  signature), the raw low-level path leaves them optional in the schema.
- _compatibility probes are import-tolerant and shape-based (posthog-js
  ADR-0005): the old module-level `from mcp.server.fastmcp import ...`
  raised ImportError straight out of instrument() on mcp>=2, crashing the
  host app. Unsupported servers now degrade to a logged no-op handle.
- Conversation-anchored sessions (posthog-js ADR-0004): with
  enable_conversation_id, $session_id is derived deterministically from
  the agent-echoed conversation_id — the only correlation that survives
  2026-07-28's per-request instances. Byte-for-byte parity with
  @posthog/mcp (new export derive_session_id_from_conversation), the
  minted-shape (uuidv7) guard so invented handles can't merge unrelated
  callers, lowercased echoes, and the prompt-back riding errored results
  so a first-call failure keeps the conversation together. Applies to the
  1.x adapters too — anchoring is not era-gated.
- Dual-shape attribute reads (isError/is_error, inputSchema/input_schema,
  clientInfo/client_info); captured payloads dump by_alias so both majors
  emit the camelCase wire shape.
- Version advisory widened to mcp>=1.26,<3.

No new runtime dependencies; mcp stays a lazily-imported peer dependency.
jlowin's fastmcp keeps the 1.x seams (it pins mcp<2).

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
The v2 adapters hand the raw request ctx to identify/event_properties/
intent_fallback callbacks via `extra` so hosts can read headers — but
handle_identify embedded the whole dict into the captured $identify
parameters, where the sanitizer leaves opaque objects untouched and
truncation stringifies them: whatever the context repr carries (headers,
transport state) would ship to PostHog without key-based redaction.

Callbacks keep the full extra; captured parameters now carry only
JSON-safe scalars (e.g. session_id).

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Reshape tests-mcp-v2 into a tests-mcp matrix over {v1, v2} x {3.10, 3.14},
so the checks list shows an explicit per-major signal instead of the v1
side being buried inside the whole-repo tests matrix. The v1 leg uses the
lockfile's mcp 1.x as-is; the v2 leg swaps in mcp>=2,<3 and drops jlowin
fastmcp (pins mcp<2). The main tests matrix is unchanged.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Manual testing against Claude Code found the conversation feature inert for
any tool that declares an output schema: clients that read structuredContent
never render the content blocks, so the [SERVER]: Reuse conversation_id text
block was invisible and the agent had nothing to echo back. Every call minted
a fresh handle, and each landed in its own session.

Ports the second delivery channel from @posthog/mcp (ADR-0004, posthog-js
 #4430/#4431), which measured the same 0% echo rate before fixing it:

- declare an optional `_mcp_instructions` key on the tool's advertised
  outputSchema at tools/list (never `required`)
- mirror {conversation_id} into the result's structuredContent on *every*
  response, not just the minting one, so an agent that dropped the handle
  can read it back

The declaration is what makes the write safe — clients validate
structuredContent against the advertised schema, so an undeclared key fails
the customer's whole tool result under additionalProperties: false. Only
tools we declared on are ever written to, and an instance that never served
a tools/list fails closed. Composed schemas (oneOf/allOf/anyOf/$ref) and
tools owning the key are skipped; the text block still carries them.

Wired into all four adapters (v1 FastMCP, v1 low-level, jlowin fastmcp, v2)
with shape-tolerant reads: the (content, structured) tuple from FastMCP 1.x's
convert_result path, CallToolResult models (structuredContent on 1.x,
structured_content on 2.x), and plain dicts.

Verified on the live playground server: outputSchema declares the key and
structuredContent carries the handle alongside the tool's own payload.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Adding v2 support left the callback contract divergent: the v2 adapters
passed the request context as extra["ctx"] while the v1 adapters passed
only session_id, so an identify() written against one major silently got
nothing on the other — and on v1 there was no way to reach headers from
extra at all. Header-based identification is the main use of identify, and
the failure is invisible: no headers, no distinct id, every event
anonymous.

- all four adapters now pass the SDK's own per-request context as
  extra["ctx"], unchanged and identically shaped across majors
- new exported get_request_headers(extra) flattens Starlette Headers, plain
  mappings, or any iterable of pairs into a lowercase-keyed dict; returns
  None on stdio and never raises
- the context object still never reaches an event: the scalar projection
  added earlier for the $identify capture strips it

Ports the intent of @posthog/mcp's getRequestHeaders (ADR-0006: hand hosts
the raw context rather than synthesising a fake uniform shape, and give
them one helper for the thing they actually need).

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
…tx read

Four defects found by the qa-swarm delegation pass (fable technical lens +
opus security lens), all reproduced before fixing:

HIGH — strict output schemas broke after a tool-cache rebuild. mcp 1.x
validates a call's structuredContent against its *cached* tool definition,
and rebuilds that cache from the internal `req=None` listing pass whenever
an unlisted tool name is called (our own advertised `get_more_tools` does
it). We skipped injecting on that pass, so the cache lost the
`_mcp_instructions` declaration while the mirror kept writing the key:
every later call to a tool with `additionalProperties: false` came back
`isError: "Additional properties are not allowed"`. The internal pass now
gets the same injections; only capture is skipped.

HIGH — cross-caller handle leak. The mirror and the prompt-back mutated the
result object in place, so a tool returning a shared/cached CallToolResult
pinned one conversation's handle onto it and served it to every later
caller — and since the handle outranks the transport session, that
collapses unrelated clients into one session, where identity merging can
write one user's person properties onto another's profile. Both channels
now copy (model_copy) instead of mutating; the low-level path rewraps and
returns the copy.

MEDIUM — a re-listing disabled the mirror. add_instructions_to_output_schema
could not tell its own prior declaration from a customer's, so servers that
return persistent Tool objects flipped ownership to False on the second
tools/list, silently switching off the feature for the schema-reading
clients it exists for (and blaming the customer in the log). It now
recognises our declaration by its description sentinel.

MEDIUM — `getattr(context, "request_context", None)` was unguarded:
FastMCP's property *raises* outside a request, and getattr only swallows
AttributeError, so the public `FastMCP.call_tool()` entry point started
raising ValueError from analytics. Read it through a guarded helper, like
every other context reader here.

Also de-footguns the get_request_headers docstring: its example resolved an
Authorization header to a user id rather than handing back the raw token,
with an explicit warning that person properties and custom event properties
are not redacted.

Regression tests for all four, each verified to fail without its fix.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Reuse existing helpers in the v2 adapter instead of hand-rolled copies:
- _ctx_mcp_session_id now goes through get_request_headers +
  read_mcp_session_header, which already handle case-insensitive keys,
  list-valued headers and whitespace — strictly better than the raw
  ctx.request.headers read it replaces
- shared schema_has_param() in _context_parameters (was duplicated between
  the lowlevel and v2 adapters)
- shared params_to_request_dict() in _instrumentation (request_to_dict now
  delegates to it)
- one tool-manager lookup per call instead of two when checking ownership
  of both injected parameters

The three adapters stay separate on purpose — they hook different SDK seams
with different semantics; only genuinely identical logic is shared.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Round 2 of the review loop caught that the previous commit's cache-rebuild
fix stopped at the FastMCP adapter. The raw low-level v1 adapter has the
identical exposure on the *input* side, and it is worse there: this adapter
advertises `context`/`conversation_id` without stripping them, relying on
the advertised schema doubling as the call's validation schema. So a cache
rebuilt from the un-injected internal listing pass rejects exactly the
arguments the SDK told the agent to send —
"Input validation error: Additional properties are not allowed ('context'
was unexpected)" — for any tool with `additionalProperties: false`. One
unknown tool name (or a fresh stateless instance) triggers it, and the
conversation prompt-back tells the agent to keep sending the argument, so
it persists until the next client-facing tools/list.

The internal pass now gets the same injections here too, via a shared
`_inject_tool_schemas` helper mirroring the FastMCP one. Regression test
verified to fail without the fix.

Also from round 2:
- `_tool_own_properties_v2` fails closed on a malformed schema again — the
  simplify pass had narrowed the guard so a non-dict `properties` would
  raise (or answer by substring) in the tool-call hot path
- finish the `schema_has_param` dedup the simplify pass started: the
  low-level adapter now uses the shared helper instead of its own copy

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Round 3 of the review loop. On the jlowin-FastMCP path the adapter strips
the injected `context`/`conversation_id` before dispatch, because that SDK
validates tool arguments against the function signature. But the listing
wrapper marked `context` *required* in the advertised schema — which is
also the schema mcp 1.x's low-level server validates the (already stripped)
arguments against. Under `FastMCP(strict_input_validation=True)` every call
therefore failed with "Input validation error: 'context' is a required
property".

Latent on main, where an un-injected cache rebuild intermittently cured it;
the previous commit's internal-pass injection made it permanent. Advertise
`context` as optional on this path instead — requiring a parameter the
validator can never see is self-contradictory, and losing the "required"
nudge only softens intent capture, where the alternative is a hard failure
of the customer's tool call.

Regression test added; the default `strict_input_validation=False` and both
MCP SDK 2.x paths were unaffected either way.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
The changeset claimed "SDK 1.x paths are unchanged", which stopped being
true as the PR grew: conversation-anchored sessions, the structuredContent
delivery channel, the uuidv7 handle guard, the prompt-back on errored
results and the callback context all reach v1 servers, and three separately
reproduced bugs on v1 paths were fixed. Since no mainstream client speaks
the 2026-07-28 era yet, that half is what current users actually get.

Restructured into v2 support / cross-SDK parity / fixes affecting existing
1.x users, and calls out the two deliberate behavioural changes.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Why a call failed only ever reached PostHog on the sibling $exception
event. The failures view reads the scalars off the primary event, so every
Python-backed MCP server showed empty error rows — reported from the field
after a team built against MCP Analytics with a hand-rolled dispatcher — and
disabling exception autocapture dropped the reason on the floor entirely.

Both values come from the same $exception_list the sibling carries, so the
two surfaces cannot disagree, and the message inherits the existing
2048-char cap because truncation runs before the event mapping. An explicit
error_type wins over the thrown class name: PostHogMCP.capture_tool_call()
and capture_tools_list() now take one, so a custom dispatcher can send a
coarse category ("validation", "timeout") that means something to the
product where "ValueError" does not.

Parity with @posthog/mcp, which derives both the same way.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
@gesh
gesh requested a review from a team as a code owner August 21, 2026 09:07
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

posthog-python Compliance Report

Date: 2026-08-21 12:23:07 UTC
Duration: 256377ms

✅ All Tests Passed!

111/111 tests passed


Capture_V1 Tests

94/94 tests passed

View Details
Test Status Duration
Endpoint And Method.Targets V1 Endpoint 518ms
Endpoint And Method.Does Not Use Legacy Endpoints 511ms
Required Headers.Has Authorization Bearer Header 510ms
Required Headers.Has Content Type Json 510ms
Required Headers.Has Posthog Sdk Info Format 510ms
Required Headers.Has Posthog Attempt Header 510ms
Required Headers.Has Posthog Request Id 511ms
Required Headers.Has Posthog Request Timestamp 510ms
Required Headers.Has User Agent 510ms
Body Format.Body Has Created At And Batch 510ms
Body Format.No Api Key In Body 510ms
Body Format.No Sent At In Body 510ms
Event Format.Event Has Required Root Fields 510ms
Event Format.Event Uuid Is Valid 511ms
Event Format.Event Timestamp Is Rfc3339 510ms
Event Format.Distinct Id Is String 510ms
Event Format.Distinct Id At Root Not Properties 510ms
Event Format.Custom Properties Preserved 510ms
Event Format.Set Properties Preserved 510ms
Event Format.Set Once Properties Preserved 510ms
Event Format.Groups Properties Preserved 511ms
Event Format.Sdk Generates Uuid If Not Provided 510ms
Event Format.Event Has Required Root Fields Batch 514ms
Event Format.Event Uuid Is Valid Batch 513ms
Event Format.Event Timestamp Is Rfc3339 Batch 514ms
Event Format.Distinct Id Is String Batch 513ms
Event Format.Distinct Id At Root Not Properties Batch 514ms
Event Format.Custom Properties Preserved Batch 513ms
Event Format.Set Properties Preserved Batch 514ms
Event Format.Set Once Properties Preserved Batch 513ms
Event Format.Groups Properties Preserved Batch 514ms
Event Format.Sdk Generates Uuid If Not Provided Batch 514ms
Batch Behavior.Multiple Events In Single Batch 517ms
Batch Behavior.Batch Envelope Smoke 515ms
Batch Behavior.Flush With No Events Sends Nothing 507ms
Batch Behavior.Flush At Triggers Batch 1010ms
Batch Behavior.Created At Reflects Batch Creation Time 511ms
Deduplication.Generates Unique Uuids 518ms
Deduplication.Different Events Same Content Different Uuids 512ms
Deduplication.Preserves Uuid On Retry 6515ms
Deduplication.Preserves Timestamp On Retry 6520ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry 6524ms
Deduplication.No Duplicate Events In Batch 518ms
Header Behavior On Retry.Attempt Header Starts At One 510ms
Header Behavior On Retry.Attempt Header Increments On Retry 13522ms
Header Behavior On Retry.Request Id Preserved On Retry 6520ms
Header Behavior On Retry.Different Requests Have Different Request Ids 3020ms
Header Behavior On Retry.Request Timestamp Changes On Retry 6520ms
Response Format Validation.Success Response Has Uuid Keyed Results 512ms
Response Format Validation.Success Response Has Ok For Each Event 514ms
Response Format Validation.Success No Retry After When All Ok 513ms
Response Format Validation.Success Retry After Present When Retry Events 1516ms
Response Format Validation.Success No Retry After When Drop Only 512ms
Response Format Validation.Response Echoes Request Id 510ms
Retry Behavior.Retries On 408 6516ms
Retry Behavior.Retries On 500 6520ms
Retry Behavior.Retries On 503 8521ms
Retry Behavior.Retries On 504 6516ms
Retry Behavior.Retryable Errors Have Retry After 3517ms
Retry Behavior.Respects Retry After On Retryable Error 11523ms
Retry Behavior.Does Not Retry On 400 2514ms
Retry Behavior.Does Not Retry On 401 2514ms
Retry Behavior.Does Not Retry On 402 2514ms
Retry Behavior.Does Not Retry On 413 2512ms
Retry Behavior.Does Not Retry On 415 2514ms
Retry Behavior.Non Retryable Errors Have No Retry After 2514ms
Retry Behavior.Implements Backoff 22537ms
Retry Behavior.Max Retries Respected 22524ms
Partial Batch Handling.Handles 200 Full Success 2511ms
Partial Batch Handling.Handles 200 With All Ok 3519ms
Partial Batch Handling.Does Not Retry Dropped Events 3513ms
Partial Batch Handling.Does Not Retry Limited Events 3516ms
Partial Batch Handling.Prunes Ok Events On Partial Retry 6522ms
Partial Batch Handling.Prunes Dropped Events On Partial Retry 6522ms
Partial Batch Handling.Retries Only Retry Events From Partial 6519ms
Partial Batch Handling.Partial Retry Preserves Uuids 6522ms
Partial Batch Handling.Partial Retry Attempt Header Increments 6522ms
Partial Batch Handling.Partial Retry Request Id Preserved 6521ms
Partial Batch Handling.Respects Retry After On Partial 8517ms
Partial Batch Handling.Unknown Result Treated As Terminal 3518ms
Partial Batch Handling.Mixed Ok Drop Limited No Retry 3519ms
Compression.Sends Gzip Content Encoding 511ms
Compression.No Content Encoding When Disabled 510ms
Compression.Compressed Body Is Decompressible 511ms
Error Handling.Does Not Retry On Unknown 4Xx 2513ms
Event Options.Cookieless Mode Override 511ms
Event Options.Disable Skew Correction Override 510ms
Event Options.Process Person Profile Override 510ms
Event Options.Product Tour Id Override 510ms
Event Options.Unset Options Omitted 510ms
Event Options.Options Override In Batch 514ms
Geoip And Historical Migration.Geoip Disable Injected Into Properties 509ms
Geoip And Historical Migration.Historical Migration Set In Body 510ms
Geoip And Historical Migration.Historical Migration Absent By Default 510ms

Feature_Flags Tests

17/17 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id 11ms
Request Payload.Flags Request Uses V2 Query Param 10ms
Request Payload.Flags Request Hits Flags Path Not Decide 9ms
Request Payload.Flags Request Omits Authorization Header 9ms
Request Payload.Token In Flags Body Matches Init 9ms
Request Payload.Groups Round Trip 9ms
Request Payload.Groups Default To Empty Object 10ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False 9ms
Request Payload.Disable Geoip Omitted Defaults To False 9ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key 9ms
Request Lifecycle.No Flags Request On Init Alone 4ms
Request Lifecycle.No Flags Request On Normal Capture 509ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests 14ms
Request Lifecycle.Mock Response Value Is Returned To Caller 9ms
Retry Behavior.Retries Flags On 502 313ms
Retry Behavior.Retries Flags On 504 314ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event 512ms

@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Security Review

The new primary-event mapping can transmit unsanitized exception text, including when exception autocapture is disabled. Exception-derived messages should pass through the existing MCP sensitive-value sanitizer before being assigned to $mcp_error_message.

Prompt To Fix All With AI
### Issue 1
posthog/mcp/_posthog_events.py:179-181
**Error message bypasses sanitization**

When a failed MCP call's exception message contains a token or another sensitive value, `_add_error_details` copies the unsanitized `$exception_list` value into `$mcp_error_message`, causing sensitive text to be transmitted on the primary event even when exception autocapture is disabled.

**How this was verified:** The MCP sanitizer processes response, parameters, and user intent but not the error payload read by this mapping.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "feat(mcp): emit $mcp_error_message and $..." | Re-trigger Greptile

Comment on lines +179 to +181
message = first.get("value")
if message:
properties[_P.ERROR_MESSAGE] = message

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security Error message bypasses sanitization

When a failed MCP call's exception message contains a token or another sensitive value, _add_error_details copies the unsanitized $exception_list value into $mcp_error_message, causing sensitive text to be transmitted on the primary event even when exception autocapture is disabled.

How this was verified: The MCP sanitizer processes response, parameters, and user intent but not the error payload read by this mapping.

Knowledge Base Used: MCP Instrumentation (posthog/mcp)

Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/mcp/_posthog_events.py
Line: 179-181

Comment:
**Error message bypasses sanitization**

When a failed MCP call's exception message contains a token or another sensitive value, `_add_error_details` copies the unsanitized `$exception_list` value into `$mcp_error_message`, causing sensitive text to be transmitted on the primary event even when exception autocapture is disabled.

**How this was verified:** The MCP sanitizer processes response, parameters, and user intent but not the error payload read by this mapping.

**Knowledge Base Used:** [MCP Instrumentation (posthog/mcp)](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-python/-/docs/mcp-instrumentation.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid, and broader than reported — fixed in c988e85. sanitize_event covered response/parameters/user_intent but never error, so the $exception sibling has been shipping unredacted exception messages since long before this property existed (@posthog/mcp sanitizes it via sanitizeExceptionValues; Python never ported that). Now every $exception_list frame's value goes through the same sanitizer, which covers both surfaces since the scalar is derived after sanitization runs — regression test verified to fail without the fix.

Review finding on this PR: $mcp_error_message copied the exception value
straight out of $exception_list, so a credential in the failure text ("auth
failed for token phc_...") shipped verbatim.

Valid, and broader than reported — sanitize_event covered response,
parameters and user_intent but never error, so the $exception sibling has
been sending unredacted exception messages since long before this property
existed. @posthog/mcp sanitizes it (sanitizeExceptionValues); Python simply
never ported that, and surfacing the message a second time made the gap
visible.

Redacts the `value` of every $exception_list frame through the same
sanitizer as everything else, leaving type/mechanism intact. Both surfaces
are covered, since the scalar is derived after sanitization runs.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Comment thread posthog/mcp/_sanitization.py
@veria-ai

veria-ai Bot commented Aug 21, 2026

Copy link
Copy Markdown

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 1 · PR risk: 0/10

A review comment pointed out that a non-PostHog credential (an sk-... key)
in an exception message still ships. Correct, but pre-existing and not
specific to this property: sanitize_captured_value redacts PostHog tokens
and sensitive-looking keys, and applies the same way to response,
parameters and user_intent. @posthog/mcp uses the identical pattern, so
broadening it here alone would diverge the two SDKs.

Not fixing the scope — enumerating every vendor's key format is an arms
race that fails quietly in both directions. Fixing the comment, which
overclaimed by citing "sk-..." as an example of what gets redacted, and
naming before_send as the gate for hosts with strict requirements.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
return {
**error,
"$exception_list": [
{**exception, "value": sanitize_captured_value(exception.get("value"))}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: sanitize_captured_value() only recognizes ph[a-z]_... tokens in free-text strings. Consequently, an exception such as AuthError("auth failed for sk-proj-...") uploads the OpenAI key unchanged through both $mcp_error_message and $exception. This is newly exposed on the primary event when mcp_exception_autocapture=False, where no error text was previously sent. Apply bounded detection/redaction for common credential formats—ideally sharing the repository’s existing secret-detection machinery—and add coverage for a non-PostHog credential.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right and my earlier reasoning was wrong — fixed in c61f8ff. I'd argued to veria that enumerating vendor key formats is an arms race, but that doesn't apply here: posthog-python already ships exception_utils._looks_like_secret (entropy + known formats like AWS key ids + PEM markers) for the code-variables path, so reusing it needs no vendor table. Applied per whitespace-separated word rather than to the whole string, so auth failed for sk-proj-... becomes auth failed for [redacted] and keeps the diagnostic text that $mcp_error_message exists for, while ordinary messages are untouched. It covers every captured string, not just error values, so parameters and responses benefit too — and your point about mcp_exception_autocapture=False is the one that changed my mind, since that configuration genuinely sent no error text before this PR. Tests added for both a non-PostHog credential and an ordinary message. Note this now goes further than @posthog/mcp, which still only strips PostHog tokens; I'll raise porting it there.

@marandaneto marandaneto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a comment but approving to unblock

gesh added 4 commits August 21, 2026 13:03
Review finding: prepare_request() anchored the session on a *freshly minted*
handle and emitted identify/initialize immediately — before either adapter
knew whether the prompt-back could ride the result. When it could not (an
exception converted outside ToolManager.call_tool, a result with nothing to
carry it), the client never received that handle, so the events stranded in
a conversation-derived session nobody holds and the next call minted
another. Reproduced: two failed calls produced two orphan sessions, each
with its own duplicate $identify and $mcp_initialize — strictly worse than
not anchoring at all, and the same degradation as an agent that never
echoes.

An echo is the only proof of delivery, so only echoed handles anchor now.
The minting call stays in the transport/memory session it would have used
anyway; every call after the agent returns the handle joins the
conversation's session. Failure therefore degrades to the pre-feature
behaviour instead of fragmenting.

The cross-pod contract is unchanged where it matters: a pod that receives
an echoed handle derives the same session as any other pod without shared
state — it just starts from the echo rather than the mint.

Also drops the `before_send` claim from the get_request_headers docs,
posthog/mcp/__init__.py and the changeset: before_send is invoked as
before_send(capture) with one argument, so a callback written to the
documented (capture, extra) contract would raise TypeError and have every
event silently dropped. It receives the finished payload, not the request,
so `extra` does not belong there.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
The prompt-back shipped an imperative sentence inside the tool result:

  [SERVER]: Reuse conversation_id=... on every subsequent tool call in this
  conversation. Required for the server to correlate calls and ...

Two problems, both of which @posthog/mcp already moved away from (it emits
JSON.stringify({conversation_id}) with the same reasoning):

- Tool results are untrusted content, so a server telling the model what to
  do on every later call is exactly the shape a client's prompt-injection
  filter looks for. A stripped block means the handle never arrives and
  conversation sessions quietly stop working — the failure is invisible.
- It renders in the user's transcript. Spotted while manually testing the
  stack through Claude Desktop.

Now emits {"conversation_id": "..."} — same channel and cadence, just data.
The earlier parity sweep compared the delivery channels (text block vs
structuredContent) but not the payload inside the block, so this slipped
through.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
gesh added 5 commits August 21, 2026 13:52
The previous fix was too blunt. It never anchored a freshly minted handle,
which removed the orphan sessions but split every conversation in two: the
call that minted the handle landed in the connection session and only the
echoes joined the conversation's. Observed live — three Claude Desktop
calls sharing one conversation_id produced two sessions.

The check was simply in the wrong place. Delivery is known a few lines
after the tool returns; the session was being resolved before the tool ran.
Resolving it after instead gives both properties at once:

  minted + delivered    -> anchors, so the whole conversation is one session
  minted + undelivered  -> anchors nothing, so no unreachable sessions
  echoed                -> anchors, unchanged

This is also a closer reading of the review comment that started it:
"delay anchoring newly minted handles until delivery is confirmed" — the
handle is now anchored exactly when confirmed, not abandoned.

prepare_request moves behind a local resolver in all four tool-call
adapters, called on each exit path with the handle that actually reached
the agent. The orphan-guard test and the mint-then-echo test now both pass,
which is the point: they were previously mutually exclusive.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Review finding, and a regression from the previous commit: moving
prepare_request() to after the call meant data.session_id still held the
*previous* request's value while the tool executed. McpAnalytics.capture()
reads that field, so a custom event emitted from inside a tool body was
attributed to the previous caller's session — and through the identity
cache, their person.

prime_session() now settles the transport/memory session before dispatch;
the conversation anchor is still finalised afterwards, once delivery is
known. An in-tool event predates that decision, so it belongs to the
transport session, which is the correct answer available at that moment.

Regression test uses two callers with distinct Mcp-Session-Id headers and
asserts B's tool body sees B's session — verified to fail without the fix
(the first version of this test passed either way and was worthless).

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Blocking review finding. The sanitizer only knew phc_/phx_ tokens, so an
exception like AuthError("auth failed for sk-proj-...") shipped someone
else's key — and this PR newly exposes that on the primary event when
mcp_exception_autocapture=False, a configuration where no error text left
the process at all before.

I argued against this earlier on the grounds that enumerating vendor key
formats is an arms race. That was wrong for this repo: posthog-python
already ships a general detector for exactly this — exception_utils
._looks_like_secret (entropy, known formats such as AWS key ids, PEM
markers), used by the code-variables path. Reusing it needs no vendor
table.

Applied per whitespace-separated word rather than to the whole string:
redacting an entire exception message would destroy the diagnostic value
$mcp_error_message exists to provide. "auth failed for sk-proj-..." becomes
"auth failed for [redacted]", and ordinary text is untouched because no
single word in it looks like a credential.

Covers every captured string, so parameters and responses get it too, not
just error values. Note this is deliberately broader than @posthog/mcp,
which still only strips PostHog tokens — worth porting there.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
Base automatically changed from posthog-code/mcp-sdk-v2 to main August 21, 2026 12:03
gesh and others added 4 commits August 21, 2026 15:05
#881 was squash-merged and released, so sampo turned
mcp-sdk-v2-support.md into the 7.40.0 changelog entry and deleted it from
main. This branch predates that merge and still carried the file; because
the squash gave main no shared history with the branch, git saw "main never
had this file" rather than a modify/delete and silently kept it — which
would have re-emitted the entire v7.40.0 entry in the next release.

Generated-By: PostHog Code
Task-Id: ebafcb71-b03b-443d-b40c-d527ed4a04f4
@gesh
gesh merged commit 2863909 into main Aug 21, 2026
42 checks passed
@gesh
gesh deleted the posthog-code/mcp-error-properties branch August 21, 2026 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants