fix(bash): use real bash on Windows instead of cmd.exe /C by w0wl0lxd · Pull Request #617 · tontinton/maki

w0wl0lxd · 2026-07-21T21:45:34Z

Summary

On Windows, the bash tool silently substituted cmd.exe /C for bash -c, breaking quoted arguments like git commit -m "feat: msg".

maki-config: add find_bash_on_path() — probes PATH first, then falls back to common install paths (Git Bash, Cygwin, MSYS2)
maki-lua: shell_command() returns Result, uses bash on Windows, errors with install guide if not found
maki-ui: same fix for UI !/!! shell commands
install.ps1: detects missing bash after installation and offers to install Git for Windows via winget (or prints manual instructions)
Error messages include winget install --id Git.Git -e --source winget for quick copy-paste
Quick-start docs: adds note that bash tool requires Git for Windows or WSL
Generated docs updated to reflect the new behavior

Unix path: zero behavioral diff. If bash is not found on Windows, the tool errors with clear instructions instead of silently corrupting arguments. The installer proactively offers to install Git for Windows.

Test plan

cargo clippy --all --tests -- -D warnings — clean
cargo nextest run --workspace — 3169 passed
CI will verify Windows path via test-windows job (PR ci: add cross-platform test jobs, drop unused ripgrep #616)

Closes #602.

w0wl0lxd · 2026-07-23T19:08:35Z

Heads-up: PR #649 (list mode for maki.fn.jobstart) is stacked on this PR and should be merged after it.

tontinton · 2026-07-24T20:45:49Z

+    std::env::var_os("PATH")
+        .and_then(|paths| {
+            std::env::split_paths(&paths).find_map(|dir| {
+                let bash = dir.join("bash.exe");
+                if bash.is_file() { Some(bash) } else { None }
+            })
+        })


the PATH scan finds C:\Windows\System32\bash.exe on any machine with WSL enabled. that's the legacy WSL launcher, and System32 sits early in PATH. meanwhile git for windows only puts Git\cmd on PATH by default, not Git\bin where bash.exe actually lives.

so on a box with both, we run WSL bash instead of git bash, which is the opposite of what the doc comment promises. and if WSL is enabled with no distro installed, that bash.exe just errors out.

I'd check the git/cygwin/msys candidates before the PATH scan, or skip PATH entries under %SystemRoot%.

the skip compares the PATH entry for equality with %SystemRoot% (C:\Windows), but the legacy bash lives in C:\Windows\System32, and that entry is never equal to C:\Windows, so it's never skipped. the comparison is also case-sensitive. so on a machine with WSL enabled and git installed somewhere non-default (scoop, D:\Git), the PATH scan still picks system32 bash. make it a prefix match, case-insensitive

tontinton · 2026-07-24T20:45:49Z

+fn find_wsl() -> Option<PathBuf> {
+    std::env::var_os("PATH")
+        .and_then(|paths| {
+            std::env::split_paths(&paths).find_map(|dir| {
+                let wsl = dir.join("wsl.exe");
+                if wsl.is_file() { Some(wsl) } else { None }
+            })
+        })
+        .or_else(|| {
+            let path = PathBuf::from(r"C:\Windows\System32\wsl.exe");
+            path.is_file().then_some(path)
+        })
+}


wsl.exe ships in System32 on stock windows 10 and 11 even when no distro is installed. so on a machine with no git bash and no distro, find_wsl succeeds and the user gets wsl's "no installed distributions" text instead of BASH_NOT_FOUND_ERROR. wsl.exe prints that as UTF-16, so it lands in tool output as null-interleaved bytes.

which means the nice error message you wrote never shows on the most common broken setup. worth running wsl.exe -e true once and caching the result before trusting wsl.

the probe runs on every call, and it's a blocking status() inside async code. wsl's vm idles out after about a minute, so wsl-only machines pay 1-3s of vm boot twice per command. cache it in a OnceLock

tontinton · 2026-07-24T20:45:49Z

        on_exit: Option<RegistryKey>,
    ) -> Result<u32, String> {
-        let mut command = shell_command(cmd);
+        let mut command = maki_config::bash_command(cmd)?;


when this resolves to the wsl fallback, .env(...) sets the vars on the wsl.exe process on the windows side. they do not cross into linux unless the names are listed in WSLENV.

so GIT_TERMINAL_PROMPT=0 (plugins/bash/init.lua:390 and shell.rs:205) and any env passed to jobstart get silently dropped under wsl. git can then hang waiting for credentials inside a tool call.

if we keep the wsl fallback, append the var names to WSLENV on the command.

two things: c.env("WSLENV", ...) replaces the user's existing WSLENV instead of appending to it. and /p is the path-translation flag, applied to every var it mangles non-path values (anything with : or \) and silently drops what it can't translate. GIT_TERMINAL_PROMPT=0 doesn't need translation. append to the inherited value and use bare names, /p only if the value looks like a windows path

tontinton · 2026-07-24T20:45:49Z

 };
 use maki_providers::Message;

