Guardrails: Implement response phase - #832
Conversation
605f420 to
a2d6e6d
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Implements response-side guardrail evaluation with block_in_place bridging and body padding/truncation to match committed Content-Length.
Overall: Implementation is mostly sound with good test coverage. Two medium-severity findings require fixes to prevent UTF-8 corruption and ensure fail-closed behavior.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 2 |
Findings without inline placement
None - all findings are inline on changed code.
| original_len, | ||
| "ai_guardrails: replacement body larger than committed Content-Length; truncating", | ||
| ); | ||
| Bytes::from(replacement).slice(0..original_len) |
There was a problem hiding this comment.
[Medium] UTF-8 boundary corruption risk in truncation path
When the error JSON replacement is larger than the original body, Bytes::from(replacement).slice(0..original_len) truncates at an arbitrary byte offset. If the reason field contains multi-byte Unicode characters (e.g., non-ASCII rail names), truncation can split a multi-byte UTF-8 sequence, producing invalid UTF-8 in the response body.
Fix by truncating at a UTF-8 character boundary, then padding:
std::cmp::Ordering::Greater => {
tracing::warn!(
new_len = replacement.len(),
original_len,
"ai_guardrails: replacement body larger than committed Content-Length; truncating",
);
// Truncate at a character boundary to avoid invalid UTF-8.
let safe = std::str::from_utf8(&replacement[..original_len])
.map(|s| s.len())
.unwrap_or_else(|e| e.valid_up_to());
let mut result = replacement;
result.truncate(safe);
result.resize(original_len, b' ');
Bytes::from(result)
},This also avoids the extra allocation from Bytes::from(replacement).slice(...).
| if let Some(choices) = json.get_mut("choices").and_then(|c| c.as_array_mut()) { | ||
| let messages: Vec<serde_json::Value> = choices | ||
| .iter_mut() | ||
| .filter_map(|c| c.get_mut("message").map(std::mem::take)) |
There was a problem hiding this comment.
[Medium] Silently skips choices without message field (fail-open behavior)
The filter_map silently discards any choice lacking a "message" field. If an upstream response has 3 choices but only 2 contain "message" keys, the filter validates only the 2 parseable messages and forwards the entire response (including the un-validated choice) to the client. In a fail-closed security filter, unparseable content should block the response.
Fix by validating all choices contain messages:
if let Some(choices) = json.get_mut("choices").and_then(|c| c.as_array_mut()) {
let num_choices = choices.len();
let messages: Vec<serde_json::Value> = choices
.iter_mut()
.filter_map(|c| c.get_mut("message").map(std::mem::take))
.collect();
if messages.is_empty() {
return Err("ai_guardrails: response body does not contain recognizable choices".into());
}
if messages.len() != num_choices {
return Err(
format!(
"ai_guardrails: {num_choices} choices but only {} contain a message field",
messages.len(),
)
.into(),
);
}
return Ok(messages);
}Add a test case with mixed choices (one with message, one without) asserting error.
a2d6e6d to
faa802d
Compare
f7406e5 to
5e39329
Compare
|
@liavweiss did you address the bot comments? |
Signed-off-by: Liav Weiss <lweiss@redhat.com>
d7ffba8 to
493e10b
Compare
|
@leseb, I addressed them now. |
praxis-bot
left a comment
There was a problem hiding this comment.
PR Re-review
Re-review: new commits since last review on 2026-08-28.
Summary: Both previous findings (UTF-8 truncation and fail-open mixed choices) are addressed. One new finding: streaming response incompatibility.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 1 |
| Medium | 0 |
| return Ok(FilterAction::Continue); | ||
| } | ||
|
|
||
| let messages = extract_response_messages(bytes)?; |
There was a problem hiding this comment.
[Large] Response-phase guardrails will break all streaming/SSE responses.
extract_response_messages expects a {"choices": [{"message": {...}}]} JSON body. Streaming responses (SSE with data: {...} events) are buffered by StreamBuffer into a raw byte blob that is not valid JSON — serde_json::from_slice will fail, producing a FilterError. Depending on failure_mode:
closed(default for security filters): every streaming response returns 500open: streaming responses pass through without guardrail evaluation
Neither outcome is correct. Add a streaming-response check before extract_response_messages so SSE bodies are either skipped with a clear log or handled differently:
if is_streaming_response(ctx) {
tracing::debug!("ai_guardrails: skipping response-phase evaluation for streaming response");
return Ok(FilterAction::Continue);
}You can detect streaming from the response Content-Type header (e.g. text/event-stream) or from Transfer-Encoding: chunked without Content-Length. Also add a test that sends an SSE-shaped body through on_response_body and verifies the filter does not error.
praxis-bot
left a comment
There was a problem hiding this comment.
PR Re-review
Re-review: new commits since last review on 2026-08-28.
Previous findings addressed:
- UTF-8 boundary truncation: fixed with
from_utf8/valid_up_to+ space padding. - Fail-open choices: fixed with
messages.len() != num_choicescheck.
Summary: Missing integration test and example config exercising the new response-phase guardrails path.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 1 |
| Medium | 0 |
| } | ||
| } | ||
|
|
||
| fn on_response_body( |
There was a problem hiding this comment.
[Large] Missing integration test for response-phase guardrails
This PR adds a significant new capability (response body evaluation and replacement) but only includes unit tests. Per the project's test requirements, new capabilities need:
- Integration tests
- Example config in
examples/configs/that exercises the feature - Functional integration test for the example config
The example config (nemo-guardrails.yaml) keeps response: false, so existing integration tests don't exercise the response path at all.
Add a dedicated example config (e.g., nemo-guardrails-response.yaml) with response: true and a corresponding integration test that verifies:
- Response-phase block verdict replaces the body with the error JSON and preserves
Content-Lengthframing - Response-phase pass verdict forwards the body unchanged
- Response-phase provider error fails closed
Summary
ai_guardrailsfilter, enabling NeMo to inspect upstream responses before they reach
the client.
on_response_bodyusingblock_in_placeto bridge the syncPingora constraint into the async
GuardProvider::evaluatecall(tracked in Investigate async
on_response_bodysupport #51 for a proper async solution).record_verdictto handle both request and response phases:request-phase blocks return a clean 403; response-phase blocks replace
the body with a JSON error payload padded to the committed
Content-Length(Pingora commits headers beforeon_response_bodyruns, so status code cannot be changed).
extract_response_messagesto parse OpenAI Chat Completionresponse format (
choices[].message), fail-closed on unrecognizedbodies.
fit_to_committed_lengthutility that pads (space) or truncatesthe replacement body to match the original
Content-Length, preventingPrematureBodyEndframing errors.phase.response: truerejection guard fromfrom_configand the
#[dead_code]suppression onGuardPhase::Response.validation (fail-closed), provider verdicts (pass/block/error via
wiremock), and
fit_to_committed_lengthedge cases (equal, padding,truncation,
Nonebody).nemo-guardrails.yamlexample to document both request andresponse phase behavior.
Validation
cargo test -p praxis-ai-filters --lib -- guardrails)make lint(no new warnings)Checklist
Signed-off-bytrailer.Breaking changes
None. The
phase.responseconfig field already existed with a default offalse. Existing configs that omit it or set it tofalseare unaffected.Fix
#50