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