ci(mcp): add integration test setup validating @posthog/mcp across MCP SDK v1 and v2 - #4565
Merged
Conversation
Contributor
Replay incident risk checkThis diff touches code involved in past incidents. This is a heads-up, not a verdict: read the matched sections of INCIDENTS.md and answer their review questions before merging. For a judgment on whether this diff has the same failure mode, run the |
Contributor
|
gesh
marked this pull request as draft
August 19, 2026 13:31
Contributor
Prompt To Fix All With AI### Issue 1
packages/mcp/harness/dual-era/matrix.mjs:133-138
**Client crashes become passing rows**
When the v1 client exits before emitting valid JSON, `runClient` converts the parse failure into an empty `results` object. The reconciliation treats every missing assertion as not applicable, so the SDK v1 lane can exit successfully without completing its integration assertions.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "ci(mcp): move the integration harness in..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
gesh
force-pushed
the
posthog-code/mcp-harness-ci
branch
from
August 19, 2026 13:35
a36a206 to
a3ef207
Compare
gesh
changed the title
…P SDK v1 and v2 Implements an integration test harness for @posthog/mcp at packages/mcp/harness/ and a blocking "MCP harness" CI workflow that runs on every PR affecting the package, with four lanes: "sdk v1", "sdk v2", "nest mcp sdk v1", "nest mcp sdk v2". The harness drives real MCP servers over real HTTP (raw JSON-RPC, not the SDK client) instrumented with the workspace build of @posthog/mcp, and asserts the core capture surface — tool calls, errors, intent, sessions with conversation-id on/off — plus identity and safety columns, across both SDK majors, both protocol eras, high/low-level instrumentation, and the NestJS/@rekog/mcp-nest customer stacks. Design points: - Fixtures import @posthog/mcp via Node self-reference, so they always test the package's own dist; nothing ships (harness/ is outside "files"). - Servers bind ephemeral ports (PORT=0) and announce them on stdout, so rows cannot collide; explicit PORT= still works for single-cell debugging. - Known-broken cells are pinned in exact-match expected-failures.json snapshots: a regression fails the lane, and so does an unexpected improvement, so every moved cell is a deliberate diff. - Three probes cover orderings the matrix cannot reach: handlers registered after instrument(), a failure on the first call of a conversation, and a two-page tool catalogue whose envelope must survive the listing wrapper. - Both @rekog/mcp-nest majors coexist via pnpm aliases (rekog-mcp-nest-v1/v2); the nest-v2 fixture uses raw JSON Schema tool parameters; the unit suite keeps real-name zod 3 for SDK v1 while fixtures use a zod4 alias. - No secrets and no network (in-process recorder), so the workflow also runs on fork PRs. - devDep @modelcontextprotocol/sdk set to ^1.30.0. Verified locally and on CI: v1 matrix rows all-green, v2 matrix matching its snapshot, probes 9/9 + 3/3 + 8/8, nest-v1 16/16 on both levels, nest-v2 35/37 on both levels with exactly the two expected failures. Generated-By: PostHog Code Task-Id: 8656d768-37c4-4e79-8d98-b3830b00c330
gesh
force-pushed
the
posthog-code/mcp-harness-ci
branch
from
August 19, 2026 13:40
a3ef207 to
c529079
Compare
Contributor
Contributor
|
Size Change: -1.79 kB (-0.01%) Total Size: 19.9 MB 📦 View Changed
ℹ️ View Unchanged
|
A client that died before printing its JSON verdict resolved to an empty results object. Absent assertions render as `·` (not applicable) and are skipped by the expected-failures reconciliation, so a crashed client produced a row of dots and the lane exited 0 having asserted nothing. Anything short of a verdict for every column is now an error that fails the lane, with the crash dump in the log. A non-zero client exit is still a result, not a crash — the client exits 1 whenever a cell is red. Also drain the client's stderr (an unread pipe can wedge the child) and exclude unreported rows from the "now passing" check, so a crash isn't reported as a stale snapshot. Separately, list the shared check-affected machinery in additional-paths, matching integration.yml/library-ci.yml/testcafe.yml: a change to how "affected" is computed should re-run this gate rather than skip it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each probe file covers both SDK majors internally, and both sdk lanes ran all of them — so every probe executed twice, and a probe regression turned both lanes red without naming the stack that broke. The probes now take the same --major filter as the matrix: the "sdk v1" lane runs only the v1 halves, the "sdk v2" lane the v2 halves plus the v2-only first-call-error probe. Same 20 probe assertions in total, each run exactly once, and a red probe now attributes to its SDK major. Running a probe file with no --major still exercises both majors for local use. Generated-By: PostHog Code Task-Id: 8656d768-37c4-4e79-8d98-b3830b00c330
gesh
marked this pull request as ready for review
August 20, 2026 08:39
Contributor
|
Reviews (2): Last reviewed commit: "ci(mcp): split the dual-era probes by SD..." | Re-trigger Greptile |
Member
Author
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated reviewers (qa-team, paul-reviewer, xp-reviewer, security-audit as warranted) Verdict: ✅ APPROVE (round 3 @ bba9bd1)Round 3 reviewed the fail-fast guard commit (bba9bd1) that resolved round 2's finding: condition precise, exit code integrates with the matrix's no-verdict handling, message surfaces correctly through the crash-reason extractor. No findings; quality loop converged. Key findingsNone — no actionable findings this round. Round 2's LOW finding was fixed in bba9bd1 and its thread resolved. Convergencen/a (single reviewer ran). Reviewer summaries
Previous rounds (2)round 1 @ 11e9f17 — ✅ APPROVE: full-diff pass, 0 findings, no delegations. Automated by QA Swarm — not a human review |
The header assertion's isV2 branch duplicated what headerOk already computes: every v2 matrix row omits headerExpect, so the client receives --header none and headerOk reduces to the same no-header-minted check. Generated-By: PostHog Code Task-Id: 8656d768-37c4-4e79-8d98-b3830b00c330
gesh
commented
Aug 20, 2026
qa-swarm flagged that `header: headerOk` (from the round-1 simplify commit) is behavior-preserving only because every current v2 matrix row omits --header, defaulting HEADER_EXPECT to 'none'. Nothing enforced that a v2 server must never mint a session header (a 2026-07-28 MUST NOT) — a future v2 row passing a non-'none' --header would silently change what the assertion means. Fail fast instead when SDK=v2 and HEADER_EXPECT != 'none'. Generated-By: PostHog Code Task-Id: 8656d768-37c4-4e79-8d98-b3830b00c330
"sdk v1" vs "nest mcp sdk v1" did not tell a passer-by what differs between the lanes. The jobs are now "official SDK v1/v2" (the @modelcontextprotocol/* packages) and "mcp-nest (SDK v1/v2)" (the @rekog/mcp-nest customer stacks), matching the real package names. Renamed before branch protection is configured, so no required-check strings need migrating. Generated-By: PostHog Code Task-Id: 8656d768-37c4-4e79-8d98-b3830b00c330
Proves the MCP harness gate catches a real capture regression on CI. This commit intentionally stops capturing tool-call exceptions on the high-level path; the harness lanes must go red naming the errors cells. It is reverted in the next commit — do not merge this commit alone. Generated-By: PostHog Code Task-Id: 8656d768-37c4-4e79-8d98-b3830b00c330
…t follows)" This reverts commit 99c644a.
marandaneto
reviewed
Aug 20, 2026
| ['v1 stateless low', 'v1.mjs', { LEVEL: 'low', MODE: 'stateless' }, '2025', 'off', 'token'], | ||
| ] | ||
| const V2_ROWS = [ | ||
| ['v2 high 2025 conv=off', 'v2.mjs', { LEVEL: 'high' }, '2025', 'off'], |
Member
There was a problem hiding this comment.
suggestion: Exercise or remove the long-lived v2 server mode
Every v2 matrix row leaves MODE unset, so coverage stays on the default perrequest mode; the new longlived branch is never tested despite being described as a comparison control. Please add explicit MODE=longlived rows or remove the unused mode and its coverage claim.
Member
Author
There was a problem hiding this comment.
Done in 32f9a89 — added two long-lived control rows (high/low, legacy era only since the long-lived transport tops out at 2025-11-25), asserting the ADR-0009 minted token. They immediately paid off: client resolves on long-lived but not per-request, isolating the documented limitation to the topology.
| with: | ||
| build: false | ||
|
|
||
| # Scoped build (only @posthog/core + @posthog/mcp): an unrelated package's |
Member
There was a problem hiding this comment.
suggestion: Correct the workflow’s build-isolation assumption
This Turbo command follows workspace task dependencies and also builds posthog-node (plus other dependencies), so an unrelated dependency build can fail every harness lane. Please restrict the invocation if possible, or document the actual transitive build scope instead of claiming only @posthog/core and @posthog/mcp are built.
Member
Author
There was a problem hiding this comment.
Right — the closure also pulls in @posthog/types, @posthog/core, and posthog-node (mcp's .d.ts build needs them), so it can't be restricted further. Corrected the comments in the workflow/run-all to document the actual transitive scope (32f9a89).
|
|
||
| const argv = process.argv.slice(2) | ||
| const arg = (name, fallback) => { | ||
| const i = argv.indexOf(`--${name}`) |
Member
There was a problem hiding this comment.
nit: Format the added harness and configuration files
pnpm exec prettier --check still reports formatting differences across the added harness/configuration files, and the @posthog/mcp lint command only covers src, so CI does not catch this drift. Please run the repository’s Prettier configuration over the changed files.
Member
Author
There was a problem hiding this comment.
Formatted the harness with the repo prettier config and extended the package lint to eslint src && prettier --check harness, so the existing Lint packages job now catches drift (32f9a89). Left pnpm-workspace.yaml as-is — it already fails prettier on main, so formatting it here would be unrelated churn.
marandaneto
reviewed
Aug 20, 2026
| @@ -0,0 +1,6 @@ | |||
| --- | |||
marandaneto
approved these changes
Aug 20, 2026
…er, changeset - Exercise the long-lived v2 topology: two matrix control rows (high/low, legacy era only — NodeStreamableHTTPServerTransport tops out at 2025-11-25) expecting the ADR-0009 minted session token. This also corrected the fail-fast header guard from SDK-scoped to era-scoped (a v2 server serving the 2025 era legitimately mints), and checkAlive now probes in the row's own era so a longlived server is not reported dead by a modern-era request. - Correct the scoped-build comments: turbo builds @posthog/mcp plus its workspace dependency closure (types, core, posthog-node), not just core+mcp. - Format the harness with the repo prettier config and gate it in the package lint script so CI catches drift. - Drop the empty changeset. Generated-By: PostHog Code Task-Id: 8656d768-37c4-4e79-8d98-b3830b00c330
Contributor
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
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
2 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.
Implements an integration test setup for
@posthog/mcpatpackages/mcp/harness/, plus a blockingMCP harnessCI workflow. It runs on every PR that affects@posthog/mcp(via the existingcheck-affectedgate), needs no secrets and no network (the PostHog client is an in-process recorder), and works on fork PRs. Nothing ships:harness/is dev-only, outside the package'sfiles.Mega issue: PostHog/posthog#64016
What we test
Real MCP servers over real HTTP (raw JSON-RPC, not the SDK client), instrumented with the workspace build of
@posthog/mcp. Four CI lanes, one job each:official SDK v1@modelcontextprotocol/sdk1.xofficial SDK v2@modelcontextprotocol/{core,server,node}2.xmcp-nest (SDK v1)@rekog/mcp-nest1.9 + SDK v1, statelessLEVEL=high|lowmcp-nest (SDK v2)@rekog/mcp-nest2.0 + SDK v2, both eras (the #4449 stack)LEVEL=high|lowEvery lane covers the core capture surface — tool calls, errors, intent, sessions (conversation-id on/off) — plus identity (client, protocol version) and safety (no warnings, correct
Mcp-Session-Idheader behaviour, server survives custom registrations).LEVEL=highis the documentedinstrument(server);LEVEL=lowis theinstrument(server.server)workaround.The probes cover orderings the matrix can't reach — each sdk lane runs only its own major's halves (--major v1|v2), so a red probe names the stack: handlers registered after
instrument()(the adapter shape), a failure on the first call of a conversation, and a two-page tool catalogue (nextCursor/ttlMs/cacheScope/ result_metamust survive the listing wrapper).The matrix
12 rows (server topology) × 10 assertion columns, current state:
The ✗ cells are known and pinned in
expected-failures.jsonfiles (each entry carries awhy): the four 2025-eraclientcells (documented limitation of the v2 SDK's legacy leg), the two low-levelsessioncells (parked ownership gap), and — on the Nest v2 stack —error message is cleanon both eras (NestJS flattens thrown errors to"Internal server error"). A run passes iff the failing set exactly matches the snapshot: a regression fails the lane, and so does an unexpected improvement, so every moved cell is a deliberate, reviewable diff.Running locally
Full reference (env vars, wire formats, design notes):
packages/mcp/harness/README.md.Design points
@posthog/mcpvia Node self-reference, so they always test the package's owndist/.PORT=0) and announce them on stdout, so rows can't collide; explicitPORT=works for debugging.matrix.mjsexits non-zero on any unexpected red — it's a gate, not a report.@rekog/mcp-nestmajors coexist via pnpm aliases (rekog-mcp-nest-v1/v2); the nest-v2 fixture uses raw JSON Schema tool parameters; the unit suite keeps zod 3 for SDK v1 while fixtures use azod4alias.@modelcontextprotocol/sdkset to^1.30.0.After merge: add the four lane names +
affectedto required status checks (skipped runs satisfy required checks, same as the existingintegrationjob).How did you test this code?
All four lanes green locally and on CI in this PR. nest-v1 16/16 on both levels; nest-v2 35/37 on both levels with exactly the two expected failures; probes 9/9 + 3/3 + 8/8. A red drill (deliberately breaking exception capture in
src/) turned the lane red naming the exact cells; reverting restored green. Hygiene: 648 unit tests pass (jest ignoresharness/), lint and--frozen-lockfileclean,pnpm packtarball contains zero harness files.Created with PostHog Code