scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-08-28 05:27:41 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

fix: simplify fabro-http crate and fix correctness issues

Browse source 
- Replace unwrap_or_default() with expect() in hooks/llm HTTP client
  builders — Default silently discards all config (timeouts, TLS, proxy)
- Route fabro-mcp through fabro_http instead of raw reqwest, respecting
  FABRO_HTTP_PROXY_POLICY for MCP HTTP transport connections
- Deduplicate HttpClientBuilder / BlockingHttpClientBuilder via macro
- Extract helpers for repeated http_client error handling in diagnostics
  and web_auth
- Remove duplicate test_http_client() in fabro-cli and fabro-llm

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-12 12:20:55 -04:00
parent 3b2cffceaf
commit e8ad12fc30
12 changed files with 203 additions and 286 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
Cargo.lock generated
View file

@ -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",

6
lib/crates/fabro-cli/src/commands/model.rs
View file

@ -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 {

6
lib/crates/fabro-hooks/src/executor.rs
View file

@ -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")
}
}

317
lib/crates/fabro-http/src/lib.rs
View file

@ -60,113 +60,135 @@ pub enum HttpClientBuildError {
Reqwest(#[from] reqwest::Error),
}
#[derive(Default)]
pub struct HttpClientBuilder {
inner: reqwest::ClientBuilder,
proxy_policy: Option<ProxyPolicy>,
/// 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<ProxyPolicy>,
}
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<String>) -> 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<P>(mut self, path: P) -> Self
where
P: AsRef<Path>,
{
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<String>) -> 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<P>(mut self, path: P) -> Self
where
P: AsRef<Path>,
{
self.inner = self.inner.unix_socket(path.as_ref());
self
}
pub fn build(self) -> Result<HttpClient, 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!(
BlockingHttpClientBuilder,
reqwest::blocking::ClientBuilder,
reqwest::blocking::Client::builder(),
BlockingHttpClient
);
pub fn http_client() -> Result<HttpClient, HttpClientBuildError> {
HttpClientBuilder::new().build()
}
@ -177,107 +199,6 @@ pub fn test_http_client() -> Result<HttpClient, HttpClientBuildError> {
.build()
}
#[derive(Default)]
pub struct BlockingHttpClientBuilder {
inner: reqwest::blocking::ClientBuilder,
proxy_policy: Option<ProxyPolicy>,
}
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<String>) -> 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<P>(mut self, path: P) -> Self
where
P: AsRef<Path>,
{
self.inner = self.inner.unix_socket(path.as_ref());
self
}
pub fn build(self) -> Result<BlockingHttpClient, 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)
}
}
pub fn blocking_http_client() -> Result<BlockingHttpClient, HttpClientBuildError> {
BlockingHttpClientBuilder::new().build()
}

40
lib/crates/fabro-llm/src/providers/fabro_server.rs
View file

@ -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");
}
}

4
lib/crates/fabro-llm/src/providers/http_api.rs
View file

@ -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")
}
}

2
lib/crates/fabro-mcp/Cargo.toml
View file

@ -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",

8
lib/crates/fabro-mcp/src/client.rs
View file

@ -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<reqwest::Client>),
Http(StreamableHttpClientTransport<fabro_http::HttpClient>),
}
/// 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 {

49
lib/crates/fabro-server/src/diagnostics.rs
View file

@ -19,6 +19,19 @@ use tokio::time::timeout;
use crate::server::AppState;
fn http_client_or_check(
name: &str,
status: CheckStatus,
) -> Result<fabro_http::HttpClient, CheckResult> {
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 {

23
lib/crates/fabro-server/src/server.rs
View file

@ -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 (

30
lib/crates/fabro-server/src/web_auth.rs
View file

@ -15,6 +15,16 @@ use tracing::{debug, error, info, warn};
use crate::server::AppState;
fn github_http_client(context: &str) -> Result<fabro_http::HttpClient, Response> {
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!(

2
test/twin/openai/tests/common/mod.rs
View file

@ -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};