tkmind/tkmind_go
1
0
Fork 0
Code Issues Pull Requests Actions 5 Packages Projects Releases Wiki Activity

bug(hooks): repair PATH for plugin hook commands (#9615)

Browse Source 
Signed-off-by: John Tennant <jtennant@squareup.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Douwe M Osinga <douwe@sidewalklabs.com>
This commit is contained in:
John Matthew Tennant
2026-06-16 15:01:17 -04:00
committed by GitHub
parent 0b67498ba5
commit 81a8b87a60
3 changed files with 228 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/goose/src/agents/agent.rs
+4 -1
View File
@@ -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),
crates/goose/src/agents/platform_extensions/developer/shell.rs
+3 -3
View File
@@ -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<String> {
pub(crate) fn resolve_login_shell_path() -> Option<String> {
use process_wrap::std::{CommandWrap, ProcessSession};
let shell = unix_shell();
crates/goose/src/hooks/mod.rs
+221 -41
View File
@@ -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<HookEvent, Vec<LoadedRule>>,
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<DiscoveredPlugin>) -> Self {
Self::from_plugins(plugins)
Self::from_plugins(plugins, false)
}
fn from_plugins(plugins: Vec<DiscoveredPlugin>) -> Self {
fn from_plugins(plugins: Vec<DiscoveredPlugin>, use_login_shell_path: bool) -> Self {
let mut rules: HashMap<HookEvent, Vec<LoadedRule>> = 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<std::process::Output> {
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<std::process::Output> {
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<String> {
static HOOK_PATH: OnceLock<tokio::sync::watch::Receiver<Option<String>>> = 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<String> {
#[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<DiscoveredPlugin>) -> 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();