feat(mcp): implement @posthog/mcp SDK on top of @posthog/core (2/2)#3653
Open
lucasheriques wants to merge 22 commits into
Open
feat(mcp): implement @posthog/mcp SDK on top of @posthog/core (2/2)#3653lucasheriques wants to merge 22 commits into
lucasheriques wants to merge 22 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
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
…3665) * fix: don't autocapture PostHog's own network errors in react-native * fix: reuse isPostHogFetchNetworkError * fix: test
Member
|
i found the docs you wrote were the best way for me to wrap my head around the SDK... wanna update those now we've made changes (or point me at them :)))) |
#3669) * chore: drop IE11 from browserslist, keep ES5 bundle as polyfill canary Remove IE 11 from packages/browser/package.json#browserslist. The ES5 bundle (array.full.es5.js) is still built with IE11-compatible Babel targets (hard-coded in rollup.config.mjs) and validated by es-check in CI, so it continues to act as a canary that surfaces when we need a new polyfill. The previous attempt to drop IE11 broke CI because two tests (web-vitals.test.ts and posthog-core-also.test.ts) reference let mockX: jest.Mock from inside jest.mock(...) factories. Jest hoists the factory above the declaration, which is a TDZ bug. With IE 11 in browserslist, babel-jest's @babel/preset-env was transpiling let -> var, masking the bug. Switching the two declarations to var is the canonical jest workaround for out-of-scope mock references. Generated-By: PostHog Code Task-Id: 4411132c-c680-4a22-be01-98a36d63cf96 * fix: pin babel.config.cjs targets so testcafe IE11 keeps working packages/browser/babel.config.cjs used @babel/preset-env without explicit targets, which falls back to package.json#browserslist. That config is what jest and testcafe both use to transpile sources and test code. With IE 11 removed from browserslist, testcafe's ClientFunction bodies stopped being transpiled to ES5 and the browserstack IE11 test hung at posthog.init (the test runner injects its ClientFunction wrapper into the page; IE11 chokes on arrow functions and default params). Pinning the babel targets here decouples this transpile pipeline from package.json#browserslist. Same reason the rollup ES5 build hard-codes its own targets in rollup.config.mjs. This also restores the let -> var test fix from the previous commit: with babel now consistently transpiling let to var for jest, the TDZ bug in those jest.mock factories is masked again. Reverting keeps the test source untouched relative to main. Generated-By: PostHog Code Task-Id: 4411132c-c680-4a22-be01-98a36d63cf96 * fix: feed IE11 to testcafe via BROWSERSLIST env, not babel.config testcafe ignores the project's babel config (BASE_BABEL_OPTIONS sets babelrc: false, configFile: false). It transpiles its injected ClientFunction wrappers with @babel/preset-env and no targets, which falls back to the browserslist module — which reads package.json#browserslist. So the real lever for fixing the IE11 hang is browserslist, not babel.config.cjs. Set BROWSERSLIST inline on the testcafe workflow step instead, which: - keeps package.json#browserslist clean (IE11 truly removed) - keeps babel.config.cjs untouched (jest runs in node, no need to transpile to es5) - limits the IE11 target to the one place it actually matters: the testcafe ClientFunction transpile pipeline Revert babel.config.cjs and restore the let -> var fix in the two jest.mock factories — the TDZ bug there is real (jest hoists the factory above the let declaration), independent of any babel config. Generated-By: PostHog Code Task-Id: 4411132c-c680-4a22-be01-98a36d63cf96 * fix: silence no-var on the TDZ workaround in two jest.mock factories The var declarations are necessary so the assignments inside the hoisted jest.mock factories don't hit TDZ. Add per-line eslint-disable instead of leaving CI red on no-var. Generated-By: PostHog Code Task-Id: 4411132c-c680-4a22-be01-98a36d63cf96 * chore: explain why the two jest.mock var declarations need eslint-disable Generated-By: PostHog Code Task-Id: 4411132c-c680-4a22-be01-98a36d63cf96
…3667) * fix(replay): stop polling preload-as-style <link> elements forever Session recorder treated <link rel="preload" as="style" href="*.css"> as if it were a stylesheet and waited for link.sheet to populate. Per spec preload links never instantiate a CSSStyleSheet, so the wait timed out, recursively re-serialized the link, scheduled another wait, and leaked a 'load' listener on every cycle - multiplying further on every real load event. Pages with Next.js-style CSS preloads accumulated thousands of active polling chains, saturating the main thread and freezing the tab on refocus. - Drop preload from the predicate so only rel=stylesheet schedules a wait. - Replace the recursive serializeNodeWithId call with the serializedNode already in scope. - Track tracked links in a WeakSet so repeat calls are no-ops; gate the load handler on the same 'fired' guard as the timer; pass { once: true } so the listener self-removes if a load event ever fires. Added jsdom unit tests in rrweb-snapshot and a real-browser Playwright spec that loads a page with five preload-as-style links, instruments HTMLLinkElement.prototype.addEventListener, and asserts the count stays bounded across timer cycles plus dispatched load events. Without the fix the spec sees ~30 leaked listeners; with the fix it sees zero. Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620 * fix(replay): re-serialize stylesheet link on load so _cssText reaches replay qa-swarm found a convergent HIGH-severity regression: dropping the recursive serializeNodeWithId call inside the load callback meant late-loading <link rel="stylesheet"> elements would deliver their original pre-load serializedNode to onStylesheetLoad — which has no _cssText, so StylesheetManager.attachLinkElement (gated on '_cssText' in attributes) never emitted the cssText mutation. Every replay with a stylesheet not loaded by first snapshot would render unstyled. The new WeakSet idempotency guard makes restoring the recursive call safe: re-entry into onceStylesheetLoaded sees the link in the WeakSet and returns early, so the chain cannot re-arm. Also from qa-swarm: - Parameterize the three new unit tests and add a fourth case that populates link.sheet between first serialize and load, asserting _cssText reaches onStylesheetLoad (regression coverage for the bug this commit fixes). - Tighten the Playwright assertion from toBeLessThanOrEqual(5) to toBe(0) — preload-as-style links bypass onceStylesheetLoaded entirely now. - HTML comment in the leak-repro playground explaining the CSS chunks are intentionally non-existent. Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620 * chore(changeset): add @posthog/rrweb-snapshot to bumped packages The fix lives in packages/rrweb/rrweb-snapshot/src/snapshot.ts so the rrweb-snapshot package needs the same patch bump as posthog-js. Flagged by the changeset-hygiene action. Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620 * fix(replay): reset stylesheet-load tracking when StylesheetManager resets The module-level stylesheetLoadTracked WeakSet persisted for the document lifetime, so a recorder stop/restart on the same page (SPA toggling session recording) silently skipped re-tracking any <link rel="stylesheet"> whose load was still pending from the previous lifecycle. The pending { once: true } listener was closed over the stopped recorder's onStylesheetLoad, so the second recorder never received the _cssText mutation when the sheet finally loaded. Pre-fix this didn't bite because there was no skip — each call attached a fresh listener (which was the leak being fixed). The WeakSet now resets inside StylesheetManager.reset() so the second recorder sees a clean tracker. Added a jsdom test that exercises the cross-lifecycle path: serialize a pending link, call resetStylesheetLoadTracking, serialize it again, populate sheet, dispatch load — assert the second session receives a _cssText payload. Note: the WeakSet still lives in rrweb-snapshot rather than in StylesheetManager. Moving it into the recorder would couple the snapshot package to load-tracking state it does not own; threading the WeakSet through serializeNodeWithId options is the cleaner long-term refactor. For now the reset hook is the minimal fix. Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620 * test(snapshot): pin firstSessionCalls to its actual value of 1 qa-swarm v2 flagged toBeLessThanOrEqual(1) as hand-wavy. The actual value is 1, not 0 — even after resetStylesheetLoadTracking(), the first session's { once: true } load listener is still attached to the link element (reset only clears the dedup tracker, not the listeners themselves). When the sheet finally loads, both session 1's and session 2's listeners fire. Pinning to toBe(1) documents this trade-off explicitly. The cost is one duplicate attachLinkElement mutation per checkout-pending link (same id, same _cssText — applied idempotently by the replayer). Tracked separately as a follow-up to abort previous-snapshot listeners on reset. Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620 * chore(lint): prettier-format playwright preload-link-leak spec CI lint failed: my multi-line function signature broke prettier's printWidth. Auto-fixed with `prettier --write`. Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620 * chore(changeset): add @posthog/rrweb to bumped packages ff5f56d modified packages/rrweb/rrweb/src/record/stylesheet-manager.ts (the @posthog/rrweb package) when wiring resetStylesheetLoadTracking() into the StylesheetManager.reset() hook. The changeset-hygiene action flagged this as missing. Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620 * fix(replay): tear down pending stylesheet listeners on reset resetStylesheetLoadTracking() previously only cleared the WeakSet of "already-tracked" links. The actual listener + setTimeout it had scheduled on each link stayed attached. So when StylesheetManager.reset() ran from takeFullSnapshot (start AND every checkout), the next full snapshot would register a SECOND listener on still-pending links and both listeners would fire when the sheet eventually loaded — emitting a duplicate attachLinkElement mutation per pending link per checkout. Switch the tracker to Map<HTMLLinkElement, { timer, onLoad }>. Reset now iterates entries, clearTimeout each timer, removeEventListener each named onLoad handler, then clears the Map. Each watch removes its own entry from the Map after fire() runs (try/finally so a thrown listener still cleans up), so successful loads behave exactly as before. Stays within the existing browser support matrix — no AbortController, no AbortSignal, just setTimeout/addEventListener with the named handler we now retain a reference to. Strong refs in the Map are bounded by pending stylesheets and cleared on every full-snapshot reset. Updates the cross-lifecycle test (firstSessionCalls is now 0, not 1, because the previous session's listener is removed in reset). Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620 * refactor(replay): use AbortController for stylesheet-load watch teardown Same semantics as 3ed123d, modern shape. Each watch owns an AbortController; the load listener is bound with { signal }, the timer is bridged to the signal via a one-shot abort handler. resetStylesheetLoadTracking just calls ac.abort() on every entry instead of manually iterating timer + listener handles. IE11 is in the compat matrix for historical reasons but session replay already does not work there, so the AbortController dependency is fine. Functionally identical: same Map<HTMLLinkElement, …> shape for the idempotency check + reset iteration, same try/finally on listener, same re-entry safety (delete happens AFTER listener() so the recursive serializeNodeWithId call sees has(link) === true and returns early). Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620 * test(snapshot): rename cross-lifecycle test to lead with the behaviour qa-swarm convergent NIT (paul + xp): the old name described the mechanism ("re-tracks a pending stylesheet link after resetStylesheetLoadTracking") not the behaviour the test guards. The new name leads with what the test asserts — that the previous session's listener is torn down — so a future reader sees the regression intent without diffing the body. Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620 * test(replay): add cross-lifecycle Playwright spec for stylesheet load tracking End-to-end coverage of the listener-teardown-on-reset fix in a real browser. The spec: - Loads a page with no stylesheet in HTML. - Holds the CSS response via a Playwright route. - Injects a <link rel="stylesheet"> after recording starts (mutation observer picks it up). - Configures session_recording.full_snapshot_interval_millis to 1500ms so rrweb's takeFullSnapshot fires repeatedly, each one calling StylesheetManager.reset() -> resetStylesheetLoadTracking(). Each checkout reschedules a fresh load watch on the still-pending link. - Releases the CSS only after several checkouts have happened. - Asserts the captured event stream contains exactly one _cssText attribute mutation for the link's mirror id. Verified the spec catches the regression: with resetStylesheetLoadTracking neutered to skip the controller.abort() loop, the spec sees 3 _cssText mutations (one per checkout listener that wasn't torn down). With the abort restored, exactly 1. Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620 * test(replay): constrain cross-lifecycle stylesheet spec to chromium CI showed the spec failing on Firefox and WebKit with Received: 0 — those browsers do not populate link.sheet.cssRules from a Playwright-route- fulfilled CSS response within our wait window. The end-to-end load delivery doesn't complete in time, so the mutation is never emitted. The fix being verified is JS-internal (Map+AbortController teardown on reset) and browser-agnostic. The jsdom unit tests cover the logic deterministically. The Playwright spec adds Chromium end-to-end confirmation; non-Chromium coverage would need a different fixture strategy (e.g., serving the CSS via a real test server instead of route interception) and isn't worth the complexity here. Generated-By: PostHog Code Task-Id: 18dbe2b5-9a25-4b4c-a756-db029804f620
marandaneto
reviewed
May 27, 2026
|
|
||
| const client = resolveClient(options) | ||
| if (!client) { | ||
| log('Warning: No PostHog project token configured. Events will not be sent anywhere.') |
Member
There was a problem hiding this comment.
should return server already then
Contributor
Author
There was a problem hiding this comment.
it does — line 57 (before the no-client log) returns early with return validatedServer as TServer when the server is already instrumented. the log at line 62 is for the separate case of no project token configured. ill let users configure projectToken via clientOptions later if they want, so we still wrap the server. but ack the readability — happy to short-circuit the no-token case too if you think thats cleaner.
Member
|
a few thoughts:
maybe @pauldambra has opinions here as well |
marandaneto
reviewed
May 27, 2026
| * Push an MCP event through the pipeline (redact → sanitize → truncate → fan out → enqueue). | ||
| * Errors at any stage are logged and the event is dropped, never re-thrown into tool code. | ||
| */ | ||
| async ingest(event: UnredactedEvent, enableAITracing: boolean): Promise<void> { |
Member
There was a problem hiding this comment.
all SDKs call this just capture or similar, i think this SDK is calling in many places with other synonyms like publish, ingest, etc, I'd align cus its already a mix across repositories and this one has all the 3 mixed up (see eg node sdk)
Contributor
Author
There was a problem hiding this comment.
done in bc745bc. renamed client.ingest() -> client.capture() and the standalone publishEvent -> captureEvent (file publish.ts -> capture.ts). the public publishCustomEvent keeps its name since it semantically means "custom event, not auto-captured", but happy to revisit if you want everything called capture*.
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.
marandaneto
reviewed
May 27, 2026
| * here with `eventType: custom` and bypass that gate — disabling auto-capture should not | ||
| * silently swallow events the host application explicitly chose to emit. | ||
| */ | ||
| export function publishEvent(server: MCPServerLike, eventInput: UnredactedEvent): void { |
Contributor
Author
There was a problem hiding this comment.
done, renamed publishEvent -> captureEvent (and the file). bc745bc.
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.
marandaneto
reviewed
May 27, 2026
| [key: string]: unknown | ||
| } | ||
|
|
||
| export interface MCPAnalyticsOptions { |
Member
There was a problem hiding this comment.
missing enableExceptionAutocapture?
Contributor
Author
There was a problem hiding this comment.
good catch, added in bc745bc. enableExceptionAutocapture?: boolean on MCPAnalyticsOptions, defaults to true. unit-tested both default-on and explicit-off paths.
* chore: ignore .worktrees directory * chore: exclude examples from Dependabot and add reviewer * chore: address review comments * chore: drop block 2 and use team-client-libraries as reviewer
…p to 1.6.0 (#3673) * chore(react-native): bump posthog-react-native-session-replay peer dep to 1.6.0 Bump the optional peer dependency `posthog-react-native-session-replay` from `>= 1.5.8` to `>= 1.6.0`. The new minor adds an opt-in path that resolves `posthog-ios` through Swift Package Manager via `"posthog.useSpm": "true"` in the consumer's `ios/Podfile.properties.json` + `use_frameworks! :linkage => :dynamic`. Default consumer behavior is unchanged. `packages/react-native` itself has no podspec or native iOS code; `posthog-ios` only reaches RN apps via the session-replay peer dependency. Consumers who don't use session-replay are unaffected. * chore: bump changeset to minor
* fix: speed up logs serialization * test: cover budgeted log serialization * test: strengthen logs serialization coverage * fix: handle logs serializer edge cases * fix: skip logs properties with non-serializable toJSON * fix: serialize boxed primitives in logs * perf: reduce logs serializer allocations * fix: type boxed strings in logs serializer * address pr review feedback
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
…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
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
6 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