diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index 5813b5381..2f920b3fc 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -166,6 +166,9 @@ pub struct EngineConfig { /// Tool restriction from custom slash command frontmatter. /// `None` means the current turn may use the normal tool set. pub allowed_tools: Option>, + /// Hook executor for control-plane hooks. + /// `ToolCallBefore` hooks may deny a tool call with exit code 2. + pub hook_executor: Option>, /// Resolved BCP-47 locale tag (e.g. `"en"`, `"zh-Hans"`, `"ja"`) /// for the `## Environment` block in the system prompt. The /// caller resolves this from `Settings` once at engine @@ -237,6 +240,7 @@ impl Default for EngineConfig { strict_tool_mode: false, goal_objective: None, allowed_tools: None, + hook_executor: None, locale_tag: "en".to_string(), workshop: None, search_provider: crate::config::SearchProvider::default(), @@ -650,6 +654,7 @@ impl Engine { translation_enabled, show_thinking, allowed_tools, + hook_executor, } => { self.handle_send_message( content, @@ -666,6 +671,7 @@ impl Engine { translation_enabled, show_thinking, allowed_tools, + hook_executor, ) .await; } @@ -884,6 +890,7 @@ impl Engine { self.config.translation_enabled, self.config.show_thinking, self.config.allowed_tools.clone(), + self.config.hook_executor.clone(), ) .await; } @@ -1008,6 +1015,7 @@ impl Engine { translation_enabled: bool, show_thinking: bool, allowed_tools: Option>, + hook_executor: Option>, ) { // Reset cancel token for fresh turn (in case previous was cancelled) self.reset_cancel_token(); @@ -1114,6 +1122,7 @@ impl Engine { ); } self.config.allowed_tools = allowed_tools; + self.config.hook_executor = hook_executor; self.session.reasoning_effort = reasoning_effort; self.session.reasoning_effort_auto = reasoning_effort_auto; self.session.auto_model = auto_model; diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index 9a3245e16..7b6a8faab 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -1255,13 +1255,17 @@ impl Engine { ))); } - if !command_allows_tool(self.config.allowed_tools.as_deref(), &tool_name) { + if blocked_error.is_none() + && !command_allows_tool(self.config.allowed_tools.as_deref(), &tool_name) + { blocked_error = Some(ToolError::permission_denied(format!( "Tool '{tool_name}' is not in the allowed-tools list for the current command" ))); } - if !caller_allowed_for_tool(tool_caller.as_ref(), tool_def) { + if blocked_error.is_none() + && !caller_allowed_for_tool(tool_caller.as_ref(), tool_def) + { blocked_error = Some(ToolError::permission_denied(format!( "Tool '{tool_name}' does not allow caller '{}'", caller_type_for_tool_use(tool_caller.as_ref()) @@ -1281,6 +1285,68 @@ impl Engine { ))); } + if blocked_error.is_none() + && let Some(hook_executor) = self.config.hook_executor.as_ref() + && hook_executor.has_hooks_for_event(crate::hooks::HookEvent::ToolCallBefore) + { + // Warn if any ToolCallBefore hook is configured as background + // — background hooks return exit_code: None immediately, so + // the denial check (exit_code == Some(2)) can never match. + if hook_executor + .has_background_hooks_for_event(crate::hooks::HookEvent::ToolCallBefore) + { + tracing::warn!( + "ToolCallBefore hook(s) configured with background=true — \ + background hooks cannot deny tool calls because they exit \ + immediately with no result" + ); + } + + let hook_context = crate::hooks::HookContext::new() + .with_tool_name(&tool_name) + .with_tool_args(&tool_input) + .with_mode(&format!("{mode:?}")) + .with_workspace(self.session.workspace.clone()) + .with_model(&self.config.model) + .with_session_id(&self.session.id); + // Run hooks off the Tokio worker thread: `execute()` calls + // `child.wait_timeout()` which is a blocking syscall that + // would stall all other async tasks on this thread. + let executor = hook_executor.clone(); + let hook_results = tokio::task::spawn_blocking(move || { + executor.execute(crate::hooks::HookEvent::ToolCallBefore, &hook_context) + }) + .await + .unwrap_or_else(|join_err| { + tracing::error!("Hook executor task panicked: {join_err}"); + Vec::new() + }); + if let Some(denial) = hook_results + .iter() + .find(|result| result.exit_code == Some(2)) + { + let reason = denial + .stdout + .trim() + .lines() + .next() + .filter(|line| !line.is_empty()) + .or_else(|| { + denial + .stderr + .trim() + .lines() + .next() + .filter(|line| !line.is_empty()) + }) + .or(denial.error.as_deref()) + .unwrap_or("ToolCallBefore hook denied tool execution"); + blocked_error = Some(ToolError::permission_denied(format!( + "ToolCallBefore hook denied tool '{tool_name}': {reason}" + ))); + } + } + if McpPool::is_mcp_tool(&tool_name) { read_only = mcp_tool_is_read_only(&tool_name); supports_parallel = mcp_tool_is_parallel_safe(&tool_name); @@ -2514,4 +2580,105 @@ mod tests { let allowed = vec!["read_file".to_string()]; assert!(command_allows_tool(Some(&allowed), &tool_name)); } + + #[test] + fn hook_gate_denies_with_exit_code_2() { + use crate::hooks::{Hook, HookContext, HookEvent, HookExecutor, HooksConfig}; + + let deny_cmd = if cfg!(windows) { "exit /b 2" } else { "exit 2" }; + let config = HooksConfig { + enabled: true, + hooks: vec![Hook::new(HookEvent::ToolCallBefore, deny_cmd)], + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, std::path::PathBuf::from(".")); + let ctx = HookContext::new() + .with_tool_name("exec_shell") + .with_tool_args(&serde_json::json!({})); + let results = executor.execute(HookEvent::ToolCallBefore, &ctx); + + assert_eq!(results.len(), 1); + assert_eq!(results[0].exit_code, Some(2)); + } + + #[test] + fn hook_gate_allows_with_exit_code_0() { + use crate::hooks::{Hook, HookContext, HookEvent, HookExecutor, HooksConfig}; + + let allow_cmd = if cfg!(windows) { "exit /b 0" } else { "exit 0" }; + let config = HooksConfig { + enabled: true, + hooks: vec![Hook::new(HookEvent::ToolCallBefore, allow_cmd)], + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, std::path::PathBuf::from(".")); + let ctx = HookContext::new() + .with_tool_name("read_file") + .with_tool_args(&serde_json::json!({})); + let results = executor.execute(HookEvent::ToolCallBefore, &ctx); + + assert_eq!(results.len(), 1); + assert_eq!(results[0].exit_code, Some(0)); + assert!(results[0].success); + } + + #[test] + fn hook_gate_failure_exit_code_1_is_not_denial() { + use crate::hooks::{Hook, HookContext, HookEvent, HookExecutor, HooksConfig}; + + let fail_cmd = if cfg!(windows) { "exit /b 1" } else { "exit 1" }; + let config = HooksConfig { + enabled: true, + hooks: vec![Hook::new(HookEvent::ToolCallBefore, fail_cmd)], + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, std::path::PathBuf::from(".")); + let ctx = HookContext::new() + .with_tool_name("write_file") + .with_tool_args(&serde_json::json!({})); + let results = executor.execute(HookEvent::ToolCallBefore, &ctx); + + assert_eq!(results.len(), 1); + assert_eq!(results[0].exit_code, Some(1)); + assert_ne!(results[0].exit_code, Some(2)); + } + + #[test] + fn hook_gate_no_hooks_returns_no_results() { + use crate::hooks::{HookContext, HookEvent, HookExecutor, HooksConfig}; + + let config = HooksConfig { + enabled: true, + hooks: vec![], + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, std::path::PathBuf::from(".")); + let ctx = HookContext::new().with_tool_name("grep_files"); + let results = executor.execute(HookEvent::ToolCallBefore, &ctx); + + assert!(results.is_empty()); + } + + #[test] + fn hook_gate_denial_reason_can_come_from_stdout() { + use crate::hooks::{Hook, HookContext, HookEvent, HookExecutor, HooksConfig}; + + let deny_cmd = if cfg!(windows) { + "echo Tool blocked by security policy & exit /b 2" + } else { + "echo 'Tool blocked by security policy' && exit 2" + }; + let config = HooksConfig { + enabled: true, + hooks: vec![Hook::new(HookEvent::ToolCallBefore, deny_cmd)], + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, std::path::PathBuf::from(".")); + let ctx = HookContext::new().with_tool_name("exec_shell"); + let results = executor.execute(HookEvent::ToolCallBefore, &ctx); + + assert_eq!(results.len(), 1); + assert_eq!(results[0].exit_code, Some(2)); + assert!(results[0].stdout.contains("security")); + } } diff --git a/crates/tui/src/core/ops.rs b/crates/tui/src/core/ops.rs index 87f479457..df6f0aa0d 100644 --- a/crates/tui/src/core/ops.rs +++ b/crates/tui/src/core/ops.rs @@ -35,6 +35,9 @@ pub enum Op { /// Tool restriction from custom slash command frontmatter. /// `None` means the current turn may use the normal tool set. allowed_tools: Option>, + /// Hook executor for control-plane hooks. + /// `ToolCallBefore` hooks may deny a tool call with exit code 2. + hook_executor: Option>, }, /// Cancel the current request diff --git a/crates/tui/src/hooks.rs b/crates/tui/src/hooks.rs index e5715dc2c..6450d9e6a 100644 --- a/crates/tui/src/hooks.rs +++ b/crates/tui/src/hooks.rs @@ -551,6 +551,23 @@ impl HookExecutor { self.config.enabled && self.config.hooks.iter().any(|h| h.event == event) } + /// Check if there are any background hooks configured for a specific event. + /// + /// Background hooks fire and forget — their `exit_code` is always `None`, + /// so they cannot deny tool calls. This is a known limitation; the check + /// is used to warn operators when a `ToolCallBefore` hook is configured + /// as background but expects to block a tool. + #[must_use] + pub fn has_background_hooks_for_event(&self, event: HookEvent) -> bool { + if !self.config.enabled { + return false; + } + self.config + .hooks + .iter() + .any(|h| h.event == event && h.background) + } + /// Run configured `message_submit` hooks as a mutable submit pipeline. /// /// This is deliberately separate from [`Self::execute`]: most hook events diff --git a/crates/tui/src/main.rs b/crates/tui/src/main.rs index 9feaaac46..47468b73c 100644 --- a/crates/tui/src/main.rs +++ b/crates/tui/src/main.rs @@ -5379,6 +5379,7 @@ async fn run_exec_agent( strict_tool_mode: config.strict_tool_mode.unwrap_or(false), goal_objective: None, allowed_tools: None, + hook_executor: None, locale_tag: crate::localization::resolve_locale(&settings.locale) .tag() .to_string(), @@ -5435,6 +5436,7 @@ async fn run_exec_agent( model: effective_model.clone(), goal_objective: None, allowed_tools: None, + hook_executor: None, reasoning_effort: effective_reasoning_effort, reasoning_effort_auto: auto_model, auto_model, diff --git a/crates/tui/src/runtime_threads.rs b/crates/tui/src/runtime_threads.rs index 51f79922c..39c009ed7 100644 --- a/crates/tui/src/runtime_threads.rs +++ b/crates/tui/src/runtime_threads.rs @@ -1654,6 +1654,7 @@ impl RuntimeThreadManager { translation_enabled: false, show_thinking, allowed_tools: None, + hook_executor: None, approval_mode: if auto_approve { crate::tui::approval::ApprovalMode::Auto } else { @@ -2020,6 +2021,7 @@ impl RuntimeThreadManager { strict_tool_mode: self.config.strict_tool_mode.unwrap_or(false), goal_objective: None, allowed_tools: None, + hook_executor: None, locale_tag: crate::localization::resolve_locale(&settings.locale) .tag() .to_string(), diff --git a/crates/tui/src/tui/tool_routing.rs b/crates/tui/src/tui/tool_routing.rs index e0e35bdd0..f76e1cd16 100644 --- a/crates/tui/src/tui/tool_routing.rs +++ b/crates/tui/src/tui/tool_routing.rs @@ -22,19 +22,11 @@ pub(super) fn handle_tool_call_started( name: &str, input: &serde_json::Value, ) { - // #455 (observer-only): fire `tool_call_before` hooks here, before - // any UI bookkeeping. Hooks are read-only observers in this slice - // — they can log, notify, or audit, but cannot mutate the args. - // Fast-path skip when no hooks are configured so per-tool - // dispatch doesn't pay for context construction in the common - // case (most users have no hooks). - if app.hooks.has_hooks_for_event(HookEvent::ToolCallBefore) { - let context = app - .base_hook_context() - .with_tool_name(name) - .with_tool_args(input); - let _ = app.execute_hooks(HookEvent::ToolCallBefore, &context); - } + // #2511: ToolCallBefore gate moved to turn-loop planning loop + // (Engine::handle_deepseek_turn). Removing observer-only firing + // here to avoid double-firing hooks for each tool call. + // Hooks that need observation can configure ToolCallBefore on + // the turn-loop gate — it processes the denial (exit code 2). let id = id.to_string(); diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index e92a2a056..b5fa3f081 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -503,6 +503,13 @@ pub async fn run_tui(config: &Config, options: TuiOptions) -> Result<()> { .shell_manager .clone() .unwrap_or_else(|| crate::tools::shell::new_shared_shell_manager(app.workspace.clone())); + // #2511: ensure hook_executor is initialized for fresh sessions — it is + // only set by apply_workspace_runtime_state (session resume / workspace + // switch), so a brand-new session would otherwise leave it None and both + // exec_shell shell_env hooks and ToolCallBefore gate would silently no-op. + if app.runtime_services.hook_executor.is_none() { + app.runtime_services.hook_executor = Some(std::sync::Arc::new(app.hooks.clone())); + } app.runtime_services = RuntimeToolServices { shell_manager: Some(shell_manager), task_manager: Some(task_manager.clone()), @@ -511,8 +518,8 @@ pub async fn run_tui(config: &Config, options: TuiOptions) -> Result<()> { active_task_id: None, active_thread_id: None, // #456: plumb the App's HookExecutor so `exec_shell` can surface - // the configured `shell_env` hooks. Wrapped in Arc once and shared. - hook_executor: Some(std::sync::Arc::new(app.hooks.clone())), + // the configured `shell_env` hooks. Clone the shared Arc. + hook_executor: app.runtime_services.hook_executor.clone(), handle_store: app.runtime_services.handle_store.clone(), rlm_sessions: app.runtime_services.rlm_sessions.clone(), }; @@ -754,6 +761,7 @@ fn build_engine_config(app: &App, config: &Config) -> EngineConfig { ), max_spawn_depth: crate::tools::subagent::DEFAULT_MAX_SPAWN_DEPTH, allowed_tools: app.active_allowed_tools.clone(), + hook_executor: app.runtime_services.hook_executor.clone(), network_policy: config.network.clone().map(|toml_cfg| { crate::network_policy::NetworkPolicyDecider::with_default_audit(toml_cfg.into_runtime()) }), @@ -4706,6 +4714,7 @@ async fn dispatch_user_message( translation_enabled: app.translation_enabled, show_thinking: app.show_thinking, allowed_tools: app.active_allowed_tools.clone(), + hook_executor: app.runtime_services.hook_executor.clone(), }) .await { diff --git a/crates/tui/src/utils.rs b/crates/tui/src/utils.rs index 756c483db..3cb7972a2 100644 --- a/crates/tui/src/utils.rs +++ b/crates/tui/src/utils.rs @@ -253,7 +253,7 @@ fn browser_open_command(url: &str) -> Result { { let mut cmd = Command::new("cmd"); cmd.args(["/C", "start", "", url]); - return Ok(cmd); + Ok(cmd) } #[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "windows")))]