From 87a82425cebe9b18f025dbd4d5b0970a4a689825 Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Tue, 24 Mar 2026 15:18:35 -0700 Subject: [PATCH] core: simplify guardian trunk reuse matching Co-authored-by: Codex --- .../core/src/guardian/review_session/mod.rs | 61 +++++------------ .../core/src/guardian/review_session/spawn.rs | 68 +------------------ .../core/src/guardian/review_session/tests.rs | 13 +--- 3 files changed, 23 insertions(+), 119 deletions(-) diff --git a/codex-rs/core/src/guardian/review_session/mod.rs b/codex-rs/core/src/guardian/review_session/mod.rs index 95f23eab7f..47753e55e2 100644 --- a/codex-rs/core/src/guardian/review_session/mod.rs +++ b/codex-rs/core/src/guardian/review_session/mod.rs @@ -30,7 +30,6 @@ use crate::thread_manager::snapshot_rollout_history; use self::execution::run_before_review_deadline; use self::execution::run_review_on_session; -use self::spawn::GuardianReviewSessionReuseKey; use self::spawn::GuardianReviewSessionSpawnOutcome; #[cfg(test)] pub(crate) use self::spawn::build_guardian_review_session_config; @@ -141,8 +140,10 @@ struct GuardianReviewSession { codex: Codex, /// Session-scoped cancellation used during shutdown. cancel_token: CancellationToken, - /// Spawn-config fingerprint used to decide when the cached trunk is still reusable. - reuse_key: GuardianReviewSessionReuseKey, + /// Effective guardian config used for this child session. + /// + /// The trunk remains reusable only while future reviews resolve to the same config. + spawn_config: Config, /// Tracks whether this session has already completed at least one review turn. has_prior_review: AtomicBool, /// Prevents overlapping reviews on the same guardian session. @@ -153,13 +154,13 @@ impl GuardianReviewSession { fn new( codex: Codex, cancel_token: CancellationToken, - reuse_key: GuardianReviewSessionReuseKey, + spawn_config: Config, has_prior_review: bool, ) -> Self { Self { codex, cancel_token, - reuse_key, + spawn_config, has_prior_review: AtomicBool::new(has_prior_review), review_lock: Mutex::new(()), } @@ -318,14 +319,8 @@ impl GuardianReviewSessionManager { params: GuardianReviewSessionParams, ) -> GuardianReviewSessionOutcome { let deadline = tokio::time::Instant::now() + GUARDIAN_REVIEW_TIMEOUT; - let next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(¶ms.spawn_config); let trunk = match self - .get_or_spawn_trunk_for_review( - ¶ms, - &next_reuse_key, - deadline, - params.external_cancel.as_ref(), - ) + .get_or_spawn_trunk_for_review(¶ms, deadline, params.external_cancel.as_ref()) .await { Ok(Some(trunk)) => trunk, @@ -335,14 +330,9 @@ impl GuardianReviewSessionManager { // A stale-but-busy trunk stays in place so the in-flight review can finish. New work forks // instead of replacing the live session. - if trunk.reuse_key != next_reuse_key { + if trunk.spawn_config != params.spawn_config { return self - .run_forked_review( - params, - next_reuse_key, - deadline, - /*initial_history*/ None, - ) + .run_forked_review(params, deadline, /*initial_history*/ None) .await; } @@ -351,7 +341,7 @@ impl GuardianReviewSessionManager { Err(_) => { let initial_history = trunk.fork_initial_history().await; return self - .run_forked_review(params, next_reuse_key, deadline, initial_history) + .run_forked_review(params, deadline, initial_history) .await; } }; @@ -383,7 +373,6 @@ impl GuardianReviewSessionManager { spawn_config: Config, eager_init_cancel: &CancellationToken, ) { - let next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(&spawn_config); let Ok(_spawn_guard) = self.spawn_lock.try_lock() else { return; }; @@ -399,7 +388,6 @@ impl GuardianReviewSessionManager { parent_session, parent_turn, spawn_config, - next_reuse_key, ) .await { @@ -424,11 +412,10 @@ impl GuardianReviewSessionManager { async fn get_or_spawn_trunk_for_review( &self, params: &GuardianReviewSessionParams, - next_reuse_key: &GuardianReviewSessionReuseKey, deadline: tokio::time::Instant, external_cancel: Option<&CancellationToken>, ) -> Result>, GuardianReviewSessionOutcome> { - match self.prepare_trunk(next_reuse_key).await { + match self.prepare_trunk(¶ms.spawn_config).await { GuardianTrunkState::Ready(trunk) => return Ok(Some(trunk)), GuardianTrunkState::ShutdownStarted => return Ok(None), GuardianTrunkState::NeedsSpawn => {} @@ -443,7 +430,7 @@ impl GuardianReviewSessionManager { }; // Another task may have finished spawning while we were waiting on `spawn_lock`. - let trunk = match self.prepare_trunk(next_reuse_key).await { + let trunk = match self.prepare_trunk(¶ms.spawn_config).await { GuardianTrunkState::Ready(trunk) => Some(trunk), GuardianTrunkState::ShutdownStarted => None, GuardianTrunkState::NeedsSpawn => { @@ -454,7 +441,6 @@ impl GuardianReviewSessionManager { ¶ms.parent_session, ¶ms.parent_turn, params.spawn_config.clone(), - next_reuse_key.clone(), ) .await { @@ -477,7 +463,6 @@ impl GuardianReviewSessionManager { parent_session: &Arc, parent_turn: &Arc, spawn_config: Config, - next_reuse_key: GuardianReviewSessionReuseKey, ) -> Result>, GuardianReviewSessionSpawnOutcome> { // Spawn under the caller's deadline policy first, then register the result against shared // trunk state exactly once in `install_spawned_trunk`. @@ -487,7 +472,6 @@ impl GuardianReviewSessionManager { parent_session, parent_turn, spawn_config, - next_reuse_key, /*initial_history*/ None, ) .await?; @@ -496,17 +480,14 @@ impl GuardianReviewSessionManager { /// Inspects the cached trunk and eagerly evicts a stale idle trunk so the caller can spawn a /// replacement. Busy trunks are left in place. - async fn prepare_trunk( - &self, - next_reuse_key: &GuardianReviewSessionReuseKey, - ) -> GuardianTrunkState { + async fn prepare_trunk(&self, next_spawn_config: &Config) -> GuardianTrunkState { let (trunk_state, stale_trunk_to_shutdown) = { let mut state = self.state.lock().await; if state.shutdown_started { return GuardianTrunkState::ShutdownStarted; } if let Some(trunk) = state.trunk.as_ref() - && trunk.reuse_key != *next_reuse_key + && trunk.spawn_config != *next_spawn_config && trunk.review_lock.try_lock().is_ok() { (GuardianTrunkState::NeedsSpawn, state.trunk.take()) @@ -591,22 +572,18 @@ impl GuardianReviewSessionManager { #[cfg(test)] pub(crate) async fn cache_for_test(&self, codex: Codex) { - let reuse_key = GuardianReviewSessionReuseKey::from_spawn_config( - codex.session.get_config().await.as_ref(), - ); + let spawn_config = codex.session.get_config().await.as_ref().clone(); self.state.lock().await.trunk = Some(Arc::new(GuardianReviewSession::new( codex, CancellationToken::new(), - reuse_key, + spawn_config, /*has_prior_review*/ false, ))); } #[cfg(test)] pub(crate) async fn register_fork_for_test(&self, codex: Codex) { - let reuse_key = GuardianReviewSessionReuseKey::from_spawn_config( - codex.session.get_config().await.as_ref(), - ); + let spawn_config = codex.session.get_config().await.as_ref().clone(); self.state .lock() .await @@ -614,7 +591,7 @@ impl GuardianReviewSessionManager { .push(Arc::new(GuardianReviewSession::new( codex, CancellationToken::new(), - reuse_key, + spawn_config, /*has_prior_review*/ false, ))); } @@ -622,7 +599,6 @@ impl GuardianReviewSessionManager { async fn run_forked_review( &self, params: GuardianReviewSessionParams, - reuse_key: GuardianReviewSessionReuseKey, deadline: tokio::time::Instant, initial_history: Option, ) -> GuardianReviewSessionOutcome { @@ -636,7 +612,6 @@ impl GuardianReviewSessionManager { ¶ms.parent_session, ¶ms.parent_turn, fork_config, - reuse_key, initial_history, ) .await diff --git a/codex-rs/core/src/guardian/review_session/spawn.rs b/codex-rs/core/src/guardian/review_session/spawn.rs index e7146d76ae..e48c1532ef 100644 --- a/codex-rs/core/src/guardian/review_session/spawn.rs +++ b/codex-rs/core/src/guardian/review_session/spawn.rs @@ -5,8 +5,6 @@ //! - what config/model should the guardian child session use? //! - how do we spawn that child session under the caller's deadline/cancel policy? -use std::collections::HashMap; -use std::path::PathBuf; use std::sync::Arc; use codex_features::Feature; @@ -21,11 +19,7 @@ use crate::codex::TurnContext; use crate::codex_delegate::run_codex_thread_interactive; use crate::config::Config; use crate::config::Constrained; -use crate::config::ManagedFeatures; use crate::config::NetworkProxySpec; -use crate::config::Permissions; -use crate::config::types::McpServerConfig; -use crate::model_provider_info::ModelProviderInfo; use crate::models_manager::manager::RefreshStrategy; use crate::protocol::SandboxPolicy; @@ -50,62 +44,6 @@ pub(super) enum GuardianReviewSessionSpawnOutcome { Aborted, } -#[derive(Debug, Clone, PartialEq)] -pub(super) struct GuardianReviewSessionReuseKey { - // Only include settings that affect spawned-session behavior so reuse - // invalidation remains explicit and does not depend on unrelated config - // bookkeeping. - model: Option, - model_provider_id: String, - model_provider: ModelProviderInfo, - model_context_window: Option, - model_auto_compact_token_limit: Option, - model_reasoning_effort: Option, - permissions: Permissions, - developer_instructions: Option, - base_instructions: Option, - user_instructions: Option, - compact_prompt: Option, - cwd: PathBuf, - mcp_servers: Constrained>, - codex_linux_sandbox_exe: Option, - main_execve_wrapper_exe: Option, - js_repl_node_path: Option, - js_repl_node_module_dirs: Vec, - zsh_path: Option, - features: ManagedFeatures, - include_apply_patch_tool: bool, - use_experimental_unified_exec_tool: bool, -} - -impl GuardianReviewSessionReuseKey { - pub(super) fn from_spawn_config(spawn_config: &Config) -> Self { - Self { - model: spawn_config.model.clone(), - model_provider_id: spawn_config.model_provider_id.clone(), - model_provider: spawn_config.model_provider.clone(), - model_context_window: spawn_config.model_context_window, - model_auto_compact_token_limit: spawn_config.model_auto_compact_token_limit, - model_reasoning_effort: spawn_config.model_reasoning_effort, - permissions: spawn_config.permissions.clone(), - developer_instructions: spawn_config.developer_instructions.clone(), - base_instructions: spawn_config.base_instructions.clone(), - user_instructions: spawn_config.user_instructions.clone(), - compact_prompt: spawn_config.compact_prompt.clone(), - cwd: spawn_config.cwd.clone(), - mcp_servers: spawn_config.mcp_servers.clone(), - codex_linux_sandbox_exe: spawn_config.codex_linux_sandbox_exe.clone(), - main_execve_wrapper_exe: spawn_config.main_execve_wrapper_exe.clone(), - js_repl_node_path: spawn_config.js_repl_node_path.clone(), - js_repl_node_module_dirs: spawn_config.js_repl_node_module_dirs.clone(), - zsh_path: spawn_config.zsh_path.clone(), - features: spawn_config.features.clone(), - include_apply_patch_tool: spawn_config.include_apply_patch_tool, - use_experimental_unified_exec_tool: spawn_config.use_experimental_unified_exec_tool, - } - } -} - /// Spawns a guardian child session and maps deadline/cancel outcomes into a small internal enum. /// /// Trunk creation and fork creation both use this helper so they do not duplicate the same @@ -116,7 +54,6 @@ pub(super) async fn spawn_review_session_before_deadline( parent_session: &Arc, parent_turn: &Arc, spawn_config: Config, - reuse_key: GuardianReviewSessionReuseKey, initial_history: Option, ) -> Result, GuardianReviewSessionSpawnOutcome> { let spawn_cancel_token = tokio_util::sync::CancellationToken::new(); @@ -128,7 +65,6 @@ pub(super) async fn spawn_review_session_before_deadline( Arc::clone(parent_session), Arc::clone(parent_turn), spawn_config, - reuse_key, spawn_cancel_token.clone(), initial_history, )), @@ -168,11 +104,11 @@ async fn spawn_guardian_review_session( parent_session: Arc, parent_turn: Arc, spawn_config: Config, - reuse_key: GuardianReviewSessionReuseKey, cancel_token: tokio_util::sync::CancellationToken, initial_history: Option, ) -> anyhow::Result { let has_prior_review = initial_history.is_some(); + let review_config = spawn_config.clone(); // Guardian runs as an ordinary child Codex thread with a different config and source label. let codex = run_codex_thread_interactive( spawn_config, @@ -189,7 +125,7 @@ async fn spawn_guardian_review_session( Ok(GuardianReviewSession::new( codex, cancel_token, - reuse_key, + review_config, has_prior_review, )) } diff --git a/codex-rs/core/src/guardian/review_session/tests.rs b/codex-rs/core/src/guardian/review_session/tests.rs index 41c54b756d..e5ab245715 100644 --- a/codex-rs/core/src/guardian/review_session/tests.rs +++ b/codex-rs/core/src/guardian/review_session/tests.rs @@ -24,7 +24,6 @@ async fn guardian_review_session_with_shutdown_signal() -> ( let (child_session, _child_turn_context) = crate::codex::make_session_and_context().await; let child_session = Arc::new(child_session); let child_config = child_session.get_config().await; - let reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(child_config.as_ref()); let (child_tx_sub, child_rx_sub) = async_channel::bounded(4); let (_child_tx_event, child_rx_event) = async_channel::unbounded(); let (_child_status_tx, child_agent_status) = watch::channel(AgentStatus::PendingInit); @@ -49,7 +48,7 @@ async fn guardian_review_session_with_shutdown_signal() -> ( let review_session = Arc::new(GuardianReviewSession::new( child_codex, CancellationToken::new(), - reuse_key, + child_config.as_ref().clone(), /*has_prior_review*/ false, )); @@ -57,12 +56,11 @@ async fn guardian_review_session_with_shutdown_signal() -> ( } #[test] -fn guardian_review_session_config_change_invalidates_cached_session() { +fn guardian_review_session_config_change_changes_spawn_config() { let parent_config = crate::config::test_config(); let cached_spawn_config = build_guardian_review_session_config(&parent_config, None, "active-model", None) .expect("cached guardian config"); - let cached_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(&cached_spawn_config); let mut changed_parent_config = parent_config; changed_parent_config.model_provider.base_url = @@ -70,13 +68,8 @@ fn guardian_review_session_config_change_invalidates_cached_session() { let next_spawn_config = build_guardian_review_session_config(&changed_parent_config, None, "active-model", None) .expect("next guardian config"); - let next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(&next_spawn_config); - assert_ne!(cached_reuse_key, next_reuse_key); - assert_eq!( - cached_reuse_key, - GuardianReviewSessionReuseKey::from_spawn_config(&cached_spawn_config) - ); + assert_ne!(cached_spawn_config, next_spawn_config); } #[test]