Decouple session isolation from subagent attribution (#44521)

## Why

Session isolation relied on Guardian source attribution. An explicit policy lets callers control inherited capabilities independently of how a session is attributed.

## What changed

- Add `SessionIsolation` with default `Inherit` and opt-in `Isolated` modes, captured at session startup through `ExtensionDataInit`.
- Use the policy to control inherited instructions, extensions, execution rules, and MCP resources. Isolated sessions retain managed execution rules and omit executor-discovered MCP servers.
- Explicitly isolate Guardian reviewers while preserving source-based fallback for older callers and saved reviewers.

## Testing

Extend delegate tests to cover isolation independently of attribution, update execution-policy coverage to supply the explicit policy, and assert that managed reviewers do not inherit the parent's configured MCP tools.

GitOrigin-RevId: f0ab42fa2f0237860ec661e72693ab191216afbf
This commit is contained in:
jif authored and copyberry committed 2026-09-10 12:52:27 +00:00
1 parent 9688359977
commit ed6dde9fda
14 files changed
+83 -18

No files matched your search

@@ -124,6 +124,12 @@ async fn managed_reviewers_reuse_fork_and_resume_after_parent_shutdown(
// The seed review finished; pause the parent before it requests concurrent approvals. // The seed review finished; pause the parent before it requests concurrent approvals.
timeout(TIMEOUT, server.wait_for_request_count(/*count*/ 3)).await?; timeout(TIMEOUT, server.wait_for_request_count(/*count*/ 3)).await?;
let seed: Value = serde_json::from_slice(&server.requests().await[1])?; let seed: Value = serde_json::from_slice(&server.requests().await[1])?;
assert!(
!seed["tools"]
.to_string()
.contains(&format!("mcp__{TEST_SERVER_NAME}")),
"reviewers must not inherit the parent's configured MCP tools"
);
let reviewer_id = seed["client_metadata"]["thread_id"] let reviewer_id = seed["client_metadata"]["thread_id"]
.as_str() .as_str()
.expect("reviewer ID") .expect("reviewer ID")
+4 -1
View File
@@ -56,6 +56,7 @@ pub(crate) async fn run_codex_thread_interactive(
parent_environments: TurnEnvironmentSnapshot, parent_environments: TurnEnvironmentSnapshot,
cancel_token: CancellationToken, cancel_token: CancellationToken,
subagent_source: SubAgentSource, subagent_source: SubAgentSource,
isolation: codex_extension_api::SessionIsolation,
initial_history: Option<InitialHistory>, initial_history: Option<InitialHistory>,
git_enrichment_policy: GitEnrichmentPolicy, git_enrichment_policy: GitEnrichmentPolicy,
windows_sandbox_proxy_settings_mode: codex_sandboxing::WindowsSandboxProxySettingsMode, windows_sandbox_proxy_settings_mode: codex_sandboxing::WindowsSandboxProxySettingsMode,
@@ -79,7 +80,7 @@ pub(crate) async fn run_codex_thread_interactive(
}; };
let session_source = SessionSource::SubAgent(subagent_source.clone()); let session_source = SessionSource::SubAgent(subagent_source.clone());
let is_guardian_reviewer = crate::guardian::is_basic_session_source(&session_source); let is_guardian_reviewer = crate::guardian::is_basic_session_source(&session_source);
let extensions = if is_guardian_reviewer { let extensions = if isolation == codex_extension_api::SessionIsolation::Isolated {
codex_extension_api::empty_extension_registry() codex_extension_api::empty_extension_registry()
} else { } else {
Arc::clone(&parent_session.services.extensions) Arc::clone(&parent_session.services.extensions)
@@ -87,6 +88,7 @@ pub(crate) async fn run_codex_thread_interactive(
// Inline delegates never register with ThreadManager or receive on_thread_ready. // Inline delegates never register with ThreadManager or receive on_thread_ready.
// Bind their standalone spawn path before inherited extensions run. // Bind their standalone spawn path before inherited extensions run.
let mut thread_extension_init = codex_extension_api::ExtensionDataInit::default(); let mut thread_extension_init = codex_extension_api::ExtensionDataInit::default();
thread_extension_init.insert(isolation);
thread_extension_init.insert(crate::guardian::GuardianReviewSessionHost::default()); thread_extension_init.insert(crate::guardian::GuardianReviewSessionHost::default());
let (session, io) = Session::spawn(SessionSpawnArgs { let (session, io) = Session::spawn(SessionSpawnArgs {
config, config,
@@ -213,6 +215,7 @@ pub(crate) async fn run_codex_thread_one_shot(
parent_environments, parent_environments,
child_cancel.clone(), child_cancel.clone(),
subagent_source, subagent_source,
codex_extension_api::SessionIsolation::Inherit,
initial_history, initial_history,
GitEnrichmentPolicy::Fresh, GitEnrichmentPolicy::Fresh,
codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile, codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile,
+18 -3
View File
@@ -202,6 +202,7 @@ async fn run_codex_thread_interactive_respects_pre_cancelled_spawn() {
parent_environments, parent_environments,
cancel_token, cancel_token,
SubAgentSource::Review, SubAgentSource::Review,
codex_extension_api::SessionIsolation::Inherit,
/*initial_history*/ None, /*initial_history*/ None,
crate::session::GitEnrichmentPolicy::Fresh, crate::session::GitEnrichmentPolicy::Fresh,
codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile, codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile,
@@ -217,7 +218,7 @@ async fn run_codex_thread_interactive_respects_pre_cancelled_spawn() {
} }
#[tokio::test] #[tokio::test]
async fn guardian_delegates_do_not_inherit_parent_extensions() { async fn delegate_isolation_does_not_depend_on_attribution() {
let (mut parent_session, parent_ctx, _rx_events) = let (mut parent_session, parent_ctx, _rx_events) =
crate::session::tests::make_session_and_context_with_rx().await; crate::session::tests::make_session_and_context_with_rx().await;
let thread_starts = Arc::new(AtomicUsize::new(0)); let thread_starts = Arc::new(AtomicUsize::new(0));
@@ -229,13 +230,25 @@ async fn guardian_delegates_do_not_inherit_parent_extensions() {
.services .services
.extensions = Arc::new(extensions.build()); .extensions = Arc::new(extensions.build());
for (subagent_source, expected_thread_starts, expected_thread_source) in [ for (subagent_source, isolation, expected_thread_starts, expected_thread_source) in [
( (
SubAgentSource::Other(crate::guardian::GUARDIAN_REVIEWER_NAME.to_string()), SubAgentSource::Other(crate::guardian::GUARDIAN_REVIEWER_NAME.to_string()),
codex_extension_api::SessionIsolation::Isolated,
0, 0,
ThreadSource::GuardianReview, ThreadSource::GuardianReview,
), ),
(SubAgentSource::Review, 1, ThreadSource::Subagent), (
SubAgentSource::Review,
codex_extension_api::SessionIsolation::Isolated,
0,
ThreadSource::Subagent,
),
(
SubAgentSource::Review,
codex_extension_api::SessionIsolation::Inherit,
1,
ThreadSource::Subagent,
),
] { ] {
let mut config = parent_ctx.config.as_ref().clone(); let mut config = parent_ctx.config.as_ref().clone();
config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never); config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never);
@@ -248,6 +261,7 @@ async fn guardian_delegates_do_not_inherit_parent_extensions() {
parent_ctx.environments.clone(), parent_ctx.environments.clone(),
CancellationToken::new(), CancellationToken::new(),
subagent_source, subagent_source,
isolation,
/*initial_history*/ None, /*initial_history*/ None,
crate::session::GitEnrichmentPolicy::Fresh, crate::session::GitEnrichmentPolicy::Fresh,
codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile, codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile,
@@ -291,6 +305,7 @@ async fn run_codex_thread_interactive_rejects_approval_policy_that_can_prompt()
parent_environments, parent_environments,
CancellationToken::new(), CancellationToken::new(),
SubAgentSource::Review, SubAgentSource::Review,
codex_extension_api::SessionIsolation::Inherit,
/*initial_history*/ None, /*initial_history*/ None,
crate::session::GitEnrichmentPolicy::Fresh, crate::session::GitEnrichmentPolicy::Fresh,
codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile, codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile,
@@ -128,6 +128,7 @@ impl ReviewerSessionFactory for PreparedSession {
self.context.environments().clone(), self.context.environments().clone(),
cancellation.clone(), cancellation.clone(),
SubAgentSource::Other(GUARDIAN_REVIEWER_NAME.to_owned()), SubAgentSource::Other(GUARDIAN_REVIEWER_NAME.to_owned()),
codex_extension_api::SessionIsolation::Isolated,
initial_history, initial_history,
GitEnrichmentPolicy::Skip, GitEnrichmentPolicy::Skip,
codex_sandboxing::WindowsSandboxProxySettingsMode::Preserve, codex_sandboxing::WindowsSandboxProxySettingsMode::Preserve,
@@ -67,7 +67,10 @@ impl ManagedReviewerThreads {
config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never); config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never);
config.model_provider.supports_websockets &= config.model_provider.supports_websockets &=
parent.services.model_client.responses_websocket_enabled(); parent.services.model_client.responses_websocket_enabled();
let mut thread_extension_init = codex_extension_api::ExtensionDataInit::default();
thread_extension_init.insert(codex_extension_api::SessionIsolation::Isolated);
let options = StartThreadOptions { let options = StartThreadOptions {
thread_extension_init,
session_source: Some(SessionSource::Internal(InternalSessionSource::Guardian)), session_source: Some(SessionSource::Internal(InternalSessionSource::Guardian)),
thread_source: Some(ThreadSource::GuardianReview), thread_source: Some(ThreadSource::GuardianReview),
environments: Some(context.environments().to_selections()), environments: Some(context.environments().to_selections()),
+1 -7
View File
@@ -135,12 +135,7 @@ impl Session {
) )
.await; .await;
let mcp_config = self let mcp_config = self
.project_selected_environment_mcp_servers( .project_selected_environment_mcp_servers(config, &environments, mcp_projection)
&session_source,
config,
&environments,
mcp_projection,
)
.await .await
.config; .config;
let local_process_cwd = environments let local_process_cwd = environments
@@ -312,7 +307,6 @@ impl Session {
.await; .await;
let mcp_projection = self let mcp_projection = self
.project_selected_environment_mcp_servers( .project_selected_environment_mcp_servers(
&desired.session_source,
&desired.config, &desired.config,
&desired.environments, &desired.environments,
mcp_projection, mcp_projection,
+1 -3
View File
@@ -143,13 +143,12 @@ impl Session {
/// Adds effective executor-owned configuration from this exact thread snapshot. /// Adds effective executor-owned configuration from this exact thread snapshot.
pub(super) fn project_selected_environment_mcp_servers<'a>( pub(super) fn project_selected_environment_mcp_servers<'a>(
&'a self, &'a self,
session_source: &'a SessionSource,
config: &'a Config, config: &'a Config,
environments: &'a TurnEnvironmentSnapshot, environments: &'a TurnEnvironmentSnapshot,
mut projection: McpRuntimeProjection, mut projection: McpRuntimeProjection,
) -> BoxFuture<'a, McpRuntimeProjection> { ) -> BoxFuture<'a, McpRuntimeProjection> {
Box::pin(async move { Box::pin(async move {
if crate::guardian::is_basic_session_source(session_source) { if self.isolation == codex_extension_api::SessionIsolation::Isolated {
return projection; return projection;
} }
@@ -306,7 +305,6 @@ impl Session {
) { ) {
let mcp_projection = self let mcp_projection = self
.project_selected_environment_mcp_servers( .project_selected_environment_mcp_servers(
&desired.session_source,
&desired.config, &desired.config,
&desired.environments, &desired.environments,
mcp_projection, mcp_projection,
+5 -1
View File
@@ -580,7 +580,11 @@ impl Session {
config config
.startup_warnings .startup_warnings
.extend(user_instruction_provider_warnings); .extend(user_instruction_provider_warnings);
let exec_policy = if crate::guardian::is_basic_session_source(&session_source) { let isolation = thread_extension_init
.get::<codex_extension_api::SessionIsolation>()
.map(|policy| *policy)
.unwrap_or_default();
let exec_policy = if isolation == codex_extension_api::SessionIsolation::Isolated {
let managed_policy = config let managed_policy = config
.config_layer_stack .config_layer_stack
.requirements() .requirements()
+6
View File
@@ -58,6 +58,7 @@ pub(crate) struct Session {
/// session. /// session.
pub(super) features: ManagedFeatures, pub(super) features: ManagedFeatures,
pub(crate) guardian_context_mode: GuardianContextMode, pub(crate) guardian_context_mode: GuardianContextMode,
pub(super) isolation: codex_extension_api::SessionIsolation,
pub(crate) windows_sandbox_proxy_settings_mode: pub(crate) windows_sandbox_proxy_settings_mode:
codex_sandboxing::WindowsSandboxProxySettingsMode, codex_sandboxing::WindowsSandboxProxySettingsMode,
pub(super) multi_agent_version: OnceLock<MultiAgentVersion>, pub(super) multi_agent_version: OnceLock<MultiAgentVersion>,
@@ -860,6 +861,10 @@ impl Session {
// Publish the already resolved model before extensions make startup decisions. // Publish the already resolved model before extensions make startup decisions.
// Turn construction refreshes this attachment when the selected model changes. // Turn construction refreshes this attachment when the selected model changes.
thread_extension_init.insert(model_info); thread_extension_init.insert(model_info);
let isolation = thread_extension_init
.get::<codex_extension_api::SessionIsolation>()
.map(|policy| *policy)
.unwrap_or_default();
let mcp_thread_init = thread_extension_init.clone(); let mcp_thread_init = thread_extension_init.clone();
let thread_extension_data = codex_extension_api::ExtensionData::new_with_init( let thread_extension_data = codex_extension_api::ExtensionData::new_with_init(
thread_id.to_string(), thread_id.to_string(),
@@ -1582,6 +1587,7 @@ impl Session {
managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1), managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1),
features: config.features.clone(), features: config.features.clone(),
guardian_context_mode, guardian_context_mode,
isolation,
windows_sandbox_proxy_settings_mode, windows_sandbox_proxy_settings_mode,
multi_agent_version, multi_agent_version,
mcp_refresh: McpRefresh::new(), mcp_refresh: McpRefresh::new(),
+2
View File
@@ -6701,6 +6701,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1), managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1),
features: config.features.clone(), features: config.features.clone(),
guardian_context_mode: GuardianContextMode::from_features(&config.features), guardian_context_mode: GuardianContextMode::from_features(&config.features),
isolation: codex_extension_api::SessionIsolation::Inherit,
windows_sandbox_proxy_settings_mode: windows_sandbox_proxy_settings_mode:
codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile, codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile,
multi_agent_version: OnceLock::from(config.multi_agent_version_from_features()), multi_agent_version: OnceLock::from(config.multi_agent_version_from_features()),
@@ -8874,6 +8875,7 @@ where
managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1), managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1),
features: config.features.clone(), features: config.features.clone(),
guardian_context_mode: GuardianContextMode::from_features(&config.features), guardian_context_mode: GuardianContextMode::from_features(&config.features),
isolation: codex_extension_api::SessionIsolation::Inherit,
windows_sandbox_proxy_settings_mode: windows_sandbox_proxy_settings_mode:
codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile, codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile,
multi_agent_version: OnceLock::from(config.multi_agent_version_from_features()), multi_agent_version: OnceLock::from(config.multi_agent_version_from_features()),
@@ -1257,6 +1257,8 @@ async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() {
/*state_db*/ None, /*state_db*/ None,
)); ));
let mut thread_extension_init = codex_extension_api::ExtensionDataInit::default();
thread_extension_init.insert(codex_extension_api::SessionIsolation::Isolated);
let (session, io) = Session::spawn(SessionSpawnArgs { let (session, io) = Session::spawn(SessionSpawnArgs {
config, config,
allow_provider_model_fallback: false, allow_provider_model_fallback: false,
@@ -1291,7 +1293,7 @@ async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() {
user_shell_override: None, user_shell_override: None,
parent_trace: None, parent_trace: None,
environment_selections: Vec::new(), environment_selections: Vec::new(),
thread_extension_init: codex_extension_api::ExtensionDataInit::default(), thread_extension_init,
client_mcp_extensions: ClientMcpExtensions::default(), client_mcp_extensions: ClientMcpExtensions::default(),
reserved_thread_id: None, reserved_thread_id: None,
analytics_events_client: None, analytics_events_client: None,
+15 -2
View File
@@ -1932,13 +1932,26 @@ impl ThreadManagerState {
environments, environments,
inherited_environments: captured_environments, inherited_environments: captured_environments,
user_instructions: supplied_user_instructions, user_instructions: supplied_user_instructions,
thread_extension_init, mut thread_extension_init,
client_mcp_extensions, client_mcp_extensions,
reserved_thread_id, reserved_thread_id,
disabled_plugin_ids, disabled_plugin_ids,
} = options; } = options;
let inherited_environments = captured_environments.or(inherited_environments); let inherited_environments = captured_environments.or(inherited_environments);
let session_source = session_source.unwrap_or_else(|| self.session_source.clone()); let session_source = session_source.unwrap_or_else(|| self.session_source.clone());
// Older callers and saved reviewers identify isolation through their source.
// New internal callers supply an explicit runtime policy before startup.
let isolation = thread_extension_init
.get::<codex_extension_api::SessionIsolation>()
.map(|policy| *policy)
.unwrap_or_else(|| {
if crate::guardian::is_basic_session_source(&session_source) {
codex_extension_api::SessionIsolation::Isolated
} else {
codex_extension_api::SessionIsolation::Inherit
}
});
thread_extension_init.insert(isolation);
let environments = environments.unwrap_or_else(|| { let environments = environments.unwrap_or_else(|| {
default_thread_environment_selections( default_thread_environment_selections(
self.environment_manager.as_ref(), self.environment_manager.as_ref(),
@@ -1989,7 +2002,7 @@ impl ThreadManagerState {
extensions, extensions,
mcp_manager, mcp_manager,
multi_agent_version, multi_agent_version,
) = if crate::guardian::is_basic_session_source(&session_source) { ) = if isolation == codex_extension_api::SessionIsolation::Isolated {
( (
LoadedUserInstructions::default(), LoadedUserInstructions::default(),
None, None,
+3
View File
@@ -1,10 +1,13 @@
mod capabilities; mod capabilities;
mod contributors; mod contributors;
mod registry; mod registry;
mod session_isolation;
mod state; mod state;
mod turn_admission; mod turn_admission;
mod user_instructions; mod user_instructions;
pub use session_isolation::SessionIsolation;
pub use capabilities::AgentSpawnFuture; pub use capabilities::AgentSpawnFuture;
pub use capabilities::AgentSpawner; pub use capabilities::AgentSpawner;
pub use capabilities::ConversationHistorySnapshot; pub use capabilities::ConversationHistorySnapshot;
@@ -0,0 +1,15 @@
//! Host-supplied isolation for internal runtimes, independent of their attribution.
//! Isolation only removes inherited capabilities; it never grants review authority.
/// Runtime policy supplied through `ExtensionDataInit` before session startup.
/// The host captures this value once so later extension-state changes cannot alter it.
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
pub enum SessionIsolation {
/// Use the host's ordinary instruction providers, extensions and execution rules.
#[default]
Inherit,
/// Start without inherited instruction providers or extensions, retain only managed
/// execution rules, and omit executor-discovered MCP servers. Explicitly supplied
/// instructions and permissions remain the responsibility of the internal caller.
Isolated,
}