diff --git a/Cargo.lock b/Cargo.lock index 62bb1c157..355ca98cc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10,6 +10,7 @@ dependencies = [ "anyhow", "async-trait", "blox-cli", + "doctor", "log", "nix", "serde", diff --git a/apps/staged/src-tauri/Cargo.lock b/apps/staged/src-tauri/Cargo.lock index d48952f07..5874474e8 100644 --- a/apps/staged/src-tauri/Cargo.lock +++ b/apps/staged/src-tauri/Cargo.lock @@ -65,6 +65,7 @@ dependencies = [ "anyhow", "async-trait", "blox-cli", + "doctor", "log", "nix", "serde", diff --git a/apps/staged/src-tauri/src/actions/commands.rs b/apps/staged/src-tauri/src/actions/commands.rs index 6b474a811..15b422106 100644 --- a/apps/staged/src-tauri/src/actions/commands.rs +++ b/apps/staged/src-tauri/src/actions/commands.rs @@ -50,14 +50,20 @@ struct DetectingActionsEvent { /// Falls back to [`AcpAiProvider::new`] (first installed agent) only when no /// provider can be resolved at all — i.e. no agents are installed, in which /// case construction would fail regardless. -pub(crate) fn build_action_provider( +pub(crate) async fn build_action_provider( provider_id: Option<&str>, working_dir: PathBuf, ) -> Result { - match crate::session_commands::discover_preferred_provider_id(provider_id) { + let provider = match crate::session_commands::discover_preferred_provider_id(provider_id) { Some(id) => AcpAiProvider::with_agent(&id, working_dir), None => AcpAiProvider::new(working_dir), - } + }?; + let home_snapshot = crate::shell_env::home_env_vars_with_extended_path( + crate::session_runner::shell_env_cache().as_ref(), + ) + .await; + + Ok(provider.with_interpreter_env_snapshot(home_snapshot)) } pub(crate) async fn detect_actions_for_repo_context( @@ -92,6 +98,7 @@ pub(crate) async fn detect_actions_for_repo_context( }; let provider = build_action_provider(provider_id, provider_dir.clone()) + .await .map_err(|e| format!("Failed to create AI provider: {e}"))?; let detector = ActionDetector::new(Box::new(provider)); diff --git a/apps/staged/src-tauri/src/actions/run_detector.rs b/apps/staged/src-tauri/src/actions/run_detector.rs index cfb8e955a..f5ebd8121 100644 --- a/apps/staged/src-tauri/src/actions/run_detector.rs +++ b/apps/staged/src-tauri/src/actions/run_detector.rs @@ -278,7 +278,8 @@ If still building, set regex and has_endpoint_capture to null/false."#, let provider_result = super::commands::build_action_provider( provider_id.as_deref(), working_dir.clone(), - ); + ) + .await; let provider = match provider_result { Ok(p) => p, Err(e) => { diff --git a/apps/staged/src-tauri/src/lib.rs b/apps/staged/src-tauri/src/lib.rs index c5dd5d4c1..02685a574 100644 --- a/apps/staged/src-tauri/src/lib.rs +++ b/apps/staged/src-tauri/src/lib.rs @@ -1460,10 +1460,19 @@ async fn ai_generate_short_names( let agent = find_badge_agent(provider)?; let prompt = build_badge_prompt(existing_badges, new_repos); let working_dir = std::env::temp_dir(); + let interpreter_env_snapshot = crate::shell_env::home_env_vars_with_extended_path( + crate::session_runner::shell_env_cache().as_ref(), + ) + .await; - let response = acp_client::run_acp_prompt(&agent, &working_dir, &prompt) - .await - .ok()?; + let response = acp_client::run_acp_prompt_with_interpreter_env_snapshot( + &agent, + &working_dir, + &prompt, + interpreter_env_snapshot, + ) + .await + .ok()?; // Extract JSON from response (may be wrapped in markdown code fences) let json_str = response diff --git a/crates/acp-client/Cargo.toml b/crates/acp-client/Cargo.toml index 7c426c62d..f6b2a4560 100644 --- a/crates/acp-client/Cargo.toml +++ b/crates/acp-client/Cargo.toml @@ -14,6 +14,7 @@ anyhow = "1" async-trait = "0.1" log = "0.4" blox-cli = { path = "../blox-cli" } +doctor = { path = "../doctor" } [target.'cfg(unix)'.dependencies] nix = { version = "0.31", features = ["signal"] } diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index cffacbb2c..595b0e27b 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -360,25 +360,14 @@ fn path_contains_executable(path_value: &str, executable: &str) -> bool { std::env::split_paths(path_value).any(|dir| is_executable_file(&dir.join(executable))) } -fn path_value_from_snapshot(snapshot: &[(String, String)]) -> Option<&str> { - snapshot - .iter() - .find_map(|(key, value)| (key == "PATH").then_some(value.as_str())) -} - -fn resolve_executable_from_path(path_value: &str, executable: &str) -> Option { - std::env::split_paths(path_value) - .map(|dir| dir.join(executable)) - .find(|path| is_executable_file(path)) -} - fn env_shebang_interpreter_from_snapshot( binary_path: &Path, snapshot: Option<&[(String, String)]>, ) -> Option { let interpreter = env_shebang_interpreter(binary_path)?; - let path_value = path_value_from_snapshot(snapshot?)?; - resolve_executable_from_path(path_value, &interpreter) + let env = doctor::DoctorEnv::new(snapshot?.to_vec()); + let path_value = env.get("PATH")?; + doctor::resolve::resolve_executable_from_path(&interpreter, path_value) } fn is_broad_toolchain_dir(path: &Path) -> bool { @@ -474,13 +463,13 @@ fn shell_exec_line( } #[derive(Debug)] -struct AcpSpawnCommand { - program: PathBuf, - args: Vec, - uses_explicit_interpreter: bool, +pub(crate) struct AcpSpawnCommand { + pub(crate) program: PathBuf, + pub(crate) args: Vec, + pub(crate) uses_explicit_interpreter: bool, } -fn acp_spawn_command( +pub(crate) fn acp_spawn_command( binary_path: &Path, acp_args: &[String], interpreter_env_snapshot: Option<&[(String, String)]>, diff --git a/crates/acp-client/src/lib.rs b/crates/acp-client/src/lib.rs index be16747a0..1bbba9ddf 100644 --- a/crates/acp-client/src/lib.rs +++ b/crates/acp-client/src/lib.rs @@ -29,7 +29,7 @@ pub use agent_client_protocol::{ pub use driver::{ strip_code_fences, AcpDriver, AgentDriver, BasicMessageWriter, MessageWriter, Store, }; -pub use simple::run_acp_prompt; +pub use simple::{run_acp_prompt, run_acp_prompt_with_interpreter_env_snapshot}; pub use types::{ discover_providers, find_acp_agent, find_acp_agent_by_id, find_command, known_agent_commands, AcpAgent, AcpProviderInfo, diff --git a/crates/acp-client/src/simple.rs b/crates/acp-client/src/simple.rs index 7f9f610c7..b2603615b 100644 --- a/crates/acp-client/src/simple.rs +++ b/crates/acp-client/src/simple.rs @@ -10,7 +10,7 @@ use anyhow::{Context, Result}; use async_trait::async_trait; use tokio_util::sync::CancellationToken; -use crate::driver::{AgentDriver, BasicMessageWriter, MessageWriter}; +use crate::driver::{acp_spawn_command, AgentDriver, BasicMessageWriter, MessageWriter}; use crate::types::AcpAgent; /// Minimal store implementation for simple prompting (no persistence). @@ -36,6 +36,7 @@ struct SimpleDriverWrapper { binary_path: std::path::PathBuf, acp_args: Vec, agent_label: String, + interpreter_env_snapshot: Option>, } impl SimpleDriverWrapper { @@ -44,8 +45,22 @@ impl SimpleDriverWrapper { binary_path: agent.binary_path.clone(), acp_args: agent.acp_args.clone(), agent_label: agent.label.clone(), + interpreter_env_snapshot: None, } } + + fn with_interpreter_env_snapshot(mut self, vars: Vec<(String, String)>) -> Self { + self.interpreter_env_snapshot = Some(vars); + self + } + + fn spawn_command(&self) -> crate::driver::AcpSpawnCommand { + acp_spawn_command( + &self.binary_path, + &self.acp_args, + self.interpreter_env_snapshot.as_deref(), + ) + } } #[async_trait(?Send)] @@ -81,8 +96,9 @@ impl AgentDriver for SimpleDriverWrapper { use tokio::sync::Mutex; use tokio_util::compat::{TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt}; - let mut child = Command::new(&self.binary_path) - .args(&self.acp_args) + let spawn_command = self.spawn_command(); + let mut child = Command::new(&spawn_command.program) + .args(&spawn_command.args) .current_dir(working_dir) .stdin(Stdio::piped()) .stdout(Stdio::piped()) @@ -293,9 +309,37 @@ impl AgentDriver for SimpleDriverWrapper { /// /// The agent's text response pub async fn run_acp_prompt(agent: &AcpAgent, working_dir: &Path, prompt: &str) -> Result { + run_acp_prompt_with_options(agent, working_dir, prompt, None).await +} + +/// Run a one-shot prompt through ACP using a separate environment snapshot +/// only for env-shebang interpreter resolution. +/// +/// The spawned agent process still inherits the caller's environment. The +/// snapshot is consulted only to turn launchers such as `#!/usr/bin/env node` +/// into ` ` so repo-local PATH entries do not choose +/// the ACP bridge interpreter. +pub async fn run_acp_prompt_with_interpreter_env_snapshot( + agent: &AcpAgent, + working_dir: &Path, + prompt: &str, + interpreter_env_snapshot: Vec<(String, String)>, +) -> Result { + run_acp_prompt_with_options(agent, working_dir, prompt, Some(interpreter_env_snapshot)).await +} + +async fn run_acp_prompt_with_options( + agent: &AcpAgent, + working_dir: &Path, + prompt: &str, + interpreter_env_snapshot: Option>, +) -> Result { let working_dir = working_dir.to_path_buf(); let prompt = prompt.to_string(); - let driver = SimpleDriverWrapper::from_agent(agent); + let mut driver = SimpleDriverWrapper::from_agent(agent); + if let Some(snapshot) = interpreter_env_snapshot { + driver = driver.with_interpreter_env_snapshot(snapshot); + } // Run the ACP session in a blocking task with its own runtime // This is needed because ACP uses !Send futures (LocalSet) @@ -334,3 +378,81 @@ pub async fn run_acp_prompt(agent: &AcpAgent, working_dir: &Path, prompt: &str) .await .context("Task join error")? } + +#[cfg(test)] +mod tests { + use super::*; + + use std::path::{Path, PathBuf}; + use std::time::{SystemTime, UNIX_EPOCH}; + + fn unique_test_dir(prefix: &str) -> PathBuf { + let nonce = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("clock must be after epoch") + .as_nanos(); + let dir = std::env::temp_dir().join(format!("{prefix}-{}-{nonce}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create test dir"); + dir + } + + fn write_executable(path: &Path, content: &str) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("create executable parent"); + } + std::fs::write(path, content).expect("write executable"); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mut perms = std::fs::metadata(path).unwrap().permissions(); + perms.set_mode(0o755); + std::fs::set_permissions(path, perms).expect("chmod executable"); + } + } + + fn join_path_entries(entries: &[PathBuf]) -> String { + std::env::join_paths(entries) + .expect("join path entries") + .into_string() + .expect("path entries should be utf8") + } + + #[test] + fn simple_driver_uses_interpreter_snapshot_for_env_shebang_bridge() { + let dir = unique_test_dir("acp-simple-home-interpreter"); + let home_bin = dir.join("home-bin"); + let project_bin = dir.join("project-bin"); + let agent_bin = dir.join("agent-bin"); + let launcher = agent_bin.join("claude-agent-acp"); + let home_node = home_bin.join("node"); + write_executable(&home_node, "#!/bin/sh\n"); + write_executable(&project_bin.join("node"), "#!/bin/sh\n"); + write_executable(&launcher, "#!/usr/bin/env node\n"); + + let agent = AcpAgent { + binary_path: launcher.clone(), + acp_args: vec![String::from("--stdio")], + label: String::from("Claude Code"), + }; + let home_snapshot = vec![( + String::from("PATH"), + join_path_entries(std::slice::from_ref(&home_bin)), + )]; + + let command = SimpleDriverWrapper::from_agent(&agent) + .with_interpreter_env_snapshot(home_snapshot) + .spawn_command(); + + assert_eq!(command.program, home_node); + assert_eq!( + command.args, + vec![ + launcher.as_os_str().to_os_string(), + std::ffi::OsString::from("--stdio"), + ] + ); + + std::fs::remove_dir_all(dir).expect("cleanup test dir"); + } +} diff --git a/crates/acp-client/src/types.rs b/crates/acp-client/src/types.rs index 1c989052f..2b2495111 100644 --- a/crates/acp-client/src/types.rs +++ b/crates/acp-client/src/types.rs @@ -160,93 +160,11 @@ impl AcpAgent { // Binary discovery // ============================================================================= -/// Common paths where CLIs might be installed (GUI apps don't inherit shell PATH). -const COMMON_PATHS: &[&str] = &[ - "/opt/homebrew/bin", - "/usr/local/bin", - "/usr/bin", - "/home/linuxbrew/.linuxbrew/bin", -]; - /// Find a CLI binary by command name. /// -/// Searches in order: -/// 1. Login shell path lookup (picks up user's PATH from `.zshrc` / `.bashrc`) -/// 2. Common install locations +/// Delegates to doctor so ACP and Doctor share one binary resolution policy. pub fn find_command(cmd: &str) -> Option { - if let Some(path) = find_via_login_shell(cmd) { - return Some(path); - } - - for dir in COMMON_PATHS { - let path = PathBuf::from(dir).join(cmd); - if is_executable_file(&path) { - return Some(path); - } - } - - None -} - -fn find_via_login_shell(cmd: &str) -> Option { - let quoted = shell_quote(cmd); - let lookups = [ - ("/bin/zsh", format!("whence -p -- {quoted}")), - ("/bin/bash", format!("type -P -- {quoted}")), - ]; - - for (shell, lookup_cmd) in lookups { - let Ok(output) = std::process::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_paths_from_shell_output(stdout.as_ref()) - .filter(|path| is_executable_file(path)) - .last() - { - return Some(path); - } - } - None -} - -fn shell_quote(value: &str) -> String { - format!("'{}'", value.replace('\'', "'\\''")) -} - -fn candidate_paths_from_shell_output(output: &str) -> impl Iterator + '_ { - output - .lines() - .map(str::trim) - .map(PathBuf::from) - .filter(|path| path.is_absolute()) -} - -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 - } + doctor::resolve::resolve_binary(cmd).path } /// Map an agent ID to the `--command` value for `blox acp`. @@ -259,57 +177,3 @@ pub(crate) fn blox_acp_command(agent_id: &str) -> Option { parts.join(",") }) } - -#[cfg(test)] -mod tests { - use super::{candidate_paths_from_shell_output, is_executable_file}; - 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()); - } - - #[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/builderbot-actions/src/acp_provider.rs b/crates/builderbot-actions/src/acp_provider.rs index a382d9f76..a3221a1cd 100644 --- a/crates/builderbot-actions/src/acp_provider.rs +++ b/crates/builderbot-actions/src/acp_provider.rs @@ -13,6 +13,7 @@ use crate::detector::AiProvider; pub struct AcpAiProvider { working_dir: PathBuf, provider_id: Option, + interpreter_env_snapshot: Option>, } impl AcpAiProvider { @@ -36,6 +37,7 @@ impl AcpAiProvider { Ok(Self { working_dir, provider_id: None, + interpreter_env_snapshot: None, }) } @@ -53,8 +55,16 @@ impl AcpAiProvider { Ok(Self { working_dir, provider_id: Some(provider_id.to_string()), + interpreter_env_snapshot: None, }) } + + /// Set a separate environment snapshot used only to resolve env-shebang + /// interpreters for ACP bridge startup. + pub fn with_interpreter_env_snapshot(mut self, vars: Vec<(String, String)>) -> Self { + self.interpreter_env_snapshot = Some(vars); + self + } } #[async_trait] @@ -66,6 +76,17 @@ impl AiProvider for AcpAiProvider { } .ok_or_else(|| anyhow::anyhow!("No ACP agent found"))?; - acp_client::run_acp_prompt(&agent, &self.working_dir, &prompt).await + match &self.interpreter_env_snapshot { + Some(snapshot) => { + acp_client::run_acp_prompt_with_interpreter_env_snapshot( + &agent, + &self.working_dir, + &prompt, + snapshot.clone(), + ) + .await + } + None => acp_client::run_acp_prompt(&agent, &self.working_dir, &prompt).await, + } } } diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index cc95d9401..dd6925e3e 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -646,7 +646,9 @@ pub fn lookup_fix_command(check_id: &str, fix_type: &FixType) -> Option #[cfg(test)] mod tests { use super::*; + use std::path::Path; use std::path::PathBuf; + use std::time::{SystemTime, UNIX_EPOCH}; fn resolved(path: Option<&str>, source: Option) -> ResolvedBinary { ResolvedBinary { @@ -660,6 +662,48 @@ mod tests { AI_AGENT_CHECKS.iter().find(|i| i.id == id).unwrap() } + fn unique_tmp_dir(name: &str) -> PathBuf { + let nonce = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("clock must be after epoch") + .as_nanos(); + let dir = std::env::temp_dir().join(format!( + "doctor-agent-{name}-{}-{nonce}", + std::process::id() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("create test dir"); + dir + } + + fn write_executable(path: &Path, contents: &str) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("create executable parent"); + } + std::fs::write(path, contents).expect("write executable"); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mut perms = std::fs::metadata(path).unwrap().permissions(); + perms.set_mode(0o755); + std::fs::set_permissions(path, perms).expect("chmod executable"); + } + } + + fn shell_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) + } + + fn write_login_path_rewrite_profiles(home: &Path, path: &Path) { + let profile = format!( + "export PATH={}\n", + shell_quote(&format!("{}:/usr/bin:/bin", path.to_string_lossy())), + ); + std::fs::write(home.join(".zprofile"), &profile).expect("write zprofile"); + std::fs::write(home.join(".bash_profile"), profile).expect("write bash_profile"); + } + /// An agent with a separate ACP bridge surfaces both binaries as independent /// readouts, each carrying its own install source. Uses Pi (no auth /// commands) so the check runs without shelling out. @@ -736,6 +780,75 @@ mod tests { assert_eq!(check.install_source, Some(InstallSource::Npm)); } + #[test] + fn agent_auth_status_uses_doctor_env_over_login_shell_path_rewrite() { + let tmp = unique_tmp_dir("auth-env-vs-hermit"); + let snapshot_bin = tmp.join("snapshot/bin"); + let hermit_bin = tmp.join("hermit/bin"); + let claude = snapshot_bin.join("claude"); + let bridge = snapshot_bin.join("claude-agent-acp"); + write_executable( + &claude, + "#!/bin/sh\n\ + test \"$DOCTOR_AGENT_ENV\" = snapshot || exit 43\n\ + test \"$1\" = auth || exit 44\n\ + test \"$2\" = status || exit 45\n\ + exit 0\n", + ); + write_executable(&bridge, "#!/bin/sh\nexit 0\n"); + write_executable(&hermit_bin.join("claude"), "#!/bin/sh\nexit 42\n"); + write_executable(&hermit_bin.join("claude-agent-acp"), "#!/bin/sh\nexit 42\n"); + write_login_path_rewrite_profiles(&tmp, &hermit_bin); + + let bridge_resolved = ResolvedBinary { + path: Some(bridge.clone()), + search_output: "BRIDGE-SNAPSHOT-SEARCH".to_string(), + install_source: Some(InstallSource::Npm), + }; + let main_resolved = ResolvedBinary { + path: Some(claude.clone()), + search_output: "MAIN-SNAPSHOT-SEARCH".to_string(), + install_source: Some(InstallSource::CurlPipe), + }; + let env = DoctorEnv::new(vec![ + ("DOCTOR_AGENT_ENV".to_string(), "snapshot".to_string()), + ( + "PATH".to_string(), + format!("{}:/usr/bin:/bin", snapshot_bin.to_string_lossy()), + ), + ("HOME".to_string(), tmp.to_string_lossy().to_string()), + ("USER".to_string(), "doctor-test".to_string()), + ("ZDOTDIR".to_string(), tmp.to_string_lossy().to_string()), + ]); + + let check = check_single_ai_agent( + agent("ai-agent-claude"), + true, + std::slice::from_ref(&bridge_resolved), + Some(&main_resolved), + None, + Some(&env), + ); + + assert_eq!(check.status, CheckStatus::Pass); + assert_eq!(check.auth_status, Some(AuthStatus::Authenticated)); + assert_eq!( + check.path.as_deref(), + Some(claude.to_string_lossy().as_ref()) + ); + assert_eq!( + check.bridge_path.as_deref(), + Some(bridge.to_string_lossy().as_ref()) + ); + let raw = check.raw_output.unwrap_or_default(); + assert!( + !raw.contains(&hermit_bin.to_string_lossy().to_string()), + "DoctorEnv-backed auth should not execute through Hermit/login PATH; raw:\n{raw}", + ); + + let _ = std::fs::remove_dir_all(tmp); + } + /// A fully unresolved agent carries neither readout. #[test] fn unresolved_agent_has_no_readouts() { diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index 1e00fa26b..b8ef9d414 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -30,7 +30,7 @@ pub(crate) fn resolve_binary_with_diagnostics( if let Some(path_value) = env.and_then(|env| env.get("PATH")) { lines.push(" strategy 0 — caller environment PATH:".to_string()); - if let Some(path) = resolve_from_path(cmd, path_value) { + if let Some(path) = resolve_executable_from_path(cmd, path_value) { lines.push(format!(" PATH => {} (resolved)", path.display())); return resolved_binary(path, &lines, timeouts, env); } @@ -160,7 +160,11 @@ pub(crate) fn resolve_binary_with_diagnostics( ) } -fn resolve_from_path(cmd: &str, path_value: &str) -> Option { +/// Resolve an executable name against a PATH value. +/// +/// This is intentionally PATH-only: callers that need doctor's full fallback +/// strategy should use [`resolve_binary`] or [`resolve_binary_with_env`]. +pub fn resolve_executable_from_path(cmd: &str, path_value: &str) -> Option { std::env::split_paths(path_value) .map(|dir| dir.join(cmd)) .find(|path| is_executable_file(path)) @@ -549,6 +553,32 @@ mod tests { use super::*; use std::fs::{self, File}; + fn write_executable(path: &Path, contents: &str) { + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).unwrap(); + } + fs::write(path, contents).unwrap(); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(path, fs::Permissions::from_mode(0o755)).unwrap(); + } + } + + fn quoted(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) + } + + fn write_login_path_rewrite_profiles(home: &Path, path: &Path) { + let profile = format!( + "export PATH={}\n", + quoted(&format!("{}:/usr/bin:/bin", path.to_string_lossy())), + ); + fs::write(home.join(".zprofile"), &profile).unwrap(); + fs::write(home.join(".bash_profile"), profile).unwrap(); + } + #[test] fn candidate_accepts_single_absolute_path() { assert_eq!( @@ -676,6 +706,45 @@ mod tests { let _ = fs::remove_dir_all(&dir); } + #[test] + fn resolve_binary_with_env_prefers_snapshot_path_over_login_shell_rewrite() { + let dir = std::env::temp_dir().join(format!( + "doctor-resolve-env-vs-hermit-{}", + std::process::id() + )); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + + let snapshot_bin = dir.join("snapshot/bin"); + let hermit_bin = dir.join("hermit/bin"); + let snapshot_tool = snapshot_bin.join("doctor-agent-cli"); + let hermit_tool = hermit_bin.join("doctor-agent-cli"); + write_executable(&snapshot_tool, "#!/bin/sh\nexit 0\n"); + write_executable(&hermit_tool, "#!/bin/sh\nexit 42\n"); + write_login_path_rewrite_profiles(&dir, &hermit_bin); + + let env = crate::DoctorEnv::new(vec![ + ( + "PATH".to_string(), + format!("{}:/usr/bin:/bin", snapshot_bin.to_string_lossy()), + ), + ("HOME".to_string(), dir.to_string_lossy().to_string()), + ("USER".to_string(), "doctor-test".to_string()), + ("ZDOTDIR".to_string(), dir.to_string_lossy().to_string()), + ]); + let resolved = resolve_binary_with_env("doctor-agent-cli", &env); + + assert_eq!(resolved.path.as_deref(), Some(snapshot_tool.as_path())); + assert!( + !resolved + .search_output + .contains(&hermit_bin.to_string_lossy().to_string()), + "DoctorEnv resolution should not fall through to the Hermit/login PATH:\n{}", + resolved.search_output, + ); + let _ = fs::remove_dir_all(&dir); + } + #[test] fn npm_search_dirs_includes_expected_paths_for_fixed_home() { let home = PathBuf::from("/home/test");