From f43051ac7480f9a4ae39a1e964c83c2e8179b42c Mon Sep 17 00:00:00 2001 From: Kalvin Chau Date: Mon, 18 May 2026 10:50:32 -0700 Subject: [PATCH 1/5] fix(doctor): harden shell path resolution use shell path-only lookup commands instead of plain which and validate resolved candidates as absolute executable files. apply the same executable validation to shared cli resolvers so function or alias output cannot be treated as a binary path. --- crates/acp-client/README.md | 3 +- crates/acp-client/src/types.rs | 65 ++++++++++--- crates/blox-cli/src/lib.rs | 65 ++++++++++--- crates/doctor/src/resolve.rs | 173 +++++++++++++++++++++++++++++---- 4 files changed, 258 insertions(+), 48 deletions(-) diff --git a/crates/acp-client/README.md b/crates/acp-client/README.md index 900b41fb2..8768c6c60 100644 --- a/crates/acp-client/README.md +++ b/crates/acp-client/README.md @@ -99,8 +99,7 @@ if let Some(agent) = find_acp_agent_by_id("goose") { ## How It Works 1. **Discovery**: Searches for agent executables using: - - Login shell `which` command (to get user's PATH) - - Direct command execution + - Login shell path lookup (to get user's PATH) - Common installation paths (`/opt/homebrew/bin`, `/usr/local/bin`, etc.) 2. **Execution**: diff --git a/crates/acp-client/src/types.rs b/crates/acp-client/src/types.rs index 263b36320..7fe1e43b1 100644 --- a/crates/acp-client/src/types.rs +++ b/crates/acp-client/src/types.rs @@ -171,20 +171,16 @@ const COMMON_PATHS: &[&str] = &[ /// Find a CLI binary by command name. /// /// Searches in order: -/// 1. Login shell `which` (picks up user's PATH from `.zshrc` / `.bashrc`) +/// 1. Login shell path lookup (picks up user's PATH from `.zshrc` / `.bashrc`) /// 2. Common install locations pub fn find_command(cmd: &str) -> Option { - // Strategy 1: Login shell `which` if let Some(path) = find_via_login_shell(cmd) { - if path.exists() { - return Some(path); - } + return Some(path); } - // Strategy 2: Common paths for dir in COMMON_PATHS { let path = PathBuf::from(dir).join(cmd); - if path.exists() { + if is_executable_file(&path) { return Some(path); } } @@ -193,19 +189,22 @@ pub fn find_command(cmd: &str) -> Option { } fn find_via_login_shell(cmd: &str) -> Option { - let which_cmd = format!("which {cmd}"); + let quoted = shell_quote(cmd); + let lookups = [ + ("/bin/zsh", format!("whence -p -- {quoted}")), + ("/bin/bash", format!("type -P -- {quoted}")), + ]; - for shell in &["/bin/zsh", "/bin/bash"] { + for (shell, lookup_cmd) in lookups { if let Ok(output) = std::process::Command::new(shell) - .args(["-l", "-c", &which_cmd]) + .args(["-l", "-c", &lookup_cmd]) .output() { if output.status.success() { let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path_str) = stdout.lines().rfind(|l| !l.is_empty()) { - let path_str = path_str.trim(); - if !path_str.is_empty() && path_str.starts_with('/') { - return Some(PathBuf::from(path_str)); + if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + if is_executable_file(&path) { + return Some(path); } } } @@ -214,6 +213,44 @@ fn find_via_login_shell(cmd: &str) -> Option { None } +fn shell_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) +} + +fn candidate_from_shell_output(output: &str) -> Option { + let mut lines = output + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()); + let candidate = lines.next()?; + if lines.next().is_some() { + return None; + } + + let path = PathBuf::from(candidate); + path.is_absolute().then_some(path) +} + +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = path.metadata() else { + return false; + }; + if !metadata.is_file() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + /// Map an agent ID to the `--command` value for `blox acp`. /// /// Returns `None` if the agent uses the workspace default (no flag needed). diff --git a/crates/blox-cli/src/lib.rs b/crates/blox-cli/src/lib.rs index 1ba1d7e2d..dcaeea86c 100644 --- a/crates/blox-cli/src/lib.rs +++ b/crates/blox-cli/src/lib.rs @@ -4,7 +4,7 @@ use serde::{Deserialize, Deserializer, Serialize}; use std::io::Read; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::time::{Duration, Instant}; use thiserror::Error; @@ -118,18 +118,16 @@ where /// Find a CLI binary by command name. /// /// Searches in order: -/// 1. Login shell `which` (picks up user's PATH from shell rc files) +/// 1. Login shell path lookup (picks up user's PATH from shell rc files) /// 2. Common install locations pub fn find_command(cmd: &str) -> Option { if let Some(path) = find_via_login_shell(cmd) { - if path.exists() { - return Some(path); - } + return Some(path); } for dir in COMMON_PATHS { let path = PathBuf::from(dir).join(cmd); - if path.exists() { + if is_executable_file(&path) { return Some(path); } } @@ -138,16 +136,19 @@ pub fn find_command(cmd: &str) -> Option { } fn find_via_login_shell(cmd: &str) -> Option { - let which_cmd = format!("which {cmd}"); + let quoted = shell_quote(cmd); + let lookups = [ + ("/bin/zsh", format!("whence -p -- {quoted}")), + ("/bin/bash", format!("type -P -- {quoted}")), + ]; - for shell in ["/bin/zsh", "/bin/bash"] { - if let Ok(output) = Command::new(shell).args(["-l", "-c", &which_cmd]).output() { + for (shell, lookup_cmd) in lookups { + if let Ok(output) = Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() { if output.status.success() { let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path_str) = stdout.lines().rfind(|line| !line.is_empty()) { - let path_str = path_str.trim(); - if !path_str.is_empty() && path_str.starts_with('/') { - return Some(PathBuf::from(path_str)); + if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + if is_executable_file(&path) { + return Some(path); } } } @@ -157,6 +158,44 @@ fn find_via_login_shell(cmd: &str) -> Option { None } +fn shell_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) +} + +fn candidate_from_shell_output(output: &str) -> Option { + let mut lines = output + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()); + let candidate = lines.next()?; + if lines.next().is_some() { + return None; + } + + let path = PathBuf::from(candidate); + path.is_absolute().then_some(path) +} + +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = path.metadata() else { + return false; + }; + if !metadata.is_file() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + /// Locate the `sq` binary. pub fn find_sq_binary() -> Option { find_command("sq") diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index cc495e0a6..d2becd7b5 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -1,34 +1,54 @@ //! Binary resolution and command output formatting helpers. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::Command; use super::types::ResolvedBinary; -/// Resolve a binary by trying login shell `which` then common install paths. +/// Resolve a binary by trying login shell path lookup then common install paths. pub fn resolve_binary(cmd: &str) -> ResolvedBinary { let mut lines = vec![format!("resolve '{cmd}':")]; - // Strategy 1: Login shell `which` (primary) - lines.push(" strategy 1 — login shell `which`:".to_string()); - for shell in &["/bin/zsh", "/bin/bash"] { - let which_cmd = format!("which {cmd}"); - match Command::new(shell).args(["-l", "-c", &which_cmd]).output() { + // Strategy 1: Login shell path lookup (primary) + lines.push(" strategy 1 — login shell path lookup:".to_string()); + for (shell, lookup_cmd) in shell_lookup_commands(cmd) { + match Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() { Ok(output) => { - let result = String::from_utf8_lossy(&output.stdout).trim().to_string(); - if output.status.success() && !result.is_empty() { - lines.push(format!( - " {shell} -l -c 'which {cmd}' => {result} (resolved)" - )); - return ResolvedBinary { - path: Some(PathBuf::from(&result)), - search_output: lines.join("\n"), - }; + let stdout = String::from_utf8_lossy(&output.stdout); + if output.status.success() { + match candidate_from_shell_output(stdout.as_ref()) { + Some(path) if is_executable_file(&path) => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (resolved)", + path.display() + )); + return ResolvedBinary { + path: Some(path), + search_output: lines.join("\n"), + }; + } + Some(path) => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an executable file)", + path.display() + )); + } + None if stdout.trim().is_empty() => { + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); + } + None => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an absolute path)", + summarize_output(stdout.as_ref()) + )); + } + } + } else { + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); } - lines.push(format!(" {shell} -l -c 'which {cmd}' => not found")); } Err(e) => { - lines.push(format!(" {shell} -l -c 'which {cmd}' => error: {e}")); + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => error: {e}")); } } } @@ -42,7 +62,7 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { "/home/linuxbrew/.linuxbrew/bin", ] { let path = PathBuf::from(dir).join(cmd); - if path.exists() { + if is_executable_file(&path) { lines.push(format!(" {} => found (resolved)", path.display())); return ResolvedBinary { path: Some(path), @@ -59,6 +79,61 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { } } +fn shell_lookup_commands(cmd: &str) -> Vec<(&'static str, String)> { + let quoted = shell_quote(cmd); + vec![ + ("/bin/zsh", format!("whence -p -- {quoted}")), + ("/bin/bash", format!("type -P -- {quoted}")), + ] +} + +fn shell_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) +} + +fn candidate_from_shell_output(output: &str) -> Option { + let mut lines = output + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()); + let candidate = lines.next()?; + if lines.next().is_some() { + return None; + } + + let path = PathBuf::from(candidate); + path.is_absolute().then_some(path) +} + +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = path.metadata() else { + return false; + }; + if !metadata.is_file() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + +fn summarize_output(output: &str) -> String { + let trimmed = output.trim(); + const MAX_LEN: usize = 120; + if trimmed.len() <= MAX_LEN { + return trimmed.replace('\n', "\\n"); + } + format!("{}...", trimmed[..MAX_LEN].replace('\n', "\\n")) +} + /// Format the raw output of a command invocation for debug diagnostics. pub fn format_command_output(cmd_desc: &str, output: &std::process::Output) -> String { let stdout = String::from_utf8_lossy(&output.stdout); @@ -72,3 +147,63 @@ pub fn format_command_output(cmd_desc: &str, output: &std::process::Output) -> S } raw } + +#[cfg(test)] +mod tests { + use super::{candidate_from_shell_output, is_executable_file, shell_quote}; + use std::fs::{self, File}; + use std::path::PathBuf; + + #[test] + fn candidate_accepts_single_absolute_path() { + assert_eq!( + candidate_from_shell_output("/opt/homebrew/bin/git\n"), + Some(PathBuf::from("/opt/homebrew/bin/git")) + ); + } + + #[test] + fn candidate_rejects_function_body_output() { + let output = "git () {\n\tcommand git \"$@\"\n}\n"; + assert_eq!(candidate_from_shell_output(output), None); + } + + #[test] + fn candidate_rejects_relative_or_command_name_output() { + assert_eq!(candidate_from_shell_output("git\n"), None); + } + + #[test] + fn shell_quote_handles_single_quotes() { + assert_eq!(shell_quote("git'bad"), "'git'\\''bad'"); + } + + #[test] + fn executable_file_validation_checks_file_and_mode() { + let dir = std::env::temp_dir().join(format!("doctor-resolve-test-{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + + let executable = dir.join("tool"); + File::create(&executable).unwrap(); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + fs::set_permissions(&executable, fs::Permissions::from_mode(0o644)).unwrap(); + assert!(!is_executable_file(&executable)); + + fs::set_permissions(&executable, fs::Permissions::from_mode(0o755)).unwrap(); + assert!(is_executable_file(&executable)); + } + + #[cfg(not(unix))] + { + assert!(is_executable_file(&executable)); + } + + assert!(!is_executable_file(&dir)); + let _ = fs::remove_dir_all(&dir); + } +} From 5eaf5c790537f40886f061d46e6481a4d6d9139a Mon Sep 17 00:00:00 2001 From: Kalvin Chau Date: Mon, 18 May 2026 10:53:28 -0700 Subject: [PATCH 2/5] refactor(doctor): simplify resolver control flow flatten login shell resolution branches and avoid an intermediate allocation when building lookup commands. preserve executable path validation while making output summarization safe for non-ascii command output. --- crates/acp-client/src/types.rs | 21 ++++++----- crates/blox-cli/src/lib.rs | 19 +++++----- crates/doctor/src/resolve.rs | 66 +++++++++++++++++----------------- 3 files changed, 57 insertions(+), 49 deletions(-) diff --git a/crates/acp-client/src/types.rs b/crates/acp-client/src/types.rs index 7fe1e43b1..fdcf5f2b4 100644 --- a/crates/acp-client/src/types.rs +++ b/crates/acp-client/src/types.rs @@ -196,17 +196,20 @@ fn find_via_login_shell(cmd: &str) -> Option { ]; for (shell, lookup_cmd) in lookups { - if let Ok(output) = std::process::Command::new(shell) + let Ok(output) = std::process::Command::new(shell) .args(["-l", "-c", &lookup_cmd]) .output() - { - if output.status.success() { - let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { - if is_executable_file(&path) { - return Some(path); - } - } + else { + continue; + }; + if !output.status.success() { + continue; + } + + let stdout = String::from_utf8_lossy(&output.stdout); + if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + if is_executable_file(&path) { + return Some(path); } } } diff --git a/crates/blox-cli/src/lib.rs b/crates/blox-cli/src/lib.rs index dcaeea86c..28c781ff8 100644 --- a/crates/blox-cli/src/lib.rs +++ b/crates/blox-cli/src/lib.rs @@ -143,14 +143,17 @@ fn find_via_login_shell(cmd: &str) -> Option { ]; for (shell, lookup_cmd) in lookups { - if let Ok(output) = Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() { - if output.status.success() { - let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { - if is_executable_file(&path) { - return Some(path); - } - } + let Ok(output) = Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() else { + continue; + }; + if !output.status.success() { + continue; + } + + let stdout = String::from_utf8_lossy(&output.stdout); + if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + if is_executable_file(&path) { + return Some(path); } } } diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index d2becd7b5..9fc866d30 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -15,36 +15,37 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { match Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() { Ok(output) => { let stdout = String::from_utf8_lossy(&output.stdout); - if output.status.success() { - match candidate_from_shell_output(stdout.as_ref()) { - Some(path) if is_executable_file(&path) => { - lines.push(format!( - " {shell} -l -c '{lookup_cmd}' => {} (resolved)", - path.display() - )); - return ResolvedBinary { - path: Some(path), - search_output: lines.join("\n"), - }; - } - Some(path) => { - lines.push(format!( - " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an executable file)", - path.display() - )); - } - None if stdout.trim().is_empty() => { - lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); - } - None => { - lines.push(format!( - " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an absolute path)", - summarize_output(stdout.as_ref()) - )); - } - } - } else { + if !output.status.success() { lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); + continue; + } + + match candidate_from_shell_output(stdout.as_ref()) { + Some(path) if is_executable_file(&path) => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (resolved)", + path.display() + )); + return ResolvedBinary { + path: Some(path), + search_output: lines.join("\n"), + }; + } + Some(path) => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an executable file)", + path.display() + )); + } + None if stdout.trim().is_empty() => { + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); + } + None => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an absolute path)", + summarize_output(stdout.as_ref()) + )); + } } } Err(e) => { @@ -79,9 +80,9 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { } } -fn shell_lookup_commands(cmd: &str) -> Vec<(&'static str, String)> { +fn shell_lookup_commands(cmd: &str) -> [(&'static str, String); 2] { let quoted = shell_quote(cmd); - vec![ + [ ("/bin/zsh", format!("whence -p -- {quoted}")), ("/bin/bash", format!("type -P -- {quoted}")), ] @@ -131,7 +132,8 @@ fn summarize_output(output: &str) -> String { if trimmed.len() <= MAX_LEN { return trimmed.replace('\n', "\\n"); } - format!("{}...", trimmed[..MAX_LEN].replace('\n', "\\n")) + let summary: String = trimmed.chars().take(MAX_LEN).collect(); + format!("{}...", summary.replace('\n', "\\n")) } /// Format the raw output of a command invocation for debug diagnostics. From a62055588b354f0610bcfa3c1cffe94ba29214c1 Mon Sep 17 00:00:00 2001 From: Kalvin Chau Date: Wed, 27 May 2026 10:06:48 -0700 Subject: [PATCH 3/5] fix(doctor): tolerate shell startup output scan login shell lookup stdout for absolute executable paths so startup messages do not hide tools installed through the user's shell path. keep relative output and shell function bodies ignored. --- crates/acp-client/src/types.rs | 41 +++++++++++----- crates/blox-cli/src/lib.rs | 33 ++++++++----- crates/doctor/src/resolve.rs | 89 ++++++++++++++++++---------------- 3 files changed, 100 insertions(+), 63 deletions(-) diff --git a/crates/acp-client/src/types.rs b/crates/acp-client/src/types.rs index fdcf5f2b4..9546347a7 100644 --- a/crates/acp-client/src/types.rs +++ b/crates/acp-client/src/types.rs @@ -207,7 +207,7 @@ fn find_via_login_shell(cmd: &str) -> Option { } let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + for path in candidate_paths_from_shell_output(stdout.as_ref()) { if is_executable_file(&path) { return Some(path); } @@ -220,18 +220,12 @@ fn shell_quote(value: &str) -> String { format!("'{}'", value.replace('\'', "'\\''")) } -fn candidate_from_shell_output(output: &str) -> Option { - let mut lines = output +fn candidate_paths_from_shell_output(output: &str) -> impl Iterator + '_ { + output .lines() .map(str::trim) - .filter(|line| !line.is_empty()); - let candidate = lines.next()?; - if lines.next().is_some() { - return None; - } - - let path = PathBuf::from(candidate); - path.is_absolute().then_some(path) + .map(PathBuf::from) + .filter(|path| path.is_absolute()) } fn is_executable_file(path: &Path) -> bool { @@ -264,3 +258,28 @@ pub(crate) fn blox_acp_command(agent_id: &str) -> Option { parts.join(",") }) } + +#[cfg(test)] +mod tests { + use super::candidate_paths_from_shell_output; + use std::path::PathBuf; + + #[test] + fn candidate_paths_tolerate_startup_output_before_absolute_path() { + let paths: Vec<_> = candidate_paths_from_shell_output( + "hello from shell init\n/opt/homebrew/bin/codex-acp\n", + ) + .collect(); + + assert_eq!(paths, vec![PathBuf::from("/opt/homebrew/bin/codex-acp")]); + } + + #[test] + fn candidate_paths_ignore_relative_lines_and_function_bodies() { + let paths: Vec<_> = + candidate_paths_from_shell_output("codex-acp () {\n\tcommand codex-acp \"$@\"\n}\n") + .collect(); + + assert!(paths.is_empty()); + } +} diff --git a/crates/blox-cli/src/lib.rs b/crates/blox-cli/src/lib.rs index 28c781ff8..b2ab4672e 100644 --- a/crates/blox-cli/src/lib.rs +++ b/crates/blox-cli/src/lib.rs @@ -151,7 +151,7 @@ fn find_via_login_shell(cmd: &str) -> Option { } let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + for path in candidate_paths_from_shell_output(stdout.as_ref()) { if is_executable_file(&path) { return Some(path); } @@ -165,18 +165,12 @@ fn shell_quote(value: &str) -> String { format!("'{}'", value.replace('\'', "'\\''")) } -fn candidate_from_shell_output(output: &str) -> Option { - let mut lines = output +fn candidate_paths_from_shell_output(output: &str) -> impl Iterator + '_ { + output .lines() .map(str::trim) - .filter(|line| !line.is_empty()); - let candidate = lines.next()?; - if lines.next().is_some() { - return None; - } - - let path = PathBuf::from(candidate); - path.is_absolute().then_some(path) + .map(PathBuf::from) + .filter(|path| path.is_absolute()) } fn is_executable_file(path: &Path) -> bool { @@ -645,4 +639,21 @@ mod tests { let input = "line1\nline2\ttab"; assert_eq!(strip_ansi_escape_sequences(input), "line1\nline2\ttab"); } + + #[test] + fn candidate_paths_tolerate_startup_output_before_absolute_path() { + let paths: Vec<_> = + candidate_paths_from_shell_output("hello from shell init\n/opt/homebrew/bin/sq\n") + .collect(); + + assert_eq!(paths, vec![PathBuf::from("/opt/homebrew/bin/sq")]); + } + + #[test] + fn candidate_paths_ignore_relative_lines_and_function_bodies() { + let paths: Vec<_> = + candidate_paths_from_shell_output("sq () {\n\tcommand sq \"$@\"\n}\n").collect(); + + assert!(paths.is_empty()); + } } diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index 9fc866d30..d4d85ae79 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -20,32 +20,30 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { continue; } - match candidate_from_shell_output(stdout.as_ref()) { - Some(path) if is_executable_file(&path) => { - lines.push(format!( - " {shell} -l -c '{lookup_cmd}' => {} (resolved)", - path.display() - )); - return ResolvedBinary { - path: Some(path), - search_output: lines.join("\n"), - }; - } - Some(path) => { - lines.push(format!( - " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an executable file)", - path.display() - )); - } - None if stdout.trim().is_empty() => { - lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); - } - None => { - lines.push(format!( - " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an absolute path)", - summarize_output(stdout.as_ref()) - )); - } + let candidate_paths = candidate_paths_from_shell_output(stdout.as_ref()); + if let Some(path) = candidate_paths.iter().find(|path| is_executable_file(path)) { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (resolved)", + path.display() + )); + return ResolvedBinary { + path: Some(path.clone()), + search_output: lines.join("\n"), + }; + } + + if let Some(path) = candidate_paths.first() { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an executable file)", + path.display() + )); + } else if stdout.trim().is_empty() { + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); + } else { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an absolute path)", + summarize_output(stdout.as_ref()) + )); } } Err(e) => { @@ -92,18 +90,13 @@ fn shell_quote(value: &str) -> String { format!("'{}'", value.replace('\'', "'\\''")) } -fn candidate_from_shell_output(output: &str) -> Option { - let mut lines = output +fn candidate_paths_from_shell_output(output: &str) -> Vec { + output .lines() .map(str::trim) - .filter(|line| !line.is_empty()); - let candidate = lines.next()?; - if lines.next().is_some() { - return None; - } - - let path = PathBuf::from(candidate); - path.is_absolute().then_some(path) + .map(PathBuf::from) + .filter(|path| path.is_absolute()) + .collect() } fn is_executable_file(path: &Path) -> bool { @@ -152,27 +145,41 @@ pub fn format_command_output(cmd_desc: &str, output: &std::process::Output) -> S #[cfg(test)] mod tests { - use super::{candidate_from_shell_output, is_executable_file, shell_quote}; + use super::{candidate_paths_from_shell_output, is_executable_file, shell_quote}; use std::fs::{self, File}; use std::path::PathBuf; #[test] fn candidate_accepts_single_absolute_path() { assert_eq!( - candidate_from_shell_output("/opt/homebrew/bin/git\n"), - Some(PathBuf::from("/opt/homebrew/bin/git")) + candidate_paths_from_shell_output("/opt/homebrew/bin/git\n"), + vec![PathBuf::from("/opt/homebrew/bin/git")] + ); + } + + #[test] + fn candidate_tolerates_startup_output_before_absolute_path() { + assert_eq!( + candidate_paths_from_shell_output("hello from shell init\n/opt/homebrew/bin/git\n"), + vec![PathBuf::from("/opt/homebrew/bin/git")] ); } #[test] fn candidate_rejects_function_body_output() { let output = "git () {\n\tcommand git \"$@\"\n}\n"; - assert_eq!(candidate_from_shell_output(output), None); + assert_eq!( + candidate_paths_from_shell_output(output), + Vec::::new() + ); } #[test] fn candidate_rejects_relative_or_command_name_output() { - assert_eq!(candidate_from_shell_output("git\n"), None); + assert_eq!( + candidate_paths_from_shell_output("git\n"), + Vec::::new() + ); } #[test] From 16ef7f42c51314ac2c4d804939cba0ef966f90d8 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 28 May 2026 10:41:01 +1000 Subject: [PATCH 4/5] fix(doctor): pick last shell lookup path rc files run before -c and can emit absolute paths before the lookup builtin's answer, so iterating from the top can shadow the real result with startup output. take the last executable absolute path instead. Signed-off-by: Matt Toohey --- crates/acp-client/src/types.rs | 9 +++++---- crates/blox-cli/src/lib.rs | 9 +++++---- crates/doctor/src/resolve.rs | 6 +++++- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/crates/acp-client/src/types.rs b/crates/acp-client/src/types.rs index 9546347a7..0fc0690eb 100644 --- a/crates/acp-client/src/types.rs +++ b/crates/acp-client/src/types.rs @@ -207,10 +207,11 @@ fn find_via_login_shell(cmd: &str) -> Option { } let stdout = String::from_utf8_lossy(&output.stdout); - for path in candidate_paths_from_shell_output(stdout.as_ref()) { - if is_executable_file(&path) { - return Some(path); - } + if let Some(path) = candidate_paths_from_shell_output(stdout.as_ref()) + .filter(|path| is_executable_file(path)) + .last() + { + return Some(path); } } None diff --git a/crates/blox-cli/src/lib.rs b/crates/blox-cli/src/lib.rs index b2ab4672e..95e2ad4d5 100644 --- a/crates/blox-cli/src/lib.rs +++ b/crates/blox-cli/src/lib.rs @@ -151,10 +151,11 @@ fn find_via_login_shell(cmd: &str) -> Option { } let stdout = String::from_utf8_lossy(&output.stdout); - for path in candidate_paths_from_shell_output(stdout.as_ref()) { - if is_executable_file(&path) { - return Some(path); - } + if let Some(path) = candidate_paths_from_shell_output(stdout.as_ref()) + .filter(|path| is_executable_file(path)) + .last() + { + return Some(path); } } diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index d4d85ae79..8d48522ff 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -21,7 +21,11 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { } let candidate_paths = candidate_paths_from_shell_output(stdout.as_ref()); - if let Some(path) = candidate_paths.iter().find(|path| is_executable_file(path)) { + if let Some(path) = candidate_paths + .iter() + .rev() + .find(|path| is_executable_file(path)) + { lines.push(format!( " {shell} -l -c '{lookup_cmd}' => {} (resolved)", path.display() From a323b6fb3f1d4ea94065c1fd8156281d23a400ef Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 28 May 2026 10:53:08 +1000 Subject: [PATCH 5/5] test(doctor): cover last-wins shell path picker add a regression test in each resolver crate that exercises the rc-file echo case: two executables on disk, both as absolute paths in stdout, decoy first. the picker should return the second one. Signed-off-by: Matt Toohey --- crates/acp-client/src/types.rs | 31 ++++++++++++++++++++++++++++++- crates/blox-cli/src/lib.rs | 29 +++++++++++++++++++++++++++++ crates/doctor/src/resolve.rs | 28 ++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 1 deletion(-) diff --git a/crates/acp-client/src/types.rs b/crates/acp-client/src/types.rs index 0fc0690eb..1c989052f 100644 --- a/crates/acp-client/src/types.rs +++ b/crates/acp-client/src/types.rs @@ -262,7 +262,7 @@ pub(crate) fn blox_acp_command(agent_id: &str) -> Option { #[cfg(test)] mod tests { - use super::candidate_paths_from_shell_output; + use super::{candidate_paths_from_shell_output, is_executable_file}; use std::path::PathBuf; #[test] @@ -283,4 +283,33 @@ mod tests { assert!(paths.is_empty()); } + + #[test] + fn login_shell_picks_last_executable_absolute_path() { + // Simulates a rc file printing an absolute path of an unrelated + // executable before the shell builtin prints the real lookup answer. + let dir = std::env::temp_dir().join(format!("acp-client-last-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + let decoy = dir.join("decoy"); + let real: PathBuf = dir.join("real"); + std::fs::File::create(&decoy).unwrap(); + std::fs::File::create(&real).unwrap(); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&decoy, std::fs::Permissions::from_mode(0o755)).unwrap(); + std::fs::set_permissions(&real, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + + let stdout = format!("{}\n{}\n", decoy.display(), real.display()); + let picked = candidate_paths_from_shell_output(&stdout) + .filter(|p| is_executable_file(p)) + .last(); + + assert_eq!(picked, Some(real)); + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/crates/blox-cli/src/lib.rs b/crates/blox-cli/src/lib.rs index 95e2ad4d5..ff643201c 100644 --- a/crates/blox-cli/src/lib.rs +++ b/crates/blox-cli/src/lib.rs @@ -657,4 +657,33 @@ mod tests { assert!(paths.is_empty()); } + + #[test] + fn login_shell_picks_last_executable_absolute_path() { + // Simulates a rc file printing an absolute path of an unrelated + // executable before the shell builtin prints the real lookup answer. + let dir = std::env::temp_dir().join(format!("blox-cli-last-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + let decoy = dir.join("decoy"); + let real = dir.join("real"); + std::fs::File::create(&decoy).unwrap(); + std::fs::File::create(&real).unwrap(); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&decoy, std::fs::Permissions::from_mode(0o755)).unwrap(); + std::fs::set_permissions(&real, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + + let stdout = format!("{}\n{}\n", decoy.display(), real.display()); + let picked = candidate_paths_from_shell_output(&stdout) + .filter(|p| is_executable_file(p)) + .last(); + + assert_eq!(picked, Some(real)); + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index 8d48522ff..89482f4d7 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -191,6 +191,34 @@ mod tests { assert_eq!(shell_quote("git'bad"), "'git'\\''bad'"); } + #[test] + fn picks_last_executable_when_rc_file_echoes_absolute_path() { + // Simulates a rc file printing an absolute path of an unrelated + // executable before the shell builtin prints the real lookup answer. + let dir = std::env::temp_dir().join(format!("doctor-resolve-last-{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + + let decoy = dir.join("decoy"); + let real = dir.join("real"); + File::create(&decoy).unwrap(); + File::create(&real).unwrap(); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&decoy, fs::Permissions::from_mode(0o755)).unwrap(); + fs::set_permissions(&real, fs::Permissions::from_mode(0o755)).unwrap(); + } + + let stdout = format!("{}\n{}\n", decoy.display(), real.display()); + let candidates = candidate_paths_from_shell_output(&stdout); + let picked = candidates.iter().rev().find(|p| is_executable_file(p)); + + assert_eq!(picked, Some(&real)); + let _ = fs::remove_dir_all(&dir); + } + #[test] fn executable_file_validation_checks_file_and_mode() { let dir = std::env::temp_dir().join(format!("doctor-resolve-test-{}", std::process::id()));