From 211e588cdf6d1ee96dfd0faca616372344bd958a Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 18 Jun 2026 15:18:04 -0400 Subject: [PATCH 1/8] fix(buzz-dev-mcp): resolve non-WSL bash so the MCP shell works on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MCP shell spawned a bare `Command::new("bash")`, which on Windows re-enters PATH search and resolves `System32\bash.exe` (the WSL launcher). That fails at spawn with 0x8007072c, so the agent's only channel-posting transport was fully dead on Windows. resolve_bash() now returns an absolute, non-WSL bash path and spawns it directly: GIT_BASH override -> installed Git for Windows -> bundled bash staged next to the exe -> PATH scan excluding %SystemRoot%. Unix is a no-op. The System32 exclusion compares path components case-INsensitively because Windows paths are case-insensitive but `Path::starts_with` is not — a PATH entry spelled `C:\WINDOWS\System32` would otherwise leak WSL's bash. To make a bare (Git-less) host work, the build bundles a genuine bash: the PortableGit MSYS runtime (`usr/` + `bin/`) with the separable `mingw64/` git-program subtree dropped. The runtime is kept whole — bash loads its DLLs lazily, so a hand-trimmed copy would pass an existence check yet fail mid-command. The `git-bash` install-root dir name is a single path contract shared byte-identical across shell.rs, build-release-config.mjs, and bundle-sidecars.sh. The download/extract/drop logic lives in scripts/stage-windows-bash.sh so the Windows PR CI job can stage the tree and spawn the staged bash on a real coreutils pipeline — gating the SFX extraction, the post-`mingw64`-drop spawn, and the lazily-loaded DLL closure that would otherwise ship unexercised until a tagged release. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .github/workflows/ci.yml | 20 ++ crates/buzz-dev-mcp/src/shell.rs | 230 ++++++++++++++++++++++- desktop/scripts/build-release-config.mjs | 21 +++ scripts/bundle-sidecars.sh | 9 + scripts/stage-windows-bash.sh | 66 +++++++ 5 files changed, 345 insertions(+), 1 deletion(-) create mode 100755 scripts/stage-windows-bash.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 281fee0a16..6dc477a0d8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -688,6 +688,26 @@ jobs: run: cargo clippy --workspace --all-targets --target $env:TARGET -- -D warnings - name: Check (workspace) run: cargo check --workspace --all-targets --target $env:TARGET + - name: Test (buzz-dev-mcp) + # The Windows-only bash resolver lives in buzz-dev-mcp; its unit tests + # only gate if this crate is tested ON Windows. + run: cargo test -p buzz-dev-mcp --target $env:TARGET + # The bundled-bash staging (PortableGit download, SFX extract, mingw64 drop) + # runs only in release.yml on tag — so without this step the agent's only + # Windows transport would ship UNEXERCISED until a tagged release hits users. + # Stage the tree and spawn the staged bash on a real coreutils pipeline: this + # gates the SFX `-o` POSIX-path extraction, that bash.exe still spawns after + # mingw64/ is dropped, and that the lazily-loaded MSYS DLL closure (msys-2.0.dll, + # coreutils) survives — the exact behaviors unconfirmable off a non-Windows host. + - name: Smoke-test bundled bash staging + shell: bash + run: | + set -euo pipefail + stage_dir="$RUNNER_TEMP/git-bash" + scripts/stage-windows-bash.sh "$stage_dir" + out=$("$stage_dir/bin/bash.exe" -c 'echo hello | rev') + [[ "$out" == "olleh" ]] || { echo "staged bash pipeline failed: got '$out'" >&2; exit 1; } + echo "staged bash spawned and ran a coreutils pipeline" - name: Check (Tauri crate) run: cargo check --manifest-path desktop/src-tauri/Cargo.toml --target $env:TARGET env: diff --git a/crates/buzz-dev-mcp/src/shell.rs b/crates/buzz-dev-mcp/src/shell.rs index 2a48d2cfd4..54a27e85ec 100644 --- a/crates/buzz-dev-mcp/src/shell.rs +++ b/crates/buzz-dev-mcp/src/shell.rs @@ -143,7 +143,11 @@ pub async fn run( )); } - let mut cmd = Command::new("bash"); + let bash = match resolve_bash(&state.shim.path_env) { + Ok(path) => path, + Err(msg) => return Ok(CallToolResult::error(vec![Content::text(msg)])), + }; + let mut cmd = Command::new(&bash); cmd.arg("-c").arg(&p.command); cmd.current_dir(&workdir); cmd.env("PATH", &state.shim.path_env); @@ -312,6 +316,137 @@ pub async fn run( Ok(CallToolResult::success(vec![Content::text(text)])) } +/// The bundled bash subtree's directory name under the install root, and the +/// relative path to its `bash.exe`. This is the THREE-FILE PATH CONTRACT — it must +/// stay byte-identical with: +/// 1. `scripts/bundle-sidecars.sh` — stages the bash tree to +/// `desktop/src-tauri/binaries/git-bash/` (the bundle-source dir). +/// 2. `desktop/scripts/build-release-config.mjs` — emits the Windows-only +/// `bundle.resources` Map `{ "binaries/git-bash": "git-bash" }`, whose TARGET +/// (`git-bash`) is what Tauri's NSIS/MSI installer stages next to the exe. +/// 3. this resolver — joins `current_exe().parent()` + `git-bash\bin\bash.exe`. +/// Drift between (2)'s target and this string ships a working bundle but a broken +/// runtime path. Keep all three in lockstep. +#[cfg(windows)] +const BUNDLED_BASH_REL: &str = r"git-bash\bin\bash.exe"; + +/// Resolve a genuine, non-WSL bash to an absolute path so we spawn it directly +/// instead of letting `Command::new("bash")` re-enter PATH search — on Windows +/// that search finds `System32\bash.exe` (the WSL launcher), which fails at spawn +/// with `0x8007072c` and can never run the agent's POSIX commands. +/// +/// On Unix, bare `bash` resolved via PATH is correct and was never broken, so the +/// resolver is a no-op there. The probe logic is Windows-only. +#[cfg(not(windows))] +fn resolve_bash(_path_env: &str) -> Result { + Ok(PathBuf::from("bash")) +} + +/// Windows bash resolution. Probe order (first hit wins): +/// 1. `GIT_BASH` env override (escape hatch / explicit operator choice). +/// 2. Installed Git for Windows (fast path when the user has Git). +/// 3. The bundled bash staged next to our exe (guaranteed target — this +/// is what makes a bare, Git-less host work since the app is self-contained). +/// 4. PATH scan, EXCLUDING System32 (so we never resolve WSL's `bash.exe`). +/// No bash found -> actionable error returned BEFORE spawn. +#[cfg(windows)] +fn resolve_bash(path_env: &str) -> Result { + if let Some(p) = std::env::var_os("GIT_BASH").map(PathBuf::from) { + if p.is_file() { + return Ok(p); + } + } + + for root in ["ProgramFiles", "LocalAppData"] { + if let Some(base) = std::env::var_os(root) { + let candidate = match root { + "LocalAppData" => PathBuf::from(&base).join("Programs").join("Git"), + _ => PathBuf::from(&base).join("Git"), + } + .join("bin") + .join("bash.exe"); + if candidate.is_file() { + return Ok(candidate); + } + } + } + + // Bundled bash, located relative to OUR OWN executable. On Windows, Tauri + // stages `bundle.resources` flat in the directory that contains the exe + // (tauri 2.11.2 `resource_dir()` == exe parent on Windows), and every sidecar + // — including this one — lives in that same dir. This relative-to-self resolution + // is Windows-ONLY: macOS stages resources to `../Resources` and Linux to + // `usr/lib/`, so a cross-platform "resource relative to exe" helper would + // be wrong on those platforms. + if let Ok(exe) = std::env::current_exe() { + if let Some(dir) = exe.parent() { + if let Some(p) = bundled_bash(dir) { + return Ok(p); + } + } + } + + if let Some(p) = scan_path_for_bash(path_env, std::env::var_os("SystemRoot").map(PathBuf::from)) + { + return Ok(p); + } + + Err("no bash found: install Git for Windows, or set GIT_BASH to a bash.exe path".into()) +} + +/// Compute the bundled bash path relative to the install dir (the exe's parent), +/// `is_file`-gated so dev/CI builds without a staged resource return None and let +/// the caller fall through cleanly — never returning a non-existent path that +/// would fail later at spawn with a worse message. +#[cfg(windows)] +fn bundled_bash(install_dir: &Path) -> Option { + let bundled = install_dir.join(BUNDLED_BASH_REL); + bundled.is_file().then_some(bundled) +} + +/// True if `dir` is `root` or lives under it, comparing path components +/// case-INsensitively. Windows paths are case-insensitive, but `Path::starts_with` +/// compares components case-sensitively on every platform — so a PATH entry spelled +/// `C:\WINDOWS\System32` would slip past a `%SystemRoot%`=`C:\Windows` prefix test +/// and let WSL's `System32\bash.exe` be resolved, reintroducing the `0x8007072c` +/// spawn failure. Component-wise comparison (not a lowercased substring match) avoids +/// a false hit on a sibling like `C:\Windows2`. +#[cfg(windows)] +fn is_under_dir(dir: &Path, root: &Path) -> bool { + let mut dir_components = dir.components(); + for root_component in root.components() { + match dir_components.next() { + Some(d) + if d.as_os_str() + .eq_ignore_ascii_case(root_component.as_os_str()) => {} + _ => return false, + } + } + true +} + +/// Scan the child's PATH for `bash.exe`, skipping the Windows system directory +/// (`system_root`, normally `%SystemRoot%`) so we never resolve WSL's +/// `System32\bash.exe`. PATH is parsed with `std::env::split_paths` (never a +/// hand-split on ';') so it matches exactly what the spawned child would see. +#[cfg(windows)] +fn scan_path_for_bash(path_env: &str, system_root: Option) -> Option { + for dir in std::env::split_paths(path_env) { + if let Some(ref root) = system_root { + // Skip System32 (and any other dir under %SystemRoot%) — that's where + // WSL's bash.exe lives. + if is_under_dir(&dir, root) { + continue; + } + } + let candidate = dir.join("bash.exe"); + if candidate.is_file() { + return Some(candidate); + } + } + None +} + #[cfg(unix)] fn set_process_group(cmd: &mut Command) { cmd.process_group(0); @@ -563,3 +698,96 @@ mod tests { ); } } + +#[cfg(all(test, windows))] +mod windows_resolver_tests { + use super::*; + use std::env; + use tempfile::tempdir; + + fn touch(path: &Path) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("mkdir"); + } + std::fs::write(path, b"").expect("touch"); + } + + #[test] + fn bundled_branch_returns_none_when_path_absent() { + // Dev/CI: no staged resource next to the exe -> the bundled branch must + // yield None so the resolver falls through instead of returning a + // non-existent path that would fail at spawn. + let dir = tempdir().expect("tempdir"); + assert!(bundled_bash(dir.path()).is_none()); + } + + #[test] + fn bundled_branch_returns_absolute_path_when_staged() { + // A staged PortableGit bash runtime next to the exe resolves to the absolute bash path. + let dir = tempdir().expect("tempdir"); + let bash = dir.path().join(BUNDLED_BASH_REL); + touch(&bash); + let resolved = bundled_bash(dir.path()).expect("bundled bash"); + assert!(resolved.is_absolute()); + assert_eq!(resolved, bash); + } + + #[test] + fn path_scan_skips_system32_and_returns_absolute() { + // A bash.exe under %SystemRoot% (where WSL's launcher lives) must be + // skipped; a bash.exe elsewhere on PATH is returned as an absolute path. + let sys_root = tempdir().expect("sysroot"); + let real = tempdir().expect("real"); + touch(&sys_root.path().join("System32").join("bash.exe")); + let real_bash = real.path().join("bash.exe"); + touch(&real_bash); + + let path_env = + env::join_paths([sys_root.path().join("System32"), real.path().to_path_buf()]) + .expect("join"); + + let found = scan_path_for_bash( + path_env.to_str().expect("utf8"), + Some(sys_root.path().to_path_buf()), + ) + .expect("bash found outside System32"); + assert!(found.is_absolute()); + assert!(!found.starts_with(sys_root.path())); + assert_eq!(found, real_bash); + } + + #[test] + fn path_scan_returns_none_when_only_system32_has_bash() { + // If the ONLY bash.exe on PATH is under System32, the scan finds nothing. + let sys_root = tempdir().expect("sysroot"); + touch(&sys_root.path().join("System32").join("bash.exe")); + let path_env = env::join_paths([sys_root.path().join("System32")]).expect("join"); + + let found = scan_path_for_bash( + path_env.to_str().expect("utf8"), + Some(sys_root.path().to_path_buf()), + ); + assert!(found.is_none()); + } + + #[test] + fn path_scan_skips_system32_when_path_case_differs_from_root() { + // Windows paths are case-insensitive; a PATH entry spelled differently from + // %SystemRoot% (e.g. `...\WINDOWS\System32` vs root `...\Windows`) must STILL + // be excluded, or WSL's bash.exe leaks through. Build the System32 dir under a + // genuinely upper-cased sibling component so the exclusion can only pass via a + // case-insensitive compare, not a literal `starts_with`. + let base = tempdir().expect("base"); + let root = base.path().join("Windows"); + let upper = base.path().join("WINDOWS"); + let sys32 = upper.join("System32"); + touch(&sys32.join("bash.exe")); + + let path_env = env::join_paths([sys32]).expect("join"); + let found = scan_path_for_bash(path_env.to_str().expect("utf8"), Some(root)); + assert!( + found.is_none(), + "case-divergent System32 must still be excluded" + ); + } +} diff --git a/desktop/scripts/build-release-config.mjs b/desktop/scripts/build-release-config.mjs index 389d18aec5..1eea02ad05 100644 --- a/desktop/scripts/build-release-config.mjs +++ b/desktop/scripts/build-release-config.mjs @@ -52,6 +52,27 @@ const releaseConfig = { }, }; +// Windows-only: bundle the PortableGit bash runtime as a resource so the MCP shell +// tool always has a genuine, non-WSL bash to spawn on a bare host (the app must +// be self-contained — we cannot assume Git for Windows is installed). +// +// This is emitted ONLY on the Windows runner because the static tauri.conf.json +// uses `targets: "all"` with a shared bundle block — a bare `resources` entry +// there would ship the ~184MB tree into the macOS .dmg and Linux packages too. +// The release build runs THIS generator on each platform's own runner and merges +// the output via --config, so guarding on process.platform keeps the tree off +// mac/Linux. +// +// PATH CONTRACT (keep byte-identical across three files): +// - source `binaries/git-bash` (relative to src-tauri/) is staged by +// scripts/bundle-sidecars.sh. +// - target `git-bash` is the install-root subdir; Tauri's Windows installer +// stages it next to the exe, and crates/buzz-dev-mcp/src/shell.rs resolves +// `git-bash\bin\bash.exe` relative to its own executable at runtime. +if (process.platform === "win32") { + releaseConfig.bundle.resources = { "binaries/git-bash": "git-bash" }; +} + console.log(`Updater enabled -> ${updaterEndpoint}`); writeFileSync(outputConfigPath, `${JSON.stringify(releaseConfig, null, 2)}\n`); diff --git a/scripts/bundle-sidecars.sh b/scripts/bundle-sidecars.sh index be37cbce0d..3db194a13a 100755 --- a/scripts/bundle-sidecars.sh +++ b/scripts/bundle-sidecars.sh @@ -38,3 +38,12 @@ for bin in "${SIDECARS[@]}"; do cp "$SRC_DIR/${bin}${EXE}" "$BINARIES_DIR/${bin}-${TARGET}${EXE}" done echo "Sidecars bundled for $TARGET" + +# Windows-only: stage a genuine, non-WSL bash next to the sidecars so the MCP +# shell tool works on a bare host. The download/extract/drop logic lives in a +# self-contained script (no release-binary precondition) so CI can call it +# directly to exercise this path on a real Windows runner — see +# scripts/stage-windows-bash.sh for the full rationale and the PATH CONTRACT. +if [[ "$TARGET" == *windows* ]]; then + "$(dirname "$0")/stage-windows-bash.sh" "$BINARIES_DIR/git-bash" +fi diff --git a/scripts/stage-windows-bash.sh b/scripts/stage-windows-bash.sh new file mode 100755 index 0000000000..6d24b12540 --- /dev/null +++ b/scripts/stage-windows-bash.sh @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Stage a genuine, non-WSL bash for the Windows MCP shell tool. The app is +# self-contained — we cannot assume Git for Windows is installed — so we bundle +# bash rather than probing for an install. +# +# There is no standalone "bash for Windows" upstream: working Windows bash ships +# only inside git-for-windows. We download PortableGit and keep ONLY the MSYS2 +# bash runtime (`usr/` + `bin/`), dropping the `mingw64/` git-program subtree as +# one separable unit (~200MB of git.exe etc. the MCP shell never invokes). We do +# NOT trim INSIDE `usr/`: bash loads `msys-2.0.dll` and other libraries lazily, +# and load-bearing pieces (terminfo, gawk libs) live alongside the docs there, so +# a hand-trimmed copy can pass an existence check yet fail mid-command with a +# cryptic error — exactly the bug class this fixes. The retained runtime is the +# untouched, complete closure git-for-windows maintains. +# +# Self-contained (no release-binary precondition) so CI can call it directly to +# exercise the download/extract/drop path on a real Windows runner — the only +# automated gate on this logic before it ships to users. +# +# Single arg: the destination dir for the staged tree (bash lands at +# /bin/bash.exe). Idempotent: skips the download if already staged. +# +# PATH CONTRACT (keep byte-identical across three files): +# - dest `git-bash` (== desktop/src-tauri/binaries/git-bash) is the +# `bundle.resources` SOURCE in desktop/scripts/build-release-config.mjs. +# - that resource's TARGET `git-bash` is staged next to the exe by Tauri's +# Windows installer, and crates/buzz-dev-mcp/src/shell.rs resolves +# `git-bash\bin\bash.exe` relative to its own executable at runtime. + +GIT_BASH_DIR=${1:?usage: stage-windows-bash.sh } +PORTABLEGIT_VERSION="2.54.0" +PORTABLEGIT_TAG="v${PORTABLEGIT_VERSION}.windows.1" +PORTABLEGIT_EXE="PortableGit-${PORTABLEGIT_VERSION}-64-bit.7z.exe" +PORTABLEGIT_URL="https://github.com/git-for-windows/git/releases/download/${PORTABLEGIT_TAG}/${PORTABLEGIT_EXE}" + +if [[ -f "$GIT_BASH_DIR/bin/bash.exe" ]]; then + echo "PortableGit bash already staged at $GIT_BASH_DIR" + exit 0 +fi + +echo "Downloading PortableGit ${PORTABLEGIT_VERSION}..." +tmp_dir=$(mktemp -d -t portablegit.XXXXXX) +trap 'rm -rf "$tmp_dir"' EXIT +tmp_sfx="$tmp_dir/portablegit.7z.exe" +extract_dir="$tmp_dir/extract" +curl -fsSL "$PORTABLEGIT_URL" -o "$tmp_sfx" +# PortableGit is a 7-Zip self-extracting archive; -o/-y are its SFX flags, +# so we don't need a separate 7z on PATH. +chmod +x "$tmp_sfx" +"$tmp_sfx" -y "-o$extract_dir" + +# Keep the bash runtime whole, drop the separable git-program subtree. +rm -rf "$extract_dir/mingw64" +rm -rf "$GIT_BASH_DIR" +mkdir -p "$GIT_BASH_DIR" +cp -a "$extract_dir/." "$GIT_BASH_DIR/" + +rm -rf "$tmp_dir" +trap - EXIT +[[ -f "$GIT_BASH_DIR/bin/bash.exe" ]] || { + echo "Error: PortableGit extracted but $GIT_BASH_DIR/bin/bash.exe is missing" >&2 + exit 1 +} +echo "PortableGit bash staged at $GIT_BASH_DIR (mingw64/ dropped)" From acfddd9173c10b0c01c1323abf625c45c8ddec2a Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 18 Jun 2026 16:09:52 -0400 Subject: [PATCH 2/8] fix(buzz-dev-mcp): gate bash-stage skip on a completion marker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The idempotency skip trusted a single file (bin/bash.exe) as proof of a whole stage. A cp -a interrupted between the sibling bin/ and usr/ subtrees can leave bash.exe present while usr/ is incomplete, and the skip would then accept that partial runtime and ship it. Unreachable in CI (fresh $RUNNER_TEMP) and release (fresh checkout), so not a shipping defect — eliminated proactively. A .stage-complete marker written last, only after cp -a and the bash.exe integrity check both pass, is positive proof the whole tree landed and is immune to which specific files a future partial leaves behind. An interrupted stage never writes it, so the skip falls through to a clean re-extract (the pre-extract rm -rf already cleans the partial). Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- scripts/stage-windows-bash.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/scripts/stage-windows-bash.sh b/scripts/stage-windows-bash.sh index 6d24b12540..35aeac76c3 100755 --- a/scripts/stage-windows-bash.sh +++ b/scripts/stage-windows-bash.sh @@ -20,7 +20,9 @@ set -euo pipefail # automated gate on this logic before it ships to users. # # Single arg: the destination dir for the staged tree (bash lands at -# /bin/bash.exe). Idempotent: skips the download if already staged. +# /bin/bash.exe). Idempotent: a `.stage-complete` marker, written last, +# proves a whole prior stage and skips the re-download; a partial stage lacks it +# and re-extracts cleanly. # # PATH CONTRACT (keep byte-identical across three files): # - dest `git-bash` (== desktop/src-tauri/binaries/git-bash) is the @@ -35,7 +37,8 @@ PORTABLEGIT_TAG="v${PORTABLEGIT_VERSION}.windows.1" PORTABLEGIT_EXE="PortableGit-${PORTABLEGIT_VERSION}-64-bit.7z.exe" PORTABLEGIT_URL="https://github.com/git-for-windows/git/releases/download/${PORTABLEGIT_TAG}/${PORTABLEGIT_EXE}" -if [[ -f "$GIT_BASH_DIR/bin/bash.exe" ]]; then +STAGE_MARKER="$GIT_BASH_DIR/.stage-complete" +if [[ -f "$STAGE_MARKER" ]]; then echo "PortableGit bash already staged at $GIT_BASH_DIR" exit 0 fi @@ -63,4 +66,8 @@ trap - EXIT echo "Error: PortableGit extracted but $GIT_BASH_DIR/bin/bash.exe is missing" >&2 exit 1 } +# Written last, only after cp -a and the integrity check both succeed, so it is +# positive proof the whole tree landed. An interrupted stage never writes it, so +# the idempotency skip falls through to a clean re-extract. +touch "$STAGE_MARKER" echo "PortableGit bash staged at $GIT_BASH_DIR (mingw64/ dropped)" From 5aa3e62147db51ab3025261fa1f3074fc712d004 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 18 Jun 2026 16:18:34 -0400 Subject: [PATCH 3/8] fix(buzz-dev-mcp): satisfy doc_lazy_continuation on the bash resolver docs rust-1.95.0 clippy on the Windows toolchain flags two doc comments in shell.rs where a numbered list is immediately followed by a left-margin prose line, parsing the prose as a malformed lazy continuation of the last list item. Under -D warnings this failed the Windows Rust job at the Clippy step, skipping Check/Test/the bundled-bash smoke gate. A blank /// line between each list and its trailing paragraph is the standard markdown separation; doc-only, no behavior change. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-dev-mcp/src/shell.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/buzz-dev-mcp/src/shell.rs b/crates/buzz-dev-mcp/src/shell.rs index 54a27e85ec..a81bc50d99 100644 --- a/crates/buzz-dev-mcp/src/shell.rs +++ b/crates/buzz-dev-mcp/src/shell.rs @@ -325,6 +325,7 @@ pub async fn run( /// `bundle.resources` Map `{ "binaries/git-bash": "git-bash" }`, whose TARGET /// (`git-bash`) is what Tauri's NSIS/MSI installer stages next to the exe. /// 3. this resolver — joins `current_exe().parent()` + `git-bash\bin\bash.exe`. +/// /// Drift between (2)'s target and this string ships a working bundle but a broken /// runtime path. Keep all three in lockstep. #[cfg(windows)] @@ -348,6 +349,7 @@ fn resolve_bash(_path_env: &str) -> Result { /// 3. The bundled bash staged next to our exe (guaranteed target — this /// is what makes a bare, Git-less host work since the app is self-contained). /// 4. PATH scan, EXCLUDING System32 (so we never resolve WSL's `bash.exe`). +/// /// No bash found -> actionable error returned BEFORE spawn. #[cfg(windows)] fn resolve_bash(path_env: &str) -> Result { From 87ba2febe677b38545c9c662e1948f579224de14 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 18 Jun 2026 16:46:54 -0400 Subject: [PATCH 4/8] test(buzz-dev-mcp): make out-of-workspace path tests cross-platform The three 'absolute path outside workspace is allowed' tests hardcoded /etc/hosts as the out-of-workspace fixture. On Windows that resolves to C:\etc\hosts, which does not exist, so the assertions got a path-not-found error (os error 3) instead of the intended outcome. Each test now writes a real file into a second tempdir outside the workspace root, giving a genuinely-absolute existing path on both platforms. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-dev-mcp/src/read_file.rs | 11 +++++++++-- crates/buzz-dev-mcp/src/str_replace.rs | 13 +++++++++---- crates/buzz-dev-mcp/src/view_image.rs | 11 ++++++++--- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/crates/buzz-dev-mcp/src/read_file.rs b/crates/buzz-dev-mcp/src/read_file.rs index b75867b61e..c9233390bc 100644 --- a/crates/buzz-dev-mcp/src/read_file.rs +++ b/crates/buzz-dev-mcp/src/read_file.rs @@ -143,9 +143,16 @@ mod tests { #[test] fn read_allows_absolute_path() { let dir = tempdir().expect("tempdir"); + // A real file in a SECOND tempdir, genuinely outside the workspace + // root — proves absolute paths beyond workdir resolve, without a + // Unix-only system path like /etc/hosts (which is C:\etc\hosts on + // Windows and does not exist). + let outside = tempdir().expect("tempdir"); + let target = outside.path().join("outside.txt"); + fs::write(&target, b"localhost").expect("write"); let state = make_state(dir.path()); let p = ReadFileParams { - path: "/etc/hosts".into(), + path: target.display().to_string(), offset: None, limit: None, workdir: Some(dir.path().display().to_string()), @@ -153,7 +160,7 @@ mod tests { let out = run(&state, p).expect("ok"); assert!( out.contains("localhost"), - "expected /etc/hosts content, got: {out}" + "expected out-of-workspace file content, got: {out}" ); } diff --git a/crates/buzz-dev-mcp/src/str_replace.rs b/crates/buzz-dev-mcp/src/str_replace.rs index 7feb0aa10d..cffd65f909 100644 --- a/crates/buzz-dev-mcp/src/str_replace.rs +++ b/crates/buzz-dev-mcp/src/str_replace.rs @@ -257,12 +257,17 @@ mod tests { #[test] fn run_allows_path_outside_workspace() { let dir = tempdir().expect("tempdir"); + // A real file in a SECOND tempdir, genuinely outside the workspace + // root, that does NOT contain our old_str — we expect a "not found" + // error (proving the path resolved), not a path-escape error. Avoids + // the Unix-only /etc/hosts assumption (C:\etc\hosts does not exist). + let outside = tempdir().expect("tempdir"); + let target = outside.path().join("outside.txt"); + fs::write(&target, b"some content").expect("write"); let state = make_state(dir.path()); - // /etc/hosts is readable but won't contain our old_str — we expect - // a "not found" error, not a path-escape error. let p = StrReplaceParams { - path: "/etc/hosts".into(), - old_str: "UNIQUE_STRING_NOT_IN_HOSTS_FILE_abc123".into(), + path: target.display().to_string(), + old_str: "UNIQUE_STRING_NOT_IN_FILE_abc123".into(), new_str: "y".into(), replace_all: false, workdir: Some(dir.path().display().to_string()), diff --git a/crates/buzz-dev-mcp/src/view_image.rs b/crates/buzz-dev-mcp/src/view_image.rs index 5a623b81e0..e7c1679d5a 100644 --- a/crates/buzz-dev-mcp/src/view_image.rs +++ b/crates/buzz-dev-mcp/src/view_image.rs @@ -687,13 +687,18 @@ mod tests { #[tokio::test] async fn allows_path_outside_workspace() { let dir = tempdir().unwrap(); + // A real non-image file in a SECOND tempdir, genuinely outside the + // workspace root — we expect a format error, not a path-escape error, + // proving the traversal limit is gone. Avoids the Unix-only /etc/hosts + // assumption (C:\etc\hosts does not exist on Windows). + let outside = tempdir().unwrap(); + let target = outside.path().join("outside.txt"); + fs::write(&target, b"not an image").unwrap(); let state = make_state(dir.path()); - // /etc/hosts exists but is not an image — we expect a format error, - // not a path-escape error, proving the traversal limit is gone. let res = run( &state, ViewImageParams { - source: "/etc/hosts".into(), + source: target.display().to_string(), max_dim: None, workdir: Some(dir.path().display().to_string()), }, From 84a5bc06cc7e3e0c0b7023010c44730d7a669a9e Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 18 Jun 2026 17:33:23 -0400 Subject: [PATCH 5/8] fix(windows): spawn bundled MSYS2 bash directly, not the launcher shim The bundled-bash resolver and CI smoke-step targeted git-bash\bin\bash.exe, which is git-for-windows's 47KB compat launcher shim, not bash. The shim validates its install root by requiring a sibling mingw64\bin marker that Option 2 deliberately drops, so it printed "Top-level not found" and exited before running any command. Retarget the bundled spawn to the real MSYS2 bash at git-bash\usr\bin\bash.exe, which boots from its co-located msys-2.0.dll and needs only usr/. The installed-Git branch stays on bin\bash.exe: a real Git-for-Windows install has mingw64/, so its launcher is the correct entry. Also swap the smoke probe's `rev` (not bundled in PortableGit at all) for `tr a-z A-Z`, which is bundled in usr/bin/ with the same msys-2.0.dll closure, and assert the file we actually run (usr/bin/bash.exe) in the staging integrity check rather than the shim. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .github/workflows/ci.yml | 4 ++-- crates/buzz-dev-mcp/src/shell.rs | 12 ++++++++++-- scripts/stage-windows-bash.sh | 10 +++++----- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6dc477a0d8..59381459dc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -705,8 +705,8 @@ jobs: set -euo pipefail stage_dir="$RUNNER_TEMP/git-bash" scripts/stage-windows-bash.sh "$stage_dir" - out=$("$stage_dir/bin/bash.exe" -c 'echo hello | rev') - [[ "$out" == "olleh" ]] || { echo "staged bash pipeline failed: got '$out'" >&2; exit 1; } + out=$("$stage_dir/usr/bin/bash.exe" -c 'echo hello | tr a-z A-Z') + [[ "$out" == "HELLO" ]] || { echo "staged bash pipeline failed: got '$out'" >&2; exit 1; } echo "staged bash spawned and ran a coreutils pipeline" - name: Check (Tauri crate) run: cargo check --manifest-path desktop/src-tauri/Cargo.toml --target $env:TARGET diff --git a/crates/buzz-dev-mcp/src/shell.rs b/crates/buzz-dev-mcp/src/shell.rs index a81bc50d99..2aa0cbf5b1 100644 --- a/crates/buzz-dev-mcp/src/shell.rs +++ b/crates/buzz-dev-mcp/src/shell.rs @@ -324,12 +324,20 @@ pub async fn run( /// 2. `desktop/scripts/build-release-config.mjs` — emits the Windows-only /// `bundle.resources` Map `{ "binaries/git-bash": "git-bash" }`, whose TARGET /// (`git-bash`) is what Tauri's NSIS/MSI installer stages next to the exe. -/// 3. this resolver — joins `current_exe().parent()` + `git-bash\bin\bash.exe`. +/// 3. this resolver — joins `current_exe().parent()` + `git-bash\usr\bin\bash.exe`. /// /// Drift between (2)'s target and this string ships a working bundle but a broken /// runtime path. Keep all three in lockstep. +/// +/// The bundled constant points at `usr\bin\bash.exe` (the real MSYS2 bash, which +/// boots from its co-located `msys-2.0.dll` and needs only `usr/`, no `mingw64/`) +/// — NOT the `bin\bash.exe` launcher shim, which refuses to start without a sibling +/// `mingw64\bin` marker the bundle deliberately drops. The installed-Git branch +/// below stays on `bin\bash.exe` on purpose: a real Git-for-Windows install has +/// `mingw64/`, so its launcher is the correct entry and sets up MSYSTEM/PATH. +/// Different tree shape -> different correct entry point. #[cfg(windows)] -const BUNDLED_BASH_REL: &str = r"git-bash\bin\bash.exe"; +const BUNDLED_BASH_REL: &str = r"git-bash\usr\bin\bash.exe"; /// Resolve a genuine, non-WSL bash to an absolute path so we spawn it directly /// instead of letting `Command::new("bash")` re-enter PATH search — on Windows diff --git a/scripts/stage-windows-bash.sh b/scripts/stage-windows-bash.sh index 35aeac76c3..ece5d08bfb 100755 --- a/scripts/stage-windows-bash.sh +++ b/scripts/stage-windows-bash.sh @@ -19,8 +19,8 @@ set -euo pipefail # exercise the download/extract/drop path on a real Windows runner — the only # automated gate on this logic before it ships to users. # -# Single arg: the destination dir for the staged tree (bash lands at -# /bin/bash.exe). Idempotent: a `.stage-complete` marker, written last, +# Single arg: the destination dir for the staged tree (the real MSYS2 bash lands +# at /usr/bin/bash.exe). Idempotent: a `.stage-complete` marker, written last, # proves a whole prior stage and skips the re-download; a partial stage lacks it # and re-extracts cleanly. # @@ -29,7 +29,7 @@ set -euo pipefail # `bundle.resources` SOURCE in desktop/scripts/build-release-config.mjs. # - that resource's TARGET `git-bash` is staged next to the exe by Tauri's # Windows installer, and crates/buzz-dev-mcp/src/shell.rs resolves -# `git-bash\bin\bash.exe` relative to its own executable at runtime. +# `git-bash\usr\bin\bash.exe` relative to its own executable at runtime. GIT_BASH_DIR=${1:?usage: stage-windows-bash.sh } PORTABLEGIT_VERSION="2.54.0" @@ -62,8 +62,8 @@ cp -a "$extract_dir/." "$GIT_BASH_DIR/" rm -rf "$tmp_dir" trap - EXIT -[[ -f "$GIT_BASH_DIR/bin/bash.exe" ]] || { - echo "Error: PortableGit extracted but $GIT_BASH_DIR/bin/bash.exe is missing" >&2 +[[ -f "$GIT_BASH_DIR/usr/bin/bash.exe" ]] || { + echo "Error: PortableGit extracted but $GIT_BASH_DIR/usr/bin/bash.exe is missing" >&2 exit 1 } # Written last, only after cp -a and the integrity check both succeed, so it is From 87fa35dd59eed33a282845e5b4ec2434145cbc5b Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 18 Jun 2026 18:39:07 -0400 Subject: [PATCH 6/8] fix(buzz-dev-mcp): kill bash process tree on Windows shell timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The timeout_ms runaway-command guard was a no-op on Windows: the kill path was three cfg(not(unix)) stubs, so on timeout TerminateProcess killed the bash parent but orphaned its MSYS-forked grandchildren (e.g. sleep). The orphans held the stdout/stderr pipes open, blocking the reap until they self-exited — surfacing as shell::tests::timeout_fires eating ~986s of CI, and meaning a real agent's bash -c hitting its timeout was never killed. Replace the PID-keyed guard and free kill functions with a platform- abstracted KillGroup: Unix keeps the killpg behavior byte-for-byte; Windows assigns the child to a Job Object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE so terminating the job kills the whole tree atomically — the structural mirror of killpg. The job HANDLE is held in KillGroup for the whole run so Drop is the last-resort reaper; closing it earlier would fire KILL_ON_JOB_CLOSE and kill the child the instant spawn returned. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- Cargo.lock | 1 + crates/buzz-dev-mcp/Cargo.toml | 9 ++ crates/buzz-dev-mcp/src/shell.rs | 217 +++++++++++++++++++++++++------ 3 files changed, 184 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 075998be85..05ff37be38 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -894,6 +894,7 @@ dependencies = [ "tokio-util", "tracing", "tracing-subscriber", + "windows-sys 0.61.2", "zeroize", ] diff --git a/crates/buzz-dev-mcp/Cargo.toml b/crates/buzz-dev-mcp/Cargo.toml index dd0872b35f..131a97df78 100644 --- a/crates/buzz-dev-mcp/Cargo.toml +++ b/crates/buzz-dev-mcp/Cargo.toml @@ -38,3 +38,12 @@ image = { version = "0.25", default-features = false, features = ["jpeg", "png", [target.'cfg(unix)'.dependencies] nix = { version = "0.31", default-features = false, features = ["signal", "process"] } + +# Windows Job Object APIs for the shell tool's timeout kill path: terminating a +# job kills the bash child AND every MSYS grandchild it forked, the Windows +# analogue of the Unix killpg above. windows-sys 0.61 is already workspace- +# resident (pulled transitively), so this adds no new crate. Win32_Security is +# required because CreateJobObjectW takes a SECURITY_ATTRIBUTES parameter; +# Win32_System_Threading supplies IO_COUNTERS inside the extended-limit struct. +[target.'cfg(windows)'.dependencies] +windows-sys = { version = "0.61", features = ["Win32_Foundation", "Win32_Security", "Win32_System_JobObjects", "Win32_System_Threading"] } diff --git a/crates/buzz-dev-mcp/src/shell.rs b/crates/buzz-dev-mcp/src/shell.rs index 2aa0cbf5b1..f639c5ec28 100644 --- a/crates/buzz-dev-mcp/src/shell.rs +++ b/crates/buzz-dev-mcp/src/shell.rs @@ -174,15 +174,11 @@ pub async fn run( let pid = child.id(); - struct PgidGuard(Option); - impl Drop for PgidGuard { - fn drop(&mut self) { - if let Some(pid) = self.0 { - kill_process_group_immediate(pid as i32); - } - } - } - let mut pgid_guard = PgidGuard(pid); + // KillGroup ties the spawned bash and all its descendants to a single kill + // primitive (Unix process group / Windows Job Object). Built from the live + // child so the Windows job can take the process handle, which only exists + // after spawn. Held for the whole run; its Drop is the last-resort reaper. + let mut kill_group = KillGroup::new(&child, pid); let stdout_pipe = child.stdout.take(); let stderr_pipe = child.stderr.take(); @@ -206,19 +202,17 @@ pub async fn run( biased; _ = ct.cancelled() => { // Kill process group, reap child, abort reader tasks. - if let Some(pid) = pid { - kill_process_group_immediate(pid as i32); - } + kill_group.kill_immediate(); // Bounded reap so we don't leak zombies. If reap times out, - // PgidGuard drop will SIGKILL again as a last resort. + // KillGroup drop will kill again as a last resort. match tokio::time::timeout(Duration::from_secs(1), child.wait()).await { - Ok(Ok(_)) => { pgid_guard.0 = None; } // reaped; disarm guard + Ok(Ok(_)) => { kill_group.disarm(); } // reaped; disarm guard Ok(Err(e)) => { tracing::debug!("cancel: child wait error: {e}"); - // Leave pgid_guard armed for drop-kill. + // Leave kill_group armed for drop-kill. } Err(_) => { - tracing::debug!("cancel: child reap timed out; guard will SIGKILL on drop"); + tracing::debug!("cancel: child reap timed out; guard will kill on drop"); } } stdout_handle.abort(); @@ -233,9 +227,7 @@ pub async fn run( } Err(_) => { // Kill process group — this closes the pipes, causing reads to EOF. - if let Some(pid) = pid { - kill_process_group_graceful(pid as i32).await; - } + kill_group.kill_graceful().await; // Reap the child so it doesn't become a zombie. let deadline = Instant::now() + Duration::from_secs(2); loop { @@ -265,9 +257,7 @@ pub async fn run( }; if !timed_out { - if let Some(pid) = pid { - kill_process_group_graceful(pid as i32).await; - } + kill_group.kill_graceful().await; } let stdout_cap = match tokio::time::timeout(Duration::from_secs(5), &mut stdout_handle).await { @@ -312,7 +302,7 @@ pub async fn run( "notes": notes, }); let text = serde_json::to_string_pretty(&body).unwrap_or_else(|_| "{}".into()); - pgid_guard.0 = None; + kill_group.disarm(); Ok(CallToolResult::success(vec![Content::text(text)])) } @@ -465,31 +455,167 @@ fn set_process_group(cmd: &mut Command) { #[cfg(not(unix))] fn set_process_group(_cmd: &mut Command) {} -/// Immediate SIGKILL of the process group. Sync; safe to call from Drop. -/// No grace period — used when the parent task is being torn down. +/// Kill primitive covering the spawned bash AND every descendant it forks, +/// mirroring the same guarantee across platforms. +/// +/// - Unix: the child's process group (set via [`set_process_group`]); kills go +/// to the whole group via `killpg`. +/// - Windows: a Job Object the child is assigned to at construction. A bare +/// `TerminateProcess` on bash leaves MSYS-forked grandchildren (e.g. `sleep`) +/// running — they hold the stdout/stderr pipes open, so the reap blocks until +/// they self-exit. Terminating the job kills the entire tree atomically. +/// +/// Held for the whole `run`; `Drop` is the last-resort reaper if an explicit +/// kill was skipped or failed. #[cfg(unix)] -fn kill_process_group_immediate(pid: i32) { - use nix::sys::signal::{killpg, Signal}; - use nix::unistd::Pid; - let _ = killpg(Pid::from_raw(pid), Signal::SIGKILL); -} +struct KillGroup(Option); -#[cfg(not(unix))] -fn kill_process_group_immediate(_pid: i32) {} +#[cfg(unix)] +impl KillGroup { + fn new(_child: &tokio::process::Child, pid: Option) -> Self { + Self(pid.map(|p| p as i32)) + } + + /// Immediate SIGKILL of the process group. Sync; safe to call from Drop. + /// No grace period — used when the parent task is being torn down. + fn kill_immediate(&self) { + use nix::sys::signal::{killpg, Signal}; + use nix::unistd::Pid; + if let Some(pid) = self.0 { + let _ = killpg(Pid::from_raw(pid), Signal::SIGKILL); + } + } + + /// Graceful SIGTERM → 200ms async sleep → SIGKILL. Async; never blocks the runtime. + async fn kill_graceful(&self) { + use nix::sys::signal::{killpg, Signal}; + use nix::unistd::Pid; + if let Some(pid) = self.0 { + let pgid = Pid::from_raw(pid); + let _ = killpg(pgid, Signal::SIGTERM); + tokio::time::sleep(Duration::from_millis(200)).await; + let _ = killpg(pgid, Signal::SIGKILL); + } + } + + /// Disarm the Drop-time kill once the child has been reaped explicitly. + fn disarm(&mut self) { + self.0 = None; + } +} -/// Graceful SIGTERM → 200ms async sleep → SIGKILL. Async; never blocks the runtime. #[cfg(unix)] -async fn kill_process_group_graceful(pid: i32) { - use nix::sys::signal::{killpg, Signal}; - use nix::unistd::Pid; - let pgid = Pid::from_raw(pid); - let _ = killpg(pgid, Signal::SIGTERM); - tokio::time::sleep(Duration::from_millis(200)).await; - let _ = killpg(pgid, Signal::SIGKILL); +impl Drop for KillGroup { + fn drop(&mut self) { + self.kill_immediate(); + } } -#[cfg(not(unix))] -async fn kill_process_group_graceful(_pid: i32) {} +#[cfg(windows)] +struct KillGroup { + job: windows_sys::Win32::Foundation::HANDLE, +} + +#[cfg(windows)] +impl KillGroup { + fn new(child: &tokio::process::Child, _pid: Option) -> Self { + use std::mem::{size_of, zeroed}; + use windows_sys::Win32::Foundation::HANDLE; + use windows_sys::Win32::System::JobObjects::{ + AssignProcessToJobObject, CreateJobObjectW, JobObjectExtendedLimitInformation, + SetInformationJobObject, JOBOBJECT_EXTENDED_LIMIT_INFORMATION, + JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }; + + // SAFETY: each call is a documented Win32 FFI call with arguments that + // satisfy its contract — a null SECURITY_ATTRIBUTES/name for an + // anonymous job, a zeroed #[repr(C)] info struct sized by size_of, and + // the live process handle from `child` (valid while it is running). + // A null job HANDLE on failure makes every later call a harmless no-op. + let job = unsafe { + let job: HANDLE = CreateJobObjectW(std::ptr::null(), std::ptr::null()); + if !job.is_null() { + let mut info: JOBOBJECT_EXTENDED_LIMIT_INFORMATION = zeroed(); + // KILL_ON_JOB_CLOSE: when the LAST handle to the job closes, + // Windows kills every process still in it. This is both the + // explicit-kill mechanism and the Drop-time safety net — and the + // reason the job HANDLE must outlive the child (see Drop). + info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + SetInformationJobObject( + job, + JobObjectExtendedLimitInformation, + std::ptr::addr_of!(info).cast(), + size_of::() as u32, + ); + if let Some(handle) = child.raw_handle() { + AssignProcessToJobObject(job, handle as HANDLE); + } + } + job + }; + Self { job } + } + + fn kill_immediate(&self) { + self.terminate(); + } + + async fn kill_graceful(&self) { + // A Job Object has no SIGTERM analogue; termination is atomic, so the + // graceful path is the same single terminate as the immediate path. + self.terminate(); + } + + fn terminate(&self) { + use windows_sys::Win32::System::JobObjects::TerminateJobObject; + if !self.job.is_null() { + // SAFETY: `self.job` is a valid job HANDLE for this struct's + // lifetime; exit code 137 mirrors the SIGKILL (128+9) we report on + // Unix. + unsafe { + TerminateJobObject(self.job, 137); + } + } + } + + /// No-op on Windows: the job is terminated explicitly, and closing the + /// handle on Drop with no live processes left is harmless. Kept for a + /// uniform call shape with the Unix guard. + fn disarm(&mut self) {} +} + +#[cfg(windows)] +impl Drop for KillGroup { + fn drop(&mut self) { + use windows_sys::Win32::Foundation::CloseHandle; + if !self.job.is_null() { + // Closing the last job handle triggers KILL_ON_JOB_CLOSE, killing any + // process still in the job — the last-resort reaper. The handle is + // held until here precisely so this fires no earlier than run end. + // SAFETY: `self.job` is a valid HANDLE created in `new` and closed + // exactly once here. + unsafe { + CloseHandle(self.job); + } + } + } +} + +// Fallback for targets that are neither unix nor windows: no process-tree kill +// primitive is wired up, so timeouts rely on the cross-platform start_kill in +// `run`. Keeps the crate compiling everywhere. +#[cfg(not(any(unix, windows)))] +struct KillGroup; + +#[cfg(not(any(unix, windows)))] +impl KillGroup { + fn new(_child: &tokio::process::Child, _pid: Option) -> Self { + Self + } + fn kill_immediate(&self) {} + async fn kill_graceful(&self) {} + fn disarm(&mut self) {} +} #[derive(Default)] struct CapturedStream { @@ -665,7 +791,12 @@ mod tests { let r = run( &state, ShellParams { - command: "sleep 999".into(), + // Short sleep, not 999: the kill path must actually terminate + // the process tree on timeout. If a regression leaves the child + // (or an MSYS grandchild) orphaned, the test stalls until this + // brief sleep self-exits — ~5s, not ~16min — so the failure + // stays visible instead of hiding behind a 999s sleep. + command: "sleep 5".into(), workdir: None, timeout_ms: Some(150), }, From dbe921d5093af5d1a9594c086e7d8ebb20eee3ed Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 18 Jun 2026 18:48:04 -0400 Subject: [PATCH 7/8] fix(buzz-dev-mcp): make Windows KillGroup Send + Sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shell tool's async future holds a KillGroup across an .await and is spawned as a Send future, but on Windows KillGroup wraps a raw HANDLE (*mut c_void) which is neither Send nor Sync — so the Windows clippy gate failed to compile (5 errors, all from this one missing bound). A job-object handle is a thread-safe kernel reference, so the impls are sound; the Unix path was unaffected and already Send + Sync. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-dev-mcp/src/shell.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/buzz-dev-mcp/src/shell.rs b/crates/buzz-dev-mcp/src/shell.rs index f639c5ec28..2362a80694 100644 --- a/crates/buzz-dev-mcp/src/shell.rs +++ b/crates/buzz-dev-mcp/src/shell.rs @@ -516,6 +516,17 @@ struct KillGroup { job: windows_sys::Win32::Foundation::HANDLE, } +// SAFETY: `job` is a raw Win32 HANDLE (`*mut c_void`), which is neither `Send` +// nor `Sync` by default. The shell tool's async future holds a `KillGroup` +// across an `.await`, so it must be `Send` to be spawned. A job-object handle +// is a kernel object reference, not thread-affine: `TerminateJobObject` and +// `CloseHandle` are thread-safe, and Rust's `&self`/`&mut self` borrows still +// serialize access to the field. Moving or sharing it across threads is sound. +#[cfg(windows)] +unsafe impl Send for KillGroup {} +#[cfg(windows)] +unsafe impl Sync for KillGroup {} + #[cfg(windows)] impl KillGroup { fn new(child: &tokio::process::Child, _pid: Option) -> Self { From 8fc710ebf35e7422d8f6053212b2f00351ccea4e Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 18 Jun 2026 19:14:35 -0400 Subject: [PATCH 8/8] build(buzz-dev-mcp): scope unsafe-code lint to Windows FFI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Job Object kill path is raw windows-sys FFI and is inherently unsafe, but the crate was #![forbid(unsafe_code)] — a hard wall an inner #[allow] cannot relax. Split the lint by target: Unix keeps the hard no-unsafe guarantee (forbid), Windows uses deny so the KillGroup FFI items can carry a scoped #[allow(unsafe_code)]. No safe Win32 API exists for job objects, mirroring the git-sign-nostr unsafe-FFI precedent. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-dev-mcp/src/lib.rs | 3 ++- crates/buzz-dev-mcp/src/shell.rs | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/buzz-dev-mcp/src/lib.rs b/crates/buzz-dev-mcp/src/lib.rs index ab4f6ad4c9..102a1c6551 100644 --- a/crates/buzz-dev-mcp/src/lib.rs +++ b/crates/buzz-dev-mcp/src/lib.rs @@ -1,4 +1,5 @@ -#![forbid(unsafe_code)] +#![cfg_attr(not(windows), forbid(unsafe_code))] +#![cfg_attr(windows, deny(unsafe_code))] use rmcp::{ handler::server::{router::tool::ToolRouter, wrapper::Parameters}, model::{CallToolResult, ServerCapabilities, ServerInfo}, diff --git a/crates/buzz-dev-mcp/src/shell.rs b/crates/buzz-dev-mcp/src/shell.rs index 2362a80694..0b27b160d1 100644 --- a/crates/buzz-dev-mcp/src/shell.rs +++ b/crates/buzz-dev-mcp/src/shell.rs @@ -523,11 +523,14 @@ struct KillGroup { // `CloseHandle` are thread-safe, and Rust's `&self`/`&mut self` borrows still // serialize access to the field. Moving or sharing it across threads is sound. #[cfg(windows)] +#[allow(unsafe_code)] unsafe impl Send for KillGroup {} #[cfg(windows)] +#[allow(unsafe_code)] unsafe impl Sync for KillGroup {} #[cfg(windows)] +#[allow(unsafe_code)] impl KillGroup { fn new(child: &tokio::process::Child, _pid: Option) -> Self { use std::mem::{size_of, zeroed}; @@ -596,6 +599,7 @@ impl KillGroup { } #[cfg(windows)] +#[allow(unsafe_code)] impl Drop for KillGroup { fn drop(&mut self) { use windows_sys::Win32::Foundation::CloseHandle;