fix(guardrails): deterministic chain order, name the blocking rule, drop dead code - #584
Merged
Conversation
…rop dead code - Sort guardrail rows (created_at asc, id tiebreak) before building the chain / the implicit env-scope index entries, and sort attachment rows by id, so which Block fires first is deterministic and matches the dashboard's oldest-first listing (#519 B.4a). Adds the optional created_at field to the DP Guardrail model; cp-api will project it in a follow-up, until then the id tiebreak keeps order total. - GuardrailChain attributes Block verdicts to the firing member: the verdict carries guardrail_name and the ops-log reason is prefixed with it. Every proxy endpoint family now builds its 422 / SSE-error message via error::guardrail_block_message - generic policy wording plus the guardrail name, never the matched-pattern detail (#153 redaction preserved) (#519 B.4b). - Remove dead code: MaxContentLength (unreachable from config; no GuardrailKind::Length) and GuardrailVerdict::Rewrite (no impl ever returned it), collapsing the chain's Cow propagation machinery (#519 B.5 + E.2).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR refactors guardrail infrastructure to support deterministic chain member ordering via ChangesGuardrail Attribution and Deterministic Ordering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aisix-proxy/src/chat.rs (1)
2633-2656:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBufferFull fail-closed still skips guardrail attribution.
When
max_buffer_bytesis exceeded withon_exceeded_fail_open = false, this branch emits the old hardcoded"response blocked by content policy"SSE payload. That makes BufferFull the one streaming block path that bypassesguardrail_block_message()and drops the guardrail identifier the rest of this PR now surfaces. Please route this branch through the shared helper and plumb the owning guardrail name into it if the stream policy can identify one.🤖 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 `@crates/aisix-proxy/src/chat.rs` around lines 2633 - 2656, The BufferFull branch of aisix_guardrails::StreamOutputPolicy currently emits a hardcoded SSE error payload and skips guardrail attribution; instead call the shared helper guardrail_block_message(...) (or the existing helper used elsewhere for blocked responses) from the BufferFull branch when on_exceeded_fail_open is false, pass through the same context flags (set errored and guard.comp().guardrail_blocked as now), and plumb the owning guardrail name/identifier into that helper (derive it from the current guard/guardrail context available in this scope) so the SSE payload includes the proper guardrail attribution rather than the hardcoded "response blocked by content policy". Ensure the pending drain/yield behavior remains unchanged for the fail-open path.
🧹 Nitpick comments (2)
crates/aisix-proxy/src/completions.rs (1)
467-502: ⚡ Quick winAssert the new guardrail-name contract here.
This test still passes if the handler regresses to the old generic
"request blocked by content policy"string, because it only checks thatBLOCKMEis redacted. Add a positive assertion forguardrail 't'(or the exact helper output) so/v1/completionskeeps the attribution behavior this PR is introducing.🤖 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 `@crates/aisix-proxy/src/completions.rs` around lines 467 - 502, The test input_guardrail_blocks_prompt_returns_422 must also assert the new guardrail-name contract: after decoding v from the response, add a positive assertion that the error message includes the guardrail identifier produced by keyword_input_guardrail (the guardrail name 't' used by that helper or the exact helper output), e.g. assert that v["error"]["message"].as_str().unwrap().contains("t") (or the exact string returned by keyword_input_guardrail), while keeping the existing check that the blocked keyword "BLOCKME" is redacted.crates/aisix-proxy/src/error.rs (1)
206-220: ⚡ Quick winPin the shared block-message shape with a unit test.
guardrail_block_message()is now the single source for every 422/SSE guardrail error, but this module never asserts either the named or unnamed form. A tinySome("foo")/Nonetest here would catch wording drift before it fans out across all endpoint families.🤖 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 `@crates/aisix-proxy/src/error.rs` around lines 206 - 220, Add a unit test that pins the exact output of guardrail_block_message for both Some and None cases to prevent wording drift: write a test function (e.g., test_guardrail_block_message_named_and_unnamed) that calls guardrail_block_message("request" or "response", Some("foo")) and guardrail_block_message(..., None) and asserts the returned Strings equal the expected literals ("request blocked by content policy (guardrail 'foo')" and "request blocked by content policy" or the analogous "response" variants); place this test in the same module (inside #[cfg(test)] mod tests) so it runs with cargo test and fails on any accidental wording change.
🤖 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 `@crates/aisix-core/src/models/schema.rs`:
- Line 418: The schema currently allows any string for the "created_at" field,
which breaks lexicographic timestamp ordering; change the JSON schema entry for
"created_at" in schema.rs to enforce canonical RFC3339 UTC (UTC Zulu,
zero-padded) by adding a "format": "date-time" and/or a strict regex pattern
(e.g. requiring trailing "Z" and ISO8601 fields) so values are lexicographically
sortable, and ensure any related deserialization/validation code (e.g., code
paths that parse or validate "created_at") uses the same RFC3339 UTC check so
invalid values are rejected at parse/validation time.
In `@docs/configuration/guardrails.md`:
- Around line 262-263: Update the explanatory text near the example to reflect
that the error now includes the guardrail name (e.g., guardrail 'block-secrets')
inside error.message rather than a generic content_filter message; change any
phrases that say the rule name is omitted to state that error.message includes
guardrail '<name>' and show how operators should parse/read error.error.type and
error.message to get the guardrail attribution (referencing error.message and
the guardrail 'block-secrets' example).
In `@tests/e2e/src/cases/guardrail-output-e2e.test.ts`:
- Around line 203-205: Replace the broad check against errorBlob with a direct
assertion on the thrown error's message: locate the test where errorBlob is
asserted (the caught error is available as caught / error) and change the
expectation to assert that caught.error.message (or error.message) contains
"guardrail 'gr-out-e2e-keyword'"; this ensures the guardrail attribution lives
on the error.message itself rather than some other field.
---
Outside diff comments:
In `@crates/aisix-proxy/src/chat.rs`:
- Around line 2633-2656: The BufferFull branch of
aisix_guardrails::StreamOutputPolicy currently emits a hardcoded SSE error
payload and skips guardrail attribution; instead call the shared helper
guardrail_block_message(...) (or the existing helper used elsewhere for blocked
responses) from the BufferFull branch when on_exceeded_fail_open is false, pass
through the same context flags (set errored and guard.comp().guardrail_blocked
as now), and plumb the owning guardrail name/identifier into that helper (derive
it from the current guard/guardrail context available in this scope) so the SSE
payload includes the proper guardrail attribution rather than the hardcoded
"response blocked by content policy". Ensure the pending drain/yield behavior
remains unchanged for the fail-open path.
---
Nitpick comments:
In `@crates/aisix-proxy/src/completions.rs`:
- Around line 467-502: The test input_guardrail_blocks_prompt_returns_422 must
also assert the new guardrail-name contract: after decoding v from the response,
add a positive assertion that the error message includes the guardrail
identifier produced by keyword_input_guardrail (the guardrail name 't' used by
that helper or the exact helper output), e.g. assert that
v["error"]["message"].as_str().unwrap().contains("t") (or the exact string
returned by keyword_input_guardrail), while keeping the existing check that the
blocked keyword "BLOCKME" is redacted.
In `@crates/aisix-proxy/src/error.rs`:
- Around line 206-220: Add a unit test that pins the exact output of
guardrail_block_message for both Some and None cases to prevent wording drift:
write a test function (e.g., test_guardrail_block_message_named_and_unnamed)
that calls guardrail_block_message("request" or "response", Some("foo")) and
guardrail_block_message(..., None) and asserts the returned Strings equal the
expected literals ("request blocked by content policy (guardrail 'foo')" and
"request blocked by content policy" or the analogous "response" variants); place
this test in the same module (inside #[cfg(test)] mod tests) so it runs with
cargo test and fails on any accidental wording change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 897a5181-78c6-4458-b792-5fd9cd3d6368
📒 Files selected for processing (28)
crates/aisix-core/src/models/guardrail.rscrates/aisix-core/src/models/schema.rscrates/aisix-guardrails/src/aliyun.rscrates/aisix-guardrails/src/bedrock.rscrates/aisix-guardrails/src/build.rscrates/aisix-guardrails/src/chain.rscrates/aisix-guardrails/src/index.rscrates/aisix-guardrails/src/keyword.rscrates/aisix-guardrails/src/length.rscrates/aisix-guardrails/src/lib.rscrates/aisix-guardrails/src/prompt_shield.rscrates/aisix-guardrails/src/text_moderation.rscrates/aisix-proxy/src/audio.rscrates/aisix-proxy/src/chat.rscrates/aisix-proxy/src/completions.rscrates/aisix-proxy/src/embeddings.rscrates/aisix-proxy/src/error.rscrates/aisix-proxy/src/images.rscrates/aisix-proxy/src/lib.rscrates/aisix-proxy/src/messages.rscrates/aisix-proxy/src/rerank.rscrates/aisix-proxy/src/responses.rsdocs/configuration/guardrails.mddocs/tutorials/add-keyword-guardrails.mddocs/tutorials/block-content-with-aws-bedrock-guardrails.mddocs/tutorials/block-prompt-injection-with-azure-content-safety.mdtests/e2e/src/cases/guardrail-keyword-e2e.test.tstests/e2e/src/cases/guardrail-output-e2e.test.ts
💤 Files with no reviewable changes (1)
- crates/aisix-guardrails/src/length.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
schemas/resources/guardrail.schema.json (1)
322-328: 💤 Low valueConsider adding
"format": "date-time"validation.The schema accepts any string value for
created_at, even though the description specifies RFC3339. Adding"format": "date-time"provides schema-level validation as a defense-in-depth measure, catching malformed timestamps before they reach the Rust deserializer.📋 Suggested format constraint
"created_at": { "description": "RFC3339 creation timestamp of the row, projected by cp-api so the DP can evaluate guardrail chains oldest-first — matching the order the dashboard lists them in (`#519` B.4a). Optional: cp-api's `marshalGuardrailKV` doesn't emit it yet, and admin-API rows may omit it; rows without it sort after rows that have it, tied broken by id, so chain order stays total and deterministic either way. RFC3339 timestamps in a fixed (UTC) offset compare correctly as strings, so no parsing is needed.", + "format": "date-time", "type": [ "string", "null" ] },🤖 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 `@schemas/resources/guardrail.schema.json` around lines 322 - 328, The created_at property currently allows any string or null but lacks RFC3339 validation; update the guardrail.schema.json entry for "created_at" to include a "format": "date-time" alongside the existing "type": ["string","null"] so the schema enforces date-time formatting for non-null values (keep the nullable behavior intact), and run/update any schema tests that validate timestamp formatting to reflect this constraint.
🤖 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.
Nitpick comments:
In `@schemas/resources/guardrail.schema.json`:
- Around line 322-328: The created_at property currently allows any string or
null but lacks RFC3339 validation; update the guardrail.schema.json entry for
"created_at" to include a "format": "date-time" alongside the existing "type":
["string","null"] so the schema enforces date-time formatting for non-null
values (keep the nullable behavior intact), and run/update any schema tests that
validate timestamp formatting to reflect this constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c84de3ee-1971-47fb-ac7d-7a49e58f7358
📒 Files selected for processing (1)
schemas/resources/guardrail.schema.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses B.4, B.5, E.2 of api7/AISIX-Cloud#519.
B.4a — deterministic guardrail chain order
build_chain_from_snapshot(and the implicit env-scope fallback inbuild_index_from_snapshot) iteratedResourceTable::entries(), which is DashMap-backed — iteration order is arbitrary and varies run to run. With multiple matching guardrails, whichBlockfired first was random.Rows are now sorted
(created_at asc, id asc)before the chain/index is built (rows withoutcreated_atsort after rows that have it), so evaluation order is total, deterministic, and matches the dashboard's oldest-first listing. Attachment rows are sorted by id so equal-priority/equal-specificity ties inGuardrailIndexresolve stably too.The DP
Guardrailmodel gains an optionalcreated_at(RFC3339 string,#[serde(default)]). The CP will start projecting it inmarshalGuardrailKVin a follow-up PR; until then every row falls back to the id tiebreak, which is still deterministic.B.4b — name the guardrail that blocked
A blocked request previously returned
422 {"error":{"type":"content_filter","message":"request blocked by content policy"}}with no indication of WHICH guardrail fired.GuardrailChainnow attributes eachBlockto the firing member: the verdict carriesguardrail_nameand the ops-logreasonis prefixed withguardrail '<name>':. All proxy endpoint families (chat, completions, messages incl. streaming/passthrough, responses, embeddings, rerank, images, audio, cache-hit path) build the public message through one shared helper:error.typestayscontent_filter, and the #153 redaction contract is preserved — the matched-pattern detail still never reaches the wire, only the operator-assigned guardrail name does. This matches LiteLLM's behavior, which includes the guardrail identity in blocked-request error details (e.g.guardrailIdentifierfor Bedrock,category/pattern_namefor its content filter).B.5 + E.2 — dead code removal
MaxContentLengthdeleted:GuardrailKindhas no Length variant, so the type was unreachable from configuration.GuardrailVerdict::Rewritedeleted: no guardrail implementation ever returned it. The chain'sCow<ChatFormat>propagation machinery and the proxy's rewrite-substitution path existed only for this variant and are collapsed.No new tests for the removals — the existing suite passing covers them.
Tests
build.rs: chain order is created_at-ascending with id tiebreak (fails deterministically on the unfixed DashMap iteration), id-only fallback whencreated_atis absent, and the same ordering through the productionGuardrailIndex::resolvefallback path.chain.rs:Blockattribution carries the firing member's name; nested chains keep the innermost attribution.guardrail-keyword-e2e,guardrail-output-e2e) assert the 422 / SSE-error envelope names the guardrail while still never leaking the matched literal.Summary by CodeRabbit
New Features
Removals
Documentation
Tests
Closes api7/AISIX-Cloud#519