refactor(library): migrate HTTP request helpers - #2211
Conversation
github-actions
Bot
added
size: L
status: needs triage
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
a0fdf05 to
d5852ff
Compare
3fa2fdb to
68b0e7e
Compare
Pouyanpi
removed
the
status: needs triage
Pouyanpi
added
the
status: triaged
Greptile SummaryThis PR migrates six integration libraries (ActiveFence, AlignScore, GLiNER, jailbreak detection, Polygraf, and Private AI) from
|
| Filename | Overview |
|---|---|
| nemoguardrails/library/activefence/actions.py | Replaces aiohttp session with http_call; http_client param forwarded directly. |
| nemoguardrails/library/factchecking/align_score/actions.py | Adds http_client param and forwards via conditional dict pattern; otherwise unchanged behavior. |
| nemoguardrails/library/factchecking/align_score/request.py | aiohttp replaced with http_call; non-200 handling and JSON decode error handling preserved correctly. |
| nemoguardrails/library/gliner/request.py | aiohttp replaced with http_call; HTTPResponseDecodeError replaces aiohttp.ContentTypeError; NIM unwrapping logic unchanged. |
| nemoguardrails/library/jailbreak_detection/request.py | All three request helpers migrated; exception order correct (Timeout before ClientError). |
| nemoguardrails/library/polygraf/request.py | _send_polygraf_request helper removed; timeout/connection error handling ported to HTTPTimeoutError/HTTPClientError. |
| tests/test_fact_checking.py | aioresponses replaced with httpx_mock rather than RecordingHTTPClient; tests remain transport-coupled. |
| tests/test_gliner.py | aioresponses replaced with RecordingHTTPClient throughout; all paths covered. |
| tests/test_polygraf.py | All session=None stubs updated to http_client=None; dead helper classes removed; new detect_pii forwarding test added. |
Reviews (6): Last reviewed commit: "refactor(polygraf): pass optional HTTP c..." | Re-trigger Greptile
68b0e7e to
9a89c02
Compare
9a89c02 to
cbc27d8
Compare
Pouyanpi
changed the title
Pouyanpi
changed the title
72d49cb to
3faf664
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>
3faf664 to
2de84ec
Compare
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (32)
📝 WalkthroughWalkthroughIntroduces a transport-neutral asynchronous HTTP client stack with HTTPX transport, retries, lifecycle management, response/error types, testing utilities, and shared-client injection across provider integrations. ChangesShared HTTP stack
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Action
participant ProviderRequest
participant http_call
participant RetryingHTTPClient
participant HttpxHTTPClient
Action->>ProviderRequest: pass optional HTTPClient
ProviderRequest->>http_call: submit outbound request
http_call->>RetryingHTTPClient: request(...)
RetryingHTTPClient->>HttpxHTTPClient: request(...)
HttpxHTTPClient-->>RetryingHTTPClient: HTTPResponse or transport error
RetryingHTTPClient-->>http_call: final response or error
http_call-->>ProviderRequest: response
ProviderRequest-->>Action: provider result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nemoguardrails/library/activefence/actions.py (1)
1-1: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
response.json()against malformed provider responses, matching theprivateai/request.pypattern.Both of these migrated call sites parse the HTTP response body with
.json()/response_json[...]without catchingHTTPResponseDecodeErroror a missing key, so a non-JSON or unexpectedly-shaped response from ActiveFence/AlignScore would raise an unhandled exception instead of the controlledValueError/None-fallback behavior these providers otherwise rely on.nemoguardrails/library/privateai/request.py(lines 70-86, this same migration) already establishes the right pattern: wrap.json()intry/except HTTPResponseDecodeErrorand re-raisefrom error.
nemoguardrails/library/activefence/actions.py#L120-141: wrapresponse.json()intry/except HTTPResponseDecodeError, re-raising asValueError(...) from error, and use.get("violations", [])instead of["violations"]for the key lookup.nemoguardrails/library/factchecking/align_score/request.py#L38-56: wraphttp_response.json()intry/except HTTPResponseDecodeError, logging/returningNoneon failure (consistent with the existing "return None on failure" contract ofalignscore_request).🛡️ Proposed fix for activefence/actions.py
+from nemoguardrails.http import HTTPClient, HTTPResponseDecodeError, http_call if response.status_code != 200: raise ValueError(f"ActiveFence call failed with status code {response.status_code}.\nDetails: {response.text}") - response_json = response.json() + try: + response_json = response.json() + except HTTPResponseDecodeError as error: + raise ValueError( + f"Failed to parse ActiveFence response as JSON. Status: {response.status_code}, Content: {response.text}" + ) from error log.info(json.dumps(response_json, indent=True)) - violations = response_json["violations"] + violations = response_json.get("violations", [])🛡️ Proposed fix for align_score/request.py
if http_response.status_code != 200: log.error(f"AlignScore API request failed with status {http_response.status_code}") return None - result = http_response.json() + try: + result = http_response.json() + except HTTPResponseDecodeError: + log.error(f"AlignScore API returned a non-JSON response (status {http_response.status_code})") + return None🤖 Prompt for 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. In `@nemoguardrails/library/activefence/actions.py` at line 1, Guard JSON parsing in the ActiveFence response handling and AlignScore’s alignscore_request: catch HTTPResponseDecodeError from response.json()/http_response.json(). In the ActiveFence path, re-raise ValueError with the original error as its cause and read violations via .get("violations", []). In alignscore_request, log the decode failure and return None, preserving its existing failure contract.
🧹 Nitpick comments (3)
nemoguardrails/library/polygraf/request.py (1)
49-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale docstring wording: "sessions".
The aiohttp session concept is gone; reword to reference the caller-provided or fallback HTTP client.
🤖 Prompt for 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. In `@nemoguardrails/library/polygraf/request.py` around lines 49 - 50, Update the timeout parameter docstring to replace the stale “sessions” reference with the caller-provided or fallback HTTP client, while preserving the description that the timeout applies to both cases.nemoguardrails/library/gliner/actions.py (1)
133-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConditional
request_kwargsis unnecessary indirection.
gliner_requestalready defaultshttp_client=None, so passing it unconditionally is equivalent and clearer (same for both actions).♻️ Proposed simplification
- request_kwargs = {"http_client": http_client} if http_client is not None else {} gliner_response = await gliner_request( text=text, ... model=gliner_config.model, - **request_kwargs, + http_client=http_client, )Also applies to: 189-200
🤖 Prompt for 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. In `@nemoguardrails/library/gliner/actions.py` around lines 133 - 144, Remove the conditional request_kwargs indirection in both GLiNER action request paths and pass http_client directly to gliner_request, relying on its existing default of None. Update the corresponding calls near the second action as well, without changing any other request arguments or behavior.tests/test_gliner.py (1)
741-743: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
_http_responsetest helper with divergent kwargs. Both suites hand-roll the sameHTTPResponsebuilder, one usingbodyand the othertext, which will keep drifting as more providers migrate.
tests/test_gliner.py#L741-L743: replace with a single shared helper (e.g., exported next toRecordingHTTPClient).tests/test_polygraf.py#L199-L201: import the shared helper instead of redefining it with atextkwarg.🤖 Prompt for 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. In `@tests/test_gliner.py` around lines 741 - 743, The test suites duplicate the HTTPResponse builder with incompatible keyword arguments. In tests/test_gliner.py lines 741-743, move or expose one shared _http_response helper next to RecordingHTTPClient; in tests/test_polygraf.py lines 199-201, remove the local helper and import the shared one, updating callers to use its body-compatible interface.
🤖 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/retry.py`:
- Around line 168-204: Update the retry loop around HTTPClient.request to
enforce timeout across the entire operation: establish a monotonic deadline,
pass the remaining duration to each request attempt, and cap both backoff and
retry-after sleeps to the remaining time. When the deadline expires, raise
HTTPTimeoutError rather than starting another attempt or sleeping beyond the
timeout, while preserving existing retry-count and response behavior.
- Around line 110-114: Update the non-clamped delay check in the retry-after
handling logic to accept a zero delay as valid, returning it directly instead of
falling back to jittered backoff; preserve the existing maximum-bound validation
and None behavior for invalid delays.
In `@nemoguardrails/http/transport.py`:
- Around line 55-64: Update the HTTP request flow in the transport class to use
httpx’s per-request timeout instead of wrapping httpx.request() with
asyncio.wait_for. Preserve timeout=None for externally supplied clients, while
applying the configured timeout to requests made by internally owned clients;
remove the repeated deadline-cancellation path.
In `@nemoguardrails/library/factchecking/align_score/request.py`:
- Line 54: Replace the broad exception handler in the request flow with only the
specific exception types expected from the dict-key lookup, matching the
established handling in privateai/request.py. Preserve the existing fallback
behavior while allowing unrelated errors such as caller-induced AttributeError
to propagate.
In `@nemoguardrails/library/jailbreak_detection/request.py`:
- Around line 70-75: Normalize timeout and transport/JSON error handling across
the migrated helpers: in nemoguardrails/library/jailbreak_detection/request.py
lines 70-75, update the jailbreak API request and response.json flow to pass an
explicit timeout and return None for HTTPTimeoutError/HTTPClientError and
invalid JSON, matching jailbreak_nim_request; in
nemoguardrails/library/gliner/request.py lines 110-119, add a timeout parameter
as used by polygraf_request, pass it to the HTTP call, and normalize transport
errors to ValueError per the docstring; in
nemoguardrails/library/jailbreak_detection/request.py lines 95-100, apply the
same explicit timeout and error normalization as the heuristics helper.
- Line 153: Update the error log in the jailbreak detection request handling to
avoid explicit str(e) conversion and use the conversion flag supported by the
logging format, preserving the existing error message and exception details.
---
Outside diff comments:
In `@nemoguardrails/library/activefence/actions.py`:
- Line 1: Guard JSON parsing in the ActiveFence response handling and
AlignScore’s alignscore_request: catch HTTPResponseDecodeError from
response.json()/http_response.json(). In the ActiveFence path, re-raise
ValueError with the original error as its cause and read violations via
.get("violations", []). In alignscore_request, log the decode failure and return
None, preserving its existing failure contract.
---
Nitpick comments:
In `@nemoguardrails/library/gliner/actions.py`:
- Around line 133-144: Remove the conditional request_kwargs indirection in both
GLiNER action request paths and pass http_client directly to gliner_request,
relying on its existing default of None. Update the corresponding calls near the
second action as well, without changing any other request arguments or behavior.
In `@nemoguardrails/library/polygraf/request.py`:
- Around line 49-50: Update the timeout parameter docstring to replace the stale
“sessions” reference with the caller-provided or fallback HTTP client, while
preserving the description that the timeout applies to both cases.
In `@tests/test_gliner.py`:
- Around line 741-743: The test suites duplicate the HTTPResponse builder with
incompatible keyword arguments. In tests/test_gliner.py lines 741-743, move or
expose one shared _http_response helper next to RecordingHTTPClient; in
tests/test_polygraf.py lines 199-201, remove the local helper and import the
shared one, updating callers to use its body-compatible interface.
🪄 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: 6adc9544-61d3-4340-997e-d42fa2967e00
📒 Files selected for processing (32)
nemoguardrails/http/__init__.pynemoguardrails/http/_url.pynemoguardrails/http/client.pynemoguardrails/http/errors.pynemoguardrails/http/request.pynemoguardrails/http/retry.pynemoguardrails/http/runtime.pynemoguardrails/http/testing.pynemoguardrails/http/transport.pynemoguardrails/http/types.pynemoguardrails/library/activefence/actions.pynemoguardrails/library/factchecking/align_score/actions.pynemoguardrails/library/factchecking/align_score/request.pynemoguardrails/library/gliner/actions.pynemoguardrails/library/gliner/request.pynemoguardrails/library/jailbreak_detection/actions.pynemoguardrails/library/jailbreak_detection/request.pynemoguardrails/library/polygraf/actions.pynemoguardrails/library/polygraf/request.pynemoguardrails/library/privateai/actions.pynemoguardrails/library/privateai/request.pytests/http/test_contract.pytests/http/test_request.pytests/http/test_retry.pytests/http/test_runtime.pytests/http/test_transport.pytests/http/test_url.pytests/test_activefence_rail.pytests/test_fact_checking.pytests/test_gliner.pytests/test_jailbreak_request.pytests/test_polygraf.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nemoguardrails/library/activefence/actions.py (1)
1-1: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
response.json()against malformed provider responses, matching theprivateai/request.pypattern.Both of these migrated call sites parse the HTTP response body with
.json()/response_json[...]without catchingHTTPResponseDecodeErroror a missing key, so a non-JSON or unexpectedly-shaped response from ActiveFence/AlignScore would raise an unhandled exception instead of the controlledValueError/None-fallback behavior these providers otherwise rely on.nemoguardrails/library/privateai/request.py(lines 70-86, this same migration) already establishes the right pattern: wrap.json()intry/except HTTPResponseDecodeErrorand re-raisefrom error.
nemoguardrails/library/activefence/actions.py#L120-141: wrapresponse.json()intry/except HTTPResponseDecodeError, re-raising asValueError(...) from error, and use.get("violations", [])instead of["violations"]for the key lookup.nemoguardrails/library/factchecking/align_score/request.py#L38-56: wraphttp_response.json()intry/except HTTPResponseDecodeError, logging/returningNoneon failure (consistent with the existing "return None on failure" contract ofalignscore_request).🛡️ Proposed fix for activefence/actions.py
+from nemoguardrails.http import HTTPClient, HTTPResponseDecodeError, http_call if response.status_code != 200: raise ValueError(f"ActiveFence call failed with status code {response.status_code}.\nDetails: {response.text}") - response_json = response.json() + try: + response_json = response.json() + except HTTPResponseDecodeError as error: + raise ValueError( + f"Failed to parse ActiveFence response as JSON. Status: {response.status_code}, Content: {response.text}" + ) from error log.info(json.dumps(response_json, indent=True)) - violations = response_json["violations"] + violations = response_json.get("violations", [])🛡️ Proposed fix for align_score/request.py
if http_response.status_code != 200: log.error(f"AlignScore API request failed with status {http_response.status_code}") return None - result = http_response.json() + try: + result = http_response.json() + except HTTPResponseDecodeError: + log.error(f"AlignScore API returned a non-JSON response (status {http_response.status_code})") + return None🤖 Prompt for 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. In `@nemoguardrails/library/activefence/actions.py` at line 1, Guard JSON parsing in the ActiveFence response handling and AlignScore’s alignscore_request: catch HTTPResponseDecodeError from response.json()/http_response.json(). In the ActiveFence path, re-raise ValueError with the original error as its cause and read violations via .get("violations", []). In alignscore_request, log the decode failure and return None, preserving its existing failure contract.
🧹 Nitpick comments (3)
nemoguardrails/library/polygraf/request.py (1)
49-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale docstring wording: "sessions".
The aiohttp session concept is gone; reword to reference the caller-provided or fallback HTTP client.
🤖 Prompt for 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. In `@nemoguardrails/library/polygraf/request.py` around lines 49 - 50, Update the timeout parameter docstring to replace the stale “sessions” reference with the caller-provided or fallback HTTP client, while preserving the description that the timeout applies to both cases.nemoguardrails/library/gliner/actions.py (1)
133-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConditional
request_kwargsis unnecessary indirection.
gliner_requestalready defaultshttp_client=None, so passing it unconditionally is equivalent and clearer (same for both actions).♻️ Proposed simplification
- request_kwargs = {"http_client": http_client} if http_client is not None else {} gliner_response = await gliner_request( text=text, ... model=gliner_config.model, - **request_kwargs, + http_client=http_client, )Also applies to: 189-200
🤖 Prompt for 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. In `@nemoguardrails/library/gliner/actions.py` around lines 133 - 144, Remove the conditional request_kwargs indirection in both GLiNER action request paths and pass http_client directly to gliner_request, relying on its existing default of None. Update the corresponding calls near the second action as well, without changing any other request arguments or behavior.tests/test_gliner.py (1)
741-743: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
_http_responsetest helper with divergent kwargs. Both suites hand-roll the sameHTTPResponsebuilder, one usingbodyand the othertext, which will keep drifting as more providers migrate.
tests/test_gliner.py#L741-L743: replace with a single shared helper (e.g., exported next toRecordingHTTPClient).tests/test_polygraf.py#L199-L201: import the shared helper instead of redefining it with atextkwarg.🤖 Prompt for 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. In `@tests/test_gliner.py` around lines 741 - 743, The test suites duplicate the HTTPResponse builder with incompatible keyword arguments. In tests/test_gliner.py lines 741-743, move or expose one shared _http_response helper next to RecordingHTTPClient; in tests/test_polygraf.py lines 199-201, remove the local helper and import the shared one, updating callers to use its body-compatible interface.
🤖 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/retry.py`:
- Around line 168-204: Update the retry loop around HTTPClient.request to
enforce timeout across the entire operation: establish a monotonic deadline,
pass the remaining duration to each request attempt, and cap both backoff and
retry-after sleeps to the remaining time. When the deadline expires, raise
HTTPTimeoutError rather than starting another attempt or sleeping beyond the
timeout, while preserving existing retry-count and response behavior.
- Around line 110-114: Update the non-clamped delay check in the retry-after
handling logic to accept a zero delay as valid, returning it directly instead of
falling back to jittered backoff; preserve the existing maximum-bound validation
and None behavior for invalid delays.
In `@nemoguardrails/http/transport.py`:
- Around line 55-64: Update the HTTP request flow in the transport class to use
httpx’s per-request timeout instead of wrapping httpx.request() with
asyncio.wait_for. Preserve timeout=None for externally supplied clients, while
applying the configured timeout to requests made by internally owned clients;
remove the repeated deadline-cancellation path.
In `@nemoguardrails/library/factchecking/align_score/request.py`:
- Line 54: Replace the broad exception handler in the request flow with only the
specific exception types expected from the dict-key lookup, matching the
established handling in privateai/request.py. Preserve the existing fallback
behavior while allowing unrelated errors such as caller-induced AttributeError
to propagate.
In `@nemoguardrails/library/jailbreak_detection/request.py`:
- Around line 70-75: Normalize timeout and transport/JSON error handling across
the migrated helpers: in nemoguardrails/library/jailbreak_detection/request.py
lines 70-75, update the jailbreak API request and response.json flow to pass an
explicit timeout and return None for HTTPTimeoutError/HTTPClientError and
invalid JSON, matching jailbreak_nim_request; in
nemoguardrails/library/gliner/request.py lines 110-119, add a timeout parameter
as used by polygraf_request, pass it to the HTTP call, and normalize transport
errors to ValueError per the docstring; in
nemoguardrails/library/jailbreak_detection/request.py lines 95-100, apply the
same explicit timeout and error normalization as the heuristics helper.
- Line 153: Update the error log in the jailbreak detection request handling to
avoid explicit str(e) conversion and use the conversion flag supported by the
logging format, preserving the existing error message and exception details.
---
Outside diff comments:
In `@nemoguardrails/library/activefence/actions.py`:
- Line 1: Guard JSON parsing in the ActiveFence response handling and
AlignScore’s alignscore_request: catch HTTPResponseDecodeError from
response.json()/http_response.json(). In the ActiveFence path, re-raise
ValueError with the original error as its cause and read violations via
.get("violations", []). In alignscore_request, log the decode failure and return
None, preserving its existing failure contract.
---
Nitpick comments:
In `@nemoguardrails/library/gliner/actions.py`:
- Around line 133-144: Remove the conditional request_kwargs indirection in both
GLiNER action request paths and pass http_client directly to gliner_request,
relying on its existing default of None. Update the corresponding calls near the
second action as well, without changing any other request arguments or behavior.
In `@nemoguardrails/library/polygraf/request.py`:
- Around line 49-50: Update the timeout parameter docstring to replace the stale
“sessions” reference with the caller-provided or fallback HTTP client, while
preserving the description that the timeout applies to both cases.
In `@tests/test_gliner.py`:
- Around line 741-743: The test suites duplicate the HTTPResponse builder with
incompatible keyword arguments. In tests/test_gliner.py lines 741-743, move or
expose one shared _http_response helper next to RecordingHTTPClient; in
tests/test_polygraf.py lines 199-201, remove the local helper and import the
shared one, updating callers to use its body-compatible interface.
🪄 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: 6adc9544-61d3-4340-997e-d42fa2967e00
📒 Files selected for processing (32)
nemoguardrails/http/__init__.pynemoguardrails/http/_url.pynemoguardrails/http/client.pynemoguardrails/http/errors.pynemoguardrails/http/request.pynemoguardrails/http/retry.pynemoguardrails/http/runtime.pynemoguardrails/http/testing.pynemoguardrails/http/transport.pynemoguardrails/http/types.pynemoguardrails/library/activefence/actions.pynemoguardrails/library/factchecking/align_score/actions.pynemoguardrails/library/factchecking/align_score/request.pynemoguardrails/library/gliner/actions.pynemoguardrails/library/gliner/request.pynemoguardrails/library/jailbreak_detection/actions.pynemoguardrails/library/jailbreak_detection/request.pynemoguardrails/library/polygraf/actions.pynemoguardrails/library/polygraf/request.pynemoguardrails/library/privateai/actions.pynemoguardrails/library/privateai/request.pytests/http/test_contract.pytests/http/test_request.pytests/http/test_retry.pytests/http/test_runtime.pytests/http/test_transport.pytests/http/test_url.pytests/test_activefence_rail.pytests/test_fact_checking.pytests/test_gliner.pytests/test_jailbreak_request.pytests/test_polygraf.py
🛑 Comments failed to post (3)
nemoguardrails/http/retry.py (2)
110-114: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor
Retry-After: 0.Zero is a valid delta-seconds value. Line 112 rejects it and falls back to jittered backoff, delaying a server-requested immediate retry.
Proposed fix
- if 0 < delay <= self.max_retry_after: + if 0 <= delay <= self.max_retry_after: return delay📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if self.clamp_retry_after: return min(max(delay, 0.0), self.max_retry_after) if 0 <= delay <= self.max_retry_after: return delay return None🤖 Prompt for 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. In `@nemoguardrails/http/retry.py` around lines 110 - 114, Update the non-clamped delay check in the retry-after handling logic to accept a zero delay as valid, returning it directly instead of falling back to jittered backoff; preserve the existing maximum-bound validation and None behavior for invalid delays.
168-204: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Enforce
timeoutacross the complete retry operation.
HTTPClient.requestdefines this as a total timeout, but each attempt receives the full value at Line 172 and Lines 189/203 add uncapped delays. A 30-second request can therefore run for multiple attempts plus backoff. Track a monotonic deadline, pass only remaining time to each attempt, cap retry sleeps to it, then raiseHTTPTimeoutErrorwhen it expires.🤖 Prompt for 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. In `@nemoguardrails/http/retry.py` around lines 168 - 204, Update the retry loop around HTTPClient.request to enforce timeout across the entire operation: establish a monotonic deadline, pass the remaining duration to each request attempt, and cap both backoff and retry-after sleeps to the remaining time. When the deadline expires, raise HTTPTimeoutError rather than starting another attempt or sleeping beyond the timeout, while preserving existing retry-count and response behavior.nemoguardrails/http/transport.py (1)
55-64: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
encode httpx AsyncClient request cancellation connection pool cleanup fixed version💡 Result:
The issue of connection pool leakage in httpx/httpcore during task cancellation has been a persistent and complex technical challenge [1][2][3][4]. When an async request is cancelled (e.g., via asyncio.Task.cancel), it can occasionally leave a connection in an orphaned state—reserved but not fully utilized or correctly reaped—which eventually exhausts the connection pool and leads to persistent PoolTimeout errors [5][6][7]. There is no single "fixed version" that definitively resolves all potential race conditions in all scenarios, as the underlying networking component, httpcore, has faced ongoing architectural challenges regarding async cancellation safety [2][3][4]. While several pull requests have been merged over time to improve cleanup and reclaim orphaned connections [6][1][7][4], users continue to report issues, and development has been fragmented [7][4]. For current projects experiencing these issues: 1. Verify if you are using the latest versions of httpx and httpcore, as many incremental fixes have been released [3]. 2. Be aware that the root cause often relates to how connections are assigned and managed within the httpcore pool [6][2][4]. If your application relies heavily on cancelling requests, you may still encounter edge cases [2]. 3. Some community members have noted that alternative libraries or forks (e.g., pydantic's httpx2) have implemented more aggressive fixes for this specific problem [7]. In production environments where stability is critical, if updates to the latest official httpx/httpcore do not resolve the leakage, consider architectural workarounds, such as increasing connection pool limits or utilizing a client-side wrapper that explicitly ensures connection cleanup upon task cancellation.
Citations:
- 1: encode/httpcore#658
- 2: encode/httpcore#830
- 3: encode/httpx#2821
- 4: encode/httpcore#880
- 5: https://github.com/encode/httpx/issues/1461
- 6: encode/httpcore#1099
- 7: encode/httpcore#1094
Avoid cancelling
httpx.request()withasyncio.wait_fortimeout=Nonedisables httpx’s native timeout handling, so repeated deadline cancellations can leave pooled connections in a bad state under retry pressure. Prefer httpx’s per-requesttimeout=here instead.🧰 Tools
🪛 Ruff (0.16.0)
[error] 60-60: Probable use of
httpxcall with timeout set toNone(S113)
🤖 Prompt for 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. In `@nemoguardrails/http/transport.py` around lines 55 - 64, Update the HTTP request flow in the transport class to use httpx’s per-request timeout instead of wrapping httpx.request() with asyncio.wait_for. Preserve timeout=None for externally supplied clients, while applying the configured timeout to requests made by internally owned clients; remove the repeated deadline-cancellation path.
There was a problem hiding this comment.
Looks good! Just a few general comments / nits. Could you add more unit-tests to cover the action lines from CodeCov? There are whole failure branches missing in the jailbreak for example
The timeouts will change after this PR. The default aiohttp.ClientSession() used by ActiveFence, AlignScore, GLiNER, jailbreak, and Private AI defaulted to 5 minutes. After this PR the default timeout is now 30s. I think 30s is much more reasonable for a blocking Guardrail action, but if the server responds in 35s this will now throw an exception rather than passing as before
| text, | ||
| server_endpoint, | ||
| api_key, | ||
| http_client=http_client, |
There was a problem hiding this comment.
nit: For Polygraf we pass the http_client directly through the named argument. For the other integrations we use:
request_kwargs = {"http_client": http_client} if http_client is not None else {}
And then pass through the kwargs. Would be cleaner to use the Polygraf approach for all the actions.py files.
2 participants
Description
Migrate shared library request helpers to the canonical HTTP client and request
helper. The affected integrations are ActiveFence, AlignScore, GLiNER,
jailbreak detection, Polygraf, and Private AI.
Each helper accepts an injected
HTTPClientand sends requests throughhttp_call. When no client is injected,http_callcreates and closes thefallback client itself. The action modules no longer repeat an
async withresolver around every outbound request.Existing response parsing, request payloads, timeouts, configuration errors, and
provider-specific exception behavior remain local to each integration. Tests
replace transport-specific mocks with the neutral recording client where
appropriate.
Impact
clients directly.
integration tests.
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-migrationsRelated Issue(s)
Verification
make test TEST="tests/http tests/test_activefence_rail.py tests/test_fact_checking.py tests/test_gliner.py tests/test_jailbreak_request.py tests/test_polygraf.py"— 144 passed.AI Assistance
Codex assisted with implementation analysis, validation, stack restructuring,
and this draft. Check the disclosure box after human review.
Checklist
Summary by CodeRabbit