diff --git a/Cargo.lock b/Cargo.lock index 53092e2b8..11cd5bb62 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1786,9 +1786,9 @@ version = "0.176.2" dependencies = [ "anyhow", "fabro-config", + "fabro-http", "fabro-types", "futures", - "reqwest 0.13.2", "rmcp", "serde", "serde_json", diff --git a/lib/crates/fabro-cli/src/commands/model.rs b/lib/crates/fabro-cli/src/commands/model.rs index 7ba873053..f4b25506c 100644 --- a/lib/crates/fabro-cli/src/commands/model.rs +++ b/lib/crates/fabro-cli/src/commands/model.rs @@ -424,12 +424,8 @@ mod tests { use super::*; - fn test_http_client() -> fabro_http::HttpClient { - fabro_http::test_http_client().unwrap() - } - fn test_api_client(api_url: &str) -> fabro_api::Client { - fabro_api::Client::new_with_client(api_url, test_http_client()) + fabro_api::Client::new_with_client(api_url, fabro_test::test_http_client()) } fn test_model_json(id: &str, provider: Provider) -> serde_json::Value { diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index c5722a143..b7cba714f 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -437,7 +437,7 @@ impl HookExecutorImpl { .await } - /// Build a reqwest client for the given TLS mode. + /// Build an HTTP client for the given TLS mode. fn build_http_client(tls: TlsMode) -> fabro_http::HttpClient { let accept_invalid = matches!(tls, TlsMode::NoVerify | TlsMode::Off); #[cfg(test)] @@ -446,14 +446,14 @@ impl HookExecutorImpl { .danger_accept_invalid_certs(accept_invalid) .no_proxy() .build() - .unwrap_or_default() + .expect("hook HTTP client should build") } #[cfg(not(test))] { fabro_http::HttpClientBuilder::new() .danger_accept_invalid_certs(accept_invalid) .build() - .unwrap_or_default() + .expect("hook HTTP client should build") } } diff --git a/lib/crates/fabro-http/src/lib.rs b/lib/crates/fabro-http/src/lib.rs index 1a560cbeb..f439b3814 100644 --- a/lib/crates/fabro-http/src/lib.rs +++ b/lib/crates/fabro-http/src/lib.rs @@ -60,113 +60,135 @@ pub enum HttpClientBuildError { Reqwest(#[from] reqwest::Error), } -#[derive(Default)] -pub struct HttpClientBuilder { - inner: reqwest::ClientBuilder, - proxy_policy: Option, +/// Generates a proxy-policy-aware builder that wraps a reqwest client builder. +macro_rules! define_builder { + ($builder_name:ident, $inner_builder:ty, $inner_new:expr, $client_type:ty) => { + #[derive(Default)] + pub struct $builder_name { + inner: $inner_builder, + proxy_policy: Option, + } + + impl $builder_name { + #[must_use] + pub fn new() -> Self { + Self { + inner: $inner_new, + proxy_policy: None, + } + } + + #[must_use] + pub fn proxy_policy(mut self, proxy_policy: ProxyPolicy) -> Self { + self.proxy_policy = Some(proxy_policy); + self + } + + #[must_use] + pub fn no_proxy(mut self) -> Self { + self.inner = self.inner.no_proxy(); + self + } + + #[must_use] + pub fn proxy(mut self, proxy: Proxy) -> Self { + self.inner = self.inner.proxy(proxy); + self + } + + #[must_use] + pub fn user_agent(mut self, value: impl Into) -> Self { + self.inner = self.inner.user_agent(value.into()); + self + } + + #[must_use] + pub fn default_headers(mut self, headers: HeaderMap) -> Self { + self.inner = self.inner.default_headers(headers); + self + } + + #[must_use] + pub fn connect_timeout(mut self, timeout: Duration) -> Self { + self.inner = self.inner.connect_timeout(timeout); + self + } + + #[must_use] + pub fn timeout(mut self, timeout: Duration) -> Self { + self.inner = self.inner.timeout(timeout); + self + } + + #[must_use] + pub fn use_rustls_tls(mut self) -> Self { + self.inner = self.inner.use_rustls_tls(); + self + } + + #[must_use] + pub fn danger_accept_invalid_certs(mut self, accept_invalid_certs: bool) -> Self { + self.inner = self.inner.danger_accept_invalid_certs(accept_invalid_certs); + self + } + + #[must_use] + pub fn add_root_certificate(mut self, cert: Certificate) -> Self { + self.inner = self.inner.add_root_certificate(cert); + self + } + + #[must_use] + pub fn identity(mut self, identity: Identity) -> Self { + self.inner = self.inner.identity(identity); + self + } + + #[cfg(unix)] + #[must_use] + pub fn unix_socket

(mut self, path: P) -> Self + where + P: AsRef, + { + self.inner = self.inner.unix_socket(path.as_ref()); + self + } + + pub fn build(self) -> Result<$client_type, HttpClientBuildError> { + let proxy_policy = ProxyPolicy::resolve(self.proxy_policy)?; + let inner = match proxy_policy { + ProxyPolicy::System => self.inner, + ProxyPolicy::Disabled => self.inner.no_proxy(), + }; + inner.build().map_err(Into::into) + } + } + }; } +define_builder!( + HttpClientBuilder, + reqwest::ClientBuilder, + reqwest::Client::builder(), + HttpClient +); + +// `read_timeout` is only available on the async builder. impl HttpClientBuilder { - #[must_use] - pub fn new() -> Self { - Self { - inner: reqwest::Client::builder(), - proxy_policy: None, - } - } - - #[must_use] - pub fn proxy_policy(mut self, proxy_policy: ProxyPolicy) -> Self { - self.proxy_policy = Some(proxy_policy); - self - } - - #[must_use] - pub fn no_proxy(mut self) -> Self { - self.inner = self.inner.no_proxy(); - self - } - - #[must_use] - pub fn proxy(mut self, proxy: Proxy) -> Self { - self.inner = self.inner.proxy(proxy); - self - } - - #[must_use] - pub fn user_agent(mut self, value: impl Into) -> Self { - self.inner = self.inner.user_agent(value.into()); - self - } - - #[must_use] - pub fn default_headers(mut self, headers: HeaderMap) -> Self { - self.inner = self.inner.default_headers(headers); - self - } - - #[must_use] - pub fn connect_timeout(mut self, timeout: Duration) -> Self { - self.inner = self.inner.connect_timeout(timeout); - self - } - - #[must_use] - pub fn timeout(mut self, timeout: Duration) -> Self { - self.inner = self.inner.timeout(timeout); - self - } - #[must_use] pub fn read_timeout(mut self, timeout: Duration) -> Self { self.inner = self.inner.read_timeout(timeout); self } - - #[must_use] - pub fn use_rustls_tls(mut self) -> Self { - self.inner = self.inner.use_rustls_tls(); - self - } - - #[must_use] - pub fn danger_accept_invalid_certs(mut self, accept_invalid_certs: bool) -> Self { - self.inner = self.inner.danger_accept_invalid_certs(accept_invalid_certs); - self - } - - #[must_use] - pub fn add_root_certificate(mut self, cert: Certificate) -> Self { - self.inner = self.inner.add_root_certificate(cert); - self - } - - #[must_use] - pub fn identity(mut self, identity: Identity) -> Self { - self.inner = self.inner.identity(identity); - self - } - - #[cfg(unix)] - #[must_use] - pub fn unix_socket

