chore: consolidated refactoring and documentation improvements

[echobt]

Summary

This PR consolidates all refactoring and documentation improvements from PRs #77 and #89.

Refactoring (from PR #77)

Timeout Constants

• Created centralized cortex_common::timeout module with documented constants
• Replaced scattered magic numbers with well-documented constants
• Added comprehensive documentation for timeout hierarchy

Code Quality

• Used LazyLock for static regex initialization
• Extracted subagent timeout values as named constants
• Improved compile-time assertions

Documentation (from PR #89)

Timeout Hierarchy Documentation

• Added detailed header documentation explaining timeout hierarchy
• Documented each timeout constant with its purpose and use case
• Established naming conventions for constants vs config fields
• Added timeout hierarchy table for quick reference

Timeout Reference Table

Use Case	Module	Constant	Value
Health checks	cortex-common/http_client	HEALTH_CHECK_TIMEOUT	5s
Standard HTTP requests	cortex-common/http_client	DEFAULT_TIMEOUT	30s
Connection pool idle	cortex-common/http_client	POOL_IDLE_TIMEOUT	60s
LLM Request (non-streaming)	cortex-exec/runner	DEFAULT_REQUEST_TIMEOUT_SECS	120s
LLM Streaming total	cortex-common/http_client	STREAMING_TIMEOUT	300s

Files Modified

• src/cortex-common/src/timeout.rs (new)
• src/cortex-common/src/http_client.rs
• src/cortex-app-server/src/config.rs
• src/cortex-exec/src/runner.rs
• Multiple other files

Closes #77, #89

[greptile-apps]

Greptile Overview

Greptile Summary

Consolidated refactoring from PRs #77 and #89 that centralizes timeout constants and improves documentation across the codebase.

Key Changes:

• Created new cortex_common::timeout module with well-documented centralized timeout constants
• Added comprehensive timeout hierarchy documentation to http_client.rs with cross-references throughout the codebase
• Refactored ConfigSubstitution to use LazyLock for static regex initialization, improving performance
• Extracted magic number timeouts into named constants (subagent timeouts, batch timeouts)
• Removed dead code: unused reconnection logic in MCP transport, unused struct fields
• Improved error handling: replaced .unwrap() with .unwrap_or_default() for UNIX_EPOCH calculations
• Added backwards compatibility alias (head_limit -> max_results) in grep handler

Issues Found:

• Critical: BatchToolArgs.timeout_secs field is documented and accepted but never used in the implementation (line 341 only uses tool_timeout_secs). This could break existing code expecting batch-level timeout behavior.

Confidence Score: 3/5

• Safe to merge with one critical issue that should be addressed
• The refactoring is well-structured with excellent documentation and test coverage. However, the unused timeout_secs parameter in BatchToolArgs represents a breaking API change that could silently fail for existing users
• Pay close attention to src/cortex-engine/src/tools/handlers/batch.rs - the timeout_secs parameter needs to be either removed or implemented

Important Files Changed

Filename	Overview
src/cortex-common/src/timeout.rs	New centralized timeout constants module with comprehensive documentation and tests
src/cortex-common/src/http_client.rs	Added detailed timeout hierarchy documentation and new health check client function
src/cortex-engine/src/tools/handlers/batch.rs	Changed timeout semantics from batch-level to tool-level, but left unused timeout_secs field causing confusion
src/cortex-common/src/config_substitution.rs	Refactored to use LazyLock for static regex initialization improving performance
src/cortex-mcp-client/src/transport.rs	Removed unused reconnection logic and test_connection method

Sequence Diagram

sequenceDiagram
    participant User
    participant cortex-exec
    participant cortex-common
    participant cortex-engine
    participant cortex-app-server
    
    Note over cortex-common: New timeout module<br/>centralized constants
    
    User->>cortex-exec: Execute CLI command
    cortex-exec->>cortex-common: Import DEFAULT_EXEC_TIMEOUT_SECS (600s)
    cortex-exec->>cortex-common: Import DEFAULT_REQUEST_TIMEOUT_SECS (120s)
    
    cortex-exec->>cortex-engine: Execute tool batch
    cortex-engine->>cortex-common: Import DEFAULT_BATCH_TIMEOUT_SECS (300s)
    Note over cortex-engine: Uses tool_timeout_secs (60s)<br/>per individual tool
    
    cortex-exec->>cortex-app-server: HTTP request to LLM
    cortex-app-server->>cortex-common: Use http_client factory
    
    alt Streaming Request
        cortex-common->>cortex-common: STREAMING_TIMEOUT (300s)
    else Standard Request
        cortex-common->>cortex-common: DEFAULT_TIMEOUT (30s)
    else Health Check
        cortex-common->>cortex-common: HEALTH_CHECK_TIMEOUT (5s)
    end
    
    Note over cortex-common: All timeouts now documented<br/>in timeout hierarchy table
    
    cortex-app-server-->>cortex-exec: Response
    cortex-exec-->>User: Result

Loading

[greptile-apps]

_{5 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

timeout_secs parameter is not used in the implementation - only tool_timeout_secs is used (line 341). Either remove this field or implement batch-level timeout enforcement.

Suggested change

	/// 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)]
	/// 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>,

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/cortex-engine/src/tools/handlers/batch.rs
Line: 46:51

Comment:
`timeout_secs` parameter is not used in the implementation - only `tool_timeout_secs` is used (line 341). Either remove this field or implement batch-level timeout enforcement.

```suggestion
    /// 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>,
```

How can I resolve this? If you propose a fix, please make it concise.