From 22ddd820143b0ce455756b24acf60ed251286622 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 19 Oct 2026 06:32:14 +0800 Subject: [PATCH] fix(uninstall): preflight repeated registration ownership Read shadowed registrations from the effective JSONC syntax tree before any client is changed. Share the same typed ownership decision with the reread before removal, including foreign OpenCode V1 fallbacks. Remove all admitted duplicates using one member/separator planner and grant serializer authority only for exact admitted paths. Setup may remove its explicitly superseded OpenCode V1 path. Preserve inactive repeated parents, unrelated entries, comments and source backups. Use the pinned JSONC scanner instead of handwritten string decoding. Validation: final 86 focused cases and 9 setup acceptance cases passed; 104 additional filesystem consumer cases passed. Static/architecture, docs and discovery checks passed. A broader 186-case run had one OMP timeout; its isolated 9-case rerun passed, with timeout cause unverified. Controlled compiled CLI comparisons verify removal/refusal and backups; real developer-client execution remains unverified. Adapted from #4977 at 4cd22d4ec2f6ec9e6c8ebf98bf97e1d9d7f8ac23. Closes #4976 Co-authored-by: Nikhil Bhagat Co-authored-by: Claude Opus 5.5 --- docs/installation.md | 7 +- ...figurationDocument.repeatedMembers.test.ts | 165 ++++++++++++++++ .../ClientConfigurationDocument.shape.test.ts | 26 ++- .../ClientConfigurationDocument.ts | 185 +++++++++++++++--- src/application/SetupClientConfiguration.ts | 24 ++- src/application/Uninstall.ts | 91 ++++++--- .../setup/duplicateJsonMemberSetup.test.ts | 143 +++++++++++++- .../filesystem/clientLifecycle.test.ts | 138 +++++++++++++ 8 files changed, 713 insertions(+), 66 deletions(-) create mode 100644 src/application/ClientConfigurationDocument.repeatedMembers.test.ts 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);