feat(llm): Introduce a centralized HTTP client factory, refactor providers to use it for consistent timeout configuration, and enhance API error parsing.

vinhnx · vinhnx · commit 5fb79ffc0cca · 2025-12-25T14:30:03.000+07:00
diff --git a/vtcode-core/src/core/agent/bootstrap.rs b/vtcode-core/src/core/agent/bootstrap.rs
@@ -244,6 +244,7 @@ mod tests {
             checkpointing_storage_dir: None,
             checkpointing_max_snapshots: DEFAULT_MAX_SNAPSHOTS,
             checkpointing_max_age_days: Some(DEFAULT_MAX_AGE_DAYS),
+            quiet: false,
         };
 
         let components = AgentComponentBuilder::new(&agent_config)
@@ -276,6 +277,7 @@ mod tests {
             checkpointing_storage_dir: None,
             checkpointing_max_snapshots: DEFAULT_MAX_SNAPSHOTS,
             checkpointing_max_age_days: Some(DEFAULT_MAX_AGE_DAYS),
+            quiet: false,
         };
 
         let custom_session = SessionInfo {
@@ -350,6 +352,7 @@ mod tests {
             checkpointing_storage_dir: None,
             checkpointing_max_snapshots: DEFAULT_MAX_SNAPSHOTS,
             checkpointing_max_age_days: Some(DEFAULT_MAX_AGE_DAYS),
+            quiet: false,
         }
     }
 }
