diff --git a/crates/buzz-dev-mcp/src/rg.rs b/crates/buzz-dev-mcp/src/rg.rs index 9d5d506d61..3ea9c1f871 100644 --- a/crates/buzz-dev-mcp/src/rg.rs +++ b/crates/buzz-dev-mcp/src/rg.rs @@ -1,7 +1,15 @@ +use std::ffi::{OsStr, OsString}; use std::io::{BufRead, BufReader}; use std::path::{Path, PathBuf}; use std::process::Command; +/// The `rg` filename as it appears on disk for this target. Without the `.exe` +/// suffix the Windows lookup never matched a real binary. +#[cfg(windows)] +const RG_EXE: &str = "rg.exe"; +#[cfg(not(windows))] +const RG_EXE: &str = "rg"; + const MAX_LINE_BYTES: usize = 1024 * 1024; // 1MB per line — skip files with longer lines (likely binary) const MAX_OUTPUT_BYTES: usize = 50 * 1024; const MAX_OUTPUT_LINES: usize = 2000; @@ -28,30 +36,38 @@ fn try_system_rg(args: &[String]) -> Option { Some(status.code().unwrap_or(2)) } -fn clean_path(self_canon: &Path) -> String { - let original = std::env::var("PATH").unwrap_or_default(); - original - .split(':') - .filter(|dir| { - if dir.is_empty() { - return false; - } - let candidate = Path::new(dir).join("rg"); - match std::fs::canonicalize(&candidate) { - Ok(c) => c != *self_canon, - Err(_) => true, - } - }) - .collect::>() - .join(":") +/// `PATH` with any directory holding *this* shim removed, so the spawned `rg` is +/// always the real one and never a recursive call back into us. +/// +/// Entries are split with `env::split_paths` rather than on `':'`: Windows +/// separates with `;` and every entry carries a drive colon, so a `':'` split +/// tore `C:\tools\bin` into `C` and `\tools\bin` and left no usable directory +/// behind — the shim then never found a system `rg` on Windows at all. +/// +/// A `join_paths` failure (an entry containing the separator itself) yields an +/// empty `PATH`, which makes `which_rg` find nothing and drops us to the +/// built-in walker. That is the safe direction: returning the original would put +/// this shim's own directory back in play. +fn clean_path(self_canon: &Path) -> OsString { + let original = std::env::var_os("PATH").unwrap_or_default(); + let kept = std::env::split_paths(&original).filter(|dir| { + if dir.as_os_str().is_empty() { + return false; + } + match std::fs::canonicalize(dir.join(RG_EXE)) { + Ok(canonical) => canonical != *self_canon, + Err(_) => true, + } + }); + std::env::join_paths(kept).unwrap_or_default() } -fn which_rg(path: &str) -> Option { - for dir in path.split(':') { - if dir.is_empty() { +fn which_rg(path: &OsStr) -> Option { + for dir in std::env::split_paths(path) { + if dir.as_os_str().is_empty() { continue; } - let candidate = Path::new(dir).join("rg"); + let candidate = dir.join(RG_EXE); if candidate.is_file() && is_executable(&candidate) { return Some(candidate); } @@ -488,4 +504,61 @@ mod tests { let found = scan_file(&f, "NEEDLE", &opts, &mut sink, &mut printed); assert!(found, "expected NEEDLE to be found"); } + + /// Regression guard for the `':'`-split lookup. Both entries are absolute, so + /// on Windows they carry drive colons — splitting on `':'` mangled every one + /// and `which_rg` returned `None` no matter what was installed. + #[test] + fn which_rg_finds_the_binary_past_an_earlier_path_entry() { + let empty = tempfile::tempdir().expect("tempdir"); + let holding = tempfile::tempdir().expect("tempdir"); + let binary = holding.path().join(RG_EXE); + std::fs::write(&binary, b"#!/bin/sh\n").expect("write"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&binary, std::fs::Permissions::from_mode(0o755)) + .expect("chmod"); + } + + let path = std::env::join_paths([empty.path(), holding.path()]).expect("join_paths"); + + assert_eq!(which_rg(&path).as_deref(), Some(binary.as_path())); + } + + #[test] + fn which_rg_returns_none_when_no_entry_holds_the_binary() { + let empty = tempfile::tempdir().expect("tempdir"); + let path = std::env::join_paths([empty.path()]).expect("join_paths"); + + assert!(which_rg(&path).is_none()); + } + + #[test] + fn which_rg_ignores_empty_path_entries() { + assert!(which_rg(OsStr::new("")).is_none()); + } + + #[test] + fn clean_path_drops_the_directory_holding_this_shim() { + let shim_dir = tempfile::tempdir().expect("tempdir"); + let shim = shim_dir.path().join(RG_EXE); + std::fs::write(&shim, b"shim").expect("write"); + let shim_canon = std::fs::canonicalize(&shim).expect("canonicalize"); + + // `clean_path` reads the process PATH, so assert on the predicate it + // applies rather than mutating global env from a test thread. + let kept: Vec = + std::env::split_paths(&std::env::join_paths([shim_dir.path()]).expect("join_paths")) + .filter(|dir| match std::fs::canonicalize(dir.join(RG_EXE)) { + Ok(canonical) => canonical != shim_canon, + Err(_) => true, + }) + .collect(); + + assert!( + kept.is_empty(), + "the shim's own directory must be filtered out: {kept:?}" + ); + } }