fix(cli): validate MCP registration before side effects (#9057)

Fixes #7375. Fixes #7608.

Share argument consumption and agent facts between admission and scope checks; reject invalid registrations before settings, backups or runner startup. Preserve missing-value priority, explicit redaction, discovery and valid registration controls.

Validated 33 focused tests (one platform skip), current-main RED and independent compiled-CLI before/after workflows; check:fast passes.

Full technical CI 42297438912 passes, including all eight shards and required aggregates. Greptile reviews this head at 5/5; no completed Codex review is available and the bot reports exhausted quota. The original author readiness declarations remain verbatim; no new human-review attestation is added.

Assisted-by: Reimu (gensokyo-skills:reimu-incident-triage)

docs/cli.md

@@ -1200,6 +1200,10 @@ such as `UNSUPPORTED_OUTPUT_COMBINATION` for `--json` with `--token-limit`. Use
 Commands reject surplus positional arguments before execution, including commands
 that accept no positionals. Use `rea <command> --help` for the declared argument
 shape; a final variadic argument continues to accept multiple values.
+`rea mcp add` accepts options only and rejects unknown options before reading or
+backing up client settings or starting registration. Its `--agent` and
+`--command`/`-c` options accept spaced and equals forms. Values must be nonempty; a following flag does not satisfy a spaced option's
+missing value.
 
 `rea mcp doctor --json` checks production MCP startup and reports child logs in
 `diagnostics`. It drains stderr while retaining at most its first 64 KiB of raw

src/cli.ts

@@ -25,11 +25,13 @@ import { registerBinaryDiagnosticsCommands } from "./cli/binaryDiagnosticsComman
 import { registerFridaCommands } from "./cli/fridaCommands.js";
 import type { FridaInstrumentationPort } from "./application/frida/FridaInstrumentationPort.js";
 import { Cli, z } from "incur";
+import { redactExplicitText } from "./domain/explicitSensitiveValues.js";
 import {
   adaptCliOutputArgumentsForIncur,
   classifyMcpAddDiscovery,
   expandMcpAddEqualsArguments,
-  mcpAddAgents,
+  declaredSensitiveValues,
+  readMcpAddArguments,
   normalizeCliArguments,
   splitIncurBuiltinArguments,
 } from "./cliBuiltinArguments.js";
@@ -311,37 +313,21 @@ export const createCli = (
         )(1);
         return Promise.resolve();
       }
