diff --git a/docs/installation.md b/docs/installation.md index 7f4cb1342..628b6b600 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -376,7 +376,12 @@ Uninstall preserves Hopper, Node.js, Evidence files, captures, unrelated skills and other MCP servers. Purging removes only REA's cache and state under `~/.rea`. A client configuration that is malformed, unreadable, or at an unsafe path stops the operation before anything is removed, and a client that fails -while being updated stops the remaining removals. A purge path that is a +while being updated stops the remaining removals. For JSON clients, uninstall +removes every repeated `rea` member in the effective server table only when +each registration is REA-owned. An unowned earlier member, or an unowned +OpenCode V1 entry that removing V2 would expose, stops preflight before any +removal. Repeats inside an inactive earlier copy of the server table stay +unchanged. A purge path that is a symbolic link is retained and reported rather than followed. Uninstall checks REA's managed shared and Claude Code personal skill locations. See the [CLI guide](cli.md#output-and-exit-status) for exit statuses. diff --git a/src/application/ClientConfigurationDocument.repeatedMembers.test.ts b/src/application/ClientConfigurationDocument.repeatedMembers.test.ts new file mode 100644 index 000000000..f24541240 --- /dev/null +++ b/src/application/ClientConfigurationDocument.repeatedMembers.test.ts @@ -0,0 +1,165 @@ +import { describe, expect, it } from "vitest"; +import { parse as parseJsonc, type ParseError } from "jsonc-parser"; + +import { + clientServerPath, + parseClientConfiguration, + serializeClientConfiguration, + withClientServers, + type ClientConfigurationEditOptions, + type ClientConfigurationFormat, +} from "./ClientConfigurationDocument.js"; + +const owned = { command: "rea", args: ["mcp"] }; +const foreign = { command: "/usr/bin/false" }; + +const removeRea = ( + text: string, + format: ClientConfigurationFormat, + options?: ClientConfigurationEditOptions, +) => { + const parsed = parseClientConfiguration(text, format); + const { rea: _removed, ...remaining } = parsed.servers; + return serializeClientConfiguration( + withClientServers(parsed, remaining), + format, + text, + [clientServerPath(parsed, "rea")], + options, + ); +}; + +const parseStrictJson = (text: string): Record => + JSON.parse(text.replace(/^\uFEFF/u, "")) as Record; + +describe("JSON client configuration repeated-member edits", () => { + it("keeps the default refusal for repeated deletions", () => { + const text = + `{"mcpServers":{"rea":${JSON.stringify(foreign)},` + + `"rea":${JSON.stringify(owned)}}}`; + expect(removeRea(text, "claude_code")).toEqual({ + ok: false, + error: { + kind: "shadowed_member", + path: ["mcpServers", "rea"], + }, + }); + }); + + it("removes all authorized repeats and preserves peer values in strict JSON", () => { + const text = + `{"mcpServers":{"rea":${JSON.stringify(foreign)},` + + `"peer":{"command":"peer"},` + + `"rea":${JSON.stringify(owned)},` + + `"other":{"args":[1,2]},` + + `"rea":${JSON.stringify(owned)}}}`; + const parsed = parseClientConfiguration(text, "claude_code"); + const path = clientServerPath(parsed, "rea"); + const result = removeRea(text, "claude_code", { + removeShadowedMembersAt: [path], + }); + + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("authorized repeats were refused"); + expect(parseStrictJson(result.value)).toEqual({ + mcpServers: { + peer: { command: "peer" }, + other: { args: [1, 2] }, + }, + }); + expect(result.value).not.toContain('"rea"'); + }); + + it("does not apply repeat-removal authority to a different path", () => { + const text = `{"mcpServers":{"rea":1,"rea":2,"peer":3,"peer":4}}`; + const parsed = parseClientConfiguration(text, "json"); + const document = withClientServers(parsed, {}); + const result = serializeClientConfiguration( + document, + "json", + text, + [clientServerPath(parsed, "rea"), clientServerPath(parsed, "peer")], + { removeShadowedMembersAt: [clientServerPath(parsed, "rea")] }, + ); + + expect(result).toEqual({ + ok: false, + error: { + kind: "shadowed_member", + path: ["mcpServers", "peer"], + }, + }); + }); + + it.each([ + ["no registration", `{"mcpServers":{"peer":1}}`, { peer: 1 }, undefined], + [ + "one registration", + `{"mcpServers":{"rea":1,"peer":2}}`, + { peer: 2 }, + undefined, + ], + [ + "multiple registrations", + `{"mcpServers":{"rea":1,"peer":2,"rea":3,"other":4,"rea":5}}`, + { peer: 2, other: 4 }, + { removeShadowedMembersAt: [["mcpServers", "rea"]] }, + ], + ] as const)( + "handles %s without losing peers", + (_label, text, expectedServers, authority) => { + const result = removeRea(text, "json", authority); + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("non-shadowed removal was refused"); + expect(parseStrictJson(result.value)).toEqual({ + mcpServers: expectedServers, + }); + }, + ); + + it("matches escaped keys and retains a repeated inactive parent byte for byte", () => { + const inactive = `{"rea":${JSON.stringify(foreign)}}`; + const text = + `{"mcpServers":${inactive},` + + `"mcpServers":{"r\\u0065a":${JSON.stringify(foreign)},` + + `"peer":2,"rea":${JSON.stringify(owned)}}}`; + const result = removeRea(text, "claude_code", { + removeShadowedMembersAt: [["mcpServers", "rea"]], + }); + + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("authorized escaped repeat was refused"); + expect(result.value).toContain(`"mcpServers":${inactive}`); + expect(parseStrictJson(result.value)).toEqual({ + mcpServers: { peer: 2 }, + }); + }); + + it("preserves JSONC comments and parses BOM/CRLF output independently", () => { + const text = + `\uFEFF{\r\n // top comment\r\n "mcp": {\r\n` + + ` "r\\u0065a": ${JSON.stringify(foreign)}, // first\r\n` + + ` "peer": { "command": "peer" },\r\n` + + ` /* second */ "rea": ${JSON.stringify(owned)},\r\n` + + ` },\r\n}\r\n`; + const result = removeRea(text, "opencode", { + removeShadowedMembersAt: [["mcp", "rea"]], + }); + + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("authorized JSONC repeats were refused"); + expect(result.value).toContain("// top comment"); + expect(result.value).toContain("// first"); + expect(result.value).toContain("/* second */"); + expect(result.value.startsWith("\uFEFF")).toBe(true); + const errors: ParseError[] = []; + const document = parseJsonc(result.value.replace(/^\uFEFF/u, " "), errors, { + allowTrailingComma: true, + }); + expect(errors).toEqual([]); + expect(result.value.replaceAll("\r\n", "")).not.toContain("\n"); + expect(document).toEqual({ + mcp: { peer: { command: "peer" } }, + }); + }); +}); diff --git a/src/application/ClientConfigurationDocument.shape.test.ts b/src/application/ClientConfigurationDocument.shape.test.ts index 67c557a44..2992a2ddb 100644 --- a/src/application/ClientConfigurationDocument.shape.test.ts +++ b/src/application/ClientConfigurationDocument.shape.test.ts @@ -1,6 +1,9 @@ import { describe, expect, it } from "vitest"; -import { parseClientConfiguration } from "./ClientConfigurationDocument.js"; +import { + parseClientConfiguration, + shadowedClientServers, +} from "./ClientConfigurationDocument.js"; describe("parseClientConfiguration shape diagnostics", () => { it.each([ @@ -31,4 +34,25 @@ describe("parseClientConfiguration shape diagnostics", () => { SyntaxError, ); }); + + it("reports only duplicates in the effective server object", () => { + const owned = '{"command":"rea","args":["mcp"]}'; + const activeTable = `{"mcpServers":{"rea":null,"rea":${owned}}}`; + const active = parseClientConfiguration(activeTable, "json"); + expect(shadowedClientServers(activeTable, active, "rea")).toEqual({ + servers: [null], + legacyServers: [], + }); + + // Removing from the effective parent leaves an earlier repeated parent + // inactive. Its members are not registrations the client would reveal. + const inactiveParent = + `{"mcpServers":{"rea":{"command":"/usr/bin/false"}},` + + `"mcpServers":{"rea":${owned}}}`; + const parsed = parseClientConfiguration(inactiveParent, "json"); + expect(shadowedClientServers(inactiveParent, parsed, "rea")).toEqual({ + servers: [], + legacyServers: [], + }); + }); }); diff --git a/src/application/ClientConfigurationDocument.ts b/src/application/ClientConfigurationDocument.ts index c22c4c546..9ee88a285 100644 --- a/src/application/ClientConfigurationDocument.ts +++ b/src/application/ClientConfigurationDocument.ts @@ -16,6 +16,7 @@ import { applyEdits, createScanner, format as formatJson, + getNodeValue, parse as parseJsonc, parseTree, printParseErrorCode, @@ -1325,6 +1326,101 @@ const isJsonMember = (property: Node, key: string): boolean => const effectiveJsonProperty = (object: Node, key: string): Node | undefined => object.children?.findLast((property) => isJsonMember(property, key)); +/** The object a client reads at `path`, following the effective members. */ +const effectiveJsonObject = ( + root: Node | undefined, + path: readonly string[], +): Node | undefined => { + let node = root; + for (const key of path) { + if (node?.type !== "object") return undefined; + node = effectiveJsonProperty(node, key)?.children?.[1]; + } + return node?.type === "object" ? node : undefined; +}; + +/** + * Earlier members of one server name repeated in the tables a client reads, + * in document order. Parsing keeps only the last member, as clients do. + */ +export interface ShadowedClientServers { + readonly servers: readonly unknown[]; + /** Repeats among OpenCode V1 members beside a native V2 table. */ + readonly legacyServers: readonly unknown[]; +} + +const NO_SHADOWED_SERVERS: ShadowedClientServers = Object.freeze({ + servers: [], + legacyServers: [], +}); + +// Token codes follow the pinned jsonc-parser scanner, as JSON_COMMA does. +const JSON_STRING = 10 satisfies SyntaxKind; +const JSON_EOF = 17 satisfies SyntaxKind; + +/** A conservative duplicate check using the parser's own string decoder. */ +const hasRepeatedJsonString = (text: string, name: string): boolean => { + const scanner = createScanner(text, true); + let matches = 0; + for (let token = scanner.scan(); token !== JSON_EOF; token = scanner.scan()) { + if (token === JSON_STRING && scanner.getTokenValue() === name) { + matches += 1; + if (matches === 2) return true; + } + } + return false; +}; + +/** Values of the earlier members named `name` in `object`. */ +const shadowedJsonMembers = ( + object: Node | undefined, + name: string, +): readonly unknown[] => + (object?.children ?? []) + .filter((property) => isJsonMember(property, name)) + .slice(0, -1) + .map((property): unknown => { + const value = property.children?.[1]; + return value === undefined ? undefined : getNodeValue(value); + }); + +/** + * Earlier members of `name` that removing its effective entry would make + * effective, so a removal decision must cover them too. Locating them needs a + * syntax tree of the whole document, which is built only when the name's + * string token occurs more than once. + */ +export const shadowedClientServers = ( + text: string, + parsed: ClientConfigurationDocument, + name: string, +): ShadowedClientServers => { + // TOML rejects repeated keys, Hermes parses YAML with unique keys, and the + // Harness owner edits REA's entry by id rather than through a server table. + if ( + parsed.dialect === "toml" || + parsed.dialect === "grok" || + parsed.dialect === "hermes" || + parsed.dialect === "dsh" || + !hasRepeatedJsonString(text, name) + ) + return NO_SHADOWED_SERVERS; + const root = parseTree(text, [], { allowTrailingComma: true }); + return { + servers: shadowedJsonMembers( + effectiveJsonObject(root, parsed.serversPath), + name, + ), + legacyServers: + parsed.dialect === "opencode_v2" && !OPENCODE_V2_MCP_SETTINGS.has(name) + ? shadowedJsonMembers( + effectiveJsonObject(root, parsed.serversPath.slice(0, 1)), + name, + ) + : [], + }; +}; + /** Apply one edit and format the lines it touched, as `modify` does. */ const applyFormattedJsonEdit = (text: string, edit: Edit): string => { const edited = applyEdits(text, [edit]); @@ -1345,38 +1441,32 @@ const applyFormattedJsonEdit = (text: string, edit: Edit): string => { ); }; -/** - * Delete a property by its JSONC AST span and the separator needed to keep its - * parent object valid. All whitespace and comments outside those tokens stay - * byte-for-byte in place. - */ -const deleteJsonProperty = ( +/** Remove selected member spans and only the separators needed by kept members. */ +const deleteJsonMembers = ( text: string, object: Node, - property: Node, + properties: readonly Node[], ): string => { const siblings = object.children ?? []; - const propertyIndex = siblings.indexOf(property); - const previous = siblings[propertyIndex - 1]; - const next = siblings[propertyIndex + 1]; - const comma = nextJsoncToken( - text, - previous === undefined - ? property.offset + property.length - : previous.offset + previous.length, - ); - if ( - propertyIndex < 0 || - (comma.kind !== JSON_COMMA && - (previous !== undefined || next !== undefined)) - ) - throw new TypeError("JSON property separator was not found"); - const edits = [ - { offset: property.offset, length: property.length, content: "" }, - ...(comma.kind === JSON_COMMA - ? [{ offset: comma.offset, length: 1, content: "" }] - : []), - ]; + const removed = new Set(properties); + const anyKept = siblings.some((member) => !removed.has(member)); + const edits: Edit[] = []; + let earlierKept = false; + for (const [index, member] of siblings.entries()) { + if (removed.has(member)) + edits.push({ offset: member.offset, length: member.length, content: "" }); + else earlierKept = true; + const separator = nextJsoncToken(text, member.offset + member.length); + const next = siblings[index + 1]; + if (separator.kind !== JSON_COMMA && next !== undefined) + throw new TypeError("JSON property separator was not found"); + // Keep the separator before the next retained member, matching the + // existing single-member deletion rule. JSONC may retain a trailing comma. + const keepSeparator = + next === undefined ? anyKept : earlierKept && !removed.has(next); + if (separator.kind === JSON_COMMA && !keepSeparator) + edits.push({ offset: separator.offset, length: 1, content: "" }); + } return applyEdits(text, edits); }; @@ -1391,12 +1481,13 @@ export interface ClientConfigurationShadowedMember { * Set or delete the member a client reads at `path`, keeping every byte * outside that member. Shadowed duplicates stay in place, and deleting a * member that shadows one is refused so the earlier member never becomes - * active, whatever its ownership. + * active, unless the caller has established removal authority for that path. */ const editJsonMember = ( text: string, path: readonly string[], value: unknown, + removeShadowedMembers: boolean, ): Result => { let object = parseTree(text, [], { allowTrailingComma: true }); for (const [index, key] of path.entries()) { @@ -1442,12 +1533,32 @@ const editJsonMember = ( (sibling) => sibling !== property && isJsonMember(sibling, key), ) ) - return err({ kind: "shadowed_member", path }); - return ok(deleteJsonProperty(text, object, property)); + return removeShadowedMembers + ? ok( + deleteJsonMembers( + text, + object, + (object.children ?? []).filter((sibling) => + isJsonMember(sibling, key), + ), + ), + ) + : err({ kind: "shadowed_member", path }); + return ok(deleteJsonMembers(text, object, [property])); } throw new TypeError("JSON edit path is empty"); }; +/** Edit authority beyond the members a client reads. */ +export interface ClientConfigurationEditOptions { + /** + * Exact deleted paths whose repeated members the caller may remove. + * Uninstall must establish ownership of every shadowed member; setup may + * grant replacement authority for an explicitly superseded registration. + */ + readonly removeShadowedMembersAt?: readonly (readonly string[])[]; +} + /** * Serialize a validated client document, retaining JSONC and TOML comments. * JSON edits fail only when they would activate a shadowed duplicate member. @@ -1457,6 +1568,7 @@ export const serializeClientConfiguration = ( format: ClientConfigurationFormat | undefined, originalText?: string, editedPaths?: readonly (readonly string[])[], + options: ClientConfigurationEditOptions = {}, ): Result => { if (format === undefined || format === "unsupported") throw new TypeError("client does not have a supported MCP config format"); @@ -1477,7 +1589,16 @@ export const serializeClientConfiguration = ( return ok(`${JSON.stringify(document, null, 2)}\n`); let text = originalText; for (const path of editedPaths) { - const edited = editJsonMember(text, path, valueAt(document, path)); + const edited = editJsonMember( + text, + path, + valueAt(document, path), + options.removeShadowedMembersAt?.some( + (allowed) => + allowed.length === path.length && + allowed.every((key, index) => key === path[index]), + ) ?? false, + ); if (!edited.ok) return edited; text = edited.value; } diff --git a/src/application/SetupClientConfiguration.ts b/src/application/SetupClientConfiguration.ts index 5d923b5df..a2d7dfc76 100644 --- a/src/application/SetupClientConfiguration.ts +++ b/src/application/SetupClientConfiguration.ts @@ -284,14 +284,22 @@ const persistClientConfiguration = async ({ : registered; let serialized: ReturnType; try { - serialized = serializeClientConfiguration(document, format, original, [ - ...Object.keys(fallbackServers) - .filter((name) => name !== PRODUCT_IDENTITY.mcpServerKey) - .map((name) => clientServerPath(parsed, name)), - clientServerPath(parsed, PRODUCT_IDENTITY.mcpServerKey), - ...(legacyPath === undefined ? [] : [legacyPath]), - ...(enableOmp ? [[OMP_DISABLED_SERVERS_KEY]] : []), - ]); + serialized = serializeClientConfiguration( + document, + format, + original, + [ + ...Object.keys(fallbackServers) + .filter((name) => name !== PRODUCT_IDENTITY.mcpServerKey) + .map((name) => clientServerPath(parsed, name)), + clientServerPath(parsed, PRODUCT_IDENTITY.mcpServerKey), + ...(legacyPath === undefined ? [] : [legacyPath]), + ...(enableOmp ? [[OMP_DISABLED_SERVERS_KEY]] : []), + ], + // Setup deletes only REA's OpenCode V1 name, which the native V2 entry + // supersedes whatever its launcher; every repeat goes, as one member does. + { removeShadowedMembersAt: legacyPath === undefined ? [] : [legacyPath] }, + ); } catch (cause: unknown) { // An unserializable edit is reported before any file is written. void cause; diff --git a/src/application/Uninstall.ts b/src/application/Uninstall.ts index 78f852d4b..70a705cc5 100644 --- a/src/application/Uninstall.ts +++ b/src/application/Uninstall.ts @@ -8,9 +8,11 @@ import { serializeClientConfiguration, clientServerPath, legacyClientServerPath, + shadowedClientServers, withClientServers, type ClientConfigurationDocument, } from "./ClientConfigurationDocument.js"; +import { err, ok, type Result } from "../domain/result.js"; import { copyFile, lstat, realpath, rm, rmdir } from "node:fs/promises"; import { constants as fsConstants } from "node:fs"; import { homeDirectoryFromEnvironment } from "../config/homeDirectory.js"; @@ -278,8 +280,17 @@ export const systemUninstallHost = ( ); if (read.kind === "item") return read.item.status === "failed" ? read.item : undefined; - const removal = clientRegistrationRemoval(read.parsed, client); - if (!removal.removeNative && removal.legacyPath === undefined) + const removal = clientRegistrationRemoval( + read.parsed, + read.original, + client, + ); + if (!removal.ok) + return item(client.name, "failed", removal.error.detail); + if ( + !removal.value.removeNative && + removal.value.legacyPath === undefined + ) return undefined; // A removal the running package would revert, or redirect into its // private storage, would leave the registration in place. @@ -447,33 +458,66 @@ const itemRead = (result: UninstallItem): ClientConfigurationRead => ({ * removable legacy entry carries its document path, so removal edits exactly * the entry this decision admitted. */ -const clientRegistrationRemoval = ( - parsed: ClientConfigurationDocument, - client: SetupClient, -): { +type RegistrationRemoval = { readonly removeNative: boolean; readonly legacyPath: readonly string[] | undefined; -} => { +}; +type RegistrationRemovalRefusal = { + readonly kind: "shadowed_unowned_member" | "foreign_legacy_fallback"; + readonly detail: string; +}; + +const clientRegistrationRemoval = ( + parsed: ClientConfigurationDocument, + original: string, + client: SetupClient, +): Result => { const name = PRODUCT_IDENTITY.mcpServerKey; if ( parsed.dialect === "dsh" && dshDocument(parsed.document).hasUnknownStructure ) - return { removeNative: false, legacyPath: undefined }; + return ok({ removeNative: false, legacyPath: undefined }); + const removeNative = + (parsed.dialect === "dsh" && dshDocument(parsed.document).carriesEntry) || + (Object.hasOwn(parsed.servers, name) && + isOwnedRegistration(parsed.servers[name], client)); const legacyPath = legacyClientServerPath(parsed, name); - return { - removeNative: - (parsed.dialect === "dsh" && dshDocument(parsed.document).carriesEntry) || - (Object.hasOwn(parsed.servers, name) && - isOwnedRegistration(parsed.servers[name], client)), - legacyPath: - legacyPath !== undefined && - isOwnedRegistration(parsed.legacyServers[name], client) - ? legacyPath - : undefined, - }; + const removeLegacy = + legacyPath !== undefined && + isOwnedRegistration(parsed.legacyServers[name], client); + if (removeNative || removeLegacy) { + const shadowed = shadowedClientServers(original, parsed, name); + if ( + (removeNative && + shadowed.servers.some( + (member) => !isOwnedRegistration(member, client), + )) || + (removeLegacy && + shadowed.legacyServers.some( + (member) => !isOwnedRegistration(member, client), + )) + ) + return err({ + kind: "shadowed_unowned_member", + detail: SHADOWED_UNOWNED_MEMBER_DETAIL, + }); + } + if (removeNative && legacyPath !== undefined && !removeLegacy) + return err({ + kind: "foreign_legacy_fallback", + detail: UNOWNED_LEGACY_FALLBACK_DETAIL, + }); + return ok({ + removeNative, + legacyPath: removeLegacy ? legacyPath : undefined, + }); }; +const SHADOWED_UNOWNED_MEMBER_DETAIL = `Configuration repeats the "${PRODUCT_IDENTITY.mcpServerKey}" member, and an earlier duplicate is not REA-owned; removing the registration the client reads would activate an earlier duplicate, so no change was made. Remove the duplicate members manually, then rerun uninstall.`; + +const UNOWNED_LEGACY_FALLBACK_DETAIL = `OpenCode falls back to the V1 "mcp.${PRODUCT_IDENTITY.mcpServerKey}" entry, which is not REA-owned; removing REA's native registration would activate it, so no change was made. Remove or replace that V1 entry manually, then rerun uninstall.`; + /** Preserve the bytes uninstall is about to edit and return the backup holding them. */ const backupClientConfiguration = async ( client: SetupClient, @@ -527,10 +571,9 @@ const removeClient = async ( ); if (!registrationPresent(parsed)) return item(client.name, "skipped", "REA registration is absent."); - const { removeNative, legacyPath } = clientRegistrationRemoval( - parsed, - client, - ); + const removal = clientRegistrationRemoval(parsed, original, client); + if (!removal.ok) return item(client.name, "failed", removal.error.detail); + const { removeNative, legacyPath } = removal.value; const removeLegacy = legacyPath !== undefined; if (!removeNative && !removeLegacy) return item( @@ -558,6 +601,8 @@ const removeClient = async ( client.format, original, editedPaths, + // clientRegistrationRemoval admitted only names whose repeats REA owns. + { removeShadowedMembersAt: editedPaths }, ); } catch { return item( diff --git a/tests/acceptance/setup/duplicateJsonMemberSetup.test.ts b/tests/acceptance/setup/duplicateJsonMemberSetup.test.ts index 20ed05da9..c7b57ecfc 100644 --- a/tests/acceptance/setup/duplicateJsonMemberSetup.test.ts +++ b/tests/acceptance/setup/duplicateJsonMemberSetup.test.ts @@ -1,5 +1,5 @@ import { mkdir, readFile, writeFile } from "node:fs/promises"; -import { join } from "node:path"; +import { dirname, join, resolve } from "node:path"; import { expect } from "vitest"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; @@ -26,6 +26,15 @@ const runIn = (cli: TestCli, home: string) => (arguments_: readonly string[]) => const setupArguments = (client: string) => ["setup", "--client", client, "--skill=false", "--json", "--yes"] as const; +const openCodeOwned = + '{"type":"local","command":["npx","-y","rea-agents@6.6.0","mcp"]}'; +const openCodeForeign = '{"type":"local","command":["/usr/bin/false"]}'; + +const writeConfiguration = async (path: string, text: string) => { + await mkdir(dirname(path), { recursive: true }); + await writeFile(path, text); +}; + for (const [layout, original] of [ [ "a repeated server table", @@ -142,3 +151,135 @@ cliTest( }); }, ); + +cliTest( + "uninstall removes repeated owned entries and preserves inactive parents, peers, and its original backup", + async ({ cli }) => { + const home = await createTestTempDirectory("rea-json-owned-repeats-"); + const config = join(home, ".claude.json"); + const inactive = '{"inactivePeer":{"command":"/usr/bin/false"}}'; + const active = `{"rea":{"command":"npx","args":["-y","rea-agents@6.6.0","mcp"]},"peer":{"command":"/usr/bin/true"},"rea":{"command":"npx","args":["-y","rea-agents@6.5.0","mcp"]}}`; + const original = `{"mcpServers":${inactive},"mcpServers":${active}}\n`; + await writeFile(config, original); + + const run = runIn(cli, home); + const removed = await run(["uninstall", "--json"]); + expect(removed.exitCode, removed.stdout).toBe(0); + expect(removed.json).toMatchObject({ + status: "complete", + items: expect.arrayContaining([ + expect.objectContaining({ name: "claude_code", status: "removed" }), + ]), + }); + const afterRemoval = await readFile(config, "utf8"); + expect(JSON.parse(afterRemoval)).toEqual({ + mcpServers: { peer: { command: "/usr/bin/true" } }, + }); + expect(afterRemoval).toContain(`"mcpServers":${inactive}`); + expect(afterRemoval).toContain('"peer":{"command":"/usr/bin/true"}'); + expect(afterRemoval).not.toContain("rea-agents@"); + expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original); + + const repeated = await run(["uninstall", "--json"]); + expect(repeated.exitCode, repeated.stdout).toBe(0); + expect(await readFile(config, "utf8")).toBe(afterRemoval); + expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original); + }, +); + +cliTest( + "preflights a later Cursor null duplicate before changing Claude or creating backups", + async ({ cli }) => { + const home = await createTestTempDirectory("rea-json-repeat-preflight-"); + const claude = join(home, ".claude.json"); + const claudeOriginal = `{"mcpServers":{${owned}}}\n`; + await writeFile(claude, claudeOriginal); + const cursor = join(home, ".cursor", "mcp.json"); + const cursorOriginal = `{"mcpServers":{"rea":null,${owned}}}\n`; + await writeConfiguration(cursor, cursorOriginal); + + const removed = await runIn(cli, home)(["uninstall", "--json"]); + expect(removed.exitCode, removed.stdout).toBe(1); + expect(removed.json).toMatchObject({ + status: "failed", + items: expect.arrayContaining([ + expect.objectContaining({ + name: "cursor", + status: "failed", + detail: expect.stringContaining("earlier duplicate"), + }), + expect.objectContaining({ name: "uninstall", status: "skipped" }), + ]), + }); + expect(await readFile(claude, "utf8")).toBe(claudeOriginal); + expect(await readFile(cursor, "utf8")).toBe(cursorOriginal); + for (const path of [claude, cursor]) + await expect(readFile(`${path}.rea.backup`)).rejects.toMatchObject({ + code: "ENOENT", + }); + }, +); + +cliTest( + "preflights a foreign OpenCode V1 fallback before changing Claude or creating backups", + async ({ cli }) => { + const home = await createTestTempDirectory("rea-json-opencode-fallback-"); + const claude = join(home, ".claude.json"); + const claudeOriginal = `{"mcpServers":{${owned}}}\n`; + await writeFile(claude, claudeOriginal); + const openCode = join(home, ".config", "opencode", "opencode.json"); + const openCodeOriginal = `{"mcp":{"servers":{"rea":${openCodeOwned}},"rea":${openCodeForeign}}}\n`; + await writeConfiguration(openCode, openCodeOriginal); + + const removed = await runIn(cli, home)(["uninstall", "--json"]); + expect(removed.exitCode, removed.stdout).toBe(1); + expect(removed.json).toMatchObject({ + status: "failed", + items: expect.arrayContaining([ + expect.objectContaining({ + name: "opencode", + status: "failed", + detail: expect.stringContaining("would activate it"), + }), + expect.objectContaining({ name: "uninstall", status: "skipped" }), + ]), + }); + expect(await readFile(claude, "utf8")).toBe(claudeOriginal); + expect(await readFile(openCode, "utf8")).toBe(openCodeOriginal); + for (const path of [claude, openCode]) + await expect(readFile(`${path}.rea.backup`)).rejects.toMatchObject({ + code: "ENOENT", + }); + }, +); + +cliTest( + "OpenCode setup replaces every repeated V1 entry while preserving the V2 peer", + async ({ cli }) => { + const home = await createTestTempDirectory("rea-json-opencode-migration-"); + const config = join(home, ".config", "opencode", "opencode.json"); + const original = `{"mcp":{"servers":{"peer":{"type":"local","command":["/usr/bin/true"]}},"rea":${openCodeForeign},"rea":${openCodeForeign}}}\n`; + await writeConfiguration(config, original); + + const setup = await runIn(cli, home)(setupArguments("opencode")); + expect(setup.exitCode, setup.stdout).toBe(0); + expect(setup.json).toMatchObject({ + clients: { opencode: { status: "configured" } }, + }); + const { mcp } = JSON.parse(await readFile(config, "utf8")) as { + mcp: Record & { servers: Record }; + }; + expect(Object.hasOwn(mcp, "rea")).toBe(false); + const launcher = resolve("scripts/rea.mjs"); + expect(mcp.servers).toMatchObject({ + peer: { command: ["/usr/bin/true"] }, + rea: { + type: "local", + command: + process.platform === "win32" + ? [process.execPath, launcher, "mcp"] + : [launcher, "mcp"], + }, + }); + }, +); diff --git a/tests/boundary/filesystem/clientLifecycle.test.ts b/tests/boundary/filesystem/clientLifecycle.test.ts index 19a4b5180..dd229f9c8 100644 --- a/tests/boundary/filesystem/clientLifecycle.test.ts +++ b/tests/boundary/filesystem/clientLifecycle.test.ts @@ -1690,6 +1690,105 @@ describe("client configuration filesystem removal", () => { }); }); +describe("uninstall registration ownership preflight", () => { + it("removes repeated owned registrations from the table the client reads", async () => { + const home = await createTestTempDirectory("rea-uninstall-repeated-owned-"); + roots.push(home); + const config = join(home, ".claude.json"); + await mkdir(dirname(config), { recursive: true }); + const owned = { + command: PRODUCT_IDENTITY.cliBinary, + args: ["mcp"], + }; + const original = + `{"mcpServers":{"rea":${JSON.stringify(owned)},` + + `"peer":{"command":"peer"},` + + `"rea":${JSON.stringify(owned)}}}\n`; + await writeFile(config, original); + + const result = await runUninstall(false, systemUninstallHost(home)); + + expect(result.status).toBe("complete"); + expect(result.items).toContainEqual( + expect.objectContaining({ name: "claude_code", status: "removed" }), + ); + expect(JSON.parse(await readFile(config, "utf8"))).toEqual({ + mcpServers: { peer: { command: "peer" } }, + }); + expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original); + }); + + it("refuses an unowned repeat before changing any client or making backups", async () => { + const home = await createTestTempDirectory( + "rea-uninstall-repeated-foreign-", + ); + roots.push(home); + const claude = join(home, ".claude.json"); + const cursor = join(home, ".cursor/mcp.json"); + await mkdir(dirname(claude), { recursive: true }); + await mkdir(dirname(cursor), { recursive: true }); + const owned = { + command: PRODUCT_IDENTITY.cliBinary, + args: ["mcp"], + }; + const claudeOriginal = JSON.stringify({ mcpServers: { rea: owned } }); + const cursorOriginal = `{"mcpServers":{"rea":null,"rea":${JSON.stringify(owned)}}}`; + await writeFile(claude, claudeOriginal); + await writeFile(cursor, cursorOriginal); + + const result = await runUninstall(false, systemUninstallHost(home)); + + expect(result.status).toBe("failed"); + expect(result.items).toContainEqual( + expect.objectContaining({ name: "cursor", status: "failed" }), + ); + expect(result.items).toContainEqual( + expect.objectContaining({ name: "uninstall", status: "skipped" }), + ); + expect(await readFile(claude, "utf8")).toBe(claudeOriginal); + expect(await readFile(cursor, "utf8")).toBe(cursorOriginal); + await expect( + readFile(`${claude}.rea.backup`, "utf8"), + ).rejects.toMatchObject({ code: "ENOENT" }); + await expect( + readFile(`${cursor}.rea.backup`, "utf8"), + ).rejects.toMatchObject({ code: "ENOENT" }); + }); + + it("keeps an owned OpenCode V2 registration when its V1 fallback is foreign", async () => { + const home = await createTestTempDirectory( + "rea-uninstall-opencode-fallback-", + ); + roots.push(home); + const config = join(home, ".config/opencode/opencode.json"); + await mkdir(dirname(config), { recursive: true }); + const original = JSON.stringify({ + mcp: { + servers: { + rea: { + type: "local", + command: [PRODUCT_IDENTITY.cliBinary, "mcp"], + enabled: true, + }, + }, + rea: { type: "local", command: ["/usr/bin/false"] }, + }, + }); + await writeFile(config, original); + + const result = await runUninstall(false, systemUninstallHost(home)); + + expect(result.status).toBe("failed"); + expect(result.items).toContainEqual( + expect.objectContaining({ name: "opencode", status: "failed" }), + ); + expect(await readFile(config, "utf8")).toBe(original); + await expect( + readFile(`${config}.rea.backup`, "utf8"), + ).rejects.toMatchObject({ code: "ENOENT" }); + }); +}); + describe("uninstall configuration preflight", () => { it("fails closed on malformed client configuration", async () => { const home = await createTestTempDirectory("rea-uninstall-bad-"); @@ -1802,6 +1901,45 @@ describe("uninstall configuration preflight", () => { }); describe("client configuration changed during uninstall", () => { + it("rechecks duplicate ownership after preflight and keeps the changed source", async () => { + const home = await createTestTempDirectory("rea-uninstall-repeat-race-"); + roots.push(home); + const cursor = join(home, ".cursor/mcp.json"); + await mkdir(dirname(cursor), { recursive: true }); + const owned = { + command: PRODUCT_IDENTITY.cliBinary, + args: ["mcp"], + }; + const original = JSON.stringify({ mcpServers: { rea: owned } }); + const changed = `{"mcpServers":{"rea":null,"rea":${JSON.stringify(owned)}}}`; + await writeFile(cursor, original); + let reads = 0; + const fileSystem: UninstallFileSystem = { + ...testFileSystem, + readText: async (path) => { + if (path !== cursor) return readFile(path, "utf8"); + reads += 1; + if (reads === 2) await writeFile(cursor, changed); + return readFile(path, "utf8"); + }, + }; + + const result = await runUninstall( + false, + systemUninstallHost(home, fileSystem), + ); + + expect(reads).toBe(2); + expect(result.status).toBe("failed"); + expect(result.items).toContainEqual( + expect.objectContaining({ name: "cursor", status: "failed" }), + ); + expect(await readFile(cursor, "utf8")).toBe(changed); + await expect( + readFile(`${cursor}.rea.backup`, "utf8"), + ).rejects.toMatchObject({ code: "ENOENT" }); + }); + it("stops the remaining removals when a configuration fails after preflight", async () => { const home = await createTestTempDirectory("rea-uninstall-race-"); roots.push(home);