fix(config): harden object-array merge-by-id fallback

src/config/merge-patch.test.ts

@@ -72,7 +72,7 @@ describe("applyMergePatch", () => {
       agents: {
         list: [
           { id: "primary", model: "new-model" },
-          { workspace: "/tmp/orphan" }, // no id
+          { workspace: "/tmp/orphan" },
         ],
       },
     };
@@ -84,7 +84,6 @@ describe("applyMergePatch", () => {
         list?: Array<{ id?: string; workspace?: string; model?: string }>;
       };
     };
-    // Both original entries preserved, patch without id appended
     expect(merged.agents?.list).toHaveLength(3);
     const primary = merged.agents?.list?.find((entry) => entry.id === "primary");
     expect(primary?.workspace).toBe("/tmp/one");
@@ -117,16 +116,55 @@ describe("applyMergePatch", () => {
         list?: Array<{ id?: string; workspace?: string; model?: string; default?: boolean }>;
       };
     };
-    // All 4 agents must survive
     expect(merged.agents?.list).toHaveLength(4);
-    const main = merged.agents?.list?.find((e) => e.id === "main");
+    const main = merged.agents?.list?.find((entry) => entry.id === "main");
     expect(main?.model).toBe("claude-opus-4-20250918");
     expect(main?.default).toBe(true);
     expect(main?.workspace).toBe("/home/main");
-    // Others untouched
-    expect(merged.agents?.list?.find((e) => e.id === "ota")?.workspace).toBe("/home/ota");
-    expect(merged.agents?.list?.find((e) => e.id === "trading")?.workspace).toBe("/home/trading");
-    expect(merged.agents?.list?.find((e) => e.id === "codex")?.workspace).toBe("/home/codex");
+    expect(merged.agents?.list?.find((entry) => entry.id === "ota")?.workspace).toBe("/home/ota");
+    expect(merged.agents?.list?.find((entry) => entry.id === "trading")?.workspace).toBe(
+      "/home/trading",
+    );
+    expect(merged.agents?.list?.find((entry) => entry.id === "codex")?.workspace).toBe(
+      "/home/codex",
+    );
+  });
+
+  it("keeps existing id entries when patch mixes id and primitive entries", () => {
+    const base = {
+      agents: {
+        list: [
+          { id: "primary", workspace: "/tmp/one" },
+          { id: "secondary", workspace: "/tmp/two" },
+        ],
+      },
+    };
+    const patch = {
+      agents: {
+        list: [{ id: "primary", workspace: "/tmp/one-updated" }, "non-object entry"],
+      },
+    };
+
+    const merged = applyMergePatch(base, patch, {
+      mergeObjectArraysById: true,
+    }) as {
+      agents?: {
+        list?: Array<{ id?: string; workspace?: string } | string>;
+      };
+    };
+
+    expect(merged.agents?.list).toHaveLength(3);
+    const primary = merged.agents?.list?.find(
+      (entry): entry is { id?: string; workspace?: string } =>
+        typeof entry === "object" && entry !== null && "id" in entry && entry.id === "primary",
+    );
+    const secondary = merged.agents?.list?.find(
+      (entry): entry is { id?: string; workspace?: string } =>
+        typeof entry === "object" && entry !== null && "id" in entry && entry.id === "secondary",
+    );
+    expect(primary?.workspace).toBe("/tmp/one-updated");
+    expect(secondary?.workspace).toBe("/tmp/two");
+    expect(merged.agents?.list?.[2]).toBe("non-object entry");
   });
 
   it("falls back to replacement for non-id arrays even when enabled", () => {

src/config/merge-patch.ts

@@ -13,25 +13,35 @@ function isObjectWithStringId(value: unknown): value is Record<string, unknown>
   return typeof value.id === "string" && value.id.length > 0;
 }
 
-function mergeObjectArraysById(base: unknown[], patch: unknown[], options: MergePatchOptions) {
-  // Require all *base* entries to have string ids — if the existing array
-  // isn't id-keyed there's nothing sensible to merge against.
+/**
+ * Merge arrays of object-like entries keyed by `id`.
+ *
+ * Contract:
+ * - Base array must be fully id-keyed; otherwise return undefined (caller should replace).
+ * - Patch entries with valid id merge by id (or append when the id is new).
+ * - Patch entries without valid id append as-is, avoiding destructive full-array replacement.
+ */
+function mergeObjectArraysById(
+  base: unknown[],
+  patch: unknown[],
+  options: MergePatchOptions,
+): unknown[] | undefined {
   if (!base.every(isObjectWithStringId)) {
     return undefined;
   }
 
-  const merged = [...base] as Array<Record<string, unknown> & { id: string }>;
+  const merged: unknown[] = [...base];
   const indexById = new Map<string, number>();
   for (const [index, entry] of merged.entries()) {
+    if (!isObjectWithStringId(entry)) {
+      return undefined;
+    }
     indexById.set(entry.id, index);
   }
 
   for (const patchEntry of patch) {
-    // Patch entries without a valid id are appended as-is (best-effort).
-    // This prevents the entire merge from falling back to full replacement
-    // just because one patch element is missing an id field.
     if (!isObjectWithStringId(patchEntry)) {
-      merged.push(structuredClone(patchEntry) as Record<string, unknown> & { id: string });
+      merged.push(structuredClone(patchEntry));
       continue;
     }
 
@@ -41,10 +51,8 @@ function mergeObjectArraysById(base: unknown[], patch: unknown[], options: Merge
       indexById.set(patchEntry.id, merged.length - 1);
       continue;
     }
-    merged[existingIndex] = applyMergePatch(merged[existingIndex], patchEntry, options) as Record<
-      string,
-      unknown
-    > & { id: string };
+
+    merged[existingIndex] = applyMergePatch(merged[existingIndex], patchEntry, options);
   }
 
   return merged;