From 6051cb2c0c723d88dc43ee5ef8a96971bf84d49e Mon Sep 17 00:00:00 2001 From: Ysopwnable <161784452+N0zoM1z0@users.noreply.github.com> Date: Fri, 9 Oct 2026 12:57:00 +0800 Subject: [PATCH] fix(setup): preserve initial backups and Codex comments (#1204) --- docs/installation.md | 5 +- scripts/verify-package-setup.mjs | 27 ++-- .../ClientConfigurationDocument.ts | 54 +++---- src/application/Uninstall.ts | 17 ++- .../clientConfigurationTransactions.test.ts | 139 ++++++++++++++++++ .../filesystem/clientLifecycle.test.ts | 25 +++- 6 files changed, 221 insertions(+), 46 deletions(-) diff --git a/docs/installation.md b/docs/installation.md index 2028ea38d..5b4f4b83e 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -195,8 +195,9 @@ Before applying changes, REA checks your current configuration. The plan lists: Malformed or unsafe existing configuration blocks the whole transaction before Hopper installation or any file write. Declining or pressing Ctrl-C makes no -changes. Agent configuration writes preserve unrelated -entries, create backups, use atomic replacement, and verify their result. +changes. Agent configuration writes preserve unrelated entries and comments, +create backups, use atomic replacement, and verify their result. Setup and +uninstall retain an existing `.rea.backup` rather than replacing the first snapshot. After setup, REA reports which agents, analysis tools, and workflow files passed its final checks. Restart any agent named in the completion message, then begin diff --git a/scripts/verify-package-setup.mjs b/scripts/verify-package-setup.mjs index 9db11edba..b415965e0 100644 --- a/scripts/verify-package-setup.mjs +++ b/scripts/verify-package-setup.mjs @@ -31,6 +31,14 @@ const OPENCODE_ORIGINAL = `{ }, }\n`; +const CODEX_ORIGINAL = [ + "# Keep Codex preferences and their spelling.", + 'model = "gpt-5"', + 'notify = ["a", "b"]', + "literal = 'C:\\demo\\path'", + "", +].join("\n"); + const verifyMcpAdd = async ({ cli, environment, npxLog }) => { await run(cli, ["mcp", "add"], environment); const mcpRegistration = await readFile(npxLog, "utf8"); @@ -248,7 +256,7 @@ const verifySetupPlan = async ({ !planned.plannedActions.some(({ kind }) => kind === "configure_client") || !planned.plannedActions.some(({ kind }) => kind === "install_skill") || plannedClaudeConfig !== '{"existing":true}\n' || - plannedCodexConfig !== 'model = "gpt-5"\n' || + plannedCodexConfig !== CODEX_ORIGINAL || plannedCursorConfig !== '{"existing":true}\n' || plannedOpenCodeConfig !== opencodeOriginal ) @@ -459,8 +467,8 @@ const assertCodexSymlink = async ({ !codex.includes("[mcp_servers.rea]") || !codex.includes(`command = "${cli}"`) || !codex.includes("startup_timeout_sec = 30") || - (await readFile(`${codexConfig}.rea.backup`, "utf8")) !== - 'model = "gpt-5"\n' || + !codex.startsWith(CODEX_ORIGINAL) || + (await readFile(`${codexConfig}.rea.backup`, "utf8")) !== CODEX_ORIGINAL || (await readFile(`${cursorConfig}.rea.backup`, "utf8")) !== '{"existing":true}\n' ) @@ -631,7 +639,6 @@ const verifyUninstall = async ({ codexTarget, opencodeConfig, }) => { - const openCodeBeforeUninstall = await readFile(opencodeConfig, "utf8"); const uninstallExecution = await runWithStatus( cli, ["uninstall", "--json"], @@ -657,7 +664,7 @@ const verifyUninstall = async ({ !(await lstat(codexConfig)).isSymbolicLink() || cursorAfterUninstall.existing !== true || cursorAfterUninstall.mcpServers?.rea !== undefined || - !codexAfterUninstall.includes('model = "gpt-5"') || + codexAfterUninstall !== CODEX_ORIGINAL || codexAfterUninstall.includes("mcp_servers.rea") || openCodeErrors.length !== 0 || openCodeAfterUninstall?.model !== "provider/model" || @@ -670,11 +677,10 @@ const verifyUninstall = async ({ "Keep this independently managed server", ) || (await readFile(`${opencodeConfig}.rea.backup`, "utf8")) !== - openCodeBeforeUninstall || - !(await readFile(`${cursorConfig}.rea.backup`, "utf8")).includes('"rea"') || - !(await readFile(`${codexConfig}.rea.backup`, "utf8")).includes( - "[mcp_servers.rea]", - ) + OPENCODE_ORIGINAL || + (await readFile(`${cursorConfig}.rea.backup`, "utf8")) !== + '{"existing":true}\n' || + (await readFile(`${codexConfig}.rea.backup`, "utf8")) !== CODEX_ORIGINAL ) throw new Error( `packaged uninstall did not preserve config symlinks: ${JSON.stringify(uninstall)}`, @@ -704,6 +710,7 @@ export async function verifyPackageSetup({ ); const siblingSkillPath = join(home, ".agents/skills/unrelated/SKILL.md"); if (supportedSetupHost) { + await writeFile(codexTarget, CODEX_ORIGINAL); await mkdir(join(home, ".agents/skills/unrelated"), { recursive: true }); await writeFile(siblingSkillPath, "unrelated skill\n"); } diff --git a/src/application/ClientConfigurationDocument.ts b/src/application/ClientConfigurationDocument.ts index 991dcd0b4..038243d8e 100644 --- a/src/application/ClientConfigurationDocument.ts +++ b/src/application/ClientConfigurationDocument.ts @@ -832,15 +832,15 @@ const disabledServerListLines = ( /** * Replace REA's TOML server tables and assignments. Comments that introduce a - * following table stay. Text inside strings is not treated as a header. Only - * Grok Build reads the root `disabled_mcp_servers` list that registration - * also edits. + * following table stay. Codex also retains comments around REA's own table. + * Text inside strings is not treated as a header. Only Grok Build reads the + * root `disabled_mcp_servers` list that registration also edits. */ const upsertTomlServerSection = ( originalText: string, serverKey: string, entry: unknown, - editDisabledList: boolean, + format: "toml" | "grok", ): string => { // smol-toml accepts a leading BOM. Leave it in place or the first header is missed. const bom = originalText.startsWith("\uFEFF"); @@ -917,24 +917,25 @@ const upsertTomlServerSection = ( mode = "none"; continue; } - const disabledLines = editDisabledList - ? disabledServerListLines( - disabledServerListEdit({ - text, - lineStart, - valueOffset, - valueEnd, - statementEnd, - tablePath, - segments: assignment.segments, - serverKey, - entry, - }), - lines, - lineIndex, - endLine, - ) - : undefined; + const disabledLines = + format === "grok" + ? disabledServerListLines( + disabledServerListEdit({ + text, + lineStart, + valueOffset, + valueEnd, + statementEnd, + tablePath, + segments: assignment.segments, + serverKey, + entry, + }), + lines, + lineIndex, + endLine, + ) + : undefined; if (disabledLines !== undefined) { flushPending(); kept.push(...disabledLines); @@ -975,7 +976,10 @@ const upsertTomlServerSection = ( continue; } if (isBlankOrComment(line)) { - pending.push(physical); + if (format === "toml" && line.trimStart().startsWith("#")) { + flushPending(); + kept.push(physical); + } else pending.push(physical); lineIndex += 1; continue; } @@ -1044,7 +1048,7 @@ const serializeGrokConfiguration = ( originalText, PRODUCT_IDENTITY.mcpServerKey, entry, - true, + "grok", ); }; @@ -1077,7 +1081,7 @@ const serializeCodexConfiguration = ( originalText, PRODUCT_IDENTITY.mcpServerKey, tomlServerEntry(document, PRODUCT_IDENTITY.mcpServerKey), - false, + "toml", ); // Removing a final table also drops the file's last line break. const edited = diff --git a/src/application/Uninstall.ts b/src/application/Uninstall.ts index 789bdc34e..e4e023d63 100644 --- a/src/application/Uninstall.ts +++ b/src/application/Uninstall.ts @@ -7,6 +7,7 @@ import { type ClientConfigurationDocument, } from "./ClientConfigurationDocument.js"; import { copyFile, lstat, readFile, realpath, rm } from "node:fs/promises"; +import { constants as fsConstants } from "node:fs"; import { homedir } from "node:os"; import { join } from "node:path"; @@ -31,6 +32,7 @@ interface ManagedPathStats { /** Injectable filesystem operations used to prove uninstall failure recovery. */ export interface UninstallFileSystem { readText(path: string): Promise; + /** Preserve the first backup; reject an existing destination with EEXIST. */ copy(source: string, destination: string): Promise; writeText(path: string, contents: string): Promise; stat(path: string): Promise; @@ -40,7 +42,8 @@ export interface UninstallFileSystem { const systemFileSystem: UninstallFileSystem = { readText: (path) => readFile(path, "utf8"), - copy: (source, destination) => copyFile(source, destination), + copy: (source, destination) => + copyFile(source, destination, fsConstants.COPYFILE_EXCL), writeText: (path, contents) => writeFileAtomic(path, contents, { encoding: "utf8" }), stat: (path) => lstat(path), @@ -274,12 +277,12 @@ const removeClient = async ( try { await fileSystem.copy(transactionPath, backupPath); } catch (cause: unknown) { - void cause; - return item( - client.name, - "failed", - "Configuration could not be backed up, so no change was made. Check file permissions, then rerun uninstall.", - ); + if (!(cause instanceof Error && "code" in cause && cause.code === "EEXIST")) + return item( + client.name, + "failed", + "Configuration could not be backed up, so no change was made. Check file permissions, then rerun uninstall.", + ); } try { await fileSystem.writeText( diff --git a/tests/boundary/filesystem/clientConfigurationTransactions.test.ts b/tests/boundary/filesystem/clientConfigurationTransactions.test.ts index 05576f563..477085381 100644 --- a/tests/boundary/filesystem/clientConfigurationTransactions.test.ts +++ b/tests/boundary/filesystem/clientConfigurationTransactions.test.ts @@ -2,8 +2,10 @@ import { chmod, readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; +import { parse as parseToml } from "smol-toml"; import { configureClientConfiguration } from "../../../src/application/SetupClientConfiguration.js"; +import { systemUninstallHost } from "../../../src/application/Uninstall.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; describe("client configuration write failures", () => { @@ -55,3 +57,140 @@ describe("TOML client configuration comparison", () => { }); }); }); + +const unrelatedToml = [ + "# Keep this explanation.", + 'notify = ["a", "b"]', + "literal = 'C:\\demo\\path'", + 'disabled_mcp_servers = ["rea", "other"] # Not a Codex registration.', + "", +].join("\n"); + +const unrelatedServer = [ + "# Keep the other server.", + '[mcp_servers."rea.env"]', + "command = 'other'", + "", +].join("\n"); + +const unrelatedValues = (text: string) => { + const document = parseToml(text); + const servers = document.mcp_servers; + if (typeof servers === "object" && servers !== null) { + Reflect.deleteProperty(servers, "rea"); + if (Object.keys(servers).length === 0) delete document.mcp_servers; + } + return document; +}; + +describe("Codex configuration text preservation", () => { + it.each([ + { + name: "new registration", + original: unrelatedToml, + preserved: [unrelatedToml], + }, + { + name: "quoted tables and nested environment", + original: `${unrelatedToml}[mcp_servers."rea"]\ncommand = "old"\n[mcp_servers.rea.env]\nOLD = "value"\n${unrelatedServer}`, + preserved: [unrelatedToml, unrelatedServer], + }, + { + name: "dotted registration keys", + original: `${unrelatedToml}mcp_servers.rea.command = "old"\n${unrelatedServer}`, + preserved: [unrelatedToml, unrelatedServer], + }, + { + name: "inline server table", + original: `${unrelatedToml}mcp_servers = { rea = { command = "old" }, other = { command = 'other' } } # Shared table.\n`, + preserved: [ + unrelatedToml, + "other = { command = 'other' }", + "# Shared table.", + ], + }, + { + name: "BOM, CRLF, and multiline text resembling a registration", + original: + `\uFEFF${unrelatedToml}banner = '''\n[mcp_servers.rea]\n# This is string content.\n'''\n`.replaceAll( + "\n", + "\r\n", + ), + preserved: [ + unrelatedToml.replaceAll("\n", "\r\n"), + "banner = '''\r\n[mcp_servers.rea]\r\n# This is string content.\r\n'''", + ], + }, + { + name: "comments without settings", + original: "# Keep this explanation.\n# And this trailing comment.\n\n", + preserved: ["# Keep this explanation.", "# And this trailing comment."], + }, + ])( + "preserves unrelated text through $name setup and uninstall", + async ({ original, preserved }) => { + const root = await createTestTempDirectory("rea-codex-preserve-"); + const configPath = join(root, "config.toml"); + const client = { name: "codex", format: "toml", configPath } as const; + await writeFile(configPath, original); + + expect( + await configureClientConfiguration(client, {}, ["rea", "mcp"]), + ).toMatchObject({ status: "configured" }); + const configured = await readFile(configPath, "utf8"); + for (const text of preserved) expect(configured).toContain(text); + expect(unrelatedValues(configured)).toEqual(unrelatedValues(original)); + expect(configured.startsWith("\uFEFF")).toBe( + original.startsWith("\uFEFF"), + ); + expect( + await configureClientConfiguration(client, {}, ["rea", "mcp"]), + ).toEqual({ status: "unchanged" }); + expect(await readFile(configPath, "utf8")).toBe(configured); + + expect( + (await systemUninstallHost(root).removeClient(client)).status, + ).toBe("removed"); + const removed = await readFile(configPath, "utf8"); + for (const text of preserved) expect(removed).toContain(text); + expect(unrelatedValues(removed)).toEqual(unrelatedValues(original)); + expect(parseToml(removed)).not.toHaveProperty("mcp_servers.rea"); + expect(removed.startsWith("\uFEFF")).toBe(original.startsWith("\uFEFF")); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original); + }, + ); + + it("retains the first backup after uninstall and a later setup", async () => { + const root = await createTestTempDirectory("rea-codex-backup-"); + const configPath = join(root, "config.toml"); + const client = { name: "codex", format: "toml", configPath } as const; + const original = 'notify = ["a", "b"]\n'; + await writeFile(configPath, original); + await configureClientConfiguration(client, {}, ["rea", "mcp"]); + expect((await systemUninstallHost(root).removeClient(client)).status).toBe( + "removed", + ); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original); + await configureClientConfiguration(client, {}, ["rea", "mcp"]); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original); + }); + + it("refuses malformed Codex TOML unchanged without creating a backup", async () => { + const root = await createTestTempDirectory("rea-codex-malformed-"); + const configPath = join(root, "config.toml"); + const client = { name: "codex", format: "toml", configPath } as const; + const original = + "# Keep this explanation.\n[mcp_servers.rea]\ncommand = [broken\n"; + await writeFile(configPath, original); + expect( + await configureClientConfiguration(client, {}, ["rea", "mcp"]), + ).toEqual({ status: "failed", reason: "readback" }); + expect((await systemUninstallHost(root).removeClient(client)).status).toBe( + "failed", + ); + expect(await readFile(configPath, "utf8")).toBe(original); + await expect( + readFile(`${configPath}.rea.backup`, "utf8"), + ).rejects.toMatchObject({ code: "ENOENT" }); + }); +}); diff --git a/tests/boundary/filesystem/clientLifecycle.test.ts b/tests/boundary/filesystem/clientLifecycle.test.ts index 6c314eec4..1333f465c 100644 --- a/tests/boundary/filesystem/clientLifecycle.test.ts +++ b/tests/boundary/filesystem/clientLifecycle.test.ts @@ -1,3 +1,4 @@ +import { constants as fsConstants } from "node:fs"; import { copyFile, lstat, @@ -1587,7 +1588,7 @@ describe("client configuration filesystem failure reporting", () => { copy: (source, destination) => failure === "backup" ? Promise.reject(new Error("SECRET backup failure")) - : copyFile(source, destination), + : copyFile(source, destination, fsConstants.COPYFILE_EXCL), writeText: async (path, contents) => { writes += 1; if ((failure === "update" && writes === 1) || failure === "restore") @@ -1607,6 +1608,25 @@ describe("client configuration filesystem failure reporting", () => { }, ); + it("restores the current configuration without replacing its initial backup", async () => { + const { home, config } = await uninstallFixture(); + const current = await readFile(config, "utf8"); + const initial = '{"existing":true}\n'; + await writeFile(`${config}.rea.backup`, initial); + let writes = 0; + const result = await systemUninstallHost(home, { + ...testFileSystem, + writeText: async (path, contents) => { + writes += 1; + await writeFile(path, contents); + if (writes === 1) throw new Error("update failed after writing"); + }, + }).removeClient({ name: "cursor", format: "json", configPath: config }); + expect(result.status).toBe("failed"); + expect(await readFile(config, "utf8")).toBe(current); + expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(initial); + }); + it("reports a managed-path removal failure", async () => { const home = await createTestTempDirectory("rea-uninstall-remove-"); roots.push(home); @@ -1631,7 +1651,8 @@ describe("client configuration filesystem failure reporting", () => { const testFileSystem: UninstallFileSystem = { readText: (path) => readFile(path, "utf8"), - copy: (source, destination) => copyFile(source, destination), + copy: (source, destination) => + copyFile(source, destination, fsConstants.COPYFILE_EXCL), writeText: (path, contents) => writeFile(path, contents), stat: (path) => lstat(path), realpath: (path) => realpath(path),