diff --git a/Cargo.lock b/Cargo.lock index de5594c..f2363d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1204,6 +1204,7 @@ dependencies = [ name = "cortex-exec" version = "0.0.6" dependencies = [ + "cortex-common", "cortex-engine", "cortex-protocol", "serde_json", diff --git a/src/cortex-common/src/lib.rs b/src/cortex-common/src/lib.rs index 0f30c44..4cfc023 100644 --- a/src/cortex-common/src/lib.rs +++ b/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, diff --git a/src/cortex-common/src/timeout.rs b/src/cortex-common/src/timeout.rs new file mode 100644 index 0000000..1f7ec37 --- /dev/null +++ b/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); + } +} diff --git a/src/cortex-engine/src/agent/handler.rs b/src/cortex-engine/src/agent/handler.rs index a328af0..f831158 100644 --- a/src/cortex-engine/src/agent/handler.rs +++ b/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) } } diff --git a/src/cortex-engine/src/tools/handlers/batch.rs b/src/cortex-engine/src/tools/handlers/batch.rs index 81ec81f..e3d4542 100644 --- a/src/cortex-engine/src/tools/handlers/batch.rs +++ b/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, + /// 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, } /// Result of a single tool call within the batch. @@ -158,7 +163,7 @@ impl BatchToolHandler { &self, calls: Vec, 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,7 +207,7 @@ impl BatchToolHandler { duration_ms, }, Ok(Err(e)) => BatchCallResult { - tool: tool_name, + tool: tool_name.clone(), index, success: false, result: None, @@ -210,11 +215,15 @@ impl BatchToolHandler { 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) diff --git a/src/cortex-engine/src/tools/handlers/mod.rs b/src/cortex-engine/src/tools/handlers/mod.rs index ccfd955..8e7d3a1 100644 --- a/src/cortex-engine/src/tools/handlers/mod.rs +++ b/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; diff --git a/src/cortex-engine/src/tools/unified_executor.rs b/src/cortex-engine/src/tools/unified_executor.rs index 985fefb..ed083a1 100644 --- a/src/cortex-engine/src/tools/unified_executor.rs +++ b/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( diff --git a/src/cortex-exec/Cargo.toml b/src/cortex-exec/Cargo.toml index 2693c7c..de3b312 100644 --- a/src/cortex-exec/Cargo.toml +++ b/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"] } diff --git a/src/cortex-exec/src/runner.rs b/src/cortex-exec/src/runner.rs index e831324..b2c7b93 100644 --- a/src/cortex-exec/src/runner.rs +++ b/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 { 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); }