Repository navigation
feat(mcp): add posthogmcpgo, MCP analytics for mark3labs/mcp-go servers - #409
Conversation
Adds a posthogmcpgo module next to posthogmcpsdk. Instrument returns a server.ServerOption: a tool filter injects the context and llm_model arguments into listed schemas, and a tool handler middleware strips them, times the call and captures $mcp_tool_call (and $exception on failure) through the shared posthogmcp core, so events match posthogmcpsdk. Out of scope for this pass: conversation anchoring, the missing-capability tool, $mcp_unknown_tool, $mcp_input_required, intent fallback. Tested: go vet ./... and go test ./... in posthogmcpgo pass, including a parity test against posthogmcpsdk; end to end, hashicorp/terraform-mcp-server's streamable HTTP server sent $mcp_tool_call events with session ids derived from Mcp-Session-Id.
posthog-go Compliance ReportDate: 2026-11-05 20:55:06 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 8f4b257: 1 Must fix, 9 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
mcp-go's WithInputSchemaValidation checks calls against the registered schema before tool middleware runs, so adding context/llm_model to a schema with additionalProperties:false made every call fail (WithStrictInputSchemaDefault closes every schema). Such schemas are now left unchanged. CI had no job for the new module: add unit tests (Go 1.25 and 1.x, race, plus a build against the pinned posthog-go release), a format/tidy check, and add it to the release workflow's sync and tag loops. Add a changeset. Tested: go test -race ./... in posthogmcpgo (new strict validating server test failed before the fix, passes after); pinned build and go mod tidy clean.
…nto open schemas
What: posthogmcpgo now captures tool calls from mcp-go call hooks
(BeforeCallTool, AfterCallTool, OnError) appended to the server's existing
hooks, so events record the response the client received. Input and output
schema validation failures become failed calls. The middleware only strips
the analytics arguments. It captures instead when the call runs as a task,
or when a later server.WithHooks replaced the hooks (reported once).
Why: a pre-PR review found seven verified defects that could break a host
server or misreport calls:
- validating servers rejected advertised calls for schemas that can refuse
extra properties. Injection now uses a root-keyword allowlist.
- task tools were advertised with the arguments but never stripped. They are
now left unchanged and not captured.
- input validation failures sent no event, and output validation failures
were captured as successes.
- absent arguments were captured as null (posthogmcpsdk sends {}).
- typed InputSchema became RawInputSchema, hiding properties from later
tool filters. The representation is now kept, on a copy.
- $mcp_protocol_version was missing off HTTP. It now comes from
AfterInitialize per session, dropped on unregister.
Tested: each defect has a table-driven test that fails on 882f191 and
passes now. gofmt -l . (clean), go vet ./... (clean),
go test -race -count=10 ./... (ok). go mod tidy leaves go.mod/go.sum
unchanged. Pinned-release build with both replaces dropped:
GOFLAGS=-mod=mod go build ./... && go vet ./... (ok).
Reviewer notes: per-call state is keyed by the *CallToolRequest that
mcp-go's hooks share, and entries older than an hour are swept, because a
handler panic recovered by a transport fires no after/error hook.
patternProperties is excluded from the allowlist: a pattern matching
"context" can reject a string value.
From a second adversarial review of the hooks-based capture: - Servers sharing one *server.Hooks each captured every call (3 servers gave 3 events per call, and an uninstrumented server sent events). Hooks now act only on their own server's requests (server.ServerFromContext). - A synchronous call mcp-go rejects for a task tool was captured as a failure. Only calls to tools the middleware wraps are tracked now. - A closed session's generated session id stayed in memory until a later sweep. Unregistering a session now drops it. - A session closing mid-call lost $mcp_protocol_version. It is read when the call starts. README documents what hook-based capture implies: resolvers get the request context (use WithHTTPContextFunc/WithStdioContextFunc), an unrecovered handler panic sends no event, and what a later WithHooks loses. Tested: go test -race ./... (4 new tests, each failing before the fix); the adversaries' repros now pass; pinned-release build and go mod tidy clean.
main released v1.40.0 and posthogmcpsdk now requires it, so go mod tidy raises posthogmcpgo's minimum to match. Tested: go test -race ./..., pinned build.
What: tool-owned context/llm_model are the tool's data (passed through, kept in $mcp_parameters, never intent/model); SSE sessions derive $session_id from the transport id, only stdio and in-process get a rotating generated id; schemas decode with UseNumber; minProperties and root "$ref": "#" schemas are no longer extended; ErrToolNotFound from a registered tool is a captured failure; task-run calls send no event; in-flight calls are bounded by count (1024, oldest dropped) instead of a 1h sweep; advertise copies session tools once per listing. Why: the cross-SDK contract (mcp-analytics spec) and never altering the host server's behavior on production servers. Tested: gofmt clean; go vet ./...; go test -race -count=2 ./...; go mod tidy leaves go.mod/go.sum unchanged; pinned build (replaces dropped) builds and vets. New table-driven tests fail before the fix.
dustinbyrne
left a comment
There was a problem hiding this comment.
Two issues remain in recursive-schema preservation and failure capture before task creation; details inline. The separate module, validation-aware hooks and release wiring otherwise look appropriate.
AI-assisted review.
…s that may ref their root Review on #409 found two gaps: - beforeCall dropped every call run as a task, so a call mcp-go refuses before creating a task (e.g. a tool filter denial) sent no failed event. Calls are now tracked until mcp-go answers; errors are captured, and a CreateTaskResult still is not. - Only a literal "$ref": "#" counted as recursion, so a $ref to the root's $id made nested objects require the analytics arguments. Any $ref other than a "#/..." pointer, $dynamicRef, or $recursiveRef now leaves the schema unchanged. Also fixes the WithContextParameter doc, which said a tool's own context is captured as intent. Tested: go test -race -count=1 ./... in posthogmcpgo (new cases fail before the fix), go vet, gofmt, go mod tidy (no diff).
There was a problem hiding this comment.
PostHog Review · Standard · reviewed f4c158b: 2 Should fix. This pull request is large, so the review ran in 4 parts with less depth than usual.
This is review round 3 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
dustinbyrne
left a comment
There was a problem hiding this comment.
The three previous findings are addressed and their original regressions pass. One additional lifecycle issue remains: tool deregistration during a call loses argument ownership, and real HTTP delivery confirms injected intent can enter captured parameters without intent-specific redaction. Details inline.
AI-assisted review.
Capture re-read the registry and resolved the session after the handler ran. Review on #409 found two consequences: - A tool that unregisters itself in its handler lost argument ownership, so the injected context and llm_model landed in $mcp_parameters unredacted and no intent was sent. - A stdio or in-process call running past the 30-minute inactivity timeout rotated $session_id at completion, splitting one conversation. callStart now holds the tool info and $session_id taken at arrival, in both the hook and middleware paths, and completion counts as session activity. Also checks server ownership before looking up session tools in beforeCall. Tested: go test -race -count=1 ./... in posthogmcpgo; both new tests fail before the fix with their stable-tool and inactivity controls passing; go vet, gofmt, go mod tidy (no diff).
dustinbyrne
left a comment
There was a problem hiding this comment.
Re-reviewed f3b284f. The earlier findings are addressed, including ownership retention through tool deregistration and intent redaction. Focused regressions and queued HTTP delivery checks pass. One optional shared-guide update is noted inline.
AI-assisted review.
Follow-up to #409, from review: CONTRIBUTING.md listed only otel and posthogmcpsdk in the nested-module checks, and RELEASING.md left posthogmcpgo out of the shared release version, though the release workflow already includes it. Docs only; no code or CI change.
* docs: add posthogmcpgo to the contributing and releasing guides Follow-up to #409, from review: CONTRIBUTING.md listed only otel and posthogmcpsdk in the nested-module checks, and RELEASING.md left posthogmcpgo out of the shared release version, though the release workflow already includes it. Docs only; no code or CI change. * docs(contributing): remove both local replaces when copying posthogmcpgo Review fix (dustinbyrne P2): posthogmcpgo replaces both the core and posthogmcpsdk, so the temporary-copy check must drop both, as CI does. Wording-only; no tests applicable.
💡 Motivation and Context
posthog-go can instrument MCP servers built on the official Go SDK (
posthogmcpsdk), but many Go MCP servers in production are built onmark3labs/mcp-go. Those servers get no MCP analytics today.This adds
posthogmcpgo, a module for mark3labs/mcp-go.Instrumentreturns oneserver.ServerOption:contextandllm_modelarguments to listed tool schemasWithInputSchemaValidationorWithOutputSchemaValidationare recorded as failures, likeposthogmcpsdkrecords themEvents go through the shared
posthogmcpcore, so they matchposthogmcpsdk.Not in this pass: conversation anchoring, the missing-capability tool,
$mcp_unknown_tool,$mcp_input_required, intent fallback.Decisions worth a look:
*server.Hooksdon't double-capture. If the host passesWithHooksafterInstrument(replacing ours), the middleware detects it, falls back to capturing, and reports it once through the error handler.WithIdentity/WithPropertiesrun from the hooks, so values added by host tool middleware aren't visible; request-scoped values go on the context withWithHTTPContextFunc/WithStdioContextFunc. Documented in the README.AddTaskTool) are listed unchanged and not captured, since mcp-go never runs middleware for them. Those calls carry no intent.posthogmcpsdkdoesn't: a tool's owncontext/llm_modelis tool data (kept in$mcp_parameters, never read as intent), and calls run as tasks emit nothing until the task's terminal status can be observed. HTTP+SSE sessions derive their id like streamable HTTP. Aligningposthogmcpsdkon tool-owned arguments is a follow-up.$mcp_error_typefor tool error results, same asposthogmcpsdk. Letting servers classify errors is a cross-SDK question.CI: adds a unit-test job (Go 1.25 and 1.x, race, plus a build against the pinned posthog-go release), a format and tidy check, and adds the module to the release workflow's sync and tag loops so releases tag
posthogmcpgo/vX.Y.Z.💚 How did you test it?
go vet ./...andgo test -race ./...inposthogmcpgo, including a parity test that runs the same call throughposthogmcpsdkand compares the captured properties, and a strict validating server test (WithInputSchemaValidation+WithStrictInputSchemaDefault) that failed before the closed-schema fix.replacedirectives dropped, as consumers will.$mcp_tool_callevents arrived with intents, error text, and session ids derived fromMcp-Session-Id.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written with Claude Code (Claude Opus). Design choice: mcp-go has no receiving middleware like the official SDK, so the adapter uses
WithToolFilterfor schema injection andWithToolHandlerMiddlewarefor capture, reusing theposthogmcpcore for event shape, redaction and session id hashing. Reviewed by Lucas, then two rounds of adversarial pre-PR review (Claude Opus and Sonnet). Each verified finding became a regression test that failed before its fix: constrained schemas, task tools, input/output validation failures, shared hooks, absent arguments, typed schemas, protocol version per transport, session cleanup.