mcp: advertise the registry's projection on all three servers (#9655) - #9812
Conversation
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-29 12:34:57 UTC
Review summary Nits — 6 non-blocking
CI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed 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.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
a1779ba to
5577551
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9812 +/- ##
===========================================
+ Coverage 79.03% 91.59% +12.56%
===========================================
Files 280 915 +635
Lines 58366 112692 +54326
Branches 6697 27078 +20381
===========================================
+ Hits 46128 103220 +57092
+ Misses 11955 8144 -3811
- Partials 283 1328 +1045
Flags with carried forward coverage won't be shown. Click here to find out more.
|
5577551 to
b365e8c
Compare
One contract entry produced three differently-advertised tools, and nothing could see it: `diffToolSets` compares name sets, and `checkAdvertisedShape` only asks whether a description is non-empty. - The REMOTE server wrote a description literal at every `register(...)` call site -- 35 had drifted from the contract's, which is the text `listToolDefinitions()` serves to the agent-spec builders and the `.well-known` catalogs, so one tool was described two different ways depending on which LoopOver surface you asked. It advertised no tool `title` and no `annotations` at all, so a client that gates confirmation on `destructiveHint` saw nothing for the server that performs the writes. The `register` wrapper now fills all three from `getToolDefinition(name)`, spread after the call site's config so a stray literal cannot win, and throws on a name with no contract entry. - The STDIO server passed `contract.annotations` raw. That field is a `Partial` stating only what differs from the default posture, so the five tools declaring one field advertised it without a `destructiveHint`, and the ~95 declaring none advertised no annotations -- both disagreeing with what the registry publishes for the same tool. - The MINER server passed neither, and passed every schema as `SomeSchema.shape`. The SDK re-wraps a raw shape in a plain `z.object` that DISCARDS the catchall, and every miner output is a `looseObject`: all 21 were advertised and enforced as `additionalProperties: false`, so any field a payload carried beyond the modelled set came back as a -32602 the caller could do nothing about. #9762 fixed this class elsewhere and added a guard, but its roots omitted `packages/loopover-miner/bin` -- the guard reported none while the server it did not look at had 21. A `registerMinerTool` helper replaces all of them, and the guard now proves its own roots cover every file that registers a tool. Also closes #9656 and #9657, which are the same registration path: the remote's descriptions come from the contract now (its `MCP_TOOL_CATEGORIES` was already derived), and `loopover_admin_rotate_secret` -- the one tool still registering from schemas declared in `src/mcp/server.ts` -- reads its four siblings' contract entry instead. The reason it survived is fixed too: every case booted a server without `LOOPOVER_MCP_ADMIN_ENABLED`, so the admin category was never diffed, compiled, smoke-called or output-validated, and the validator's own "nothing is registered without a contract entry" assertion was structurally unable to see the tool that violated it. A second remote surface with the flag set now runs the full pass over all five. `ToolContract` becomes generic in its input/output schemas so a server registering from an entry gets that tool's real argument type in its handler rather than the erased `z.ZodObject`. New invariant `checkAdvertisedMetadata` compares advertised title/description/posture against the projection, and runs for all three surfaces. No tool's declared posture changed.
b365e8c to
be0e20e
Compare
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | be0e20e | Commit Preview URL Branch Preview URL |
Jul 29 2026, 12:20 PM |
Closes #9655. Also closes #9656 and #9657 — all three are the same registration path, and fixing one without the others would leave the check that proves it green for the wrong reason.
The defect
@loopover/contractgives every tool atitle, adescriptionand anannotationsposture, andprojectToolDefinitionsapplies{ readOnlyHint: true, destructiveHint: false }so the projection every consumer reads carries a complete, defaulted pair. None of the three servers advertised that projection, and nothing could see it:diffToolSetscompares name sets, andcheckAdvertisedShapeonly asks whether a description is non-empty.src/mcp/server.ts)descriptionliteral per call site — 35 had drifted from the contract's. No tooltitle, noannotationsanywhere in the file.contract.annotationsraw — aPartialstating only what differs from the default. Five tools shippedreadOnlyHintwith nodestructiveHint; ~95 shipped no annotations at all.The drifted descriptions matter because the two texts reach different audiences: the remote's
tools/listserved the inline literal, whilelistToolDefinitions()— which feeds the OpenAI/Anthropic spec builders and the.well-knowncatalogs — served the contract's. The posture matters because an MCP client that gates a confirmation prompt ondestructiveHintgot nothing from the server that performs the writes.The miner's 21 -32602s
Every miner registration passed
SomeSchema.shape. The SDK re-wraps a raw shape in a plainz.objectthat discards the catchall — and every miner output is alooseObject:So all 21 were advertised and enforced as
additionalProperties: false, and any field a payload carried beyond the modelled set came back as a-32602the caller could do nothing about. This is exactly the class #9762 fixed on the other two servers — but its guard'sSERVER_ROOTSomittedpackages/loopover-miner/bin, so it reported zero offenders while the server it never looked at had 21. A guard that can quietly watch nothing is worse than no guard, so it now proves its own roots cover every file that registers a tool (verified failing when a root is removed).What changed
registerwrapper resolvesgetToolDefinition(name)and throws on a name with no entry;registerStdioTooluses the projected annotations;registerMinerToolreplaces 21 hand-written config objects.projectToolDefinition), so a server that already resolved a contract applies the defaults without a second lookup that can fail.ToolContractis generic in its input/output schemas, so a server registering from an entry gets that tool's real argument type in its handler rather than the erasedz.ZodObject.checkAdvertisedMetadata(scripts/lib/validate-mcp/invariants.ts) compares advertised title/description/posture against the projection, and runs for all three surfaces invalidateSurface.#9656 — the remote's descriptions and categories
166 advertised literals deleted from the call sites.
MCP_TOOL_CATEGORIESwas already derived fromTOOL_CONTRACTSandMCP_TOOL_CATEGORY_IDSalready aliases the contract'sTOOL_CATEGORIES, so the category half needed nothing further;_meta.categorynow reads the typed projection instead of an index-signature lookup that yieldedundefinedfor an unlisted name with no compile error.#9657 — the fifth admin tool
loopover_admin_rotate_secretwas the one tool still registering from schemas declared insrc/mcp/server.tswhile its four siblings read the contract. Those local declarations are deleted and the registration readsAdminRotateSecretInput/AdminRotateSecretOutput.The reason it survived is fixed too: the admin tools register only under
LOOPOVER_MCP_ADMIN_ENABLED, which novalidate-mcpcase set — so the admin category was never diffed, compiled, smoke-called or output-validated, and the validator's own "nothing is registered without a contract entry" assertion was structurally unable to see the one tool that violated it. A second remote surface with the flag set now runs the full pass: 139 tools, 99 output-validated, up from 134.No posture changed
Requirement 4 of #9655, held exactly. Notably
loopover_delete_branchis not advertised destructive: the remote tool builds a local-execution spec and touches nothing, and the hints describe the tool call rather than the command a caller may later run with their own credentials. The test says so, so a future reader does not "fix" it.Validation
npx vitest run --changed=origin/main: 2528 passed, 171 files.test/contract/: 108 passed.tsc --noEmitclean.checkAdvertisedMetadataunit-tested for: agreement, a rewritten title, a rewritten description, each hint disagreeing, a tool advertising no annotations at all, and staying quiet about a tool the server never registered (that isdiffToolSets' finding).additionalPropertiesfor the miner's loose outputs.docs:drift-check,manifest:drift-check,command-reference:check,ui:openapi:check,contract:api-schemas:checkall clean.