diff --git a/src/application/ClientConfigurationDocument.capacity.test.ts b/src/application/ClientConfigurationDocument.capacity.test.ts index 190435a363..2fdb58ed5f 100644 --- a/src/application/ClientConfigurationDocument.capacity.test.ts +++ b/src/application/ClientConfigurationDocument.capacity.test.ts @@ -8,6 +8,14 @@ import { const registration = { command: "rea", args: ["mcp"] }; +const serializedText = ( + ...input: Parameters +): string => { + const serialized = serializeClientConfiguration(...input); + if (!serialized.ok) throw new Error("TOML serialization was refused"); + return serialized.value; +}; + describe("TOML configuration source capacity", () => { it.each(["\n", "\r\n"])( "retains adjacent assignments and multiline values with %j breaks", @@ -29,18 +37,14 @@ describe("TOML configuration source capacity", () => { ...parsed.servers, rea: registration, }); - const configured = serializeClientConfiguration( - desired, - "toml", - original, - ); + const configured = serializedText(desired, "toml", original); expect( configured.startsWith("\uFEFF" + prefix.replaceAll("\n", newline)), ).toBe(true); expect(parseClientConfiguration(configured, "toml").document).toEqual( desired, ); - const removed = serializeClientConfiguration( + const removed = serializedText( withClientServers(parsed, {}), "toml", configured, @@ -61,16 +65,12 @@ describe("TOML configuration source capacity", () => { ...parsed.servers, rea: registration, }); - const configured = serializeClientConfiguration(desired, "toml", original); + const configured = serializedText(desired, "toml", original); expect(configured).toContain(`NOTE="""${note}"""`); expect(parseClientConfiguration(configured, "toml").document).toEqual( desired, ); - const removed = serializeClientConfiguration( - parsed.document, - "toml", - configured, - ); + const removed = serializedText(parsed.document, "toml", configured); expect(removed).toContain(`NOTE="""${note}"""`); expect(parseClientConfiguration(removed, "toml").document).toEqual( parsed.document, @@ -87,7 +87,7 @@ describe("TOML configuration source capacity", () => { ...parsed.servers, rea: registration, }); - const configured = serializeClientConfiguration(desired, "grok", original); + const configured = serializedText(desired, "grok", original); const result = parseClientConfiguration(configured, "grok"); expect(result.document.disabled_mcp_servers).toEqual( Array(500_000).fill("other"), diff --git a/src/application/ClientConfigurationDocument.ts b/src/application/ClientConfigurationDocument.ts index a26c32c7aa..c22c4c5462 100644 --- a/src/application/ClientConfigurationDocument.ts +++ b/src/application/ClientConfigurationDocument.ts @@ -15,16 +15,18 @@ import { import { applyEdits, createScanner, - findNodeAtLocation, - modify, + format as formatJson, parse as parseJsonc, parseTree, printParseErrorCode, + type Edit, + type Node, type ParseError, type SyntaxKind, } from "jsonc-parser"; import { z } from "zod"; +import { err, ok, type Result } from "../domain/result.js"; import { PRODUCT_IDENTITY } from "../identity.js"; import type { SetupClient } from "./SupportedClients.js"; import { compareUnicodeCodePoints } from "../domain/unicodeCodePointOrder.js"; @@ -1312,6 +1314,37 @@ const nextJsoncToken = ( return { kind, offset: scanner.getTokenOffset() }; }; +const isJsonMember = (property: Node, key: string): boolean => + property.children?.length === 2 && property.children[0]?.value === key; + +/** + * The property a client reads for `key`. `JSON.parse` and JSONC `parse` keep + * the last duplicate member, while jsonc-parser's `findNodeAtLocation` and + * `modify` select the first, so JSON text edits locate members only here. + */ +const effectiveJsonProperty = (object: Node, key: string): Node | undefined => + object.children?.findLast((property) => isJsonMember(property, key)); + +/** Apply one edit and format the lines it touched, as `modify` does. */ +const applyFormattedJsonEdit = (text: string, edit: Edit): string => { + const edited = applyEdits(text, [edit]); + let begin = edit.offset; + let end = edit.offset + edit.content.length; + if (edit.length === 0) { + while (begin > 0 && !"\r\n".includes(edited.charAt(begin - 1))) begin -= 1; + while (end < edited.length && !"\r\n".includes(edited.charAt(end))) + end += 1; + } + return applyEdits( + edited, + formatJson( + edited, + { offset: begin, length: end - begin }, + { insertSpaces: true, tabSize: 2, eol: "\n", keepLines: false }, + ), + ); +}; + /** * 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 @@ -1319,80 +1352,136 @@ const nextJsoncToken = ( */ const deleteJsonProperty = ( text: string, - path: readonly string[], -): string | undefined => { - const root = parseTree(text, [], { allowTrailingComma: true }); - const value = - root === undefined ? undefined : findNodeAtLocation(root, [...path]); - const property = value?.parent; - const parent = property?.parent; + object: Node, + property: 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 ( - property === undefined || - parent?.type !== "object" || - property.type !== "property" || - parent.children === undefined + propertyIndex < 0 || + (comma.kind !== JSON_COMMA && + (previous !== undefined || next !== undefined)) ) - return undefined; - const propertyIndex = parent.children.indexOf(property); - if (propertyIndex < 0) return undefined; - const previous = parent.children[propertyIndex - 1]; - const next = parent.children[propertyIndex + 1]; - let commaOffset: number | undefined; - if (previous !== undefined) { - const comma = nextJsoncToken(text, previous.offset + previous.length); - if (comma.kind !== JSON_COMMA) return undefined; - commaOffset = comma.offset; - } else { - const token = nextJsoncToken(text, property.offset + property.length); - if (token.kind === JSON_COMMA) commaOffset = token.offset; - else if (next !== undefined) return undefined; - } + throw new TypeError("JSON property separator was not found"); const edits = [ { offset: property.offset, length: property.length, content: "" }, - ...(commaOffset === undefined - ? [] - : [{ offset: commaOffset, length: 1, content: "" }]), + ...(comma.kind === JSON_COMMA + ? [{ offset: comma.offset, length: 1, content: "" }] + : []), ]; return applyEdits(text, edits); }; -/** Serialize a validated client document, retaining JSONC and TOML comments. */ +/** A JSON edit refused because it would activate a shadowed duplicate member. */ +export interface ClientConfigurationShadowedMember { + readonly kind: "shadowed_member"; + /** Key path of the effective member whose removal was refused. */ + readonly path: readonly string[]; +} + +/** + * 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. + */ +const editJsonMember = ( + text: string, + path: readonly string[], + value: unknown, +): Result => { + let object = parseTree(text, [], { allowTrailingComma: true }); + for (const [index, key] of path.entries()) { + if (object?.type !== "object") + throw new TypeError("JSON edit parent is not an object"); + const property = effectiveJsonProperty(object, key); + const member = property?.children?.[1]; + if (property === undefined || member === undefined) { + if (value === undefined) return ok(text); + const content = `${JSON.stringify(key)}: ${JSON.stringify( + path + .slice(index + 1) + .reduceRight((nested, name) => ({ [name]: nested }), value), + )}`; + const previous = object.children?.at(-1); + return ok( + applyFormattedJsonEdit( + text, + previous === undefined + ? { offset: object.offset + 1, length: 0, content } + : { + offset: previous.offset + previous.length, + length: 0, + content: `,${content}`, + }, + ), + ); + } + if (index < path.length - 1) { + object = member; + continue; + } + if (value !== undefined) + return ok( + applyFormattedJsonEdit(text, { + offset: member.offset, + length: member.length, + content: JSON.stringify(value), + }), + ); + if ( + object.children?.some( + (sibling) => sibling !== property && isJsonMember(sibling, key), + ) + ) + return err({ kind: "shadowed_member", path }); + return ok(deleteJsonProperty(text, object, property)); + } + throw new TypeError("JSON edit path is empty"); +}; + +/** + * Serialize a validated client document, retaining JSONC and TOML comments. + * JSON edits fail only when they would activate a shadowed duplicate member. + */ export const serializeClientConfiguration = ( document: Record, format: ClientConfigurationFormat | undefined, originalText?: string, editedPaths?: readonly (readonly string[])[], -): string => { +): Result => { if (format === undefined || format === "unsupported") throw new TypeError("client does not have a supported MCP config format"); if (format === "toml") - return serializeCodexConfiguration(document, originalText); + return ok(serializeCodexConfiguration(document, originalText)); if (format === "grok") - return serializeGrokConfiguration(document, originalText); + return ok(serializeGrokConfiguration(document, originalText)); if (format === "hermes") - return serializeHermesConfiguration(document, originalText); - if (format === "dsh") return serializeDshDocument(document); + return ok(serializeHermesConfiguration(document, originalText)); + if (format === "dsh") return ok(serializeDshDocument(document)); // An empty original holds no comments or settings to preserve. Writing a // fresh document also keeps a lone BOM from shifting past the closing brace. if ( - originalText !== undefined && - !isEmptyClientConfigurationText(originalText) && - editedPaths !== undefined + originalText === undefined || + isEmptyClientConfigurationText(originalText) || + editedPaths === undefined ) - return editedPaths.reduce((text, path) => { - const value = valueAt(document, path); - if (value === undefined) { - const preserved = deleteJsonProperty(text, path); - if (preserved !== undefined) return preserved; - } - return applyEdits( - text, - modify(text, [...path], value, { - formattingOptions: { insertSpaces: true, tabSize: 2, eol: "\n" }, - }), - ); - }, originalText); - return `${JSON.stringify(document, null, 2)}\n`; + return ok(`${JSON.stringify(document, null, 2)}\n`); + let text = originalText; + for (const path of editedPaths) { + const edited = editJsonMember(text, path, valueAt(document, path)); + if (!edited.ok) return edited; + text = edited.value; + } + return ok(text); }; /** Key path of a named registration in the server table. */ diff --git a/src/application/SetupClientConfiguration.ts b/src/application/SetupClientConfiguration.ts index ed7b1bb495..5d923b5df7 100644 --- a/src/application/SetupClientConfiguration.ts +++ b/src/application/SetupClientConfiguration.ts @@ -250,16 +250,6 @@ const persistClientConfiguration = async ({ }; if (target.value) return { status: "failed", reason: "target_changed" }; } - let recoveryBackupPath: string | undefined; - if (original !== undefined) { - const preserved = await preserveCurrentRecoveryBackup( - `${client.configPath}.rea.backup`, - transactionPath, - original, - ); - if (!preserved.ok) return { status: "failed", reason: "backup" }; - recoveryBackupPath = preserved.value; - } // A native OpenCode V2 entry replaces REA's legacy V1 entry, which would // otherwise conflict with it. const legacyPath = legacyClientServerPath( @@ -292,19 +282,43 @@ 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]] : []), + ]); + } catch (cause: unknown) { + // An unserializable edit is reported before any file is written. + void cause; + return { status: "failed", reason: "write" }; + } + if (!serialized.ok) + return { + status: "failed", + reason: "shadowed_member", + path: serialized.error.path, + }; + let recoveryBackupPath: string | undefined; + if (original !== undefined) { + const preserved = await preserveCurrentRecoveryBackup( + `${client.configPath}.rea.backup`, + transactionPath, + original, + ); + if (!preserved.ok) return { status: "failed", reason: "backup" }; + recoveryBackupPath = preserved.value; + } let publication: ClientConfigurationPublication; try { await mkdir(dirname(client.configPath), { recursive: true }); publication = await ClientConfigurationPublication.write( transactionPath, - 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.value, original, 0o600, ); diff --git a/src/application/SetupClients.ts b/src/application/SetupClients.ts index db03654145..aa5ff67a48 100644 --- a/src/application/SetupClients.ts +++ b/src/application/SetupClients.ts @@ -20,6 +20,8 @@ export const failedConfigurationMessage = ( return "Agent configuration could not be backed up, so no change was made. Check file permissions, then rerun setup."; case "write": return "Agent configuration could not be updated. Check file permissions, then rerun setup."; + case "shadowed_member": + return `Agent configuration repeats the member at ${JSON.stringify(result.path)}; removing the member the client reads would activate an earlier duplicate, so no change was made. Remove the duplicate members, then rerun setup.`; case "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.` diff --git a/src/application/SetupTypes.ts b/src/application/SetupTypes.ts index a4e80893cd..8acfbe139b 100644 --- a/src/application/SetupTypes.ts +++ b/src/application/SetupTypes.ts @@ -37,6 +37,12 @@ export type ClientConfigurationResult = readonly status: "failed"; readonly reason: "path" | "target_changed" | "backup" | "write"; } + | { + readonly status: "failed"; + /** Removing the member a client reads would activate an earlier duplicate. */ + readonly reason: "shadowed_member"; + readonly path: readonly string[]; + } | { readonly status: "failed"; readonly reason: "readback"; diff --git a/src/application/Uninstall.ts b/src/application/Uninstall.ts index 5dc44e1533..78f852d4b5 100644 --- a/src/application/Uninstall.ts +++ b/src/application/Uninstall.ts @@ -551,6 +551,27 @@ const removeClient = async ( ...(removeNative ? [clientServerPath(parsed, name)] : []), ...(legacyPath === undefined ? [] : [legacyPath]), ]; + let serialized: ReturnType; + try { + serialized = serializeClientConfiguration( + document, + client.format, + original, + editedPaths, + ); + } catch { + return item( + client.name, + "failed", + "Configuration could not be updated, so no change was made. Repair the configuration, then rerun uninstall.", + ); + } + if (!serialized.ok) + return item( + client.name, + "failed", + `Configuration repeats the member at ${JSON.stringify(serialized.error.path)}; 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 backupPath = await backupClientConfiguration( client, transactionPath, @@ -573,12 +594,7 @@ const removeClient = async ( try { publication = await fileSystem.publishConfiguration( transactionPath, - serializeClientConfiguration( - document, - client.format, - original, - editedPaths, - ), + serialized.value, original, ); } catch (cause: unknown) { diff --git a/tests/acceptance/setup/duplicateJsonMemberSetup.test.ts b/tests/acceptance/setup/duplicateJsonMemberSetup.test.ts new file mode 100644 index 0000000000..20ed05da95 --- /dev/null +++ b/tests/acceptance/setup/duplicateJsonMemberSetup.test.ts @@ -0,0 +1,144 @@ +import { mkdir, readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { expect } from "vitest"; + +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { cliTest, type TestCli } from "../../support/cli/cliFixture.js"; + +// JSON.parse, JSONC parse, and native Claude Code read the last duplicate. +const shadowParent = '"mcpServers": {"shadow": {"command": "/usr/bin/false"}},'; +const shadowLeaf = '"rea": {"command": "/usr/bin/false"},'; +const owned = + '"rea": {"command": "npx", "args": ["-y", "rea-agents@6.6.0", "mcp"]}'; + +const runIn = (cli: TestCli, home: string) => (arguments_: readonly string[]) => + cli.run({ + arguments: arguments_, + cwd: home, + environment: { + HOME: home, + USERPROFILE: home, + CLAUDE_CONFIG_DIR: home, + XDG_CONFIG_HOME: join(home, ".config"), + }, + }); + +const setupArguments = (client: string) => + ["setup", "--client", client, "--skill=false", "--json", "--yes"] as const; + +for (const [layout, original] of [ + [ + "a repeated server table", + `{\n ${shadowParent}\n "mcpServers": {"peer": {"command": "/usr/bin/true"}}\n}\n`, + ], + [ + "a repeated rea member", + `{\n "mcpServers": {${shadowLeaf} "rea": {"command": "/usr/bin/true"}}\n}\n`, + ], +] as const) + cliTest( + `Claude Code setup edits the effective member of ${layout}`, + async ({ cli }) => { + const home = await createTestTempDirectory("rea-json-duplicate-"); + const config = join(home, ".claude.json"); + await writeFile(config, original); + const run = runIn(cli, home); + + const setup = await run(setupArguments("claude_code")); + expect(setup.exitCode, setup.stdout).toBe(0); + expect(setup.json).toMatchObject({ + status: "ready", + clients: { claude_code: { status: "configured" } }, + }); + expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original); + const configured = await readFile(config, "utf8"); + expect(configured).toContain( + layout === "a repeated server table" ? shadowParent : shadowLeaf, + ); + expect(JSON.parse(configured)).toMatchObject({ + mcpServers: { rea: { command: expect.not.stringMatching(/^\/usr/) } }, + }); + + const repeated = await run(setupArguments("claude_code")); + expect(repeated.exitCode, repeated.stdout).toBe(0); + expect(repeated.json).toMatchObject({ + status: "ready", + plannedActions: [], + }); + expect(await readFile(config, "utf8")).toBe(configured); + }, + ); + +cliTest( + "JSONC setup keeps comments and the shadowed server table", + async ({ cli }) => { + const home = await createTestTempDirectory("rea-jsonc-duplicate-"); + await mkdir(join(home, ".cursor")); + const config = join(home, ".cursor", "mcp.json"); + const head = `{\n // shadowed table\n ${shadowParent}\n /* effective table */\n`; + await writeFile( + config, + `${head} "mcpServers": {\n "peer": {"command": "/usr/bin/true"} // peer\n }\n}\n`, + ); + + const setup = await runIn(cli, home)(setupArguments("cursor")); + expect(setup.exitCode, setup.stdout).toBe(0); + expect(setup.json).toMatchObject({ + clients: { cursor: { status: "configured" } }, + }); + const configured = await readFile(config, "utf8"); + expect(configured.startsWith(head)).toBe(true); + expect(configured).toContain("// peer"); + expect(configured.indexOf('"rea"')).toBeGreaterThan(head.length); + }, +); + +cliTest( + "uninstall removes an owned registration from the effective server table", + async ({ cli }) => { + const home = await createTestTempDirectory("rea-json-duplicate-"); + const config = join(home, ".claude.json"); + const original = `{\n ${shadowParent}\n "mcpServers": {"peer": {"command": "/usr/bin/true"}, ${owned}}\n}\n`; + await writeFile(config, original); + + const removed = await runIn(cli, home)(["uninstall", "--json"]); + expect(removed.exitCode, removed.stdout).toBe(0); + expect(removed.json).toMatchObject({ + items: expect.arrayContaining([ + expect.objectContaining({ name: "claude_code", status: "removed" }), + ]), + }); + const remaining = await readFile(config, "utf8"); + expect(remaining.startsWith(`{\n ${shadowParent}\n`)).toBe(true); + expect(JSON.parse(remaining)).toEqual({ + mcpServers: { peer: { command: "/usr/bin/true" } }, + }); + }, +); + +cliTest( + "uninstall refuses to activate a shadowed duplicate registration", + async ({ cli }) => { + const home = await createTestTempDirectory("rea-json-duplicate-"); + const config = join(home, ".claude.json"); + const original = `{\n "mcpServers": {${shadowLeaf} ${owned}}\n}\n`; + await writeFile(config, original); + + 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: "claude_code", + status: "failed", + detail: expect.stringContaining("activate an earlier duplicate"), + }), + ]), + }); + expect(await readFile(config, "utf8")).toBe(original); + await expect(readFile(`${config}.rea.backup`)).rejects.toMatchObject({ + code: "ENOENT", + }); + }, +); diff --git a/tests/boundary/filesystem/clientConfigurationEnvironment.test.ts b/tests/boundary/filesystem/clientConfigurationEnvironment.test.ts index 5c5340b02d..10abf90325 100644 --- a/tests/boundary/filesystem/clientConfigurationEnvironment.test.ts +++ b/tests/boundary/filesystem/clientConfigurationEnvironment.test.ts @@ -39,7 +39,7 @@ it.each([ REA_GHIDRA_COMPILER_SPEC_ID: "borlandcpp", CALLER_SETTING: "preserve exactly", }; - const original = serializeClientConfiguration( + const serialized = serializeClientConfiguration( { [serversKey]: { rea: clientRegistrationEntry(format, ["rea-old", "mcp"], custom), @@ -48,6 +48,8 @@ it.each([ }, format, ); + if (!serialized.ok) throw new Error("fixture serialization failed"); + const original = serialized.value; await writeFile(configPath, original); const providers = { GHIDRA_INSTALL_DIR: join(root, "new-ghidra") }; const command = ["rea", "mcp"];