fix(setup): preserve YAML annotations during refresh (#7944)

* fix(setup): preserve YAML annotations during refresh

* fix(setup): preserve yaml argument comment ownership

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

src/application/HermesConfigurationAliases.ts

@@ -13,6 +13,7 @@ interface AliasCopies {
   readonly targets: ReadonlyMap<Alias, Node | undefined>;
   readonly removed: ReadonlySet<Node>;
   readonly retained: Map<Node, Node>;
+  readonly copied: Set<Node>;
   readonly survivingNames: Map<Node, string>;
   readonly visiting: Set<Node>;
   readonly validated: Set<Node>;
@@ -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<Node> => {
   const roots = previous.filter(isNode);
   const removed = new Set<Node>();
   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<Alias, Node | undefined>();
   const names = new Set<string>();
   const reusedNames = new Set<string>();
@@ -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;
 };

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

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

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<string, number>();
+  const targetCounts = new Map<string, number>();
+  const targets = new Map<string, number>();
+  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");
+};

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

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,