From 588ee893241de551836bfd97e4964c22f601882b Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Fri, 14 Aug 2026 16:18:37 +0000 Subject: [PATCH] Isolate Guardian reviewer sessions from parent extensions (#38602) ## What changed - Start Guardian reviewer delegates with an empty extension registry while preserving extension inheritance for other delegates. - Disable `GuardianV2` in the reviewer session configuration so reviews do not trigger nested Guardian processing. - Cover both the reviewer isolation and continued delivery of parent-session tool lifecycle events. GitOrigin-RevId: 858b3e23d1c06177e443fb0779fcf06cc5294fc2 --- codex-rs/core/src/codex_delegate.rs | 10 ++- codex-rs/core/src/codex_delegate_tests.rs | 71 ++++++++++++++++++++ codex-rs/core/src/guardian/review_session.rs | 1 + codex-rs/core/src/guardian/tests.rs | 7 +- codex-rs/core/tests/suite/guardian_review.rs | 35 ++++++++++ 5 files changed, 121 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 3d7dd2e128..5b170720b9 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -75,6 +75,12 @@ pub(crate) async fn run_codex_thread_interactive( instructions: parent_session.user_instructions().await, warnings: Vec::new(), }; + let session_source = SessionSource::SubAgent(subagent_source.clone()); + let extensions = if crate::guardian::is_guardian_reviewer_source(&session_source) { + codex_extension_api::empty_extension_registry() + } else { + Arc::clone(&parent_session.services.extensions) + }; let (session, io) = Box::pin(Session::spawn(SessionSpawnArgs { config, allow_provider_model_fallback: false, @@ -90,11 +96,11 @@ pub(crate) async fn run_codex_thread_interactive( plugins_manager: Arc::clone(&parent_session.services.plugins_manager), mcp_manager: Arc::clone(&parent_session.services.mcp_manager), code_mode_session_provider: parent_session.services.code_mode_service.session_provider(), - extensions: Arc::clone(&parent_session.services.extensions), + extensions, conversation_history, requested_history_mode: None, fork_persistence: ForkPersistence::Copied, - session_source: SessionSource::SubAgent(subagent_source.clone()), + session_source, forked_from_thread_id, parent_thread_id: Some(parent_session.thread_id), thread_source: Some(ThreadSource::Subagent), diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index ce9ded2a68..2de63f4fde 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -1,5 +1,9 @@ use super::*; use async_channel::bounded; +use codex_extension_api::ExtensionFuture; +use codex_extension_api::ExtensionRegistryBuilder; +use codex_extension_api::ThreadLifecycleContributor; +use codex_extension_api::ThreadStartInput; use codex_protocol::error::CodexErrorDetails; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::AgentStatus; @@ -12,9 +16,23 @@ use codex_protocol::protocol::TurnAbortReason; use codex_protocol::protocol::TurnAbortedEvent; use pretty_assertions::assert_eq; use std::sync::Arc; +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering; use tokio::sync::watch; use tokio::time::timeout; +struct ThreadStartRecorder(Arc); + +impl ThreadLifecycleContributor for ThreadStartRecorder { + fn on_thread_start<'a>( + &'a self, + _input: ThreadStartInput<'a, Config>, + ) -> ExtensionFuture<'a, ()> { + self.0.fetch_add(1, Ordering::SeqCst); + Box::pin(std::future::ready(())) + } +} + #[tokio::test] async fn forward_events_filters_private_events_before_blocked_send_is_cancelled() { let (tx_events, rx_events) = bounded(SUBMISSION_CHANNEL_CAPACITY); @@ -198,6 +216,59 @@ async fn run_codex_thread_interactive_respects_pre_cancelled_spawn() { )); } +#[tokio::test] +async fn guardian_delegates_do_not_inherit_parent_extensions() { + let (mut parent_session, parent_ctx, _rx_events) = + crate::session::tests::make_session_and_context_with_rx().await; + let thread_starts = Arc::new(AtomicUsize::new(0)); + let mut extensions = ExtensionRegistryBuilder::::new(); + extensions + .thread_lifecycle_contributor(Arc::new(ThreadStartRecorder(Arc::clone(&thread_starts)))); + Arc::get_mut(&mut parent_session) + .expect("parent session should be uniquely owned") + .services + .extensions = Arc::new(extensions.build()); + + for (subagent_source, expected_thread_starts) in [ + ( + SubAgentSource::Other(crate::guardian::GUARDIAN_REVIEWER_NAME.to_string()), + 0, + ), + (SubAgentSource::Review, 1), + ] { + let mut config = parent_ctx.config.as_ref().clone(); + config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never); + let (session, io) = run_codex_thread_interactive( + config, + Arc::clone(&parent_session.services.auth_manager), + Arc::clone(&parent_session.services.models_manager), + Arc::clone(&parent_session), + Arc::clone(&parent_ctx), + parent_ctx.environments.clone(), + CancellationToken::new(), + subagent_source, + /*initial_history*/ None, + crate::session::GitEnrichmentPolicy::Fresh, + codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile, + ) + .await + .expect("delegate session should start"); + + assert_eq!( + session + .services + .extensions + .thread_lifecycle_contributors() + .len(), + expected_thread_starts + ); + assert_eq!(thread_starts.load(Ordering::SeqCst), expected_thread_starts); + io.shutdown_and_wait() + .await + .expect("delegate session should shut down"); + } +} + #[tokio::test] async fn run_codex_thread_interactive_rejects_approval_policy_that_can_prompt() { let (parent_session, parent_ctx, _rx_events) = diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 1b38bcce2e..623f915b33 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -1344,6 +1344,7 @@ pub(crate) fn build_guardian_review_session_config( for feature in [ Feature::Collab, Feature::MultiAgentV2, + Feature::GuardianV2, Feature::CodexHooks, Feature::Apps, Feature::Plugins, diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 7a8815db74..a2e7e267cc 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -3445,7 +3445,7 @@ async fn guardian_review_session_config_uses_live_network_proxy_state() { } #[tokio::test] -async fn guardian_review_session_config_disables_mcp_apps_plugins_and_memories() { +async fn guardian_review_session_config_disables_mcp_apps_plugins_memories_and_guardian_v2() { let mut parent_config = test_config().await; let server: McpServerConfig = toml::from_str("command = \"docs-server\"").expect("deserialize MCP server"); @@ -3461,6 +3461,10 @@ async fn guardian_review_session_config_disables_mcp_apps_plugins_and_memories() .features .enable(Feature::Plugins) .expect("plugins feature is configurable"); + parent_config + .features + .enable(Feature::GuardianV2) + .expect("guardian v2 feature is configurable"); parent_config.include_apps_instructions = true; parent_config.memories.use_memories = true; parent_config.memories.dedicated_tools = true; @@ -3477,6 +3481,7 @@ async fn guardian_review_session_config_disables_mcp_apps_plugins_and_memories() assert!(guardian_config.mcp_servers.get().is_empty()); assert!(!guardian_config.features.enabled(Feature::Apps)); assert!(!guardian_config.features.enabled(Feature::Plugins)); + assert!(!guardian_config.features.enabled(Feature::GuardianV2)); assert!(!guardian_config.include_apps_instructions); assert!(!guardian_config.memories.use_memories); assert!(!guardian_config.memories.dedicated_tools); diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index 368ad6a9bb..6c940cc56e 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -9,9 +9,14 @@ use codex_core::SleepFuture; use codex_core::TimeFuture; use codex_core::TimeProvider; use codex_core::TurnInputRequest; +use codex_core::config::Config; use codex_core::config::Constrained; use codex_core::config::CurrentTimeReminderConfig; use codex_core::sandboxing::SandboxPermissions; +use codex_extension_api::ExtensionRegistryBuilder; +use codex_extension_api::ToolLifecycleContributor; +use codex_extension_api::ToolLifecycleFuture; +use codex_extension_api::ToolStartInput; use codex_features::CurrentTimeSource; use codex_features::Feature; use codex_history::RolloutItem; @@ -71,6 +76,22 @@ struct RecordingTimeProvider { thread_ids: Mutex>, } +#[derive(Default)] +struct RecordingToolLifecycleContributor { + call_ids: Mutex>, +} + +impl ToolLifecycleContributor for RecordingToolLifecycleContributor { + fn on_tool_start<'a>(&'a self, input: ToolStartInput<'a>) -> ToolLifecycleFuture<'a> { + Box::pin(async move { + self.call_ids + .lock() + .expect("recorded tool call ids lock should not be poisoned") + .push(input.call_id.to_string()); + }) + } +} + impl TimeProvider for RecordingTimeProvider { fn current_time(&self, thread_id: ThreadId) -> TimeFuture<'_> { self.thread_ids @@ -361,7 +382,11 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm const SECRET: &str = "guardian-parent-policy-test-secret"; let server = start_mock_server().await; let approval_policy = AskForApproval::OnRequest; + let lifecycle_recorder = Arc::new(RecordingToolLifecycleContributor::default()); + let mut extensions = ExtensionRegistryBuilder::::new(); + extensions.tool_lifecycle_contributor(lifecycle_recorder.clone()); let mut builder = test_codex() + .with_extensions(Arc::new(extensions.build())) .with_config(move |config| { let secret_file = config.cwd.join("guardian-secret.txt"); config.permissions.approval_policy = Constrained::allow_any(approval_policy); @@ -534,6 +559,16 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm ); assert_eq!(fs::read_to_string(first_output_file)?, "first"); assert_eq!(fs::read_to_string(second_output_file)?, "second"); + assert_eq!( + *lifecycle_recorder + .call_ids + .lock() + .expect("recorded tool call ids lock should not be poisoned"), + vec![ + "exec-call-first".to_string(), + "exec-call-second".to_string() + ] + ); Ok(()) }