diff --git a/packages/loopover-contract/src/api-requests.ts b/packages/loopover-contract/src/api-requests.ts index 7799f2693..6b49b49c5 100644 --- a/packages/loopover-contract/src/api-requests.ts +++ b/packages/loopover-contract/src/api-requests.ts @@ -344,7 +344,14 @@ export const linkedIssueContextSchema = z }) .strict(); -export const branchEligibilitySchema = z +/** + * The FIELDS a caller may send, before the route's normalisation runs (#9773). + * + * Exported separately because `branchEligibilitySchema` is a pipe (it transforms), so its members are not + * reachable through it -- and the stdio CLI needs exactly those members to validate `--branch-eligibility` + * and `--branch-eligibility-source` against the route's own vocabulary instead of restating both lists. + */ +export const branchEligibilityFields = z .object({ status: z.enum(["eligible", "ineligible", "unknown"]), source: z.enum(["github_metadata", "local_metadata", "registry", "user_supplied"]).optional(), @@ -352,8 +359,13 @@ export const branchEligibilitySchema = z checkedAt: z.string().max(MAX_LOCAL_BRANCH_REF_CHARS).optional(), stale: z.boolean().optional(), }) - .strict() - .transform((value) => ({ ...value, status: value.status === "eligible" ? ("unknown" as const) : value.status, source: "user_supplied" as const })); + .strict(); + +export const branchEligibilitySchema = branchEligibilityFields.transform((value) => ({ + ...value, + status: value.status === "eligible" ? ("unknown" as const) : value.status, + source: "user_supplied" as const, +})); export const focusManifestInputSchema = z .record(z.string(), z.unknown()) diff --git a/packages/loopover-mcp/bin/loopover-mcp.ts b/packages/loopover-mcp/bin/loopover-mcp.ts index 1602c3000..96104744a 100644 --- a/packages/loopover-mcp/bin/loopover-mcp.ts +++ b/packages/loopover-mcp/bin/loopover-mcp.ts @@ -15,7 +15,7 @@ import { type ValidatedApiPath, } from "@loopover/contract/api-schemas"; import type { LoopoverConfig } from "@loopover/contract/cli-config"; -import { validateFocusManifestSchema } from "@loopover/contract/api-requests"; +import { branchEligibilityFields, validateFocusManifestSchema } from "@loopover/contract/api-requests"; import { CLIENT_HOSTS, CLIENT_HOST_SPEC, @@ -379,12 +379,16 @@ type BooleanFlag = { [K in keyof typeof CLI_FLAG_SPEC]: (typeof CLI_FLAG_SPEC)[K * lie. What it is NOT is `any`: an unlisted flag is `string | boolean`, which still has to be narrowed * before it can be used as either. */ +/** What any one option can hold: a value, a bare flag, or a repeated value. Named so the helpers that + * narrow it (`optionText`, `optionalNumber`, `optionalInteger`, `asRepeated`) state their input once. */ +export type CliOptionValue = string | boolean | string[] | undefined; + export type CliOptions = { [K in RepeatableFlag]?: string[]; } & { [K in BooleanFlag]?: boolean; } & { - [flag: string]: string | boolean | string[] | undefined; + [flag: string]: CliOptionValue; }; const REPEATABLE_FLAGS = new Set(Object.entries(CLI_FLAG_SPEC).filter(([, kind]) => kind === "repeatable").map(([flag]) => flag)); const BOOLEAN_FLAGS = new Set(Object.entries(CLI_FLAG_SPEC).filter(([, kind]) => kind === "boolean").map(([flag]) => flag)); @@ -2838,8 +2842,8 @@ export async function maintainCli(args: readonly string[]) { // The API enforces maintainer authorization; the CLI never decides locally. Optional --window-days bounds // the block ledger the same way the route's ?windowDays query does (a non-positive value falls through to // full history server-side). - const windowDays = Number(options.windowDays); - const query = windowDays > 0 ? `?windowDays=${encodeURIComponent(windowDays)}` : ""; + const windowDays = optionalNumber(options.windowDays); + const query = windowDays !== undefined && windowDays > 0 ? `?windowDays=${encodeURIComponent(windowDays)}` : ""; const payload = await apiGet(`${repoBase}/gate-precision${query}`); const overall = payload.overall ?? {}; const window = payload.windowDays ? `last ${payload.windowDays}d` : "all history"; @@ -2859,8 +2863,8 @@ export async function maintainCli(args: readonly string[]) { // trail the remote loopover_get_selftune_override_audit tool returns). The API enforces maintainer // authorization; the CLI never decides locally. Optional --limit caps the rows the same way the route's // ?limit query does (a non-positive value falls through to the server default). - const limit = Number(options.limit); - const query = limit > 0 ? `?limit=${encodeURIComponent(limit)}` : ""; + const limit = optionalNumber(options.limit); + const query = limit !== undefined && limit > 0 ? `?limit=${encodeURIComponent(limit)}` : ""; const payload = await apiGet(`${repoBase}/selftune/overrides/audit${query}`); const audit = payload.audit ?? []; const lines = [ @@ -2874,8 +2878,8 @@ export async function maintainCli(args: readonly string[]) { // #6735 outcome calibration: read-only measurement of whether higher-slop bands merge less often and how // agent recommendations panned out. Same --window-days handling the sibling precision command uses (a // non-positive value omits ?windowDays, so the server reports full history). - const windowDays = Number(options.windowDays); - const query = windowDays > 0 ? `?windowDays=${encodeURIComponent(windowDays)}` : ""; + const windowDays = optionalNumber(options.windowDays); + const query = windowDays !== undefined && windowDays > 0 ? `?windowDays=${encodeURIComponent(windowDays)}` : ""; const payload = await apiGet(`${repoBase}/outcome-calibration${query}`); const window = payload.windowDays ? `last ${payload.windowDays}d` : "all history"; const recommendations = payload.recommendations ?? {}; @@ -2913,9 +2917,13 @@ export async function maintainCli(args: readonly string[]) { // forwards them verbatim rather than re-deciding locally, and a bad value surfaces as the API's own 400 // detail. Omitted flags are omitted from the query entirely, so the route applies its own defaults. const query = new URLSearchParams(); - if (options.since !== undefined) query.set("since", String(options.since)); - if (options.limit !== undefined) query.set("limit", String(options.limit)); - if (options.pull !== undefined) query.set("pull", String(options.pull)); + // #9773: through optionText, so a bare `--since` is absent rather than the literal string "true". + const since = optionText(options.since); + const auditLimit = optionText(options.limit); + const pull = optionText(options.pull); + if (since !== undefined) query.set("since", since); + if (auditLimit !== undefined) query.set("limit", auditLimit); + if (pull !== undefined) query.set("pull", pull); const payload = await apiGet(`${repoBase}/agent/audit-feed${query.size > 0 ? `?${query}` : ""}`); const events = payload.events ?? []; // `pullNumber` is echoed by the route only on the ?pull= branch, so the scope line reports what was asked for. @@ -2966,8 +2974,8 @@ export async function maintainCli(args: readonly string[]) { // the write path, and it is forwarded as {create:true, dryRun:false}, the exact shape the route's // explicit_create_requires_dry_run_false guard demands. A plain `generate-issue-drafts` can never create. const create = options.create === true; - const parsedLimit = Number(options.limit); - const body = { create, dryRun: !create, ...(Number.isFinite(parsedLimit) ? { limit: parsedLimit } : {}) }; + const parsedLimit = optionalNumber(options.limit); + const body = { create, dryRun: !create, ...(parsedLimit !== undefined ? { limit: parsedLimit } : {}) }; const payload = await apiPost(`${repoBase}/contributor-issue-drafts/generate`, body); const mode = payload.dryRun ? "dry-run" : "create"; const lines = [ @@ -2990,8 +2998,8 @@ export async function maintainCli(args: readonly string[]) { const goal = typeof options.goal === "string" ? options.goal.trim() : ""; if (!goal) throw new Error('Pass the planning goal: loopover-mcp maintain plan-issues --repo owner/repo --goal "...".'); const create = options.create === true; - const parsedLimit = Number(options.limit); - const body = { goal, create, dryRun: !create, ...(Number.isFinite(parsedLimit) ? { limit: parsedLimit } : {}) }; + const parsedLimit = optionalNumber(options.limit); + const body = { goal, create, dryRun: !create, ...(parsedLimit !== undefined ? { limit: parsedLimit } : {}) }; const payload = await apiPost(`${repoBase}/issue-plan-drafts/generate`, body); const mode = payload.dryRun ? "dry-run" : "create"; const lines = [ @@ -3083,7 +3091,7 @@ async function analyzeOrPreflightCli(command: "analyze-branch" | "preflight", re const options = parseOptions(args.slice(1)); // Match every other subcommand: honor --help before requiring --login / hitting git+network (#6256). if (options.help === true) return printHelp(); - const contributorLogin = options.login ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const contributorLogin = resolveLogin(options, { fromProfile: false }); if (!contributorLogin) throw new Error("Pass --login or set LOOPOVER_LOGIN."); const result = await analyzeCurrentBranch({ login: contributorLogin, @@ -3093,7 +3101,7 @@ async function analyzeOrPreflightCli(command: "analyze-branch" | "preflight", re title: options.title, body: options.body, labels: options.label, - linkedIssues: options.issue?.map((value: any) => Number(value)).filter((value: any) => Number.isInteger(value) && value > 0), + linkedIssues: issueNumbersOption(options.issue), pendingMergedPrCount: optionalInteger(options.pendingMergedPrs), pendingClosedPrCount: optionalInteger(options.pendingClosedPrs), approvedPrCount: optionalInteger(options.approvedPrs), @@ -3155,10 +3163,10 @@ function printReviewPrHelp() { async function reviewPrCli(options: CliOptions) { if (options.help === true) return printReviewPrHelp(); - const contributorLogin = options.login ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const contributorLogin = resolveLogin(options, { fromProfile: false }); if (!contributorLogin) throw new Error("Pass --login or set LOOPOVER_LOGIN."); let prBody = optionText(options.body); - if (options.bodyFile) prBody = readCliTextFile(optionText(options.bodyFile) ?? "", "Body"); + prBody = readOptionalTextFile(options.bodyFile, "Body") ?? prBody; const commitMessages = Array.isArray(options.commit) ? options.commit : options.commit ? [options.commit] : undefined; const linkedIssue = parsePositiveIntegerOption(options.linkedIssue, "--linked-issue"); const payload = await reviewLocalPr({ @@ -3170,7 +3178,7 @@ async function reviewPrCli(options: CliOptions) { body: prBody, labels: options.label, commitMessages, - linkedIssues: linkedIssue !== undefined ? [linkedIssue] : options.issue?.map((value: any) => Number(value)).filter((value: any) => Number.isInteger(value) && value > 0), + linkedIssues: linkedIssue !== undefined ? [linkedIssue] : issueNumbersOption(options.issue), }); if (options.json) { process.stdout.write(`${JSON.stringify(payload, null, 2)}\n`); @@ -3240,7 +3248,7 @@ async function lintPrTextCli(args: readonly string[]) { const commitMessages = Array.isArray(options.commit) ? options.commit : options.commit ? [options.commit] : undefined; let prBody = optionText(options.body); if (options.bodyFile) { - prBody = readCliTextFile(optionText(options.bodyFile) ?? "", "Body"); + prBody = readOptionalTextFile(options.bodyFile, "Body") ?? prBody; } const linkedIssue = parsePositiveIntegerOption(options.linkedIssue, "--linked-issue"); const payload = await apiPost("/v1/lint/pr-text", { @@ -3291,8 +3299,11 @@ function printValidateConfigHelp() { async function validateConfigCli(args: readonly string[]) { if (!args.length || args[0] === "--help" || args[0] === "help") return printValidateConfigHelp(); const options = parseOptions(args); - if (!options.file) throw new Error("Pass --file to the manifest to validate."); - const content = readCliTextFile(optionText(options.file) ?? "", "Manifest"); + // #9773: `!options.file` let a VALUELESS `--file` through -- it parses to `true`, which is truthy -- and + // the manifest was then read from the path "". Absent and valueless are the same thing to the user. + const manifestPath = optionText(options.file); + if (manifestPath === undefined) throw new Error("Pass --file to the manifest to validate."); + const content = readCliTextFile(manifestPath, "Manifest"); // #9773: parsed against the CONTRACT's own enum rather than a restated list, so the accepted values and // the error naming them both come from the schema the route validates with -- and the parsed result // narrows, which a `.includes()` guard never did. That is why the request body could carry a free string @@ -3401,7 +3412,7 @@ async function improvementPotentialCli(args: readonly string[]) { const testFiles = stringArrayOption(options.testFile); let patchCoverageDeltaPercent; if (options.patchCoverageDelta !== undefined) { - patchCoverageDeltaPercent = Number(options.patchCoverageDelta); + patchCoverageDeltaPercent = Number(optionText(options.patchCoverageDelta)); if (!Number.isFinite(patchCoverageDeltaPercent)) { throw new Error("--patch-coverage-delta must be a finite number"); } @@ -3441,7 +3452,7 @@ async function issueSlopCli(args: readonly string[]) { const options = parseOptions(args); let body = normalizeOptionalStringOption(options.body); if (options.bodyFile) { - body = readCliTextFile(optionText(options.bodyFile) ?? "", "Body"); + body = readOptionalTextFile(options.bodyFile, "Body") ?? body; } const title = normalizeOptionalStringOption(options.title); const payload = await apiPost("/v1/lint/issue-slop", { @@ -3494,7 +3505,7 @@ function printContributorProfileHelp() { // v8 can't instrument, so the shared getContributorProfile call below is graded through this in-process entry. export async function contributorProfileCli(options: CliOptions) { if (options.help === true) return printContributorProfileHelp(); - const login = optionText(options.login) ?? activeProfile.session?.login ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const login = resolveLogin(options); if (!login) throw new Error("Pass --login , log in with `loopover-mcp login`, or set LOOPOVER_LOGIN."); // #7760: shared with the loopover_get_contributor_profile stdio tool so the endpoint path lives in one place. const payload = await getContributorProfile(login); @@ -3512,7 +3523,7 @@ export async function contributorProfileCli(options: CliOptions) { async function decisionPackCli(options: CliOptions) { if (options.help === true) return printDecisionPackHelp(); - const login = optionText(options.login) ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const login = resolveLogin(options, { fromProfile: false }); if (!login) throw new Error("Pass --login or set LOOPOVER_LOGIN."); const payload = await getDecisionPackWithCache(login); if (options.json) { @@ -3542,7 +3553,7 @@ function printMonitorOpenPrsHelp() { async function monitorOpenPrsCli(options: CliOptions) { if (options.help === true) return printMonitorOpenPrsHelp(); - const login = optionText(options.login) ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const login = resolveLogin(options, { fromProfile: false }); if (!login) throw new Error("Pass --login or set LOOPOVER_LOGIN."); const payload = await getOpenPrMonitor(login); if (options.json) { @@ -3576,7 +3587,7 @@ function printPrOutcomesHelp() { async function prOutcomesCli(options: CliOptions) { if (options.help === true) return printPrOutcomesHelp(); - const login = optionText(options.login) ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const login = resolveLogin(options, { fromProfile: false }); if (!login) throw new Error("Pass --login or set LOOPOVER_LOGIN."); const limitRaw = options.limit; let limit; @@ -3626,11 +3637,7 @@ async function explainReviewRiskCli(options: CliOptions) { const contributorLogin = optionText(options.login) ?? optionText(options.contributorLogin); const labels = Array.isArray(options.label) ? options.label : options.label ? [options.label] : undefined; const changedFiles = Array.isArray(options.changedFile) ? options.changedFile : options.changedFile ? [options.changedFile] : undefined; - const linkedIssues = Array.isArray(options.issue) - ? options.issue.map((value: any) => Number(value)).filter((value: any) => Number.isInteger(value) && value > 0) - : options.issue - ? [Number(options.issue)].filter((value) => Number.isInteger(value) && value > 0) - : undefined; + const linkedIssues = issueNumbersOption(options.issue); const tests = Array.isArray(options.test) ? options.test : options.test ? [options.test] : undefined; const payload = await apiPost( "/v1/preflight/review-risk", @@ -3672,7 +3679,7 @@ function printNotificationsHelp() { // LOOPOVER_LOGIN / GITHUB_LOGIN, like the sibling contributor commands. async function notificationsCli(options: CliOptions) { if (options.help === true) return printNotificationsHelp(); - const login = optionText(options.login) ?? activeProfile.session?.login ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const login = resolveLogin(options); if (!login) throw new Error("Pass --login , log in with `loopover-mcp login`, or set LOOPOVER_LOGIN."); const payload = await getNotifications(login); if (options.json) { @@ -3706,7 +3713,7 @@ export async function watchCli(args: readonly string[]) { if (!subcommand || subcommand === "--help" || subcommand === "help") return printWatchHelp(); const positional = args[1] && !args[1].startsWith("--") ? args[1] : undefined; const options = parseOptions(args.slice(1)); - const login = optionText(options.login) ?? activeProfile.session?.login ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const login = resolveLogin(options); if (!login) throw new Error("Pass --login , log in with `loopover-mcp login`, or set LOOPOVER_LOGIN."); // The API chooses `changed` / repo / label text, so the plain-text path is sanitized (#6261); `login` is the // user's own value. @@ -3783,7 +3790,7 @@ function printNotificationsReadHelp() { // them marks every delivered notification read (mirrors the route's absent-body behavior). async function notificationsReadCli(options: CliOptions) { if (options.help === true) return printNotificationsReadHelp(); - const login = optionText(options.login) ?? activeProfile.session?.login ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const login = resolveLogin(options); if (!login) throw new Error("Pass --login , log in with `loopover-mcp login`, or set LOOPOVER_LOGIN."); const ids = Array.isArray(options.id) ? options.id : options.id ? [options.id] : undefined; const payload = await postMarkNotificationsRead(login, ids); @@ -3809,7 +3816,7 @@ function printRepoDecisionHelp() { async function repoDecisionCli(options: CliOptions) { if (options.help === true) return printRepoDecisionHelp(); - const login = optionText(options.login) ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const login = resolveLogin(options, { fromProfile: false }); if (!login) throw new Error("Pass --login or set LOOPOVER_LOGIN."); const repoFullName = options.repo; // #9773: `typeof`, not truthiness. A bare `--repo` with no value parses to `true`, which passed the old @@ -3867,7 +3874,7 @@ async function runAgentCli(args: readonly string[]) { // /v1/agent/runs request shape. `objective` and `actorLogin` are non-optional in agentRunShape // (src/mcp/server.ts), so enforce both here, resolving --login exactly as plan/packet do. surface is "cli" // (not the stdio tool's "mcp") so the two entry points stay distinguishable server-side, per the issue. - const login = optionText(options.login) ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const login = resolveLogin(options, { fromProfile: false }); if (!login) throw new Error("Pass --login or set LOOPOVER_LOGIN."); if (!options.objective || options.objective === true) throw new Error('Pass --objective "..." to describe the run.'); const payload = await apiPost("/v1/agent/runs", { @@ -3883,7 +3890,7 @@ async function runAgentCli(args: readonly string[]) { return outputAgentPayload(payload, options, `Queued LoopOver base-agent run for ${login}.`); } if (subcommand === "plan") { - const login = optionText(options.login) ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const login = resolveLogin(options, { fromProfile: false }); if (!login) throw new Error("Pass --login or set LOOPOVER_LOGIN."); const payload = await apiPost("/v1/agent/plan-next-work", stripUndefined({ login, repoFullName: optionText(options.repo), objective: options.objective, surface: "mcp" })); return outputAgentPayload(payload, options, `LoopOver agent plan: ${payload.summary ?? payload.run?.status ?? "ready"}`); @@ -3902,7 +3909,7 @@ async function runAgentCli(args: readonly string[]) { return outputAgentPayload({ ...payload, topAction }, options, topAction ? `Top action: ${topAction.recommendation}` : "No top action is available yet."); } if (subcommand === "packet") { - const login = optionText(options.login) ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; + const login = resolveLogin(options, { fromProfile: false }); if (!login) throw new Error("Pass --login or set LOOPOVER_LOGIN."); const payload = await agentPreparePrPacket({ login, @@ -3912,7 +3919,7 @@ async function runAgentCli(args: readonly string[]) { title: options.title, body: options.body, labels: options.label, - linkedIssues: options.issue?.map((value: any) => Number(value)).filter((value: any) => Number.isInteger(value) && value > 0), + linkedIssues: issueNumbersOption(options.issue), pendingMergedPrCount: optionalInteger(options.pendingMergedPrs), pendingClosedPrCount: optionalInteger(options.pendingClosedPrs), approvedPrCount: optionalInteger(options.approvedPrs), @@ -4368,6 +4375,34 @@ function printProfileHelp() { process.stdout.write(printableUsage("profile")); } +/** + * The contributor login a command should act on (#9773). + * + * One chain, not eight copies. Every command that takes `--login` resolved it inline -- flag, then the + * active profile's session, then two env vars -- and each copy was its own chance to miss a step, which is + * exactly how two of them ended up reading a valueless `--login` as the string "true". The profile leg is + * skipped by the commands that never had it, so this is a superset of no call site's behaviour. + */ +function resolveLogin(options: CliOptions, { fromProfile = true }: { fromProfile?: boolean } = {}): string | undefined { + const fromFlag = optionText(options.login); + const fromSession = fromProfile ? activeProfile.session?.login : undefined; + return fromFlag ?? fromSession ?? process.env.LOOPOVER_LOGIN ?? process.env.GITHUB_LOGIN; +} + +/** A `--body-file`/`--file` option read from disk, or undefined when the flag carries no value (#9773). */ +function readOptionalTextFile(value: CliOptionValue, label: string): string | undefined { + const path = optionText(value); + return path === undefined ? undefined : readCliTextFile(path, label); +} + +/** `--issue` as positive integers, however it was supplied: repeated, once, or not at all (#9773). */ +function issueNumbersOption(value: CliOptionValue): number[] | undefined { + const numbers = asRepeated(value) + .map((entry) => Number(entry)) + .filter((entry) => Number.isInteger(entry) && entry > 0); + return numbers.length > 0 ? numbers : undefined; +} + /** * Whether a user-supplied string is one of a closed set (#9773). * @@ -4387,7 +4422,7 @@ function isOneOf(list: T, value: string): val * NAMED "true" and report them not found, rather than telling the user they forgot the value. Absent is the * honest answer, and every caller here already handles absent. */ -function optionText(value: string | boolean | string[] | undefined): string | undefined { +function optionText(value: CliOptionValue): string | undefined { return typeof value === "string" ? value : undefined; } @@ -4468,7 +4503,7 @@ function emitList(options: CliOptions, items: any, pretty: any) { async function login(options: CliOptions) { const profileName = selectedProfileName(options); - const githubToken = options.githubToken ?? process.env.GITHUB_TOKEN; + const githubToken = optionText(options.githubToken) ?? process.env.GITHUB_TOKEN; const session = githubToken ? await apiFetch("/v1/auth/github/session", { method: "POST", body: JSON.stringify({ githubToken }) }, { auth: false }) : await loginWithDeviceFlow(); const nextConfig = upsertProfile(config, profileName, { apiUrl, @@ -5231,7 +5266,7 @@ function upsertProfile(currentConfig: LoopoverConfig, profileName: string, patch return normalizeConfig({ ...currentConfig, apiUrl: patch.apiUrl ?? currentConfig.apiUrl, activeProfile: profileName, profiles }); } -function ensureProfile(currentConfig: LoopoverConfig, profileName: string, options: CliOptions = {}) { +function ensureProfile(currentConfig: LoopoverConfig, profileName: string, options: CliOptions) { const existing = currentConfig.profiles?.[profileName]; const nextConfig = existing ? currentConfig : upsertProfile(currentConfig, profileName, {}); return options.activate ? setActiveProfile(nextConfig, profileName) : nextConfig; @@ -5330,7 +5365,8 @@ function validationEntry({ command, statusText, summaryText, durationText, durat }); } -function optionalInteger(value: any) { +/** A non-negative integer option, or undefined. `true` -- a flag with no value -- is absent, not a number. */ +function optionalInteger(value: CliOptionValue) { if (value === undefined || value === true) return undefined; const parsed = Number(value); return Number.isInteger(parsed) && parsed >= 0 ? parsed : undefined; @@ -5350,7 +5386,8 @@ function normalizeOptionalStringOption(value: any) { throw new Error("Expected a string flag value."); } -function optionalNumber(value: any) { +/** A finite number option, or undefined. Same treatment of a valueless flag as `optionalInteger`. */ +function optionalNumber(value: CliOptionValue) { if (value === undefined || value === true) return undefined; const parsed = Number(value); return Number.isFinite(parsed) ? parsed : undefined; @@ -6342,18 +6379,22 @@ function localDiffTargetKey(branchPayload: any, baseRef: any) { } function branchEligibilityFromOptions(options: CliOptions) { - const status = optionText(options.branchEligibility) ?? optionText(options.branchEligibilityStatus); - if (!status || !["eligible", "ineligible", "unknown"].includes(status)) return undefined; - const source = ["github_metadata", "local_metadata", "registry", "user_supplied"].includes(optionText(options.branchEligibilitySource) ?? "") ? optionText(options.branchEligibilitySource)! : "user_supplied"; + // #9773: parsed against the ROUTE'S OWN schema rather than two lists restated here. Both vocabularies + // were hand-copied from `branchEligibilitySchema`, so a status the route accepts could be dropped on the + // floor by the CLI -- silently, since an unrecognised value simply returned undefined. + const status = branchEligibilityFields.shape.status.safeParse(optionText(options.branchEligibility) ?? optionText(options.branchEligibilityStatus)); + if (!status.success) return undefined; + const source = branchEligibilityFields.shape.source.unwrap().safeParse(optionText(options.branchEligibilitySource)); return stripUndefined({ - status, - source, + status: status.data, + source: source.success ? source.data : "user_supplied", reason: optionText(options.branchEligibilityReason), checkedAt: optionText(options.branchEligibilityCheckedAt), stale: optionalBoolean(options.branchEligibilityStale), }); } + function optionalBoolean(value: any) { if (value === undefined) return undefined; if (value === true) return true; diff --git a/test/unit/mcp-cli-bool-flag-parsing.test.ts b/test/unit/mcp-cli-bool-flag-parsing.test.ts index 73d304304..84a55455b 100644 --- a/test/unit/mcp-cli-bool-flag-parsing.test.ts +++ b/test/unit/mcp-cli-bool-flag-parsing.test.ts @@ -157,3 +157,60 @@ describe("a flag given with no value (#9773)", () => { await expect(withEnv({ ...AUTHED, LOOPOVER_LOGIN: undefined, GITHUB_LOGIN: undefined }, () => captureStdout(() => mod.runCli(["decision-pack", "--login"])))).rejects.toThrow(/--login/); }); }); + +// #9773 follow-up: the same class, at the four sites the first sweep missed. A review found one of them +// (`explain-review-risk --login`); these are the rest of the family, each reachable by a plausible typo. +// +// The two query-shaping cases assert on the URL the CLI actually requests, which is the only place the +// difference between "absent" and "the literal true" is observable. +describe("a flag given with no value, everywhere else it is read (#9773)", () => { + /** Run with `fetch` stubbed, and return every URL the CLI asked for. */ + async function requestedUrls(args: string[], env: Record): Promise { + const urls: string[] = []; + const stub = vi.fn(async (input: unknown) => { + urls.push(String(input)); + return new Response(JSON.stringify({ audit: [], events: [], overall: {} }), { status: 200, headers: { "content-type": "application/json" } }); + }); + vi.stubGlobal("fetch", stub); + try { + await withEnv(env, () => captureStdout(() => mod.runCli(args))).catch(() => undefined); + } finally { + vi.unstubAllGlobals(); + } + return urls; + } + + it("asks for a login on analyze-branch when the bare flag has no env fallback", async () => { + // Was: `login: true` sent to the branch-analysis route -- neither a login nor an error a user could act + // on. Absent means absent, so the usual "pass it or set the env var" error is what they get. + await expect( + withEnv({ ...AUTHED, LOOPOVER_LOGIN: undefined, GITHUB_LOGIN: undefined }, () => + captureStdout(() => mod.runCli(["analyze-branch", "--login", "--repo", "acme/widgets"])), + ), + ).rejects.toThrow(/--login/); + }); + + it("asks for a login on review-pr when the bare flag has no env fallback", async () => { + await expect( + withEnv({ ...AUTHED, LOOPOVER_LOGIN: undefined, GITHUB_LOGIN: undefined }, () => + captureStdout(() => mod.runCli(["review-pr", "--login", "--repo", "acme/widgets", "--pull", "1"])), + ), + ).rejects.toThrow(/--login/); + }); + + it("does not read a bare --limit as the number 1", async () => { + // `Number(true)` is 1, so `--limit` with no value silently capped the audit at a single row instead of + // leaving the server's own default in place. + const urls = await requestedUrls(["maintain", "selftune-audit", "--repo", "owner/repo", "--limit", "--json"], AUTHED); + expect(urls.some((url) => url.includes("/selftune/overrides/audit"))).toBe(true); + expect(urls.find((url) => url.includes("/selftune/overrides/audit"))).not.toContain("limit="); + }); + + it("does not send the literal string \"true\" as an audit-feed query value", async () => { + // `String(options.since)` on a bare `--since` sent `?since=true`, which the route reads as a cursor. + const urls = await requestedUrls(["maintain", "audit-feed", "--repo", "owner/repo", "--since", "--json"], AUTHED); + const feed = urls.find((url) => url.includes("/agent/audit-feed")); + expect(feed).toBeTruthy(); + expect(feed).not.toContain("since="); + }); +}); diff --git a/test/unit/mcp-cli-option-resolution.test.ts b/test/unit/mcp-cli-option-resolution.test.ts new file mode 100644 index 000000000..a1fed5ca1 --- /dev/null +++ b/test/unit/mcp-cli-option-resolution.test.ts @@ -0,0 +1,287 @@ +// How the stdio CLI resolves an option's value, on both sides of every fallback (#9773). +// +// Typing `parseOptions` from `CLI_FLAG_SPEC` turned a pile of `any` reads into explicit chains -- the flag, +// then the profile session, then the env -- and a chain is only correct if each step is exercised. These +// drive the real dispatcher in-process (the #8587 pattern), because the subprocess harness the rest of the +// CLI suite uses runs the compiled `dist/`, which v8 cannot instrument: a path covered only there reads as +// uncovered, which is how a regression in one of these arms would land unnoticed. +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; +import { closeFixtureServer, startFixtureServer } from "./support/mcp-cli-harness"; + +type BinModule = { + runCli: (args: string[]) => Promise; + server: Parameters[0] extends never ? never : { connect: (transport: unknown) => Promise }; +}; + +const BIN_MODULE = "../../packages/loopover-mcp/bin/loopover-mcp.ts"; + +let configDir = ""; +let mod: BinModule; + +beforeAll(async () => { + configDir = mkdtempSync(join(tmpdir(), "loopover-option-resolution-")); + const apiUrl = await startFixtureServer(); + process.env.LOOPOVER_API_URL = apiUrl; + process.env.LOOPOVER_NPM_REGISTRY_URL = apiUrl; + process.env.LOOPOVER_API_TIMEOUT_MS = "2000"; + process.env.LOOPOVER_CONFIG_DIR = configDir; + mod = (await import(BIN_MODULE)) as unknown as BinModule; +}, 120_000); + +afterAll(async () => { + await closeFixtureServer(); + if (configDir) rmSync(configDir, { recursive: true, force: true }); + for (const key of ["LOOPOVER_API_URL", "LOOPOVER_NPM_REGISTRY_URL", "LOOPOVER_API_TIMEOUT_MS", "LOOPOVER_CONFIG_DIR"]) delete process.env[key]; +}); + +async function withEnv(overrides: Record, fn: () => Promise): Promise { + const saved = new Map(); + for (const [key, value] of Object.entries(overrides)) { + saved.set(key, process.env[key]); + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + try { + return await fn(); + } finally { + for (const [key, value] of saved) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + } +} + +/** Run with `fetch` stubbed; return every request the CLI made, so the RESOLVED value is observable. */ +async function capture(args: string[], env: Record = {}): Promise> { + const requests: Array<{ url: string; body: unknown }> = []; + vi.stubGlobal( + "fetch", + vi.fn(async (input: unknown, init?: { body?: string }) => { + requests.push({ url: String(input), body: init?.body ? (JSON.parse(init.body) as unknown) : undefined }); + return new Response(JSON.stringify({ audit: [], events: [], overall: {}, findings: [], run: { status: "queued" }, actions: [] }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + }), + ); + const stdout = vi.spyOn(process.stdout, "write").mockImplementation(() => true); + try { + await withEnv({ LOOPOVER_TOKEN: "session-token", LOOPOVER_SKIP_NPM_VERSION_CHECK: "true", ...env }, () => mod.runCli(args)).catch(() => undefined); + } finally { + stdout.mockRestore(); + vi.unstubAllGlobals(); + } + return requests; +} + +const NO_LOGIN_ENV = { LOOPOVER_LOGIN: undefined, GITHUB_LOGIN: undefined }; + +describe("the login chain, one step at a time (#9773)", () => { + it("prefers the flag", async () => { + const [request] = await capture(["contributor-profile", "--login", "from-flag", "--json"], { ...NO_LOGIN_ENV }); + expect(request?.url).toContain("/from-flag/"); + }); + + it("falls through to LOOPOVER_LOGIN when the flag is absent", async () => { + const [request] = await capture(["contributor-profile", "--json"], { ...NO_LOGIN_ENV, LOOPOVER_LOGIN: "from-loopover-env" }); + expect(request?.url).toContain("/from-loopover-env/"); + }); + + it("falls through again to GITHUB_LOGIN", async () => { + const [request] = await capture(["contributor-profile", "--json"], { ...NO_LOGIN_ENV, GITHUB_LOGIN: "from-github-env" }); + expect(request?.url).toContain("/from-github-env/"); + }); + + it("reports the usage error when every step is empty", async () => { + await expect( + withEnv({ LOOPOVER_TOKEN: "session-token", ...NO_LOGIN_ENV }, () => Promise.resolve(mod.runCli(["contributor-profile", "--json"]))), + ).rejects.toThrow(/--login/); + }); + + it("treats a bare --login as absent on a command with no profile leg", async () => { + // `explain-repo-decision` resolves flag -> env only. A valueless flag must not become the login "true". + const [request] = await capture(["repo-decision", "--login", "--repo", "owner/repo"], { ...NO_LOGIN_ENV, LOOPOVER_LOGIN: "env-login" }); + expect(request?.url ?? "").not.toContain("login=true"); + expect(request?.url ?? "").toContain("env-login"); + }); +}); + +describe("a --body-file / --file option, present and absent (#9773)", () => { + it("reads the file when the flag carries a path", async () => { + const bodyPath = join(configDir, "body.md"); + writeFileSync(bodyPath, "body from disk"); + const [request] = await capture(["review-pr", "--login", "acme", "--repo", "owner/repo", "--pull", "1", "--body-file", bodyPath]); + expect(JSON.stringify(request?.body)).toContain("body from disk"); + }); + + it("keeps the inline --body when no --body-file is given", async () => { + const [request] = await capture(["review-pr", "--login", "acme", "--repo", "owner/repo", "--pull", "1", "--body", "inline body"]); + expect(JSON.stringify(request?.body)).toContain("inline body"); + }); + + it("leaves the inline --body alone when --body-file carries no value", async () => { + // Was `readCliTextFile(optionText(...) ?? "", ...)`, i.e. a read of the path "" -- an ENOENT crash + // where the user had simply typed the flag without a path. + const [request] = await capture(["review-pr", "--login", "acme", "--repo", "owner/repo", "--pull", "1", "--body", "inline body", "--body-file"]); + expect(JSON.stringify(request?.body)).toContain("inline body"); + }); + + it("reads --body-file for lint-pr-text and check-issue-slop, and leaves --body alone without it", async () => { + // The same pair of arms in two more commands. Both used to read the path "" for a valueless flag. + const filePath = join(configDir, "shared-body.md"); + writeFileSync(filePath, "text from disk"); + for (const args of [ + ["lint-pr-text", "--title", "t", "--body-file", filePath], + ["issue-slop", "--title", "t", "--body-file", filePath], + ]) { + const [request] = await capture(args); + expect(JSON.stringify(request?.body), args[0]).toContain("text from disk"); + } + for (const args of [ + ["lint-pr-text", "--title", "t", "--body", "inline", "--body-file"], + ["issue-slop", "--title", "t", "--body", "inline", "--body-file"], + ]) { + const [request] = await capture(args); + expect(JSON.stringify(request?.body), args[0]).toContain("inline"); + } + }); + + it("reports the usage error when validate-config's --file carries no value", async () => { + // `!options.file` let a valueless flag through -- `true` is truthy -- and the manifest was read from + // the path "". Absent and valueless are the same thing to the person typing it. + await expect(Promise.resolve(mod.runCli(["validate-config", "--file"]))).rejects.toThrow(/--file/); + }); +}); + +describe("the usage errors a valueless flag must produce (#9773)", () => { + it("names --source when validate-config is given one the schema does not accept", async () => { + const manifestPath = join(configDir, "manifest.json"); + writeFileSync(manifestPath, "{}"); + await expect(Promise.resolve(mod.runCli(["validate-config", "--file", manifestPath, "--source", "invented"]))).rejects.toThrow(/--source must be one of/); + }); + + it("names --title when explain-review-risk is given a valueless one", async () => { + await expect(Promise.resolve(mod.runCli(["explain-review-risk", "--repo", "owner/repo", "--title"]))).rejects.toThrow(/--title/); + }); + + it("names --repo when repo-decision is given a valueless one", async () => { + // `typeof repoFullName !== "string"` is the arm a bare `--repo` takes: it parses to `true`, which would + // otherwise reach `.includes` and throw a TypeError instead of the usage error. + await expect( + withEnv({ LOOPOVER_TOKEN: "session-token", LOOPOVER_LOGIN: "acme" }, () => Promise.resolve(mod.runCli(["repo-decision", "--repo"]))), + ).rejects.toThrow(/--repo owner\/repo/); + }); + + it("uses --body as the slop-risk description when --description is absent", async () => { + const [request] = await capture(["slop-risk", "--title", "t", "--body", "from body flag"]); + expect(JSON.stringify(request?.body)).toContain("from body flag"); + }); +}); + +describe("--issue, however it is supplied (#9773)", () => { + it("accepts a single occurrence", async () => { + const [request] = await capture(["preflight", "--login", "acme", "--repo", "owner/repo", "--title", "t", "--issue", "7"]); + expect((request?.body as { linkedIssues?: number[] })?.linkedIssues).toContain(7); + }); + + it("accepts repeated occurrences", async () => { + const [request] = await capture(["preflight", "--login", "acme", "--repo", "owner/repo", "--title", "t", "--issue", "7", "--issue", "9"]); + expect((request?.body as { linkedIssues?: number[] })?.linkedIssues).toEqual(expect.arrayContaining([7, 9])); + }); + + it("accepts repeated inline `--issue=N`", async () => { + const [request] = await capture(["preflight", "--login", "acme", "--repo", "owner/repo", "--title", "t", "--issue=7", "--issue=9"]); + expect((request?.body as { linkedIssues?: number[] })?.linkedIssues).toEqual(expect.arrayContaining([7, 9])); + }); + + it("contributes nothing when the value is not a positive integer", async () => { + const [request] = await capture(["preflight", "--login", "acme", "--repo", "owner/repo", "--title", "t", "--issue", "not-a-number"]); + expect((request?.body as { linkedIssues?: number[] })?.linkedIssues ?? []).not.toContain(Number.NaN); + }); +}); + +describe("a run id from the positional or the flag (#9773)", () => { + it("takes the positional argument", async () => { + const [request] = await capture(["agent", "status", "run-from-positional"]); + expect(request?.url).toContain("run-from-positional"); + }); + + it("falls back to --run-id, and treats a bare one as absent", async () => { + const [request] = await capture(["agent", "explain", "--run-id", "run-from-flag"]); + expect(request?.url).toContain("run-from-flag"); + await expect(Promise.resolve(mod.runCli(["agent", "status", "--run-id"]))).rejects.toThrow(/run-id/); + }); +}); + +describe("--branch-eligibility, parsed against the route's own schema (#9773)", () => { + it("keeps a source the schema declares", async () => { + const [request] = await capture([ + "preflight", "--login", "acme", "--repo", "owner/repo", "--title", "t", + "--branch-eligibility", "ineligible", "--branch-eligibility-source", "registry", + ]); + expect((request?.body as { branchEligibility?: { source?: string } })?.branchEligibility?.source).toBe("registry"); + }); + + it("falls back to user_supplied for a source the schema does not declare", async () => { + const [request] = await capture([ + "preflight", "--login", "acme", "--repo", "owner/repo", "--title", "t", + "--branch-eligibility", "ineligible", "--branch-eligibility-source", "invented", + ]); + expect((request?.body as { branchEligibility?: { source?: string } })?.branchEligibility?.source).toBe("user_supplied"); + }); + + it("drops the whole block when the status is not one the schema accepts", async () => { + const [request] = await capture(["preflight", "--login", "acme", "--repo", "owner/repo", "--title", "t", "--branch-eligibility", "maybe"]); + expect((request?.body as { branchEligibility?: unknown })?.branchEligibility).toBeUndefined(); + }); +}); + +describe("a stdio tool's optional list fields (#9773)", () => { + /** The stdio server in-process, so a tool handler's own branches are exercised where v8 can see them. */ + async function callTool(name: string, args: Record): Promise { + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "option-resolution", version: "0.0.0" }); + await Promise.all([(mod.server as { connect: (t: unknown) => Promise }).connect(serverTransport), client.connect(clientTransport)]); + try { + return await client.callTool({ name, arguments: args }); + } finally { + await client.close().catch(() => undefined); + } + } + + it("sends changedFiles only when changedPaths was supplied", async () => { + const requests: Array<{ body: unknown }> = []; + vi.stubGlobal( + "fetch", + vi.fn(async (_input: unknown, init?: { body?: string }) => { + requests.push({ body: init?.body ? (JSON.parse(init.body) as unknown) : undefined }); + return new Response(JSON.stringify({ verdict: "pass", findings: [] }), { status: 200, headers: { "content-type": "application/json" } }); + }), + ); + const savedToken = process.env.LOOPOVER_TOKEN; + process.env.LOOPOVER_TOKEN = "session-token"; + try { + void (await callTool("loopover_predict_gate", { login: "acme", owner: "owner", repo: "repo", title: "t", changedPaths: ["src/a.ts"] })); + void (await callTool("loopover_predict_gate", { login: "acme", owner: "owner", repo: "repo", title: "t" })); + // The same pair of arms in the sibling handler. + void (await callTool("loopover_explain_gate_disposition", { login: "acme", owner: "owner", repo: "repo", title: "t", changedPaths: ["src/b.ts"] })); + void (await callTool("loopover_explain_gate_disposition", { login: "acme", owner: "owner", repo: "repo", title: "t" })); + } finally { + vi.unstubAllGlobals(); + if (savedToken === undefined) delete process.env.LOOPOVER_TOKEN; + else process.env.LOOPOVER_TOKEN = savedToken; + } + // The request reaching the stub at all is the assertion that the handler ran; what the stub answers + // with is not this test's subject. + expect(requests).toHaveLength(4); + expect((requests[0]?.body as { changedFiles?: unknown })?.changedFiles).toEqual([{ path: "src/a.ts" }]); + expect((requests[1]?.body as { changedFiles?: unknown })?.changedFiles).toBeUndefined(); + expect((requests[2]?.body as { changedFiles?: unknown })?.changedFiles).toEqual([{ path: "src/b.ts" }]); + expect((requests[3]?.body as { changedFiles?: unknown })?.changedFiles).toBeUndefined(); + }); +});