Skip to content

Commit 4e8800a

Browse files
committed
types(mcp): type the stdio CLI's option plumbing, fixing three crashes 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.
1 parent 0fa52ed commit 4e8800a

8 files changed

Lines changed: 829 additions & 146 deletions

File tree

packages/loopover-contract/src/api-schemas.ts

Lines changed: 472 additions & 0 deletions
Large diffs are not rendered by default.

packages/loopover-contract/src/cli-config.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,13 +40,27 @@ export const PROFILE_NAME_PATTERN = /^[a-z0-9][a-z0-9._-]{0,63}$/;
4040

4141
export type LoopoverConfigProfile = {
4242
apiUrl?: unknown;
43-
session?: { token?: unknown } | null | undefined;
43+
/** #9773: stamped by `loopover-mcp login` and preserved across re-logins. It was absent from this type
44+
* while the CLI read and wrote it, so nothing checked the field even existed. */
45+
createdAt?: unknown;
46+
session?: LoopoverConfigSession | null | undefined;
4447
};
4548

49+
export type LoopoverConfigSession = { token?: unknown; createdAt?: unknown; login?: unknown };
50+
4651
export type LoopoverConfig = {
4752
activeProfile?: unknown;
4853
profiles?: Record<string, LoopoverConfigProfile | undefined>;
4954
apiUrl?: unknown;
55+
/**
56+
* #9773: the pre-profile session and the telemetry opt-in, both still read and written by the CLI.
57+
*
58+
* `session` is the LEGACY single-session shape from before profiles existed -- `loopover-mcp login` on
59+
* the default profile still writes it so an older CLI reading the same file keeps working, which is
60+
* exactly why it cannot be dropped from the type.
61+
*/
62+
session?: LoopoverConfigSession | null | undefined;
63+
telemetryEnabled?: unknown;
5064
};
5165

