From f6775a36694a6a9c620f4248638e8056a6cbcb66 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Tue, 13 Oct 2026 14:51:55 +0800 Subject: [PATCH] fix(setup): preserve current recovery snapshots and decoding failures --- docs/installation.md | 4 + src/application/RegularFileRead.ts | 65 ++-- src/application/Setup.recovery.test.ts | 34 ++ src/application/SetupClientConfiguration.ts | 175 +++++++++- src/application/SetupClients.ts | 18 +- src/application/SetupHost.ts | 8 +- src/application/SetupPlan.ts | 2 +- src/application/SetupTypes.ts | 10 + src/application/Uninstall.ts | 12 +- .../clientConfigurationDiagnosticsCli.test.ts | 17 +- .../clientConfigurationTransactions.test.ts | 80 ++++- .../setupClientConfigCleanupOwnership.test.ts | 227 +++++++----- .../setupConfigurationRollback.test.ts | 325 ++++++++++++++++++ 13 files changed, 835 insertions(+), 142 deletions(-) create mode 100644 tests/process-global/setupConfigurationRollback.test.ts diff --git a/docs/installation.md b/docs/installation.md index 0a8041898..3bd4e0928 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -57,6 +57,10 @@ Hopper installation or any file write. Declining or pressing Ctrl-C makes no 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. +If that snapshot differs from the current configuration, setup previews and +preserves a verified `.rea.backup..recovery` copy of the current bytes. +Both snapshots remain available; a failed rollback reports the current recovery +copy's path and the restore failure. Setup also retains valid environment overrides in REA's existing server entry, such as `REA_MCP_INPUT_SCHEMA_PROFILE` and `REA_MCP_MAX_RESPONSE_BYTES`. Detected provider settings replace only matching environment keys. diff --git a/src/application/RegularFileRead.ts b/src/application/RegularFileRead.ts index f09f4c192..ee4d6170a 100644 --- a/src/application/RegularFileRead.ts +++ b/src/application/RegularFileRead.ts @@ -19,6 +19,17 @@ export interface RegularFileReadOptions { readonly resources?: ArtifactResourceScope; } +export class InvalidUtf8FileError extends TypeError { + readonly _tag = "InvalidUtf8FileError"; + constructor( + readonly path: string, + cause: unknown, + ) { + super(`File is not valid UTF-8: ${path}`, { cause }); + this.name = this._tag; + } +} + export type RegularFileReadOutcome = | { readonly kind: "completed"; readonly value: Value } | { readonly kind: "failed"; readonly cause: unknown }; @@ -142,7 +153,34 @@ const withRegularFileOwned = async ( export const readRegularFile = ( path: string, options: RegularFileReadOptions = {}, -): Promise => +): Promise => readRegularFileContent(path, (bytes) => bytes, options); + +/** Read valid UTF-8 without replacing source bytes or stripping an initial BOM. */ +export const readRegularFileText = ( + path: string, + options: RegularFileReadOptions = {}, +): Promise => + readRegularFileContent( + path, + (bytes) => { + try { + return new TextDecoder("utf-8", { + fatal: true, + ignoreBOM: true, + }).decode(bytes); + } catch (cause: unknown) { + if (!(cause instanceof TypeError)) throw cause; + throw new InvalidUtf8FileError(path, cause); + } + }, + options, + ); + +const readRegularFileContent = ( + path: string, + decode: (bytes: Buffer) => Value, + options: RegularFileReadOptions, +): Promise => withRegularFile( path, async (handle, stats) => { @@ -153,30 +191,7 @@ export const readRegularFile = ( ); if (bytes === undefined || bytes.length !== stats.size) throw new RegularFileChangedError(path); - return bytes; + return decode(bytes); }, options, ); - -/** Read one admitted regular file as UTF-8 without waiting on a pipe. */ -export const readRegularFileText = async ( - path: string, - options: RegularFileReadOptions = {}, -): Promise => { - try { - return (await readRegularFile(path, options)).toString("utf8"); - } catch (cause: unknown) { - if ( - cause instanceof RegularFileCleanupFailure && - cause.outcome.kind === "completed" && - Buffer.isBuffer(cause.outcome.value) - ) - throw new RegularFileCleanupFailure( - cause.path, - cause.owner, - { kind: "completed", value: cause.outcome.value.toString("utf8") }, - cause.cleanupCause, - ); - throw cause; - } -}; diff --git a/src/application/Setup.recovery.test.ts b/src/application/Setup.recovery.test.ts index 951ef3a54..f29c9d375 100644 --- a/src/application/Setup.recovery.test.ts +++ b/src/application/Setup.recovery.test.ts @@ -54,6 +54,40 @@ describe("setup workflow", () => { expect(result.remediation).toContain("could not be updated"); }); + it("reports a failed config rollback with its destination and recovery backup", async () => { + const host = new FakeSetupHost(); + host.hopper = "/Applications/Hopper"; + host.clients = [{ name: "codex", configPath: "/fixture/codex.json" }]; + host.clientResults.set("codex", { + status: "failed", + reason: "rollback", + backupPath: "/fixture/codex.json.rea.backup", + failure: { + kind: "restore", + path: "/fixture/codex.json", + detail: "permission denied", + }, + }); + + const result = await runSetup( + { ...options(true), clientIds: ["codex"] }, + host, + ); + + expect(result.clients.codex).toEqual({ + status: "failed", + reason: "rollback", + backupPath: "/fixture/codex.json.rea.backup", + failure: { + kind: "restore", + path: "/fixture/codex.json", + detail: "permission denied", + }, + }); + expect(result.remediation).toContain("permission denied"); + expect(result.remediation).toContain("/fixture/codex.json.rea.backup"); + }); + it("explains skill installation recovery", async () => { const host = new FakeSetupHost(); host.hopper = "/Applications/Hopper"; diff --git a/src/application/SetupClientConfiguration.ts b/src/application/SetupClientConfiguration.ts index bfab80cbf..981b829a4 100644 --- a/src/application/SetupClientConfiguration.ts +++ b/src/application/SetupClientConfiguration.ts @@ -19,6 +19,7 @@ import { } from "./ClientConfigurationDocument.js"; import { dshDocument, DshCordisDocument } from "./DshCordisPatch.js"; import { constants as fsConstants } from "node:fs"; +import { createHash } from "node:crypto"; import { copyFile, mkdir, rm, writeFile } from "node:fs/promises"; import { dirname } from "node:path"; import writeFileAtomic from "write-file-atomic"; @@ -29,6 +30,7 @@ import { npxRegistrationCommand } from "./ClientRegistrationIdentity.js"; import { MCP_STARTUP_POLICY } from "../mcpStartupPolicy.js"; import { resolveClientConfigTransactionPath } from "./ClientConfigPath.js"; import { + readRegularFile, readRegularFileText, RegularFileCleanupFailure, } from "./RegularFileRead.js"; @@ -40,6 +42,26 @@ import type { } from "./SetupTypes.js"; import type { SetupClient } from "./SupportedClients.js"; +export class ClientConfigurationCleanupRollbackFailure extends RegularFileCleanupFailure { + constructor( + failure: RegularFileCleanupFailure, + readonly configurationFailure: Extract< + ClientConfigurationResult, + { readonly status: "failed"; readonly reason: "rollback" } + >, + ) { + super( + failure.path, + failure.owner, + failure.outcome, + new AggregateError( + [failure.cleanupCause, new Error(configurationFailure.failure.detail)], + `Configuration readback cleanup and rollback failed: ${configurationFailure.failure.path}`, + ), + ); + } +} + /** Configure one supported client's native stdio MCP registration shape. */ export const configureClientConfiguration = ( client: SetupClient, @@ -167,11 +189,23 @@ const persistClientConfiguration = async ({ }: ClientConfigurationUpdate): Promise => { const backupPath = original === undefined ? undefined : `${client.configPath}.rea.backup`; - if ( - backupPath !== undefined && - !(await preserveConfigBackup(transactionPath, backupPath)) - ) - return { status: "failed", reason: "backup" }; + let recoveryBackupPath: string | undefined; + if (backupPath !== undefined && original !== undefined) { + const selection = await selectCurrentRecoveryBackup(backupPath, original); + if (!selection.ok) return { status: "failed", reason: "backup" }; + if ( + !(await preserveConfigBackup(transactionPath, selection.value.backupPath)) + ) + return { status: "failed", reason: "backup" }; + const verified = await selectCurrentRecoveryBackup(backupPath, original); + if ( + !verified.ok || + verified.value.backupPath !== selection.value.backupPath || + !verified.value.present + ) + return { status: "failed", reason: "backup" }; + recoveryBackupPath = verified.value.backupPath; + } // A native OpenCode V2 entry replaces REA's legacy V1 entry, which would // otherwise conflict with it. const legacyPath = legacyClientServerPath( @@ -239,19 +273,39 @@ const persistClientConfiguration = async ({ !clientConfigurationValuesEqual(readback.servers[name], entry), ) ) { - await restoreConfig(transactionPath, original); - return { status: "failed", reason: "readback" }; + return rollbackOrReadbackFailure( + transactionPath, + original, + recoveryBackupPath, + ); } } catch (cause: unknown) { - if (cause instanceof RegularFileCleanupFailure) throw cause; + if (cause instanceof RegularFileCleanupFailure) { + const rollback = await restoreConfig(transactionPath, original); + if (!rollback.ok) + throw new ClientConfigurationCleanupRollbackFailure(cause, { + status: "failed", + reason: "rollback", + ...(recoveryBackupPath === undefined + ? {} + : { backupPath: recoveryBackupPath }), + failure: rollback.error, + }); + throw cause; + } // Readback failure restores the transaction before reporting. void cause; - await restoreConfig(transactionPath, original); - return { status: "failed", reason: "readback" }; + return rollbackOrReadbackFailure( + transactionPath, + original, + recoveryBackupPath, + ); } return { status: "configured", - ...(backupPath === undefined ? {} : { backupPath }), + ...(recoveryBackupPath === undefined + ? {} + : { backupPath: recoveryBackupPath }), }; }; @@ -406,9 +460,18 @@ export const inspectClientConfiguration = async ( "The existing configuration is malformed. Repair it before setup applies any other changes.", }; } + const backupSelection = await selectCurrentRecoveryBackup( + `${client.configPath}.rea.backup`, + original, + ); + if (!backupSelection.ok) + return { + status: "invalid", + remediation: backupSelection.error.detail, + }; return { status: "update", - backupPath: `${client.configPath}.rea.backup`, + backupPath: backupSelection.value.backupPath, }; }; @@ -473,19 +536,101 @@ const preserveConfigBackup = async ( } }; +interface RecoveryBackupSelection { + readonly backupPath: string; + readonly present: boolean; +} + +interface RecoveryBackupSelectionFailure { + readonly kind: "unreadable" | "occupied"; + readonly path: string; + readonly detail: string; +} + +const selectCurrentRecoveryBackup = async ( + firstBackupPath: string, + original: string, +): Promise> => { + const expected = Buffer.from(original, "utf8"); + let firstBackup: Buffer | undefined; + try { + firstBackup = await readRegularFile(firstBackupPath, { + symlinks: "reject", + }); + } catch (cause: unknown) { + if (cause instanceof RegularFileCleanupFailure) throw cause; + if (!isMissing(cause)) + return err({ + kind: "unreadable", + path: firstBackupPath, + detail: `The recovery backup could not be checked: ${errorDetail(cause)}`, + }); + } + if (firstBackup === undefined) + return ok({ backupPath: firstBackupPath, present: false }); + if (firstBackup.equals(expected)) + return ok({ backupPath: firstBackupPath, present: true }); + + const digest = createHash("sha256").update(original, "utf8").digest("hex"); + const recoveryPath = `${firstBackupPath}.${digest}.recovery`; + let recovery: Buffer | undefined; + try { + recovery = await readRegularFile(recoveryPath, { symlinks: "reject" }); + } catch (cause: unknown) { + if (cause instanceof RegularFileCleanupFailure) throw cause; + if (!isMissing(cause)) + return err({ + kind: "unreadable", + path: recoveryPath, + detail: `The recovery backup could not be checked: ${errorDetail(cause)}`, + }); + } + if (recovery !== undefined && !recovery.equals(expected)) + return err({ + kind: "occupied", + path: recoveryPath, + detail: `The recovery path already contains different data: ${recoveryPath}`, + }); + return ok({ backupPath: recoveryPath, present: recovery !== undefined }); +}; + const restoreConfig = async ( path: string, original: string | undefined, -): Promise => { +): Promise< + Result< + void, + { readonly kind: "restore"; readonly path: string; readonly detail: string } + > +> => { try { if (original === undefined) await rm(path, { force: true }); else await writeFile(path, original, { encoding: "utf8", mode: 0o600 }); + return ok(undefined); } catch (cause: unknown) { - // The backup remains available for the remediation reported by setup. - void cause; + return err({ kind: "restore", path, detail: errorDetail(cause) }); } }; +const rollbackOrReadbackFailure = async ( + targetPath: string, + original: string | undefined, + backupPath: string | undefined, +): Promise => { + const rollback = await restoreConfig(targetPath, original); + return rollback.ok + ? { status: "failed", reason: "readback" } + : { + status: "failed", + reason: "rollback", + ...(backupPath === undefined ? {} : { backupPath }), + failure: rollback.error, + }; +}; + +const errorDetail = (cause: unknown): string => + cause instanceof Error ? cause.message : String(cause); + /** Whether REA's entry matches, no legacy entry remains, and no client disable list suppresses it. */ const registrationCurrent = ( parsed: ClientConfigurationDocument, diff --git a/src/application/SetupClients.ts b/src/application/SetupClients.ts index bbeeedf3f..d86d04b9e 100644 --- a/src/application/SetupClients.ts +++ b/src/application/SetupClients.ts @@ -6,15 +6,19 @@ import type { } from "./SetupTypes.js"; import type { SetupClient } from "./SupportedClients.js"; -const failedConfigurationMessage = ( - reason: "path" | "backup" | "write" | "readback", +export const failedConfigurationMessage = ( + result: Extract, ): string => { - if (reason === "path") + if (result.reason === "path") return "Agent configuration path could not be safely verified. Check its permissions and, if it is a symbolic link, verify that the link resolves to a regular file owned by the current user, then rerun setup."; - if (reason === "backup") + if (result.reason === "backup") return "Agent configuration could not be backed up, so no change was made. Check file permissions, then rerun setup."; - if (reason === "write") + if (result.reason === "write") return "Agent configuration could not be updated. Check file permissions, then rerun setup."; + if (result.reason === "rollback") + return result.backupPath === undefined + ? `Agent configuration rollback failed at ${result.failure.path}: ${result.failure.detail}; the failed first-run file may remain. Remove or repair it before rerunning setup.` + : `Agent configuration rollback failed at ${result.failure.path}: ${result.failure.detail}. Inspect the preserved backup at ${result.backupPath} and restore the intended configuration, then rerun setup.`; return "Agent configuration could not be verified after writing. Repair the configuration file or restore its `.rea.backup`, then rerun setup."; }; @@ -40,12 +44,12 @@ export const configureDetectedClients = async (options: { ); options.clients[client.name] = result; if (result.status === "failed") { - failure ??= failedConfigurationMessage(result.reason); + failure ??= failedConfigurationMessage(result); options.onProgress?.({ actionId, label, state: "failed", - detail: failedConfigurationMessage(result.reason), + detail: failedConfigurationMessage(result), }); } else if (result.status === "configured") { options.appliedActions.push(`configured_${client.name}`); diff --git a/src/application/SetupHost.ts b/src/application/SetupHost.ts index 9e2ae95f1..7ac6828fd 100644 --- a/src/application/SetupHost.ts +++ b/src/application/SetupHost.ts @@ -33,9 +33,11 @@ import { } from "./SetupSkill.js"; import { clientConfigurationAligned, + ClientConfigurationCleanupRollbackFailure, configureClientConfiguration, inspectClientConfiguration, } from "./SetupClientConfiguration.js"; +import { failedConfigurationMessage } from "./SetupClients.js"; import { setupInstallFailure } from "./SetupInstallFailure.js"; import { RegularFileCleanupFailure, @@ -275,8 +277,12 @@ const runClientConfigurationOperation = async ( cleanup.outcome.kind === "failed" ? cleanup.outcome.cause : cause.cleanupCause; + const rollbackMessage = + cause instanceof ClientConfigurationCleanupRollbackFailure + ? ` ${failedConfigurationMessage(cause.configurationFailure)}` + : ""; throw configurationCleanupError( - `Could not safely access client configuration at ${cause.path}: ${errorMessage(primary)}`, + `Could not safely access client configuration at ${cause.path}: ${errorMessage(primary)}.${rollbackMessage}`, cause, cleanup.cleanup, ); diff --git a/src/application/SetupPlan.ts b/src/application/SetupPlan.ts index 83402d1a4..a78697c80 100644 --- a/src/application/SetupPlan.ts +++ b/src/application/SetupPlan.ts @@ -320,5 +320,5 @@ const clientConfigurationDetail = ( ? " If this plan installs Hopper, REA will add HOPPER_LAUNCHER_PATH using the launcher path reported by that installation." : " The HOPPER_LAUNCHER_PATH value shown above is the current fallback; if this plan installs Hopper, REA will replace it with the launcher path reported by that installation." : ""; - return `Add the REA MCP registration for ${client}; preserve unrelated configuration. Command: ${command.map((part) => JSON.stringify(part)).join(" ")}.${environmentDetail}${hopperDetail}`; + return `Add the REA MCP registration for ${client}; preserve unrelated configuration. The backup path reflects the configuration inspected for this plan; if that file changes before execution, setup will select a recovery path for its current contents. Command: ${command.map((part) => JSON.stringify(part)).join(" ")}.${environmentDetail}${hopperDetail}`; }; diff --git a/src/application/SetupTypes.ts b/src/application/SetupTypes.ts index da58abbff..241e0f6dd 100644 --- a/src/application/SetupTypes.ts +++ b/src/application/SetupTypes.ts @@ -26,6 +26,16 @@ export type ClientConfigurationResult = | { readonly status: "failed"; readonly reason: "path" | "backup" | "write" | "readback"; + } + | { + readonly status: "failed"; + readonly reason: "rollback"; + readonly backupPath?: string; + readonly failure: { + readonly kind: "restore"; + readonly path: string; + readonly detail: string; + }; }; /** Read-only state used to block unsafe setup plans before any mutation. */ diff --git a/src/application/Uninstall.ts b/src/application/Uninstall.ts index c410280bc..a7f051a49 100644 --- a/src/application/Uninstall.ts +++ b/src/application/Uninstall.ts @@ -25,7 +25,11 @@ import { import { isOwnedClientRegistrationCommand } from "./ClientRegistrationIdentity.js"; import { dshDocument } from "./DshCordisPatch.js"; import { resolveClientConfigTransactionPath } from "./ClientConfigPath.js"; -import { readRegularFileText } from "./RegularFileRead.js"; +import { + InvalidUtf8FileError, + RegularFileCleanupFailure, + readRegularFileText, +} from "./RegularFileRead.js"; import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { manualRegistrationRemediation, @@ -269,13 +273,17 @@ const readClientConfiguration = async ( try { original = await fileSystem.readText(transactionPath); } catch (cause: unknown) { + const readFailure = + cause instanceof RegularFileCleanupFailure ? cause.cause : cause; return itemRead( isMissing(cause) ? item(client.name, "skipped", "Configuration does not exist.") : item( client.name, "failed", - "Configuration could not be read. Check file permissions, then rerun uninstall.", + readFailure instanceof InvalidUtf8FileError + ? "Configuration is not valid UTF-8 and was not changed. Repair it, then rerun uninstall." + : "Configuration could not be read. Check file permissions, then rerun uninstall.", ), ); } diff --git a/tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts b/tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts index d9363988f..425e0e610 100644 --- a/tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts +++ b/tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts @@ -6,6 +6,17 @@ import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { cliTest } from "../../support/cli/cliFixture.js"; const cases = [ + { + client: "cursor", + path: ".cursor/mcp.json", + text: "malformed UTF-8", + bytes: Buffer.concat([ + Buffer.from('{"label":"'), + Buffer.from([0xff]), + Buffer.from('"}'), + ]), + diagnostic: "not valid UTF-8", + }, { client: "cursor", path: ".cursor/mcp.json", @@ -92,7 +103,9 @@ for (const scenario of cases) ); const config = join(home, scenario.path); await mkdir(dirname(config), { recursive: true }); - await writeFile(config, scenario.text); + const original = + "bytes" in scenario ? scenario.bytes : Buffer.from(scenario.text); + await writeFile(config, original); const environment = { HOME: home, USERPROFILE: home, @@ -146,7 +159,7 @@ for (const scenario of cases) }); if (!("syntax" in scenario)) expect(uninstall.stdout).not.toContain("not valid JSON"); - expect(await readFile(config, "utf8")).toBe(scenario.text); + expect(await readFile(config)).toEqual(original); expect(await readdir(dirname(config))).toEqual([ scenario.path.split("/").at(-1), ]); diff --git a/tests/boundary/filesystem/clientConfigurationTransactions.test.ts b/tests/boundary/filesystem/clientConfigurationTransactions.test.ts index 477085381..507b7137e 100644 --- a/tests/boundary/filesystem/clientConfigurationTransactions.test.ts +++ b/tests/boundary/filesystem/clientConfigurationTransactions.test.ts @@ -1,13 +1,91 @@ import { chmod, readFile, writeFile } from "node:fs/promises"; +import { createHash } from "node:crypto"; 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 { + configureClientConfiguration, + inspectClientConfiguration, +} from "../../../src/application/SetupClientConfiguration.js"; import { systemUninstallHost } from "../../../src/application/Uninstall.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +it("accepts a literal UTF-8 replacement character without changing its value", async () => { + const root = await createTestTempDirectory("rea-client-config-utf8-"); + const configPath = join(root, "config.json"); + const original = Buffer.concat([ + Buffer.from('{"label":"'), + Buffer.from("\uFFFD"), + Buffer.from('"}'), + ]); + await writeFile(configPath, original); + const result = await configureClientConfiguration( + { name: "cursor", format: "json", configPath }, + {}, + ["rea", "mcp"], + ); + expect(result.status).toBe("configured"); + expect(await readFile(`${configPath}.rea.backup`)).toEqual(original); + expect(JSON.parse(await readFile(configPath, "utf8")).label).toBe("\uFFFD"); +}); + +it("previews and creates a current recovery backup while preserving the first backup", async () => { + const root = await createTestTempDirectory("rea-client-current-recovery-"); + const configPath = join(root, "config.json"); + const firstOriginal = '{"keep":"first"}\n'; + const currentOriginal = '{"keep":"current"}\n'; + const client = { name: "cursor", format: "json", configPath } as const; + await writeFile(configPath, currentOriginal); + await writeFile(`${configPath}.rea.backup`, firstOriginal); + + const recoveryPath = `${configPath}.rea.backup.${createHash("sha256") + .update(currentOriginal, "utf8") + .digest("hex")}.recovery`; + expect(await inspectClientConfiguration(client, {}, ["rea", "mcp"])).toEqual({ + status: "update", + backupPath: recoveryPath, + }); + + expect( + await configureClientConfiguration(client, {}, ["rea", "mcp"]), + ).toEqual({ status: "configured", backupPath: recoveryPath }); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe( + firstOriginal, + ); + expect(await readFile(recoveryPath, "utf8")).toBe(currentOriginal); +}); + +it("blocks configuration when the planned content-addressed backup is occupied", async () => { + const root = await createTestTempDirectory("rea-client-recovery-conflict-"); + const configPath = join(root, "config.json"); + const firstOriginal = '{"keep":"first"}\n'; + const currentOriginal = '{"keep":"current"}\n'; + const client = { name: "cursor", format: "json", configPath } as const; + await writeFile(configPath, currentOriginal); + await writeFile(`${configPath}.rea.backup`, firstOriginal); + const recoveryPath = `${configPath}.rea.backup.${createHash("sha256") + .update(currentOriginal, "utf8") + .digest("hex")}.recovery`; + await writeFile(recoveryPath, "occupied by other data"); + + expect( + await inspectClientConfiguration(client, {}, ["rea", "mcp"]), + ).toMatchObject({ + status: "invalid", + remediation: expect.stringContaining(recoveryPath), + }); + expect( + await configureClientConfiguration(client, {}, ["rea", "mcp"]), + ).toEqual({ status: "failed", reason: "backup" }); + expect(await readFile(configPath, "utf8")).toBe(currentOriginal); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe( + firstOriginal, + ); + expect(await readFile(recoveryPath, "utf8")).toBe("occupied by other data"); +}); + describe("client configuration write failures", () => { it.skipIf(process.platform === "win32" || process.getuid?.() === 0).each([ ["json", configureClientConfiguration], diff --git a/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts b/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts index 3bf33c312..ab2b5e5a7 100644 --- a/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts +++ b/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts @@ -11,6 +11,7 @@ import { } from "../../../src/application/SetupHost.js"; import { RegularFileCleanupFailure } from "../../../src/application/RegularFileRead.js"; import { configureDetectedClients } from "../../../src/application/SetupClients.js"; +import { systemUninstallHost } from "../../../src/application/Uninstall.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; vi.mock("node:fs/promises", async (importOriginal) => { @@ -31,6 +32,61 @@ beforeEach(async () => { afterEach(() => openMock.mockReset()); +const holdReadHandle = (path: string, reason: string) => { + const closeAttempts: object[] = []; + let released = false; + openMock.mockImplementation(async (...args) => { + const handle = await actualFs.open(...args); + if (String(args[0]) !== path) return handle; + const nativeClose = handle.close.bind(handle); + vi.spyOn(handle, "close").mockImplementation(async () => { + closeAttempts.push(handle); + if (!released) throw new Error(reason); + await nativeClose(); + }); + return handle; + }); + return { + closeAttempts, + release: () => { + released = true; + }, + }; +}; + +it("retains invalid UTF-8 diagnostics and the uninstall read owner when close fails", async () => { + const home = await createTestTempDirectory("rea-uninstall-utf8-close-"); + const path = join(home, "client.json"); + const original = Buffer.from([0xff]); + await writeFile(path, original); + const { closeAttempts, release } = holdReadHandle( + path, + "held uninstall handle", + ); + const host = systemUninstallHost(home, undefined, { HOME: home }, "linux"); + try { + expect( + await host.removeClient({ + name: "cursor", + format: "json", + configPath: path, + }), + ).toMatchObject({ + status: "failed", + detail: expect.stringContaining("not valid UTF-8"), + }); + expect(await readFile(path)).toEqual(original); + expect(closeAttempts).toHaveLength(2); + release(); + expect(await host.close?.()).toBeUndefined(); + expect(closeAttempts).toHaveLength(3); + expect(closeAttempts[2]).toBe(closeAttempts[0]); + } finally { + release(); + await host.close?.(); + } +}); + describe("ZCode fallback cleanup ownership", () => { it.each(["preflight", "configure"])( "retains a failed ZCode fallback close during %s until host cleanup", @@ -49,19 +105,10 @@ describe("ZCode fallback cleanup ownership", () => { await mkdir(dirname(fallback), { recursive: true }); const original = '{"mcpServers":{"other":{"command":"other"}}}\n'; await writeFile(fallback, original); - const closeAttempts: object[] = []; - let allowClose = false; - openMock.mockImplementation(async (...args) => { - const handle = await actualFs.open(...args); - if (String(args[0]) !== fallback) return handle; - const nativeClose = handle.close.bind(handle); - vi.spyOn(handle, "close").mockImplementation(async () => { - closeAttempts.push(handle); - if (!allowClose) throw new Error("held fallback handle"); - await nativeClose(); - }); - return handle; - }); + const { closeAttempts, release } = holdReadHandle( + fallback, + "held fallback handle", + ); try { const action = operation === "preflight" @@ -79,12 +126,12 @@ describe("ZCode fallback cleanup ownership", () => { await expect(readFile(client.configPath)).rejects.toMatchObject({ code: "ENOENT", }); - allowClose = true; + release(); expect(await host.close?.()).toBeUndefined(); expect(closeAttempts).toHaveLength(3); expect(closeAttempts[2]).toBe(closeAttempts[0]); } finally { - allowClose = true; + release(); await host.close?.(); } }, @@ -92,83 +139,87 @@ describe("ZCode fallback cleanup ownership", () => { }); describe("setup client configuration cleanup ownership", () => { - it("stops client configuration and retries the same alignment handle from host close", async () => { - const home = await createTestTempDirectory("rea-setup-config-close-"); - const environment = { - HOME: home, - XDG_CONFIG_HOME: join(home, "xdg"), - }; - const host = systemSetupHost( - { platform: "linux", homeDirectory: home } as DoctorHost, - environment, - ); - const client = (await host.supportedClients()).find( - ({ name }) => name === "vscode", - ); - if (client === undefined) throw new Error("VS Code setup client missing"); - await mkdir(dirname(client.configPath), { recursive: true }); - const original = Buffer.from('{"servers": {"other": {}}}\n'); - await writeFile(client.configPath, original); + it.each([ + ["valid UTF-8", Buffer.from('{"servers": {"other": {}}}\n')], + ["malformed UTF-8", Buffer.from([0xff])], + ] as const)( + "stops configuration and retains the %s read owner until host close", + async (encoding, original) => { + const home = await createTestTempDirectory("rea-setup-config-close-"); + const environment = { + HOME: home, + XDG_CONFIG_HOME: join(home, "xdg"), + }; + const host = systemSetupHost( + { platform: "linux", homeDirectory: home } as DoctorHost, + environment, + ); + const client = (await host.supportedClients()).find( + ({ name }) => name === "vscode", + ); + if (client === undefined) throw new Error("VS Code setup client missing"); + await mkdir(dirname(client.configPath), { recursive: true }); + await writeFile(client.configPath, original); - const closeAttempts: object[] = []; - let allowClose = false; - openMock.mockImplementation(async (...args) => { - const handle = await actualFs.open(...args); - if (String(args[0]) !== client.configPath) return handle; - const nativeClose = handle.close.bind(handle); - vi.spyOn(handle, "close").mockImplementation(async () => { - closeAttempts.push(handle); - if (!allowClose) throw new Error("test holds the config handle open"); - await nativeClose(); - }); - return handle; - }); - const configure = vi.spyOn(host, "configureClient"); + const { closeAttempts, release } = holdReadHandle( + client.configPath, + "test holds the config handle open", + ); + const configure = vi.spyOn(host, "configureClient"); - try { - const failure = await filterClientsNeedingConfigure(host, [client], {}, [ - "rea", - "mcp", - ]) - .then((detectedClients) => - configureDetectedClients({ - host, - detectedClients, - providerEnvironment: {}, - command: ["rea", "mcp"], - clients: {}, - appliedActions: [], - }), + try { + const failure = await filterClientsNeedingConfigure( + host, + [client], + {}, + ["rea", "mcp"], ) - .then( - () => undefined, - (cause: unknown) => cause, - ); - expect(failure).toBeInstanceOf(ConfigurationError); - expect(failure).toMatchObject({ - _tag: "ConfigurationError", - cause: expect.any(RegularFileCleanupFailure), - cleanup: { - resources: [client.configPath], - reason: expect.stringContaining("test holds the config handle open"), - }, - }); - expect(configure).not.toHaveBeenCalled(); - expect(closeAttempts).toHaveLength(2); - expect(closeAttempts[0]).toBe(closeAttempts[1]); - expect(await readFile(client.configPath)).toEqual(original); + .then((detectedClients) => + configureDetectedClients({ + host, + detectedClients, + providerEnvironment: {}, + command: ["rea", "mcp"], + clients: {}, + appliedActions: [], + }), + ) + .then( + () => undefined, + (cause: unknown) => cause, + ); + expect(failure).toBeInstanceOf(ConfigurationError); + if (encoding === "malformed UTF-8") + expect(failure).toMatchObject({ + cause: { + outcome: { kind: "failed", cause: expect.any(TypeError) }, + }, + }); + expect(failure).toMatchObject({ + _tag: "ConfigurationError", + cause: expect.any(RegularFileCleanupFailure), + cleanup: { + resources: [client.configPath], + reason: expect.stringContaining( + "test holds the config handle open", + ), + }, + }); + expect(configure).not.toHaveBeenCalled(); + expect(closeAttempts).toHaveLength(2); + expect(closeAttempts[0]).toBe(closeAttempts[1]); + expect(await readFile(client.configPath)).toEqual(original); - allowClose = true; - const close = host.close; - if (close === undefined) throw new Error("Setup host close is missing"); - expect(await close()).toBeUndefined(); - expect(closeAttempts).toHaveLength(3); - expect(closeAttempts[2]).toBe(closeAttempts[0]); - } finally { - allowClose = true; - await host.close?.(); - } - }); + release(); + expect(await host.close?.()).toBeUndefined(); + expect(closeAttempts).toHaveLength(3); + expect(closeAttempts[2]).toBe(closeAttempts[0]); + } finally { + release(); + await host.close?.(); + } + }, + ); it("waits for an admitted configuration read and rejects later admissions", async () => { const home = await createTestTempDirectory("rea-setup-config-inflight-"); diff --git a/tests/process-global/setupConfigurationRollback.test.ts b/tests/process-global/setupConfigurationRollback.test.ts new file mode 100644 index 000000000..cf6504c32 --- /dev/null +++ b/tests/process-global/setupConfigurationRollback.test.ts @@ -0,0 +1,325 @@ +import { open, readFile } from "node:fs/promises"; +import { createHash } from "node:crypto"; +import { join } from "node:path"; + +import { afterEach, expect, it, onTestFinished, vi } from "vitest"; + +const state = vi.hoisted(() => ({ + readbackFailed: false, + rejectRestore: false, + rejectRemove: false, + removeCopiedRecovery: false, + initialCleanupFailure: undefined as unknown, + readbackCleanupFailure: undefined as unknown, +})); + +afterEach(() => { + state.readbackFailed = false; + state.rejectRestore = false; + state.rejectRemove = false; + state.removeCopiedRecovery = false; + state.initialCleanupFailure = undefined; + state.readbackCleanupFailure = undefined; +}); + +vi.mock("node:fs/promises", async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + rm: ( + ...arguments_: Parameters + ): ReturnType => { + if (state.rejectRemove && state.readbackFailed) + return Promise.reject(new Error("injected restore remove failure")); + return original.rm(...arguments_); + }, + writeFile: ( + ...arguments_: Parameters + ): ReturnType => { + if (state.rejectRestore && state.readbackFailed) + return Promise.reject(new Error("injected restore write failure")); + return original.writeFile(...arguments_); + }, + copyFile: async ( + ...arguments_: Parameters + ): Promise => { + await original.copyFile(...arguments_); + const destination = arguments_[1]; + if ( + state.removeCopiedRecovery && + typeof destination === "string" && + destination.endsWith(".recovery") + ) + await original.rm(destination); + }, + }; +}); + +vi.mock("write-file-atomic", async (importOriginal) => { + const original = await importOriginal<{ + default: typeof import("write-file-atomic"); + }>(); + return { + ...original, + default: async (...arguments_: Parameters) => { + if (state.initialCleanupFailure !== undefined) + throw new Error("unexpected config write before initial read"); + const result = await original.default(...arguments_); + state.readbackFailed = true; + return result; + }, + }; +}); + +vi.mock("../../src/application/RegularFileRead.js", async (importOriginal) => { + const original = + await importOriginal< + typeof import("../../src/application/RegularFileRead.js") + >(); + return { + ...original, + readRegularFileText: ( + ...arguments_: Parameters + ) => + state.initialCleanupFailure !== undefined + ? Promise.reject(state.initialCleanupFailure) + : state.readbackFailed && state.readbackCleanupFailure !== undefined + ? Promise.reject(state.readbackCleanupFailure) + : state.readbackFailed + ? Promise.reject(new Error("injected readback failure")) + : original.readRegularFileText(...arguments_), + }; +}); + +import { configureClientConfiguration } from "../../src/application/SetupClientConfiguration.js"; +import { RegularFileCleanupFailure } from "../../src/application/RegularFileRead.js"; +import { OwnedFileHandle } from "../../src/filesystem/OwnedFileHandle.js"; +import type { DoctorHost } from "../../src/application/Doctor.js"; +import { systemSetupHost } from "../../src/application/SetupHost.js"; +import { ConfigurationError } from "../../src/domain/configurationErrors.js"; +import { createTestTempDirectory } from "../fixtures/temporaryDirectory.js"; + +const ownReadHandle = async (path: string) => { + const owner = new OwnedFileHandle(await open(path, "r")); + onTestFinished(() => owner.close()); + return owner; +}; + +it("reports a failed config rollback and retains its backup path", async () => { + state.readbackFailed = false; + state.rejectRestore = false; + state.rejectRemove = false; + state.initialCleanupFailure = undefined; + state.readbackCleanupFailure = undefined; + const root = await createTestTempDirectory("rea-config-rollback-"); + const path = join(root, "client.json"); + const original = '{"keep":true}\n'; + await import("node:fs/promises").then(({ writeFile }) => + writeFile(path, original), + ); + state.rejectRestore = true; + + const result = await configureClientConfiguration( + { name: "cursor", configPath: path, format: "json" }, + {}, + ["rea", "mcp"], + ); + + expect(result).toEqual({ + status: "failed", + reason: "rollback", + backupPath: `${path}.rea.backup`, + failure: { + kind: "restore", + path, + detail: "injected restore write failure", + }, + }); + expect(await readFile(`${path}.rea.backup`, "utf8")).toBe(original); + expect(JSON.parse(await readFile(path, "utf8"))).toMatchObject({ + mcpServers: { rea: { command: "rea", args: ["mcp"] } }, + }); +}); + +it("preserves the first backup and reports a verified backup of the current original", async () => { + state.readbackFailed = false; + state.rejectRestore = false; + state.rejectRemove = false; + state.initialCleanupFailure = undefined; + state.readbackCleanupFailure = undefined; + const root = await createTestTempDirectory("rea-config-stale-backup-"); + const path = join(root, "client.json"); + const firstOriginal = '{"keep":"first"}\n'; + const currentOriginal = '{"keep":"current"}\n'; + await import("node:fs/promises").then(async ({ writeFile }) => { + await writeFile(path, currentOriginal); + await writeFile(`${path}.rea.backup`, firstOriginal); + }); + state.rejectRestore = true; + + const result = await configureClientConfiguration( + { name: "cursor", configPath: path, format: "json" }, + {}, + ["rea", "mcp"], + ); + state.rejectRestore = false; + + const recoveryPath = `${path}.rea.backup.${createHash("sha256") + .update(currentOriginal, "utf8") + .digest("hex")}.recovery`; + expect(result).toMatchObject({ + status: "failed", + reason: "rollback", + backupPath: recoveryPath, + failure: { + kind: "restore", + path, + detail: "injected restore write failure", + }, + }); + expect(await readFile(`${path}.rea.backup`, "utf8")).toBe(firstOriginal); + expect(await readFile(recoveryPath, "utf8")).toBe(currentOriginal); +}); + +it("does not write config when a recovery copy disappears before verification", async () => { + state.readbackFailed = false; + state.rejectRestore = false; + state.rejectRemove = false; + state.removeCopiedRecovery = false; + state.initialCleanupFailure = undefined; + state.readbackCleanupFailure = undefined; + const root = await createTestTempDirectory("rea-config-vanished-recovery-"); + const path = join(root, "client.json"); + const firstOriginal = '{"keep":"first"}\n'; + const currentOriginal = '{"keep":"current"}\n'; + await import("node:fs/promises").then(async ({ writeFile }) => { + await writeFile(path, currentOriginal); + await writeFile(`${path}.rea.backup`, firstOriginal); + }); + state.removeCopiedRecovery = true; + + const result = await configureClientConfiguration( + { name: "cursor", configPath: path, format: "json" }, + {}, + ["rea", "mcp"], + ); + state.removeCopiedRecovery = false; + + expect(result).toEqual({ status: "failed", reason: "backup" }); + expect(await readFile(path, "utf8")).toBe(currentOriginal); + expect(await readFile(`${path}.rea.backup`, "utf8")).toBe(firstOriginal); +}); + +it("does not roll back or remove config when its initial read cleanup fails", async () => { + state.readbackFailed = false; + state.rejectRestore = false; + state.rejectRemove = false; + state.readbackCleanupFailure = undefined; + const root = await createTestTempDirectory("rea-config-initial-cleanup-"); + const path = join(root, "client.json"); + const original = '{"keep":true}\n'; + await import("node:fs/promises").then(({ writeFile }) => + writeFile(path, original), + ); + const failure = new RegularFileCleanupFailure( + path, + await ownReadHandle(path), + { kind: "completed", value: original }, + new Error("injected initial close failure"), + ); + state.initialCleanupFailure = failure; + + await expect( + configureClientConfiguration( + { name: "cursor", configPath: path, format: "json" }, + {}, + ["rea", "mcp"], + ), + ).rejects.toBe(failure); + expect(await readFile(path, "utf8")).toBe(original); +}); + +it("reports first-run rollback removal failure without a backup", async () => { + state.readbackFailed = false; + state.rejectRestore = false; + state.rejectRemove = false; + state.initialCleanupFailure = undefined; + state.readbackCleanupFailure = undefined; + const root = await createTestTempDirectory("rea-config-first-run-rollback-"); + const path = join(root, "client.json"); + state.rejectRemove = true; + + const result = await configureClientConfiguration( + { name: "cursor", configPath: path, format: "json" }, + {}, + ["rea", "mcp"], + ); + state.rejectRemove = false; + + expect(result).toMatchObject({ + status: "failed", + reason: "rollback", + failure: { + kind: "restore", + path, + detail: "injected restore remove failure", + }, + }); + expect(result).not.toHaveProperty("backupPath"); + expect(JSON.parse(await readFile(path, "utf8"))).toMatchObject({ + mcpServers: { rea: { command: "rea", args: ["mcp"] } }, + }); +}); + +it("retains readback cleanup ownership when rollback also fails", async () => { + state.readbackFailed = false; + state.rejectRestore = false; + state.rejectRemove = false; + state.initialCleanupFailure = undefined; + const root = await createTestTempDirectory("rea-config-readback-cleanup-"); + const path = join(root, "client.json"); + const original = '{"keep":true}\n'; + await import("node:fs/promises").then(({ writeFile }) => + writeFile(path, original), + ); + const owner = await ownReadHandle(path); + const cleanupFailure = new RegularFileCleanupFailure( + path, + owner, + { kind: "completed", value: "" }, + new Error("injected readback close failure"), + ); + state.readbackCleanupFailure = cleanupFailure; + state.rejectRestore = true; + + const host = systemSetupHost( + { platform: "linux", homeDirectory: root } as DoctorHost, + { HOME: root }, + ); + let caught: unknown; + try { + await host.configureClient( + { name: "cursor", configPath: path, format: "json" }, + {}, + ["rea", "mcp"], + ); + } catch (cause: unknown) { + caught = cause; + } finally { + await host.close?.(); + } + + if (!(caught instanceof ConfigurationError)) throw caught; + expect(caught.message).toContain("injected restore write failure"); + expect(caught.message).toContain(`${path}.rea.backup`); + const cleanupOwnerError = caught.cause; + if (!(cleanupOwnerError instanceof RegularFileCleanupFailure)) + throw new Error("Configuration failure lost its read owner"); + expect(cleanupOwnerError).toMatchObject({ + path, + outcome: { kind: "completed", value: "" }, + cleanupCause: expect.any(AggregateError), + }); + expect(cleanupOwnerError.owner).toBe(owner); + expect(await readFile(`${path}.rea.backup`, "utf8")).toBe(original); +});