From 2927553ba245b83d904a479fae890eff814483e3 Mon Sep 17 00:00:00 2001 From: nghetienhiep <13849419+nghetienhiep@users.noreply.github.com> Date: Wed, 15 Jul 2026 05:59:38 +0000 Subject: [PATCH] fix(mcp): make loopover-mcp CLI failures respect --json loopover-mcp had no equivalent of @loopover/miner's cli-error.js contract, so any thrown error escaped the top-level dispatch as a raw Node stack trace on stderr, ignoring --json entirely. Wrap the runCli call in a try/catch and mirror the miner's reportCliFailure shape: --json failures print { ok: false, error } on stdout, otherwise the existing plain-text stderr message is unchanged. Closes #5928 --- packages/loopover-mcp/bin/loopover-mcp.js | 30 ++++++++++++- test/unit/mcp-cli-basics.test.ts | 23 +++++++++- test/unit/support/mcp-cli-harness.ts | 51 ++++++++++++++++++----- 3 files changed, 90 insertions(+), 14 deletions(-) diff --git a/packages/loopover-mcp/bin/loopover-mcp.js b/packages/loopover-mcp/bin/loopover-mcp.js index b52228ca43..395aff40b5 100755 --- a/packages/loopover-mcp/bin/loopover-mcp.js +++ b/packages/loopover-mcp/bin/loopover-mcp.js @@ -586,9 +586,35 @@ function stdioToolDescription(name) { return tool.description; } +/** Shared CLI failure output (#5928): mirrors @loopover/miner's cli-error.js contract so a `--json` + * invocation of this CLI never lets a thrown error escape as a raw Node stack trace — when `--json` + * is set, emit a parseable `{ ok: false, error }` object on stdout; otherwise log plain text to stderr. */ +function reportCliFailure(wantsJson, message) { + if (wantsJson) { + console.log(JSON.stringify({ ok: false, error: message }, null, 2)); + } else { + console.error(message); + } +} + +/** True when `--json` (or `--json=...`) appears anywhere in argv, ahead of any parsed option result. */ +function argsWantJson(args) { + return args.some((arg) => arg === "--json" || arg?.startsWith("--json=")); +} + +/** Normalize a thrown value to a safe error string for CLI output. */ +function describeCliError(error) { + return error instanceof Error ? error.message : String(error); +} + if (cliArgs[0] && cliArgs[0] !== "--stdio") { - const exitCode = await runCli(cliArgs); - process.exit(typeof exitCode === "number" ? exitCode : 0); + try { + const exitCode = await runCli(cliArgs); + process.exit(typeof exitCode === "number" ? exitCode : 0); + } catch (error) { + reportCliFailure(argsWantJson(cliArgs), describeCliError(error)); + process.exit(1); + } } const server = new McpServer({ diff --git a/test/unit/mcp-cli-basics.test.ts b/test/unit/mcp-cli-basics.test.ts index 014722f4a8..fcdcfc6055 100644 --- a/test/unit/mcp-cli-basics.test.ts +++ b/test/unit/mcp-cli-basics.test.ts @@ -2,7 +2,7 @@ import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { closeFixtureServer, createPacketRepo, run, runAsync, startFixtureServer } from "./support/mcp-cli-harness"; +import { closeFixtureServer, createPacketRepo, run, runAsync, runExpectingFailure, startFixtureServer } from "./support/mcp-cli-harness"; import mcpPackageJson from "../../packages/loopover-mcp/package.json"; describe("loopover-mcp CLI — basics", () => { @@ -162,6 +162,27 @@ describe("loopover-mcp CLI — basics", () => { expect(() => run(["bogus-command"])).toThrow(/loopover-mcp --help/); }); + it("emits a parseable { ok: false, error } object on stdout for --json failures instead of a raw stack trace (#5928)", () => { + const unknownCommand = runExpectingFailure(["bogus-command", "--json"]); + expect(unknownCommand.status).not.toBe(0); + expect(unknownCommand.stderr).toBe(""); + expect(JSON.parse(unknownCommand.stdout)).toMatchObject({ ok: false, error: expect.stringMatching(/Unknown command: bogus-command/) }); + + const missingLogin = runExpectingFailure(["preflight", "--json"]); + expect(missingLogin.status).not.toBe(0); + expect(JSON.parse(missingLogin.stdout)).toMatchObject({ ok: false, error: expect.stringMatching(/Pass --login or set LOOPOVER_LOGIN/) }); + + const badProfile = runExpectingFailure(["init-client", "--print", "codex", "--agent-profile", "autopilot", "--json"]); + expect(badProfile.status).not.toBe(0); + expect(JSON.parse(badProfile.stdout)).toMatchObject({ ok: false, error: expect.stringMatching(/Unsupported agent profile/) }); + + // the non-`--json` path is unchanged: plain text on stderr, nothing on stdout. + const plainFailure = runExpectingFailure(["bogus-command"]); + expect(plainFailure.status).not.toBe(0); + expect(plainFailure.stdout).toBe(""); + expect(plainFailure.stderr).toMatch(/Unknown command: bogus-command/); + }); + it("suggests the closest command for a near-miss typo", () => { // The suggestion is interpolated into the message, so matching `status` (not the literal // template token) confirms it ran rather than just matching the throw's printed source line. diff --git a/test/unit/support/mcp-cli-harness.ts b/test/unit/support/mcp-cli-harness.ts index 6135bbc390..d95b8ed1ed 100644 --- a/test/unit/support/mcp-cli-harness.ts +++ b/test/unit/support/mcp-cli-harness.ts @@ -14,16 +14,24 @@ export async function closeFixtureServer() { } export function run(args: string[], env: Record = {}) { - return execFileSync("node", [bin, ...args], { - encoding: "utf8", - env: { - ...process.env, - LOOPOVER_API_TIMEOUT_MS: "1000", - LOOPOVER_CONFIG_DIR: mkdtempSync(join(tmpdir(), "loopover-cli-config-")), - ...env, - }, - stdio: ["ignore", "pipe", "pipe"], - }); + try { + return execFileSync("node", [bin, ...args], { + encoding: "utf8", + env: { + ...process.env, + LOOPOVER_API_TIMEOUT_MS: "1000", + LOOPOVER_CONFIG_DIR: mkdtempSync(join(tmpdir(), "loopover-cli-config-")), + ...env, + }, + stdio: ["ignore", "pipe", "pipe"], + }); + } catch (error) { + // A failing command's message may now land on stdout (the --json contract) or stderr (plain text) -- + // fold both into the thrown Error's message so callers can keep matching it with toThrow(/regex/). + const failure = error as NodeJS.ErrnoException & { stdout?: string }; + if (failure instanceof Error && failure.stdout) failure.message = `${failure.message}\n${failure.stdout}`; + throw failure; + } } export function runAsync(args: string[], env: Record = {}) { @@ -42,7 +50,7 @@ export function runAsync(args: string[], env: Record = {}) { }, (error, stdout, stderr) => { if (error) { - reject(new Error(`${error.message}\n${stderr}`)); + reject(new Error(`${error.message}\n${stdout}\n${stderr}`)); return; } resolve(stdout); @@ -51,6 +59,27 @@ export function runAsync(args: string[], env: Record = {}) { }); } +/** Run a command expected to fail and return its exit code + both streams, instead of throwing -- + * for asserting the shape of the failure output itself (e.g. the --json `{ ok: false, error }` contract). */ +export function runExpectingFailure(args: string[], env: Record = {}) { + try { + execFileSync("node", [bin, ...args], { + encoding: "utf8", + env: { + ...process.env, + LOOPOVER_API_TIMEOUT_MS: "1000", + LOOPOVER_CONFIG_DIR: mkdtempSync(join(tmpdir(), "loopover-cli-config-")), + ...env, + }, + stdio: ["ignore", "pipe", "pipe"], + }); + } catch (error) { + const failure = error as NodeJS.ErrnoException & { status?: number | null; stdout?: string; stderr?: string }; + return { status: failure.status ?? null, stdout: failure.stdout ?? "", stderr: failure.stderr ?? "" }; + } + throw new Error(`expected \`node ${bin} ${args.join(" ")}\` to fail`); +} + export function git(cwd: string, ...args: string[]) { execFileSync("git", args, { cwd, encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }); }