fix: editing plan without changes still triggers agent to replan#24123
fix: editing plan without changes still triggers agent to replan#24123S-Spektrum-M wants to merge 6 commits intogoogle-gemini:mainfrom
Conversation
In plan mode, if a user opens a plan in an external editor via Ctrl+X and exits without making any modifications to the plan file, the CLI would previously trigger an unnecessary replan cycle. This updates the logic to hash the file before and after the editor is opened and only trigger a replan if the file was actually modified. issue: google-gemini#24122
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 improves the user experience in Plan Mode by ensuring that the system only triggers a replan cycle if the plan file has been explicitly modified by the user. By comparing file hashes before and after an external editor session, the CLI avoids redundant processing, streamlining the workflow for users who open the editor but decide not to make changes. 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
|
|
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. |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to only trigger feedback submission when the plan file is actually modified in the editor, using SHA-256 hashing to detect changes. Corresponding unit tests have been added to verify this behavior. Feedback was provided regarding a potential command injection vulnerability on Windows due to unsanitized file paths and an improvement to error handling in the hashing logic to prevent masking file system issues.
When launching an external editor on Windows, the CLI uses 'shell: true' to ensure '.cmd' files (like VS Code's 'code.cmd') can be spawned correctly. This commit wraps the argument to spawn/spawnSync in double quotes and escapes internal double quotes before passing them to the shell on Windows, which prevents command injection.
Updates the plan file hash calculation to only swallow ENOENT (file not found) errors. Other file system errors (like permission issues) will now be properly thrown and caught by the outer error handler, rather than being swallowed as 'null' and potentially causing false negatives when comparing file hashes.
|
Edit: unhid step 3 in validation |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies the Exit Plan Mode to only submit feedback if the plan file has been changed, using SHA-256 hashing for verification. It also adds logic to prevent command injection on Windows when opening the editor. Review feedback highlights that the manual quoting implementation on Windows is incorrect and could lead to command injection or execution errors. It is recommended to rely on Node.js's native argument handling or properly configure the windowsVerbatimArguments option.
| const safeArgs = | ||
| process.platform === 'win32' | ||
| ? [...initialArgs, ...args].map((arg) => `"${arg.replace(/"/g, '')}"`) | ||
| : [...initialArgs, ...args]; |
There was a problem hiding this comment.
A command injection vulnerability exists due to the manual quoting of arguments on Windows, especially when combined with shell: true. Node.js's spawn implementation automatically quotes arguments containing spaces when shell: true is used on Windows. Manually adding double quotes (lines 86-89) interferes with this, leading to double-quoting (e.g., ""C:\Path With Spaces\file.md"") which cmd.exe cannot parse correctly, and allows special characters like & to be interpreted as command separators. Furthermore, the executable path itself is not quoted (lines 96 and 121), which can also lead to command injection if it contains spaces. Unconditionally quoting all arguments (like "-u" for vim) may also break some editors. To fix this, remove the manual quoting and rely on Node.js's built-in argument handling. Alternatively, if shell: true is required and you need precise control over arguments, use the windowsVerbatimArguments: true option in spawn and spawnSync calls. Additionally, ensure the executable path is properly quoted.
| const result = spawnSync(executable, safeArgs, { | ||
| stdio: 'inherit', | ||
| shell: process.platform === 'win32', | ||
| }); |
There was a problem hiding this comment.
When using manual quoting for Windows arguments to prevent injection, you must set windowsVerbatimArguments: true to prevent Node.js from double-quoting the arguments.
const result = spawnSync(executable, safeArgs, {
stdio: 'inherit',
shell: process.platform === 'win32',
windowsVerbatimArguments: process.platform === 'win32',
});| const child = spawn(executable, safeArgs, { | ||
| stdio: 'inherit', | ||
| shell: process.platform === 'win32', | ||
| }); |
There was a problem hiding this comment.
When using manual quoting for Windows arguments to prevent injection, you must set windowsVerbatimArguments: true to prevent Node.js from double-quoting the arguments.
| const child = spawn(executable, safeArgs, { | |
| stdio: 'inherit', | |
| shell: process.platform === 'win32', | |
| }); | |
| const child = spawn(executable, safeArgs, { | |
| stdio: 'inherit', | |
| shell: process.platform === 'win32', | |
| windowsVerbatimArguments: process.platform === 'win32', | |
| }); |
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we have updated our contribution policy (see Discussion #17383). We only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. All other community pull requests are subject to closure after 14 days if they do not align with our current focus areas. For this reason, we strongly recommend that contributors only submit pull requests against issues explicitly labeled as 'help-wanted'. This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding and for being part of our community! |
In plan mode, if a user opens a plan in an external editor via Ctrl+X
and exits without making any modifications to the plan file, the CLI
would previously trigger an unnecessary replan cycle.
This updates the logic to hash the file before and after the editor is
opened and only trigger a replan if the file was actually modified.
fixes issue: #24122
Summary
Optimizes the Ctrl+X workflow in Plan Mode by skipping replanning if the plan
file remains unchanged after the system editor closes.
Details
Related Issues
#24122
How to Validate
Pre-Merge Checklist