fix(commons): use ancestor-chain tracking in deepMerge to correctly handle shared references

deepMerge used a WeakSet to detect circular references, which incorrectly
skipped shared (non-circular) object references. Replace with an ancestor
array (push/pop stack) so only true cycles are detected.

Author: svozza

packages/commons/src/deepMerge.ts

@@ -24,23 +24,24 @@ const isPlainObject = (value: unknown): value is Record<string, unknown> => {
 const mergeArrayItemsByIndex = (
   targetArray: unknown[],
   sourceArray: unknown[],
-  seen: WeakSet<object>
+  ancestors: object[]
 ): void => {
   for (let i = 0; i < sourceArray.length; i++) {
     const srcItem = sourceArray[i];
     const tgtItem = targetArray[i];
 
     const isSrcPlainObject = isPlainObject(srcItem);
 
-    // Skip already-seen objects to prevent circular references
-    if (isSrcPlainObject && seen.has(srcItem)) {
+    // Skip objects in the current ancestor chain to prevent circular references
+    if (isSrcPlainObject && ancestors.includes(srcItem)) {
       continue;
     }
 
     // Merge nested plain objects recursively
     if (isSrcPlainObject && isPlainObject(tgtItem)) {
-      seen.add(srcItem);
-      mergeRecursive(tgtItem, srcItem, seen);
+      ancestors.push(srcItem);
+      mergeRecursive(tgtItem, srcItem, ancestors);
+      ancestors.pop();
       continue;
     }
 
@@ -61,14 +62,14 @@ const handleArrayMerge = (
   key: string,
   sourceArray: unknown[],
   targetValue: unknown,
-  seen: WeakSet<object>
+  ancestors: object[]
 ): void => {
   if (!Array.isArray(targetValue)) {
     target[key] = [...sourceArray];
     return;
   }
 
-  mergeArrayItemsByIndex(targetValue, sourceArray, seen);
+  mergeArrayItemsByIndex(targetValue, sourceArray, ancestors);
 };
 
 /**
@@ -81,15 +82,15 @@ const handleObjectMerge = (
   key: string,
   sourceObject: Record<string, unknown>,
   targetValue: unknown,
-  seen: WeakSet<object>
+  ancestors: object[]
 ): void => {
   if (isPlainObject(targetValue)) {
-    mergeRecursive(targetValue, sourceObject, seen);
+    mergeRecursive(targetValue, sourceObject, ancestors);
     return;
   }
 
   const newTarget: Record<string, unknown> = {};
-  mergeRecursive(newTarget, sourceObject, seen);
+  mergeRecursive(newTarget, sourceObject, ancestors);
   target[key] = newTarget;
 };
 
@@ -101,7 +102,7 @@ const handleObjectMerge = (
 const mergeRecursive = (
   target: Record<string, unknown>,
   source: Record<string, unknown>,
-  seen: WeakSet<object>
+  ancestors: object[]
 ): void => {
   for (const key of Object.keys(source)) {
     if (UNSAFE_KEYS.has(key)) {
@@ -112,16 +113,18 @@ const mergeRecursive = (
     const targetValue = target[key];
 
     if (Array.isArray(sourceValue)) {
-      if (seen.has(sourceValue)) continue;
-      seen.add(sourceValue);
-      handleArrayMerge(target, key, sourceValue, targetValue, seen);
+      if (ancestors.includes(sourceValue)) continue;
+      ancestors.push(sourceValue);
+      handleArrayMerge(target, key, sourceValue, targetValue, ancestors);
+      ancestors.pop();
       continue;
     }
 
     if (isPlainObject(sourceValue)) {
-      if (seen.has(sourceValue)) continue;
-      seen.add(sourceValue);
-      handleObjectMerge(target, key, sourceValue, targetValue, seen);
+      if (ancestors.includes(sourceValue)) continue;
+      ancestors.push(sourceValue);
+      handleObjectMerge(target, key, sourceValue, targetValue, ancestors);
+      ancestors.pop();
       continue;
     }
 
@@ -135,8 +138,9 @@ const mergeRecursive = (
  * Recursively merge properties from source objects into the target object, mutating it.
  *
  * Nested plain objects are merged recursively, arrays are merged by index (e.g., `[1, 2]` + `[3]` → `[3, 2]`),
- * and class instances (Date, RegExp, custom classes) are assigned by reference. Circular references and
- * prototype pollution attempts (`__proto__`, `constructor`) are safely skipped.
+ * and class instances (Date, RegExp, custom classes) are assigned by reference. Circular references are
+ * detected via ancestor-chain tracking and safely skipped, while shared (non-circular) object references
+ * are merged correctly. Prototype pollution attempts (`__proto__`, `constructor`) are also skipped.
  *
  * @example
  * ```typescript
@@ -155,13 +159,13 @@ const deepMerge = <T extends Record<string, unknown>>(
   target: T,
   ...sources: Array<Record<string, unknown> | undefined | null>
 ): T => {
-  const seen = new WeakSet<object>();
-  seen.add(target);
+  const ancestors: object[] = [target];
 
   for (const source of sources) {
     if (source != null) {
-      seen.add(source);
-      mergeRecursive(target, source, seen);
+      ancestors.push(source);
+      mergeRecursive(target, source, ancestors);
+      ancestors.pop();
     }
   }
 

packages/commons/tests/unit/deepMerge.test.ts

@@ -303,7 +303,7 @@ describe('Function: deepMerge', () => {
       expect((result.arr as unknown[])[1]).toBe(2);
     });
 
-    it('skips already-seen arrays to prevent duplication', () => {
+    it('merges shared array references into all properties', () => {
       // Prepare
       const target = {};
       const sharedArray = [1, 2, 3];
@@ -315,16 +315,14 @@ describe('Function: deepMerge', () => {
       // Act
       const result = deepMerge(target, source);
 
-      // Assess - shared array is copied on first occurrence,
-      // skipped on subsequent occurrences
+      // Assess - shared (non-circular) array is merged into both properties
       expect(result).toEqual({
         first: [1, 2, 3],
-        second: undefined,
+        second: [1, 2, 3],
       });
-      expect(result).not.toHaveProperty('second');
     });
 
-    it('skips already-seen objects within arrays', () => {
+    it('merges shared objects within arrays', () => {
       // Prepare
       const sharedObj = { shared: true };
       const target = {
@@ -338,17 +336,85 @@ describe('Function: deepMerge', () => {
       // Act
       const result = deepMerge(target, source);
 
-      // Assess - sharedObj is merged into prop first, then skipped when
-      // encountered again in the array merge
+      // Assess - sharedObj is merged into both prop and arr[0]
       expect(result.prop).toEqual({ shared: true });
-      expect((result.arr as Record<string, unknown>[])[0]).toEqual({ a: 1 });
+      expect((result.arr as Record<string, unknown>[])[0]).toEqual({
+        a: 1,
+        shared: true,
+      });
+      expect((result.arr as Record<string, unknown>[])[1]).toEqual({
+        b: 2,
+        c: 3,
+      });
+    });
+
+    it('merges shared objects into all referencing properties', () => {
+      // Prepare
+      const target = {};
+      const shared = { value: 42 };
+      const source = {
+        first: shared,
+        second: { nested: shared },
+      };
+
+      // Act
+      const result = deepMerge(target, source);
+
+      // Assess - shared (non-circular) object appears in both locations
+      expect(result).toEqual({
+        first: { value: 42 },
+        second: { nested: { value: 42 } },
+      });
+    });
+  });
+
+  describe('Shared (non-circular) references', () => {
+    it('correctly merges shared array references into both properties', () => {
+      // Prepare
+      const target = {};
+      const sharedArray = [1, 2, 3];
+      const source = {
+        first: sharedArray,
+        second: sharedArray,
+      };
+
+      // Act
+      const result = deepMerge(target, source);
+
+      // Assess - shared array should be merged into both properties
+      expect(result).toEqual({
+        first: [1, 2, 3],
+        second: [1, 2, 3],
+      });
+    });
+
+    it('correctly merges shared objects referenced in arrays', () => {
+      // Prepare
+      const sharedObj = { shared: true };
+      const target = {
+        arr: [{ a: 1 }, { b: 2 }],
+      };
+      const source = {
+        prop: sharedObj,
+        arr: [sharedObj, { c: 3 }],
+      };
+
+      // Act
+      const result = deepMerge(target, source);
+
+      // Assess - sharedObj should be merged into both prop and arr[0]
+      expect(result.prop).toEqual({ shared: true });
+      expect((result.arr as Record<string, unknown>[])[0]).toEqual({
+        a: 1,
+        shared: true,
+      });
       expect((result.arr as Record<string, unknown>[])[1]).toEqual({
         b: 2,
         c: 3,
       });
     });
 
-    it('skips already-seen objects to prevent duplication', () => {
+    it('correctly merges shared objects into all referencing properties', () => {
       // Prepare
       const target = {};
       const shared = { value: 42 };
@@ -360,11 +426,81 @@ describe('Function: deepMerge', () => {
       // Act
       const result = deepMerge(target, source);
 
-      // Assess - shared object is merged on first occurrence,
-      // skipped on subsequent occurrences to prevent infinite recursion
+      // Assess - shared object should appear in both locations
       expect(result).toEqual({
         first: { value: 42 },
-        second: {},
+        second: { nested: { value: 42 } },
+      });
+    });
+
+    it('correctly merges shared objects across multiple sources', () => {
+      // Prepare
+      const shared = { x: 1 };
+      const target = {};
+      const source1 = { a: shared };
+      const source2 = { b: shared };
+
+      // Act
+      const result = deepMerge(target, source1, source2);
+
+      // Assess
+      expect(result).toEqual({ a: { x: 1 }, b: { x: 1 } });
+    });
+
+    it('correctly merges diamond-shaped shared references', () => {
+      // Prepare
+      const shared = { value: 'shared' };
+      const target = {};
+      const source = {
+        branch1: { leaf: shared },
+        branch2: { leaf: shared },
+      };
+
+      // Act
+      const result = deepMerge(target, source);
+
+      // Assess - same object at different depths in two branches
+      expect(result).toEqual({
+        branch1: { leaf: { value: 'shared' } },
+        branch2: { leaf: { value: 'shared' } },
+      });
+    });
+
+    it('merges a shared object that itself contains a circular reference', () => {
+      // Prepare
+      const shared: Record<string, unknown> = { value: 1 };
+      shared.self = shared;
+      const target = {};
+      const source = {
+        first: shared,
+        second: { nested: shared },
+      };
+
+      // Act
+      const result = deepMerge(target, source);
+
+      // Assess - shared object is merged in both locations,
+      // but the circular self-reference within it is skipped
+      expect(result).toEqual({
+        first: { value: 1 },
+        second: { nested: { value: 1 } },
+      });
+    });
+
+    it('merges when source references an object from the target', () => {
+      // Prepare
+      const inner = { x: 1 };
+      const target: Record<string, unknown> = { a: inner };
+      const source = { b: inner };
+
+      // Act
+      const result = deepMerge(target, source);
+
+      // Assess - inner is not in the ancestor chain when processing source,
+      // so it should be merged into both properties
+      expect(result).toEqual({
+        a: { x: 1 },
+        b: { x: 1 },
       });
     });
   });