(mut self, path: P) -> Self - where - P: AsRef, - { - self.inner = self.inner.unix_socket(path.as_ref()); - self - } - - pub fn build(self) -> Result { - let proxy_policy = ProxyPolicy::resolve(self.proxy_policy)?; - let inner = match proxy_policy { - ProxyPolicy::System => self.inner, - ProxyPolicy::Disabled => self.inner.no_proxy(), - }; - inner.build().map_err(Into::into) - } } +define_builder!( + BlockingHttpClientBuilder, + reqwest::blocking::ClientBuilder, + reqwest::blocking::Client::builder(), + BlockingHttpClient +); + pub fn http_client() -> Result { HttpClientBuilder::new().build() } @@ -177,107 +199,6 @@ pub fn test_http_client() -> Result { .build() } -#[derive(Default)] -pub struct BlockingHttpClientBuilder { - inner: reqwest::blocking::ClientBuilder, - proxy_policy: Option, -} - -impl BlockingHttpClientBuilder { - #[must_use] - pub fn new() -> Self { - Self { - inner: reqwest::blocking::Client::builder(), - proxy_policy: None, - } - } - - #[must_use] - pub fn proxy_policy(mut self, proxy_policy: ProxyPolicy) -> Self { - self.proxy_policy = Some(proxy_policy); - self - } - - #[must_use] - pub fn no_proxy(mut self) -> Self { - self.inner = self.inner.no_proxy(); - self - } - - #[must_use] - pub fn proxy(mut self, proxy: Proxy) -> Self { - self.inner = self.inner.proxy(proxy); - self - } - - #[must_use] - pub fn user_agent(mut self, value: impl Into) -> Self { - self.inner = self.inner.user_agent(value.into()); - self - } - - #[must_use] - pub fn default_headers(mut self, headers: HeaderMap) -> Self { - self.inner = self.inner.default_headers(headers); - self - } - - #[must_use] - pub fn connect_timeout(mut self, timeout: Duration) -> Self { - self.inner = self.inner.connect_timeout(timeout); - self - } - - #[must_use] - pub fn timeout(mut self, timeout: Duration) -> Self { - self.inner = self.inner.timeout(timeout); - self - } - - #[must_use] - pub fn use_rustls_tls(mut self) -> Self { - self.inner = self.inner.use_rustls_tls(); - self - } - - #[must_use] - pub fn danger_accept_invalid_certs(mut self, accept_invalid_certs: bool) -> Self { - self.inner = self.inner.danger_accept_invalid_certs(accept_invalid_certs); - self - } - - #[must_use] - pub fn add_root_certificate(mut self, cert: Certificate) -> Self { - self.inner = self.inner.add_root_certificate(cert); - self - } - - #[must_use] - pub fn identity(mut self, identity: Identity) -> Self { - self.inner = self.inner.identity(identity); - self - } - - #[cfg(unix)] - #[must_use] - pub fn unix_socket

