From 4f2a1d866697144c519c6ee135a114dcd46afc95 Mon Sep 17 00:00:00 2001 From: jif Date: Thu, 27 Aug 2026 14:24:52 +0000 Subject: [PATCH] Fail closed on unbounded Guardian parent compactions (#41152) ## Why Guardian classification must not proceed after silently dropping a parent compaction that cannot fit within its configured size limit. ## What changed - Distinguish an absent or unusable parent compaction from serialization and size-limit failures. - Record a fail-closed score and failed classification when the latest encrypted parent compaction cannot be serialized or exceeds the complete-item byte budget. - Preserve omission of compactions with missing or empty encrypted content without falling back to older context. ## Testing Cover compactions at the exact byte limit, one byte over it, and oversized passthrough metadata. GitOrigin-RevId: 8e607c88f10003cdfab273963bc73c848b09c821 --- .../guardian-v2/src/async_scorer/extension.rs | 93 +++++++++---------- .../src/async_scorer/extension_tests.rs | 13 +-- 2 files changed, 49 insertions(+), 57 deletions(-) diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs index 330a139af2..6260214b19 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -442,49 +442,25 @@ impl GuardianV2Extension { .enable_image_capture(); } input.thread_store.insert(guardian_config.clone()); - let latest_parent_compaction = if guardian_config.reuse_parent_compaction { - input - .conversation_history - .items() - .filter(|item| { - matches!( - item, - ResponseItem::Compaction { .. } | ResponseItem::ContextCompaction { .. } - ) - }) - .last() + let parent_compaction = if guardian_config.reuse_parent_compaction { + match encrypted_parent_compaction( + input.conversation_history.items(), + guardian_config.max_parent_compaction_tokens, + ) { + Ok(compaction) => compaction, + Err(_) => { + Self::record_fail_closed_score(input.thread_store, sampled_at); + record_classification( + metrics.as_deref(), + classification_started_at.elapsed(), + "failure", + ); + return; + } + } } else { None }; - let parent_compaction = latest_parent_compaction.and_then(|item| { - encrypted_parent_compaction( - std::iter::once(item), - guardian_config.max_parent_compaction_tokens, - ) - }); - if parent_compaction.is_none() - && latest_parent_compaction.is_some_and(|item| match item { - ResponseItem::Compaction { - id: Some(_), - encrypted_content, - .. - } - | ResponseItem::ContextCompaction { - id: Some(_), - encrypted_content: Some(encrypted_content), - .. - } => !encrypted_content.is_empty(), - _ => false, - }) - { - Self::record_fail_closed_score(input.thread_store, sampled_at); - record_classification( - metrics.as_deref(), - classification_started_at.elapsed(), - "failure", - ); - return; - } let call_id = input.call_id.to_owned(); let mcp_tool = input.mcp_tool.cloned(); let action = GuardianAction { @@ -754,21 +730,32 @@ impl GuardianV2Extension { } } +#[derive(Debug, Eq, PartialEq)] +enum ParentCompactionError { + Serialization, + Oversized, +} + +// An unusable latest compaction must never fall back to an older one. Missing +// encrypted content can be omitted; content that cannot be bounded rejects the sample. fn encrypted_parent_compaction<'a>( items: impl Iterator, max_parent_compaction_tokens: usize, -) -> Option { +) -> Result, ParentCompactionError> { let max_compaction_bytes = TruncationPolicy::Tokens(max_parent_compaction_tokens).byte_budget(); - let item = items + let Some(item) = items .filter(|item| { matches!( item, ResponseItem::Compaction { .. } | ResponseItem::ContextCompaction { .. } ) }) - .last()?; + .last() + else { + return Ok(None); + }; - match item { + let encrypted_content = match item { ResponseItem::Compaction { id: Some(_), encrypted_content, @@ -778,14 +765,18 @@ fn encrypted_parent_compaction<'a>( id: Some(_), encrypted_content: Some(encrypted_content), .. - } if !encrypted_content.is_empty() - && serde_json::to_vec(item) - .is_ok_and(|serialized| serialized.len() <= max_compaction_bytes) => - { - Some(item.clone()) - } - _ => None, + } => encrypted_content, + _ => return Ok(None), + }; + if encrypted_content.is_empty() { + return Ok(None); } + let serialized = serde_json::to_vec(item).map_err(|_| ParentCompactionError::Serialization)?; + if serialized.len() > max_compaction_bytes { + return Err(ParentCompactionError::Oversized); + } + + Ok(Some(item.clone())) } /// Installs feature-gated Guardian V2 tool classification for each thread. diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs index 75b360c0be..8b4025e2ed 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs @@ -57,6 +57,7 @@ use serde_json::json; use super::GuardianV2Extension; use super::GuardianV2ScoreProgress; +use super::ParentCompactionError; use super::StrictReviewReason; use super::encrypted_parent_compaction; use super::should_classify_tool; @@ -645,14 +646,14 @@ fn encrypted_parent_compaction_preserves_the_latest_valid_item() { [&older, &latest].into_iter(), DEFAULT_PARENT_COMPACTION_TOKENS, ), - Some(latest.clone()) + Ok(Some(latest.clone())) ); assert_eq!( encrypted_parent_compaction( [&latest, &older].into_iter(), DEFAULT_PARENT_COMPACTION_TOKENS, ), - Some(older) + Ok(Some(older)) ); } @@ -697,7 +698,7 @@ fn encrypted_parent_compaction_rejects_invalid_latest_item() { [&older, latest].into_iter(), DEFAULT_PARENT_COMPACTION_TOKENS, ), - None, + Ok(None), "an unusable latest summary must not resurrect older context" ); } @@ -736,7 +737,7 @@ fn encrypted_parent_compaction_rejects_oversized_latest_item() -> Result<()> { assert_eq!(serde_json::to_vec(&*item)?.len(), max_compaction_bytes); assert_eq!( encrypted_parent_compaction(std::iter::once(&*item), DEFAULT_PARENT_COMPACTION_TOKENS,), - Some(item.clone()) + Ok(Some(item.clone())) ); let mut oversized = item.clone(); @@ -759,7 +760,7 @@ fn encrypted_parent_compaction_rejects_oversized_latest_item() -> Result<()> { [&*item, &oversized].into_iter(), DEFAULT_PARENT_COMPACTION_TOKENS, ), - None, + Err(ParentCompactionError::Oversized), "an oversized latest summary must not resurrect older context" ); } @@ -780,7 +781,7 @@ fn encrypted_parent_compaction_rejects_oversized_latest_item() -> Result<()> { [&bounded[0], &oversized_metadata].into_iter(), DEFAULT_PARENT_COMPACTION_TOKENS, ), - None, + Err(ParentCompactionError::Oversized), "oversized passthrough metadata must not bypass the complete-item limit" );