Harden Guardian reviews after context compaction (#42852)

## Why

Guardian reviews must not lose user authorization constraints or reuse an
unreadable or incompatible parent checkpoint after compaction.

## What changed

- Retain bounded excerpts of oversized root user messages across compaction,
  resume, and rollback, and recover excerpts from legacy Guardian history when
  available.
- Require thread-owned Guardian review sessions and asynchronous scoring to
  reuse only non-empty checkpoints produced by a compatible review model.
- Fail closed instead of reviewing without the required checkpoint when reuse
  is disabled, compatibility is unknown, or checkpoint content is unusable.

## Testing

Expanded retained-context, review-session, asynchronous-scoring, and app-server
coverage for oversized instructions, legacy recovery, model incompatibility,
missing content, resume, rollback, and disabled checkpoint reuse.

GitOrigin-RevId: f30d837197e0d838e656a632109f490c38208f5e
This commit is contained in:
felixxia-oai
2026-09-04 21:07:14 +00:00
committed by copyberry
parent 8e4b7d31de
commit 4636819a35
15 changed files with 598 additions and 104 deletions
@@ -11,6 +11,7 @@ use crate::compact::is_summary_message;
use crate::context::GuardianReviewEvidence;
use crate::context::is_contextual_user_fragment;
use crate::event_mapping::parse_turn_item;
use crate::guardian::GUARDIAN_MAX_ROOT_MESSAGE_TOKENS;
use crate::guardian::guardian_truncate_text;
use codex_protocol::AgentPath;
use codex_protocol::ThreadId;
@@ -22,7 +23,6 @@ use codex_protocol::models::ResponseItem;
use codex_protocol::protocol::MultiAgentVersion;
const MAX_ROOT_MESSAGES: usize = 8;
const MAX_ROOT_MESSAGE_TOKENS: usize = 900;
impl AgentControl {
/// Returns bounded root conversation and authorization state for a MultiAgent V2 worker.
@@ -57,8 +57,8 @@ impl AgentControl {
.filter_map(|entry| match entry {
codex_history::RetainedContextEntry::UserMessage(message) => {
let text = if message.text.is_empty() && !message.complete {
// Storage may omit a large instruction. After parent compaction,
// Guardian history can still retain that exact source message.
// Older records may omit a large instruction. Recover that exact
// source while it remains available in the parent context.
let original = message.message_id.as_deref().and_then(|id| {
root_history.raw_items().chain(history.review_items()).find(
|item| item.id().is_some_and(|item_id| item_id.as_str() == id),
@@ -84,7 +84,7 @@ impl AgentControl {
.then(|| {
latest_user_turn_id = Some(message.turn_id.clone());
GuardianRootMessage::User(
guardian_truncate_text(&text, MAX_ROOT_MESSAGE_TOKENS).0,
guardian_truncate_text(&text, GUARDIAN_MAX_ROOT_MESSAGE_TOKENS).0,
)
})
}
@@ -113,7 +113,7 @@ impl AgentControl {
})
.collect::<String>();
Some(GuardianRootMessage::Assistant(
guardian_truncate_text(&text, MAX_ROOT_MESSAGE_TOKENS).0,
guardian_truncate_text(&text, GUARDIAN_MAX_ROOT_MESSAGE_TOKENS).0,
))
})
.collect::<Vec<_>>();
@@ -147,7 +147,8 @@ impl AgentControl {
.then(|| {
latest_user_turn_id = item.turn_id().map(str::to_owned);
GuardianRootMessage::User(
guardian_truncate_text(&message, MAX_ROOT_MESSAGE_TOKENS).0,
guardian_truncate_text(&message, GUARDIAN_MAX_ROOT_MESSAGE_TOKENS)
.0,
)
})
}
@@ -162,7 +163,7 @@ impl AgentControl {
})
.collect::<String>();
Some(GuardianRootMessage::Assistant(
guardian_truncate_text(&text, MAX_ROOT_MESSAGE_TOKENS).0,
guardian_truncate_text(&text, GUARDIAN_MAX_ROOT_MESSAGE_TOKENS).0,
))
}
(_, ResponseItem::FunctionCall { call_id, .. }) => root_evidence
+42 -3
View File
@@ -1,6 +1,8 @@
//! Parent model history and bounded host-owned context facts.
//! Compaction replaces only the model window. Snapshots include retained facts atomically;
//! checkpoint replay and source-call rollback share their live lifecycle.
//! Oversized instructions keep an incomplete excerpt for bounded root review, including
//! sources recovered from legacy Guardian checkpoints before their raw history is dropped.
use crate::context::ContextualUserFragment;
use crate::context::ModelSwitchInstructions;
@@ -11,6 +13,9 @@ use crate::context_manager::normalize;
use crate::event_mapping::has_non_contextual_dev_message_content;
use crate::event_mapping::is_contextual_dev_message_content;
use crate::event_mapping::is_contextual_user_message_content;
use crate::event_mapping::parse_turn_item;
use crate::guardian::GUARDIAN_MAX_ROOT_MESSAGE_TOKENS;
use crate::guardian::guardian_truncate_text;
use crate::session::turn_context::TurnContext;
use crate::utils::json::serialized_json_bytes;
use base64::Engine;
@@ -25,6 +30,7 @@ use codex_history::GuardianHistoryCheckpoint;
use codex_history::ResponseItemEnvelope;
use codex_history::RetainedContext;
use codex_history::RetainedContextEvent;
use codex_protocol::items::TurnItem;
use codex_protocol::models::AgentMessageInputContent;
use codex_protocol::models::BaseInstructions;
use codex_protocol::models::ContentItem;
@@ -57,7 +63,7 @@ pub(crate) struct ContextManager {
/// The oldest items are at the beginning of the vector. Snapshots share the vector until a
/// caller needs to mutate it, avoiding deep copies for read-only history consumers.
items: Arc<Vec<ResponseItemEnvelope>>,
/// Starts at the first compaction; ordinary history snapshots need no second payload copy.
/// Legacy-only history, started at first compaction. Thread-owned mode reads parent context.
review_history: Option<TranscriptHistory>,
/// Host facts independent of the model window; snapshots share immutable state.
retained_context: Arc<RetainedContext>,
@@ -207,15 +213,42 @@ impl ContextManager {
}
pub(crate) fn guardian_history_checkpoint(&self) -> Option<GuardianHistoryCheckpoint> {
if self.retain_user_messages {
return None;
}
self.review_history
.as_ref()
.map(|history| GuardianHistoryCheckpoint(history.items().cloned().collect()))
}
pub(crate) fn restore_guardian_history(
pub(crate) fn restore_review_context(
&mut self,
retained_context: Option<&RetainedContext>,
checkpoint: Option<&GuardianHistoryCheckpoint>,
) {
self.restore_retained_context(retained_context);
if self.retain_user_messages {
// Older retained checkpoints cleared oversized instructions. Recover their
// bounded root excerpts before discarding the legacy source transcript.
let items = &self.items;
Arc::make_mut(&mut self.retained_context).recover_user_message_excerpts(|id| {
// Prefer the backup over a compacted copy that retains the original ID.
let original = checkpoint
.into_iter()
.flat_map(|checkpoint| &checkpoint.0)
.chain(items.iter().map(|envelope| &envelope.item))
.find(|item| item.id().is_some_and(|item_id| item_id.as_str() == id));
let Some(TurnItem::UserMessage(original)) = original.and_then(parse_turn_item)
else {
return None;
};
Some(
guardian_truncate_text(&original.message(), GUARDIAN_MAX_ROOT_MESSAGE_TOKENS).0,
)
});
self.review_history = None;
return;
}
let generation = self
.review_history
.as_ref()
@@ -361,6 +394,12 @@ impl ContextManager {
})
.collect::<Vec<_>>()
.join("\n");
// Keep the same bounded text that child reviewers receive before
// compaction, instead of letting storage discard a large source.
// Local instruction sections still omit incomplete originals whole.
let (text, truncated) =
guardian_truncate_text(&text, GUARDIAN_MAX_ROOT_MESSAGE_TOKENS);
complete &= !truncated;
Arc::make_mut(&mut self.retained_context).record_user_message(
codex_history::RetainedUserMessage {
turn_id: item.turn_id().unwrap_or_default().to_owned(),
@@ -489,7 +528,7 @@ impl ContextManager {
/// Compaction changes the model's history without changing the user's authorization.
pub(crate) fn replace_compacted(&mut self, items: Vec<ResponseItemEnvelope>) {
if self.review_history.is_none() {
if !self.retain_user_messages && self.review_history.is_none() {
let mut retained = TranscriptHistory::new(self.history_version.saturating_add(1));
for item in self.raw_items().filter(|item| {
!matches!(item, ResponseItem::Message { role, content, .. }
+1
View File
@@ -73,6 +73,7 @@ const GUARDIAN_MAX_MESSAGE_TRANSCRIPT_TOKENS: usize = 20_000;
const GUARDIAN_MAX_TOOL_TRANSCRIPT_TOKENS: usize = 10_000;
const GUARDIAN_MAX_MESSAGE_ENTRY_TOKENS: usize = 5_000;
const GUARDIAN_MAX_TOOL_ENTRY_TOKENS: usize = 1_000;
pub(crate) const GUARDIAN_MAX_ROOT_MESSAGE_TOKENS: usize = 900;
pub(crate) const GUARDIAN_MAX_NODE_REPL_TOOL_RESULT_TOKENS: usize = 6_000;
pub(crate) const GUARDIAN_MAX_ACTION_BYTES: usize = 8_000;
const GUARDIAN_MAX_ACTION_STRING_TOKENS: usize = 16_000;
+3
View File
@@ -962,6 +962,7 @@ pub(crate) fn spawn_approval_request_review(
pub(super) struct GuardianReviewSessionConfig {
pub(super) spawn_config: crate::config::Config,
pub(super) node_repl_policy: GuardianNodeReplPolicy,
pub(super) compaction_model_hash: Option<String>,
model: String,
reasoning_effort: Option<codex_protocol::openai_models::ReasoningEffort>,
default_review_model_id: String,
@@ -1063,6 +1064,7 @@ pub(super) async fn guardian_review_session_config(
}
Ok(GuardianReviewSessionConfig {
spawn_config,
compaction_model_hash: guardian_model_info.comp_hash.clone(),
node_repl_policy: GuardianNodeReplPolicy::from_model_messages(
guardian_model_info.model_messages.as_ref(),
),
@@ -1121,6 +1123,7 @@ async fn run_guardian_review_session_before_deadline(
reasons,
schema,
model: session_config.model,
compaction_model_hash: session_config.compaction_model_hash,
reasoning_effort: session_config.reasoning_effort,
guardian_default_review_model_id: session_config.default_review_model_id,
guardian_catalog_contains_auto_review: session_config.catalog_contains_auto_review,
+70 -24
View File
@@ -54,6 +54,7 @@ use crate::config::Permissions;
use crate::context::ContextualUserFragment;
use crate::context::GuardianFollowupReviewReminder;
use crate::context::GuardianNodeReplPolicy;
use crate::context_manager::ContextManager;
use crate::environment_selection::TurnEnvironmentSnapshot;
use crate::image_preparation::ImagePreparationMode;
use crate::image_preparation::ImageResizeNoticeMode;
@@ -113,6 +114,7 @@ pub(crate) struct GuardianReviewSessionParams {
pub(crate) reasons: ApprovalRequestReasons,
pub(crate) schema: Value,
pub(crate) model: String,
pub(crate) compaction_model_hash: Option<String>,
pub(crate) reasoning_effort: Option<ReasoningEffortConfig>,
pub(crate) guardian_default_review_model_id: String,
pub(crate) guardian_catalog_contains_auto_review: bool,
@@ -230,6 +232,9 @@ impl GuardianReviewSessionReuseKey {
parent_history_version: if spawn_config
.features
.enabled(Feature::GuardianReuseParentCompaction)
|| spawn_config
.features
.enabled(Feature::GuardianThreadContext)
{
parent_history_version
} else {
@@ -280,31 +285,62 @@ impl GuardianReviewSessionReuseKey {
}
}
fn encrypted_parent_compaction<'a, I>(items: I) -> Option<ResponseItem>
where
I: IntoIterator<Item = &'a ResponseItem>,
I::IntoIter: DoubleEndedIterator,
{
let item = items.into_iter().rev().find(|item| {
fn encrypted_parent_compaction(
history: &ContextManager,
features: &ManagedFeatures,
reviewer_compaction_hash: Option<&str>,
) -> anyhow::Result<Option<ResponseItem>> {
let strict = features.enabled(Feature::GuardianThreadContext);
if !strict && !features.enabled(Feature::GuardianReuseParentCompaction) {
return Ok(None);
}
let Some(envelope) = history.annotated_items().iter().rev().find(|envelope| {
matches!(
item,
envelope.item,
ResponseItem::Compaction { .. } | ResponseItem::ContextCompaction { .. }
)
})?;
}) else {
return Ok(None);
};
match item {
let item = &envelope.item;
let valid = match item {
ResponseItem::Compaction {
id: Some(_),
encrypted_content,
..
} if !encrypted_content.is_empty() => Some(item.clone()),
} if !encrypted_content.is_empty() => true,
ResponseItem::ContextCompaction {
id: Some(_),
encrypted_content: Some(encrypted_content),
..
} if !encrypted_content.is_empty() => Some(item.clone()),
_ => None,
} if !encrypted_content.is_empty() => true,
_ => false,
};
if !valid && !strict {
return Ok(None);
}
anyhow::ensure!(
valid,
"parent compaction checkpoint is unusable for Guardian review"
);
if strict {
// A resumed parent may now use a different model. Compare the actual
// checkpoint producer with the selected reviewer, not the live parent model.
let producer_hash = envelope
.metadata
.as_ref()
.and_then(|metadata| metadata.compaction_model_hash.as_deref());
anyhow::ensure!(
producer_hash
.zip(reviewer_compaction_hash)
.is_some_and(|(producer, reviewer)| {
!producer.is_empty() && producer == reviewer
}),
"parent compaction checkpoint is incompatible with the Guardian review model or its compatibility is unknown"
);
}
Ok(Some(item.clone()))
}
pub(crate) fn prompt_cache_key_override_for_review_session(
@@ -446,11 +482,11 @@ impl GuardianReviewSessionManager {
} else {
None
};
let parent_compaction = spawn_config
.features
.enabled(Feature::GuardianReuseParentCompaction)
.then(|| encrypted_parent_compaction(parent_history.raw_items()))
.flatten();
let parent_compaction = encrypted_parent_compaction(
&parent_history,
&spawn_config.features,
session_config.compaction_model_hash.as_deref(),
)?;
let parent_context = GuardianReviewContext::from(parent_turn);
let mut reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
&spawn_config,
@@ -551,12 +587,19 @@ impl GuardianReviewSessionManager {
} else {
None
};
let parent_compaction = params
.spawn_config
.features
.enabled(Feature::GuardianReuseParentCompaction)
.then(|| encrypted_parent_compaction(parent_history.raw_items()))
.flatten();
let parent_compaction = match encrypted_parent_compaction(
&parent_history,
&params.spawn_config.features,
params.compaction_model_hash.as_deref(),
) {
Ok(checkpoint) => checkpoint,
Err(error) => {
return (
GuardianReviewSessionOutcome::PromptBuildFailed(error),
GuardianReviewAnalyticsResult::without_session(),
);
}
};
let mut next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
&params.spawn_config,
params.parent_session.user_instructions().await,
@@ -581,7 +624,10 @@ impl GuardianReviewSessionManager {
.await
{
Ok(mut state) => {
if parent_compaction.is_none()
if !params
.parent_session
.enabled(Feature::GuardianThreadContext)
&& parent_compaction.is_none()
&& let Some(trunk) = state.trunk.as_ref()
{
// Without a decryptable summary, the existing reviewer may
@@ -1,4 +1,6 @@
use super::*;
use codex_history::CodexHarnessMetadata;
use codex_history::ResponseItemEnvelope;
use codex_protocol::openai_models::AutoReviewMessages;
use codex_protocol::protocol::AgentStatus;
use codex_protocol::protocol::ErrorEvent;
@@ -115,6 +117,7 @@ async fn test_review_params() -> GuardianReviewSessionParams {
reasons: ApprovalRequestReasons::default(),
schema: super::super::prompt::guardian_output_schema(),
model,
compaction_model_hash: None,
reasoning_effort,
guardian_default_review_model_id: "codex-auto-review".to_string(),
guardian_catalog_contains_auto_review: true,
@@ -253,8 +256,18 @@ async fn guardian_review_session_config_change_invalidates_cached_session() {
);
}
#[test]
fn encrypted_parent_compaction_requires_original_item_id() {
#[test_case::test_case(true; "thread owned")]
#[test_case::test_case(false; "legacy")]
#[tokio::test]
async fn encrypted_parent_compaction_requires_original_item_id(thread_context_enabled: bool) {
let (session, _) = crate::session::tests::make_session_and_context().await;
let mut features = session.get_config().await.features.clone();
features
.enable(Feature::GuardianReuseParentCompaction)
.expect("legacy reuse");
features
.set_enabled(Feature::GuardianThreadContext, thread_context_enabled)
.expect("context mode");
let item = ResponseItem::Compaction {
id: Some(codex_protocol::ResponseItemId::from_server(
"cmp_guardian_parent_summary".to_string(),
@@ -263,18 +276,36 @@ fn encrypted_parent_compaction_requires_original_item_id() {
internal_chat_message_metadata_passthrough: None,
};
let mut history = ContextManager::new();
history.replace_annotated(vec![ResponseItemEnvelope {
item: item.clone(),
metadata: Some(CodexHarnessMetadata {
compaction_model_hash: Some("compatible".to_owned()),
..Default::default()
}),
}]);
assert_eq!(
encrypted_parent_compaction(std::slice::from_ref(&item)),
encrypted_parent_compaction(&history, &features, Some("compatible"))
.expect("valid checkpoint"),
Some(item)
);
assert_eq!(
encrypted_parent_compaction(&[ResponseItem::Compaction {
// The latest unusable checkpoint must not fall back to the older valid one.
let mut items = history.annotated_items().to_vec();
items.push(
ResponseItem::Compaction {
id: None,
encrypted_content: "encrypted guardian parent summary".to_string(),
internal_chat_message_metadata_passthrough: None,
}]),
None
}
.into(),
);
history.replace_annotated(items);
let result = encrypted_parent_compaction(&history, &features, Some("compatible"));
if thread_context_enabled {
assert!(result.is_err());
} else {
assert_eq!(result.expect("legacy omission"), None);
}
}
#[tokio::test]
+1 -4
View File
@@ -1591,10 +1591,7 @@ impl Session {
);
state
.history
.restore_guardian_history(guardian_history.as_ref());
state
.history
.restore_retained_context(Some(&retained_context));
.restore_review_context(Some(&retained_context), guardian_history.as_ref());
if let Some(world_state) = world_state_baseline {
state.history.set_world_state_baseline(world_state);
}
@@ -347,8 +347,10 @@ impl Session {
&& let Some(items) = &checkpoint.compacted.replacement_history
{
history.replace_annotated(items.clone());
history.restore_guardian_history(checkpoint.compacted.guardian_history.as_ref());
history.restore_retained_context(checkpoint.compacted.retained_context.as_ref());
history.restore_review_context(
checkpoint.compacted.retained_context.as_ref(),
checkpoint.compacted.guardian_history.as_ref(),
);
}
// Materialize exact history semantics from the replay-derived suffix. The eventual lazy
// design should keep this same replay shape, but drive it from a resumable reverse source