From f25d1e3e9fbda7971c312caa3da757dc525a73f6 Mon Sep 17 00:00:00 2001 From: STDquantum <60764289+STDquantum@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:23:42 +0800 Subject: [PATCH] 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 | 212 ++++++++++++++++ src/cli/ampRegistrationBackup.ts | 78 ++++++ .../cli/ampRegistrationPreservation.test.ts | 227 ++++++++++++++++++ 3 files changed, 517 insertions(+) create mode 100644 src/cli/ampRegistrationBackup.ts create mode 100644 tests/boundary/cli/ampRegistrationPreservation.test.ts diff --git a/src/cli.ts b/src/cli.ts index 0be35d9423..6d6b70286c 100644 --- a/src/cli.ts +++ b/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 | 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; + } 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)["rea"] + : undefined; + if (typeof entry === "object" && entry !== null) + priorEnv = (entry as Record)["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 => { + 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; + 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)["rea"]; + if ( + priorEnv !== undefined && + typeof entry === "object" && + entry !== null && + (entry as Record)["env"] === undefined + ) { + (entry as Record)["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) => diff --git a/src/cli/ampRegistrationBackup.ts b/src/cli/ampRegistrationBackup.ts new file mode 100644 index 0000000000..712dfac488 --- /dev/null +++ b/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 => { + let existing: ReturnType; + 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 => { + await writeFile(backupPath, await readFile(sourcePath), { + flag: "wx", + mode: 0o600, + }); +}; + +export const writeAmpRegistrationBackup = async ( + settingsPath: string, + settingsText: string, +): Promise> => { + 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); +}; diff --git a/tests/boundary/cli/ampRegistrationPreservation.test.ts b/tests/boundary/cli/ampRegistrationPreservation.test.ts new file mode 100644 index 0000000000..97d0842a10 --- /dev/null +++ b/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, + }, + }); +});