feat(mcp): one registry-driven telemetry contract at all three dispatch chokepoints (#9525) - #9579
Open
JSONbored wants to merge 7 commits into
Open
feat(mcp): one registry-driven telemetry contract at all three dispatch chokepoints (#9525)#9579JSONbored wants to merge 7 commits into
JSONbored wants to merge 7 commits into
Conversation
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-28 13:55:13 UTC
Review summary Nits — 6 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9579 +/- ##
==========================================
+ Coverage 89.67% 95.63% +5.95%
==========================================
Files 869 772 -97
Lines 110868 61240 -49628
Branches 26366 21499 -4867
==========================================
- Hits 99421 58565 -40856
+ Misses 10183 1412 -8771
+ Partials 1264 1263 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Bundle ReportBundle size has no change ✅ |
JSONbored
force-pushed
the
feat/mcp-telemetry-9525
branch
from
July 28, 2026 11:49
7143e95 to
3c09bb5
Compare
… three servers Closes #9520. Makes LoopOver's tool contract enforced rather than aspirational: every registered tool on every server is now smoke-called in CI and its answer validated against the schema it advertises. It found a live production bug on the first run, which is the point. THE BUG. The remote server passed each contract schema's `.shape` to registerTool. The SDK re-wraps a raw shape in a plain z.object, which discards the catchall -- so every output modelled as a looseObject was advertised AND ENFORCED as additionalProperties:false, and the SDK rejected the handler's own payload with a -32602 the caller could do nothing about. 20+ tools were returning errors instead of answers to real callers, among them get_pr_maintainer_packet, get_repo_settings, and the whole current-branch family. Passing the schema OBJECTS fixes it; the stdio server already did this, which is why only the remote one was affected. THE DESIGN, and the deliberate difference from metagraphed's validator: smoke arguments are SYNTHESIZED from each tool's own advertised inputSchema rather than read from a hand-maintained table. metagraphed's table is why 92 of its 205 tools are never exercised with nothing to catch it; here a new tool is called the day it is registered, with no table edit. The overrides file holds only the seven tools where a structurally valid value is not a semantically useful one -- mostly dry-run flags the synthesizer would otherwise send as false. Per surface it asserts: tools/list agrees with the registry, every tool advertises a description and object-typed schemas, every outputSchema Ajv-compiles up front, every successful call's structuredContent validates, no tool was skipped, and the negative paths refuse rather than crash. Plus the version tri-lock and the anti-rot guard metagraphed lacks -- every path the release automation reads must still exist, because a lock that only compares constants to each other stays green while the thing meant to update them has stopped running. It reports the split rather than a bare pass: remote 107 tools / 99 validated, stdio 102 / 25, miner 11 / 11. A tool that declines in a cold fixture env has answered correctly but did not exercise its schema, and hiding that behind a green check would be the same self-congratulatory reporting this replaces. Runs as a vitest spec rather than a bare script because the remote server imports cloudflare: modules the plain Node loader cannot resolve, and because it then reuses the same seeded-D1 helper the rest of the suite does instead of standing up a second fixture environment. Helpers are pure and separately covered at 100% branch.
…sweep missed Review catch on PR #9575. The sweep matched 'Schema: X.shape,' with a trailing comma, so it rewrote every multi-line register config and skipped the ten single-line ones -- open_pr, file_issue, apply_labels, post_eligibility_comment, create_branch, delete_branch, close_pr, and the three plan tools -- which end with a closing brace instead. Latent rather than active for these ten, which is why the contract validator did not flag them: LocalWriteActionOutput and PlanViewOutput declare exactly the fields their handlers return today, so a closed schema had nothing extra to reject. The next field either one gains would have started returning -32602 to callers, exactly as the twenty tools this PR set out to fix already were.
…ch chokepoints Closes #9525. Before this the three MCP servers had three telemetry implementations with three property lists, no exception capture on tool dispatch, and no tracing. They now share one definition of what a tool call emits, in @loopover/contract, with each runtime keeping only its own thin sink. Per call, subject to each surface's existing gates: the minimal identity-free usage_event, and PostHog's own $mcp_tool_call carrying REDACTED, size-capped arguments and results. A genuine throw is additionally captured as an exception grouped by mcp_tool + error_code; an error ENVELOPE is a tool answering, not a crash, and is not one. The self-host path also opens an OTel span mcp.tool/<name> whose attributes are a strict subset -- never arguments. error_code is a closed developer-defined set. A caller-supplied string in that position would be both a cardinality explosion and untrusted text injected into a dashboard. Payload exclusion is DERIVED from the contract, not a hand list: admin-category and operator-authenticated tools never send arguments or results, so an operator tool added tomorrow is excluded the day it is registered. When payloads are excluded the event says so rather than silently omitting them -- an absent field and a withheld one are different facts and only one is worth alerting on. Two bugs found by the tests that assert this rather than assume it: - The PEM-header alternative of the secret-value pattern could never match. A leading word-boundary anchor before a hyphen has no boundary to find, so a full private-key header passed through untouched. - Redaction kept secret-shaped KEYS with a [redacted] value. The key NAME survived, and a property literally named coldkey is itself a finding by the repo's own forbidden-content rules. Such keys are now dropped outright; no telemetry question is answered by a key name. Gates are unchanged and deliberately not merged: usage events keep POSTHOG_API_KEY, exception capture keeps WORKER_POSTHOG_API_KEY, stdio stays double-gated opt-in, the miner rides its own opt-in client. The span runner is a nullable registry (the pattern private-config-admin-registry.ts already uses) so the Workers bundle never pulls the self-host OTel module in for a collector it does not have. The stdio server emits the new pair ALONGSIDE its legacy mcp_tool_call event rather than instead of it: that event has been there since #6236 and an operator's dashboards read it. Every sink is best-effort and every wrapper catches its own failures, asserted by a test that makes the sink throw on both paths and checks the ORIGINAL error still reaches the caller.
JSONbored
force-pushed
the
feat/mcp-telemetry-9525
branch
from
July 28, 2026 11:53
3c09bb5 to
6489953
Compare
…tool The chokepoint test (test/unit/mcp-local-telemetry-chokepoint.test.ts) rejected the payload design within one run by finding a real commit message on the wire. It runs the stdio server as a real subprocess against a local recorder and asserts what actually leaves the process, so it was checking the guarantee rather than the intent. The first design included arguments and results except for admin/operator tools, on the theory that redaction made the rest safe. It does not: most of these tools take the USER'S OWN CONTENT as their input. lint_pr_text takes the PR body. check_slop_risk takes the commit messages. intake_idea takes a freeform brief. None of that is secret-SHAPED, so a redaction pass ships it verbatim -- and LoopOver's telemetry has promised since #6238 that the call's content never leaves the machine. Inverted: payloads are excluded for all 125 tools, with an opt-in allowlist that is empty. The mechanism stays because PostHog's MCP-Analytics event family is defined to carry those fields and a future tool whose input is genuinely server-derived metadata could qualify -- but adding one is now an argued change with the tool named in the diff, not a default. The registry meta-test asserts the allowlist is empty rather than asserting a handful of names, and the operator surfaces are excluded a second, independent way so populating it can never accidentally qualify one. Also extends the chokepoint test's exhaustive property allowlist to every event rather than only the first. Adding two events without extending that check is precisely how a new field would have reached the wire unexamined.
… chokepoint codecov/patch flagged all three: the wrapper in dispatch-telemetry.ts had tests, the I/O half had none. Both sides of every gate now, including the two that are deliberately independent -- exception capture on with usage events off -- plus the never-rejects guarantee on the deferred capture with no reachable host. Also drops a redundant API-key guard inside captureEvents. Its only caller already gates on the key, so the second check was unreachable by construction; it now takes the resolved key instead of re-deriving one it cannot fail to find.
JSONbored
force-pushed
the
feat/mcp-telemetry-9525
branch
from
July 28, 2026 12:45
a23261d to
615df1b
Compare
…ackage sinks codecov/patch's second pass flagged what the first round missed: the stdio dispatch emitter's no-contract and no-error branches, the miner's thin capture send, and the sink-injected path through LoopoverMcp's constructor -- every existing test builds that server without a sink, so only the NOOP fallback side of that '??' was ever taken. Also makes withMinerToolErrorHandling's toolName REQUIRED. It was optional so an un-threaded caller would still compile; every registration passes it, and leaving it optional made 'instrumented' a property of each call site rather than of the wrapper -- plus an unreachable branch on both arms.
The last two partial branches on the contract's telemetry module: stringify answers undefined for a bare function or symbol rather than throwing, so the nullish arm and the empty-string check are a separate path from the catch that handles a BigInt. 100% branches on the module now.
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
1 participant
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.
Closes #9525. Stacked on #9575 (#9520), which is stacked on #9565 (#9537).
What changed
Before this, the three MCP servers had three telemetry implementations with three property lists, no exception capture on tool dispatch, and no tracing. They now share one definition of what a tool call emits —
packages/loopover-contract/src/telemetry.ts— with each runtime keeping only its own thin sink. That is what makes a single property allowlist enforceable rather than aspirational.Per call, subject to each surface's existing gates:
usage_event(tool, category, surface, ok, duration, closed error code);$mcp_tool_call, additionally carrying redacted, size-capped arguments and results;$exceptioncapture for a genuine throw, grouped bymcp_tool+error_code. An error envelope is a tool answering, not a crash, and is deliberately not one;mcp.tool/<name>whose attributes are a strict subset — never arguments.One chokepoint per server: the remote's
registerwrapper, the stdio server'swrapStdioToolHandler, and the miner'swithMinerToolErrorHandling. ~230 handlers across the three are instrumented without one of them being touched.Two bugs the tests found
Both come from writing tests that assert the safety properties rather than assume them, which is what requirement 7 asks for.
The PEM alternative of the secret-value pattern could never match. A leading
\bbefore a hyphen has no word boundary to find, so a full-----BEGIN RSA PRIVATE KEY-----passed through redaction untouched. Caught by theFORBIDDEN_CONTENTtest.Redaction kept secret-shaped keys with a
[redacted]value — so the key name survived, and a property literally namedcoldkeyorgithubTokenis itself a finding under this repo's own forbidden-content rules. Such keys are now dropped outright, name and value. No telemetry question is answered by a key name.Decisions worth reviewing
Payloads are excluded for every tool, and that is the second design. The first one included arguments and results except for admin/operator tools, reasoning that redaction made the rest safe. The subprocess-level chokepoint test rejected it within one run by finding a real commit message on the wire — most of these tools take the user's own content as their input (
lint_pr_texttakes the PR body,check_slop_risktakes the commit messages), and none of that is secret-shaped, so a redaction pass ships it verbatim. LoopOver's telemetry has promised since #6238 that the call's content never leaves the machine, and that promise wins. The opt-in allowlist is empty; the registry meta-test asserts it stays empty rather than asserting a handful of names.error_codeis a closed set. A caller-supplied string in that position would be both a cardinality explosion and untrusted text injected into a dashboard. Anything unmapped isunknown_errorrather than a guess.Gates are unchanged and deliberately not merged. Usage events keep
POSTHOG_API_KEY; exception capture keepsWORKER_POSTHOG_API_KEY.src/api/worker-posthog.tsexplains at length why those two are separate — merging them would silently activate Worker exception capture inside a self-hoster's Node process the moment they set the MCP telemetry key. Stdio stays double-gated opt-in; the miner rides its own opt-in client.The span runner is a nullable registry, the same pattern
private-config-admin-registry.tsalready uses, so the Workers bundle never pulls the self-host OTel module in for a collector it does not have.The stdio server emits the new pair alongside its legacy
mcp_tool_callevent, not instead of it. That event has been there since #6236 and an operator's existing dashboards read it; removing it here would break them silently.posthog-node, not a hand-built$exception. The issue text specified raw fetch, reasoning from metagraphed's bundle constraint. That constraint does not hold here: this Worker already bundlesposthog-nodefor #6235, and PostHog's own docs warn a hand-constructed exception event "fails in the vast majority of cases because the exception event schema is strict" — which is exactly the conclusionsrc/api/worker-posthog.tsreached and recorded in #8288.Not in this PR
Requirement 8 (the #8606 dashboard updates). Those live in the PostHog project, not this repo, and the PostHog MCP connector is unauthorized in my session — I can't reach them. The event shapes are stable and documented above; the dashboards need someone with project access.
The miner's Sentry→PostHog swap for exception capture remains #8292's. This PR routes the miner's dispatch events and its exception capture through the PostHog client that issue already added (
lib/posthog.ts); it does not touch the parallel Sentry seam, which #8292 owns.Validation
npx vitest run test/unit test/contract— 23,513 passed, 6 skipped, 0 failed.npm run typecheckclean across the root and all three stricter packages.npm run validate:mcpgreen: remote 107/99, stdio 102/25, miner 11/11.selfhost:env-reference:check,miner:env-reference:check,selfhost:validate-observability,ui:openapi:checkgreen.src/mcp/dispatch-telemetry.tsat 100% statements and branches; the contract's telemetry module at 100% functions and lines. Coverage includes the never-throws guarantee on both paths, both sides of every gate, and the fallback when a tool has no registry entry.