fix: improve tool call 'Ask' permission handling with proper logging and error handling

src/cortex-common/src/lib.rs

@@ -18,6 +18,7 @@ pub mod signal_safety;
 pub mod subprocess_env;
 pub mod subprocess_output;
 pub mod text_sanitize;
+pub mod timeout;
 pub mod truncate;
 
 #[cfg(feature = "cli")]
@@ -73,6 +74,11 @@ pub use subprocess_output::{
 pub use text_sanitize::{
     has_control_chars, normalize_code_fences, sanitize_control_chars, sanitize_for_terminal,
 };
+pub use timeout::{
+    DEFAULT_BATCH_TIMEOUT_SECS, DEFAULT_EXEC_TIMEOUT_SECS, DEFAULT_HEALTH_CHECK_TIMEOUT_SECS,
+    DEFAULT_READ_TIMEOUT_SECS, DEFAULT_REQUEST_TIMEOUT_SECS, DEFAULT_SHUTDOWN_TIMEOUT_SECS,
+    DEFAULT_STREAMING_TIMEOUT_SECS,
+};
 pub use truncate::{
     truncate_command, truncate_first_line, truncate_for_display, truncate_id, truncate_id_default,
     truncate_model_name, truncate_with_ellipsis, truncate_with_unicode_ellipsis,

src/cortex-common/src/timeout.rs

@@ -0,0 +1,65 @@
+//! Centralized timeout constants for the Cortex CLI.
+//!
+//! This module provides consistent timeout values used throughout the codebase.
+//! Centralizing these values ensures uniformity and makes it easier to adjust
+//! timeouts across the application.
+
+/// Default timeout for the entire execution in seconds (10 minutes).
+///
+/// This is the maximum time allowed for a complete headless execution,
+/// including all LLM requests and tool executions.
+pub const DEFAULT_EXEC_TIMEOUT_SECS: u64 = 600;
+
+/// Default timeout for a single LLM request in seconds (2 minutes).
+///
+/// This is the maximum time to wait for a single completion request
+/// to the LLM provider.
+pub const DEFAULT_REQUEST_TIMEOUT_SECS: u64 = 120;
+
+/// Default timeout for streaming responses in seconds (5 minutes).
+///
+/// Extended timeout for LLM streaming requests where responses are
+/// delivered incrementally over time.
+pub const DEFAULT_STREAMING_TIMEOUT_SECS: u64 = 300;
+
+/// Default timeout for health check requests in seconds (5 seconds).
+///
+/// Short timeout used for quick health check endpoints.
+pub const DEFAULT_HEALTH_CHECK_TIMEOUT_SECS: u64 = 5;
+
+/// Default timeout for graceful shutdown in seconds (30 seconds).
+///
+/// Maximum time to wait for in-flight operations to complete during
+/// shutdown before forcing termination.
+pub const DEFAULT_SHUTDOWN_TIMEOUT_SECS: u64 = 30;
+
+/// Default timeout for batch execution in seconds (5 minutes).
+///
+/// Maximum time allowed for executing a batch of parallel tool calls.
+pub const DEFAULT_BATCH_TIMEOUT_SECS: u64 = 300;
+
+/// Default timeout for individual read operations in seconds (30 seconds).
+///
+/// Timeout for individual read operations to prevent hangs when
+/// Content-Length doesn't match actual body size.
+pub const DEFAULT_READ_TIMEOUT_SECS: u64 = 30;
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_timeout_values_are_reasonable() {
+        // Exec timeout should be greater than request timeout
+        assert!(DEFAULT_EXEC_TIMEOUT_SECS > DEFAULT_REQUEST_TIMEOUT_SECS);
+
+        // Streaming timeout should be greater than request timeout
+        assert!(DEFAULT_STREAMING_TIMEOUT_SECS > DEFAULT_REQUEST_TIMEOUT_SECS);
+
+        // Health check should be short
+        assert!(DEFAULT_HEALTH_CHECK_TIMEOUT_SECS <= 10);
+
+        // Batch timeout should be reasonable
+        assert!(DEFAULT_BATCH_TIMEOUT_SECS >= 60);
+    }
+}

src/cortex-engine/src/agent/handler.rs

@@ -68,9 +68,24 @@ impl MessageHandler {
 
     /// Validates and handles tool execution based on agent profile permissions.
     ///
-    /// If a tool is 'deny', returns an error immediately.
-    /// If 'ask', triggers a protocol 'ExecApprovalRequest' via the event sender.
-    /// Returns Ok(true) if execution should proceed, Ok(false) if waiting for approval.
+    /// This method checks the permission level for a tool call and determines
+    /// whether execution should proceed immediately, wait for user approval,
+    /// or be denied entirely.
+    ///
+    /// # Return Value Semantics
+    ///
+    /// - `Ok(true)` - The tool has 'Allow' permission; caller should proceed with execution
+    /// - `Ok(false)` - The tool has 'Ask' permission; caller should NOT proceed immediately.
+    ///   A `ToolCallPending` event has been emitted, and the session handler will convert
+    ///   this to a protocol `ExecApprovalRequest`. The caller should wait for user approval
+    ///   before retrying execution.
+    /// - `Err(_)` - The tool has 'Deny' permission; execution is forbidden for this profile
+    ///
+    /// # Errors
+    ///
+    /// Returns `CortexError::Internal` if:
+    /// - The tool is explicitly denied by the profile
+    /// - Failed to send the approval request event (for 'Ask' permission)
     pub async fn handle_tool_execution(
         &self,
         profile: &AgentProfile,
@@ -95,15 +110,29 @@ impl MessageHandler {
                 Err(CortexError::Internal(error_msg))
             }
             ToolPermission::Ask => {
-                // Trigger protocol ExecApprovalRequest by emitting ToolCallPending event
-                // This will be picked up by the session handler and converted to a protocol event
-                let _ = event_tx.send(AgentEvent::ToolCallPending {
-                    id: tool_call.id.clone(),
-                    name: tool_call.function.name.clone(),
-                    arguments: tool_call.function.arguments.clone(),
-                    risk_level: RiskLevel::Medium, // Tools marked as 'ask' are considered medium risk by default
-                });
+                // The 'Ask' permission requires user approval before execution.
+                // We emit a ToolCallPending event and return Ok(false) to indicate
+                // that the caller should NOT proceed with execution immediately.
+                // The session handler will convert this to a protocol ExecApprovalRequest
+                // and wait for user approval before retrying.
+                tracing::debug!(
+                    tool = %tool_call.function.name,
+                    id = %tool_call.id,
+                    "Tool execution requires user approval"
+                );
+
+                event_tx
+                    .send(AgentEvent::ToolCallPending {
+                        id: tool_call.id.clone(),
+                        name: tool_call.function.name.clone(),
+                        arguments: tool_call.function.arguments.clone(),
+                        risk_level: RiskLevel::Medium, // Tools marked as 'ask' are considered medium risk by default
+                    })
+                    .map_err(|e| {
+                        CortexError::Internal(format!("Failed to send approval request: {}", e))
+                    })?;
 
+                // Return false to indicate caller should wait for approval
                 Ok(false)
             }
         }

src/cortex-engine/src/tools/handlers/batch.rs

@@ -20,8 +20,9 @@ use crate::tools::spec::{ToolDefinition, ToolHandler, ToolResult};
 /// Maximum number of tools that can be executed in a batch.
 pub const MAX_BATCH_SIZE: usize = 10;
 
-/// Default timeout for batch execution in seconds.
-pub const DEFAULT_BATCH_TIMEOUT_SECS: u64 = 300;
+/// Default timeout for individual tool execution in seconds.
+/// This prevents a single tool from consuming the entire batch timeout.
+pub const DEFAULT_TOOL_TIMEOUT_SECS: u64 = 60;
 
 /// Tools that cannot be called within a batch (prevent recursion).
 /// Note: Task is now allowed to enable parallel task execution.
@@ -45,6 +46,10 @@ pub struct BatchToolArgs {
     /// Optional timeout in seconds for the entire batch (default: 300s).
     #[serde(default)]
     pub timeout_secs: Option<u64>,
+    /// Optional timeout in seconds for individual tools (default: 60s).
+    /// This prevents a single tool from consuming the entire batch timeout.
+    #[serde(default)]
+    pub tool_timeout_secs: Option<u64>,
 }
 
 /// Result of a single tool call within the batch.
@@ -158,7 +163,7 @@ impl BatchToolHandler {
         &self,
         calls: Vec<BatchToolCall>,
         context: &ToolContext,
-        timeout_duration: Duration,
+        tool_timeout: Duration,
     ) -> BatchResult {
         let start_time = Instant::now();
         let results = Arc::new(Mutex::new(Vec::with_capacity(calls.len())));
@@ -176,9 +181,9 @@ impl BatchToolHandler {
                 async move {
                     let call_start = Instant::now();
 
-                    // Execute with per-call timeout (use batch timeout for each call)
+                    // Execute with per-tool timeout to prevent single tools from blocking others
                     let execution_result = timeout(
-                        timeout_duration,
+                        tool_timeout,
                         executor.execute_tool(&call.tool, call.arguments, &ctx),
                     )
                     .await;
@@ -202,19 +207,23 @@ impl BatchToolHandler {
                             duration_ms,
                         },
                         Ok(Err(e)) => BatchCallResult {
-                            tool: tool_name,
+                            tool: tool_name.clone(),
                             index,
                             success: false,
                             result: None,
                             error: Some(format!("Execution error: {}", e)),
                             duration_ms,
                         },
                         Err(_) => BatchCallResult {
-                            tool: tool_name,
+                            tool: tool_name.clone(),
                             index,
                             success: false,
                             result: None,
-                            error: Some(format!("Timeout after {}s", timeout_duration.as_secs())),
+                            error: Some(format!(
+                                "Tool '{}' timed out after {}s",
+                                tool_name,
+                                tool_timeout.as_secs()
+                            )),
                             duration_ms,
                         },
                     };
@@ -328,13 +337,13 @@ impl ToolHandler for BatchToolHandler {
         // Validate calls
         self.validate_calls(&args.calls)?;
 
-        // Determine timeout
-        let timeout_secs = args.timeout_secs.unwrap_or(DEFAULT_BATCH_TIMEOUT_SECS);
-        let timeout_duration = Duration::from_secs(timeout_secs);
+        // Determine per-tool timeout (prevents single tool from blocking others)
+        let tool_timeout_secs = args.tool_timeout_secs.unwrap_or(DEFAULT_TOOL_TIMEOUT_SECS);
+        let tool_timeout = Duration::from_secs(tool_timeout_secs);
 
         // Execute all tools in parallel
         let batch_result = self
-            .execute_parallel(args.calls, context, timeout_duration)
+            .execute_parallel(args.calls, context, tool_timeout)
             .await;
 
         // Format output
@@ -384,6 +393,12 @@ pub fn batch_tool_definition() -> ToolDefinition {
                     "description": "Optional timeout in seconds for the entire batch execution (default: 300)",
                     "minimum": 1,
                     "maximum": 600
+                },
+                "tool_timeout_secs": {
+                    "type": "integer",
+                    "description": "Optional timeout in seconds for individual tool execution (default: 60). Prevents a single tool from consuming the entire batch timeout.",
+                    "minimum": 1,
+                    "maximum": 300
                 }
             }
         }),
@@ -409,6 +424,7 @@ pub async fn execute_batch(
             })
             .collect(),
         timeout_secs: None,
+        tool_timeout_secs: None,
     };
 
     let arguments = serde_json::to_value(args)

