fix(cli): guard customDeepMerge against circular references#28349
fix(cli): guard customDeepMerge against circular references#28349vedhakoushik wants to merge 4 commits into
Conversation
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 google-gemini#28270 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Bellam vedha kousik <bellam.vedhakoushik@gmail.com>
|
📊 PR Size: size/M
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@googlebot I signed it! |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where circular references in configuration objects caused unbounded recursion and stack overflow crashes in the CLI's settings manager. By introducing a cycle-tracking mechanism using a WeakSet during the deep merge process, the fix allows the system to gracefully handle circular structures by assigning them by reference instead of recursing, while maintaining correct behavior for non-circular shared references. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces circular reference handling to customDeepMerge to prevent stack overflow errors when merging self-referential or mutually circular objects, using a WeakSet to track visited source objects during recursion. The review feedback suggests using a clones map instead of a WeakSet to ensure that circular references are preserved within the newly cloned structure rather than pointing back to the original source objects, and provides corresponding updates to the test assertions.
| 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; | ||
| } |
There was a problem hiding this comment.
Retrieve the cloned ancestor from the clones map instead of assigning the original srcValue directly.
| 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(srcValue) && clones.has(srcValue)) { | |
| // Circular reference back to an ancestor source object: assign the | |
| // cloned reference directly instead of recursing to avoid infinite recursion | |
| // and reference sharing with the original source object. | |
| target[key] = clones.get(srcValue)!; | |
| continue; | |
| } |
There was a problem hiding this comment.
Done in 11948e9. Switched from a WeakSet of visited sources to a Map<source, clone>, and on a circular reference now assign the ancestor's clone so the merge produces a fully independent clone (result.self === result) rather than embedding a reference to the original source. Used a lint-safe ancestorClone !== undefined check instead of a non-null assertion.
| target[key] = srcValue; | ||
| } | ||
| } | ||
| seen.delete(source); |
| 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); |
There was a problem hiding this comment.
Update the test assertion to verify that the circular reference is preserved within the cloned structure, rather than pointing back to the original source object.
| 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); | |
| const result = customDeepMerge(getMergeStrategy, {}, circular); | |
| expect(result['a']).toBe(1); | |
| // The cycle is preserved within the cloned structure rather than pointing back to the source. | |
| expect(result['self']).toBe(result); |
There was a problem hiding this comment.
Done in 11948e9. The assertion now checks expect(result['self']).toBe(result) (plus .not.toBe(circular)), and I strengthened the mutual-cycle test to assert result.b.a === result.
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 <noreply@anthropic.com> Signed-off-by: Bellam vedha kousik <bellam.vedhakoushik@gmail.com>
|
@googlebot I signed it! |
|
Superseded by #28387 — same fix, clean re-submission without the Co-Authored-By trailer that was blocking the CLA check. |
What
Fixes #28270.
customDeepMerge'smergeRecursivelyrecursed 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 withRangeError: Maximum call stack size exceeded, breaking CLI initialization until the user manually scrubbed their config.The repo already documented this limitation with a test in
settings.test.tsthat asserted the crash (.toThrow(/Maximum call stack size exceeded/)).Fix
packages/cli/src/utils/deepMerge.ts— track the chain of source objects currently being merged in aWeakSet, added on entry and removed on exit so it represents only the current ancestor path (not already-processed siblings). When a source value is a circular reference back to an ancestor, assign it by reference instead of recursing.This is deliberately scoped so that shared-but-non-circular references (DAGs) still merge normally — only true cycles are short-circuited.
Tests
deepMerge.test.ts: added regression tests for self-reference (obj.self = obj), mutual/indirect cycles (a → b → a), and a shared non-circular reference (DAG) to guard against over-eager short-circuiting.settings.test.ts: flipped the existing circular-reference test from asserting the crash to asserting it no longer throws.Verification
deepMerge.test.ts— 20/20 pass (4 new)settings.test.tscircular-reference test — passeseslint,tsc --noEmit, andprettier --checkall clean on the changed files🤖 Authored with Claude Code. First-time contributor here — happy to adjust the cycle-handling behavior (e.g. omit vs. assign-by-reference) to match maintainer preference.