fix(core): preserve duplicate object references in safeJsonStringify - #4407
Conversation
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.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] PR description lists src/telemetry/file-exporters.test.ts as a downstream test file, but this file does not exist in the repo. Additionally, qwen-logger.test.ts:42 and message-bus.test.ts:27 mock safeJsonStringify with native JSON.stringify, so those 2 of 6 downstream files don't exercise the new ancestor-stack implementation. Consider updating the validation section to reflect which files actually test the new code.
— qwen-latest-series-invite-beta-v36 via Qwen Code /review
| // to `this` so the stack reflects only the current chain of ancestors. | ||
| while (ancestors.length > 0 && ancestors[ancestors.length - 1] !== this) { | ||
| ancestors.pop(); | ||
| } |
There was a problem hiding this comment.
[Suggestion] DAG with shared refs can produce exponentially large output. The old WeakSet collapsed all revisited objects to [Circular] (O(n) output). The new algorithm fully expands every duplicate reference — correct per JSON.stringify semantics, but it removes the implicit output-size guard. A DAG with 50 levels where each node has 2 children referencing the same leaf produces ~2⁵⁰ copies of the leaf in the output, blocking the event loop until OOM.
This matters because safeJsonStringify is called on MCP tool params (mcp-tool.ts:452) and tool registry params (tool-registry.ts:48), which originate from external MCP servers (attacker-controlled input).
Consider adding a node-count cap as a safety valve:
let nodeCount = 0;
const MAX_NODES = 10_000;
// in replacer, before ancestors.push:
if (++nodeCount > MAX_NODES) return '[Truncated]';— qwen-latest-series-invite-beta-v36 via Qwen Code /review
There was a problem hiding this comment.
Fair concern, but I think this is the cost of correctness over the old behavior.
Native JSON.stringify already produces the 2^N output on the same DAG shape, so what the helper does now matches what callers would get if they removed the safeJsonStringify wrapper. The old WeakSet doubled as a hidden size guard, but it did so by silently corrupting the output for any acyclic graph with shared refs.
I looked at the actual callers (loggers.ts, file-exporters.ts, qwen-logger.ts, message-bus.ts, mcp-tool.ts, tool-registry.ts). They all stringify request/response payloads, tool args, error messages. Flat-ish JSON from the model or typed message structs. None of them currently fan out into a DAG shape that would blow up.
If size-guarded serialization for arbitrary user input becomes a real concern, I think that's a separate enhancement (a max output size or max duplicate count knob) rather than reintroducing the false [Circular]s here. Can revisit as a follow-up if telemetry ever surfaces a caller that needs it.
|
Both correct. Validation section overstated coverage.
And yes, Files that genuinely cover the change:
143 tests across 4 files actually exercise the new ancestor-stack walk. PR body updated. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No new issues found since prior review. The 3 Suggestions (DAG blowup, deep unwinding test, toJSON test) remain the complete set. LGTM! ✅ — qwen-latest-series-invite-beta-v36 via Qwen Code /review
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 QwenLM#4407.
PR 4407 本地 tmux 验证报告PR: #4407 fix(core): preserve duplicate object references in safeJsonStringify 1. 总体结论
合并建议：✅ 可以合并。小改动、精准修复、测试覆盖充分、无回归。 2. 验证矩阵
3. 根因与修复3.1 问题
这不是循环，只是重复引用（同一对象在多个不相关位置）。 3.2 修复将 // Before: 全局 seen set — 无法区分循环 vs 重复引用
const seen = new WeakSet();
(key, value) => {
if (typeof value === 'object' && value !== null) {
if (seen.has(value)) return '[Circular]';
seen.add(value);
}
return value;
}
// After: ancestor-stack — 只有真循环才标记
const ancestors: object[] = [];
function (this, _key, value) {
if (typeof value !== 'object' || value === null) return value;
// DFS 回溯时弹出已离开的祖先
while (ancestors.length > 0 && ancestors[ancestors.length - 1] !== this) {
ancestors.pop();
}
// 只有值在当前 ancestor chain 上才是真循环
if (ancestors.includes(value as object)) return '[Circular]';
ancestors.push(value as object);
return value;
}利用 4. 测试覆盖4.1 新增 8 个回归测试
4.2 下游调用方（用真实 safeJsonStringify，非 mock）
合计 132 tests, 0 failed。注： 4.3 Built Artifact E2E 行为验证直接 import 5. Behavior Change Impact唯一输出变化：对于包含重复对象引用（非循环）的 graph，输出从
影响面：
6. 代码质量
7. 合并建议✅ 建议合并。 精准的两文件改动（+130/-7），8 个新回归测试覆盖了所有边界（兄弟、数组、深层、toJSON、混合场景），3 个真实下游调用方测试全过，built artifact E2E 验证 7/7。无需任何下游代码修改。 8. 复现指引# 进入 tmux 会话
tmux attach -t pr4407
# 验证环境
cd /Users/wenshao/Work/git/qwen-code-x4
git branch # fix/safe-json-stringify-dup-refs
# 运行测试
cd packages/core
npx vitest run --no-coverage src/utils/safeJsonStringify.test.ts \
src/telemetry/loggers.test.ts \
src/tools/tool-registry.test.ts \
src/tools/mcp-tool.test.ts
# E2E 行为验证
node /tmp/pr4407-verify.mjs
# Lint
npx eslint src/utils/safeJsonStringify.ts src/utils/safeJsonStringify.test.ts报告由 Claude Opus 4.7 在本地 tmux 上完整验证，作为维护者 merge 决策参考。 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
|
Validation matrix matches what I had locally on the helper + the three real-impl downstream files. The built-artifact E2E pass is more than I'd done myself, good to know it holds at the dist layer. The DAG-blowup thread is the only one still open. If the no-size-knob-in-this-PR reasoning reads as right to you, feel free to resolve. Otherwise tell me what shape you'd want and I'll fold it in. |
pomelo-nwu
left a comment
pomelo-nwu
left a comment
There was a problem hiding this comment.
LGTM. Fix safeJsonStringify incorrectly marking duplicate (non-circular) object references as [Circular] — uses ancestor-path tracking instead of global WeakSet.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No new issues found since prior review. The ancestor-stack algorithm is correct and well-tested. LGTM! ✅ — qwen3.7-max via Qwen Code /review
…M#4407) Co-authored-by: Bryan Morgan <bryanmorgan@google.com>
3 participants
Summary
safeJsonStringifynow flags only true cycles. Duplicate references (the same object appearing in multiple unrelated positions in a graph) are preserved as full copies, matching nativeJSON.stringifyon acyclic graphs.WeakSetof every object it had ever seen. SinceJSON.stringifycalls the replacer for every key in a DFS walk and the set was never trimmed when the walk unwound, 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 shared leaves on multiple branches.packages/core/src/utils/safeJsonStringify.ts. The replacer'sthisis the parent ofvalue, so on each call we trim the ancestor stack back to wherever the walk currently is, then check the remaining ancestors for membership. True cycles still produce[Circular]; duplicates serialize normally.Validation
safeJsonStringify.test.ts: 13/13 pass.loggers.test.ts: 45/45 pass.tool-registry.test.ts: 32/32 pass.mcp-tool.test.ts: 53/53 pass.qwen-logger.test.tsandmessage-bus.test.tsbothvi.mock('../utils/safeJsonStringify.js', ...)with nativeJSON.stringify, so they do not exercise this change. Earlier draft listed them in the validation set; corrected here.mainand pass after the change.Scope / Risk
[Circular]as a "we already saw this object" signal would lose that signal. All current callers in the tree are telemetry / logging / error-message surfaces where the underlying data is the goal, not a "we saw it" marker, so impact is positive (more readable telemetry, fewer mystery[Circular]s in MCP error messages, fewer[Circular]s in tool-registrytoString()output for shared param schemas).safeJsonStringifyare unchanged.[Circular]token for true cycles, samespaceparameter behavior.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
No linked issues.