fix(core): preserve trailing newlines in shell execution#23705
fix(core): preserve trailing newlines in shell execution#23705Aaxhirrr wants to merge 2 commits intogoogle-gemini:mainfrom
Conversation
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 a regression in the 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces changes to preserve trailing newlines in shell commands, particularly for heredocs and comment-only commands. The stripShellWrapper utility now accepts an option to retain trailing whitespace, and the ShellToolInvocation logic has been updated to correctly wrap commands that end with newlines. New unit and integration tests have been added to validate this behavior. A security concern was identified where the tempFilePath variable is directly interpolated into a shell command, posing a potential command injection vulnerability if the path contains special characters.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces changes to correctly preserve trailing newlines in shell commands, particularly for heredocs and comment-only commands. It modifies the stripShellWrapper utility to include an option for preserving trailing whitespace and updates the ShellToolInvocation to use this option and adjust command wrapping logic accordingly. Additionally, it incorporates escapeShellArg for temporary file paths in wrapped commands for improved robustness. New unit and integration tests have been added to validate these changes. I have no feedback to provide on the review comments as there were none.
Summary
Fixes a regression in
run_shell_commandwhere trailing newlines were stripped before execution, which breaks heredocs and other multiline shell constructs that require a final newline delimiter.Details
This change keeps the fix narrowly scoped to the execution path.
pgrepwrapper so newline-terminated commands close cleanly instead of being trimmed and forced onto a same-line;.No documentation changes were needed.
Related Issues
Fixes #20755
How to Validate
Run the core shell regressions:
npm run test --workspace @google/gemini-cli-core -- shell.test.ts shell-utils.test.tsExpected result:
shell.test.tspassesshell-utils.test.tspassesIn a bash environment, run the targeted integration regression:
npm run test:integration:sandbox:none -- run_shell_command.test.ts -t "should preserve trailing newlines for heredoc shell commands"Expected result:
run_shell_commandargs still end with\nTRAILING_NEWLINE_20755here-document delimited by end-of-fileorsyntax error: unexpected end of fileManual sanity check in a bash environment:
Execute a shell tool call with:
Expected result:
testOptional extra edge case:
Execute a comment-only command ending in a newline:
# commentExpected result:
Note:
npm run.Pre-Merge Checklist