feat(mcp): migrate the stdio MCP server's 102 tools to @loopover/contract (#9537) - #9565
Merged
Conversation
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-28 11:25:10 UTC
Review summary Nits — 6 non-blocking
Decision 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9565 +/- ##
==========================================
+ Coverage 89.62% 89.67% +0.04%
==========================================
Files 868 869 +1
Lines 110873 110865 -8
Branches 26360 26364 +4
==========================================
+ Hits 99371 99418 +47
+ Misses 10237 10183 -54
+ Partials 1265 1264 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
…opover/engine Completes #9537's last requirement, and fixes the build failure the first push of PR #9565 hit. THE BUILD FIX: `ToolContract` was imported from @loopover/contract/tools, which only imports that type internally and never re-exports it. The local typecheck resolves the package through a src alias and passed; the real package build resolves the export map and failed at 'Build MCP'. Now imported from the package root, which does export it. THE PLAN-DAG DEDUP: buildPlanDag/validatePlanDag/the step state machine existed in src/services/plan-dag.ts AND hand-copied, untyped, into the stdio MCP bin -- whose own comment explained why: it resolves @loopover/engine through the published package, whose export map did not surface them. It does now. Moving it turned up a THIRD partial copy already in the engine: plan-export.ts holds the step/plan types, plan-step-readiness.ts holds nextReadySteps/isDone, and plan-overall-status.ts holds the status vocabulary, each decomposed into its own module. The moved file imports all three rather than restating them -- a fourth PlanDag would have defeated the point -- and contributes only the mutating half (build, validate, advance) that had no engine home. src/services/plan-dag.ts stays as the Worker's import path, now a re-export, so nothing else had to move. Release ordering, which is why this looked blocked: both publish workflows are workflow_dispatch-only, so nothing auto-publishes on merge and an operator publishes the engine before the CLI, as usual. The engine takes a minor bump for the new export, with packages/loopover-miner/expected-engine.version and both consumers' dependency ranges moved to ^3.16.0 in the same commit so a published CLI can never resolve an engine without it. Also declares @loopover/contract#build as an edge of the root typecheck task. That was working only because ci.yml happens to run a contract build first; the edge makes a bare 'turbo run typecheck' correct on its own.
4 tasks
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 22c8763 | Commit Preview URL Branch Preview URL |
Jul 28 2026, 10:25 AM |
JSONbored
force-pushed
the
feat/stdio-mcp-contract-9537
branch
from
July 28, 2026 10:03
0b63d3f to
a9a7f25
Compare
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
…opover/engine Completes #9537's last requirement, and fixes the build failure the first push of PR #9565 hit. THE BUILD FIX: `ToolContract` was imported from @loopover/contract/tools, which only imports that type internally and never re-exports it. The local typecheck resolves the package through a src alias and passed; the real package build resolves the export map and failed at 'Build MCP'. Now imported from the package root, which does export it. THE PLAN-DAG DEDUP: buildPlanDag/validatePlanDag/the step state machine existed in src/services/plan-dag.ts AND hand-copied, untyped, into the stdio MCP bin -- whose own comment explained why: it resolves @loopover/engine through the published package, whose export map did not surface them. It does now. Moving it turned up a THIRD partial copy already in the engine: plan-export.ts holds the step/plan types, plan-step-readiness.ts holds nextReadySteps/isDone, and plan-overall-status.ts holds the status vocabulary, each decomposed into its own module. The moved file imports all three rather than restating them -- a fourth PlanDag would have defeated the point -- and contributes only the mutating half (build, validate, advance) that had no engine home. src/services/plan-dag.ts stays as the Worker's import path, now a re-export, so nothing else had to move. Release ordering, which is why this looked blocked: both publish workflows are workflow_dispatch-only, so nothing auto-publishes on merge and an operator publishes the engine before the CLI, as usual. The engine takes a minor bump for the new export, with packages/loopover-miner/expected-engine.version and both consumers' dependency ranges moved to ^3.16.0 in the same commit so a published CLI can never resolve an engine without it. Also declares @loopover/contract#build as an edge of the root typecheck task. That was working only because ci.yml happens to run a contract build first; the edge makes a bare 'turbo run typecheck' correct on its own.
…onverge the hand-mirrored inputs Adds the 19 tools the remote migration could not cover, which is every tool the stdio server registers that the remote one either does not have or declares differently. All 102 stdio tools now have a contract entry. Thirteen of these existed on both servers with two hand-mirrored input schemas that had already drifted -- the stdio copies of find_opportunities, retrieve_issue_context and mark_notifications_read carried no length bounds at all. The contract takes the wider bound in every such case, and resolves the eligibility-transform question #9518 documented rather than solved: the contract describes what a caller may SEND, so the pre-transform shape is the one that belongs here and the remote server's downgrade stays a server-side control over the parsed value.
All 102 stdio tools now take their title, description, category, annotations, and BOTH schemas from the registry. ~85 hand-mirrored input shapes and the 560-line STDIO_TOOL_DESCRIPTORS table are gone (the bin file drops ~1,100 lines), every handler is typed via z.infer instead of `(input: any)`, and 97 of the 102 gain a real output schema where they had none. The registration helper passes the schema OBJECTS, not their .shape. The SDK accepts either, but a raw shape is re-wrapped in a plain z.object, which discards the catchall -- so every output modelled as a looseObject would be advertised and enforced as additionalProperties:false, and any field the payload carries beyond the modelled set becomes a -32602 the caller cannot act on. Turning the output schemas on surfaced six real defects, which is what they are for: - the agent audit feed models pullNumber/actor/detail as nullable, but the route OMITS them rather than sending null, so every real feed failed validation; - plan_repo_issues and generate_contributor_issue_drafts declared their six counters required, but the service short-circuits to a countless disabled/unavailable posture -- which is exactly why the CLI proxies carry '?? 0' fallbacks; - three test fixtures described payloads the services do not produce (laneFit as a lane name rather than a fit score, autoMaintain as a string rather than the settings object, a pending-action row missing four ledger columns). Fixtures fixed, not schemas. Four input divergences converged, each by widening so no live caller breaks: get_repo_onboarding_pack gains stdio's 'refresh'; preflight_local_diff and explain_score_breakdown gain stdio's local-diff fields; and get_pr_ai_review_findings -- the one tool whose two servers disagreed on a FIELD NAME -- accepts both 'number' (canonical, and what every other PR-scoped tool uses) and 'pullNumber' (the remote's alias). list_pending_actions is the single deliberate narrowing: its route hardcodes status 'pending' and cannot honour a filter, so the stdio server registers ListPendingActionsStdioInput -- derived from the contract entry with .omit() -- rather than advertising a filter that would silently do nothing. Also restores .strict() on callerBranchEligibilitySchema, dropped when it was relocated in #9518: both servers wrapped it strictly before the migration, and without it a caller inventing an eligibility field is silently stripped instead of rejected.
… and one real miss The checker exits 1 on main today. Two of the three reports are its own false positives and the third is mine. - check-dead-source-files-script.test.ts embeds import-statement TEXT as string fixtures for its script's injectable readFile, and a js-suffixed relative specifier is the exact case that test exists to pin -- so its fixtures read as violations. Allowlisted, the same way this checker already allowlists its own test file for the same reason. - scripts/actionlint.ts runs under 'node --experimental-strip-types', whose loader does no extension resolution: the literal .ts is the only form that starts. That is a fact about how package.json invokes it, not something readable from the source, so the entrypoint is listed explicitly. - The three relative imports #9517/#9518 added to contract-registry.test.ts carried .js in a Bundler-resolved zone. Extension dropped.
…opover/engine Completes #9537's last requirement, and fixes the build failure the first push of PR #9565 hit. THE BUILD FIX: `ToolContract` was imported from @loopover/contract/tools, which only imports that type internally and never re-exports it. The local typecheck resolves the package through a src alias and passed; the real package build resolves the export map and failed at 'Build MCP'. Now imported from the package root, which does export it. THE PLAN-DAG DEDUP: buildPlanDag/validatePlanDag/the step state machine existed in src/services/plan-dag.ts AND hand-copied, untyped, into the stdio MCP bin -- whose own comment explained why: it resolves @loopover/engine through the published package, whose export map did not surface them. It does now. Moving it turned up a THIRD partial copy already in the engine: plan-export.ts holds the step/plan types, plan-step-readiness.ts holds nextReadySteps/isDone, and plan-overall-status.ts holds the status vocabulary, each decomposed into its own module. The moved file imports all three rather than restating them -- a fourth PlanDag would have defeated the point -- and contributes only the mutating half (build, validate, advance) that had no engine home. src/services/plan-dag.ts stays as the Worker's import path, now a re-export, so nothing else had to move. Release ordering, which is why this looked blocked: both publish workflows are workflow_dispatch-only, so nothing auto-publishes on merge and an operator publishes the engine before the CLI, as usual. The engine takes a minor bump for the new export, with packages/loopover-miner/expected-engine.version and both consumers' dependency ranges moved to ^3.16.0 in the same commit so a published CLI can never resolve an engine without it. Also declares @loopover/contract#build as an edge of the root typecheck task. That was working only because ci.yml happens to run a contract build first; the edge makes a bare 'turbo run typecheck' correct on its own.
…, and make the root typecheck catch them The stdio migration left ~40 imports and shape constants unreferenced once every tool started resolving its schemas through getToolContract. @loopover/mcp's own tsconfig sets noUnusedLocals, so its package build failed on all of them -- twice -- while 'npm run typecheck' and the full unit suite stayed green locally. That gap is the real bug, and it is fixed here rather than worked around: the root 'typecheck' script now runs the root tsconfig AND each package whose config is stricter than it (@loopover/contract, @loopover/mcp, @loopover/miner). A bare 'npm run typecheck' now covers the same surface CI's build steps do, so this class of failure cannot pass locally and fail in CI again. Also defaults preflight_local_diff's baseRef at the call site. It is optional in the shared contract input -- the remote server has no checkout to read -- so the local diff collector needs the 'HEAD' default the stdio-only shape used to bake into the schema.
JSONbored
force-pushed
the
feat/stdio-mcp-contract-9537
branch
from
July 28, 2026 10:23
66d2a3d to
22c8763
Compare
….0 bump The engine version bump that gives the stdio server a published plan-DAG export has to be reflected in .release-please-manifest.json, which release-please reads to decide what it is releasing. Caught by release-manifest:sync:check.
Bundle ReportBundle size has no change ✅ |
…wo caller-error paths codecov/patch flagged the new branches in getPrAiReviewFindings: every existing test supplies pullNumber, so the canonical 'number' side of the alias and both 'required field missing' throws were unexercised.
JSONbored
force-pushed
the
feat/stdio-mcp-contract-9537
branch
from
July 28, 2026 10:42
519b383 to
0b6e95b
Compare
They were swept in by a git add -A while both branches were in flight; they belong to the contract-validator issue, not the stdio migration.
…ntation codecov/patch flagged packages/loopover-engine/src/plan-dag.ts at 40%: the engine uploads its own coverage from its own node:test suite, so the root suite's test/unit/plan-dag.test.ts -- which exercises the same code thoroughly through the Worker's re-export -- contributes nothing to it. The tests move with the implementation. 100% statements and branches on the moved module, verified with c8 against the engine's own dist.
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
…d 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.
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 #9537.
What changed
All 102 stdio tools now register from
@loopover/contract. Title, description, category, annotations and both schemas come from the registry;packages/loopover-mcp/bin/loopover-mcp.tsno longer states any of them. That deletes ~85 hand-mirrored input shapes and the 560-lineSTDIO_TOOL_DESCRIPTORStable — the bin file drops 1,100 lines — and every handler is now typed viaz.inferinstead of(input: any). Combined with #9542 and #9559, all three MCP servers are on one contract; the registry holds 125 entries.97 of the 102 gained a real output schema where they had none. That is the substantive change, and it is what found most of what follows.
One implementation detail worth calling out: the registration helper passes the schema objects, not their
.shape. The SDK accepts either, but a raw shape gets re-wrapped in a plainz.object, which discards the catchall — so every output modelled as alooseObjectwould be advertised and enforced asadditionalProperties: false, and any field the payload carries beyond the modelled set becomes a-32602the caller can do nothing about.What turning the schemas on found
pullNumber/actor/detailwere modellednullable, but the route omits them for an event that has none rather than sendingnull. Every real feed would have failed. Nownullish.plan_repo_issuesandgenerate_contributor_issue_draftsdeclared six counters required, but the service short-circuits to a countlessdisabled/unavailableposture when AI is off — which is exactly why the CLI proxies carry?? 0fallbacks. The schemas described a response the service does not always send.laneFitas a lane name rather than a fit score,autoMaintainas a string rather than the settings object, and a pending-action row missing four ledger columns. Fixtures fixed, not schemas.callerBranchEligibilitySchemalost its.strict()when contract(remote): migrate the remote MCP server's tools to @loopover/contract — typed handlers, real output schemas #9518 relocated it. Both servers wrapped it strictly before the migration; without it a caller inventing an eligibility field is silently stripped instead of rejected. Restored — same class as the.strict()regressions feat(contract): migrate every remote MCP tool contract to @loopover/contract (#9518) #9559 fixed, and the reason that PR added regression tests.Divergences resolved
Thirteen tools existed on both servers with two hand-mirrored inputs that had already drifted (the stdio copies of
find_opportunities,retrieve_issue_contextandmark_notifications_readcarried no length bounds at all). Every convergence widens, so no live caller breaks:get_repo_onboarding_packgains stdio'srefresh;preflight_local_diffandexplain_score_breakdowngain stdio's local-diff fields.get_pr_ai_review_findings— the one tool whose two servers disagreed on a field name — accepts bothnumber(canonical, and what every other PR-scoped tool uses) andpullNumber(the remote's). Narrowing to either would break live callers of the other: published CLI on one side, hosted API on the other.loginbecomes optional wherever both servers already resolved it themselves (stdio from its session, remote from its authenticated identity).local_statusis a third divergence the issue did not name — not a payload that drifted but two different tools that collided on one name. The remote answers "what does this endpoint support"; the stdio server answers "what is the state of this CLI on this machine". Neither can answer the other's question, so there is nothing to converge. The union keeps both wires working and validated; the real fix is a rename, which breaks every caller of whichever side loses the name, so it is documented in the schema rather than done in flight.list_pending_actionsis the single deliberate narrowing. Its route hardcodesstatus: "pending"and cannot honour a filter, so the stdio server registersListPendingActionsStdioInput— derived from the contract entry with.omit()— rather than advertising a filter that would silently do nothing.Wire-visible: descriptions converged
76 stdio descriptions now match the remote's, because one tool name gets one description. Where the contract had no prior wording (the 19 local-git tools), the stdio original is used verbatim. Three stdio tests asserted the phrase "no API round-trip", which cannot survive convergence — on the remote server a
tools/callis an API round-trip — so they now assert the purity guarantee that is true on both ("Pure computation … no repo data, no writes").Also fixed
A build failure the first push hit, worth recording because the local signal was wrong.
ToolContractwas imported from@loopover/contract/tools, which only imports that type internally and never re-exports it.tsc --noEmitat the repo root resolves the package through a src alias and passed; the real package build resolves the export map and failed at "Build MCP". Now imported from the package root. Relatedly,@loopover/contract#buildis now a declared edge of the root typecheck task — it was working only becauseci.ymlhappens to run a contract build first.import-specifiers:checkwas red on main.It exits 1 on
origin/maintoday. Two reports are its own false positives — a test whose fixtures are import statements, andscripts/actionlint.ts, which runs undernode --experimental-strip-typeswhere the literal.tsis the only form that starts — and the third is mine, from #9517/#9518. All three fixed.The plan-DAG duplication (requirement 6)
buildPlanDag/validatePlanDag/ the step state machine lived insrc/services/plan-dag.tsand hand-copied, untyped, into the stdio bin — whose own comment explained why: it resolves@loopover/enginethrough the published package, whose export map did not surface them. It does now.Moving it turned up a third partial copy already in the engine:
plan-export.tsholds the step/plan types,plan-step-readiness.tsholdsnextReadySteps/isDone,plan-overall-status.tsholds the status vocabulary. The moved module imports all three rather than restating them — a fourthPlanDagwould defeat the point — and contributes only the mutating half (build, validate, advance) that had no engine home.src/services/plan-dag.tsstays as the Worker's import path, now a re-export.On release ordering, which is what made this look blocked: both publish workflows are
workflow_dispatch-only, so nothing auto-publishes on merge and an operator publishes the engine before the CLI as usual. The engine takes a minor bump for the new export, withpackages/loopover-miner/expected-engine.versionand both consumers' ranges moved to^3.16.0in the same commit, so a published CLI can never resolve an engine without it.Validation
npx vitest run test/unit test/contract— 23,396 passed, 6 skipped, 0 failed (all 78mcp-cli-*files green).tsc --noEmitclean;build:mcp,test:mcp-packgreen.ui:openapi:check,docs:drift-check,manifest:drift-check,engine-parity:drift-check,command-reference:check,validate:no-hand-written-js,dead-source-files:check,import-specifiers:checkall green.openapi.jsonregenerates byte-identical.test/unit/mcp-cli-tools.test.ts: every registered tool is in the registry with a matching description and an object-typed input and output schema; and a source-level assertion that every handler is annotatedz.infer<typeof …>with noanyleft —tscenforces that the annotation matches the schema, but cannot enforce that one exists.