diff --git a/README.md b/README.md index c6bcd9a..4dce871 100644 --- a/README.md +++ b/README.md @@ -991,10 +991,14 @@ moshcode mcp catalog # what we know how to run moshcode mcp add porkbun # expands to: npx -y @porkbunllc/mcp-server ``` -That registers it across every engine that supports MCP (claude, gemini, codex, -opencode, privacycode) in one go. Kimi is skipped with a reason: it runs MCP -servers but has no command to register one from a script — add those in-session -with its own `/mcp-config`, or in `~/.kimi-code/mcp.json`. +That registers it across every engine that supports MCP (claude, gemini, qwen, +codex, opencode, privacycode) in one go. Kimi is skipped with a reason: it runs +MCP servers but has no command to register one from a script — add those +in-session with its own `/mcp-config`, or in `~/.kimi-code/mcp.json`. + +Re-running an install is safe: an engine that already has the server reports +`already registered` rather than an error, so the summary only goes red when +something actually went wrong. The catalog is a convenience, never a gate — an explicit command always wins, so `moshcode mcp add porkbun -- node ./my-fork.js` runs your fork. diff --git a/src/engines.mjs b/src/engines.mjs index 150a48b..2b7a391 100644 --- a/src/engines.mjs +++ b/src/engines.mjs @@ -405,15 +405,37 @@ export function agentLaunchArgs(engine, args = []) { * Spawn an arbitrary command with stdio inherited (so its own progress/prompts * own the terminal). Resolves { ok, code, signal } on exit. Used by install + * upgrade to run engine installers/updaters. + * + * With `{ capture: true }` the child's stdout/stderr are piped and *echoed + * through* rather than inherited, and the combined text comes back as `output`. + * The terminal still sees exactly what it saw before — the tee exists so a + * caller can read the engine's own words about *why* it exited non-zero, which + * a bare exit code cannot tell apart (see `alreadyRegistered` in mcp.mjs). + * Inherit stays the default: piping costs a couple of streams, and every other + * caller runs installers whose output nobody needs to parse. + * + * stdin is inherited either way, so a child that prompts still reaches the user. */ -export function runCmd(cmd, args = []) { +export function runCmd(cmd, args = [], { capture = false } = {}) { return new Promise((resolve) => { let child; const spec = spawnSpec(cmd, args); - try { child = spawn(spec.cmd, spec.args, { stdio: "inherit" }); } + const stdio = capture ? ["inherit", "pipe", "pipe"] : "inherit"; + try { child = spawn(spec.cmd, spec.args, { stdio }); } catch (e) { resolve({ ok: false, error: e }); return; } - child.on("error", (e) => resolve({ ok: false, error: e })); - child.on("exit", (code, signal) => resolve({ ok: true, code, signal })); + let output = ""; + if (capture) { + for (const [stream, sink] of [[child.stdout, process.stdout], [child.stderr, process.stderr]]) { + stream?.on("data", (chunk) => { output += chunk.toString(); sink.write(chunk); }); + } + } + child.on("error", (e) => resolve({ ok: false, error: e, output })); + // "exit" fires as soon as the process is gone, which with pipes can leave + // the last chunk still queued — the one line we are trying to read. "close" + // waits for the streams too. With stdio inherited there are no streams, so + // the two are the same moment and existing callers are unaffected; the + // distinction is kept explicit so neither branch changes by accident. + child.on(capture ? "close" : "exit", (code, signal) => resolve({ ok: true, code, signal, output })); }); } diff --git a/src/integrations.mjs b/src/integrations.mjs index c23a8ad..2b377fc 100644 --- a/src/integrations.mjs +++ b/src/integrations.mjs @@ -105,6 +105,22 @@ export function parseMcp(tokens) { }; } + // A remote server is a URL and nothing else — every engine's builder pushes + // the target alone and discards `args`. So a leftover token here is not a + // command line, it is something the user typed that this command will silently + // throw away. `mcp install --dry-run` is the case that matters: the flag + // does not exist, it lands here, and the install goes ahead and writes to + // every engine's config — the exact opposite of what the person typing it + // expected. Say so instead of dropping it on the floor. + if (!cmdParts && target && isRemoteTarget(target) && args.length) { + const extra = args[0]; + return { + error: extra.startsWith("-") + ? `unknown mcp flag "${extra}" — mcp takes --name, -t/--transport, -e/--env, and -H/--header, and has no --dry-run` + : `unexpected argument "${extra}" after a remote server URL — a URL server takes no command arguments`, + }; + } + if (verb === "install" && !name) { if (target && isRemoteTarget(target)) name = deriveName(target); else return { error: "a stdio command server needs an explicit --name" }; @@ -185,6 +201,9 @@ export function printSkillTargets(json = false) { function summarize(results) { for (const r of results) { if (r.status === "added" || r.status === "installed" || r.status === "removed") console.log(line(r.key, ok(r.status))); + // Nothing to do and nothing wrong: grey, like the other "we didn't act" + // rows, rather than the green of a change we actually made. + else if (r.status === "already") console.log(line(r.key, ash("already registered"))); else if (r.status === "failed") console.log(line(r.key, err(`failed${r.code != null ? ` (code ${r.code})` : r.signal ? ` (${r.signal})` : ""}`))); else if (r.status === "not-installed") console.log(line(r.key, ash("not installed — /install " + r.key))); else console.log(line(r.key, ash(`skipped — ${r.reason}`))); diff --git a/src/mcp.mjs b/src/mcp.mjs index 8da2d62..7d29f68 100644 --- a/src/mcp.mjs +++ b/src/mcp.mjs @@ -5,7 +5,7 @@ import { ENGINES, isInstalled, ranOk, runCmd } from "./engines.mjs"; import { isIP } from "node:net"; // Coding engines that can register MCP servers. Aider has no MCP support. -export const MCP_ENGINES = ["claude", "gemini", "codex", "opencode", "privacycode"]; +export const MCP_ENGINES = ["claude", "gemini", "qwen", "codex", "opencode", "privacycode"]; /** Is this target a remote server URL (vs a local stdio command)? */ export function isRemoteTarget(target) { @@ -71,7 +71,11 @@ export function mcpAddArgs(key, spec) { else argv.push("--", target, ...args); return { argv }; } - case "gemini": { + // Qwen Code is a Gemini CLI fork and kept the whole `mcp add` surface — + // same `-s/-t/-e/-H` flags, same "URL or command" positional. It shares the + // builder rather than getting a copy, so the two can only drift on purpose. + case "gemini": + case "qwen": { const argv = ["mcp", "add", "-s", "user"]; if (remote) argv.push("-t", transport); for (const [k, v] of env) argv.push("-e", `${k}=${v}`); @@ -138,9 +142,27 @@ export function planMcpAdd(spec, { installedSet } = {}) { }); } +/** + * Did this engine exit non-zero only because the server was already there? + * + * Registering the same server twice is the normal way to re-run `mcp install`, + * and it is not a failure — but Claude Code and Gemini/Qwen exit 1 on it, so the + * fan-out summary painted `claude ✗ failed (code 1)` next to opencode's cheerful + * green box. Read from a box where four engines already had the server, that + * says "moshcode cannot register with Claude Code" — which is exactly the wrong + * conclusion, and the reason this function exists rather than a nicer exit code. + * + * Matched against the engine's own words, so it stays honest: an engine that + * fails for any *other* reason still comes back failed. + */ +const ALREADY_RE = /already (?:exists|configured|registered|added)|exists in (?:user|global|project) config/i; +export function alreadyRegistered(r) { + return ALREADY_RE.test(String(r?.output ?? "")); +} + /** * Execute a plan: run each installed, non-skipped engine's `mcp add`. Returns - * results [{ key, status: "added"|"skipped"|"failed"|"not-installed", reason? }]. + * results [{ key, status: "added"|"already"|"skipped"|"failed"|"not-installed", reason? }]. * `run` is injectable for tests; defaults to the real spawner. */ export async function runMcpAdd(plan, { run = runCmd } = {}) { @@ -148,8 +170,11 @@ export async function runMcpAdd(plan, { run = runCmd } = {}) { for (const item of plan) { if (item.skip) { results.push({ key: item.key, status: "skipped", reason: item.skip }); continue; } if (!item.installed) { results.push({ key: item.key, status: "not-installed" }); continue; } - const r = await run(item.bin, item.argv); - results.push({ key: item.key, status: ranOk(r) ? "added" : "failed", code: r.code, signal: r.signal ?? null }); + // capture so a non-zero exit can be read for "already exists" rather than + // reported as a failure; the child's output still reaches the terminal. + const r = await run(item.bin, item.argv, { capture: true }); + const status = ranOk(r) ? "added" : alreadyRegistered(r) ? "already" : "failed"; + results.push({ key: item.key, status, code: r.code, signal: r.signal ?? null }); } return results; } diff --git a/test/mcp-add-fanout.test.mjs b/test/mcp-add-fanout.test.mjs index 5687e74..ccee865 100644 --- a/test/mcp-add-fanout.test.mjs +++ b/test/mcp-add-fanout.test.mjs @@ -7,7 +7,7 @@ import assert from "node:assert/strict"; import test from "node:test"; import { ENGINES } from "../src/engines.mjs"; -import { MCP_ENGINES, mcpAddArgs, planMcpAdd, runMcpAdd } from "../src/mcp.mjs"; +import { MCP_ENGINES, alreadyRegistered, mcpAddArgs, planMcpAdd, runMcpAdd } from "../src/mcp.mjs"; import { mcpTargetStatus } from "../src/integrations.mjs"; const REMOTE = { name: "sentry", target: "https://mcp.sentry.dev/mcp", args: [], env: [], headers: [] }; @@ -77,8 +77,13 @@ test("the fan-out and the /mcp list matrix name the same engines", () => { // --- controls: the fix must not over-report ---------------------------------- -test("MCP_ENGINES is unchanged — no engine gained MCP support", () => { - assert.deepEqual(MCP_ENGINES, ["claude", "gemini", "codex", "opencode", "privacycode"]); +test("MCP_ENGINES is the reviewed list — an engine only joins on purpose", () => { + // qwen joined deliberately: Qwen Code is a Gemini CLI fork and shipped the + // whole `qwen mcp add` surface, verified against its own --help. Everything + // else here is unchanged. This stays a pinned list rather than something + // derived from ENGINES, because "can moshcode register a server here" is a + // claim someone has to check against a real CLI, not infer from a roster. + assert.deepEqual(MCP_ENGINES, ["claude", "gemini", "qwen", "codex", "opencode", "privacycode"]); }); test("kimi is skipped for the reason that actually applies to it", () => { @@ -108,6 +113,29 @@ test("gemini's argv is byte-identical", () => { assert.deepEqual(plan.gemini.argv, ["mcp", "add", "-s", "user", "-t", "http", "sentry", "https://mcp.sentry.dev/mcp"]); }); +test("qwen's argv matches gemini's — it is the same CLI surface", () => { + const plan = byKey(planMcpAdd(REMOTE, { installedSet: new Set() })); + assert.deepEqual(plan.qwen.argv, ["mcp", "add", "-s", "user", "-t", "http", "sentry", "https://mcp.sentry.dev/mcp"]); + assert.deepEqual(plan.qwen.argv, plan.gemini.argv); + assert.equal(plan.qwen.skip, undefined, "qwen registers servers; it must not be skipped"); +}); + +test("qwen carries env and headers through, like gemini", () => { + const spec = { ...REMOTE, env: [["TOKEN", "z"]], headers: ["X-Api-Key: abc"] }; + const plan = byKey(planMcpAdd(spec, { installedSet: new Set() })); + assert.deepEqual(plan.qwen.argv, [ + "mcp", "add", "-s", "user", "-t", "http", + "-e", "TOKEN=z", "-H", "X-Api-Key: abc", + "sentry", "https://mcp.sentry.dev/mcp", + ]); +}); + +test("qwen takes a stdio command server too", () => { + const stdio = { name: "my-tools", target: "npx", args: ["-y", "my-mcp-server"], env: [], headers: [] }; + const plan = byKey(planMcpAdd(stdio, { installedSet: new Set() })); + assert.deepEqual(plan.qwen.argv, ["mcp", "add", "-s", "user", "my-tools", "npx", "-y", "my-mcp-server"]); +}); + test("codex's and opencode's argv are byte-identical", () => { const plan = byKey(planMcpAdd(REMOTE, { installedSet: new Set() })); assert.deepEqual(plan.codex.argv, ["mcp", "add", "sentry", "--url", "https://mcp.sentry.dev/mcp"]); @@ -159,6 +187,59 @@ test("a signal or non-zero exit still reports failed, not skipped", async () => assert.equal(results.gemini.status, "failed"); }); +// --- re-running an install is not a failure ---------------------------------- + +test("an engine that says the server already exists reports `already`, not failed", async () => { + // Claude Code exits 1 with "MCP server X already exists in user config". Read + // as a failure, that row said moshcode could not register with Claude Code — + // when in fact it already had. + const plan = planMcpAdd(REMOTE, { installedSet: new Set(["claude"]) }); + const results = byKey(await runMcpAdd(plan, { + run: async () => ({ ok: true, code: 1, output: "MCP server sentry already exists in user config\n" }), + })); + assert.equal(results.claude.status, "already"); +}); + +test("the same wording is recognised from each engine that uses it", () => { + for (const words of [ + "MCP server sentry already exists in user config", // claude + "Server \"sentry\" is already configured", // gemini / qwen + "server already registered", + "sentry already added", + ]) { + assert.equal(alreadyRegistered({ code: 1, output: words }), true, words); + } +}); + +test("a failure for any other reason is still a failure", async () => { + const plan = planMcpAdd(REMOTE, { installedSet: new Set(["claude"]) }); + const results = byKey(await runMcpAdd(plan, { + run: async () => ({ ok: true, code: 1, output: "error: connection refused\n" }), + })); + assert.equal(results.claude.status, "failed"); + assert.equal(results.claude.code, 1); + // and no output at all must never be read as "already there" + assert.equal(alreadyRegistered({ code: 1 }), false); + assert.equal(alreadyRegistered({ code: 1, output: "" }), false); +}); + +test("a zero exit is `added` even if the word `already` appears in the noise", async () => { + const plan = planMcpAdd(REMOTE, { installedSet: new Set(["claude"]) }); + const results = byKey(await runMcpAdd(plan, { + run: async () => ({ ok: true, code: 0, output: "note: sentry already exists upstream\n" }), + })); + assert.equal(results.claude.status, "added", "a successful add is an add"); +}); + +test("runMcpAdd asks for captured output — it cannot classify what it cannot read", async () => { + const seen = []; + const plan = planMcpAdd(REMOTE, { installedSet: new Set(["claude"]) }); + await runMcpAdd(plan, { + run: async (bin, argv, opts) => { seen.push(opts); return { ok: true, code: 0 }; }, + }); + assert.deepEqual(seen, [{ capture: true }]); +}); + test("mcpAddArgs itself is untouched for a supported and an unsupported key", () => { assert.equal(mcpAddArgs("aider", REMOTE).skip, "no MCP support"); assert.deepEqual(mcpAddArgs("codex", REMOTE).argv, ["mcp", "add", "sentry", "--url", "https://mcp.sentry.dev/mcp"]); diff --git a/test/mcp-stray-flag.test.mjs b/test/mcp-stray-flag.test.mjs index 52ce561..3639c91 100644 --- a/test/mcp-stray-flag.test.mjs +++ b/test/mcp-stray-flag.test.mjs @@ -120,3 +120,66 @@ test("an unknown verb still reports an unknown verb, not an unknown flag", () => test("a stdio install with no name still asks for --name", () => { assert.match(parseMcp(["install", "--", "npx", "srv"]).error, /explicit --name/); }); + +// ---------- flags AFTER a remote URL were swallowed too ---------- +// +// The guard above only inspected the server name and the target. A flag typed +// *after* a remote URL landed in `args`, which every engine's remote builder +// discards — so it vanished with no error and the install went ahead. The case +// that bites is `--dry-run`, a flag mcp does not have: someone expecting a +// preview got five engines' config files written instead. + +test("an unknown flag after a remote URL is rejected, not silently dropped", () => { + const { error, spec } = parseMcp(["install", "https://mcp.example.com", "--dry-run"]); + assert.equal(spec, undefined, "the install must not proceed"); + assert.match(error, /unknown mcp flag "--dry-run"/); + assert.match(error, /no --dry-run/); +}); + +test("the first stray flag is the one named, whichever follows it", () => { + const { error } = parseMcp(["install", "https://mcp.example.com", "--wat", "--dry-run"]); + assert.match(error, /unknown mcp flag "--wat"/); +}); + +test("a stray non-flag argument after a remote URL is reported as unexpected", () => { + const { error, spec } = parseMcp(["install", "https://mcp.example.com", "extra"]); + assert.equal(spec, undefined); + assert.match(error, /unexpected argument "extra"/); +}); + +test("the same guard applies to `mcp add`, not just `install`", () => { + const { error } = parseMcp(["add", "sentry", "https://mcp.sentry.dev/mcp", "--dry-run"]); + assert.match(error, /unknown mcp flag "--dry-run"/); +}); + +test("a legitimate remote install is unaffected", () => { + const { error, spec } = parseMcp(["install", "https://mcp.sentry.dev/mcp"]); + assert.equal(error, undefined); + assert.equal(spec.name, "sentry"); + assert.deepEqual(spec.args, []); +}); + +test("supported flags around a remote URL still parse", () => { + const { error, spec } = parseMcp([ + "install", "https://mcp.example.com", "-H", "Authorization: Bearer z", "-e", "TOKEN=z", "--name", "mine", + ]); + assert.equal(error, undefined); + assert.equal(spec.name, "mine"); + assert.deepEqual(spec.headers, ["Authorization: Bearer z"]); + assert.deepEqual(spec.env, [["TOKEN", "z"]]); +}); + +test("a stdio command's own flags after `--` are still not second-guessed", () => { + const { error, spec } = parseMcp(["add", "mine", "--", "npx", "-y", "my-mcp-server", "--dry-run"]); + assert.equal(error, undefined); + assert.equal(spec.target, "npx"); + assert.deepEqual(spec.args, ["-y", "my-mcp-server", "--dry-run"]); +}); + +test("a stdio server given without `--` keeps its arguments", () => { + // Not a remote target, so the discard guard must not fire: these args really + // are spliced into the engine's argv and do reach the command. + const { error, spec } = parseMcp(["add", "mine", "npx", "my-mcp-server"]); + assert.equal(error, undefined); + assert.deepEqual(spec.args, ["my-mcp-server"]); +});