mcp: register with qwen too, and stop calling a re-install a failure

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.

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 }));
   });
 }
 

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 <url> --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}`)));

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,18 +142,39 @@ 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 } = {}) {
   const results = [];
   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;
 }

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"]);

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"]);
+});