diff --git a/vtcode-core/src/llm/http_client.rs b/vtcode-core/src/llm/http_client.rs
@@ -0,0 +1,96 @@
+//! Centralized HTTP client factory for LLM providers.
+//!
+//! Provides consistent timeout configuration and client creation
+//! across all providers based on the application's TimeoutsConfig.
+
+use reqwest::Client;
+use std::time::Duration;
+
+use vtcode_config::TimeoutsConfig;
+
+/// Factory for creating pre-configured HTTP clients.
+pub struct HttpClientFactory;
+
+impl HttpClientFactory {
+    /// Create an HTTP client for standard LLM API requests.
+    ///
+    /// Uses the `default_ceiling_seconds` from TimeoutsConfig for the request timeout.
+    /// Includes a 30-second connect timeout.
+    pub fn for_llm(config: &TimeoutsConfig) -> Client {
+        let timeout_secs = if config.default_ceiling_seconds > 0 {
+            config.default_ceiling_seconds
+        } else {
+            180 // Fallback if disabled
+        };
+
+        Client::builder()
+            .timeout(Duration::from_secs(timeout_secs))
+            .connect_timeout(Duration::from_secs(30))
+            .build()
+            .unwrap_or_else(|_| Client::new())
+    }
+
+    /// Create an HTTP client optimized for streaming requests.
+    ///
+    /// Uses the `streaming_ceiling_seconds` from TimeoutsConfig.
+    /// Streaming requests typically take longer as they wait for incremental output.
+    pub fn for_streaming(config: &TimeoutsConfig) -> Client {
+        let timeout_secs = if config.streaming_ceiling_seconds > 0 {
+            config.streaming_ceiling_seconds
+        } else {
+            600 // 10 minutes fallback for streaming
+        };
+
+        Client::builder()
+            .timeout(Duration::from_secs(timeout_secs))
+            .connect_timeout(Duration::from_secs(30))
+            .pool_idle_timeout(Some(Duration::from_secs(90)))
+            .pool_max_idle_per_host(1)
+            .build()
+            .unwrap_or_else(|_| Client::new())
+    }
+
+    /// Create an HTTP client with explicit timeout values.
+    ///
+    /// Use this when providers need custom timeout handling.
+    pub fn with_timeouts(request_timeout: Duration, connect_timeout: Duration) -> Client {
+        Client::builder()
+            .timeout(request_timeout)
+            .connect_timeout(connect_timeout)
+            .build()
+            .unwrap_or_else(|_| Client::new())
+    }
+
+    /// Create a default HTTP client with reasonable defaults.
+    ///
+    /// Uses 180s request timeout and 30s connect timeout.
+    pub fn default_client() -> Client {
+        Self::with_timeouts(Duration::from_secs(180), Duration::from_secs(30))
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn default_client_is_created() {
+        let client = HttpClientFactory::default_client();
+        // Just verify it doesn't panic
+        drop(client);
+    }
+
+    #[test]
+    fn for_llm_uses_config_timeout() {
+        let config = TimeoutsConfig::default();
+        let client = HttpClientFactory::for_llm(&config);
+        drop(client);
+    }
+
+    #[test]
+    fn for_streaming_uses_longer_timeout() {
+        let config = TimeoutsConfig::default();
+        let client = HttpClientFactory::for_streaming(&config);
+        drop(client);
+    }
+}
diff --git a/vtcode-core/src/llm/mod.rs b/vtcode-core/src/llm/mod.rs
@@ -178,6 +178,7 @@ pub mod token_metrics;
 pub mod tool_bridge;
 pub mod types;
 pub mod utils; // Shared utilities for request/response processing
+pub mod http_client; // Centralized HTTP client factory
 
 #[cfg(test)]
 mod error_display_test;
diff --git a/vtcode-core/src/llm/providers/anthropic.rs b/vtcode-core/src/llm/providers/anthropic.rs
@@ -80,6 +80,8 @@ impl AnthropicProvider {
         base_url: Option<String>,
         anthropic_config: AnthropicConfig,
     ) -> Self {
+        use crate::llm::http_client::HttpClientFactory;
+        
         let (prompt_cache_enabled, prompt_cache_settings) = extract_prompt_cache_settings(
             prompt_cache,
             |providers| &providers.anthropic,
@@ -98,7 +100,7 @@ impl AnthropicProvider {
 
         Self {
             api_key,
-            http_client: HttpClient::new(),
+            http_client: HttpClientFactory::default_client(),
             base_url: base_url_value,
             model,
             prompt_cache_enabled,
diff --git a/vtcode-core/src/llm/providers/base.rs b/vtcode-core/src/llm/providers/base.rs
@@ -45,14 +45,11 @@ impl ProviderConfig {
     /// Build HTTP client with provider-specific configuration
     #[allow(clippy::result_large_err)]
     pub fn build_http_client(&self) -> Result<HttpClient, LLMError> {
-        HttpClient::builder()
-            .timeout(self.timeout)
-            .connect_timeout(Duration::from_secs(30))
-            .build()
-            .map_err(|e| LLMError::Network {
-                message: format!("Failed to build HTTP client: {}", e),
-                metadata: None,
-            })
+        use crate::llm::http_client::HttpClientFactory;
+        Ok(HttpClientFactory::with_timeouts(
+            self.timeout,
+            Duration::from_secs(30),
+        ))
     }
 }
 
diff --git a/vtcode-core/src/llm/providers/deepseek.rs b/vtcode-core/src/llm/providers/deepseek.rs
@@ -67,6 +67,8 @@ impl DeepSeekProvider {
         prompt_cache: Option<PromptCachingConfig>,
         base_url: Option<String>,
     ) -> Self {
+        use crate::llm::http_client::HttpClientFactory;
+        
         let (prompt_cache_enabled, prompt_cache_settings) = extract_prompt_cache_settings(
             prompt_cache,
             |providers| &providers.deepseek,
@@ -75,7 +77,7 @@ impl DeepSeekProvider {
 
         Self {
             api_key,
-            http_client: HttpClient::new(),
+            http_client: HttpClientFactory::default_client(),
             base_url: override_base_url(
                 urls::DEEPSEEK_API_BASE,
                 base_url,
diff --git a/vtcode-core/src/llm/providers/error_handling.rs b/vtcode-core/src/llm/providers/error_handling.rs
@@ -259,6 +259,119 @@ pub fn format_http_error(provider: &str, status: reqwest::StatusCode, error_text
     error_display::format_llm_error(provider, &format!("HTTP {}: {}", status, error_text))
 }
 
+/// Parse standard API error response body into LLMError.
+///
+/// Handles multiple provider error formats:
+/// - OpenAI/DeepSeek/ZAI: `{"error": {"message": "..."}}`
+/// - Anthropic: `{"type": "error", "error": {"message": "..."}}`
+/// - Gemini: `{"error": {"message": "...", "status": "..."}}`
+/// - HuggingFace: `{"error": "..."}`
+///
+/// Falls back to raw body if JSON parsing fails.
+pub fn parse_api_error(
+    provider_name: &'static str,
+    status: reqwest::StatusCode,
+    body: &str,
+) -> LLMError {
+    // Try to extract a meaningful error message from JSON
+    let error_message = if let Ok(json) = serde_json::from_str::<Value>(body) {
+        // OpenAI/DeepSeek/ZAI/Anthropic format: {"error": {"message": "..."}}
+        if let Some(msg) = json
+            .get("error")
+            .and_then(|e| e.get("message"))
+            .and_then(|m| m.as_str())
+        {
+            msg.to_string()
+        }
+        // HuggingFace simple format: {"error": "..."}
+        else if let Some(msg) = json.get("error").and_then(|e| e.as_str()) {
+            msg.to_string()
+        }
+        // Gemini alternate format: {"error": {"status": "...", "code": ...}}
+        else if let Some(status_msg) = json
+            .get("error")
+            .and_then(|e| e.get("status"))
+            .and_then(|s| s.as_str())
+        {
+            status_msg.to_string()
+        }
+        // Fallback to raw body
+        else {
+            body.to_string()
+        }
+    } else {
+        body.to_string()
+    };
+
+    // Categorize by status code
+    let status_code = status.as_u16();
+    
+    match status_code {
+        401 | 403 => LLMError::Authentication {
+            message: error_display::format_llm_error(
+                provider_name,
+                &format!("Authentication failed: {}", error_message),
+            ),
+            metadata: Some(LLMErrorMetadata::new(
+                provider_name,
+                Some(status_code),
+                Some("authentication_error".to_string()),
+                None,
+                None,
+                Some(body.to_string()),
+            )),
+        },
+        429 => LLMError::RateLimit {
+            metadata: Some(LLMErrorMetadata::new(
+                provider_name,
+                Some(status_code),
+                Some("rate_limit_error".to_string()),
+                None,
+                None,
+                Some(error_message),
+            )),
+        },
+        400 if is_rate_limit_error(status_code, body) => LLMError::RateLimit {
+            metadata: Some(LLMErrorMetadata::new(
+                provider_name,
+                Some(status_code),
+                Some("quota_exceeded".to_string()),
+                None,
+                None,
+                Some(error_message),
+            )),
+        },
+        400 => LLMError::InvalidRequest {
+            message: error_display::format_llm_error(
+                provider_name,
+                &format!("Invalid request: {}", error_message),
+            ),
+            metadata: Some(LLMErrorMetadata::new(
+                provider_name,
+                Some(status_code),
+                Some("invalid_request".to_string()),
+                None,
+                None,
+                Some(body.to_string()),
+            )),
+        },
+        _ => LLMError::Provider {
+            message: error_display::format_llm_error(
+                provider_name,
+                &format!("HTTP {}: {}", status, error_message),
+            ),
+            metadata: Some(LLMErrorMetadata::new(
+                provider_name,
+                Some(status_code),
+                None,
+                None,
+                None,
+                Some(body.to_string()),
+            )),
+        },
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/vtcode-core/src/llm/providers/gemini.rs b/vtcode-core/src/llm/providers/gemini.rs
@@ -108,6 +108,8 @@ impl GeminiProvider {
         base_url: Option<String>,
         timeouts: TimeoutsConfig,
     ) -> Self {
+        use crate::llm::http_client::HttpClientFactory;
+        
         let (prompt_cache_enabled, prompt_cache_settings) = extract_prompt_cache_settings(
             prompt_cache,
             |providers| &providers.gemini,
@@ -120,7 +122,7 @@ impl GeminiProvider {
 
         Self {
             api_key: Arc::from(api_key.as_str()),
-            http_client: HttpClient::new(),
+            http_client: HttpClientFactory::default_client(),
             base_url: Arc::from(
                 override_base_url(
                     urls::GEMINI_API_BASE,
diff --git a/vtcode-core/src/llm/providers/huggingface.rs b/vtcode-core/src/llm/providers/huggingface.rs
@@ -148,15 +148,13 @@ impl HuggingFaceProvider {
         base_url: Option<String>,
         timeouts: Option<TimeoutsConfig>,
     ) -> Self {
+        use crate::llm::http_client::HttpClientFactory;
+        
         let timeouts = timeouts.unwrap_or_default();
-        let timeout = std::time::Duration::from_secs(timeouts.default_ceiling_seconds);
         
         Self {
             api_key,
-            http_client: HttpClient::builder()
-                .timeout(timeout)
-                .build()
-                .unwrap_or_else(|_| HttpClient::new()),
+            http_client: HttpClientFactory::for_llm(&timeouts),
             base_url: override_base_url(
                 urls::HUGGINGFACE_API_BASE,
                 base_url,
diff --git a/vtcode-core/src/llm/providers/openai.rs b/vtcode-core/src/llm/providers/openai.rs
@@ -877,12 +877,16 @@ impl OpenAIProvider {
         };
         responses_api_modes.insert(model.clone(), initial_state);
 
+        // Use centralized HTTP client factory for consistent timeout handling
+        use crate::llm::http_client::HttpClientFactory;
+        let http_client = HttpClientFactory::with_timeouts(
+            Duration::from_secs(120),
+            Duration::from_secs(30),
+        );
+
         Self {
             api_key: Arc::from(api_key.as_str()),
-            http_client: HttpClient::builder()
-                .timeout(Duration::from_secs(120))
-                .build()
-                .unwrap_or_else(|_| HttpClient::new()),
+            http_client,
             base_url: Arc::from(resolved_base_url.as_str()),
             model: Arc::from(model.as_str()),
             responses_api_modes: Mutex::new(responses_api_modes),
diff --git a/vtcode-core/src/llm/providers/zai.rs b/vtcode-core/src/llm/providers/zai.rs
@@ -50,9 +50,11 @@ impl ZAIProvider {
         base_url: Option<String>,
         _prompt_cache: Option<PromptCachingConfig>,
     ) -> Self {
+        use crate::llm::http_client::HttpClientFactory;
+        
         Self {
             api_key,
-            http_client: HttpClient::new(),
+            http_client: HttpClientFactory::default_client(),
             base_url: override_base_url(
                 urls::Z_AI_API_BASE,
                 base_url,
