feat(mcp): add validate:mcp, the contract validator that enforces all three servers (#9520) - #9575
Merged
Conversation
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-28 17:50:44 UTC
Review summary Blockers
Nits — 5 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.
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | ada4fc0 | Commit Preview URL Branch Preview URL |
Jul 28 2026, 11:54 AM |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9575 +/- ##
==========================================
+ Coverage 89.67% 89.82% +0.15%
==========================================
Files 869 869
Lines 110868 110868
Branches 26366 26366
==========================================
+ Hits 99421 99589 +168
+ Misses 10183 9998 -185
- Partials 1264 1281 +17
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/validate-mcp-9520
branch
from
July 28, 2026 11:05
fefd3c1 to
613bb60
Compare
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
…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.
… 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.
JSONbored
force-pushed
the
feat/validate-mcp-9520
branch
from
July 28, 2026 11:51
58e35c1 to
ada4fc0
Compare
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
…ch chokepoints (#9525) (#9579) * feat(mcp): add validate:mcp, the contract validator that enforces all 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. * fix(mcp): pass the schema object at the ten register sites the first 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. * feat(mcp): one registry-driven telemetry contract at all three dispatch 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. * fix(mcp): never send tool arguments or results in telemetry, for any 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. * test(mcp): cover the telemetry sink, the span registry, and the miner 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. * test(mcp): close the remaining telemetry coverage gaps in the three package 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. * test(mcp): cover the payload path JSON.stringify declines to represent 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 #9520. Stacked on #9565 (#9537); the diff below it is that PR's.
It found a live production bug on the first run
That is the point of the issue, so leading with it.
The remote server passed each contract schema's
.shapetoregisterTool. The SDK re-wraps a raw shape in a plainz.object, which discards the catchall — so every output modelled as alooseObjectwas advertised and enforced asadditionalProperties: false, and the SDK rejected the handler's own payload with a-32602the caller could do nothing about.20+ tools were returning errors instead of answers to real callers, including
get_pr_maintainer_packet,get_repo_settings,get_upstream_drift, and the whole current-branch family (preflight_current_branch,preview_current_branch_score,rank_local_next_actions,prepare_pr_packet). Passing the schema objects fixes it. The stdio server already did this — #9537 got it right for the same reason, which is why only the remote server was affected.Before:
remote: 107 tools — 20+ rejected their own payloads. After:remote: 107 tools — 99 validated against their output schema, 8 declined in this env.The design, and why it differs from the reference implementation
Smoke-call arguments are synthesized from each tool's own advertised
inputSchema, not read from a hand-maintained table.metagraphed's validator keeps a name-keyed table, and its documented gap is the direct consequence: only 113 of its 205 tools are ever actually called, with nothing forcing a new tool to add an entry. A table maintained by hand for every tool is a table that rots. Here a new tool is smoke-called the day it is registered, with no table edit — so requirement 4's "fail if any registered tool has no entry" is satisfied by construction, and the explicit check that remains catches a tool the driver deliberately skipped.
overrides.tsholds seven entries, each a place where a structurally valid value is not a semantically useful one — mostlydryRun: trueflags that the synthesizer would otherwise send asfalse, flipping a create-safe tool into its write path.What it enforces, per surface
tools/listagrees with the registry; every tool advertises a description and object-typed input and output schemas; everyoutputSchemaAjv-compiles up front (so an uncompilable schema fails once, loudly, rather than at whichever call happens to hit it); every successful call'sstructuredContentvalidates against that compiled schema; no tool was skipped; and the negative paths (unknown tool, malformed input) 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. A lock that only compares constants to each other stays green while the thing meant to update them has stopped running — the constants agree precisely because nothing is touching them.
It reports its own coverage honestly
A tool that declines — not configured, no repo access, elicitation withheld — has answered correctly for a cold fixture env, but it did not exercise its output schema. Collapsing that into a green checkmark would be the same self-congratulatory reporting this validator exists to replace, so the split is printed every run. The stdio number is low because that server proxies a hosted API it has no token for; the remote number is high because it runs against a seeded D1, which is exactly where output validation earns its keep.
Two implementation notes
It runs as a vitest spec, not a bare script. The remote server imports
cloudflare:modules the plain Node loader cannot resolve; vitest's Workers-aware resolution can. Running there also means it reuses the same seeded-D1 helper the rest of the suite uses instead of standing up a second, divergent fixture environment.npm run validate:mcpruns exactly this file andtest:ciruns it as its own step.Ajv needed the dialect stripped. The contract emits draft-2020-12, but the SDK re-serializes it with a draft-07
$schema, so anAjv2020instance refuses all 220 of them. Dropping the dialect declaration is right rather than merely convenient: 2020-12 is what the contract authored, and none of these schemas use a construct whose meaning differs between the drafts.Validation
npm run validate:mcp— 5 checks, 3 servers, green.npx vitest run test/unit test/contract— 23,486 passed, 6 skipped, 0 failed.npm run typecheckclean;import-specifiers:check,dead-source-files:check,validate:no-hand-written-js,coverage-boltons:checkgreen;openapi.jsonunchanged.test/unit/validate-mcp-helpers.test.ts), including the branches a green run never reaches: a tool that goes missing, a non-object schema, a drifted version, a deleted release path.