mirror of
https://github.com/openai/codex.git
synced 2026-10-21 12:10:06 +00:00
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
This commit is contained in:
@@ -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),
|
||||
|
||||
@@ -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<AtomicUsize>);
|
||||
|
||||
impl ThreadLifecycleContributor<Config> 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::<Config>::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) =
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<Vec<ThreadId>>,
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
struct RecordingToolLifecycleContributor {
|
||||
call_ids: Mutex<Vec<String>>,
|
||||
}
|
||||
|
||||
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::<Config>::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(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user