Reuse parent compactions in Guardian review sessions (#37513)

## What changed

- Add the under-development `guardian_reuse_parent_compaction` feature.
- Restart Guardian review sessions after parent history rewrites and seed them with the latest encrypted compaction that has a response item ID.
- Keep the existing reviewer when a rewritten history has no reusable compaction, preserving authorization and restriction context held by that session.

## Testing

- Cover reuse-key invalidation and compaction eligibility.
- Verify review-session behavior across parent compaction and a subsequent summary-free history reset.

GitOrigin-RevId: 891805d3c3dca34ddda6e3bfc5097be4ff164267
This commit is contained in:
Won Park
2026-08-07 23:49:12 +00:00
committed by copyberry
parent 208f05b233
commit c2bcb9a26b
4 changed files with 260 additions and 33 deletions
+6
View File
@@ -570,6 +570,9 @@
"guardian_approval": {
"type": "boolean"
},
"guardian_reuse_parent_compaction": {
"type": "boolean"
},
"guardianv2": {
"type": "boolean"
},
@@ -5235,6 +5238,9 @@
"guardian_approval": {
"type": "boolean"
},
"guardian_reuse_parent_compaction": {
"type": "boolean"
},
"guardianv2": {
"type": "boolean"
},
+135 -5
View File
@@ -155,9 +155,9 @@ struct GuardianReviewForkSnapshot {
#[derive(Debug, Clone, PartialEq)]
struct GuardianReviewSessionReuseKey {
// Only include settings that affect spawned-session behavior so reuse
// invalidation remains explicit and does not depend on unrelated config
// bookkeeping.
// Only include settings that affect spawned-session behavior and parent
// history rewrites that invalidate existing reviewer context.
parent_history_version: u64,
model: Option<String>,
model_provider_id: String,
model_provider: ModelProviderInfo,
@@ -184,8 +184,17 @@ impl GuardianReviewSessionReuseKey {
fn from_spawn_config(
spawn_config: &Config,
user_instructions: Option<UserInstructions>,
parent_history_version: u64,
) -> Self {
Self {
parent_history_version: if spawn_config
.features
.enabled(Feature::GuardianReuseParentCompaction)
{
parent_history_version
} else {
0
},
model: spawn_config.model.clone(),
model_provider_id: spawn_config.model_provider_id.clone(),
model_provider: spawn_config.model_provider.clone(),
@@ -210,6 +219,29 @@ impl GuardianReviewSessionReuseKey {
}
}
fn encrypted_parent_compaction(items: &[ResponseItem]) -> Option<ResponseItem> {
let item = items.iter().rev().find(|item| {
matches!(
item,
ResponseItem::Compaction { .. } | ResponseItem::ContextCompaction { .. }
)
})?;
match item {
ResponseItem::Compaction {
id: Some(_),
encrypted_content,
..
} if !encrypted_content.is_empty() => Some(item.clone()),
ResponseItem::ContextCompaction {
id: Some(_),
encrypted_content: Some(encrypted_content),
..
} if !encrypted_content.is_empty() => Some(item.clone()),
_ => None,
}
}
pub(crate) fn prompt_cache_key_override_for_review_session(
session_source: &SessionSource,
parent_thread_id: Option<ThreadId>,
@@ -311,9 +343,16 @@ impl GuardianReviewSessionManager {
let spawn_config = guardian_review_session_config(&parent_session, &parent_turn)
.await?
.spawn_config;
let parent_history = parent_session.clone_history().await;
let parent_compaction = spawn_config
.features
.enabled(Feature::GuardianReuseParentCompaction)
.then(|| encrypted_parent_compaction(parent_history.raw_items()))
.flatten();
let reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
&spawn_config,
parent_session.user_instructions().await,
parent_history.history_version(),
);
let spawn_cancel_token = self.cancellation_token.child_token();
let spawn_cancel_guard = spawn_cancel_token.clone().drop_guard();
@@ -323,6 +362,7 @@ impl GuardianReviewSessionManager {
spawn_config,
reuse_key,
spawn_cancel_token.clone(),
parent_compaction,
/*fork_snapshot*/ None,
)
.await?;
@@ -375,9 +415,17 @@ impl GuardianReviewSessionManager {
params: GuardianReviewSessionParams,
) -> (GuardianReviewSessionOutcome, GuardianReviewAnalyticsResult) {
let deadline = params.deadline;
let next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
let parent_history = params.parent_session.clone_history().await;
let parent_compaction = params
.spawn_config
.features
.enabled(Feature::GuardianReuseParentCompaction)
.then(|| encrypted_parent_compaction(parent_history.raw_items()))
.flatten();
let mut next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
&params.spawn_config,
params.parent_session.user_instructions().await,
parent_history.history_version(),
);
let mut stale_trunk_to_shutdown = None;
let mut spawned_trunk = false;
@@ -389,6 +437,13 @@ impl GuardianReviewSessionManager {
.await
{
Ok(mut state) => {
if parent_compaction.is_none()
&& let Some(trunk) = state.trunk.as_ref()
{
// Without a decryptable summary, the existing reviewer may
// hold the only remaining authorization or restriction.
next_reuse_key.parent_history_version = trunk.reuse_key.parent_history_version;
}
if let Some(trunk) = state.trunk.as_ref()
&& trunk.reuse_key != next_reuse_key
&& trunk.review_lock.try_acquire().is_ok()
@@ -408,6 +463,7 @@ impl GuardianReviewSessionManager {
params.spawn_config.clone(),
next_reuse_key.clone(),
spawn_cancel_token.clone(),
parent_compaction.clone(),
/*fork_snapshot*/ None,
)),
)
@@ -453,6 +509,7 @@ impl GuardianReviewSessionManager {
params,
next_reuse_key,
deadline,
parent_compaction,
/*fork_snapshot*/ None,
))
.await;
@@ -465,6 +522,7 @@ impl GuardianReviewSessionManager {
params,
next_reuse_key,
deadline,
parent_compaction,
trunk.fork_snapshot().await,
))
.await;
@@ -503,6 +561,7 @@ impl GuardianReviewSessionManager {
let reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
session.get_config().await.as_ref(),
session.user_instructions().await,
session.clone_history().await.history_version(),
);
self.state.lock().await.trunk = Some(Arc::new(GuardianReviewSession {
reuse_key,
@@ -523,6 +582,7 @@ impl GuardianReviewSessionManager {
let reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
session.get_config().await.as_ref(),
session.user_instructions().await,
session.clone_history().await.history_version(),
);
self.state
.lock()
@@ -606,6 +666,7 @@ impl GuardianReviewSessionManager {
params: GuardianReviewSessionParams,
reuse_key: GuardianReviewSessionReuseKey,
deadline: tokio::time::Instant,
parent_compaction: Option<ResponseItem>,
fork_snapshot: Option<GuardianReviewForkSnapshot>,
) -> (GuardianReviewSessionOutcome, GuardianReviewAnalyticsResult) {
let spawn_cancel_token = self.cancellation_token.child_token();
@@ -621,6 +682,7 @@ impl GuardianReviewSessionManager {
fork_config,
reuse_key,
spawn_cancel_token.clone(),
parent_compaction,
fork_snapshot,
)),
)
@@ -663,6 +725,7 @@ async fn spawn_guardian_review_session(
spawn_config: Config,
reuse_key: GuardianReviewSessionReuseKey,
cancel_token: CancellationToken,
parent_compaction: Option<ResponseItem>,
fork_snapshot: Option<GuardianReviewForkSnapshot>,
) -> anyhow::Result<GuardianReviewSession> {
let (initial_history, prior_review_count, initial_transcript_cursor) = match fork_snapshot {
@@ -671,7 +734,12 @@ async fn spawn_guardian_review_session(
fork_snapshot.prior_review_count,
fork_snapshot.last_reviewed_transcript_cursor,
),
None => (None, 0, None),
None => (
parent_compaction
.map(|item| InitialHistory::Forked(vec![RolloutItem::ResponseItem(item)])),
0,
None,
),
};
let (session, io) = Box::pin(run_codex_thread_interactive(
spawn_config,
@@ -1163,6 +1231,7 @@ mod tests {
let reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
session.get_config().await.as_ref(),
session.user_instructions().await,
session.clone_history().await.history_version(),
);
(
@@ -1303,6 +1372,7 @@ mod tests {
let cached_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
&cached_spawn_config,
/*user_instructions*/ None,
/*parent_history_version*/ 0,
);
let mut changed_parent_config = parent_config;
@@ -1319,6 +1389,7 @@ mod tests {
let next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
&next_spawn_config,
/*user_instructions*/ None,
/*parent_history_version*/ 0,
);
assert_eq!(
@@ -1331,8 +1402,60 @@ mod tests {
GuardianReviewSessionReuseKey::from_spawn_config(
&cached_spawn_config,
/*user_instructions*/ None,
/*parent_history_version*/ 0,
)
);
assert_eq!(
cached_reuse_key,
GuardianReviewSessionReuseKey::from_spawn_config(
&cached_spawn_config,
/*user_instructions*/ None,
/*parent_history_version*/ 1,
)
);
let mut compaction_enabled_config = cached_spawn_config;
compaction_enabled_config
.features
.enable(Feature::GuardianReuseParentCompaction)
.expect("Guardian parent-compaction reuse should be configurable");
assert_ne!(
GuardianReviewSessionReuseKey::from_spawn_config(
&compaction_enabled_config,
/*user_instructions*/ None,
/*parent_history_version*/ 0,
),
GuardianReviewSessionReuseKey::from_spawn_config(
&compaction_enabled_config,
/*user_instructions*/ None,
/*parent_history_version*/ 1,
)
);
}
#[test]
fn encrypted_parent_compaction_requires_original_item_id() {
let item = ResponseItem::Compaction {
id: Some(codex_protocol::ResponseItemId::from_server(
"cmp_guardian_parent_summary".to_string(),
)),
encrypted_content: "encrypted guardian parent summary".to_string(),
internal_chat_message_metadata_passthrough: None,
};
assert_eq!(
encrypted_parent_compaction(std::slice::from_ref(&item)),
Some(item)
);
assert_eq!(
encrypted_parent_compaction(&[ResponseItem::Compaction {
id: None,
encrypted_content: "encrypted guardian parent summary".to_string(),
internal_chat_message_metadata_passthrough: None,
}]),
None
);
}
#[tokio::test]
@@ -1389,6 +1512,7 @@ mod tests {
let cached_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
&cached_spawn_config,
/*user_instructions*/ None,
/*parent_history_version*/ 0,
);
let mut changed_parent_config = parent_config;
@@ -1405,6 +1529,7 @@ mod tests {
let next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
&next_spawn_config,
/*user_instructions*/ None,
/*parent_history_version*/ 0,
);
assert_ne!(cached_reuse_key, next_reuse_key);
@@ -1767,6 +1892,11 @@ mod tests {
review_session.reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
&params.spawn_config,
params.parent_session.user_instructions().await,
params
.parent_session
.clone_history()
.await
.history_version(),
);
let manager = GuardianReviewSessionManager {
state: Arc::new(Mutex::new(GuardianReviewSessionState {
+111 -28
View File
@@ -2053,15 +2053,27 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow:
),
ev_completed("resp-guardian-3"),
]),
sse(vec![
ev_response_created("resp-guardian-4"),
ev_assistant_message(
"msg-guardian-4",
"{\"risk_level\":\"low\",\"user_authorization\":\"high\",\"outcome\":\"allow\",\"rationale\":\"fourth guardian rationale\"}",
),
ev_completed("resp-guardian-4"),
]),
],
)
.await;
let (session, mut turn) = guardian_test_session_and_turn(&server).await;
Arc::get_mut(&mut turn)
.expect("turn should be unique")
.model_info
.auto_review_model_override = Some("codex-auto-review".to_string());
let mut config = (*turn.config).clone();
config
.features
.enable(Feature::GuardianReuseParentCompaction)
.expect("Guardian parent-compaction reuse should be configurable");
let turn_mut = Arc::get_mut(&mut turn).expect("turn should be unique");
turn_mut.model_info.auto_review_model_override = Some("codex-auto-review".to_string());
turn_mut.config = Arc::new(config);
seed_guardian_parent_history(&session, &turn).await;
let first_request = GuardianApprovalRequest::Shell {
@@ -2135,10 +2147,32 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow:
/*max_attempts*/ 1,
)
.await;
let committed_rollout_items = session
.guardian_review_session
.committed_fork_rollout_items_for_test()
.await
.expect("committed guardian fork snapshot");
assert_eq!(
committed_rollout_items
.iter()
.filter(|item| rollout_item_contains_message_text(
item,
"Use prior reviews as context, not binding precedent."
))
.count(),
1,
"follow-up reminder should be persisted for guardian forks"
);
session
.record_conversation_items(
turn.as_ref(),
&[
.replace_history(
vec![
ResponseItem::Compaction {
id: Some(codex_protocol::ResponseItemId::from_server(
"cmp_guardian_parent_summary".to_string(),
)),
encrypted_content: "encrypted guardian parent summary".to_string(),
internal_chat_message_metadata_passthrough: None,
},
ResponseItem::Message {
id: None,
role: "user".to_string(),
@@ -2158,6 +2192,7 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow:
internal_chat_message_metadata_passthrough: None,
},
],
/*reference_context_item*/ None,
)
.await;
let third_request = GuardianApprovalRequest::Shell {
@@ -2181,6 +2216,30 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow:
/*max_attempts*/ 1,
)
.await;
session
.replace_history(
vec![ResponseItem::Message {
id: None,
role: "user".to_string(),
content: vec![ContentItem::InputText {
text: "Please review after a summary-free context reset.".to_string(),
}],
phase: None,
internal_chat_message_metadata_passthrough: None,
}],
/*reference_context_item*/ None,
)
.await;
let fourth_outcome = run_guardian_review_session_for_test(
Arc::clone(&session),
Arc::clone(&turn),
guardian_shell_request("shell-4"),
ApprovalRequestReasons::default(),
guardian_output_schema(),
/*external_cancel*/ None,
/*max_attempts*/ 1,
)
.await;
let (GuardianReviewOutcome::Completed(first_assessment), first_metadata) = first_outcome else {
panic!("expected first guardian assessment");
@@ -2192,9 +2251,14 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow:
let (GuardianReviewOutcome::Completed(third_assessment), third_metadata) = third_outcome else {
panic!("expected third guardian assessment");
};
let (GuardianReviewOutcome::Completed(fourth_assessment), fourth_metadata) = fourth_outcome
else {
panic!("expected fourth guardian assessment");
};
assert_eq!(first_assessment.outcome, GuardianAssessmentOutcome::Allow);
assert_eq!(second_assessment.outcome, GuardianAssessmentOutcome::Allow);
assert_eq!(third_assessment.outcome, GuardianAssessmentOutcome::Allow);
assert_eq!(fourth_assessment.outcome, GuardianAssessmentOutcome::Allow);
assert!(matches!(
first_metadata.guardian_session_kind,
Some(codex_analytics::GuardianReviewSessionKind::TrunkNew)
@@ -2205,6 +2269,10 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow:
));
assert!(matches!(
third_metadata.guardian_session_kind,
Some(codex_analytics::GuardianReviewSessionKind::TrunkNew)
));
assert!(matches!(
fourth_metadata.guardian_session_kind,
Some(codex_analytics::GuardianReviewSessionKind::TrunkReused)
));
ThreadId::from_string(
@@ -2230,26 +2298,49 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow:
.expect("third guardian thread id should be a valid UUID");
assert_eq!(first_metadata.had_prior_review_context, Some(false));
assert_eq!(second_metadata.had_prior_review_context, Some(true));
assert_eq!(third_metadata.had_prior_review_context, Some(true));
assert_eq!(third_metadata.had_prior_review_context, Some(false));
assert_eq!(fourth_metadata.had_prior_review_context, Some(true));
assert_eq!(
first_metadata.guardian_thread_id,
second_metadata.guardian_thread_id
);
assert_eq!(
assert_ne!(
second_metadata.guardian_thread_id,
third_metadata.guardian_thread_id
);
assert_eq!(
third_metadata.guardian_thread_id,
fourth_metadata.guardian_thread_id
);
let requests = request_log.requests();
assert_eq!(requests.len(), 3);
assert_eq!(requests.len(), 4);
let first_body = requests[0].body_json();
let second_body = requests[1].body_json();
let third_body = requests[2].body_json();
let fourth_body = requests[3].body_json();
let third_input = third_body["input"]
.as_array()
.expect("guardian review should include input items");
assert!(third_input.iter().any(|item| {
item["type"] == "compaction"
&& item["id"] == "cmp_guardian_parent_summary"
&& item["encrypted_content"] == "encrypted guardian parent summary"
}));
assert_eq!(
first_body["prompt_cache_key"],
second_body["prompt_cache_key"]
);
assert_eq!(
second_body["prompt_cache_key"],
third_body["prompt_cache_key"]
);
assert_eq!(
third_body["prompt_cache_key"],
fourth_body["prompt_cache_key"]
);
assert!(fourth_body.to_string().contains("third guardian rationale"));
assert!(
second_body.to_string().contains(concat!(
"Use prior reviews as context, not binding precedent. ",
@@ -2269,25 +2360,17 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow:
.to_string()
.matches("Use prior reviews as context, not binding precedent.")
.count(),
1,
"later follow-up guardian requests should not append the reminder again"
);
let committed_rollout_items = session
.guardian_review_session
.committed_fork_rollout_items_for_test()
.await
.expect("committed guardian fork snapshot");
assert_eq!(
committed_rollout_items
.iter()
.filter(|item| rollout_item_contains_message_text(
item,
"Use prior reviews as context, not binding precedent."
))
.count(),
1,
"follow-up reminder should be persisted for guardian forks"
0,
"a fresh guardian session should not inherit the follow-up reminder"
);
let third_user_message = requests[2]
.message_input_text_groups("user")
.last()
.expect("fresh guardian user message")
.join("");
assert!(third_user_message.contains(">>> TRANSCRIPT START\n"));
assert!(third_user_message.contains("Please push the third docs fix too."));
assert!(!third_body.to_string().contains(first_rationale));
let second_user_message = requests[1]
.message_input_text_groups("user")
.last()
+8
View File
@@ -247,6 +247,8 @@ pub enum Feature {
DefaultModeRequestUserInput,
/// Enable automatic review for approval prompts.
GuardianApproval,
/// Reuse encrypted parent compaction when restarting Guardian review sessions.
GuardianReuseParentCompaction,
/// Enable Guardian V2 automatic approval reviews.
GuardianV2,
/// Enable persisted thread goals and automatic goal continuation.
@@ -1365,6 +1367,12 @@ pub const FEATURES: &[FeatureSpec] = &[
stage: Stage::Stable,
default_enabled: true,
},
FeatureSpec {
id: Feature::GuardianReuseParentCompaction,
key: "guardian_reuse_parent_compaction",
stage: Stage::UnderDevelopment,
default_enabled: false,
},
FeatureSpec {
id: Feature::GuardianV2,
key: "guardianv2",