fix(cli): forward parsed args to WSL bash via WSLENV on windows - #764
fix(cli): forward parsed args to WSL bash via WSLENV on windows#764JamBalaya56562 wants to merge 1 commit into
Conversation
On Windows with WSL installed, `usage bash script.sh myws --region eu-1`
runs the script with every `usage_*` variable unset. No error, no warning —
the script just sees empty values.
Two independent facts combine:
- The Win32 executable search order puts the system directory ahead of PATH,
so `Command::new("bash")` resolves to `C:\Windows\System32\bash.exe`, the
WSL launcher, regardless of what else is on PATH. (`OSTYPE=linux-gnu`,
`uname -s = Linux`, while PowerShell's `Get-Command bash` reports Git Bash —
which is why looking at PATH does not reveal this.)
- WSL only carries a Win32 variable across the boundary if `WSLENV` names it.
So the `cmd.env(...)` calls in `shell.rs` and `exec.rs` are a no-op there.
Both call sites now go through `env::apply_parsed_env`, which additionally
adds the variable names to `WSLENV` on Windows. Names are added bare: usage
does not know whether a value is a path, and `/p` would silently rewrite
anything that merely looks like one, so values cross verbatim as they do on
Unix. Entries already in `WSLENV` are preserved untouched, flags included.
Which bash gets resolved is deliberately left alone. Changing it would break
scripts already relying on WSL bash, and there is no single right answer —
mise, for comparison, does not guess either: it defaults to `cmd /c` on
Windows and lets the shell be configured explicitly.
`cfg!(windows)` rather than `#[cfg(windows)]` because CI only runs on Linux,
where a `#[cfg]` block would never be compiled, type-checked or linted.
|
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 (3)
📝 WalkthroughWalkthroughThe change centralizes parsed environment application for ChangesParsed environment propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ExecOrShell
participant apply_parsed_env
participant Command
participant WSLENV
ExecOrShell->>apply_parsed_env: pass parsed environment
apply_parsed_env->>Command: set environment variables
apply_parsed_env->>WSLENV: append valid names on Windows
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 |
On Windows with WSL installed, a script run through
usage bashsees everyusage_*variable unset — no error, no warning, just empty values.Cause
Two independent facts combine.
1. The
bashthat gets resolved is always WSL's. The Win32 executable search order is application directory → current directory → system directory → … → PATH, with the system directory ahead of PATH. Installing WSL putsC:\Windows\System32\bash.exethere, soCommand::new("bash")incli/src/cli/shell.rspicks the WSL launcher no matter what else is on PATH. A diagnostic script confirms it:OSTYPE=linux-gnu,uname -s = Linux. Note that PowerShell'sGet-Command bashreports Git Bash on the same machine, which is why inspecting PATH does not reveal this.2. WSL only carries a Win32 variable across the boundary if
WSLENVnames it. So thecmd.env(key, val)loop inshell.rs(and the identical one inexec.rs) transfers nothing.Fix
Both call sites now go through a shared
env::apply_parsed_env, which sets the variables as before and, on Windows, also appends their names toWSLENV.Names are added bare, with no flags.
WSLENVsupports/p(translate as a path) and/l(as a path list), but usage has no idea whether a given value is a path —/pwould silently rewrite anything that merely looks like one, and variadic values are joined withshell_words::join, so they are not a;-separated path list either. Unflagged names copy the value verbatim, so a script receives the same bytes it would on Unix.Entries already present in
WSLENVare preserved exactly, flags included: a name the user configured is theirs, not ours to redefine.cfg!(windows)rather than#[cfg(windows)], becausetest.ymlruns onubuntu-latestonly (the Windows job inpublish-cli.ymlis a tag-triggeredcargo buildwith no test or clippy). A#[cfg]block here would never be compiled, type-checked or linted anywhere.Why not change which shell gets resolved
Deliberately out of scope. Changing the resolution would break scripts that already depend on WSL bash, and there is no single right answer — some Windows users want WSL, others want Git Bash or MSYS2. For comparison, mise does not guess either: it defaults to
cmd /con Windows and lets the shell be specified explicitly, including as an absolute path. usage-lib already follows the same philosophy inlib/src/sh.rs, which usessh -con Unix andcmd /con Windows. Only theusage <shell> <script>path assumes bash, and that assumption comes from the script's own shebang.This PR restores a transport that was silently dropping data; it does not decide policy.
Verified
Windows 11 + WSL2, before and after:
With a pre-existing
WSLENV=MY_EXISTINGandMY_EXISTING=hello, that entry still transfers alongside the new ones (MY_EXISTING=[hello]).11 unit tests cover the
WSLENVstring building — ordering, appending, dedup against existing bare and flagged names, empty-segment handling, and the no-flags guarantee. They are pure-function tests, so they run on the Linux CI. One further test pins the invariant they rely on: thatas_env()never produces a key containing:or/, which would otherwise corrupt the list.No integration test is added — it would need Windows plus WSL2, and there is no Windows runner in CI, so a
#[cfg(windows)] #[ignore]test would simply rot.Not covered
Path translation (a Windows path passed as an argument still arrives as
C:\..., unusable inside WSL), shell resolution, and any behaviour off Windows (WSLENVis untouched there).This pull request was generated by Claude Code.
Summary by CodeRabbit
WSLENVsettings and flags are preserved, while duplicate or invalid entries are safely excluded.