From ec50f6818fb0a308dcb63cdf0f0e608aceef496a Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Mon, 5 Oct 2026 18:22:16 +1300 Subject: [PATCH 1/5] refactor(core): map abnegate-http errors into the app error claudear's local HTTP client is moving to abnegate-http 0.1.2. Its consumers call the crate's HttpClient and HttpResponse::json inside functions that return claudear's Result, so the crate error needs one explicit conversion, and that conversion decides which failures the watcher retries (retry_trigger_error_is_transient treats Http and Network as transient). The mapping keeps every classification the local client had: - Request and UnreadableBody become Error::Http, as reqwest errors did, so they stay transient. The crate strips the URL from these errors, so messages no longer repeat a query string. - TooManyRedirects becomes Error::Network, transient like the reqwest redirect error it replaces. - Everything else becomes Error::Other with the crate's message: Json keeps the "JSON parse error: ..." text the local HttpResponse::json produced, and Unsupported, OversizedBody and the refused-destination variants cannot change on a retry. Mapping Json into Http would have made a malformed body look transient and retried it. The last arm also covers variants a later release adds, since the crate enum is non_exhaustive. Tests pin each variant's mapping in error.rs, using a real refused connection (through a client without a proxy) for the transport variants, and the watcher tests pin that a JSON error and an oversized body do not give a retry back while a refused connection does. SentryHttpClient lives in claudear-core's types.rs (also edited by #163) and answers with an HttpResponse, so its return type moves to the crate's HttpResponse here, with its two implementers (source/sentry.rs and the regression/sentry.rs test double); otherwise this commit would not build. The crate's HttpResponse is non_exhaustive, so they build it with HttpResponse::new. #147 and #45 also edit source/sentry.rs. The lockfile gains abnegate-http and dashmap 6. abnegate-http needs tokio 1.53, so tokio moves 1.50.0 -> 1.53.2 with tokio-macros pinned at 2.7.0 (as #168 does; 2.7.2 would pull in syn 3), and mio, socket2 and once_cell take patch bumps. #163, #167, #168 and the dependabot PRs also touch Cargo.toml and Cargo.lock. The sentry.rs boundary test loses its narrating comments and the file its section header. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 57 +++++-- Cargo.toml | 1 + crates/claudear-analysis/Cargo.toml | 1 + .../src/regression/sentry.rs | 20 +-- crates/claudear-core/Cargo.toml | 1 + crates/claudear-core/src/error.rs | 142 +++++++++++++++--- crates/claudear-core/src/types.rs | 4 +- crates/claudear-engine/Cargo.toml | 1 + crates/claudear-engine/src/watcher.rs | 37 ++++- crates/claudear-integrations/Cargo.toml | 1 + .../src/source/sentry.rs | 111 ++++---------- 11 files changed, 244 insertions(+), 132 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 636a5e2c..dea1320a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,6 +2,22 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "abnegate-http" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "220b9ca87e0f28ab4c6658650248cc9ec8cd68c792a7c3cafe20160d472354c1" +dependencies = [ + "async-trait", + "dashmap", + "rand 0.10.0", + "reqwest 0.13.2", + "serde", + "serde_json", + "thiserror 2.0.18", + "tokio", +] + [[package]] name = "abnegate-index" version = "0.1.0" @@ -839,6 +855,7 @@ dependencies = [ name = "claudear-analysis" version = "1.0.0" dependencies = [ + "abnegate-http", "abnegate-index", "async-trait", "base64 0.22.1", @@ -901,6 +918,7 @@ dependencies = [ name = "claudear-core" version = "1.0.0" dependencies = [ + "abnegate-http", "aes-gcm", "anyhow", "async-trait", @@ -952,6 +970,7 @@ dependencies = [ name = "claudear-engine" version = "1.0.0" dependencies = [ + "abnegate-http", "anyhow", "async-trait", "axum", @@ -992,6 +1011,7 @@ dependencies = [ name = "claudear-integrations" version = "1.0.0" dependencies = [ + "abnegate-http", "anyhow", "async-trait", "axum", @@ -1356,6 +1376,20 @@ dependencies = [ "serde", ] +[[package]] +name = "dashmap" +version = "6.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6361d5c062261c78a176addb82d4c821ae42bed6089de0e12603cd25de2059c" +dependencies = [ + "cfg-if", + "crossbeam-utils", + "hashbrown 0.14.5", + "lock_api", + "once_cell", + "parking_lot_core", +] + [[package]] name = "data-encoding" version = "2.10.0" @@ -2916,9 +2950,9 @@ dependencies = [ [[package]] name = "mio" -version = "1.1.1" +version = "1.2.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a69bcab0ad47271a0234d9422b131806bf3968021e5dc9328caf2d4cd58557fc" +checksum = "1788edb87fdc09c7e26304471e2f5be8cdefb1b6930d6e3985fc02ff53bf86ee" dependencies = [ "libc", "wasi", @@ -3348,9 +3382,9 @@ dependencies = [ [[package]] name = "once_cell" -version = "1.21.3" +version = "1.21.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "42f5e15c9953c5e4ccceeb2e7382a716482c34515315f7b03532b8b4e8393d2d" +checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" [[package]] name = "once_cell_polyfill" @@ -4090,6 +4124,7 @@ dependencies = [ "js-sys", "log", "mime", + "mime_guess", "percent-encoding", "pin-project-lite", "quinn", @@ -4768,12 +4803,12 @@ checksum = "67b1b7a3b5fe4f1376887184045fcf45c69e92af734b7aaddc05fb777b6fbd03" [[package]] name = "socket2" -version = "0.6.2" +version = "0.6.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "86f4aa3ad99f2088c990dfa82d367e19cb29268ed67c574d10d0a4bfe71f07e0" +checksum = "c3d1e2c7f27f8d4cb10542a02c49005dbd6e93095799d6f3be745fae9f8fedd4" dependencies = [ "libc", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -5095,9 +5130,9 @@ dependencies = [ [[package]] name = "tokio" -version = "1.50.0" +version = "1.53.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "27ad5e34374e03cfffefc301becb44e9dc3c17584f414349ebe29ed26661822d" +checksum = "e95f91fcc7a621e8b030f6aa23c71fe9838ae2fb4d8118b75602a328f5144044" dependencies = [ "bytes", "libc", @@ -5111,9 +5146,9 @@ dependencies = [ [[package]] name = "tokio-macros" -version = "2.6.0" +version = "2.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "af407857209536a95c8e56f8231ef2c2e2aff839b22e07a1ffcbc617e9db9fa5" +checksum = "385a6cb71ab9ab790c5fe8d67f1645e6c450a7ce006a33de03daa956cf70a496" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index 16a089ee..abea00da 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -151,6 +151,7 @@ mockall = "0.14" http-body-util = "0.1" # Shared infrastructure +abnegate-http = "0.1.2" abnegate-index = "0.1.0" # Internal crates diff --git a/crates/claudear-analysis/Cargo.toml b/crates/claudear-analysis/Cargo.toml index 9612dc31..9d8c74f3 100644 --- a/crates/claudear-analysis/Cargo.toml +++ b/crates/claudear-analysis/Cargo.toml @@ -12,6 +12,7 @@ sqlite = ["claudear-core/sqlite", "claudear-storage/sqlite"] cuda = ["ort/cuda"] [dependencies] +abnegate-http = { workspace = true } abnegate-index = { workspace = true } claudear-core = { workspace = true } claudear-config = { workspace = true } diff --git a/crates/claudear-analysis/src/regression/sentry.rs b/crates/claudear-analysis/src/regression/sentry.rs index daa29ed0..fa12f891 100644 --- a/crates/claudear-analysis/src/regression/sentry.rs +++ b/crates/claudear-analysis/src/regression/sentry.rs @@ -150,8 +150,9 @@ impl RegressionChecker for SentryRegressionChecker { #[cfg(test)] mod tests { use super::*; - use chrono::{Duration, Utc}; - use claudear_core::http::HttpResponse; + use abnegate_http::HttpResponse; + use chrono::Duration; + use chrono::Utc; use claudear_core::types::IssueType; struct MockSentryClient { @@ -161,10 +162,7 @@ mod tests { impl MockSentryClient { fn new(status: u16, body: &str) -> Self { Self { - response: HttpResponse { - status, - body: body.to_string(), - }, + response: HttpResponse::new(status, body.to_string()), } } } @@ -172,10 +170,7 @@ mod tests { #[async_trait] impl SentryHttpClient for MockSentryClient { async fn get(&self, _url: &str, _auth_token: &str) -> Result { - Ok(HttpResponse { - status: self.response.status, - body: self.response.body.clone(), - }) + Ok(self.response.clone()) } async fn put( @@ -184,10 +179,7 @@ mod tests { _auth_token: &str, _body: serde_json::Value, ) -> Result { - Ok(HttpResponse { - status: 200, - body: "{}".to_string(), - }) + Ok(HttpResponse::new(200, "{}")) } } diff --git a/crates/claudear-core/Cargo.toml b/crates/claudear-core/Cargo.toml index f52d2a03..89b14bc0 100644 --- a/crates/claudear-core/Cargo.toml +++ b/crates/claudear-core/Cargo.toml @@ -28,6 +28,7 @@ futures = { workspace = true } tokio = { workspace = true } # HTTP +abnegate-http = { workspace = true } reqwest = { workspace = true } # Logging diff --git a/crates/claudear-core/src/error.rs b/crates/claudear-core/src/error.rs index ed513294..e3fd20c5 100644 --- a/crates/claudear-core/src/error.rs +++ b/crates/claudear-core/src/error.rs @@ -1,5 +1,6 @@ //! Error types for the claudear application. +use abnegate_http::Error as HttpError; use thiserror::Error; /// Main error type for the application. @@ -61,8 +62,8 @@ pub enum Error { } impl Error { - pub fn config(msg: impl Into) -> Self { - Self::Config(msg.into()) + pub fn config(message: impl Into) -> Self { + Self::Config(message.into()) } pub fn source(source_name: impl Into, message: impl Into) -> Self { @@ -72,12 +73,12 @@ impl Error { } } - pub fn webhook(msg: impl Into) -> Self { - Self::Webhook(msg.into()) + pub fn webhook(message: impl Into) -> Self { + Self::Webhook(message.into()) } - pub fn runner(msg: impl Into) -> Self { - Self::Runner(msg.into()) + pub fn runner(message: impl Into) -> Self { + Self::Runner(message.into()) } pub fn notifier(notifier: impl Into, message: impl Into) -> Self { @@ -94,35 +95,45 @@ impl Error { } } - pub fn network(msg: impl Into) -> Self { - Self::Network(msg.into()) + pub fn network(message: impl Into) -> Self { + Self::Network(message.into()) } - pub fn api(msg: impl Into) -> Self { - Self::Api(msg.into()) + pub fn api(message: impl Into) -> Self { + Self::Api(message.into()) } - pub fn storage(msg: impl Into) -> Self { - Self::Storage(msg.into()) + pub fn storage(message: impl Into) -> Self { + Self::Storage(message.into()) } - pub fn git(msg: impl Into) -> Self { - Self::Git(msg.into()) + pub fn git(message: impl Into) -> Self { + Self::Git(message.into()) } - pub fn io(msg: impl Into) -> Self { - Self::Io(std::io::Error::other(msg.into())) + pub fn io(message: impl Into) -> Self { + Self::Io(std::io::Error::other(message.into())) } - pub fn database(msg: impl Into) -> Self { - Self::Database(msg.into()) + pub fn database(message: impl Into) -> Self { + Self::Database(message.into()) } } #[cfg(feature = "sqlite")] impl From for Error { - fn from(e: rusqlite::Error) -> Self { - Error::Database(e.to_string()) + fn from(error: rusqlite::Error) -> Self { + Error::Database(error.to_string()) + } +} + +impl From for Error { + fn from(error: HttpError) -> Self { + match error { + HttpError::Request(source) | HttpError::UnreadableBody(source) => Self::Http(source), + HttpError::TooManyRedirects => Self::Network(error.to_string()), + _ => Self::Other(error.to_string()), + } } } @@ -613,4 +624,95 @@ mod tests { } assert!(inner_fail().is_err()); } + + async fn refused_connection() -> reqwest::Error { + reqwest::Client::builder() + .no_proxy() + .build() + .expect("a client without a proxy builds") + .get("http://127.0.0.1:1/") + .send() + .await + .expect_err("nothing listens on port 1") + } + + #[tokio::test] + async fn a_transport_error_from_the_http_client_stays_an_http_error() { + let error = Error::from(HttpError::from(refused_connection().await)); + + assert!(matches!(error, Error::Http(_)), "{error:?}"); + } + + #[tokio::test] + async fn an_unreadable_body_from_the_http_client_is_an_http_error() { + let error = Error::from(HttpError::UnreadableBody(refused_connection().await)); + + assert!(matches!(error, Error::Http(_)), "{error:?}"); + } + + #[test] + fn a_redirect_loop_from_the_http_client_is_a_network_error() { + let error = Error::from(HttpError::TooManyRedirects); + + assert!(matches!(error, Error::Network(_)), "{error:?}"); + assert_eq!(error.to_string(), "Network error: Too many redirects."); + } + + #[test] + fn a_json_error_from_the_http_client_keeps_its_parse_message() { + let parse = serde_json::from_str::("not json") + .expect_err("the body is not JSON"); + + let error = Error::from(HttpError::from(parse)); + + assert!(matches!(error, Error::Other(_)), "{error:?}"); + assert!( + error.to_string().starts_with("JSON parse error: "), + "{error}" + ); + } + + #[test] + fn an_unsupported_method_from_the_http_client_is_other() { + let error = Error::from(HttpError::Unsupported(reqwest::Method::POST)); + + assert!(matches!(error, Error::Other(_)), "{error:?}"); + assert_eq!( + error.to_string(), + "POST is not supported by this HTTP client" + ); + } + + #[test] + fn an_oversized_body_from_the_http_client_is_other() { + let error = Error::from(HttpError::oversized_body(16)); + + assert!(matches!(error, Error::Other(_)), "{error:?}"); + assert_eq!( + error.to_string(), + "The response body is larger than 16 bytes." + ); + } + + #[test] + fn every_refused_destination_from_the_http_client_is_other() { + for refusal in [ + HttpError::InvalidUrl, + HttpError::InvalidHeaderName, + HttpError::InvalidHeaderValue, + HttpError::UnsupportedScheme, + HttpError::EmbeddedCredentials, + HttpError::MissingHost, + HttpError::PrivateAddress, + HttpError::InternalHost, + HttpError::unfetchable_resolution("inside.example"), + ] { + let message = refusal.to_string(); + + let error = Error::from(refusal); + + assert!(matches!(error, Error::Other(_)), "{error:?}"); + assert_eq!(error.to_string(), message); + } + } } diff --git a/crates/claudear-core/src/types.rs b/crates/claudear-core/src/types.rs index 6383d388..bea9896d 100644 --- a/crates/claudear-core/src/types.rs +++ b/crates/claudear-core/src/types.rs @@ -3454,7 +3454,7 @@ pub fn normalize_text(input: &str) -> String { #[async_trait::async_trait] pub trait SentryHttpClient: Send + Sync { /// Perform a GET request with bearer auth. - async fn get(&self, url: &str, auth_token: &str) -> crate::Result; + async fn get(&self, url: &str, auth_token: &str) -> crate::Result; /// Perform a PUT request with bearer auth and JSON body. async fn put( @@ -3462,7 +3462,7 @@ pub trait SentryHttpClient: Send + Sync { url: &str, auth_token: &str, body: serde_json::Value, - ) -> crate::Result; + ) -> crate::Result; } /// timeline for metadata of an issue diff --git a/crates/claudear-engine/Cargo.toml b/crates/claudear-engine/Cargo.toml index 318f517d..b204e6c5 100644 --- a/crates/claudear-engine/Cargo.toml +++ b/crates/claudear-engine/Cargo.toml @@ -90,5 +90,6 @@ indicatif = { workspace = true } libc = { workspace = true } [dev-dependencies] +abnegate-http = { workspace = true } http-body-util = { workspace = true } tokio = { workspace = true, features = ["test-util"] } diff --git a/crates/claudear-engine/src/watcher.rs b/crates/claudear-engine/src/watcher.rs index f6fcbaec..45fba115 100644 --- a/crates/claudear-engine/src/watcher.rs +++ b/crates/claudear-engine/src/watcher.rs @@ -137,9 +137,9 @@ pub enum RetryOutcome { } /// Whether a retry that failed to start should get its retry back. -fn retry_trigger_error_is_transient(e: &claudear_core::error::Error) -> bool { +fn retry_trigger_error_is_transient(error: &claudear_core::error::Error) -> bool { use claudear_core::error::Error; - match e { + match error { Error::Http(_) | Error::Network(_) => true, Error::Source { message, .. } => message.contains("already being processed"), _ => false, @@ -5963,6 +5963,39 @@ mod tests { } } + #[test] + fn a_json_error_from_the_http_client_does_not_give_the_retry_back() { + let parse = serde_json::from_str::("not json") + .expect_err("the body is not JSON"); + + let error = claudear_core::error::Error::from(abnegate_http::Error::from(parse)); + + assert!(!retry_trigger_error_is_transient(&error), "{error:?}"); + } + + #[test] + fn an_oversized_body_from_the_http_client_does_not_give_the_retry_back() { + let error = claudear_core::error::Error::from(abnegate_http::Error::oversized_body(16)); + + assert!(!retry_trigger_error_is_transient(&error), "{error:?}"); + } + + #[tokio::test] + async fn a_transport_error_from_the_http_client_gives_the_retry_back() { + let transport = reqwest::Client::builder() + .no_proxy() + .build() + .expect("a client without a proxy builds") + .get("http://127.0.0.1:1/") + .send() + .await + .expect_err("nothing listens on port 1"); + + let error = claudear_core::error::Error::from(abnegate_http::Error::from(transport)); + + assert!(retry_trigger_error_is_transient(&error), "{error:?}"); + } + #[test] fn test_extract_rate_limit_reset_from_banner_utc_same_day() { let now = chrono::DateTime::parse_from_rfc3339("2026-02-23T04:11:25Z") diff --git a/crates/claudear-integrations/Cargo.toml b/crates/claudear-integrations/Cargo.toml index 59e86eea..ea3c2153 100644 --- a/crates/claudear-integrations/Cargo.toml +++ b/crates/claudear-integrations/Cargo.toml @@ -11,6 +11,7 @@ default = [] sqlite = ["claudear-core/sqlite", "claudear-storage/sqlite", "claudear-analysis/sqlite"] [dependencies] +abnegate-http = { workspace = true } claudear-core = { workspace = true } claudear-config = { workspace = true } claudear-storage = { workspace = true } diff --git a/crates/claudear-integrations/src/source/sentry.rs b/crates/claudear-integrations/src/source/sentry.rs index e384e40c..88042fa7 100644 --- a/crates/claudear-integrations/src/source/sentry.rs +++ b/crates/claudear-integrations/src/source/sentry.rs @@ -1,12 +1,18 @@ //! Sentry issue source adapter. use super::IssueSource; -use crate::webhook::{sentry_map_priority as map_priority, sentry_map_status as map_status}; +use crate::webhook::sentry_map_priority as map_priority; +use crate::webhook::sentry_map_status as map_status; +use abnegate_http::HttpResponse; use async_trait::async_trait; use claudear_config::config::SentryConfig; -use claudear_core::error::{Error, Result}; -use claudear_core::http::HttpResponse; -use claudear_core::types::{Issue, IssuePriority, IssueStatus, MatchPriority, MatchResult}; +use claudear_core::error::Error; +use claudear_core::error::Result; +use claudear_core::types::Issue; +use claudear_core::types::IssuePriority; +use claudear_core::types::IssueStatus; +use claudear_core::types::MatchPriority; +use claudear_core::types::MatchResult; use futures::future::join_all; use serde::Deserialize; use std::collections::HashSet; @@ -51,7 +57,7 @@ impl SentryHttpClient for ReqwestSentryClient { .await?; let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } async fn put( @@ -69,10 +75,7 @@ impl SentryHttpClient for ReqwestSentryClient { .await?; let status = response.status().as_u16(); let body_text = response.text().await.unwrap_or_default(); - Ok(HttpResponse { - status, - body: body_text, - }) + Ok(HttpResponse::new(status, body_text)) } } @@ -179,7 +182,7 @@ impl SentrySource { )); } - response.json() + Ok(response.json()?) } async fn fetch_top_issues(&self) -> Result> { @@ -731,24 +734,12 @@ mod tests { pub fn mock_get(&self, url: impl Into, status: u16, body: impl Into) { let mut responses = self.get_responses.lock().unwrap(); - responses.insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + responses.insert(url.into(), HttpResponse::new(status, body)); } pub fn mock_put(&self, url: impl Into, status: u16, body: impl Into) { let mut responses = self.put_responses.lock().unwrap(); - responses.insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + responses.insert(url.into(), HttpResponse::new(status, body)); } #[expect(dead_code)] @@ -769,15 +760,9 @@ mod tests { // tests about the window register the windowed URL explicitly let unwindowed = url.split("&statsPeriod=").next().unwrap_or(url); if let Some(response) = responses.get(url).or_else(|| responses.get(unwindowed)) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } @@ -793,39 +778,24 @@ mod tests { .push(("PUT".to_string(), url.to_string())); let responses = self.put_responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } } #[test] fn test_http_response_is_success() { - let response = HttpResponse { - status: 200, - body: "{}".to_string(), - }; + let response = HttpResponse::new(200, "{}"); assert!(response.is_success()); - let response = HttpResponse { - status: 404, - body: "{}".to_string(), - }; + let response = HttpResponse::new(404, "{}"); assert!(!response.is_success()); } #[test] fn test_http_response_json() { - let response = HttpResponse { - status: 200, - body: r#"{"id": "123"}"#.to_string(), - }; + let response = HttpResponse::new(200, r#"{"id": "123"}"#); let parsed: serde_json::Value = response.json().unwrap(); assert_eq!(parsed["id"], "123"); } @@ -2548,41 +2518,18 @@ mod tests { #[test] fn test_http_response_json_parse_failure() { - let response = HttpResponse { - status: 200, - body: "not json at all".to_string(), - }; - let result: Result = response.json(); + let response = HttpResponse::new(200, "not json at all"); + let result: Result = response.json().map_err(Error::from); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("JSON parse error")); } #[test] fn test_http_response_boundary_status_codes() { - // 199 is not success - assert!(!HttpResponse { - status: 199, - body: String::new() - } - .is_success()); - // 200 is success - assert!(HttpResponse { - status: 200, - body: String::new() - } - .is_success()); - // 299 is success - assert!(HttpResponse { - status: 299, - body: String::new() - } - .is_success()); - // 300 is not success - assert!(!HttpResponse { - status: 300, - body: String::new() - } - .is_success()); + assert!(!HttpResponse::new(199, "").is_success()); + assert!(HttpResponse::new(200, "").is_success()); + assert!(HttpResponse::new(299, "").is_success()); + assert!(!HttpResponse::new(300, "").is_success()); } #[tokio::test] @@ -2699,8 +2646,6 @@ mod tests { assert!(result.reason.contains("Top issue")); } - // --- New tests for coverage --- - #[tokio::test] async fn test_build_issue_context_stacktrace_missing_fields() { // Exercises stacktrace frame rendering when function/filename/lineNo/colNo From e02fcbf8e920d69689b2b6b471cc39a081f67e0a Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Mon, 5 Oct 2026 18:33:17 +1300 Subject: [PATCH 2/5] refactor(integrations): move the HTTP consumers onto abnegate-http The integrations crate's GitHub, GitLab, HelpScout, Discord, notifier and source clients used claudear-core's local HttpClient, HttpResponse and ReqwestHttpClient, which abnegate-http 0.1.2 now provides. - Imports point at abnegate_http. The crate's HttpResponse is non_exhaustive, so every response is built with HttpResponse::new. - The HttpClient test doubles (github.rs, gitlab.rs, source/github.rs, source/gitlab.rs, source/helpscout.rs) answer with abnegate_http::Result, the trait's own result type. Production code converts with `?` through the one From impl in claudear-core, so JSON errors keep their variant and message. - The crate's ReqwestHttpClient::new returns a Result and the crate has no Default. GitHubClient, GitLabClient and HelpScoutSource keep their infallible constructors: they fall back to reqwest's default client when the configured one fails to build, exactly as the local ReqwestHttpClient::new did. The trusted transport is kept on purpose; PublicClient would refuse self-hosted GitLab, Sentry and Jira on private addresses. - The crate caps response bodies at 8 MiB, where the local client read them whole. GitHubClient and GitLabClient fetch PR diffs, so their transport reads up to scm::BODY_LIMIT, eight times the crate default (64 MiB). A body over the cap is now an error (Error::Other, not retried) rather than a success. Behaviour of requests that go through ReqwestHttpClient (GitHub, GitLab, HelpScout) changes in ways the crate brings: - A body that cannot be read to the end is an error (Error::Http, retried) rather than an empty successful body. - Bodies decode as UTF-8 with invalid sequences replaced, where reqwest honoured a charset from Content-Type; every API involved sends UTF-8. - Transport errors no longer carry the request URL. - Timeouts (30s, 10s connect), redirects (reqwest's 10 hops), proxy handling and the absent user agent are unchanged. The Discord, Slack, SMS, Telegram, WhatsApp, Jira, Linear and Sentry clients build HttpResponse from their own reqwest calls, so they keep their previous body handling. Tests pin that GitHubClient::new and GitLabClient::new configure their transport with BODY_LIMIT, and drive a transport with that limit against a loopback server: an 8 MiB + 1 byte diff is read whole, a declared length over BODY_LIMIT is refused, and a body cut short is an Error::Http. The loopback transport is built with no_proxy so an HTTP_PROXY in the environment cannot reroute the requests. Touched files also lose their section header comments and the abbreviated names around the changed lines (k/v in the GitHub and GitLab doubles, resp/tok/r/e in HelpScout's source, resp_body in WhatsApp, now text so it no longer shadows the body parameter). notifier/telegram.rs keeps its headers and changes only in its imports and HttpResponse construction, because the Telegram escaping fix stacks on this branch. Overlaps: #147 (source/sentry.rs, via the first commit), #162 (notifier/discord.rs) and #45 (discord/client.rs, notifiers, source/{jira,linear,sentry}.rs). Co-Authored-By: Claude Opus 5.5 --- crates/claudear-integrations/src/deploy_qa.rs | 20 +- .../src/discord/client.rs | 154 ++++------- .../src/discord/thread_manager.rs | 68 ++--- crates/claudear-integrations/src/github.rs | 255 +++++++++--------- crates/claudear-integrations/src/gitlab.rs | 205 +++++++------- crates/claudear-integrations/src/lib.rs | 2 + .../src/notifier/discord.rs | 65 ++--- .../src/notifier/slack.rs | 59 ++-- .../claudear-integrations/src/notifier/sms.rs | 40 +-- .../src/notifier/telegram.rs | 39 +-- .../src/notifier/whatsapp.rs | 63 ++--- crates/claudear-integrations/src/scm.rs | 28 +- .../src/source/github.rs | 53 ++-- .../src/source/gitlab.rs | 26 +- .../src/source/helpscout.rs | 114 ++++---- .../claudear-integrations/src/source/jira.rs | 93 ++----- .../src/source/linear.rs | 96 ++----- .../claudear-integrations/src/test_support.rs | 57 ++++ 18 files changed, 602 insertions(+), 835 deletions(-) create mode 100644 crates/claudear-integrations/src/test_support.rs diff --git a/crates/claudear-integrations/src/deploy_qa.rs b/crates/claudear-integrations/src/deploy_qa.rs index 53400855..5a7a705c 100644 --- a/crates/claudear-integrations/src/deploy_qa.rs +++ b/crates/claudear-integrations/src/deploy_qa.rs @@ -464,14 +464,17 @@ pub fn try_build_discord( #[cfg(test)] mod tests { use super::*; + use abnegate_http::HttpResponse; use async_trait::async_trait; - use claudear_analysis::deploy_qa::{ - VERDICT_ALL_VERIFIED, VERDICT_FAIL, VERDICT_PREFIX, VERDICT_UNVERIFIED, - }; + use claudear_analysis::deploy_qa::VERDICT_ALL_VERIFIED; + use claudear_analysis::deploy_qa::VERDICT_FAIL; + use claudear_analysis::deploy_qa::VERDICT_PREFIX; + use claudear_analysis::deploy_qa::VERDICT_UNVERIFIED; use claudear_core::error::Error; - use claudear_core::http::HttpResponse; - use serde_json::{json, Value}; - use std::sync::{Arc, Mutex}; + use serde_json::json; + use serde_json::Value; + use std::sync::Arc; + use std::sync::Mutex; const CHANNEL: &str = "990878183580651571"; const RELEASE_MESSAGE: &str = "1111"; @@ -519,10 +522,7 @@ mod tests { } fn respond(status: u16, body: Value) -> Result { - Ok(HttpResponse { - status, - body: body.to_string(), - }) + Ok(HttpResponse::new(status, body.to_string())) } #[async_trait] diff --git a/crates/claudear-integrations/src/discord/client.rs b/crates/claudear-integrations/src/discord/client.rs index a0f64ac9..10899554 100644 --- a/crates/claudear-integrations/src/discord/client.rs +++ b/crates/claudear-integrations/src/discord/client.rs @@ -1,12 +1,18 @@ //! Discord API client for thread management. -use super::types::{ - CreateMessageParams, CreateThreadParams, DiscordChannel, DiscordMessage, DiscordThread, -}; +use super::types::CreateMessageParams; +use super::types::CreateThreadParams; +use super::types::DiscordChannel; +use super::types::DiscordMessage; +use super::types::DiscordThread; +use abnegate_http::HttpResponse; use async_trait::async_trait; -use claudear_core::error::{Error, Result}; -use claudear_core::http::HttpResponse; -use reqwest::header::{HeaderMap, HeaderValue, AUTHORIZATION, CONTENT_TYPE}; +use claudear_core::error::Error; +use claudear_core::error::Result; +use reqwest::header::HeaderMap; +use reqwest::header::HeaderValue; +use reqwest::header::AUTHORIZATION; +use reqwest::header::CONTENT_TYPE; const DISCORD_API_BASE: &str = "https://discord.com/api/v10"; @@ -53,28 +59,28 @@ impl DiscordHttpClient for ReqwestDiscordClient { let response = self.client.get(url).send().await?; let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } async fn post(&self, url: &str, body: serde_json::Value) -> Result { let response = self.client.post(url).json(&body).send().await?; let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } async fn patch(&self, url: &str, body: serde_json::Value) -> Result { let response = self.client.patch(url).json(&body).send().await?; let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } async fn put_empty(&self, url: &str) -> Result { let response = self.client.put(url).send().await?; let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } } @@ -127,7 +133,7 @@ impl DiscordClient { )); } - response.json() + Ok(response.json()?) } /// Create a thread in a channel (without a starting message). @@ -152,7 +158,7 @@ impl DiscordClient { )); } - response.json() + Ok(response.json()?) } /// Create a thread from an existing message. @@ -181,7 +187,7 @@ impl DiscordClient { )); } - response.json() + Ok(response.json()?) } /// Get a thread by ID. @@ -199,7 +205,7 @@ impl DiscordClient { )); } - response.json() + Ok(response.json()?) } /// Send a message to a channel or thread. @@ -224,7 +230,7 @@ impl DiscordClient { )); } - response.json() + Ok(response.json()?) } /// Fetch a single message by ID from a channel. @@ -245,7 +251,7 @@ impl DiscordClient { )); } - response.json() + Ok(response.json()?) } /// List recent messages from a channel. @@ -271,7 +277,7 @@ impl DiscordClient { )); } - response.json() + Ok(response.json()?) } /// List messages from a channel after a given message ID (for incremental polling). @@ -379,7 +385,7 @@ impl DiscordClient { )); } - response.json() + Ok(response.json()?) } /// Unarchive a thread. @@ -400,7 +406,7 @@ impl DiscordClient { )); } - response.json() + Ok(response.json()?) } /// List active threads in a channel. @@ -565,33 +571,24 @@ pub mod mock { } pub fn mock_get(&self, url: impl Into, status: u16, body: impl Into) { - self.get_responses.lock().unwrap().insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + self.get_responses + .lock() + .unwrap() + .insert(url.into(), HttpResponse::new(status, body)); } pub fn mock_post(&self, url: impl Into, status: u16, body: impl Into) { - self.post_responses.lock().unwrap().insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + self.post_responses + .lock() + .unwrap() + .insert(url.into(), HttpResponse::new(status, body)); } pub fn mock_patch(&self, url: impl Into, status: u16, body: impl Into) { - self.patch_responses.lock().unwrap().insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + self.patch_responses + .lock() + .unwrap() + .insert(url.into(), HttpResponse::new(status, body)); } } @@ -599,54 +596,33 @@ pub mod mock { impl DiscordHttpClient for MockDiscordClient { async fn get(&self, url: &str) -> Result { let responses = self.get_responses.lock().unwrap(); - if let Some(r) = responses.get(url) { - Ok(HttpResponse { - status: r.status, - body: r.body.clone(), - }) + if let Some(response) = responses.get(url) { + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } async fn post(&self, url: &str, _body: serde_json::Value) -> Result { let responses = self.post_responses.lock().unwrap(); - if let Some(r) = responses.get(url) { - Ok(HttpResponse { - status: r.status, - body: r.body.clone(), - }) + if let Some(response) = responses.get(url) { + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } async fn patch(&self, url: &str, _body: serde_json::Value) -> Result { let responses = self.patch_responses.lock().unwrap(); - if let Some(r) = responses.get(url) { - Ok(HttpResponse { - status: r.status, - body: r.body.clone(), - }) + if let Some(response) = responses.get(url) { + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } async fn put_empty(&self, _url: &str) -> Result { - Ok(HttpResponse { - status: 204, - body: String::new(), - }) + Ok(HttpResponse::new(204, "")) } } } @@ -670,45 +646,23 @@ mod tests { #[test] fn test_http_response_is_success() { - assert!(HttpResponse { - status: 200, - body: "".to_string() - } - .is_success()); - assert!(HttpResponse { - status: 201, - body: "".to_string() - } - .is_success()); - assert!(!HttpResponse { - status: 400, - body: "".to_string() - } - .is_success()); - assert!(!HttpResponse { - status: 500, - body: "".to_string() - } - .is_success()); + assert!(HttpResponse::new(200, "").is_success()); + assert!(HttpResponse::new(201, "").is_success()); + assert!(!HttpResponse::new(400, "").is_success()); + assert!(!HttpResponse::new(500, "").is_success()); } #[test] fn test_http_response_json() { - let response = HttpResponse { - status: 200, - body: r#"{"id": "123"}"#.to_string(), - }; + let response = HttpResponse::new(200, r#"{"id": "123"}"#); let parsed: serde_json::Value = response.json().unwrap(); assert_eq!(parsed["id"], "123"); } #[test] fn test_http_response_json_error() { - let response = HttpResponse { - status: 200, - body: "invalid".to_string(), - }; - let result: Result = response.json(); + let response = HttpResponse::new(200, "invalid"); + let result: Result = response.json().map_err(Error::from); assert!(result.is_err()); } diff --git a/crates/claudear-integrations/src/discord/thread_manager.rs b/crates/claudear-integrations/src/discord/thread_manager.rs index cc085cc9..42d56440 100644 --- a/crates/claudear-integrations/src/discord/thread_manager.rs +++ b/crates/claudear-integrations/src/discord/thread_manager.rs @@ -1562,8 +1562,8 @@ mod tests { /// Shared state for the capturing mock, held via Arc so tests can /// inspect captured requests after passing the mock to the manager. struct CapturedRequests { - post_responses: std::sync::Mutex>, - patch_responses: std::sync::Mutex>, + post_responses: std::sync::Mutex>, + patch_responses: std::sync::Mutex>, captured_posts: std::sync::Mutex>, captured_patches: std::sync::Mutex>, } @@ -1579,23 +1579,17 @@ mod tests { } fn mock_post(&self, url: impl Into, status: u16, body: impl Into) { - self.post_responses.lock().unwrap().insert( - url.into(), - claudear_core::http::HttpResponse { - status, - body: body.into(), - }, - ); + self.post_responses + .lock() + .unwrap() + .insert(url.into(), abnegate_http::HttpResponse::new(status, body)); } fn mock_patch(&self, url: impl Into, status: u16, body: impl Into) { - self.patch_responses.lock().unwrap().insert( - url.into(), - claudear_core::http::HttpResponse { - status, - body: body.into(), - }, - ); + self.patch_responses + .lock() + .unwrap() + .insert(url.into(), abnegate_http::HttpResponse::new(status, body)); } fn get_captured_posts(&self) -> Vec<(String, serde_json::Value)> { @@ -1624,68 +1618,50 @@ mod tests { async fn get( &self, _url: &str, - ) -> claudear_core::error::Result { - Ok(claudear_core::http::HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + ) -> claudear_core::error::Result { + Ok(abnegate_http::HttpResponse::new(404, "Not found")) } async fn post( &self, url: &str, body: serde_json::Value, - ) -> claudear_core::error::Result { + ) -> claudear_core::error::Result { self.inner .captured_posts .lock() .unwrap() .push((url.to_string(), body)); let responses = self.inner.post_responses.lock().unwrap(); - if let Some(r) = responses.get(url) { - Ok(claudear_core::http::HttpResponse { - status: r.status, - body: r.body.clone(), - }) + if let Some(response) = responses.get(url) { + Ok(response.clone()) } else { - Ok(claudear_core::http::HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(abnegate_http::HttpResponse::new(404, "Not found")) } } async fn put_empty( &self, _url: &str, - ) -> claudear_core::error::Result { - Ok(claudear_core::http::HttpResponse { - status: 204, - body: String::new(), - }) + ) -> claudear_core::error::Result { + Ok(abnegate_http::HttpResponse::new(204, "")) } async fn patch( &self, url: &str, body: serde_json::Value, - ) -> claudear_core::error::Result { + ) -> claudear_core::error::Result { self.inner .captured_patches .lock() .unwrap() .push((url.to_string(), body)); let responses = self.inner.patch_responses.lock().unwrap(); - if let Some(r) = responses.get(url) { - Ok(claudear_core::http::HttpResponse { - status: r.status, - body: r.body.clone(), - }) + if let Some(response) = responses.get(url) { + Ok(response.clone()) } else { - Ok(claudear_core::http::HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(abnegate_http::HttpResponse::new(404, "Not found")) } } } diff --git a/crates/claudear-integrations/src/github.rs b/crates/claudear-integrations/src/github.rs index 464a7f0f..c5bdc803 100644 --- a/crates/claudear-integrations/src/github.rs +++ b/crates/claudear-integrations/src/github.rs @@ -1,12 +1,21 @@ //! GitHub PR monitoring and issue resolution. -use crate::scm::{ - CodeReview, InlineReviewComment, PostReviewAction, PrSummary, RemoteRepo, ReviewComment, - ReviewUser, ScmProvider, ScmRelease, -}; +use crate::scm::CodeReview; +use crate::scm::InlineReviewComment; +use crate::scm::PostReviewAction; +use crate::scm::PrSummary; +use crate::scm::RemoteRepo; +use crate::scm::ReviewComment; +use crate::scm::ReviewUser; +use crate::scm::ScmProvider; +use crate::scm::ScmRelease; +use crate::scm::BODY_LIMIT; +use abnegate_http::HttpClient; +use abnegate_http::ReqwestHttpClient; use async_trait::async_trait; use claudear_config::config::GitHubConfig; -use claudear_core::error::{Error, Result}; +use claudear_core::error::Error; +use claudear_core::error::Result; use claudear_core::secret::OptionalSecretExt; use serde::Deserialize; @@ -17,9 +26,6 @@ pub use crate::scm::{ ReviewWatcher, }; -// Backward-compatibility re-exports (types moved to http module) -pub use claudear_core::http::{HttpClient, ReqwestHttpClient}; - /// GitHub API client for PR monitoring. pub struct GitHubClient { config: GitHubConfig, @@ -82,7 +88,9 @@ impl GitHubClient { pub fn new(config: GitHubConfig) -> Self { Self { config, - http: ReqwestHttpClient::new(), + http: ReqwestHttpClient::new() + .unwrap_or_else(|_| ReqwestHttpClient::from(reqwest::Client::new())) + .with_body_limit(BODY_LIMIT), self_login: std::sync::OnceLock::new(), } } @@ -917,7 +925,7 @@ impl GitHubClient { return Err(Error::Other(format!("GitHub API error: {}", response.body))); } - response.json() + Ok(response.json()?) } /// Close an issue. @@ -1165,9 +1173,13 @@ impl ScmProvider for GitHubClient { #[cfg(test)] mod tests { use super::*; - use claudear_core::http::HttpResponse; + use crate::test_support::loopback_transport; + use crate::test_support::ok_response; + use crate::test_support::serve_once; + use abnegate_http::HttpResponse; use std::collections::HashMap; - use std::sync::{Arc, Mutex}; + use std::sync::Arc; + use std::sync::Mutex; /// Mock HTTP client for testing. #[expect(clippy::type_complexity)] @@ -1187,13 +1199,7 @@ mod tests { /// Add a mock response for a URL. pub fn mock_response(&self, url: impl Into, status: u16, body: impl Into) { let mut responses = self.responses.lock().unwrap(); - responses.insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + responses.insert(url.into(), HttpResponse::new(status, body)); } /// Get recorded requests. @@ -1204,29 +1210,25 @@ mod tests { #[async_trait] impl HttpClient for MockHttpClient { - async fn get(&self, url: &str, headers: Vec<(&str, String)>) -> Result { - // Record the request + async fn get( + &self, + url: &str, + headers: Vec<(&str, String)>, + ) -> abnegate_http::Result { let owned_headers: Vec<(String, String)> = headers .iter() - .map(|(k, v)| (k.to_string(), v.clone())) + .map(|(name, value)| (name.to_string(), value.clone())) .collect(); self.requests .lock() .unwrap() .push((url.to_string(), owned_headers)); - // Return mock response let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } @@ -1235,10 +1237,10 @@ mod tests { url: &str, headers: Vec<(&str, String)>, _body: &str, - ) -> Result { + ) -> abnegate_http::Result { let owned_headers: Vec<(String, String)> = headers .iter() - .map(|(k, v)| (k.to_string(), v.clone())) + .map(|(name, value)| (name.to_string(), value.clone())) .collect(); self.requests .lock() @@ -1247,15 +1249,9 @@ mod tests { let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } @@ -1264,10 +1260,10 @@ mod tests { url: &str, headers: Vec<(&str, String)>, _body: &str, - ) -> Result { + ) -> abnegate_http::Result { let owned_headers: Vec<(String, String)> = headers .iter() - .map(|(k, v)| (k.to_string(), v.clone())) + .map(|(name, value)| (name.to_string(), value.clone())) .collect(); self.requests .lock() @@ -1276,15 +1272,9 @@ mod tests { let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } @@ -1293,10 +1283,10 @@ mod tests { url: &str, headers: Vec<(&str, String)>, _body: &str, - ) -> Result { + ) -> abnegate_http::Result { let owned_headers: Vec<(String, String)> = headers .iter() - .map(|(k, v)| (k.to_string(), v.clone())) + .map(|(name, value)| (name.to_string(), value.clone())) .collect(); self.requests .lock() @@ -1305,22 +1295,20 @@ mod tests { let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } - async fn delete(&self, url: &str, headers: Vec<(&str, String)>) -> Result { + async fn delete( + &self, + url: &str, + headers: Vec<(&str, String)>, + ) -> abnegate_http::Result { let owned_headers: Vec<(String, String)> = headers .iter() - .map(|(k, v)| (k.to_string(), v.clone())) + .map(|(name, value)| (name.to_string(), value.clone())) .collect(); self.requests .lock() @@ -1329,78 +1317,48 @@ mod tests { let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } } #[test] fn test_http_response_is_success() { - let response = HttpResponse { - status: 200, - body: "{}".to_string(), - }; + let response = HttpResponse::new(200, "{}"); assert!(response.is_success()); - let response = HttpResponse { - status: 201, - body: "{}".to_string(), - }; + let response = HttpResponse::new(201, "{}"); assert!(response.is_success()); - let response = HttpResponse { - status: 404, - body: "{}".to_string(), - }; + let response = HttpResponse::new(404, "{}"); assert!(!response.is_success()); - let response = HttpResponse { - status: 500, - body: "{}".to_string(), - }; + let response = HttpResponse::new(500, "{}"); assert!(!response.is_success()); } #[test] fn test_http_response_is_not_found() { - let response = HttpResponse { - status: 404, - body: "{}".to_string(), - }; + let response = HttpResponse::new(404, "{}"); assert!(response.is_not_found()); - let response = HttpResponse { - status: 200, - body: "{}".to_string(), - }; + let response = HttpResponse::new(200, "{}"); assert!(!response.is_not_found()); } #[test] fn test_http_response_json() { - let response = HttpResponse { - status: 200, - body: r#"{"name": "test"}"#.to_string(), - }; + let response = HttpResponse::new(200, r#"{"name": "test"}"#); let parsed: serde_json::Value = response.json().unwrap(); assert_eq!(parsed["name"], "test"); } #[test] fn test_http_response_json_error() { - let response = HttpResponse { - status: 200, - body: "invalid json".to_string(), - }; - let result: Result = response.json(); + let response = HttpResponse::new(200, "invalid json"); + let result: Result = response.json().map_err(Error::from); assert!(result.is_err()); } @@ -4064,7 +4022,68 @@ mod tests { } } - // --- merge_pr tests --- + #[test] + fn the_default_transport_reads_up_to_the_scm_limit() { + let client = GitHubClient::new(test_config()); + + let transport = format!("{:?}", client.http); + + assert!( + transport.contains(&format!("body_limit: {BODY_LIMIT}")), + "{transport}" + ); + } + + #[tokio::test] + async fn the_scm_limit_admits_a_diff_larger_than_the_crate_default() { + let size = ReqwestHttpClient::DEFAULT_BODY_LIMIT + 1; + let url = serve_once(ok_response(size, &vec![b'+'; size])).await; + let client = GitHubClient::with_http_client(test_config(), loopback_transport(BODY_LIMIT)); + + let response = client + .http + .get(&url, Vec::new()) + .await + .expect("a diff under the SCM limit is read"); + + assert_eq!(response.body.len(), size); + } + + #[tokio::test] + async fn a_body_cut_short_is_a_transient_error_rather_than_an_empty_success() { + let url = serve_once(ok_response(100, b"short")).await; + let client = GitHubClient::with_http_client(test_config(), loopback_transport(BODY_LIMIT)); + + let error = client + .http + .get(&url, Vec::new()) + .await + .expect_err("the body ended 95 bytes early"); + + let error = Error::from(error); + + assert!(matches!(error, Error::Http(_)), "{error:?}"); + } + + #[tokio::test] + async fn the_scm_limit_refuses_a_larger_body() { + let url = serve_once(ok_response(BODY_LIMIT + 1, b"")).await; + let client = GitHubClient::with_http_client(test_config(), loopback_transport(BODY_LIMIT)); + + let error = client + .http + .get(&url, Vec::new()) + .await + .expect_err("a declared length over the SCM limit is refused"); + + assert!( + matches!( + error, + abnegate_http::Error::OversizedBody { limit, .. } if limit == BODY_LIMIT + ), + "{error}" + ); + } #[tokio::test] async fn test_merge_pr_success() { @@ -4126,8 +4145,6 @@ mod tests { assert!(result.is_err()); } - // --- close_pr tests --- - #[tokio::test] async fn test_close_pr_success() { let mock = MockHttpClient::new(); @@ -4174,8 +4191,6 @@ mod tests { ); } - // --- delete_branch tests --- - #[tokio::test] async fn test_delete_branch_success() { let mock = MockHttpClient::new(); @@ -4240,8 +4255,6 @@ mod tests { ); } - // --- post_review tests --- - #[tokio::test] async fn test_post_review_comment_success() { let mock = MockHttpClient::new(); @@ -4331,8 +4344,6 @@ mod tests { ); } - // --- post_review_with_comments tests --- - #[tokio::test] async fn test_post_review_with_comments_success() { let mock = MockHttpClient::new(); @@ -4432,8 +4443,6 @@ mod tests { ); } - // --- list_open_prs tests --- - #[tokio::test] async fn test_list_open_prs_success() { let mock = MockHttpClient::new(); @@ -4502,8 +4511,6 @@ mod tests { ); } - // --- get_latest_release tests --- - #[tokio::test] async fn test_get_latest_release_success() { let mock = MockHttpClient::new(); @@ -4574,8 +4581,6 @@ mod tests { ); } - // --- create_release tests --- - #[tokio::test] async fn test_create_release_success() { let mock = MockHttpClient::new(); @@ -4639,8 +4644,6 @@ mod tests { ); } - // --- parse_pr_number tests --- - #[test] fn test_parse_pr_number_pull_url() { assert_eq!( @@ -4686,8 +4689,6 @@ mod tests { ); } - // --- list_repo_issues tests --- - #[tokio::test] async fn test_list_repo_issues_success() { let mock = MockHttpClient::new(); @@ -4798,8 +4799,6 @@ mod tests { ); } - // --- get_issue tests --- - #[tokio::test] async fn test_get_issue_success() { let mock = MockHttpClient::new(); @@ -4874,8 +4873,6 @@ mod tests { ); } - // --- close_issue tests --- - #[tokio::test] async fn test_close_issue_success() { let mock = MockHttpClient::new(); @@ -4922,8 +4919,6 @@ mod tests { ); } - // --- add_issue_comment tests --- - #[tokio::test] async fn test_add_issue_comment_success() { let mock = MockHttpClient::new(); @@ -5060,8 +5055,6 @@ mod tests { ); } - // --- ScmProvider trait delegation tests --- - #[tokio::test] async fn test_scm_provider_merge_pr() { let mock = MockHttpClient::new(); diff --git a/crates/claudear-integrations/src/gitlab.rs b/crates/claudear-integrations/src/gitlab.rs index 3f0d4ef8..c9b9c1b5 100644 --- a/crates/claudear-integrations/src/gitlab.rs +++ b/crates/claudear-integrations/src/gitlab.rs @@ -1,17 +1,25 @@ //! GitLab API client for MR monitoring and issue management. -use crate::scm::{ - CodeReview, PostReviewAction, PrInfo, PrStatus, PrSummary, RemoteRepo, ReviewComment, - ReviewUser, ScmProvider, -}; +use crate::scm::CodeReview; +use crate::scm::PostReviewAction; +use crate::scm::PrInfo; +use crate::scm::PrStatus; +use crate::scm::PrSummary; +use crate::scm::RemoteRepo; +use crate::scm::ReviewComment; +use crate::scm::ReviewUser; +use crate::scm::ScmProvider; +use crate::scm::BODY_LIMIT; +use abnegate_http::HttpClient; +use abnegate_http::ReqwestHttpClient; use async_trait::async_trait; use claudear_config::config::GitLabConfig; -use claudear_core::error::{Error, Result}; -use claudear_core::http::HttpClient; +use claudear_core::error::Error; +use claudear_core::error::Result; use serde::Deserialize; /// GitLab API client for MR monitoring. -pub struct GitLabClient { +pub struct GitLabClient { config: GitLabConfig, http: H, } @@ -105,12 +113,14 @@ pub struct GitLabIssue { pub assignees: Vec, } -impl GitLabClient { +impl GitLabClient { /// Create a new GitLab client with the default HTTP client. pub fn new(config: GitLabConfig) -> Self { Self { config, - http: claudear_core::http::ReqwestHttpClient::new(), + http: ReqwestHttpClient::new() + .unwrap_or_else(|_| ReqwestHttpClient::from(reqwest::Client::new())) + .with_body_limit(BODY_LIMIT), } } } @@ -208,7 +218,7 @@ impl GitLabClient { ))); } - response.json() + Ok(response.json()?) } /// Fetch group issues. @@ -252,7 +262,7 @@ impl GitLabClient { ))); } - response.json() + Ok(response.json()?) } /// Get a single issue. @@ -281,7 +291,7 @@ impl GitLabClient { ))); } - response.json() + Ok(response.json()?) } /// Get MR notes (comments) for mapping to reviews. @@ -365,7 +375,7 @@ impl GitLabClient { ))); } - response.json() + Ok(response.json()?) } /// Map a GitLab note to a CodeReview (for general notes). @@ -1023,8 +1033,11 @@ impl ScmProvider for GitLabClient { #[cfg(test)] mod tests { use super::*; + use crate::test_support::loopback_transport; + use crate::test_support::ok_response; + use crate::test_support::serve_once; + use abnegate_http::HttpResponse; use claudear_config::config::GitLabConfig; - use claudear_core::http::HttpResponse; use std::collections::HashMap; use std::sync::Mutex; @@ -1045,13 +1058,10 @@ mod tests { /// Add a mock response for a URL. fn mock_response(&self, url: impl Into, status: u16, body: impl Into) { - self.responses.lock().unwrap().insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + self.responses + .lock() + .unwrap() + .insert(url.into(), HttpResponse::new(status, body)); } /// Get captured headers for a given URL. @@ -1062,12 +1072,16 @@ mod tests { #[async_trait] impl HttpClient for MockHttpClient { - async fn get(&self, url: &str, headers: Vec<(&str, String)>) -> Result { + async fn get( + &self, + url: &str, + headers: Vec<(&str, String)>, + ) -> abnegate_http::Result { // Capture headers for inspection { let owned: Vec<(String, String)> = headers .iter() - .map(|(k, v)| (k.to_string(), v.clone())) + .map(|(name, value)| (name.to_string(), value.clone())) .collect(); self.captured_headers .lock() @@ -1077,15 +1091,9 @@ mod tests { let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } @@ -1094,11 +1102,11 @@ mod tests { url: &str, headers: Vec<(&str, String)>, _body: &str, - ) -> Result { + ) -> abnegate_http::Result { { let owned: Vec<(String, String)> = headers .iter() - .map(|(k, v)| (k.to_string(), v.clone())) + .map(|(name, value)| (name.to_string(), value.clone())) .collect(); self.captured_headers .lock() @@ -1107,15 +1115,9 @@ mod tests { } let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } @@ -1124,11 +1126,11 @@ mod tests { url: &str, headers: Vec<(&str, String)>, _body: &str, - ) -> Result { + ) -> abnegate_http::Result { { let owned: Vec<(String, String)> = headers .iter() - .map(|(k, v)| (k.to_string(), v.clone())) + .map(|(name, value)| (name.to_string(), value.clone())) .collect(); self.captured_headers .lock() @@ -1137,23 +1139,21 @@ mod tests { } let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } - async fn delete(&self, url: &str, headers: Vec<(&str, String)>) -> Result { + async fn delete( + &self, + url: &str, + headers: Vec<(&str, String)>, + ) -> abnegate_http::Result { { let owned: Vec<(String, String)> = headers .iter() - .map(|(k, v)| (k.to_string(), v.clone())) + .map(|(name, value)| (name.to_string(), value.clone())) .collect(); self.captured_headers .lock() @@ -1162,15 +1162,9 @@ mod tests { } let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } } @@ -1183,6 +1177,53 @@ mod tests { config } + #[test] + fn the_default_transport_reads_up_to_the_scm_limit() { + let client = GitLabClient::new(test_config()); + + let transport = format!("{:?}", client.http); + + assert!( + transport.contains(&format!("body_limit: {BODY_LIMIT}")), + "{transport}" + ); + } + + #[tokio::test] + async fn the_scm_limit_admits_a_diff_larger_than_the_crate_default() { + let size = ReqwestHttpClient::DEFAULT_BODY_LIMIT + 1; + let url = serve_once(ok_response(size, &vec![b'+'; size])).await; + let client = GitLabClient::with_http_client(test_config(), loopback_transport(BODY_LIMIT)); + + let response = client + .http + .get(&url, Vec::new()) + .await + .expect("a diff under the SCM limit is read"); + + assert_eq!(response.body.len(), size); + } + + #[tokio::test] + async fn the_scm_limit_refuses_a_larger_body() { + let url = serve_once(ok_response(BODY_LIMIT + 1, b"")).await; + let client = GitLabClient::with_http_client(test_config(), loopback_transport(BODY_LIMIT)); + + let error = client + .http + .get(&url, Vec::new()) + .await + .expect_err("a declared length over the SCM limit is refused"); + + assert!( + matches!( + error, + abnegate_http::Error::OversizedBody { limit, .. } if limit == BODY_LIMIT + ), + "{error}" + ); + } + #[tokio::test] async fn test_get_mr_status_open() { let mock = MockHttpClient::new(); @@ -1245,15 +1286,11 @@ mod tests { #[test] fn test_encode_project_path() { assert_eq!( - GitLabClient::::encode_project_path( - "group/repo" - ), + GitLabClient::::encode_project_path("group/repo"), "group%2Frepo" ); assert_eq!( - GitLabClient::::encode_project_path( - "group/subgroup/repo" - ), + GitLabClient::::encode_project_path("group/subgroup/repo"), "group%2Fsubgroup%2Frepo" ); } @@ -5481,8 +5518,6 @@ mod tests { ); } - // --- merge_pr tests --- - #[tokio::test] async fn test_merge_pr_success() { let mock = MockHttpClient::new(); @@ -5547,8 +5582,6 @@ mod tests { ); } - // --- close_pr tests --- - #[tokio::test] async fn test_close_pr_success() { let mock = MockHttpClient::new(); @@ -5594,8 +5627,6 @@ mod tests { ); } - // --- delete_branch tests --- - #[tokio::test] async fn test_delete_branch_success() { let mock = MockHttpClient::new(); @@ -5661,8 +5692,6 @@ mod tests { ); } - // --- post_review tests --- - #[tokio::test] async fn test_post_review_comment_success() { let mock = MockHttpClient::new(); @@ -5841,8 +5870,6 @@ mod tests { ); } - // --- list_open_prs tests --- - #[tokio::test] async fn test_list_open_prs_success() { let mock = MockHttpClient::new(); @@ -5911,8 +5938,6 @@ mod tests { ); } - // --- get_pr_branch tests --- - #[tokio::test] async fn test_get_pr_branch_success() { let mock = MockHttpClient::new(); @@ -5946,16 +5971,12 @@ mod tests { ); } - // --- pr_url_pattern tests --- - #[test] fn test_pr_url_pattern() { let client = GitLabClient::new(test_config()); assert_eq!(ScmProvider::pr_url_pattern(&client), "%-/merge_requests/%"); } - // --- parse_pr_number tests --- - #[test] fn test_parse_pr_number_valid() { let client = GitLabClient::new(test_config()); @@ -6001,8 +6022,6 @@ mod tests { ); } - // --- get_latest_release tests --- - #[tokio::test] async fn test_get_latest_release_success() { let mock = MockHttpClient::new(); @@ -6086,8 +6105,6 @@ mod tests { ); } - // --- create_release tests --- - #[tokio::test] async fn test_create_release_success() { let mock = MockHttpClient::new(); @@ -6154,8 +6171,6 @@ mod tests { ); } - // --- ScmProvider name test --- - #[test] fn test_scm_provider_name_is_gitlab() { let config = test_config(); @@ -6164,8 +6179,6 @@ mod tests { assert_eq!(provider.name(), "gitlab"); } - // --- get_mr_approvals no-token test --- - #[tokio::test] async fn test_get_mr_approvals_no_token_direct() { let client = GitLabClient::with_http_client(no_token_config(), MockHttpClient::new()); @@ -6178,8 +6191,6 @@ mod tests { ); } - // --- get_mr_notes no-token test --- - #[tokio::test] async fn test_get_mr_notes_no_token() { let client = GitLabClient::with_http_client(no_token_config(), MockHttpClient::new()); @@ -6192,8 +6203,6 @@ mod tests { ); } - // --- merge_pr via ScmProvider trait --- - #[tokio::test] async fn test_scm_provider_merge_pr() { let mock = MockHttpClient::new(); @@ -6209,8 +6218,6 @@ mod tests { assert!(result.is_ok()); } - // --- close_pr via ScmProvider trait --- - #[tokio::test] async fn test_scm_provider_close_pr() { let mock = MockHttpClient::new(); @@ -6226,8 +6233,6 @@ mod tests { assert!(result.is_ok()); } - // --- delete_branch via ScmProvider trait --- - #[tokio::test] async fn test_scm_provider_delete_branch() { let mock = MockHttpClient::new(); @@ -6243,8 +6248,6 @@ mod tests { assert!(result.is_ok()); } - // --- nested project path in write ops --- - #[tokio::test] async fn test_merge_pr_nested_project_path() { let mock = MockHttpClient::new(); diff --git a/crates/claudear-integrations/src/lib.rs b/crates/claudear-integrations/src/lib.rs index 8a2dfff8..b6cc275a 100644 --- a/crates/claudear-integrations/src/lib.rs +++ b/crates/claudear-integrations/src/lib.rs @@ -17,5 +17,7 @@ pub mod runner; pub mod scm; pub mod source; pub mod telemetry; +#[cfg(test)] +mod test_support; pub mod tls; pub mod webhook; diff --git a/crates/claudear-integrations/src/notifier/discord.rs b/crates/claudear-integrations/src/notifier/discord.rs index 5ea3a480..b280300d 100644 --- a/crates/claudear-integrations/src/notifier/discord.rs +++ b/crates/claudear-integrations/src/notifier/discord.rs @@ -2,15 +2,23 @@ use super::Notifier; use crate::ask_reply_inbox; -use crate::discord::{CreateMessageParams, DiscordClient, DiscordMessageReference, MessageEmbed}; +use crate::discord::CreateMessageParams; +use crate::discord::DiscordClient; +use crate::discord::DiscordMessageReference; +use crate::discord::MessageEmbed; use crate::reports::RepetitiveDigest; +use abnegate_http::HttpResponse; use async_trait::async_trait; -use chrono::{DateTime, Utc}; +use chrono::DateTime; +use chrono::Utc; use claudear_config::config::DiscordConfig; use claudear_config::users::UserRegistry; -use claudear_core::error::{Error, Result}; -use claudear_core::http::HttpResponse; -use claudear_core::types::{AskDelivery, AskReply, AskRequest, Issue}; +use claudear_core::error::Error; +use claudear_core::error::Result; +use claudear_core::types::AskDelivery; +use claudear_core::types::AskReply; +use claudear_core::types::AskRequest; +use claudear_core::types::Issue; use serde::Serialize; /// Trait for HTTP client used by Discord notifier. @@ -50,7 +58,7 @@ impl DiscordWebhookClient for ReqwestDiscordWebhookClient { let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } } @@ -1637,8 +1645,6 @@ mod tests { assert!(msgs[1].embeds.as_ref().unwrap()[0].title.is_none()); } - // --- Native reply reference (threading answers to the question) --- - #[test] fn test_origin_reply_reference_targets_original_message() { let issue = Issue::new( @@ -2020,10 +2026,10 @@ mod tests { .unwrap() .push((url.to_string(), body.clone())); - Ok(HttpResponse { - status: self.response_status, - body: self.response_body.clone(), - }) + Ok(HttpResponse::new( + self.response_status, + self.response_body.clone(), + )) } } @@ -2377,10 +2383,7 @@ mod tests { #[test] fn test_http_response_fields() { - let response = HttpResponse { - status: 201, - body: "Created".to_string(), - }; + let response = HttpResponse::new(201, "Created"); assert_eq!(response.status, 201); assert_eq!(response.body, "Created"); } @@ -2861,8 +2864,6 @@ mod tests { assert!(delivery.message_id.is_none()); } - // --- Additional tests for coverage --- - fn make_ask_request( correlation_id: &str, question: &str, @@ -3998,8 +3999,6 @@ mod tests { assert!(body["embeds"][0]["url"].is_null()); } - // --- Synchronous tests for standalone build_* helpers --- - fn test_issue() -> Issue { Issue::new( "42", @@ -4701,8 +4700,6 @@ mod tests { .contains("require attention")); } - // --- Tests for notify_merged message building --- - #[tokio::test] async fn test_notify_merged_sends_correct_embed() { let mock = MockDiscordWebhookClient::success(); @@ -4743,8 +4740,6 @@ mod tests { assert!(content.contains("<@987654321>")); } - // --- Tests for notify_closed message building --- - #[tokio::test] async fn test_notify_closed_sends_correct_embed() { let mock = MockDiscordWebhookClient::success(); @@ -4786,8 +4781,6 @@ mod tests { assert!(content.contains("<@987654321>")); } - // --- Tests for cascade success message --- - #[tokio::test] async fn test_notify_success_cascade_sends_cascade_embed() { let mock = MockDiscordWebhookClient::success(); @@ -4834,8 +4827,6 @@ mod tests { assert!(footer.contains("Cascade")); } - // --- Tests for cascade failed message --- - #[tokio::test] async fn test_notify_failed_cascade_sends_cascade_failed_embed() { let mock = MockDiscordWebhookClient::success(); @@ -4871,8 +4862,6 @@ mod tests { assert!(footer.contains("Cascade")); } - // --- Tests for regression detected message --- - #[tokio::test] async fn test_notify_failed_regression_sends_regression_embed() { let mock = MockDiscordWebhookClient::success(); @@ -4909,8 +4898,6 @@ mod tests { assert!(footer.contains("Regression Monitor")); } - // --- Tests for regression resolved message --- - #[tokio::test] async fn test_notify_completed_regression_resolved_sends_resolved_embed() { let mock = MockDiscordWebhookClient::success(); @@ -4933,8 +4920,6 @@ mod tests { assert!(footer.contains("Regression Monitor")); } - // --- Tests for is_pr_update path in success message --- - #[tokio::test] async fn test_notify_success_pr_update_sends_updated_title() { let mock = MockDiscordWebhookClient::success(); @@ -4955,8 +4940,6 @@ mod tests { assert!(pr_field["value"].as_str().unwrap().contains("View PR")); } - // --- Test to_create_message_params content truncation --- - #[test] fn test_to_create_message_params_truncates_long_content() { let long_content = "x".repeat(2500); @@ -5034,8 +5017,6 @@ mod tests { assert!(params.embeds.is_some()); } - // --- Test cascade success without optional fields --- - #[test] fn test_build_cascade_success_message_without_optional_metadata() { let issue = Issue::new("1", "LIN-1", "Fix", "https://example.com", "linear"); @@ -5065,8 +5046,6 @@ mod tests { assert_eq!(error_field.value, "Some error"); } - // --- Test build functions directly --- - #[test] fn test_build_start_message_fields() { let issue = Issue::new("1", "LIN-1", "Test Issue", "https://linear.app/1", "linear"); @@ -5200,8 +5179,6 @@ mod tests { assert_eq!(trigger.value, "Manual trigger"); } - // === Coverage: build_success_message with changelog metadata === - #[test] fn test_build_success_message_changelog_field() { let mut issue = Issue::new("1", "PROJ-1", "Test", "https://example.com", "linear"); @@ -5214,8 +5191,6 @@ mod tests { .any(|f| f.name == "Changes" && f.value.contains("Fixed auth bug"))); } - // === Coverage: build_completed_message with custom completion_reason === - #[test] fn test_build_completed_message_custom_reason() { let mut issue = Issue::new("1", "PROJ-1", "Test", "https://example.com", "linear"); @@ -5228,8 +5203,6 @@ mod tests { .any(|f| f.name == "Reason" && f.value.contains("Already fixed"))); } - // === Coverage: confidence field in success messages === - #[test] fn test_build_success_message_with_confidence() { let mut issue = Issue::new("1", "LIN-1", "Test", "https://linear.app/1", "linear"); diff --git a/crates/claudear-integrations/src/notifier/slack.rs b/crates/claudear-integrations/src/notifier/slack.rs index 29cc3425..4b474bdd 100644 --- a/crates/claudear-integrations/src/notifier/slack.rs +++ b/crates/claudear-integrations/src/notifier/slack.rs @@ -3,14 +3,20 @@ use super::get_source_emoji; use super::Notifier; use crate::reports::Report; +use abnegate_http::HttpResponse; use async_trait::async_trait; -use chrono::{DateTime, Utc}; +use chrono::DateTime; +use chrono::Utc; use claudear_config::config::SlackConfig; use claudear_config::users::UserRegistry; -use claudear_core::error::{Error, Result}; -use claudear_core::http::HttpResponse; -use claudear_core::types::{AskDelivery, AskReply, AskRequest, Issue}; -use serde::{Deserialize, Serialize}; +use claudear_core::error::Error; +use claudear_core::error::Result; +use claudear_core::types::AskDelivery; +use claudear_core::types::AskReply; +use claudear_core::types::AskRequest; +use claudear_core::types::Issue; +use serde::Deserialize; +use serde::Serialize; /// Trait for HTTP client used by Slack notifier. #[async_trait] @@ -65,7 +71,7 @@ impl SlackHttpClient for ReqwestSlackHttpClient { let response = req.send().await?; let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } async fn get_json(&self, url: &str, auth_token: Option<&str>) -> Result { @@ -76,7 +82,7 @@ impl SlackHttpClient for ReqwestSlackHttpClient { let response = req.send().await?; let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } } @@ -1422,10 +1428,10 @@ mod tests { auth_token.map(|s| s.to_string()), )); - Ok(HttpResponse { - status: self.response_status, - body: self.response_body.clone(), - }) + Ok(HttpResponse::new( + self.response_status, + self.response_body.clone(), + )) } async fn get_json(&self, url: &str, auth_token: Option<&str>) -> Result { @@ -1439,17 +1445,14 @@ mod tests { let responses = self.get_responses.lock().unwrap(); for (prefix, body) in responses.iter() { if url.starts_with(prefix) || url.contains(prefix) { - return Ok(HttpResponse { - status: self.response_status, - body: body.clone(), - }); + return Ok(HttpResponse::new(self.response_status, body.clone())); } } - Ok(HttpResponse { - status: self.response_status, - body: self.response_body.clone(), - }) + Ok(HttpResponse::new( + self.response_status, + self.response_body.clone(), + )) } } @@ -4299,8 +4302,6 @@ mod tests { assert!(trigger_block.is_some()); } - // === Coverage tests for build_closed_message === - #[test] fn test_build_closed_message_without_mention_v2() { let issue = test_issue(); @@ -4333,8 +4334,6 @@ mod tests { } } - // === Coverage tests for build_cascade_success_message === - #[test] fn test_build_cascade_success_message_basic() { let mut issue = test_issue(); @@ -4370,8 +4369,6 @@ mod tests { } } - // === Coverage tests for build_cascade_failed_message === - #[test] fn test_build_cascade_failed_message_basic() { let mut issue = test_issue(); @@ -4427,8 +4424,6 @@ mod tests { } } - // === Coverage tests for build_regression_detected_message === - #[test] fn test_build_regression_detected_message_basic() { let issue = test_issue(); @@ -4481,8 +4476,6 @@ mod tests { } } - // === Coverage tests for build_regression_resolved_message === - #[test] fn test_build_regression_resolved_message_basic() { let issue = test_issue(); @@ -4509,8 +4502,6 @@ mod tests { } } - // === Coverage tests for Notifier trait methods via mock HTTP === - #[tokio::test] async fn test_notify_merged_sends_correct_content() { let mock = MockSlackHttpClient::success(); @@ -4625,8 +4616,6 @@ mod tests { assert!(text.contains("Cascade Failed")); } - // === Coverage: build_success_message with is_pr_update and changelog === - #[test] fn test_build_success_message_pr_update_v2() { let mut issue = test_issue(); @@ -4646,8 +4635,6 @@ mod tests { assert!(block_json.contains("Fixed authentication bug")); } - // === Coverage: build_completed_message with custom completion_reason === - #[test] fn test_build_completed_message_with_custom_reason() { let mut issue = test_issue(); @@ -4658,8 +4645,6 @@ mod tests { assert!(block_json.contains("Already fixed in previous release")); } - // === Coverage: confidence in success messages === - #[test] fn test_build_success_message_with_confidence() { let mut issue = test_issue(); diff --git a/crates/claudear-integrations/src/notifier/sms.rs b/crates/claudear-integrations/src/notifier/sms.rs index babe22c5..a10a28b6 100644 --- a/crates/claudear-integrations/src/notifier/sms.rs +++ b/crates/claudear-integrations/src/notifier/sms.rs @@ -1,12 +1,15 @@ //! SMS notifier via Twilio. use super::Notifier; +use abnegate_http::HttpResponse; use async_trait::async_trait; use claudear_config::config::SmsConfig; use claudear_config::users::UserRegistry; -use claudear_core::error::{Error, Result}; -use claudear_core::http::HttpResponse; -use claudear_core::types::{AskDelivery, AskRequest, Issue}; +use claudear_core::error::Error; +use claudear_core::error::Result; +use claudear_core::types::AskDelivery; +use claudear_core::types::AskRequest; +use claudear_core::types::Issue; /// Trait for HTTP client used by SMS notifier. #[async_trait] @@ -63,7 +66,7 @@ impl SmsHttpClient for ReqwestSmsClient { let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } } @@ -400,10 +403,10 @@ mod tests { params_owned, )); - Ok(HttpResponse { - status: self.response_status, - body: self.response_body.clone(), - }) + Ok(HttpResponse::new( + self.response_status, + self.response_body.clone(), + )) } } @@ -963,10 +966,7 @@ mod tests { #[test] fn test_http_response_fields() { - let response = HttpResponse { - status: 201, - body: "Created".to_string(), - }; + let response = HttpResponse::new(201, "Created"); assert_eq!(response.status, 201); assert_eq!(response.body, "Created"); } @@ -1185,8 +1185,6 @@ mod tests { assert_eq!(to_param.1, "+15550009999"); } - // --- Tests for cascade success message --- - #[tokio::test] async fn test_notify_success_cascade_message_format() { let mock = MockSmsClient::success(); @@ -1207,8 +1205,6 @@ mod tests { assert!(body.contains("https://github.com/downstream/repo/pull/5")); } - // --- Tests for PR update success message --- - #[tokio::test] async fn test_notify_success_pr_update_message_format() { let mock = MockSmsClient::success(); @@ -1228,8 +1224,6 @@ mod tests { assert!(body.contains("https://github.com/org/repo/pull/77")); } - // --- Tests for regression resolved completed message --- - #[tokio::test] async fn test_notify_completed_regression_resolved_message_format() { let mock = MockSmsClient::success(); @@ -1246,8 +1240,6 @@ mod tests { assert!(body.contains("no regression")); } - // --- Tests for regression detected failed message --- - #[tokio::test] async fn test_notify_failed_regression_detected_message_format() { let mock = MockSmsClient::success(); @@ -1267,8 +1259,6 @@ mod tests { assert!(body.contains("Tests failing again")); } - // --- Tests for cascade failed message --- - #[tokio::test] async fn test_notify_failed_cascade_message_format() { let mock = MockSmsClient::success(); @@ -1286,8 +1276,6 @@ mod tests { assert!(body.contains("Build error")); } - // --- Tests for notify_merged and notify_closed --- - #[tokio::test] async fn test_notify_merged_message_format() { let mock = MockSmsClient::success(); @@ -1324,8 +1312,6 @@ mod tests { assert!(body.contains("https://github.com/org/repo/pull/43")); } - // --- Test failed cascade with long error truncation --- - #[tokio::test] async fn test_notify_failed_cascade_truncates_long_error() { let mock = MockSmsClient::success(); @@ -1342,8 +1328,6 @@ mod tests { assert!(body.contains("...")); } - // --- Test regression with long error truncation --- - #[tokio::test] async fn test_notify_failed_regression_truncates_long_error() { let mock = MockSmsClient::success(); diff --git a/crates/claudear-integrations/src/notifier/telegram.rs b/crates/claudear-integrations/src/notifier/telegram.rs index 3ace1c30..50da2292 100644 --- a/crates/claudear-integrations/src/notifier/telegram.rs +++ b/crates/claudear-integrations/src/notifier/telegram.rs @@ -2,13 +2,19 @@ use super::Notifier; use crate::ask_reply_inbox; +use abnegate_http::HttpResponse; use async_trait::async_trait; -use chrono::{DateTime, TimeZone, Utc}; +use chrono::DateTime; +use chrono::TimeZone; +use chrono::Utc; use claudear_config::config::TelegramConfig; use claudear_config::users::UserRegistry; -use claudear_core::error::{Error, Result}; -use claudear_core::http::HttpResponse; -use claudear_core::types::{AskDelivery, AskReply, AskRequest, Issue}; +use claudear_core::error::Error; +use claudear_core::error::Result; +use claudear_core::types::AskDelivery; +use claudear_core::types::AskReply; +use claudear_core::types::AskRequest; +use claudear_core::types::Issue; use serde::Deserialize; use std::collections::HashSet; use std::sync::RwLock; @@ -51,7 +57,7 @@ impl TelegramHttpClient for ReqwestTelegramClient { let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } async fn get_json(&self, url: &str) -> Result { @@ -60,7 +66,7 @@ impl TelegramHttpClient for ReqwestTelegramClient { let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } } @@ -670,19 +676,19 @@ mod tests { .unwrap() .push((url.to_string(), body.clone())); - Ok(HttpResponse { - status: self.post_response_status, - body: self.post_response_body.clone(), - }) + Ok(HttpResponse::new( + self.post_response_status, + self.post_response_body.clone(), + )) } async fn get_json(&self, url: &str) -> Result { self.call_count.fetch_add(1, Ordering::SeqCst); self.get_calls.lock().unwrap().push(url.to_string()); - Ok(HttpResponse { - status: self.get_response_status, - body: self.get_response_body.clone(), - }) + Ok(HttpResponse::new( + self.get_response_status, + self.get_response_body.clone(), + )) } } @@ -1825,10 +1831,7 @@ mod tests { #[test] fn test_http_response_fields() { - let response = HttpResponse { - status: 201, - body: "Created".to_string(), - }; + let response = HttpResponse::new(201, "Created"); assert_eq!(response.status, 201); assert_eq!(response.body, "Created"); } diff --git a/crates/claudear-integrations/src/notifier/whatsapp.rs b/crates/claudear-integrations/src/notifier/whatsapp.rs index 73b8000e..45fd2daa 100644 --- a/crates/claudear-integrations/src/notifier/whatsapp.rs +++ b/crates/claudear-integrations/src/notifier/whatsapp.rs @@ -2,13 +2,18 @@ use super::Notifier; use crate::ask_reply_inbox; +use abnegate_http::HttpResponse; use async_trait::async_trait; -use chrono::{DateTime, Utc}; +use chrono::DateTime; +use chrono::Utc; use claudear_config::config::WhatsAppConfig; use claudear_config::users::UserRegistry; -use claudear_core::error::{Error, Result}; -use claudear_core::http::HttpResponse; -use claudear_core::types::{AskDelivery, AskReply, AskRequest, Issue}; +use claudear_core::error::Error; +use claudear_core::error::Result; +use claudear_core::types::AskDelivery; +use claudear_core::types::AskReply; +use claudear_core::types::AskRequest; +use claudear_core::types::Issue; use serde::Deserialize; use std::collections::HashSet; @@ -74,12 +79,9 @@ impl WhatsAppHttpClient for ReqwestWhatsAppClient { .await?; let status = response.status().as_u16(); - let resp_body = response.text().await.unwrap_or_default(); + let text = response.text().await.unwrap_or_default(); - Ok(HttpResponse { - status, - body: resp_body, - }) + Ok(HttpResponse::new(status, text)) } } @@ -486,10 +488,10 @@ mod tests { body.clone(), )); - Ok(HttpResponse { - status: self.response_status, - body: self.response_body.clone(), - }) + Ok(HttpResponse::new( + self.response_status, + self.response_body.clone(), + )) } } @@ -581,8 +583,6 @@ mod tests { } } - // --- Basic trait tests --- - #[test] fn test_name() { let notifier = WhatsAppNotifier::new(disabled_config(), empty_registry()); @@ -607,8 +607,6 @@ mod tests { assert!(!WhatsAppNotifier::new(partial_config_no_to(), empty_registry()).is_enabled()); } - // --- Disabled config tests (silent no-op) --- - #[tokio::test] async fn test_notify_start_disabled() { let notifier = WhatsAppNotifier::new(disabled_config(), empty_registry()); @@ -710,8 +708,6 @@ mod tests { assert!(notifier.is_enabled()); } - // --- Mock-based tests for HTTP-dependent functionality --- - #[tokio::test] async fn test_send_message_success() { let mock = MockWhatsAppClient::success(); @@ -1334,8 +1330,6 @@ mod tests { assert_eq!(to, "+15550009999"); } - // --- Tests for cascade success message --- - #[tokio::test] async fn test_notify_success_cascade_message_format() { let mock = MockWhatsAppClient::success(); @@ -1356,8 +1350,6 @@ mod tests { assert!(body.contains("https://github.com/downstream/repo/pull/5")); } - // --- Tests for PR update success message --- - #[tokio::test] async fn test_notify_success_pr_update_message_format() { let mock = MockWhatsAppClient::success(); @@ -1377,8 +1369,6 @@ mod tests { assert!(body.contains("https://github.com/org/repo/pull/77")); } - // --- Tests for regression resolved completed message --- - #[tokio::test] async fn test_notify_completed_regression_resolved_message_format() { let mock = MockWhatsAppClient::success(); @@ -1395,8 +1385,6 @@ mod tests { assert!(body.contains("no regression")); } - // --- Tests for regression detected failed message --- - #[tokio::test] async fn test_notify_failed_regression_detected_message_format() { let mock = MockWhatsAppClient::success(); @@ -1416,8 +1404,6 @@ mod tests { assert!(body.contains("Tests failing again")); } - // --- Tests for cascade failed message --- - #[tokio::test] async fn test_notify_failed_cascade_message_format() { let mock = MockWhatsAppClient::success(); @@ -1435,8 +1421,6 @@ mod tests { assert!(body.contains("Build error")); } - // --- Tests for notify_merged and notify_closed --- - #[tokio::test] async fn test_notify_merged_message_format() { let mock = MockWhatsAppClient::success(); @@ -1473,8 +1457,6 @@ mod tests { assert!(body.contains("https://github.com/org/repo/pull/43")); } - // --- Test failed cascade with long error truncation --- - #[tokio::test] async fn test_notify_failed_cascade_truncates_long_error() { let mock = MockWhatsAppClient::success(); @@ -1491,8 +1473,6 @@ mod tests { assert!(body.contains("...")); } - // --- Test regression with long error truncation --- - #[tokio::test] async fn test_notify_failed_regression_truncates_long_error() { let mock = MockWhatsAppClient::success(); @@ -1509,8 +1489,6 @@ mod tests { assert!(body.contains("...")); } - // --- Additional test: JSON payload structure --- - #[tokio::test] async fn test_json_payload_has_correct_structure() { let mock = MockWhatsAppClient::success(); @@ -1529,20 +1507,13 @@ mod tests { assert!(payload["text"].get("body").is_some()); } - // --- Test http response fields --- - #[test] fn test_http_response_fields() { - let response = HttpResponse { - status: 201, - body: "Created".to_string(), - }; + let response = HttpResponse::new(201, "Created"); assert_eq!(response.status, 201); assert_eq!(response.body, "Created"); } - // --- Additional coverage tests --- - #[tokio::test] async fn test_notify_merged_disabled() { let notifier = WhatsAppNotifier::new(disabled_config(), empty_registry()); @@ -2149,8 +2120,6 @@ mod tests { assert!(replies.is_empty()); } - // --- Dynamic dispatch (Box) tests for tarpaulin coverage --- - fn boxed_notifier(mock: MockWhatsAppClient, config: WhatsAppConfig) -> Box { Box::new(WhatsAppNotifier::with_http_client(config, mock)) } diff --git a/crates/claudear-integrations/src/scm.rs b/crates/claudear-integrations/src/scm.rs index 3791de08..7a42ada6 100644 --- a/crates/claudear-integrations/src/scm.rs +++ b/crates/claudear-integrations/src/scm.rs @@ -3,6 +3,7 @@ //! Defines a common trait and shared types used by both GitHub and GitLab //! backends for PR monitoring and review watching. +use abnegate_http::ReqwestHttpClient; use async_trait::async_trait; use claudear_core::error::Result; use claudear_core::types::{ @@ -14,6 +15,9 @@ use std::sync::Arc; pub use claudear_core::types::{PrReviewState, ReviewComment, ReviewUser}; +/// Room for the unified diff of a large pull request. +pub const BODY_LIMIT: usize = 8 * ReqwestHttpClient::DEFAULT_BODY_LIMIT; + /// Check whether a bot user should be skipped based on the allowed-bots list. /// /// Returns `true` if the user is a bot and its login does NOT match any entry in @@ -6524,8 +6528,6 @@ mod tests { } } - // --- comment_is_after_cursor tests --- - #[test] fn comment_is_after_cursor_no_cursor() { let comment = make_review_comment("src/main.rs", "fix", Some(1)); @@ -6606,8 +6608,6 @@ mod tests { )); } - // --- ScmRelease serde --- - #[test] fn scm_release_serialization_round_trip() { let release = ScmRelease { @@ -6658,8 +6658,6 @@ mod tests { assert_eq!(cloned.name, release.name); } - // --- RemoteRepo serde --- - #[test] fn remote_repo_serialization_round_trip() { let repo = RemoteRepo { @@ -6698,8 +6696,6 @@ mod tests { assert_eq!(repo.ssh_url, ""); } - // --- ReviewComment serde --- - #[test] fn review_comment_serialization_round_trip() { let comment = ReviewComment { @@ -6765,8 +6761,6 @@ mod tests { assert!(deserialized.user.user_type.is_none()); } - // --- CodeReview serde --- - #[test] fn code_review_serialization_round_trip() { let review = CodeReview { @@ -6793,8 +6787,6 @@ mod tests { ); } - // --- PrSummary serde --- - #[test] fn pr_summary_serialization_round_trip() { let summary = PrSummary { @@ -6811,8 +6803,6 @@ mod tests { assert_eq!(deserialized.url, "https://github.com/org/repo/pull/42"); } - // --- ReviewUser serde --- - #[test] fn review_user_serialization_round_trip() { let user = ReviewUser { @@ -6837,8 +6827,6 @@ mod tests { assert_eq!(user.login, "dependabot[bot]"); } - // --- PostReviewAction --- - #[test] fn post_review_action_equality() { assert_eq!(PostReviewAction::Comment, PostReviewAction::Comment); @@ -6860,8 +6848,6 @@ mod tests { assert_eq!(format!("{:?}", PostReviewAction::Approve), "Approve"); } - // --- PrStatusUpdate --- - #[test] fn pr_status_update_fields() { let update = PrStatusUpdate { @@ -6896,8 +6882,6 @@ mod tests { assert!(update.regression_watch_id.is_none()); } - // --- InlineReviewComment --- - #[test] fn inline_review_comment_fields() { let comment = InlineReviewComment { @@ -6920,8 +6904,6 @@ mod tests { assert!(comment.position.is_none()); } - // --- ReviewEvent requires_action for case-insensitive states (extended) --- - #[test] fn review_event_requires_action_lowercase_changes_requested_2() { let event = ReviewEvent::ReviewSubmitted { @@ -6970,8 +6952,6 @@ mod tests { assert!(!event.requires_action()); } - // --- PrReviewState serde with all-None cursors --- - #[test] fn pr_review_state_serde_none_cursors() { let state = PrReviewState::new("https://example.com/pr/1", "org/repo", 1, "ISS-1", "jira"); diff --git a/crates/claudear-integrations/src/source/github.rs b/crates/claudear-integrations/src/source/github.rs index 20f21a9e..04f84e33 100644 --- a/crates/claudear-integrations/src/source/github.rs +++ b/crates/claudear-integrations/src/source/github.rs @@ -1,18 +1,23 @@ //! GitHub Issues source adapter. use super::IssueSource; -use crate::github::{GitHubClient, GitHubIssue}; +use crate::github::GitHubClient; +use crate::github::GitHubIssue; +use abnegate_http::HttpClient; use async_trait::async_trait; use claudear_config::config::GitHubConfig; -use claudear_core::error::{Error, Result}; -use claudear_core::http::HttpClient; -use claudear_core::types::{Issue, IssueStatus, MatchPriority, MatchResult}; +use claudear_core::error::Error; +use claudear_core::error::Result; +use claudear_core::types::Issue; +use claudear_core::types::IssueStatus; +use claudear_core::types::MatchPriority; +use claudear_core::types::MatchResult; /// GitHub Issues source. /// /// Fetches issues from GitHub repositories and maps them to the unified Issue type. /// Source name is `"github_issues"` to avoid collision with the `"github"` ScmProvider. -pub struct GitHubSource { +pub struct GitHubSource { client: GitHubClient, config: GitHubConfig, } @@ -274,8 +279,6 @@ mod tests { GitHubConfig::test_default() } - // --- Unit tests for free functions --- - #[test] fn test_format_issue_id() { assert_eq!(format_issue_id("owner/repo", 42), "owner:repo#42"); @@ -327,8 +330,6 @@ mod tests { assert_eq!(number, 99); } - // --- Unit tests for map_issue --- - #[test] fn test_map_issue_open() { let config = test_config(); @@ -405,8 +406,6 @@ mod tests { assert_eq!(issue.description, None); } - // --- Unit tests for matches_criteria --- - #[test] fn test_matches_criteria_basic_match() { let source = GitHubSource::new(test_config()); @@ -503,8 +502,6 @@ mod tests { assert!(result.matches); } - // --- Unit tests for context formatting --- - #[test] fn test_format_context() { let mut issue = Issue::new( @@ -546,8 +543,6 @@ mod tests { assert!(!context.contains("## Description")); } - // --- Source trait tests --- - #[test] fn test_source_name() { let source = GitHubSource::new(test_config()); @@ -603,12 +598,11 @@ mod tests { assert!(context.contains("**Title:** Async context test")); } - // --- Mock HTTP integration tests --- - mod fetch_tests { use super::*; + use abnegate_http::HttpClient; + use abnegate_http::HttpResponse; use async_trait::async_trait; - use claudear_core::http::{HttpClient, HttpResponse}; use std::collections::HashMap; use std::sync::Mutex; @@ -624,13 +618,10 @@ mod tests { } fn mock_response(&self, url: impl Into, status: u16, body: impl Into) { - self.responses.lock().unwrap().insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + self.responses + .lock() + .unwrap() + .insert(url.into(), HttpResponse::new(status, body)); } } @@ -640,18 +631,12 @@ mod tests { &self, url: &str, _headers: Vec<(&str, String)>, - ) -> claudear_core::error::Result { + ) -> abnegate_http::Result { let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } } diff --git a/crates/claudear-integrations/src/source/gitlab.rs b/crates/claudear-integrations/src/source/gitlab.rs index 6e97e43d..76255e78 100644 --- a/crates/claudear-integrations/src/source/gitlab.rs +++ b/crates/claudear-integrations/src/source/gitlab.rs @@ -741,8 +741,9 @@ mod tests { mod fetch_tests { use super::*; use crate::gitlab::GitLabClient; + use abnegate_http::HttpClient; + use abnegate_http::HttpResponse; use async_trait::async_trait; - use claudear_core::http::{HttpClient, HttpResponse}; use std::collections::HashMap; use std::sync::Mutex; @@ -758,13 +759,10 @@ mod tests { } fn mock_response(&self, url: impl Into, status: u16, body: impl Into) { - self.responses.lock().unwrap().insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + self.responses + .lock() + .unwrap() + .insert(url.into(), HttpResponse::new(status, body)); } } @@ -774,18 +772,12 @@ mod tests { &self, url: &str, _headers: Vec<(&str, String)>, - ) -> claudear_core::error::Result { + ) -> abnegate_http::Result { let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } } diff --git a/crates/claudear-integrations/src/source/helpscout.rs b/crates/claudear-integrations/src/source/helpscout.rs index 3544aee4..a425b8ef 100644 --- a/crates/claudear-integrations/src/source/helpscout.rs +++ b/crates/claudear-integrations/src/source/helpscout.rs @@ -4,14 +4,21 @@ //! as an "inbox". Uses the Mailbox API v2 with OAuth2 client-credentials. use super::IssueSource; +use abnegate_http::HttpClient; +use abnegate_http::HttpResponse; +use abnegate_http::ReqwestHttpClient; use async_trait::async_trait; -use claudear_config::config::{HelpScoutConfig, ReplyAs}; -use claudear_core::error::{Error, Result}; -use claudear_core::http::{HttpClient, ReqwestHttpClient}; -use claudear_core::types::{Issue, MatchPriority, MatchResult}; +use claudear_config::config::HelpScoutConfig; +use claudear_config::config::ReplyAs; +use claudear_core::error::Error; +use claudear_core::error::Result; +use claudear_core::types::Issue; +use claudear_core::types::MatchPriority; +use claudear_core::types::MatchResult; use serde::Deserialize; use std::sync::Mutex; -use std::time::{Duration, Instant}; +use std::time::Duration; +use std::time::Instant; /// Base URL for the HelpScout Mailbox API v2. const HELPSCOUT_API_BASE: &str = "https://api.helpscout.net"; @@ -19,8 +26,6 @@ const HELPSCOUT_API_BASE: &str = "https://api.helpscout.net"; /// Refresh the access token this many seconds before it actually expires. const TOKEN_EXPIRY_BUFFER_SECS: u64 = 60; -// ---- HelpScout API response shapes ---- - #[derive(Debug, Deserialize)] struct TokenResponse { access_token: String, @@ -113,7 +118,9 @@ pub struct HelpScoutSource { impl HelpScoutSource { /// Create a HelpScout source with the default HTTP client. pub fn new(config: HelpScoutConfig) -> Self { - Self::with_http_client(config, ReqwestHttpClient::new()) + let http = ReqwestHttpClient::new() + .unwrap_or_else(|_| ReqwestHttpClient::from(reqwest::Client::new())); + Self::with_http_client(config, http) } } @@ -129,22 +136,20 @@ impl HelpScoutSource { /// Return a valid bearer token, fetching a new one if needed. async fn access_token(&self) -> Result { - // Fast path: a cached, non-expired token. if let Ok(guard) = self.token.lock() { - if let Some((tok, refresh_at)) = guard.as_ref() { + if let Some((cached, refresh_at)) = guard.as_ref() { if Instant::now() < *refresh_at { - return Ok(tok.clone()); + return Ok(cached.clone()); } } } - // Fetch a fresh token via client-credentials. let body = format!( "grant_type=client_credentials&client_id={}&client_secret={}", urlencoding(self.config.app_id.expose()), urlencoding(self.config.app_secret.expose()), ); - let resp = self + let response = self .http .post( &format!("{HELPSCOUT_API_BASE}/v2/oauth2/token"), @@ -155,13 +160,16 @@ impl HelpScoutSource { &body, ) .await?; - if !resp.is_success() { + if !response.is_success() { return Err(Error::source( "helpscout", - format!("token request failed ({}): {}", resp.status, resp.body), + format!( + "token request failed ({}): {}", + response.status, response.body + ), )); } - let token: TokenResponse = resp.json()?; + let token: TokenResponse = response.json()?; let ttl = token .expires_in .saturating_sub(TOKEN_EXPIRY_BUFFER_SECS) @@ -176,14 +184,15 @@ impl HelpScoutSource { } /// Authorized GET returning the raw response. - async fn api_get(&self, path_and_query: &str) -> Result { + async fn api_get(&self, path_and_query: &str) -> Result { let token = self.access_token().await?; - self.http + Ok(self + .http .get( &format!("{HELPSCOUT_API_BASE}{path_and_query}"), vec![("Authorization", format!("Bearer {token}"))], ) - .await + .await?) } /// The status used when none is requested: the configured trigger status, @@ -206,14 +215,17 @@ impl HelpScoutSource { urlencoding(mailbox), urlencoding(status), ); - let resp = self.api_get(&path).await?; - if !resp.is_success() { + let response = self.api_get(&path).await?; + if !response.is_success() { return Err(Error::source( "helpscout", - format!("list conversations failed ({}): {}", resp.status, resp.body), + format!( + "list conversations failed ({}): {}", + response.status, response.body + ), )); } - let list: ConversationsListResponse = resp.json()?; + let list: ConversationsListResponse = response.json()?; if let Some(embedded) = list.embedded { for c in embedded.conversations { issues.push(self.map_conversation(c)); @@ -229,11 +241,11 @@ impl HelpScoutSource { .api_get(&format!("/v2/conversations/{conversation_id}/threads")) .await { - Ok(resp) if resp.is_success() => resp + Ok(response) if response.is_success() => response .json::() .ok() - .and_then(|r| r.embedded) - .map(|e| e.threads) + .and_then(|threads| threads.embedded) + .map(|embedded| embedded.threads) .unwrap_or_default(), _ => Vec::new(), } @@ -394,19 +406,22 @@ impl IssueSource for HelpScoutSource { } async fn get_issue(&self, issue_id: &str) -> Result { - let resp = self + let response = self .api_get(&format!("/v2/conversations/{issue_id}")) .await?; - if resp.is_not_found() { + if response.is_not_found() { return Err(Error::issue_not_found("helpscout", issue_id)); } - if !resp.is_success() { + if !response.is_success() { return Err(Error::source( "helpscout", - format!("get conversation failed ({}): {}", resp.status, resp.body), + format!( + "get conversation failed ({}): {}", + response.status, response.body + ), )); } - let mut conversation: HsConversation = resp.json()?; + let mut conversation: HsConversation = response.json()?; // Ensure we have the full thread history for context. if conversation .embedded @@ -424,7 +439,7 @@ impl IssueSource for HelpScoutSource { // HelpScout uses JSON-PATCH to update conversation status. let token = self.access_token().await?; let body = r#"[{"op":"replace","path":"/status","value":"closed"}]"#; - let resp = self + let response = self .http .patch( &format!("{HELPSCOUT_API_BASE}/v2/conversations/{issue_id}"), @@ -435,12 +450,15 @@ impl IssueSource for HelpScoutSource { body, ) .await?; - if resp.is_success() { + if response.is_success() { Ok(()) } else { Err(Error::source( "helpscout", - format!("close conversation failed ({}): {}", resp.status, resp.body), + format!( + "close conversation failed ({}): {}", + response.status, response.body + ), )) } } @@ -469,7 +487,7 @@ impl IssueSource for HelpScoutSource { } }; - let resp = self + let response = self .http .post( &format!("{HELPSCOUT_API_BASE}{endpoint}"), @@ -480,12 +498,12 @@ impl IssueSource for HelpScoutSource { &body, ) .await?; - if resp.is_success() { + if response.is_success() { Ok(()) } else { Err(Error::source( "helpscout", - format!("post reply failed ({}): {}", resp.status, resp.body), + format!("post reply failed ({}): {}", response.status, response.body), )) } } @@ -494,7 +512,7 @@ impl IssueSource for HelpScoutSource { #[cfg(test)] mod tests { use super::*; - use claudear_core::http::HttpResponse; + use abnegate_http::HttpResponse; use std::collections::HashMap; /// Mock HTTP client: returns canned responses keyed by a substring of the URL. @@ -520,22 +538,20 @@ mod tests { let responses = self.responses.lock().unwrap(); for (key, status, body) in responses.iter() { if url.contains(key.as_str()) { - return HttpResponse { - status: *status, - body: body.clone(), - }; + return HttpResponse::new(*status, body.clone()); } } - HttpResponse { - status: 404, - body: "{}".to_string(), - } + HttpResponse::new(404, "{}") } } #[async_trait] impl HttpClient for MockHttpClient { - async fn get(&self, url: &str, _headers: Vec<(&str, String)>) -> Result { + async fn get( + &self, + url: &str, + _headers: Vec<(&str, String)>, + ) -> abnegate_http::Result { self.requests .lock() .unwrap() @@ -548,7 +564,7 @@ mod tests { url: &str, _headers: Vec<(&str, String)>, body: &str, - ) -> Result { + ) -> abnegate_http::Result { self.requests .lock() .unwrap() @@ -561,7 +577,7 @@ mod tests { url: &str, _headers: Vec<(&str, String)>, body: &str, - ) -> Result { + ) -> abnegate_http::Result { self.requests .lock() .unwrap() diff --git a/crates/claudear-integrations/src/source/jira.rs b/crates/claudear-integrations/src/source/jira.rs index 649cfe19..cb759843 100644 --- a/crates/claudear-integrations/src/source/jira.rs +++ b/crates/claudear-integrations/src/source/jira.rs @@ -1,12 +1,18 @@ //! Jira issue source adapter. use super::IssueSource; +use abnegate_http::HttpResponse; use async_trait::async_trait; -use base64::{engine::general_purpose::STANDARD as BASE64, Engine}; +use base64::engine::general_purpose::STANDARD as BASE64; +use base64::Engine; use claudear_config::config::JiraConfig; -use claudear_core::error::{Error, Result}; -use claudear_core::http::HttpResponse; -use claudear_core::types::{Issue, IssuePriority, IssueStatus, MatchPriority, MatchResult}; +use claudear_core::error::Error; +use claudear_core::error::Result; +use claudear_core::types::Issue; +use claudear_core::types::IssuePriority; +use claudear_core::types::IssueStatus; +use claudear_core::types::MatchPriority; +use claudear_core::types::MatchResult; use serde::Deserialize; /// Trait for HTTP client operations to enable testing. @@ -62,7 +68,7 @@ impl JiraHttpClient for ReqwestJiraClient { .await?; let status = response.status().as_u16(); let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) + Ok(HttpResponse::new(status, body)) } async fn post( @@ -80,10 +86,7 @@ impl JiraHttpClient for ReqwestJiraClient { .await?; let status = response.status().as_u16(); let body_text = response.text().await.unwrap_or_default(); - Ok(HttpResponse { - status, - body: body_text, - }) + Ok(HttpResponse::new(status, body_text)) } } @@ -994,24 +997,12 @@ mod tests { pub fn mock_get(&self, url: impl Into, status: u16, body: impl Into) { let mut responses = self.get_responses.lock().unwrap(); - responses.insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + responses.insert(url.into(), HttpResponse::new(status, body)); } pub fn mock_post(&self, url: impl Into, status: u16, body: impl Into) { let mut responses = self.post_responses.lock().unwrap(); - responses.insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + responses.insert(url.into(), HttpResponse::new(status, body)); } #[expect(dead_code)] @@ -1029,15 +1020,9 @@ mod tests { .push(("GET".to_string(), url.to_string())); let responses = self.get_responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } @@ -1053,15 +1038,9 @@ mod tests { .push(("POST".to_string(), url.to_string())); let responses = self.post_responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } } @@ -3060,8 +3039,6 @@ mod tests { ); } - // --- Tests for create_issue coverage --- - #[tokio::test] async fn test_create_issue_success() { let config = test_config(); @@ -3121,8 +3098,6 @@ mod tests { .contains("Failed to create issue")); } - // --- Tests for find_or_create_label coverage --- - #[tokio::test] async fn test_find_or_create_label_returns_name() { let config = test_config(); @@ -3135,8 +3110,6 @@ mod tests { assert_eq!(label, "auto-implement"); } - // --- Tests for list_open_issues coverage --- - #[tokio::test] async fn test_list_open_issues_no_filter() { let config = test_config(); @@ -3221,8 +3194,6 @@ mod tests { .contains("Failed to search issues")); } - // --- Tests for build_issue_context coverage --- - #[tokio::test] async fn test_build_issue_context_delegates_to_format() { let config = test_config(); @@ -3248,8 +3219,6 @@ mod tests { assert!(context.contains("Some description")); } - // --- Tests for get_issue_status coverage --- - #[tokio::test] async fn test_get_issue_status_api_error() { let config = test_config(); @@ -3262,8 +3231,6 @@ mod tests { assert!(result.is_err()); } - // --- Tests for search_issues max_results cap --- - #[tokio::test] async fn test_search_issues_caps_max_results_at_100() { let mut config = test_config(); @@ -3290,8 +3257,6 @@ mod tests { assert!(issues.is_empty()); } - // --- Tests for escape_jql_value edge cases --- - #[test] fn test_escape_jql_value_multiple_backslashes() { type JS = JiraSource; @@ -3305,8 +3270,6 @@ mod tests { assert_eq!(JS::escape_jql_value(r#"""#), r#"\""#); } - // --- Tests for parse_jira_datetime edge cases --- - #[test] fn test_parse_jira_datetime_with_colon_offset() { let dt = parse_jira_datetime("2024-03-15T10:30:00.000+00:00"); @@ -3325,8 +3288,6 @@ mod tests { assert!(dt.is_some()); } - // --- Tests for map_issue dates --- - #[test] fn test_map_issue_rfc3339_dates() { let config = test_config(); @@ -3391,8 +3352,6 @@ mod tests { assert!(issue.updated_at.is_none()); } - // --- Tests for extract_adf_text edge cases --- - #[test] fn test_extract_adf_text_ordered_list() { let value = serde_json::json!({ @@ -3471,8 +3430,6 @@ mod tests { assert!(text.contains("Deep text")); } - // --- Tests for create_issue with trailing slash on base_url --- - #[tokio::test] async fn test_create_issue_url_trailing_slash() { let mut config = test_config(); @@ -3492,8 +3449,6 @@ mod tests { assert_eq!(issue.url, "https://test.atlassian.net/browse/PROJ-30"); } - // --- Tests for matches_criteria medium priority --- - #[test] fn test_matches_criteria_medium_priority() { let config = test_config(); @@ -3510,8 +3465,6 @@ mod tests { assert_eq!(result.priority, MatchPriority::Normal); } - // --- Test get_issue with trailing slash --- - #[tokio::test] async fn test_get_issue_trailing_slash_base_url() { let mut config = test_config(); @@ -3531,8 +3484,6 @@ mod tests { assert_eq!(issue.short_id, "PROJ-99"); } - // --- Test map_issue with assignee account_id null --- - #[test] fn test_map_issue_assignee_no_account_id() { let config = test_config(); @@ -3570,8 +3521,6 @@ mod tests { .is_none()); } - // --- Test map_issue with labels but no assignee --- - #[test] fn test_map_issue_labels_no_assignee() { let config = test_config(); @@ -3607,8 +3556,6 @@ mod tests { assert!(issue.get_metadata::("assignee").is_none()); } - // --- Test build_issue_context returns Ok --- - #[tokio::test] async fn test_build_issue_context_returns_ok() { let config = test_config(); @@ -3619,8 +3566,6 @@ mod tests { assert!(result.is_ok()); } - // --- Test extract_adf_text with object without type field --- - #[test] fn test_extract_adf_text_object_no_type() { let value = serde_json::json!({"key": "value"}); @@ -3628,8 +3573,6 @@ mod tests { assert_eq!(text, ""); } - // --- Test list_open_issues with title containing special JQL characters --- - #[tokio::test] async fn test_list_open_issues_title_with_special_chars() { let config = test_config(); diff --git a/crates/claudear-integrations/src/source/linear.rs b/crates/claudear-integrations/src/source/linear.rs index 17b1a3c4..54761514 100644 --- a/crates/claudear-integrations/src/source/linear.rs +++ b/crates/claudear-integrations/src/source/linear.rs @@ -1,12 +1,18 @@ //! Linear issue source adapter. use super::IssueSource; +use abnegate_http::HttpResponse; use async_trait::async_trait; use claudear_config::config::LinearConfig; -use claudear_core::error::{Error, Result}; -use claudear_core::http::HttpResponse; -use claudear_core::types::{Issue, IssuePriority, IssueStatus, MatchPriority, MatchResult}; -use serde::{Deserialize, Serialize}; +use claudear_core::error::Error; +use claudear_core::error::Result; +use claudear_core::types::Issue; +use claudear_core::types::IssuePriority; +use claudear_core::types::IssueStatus; +use claudear_core::types::MatchPriority; +use claudear_core::types::MatchResult; +use serde::Deserialize; +use serde::Serialize; /// Trait for GraphQL client operations to enable testing. #[async_trait] @@ -60,10 +66,7 @@ impl LinearHttpClient for ReqwestLinearClient { .await?; let status = response.status().as_u16(); let body_text = response.text().await.unwrap_or_default(); - Ok(HttpResponse { - status, - body: body_text, - }) + Ok(HttpResponse::new(status, body_text)) } } @@ -917,13 +920,7 @@ mod tests { pub fn mock_response(&self, url: impl Into, status: u16, body: impl Into) { let mut responses = self.responses.lock().unwrap(); - responses.insert( - url.into(), - HttpResponse { - status, - body: body.into(), - }, - ); + responses.insert(url.into(), HttpResponse::new(status, body)); } pub fn get_requests(&self) -> Vec<(String, serde_json::Value)> { @@ -942,30 +939,18 @@ mod tests { self.requests.lock().unwrap().push((url.to_string(), body)); let responses = self.responses.lock().unwrap(); if let Some(response) = responses.get(url) { - Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }) + Ok(response.clone()) } else { - Ok(HttpResponse { - status: 404, - body: "Not found".to_string(), - }) + Ok(HttpResponse::new(404, "Not found")) } } } #[test] fn test_http_response_is_success() { - let response = HttpResponse { - status: 200, - body: "{}".to_string(), - }; + let response = HttpResponse::new(200, "{}"); assert!(response.is_success()); - let response = HttpResponse { - status: 404, - body: "{}".to_string(), - }; + let response = HttpResponse::new(404, "{}"); assert!(!response.is_success()); } @@ -2249,37 +2234,18 @@ mod tests { #[test] fn test_http_response_json_parse_failure() { - let response = HttpResponse { - status: 200, - body: "not valid json".to_string(), - }; - let result: Result = response.json(); + let response = HttpResponse::new(200, "not valid json"); + let result: Result = response.json().map_err(Error::from); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("JSON parse error")); } #[test] fn test_http_response_boundary_status_codes() { - assert!(!HttpResponse { - status: 199, - body: String::new() - } - .is_success()); - assert!(HttpResponse { - status: 200, - body: String::new() - } - .is_success()); - assert!(HttpResponse { - status: 299, - body: String::new() - } - .is_success()); - assert!(!HttpResponse { - status: 300, - body: String::new() - } - .is_success()); + assert!(!HttpResponse::new(199, "").is_success()); + assert!(HttpResponse::new(200, "").is_success()); + assert!(HttpResponse::new(299, "").is_success()); + assert!(!HttpResponse::new(300, "").is_success()); } #[test] @@ -2467,8 +2433,6 @@ mod tests { assert!(response.errors.is_none()); } - // --- New tests for coverage --- - /// Sequential mock HTTP client that returns queued responses in order. pub struct SequentialMockLinearClient { responses: Mutex>, @@ -2482,10 +2446,7 @@ mod tests { responses .into_iter() .rev() // Reverse so we can pop from the end - .map(|(status, body)| HttpResponse { - status, - body: body.to_string(), - }) + .map(|(status, body)| HttpResponse::new(status, body.to_string())) .collect(), ), requests: Mutex::new(Vec::new()), @@ -2506,10 +2467,7 @@ mod tests { if let Some(response) = responses.pop() { Ok(response) } else { - Ok(HttpResponse { - status: 500, - body: "No more mock responses".to_string(), - }) + Ok(HttpResponse::new(500, "No more mock responses")) } } } @@ -4423,8 +4381,6 @@ mod tests { assert!(config.trigger_assignee.is_none()); } - // --- Tests for create_issue coverage --- - #[tokio::test] async fn test_create_issue_success() { let mock = SequentialMockLinearClient::new(vec![ @@ -4559,8 +4515,6 @@ mod tests { .contains("returned no issue")); } - // --- Tests for find_or_create_label coverage --- - #[tokio::test] async fn test_find_or_create_label_existing_label() { let mock = SequentialMockLinearClient::new(vec![( @@ -4734,8 +4688,6 @@ mod tests { .contains("returned no label")); } - // --- Tests for list_open_issues coverage --- - #[tokio::test] async fn test_list_open_issues_no_filter() { let mock = SequentialMockLinearClient::new(vec![( diff --git a/crates/claudear-integrations/src/test_support.rs b/crates/claudear-integrations/src/test_support.rs new file mode 100644 index 00000000..ff3c2801 --- /dev/null +++ b/crates/claudear-integrations/src/test_support.rs @@ -0,0 +1,57 @@ +//! A loopback HTTP server, and a transport that reaches it, for tests that +//! drive a real HTTP client. + +use abnegate_http::ReqwestHttpClient; +use tokio::io::AsyncReadExt; +use tokio::io::AsyncWriteExt; +use tokio::net::TcpListener; + +const HEADER_END: &[u8] = b"\r\n\r\n"; + +/// Answer the first request on a loopback port with `response`, written +/// verbatim, and return the URL that reaches it. +pub async fn serve_once(response: Vec) -> String { + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("a loopback port binds"); + let address = listener + .local_addr() + .expect("a bound listener has an address"); + tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.expect("the client connects"); + let mut request = Vec::new(); + let mut buffer = [0; 1024]; + while !request + .windows(HEADER_END.len()) + .any(|window| window == HEADER_END) + { + let read = stream.read(&mut buffer).await.expect("the request arrives"); + if read == 0 { + break; + } + request.extend_from_slice(&buffer[..read]); + } + let _ = stream.write_all(&response).await; + let _ = stream.shutdown().await; + }); + format!("http://{address}/") +} + +/// A `200 OK` response that declares `length` bytes and carries `body`. +pub fn ok_response(length: usize, body: &[u8]) -> Vec { + let mut response = + format!("HTTP/1.1 200 OK\r\ncontent-length: {length}\r\nconnection: close\r\n\r\n") + .into_bytes(); + response.extend_from_slice(body); + response +} + +/// A transport that reaches loopback directly, whatever proxy the +/// environment configures, and reads bodies up to `body_limit` bytes. +pub fn loopback_transport(body_limit: usize) -> ReqwestHttpClient { + let client = reqwest::Client::builder() + .no_proxy() + .build() + .expect("a client without a proxy builds"); + ReqwestHttpClient::from(client).with_body_limit(body_limit) +} From 2bca87e334f6ef68232f986f9e71e82fb995206b Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Mon, 5 Oct 2026 18:42:41 +1300 Subject: [PATCH 3/5] refactor(analysis): move the HTTP consumers onto abnegate-http The deploy QA tracker, the Linear regression checker and the release client and tracker used claudear-core's local HTTP client, which abnegate-http 0.1.2 now provides. - Imports point at abnegate_http, responses are built with HttpResponse::new (the crate struct is non_exhaustive), and the test doubles answer with abnegate_http::Result. Production code converts through claudear-core's From impl with `?`. - ReleaseClient::new and LinearRegressionChecker::new stay infallible and fall back to reqwest's default client when the configured one fails to build, as the local ReqwestHttpClient::new did; claudear- analysis now names reqwest directly for that. The already fallible LinearRegressionChecker::with_embeddings builds its client first and returns the build error before spending an embedding call. - The regression/linear.rs MockErrorHttpClient used to fail with Error::network("connection refused"). The crate has no convenience constructor for that, so the double makes a real refused connection to 127.0.0.1:1, through a client it builds once without a proxy, and converts it into abnegate_http::Error::Request, which maps to the same transient Error::Http. - The release tracker imports HttpClient and ReqwestHttpClient instead of spelling the paths out, its test double indexes responses with get() instead of a bounds check, and both files lose their section header banners. ReleaseClient (used by the release and deploy QA trackers) reads bodies up to release::BODY_LIMIT, eight times the crate default (64 MiB): is_commit_in_release fetches compare/{commit}...{tag}, which carries up to 250 commits and 300 files with patches. Under the 8 MiB default an oversized comparison would be a non-retried Error::Other and fail the release watch on every poll. scm::BODY_LIMIT in claudear-integrations now re-exports this constant, so the GitHub, GitLab and release clients share one value. A test pins that ReleaseClient::new applies it. The Linear regression checker keeps the 8 MiB default: its responses are Linear GraphQL pages and GitHub issue searches. These clients now error on a body cut short instead of returning it empty, decode bodies as lossy UTF-8, and drop the URL from transport errors. Timeouts and redirects are unchanged. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 1 + crates/claudear-analysis/Cargo.toml | 3 + .../src/deploy_qa/tracker.rs | 74 +++--- .../src/regression/linear.rs | 236 +++++------------- .../claudear-analysis/src/release/github.rs | 49 ++-- crates/claudear-analysis/src/release/mod.rs | 10 +- .../claudear-analysis/src/release/tracker.rs | 154 +++--------- crates/claudear-integrations/src/scm.rs | 20 +- 8 files changed, 184 insertions(+), 363 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dea1320a..59e453f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -870,6 +870,7 @@ dependencies = [ "rand 0.10.0", "rayon", "regex-lite", + "reqwest 0.13.2", "semver", "serde", "serde_json", diff --git a/crates/claudear-analysis/Cargo.toml b/crates/claudear-analysis/Cargo.toml index 9d8c74f3..313787f8 100644 --- a/crates/claudear-analysis/Cargo.toml +++ b/crates/claudear-analysis/Cargo.toml @@ -29,6 +29,9 @@ chrono = { workspace = true } async-trait = { workspace = true } tokio = { workspace = true } +# HTTP +reqwest = { workspace = true } + # Regex regex-lite = { workspace = true } diff --git a/crates/claudear-analysis/src/deploy_qa/tracker.rs b/crates/claudear-analysis/src/deploy_qa/tracker.rs index 831bf7c8..98cde21b 100644 --- a/crates/claudear-analysis/src/deploy_qa/tracker.rs +++ b/crates/claudear-analysis/src/deploy_qa/tracker.rs @@ -1,16 +1,27 @@ //! Poll GitHub for new release tips and persist last-seen / attempt state. -use crate::deploy_qa::playbook::{load_playbook, DEPLOY_QA_SOURCE}; -use crate::deploy_qa::probe::{ - VERDICT_ALL_VERIFIED, VERDICT_FAIL, VERDICT_PREFIX, VERDICT_UNVERIFIED, -}; -use crate::release::{GitHubRelease, GitHubTag, ReleaseClient}; -use claudear_config::config::{DeployQaConfig, DeployQaTagFilter, DeployQaTrackConfig}; +use crate::deploy_qa::playbook::load_playbook; +use crate::deploy_qa::playbook::DEPLOY_QA_SOURCE; +use crate::deploy_qa::probe::VERDICT_ALL_VERIFIED; +use crate::deploy_qa::probe::VERDICT_FAIL; +use crate::deploy_qa::probe::VERDICT_PREFIX; +use crate::deploy_qa::probe::VERDICT_UNVERIFIED; +use crate::release::GitHubRelease; +use crate::release::GitHubTag; +use crate::release::ReleaseClient; +use abnegate_http::HttpClient; +use abnegate_http::ReqwestHttpClient; +use claudear_config::config::DeployQaConfig; +use claudear_config::config::DeployQaTagFilter; +use claudear_config::config::DeployQaTrackConfig; use claudear_core::error::Result; -use claudear_core::http::{HttpClient, ReqwestHttpClient}; -use claudear_core::types::{ - DeployQaTip, DeployQaTipStatus, Issue, IssuePriority, IssueStatus, MatchPriority, MatchResult, -}; +use claudear_core::types::DeployQaTip; +use claudear_core::types::DeployQaTipStatus; +use claudear_core::types::Issue; +use claudear_core::types::IssuePriority; +use claudear_core::types::IssueStatus; +use claudear_core::types::MatchPriority; +use claudear_core::types::MatchResult; use claudear_storage::FixAttemptTracker; use std::path::Path; use std::sync::Arc; @@ -328,9 +339,11 @@ pub fn deploy_qa_match_result(track: &str, tag: &str) -> MatchResult { mod tests { use super::*; use crate::deploy_qa::playbook::bundled_playbook; - use crate::deploy_qa::probe::{classify_deploy_qa_verdict, DeployQaVerdict}; + use crate::deploy_qa::probe::classify_deploy_qa_verdict; + use crate::deploy_qa::probe::DeployQaVerdict; + use abnegate_http::HttpClient; + use abnegate_http::HttpResponse; use async_trait::async_trait; - use claudear_core::http::{HttpClient, HttpResponse}; use claudear_storage::SqliteTracker; use std::collections::HashMap; use std::sync::Mutex; @@ -344,47 +357,36 @@ mod tests { fn new() -> Self { Self { by_url: Mutex::new(HashMap::new()), - default: HttpResponse { - status: 404, - body: r#"{"message":"Not Found"}"#.to_string(), - }, + default: HttpResponse::new(404, r#"{"message":"Not Found"}"#), } } fn on(self, url: &str, status: u16, body: &str) -> Self { - self.by_url.lock().unwrap().insert( - url.to_string(), - HttpResponse { - status, - body: body.to_string(), - }, - ); + self.by_url + .lock() + .unwrap() + .insert(url.to_string(), HttpResponse::new(status, body.to_string())); self } } #[async_trait] impl HttpClient for MapMockHttp { - async fn get(&self, url: &str, _headers: Vec<(&str, String)>) -> Result { + async fn get( + &self, + url: &str, + _headers: Vec<(&str, String)>, + ) -> abnegate_http::Result { let map = self.by_url.lock().unwrap(); if let Some(response) = map.get(url) { - return Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }); + return Ok(response.clone()); } for (key, response) in map.iter() { if url.starts_with(key) { - return Ok(HttpResponse { - status: response.status, - body: response.body.clone(), - }); + return Ok(response.clone()); } } - Ok(HttpResponse { - status: self.default.status, - body: self.default.body.clone(), - }) + Ok(self.default.clone()) } } diff --git a/crates/claudear-analysis/src/regression/linear.rs b/crates/claudear-analysis/src/regression/linear.rs index 490302f0..df8b87af 100644 --- a/crates/claudear-analysis/src/regression/linear.rs +++ b/crates/claudear-analysis/src/regression/linear.rs @@ -5,11 +5,14 @@ //! 2. Scraping appwrite.io/threads for similar mentions //! 3. Using semantic similarity with embeddings to match issues -use crate::feedback::{cosine_similarity, EmbeddingClient}; -use crate::regression::{RegressionChecker, RegressionResult}; +use crate::feedback::cosine_similarity; +use crate::feedback::EmbeddingClient; +use crate::regression::RegressionChecker; +use crate::regression::RegressionResult; +use abnegate_http::HttpClient; +use abnegate_http::ReqwestHttpClient; use async_trait::async_trait; use claudear_core::error::Result; -use claudear_core::http::{HttpClient, ReqwestHttpClient}; use claudear_core::types::RegressionWatch; use serde::Deserialize; use std::sync::Arc; @@ -98,7 +101,8 @@ impl LinearRegressionChecker { ) -> Self { Self { config, - http: ReqwestHttpClient::new(), + http: ReqwestHttpClient::new() + .unwrap_or_else(|_| ReqwestHttpClient::from(reqwest::Client::new())), keywords, original_issue_text, original_embedding: None, @@ -113,12 +117,12 @@ impl LinearRegressionChecker { original_issue_text: String, embedding_client: Arc, ) -> Result { - // Pre-compute the embedding for the original issue + let http = ReqwestHttpClient::new()?; let original_embedding = embedding_client.embed(&original_issue_text).await?; Ok(Self { config, - http: ReqwestHttpClient::new(), + http, keywords, original_issue_text, original_embedding: Some(original_embedding), @@ -582,10 +586,12 @@ impl RegressionChecker for LinearRegressionChecker { #[cfg(test)] mod tests { use super::*; - use chrono::{Duration, Utc}; - use claudear_core::http::HttpResponse; + use abnegate_http::HttpResponse; + use chrono::Duration; + use chrono::Utc; use claudear_core::types::IssueType; - use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::atomic::AtomicUsize; + use std::sync::atomic::Ordering; struct MockHttpClient { responses: Vec, @@ -597,10 +603,7 @@ mod tests { Self { responses: responses .into_iter() - .map(|(status, body)| HttpResponse { - status, - body: body.to_string(), - }) + .map(|(status, body)| HttpResponse::new(status, body.to_string())) .collect(), call_count: AtomicUsize::new(0), } @@ -609,19 +612,17 @@ mod tests { #[async_trait] impl HttpClient for MockHttpClient { - async fn get(&self, _url: &str, _headers: Vec<(&str, String)>) -> Result { - let idx = self.call_count.fetch_add(1, Ordering::SeqCst); - if idx < self.responses.len() { - Ok(HttpResponse { - status: self.responses[idx].status, - body: self.responses[idx].body.clone(), - }) - } else { - Ok(HttpResponse { - status: 404, - body: "Not Found".to_string(), - }) - } + async fn get( + &self, + _url: &str, + _headers: Vec<(&str, String)>, + ) -> abnegate_http::Result { + let call = self.call_count.fetch_add(1, Ordering::SeqCst); + Ok(self + .responses + .get(call) + .cloned() + .unwrap_or_else(|| HttpResponse::new(404, "Not Found"))) } } @@ -880,12 +881,35 @@ mod tests { assert!(result.is_ok()); } - struct MockErrorHttpClient; + struct MockErrorHttpClient { + client: reqwest::Client, + } + + impl MockErrorHttpClient { + fn new() -> Self { + Self { + client: reqwest::Client::builder() + .no_proxy() + .build() + .expect("a client without a proxy builds"), + } + } + } #[async_trait] impl HttpClient for MockErrorHttpClient { - async fn get(&self, _url: &str, _headers: Vec<(&str, String)>) -> Result { - Err(claudear_core::error::Error::network("connection refused")) + async fn get( + &self, + _url: &str, + _headers: Vec<(&str, String)>, + ) -> abnegate_http::Result { + let refusal = self + .client + .get("http://127.0.0.1:1/") + .send() + .await + .expect_err("nothing listens on port 1"); + Err(abnegate_http::Error::from(refusal)) } } @@ -899,10 +923,6 @@ mod tests { ) } - // ═══════════════════════════════════════════════════════════════════ - // 1. strip_html_tags edge cases - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_strip_html_tags_empty_string() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -950,10 +970,6 @@ mod tests { assert_eq!(result, "plain text"); } - // ═══════════════════════════════════════════════════════════════════ - // 2. extract_thread_sections - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_extract_thread_sections_no_headings_fallback() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -1011,10 +1027,6 @@ mod tests { assert!(!sections.is_empty()); } - // ═══════════════════════════════════════════════════════════════════ - // 3. check_similarity without embedding client - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_check_similarity_without_embedding_returns_zero() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -1023,10 +1035,6 @@ mod tests { assert!((similarity - 0.0).abs() < f32::EPSILON); } - // ═══════════════════════════════════════════════════════════════════ - // 4. search_github_issues_since - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_search_github_issues_since_empty_token() { let config = LinearRegressionConfig { @@ -1149,10 +1157,6 @@ mod tests { assert_eq!(checker.http.call_count.load(Ordering::SeqCst), 1); } - // ═══════════════════════════════════════════════════════════════════ - // 5. check_appwrite_threads - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_check_appwrite_threads_non_200() { let mock = MockHttpClient::new(vec![(503, "Service Unavailable")]); @@ -1174,7 +1178,7 @@ mod tests { create_config(), vec!["keyword".to_string()], "Some issue".to_string(), - MockErrorHttpClient, + MockErrorHttpClient::new(), ); // Network error should be caught and return empty vec (not propagate error) @@ -1182,10 +1186,6 @@ mod tests { assert!(results.is_empty()); } - // ═══════════════════════════════════════════════════════════════════ - // 6. check_regression trait impl - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_check_regression_empty_token_and_threads_error() { // Empty GitHub token skips GitHub search; threads returns an error @@ -1200,7 +1200,7 @@ mod tests { config, vec!["keyword".to_string()], "Some issue".to_string(), - MockErrorHttpClient, // threads will get a network error + MockErrorHttpClient::new(), // threads will get a network error ); let mut watch = RegressionWatch::new(IssueType::LinearBug, "linear-err", 1); @@ -1322,10 +1322,6 @@ mod tests { ); } - // ═══════════════════════════════════════════════════════════════════ - // 7. LinearRegressionConfig::default() - // ═══════════════════════════════════════════════════════════════════ - #[test] fn test_linear_regression_config_default_repos() { let config = LinearRegressionConfig::default(); @@ -1351,10 +1347,6 @@ mod tests { assert!(config.github_token.is_empty()); } - // ═══════════════════════════════════════════════════════════════════ - // 8. SimilarityMatch struct — Clone and Debug - // ═══════════════════════════════════════════════════════════════════ - #[test] fn test_similarity_match_clone() { let original = SimilarityMatch { @@ -1384,10 +1376,6 @@ mod tests { assert!(debug_output.contains("0.77")); } - // ═══════════════════════════════════════════════════════════════════ - // 9. Constructor field verification - // ═══════════════════════════════════════════════════════════════════ - #[test] fn test_with_http_client_sets_fields_correctly() { let config = LinearRegressionConfig { @@ -1439,10 +1427,6 @@ mod tests { assert_eq!(checker.original_issue_text, "Issue text"); } - // ═══════════════════════════════════════════════════════════════════ - // 10. LinearRegressionConfig clone and debug - // ═══════════════════════════════════════════════════════════════════ - #[test] fn test_linear_regression_config_clone() { let config = LinearRegressionConfig { @@ -1465,10 +1449,6 @@ mod tests { assert!(debug.contains("test/repo")); } - // ═══════════════════════════════════════════════════════════════════ - // 11. search_github_issues (the non-_since variant) - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_search_github_issues_empty_token_returns_empty() { let config = LinearRegressionConfig { @@ -1616,10 +1596,6 @@ mod tests { assert!(results.is_empty()); } - // ═══════════════════════════════════════════════════════════════════ - // 12. search_github_issues_since - date filtering edge cases - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_search_github_issues_since_invalid_date_format() { // Issue with an unparseable created_at should be filtered out @@ -1762,10 +1738,6 @@ mod tests { assert!(results.is_empty()); } - // ═══════════════════════════════════════════════════════════════════ - // 13. check_appwrite_threads - additional paths - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_check_appwrite_threads_success_empty_body() { let mock = MockHttpClient::new(vec![(200, "")]); @@ -1816,10 +1788,6 @@ mod tests { assert!(results.is_empty()); } - // ═══════════════════════════════════════════════════════════════════ - // 14. extract_thread_sections - more tag patterns - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_extract_thread_sections_h3_tag() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -1892,10 +1860,6 @@ mod tests { assert!(sections[0].1.contains("Just plain text")); } - // ═══════════════════════════════════════════════════════════════════ - // 15. strip_html_tags - unicode and edge cases - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_strip_html_tags_unicode_content() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -1937,10 +1901,6 @@ mod tests { assert_eq!(result, "body{color:red}Visible"); } - // ═══════════════════════════════════════════════════════════════════ - // 16. check_regression - additional trait impl paths - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_check_regression_github_error_propagates() { // If the GitHub HTTP call itself errors, it should propagate @@ -1948,7 +1908,7 @@ mod tests { create_config(), vec!["keyword".to_string()], "Some issue".to_string(), - MockErrorHttpClient, + MockErrorHttpClient::new(), ); let mut watch = RegressionWatch::new(IssueType::LinearBug, "linear-err2", 1); @@ -2028,10 +1988,6 @@ mod tests { assert!(details.contains("0 repos"), "Got: {}", details); } - // ═══════════════════════════════════════════════════════════════════ - // 17. extract_thread_sections - UTF-8 boundary safety - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_extract_thread_sections_with_multibyte_utf8() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -2055,10 +2011,6 @@ mod tests { assert!(title.contains("Long Thread")); } - // ═══════════════════════════════════════════════════════════════════ - // 18. search_github_issues - keyword limiting (take(5)) - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_search_github_issues_limits_to_5_keywords() { let mock = MockHttpClient::new(vec![(200, r#"{"total_count": 0, "items": []}"#)]); @@ -2085,10 +2037,6 @@ mod tests { assert_eq!(checker.http.call_count.load(Ordering::SeqCst), 1); } - // ═══════════════════════════════════════════════════════════════════ - // 19. search_github_issues_since - mixed results (some old, some new) - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_search_github_issues_since_filters_old_keeps_new() { let old_time = (Utc::now() - Duration::days(30)).to_rfc3339(); @@ -2137,10 +2085,6 @@ mod tests { assert!(results.is_empty()); } - // ═══════════════════════════════════════════════════════════════════ - // 20. check_regression with empty github token + threads success - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_check_regression_empty_token_skips_github_checks_threads() { let config = LinearRegressionConfig { @@ -2170,10 +2114,6 @@ mod tests { assert_eq!(checker.http.call_count.load(Ordering::SeqCst), 1); } - // ═══════════════════════════════════════════════════════════════════ - // 21. SimilarityMatch edge values - // ═══════════════════════════════════════════════════════════════════ - #[test] fn test_similarity_match_zero_similarity() { let m = SimilarityMatch { @@ -2205,10 +2145,6 @@ mod tests { assert!(m.title.is_empty()); } - // ═══════════════════════════════════════════════════════════════════ - // 22. LinearRegressionConfig - boundary threshold values - // ═══════════════════════════════════════════════════════════════════ - #[test] fn test_config_zero_threshold() { let config = LinearRegressionConfig { @@ -2240,10 +2176,6 @@ mod tests { assert_eq!(config.scm_repos.len(), 100); } - // ═══════════════════════════════════════════════════════════════════ - // 23. extract_thread_sections - title with only closing tag - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_extract_thread_sections_empty_title_is_skipped() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -2262,10 +2194,6 @@ mod tests { assert_eq!(sections[0].0, "Appwrite Threads Page"); } - // ═══════════════════════════════════════════════════════════════════ - // 24. Verify MockHttpClient exhaustion returns 404 - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_mock_http_client_returns_404_when_exhausted() { let mock = MockHttpClient::new(vec![(200, "ok")]); @@ -2280,10 +2208,6 @@ mod tests { assert_eq!(r2.body, "Not Found"); } - // ═══════════════════════════════════════════════════════════════════ - // 25. check_regression with embedding client present (semantic method) - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_check_regression_with_embedding_client_shows_semantic_method() { let config = create_config(); @@ -2323,10 +2247,6 @@ mod tests { ); } - // ═══════════════════════════════════════════════════════════════════ - // 26. strip_html_tags - more edge cases - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_strip_html_tags_unclosed_tag() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -2350,10 +2270,6 @@ mod tests { assert_eq!(result, "& < >"); } - // ═══════════════════════════════════════════════════════════════════ - // 27. extract_thread_sections - content/title emptiness combinations - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_extract_thread_sections_title_but_empty_content() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -2369,10 +2285,6 @@ mod tests { assert!(!sections.is_empty()); } - // ═══════════════════════════════════════════════════════════════════ - // 28. RegressionResult helper methods - // ═══════════════════════════════════════════════════════════════════ - #[test] fn test_regression_result_no_regression() { let result = crate::regression::RegressionResult::no_regression(); @@ -2388,10 +2300,6 @@ mod tests { assert_eq!(result.details.unwrap(), "Found similar issues"); } - // ═══════════════════════════════════════════════════════════════════ - // 29. check_similarity with embedding client but no original_embedding - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_check_similarity_with_client_but_no_original_embedding() { let config = create_config(); @@ -2414,10 +2322,6 @@ mod tests { assert!((sim - 0.0).abs() < f32::EPSILON); } - // ═══════════════════════════════════════════════════════════════════ - // 30. GitHub issue search result JSON deserialization edge cases - // ═══════════════════════════════════════════════════════════════════ - #[tokio::test] async fn test_search_github_issues_since_empty_items_array() { let mock = MockHttpClient::new(vec![(200, r#"{"total_count": 0, "items": []}"#)]); @@ -2475,10 +2379,6 @@ mod tests { assert!(results.is_empty()); } - // ═══════════════════════════════════════════════════════════════════ - // 31. Regression detection format strings - // ═══════════════════════════════════════════════════════════════════ - #[test] fn test_similarity_match_format_string() { let m = SimilarityMatch { @@ -2498,10 +2398,6 @@ mod tests { assert!(formatted.contains("issues/42")); } - // ═══════════════════════════════════════════════════════════════════ - // 32. LinearRegressionChecker::new (production constructor) - // ═══════════════════════════════════════════════════════════════════ - #[test] fn test_linear_regression_checker_new() { let config = LinearRegressionConfig { @@ -2523,8 +2419,6 @@ mod tests { assert!(checker.embedding_client.is_none()); } - // --- RegressionResult construction (extended) --- - #[test] fn test_regression_result_no_regression_2() { let result = crate::regression::RegressionResult::no_regression(); @@ -2540,8 +2434,6 @@ mod tests { assert_eq!(result.details, Some("Found similar issue".to_string())); } - // --- extract_thread_sections with

tags --- - #[tokio::test] async fn test_extract_thread_sections_h3_tags() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -2550,8 +2442,6 @@ mod tests { assert!(!sections.is_empty()); } - // --- extract_thread_sections with
(extended) --- - #[tokio::test] async fn test_extract_thread_sections_div_post_class_2() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -2561,8 +2451,6 @@ mod tests { assert!(!sections.is_empty()); } - // --- strip_html_tags with angle brackets in text (extended) --- - #[tokio::test] async fn test_strip_html_tags_unclosed_tag_2() { let checker = make_checker(MockHttpClient::new(vec![])); @@ -2578,8 +2466,6 @@ mod tests { assert_eq!(result, "word1 word2 word3"); } - // --- search_github_issues_since with multiple keywords over limit --- - #[tokio::test] async fn test_search_github_issues_since_exactly_5_keywords() { let mock = MockHttpClient::new(vec![(200, r#"{"total_count": 0, "items": []}"#)]); @@ -2603,8 +2489,6 @@ mod tests { assert_eq!(checker.http.call_count.load(Ordering::SeqCst), 1); } - // --- check_regression with empty repos --- - #[tokio::test] async fn test_check_regression_empty_repos() { let config = LinearRegressionConfig { @@ -2634,8 +2518,6 @@ mod tests { assert!(details.contains("0 repos")); } - // --- check_regression details includes threshold --- - #[tokio::test] async fn test_check_regression_details_includes_threshold_percentage() { let config = LinearRegressionConfig { @@ -2668,8 +2550,6 @@ mod tests { ); } - // --- check_appwrite_threads success with no matching content --- - #[tokio::test] async fn test_check_appwrite_threads_html_with_only_tags() { let mock = MockHttpClient::new(vec![(200, "")]); @@ -2685,8 +2565,6 @@ mod tests { assert!(results.is_empty()); } - // --- GitHubSearchResult deserialization --- - #[test] fn test_github_search_result_deserialization() { let json = r#"{ @@ -2730,8 +2608,6 @@ mod tests { assert!(result.items.is_empty()); } - // --- GitHubIssue deserialization --- - #[test] fn test_github_issue_with_null_body() { let json = r#"{ diff --git a/crates/claudear-analysis/src/release/github.rs b/crates/claudear-analysis/src/release/github.rs index 3c4978e8..fa349760 100644 --- a/crates/claudear-analysis/src/release/github.rs +++ b/crates/claudear-analysis/src/release/github.rs @@ -1,7 +1,9 @@ //! GitHub Release API client. -use claudear_core::error::{Error, Result}; -use claudear_core::http::{HttpClient, ReqwestHttpClient}; +use abnegate_http::HttpClient; +use abnegate_http::ReqwestHttpClient; +use claudear_core::error::Error; +use claudear_core::error::Result; use serde::Deserialize; use std::cmp::Ordering; @@ -58,6 +60,11 @@ pub struct GitHubTagCommit { pub sha: Option, } +/// The largest GitHub API response body read where a pull request's diff +/// or a release comparison's commits and patches can arrive: eight times +/// abnegate-http's default. +pub const BODY_LIMIT: usize = 8 * ReqwestHttpClient::DEFAULT_BODY_LIMIT; + /// GitHub Release API client. pub struct ReleaseClient { token: String, @@ -69,7 +76,9 @@ impl ReleaseClient { pub fn new(token: impl Into) -> Self { Self { token: token.into(), - http: ReqwestHttpClient::new(), + http: ReqwestHttpClient::new() + .unwrap_or_else(|_| ReqwestHttpClient::from(reqwest::Client::new())) + .with_body_limit(BODY_LIMIT), } } } @@ -230,7 +239,7 @@ impl ReleaseClient { ))); } - response.json() + Ok(response.json()?) } /// List recent tags for a repository (fallback when Releases is empty). @@ -250,7 +259,7 @@ impl ReleaseClient { ))); } - response.json() + Ok(response.json()?) } /// Get a specific release by tag. @@ -817,8 +826,20 @@ pub struct PrDetails { #[cfg(test)] mod tests { use super::*; + use abnegate_http::HttpResponse; use async_trait::async_trait; - use claudear_core::http::HttpResponse; + + #[test] + fn the_default_transport_reads_a_release_comparison_up_to_the_body_limit() { + let client = ReleaseClient::new("test-token"); + + let transport = format!("{:?}", client.http); + + assert!( + transport.contains(&format!("body_limit: {BODY_LIMIT}")), + "{transport}" + ); + } struct MockHttpClient { response: HttpResponse, @@ -827,21 +848,19 @@ mod tests { impl MockHttpClient { fn new(status: u16, body: &str) -> Self { Self { - response: HttpResponse { - status, - body: body.to_string(), - }, + response: HttpResponse::new(status, body.to_string()), } } } #[async_trait] impl HttpClient for MockHttpClient { - async fn get(&self, _url: &str, _headers: Vec<(&str, String)>) -> Result { - Ok(HttpResponse { - status: self.response.status, - body: self.response.body.clone(), - }) + async fn get( + &self, + _url: &str, + _headers: Vec<(&str, String)>, + ) -> abnegate_http::Result { + Ok(self.response.clone()) } } diff --git a/crates/claudear-analysis/src/release/mod.rs b/crates/claudear-analysis/src/release/mod.rs index 4fdc4af4..e48ac35f 100644 --- a/crates/claudear-analysis/src/release/mod.rs +++ b/crates/claudear-analysis/src/release/mod.rs @@ -8,5 +8,11 @@ mod github; mod tracker; -pub use github::{GitHubRelease, GitHubTag, PrDetails, ReleaseAuthor, ReleaseClient}; -pub use tracker::{ReleaseTracker, ReleaseTrackerConfig}; +pub use github::GitHubRelease; +pub use github::GitHubTag; +pub use github::PrDetails; +pub use github::ReleaseAuthor; +pub use github::ReleaseClient; +pub use github::BODY_LIMIT; +pub use tracker::ReleaseTracker; +pub use tracker::ReleaseTrackerConfig; diff --git a/crates/claudear-analysis/src/release/tracker.rs b/crates/claudear-analysis/src/release/tracker.rs index e59b4479..1b6596b3 100644 --- a/crates/claudear-analysis/src/release/tracker.rs +++ b/crates/claudear-analysis/src/release/tracker.rs @@ -11,9 +11,14 @@ //! - Manual → check release_after use crate::release::ReleaseClient; -use crate::repo::{DependencyType, RepoRelationships}; +use crate::repo::DependencyType; +use crate::repo::RepoRelationships; +use abnegate_http::HttpClient; +use abnegate_http::ReqwestHttpClient; use claudear_core::error::Result; -use claudear_core::types::{RegressionWatch, RegressionWatchStatus, ReleaseTracking}; +use claudear_core::types::RegressionWatch; +use claudear_core::types::RegressionWatchStatus; +use claudear_core::types::ReleaseTracking; use claudear_storage::FixAttemptTracker; use std::collections::HashMap; use std::sync::Arc; @@ -30,9 +35,7 @@ pub struct ReleaseTrackerConfig { } /// Tracks releases to detect when bug fixes are included in production. -pub struct ReleaseTracker< - C: claudear_core::http::HttpClient = claudear_core::http::ReqwestHttpClient, -> { +pub struct ReleaseTracker { client: ReleaseClient, tracker: Arc, config: ReleaseTrackerConfig, @@ -40,7 +43,7 @@ pub struct ReleaseTracker< relationships: RepoRelationships, } -impl ReleaseTracker { +impl ReleaseTracker { /// Create a new release tracker with the default HTTP client. pub fn new(token: impl Into, tracker: Arc) -> Self { Self { @@ -81,7 +84,7 @@ impl ReleaseTracker { } } -impl ReleaseTracker { +impl ReleaseTracker { /// Create a new release tracker with a custom HTTP client. pub fn with_http_client( client: ReleaseClient, @@ -477,13 +480,12 @@ impl ReleaseTracker { }; // Check if the package version in lock file includes the fix - let version_ok = - ReleaseClient::::check_lock_file_version( - &lock_content, - lock_file_path, - package_name, - &min_version, - )?; + let version_ok = ReleaseClient::::check_lock_file_version( + &lock_content, + lock_file_path, + package_name, + &min_version, + )?; if version_ok { tracing::debug!( @@ -645,12 +647,13 @@ impl ReleaseTracker { #[cfg(test)] mod tests { use super::*; + use abnegate_http::HttpResponse; use async_trait::async_trait; - use claudear_core::http::HttpClient; - use claudear_core::http::HttpResponse; use claudear_core::types::IssueType; - use claudear_storage::{AttemptTracker, SqliteTracker}; - use std::sync::atomic::{AtomicUsize, Ordering}; + use claudear_storage::AttemptTracker; + use claudear_storage::SqliteTracker; + use std::sync::atomic::AtomicUsize; + use std::sync::atomic::Ordering; struct MockHttpClient { call_count: AtomicUsize, @@ -663,10 +666,7 @@ mod tests { call_count: AtomicUsize::new(0), responses: responses .into_iter() - .map(|(status, body)| HttpResponse { - status, - body: body.to_string(), - }) + .map(|(status, body)| HttpResponse::new(status, body.to_string())) .collect(), } } @@ -674,19 +674,17 @@ mod tests { #[async_trait] impl HttpClient for MockHttpClient { - async fn get(&self, _url: &str, _headers: Vec<(&str, String)>) -> Result { - let idx = self.call_count.fetch_add(1, Ordering::SeqCst); - if idx < self.responses.len() { - Ok(HttpResponse { - status: self.responses[idx].status, - body: self.responses[idx].body.clone(), - }) - } else { - Ok(HttpResponse { - status: 404, - body: r#"{"message": "Not Found"}"#.to_string(), - }) - } + async fn get( + &self, + _url: &str, + _headers: Vec<(&str, String)>, + ) -> abnegate_http::Result { + let call = self.call_count.fetch_add(1, Ordering::SeqCst); + Ok(self + .responses + .get(call) + .cloned() + .unwrap_or_else(|| HttpResponse::new(404, r#"{"message": "Not Found"}"#))) } } @@ -1311,8 +1309,6 @@ mod tests { assert!(debug_str.contains("ReleaseTrackerConfig")); } - // --- New tests targeting uncovered lines --- - // Covers lines 43-48: ReleaseTracker::new constructor #[test] fn test_release_tracker_new_constructor() { @@ -3144,8 +3140,6 @@ mod tests { assert!(result); } - // --- Config edge cases --- - #[test] fn test_release_tracker_config_empty_package_names() { let config = ReleaseTrackerConfig { @@ -3200,8 +3194,6 @@ mod tests { assert_eq!(config.poll_interval_ms, u64::MAX); } - // --- check_pending_watches edge cases --- - #[tokio::test] async fn test_check_pending_watches_empty_tracker() { // Fresh in-memory tracker has no watches at all @@ -3293,8 +3285,6 @@ mod tests { assert_eq!(updated_a.status, RegressionWatchStatus::Monitoring); } - // --- check_watch_release: URL prefix stripping --- - #[tokio::test] async fn test_check_watch_release_strips_scm_url_prefix() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -3386,8 +3376,6 @@ mod tests { assert_eq!(updated.status, RegressionWatchStatus::Monitoring); } - // --- check_watch_release: no target repos configured --- - #[tokio::test] async fn test_check_watch_release_empty_target_repos() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -3445,8 +3433,6 @@ mod tests { assert!(!result); } - // --- check_watch_release: multiple target repos, second matches --- - #[tokio::test] async fn test_check_watch_release_multiple_targets_second_matches() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -3566,8 +3552,6 @@ mod tests { assert_eq!(updated.status, RegressionWatchStatus::Monitoring); } - // --- check_graph_release: Composer with version NOT ok --- - #[tokio::test] async fn test_check_graph_release_composer_version_too_old() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -3639,8 +3623,6 @@ mod tests { assert!(!result); } - // --- check_graph_release: Npm with no lock file found --- - #[tokio::test] async fn test_check_graph_release_npm_lock_file_not_found() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -3696,8 +3678,6 @@ mod tests { assert!(!result); } - // --- check_graph_release: GitSubmodule with commit NOT in release --- - #[tokio::test] async fn test_check_graph_release_git_submodule_not_in_release() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -3743,8 +3723,6 @@ mod tests { assert!(!result); } - // --- check_graph_release: GitSubmodule with no merge commit sha --- - #[tokio::test] async fn test_check_graph_release_git_submodule_no_merge_commit() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -3788,8 +3766,6 @@ mod tests { assert!(!result); } - // --- check_graph_release: Manual with target before source --- - #[tokio::test] async fn test_check_graph_release_manual_target_before_source() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -3843,8 +3819,6 @@ mod tests { assert!(!result); } - // --- transition_to_monitoring: verify release tracking is recorded --- - #[tokio::test] async fn test_transition_to_monitoring_records_release_tracking() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -3876,8 +3850,6 @@ mod tests { assert_eq!(updated.status, RegressionWatchStatus::Monitoring); } - // --- transition_to_monitoring with LinearBug issue type --- - #[tokio::test] async fn test_transition_to_monitoring_linear_bug() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -3909,8 +3881,6 @@ mod tests { assert_eq!(updated.issue_type, IssueType::LinearBug); } - // --- verify_release_after: source release found but published_at is null -> error --- - #[tokio::test] async fn test_verify_release_after_source_release_no_published_at_errors() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -3969,8 +3939,6 @@ mod tests { assert!(!result); } - // --- verify_release_after: target published just 1 second after --- - #[tokio::test] async fn test_verify_release_after_target_one_second_after() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4011,8 +3979,6 @@ mod tests { assert!(result); } - // --- verify_release_after: invalid source merged_at timestamp --- - #[tokio::test] async fn test_verify_release_after_invalid_source_merged_at() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4052,8 +4018,6 @@ mod tests { assert!(result.is_err()); } - // --- verify_release_after: invalid target published_at timestamp --- - #[tokio::test] async fn test_verify_release_after_invalid_target_published_at() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4092,8 +4056,6 @@ mod tests { assert!(result.is_err()); } - // --- find_dependency_type: transitive chain returns first hop type --- - #[test] fn test_find_dependency_type_transitive_returns_first_hop() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4121,8 +4083,6 @@ mod tests { assert_eq!(dep_type, DependencyType::Npm); } - // --- find_dependency_type: direct dependency --- - #[test] fn test_find_dependency_type_direct_dependency() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4145,8 +4105,6 @@ mod tests { assert_eq!(dep_type, DependencyType::GitSubmodule); } - // --- Npm package name extraction: repo with no slash --- - #[tokio::test] async fn test_npm_package_name_no_slash_uses_whole_name() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4222,8 +4180,6 @@ mod tests { assert!(result); } - // --- check_direct_release_any_target: multiple targets, all fail --- - #[tokio::test] async fn test_check_direct_release_any_target_all_fail() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4267,8 +4223,6 @@ mod tests { assert!(!result); } - // --- check_direct_release_any_target: second target succeeds --- - #[tokio::test] async fn test_check_direct_release_any_target_second_succeeds() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4325,8 +4279,6 @@ mod tests { assert_eq!(updated.status, RegressionWatchStatus::Monitoring); } - // --- verify_lock_file: npm lock with package name override --- - #[tokio::test] async fn test_verify_lock_file_npm_with_scoped_package() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4375,8 +4327,6 @@ mod tests { assert!(result); } - // --- verify_lock_file: composer lock with packages-dev dependency --- - #[tokio::test] async fn test_verify_lock_file_composer_dev_dependency() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4429,8 +4379,6 @@ mod tests { assert!(result); } - // --- verify_lock_file: package not found in lock file --- - #[tokio::test] async fn test_verify_lock_file_package_not_found() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4483,8 +4431,6 @@ mod tests { assert!(!result); } - // --- check_watch_release: fallback to direct when graph has no path --- - #[tokio::test] async fn test_check_watch_release_no_graph_path_fallback_succeeds() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4566,8 +4512,6 @@ mod tests { assert_eq!(updated.status, RegressionWatchStatus::Monitoring); } - // --- check_watch_release: PR returns 404 --- - #[tokio::test] async fn test_check_watch_release_pr_details_404() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4616,8 +4560,6 @@ mod tests { assert!(!result); } - // --- check_direct_release: release found but commit NOT in it --- - #[tokio::test] async fn test_check_direct_release_release_found_commit_diverged() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4659,8 +4601,6 @@ mod tests { assert!(!result); } - // --- verify_commit_ancestry: commit is NOT found (404) --- - #[tokio::test] async fn test_verify_commit_ancestry_404() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4689,8 +4629,6 @@ mod tests { assert!(!result); } - // --- verify_commit_ancestry: commit is identical --- - #[tokio::test] async fn test_verify_commit_ancestry_identical() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4719,8 +4657,6 @@ mod tests { assert!(result); } - // --- config accessor returns reference --- - #[test] fn test_config_accessor_returns_reference() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4747,8 +4683,6 @@ mod tests { ); } - // --- verify_release_after: source release with very distant timestamps --- - #[tokio::test] async fn test_verify_release_after_distant_timestamps() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4798,8 +4732,6 @@ mod tests { assert!(result); } - // --- check_watch_release: fix repo matches target with github prefix --- - #[tokio::test] async fn test_check_watch_release_fix_repo_matches_target_with_github_prefix() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -4873,8 +4805,6 @@ mod tests { assert_eq!(updated.status, RegressionWatchStatus::Monitoring); } - // --- MockHttpClient: exhausted responses returns 404 --- - #[tokio::test] async fn test_mock_http_client_exhausted_responses() { let mock = MockHttpClient::new(vec![(200, r#"{"ok": true}"#)]); @@ -4889,8 +4819,6 @@ mod tests { assert!(resp2.body.contains("Not Found")); } - // --- ReleaseTrackerConfig Debug format --- - #[test] fn test_release_tracker_config_debug_format_with_values() { let mut package_names = HashMap::new(); @@ -4909,8 +4837,6 @@ mod tests { assert!(debug_str.contains("pkg")); } - // --- End-to-end: full check_pending_watches through Composer graph --- - #[tokio::test] async fn test_end_to_end_composer_graph_flow() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -5015,8 +4941,6 @@ mod tests { assert_eq!(updated.status, RegressionWatchStatus::Monitoring); } - // --- End-to-end: full flow with transitive dependency chain --- - #[tokio::test] async fn test_end_to_end_transitive_dependency_chain() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -5118,8 +5042,6 @@ mod tests { assert_eq!(updated.status, RegressionWatchStatus::Monitoring); } - // --- check_graph_release: Manual, no source release, target after merge time --- - #[tokio::test] async fn test_check_graph_release_manual_no_source_release() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -5183,8 +5105,6 @@ mod tests { assert_eq!(updated.status, RegressionWatchStatus::Monitoring); } - // --- RegressionWatch::new sets default fields correctly --- - #[test] fn test_regression_watch_new_defaults() { let watch = RegressionWatch::new(IssueType::LinearBug, "LIN-123", 42); @@ -5199,8 +5119,6 @@ mod tests { assert!(watch.regressed_at.is_none()); } - // --- ReleaseTracking::new sets fields correctly --- - #[test] fn test_release_tracking_new() { let tracking = ReleaseTracking::new(5, "v1.2.3", "org/repo"); @@ -5211,8 +5129,6 @@ mod tests { assert!(tracking.released_at.is_some()); } - // --- check_pending_watches returns empty when all watches are non-AwaitingRelease --- - #[tokio::test] async fn test_check_pending_watches_only_awaiting_release_status() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -5239,8 +5155,6 @@ mod tests { assert!(result.is_empty()); } - // --- find_dependency_type with empty graph --- - #[test] fn test_find_dependency_type_empty_graph() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); @@ -5258,8 +5172,6 @@ mod tests { assert_eq!(dep_type, DependencyType::Manual); } - // --- check_graph_release: Composer with package name from config --- - #[tokio::test] async fn test_check_graph_release_composer_with_custom_package_name() { let tracker = Arc::new(SqliteTracker::in_memory().unwrap()); diff --git a/crates/claudear-integrations/src/scm.rs b/crates/claudear-integrations/src/scm.rs index 7a42ada6..3fde54c6 100644 --- a/crates/claudear-integrations/src/scm.rs +++ b/crates/claudear-integrations/src/scm.rs @@ -3,20 +3,22 @@ //! Defines a common trait and shared types used by both GitHub and GitLab //! backends for PR monitoring and review watching. -use abnegate_http::ReqwestHttpClient; use async_trait::async_trait; use claudear_core::error::Result; -use claudear_core::types::{ - ActivityLogEntry, FixAttempt, IssueType, PrReviewRecord, RegressionWatch, -}; +use claudear_core::types::ActivityLogEntry; +use claudear_core::types::FixAttempt; +use claudear_core::types::IssueType; +use claudear_core::types::PrReviewRecord; +use claudear_core::types::RegressionWatch; use claudear_storage::FixAttemptTracker; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; +use serde::Serialize; use std::sync::Arc; -pub use claudear_core::types::{PrReviewState, ReviewComment, ReviewUser}; - -/// Room for the unified diff of a large pull request. -pub const BODY_LIMIT: usize = 8 * ReqwestHttpClient::DEFAULT_BODY_LIMIT; +pub use claudear_analysis::release::BODY_LIMIT; +pub use claudear_core::types::PrReviewState; +pub use claudear_core::types::ReviewComment; +pub use claudear_core::types::ReviewUser; /// Check whether a bot user should be skipped based on the allowed-bots list. /// From 1c13990d15214c3557a23daa6817fe0e77ce3b76 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Mon, 5 Oct 2026 18:44:31 +1300 Subject: [PATCH 4/5] refactor(core): delete the local http client Nothing imports claudear_core::http any more: every consumer uses abnegate-http 0.1.2's HttpClient, HttpResponse and ReqwestHttpClient, and claudear-core converts the crate's errors in one From impl. The module, its claudear_core::http path and the claudear::http re-export in src/lib.rs go, so there is one HTTP client implementation to maintain. Its HttpResponse tests live on in the crate. Co-Authored-By: Claude Opus 5.5 --- crates/claudear-core/src/http.rs | 242 ------------------------------- crates/claudear-core/src/lib.rs | 5 +- src/lib.rs | 1 - 3 files changed, 2 insertions(+), 246 deletions(-) delete mode 100644 crates/claudear-core/src/http.rs diff --git a/crates/claudear-core/src/http.rs b/crates/claudear-core/src/http.rs deleted file mode 100644 index 3ffe1914..00000000 --- a/crates/claudear-core/src/http.rs +++ /dev/null @@ -1,242 +0,0 @@ -//! Shared HTTP types used by source and client HTTP abstractions. - -use crate::error::{Error, Result}; -use async_trait::async_trait; - -/// HTTP response abstraction for testability. -/// -/// Used by source adapters (Linear, Sentry) and API clients (Discord) -/// to abstract over the actual HTTP client for unit testing. -#[derive(Debug)] -pub struct HttpResponse { - pub status: u16, - pub body: String, -} - -impl HttpResponse { - /// Check if the status is successful (2xx). - pub fn is_success(&self) -> bool { - (200..300).contains(&self.status) - } - - /// Check if the status is 404 Not Found. - pub fn is_not_found(&self) -> bool { - self.status == 404 - } - - /// Parse the body as JSON. - pub fn json(&self) -> Result { - serde_json::from_str(&self.body) - .map_err(|e| Error::Other(format!("JSON parse error: {}", e))) - } -} - -/// Trait for HTTP client operations to enable testing. -#[async_trait] -pub trait HttpClient: Send + Sync { - /// Perform a GET request with headers. - async fn get(&self, url: &str, headers: Vec<(&str, String)>) -> Result; - - /// Perform a POST request with headers and a body. - async fn post( - &self, - url: &str, - headers: Vec<(&str, String)>, - body: &str, - ) -> Result { - let _ = (url, headers, body); - Err(Error::Other( - "POST not supported by this HTTP client".into(), - )) - } - - /// Perform a PUT request with headers and a body. - async fn put( - &self, - url: &str, - headers: Vec<(&str, String)>, - body: &str, - ) -> Result { - let _ = (url, headers, body); - Err(Error::Other("PUT not supported by this HTTP client".into())) - } - - /// Perform a PATCH request with headers and a body. - async fn patch( - &self, - url: &str, - headers: Vec<(&str, String)>, - body: &str, - ) -> Result { - let _ = (url, headers, body); - Err(Error::Other( - "PATCH not supported by this HTTP client".into(), - )) - } - - /// Perform a DELETE request with headers. - async fn delete(&self, url: &str, headers: Vec<(&str, String)>) -> Result { - let _ = (url, headers); - Err(Error::Other( - "DELETE not supported by this HTTP client".into(), - )) - } -} - -/// Default HTTP client using reqwest. -pub struct ReqwestHttpClient { - client: reqwest::Client, -} - -impl ReqwestHttpClient { - /// Create a new reqwest-based HTTP client. - pub fn new() -> Self { - Self { - client: reqwest::Client::builder() - .timeout(std::time::Duration::from_secs(30)) - .connect_timeout(std::time::Duration::from_secs(10)) - .build() - .unwrap_or_else(|_| reqwest::Client::new()), - } - } -} - -impl Default for ReqwestHttpClient { - fn default() -> Self { - Self::new() - } -} - -#[async_trait] -impl HttpClient for ReqwestHttpClient { - async fn get(&self, url: &str, headers: Vec<(&str, String)>) -> Result { - let mut request = self.client.get(url); - for (name, value) in headers { - request = request.header(name, value); - } - let response = request.send().await?; - let status = response.status().as_u16(); - let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) - } - - async fn post( - &self, - url: &str, - headers: Vec<(&str, String)>, - body: &str, - ) -> Result { - let mut request = self.client.post(url); - for (name, value) in headers { - request = request.header(name, value); - } - let response = request.body(body.to_string()).send().await?; - let status = response.status().as_u16(); - let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) - } - - async fn put( - &self, - url: &str, - headers: Vec<(&str, String)>, - body: &str, - ) -> Result { - let mut request = self.client.put(url); - for (name, value) in headers { - request = request.header(name, value); - } - let response = request.body(body.to_string()).send().await?; - let status = response.status().as_u16(); - let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) - } - - async fn patch( - &self, - url: &str, - headers: Vec<(&str, String)>, - body: &str, - ) -> Result { - let mut request = self.client.patch(url); - for (name, value) in headers { - request = request.header(name, value); - } - let response = request.body(body.to_string()).send().await?; - let status = response.status().as_u16(); - let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) - } - - async fn delete(&self, url: &str, headers: Vec<(&str, String)>) -> Result { - let mut request = self.client.delete(url); - for (name, value) in headers { - request = request.header(name, value); - } - let response = request.send().await?; - let status = response.status().as_u16(); - let body = response.text().await.unwrap_or_default(); - Ok(HttpResponse { status, body }) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_http_response_success_200() { - let resp = HttpResponse { - status: 200, - body: String::new(), - }; - assert!(resp.is_success()); - } - - #[test] - fn test_http_response_success_299() { - let resp = HttpResponse { - status: 299, - body: String::new(), - }; - assert!(resp.is_success()); - } - - #[test] - fn test_http_response_failure_400() { - let resp = HttpResponse { - status: 400, - body: String::new(), - }; - assert!(!resp.is_success()); - } - - #[test] - fn test_http_response_failure_500() { - let resp = HttpResponse { - status: 500, - body: String::new(), - }; - assert!(!resp.is_success()); - } - - #[test] - fn test_http_response_json_valid() { - let resp = HttpResponse { - status: 200, - body: r#"{"key": "value"}"#.to_string(), - }; - let parsed: std::collections::HashMap = resp.json().unwrap(); - assert_eq!(parsed.get("key").unwrap(), "value"); - } - - #[test] - fn test_http_response_json_invalid() { - let resp = HttpResponse { - status: 200, - body: "not json".to_string(), - }; - let result: Result = resp.json(); - assert!(result.is_err()); - } -} diff --git a/crates/claudear-core/src/lib.rs b/crates/claudear-core/src/lib.rs index 40bbba73..1eaa0156 100644 --- a/crates/claudear-core/src/lib.rs +++ b/crates/claudear-core/src/lib.rs @@ -1,10 +1,9 @@ //! Core foundation types for the claudear application. //! -//! This crate provides the shared types, error handling, HTTP abstractions, -//! secret management, and template rendering used across all claudear crates. +//! This crate provides the shared types, error handling, secret management, +//! and template rendering used across all claudear crates. pub mod error; -pub mod http; pub mod secret; pub mod templates; pub mod types; diff --git a/src/lib.rs b/src/lib.rs index b069c055..2e991727 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -27,7 +27,6 @@ // Re-exported from claudear-core pub use claudear_core::error; -pub use claudear_core::http; pub use claudear_core::secret; pub use claudear_core::templates; pub use claudear_core::types; From aad70a3ff7b3587d0f59ea3b8d4fb12cb5baf483 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 9 Oct 2026 15:51:44 +1300 Subject: [PATCH 5/5] test(http): pin the body limits by what the clients read, not by debug output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The constructor tests read `body_limit: …` out of ReqwestHttpClient's derived Debug, which the crate can reformat without changing behaviour. Each client's `new` now hands its transport to a private `with_transport` that applies BODY_LIMIT, and the tests drive that path against a loopback server: a body over the crate default is read, and a declared length over BODY_LIMIT is refused as OversizedBody { limit: BODY_LIMIT }. The tests inject only the reqwest client, built without a proxy, so an environment proxy cannot reroute loopback. The loopback helpers move to claudear-analysis behind a `test-support` feature so the release client's tests can share them with integrations. Co-Authored-By: Claude Opus 5.5 --- crates/claudear-analysis/Cargo.toml | 1 + crates/claudear-analysis/src/lib.rs | 2 + .../claudear-analysis/src/release/github.rs | 48 +++++++++++++++---- .../src/test_support.rs | 8 ++-- crates/claudear-integrations/Cargo.toml | 1 + crates/claudear-integrations/src/github.rs | 38 +++++---------- crates/claudear-integrations/src/gitlab.rs | 35 +++++--------- crates/claudear-integrations/src/lib.rs | 2 - 8 files changed, 72 insertions(+), 63 deletions(-) rename crates/{claudear-integrations => claudear-analysis}/src/test_support.rs (86%) diff --git a/crates/claudear-analysis/Cargo.toml b/crates/claudear-analysis/Cargo.toml index 313787f8..5d74c110 100644 --- a/crates/claudear-analysis/Cargo.toml +++ b/crates/claudear-analysis/Cargo.toml @@ -10,6 +10,7 @@ workspace = true default = [] sqlite = ["claudear-core/sqlite", "claudear-storage/sqlite"] cuda = ["ort/cuda"] +test-support = [] [dependencies] abnegate-http = { workspace = true } diff --git a/crates/claudear-analysis/src/lib.rs b/crates/claudear-analysis/src/lib.rs index 8117a817..197e19ed 100644 --- a/crates/claudear-analysis/src/lib.rs +++ b/crates/claudear-analysis/src/lib.rs @@ -16,3 +16,5 @@ pub mod qa; pub mod regression; pub mod release; pub mod repo; +#[cfg(any(test, feature = "test-support"))] +pub mod test_support; diff --git a/crates/claudear-analysis/src/release/github.rs b/crates/claudear-analysis/src/release/github.rs index fa349760..073ee3bb 100644 --- a/crates/claudear-analysis/src/release/github.rs +++ b/crates/claudear-analysis/src/release/github.rs @@ -74,11 +74,15 @@ pub struct ReleaseClient { impl ReleaseClient { /// Create a new release client with the default HTTP client. pub fn new(token: impl Into) -> Self { + let transport = ReqwestHttpClient::new() + .unwrap_or_else(|_| ReqwestHttpClient::from(reqwest::Client::new())); + Self::with_transport(token, transport) + } + + fn with_transport(token: impl Into, transport: ReqwestHttpClient) -> Self { Self { token: token.into(), - http: ReqwestHttpClient::new() - .unwrap_or_else(|_| ReqwestHttpClient::from(reqwest::Client::new())) - .with_body_limit(BODY_LIMIT), + http: transport.with_body_limit(BODY_LIMIT), } } } @@ -826,18 +830,44 @@ pub struct PrDetails { #[cfg(test)] mod tests { use super::*; + use crate::test_support::loopback_transport; + use crate::test_support::ok_response; + use crate::test_support::serve_once; use abnegate_http::HttpResponse; use async_trait::async_trait; - #[test] - fn the_default_transport_reads_a_release_comparison_up_to_the_body_limit() { - let client = ReleaseClient::new("test-token"); + #[tokio::test] + async fn a_release_comparison_larger_than_the_crate_default_is_read() { + let size = ReqwestHttpClient::DEFAULT_BODY_LIMIT + 1; + let url = serve_once(ok_response(size, &vec![b'+'; size])).await; + let client = ReleaseClient::with_transport("test-token", loopback_transport()); + + let response = client + .http + .get(&url, Vec::new()) + .await + .expect("a comparison under the body limit is read"); - let transport = format!("{:?}", client.http); + assert_eq!(response.body.len(), size); + } + + #[tokio::test] + async fn a_release_comparison_over_the_body_limit_is_refused() { + let url = serve_once(ok_response(BODY_LIMIT + 1, b"")).await; + let client = ReleaseClient::with_transport("test-token", loopback_transport()); + + let error = client + .http + .get(&url, Vec::new()) + .await + .expect_err("a declared length over the body limit is refused"); assert!( - transport.contains(&format!("body_limit: {BODY_LIMIT}")), - "{transport}" + matches!( + error, + abnegate_http::Error::OversizedBody { limit, .. } if limit == BODY_LIMIT + ), + "{error}" ); } diff --git a/crates/claudear-integrations/src/test_support.rs b/crates/claudear-analysis/src/test_support.rs similarity index 86% rename from crates/claudear-integrations/src/test_support.rs rename to crates/claudear-analysis/src/test_support.rs index ff3c2801..ef6c5148 100644 --- a/crates/claudear-integrations/src/test_support.rs +++ b/crates/claudear-analysis/src/test_support.rs @@ -46,12 +46,12 @@ pub fn ok_response(length: usize, body: &[u8]) -> Vec { response } -/// A transport that reaches loopback directly, whatever proxy the -/// environment configures, and reads bodies up to `body_limit` bytes. -pub fn loopback_transport(body_limit: usize) -> ReqwestHttpClient { +/// A transport with the crate's default body limit that reaches loopback +/// directly, whatever proxy the environment configures. +pub fn loopback_transport() -> ReqwestHttpClient { let client = reqwest::Client::builder() .no_proxy() .build() .expect("a client without a proxy builds"); - ReqwestHttpClient::from(client).with_body_limit(body_limit) + ReqwestHttpClient::from(client) } diff --git a/crates/claudear-integrations/Cargo.toml b/crates/claudear-integrations/Cargo.toml index ea3c2153..d03553e8 100644 --- a/crates/claudear-integrations/Cargo.toml +++ b/crates/claudear-integrations/Cargo.toml @@ -107,5 +107,6 @@ openssl = { version = "0.10", features = ["vendored"] } libc = { workspace = true } [dev-dependencies] +claudear-analysis = { workspace = true, features = ["test-support"] } tempfile = { workspace = true } mockall = { workspace = true } diff --git a/crates/claudear-integrations/src/github.rs b/crates/claudear-integrations/src/github.rs index c5bdc803..2854a1b5 100644 --- a/crates/claudear-integrations/src/github.rs +++ b/crates/claudear-integrations/src/github.rs @@ -86,13 +86,13 @@ pub struct GitHubLabel { impl GitHubClient { /// Create a new GitHub client with the default HTTP client. pub fn new(config: GitHubConfig) -> Self { - Self { - config, - http: ReqwestHttpClient::new() - .unwrap_or_else(|_| ReqwestHttpClient::from(reqwest::Client::new())) - .with_body_limit(BODY_LIMIT), - self_login: std::sync::OnceLock::new(), - } + let transport = ReqwestHttpClient::new() + .unwrap_or_else(|_| ReqwestHttpClient::from(reqwest::Client::new())); + Self::with_transport(config, transport) + } + + fn with_transport(config: GitHubConfig, transport: ReqwestHttpClient) -> Self { + Self::with_http_client(config, transport.with_body_limit(BODY_LIMIT)) } } @@ -1173,10 +1173,10 @@ impl ScmProvider for GitHubClient { #[cfg(test)] mod tests { use super::*; - use crate::test_support::loopback_transport; - use crate::test_support::ok_response; - use crate::test_support::serve_once; use abnegate_http::HttpResponse; + use claudear_analysis::test_support::loopback_transport; + use claudear_analysis::test_support::ok_response; + use claudear_analysis::test_support::serve_once; use std::collections::HashMap; use std::sync::Arc; use std::sync::Mutex; @@ -4022,23 +4022,11 @@ mod tests { } } - #[test] - fn the_default_transport_reads_up_to_the_scm_limit() { - let client = GitHubClient::new(test_config()); - - let transport = format!("{:?}", client.http); - - assert!( - transport.contains(&format!("body_limit: {BODY_LIMIT}")), - "{transport}" - ); - } - #[tokio::test] async fn the_scm_limit_admits_a_diff_larger_than_the_crate_default() { let size = ReqwestHttpClient::DEFAULT_BODY_LIMIT + 1; let url = serve_once(ok_response(size, &vec![b'+'; size])).await; - let client = GitHubClient::with_http_client(test_config(), loopback_transport(BODY_LIMIT)); + let client = GitHubClient::with_transport(test_config(), loopback_transport()); let response = client .http @@ -4052,7 +4040,7 @@ mod tests { #[tokio::test] async fn a_body_cut_short_is_a_transient_error_rather_than_an_empty_success() { let url = serve_once(ok_response(100, b"short")).await; - let client = GitHubClient::with_http_client(test_config(), loopback_transport(BODY_LIMIT)); + let client = GitHubClient::with_transport(test_config(), loopback_transport()); let error = client .http @@ -4068,7 +4056,7 @@ mod tests { #[tokio::test] async fn the_scm_limit_refuses_a_larger_body() { let url = serve_once(ok_response(BODY_LIMIT + 1, b"")).await; - let client = GitHubClient::with_http_client(test_config(), loopback_transport(BODY_LIMIT)); + let client = GitHubClient::with_transport(test_config(), loopback_transport()); let error = client .http diff --git a/crates/claudear-integrations/src/gitlab.rs b/crates/claudear-integrations/src/gitlab.rs index c9b9c1b5..04b8c6d7 100644 --- a/crates/claudear-integrations/src/gitlab.rs +++ b/crates/claudear-integrations/src/gitlab.rs @@ -116,12 +116,13 @@ pub struct GitLabIssue { impl GitLabClient { /// Create a new GitLab client with the default HTTP client. pub fn new(config: GitLabConfig) -> Self { - Self { - config, - http: ReqwestHttpClient::new() - .unwrap_or_else(|_| ReqwestHttpClient::from(reqwest::Client::new())) - .with_body_limit(BODY_LIMIT), - } + let transport = ReqwestHttpClient::new() + .unwrap_or_else(|_| ReqwestHttpClient::from(reqwest::Client::new())); + Self::with_transport(config, transport) + } + + fn with_transport(config: GitLabConfig, transport: ReqwestHttpClient) -> Self { + Self::with_http_client(config, transport.with_body_limit(BODY_LIMIT)) } } @@ -1033,10 +1034,10 @@ impl ScmProvider for GitLabClient { #[cfg(test)] mod tests { use super::*; - use crate::test_support::loopback_transport; - use crate::test_support::ok_response; - use crate::test_support::serve_once; use abnegate_http::HttpResponse; + use claudear_analysis::test_support::loopback_transport; + use claudear_analysis::test_support::ok_response; + use claudear_analysis::test_support::serve_once; use claudear_config::config::GitLabConfig; use std::collections::HashMap; use std::sync::Mutex; @@ -1177,23 +1178,11 @@ mod tests { config } - #[test] - fn the_default_transport_reads_up_to_the_scm_limit() { - let client = GitLabClient::new(test_config()); - - let transport = format!("{:?}", client.http); - - assert!( - transport.contains(&format!("body_limit: {BODY_LIMIT}")), - "{transport}" - ); - } - #[tokio::test] async fn the_scm_limit_admits_a_diff_larger_than_the_crate_default() { let size = ReqwestHttpClient::DEFAULT_BODY_LIMIT + 1; let url = serve_once(ok_response(size, &vec![b'+'; size])).await; - let client = GitLabClient::with_http_client(test_config(), loopback_transport(BODY_LIMIT)); + let client = GitLabClient::with_transport(test_config(), loopback_transport()); let response = client .http @@ -1207,7 +1196,7 @@ mod tests { #[tokio::test] async fn the_scm_limit_refuses_a_larger_body() { let url = serve_once(ok_response(BODY_LIMIT + 1, b"")).await; - let client = GitLabClient::with_http_client(test_config(), loopback_transport(BODY_LIMIT)); + let client = GitLabClient::with_transport(test_config(), loopback_transport()); let error = client .http diff --git a/crates/claudear-integrations/src/lib.rs b/crates/claudear-integrations/src/lib.rs index b6cc275a..8a2dfff8 100644 --- a/crates/claudear-integrations/src/lib.rs +++ b/crates/claudear-integrations/src/lib.rs @@ -17,7 +17,5 @@ pub mod runner; pub mod scm; pub mod source; pub mod telemetry; -#[cfg(test)] -mod test_support; pub mod tls; pub mod webhook;