fix(cli): skip redundant settings writes and preserve trailing newlines#26282
fix(cli): skip redundant settings writes and preserve trailing newlines#26282cocosheng-g wants to merge 1 commit intomainfrom
Conversation
This change prevents settings.json from being mangled when logical content hasn't changed, preserving user formatting like single-line arrays and trailing newlines. Fixes #18934
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 optimizes the CLI settings management by preventing unnecessary file system operations when settings are already up to date. It also improves the robustness of the settings update process by ensuring that existing file formatting, specifically trailing newlines, is preserved during the write process. 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 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 counter productive. 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
|
|
Size Change: +1.24 kB (0%) Total Size: 33.9 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Code Review
This pull request introduces logic to skip unnecessary file writes in updateSettingsFilePreservingFormat by performing a deep equality check on the logical content. It also improves trailing newline handling for settings files and adds comprehensive tests for these scenarios. A security concern was raised regarding potential prototype pollution in the update application logic, along with a suggestion to use a more robust method for stripping metadata during content comparison.
| } | ||
|
|
||
| // First, check if logical content would change using the clean parse | ||
| const updatedClean = applyUpdates(structuredClone(cleanParsed), updates); |
There was a problem hiding this comment.
The call to applyUpdates on line 52 introduces a Prototype Pollution vulnerability. The helper function applyKeyDiff (used by applyUpdates) does not validate or skip sensitive keys like __proto__, constructor, or prototype. This new call path expands the attack surface, potentially leading to Denial of Service (DoS) or Remote Code Execution (RCE) if Object.prototype is modified. To remediate this, applyKeyDiff (line 170) should be updated to explicitly skip these sensitive keys.
Additionally, the current implementation uses JSON.parse(stripJsonComments(...)) for content comparison, which is stricter than comment-json and could cause errors with valid (but lenient) JSON (e.g., trailing commas). For more robust comparison, consider using structuredClone on the comment-json parsed object, as it naturally strips Symbols, providing a clean object without parsing errors.
- google-gemini/gemini-cli#26284 merge-after-nits: voice-mode wave animation, must remove duplicate ternary arm before merge - google-gemini/gemini-cli#26282 merge-after-nits (CLOSED, design-of-record): clean-parse equality + trailing-newline preservation for settings.json - docs: append drip-236 block to reviews/INDEX.md
Summary
Prevent unnecessary rewrites of
settings.jsonand preserve formatting (like single-line arrays and trailing newlines) even when internal migrations occur.Details
deepEqualutility to compare JSON structures ignoringcomment-jsonmetadata (Symbol keys).updateSettingsFilePreservingFormatto skipfs.writeFileSyncif logical content is unchanged.commentJson.test.tsfor formatting preservation, newline handling, and redundant write avoidance.Related Issues
Fixes #18934
How to Validate
.gemini/settings.jsonwith a single-line array and trailing newline:printf '{\n "context": {\n "fileName": ["A.md", "B.md"]\n }\n}\n' > .gemini/settings.jsongemini --help(or any command that loads settings).cat .gemini/settings.json.npm test packages/cli/src/utils/commentJson.test.ts.Pre-Merge Checklist