feat(callout): add praxis-proxy-callout crate#663
Conversation
|
PR too large: 1461 lines added (limit: 750, excludes Cargo files, tests, docs, examples, and benchmarks). Please split into smaller PRs. Add |
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Adds praxis-proxy-callout crate — a standalone reqwest-based HTTP callout client with circuit breaker, loop-depth prevention, and fail-open/closed semantics. PR 1 of 3 for #358.
Overall the architecture and code quality are strong. The circuit breaker state machine is correct, the test coverage is thorough (45 tests), and the crate is properly standalone. A few issues around response body handling, workspace consistency, and missing #![deny(unsafe_code)].
| Severity | Count |
|---|---|
| Critical | 1 |
| Large | 3 |
| Medium | 5 |
Non-inline findings
- [Medium] The
tests/smokeworkspace member present on main is missing from the PR'sCargo.tomlmemberslist. Verify this was not accidentally dropped during the rebase. - [Medium] The PR bumps
rust-versionfrom1.94to1.96and upgrades several existing dependency versions (bytes,chrono,h2,http,prost,regex,zeroize, etc.) plus changes Pingora from git to crates.io. These are unrelated to the callout crate and should be split into a separate PR or at minimum noted in the PR description.
| None | ||
| } | ||
|
|
||
| /// Build and send the HTTP request with timeout. |
There was a problem hiding this comment.
[Critical] process_response takes &reqwest::Response but never reads the body — CalloutResponse::body is always Vec::new(). The response body is consumed by response.bytes().await, which requires ownership or &mut. Since the caller (execute) has an owned Response, change process_response to take ownership and .bytes().await the body. As-is, every successful callout returns an empty body, which will make the JSONPath extraction in PR 2 impossible.
async fn process_response(&self, response: reqwest::Response) -> CalloutResult {
let status = response.status();
if !status.is_success() { ... }
self.record_success();
let headers = extract_headers(&response);
let body = response.bytes().await.map(|b| b.to_vec()).unwrap_or_default();
CalloutResult::Success(CalloutResponse { body, headers, status: status.as_u16() })
}| @@ -0,0 +1,435 @@ | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
[Large] Missing #![deny(unsafe_code)] at the crate root. Project convention (CLAUDE.md: "#![deny(unsafe_code)] in all crate roots") requires this. Add it after the license/copyright header.
| /// match client.execute(req).await { | ||
| /// CalloutResult::Success(resp) => { /* use resp */ }, | ||
| /// CalloutResult::Failed => { /* proceed anyway */ }, | ||
| /// CalloutResult::Rejected(r) => { /* reject with r.status */ }, |
There was a problem hiding this comment.
[Large] The doctest uses ignore instead of being runnable. Project conventions prefer ample, runnable doctests. Since CalloutClient::new is sync and only execute is async, the constructor portion can at least be a no_run test:
/// ```no_run
/// use praxis_callout::{CalloutClient, CalloutConfig};
///
/// let client = CalloutClient::new(CalloutConfig::default()).unwrap();
/// ```
If the full async example is needed, use tokio::main with no_run.
| } | ||
| } | ||
|
|
||
| // ----------------------------------------------------------------------------- |
There was a problem hiding this comment.
[Medium] extract_headers silently drops headers with non-UTF-8 values via filter_map + to_str().ok(). This is a reasonable default for most use cases, but a tracing::debug! for dropped headers would aid debugging when a callout target returns binary header values that the caller expects to see.
| cb.record_failure(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[Medium] name.to_string() on a HeaderName triggers the str_to_string lint that is denied in the workspace. HeaderName implements Display, not &str-to-String, so this is technically to_string() on a non-string type — which is the allowed case per conventions. However, verify this compiles under the workspace lint profile since str_to_string is denied and the lint sometimes fires on Display impls depending on the clippy version.
| } | ||
|
|
||
| // ----------------------------------------------------------------- | ||
| // Private helpers |
There was a problem hiding this comment.
[Medium] body.clone() in send_request copies the entire request body. For large bodies (the proposal mentions 1 MiB max), consider taking body by value from CalloutRequest (the execute method already takes ownership of the request). Change the signature to async fn send_request(&self, request: CalloutRequest) and use request.body directly instead of borrowing.
| { | ||
| inner.state = CircuitState::HalfOpen; | ||
| CircuitState::HalfOpen | ||
| } else { |
There was a problem hiding this comment.
[Large] In check(), when the state is HalfOpen, it returns Open (line 97). The doc comment says "the first caller gets HalfOpen (probe allowed); subsequent callers still see Open." This is correct for single-probe semantics, but there is a race: between check() returning HalfOpen and the probe completing (calling record_success/record_failure), concurrent callers see Open and are rejected. If the probe is slow, a burst of legitimate requests is rejected unnecessarily.
Consider adding a probing: bool field to CircuitInner that check() sets when transitioning to HalfOpen, so the state remains HalfOpen (not Open) for concurrent readers — they would still be rejected, but the observability would be accurate. Not blocking, but worth documenting as a known limitation.
| //! Unit tests for the callout client. | ||
|
|
||
| #[expect(clippy::allow_attributes, reason = "blanket test suppressions")] | ||
| #[allow( |
There was a problem hiding this comment.
[Medium] The blanket #[allow(clippy::missing_assert_message)] suppresses the workspace-required missing_assert_message lint for all tests. The project convention (CLAUDE.md) says every assert! needs a message string. Most assert!(matches!(...)) calls here would benefit from a failure message (e.g., "expected Success variant"). Consider removing the blanket allow and adding messages to each assertion.
| @@ -75,6 +77,7 @@ serde = { version = "1.0.228", features = ["derive", "rc"] } | |||
| serde_json = "1.0.150" | |||
There was a problem hiding this comment.
[Medium] reqwest = { version = "0.12.18", ... } — the Cargo.lock resolves to 0.12.28. Since this is a workspace dependency, consider pinning to the actual resolved version (0.12.28) to be explicit about which version is tested and to match the Cargo.lock. Same for wiremock = "0.6.4" resolving to 0.6.5.
d5a8423 to
25f053e
Compare
1311f9a to
8d07f37
Compare
Add the design and implementation plan for the http_callout filter: - Per-instance reqwest::Client with rustls-tls, connection and TLS isolation for multi-tenant safety (no shared pools across filter instances) - RFC 9535 JSONPath extraction via serde_json_path with config-time validation - Satellite crate at filter/http-callout/ behind an opt-in cargo feature flag, following the ext-proc pattern - Circuit breaker reuse from existing state machine - SSRF prevention via static config-declared targets - Fail-open/closed semantics, phase selection, body forwarding - 4-PR implementation sequence - Documents relationship to praxis-proxy#138 (ai_guardrails) with two integration paths Assisted by Opus 4.6 Signed-off-by: usize <mofoster@redhat.com>
These are out of scope for prior art. Signed-off-by: Morgan Foster <39788015+usize@users.noreply.github.com> Signed-off-by: usize <mofoster@redhat.com>
Added details about the HTTP call-out filter implementation and requirements. Signed-off-by: Morgan Foster <39788015+usize@users.noreply.github.com> Signed-off-by: usize <mofoster@redhat.com>
Updated the HTTP callout filter documentation by removing outdated sections and renumbering subsequent items. Signed-off-by: Morgan Foster <39788015+usize@users.noreply.github.com> Signed-off-by: usize <mofoster@redhat.com>
- Restore Lakera Guard and OpenAI Moderation API to Prior Art (removed earlier but still used as examples throughout) - Add request_headers phase entry branch to request flow diagram - Remove redundant body_from config field (implied by phase) Assisted by Opus 4.6 Signed-off-by: usize <mofoster@redhat.com>
Standalone HTTP callout client library with timeout enforcement, circuit breaking, and callout-depth loop prevention. No dependency on praxis-core or praxis-filter — any crate can use it for outbound HTTP requests. Public API: CalloutClient, CalloutConfig, CalloutRequest, CalloutResponse, CalloutResult, FailureMode, Rejection, CalloutError, CircuitBreakerConfig, DEPTH_HEADER. 45 unit tests (wiremock) covering circuit breaker state machine, config validation, happy path, failure modes, loop prevention, and circuit breaker integration. Refs: praxis-proxy#358 Assisted by Opus 4.6 Signed-off-by: usize <mofoster@redhat.com>
Fixes RUSTSEC-2026-0185 (remote memory exhaustion from unbounded out-of-order stream reassembly). Assisted by Opus 4.6 Signed-off-by: usize <mofoster@redhat.com>
record_success() was called before response.bytes().await, so the circuit breaker recorded success even if the body transfer failed. The body-read error was also silently swallowed via unwrap_or_default, reporting Success to the caller with an empty body. Move record_success() after the body is fully consumed and treat a body-read error as a failure (record_failure + apply failure_mode). Assisted by Opus 4.6 Signed-off-by: usize <mofoster@redhat.com>
The doc comment on check() said a dropped probe would leave callers seeing Open "until the recovery window elapses again." In reality, opened_at is not reset when entering HalfOpen, so the recovery window has already elapsed and the next caller immediately gets a new HalfOpen probe opportunity. Assisted by Opus 4.6 Signed-off-by: usize <mofoster@redhat.com>
Move the callout client into `core/src/callout/` so it becomes a first-class framework primitive available to any crate that depends on `praxis-core`, including custom proxy builds. The standalone `praxis-proxy-callout` crate is removed; its source files move into `core/src/callout/` with only import-path adjustments (no behavioral changes). Assisted by Opus 4.6
28058ed to
7abe5dc
Compare
This establishes a standalone crate for a reqwest-based callout client. It has its own circuit breaker, loop detection, and error handling logic. The loop detection in particular is built with the intention of supporting callouts from within filters.
Part of #358. Follow-ups will use this client to implement a reference filter implementation, capable of handling arbitrary HTTP requests from within filters, for easy integration with external guardrails systems and other similar services.