src/cortex-engine/src/tools/handlers/mod.rs

@@ -23,9 +23,10 @@ mod web_search;
 pub use apply_patch::ApplyPatchHandler;
 pub use batch::{
     BatchCallResult, BatchParams, BatchResult, BatchToolArgs, BatchToolCall, BatchToolExecutor,
-    BatchToolHandler, DEFAULT_BATCH_TIMEOUT_SECS, DISALLOWED_TOOLS, LegacyBatchToolCall,
-    MAX_BATCH_SIZE, batch_tool_definition, execute_batch,
+    BatchToolHandler, DISALLOWED_TOOLS, LegacyBatchToolCall, MAX_BATCH_SIZE, batch_tool_definition,
+    execute_batch,
 };
+pub use cortex_common::DEFAULT_BATCH_TIMEOUT_SECS;
 pub use create_agent::CreateAgentHandler;
 pub use edit_file::PatchHandler;
 

src/cortex-engine/src/tools/unified_executor.rs

@@ -466,6 +466,7 @@ impl UnifiedToolExecutor {
                 Ok(BatchToolArgs {
                     calls,
                     timeout_secs: arguments.get("timeout_secs").and_then(|v| v.as_u64()),
+                    tool_timeout_secs: arguments.get("tool_timeout_secs").and_then(|v| v.as_u64()),
                 })
             } else {
                 Err(CortexError::InvalidInput(

src/cortex-exec/Cargo.toml

@@ -13,6 +13,7 @@ path = "src/lib.rs"
 workspace = true
 
 [dependencies]
+cortex-common = { workspace = true }
 cortex-engine = { workspace = true }
 cortex-protocol = { workspace = true }
 tokio = { workspace = true, features = ["full"] }

src/cortex-exec/src/runner.rs

@@ -14,6 +14,7 @@ use std::time::Duration;
 
 use tokio_stream::StreamExt;
 
+use cortex_common::{DEFAULT_EXEC_TIMEOUT_SECS, DEFAULT_REQUEST_TIMEOUT_SECS};
 use cortex_engine::{
     Config, ConversationManager, CortexError,
     client::{
@@ -26,12 +27,6 @@ use cortex_protocol::ConversationId;
 
 use crate::output::{OutputFormat, OutputWriter};
 
-/// Default timeout for the entire execution (10 minutes).
-const DEFAULT_TIMEOUT_SECS: u64 = 600;
-
-/// Default timeout for a single LLM request (2 minutes).
-const DEFAULT_REQUEST_TIMEOUT_SECS: u64 = 120;
-
 /// Maximum retries for transient errors.
 const MAX_RETRIES: usize = 3;
 
@@ -75,7 +70,7 @@ impl Default for ExecOptions {
             output_format: OutputFormat::Text,
             full_auto: false,
             max_turns: Some(10),
-            timeout_secs: Some(DEFAULT_TIMEOUT_SECS),
+            timeout_secs: Some(DEFAULT_EXEC_TIMEOUT_SECS),
             request_timeout_secs: Some(DEFAULT_REQUEST_TIMEOUT_SECS),
             sandbox: true,
             system_prompt: None,
@@ -267,7 +262,7 @@ impl ExecRunner {
     /// Run the execution with full timeout enforcement.
     pub async fn run(&mut self) -> Result<ExecResult, CortexError> {
         let timeout =
-            Duration::from_secs(self.options.timeout_secs.unwrap_or(DEFAULT_TIMEOUT_SECS));
+            Duration::from_secs(self.options.timeout_secs.unwrap_or(DEFAULT_EXEC_TIMEOUT_SECS));
 
         // Wrap the entire execution in a timeout
         match tokio::time::timeout(timeout, self.run_inner()).await {
@@ -766,7 +761,7 @@ mod tests {
         assert!(opts.prompt.is_empty());
         assert!(opts.sandbox);
         assert_eq!(opts.max_turns, Some(10));
-        assert_eq!(opts.timeout_secs, Some(DEFAULT_TIMEOUT_SECS));
+        assert_eq!(opts.timeout_secs, Some(DEFAULT_EXEC_TIMEOUT_SECS));
         assert!(!opts.full_auto);
         assert!(opts.streaming);
     }