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

[echobt]

Summary

This PR improves the handling of the 'Ask' permission case in tool call validation. The handle_tool_execution method now has clearer documentation about return value semantics, proper logging when user approval is requested, and error handling for the event send operation.

Changes

• Add comprehensive documentation explaining Ok(true)/Ok(false)/Err semantics
• Add debug logging when tool execution requires user approval
• Handle potential send failure instead of ignoring with let _ =
• Clarify the intended workflow for the session handler

Verification

cargo check -p cortex-engine
cargo test -p cortex-engine --lib -- handler

[greptile-apps]

Greptile Overview

Greptile Summary

This PR makes two primary improvements to the codebase:

1. Enhanced 'Ask' Permission Handling (handler.rs)

• Added comprehensive documentation explaining the return value semantics (Ok(true), Ok(false), Err)
• Added debug logging when tool execution requires user approval
• Changed from ignoring send failures (let _ =) to proper error handling with .map_err()
• Improved code comments to clarify the workflow for session handlers

2. Timeout Constant Centralization

• Created new cortex-common/src/timeout.rs module to centralize all timeout constants
• Moved timeout constants from individual modules to the shared common module
• Maintained backward compatibility by re-exporting DEFAULT_BATCH_TIMEOUT_SECS from the handlers module
• Added comprehensive documentation for each timeout constant
• Included unit tests to validate timeout value relationships

Additional Enhancements

• Added per-tool timeout support in batch execution (tool_timeout_secs parameter)
• Prevents single tools from consuming the entire batch timeout
• Improved timeout error messages to include tool name for better debugging
• Fixed minor issue where tool_name wasn't being cloned in error paths (lines 210, 218 in batch.rs)

All changes follow Rust best practices with proper error handling, clear documentation, and maintain backward compatibility.

Confidence Score: 5/5

• This PR is safe to merge with no issues found
• All changes are well-documented, maintain backward compatibility, include proper error handling, and follow Rust best practices. The refactoring centralizes timeout constants without changing behavior, and the improved Ask permission handling adds necessary error handling that was previously missing.
• No files require special attention

Important Files Changed

Filename	Overview
src/cortex-engine/src/agent/handler.rs	Improved Ask permission handling with comprehensive documentation, debug logging, and proper error handling for event send operations
src/cortex-common/src/timeout.rs	New module centralizing timeout constants with comprehensive documentation and validation tests
src/cortex-engine/src/tools/handlers/batch.rs	Added per-tool timeout support to prevent single tools from blocking batch execution, renamed constant for clarity
src/cortex-exec/src/runner.rs	Updated to use centralized timeout constants from cortex-common

Sequence Diagram

sequenceDiagram
    participant Caller as Tool Executor
    participant Handler as MessageHandler
    participant EventTx as Event Channel
    participant Session as Session Handler
    participant User as User

    Caller->>Handler: handle_tool_execution(profile, tool_call, event_tx)
    Handler->>Handler: Check tool permission
    
    alt Permission: Allow
        Handler-->>Caller: Ok(true) - Proceed with execution
    else Permission: Deny
        Handler-->>Caller: Err(Internal) - Execution forbidden
    else Permission: Ask
        Handler->>Handler: Log debug message
        Handler->>EventTx: send(ToolCallPending event)
        alt Send Success
            EventTx->>Session: ToolCallPending event
            Session->>User: ExecApprovalRequest (protocol)
            Handler-->>Caller: Ok(false) - Wait for approval
            User->>Session: Approval response
            Session->>Caller: Retry execution
        else Send Failure
            Handler-->>Caller: Err(Internal) - Failed to send approval request
        end
    end

Loading

[greptile-apps]

_{4 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[echobt]

Closing as part of PR consolidation. This fix for tool call permission handling has been consolidated into PR #73 (consolidated error handling improvements). See #73 for the comprehensive error handling changes.