fix(setup): edit the duplicate JSON member that clients read (#4957)

JSON.parse, JSONC parse, and native Claude Code keep the last duplicate
object member, and parseClientConfiguration therefore validates the last
one. The shared setup/uninstall serializer instead edited text through
jsonc-parser's modify and findNodeAtLocation, which select the first
matching member, so setup and uninstall on configurations with repeated
server tables or rea members wrote into a shadowed member and failed
readback.

Locate every JSON text edit by the effective (last) member in one place,
replacing the first-member modify path. Removing an effective member
that shadows an earlier duplicate would activate that duplicate, whatever
its ownership, so the serializer now returns a tagged shadowed_member
failure and setup and uninstall refuse before writing a backup or the
configuration.

Fixes #4952

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

src/application/ClientConfigurationDocument.capacity.test.ts

@@ -8,6 +8,14 @@ import {
 
 const registration = { command: "rea", args: ["mcp"] };
 
+const serializedText = (
+  ...input: Parameters<typeof serializeClientConfiguration>
+): 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<string>(500_000).fill("other"),

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<string, ClientConfigurationShadowedMember> => {
+  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<unknown>((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<string, unknown>,
   format: ClientConfigurationFormat | undefined,
   originalText?: string,
   editedPaths?: readonly (readonly string[])[],
-): string => {
+): Result<string, ClientConfigurationShadowedMember> => {
   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. */

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<typeof serializeClientConfiguration>;
+  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,
     );

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.`

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";

src/application/Uninstall.ts

@@ -551,6 +551,27 @@ const removeClient = async (
     ...(removeNative ? [clientServerPath(parsed, name)] : []),
     ...(legacyPath === undefined ? [] : [legacyPath]),
   ];
+  let serialized: ReturnType<typeof serializeClientConfiguration>;
+  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) {

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",
+    });
+  },
+);

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"];