diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 6e9bcee9991..f3934a9cbba 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -3636,15 +3636,16 @@ describe('LoadedSettings Isolation and Serializability', () => { expect(settingsValue.myMap).not.toBe(mapValue.myMap); }); - it('should handle circular references (structuredClone supports them, but deepMerge may not)', () => { + it('should handle circular references without crashing', () => { const circular: Record = { a: 1 }; circular['self'] = circular; - // structuredClone(circular) works, but LoadedSettings.setValue calls - // computeMergedSettings() -> customDeepMerge() which blows up on circularity. + // LoadedSettings.setValue() calls computeMergedSettings() -> + // customDeepMerge(), which now guards against circular references instead + // of overflowing the stack (see deepMerge cycle handling). expect(() => { loadedSettings.setValue(SettingScope.User, 'test', circular); - }).toThrow(/Maximum call stack size exceeded/); + }).not.toThrow(); }); }); }); diff --git a/packages/cli/src/utils/deepMerge.test.ts b/packages/cli/src/utils/deepMerge.test.ts index 3310924795c..c5d6ffc573c 100644 --- a/packages/cli/src/utils/deepMerge.test.ts +++ b/packages/cli/src/utils/deepMerge.test.ts @@ -5,7 +5,7 @@ */ import { describe, it, expect } from 'vitest'; -import { customDeepMerge } from './deepMerge.js'; +import { customDeepMerge, type MergeableObject } from './deepMerge.js'; import { MergeStrategy } from '../config/settingsSchema.js'; describe('customDeepMerge', () => { @@ -237,4 +237,47 @@ describe('customDeepMerge', () => { const result = customDeepMerge(getMergeStrategy, target, source); expect(result).toEqual({ a: 1 }); }); + + it('should handle self-referential objects without stack overflow', () => { + const circular: MergeableObject = { a: 1 }; + circular['self'] = circular; + const getMergeStrategy = () => undefined; + + expect(() => + customDeepMerge(getMergeStrategy, { existing: true }, circular), + ).not.toThrow(); + + const result = customDeepMerge(getMergeStrategy, {}, circular); + expect(result['a']).toBe(1); + // The cycle is reproduced inside the cloned structure rather than pointing + // back to the original source object (a fully independent clone). + expect(result['self']).toBe(result); + expect(result['self']).not.toBe(circular); + }); + + it('should handle indirect (mutual) circular references', () => { + const a: MergeableObject = { name: 'a' }; + const b: MergeableObject = { name: 'b' }; + a['b'] = b; + b['a'] = a; // a -> b -> a + const getMergeStrategy = () => undefined; + + const result = customDeepMerge(getMergeStrategy, {}, a); + // The mutual cycle is reproduced within the clone (a -> b -> a), not shared + // with the original source objects. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((result as any)['b']['a']).toBe(result); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((result as any)['b']['a']).not.toBe(a); + }); + + it('should still merge shared but non-circular references normally', () => { + // The same nested object referenced twice is a DAG, not a cycle, and must + // still be merged (the cycle guard must not short-circuit it). + const shared = { x: 1 }; + const source = { first: shared, second: shared }; + const getMergeStrategy = () => undefined; + const result = customDeepMerge(getMergeStrategy, {}, source); + expect(result).toEqual({ first: { x: 1 }, second: { x: 1 } }); + }); }); diff --git a/packages/cli/src/utils/deepMerge.ts b/packages/cli/src/utils/deepMerge.ts index 2eef3b4adae..b0d644de4a9 100644 --- a/packages/cli/src/utils/deepMerge.ts +++ b/packages/cli/src/utils/deepMerge.ts @@ -26,7 +26,17 @@ function mergeRecursively( source: MergeableObject, getMergeStrategyForPath: (path: string[]) => MergeStrategy | undefined, path: string[] = [], + clones: Map = new Map(), ) { + // Track the chain of source objects currently being merged, mapping each to + // the clone being built for it. A circular reference (a source object that + // points back to one of its own ancestors) is resolved to that ancestor's + // clone instead of being recursed into, which would otherwise overflow the + // stack — and keeps the merged result a fully independent clone rather than + // embedding a reference to the original source. Entries are scoped to the + // current recursion branch (added on entry, removed on exit), so shared but + // non-circular references are still merged normally. + clones.set(source, target); for (const key of Object.keys(source)) { // JSON.parse can create objects with __proto__ as an own property. // We must skip it to prevent prototype pollution. @@ -62,8 +72,25 @@ function mergeRecursively( } } + const ancestorClone = isPlainObject(srcValue) + ? clones.get(srcValue) + : undefined; + if (ancestorClone !== undefined) { + // Circular reference back to an ancestor source object: assign that + // ancestor's clone so the cycle is reproduced inside the new structure, + // instead of recursing forever or leaking a reference to the source. + target[key] = ancestorClone; + continue; + } + if (isPlainObject(objValue) && isPlainObject(srcValue)) { - mergeRecursively(objValue, srcValue, getMergeStrategyForPath, newPath); + mergeRecursively( + objValue, + srcValue, + getMergeStrategyForPath, + newPath, + clones, + ); } else if (isPlainObject(srcValue)) { target[key] = {}; mergeRecursively( @@ -72,11 +99,13 @@ function mergeRecursively( srcValue, getMergeStrategyForPath, newPath, + clones, ); } else { target[key] = srcValue; } } + clones.delete(source); return target; }