feat(mcp): implement @posthog/mcp SDK on top of @posthog/core (2/2)#3653
Open
lucasheriques wants to merge 14 commits into
Open
feat(mcp): implement @posthog/mcp SDK on top of @posthog/core (2/2)#3653lucasheriques wants to merge 14 commits into
lucasheriques wants to merge 14 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
Member
|
a few thoughts:
maybe @pauldambra has opinions here as well |
marandaneto
reviewed
May 27, 2026
marandaneto
reviewed
May 27, 2026
| * @param contextStack - Optional Error object to use for stack context (for validation errors) | ||
| * @returns ErrorData object with structured error information | ||
| */ | ||
| export function captureException(error: unknown, contextStack?: Error): ErrorData { |
Member
There was a problem hiding this comment.
node and rn have something called buildEventMessage or buildFromUnknown
this whole parsing now is being duplicated the 3rd time (not sure if they match 100%), but i do see a reason here to extract something of it to the core and reuse here, probably from node i guess
Member
There was a problem hiding this comment.
at least the stack trace part should be the same
Member
There was a problem hiding this comment.
are MCP servers minified? if so, we'll need to attach debug ids in order to symbolicate those errors
Contributor
Author
There was a problem hiding this comment.
agreed, this is duplicated work. filing as a follow-up — i did not want this pr to also touch @posthog/core since it would need a coordinated migration across node, react-native and us. happy to pick it up next.
on debug ids: yes, mcp servers built with tsdown/rollup/bun do get minified. the symbolication story for an mcp server is the same as for posthog-node (uploading sourcemaps via the cli), so once core gets the shared exception parsing, this falls out of the migration too.
Member
There was a problem hiding this comment.
error tracking team solved it already, not sure if merged, then you can just pull changes and reuse that bit
marandaneto
reviewed
May 27, 2026
marandaneto
reviewed
May 27, 2026
| * persist anything across restarts — every server gets a fresh client — so a | ||
| * plain object is enough. Mirrors `PostHogMemoryStorage` from `posthog-node`. | ||
| */ | ||
| export class PostHogMemoryStorage { |
Member
There was a problem hiding this comment.
node also has that, move to core and reuse
Contributor
Author
There was a problem hiding this comment.
ack, follow-up. would prefer to do this in a separate pr that touches @posthog/core so the migration is reviewable on its own. for now the duplication is intentional and small — PostHogMemoryStorage is ~20 lines.
Member
There was a problem hiding this comment.
its ok to touch multiple packages and release them together as long as the refactor makes sense to this PR which i think it does
not a blocker, but i'd rather have done this before duplicating the code.
Member
There was a problem hiding this comment.
ps: reason is, most of the time, we forget and move on, and the duplicated code lives on, my own exp (meaning been there done that).
marandaneto
reviewed
May 27, 2026
…ocapture Per @marandaneto review on PR #3653: - Rename internal `publishEvent` -> `captureEvent` and file `publish.ts` -> `capture.ts`. Rename `PostHogMCP#ingest` -> `PostHogMCP#capture`. Aligns with posthog-node's `capture` verb instead of mixing publish/ingest/capture across the SDK. - Add `enableExceptionAutocapture?: boolean` to MCPAnalyticsOptions (default true). Set to false to skip the parallel `$exception` event when a tool errors. Plumbed through `buildPostHogCaptureEvents` and the new `PostHogMCPCaptureOptions` passed to `client.capture()`. Unit tests cover both the default-on and explicit-off paths. Public surface unchanged: `publishCustomEvent` is still the user-facing name for arbitrary `$mcp_custom` events. Generated-By: PostHog Code Task-Id: baa7e0cd-4946-4524-a05f-42c547a55f44
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: +256 kB (+1.56%) Total Size: 16.7 MB
ℹ️ View Unchanged
|
…ementation # Conflicts: # packages/mcp/README.md # packages/mcp/src/index.ts
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