-      // Incur's `mcp add` silently skips a valued option with no value and
-      // swallows the next flag as the value: the registration succeeds without
-      // the requested agent or command. Reject a trailing --agent/--command/-c,
-      // a value position occupied by another flag, and an empty `--flag=` form
-      // up front.
-      const valued = ["--agent", "--command", "-c"];
-      let missingValue: string | undefined;
-      for (let index = 0; index < command.length; index += 1) {
-        const next = command[index + 1];
-        if (
-          valued.includes(command[index]!) &&
-          (next === undefined || next.startsWith("-"))
-        ) {
-          missingValue = command[index];
-          break;
-        }
-      }
-      const emptyValue = command.find(
-        (token) =>
-          (token.startsWith("--agent=") ||
-            token.startsWith("--command=") ||
-            token.startsWith("-c=")) &&
-          token.length === token.indexOf("=") + 1,
+      // Validate before reading settings, creating a backup or invoking Incur's
+      // writer. The same owner supplies the agent selection used below.
+      const parsedRegistration = readMcpAddArguments(
+        command.slice(builtin + 2),
       );
-      if (missingValue !== undefined || emptyValue !== undefined) {
-        const flag = missingValue ?? emptyValue!;
-        const message = `Missing value for flag: ${flag.replace(/=.*$/, "")}`;
+      if (!parsedRegistration.validation.ok) {
+        const failure = parsedRegistration.validation.error;
+        const message = redactExplicitText(
+          failure.kind === "missing_value"
+            ? `Missing value for flag: ${failure.flag}`
+            : `Unexpected argument: ${failure.token}`,
+          declaredSensitiveValues(command),
+        );
         const field =
-          flag.includes("command") || flag === "-c" || flag.startsWith("-c=")
-            ? "command"
-            : "agent";
+          failure.kind === "missing_value" ? failure.field : "arguments";
         const output = parseCliOutputArguments(normalized);
         if (!output.validation.ok)
           write(renderCliOutputArgumentError(output.validation));
@@ -390,8 +376,7 @@ export const createCli = (
       // non-Amp equals forms. Incur consumes command values before agent flags.
       normalized = serveArguments;
       const registration = splitIncurBuiltinArguments(serveArguments);
-      const rest = registration.command.slice(builtin + 2);
-      const agents = mcpAddAgents(rest);
+      const { agents } = parsedRegistration;
       const ampRequested =
         ![
           "--help",

src/cliBuiltinArguments.ts

@@ -1,3 +1,5 @@
+import { err, ok, type Result } from "./domain/result.js";
+
 /** Global output flags that take a value. */
 export const VALUED_OUTPUT_FLAGS: ReadonlySet<string> = new Set([
   "--format",
@@ -214,7 +216,11 @@ export const expandMcpAddEqualsArguments = (
   arguments_: readonly string[],
 ): string[] =>
   arguments_.flatMap((token) => {
-    if (token.startsWith("--agent=") || token.startsWith("--command=")) {
+    if (
+      token.startsWith("--agent=") ||
+      token.startsWith("--command=") ||
+      token.startsWith("-c=")
+    ) {
       const separator = token.indexOf("=");
       const value = token.slice(separator + 1);
       return value === "" ? [token] : [token.slice(0, separator), value];
@@ -222,26 +228,55 @@ export const expandMcpAddEqualsArguments = (
     return [token];
   });
 
+type McpAddArgumentError =
+  | {
+      readonly kind: "missing_value";
+      readonly flag: string;
+      readonly field: "agent" | "command";
+    }
+  | { readonly kind: "unexpected_argument"; readonly token: string };
+
 /**
- * Agent selection for an `mcp add` command, after equals-form expansion.
- * Single owner: the project-scope guard and the `mcp add` dispatch in
- * `cli.ts` both consume this. Incur consumes command values before agent
- * flags, so a `--command` value is never read as an agent.
+ * Own token consumption for the pinned registration builtin. Agent selection
+ * remains observable even for invalid input so the launcher's scope refusal
+ * and dispatch cannot interpret a command value as an agent selector.
  */
-export const mcpAddAgents = (
+export const readMcpAddArguments = (
   command: IncurBuiltinArgumentSplit["command"],
-): string[] => {
+): {
+  readonly agents: readonly string[];
+  readonly validation: Result<void, McpAddArgumentError>;
+} => {
   const agents: string[] = [];
+  let missing: McpAddArgumentError | undefined;
+  let unexpected: McpAddArgumentError | undefined;
   for (let index = 0; index < command.length; index += 1) {
-    if (
-      (command[index] === "--command" || command[index] === "-c") &&
-      command[index + 1]
-    )
-      index += 1;
-    else if (command[index] === "--agent" && command[index + 1])
-      agents.push(command[++index]!);
+    const token = command[index]!;
+    const flag = token.split("=", 1)[0]!;
+    if (flag === "--agent" || flag === "--command" || flag === "-c") {
+      const equals = token.indexOf("=");
+      const value = equals < 0 ? command[++index] : token.slice(equals + 1);
+      // Spaced flags cannot satisfy another option's missing value. Equals
+      // values retain Incur's existing expansion/discovery behavior.
+      if (
+        value === undefined ||
+        value === "" ||
+        (equals < 0 && value.startsWith("-"))
+      )
+        missing ??= {
+          kind: "missing_value",
+          flag,
+          field: flag === "--agent" ? "agent" : "command",
+        };
+      if (flag === "--agent" && value) agents.push(value);
+    } else if (token !== "--no-global")
+      unexpected ??= { kind: "unexpected_argument", token };
   }
-  return agents;
+  const failure = missing ?? unexpected;
+  return {
+    agents,
+    validation: failure === undefined ? ok(undefined) : err(failure),
+  };
 };
 
 /**
@@ -278,7 +313,10 @@ export const classifyMcpAddDiscovery = (
     flags.has("--help") ||
     flags.has("-h") ||
     flags.has("--llms") ||
-    flags.has("--llms-full")
+    flags.has("--llms-full") ||
+    flags.has("--mcp") ||
+    flags.has("--update") ||
+    flags.has("--incur-update-check")
   )
     return "bypass";
   if (flags.has("--version")) return "version";
@@ -296,7 +334,7 @@ export const isProjectScopedAmpRegistration = (
   if (classifyMcpAddDiscovery(flags) !== undefined) return false;
   const rest = command.slice(2);
   if (!rest.includes("--no-global")) return false;
-  const agents = mcpAddAgents(rest);
+  const { agents } = readMcpAddArguments(rest);
   return agents.length === 0 || agents.includes("amp");
 };
 

tests/boundary/cli/cliBuiltinArguments.test.ts

@@ -13,7 +13,15 @@ import {
 
 describe("mcp add discovery classification", () => {
   it("bypasses help and listing flags", () => {
-    for (const flag of ["--help", "-h", "--llms", "--llms-full"]) {
+    for (const flag of [
+      "--help",
+      "-h",
+      "--llms",
+      "--llms-full",
+      "--mcp",
+      "--update",
+      "--incur-update-check",
+    ]) {
       expect(classifyMcpAddDiscovery(new Set([flag]))).toBe("bypass");
     }
   });

tests/boundary/cli/defaultAmpRegistration.test.ts

@@ -375,3 +375,37 @@ cliTest(
     }
   },
 );
+
+cliTest(
+  "invalid registration never starts a package runner or changes settings",
+  async ({ cli }) => {
+    const f = await fixture(original);
+    const before = await stat(f.settings);
+    for (const options of [
+      ["--bogus"],
+      ["--agents", "claude-code"],
+      ["--unknown=declared-literal", "--sensitive-value", "declared-literal"],
+      ["--unknown=API_SECRET_ordinary-evidence"],
+    ]) {
+      const result = await cli.run({
+        arguments: ["mcp", "add", ...options, "--json"],
+        cwd: f.home,
+        environment: f.environment,
+        timeoutMs: 20_000,
+      });
+      expect(result.exitCode, result.stderr).toBe(1);
+      expect(result.json).toMatchObject({
+        code: "VALIDATION_ERROR",
+        fieldErrors: [expect.objectContaining({ path: "arguments" })],
+      });
+      if (options.includes("--sensitive-value")) {
+        expect(result.stdout + result.stderr).not.toContain("declared-literal");
+        expect(result.stdout).toContain("[REDACTED]");
+      } else expect(result.stdout).toContain(options[0]);
+      expect(await readFile(f.settings, "utf8")).toBe(original);
+      expect((await stat(f.settings)).mtimeMs).toBe(before.mtimeMs);
+      expect(await readdir(f.directory)).toEqual(["settings.json"]);
+      expect(await readFile(f.log, "utf8")).toBe("");
+    }
+  },
+);

tests/boundary/cli/mcpAddOptionValidation.test.ts

@@ -1,4 +1,11 @@
-import { access, mkdir, readFile, writeFile } from "node:fs/promises";
+import {
+  access,
+  mkdir,
+  readFile,
+  readdir,
+  stat,
+  writeFile,
+} from "node:fs/promises";
 import { join } from "node:path";
 import { expect } from "vitest";
 
@@ -89,3 +96,119 @@ cliTest("mcp add rejects valued options without a value", async ({ cli }) => {
     command: expect.any(String),
   });
 });
+
+cliTest(
+  "mcp add rejects an argument that is neither an option nor a value",
+  async ({ cli }) => {
+    const root = await createTestTempDirectory("rea-mcp-add-argument-");
+    const home = join(root, "home");
+    const settingsPath = join(home, ".config", "amp", "settings.json");
+    const run = (arguments_: readonly string[]) =>
+      cli.run({
+        arguments: [...arguments_, "--json"],
+        cwd: root,
+        environment: {
+          HOME: home,
+          USERPROFILE: home,
+          XDG_CONFIG_HOME: join(home, ".config"),
+        },
+        timeoutMs: 20_000,
+      });
+
+    // Incur skips a stray word, so the registration used to succeed for a request
+    // REA cannot honor and wrote the settings file anyway.
+    for (const arguments_ of [
+      ["mcp", "add", "extra", "--agent", "amp"],
+      ["mcp", "add", "--agent", "amp", "extra"],
+      ["mcp", "add", "--agent=amp", "extra"],
+      ["mcp", "add", "--bogus", "--agent", "amp"],
+    ] as const) {
+      const result = await run(arguments_);
+      expect(result.exitCode, result.stderr).toBe(1);
+      expect(result.json).toMatchObject({
+        code: "VALIDATION_ERROR",
+        message: expect.stringMatching(/Unexpected argument: (extra|--bogus)/),
+        fieldErrors: expect.arrayContaining([
+          expect.objectContaining({
+            code: "invalid_value",
+            missing: false,
+            path: expect.stringMatching(/arguments|command/),
+          }),
+        ]),
+      });
+      await expect(access(settingsPath)).rejects.toMatchObject({
+        code: "ENOENT",
+      });
+    }
+
+    const original =
+      '{"amp.mcpServers":{"rea":{"command":"old","env":{"KEEP":"1"}}},"theme":"dark"}\n';
+    await mkdir(join(home, ".config", "amp"), { recursive: true });
+    await writeFile(settingsPath, original);
+    const before = await stat(settingsPath);
+    const rejected = await run(["mcp", "add", "extra", "--agent=amp"]);
+    expect(rejected.exitCode, rejected.stderr).toBe(1);
+    expect(await readFile(settingsPath, "utf8")).toBe(original);
+    expect((await stat(settingsPath)).mtimeMs).toBe(before.mtimeMs);
+    expect(await readdir(join(home, ".config", "amp"))).toEqual([
+      "settings.json",
+    ]);
+
+    // Verify the actual stored command, rather than an already-existing file.
+    for (const options of [
+      ["--agent", "amp", "--command", "node --version"],
+      ["--agent=amp", "--command=node --version"],
+      ["--agent=amp", "-c=node --version"],
+    ]) {
+      const registered = await run(["mcp", "add", ...options]);
+      expect(registered.exitCode, registered.stderr).toBe(0);
+      expect(JSON.parse(await readFile(settingsPath, "utf8"))).toEqual({
+        "amp.mcpServers": {
+          rea: { command: "node", args: ["--version"], env: { KEEP: "1" } },
+        },
+        theme: "dark",
+      });
+    }
+    expect(await readFile(`${settingsPath}.rea.backup`, "utf8")).toBe(original);
+  },
+);
+
+cliTest(
+  "mcp add names the option the reported error is about",
+  async ({ cli }) => {
+    const root = await createTestTempDirectory("rea-mcp-add-field-");
+    const home = join(root, "home");
+    const run = (arguments_: readonly string[]) =>
+      cli.run({
+        arguments: [...arguments_, "--json"],
+        cwd: root,
+        environment: {
+          HOME: home,
+          USERPROFILE: home,
+          XDG_CONFIG_HOME: join(home, ".config"),
+        },
+        timeoutMs: 20_000,
+      });
+
+    // A stray word alongside a missing value: the message describes the missing
+    // flag, so `path` must name that option rather than the stray argument.
+    for (const [arguments_, field] of [
+      [["mcp", "add", "extra", "--command"], "command"],
+      [["mcp", "add", "extra", "-c"], "command"],
+      [["mcp", "add", "extra", "--agent"], "agent"],
+    ] as const) {
+      const result = await run(arguments_);
+      expect(result.exitCode, result.stderr).toBe(1);
+      expect(result.json).toMatchObject({
+        code: "VALIDATION_ERROR",
+        fieldErrors: [
+          expect.objectContaining({ code: "invalid_value", path: field }),
+        ],
+      });
+      expect(
+        (result.json as { message?: string }).message,
+        "the message and the field must describe the same error",
+      ).toContain("Missing value for flag");
+    }
+  },
+);