diff --git a/src/application/DshCordisYaml.ts b/src/application/DshCordisYaml.ts index c7e88958f5..7bb664dab2 100644 --- a/src/application/DshCordisYaml.ts +++ b/src/application/DshCordisYaml.ts @@ -1,3 +1,4 @@ +import { retainYamlSequenceAnnotations } from "./YamlSequenceAnnotations.js"; import { isDeepStrictEqual } from "node:util"; import { isAlias, @@ -144,6 +145,22 @@ export class DshCordisYaml { if (original.commentBefore !== undefined) replacement.commentBefore = original.commentBefore; } + if (isSeq(original) && isSeq(replacement)) { + retainYamlSequenceAnnotations( + original, + replacement, + (node) => this.value(node), + (source, target) => { + if (source.commentBefore !== undefined) + target.commentBefore = source.commentBefore; + if (source.comment !== undefined) target.comment = source.comment; + if (source.spaceBefore !== undefined) + target.spaceBefore = source.spaceBefore; + return target; + }, + () => true, + ); + } if (pair === undefined) map.set(key, replacement); else pair.value = replacement; } diff --git a/src/application/HermesConfigurationAliases.ts b/src/application/HermesConfigurationAliases.ts index f52719cad2..e4456f7bf3 100644 --- a/src/application/HermesConfigurationAliases.ts +++ b/src/application/HermesConfigurationAliases.ts @@ -13,6 +13,7 @@ interface AliasCopies { readonly targets: ReadonlyMap; readonly removed: ReadonlySet; readonly retained: Map; + readonly copied: Set; readonly survivingNames: Map; readonly visiting: Set; readonly validated: Set; @@ -98,6 +99,7 @@ const copyAliasValue = ( const copy = cloned ?? source.clone(); if (!isNode(copy) || isAlias(copy)) throw new TypeError("Invalid Hermes YAML value copy"); + context.copied.add(source); if (source.anchor !== undefined) { copy.anchor = newAnchorName(source.anchor, context); context.retained.set(source, copy); @@ -127,7 +129,7 @@ const copyAliasValue = ( export const preserveHermesAliasValues = ( document: Document, previous: readonly unknown[], -): void => { +): ReadonlySet => { const roots = previous.filter(isNode); const removed = new Set(); for (const root of roots) @@ -136,7 +138,7 @@ export const preserveHermesAliasValues = ( if (node.anchor !== undefined) removed.add(node); }, }); - if (removed.size === 0) return; + if (removed.size === 0) return new Set(); const targets = new Map(); const names = new Set(); const reusedNames = new Set(); @@ -154,6 +156,7 @@ export const preserveHermesAliasValues = ( targets, removed, retained: new Map(), + copied: new Set(), survivingNames: new Map(), visiting: new Set(), validated: new Set(), @@ -189,4 +192,5 @@ export const preserveHermesAliasValues = ( : copyAliasComments(node, new Alias(name)); }, }); + return context.copied; }; diff --git a/src/application/HermesConfigurationYaml.test.ts b/src/application/HermesConfigurationYaml.test.ts index 4e60c4f9ba..2ae7ffd9b4 100644 --- a/src/application/HermesConfigurationYaml.test.ts +++ b/src/application/HermesConfigurationYaml.test.ts @@ -7,6 +7,30 @@ import { stringifyHermesConfigurationYaml, } from "./HermesConfigurationYaml.js"; +it("preserves annotations on changed registration mappings and scalar fields", () => { + const source = + "mcp_servers:\n rea: # registration note\n command: old # command note\n env: # environment note\n MODE: old # mode note\n args: # arguments note\n - old # argument note\n"; + const updated = editHermesConfigurationYaml(source, ["mcp_servers", "rea"], { + command: "new", + env: { MODE: "new" }, + args: ["new"], + }); + expect(parseHermesConfigurationYaml(updated)).toEqual({ + mcp_servers: { + rea: { command: "new", env: { MODE: "new" }, args: ["new"] }, + }, + }); + for (const comment of [ + "registration note", + "command note", + "environment note", + "mode note", + "arguments note", + "argument note", + ]) + expect(updated.split(comment)).toHaveLength(2); +}); + it("refreshes an anchored entry without changing another server's resolved value", () => { const source = "mcp_servers:\n rea: &server\n command: old\n enabled: false\n other: *server # Independent server\n"; @@ -23,6 +47,31 @@ it("refreshes an anchored entry without changing another server's resolved value expect(updated).toContain("# Independent server"); }); +it("keeps comments once when an updated anchored entry has a surviving consumer", () => { + const source = + "mcp_servers:\n rea: &server # registration note\n # command key note\n command: old # command note\n enabled: true # enabled note\n env: # environment note\n MODE: old # mode note\n other: *server\n"; + const updated = editHermesConfigurationYaml(source, ["mcp_servers", "rea"], { + command: "new", + enabled: true, + env: { MODE: "new" }, + }); + expect(parseHermesConfigurationYaml(updated)).toEqual({ + mcp_servers: { + rea: { command: "new", enabled: true, env: { MODE: "new" } }, + other: { command: "old", enabled: true, env: { MODE: "old" } }, + }, + }); + for (const comment of [ + "registration note", + "command key note", + "command note", + "enabled note", + "environment note", + "mode note", + ]) + expect(updated.split(comment)).toHaveLength(2); +}); + describe("Hermes surviving anchor definitions", () => { for (const shadowed of [false, true]) { it.each([undefined, { command: "rea" }])( @@ -499,3 +548,23 @@ describe("Hermes configuration YAML", () => { ).toEqual({ date: new Date("2026-10-14") }); }); }); + +it("keeps argument annotations with unique values and retains removed annotations on the list", () => { + const source = + "mcp_servers:\n rea:\n args:\n - mcp # mcp note\n - old # removed note\n - dup # first duplicate\n - dup # second duplicate\n"; + const updated = editHermesConfigurationYaml(source, ["mcp_servers", "rea"], { + args: ["script", "mcp", "dup"], + }); + expect(parseHermesConfigurationYaml(updated)).toEqual({ + mcp_servers: { rea: { args: ["script", "mcp", "dup"] } }, + }); + expect(updated).toMatch(/- mcp # mcp note/u); + expect(updated).not.toMatch(/- script # mcp note/u); + for (const comment of [ + "mcp note", + "removed note", + "first duplicate", + "second duplicate", + ]) + expect(updated.split(comment)).toHaveLength(2); +}); diff --git a/src/application/HermesConfigurationYaml.ts b/src/application/HermesConfigurationYaml.ts index d3cd77ed10..86a66bff03 100644 --- a/src/application/HermesConfigurationYaml.ts +++ b/src/application/HermesConfigurationYaml.ts @@ -1,3 +1,4 @@ +import { retainYamlSequenceAnnotations } from "./YamlSequenceAnnotations.js"; import { isAlias, isMap, @@ -328,10 +329,32 @@ export const parseHermesConfigurationYaml = (text: string): unknown => { }); }; -/** Keep unchanged scalar nodes, including their quoting and explicit tags. */ -const retainHermesScalarSource = (previous: unknown, next: Node): Node => { - if (isScalar(previous) && isScalar(next) && previous.value === next.value) - return previous; +/** Keep source annotations and unchanged scalars when updating a value. */ +const retainHermesSource = ( + previous: unknown, + next: Node, + retainedSources: ReadonlySet, +): Node => { + if (isScalar(previous) && isScalar(next) && previous.value === next.value) { + if (!retainedSources.has(previous)) return previous; + const copied = previous.clone(); + if (!isScalar(copied)) throw new TypeError("Invalid Hermes scalar copy"); + delete copied.anchor; + delete copied.comment; + delete copied.commentBefore; + delete copied.spaceBefore; + return copied; + } + // An anchor moved to a surviving consumer owns its original comments. + // Otherwise retain annotations on the replacement without retaining the + // old anchor, tag, or scalar spelling for the changed value. + if (isNode(previous) && !retainedSources.has(previous)) { + if (previous.comment !== undefined) next.comment = previous.comment; + if (previous.commentBefore !== undefined) + next.commentBefore = previous.commentBefore; + if (previous.spaceBefore !== undefined) + next.spaceBefore = previous.spaceBefore; + } if (isMap(previous) && isMap(next)) { const entries = new Map( previous.items.flatMap((pair) => @@ -344,16 +367,33 @@ const retainHermesScalarSource = (previous: unknown, next: Node): Node => { const key = isScalar(pair.key) ? pair.key.value : undefined; const prior = typeof key === "string" ? entries.get(key) : undefined; if (prior === undefined) continue; - pair.key = prior.key; + pair.key = isNode(pair.key) + ? retainHermesSource(prior.key, pair.key, retainedSources) + : prior.key; if (isNode(pair.value)) - pair.value = retainHermesScalarSource(prior.value, pair.value); + pair.value = retainHermesSource( + prior.value, + pair.value, + retainedSources, + ); } } else if (isSeq(previous) && isSeq(next)) { - next.items = next.items.map((item, index) => - isNode(item) - ? retainHermesScalarSource(previous.items[index], item) - : item, - ); + const stringItems = (items: readonly unknown[]) => + items.every((item) => isScalar(item) && typeof item.value === "string"); + if (stringItems(previous.items) && stringItems(next.items)) + retainYamlSequenceAnnotations( + previous, + next, + (item) => (isScalar(item) ? item.value : undefined), + (source, target) => retainHermesSource(source, target, retainedSources), + (source) => !retainedSources.has(source), + ); + else + next.items = next.items.map((item, index) => + isNode(item) + ? retainHermesSource(previous.items[index], item, retainedSources) + : item, + ); } return next; }; @@ -435,7 +475,10 @@ export const editHermesConfigurationYaml = ( (pair) => isScalar(pair.key) && pair.key.value === path.at(-1), )?.key : undefined; - preserveHermesAliasValues(document, [previous, removedKey]); + const retainedSources = preserveHermesAliasValues(document, [ + previous, + removedKey, + ]); if (value === undefined) { if (!document.deleteIn(path)) return text; } else { @@ -444,7 +487,11 @@ export const editHermesConfigurationYaml = ( document.setIn( path, isMap(previous) || isSeq(previous) - ? retainHermesScalarSource(previous, document.createNode(value)) + ? retainHermesSource( + previous, + document.createNode(value), + retainedSources, + ) : value, ); } diff --git a/src/application/YamlSequenceAnnotations.ts b/src/application/YamlSequenceAnnotations.ts new file mode 100644 index 0000000000..8d0559b99b --- /dev/null +++ b/src/application/YamlSequenceAnnotations.ts @@ -0,0 +1,47 @@ +import { isNode, type Node, type YAMLSeq } from "yaml"; + +/** Move unique argument annotations with their value; keep ambiguous comments on the list. */ +export const retainYamlSequenceAnnotations = ( + previous: YAMLSeq, + next: YAMLSeq, + valueOf: (item: unknown) => unknown, + retain: (source: Node, target: Node) => Node, + ownsComments: (source: Node) => boolean, +): void => { + const sourceCounts = new Map(); + const targetCounts = new Map(); + const targets = new Map(); + for (const item of previous.items) { + const value = valueOf(item); + if (typeof value === "string") + sourceCounts.set(value, (sourceCounts.get(value) ?? 0) + 1); + } + next.items.forEach((item, index) => { + const value = valueOf(item); + if (typeof value !== "string" || !isNode(item)) return; + targetCounts.set(value, (targetCounts.get(value) ?? 0) + 1); + targets.set(value, index); + }); + const orphaned: string[] = []; + for (const source of previous.items) { + if (!isNode(source)) continue; + const value = valueOf(source); + const index = + typeof value === "string" && + sourceCounts.get(value) === 1 && + targetCounts.get(value) === 1 + ? targets.get(value) + : undefined; + const target = index === undefined ? undefined : next.items[index]; + if (index !== undefined && isNode(target)) + next.items[index] = retain(source, target); + else if (ownsComments(source)) { + for (const comment of [source.commentBefore, source.comment]) + if (typeof comment === "string") orphaned.push(comment); + } + } + if (orphaned.length > 0) + next.commentBefore = [next.commentBefore, ...orphaned] + .filter((comment): comment is string => typeof comment === "string") + .join("\n"); +}; diff --git a/tests/acceptance/setup/hermesClientSetup.test.ts b/tests/acceptance/setup/hermesClientSetup.test.ts index b0a5a44efd..00c45c74d0 100644 --- a/tests/acceptance/setup/hermesClientSetup.test.ts +++ b/tests/acceptance/setup/hermesClientSetup.test.ts @@ -120,8 +120,8 @@ cliTest( const original = [ "# Caller configuration", "mcp_servers:", - " rea:", - " command: rea", + " rea: # Registration explanation", + " command: rea # Launcher explanation", " args: [mcp]", " env:", " CALLER_SETTING: keep exactly", @@ -183,6 +183,8 @@ cliTest( mcp_servers: { rea: { command: "rea" } }, }); expect(configured).toContain("# Curated for this client."); + expect(configured).toContain("# Registration explanation"); + expect(configured).toContain("# Launcher explanation"); expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original); expect( (await run(["doctor", "--client", "hermes", "--skill=false", "--json"])) diff --git a/tests/boundary/filesystem/dshClientConfiguration.test.ts b/tests/boundary/filesystem/dshClientConfiguration.test.ts index d775ca2378..deca95a211 100644 --- a/tests/boundary/filesystem/dshClientConfiguration.test.ts +++ b/tests/boundary/filesystem/dshClientConfiguration.test.ts @@ -2231,7 +2231,7 @@ describe("DeepSeek Harness ordered selector applicability", () => { }); describe("DeepSeek Harness caller node metadata", () => { - it.each(["environment", "launcher"])( + it.each(["environment", "launcher", "arguments"])( "preserves scalar and argument comments during %s repairs", async (repair) => { const { home, client } = await harnessClient(); @@ -2245,6 +2245,9 @@ describe("DeepSeek Harness caller node metadata", () => { config: { ...entry.config, ...(repair === "launcher" ? { command: "stale" } : {}), + ...(repair === "arguments" + ? { command: "stale", args: ["mcp", "--legacy"] } + : {}), env: { TOKEN: "original" }, }, }, @@ -2273,6 +2276,10 @@ describe("DeepSeek Harness caller node metadata", () => { if (!isSeq(args) || !isNode(args.items[0])) throw new Error("missing launcher arguments"); args.items[0].comment = " caller argument"; + if (repair === "arguments") { + if (!isNode(args.items[1])) throw new Error("missing old argument"); + args.items[1].comment = " removed argument explanation"; + } const initial = yaml.toString(); await writeFile(client.configPath, initial); expect( @@ -2298,6 +2305,23 @@ describe("DeepSeek Harness caller node metadata", () => { expect(text).toContain(`# before ${key}`); } expect(text).toContain("# caller argument"); + if (repair === "arguments") { + expect(text).toContain("# removed argument explanation"); + const updatedArgs = parseDocument(text).getIn( + [0, "insert", 0, "config", "args"], + true, + ); + if (!isSeq(updatedArgs)) throw new Error("missing updated arguments"); + expect(updatedArgs.commentBefore).toContain( + "removed argument explanation", + ); + const mcp = updatedArgs.items.find( + (item) => isNode(item) && "value" in item && item.value === "mcp", + ); + expect(isNode(mcp) ? mcp.comment : undefined).toContain( + "caller argument", + ); + } expect(loadedInserts(text)).toEqual([ { ...entry,