types(mcp): type the stdio CLI end to end, from options to request bodies (#9773) - #9785
Merged
Conversation
Contributor
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-29 11:24:37 UTC
Review summary Nits — 6 non-blocking
CI checks failing
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
Visual preview
Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy. Scroll preview
A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show. 🟩 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. |
JSONbored
force-pushed
the
types/stdio-server-no-any-9773
branch
2 times, most recently
from
July 29, 2026 09:22
4e8800a to
08eb323
Compare
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 239b705 | Commit Preview URL Branch Preview URL |
Jul 29 2026, 10:06 AM |
…s it hid (#9773) `parseOptions` is now typed from CLI_FLAG_SPEC -- repeatable flags as arrays, boolean flags as booleans, anything else as `string | boolean` behind an index signature, because the parser genuinely accepts any `--flag` and a closed record would be a lie. That type flows into every `options` parameter, every argv parameter becomes `readonly string[]`, and the config parameters take the contract's LoopoverConfig. Three defects fell out immediately, each reproduced against main before the fix: TypeError: (options[key] ?? []) is not iterable repoFullName.includes is not a function LoopOver API 404: {"error":"not_found"} The first is `--issue --issue 5`: a bare repeatable flag is stored as `true` by the no-value branch, and the accumulator then spread it. Anything not already a list now starts a fresh one -- the only sane reading of a flag that carried no value to keep. The second is `maintain <sub> --repo` with no value. `true` passed the `!repoFullName` truthiness guard and then died on a string method, where "Pass --repo owner/repo." was intended. The third is a bare `--login`, read as the literal string "true", so `decision-pack --login` requested a contributor NAMED "true" and reported them not found instead of saying the value was missing. Options are read through optionText() now, which treats a valueless flag as absent -- and every one of those call sites already had an env or profile fallback for absent. Also: the contract's LoopoverConfig was missing `session`, `telemetryEnabled`, and profile `createdAt`, all three read and written by the CLI with nothing checking they existed. The legacy top-level `session` is still written on the default profile so an older CLI reading the same file keeps working, which is exactly why it cannot be left undeclared. 277 -> 184 `: any` occurrences in the bin. The remainder is a long tail of callbacks over API payloads that stay untyped for a structural reason worth its own issue: CLI_RESPONSE_SCHEMAS covers only the 24 STATIC paths, so all 53 parameterised calls fall through to the untyped overload. #9773 stays open for that.
… document (#9773) The second tranche. #9521 built the typed accessors; its scanner rejected any template containing an interpolation, so every per-repo and per-contributor call -- the majority of the CLI -- missed the typed overload and read its payload as `any`. The document already described most of them. Three things had to change for a composed call to resolve. The path builders now DECLARE their shape. `toolRepoBase` returned `string`, which erases the path at the type level, so `apiGet(`${toolRepoBase(o, r)}/settings`)` could never match anything; it and the 24 locally-built bases now carry template-literal types. That is also what lets the generator's scanner resolve them: it reads the same declarations the type checker does, so the two cannot disagree about what a base is. The tables are keyed by METHOD, not by path. `/v1/repos/{owner}/{repo}/agent/pending-actions` lists on GET and proposes on POST, and those return different shapes -- a path-keyed table had to guess, and the first version of this guessed `post`, handing the GET call site the POST response type. The CLI's own `payload.pendingActions` read is what contradicted it, the moment a schema was attached at all. Caught by the type checker before it shipped. And the generated copy now carries what a copied schema REFERENCES. `closure` followed only `*Schema` names, so a schema depending on a plain value beside it (`AGENT_ACTION_CLASS_VALUES`) emitted a file that would not compile. Values declared in the source are copied; anything else is imported from the contract's limits.ts, where it is restated and pinned -- and a bound missing there fails the contract build rather than emitting something broken. 30 parameterised calls are typed now, up from 8, and the guards are in mcp-api-client.test.ts: method disambiguation, base-path resolution, the copied-value closure, and the prose false-positive the first cut of the constant scanner hit (it emitted imports for DELETE, REQUIRED and REST, read out of doc comments). Still `any` at the fallback overload, for the endpoints whose 200 the document does not describe with a named schema. Flipping that to `unknown` leaves 72 narrowing sites, and the honest fix for them is to describe those endpoints -- #9773 stays open for it.
…emas that lied (#9773) A review found a contributor login being sent to the API as boolean `true` -- a bare `--login` that my sweep had missed. The first answer was a test that grepped the source for the shape; that is a guard against one spelling, not against the defect, so it is gone. The defect is now a compile error. `apiPost`'s body is typed from the request schemas the published document names, so an option value -- `string | boolean | string[]`, because a bare flag is `true` -- cannot reach a field the API declares as a string. Verified by reverting one fix and watching tsc say `Type 'boolean' is not assignable to type 'string'`. Making the types BINDING mattered as much as adding them. The fallback overloads accepted `path: string, body: unknown`, so a call that failed a typed overload did not error -- it fell through and was accepted unchecked. They now refuse any path the typed overloads cover. That found three more instances of the reviewer's class, each in a different command: `lint-pr-text` sent `--body` as `true`, `check-slop-risk` sent `--description` as `true`, and `validate-focus-manifest` sent `--source` as an unchecked free string where the API takes three literals -- its `.includes()` guard never narrowed, so the body kept the raw value. The last is now parsed against the contract's own enum, so the accepted values and the error naming them come from the schema the route validates with. And four published request schemas were wrong. ValidateLinkedIssueRequest required `owner` and `repo` in the BODY though both are path params; CheckSlopRiskRequest required `changedFiles` the handler has optional; ValidateFocusManifestRequest typed an enum as a free string. They were hand-written parallels of the schemas the handlers actually parse with, and they had drifted -- so they are now built from those schemas. Rebuilt via `z.object(shape)` rather than used directly, because `.openapi()` exists only after `extendZodWithOpenApi` and the contract must never run it. The generator carries what a copied schema references, resolved against what each module really exports: bounds from limits.ts, request schemas from api-requests.ts. A name in neither fails the contract build instead of emitting a dangling reference.
…ract's modules (#9773) Two things the merge with #9762 exposed, now that the action-class and autonomy-level lists are readonly literal tuples rather than `string[]`: - The CLI validated `<action>` and `<level>` with `LIST.includes(value)` and then passed the still- `string` value to a typed request. `includes` returns a boolean and narrows nothing, so the check ran and the type system learned nothing from it. `isOneOf` is the same check written as a type predicate, so a validated value arrives at the API as the union it was just proved to be. - The generator resolved a copied schema's constants against a hardcoded pair of contract modules. That is a hand-maintained list by another name, and it fails in the quietest way available: a constant that moves between modules yields a generated file referencing a name it never imported. It now reads the contract's source directory, so a constant can move -- or a module can appear -- without this script knowing anything about it. Regression test pins the discovery against wherever PUBLIC_SURFACE_SKIP_REASONS lives, rather than against the module it happens to live in today.
JSONbored
force-pushed
the
types/stdio-server-no-any-9773
branch
from
July 29, 2026 10:04
5f89c79 to
239b705
Compare
JSONbored
changed the title
Bundle ReportChanges will increase total bundle size by 503 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9785 +/- ##
=======================================
Coverage 90.33% 90.34%
=======================================
Files 918 918
Lines 113936 114012 +76
Branches 26975 26979 +4
=======================================
+ Hits 102926 103006 +80
+ Misses 9681 9680 -1
+ Partials 1329 1326 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 #9773.
The stdio CLI read every API response as
anyand every option asstring | boolean | string[] | undefined, and picked fields out by optional-chaining guesswork. That is not a style problem — it hid three reproducible crashes and four published request schemas that disagreed with the handlers they described.1. The option plumbing, and the three crashes it hid
preflight --issue --issue 5true, and the accumulator spread it. Anything not already a list now starts a fresh one — the only sane reading of a flag carrying no value.maintain list --repotruepassed the!repoFullNametruthiness guard, then died on a string method, wherePass --repo owner/repo.was intended.decision-pack --login"true", so it requested a contributor named "true" and reported them not found instead of saying the value was missing.Options are read through
optionText()now, which treats a valueless flag as absent — and every one of those call sites already had an env or profile fallback for absent.CliOptionsis derived fromCLI_FLAG_SPECrather than restated, so a new flag is typed by declaring it.2. Parameterised paths: the reason the rest stayed
anyCLI_RESPONSE_SCHEMAScovered only the 24 static paths. Every per-repo and per-contributor call —/v1/repos/{owner}/{repo}/…,/v1/contributors/{login}/…— missed the typed overload and fell through to the untyped one, while the published document described most of them precisely.The generator now resolves a composed path (
${repoBase}/settings) through the base's declared template-literal type, and emitsCLI_PARAMETERISED_RESPONSE_SCHEMASkeyed by"METHOD path"— method-keyed because/agent/pending-actionslists on GET and proposes on POST, and a path-keyed table had to guess between them.anyoccurrences in the bin: 245 → 185, and the remainder is now only endpoints the document does not describe a 200 for.3. The request bodies, and four schemas that lied
apiPost's body is typed asApiRequestBody<"POST", Path>, so sending a field the route does not accept is a compile error rather than a 400 at runtime. Making that compile surfaced thatValidateLinkedIssueRequest,CheckBeforeStartRequest,CheckSlopRiskRequestandValidateFocusManifestRequestwere hand-written approximations of their handlers' real schemas — they are now built from those schemas directly, andopenapi.jsonis regenerated accordingly.The fallback overload had to be taught to refuse paths the typed overloads cover; without that it silently absorbed every call that failed the typed ones, and the new types did nothing.
4. Closed-set guards, and one more hand-maintained list
LIST.includes(value)returns a boolean and narrows nothing, so a validated<action>/<level>still reached the API asstring.isOneOfis the same check as a type predicate.The generator resolved a copied schema's constants against a hardcoded pair of contract modules — a hand-maintained list by another name, which fails silently when a constant moves between modules. It reads the contract's source directory now.
Contract gaps closed
LoopoverConfigwas missingsession,telemetryEnabled, and profilecreatedAt— all read and written by the CLI with nothing checking they existed. The legacy top-levelsessionis still written on the default profile so an older CLI reading the same file keeps working, which is precisely why it cannot be left undeclared.Validation
npx vitest run --changed=origin/main: 3729 passed, 275 files.tsc --noEmitclean at package and root level.contract:api-schemas:checkandui:openapi:checkboth clean.test/unit/mcp-cli-bool-flag-parsing.test.tsand verified failing againstmainby checking out the pre-fix sources and re-running.test/unit/mcp-api-client.test.ts: method disambiguation, base-path resolution, coverage, constant discovery, and the prose false-positive that an earlier cut produced.