+use maki_config;


bare use maki_config; is redundant here, and clippy's single_component_path_imports should fail it under -D warnings. drop it, or use maki_config::bash_command; and call it directly.

tontinton · 2026-08-06T01:19:19Z

+    let system_root = std::env::var_os("SystemRoot").unwrap_or_else(|| "C:\\Windows".into());
+    std::env::var_os("PATH").and_then(|paths| {
+        std::env::split_paths(&paths).find_map(|dir| {
+            if dir.as_os_str() == system_root {
+                return None;
+            }
+            let bash = dir.join("bash.exe");
+            bash.is_file().then_some(bash)
+        })
+    })


if PATH has a trailing or doubled ; (super common on windows), split_paths yields an empty entry, dir.join("bash.exe") becomes relative, and we check + execute it against the cwd, which is the user's repo. a cloned repo with a bash.exe at its root becomes the shell. same for wsl.exe, which is worse because find_wsl runs it immediately for the -e true probe. skip empty/relative entries:

if dir.as_os_str().is_empty() || dir.is_relative() { return None; }

tontinton · 2026-08-06T01:19:26Z

+    $paths = $env:Path -split ';'
+    foreach ($dir in $paths) {
+        $bashPath = Join-Path $dir.Trim('"') "bash.exe"


the script runs with $ErrorActionPreference = "Stop", and Join-Path -Path "" -ChildPath "bash.exe" throws on an empty PATH segment. so anyone with a trailing ; in PATH gets a red error mid-install. Add-ToUserPath below already filters empty entries, do the same here:

$paths = $env:Path -split ';' | Where-Object { $_.Trim() -ne "" }

tontinton · 2026-08-06T01:19:33Z

+    foreach ($dir in $paths) {
+        $wslPath = Join-Path $dir.Trim('"') "wsl.exe"
+        if (Test-Path -LiteralPath $wslPath -PathType Leaf) {
+            return $true
+        }
+    }
+    if (Test-Path -LiteralPath "C:\Windows\System32\wsl.exe" -PathType Leaf) {
+        return $true
+    }


Test-BashAvailable returns true on mere wsl.exe presence, but the stub ships on stock win 10/11 with no distro, which is exactly why the rust side probes with -e true. so the warning never fires on the machines it was written for. also the bash loop above doesn't skip system32 at all. mirror the rust logic:

wsl.exe -e true 2>$null; if ($LASTEXITCODE -eq 0) { return $true }

tontinton · 2026-08-06T01:19:39Z

        on_exit: Option<RegistryKey>,
    ) -> Result<u32, String> {
-        let mut command = shell_command(cmd);
+        let mut command = maki_config::bash_command(cmd, env.as_ref())?;


the jobstart doc comment below and site/docs/content/lua-api/_index.md still say the command runs through cmd /C on windows

tontinton

please read also the comments made before my last review, they were not fixed

tontinton · 2026-08-11T20:19:41Z

+    let system_root = std::env::var_os("SystemRoot").unwrap_or_else(|| "C:\\Windows".into());
+    std::env::var_os("PATH").and_then(|paths| {
+        std::env::split_paths(&paths).find_map(|dir| {
+            if dir.as_os_str() == system_root {
+                return None;
+            }


This guard still never matches. the legacy bash.exe is in C:\Windows\System32, and SystemRoot is C:\Windows, so dir.as_os_str() == system_root is comparing against the wrong dir. the doc comment above says PATH entries under %SystemRoot% are skipped, but the code checks equality, not prefix.

So on a machine with WSL enabled and bash on PATH in a non-default spot (scoop, portable git), System32 comes first and we still pick the legacy WSL launcher, then run it as plain bash -c with none of the WSLENV handling below. needs a case-insensitive prefix check.

tontinton · 2026-08-11T20:19:41Z

+    let candidates = [
+        r"C:\Program Files\Git\bin\bash.exe",
+        r"C:\Program Files\Git\usr\bin\bash.exe",
+        r"C:\Program Files (x86)\Git\bin\bash.exe",
+        r"C:\cygwin64\bin\bash.exe",
+        r"C:\cygwin\bin\bash.exe",
+        r"C:\msys64\usr\bin\bash.exe",
+        r"C:\msys32\usr\bin\bash.exe",
+    ];


Still hardcoded to C:\. windows on another drive, or a per-user git install, and all candidates miss. and since git only puts Git\cmd on PATH (no bash.exe there), the PATH scan misses too, so we fall to WSL or error even though git bash is installed. build these from %ProgramFiles%, %ProgramFiles(x86)% and %ProgramW6432% instead of the literal C:\ prefix.

tontinton · 2026-08-11T20:19:41Z

+/// blindly trusting its presence leads to garbled UTF-16 "no installed
+/// distributions" output instead of our nice error message.
+#[cfg(windows)]
+fn find_wsl() -> Option<PathBuf> {


I asked for this to be cached. bash_command runs on every bash tool call and every jobstart, and on a WSL-only machine each one now spawns wsl.exe twice (once for -e true, once for the real command). wsl startup is not cheap, can be seconds when the VM is cold. wrap the whole resolution in a OnceLock and pay the probe once per process.

tontinton · 2026-08-11T20:19:41Z

+        if let Some(wsl) = find_wsl() {
+            let mut c = Command::new(wsl);
+            c.arg("-e").arg("bash").arg("-c").arg(cmd);
+            if let Some(env_map) = env {
+                let wsl_env: Vec<String> = env_map.keys().map(|k| format!("{k}/p")).collect();
+                if !wsl_env.is_empty() {
+                    c.env("WSLENV", wsl_env.join(":"));
+                }
+            }
+            return Ok(c);


Two problems with the WSLENV handling:

/p marks the value as a path to be translated. jobstart env is arbitrary user vars, and GIT_TERMINAL_PROMPT=0 is not a path. values with colons or backslashes get mangled. only actual paths should get /p.

c.env("WSLENV", ...) overwrites whatever WSLENV the user already has, silently breaking their own var sharing. append instead.

tontinton · 2026-08-11T20:19:41Z

+            c.arg("-c").arg(cmd);
+            return Ok(c);
+        }
+        if let Some(wsl) = find_wsl() {


I still think this fallback should be opt-in or dropped. two things make it worse than the error message:

kill_job on windows does TerminateProcess on the pid, which is wsl.exe. the linux child keeps running in the VM, so every timeout or jobstop leaks a process.

the agent works with windows paths from read/glob/index, then bash sees a linux world where C:\ paths don't resolve. half the tool calls will just fail in confusing ways.

A clear "install git for windows" error is a better experience than a fallback that half works.

tontinton · 2026-08-11T20:19:41Z

+    let mut env = HashMap::new();
+    env.insert("GIT_TERMINAL_PROMPT".to_string(), "0".to_string());


This is the exact case from my last review and it's still broken: env is None here, so under the wsl fallback GIT_TERMINAL_PROMPT never makes it into WSLENV and git can still hang waiting for credentials. pass it through the env param instead of .env() after.

tontinton · 2026-08-11T20:19:41Z

+function Test-BashAvailable {
+    $paths = $env:Path -split ';'
+    foreach ($dir in $paths) {
+        $bashPath = Join-Path $dir.Trim('"') "bash.exe"
+        if (Test-Path -LiteralPath $bashPath -PathType Leaf) {
+            return $true
+        }
+    }
+    $candidates = @(
+        "C:\Program Files\Git\bin\bash.exe",
+        "C:\Program Files\Git\usr\bin\bash.exe",
+        "C:\Program Files (x86)\Git\bin\bash.exe",
+        "C:\cygwin64\bin\bash.exe",
+        "C:\cygwin\bin\bash.exe",
+        "C:\msys64\usr\bin\bash.exe",
+        "C:\msys32\usr\bin\bash.exe"
+    )
+    foreach ($candidate in $candidates) {
+        if (Test-Path -LiteralPath $candidate -PathType Leaf) {
+            return $true
+        }
+    }
+    foreach ($dir in $paths) {
+        $wslPath = Join-Path $dir.Trim('"') "wsl.exe"
+        if (Test-Path -LiteralPath $wslPath -PathType Leaf) {
+            return $true
+        }
+    }
+    if (Test-Path -LiteralPath "C:\Windows\System32\wsl.exe" -PathType Leaf) {
+        return $true
+    }


Test-BashAvailable returns true on stock windows 10/11 because System32\wsl.exe ships with the OS even when no distro is installed. so this whole warning/winget block never runs for the exact user it was written for. the rust side verifies with wsl.exe -e true, do the same here instead of Test-Path on the exe.

tontinton · 2026-08-11T20:19:41Z

-
 fn kill_job(meta: &mut JobMeta) {
    let pid = meta.pid;
    #[cfg(unix)]


Doc comment still says cmd /C on windows, and the generated lua-api page (site/docs/content/lua-api/_index.md:1307) still has it too. update and run just gen-docs.

tontinton · 2026-08-20T18:13:48Z

Pushed a fix for the open review points (KISS route: dropped the WSL fallback rather than patching it).

find_bash builds candidates from %ProgramFiles%, %ProgramFiles(x86)%, %ProgramW6432% and %SystemDrive%, no more hardcoded C:\
PATH scan skips empty/relative entries and anything under %SystemRoot% via a case-insensitive prefix check, so System32\bash.exe never wins
resolution cached in a OnceLock, one lookup per process
WSL fallback removed: kill_job leaked the linux child, and windows paths from read/glob/index do not resolve inside the VM. A clear install error is better. This also deletes the whole WSLENV mess, so bash_command no longer takes env and callers just set their own vars
install.ps1 mirrors the rust lookup (program files roots, system drive, PATH minus SystemRoot, empty segments filtered) and no longer treats the wsl.exe stub as bash
jobstart doc comment and the generated lua-api page no longer say cmd /C

cargo clippy --all --tests -D warnings and cargo nextest run --workspace are clean.

Closes tontinton#602. On Windows the bash tool swapped `bash -c` for `cmd.exe /C`, so a command with quotes like `git commit -m "feat: msg"` arrived mangled. `bash_command` now finds a real bash (Git for Windows, Cygwin, MSYS2), caches the hit in a `OnceLock`, and errors with install instructions when there is none. Finding it is the fiddly part: git puts its `cmd` dir on PATH and keeps bash in the sibling `bin`, so we walk from git to `bin\bash.exe` instead of scanning PATH for a shell that is never there, with `%ProgramFiles%` and `%SystemDrive%` as backup. PATH entries under `%SystemRoot%` are dropped because `C:\Windows\System32\bash.exe` is the legacy WSL launcher, and so are non absolute ones, since a trailing `;` would otherwise turn a `bash.exe` sitting in the user's repo into the shell. There is no WSL fallback on purpose, killing `wsl.exe` leaves the linux child alive and windows paths mean nothing inside the VM, and picking the candidates is plain string work in `bash_candidates` so it is tested on every platform.

w0wl0lxd changed the title ~~Use real bash on Windows instead of cmd.exe /C~~ fix(bash): use real bash on Windows instead of cmd.exe /C Jul 21, 2026

w0wl0lxd marked this pull request as ready for review July 21, 2026 22:19

This was referenced Jul 22, 2026

feat(lua): add list mode to maki.fn.jobstart w0wl0lxd/maki#28

Closed

feat(lua): add list mode to maki.fn.jobstart w0wl0lxd/maki#29

Closed

bash tool mangles quoted arguments on Windows #602

Closed

feat(lua): add list mode to maki.fn.jobstart #649

Open

w0wl0lxd force-pushed the fix/windows-bash-exec-clean branch from 872275f to 2afaa81 Compare July 23, 2026 04:10

tontinton mentioned this pull request Jul 24, 2026

ci: add cross-platform test jobs, drop unused ripgrep #616

Closed

tontinton requested changes Jul 24, 2026

View reviewed changes

w0wl0lxd force-pushed the fix/windows-bash-exec-clean branch from 4eb6d9a to 8f4ea42 Compare August 4, 2026 15:33

tontinton requested changes Aug 6, 2026

View reviewed changes

tontinton requested changes Aug 11, 2026

View reviewed changes

tontinton force-pushed the fix/windows-bash-exec-clean branch 5 times, most recently from df02f22 to a213ae0 Compare August 20, 2026 19:00

tontinton force-pushed the fix/windows-bash-exec-clean branch from a213ae0 to 6c50f10 Compare August 20, 2026 19:09

tontinton approved these changes Aug 20, 2026

View reviewed changes

tontinton enabled auto-merge (rebase) August 20, 2026 19:11

tontinton merged commit d0805c7 into tontinton:main Aug 20, 2026
11 checks passed

		let mut env = HashMap::new();
		env.insert("GIT_TERMINAL_PROMPT".to_string(), "0".to_string());

Conversation

w0wl0lxd commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

w0wl0lxd commented Jul 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton commented Aug 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

w0wl0lxd commented Jul 21, 2026 •

edited

Loading

tontinton Aug 6, 2026 •

edited

Loading

tontinton Aug 6, 2026 •

edited

Loading

tontinton Aug 6, 2026 •

edited

Loading

tontinton Aug 6, 2026 •

edited

Loading

tontinton Aug 6, 2026 •

edited

Loading

tontinton Aug 6, 2026 •

edited

Loading

tontinton Aug 6, 2026 •

edited

Loading