-
Notifications
You must be signed in to change notification settings - Fork 9.9k
feat: detect and use parent shell on Windows #8429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: detect and use parent shell on Windows #8429
Conversation
Allow Windows users to use their preferred shell (Git Bash, PowerShell, MSYS2) instead of forcing cmd.exe. Detect parent shell via environment variables and use consistently across execution methods. - Add priority-based shell detection via SHELL, PSModulePath, ComSpec, LOGINSHELL - Update ShellExecutionService to use detected shell configuration - Remove redundant platform-specific switches in execution paths - Add comprehensive tests for all detection scenarios - Update documentation with new shell detection behavior - Maintain backward compatibility with fallback to cmd.exe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @0dminnimda, 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 significantly improves the user experience for Windows users of the Gemini CLI by enabling it to detect and utilize their preferred shell environment instead of being confined to cmd.exe. This change addresses long-standing issues related to limited shell capabilities, inconsistent user experience, and AI confusion, by providing a more robust and user-centric approach to command execution.
Highlights
- Enhanced Shell Detection on Windows: The Gemini CLI now intelligently detects and uses the user's preferred shell (e.g., Git Bash, PowerShell, MSYS2) on Windows, moving away from the previous default of
cmd.exe. - Priority-Based Detection Logic: A new detection logic has been implemented for Windows, prioritizing environment variables such as
SHELL,LOGINSHELL,PSModulePath, andComSpecto determine the active shell, withcmd.exeas a robust fallback. - Consistent Command Execution: The
ShellExecutionServicehas been updated to consistently use the detected shell configuration for both direct command execution (cpSpawn) and pseudo-terminal (PTY) processes, simplifying platform checks. - Comprehensive Testing and Documentation: Extensive new test cases have been added to validate all shell detection scenarios, and the
docs/tools/shell.mddocumentation has been updated to reflect the new shell detection behavior and priority order.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a great feature to detect and use the parent shell on Windows, which significantly improves the user experience for those who prefer shells like Git Bash or PowerShell over cmd.exe. The implementation is well-structured, with the core logic encapsulated in detectParentShell and comprehensive tests to cover various scenarios. I've identified a few areas for improvement to enhance code clarity and correctness. Specifically, there are opportunities to refactor some duplicated logic in the shell detection function and a critical fix is needed in how shell arguments are constructed to ensure consistent behavior.
- Remove special cmd handling to preserve /d and /s flags - Simplify redundant conditional in shell detection - Consolidate duplicated PowerShell detection logic - Update documentation to reflect consolidated logic Addresses review feedback from gemini-code-assist bot
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant improvement for Windows users by detecting and using their preferred shell, moving away from the cmd.exe default. The implementation is clean, with a new detectParentShell function that uses a clear, priority-based logic. The changes are well-supported by documentation updates and a comprehensive suite of new tests. I've identified one potential high-severity issue regarding the handling of the LOGINSHELL environment variable, which could lead to incorrect shell executable resolution. My review includes a specific code suggestion to address this. Overall, this is a valuable contribution that greatly enhances the tool's usability on Windows.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@gemini-code-assist review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable feature for Windows users by detecting and using their preferred shell instead of always defaulting to cmd.exe. The implementation introduces a detectParentShell function that intelligently checks environment variables like SHELL, LOGINSHELL, PSModulePath, and ComSpec to determine the correct shell. The changes are well-structured, with corresponding updates to documentation and comprehensive new tests.
My review identifies one area for improvement in the shell detection logic to make it more robust.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@gemini-code-assist review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great step forward for improving the experience for Windows users by detecting their preferred shell. The implementation is well-structured and the addition of comprehensive tests is commendable. However, I've identified a significant issue in the shell detection logic on Windows that could lead to incorrect shell detection for cmd.exe users, along with a minor regression in the fallback mechanism. My review includes a detailed comment and a code suggestion to address these points, making the detection more robust and reliable.
- Consolidate Windows/Unix logic into unified detection order - Support PowerShell Core (pwsh) on all platforms - Use descriptive variable names instead of comments - Add comprehensive cross-platform tests - Update documentation to reflect new behavior Detection order: 1. SHELL environment variable (all platforms) 2. LOGINSHELL environment variable (all platforms) 3. PowerShell via ComSpec (Windows only) 4. PowerShell via process.title (Windows only) 5. Platform defaults: bash on Unix, cmd.exe on Windows
|
@gemini-code-assist review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a great feature to detect and use the parent shell on Windows, improving the experience for users of PowerShell, Git Bash, and other shells. The implementation is solid and well-tested. I've found a couple of potential issues in the detection logic that could lead to incorrect shell identification in some edge cases. My review includes suggestions to make the detection more robust.
- Remove unnecessary comments that just explain what code does - Move process.title explanation right next to where it's used - Use helper functions consistently throughout - Keep only comments that explain the 'why', not the 'what' - Simplify function documentation
Address code review feedback: The previous logic using shellName.includes('cmd')
was too broad and could misclassify paths like '/home/user/commands/my-script.sh'
as cmd shell, leading to incorrect shell arguments.
- Use precise regex /(^|\|\/)cmd(\.exe)?$/ to only match actual cmd executables
- Add comprehensive tests for edge cases to prevent regressions
- Verify paths containing 'cmd' substring are correctly classified as bash
- Ensure actual cmd.exe paths still work correctly
Fixes potential command execution failures from incorrect shell classification.
Address code review feedback: The previous logic only detected 'powershell' in process.title and hardcoded 'powershell.exe', missing PowerShell Core (pwsh). - Detect both 'pwsh' and 'powershell' in process.title - Use appropriate executable: 'pwsh.exe' for PowerShell Core, 'powershell.exe' for Windows PowerShell - Prioritize pwsh when both strings are present in title - Add comprehensive tests for PowerShell Core detection scenarios - Update comment to reflect both PowerShell variants Fixes PowerShell Core falling back to cmd.exe incorrectly.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant improvement for Windows users by detecting and using their preferred shell instead of defaulting to cmd.exe. The implementation adds new shell detection logic, updates the shell execution service, and includes a comprehensive set of tests. The overall approach is solid. I've identified a couple of areas in the detection logic where the implementation could be made more robust to prevent incorrect shell identification, which could lead to command execution failures. My feedback focuses on making these checks more specific.
- Use precise regex patterns for PowerShell detection in shell paths - Replace broad includes() checks with word boundary detection - Prevent misclassification of paths containing 'cmd', 'powershell', or 'pwsh' - Improve process.title detection for Windows PowerShell vs cmd.exe - Add comprehensive tests for edge cases and false positive prevention Fixes command execution failures caused by incorrect shell type detection.
|
This PR has been obsoleted by #11157 which adds similar functionality. |
Closes #2353
Closes #3126
Closes #6413
Enable Windows users to use their preferred shell (Git Bash, PowerShell, MSYS2) instead of being forced to use cmd.exe. Gemini CLI now detects the shell that launched it and uses the same shell for command execution.
Problem
Previously, Gemini CLI would always use cmd.exe on Windows regardless of what shell the user actually preferred. This caused several issues:
spawn({ shell: true })defaults to cmd.exe even when better shells are available ((shell) - Adopt Explicit Shell Invocation and Prioritize PowerShell on Windows #6413)Solution
SHELL,PSModulePath,ComSpec,LOGINSHELL) to detect the parent shellDetection Logic
Windows Priority Order:
PSModulePathenvironment variable existsComSpecpoints to powershell/pwshUnix-like systems: Uses
SHELLenvironment variable or defaults to bashChanges Made
detectParentShell()inshell-utils.tsto intelligently detect the parent shellgetShellConfiguration()to use parent shell detection as primary methodShellExecutionServiceby removing redundant platform checksBenefits
Related Issues
This change may also help with issues related to shell command execution and path handling like #4104, #8267, and #2488 by providing better shell environment detection, though those may require additional specific fixes.