fix(guardrails): enforce monitor mode and fail output closed by default - #640
Merged
Merged
Conversation
Two guardrail behaviors contradicted their configuration (AISIX-Cloud#788
P1-3).
enforcement_mode: monitor was stored but never enforced — the DP logged a
warning and then hard-blocked anyway, so operators who expected an audit
mode were actually rejecting traffic. build_one now wraps a monitor-mode
row in a MonitorGuardrail decorator that runs the rule exactly as configured
but downgrades a Block to Allow, emitting an audit log line ("would have
blocked") with the guardrail name and hook side. It also reports
EndOfStreamCheck for streamed output so a rule that can never block does not
force the response to hold back; it can never weaken a blocking peer because
the chain keeps the strictest member's policy. An unrecognized mode falls
back to block (fail-safe). chain.rs and index.rs are untouched.
Remote guardrail output fail-open was inconsistent: Bedrock and Azure Prompt
Shield used a single fail_open (default true) for both hooks, so a provider
outage on the output side released unscanned model output. They now carry an
independent output_fail_open (default false / fail-closed), matching the
Azure/Aliyun text-moderation guardrails. The input hook still follows the
outer fail_open; only the output hook changes its default.
Behavior changes:
- monitor-mode guardrails observe and log instead of blocking.
- Bedrock / azure_content_safety output hook fails closed by default on a
provider outage. Operators who want the old behavior set
output_fail_open: true.
Part of api7/AISIX-Cloud#788
|
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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds independent per-hook fail-open policies to Bedrock and Azure Content Safety guardrails: the input hook follows the existing outer ChangesPer-hook output fail-open policy (Bedrock + Azure Content Safety)
Monitor enforcement mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@crates/aisix-core/src/models/guardrail.rs`:
- Around line 131-134: The doc comment for the fail-open policy field in the
guardrail model at lines 131-134 contains implementation details and internal
references that are not appropriate for public API documentation. Rewrite this
comment to focus on clear, operator-facing semantics by removing references to
"outer `fail_open`" and internal comparisons like "Mirrors the independent
output policy the Azure/Aliyun text-moderation guardrails already use". Describe
what the feature does from the user's perspective in straightforward language.
Apply the same user-facing approach to the additional comments at lines 356-359,
then regenerate the JSON schema to reflect the updated model documentation.
In `@crates/aisix-guardrails/src/bedrock.rs`:
- Line 519: The test in the BedrockGuardrail::new call is setting both the outer
fail_open parameter and output_fail_open to true, which means the test cannot
distinguish whether output is correctly using its own policy versus accidentally
using the input policy. Change the fourth parameter (the outer fail_open) from
true to false in the BedrockGuardrail::new call so that input and output
policies are properly distinguished during testing.
In `@crates/aisix-guardrails/src/prompt_shield.rs`:
- Around line 563-566: The output-policy tests are not properly verifying the
HTTP path and wiring because they can pass for the wrong reason (404/ConfigError
can also block). Replace the direct call to `handle_failure` in the opt-in test
with a call to `check_output` function and pass `fail_open=false` as a
parameter. This will prove that the output hook properly uses the
`output_fail_open` setting and actually exercises the intended HTTP path with
the 500 response from the mock at `/contentsafety/text:shieldPrompt`, rather
than just relying on unmatched mocks returning errors.
🪄 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: c3f122c8-bf4c-42a2-b702-701dbceb1418
📒 Files selected for processing (6)
crates/aisix-core/src/models/guardrail.rscrates/aisix-guardrails/src/bedrock.rscrates/aisix-guardrails/src/build.rscrates/aisix-guardrails/src/prompt_shield.rsschemas/resources/guardrail.schema.jsontests/e2e/src/cases/guardrail-monitor-mode-e2e.test.ts
Address CodeRabbit on #640: - Reword output_fail_open schema descriptions to user-facing semantics (drop the internal 'outer fail_open' / 'mirrors Azure/Aliyun' phrasing); regenerate guardrail.schema.json. - Bedrock opt-in test sets input fail_open=false so a passing output bypass proves the output hook uses output_fail_open, not the input policy; also assert input still blocks. - Prompt Shield output tests drive the real HTTP path (path + api-version matcher, expect(1)) so the verdict can't come from an unmatched-route 404; the opt-in test runs check_output with input fail_open=false.
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.
Problem
Two guardrail behaviors contradicted their own configuration (architecture review item AISIX-Cloud#788 P1-3):
enforcement_mode: monitorwas never enforced. The DP stored the field, logged a warning at chain-build time, then hard-blocked anyway. Operators who set monitor expecting an audit/observe mode were actually rejecting traffic.azure_content_safety) used a singlefail_open(defaulttrue) for both the input and output hooks, so a provider outage on the output side released unscanned model output. The Azure/Aliyun text-moderation guardrails already split this with an independentoutput_fail_opendefaulting to fail-closed.Implementation
Monitor mode —
build_onenow appliesenforcement_modeafter building a row's runtime guardrail:block(default): returned as-is.monitor: wrapped in aMonitorGuardraildecorator that runs the rule exactly as configured but downgrades aBlockverdict toAllow, emitting an audit log line (guardrail name +input/outputhook + reason). It reportsEndOfStreamCheckfor streamed output so a rule that can never block does not force hold-back; it can never weaken a blocking peer because the chain folds to the strictest member.block(fail-safe) with a warning.chain.rsandindex.rsare untouched — the decorator is fully contained inbuild.rs.Output fail-closed —
BedrockConfigandAzureContentSafetyConfiggain an independentoutput_fail_open(#[serde(default)]→false). The bedrock/prompt-shield dispatchers now pick the input-sidefail_openforcheck_inputandoutput_fail_openforcheck_output, mirroring the existing Azure/Aliyun text-moderation pattern. The runtime/published JSON Schema is regenerated viadump-schema.Behavior changes
bedrock/azure_content_safetyoutput hook now fails closed by default on a provider outage. Operators who prefer availability over correctness setoutput_fail_open: true. The input hook is unchanged.Tests
tests/e2e/src/cases/guardrail-monitor-mode-e2e.test.ts): creates a keyword rule inblockmode, confirms it 422s, flips the same rule tomonitor, and confirms the forbidden request now reaches the upstream — the block→monitor flip makes the assertion non-racy.Docs: paired
api7/docsPR documentsenforcement_modeand the input/output fail-open split.Part of api7/AISIX-Cloud#788
Summary by CodeRabbit
New Features
Enhancements
output_fail_openpolicy for the output hook, defaulting to fail-closed while the input hook continues using the existing top-levelfail_open.Testing / Validation
Fixes api7/AISIX-Cloud#788