From 5cc841d5c154f692bdc257e106cc4b212dc19dc0 Mon Sep 17 00:00:00 2001 From: Kaoru0822 <208281322+kaoru0822-kitauji@users.noreply.github.com> Date: Fri, 9 Oct 2026 03:07:40 +0000 Subject: [PATCH] fix(setup): stop uninstall before removals when a client config is unsafe The installation guide says malformed client configuration stops `rea uninstall`, but uninstall reported that client as failed and still removed other clients' registrations, the bundled skill, and with --purge-data REA's cache and state. Inspect every client configuration first, using the conditions setup already treats as blocking (unsafe path, unreadable file, malformed document), and stop before any removal when one fails. Purge paths that are symbolic links are still retained and reported per item, as the guide now states. Fixes #1164 Co-Authored-By: Claude Opus 5.5 --- docs/installation.md | 6 +- src/application/Uninstall.ts | 127 +++++++++++++----- src/cliLogging.test.ts | 1 + .../filesystem/clientLifecycle.test.ts | 47 +++++++ 4 files changed, 148 insertions(+), 33 deletions(-) diff --git a/docs/installation.md b/docs/installation.md index 17c9be092..1bc9fa286 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -573,8 +573,10 @@ rea uninstall --purge-data Uninstall preserves Hopper, Node.js, Evidence files, captures, unrelated skills and other MCP servers. Purging removes only REA's cache and state under -`~/.rea`; malformed client configuration or purge-path symlinks stop the -operation. See the [CLI guide](cli.md#output-and-exit-status) for exit statuses. +`~/.rea`. A client configuration that is malformed, unreadable, or at an unsafe +path stops the operation before anything is removed; a purge path that is a +symbolic link is retained and reported rather than followed. See the +[CLI guide](cli.md#output-and-exit-status) for exit statuses. ## MCP Registry diff --git a/src/application/Uninstall.ts b/src/application/Uninstall.ts index d0203ae2f..0f926d440 100644 --- a/src/application/Uninstall.ts +++ b/src/application/Uninstall.ts @@ -81,21 +81,39 @@ export const isUninstallFailure = (result: UninstallResult): boolean => { /** Filesystem boundary used by the contained uninstall workflow. */ export interface UninstallHost { clients(): Promise; + /** Report a configuration that cannot be read safely, before any removal. */ + inspectClient(client: SetupClient): Promise; removeClient(client: SetupClient): Promise; removeSkill(): Promise; purgeData(): Promise; } -/** Remove only REA-owned registrations and managed files, optionally including local state. */ +/** + * Remove only REA-owned registrations and managed files, optionally including + * local state. A client configuration that cannot be read safely stops the + * operation before anything is removed. + */ export const runUninstall = async ( purgeData: boolean, host: UninstallHost = systemUninstallHost(), ): Promise => { - const items: UninstallItem[] = []; - for (const client of await host.clients()) - items.push(await host.removeClient(client)); - items.push(await host.removeSkill()); - if (purgeData) items.push(...(await host.purgeData())); + const clients = await host.clients(); + const blocked: UninstallItem[] = []; + for (const client of clients) { + const blocking = await host.inspectClient(client); + if (blocking !== undefined) blocked.push(blocking); + } + const items: UninstallItem[] = + blocked.length > 0 + ? [ + ...blocked, + item( + "uninstall", + "skipped", + "No REA registration, skill, or data was removed. Repair each failed configuration, then rerun uninstall.", + ), + ] + : await removeAll(clients, purgeData, host); items.push({ name: "analysis_engine", status: "retained", @@ -109,12 +127,30 @@ export const runUninstall = async ( }; }; +const removeAll = async ( + clients: readonly SetupClient[], + purgeData: boolean, + host: UninstallHost, +): Promise => { + const items: UninstallItem[] = []; + for (const client of clients) items.push(await host.removeClient(client)); + items.push(await host.removeSkill()); + if (purgeData) items.push(...(await host.purgeData())); + return items; +}; + /** Create uninstall effects contained to detected client configs and REA-owned paths. */ export const systemUninstallHost = ( home = homedir(), fileSystem: UninstallFileSystem = systemFileSystem, ): UninstallHost => ({ clients: () => Promise.resolve(supportedClients(home)), + inspectClient: async (client) => { + const read = await readClientConfiguration(client, fileSystem); + return read.kind === "item" && read.item.status === "failed" + ? read.item + : undefined; + }, removeClient: (client) => removeClient(client, fileSystem), removeSkill: () => removeManagedSkills(home, fileSystem), purgeData: async () => [ @@ -123,43 +159,72 @@ export const systemUninstallHost = ( ], }); -const removeClient = async ( +type ClientConfigurationRead = + | { readonly kind: "item"; readonly item: UninstallItem } + | { + readonly kind: "configuration"; + readonly transactionPath: string; + readonly original: string; + readonly parsed: ClientConfigurationDocument; + }; + +const readClientConfiguration = async ( client: SetupClient, fileSystem: UninstallFileSystem, -): Promise => { +): Promise => { if (client.format === "unsupported") - return item( - client.name, - "skipped", - manualRegistrationRemediation(client.name) ?? - "This client has no documented local MCP configuration boundary.", + return itemRead( + item( + client.name, + "skipped", + manualRegistrationRemediation(client.name) ?? + "This client has no documented local MCP configuration boundary.", + ), ); const resolved = await resolveUninstallConfigPath(client, fileSystem); - if (typeof resolved !== "string") return resolved; + if (typeof resolved !== "string") return itemRead(resolved); const transactionPath = resolved; let original: string; try { original = await fileSystem.readText(transactionPath); } catch (cause: unknown) { - return 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.", - ); - } - let parsed: ClientConfigurationDocument; - try { - parsed = parseClientConfiguration(original, client.format); - } catch (cause: unknown) { - void cause; - return item( - client.name, - "failed", - `Configuration is not valid ${client.format === "toml" || client.format === "grok" ? "TOML" : client.format === "opencode" ? "JSONC" : "JSON"} and was not changed. Repair it, then rerun uninstall.`, + 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.", + ), ); } + try { + const parsed = parseClientConfiguration(original, client.format); + return { kind: "configuration", transactionPath, original, parsed }; + } catch (cause: unknown) { + void cause; + return itemRead( + item( + client.name, + "failed", + `Configuration is not valid ${client.format === "toml" || client.format === "grok" ? "TOML" : client.format === "opencode" ? "JSONC" : "JSON"} and was not changed. Repair it, then rerun uninstall.`, + ), + ); + } +}; + +const itemRead = (result: UninstallItem): ClientConfigurationRead => ({ + kind: "item", + item: result, +}); + +const removeClient = async ( + client: SetupClient, + fileSystem: UninstallFileSystem, +): Promise => { + const read = await readClientConfiguration(client, fileSystem); + if (read.kind === "item") return read.item; + const { transactionPath, original, parsed } = read; const name = PRODUCT_IDENTITY.mcpServerKey; // Beside a native OpenCode V2 table, OpenCode also loads a V1 `mcp.rea`. const legacyPath = legacyClientServerPath(parsed, name); diff --git a/src/cliLogging.test.ts b/src/cliLogging.test.ts index e275519e6..dc0544109 100644 --- a/src/cliLogging.test.ts +++ b/src/cliLogging.test.ts @@ -145,6 +145,7 @@ describe("CLI command outcome classification", () => { it("returns nonzero when the real uninstall outcome contains a failed action", async () => { const host: UninstallHost = { clients: () => Promise.resolve([]), + inspectClient: () => Promise.resolve(undefined), removeClient: () => Promise.resolve({ name: "client", status: "failed", detail: "write" }), removeSkill: () => diff --git a/tests/boundary/filesystem/clientLifecycle.test.ts b/tests/boundary/filesystem/clientLifecycle.test.ts index 8ea5a9945..2c66bd42d 100644 --- a/tests/boundary/filesystem/clientLifecycle.test.ts +++ b/tests/boundary/filesystem/clientLifecycle.test.ts @@ -1418,6 +1418,53 @@ describe("client configuration filesystem removal", () => { ); expect(await readFile(config, "utf8")).toBe("not-json"); }); + + it("stops before any removal when one client configuration is malformed", async () => { + const home = await createTestTempDirectory("rea-uninstall-stop-"); + roots.push(home); + const codex = join(home, ".codex/config.toml"); + const cursor = join(home, ".cursor/mcp.json"); + const managed = [ + join(home, ".agents/skills/reverse-engineer-anything/marker.txt"), + join(home, ".rea/cache/marker.txt"), + join(home, ".rea/state/marker.txt"), + ]; + const cursorConfig = JSON.stringify({ + mcpServers: { + rea: { command: "npx", args: ["-y", "rea-agents@6.1.0", "mcp"] }, + other: { command: "echo", args: ["hello"] }, + }, + }); + for (const path of [codex, cursor, ...managed]) + await mkdir(dirname(path), { recursive: true }); + await writeFile(codex, "[mcp_servers.rea]\ncommand = [broken\n"); + await writeFile(cursor, cursorConfig); + for (const path of managed) await writeFile(path, "marker"); + + const stopped = await runUninstall(true, systemUninstallHost(home)); + + expect(stopped.status).toBe("failed"); + expect(stopped.items.map(({ name, status }) => [name, status])).toEqual([ + ["codex", "failed"], + ["uninstall", "skipped"], + ["analysis_engine", "retained"], + ]); + expect(await readFile(codex, "utf8")).toBe( + "[mcp_servers.rea]\ncommand = [broken\n", + ); + expect(await readFile(cursor, "utf8")).toBe(cursorConfig); + for (const path of managed) + expect(await readFile(path, "utf8")).toBe("marker"); + + await writeFile(codex, ""); + const repaired = await runUninstall(true, systemUninstallHost(home)); + expect(repaired.status).toBe("complete"); + expect(JSON.parse(await readFile(cursor, "utf8"))).toEqual({ + mcpServers: { other: { command: "echo", args: ["hello"] } }, + }); + for (const path of managed) + await expect(readFile(path, "utf8")).rejects.toThrow(); + }); }); describe("client configuration filesystem failure reporting", () => {