(mut self, path: P) -> Self - where - P: AsRef, - { - self.inner = self.inner.unix_socket(path.as_ref()); - self - } - - pub fn build(self) -> Result { - let proxy_policy = ProxyPolicy::resolve(self.proxy_policy)?; - let inner = match proxy_policy { - ProxyPolicy::System => self.inner, - ProxyPolicy::Disabled => self.inner.no_proxy(), - }; - inner.build().map_err(Into::into) - } -} - pub fn blocking_http_client() -> Result { BlockingHttpClientBuilder::new().build() } diff --git a/lib/crates/fabro-llm/src/providers/fabro_server.rs b/lib/crates/fabro-llm/src/providers/fabro_server.rs index e7f5bcb51..da9c643e8 100644 --- a/lib/crates/fabro-llm/src/providers/fabro_server.rs +++ b/lib/crates/fabro-llm/src/providers/fabro_server.rs @@ -228,10 +228,6 @@ mod tests { use crate::error::ProviderErrorKind; use crate::types::Message; - fn test_http_client() -> fabro_http::HttpClient { - fabro_http::test_http_client().unwrap() - } - fn make_request() -> Request { Request { model: "test-model".to_string(), @@ -273,7 +269,11 @@ data: {\"type\":\"text_delta\",\"delta\":\" world\",\"text_id\":null}\n\ .body(sse_body); }); - let adapter = Adapter::new(test_http_client(), server.base_url(), "test-provider"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + server.base_url(), + "test-provider", + ); let mut stream = adapter.stream(&make_request()).await.unwrap(); @@ -326,7 +326,11 @@ data: {\"type\":\"text_delta\",\"delta\":\" world\",\"text_id\":null}\n\ .json_body(response_json); }); - let adapter = Adapter::new(test_http_client(), server.base_url(), "test-provider"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + server.base_url(), + "test-provider", + ); let response = adapter.complete(&make_request()).await.unwrap(); @@ -349,7 +353,11 @@ data: {\"type\":\"text_delta\",\"delta\":\" world\",\"text_id\":null}\n\ then.status(502).body("Bad Gateway"); }); - let adapter = Adapter::new(test_http_client(), server.base_url(), "test-provider"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + server.base_url(), + "test-provider", + ); let err = adapter.complete(&make_request()).await.unwrap_err(); match &err { @@ -370,7 +378,11 @@ data: {\"type\":\"text_delta\",\"delta\":\" world\",\"text_id\":null}\n\ then.status(502).body("Bad Gateway"); }); - let adapter = Adapter::new(test_http_client(), server.base_url(), "test-provider"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + server.base_url(), + "test-provider", + ); let result = adapter.stream(&make_request()).await; let Err(err) = result else { @@ -404,7 +416,11 @@ data: {\"type\":\"stream_start\"}\n\ .body(sse_body); }); - let adapter = Adapter::new(test_http_client(), server.base_url(), "test-provider"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + server.base_url(), + "test-provider", + ); let mut stream = adapter.stream(&make_request()).await.unwrap(); @@ -450,7 +466,11 @@ data: {\"type\":\"stream_start\"}\n\ #[test] fn adapter_name() { - let adapter = Adapter::new(test_http_client(), "http://localhost", "anthropic"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + "http://localhost", + "anthropic", + ); assert_eq!(adapter.name(), "anthropic"); } } diff --git a/lib/crates/fabro-llm/src/providers/http_api.rs b/lib/crates/fabro-llm/src/providers/http_api.rs index 7c831dd6e..809f605c5 100644 --- a/lib/crates/fabro-llm/src/providers/http_api.rs +++ b/lib/crates/fabro-llm/src/providers/http_api.rs @@ -25,14 +25,14 @@ impl HttpApi { .connect_timeout(Duration::from_secs_f64(timeout.connect)) .no_proxy() .build() - .unwrap_or_default() + .expect("LLM HTTP client should build") } #[cfg(not(test))] { fabro_http::HttpClientBuilder::new() .connect_timeout(Duration::from_secs_f64(timeout.connect)) .build() - .unwrap_or_default() + .expect("LLM HTTP client should build") } } diff --git a/lib/crates/fabro-mcp/Cargo.toml b/lib/crates/fabro-mcp/Cargo.toml index 5ce17655a..03bd6455a 100644 --- a/lib/crates/fabro-mcp/Cargo.toml +++ b/lib/crates/fabro-mcp/Cargo.toml @@ -15,12 +15,12 @@ workspace = true [dependencies] anyhow.workspace = true fabro-config = { path = "../fabro-config" } +fabro-http.workspace = true fabro-types = { path = "../fabro-types" } serde.workspace = true serde_json.workspace = true tokio.workspace = true futures.workspace = true -reqwest.workspace = true tracing.workspace = true rmcp = { workspace = true, features = [ "client", diff --git a/lib/crates/fabro-mcp/src/client.rs b/lib/crates/fabro-mcp/src/client.rs index b282659a7..1c2cfde0f 100644 --- a/lib/crates/fabro-mcp/src/client.rs +++ b/lib/crates/fabro-mcp/src/client.rs @@ -1,11 +1,9 @@ -#![allow(clippy::disallowed_methods, clippy::disallowed_types)] - use std::process::Stdio; use std::sync::Arc; use std::time::Duration; use anyhow::{Result, anyhow}; -use reqwest::header::{HeaderMap, HeaderName, HeaderValue}; +use fabro_http::{HeaderMap, HeaderName, HeaderValue}; use rmcp::model::{CallToolRequestParams, CallToolResult}; use rmcp::service::{RoleClient, RunningService, serve_client}; use rmcp::transport::StreamableHttpClientTransport; @@ -28,7 +26,7 @@ enum ClientState { enum PendingTransport { Stdio(TokioChildProcess), - Http(StreamableHttpClientTransport), + Http(StreamableHttpClientTransport), } /// MCP client wrapping the rmcp SDK. Handles stdio and HTTP transports. @@ -68,7 +66,7 @@ impl McpClient { McpTransport::Http { url, headers } => { let http_config = StreamableHttpClientTransportConfig::with_uri(url.clone()); - let mut builder = reqwest::Client::builder(); + let mut builder = fabro_http::HttpClientBuilder::new(); if !headers.is_empty() { let mut header_map = HeaderMap::new(); for (key, value) in headers { diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index abcc9f0c1..649f8a5d5 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -19,6 +19,19 @@ use tokio::time::timeout; use crate::server::AppState; +fn http_client_or_check( + name: &str, + status: CheckStatus, +) -> Result { + fabro_http::http_client().map_err(|err| CheckResult { + name: name.to_string(), + status, + summary: "client error".to_string(), + details: vec![CheckDetail::new(err.to_string())], + remediation: Some(err.to_string()), + }) +} + #[derive(Debug, Serialize)] pub struct DiagnosticsReport { pub version: String, @@ -321,17 +334,9 @@ async fn check_github_app(state: &AppState) -> CheckResult { } }; - let http = match fabro_http::http_client() { + let http = match http_client_or_check("GitHub CLI", CheckStatus::Error) { Ok(http) => http, - Err(err) => { - return CheckResult { - name: "GitHub CLI".to_string(), - status: CheckStatus::Error, - summary: "client error".to_string(), - details: vec![CheckDetail::new(err.to_string())], - remediation: Some(err.to_string()), - }; - } + Err(result) => return result, }; let probe = timeout( Duration::from_secs(15), @@ -472,17 +477,9 @@ async fn check_github_app(state: &AppState) -> CheckResult { } }; - let http = match fabro_http::http_client() { + let http = match http_client_or_check("GitHub App", CheckStatus::Error) { Ok(http) => http, - Err(err) => { - return CheckResult { - name: "GitHub App".to_string(), - status: CheckStatus::Error, - summary: "client error".to_string(), - details: vec![CheckDetail::new(err.to_string())], - remediation: Some(err.to_string()), - }; - } + Err(result) => return result, }; let auth_result = timeout( Duration::from_secs(15), @@ -545,17 +542,9 @@ async fn check_brave_search(state: &AppState) -> CheckResult { }; }; - let http = match fabro_http::http_client() { + let http = match http_client_or_check("Brave Search", CheckStatus::Warning) { Ok(http) => http, - Err(err) => { - return CheckResult { - name: "Brave Search".to_string(), - status: CheckStatus::Warning, - summary: "client error".to_string(), - details: vec![CheckDetail::new(err.to_string())], - remediation: Some(err.to_string()), - }; - } + Err(result) => return result, }; let probe = timeout(Duration::from_secs(15), async move { diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index cf148cc63..08aee15b7 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -1764,20 +1764,15 @@ async fn get_github_repo( }); } let client_ref = client.as_ref().expect("client initialized above"); - let installed = match fabro_github::check_app_installed( - client_ref, - &jwt, - &owner, - &name, - &base_url, - ) - .await - { - Ok(installed) => installed, - Err(err) => { - return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(); - } - }; + let installed = + match fabro_github::check_app_installed(client_ref, &jwt, &owner, &name, &base_url) + .await + { + Ok(installed) => installed, + Err(err) => { + return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(); + } + }; if !installed { return ( diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index cafb99e38..470b073a8 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -15,6 +15,16 @@ use tracing::{debug, error, info, warn}; use crate::server::AppState; +fn github_http_client(context: &str) -> Result { + fabro_http::http_client().map_err(|err| { + error!(error = %err, "{context}: could not build GitHub HTTP client"); + json_response( + StatusCode::SERVICE_UNAVAILABLE, + json!({"error": format!("Failed to build GitHub HTTP client: {err}")}), + ) + }) +} + pub const SESSION_COOKIE_NAME: &str = "__fabro_session"; const OAUTH_STATE_COOKIE_NAME: &str = "fabro_oauth_state"; @@ -286,15 +296,9 @@ async fn callback_github( } }; - let http = match fabro_http::http_client() { + let http = match github_http_client("OAuth callback failed") { Ok(http) => http, - Err(err) => { - error!(error = %err, "OAuth callback failed: could not build GitHub HTTP client"); - return json_response( - StatusCode::SERVICE_UNAVAILABLE, - json!({"error": format!("Failed to build GitHub HTTP client: {err}")}), - ); - } + Err(response) => return response, }; let token = match http .post("https://github.com/login/oauth/access_token") @@ -534,15 +538,9 @@ async fn setup_register( .and_then(|s| fabro_http::Url::parse(s).ok()) .map(|url| format!("{}://{}", url.scheme(), url.authority())); - let http = match fabro_http::http_client() { + let http = match github_http_client("Setup register failed") { Ok(http) => http, - Err(err) => { - error!(error = %err, "Setup register failed: could not build GitHub HTTP client"); - return json_response( - StatusCode::SERVICE_UNAVAILABLE, - json!({"error": format!("Failed to build GitHub HTTP client: {err}")}), - ); - } + Err(response) => return response, }; let response = match http .post(format!( diff --git a/test/twin/openai/tests/common/mod.rs b/test/twin/openai/tests/common/mod.rs index cca6a5898..af1f7c93e 100644 --- a/test/twin/openai/tests/common/mod.rs +++ b/test/twin/openai/tests/common/mod.rs @@ -6,8 +6,8 @@ use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{Duration, Instant}; use anyhow::Result; -use fabro_http::{HttpClient as Client, HttpClientBuilder}; use fabro_http::header::AUTHORIZATION; +use fabro_http::{HttpClient as Client, HttpClientBuilder}; use futures_util::StreamExt; use serde_json::Value; use tokio::io::{AsyncReadExt, AsyncWriteExt};