feat(mcp): one registry-driven telemetry contract at all three dispatch chokepoints (#9525) - #9579
Merged
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.
7 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
Rebase onto main after #9579 and #9580 landed. The newly-enabled noUnusedLocals immediately flagged twelve dead symbols in code merged since this PR opened -- including two I left in #9580 myself: - src/queue/processors.ts: PrCommandPrologueOutcome imported but unused (the adapter's annotated return type was dropped in favour of inference), plus eight unused bindings in the prologue destructures -- handlers that do not need `pr`, `settings`, `authorization` or `command` were still pulling them out. - src/queue/pr-command-prologue.ts: LoopOverMentionCommandName, superseded by LoopOverActionCommandName once the spec narrowed to action verbs. - src/mcp/dispatch-telemetry-sink.ts: an unused McpToolCallTelemetry import from #9579. Which is the point of the PR: the flags catch dead code at the commit that introduces it rather than at the next audit. Zero behaviour change -- every removal is a binding or an import TypeScript proved unreferenced, and the suite passes 24,014 tests. The rebase conflict itself was in processors.ts's import block: main added the pr-command-prologue import on the same lines this PR removed the unused runRetentionPrune one. Both intents kept.
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
…#9553, #9570, #9571, #9572) (#9573) * build(typescript): enable noUnusedLocals/noUnusedParameters repo-wide (#9553) Dead code is the substrate every drift bug in the 2026-07-27 audit grew on: a stale import or an orphaned constant reads exactly like a live wire, so the next person greps, finds it, and reasons about a code path that no longer runs. Enabling the flags made the compiler enumerate all 515 instances. Every one in src/** and packages/** was traced to its replacement before deletion -- all 82 were genuine supersession leftovers, no behaviour bug hiding among them -- but the triage turned up three real problems that were invisible under the noise: 1. src/queue/processors.ts -- sweepRepoBacklogConvergence accepted `requestedBy` ("schedule" | "api" | "test") and dropped it. Every sibling sweep stamps it into recordAuditEvent's metadata; both agent.sweep.backlog_convergence events here omitted it, so those records could not be attributed to a schedule vs a manual API trigger. Now wired into both. (Note the mechanical fix would have been to rename it `_requestedBy`, which cements the gap instead of closing it.) 2. test/unit/openapi.test.ts -- the #9302 REST<->MCP parity guard asserted against src/mcp/server.ts's gatePrecisionOutputSchema and maintainerMeasurementReportOutputSchema, which the tools stopped registering when #9518 moved their outputs to @loopover/contract. The shapes are still identical, so nothing had drifted YET -- but the guard was watching objects no runtime reads and would not have caught a future contract change. Re-anchored onto GetGatePrecisionOutput.shape / GetOutcomeCalibrationOutput.shape, which is what the tools actually register, and what that file's own header comment already claimed it did. 3. src/github/resolve-command.ts was reachable only from its own test, because src/review/review-memory-wire.ts carried a SECOND byte-identical copy of normalizeResolveFindingRef (regex included) and that copy was the one production used. Two independent implementations of the same public-safety validation, free to drift. Deduped onto the original via re-export; dead-source-files:check now passes on a file it was about to start failing on. Also corrects packages/loopover-engine/src/scoring/preview.ts's header, which claimed a ReDoS guarantee via a hasUnsafeWildcardCount import that had been dead since 625e236 deduped its label matching onto label-match.ts. The guarantee is real and unchanged; it now arrives through labelMatchesPattern, and the comment says so. Pre-existing import-specifier violations fixed in the same pass, since the tree has to be green for the flags to mean anything: - scripts/actionlint.ts imported a `.ts` specifier (TS5097) - test/unit/contract-registry.test.ts had three `.js` specifiers in a Bundler zone - check-dead-source-files-script.test.ts tripped check-import-specifiers on its own string FIXTURES, the same self-referential false positive that checker's ALLOWED_FILENAMES already documents for its own test Mechanics: unused parameters are renamed with a leading underscore, never deleted -- they are positional, so removing one silently re-binds every later argument. Everything else was removed by its real TypeScript AST node span (a regex pass was tried first and mis-bounded declarations badly enough to produce unparseable files). Full suite green: 23,894 passed, 0 failed. tsc clean with the flags on. * chore(engine): bump to 3.15.4 for the dead type-import removal in gate-advisory.ts check-engine-parity holds the two hand-duplicated gate-decision twins (packages/loopover-engine/src/advisory/gate-advisory.ts and src/rules/advisory.ts) in lockstep: touching one without the other requires an engine version bump. That is the mechanism, and it is doing its job here. the engine twin. They are genuinely dead there and NOT in the host: the engine copy is a deliberately slimmed re-implementation (#4881) that omits buildIssueAdvisory / addIssueFindings / collisionClustersForPull, which are what use those types on the host side. So there is no matching host edit to make -- the asymmetry is correct, and the version bump is the sanctioned way to record it. No behaviour change: type-only imports are erased at compile time. The bump exists so the parity contract stays enforceable, not because the gate decides anything differently. packages/loopover-miner/expected-engine.version moves in lockstep, as its own check requires. * fix(scripts,mcp): restore actionlint's `.ts` specifier and drop a dead shape #9565 added Two rebase follow-ups after #9565 and #9574 landed. 1. scripts/actionlint.ts gets its `.ts` extension back. This PR had removed it to satisfy check-import-specifiers, which broke the script outright -- it runs under `node --experimental-strip-types`, whose ESM resolver does no extension resolution, so the process dies at startup with ERR_MODULE_NOT_FOUND. #9565 independently reached the same conclusion and added TYPE_STRIPPED_ENTRYPOINTS to the checker for exactly this file, so the extension is now permitted where it is required. Verified by running `npm run actionlint`, which fails before this change and passes after. #9565's version of the checker is taken wholesale over this PR's: it solves the same two problems (that entrypoint set, plus allowlisting check-dead-source-files-script.test.ts for its string fixtures), and re-litigating a file main just rewrote buys nothing. 2. src/mcp/server.ts's `loginRepoPullShape` is removed -- dead on arrival in #9565, and the first thing the newly-enabled noUnusedLocals caught on main. Which is the point of this PR: dead code now surfaces at the commit that introduces it rather than at the next audit. The engine bump lands at 3.16.1 (main released 3.16.0 while this was open). It is required by check-engine-parity: this PR removes two dead TYPE-only imports from packages/loopover-engine/src/advisory/gate-advisory.ts, and the parity contract holds that file in lockstep with its host twin src/rules/advisory.ts. There is no matching host edit to make -- the engine copy is a deliberately slimmed re-implementation (#4881) omitting the functions that use those types -- so the version bump is the sanctioned way to record a one-sided change. No behaviour change: type-only imports are erased at compile time. packages/loopover-miner/expected-engine.version moves with it, as its own check requires. * chore(release): sync .release-please-manifest.json to the 3.16.1 engine bump The manifest is a generated artifact that must move with any package.json version, and release-manifest:sync:check fails CI when it drifts. Regenerated with the repo's own `npm run release-manifest:sync` rather than hand-edited. * chore: prune the dead symbols the new flags caught in newly-merged code Rebase onto main after #9579 and #9580 landed. The newly-enabled noUnusedLocals immediately flagged twelve dead symbols in code merged since this PR opened -- including two I left in #9580 myself: - src/queue/processors.ts: PrCommandPrologueOutcome imported but unused (the adapter's annotated return type was dropped in favour of inference), plus eight unused bindings in the prologue destructures -- handlers that do not need `pr`, `settings`, `authorization` or `command` were still pulling them out. - src/queue/pr-command-prologue.ts: LoopOverMentionCommandName, superseded by LoopOverActionCommandName once the spec narrowed to action verbs. - src/mcp/dispatch-telemetry-sink.ts: an unused McpToolCallTelemetry import from #9579. Which is the point of the PR: the flags catch dead code at the commit that introduces it rather than at the next audit. Zero behaviour change -- every removal is a binding or an import TypeScript proved unreferenced, and the suite passes 24,014 tests. The rebase conflict itself was in processors.ts's import block: main added the pr-command-prologue import on the same lines this PR removed the unused runRetentionPrune one. Both intents kept.
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.