feat(cli): allow overriding the shell program with USAGE_SHELL_<SHELL> - #767
feat(cli): allow overriding the shell program with USAGE_SHELL_<SHELL>#767JamBalaya56562 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughShell execution now supports ChangesShell execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ShellExecution
participant Environment
participant ChildProcess
participant WSLHint
User->>ShellExecution: run script with requested shell
ShellExecution->>Environment: read USAGE_SHELL_* override
Environment-->>ShellExecution: selected executable or default
ShellExecution->>ChildProcess: spawn executable with script arguments
ChildProcess-->>ShellExecution: exit status
ShellExecution->>WSLHint: inspect Bash status and Windows path
WSLHint-->>User: print targeted guidance when matched
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/tests/shell_override.rs`:
- Around line 49-56: Update another_shell_can_stand_in so the shell selected by
USAGE_SHELL_BASH uses a POSIX-compatible fixture without set -o pipefail, or
replace /bin/sh with an interpreter that supports the existing usage_bash
fixture syntax. Preserve the test’s successful output comparison.
In `@docs/cli/scripts.md`:
- Around line 124-126: Correct the override example around USAGE_SHELL_BASH by
marking the fenced block as Command Prompt/batch syntax and using the properly
quoted set form; do not leave it labeled as Bash. If this section explicitly
targets multiple Windows terminals, also provide the corresponding PowerShell
assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bee0899-a406-4c95-abec-29953af61185
📒 Files selected for processing (4)
cli/src/cli/shell.rscli/src/env.rscli/tests/shell_override.rsdocs/cli/scripts.md
dfb4e9e to
53b3f5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cli/scripts.md`:
- Around line 116-119: Update the fenced code block containing the “usage bash
C:/work/mycli” example to include a language identifier such as text or console,
while preserving the example content.
- Around line 146-149: Update the documentation describing the `USAGE_SHELL_*`
value to state that an empty or whitespace-only value is treated the same as an
unset value, while preserving the existing explanation of inheritance and shell
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 18cf24b8-9996-4a03-8ef6-0fcdb98f0221
📒 Files selected for processing (4)
cli/src/cli/shell.rscli/src/env.rscli/tests/shell_override.rsdocs/cli/scripts.md
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/tests/shell_override.rs
- cli/src/env.rs
- cli/src/cli/shell.rs
On Windows, `usage bash C:/work/mycli` fails with
/bin/bash: C:/work/mycli: No such file or directory
Two facts combine. The Win32 executable search order puts the system
directory ahead of PATH, and installing WSL puts `bash.exe` there — so
`Command::new("bash")` picks the WSL launcher on such a machine regardless of
what else is installed. And the WSL bash cannot open a Windows path.
Translating the path is not a fix: the target spelling depends on which shell
was resolved (`/c/...` for msys, `/cygdrive/c/...` for cygwin, `/mnt/c/...`
for WSL, and WSL's mount point is itself configurable via /etc/wsl.conf).
usage has no way to know what `Command::new` resolved to short of
reimplementing the CreateProcess search order, which would be wrong in a
harder-to-diagnose way the moment it diverged from the real one.
So instead: name the shell. `USAGE_SHELL_BASH` (and _ZSH, _FISH, _PWSH)
replaces the program `usage <shell>` runs. The variable is keyed by the
program rather than the subcommand — `usage powershell` runs `pwsh`, so its
variable is USAGE_SHELL_PWSH, which also lets it point at powershell.exe on a
machine without pwsh. mise settled on the same shape for the same reason,
letting its shell settings hold an absolute path.
The value is a program, not a command line: shells on Windows live at paths
like `C:\Program Files\Git\bin\bash.exe`, and `Command` passes the program
and each argument separately, so a path with spaces needs no quoting and
usage takes on no quoting rules of its own.
Unset, blank, or a value that is only whitespace all mean "run the shell as
before", so nothing changes for anyone not setting it.
Two smaller improvements ride along. A shell that cannot be started now
reports which program was tried, and whether a variable pointed there — the
bare io error said only "No such file or directory". And on Windows, when
`bash` exits 127 having been handed a Windows path, usage explains what
probably happened and how to fix it; the guess is hedged and does not change
the exit code, because a script really can exit 127 on its own.
53b3f5f to
b5ebe20
Compare
Problem
On Windows, handing
usage bashan absolute path fails:Two facts combine. The Win32 executable search order is application directory → current directory → system directory → … → PATH, with the system directory ahead of PATH. Installing WSL puts
bash.exethere, soCommand::new("bash")picks the WSL launcher on such a machine no matter what else is installed — confirmed on mine:OSTYPE=linux-gnu,uname -s = Linux, while PowerShell'sGet-Command bashreports Git Bash. And WSL's bash cannot open aC:\…path.Why not translate the path
The target spelling depends on which shell was resolved:
/c/…for msys/Git Bash,/cygdrive/c/…for Cygwin,/mnt/c/…for WSL — and WSL's mount point is itself configurable through/etc/wsl.conf. So even knowing it is WSL is not enough to translate correctly.More fundamentally, usage has no way to learn what
Command::new("bash")resolved to. Finding out would mean reimplementing the CreateProcess search order — App Paths,PATHEXT, safe-search mode — and that becomes a harder-to-diagnose bug the moment it diverges from what the OS actually does.Fix: name the shell
USAGE_SHELL_BASHreplaces the programusage bashruns.usage bashUSAGE_SHELL_BASHusage zshUSAGE_SHELL_ZSHusage fishUSAGE_SHELL_FISHusage powershellUSAGE_SHELL_PWSHThe variable is keyed by the program, not the subcommand.
usage powershellrunspwsh, so its variable is named for that — which also lets it point atpowershell.exeon a machine that only has Windows PowerShell. If this ever extends torun=execution,USAGE_SHELL_SHfalls out of the same rule.mise reached the same shape for the same reason: its
unix_default_*_shell_args/windows_default_*_shell_argsaccept an absolute path rather than trying to work out which shell you have.The value is a program, not a command line. Shells on Windows live at paths like
C:\Program Files\Git\bin\bash.exe, andCommandpasses the program and each argument separately, so a path with spaces needs no quoting — and usage takes on no quoting rules of its own. mise can afford the command-line form because its settings file also accepts arrays; a single environment variable cannot.Unset, empty, or whitespace-only all mean "run the shell as before", matching the
FOO= cmdconvention for switching something off. Nothing changes for anyone not setting it.Nothing checks that the program exists: the value need not be an absolute path, so validating would mean reimplementing
PATH,PATHEXTand permission lookup, then racing the spawn that follows. A bad value surfaces as a spawn error naming it.Two smaller improvements
A shell that cannot be started now says which program was tried, and whether a variable pointed there. The bare io error said only
No such file or directory (os error 2).And on Windows, when
bashexits 127 having been handed a Windows path, usage explains what probably happened:It fires only when all of: Windows, no override set, exit 127,
shell == "bash"(the only one of the four that ships in the system directory), and the script is a drive-letter or UNC path. It is hedged, prints after the child's own stderr, and does not touch the exit code — a script really can exit 127 on its own.Verified
Windows 11 with WSL installed. Before, with an absolute path: exit 127 plus the hint above. After pointing the variable at Git Bash, the same command works:
Five integration tests (
cli/tests/shell_override.rs,#[cfg(unix)]) cover the override actually replacing the program (USAGE_SHELL_BASH=/bin/echoprints the argv instead of running the script), a blank value falling back, a name resolved throughPATHworking like an absolute one, an unstartable program naming itself and the variable, and another shell's variable being ignored. Unit tests cover variable-name derivation, trimming, blank handling, and the hint's conditions and wording — all pure, so they run on the Linux CI.Those integration tests are Unix-only and I develop on Windows, so they are also run on Linux before pushing, not just type-checked and linted for
x86_64-unknown-linux-gnu.cargo test -p usage-lib --all-features,cargo clippy --all --all-features -- -D warningsandcargo fmt --all -- --checkpass. Help text is untouched, so no generated artifact changes.Deliberately not doing
bashourselves ahead of the system directory. It would change the resolution order for every user, away from what the OS does, and needs a new dependency. Worth discussing separately.usage exec. Its interpreter is already named by the caller, so a shebang can point straight at one:#!/usr/bin/env -S usage exec "C:/msys64/usr/bin/bash.exe". Generalising the variable to arbitrary program names would also mean a third party could redirect the interpreter a script author chose. Documented as the escape hatch instead.test.ymlisubuntu-latestonly, so the Windows path here rests on manual verification. That belongs to a separate CI change.This pull request was generated by Claude Code.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation