fix(cli): preserve Amp MCP registration settings (#7995)

* fix(cli): preserve Amp registration environment and report failures (#7538, #7536)

* fix(cli): normalize equals-form mcp add agents before Incur parses them (#7538)

* fix(cli): remove the Amp settings backup after a verified registration (#7538)

* fix(cli): secure Amp registration backups

* test(cli): cover Amp registration backup permissions

* fix(cli): retain Amp registration recovery backups

* fix(cli): create private Amp recovery copies

* test(cli): fix Amp backup assertion typing

* fix(cli): preserve typed Amp backup failures

* fix(cli): handle Amp backup cleanup failures

src/cli.ts

@@ -1,3 +1,7 @@
+import { existsSync, readFileSync, writeFileSync } from "node:fs";
+import { homedir } from "node:os";
+import { join } from "node:path";
+import { writeAmpRegistrationBackup } from "./cli/ampRegistrationBackup.js";
 import { registerGpuCommands } from "./cli/gpuCommands.js";
 import { registerNativeFunctionMatchingCommands } from "./cli/nativeFunctionMatchingCommands.js";
 import { registerCaptureCommands } from "./cli/captureCommands.js";
@@ -264,6 +268,214 @@ export const createCli = (
         )(1);
         return Promise.resolve();
       }
+      // Incur's mcp add parser only understands spaced `--agent amp` forms; an
+      // equals form like `--agent=amp` is silently ignored, which would drop
+      // the requested agent and register against the default target. Normalize
+      // equals forms back to spaced forms before handing over to Incur.
+      const serveArguments = normalized.flatMap((token) => {
+        if (token.startsWith("--agent=") || token.startsWith("--command=")) {
+          const separator = token.indexOf("=");
+          const value = token.slice(separator + 1);
+          return value === "" ? [token] : [token.slice(0, separator), value];
+        }
+        return [token];
+      });
+      // Incur's Amp registration overwrites an existing rea entry (dropping its
+      // environment map) and reports success even when the settings file
+      // prevents a save. Guard that path: reject a settings file REA cannot
+      // update, back up the file, preserve the caller's environment, and verify
+      // the registration landed.
+      const ampRequested =
+        command.some(
+          (token, index) => token === "--agent" && command[index + 1] === "amp",
+        ) || command.includes("--agent=amp");
+      if (ampRequested) {
+        const settingsPath = join(homedir(), ".config", "amp", "settings.json");
+        let priorEnv: unknown;
+        let guardError: string | undefined;
+        let settingsText: string | undefined;
+        let parsedSettings: Record<string, unknown> | undefined;
+        try {
+          if (existsSync(settingsPath))
+            settingsText = readFileSync(settingsPath, "utf8");
+        } catch (cause) {
+          guardError = `Unable to read Amp settings.json: ${
+            cause instanceof Error ? cause.message : String(cause)
+          }`;
+        }
+        if (settingsText !== undefined) {
+          try {
+            const parsed: unknown = JSON.parse(settingsText);
+            if (
+              typeof parsed !== "object" ||
+              parsed === null ||
+              Array.isArray(parsed)
+            )
+              guardError = "Amp settings.json must contain a JSON object.";
+            else parsedSettings = parsed as Record<string, unknown>;
+          } catch (cause) {
+            guardError = `Amp settings.json is not valid JSON: ${
+              cause instanceof Error ? cause.message : String(cause)
+            }`;
+          }
+        }
+        if (parsedSettings !== undefined && settingsText !== undefined) {
+          const servers = parsedSettings["amp.mcpServers"];
+          if (
+            servers !== undefined &&
+            (typeof servers !== "object" ||
+              servers === null ||
+              Array.isArray(servers))
+          )
+            guardError =
+              "amp.mcpServers must be an object in Amp settings.json.";
+          else {
+            const entry =
+              typeof servers === "object" && servers !== null
+                ? (servers as Record<string, unknown>)["rea"]
+                : undefined;
+            if (typeof entry === "object" && entry !== null)
+              priorEnv = (entry as Record<string, unknown>)["env"];
+          }
+        }
+        const reportValidationError = (message: string): void => {
+          const output = parseCliOutputArguments(normalized);
+          if (!output.validation.ok)
+            write(renderCliOutputArgumentError(output.validation));
+          else
+            write(
+              renderCliDocument(
+                {
+                  code: "VALIDATION_ERROR",
+                  message,
+                  fieldErrors: [
+                    {
+                      code: "invalid_value",
+                      missing: false,
+                      path: "amp",
+                      message,
+                    },
+                  ],
+                },
+                output.format,
+                process.stdout.isTTY === true,
+              ),
+            );
+          (
+            options?.exit ??
+            ((code: number) => {
+              process.exitCode = code;
+            })
+          )(1);
+        };
+        if (guardError !== undefined) {
+          reportValidationError(`${guardError} Fix it before registering.`);
+          return Promise.resolve();
+        }
+
+        const runRegistration = (backupPath?: string): Promise<void> => {
+          const result = serveStrict(
+            serveArguments,
+            options ?? {},
+            (tokens, selected) =>
+              serveDeclared(tokens, selected, (declared, declaredOptions) =>
+                serve(
+                  adaptCliOutputArgumentsForIncur(declared),
+                  declaredOptions,
+                ),
+              ),
+          );
+          return result.then(() => {
+            let failure: string | undefined;
+            try {
+              if (!existsSync(settingsPath))
+                failure = "Amp settings.json was not created.";
+              else {
+                const text = readFileSync(settingsPath, "utf8");
+                const parsed = JSON.parse(text) as Record<string, unknown>;
+                const servers = parsed["amp.mcpServers"];
+                if (
+                  typeof servers !== "object" ||
+                  servers === null ||
+                  Array.isArray(servers) ||
+                  !("rea" in servers)
+                )
+                  failure =
+                    "Amp settings.json does not contain the rea registration after mcp add.";
+                else {
+                  const entry = (servers as Record<string, unknown>)["rea"];
+                  if (
+                    priorEnv !== undefined &&
+                    typeof entry === "object" &&
+                    entry !== null &&
+                    (entry as Record<string, unknown>)["env"] === undefined
+                  ) {
+                    (entry as Record<string, unknown>)["env"] = priorEnv;
+                    writeFileSync(
+                      settingsPath,
+                      `${JSON.stringify(parsed, null, 2)}\n`,
+                    );
+                  }
+                }
+              }
+            } catch (cause) {
+              failure = cause instanceof Error ? cause.message : String(cause);
+            }
+            if (failure !== undefined) {
+              const output = parseCliOutputArguments(normalized);
+              if (output.validation.ok)
+                write(
+                  renderCliDocument(
+                    {
+                      code: "VALIDATION_ERROR",
+                      message: failure,
+                      fieldErrors: [
+                        {
+                          code: "invalid_value",
+                          missing: false,
+                          path: "amp",
+                          message: failure,
+                        },
+                      ],
+                    },
+                    output.format,
+                    process.stdout.isTTY === true,
+                  ),
+                );
+              (
+                options?.exit ??
+                ((code: number) => {
+                  process.exitCode = code;
+                })
+              )(1);
+            } else if (backupPath !== undefined)
+              process.stderr.write(
+                `Amp MCP registration backup retained at ${backupPath}\n`,
+              );
+          });
+        };
+
+        if (settingsText === undefined) return runRegistration();
+        return writeAmpRegistrationBackup(settingsPath, settingsText).then(
+          (backup) => {
+            if (!backup.ok) {
+              const { kind, path, detail } = backup.error;
+              reportValidationError(
+                `Unable to create Amp settings backup (${kind}) at ${path}: ${detail}`,
+              );
+              return;
+            }
+            return runRegistration(backup.value);
+          },
+          (cause: unknown) => {
+            const detail =
+              cause instanceof Error ? cause.message : String(cause);
+            reportValidationError(
+              `Unable to create Amp settings backup: ${detail}`,
+            );
+          },
+        );
+      }
     }
     const result = serveStrict(normalized, options ?? {}, (tokens, selected) =>
       serveDeclared(tokens, selected, (declared, declaredOptions) =>

src/cli/ampRegistrationBackup.ts

@@ -0,0 +1,78 @@
+import { chmodSync, lstatSync } from "node:fs";
+import { readFile, writeFile } from "node:fs/promises";
+
+import {
+  preserveCurrentRecoveryBackup,
+  type RecoveryBackupPreservationFailure,
+} from "../application/SetupRecoveryBackup.js";
+import { err, ok, type Result } from "../domain/result.js";
+
+const errorDetail = (cause: unknown): string =>
+  cause instanceof Error ? cause.message : String(cause);
+
+const isMissing = (cause: unknown): boolean =>
+  typeof cause === "object" &&
+  cause !== null &&
+  "code" in cause &&
+  cause.code === "ENOENT";
+
+const restrictExistingBackup = (
+  backupPath: string,
+): Result<void, RecoveryBackupPreservationFailure> => {
+  let existing: ReturnType<typeof lstatSync>;
+  try {
+    existing = lstatSync(backupPath);
+  } catch (cause: unknown) {
+    if (isMissing(cause)) return ok(undefined);
+    return err({
+      kind: "unreadable",
+      path: backupPath,
+      detail: `The existing Amp settings backup could not be checked: ${errorDetail(cause)}`,
+    });
+  }
+  if (!existing.isFile())
+    return err({
+      kind: "occupied",
+      path: backupPath,
+      detail: "The existing Amp settings backup is not a regular file.",
+    });
+  try {
+    chmodSync(backupPath, 0o600);
+  } catch (cause: unknown) {
+    return err({
+      kind: "unreadable",
+      path: backupPath,
+      detail: `The existing Amp settings backup permissions could not be secured: ${errorDetail(cause)}`,
+    });
+  }
+  return ok(undefined);
+};
+
+const copyPrivateBackup = async (
+  sourcePath: string,
+  backupPath: string,
+): Promise<void> => {
+  await writeFile(backupPath, await readFile(sourcePath), {
+    flag: "wx",
+    mode: 0o600,
+  });
+};
+
+export const writeAmpRegistrationBackup = async (
+  settingsPath: string,
+  settingsText: string,
+): Promise<Result<string, RecoveryBackupPreservationFailure>> => {
+  const firstBackupPath = `${settingsPath}.rea.backup`;
+  const firstRestricted = restrictExistingBackup(firstBackupPath);
+  if (!firstRestricted.ok) return firstRestricted;
+  const preserved = await preserveCurrentRecoveryBackup(
+    firstBackupPath,
+    settingsPath,
+    settingsText,
+    { copy: copyPrivateBackup },
+  );
+  if (!preserved.ok) return preserved;
+  const selectedRestricted = restrictExistingBackup(preserved.value);
+  if (!selectedRestricted.ok) return selectedRestricted;
+  return ok(preserved.value);
+};

tests/boundary/cli/ampRegistrationPreservation.test.ts

@@ -0,0 +1,227 @@
+import { chmod, mkdir, readFile, stat, writeFile } from "node:fs/promises";
+import { join } from "node:path";
+import { expect, it } from "vitest";
+
+import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js";
+import { writeAmpRegistrationBackup } from "../../../src/cli/ampRegistrationBackup.js";
+import { cliTest } from "../../support/cli/cliFixture.js";
+
+const ampSettings = (home: string) =>
+  join(home, ".config", "amp", "settings.json");
+
+cliTest(
+  "mcp add preserves Amp environment and reports unreadable settings",
+  async ({ cli }) => {
+    const root = await createTestTempDirectory("rea-amp-registration-");
+    const home = join(root, "home");
+    const environment = {
+      HOME: home,
+      USERPROFILE: home,
+    };
+    const run = (arguments_: readonly string[]) =>
+      cli.run({
+        arguments: [...arguments_, "--json"],
+        cwd: root,
+        environment,
+        timeoutMs: 20_000,
+      });
+
+    // An existing rea registration keeps its environment map and the file is
+    // backed up before the refresh.
+    const settingsPath = ampSettings(home);
+    const original = JSON.stringify(
+      {
+        "amp.mcpServers": {
+          rea: {
+            command: "npx",
+            args: ["-y", "rea-agents@6.8.0", "mcp"],
+            env: { KEEP_ME: "1" },
+          },
+          other: { command: "other" },
+        },
+        theme: "dark",
+      },
+      null,
+      2,
+    );
+    await mkdir(join(home, ".config", "amp"), { recursive: true });
+    await writeFile(settingsPath, original);
+
+    const refreshed = await run(["mcp", "add", "--agent", "amp"]);
+    expect(refreshed.exitCode, refreshed.stderr).toBe(0);
+    const after = JSON.parse(await readFile(settingsPath, "utf8"));
+    expect(after["amp.mcpServers"].rea).toMatchObject({
+      command: expect.any(String),
+      env: { KEEP_ME: "1" },
+    });
+    const registrationBackupPath = `${settingsPath}.rea.backup`;
+    expect(await readFile(registrationBackupPath, "utf8")).toBe(original);
+    expect(refreshed.stderr).toContain(registrationBackupPath);
+    expect(after.theme).toBe("dark");
+
+    // The equals form --agent=amp is parsed as a spaced form before Incur sees
+    // it, so the Amp registration lands and keeps the environment map.
+    const settingsBeforeEquals = await readFile(settingsPath, "utf8");
+    const equals = await run(["mcp", "add", "--agent=amp"]);
+    expect(equals.exitCode, equals.stderr).toBe(0);
+    const afterEquals = JSON.parse(await readFile(settingsPath, "utf8"));
+    expect(afterEquals["amp.mcpServers"].rea).toMatchObject({
+      command: expect.any(String),
+      env: { KEEP_ME: "1" },
+    });
+    expect(await readFile(registrationBackupPath, "utf8")).toBe(original);
+    const recoveryBackupPath = equals.stderr.match(
+      /Amp MCP registration backup retained at (.+)/,
+    )?.[1];
+    expect(recoveryBackupPath).toContain(".recovery");
+    if (recoveryBackupPath === undefined)
+      throw new Error("Amp did not report the current recovery backup path.");
+    expect(await readFile(recoveryBackupPath, "utf8")).toBe(
+      settingsBeforeEquals,
+    );
+
+    // A malformed settings file is rejected instead of reporting success.
+    const broken = join(root, "broken");
+    const brokenSettings = ampSettings(broken);
+    const malformed = '{ "amp.mcpServers": { "rea": {} ';
+    await mkdir(join(broken, ".config", "amp"), { recursive: true });
+    await writeFile(brokenSettings, malformed);
+    const rejected = await cli.run({
+      arguments: ["mcp", "add", "--agent", "amp", "--json"],
+      cwd: root,
+      environment: { HOME: broken, USERPROFILE: broken },
+      timeoutMs: 20_000,
+    });
+    expect(rejected.exitCode, rejected.stderr).toBe(1);
+    expect(rejected.json).toMatchObject({
+      code: "VALIDATION_ERROR",
+      fieldErrors: expect.arrayContaining([
+        expect.objectContaining({
+          code: "invalid_value",
+          missing: false,
+          path: "amp",
+        }),
+      ]),
+    });
+    expect(await readFile(brokenSettings, "utf8")).toBe(malformed);
+
+    // JSON null is valid JSON but not a settings object; reject it as a
+    // validation error instead of letting property access throw.
+    const nullHome = join(root, "null-settings");
+    const nullSettings = ampSettings(nullHome);
+    await mkdir(join(nullHome, ".config", "amp"), { recursive: true });
+    await writeFile(nullSettings, "null\n");
+    const nullRejected = await cli.run({
+      arguments: ["mcp", "add", "--agent", "amp", "--json"],
+      cwd: root,
+      environment: { HOME: nullHome, USERPROFILE: nullHome },
+      timeoutMs: 20_000,
+    });
+    expect(nullRejected.exitCode, nullRejected.stderr).toBe(1);
+    expect(nullRejected.json).toMatchObject({
+      code: "VALIDATION_ERROR",
+      message: expect.stringContaining("must contain a JSON object"),
+    });
+    expect(await readFile(nullSettings, "utf8")).toBe("null\n");
+
+    // An array-valued amp.mcpServers is rejected instead of claiming success.
+    const arrayHome = join(root, "array");
+    const arraySettings = ampSettings(arrayHome);
+    const array = '{"amp.mcpServers": []}\n';
+    await mkdir(join(arrayHome, ".config", "amp"), { recursive: true });
+    await writeFile(arraySettings, array);
+    const arrayRejected = await cli.run({
+      arguments: ["mcp", "add", "--agent", "amp", "--json"],
+      cwd: root,
+      environment: { HOME: arrayHome, USERPROFILE: arrayHome },
+      timeoutMs: 20_000,
+    });
+    expect(arrayRejected.exitCode, arrayRejected.stderr).toBe(1);
+    expect(arrayRejected.json).toMatchObject({
+      code: "VALIDATION_ERROR",
+      fieldErrors: expect.arrayContaining([
+        expect.objectContaining({
+          code: "invalid_value",
+          missing: false,
+          path: "amp",
+        }),
+      ]),
+    });
+    expect(await readFile(arraySettings, "utf8")).toBe(array);
+
+    // A backup write failure is reported as a filesystem problem, not as bad
+    // JSON.
+    const backupHome = join(root, "backup-error");
+    const backupSettings = ampSettings(backupHome);
+    const backupPath = `${backupSettings}.rea.backup`;
+    await mkdir(join(backupHome, ".config", "amp"), { recursive: true });
+    await writeFile(backupSettings, '{"amp.mcpServers": {}}\n');
+    await mkdir(backupPath);
+    const backupRejected = await cli.run({
+      arguments: ["mcp", "add", "--agent", "amp", "--json"],
+      cwd: root,
+      environment: { HOME: backupHome, USERPROFILE: backupHome },
+      timeoutMs: 20_000,
+    });
+    expect(backupRejected.exitCode, backupRejected.stderr).toBe(1);
+    expect(backupRejected.json).toMatchObject({
+      code: "VALIDATION_ERROR",
+      message: expect.stringContaining("Unable to create Amp settings backup"),
+    });
+  },
+);
+
+it("keeps the first Amp registration backup and secures later recovery copies", async () => {
+  const root = await createTestTempDirectory("rea-amp-backup-permissions-");
+  const settingsPath = join(root, "settings.json");
+  const settingsText =
+    '{"amp.mcpServers":{"rea":{"env":{"TOKEN":"secret"}}}}\n';
+  await writeFile(settingsPath, settingsText);
+
+  const backup = await writeAmpRegistrationBackup(settingsPath, settingsText);
+  if (!backup.ok)
+    throw new Error(
+      `${backup.error.kind} ${backup.error.path}: ${backup.error.detail}`,
+    );
+  const backupPath = backup.value;
+  expect(await readFile(backupPath, "utf8")).toBe(settingsText);
+  if (process.platform !== "win32")
+    expect((await stat(backupPath)).mode & 0o777).toBe(0o600);
+
+  await chmod(backupPath, 0o644);
+  const updatedSettings = "updated settings";
+  await writeFile(settingsPath, updatedSettings);
+  const updatedBackup = await writeAmpRegistrationBackup(
+    settingsPath,
+    updatedSettings,
+  );
+  if (!updatedBackup.ok)
+    throw new Error(
+      `${updatedBackup.error.kind} ${updatedBackup.error.path}: ${updatedBackup.error.detail}`,
+    );
+  const updatedBackupPath = updatedBackup.value;
+  expect(updatedBackupPath).not.toBe(backupPath);
+  expect(await readFile(updatedBackupPath, "utf8")).toBe(updatedSettings);
+  expect(await readFile(backupPath, "utf8")).toBe(settingsText);
+  if (process.platform !== "win32")
+    expect((await stat(updatedBackupPath)).mode & 0o777).toBe(0o600);
+  if (process.platform !== "win32")
+    expect((await stat(backupPath)).mode & 0o777).toBe(0o600);
+});
+
+it("returns a tagged failure when the Amp backup path is occupied", async () => {
+  const root = await createTestTempDirectory("rea-amp-backup-occupied-");
+  const settingsPath = join(root, "settings.json");
+  const backupPath = `${settingsPath}.rea.backup`;
+  await writeFile(settingsPath, "{}\n");
+  await mkdir(backupPath);
+
+  const result = await writeAmpRegistrationBackup(settingsPath, "{}\n");
+  expect(result).toMatchObject({
+    ok: false,
+    error: {
+      kind: "occupied",
+      path: backupPath,
+    },
+  });
+});