From d0fc739a4ce2e581f4205121e756aa4b2e87e7ef Mon Sep 17 00:00:00 2001 From: Tymofii Pidlisnyi Date: Fri, 21 Aug 2026 00:38:53 +0000 Subject: [PATCH] feat(hooks): add PreToolUseResult event and stable tool_call_id across tool lifecycle (#11120) Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com> Signed-off-by: Douwe Osinga Co-authored-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com> Co-authored-by: Douwe Osinga --- crates/goose/src/agents/agent.rs | 478 +++++- crates/goose/src/agents/final_output_tool.rs | 3 +- .../src/agents/state_machine/ops_recipe.rs | 228 ++- .../src/agents/state_machine/ops_skills.rs | 94 +- .../agents/state_machine/ops_toolcalling.rs | 400 ++++-- .../agents/state_machine/ops_unknown_tool.rs | 139 +- .../state_machine/tests/hooks_lifecycle.rs | 1280 +++++++++++++++++ .../agents/state_machine/tests/pipeline.rs | 6 +- .../reconstruction_isolation_lifecycle.rs | 44 +- crates/goose/src/hooks/mod.rs | 269 +++- .../docs/guides/context-engineering/hooks.md | 8 + 11 files changed, 2661 insertions(+), 288 deletions(-) diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 5d4f9627c..9c6bb8ebb 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -666,16 +666,45 @@ impl Agent { self.hook_manager.emit(event, ctx).await; } + /// Observation-only record of what the `PreToolUse` chain decided. Carries + /// no veto: the decision has already been made by the time this runs. + async fn emit_pre_tool_use_result( + &self, + session: &Session, + tool_call_id: &str, + tool_name: &str, + tool_input: Option<&Value>, + outcome: &crate::hooks::HookChainOutcome, + ) { + if !self + .hook_manager + .has_hooks(crate::hooks::HookEvent::PreToolUseResult) + { + return; + } + let ctx = + crate::hooks::HookContext::new(crate::hooks::HookEvent::PreToolUseResult, &session.id) + .with_tool(tool_name.to_string(), tool_input.cloned()) + .with_tool_call_id(tool_call_id) + .with_working_dir(session.working_dir.to_string_lossy().to_string()) + .with_pre_tool_use_outcome(outcome); + self.hook_manager + .emit(crate::hooks::HookEvent::PreToolUseResult, ctx) + .await; + } + fn with_post_tool_hook( &self, result: ToolCallResult, tool_call: &CallToolRequestParams, session: &Session, + tool_call_id: &str, ) -> ToolCallResult { let hook_manager = self.hook_manager.clone(); let session_id = session.id.clone(); let working_dir = session.working_dir.to_string_lossy().to_string(); let tool_name = tool_call.name.to_string(); + let tool_call_id = tool_call_id.to_string(); let tool_input = tool_call .arguments .as_ref() @@ -706,6 +735,7 @@ impl Agent { if hook_manager.has_hooks(event) { let ctx = crate::hooks::HookContext::new(event, &session_id) .with_tool(tool_name.clone(), tool_input.clone()) + .with_tool_call_id(tool_call_id.as_str()) .with_working_dir(working_dir.clone()); hook_manager.emit(event, ctx).await; } @@ -1176,66 +1206,79 @@ impl Agent { .await .record_tool_arguments(&tool_call.arguments, &session.working_dir); - if self + let tool_input_for_hooks = tool_call + .arguments + .as_ref() + .map(|a| serde_json::Value::Object(a.clone())); + + let pre_tool_outcome = if self .hook_manager .has_hooks(crate::hooks::HookEvent::PreToolUse) { let ctx = crate::hooks::HookContext::new(crate::hooks::HookEvent::PreToolUse, &session.id) - .with_tool( - tool_call.name.to_string(), - tool_call - .arguments - .as_ref() - .map(|a| serde_json::Value::Object(a.clone())), - ) + .with_tool(tool_call.name.to_string(), tool_input_for_hooks.clone()) + .with_tool_call_id(request_id.as_str()) .with_working_dir(session.working_dir.to_string_lossy().to_string()); - if let crate::hooks::HookDecision::Deny { reason, plugin } = self - .hook_manager - .emit_blocking(crate::hooks::HookEvent::PreToolUse, ctx) + self.hook_manager + .emit_blocking_with_outcome(crate::hooks::HookEvent::PreToolUse, ctx) .await - { - return ( - request_id, - Err(ErrorData::new( - ErrorCode::INTERNAL_ERROR, - format!( - "Tool call denied by policy hook `{plugin}`: {reason}. \ - Do not retry; this is a policy denial, not a transient failure." - ), - None, - )), - ); - } - } + } else { + crate::hooks::HookChainOutcome::allow(false) + }; - let tool_input_for_extended = tool_call - .arguments - .as_ref() - .map(|a| serde_json::Value::Object(a.clone())); - self.emit_pre_tool_extended_hooks( - &tool_call.name, - tool_input_for_extended.as_ref(), + // Emitted before the denial returns, so an observer sees the denial + // before the model receives the refusal. Best effort, like every other + // hook emission: a subscriber that fails or is absent changes nothing. + self.emit_pre_tool_use_result( session, + request_id.as_str(), + &tool_call.name, + tool_input_for_hooks.as_ref(), + &pre_tool_outcome, ) .await; + if let crate::hooks::HookDecision::Deny { reason, plugin } = pre_tool_outcome.decision { + return ( + request_id, + Err(ErrorData::new( + ErrorCode::INTERNAL_ERROR, + format!( + "Tool call denied by policy hook `{plugin}`: {reason}. \ + Do not retry; this is a policy denial, not a transient failure." + ), + None, + )), + ); + } + + self.emit_pre_tool_extended_hooks(&tool_call.name, tool_input_for_hooks.as_ref(), session) + .await; + if tool_call.name == FINAL_OUTPUT_TOOL_NAME { return if let Some(final_output_tool) = self.final_output_tool.lock().await.as_mut() { let result = final_output_tool.execute_tool_call(tool_call.clone()).await; - ( - request_id, - Ok(self.with_post_tool_hook(result, &tool_call, session)), - ) + let result = self.with_post_tool_hook(result, &tool_call, session, &request_id); + (request_id, Ok(result)) } else { - ( - request_id, - Err(ErrorData::new( - ErrorCode::INTERNAL_ERROR, - "Final output tool not defined".to_string(), - None, - )), - ) + // This method has always reported a missing final-output tool as + // the outer error. Keep that contract and emit the failure + // observation directly, the same event the wrapper would emit. + let error = ErrorData::new( + ErrorCode::INTERNAL_ERROR, + "Final output tool not defined".to_string(), + None, + ); + let failure = crate::hooks::HookEvent::PostToolUseFailure; + if self.hook_manager.has_hooks(failure) { + let ctx = crate::hooks::HookContext::new(failure, &session.id) + .with_tool(tool_call.name.to_string(), tool_input_for_hooks.clone()) + .with_tool_call_id(request_id.as_str()) + .with_working_dir(session.working_dir.to_string_lossy().to_string()); + self.hook_manager.emit(failure, ctx).await; + } + (request_id, Err(error)) }; } @@ -1273,10 +1316,8 @@ impl Agent { debug!("WAITING_TOOL_END: {}", tool_call.name); - ( - request_id, - Ok(self.with_post_tool_hook(result, &tool_call, session)), - ) + let result = self.with_post_tool_hook(result, &tool_call, session, &request_id); + (request_id, Ok(result)) } /// Save current extension state to session metadata @@ -1696,14 +1737,14 @@ impl Agent { )), Arc::new(DoctorOperation), Arc::new(ProjectOperation), - Arc::new(SkillOperation), - Arc::new(RecipeOperation), + Arc::new(SkillOperation::new(self.hook_manager.clone())), + Arc::new(RecipeOperation::new(self.hook_manager.clone())), Arc::new(ToolExecutionOperation::new( &self.current_goose_mode, self.extension_manager.clone(), self.hook_manager.clone(), )), - Arc::new(UnknownToolOperation), + Arc::new(UnknownToolOperation::new(self.hook_manager.clone())), Arc::new(RetryOperation::new( &self.goal, &self.grind, @@ -4223,6 +4264,7 @@ mod tests { )]))), &tool_call, &session, + "call-post-hook", ); drop(entered); drop(span); @@ -5619,4 +5661,338 @@ echo start >> "$PLUGIN_ROOT/hook.log" assert_eq!(stored.input_tokens, Some(1200)); assert_eq!(stored.output_tokens, Some(340)); } + + /// Plugin fixture that can register several events at once, each with its + /// own matcher and script, and read back the JSON payloads a script recorded. + struct RecordingHookEnv { + _temp_dir: TempDir, + plugin_dir: PathBuf, + } + + /// (event name, matcher or "" for none, script file name, script body) + type HookSpec<'a> = (&'a str, &'a str, &'a str, &'a str); + + impl RecordingHookEnv { + fn new(specs: &[HookSpec<'_>]) -> Self { + let temp_dir = tempfile::tempdir().unwrap(); + let plugin_dir = temp_dir.path().join("test-plugin"); + std::fs::create_dir_all(plugin_dir.join("hooks")).unwrap(); + let entries: Vec = specs + .iter() + .map(|(event, matcher, script, _)| { + let matcher = if matcher.is_empty() { + String::new() + } else { + format!(r#""matcher": "{matcher}", "#) + }; + format!( + r#""{event}": [{{{matcher}"hooks": [{{"type": "command", "command": "sh ${{PLUGIN_ROOT}}/{script}"}}]}}]"# + ) + }) + .collect(); + std::fs::write( + plugin_dir.join("hooks/hooks.json"), + format!(r#"{{"hooks": {{{}}}}}"#, entries.join(", ")), + ) + .unwrap(); + for (_, _, script, script_body) in specs { + std::fs::write(plugin_dir.join(script), script_body).unwrap(); + } + Self { + _temp_dir: temp_dir, + plugin_dir, + } + } + + fn hook_manager(&self) -> crate::hooks::HookManager { + crate::hooks::HookManager::from_plugins_for_test(vec![DiscoveredPlugin { + name: "test-plugin".into(), + root: self.plugin_dir.clone(), + scope: PluginScope::Project, + }]) + } + + fn payloads(&self, log: &str) -> Vec { + std::fs::read_to_string(self.plugin_dir.join(log)) + .unwrap_or_default() + .lines() + .filter(|line| !line.trim().is_empty()) + .map(|line| serde_json::from_str(line).unwrap()) + .collect() + } + } + + const RECORD_PRE_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/pre.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/pre.log\"\nexit 0\n"; + const RECORD_RESULT_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/result.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/result.log\"\nexit 0\n"; + const RECORD_POST_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/post.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/post.log\"\nexit 0\n"; + const RECORD_POST_FAILURE_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/postfail.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/postfail.log\"\nexit 0\n"; + const DENY_AND_RECORD_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/pre.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/pre.log\"\necho \"blocked by test policy\" >&2\nexit 2\n"; + /// Logs its stdin like the others, writes nothing to stdout, and exits + /// non-zero. That is a hook that ran but never returned a decision. + const ABNORMAL_EXIT_AND_RECORD_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/pre.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/pre.log\"\necho boom >&2\nexit 3\n"; + + async fn agent_with_hooks( + hook_manager: crate::hooks::HookManager, + ) -> (Agent, Session, TempDir) { + let data_dir = TempDir::new().unwrap(); + let data_path = data_dir.path().to_path_buf(); + let session_manager = Arc::new(SessionManager::new(data_path.clone())); + let mut agent = Agent::with_config(AgentConfig::new( + Arc::clone(&session_manager), + Arc::new(PermissionManager::new(data_path)), + None, + GooseMode::default(), + false, + GoosePlatform::GooseCli, + )); + agent.set_hook_manager_for_test(hook_manager); + let session = session_manager + .create_session( + std::env::current_dir().unwrap(), + "pre-tool-use-result".to_string(), + SessionType::Hidden, + GooseMode::default(), + ) + .await + .unwrap(); + (agent, session, data_dir) + } + + fn shell_call() -> CallToolRequestParams { + use rmcp::object; + CallToolRequestParams::new("developer__shell") + .with_arguments(object!({ "command": "echo hi" })) + } + + /// deny-invisible: the tool never dispatches, neither post event fires, and a + /// PreToolUseResult subscriber still sees the denial with blocked_by and reason. + #[tokio::test] + async fn pre_tool_use_result_observes_denial_that_post_hooks_never_see() { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", DENY_AND_RECORD_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (agent, session, _data_dir) = agent_with_hooks(env.hook_manager()).await; + + let (request_id, result) = agent + .dispatch_tool_call(shell_call(), "call-deny-1".to_string(), None, &session) + .await; + + assert_eq!(request_id, "call-deny-1"); + let Err(error) = result else { + panic!("a denied call must not dispatch"); + }; + assert!(error.message.contains("denied by policy hook")); + + assert!( + env.payloads("post.log").is_empty(), + "PostToolUse must not fire for a denied call" + ); + assert!( + env.payloads("postfail.log").is_empty(), + "PostToolUseFailure must not fire for a denied call" + ); + + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["event"], "PreToolUseResult"); + assert_eq!(results[0]["decision"], "deny"); + assert_eq!(results[0]["policy_evaluated"], true); + assert_eq!(results[0]["blocked_by"], "test-plugin"); + assert_eq!(results[0]["reason"], "blocked by test policy"); + assert_eq!(results[0]["tool_call_id"], "call-deny-1"); + } + + /// repeated identical calls: two calls with the same name and input in one + /// session correlate to their outcomes by tool_call_id, not by name plus input. + #[tokio::test] + async fn repeated_identical_calls_correlate_by_tool_call_id() { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (agent, session, _data_dir) = agent_with_hooks(env.hook_manager()).await; + + for id in ["call-1", "call-2"] { + let (_, result) = agent + .dispatch_tool_call(shell_call(), id.to_string(), None, &session) + .await; + let Ok(handle) = result else { + panic!("dispatch must return a result handle"); + }; + let _ = handle.result.await; + } + + let pres = env.payloads("pre.log"); + let results = env.payloads("result.log"); + let outcomes = env.payloads("postfail.log"); + assert_eq!(pres.len(), 2); + assert_eq!(results.len(), 2); + assert_eq!(outcomes.len(), 2); + + for payloads in [&pres, &results, &outcomes] { + assert_eq!(payloads[0]["tool_name"], payloads[1]["tool_name"]); + assert_eq!(payloads[0]["tool_input"], payloads[1]["tool_input"]); + } + + let ids: Vec<&str> = results + .iter() + .map(|payload| payload["tool_call_id"].as_str().unwrap()) + .collect(); + assert_eq!(ids, vec!["call-1", "call-2"]); + assert_ne!( + ids[0], ids[1], + "identical name and input must still carry distinct ids" + ); + + for (index, id) in ids.iter().enumerate() { + assert_eq!( + pres[index]["tool_call_id"], results[index]["tool_call_id"], + "PreToolUse and PreToolUseResult must carry one id per call" + ); + assert_eq!( + outcomes + .iter() + .filter(|payload| payload["tool_call_id"] == *id) + .count(), + 1, + "each call must pair with exactly one outcome by id" + ); + } + } + + /// no matching hook: a PreToolUse rule is registered but its matcher does not + /// match, so nothing runs and the event reports allow with policy_evaluated false. + #[tokio::test] + async fn pre_tool_use_result_reports_allow_and_unevaluated_when_no_hook_matches() { + let env = RecordingHookEnv::new(&[ + ( + "PreToolUse", + "a_tool_name_that_never_matches", + "pre.sh", + DENY_AND_RECORD_SCRIPT, + ), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ]); + let (agent, session, _data_dir) = agent_with_hooks(env.hook_manager()).await; + + let (_, result) = agent + .dispatch_tool_call(shell_call(), "call-allow-1".to_string(), None, &session) + .await; + let Ok(handle) = result else { + panic!("dispatch must return a result handle"); + }; + let _ = handle.result.await; + + assert!( + env.payloads("pre.log").is_empty(), + "the non-matching rule must not run" + ); + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["decision"], "allow"); + assert_eq!(results[0]["policy_evaluated"], false); + assert!(results[0].get("blocked_by").is_none()); + assert!(results[0].get("reason").is_none()); + assert_eq!(results[0]["tool_call_id"], "call-allow-1"); + } + + /// sole abnormal hook: the only matching PreToolUse hook runs, writes nothing + /// to stdout and exits non-zero, so it never returned a decision. Execution + /// stays fail-open and the event reports allow with policy_evaluated false. + #[tokio::test] + async fn pre_tool_use_result_reports_unevaluated_when_the_only_hook_exits_without_a_decision() { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", ABNORMAL_EXIT_AND_RECORD_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ]); + let (agent, session, _data_dir) = agent_with_hooks(env.hook_manager()).await; + + let (_, result) = agent + .dispatch_tool_call(shell_call(), "call-abnormal-1".to_string(), None, &session) + .await; + let Ok(handle) = result else { + panic!("dispatch must stay fail-open and return a result handle"); + }; + let _ = handle.result.await; + + assert_eq!( + env.payloads("pre.log").len(), + 1, + "the matching hook must still run", + ); + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["decision"], "allow"); + assert_eq!(results[0]["policy_evaluated"], false); + assert_eq!(results[0]["tool_call_id"], "call-abnormal-1"); + } + + /// inactive final output: the tool is not installed, so nothing executes. The + /// outer error stays the one this method has always returned, and the failure + /// is still observed exactly once, carrying the request id. + #[tokio::test] + async fn inactive_final_output_keeps_the_outer_error_and_emits_one_failure_event() { + use rmcp::object; + + let env = RecordingHookEnv::new(&[ + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + // agent_with_hooks builds the agent through Agent::with_config, which + // leaves final_output_tool as None, so the tool is inactive here without + // any extra setup. + let (agent, session, _data_dir) = agent_with_hooks(env.hook_manager()).await; + + let call = CallToolRequestParams::new(FINAL_OUTPUT_TOOL_NAME) + .with_arguments(object!({ "answer": "unused" })); + let (_, result) = agent + .dispatch_tool_call(call, "call-inactive-1".to_string(), None, &session) + .await; + + let Err(error) = result else { + panic!("an inactive final-output tool must report the outer error"); + }; + assert_eq!(error.message, "Final output tool not defined"); + assert_eq!(error.code, ErrorCode::INTERNAL_ERROR); + + let failures = env.payloads("postfail.log"); + assert_eq!( + failures.len(), + 1, + "the failure must be observed exactly once", + ); + assert_eq!(failures[0]["tool_call_id"], "call-inactive-1"); + assert_eq!(failures[0]["tool_name"], FINAL_OUTPUT_TOOL_NAME); + assert!( + env.payloads("post.log").is_empty(), + "PostToolUse must not fire for a tool that never ran", + ); + } } diff --git a/crates/goose/src/agents/final_output_tool.rs b/crates/goose/src/agents/final_output_tool.rs index 08ab940f6..f4b2e749b 100644 --- a/crates/goose/src/agents/final_output_tool.rs +++ b/crates/goose/src/agents/final_output_tool.rs @@ -8,6 +8,7 @@ use serde_json::Value; use std::borrow::Cow; pub const FINAL_OUTPUT_TOOL_NAME: &str = "recipe__final_output"; +pub const FINAL_OUTPUT_SUCCESS_MESSAGE: &str = "Final output successfully collected."; pub const FINAL_OUTPUT_CONTINUATION_MESSAGE: &str = "You MUST call the `final_output` tool NOW with the final output for the user."; @@ -131,7 +132,7 @@ impl FinalOutputTool { Ok(parsed_value) => { self.final_output = Some(Self::parsed_final_output_string(parsed_value)); ToolCallResult::from(Ok(rmcp::model::CallToolResult::success(vec![ - ContentBlock::text("Final output successfully collected.".to_string()), + ContentBlock::text(FINAL_OUTPUT_SUCCESS_MESSAGE.to_string()), ]))) } Err(error) => ToolCallResult::from(Err(ErrorData { diff --git a/crates/goose/src/agents/state_machine/ops_recipe.rs b/crates/goose/src/agents/state_machine/ops_recipe.rs index 1154d483f..d80d4da18 100644 --- a/crates/goose/src/agents/state_machine/ops_recipe.rs +++ b/crates/goose/src/agents/state_machine/ops_recipe.rs @@ -4,26 +4,36 @@ use std::collections::HashSet; use anyhow::{anyhow, Result}; use async_trait::async_trait; -use rmcp::model::Tool; +use rmcp::model::{CallToolResult, ContentBlock, Tool}; use tracing_futures::Instrument; use crate::agents::final_output_tool::{ - FinalOutputTool, FINAL_OUTPUT_CONTINUATION_MESSAGE, FINAL_OUTPUT_TOOL_NAME, + FinalOutputTool, FINAL_OUTPUT_CONTINUATION_MESSAGE, FINAL_OUTPUT_SUCCESS_MESSAGE, + FINAL_OUTPUT_TOOL_NAME, }; use crate::agents::state_machine::ops_toolcalling::{ - pending_tool_requests, tool_span, ToolDisposition, + emit_post_tool_use, pending_tool_requests, run_pre_tool_hooks, tool_span, ToolDisposition, }; use crate::agents::state_machine::{ applied, ends_turn, last_effective_role, messages_since_kickoff, not_applicable, yielded_with, ConversationEffect, Emitter, GooseEffect, Operation, OperationResult, SlashCommand, }; +use crate::agents::tool_execution::CHAT_MODE_TOOL_SKIPPED_RESPONSE; +use crate::config::GooseMode; use crate::conversation::message::{Message, MessageContent}; use crate::conversation::{Conversation, EffectiveRole}; +use crate::hooks::HookManager; use crate::session::Session; -pub struct RecipeOperation; +pub struct RecipeOperation { + hook_manager: HookManager, +} impl RecipeOperation { + pub fn new(hook_manager: HookManager) -> Self { + Self { hook_manager } + } + fn final_output(session: &Session) -> Result> { session .recipe @@ -34,16 +44,82 @@ impl RecipeOperation { .map_err(|error| anyhow!(error)) } + fn assistant_block_bounds(messages: &[Message], message_index: usize) -> (usize, usize) { + let start = (0..message_index) + .rev() + .take_while(|index| messages[*index].role == rmcp::model::Role::Assistant) + .last() + .unwrap_or(message_index); + let end = (message_index + 1..messages.len()) + .take_while(|index| messages[*index].role == rmcp::model::Role::Assistant) + .last() + .map_or(message_index + 1, |index| index + 1); + (start, end) + } + + fn has_unanswered_siblings(messages: &[Message], request_id: &str) -> bool { + let answered: HashSet<&str> = messages + .iter() + .flat_map(|message| &message.content) + .filter_map(|content| match content { + MessageContent::ToolResponse(response) => Some(response.id.as_str()), + _ => None, + }) + .collect(); + let Some(message_index) = messages.iter().position(|message| { + message.content.iter().any(|content| { + matches!( + content, + MessageContent::ToolRequest(request) if request.id == request_id + ) + }) + }) else { + return false; + }; + let (start, end) = Self::assistant_block_bounds(messages, message_index); + messages[start..end] + .iter() + .flat_map(|message| &message.content) + .any(|content| match content { + // Another unanswered final-output call is not a reason to wait. + // This operation drains them one per pass, so treating a sibling + // final-output call as unfinished work would deadlock the pair: + // each would wait for the other and neither would be answered. + // Ordinary tool calls still have to finish first. + MessageContent::ToolRequest(request) => { + request.id != request_id + && !answered.contains(request.id.as_str()) + && !request + .tool_call + .as_ref() + .is_ok_and(|tool_call| tool_call.name == FINAL_OUTPUT_TOOL_NAME) + } + _ => false, + }) + } + fn successful_final_output(messages: &[Message]) -> Option { + let answered_responses: HashSet<&str> = messages + .iter() + .flat_map(|message| &message.content) + .filter_map(|content| match content { + MessageContent::ToolResponse(response) => Some(response.id.as_str()), + _ => None, + }) + .collect(); let successful_responses: HashSet<&str> = messages .iter() .flat_map(|message| &message.content) .filter_map(|content| match content { MessageContent::ToolResponse(response) - if response - .tool_result - .as_ref() - .is_ok_and(|result| result.is_error != Some(true)) => + if response.tool_result.as_ref().is_ok_and(|result| { + result.is_error != Some(true) + && result.content.iter().any(|content| { + content + .as_text() + .is_some_and(|text| text.text == FINAL_OUTPUT_SUCCESS_MESSAGE) + }) + }) => { Some(response.id.as_str()) } @@ -51,25 +127,42 @@ impl RecipeOperation { }) .collect(); - messages - .iter() - .rev() - .flat_map(|message| message.content.iter().rev()) - .find_map(|content| match content { - MessageContent::ToolRequest(request) - if successful_responses.contains(request.id.as_str()) => - { - request.tool_call.as_ref().ok().and_then(|tool_call| { - (tool_call.name == FINAL_OUTPUT_TOOL_NAME).then(|| { - serde_json::Value::Object( - tool_call.arguments.clone().unwrap_or_default(), - ) - .to_string() + for (message_index, message) in messages.iter().enumerate().rev() { + let output = message + .content + .iter() + .rev() + .find_map(|content| match content { + MessageContent::ToolRequest(request) + if successful_responses.contains(request.id.as_str()) => + { + request.tool_call.as_ref().ok().and_then(|tool_call| { + (tool_call.name == FINAL_OUTPUT_TOOL_NAME).then(|| { + serde_json::Value::Object( + tool_call.arguments.clone().unwrap_or_default(), + ) + .to_string() + }) }) - }) - } - _ => None, - }) + } + _ => None, + }); + if output.is_some() { + let (block_start, block_end) = + Self::assistant_block_bounds(messages, message_index); + let siblings_answered = messages[block_start..block_end] + .iter() + .flat_map(|message| &message.content) + .all(|content| match content { + MessageContent::ToolRequest(request) => { + answered_responses.contains(request.id.as_str()) + } + _ => true, + }); + return siblings_answered.then_some(output).flatten(); + } + } + None } async fn command_error( @@ -200,24 +293,81 @@ impl Operation for RecipeOperation { .is_ok_and(|tool_call| tool_call.name == FINAL_OUTPUT_TOOL_NAME) }); if let Some((request, _)) = pending { + if session.goose_mode == GooseMode::Chat { + let mut response = Message::user(); + response.add_tool_response_with_metadata( + request.id, + Ok(CallToolResult::success(vec![ContentBlock::text( + CHAT_MODE_TOOL_SKIPPED_RESPONSE, + )])), + request.metadata.as_ref(), + ); + let response = emit.message(response).await; + return applied([response.into()]); + } + if Self::has_unanswered_siblings(messages, &request.id) { + return not_applicable(); + } + let tool_call = request .tool_call .map_err(|error| anyhow!("final output tool call could not be parsed: {error}"))?; let span = tool_span(&tool_call.name, &request.id, &session.id); - let result = final_output - .execute_tool_call(tool_call) - .instrument(span.clone()) - .await; - let output = result.result.instrument(span.clone()).await; - match &output { - Ok(result) if result.is_error == Some(true) => { - span.record("error.type", "tool_error"); + // `recipe__final_output` is executed here rather than by + // ToolExecutionOperation, which is registered after this one. Run the + // same hook lifecycle it would have run, so the state machine and the + // legacy loop agree on what a final-output call emits. + let tool_input = tool_call + .arguments + .as_ref() + .map(|arguments| serde_json::Value::Object(arguments.clone())); + let output = match run_pre_tool_hooks( + &self.hook_manager, + session, + &request.id, + &tool_call.name, + tool_input.as_ref(), + ) + .instrument(span.clone()) + .await + { + // A denial returns before execution and emits no post event, the + // same shape ToolExecutionOperation has: its dispatch returns the + // denial before the post-hook wrapper is ever applied. + Err(denial) => Err(denial), + Ok(()) => { + let result = final_output + .execute_tool_call(tool_call.clone()) + .instrument(span.clone()) + .await; + let output = result.result.instrument(span.clone()).await; + match &output { + Ok(result) if result.is_error == Some(true) => { + span.record("error.type", "tool_error"); + } + Err(_) => { + span.record("error.type", "tool_execution_error"); + } + _ => {} + } + // Post event carries the same tool_call_id as the pre events. + // The large-response rewrite ToolExecutionOperation applies is + // deliberately not reused: the recipe's structured output is + // the deliverable, not a payload to offload to a temp file. + emit_post_tool_use( + &self.hook_manager, + &session.id, + &session.working_dir.to_string_lossy(), + &tool_call.name, + &request.id, + tool_input.as_ref(), + &output, + ) + .instrument(span.clone()) + .await; + output } - Err(_) => { - span.record("error.type", "tool_execution_error"); - } - _ => {} - } + }; let mut response = Message::user(); response.add_tool_response_with_metadata(request.id, output, request.metadata.as_ref()); let response = emit.message(response).await; diff --git a/crates/goose/src/agents/state_machine/ops_skills.rs b/crates/goose/src/agents/state_machine/ops_skills.rs index 1575ee930..f43e81d50 100644 --- a/crates/goose/src/agents/state_machine/ops_skills.rs +++ b/crates/goose/src/agents/state_machine/ops_skills.rs @@ -5,13 +5,14 @@ use std::path::{Path, PathBuf}; use anyhow::{anyhow, Result}; use async_trait::async_trait; use goose_sdk_types::custom_requests::{SourceEntry, SourceType}; -use rmcp::model::{CallToolResult, ContentBlock, JsonObject, Tool}; +use rmcp::model::{CallToolResult, ContentBlock, ErrorData, JsonObject, Tool}; use schemars::{schema_for, JsonSchema}; use serde::Deserialize; use serde_json::Value; +use tracing_futures::Instrument; use crate::agents::state_machine::ops_toolcalling::{ - pending_tool_requests, tool_span, ToolDisposition, + emit_post_tool_use, pending_tool_requests, run_pre_tool_hooks, tool_span, ToolDisposition, }; use crate::agents::state_machine::{ applied, messages_since_kickoff, not_applicable, yielded_with, ConversationEffect, Emitter, @@ -21,11 +22,14 @@ use crate::agents::tool_execution::{CHAT_MODE_TOOL_SKIPPED_RESPONSE, DECLINED_RE use crate::config::GooseMode; use crate::conversation::message::Message; use crate::conversation::Conversation; +use crate::hooks::HookManager; use crate::session::Session; const LOAD_SKILL_TOOL_NAME: &str = "load_skill"; -pub struct SkillOperation; +pub struct SkillOperation { + hook_manager: HookManager, +} #[derive(Deserialize, JsonSchema)] struct LoadSkillParams { @@ -187,6 +191,10 @@ fn load_supporting_file( } impl SkillOperation { + pub fn new(hook_manager: HookManager) -> Self { + Self { hook_manager } + } + async fn command_response( conversation: &Conversation, message: String, @@ -308,38 +316,80 @@ impl Operation for SkillOperation { let mut response = Message::user(); for (request, disposition) in pending { - let result = match disposition { + let result: std::result::Result = match disposition { ToolDisposition::Execute if session.goose_mode == GooseMode::Chat => { - CallToolResult::success(vec![ContentBlock::text(CHAT_MODE_TOOL_SKIPPED_RESPONSE)]) + // Nothing executes in chat mode, so no tool lifecycle runs. + Ok(CallToolResult::success(vec![ContentBlock::text( + CHAT_MODE_TOOL_SKIPPED_RESPONSE, + )])) } ToolDisposition::Execute => { let tool_call = request.tool_call.as_ref().map_err(|error| { anyhow!("load_skill tool call could not be parsed: {error}") })?; let span = tool_span(&tool_call.name, &request.id, &session.id); - let result = { - let _entered = span.enter(); - execute_skill(&session.working_dir, tool_call.arguments.clone()) - }; - if result.is_error == Some(true) { - span.record("error.type", "tool_error"); + // `load_skill` is executed here rather than by + // ToolExecutionOperation, which is registered after this one. + // Run the same hook lifecycle it would have run, so the state + // machine and the legacy loop agree on what a skill load emits. + let tool_input = tool_call + .arguments + .as_ref() + .map(|arguments| Value::Object(arguments.clone())); + match run_pre_tool_hooks( + &self.hook_manager, + session, + &request.id, + &tool_call.name, + tool_input.as_ref(), + ) + .instrument(span.clone()) + .await + { + // A denial returns before execution and emits no post + // event, the same shape ToolExecutionOperation has: its + // dispatch returns the denial before the post-hook wrapper + // is ever applied. + Err(denial) => Err(denial), + Ok(()) => { + let result = { + let _entered = span.enter(); + execute_skill(&session.working_dir, tool_call.arguments.clone()) + }; + if result.is_error == Some(true) { + span.record("error.type", "tool_error"); + } + let output = Ok(result); + // Post event carries the same tool_call_id as the pre + // events. The large-response rewrite + // ToolExecutionOperation applies is deliberately not + // reused: a skill body is content the model is meant to + // read, not a payload to offload to a temp file. + emit_post_tool_use( + &self.hook_manager, + &session.id, + &session.working_dir.to_string_lossy(), + &tool_call.name, + &request.id, + tool_input.as_ref(), + &output, + ) + .instrument(span.clone()) + .await; + output + } } - result - } - ToolDisposition::Decline => { - CallToolResult::error(vec![ContentBlock::text(DECLINED_RESPONSE)]) } + ToolDisposition::Decline => Ok(CallToolResult::error(vec![ContentBlock::text( + DECLINED_RESPONSE, + )])), ToolDisposition::ParseError(error) => { - CallToolResult::error(vec![ContentBlock::text(format!( + Ok(CallToolResult::error(vec![ContentBlock::text(format!( "The tool call could not be parsed: {error}. Correct the arguments and try again." - ))]) + ))])) } }; - response.add_tool_response_with_metadata( - request.id, - Ok(result), - request.metadata.as_ref(), - ); + response.add_tool_response_with_metadata(request.id, result, request.metadata.as_ref()); } let response = emit.message(response).await; applied([response.into()]) diff --git a/crates/goose/src/agents/state_machine/ops_toolcalling.rs b/crates/goose/src/agents/state_machine/ops_toolcalling.rs index 97106c220..8596b1300 100644 --- a/crates/goose/src/agents/state_machine/ops_toolcalling.rs +++ b/crates/goose/src/agents/state_machine/ops_toolcalling.rs @@ -22,7 +22,7 @@ use crate::config::GooseMode; use crate::conversation::message::{ActionRequiredData, Message, MessageContent, ToolRequest}; use crate::conversation::Conversation; use crate::hints::load_hints::SubdirectoryHintTracker; -use crate::hooks::{HookContext, HookDecision, HookEvent, HookManager}; +use crate::hooks::{HookChainOutcome, HookContext, HookDecision, HookEvent, HookManager}; use crate::session::{EnabledExtensionsState, ExtensionState, Session}; use std::sync::Arc; use tokio::sync::Mutex; @@ -80,6 +80,239 @@ pub(super) fn tool_span(tool_name: &str, tool_call_id: &str, session_id: &str) - ) } +/// Observation-only record of what the `PreToolUse` chain decided. Carries +/// no veto: the decision has already been made by the time this runs. +async fn emit_pre_tool_use_result( + hook_manager: &HookManager, + session: &Session, + tool_call_id: &str, + tool_name: &str, + tool_input: Option<&serde_json::Value>, + outcome: &HookChainOutcome, +) { + if !hook_manager.has_hooks(HookEvent::PreToolUseResult) { + return; + } + let context = HookContext::new(HookEvent::PreToolUseResult, &session.id) + .with_tool(tool_name.to_string(), tool_input.cloned()) + .with_tool_call_id(tool_call_id) + .with_working_dir(session.working_dir.to_string_lossy().to_string()) + .with_pre_tool_use_outcome(outcome); + hook_manager + .emit(HookEvent::PreToolUseResult, context) + .await; +} + +/// Runs the `PreToolUse` chain, emits `PreToolUseResult`, and reports a denial +/// as the error the caller must return instead of executing. +/// +/// Shared rather than duplicated because it carries policy: which decisions +/// block, what `policy_evaluated` means, and that the result event is emitted +/// on both the allow and the deny path. `RecipeOperation` executes +/// `recipe__final_output` without going through [`ToolExecutionOperation`], so +/// it calls this to get the identical lifecycle rather than its own copy. +pub(super) async fn run_pre_tool_hooks( + hook_manager: &HookManager, + session: &Session, + tool_call_id: &str, + tool_name: &str, + tool_input: Option<&serde_json::Value>, +) -> std::result::Result<(), ErrorData> { + let outcome = if hook_manager.has_hooks(HookEvent::PreToolUse) { + let context = HookContext::new(HookEvent::PreToolUse, &session.id) + .with_tool(tool_name.to_string(), tool_input.cloned()) + .with_tool_call_id(tool_call_id) + .with_working_dir(session.working_dir.to_string_lossy().to_string()); + hook_manager + .emit_blocking_with_outcome(HookEvent::PreToolUse, context) + .await + } else { + HookChainOutcome::allow(false) + }; + + // Emitted before the denial returns, so an observer sees the denial + // before the model receives the refusal. Best effort, like every other + // hook emission: a subscriber that fails or is absent changes nothing. + emit_pre_tool_use_result( + hook_manager, + session, + tool_call_id, + tool_name, + tool_input, + &outcome, + ) + .await; + + if let HookDecision::Deny { reason, plugin } = outcome.decision { + tracing::Span::current().record("error.type", "hook_denied"); + return Err(ErrorData::new( + rmcp::model::ErrorCode::INTERNAL_ERROR, + format!( + "Tool call denied by policy hook `{plugin}`: {reason}. \ + Do not retry; this is a policy denial, not a transient failure." + ), + None, + )); + } + Ok(()) +} + +async fn emit_with_matcher( + hook_manager: &HookManager, + event: HookEvent, + session: &Session, + matcher: String, + tool_name: &str, + tool_input: Option, +) { + if !hook_manager.has_hooks(event) { + return; + } + let mut context = HookContext::new(event, &session.id) + .with_tool(tool_name.to_string(), tool_input) + .with_working_dir(session.working_dir.to_string_lossy().to_string()); + context.matcher_context = Some(matcher); + hook_manager.emit(event, context).await; +} + +pub(super) async fn emit_extended_pre_hooks( + hook_manager: &HookManager, + tool_name: &str, + tool_input: Option<&serde_json::Value>, + session: &Session, +) { + let (event, matcher) = match categorize_tool(tool_name) { + ToolCategory::Shell => ( + HookEvent::BeforeShellExecution, + tool_input.and_then(|input| string_argument(input, &["command"])), + ), + ToolCategory::Read => ( + HookEvent::BeforeReadFile, + tool_input.and_then(|input| string_argument(input, &["path", "file", "file_path"])), + ), + ToolCategory::Write | ToolCategory::Other => return, + }; + if let Some(matcher) = matcher { + emit_with_matcher( + hook_manager, + event, + session, + matcher, + tool_name, + tool_input.cloned(), + ) + .await; + } +} + +/// Emits the post-tool event for a finished call and reports which one fired. +/// +/// Which event fires is policy: a result flagged `is_error` counts as a failure, +/// as does a transport error. Shared so every execution path classifies the +/// outcome the same way. Takes the session id and working dir as strings because +/// the caller inside [`with_post_tool_hooks`] holds owned copies, not a session. +pub(super) async fn emit_post_tool_use( + hook_manager: &HookManager, + session_id: &str, + working_dir: &str, + tool_name: &str, + tool_call_id: &str, + tool_input: Option<&serde_json::Value>, + result: &std::result::Result, +) -> HookEvent { + let event = match result { + Ok(result) if result.is_error != Some(true) => HookEvent::PostToolUse, + _ => HookEvent::PostToolUseFailure, + }; + if hook_manager.has_hooks(event) { + let context = HookContext::new(event, session_id) + .with_tool(tool_name.to_string(), tool_input.cloned()) + .with_tool_call_id(tool_call_id) + .with_working_dir(working_dir.to_string()); + hook_manager.emit(event, context).await; + } + event +} + +/// Wraps a tool result so the post-tool event fires once the call completes, +/// carrying the same `tool_call_id` the pre events carried. +pub(super) fn with_post_tool_hooks( + hook_manager: &HookManager, + result: ToolCallResult, + tool_call: &CallToolRequestParams, + session: &Session, + span: tracing::Span, + tool_call_id: &str, +) -> ToolCallResult { + let hook_manager = hook_manager.clone(); + let session_id = session.id.clone(); + let working_dir = session.working_dir.to_string_lossy().to_string(); + let tool_name = tool_call.name.to_string(); + let tool_call_id = tool_call_id.to_string(); + let tool_input = tool_call + .arguments + .as_ref() + .map(|arguments| serde_json::Value::Object(arguments.clone())); + let category = categorize_tool(&tool_name); + let future = async move { + let result = + crate::agents::large_response_handler::process_tool_response(result.result.await); + match &result { + Ok(result) if result.is_error == Some(true) => { + tracing::Span::current().record("error.type", "tool_error"); + } + Err(_) => { + tracing::Span::current().record("error.type", "tool_execution_error"); + } + _ => {} + } + let event = emit_post_tool_use( + &hook_manager, + &session_id, + &working_dir, + &tool_name, + &tool_call_id, + tool_input.as_ref(), + &result, + ) + .await; + + if event == HookEvent::PostToolUse { + let extended = match category { + ToolCategory::Shell => Some(( + HookEvent::AfterShellExecution, + tool_input + .as_ref() + .and_then(|input| string_argument(input, &["command"])), + )), + ToolCategory::Write => Some(( + HookEvent::AfterFileEdit, + tool_input + .as_ref() + .and_then(|input| string_argument(input, &["path", "file", "file_path"])), + )), + ToolCategory::Read | ToolCategory::Other => None, + }; + if let Some((event, Some(matcher))) = extended { + if hook_manager.has_hooks(event) { + let mut context = HookContext::new(event, &session_id) + .with_tool(tool_name, tool_input) + .with_working_dir(working_dir); + context.matcher_context = Some(matcher); + hook_manager.emit(event, context).await; + } + } + } + result + } + .instrument(span); + ToolCallResult { + notification_stream: result.notification_stream, + action_required_stream: result.action_required_stream, + result: Box::new(future.boxed()), + } +} + pub struct ToolExecutionOperation<'a> { goose_mode: &'a Mutex, extension_manager: Arc, @@ -99,122 +332,6 @@ impl<'a> ToolExecutionOperation<'a> { } } - async fn emit_with_matcher( - &self, - event: HookEvent, - session: &Session, - matcher: String, - tool_name: &str, - tool_input: Option, - ) { - if !self.hook_manager.has_hooks(event) { - return; - } - let mut context = HookContext::new(event, &session.id) - .with_tool(tool_name.to_string(), tool_input) - .with_working_dir(session.working_dir.to_string_lossy().to_string()); - context.matcher_context = Some(matcher); - self.hook_manager.emit(event, context).await; - } - - async fn emit_extended_pre_hooks( - &self, - tool_name: &str, - tool_input: Option<&serde_json::Value>, - session: &Session, - ) { - let (event, matcher) = match categorize_tool(tool_name) { - ToolCategory::Shell => ( - HookEvent::BeforeShellExecution, - tool_input.and_then(|input| string_argument(input, &["command"])), - ), - ToolCategory::Read => ( - HookEvent::BeforeReadFile, - tool_input.and_then(|input| string_argument(input, &["path", "file", "file_path"])), - ), - ToolCategory::Write | ToolCategory::Other => return, - }; - if let Some(matcher) = matcher { - self.emit_with_matcher(event, session, matcher, tool_name, tool_input.cloned()) - .await; - } - } - - fn with_post_hooks( - &self, - result: ToolCallResult, - tool_call: &CallToolRequestParams, - session: &Session, - span: tracing::Span, - ) -> ToolCallResult { - let hook_manager = self.hook_manager.clone(); - let session_id = session.id.clone(); - let working_dir = session.working_dir.to_string_lossy().to_string(); - let tool_name = tool_call.name.to_string(); - let tool_input = tool_call - .arguments - .as_ref() - .map(|arguments| serde_json::Value::Object(arguments.clone())); - let category = categorize_tool(&tool_name); - let future = async move { - let result = - crate::agents::large_response_handler::process_tool_response(result.result.await); - match &result { - Ok(result) if result.is_error == Some(true) => { - tracing::Span::current().record("error.type", "tool_error"); - } - Err(_) => { - tracing::Span::current().record("error.type", "tool_execution_error"); - } - _ => {} - } - let event = match &result { - Ok(result) if result.is_error != Some(true) => HookEvent::PostToolUse, - _ => HookEvent::PostToolUseFailure, - }; - if hook_manager.has_hooks(event) { - let context = HookContext::new(event, &session_id) - .with_tool(tool_name.clone(), tool_input.clone()) - .with_working_dir(working_dir.clone()); - hook_manager.emit(event, context).await; - } - - if event == HookEvent::PostToolUse { - let extended = match category { - ToolCategory::Shell => Some(( - HookEvent::AfterShellExecution, - tool_input - .as_ref() - .and_then(|input| string_argument(input, &["command"])), - )), - ToolCategory::Write => Some(( - HookEvent::AfterFileEdit, - tool_input.as_ref().and_then(|input| { - string_argument(input, &["path", "file", "file_path"]) - }), - )), - ToolCategory::Read | ToolCategory::Other => None, - }; - if let Some((event, Some(matcher))) = extended { - if hook_manager.has_hooks(event) { - let mut context = HookContext::new(event, &session_id) - .with_tool(tool_name, tool_input) - .with_working_dir(working_dir); - context.matcher_context = Some(matcher); - hook_manager.emit(event, context).await; - } - } - } - result - } - .instrument(span); - ToolCallResult { - notification_stream: result.notification_stream, - action_required_stream: result.action_required_stream, - result: Box::new(future.boxed()), - } - } - async fn dispatch_tool_call( &self, tool_call: CallToolRequestParams, @@ -230,33 +347,27 @@ impl<'a> ToolExecutionOperation<'a> { .arguments .as_ref() .map(|arguments| serde_json::Value::Object(arguments.clone())); - if self.hook_manager.has_hooks(HookEvent::PreToolUse) { - let context = HookContext::new(HookEvent::PreToolUse, &session.id) - .with_tool(tool_call.name.to_string(), tool_input.clone()) - .with_working_dir(session.working_dir.to_string_lossy().to_string()); - if let HookDecision::Deny { reason, plugin } = self - .hook_manager - .emit_blocking(HookEvent::PreToolUse, context) - .await - { - tracing::Span::current().record("error.type", "hook_denied"); - return Err(ErrorData::new( - rmcp::model::ErrorCode::INTERNAL_ERROR, - format!( - "Tool call denied by policy hook `{plugin}`: {reason}. \ - Do not retry; this is a policy denial, not a transient failure." - ), - None, - )); - } - } - self.emit_extended_pre_hooks(&tool_call.name, tool_input.as_ref(), session) - .await; + run_pre_tool_hooks( + &self.hook_manager, + session, + request_id.as_str(), + &tool_call.name, + tool_input.as_ref(), + ) + .await?; + + emit_extended_pre_hooks( + &self.hook_manager, + &tool_call.name, + tool_input.as_ref(), + session, + ) + .await; let context = crate::agents::tool_execution::ToolCallContext::new( session.id.clone(), Some(session.working_dir.clone()), - Some(request_id), + Some(request_id.clone()), ); let result = self .extension_manager @@ -270,7 +381,14 @@ impl<'a> ToolExecutionOperation<'a> { ); ToolCallResult::from(Err(error)) }); - Ok(self.with_post_hooks(result, &tool_call, session, result_span)) + Ok(with_post_tool_hooks( + &self.hook_manager, + result, + &tool_call, + session, + result_span, + &request_id, + )) } .instrument(span) .await diff --git a/crates/goose/src/agents/state_machine/ops_unknown_tool.rs b/crates/goose/src/agents/state_machine/ops_unknown_tool.rs index 2466a9fa8..142f1f74e 100644 --- a/crates/goose/src/agents/state_machine/ops_unknown_tool.rs +++ b/crates/goose/src/agents/state_machine/ops_unknown_tool.rs @@ -2,22 +2,36 @@ use anyhow::Result; use async_trait::async_trait; -use rmcp::model::{CallToolResult, ContentBlock}; +use rmcp::model::{CallToolResult, ContentBlock, ErrorCode, ErrorData}; +use tracing_futures::Instrument; +use crate::agents::final_output_tool::FINAL_OUTPUT_TOOL_NAME; use crate::agents::state_machine::effects::GooseEffect; use crate::agents::state_machine::ops_toolcalling::{ - pending_tool_requests, tool_span, ToolDisposition, + emit_extended_pre_hooks, emit_post_tool_use, pending_tool_requests, run_pre_tool_hooks, + tool_span, ToolDisposition, }; use crate::agents::state_machine::{ applied, messages_since_kickoff, not_applicable, Emitter, Operation, OperationResult, }; +use crate::agents::tool_execution::{CHAT_MODE_TOOL_SKIPPED_RESPONSE, DECLINED_RESPONSE}; +use crate::config::GooseMode; use crate::conversation::message::Message; use crate::conversation::Conversation; +use crate::hooks::HookManager; use crate::session::Session; pub(super) const UNCLAIMED_TOOL_ERROR: &str = "goose.unclaimed_tool"; -pub struct UnknownToolOperation; +pub struct UnknownToolOperation { + hook_manager: HookManager, +} + +impl UnknownToolOperation { + pub fn new(hook_manager: HookManager) -> Self { + Self { hook_manager } + } +} #[async_trait] impl Operation for UnknownToolOperation { @@ -31,7 +45,26 @@ impl Operation for UnknownToolOperation { conversation: &Conversation, emit: &Emitter, ) -> Result> { - let pending = pending_tool_requests(messages_since_kickoff(conversation)?); + let active_final_output = session + .recipe + .as_ref() + .is_some_and(|recipe| recipe.response.is_some()); + let pending: Vec<_> = pending_tool_requests(messages_since_kickoff(conversation)?) + .into_iter() + .filter(|(request, disposition)| { + // Reserve for RecipeOperation only the final-output calls it will + // actually execute. A declined one is left here so it still gets a + // response; RecipeOperation matches on Execute alone and would + // otherwise leave the request unanswered, which strict providers can + // reject on the next request. + !(active_final_output + && *disposition == ToolDisposition::Execute + && request + .tool_call + .as_ref() + .is_ok_and(|tool_call| tool_call.name == FINAL_OUTPUT_TOOL_NAME)) + }) + .collect(); if pending.is_empty() { return not_applicable(); } @@ -44,7 +77,6 @@ impl Operation for UnknownToolOperation { .map(|tool_call| tool_call.name.as_ref()) .unwrap_or("unknown"); let span = tool_span(tool_name, &request.id, &session.id); - span.record("error.type", "tool_not_available"); let (result, unclaimed) = match disposition { ToolDisposition::ParseError(error) => ( Ok(CallToolResult::error(vec![ContentBlock::text(format!( @@ -52,26 +84,91 @@ impl Operation for UnknownToolOperation { ))])), false, ), - ToolDisposition::Execute | ToolDisposition::Decline => request - .tool_call - .as_ref() - .map(|tool_call| { - ( - Ok(CallToolResult::error(vec![ContentBlock::text(format!( - "Tool '{}' is not available.", - tool_call.name - ))])), - true, - ) - }) - .unwrap_or_else(|error| { - ( + ToolDisposition::Execute if session.goose_mode == GooseMode::Chat => ( + Ok(CallToolResult::success(vec![ContentBlock::text( + CHAT_MODE_TOOL_SKIPPED_RESPONSE, + )])), + false, + ), + ToolDisposition::Execute => { + match request.tool_call.as_ref() { + Ok(tool_call) => { + let tool_input = tool_call + .arguments + .as_ref() + .map(|arguments| serde_json::Value::Object(arguments.clone())); + match run_pre_tool_hooks( + &self.hook_manager, + session, + &request.id, + &tool_call.name, + tool_input.as_ref(), + ) + .instrument(span.clone()) + .await + { + Err(denial) => (Err(denial), false), + Ok(()) => { + emit_extended_pre_hooks( + &self.hook_manager, + &tool_call.name, + tool_input.as_ref(), + session, + ) + .instrument(span.clone()) + .await; + let (output, unclaimed) = + if tool_call.name == FINAL_OUTPUT_TOOL_NAME { + span.record("error.type", "final_output_not_defined"); + ( + Err(ErrorData::new( + ErrorCode::INTERNAL_ERROR, + "Final output tool not defined".to_string(), + None, + )), + false, + ) + } else { + span.record("error.type", "tool_not_available"); + ( + Ok(CallToolResult::error(vec![ + ContentBlock::text(format!( + "Tool '{}' is not available.", + tool_call.name + )), + ])), + true, + ) + }; + emit_post_tool_use( + &self.hook_manager, + &session.id, + &session.working_dir.to_string_lossy(), + &tool_call.name, + &request.id, + tool_input.as_ref(), + &output, + ) + .instrument(span.clone()) + .await; + (output, unclaimed) + } + } + } + Err(error) => ( Ok(CallToolResult::error(vec![ContentBlock::text(format!( "The tool call could not be parsed: {error}." ))])), false, - ) - }), + ), + } + } + ToolDisposition::Decline => ( + Ok(CallToolResult::error(vec![ContentBlock::text( + DECLINED_RESPONSE, + )])), + false, + ), }; let mut metadata = request.metadata.clone(); if unclaimed { diff --git a/crates/goose/src/agents/state_machine/tests/hooks_lifecycle.rs b/crates/goose/src/agents/state_machine/tests/hooks_lifecycle.rs index fcb944085..8ada75038 100644 --- a/crates/goose/src/agents/state_machine/tests/hooks_lifecycle.rs +++ b/crates/goose/src/agents/state_machine/tests/hooks_lifecycle.rs @@ -4,8 +4,15 @@ use serde_json::Value; use super::calculator_extension::{value, ADD}; use super::pipeline::{test_pipeline, MessageKind::Agent, MessageKind::ToolResponse, MAX_TURNS}; +use crate::agents::final_output_tool::FINAL_OUTPUT_TOOL_NAME; +use crate::agents::platform_extensions::MANAGE_EXTENSIONS_TOOL_NAME_COMPLETE; use crate::agents::state_machine::ops_stop_hook::DENIED; +use crate::agents::state_machine::ops_unknown_tool::UNCLAIMED_TOOL_ERROR; +use crate::agents::tool_execution::{CHAT_MODE_TOOL_SKIPPED_RESPONSE, DECLINED_RESPONSE}; +use crate::config::permission::PermissionLevel; +use crate::config::GooseMode; use crate::conversation::message::{Message, MessageContent, SystemNotificationType}; +use crate::permission::Permission; struct HookTestEnv { _temp_dir: tempfile::TempDir, @@ -318,3 +325,1276 @@ async fn session_prompt_and_tool_hooks_fire_at_their_boundaries() -> Result<()> Ok(()) } + +/// Plugin fixture that can register several events at once, each with its own +/// matcher and script, and read back the JSON payloads a script recorded. +struct RecordingHookEnv { + _temp_dir: tempfile::TempDir, + plugin_dir: std::path::PathBuf, +} + +/// (event name, matcher or "" for none, script file name, script body) +type HookSpec<'a> = (&'a str, &'a str, &'a str, &'a str); + +impl RecordingHookEnv { + fn new(specs: &[HookSpec<'_>]) -> Self { + let temp_dir = tempfile::tempdir().unwrap(); + let plugin_dir = temp_dir.path().join("test-plugin"); + std::fs::create_dir_all(plugin_dir.join("hooks")).unwrap(); + let entries: Vec = specs + .iter() + .map(|(event, matcher, script, _)| { + let matcher = if matcher.is_empty() { + String::new() + } else { + format!(r#""matcher": "{matcher}", "#) + }; + format!( + r#""{event}": [{{{matcher}"hooks": [{{"type": "command", "command": "sh ${{PLUGIN_ROOT}}/{script}"}}]}}]"# + ) + }) + .collect(); + std::fs::write( + plugin_dir.join("hooks/hooks.json"), + format!(r#"{{"hooks": {{{}}}}}"#, entries.join(", ")), + ) + .unwrap(); + for (_, _, script, body) in specs { + std::fs::write(plugin_dir.join(script), body).unwrap(); + } + Self { + _temp_dir: temp_dir, + plugin_dir, + } + } + + fn hook_manager(&self) -> crate::hooks::HookManager { + use crate::plugins::discovery::{DiscoveredPlugin, PluginScope}; + crate::hooks::HookManager::from_plugins_for_test(vec![DiscoveredPlugin { + name: "test-plugin".into(), + root: self.plugin_dir.clone(), + scope: PluginScope::Project, + }]) + } + + fn payloads(&self, log: &str) -> Vec { + std::fs::read_to_string(self.plugin_dir.join(log)) + .unwrap_or_default() + .lines() + .filter(|line| !line.trim().is_empty()) + .map(|line| serde_json::from_str(line).unwrap()) + .collect() + } +} + +const RECORD_PRE_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/pre.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/pre.log\"\nexit 0\n"; +const RECORD_RESULT_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/result.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/result.log\"\nexit 0\n"; +const RECORD_POST_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/post.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/post.log\"\nexit 0\n"; +const RECORD_POST_FAILURE_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/postfail.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/postfail.log\"\nexit 0\n"; +const RECORD_EXTENDED_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/extended.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/extended.log\"\nexit 0\n"; +const DENY_AND_RECORD_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/pre.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/pre.log\"\necho \"blocked by test policy\" >&2\nexit 2\n"; +/// Logs its stdin like the others, writes nothing to stdout, and exits +/// non-zero. That is a hook that ran but never returned a decision. +const ABNORMAL_EXIT_AND_RECORD_SCRIPT: &str = + "#!/bin/sh\ncat >> \"$PLUGIN_ROOT/pre.log\"\nprintf '\\n' >> \"$PLUGIN_ROOT/pre.log\"\necho boom >&2\nexit 3\n"; + +/// deny-invisible: the tool never dispatches, neither post event fires, and a +/// PreToolUseResult subscriber still sees the denial with blocked_by and reason. +#[tokio::test] +async fn pre_tool_use_result_observes_denial_that_post_hooks_never_see() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", DENY_AND_RECORD_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + api.on("add one").call(ADD, value(1)); + api.on("denied by policy hook").reply("understood"); + + pipeline.run(["add one"]).await?; + + assert_eq!(pipeline.calculator_total(), 0, "tool must not dispatch"); + assert!( + env.payloads("post.log").is_empty(), + "PostToolUse must not fire for a denied call" + ); + assert!( + env.payloads("postfail.log").is_empty(), + "PostToolUseFailure must not fire for a denied call" + ); + + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["event"], "PreToolUseResult"); + assert_eq!(results[0]["decision"], "deny"); + assert_eq!(results[0]["policy_evaluated"], true); + assert_eq!(results[0]["blocked_by"], "test-plugin"); + assert_eq!(results[0]["reason"], "blocked by test policy"); + assert!(results[0]["tool_call_id"] + .as_str() + .is_some_and(|id| !id.is_empty())); + Ok(()) +} + +/// repeated identical calls: two calls with the same name and input in one +/// session correlate to their outcomes by tool_call_id, not by name plus input. +#[tokio::test] +async fn repeated_identical_calls_correlate_by_tool_call_id() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + api.on("add one").call(ADD, value(1)); + api.on("result: 1").call(ADD, value(1)); + api.on("result: 2").reply("done"); + + pipeline.run(["add one"]).await?; + assert_eq!(pipeline.calculator_total(), 2); + + let pres = env.payloads("pre.log"); + let results = env.payloads("result.log"); + let posts = env.payloads("post.log"); + assert_eq!(pres.len(), 2); + assert_eq!(results.len(), 2); + assert_eq!(posts.len(), 2); + + for payloads in [&pres, &results, &posts] { + assert_eq!(payloads[0]["tool_name"], payloads[1]["tool_name"]); + assert_eq!(payloads[0]["tool_input"], payloads[1]["tool_input"]); + } + + let ids: Vec<&str> = results + .iter() + .map(|payload| payload["tool_call_id"].as_str().unwrap()) + .collect(); + assert_ne!( + ids[0], ids[1], + "identical name and input must still carry distinct ids" + ); + + for (index, id) in ids.iter().enumerate() { + assert_eq!( + pres[index]["tool_call_id"], results[index]["tool_call_id"], + "PreToolUse and PreToolUseResult must carry one id per call" + ); + assert_eq!( + posts + .iter() + .filter(|payload| payload["tool_call_id"] == *id) + .count(), + 1, + "each call must pair with exactly one outcome by id" + ); + } + Ok(()) +} + +/// no matching hook: a PreToolUse rule is registered but its matcher does not +/// match, so nothing runs and the event reports allow with policy_evaluated false. +#[tokio::test] +async fn pre_tool_use_result_reports_allow_and_unevaluated_when_no_hook_matches() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ( + "PreToolUse", + "a_tool_name_that_never_matches", + "pre.sh", + DENY_AND_RECORD_SCRIPT, + ), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + api.on("add one").call(ADD, value(1)); + api.on("result: 1").reply("done"); + + pipeline.run(["add one"]).await?; + assert_eq!(pipeline.calculator_total(), 1, "tool must still run"); + + assert!( + env.payloads("pre.log").is_empty(), + "the non-matching rule must not run" + ); + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["decision"], "allow"); + assert_eq!(results[0]["policy_evaluated"], false); + assert!(results[0].get("blocked_by").is_none()); + assert!(results[0].get("reason").is_none()); + Ok(()) +} + +/// sole abnormal hook: the only matching PreToolUse hook runs, writes nothing to +/// stdout and exits non-zero, so it never returned a decision. Execution stays +/// fail-open and the event reports allow with policy_evaluated false. +#[tokio::test] +async fn pre_tool_use_result_reports_unevaluated_when_the_only_hook_exits_without_a_decision( +) -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", ABNORMAL_EXIT_AND_RECORD_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + api.on("add one").call(ADD, value(1)); + api.on("result: 1").reply("done"); + + pipeline.run(["add one"]).await?; + assert_eq!(pipeline.calculator_total(), 1, "tool must still run"); + + assert_eq!( + env.payloads("pre.log").len(), + 1, + "the matching hook must still run" + ); + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["decision"], "allow"); + assert_eq!(results[0]["policy_evaluated"], false); + // The pipeline mints the id rather than the caller, so pin that one is + // present and non-empty instead of pinning a literal. + assert!( + results[0]["tool_call_id"] + .as_str() + .is_some_and(|id| !id.is_empty()), + "the result event must carry the tool_call_id" + ); + Ok(()) +} + +/// Builds a recipe whose structured response forces the model through +/// `recipe__final_output`, which `RecipeOperation` executes itself rather than +/// handing to `ToolExecutionOperation`. +fn final_output_recipe() -> crate::recipe::Recipe { + crate::recipe::Recipe::builder() + .title("Hook parity recipe") + .description("Exercises the final-output hook lifecycle") + .instructions("Return a structured answer") + .response(crate::recipe::Response { + json_schema: Some(serde_json::json!({ + "type": "object", + "properties": { "answer": { "type": "string" } }, + "required": ["answer"] + })), + }) + .build() + .expect("valid recipe") +} + +/// recipe final-output parity: the call `RecipeOperation` executes directly still +/// emits `PreToolUse` and `PreToolUseResult`, correlated by one `tool_call_id`. +#[tokio::test] +async fn recipe_final_output_emits_pre_tool_use_and_result_with_matching_id() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + pipeline.set_recipe(final_output_recipe()).await?; + api.on("produce the answer").call( + FINAL_OUTPUT_TOOL_NAME, + serde_json::json!({ "answer": "done" }), + ); + + pipeline.run(["produce the answer"]).await?; + + let pres = env.payloads("pre.log"); + let results = env.payloads("result.log"); + assert_eq!( + pres.len(), + 1, + "PreToolUse must fire for recipe final output" + ); + assert_eq!( + results.len(), + 1, + "PreToolUseResult must fire for recipe final output" + ); + assert_eq!(pres[0]["tool_name"], FINAL_OUTPUT_TOOL_NAME); + assert_eq!(results[0]["tool_name"], FINAL_OUTPUT_TOOL_NAME); + assert_eq!(results[0]["event"], "PreToolUseResult"); + assert_eq!(results[0]["decision"], "allow"); + assert_eq!( + pres[0]["tool_call_id"], results[0]["tool_call_id"], + "PreToolUse and PreToolUseResult must carry the same tool_call_id" + ); + assert!(pres[0]["tool_call_id"] + .as_str() + .is_some_and(|id| !id.is_empty())); + Ok(()) +} + +/// recipe final-output parity: the post-tool event fires once the call completes, +/// carrying the id the pre events carried. +#[tokio::test] +async fn recipe_final_output_emits_post_tool_event() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + pipeline.set_recipe(final_output_recipe()).await?; + api.on("produce the answer").call( + FINAL_OUTPUT_TOOL_NAME, + serde_json::json!({ "answer": "done" }), + ); + + pipeline.run(["produce the answer"]).await?; + + let pres = env.payloads("pre.log"); + let posts = env.payloads("post.log"); + assert_eq!(pres.len(), 1); + assert_eq!( + posts.len(), + 1, + "PostToolUse must fire for a successful recipe final output" + ); + assert_eq!(posts[0]["event"], "PostToolUse"); + assert_eq!(posts[0]["tool_name"], FINAL_OUTPUT_TOOL_NAME); + assert_eq!( + posts[0]["tool_call_id"], pres[0]["tool_call_id"], + "the post event must carry the same tool_call_id as the pre events" + ); + Ok(()) +} + +#[tokio::test] +async fn recipe_final_output_waits_for_sibling_tools_and_is_emitted_once() -> Result<()> { + let (pipeline, api) = test_pipeline().await?; + pipeline.set_recipe(final_output_recipe()).await?; + api.on("finish and add").calls([ + ( + "final-output", + FINAL_OUTPUT_TOOL_NAME, + serde_json::json!({ "answer": "done" }), + ), + ("side-effect", ADD, value(1)), + ]); + + let result = pipeline.run(["finish and add"]).await?; + + assert_eq!(pipeline.calculator_total(), 1); + let messages = result.conversation().messages(); + let side_effect_response = messages + .iter() + .position(|message| { + message.content.iter().any(|content| { + matches!( + content, + MessageContent::ToolResponse(response) if response.id == "side-effect" + ) + }) + }) + .expect("sibling tool response"); + let final_answers: Vec<_> = messages + .iter() + .enumerate() + .filter(|(_, message)| message.as_concat_text() == r#"{"answer":"done"}"#) + .collect(); + assert_eq!(final_answers.len(), 1, "final output must be emitted once"); + assert!( + side_effect_response < final_answers[0].0, + "final output must wait until sibling tools finish" + ); + Ok(()) +} + +#[tokio::test] +async fn recipe_final_output_waits_for_approval_pending_sibling() -> Result<()> { + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_goose_mode(GooseMode::Approve).await; + pipeline.set_permission(FINAL_OUTPUT_TOOL_NAME, PermissionLevel::AlwaysAllow); + pipeline.set_recipe(final_output_recipe()).await?; + api.on("finish after approval").calls([ + ( + "final-output", + FINAL_OUTPUT_TOOL_NAME, + serde_json::json!({ "answer": "approved" }), + ), + ( + "side-effect", + MANAGE_EXTENSIONS_TOOL_NAME_COMPLETE, + serde_json::json!({ + "action": "enable", + "extension_name": "analyze" + }), + ), + ]); + + let awaiting_approval = pipeline.run(["finish after approval"]).await?; + assert!( + awaiting_approval + .conversation() + .messages() + .iter() + .all(|message| message.as_concat_text() != r#"{"answer":"approved"}"#), + "final output must wait while a sibling needs approval" + ); + + pipeline + .confirm("side-effect", Permission::AllowOnce) + .await?; + let result = pipeline.resume().await?; + + let messages = result.conversation().messages(); + let sibling_response = messages + .iter() + .position(|message| { + message.content.iter().any(|content| { + matches!( + content, + MessageContent::ToolResponse(response) if response.id == "side-effect" + ) + }) + }) + .expect("approved sibling response"); + let final_answers = result + .conversation() + .messages() + .iter() + .filter(|message| message.as_concat_text() == r#"{"answer":"approved"}"#) + .count(); + assert_eq!(final_answers, 1, "approved final output must emit once"); + let final_answer = messages + .iter() + .position(|message| message.as_concat_text() == r#"{"answer":"approved"}"#) + .expect("approved final output"); + assert!( + sibling_response < final_answer, + "the approved sibling must execute before finalization" + ); + result.assert_message(-1, Agent, r#"{"answer":"approved"}"#); + Ok(()) +} + +/// recipe final-output parity: a denying hook stops the call. The final-output +/// tool never runs, so the recipe never reports a successful structured answer, +/// and no post event fires — the same shape a denied ordinary tool call has. +#[tokio::test] +async fn recipe_final_output_denied_by_hook_does_not_execute() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", DENY_AND_RECORD_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline + .with_hook_manager(env.hook_manager()) + .with_max_turns(2); + pipeline.set_recipe(final_output_recipe()).await?; + api.on("produce the answer").call( + FINAL_OUTPUT_TOOL_NAME, + serde_json::json!({ "answer": "done" }), + ); + api.on("denied by policy hook").reply("understood"); + + let result = pipeline.run(["produce the answer"]).await?; + + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["decision"], "deny"); + assert_eq!(results[0]["blocked_by"], "test-plugin"); + assert_eq!(results[0]["tool_name"], FINAL_OUTPUT_TOOL_NAME); + + assert!( + env.payloads("post.log").is_empty(), + "PostToolUse must not fire for a denied final-output call" + ); + assert!( + env.payloads("postfail.log").is_empty(), + "PostToolUseFailure must not fire for a denied final-output call" + ); + + let produced_answer = result + .conversation() + .messages() + .iter() + .any(|message| message.as_concat_text().contains("\"answer\"")); + assert!( + !produced_answer, + "a denied final-output call must not execute the tool" + ); + Ok(()) +} + +/// Writes a skill `SkillOperation` can load, and returns the tool arguments that +/// load it. `load_skill` is executed by `SkillOperation`, which is registered +/// ahead of `ToolExecutionOperation`, so it never reaches the hook wrapper. +fn install_skill(working_dir: &std::path::Path) -> serde_json::Value { + let skill_dir = working_dir.join(".agents/skills/review"); + std::fs::create_dir_all(&skill_dir).expect("skill dir"); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: review\ndescription: Review helper\n---\nSKILL_BODY_CONTENT\n", + ) + .expect("skill file"); + serde_json::json!({ "name": "review" }) +} + +/// load_skill parity: the call `SkillOperation` executes directly still emits +/// `PreToolUse` and `PreToolUseResult`, correlated by one `tool_call_id`. +#[tokio::test] +async fn load_skill_emits_pre_tool_use_and_result_with_matching_id() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + let arguments = install_skill(pipeline.working_dir()); + api.on("use the skill").call("load_skill", arguments); + api.on("SKILL_BODY_CONTENT").reply("skill loaded"); + + pipeline.run(["use the skill"]).await?; + + let pres = env.payloads("pre.log"); + let results = env.payloads("result.log"); + assert_eq!(pres.len(), 1, "PreToolUse must fire for load_skill"); + assert_eq!( + results.len(), + 1, + "PreToolUseResult must fire for load_skill" + ); + assert_eq!(pres[0]["tool_name"], "load_skill"); + assert_eq!(results[0]["tool_name"], "load_skill"); + assert_eq!(results[0]["event"], "PreToolUseResult"); + assert_eq!(results[0]["decision"], "allow"); + assert_eq!( + pres[0]["tool_call_id"], results[0]["tool_call_id"], + "PreToolUse and PreToolUseResult must carry the same tool_call_id" + ); + assert!(pres[0]["tool_call_id"] + .as_str() + .is_some_and(|id| !id.is_empty())); + Ok(()) +} + +/// load_skill parity: the post-tool event fires once the skill load completes, +/// carrying the id the pre events carried. +#[tokio::test] +async fn load_skill_emits_post_tool_event() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + let arguments = install_skill(pipeline.working_dir()); + api.on("use the skill").call("load_skill", arguments); + api.on("SKILL_BODY_CONTENT").reply("skill loaded"); + + pipeline.run(["use the skill"]).await?; + + let pres = env.payloads("pre.log"); + let posts = env.payloads("post.log"); + assert_eq!(pres.len(), 1); + assert_eq!( + posts.len(), + 1, + "PostToolUse must fire for a successful load_skill" + ); + assert_eq!(posts[0]["event"], "PostToolUse"); + assert_eq!(posts[0]["tool_name"], "load_skill"); + assert_eq!( + posts[0]["tool_call_id"], pres[0]["tool_call_id"], + "the post event must carry the same tool_call_id as the pre events" + ); + Ok(()) +} + +/// load_skill parity: a denying hook stops the call. The skill body never +/// reaches the conversation and no post event fires — the same shape a denied +/// ordinary tool call has. +#[tokio::test] +async fn load_skill_denied_by_hook_does_not_execute() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", DENY_AND_RECORD_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + let arguments = install_skill(pipeline.working_dir()); + api.on("use the skill").call("load_skill", arguments); + api.on("denied by policy hook").reply("understood"); + + let result = pipeline.run(["use the skill"]).await?; + + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["decision"], "deny"); + assert_eq!(results[0]["blocked_by"], "test-plugin"); + assert_eq!(results[0]["tool_name"], "load_skill"); + + assert!( + env.payloads("post.log").is_empty(), + "PostToolUse must not fire for a denied load_skill call" + ); + assert!( + env.payloads("postfail.log").is_empty(), + "PostToolUseFailure must not fire for a denied load_skill call" + ); + + let loaded_body = result + .conversation() + .messages() + .iter() + .any(|message| message.as_concat_text().contains("SKILL_BODY_CONTENT")); + assert!( + !loaded_body, + "a denied load_skill call must not execute the skill load" + ); + Ok(()) +} + +/// Unknown-tool parity: a valid unadvertised call still emits `PreToolUse` and +/// `PreToolUseResult`, correlated by one `tool_call_id`. +#[tokio::test] +async fn unknown_tool_emits_pre_tool_use_and_result_with_matching_id() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + api.on("try the missing tool") + .unadvertised_call("missing__tool", serde_json::json!({})); + api.on("not available").reply("recovered"); + + pipeline.run(["try the missing tool"]).await?; + + let pres = env.payloads("pre.log"); + let results = env.payloads("result.log"); + assert_eq!(pres.len(), 1, "PreToolUse must fire for an unknown tool"); + assert_eq!( + results.len(), + 1, + "PreToolUseResult must fire for an unknown tool" + ); + assert_eq!(pres[0]["tool_name"], "missing__tool"); + assert_eq!(results[0]["tool_name"], "missing__tool"); + assert_eq!(results[0]["event"], "PreToolUseResult"); + assert_eq!(results[0]["decision"], "allow"); + assert_eq!( + pres[0]["tool_call_id"], results[0]["tool_call_id"], + "PreToolUse and PreToolUseResult must carry the same tool_call_id" + ); + assert!(pres[0]["tool_call_id"] + .as_str() + .is_some_and(|id| !id.is_empty())); + Ok(()) +} + +/// Unknown-tool parity: the unavailable result is a failed tool outcome and +/// carries the same id as the pre event. +#[tokio::test] +async fn unknown_tool_emits_post_tool_failure_event() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + api.on("try the missing tool") + .unadvertised_call("missing__tool", serde_json::json!({})); + api.on("not available").reply("recovered"); + + pipeline.run(["try the missing tool"]).await?; + + let pres = env.payloads("pre.log"); + let post_failures = env.payloads("postfail.log"); + assert_eq!(pres.len(), 1); + assert!( + env.payloads("post.log").is_empty(), + "PostToolUse must not fire for an unavailable tool" + ); + assert_eq!( + post_failures.len(), + 1, + "PostToolUseFailure must fire for an unavailable tool" + ); + assert_eq!(post_failures[0]["event"], "PostToolUseFailure"); + assert_eq!(post_failures[0]["tool_name"], "missing__tool"); + assert_eq!( + post_failures[0]["tool_call_id"], pres[0]["tool_call_id"], + "the post event must carry the same tool_call_id as the pre events" + ); + Ok(()) +} + +#[tokio::test] +async fn inactive_final_output_emits_failure_without_unclaimed_metadata() -> Result<()> { + let env = RecordingHookEnv::new(&[( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + )]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + api.on("call inactive final output").unadvertised_call( + FINAL_OUTPUT_TOOL_NAME, + serde_json::json!({ "answer": "unused" }), + ); + api.on("Final output tool not defined") + .reply("inactive call handled"); + + let result = pipeline.run(["call inactive final output"]).await?; + + result.assert_message(-2, ToolResponse, "Final output tool not defined"); + let failures = env.payloads("postfail.log"); + assert_eq!(failures.len(), 1); + assert_eq!(failures[0]["tool_name"], FINAL_OUTPUT_TOOL_NAME); + let response_metadata = result + .conversation() + .messages() + .iter() + .flat_map(|message| &message.content) + .find_map(|content| match content { + MessageContent::ToolResponse(response) => response.metadata.as_ref(), + _ => None, + }); + assert!(response_metadata.is_none_or(|metadata| !metadata.contains_key(UNCLAIMED_TOOL_ERROR))); + Ok(()) +} + +#[tokio::test] +async fn unknown_shell_and_read_tools_emit_extended_pre_hooks() -> Result<()> { + let shell = RecordingHookEnv::new(&[( + "BeforeShellExecution", + "echo lifecycle", + "extended.sh", + RECORD_EXTENDED_SCRIPT, + )]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(shell.hook_manager()); + api.on("probe unknown shell").unadvertised_call( + "missing__shell", + serde_json::json!({ "command": "echo lifecycle" }), + ); + api.on("not available").reply("shell probe complete"); + + pipeline.run(["probe unknown shell"]).await?; + + let shell_events = shell.payloads("extended.log"); + assert_eq!(shell_events.len(), 1); + assert_eq!(shell_events[0]["event"], "BeforeShellExecution"); + assert_eq!(shell_events[0]["tool_name"], "missing__shell"); + assert_eq!(shell_events[0]["tool_input"]["command"], "echo lifecycle"); + + let read = RecordingHookEnv::new(&[( + "BeforeReadFile", + "/tmp/missing-lifecycle-file", + "extended.sh", + RECORD_EXTENDED_SCRIPT, + )]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(read.hook_manager()); + api.on("probe unknown read").unadvertised_call( + "missing__read", + serde_json::json!({ "path": "/tmp/missing-lifecycle-file" }), + ); + api.on("not available").reply("read probe complete"); + + pipeline.run(["probe unknown read"]).await?; + + let read_events = read.payloads("extended.log"); + assert_eq!(read_events.len(), 1); + assert_eq!(read_events[0]["event"], "BeforeReadFile"); + assert_eq!(read_events[0]["tool_name"], "missing__read"); + assert_eq!( + read_events[0]["tool_input"]["path"], + "/tmp/missing-lifecycle-file" + ); + Ok(()) +} + +/// Unknown-tool parity: a denying hook returns before the unknown-tool handler +/// creates its unavailable result, and no post event fires. +#[tokio::test] +async fn unknown_tool_denied_by_hook_does_not_resolve_as_unavailable() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", DENY_AND_RECORD_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + api.on("try the missing tool") + .unadvertised_call("missing__tool", serde_json::json!({})); + api.on("denied by policy hook").reply("understood"); + + let result = pipeline.run(["try the missing tool"]).await?; + + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["decision"], "deny"); + assert_eq!(results[0]["blocked_by"], "test-plugin"); + assert_eq!(results[0]["tool_name"], "missing__tool"); + assert!( + env.payloads("post.log").is_empty(), + "PostToolUse must not fire for a denied unknown tool" + ); + assert!( + env.payloads("postfail.log").is_empty(), + "PostToolUseFailure must not fire for a denied unknown tool" + ); + let tool_error = result + .conversation() + .messages() + .iter() + .flat_map(|message| &message.content) + .find_map(|content| match content { + MessageContent::ToolResponse(response) => response.tool_result.as_ref().err(), + _ => None, + }) + .expect("denied unknown tool response"); + assert!(tool_error.message.contains("denied by policy hook")); + assert!(!tool_error.message.contains("is not available")); + Ok(()) +} + +#[tokio::test] +async fn chat_mode_does_not_collect_skipped_recipe_final_output_or_run_hooks() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline + .with_hook_manager(env.hook_manager()) + .with_goose_mode(GooseMode::Chat) + .await + .with_max_turns(2); + pipeline.set_recipe(final_output_recipe()).await?; + api.on("produce the answer").call( + FINAL_OUTPUT_TOOL_NAME, + serde_json::json!({ "answer": "done" }), + ); + api.on(CHAT_MODE_TOOL_SKIPPED_RESPONSE) + .reply("continued without the final output tool"); + + let result = pipeline.run(["produce the answer"]).await?; + + let messages = result.conversation().messages(); + let emitted_chat_skip = messages + .iter() + .flat_map(|message| &message.content) + .any(|content| match content { + MessageContent::ToolResponse(response) => { + response.tool_result.as_ref().is_ok_and(|result| { + result.content.iter().any(|content| { + content + .as_text() + .is_some_and(|text| text.text == CHAT_MODE_TOOL_SKIPPED_RESPONSE) + }) + }) + } + _ => false, + }); + assert!(emitted_chat_skip, "Chat mode must emit its skip response"); + assert!( + messages + .iter() + .all(|message| message.as_concat_text() != r#"{"answer":"done"}"#), + "a skipped final-output call must not be collected" + ); + assert!(env.payloads("pre.log").is_empty()); + assert!(env.payloads("result.log").is_empty()); + assert!(env.payloads("post.log").is_empty()); + assert!(env.payloads("postfail.log").is_empty()); + Ok(()) +} + +#[tokio::test] +async fn chat_mode_skips_unknown_tool_without_tool_hooks() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline + .with_hook_manager(env.hook_manager()) + .with_goose_mode(GooseMode::Chat) + .await; + api.on("try the missing tool") + .unadvertised_call("missing__tool", serde_json::json!({})); + api.on(CHAT_MODE_TOOL_SKIPPED_RESPONSE) + .reply("continued without the missing tool"); + + let result = pipeline.run(["try the missing tool"]).await?; + + result.assert_message(-2, ToolResponse, CHAT_MODE_TOOL_SKIPPED_RESPONSE); + result.assert_message(-1, Agent, "continued without the missing tool"); + assert!(env.payloads("pre.log").is_empty()); + assert!(env.payloads("result.log").is_empty()); + assert!(env.payloads("post.log").is_empty()); + assert!(env.payloads("postfail.log").is_empty()); + Ok(()) +} + +#[tokio::test] +async fn denied_unknown_tool_reports_policy_decline_without_tool_hooks() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline + .with_hook_manager(env.hook_manager()) + .with_goose_mode(GooseMode::Approve) + .await; + pipeline.set_permission("missing__tool", PermissionLevel::NeverAllow); + api.on("try the missing tool") + .unadvertised_call("missing__tool", serde_json::json!({})); + api.on(DECLINED_RESPONSE).reply("understood"); + + let result = pipeline.run(["try the missing tool"]).await?; + + result.assert_message(-2, ToolResponse, DECLINED_RESPONSE); + assert!(env.payloads("pre.log").is_empty()); + assert!(env.payloads("result.log").is_empty()); + assert!(env.payloads("post.log").is_empty()); + assert!(env.payloads("postfail.log").is_empty()); + Ok(()) +} + +/// Enforces a bijection between tool requests and tool responses over the whole +/// transcript. Presence alone is not enough: a duplicate response reuses a +/// tool_call_id and an orphan response names one that was never requested, and +/// strict providers can reject either on the next request. +fn assert_tool_transcript_bijection(messages: &[Message]) { + let mut request_ids: Vec = Vec::new(); + let mut response_ids: Vec = Vec::new(); + for message in messages { + for content in &message.content { + match content { + MessageContent::ToolRequest(request) => request_ids.push(request.id.clone()), + MessageContent::ToolResponse(response) => response_ids.push(response.id.clone()), + _ => {} + } + } + } + let unique_requests: std::collections::HashSet<&String> = request_ids.iter().collect(); + assert_eq!( + unique_requests.len(), + request_ids.len(), + "a tool request id appears more than once: {request_ids:?}" + ); + for id in &request_ids { + let answers = response_ids.iter().filter(|other| *other == id).count(); + assert_eq!( + answers, 1, + "request {id} has {answers} responses, expected exactly one; responses {response_ids:?}" + ); + } + for id in &response_ids { + assert!( + unique_requests.contains(id), + "response {id} references no request; requests {request_ids:?}" + ); + } +} + +fn lifecycle_ids(env: &RecordingHookEnv, log: &str) -> Vec { + env.payloads(log) + .iter() + .filter_map(|payload| payload["tool_call_id"].as_str().map(str::to_string)) + .collect() +} + +fn recording_lifecycle_env() -> RecordingHookEnv { + RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", RECORD_PRE_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ( + "PostToolUseFailure", + "", + "postfail.sh", + RECORD_POST_FAILURE_SCRIPT, + ), + ]) +} + +/// A final-output call refused by permission is answered like any other declined +/// tool. RecipeOperation matches on Execute alone, so before the fix nothing +/// answered this request at all. +#[tokio::test] +async fn recipe_final_output_denied_by_permission_receives_declined_response() -> Result<()> { + let env = recording_lifecycle_env(); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline + .with_hook_manager(env.hook_manager()) + .with_goose_mode(GooseMode::Approve) + .await; + pipeline.set_permission(FINAL_OUTPUT_TOOL_NAME, PermissionLevel::NeverAllow); + pipeline.set_recipe(final_output_recipe()).await?; + api.on("finish now").call( + FINAL_OUTPUT_TOOL_NAME, + serde_json::json!({ "answer": "denied" }), + ); + api.on("declined to run this tool").reply("understood"); + + let result = pipeline.run(["finish now"]).await?; + let messages = result.conversation().messages(); + assert_tool_transcript_bijection(messages); + + let declined = messages + .iter() + .flat_map(|message| &message.content) + .filter(|content| match content { + MessageContent::ToolResponse(response) => { + response.tool_result.as_ref().is_ok_and(|result| { + result.content.iter().any(|block| { + block + .as_text() + .is_some_and(|text| text.text == DECLINED_RESPONSE) + }) + }) + } + _ => false, + }) + .count(); + assert_eq!( + declined, 1, + "the declined call must get one DECLINED_RESPONSE" + ); + + assert!( + env.payloads("pre.log").is_empty(), + "no PreToolUse on decline" + ); + assert!( + env.payloads("result.log").is_empty(), + "no PreToolUseResult on decline" + ); + assert!( + env.payloads("post.log").is_empty(), + "no PostToolUse on decline" + ); + assert!( + env.payloads("postfail.log").is_empty(), + "no PostToolUseFailure on decline" + ); + Ok(()) +} + +/// Two final-output calls in one assistant block both get answered, each with its +/// own lifecycle, and the last valid one is published. Before the fix the pair +/// deadlocked: each waited for the other and neither was answered. +#[tokio::test] +async fn duplicate_final_output_calls_are_each_answered_and_last_valid_wins() -> Result<()> { + let env = recording_lifecycle_env(); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline.with_hook_manager(env.hook_manager()); + pipeline.set_recipe(final_output_recipe()).await?; + api.on("finish twice").calls([ + ( + "final-a", + FINAL_OUTPUT_TOOL_NAME, + serde_json::json!({ "answer": "first" }), + ), + ( + "final-b", + FINAL_OUTPUT_TOOL_NAME, + serde_json::json!({ "answer": "second" }), + ), + ]); + + let result = pipeline.run(["finish twice"]).await?; + let messages = result.conversation().messages(); + assert_tool_transcript_bijection(messages); + + // Assert the recorded order, not just membership: execution order is what + // decides which call wins, so sorting here would erase the property. + let pre = lifecycle_ids(&env, "pre.log"); + let results = lifecycle_ids(&env, "result.log"); + let posts = lifecycle_ids(&env, "post.log"); + assert!( + env.payloads("postfail.log").is_empty(), + "both calls succeed, so no post-failure event may be recorded" + ); + let expected = vec!["final-a".to_string(), "final-b".to_string()]; + assert_eq!(pre, expected, "both calls need a PreToolUse"); + assert_eq!(results, expected, "both calls need a PreToolUseResult"); + assert_eq!(posts, expected, "both calls need a post event"); + + let published: Vec<_> = messages + .iter() + .filter(|message| message.as_concat_text() == r#"{"answer":"second"}"#) + .collect(); + assert_eq!( + published.len(), + 1, + "the last valid final output is published exactly once" + ); + assert!( + messages + .iter() + .all(|message| message.as_concat_text() != r#"{"answer":"first"}"#), + "the superseded final output must not be published" + ); + Ok(()) +} + +/// A malformed final-output call gets one parse-error response and runs no +/// lifecycle, because nothing executes. +#[tokio::test] +async fn malformed_final_output_call_receives_parse_error_and_no_lifecycle() -> Result<()> { + let env = recording_lifecycle_env(); + let (pipeline, api) = test_pipeline().await?; + let pipeline = pipeline + .with_hook_manager(env.hook_manager()) + .with_max_turns(2); + pipeline.set_recipe(final_output_recipe()).await?; + api.on("finish badly") + .malformed_tool_call(FINAL_OUTPUT_TOOL_NAME, r#"{"answer":"#); + api.on("could not be parsed").reply("understood"); + + let result = pipeline.run(["finish badly"]).await?; + let messages = result.conversation().messages(); + assert_tool_transcript_bijection(messages); + + // The parse error rides on the tool response, not on message text, so + // as_concat_text would not see it. + let parse_errors = messages + .iter() + .flat_map(|message| &message.content) + .filter(|content| match content { + MessageContent::ToolResponse(response) => { + response.tool_result.as_ref().is_ok_and(|result| { + result.content.iter().any(|block| { + block + .as_text() + .is_some_and(|text| text.text.contains("could not be parsed")) + }) + }) + } + _ => false, + }) + .count(); + assert_eq!(parse_errors, 1, "one parse-error response"); + + assert!(env.payloads("pre.log").is_empty()); + assert!(env.payloads("result.log").is_empty()); + assert!(env.payloads("post.log").is_empty()); + assert!(env.payloads("postfail.log").is_empty()); + Ok(()) +} + +/// An unfinished ordinary sibling still delays publication, and every request in +/// the block is answered exactly once. +#[tokio::test] +async fn final_output_waits_for_ordinary_sibling_and_answers_every_request() -> Result<()> { + let (pipeline, api) = test_pipeline().await?; + pipeline.set_recipe(final_output_recipe()).await?; + api.on("finish and add").calls([ + ( + "final-output", + FINAL_OUTPUT_TOOL_NAME, + serde_json::json!({ "answer": "after sibling" }), + ), + ("side-effect", ADD, value(1)), + ]); + + let result = pipeline.run(["finish and add"]).await?; + let messages = result.conversation().messages(); + assert_tool_transcript_bijection(messages); + assert_eq!(pipeline.calculator_total(), 1); + + let sibling_answer = messages + .iter() + .position(|message| { + message.content.iter().any(|content| { + matches!( + content, + MessageContent::ToolResponse(response) if response.id == "side-effect" + ) + }) + }) + .expect("sibling tool response"); + let published = messages + .iter() + .position(|message| message.as_concat_text() == r#"{"answer":"after sibling"}"#) + .expect("published final output"); + assert!( + sibling_answer < published, + "final output must wait until the ordinary sibling finishes" + ); + Ok(()) +} diff --git a/crates/goose/src/agents/state_machine/tests/pipeline.rs b/crates/goose/src/agents/state_machine/tests/pipeline.rs index e92813301..8c0e80083 100644 --- a/crates/goose/src/agents/state_machine/tests/pipeline.rs +++ b/crates/goose/src/agents/state_machine/tests/pipeline.rs @@ -143,14 +143,14 @@ impl TestPipeline { )), Arc::new(DoctorOperation), Arc::new(ProjectOperation), - Arc::new(SkillOperation), - Arc::new(RecipeOperation), + Arc::new(SkillOperation::new(self.hook_manager.clone())), + Arc::new(RecipeOperation::new(self.hook_manager.clone())), Arc::new(ToolExecutionOperation::new( &self.goose_mode, self.extension_manager.clone(), self.hook_manager.clone(), )), - Arc::new(UnknownToolOperation), + Arc::new(UnknownToolOperation::new(self.hook_manager.clone())), Arc::new(RetryOperation::new( &self.goal, &self.grind, diff --git a/crates/goose/src/agents/state_machine/tests/reconstruction_isolation_lifecycle.rs b/crates/goose/src/agents/state_machine/tests/reconstruction_isolation_lifecycle.rs index e762a5bad..1b1a5287c 100644 --- a/crates/goose/src/agents/state_machine/tests/reconstruction_isolation_lifecycle.rs +++ b/crates/goose/src/agents/state_machine/tests/reconstruction_isolation_lifecycle.rs @@ -10,6 +10,7 @@ use crate::agents::final_output_tool::FINAL_OUTPUT_TOOL_NAME; use crate::agents::platform_extensions::MANAGE_EXTENSIONS_TOOL_NAME_COMPLETE; use crate::agents::tool_execution::CHAT_MODE_TOOL_SKIPPED_RESPONSE; use crate::config::GooseMode; +use crate::conversation::message::MessageContent; use crate::recipe::{Recipe, Response}; use goose_providers::model::ModelConfig; @@ -81,7 +82,7 @@ async fn reconstruction_and_session_isolation() -> Result<()> { .with_goose_mode(GooseMode::Chat) .await; - let pipeline = pipeline.reconstruct().await?; + let pipeline = pipeline.reconstruct().await?.with_max_turns(2); let restored = pipeline.session().await?; assert_eq!(restored.provider_name.as_deref(), Some("openai")); assert_eq!( @@ -102,14 +103,50 @@ async fn reconstruction_and_session_isolation() -> Result<()> { FINAL_OUTPUT_TOOL_NAME, json!({ "answer": "state restored" }), ); + api.on(CHAT_MODE_TOOL_SKIPPED_RESPONSE) + .reply("structured output stayed disabled in chat mode"); let restored_call_index = api.call_count(); let restored_result = pipeline.run(["check restored state"]).await?; - restored_result.assert_message(-1, Agent, r#"{"answer":"state restored"}"#); + let restored_messages = restored_result.conversation().messages(); + assert!( + restored_messages + .iter() + .flat_map(|message| &message.content) + .any(|content| match content { + MessageContent::ToolResponse(response) => + response + .tool_result + .as_ref() + .is_ok_and(|result| result.content.iter().any(|content| { + content + .as_text() + .is_some_and(|text| text.text == CHAT_MODE_TOOL_SKIPPED_RESPONSE) + })), + _ => false, + }), + "a restored Chat-mode recipe must skip its final-output tool" + ); + assert!( + restored_messages + .iter() + .all(|message| message.as_concat_text() != r#"{"answer":"state restored"}"#), + "a skipped final-output call must not be collected after reconstruction" + ); let restored_call = api.calls()[restored_call_index].clone(); assert!(restored_call.uses_model("gpt-4o")); assert!(restored_call.advertises_tool("analyze")); assert!(restored_call.advertises_tool(FINAL_OUTPUT_TOOL_NAME)); + let pipeline = pipeline.with_goose_mode(GooseMode::Auto).await; + let pipeline = pipeline.reconstruct().await?; + api.on("collect restored structured state").call( + FINAL_OUTPUT_TOOL_NAME, + json!({ "answer": "state restored" }), + ); + let restored_auto = pipeline.run(["collect restored structured state"]).await?; + restored_auto.assert_message(-1, Agent, r#"{"answer":"state restored"}"#); + + let pipeline = pipeline.with_goose_mode(GooseMode::Chat).await; pipeline .session_manager .update(&pipeline.session_id) @@ -118,7 +155,8 @@ async fn reconstruction_and_session_isolation() -> Result<()> { .await?; let pipeline = pipeline.reconstruct().await?; api.on("try the restored calculator").call(ADD, value(1)); - api.on(CHAT_MODE_TOOL_SKIPPED_RESPONSE) + let next_tool_call_id = format!("dummy-tool-call-{}", api.call_count() + 1); + api.on(next_tool_call_id) .reply("chat mode kept the tool idle"); let chat = pipeline.run(["try the restored calculator"]).await?; chat.assert_message(-2, ToolResponse, CHAT_MODE_TOOL_SKIPPED_RESPONSE); diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index 1d207bd72..7bdce3933 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -50,6 +50,7 @@ const DEFAULT_HOOK_TIMEOUT_SECS: u64 = 30; #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum HookEvent { PreToolUse, + PreToolUseResult, PostToolUse, PostToolUseFailure, SessionStart, @@ -66,6 +67,7 @@ impl HookEvent { fn name(&self) -> &'static str { match self { HookEvent::PreToolUse => "PreToolUse", + HookEvent::PreToolUseResult => "PreToolUseResult", HookEvent::PostToolUse => "PostToolUse", HookEvent::PostToolUseFailure => "PostToolUseFailure", HookEvent::SessionStart => "SessionStart", @@ -82,6 +84,7 @@ impl HookEvent { fn from_name(name: &str) -> Option { Some(match name { "PreToolUse" => HookEvent::PreToolUse, + "PreToolUseResult" => HookEvent::PreToolUseResult, "PostToolUse" => HookEvent::PostToolUse, "PostToolUseFailure" => HookEvent::PostToolUseFailure, "SessionStart" => HookEvent::SessionStart, @@ -158,6 +161,11 @@ pub struct HookContext { pub event: String, pub session_id: String, pub matcher_context: Option, + /// Stable identifier for one tool call, the same value goose records as + /// `gen_ai.tool.call.id`. Correlates the pre and post events of a single + /// call, which tool name plus input cannot do when a call repeats. + #[serde(skip_serializing_if = "Option::is_none")] + pub tool_call_id: Option, #[serde(skip_serializing_if = "Option::is_none")] pub tool_name: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -170,6 +178,19 @@ pub struct HookContext { pub last_assistant_message: Option, #[serde(skip_serializing_if = "Option::is_none")] pub working_dir: Option, + /// `PreToolUseResult` only: "allow" or "deny". There is no third value. + #[serde(skip_serializing_if = "Option::is_none")] + pub decision: Option, + /// `PreToolUseResult` only: true when at least one matching `PreToolUse` + /// hook ran to completion for this call. + #[serde(skip_serializing_if = "Option::is_none")] + pub policy_evaluated: Option, + /// `PreToolUseResult` on deny only: the plugin that denied. + #[serde(skip_serializing_if = "Option::is_none")] + pub blocked_by: Option, + /// `PreToolUseResult` on deny only: the reason the plugin gave. + #[serde(skip_serializing_if = "Option::is_none")] + pub reason: Option, } impl HookContext { @@ -178,12 +199,17 @@ impl HookContext { event: event.to_string(), session_id: session_id.into(), matcher_context: None, + tool_call_id: None, tool_name: None, tool_input: None, tool_output: None, message: None, last_assistant_message: None, working_dir: None, + decision: None, + policy_evaluated: None, + blocked_by: None, + reason: None, } } @@ -219,6 +245,26 @@ impl HookContext { self.working_dir = Some(dir.into()); self } + + pub fn with_tool_call_id(mut self, tool_call_id: impl Into) -> Self { + self.tool_call_id = Some(tool_call_id.into()); + self + } + + /// Populate the `PreToolUseResult` outcome fields. `blocked_by` and `reason` + /// are set only on deny, so an allow payload omits them entirely. + pub(crate) fn with_pre_tool_use_outcome(mut self, outcome: &HookChainOutcome) -> Self { + self.policy_evaluated = Some(outcome.policy_evaluated); + match &outcome.decision { + HookDecision::Allow => self.decision = Some("allow".to_string()), + HookDecision::Deny { reason, plugin } => { + self.decision = Some("deny".to_string()); + self.blocked_by = Some(plugin.clone()); + self.reason = Some(reason.clone()); + } + } + self + } } #[derive(Debug, Clone, PartialEq, Eq)] @@ -227,6 +273,29 @@ pub enum HookDecision { Deny { reason: String, plugin: String }, } +/// Result of running a blocking hook chain: the decision, plus whether any +/// matching hook actually ran to completion for this event. A hook counts as +/// evaluated when it exited 0 or returned a decision. A hook that exited +/// non-zero without a decision, failed to spawn, timed out, or was never +/// reached does not count. +/// +/// Crate-internal: the public [`HookManager::emit_blocking`] contract is +/// unchanged and still returns a [`HookDecision`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct HookChainOutcome { + pub decision: HookDecision, + pub policy_evaluated: bool, +} + +impl HookChainOutcome { + pub(crate) fn allow(policy_evaluated: bool) -> Self { + Self { + decision: HookDecision::Allow, + policy_evaluated, + } + } +} + /// Loads and executes plugin hooks. #[derive(Debug, Default, Clone)] pub struct HookManager { @@ -477,18 +546,30 @@ impl HookManager { /// to stdout. All other failures (spawn, timeout, other non-zero exits) /// are logged and treated as Allow — a misbehaving hook MUST NOT block. pub async fn emit_blocking(&self, event: HookEvent, ctx: HookContext) -> HookDecision { + self.emit_blocking_with_outcome(event, ctx).await.decision + } + + /// Like [`Self::emit_blocking`], but also reports whether any matching hook + /// ran to completion, which `PreToolUseResult` needs for `policy_evaluated`. + pub(crate) async fn emit_blocking_with_outcome( + &self, + event: HookEvent, + ctx: HookContext, + ) -> HookChainOutcome { let Some(rules) = self.rules.get(&event) else { - return HookDecision::Allow; + return HookChainOutcome::allow(false); }; let payload = match serde_json::to_string(&ctx) { Ok(s) => s, Err(err) => { warn!(event = %event, error = %err, "Failed to serialize hook context"); - return HookDecision::Allow; + return HookChainOutcome::allow(false); } }; + let mut policy_evaluated = false; + for rule in rules { if let Some(matcher) = &rule.matcher { let target = ctx.matcher_context.as_deref().unwrap_or(""); @@ -503,7 +584,15 @@ impl HookManager { .run_action(event, &ctx.session_id, rule, command, &payload, *timeout) .await { - Ok(o) => o, + Ok(output) => { + // Exiting 0 or returning a decision is what makes a hook + // an evaluation. A non-zero exit with no decision means + // the hook did not answer, so it does not count. + if output.status.success() || deny_reason(&output).is_some() { + policy_evaluated = true; + } + output + } Err(err) => { warn!( plugin = %rule.plugin_name, @@ -524,15 +613,18 @@ impl HookManager { reason = %reason, "Plugin hook denied tool call", ); - return HookDecision::Deny { - reason, - plugin: rule.plugin_name.clone(), + return HookChainOutcome { + decision: HookDecision::Deny { + reason, + plugin: rule.plugin_name.clone(), + }, + policy_evaluated, }; } } } - HookDecision::Allow + HookChainOutcome::allow(policy_evaluated) } } @@ -797,6 +889,169 @@ mod tests { HookManager::from_plugins(plugins, false) } + /// `decision` is "allow" or "deny" and nothing else, and `blocked_by` and + /// `reason` appear only on the deny arm — absent from an allow payload + /// rather than present as null or an empty string. + #[test] + fn pre_tool_use_result_payload_reports_decision_and_denies_alone_carry_the_plugin() { + let payload = |outcome: &HookChainOutcome| -> Value { + let ctx = HookContext::new(HookEvent::PreToolUseResult, "session-1") + .with_tool("developer__shell", None) + .with_tool_call_id("call-1") + .with_pre_tool_use_outcome(outcome); + serde_json::from_str(&serde_json::to_string(&ctx).unwrap()).unwrap() + }; + + let allow = payload(&HookChainOutcome::allow(true)); + assert_eq!(allow["decision"], "allow"); + assert_eq!(allow["policy_evaluated"], true); + assert_eq!(allow["tool_call_id"], "call-1"); + assert!( + allow.get("blocked_by").is_none(), + "allow payload must omit blocked_by entirely, got {:?}", + allow.get("blocked_by") + ); + assert!( + allow.get("reason").is_none(), + "allow payload must omit reason entirely, got {:?}", + allow.get("reason") + ); + + let deny = payload(&HookChainOutcome { + decision: HookDecision::Deny { + reason: "blocked by test policy".to_string(), + plugin: "test-plugin".to_string(), + }, + policy_evaluated: true, + }); + assert_eq!(deny["decision"], "deny"); + assert_eq!(deny["blocked_by"], "test-plugin"); + assert_eq!(deny["reason"], "blocked by test policy"); + + for value in [&allow, &deny] { + let decision = value["decision"].as_str().unwrap(); + assert!( + matches!(decision, "allow" | "deny"), + "decision must be allow or deny, got {decision}" + ); + } + + let unevaluated = payload(&HookChainOutcome::allow(false)); + assert_eq!(unevaluated["decision"], "allow"); + assert_eq!(unevaluated["policy_evaluated"], false); + } + + /// A hook is an evaluation only if it exited 0 or returned a decision. A + /// non-zero exit carrying no decision means the hook never answered, and an + /// earlier hook that did answer keeps the aggregate true. + #[tokio::test] + async fn policy_evaluated_counts_clean_exits_and_decisions_only() { + let plugin = |root: &Path, name: &str, command: &str| -> DiscoveredPlugin { + let hooks = format!( + r#"{{"hooks":{{"PreToolUse":[{{"hooks":[{{"type":"command","command":"{command}"}}]}}]}}}}"# + ); + DiscoveredPlugin { + name: name.into(), + root: write_plugin(root, name, &hooks), + scope: PluginScope::User, + } + }; + let ctx = + || HookContext::new(HookEvent::PreToolUse, "s").with_tool("developer__shell", None); + + // Case 1: the only hook exits non-zero with nothing on stdout. It gave no + // decision, so the call is allowed and nothing was evaluated. + let tmp = tempfile::tempdir().unwrap(); + let mgr = make_manager(vec![plugin(tmp.path(), "abnormal", "exit 3")]); + let outcome = mgr + .emit_blocking_with_outcome(HookEvent::PreToolUse, ctx()) + .await; + assert_eq!(outcome.decision, HookDecision::Allow); + assert!( + !outcome.policy_evaluated, + "a sole hook exiting 3 with no decision must not count as evaluated", + ); + + // Case 2: one hook exits 0 and another exits non-zero. policy_evaluated is + // an at-least-one aggregate, so the clean exit keeps it true. + let tmp = tempfile::tempdir().unwrap(); + let mgr = make_manager(vec![ + plugin(tmp.path(), "a", "exit 0"), + plugin(tmp.path(), "b", "exit 3"), + ]); + let outcome = mgr + .emit_blocking_with_outcome(HookEvent::PreToolUse, ctx()) + .await; + assert_eq!(outcome.decision, HookDecision::Allow); + assert!( + outcome.policy_evaluated, + "a hook that exited 0 must keep policy_evaluated true when a later hook fails", + ); + + // Case 3: the only hook exits 2 with a reason. That is a decision, so it + // both denies and counts as evaluated. + let tmp = tempfile::tempdir().unwrap(); + let mgr = make_manager(vec![plugin( + tmp.path(), + "denier", + "echo refused by policy >&2; exit 2", + )]); + let outcome = mgr + .emit_blocking_with_outcome(HookEvent::PreToolUse, ctx()) + .await; + assert_eq!( + outcome.decision, + HookDecision::Deny { + reason: "refused by policy".to_string(), + plugin: "denier".to_string(), + } + ); + assert!( + outcome.policy_evaluated, + "an exit 2 decision must count as evaluated", + ); + } + + /// PreToolUseResult honours its matcher against the tool name like every + /// other tool-scoped event, so a subscriber can watch one tool rather than + /// every call. + #[tokio::test] + async fn pre_tool_use_result_matcher_targets_the_tool_name() { + let tmp = tempfile::tempdir().unwrap(); + let root = write_plugin( + tmp.path(), + "p", + r#"{"hooks":{"PreToolUseResult":[{"matcher":"^developer__shell$","hooks":[{"type":"command","command":"echo ran >> \"$PLUGIN_ROOT/marker.log\""}]}]}}"#, + ); + let marker = root.join("marker.log"); + let mgr = make_manager(vec![DiscoveredPlugin { + name: "p".into(), + root, + scope: PluginScope::User, + }]); + let lines = || { + std::fs::read_to_string(&marker) + .unwrap_or_default() + .lines() + .filter(|line| !line.trim().is_empty()) + .count() + }; + + mgr.emit( + HookEvent::PreToolUseResult, + HookContext::new(HookEvent::PreToolUseResult, "s").with_tool("developer__shell", None), + ) + .await; + assert_eq!(lines(), 1, "the matching tool must run the hook"); + + mgr.emit( + HookEvent::PreToolUseResult, + HookContext::new(HookEvent::PreToolUseResult, "s").with_tool("other__tool", None), + ) + .await; + assert_eq!(lines(), 1, "a non-matching tool must not run the hook"); + } + #[test] fn ignores_unknown_events() { let tmp = tempfile::tempdir().unwrap(); diff --git a/documentation/docs/guides/context-engineering/hooks.md b/documentation/docs/guides/context-engineering/hooks.md index f5653e268..62d91e472 100644 --- a/documentation/docs/guides/context-engineering/hooks.md +++ b/documentation/docs/guides/context-engineering/hooks.md @@ -136,6 +136,7 @@ Use `${PLUGIN_ROOT}` in a command to reference the plugin directory. goose also | `Stop` | goose finishes a turn or receives a stop event | None | | `UserPromptSubmit` | The user submits a prompt | Prompt text | | `PreToolUse` | Before goose runs a tool | Tool name | +| `PreToolUseResult` | After the `PreToolUse` chain resolves, for allowed and denied calls alike, before the tool runs or the denial is returned. Observation only | Tool name | | `PostToolUse` | After a tool succeeds | Tool name | | `PostToolUseFailure` | After a tool fails | Tool name | | `BeforeReadFile` | Before goose reads a file | File path | @@ -153,6 +154,8 @@ The matcher is a regular expression, not a glob. A bare `"*"` is an invalid rege `AfterFileEdit` and `AfterShellExecution` only run after successful tool calls. To react to failed edits, failed shell commands, or other failed tool calls, use `PostToolUseFailure`. ::: +`PreToolUseResult` is observation only in authority, not asynchronous in delivery. Matching hooks are run and awaited before goose continues to the tool or returns the denial, so a slow subscriber adds its runtime, up to its timeout, to the tool call. Delivery is best effort and not durable: a subscriber that fails or is absent changes nothing about the decision, and no record is kept if the hook does not run. + ## Hook Payload When a hook runs, goose writes a JSON payload to the command's stdin. Every payload includes the event name and session ID. The remaining fields are only present when they apply to the event, so a hook should treat them as optional. @@ -167,6 +170,11 @@ When a hook runs, goose writes a JSON payload to the command's stdin. Every payl | `message` | Prompt text the user submitted, on `UserPromptSubmit`. | | `last_assistant_message` | Final assistant text for the turn, on `Stop` when there is assistant output. | | `working_dir` | Working directory of the session, on tool events. | +| `tool_call_id` | Stable identifier for one tool call, on `PreToolUse`, `PreToolUseResult`, `PostToolUse`, and `PostToolUseFailure`. Correlates the events of a single call, which tool name plus input cannot do when the same call repeats. | +| `decision` | `allow` or `deny`, on `PreToolUseResult`. There is no third value. | +| `policy_evaluated` | `true` when at least one matching `PreToolUse` hook exited 0 or returned a decision (exit `2`, or `{"decision":"block"}` on stdout), on `PreToolUseResult`. A hook that exits non-zero without a decision, fails to spawn, or times out does not count, and neither does the absence of a matching hook. It is an at-least-one value: a hook that evaluated keeps it `true` even if a later hook fails. | +| `blocked_by` | Plugin whose hook denied the call, on `PreToolUseResult` when `decision` is `deny`. | +| `reason` | Reason the denying hook gave, on `PreToolUseResult` when `decision` is `deny`. | Example payload for a tool event: