chore: consolidated refactoring and documentation improvements

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 | Rationale |
|-----------------------------|-----------------------------|-----------------------------|-------|-----------|
| Health checks               | cortex-common/http_client   | HEALTH_CHECK_TIMEOUT        | 5s    | Quick validation |
| Standard HTTP requests      | cortex-common/http_client   | DEFAULT_TIMEOUT             | 30s   | Normal API calls |
| Connection pool idle        | cortex-common/http_client   | POOL_IDLE_TIMEOUT           | 60s   | DNS refresh |
| LLM Request (non-streaming) | cortex-exec/runner          | DEFAULT_REQUEST_TIMEOUT_SECS| 120s  | Model inference |
| LLM Streaming total         | cortex-common/http_client   | STREAMING_TIMEOUT           | 300s  | Long-running streams |

## 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

Author: echobt

Cargo.lock

@@ -45,7 +45,7 @@ impl Claims {
     pub fn new(user_id: impl Into<String>, expiry_seconds: u64) -> Self {
         let now = SystemTime::now()
             .duration_since(UNIX_EPOCH)
-            .unwrap()
+            .unwrap_or_default()
             .as_secs();
 
         Self {
@@ -75,7 +75,7 @@ impl Claims {
     pub fn is_expired(&self) -> bool {
         let now = SystemTime::now()
             .duration_since(UNIX_EPOCH)
-            .unwrap()
+            .unwrap_or_default()
             .as_secs();
         self.exp < now
     }
@@ -187,7 +187,7 @@ impl AuthService {
     pub async fn cleanup_revoked_tokens(&self) {
         let now = SystemTime::now()
             .duration_since(UNIX_EPOCH)
-            .unwrap()
+            .unwrap_or_default()
             .as_secs();
 
         let mut revoked = self.revoked_tokens.write().await;

src/cortex-app-server/src/auth.rs

@@ -49,12 +49,18 @@ pub struct ServerConfig {
     pub max_body_size: usize,
 
     /// Request timeout in seconds (applies to full request lifecycle).
+    ///
+    /// See `cortex_common::http_client` module documentation for the complete
+    /// timeout hierarchy across Cortex services.
     #[serde(default = "default_request_timeout")]
     pub request_timeout: u64,
 
     /// Read timeout for individual chunks in seconds.
     /// Applies to chunked transfer encoding to prevent indefinite hangs
     /// when clients disconnect without sending the terminal chunk.
+    ///
+    /// See `cortex_common::http_client` module documentation for the complete
+    /// timeout hierarchy across Cortex services.
     #[serde(default = "default_read_timeout")]
     pub read_timeout: u64,
 
@@ -71,12 +77,17 @@ pub struct ServerConfig {
     pub cors_origins: Vec<String>,
 
     /// Graceful shutdown timeout in seconds.
+    ///
+    /// See `cortex_common::http_client` module documentation for the complete
+    /// timeout hierarchy across Cortex services.
     #[serde(default = "default_shutdown_timeout")]
     pub shutdown_timeout: u64,
 }
 
 fn default_shutdown_timeout() -> u64 {
-    30 // 30 seconds for graceful shutdown
+    // 30 seconds for graceful shutdown
+    // See cortex_common::http_client for timeout hierarchy documentation
+    30
 }
 
 fn default_listen_addr() -> String {

src/cortex-app-server/src/config.rs

@@ -47,8 +47,6 @@ pub struct StoredToolCall {
 
 /// Session storage manager.
 pub struct SessionStorage {
-    #[allow(dead_code)]
-    base_dir: PathBuf,
     sessions_dir: PathBuf,
     history_dir: PathBuf,
 }
@@ -66,7 +64,6 @@ impl SessionStorage {
         info!("Session storage initialized at {:?}", base_dir);
 
         Ok(Self {
-            base_dir,
             sessions_dir,
             history_dir,
         })

src/cortex-app-server/src/storage.rs

@@ -510,10 +510,10 @@ async fn session_events_stream(
             serde_json::to_string(&StreamEvent::Ping {
                 timestamp: std::time::SystemTime::now()
                     .duration_since(std::time::UNIX_EPOCH)
-                    .unwrap()
+                    .unwrap_or_default()
                     .as_secs(),
             })
-            .unwrap(),
+            .unwrap_or_default(),
         )))
         .await;
 

src/cortex-app-server/src/streaming.rs

@@ -250,9 +250,6 @@ pub struct SearchReplace {
     pub search: String,
     /// The text to replace with.
     pub replace: String,
-    /// Replace all occurrences (true) or just the first (false).
-    #[allow(dead_code)]
-    pub replace_all: bool,
 }
 
 impl SearchReplace {
@@ -266,16 +263,8 @@ impl SearchReplace {
             path: path.into(),
             search: search.into(),
             replace: replace.into(),
-            replace_all: false,
         }
     }
-
-    /// Set whether to replace all occurrences.
-    #[allow(dead_code)]
-    pub fn with_replace_all(mut self, replace_all: bool) -> Self {
-        self.replace_all = replace_all;
-        self
-    }
 }
 
 #[cfg(test)]

src/cortex-apply-patch/src/hunk.rs

@@ -8,8 +8,22 @@
 
 use regex::Regex;
 use std::path::PathBuf;
+use std::sync::LazyLock;
 use thiserror::Error;
 
+/// Static regex for environment variable substitution: {env:VAR} or {env:VAR:default}
+/// Group 1: variable name
+/// Group 2: optional default value (after second colon)
+static ENV_REGEX: LazyLock<Regex> = LazyLock::new(|| {
+    Regex::new(r"\{env:([^:}]+)(?::([^}]*))?\}").expect("env regex pattern is valid and tested")
+});
+
+/// Static regex for file content substitution: {file:path}
+/// Group 1: file path
+static FILE_REGEX: LazyLock<Regex> = LazyLock::new(|| {
+    Regex::new(r"\{file:([^}]+)\}").expect("file regex pattern is valid and tested")
+});
+
 /// Errors that can occur during configuration substitution.
 #[derive(Debug, Error)]
 pub enum SubstitutionError {
@@ -42,11 +56,13 @@ pub enum SubstitutionError {
 ///
 /// Handles replacement of `{env:...}` and `{file:...}` placeholders
 /// in configuration strings.
+///
+/// This struct uses statically initialized regex patterns via `LazyLock`,
+/// making regex compilation a one-time cost shared across all instances.
 pub struct ConfigSubstitution {
-    /// Regex for environment variable substitution: {env:VAR} or {env:VAR:default}
-    env_regex: Regex,
-    /// Regex for file content substitution: {file:path}
-    file_regex: Regex,
+    // This struct is kept for API compatibility.
+    // Regex patterns are now static module-level constants.
+    _private: (),
 }
 
 impl Default for ConfigSubstitution {
@@ -56,22 +72,13 @@ impl Default for ConfigSubstitution {
 }
 
 impl ConfigSubstitution {
-    /// Creates a new `ConfigSubstitution` instance with compiled regex patterns.
+    /// Creates a new `ConfigSubstitution` instance.
+    ///
+    /// The regex patterns are statically initialized on first use,
+    /// so creating multiple instances has no additional cost.
     #[must_use]
     pub fn new() -> Self {
-        Self {
-            // Matches {env:VAR_NAME} or {env:VAR_NAME:default_value}
-            // Group 1: variable name
-            // Group 2: optional default value (after second colon)
-            env_regex: Regex::new(r"\{env:([^:}]+)(?::([^}]*))?\}").unwrap_or_else(|e| {
-                panic!("Failed to compile env regex: {e}");
-            }),
-            // Matches {file:path}
-            // Group 1: file path
-            file_regex: Regex::new(r"\{file:([^}]+)\}").unwrap_or_else(|e| {
-                panic!("Failed to compile file regex: {e}");
-            }),
-        }
+        Self { _private: () }
     }
 
     /// Substitutes all variables in a string.
@@ -109,8 +116,7 @@ impl ConfigSubstitution {
         let mut error: Option<SubstitutionError> = None;
 
         // Collect all matches first to avoid borrowing issues
-        let matches: Vec<_> = self
-            .env_regex
+        let matches: Vec<_> = ENV_REGEX
             .captures_iter(input)
             .map(|cap| {
                 let full_match = cap.get(0).map(|m| m.as_str().to_string());
@@ -155,8 +161,7 @@ impl ConfigSubstitution {
         let mut error: Option<SubstitutionError> = None;
 
         // Collect all matches first
-        let matches: Vec<_> = self
-            .file_regex
+        let matches: Vec<_> = FILE_REGEX
             .captures_iter(input)
             .map(|cap| {
                 let full_match = cap.get(0).map(|m| m.as_str().to_string());

src/cortex-common/src/config_substitution.rs

@@ -9,25 +9,99 @@
 //!
 //! DNS caching is configured with reasonable TTL to allow failover and load
 //! balancer updates (#2177).
+//!
+//! # Timeout Configuration Guide
+//!
+//! This section documents the timeout hierarchy across the Cortex codebase. Use this
+//! as a reference when configuring timeouts for new features or debugging timeout issues.
+//!
+//! ## Timeout Hierarchy
+//!
+//! | Use Case                    | Timeout | Constant/Location                          | Rationale                               |
+//! |-----------------------------|---------|--------------------------------------------|-----------------------------------------|
+//! | Health checks               | 5s      | `HEALTH_CHECK_TIMEOUT` (this module)       | Quick validation of service status      |
+//! | Standard HTTP requests      | 30s     | `DEFAULT_TIMEOUT` (this module)            | Normal API calls with reasonable margin |
+//! | Per-chunk read (streaming)  | 30s     | `read_timeout` (cortex-app-server/config)  | Individual chunk timeout during stream  |
+//! | Pool idle timeout           | 60s     | `POOL_IDLE_TIMEOUT` (this module)          | DNS re-resolution for failover          |
+//! | LLM Request (non-streaming) | 120s    | `DEFAULT_REQUEST_TIMEOUT_SECS` (cortex-exec/runner) | Model inference takes time |
+//! | LLM Streaming total         | 300s    | `STREAMING_TIMEOUT` (this module)          | Long-running streaming responses        |
+//! | Server request lifecycle    | 300s    | `request_timeout` (cortex-app-server/config) | Full HTTP request/response cycle      |
+//! | Entire exec session         | 600s    | `DEFAULT_TIMEOUT_SECS` (cortex-exec/runner) | Multi-turn conversation limit          |
+//! | Graceful shutdown           | 30s     | `shutdown_timeout` (cortex-app-server/config) | Time for cleanup on shutdown        |
+//!
+//! ## Module-Specific Timeouts
+//!
+//! ### cortex-common (this module)
+//! - `DEFAULT_TIMEOUT` (30s): Use for standard API calls.
+//! - `STREAMING_TIMEOUT` (300s): Use for LLM streaming endpoints.
+//! - `HEALTH_CHECK_TIMEOUT` (5s): Use for health/readiness checks.
+//! - `POOL_IDLE_TIMEOUT` (60s): Connection pool cleanup for DNS freshness.
+//!
+//! ### cortex-exec (runner.rs)
+//! - `DEFAULT_TIMEOUT_SECS` (600s): Maximum duration for entire exec session.
+//! - `DEFAULT_REQUEST_TIMEOUT_SECS` (120s): Single LLM request timeout.
+//!
+//! ### cortex-app-server (config.rs)
+//! - `request_timeout` (300s): Full request lifecycle timeout.
+//! - `read_timeout` (30s): Per-chunk timeout for streaming reads.
+//! - `shutdown_timeout` (30s): Graceful shutdown duration.
+//!
+//! ### cortex-engine (api_client.rs)
+//! - Re-exports constants from this module for consistency.
+//!
+//! ## Recommendations
+//!
+//! When adding new timeout configurations:
+//! 1. Use constants from this module when possible for consistency.
+//! 2. Document any new timeout constants with their rationale.
+//! 3. Consider the timeout hierarchy - inner timeouts should be shorter than outer ones.
+//! 4. For LLM operations, use longer timeouts (120s-300s) to accommodate model inference.
+//! 5. For health checks and quick validations, use short timeouts (5s-10s).
 
 use reqwest::Client;
 use std::time::Duration;
 
+// ============================================================
+// Timeout Configuration Constants
+// ============================================================
+//
+// Timeout Hierarchy Documentation
+// ===============================
+//
+// This module defines the authoritative timeout constants for HTTP operations.
+// Other modules should reference these constants or document deviations.
+//
+// Timeout Categories:
+// - Request timeouts: Total time for a complete request/response cycle
+// - Connection timeouts: Time to establish TCP connection
+// - Read timeouts: Time to receive response data
+// - Pool timeouts: How long idle connections stay in pool
+//
+// Recommended Naming Convention:
+// - Constants: SCREAMING_SNAKE_CASE with Duration type
+// - Config fields: snake_case with _secs suffix for u64 values
+//
+// ============================================================
+
 /// User-Agent string for all HTTP requests
 pub const USER_AGENT: &str = concat!("cortex-cli/", env!("CARGO_PKG_VERSION"));
 
-/// Default timeout for standard API requests (30 seconds)
+/// Default timeout for standard HTTP requests (30 seconds).
+/// Used for non-streaming API calls, health checks with retries, etc.
 pub const DEFAULT_TIMEOUT: Duration = Duration::from_secs(30);
 
-/// Extended timeout for LLM streaming requests (5 minutes)
+/// Timeout for streaming HTTP requests (5 minutes).
+/// Longer duration to accommodate LLM inference time.
 pub const STREAMING_TIMEOUT: Duration = Duration::from_secs(300);
 
-/// Short timeout for health checks (5 seconds)
+/// Timeout for health check requests (5 seconds).
+/// Short timeout since health endpoints should respond quickly.
 pub const HEALTH_CHECK_TIMEOUT: Duration = Duration::from_secs(5);
 
-/// Connection pool idle timeout to ensure DNS is re-resolved periodically.
+/// Idle timeout for connection pool (60 seconds).
+/// Connections are closed after being idle for this duration
+/// to allow DNS re-resolution for services behind load balancers.
 /// This helps with failover scenarios where DNS records change (#2177).
-/// Set to 60 seconds to balance between performance and DNS freshness.
 pub const POOL_IDLE_TIMEOUT: Duration = Duration::from_secs(60);
 
 /// Creates an HTTP client with default configuration (30s timeout).

src/cortex-common/src/http_client.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,