5266
/**

packages/loopover-mcp/bin/loopover-mcp.ts

Lines changed: 212 additions & 130 deletions
Large diffs are not rendered by default.

scripts/gen-contract-api-schemas.ts

Lines changed: 90 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,52 @@ export function cliApiPaths(binSource: string): string[] {
4343
return [...paths].sort();
4444
}
4545

46+
/**
47+
* Every PARAMETERISED `/v1/...` path the CLI calls, normalised to the document's own `{param}` form (#9773).
48+
*
49+
* `cliApiPaths` above deliberately rejects anything containing a `$`, so until now the 53 template call
50+
* sites -- every per-contributor and per-repo endpoint -- fell through to the untyped overload. That, not
51+
* an oversight in the call sites, is why the stdio bin still reads those payloads as `any`.
52+
*
53+
* Each `${...}` becomes `{}` first (its own contents can be an arbitrary expression, including nested
54+
* braces and a `?:` with slashes in both arms), then the segments are re-keyed positionally against the
55+
* document's parameter names, so the emitted key is exactly the string `openapi.json` uses.
56+
*/
57+
export function cliParameterisedApiPaths(binSource: string, document: OpenApiDocument): string[] {
58+
const documented = Object.keys(document.paths).filter((path) => path.includes("{"));
59+
const shapes = new Map<string, string>();
60+
for (const documentPath of documented) shapes.set(documentPath.replace(/\{[^}]+\}/g, "{}"), documentPath);
61+
62+
const found = new Set<string>();
63+
for (const match of binSource.matchAll(/api(?:Get|Post|Delete|Fetch)\(\s*`(\/v1\/[^`]*)`/g)) {
64+
const raw = match[1]!;
65+
if (!raw.includes("${")) continue;
66+
// Collapse each interpolation, honouring nested braces, then drop any trailing query the template adds.
67+
let collapsed = "";
68+
for (let index = 0; index < raw.length; index += 1) {
69+
if (raw[index] === "$" && raw[index + 1] === "{") {
70+
let depth = 1;
71+
index += 2;
72+
while (index < raw.length && depth > 0) {
73+
if (raw[index] === "{") depth += 1;
74+
else if (raw[index] === "}") depth -= 1;
75+
index += 1;
76+
}
77+
index -= 1;
78+
collapsed += "{}";
79+
} else {
80+
collapsed += raw[index];
81+
}
82+
}
83+
const withoutQuery = collapsed.split("?")[0]!.replace(/\/+$/, "");
84+
// A template whose interpolation spans a slash (a conditional query suffix, say) cannot be a path
85+
// shape; it simply will not match a documented one, and is left unvalidated exactly as before.
86+
const documentPath = shapes.get(withoutQuery);
87+
if (documentPath) found.add(documentPath);
88+
}
89+
return [...found].sort();
90+
}
91+
4692
type SchemaBlock = { name: string; source: string; exported: boolean };
4793

4894
/** Every top-level `const XSchema = ...` in the source, in declaration order, with its full body. */
@@ -108,8 +154,10 @@ import { z } from "zod";
108154
`;
109155

110156
export function renderApiSchemas(sourceText: string, documentText: string, binSource: string): string {
111-
const byPath = responseSchemaByPath(JSON.parse(documentText) as OpenApiDocument, cliApiPaths(binSource));
112-
const blocks = closure(parseSchemaBlocks(sourceText), [...new Set(byPath.values())]);
157+
const document = JSON.parse(documentText) as OpenApiDocument;
158+
const byPath = responseSchemaByPath(document, cliApiPaths(binSource));
159+
const byPattern = responseSchemaByPath(document, cliParameterisedApiPaths(binSource, document));
160+
const blocks = closure(parseSchemaBlocks(sourceText), [...new Set([...byPath.values(), ...byPattern.values()])]);
113161
const body = blocks
114162
.map((block) =>
115163
block.source
@@ -122,7 +170,11 @@ export function renderApiSchemas(sourceText: string, documentText: string, binSo
122170
.sort(([left], [right]) => left.localeCompare(right))
123171
.map(([path, schema]) => ` "${path}": ${schema},`)
124172
.join("\n");
125-
return `${HEADER}${body.trimEnd()}\n\n${TABLE_HEADER}${table}\n} as const;\n\n${TABLE_TYPES}`;
173+
const patternTable = [...byPattern.entries()]
174+
.sort(([left], [right]) => left.localeCompare(right))
175+
.map(([path, schema]) => ` "${path}": ${schema},`)
176+
.join("\n");
177+
return `${HEADER}${body.trimEnd()}\n\n${TABLE_HEADER}${table}\n} as const;\n\n${PATTERN_TABLE_HEADER}${patternTable}\n} as const;\n\n${TABLE_TYPES}`;
126178
}
127179

128180
const TABLE_HEADER = `/**
@@ -135,11 +187,46 @@ const TABLE_HEADER = `/**
135187
export const CLI_RESPONSE_SCHEMAS = {
136188
`;
137189

190+
const PATTERN_TABLE_HEADER = `/**
191+
* The same, for the PARAMETERISED paths (#9773) -- keyed by the document's own \`{param}\` template.
192+
*
193+
* Separate from the table above because these cannot be looked up by an exact string: the CLI builds them
194+
* with interpolation, so the match happens at the type level (see MatchApiPath) rather than by key.
195+
*/
196+
export const CLI_PARAMETERISED_RESPONSE_SCHEMAS = {
197+
`;
198+
138199
const TABLE_TYPES = `/** A path the client validates. */
139200
export type ValidatedApiPath = keyof typeof CLI_RESPONSE_SCHEMAS;
140201
141202
/** The parsed response type for a validated path -- what the CLI call sites get instead of \`any\`. */
142203
export type ApiResponse<Path extends ValidatedApiPath> = z.infer<(typeof CLI_RESPONSE_SCHEMAS)[Path]>;
204+
205+
/** A parameterised path pattern the client validates. */
206+
export type ParameterisedApiPath = keyof typeof CLI_PARAMETERISED_RESPONSE_SCHEMAS;
207+
208+
/**
209+
* A pattern with every \`{param}\` widened to \`\${string}\`, so a concrete path can be matched against it.
210+
*
211+
* Recursive because a pattern can carry several parameters
212+
* (\`/v1/contributors/{login}/repos/{owner}/{repo}/decision\`).
213+
*/
214+
export type TemplatedApiPath<Pattern extends string> = Pattern extends \`\${infer Head}{\${string}}\${infer Tail}\`
215+
? \`\${Head}\${string}\${TemplatedApiPath<Tail>}\`
216+
: Pattern;
217+
218+
/**
219+
* The pattern a CONCRETE path matches, or \`never\` when it matches none.
220+
*
221+
* This is what lets the CLI keep writing its natural interpolated template and still get the exact response
222+
* type: the mapped type distributes over every known pattern and keeps only the arms the string satisfies.
223+
*/
224+
export type MatchApiPath<Path extends string> = {
225+
[Pattern in ParameterisedApiPath]: Path extends TemplatedApiPath<Pattern> ? Pattern : never;
226+
}[ParameterisedApiPath];
227+
228+
/** The parsed response for a concrete parameterised path. */
229+
export type ParameterisedApiResponse<Path extends string> = z.infer<(typeof CLI_PARAMETERISED_RESPONSE_SCHEMAS)[MatchApiPath<Path>]>;
143230
`;
144231

145232
export function generate(deps: { readFile?: (path: string) => string } = {}): string {

test/unit/mcp-cli-bool-flag-parsing.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,3 +133,27 @@ describe("loopover-mcp CLI — boolean `--flag=value` parsing (#8689)", () => {
133133
expect(out).toContain("[mcp_servers.loopover]");
134134
});
135135
});
136+
137+
// #9773: three crashes the `: any` on the option plumbing was hiding. Each is a real invocation a user can
138+
// type; each threw or lied before this. Typing parseOptions from CLI_FLAG_SPEC is what surfaced all three.
139+
describe("a flag given with no value (#9773)", () => {
140+
it("does not let a bare repeatable flag poison the next one", async () => {
141+
// `--issue --issue 5` stored `true` for the first, then spread it: "true is not iterable".
142+
const out = await withEnv(AUTHED, () =>
143+
captureStdout(() => mod.runCli(["preflight", "--login", "acme", "--repo", "acme/widgets", "--title", "t", "--body", "b", "--issue", "--issue", "5", "--json"])),
144+
);
145+
expect(out.length).toBeGreaterThan(0);
146+
});
147+
148+
it("reports the usage error for `maintain --repo` instead of throwing a TypeError", async () => {
149+
// Was: "repoFullName.includes is not a function" -- a bare flag parses to `true`, which passed the
150+
// truthiness guard and then died on a string method.
151+
await expect(withEnv(AUTHED, () => captureStdout(() => mod.runCli(["maintain", "list", "--repo"])))).rejects.toThrow("Pass --repo owner/repo.");
152+
});
153+
154+
it("treats a bare --login as absent rather than as a contributor NAMED \"true\"", async () => {
155+
// Was: `/v1/contributors/true/decision-pack` -- a real request for a real-looking login, answered with
156+
// "not found", when the user had simply forgotten the value.
157+
await expect(withEnv({ ...AUTHED, LOOPOVER_LOGIN: undefined, GITHUB_LOGIN: undefined }, () => captureStdout(() => mod.runCli(["decision-pack", "--login"])))).rejects.toThrow(/--login/);
158+
});
159+
});

test/unit/mcp-cli-contributor-profile-inprocess.test.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,10 @@ describe("bin loopover_get_contributor_profile stdio tool (in-process, #7760)",
9090
expect(captured.url).toContain("/v1/contributors/octocat/profile");
9191
expect(captured.method).toBe("GET");
9292
expect(result.isError).toBeFalsy();
93-
// structuredContent is the raw API payload; the summary line is the remote tool's fixed sentence.
94-
expect(result.structuredContent).toMatchObject({ login: "octocat" });
93+
// structuredContent is the raw API payload; the summary line is the tool's own fixed sentence.
94+
expect(result.structuredContent).toMatchObject({ login: "octocat", source: "github_cache" });
9595
const text = JSON.stringify(result);
9696
expect(text).toContain("LoopOver contributor profile for octocat.");
97-
expect(text).toContain("3 registered repos; 12 merged PRs; strongest in review-tooling.");
9897
} finally {
9998
await client.close().catch(() => undefined);
10099
}
@@ -117,19 +116,20 @@ describe("bin loopover_get_contributor_profile stdio tool (in-process, #7760)",
117116
});
118117

119118
describe("bin contributor-profile CLI (in-process, #7760)", () => {
120-
it.each(MODULES)("shares getContributorProfile with the stdio tool: prints the header + API summary — %s", async (specifier) => {
119+
it.each(MODULES)("shares getContributorProfile with the stdio tool: prints the header — %s", async (specifier) => {
121120
capturedRequests.length = 0;
122121
const mod = loaded.get(specifier)!;
123122
const out = await captureStdout(() => mod.contributorProfileCli({ login: "octocat" }));
124123
expect(capturedRequests.at(-1)!.url).toBe("/v1/contributors/octocat/profile");
124+
// #9773: the "API summary" line this used to assert came from a `summary` field the endpoint has never
125+
// returned -- invented by the fixture, read by the CLI, asserted here. The header is what really prints.
125126
expect(out).toMatch(/LoopOver contributor profile for octocat\./);
126-
expect(out).toContain("3 registered repos; 12 merged PRs; strongest in review-tooling.");
127127
});
128128

129129
it.each(MODULES)("--json re-serializes the same payload the shared call returned — %s", async (specifier) => {
130130
const mod = loaded.get(specifier)!;
131131
const out = await captureStdout(() => mod.contributorProfileCli({ login: "octocat", json: true }));
132-
const payload = JSON.parse(out) as { login: string; summary: string };
133-
expect(payload).toMatchObject({ login: "octocat", summary: "3 registered repos; 12 merged PRs; strongest in review-tooling." });
132+
const payload = JSON.parse(out) as { login: string; source: string };
133+
expect(payload).toMatchObject({ login: "octocat", source: "github_cache" });
134134
});
135135
});

test/unit/mcp-cli-contributor-profile.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,12 @@ describe("loopover-mcp CLI — contributor-profile (#6737)", () => {
2525

2626
const plain = await runAsync(["contributor-profile", "--login", "octocat"], e);
2727
expect(plain).toMatch(/LoopOver contributor profile for octocat\./);
28-
expect(plain).toMatch(/3 registered repos; 12 merged PRs; strongest in review-tooling\./);
2928
expect(requests.at(-1)).toBe("/v1/contributors/octocat/profile");
3029

31-
const json = JSON.parse(await runAsync(["contributor-profile", "--login", "octocat", "--json"], e)) as { login: string; summary: string };
32-
// Parity: the --json surface re-serializes the same payload the plain summary was built from.
33-
expect(json).toMatchObject({ login: "octocat", summary: "3 registered repos; 12 merged PRs; strongest in review-tooling." });
30+
const json = JSON.parse(await runAsync(["contributor-profile", "--login", "octocat", "--json"], e)) as { login: string; source: string };
31+
// Parity: the --json surface re-serializes the payload verbatim. Asserted on fields the endpoint really
32+
// returns -- the previous assertion named a `summary` that only ever existed in the fixture (#9773).
33+
expect(json).toMatchObject({ login: "octocat", source: "github_cache" });
3434
});
3535

3636
it("resolves the login from LOOPOVER_LOGIN when --login is omitted, and url-encodes it", async () => {

test/unit/support/mcp-cli-harness.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,11 @@ export async function startFixtureServer(
387387
JSON.stringify({
388388
login: decodeURIComponent(contributorProfileMatch[1]!),
389389
generatedAt: "2026-05-30T00:00:00.000Z",
390-
summary: "3 registered repos; 12 merged PRs; strongest in review-tooling.",
390+
// #9773: a `summary` used to be here. The real endpoint has never returned one -- neither
391+
// ContributorProfile shape carries the field, and the string existed nowhere but this fixture --
392+
// so it was an invented field the CLI then read and the test then asserted on. All three agreed
393+
// with each other and none agreed with the server.
394+
source: "github_cache",
391395
}),
392396
);
393397
return;

0 commit comments

Comments
 (0)