fix(guardrails): split over-limit content instead of clipping it, and fail closed by default

crates/aisix-core/src/models/guardrail.rs

@@ -1198,10 +1198,17 @@ pub struct Guardrail {
     #[serde(default)]
     pub hook_point: GuardrailHookPoint,
 
-    /// Behavior when a remote API guardrail cannot reach its upstream.
-    /// `true` allows the request and records the bypass reason in
-    /// `usage_events.guardrail_bypassed_reason`. `false` blocks with
-    /// 422. Keyword guardrails do not use this setting.
+    /// Behavior when a remote API guardrail cannot complete its check —
+    /// upstream unreachable, timing out, throttling, or rejecting the
+    /// call. `true` allows the request and records the bypass reason in
+    /// `usage_events.guardrail_bypassed_reason`; `false` (the default)
+    /// blocks with 422. Keyword guardrails do not use this setting.
+    ///
+    /// Defaults to fail-closed so an unchecked request is never released
+    /// on the strength of a guardrail that did not run: an operator who
+    /// prefers availability over enforcement opts in explicitly. This
+    /// matches `output_fail_open` and `on_buffer_exceeded`, which have
+    /// always defaulted closed (AISIX-Cloud#1382).
     #[serde(default = "default_fail_open")]
     pub fail_open: bool,
 
@@ -1242,7 +1249,7 @@ fn default_enabled() -> bool {
 }
 
 fn default_fail_open() -> bool {
-    true
+    false
 }
 
 fn default_enforcement_mode() -> String {
@@ -1402,7 +1409,9 @@ mod tests {
         let g: Guardrail = serde_json::from_value(v).unwrap();
         assert!(g.enabled);
         assert_eq!(g.hook_point, GuardrailHookPoint::Both);
-        assert!(g.fail_open);
+        // Fail-closed by default: a guardrail that could not run must not
+        // release the request (AISIX-Cloud#1382).
+        assert!(!g.fail_open);
     }
 
     #[test]

crates/aisix-guardrails/AGENTS.md

@@ -0,0 +1,42 @@
+# aisix-guardrails
+
+## A provider's per-call size limit is handled by `chunk::chunk_text`, never by truncating
+
+Every remote kind talks to an API that caps how much text one call may
+carry. When you add or change one, do not re-decide what happens at that
+cap — the family already answered it, and answering it again in isolation
+is how the same defect shipped twice (#448, then AISIX-Cloud#1381):
+
+- **Split, never clip.** Over-limit content is chunked and *every* chunk
+  is submitted. Truncating hands the caller a bypass they control: the
+  text is assembled oldest-message-first, so clipping drops the newest
+  turn — the one being screened — and the call still returns a clean
+  verdict, so nothing logs, counts, or looks wrong.
+- **No cap on chunk count.** A per-request chunk budget is unscanned
+  content through the back door. Cost scales with content; that is the
+  trade.
+- **The split is lossless** (`chunks.concat() == text`). Kinds that write
+  masked text back rebuild the caller's content from per-chunk
+  replacements, so any "clever" normalising split silently corrupts
+  bodies rather than failing.
+- **Count characters, not bytes.** The vendor limits are documented in
+  characters, and byte slicing halves a multi-byte character.
+
+A kind whose provider documents no limit (bedrock, lakera, presidio,
+openai_moderation) submits whole — its bound is the provider's own. Do
+not invent a local one for it.
+
+## Fail policies default closed
+
+`fail_open`, `output_fail_open` and `on_buffer_exceeded` all default to
+the blocking side: a check that could not run must not release the
+request. Any new failure path gets the same default, and an operator who
+prefers availability opts in explicitly.
+
+A guardrail that could not evaluate reaches the request through **two**
+shapes, and a change to either must keep both intact: an explicitly
+fail-open row emits `Bypass` (which `MandatoryGuardrail` upgrades on the
+way out), while a fail-closed row emits `Block { unavailable: Some(tag) }`
+(which `MonitorGuardrail` must not downgrade for a `mandatory` row). Both
+carry the same bounded per-kind failure tag; neither may carry matched
+content (#153).

crates/aisix-guardrails/src/aliyun.rs

@@ -53,6 +53,7 @@ use hmac::{Hmac, Mac};
 use serde::Deserialize;
 use sha1::Sha1;
 
+use crate::chunk::chunk_text;
 use crate::{Guardrail, GuardrailVerdict, StreamOutputPolicy};
 
 type HmacSha1 = Hmac<Sha1>;
@@ -64,7 +65,9 @@ const SERVICE_OUTPUT: &str = "llm_response_moderation";
 
 /// Per-call content cap (chars). Aliyun caps `llm_query_moderation` at
 /// 2 000 and `llm_response_moderation` at 5 000; 2 000 is the safe shared
-/// bound and matches the default streaming window.
+/// bound and matches the default streaming window. It bounds one CALL,
+/// not one request — longer text is split across calls, never clipped
+/// (see `crate::chunk`).
 const MAX_CONTENT_CHARS: usize = 2_000;
 
 /// One Aliyun Text Moderation row, materialised into a request-time
@@ -136,18 +139,43 @@ impl AliyunTextModerationGuardrail {
     /// Moderate one piece of text with the given service code. `session_id`
     /// (when set) is forwarded as `ServiceParameters.sessionId` so Aliyun
     /// correlates the chunks of one streamed response.
+    ///
+    /// Aliyun caps content per call, so text over the cap is split and
+    /// EVERY chunk is submitted — the first one that reaches the risk
+    /// threshold blocks the request. It is deliberately not truncated:
+    /// the text is assembled oldest-message-first, so clipping to the cap
+    /// dropped the newest turn — the one actually being screened — and
+    /// released it under a clean verdict (AISIX-Cloud#1381).
     async fn moderate(
         &self,
         service: &str,
         text: &str,
         session_id: Option<&str>,
         fail_open: bool,
     ) -> GuardrailVerdict {
-        // Aliyun caps content per call; truncate to the cap. Streaming
-        // already windows to MAX_CONTENT_CHARS; non-streaming long inputs
-        // are clamped (the leading content carries the risk in practice).
-        let content: String = text.chars().take(MAX_CONTENT_CHARS).collect();
-        let (outcome, diag) = self.call(service, &content, session_id).await;
+        for content in chunk_text(text, MAX_CONTENT_CHARS) {
+            match self
+                .moderate_chunk(service, &content, session_id, fail_open)
+                .await
+            {
+                GuardrailVerdict::Allow => continue,
+                // Block or Bypass: strictest-wins, and there is no point
+                // burning provider calls on the rest of the request.
+                verdict => return verdict,
+            }
+        }
+        GuardrailVerdict::Allow
+    }
+
+    /// One provider call over one already-bounded chunk.
+    async fn moderate_chunk(
+        &self,
+        service: &str,
+        content: &str,
+        session_id: Option<&str>,
+        fail_open: bool,
+    ) -> GuardrailVerdict {
+        let (outcome, diag) = self.call(service, content, session_id).await;
         match outcome {
             Ok(level) => {
                 let blocked = risk_rank(&level) >= self.threshold_rank;
@@ -866,6 +894,96 @@ mod tests {
         assert_eq!(g.check_input(&req("hello")).await, GuardrailVerdict::Allow);
     }
 
+    // --- per-call cap: split, never clip (AISIX-Cloud#1381) ---------------
+
+    /// The bug this replaced: the text is assembled oldest-message-first,
+    /// so clipping to `MAX_CONTENT_CHARS` meant the newest turn — the one
+    /// actually being screened — was never submitted. Aliyun answered
+    /// `none` for the benign history and the request was released clean.
+    #[tokio::test]
+    async fn risk_past_the_cap_in_the_newest_message_still_blocks() {
+        let server = MockServer::start().await;
+        // The benign history: every call whose content lacks the marker
+        // comes back clean.
+        Mock::given(method("POST"))
+            .and(body_string_contains("RISKMARKER"))
+            .respond_with(ResponseTemplate::new(200).set_body_json(risk_body("high")))
+            .mount(&server)
+            .await;
+        Mock::given(method("POST"))
+            .respond_with(ResponseTemplate::new(200).set_body_json(risk_body("none")))
+            .mount(&server)
+            .await;
+
+        let history = "benign ".repeat(MAX_CONTENT_CHARS); // far past one call
+        let newest = "RISKMARKER";
+        let req = ChatFormat::new(
+            "m",
+            vec![ChatMessage::user(&history), ChatMessage::user(newest)],
+        );
+
+        let g = build(&server.uri(), "high", true);
+        assert!(
+            g.check_input(&req).await.is_block(),
+            "the newest message sits past the per-call cap; it must still be scanned",
+        );
+    }
+
+    /// The output side of the same clip (`check_output` shares `moderate`).
+    #[tokio::test]
+    async fn risk_past_the_cap_in_the_output_tail_still_blocks() {
+        let server = MockServer::start().await;
+        Mock::given(method("POST"))
+            .and(body_string_contains("RISKMARKER"))
+            .respond_with(ResponseTemplate::new(200).set_body_json(risk_body("high")))
+            .mount(&server)
+            .await;
+        Mock::given(method("POST"))
+            .respond_with(ResponseTemplate::new(200).set_body_json(risk_body("none")))
+            .mount(&server)
+            .await;
+
+        let long = format!("{}RISKMARKER", "benign ".repeat(MAX_CONTENT_CHARS));
+        let g = build(&server.uri(), "high", true);
+        assert!(
+            g.check_output(&resp(&long)).await.is_block(),
+            "a response tail past the per-call cap must still be scanned",
+        );
+    }
+
+    /// Coverage is what matters, but the cost shape matters too: content
+    /// that fits stays exactly one call, and content that does not costs
+    /// one call per chunk rather than silently dropping the remainder.
+    #[tokio::test]
+    async fn content_within_the_cap_is_still_a_single_call() {
+        let server = MockServer::start().await;
+        Mock::given(method("POST"))
+            .respond_with(ResponseTemplate::new(200).set_body_json(risk_body("none")))
+            .expect(1)
+            .mount(&server)
+            .await;
+        let g = build(&server.uri(), "high", true);
+        assert_eq!(
+            g.check_input(&req("short and clean")).await,
+            GuardrailVerdict::Allow
+        );
+    }
+
+    #[tokio::test]
+    async fn content_over_the_cap_costs_one_call_per_chunk() {
+        let server = MockServer::start().await;
+        Mock::given(method("POST"))
+            .respond_with(ResponseTemplate::new(200).set_body_json(risk_body("none")))
+            // 3 chunks' worth of whitespace-free text → 3 calls, and the
+            // `expect` fails the test if the tail was dropped instead.
+            .expect(3)
+            .mount(&server)
+            .await;
+        let g = build(&server.uri(), "high", true);
+        let text = "x".repeat(MAX_CONTENT_CHARS * 3);
+        assert_eq!(g.check_input(&req(&text)).await, GuardrailVerdict::Allow);
+    }
+
     #[tokio::test]
     async fn high_risk_input_blocks_at_high_threshold() {
         let server = MockServer::start().await;