From ff5935fa120474f5068a36741e61577e5b74310a Mon Sep 17 00:00:00 2001 From: Tymofii Pidlisnyi Date: Tue, 25 Aug 2026 02:08:37 +0000 Subject: [PATCH] feat(hooks): add on_failure block for PreToolUse hooks (#11449) Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com> Co-authored-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com> Co-authored-by: Lifei Zhou --- crates/goose/src/agents/agent.rs | 79 +- .../agents/state_machine/ops_toolcalling.rs | 15 +- .../state_machine/tests/hooks_lifecycle.rs | 87 +- crates/goose/src/hooks/mod.rs | 1246 +++++++++++++++-- .../docs/guides/context-engineering/hooks.md | 95 +- 5 files changed, 1378 insertions(+), 144 deletions(-) diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 84a06aa43..42794f68b 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -691,9 +691,7 @@ impl Agent { .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; + self.hook_manager.emit_pre_tool_use_result(ctx).await; } fn with_post_tool_hook( @@ -1177,6 +1175,7 @@ impl Agent { gen_ai.tool.call.id = %request_id, gen_ai.tool.call.arguments = tracing::field::Empty, gen_ai.tool.call.result = tracing::field::Empty, + error.type = tracing::field::Empty, ) )] pub async fn dispatch_tool_call( @@ -1231,15 +1230,13 @@ impl Agent { ) .await; - if let crate::hooks::HookDecision::Deny { reason, plugin } = pre_tool_outcome.decision { + if let Some(denial) = pre_tool_outcome.denial() { + tracing::Span::current().record("error.type", denial.error_type); 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." - ), + denial.message, None, )), ); @@ -5728,6 +5725,14 @@ echo start >> "$PLUGIN_ROOT/hook.log" impl RecordingHookEnv { fn new(specs: &[HookSpec<'_>]) -> Self { + Self::with_on_failure(specs, "") + } + + fn blocking_on_failure(specs: &[HookSpec<'_>]) -> Self { + Self::with_on_failure(specs, r#", "on_failure": "block""#) + } + + fn with_on_failure(specs: &[HookSpec<'_>], on_failure: &str) -> 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(); @@ -5740,7 +5745,7 @@ echo start >> "$PLUGIN_ROOT/hook.log" format!(r#""matcher": "{matcher}", "#) }; format!( - r#""{event}": [{{{matcher}"hooks": [{{"type": "command", "command": "sh ${{PLUGIN_ROOT}}/{script}"}}]}}]"# + r#""{event}": [{{{matcher}"hooks": [{{"type": "command", "command": "sh ${{PLUGIN_ROOT}}/{script}"{on_failure}}}]}}]"# ) }) .collect(); @@ -5790,6 +5795,10 @@ echo start >> "$PLUGIN_ROOT/hook.log" /// 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"; + const HOOK_FAILURE_REFUSAL: &str = + "Tool call blocked because policy hook `test-plugin` could not complete: \ + the hook exited with status 3 and no usable decision. \ + That hook is configured to block on failure."; async fn agent_with_hooks( hook_manager: crate::hooks::HookManager, @@ -6049,4 +6058,56 @@ echo start >> "$PLUGIN_ROOT/hook.log" "PostToolUse must not fire for a tool that never ran", ); } + + #[tokio::test] + async fn pre_tool_use_hook_failure_allows_by_default() { + 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-open-1".to_string(), None, &session) + .await; + assert!(result.is_ok(), "a broken hook must not block the call"); + + assert_eq!(env.payloads("pre.log").len(), 1); + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["decision"], "allow"); + assert_eq!(results[0]["cause"], "hook_failure"); + assert_eq!(results[0]["policy_evaluated"], false); + } + + #[tokio::test] + async fn pre_tool_use_hook_failure_blocks_when_configured() { + let env = RecordingHookEnv::blocking_on_failure(&[ + ("PreToolUse", "", "pre.sh", ABNORMAL_EXIT_AND_RECORD_SCRIPT), + ("PreToolUseResult", "", "result.sh", RECORD_RESULT_SCRIPT), + ("PostToolUse", "", "post.sh", RECORD_POST_SCRIPT), + ]); + let (agent, session, _data_dir) = agent_with_hooks(env.hook_manager()).await; + + let (_, result) = agent + .dispatch_tool_call(shell_call(), "call-closed-1".to_string(), None, &session) + .await; + + let Err(error) = result else { + panic!("a fail-closed hook failure must not dispatch"); + }; + assert_eq!(error.message, HOOK_FAILURE_REFUSAL); + assert!( + env.payloads("post.log").is_empty(), + "PostToolUse must not fire for a blocked call" + ); + + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["decision"], "deny"); + assert_eq!(results[0]["cause"], "hook_failure"); + assert_eq!(results[0]["policy_evaluated"], false); + assert_eq!(results[0]["blocked_by"], "test-plugin"); + assert_eq!(results[0]["tool_call_id"], "call-closed-1"); + } } diff --git a/crates/goose/src/agents/state_machine/ops_toolcalling.rs b/crates/goose/src/agents/state_machine/ops_toolcalling.rs index 40736404a..f97acd677 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::{HookChainOutcome, HookContext, HookDecision, HookEvent, HookManager}; +use crate::hooks::{HookChainOutcome, HookContext, HookEvent, HookManager}; use crate::session::{EnabledExtensionsState, ExtensionState, Session}; use std::sync::Arc; use tokio::sync::Mutex; @@ -100,9 +100,7 @@ async fn emit_pre_tool_use_result( .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; + hook_manager.emit_pre_tool_use_result(context).await; } /// Runs the `PreToolUse` chain, emits `PreToolUseResult`, and reports a denial @@ -145,14 +143,11 @@ pub(super) async fn run_pre_tool_hooks( ) .await; - if let HookDecision::Deny { reason, plugin } = outcome.decision { - tracing::Span::current().record("error.type", "hook_denied"); + if let Some(denial) = outcome.denial() { + tracing::Span::current().record("error.type", denial.error_type); 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." - ), + denial.message, None, )); } 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 8ada75038..a86c1bd6e 100644 --- a/crates/goose/src/agents/state_machine/tests/hooks_lifecycle.rs +++ b/crates/goose/src/agents/state_machine/tests/hooks_lifecycle.rs @@ -338,6 +338,14 @@ type HookSpec<'a> = (&'a str, &'a str, &'a str, &'a str); impl RecordingHookEnv { fn new(specs: &[HookSpec<'_>]) -> Self { + Self::with_on_failure(specs, "") + } + + fn blocking_on_failure(specs: &[HookSpec<'_>]) -> Self { + Self::with_on_failure(specs, r#", "on_failure": "block""#) + } + + fn with_on_failure(specs: &[HookSpec<'_>], on_failure: &str) -> 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(); @@ -350,7 +358,7 @@ impl RecordingHookEnv { format!(r#""matcher": "{matcher}", "#) }; format!( - r#""{event}": [{{{matcher}"hooks": [{{"type": "command", "command": "sh ${{PLUGIN_ROOT}}/{script}"}}]}}]"# + r#""{event}": [{{{matcher}"hooks": [{{"type": "command", "command": "sh ${{PLUGIN_ROOT}}/{script}"{on_failure}}}]}}]"# ) }) .collect(); @@ -403,6 +411,10 @@ const DENY_AND_RECORD_SCRIPT: &str = /// 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"; +const HOOK_FAILURE_REFUSAL: &str = + "Tool call blocked because policy hook `test-plugin` could not complete: \ + the hook exited with status 3 and no usable decision. \ + That hook is configured to block on failure."; /// deny-invisible: the tool never dispatches, neither post event fires, and a /// PreToolUseResult subscriber still sees the denial with blocked_by and reason. @@ -1598,3 +1610,76 @@ async fn final_output_waits_for_ordinary_sibling_and_answers_every_request() -> ); Ok(()) } + +#[tokio::test] +async fn pre_tool_use_hook_failure_allows_by_default() -> Result<()> { + let env = RecordingHookEnv::new(&[ + ("PreToolUse", "", "pre.sh", ABNORMAL_EXIT_AND_RECORD_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").reply("done"); + + pipeline.run(["add one"]).await?; + + assert_eq!( + pipeline.calculator_total(), + 1, + "a broken hook must not block" + ); + assert_eq!(env.payloads("post.log").len(), 1); + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["decision"], "allow"); + assert_eq!(results[0]["cause"], "hook_failure"); + assert_eq!(results[0]["policy_evaluated"], false); + Ok(()) +} + +#[tokio::test] +async fn pre_tool_use_hook_failure_blocks_when_configured() -> Result<()> { + let env = RecordingHookEnv::blocking_on_failure(&[ + ("PreToolUse", "", "pre.sh", ABNORMAL_EXIT_AND_RECORD_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("could not complete").reply("understood"); + + let result = 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 blocked call" + ); + + let results = env.payloads("result.log"); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["decision"], "deny"); + assert_eq!(results[0]["cause"], "hook_failure"); + assert_eq!(results[0]["policy_evaluated"], false); + assert_eq!(results[0]["blocked_by"], "test-plugin"); + + 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("blocked tool response"); + assert!( + tool_error.message.ends_with(HOOK_FAILURE_REFUSAL), + "expected the shared refusal, got {}", + tool_error.message + ); + Ok(()) +} diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index 7bdce3933..8f600d5cd 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -43,6 +43,10 @@ use crate::plugins::discovery::{discover_enabled_plugins, DiscoveredPlugin}; /// Default per-hook timeout when the plugin does not specify one. const DEFAULT_HOOK_TIMEOUT_SECS: u64 = 30; +const COMMAND_FAILED_REASON: &str = "the hook command failed to run"; +const SERIALIZATION_FAILED_REASON: &str = "the hook payload could not be serialized"; +const STDIN_DELIVERY_FAILED_REASON: &str = "the hook did not receive the request payload"; + /// Lifecycle events a hook can subscribe to. /// /// The variant names match the event names used in `hooks.json`. Unknown @@ -106,33 +110,83 @@ impl std::fmt::Display for HookEvent { } } -/// Top-level `hooks.json` shape. #[derive(Debug, Default, Deserialize)] struct HooksFile { #[serde(default)] - hooks: HashMap>, + hooks: HashMap, } -/// One rule within a `hooks.json` event entry. #[derive(Debug, Deserialize)] struct RawHookRule { #[serde(default)] matcher: Option, #[serde(default)] - hooks: Vec, + hooks: Vec, } -/// One action entry under a rule's `hooks` array. We only run `command` -/// today, but we deserialize the others so that loading a plugin which uses -/// them does not fail. #[derive(Debug, Deserialize)] -struct RawHookAction { - #[serde(default, rename = "type")] - action_type: Option, - #[serde(default)] - command: Option, +struct RawCommandAction { + command: String, #[serde(default)] timeout: Option, + #[serde(default, deserialize_with = "deserialize_present_on_failure")] + on_failure: Option, +} + +#[derive(Debug, PartialEq, Eq)] +enum ActionSelection { + Selected, + Ignored, +} + +fn select_action(action: &Value, plugin_name: &str, path: &Path) -> Result { + let Some(obj) = action.as_object() else { + anyhow::bail!("hook action in {} must be an object", path.display()); + }; + + match obj.get("type") { + None | Some(Value::Null) => {} + Some(Value::String(action_type)) if action_type == "command" => {} + Some(Value::String(action_type)) => { + debug!( + plugin = plugin_name, + action_type = %action_type, + "Ignoring unsupported hook action type", + ); + return Ok(ActionSelection::Ignored); + } + Some(_) => anyhow::bail!("hook action `type` in {} must be a string", path.display()), + } + + match obj.get("command") { + None | Some(Value::Null) => { + debug!( + plugin = plugin_name, + "Ignoring command hook action with no command", + ); + Ok(ActionSelection::Ignored) + } + Some(Value::String(_)) => Ok(ActionSelection::Selected), + Some(_) => anyhow::bail!( + "hook action `command` in {} must be a string", + path.display() + ), + } +} + +fn deserialize_present_on_failure<'de, D>(d: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + serde_json::Value::deserialize(d).map(Some) +} + +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "lowercase")] +enum OnFailure { + #[default] + Allow, + Block, } /// A loaded, plugin-bound hook rule ready to execute. @@ -146,7 +200,18 @@ struct LoadedRule { #[derive(Debug, Clone)] enum LoadedAction { - Command { command: String, timeout: Duration }, + Command { + command: String, + timeout: Duration, + on_failure: OnFailure, + }, +} + +impl LoadedAction { + fn on_failure(&self) -> OnFailure { + let LoadedAction::Command { on_failure, .. } = self; + *on_failure + } } /// Context passed to a hook as JSON on stdin. @@ -253,7 +318,10 @@ impl HookContext { /// 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 { + pub(crate) fn with_pre_tool_use_outcome( + mut self, + outcome: &HookChainOutcome, + ) -> PreToolUseResultPayload { self.policy_evaluated = Some(outcome.policy_evaluated); match &outcome.decision { HookDecision::Allow => self.decision = Some("allow".to_string()), @@ -263,28 +331,49 @@ impl HookContext { self.reason = Some(reason.clone()); } } - self + PreToolUseResultPayload { + context: self, + cause: outcome.cause.map(|cause| cause.as_str().to_string()), + } } } +#[derive(Debug, Serialize)] +pub(crate) struct PreToolUseResultPayload { + #[serde(flatten)] + context: HookContext, + #[serde(skip_serializing_if = "Option::is_none")] + cause: Option, +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum HookDecision { Allow, 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. -/// +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum HookOutcomeCause { + PolicyDenial, + HookFailure, +} + +impl HookOutcomeCause { + pub(crate) fn as_str(self) -> &'static str { + match self { + HookOutcomeCause::PolicyDenial => "policy_denial", + HookOutcomeCause::HookFailure => "hook_failure", + } + } +} + /// 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, + pub cause: Option, } impl HookChainOutcome { @@ -292,8 +381,36 @@ impl HookChainOutcome { Self { decision: HookDecision::Allow, policy_evaluated, + cause: None, } } + + pub(crate) fn denial(&self) -> Option { + let HookDecision::Deny { reason, plugin } = &self.decision else { + return None; + }; + Some(match self.cause { + Some(HookOutcomeCause::HookFailure) => HookDenial { + message: format!( + "Tool call blocked because policy hook `{plugin}` could not complete: \ + {reason}. That hook is configured to block on failure." + ), + error_type: "hook_failed", + }, + _ => HookDenial { + message: format!( + "Tool call denied by policy hook `{plugin}`: {reason}. \ + Do not retry; this is a policy denial, not a transient failure." + ), + error_type: "hook_denied", + }, + }) + } +} + +pub(crate) struct HookDenial { + pub message: String, + pub error_type: &'static str, } /// Loads and executes plugin hooks. @@ -367,7 +484,7 @@ impl HookManager { command: &str, payload: &str, timeout: Duration, - ) -> Result { + ) -> Result { let span = tracing::info_span!( target: "goose::hooks", "execute_hook", @@ -387,7 +504,7 @@ impl HookManager { .instrument(span.clone()) .await; match &result { - Ok(output) if !output.status.success() => { + Ok(run) if !run.output.status.success() => { span.record("error.type", "hook_exit"); } Err(_) => { @@ -402,13 +519,9 @@ impl HookManager { /// individual hooks are logged but never propagated — a misbehaving hook /// MUST NOT crash the host tool. pub async fn emit(&self, event: HookEvent, ctx: HookContext) { - let Some(rules) = self.rules.get(&event) else { - return; - }; - if rules.is_empty() { + if !self.has_hooks(event) { return; } - let payload = match serde_json::to_string(&ctx) { Ok(s) => s, Err(err) => { @@ -416,17 +529,59 @@ impl HookManager { return; } }; + self.emit_serialized( + event, + ctx.matcher_context.as_deref(), + &ctx.session_id, + &payload, + ) + .await; + } + + pub(crate) async fn emit_pre_tool_use_result(&self, payload: PreToolUseResultPayload) { + let event = HookEvent::PreToolUseResult; + if !self.has_hooks(event) { + return; + } + let json = match serde_json::to_string(&payload) { + Ok(s) => s, + Err(err) => { + warn!(event = %event, error = %err, "Failed to serialize hook context"); + return; + } + }; + self.emit_serialized( + event, + payload.context.matcher_context.as_deref(), + &payload.context.session_id, + &json, + ) + .await; + } + + async fn emit_serialized( + &self, + event: HookEvent, + matcher_context: Option<&str>, + session_id: &str, + payload: &str, + ) { + let Some(rules) = self.rules.get(&event) else { + return; + }; for rule in rules { if let Some(matcher) = &rule.matcher { - let target = ctx.matcher_context.as_deref().unwrap_or(""); + let target = matcher_context.unwrap_or(""); if !matcher.is_match(target) { continue; } } for action in &rule.actions { - let LoadedAction::Command { command, timeout } = action; + let LoadedAction::Command { + command, timeout, .. + } = action; debug!( plugin = %rule.plugin_name, event = %event, @@ -434,16 +589,16 @@ impl HookManager { "Running plugin hook", ); let res = self - .run_action(event, &ctx.session_id, rule, command, &payload, *timeout) + .run_action(event, session_id, rule, command, payload, *timeout) .await - .and_then(|o| { - if o.status.success() { + .and_then(|run| { + if run.output.status.success() { Ok(()) } else { anyhow::bail!( "hook `{command}` exited with {:?}: {}", - o.status.code(), - String::from_utf8_lossy(&o.stderr).trim() + run.output.status.code(), + String::from_utf8_lossy(&run.output.stderr).trim() ) } }); @@ -492,7 +647,9 @@ impl HookManager { } for action in &rule.actions { - let LoadedAction::Command { command, timeout } = action; + let LoadedAction::Command { + command, timeout, .. + } = action; debug!( plugin = %rule.plugin_name, event = %event, @@ -508,20 +665,20 @@ impl HookManager { ) .await { - Ok(output) if output.status.success() => { - let stdout = String::from_utf8_lossy(&output.stdout); + Ok(run) if run.output.status.success() => { + let stdout = String::from_utf8_lossy(&run.output.stdout); if let Some(banner) = extract_banner(stdout.trim()) { banners.push(banner); } } - Ok(output) => { + Ok(run) => { warn!( plugin = %rule.plugin_name, event = %event, command = %command, "hook exited with {:?}: {}", - output.status.code(), - String::from_utf8_lossy(&output.stderr).trim(), + run.output.status.code(), + String::from_utf8_lossy(&run.output.stderr).trim(), ); } Err(err) => { @@ -543,73 +700,86 @@ impl HookManager { /// Like [`Self::emit`], but stops at the first rule that denies the event /// and returns the denial. A hook denies by exiting with status code 2 /// (reason on stderr) or by printing `{"decision":"block","reason":"..."}` - /// 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 { + let matched = self.matching_actions(event, &ctx); + if matched.is_empty() { return HookChainOutcome::allow(false); - }; + } let payload = match serde_json::to_string(&ctx) { - Ok(s) => s, + Ok(payload) => payload, Err(err) => { warn!(event = %event, error = %err, "Failed to serialize hook context"); - return HookChainOutcome::allow(false); + return serialization_failure_outcome(&matched, event); } }; let mut policy_evaluated = false; + let mut failed = false; - for rule in rules { - if let Some(matcher) = &rule.matcher { - let target = ctx.matcher_context.as_deref().unwrap_or(""); - if !matcher.is_match(target) { - continue; + for (rule, action) in matched { + let LoadedAction::Command { + command, + timeout, + on_failure, + } = action; + let (verdict, evaluated, already_logged) = match self + .run_action(event, &ctx.session_id, rule, command, &payload, *timeout) + .await + { + Ok(run) => { + let verdict = classify_run(&run); + let evaluated = run.output.status.success() + || matches!(verdict, HookVerdict::PolicyDeny { .. }); + (verdict, evaluated, false) } - } + Err(err) => { + warn!( + plugin = %rule.plugin_name, + event = %event, + command = %command, + error = %format!("{err:#}"), + "Plugin hook could not be executed", + ); + ( + HookVerdict::HookFailure { + reason: COMMAND_FAILED_REASON.to_string(), + }, + false, + true, + ) + } + }; + policy_evaluated |= evaluated; - for action in &rule.actions { - let LoadedAction::Command { command, timeout } = action; - let output = match self - .run_action(event, &ctx.session_id, rule, command, &payload, *timeout) - .await - { - 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) => { + match apply_verdict(verdict, *on_failure, event) { + ChainStep::Allowed => {} + ChainStep::FailedOpen { reason } => { + if !already_logged { warn!( plugin = %rule.plugin_name, event = %event, command = %command, - error = %err, - "Plugin hook failed", + reason = %reason, + "Plugin hook failed; continuing without it", ); - continue; } - }; - - if let Some(reason) = deny_reason(&output) { + failed = true; + } + ChainStep::Denied { reason, cause } => { info!( plugin = %rule.plugin_name, event = %event, command = %command, + cause = cause.as_str(), reason = %reason, "Plugin hook denied tool call", ); @@ -619,12 +789,103 @@ impl HookManager { plugin: rule.plugin_name.clone(), }, policy_evaluated, + cause: Some(cause), }; } } } - HookChainOutcome::allow(policy_evaluated) + HookChainOutcome { + decision: HookDecision::Allow, + policy_evaluated, + cause: failed.then_some(HookOutcomeCause::HookFailure), + } + } + + fn matching_actions( + &self, + event: HookEvent, + ctx: &HookContext, + ) -> Vec<(&LoadedRule, &LoadedAction)> { + let Some(rules) = self.rules.get(&event) else { + return Vec::new(); + }; + let target = ctx.matcher_context.as_deref().unwrap_or(""); + rules + .iter() + .filter(|rule| rule.matcher.as_ref().is_none_or(|m| m.is_match(target))) + .flat_map(|rule| rule.actions.iter().map(move |action| (rule, action))) + .collect() + } +} + +fn serialization_failure_outcome( + matched: &[(&LoadedRule, &LoadedAction)], + event: HookEvent, +) -> HookChainOutcome { + let reason = SERIALIZATION_FAILED_REASON.to_string(); + let blocker = matched + .iter() + .find(|(_, action)| action.on_failure() == OnFailure::Block); + match blocker { + Some((rule, _)) if event == HookEvent::PreToolUse => HookChainOutcome { + decision: HookDecision::Deny { + reason, + plugin: rule.plugin_name.clone(), + }, + policy_evaluated: false, + cause: Some(HookOutcomeCause::HookFailure), + }, + _ => HookChainOutcome { + decision: HookDecision::Allow, + policy_evaluated: false, + cause: Some(HookOutcomeCause::HookFailure), + }, + } +} + +#[derive(Debug)] +struct HookRun { + output: std::process::Output, + stdin_delivered: bool, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +enum HookVerdict { + Allow, + PolicyDeny { reason: String }, + HookFailure { reason: String }, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +enum ChainStep { + Allowed, + Denied { + reason: String, + cause: HookOutcomeCause, + }, + FailedOpen { + reason: String, + }, +} + +fn apply_verdict(verdict: HookVerdict, on_failure: OnFailure, event: HookEvent) -> ChainStep { + match verdict { + HookVerdict::Allow => ChainStep::Allowed, + HookVerdict::PolicyDeny { reason } => ChainStep::Denied { + reason, + cause: HookOutcomeCause::PolicyDenial, + }, + HookVerdict::HookFailure { reason } => { + if on_failure == OnFailure::Block && event == HookEvent::PreToolUse { + ChainStep::Denied { + reason, + cause: HookOutcomeCause::HookFailure, + } + } else { + ChainStep::FailedOpen { reason } + } + } } } @@ -642,13 +903,25 @@ fn extract_banner(stdout: &str) -> Option { parsed.banner.filter(|b| !b.is_empty()) } -fn deny_reason(output: &std::process::Output) -> Option { - const DEFAULT: &str = "denied by plugin hook"; - let non_empty = |s: String| if s.is_empty() { DEFAULT.into() } else { s }; +fn classify_run(run: &HookRun) -> HookVerdict { + let verdict = classify_output(&run.output); + if run.stdin_delivered || matches!(verdict, HookVerdict::PolicyDeny { .. }) { + return verdict; + } + HookVerdict::HookFailure { + reason: STDIN_DELIVERY_FAILED_REASON.to_string(), + } +} + +fn classify_output(output: &std::process::Output) -> HookVerdict { + const DEFAULT_DENY: &str = "denied by plugin hook"; + let non_empty = |s: String| if s.is_empty() { DEFAULT_DENY.into() } else { s }; if output.status.code() == Some(2) { let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); - return Some(non_empty(stderr)); + return HookVerdict::PolicyDeny { + reason: non_empty(stderr), + }; } #[derive(Deserialize)] @@ -657,14 +930,39 @@ fn deny_reason(output: &std::process::Output) -> Option { reason: Option, } - let stdout = String::from_utf8_lossy(&output.stdout); + let Ok(stdout) = std::str::from_utf8(&output.stdout) else { + return HookVerdict::HookFailure { + reason: "the hook wrote invalid UTF-8 to stdout".to_string(), + }; + }; let trimmed = stdout.trim(); - if !trimmed.starts_with('{') { - return None; + let parsed = trimmed + .starts_with('{') + .then(|| serde_json::from_str::(trimmed).ok()) + .flatten(); + let (decision, reason) = match parsed { + Some(resp) => (resp.decision, resp.reason), + None => (None, None), + }; + + if decision.as_deref() == Some("block") { + return HookVerdict::PolicyDeny { + reason: non_empty(reason.unwrap_or_default()), + }; + } + if output.status.code() == Some(0) + && (trimmed.is_empty() || decision.as_deref() == Some("allow")) + { + return HookVerdict::Allow; + } + + HookVerdict::HookFailure { + reason: match output.status.code() { + Some(0) => "the hook exited 0 without an allow or block decision on stdout".to_string(), + Some(code) => format!("the hook exited with status {code} and no usable decision"), + None => "the hook was terminated by a signal".to_string(), + }, } - let parsed: Resp = serde_json::from_str(trimmed).ok()?; - (parsed.decision.as_deref() == Some("block")) - .then(|| non_empty(parsed.reason.unwrap_or_default())) } fn load_hooks_file( @@ -678,13 +976,25 @@ fn load_hooks_file( serde_json::from_str(&text).with_context(|| format!("parsing {}", path.display()))?; let mut out: HashMap> = HashMap::new(); - for (event_name, raw_rules) in parsed.hooks { + for (event_name, raw_payload) in parsed.hooks { let Some(event) = HookEvent::from_name(&event_name) else { debug!(plugin = plugin_name, event = %event_name, "Ignoring unknown hook event"); continue; }; + let raw_rules: Vec = serde_json::from_value(raw_payload) + .with_context(|| format!("reading `{event_name}` rules in {}", path.display()))?; for raw in raw_rules { + let mut selected = Vec::new(); + for raw_action in raw.hooks { + if select_action(&raw_action, plugin_name, path)? == ActionSelection::Ignored { + continue; + } + let action: RawCommandAction = serde_json::from_value(raw_action) + .with_context(|| format!("reading hook action in {}", path.display()))?; + selected.push(action); + } + let matcher = match raw.matcher.as_deref().filter(|s| !s.is_empty()) { Some(pattern) => match Regex::new(pattern) { Ok(re) => Some(re), @@ -702,27 +1012,23 @@ fn load_hooks_file( }; let mut actions = Vec::new(); - for raw_action in raw.hooks { - match raw_action.action_type.as_deref().unwrap_or("command") { - "command" => { - if let Some(cmd) = raw_action.command { - let timeout = Duration::from_secs( - raw_action.timeout.unwrap_or(DEFAULT_HOOK_TIMEOUT_SECS), - ); - actions.push(LoadedAction::Command { - command: cmd, - timeout, - }); - } + for action in selected { + let timeout = + Duration::from_secs(action.timeout.unwrap_or(DEFAULT_HOOK_TIMEOUT_SECS)); + let on_failure = if event == HookEvent::PreToolUse { + match action.on_failure { + None => OnFailure::Allow, + Some(value) => serde_json::from_value::(value) + .with_context(|| format!("reading on_failure in {}", path.display()))?, } - other => { - debug!( - plugin = plugin_name, - action_type = other, - "Ignoring unsupported hook action type", - ); - } - } + } else { + OnFailure::Allow + }; + actions.push(LoadedAction::Command { + command: action.command, + timeout, + on_failure, + }); } if actions.is_empty() { @@ -747,7 +1053,7 @@ async fn run_command_hook( payload: &str, timeout: Duration, use_login_shell_path: bool, -) -> Result { +) -> Result { match tokio::time::timeout( timeout, run_command_hook_inner(raw_command, plugin_root, payload, use_login_shell_path), @@ -764,7 +1070,7 @@ async fn run_command_hook_inner( plugin_root: &Path, payload: &str, use_login_shell_path: bool, -) -> Result { +) -> Result { let command = expand_plugin_root(raw_command, plugin_root); let path = if use_login_shell_path { hook_path().await @@ -781,15 +1087,25 @@ async fn run_command_hook_inner( .spawn() .with_context(|| format!("spawning hook `{command}`"))?; + let mut stdin_delivered = true; if let Some(mut stdin) = child.stdin.take() { - let _ = stdin.write_all(payload.as_bytes()).await; - let _ = stdin.shutdown().await; + if let Err(err) = stdin.write_all(payload.as_bytes()).await { + stdin_delivered = false; + warn!(command = %command, error = %err, "Could not deliver the hook payload"); + } else if let Err(err) = stdin.shutdown().await { + stdin_delivered = false; + warn!(command = %command, error = %err, "Could not close the hook stdin pipe"); + } } - child + let output = child .wait_with_output() .await - .with_context(|| format!("waiting on hook `{command}`")) + .with_context(|| format!("waiting on hook `{command}`"))?; + Ok(HookRun { + output, + stdin_delivered, + }) } fn hook_command(command: &str, plugin_root: &Path, path: Option<&str>) -> Command { @@ -889,6 +1205,672 @@ mod tests { HookManager::from_plugins(plugins, false) } + fn action(command: &str) -> Value { + serde_json::json!({ "type": "command", "command": command }) + } + + fn blocking_action(command: &str) -> Value { + serde_json::json!({ "type": "command", "command": command, "on_failure": "block" }) + } + + fn manager_for(root: &Path, event: HookEvent, plugins: &[(&str, Vec)]) -> HookManager { + let discovered = plugins + .iter() + .map(|(name, actions)| { + let hooks = serde_json::json!({ + "hooks": { event.name(): [{ "hooks": actions }] } + }) + .to_string(); + DiscoveredPlugin { + name: (*name).to_string(), + root: write_plugin(root, name, &hooks), + scope: PluginScope::User, + } + }) + .collect(); + make_manager(discovered) + } + + fn blocking_context(event: HookEvent) -> HookContext { + HookContext::new(event, "s").with_tool("developer__shell", None) + } + + async fn run_chain(event: HookEvent, actions: Vec) -> HookChainOutcome { + let tmp = tempfile::tempdir().unwrap(); + manager_for(tmp.path(), event, &[("p", actions)]) + .emit_blocking_with_outcome(event, blocking_context(event)) + .await + } + + fn oversized_context(event: HookEvent) -> HookContext { + let filler = "x".repeat(1024 * 1024); + HookContext::new(event, "s").with_tool( + "developer__shell", + Some(serde_json::json!({ "arg": filler })), + ) + } + + async fn run_chain_with( + event: HookEvent, + actions: Vec, + ctx: HookContext, + ) -> HookChainOutcome { + let tmp = tempfile::tempdir().unwrap(); + manager_for(tmp.path(), event, &[("p", actions)]) + .emit_blocking_with_outcome(event, ctx) + .await + } + + #[cfg(unix)] + fn exited(code: i32, stdout: &str, stderr: &str) -> std::process::Output { + use std::os::unix::process::ExitStatusExt; + std::process::Output { + status: std::process::ExitStatus::from_raw(code << 8), + stdout: stdout.as_bytes().to_vec(), + stderr: stderr.as_bytes().to_vec(), + } + } + + #[cfg(unix)] + fn exited_raw(code: i32, stdout: &[u8], stderr: &str) -> std::process::Output { + use std::os::unix::process::ExitStatusExt; + std::process::Output { + status: std::process::ExitStatus::from_raw(code << 8), + stdout: stdout.to_vec(), + stderr: stderr.as_bytes().to_vec(), + } + } + + #[cfg(unix)] + fn signalled(signal: i32) -> std::process::Output { + use std::os::unix::process::ExitStatusExt; + std::process::Output { + status: std::process::ExitStatus::from_raw(signal), + stdout: Vec::new(), + stderr: Vec::new(), + } + } + + #[cfg(unix)] + #[test] + fn classify_output_pins_every_boundary() { + let denies = |reason: &str| HookVerdict::PolicyDeny { + reason: reason.to_string(), + }; + + assert_eq!( + classify_output(&exited_raw(2, b"\xff", " path is protected ")), + denies("path is protected") + ); + assert_eq!( + classify_output(&exited(2, "", "")), + denies("denied by plugin hook") + ); + assert_eq!( + classify_output(&exited(1, r#"{"decision":"block","reason":"nope"}"#, "")), + denies("nope") + ); + assert_eq!( + classify_output(&exited(0, r#"{"decision":"block"}"#, "")), + denies("denied by plugin hook") + ); + + for stdout in ["", r#"{"decision":"allow"}"#] { + assert_eq!(classify_output(&exited(0, stdout, "")), HookVerdict::Allow); + } + + for output in [ + exited(1, r#"{"decision":"allow"}"#, ""), + exited(0, r#"{"decision":"maybe"}"#, ""), + exited(0, r#"{"decision" "allow"}"#, ""), + exited_raw(0, b"{\"decision\":\"allow\",\"reason\":\"\xff\"}", ""), + exited(1, "", ""), + signalled(9), + ] { + assert!( + matches!(classify_output(&output), HookVerdict::HookFailure { .. }), + "expected a hook failure, got {:?}", + classify_output(&output) + ); + } + } + + #[test] + fn on_failure_block_denies_pre_tool_use_failures_and_leaves_stop_alone() { + let failure = || HookVerdict::HookFailure { + reason: "failed".to_string(), + }; + let failed_open = ChainStep::FailedOpen { + reason: "failed".to_string(), + }; + assert_eq!( + apply_verdict(failure(), OnFailure::Allow, HookEvent::PreToolUse), + failed_open + ); + assert_eq!( + apply_verdict(failure(), OnFailure::Block, HookEvent::PreToolUse), + ChainStep::Denied { + reason: "failed".to_string(), + cause: HookOutcomeCause::HookFailure, + } + ); + assert_eq!( + apply_verdict(failure(), OnFailure::Block, HookEvent::Stop), + failed_open + ); + + for event in [HookEvent::PreToolUse, HookEvent::Stop] { + for mode in [OnFailure::Allow, OnFailure::Block] { + assert_eq!( + apply_verdict(HookVerdict::Allow, mode, event), + ChainStep::Allowed + ); + assert_eq!( + apply_verdict( + HookVerdict::PolicyDeny { + reason: "nope".to_string() + }, + mode, + event + ), + ChainStep::Denied { + reason: "nope".to_string(), + cause: HookOutcomeCause::PolicyDenial, + } + ); + } + } + } + + #[cfg(unix)] + #[tokio::test] + async fn a_blocked_execution_failure_never_leaks_the_command_or_path() { + const SECRET: &str = "s3cr3t-token"; + let spec = serde_json::json!({ + "type": "command", + "command": format!("curl -H 'Authorization: Bearer {SECRET}' \u{0}"), + "on_failure": "block", + }); + let outcome = run_chain(HookEvent::PreToolUse, vec![spec]).await; + let HookDecision::Deny { reason, .. } = &outcome.decision else { + panic!("expected a denial, got {:?}", outcome.decision); + }; + assert_eq!(reason, COMMAND_FAILED_REASON); + for leaked in [SECRET, "curl", "nul byte"] { + assert!(!reason.contains(leaked)); + } + } + + #[cfg(unix)] + #[tokio::test] + async fn subprocess_failures_fail_open_by_default_and_block_when_configured() { + let cases = [ + ( + "spawn failure", + serde_json::json!({"type":"command","command":"echo \u{0}"}), + false, + ), + ( + "timeout", + serde_json::json!({"type":"command","command":"sleep 30","timeout":0}), + false, + ), + ("termination by signal", action("kill -9 $$"), false), + ("an unexpected exit", action("exit 3"), false), + ( + "malformed output", + action(r#"printf '%s' '{"decision" "allow"}'"#), + true, + ), + ("invalid UTF-8", action(r#"printf '\377'"#), true), + ( + "allow JSON with a non-zero exit", + action(r#"printf '%s' '{"decision":"allow"}'; exit 1"#), + false, + ), + ]; + + for (label, mut spec, evaluated) in cases { + let allowed = run_chain(HookEvent::PreToolUse, vec![spec.clone()]).await; + assert_eq!( + allowed.decision, + HookDecision::Allow, + "{label} must fail open by default" + ); + assert_eq!( + allowed.cause, + Some(HookOutcomeCause::HookFailure), + "{label}" + ); + assert_eq!( + allowed.policy_evaluated, evaluated, + "{label}: policy_evaluated is exit 0 or an explicit decision, \ + not whether the hook produced a usable one" + ); + + spec["on_failure"] = serde_json::json!("block"); + let blocked = run_chain(HookEvent::PreToolUse, vec![spec.clone()]).await; + assert!( + matches!(blocked.decision, HookDecision::Deny { .. }), + "{label} must deny under on_failure block, got {:?}", + blocked.decision + ); + assert_eq!( + blocked.cause, + Some(HookOutcomeCause::HookFailure), + "{label}" + ); + assert_eq!( + blocked.policy_evaluated, evaluated, + "{label}: on_failure must not change policy_evaluated" + ); + } + } + + #[cfg(unix)] + #[tokio::test] + async fn a_hook_that_never_received_the_payload_cannot_allow_but_can_still_deny() { + let event = HookEvent::PreToolUse; + + let open = run_chain_with(event, vec![action("exit 0")], oversized_context(event)).await; + assert_eq!( + open.decision, + HookDecision::Allow, + "the default still allows" + ); + assert_eq!( + open.cause, + Some(HookOutcomeCause::HookFailure), + "a hook that never saw the request produced no decision", + ); + assert!(open.policy_evaluated, "it did exit 0"); + + let printed_allow = run_chain_with( + event, + vec![blocking_action(r#"printf '%s' '{"decision":"allow"}'"#)], + oversized_context(event), + ) + .await; + let HookDecision::Deny { reason, .. } = &printed_allow.decision else { + panic!( + "an allow from a hook that never saw the request must not stand under block, got {:?}", + printed_allow.decision + ); + }; + assert_eq!(reason, STDIN_DELIVERY_FAILED_REASON); + assert_eq!(printed_allow.cause, Some(HookOutcomeCause::HookFailure)); + assert!(printed_allow.policy_evaluated, "it did exit 0"); + + let denied = run_chain_with( + event, + vec![action("echo refused by policy >&2; exit 2")], + oversized_context(event), + ) + .await; + assert!(matches!(denied.decision, HookDecision::Deny { .. })); + assert_eq!(denied.cause, Some(HookOutcomeCause::PolicyDenial)); + } + + #[tokio::test] + async fn decisions_are_unchanged_by_on_failure_block() { + let block = r#"printf '%s' '{"decision":"block","reason":"nope"}'"#; + let denied = HookDecision::Deny { + reason: "nope".to_string(), + plugin: "p".to_string(), + }; + + for spec in [action(block), blocking_action(block)] { + let outcome = run_chain(HookEvent::PreToolUse, vec![spec]).await; + assert_eq!(outcome.decision, denied); + assert_eq!(outcome.cause, Some(HookOutcomeCause::PolicyDenial)); + assert!(outcome.policy_evaluated); + } + + for spec in [ + action("cat >/dev/null 2>&1; exit 0"), + blocking_action(r#"cat >/dev/null 2>&1; printf '%s' '{"decision":"allow"}'"#), + ] { + let outcome = run_chain(HookEvent::PreToolUse, vec![spec]).await; + assert_eq!(outcome.decision, HookDecision::Allow); + assert_eq!(outcome.cause, None); + assert!(outcome.policy_evaluated); + } + } + + #[tokio::test] + async fn mixed_chains_preserve_evaluation_and_report_the_final_cause() { + let failure = run_chain( + HookEvent::PreToolUse, + vec![action("exit 0"), blocking_action("exit 3")], + ) + .await; + + assert!(matches!(failure.decision, HookDecision::Deny { .. })); + assert_eq!(failure.cause, Some(HookOutcomeCause::HookFailure)); + assert!(failure.policy_evaluated); + + let denial = run_chain( + HookEvent::PreToolUse, + vec![ + action("exit 3"), + action(r#"printf '%s' '{"decision":"block","reason":"nope"}'"#), + ], + ) + .await; + + assert_eq!( + denial.decision, + HookDecision::Deny { + reason: "nope".to_string(), + plugin: "p".to_string(), + } + ); + assert_eq!(denial.cause, Some(HookOutcomeCause::PolicyDenial)); + assert!(denial.policy_evaluated); + } + + #[tokio::test] + async fn a_block_mode_failure_stops_the_chain() { + let tmp = tempfile::tempdir().unwrap(); + let marker = tmp.path().join("later-ran.txt"); + let mgr = manager_for( + tmp.path(), + HookEvent::PreToolUse, + &[( + "p", + vec![ + blocking_action("exit 3"), + action(&format!("touch {}", marker.to_string_lossy())), + ], + )], + ); + + let outcome = mgr + .emit_blocking_with_outcome( + HookEvent::PreToolUse, + blocking_context(HookEvent::PreToolUse), + ) + .await; + + assert!(matches!(outcome.decision, HookDecision::Deny { .. })); + assert!(!marker.exists(), "the chain must stop at the denial"); + } + + #[tokio::test] + async fn no_matching_rule_allows_without_evaluating_a_policy() { + let tmp = tempfile::tempdir().unwrap(); + let hooks = serde_json::json!({ + "hooks": { + "PreToolUse": [{ + "matcher": "developer__shell", + "hooks": [blocking_action("exit 3")], + }] + } + }) + .to_string(); + let root = write_plugin(tmp.path(), "p", &hooks); + let mgr = make_manager(vec![DiscoveredPlugin { + name: "p".into(), + root, + scope: PluginScope::User, + }]); + + let outcome = mgr + .emit_blocking_with_outcome( + HookEvent::PreToolUse, + HookContext::new(HookEvent::PreToolUse, "s").with_tool("other__tool", None), + ) + .await; + + assert_eq!(outcome.decision, HookDecision::Allow); + assert!(!outcome.policy_evaluated); + assert_eq!(outcome.cause, None); + } + + #[test] + fn serialization_failure_fails_open_by_default_and_blocks_when_configured() { + let outcome = |event: HookEvent, plugins: &[(&str, Vec)]| { + let tmp = tempfile::tempdir().unwrap(); + let manager = manager_for(tmp.path(), event, plugins); + let ctx = blocking_context(event); + let matched = manager.matching_actions(event, &ctx); + assert!(!matched.is_empty(), "the fixture must match an action"); + serialization_failure_outcome(&matched, event) + }; + + let open = outcome( + HookEvent::PreToolUse, + &[("fail-open", vec![action("exit 0")])], + ); + assert_eq!(open.decision, HookDecision::Allow); + assert_eq!(open.cause, Some(HookOutcomeCause::HookFailure)); + assert!(!open.policy_evaluated); + + let closed = outcome( + HookEvent::PreToolUse, + &[ + ("fail-open", vec![action("exit 0")]), + ("fail-closed", vec![blocking_action("exit 0")]), + ], + ); + let HookDecision::Deny { plugin, reason } = &closed.decision else { + panic!("expected a denial, got {:?}", closed.decision); + }; + assert_eq!(plugin, "fail-closed", "the first blocking action owns it"); + assert_eq!(reason, SERIALIZATION_FAILED_REASON); + assert_eq!(closed.cause, Some(HookOutcomeCause::HookFailure)); + assert!(!closed.policy_evaluated); + + let stop = outcome( + HookEvent::Stop, + &[("fail-closed", vec![blocking_action("exit 0")])], + ); + assert_eq!(stop.decision, HookDecision::Allow); + assert!(!stop.policy_evaluated); + } + + fn loaded_on_failure(hooks_json: &str) -> Result { + let tmp = tempfile::tempdir().unwrap(); + let root = write_plugin(tmp.path(), "p", hooks_json); + let loaded = load_hooks_file(&root.join("hooks").join("hooks.json"), "p", &root)?; + Ok(loaded[&HookEvent::PreToolUse][0].actions[0].on_failure()) + } + + #[test] + fn on_failure_values_are_validated() { + let rule = |extra: &str| { + format!( + r#"{{"hooks":{{"PreToolUse":[{{"hooks":[{{"type":"command","command":"echo"{extra}}}]}}]}}}}"# + ) + }; + + for (extra, expected) in [ + ("", Some(OnFailure::Allow)), + (r#","on_failure":"allow""#, Some(OnFailure::Allow)), + (r#","on_failure":"block""#, Some(OnFailure::Block)), + (r#","on_failure":"deny""#, None), + (r#","on_failure":null"#, None), + ] { + match expected { + Some(expected) => assert_eq!(loaded_on_failure(&rule(extra)).unwrap(), expected), + None => assert!(loaded_on_failure(&rule(extra)).is_err()), + } + } + } + + #[test] + fn on_failure_outside_pre_tool_use_is_ignored_not_validated() { + for ignored in [r#""retry""#, "null"] { + let hooks = format!( + r#"{{"hooks":{{ + "Stop":[{{"hooks":[{{"type":"command","command":"echo","on_failure":{ignored}}}]}}], + "PreToolUse":[{{"hooks":[{{"type":"command","command":"echo refused by policy >&2; exit 2","on_failure":"block"}}]}}] + }}}}"# + ); + let tmp = tempfile::tempdir().unwrap(); + let root = write_plugin(tmp.path(), "p", &hooks); + + let loaded = load_hooks_file(&root.join("hooks").join("hooks.json"), "p", &root) + .unwrap_or_else(|err| { + panic!("on_failure {ignored} on Stop must not reject the file: {err:#}") + }); + + let stop = &loaded[&HookEvent::Stop][0].actions; + assert_eq!(stop.len(), 1, "on_failure {ignored}"); + assert_eq!( + stop[0].on_failure(), + OnFailure::Allow, + "a Stop action loads with the default: {ignored}" + ); + + let pre = &loaded[&HookEvent::PreToolUse][0].actions; + assert_eq!( + pre[0].on_failure(), + OnFailure::Block, + "the PreToolUse action keeps its own on_failure: {ignored}" + ); + } + } + + fn load_hooks_json(hooks_json: &str) -> Result>> { + let tmp = tempfile::tempdir().unwrap(); + let root = write_plugin(tmp.path(), "p", hooks_json); + load_hooks_file(&root.join("hooks").join("hooks.json"), "p", &root) + } + + fn surviving_pre_tool_use_command(hooks_json: &str) -> String { + let tmp = tempfile::tempdir().unwrap(); + let root = write_plugin(tmp.path(), "p", hooks_json); + let loaded = load_hooks_file(&root.join("hooks").join("hooks.json"), "p", &root) + .unwrap_or_else(|err| panic!("the file must load: {err:#}")); + let rules = &loaded[&HookEvent::PreToolUse]; + assert_eq!(rules.len(), 1); + assert_eq!(rules[0].actions.len(), 1); + let LoadedAction::Command { command, .. } = &rules[0].actions[0]; + command.clone() + } + + const DENYING_COMMAND: &str = + r#"{"type":"command","command":"echo refused by policy >&2; exit 2"}"#; + + #[test] + fn an_unknown_event_payload_is_opaque_whatever_its_shape() { + let hooks = format!( + r#"{{"hooks":{{ + "SomeFutureEvent":{{"mode":"batch","rules":42,"on_failure":"retry"}}, + "AnotherFutureEvent":null, + "ThirdFutureEvent":[{{"matcher":{{"kind":"glob"}},"hooks":"soon","on_failure":null}}], + "PreToolUse":[{{"hooks":[{DENYING_COMMAND}]}}] + }}}}"# + ); + assert_eq!( + surviving_pre_tool_use_command(&hooks), + "echo refused by policy >&2; exit 2" + ); + } + + #[test] + fn action_selection_preserves_ignored_and_null_shapes() { + let hooks = r#"{"hooks":{"PreToolUse":[{"hooks":[ + {"type":"webhook","command":{"url":"https://example.invalid"},"timeout":"soon","on_failure":{"mode":"retry"},"headers":[1,2]}, + {"type":"command","command":null}, + {"type":"command","timeout":"5s","on_failure":{"mode":"retry"}}, + {"type":null,"command":"echo refused by policy >&2; exit 2"} + ]}]}}"#; + assert_eq!( + surviving_pre_tool_use_command(hooks), + "echo refused by policy >&2; exit 2" + ); + } + + #[test] + fn malformed_selected_configuration_is_rejected() { + for (hooks, expected) in [ + ( + r#"{"hooks":{"PreToolUse":[{"matcher":{"kind":"glob"},"hooks":[{"type":"command","command":"echo"}]}]}}"#, + "expected a string", + ), + ( + r#"{"hooks":{"PreToolUse":[{"hooks":[{"type":42,"command":"echo"}]}]}}"#, + "must be a string", + ), + (r#"{"hooks":{"PreToolUse":{}}}"#, "expected a sequence"), + ( + r#"{"hooks":{"PreToolUse":[{"hooks":["echo"]}]}}"#, + "must be an object", + ), + ( + r#"{"hooks":{"PreToolUse":[{"hooks":[{"type":"command","command":"echo","timeout":"5s"}]}]}}"#, + "expected u64", + ), + ( + r#"{"hooks":{"PreToolUse":[{"hooks":[{"type":"command","command":["echo"]}]}]}}"#, + "must be a string", + ), + ] { + let err = load_hooks_json(hooks).unwrap_err(); + assert!( + format!("{err:#}").contains(expected), + "expected {expected:?}, got {err:#}" + ); + } + } + + #[test] + fn an_invalid_matcher_skips_its_rule_before_on_failure_is_read() { + let hooks = format!( + r#"{{"hooks":{{"PreToolUse":[ + {{"matcher":"[unclosed","hooks":[{{"type":"command","command":"echo","on_failure":"retry"}}]}}, + {{"hooks":[{DENYING_COMMAND}]}} + ]}}}}"# + ); + assert_eq!( + surviving_pre_tool_use_command(&hooks), + "echo refused by policy >&2; exit 2" + ); + } + + #[test] + fn denial_wording_separates_policy_from_failure() { + let deny = |cause: HookOutcomeCause, reason: &str| { + HookChainOutcome { + decision: HookDecision::Deny { + reason: reason.to_string(), + plugin: "guard".to_string(), + }, + policy_evaluated: cause == HookOutcomeCause::PolicyDenial, + cause: Some(cause), + } + .denial() + .expect("a denial reports a refusal") + }; + + let policy = deny(HookOutcomeCause::PolicyDenial, "path is protected"); + assert_eq!(policy.error_type, "hook_denied"); + assert!(policy + .message + .contains("denied by policy hook `guard`: path is protected")); + assert!(policy.message.contains("Do not retry")); + + let failure = deny( + HookOutcomeCause::HookFailure, + "the hook exited with status 3 and no usable decision", + ); + assert_eq!(failure.error_type, "hook_failed"); + assert!(failure.message.contains( + "blocked because policy hook `guard` could not complete: \ + the hook exited with status 3 and no usable decision" + )); + assert!( + !failure.message.contains("Do not retry") + && !failure.message.contains("denied by policy hook"), + "a failure must not be worded as a policy decision: {}", + failure.message + ); + + assert!(HookChainOutcome::allow(true).denial().is_none()); + } + /// `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. @@ -917,16 +1899,43 @@ mod tests { allow.get("reason") ); + assert!( + allow.get("cause").is_none(), + "a clean allow payload must omit cause entirely, got {:?}", + allow.get("cause") + ); + let deny = payload(&HookChainOutcome { decision: HookDecision::Deny { reason: "blocked by test policy".to_string(), plugin: "test-plugin".to_string(), }, policy_evaluated: true, + cause: Some(HookOutcomeCause::PolicyDenial), }); assert_eq!(deny["decision"], "deny"); assert_eq!(deny["blocked_by"], "test-plugin"); assert_eq!(deny["reason"], "blocked by test policy"); + assert_eq!(deny["cause"], "policy_denial"); + + let failed_open = payload(&HookChainOutcome { + decision: HookDecision::Allow, + policy_evaluated: false, + cause: Some(HookOutcomeCause::HookFailure), + }); + assert_eq!(failed_open["decision"], "allow"); + assert_eq!(failed_open["cause"], "hook_failure"); + + let failed_closed = payload(&HookChainOutcome { + decision: HookDecision::Deny { + reason: "the hook exited with status 3 and no usable decision".to_string(), + plugin: "test-plugin".to_string(), + }, + policy_evaluated: false, + cause: Some(HookOutcomeCause::HookFailure), + }); + assert_eq!(failed_closed["decision"], "deny"); + assert_eq!(failed_closed["cause"], "hook_failure"); for value in [&allow, &deny] { let decision = value["decision"].as_str().unwrap(); @@ -1199,7 +2208,7 @@ mod tests { ), ]); - let output = run_command_hook( + let run = run_command_hook( "hook-visible-tool", tmp.path(), "{}", @@ -1209,9 +2218,10 @@ mod tests { .await .unwrap(); - assert!(output.status.success()); + assert!(run.output.status.success()); + assert!(run.stdin_delivered, "a small payload must reach the hook"); assert_eq!( - String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&run.output.stdout), "hook-visible-tool-ran" ); } diff --git a/documentation/docs/guides/context-engineering/hooks.md b/documentation/docs/guides/context-engineering/hooks.md index 62d91e472..f6f590f54 100644 --- a/documentation/docs/guides/context-engineering/hooks.md +++ b/documentation/docs/guides/context-engineering/hooks.md @@ -124,6 +124,7 @@ Place the plugin under a discovered plugin location, such as `~/.agents/plugins/ | `type` | No | Action type. goose currently supports `command`. If omitted, `command` is used. | | `command` | Yes for command hooks | Shell command to run. goose runs it with `sh -c`. | | `timeout` | No | Timeout in seconds for the command. Defaults to 30 seconds. | +| `on_failure` | No | What to do when a `PreToolUse` hook fails: it cannot be executed, times out, does not receive its payload, produces output goose cannot decode, or exits without a decision goose recognizes. `allow` (the default) continues the tool call, `block` denies it. Interpreted only for selected `PreToolUse` command actions and ignored for every other event. See [Blocking a Tool Call](#blocking-a-tool-call). | Use `${PLUGIN_ROOT}` in a command to reference the plugin directory. goose also sets `PLUGIN_ROOT` in the hook command's environment. @@ -172,9 +173,10 @@ When a hook runs, goose writes a JSON payload to the command's stdin. Every payl | `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. | +| `policy_evaluated` | `true` when at least one matching `PreToolUse` hook exited 0 or returned an explicit decision (exit `2`, or `{"decision":"block"}` on stdout), on `PreToolUseResult`. A hook that exits non-zero without a decision, fails to spawn, times out, or whose payload could not be serialized does not count, and neither does the absence of a matching hook. It is an at-least-one value that stays `true` if a later hook fails. It reports that a hook ran to a conclusion, not that the conclusion was usable: read `cause` for that. | | `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`. | +| `reason` | Message for a denial, on `PreToolUseResult` when `decision` is `deny`. For an explicit policy denial, goose derives this from exit-2 stderr or the JSON `reason` field. goose does not bound or redact hook-supplied reasons, so hooks must not include secrets. For a hook failure under `on_failure: block`, goose supplies a bounded framework-generated message that does not include command strings, plugin paths, or payload content. | +| `cause` | Why the chain ended as it did, on `PreToolUseResult`. `policy_denial` means an explicit policy decision blocked the call. `hook_failure` means at least one hook failed to execute, to receive its payload, to produce decodable output, or to answer the decision protocol, and that failure determined or qualified the reported result. Absent only for an allow with no hook failure. | Example payload for a tool event: @@ -246,7 +248,7 @@ A `PreToolUse` hook denies the tool call with either of two signals: - **Exit code `2`** — goose blocks and takes the reason from **stderr**. - **`{"decision":"block","reason":"..."}` on stdout** — goose blocks and takes the reason from the `reason` field. goose checks stdout whenever the exit code is not `2`, so this signal is honored regardless of whether the hook exits `0` or non-zero. -For the stdout signal, stdout must start with `{` and `decision` must be exactly `"block"`; any other value allows the call. If the `reason` is empty, goose substitutes `denied by plugin hook`. +For the stdout signal, stdout must start with `{` and `decision` must be exactly `"block"`. If the `reason` is empty, goose substitutes `denied by plugin hook`. When a `PreToolUse` hook blocks, goose does not run the tool and returns this message to the model: @@ -254,10 +256,84 @@ When a `PreToolUse` hook blocks, goose does not run the tool and returns this me Tool call denied by policy hook ``: . Do not retry; this is a policy denial, not a transient failure. ``` -**A broken hook fails open.** goose blocks only on one of the two deny signals above. If the hook produces neither—it prints nothing or non-`{` stdout and does not exit `2`, or it fails to run at all (a spawn error or a timeout)—the call is logged and allowed. Because the stdout signal is checked independently of the exit code, a hook that prints `{"decision":"block"}` and *then* exits non-zero still blocks; do not rely on a non-zero exit to cancel a block you have already printed. - A `Stop` hook that blocks forces the turn to keep going instead of ending. To prevent a misbehaving hook from looping forever, goose caps the number of consecutive `Stop` blocks; once the cap is hit, goose overrides the hook and ends the turn. Raise the cap with the `GOOSE_STOP_HOOK_BLOCK_CAP` environment variable. +### stdout Is the Decision Channel + +For a blocking event, goose reads a decision out of the hook's exit status and stdout, in this order: + +1. Exit `2` denies, with the reason from stderr. +2. `{"decision":"block"}` on stdout denies, whatever the exit status. +3. Exit `0` with empty stdout allows. +4. Exit `0` with `{"decision":"allow"}` on stdout allows. +5. Anything else means the hook returned no decision. + +Rule 5 covers more than a crash. A stray log line on stdout, truncated or malformed JSON, a JSON array, `{}` with no `decision`, an unrecognized `decision` value, allow JSON printed alongside a non-zero exit, an unexpected exit status, and termination by a signal all read as no decision, as do a spawn error and a timeout. Write logs and diagnostics to stderr and keep stdout for the decision. + +Two further cases read as no decision: + +- **stdout is not valid UTF-8.** goose reads stdout strictly and does not repair invalid bytes, because repairing them can turn malformed output into JSON that parses as an allow. stderr is decoded leniently, since it is only ever shown to a human. +- **goose could not deliver the request.** If the payload cannot be written to the hook's stdin, the hook decided without seeing the request, so an allow from it does not count. An explicit denial still does: goose reads the hook's output before deciding, and a block it managed to print is honored. + +Because the block signal is read independently of the exit code, a hook that prints `{"decision":"block"}` and *then* exits non-zero still blocks. Do not rely on a non-zero exit to cancel a block you have already printed. + +### Choose What Happens When a Hook Fails + +A hook allows cleanly only when goose delivered the payload and the process exited 0 with either empty stdout or a valid allow decision. A failed delivery, output goose cannot use, or a non-zero exit without a denial is a hook failure, not an allow. + +**By default a hook failure is logged and the tool call proceeds.** That is the historical behavior and it stays the default, so a broken hook never wedges a session. + +For a policy you actually depend on, set `on_failure` to `block` on the action. goose then denies the tool call when that hook fails: + +```json title="hooks/hooks.json" +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "developer__shell", + "hooks": [ + { + "type": "command", + "command": "${PLUGIN_ROOT}/scripts/block-sudo.sh", + "on_failure": "block" + } + ] + } + ] + } +} +``` + +A denial caused by a failure is worded differently from a policy denial, so the model and your logs can tell them apart: + +```text +Tool call blocked because policy hook `` could not complete: . That hook is configured to block on failure. +``` + +Three limits are worth knowing: + +- `on_failure` applies to `PreToolUse` only. A `Stop` hook stays fail-open even with `on_failure: block`, so a broken hook can never keep a finished turn from ending. +- `on_failure` never overrides an explicit denial. A hook that explicitly blocks is honored in either mode. It also changes nothing about a clean allow, meaning one where the payload was delivered and the process exited 0 with empty stdout or a valid allow decision. +- On a selected `PreToolUse` command action, any value other than `allow` or `block` is a configuration error, and goose skips that plugin's whole `hooks.json` file with a warning rather than guessing a policy. Other events do not interpret the field. `on_failure` governs what happens when a hook runs and fails, not how configuration load errors are handled. + +### What goose Validates in `hooks.json` + +Unknown event names and unsupported string action types are ignored without their payloads being interpreted, so a plugin can carry configuration for a newer goose without breaking on this one. Recognized event schemas and selected runnable command actions are validated. A malformed selected configuration skips that plugin's `hooks.json` with a warning, and an invalid matcher regex warns and skips only that rule. `on_failure` governs what happens when a hook fails at runtime; it does not make configuration-loading or matcher errors fail closed. + +The `PreToolUseResult` event reports the outcome through `cause`: `policy_denial` for an explicit block, `hook_failure` for an execution or protocol failure, and no `cause` for a clean allow. + +### What `PreToolUseResult` Reports + +`PreToolUseResult` carries the outcome of the `PreToolUse` chain. Its fields are defined once in [Hook Payload](#hook-payload); this section covers only what `on_failure` changes. + +`decision` is the result actually applied to the tool call, so under `on_failure: block` a hook failure shows as `deny`. `cause` is how a subscriber tells the two kinds of denial apart: `policy_denial` when a hook explicitly blocked, `hook_failure` when a hook could not be executed, did not receive its payload, produced output goose could not decode, or answered nothing the decision protocol recognizes. + +`policy_evaluated` answers a different question from `cause` and the two move independently. A hook that exits 0 while printing a stray log line has run to a conclusion, so `policy_evaluated` is `true`, but it produced no usable decision, so `cause` is `hook_failure`. A hook that fails to spawn never ran at all, so `policy_evaluated` stays `false`. + +In a mixed chain both readings survive. If an earlier hook allows cleanly and a later one fails, the event reports `policy_evaluated: true` from the earlier hook and `cause: hook_failure` from the later one, whether the call ends up allowed or denied. + +A hook that fails while `on_failure` is `allow` still emits `decision: "allow"` with `cause: "hook_failure"`, so a monitoring plugin can see that a hook was skipped even though the tool call proceeded. + ### Block a Dangerous Command This `PreToolUse` hook blocks any shell command that uses `sudo`: @@ -437,7 +513,14 @@ Check the following: ### My Hook Timed Out or Failed -Hook failures are logged but do not crash goose or the tool that triggered the hook. If a hook fails or exceeds its timeout, goose logs the failure and continues. This is the fail-open behavior described in [Blocking a Tool Call](#blocking-a-tool-call): to intentionally stop a tool call, a `PreToolUse` hook must emit a clean block signal, not just exit non-zero. +Hook failures are logged but do not crash goose or the tool that triggered the hook. By default, a hook that fails to run, exceeds its timeout, never receives its payload, or exits without a decision goose recognizes is logged and the tool call proceeds. To intentionally stop a tool call, a `PreToolUse` hook must emit a clean block signal, not just exit non-zero. + +Two cases look like a failure but are not what people expect: + +- **The hook printed a log line to stdout.** goose reads stdout as the decision channel, so `echo "checking policy"` makes an otherwise healthy hook read as no decision. Send diagnostics to stderr instead. +- **The hook printed `{"decision":"allow"}` and then exited non-zero.** The exit status contradicts the decision, so goose treats it as no decision. Exit `0` alongside an allow. + +If you would rather a failing hook stop the tool call instead of being ignored, set `on_failure` to `block` on that action. See [Choose What Happens When a Hook Fails](#choose-what-happens-when-a-hook-fails). Set a larger timeout for long-running hooks: