From 81a8b87a60ee06cd2f2b7860948841fb291c8e21 Mon Sep 17 00:00:00 2001 From: John Matthew Tennant Date: Tue, 16 Jun 2026 15:01:17 -0400 Subject: [PATCH] bug(hooks): repair PATH for plugin hook commands (#9615) Signed-off-by: John Tennant Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: Douwe M Osinga --- crates/goose/src/agents/agent.rs | 5 +- .../platform_extensions/developer/shell.rs | 6 +- crates/goose/src/hooks/mod.rs | 262 +++++++++++++++--- 3 files changed, 228 insertions(+), 45 deletions(-) diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index b5ef13a52..ad51f9a8f 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -363,7 +363,10 @@ impl Agent { permission_manager, provider.clone(), ), - hook_manager: crate::hooks::HookManager::load(std::env::current_dir().ok().as_deref()), + hook_manager: crate::hooks::HookManager::load( + std::env::current_dir().ok().as_deref(), + use_login_shell_path, + ), #[cfg(test)] stop_hook_block_cap_override: None, container: Mutex::new(None), diff --git a/crates/goose/src/agents/platform_extensions/developer/shell.rs b/crates/goose/src/agents/platform_extensions/developer/shell.rs index 2f7245944..a721756a4 100644 --- a/crates/goose/src/agents/platform_extensions/developer/shell.rs +++ b/crates/goose/src/agents/platform_extensions/developer/shell.rs @@ -26,7 +26,7 @@ use crate::subprocess::SubprocessExt; /// When inside Flatpak, shell commands must be wrapped with `flatpak-spawn --host` /// to execute on the host system rather than inside the sandbox. #[cfg(not(windows))] -fn is_flatpak() -> bool { +pub(crate) fn is_flatpak() -> bool { std::path::Path::new("/.flatpak-info").exists() } @@ -34,7 +34,7 @@ fn is_flatpak() -> bool { const FLATPAK_HOST_ARGS: [&str; 2] = ["--host", "--watch-bus"]; #[cfg(not(windows))] -fn flatpak_spawn_command() -> tokio::process::Command { +pub(crate) fn flatpak_spawn_command() -> tokio::process::Command { let mut command = tokio::process::Command::new("flatpak-spawn"); command.args(FLATPAK_HOST_ARGS); command @@ -201,7 +201,7 @@ pub struct ShellOutput { /// a minimal PATH like `/usr/bin:/bin`. This function spawns a login shell to /// source the user's profile and recover the full PATH. #[cfg(not(windows))] -fn resolve_login_shell_path() -> Option { +pub(crate) fn resolve_login_shell_path() -> Option { use process_wrap::std::{CommandWrap, ProcessSession}; let shell = unix_shell(); diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index 590b2adcf..c333e9a4c 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -26,6 +26,7 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::process::Stdio; +use std::sync::OnceLock; use std::time::Duration; use anyhow::{Context, Result}; @@ -224,21 +225,22 @@ pub enum HookDecision { #[derive(Debug, Default, Clone)] pub struct HookManager { rules: HashMap>, + use_login_shell_path: bool, } impl HookManager { /// Build a manager by scanning all enabled plugins for `hooks/hooks.json`. - pub fn load(project_root: Option<&Path>) -> Self { + pub fn load(project_root: Option<&Path>, use_login_shell_path: bool) -> Self { let plugins = discover_enabled_plugins(project_root); - Self::from_plugins(plugins) + Self::from_plugins(plugins, use_login_shell_path) } #[cfg(test)] pub(crate) fn from_plugins_for_test(plugins: Vec) -> Self { - Self::from_plugins(plugins) + Self::from_plugins(plugins, false) } - fn from_plugins(plugins: Vec) -> Self { + fn from_plugins(plugins: Vec, use_login_shell_path: bool) -> Self { let mut rules: HashMap> = HashMap::new(); let mut total = 0usize; @@ -271,7 +273,10 @@ impl HookManager { ); } - Self { rules } + Self { + rules, + use_login_shell_path, + } } /// Returns true if any rule is registered for `event`. @@ -314,19 +319,25 @@ impl HookManager { command = %command, "Running plugin hook", ); - let res = run_command_hook(command, &rule.plugin_root, &payload, *timeout) - .await - .and_then(|o| { - if o.status.success() { - Ok(()) - } else { - anyhow::bail!( - "hook `{command}` exited with {:?}: {}", - o.status.code(), - String::from_utf8_lossy(&o.stderr).trim() - ) - } - }); + let res = run_command_hook( + command, + &rule.plugin_root, + &payload, + *timeout, + self.use_login_shell_path, + ) + .await + .and_then(|o| { + if o.status.success() { + Ok(()) + } else { + anyhow::bail!( + "hook `{command}` exited with {:?}: {}", + o.status.code(), + String::from_utf8_lossy(&o.stderr).trim() + ) + } + }); if let Err(err) = res { warn!( plugin = %rule.plugin_name, @@ -368,20 +379,27 @@ impl HookManager { for action in &rule.actions { let LoadedAction::Command { command, timeout } = action; - let output = - match run_command_hook(command, &rule.plugin_root, &payload, *timeout).await { - Ok(o) => o, - Err(err) => { - warn!( - plugin = %rule.plugin_name, - event = %event, - command = %command, - error = %err, - "Plugin hook failed", - ); - continue; - } - }; + let output = match run_command_hook( + command, + &rule.plugin_root, + &payload, + *timeout, + self.use_login_shell_path, + ) + .await + { + Ok(o) => o, + Err(err) => { + warn!( + plugin = %rule.plugin_name, + event = %event, + command = %command, + error = %err, + "Plugin hook failed", + ); + continue; + } + }; if let Some(reason) = deny_reason(&output) { info!( @@ -507,16 +525,38 @@ async fn run_command_hook( plugin_root: &Path, payload: &str, timeout: Duration, + use_login_shell_path: bool, +) -> Result { + match tokio::time::timeout( + timeout, + run_command_hook_inner(raw_command, plugin_root, payload, use_login_shell_path), + ) + .await + { + Ok(res) => res, + Err(_) => anyhow::bail!("hook `{raw_command}` timed out after {:?}", timeout), + } +} + +async fn run_command_hook_inner( + raw_command: &str, + plugin_root: &Path, + payload: &str, + use_login_shell_path: bool, ) -> Result { let command = expand_plugin_root(raw_command, plugin_root); - let mut child = Command::new("sh") - .arg("-c") - .arg(&command) - .env("PLUGIN_ROOT", plugin_root) + let path = if use_login_shell_path { + hook_path().await + } else { + None + }; + let mut process = hook_command(&command, plugin_root, path.as_deref()); + process .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) - .kill_on_drop(true) + .kill_on_drop(true); + let mut child = process .spawn() .with_context(|| format!("spawning hook `{command}`"))?; @@ -525,10 +565,87 @@ async fn run_command_hook( let _ = stdin.shutdown().await; } - match tokio::time::timeout(timeout, child.wait_with_output()).await { - Ok(res) => res.with_context(|| format!("waiting on hook `{command}`")), - Err(_) => anyhow::bail!("hook `{command}` timed out after {:?}", timeout), + child + .wait_with_output() + .await + .with_context(|| format!("waiting on hook `{command}`")) +} + +fn hook_command(command: &str, plugin_root: &Path, path: Option<&str>) -> Command { + #[cfg(not(windows))] + { + if crate::agents::platform_extensions::developer::shell::is_flatpak() { + let mut process = + crate::agents::platform_extensions::developer::shell::flatpak_spawn_command(); + process.arg(format!("--env=PLUGIN_ROOT={}", plugin_root.display())); + if let Some(path) = path { + process.arg(format!("--env=PATH={path}")); + } + process.arg("sh").arg("-c").arg(command); + return process; + } } + + let mut process = Command::new("sh"); + process + .arg("-c") + .arg(command) + .env("PLUGIN_ROOT", plugin_root); + if let Some(path) = path { + process.env("PATH", path); + } + process +} + +async fn hook_path() -> Option { + static HOOK_PATH: OnceLock>> = OnceLock::new(); + let mut rx = HOOK_PATH + .get_or_init(|| { + let (tx, rx) = tokio::sync::watch::channel(None); + tokio::spawn(async move { + let path = resolve_hook_path().await; + let _ = tx.send(path); + }); + rx + }) + .clone(); + + if rx.borrow().is_some() { + return rx.borrow().clone(); + } + if rx.changed().await.is_ok() { + rx.borrow().clone() + } else { + None + } +} + +async fn resolve_hook_path() -> Option { + #[cfg(not(windows))] + { + tokio::task::spawn_blocking(|| { + crate::agents::platform_extensions::developer::shell::resolve_login_shell_path() + .map(|login| merge_paths(&login, &std::env::var("PATH").unwrap_or_default())) + }) + .await + .ok() + .flatten() + } + #[cfg(windows)] + { + None + } +} + +fn merge_paths(first: &str, second: &str) -> String { + let mut seen = std::collections::HashSet::new(); + let mut merged = Vec::new(); + for entry in first.split(':').chain(second.split(':')) { + if !entry.is_empty() && seen.insert(entry) { + merged.push(entry); + } + } + merged.join(":") } fn expand_plugin_root(command: &str, plugin_root: &Path) -> String { @@ -548,7 +665,7 @@ mod tests { } fn make_manager(plugins: Vec) -> HookManager { - HookManager::from_plugins(plugins) + HookManager::from_plugins(plugins, false) } #[test] @@ -652,6 +769,69 @@ mod tests { ); } + #[test] + fn merge_paths_keeps_login_entries_first() { + assert_eq!( + merge_paths("/opt/homebrew/bin:/bin", "/bin:/usr/bin:/custom/bin"), + "/opt/homebrew/bin:/bin:/usr/bin:/custom/bin" + ); + } + + #[cfg(not(windows))] + #[tokio::test] + async fn command_hooks_repair_path_when_enabled() { + let tmp = tempfile::tempdir().unwrap(); + let login_bin = tmp.path().join("login-bin"); + std::fs::create_dir(&login_bin).unwrap(); + + let fake_shell = tmp.path().join("fake-login-shell"); + std::fs::write( + &fake_shell, + "#!/bin/sh\nprintf '%s\\n' \"$FAKE_LOGIN_PATH\"\n", + ) + .unwrap(); + let helper = login_bin.join("hook-visible-tool"); + std::fs::write(&helper, "#!/bin/sh\nprintf 'hook-visible-tool-ran'\n").unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + for path in [&fake_shell, &helper] { + let mut perms = std::fs::metadata(path).unwrap().permissions(); + perms.set_mode(0o755); + std::fs::set_permissions(path, perms).unwrap(); + } + } + + let fake_shell = fake_shell.to_string_lossy().into_owned(); + let fake_login_path = format!("{}:/usr/bin:/bin", login_bin.display()); + let _guard = env_lock::lock_env([ + ("GOOSE_SHELL", Some(fake_shell.as_str())), + ("FAKE_LOGIN_PATH", Some(fake_login_path.as_str())), + ( + "PATH", + Some( + "/Applications/Goose.app/Contents/Resources/bin:/usr/bin:/bin:/usr/sbin:/sbin", + ), + ), + ]); + + let output = run_command_hook( + "hook-visible-tool", + tmp.path(), + "{}", + Duration::from_secs(5), + true, + ) + .await + .unwrap(); + + assert!(output.status.success()); + assert_eq!( + String::from_utf8_lossy(&output.stdout), + "hook-visible-tool-ran" + ); + } + #[tokio::test] async fn matcher_filters_by_tool_name() { let tmp = tempfile::tempdir().unwrap();