fix(task): honor explicit and quoted shell paths on Windows (#9932)#10148
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves shell command parsing and resolution, particularly on Windows, by introducing a host-aware split_shell_command function. This ensures that explicit shell paths with backslashes or spaces (when double-quoted) are parsed correctly without being mangled, and that malformed shell configurations fail loudly. Additionally, on Windows, explicit paths are now honored verbatim instead of being over-resolved. Feedback on the changes suggests a more idiomatic, allocation-free implementation for checking if a program path contains directory components using Path::components().count() > 1.
Greptile SummaryThis PR fixes the regression from #9750 where an explicitly configured shell path on Windows (e.g.
Confidence Score: 5/5Safe to merge; the changes are tightly scoped to Windows shell resolution and the shell-string parsing layer, with no logic paths altered on non-Windows builds beyond substituting The core fix ( No files require special attention; the most complex new code ( Important Files Changed
Reviews (3): Last reviewed commit: "refactor(task): use Path::components for..." | Re-trigger Greptile |
jdx#10148) Per Gemini review, replace the lossy-string separator check with Path::new(program).components().count() > 1 — allocation-free and idiomatic, with the same behavior for all realistic shell paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On Windows an explicitly configured bash path (e.g. a task shell of C:/msys64/usr/bin/bash.exe -c, or windows_default_inline_shell_args) was ignored and silently re-resolved to a different bash such as Git Bash, via the WSL-avoidance logic added in jdx#9750. Honor an explicit path (absolute, or relative with a directory component) verbatim instead of re-resolving it. Also unify how configured shell strings are parsed. A new split_shell_command treats backslashes as literal path characters and only groups double-quoted spans on Windows (Unix keeps shell_words / POSIX semantics), so shell paths with spaces (when quoted) or backslashes survive instead of being mangled. It is used for task shell, hook and watch_files shells, and the *_default_*_shell_args settings. Task::shell() now returns a Result so a malformed explicit shell (e.g. an unbalanced quote) fails loudly instead of silently falling back to the default shell. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jdx#10148) Per Gemini review, replace the lossy-string separator check with Path::new(program).components().count() > 1 — allocation-free and idiomatic, with the same behavior for all realistic shell paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
79c3481 to
aae6090
Compare
Problem
Fixes the regression reported in #9932. On Windows, an explicitly configured bash path — e.g. a task
shell = "C:/msys64/usr/bin/bash.exe -c",windows_default_inline_shell_args, or a{{ env.X }}-interpolated absolute path — was ignored and silently re-resolved to a different bash (typically Git Bash) by the WSL-avoidance logic added in #9750. Trace logs showedusing shell: C:/.../bash.exebut execution underC:\Program Files\Git\bin\bash.exe. This broke setups that worked before 26.5.8.Fix
Honor explicit paths.
resolve_posix_shell_program_pathnow returns early (keeps the program verbatim) when the program carries a directory component, so only a barebash/bash.exeflows into the candidate-list / PATH WSL-avoidance resolution. An explicitly chosen binary is used as-is.Parse shell strings consistently. A new
split_shell_command(src/path.rs) parses every configured shell string: on Windows it treats backslashes as literal path characters and only groups double-quoted spans (\"\"-> literal\"); on Unix it keepsshell_words/POSIX semantics. A shell path with spaces (when double-quoted) or with backslashes now survives instead of being mangled. Applied to taskshell, hook and[[watch_files]]shells, and the*_default_*_shell_argssettings.Fail loudly on malformed shells.
Task::shell()now returnsResult<Option<Vec<String>>>, so a malformed explicit shell (e.g. an unbalanced quote) errors instead of silently falling back to the default shell.Notes
PATHto MSYS/c/...form, which Cygwin's/cygdrive/c/...expectation does not match. Honoring the explicit Cygwin binary is an improvement, but full CygwinPATHsupport is out of scope here.docs/troubleshooting.mdupdated with explicit-path and spaces-in-path guidance.Testing
split_shell_command,program_has_directory_component, explicit-path resolution, and the loud-fail behavior.x86_64-pc-windows-msvc: targeted + affected-module regression tests pass;clippyclean for the new code;rustfmtapplied.🤖 Generated with Claude Code