feat(mcp): implement @posthog/mcp SDK on top of @posthog/core (2/2)#3653
Open
lucasheriques wants to merge 16 commits into
Open
feat(mcp): implement @posthog/mcp SDK on top of @posthog/core (2/2)#3653lucasheriques wants to merge 16 commits into
lucasheriques wants to merge 16 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4 tasks
lucasheriques
changed the title
Graphite Automations"sdk release label" took an action on this PR • (05/21/26)1 label was added to this PR based on Adam Bowker's automation. "Add graphite merge queue [copy]" took an action on this PR • (05/21/26)2 labels were added to this PR based on Lucas Faria's automation. |
Contributor
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
packages/mcp/src/extensions/publish.ts:20-22
**`publishCustomEvent` silently no-ops when `enableTracing: false`**
The `enableTracing` option is documented as a "master switch for auto-captured events," implying `publishCustomEvent` (a deliberate, user-initiated call) should still fire. But because `publishEvent` gates on `!data.options.enableTracing` before the client check, any call to `publishCustomEvent` is silently swallowed when the user has opted out of automatic tracing. The test suite has no coverage for this case. A user who disables auto-capture but explicitly calls `publishCustomEvent` will get no event, no error, and no warning.
### Issue 2 of 4
packages/mcp/src/extensions/tracing.ts:38-40
**Three helpers duplicated across `tracing.ts` and `tracing-v2.ts`**
`isToolResultError`, `applyResolvedMetadata`, and `getContextArgument` are all defined identically in both `tracing.ts` and `tracing-v2.ts`. This violates OnceAndOnlyOnce and means any future fix or behaviour change must be applied in two places. These should be extracted to a shared utility module and imported by both files.
### Issue 3 of 4
packages/mcp/src/extensions/tracing.ts:173-264
**Initialize-handler logic duplicated inside `setupToolCallTracing`**
`setupInitializeTracing` (lines 173–212) was extracted so that `tracing-v2.ts` can call it directly, but the identical inline copy inside `setupToolCallTracing` (lines 222–254) was never removed. Any behaviour change to the initialize handler must now be applied in both places. The inline block inside `setupToolCallTracing` should be replaced with a call to `setupInitializeTracing`.
### Issue 4 of 4
packages/mcp/src/index.ts:51
This looks like a debug log left over from development. It will appear in production logs of any STDIO-based MCP server where the user has provided a logger.
```suggestion
log('track() - Server already being tracked, skipping initialization')
```
Reviews (1): Last reviewed commit: "feat(mcp): implement @posthog/mcp SDK on..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Contributor
|
Size Change: +273 kB (+1.66%) Total Size: 16.7 MB
ℹ️ View Unchanged
|
lucasheriques
added a commit
that referenced
this pull request
May 21, 2026
Four review items from the AI reviewer on the implementation PR: 1. (P1) `publishCustomEvent` was silently swallowed when the host opted out of auto-capture via `enableTracing: false`. That option is the master switch for *auto*-captured events (tool calls, listings, identify) — a user-initiated `publishCustomEvent` call is explicit and shouldn't be gated by it. Fixed in `publish.ts` by exempting `MCPAnalyticsEventType.custom` from the gate. Added a regression test in `publishCustomEvent.test.ts`. 2. (P2) Three helpers (`isToolResultError`, `applyResolvedMetadata`, `getContextArgument`) were defined identically in both `tracing.ts` and `tracing-v2.ts`. Extracted to a new `tracing-helpers.ts` along with `getEventDuration`; both files now import from it. 3. (P2) `setupToolCallTracing` in `tracing.ts` contained an inline copy of the initialize-handler wrapping that the also-exported `setupInitializeTracing` function already implements. Changed `setupInitializeTracing` to take `MCPServerLike` directly (it only ever used `highLevelServer.server` anyway) and replaced the inline block with a call. Updated the caller in `tracing-v2.ts` to pass `server.server` instead of `server`. 4. (P2) Removed the `[SESSION DEBUG]` prefix from the "Server already being tracked" log in `index.ts` — it was leftover development noise, not a useful prefix for production logs. Verification: - `pnpm --filter=@posthog/mcp test:unit` — 346 passing (was 345; +1 for the new `enableTracing: false` regression test). - `pnpm --filter=@posthog/mcp lint` — clean. - `pnpm --filter=@posthog/mcp build` — clean. Generated-By: PostHog Code Task-Id: baa7e0cd-4946-4524-a05f-42c547a55f44
lucasheriques
force-pushed
the
posthog-code/mcp-implementation
branch
from
54edfbe to
407003e
Compare
lucasheriques
added a commit
that referenced
this pull request
May 21, 2026
Four review items from the AI reviewer on the implementation PR: 1. (P1) `publishCustomEvent` was silently swallowed when the host opted out of auto-capture via `enableTracing: false`. That option is the master switch for *auto*-captured events (tool calls, listings, identify) — a user-initiated `publishCustomEvent` call is explicit and shouldn't be gated by it. Fixed in `publish.ts` by exempting `MCPAnalyticsEventType.custom` from the gate. Added a regression test in `publishCustomEvent.test.ts`. 2. (P2) Three helpers (`isToolResultError`, `applyResolvedMetadata`, `getContextArgument`) were defined identically in both `tracing.ts` and `tracing-v2.ts`. Extracted to a new `tracing-helpers.ts` along with `getEventDuration`; both files now import from it. 3. (P2) `setupToolCallTracing` in `tracing.ts` contained an inline copy of the initialize-handler wrapping that the also-exported `setupInitializeTracing` function already implements. Changed `setupInitializeTracing` to take `MCPServerLike` directly (it only ever used `highLevelServer.server` anyway) and replaced the inline block with a call. Updated the caller in `tracing-v2.ts` to pass `server.server` instead of `server`. 4. (P2) Removed the `[SESSION DEBUG]` prefix from the "Server already being tracked" log in `index.ts` — it was leftover development noise, not a useful prefix for production logs. Verification: - `pnpm --filter=@posthog/mcp test:unit` — 346 passing (was 345; +1 for the new `enableTracing: false` regression test). - `pnpm --filter=@posthog/mcp lint` — clean. - `pnpm --filter=@posthog/mcp build` — clean. Generated-By: PostHog Code Task-Id: baa7e0cd-4946-4524-a05f-42c547a55f44
lucasheriques
force-pushed
the
posthog-code/mcp-implementation
branch
from
407003e to
fe591a3
Compare
lucasheriques
changed the base branch from
posthog-code/mcp-scaffold
to
graphite-base/3653
…script Per Greptile / graphite-app / mendral-app review on PR #3652: - Bump @modelcontextprotocol/sdk devDep ~1.24.2 -> ~1.29.0 to pick up the GHSA-345p-7cg4-v4c7 (data leak, 1.26.0) and GHSA-8r9q-7v3j-jr4g (ReDoS, 1.25.2) fixes. - Raise peerDependencies floor >=1.11 -> >=1.26.0 so consumers cannot resolve to a known-vulnerable host SDK. - Extract the src/version.ts generator into a reusable `generate-version` script and hook it from both `pretest:unit` and `prebuild`, so `pnpm --filter=@posthog/mcp test:unit` works standalone (Greptile flagged that test:unit failed against a fresh checkout because version.ts had not been generated yet). - Reorder prepublishOnly to build before test:unit for the same reason. Generated-By: PostHog Code Task-Id: baa7e0cd-4946-4524-a05f-42c547a55f44
lucasheriques
changed the base branch from
graphite-base/3653
to
posthog-code/mcp-scaffold
Contributor
|
Size Change: +231 kB (+1.4%) Total Size: 16.7 MB
ℹ️ View Unchanged
|
…ementation # Conflicts: # packages/mcp/README.md # packages/mcp/src/index.ts
Per @marandaneto review on PR #3653: the in-memory persisted-property storage was duplicated in posthog-node and @posthog/mcp. - Add PostHogMemoryStorage to @posthog/core and export it. - posthog-node and @posthog/mcp now import it from core; their local copies are deleted. No behavior change. Also restores .changeset/silver-safari-fetch.md, which was dropped during an earlier merge resolution. Generated-By: PostHog Code Task-Id: baa7e0cd-4946-4524-a05f-42c547a55f44
Per @marandaneto review on PR #3653: the bespoke ~850-line V8 stack parser / path normalizer in exceptions.ts duplicated logic the error tracking team already shipped in @posthog/core. - captureException() is now a thin wrapper over core's ErrorPropertiesBuilder (coercers + node stack parser), keeping only the MCP-specific CallToolResult message extraction. - $exception events now emit the standard $exception_list / $exception_level contract instead of the flat $exception_message/type/stacktrace fields, so MCP errors group and symbolicate like every other PostHog SDK. - event.error is now core's ErrorProperties; truncation digs into the nested $exception_list to cap message length + stack frames. - Manual `event.error = { message }` sites route through captureException so every error path produces the same shape. - Tests updated to the new shape; dropped the path-normalization / context-line tests since that behavior now lives in core (and the node-only frame modifiers are intentionally not included). Note: source-context lines and relative-path normalization come from posthog-node's frame modifiers, which live in that package and need async fs; not wired here. Debug-id symbolication for minified MCP servers works via the standard source-map upload flow. Generated-By: PostHog Code Task-Id: baa7e0cd-4946-4524-a05f-42c547a55f44
Contributor
Author
|
@marandaneto tracking the still-open threads here since the convo history got long — all 4 have a reply from me and are just waiting on your ack:
if you're happy with each, feel free to resolve them. anything still off, lmk and i'll jump back on it. |
marandaneto
reviewed
Jun 2, 2026
| "posthog-js": patch | ||
| --- | ||
|
|
||
| fix(browser): avoid exposing internally-created Request bodies to downstream fetch wrappers in Safari. |
Member
There was a problem hiding this comment.
doesn ot belong here i believe? i dont see any browser changes
marandaneto
reviewed
Jun 2, 2026
| * Set to `false` if you handle error tracking elsewhere and don't want MCP errors | ||
| * fanning out into PostHog error tracking. | ||
| */ | ||
| enableExceptionAutocapture?: boolean |
Member
There was a problem hiding this comment.
I think this should be:
exceptionAutocapture?: boolean | ExceptionAutocaptureConfig like we do with the other SDKs so we can add more config within the ExceptionAutocaptureConfig later on
check types:
/** * Determines whether to capture exceptions. * * @see {ExceptionAutoCaptureConfig} * @default undefined */ capture_exceptions?: boolean | ExceptionAutoCaptureConfig
Member
|
@lucasheriques some questions/issues here #3653 (comment) yet not replied/addressed so i am not sure if its still a TODO, wont do, etc |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second (and main) half of the
@posthog/mcpport. Stacks on top of #3652 (scaffold). Brings in the real public API, all SDK modules undersrc/extensions/, the consolidated test suite, and the architecture doc.The diff is large because porting the SDK is large, but the actual review surface is concentrated in ~10 files (listed below). The rest is a verbatim port of behavior we already shipped in the standalone repo, plus tests.
@posthog/mcpis already published on npm from a different repo (PostHog/mcp-analytics, the previous standalone). The release workflow in this monorepo publishes via OIDC trusted publishing (noNPM_TOKEN), which means npm pins the publisher to a specific GitHub Actions workflow path on a specific repo.The npm trusted publisher for
@posthog/mcpneeds to be updated (or a second one added) before the release approval Slack ping fires for this PR. On npmjs.com:@posthog/mcp→ Settings → Trusted PublisherPostHog/posthog-js, workflow file.github/workflows/release.yml, environmentNPM ReleaseOnce that's done, merging this PR triggers the release workflow, the maintainer approves the Slack notification, and
@posthog/mcp 0.1.0publishes from the monorepo. If you approve without updating the trusted publisher, the publish step will fail with a provenance/publisher mismatch and we'll need to retry after the npm settings change.Recommended order of operations:
@posthog/mcp.📊 Composition
src/extensions/*.ts,src/index.ts,src/types.ts,src/storage-memory.ts)src/__tests__/)package.json,tsconfig*,rslib,jest,babel,.prettierrc,.gitignore)Test-to-code ratio: 1.33x. Most of the test volume is integration tests against a real in-memory MCP transport (in
test-utils/client-server-factory.ts), which is harder to compress than unit tests but catches real regressions in the tracing wrapper.📝 Where to look
packages/mcp/src/index.tspackages/mcp/src/types.tsPostHogMCP extends PostHogCoreStatelesspackages/mcp/src/extensions/client.tspackages/mcp/src/extensions/publish.tspackages/mcp/src/extensions/logger.tspackages/mcp/src/storage-memory.ts$-prefixed wire contractpackages/mcp/src/extensions/constants.ts$mcp_*+$exception+$ai_span)packages/mcp/src/extensions/posthog-events.tspackages/mcp/docs/ARCHITECTURE.mdSkim only — verbatim ports from the standalone repo, behavior unchanged:
tracing.ts,tracing-v2.ts,tools.ts,compatibility.ts,mcp-sdk-compat.ts,context-parameters.ts,conversation-id.ts,intent.ts,internal.ts,session.ts,ids.ts,redaction.ts,sanitization.ts,truncation.ts,mcp-payloads.ts,exceptions.ts,event-types.ts,tracing-helpers.ts.🤖 PostHog Code review prompt
Copy-paste this into PostHog Code (or any code-aware AI) to get a focused second pass:
Stack
index.ts)Test plan
pnpm --filter=@posthog/mcp build— clean (ESM + CJS +.d.ts, ~155 kB CJS).pnpm --filter=@posthog/mcp test:unit— 346 passing, 1 skipped, 0 failing across 24 jest test files.pnpm --filter=@posthog/mcp lint— clean.pnpm lint(all 26 monorepo packages) — clean.@posthog/mcpupdated to point atPostHog/posthog-js(see "Before merging" above).PostHog/mcp-analyticsrepo to prevent accidental dual publishes.Created with PostHog Code