Repository navigation
feat(mcp): align PostHog::MCP with the MCP analytics spec - #332
Open
lucasheriques wants to merge 4 commits into
Open
lucasheriques wants to merge 4 commits into
lucasheriques wants to merge 4 commits into
Conversation
What: get_more_tools advertises all four annotation hints as false; the injected `context` description uses the shared cross-SDK wording; `llm_model` is never added to a schema's `required` list; a tools/call naming an unregistered tool emits `$mcp_unknown_tool` instead of a failed `$mcp_tool_call` (no `$exception` sibling, same client response). Why: customers' saved insights must keep working after the 1.0 release, and the annotations and `context` wording were promised to OpenAI's plugin review. Tests: bundle exec rspec spec/posthog/mcp (203 examples, 0 failures, 2 pending); bundle exec rspec (2111 examples, 0 failures, 40 pending); bundle exec rubocop (no offenses). Notes: no acceptance adapter declares capability tags in this repo, so `@mcp_unknown_tool_capable` is not declared. Ruby has no send_feedback tool. A registered-but-disabled tool and an unreadable registry stay failed calls.
posthog-ruby-sync Compliance ReportDate: 2026-11-06T18:03:06.099991+00:00
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 7ms |
| Format Validation.Event Has Uuid | ✅ | 4ms |
| Format Validation.Event Has Lib Properties | ✅ | 4ms |
| Format Validation.Distinct Id Is String | ✅ | 7ms |
| Format Validation.Token Is Present | ✅ | 5ms |
| Format Validation.Custom Properties Preserved | ✅ | 5ms |
| Format Validation.Event Has Timestamp | ✅ | 5ms |
| Format Validation.Non Utc Event Timestamp Is Converted To Utc | ✅ | 5ms |
| Retry Behavior.Retries On 503 | ✅ | 5227ms |
| Retry Behavior.Does Not Retry On 400 | ❌ | 5ms |
| Retry Behavior.Does Not Retry On 401 | ❌ | 5ms |
| Retry Behavior.Respects Retry After Header | ✅ | 8009ms |
| Retry Behavior.Implements Backoff | ❌ | 328ms |
| Retry Behavior.Retries On 500 | ✅ | 5144ms |
| Retry Behavior.Retries On 502 | ✅ | 5110ms |
| Retry Behavior.Retries On 504 | ✅ | 5158ms |
| Retry Behavior.Max Retries Respected | ❌ | 581ms |
| Deduplication.Generates Unique Uuids | ✅ | 13ms |
| Deduplication.Preserves Uuid On Retry | ✅ | 5111ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ✅ | 10247ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ✅ | 5160ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 15ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 7ms |
| Compression.Sends Gzip When Enabled | ✅ | 5ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 5ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 2ms |
| Batch Format.Multiple Events Batched Together | ❌ | 13ms |
| Error Handling.Does Not Retry On 403 | ❌ | 4ms |
| Error Handling.Does Not Retry On 413 | ❌ | 3ms |
| Error Handling.Retries On 408 | ✅ | 5143ms |
Failures
retry_behavior.does_not_retry_on_400
500, message='Internal Server Error', url='http://sdk-adapter:8080/capture'
retry_behavior.does_not_retry_on_401
500, message='Internal Server Error', url='http://sdk-adapter:8080/capture'
retry_behavior.implements_backoff
500, message='Internal Server Error', url='http://sdk-adapter:8080/capture'
retry_behavior.max_retries_respected
500, message='Internal Server Error', url='http://sdk-adapter:8080/capture'
batch_format.multiple_events_batched_together
Expected 1 requests, got 5
error_handling.does_not_retry_on_403
500, message='Internal Server Error', url='http://sdk-adapter:8080/capture'
error_handling.does_not_retry_on_413
500, message='Internal Server Error', url='http://sdk-adapter:8080/capture'
Feature_Flags Tests
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 8ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 6ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 7ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 5ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 5ms |
| Request Payload.Groups Round Trip | ✅ | 4ms |
| Request Payload.Groups Default To Empty Object | ✅ | 5ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 4ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ❌ | 5ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 4ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 2ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 4ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 6ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ✅ | 4ms |
| Retry Behavior.Retries Flags On 502 | ✅ | 106ms |
| Retry Behavior.Retries Flags On 504 | ✅ | 106ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ✅ | 6ms |
Failures
request_payload.disable_geoip_omitted_defaults_to_false
Field 'geoip_disable' not found in /flags request body at path 'geoip_disable'. Available keys: ['distinct_id', 'groups', 'person_properties', 'group_properties', 'flag_keys_to_evaluate', 'token']
posthog-ruby-async Compliance ReportDate: 2026-11-06T18:03:52.371629+00:00
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 108ms |
| Format Validation.Event Has Uuid | ✅ | 106ms |
| Format Validation.Event Has Lib Properties | ✅ | 107ms |
| Format Validation.Distinct Id Is String | ✅ | 106ms |
| Format Validation.Token Is Present | ✅ | 106ms |
| Format Validation.Custom Properties Preserved | ✅ | 108ms |
| Format Validation.Event Has Timestamp | ✅ | 106ms |
| Format Validation.Non Utc Event Timestamp Is Converted To Utc | ✅ | 106ms |
| Retry Behavior.Retries On 503 | ✅ | 5411ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 2109ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 2107ms |
| Retry Behavior.Respects Retry After Header | ✅ | 8115ms |
| Retry Behavior.Implements Backoff | ✅ | 15622ms |
| Retry Behavior.Retries On 500 | ✅ | 5211ms |
| Retry Behavior.Retries On 502 | ✅ | 5212ms |
| Retry Behavior.Retries On 504 | ✅ | 5211ms |
| Retry Behavior.Max Retries Respected | ✅ | 15621ms |
| Deduplication.Generates Unique Uuids | ✅ | 109ms |
| Deduplication.Preserves Uuid On Retry | ✅ | 5210ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ✅ | 10317ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ✅ | 5213ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 109ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 106ms |
| Compression.Sends Gzip When Enabled | ✅ | 106ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 105ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 4ms |
| Batch Format.Multiple Events Batched Together | ✅ | 110ms |
| Error Handling.Does Not Retry On 403 | ✅ | 2107ms |
| Error Handling.Does Not Retry On 413 | ✅ | 2107ms |
| Error Handling.Retries On 408 | ✅ | 5211ms |
Feature_Flags Tests
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 107ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 105ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 106ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 106ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 106ms |
| Request Payload.Groups Round Trip | ✅ | 106ms |
| Request Payload.Groups Default To Empty Object | ✅ | 106ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 106ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ❌ | 105ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 107ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 2ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 106ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 111ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ✅ | 105ms |
| Retry Behavior.Retries Flags On 502 | ✅ | 240ms |
| Retry Behavior.Retries Flags On 504 | ✅ | 208ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ✅ | 107ms |
Failures
request_payload.disable_geoip_omitted_defaults_to_false
Field 'geoip_disable' not found in /flags request body at path 'geoip_disable'. Available keys: ['distinct_id', 'groups', 'person_properties', 'group_properties', 'flag_keys_to_evaluate', 'token']
SDK performance: posthog-rubyBenchmarks completed. No changes flagged for review. Flags require at least a 5% change in the median and all six pairs agreeing on direction. This is a noise filter, not proof of a performance change or a CI gate. View all results
Values are medians of paired measurements. Not flagged does not mean unchanged. Unavailable means a measurement could not be provided, not zero. Estimated allocation measurements retain their instrumentation uncertainty. RSS measures the whole process, not just SDK allocations; observed peaks are sampled checkpoints. Paired ranges in the diagnostics describe variability, not confidence intervals. Each shard uses its own runner; compare Before PR against With PR within a row, not across shards. Compared 102 metric rows; 102 rows shown in all results. Omitted or shortened data remain in |
…I snapshot Tested: bundle exec rake public_api:check passes.
The spec says an optional event never comes with $mcp_initialize unless the client sent initialize. record_unknown_tool now prepares the request with skip_initialize, like the virtual-tool paths do. Tested: the unknown-tool spec now asserts only $mcp_unknown_tool is sent (it failed first with [$mcp_initialize, $mcp_unknown_tool]); bundle exec rspec spec/posthog/mcp: 203 examples, 0 failures; rubocop clean.
lucasheriques
marked this pull request as ready for review
November 6, 2026 17:05
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 02608a6: 1 Should fix.
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.
What: record_unknown_tool now receives the handle dispatch_tool_call resolved, passes it to session preparation and stamps $mcp_conversation_id, as the failed-tool-call path does. skip_initialize stays, and a minted handle is never stamped. Why: spec lists $mcp_conversation_id on $mcp_unknown_tool; the event fell back to another $session_id and detached from its conversation. Tested with: bundle exec rspec spec/posthog/mcp (205 examples, 0 failures); bundle exec rubocop (no offenses); rake public_api:check (rc=0). New spec failed first on the session/handle assertion.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-adding the stamphog label gives the same result unless the changed files change.
stamphog can't approve this pull request automatically. Two gates refused it: the deny-list gate matched public_api, because public_api_snapshot.txt changes with the new PostHog::MCP::Event::UNKNOWN_TOOL constant. The tier gate classified the change as T2-never (132 lines across 11 files, cross-cutting, a feature), a category that always needs a human reviewer.
The size and prerequisites gates passed, so size is not the problem. Please ask a human maintainer to review this pull request. Splitting it into smaller pieces, such as the get_more_tools annotations, the llm_model default, and the unknown-tool event, may make that review easier, though the public API change will still need a person to look at it.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: public_api |
| size | ✓ | 59L, 5F substantive, 132L/11F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (132L, 11F, cross-cutting, feat) |
| stamphog 2.7.0 | .stamphog/policy.yml @ unknown · reviewed head 953d6d2 |
This branch has not been deployed
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
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.
Why
We're aligning every PostHog MCP SDK with the sdk-specs MCP spec before
@posthog/mcp1.0 and a stableposthog.mcp, so customers' insights mean the same thing whichever SDK sent the data. The annotations and thecontextwording were also promised to OpenAI's plugin directory review this week.Changes
get_more_toolsannotations are allfalse, per "Virtual tool annotations". The tool records an event in the developer's project, so it isn't read-only, and repeat calls send more events. Thetitleis gone because the spec's acceptance scenario matches the annotations JSON exactly. The Go adapter makes the same change in posthog-go#418.contextdescription is the shared text every SDK now sends: a few words of purpose, people by role, no quoting the user.llm_modelis optional (add_model_parameterdefaults torequired: false), per sdk-specs#189.$mcp_unknown_toolwith$mcp_tool_name, session and identity, instead of a failed$mcp_tool_callplus$exception. The client gets the same error as before. The spec says an unregistered tool is never a tool call. A server whose registry the SDK can't read still records a failed call, as the spec allows.EXPERIMENTAL_NOTICEand the README banner are unchanged.How I tested
bundle exec rspec spec/posthog/mcp: 203 examples, 0 failures, 2 pending (existing auth_info skips).bundle exec rspec: 2111 examples, 0 failures, 40 pending.bundle exec rubocop: no offenses.tools/list$mcp_unknown_tool, with no sibling events and a client response identical to an uninstrumented serverPre-PR gate: a Sonnet adversary found an unknown-tool call also sent
$mcp_initializewith noinitializerequest. Fixed, and the spec now asserts only$mcp_unknown_toolis sent. Its concurrency runs (60 and 100 threads) misclassified nothing. Ordinary tool calls still send$mcp_initializeon a session's first request, as main does today; that's a separate fix.