Repository navigation
Conversation
safeJsonStringify used a global 'seen' WeakSet, so any shared (non-circular) object reference was replaced with [Circular]. OpenTelemetry records share endTime objects and histogram bound arrays, which corrupted exported telemetry files. Track only the current depth-first ancestor path via the replacer's 'this' (the holder of the current key) and unwind completed subtrees. True cycles still produce [Circular]; shared references are now serialized at each occurrence. Fixes google-gemini#29406
|
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. |
|
📊 PR Size: size/M
|
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 an issue where the safeJsonStringify utility incorrectly flagged shared object references as circular. By switching from a global tracking mechanism to a path-based stack, the utility now correctly distinguishes between shared references (diamond dependencies) and true circular structures, ensuring accurate JSON serialization for complex objects like OpenTelemetry metrics. 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
|
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the safeJsonStringify utility to use an ancestor stack instead of a global WeakSet for circular reference detection. This allows shared, non-circular references to be serialized correctly instead of being incorrectly flagged as [Circular], while still properly detecting true circular references. Unit tests have been added to verify these scenarios. I have no feedback to provide as the implementation is correct and well-tested.
|
@google-cla-bot recheck |
1 similar comment
|
@google-cla-bot recheck |
gemini-cli
Bot
added
priority/p2
This branch has not been deployed
1 participant
Problem
safeJsonStringifyused a single global "seen" WeakSet to detect circular references. An object referenced more than once (but not part of a cycle) was wrongly replaced with[Circular]— e.g. the OTel metricsendTimeobject shared between data points and histogram boundaries reused across buckets. Reported in #29406.Root cause
A reference appearing twice on different DFS branches is not circular, but a global visited-set cannot distinguish "already visited elsewhere" from "on the current path".
Fix
Track only the current DFS path: the replacer adds each object key via
thisbefore serializing its subtree and removes it after the subtree completes. True cycles still contain the reference on the active path and are still rendered as[Circular]; shared (diamond) references are serialized normally on every occurrence.Tests
packages/core/src/utils/safeJsonStringify.test.ts: shared references (endTime-style, histogram-boundary-style) are preserved; true cycles still produce[Circular]; nesting/escaping behavior unchanged.npx vitest run src/utils/safeJsonStringify.test.ts→ 11/11 pass.mcp-tool,recordingContentGenerator) suites: 69/69 pass; eslint + prettier clean.Fixes #29406