From 796e95caa68917cb45f153834ee120956fac1f04 Mon Sep 17 00:00:00 2001 From: Paulo Aboim Pinto Date: Mon, 1 Jun 2026 17:40:13 +0200 Subject: [PATCH] fix: address PR #2511 review comments --- crates/tui/src/core/engine/turn_loop.rs | 58 +++++++++++++++---------- crates/tui/src/hooks.rs | 14 ++++++ crates/tui/src/tui/ui.rs | 10 ++--- 3 files changed, 55 insertions(+), 27 deletions(-) diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index 9d0bdd91..3416b2ef 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -1255,16 +1255,50 @@ 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 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()) + ))); + } + + if blocked_error.is_none() + && tool_def.is_none() + && !McpPool::is_mcp_tool(&tool_name) + && tool_name != CODE_EXECUTION_TOOL_NAME + && tool_name != JS_EXECUTION_TOOL_NAME + && !is_tool_search_tool(&tool_name) + { + blocked_error = Some(ToolError::not_available(missing_tool_error_message( + &tool_name, + &tool_catalog, + ))); + } + 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) @@ -1300,26 +1334,6 @@ impl Engine { } } - if !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()) - ))); - } - - if blocked_error.is_none() - && tool_def.is_none() - && !McpPool::is_mcp_tool(&tool_name) - && tool_name != CODE_EXECUTION_TOOL_NAME - && tool_name != JS_EXECUTION_TOOL_NAME - && !is_tool_search_tool(&tool_name) - { - blocked_error = Some(ToolError::not_available(missing_tool_error_message( - &tool_name, - &tool_catalog, - ))); - } - 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); @@ -2654,4 +2668,4 @@ mod tests { assert_eq!(results[0].exit_code, Some(2)); assert!(results[0].stdout.contains("security")); } -} +} \ No newline at end of file diff --git a/crates/tui/src/hooks.rs b/crates/tui/src/hooks.rs index e5715dc2..68dd6e4a 100644 --- a/crates/tui/src/hooks.rs +++ b/crates/tui/src/hooks.rs @@ -551,6 +551,20 @@ 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/tui/ui.rs b/crates/tui/src/tui/ui.rs index d0dc6406..cf92cdcc 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -511,8 +511,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,7 +754,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: Some(std::sync::Arc::new(app.hooks.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()) }), @@ -4707,7 +4707,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: Some(std::sync::Arc::new(app.hooks.clone())), + hook_executor: app.runtime_services.hook_executor.clone(), }) .await { @@ -8875,4 +8875,4 @@ fn parse_semver(v: &str) -> Option<(u32, u32, u32)> { } #[cfg(test)] -mod tests; +mod tests; \ No newline at end of file