From 8a2bbf70c657e0bdb253cd5da8e2b214f4c9cb60 Mon Sep 17 00:00:00 2001 From: Tyler Longwell Date: Sat, 21 Feb 2026 14:19:46 -0500 Subject: [PATCH 01/10] feat: add agent lifecycle hooks (Claude Code-compatible config) Hooks fire at key agent lifecycle events and execute user-configured actions via MCP tools. Supports blocking (prompt submit, tool use) and context injection (PostCompact). - 8 of 16 events wired: SessionStart, UserPromptSubmit, PreToolUse, PostToolUse, PostToolUseFailure, PreCompact, PostCompact, Stop - Two action types: command (via developer__shell) and mcp_tool - Config reads .goose/settings.json, .claude/settings.json, or ~/.config/goose/hooks.json with forward-compatible parsing - Fail-open: hook errors never crash the agent --- crates/goose/src/agents/agent.rs | 196 +++++++++++- crates/goose/src/agents/execute_commands.rs | 38 +++ crates/goose/src/agents/tool_execution.rs | 34 ++ crates/goose/src/hooks/config.rs | 173 +++++++++++ crates/goose/src/hooks/mod.rs | 326 ++++++++++++++++++++ crates/goose/src/hooks/types.rs | 301 ++++++++++++++++++ crates/goose/src/lib.rs | 1 + 7 files changed, 1065 insertions(+), 4 deletions(-) create mode 100644 crates/goose/src/hooks/config.rs create mode 100644 crates/goose/src/hooks/mod.rs create mode 100644 crates/goose/src/hooks/types.rs diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index a1439079ea49..54029121bce8 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -386,12 +386,44 @@ impl Agent { request_to_response_map: &HashMap>>, cancel_token: Option, session: &Session, + hooks: &Option, ) -> Result> { let mut tool_futures: Vec<(String, ToolStream)> = Vec::new(); // Handle pre-approved and read-only tools for request in &permission_check_result.approved { if let Ok(tool_call) = request.tool_call.clone() { + // Fire PreToolUse hook + if let Some(ref hooks) = hooks { + let invocation = crate::hooks::HookInvocation::pre_tool_use( + session.id.clone(), + tool_call.name.to_string(), + serde_json::to_value(&tool_call.arguments) + .unwrap_or(serde_json::Value::Null), + session.working_dir.to_string_lossy().to_string(), + ); + let outcome = hooks + .run(invocation, &self.extension_manager, &session.working_dir) + .await + .unwrap_or_default(); + if outcome.blocked { + // Create an error response instead of dispatching + let error_result = Err(ErrorData::new( + ErrorCode::INTERNAL_ERROR, + "Tool execution blocked by hook".to_string(), + None, + )); + tool_futures.push(( + request.id.clone(), + tool_stream( + Box::new(stream::empty()), + futures::future::ready(error_result), + ), + )); + continue; + } + } + let (req_id, tool_result) = self .dispatch_tool_call( tool_call, @@ -980,6 +1012,9 @@ impl Agent { .clone() .ok_or_else(|| anyhow::anyhow!("Session {} has no conversation", session_config.id))?; + // Load hooks configuration + let hooks = crate::hooks::Hooks::load(&session.working_dir).ok(); + let needs_auto_compact = check_if_compaction_needed( self.provider().await?.as_ref(), &conversation, @@ -1019,6 +1054,17 @@ impl Agent { ) ); + // Fire PreCompact hook + if let Some(ref hooks) = hooks { + let invocation = crate::hooks::HookInvocation::pre_compact( + session_config.id.clone(), + conversation_to_compact.messages().len(), + false, // manual = false for auto-compact + session.working_dir.to_string_lossy().to_string(), + ); + let _ = hooks.run(invocation, &self.extension_manager, &session.working_dir).await; + } + match compact_messages( self.provider().await?.as_ref(), &session_config.id, @@ -1027,10 +1073,30 @@ impl Agent { ) .await { - Ok((compacted_conversation, summarization_usage)) => { + Ok((mut compacted_conversation, summarization_usage)) => { session_manager.replace_conversation(&session_config.id, &compacted_conversation).await?; self.update_session_metrics(&session_config.id, session_config.schedule_id.clone(), &summarization_usage, true).await?; + // Fire PostCompact hook and inject additional context if any + if let Some(ref hooks) = hooks { + let invocation = crate::hooks::HookInvocation::post_compact( + session_config.id.clone(), + conversation_to_compact.messages().len(), + compacted_conversation.messages().len(), + false, // manual = false for auto-compact + session.working_dir.to_string_lossy().to_string(), + ); + if let Ok(outcome) = hooks.run(invocation, &self.extension_manager, &session.working_dir).await { + if let Some(context) = outcome.context { + let context_msg = Message::assistant() + .with_text(context) + .with_visibility(false, true); // agent-only + session_manager.add_message(&session_config.id, &context_msg).await?; + compacted_conversation.push(context_msg); + } + } + } + yield AgentEvent::HistoryReplaced(compacted_conversation.clone()); yield AgentEvent::Message( @@ -1053,7 +1119,7 @@ impl Agent { } }; - let mut reply_stream = self.reply_internal(final_conversation, session_config, session, cancel_token).await?; + let mut reply_stream = self.reply_internal(final_conversation, session_config, session, cancel_token, hooks).await?; while let Some(event) = reply_stream.next().await { yield event?; } @@ -1066,6 +1132,7 @@ impl Agent { session_config: SessionConfig, session: Session, cancel_token: Option, + hooks: Option, ) -> Result>> { let context = self .prepare_reply_context(&session.id, conversation, session.working_dir.as_path()) @@ -1086,9 +1153,10 @@ impl Agent { let session_id = session_config.id.clone(); if !self.config.disable_session_naming { let manager_for_spawn = session_manager.clone(); + let session_id_for_spawn = session_id.clone(); tokio::spawn(async move { if let Err(e) = manager_for_spawn - .maybe_update_name(&session_id, provider) + .maybe_update_name(&session_id_for_spawn, provider) .await { warn!("Failed to generate session description: {}", e); @@ -1097,6 +1165,44 @@ impl Agent { } let working_dir = session.working_dir.clone(); + + // Fire SessionStart hook (fires at the start of each reply, not once per session lifetime) + if let Some(ref hooks) = hooks { + let invocation = crate::hooks::HookInvocation::session_start( + session_id.clone(), + working_dir.to_string_lossy().to_string(), + ); + let _ = hooks + .run(invocation, &self.extension_manager, &working_dir) + .await; + } + + // Fire UserPromptSubmit hook with the last user message + if let Some(ref hooks) = hooks { + if let Some(last_user_msg) = conversation + .messages() + .iter() + .rev() + .find(|m| m.role == rmcp::model::Role::User) + { + let user_prompt = last_user_msg.as_concat_text(); + let invocation = crate::hooks::HookInvocation::user_prompt_submit( + session_id.clone(), + user_prompt, + working_dir.to_string_lossy().to_string(), + ); + let outcome = hooks + .run(invocation, &self.extension_manager, &working_dir) + .await + .unwrap_or_default(); + if outcome.blocked { + return Ok(Box::pin(async_stream::try_stream! { + yield AgentEvent::Message(Message::assistant().with_text("Prompt blocked by hook.")); + })); + } + } + } + Ok(Box::pin(async_stream::try_stream! { let reply_stream_span = tracing::info_span!(target: "goose::agents::agent", "reply_stream"); let _stream_guard = reply_stream_span.enter(); @@ -1218,9 +1324,11 @@ impl Agent { let mut request_to_response_map = HashMap::new(); let mut request_metadata: HashMap> = HashMap::new(); + let mut request_id_to_request: HashMap = HashMap::new(); for (idx, request) in frontend_requests.iter().chain(remaining_requests.iter()).enumerate() { request_to_response_map.insert(request.id.clone(), tool_response_messages[idx].clone()); request_metadata.insert(request.id.clone(), request.metadata.clone()); + request_id_to_request.insert(request.id.clone(), request.clone()); } for (idx, request) in frontend_requests.iter().enumerate() { @@ -1290,6 +1398,7 @@ impl Agent { &request_to_response_map, cancel_token.clone(), &session, + &hooks, ).await?; let tool_futures_arc = Arc::new(Mutex::new(tool_futures)); @@ -1301,6 +1410,7 @@ impl Agent { cancel_token.clone(), &session, &inspection_results, + &hooks, ); while let Some(msg) = tool_approval_stream.try_next().await? { @@ -1341,6 +1451,38 @@ impl Agent { ToolStreamItem::Result(output) => { let output = call_tool_result::validate(output); + // Fire PostToolUse or PostToolUseFailure hooks + if let Some(ref hooks) = hooks { + if let Some(original_request) = request_id_to_request.get(&request_id) { + if let Ok(ref tool_call) = original_request.tool_call { + let tool_input = serde_json::to_value(&tool_call.arguments).unwrap_or(serde_json::Value::Null); + match &output { + Ok(call_result) => { + let tool_output = serde_json::to_value(&call_result.content).unwrap_or(serde_json::Value::Null); + let invocation = crate::hooks::HookInvocation::post_tool_use( + session_config.id.clone(), + tool_call.name.to_string(), + tool_input, + tool_output, + working_dir.to_string_lossy().to_string(), + ); + let _ = hooks.run(invocation, &self.extension_manager, &working_dir).await; + } + Err(error_data) => { + let invocation = crate::hooks::HookInvocation::post_tool_use_failure( + session_config.id.clone(), + tool_call.name.to_string(), + tool_input, + error_data.message.to_string(), + working_dir.to_string_lossy().to_string(), + ); + let _ = hooks.run(invocation, &self.extension_manager, &working_dir).await; + } + } + } + } + } + if let Ok(ref call_result) = output { if let Some(ref meta) = call_result.meta { if let Some(notification_data) = meta.0.get("platform_notification") { @@ -1477,6 +1619,17 @@ impl Agent { ) ); + // Fire PreCompact hook (recovery compaction) + if let Some(ref hooks) = hooks { + let invocation = crate::hooks::HookInvocation::pre_compact( + session_config.id.clone(), + conversation.messages().len(), + false, // manual = false for auto recovery-compact + working_dir.to_string_lossy().to_string(), + ); + let _ = hooks.run(invocation, &self.extension_manager, &working_dir).await; + } + match compact_messages( self.provider().await?.as_ref(), &session_config.id, @@ -1485,12 +1638,37 @@ impl Agent { ) .await { - Ok((compacted_conversation, usage)) => { + Ok((mut compacted_conversation, usage)) => { + let pre_compact_len = conversation.messages().len(); + let post_compact_len = compacted_conversation.messages().len(); + session_manager.replace_conversation(&session_config.id, &compacted_conversation).await?; self.update_session_metrics(&session_config.id, session_config.schedule_id.clone(), &usage, true).await?; + + // Fire PostCompact hook (recovery compaction) + if let Some(ref hooks) = hooks { + let invocation = crate::hooks::HookInvocation::post_compact( + session_config.id.clone(), + pre_compact_len, + post_compact_len, + false, // manual = false for auto recovery-compact + working_dir.to_string_lossy().to_string(), + ); + if let Ok(outcome) = hooks.run(invocation, &self.extension_manager, &working_dir).await { + if let Some(context) = outcome.context { + let context_msg = Message::assistant() + .with_text(context) + .with_visibility(false, true); // agent-only + session_manager.add_message(&session_config.id, &context_msg).await?; + compacted_conversation.push(context_msg); + } + } + } + conversation = compacted_conversation; did_recovery_compact_this_iteration = true; yield AgentEvent::HistoryReplaced(conversation.clone()); + break; } Err(e) => { @@ -1618,6 +1796,16 @@ impl Agent { tokio::task::yield_now().await; } + // Fire Stop hook before finishing + if let Some(ref hooks) = hooks { + let invocation = crate::hooks::HookInvocation::stop( + session_id.clone(), + None, // reason + working_dir.to_string_lossy().to_string(), + ); + let _ = hooks.run(invocation, &self.extension_manager, &working_dir).await; + } + if !last_assistant_text.is_empty() { tracing::info!(target: "goose::agents::agent", trace_output = last_assistant_text.as_str()); } diff --git a/crates/goose/src/agents/execute_commands.rs b/crates/goose/src/agents/execute_commands.rs index 2c115249aa71..b1e3f1ec7a7d 100644 --- a/crates/goose/src/agents/execute_commands.rs +++ b/crates/goose/src/agents/execute_commands.rs @@ -86,6 +86,21 @@ impl Agent { .conversation .ok_or_else(|| anyhow!("Session has no conversation"))?; + // Load hooks and fire PreCompact + let hooks = crate::hooks::Hooks::load(&session.working_dir).ok(); + if let Some(ref hooks) = hooks { + let invocation = crate::hooks::HookInvocation::pre_compact( + session_id.to_string(), + conversation.messages().len(), + true, // manual = true for /compact command + session.working_dir.to_string_lossy().to_string(), + ); + let _ = hooks + .run(invocation, &self.extension_manager, &session.working_dir) + .await; + } + + let pre_compact_len = conversation.messages().len(); let (compacted_conversation, usage) = compact_messages( self.provider().await?.as_ref(), session_id, @@ -93,6 +108,7 @@ impl Agent { true, // is_manual_compact ) .await?; + let post_compact_len = compacted_conversation.messages().len(); manager .replace_conversation(session_id, &compacted_conversation) @@ -101,6 +117,28 @@ impl Agent { self.update_session_metrics(session_id, session.schedule_id, &usage, true) .await?; + // Fire PostCompact hook and inject context if any + if let Some(ref hooks) = hooks { + let invocation = crate::hooks::HookInvocation::post_compact( + session_id.to_string(), + pre_compact_len, + post_compact_len, + true, // manual = true for /compact command + session.working_dir.to_string_lossy().to_string(), + ); + if let Ok(outcome) = hooks + .run(invocation, &self.extension_manager, &session.working_dir) + .await + { + if let Some(context) = outcome.context { + let context_msg = Message::assistant() + .with_text(context) + .with_visibility(false, true); // agent-only + manager.add_message(session_id, &context_msg).await?; + } + } + } + Ok(Some(Message::assistant().with_system_notification( SystemNotificationType::InlineMessage, "Compaction complete", diff --git a/crates/goose/src/agents/tool_execution.rs b/crates/goose/src/agents/tool_execution.rs index b8813900b8a4..2287d5b221aa 100644 --- a/crates/goose/src/agents/tool_execution.rs +++ b/crates/goose/src/agents/tool_execution.rs @@ -49,6 +49,7 @@ pub const CHAT_MODE_TOOL_SKIPPED_RESPONSE: &str = "Let the user know the tool ca If needed, adjust the explanation based on user preferences or questions."; impl Agent { + #[allow(clippy::too_many_arguments)] pub(crate) fn handle_approval_tool_requests<'a>( &'a self, tool_requests: &'a [ToolRequest], @@ -57,6 +58,7 @@ impl Agent { cancellation_token: Option, session: &'a Session, inspection_results: &'a [crate::tool_inspection::InspectionResult], + hooks: &'a Option, ) -> BoxStream<'a, anyhow::Result> { try_stream! { for request in tool_requests.iter() { @@ -97,6 +99,38 @@ impl Agent { } if confirmation.permission == Permission::AllowOnce || confirmation.permission == Permission::AlwaysAllow { + // Fire PreToolUse hook — if blocked, treat as declined + if let Some(ref hooks) = hooks { + let invocation = crate::hooks::HookInvocation::pre_tool_use( + session.id.clone(), + tool_call.name.to_string(), + serde_json::to_value(&tool_call.arguments) + .unwrap_or(serde_json::Value::Null), + session.working_dir.to_string_lossy().to_string(), + ); + let outcome = hooks + .run(invocation, &self.extension_manager, &session.working_dir) + .await + .unwrap_or_default(); + if outcome.blocked { + // Hook blocked — treat same as user declining + if let Some(response_msg) = request_to_response_map.get(&request.id) { + let mut response = response_msg.lock().await; + *response = response.clone().with_tool_response_with_metadata( + request.id.clone(), + Ok(rmcp::model::CallToolResult { + content: vec![Content::text("Tool execution blocked by hook")], + structured_content: None, + is_error: Some(true), + meta: None, + }), + request.metadata.as_ref(), + ); + } + break; + } + } + let (req_id, tool_result) = self.dispatch_tool_call(tool_call.clone(), request.id.clone(), cancellation_token.clone(), session).await; let mut futures = tool_futures.lock().await; diff --git a/crates/goose/src/hooks/config.rs b/crates/goose/src/hooks/config.rs new file mode 100644 index 000000000000..30a1f5dffea6 --- /dev/null +++ b/crates/goose/src/hooks/config.rs @@ -0,0 +1,173 @@ +use anyhow::{Context, Result}; +use serde::Deserialize; +use std::collections::HashMap; +use std::path::Path; +use std::str::FromStr; + +use super::types::HookEventKind; + +#[derive(Debug, Clone, Default)] +pub struct HookSettingsFile { + pub hooks: HashMap>, + pub allow_project_hooks: bool, +} + +impl<'de> serde::Deserialize<'de> for HookSettingsFile { + fn deserialize>(deserializer: D) -> Result { + #[derive(Deserialize)] + struct Raw { + #[serde(default)] + hooks: HashMap>, + #[serde(default)] + allow_project_hooks: bool, + } + + let raw = Raw::deserialize(deserializer)?; + let mut hooks = HashMap::new(); + + for (key, configs) in raw.hooks { + match HookEventKind::from_str(&key) { + Ok(event) => { + hooks.insert(event, configs); + } + Err(_) => { + tracing::warn!("Unknown hook event '{}', ignoring", key); + } + } + } + + Ok(Self { + hooks, + allow_project_hooks: raw.allow_project_hooks, + }) + } +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct HookEventConfig { + #[serde(default)] + pub matcher: Option, + + pub hooks: Vec, +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(tag = "type", rename_all = "lowercase")] +pub enum HookAction { + Command { + command: String, + + #[serde(default = "default_timeout")] + timeout: u64, + }, + #[serde(rename = "mcp_tool")] + McpTool { + tool: String, + #[serde(default)] + arguments: serde_json::Map, + #[serde(default = "default_timeout")] + timeout: u64, + }, +} + +fn default_timeout() -> u64 { + 600 +} + +impl HookSettingsFile { + pub fn load_merged(working_dir: &Path) -> Result { + let global_path = crate::config::paths::Paths::in_config_dir("hooks.json"); + let goose_project_path = working_dir.join(".goose").join("settings.json"); + let claude_project_path = working_dir.join(".claude").join("settings.json"); + + let global = Self::load_from_file(&global_path).unwrap_or_else(|e| { + tracing::debug!("No global hooks config at {:?}: {}", global_path, e); + Self::default() + }); + + // Use the allow_project_hooks setting from the global config + let allow_project_hooks = global.allow_project_hooks; + + // If project hooks are not allowed, check if they exist and log a warning + if !allow_project_hooks { + let project_path = if goose_project_path.exists() { + Some(&goose_project_path) + } else if claude_project_path.exists() { + Some(&claude_project_path) + } else { + None + }; + + if let Some(path) = project_path { + tracing::info!( + "Project hooks found at {:?} but project hooks are not enabled. Set allow_project_hooks: true in ~/.config/goose/hooks.json to enable.", + path + ); + } + + return Ok(global); + } + + let project = if goose_project_path.exists() { + if claude_project_path.exists() { + tracing::warn!("Found hooks config in both .goose/ and .claude/; using .goose/"); + } + Self::load_from_file(&goose_project_path).unwrap_or_else(|e| { + tracing::warn!( + "Failed to parse hooks config {:?}: {}", + goose_project_path, + e + ); + Self::default() + }) + } else { + Self::load_from_file(&claude_project_path).unwrap_or_else(|e| { + if claude_project_path.exists() { + tracing::warn!( + "Failed to parse hooks config {:?}: {}", + claude_project_path, + e + ); + } + Self::default() + }) + }; + + Ok(Self::merge(global, project)) + } + + fn load_from_file(path: &Path) -> Result { + if !path.exists() { + anyhow::bail!("Config file does not exist: {:?}", path); + } + + let content = std::fs::read_to_string(path) + .with_context(|| format!("Failed to read hooks config from {:?}", path))?; + + let config: Self = serde_json::from_str(&content) + .with_context(|| format!("Failed to parse hooks config from {:?}", path))?; + + Ok(config) + } + + fn merge(global: Self, project: Self) -> Self { + let mut merged_hooks: HashMap> = global.hooks; + + for (event, project_configs) in project.hooks { + merged_hooks + .entry(event) + .or_default() + .extend(project_configs); + } + + Self { + hooks: merged_hooks, + allow_project_hooks: global.allow_project_hooks, + } + } + + pub fn get_hooks_for_event(&self, event: HookEventKind) -> &[HookEventConfig] { + self.hooks.get(&event).map(|v| v.as_slice()).unwrap_or(&[]) + } +} diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs new file mode 100644 index 000000000000..35cf40d8152a --- /dev/null +++ b/crates/goose/src/hooks/mod.rs @@ -0,0 +1,326 @@ +mod config; +pub mod types; + +pub use config::{HookAction, HookEventConfig, HookSettingsFile}; +pub use types::{HookDecision, HookEventKind, HookInvocation, HookResult, HooksOutcome}; + +use anyhow::Result; +use rmcp::model::{CallToolRequestParams, CallToolResult}; +use std::path::Path; +use std::time::Duration; +use tokio_util::sync::CancellationToken; + +pub struct Hooks { + settings: HookSettingsFile, +} + +impl Hooks { + pub fn load(working_dir: &Path) -> Result { + let settings = HookSettingsFile::load_merged(working_dir)?; + Ok(Self { settings }) + } + + pub async fn run( + &self, + invocation: HookInvocation, + extension_manager: &crate::agents::extension_manager::ExtensionManager, + working_dir: &Path, + ) -> Result { + let event_configs = self.settings.get_hooks_for_event(invocation.event); + + let mut outcome = HooksOutcome::default(); + let mut contexts = Vec::new(); + + for config in event_configs { + if !Self::matches_config(config, &invocation) { + continue; + } + + for action in &config.hooks { + match Self::execute_action(action, &invocation, extension_manager, working_dir) + .await + { + Ok(Some(result)) => { + if let Some(HookDecision::Block) = result.decision { + if invocation.event.can_block() { + outcome.blocked = true; + tracing::info!("Hook blocked event {:?}", invocation.event); + return Ok(outcome); + } + tracing::warn!( + "Hook returned Block for non-blockable event {:?}, ignoring", + invocation.event + ); + } + + if let Some(context) = result.additional_context { + contexts.push(context); + } + } + Ok(None) => { + tracing::debug!("Hook returned no result, continuing"); + } + Err(e) => { + tracing::warn!("Hook execution failed: {}, continuing", e); + } + } + } + } + + if !contexts.is_empty() { + outcome.context = Some(contexts.join("\n")); + } + + Ok(outcome) + } + + pub async fn fire( + &self, + invocation: HookInvocation, + extension_manager: &crate::agents::extension_manager::ExtensionManager, + working_dir: &Path, + ) { + if let Err(e) = self.run(invocation, extension_manager, working_dir).await { + tracing::warn!("Hook fire failed: {}", e); + } + } + + // Dispatches hook actions directly via ExtensionManager, bypassing tool inspection + // and approval prompts. This is intentional: hooks are a privileged execution path + // configured by the user (global) or opted-in (project). Running hooks through the + // normal tool pipeline would cause infinite recursion (PreToolUse → hook → tool → PreToolUse). + async fn execute_action( + action: &HookAction, + invocation: &HookInvocation, + extension_manager: &crate::agents::extension_manager::ExtensionManager, + working_dir: &Path, + ) -> Result> { + let (tool_call, timeout_secs) = Self::build_tool_call(action, invocation)?; + let cancel_token = CancellationToken::new(); + + let tool_call_result = extension_manager + .dispatch_tool_call( + &invocation.session_id, + tool_call, + Some(working_dir), + cancel_token.clone(), + ) + .await?; + + let result = + tokio::time::timeout(Duration::from_secs(timeout_secs), tool_call_result.result).await; + + match result { + Ok(Ok(call_result)) => Self::parse_result(call_result, action, invocation.event), + Ok(Err(e)) => { + tracing::warn!("Hook tool call failed: {}, failing open", e); + Ok(None) + } + Err(_) => { + cancel_token.cancel(); + tracing::warn!("Hook timed out after {}s, failing open", timeout_secs); + Ok(None) + } + } + } + + fn build_tool_call( + action: &HookAction, + invocation: &HookInvocation, + ) -> Result<(CallToolRequestParams, u64)> { + match action { + HookAction::Command { command, timeout } => { + let json = serde_json::to_string(invocation)?; + let escaped = json.replace('\'', "'\\''"); + let shell_cmd = format!( + "printf '%s' '{}' | {}; printf '\\nGOOSE_HOOK_EXIT:%d' $?", + escaped, command + ); + + let args = serde_json::json!({"command": shell_cmd}); + Ok(( + CallToolRequestParams { + meta: None, + task: None, + name: "developer__shell".into(), + arguments: args.as_object().cloned(), + }, + *timeout, + )) + } + HookAction::McpTool { + tool, + arguments, + timeout, + } => Ok(( + CallToolRequestParams { + meta: None, + task: None, + name: tool.clone().into(), + arguments: Some(arguments.clone()), + }, + *timeout, + )), + } + } + + fn parse_result( + result: CallToolResult, + action: &HookAction, + event: HookEventKind, + ) -> Result> { + if result.is_error.unwrap_or(false) { + tracing::warn!("Hook tool returned error, failing open"); + return Ok(None); + } + + let text = result + .content + .iter() + .filter_map(|c| c.as_text().map(|t| t.text.as_str())) + .collect::>() + .join(""); + + match action { + HookAction::Command { .. } => { + if text.is_empty() { + return Ok(Some(HookResult::default())); + } + + if let Some(exit_marker) = text.rfind("GOOSE_HOOK_EXIT:") { + let (output, exit_part) = text.split_at(exit_marker); + if let Some(code_str) = exit_part.strip_prefix("GOOSE_HOOK_EXIT:") { + if let Ok(code) = code_str + .split_whitespace() + .next() + .unwrap_or("") + .parse::() + { + return match code { + 0 => { + if output.trim().is_empty() { + Ok(Some(HookResult::default())) + } else { + Ok(serde_json::from_str(output.trim()) + .map(Some) + .unwrap_or_else(|_| { + // Non-JSON stdout from exit-0 command → surface as context (Claude Code compat) + let mut context = output.trim().to_string(); + if context.len() > 32_768 { + tracing::warn!( + "Hook stdout truncated from {} to 32KB", + context.len() + ); + context.truncate( + context.floor_char_boundary(32_768), + ); + } + Some(HookResult { + additional_context: Some(context), + ..Default::default() + }) + })) + } + } + 2 if event.can_block() => Ok(Some(HookResult { + decision: Some(HookDecision::Block), + ..Default::default() + })), + _ => Ok(None), + }; + } + } + } + + // No exit marker — can't confirm exit 0, so don't surface raw output + Ok(serde_json::from_str(text.trim()) + .map(Some) + .unwrap_or_else(|e| { + tracing::debug!("Hook output is not JSON (no exit marker): {}", e); + None + })) + } + HookAction::McpTool { .. } => { + if text.trim().is_empty() { + Ok(Some(HookResult::default())) + } else { + Ok(serde_json::from_str(text.trim()) + .map(Some) + .unwrap_or_else(|e| { + tracing::debug!("Hook output is not HookResult JSON: {}", e); + None + })) + } + } + } + } + + fn matches_config(config: &HookEventConfig, invocation: &HookInvocation) -> bool { + let Some(pattern) = &config.matcher else { + return true; + }; + + use HookEventKind::*; + match invocation.event { + PreToolUse | PostToolUse | PostToolUseFailure | PermissionRequest => { + Self::matches_tool(pattern, invocation) + } + Notification => invocation + .notification_type + .as_ref() + .is_some_and(|t| t.contains(pattern)), + PreCompact | PostCompact => invocation.manual_compact.is_some_and(|manual| { + (manual && pattern == "manual") || (!manual && pattern == "auto") + }), + _ => true, + } + } + + /// Match a tool invocation against a Claude Code-style matcher pattern. + /// Supports: + /// "Bash" or "Bash(...)" — maps to developer__shell, optionally matching command content + /// "tool_name" — direct tool name substring match (goose-native) + fn matches_tool(pattern: &str, invocation: &HookInvocation) -> bool { + let tool_name = match &invocation.tool_name { + Some(name) => name, + None => return false, + }; + + // Claude Code "Bash" / "Bash(pattern)" syntax + if pattern == "Bash" { + return tool_name.contains("shell"); + } + + if let Some(inner) = pattern + .strip_prefix("Bash(") + .and_then(|s| s.strip_suffix(')')) + { + if !tool_name.contains("shell") { + return false; + } + // Match the inner pattern against the command argument + let command_str = invocation + .tool_input + .as_ref() + .and_then(|v| v.get("command")) + .and_then(|v| v.as_str()) + .unwrap_or(""); + + return Self::glob_match(inner, command_str); + } + + // Direct tool name match (goose-native: "developer__shell", "slack__post_message", etc.) + tool_name.contains(pattern) + } + + /// Simple glob matching: only supports trailing * (prefix match). + /// "git push*" matches "git push origin main" + /// "git push" matches exactly "git push" + fn glob_match(pattern: &str, text: &str) -> bool { + if let Some(prefix) = pattern.strip_suffix('*') { + text.starts_with(prefix) + } else { + text == pattern + } + } +} diff --git a/crates/goose/src/hooks/types.rs b/crates/goose/src/hooks/types.rs new file mode 100644 index 000000000000..878c73460897 --- /dev/null +++ b/crates/goose/src/hooks/types.rs @@ -0,0 +1,301 @@ +use serde::{Deserialize, Serialize}; +use serde_json::Value; + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, Default)] +#[serde(rename_all = "PascalCase")] +pub enum HookEventKind { + #[default] + SessionStart, + PreToolUse, + PostToolUse, + PostToolUseFailure, + UserPromptSubmit, + Stop, + SubagentStop, + SubagentStart, + SessionEnd, + PreCompact, + PostCompact, + Notification, + PermissionRequest, + TeammateIdle, + TaskCompleted, + ConfigChange, +} + +impl HookEventKind { + pub fn can_block(&self) -> bool { + matches!( + self, + Self::PreToolUse + | Self::PermissionRequest + | Self::UserPromptSubmit + | Self::Stop + | Self::SubagentStop + | Self::TeammateIdle + | Self::TaskCompleted + | Self::ConfigChange + ) + } +} + +impl std::str::FromStr for HookEventKind { + type Err = String; + + fn from_str(s: &str) -> Result { + serde_json::from_value(serde_json::Value::String(s.to_string())) + .map_err(|e| format!("unknown hook event '{}': {}", s, e)) + } +} + +#[derive(Debug, Clone, Serialize, Default)] +#[serde(rename_all = "snake_case")] +pub struct HookInvocation { + #[serde(rename = "hook_event_name")] + pub event: HookEventKind, + pub session_id: String, + + #[serde(skip_serializing_if = "Option::is_none")] + pub cwd: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub tool_name: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub tool_input: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub tool_output: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub tool_error: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub user_prompt: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub notification_type: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub reason: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub message_count_before: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub message_count_after: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub manual_compact: Option, +} + +impl HookInvocation { + fn base(event: HookEventKind, session_id: String) -> Self { + Self { + event, + session_id, + ..Default::default() + } + } + + pub fn pre_tool_use( + session_id: String, + tool_name: String, + tool_input: Value, + cwd: String, + ) -> Self { + Self { + tool_name: Some(tool_name), + tool_input: Some(tool_input), + cwd: Some(cwd), + ..Self::base(HookEventKind::PreToolUse, session_id) + } + } + + pub fn post_tool_use( + session_id: String, + tool_name: String, + tool_input: Value, + tool_output: Value, + cwd: String, + ) -> Self { + Self { + tool_name: Some(tool_name), + tool_input: Some(tool_input), + tool_output: Some(tool_output), + cwd: Some(cwd), + ..Self::base(HookEventKind::PostToolUse, session_id) + } + } + + pub fn post_tool_use_failure( + session_id: String, + tool_name: String, + tool_input: Value, + tool_error: String, + cwd: String, + ) -> Self { + Self { + tool_name: Some(tool_name), + tool_input: Some(tool_input), + tool_error: Some(tool_error), + cwd: Some(cwd), + ..Self::base(HookEventKind::PostToolUseFailure, session_id) + } + } + + pub fn user_prompt_submit(session_id: String, user_prompt: String, cwd: String) -> Self { + Self { + user_prompt: Some(user_prompt), + cwd: Some(cwd), + ..Self::base(HookEventKind::UserPromptSubmit, session_id) + } + } + + pub fn session_start(session_id: String, cwd: String) -> Self { + Self { + cwd: Some(cwd), + ..Self::base(HookEventKind::SessionStart, session_id) + } + } + + pub fn session_end(session_id: String, reason: Option) -> Self { + Self { + reason, + ..Self::base(HookEventKind::SessionEnd, session_id) + } + } + + pub fn stop(session_id: String, reason: Option, cwd: String) -> Self { + Self { + reason, + cwd: Some(cwd), + ..Self::base(HookEventKind::Stop, session_id) + } + } + + pub fn subagent_start(session_id: String, cwd: String) -> Self { + Self { + cwd: Some(cwd), + ..Self::base(HookEventKind::SubagentStart, session_id) + } + } + + pub fn subagent_stop(session_id: String, reason: Option) -> Self { + Self { + reason, + ..Self::base(HookEventKind::SubagentStop, session_id) + } + } + + pub fn pre_compact( + session_id: String, + message_count_before: usize, + manual: bool, + cwd: String, + ) -> Self { + Self { + message_count_before: Some(message_count_before), + manual_compact: Some(manual), + cwd: Some(cwd), + ..Self::base(HookEventKind::PreCompact, session_id) + } + } + + pub fn post_compact( + session_id: String, + message_count_before: usize, + message_count_after: usize, + manual: bool, + cwd: String, + ) -> Self { + Self { + message_count_before: Some(message_count_before), + message_count_after: Some(message_count_after), + manual_compact: Some(manual), + cwd: Some(cwd), + ..Self::base(HookEventKind::PostCompact, session_id) + } + } + + pub fn permission_request( + session_id: String, + tool_name: String, + tool_input: Value, + cwd: String, + ) -> Self { + Self { + tool_name: Some(tool_name), + tool_input: Some(tool_input), + cwd: Some(cwd), + ..Self::base(HookEventKind::PermissionRequest, session_id) + } + } + + pub fn notification(session_id: String, notification_type: String, cwd: String) -> Self { + Self { + notification_type: Some(notification_type), + cwd: Some(cwd), + ..Self::base(HookEventKind::Notification, session_id) + } + } + + pub fn teammate_idle(session_id: String, cwd: String) -> Self { + Self { + cwd: Some(cwd), + ..Self::base(HookEventKind::TeammateIdle, session_id) + } + } + + pub fn task_completed(session_id: String, cwd: String) -> Self { + Self { + cwd: Some(cwd), + ..Self::base(HookEventKind::TaskCompleted, session_id) + } + } + + pub fn config_change(session_id: String, cwd: String) -> Self { + Self { + cwd: Some(cwd), + ..Self::base(HookEventKind::ConfigChange, session_id) + } + } +} + +#[derive(Debug, Clone, Default, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct HookResult { + #[serde(default)] + pub decision: Option, + + #[serde(default)] + pub reason: Option, + + #[serde(default)] + pub hook_specific_output: Option, + + #[serde(default, rename = "continue")] + pub continue_: Option, + + #[serde(default)] + pub stop_reason: Option, + + #[serde(default)] + pub additional_context: Option, + + #[serde(default)] + pub system_message: Option, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum HookDecision { + Allow, + Block, +} + +#[derive(Debug, Default)] +pub struct HooksOutcome { + pub blocked: bool, + pub context: Option, +} diff --git a/crates/goose/src/lib.rs b/crates/goose/src/lib.rs index 322927d093ec..623a4eb7b322 100644 --- a/crates/goose/src/lib.rs +++ b/crates/goose/src/lib.rs @@ -9,6 +9,7 @@ pub mod download_manager; pub mod execution; pub mod goose_apps; pub mod hints; +pub mod hooks; pub mod logging; pub mod mcp_utils; pub mod model; From e4b822bd556a59fcc581d1be01976e0bd09ec9a6 Mon Sep 17 00:00:00 2001 From: Tyler Longwell Date: Sat, 21 Feb 2026 20:48:58 -0500 Subject: [PATCH 02/10] fix: crossfire review fixes for hooks implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses issues identified in multi-round crossfire review: - Remove Option wrapping; Hooks::load() returns empty default on error, eliminating 12 if-let-Some guards across agent integration - Extract run_post_compact_hook() helper to deduplicate 3 identical PostCompact context-injection blocks - Fix SessionStart to fire once per session (first user message) instead of on every reply - Thread parent CancellationToken through all hooks.run() calls so session cancellation properly cancels running hooks - Standardize blocked-tool errors on Err(ErrorData) — Ok(CallToolResult {is_error:true}) was silently treated as success by LLM formatters - Tighten tool name matching from contains() to exact equality - Replace hand-rolled trailing-* glob with glob crate for full pattern support - Change manual_compact from Option to bool (always set for compact events, defaults false otherwise) - Delete unused Hooks::fire() method (all call sites use run()) --- Cargo.lock | 1 + crates/goose/Cargo.toml | 2 + crates/goose/src/agents/agent.rs | 362 ++++++++++++-------- crates/goose/src/agents/execute_commands.rs | 68 ++-- crates/goose/src/agents/tool_execution.rs | 61 ++-- crates/goose/src/hooks/mod.rs | 85 +++-- crates/goose/src/hooks/types.rs | 8 +- 7 files changed, 332 insertions(+), 255 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c5409c4e14a0..22e2e142de2d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4258,6 +4258,7 @@ dependencies = [ "etcetera 0.11.0", "fs2", "futures", + "glob", "goose-mcp", "goose-test-support", "hf-hub", diff --git a/crates/goose/Cargo.toml b/crates/goose/Cargo.toml index a0de55ef8f08..790bb5798a1f 100644 --- a/crates/goose/Cargo.toml +++ b/crates/goose/Cargo.toml @@ -16,6 +16,8 @@ workspace = true [dependencies] lru = { workspace = true } +glob = "0.3" + rmcp = { workspace = true, features = [ "client", "reqwest", diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 54029121bce8..13f0ca15cd78 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -35,6 +35,7 @@ use crate::conversation::message::{ }; use crate::conversation::tool_result_serde::call_tool_result; use crate::conversation::{debug_conversation_fix, fix_conversation, Conversation}; +use crate::hooks::Hooks; use crate::mcp_utils::ToolResult; use crate::permission::permission_inspector::PermissionInspector; use crate::permission::permission_judge::PermissionCheckResult; @@ -386,7 +387,7 @@ impl Agent { request_to_response_map: &HashMap>>, cancel_token: Option, session: &Session, - hooks: &Option, + hooks: &Hooks, ) -> Result> { let mut tool_futures: Vec<(String, ToolStream)> = Vec::new(); @@ -394,34 +395,36 @@ impl Agent { for request in &permission_check_result.approved { if let Ok(tool_call) = request.tool_call.clone() { // Fire PreToolUse hook - if let Some(ref hooks) = hooks { - let invocation = crate::hooks::HookInvocation::pre_tool_use( - session.id.clone(), - tool_call.name.to_string(), - serde_json::to_value(&tool_call.arguments) - .unwrap_or(serde_json::Value::Null), - session.working_dir.to_string_lossy().to_string(), - ); - let outcome = hooks - .run(invocation, &self.extension_manager, &session.working_dir) - .await - .unwrap_or_default(); - if outcome.blocked { - // Create an error response instead of dispatching - let error_result = Err(ErrorData::new( - ErrorCode::INTERNAL_ERROR, - "Tool execution blocked by hook".to_string(), - None, - )); - tool_futures.push(( - request.id.clone(), - tool_stream( - Box::new(stream::empty()), - futures::future::ready(error_result), - ), - )); - continue; - } + let invocation = crate::hooks::HookInvocation::pre_tool_use( + session.id.clone(), + tool_call.name.to_string(), + serde_json::to_value(&tool_call.arguments).unwrap_or(serde_json::Value::Null), + session.working_dir.to_string_lossy().to_string(), + ); + let outcome = hooks + .run( + invocation, + &self.extension_manager, + &session.working_dir, + cancel_token.clone().unwrap_or_default(), + ) + .await + .unwrap_or_default(); + if outcome.blocked { + // Create an error response instead of dispatching + let error_result = Err(ErrorData::new( + ErrorCode::INTERNAL_ERROR, + "Tool execution blocked by hook".to_string(), + None, + )); + tool_futures.push(( + request.id.clone(), + tool_stream( + Box::new(stream::empty()), + futures::future::ready(error_result), + ), + )); + continue; } let (req_id, tool_result) = self @@ -454,6 +457,48 @@ impl Agent { Ok(tool_futures) } + #[allow(clippy::too_many_arguments)] + pub(crate) async fn run_post_compact_hook( + &self, + hooks: &Hooks, + session_id: &str, + pre_compact_len: usize, + post_compact_len: usize, + manual: bool, + working_dir: &std::path::Path, + session_manager: &SessionManager, + conversation: &mut crate::conversation::Conversation, + cancel_token: CancellationToken, + ) -> Result<()> { + let invocation = crate::hooks::HookInvocation::post_compact( + session_id.to_string(), + pre_compact_len, + post_compact_len, + manual, + working_dir.to_string_lossy().to_string(), + ); + if let Ok(outcome) = hooks + .run( + invocation, + &self.extension_manager, + working_dir, + cancel_token, + ) + .await + { + if let Some(context) = outcome.context { + let context_msg = Message::assistant() + .with_text(context) + .with_visibility(false, true); + session_manager + .add_message(session_id, &context_msg) + .await?; + conversation.push(context_msg); + } + } + Ok(()) + } + async fn handle_denied_tools( permission_check_result: &PermissionCheckResult, request_to_response_map: &HashMap>>, @@ -1013,7 +1058,7 @@ impl Agent { .ok_or_else(|| anyhow::anyhow!("Session {} has no conversation", session_config.id))?; // Load hooks configuration - let hooks = crate::hooks::Hooks::load(&session.working_dir).ok(); + let hooks = crate::hooks::Hooks::load(&session.working_dir); let needs_auto_compact = check_if_compaction_needed( self.provider().await?.as_ref(), @@ -1055,15 +1100,20 @@ impl Agent { ); // Fire PreCompact hook - if let Some(ref hooks) = hooks { - let invocation = crate::hooks::HookInvocation::pre_compact( - session_config.id.clone(), - conversation_to_compact.messages().len(), - false, // manual = false for auto-compact - session.working_dir.to_string_lossy().to_string(), - ); - let _ = hooks.run(invocation, &self.extension_manager, &session.working_dir).await; - } + let invocation = crate::hooks::HookInvocation::pre_compact( + session_config.id.clone(), + conversation_to_compact.messages().len(), + false, // manual = false for auto-compact + session.working_dir.to_string_lossy().to_string(), + ); + let _ = hooks + .run( + invocation, + &self.extension_manager, + &session.working_dir, + cancel_token.clone().unwrap_or_default(), + ) + .await; match compact_messages( self.provider().await?.as_ref(), @@ -1077,25 +1127,18 @@ impl Agent { session_manager.replace_conversation(&session_config.id, &compacted_conversation).await?; self.update_session_metrics(&session_config.id, session_config.schedule_id.clone(), &summarization_usage, true).await?; - // Fire PostCompact hook and inject additional context if any - if let Some(ref hooks) = hooks { - let invocation = crate::hooks::HookInvocation::post_compact( - session_config.id.clone(), - conversation_to_compact.messages().len(), - compacted_conversation.messages().len(), - false, // manual = false for auto-compact - session.working_dir.to_string_lossy().to_string(), - ); - if let Ok(outcome) = hooks.run(invocation, &self.extension_manager, &session.working_dir).await { - if let Some(context) = outcome.context { - let context_msg = Message::assistant() - .with_text(context) - .with_visibility(false, true); // agent-only - session_manager.add_message(&session_config.id, &context_msg).await?; - compacted_conversation.push(context_msg); - } - } - } + self.run_post_compact_hook( + &hooks, + &session_config.id, + conversation_to_compact.messages().len(), + compacted_conversation.messages().len(), + false, + &session.working_dir, + &session_manager, + &mut compacted_conversation, + cancel_token.clone().unwrap_or_default(), + ) + .await?; yield AgentEvent::HistoryReplaced(compacted_conversation.clone()); @@ -1132,7 +1175,7 @@ impl Agent { session_config: SessionConfig, session: Session, cancel_token: Option, - hooks: Option, + hooks: Hooks, ) -> Result>> { let context = self .prepare_reply_context(&session.id, conversation, session.working_dir.as_path()) @@ -1166,40 +1209,50 @@ impl Agent { let working_dir = session.working_dir.clone(); - // Fire SessionStart hook (fires at the start of each reply, not once per session lifetime) - if let Some(ref hooks) = hooks { + // Fire SessionStart hook on first reply only (new session: exactly 1 user message, no assistant response yet) + if conversation.messages().len() == 1 + && conversation.messages()[0].role == rmcp::model::Role::User + { let invocation = crate::hooks::HookInvocation::session_start( session_id.clone(), working_dir.to_string_lossy().to_string(), ); let _ = hooks - .run(invocation, &self.extension_manager, &working_dir) + .run( + invocation, + &self.extension_manager, + &working_dir, + cancel_token.clone().unwrap_or_default(), + ) .await; } // Fire UserPromptSubmit hook with the last user message - if let Some(ref hooks) = hooks { - if let Some(last_user_msg) = conversation - .messages() - .iter() - .rev() - .find(|m| m.role == rmcp::model::Role::User) - { - let user_prompt = last_user_msg.as_concat_text(); - let invocation = crate::hooks::HookInvocation::user_prompt_submit( - session_id.clone(), - user_prompt, - working_dir.to_string_lossy().to_string(), - ); - let outcome = hooks - .run(invocation, &self.extension_manager, &working_dir) - .await - .unwrap_or_default(); - if outcome.blocked { - return Ok(Box::pin(async_stream::try_stream! { - yield AgentEvent::Message(Message::assistant().with_text("Prompt blocked by hook.")); - })); - } + if let Some(last_user_msg) = conversation + .messages() + .iter() + .rev() + .find(|m| m.role == rmcp::model::Role::User) + { + let user_prompt = last_user_msg.as_concat_text(); + let invocation = crate::hooks::HookInvocation::user_prompt_submit( + session_id.clone(), + user_prompt, + working_dir.to_string_lossy().to_string(), + ); + let outcome = hooks + .run( + invocation, + &self.extension_manager, + &working_dir, + cancel_token.clone().unwrap_or_default(), + ) + .await + .unwrap_or_default(); + if outcome.blocked { + return Ok(Box::pin(async_stream::try_stream! { + yield AgentEvent::Message(Message::assistant().with_text("Prompt blocked by hook.")); + })); } } @@ -1452,32 +1505,46 @@ impl Agent { let output = call_tool_result::validate(output); // Fire PostToolUse or PostToolUseFailure hooks - if let Some(ref hooks) = hooks { - if let Some(original_request) = request_id_to_request.get(&request_id) { - if let Ok(ref tool_call) = original_request.tool_call { - let tool_input = serde_json::to_value(&tool_call.arguments).unwrap_or(serde_json::Value::Null); - match &output { - Ok(call_result) => { - let tool_output = serde_json::to_value(&call_result.content).unwrap_or(serde_json::Value::Null); - let invocation = crate::hooks::HookInvocation::post_tool_use( - session_config.id.clone(), - tool_call.name.to_string(), - tool_input, - tool_output, - working_dir.to_string_lossy().to_string(), - ); - let _ = hooks.run(invocation, &self.extension_manager, &working_dir).await; - } - Err(error_data) => { - let invocation = crate::hooks::HookInvocation::post_tool_use_failure( - session_config.id.clone(), - tool_call.name.to_string(), - tool_input, - error_data.message.to_string(), - working_dir.to_string_lossy().to_string(), - ); - let _ = hooks.run(invocation, &self.extension_manager, &working_dir).await; - } + if let Some(original_request) = request_id_to_request.get(&request_id) { + if let Ok(ref tool_call) = original_request.tool_call { + let tool_input = serde_json::to_value(&tool_call.arguments) + .unwrap_or(serde_json::Value::Null); + match &output { + Ok(call_result) => { + let tool_output = serde_json::to_value(&call_result.content) + .unwrap_or(serde_json::Value::Null); + let invocation = crate::hooks::HookInvocation::post_tool_use( + session_config.id.clone(), + tool_call.name.to_string(), + tool_input, + tool_output, + working_dir.to_string_lossy().to_string(), + ); + let _ = hooks + .run( + invocation, + &self.extension_manager, + &working_dir, + cancel_token.clone().unwrap_or_default(), + ) + .await; + } + Err(error_data) => { + let invocation = crate::hooks::HookInvocation::post_tool_use_failure( + session_config.id.clone(), + tool_call.name.to_string(), + tool_input, + error_data.message.to_string(), + working_dir.to_string_lossy().to_string(), + ); + let _ = hooks + .run( + invocation, + &self.extension_manager, + &working_dir, + cancel_token.clone().unwrap_or_default(), + ) + .await; } } } @@ -1620,15 +1687,20 @@ impl Agent { ); // Fire PreCompact hook (recovery compaction) - if let Some(ref hooks) = hooks { - let invocation = crate::hooks::HookInvocation::pre_compact( - session_config.id.clone(), - conversation.messages().len(), - false, // manual = false for auto recovery-compact - working_dir.to_string_lossy().to_string(), - ); - let _ = hooks.run(invocation, &self.extension_manager, &working_dir).await; - } + let invocation = crate::hooks::HookInvocation::pre_compact( + session_config.id.clone(), + conversation.messages().len(), + false, // manual = false for auto recovery-compact + working_dir.to_string_lossy().to_string(), + ); + let _ = hooks + .run( + invocation, + &self.extension_manager, + &working_dir, + cancel_token.clone().unwrap_or_default(), + ) + .await; match compact_messages( self.provider().await?.as_ref(), @@ -1645,25 +1717,18 @@ impl Agent { session_manager.replace_conversation(&session_config.id, &compacted_conversation).await?; self.update_session_metrics(&session_config.id, session_config.schedule_id.clone(), &usage, true).await?; - // Fire PostCompact hook (recovery compaction) - if let Some(ref hooks) = hooks { - let invocation = crate::hooks::HookInvocation::post_compact( - session_config.id.clone(), - pre_compact_len, - post_compact_len, - false, // manual = false for auto recovery-compact - working_dir.to_string_lossy().to_string(), - ); - if let Ok(outcome) = hooks.run(invocation, &self.extension_manager, &working_dir).await { - if let Some(context) = outcome.context { - let context_msg = Message::assistant() - .with_text(context) - .with_visibility(false, true); // agent-only - session_manager.add_message(&session_config.id, &context_msg).await?; - compacted_conversation.push(context_msg); - } - } - } + self.run_post_compact_hook( + &hooks, + &session_config.id, + pre_compact_len, + post_compact_len, + false, + &working_dir, + &session_manager, + &mut compacted_conversation, + cancel_token.clone().unwrap_or_default(), + ) + .await?; conversation = compacted_conversation; did_recovery_compact_this_iteration = true; @@ -1797,14 +1862,19 @@ impl Agent { } // Fire Stop hook before finishing - if let Some(ref hooks) = hooks { - let invocation = crate::hooks::HookInvocation::stop( - session_id.clone(), - None, // reason - working_dir.to_string_lossy().to_string(), - ); - let _ = hooks.run(invocation, &self.extension_manager, &working_dir).await; - } + let invocation = crate::hooks::HookInvocation::stop( + session_id.clone(), + None, // reason + working_dir.to_string_lossy().to_string(), + ); + let _ = hooks + .run( + invocation, + &self.extension_manager, + &working_dir, + cancel_token.clone().unwrap_or_default(), + ) + .await; if !last_assistant_text.is_empty() { tracing::info!(target: "goose::agents::agent", trace_output = last_assistant_text.as_str()); diff --git a/crates/goose/src/agents/execute_commands.rs b/crates/goose/src/agents/execute_commands.rs index b1e3f1ec7a7d..06c805c185e7 100644 --- a/crates/goose/src/agents/execute_commands.rs +++ b/crates/goose/src/agents/execute_commands.rs @@ -4,8 +4,11 @@ use anyhow::{anyhow, Result}; use crate::context_mgmt::compact_messages; use crate::conversation::message::{Message, SystemNotificationType}; +use crate::hooks::Hooks; use crate::recipe::build_recipe::build_recipe_from_template_with_positional_params; +use tokio_util::sync::CancellationToken; + use super::Agent; pub const COMPACT_TRIGGERS: &[&str] = @@ -87,18 +90,21 @@ impl Agent { .ok_or_else(|| anyhow!("Session has no conversation"))?; // Load hooks and fire PreCompact - let hooks = crate::hooks::Hooks::load(&session.working_dir).ok(); - if let Some(ref hooks) = hooks { - let invocation = crate::hooks::HookInvocation::pre_compact( - session_id.to_string(), - conversation.messages().len(), - true, // manual = true for /compact command - session.working_dir.to_string_lossy().to_string(), - ); - let _ = hooks - .run(invocation, &self.extension_manager, &session.working_dir) - .await; - } + let hooks = Hooks::load(&session.working_dir); + let invocation = crate::hooks::HookInvocation::pre_compact( + session_id.to_string(), + conversation.messages().len(), + true, // manual = true for /compact command + session.working_dir.to_string_lossy().to_string(), + ); + let _ = hooks + .run( + invocation, + &self.extension_manager, + &session.working_dir, + CancellationToken::new(), + ) + .await; let pre_compact_len = conversation.messages().len(); let (compacted_conversation, usage) = compact_messages( @@ -117,27 +123,23 @@ impl Agent { self.update_session_metrics(session_id, session.schedule_id, &usage, true) .await?; - // Fire PostCompact hook and inject context if any - if let Some(ref hooks) = hooks { - let invocation = crate::hooks::HookInvocation::post_compact( - session_id.to_string(), - pre_compact_len, - post_compact_len, - true, // manual = true for /compact command - session.working_dir.to_string_lossy().to_string(), - ); - if let Ok(outcome) = hooks - .run(invocation, &self.extension_manager, &session.working_dir) - .await - { - if let Some(context) = outcome.context { - let context_msg = Message::assistant() - .with_text(context) - .with_visibility(false, true); // agent-only - manager.add_message(session_id, &context_msg).await?; - } - } - } + // Fire PostCompact hook and inject context if any. + // The helper also pushes to the in-memory conversation, but we don't need + // that here — session persistence happens via session_manager.add_message() + // inside the helper, and this conversation is about to be dropped. + let mut compacted_conversation = compacted_conversation; + self.run_post_compact_hook( + &hooks, + session_id, + pre_compact_len, + post_compact_len, + true, + &session.working_dir, + &manager, + &mut compacted_conversation, + CancellationToken::new(), + ) + .await?; Ok(Some(Message::assistant().with_system_notification( SystemNotificationType::InlineMessage, diff --git a/crates/goose/src/agents/tool_execution.rs b/crates/goose/src/agents/tool_execution.rs index 2287d5b221aa..bafd15e439d0 100644 --- a/crates/goose/src/agents/tool_execution.rs +++ b/crates/goose/src/agents/tool_execution.rs @@ -9,6 +9,7 @@ use tokio::sync::Mutex; use tokio_util::sync::CancellationToken; use crate::config::permission::PermissionLevel; +use crate::hooks::Hooks; use crate::mcp_utils::ToolResult; use crate::permission::Permission; use rmcp::model::{Content, ServerNotification}; @@ -58,7 +59,7 @@ impl Agent { cancellation_token: Option, session: &'a Session, inspection_results: &'a [crate::tool_inspection::InspectionResult], - hooks: &'a Option, + hooks: &'a Hooks, ) -> BoxStream<'a, anyhow::Result> { try_stream! { for request in tool_requests.iter() { @@ -100,35 +101,37 @@ impl Agent { if confirmation.permission == Permission::AllowOnce || confirmation.permission == Permission::AlwaysAllow { // Fire PreToolUse hook — if blocked, treat as declined - if let Some(ref hooks) = hooks { - let invocation = crate::hooks::HookInvocation::pre_tool_use( - session.id.clone(), - tool_call.name.to_string(), - serde_json::to_value(&tool_call.arguments) - .unwrap_or(serde_json::Value::Null), - session.working_dir.to_string_lossy().to_string(), - ); - let outcome = hooks - .run(invocation, &self.extension_manager, &session.working_dir) - .await - .unwrap_or_default(); - if outcome.blocked { - // Hook blocked — treat same as user declining - if let Some(response_msg) = request_to_response_map.get(&request.id) { - let mut response = response_msg.lock().await; - *response = response.clone().with_tool_response_with_metadata( - request.id.clone(), - Ok(rmcp::model::CallToolResult { - content: vec![Content::text("Tool execution blocked by hook")], - structured_content: None, - is_error: Some(true), - meta: None, - }), - request.metadata.as_ref(), - ); - } - break; + let invocation = crate::hooks::HookInvocation::pre_tool_use( + session.id.clone(), + tool_call.name.to_string(), + serde_json::to_value(&tool_call.arguments) + .unwrap_or(serde_json::Value::Null), + session.working_dir.to_string_lossy().to_string(), + ); + let outcome = hooks + .run( + invocation, + &self.extension_manager, + &session.working_dir, + cancellation_token.clone().unwrap_or_default(), + ) + .await + .unwrap_or_default(); + if outcome.blocked { + // Hook blocked — treat same as user declining + if let Some(response_msg) = request_to_response_map.get(&request.id) { + let mut response = response_msg.lock().await; + *response = response.clone().with_tool_response_with_metadata( + request.id.clone(), + Err(rmcp::model::ErrorData::new( + rmcp::model::ErrorCode::INTERNAL_ERROR, + "Tool execution blocked by hook".to_string(), + None, + )), + request.metadata.as_ref(), + ); } + break; } let (req_id, tool_result) = self.dispatch_tool_call(tool_call.clone(), request.id.clone(), cancellation_token.clone(), session).await; diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index 35cf40d8152a..ae97e98852e6 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -15,9 +15,12 @@ pub struct Hooks { } impl Hooks { - pub fn load(working_dir: &Path) -> Result { - let settings = HookSettingsFile::load_merged(working_dir)?; - Ok(Self { settings }) + pub fn load(working_dir: &Path) -> Self { + let settings = HookSettingsFile::load_merged(working_dir).unwrap_or_else(|e| { + tracing::debug!("No hooks config loaded: {}", e); + HookSettingsFile::default() + }); + Self { settings } } pub async fn run( @@ -25,6 +28,7 @@ impl Hooks { invocation: HookInvocation, extension_manager: &crate::agents::extension_manager::ExtensionManager, working_dir: &Path, + cancel_token: CancellationToken, ) -> Result { let event_configs = self.settings.get_hooks_for_event(invocation.event); @@ -37,8 +41,14 @@ impl Hooks { } for action in &config.hooks { - match Self::execute_action(action, &invocation, extension_manager, working_dir) - .await + match Self::execute_action( + action, + &invocation, + extension_manager, + working_dir, + cancel_token.clone(), + ) + .await { Ok(Some(result)) => { if let Some(HookDecision::Block) = result.decision { @@ -74,17 +84,6 @@ impl Hooks { Ok(outcome) } - pub async fn fire( - &self, - invocation: HookInvocation, - extension_manager: &crate::agents::extension_manager::ExtensionManager, - working_dir: &Path, - ) { - if let Err(e) = self.run(invocation, extension_manager, working_dir).await { - tracing::warn!("Hook fire failed: {}", e); - } - } - // Dispatches hook actions directly via ExtensionManager, bypassing tool inspection // and approval prompts. This is intentional: hooks are a privileged execution path // configured by the user (global) or opted-in (project). Running hooks through the @@ -94,9 +93,9 @@ impl Hooks { invocation: &HookInvocation, extension_manager: &crate::agents::extension_manager::ExtensionManager, working_dir: &Path, + cancel_token: CancellationToken, ) -> Result> { let (tool_call, timeout_secs) = Self::build_tool_call(action, invocation)?; - let cancel_token = CancellationToken::new(); let tool_call_result = extension_manager .dispatch_tool_call( @@ -107,18 +106,22 @@ impl Hooks { ) .await?; - let result = - tokio::time::timeout(Duration::from_secs(timeout_secs), tool_call_result.result).await; - - match result { - Ok(Ok(call_result)) => Self::parse_result(call_result, action, invocation.event), - Ok(Err(e)) => { - tracing::warn!("Hook tool call failed: {}, failing open", e); - Ok(None) + tokio::select! { + result = tokio::time::timeout(Duration::from_secs(timeout_secs), tool_call_result.result) => { + match result { + Ok(Ok(call_result)) => Self::parse_result(call_result, action, invocation.event), + Ok(Err(e)) => { + tracing::warn!("Hook tool call failed: {}, failing open", e); + Ok(None) + } + Err(_) => { + tracing::warn!("Hook timed out after {}s, failing open", timeout_secs); + Ok(None) + } + } } - Err(_) => { - cancel_token.cancel(); - tracing::warn!("Hook timed out after {}s, failing open", timeout_secs); + _ = cancel_token.cancelled() => { + tracing::info!("Hook cancelled by session cancellation"); Ok(None) } } @@ -269,9 +272,10 @@ impl Hooks { .notification_type .as_ref() .is_some_and(|t| t.contains(pattern)), - PreCompact | PostCompact => invocation.manual_compact.is_some_and(|manual| { - (manual && pattern == "manual") || (!manual && pattern == "auto") - }), + PreCompact | PostCompact => { + (invocation.manual_compact && pattern == "manual") + || (!invocation.manual_compact && pattern == "auto") + } _ => true, } } @@ -279,7 +283,7 @@ impl Hooks { /// Match a tool invocation against a Claude Code-style matcher pattern. /// Supports: /// "Bash" or "Bash(...)" — maps to developer__shell, optionally matching command content - /// "tool_name" — direct tool name substring match (goose-native) + /// "tool_name" — direct tool name match (goose-native) fn matches_tool(pattern: &str, invocation: &HookInvocation) -> bool { let tool_name = match &invocation.tool_name { Some(name) => name, @@ -288,14 +292,14 @@ impl Hooks { // Claude Code "Bash" / "Bash(pattern)" syntax if pattern == "Bash" { - return tool_name.contains("shell"); + return tool_name == "developer__shell"; } if let Some(inner) = pattern .strip_prefix("Bash(") .and_then(|s| s.strip_suffix(')')) { - if !tool_name.contains("shell") { + if tool_name != "developer__shell" { return false; } // Match the inner pattern against the command argument @@ -310,17 +314,12 @@ impl Hooks { } // Direct tool name match (goose-native: "developer__shell", "slack__post_message", etc.) - tool_name.contains(pattern) + tool_name == pattern } - /// Simple glob matching: only supports trailing * (prefix match). - /// "git push*" matches "git push origin main" - /// "git push" matches exactly "git push" fn glob_match(pattern: &str, text: &str) -> bool { - if let Some(prefix) = pattern.strip_suffix('*') { - text.starts_with(prefix) - } else { - text == pattern - } + glob::Pattern::new(pattern) + .map(|p| p.matches(text)) + .unwrap_or(false) } } diff --git a/crates/goose/src/hooks/types.rs b/crates/goose/src/hooks/types.rs index 878c73460897..fe2a8eaaa0a0 100644 --- a/crates/goose/src/hooks/types.rs +++ b/crates/goose/src/hooks/types.rs @@ -85,8 +85,8 @@ pub struct HookInvocation { #[serde(skip_serializing_if = "Option::is_none")] pub message_count_after: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub manual_compact: Option, + #[serde(default)] + pub manual_compact: bool, } impl HookInvocation { @@ -196,7 +196,7 @@ impl HookInvocation { ) -> Self { Self { message_count_before: Some(message_count_before), - manual_compact: Some(manual), + manual_compact: manual, cwd: Some(cwd), ..Self::base(HookEventKind::PreCompact, session_id) } @@ -212,7 +212,7 @@ impl HookInvocation { Self { message_count_before: Some(message_count_before), message_count_after: Some(message_count_after), - manual_compact: Some(manual), + manual_compact: manual, cwd: Some(cwd), ..Self::base(HookEventKind::PostCompact, session_id) } From db8d069ba7c3829f49f20f998eed7e52a7e163f1 Mon Sep 17 00:00:00 2001 From: Tyler Longwell Date: Sat, 21 Feb 2026 21:30:07 -0500 Subject: [PATCH 03/10] fix: skip unsupported hook action types instead of failing config parse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude Code supports "prompt" and "agent" hook action types that goose doesn't implement yet. Previously, a .claude/settings.json containing these types would fail serde deserialization entirely, silently dropping all hooks in the file — including valid "command" hooks. Adds a custom deserializer for HookEventConfig.hooks that parses each action individually: - Known types (command, mcp_tool): deserialize normally, warn on malformed config with the actual error - Unknown types (prompt, agent, etc.): warn and skip - Missing type field: warn and skip This matches the existing pattern in HookSettingsFile where unknown event names are warned and skipped rather than causing parse failures. --- crates/goose/src/hooks/config.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/crates/goose/src/hooks/config.rs b/crates/goose/src/hooks/config.rs index 30a1f5dffea6..08a27728fad2 100644 --- a/crates/goose/src/hooks/config.rs +++ b/crates/goose/src/hooks/config.rs @@ -49,9 +49,35 @@ pub struct HookEventConfig { #[serde(default)] pub matcher: Option, + #[serde(deserialize_with = "deserialize_hooks_skip_unknown")] pub hooks: Vec, } +fn deserialize_hooks_skip_unknown<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + let raw: Vec = Vec::deserialize(deserializer)?; + let mut actions = Vec::new(); + for value in raw { + match value.get("type").and_then(|t| t.as_str()) { + Some("command") | Some("mcp_tool") => match serde_json::from_value(value) { + Ok(action) => actions.push(action), + Err(e) => { + tracing::warn!("Invalid hook action config: {}", e); + } + }, + Some(other) => { + tracing::warn!("Unsupported hook action type '{}', skipping", other); + } + None => { + tracing::warn!("Hook action missing 'type' field, skipping"); + } + } + } + Ok(actions) +} + #[derive(Debug, Clone, Deserialize)] #[serde(tag = "type", rename_all = "lowercase")] pub enum HookAction { From 9ead0de4efb20c6a96ba20645d818e0ee0573518 Mon Sep 17 00:00:00 2001 From: Tyler Longwell Date: Sun, 22 Feb 2026 11:11:27 -0500 Subject: [PATCH 04/10] feat: inject hook context for SessionStart, UserPromptSubmit, PostToolUse, PostToolUseFailure - Add inject_hook_context helper, refactor run_post_compact_hook to use it - Wire context injection for 4 additional events (was PostCompact-only) - mcp_tool parse_result: non-HookResult output becomes additional_context - 32KB truncation on mcp_tool output matching command path - Context injected as user message (hidden from user, visible to agent) --- crates/goose/src/agents/agent.rs | 60 +++++++++++++++++++++++++------- crates/goose/src/hooks/mod.rs | 23 ++++++++---- 2 files changed, 65 insertions(+), 18 deletions(-) diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 13f0ca15cd78..435de67fccd8 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -457,6 +457,20 @@ impl Agent { Ok(tool_futures) } + async fn inject_hook_context( + session_id: &str, + context: String, + session_manager: &SessionManager, + conversation: &mut crate::conversation::Conversation, + ) -> Result<()> { + let msg = Message::user() + .with_text(context) + .with_visibility(false, true); + session_manager.add_message(session_id, &msg).await?; + conversation.push(msg); + Ok(()) + } + #[allow(clippy::too_many_arguments)] pub(crate) async fn run_post_compact_hook( &self, @@ -487,13 +501,8 @@ impl Agent { .await { if let Some(context) = outcome.context { - let context_msg = Message::assistant() - .with_text(context) - .with_visibility(false, true); - session_manager - .add_message(session_id, &context_msg) + Self::inject_hook_context(session_id, context, session_manager, conversation) .await?; - conversation.push(context_msg); } } Ok(()) @@ -1217,14 +1226,26 @@ impl Agent { session_id.clone(), working_dir.to_string_lossy().to_string(), ); - let _ = hooks + if let Ok(outcome) = hooks .run( invocation, &self.extension_manager, &working_dir, cancel_token.clone().unwrap_or_default(), ) - .await; + .await + { + if let Some(ctx) = outcome.context { + Self::inject_hook_context( + &session_id, + ctx, + &session_manager, + &mut conversation, + ) + .await + .ok(); + } + } } // Fire UserPromptSubmit hook with the last user message @@ -1254,6 +1275,11 @@ impl Agent { yield AgentEvent::Message(Message::assistant().with_text("Prompt blocked by hook.")); })); } + if let Some(ctx) = outcome.context { + Self::inject_hook_context(&session_id, ctx, &session_manager, &mut conversation) + .await + .ok(); + } } Ok(Box::pin(async_stream::try_stream! { @@ -1520,14 +1546,19 @@ impl Agent { tool_output, working_dir.to_string_lossy().to_string(), ); - let _ = hooks + if let Ok(outcome) = hooks .run( invocation, &self.extension_manager, &working_dir, cancel_token.clone().unwrap_or_default(), ) - .await; + .await + { + if let Some(ctx) = outcome.context { + Self::inject_hook_context(&session_config.id, ctx, &session_manager, &mut conversation).await.ok(); + } + } } Err(error_data) => { let invocation = crate::hooks::HookInvocation::post_tool_use_failure( @@ -1537,14 +1568,19 @@ impl Agent { error_data.message.to_string(), working_dir.to_string_lossy().to_string(), ); - let _ = hooks + if let Ok(outcome) = hooks .run( invocation, &self.extension_manager, &working_dir, cancel_token.clone().unwrap_or_default(), ) - .await; + .await + { + if let Some(ctx) = outcome.context { + Self::inject_hook_context(&session_config.id, ctx, &session_manager, &mut conversation).await.ok(); + } + } } } } diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index ae97e98852e6..db8deeb28964 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -247,12 +247,23 @@ impl Hooks { if text.trim().is_empty() { Ok(Some(HookResult::default())) } else { - Ok(serde_json::from_str(text.trim()) - .map(Some) - .unwrap_or_else(|e| { - tracing::debug!("Hook output is not HookResult JSON: {}", e); - None - })) + Ok(Some(serde_json::from_str(text.trim()).unwrap_or_else( + |e| { + tracing::debug!("MCP hook output is not HookResult JSON: {}", e); + let mut context = text.trim().to_string(); + if context.len() > 32_768 { + tracing::warn!( + "MCP hook output truncated from {} to 32KB", + context.len() + ); + context.truncate(context.floor_char_boundary(32_768)); + } + HookResult { + additional_context: Some(context), + ..Default::default() + } + }, + ))) } } } From 40640e260cf8344a68df0ce6c7e58086220a87ba Mon Sep 17 00:00:00 2001 From: Tyler Longwell Date: Fri, 27 Feb 2026 09:41:49 -0500 Subject: [PATCH 05/10] feat: direct subprocess execution for hook commands Replace MCP dispatch + GOOSE_HOOK_EXIT workaround with direct subprocess spawning for hook commands. Leverages PR #7466's new platform developer shell (build_shell_command, user_login_path) for native exit codes, stdin piping, and separate stdout/stderr. New hooks/shell.rs: - Deadlock-safe I/O: stdout + stderr drain concurrently before stdin write - Process group isolation (unix) so Ctrl+C doesn't kill hooks - Timeout + cancellation support with secondary drain timeouts - 30s stdin write timeout prevents hanging on uncooperative children Rewritten hooks/mod.rs: - Split execute_action into execute_command (direct subprocess) and execute_mcp_tool (MCP dispatch, unchanged behavior) - New parse_command_output with native exit code handling: exit 0 = JSON or plain text context, exit 2 = block with stderr reason, other = fail-open - Removed GOOSE_HOOK_EXIT marker parsing entirely - Zero-timeout guard (0 -> 600s) on both Command and McpTool paths HooksOutcome.reason: - Added reason field to propagate hook block reasons - All 3 call sites (agent.rs x2, tool_execution.rs x1) now surface the hook's stderr message instead of hardcoded strings Tests (11 total): - 9 pure-logic tests for parse_command_output covering every exit code path, JSON/non-JSON parsing, truncation, blockable/non-blockable - 2 real-subprocess tests for run_hook_command: stdin piping via jq, exit-2 stderr capture (Claude Code block protocol) Crossfire reviewed: R1 6/10 -> R2 8/10 -> R3 9/10 APPROVE (Opus+Codex) --- crates/goose/src/agents/agent.rs | 13 +- .../platform_extensions/developer/shell.rs | 4 +- crates/goose/src/agents/tool_execution.rs | 2 +- crates/goose/src/hooks/mod.rs | 412 ++++++++++++------ crates/goose/src/hooks/shell.rs | 206 +++++++++ crates/goose/src/hooks/types.rs | 1 + 6 files changed, 500 insertions(+), 138 deletions(-) create mode 100644 crates/goose/src/hooks/shell.rs diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 954f4f684727..59a388a8f8af 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -414,7 +414,11 @@ impl Agent { // Create an error response instead of dispatching let error_result = Err(ErrorData::new( ErrorCode::INTERNAL_ERROR, - "Tool execution blocked by hook".to_string(), + outcome + .reason + .as_deref() + .unwrap_or("Tool execution blocked by hook") + .to_string(), None, )); tool_futures.push(( @@ -1288,8 +1292,13 @@ impl Agent { .await .unwrap_or_default(); if outcome.blocked { + let block_msg = outcome + .reason + .as_deref() + .unwrap_or("Prompt blocked by hook.") + .to_string(); return Ok(Box::pin(async_stream::try_stream! { - yield AgentEvent::Message(Message::assistant().with_text("Prompt blocked by hook.")); + yield AgentEvent::Message(Message::assistant().with_text(block_msg)); })); } if let Some(ctx) = outcome.context { diff --git a/crates/goose/src/agents/platform_extensions/developer/shell.rs b/crates/goose/src/agents/platform_extensions/developer/shell.rs index f7dcfc7a595a..70b3262a8976 100644 --- a/crates/goose/src/agents/platform_extensions/developer/shell.rs +++ b/crates/goose/src/agents/platform_extensions/developer/shell.rs @@ -59,7 +59,7 @@ fn resolve_login_shell_path() -> Option { /// Returns the user's full login shell PATH, resolved once and cached. #[cfg(not(windows))] -fn user_login_path() -> Option<&'static str> { +pub(crate) fn user_login_path() -> Option<&'static str> { static CACHED: OnceLock> = OnceLock::new(); CACHED.get_or_init(resolve_login_shell_path).as_deref() } @@ -204,7 +204,7 @@ async fn run_command( }) } -fn build_shell_command(command_line: &str) -> tokio::process::Command { +pub(crate) fn build_shell_command(command_line: &str) -> tokio::process::Command { #[cfg(windows)] let mut command = { let mut command = tokio::process::Command::new("cmd"); diff --git a/crates/goose/src/agents/tool_execution.rs b/crates/goose/src/agents/tool_execution.rs index bafd15e439d0..a00c0d1aa28c 100644 --- a/crates/goose/src/agents/tool_execution.rs +++ b/crates/goose/src/agents/tool_execution.rs @@ -125,7 +125,7 @@ impl Agent { request.id.clone(), Err(rmcp::model::ErrorData::new( rmcp::model::ErrorCode::INTERNAL_ERROR, - "Tool execution blocked by hook".to_string(), + outcome.reason.as_deref().unwrap_or("Tool execution blocked by hook").to_string(), None, )), request.metadata.as_ref(), diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index db8deeb28964..35003d1216bf 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -1,4 +1,5 @@ mod config; +mod shell; pub mod types; pub use config::{HookAction, HookEventConfig, HookSettingsFile}; @@ -54,6 +55,7 @@ impl Hooks { if let Some(HookDecision::Block) = result.decision { if invocation.event.can_block() { outcome.blocked = true; + outcome.reason = result.reason.clone(); tracing::info!("Hook blocked event {:?}", invocation.event); return Ok(outcome); } @@ -84,10 +86,10 @@ impl Hooks { Ok(outcome) } - // Dispatches hook actions directly via ExtensionManager, bypassing tool inspection - // and approval prompts. This is intentional: hooks are a privileged execution path - // configured by the user (global) or opted-in (project). Running hooks through the - // normal tool pipeline would cause infinite recursion (PreToolUse → hook → tool → PreToolUse). + // Dispatches hook actions directly via ExtensionManager (McpTool) or subprocess (Command), + // bypassing tool inspection and approval prompts. This is intentional: hooks are a privileged + // execution path configured by the user (global) or opted-in (project). Running hooks through + // the normal tool pipeline would cause infinite recursion (PreToolUse → hook → tool → PreToolUse). async fn execute_action( action: &HookAction, invocation: &HookInvocation, @@ -95,7 +97,145 @@ impl Hooks { working_dir: &Path, cancel_token: CancellationToken, ) -> Result> { - let (tool_call, timeout_secs) = Self::build_tool_call(action, invocation)?; + match action { + HookAction::Command { command, timeout } => { + Self::execute_command(command, *timeout, invocation, working_dir, cancel_token) + .await + } + HookAction::McpTool { + tool, + arguments, + timeout, + } => { + Self::execute_mcp_tool( + tool, + arguments, + *timeout, + invocation, + extension_manager, + working_dir, + cancel_token, + ) + .await + } + } + } + + /// Execute a hook command as a direct subprocess. + async fn execute_command( + command: &str, + timeout: u64, + invocation: &HookInvocation, + working_dir: &Path, + cancel_token: CancellationToken, + ) -> Result> { + let effective_timeout = if timeout == 0 { 600 } else { timeout }; + let stdin_json = serde_json::to_string(invocation)?; + let output = shell::run_hook_command( + command, + Some(&stdin_json), + effective_timeout, + working_dir, + cancel_token, + ) + .await + .map_err(|e| anyhow::anyhow!("{}", e))?; + + if output.timed_out { + tracing::warn!("Hook timed out after {}s, failing open", effective_timeout); + return Ok(None); + } + + Self::parse_command_output(output, invocation.event) + } + + /// Parse the output of a hook command into a HookResult. + /// + /// Exit code semantics: + /// 0 → success. Stdout is parsed as JSON HookResult, or treated as additionalContext. + /// 2 → block (on blockable events). Stderr is the block reason. + /// other → fail open (warning logged, hook result ignored). + /// None (signal) → fail open. + fn parse_command_output( + output: shell::HookCommandOutput, + event: HookEventKind, + ) -> Result> { + match output.exit_code { + Some(0) => { + let stdout = output.stdout.trim(); + if stdout.is_empty() { + Ok(Some(HookResult::default())) + } else { + // Try JSON first, fall back to plain text as additionalContext + Ok(Some( + serde_json::from_str::(stdout).unwrap_or_else(|_| { + let mut context = stdout.to_string(); + if context.len() > 32_768 { + tracing::warn!( + "Hook stdout truncated from {} to 32KB", + context.len() + ); + context.truncate(context.floor_char_boundary(32_768)); + } + HookResult { + additional_context: Some(context), + ..Default::default() + } + }), + )) + } + } + Some(2) if event.can_block() => { + // Exit 2 → block. Stderr is the error message (Claude Code compat) + let reason = { + let s = output.stderr.trim(); + if s.is_empty() { + None + } else { + // Cap stderr at 4KB for block reason + let mut r = s.to_string(); + if r.len() > 4096 { + r.truncate(r.floor_char_boundary(4096)); + } + Some(r) + } + }; + Ok(Some(HookResult { + decision: Some(HookDecision::Block), + reason, + ..Default::default() + })) + } + Some(code) => { + tracing::warn!("Hook exited with code {}, failing open", code); + Ok(None) + } + None => { + tracing::warn!("Hook terminated without exit code, failing open"); + Ok(None) + } + } + } + + /// Execute a hook via MCP tool dispatch through ExtensionManager. + async fn execute_mcp_tool( + tool: &str, + arguments: &serde_json::Map, + timeout: u64, + invocation: &HookInvocation, + extension_manager: &crate::agents::extension_manager::ExtensionManager, + working_dir: &Path, + cancel_token: CancellationToken, + ) -> Result> { + // Guard zero timeout — default to 10 minutes + let effective_timeout = if timeout == 0 { 600 } else { timeout }; + + let tool_call = CallToolRequestParams { + meta: None, + task: None, + name: tool.to_string().into(), + arguments: Some(arguments.clone()), + }; let tool_call_result = extension_manager .dispatch_tool_call( @@ -107,15 +247,15 @@ impl Hooks { .await?; tokio::select! { - result = tokio::time::timeout(Duration::from_secs(timeout_secs), tool_call_result.result) => { + result = tokio::time::timeout(Duration::from_secs(effective_timeout), tool_call_result.result) => { match result { - Ok(Ok(call_result)) => Self::parse_result(call_result, action, invocation.event), + Ok(Ok(call_result)) => Self::parse_mcp_result(call_result, invocation.event), Ok(Err(e)) => { - tracing::warn!("Hook tool call failed: {}, failing open", e); + tracing::warn!("Hook MCP tool call failed: {}, failing open", e); Ok(None) } Err(_) => { - tracing::warn!("Hook timed out after {}s, failing open", timeout_secs); + tracing::warn!("Hook MCP tool timed out after {}s, failing open", effective_timeout); Ok(None) } } @@ -127,53 +267,16 @@ impl Hooks { } } - fn build_tool_call( - action: &HookAction, - invocation: &HookInvocation, - ) -> Result<(CallToolRequestParams, u64)> { - match action { - HookAction::Command { command, timeout } => { - let json = serde_json::to_string(invocation)?; - let escaped = json.replace('\'', "'\\''"); - let shell_cmd = format!( - "printf '%s' '{}' | {}; printf '\\nGOOSE_HOOK_EXIT:%d' $?", - escaped, command - ); - - let args = serde_json::json!({"command": shell_cmd}); - Ok(( - CallToolRequestParams { - meta: None, - task: None, - name: "developer__shell".into(), - arguments: args.as_object().cloned(), - }, - *timeout, - )) - } - HookAction::McpTool { - tool, - arguments, - timeout, - } => Ok(( - CallToolRequestParams { - meta: None, - task: None, - name: tool.clone().into(), - arguments: Some(arguments.clone()), - }, - *timeout, - )), - } - } - - fn parse_result( + /// Parse the result of an MCP tool call into a HookResult. + fn parse_mcp_result( result: CallToolResult, - action: &HookAction, event: HookEventKind, ) -> Result> { + // Suppress unused variable warning — event is kept for future use and API symmetry + let _ = event; + if result.is_error.unwrap_or(false) { - tracing::warn!("Hook tool returned error, failing open"); + tracing::warn!("Hook MCP tool returned error, failing open"); return Ok(None); } @@ -184,88 +287,23 @@ impl Hooks { .collect::>() .join(""); - match action { - HookAction::Command { .. } => { - if text.is_empty() { - return Ok(Some(HookResult::default())); - } - - if let Some(exit_marker) = text.rfind("GOOSE_HOOK_EXIT:") { - let (output, exit_part) = text.split_at(exit_marker); - if let Some(code_str) = exit_part.strip_prefix("GOOSE_HOOK_EXIT:") { - if let Ok(code) = code_str - .split_whitespace() - .next() - .unwrap_or("") - .parse::() - { - return match code { - 0 => { - if output.trim().is_empty() { - Ok(Some(HookResult::default())) - } else { - Ok(serde_json::from_str(output.trim()) - .map(Some) - .unwrap_or_else(|_| { - // Non-JSON stdout from exit-0 command → surface as context (Claude Code compat) - let mut context = output.trim().to_string(); - if context.len() > 32_768 { - tracing::warn!( - "Hook stdout truncated from {} to 32KB", - context.len() - ); - context.truncate( - context.floor_char_boundary(32_768), - ); - } - Some(HookResult { - additional_context: Some(context), - ..Default::default() - }) - })) - } - } - 2 if event.can_block() => Ok(Some(HookResult { - decision: Some(HookDecision::Block), - ..Default::default() - })), - _ => Ok(None), - }; - } + if text.trim().is_empty() { + Ok(Some(HookResult::default())) + } else { + Ok(Some(serde_json::from_str(text.trim()).unwrap_or_else( + |e| { + tracing::debug!("MCP hook output is not HookResult JSON: {}", e); + let mut context = text.trim().to_string(); + if context.len() > 32_768 { + tracing::warn!("MCP hook output truncated from {} to 32KB", context.len()); + context.truncate(context.floor_char_boundary(32_768)); } - } - - // No exit marker — can't confirm exit 0, so don't surface raw output - Ok(serde_json::from_str(text.trim()) - .map(Some) - .unwrap_or_else(|e| { - tracing::debug!("Hook output is not JSON (no exit marker): {}", e); - None - })) - } - HookAction::McpTool { .. } => { - if text.trim().is_empty() { - Ok(Some(HookResult::default())) - } else { - Ok(Some(serde_json::from_str(text.trim()).unwrap_or_else( - |e| { - tracing::debug!("MCP hook output is not HookResult JSON: {}", e); - let mut context = text.trim().to_string(); - if context.len() > 32_768 { - tracing::warn!( - "MCP hook output truncated from {} to 32KB", - context.len() - ); - context.truncate(context.floor_char_boundary(32_768)); - } - HookResult { - additional_context: Some(context), - ..Default::default() - } - }, - ))) - } - } + HookResult { + additional_context: Some(context), + ..Default::default() + } + }, + ))) } } @@ -334,3 +372,111 @@ impl Hooks { .unwrap_or(false) } } + +#[cfg(test)] +mod tests { + use super::*; + use types::{HookDecision, HookEventKind}; + + fn make_output(stdout: &str, stderr: &str, exit_code: Option) -> shell::HookCommandOutput { + shell::HookCommandOutput { + stdout: stdout.to_string(), + stderr: stderr.to_string(), + exit_code, + timed_out: false, + } + } + + // -- parse_command_output: the core exit-code contract -- + + #[test] + fn exit_0_empty_stdout_approves() { + let output = make_output("", "", Some(0)); + let result = Hooks::parse_command_output(output, HookEventKind::PreToolUse) + .unwrap() + .unwrap(); + assert!(result.decision.is_none()); + assert!(result.additional_context.is_none()); + } + + #[test] + fn exit_0_json_parsed_as_hook_result() { + let json = r#"{"decision":"block","reason":"tests must pass"}"#; + let output = make_output(json, "", Some(0)); + let result = Hooks::parse_command_output(output, HookEventKind::PreToolUse) + .unwrap() + .unwrap(); + assert_eq!(result.decision, Some(HookDecision::Block)); + assert_eq!(result.reason.as_deref(), Some("tests must pass")); + } + + #[test] + fn exit_0_non_json_becomes_additional_context() { + let output = make_output("plain text from hook", "", Some(0)); + let result = Hooks::parse_command_output(output, HookEventKind::SessionStart) + .unwrap() + .unwrap(); + assert_eq!( + result.additional_context.as_deref(), + Some("plain text from hook") + ); + assert!(result.decision.is_none()); + } + + #[test] + fn exit_0_large_stdout_truncated_to_32kb() { + let big = "x".repeat(40_000); + let output = make_output(&big, "", Some(0)); + let result = Hooks::parse_command_output(output, HookEventKind::PostToolUse) + .unwrap() + .unwrap(); + let ctx = result.additional_context.unwrap(); + assert!(ctx.len() <= 32_768); + } + + #[test] + fn exit_2_blocks_on_blockable_event() { + let output = make_output("ignored stdout", "rm is dangerous", Some(2)); + let result = Hooks::parse_command_output(output, HookEventKind::PreToolUse) + .unwrap() + .unwrap(); + assert_eq!(result.decision, Some(HookDecision::Block)); + assert_eq!(result.reason.as_deref(), Some("rm is dangerous")); + } + + #[test] + fn exit_2_stderr_capped_at_4kb() { + let big_err = "e".repeat(8_000); + let output = make_output("", &big_err, Some(2)); + let result = Hooks::parse_command_output(output, HookEventKind::UserPromptSubmit) + .unwrap() + .unwrap(); + assert_eq!(result.decision, Some(HookDecision::Block)); + assert!(result.reason.as_ref().unwrap().len() <= 4096); + } + + #[test] + fn exit_2_on_non_blockable_event_fails_open() { + // SessionStart.can_block() == false + let output = make_output("", "error", Some(2)); + let result = Hooks::parse_command_output(output, HookEventKind::SessionStart).unwrap(); + assert!( + result.is_none(), + "exit 2 on non-blockable event should fail open" + ); + } + + #[test] + fn nonzero_non2_exit_fails_open() { + let output = make_output("some output", "some error", Some(1)); + let result = Hooks::parse_command_output(output, HookEventKind::PreToolUse).unwrap(); + assert!(result.is_none()); + } + + #[test] + fn no_exit_code_fails_open() { + let output = make_output("", "", None); + let result = Hooks::parse_command_output(output, HookEventKind::PreToolUse).unwrap(); + assert!(result.is_none()); + } +} diff --git a/crates/goose/src/hooks/shell.rs b/crates/goose/src/hooks/shell.rs new file mode 100644 index 000000000000..31ce17060697 --- /dev/null +++ b/crates/goose/src/hooks/shell.rs @@ -0,0 +1,206 @@ +use std::path::Path; +use std::process::Stdio; +use std::time::Duration; + +use tokio::io::AsyncReadExt; +use tokio::io::AsyncWriteExt; +use tokio_util::sync::CancellationToken; + +use crate::agents::platform_extensions::developer::shell::build_shell_command; +#[cfg(not(windows))] +use crate::agents::platform_extensions::developer::shell::user_login_path; + +/// Output from a hook command execution. +pub struct HookCommandOutput { + pub stdout: String, + pub stderr: String, + pub exit_code: Option, + pub timed_out: bool, +} + +/// Run a hook command as a direct subprocess. +/// +/// Deadlock-safe: stdout and stderr are drained concurrently via spawned tasks, +/// and both drains start BEFORE stdin is written. This prevents circular deadlock +/// when the child echoes input back to stdout/stderr before consuming all stdin. +/// +/// The child is placed in its own process group (unix) so terminal SIGINT does not +/// kill it — the cancellation token is the intended shutdown path. +pub async fn run_hook_command( + command_line: &str, + stdin_data: Option<&str>, + timeout_secs: u64, + working_dir: &Path, + cancel_token: CancellationToken, +) -> Result { + // Guard zero timeout — default to 10 minutes + let timeout = if timeout_secs == 0 { 600 } else { timeout_secs }; + + let mut command = build_shell_command(command_line); + command.current_dir(working_dir); + + // Inherit the user's full login shell PATH (not the minimal desktop-app PATH) + #[cfg(not(windows))] + if let Some(path) = user_login_path() { + command.env("PATH", path); + } + + // Isolate hook into its own process group so Ctrl+C (SIGINT) doesn't kill it. + // The cancellation token is the intended shutdown path for hooks. + #[cfg(unix)] + command.process_group(0); + + command.stdout(Stdio::piped()); + command.stderr(Stdio::piped()); + + if stdin_data.is_some() { + command.stdin(Stdio::piped()); + } else { + command.stdin(Stdio::null()); + } + + let mut child = command + .spawn() + .map_err(|e| format!("Failed to spawn hook command: {}", e))?; + + // Take ALL handles before spawning any tasks + let stdin_handle = child.stdin.take(); + let stdout_handle = child + .stdout + .take() + .ok_or_else(|| "Failed to capture stdout".to_string())?; + let stderr_handle = child + .stderr + .take() + .ok_or_else(|| "Failed to capture stderr".to_string())?; + + // Spawn stdout drain FIRST (before stdin write to prevent circular deadlock) + let stdout_task = tokio::spawn(async move { + let mut output = String::new(); + let mut reader = stdout_handle; + let _ = reader.read_to_string(&mut output).await; + output + }); + + // Spawn stderr drain concurrently + let stderr_task = tokio::spawn(async move { + let mut output = String::new(); + let mut reader = stderr_handle; + let _ = reader.read_to_string(&mut output).await; + output + }); + + // Write stdin data concurrently with drains. + // Wrapped in a timeout to prevent hanging if the child stops reading stdin. + let stdin_data_owned = stdin_data.map(|s| s.to_string()); + let stdin_task = tokio::spawn(async move { + if let Some(data) = stdin_data_owned { + if let Some(mut stdin) = stdin_handle { + let _ = + tokio::time::timeout(Duration::from_secs(30), stdin.write_all(data.as_bytes())) + .await; + drop(stdin); // Close stdin so child sees EOF + } + } + }); + + // Wait for child with timeout + cancellation + let (exit_code, timed_out) = tokio::select! { + result = tokio::time::timeout(Duration::from_secs(timeout), child.wait()) => { + match result { + Ok(Ok(status)) => (status.code(), false), + Ok(Err(e)) => { + return Err(format!("Failed waiting on hook command: {}", e)); + } + Err(_) => { + // Timeout — kill the process + let _ = child.start_kill(); + let _ = child.wait().await; + (None, true) + } + } + } + _ = cancel_token.cancelled() => { + // Cancellation — kill the process + let _ = child.start_kill(); + let _ = child.wait().await; + (None, true) + } + }; + + // Collect output from drain tasks. + // Use a secondary timeout to prevent hanging if grandchild processes hold pipe FDs open. + let drain_timeout = Duration::from_secs(5); + let stdout_output = tokio::time::timeout(drain_timeout, stdout_task) + .await + .ok() + .and_then(|r| r.ok()) + .unwrap_or_default(); + let stderr_output = tokio::time::timeout(drain_timeout, stderr_task) + .await + .ok() + .and_then(|r| r.ok()) + .unwrap_or_default(); + + // Best-effort wait for stdin task to finish + let _ = tokio::time::timeout(Duration::from_secs(1), stdin_task).await; + + Ok(HookCommandOutput { + stdout: stdout_output, + stderr: stderr_output, + exit_code, + timed_out, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use tokio_util::sync::CancellationToken; + + #[cfg(not(windows))] + #[tokio::test] + async fn hook_receives_stdin_and_returns_stdout() { + // Real subprocess: reads JSON from stdin, echoes a field back to stdout + let dir = tempfile::tempdir().unwrap(); + let output = run_hook_command( + r#"jq -r '.hook_event_name'"#, + Some(r#"{"hook_event_name":"PreToolUse","session_id":"s1"}"#), + 10, + dir.path(), + CancellationToken::new(), + ) + .await + .unwrap(); + + assert_eq!(output.exit_code, Some(0)); + assert_eq!(output.stdout.trim(), "PreToolUse"); + assert!(output.stderr.is_empty()); + assert!(!output.timed_out); + } + + #[cfg(not(windows))] + #[tokio::test] + async fn hook_exit_2_captures_stderr_separately() { + // Real subprocess: writes to stderr and exits 2 (Claude Code block protocol) + let dir = tempfile::tempdir().unwrap(); + let output = run_hook_command( + "echo 'blocked: rm not allowed' >&2; exit 2", + None, + 10, + dir.path(), + CancellationToken::new(), + ) + .await + .unwrap(); + + assert_eq!(output.exit_code, Some(2)); + assert!( + output.stderr.contains("rm not allowed"), + "stderr should contain the block reason, got: {:?}", + output.stderr + ); + // stdout should be empty — Claude Code ignores stdout on exit 2 + assert!(output.stdout.trim().is_empty()); + } +} diff --git a/crates/goose/src/hooks/types.rs b/crates/goose/src/hooks/types.rs index fe2a8eaaa0a0..0d2e80fe2ee3 100644 --- a/crates/goose/src/hooks/types.rs +++ b/crates/goose/src/hooks/types.rs @@ -298,4 +298,5 @@ pub enum HookDecision { pub struct HooksOutcome { pub blocked: bool, pub context: Option, + pub reason: Option, } From 2fef298c6bd6e85a98688cc5afd462feb3dea2f9 Mon Sep 17 00:00:00 2001 From: Tyler Longwell Date: Fri, 27 Feb 2026 10:19:23 -0500 Subject: [PATCH 06/10] feat: regex matchers, ContextFill hook, SessionEnd wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regex matchers (Claude Code compat): - Replace glob/exact-match with regex for tool name matching - Edit|Write, mcp__memory__.*, Notebook.* patterns now work - Bash/Bash(pattern) special case preserved, also accepts 'shell' - Notification matcher also upgraded to regex - Removed glob dependency from Cargo.toml ContextFill hook (goose extension): - New HookEventKind::ContextFill fires when context reaches a configured percentage threshold (e.g., 70%) - Matcher is the threshold: {"matcher": "70", ...} - Fires once per threshold crossing, not every turn while above - Hook receives current_tokens, context_limit, fill_percentage - Checked after each LLM response using provider-reported token count - Output injected as context via inject_hook_context SessionEnd hook: - Fires after Stop hook at end of reply_internal - Side-effect only (cleanup, logging) — cannot block Tests (15 total): - 9 exit-code contract tests (unchanged) - 2 real-subprocess tests (unchanged) - 4 new regex matcher tests: alternation, wildcard, exact, invalid --- Cargo.lock | 1 - crates/goose/Cargo.toml | 2 +- crates/goose/src/agents/agent.rs | 29 +++++ crates/goose/src/hooks/mod.rs | 186 ++++++++++++++++++++++++++++--- crates/goose/src/hooks/types.rs | 30 +++++ 5 files changed, 232 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 059ebf2c587d..27bbfa9ebcda 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4295,7 +4295,6 @@ dependencies = [ "etcetera 0.11.0", "fs2", "futures", - "glob", "goose-test-support", "hf-hub", "ignore", diff --git a/crates/goose/Cargo.toml b/crates/goose/Cargo.toml index f3c7e39a0d0f..c283ab9855f2 100644 --- a/crates/goose/Cargo.toml +++ b/crates/goose/Cargo.toml @@ -16,7 +16,7 @@ workspace = true [dependencies] lru = { workspace = true } -glob = "0.3" + rmcp = { workspace = true, features = [ "client", diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 59a388a8f8af..e8baee6038ec 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -1401,6 +1401,21 @@ impl Agent { if let Some(ref usage) = usage { self.update_session_metrics(&session_config.id, session_config.schedule_id.clone(), usage, false).await?; + + // Check ContextFill hooks after each LLM response + if let Some(total) = usage.usage.total_tokens { + let ctx_limit = self.provider().await?.get_model_config().context_limit(); + if let Some(ctx) = hooks.check_context_fill( + &session_config.id, + total as usize, + ctx_limit, + &self.extension_manager, + &working_dir, + cancel_token.clone().unwrap_or_default(), + ).await { + Self::inject_hook_context(&session_config.id, ctx, &session_manager, &mut conversation).await.ok(); + } + } } if let Some(response) = response { @@ -1948,6 +1963,20 @@ impl Agent { ) .await; + // Fire SessionEnd hook after stop + let invocation = crate::hooks::HookInvocation::session_end( + session_id.clone(), + None, + ); + let _ = hooks + .run( + invocation, + &self.extension_manager, + &working_dir, + cancel_token.clone().unwrap_or_default(), + ) + .await; + if !last_assistant_text.is_empty() { tracing::info!(target: "goose::agents::agent", trace_output = last_assistant_text.as_str()); } diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index 35003d1216bf..0c4ccddfeb85 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -13,6 +13,9 @@ use tokio_util::sync::CancellationToken; pub struct Hooks { settings: HookSettingsFile, + /// Tracks which ContextFill thresholds have already fired this session. + /// Prevents re-firing every turn while above the threshold. + fired_context_thresholds: std::sync::Mutex>, } impl Hooks { @@ -21,7 +24,100 @@ impl Hooks { tracing::debug!("No hooks config loaded: {}", e); HookSettingsFile::default() }); - Self { settings } + Self { + settings, + fired_context_thresholds: std::sync::Mutex::new(std::collections::HashSet::new()), + } + } + + /// Check context fill and fire ContextFill hooks for any thresholds that have been crossed. + /// Returns context to inject, if any. + /// + /// Call this once per turn in the agent loop with the current token count. + pub async fn check_context_fill( + &self, + session_id: &str, + current_tokens: usize, + context_limit: usize, + extension_manager: &crate::agents::extension_manager::ExtensionManager, + working_dir: &Path, + cancel_token: CancellationToken, + ) -> Option { + if context_limit == 0 { + return None; + } + + let fill_pct = ((current_tokens as f64 / context_limit as f64) * 100.0) as u32; + + // Get configured ContextFill thresholds from settings + let event_configs = self + .settings + .get_hooks_for_event(HookEventKind::ContextFill); + if event_configs.is_empty() { + return None; + } + + // Find thresholds that are newly crossed + let mut new_thresholds = Vec::new(); + { + let mut fired = self + .fired_context_thresholds + .lock() + .unwrap_or_else(|e| e.into_inner()); + for config in event_configs { + if let Some(pattern) = &config.matcher { + if let Ok(threshold) = pattern.parse::() { + if fill_pct >= threshold && !fired.contains(&threshold) { + fired.insert(threshold); + new_thresholds.push(threshold); + } + } + } + } + } + + if new_thresholds.is_empty() { + return None; + } + + // Fire hooks for each newly crossed threshold. + // fill_percentage is set to the threshold value (not the actual fill) so that + // matches_config can use exact equality to route to the correct config entry. + // The actual fill level is derivable from current_tokens / context_limit. + let mut all_context = Vec::new(); + for threshold in new_thresholds { + tracing::info!( + "Context fill {}% crossed threshold {}%, firing hooks", + fill_pct, + threshold + ); + let invocation = HookInvocation::context_fill( + session_id.to_string(), + current_tokens, + context_limit, + threshold, + working_dir.to_string_lossy().to_string(), + ); + if let Ok(outcome) = self + .run( + invocation, + extension_manager, + working_dir, + cancel_token.clone(), + ) + .await + { + if let Some(ctx) = outcome.context { + all_context.push(ctx); + } + } + } + + if all_context.is_empty() { + None + } else { + Some(all_context.join("\n")) + } } pub async fn run( @@ -320,19 +416,36 @@ impl Hooks { Notification => invocation .notification_type .as_ref() - .is_some_and(|t| t.contains(pattern)), + .is_some_and(|t| Self::regex_matches(pattern, t)), PreCompact | PostCompact => { (invocation.manual_compact && pattern == "manual") || (!invocation.manual_compact && pattern == "auto") } + ContextFill => { + // Matcher is a threshold percentage (e.g., "70"). + // Exact equality: check_context_fill sets fill_percentage to the + // specific threshold being fired (not the current fill level), + // preventing double-execution when multiple thresholds cross at once. + if let (Ok(threshold), Some(fill)) = + (pattern.parse::(), invocation.fill_percentage) + { + fill == threshold + } else { + false + } + } _ => true, } } /// Match a tool invocation against a Claude Code-style matcher pattern. - /// Supports: - /// "Bash" or "Bash(...)" — maps to developer__shell, optionally matching command content - /// "tool_name" — direct tool name match (goose-native) + /// + /// Supports regex patterns (Claude Code compat): + /// "Bash" — maps to developer__shell or shell + /// "Bash(regex)" — developer__shell/shell with command content regex + /// "Edit|Write" — regex alternation matching tool names + /// "mcp__memory__.*" — regex wildcard matching MCP tool names + /// "developer__shell" — exact match (also valid regex) fn matches_tool(pattern: &str, invocation: &HookInvocation) -> bool { let tool_name = match &invocation.tool_name { Some(name) => name, @@ -341,17 +454,16 @@ impl Hooks { // Claude Code "Bash" / "Bash(pattern)" syntax if pattern == "Bash" { - return tool_name == "developer__shell"; + return tool_name == "developer__shell" || tool_name == "shell"; } if let Some(inner) = pattern .strip_prefix("Bash(") .and_then(|s| s.strip_suffix(')')) { - if tool_name != "developer__shell" { + if tool_name != "developer__shell" && tool_name != "shell" { return false; } - // Match the inner pattern against the command argument let command_str = invocation .tool_input .as_ref() @@ -359,16 +471,23 @@ impl Hooks { .and_then(|v| v.as_str()) .unwrap_or(""); - return Self::glob_match(inner, command_str); + return Self::regex_matches(inner, command_str); } - // Direct tool name match (goose-native: "developer__shell", "slack__post_message", etc.) - tool_name == pattern + // Regex match against tool name (Claude Code compat: "Edit|Write", "mcp__.*", etc.) + Self::regex_matches(pattern, tool_name) } - fn glob_match(pattern: &str, text: &str) -> bool { - glob::Pattern::new(pattern) - .map(|p| p.matches(text)) + /// Test if `text` matches `pattern` as a full-string regex. + /// Anchors the pattern to match the entire string (not a substring). + fn regex_matches(pattern: &str, text: &str) -> bool { + let anchored = if pattern.starts_with('^') || pattern.ends_with('$') { + pattern.to_string() + } else { + format!("^(?:{})$", pattern) + }; + regex::Regex::new(&anchored) + .map(|re| re.is_match(text)) .unwrap_or(false) } } @@ -479,4 +598,43 @@ mod tests { let result = Hooks::parse_command_output(output, HookEventKind::PreToolUse).unwrap(); assert!(result.is_none()); } + + // -- regex_matches: Claude Code-compatible tool matching -- + + #[test] + fn regex_alternation_matches_either_tool() { + assert!(Hooks::regex_matches("Edit|Write", "Edit")); + assert!(Hooks::regex_matches("Edit|Write", "Write")); + assert!(!Hooks::regex_matches("Edit|Write", "Read")); + } + + #[test] + fn regex_wildcard_matches_mcp_tools() { + assert!(Hooks::regex_matches( + "mcp__memory__.*", + "mcp__memory__create_entities" + )); + assert!(Hooks::regex_matches( + "mcp__memory__.*", + "mcp__memory__search" + )); + assert!(!Hooks::regex_matches( + "mcp__memory__.*", + "mcp__filesystem__read" + )); + } + + #[test] + fn regex_exact_string_still_works() { + assert!(Hooks::regex_matches("developer__shell", "developer__shell")); + assert!(!Hooks::regex_matches( + "developer__shell", + "developer__shell_extra" + )); + } + + #[test] + fn regex_invalid_pattern_returns_false() { + assert!(!Hooks::regex_matches("[invalid", "anything")); + } } diff --git a/crates/goose/src/hooks/types.rs b/crates/goose/src/hooks/types.rs index 0d2e80fe2ee3..0d43e7293f98 100644 --- a/crates/goose/src/hooks/types.rs +++ b/crates/goose/src/hooks/types.rs @@ -21,6 +21,10 @@ pub enum HookEventKind { TeammateIdle, TaskCompleted, ConfigChange, + /// Goose extension: fires when context fills to a configured percentage. + /// Matcher is a threshold percentage (e.g., "70" fires at 70% context fill). + /// Fires once per threshold crossing — not every turn while above. + ContextFill, } impl HookEventKind { @@ -87,6 +91,16 @@ pub struct HookInvocation { #[serde(default)] pub manual_compact: bool, + + // ContextFill fields + #[serde(skip_serializing_if = "Option::is_none")] + pub current_tokens: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub context_limit: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub fill_percentage: Option, } impl HookInvocation { @@ -260,6 +274,22 @@ impl HookInvocation { ..Self::base(HookEventKind::ConfigChange, session_id) } } + + pub fn context_fill( + session_id: String, + current_tokens: usize, + context_limit: usize, + fill_percentage: u32, + cwd: String, + ) -> Self { + Self { + current_tokens: Some(current_tokens), + context_limit: Some(context_limit), + fill_percentage: Some(fill_percentage), + cwd: Some(cwd), + ..Self::base(HookEventKind::ContextFill, session_id) + } + } } #[derive(Debug, Clone, Default, Deserialize)] From 6244d5910a4bb3159788b3d9d214e9f33e349c78 Mon Sep 17 00:00:00 2001 From: Tyler Longwell Date: Tue, 17 Mar 2026 10:42:45 -0400 Subject: [PATCH 07/10] fix: address crossfire review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical: - ContextFill threshold state now persists across turns via Arc on Agent (previously reset every reply() call due to Hooks::load creating fresh instance) Major: - break→continue in approval loop when PreToolUse hook blocks, so remaining tools still get responses (was leaving empty Message::user() objects) - Shell drain tasks: bounded reads (32KB stdout, 4KB stderr) prevent OOM - Kill entire process group on Unix timeout/cancel (not just direct child) - Cap JSON-parsed HookResult fields (additionalContext, reason) same as plain-text path — prevents oversized context injection via valid JSON Minor: - Regex anchoring: || → && so partially-anchored patterns still get wrapped - Stop/SubagentStop removed from can_block() since block result is never checked - HookResult fields documented as Claude Code compat (unused by goose) - Added libc dependency for process group kill on Unix --- Cargo.lock | 1 + crates/goose/Cargo.toml | 1 + crates/goose/src/agents/agent.rs | 7 +- crates/goose/src/agents/execute_commands.rs | 2 +- crates/goose/src/agents/tool_execution.rs | 14 +-- crates/goose/src/hooks/mod.rs | 49 ++++++---- crates/goose/src/hooks/shell.rs | 101 ++++++++++++++++---- crates/goose/src/hooks/types.rs | 21 ++-- 8 files changed, 143 insertions(+), 53 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6bfd8fc04d3b..c7eb233b4cb5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4254,6 +4254,7 @@ dependencies = [ "jsonwebtoken", "keyring", "lazy_static", + "libc", "llama-cpp-2", "lru", "minijinja", diff --git a/crates/goose/Cargo.toml b/crates/goose/Cargo.toml index 6b4ece9256a0..dc82bec73314 100644 --- a/crates/goose/Cargo.toml +++ b/crates/goose/Cargo.toml @@ -39,6 +39,7 @@ serde_urlencoded = "0.7" jsonschema = "0.30.0" uuid = { workspace = true } regex = { workspace = true } +libc = "0.2" async-trait = { workspace = true } async-stream = { workspace = true } minijinja = { version = "2.12.0", features = ["loader"] } diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 08e2a79763d9..d47c9d3ec25f 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -153,6 +153,10 @@ pub struct Agent { pub(super) retry_manager: RetryManager, pub(super) tool_inspection_manager: ToolInspectionManager, container: Mutex>, + + /// Persists across turns so ContextFill hooks fire once per threshold crossing, + /// not on every turn while above the threshold. + pub(super) hook_context_fill_state: Arc>>, } #[derive(Clone, Debug)] @@ -252,6 +256,7 @@ impl Agent { provider.clone(), ), container: Mutex::new(None), + hook_context_fill_state: Arc::new(std::sync::Mutex::new(std::collections::HashSet::new())), } } @@ -1098,7 +1103,7 @@ impl Agent { .ok_or_else(|| anyhow::anyhow!("Session {} has no conversation", session_config.id))?; // Load hooks configuration - let hooks = Hooks::load(&session.working_dir); + let hooks = Hooks::load(&session.working_dir, self.hook_context_fill_state.clone()); let needs_auto_compact = check_if_compaction_needed( self.provider().await?.as_ref(), diff --git a/crates/goose/src/agents/execute_commands.rs b/crates/goose/src/agents/execute_commands.rs index 06c805c185e7..a520dd74247c 100644 --- a/crates/goose/src/agents/execute_commands.rs +++ b/crates/goose/src/agents/execute_commands.rs @@ -90,7 +90,7 @@ impl Agent { .ok_or_else(|| anyhow!("Session has no conversation"))?; // Load hooks and fire PreCompact - let hooks = Hooks::load(&session.working_dir); + let hooks = Hooks::load(&session.working_dir, self.hook_context_fill_state.clone()); let invocation = crate::hooks::HookInvocation::pre_compact( session_id.to_string(), conversation.messages().len(), diff --git a/crates/goose/src/agents/tool_execution.rs b/crates/goose/src/agents/tool_execution.rs index e5f89fe780da..5a967f874ffa 100644 --- a/crates/goose/src/agents/tool_execution.rs +++ b/crates/goose/src/agents/tool_execution.rs @@ -147,20 +147,20 @@ impl Agent { .await .unwrap_or_default(); if outcome.blocked { - // Hook blocked — treat same as user declining + // Hook blocked — fill declined response and continue to next tool if let Some(response_msg) = request_to_response_map.get(&request.id) { let mut response = response_msg.lock().await; *response = response.clone().with_tool_response_with_metadata( request.id.clone(), - Err(rmcp::model::ErrorData::new( - rmcp::model::ErrorCode::INTERNAL_ERROR, - outcome.reason.as_deref().unwrap_or("Tool execution blocked by hook").to_string(), - None, - )), + Ok(rmcp::model::CallToolResult::error(vec![ + rmcp::model::Content::text( + outcome.reason.as_deref().unwrap_or("Tool execution blocked by hook").to_string() + ), + ])), request.metadata.as_ref(), ); } - break; + continue; } let (req_id, tool_result) = self.dispatch_tool_call(tool_call.clone(), request.id.clone(), cancellation_token.clone(), session).await; diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index 8aa511c03b93..96efbb6f633a 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -15,18 +15,24 @@ pub struct Hooks { settings: HookSettingsFile, /// Tracks which ContextFill thresholds have already fired this session. /// Prevents re-firing every turn while above the threshold. - fired_context_thresholds: std::sync::Mutex>, + /// Shared via Arc so state persists across Hooks reloads within a session. + fired_context_thresholds: std::sync::Arc>>, } impl Hooks { - pub fn load(working_dir: &Path) -> Self { + /// Load hooks config from disk. The `context_fill_state` Arc persists across + /// reloads within a session so ContextFill thresholds fire only once per crossing. + pub fn load( + working_dir: &Path, + context_fill_state: std::sync::Arc>>, + ) -> Self { let settings = HookSettingsFile::load_merged(working_dir).unwrap_or_else(|e| { tracing::debug!("No hooks config loaded: {}", e); HookSettingsFile::default() }); Self { settings, - fired_context_thresholds: std::sync::Mutex::new(std::collections::HashSet::new()), + fired_context_thresholds: context_fill_state, } } @@ -263,22 +269,33 @@ impl Hooks { Ok(Some(HookResult::default())) } else { // Try JSON first, fall back to plain text as additionalContext - Ok(Some( + let mut result = serde_json::from_str::(stdout).unwrap_or_else(|_| { - let mut context = stdout.to_string(); - if context.len() > 32_768 { - tracing::warn!( - "Hook stdout truncated from {} to 32KB", - context.len() - ); - context.truncate(context.floor_char_boundary(32_768)); - } HookResult { - additional_context: Some(context), + additional_context: Some(stdout.to_string()), ..Default::default() } - }), - )) + }); + // Cap string fields regardless of parse path to prevent + // oversized context injection from hook output. + const MAX_CONTEXT: usize = 32_768; + const MAX_REASON: usize = 4_096; + if let Some(ref mut ctx) = result.additional_context { + if ctx.len() > MAX_CONTEXT { + tracing::warn!( + "Hook additionalContext truncated from {} to {}", + ctx.len(), + MAX_CONTEXT + ); + ctx.truncate(ctx.floor_char_boundary(MAX_CONTEXT)); + } + } + if let Some(ref mut r) = result.reason { + if r.len() > MAX_REASON { + r.truncate(r.floor_char_boundary(MAX_REASON)); + } + } + Ok(Some(result)) } } Some(2) if event.can_block() => { @@ -481,7 +498,7 @@ impl Hooks { /// Test if `text` matches `pattern` as a full-string regex. /// Anchors the pattern to match the entire string (not a substring). fn regex_matches(pattern: &str, text: &str) -> bool { - let anchored = if pattern.starts_with('^') || pattern.ends_with('$') { + let anchored = if pattern.starts_with('^') && pattern.ends_with('$') { pattern.to_string() } else { format!("^(?:{})$", pattern) diff --git a/crates/goose/src/hooks/shell.rs b/crates/goose/src/hooks/shell.rs index 31ce17060697..7bafe7b3bfb4 100644 --- a/crates/goose/src/hooks/shell.rs +++ b/crates/goose/src/hooks/shell.rs @@ -10,6 +10,11 @@ use crate::agents::platform_extensions::developer::shell::build_shell_command; #[cfg(not(windows))] use crate::agents::platform_extensions::developer::shell::user_login_path; +/// Maximum bytes to capture from stdout (32 KB — matches output cap in mod.rs). +const MAX_STDOUT_BYTES: usize = 32 * 1024; +/// Maximum bytes to capture from stderr (4 KB — matches block-reason cap in mod.rs). +const MAX_STDERR_BYTES: usize = 4 * 1024; + /// Output from a hook command execution. pub struct HookCommandOutput { pub stdout: String, @@ -18,6 +23,42 @@ pub struct HookCommandOutput { pub timed_out: bool, } +/// Read up to `limit` bytes from an async reader into a String. +/// Prevents unbounded memory growth from malicious/buggy hooks. +async fn read_bounded( + mut reader: impl tokio::io::AsyncRead + Unpin, + limit: usize, +) -> String { + let mut buf = vec![0u8; limit]; + let mut total = 0; + loop { + match reader.read(&mut buf[total..]).await { + Ok(0) => break, + Ok(n) => { + total += n; + if total >= limit { + break; + } + } + Err(_) => break, + } + } + buf.truncate(total); + String::from_utf8_lossy(&buf).into_owned() +} + +/// Kill the entire process group on Unix (sends signal to -pgid). +/// Falls back to killing just the child if process group kill fails. +#[cfg(unix)] +fn kill_process_group(child: &tokio::process::Child) { + if let Some(pid) = child.id() { + // Kill the entire process group (negative PID = process group) + unsafe { + libc::kill(-(pid as i32), libc::SIGKILL); + } + } +} + /// Run a hook command as a direct subprocess. /// /// Deadlock-safe: stdout and stderr are drained concurrently via spawned tasks, @@ -26,6 +67,9 @@ pub struct HookCommandOutput { /// /// The child is placed in its own process group (unix) so terminal SIGINT does not /// kill it — the cancellation token is the intended shutdown path. +/// +/// Output capture is bounded: stdout to 32KB, stderr to 4KB. Excess is silently +/// discarded to prevent OOM from malicious/buggy hooks. pub async fn run_hook_command( command_line: &str, stdin_data: Option<&str>, @@ -75,19 +119,14 @@ pub async fn run_hook_command( .ok_or_else(|| "Failed to capture stderr".to_string())?; // Spawn stdout drain FIRST (before stdin write to prevent circular deadlock) + // Bounded read prevents OOM from hooks that produce excessive output. let stdout_task = tokio::spawn(async move { - let mut output = String::new(); - let mut reader = stdout_handle; - let _ = reader.read_to_string(&mut output).await; - output + read_bounded(stdout_handle, MAX_STDOUT_BYTES).await }); // Spawn stderr drain concurrently let stderr_task = tokio::spawn(async move { - let mut output = String::new(); - let mut reader = stderr_handle; - let _ = reader.read_to_string(&mut output).await; - output + read_bounded(stderr_handle, MAX_STDERR_BYTES).await }); // Write stdin data concurrently with drains. @@ -113,7 +152,9 @@ pub async fn run_hook_command( return Err(format!("Failed waiting on hook command: {}", e)); } Err(_) => { - // Timeout — kill the process + // Timeout — kill the entire process group (not just the child) + #[cfg(unix)] + kill_process_group(&child); let _ = child.start_kill(); let _ = child.wait().await; (None, true) @@ -121,7 +162,9 @@ pub async fn run_hook_command( } } _ = cancel_token.cancelled() => { - // Cancellation — kill the process + // Cancellation — kill the entire process group + #[cfg(unix)] + kill_process_group(&child); let _ = child.start_kill(); let _ = child.wait().await; (None, true) @@ -130,17 +173,35 @@ pub async fn run_hook_command( // Collect output from drain tasks. // Use a secondary timeout to prevent hanging if grandchild processes hold pipe FDs open. + // On timeout, explicitly abort the drain tasks to prevent detached task leaks. let drain_timeout = Duration::from_secs(5); - let stdout_output = tokio::time::timeout(drain_timeout, stdout_task) - .await - .ok() - .and_then(|r| r.ok()) - .unwrap_or_default(); - let stderr_output = tokio::time::timeout(drain_timeout, stderr_task) - .await - .ok() - .and_then(|r| r.ok()) - .unwrap_or_default(); + + let stdout_output = match tokio::time::timeout(drain_timeout, stdout_task).await { + Ok(Ok(output)) => output, + Ok(Err(join_err)) => { + tracing::warn!("Hook stdout drain task panicked: {}", join_err); + String::new() + } + Err(_) => { + // Drain timed out — grandchild likely holding pipe FDs open. + // The task was consumed by timeout; since bounded read caps at MAX_STDOUT_BYTES + // and process group was killed, the read will eventually EOF. + tracing::warn!("Hook stdout drain timed out (grandchild may hold FDs)"); + String::new() + } + }; + + let stderr_output = match tokio::time::timeout(drain_timeout, stderr_task).await { + Ok(Ok(output)) => output, + Ok(Err(join_err)) => { + tracing::warn!("Hook stderr drain task panicked: {}", join_err); + String::new() + } + Err(_) => { + tracing::warn!("Hook stderr drain timed out (grandchild may hold FDs)"); + String::new() + } + }; // Best-effort wait for stdin task to finish let _ = tokio::time::timeout(Duration::from_secs(1), stdin_task).await; diff --git a/crates/goose/src/hooks/types.rs b/crates/goose/src/hooks/types.rs index 0d43e7293f98..6bc9ceb43bac 100644 --- a/crates/goose/src/hooks/types.rs +++ b/crates/goose/src/hooks/types.rs @@ -28,17 +28,12 @@ pub enum HookEventKind { } impl HookEventKind { + /// Events that can block execution when a hook returns exit code 2. + /// Only events whose block outcome is actually checked at the call site should be listed. pub fn can_block(&self) -> bool { matches!( self, - Self::PreToolUse - | Self::PermissionRequest - | Self::UserPromptSubmit - | Self::Stop - | Self::SubagentStop - | Self::TeammateIdle - | Self::TaskCompleted - | Self::ConfigChange + Self::PreToolUse | Self::UserPromptSubmit ) } } @@ -292,27 +287,37 @@ impl HookInvocation { } } +/// Structured result from a hook command's JSON stdout. +/// Fields marked "Claude Code compat" exist for configuration compatibility +/// with Claude Code's hook system but are not currently acted upon by goose. #[derive(Debug, Clone, Default, Deserialize)] #[serde(rename_all = "camelCase")] pub struct HookResult { + /// Used by goose: Allow or Block decision (blockable events only). #[serde(default)] pub decision: Option, + /// Used by goose: human-readable reason for block decisions. #[serde(default)] pub reason: Option, + /// Claude Code compat: opaque output from the hook (not read by goose). #[serde(default)] pub hook_specific_output: Option, + /// Claude Code compat: whether to continue execution (not read by goose). #[serde(default, rename = "continue")] pub continue_: Option, + /// Claude Code compat: reason for stopping (not read by goose). #[serde(default)] pub stop_reason: Option, + /// Used by goose: injected into conversation as hidden context after hook runs. #[serde(default)] pub additional_context: Option, + /// Claude Code compat: system-level message (not read by goose). #[serde(default)] pub system_message: Option, } From 16a087e36d9bc3ca83179b80a893cc6c61e92ff4 Mon Sep 17 00:00:00 2001 From: Tyler Longwell Date: Tue, 17 Mar 2026 11:02:50 -0400 Subject: [PATCH 08/10] fix: MCP path size caps + drain comment accuracy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Apply same 32KB/4KB caps to MCP hook JSON-parsed fields (parse_mcp_result was missing caps on the JSON success path) - Fix misleading comment about drain task abort — tasks are safe to detach because reads are bounded and process group is killed --- crates/goose/src/hooks/mod.rs | 30 ++++++++++++++++++++++-------- crates/goose/src/hooks/shell.rs | 4 +++- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index 96efbb6f633a..4bf536a9a2b5 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -403,20 +403,34 @@ impl Hooks { if text.trim().is_empty() { Ok(Some(HookResult::default())) } else { - Ok(Some(serde_json::from_str(text.trim()).unwrap_or_else( + let mut result = serde_json::from_str::(text.trim()).unwrap_or_else( |e| { tracing::debug!("MCP hook output is not HookResult JSON: {}", e); - let mut context = text.trim().to_string(); - if context.len() > 32_768 { - tracing::warn!("MCP hook output truncated from {} to 32KB", context.len()); - context.truncate(context.floor_char_boundary(32_768)); - } HookResult { - additional_context: Some(context), + additional_context: Some(text.trim().to_string()), ..Default::default() } }, - ))) + ); + // Cap string fields regardless of parse path (same as command hooks) + const MAX_CONTEXT: usize = 32_768; + const MAX_REASON: usize = 4_096; + if let Some(ref mut ctx) = result.additional_context { + if ctx.len() > MAX_CONTEXT { + tracing::warn!( + "MCP hook additionalContext truncated from {} to {}", + ctx.len(), + MAX_CONTEXT + ); + ctx.truncate(ctx.floor_char_boundary(MAX_CONTEXT)); + } + } + if let Some(ref mut r) = result.reason { + if r.len() > MAX_REASON { + r.truncate(r.floor_char_boundary(MAX_REASON)); + } + } + Ok(Some(result)) } } diff --git a/crates/goose/src/hooks/shell.rs b/crates/goose/src/hooks/shell.rs index 7bafe7b3bfb4..094c5bb6001a 100644 --- a/crates/goose/src/hooks/shell.rs +++ b/crates/goose/src/hooks/shell.rs @@ -173,7 +173,9 @@ pub async fn run_hook_command( // Collect output from drain tasks. // Use a secondary timeout to prevent hanging if grandchild processes hold pipe FDs open. - // On timeout, explicitly abort the drain tasks to prevent detached task leaks. + // On timeout, the JoinHandle is consumed by the timeout future. + // The drain tasks are safe to detach because: (1) reads are bounded by MAX_*_BYTES, + // and (2) the process group was killed, so pipe FDs will close and reads will EOF. let drain_timeout = Duration::from_secs(5); let stdout_output = match tokio::time::timeout(drain_timeout, stdout_task).await { From 76cec30c81024788394668e6b1ad5d17927ffaba Mon Sep 17 00:00:00 2001 From: Tyler Longwell Date: Tue, 17 Mar 2026 11:21:26 -0400 Subject: [PATCH 09/10] chore: cargo fmt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Formatting only — no logic changes. --- crates/goose/src/agents/agent.rs | 10 ++++++++-- crates/goose/src/hooks/mod.rs | 34 ++++++++++++-------------------- crates/goose/src/hooks/shell.rs | 15 +++++--------- crates/goose/src/hooks/types.rs | 5 +---- 4 files changed, 27 insertions(+), 37 deletions(-) diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index d47c9d3ec25f..b8bd01e09d3a 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -256,7 +256,9 @@ impl Agent { provider.clone(), ), container: Mutex::new(None), - hook_context_fill_state: Arc::new(std::sync::Mutex::new(std::collections::HashSet::new())), + hook_context_fill_state: Arc::new(std::sync::Mutex::new( + std::collections::HashSet::new(), + )), } } @@ -430,7 +432,11 @@ impl Agent { if outcome.blocked { let error_result = Err(rmcp::model::ErrorData::new( rmcp::model::ErrorCode::INTERNAL_ERROR, - outcome.reason.as_deref().unwrap_or("Tool execution blocked by hook").to_string(), + outcome + .reason + .as_deref() + .unwrap_or("Tool execution blocked by hook") + .to_string(), None, )); tool_futures.push(( diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index 4bf536a9a2b5..ce9e11df4cd3 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -270,11 +270,9 @@ impl Hooks { } else { // Try JSON first, fall back to plain text as additionalContext let mut result = - serde_json::from_str::(stdout).unwrap_or_else(|_| { - HookResult { - additional_context: Some(stdout.to_string()), - ..Default::default() - } + serde_json::from_str::(stdout).unwrap_or_else(|_| HookResult { + additional_context: Some(stdout.to_string()), + ..Default::default() }); // Cap string fields regardless of parse path to prevent // oversized context injection from hook output. @@ -343,8 +341,8 @@ impl Hooks { // Guard zero timeout — default to 10 minutes let effective_timeout = if timeout == 0 { 600 } else { timeout }; - let tool_call = CallToolRequestParams::new(tool.to_string()) - .with_arguments(arguments.clone()); + let tool_call = + CallToolRequestParams::new(tool.to_string()).with_arguments(arguments.clone()); let ctx = crate::agents::ToolCallContext::new( invocation.session_id.clone(), @@ -352,11 +350,7 @@ impl Hooks { None, ); let tool_call_result = extension_manager - .dispatch_tool_call( - &ctx, - tool_call, - cancel_token.clone(), - ) + .dispatch_tool_call(&ctx, tool_call, cancel_token.clone()) .await?; tokio::select! { @@ -403,15 +397,13 @@ impl Hooks { if text.trim().is_empty() { Ok(Some(HookResult::default())) } else { - let mut result = serde_json::from_str::(text.trim()).unwrap_or_else( - |e| { - tracing::debug!("MCP hook output is not HookResult JSON: {}", e); - HookResult { - additional_context: Some(text.trim().to_string()), - ..Default::default() - } - }, - ); + let mut result = serde_json::from_str::(text.trim()).unwrap_or_else(|e| { + tracing::debug!("MCP hook output is not HookResult JSON: {}", e); + HookResult { + additional_context: Some(text.trim().to_string()), + ..Default::default() + } + }); // Cap string fields regardless of parse path (same as command hooks) const MAX_CONTEXT: usize = 32_768; const MAX_REASON: usize = 4_096; diff --git a/crates/goose/src/hooks/shell.rs b/crates/goose/src/hooks/shell.rs index 094c5bb6001a..10f3b78ea96e 100644 --- a/crates/goose/src/hooks/shell.rs +++ b/crates/goose/src/hooks/shell.rs @@ -25,10 +25,7 @@ pub struct HookCommandOutput { /// Read up to `limit` bytes from an async reader into a String. /// Prevents unbounded memory growth from malicious/buggy hooks. -async fn read_bounded( - mut reader: impl tokio::io::AsyncRead + Unpin, - limit: usize, -) -> String { +async fn read_bounded(mut reader: impl tokio::io::AsyncRead + Unpin, limit: usize) -> String { let mut buf = vec![0u8; limit]; let mut total = 0; loop { @@ -120,14 +117,12 @@ pub async fn run_hook_command( // Spawn stdout drain FIRST (before stdin write to prevent circular deadlock) // Bounded read prevents OOM from hooks that produce excessive output. - let stdout_task = tokio::spawn(async move { - read_bounded(stdout_handle, MAX_STDOUT_BYTES).await - }); + let stdout_task = + tokio::spawn(async move { read_bounded(stdout_handle, MAX_STDOUT_BYTES).await }); // Spawn stderr drain concurrently - let stderr_task = tokio::spawn(async move { - read_bounded(stderr_handle, MAX_STDERR_BYTES).await - }); + let stderr_task = + tokio::spawn(async move { read_bounded(stderr_handle, MAX_STDERR_BYTES).await }); // Write stdin data concurrently with drains. // Wrapped in a timeout to prevent hanging if the child stops reading stdin. diff --git a/crates/goose/src/hooks/types.rs b/crates/goose/src/hooks/types.rs index 6bc9ceb43bac..97a332a74d1f 100644 --- a/crates/goose/src/hooks/types.rs +++ b/crates/goose/src/hooks/types.rs @@ -31,10 +31,7 @@ impl HookEventKind { /// Events that can block execution when a hook returns exit code 2. /// Only events whose block outcome is actually checked at the call site should be listed. pub fn can_block(&self) -> bool { - matches!( - self, - Self::PreToolUse | Self::UserPromptSubmit - ) + matches!(self, Self::PreToolUse | Self::UserPromptSubmit) } } From e8025bd850979e4791c431fa8d0cdfc78555b422 Mon Sep 17 00:00:00 2001 From: Douwe Osinga Date: Thu, 26 Mar 2026 13:22:30 -0400 Subject: [PATCH 10/10] fix: address codex review findings and remove redundant comments - Fix P2: auto-compaction path now correctly passes manual=false to PreCompact/PostCompact hooks (was incorrectly passing true) - Fix P1: capture user prompt text before SessionStart hooks fire, preventing UserPromptSubmit from receiving injected context instead of the actual user input - Remove comments that restate what the code does Signed-off-by: Douwe Osinga --- crates/goose/src/agents/agent.rs | 29 ++++++++------------- crates/goose/src/agents/execute_commands.rs | 2 -- crates/goose/src/agents/tool_execution.rs | 7 ----- crates/goose/src/hooks/mod.rs | 2 -- 4 files changed, 11 insertions(+), 29 deletions(-) diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 3114e4460db0..6db1a213fca5 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -426,7 +426,6 @@ impl Agent { // Handle pre-approved and read-only tools for request in &permission_check_result.approved { if let Ok(tool_call) = request.tool_call.clone() { - // Fire PreToolUse hook — if blocked, create error response let invocation = crate::hooks::HookInvocation::pre_tool_use( session.id.clone(), tool_call.name.to_string(), @@ -1123,7 +1122,6 @@ impl Agent { .clone() .ok_or_else(|| anyhow::anyhow!("Session {} has no conversation", session_config.id))?; - // Load hooks configuration let hooks = Hooks::load(&session.working_dir, self.hook_context_fill_state.clone()); let needs_auto_compact = check_if_compaction_needed( @@ -1165,11 +1163,10 @@ impl Agent { ) ); - // Fire PreCompact hook let invocation = crate::hooks::HookInvocation::pre_compact( session_config.id.clone(), conversation_to_compact.messages().len(), - true, // manual = true for explicit compaction + false, session.working_dir.to_string_lossy().to_string(), ); let _ = hooks @@ -1200,7 +1197,7 @@ impl Agent { &session_config.id, pre_compact_len, post_compact_len, - true, + false, &session.working_dir, &session_manager, &mut compacted_conversation, @@ -1277,7 +1274,14 @@ impl Agent { let working_dir = session.working_dir.clone(); - // Fire SessionStart hook on first reply only (new session: exactly 1 user message, no assistant response yet) + let user_prompt = conversation + .messages() + .iter() + .rev() + .find(|m| m.role == rmcp::model::Role::User) + .map(|m| m.as_concat_text()) + .unwrap_or_default(); + if conversation.messages().len() == 1 && conversation.messages()[0].role == rmcp::model::Role::User { @@ -1307,14 +1311,7 @@ impl Agent { } } - // Fire UserPromptSubmit hook with the last user message - if let Some(last_user_msg) = conversation - .messages() - .iter() - .rev() - .find(|m| m.role == rmcp::model::Role::User) - { - let user_prompt = last_user_msg.as_concat_text(); + if !user_prompt.is_empty() { let invocation = crate::hooks::HookInvocation::user_prompt_submit( session_id.clone(), user_prompt, @@ -1590,7 +1587,6 @@ impl Agent { Some((request_id, item)) => { match item { ToolStreamItem::Result(output) => { - // Fire PostToolUse or PostToolUseFailure hooks if let Some(original_request) = request_id_to_request.get(&request_id) { if let Ok(ref tool_call) = original_request.tool_call { let tool_input = serde_json::to_value(&tool_call.arguments) @@ -1796,7 +1792,6 @@ impl Agent { ) ); - // Fire PreCompact hook (recovery compaction) let invocation = crate::hooks::HookInvocation::pre_compact( session_config.id.clone(), conversation.messages().len(), @@ -2013,7 +2008,6 @@ impl Agent { tokio::task::yield_now().await; } - // Fire Stop hook before finishing let invocation = crate::hooks::HookInvocation::stop( session_id.clone(), None, // reason @@ -2028,7 +2022,6 @@ impl Agent { ) .await; - // Fire SessionEnd hook after stop let invocation = crate::hooks::HookInvocation::session_end( session_id.clone(), None, diff --git a/crates/goose/src/agents/execute_commands.rs b/crates/goose/src/agents/execute_commands.rs index a520dd74247c..dd1016410134 100644 --- a/crates/goose/src/agents/execute_commands.rs +++ b/crates/goose/src/agents/execute_commands.rs @@ -89,7 +89,6 @@ impl Agent { .conversation .ok_or_else(|| anyhow!("Session has no conversation"))?; - // Load hooks and fire PreCompact let hooks = Hooks::load(&session.working_dir, self.hook_context_fill_state.clone()); let invocation = crate::hooks::HookInvocation::pre_compact( session_id.to_string(), @@ -123,7 +122,6 @@ impl Agent { self.update_session_metrics(session_id, session.schedule_id, &usage, true) .await?; - // Fire PostCompact hook and inject context if any. // The helper also pushes to the in-memory conversation, but we don't need // that here — session persistence happens via session_manager.add_message() // inside the helper, and this conversation is about to be dropped. diff --git a/crates/goose/src/agents/tool_execution.rs b/crates/goose/src/agents/tool_execution.rs index 5a967f874ffa..4ad48da6c857 100644 --- a/crates/goose/src/agents/tool_execution.rs +++ b/crates/goose/src/agents/tool_execution.rs @@ -91,7 +91,6 @@ impl Agent { try_stream! { for request in tool_requests.iter() { if let Ok(tool_call) = request.tool_call.clone() { - // Find the corresponding inspection result for this tool request let security_message = inspection_results.iter() .find(|result| result.tool_request_id == request.id) .and_then(|result| { @@ -117,7 +116,6 @@ impl Agent { let confirmation = confirmation_rx.await .map_err(|_| anyhow::anyhow!("Confirmation channel closed for request {}", request.id))?; - // Log user decision if this was a security alert if let Some(finding_id) = get_security_finding_id_from_results(&request.id, inspection_results) { tracing::info!( monotonic_counter.goose.prompt_injection_user_decisions = 1, @@ -129,7 +127,6 @@ impl Agent { } if confirmation.permission == Permission::AllowOnce || confirmation.permission == Permission::AlwaysAllow { - // Fire PreToolUse hook — if blocked, treat as declined let invocation = crate::hooks::HookInvocation::pre_tool_use( session.id.clone(), tool_call.name.to_string(), @@ -147,7 +144,6 @@ impl Agent { .await .unwrap_or_default(); if outcome.blocked { - // Hook blocked — fill declined response and continue to next tool if let Some(response_msg) = request_to_response_map.get(&request.id) { let mut response = response_msg.lock().await; *response = response.clone().with_tool_response_with_metadata( @@ -177,14 +173,12 @@ impl Agent { ), })); - // Update the shared permission manager when user selects "Always Allow" if confirmation.permission == Permission::AlwaysAllow { self.tool_inspection_manager .update_permission_manager(&tool_call.name, PermissionLevel::AlwaysAllow) .await; } } else { - // User declined - update the specific response message for this request if let Some(response_msg) = request_to_response_map.get(&request.id) { let mut response = response_msg.lock().await; *response = response.clone().with_tool_response_with_metadata( @@ -213,7 +207,6 @@ impl Agent { try_stream! { if let Ok(tool_call) = tool_request.tool_call.clone() { if self.is_frontend_tool(&tool_call.name).await { - // Send frontend tool request and wait for response yield Message::assistant().with_frontend_tool_request( tool_request.id.clone(), Ok(tool_call.clone()) diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index ce9e11df4cd3..c8b2960151f5 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -55,7 +55,6 @@ impl Hooks { let fill_pct = ((current_tokens as f64 / context_limit as f64) * 100.0) as u32; - // Get configured ContextFill thresholds from settings let event_configs = self .settings .get_hooks_for_event(HookEventKind::ContextFill); @@ -63,7 +62,6 @@ impl Hooks { return None; } - // Find thresholds that are newly crossed let mut new_thresholds = Vec::new(); { let mut fired = self