From 7b66c21f580e2695d0345f62b4f860b1915a182e Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Wed, 23 Sep 2026 22:33:45 +0000 Subject: [PATCH] Remove obsolete Guardian context capture mode branches (#47690) ## What changed Remove the session-level capture mode and make thread-owned retained context the default throughout history management. Simplify input ordering, compaction identity preservation, fork handling, and approval freshness checks by removing legacy capture branches. Keep checkpoint compatibility review separate: unknown or incompatible checkpoints can still use legacy review while retaining user evidence. Set the already-removed `guardianv2.thread_context` feature's default to false and remove redundant opt-out test variants. ## Testing Update checkpoint tests to exercise legacy review through restored checkpoint data. Adapt existing compaction, resume, rollback, and fork tests to use unconditional context capture. GitOrigin-RevId: ab23a0d7aca71d5a103458cff8b0c68137dd9708 --- .../suite/v2/guardian_v2_history_tests.rs | 188 +++++------------- codex-rs/core/src/agent/control/spawn.rs | 25 +-- codex-rs/core/src/agent/control_tests.rs | 14 +- codex-rs/core/src/compact.rs | 20 +- codex-rs/core/src/compact_tests.rs | 2 +- .../core/src/context/guardian_context_mode.rs | 9 +- codex-rs/core/src/context_manager/history.rs | 77 +++---- .../core/src/context_manager/history_tests.rs | 40 +--- .../history_user_authorization.rs | 38 ++-- codex-rs/core/src/guardian/review.rs | 1 - codex-rs/core/src/guardian/review_request.rs | 21 +- .../src/session/guardian_checkpoint_tests.rs | 20 +- codex-rs/core/src/session/mod.rs | 72 +++---- codex-rs/core/src/session/retained_context.rs | 16 +- .../src/session/rollout_reconstruction.rs | 18 +- .../session/rollout_reconstruction_tests.rs | 21 +- codex-rs/core/src/session/session.rs | 8 +- codex-rs/core/src/session/tests.rs | 15 +- codex-rs/core/src/session/turn_input.rs | 18 +- codex-rs/core/src/state/turn.rs | 4 +- .../src/tools/handlers/request_user_input.rs | 9 +- .../src/async_scorer/extension_tests.rs | 43 +--- codex-rs/features/src/lib.rs | 2 +- codex-rs/features/src/tests.rs | 1 - 24 files changed, 213 insertions(+), 469 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2_history_tests.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2_history_tests.rs index 4045c66936..808825c13f 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2_history_tests.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2_history_tests.rs @@ -64,12 +64,6 @@ enum EvidenceSize { OversizedInstruction, } -#[derive(Clone, Copy)] -enum ContextPath { - Legacy, - ThreadOwned, -} - #[derive(Clone, Copy)] enum CheckpointReuse { Enabled, @@ -85,30 +79,22 @@ enum ReviewCheckpoint { EmptyReviewerHash, } -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::EmptyContent; "empty checkpoint fails closed")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::DifferentReviewerHash; "different sync hash preserves retained evidence")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::UnknownReviewer; "unknown sync hash preserves retained evidence")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::EmptyReviewerHash; "empty sync hash preserves retained evidence")] -#[test_case(ContextPath::Legacy, CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::DifferentReviewerHash; "ignored opt-out preserves sync compatibility")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Disabled, Some("matching"), Some("matching"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "disabled Luna reuse requires sync")] -#[test_case(ContextPath::Legacy, CheckpointReuse::Disabled, Some("matching"), Some("matching"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "ignored opt-out with disabled Luna reuse requires sync")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some("matching"), Some("matching"), 0, EvidenceSize::OversizedInstruction, ReviewCheckpoint::Valid; "instruction budget preserves fresh low score")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some("matching"), Some("matching"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "compatible checkpoint")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "incompatible checkpoint")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some("matching"), None, 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "unknown Luna compatibility")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some("matching"), Some(""), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "empty Luna compatibility")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, None, Some("matching"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "unknown producer preserves retained evidence")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some(""), Some("matching"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "empty producer preserves retained evidence")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some("matching"), Some("matching"), 140, EvidenceSize::Normal, ReviewCheckpoint::Valid; "source call evicted")] -#[test_case(ContextPath::ThreadOwned, CheckpointReuse::Enabled, Some("matching"), Some("matching"), 0, EvidenceSize::OversizedAnswer, ReviewCheckpoint::Valid; "incomplete answers reject fresh low score")] -#[test_case(ContextPath::Legacy, CheckpointReuse::Enabled, Some("matching"), Some("matching"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "ignored opt-out retains answers across resume")] -#[test_case(ContextPath::Legacy, CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "ignored opt-out with incompatible checkpoint requires sync")] -#[test_case(ContextPath::Legacy, CheckpointReuse::Enabled, Some("matching"), None, 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "ignored opt-out with unknown Luna compatibility requires sync")] -#[test_case(ContextPath::Legacy, CheckpointReuse::Enabled, Some("matching"), Some("matching"), 140, EvidenceSize::Normal, ReviewCheckpoint::Valid; "ignored opt-out preserves answers after source call eviction")] -#[test_case(ContextPath::Legacy, CheckpointReuse::Enabled, Some("matching"), Some("matching"), 0, EvidenceSize::OversizedAnswer, ReviewCheckpoint::Valid; "ignored opt-out rejects incomplete answers")] +#[test_case(CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::EmptyContent; "empty checkpoint fails closed")] +#[test_case(CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::DifferentReviewerHash; "different sync hash preserves retained evidence")] +#[test_case(CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::UnknownReviewer; "unknown sync hash preserves retained evidence")] +#[test_case(CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::EmptyReviewerHash; "empty sync hash preserves retained evidence")] +#[test_case(CheckpointReuse::Disabled, Some("matching"), Some("matching"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "disabled Luna reuse requires sync")] +#[test_case(CheckpointReuse::Enabled, Some("matching"), Some("matching"), 0, EvidenceSize::OversizedInstruction, ReviewCheckpoint::Valid; "instruction budget preserves fresh low score")] +#[test_case(CheckpointReuse::Enabled, Some("matching"), Some("matching"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "compatible checkpoint")] +#[test_case(CheckpointReuse::Enabled, Some("matching"), Some("different"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "incompatible checkpoint")] +#[test_case(CheckpointReuse::Enabled, Some("matching"), None, 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "unknown Luna compatibility")] +#[test_case(CheckpointReuse::Enabled, Some("matching"), Some(""), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "empty Luna compatibility")] +#[test_case(CheckpointReuse::Enabled, None, Some("matching"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "unknown producer preserves retained evidence")] +#[test_case(CheckpointReuse::Enabled, Some(""), Some("matching"), 0, EvidenceSize::Normal, ReviewCheckpoint::Valid; "empty producer preserves retained evidence")] +#[test_case(CheckpointReuse::Enabled, Some("matching"), Some("matching"), 140, EvidenceSize::Normal, ReviewCheckpoint::Valid; "source call evicted")] +#[test_case(CheckpointReuse::Enabled, Some("matching"), Some("matching"), 0, EvidenceSize::OversizedAnswer, ReviewCheckpoint::Valid; "incomplete answers reject fresh low score")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn guardians_retain_evidence_after_compaction_and_resume( - requested_context_path: ContextPath, checkpoint_reuse: CheckpointReuse, parent_hash: Option<&str>, luna_hash: Option<&str>, @@ -118,16 +104,13 @@ async fn guardians_retain_evidence_after_compaction_and_resume( ) -> Result<()> { skip_if_no_network!(Ok(())); - // Both values of the retired config setting use thread-owned context. - let context_path = ContextPath::ThreadOwned; - const RESTRICTION: &str = "Only publish to a private repository."; const EVIDENCE: &str = "repository is private"; const SUMMARY: &str = "Repository inspection was summarized."; let reuse_parent_compaction = matches!(checkpoint_reuse, CheckpointReuse::Enabled); let compatible = reuse_parent_compaction && parent_hash == Some("matching") && luna_hash == parent_hash; - let requires_sync = matches!(context_path, ContextPath::ThreadOwned) && !compatible; + let requires_sync = !compatible; let oversized_instruction = matches!(evidence_size, EvidenceSize::OversizedInstruction); let reviewer_hash = match review_checkpoint { ReviewCheckpoint::DifferentReviewerHash => Some("different-reviewer"), @@ -136,8 +119,7 @@ async fn guardians_retain_evidence_after_compaction_and_resume( ReviewCheckpoint::Valid | ReviewCheckpoint::EmptyContent => Some("matching"), }; // Sync review accepts any hash metadata; only the checkpoint payload must be usable. - let reject_sync_checkpoint = matches!(context_path, ContextPath::ThreadOwned) - && matches!(review_checkpoint, ReviewCheckpoint::EmptyContent); + let reject_sync_checkpoint = matches!(review_checkpoint, ReviewCheckpoint::EmptyContent); let answer = match evidence_size { EvidenceSize::Normal | EvidenceSize::OversizedInstruction => { USER_INPUT_RESTRICTION.to_owned() @@ -165,10 +147,7 @@ async fn guardians_retain_evidence_after_compaction_and_resume( | ReviewCheckpoint::UnknownReviewer | ReviewCheckpoint::EmptyReviewerHash => {} } - let rejects_incomplete_score = matches!( - (context_path, evidence_size), - (ContextPath::ThreadOwned, EvidenceSize::OversizedAnswer) - ); + let rejects_incomplete_score = matches!(evidence_size, EvidenceSize::OversizedAnswer); let classifier = Arc::new(MockResponsesState { luna_score: if rejects_incomplete_score || oversized_instruction || requires_sync { 0.0 @@ -273,11 +252,7 @@ async fn guardians_retain_evidence_after_compaction_and_resume( }); let (mcp_url, mcp_server) = start_mcp_server(/*sensitive_action*/ None).await?; let codex_home = TempDir::new()?; - let thread_context_enabled = match requested_context_path { - ContextPath::Legacy => false, - ContextPath::ThreadOwned => true, - }; - let mut mock_config = MockResponsesConfig::new(&responses_url) + let mock_config = MockResponsesConfig::new(&responses_url) .with_provider_name("OpenAI") .with_provider_config("requires_openai_auth = true\nsupports_websockets = false") .with_root_config("approvals_reviewer = \"auto_review\"\nmodel_auto_compact_token_limit = 1000000") @@ -285,12 +260,10 @@ async fn guardians_retain_evidence_after_compaction_and_resume( .enable_feature(Feature::GuardianApproval) .disable_feature(Feature::EnableRequestCompression) .disable_feature(Feature::TokenBudget) + .disable_feature(Feature::GuardianReuseParentCompaction) .with_extra_config(&format!( - "[mcp_servers.{TEST_SERVER_NAME}]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"prompt\"\n\n[features.guardianv2]\nenabled = true\nthread_context = {thread_context_enabled}\npersist_scores = true\nreuse_parent_compaction = {reuse_parent_compaction}\n\n[features.guardianv2.review_scope]\ncomputer_use_only = false" + "[mcp_servers.{TEST_SERVER_NAME}]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"prompt\"\n\n[features.guardianv2]\nenabled = true\npersist_scores = true\nreuse_parent_compaction = {reuse_parent_compaction}\n\n[features.guardianv2.review_scope]\ncomputer_use_only = false" )); - if matches!(context_path, ContextPath::ThreadOwned) { - mock_config = mock_config.disable_feature(Feature::GuardianReuseParentCompaction); - } mock_config.write(codex_home.path())?; let config = load_default_config_for_test(&codex_home).await; let models = [ @@ -587,16 +560,10 @@ async fn guardians_retain_evidence_after_compaction_and_resume( })); { assert!(sync_text.contains(RESTRICTION)); - if matches!(context_path, ContextPath::ThreadOwned) { - assert!( - !sync_text.contains(&expected_output), - "raw tool result must not survive the parent checkpoint" - ); - } else if tool_traffic == 0 { - assert!(sync_text.contains(EVIDENCE)); - assert!(sync_text.contains(&format!("tool {TEST_TOOL_NAME} result:"))); - assert!(sync_text.contains(&expected_output)); - } + assert!( + !sync_text.contains(&expected_output), + "raw tool result must not survive the parent checkpoint" + ); assert!(sync_text.contains(">>> TRANSCRIPT START")); assert!(!sync_text.contains(">>> TRANSCRIPT DELTA START")); // The endpoint receives the original evidence. The returned checkpoint is @@ -619,61 +586,31 @@ async fn guardians_retain_evidence_after_compaction_and_resume( "{compact_output}" ); assert!(parent_items.contains(&checkpoint)); - if matches!(context_path, ContextPath::ThreadOwned) { - assert!( - content.contains(RESTRICTION), - "retained instruction must survive" - ); - assert!( - !transcript.contains(&expected_output), - "raw tool result must not survive the parent checkpoint" - ); - assert!(transcript.contains(RESTRICTION)); - assert!(!transcript.contains(SUMMARY)); - } else { - assert!( - transcript.contains(RESTRICTION), - "retained user restriction missing: {transcript}" - ); - if tool_traffic == 0 { - assert!(transcript.contains(&format!("tool {TEST_TOOL_NAME} call:"))); - assert!(transcript.contains(&format!("tool {TEST_TOOL_NAME} result:"))); - assert!(transcript.contains(&expected_output)); - } else { - assert!(!transcript.contains("tool request_user_input call:")); - } - } + assert!( + content.contains(RESTRICTION), + "retained instruction must survive" + ); + assert!( + !transcript.contains(&expected_output), + "raw tool result must not survive the parent checkpoint" + ); + assert!(transcript.contains(RESTRICTION)); + assert!(!transcript.contains(SUMMARY)); } } for (consumer, text) in [("async", &content), ("sync", &sync_text)] { - if matches!(context_path, ContextPath::ThreadOwned) || index == 0 { - let answers = text - .split_once(">>> TRUSTED USER ANSWERS START") - .unwrap_or_else(|| { - panic!("missing {consumer} answers at step {index}: {text}") - }) - .1; - match evidence_size { - EvidenceSize::Normal | EvidenceSize::OversizedInstruction => { - assert!(answers.contains("assistant: Can I keep using the browser?")); - assert!(answers.contains(&format!("user: {USER_INPUT_RESTRICTION}"))); - } - EvidenceSize::OversizedAnswer => match context_path { - ContextPath::ThreadOwned => { - assert!( - answers.contains("some verified user answers are unavailable") - ); - } - ContextPath::Legacy => { - assert!(answers.contains(">> TRUSTED USER ANSWERS START") + .unwrap_or_else(|| panic!("missing {consumer} answers at step {index}: {text}")) + .1; + match evidence_size { + EvidenceSize::Normal | EvidenceSize::OversizedInstruction => { + assert!(answers.contains("assistant: Can I keep using the browser?")); + assert!(answers.contains(&format!("user: {USER_INPUT_RESTRICTION}"))); + } + EvidenceSize::OversizedAnswer => { + assert!(answers.contains("some verified user answers are unavailable")); } - } else { - assert!(!text.contains(">>> TRUSTED USER ANSWERS START")); } } } @@ -739,14 +676,12 @@ async fn guardians_retain_evidence_after_compaction_and_resume( app_server.shutdown_gracefully().await?; let rollout = std::fs::read_to_string(thread.path.as_ref().expect("saved rollout path"))?; - if matches!(context_path, ContextPath::ThreadOwned) { - for line in rollout - .lines() - .filter_map(|line| serde_json::from_str::(line).ok()) - { - if line["type"] == "compacted" { - assert!(line["payload"]["guardian_history"].is_null()); - } + for line in rollout + .lines() + .filter_map(|line| serde_json::from_str::(line).ok()) + { + if line["type"] == "compacted" { + assert!(line["payload"]["guardian_history"].is_null()); } } let items = rollout @@ -758,8 +693,8 @@ async fn guardians_retain_evidence_after_compaction_and_resume( .iter() .filter(|line| matches!(line.item, RolloutItem::RetainedContext(_))) .count(), - usize::from(matches!(context_path, ContextPath::ThreadOwned)), - "only the enabled path may persist a retained-answer event", + 1, + "verified answers must be persisted", ); for line in &items { if let RolloutItem::Compacted(checkpoint) = &line.item { @@ -780,22 +715,7 @@ async fn guardians_retain_evidence_after_compaction_and_resume( } } } - if matches!(context_path, ContextPath::Legacy) { - for line in &items { - if let RolloutItem::Compacted(checkpoint) = &line.item { - assert_eq!( - checkpoint - .retained_context - .as_ref() - .expect("retained context checkpoint") - .verified_answers() - .count(), - 0, - "flag-off compaction must not populate retained answers", - ); - } - } - } + mcp_server.abort(); responses_server.abort(); Ok(()) diff --git a/codex-rs/core/src/agent/control/spawn.rs b/codex-rs/core/src/agent/control/spawn.rs index 3db203f3bf..7fe302678d 100644 --- a/codex-rs/core/src/agent/control/spawn.rs +++ b/codex-rs/core/src/agent/control/spawn.rs @@ -15,7 +15,6 @@ use crate::context::ContextualUserFragment; use crate::context::CurrentTimeReminder; use crate::context::CurrentTimeUnavailable; use crate::context::DeveloperInstructions; -use crate::context::GuardianContextMode; use crate::context::ManagedDeveloperInstructions; use crate::context::MultiAgentModeInstructions; use crate::context::MultiAgentRoleInstructions; @@ -118,11 +117,7 @@ fn keep_forked_rollout_item(item: &RolloutItem, preserve_context_baselines: bool } } -fn retain_forked_developer_message( - item: &mut ResponseItem, - usage_hint_texts: &[String], - context_mode: GuardianContextMode, -) -> bool { +fn retain_forked_developer_message(item: &mut ResponseItem, usage_hint_texts: &[String]) -> bool { if !matches!(item, ResponseItem::Message { role, .. } if role == "developer") { return true; } @@ -131,9 +126,7 @@ fn retain_forked_developer_message( return false; }; content.retain(|content_item| { - if context_mode == GuardianContextMode::ThreadOwned - && content_item.kind().0 == "guardian.approved_action" - { + if content_item.kind().0 == "guardian.approved_action" { return false; } let ContentItem::InputText { text } = content_item.content() else { @@ -141,10 +134,8 @@ fn retain_forked_developer_message( }; !(MultiAgentRoleInstructions::matches_text(text) - || (context_mode == GuardianContextMode::ThreadOwned - && text.starts_with( - crate::guardian::AUTO_REVIEW_DENIED_ACTION_APPROVAL_DEVELOPER_PREFIX, - )) + || text + .starts_with(crate::guardian::AUTO_REVIEW_DENIED_ACTION_APPROVAL_DEVELOPER_PREFIX) || MultiAgentModeInstructions::matches_text(text) || CurrentTimeReminder::matches_text(text) || CurrentTimeUnavailable::matches_text(text) @@ -980,14 +971,12 @@ impl LocalAgentControl { break; } } - let context_mode = GuardianContextMode::ThreadOwned; let mut replaced_parent_developer_instructions = false; // Scrub inherited hints and replace only the parent's developer-instruction fragment. // Compaction stores response items separately, so sanitize both top-level messages and // compacted replacement histories with the same policy. let retain_forked_item = |envelope: &mut ResponseItemEnvelope, replaced: &mut bool| { - if context_mode == GuardianContextMode::ThreadOwned - && multi_agent_version == MultiAgentVersion::V2 + if multi_agent_version == MultiAgentVersion::V2 && matches!(&envelope.item, ResponseItem::Message { role, .. } if role == "user" || role == "assistant") { // Persist the scope of every inherited conversational message, including the suffix @@ -1011,7 +1000,6 @@ impl LocalAgentControl { if !retain_forked_developer_message( response_item, &multi_agent_v2_usage_hint_texts_to_filter, - context_mode, ) { return false; } @@ -1097,8 +1085,7 @@ impl LocalAgentControl { compacted.guardian_history = None; // Only V2 fetches root authorization live. Its local scope starts known-empty; // V1 must remain incomplete when inherited authorization has been stripped. - compacted.retained_context = (context_mode == GuardianContextMode::ThreadOwned - && multi_agent_version == MultiAgentVersion::V2) + compacted.retained_context = (multi_agent_version == MultiAgentVersion::V2) .then(codex_history::RetainedContext::default); if let Some(replacement_history) = compacted.replacement_history.as_mut() { // Matches before this checkpoint cannot survive its replacement history. diff --git a/codex-rs/core/src/agent/control_tests.rs b/codex-rs/core/src/agent/control_tests.rs index ff3410b8d4..1357e6c3c1 100644 --- a/codex-rs/core/src/agent/control_tests.rs +++ b/codex-rs/core/src/agent/control_tests.rs @@ -1927,17 +1927,11 @@ async fn spawn_agent_without_fork_from_paginated_parent_stays_fresh_and_paginate .expect("parent shutdown should submit"); } -#[test_case::test_case(true; "thread context enabled")] -#[test_case::test_case(false; "thread context disabled")] #[tokio::test] -async fn spawn_agent_fork_sanitizes_inherited_compaction_metadata(thread_context_enabled: bool) { +async fn spawn_agent_fork_sanitizes_inherited_compaction_metadata() { let mut harness = AgentControlHarness::new().await; let _ = harness.config.features.disable(Feature::MultiAgentV2); - harness - .config - .features - .set_enabled(Feature::GuardianThreadContext, thread_context_enabled) - .expect("test context mode"); + let (parent_thread_id, parent_thread) = harness.start_paginated_thread().await; let parent_resume_metadata = codex_history::CompactionResumeMetadata { multi_agent_version: Some(MultiAgentVersion::V2), @@ -2178,9 +2172,6 @@ async fn spawn_agent_can_fork_parent_thread_history_with_sanitized_items() { let harness = AgentControlHarness::new().await; let mut parent_config = harness.config.clone(); let _ = parent_config.features.enable(Feature::MultiAgentV2); - let _ = parent_config - .features - .enable(Feature::GuardianThreadContext); parent_config.developer_instructions = Some("Parent developer instructions.".to_string()); parent_config.multi_agent_v2.root_agent_usage_hint_text = Some("Parent root guidance.".to_string()); @@ -2188,7 +2179,6 @@ async fn spawn_agent_can_fork_parent_thread_history_with_sanitized_items() { Some("Parent subagent guidance.".to_string()); let mut child_config = harness.config.clone(); let _ = child_config.features.enable(Feature::MultiAgentV2); - let _ = child_config.features.enable(Feature::GuardianThreadContext); child_config.developer_instructions = Some("Child developer instructions.".to_string()); child_config.multi_agent_v2.subagent_developer_instructions = Some("Child developer instructions.".to_string()); diff --git a/codex-rs/core/src/compact.rs b/codex-rs/core/src/compact.rs index c9abdaf716..f44a6bdf8d 100644 --- a/codex-rs/core/src/compact.rs +++ b/codex-rs/core/src/compact.rs @@ -1,4 +1,3 @@ -use crate::context::GuardianContextMode; use std::sync::Arc; use std::time::Instant; @@ -358,12 +357,7 @@ async fn run_compact_task_inner_impl( get_last_assistant_message_from_turn(history_snapshot.raw_items()).unwrap_or_default() }; let summary_text = format!("{SUMMARY_PREFIX}\n{summary_suffix}"); - let identity = if sess.guardian_context_mode == GuardianContextMode::ThreadOwned { - CompactedMessageIdentity::Preserve - } else { - CompactedMessageIdentity::Regenerate - }; - let user_messages = collect_annotated_user_messages(history_items, identity); + let user_messages = collect_annotated_user_messages(history_items); let mut new_history = build_compacted_history(Vec::new(), &user_messages, &summary_text); if let Some(summary_item) = new_history.last_mut() { @@ -552,24 +546,12 @@ pub(crate) fn collect_user_messages(items: &[ResponseItem]) -> Vec Vec { items .iter() .filter_map(|envelope| compacted_user_message(&envelope.item, envelope.metadata.clone())) - .map(|mut message| { - if matches!(identity, CompactedMessageIdentity::Regenerate) { - message.id = None; - } - message - }) .collect() } diff --git a/codex-rs/core/src/compact_tests.rs b/codex-rs/core/src/compact_tests.rs index 532d80ed5e..088d1234cf 100644 --- a/codex-rs/core/src/compact_tests.rs +++ b/codex-rs/core/src/compact_tests.rs @@ -309,7 +309,7 @@ fn collect_annotated_user_messages_extracts_user_text_only() { ResponseItemEnvelope::new(ResponseItem::Other), ]; - let collected = collect_annotated_user_messages(&items, CompactedMessageIdentity::Preserve); + let collected = collect_annotated_user_messages(&items); assert_eq!( vec![CompactedUserMessage { diff --git a/codex-rs/core/src/context/guardian_context_mode.rs b/codex-rs/core/src/context/guardian_context_mode.rs index 7bc1588f9e..2869cb5891 100644 --- a/codex-rs/core/src/context/guardian_context_mode.rs +++ b/codex-rs/core/src/context/guardian_context_mode.rs @@ -1,14 +1,14 @@ -//! Session capture policy and the reviewer policy carried by each history snapshot. +//! Reviewer policy carried by each history snapshot. //! Unknown or incompatible checkpoints keep legacy review alongside retained user evidence. use codex_extension_api::ConversationHistorySnapshot; use codex_history::ResponseItemEnvelope; -/// Selects legacy compatibility or thread-owned evidence. +/// Selects checkpoint compatibility review or thread-owned evidence. #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] pub enum GuardianContextMode { - #[default] Legacy, + #[default] ThreadOwned, } @@ -23,14 +23,13 @@ impl GuardianContextMode { } pub(crate) fn for_checkpoint( - self, items: &[ResponseItemEnvelope], reviewer_compaction_hash: Option<&str>, ) -> Self { if codex_history::CompactionCheckpoint::latest(items) .is_none_or(|checkpoint| checkpoint.is_compatible_with(reviewer_compaction_hash)) { - self + Self::ThreadOwned } else { Self::Legacy } diff --git a/codex-rs/core/src/context_manager/history.rs b/codex-rs/core/src/context_manager/history.rs index 4cb7dbe563..364d147117 100644 --- a/codex-rs/core/src/context_manager/history.rs +++ b/codex-rs/core/src/context_manager/history.rs @@ -84,8 +84,6 @@ pub(crate) struct ContextManager { review_history: Option, /// Host facts independent of the model window; snapshots share immutable state. retained_context: Arc, - /// Capture follows the session flag, including while an older checkpoint uses legacy review. - guardian_context_mode: GuardianContextMode, /// Reviewer policy travels with the history snapshot, independently of capture. guardian_review_mode: GuardianContextMode, retain_inherited_user_messages: bool, @@ -115,7 +113,6 @@ struct SharedConversationHistory { items: Arc>, review_history: Option, retained_context: Arc, - guardian_context_mode: GuardianContextMode, guardian_review_mode: GuardianContextMode, history_version: u64, user_message_revision: u64, @@ -134,8 +131,7 @@ impl ConversationHistorySnapshot for SharedConversationHistory { } fn retained_context(&self) -> Option<&RetainedContext> { - (self.guardian_context_mode == GuardianContextMode::ThreadOwned) - .then_some(&self.retained_context) + Some(&self.retained_context) } fn uses_parent_context_for_review(&self) -> bool { @@ -191,8 +187,7 @@ impl ContextManager { items: Arc::new(Vec::new()), review_history: None, retained_context: Arc::default(), - guardian_context_mode: GuardianContextMode::Legacy, - guardian_review_mode: GuardianContextMode::Legacy, + guardian_review_mode: GuardianContextMode::ThreadOwned, retain_inherited_user_messages: false, history_version: 0, reset_version: 0, @@ -210,7 +205,6 @@ impl ContextManager { items: Arc::clone(&self.items), review_history: self.review_history.clone(), retained_context: Arc::clone(&self.retained_context), - guardian_context_mode: self.guardian_context_mode, guardian_review_mode: self.guardian_review_mode, history_version: self.history_version, user_message_revision: self.user_message_revision, @@ -221,16 +215,9 @@ impl ContextManager { &self.retained_context } - pub(crate) fn with_guardian_context_mode( - guardian_context_mode: GuardianContextMode, - source: &SessionSource, - ) -> Self { + pub(crate) fn for_session(source: &SessionSource) -> Self { Self { - guardian_context_mode, - guardian_review_mode: guardian_context_mode, - retain_inherited_user_messages: guardian_context_mode - == GuardianContextMode::ThreadOwned - && !source.is_non_root_agent(), + retain_inherited_user_messages: !source.is_non_root_agent(), ..Self::new() } } @@ -314,32 +301,26 @@ impl ContextManager { }) }); self.guardian_review_mode = if requires_parent_context { - self.guardian_context_mode + GuardianContextMode::ThreadOwned } else { - self.guardian_context_mode - .for_checkpoint(&self.items, reviewer_compaction_hash) + GuardianContextMode::for_checkpoint(&self.items, reviewer_compaction_hash) }; self.restore_retained_context(retained_context); - if self.guardian_context_mode == GuardianContextMode::ThreadOwned { - // 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, - ) - }); - } + // 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) + }); let retain_legacy_authorization = self.retain_inherited_user_messages && self.retained_context.has_missing_user_messages() && (checkpoint.is_some() @@ -347,9 +328,8 @@ impl ContextManager { // Preserve its surviving instructions without treating an opaque checkpoint's // partial model window as a complete compatibility transcript. || codex_history::CompactionCheckpoint::latest(&self.items).is_none()); - if (self.guardian_review_mode == GuardianContextMode::ThreadOwned - && !retain_legacy_authorization) - || (self.guardian_context_mode == GuardianContextMode::Legacy && checkpoint.is_none()) + if self.guardian_review_mode == GuardianContextMode::ThreadOwned + && !retain_legacy_authorization { self.review_history = None; return; @@ -486,8 +466,7 @@ impl ContextManager { review_history.record(&processed.item); } Arc::make_mut(&mut self.items).push(processed); - if self.guardian_context_mode == GuardianContextMode::ThreadOwned - && let Some(metadata) = metadata + if let Some(metadata) = metadata && Arc::make_mut(&mut self.retained_context).record_sender_user_messages(metadata) { self.user_message_revision = self.user_message_revision.saturating_add(1); @@ -613,9 +592,7 @@ impl ContextManager { reviewer_compaction_hash: Option<&str>, ) -> bool { let promoted = self.guardian_review_mode == GuardianContextMode::Legacy - && self - .guardian_context_mode - .for_checkpoint(&items, reviewer_compaction_hash) + && GuardianContextMode::for_checkpoint(&items, reviewer_compaction_hash) == GuardianContextMode::ThreadOwned; if promoted { self.guardian_review_mode = GuardianContextMode::ThreadOwned; @@ -698,9 +675,7 @@ impl ContextManager { self.trim_pre_turn_context_updates(&snapshot, first_instruction_turn_idx, cut_idx); let mut retained_items = snapshot[..cut_idx].to_vec(); - if self.guardian_context_mode == GuardianContextMode::ThreadOwned - && let Some(boundary) = source.acceptance_order() - { + if let Some(boundary) = source.acceptance_order() { // A later assistant item may have finished before an earlier-accepted // steer was persisted. Drop its raw source too, so recovery cannot // reintroduce context removed at the retained rollback boundary. diff --git a/codex-rs/core/src/context_manager/history_tests.rs b/codex-rs/core/src/context_manager/history_tests.rs index 5fe7369785..f141375086 100644 --- a/codex-rs/core/src/context_manager/history_tests.rs +++ b/codex-rs/core/src/context_manager/history_tests.rs @@ -170,10 +170,7 @@ fn conversation_history_snapshot_binds_review_mode_and_hash_to_the_latest_item( ..Default::default() }), }; - let mut history = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &codex_protocol::protocol::SessionSource::Cli, - ); + let mut history = ContextManager::for_session(&codex_protocol::protocol::SessionSource::Cli); history.replace_annotated(vec![checkpoint.clone()]); history.restore_review_context( /*retained_context*/ None, @@ -257,10 +254,7 @@ fn conversation_history_snapshot_binds_review_mode_and_hash_to_the_latest_item( }] }); "retained assistant context")] fn checkpoint_retained_evidence_survives_legacy_review(saved_context: serde_json::Value) { - let mut history = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &codex_protocol::protocol::SessionSource::Cli, - ); + let mut history = ContextManager::for_session(&codex_protocol::protocol::SessionSource::Cli); history.replace_annotated(vec![ResponseItemEnvelope::new( serde_json::from_value(serde_json::json!({ "type": "compaction", "id": "unknown", "encrypted_content": "opaque checkpoint" @@ -296,10 +290,7 @@ fn checkpoint_retained_evidence_survives_legacy_review(saved_context: serde_json #[test] fn checkpoint_replayed_messages_keep_legacy_review_when_the_source_survives() { - let mut history = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &codex_protocol::protocol::SessionSource::Cli, - ); + let mut history = ContextManager::for_session(&codex_protocol::protocol::SessionSource::Cli); history.replace_annotated(vec![ResponseItemEnvelope::new( serde_json::from_value(serde_json::json!({ "type": "compaction", "id": "unknown", "encrypted_content": "opaque checkpoint" @@ -345,10 +336,7 @@ fn plaintext_checkpoint_without_backup_preserves_root_instructions( .expect("old instruction without ordering metadata"); let saved_context: Option = saved_context.map(|value| serde_json::from_value(value).expect("legacy retained context")); - let mut history = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &SessionSource::Cli, - ); + let mut history = ContextManager::for_session(&SessionSource::Cli); history.replace(vec![instruction]); history.restore_review_context( saved_context.as_ref(), @@ -369,10 +357,7 @@ fn plaintext_checkpoint_without_backup_preserves_root_instructions( }), }]; history.replace_compacted(compacted.clone(), Some("reviewer")); - let mut resumed = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &SessionSource::Cli, - ); + let mut resumed = ContextManager::for_session(&SessionSource::Cli); resumed.replace_annotated(compacted.clone()); resumed.restore_review_context( Some(history.retained_context()), @@ -429,10 +414,7 @@ fn legacy_checkpoint_rollback_keeps_answers_before_a_same_turn_steer() { .verified_answers() .cloned() .collect::>(); - let mut history = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &SessionSource::Cli, - ); + let mut history = ContextManager::for_session(&SessionSource::Cli); history.replace_annotated(checkpoint.replacement_history.take().unwrap()); history.restore_review_context( checkpoint.retained_context.as_ref(), @@ -1541,10 +1523,7 @@ fn drop_last_n_user_turns_preserves_prefix() { // A steered message shares its source turn, but rollback must keep the earlier // instruction and answer as complete evidence, including after the next compaction. - let mut history = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &codex_protocol::protocol::SessionSource::Exec, - ); + let mut history = ContextManager::for_session(&codex_protocol::protocol::SessionSource::Exec); let mut expected = None; for (id, text) in [ ("restriction", "Never publish publicly."), @@ -1594,10 +1573,7 @@ fn drop_last_n_user_turns_preserves_prefix() { #[test] fn rollback_removes_assistant_sources_recorded_ahead_of_queued_input() { - let mut history = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &codex_protocol::protocol::SessionSource::Exec, - ); + let mut history = ContextManager::for_session(&codex_protocol::protocol::SessionSource::Exec); let original = user_msg("Staging only."); let items = [ (original.clone(), 0), diff --git a/codex-rs/core/src/context_manager/history_user_authorization.rs b/codex-rs/core/src/context_manager/history_user_authorization.rs index 0330b2ec7a..9eaad2b439 100644 --- a/codex-rs/core/src/context_manager/history_user_authorization.rs +++ b/codex-rs/core/src/context_manager/history_user_authorization.rs @@ -7,7 +7,6 @@ use std::sync::Arc; use super::ContextManager; use crate::compact::is_summary_message; -use crate::context::GuardianContextMode; use crate::context::is_contextual_user_fragment; use crate::event_mapping::parse_turn_item; use crate::guardian::GUARDIAN_MAX_ROOT_MESSAGE_TOKENS; @@ -66,20 +65,18 @@ impl ContextManager { pub(crate) fn restore_retained_context(&mut self, checkpoint: Option<&RetainedContext>) { Arc::make_mut(&mut self.retained_context).restore(checkpoint, &self.items); - if self.guardian_context_mode == GuardianContextMode::ThreadOwned { - let items = Arc::clone(&self.items); - for envelope in items.iter().filter(|envelope| { - envelope - .metadata - .as_ref() - .is_some_and(|metadata| metadata.delivered_assistant_message.is_some()) - }) { - self.record_retained_message( - &envelope.item, - envelope.metadata.as_ref(), - RetainedMessageSource::Checkpoint, - ); - } + let items = Arc::clone(&self.items); + for envelope in items.iter().filter(|envelope| { + envelope + .metadata + .as_ref() + .is_some_and(|metadata| metadata.delivered_assistant_message.is_some()) + }) { + self.record_retained_message( + &envelope.item, + envelope.metadata.as_ref(), + RetainedMessageSource::Checkpoint, + ); } if self.retain_inherited_user_messages && !self.retained_context.has_inherited_user_messages() @@ -108,9 +105,8 @@ impl ContextManager { metadata: Option<&CodexHarnessMetadata>, source: RetainedMessageSource, ) { - if self.guardian_context_mode == GuardianContextMode::ThreadOwned - && let Some(text) = - metadata.and_then(|metadata| metadata.delivered_assistant_message.as_ref()) + if let Some(text) = + metadata.and_then(|metadata| metadata.delivered_assistant_message.as_ref()) && let ResponseItem::FunctionCallOutput { call_id: Some(call_id), .. @@ -147,11 +143,7 @@ impl ContextManager { return; } let inherited = metadata.is_some_and(|metadata| metadata.inherited_user_message); - if self.guardian_context_mode == GuardianContextMode::Legacy { - if !is_assistant { - Arc::make_mut(&mut self.retained_context).mark_user_messages_incomplete(); - } - } else if (!inherited || self.retain_inherited_user_messages) + if (!inherited || self.retain_inherited_user_messages) && let ResponseItem::Message { content, internal_chat_message_metadata_passthrough, diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 32f6038c63..2928e9e1b4 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -4,7 +4,6 @@ #[path = "review_request.rs"] mod request; -use crate::context::GuardianContextMode; use codex_analytics::GuardianApprovalRequestSource; use codex_analytics::GuardianReviewAnalyticsResult; use codex_core_plugins::PluginCommandAttribution; diff --git a/codex-rs/core/src/guardian/review_request.rs b/codex-rs/core/src/guardian/review_request.rs index fd2e17416c..0e271738ba 100644 --- a/codex-rs/core/src/guardian/review_request.rs +++ b/codex-rs/core/src/guardian/review_request.rs @@ -163,19 +163,18 @@ impl ReviewHost for super::super::runtime::ReviewRuntime { let root_authorization_version = prepared.root_authorization_version; let user_message_revision = prepared.user_message_revision; if matches!(&outcome, GuardianReviewOutcome::Completed(assessment) if assessment.outcome == GuardianAssessmentOutcome::Allow) - && ((session.guardian_context_mode == GuardianContextMode::ThreadOwned - && (root_authorization_version + && ((root_authorization_version + != session + .services + .agent_control + .get_guardian_package(session.thread_id) + .await + .map(|snapshot| snapshot.authorization_version) + || user_message_revision != session - .services - .agent_control - .get_guardian_package(session.thread_id) + .conversation_history_snapshot() .await - .map(|snapshot| snapshot.authorization_version) - || user_message_revision - != session - .conversation_history_snapshot() - .await - .user_message_revision())) + .user_message_revision()) || self.history_reset.is_cancelled() || cancellation.is_cancelled()) { diff --git a/codex-rs/core/src/session/guardian_checkpoint_tests.rs b/codex-rs/core/src/session/guardian_checkpoint_tests.rs index 88d26dc04b..c4b879412b 100644 --- a/codex-rs/core/src/session/guardian_checkpoint_tests.rs +++ b/codex-rs/core/src/session/guardian_checkpoint_tests.rs @@ -14,8 +14,7 @@ use test_case::test_case; #[test_case(GuardianContextMode::ThreadOwned; "thread_owned")] #[tokio::test] async fn guardian_checkpoint_preserves_live_context_without_storage(mode: GuardianContextMode) { - let (mut session, turn) = make_session_and_context().await; - session.guardian_context_mode = mode; + let (session, turn) = make_session_and_context().await; // This session has no live store. A checkpoint must still capture the complete context. assert!(session.live_thread().is_none()); let instruction: ResponseItem = serde_json::from_value(json!({ @@ -30,7 +29,7 @@ async fn guardian_checkpoint_preserves_live_context_without_storage(mode: Guardi let world_state = WorldStateSnapshot::from(baseline.as_object().unwrap()); { let mut state = session.state.lock().await; - state.history = ContextManager::with_guardian_context_mode(mode, &SessionSource::default()); + state.history = ContextManager::for_session(&SessionSource::default()); state .history .record_items([&instruction], turn.model_info().truncation_policy.into()); @@ -41,8 +40,13 @@ async fn guardian_checkpoint_preserves_live_context_without_storage(mode: Guardi "type": "compaction", "id": "compaction-1", "encrypted_content": "summary", })) .unwrap(); - state.history.replace_compacted( - vec![compacted.into()], + let retained = state.history.retained_context().clone(); + state.history.replace_annotated(vec![compacted.into()]); + state.history.restore_review_context( + Some(&retained), + Some(&codex_history::GuardianHistoryCheckpoint(vec![ + instruction.clone(), + ])), /*reviewer_compaction_hash*/ None, ); } @@ -88,10 +92,8 @@ async fn guardian_checkpoint_preserves_live_context_without_storage(mode: Guardi .replace_history(Vec::new(), /*reference_context_item*/ None) .await; // Replay into a fresh session so preserved live state cannot mask missing checkpoint data. - let (mut fork, _) = make_session_and_context().await; - fork.guardian_context_mode = mode; - fork.state.lock().await.history = - ContextManager::with_guardian_context_mode(mode, &SessionSource::default()); + let (fork, _) = make_session_and_context().await; + fork.state.lock().await.history = ContextManager::for_session(&SessionSource::default()); fork.record_initial_history(InitialHistory::Forked(items)) .await; let restored = fork.clone_history().await; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index c644f072f1..3a76a5b3c7 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -24,7 +24,6 @@ use crate::config::ManagedFeatures; use crate::config::resolve_tool_suggest_config_from_layer_stack; use crate::context::ContextualUserFragment; use crate::context::DeveloperInstructions; -use crate::context::GuardianContextMode; use crate::context::GuardianPolicy; use crate::context::ManagedDeveloperInstructions; use crate::context::ModelSwitchInstructions; @@ -1742,14 +1741,9 @@ impl Session { .zip(metadata) .map(|(item, metadata)| ResponseItemEnvelope { item, metadata }) .collect(); - let reviewer_compaction_hash = - if self.guardian_context_mode == crate::context::GuardianContextMode::ThreadOwned { - let context = crate::guardian::GuardianReviewContext::from(turn_context); - let (_, reviewer) = crate::guardian::resolve_review_model(self, &context).await; - reviewer.comp_hash.clone() - } else { - None - }; + let context = crate::guardian::GuardianReviewContext::from(turn_context); + let (_, reviewer) = crate::guardian::resolve_review_model(self, &context).await; + let reviewer_compaction_hash = reviewer.comp_hash.clone(); { let mut state = self.state.lock().await; state.replace_annotated_history( @@ -3613,39 +3607,37 @@ impl Session { state .current_time_reminder .note_recorded_items(&response_items); - if self.guardian_context_mode == GuardianContextMode::ThreadOwned { - let pending_orders = turn_context - .extension_data - .get::(); - for envelope in &mut items { - if envelope + let pending_orders = turn_context + .extension_data + .get::(); + for envelope in &mut items { + if envelope + .metadata + .as_ref() + .is_some_and(|metadata| metadata.compaction_output) + { + continue; + } + if matches!(&envelope.item, ResponseItem::Message { role, .. } if role == "assistant") + || matches!(&envelope.item, ResponseItem::FunctionCall { .. }) + || crate::context::is_user_authorization_message(&envelope.item) + { + let message_order = pending_orders.as_ref().and_then(|orders| { + orders + .0 + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .remove(envelope.item.id()?.as_str()) + }); + // Preserve input acceptance and source-message start order. + // Synthetic messages still receive their order here. + envelope .metadata - .as_ref() - .is_some_and(|metadata| metadata.compaction_output) - { - continue; - } - if matches!(&envelope.item, ResponseItem::Message { role, .. } if role == "assistant") - || matches!(&envelope.item, ResponseItem::FunctionCall { .. }) - || crate::context::is_user_authorization_message(&envelope.item) - { - let message_order = pending_orders.as_ref().and_then(|orders| { - orders - .0 - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .remove(envelope.item.id()?.as_str()) + .get_or_insert_default() + .user_input_order + .get_or_insert_with(|| { + message_order.unwrap_or_else(|| state.history.reserve_input_order()) }); - // Preserve input acceptance and source-message start order. - // Synthetic messages still receive their order here. - envelope - .metadata - .get_or_insert_default() - .user_input_order - .get_or_insert_with(|| { - message_order.unwrap_or_else(|| state.history.reserve_input_order()) - }); - } } } state diff --git a/codex-rs/core/src/session/retained_context.rs b/codex-rs/core/src/session/retained_context.rs index 72f22c7367..0d12b5b467 100644 --- a/codex-rs/core/src/session/retained_context.rs +++ b/codex-rs/core/src/session/retained_context.rs @@ -4,7 +4,6 @@ use std::collections::HashMap; use std::sync::Mutex; use std::sync::PoisonError; -use crate::context::GuardianContextMode; use codex_history::RetainedContextEvent; use codex_history::RolloutItem; use codex_protocol::models::ResponseItem; @@ -25,10 +24,9 @@ impl Session { turn_context: &TurnContext, item: &ResponseItem, ) { - if self.guardian_context_mode == GuardianContextMode::ThreadOwned - && let ResponseItem::Message { - id: Some(id), role, .. - } = item + if let ResponseItem::Message { + id: Some(id), role, .. + } = item && role == "assistant" { let mut state = self.state.lock().await; @@ -49,12 +47,8 @@ impl Session { } } - /// Legacy mode neither reserves a sequence nor takes the session-state lock. - pub(crate) async fn reserve_user_input_order(&self) -> Option { - if self.guardian_context_mode == GuardianContextMode::Legacy { - return None; - } - Some(self.state.lock().await.history.reserve_input_order()) + pub(crate) async fn reserve_user_input_order(&self) -> u64 { + self.state.lock().await.history.reserve_input_order() } pub(crate) async fn record_retained_context(&self, mut event: RetainedContextEvent) { diff --git a/codex-rs/core/src/session/rollout_reconstruction.rs b/codex-rs/core/src/session/rollout_reconstruction.rs index 473cd17bc7..298fd6806d 100644 --- a/codex-rs/core/src/session/rollout_reconstruction.rs +++ b/codex-rs/core/src/session/rollout_reconstruction.rs @@ -1,5 +1,4 @@ use super::*; -use crate::context::GuardianContextMode; use crate::context::world_state::WorldStateSnapshot; use crate::context_manager::is_user_turn_boundary; use codex_history::ResponseItemEnvelope; @@ -393,10 +392,7 @@ impl Session { .unwrap_or(u64::MAX); // Build model-visible history from the selected compaction and its newer suffix. - let mut history = ContextManager::with_guardian_context_mode( - self.guardian_context_mode, - &turn_context.session_source, - ); + let mut history = ContextManager::for_session(&turn_context.session_source); let mut saw_legacy_compaction_without_replacement_history = false; if let Some(checkpoint) = history_checkpoint && let Some(items) = &checkpoint.compacted.replacement_history @@ -445,16 +441,8 @@ impl Session { // prompt shape. // TODO(ccunningham): if we drop support for None replacement_history compaction items, // we can get rid of this second loop entirely and just build `history` directly in the first loop. - let identity = - if self.guardian_context_mode == GuardianContextMode::ThreadOwned { - compact::CompactedMessageIdentity::Preserve - } else { - compact::CompactedMessageIdentity::Regenerate - }; - let user_messages = compact::collect_annotated_user_messages( - history.annotated_items(), - identity, - ); + let user_messages = + compact::collect_annotated_user_messages(history.annotated_items()); let rebuilt = compact::build_compacted_history( Vec::new(), &user_messages, diff --git a/codex-rs/core/src/session/rollout_reconstruction_tests.rs b/codex-rs/core/src/session/rollout_reconstruction_tests.rs index e69c0fb585..f5d5aa74d9 100644 --- a/codex-rs/core/src/session/rollout_reconstruction_tests.rs +++ b/codex-rs/core/src/session/rollout_reconstruction_tests.rs @@ -1,5 +1,4 @@ use super::*; -use crate::context::GuardianContextMode; use super::tests::build_world_state_from_turn_context; use super::tests::make_session_and_context; @@ -33,12 +32,7 @@ use uuid::Uuid; #[tokio::test] async fn recorded_questions_share_queued_input_order_across_resume() { - let (mut session, turn) = make_session_and_context().await; - session.guardian_context_mode = GuardianContextMode::ThreadOwned; - session.state.lock().await.history = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &SessionSource::default(), - ); + let (session, turn) = make_session_and_context().await; let question = |call_id: &str| { serde_json::from_value::(json!({ "type": "function_call", "call_id": call_id, @@ -64,7 +58,7 @@ async fn recorded_questions_share_queued_input_order_across_resume() { ResponseItemEnvelope { item: user_message("Yes."), metadata: Some(codex_history::CodexHarnessMetadata { - user_input_order: reply_order, + user_input_order: Some(reply_order), ..Default::default() }), }, @@ -109,17 +103,14 @@ async fn recorded_questions_share_queued_input_order_across_resume() { .collect::>(), vec![Some(0), Some(2), Some(1), None] ); - assert_eq!(session.reserve_user_input_order().await, Some(3)); + assert_eq!(session.reserve_user_input_order().await, 3); } #[tokio::test] async fn sender_context_follows_its_delivery_through_checkpoint_and_rollback() { - let (mut session, turn_context) = make_session_and_context().await; - session.guardian_context_mode = GuardianContextMode::ThreadOwned; - let mut live = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &SessionSource::default(), - ); + let (session, turn_context) = make_session_and_context().await; + + let mut live = ContextManager::for_session(&SessionSource::default()); let mut items = Vec::new(); let mut snapshots = Vec::new(); for index in 0..2 { diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 970fa606cb..22f3d42d9d 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -10,7 +10,6 @@ use crate::agent::api::AgentControl; use crate::agents_md_manager::AgentsMdManager; use crate::agents_md_manager::SessionInstructions; use crate::config::ConstraintError; -use crate::context::GuardianContextMode; use crate::environment_selection::ThreadEnvironments; use crate::environment_selection::TurnEnvironmentSnapshot; use crate::hook_mcp_executor::CoreHookMcpExecutor; @@ -71,7 +70,6 @@ pub(crate) struct Session { /// The set of enabled features should be invariant for the lifetime of the /// session. pub(super) features: ManagedFeatures, - pub(crate) guardian_context_mode: GuardianContextMode, pub(super) isolation: codex_extension_api::SessionIsolation, pub(crate) tool_policy: Arc, pub(crate) windows_sandbox_proxy_settings_mode: @@ -988,8 +986,6 @@ impl Session { thread_id.to_string(), thread_extension_init, ); - // Capture is unconditional; replay selects reviewer policy from the saved checkpoint. - let guardian_context_mode = GuardianContextMode::ThreadOwned; thread_extension_data.insert(crate::context::GuardianReviewEvidence::default()); // Kick off independent async setup tasks in parallel to reduce startup latency. // @@ -1485,8 +1481,7 @@ impl Session { let mut state = SessionState::new_with_auto_compact_window_ids( session_configuration.clone(), initial_auto_compact_window_ids, - ContextManager::with_guardian_context_mode( - guardian_context_mode, + ContextManager::for_session( &session_configuration.session_source, ), ); @@ -1766,7 +1761,6 @@ impl Session { thread_settings_persistence: Semaphore::new(/*permits*/ 1), managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1), features: config.features.clone(), - guardian_context_mode, isolation, tool_policy, windows_sandbox_proxy_settings_mode, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index f2e2ae0193..14431708f0 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -18,7 +18,6 @@ use crate::config::ConfigBuilder; use crate::config::ConfigOverrides; use crate::context::ContextualUserFragment; use crate::context::DeveloperInstructions; -use crate::context::GuardianContextMode; use crate::context::TurnAborted; use crate::environment_selection::EnvironmentConfigOrigin; use crate::environment_selection::ThreadEnvironments; @@ -6402,10 +6401,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { ); let mut state = SessionState::new(session_configuration.clone()); - state.history = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &session_configuration.session_source, - ); + state.history = ContextManager::for_session(&session_configuration.session_source); let (environment_manager, resolved_environments) = resolved_environments_for_configuration(&session_configuration, &default_environments) .await; @@ -6551,7 +6547,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { thread_settings_persistence: Semaphore::new(/*permits*/ 1), managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1), features: config.features.clone(), - guardian_context_mode: GuardianContextMode::ThreadOwned, + isolation: codex_extension_api::SessionIsolation::Inherit, tool_policy: Arc::default(), windows_sandbox_proxy_settings_mode: @@ -8674,10 +8670,7 @@ where ); let mut state = SessionState::new(session_configuration.clone()); - state.history = ContextManager::with_guardian_context_mode( - GuardianContextMode::ThreadOwned, - &session_configuration.session_source, - ); + state.history = ContextManager::for_session(&session_configuration.session_source); let (environment_manager, resolved_turn_environments) = resolved_environments_for_configuration(&session_configuration, &default_environments) .await; @@ -8822,7 +8815,7 @@ where thread_settings_persistence: Semaphore::new(/*permits*/ 1), managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1), features: config.features.clone(), - guardian_context_mode: GuardianContextMode::ThreadOwned, + isolation: codex_extension_api::SessionIsolation::Inherit, tool_policy: Arc::default(), windows_sandbox_proxy_settings_mode: diff --git a/codex-rs/core/src/session/turn_input.rs b/codex-rs/core/src/session/turn_input.rs index 56cfea6049..3e2034ff9b 100644 --- a/codex-rs/core/src/session/turn_input.rs +++ b/codex-rs/core/src/session/turn_input.rs @@ -18,7 +18,6 @@ use super::session::SessionSettingsUpdate; use super::thread_settings; use super::turn_context::NewTurnContextOptions; use super::turn_context::TurnContext; -use crate::context::GuardianContextMode; use crate::state::ActiveTurn; use crate::state::TurnState; use crate::tasks::RegularTask; @@ -691,7 +690,7 @@ impl Session { TurnInput::UserInput { content: std::mem::take(content), client_id: client_id.clone(), - acceptance_order: self.reserve_user_input_order().await, + acceptance_order: Some(self.reserve_user_input_order().await), } } input => pending_turn_input(self, input.clone(), active_turn_id).await, @@ -731,7 +730,7 @@ async fn pending_turn_input( SubmittedTurnInput::UserInput { content, client_id } => TurnInput::UserInput { content, client_id, - acceptance_order: session.reserve_user_input_order().await, + acceptance_order: Some(session.reserve_user_input_order().await), }, SubmittedTurnInput::ResponseItem(mut item) if matches!( @@ -740,15 +739,14 @@ async fn pending_turn_input( ) => { Session::assign_missing_response_item_id(&mut item); - let metadata = if session.guardian_context_mode == GuardianContextMode::ThreadOwned - && let Some(messages) = session - .services - .local_agent_runtime - .capture_sender_user_messages(&item, session.thread_id, turn_id) - .await + let metadata = if let Some(messages) = session + .services + .local_agent_runtime + .capture_sender_user_messages(&item, session.thread_id, turn_id) + .await { Some(CodexHarnessMetadata { - user_input_order: session.reserve_user_input_order().await, + user_input_order: Some(session.reserve_user_input_order().await), sender_user_messages: Some(Box::new(messages)), ..Default::default() }) diff --git a/codex-rs/core/src/state/turn.rs b/codex-rs/core/src/state/turn.rs index 8f45fc5026..334372019f 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -108,8 +108,8 @@ pub(crate) struct TurnState { /// Host receipt metadata follows the response through the asynchronous tool waiter. pub(crate) struct AcceptedUserInputResponse { pub(crate) response: RequestUserInputResponse, - /// Absent in legacy mode, which does not reserve or persist acceptance order. - pub(crate) acceptance_order: Option, + /// Order reserved when the host accepts this response. + pub(crate) acceptance_order: u64, } pub(crate) struct PendingRequestPermissions { diff --git a/codex-rs/core/src/tools/handlers/request_user_input.rs b/codex-rs/core/src/tools/handlers/request_user_input.rs index 0f4e7d501f..d55eca0297 100644 --- a/codex-rs/core/src/tools/handlers/request_user_input.rs +++ b/codex-rs/core/src/tools/handlers/request_user_input.rs @@ -1,4 +1,3 @@ -use crate::context::GuardianContextMode; use crate::function_tool::FunctionCallError; use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; @@ -101,10 +100,8 @@ impl RequestUserInputHandler { "failed to serialize {REQUEST_USER_INPUT_TOOL_NAME} response: {err}" )) })?; - // Capture follows the session flag even while an older checkpoint uses legacy review. - if turn.config.features.enabled(Feature::GuardianApproval) - && session.guardian_context_mode == GuardianContextMode::ThreadOwned - { + // Persist answers even while an older checkpoint needs compatibility review. + if turn.config.features.enabled(Feature::GuardianApproval) { let user_input = questions .iter() .filter_map(|question| { @@ -142,7 +139,7 @@ impl RequestUserInputHandler { call_id, questions: user_input, }, - acceptance_order: accepted.acceptance_order, + acceptance_order: Some(accepted.acceptance_order), }) .await; } 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 cd75095ab4..2a1403cc32 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 @@ -1893,15 +1893,6 @@ async fn contributor_uses_model_defaults_and_preserves_local_overrides() -> Resu #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<()> { - assert_luna_pool_context(/*thread_context_enabled*/ true).await -} - -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn deprecated_opt_out_still_samples_with_thread_owned_context() -> Result<()> { - assert_luna_pool_context(/*thread_context_enabled*/ false).await -} - -async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> { skip_if_no_network!(Ok(())); let conversation_history = vec![ @@ -1946,7 +1937,7 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> { conversation_history, r#"{"path":"README.md"}"#, Some(TEST_GUARDIAN_POLICY), - &format!("[features.guardianv2]\nthread_context = {thread_context_enabled}\n"), + "", /*model_defaults*/ None, ) .await?; @@ -2028,6 +2019,7 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> { }, {"type": "input_text", "text": ">>> APPROVAL REQUEST END\n"}, ]); + assert_eq!(request["input"][2]["content"], expected_content); let score = tokio::time::timeout(ASYNC_TEST_TIMEOUT, async { loop { @@ -2413,21 +2405,9 @@ async fn cached_score_survives_compaction_and_internal_context_but_not_user_inpu #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn incompatible_compaction_blocks_cached_score_and_initial_cua_allowance() -> Result<()> { - assert_compaction_approval_policy(/*thread_context_enabled*/ true).await -} - -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn deprecated_opt_out_still_rejects_incompatible_compaction() -> Result<()> { - assert_compaction_approval_policy(/*thread_context_enabled*/ false).await -} - -async fn assert_compaction_approval_policy(thread_context_enabled: bool) -> Result<()> { skip_if_no_network!(Ok(())); - let fixture = GuardianFailureFixture::with_config(&format!( - "[features.guardianv2]\nthread_context = {thread_context_enabled}\n" - )) - .await?; + let fixture = GuardianFailureFixture::with_config("").await?; let thread_store = fixture.test.codex.thread_extension_data(); set_cached_score( thread_store, @@ -2481,7 +2461,7 @@ async fn assert_compaction_approval_policy(thread_context_enabled: bool) -> Resu /*index*/ 1, score_authorization, ); - // No new sample runs: only the enabled path rejects cached and initial-call approvals. + // No new sample runs: the live checkpoint rejects cached and initial-call approvals. for (computer_use_only, prompt) in [ (false, "review action"), ( @@ -2964,25 +2944,22 @@ async fn contributor_sends_compacted_conversation_history_to_luna() -> Result<() #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn contributor_reuses_the_latest_compatible_parent_compaction() -> Result<()> { - assert_parent_compaction_reuse(/*thread_context_enabled*/ true).await + assert_parent_compaction_reuse(/*parent_context_for_review*/ true).await } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn legacy_contributor_reuses_the_latest_compatible_parent_compaction() -> Result<()> { - assert_parent_compaction_reuse(/*thread_context_enabled*/ false).await + assert_parent_compaction_reuse(/*parent_context_for_review*/ false).await } -async fn assert_parent_compaction_reuse(thread_context_enabled: bool) -> Result<()> { +async fn assert_parent_compaction_reuse(parent_context_for_review: bool) -> Result<()> { skip_if_no_network!(Ok(())); let thread_server = responses::start_mock_server().await; let test = test_codex() .with_config(move |config| { config.approvals_reviewer = ApprovalsReviewer::AutoReview; - config - .features - .set_enabled(Feature::GuardianThreadContext, thread_context_enabled) - .expect("test context mode"); + }) .with_pre_build_hook(|home| { std::fs::write( @@ -3074,7 +3051,7 @@ async fn assert_parent_compaction_reuse(thread_context_enabled: bool) -> Result< retained, current: conversation_history, compaction_model_hash: parent_model.comp_hash.clone(), - retained_context: thread_context_enabled.then(codex_history::RetainedContext::default), + retained_context: parent_context_for_review.then(codex_history::RetainedContext::default), }; thread_store.insert(parent_model); @@ -3170,7 +3147,7 @@ async fn assert_parent_compaction_reuse(thread_context_enabled: bool) -> Result< conversation_history: Arc::new(TestRetainedHistory { current: TestConversationHistory(vec![latest_compaction, oversized_compaction]), retained: Vec::new(), - retained_context: thread_context_enabled + retained_context: parent_context_for_review .then(codex_history::RetainedContext::default), compaction_model_hash: thread_store .get::() diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 5f80c39a8d..dadf5b40ae 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -1646,7 +1646,7 @@ pub const FEATURES: &[FeatureSpec] = &[ id: Feature::GuardianThreadContext, key: "guardianv2.thread_context", stage: Stage::Removed, - default_enabled: true, + default_enabled: false, }, FeatureSpec { id: Feature::GuardianReuseParentCompaction, diff --git a/codex-rs/features/src/tests.rs b/codex-rs/features/src/tests.rs index eb815aac28..bc28a146ea 100644 --- a/codex-rs/features/src/tests.rs +++ b/codex-rs/features/src/tests.rs @@ -207,7 +207,6 @@ fn guardian_thread_context_is_ignored_with_a_migration_notice() { }, FeatureOverrides::default(), ); - assert!(features.enabled(Feature::GuardianThreadContext)); assert_eq!( features .legacy_feature_usages()