diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 9790905468..d7b52ff6ef 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -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" }, diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index f84a1d8c65..804d2f1d8d 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -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, model_provider_id: String, model_provider: ModelProviderInfo, @@ -184,8 +184,17 @@ impl GuardianReviewSessionReuseKey { fn from_spawn_config( spawn_config: &Config, user_instructions: Option, + 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 { + 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, @@ -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( ¶ms.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, fork_snapshot: Option, ) -> (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, fork_snapshot: Option, ) -> anyhow::Result { 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( ¶ms.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 { diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index e3a2b9526f..6937d9164d 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -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() diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index d0b1847977..7f08f62643 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -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",