fix(windows): run run= scripts with sh when available - #765
fix(windows): run run= scripts with sh when available#765JamBalaya56562 wants to merge 1 commit into
run= scripts with sh when available#765Conversation
📝 WalkthroughWalkthroughThe shared ChangesShell execution and completion
Estimated code review effort: 3 (Moderate) | ~25 minutes 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: 1
🤖 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 `@lib/src/sh.rs`:
- Line 88: Update the stdout conversion in sh() to use a lossy UTF-8 conversion
instead of String::from_utf8(...).expect(...), so non-UTF-8 run= script output
is converted with replacement characters and does not panic.
🪄 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: c7cb8947-9c97-4803-bd01-40779eab582f
📒 Files selected for processing (6)
cli/src/cli/complete_word.rscli/tests/complete_word.rsdocs/spec/reference/cmd.mddocs/spec/reference/complete.mdlib/src/lib.rslib/src/sh.rs
Executing a spec's `run=` was split across two nearly identical private
functions — `lib/src/sh.rs` for `mount run=`, and a copy in
`cli/src/cli/complete_word.rs` for `complete run=`. They differed in exactly
one respect: the lib copy used `cmd /c` on Windows, while the cli copy had no
platform branch at all and always used `sh -c`. The same `run=` syntax
therefore ran under a different shell depending on which KDL node it was
written in.
`run=` is a POSIX command line. The reference docs' own examples are `ls
modules/{{words[PREV]}}/controllers` and `echo {{ words | slice(...) }}`;
mise's spec uses `| awk`, and a multi-line `case ... esac` with `cut` and
`sed`. `cmd /c` cannot run any of it, and does not fail when it can't:
`cmd /c ./mounted.sh --mount` hands the `.sh` to whatever program is
registered for the extension, then returns exit code 0 with empty stdout,
which the spec parser accepts as an empty spec.
Both call sites now use `usage::sh::sh`. It runs `sh -c`, and on Windows
retries with `cmd /c` only when `sh` could not be found — so the `cmd /c`
path is preserved byte for byte for machines without a POSIX shell, and
`mount run="mise tasks --usage"` keeps working there. A shell that exists but
fails to start is reported rather than quietly demoted.
Output that is not valid UTF-8 is now an error rather than a panic. That
mattered less when this ran only under `sh`; the `cmd /c` fallback emits the
console code page, which is not UTF-8 outside English locales, so a single
`run=` script could take the process down.
The context string on process errors is also fixed: the cli copy hardcoded
`sh -c {script}`, so Windows users were told about a shell that had not run.
afe25b1 to
2f3c019
Compare
Compatibility first
There is no (OS, PATH) combination where this is worse than today. The
cmd /cpath is preserved byte for byte and is still used whenevershcannot be found, so a Windows machine without a POSIX shell behaves exactly as it does now —mount run="mise tasks --usage"and other plain command invocations keep working. Whereshis available, shebang mounts and POSIX one-liners start working instead of silently doing the wrong thing. Unix is untouched.Problem
Executing a spec's
run=is split across two nearly identical private functions:lib/src/sh.rsmount run=(viaSpecCommand::mount)cmd /ccli/src/cli/complete_word.rs:414complete run=sh -c, no platform branchEverything else about them is identical — stdin closed, stderr inherited,
__USAGEset, same error handling. The cli copy exists only becauselib/src/lib.rsdeclarespub(crate) mod sh. The result is that the samerun=syntax runs under a different shell depending on which KDL node it was written in.run=is a POSIX command line. The reference docs' own examples arels modules/{{words[PREV]}}/controllersandecho {{ words | slice(start=-4) | join(...) }}.examples/mise.usage.kdl:1163ismise alias ls {{words[PREV]}} | awk '{print $2}', and:1177-1215is a multi-linecase $cur in … esacwithcutandsed.cmd /ccannot run any of that:Worse, for the shebang-script case it does not fail at all.
cmd /c ./mounted.sh --mounthands the.shto whatever program is registered for the extension — on my machine that opened an editor window — and then returns exit code 0 with empty stdout. That empty string goes straight to the spec parser, which accepts it as a spec with nothing in it. A mounted command silently completes to nothing.cmd /carrived in v2.16.0, "(windows) add Windows binaries and fix completion support (#472)". That PR was about shipping Windows binaries; which shell interpretsrun=does not look like it was the question being answered.Fix
lib/src/sh.rsbecomes the only implementation, exposed asusage::sh::sh(pub(crate) mod sh→pub mod sh; additive,cargo semver-checks check-releasereportsno semver update required). The cli copy is deleted and its call site now uses the shared one.Shell selection: run
sh -c; on Windows, if that fails withErrorKind::NotFound, retry withcmd /c; if neither can start, return an explicit error naming both. Any other spawn failure is reported as-is rather than falling back — quietly demoting a brokenshto a different shell is the same class of silent wrongness this is meant to remove.The platform branch lives in one pure function (
fallback_for) behindcfg!(windows)rather than#[cfg(windows)], so the fallback logic is still compiled, linted and unit-tested on the Linux CI. On Unix it returnsNone, so the syscall sequence is identical to today's.bashis deliberately not searched for.C:\Windows\System32\bash.exeis the WSL launcher and wins over PATH on any machine with WSL installed, which would run completion scripts against a different filesystem. Git for Windows shipssh.exeandbash.exeside by side, so looking forbashwould not widen coverage anyway. Nowhich-style probe either: a pre-flight spawn costs a process on the completion hot path and races the real call, while a failedCreateProcessalready answers the question using the OS's own resolution rules.Also fixed in passing: the cli copy hardcoded
format!("sh -c {script}")as the error context, so Windows users were told about a shell that had not actually run.Verified
Windows 11,
shon PATH via Git for Windows.A
mount run=holding a POSIX one-liner — the casecmd /ccannot run at all:cargo test -p usage-cli --test complete_wordwent from 82 seconds for a single test, with an editor window opening per fixture, to 2.7 seconds for the whole file with none. The eight mount tests still fail on this particular machine, but for an unrelated reason that this change makes visible instead of hiding: the fixtures are#!/usr/bin/env -S usage bashscripts, andusage bashresolves to WSL's bash here, which cannot open aC:/…path. Previously that surfaced as exit 0 and an empty spec; now it isexited with code 127 — sh -c mounted.sh --mount. On a Windows machine without WSL those eight would pass.Also run:
cargo test -p usage-lib --all-features(311 pass),cargo clippy --all --all-features -- -D warnings,cargo fmt --all -- --check,prettier -con the touched docs.Tests
lib/src/sh.rsgets its first unit tests. The shell-selection and message-building functions are pure, so they run on every platform — including thatPermissionDenieddoes not fall back, and that the message names both shells and truncates a multi-line script to its first line.cli/tests/complete_word.rsgains askip_if_posix_shell_missingguard on the eight mount tests, mirroringskip_if_shell_missinginshell_completions_integration.rsincluding thepanic!underCI. Those fixtures are shebang scripts and cannot work without a POSIX shell; without the guard, a Windows machine lacking one falls into thecmd /cpath and gets an editor window per test rather than a failure. The Linux CI is unaffected —shis always there, and a missing one underCIstill fails loudly.Deliberately not doing
test.ymlisubuntu-latestonly, so the fallback's real behaviour is not covered here; that belongs to a separate CI discussion.UsageErrvariant for "no shell". That would be a major semver bump (as fix(parse): enforce double_dash="required" for positional args #762 is currently discovering), so the message rides on the existingXXError::Error.Non-UTF-8 output (added after review)
String::from_utf8(...).expect(...)used to panic on non-UTF-8 stdout. I had this down as a follow-up, but it belongs here: thecmd /cfallback emits the console code page, which is not UTF-8 outside English locales, so on a Japanese or Cyrillic Windows a singlerun=script could take the whole process down. It is now an error naming the script and the shell that ran it.Reported rather than converted lossily: a mount's output is parsed as a spec, and replacement characters there would resurface as a baffling KDL syntax error instead of an encoding one. No new
UsageErrvariant — the message rides on the existingXXError::Error, so the public API is unchanged.This pull request was generated by Claude Code.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes