Skip to content

Commit 6a06b1e

Browse files
committed
fix(tools): consolidated tools parameter aliases and handler name fixes
This PR consolidates the following tools fixes: - #53: Add serde alias for Glob tool folder parameter - #62: Align tool handler names with registration names - #63: Add serde alias for Grep tool head_limit parameter Key changes: - Added serde aliases for backward compatibility - Fixed mismatched handler names to match tool registration - Improved tool parameter handling consistency
1 parent c398212 commit 6a06b1e

File tree

10 files changed

+74
-121
lines changed

10 files changed

+74
-121
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ impl Claims {
4545
pub fn new(user_id: impl Into<String>, expiry_seconds: u64) -> Self {
4646
let now = SystemTime::now()
4747
.duration_since(UNIX_EPOCH)
48-
.unwrap()
48+
.unwrap_or_default()
4949
.as_secs();
5050

5151
Self {
@@ -75,7 +75,7 @@ impl Claims {
7575
pub fn is_expired(&self) -> bool {
7676
let now = SystemTime::now()
7777
.duration_since(UNIX_EPOCH)
78-
.unwrap()
78+
.unwrap_or_default()
7979
.as_secs();
8080
self.exp < now
8181
}
@@ -187,7 +187,7 @@ impl AuthService {
187187
pub async fn cleanup_revoked_tokens(&self) {
188188
let now = SystemTime::now()
189189
.duration_since(UNIX_EPOCH)
190-
.unwrap()
190+
.unwrap_or_default()
191191
.as_secs();
192192

193193
let mut revoked = self.revoked_tokens.write().await;

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,12 +49,18 @@ pub struct ServerConfig {
4949
pub max_body_size: usize,
5050

5151
/// Request timeout in seconds (applies to full request lifecycle).
52+
///
53+
/// See `cortex_common::http_client` module documentation for the complete
54+
/// timeout hierarchy across Cortex services.
5255
#[serde(default = "default_request_timeout")]
5356
pub request_timeout: u64,
5457

5558
/// Read timeout for individual chunks in seconds.
5659
/// Applies to chunked transfer encoding to prevent indefinite hangs
5760
/// when clients disconnect without sending the terminal chunk.
61+
///
62+
/// See `cortex_common::http_client` module documentation for the complete
63+
/// timeout hierarchy across Cortex services.
5864
#[serde(default = "default_read_timeout")]
5965
pub read_timeout: u64,
6066

@@ -71,12 +77,16 @@ pub struct ServerConfig {
7177
pub cors_origins: Vec<String>,
7278

7379
/// Graceful shutdown timeout in seconds.
80+
///
81+
/// See `cortex_common::http_client` module documentation for the complete
82+
/// timeout hierarchy across Cortex services.
7483
#[serde(default = "default_shutdown_timeout")]
7584
pub shutdown_timeout: u64,
7685
}
7786

7887
fn default_shutdown_timeout() -> u64 {
7988
30 // 30 seconds for graceful shutdown
89+
// See cortex_common::http_client for timeout hierarchy documentation
8090
}
8191

8292
fn default_listen_addr() -> String {

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,6 @@ pub struct StoredToolCall {
4747

4848
/// Session storage manager.
4949
pub struct SessionStorage {
50-
#[allow(dead_code)]
51-
base_dir: PathBuf,
5250
sessions_dir: PathBuf,
5351
history_dir: PathBuf,
5452
}
@@ -66,7 +64,6 @@ impl SessionStorage {
6664
info!("Session storage initialized at {:?}", base_dir);
6765

6866
Ok(Self {
69-
base_dir,
7067
sessions_dir,
7168
history_dir,
7269
})

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

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -250,9 +250,6 @@ pub struct SearchReplace {
250250
pub search: String,
251251
/// The text to replace with.
252252
pub replace: String,
253-
/// Replace all occurrences (true) or just the first (false).
254-
#[allow(dead_code)]
255-
pub replace_all: bool,
256253
}
257254

258255
impl SearchReplace {
@@ -266,16 +263,8 @@ impl SearchReplace {
266263
path: path.into(),
267264
search: search.into(),
268265
replace: replace.into(),
269-
replace_all: false,
270266
}
271267
}
272-
273-
/// Set whether to replace all occurrences.
274-
#[allow(dead_code)]
275-
pub fn with_replace_all(mut self, replace_all: bool) -> Self {
276-
self.replace_all = replace_all;
277-
self
278-
}
279268
}
280269

281270
#[cfg(test)]

src/cortex-common/src/http_client.rs

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,54 @@
99
//!
1010
//! DNS caching is configured with reasonable TTL to allow failover and load
1111
//! balancer updates (#2177).
12+
//!
13+
//! # Timeout Configuration Guide
14+
//!
15+
//! This section documents the timeout hierarchy across the Cortex codebase. Use this
16+
//! as a reference when configuring timeouts for new features or debugging timeout issues.
17+
//!
18+
//! ## Timeout Hierarchy
19+
//!
20+
//! | Use Case | Timeout | Constant/Location | Rationale |
21+
//! |-----------------------------|---------|--------------------------------------------|-----------------------------------------|
22+
//! | Health checks | 5s | `HEALTH_CHECK_TIMEOUT` (this module) | Quick validation of service status |
23+
//! | Standard HTTP requests | 30s | `DEFAULT_TIMEOUT` (this module) | Normal API calls with reasonable margin |
24+
//! | Per-chunk read (streaming) | 30s | `read_timeout` (cortex-app-server/config) | Individual chunk timeout during stream |
25+
//! | Pool idle timeout | 60s | `POOL_IDLE_TIMEOUT` (this module) | DNS re-resolution for failover |
26+
//! | LLM Request (non-streaming) | 120s | `DEFAULT_REQUEST_TIMEOUT_SECS` (cortex-exec/runner) | Model inference takes time |
27+
//! | LLM Streaming total | 300s | `STREAMING_TIMEOUT` (this module) | Long-running streaming responses |
28+
//! | Server request lifecycle | 300s | `request_timeout` (cortex-app-server/config) | Full HTTP request/response cycle |
29+
//! | Entire exec session | 600s | `DEFAULT_TIMEOUT_SECS` (cortex-exec/runner) | Multi-turn conversation limit |
30+
//! | Graceful shutdown | 30s | `shutdown_timeout` (cortex-app-server/config) | Time for cleanup on shutdown |
31+
//!
32+
//! ## Module-Specific Timeouts
33+
//!
34+
//! ### cortex-common (this module)
35+
//! - `DEFAULT_TIMEOUT` (30s): Use for standard API calls.
36+
//! - `STREAMING_TIMEOUT` (300s): Use for LLM streaming endpoints.
37+
//! - `HEALTH_CHECK_TIMEOUT` (5s): Use for health/readiness checks.
38+
//! - `POOL_IDLE_TIMEOUT` (60s): Connection pool cleanup for DNS freshness.
39+
//!
40+
//! ### cortex-exec (runner.rs)
41+
//! - `DEFAULT_TIMEOUT_SECS` (600s): Maximum duration for entire exec session.
42+
//! - `DEFAULT_REQUEST_TIMEOUT_SECS` (120s): Single LLM request timeout.
43+
//!
44+
//! ### cortex-app-server (config.rs)
45+
//! - `request_timeout` (300s): Full request lifecycle timeout.
46+
//! - `read_timeout` (30s): Per-chunk timeout for streaming reads.
47+
//! - `shutdown_timeout` (30s): Graceful shutdown duration.
48+
//!
49+
//! ### cortex-engine (api_client.rs)
50+
//! - Re-exports constants from this module for consistency.
51+
//!
52+
//! ## Recommendations
53+
//!
54+
//! When adding new timeout configurations:
55+
//! 1. Use constants from this module when possible for consistency.
56+
//! 2. Document any new timeout constants with their rationale.
57+
//! 3. Consider the timeout hierarchy - inner timeouts should be shorter than outer ones.
58+
//! 4. For LLM operations, use longer timeouts (120s-300s) to accommodate model inference.
59+
//! 5. For health checks and quick validations, use short timeouts (5s-10s).
1260
1361
use reqwest::Client;
1462
use std::time::Duration;

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ impl Default for WriteFileHandler {
204204
#[async_trait]
205205
impl ToolHandler for WriteFileHandler {
206206
fn name(&self) -> &str {
207-
"Write"
207+
"Create"
208208
}
209209

210210
async fn execute(&self, arguments: Value, context: &ToolContext) -> Result<ToolResult> {
@@ -445,7 +445,7 @@ impl Default for SearchFilesHandler {
445445
#[async_trait]
446446
impl ToolHandler for SearchFilesHandler {
447447
fn name(&self) -> &str {
448-
"search_files"
448+
"SearchFiles"
449449
}
450450

451451
async fn execute(&self, arguments: Value, context: &ToolContext) -> Result<ToolResult> {

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ pub struct GlobHandler;
1717
#[derive(Debug, Deserialize)]
1818
struct GlobArgs {
1919
patterns: Vec<String>,
20+
#[serde(alias = "folder")]
2021
directory: Option<String>,
2122
#[serde(default)]
2223
exclude_patterns: Vec<String>,

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ struct GrepArgs {
2929
glob_pattern: Option<String>,
3030
#[serde(default = "default_output_mode")]
3131
output_mode: String,
32+
#[serde(alias = "head_limit")]
3233
max_results: Option<usize>,
3334
#[serde(default)]
3435
multiline: bool,

src/cortex-exec/src/runner.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,17 @@ use cortex_protocol::ConversationId;
2727
use crate::output::{OutputFormat, OutputWriter};
2828

2929
/// Default timeout for the entire execution (10 minutes).
30+
///
31+
/// This is the maximum duration for a multi-turn exec session.
32+
/// See `cortex_common::http_client` module documentation for the complete
33+
/// timeout hierarchy across Cortex services.
3034
const DEFAULT_TIMEOUT_SECS: u64 = 600;
3135

3236
/// Default timeout for a single LLM request (2 minutes).
37+
///
38+
/// Allows sufficient time for model inference while preventing indefinite hangs.
39+
/// See `cortex_common::http_client` module documentation for the complete
40+
/// timeout hierarchy across Cortex services.
3341
const DEFAULT_REQUEST_TIMEOUT_SECS: u64 = 120;
3442

3543
/// Maximum retries for transient errors.

src/cortex-mcp-client/src/transport.rs

Lines changed: 1 addition & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,7 @@ use cortex_mcp_types::{
2020
use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader};
2121
use tokio::process::{Child, Command};
2222
use tokio::sync::{Mutex, RwLock};
23-
use tokio::time::sleep;
24-
use tracing::{debug, error, info, warn};
23+
use tracing::{debug, info, warn};
2524

2625
// ============================================================================
2726
// Transport Trait
@@ -199,61 +198,6 @@ impl StdioTransport {
199198
Ok(())
200199
}
201200

202-
/// Reconnect with exponential backoff.
203-
///
204-
/// Properly cleans up existing connections before each attempt to prevent
205-
/// file descriptor leaks (#2198).
206-
#[allow(dead_code)]
207-
async fn reconnect(&self) -> Result<()> {
208-
if !self.reconnect_config.enabled {
209-
return Err(anyhow!("Reconnection disabled"));
210-
}
211-
212-
let mut attempt = 0;
213-
let mut delay = self.reconnect_config.initial_delay;
214-
215-
while attempt < self.reconnect_config.max_attempts {
216-
attempt += 1;
217-
info!(
218-
attempt,
219-
max = self.reconnect_config.max_attempts,
220-
"Attempting reconnection"
221-
);
222-
223-
// Clean up any existing connection before attempting reconnect
224-
// This prevents file descriptor leaks on repeated failures (#2198)
225-
{
226-
let mut process_guard = self.process.lock().await;
227-
if let Some(mut child) = process_guard.take() {
228-
// Kill the process and wait for it to clean up
229-
let _ = child.kill().await;
230-
// Wait a short time for resources to be released
231-
drop(child);
232-
}
233-
self.connected.store(false, Ordering::SeqCst);
234-
}
235-
236-
// Clear any stale pending responses
237-
self.pending_responses.write().await.clear();
238-
239-
match self.connect().await {
240-
Ok(()) => {
241-
info!("Reconnection successful");
242-
return Ok(());
243-
}
244-
Err(e) => {
245-
error!(error = %e, attempt, "Reconnection failed");
246-
if attempt < self.reconnect_config.max_attempts {
247-
sleep(delay).await;
248-
delay = (delay * 2).min(self.reconnect_config.max_delay);
249-
}
250-
}
251-
}
252-
}
253-
254-
Err(anyhow!("Failed to reconnect after {} attempts", attempt))
255-
}
256-
257201
/// Send a request and wait for response.
258202
async fn send_request(&self, request: JsonRpcRequest) -> Result<JsonRpcResponse> {
259203
// Ensure connected
@@ -516,51 +460,6 @@ impl HttpTransport {
516460
fn next_request_id(&self) -> RequestId {
517461
RequestId::Number(self.request_id.fetch_add(1, Ordering::SeqCst) as i64)
518462
}
519-
520-
/// Test connection.
521-
#[allow(dead_code)]
522-
async fn test_connection(&self) -> Result<()> {
523-
let request = JsonRpcRequest::new(self.next_request_id(), methods::PING);
524-
self.send_request(request).await?;
525-
Ok(())
526-
}
527-
528-
/// Reconnect with exponential backoff.
529-
#[allow(dead_code)]
530-
async fn reconnect(&self) -> Result<()> {
531-
if !self.reconnect_config.enabled {
532-
return Err(anyhow!("Reconnection disabled"));
533-
}
534-
535-
let mut attempt = 0;
536-
let mut delay = self.reconnect_config.initial_delay;
537-
538-
while attempt < self.reconnect_config.max_attempts {
539-
attempt += 1;
540-
info!(
541-
attempt,
542-
max = self.reconnect_config.max_attempts,
543-
"Attempting HTTP reconnection"
544-
);
545-
546-
match self.test_connection().await {
547-
Ok(()) => {
548-
info!("HTTP reconnection successful");
549-
self.connected.store(true, Ordering::SeqCst);
550-
return Ok(());
551-
}
552-
Err(e) => {
553-
error!(error = %e, attempt, "HTTP reconnection failed");
554-
if attempt < self.reconnect_config.max_attempts {
555-
sleep(delay).await;
556-
delay = (delay * 2).min(self.reconnect_config.max_delay);
557-
}
558-
}
559-
}
560-
}
561-
562-
Err(anyhow!("Failed to reconnect after {} attempts", attempt))
563-
}
564463
}
565464

566465
#[async_trait]

0 commit comments

Comments
 (0)