From 91a9d1d4a729119edafe90d33e82f62ce245456c Mon Sep 17 00:00:00 2001 From: Bellam vedha kousik Date: Fri, 10 Jul 2026 12:59:12 +0530 Subject: [PATCH 1/2] fix(cli): guard customDeepMerge against circular references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit customDeepMerge's mergeRecursively recursed into nested plain objects with no cycle tracking, so a settings object containing a circular reference (e.g. `obj.self = obj`) caused unbounded recursion and crashed the settings manager with `RangeError: Maximum call stack size exceeded` — breaking CLI initialization until the user manually scrubbed their config. The repo already documented this with a test asserting the crash. Track the chain of source objects currently being merged in a WeakSet (added on entry, removed on exit so it represents only the current ancestor path). When a source value is a circular reference back to an ancestor, assign it by reference instead of recursing. Shared-but-non-circular references (DAGs) are unaffected and still merge normally. - Add regression tests for self, mutual (a->b->a), and shared-reference cases. - Flip the settings.test.ts circular-reference test from asserting the crash to asserting it no longer throws. Fixes #28270 Co-Authored-By: Claude Opus 4.8 Signed-off-by: Bellam vedha kousik --- packages/cli/src/config/settings.test.ts | 9 +++--- packages/cli/src/utils/deepMerge.test.ts | 37 +++++++++++++++++++++++- packages/cli/src/utils/deepMerge.ts | 25 +++++++++++++++- 3 files changed, 65 insertions(+), 6 deletions(-) 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..fb67a1e915f 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,39 @@ 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 preserved by reference rather than recursed into. + expect(result['self']).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; + + expect(() => customDeepMerge(getMergeStrategy, {}, a)).not.toThrow(); + }); + + 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..003f891bf01 100644 --- a/packages/cli/src/utils/deepMerge.ts +++ b/packages/cli/src/utils/deepMerge.ts @@ -26,7 +26,15 @@ function mergeRecursively( source: MergeableObject, getMergeStrategyForPath: (path: string[]) => MergeStrategy | undefined, path: string[] = [], + seen: WeakSet = new WeakSet(), ) { + // Track the chain of source objects currently being merged. A circular + // reference (a source object that points back to one of its own ancestors) + // is assigned by reference instead of being recursed into, which would + // otherwise overflow the stack. `seen` is scoped to the current recursion + // branch (added on entry, removed on exit), so shared but non-circular + // references are still merged normally. + seen.add(source); 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 +70,21 @@ function mergeRecursively( } } + if (isPlainObject(srcValue) && seen.has(srcValue)) { + // Circular reference back to an ancestor source object: assign the + // reference directly instead of recursing to avoid infinite recursion. + target[key] = srcValue; + continue; + } + if (isPlainObject(objValue) && isPlainObject(srcValue)) { - mergeRecursively(objValue, srcValue, getMergeStrategyForPath, newPath); + mergeRecursively( + objValue, + srcValue, + getMergeStrategyForPath, + newPath, + seen, + ); } else if (isPlainObject(srcValue)) { target[key] = {}; mergeRecursively( @@ -72,11 +93,13 @@ function mergeRecursively( srcValue, getMergeStrategyForPath, newPath, + seen, ); } else { target[key] = srcValue; } } + seen.delete(source); return target; } From 11948e90b0e8f7d748a92c243a87932361b63989 Mon Sep 17 00:00:00 2001 From: Bellam vedha kousik Date: Fri, 10 Jul 2026 13:26:39 +0530 Subject: [PATCH 2/2] fix(cli): reproduce cycles in the clone instead of aliasing the source Address Gemini Code Assist review: track source->clone in a Map instead of a WeakSet of visited sources. On a circular reference, assign the ancestor's *clone* rather than the original source object, so the merged result is a fully independent clone (result.self === result) and never embeds a live reference back into the caller's input (which risked cross-mutation of settings). Strengthen tests to assert the cycle is reproduced within the clone for both self-referential and mutual (a -> b -> a) cases. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Bellam vedha kousik --- packages/cli/src/utils/deepMerge.test.ts | 14 +++++++-- packages/cli/src/utils/deepMerge.ts | 36 ++++++++++++++---------- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/utils/deepMerge.test.ts b/packages/cli/src/utils/deepMerge.test.ts index fb67a1e915f..c5d6ffc573c 100644 --- a/packages/cli/src/utils/deepMerge.test.ts +++ b/packages/cli/src/utils/deepMerge.test.ts @@ -249,8 +249,10 @@ describe('customDeepMerge', () => { const result = customDeepMerge(getMergeStrategy, {}, circular); expect(result['a']).toBe(1); - // The cycle is preserved by reference rather than recursed into. - expect(result['self']).toBe(circular); + // 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', () => { @@ -260,7 +262,13 @@ describe('customDeepMerge', () => { b['a'] = a; // a -> b -> a const getMergeStrategy = () => undefined; - expect(() => customDeepMerge(getMergeStrategy, {}, a)).not.toThrow(); + 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', () => { diff --git a/packages/cli/src/utils/deepMerge.ts b/packages/cli/src/utils/deepMerge.ts index 003f891bf01..b0d644de4a9 100644 --- a/packages/cli/src/utils/deepMerge.ts +++ b/packages/cli/src/utils/deepMerge.ts @@ -26,15 +26,17 @@ function mergeRecursively( source: MergeableObject, getMergeStrategyForPath: (path: string[]) => MergeStrategy | undefined, path: string[] = [], - seen: WeakSet = new WeakSet(), + clones: Map = new Map(), ) { - // Track the chain of source objects currently being merged. A circular - // reference (a source object that points back to one of its own ancestors) - // is assigned by reference instead of being recursed into, which would - // otherwise overflow the stack. `seen` is scoped to the current recursion - // branch (added on entry, removed on exit), so shared but non-circular - // references are still merged normally. - seen.add(source); + // 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. @@ -70,10 +72,14 @@ function mergeRecursively( } } - if (isPlainObject(srcValue) && seen.has(srcValue)) { - // Circular reference back to an ancestor source object: assign the - // reference directly instead of recursing to avoid infinite recursion. - target[key] = srcValue; + 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; } @@ -83,7 +89,7 @@ function mergeRecursively( srcValue, getMergeStrategyForPath, newPath, - seen, + clones, ); } else if (isPlainObject(srcValue)) { target[key] = {}; @@ -93,13 +99,13 @@ function mergeRecursively( srcValue, getMergeStrategyForPath, newPath, - seen, + clones, ); } else { target[key] = srcValue; } } - seen.delete(source); + clones.delete(source); return target; }