diff --git a/docs/cli.md b/docs/cli.md index 6e5c556d1c..bb5fe36030 100644 --- a/docs/cli.md +++ b/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 --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 diff --git a/src/cli.ts b/src/cli.ts index e9050e7682..9f2520948d 100644 --- a/src/cli.ts +++ b/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", diff --git a/src/cliBuiltinArguments.ts b/src/cliBuiltinArguments.ts index 54bcd0d37c..6b21088995 100644 --- a/src/cliBuiltinArguments.ts +++ b/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 = 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; +} => { 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"); }; diff --git a/tests/boundary/cli/cliBuiltinArguments.test.ts b/tests/boundary/cli/cliBuiltinArguments.test.ts index 598b86e6c5..1562be6d57 100644 --- a/tests/boundary/cli/cliBuiltinArguments.test.ts +++ b/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"); } }); diff --git a/tests/boundary/cli/defaultAmpRegistration.test.ts b/tests/boundary/cli/defaultAmpRegistration.test.ts index e52271e21a..65e2cdc86b 100644 --- a/tests/boundary/cli/defaultAmpRegistration.test.ts +++ b/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(""); + } + }, +); diff --git a/tests/boundary/cli/mcpAddOptionValidation.test.ts b/tests/boundary/cli/mcpAddOptionValidation.test.ts index f9ef555588..336be78001 100644 --- a/tests/boundary/cli/mcpAddOptionValidation.test.ts +++ b/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"); + } + }, +);