fix(tui): resolve Windows shell commands via PowerShell#2279
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hmbown has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors platform shell resolution by introducing a new shell_invocation module. It extracts and unifies the logic for constructing shell commands on Windows and Unix platforms, adding support for detecting and utilizing pwsh.exe, powershell.exe, and POSIX-like shells on Windows based on environment variables and path probes. The reviewer provided valuable feedback regarding a compatibility issue when falling back to Windows PowerShell 5.1 (powershell.exe) due to its lack of support for pipeline chain operators (&&/||), recommending falling back to cmd.exe instead. Additionally, the reviewer pointed out a quote-preservation bug when cmd.exe is resolved via its full path, suggesting the reuse of shell_program_stem to robustly identify the shell program.
| if probe.pwsh_on_path { | ||
| return powershell_invocation("pwsh.exe", command); | ||
| } | ||
|
|
||
| if probe.powershell_on_path { | ||
| return powershell_invocation("powershell.exe", command); | ||
| } |
There was a problem hiding this comment.
High Severity: Incompatible Default Fallback to Windows PowerShell 5.1 (powershell.exe)
Automatically falling back to powershell.exe when pwsh.exe (PowerShell Core 7+) is not found introduces major compatibility issues on default Windows installations:
-
Lack of Pipeline Chain Operators (
&&/||):
Windows PowerShell 5.1 (powershell.exe) does not support&&and||operators (which were only introduced in PowerShell 7.0). Any LLM-generated or tool-generated multi-command sequence (e.g.,cd dir && cargo build) will fail with a syntax error:
The token '&&' is not a valid statement separator in this version. -
Variable Expansion and Quoting Quirks:
PowerShell parses-Commandarguments as PowerShell code, meaning environment variable references (like$VARor%VAR%) and double quotes are parsed differently and often stripped or mangled before being passed to external executables.
Recommendation:
If pwsh.exe is not available on the PATH, it is much safer to fall back to cmd.exe (which is highly compatible with standard command syntax and handles &&/|| correctly). If a user explicitly wants to use powershell.exe, they can still do so by setting the SHELL environment variable, which is already handled by probe.shell at the beginning of this function.
| if probe.pwsh_on_path { | |
| return powershell_invocation("pwsh.exe", command); | |
| } | |
| if probe.powershell_on_path { | |
| return powershell_invocation("powershell.exe", command); | |
| } | |
| if probe.pwsh_on_path { | |
| return powershell_invocation("pwsh.exe", command); | |
| } |
| } | ||
|
|
||
| fn push_eval_shell_args(cmd: &mut Command, invocation: &EvalShellInvocation) { | ||
| fn push_eval_shell_args(cmd: &mut Command, invocation: &ShellInvocation) { |
There was a problem hiding this comment.
High Severity: Quote-preservation bug when cmd.exe is resolved via full path
In push_eval_shell_args (line 40), the code checks invocation.program.eq_ignore_ascii_case("cmd") to decide whether to use cmd.raw_arg for quote preservation.
However, if cmd.exe is resolved via its full path (e.g., C:\Windows\System32\cmd.exe from COMSPEC), eq_ignore_ascii_case("cmd") will return false. This bypasses the raw_arg quote-preservation logic, causing quotes in command payloads (like commit messages) to be mangled by MSVCRT escaping on Windows.
Since we made shell_program_stem pub(crate), we should use it here to robustly check if the program stem is "cmd".
| fn shell_program_stem(program: &str) -> Option<String> { | ||
| let normalized = program.trim().replace('\\', "/"); | ||
| let filename = normalized.rsplit('/').next()?.trim(); | ||
| let stem = filename | ||
| .strip_suffix(".exe") | ||
| .or_else(|| filename.strip_suffix(".EXE")) | ||
| .unwrap_or(filename); | ||
| if stem.is_empty() { | ||
| None | ||
| } else { | ||
| Some(stem.to_ascii_lowercase()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Medium Severity: Simplify shell_program_stem and make it pub(crate)
We can simplify the case-insensitive .exe suffix stripping by converting the filename to lowercase first. This avoids chained .strip_suffix calls and handles mixed-case extensions (e.g., .Exe).
Additionally, making this function pub(crate) allows other modules (such as eval.rs in push_eval_shell_args) to reuse it to robustly check if the program is cmd.exe or cmd regardless of its full path.
pub(crate) fn shell_program_stem(program: &str) -> Option<String> {
let normalized = program.trim().replace('\\', "/");
let filename = normalized.rsplit('/').next()?.trim().to_ascii_lowercase();
let stem = filename
.strip_suffix(".exe")
.unwrap_or(&filename);
if stem.is_empty() {
None
} else {
Some(stem.to_string())
}
}| args: Vec<String>, | ||
| raw_payload_on_windows: bool, | ||
| } | ||
| use crate::shell_invocation::{ShellInvocation, shell_invocation}; |
There was a problem hiding this comment.
Medium Severity: Import shell_program_stem to robustly check for cmd
Import shell_program_stem so we can use it in push_eval_shell_args to robustly identify cmd.exe even when it is specified via a full path (e.g., from COMSPEC).
| use crate::shell_invocation::{ShellInvocation, shell_invocation}; | |
| use crate::shell_invocation::{ShellInvocation, shell_invocation, shell_program_stem}; |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } | ||
|
|
||
| fn push_eval_shell_args(cmd: &mut Command, invocation: &EvalShellInvocation) { | ||
| fn push_eval_shell_args(cmd: &mut Command, invocation: &ShellInvocation) { |
There was a problem hiding this comment.
🔴 push_eval_shell_args fails to use raw_arg when cmd.exe program is a full path
The push_eval_shell_args function at crates/tui/src/eval.rs:40 checks invocation.program.eq_ignore_ascii_case("cmd") to decide whether to use raw_arg for cmd.exe. Before this PR, the old EvalShellInvocation always used program: "cmd", so this check always passed. After the refactoring to use the shared ShellInvocation, the program field can now be a full path like C:\Windows\System32\cmd.exe (from COMSPEC) or cmd.exe, causing the check to fail. When it fails, the function falls through to cmd.args(...) instead of cmd.raw_arg(...), which breaks quoting for commands with embedded quotes on Windows — the exact regression that issue #1691 fixed.
The correct approach is already demonstrated in crates/tui/src/tools/shell.rs:198-202, which extracts the file stem from the program path using Path::file_stem() before comparing.
Prompt for agents
The function push_eval_shell_args in crates/tui/src/eval.rs uses invocation.program.eq_ignore_ascii_case("cmd") on line 40 to detect cmd.exe and route through raw_arg. This worked when program was always "cmd" (old EvalShellInvocation), but now that ShellInvocation is used, program can be a full path like C:\Windows\System32\cmd.exe.
The fix should match the approach used in crates/tui/src/tools/shell.rs push_shell_args (lines 198-202), which uses std::path::Path::new(program).file_stem() to extract just the stem before comparing. Replace the eq_ignore_ascii_case("cmd") check on line 40 with a file_stem-based check, e.g.:
let is_cmd = std::path::Path::new(&invocation.program)
.file_stem()
.and_then(|s| s.to_str())
.map(|s| s.eq_ignore_ascii_case("cmd"))
.unwrap_or(false);
if invocation.raw_payload_on_windows
&& is_cmd
&& invocation.args.len() == 2
&& invocation.args[0].eq_ignore_ascii_case("/C")
This is important because COMSPEC is almost always set to C:\Windows\System32\cmd.exe on Windows, so the current code would fail to use raw_arg in the vast majority of cases.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Independent review (Opus, post-greptile/gemini, simulated merge in
|
|
I cannot make direct changes to this branch from my current environment, so I pushed the remaining fixes to a follow-up branch and opened a separate draft PR: #2284. That follow-up includes the shell stem cleanup in the prompt environment block, Paulo Aboim Pinto |
1 similar comment
|
I cannot make direct changes to this branch from my current environment, so I pushed the remaining fixes to a follow-up branch and opened a separate draft PR: #2284. That follow-up includes the shell stem cleanup in the prompt environment block, Paulo Aboim Pinto |
Summary
SHELLfirst, then preferpwsh.exe/powershell.exe, withcmd /Ckept as the UTF-8 fallbackCommandSpec::shell, the offline eval shell step, and the prompt environment shell label through the resolvercmdraw-argument fallback covered so quoted command payloads still survive when PowerShell is unavailableFixes #1779.
Refs #1781.
Credit
Reported by @aboimpinto in #1779, with the broader ShellDispatcher v2 exploration in #1781. This PR intentionally harvests the narrow shell-resolution piece without taking the raw-mode/logging parts from that stale branch.
Testing
CARGO_TARGET_DIR=/Volumes/VIXinSSD/whalebro/codewhale/target cargo test -p codewhale-tui shell_invocation -- --nocaptureCARGO_TARGET_DIR=/Volumes/VIXinSSD/whalebro/codewhale/target cargo test -p codewhale-tui test_command_spec_shell -- --nocaptureRUSTFLAGS=-Dwarnings CARGO_TARGET_DIR=/Volumes/VIXinSSD/whalebro/codewhale/target cargo check -p codewhale-tui --all-features --lockedcargo fmt --all -- --checkgit diff --check