fix(hermes): detach aliased server tables before editing (#3785)

* fix(hermes): detach aliased server tables before editing

Copy aliased parent mappings before setting or removing a Hermes
registration. Resolve references against the original document and remove
anchors from the private copies to preserve existing sources and consumers,
including shadowed anchor names.

Cover YAML 1.1 and 1.2 setup, refresh, doctor readiness, repeated setup and
uninstall, along with comments, exact backups and invalid-alias refusal.

Fixes #3320

* test(setup): isolate Hermes flows from host Hopper detection

---------

Co-authored-by: morluto <76467478+morluto@users.noreply.github.com>

src/application/HermesConfigurationYaml.test.ts

@@ -7,6 +7,57 @@ import {
   stringifyHermesConfigurationYaml,
 } from "./HermesConfigurationYaml.js";
 
+describe("Hermes YAML deletion", () => {
+  for (const version of ["1.1", "1.2"] as const) {
+    for (const [name, body, path] of [
+      [
+        "an aliased table",
+        "server_defaults: &servers\n  other:\n    command: other-server\n\nmcp_servers: *servers # Selected table\n",
+        ["mcp_servers", "rea"],
+      ],
+      [
+        "nested aliases",
+        "defaults: &defaults\n  other: {command: other-server}\nservers: &servers\n  mcp_servers: *defaults\nclient: *servers\nobserver: *servers\n",
+        ["client", "mcp_servers", "rea"],
+      ],
+      [
+        "shadowed anchors",
+        "first: &server {command: original}\ndefaults: &servers\n  other: *server\nsecond: &server {command: replacement}\nmcp_servers: *servers\nobserver: *server\n",
+        ["mcp_servers", "rea"],
+      ],
+      [
+        "shared consumers",
+        "defaults: &servers\n  other: &other {command: other-server}\nmcp_servers: *servers\nobserver: *servers\nother_observer: *other\n",
+        ["mcp_servers", "rea"],
+      ],
+      [
+        "an explicit flow table with CRLF",
+        "# Keep formatting\r\nmcp_servers: {other: {command: other-server}}\r\n",
+        ["mcp_servers", "rea"],
+      ],
+    ] as const) {
+      it(`preserves YAML ${version} bytes when deleting an absent key from ${name}`, () => {
+        const source = (version === "1.2" ? "%YAML 1.2\n---\n" : "") + body;
+        expect(editHermesConfigurationYaml(source, path, undefined)).toBe(
+          source,
+        );
+      });
+    }
+  }
+
+  for (const [name, reference] of [
+    ["cyclic", "servers"],
+    ["unresolved", "missing"],
+  ]) {
+    it(`still refuses deletion through a table containing a ${name} alias`, () => {
+      const source = `defaults: &servers\n  rea: {command: rea}\n  other: *${reference}\nmcp_servers: *servers\n`;
+      expect(() =>
+        editHermesConfigurationYaml(source, ["mcp_servers", "rea"], undefined),
+      ).toThrow(TypeError);
+    });
+  }
+});
+
 describe("Hermes configuration YAML", () => {
   it("retains a quoted sequence element when replacing its registration", () => {
     const source =

src/application/HermesConfigurationYaml.ts

@@ -1,4 +1,5 @@
 import {
+  isAlias,
   isMap,
   isNode,
   isScalar,
@@ -356,6 +357,43 @@ const retainHermesScalarSource = (previous: unknown, next: Node): Node => {
   return next;
 };
 
+/** Copy original bindings so moved aliases cannot bind to later anchors. */
+const copyHermesAliasSource = (
+  source: unknown,
+  document: Document,
+  visiting = new Set<Node>(),
+): unknown => {
+  const node = isAlias(source) ? source.resolve(document) : source;
+  if (isAlias(source) && node === undefined)
+    throw new TypeError("Cannot edit an unresolved Hermes YAML alias");
+  if (!isNode(node)) return node;
+  if (visiting.has(node))
+    throw new TypeError("Cannot edit cyclic Hermes YAML aliases");
+  visiting.add(node);
+  const copied = node.clone();
+  if (isScalar(copied) || isMap(copied) || isSeq(copied)) delete copied.anchor;
+  if (isSeq(node) && isSeq(copied))
+    copied.items = node.items.map((item) =>
+      copyHermesAliasSource(item, document, visiting),
+    );
+  else if (isMap(node) && isMap(copied))
+    copied.items.forEach((pair, index) => {
+      const original = node.items[index];
+      if (original === undefined) return;
+      pair.key = copyHermesAliasSource(original.key, document, visiting);
+      pair.value = copyHermesAliasSource(original.value, document, visiting);
+    });
+  if (isAlias(source)) {
+    delete copied.commentBefore;
+    delete copied.comment;
+    if (source.commentBefore !== undefined)
+      copied.commentBefore = source.commentBefore;
+    if (source.comment !== undefined) copied.comment = source.comment;
+  }
+  visiting.delete(node);
+  return copied;
+};
+
 /** Set or delete one value in a Hermes `config.yaml`, keeping the rest as written. */
 export const editHermesConfigurationYaml = (
   text: string,
@@ -373,15 +411,23 @@ export const editHermesConfigurationYaml = (
         .join("\n");
     document.contents = map;
   }
-  if (value === undefined) document.deleteIn(path);
-  else {
-    // setIn's implicit JS Map parents serialize as !!omap in YAML 1.1.
-    // Config settings are ordinary mappings, including absent server tables.
-    for (let length = 1; length < path.length; length++) {
-      const parent = path.slice(0, length);
-      if (!document.hasIn(parent))
-        document.setIn(parent, document.createNode({}));
+  for (let length = 1; length < path.length; length++) {
+    const parent = path.slice(0, length);
+    const previous = document.getIn(parent, true);
+    if (isAlias(previous)) {
+      const copied = copyHermesAliasSource(previous, document);
+      if (isMap(copied)) {
+        // Edit a private mapping, keeping the alias source and its consumers.
+        document.setIn(parent, copied);
+      }
+    } else if (value !== undefined && !document.hasIn(parent)) {
+      // Implicit JS Map parents serialize as !!omap in YAML 1.1.
+      document.setIn(parent, document.createNode({}));
     }
+  }
+  if (value === undefined) {
+    if (!document.deleteIn(path)) return text;
+  } else {
     const previous = document.getIn(path, true);
     // Let YAML's scalar update preserve numeric formats for a direct edit.
     // Replacing containers needs explicit retention of unchanged child nodes.

tests/acceptance/setup/hermesAliasedTableSetup.test.ts

@@ -0,0 +1,315 @@
+import { access, mkdir, readFile, writeFile } from "node:fs/promises";
+import { join } from "node:path";
+import { parse, stringify } from "yaml";
+import { expect } from "vitest";
+
+import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js";
+import { cliTest } from "../../support/cli/cliFixture.js";
+
+const configuration = (
+  version: "1.1" | "1.2",
+  existing: boolean,
+  aliased = true,
+) => {
+  const defaults =
+    (version === "1.2" ? "%YAML 1.2\n---\n" : "") +
+    "# Personal settings\nmodel: custom\nmask: 0x1F\n" +
+    "server_defaults: &servers\n" +
+    (existing
+      ? "  rea: &server\n    command: rea-old\n    args: [ 'mcp' ]\n    env:\n      FLAG: 'on'\n  other: *server\n"
+      : "  other: &server\n    command: other-server\n    args: [ 'on', '0600' ]\n");
+  return {
+    defaults,
+    text:
+      defaults +
+      (aliased
+        ? "mcp_servers: *servers # Selected table\n"
+        : "mcp_servers:\n  other: *server\n") +
+      "observer: *server\n",
+  };
+};
+
+for (const version of ["1.1", "1.2"] as const) {
+  cliTest(
+    `Hermes YAML ${version} uninstall preserves an aliased table without REA`,
+    async ({ cli }) => {
+      const home = await createTestTempDirectory(
+        "rea-hermes-absent-uninstall-",
+      );
+      const profile = join(home, "profile");
+      const config = join(profile, "config.yaml");
+      const original = configuration(version, false).text;
+      await mkdir(profile);
+      await writeFile(config, original);
+      for (let attempt = 0; attempt < 2; attempt++) {
+        const removed = await cli.run({
+          arguments: ["uninstall", "--json"],
+          cwd: home,
+          environment: {
+            USERPROFILE: home,
+            HERMES_HOME: profile,
+            HOPPER_LAUNCHER_PATH: process.execPath,
+          },
+          timeoutMs: 20_000,
+        });
+        expect(removed.exitCode).toBe(0);
+        expect(await readFile(config, "utf8")).toBe(original);
+        await expect(access(`${config}.rea.backup`)).rejects.toMatchObject({
+          code: "ENOENT",
+        });
+      }
+    },
+  );
+
+  for (const existing of [false, true]) {
+    cliTest(
+      `Hermes YAML ${version} ${existing ? "refreshes" : "adds"} REA in an aliased table without changing its source`,
+      async ({ cli }) => {
+        const home = await createTestTempDirectory("rea-hermes-table-alias-");
+        const profile = join(home, "profile");
+        const config = join(profile, "config.yaml");
+        const original = configuration(version, existing);
+        const before = parse(original.text, { version });
+        await mkdir(profile);
+        await writeFile(config, original.text);
+        const run = (arguments_: readonly string[]) =>
+          cli.run({
+            arguments: arguments_,
+            cwd: home,
+            environment: {
+              USERPROFILE: home,
+              HERMES_HOME: profile,
+              HOPPER_LAUNCHER_PATH: process.execPath,
+            },
+            timeoutMs: 20_000,
+          });
+        const setup = [
+          "setup",
+          "--client",
+          "hermes",
+          "--skill=false",
+          "--yes",
+          "--json",
+        ];
+        const applied = await run(setup);
+        expect(applied.exitCode).toBe(0);
+        expect(applied.json).toMatchObject({ status: "ready" });
+        const configured = await readFile(config, "utf8");
+        const parsed = parse(configured, { version });
+        expect(parsed.server_defaults).toEqual(before.server_defaults);
+        expect(parsed.observer).toEqual(before.observer);
+        expect(parsed.mcp_servers.other).toEqual(before.mcp_servers.other);
+        expect(parsed.mcp_servers.rea).toMatchObject({ enabled: true });
+        if (existing)
+          expect(parsed.mcp_servers.rea.env).toEqual({
+            FLAG: "on",
+            HOPPER_LAUNCHER_PATH: process.execPath,
+          });
+        expect(configured.startsWith(original.defaults)).toBe(true);
+        expect(configured).toContain("# Selected table");
+        expect(configured.match(/&servers\b/gu)).toHaveLength(1);
+        expect(configured.match(/&server\b/gu)).toHaveLength(1);
+        expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(
+          original.text,
+        );
+        const doctor = await run(["doctor", "--client", "hermes", "--json"]);
+        expect(doctor.exitCode).toBe(0);
+        expect(doctor.json).toMatchObject({
+          healthy: true,
+          identity: { registrations: [{ client: "hermes", state: "aligned" }] },
+        });
+        const repeat = await run(setup);
+        expect(repeat.exitCode).toBe(0);
+        expect(repeat.json).toMatchObject({
+          status: "ready",
+          plannedActions: [],
+          appliedActions: [],
+        });
+        expect(await readFile(config, "utf8")).toBe(configured);
+        const removed = await run(["uninstall", "--json"]);
+        expect(removed.exitCode).toBe(0);
+        const remaining = parse(await readFile(config, "utf8"), { version });
+        expect(remaining.mcp_servers).toEqual({
+          other: before.mcp_servers.other,
+        });
+        expect(remaining.server_defaults).toEqual(before.server_defaults);
+        expect(remaining.observer).toEqual(before.observer);
+      },
+    );
+  }
+}
+
+cliTest("Hermes still registers into an explicit table", async ({ cli }) => {
+  const home = await createTestTempDirectory("rea-hermes-explicit-table-");
+  const profile = join(home, "profile");
+  const config = join(profile, "config.yaml");
+  const original = configuration("1.1", false, false);
+  await mkdir(profile);
+  await writeFile(config, original.text);
+  const applied = await cli.run({
+    arguments: [
+      "setup",
+      "--client",
+      "hermes",
+      "--skill=false",
+      "--yes",
+      "--json",
+    ],
+    cwd: home,
+    environment: {
+      USERPROFILE: home,
+      HERMES_HOME: profile,
+      HOPPER_LAUNCHER_PATH: process.execPath,
+    },
+    timeoutMs: 20_000,
+  });
+  expect(applied.exitCode).toBe(0);
+  expect(applied.json).toMatchObject({ status: "ready" });
+  const parsed = parse(await readFile(config, "utf8"));
+  expect(parsed.mcp_servers.other).toEqual(
+    parse(original.text).mcp_servers.other,
+  );
+  expect(parsed.mcp_servers.rea).toMatchObject({ enabled: true });
+  expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original.text);
+});
+
+for (const version of ["1.1", "1.2"] as const) {
+  cliTest(
+    `Hermes YAML ${version} preserves shadowed alias bindings`,
+    async ({ cli }) => {
+      const home = await createTestTempDirectory("rea-hermes-shadowed-anchor-");
+      const profile = join(home, "profile");
+      const config = join(profile, "config.yaml");
+      const original =
+        (version === "1.2" ? "%YAML 1.2\n---\n" : "") +
+        "first: &server\n  command: original-server\n" +
+        "server_defaults: &servers\n  other: *server\n" +
+        "replacement: &server\n  command: replacement-server\n" +
+        "mcp_servers: *servers\nobserver: *server\n";
+      const before = parse(original, { version });
+      await mkdir(profile);
+      await writeFile(config, original);
+      const applied = await cli.run({
+        arguments: [
+          "setup",
+          "--client",
+          "hermes",
+          "--skill=false",
+          "--yes",
+          "--json",
+        ],
+        cwd: home,
+        environment: {
+          USERPROFILE: home,
+          HERMES_HOME: profile,
+          HOPPER_LAUNCHER_PATH: process.execPath,
+        },
+        timeoutMs: 20_000,
+      });
+      expect(applied.exitCode).toBe(0);
+      const configured = parse(await readFile(config, "utf8"), { version });
+      expect(configured.mcp_servers.other).toEqual(before.mcp_servers.other);
+      expect(configured.server_defaults).toEqual(before.server_defaults);
+      expect(configured.observer).toEqual(before.observer);
+      expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original);
+    },
+  );
+}
+
+cliTest(
+  "Hermes uninstall detaches an aliased table before deleting REA",
+  async ({ cli }) => {
+    const home = await createTestTempDirectory("rea-hermes-aliased-uninstall-");
+    const profile = join(home, "profile");
+    const config = join(profile, "config.yaml");
+    await mkdir(profile);
+    const run = (arguments_: readonly string[]) =>
+      cli.run({
+        arguments: arguments_,
+        cwd: home,
+        environment: {
+          USERPROFILE: home,
+          HERMES_HOME: profile,
+          HOPPER_LAUNCHER_PATH: process.execPath,
+        },
+        timeoutMs: 20_000,
+      });
+    const setup = await run([
+      "setup",
+      "--client",
+      "hermes",
+      "--skill=false",
+      "--yes",
+      "--json",
+    ]);
+    expect(setup.exitCode).toBe(0);
+    const entry = parse(await readFile(config, "utf8")).mcp_servers.rea;
+    const defaults = { rea: entry, other: { command: "other-server" } };
+    const original =
+      "server_defaults: &servers\n" +
+      stringify(defaults)
+        .split("\n")
+        .filter((line) => line.length > 0)
+        .map((line) => `  ${line}\n`)
+        .join("") +
+      "mcp_servers: *servers\nobserver: *servers\n";
+    await writeFile(config, original);
+    expect(
+      (await run(["doctor", "--client", "hermes", "--json"])).exitCode,
+    ).toBe(0);
+    const removed = await run(["uninstall", "--json"]);
+    expect(removed.exitCode).toBe(0);
+    const remaining = parse(await readFile(config, "utf8"));
+    expect(remaining.mcp_servers).toEqual({ other: defaults.other });
+    expect(remaining.server_defaults).toEqual(defaults);
+    expect(remaining.observer).toEqual(defaults);
+    const configured = await readFile(config, "utf8");
+    expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original);
+    expect((await run(["uninstall", "--json"])).exitCode).toBe(0);
+    expect(await readFile(config, "utf8")).toBe(configured);
+    expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original);
+  },
+);
+
+for (const original of [
+  "mcp_servers: *missing\n",
+  "server_defaults: &servers invalid\nmcp_servers: *servers\n",
+]) {
+  cliTest(
+    `Hermes refuses an invalid aliased table: ${original.trim()}`,
+    async ({ cli }) => {
+      const home = await createTestTempDirectory("rea-hermes-invalid-alias-");
+      const profile = join(home, "profile");
+      const config = join(profile, "config.yaml");
+      await mkdir(profile);
+      await writeFile(config, original);
+      const rejected = await cli.run({
+        arguments: [
+          "setup",
+          "--client",
+          "hermes",
+          "--skill=false",
+          "--yes",
+          "--json",
+        ],
+        cwd: home,
+        environment: {
+          USERPROFILE: home,
+          HERMES_HOME: profile,
+          HOPPER_LAUNCHER_PATH: process.execPath,
+        },
+        timeoutMs: 20_000,
+      });
+      expect(rejected.exitCode).not.toBe(0);
+      expect(rejected.json).toMatchObject({
+        status: "needs_human",
+        plannedActions: [],
+        appliedActions: [],
+      });
+      expect(await readFile(config, "utf8")).toBe(original);
+      await expect(access(`${config}.rea.backup`)).rejects.toMatchObject({
+        code: "ENOENT",
+      });
+    },
+  );
+}