From e53a5282ccb6cc097bc014dc53663baaa4bc17cc Mon Sep 17 00:00:00 2001 From: snf Date: Thu, 21 May 2026 15:58:39 +0300 Subject: [PATCH 1/2] fix(core): preserve duplicate object references in safeJsonStringify The replacer kept a WeakSet of every object it had ever seen. JSON.stringify calls the replacer for every key in a DFS walk, siblings included, and the set was never trimmed when the walk unwound. So the second sibling that pointed at the same object got replaced with [Circular]. Not a cycle, just a duplicate reference. Same false positive for repeated array elements and for any shared leaf that appears on more than one branch. Track the current ancestor path instead. The replacer's `this` is the parent of `value`, so on each call pop the stack back to wherever the walk currently is, then check the remaining ancestors for membership. Only real cycles get flagged. Existing cycle tests still pass. Added five regression tests covering shared siblings, repeated array elements, shared subtree leaves, indirect cycles, and a mix of duplicate ref + real cycle in the same graph. --- .../core/src/utils/safeJsonStringify.test.ts | 59 +++++++++++++++++++ packages/core/src/utils/safeJsonStringify.ts | 26 +++++--- 2 files changed, 78 insertions(+), 7 deletions(-) diff --git a/packages/core/src/utils/safeJsonStringify.test.ts b/packages/core/src/utils/safeJsonStringify.test.ts index 9a38c048810..e2ff7527f6d 100644 --- a/packages/core/src/utils/safeJsonStringify.test.ts +++ b/packages/core/src/utils/safeJsonStringify.test.ts @@ -70,4 +70,63 @@ describe('safeJsonStringify', () => { expect(safeJsonStringify(42)).toBe('42'); expect(safeJsonStringify(true)).toBe('true'); }); + + it('should preserve duplicate sibling references as full copies', () => { + // The same object referenced from two sibling properties is not a cycle: + // both branches must serialize in full, matching native JSON.stringify. + const shared = { name: 'shared', n: 1 }; + const obj = { a: shared, b: shared }; + + const result = safeJsonStringify(obj); + expect(result).toBe( + '{"a":{"name":"shared","n":1},"b":{"name":"shared","n":1}}', + ); + expect(result).not.toContain('[Circular]'); + }); + + it('should preserve duplicate references repeated in an array', () => { + const shared = { id: 1 }; + const arr = [shared, shared, shared]; + + const result = safeJsonStringify(arr); + expect(result).toBe('[{"id":1},{"id":1},{"id":1}]'); + expect(result).not.toContain('[Circular]'); + }); + + it('should preserve a shared leaf appearing on multiple branches', () => { + const leaf = { kind: 'leaf' }; + const tree = { left: { sub: leaf }, right: { sub: leaf } }; + + const result = safeJsonStringify(tree); + expect(result).toBe( + '{"left":{"sub":{"kind":"leaf"}},"right":{"sub":{"kind":"leaf"}}}', + ); + expect(result).not.toContain('[Circular]'); + }); + + it('should detect indirect cycles via an intermediate object', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const parent: any = { name: 'parent' }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const child: any = { name: 'child' }; + parent.child = child; + child.parent = parent; + + const result = safeJsonStringify(parent); + expect(result).toBe( + '{"name":"parent","child":{"name":"child","parent":"[Circular]"}}', + ); + }); + + it('should preserve a shared subtree alongside a real cycle', () => { + const shared = { tag: 'shared' }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const root: any = { a: shared, b: shared }; + root.self = root; + + const result = safeJsonStringify(root); + expect(result).toBe( + '{"a":{"tag":"shared"},"b":{"tag":"shared"},"self":"[Circular]"}', + ); + }); }); diff --git a/packages/core/src/utils/safeJsonStringify.ts b/packages/core/src/utils/safeJsonStringify.ts index f439bcea1e9..0b73a6085ac 100644 --- a/packages/core/src/utils/safeJsonStringify.ts +++ b/packages/core/src/utils/safeJsonStringify.ts @@ -7,6 +7,11 @@ /** * Safely stringifies an object to JSON, handling circular references by replacing them with [Circular]. * + * Only true cycles (an object reachable from itself along the current ancestor + * path) are replaced. Duplicate references (the same object appearing in + * multiple sibling positions) are preserved as full copies, matching the + * behavior of `JSON.stringify` on acyclic graphs. + * * @param obj - The object to stringify * @param space - Optional space parameter for formatting (defaults to no formatting) * @returns JSON string with circular references replaced by [Circular] @@ -15,16 +20,23 @@ export function safeJsonStringify( obj: unknown, space?: string | number, ): string { - const seen = new WeakSet(); + const ancestors: object[] = []; return JSON.stringify( obj, - (key, value) => { - if (typeof value === 'object' && value !== null) { - if (seen.has(value)) { - return '[Circular]'; - } - seen.add(value); + function (this: unknown, _key, value) { + if (typeof value !== 'object' || value === null) { + return value; + } + // `this` is the parent of `value`. As JSON.stringify's DFS walk unwinds + // back up the tree, pop any ancestors that are no longer on the path + // to `this` so the stack reflects only the current chain of ancestors. + while (ancestors.length > 0 && ancestors[ancestors.length - 1] !== this) { + ancestors.pop(); + } + if (ancestors.includes(value as object)) { + return '[Circular]'; } + ancestors.push(value as object); return value; }, space, From dad73ac5789621c60c7e58498c09e70c6337a9ed Mon Sep 17 00:00:00 2001 From: snf Date: Thu, 21 May 2026 16:56:38 +0300 Subject: [PATCH 2/2] test(core): cover deep unwinding and toJSON paths in safeJsonStringify Four regression tests covering corners the initial five missed: - Shared leaf reached through five levels of nesting plus a sibling branch. Exercises the unwind loop popping multiple frames between the deep arm and the sibling arm of the walk. - Real cycle (root referenced back from depth 5). Same depth as above but the deep arm closes the loop, so the ancestor check must still fire. - Shared object returned by toJSON from two sibling positions. The replacer sees the post-toJSON value, so duplicate-ref handling has to recognize these as duplicates even though the carriers are different objects. - Cycle through a toJSON that returns an ancestor. Confirms the ancestor check fires on the toJSON return value, not the toJSON-bearing carrier. Per review feedback on #4407. --- .../core/src/utils/safeJsonStringify.test.ts | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/packages/core/src/utils/safeJsonStringify.test.ts b/packages/core/src/utils/safeJsonStringify.test.ts index e2ff7527f6d..fade21f9db1 100644 --- a/packages/core/src/utils/safeJsonStringify.test.ts +++ b/packages/core/src/utils/safeJsonStringify.test.ts @@ -129,4 +129,56 @@ describe('safeJsonStringify', () => { '{"a":{"tag":"shared"},"b":{"tag":"shared"},"self":"[Circular]"}', ); }); + + it('should preserve a shared leaf reached through deep ancestor chains', () => { + // Forces the unwind loop to pop five frames between the deep branch and + // the sibling branch. Without the pop, the second occurrence of `shared` + // would still see `shared` on the stack and emit [Circular]. + const shared = { tag: 'shared' }; + const root = { + l1: { l2: { l3: { l4: { l5: { leaf: shared } } } } }, + sibling: { leaf: shared }, + }; + + const result = safeJsonStringify(root); + expect(result).toBe( + '{"l1":{"l2":{"l3":{"l4":{"l5":{"leaf":{"tag":"shared"}}}}}},"sibling":{"leaf":{"tag":"shared"}}}', + ); + expect(result).not.toContain('[Circular]'); + }); + + it('should detect a real cycle through deep ancestor chains', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const root: any = { l1: { l2: { l3: { l4: { l5: { back: null } } } } } }; + root.l1.l2.l3.l4.l5.back = root; + + const result = safeJsonStringify(root); + expect(result).toBe( + '{"l1":{"l2":{"l3":{"l4":{"l5":{"back":"[Circular]"}}}}}}', + ); + }); + + it('should preserve a shared object returned by toJSON from sibling positions', () => { + // JSON.stringify calls toJSON() before invoking the replacer, so the + // replacer sees the post-toJSON value. Two siblings whose toJSON returns + // the same object are duplicate refs, not a cycle. + const shared = { tag: 'shared' }; + const root = { + a: { toJSON: () => shared }, + b: { toJSON: () => shared }, + }; + + const result = safeJsonStringify(root); + expect(result).toBe('{"a":{"tag":"shared"},"b":{"tag":"shared"}}'); + expect(result).not.toContain('[Circular]'); + }); + + it('should detect a cycle when toJSON returns an ancestor', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const root: any = { name: 'root' }; + root.child = { toJSON: () => root }; + + const result = safeJsonStringify(root); + expect(result).toBe('{"name":"root","child":"[Circular]"}'); + }); });