feat(http): add canonical outbound HTTP foundation - #2209
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR introduces the transport-neutral outbound HTTP foundation for NeMo Guardrails library rails: a
|
| Filename | Overview |
|---|---|
| nemoguardrails/http/transport.py | New HTTPX adapter translating transport errors to neutral types; uses asyncio.wait_for for total-operation deadline with HTTPX internal timeouts disabled; owned vs injected client distinction enforced at construction time. |
| nemoguardrails/http/retry.py | New RetryPolicy dataclass and RetryingHTTPClient decorator; bounded exponential backoff with jitter; Retry-After parsing; X-Should-Retry opt-in; context manager and idempotent close confirmed correct. |
| nemoguardrails/http/types.py | Frozen HTTPRequest and HTTPResponse dataclasses; charset-aware text decoding with LookupError fallback; json() raises HTTPResponseDecodeError without leaking body. |
| nemoguardrails/http/_url.py | sanitize_url strips credentials, query params, and fragment; handles IPv6, invalid ports, missing hostname, and relative URLs correctly. |
| nemoguardrails/http/errors.py | Clean exception hierarchy under HTTPClientError; retry_count on base class; URL credential redaction in HTTPStatusError. |
| nemoguardrails/http/client.py | runtime_checkable HTTPClient and ClosableHTTPClient protocols; no concrete inheritance required from implementors. |
| nemoguardrails/testing/http.py | RecordingHTTPClient queues responses or exceptions, records requests, tracks close_calls; satisfies both protocols structurally. |
| tests/http/test_retry.py | Comprehensive retry tests covering method gating, status triggers, Retry-After, X-Should-Retry, transport errors, context manager, clamped backoff, and boundary validation. |
| tests/http/test_transport.py | Tests cover forwarding, error translation, close idempotency, deadline enforcement, timeout validation, redirect flag, and injected-client option rejection. |
| tests/http/test_contract.py | Contract tests verify text/JSON parsing, charset fallback, credential redaction, protocol satisfaction, and exception hierarchy. |
| tests/http/test_url.py | Parametrized sanitize_url tests cover standard, IPv6, missing hostname, scheme-relative, invalid port, relative, query-only, and empty inputs. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Retrying as RetryingHTTPClient
participant Transport as HttpxHTTPClient
participant HTTPX as httpx.AsyncClient
Caller->>Retrying: request(method, url, ...)
Note over Retrying: can_retry_method = policy.can_retry_method(method)
loop Up to max_attempts
Retrying->>Transport: request(method, url, ...)
Transport->>HTTPX: "asyncio.wait_for(client.request(...), timeout=deadline)"
alt Success
HTTPX-->>Transport: httpx.Response (buffered)
Transport-->>Retrying: HTTPResponse(status_code, headers, content, extensions)
alt should_retry(response) AND attempts remain
Note over Retrying: sleep(retry_after or backoff)
Note over Retrying: retries += 1
else Done
Note over Retrying: inject retry_count into extensions
Retrying-->>Caller: HTTPResponse with retry_count
end
else asyncio.TimeoutError
Transport-->>Retrying: HTTPTimeoutError
alt retry_transport_errors AND attempts remain
Note over Retrying: sleep(backoff)
else
Retrying-->>Caller: "HTTPTimeoutError(retry_count=N)"
end
else httpx.RequestError
Transport-->>Retrying: HTTPConnectionError
alt retry_transport_errors AND attempts remain
Note over Retrying: sleep(backoff)
else
Retrying-->>Caller: "HTTPConnectionError(retry_count=N)"
end
end
end
Reviews (5): Last reviewed commit: "test(http): cover outbound client branch..." | Re-trigger Greptile
📝 WalkthroughWalkthroughIntroduces a transport-neutral asynchronous HTTP client package with request/response types, standardized errors, an HTTPX adapter, configurable retries, lifecycle management, public exports, and deterministic test coverage. ChangesAsync HTTP Client
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant RetryingHTTPClient
participant HttpxHTTPClient
participant httpx.AsyncClient
Caller->>RetryingHTTPClient: request(method, url, request data)
RetryingHTTPClient->>HttpxHTTPClient: send attempt
HttpxHTTPClient->>httpx.AsyncClient: execute HTTP request
httpx.AsyncClient-->>HttpxHTTPClient: response or transport exception
HttpxHTTPClient-->>RetryingHTTPClient: HTTPResponse or mapped error
RetryingHTTPClient-->>Caller: retry or return final result
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemoguardrails/http/errors.py`:
- Around line 25-36: Update the hostname-None fallback in _safe_url so it
removes any userinfo from parts.netloc before rebuilding the URL, including
empty-host values such as user:pass@. Preserve the existing scheme, path, and
query/fragment redaction behavior, and add coverage for this credential-bearing
fallback case.
In `@nemoguardrails/http/transport.py`:
- Around line 35-64: Update the HTTP request exception handling in the transport
adapter to catch all relevant httpx.RequestError subclasses, including
DecodingError and TooManyRedirects, rather than only TransportError. Preserve
the existing transport-neutral wrapping and retry behavior for these errors.
- Around line 106-116: Update the exception mapping in the HTTP request flow
around the client request and existing httpx exception handlers to catch
request-level httpx failures, including httpx.DecodingError and
httpx.TooManyRedirects, and translate them into the established neutral
HTTPConnectionError path. Preserve the existing timeout mappings and exception
chaining.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4aad114e-00e6-4f57-bbce-0bf8a33a34cd
📒 Files selected for processing (10)
nemoguardrails/http/__init__.pynemoguardrails/http/client.pynemoguardrails/http/errors.pynemoguardrails/http/retry.pynemoguardrails/http/testing.pynemoguardrails/http/transport.pynemoguardrails/http/types.pytests/http/test_contract.pytests/http/test_retry.pytests/http/test_transport.py
e956bc5 to
5883eb9
Compare
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
5883eb9 to
76981a4
Compare
tgasser-nv
left a comment
There was a problem hiding this comment.
Looks good! Can you check the code coverage and add tests to improve this? Looks like missing lines in the owned-client request-path (where client is None) and a couple of other places.
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
|
Staged Fern docs preview: https://nvidia-preview-pr-2209.docs.buildwithfern.com/nemo/guardrails |
|
Thanks @tgasser-nv for the review. I've added coverage and addressed your review comments. |
Description
Define the transport-neutral outbound HTTP foundation used by built-in library
rails.
The neutral response owns decoded bytes rather than exposing a live transport
response. Callers can therefore inspect a response after the underlying client
context closes. The HTTPX adapter provides pooled connections while keeping
transport-specific types out of rail actions.
Retry behavior is bounded and conservative by default. Safe methods are
retryable, POST retries require an explicit rail-owned policy, and vendor
override headers are ignored unless a policy opts in.
Retry-Afterremainsbounded by policy, total request timeouts cover the complete operation, and
redirect following is disabled unless explicitly enabled.
Design
There is one real transport and a set of thin, single-purpose decorators, not a
family of client implementations.
HTTPClientis a runtime-checkable protocol,so
HttpxHTTPClient, a recording fake, and any futureaiohttpadapter satisfythe same contract without sharing a base class. Rail actions and tests depend on
the protocol and never import a concrete transport.
Behavior is added by composition. The default stack is
InstrumentedHTTPClient(RetryingHTTPClient(HttpxHTTPClient(...))), where eachlayer satisfies
HTTPClientand wraps the next:HttpxHTTPClientis the only layer that touches the network. It owns theconnection pool, the total-operation timeout, and TLS.
RetryingHTTPClientre-issues a request under a bounded policy and knowsnothing about HTTPX or telemetry.
InstrumentedHTTPClientopens one span per logical call and knows nothingabout HTTPX or retries. It is a transparent pass-through when no tracer is
supplied, so importing the module never requires the OpenTelemetry SDK.
This design keeps each concern independently testable and configurable: retry
logic is unit-tested against a fake transport with no HTTPX and no telemetry,
and a layer is only present when it is needed (retries only when a policy is
passed, instrumentation only when tracing is enabled). The composition order is
load-bearing. Instrumenting outside retry yields one span per logical operation
that records the resend count. The reverse order would emit one sibling span per
attempt and make a single operation look like several unrelated calls.
Impact
that resending is safe.
covered by focused tests.
in later stack layers.
Stack
pouyanpi/rail-library-stack-9-http-contractdeveloppouyanpi/rail-library-stack-10-http-instrumentationpouyanpi/rail-library-stack-11-http-observabilitypouyanpi/rail-library-stack-12-http-request-migrationspouyanpi/rail-library-stack-13-http-action-migrationsSummary by CodeRabbit
Retry-Aftersupport.