From 4a90059fe2a8e561a682fdd36f81f1120cfef686 Mon Sep 17 00:00:00 2001 From: Haoran Xu Date: Fri, 23 Oct 2026 02:32:48 +0800 Subject: [PATCH] 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> --- .../HermesConfigurationYaml.test.ts | 51 +++ src/application/HermesConfigurationYaml.ts | 62 +++- .../setup/hermesAliasedTableSetup.test.ts | 315 ++++++++++++++++++ 3 files changed, 420 insertions(+), 8 deletions(-) create mode 100644 tests/acceptance/setup/hermesAliasedTableSetup.test.ts diff --git a/src/application/HermesConfigurationYaml.test.ts b/src/application/HermesConfigurationYaml.test.ts index 4297b33a0d..e1b8507b36 100644 --- a/src/application/HermesConfigurationYaml.test.ts +++ b/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 = diff --git a/src/application/HermesConfigurationYaml.ts b/src/application/HermesConfigurationYaml.ts index 43471fa290..11ddad51e6 100644 --- a/src/application/HermesConfigurationYaml.ts +++ b/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(), +): 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. diff --git a/tests/acceptance/setup/hermesAliasedTableSetup.test.ts b/tests/acceptance/setup/hermesAliasedTableSetup.test.ts new file mode 100644 index 0000000000..3e7b49626e --- /dev/null +++ b/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", + }); + }, + ); +}