From 76dcb8199b408793803961d5e17a72666a183fe8 Mon Sep 17 00:00:00 2001 From: Dmitriy Kovalenko Date: Mon, 9 Nov 2026 21:32:34 +0000 Subject: [PATCH] Honor permission grant scopes in executor capability discovery (#60461) ## Why Approved filesystem access should make executor skills discoverable and readable for the lifetime of the grant. Turn-scoped access must not authorize persistent plugin servers or hooks, or bypass disabled-plugin policy. ## What changed - Apply environment-specific session grants to capability discovery and refresh MCP after session grants change. - Use a separate discovery inventory for skills with turn-scoped grants, keeping plugin servers and hooks on the session inventory. Reuse discovery results across unchanged model steps. - Preserve disabled-plugin checks for unreadable manifests and symlinked manifest files or directories, including ancestor markers, while retaining standalone skill roots. ## Testing Add coverage for V1 and V2 skill discovery before approval, after approval, and on the next turn; disabled-plugin ownership checks; discovery cache reuse; and session hooks surviving turn-scoped discovery failure. GitOrigin-RevId: 929a7f8cb01f4bcfb4bee2c50ae02cd762066b2d --- .../core-plugins/src/executor_provider.rs | 117 ++-- .../src/executor_provider_tests.rs | 85 +-- codex-rs/core/src/mcp.rs | 7 +- codex-rs/core/src/session/mcp.rs | 68 ++- codex-rs/core/src/session/mod.rs | 105 +++- codex-rs/core/src/session/tests.rs | 1 + codex-rs/core/src/state/service.rs | 15 + codex-rs/core/tests/common/exec_server.rs | 12 +- .../core/tests/suite/executor_discovery_v2.rs | 150 ++++- codex-rs/core/tests/suite/hooks_executor.rs | 138 ++++- codex-rs/core/tests/suite/skills_extension.rs | 3 + .../discovery_grants_tests.rs | 519 ++++++++++++++++++ .../ext/extension-api/src/contributors/mcp.rs | 15 + codex-rs/ext/mcp/src/plugin.rs | 9 +- codex-rs/ext/mcp/tests/plugin_mcp.rs | 6 +- 15 files changed, 1117 insertions(+), 133 deletions(-) create mode 100644 codex-rs/core/tests/suite/skills_extension/discovery_grants_tests.rs diff --git a/codex-rs/core-plugins/src/executor_provider.rs b/codex-rs/core-plugins/src/executor_provider.rs index 78a66b1ad6..d5915475e8 100644 --- a/codex-rs/core-plugins/src/executor_provider.rs +++ b/codex-rs/core-plugins/src/executor_provider.rs @@ -17,6 +17,7 @@ use codex_utils_path_uri::PathUri; use codex_utils_path_uri::PathUriParseError; use codex_utils_plugins::DISCOVERABLE_PLUGIN_MANIFEST_PATHS; use futures::StreamExt; +use futures::TryStreamExt; use std::io; use std::sync::Arc; use thiserror::Error; @@ -152,9 +153,6 @@ impl ExecutorPluginProvider { { return Ok(PluginRootOwnership::Plugin); } - if discovery.warnings.is_empty() { - return Ok(PluginRootOwnership::Standalone); - } } let environment = self .environment_manager @@ -164,10 +162,14 @@ impl ExecutorPluginProvider { environment_id: environment_id.clone(), })?; let file_system = environment.get_filesystem(); + // Warning-free discovery can still omit hidden ancestor manifests. let manifest = find_manifest( selected_root, std::iter::successors(Some(path.clone()), PathUri::parent), file_system.as_ref(), + GetMetadataOptions { + follow_symlinks: false, + }, sandbox, ) .await?; @@ -241,6 +243,7 @@ async fn resolve_plugin_root( selected_root, std::iter::once(plugin_root.clone()), file_system, + GetMetadataOptions::default(), /*sandbox*/ None, ) .await? @@ -285,48 +288,92 @@ async fn resolve_plugin_root( async fn find_manifest( selected_root: &SelectedCapabilityRoot, - mut plugin_roots: impl Iterator + Send, + plugin_roots: impl Iterator + Send, file_system: &dyn ExecutorFileSystem, + metadata_options: GetMetadataOptions, sandbox: Option<&FileSystemSandboxContext>, ) -> Result, ExecutorPluginProviderError> { let root_id = &selected_root.id; - let mut plugin_root = plugin_roots.next(); - let mut marker_index = 0; - let probes = std::iter::from_fn(move || { - let root = plugin_root.clone()?; - let relative_path = *DISCOVERABLE_PLUGIN_MANIFEST_PATHS.get(marker_index)?; - let candidate_uri = root.join(relative_path).map_err(|source| { - ExecutorPluginProviderError::InvalidManifestPath { - root_id: root_id.clone(), - root, - relative_path, - source, + let roots = futures::stream::iter(plugin_roots) + .map(|root| async move { + // Keep lexical ancestors, but resolve aliases before no-follow marker probes. + // Native anchors need no resolution; `file:///C:` converts to a drive-relative path. + if metadata_options.follow_symlinks || root.parent().is_none() { + Ok(root) + } else { + file_system + .canonicalize(&root, sandbox) + .await + .map_err(|source| ExecutorPluginProviderError::InspectRoot { + root_id: root_id.clone(), + path: root.clone(), + source, + }) } - }); - marker_index += 1; - if marker_index == DISCOVERABLE_PLUGIN_MANIFEST_PATHS.len() { - marker_index = 0; - plugin_root = plugin_roots.next(); - } - Some(candidate_uri) - }); + }) + .buffered(MAX_CONCURRENT_MANIFEST_PROBES); + let probes = roots + .map_ok(|root| { + futures::stream::iter( + (0..DISCOVERABLE_PLUGIN_MANIFEST_PATHS.len()) + .map(move |index| Ok((root.clone(), index))), + ) + }) + .try_flatten(); // Pipeline remote metadata calls while consuming results in ancestor/manifest priority order. - let mut results = futures::stream::iter(probes) - .map(|candidate_uri| async move { - let candidate_uri = candidate_uri?; + let mut results = probes + .map(|probe| async move { + let (root, marker_index) = probe?; + let relative_path = DISCOVERABLE_PLUGIN_MANIFEST_PATHS[marker_index]; + let candidate_uri = root.join(relative_path).map_err(|source| { + ExecutorPluginProviderError::InvalidManifestPath { + root_id: root_id.clone(), + root, + relative_path, + source, + } + })?; match file_system - .get_metadata(&candidate_uri, GetMetadataOptions::default(), sandbox) + .get_metadata(&candidate_uri, metadata_options, sandbox) .await { - Ok(metadata) if metadata.is_file => Ok(Some(candidate_uri)), - Ok(_) => Ok(None), - Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(None), - Err(source) => Err(ExecutorPluginProviderError::InspectManifest { - root_id: root_id.clone(), - path: candidate_uri, - source, - }), + Ok(metadata) + if metadata.is_file + || (!metadata_options.follow_symlinks && metadata.is_symlink) => + { + return Ok(Some(candidate_uri)); + } + Ok(_) => {} + Err(err) if err.kind() == io::ErrorKind::NotFound => {} + Err(source) => { + return Err(ExecutorPluginProviderError::InspectManifest { + root_id: root_id.clone(), + path: candidate_uri, + source, + }); + } } + // A symlinked marker directory can hide its manifest, including in an ancestor. + if !metadata_options.follow_symlinks + && let Some(directory) = candidate_uri.parent() + { + match file_system + .get_metadata(&directory, metadata_options, sandbox) + .await + { + Ok(metadata) if metadata.is_symlink => return Ok(Some(candidate_uri)), + Ok(_) => {} + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(source) => { + return Err(ExecutorPluginProviderError::InspectManifest { + root_id: root_id.clone(), + path: directory, + source, + }); + } + } + } + Ok(None) }) .buffered(MAX_CONCURRENT_MANIFEST_PROBES); while let Some(result) = results.next().await { diff --git a/codex-rs/core-plugins/src/executor_provider_tests.rs b/codex-rs/core-plugins/src/executor_provider_tests.rs index a6609ab727..fa76c4c187 100644 --- a/codex-rs/core-plugins/src/executor_provider_tests.rs +++ b/codex-rs/core-plugins/src/executor_provider_tests.rs @@ -250,6 +250,13 @@ async fn root_ownership_survives_malformed_ancestor_manifests() { let directory = tempdir().expect("tempdir"); let root = directory.path().join("package/skills"); fs::create_dir_all(&root).expect("create skills root"); + #[cfg(unix)] + let root = { + let alias = directory.path().join("owning-package/skills"); + fs::create_dir_all(alias.parent().unwrap()).expect("create owning package"); + std::os::unix::fs::symlink(&root, &alias).expect("symlink skills"); + alias + }; let provider = ExecutorPluginProvider::new(Arc::new(EnvironmentManager::default_for_tests())); let selected = selected_root("example@marketplace", LOCAL_ENVIRONMENT_ID, &root); assert_eq!( @@ -300,9 +307,8 @@ async fn root_ownership_survives_malformed_ancestor_manifests() { async fn discovery_ownership_uses_ancestors_not_nested_plugins() { let directory = tempdir().expect("tempdir"); let path = PathUri::from_host_native_path(directory.path()).unwrap(); - let selected = selected_root_uri("example@marketplace", "executor", path.clone()); - let provider = - ExecutorPluginProvider::new(Arc::new(environment_manager_without_environments())); + let selected = selected_root_uri("example@marketplace", LOCAL_ENVIRONMENT_ID, path.clone()); + let provider = ExecutorPluginProvider::new(Arc::new(EnvironmentManager::default_for_tests())); let mut discovery = CapabilityRootDiscovery { id: selected.id.clone(), path: path.clone(), @@ -330,6 +336,8 @@ async fn discovery_ownership_uses_ancestors_not_nested_plugins() { .unwrap(), PluginRootOwnership::Plugin ); + let provider = + ExecutorPluginProvider::new(Arc::new(environment_manager_without_environments())); assert!(matches!( provider .root_ownership(&selected, /*discovery*/ None, /*sandbox*/ None) @@ -377,6 +385,7 @@ async fn ancestor_manifest_probes_pipeline_and_preserve_error_order() { &selected, std::iter::successors(Some(root.clone()), PathUri::parent), &file_system, + GetMetadataOptions::default(), /*sandbox*/ None, )); assert!(futures::poll!(resolution.as_mut()).is_pending()); @@ -407,42 +416,46 @@ async fn ancestor_manifest_probes_pipeline_and_preserve_error_order() { #[tokio::test] async fn manifest_presence_uses_sandboxed_metadata_without_reading_contents() { - let directory = tempdir().expect("tempdir"); - let plugin_root = PathUri::from_host_native_path(directory.path()).expect("root URI"); + let plugin_root = PathUri::parse("file:///C:").expect("Windows anchor URI"); let manifest_path = plugin_root .join(".codex-plugin/plugin.json") .expect("manifest URI"); - let file_system = SyntheticPluginFileSystem { - plugin_root: plugin_root.clone(), - manifest_path: manifest_path.clone(), - calls: Mutex::new(Vec::new()), - metadata_gate: None, - metadata_delay: None, - metadata_error: None, - }; - let sandbox = - FileSystemSandboxContext::from_permission_profile(Default::default(), plugin_root.clone()); - assert_eq!( - find_manifest( - &selected_root_uri("selected-demo", "executor-test", plugin_root.clone()), - std::iter::once(plugin_root.clone()), - &file_system, - Some(&sandbox), - ) - .await - .expect("probe manifest"), - Some(manifest_path.clone()), - ); - assert_eq!( - *file_system - .calls - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner), - vec![FileSystemCall::SandboxedMetadata { - path: manifest_path, - sandbox: Box::new(sandbox) - }], - ); + for follow_symlinks in [true, false] { + let file_system = SyntheticPluginFileSystem { + plugin_root: plugin_root.clone(), + manifest_path: manifest_path.clone(), + calls: Mutex::new(Vec::new()), + metadata_gate: None, + metadata_delay: None, + metadata_error: None, + }; + let sandbox = FileSystemSandboxContext::from_permission_profile( + Default::default(), + plugin_root.clone(), + ); + assert_eq!( + find_manifest( + &selected_root_uri("selected-demo", "executor-test", plugin_root.clone()), + std::iter::once(plugin_root.clone()), + &file_system, + GetMetadataOptions { follow_symlinks }, + Some(&sandbox), + ) + .await + .expect("probe manifest"), + Some(manifest_path.clone()), + ); + assert_eq!( + *file_system + .calls + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner), + vec![FileSystemCall::SandboxedMetadata { + path: manifest_path.clone(), + sandbox: Box::new(sandbox) + }], + ); + } } #[tokio::test] diff --git a/codex-rs/core/src/mcp.rs b/codex-rs/core/src/mcp.rs index ce3f0b26ad..a6270a6ccf 100644 --- a/codex-rs/core/src/mcp.rs +++ b/codex-rs/core/src/mcp.rs @@ -250,12 +250,13 @@ impl McpManager { } /// Resolves only plugins from this step's ready roots, independent of shared MCP refresh. - pub(crate) async fn selected_plugins_for_step( + pub(crate) async fn selected_plugins_for_step<'a>( &self, - context: McpServerContributionContext<'_, Config>, + context: McpServerContributionContext<'a, Config>, plugins_config: &PluginsConfigToml, - disabled_plugin_ids: &[String], + disabled_plugin_ids: &'a [String], ) -> SelectedPluginSnapshot { + let context = context.with_disabled_plugin_ids(disabled_plugin_ids); let mut selected = SelectedPluginSnapshot { disabled_plugin_ids: disabled_plugin_ids.to_vec(), ..Default::default() diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index c1e6f2ba17..e8cb500650 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -109,7 +109,11 @@ impl Session { let ready_selected_capability_roots = Self::ready_selected_capability_roots(&selected_capability_roots); let (executor_capability_discovery_v2, inventory_changed) = self - .executor_capability_discovery_for_step_v2(config, &environments) + .executor_capability_discovery_for_step_v2( + config, + &environments, + /*skill_turn*/ None, + ) .await; let (executor_capability_discovery, discovery_recovered) = self .executor_capability_discovery_for_step( @@ -117,6 +121,7 @@ impl Session { &selected_capability_roots, &environments, executor_capability_discovery_v2.as_deref(), + /*skill_turn*/ None, ) .await; if inventory_changed || discovery_recovered { @@ -229,7 +234,11 @@ impl Session { let ready_selected_capability_roots = Self::ready_selected_capability_roots(&selected_capability_roots); let (executor_capability_discovery_v2, _) = self - .executor_capability_discovery_for_step_v2(&desired.config, &desired.environments) + .executor_capability_discovery_for_step_v2( + &desired.config, + &desired.environments, + /*skill_turn*/ None, + ) .await; let (executor_capability_discovery, _) = self .executor_capability_discovery_for_step( @@ -237,6 +246,7 @@ impl Session { &selected_capability_roots, &desired.environments, executor_capability_discovery_v2.as_deref(), + /*skill_turn*/ None, ) .await; let mcp_projection = self @@ -312,7 +322,11 @@ impl Session { let ready_selected_capability_roots = Self::ready_selected_capability_roots(&selected_capability_roots); let (executor_capability_discovery_v2, _) = self - .executor_capability_discovery_for_step_v2(&desired.config, &desired.environments) + .executor_capability_discovery_for_step_v2( + &desired.config, + &desired.environments, + /*skill_turn*/ None, + ) .await; let (executor_capability_discovery, _) = self .executor_capability_discovery_for_step( @@ -320,6 +334,7 @@ impl Session { &selected_capability_roots, &desired.environments, executor_capability_discovery_v2.as_deref(), + /*skill_turn*/ None, ) .await; let mcp_projection = self @@ -462,6 +477,19 @@ impl Session { Arc::new(codex_mcp::McpBinding::empty(config)) } + fn discovery_sandbox( + &self, + environment: &super::turn_context::TurnEnvironment, + skill_turn: Option<&TurnContext>, + ) -> codex_exec_server::FileSystemSandboxContext { + let id = &environment.selection.environment_id; + let grants = skill_turn.map_or_else( + || self.services.granted_permissions(id), + |turn| turn.granted_permissions(id), + ); + environment.sandbox_context(grants) + } + #[tracing::instrument( name = "capability_roots.snapshot_for_step", skip_all, @@ -473,6 +501,7 @@ impl Session { resolved_roots: &[ResolvedSelectedCapabilityRoot], environments: &TurnEnvironmentSnapshot, v2_snapshots: Option<&[ExecutorCapabilityDiscoveryV2Snapshot]>, + skill_turn: Option<&TurnContext>, ) -> (Option>, bool) { if v2_snapshots.is_some() { return (None, false); @@ -506,16 +535,19 @@ impl Session { .map(|environment| { ( environment.selection.environment_id.clone(), - environment.sandbox_context(/*additional_permissions*/ None), + self.discovery_sandbox(environment, skill_turn), ) }) .collect::>() } else { HashMap::new() }; - let cache = self - .services - .thread_extension_data + // Each scope retains one result per root; temporary grants must not evict the + // session inventory on every model step. The turn cache expires with its grants. + let cache = skill_turn + .map_or(&self.services.thread_extension_data, |turn| { + turn.extension_data.as_ref() + }) .get_or_init(ExecutorCapabilityDiscoveryCache::default); let selected_capability_roots = resolved_roots .iter() @@ -540,7 +572,10 @@ impl Session { let discovery = cache .snapshot(&selected_capability_roots, &sandbox_contexts) .await; - (Some(Arc::new(discovery)), cache.take_recovered_discovery()) + ( + Some(Arc::new(discovery)), + skill_turn.is_none() && cache.take_recovered_discovery(), + ) } /// Acquires V2 inventories and reports changes without scheduling an MCP refresh. @@ -548,6 +583,7 @@ impl Session { &self, config: &Config, environments: &TurnEnvironmentSnapshot, + skill_turn: Option<&TurnContext>, ) -> (Option>, bool) { if !config .features @@ -588,7 +624,7 @@ impl Session { } let request = DiscoverV2CapabilitiesRequest { cwd: selected.cwd().clone(), - sandbox: Some(selected.sandbox_context(/*additional_permissions*/ None)), + sandbox: Some(self.discovery_sandbox(selected, skill_turn)), }; let result = cache .discover(environment_id, selected.environment(), request) @@ -598,7 +634,12 @@ impl Session { result, }); } - if snapshots.is_empty() && skipped_incompatible_executor { + let incompatible = snapshots.is_empty() && skipped_incompatible_executor; + if skill_turn.is_some() { + // Only session discovery updates the inventory used to invalidate MCP. + return ((!incompatible).then_some(snapshots), false); + } + if incompatible { let changed = self .services .thread_extension_data @@ -871,7 +912,11 @@ impl Session { let environment_selections = environments.all_selections(); let desired = self.latest_mcp_desired_state(auth, environments).await; let (executor_capability_discovery_v2, _) = self - .executor_capability_discovery_for_step_v2(&desired.config, &desired.environments) + .executor_capability_discovery_for_step_v2( + &desired.config, + &desired.environments, + /*skill_turn*/ None, + ) .await; let (executor_capability_discovery, _) = self .executor_capability_discovery_for_step( @@ -879,6 +924,7 @@ impl Session { &selected_capability_roots, &desired.environments, executor_capability_discovery_v2.as_deref(), + /*skill_turn*/ None, ) .await; let mcp_projection = self diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 814799a59f..171e0a4e1d 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -3816,6 +3816,7 @@ impl Session { environment_id, response.permissions.clone().into(), ); + self.mark_mcp_runtime_dirty(); } } } @@ -4365,7 +4366,11 @@ impl Session { let ready_selected_capability_roots = Self::ready_selected_capability_roots(&selected_capability_roots); let (executor_capability_discovery_v2, inventory_changed) = self - .executor_capability_discovery_for_step_v2(&turn_context.config, &environments) + .executor_capability_discovery_for_step_v2( + &turn_context.config, + &environments, + /*skill_turn*/ None, + ) .await; let v2_snapshots = executor_capability_discovery_v2.as_ref(); let (executor_capability_discovery, discovery_recovered) = if v2_snapshots.is_some() { @@ -4376,6 +4381,7 @@ impl Session { &selected_capability_roots, &environments, executor_capability_discovery_v2.as_deref(), + /*skill_turn*/ None, ) .await }; @@ -4392,14 +4398,37 @@ impl Session { self.host_skills_for_step(&turn_context, &environments) .await, ); - if turn_context - .config - .features - .enabled(Feature::SkillCatalogDeltas) - && let Some(grants) = turn_context.turn_skill_filesystem_grants(&environments) - { - extension_data.insert(grants); - } + // Only skill discovery may use a turn's temporary file access. Plugin servers and + // hooks use the session inventory above because they can outlive the step. + let turn_skill_discovery = + if let Some(grants) = turn_context.turn_skill_filesystem_grants(&environments) { + if turn_context + .config + .features + .enabled(Feature::SkillCatalogDeltas) + { + extension_data.insert(grants); + } + let (v2, _) = self + .executor_capability_discovery_for_step_v2( + &turn_context.config, + &environments, + Some(&turn_context), + ) + .await; + let (v1, _) = self + .executor_capability_discovery_for_step( + &turn_context.config, + &selected_capability_roots, + &environments, + v2.as_deref(), + Some(&turn_context), + ) + .await; + Some((v1, v2)) + } else { + None + }; if let Some(messages) = settings .model_info .model_messages @@ -4462,28 +4491,53 @@ impl Session { mcp }; extension_data.insert(selected_capability_roots); - // A step keeps the plugins from the environments it captured, even if shared MCP - // moves on to another environment. Its skill tools and the model use this same copy. - // Policy follows this step's MCP binding, which may have refreshed mid-turn. - let selected_plugins = self - .services - .mcp_manager - .selected_plugins_for_step( + let selected_environments = environments.all_selections(); + let select_plugins = |discovery, discovery_v2| { + self.services.mcp_manager.selected_plugins_for_step( codex_extension_api::McpServerContributionContext::for_step( turn_context.config.as_ref(), &self.services.mcp_thread_init, &self.services.thread_extension_data, &turn_context.originator, &ready_selected_capability_roots, - executor_capability_discovery.as_deref(), - executor_capability_discovery_v2.as_deref(), + discovery, + discovery_v2, ) .with_session_source(&turn_context.session_source) - .with_selected_environments(&environments.all_selections()), + .with_selected_environments(&selected_environments), &mcp.config().plugins, &turn_context.disabled_plugin_ids, ) - .await; + }; + // Resolve skill ownership from the same inventory as skill discovery. + let (skill_discovery, skill_discovery_v2) = match &turn_skill_discovery { + Some((v1, v2)) => (v1.as_deref(), v2.as_deref()), + None => ( + executor_capability_discovery.as_deref(), + executor_capability_discovery_v2.as_deref(), + ), + }; + let selected_plugins = select_plugins(skill_discovery, skill_discovery_v2).await; + // V2 hooks match accepted installations from the session inventory. V1 hooks + // only need the explicit disabled IDs, already present in selected_plugins. + let session_plugins = + if turn_skill_discovery.is_some() && executor_capability_discovery_v2.is_some() { + Some( + select_plugins( + executor_capability_discovery.as_deref(), + executor_capability_discovery_v2.as_deref(), + ) + .await, + ) + } else { + None + }; + let executor_hook_sources = crate::hook_runtime::prepare_executor_hook_sources( + &mcp, + session_plugins.as_ref().unwrap_or(&selected_plugins), + executor_capability_discovery.as_deref(), + executor_capability_discovery_v2.as_deref(), + ); let mut filesystem_access = HashMap::new(); for environment in environments.turn_environments() { let environment_id = &environment.selection.environment_id; @@ -4508,9 +4562,8 @@ impl Session { .prepare_for_step(codex_extension_api::StepPreparationInput { turn_id: &turn_context.sub_id, ready_selected_capability_roots: &accessed_roots, - executor_capability_discovery: executor_capability_discovery.as_deref(), - executor_capability_discovery_v2: executor_capability_discovery_v2 - .as_deref(), + executor_capability_discovery: skill_discovery, + executor_capability_discovery_v2: skill_discovery_v2, selected_plugins: &selected_plugins, session_store: &self.services.session_extension_data, thread_store: &self.services.thread_extension_data, @@ -4520,12 +4573,6 @@ impl Session { .or_cancel(cancellation_token) .await?; } - let executor_hook_sources = crate::hook_runtime::prepare_executor_hook_sources( - &mcp, - &selected_plugins, - executor_capability_discovery.as_deref(), - executor_capability_discovery_v2.as_deref(), - ); extension_data.insert(selected_plugins); let tool_router = turn::built_tools( self.as_ref(), diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index c332c8d920..8faddfd4bb 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -10290,6 +10290,7 @@ async fn capability_discovery_uses_environment_permission_profile() { /*resolved_roots*/ &[], &turn_context.initial_environments, /*v2_snapshots*/ None, + /*skill_turn*/ None, ) .await; let discovery = discovery.expect("restricted environment should trigger capability discovery"); diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 2bb25c7557..54aee82bb0 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -108,6 +108,21 @@ pub(crate) struct SessionServices { } impl SessionServices { + #[expect( + clippy::expect_used, + reason = "poisoned grants must not authorize discovery" + )] + pub(crate) fn granted_permissions( + &self, + environment_id: &str, + ) -> Option { + self.granted_permissions_by_environment_id + .lock() + .expect("session permission grants lock poisoned") + .get(environment_id) + .cloned() + } + #[expect( clippy::expect_used, reason = "poisoned grant state must not authorize further operations" diff --git a/codex-rs/core/tests/common/exec_server.rs b/codex-rs/core/tests/common/exec_server.rs index 70f321cec0..58c75fa229 100644 --- a/codex-rs/core/tests/common/exec_server.rs +++ b/codex-rs/core/tests/common/exec_server.rs @@ -36,7 +36,10 @@ impl ExecServerProcess { } pub async fn start_with_version(version: &str) -> Result { - let codex_home = TempDir::new()?; + Self::start_with_version_and_home(version, TempDir::new()?).await + } + + pub async fn start_with_version_and_home(version: &str, codex_home: TempDir) -> Result { let package = codex_home.path().join("package"); let bin = package.join("bin"); std::fs::create_dir_all(&bin)?; @@ -49,7 +52,12 @@ impl ExecServerProcess { package.join("codex-package.json"), serde_json::to_vec(&serde_json::json!({"version": version}))?, )?; - Self::start_command(Command::new(executable), codex_home).await + let mut command = Command::new(executable); + // V2 must not discover skills installed in the developer's home directory. + command + .env("HOME", codex_home.path()) + .env("USERPROFILE", codex_home.path()); + Self::start_command(command, codex_home).await } async fn start_command(mut command: Command, codex_home: TempDir) -> Result { diff --git a/codex-rs/core/tests/suite/executor_discovery_v2.rs b/codex-rs/core/tests/suite/executor_discovery_v2.rs index 73d6d45d0c..f536332577 100644 --- a/codex-rs/core/tests/suite/executor_discovery_v2.rs +++ b/codex-rs/core/tests/suite/executor_discovery_v2.rs @@ -1,4 +1,4 @@ -//! Verifies V2 discovery, plugin acceptance across auth changes, and cache behavior. +//! Verifies executor discovery, plugin acceptance across auth changes, and cache behavior. use anyhow::Context; use anyhow::Result; @@ -193,22 +193,24 @@ impl ContextContributor for DiscoveryRecorder { } } -struct Executor { - url: String, - inventory: Arc>>, - requests: Arc>>, +pub(super) struct Executor { + pub(super) url: String, + pub(super) inventory: Arc>>, + pub(super) requests: Arc>>, _task: AbortOnDropHandle>, } #[derive(Clone, Copy)] -enum DiscoveryFailure { +pub(super) enum DiscoveryFailure { None, Server, Disconnect, + #[cfg_attr(windows, expect(dead_code, reason = "executor hook tests run on Unix"))] + TurnGrant, } impl Executor { - async fn start(supports_v2: bool, failure: DiscoveryFailure) -> Result { + pub(super) async fn start(supports_v2: bool, failure: DiscoveryFailure) -> Result { Self::start_with_version(supports_v2, failure, "0.164.0").await } @@ -265,23 +267,27 @@ impl Executor { match failure { // Exhaust the environment's one transport replay so the cache sees Err. DiscoveryFailure::Disconnect if attempt < 2 => break, + DiscoveryFailure::TurnGrant if request["params"]["sandbox"].to_string().contains("turn-only-skills") => { + socket.send(Message::Text(json!({"id":request["id"],"error":{"code":-32004,"message":"turn discovery failed"}}).to_string().into())).await?; + continue; + } DiscoveryFailure::Server if attempt == 0 => { socket.send(Message::Text(json!({"id":request["id"],"error":{"code":-32004,"message":"fixture discovery failure"}}).to_string().into())).await?; continue; } - DiscoveryFailure::None | DiscoveryFailure::Server | DiscoveryFailure::Disconnect => {} + DiscoveryFailure::None | DiscoveryFailure::Server | DiscoveryFailure::Disconnect | DiscoveryFailure::TurnGrant => {} } } let result = match request["method"].as_str() { Some("initialize") => Some(json!({"sessionId":"acquisition-only", "environmentInfo": { "shell":{"name":"bash","path":"/bin/bash"}, "executorVersion":version, - "capabilities":{"sandboxedFileStreaming":true,"capabilityDiscoveryV2":supports_v2} + "capabilities":{"sandboxedFileStreaming":true,"capabilityDiscoverySandbox":true,"capabilityDiscoveryV2":supports_v2} }})), Some("environment/info") => Some(json!({ "shell":{"name":"bash","path":"/bin/bash"}, "executorVersion":version, - "capabilities":{"sandboxedFileStreaming":true,"capabilityDiscoveryV2":supports_v2} + "capabilities":{"sandboxedFileStreaming":true,"capabilityDiscoverySandbox":true,"capabilityDiscoveryV2":supports_v2} })), Some("capabilityRoots/discoverV1") => Some(json!({ "roots": request["params"]["roots"].as_array().context("V1 roots")? @@ -537,7 +543,9 @@ async fn v2_discovery_failures_retry_or_remain_cached_by_error_kind() -> Result< // Two disconnected attempts, then one shared successful retry in the same cache. 3 } - DiscoveryFailure::None => unreachable!("the test only exercises failures"), + DiscoveryFailure::None | DiscoveryFailure::TurnGrant => { + unreachable!("the test only exercises startup failures") + } }; assert_eq!( executor @@ -840,6 +848,126 @@ async fn check_acquisition(enabled: bool, version: &'static str) -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn v1_reuses_session_and_turn_discovery_across_model_steps() -> Result<()> { + skip_if_no_network!(Ok(())); + let executor = Executor::start(/*supports_v2*/ false, DiscoveryFailure::None).await?; + let model = responses::start_mock_server().await; + let test = test_codex() + .with_exec_server_url(executor.url.clone()) + .with_config(|config| { + config.update_plan_enabled = true; + config + .features + .enable(Feature::RequestPermissionsTool) + .unwrap(); + config + .features + .disable(Feature::ExecutorCapabilityDiscoveryV2) + .unwrap(); + config.permissions.approval_policy = codex_core::config::Constrained::allow_any( + codex_protocol::protocol::AskForApproval::OnRequest, + ); + config + .permissions + .set_permission_profile( + codex_protocol::models::PermissionProfile::from_runtime_permissions( + &codex_protocol::permissions::FileSystemSandboxPolicy::restricted( + Vec::new(), + ), + codex_protocol::permissions::NetworkSandboxPolicy::Restricted, + ), + ) + .unwrap(); + }) + .build_with_auto_env(&model) + .await?; + let selection = test.codex.environment_selections().await.remove(0); + let mut config = + core_test_support::test_codex::environment_config_for_selection(&test.config, &selection); + config.selected_capability_roots = vec![codex_protocol::capabilities::SelectedCapabilityRoot { + id: "legacy-root".to_string(), + location: codex_protocol::capabilities::CapabilityRootLocation::Environment { + environment_id: selection.environment_id.clone(), + path: selection.cwd.clone(), + }, + }]; + test.codex.environment_ready(&selection, config).await?; + let permissions = codex_protocol::request_permissions::RequestPermissionProfile { + file_system: Some( + codex_protocol::models::FileSystemPermissions::from_read_write_path_uris( + Some(vec![selection.cwd.join("approved")?]), + /*write*/ None, + ), + ), + ..Default::default() + }; + let mut sequence = vec![responses::sse(vec![ + responses::ev_function_call( + "grant", + "request_permissions", + &json!({ + "permissions": permissions, + }) + .to_string(), + ), + responses::ev_completed("grant"), + ])]; + for id in ["first-step", "second-step"] { + sequence.push(responses::sse(vec![ + responses::ev_function_call( + id, + "update_plan", + r#"{"plan":[{"step":"Inspect skills","status":"completed"}]}"#, + ), + responses::ev_completed(id), + ])); + } + sequence.push(responses::sse(vec![responses::ev_completed("done")])); + let responses = responses::mount_sse_sequence(&model, sequence).await; + test.codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "Inspect the approved skills across two steps.".into(), + text_elements: Vec::new(), + }])) + .await?; + let request = core_test_support::wait_for_event_match(&test.codex, |event| match event { + EventMsg::RequestPermissions(request) => Some(request.clone()), + _ => None, + }) + .await; + test.codex + .submit(Op::RequestPermissionsResponse { + id: request.call_id, + response: codex_protocol::request_permissions::RequestPermissionsResponse { + permissions, + scope: codex_protocol::request_permissions::PermissionGrantScope::Turn, + strict_auto_review: false, + }, + }) + .await?; + core_test_support::wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + assert_eq!(responses.requests().len(), 4); + let requests = executor.requests.lock().await; + let discovery = requests + .iter() + .filter(|request| request["method"] == "capabilityRoots/discoverV1") + .collect::>(); + assert_eq!( + discovery.len(), + 2, + "unchanged steps reuse both discovery scopes" + ); + assert_ne!( + discovery[0]["params"]["roots"][0]["sandbox"], + discovery[1]["params"]["roots"][0]["sandbox"] + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn v2_discovery_cache_reuses_matching_requests_and_isolates_execution_contexts() -> Result<()> { diff --git a/codex-rs/core/tests/suite/hooks_executor.rs b/codex-rs/core/tests/suite/hooks_executor.rs index 79e68ebe4d..4f47598e9a 100644 --- a/codex-rs/core/tests/suite/hooks_executor.rs +++ b/codex-rs/core/tests/suite/hooks_executor.rs @@ -711,6 +711,138 @@ async fn thread_plugin_selection_disables_executor_hooks_without_disabling_their Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn session_hooks_survive_turn_skill_discovery_failure() -> Result<()> { + skip_if_no_network!(Ok(())); + let executor = super::executor_discovery_v2::Executor::start( + /*supports_v2*/ true, + super::executor_discovery_v2::DiscoveryFailure::TurnGrant, + ) + .await?; + *executor.inventory.lock().await = Some(json!({ + "plugins": [{ + "id": "computer-use@openai-bundled", "displayName": "computer-use", + "remotePluginId": null, "version": "1", "root": "file:///computer-use", + "files": { + "manifest": {"path": "file:///computer-use/.codex-plugin/plugin.json", + "contents": computer_use_hook_manifest().to_string()}, + "mcpConfig": null, "appsConfig": null, + }, "skills": [], + }], + "skills": [], "warnings": [], + })); + let mut extensions = codex_extension_api::ExtensionRegistryBuilder::new(); + codex_mcp_extension::install_plugins( + &mut extensions, + Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()), + ); + let fixture = executor_plugin_hook_fixture( + test_codex() + .with_exec_server_url(executor.url.clone()) + .with_extensions(Arc::new(extensions.build())) + .with_config(|config| { + for feature in [ + Feature::ExecutorCapabilityDiscoveryV2, + Feature::Plugins, + Feature::RequestPermissionsTool, + ] { + config.features.enable(feature).unwrap(); + } + config.config_layer_stack = config + .config_layer_stack + .with_user_config( + &config.codex_home.join("config.toml"), + toml::from_str("[plugins.\"computer-use@openai-bundled\"]\nenabled = true") + .unwrap(), + ) + .unwrap(); + config.permissions.approval_policy = codex_core::config::Constrained::allow_any( + codex_protocol::protocol::AskForApproval::OnRequest, + ); + config + .permissions + .set_permission_profile( + codex_protocol::models::PermissionProfile::from_runtime_permissions( + &codex_protocol::permissions::FileSystemSandboxPolicy::restricted( + Vec::new(), + ), + codex_protocol::permissions::NetworkSandboxPolicy::Restricted, + ), + ) + .unwrap(); + }), + &[], + vec![ + sse_response(sse(vec![ + ev_function_call( + "grant", + "request_permissions", + r#"{"permissions":{"file_system":{"read":["turn-only-skills"]}}}"#, + ), + ev_completed("grant"), + ])), + completed_turn_response("done"), + completed_turn_response("disabled"), + ], + ) + .await?; + fixture.attach().await?; + fixture + .test + .codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "Grant skill access, then finish.".into(), + text_elements: Vec::new(), + }])) + .await?; + let request = wait_for_event_match(&fixture.test.codex, |event| match event { + EventMsg::RequestPermissions(request) => Some(request.clone()), + _ => None, + }) + .await; + fixture + .test + .codex + .submit(Op::RequestPermissionsResponse { + id: request.call_id, + response: codex_protocol::request_permissions::RequestPermissionsResponse { + permissions: request.permissions, + scope: codex_protocol::request_permissions::PermissionGrantScope::Turn, + strict_auto_review: false, + }, + }) + .await?; + wait_for_event(&fixture.test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + fixture.wait_for_hook_call().await?; + assert_eq!(fixture.calls().await?.len(), 1); + assert!( + executor.requests.lock().await.iter().any(|request| { + request["method"] == "capabilities/discoverV2" + && request["params"]["sandbox"] + .to_string() + .contains("turn-only-skills") + }), + "the turn inventory failed while the session inventory stayed healthy" + ); + submit_thread_settings( + &fixture.test.codex, + ThreadSettingsOverrides { + disabled_plugin_ids: Some(vec!["computer-use@openai-bundled".into()]), + ..Default::default() + }, + ) + .await?; + fixture + .test + .submit_text_turn("Finish with the plugin disabled.") + .await?; + assert_eq!(fixture.calls().await?.len(), 1); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn executor_stop_hook_uses_plugin_policy_refreshed_during_turn() -> Result<()> { skip_if_no_network!(Ok(())); @@ -1454,7 +1586,11 @@ async fn executor_plugin_hook_fixture( .expect("disable ordinary hooks"); let node_repl: McpServerConfig = serde_json::from_value(json!({ "url": node_repl_url, - "environment_id": remote_aware_environment_id(), + "environment_id": if config.features.enabled(Feature::ExecutorCapabilityDiscoveryV2) { + codex_exec_server::REMOTE_ENVIRONMENT_ID.to_string() + } else { + remote_aware_environment_id() + }, })) .expect("valid Node REPL MCP server configuration"); config diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index eb5b6812b4..004a9abc05 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -4167,3 +4167,6 @@ async fn production_turn_fairly_shortens_extension_catalog_descriptions() -> Res Ok(()) } + +#[path = "skills_extension/discovery_grants_tests.rs"] +mod discovery_grants_tests; diff --git a/codex-rs/core/tests/suite/skills_extension/discovery_grants_tests.rs b/codex-rs/core/tests/suite/skills_extension/discovery_grants_tests.rs new file mode 100644 index 0000000000..c7a55e469c --- /dev/null +++ b/codex-rs/core/tests/suite/skills_extension/discovery_grants_tests.rs @@ -0,0 +1,519 @@ +#![cfg(test)] +//! Exercises skill and plugin discovery before approval, after approval, and next turn. + +use super::*; +use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; +use core_test_support::exec_server::ExecServerProcess; +use core_test_support::test_codex::TestEnv; +use pretty_assertions::assert_eq; +use test_case::test_case; + +#[derive(Clone, Copy)] +enum Discovery { + ExecutorV1, + DisabledExecutorV1Plugin, + DisabledExecutorV1UnreadablePlugin, + DisabledExecutorV1UnreadablePluginDirectory, + DisabledExecutorV1UnreadableAncestorPlugin, + DisabledExecutorV1UnreadableAncestorPluginDirectory, + StandaloneExecutorV1WithDisabledId, + ExecutorV2, +} + +#[test_case(Discovery::ExecutorV1, PermissionGrantScope::Session; "executor v1 session grant")] +#[test_case(Discovery::ExecutorV1, PermissionGrantScope::Turn; "executor v1 turn grant")] +#[test_case(Discovery::ExecutorV2, PermissionGrantScope::Session; "executor v2 session grant")] +#[test_case(Discovery::ExecutorV2, PermissionGrantScope::Turn; "executor v2 turn grant")] +#[test_case(Discovery::DisabledExecutorV1Plugin, PermissionGrantScope::Turn; "turn grant respects plugin disablement")] +#[test_case(Discovery::DisabledExecutorV1UnreadablePlugin, PermissionGrantScope::Turn; "turn grant cannot bypass unreadable disabled plugin manifest")] +#[test_case(Discovery::DisabledExecutorV1UnreadablePluginDirectory, PermissionGrantScope::Turn; "turn grant cannot bypass unreadable disabled plugin directory")] +#[test_case(Discovery::DisabledExecutorV1UnreadableAncestorPlugin, PermissionGrantScope::Turn; "turn grant cannot bypass unreadable disabled ancestor plugin manifest")] +#[test_case(Discovery::DisabledExecutorV1UnreadableAncestorPluginDirectory, PermissionGrantScope::Turn; "turn grant cannot bypass unreadable disabled ancestor plugin directory")] +#[test_case(Discovery::StandaloneExecutorV1WithDisabledId, PermissionGrantScope::Turn; "explicit plugin deny preserves a standalone root")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn executor_discovery_uses_current_environment_grants( + discovery: Discovery, + scope: PermissionGrantScope, +) -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_target_windows!( + Ok(()), + "restricted reads require the elevated Windows sandbox backend, unavailable in this fixture" + ); + let v2 = matches!(discovery, Discovery::ExecutorV2); + let plugin_enabled = !matches!( + discovery, + Discovery::DisabledExecutorV1Plugin + | Discovery::DisabledExecutorV1UnreadablePlugin + | Discovery::DisabledExecutorV1UnreadablePluginDirectory + | Discovery::DisabledExecutorV1UnreadableAncestorPlugin + | Discovery::DisabledExecutorV1UnreadableAncestorPluginDirectory + ); + let plugin_mcp_server = "approved_plugin_mcp"; + let environment = if v2 { + None + } else { + Some(core_test_support::test_codex::test_env().await?) + }; + let mut extensions = ExtensionRegistryBuilder::new(); + if let Some(environment) = &environment { + #[cfg(target_os = "linux")] + let sandbox_exe = Some(core_test_support::find_codex_linux_sandbox_exe()?); + #[cfg(not(target_os = "linux"))] + let sandbox_exe = None; + let runtime_paths = codex_exec_server::ExecServerRuntimeOptions::new( + std::env::current_exe()?, + sandbox_exe, + )?; + codex_mcp_extension::install_plugins( + &mut extensions, + Arc::new( + codex_exec_server::EnvironmentManager::create_for_tests( + environment.exec_server_url().map(str::to_owned), + Some(runtime_paths), + ) + .await, + ), + ); + } + let providers = SkillProviders::new().with_executor_provider(Arc::new( + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None), + )); + install_with_providers(&mut extensions, providers, |config: &Config| { + SkillsExtensionConfig { + include_instructions: config.include_skill_instructions, + max_context_tokens: config.skill_max_context_tokens, + catalog_deltas_enabled: config.features.enabled(Feature::SkillCatalogDeltas), + omit_descriptions: false, + bundled_skills_enabled: false, + cloud_skill_enabled: false, + shadow_selection_enabled: false, + } + }); + let mut builder = test_codex() + .with_extensions(Arc::new(extensions.build())) + .with_config(move |config| { + configure_catalog_test(config); + config + .features + .enable(Feature::RequestPermissionsTool) + .expect("permission tool"); + config + .features + .set_enabled(Feature::ExecutorCapabilityDiscoveryV2, v2) + .expect("executor discovery version"); + config + .features + .set_enabled(Feature::Plugins, !v2) + .expect("selected plugins in V1"); + config + .features + .enable(Feature::SkipHostSkillDiscovery) + .expect("executor-only discovery"); + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config + .permissions + .set_permission_profile(PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(Vec::new()), + NetworkSandboxPolicy::Restricted, + )) + .expect("no initial filesystem access"); + }); + let server = responses::start_mock_server().await; + // V2 requires a remote executor with the final protocol version. + let executor; + let (test, root) = if v2 { + skip_if_remote!(Ok(()), "this case starts its own native exec-server"); + let home = tempfile::tempdir()?; + let root = PathUri::from_host_native_path(home.path().canonicalize()?.join("skills"))?; + // V2 scans installed roots; create this root before the executor prewarms its locations. + std::fs::create_dir(home.path().join("skills"))?; + executor = ExecServerProcess::start_with_version_and_home("0.164.0", home).await?; + let environment = + TestEnv::local_with_exec_server_url(Some(executor.websocket_url.clone())).await?; + ( + builder.build_with_environment(&server, environment).await?, + root, + ) + } else { + let test = builder + .build_with_environment(&server, environment.expect("V1 test environment")) + .await?; + let root = test + .executor_environment() + .selection() + .cwd + .join(".agents/skills")?; + (test, root) + }; + let selection = test.executor_environment().selection().clone(); + let plugin_root = if matches!( + discovery, + Discovery::DisabledExecutorV1UnreadableAncestorPlugin + | Discovery::DisabledExecutorV1UnreadableAncestorPluginDirectory + ) { + root.parent() + .expect("plugin owns selected skill subdirectory") + } else { + root.clone() + }; + let skill = root.join("approved-skill")?; + test.fs() + .create_directory( + &skill, + CreateDirectoryOptions { + recursive: true, + follow_symlinks: true, + }, + /*sandbox*/ None, + ) + .await?; + test.fs().write_file( + &skill.join("SKILL.md")?, + b"---\nname: approved-skill\ndescription: Approved repository instructions.\n---\n\nApproved skill body.".to_vec(), + Default::default(), + /*sandbox*/ None, + ).await?; + if !v2 && !matches!(discovery, Discovery::StandaloneExecutorV1WithDisabledId) { + let directory = if matches!( + discovery, + Discovery::DisabledExecutorV1UnreadablePluginDirectory + | Discovery::DisabledExecutorV1UnreadableAncestorPluginDirectory + ) { + plugin_root + .parent() + .expect("root parent") + .join("denied-plugin")? + } else { + plugin_root.join(".codex-plugin")? + }; + test.fs() + .create_directory( + &directory, + CreateDirectoryOptions { + recursive: true, + follow_symlinks: true, + }, + /*sandbox*/ None, + ) + .await?; + let manifest = plugin_root.join(".codex-plugin/plugin.json")?; + let target = if matches!( + discovery, + Discovery::DisabledExecutorV1UnreadablePlugin + | Discovery::DisabledExecutorV1UnreadableAncestorPlugin + ) { + plugin_root + .parent() + .expect("root parent") + .join("denied-plugin.json")? + } else { + directory.join("plugin.json")? + }; + test.fs() + .write_file( + &target, + br#"{"name":"approved-plugin"}"#.to_vec(), + Default::default(), + /*sandbox*/ None, + ) + .await?; + let (target, link) = if matches!( + discovery, + Discovery::DisabledExecutorV1UnreadablePluginDirectory + | Discovery::DisabledExecutorV1UnreadableAncestorPluginDirectory + ) { + (directory, plugin_root.join(".codex-plugin")?) + } else { + (target, manifest) + }; + if target != link { + let process = test + .executor_environment() + .environment() + .get_exec_backend() + .start(ExecParams { + process_id: ProcessId::from("link-denied-plugin-manifest"), + metadata: None, + argv: vec![ + "/bin/ln".to_string(), + "-s".to_string(), + target.inferred_native_path_string(), + link.inferred_native_path_string(), + ], + cwd: selection.cwd.clone(), + env_policy: None, + shell_snapshot: None, + env: Default::default(), + tty: false, + pipe_stdin: false, + arg0: None, + sandbox: None, + enforce_managed_network: false, + managed_network: None, + network_proxy: None, + }) + .await? + .process; + let mut events = process.subscribe_events(); + loop { + match events.recv().await? { + ExecProcessEvent::Exited { exit_code, .. } => { + assert_eq!(exit_code, 0); + break; + } + ExecProcessEvent::Output(_) => {} + event @ (ExecProcessEvent::Closed { .. } | ExecProcessEvent::Failed(_)) => { + panic!("manifest symlink process did not exit: {event:?}") + } + } + } + } + let command = crate::suite::rmcp_client::remote_aware_stdio_server_bin()?; + test.fs() + .write_file( + &root.join(".mcp.json")?, + serde_json::to_vec(&json!({ + "mcpServers": {(plugin_mcp_server): {"command": command}}, + }))?, + Default::default(), + /*sandbox*/ None, + ) + .await?; + } + let package = format!( + "skill://{}/{}", + match discovery { + Discovery::ExecutorV1 + | Discovery::DisabledExecutorV1Plugin + | Discovery::DisabledExecutorV1UnreadablePlugin + | Discovery::DisabledExecutorV1UnreadablePluginDirectory + | Discovery::DisabledExecutorV1UnreadableAncestorPlugin + | Discovery::DisabledExecutorV1UnreadableAncestorPluginDirectory + | Discovery::StandaloneExecutorV1WithDisabledId => "approved-root", + Discovery::ExecutorV2 => "codex-home-skills", + }, + skill + .inferred_native_path_string() + .replace('\\', "/") + .trim_start_matches('/') + ); + let read_skill = |call_id| { + responses::ev_function_call_with_namespace( + call_id, + "skills", + "read", + &json!({"package": package}).to_string(), + ) + }; + let mut thread_extension_init = ExtensionDataInit::new(); + if !v2 { + thread_extension_init.insert(vec![SelectedCapabilityRoot { + id: "approved-root".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: selection.environment_id.clone(), + path: root.clone(), + }, + }]); + } + let thread = test + .thread_manager + .start_thread(StartThreadOptions { + environments: Some(vec![selection.into_request()]), + disabled_plugin_ids: (!plugin_enabled + || matches!(discovery, Discovery::StandaloneExecutorV1WithDisabledId)) + .then(|| vec!["approved-root".to_string()]), + thread_extension_init, + ..StartThreadOptions::new(test.config.clone()) + }) + .await? + .thread; + let plugin_is_published = async || -> Result { + let (config, status) = thread + .mcp_server_status_snapshot( + plugin_mcp_server, + codex_mcp::McpSnapshotDetail::ToolsAndAuthOnly, + /*expected_registration*/ None, + ) + .await?; + let published = config + .mcp_server_catalog + .server(plugin_mcp_server) + .is_some(); + let has_echo = status + .tools_by_server + .get(plugin_mcp_server) + .is_some_and(|tools| tools.contains_key("echo")); + assert_eq!(has_echo, published); + Ok(published) + }; + let permissions = RequestPermissionProfile { + file_system: Some(FileSystemPermissions::from_read_write_path_uris( + Some(vec![ + test.fs() + .canonicalize(&plugin_root, /*sandbox*/ None) + .await?, + ]), + /*write*/ None, + )), + ..Default::default() + }; + let mut sequence = vec![ + sse(vec![ + ev_response_created("before-approval"), + read_skill("read-before-approval"), + ev_completed("before-approval"), + ]), + sse(vec![ + ev_response_created("grant"), + responses::ev_function_call( + "grant-skills", + "request_permissions", + &json!({ + "reason": "Read repository skills", "permissions": permissions, + }) + .to_string(), + ), + ev_completed("grant"), + ]), + ]; + for (response_id, call_id) in [ + ("granted", "read-after-approval"), + ("next-turn", "read-next-turn"), + ] { + let mut events = vec![ev_response_created(response_id), read_skill(call_id)]; + if response_id == "granted" && !v2 { + events.push(responses::ev_function_call( + "request-sibling", + "request_permissions", + &json!({ + "reason": "Read the sibling file", + "permissions": {"file_system": FileSystemPermissions::from_read_write_path_uris( + Some(vec![root.parent().expect("skills parent").join("private")?]), /*write*/ None, + )}, + }) + .to_string(), + )); + } + events.push(ev_completed(response_id)); + sequence.push(sse(events)); + sequence.push(sse(vec![ev_response_created("done"), ev_completed("done")])); + } + let responses = responses::mount_sse_sequence(&server, sequence).await; + thread + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "Read the selected skill, requesting permission if needed.".into(), + text_elements: Vec::new(), + }])) + .await?; + let event = wait_for_event(&thread, |event| { + matches!( + event, + EventMsg::RequestPermissions(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + let EventMsg::RequestPermissions(request) = event else { + panic!("expected permission request: {event:?}"); + }; + assert_eq!(request.permissions, permissions); + if !v2 { + assert!(!plugin_is_published().await?); + } + thread + .submit(Op::RequestPermissionsResponse { + id: request.call_id, + response: RequestPermissionsResponse { + permissions, + scope, + strict_auto_review: false, + }, + }) + .await?; + if !v2 { + // The second permission request keeps the turn open while we inspect the live grant. + let event = wait_for_event(&thread, |event| { + matches!( + event, + EventMsg::RequestPermissions(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + let EventMsg::RequestPermissions(request) = event else { + panic!("expected the sibling permission request: {event:?}"); + }; + assert_eq!(request.call_id, "request-sibling"); + assert_eq!( + plugin_is_published().await?, + scope == PermissionGrantScope::Session + ); + thread + .submit(Op::RequestPermissionsResponse { + id: request.call_id, + response: RequestPermissionsResponse { + permissions: RequestPermissionProfile::default(), + scope: PermissionGrantScope::Turn, + strict_auto_review: false, + }, + }) + .await?; + } + wait_for_event(&thread, |event| matches!(event, EventMsg::TurnComplete(_))).await; + if !v2 { + assert_eq!( + plugin_is_published().await?, + scope == PermissionGrantScope::Session + ); + } + thread + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "Read the selected skill again.".into(), + text_elements: Vec::new(), + }])) + .await?; + wait_for_event(&thread, |event| matches!(event, EventMsg::TurnComplete(_))).await; + + let requests = responses.requests(); + assert_eq!(requests.len(), 6); + let visible = [0, 2, 4] + .into_iter() + .map(|index| { + requests[index] + .message_input_texts("developer") + .iter() + .rev() + .find(|text| text.contains(SKILLS_INSTRUCTIONS_OPEN_TAG)) + .is_some_and(|text| text.contains("approved-skill")) + }) + .collect::>(); + assert_eq!( + visible, + vec![ + false, + plugin_enabled, + plugin_enabled && scope == PermissionGrantScope::Session + ] + ); + let readable = [ + (1, "read-before-approval"), + (3, "read-after-approval"), + (5, "read-next-turn"), + ] + .into_iter() + .map(|(index, call)| { + requests[index] + .function_call_output_text(call) + .expect("skill read result") + .contains("Approved skill body.") + }) + .collect::>(); + assert_eq!( + readable, + vec![ + false, + plugin_enabled, + plugin_enabled && scope == PermissionGrantScope::Session + ] + ); + Ok(()) +} diff --git a/codex-rs/ext/extension-api/src/contributors/mcp.rs b/codex-rs/ext/extension-api/src/contributors/mcp.rs index fc8df636a9..898626394f 100644 --- a/codex-rs/ext/extension-api/src/contributors/mcp.rs +++ b/codex-rs/ext/extension-api/src/contributors/mcp.rs @@ -18,6 +18,8 @@ use crate::ExtensionFuture; pub struct McpServerContributionContext<'a, C> { /// Host configuration visible during MCP resolution. config: &'a C, + /// Explicit thread denials, including V1 roots whose manifests cannot be read. + disabled_plugin_ids: &'a [String], /// Whether pending auth differs from published auth; invalidate catalogs before projection. auth_changed: bool, /// Extension-owned data for the active thread, when resolution is thread-scoped. @@ -51,6 +53,7 @@ impl<'a, C> McpServerContributionContext<'a, C> { pub fn global(config: &'a C) -> Self { Self { config, + disabled_plugin_ids: &[], auth_changed: false, thread_store: None, thread_init: None, @@ -75,6 +78,7 @@ impl<'a, C> McpServerContributionContext<'a, C> { ) -> Self { Self { config, + disabled_plugin_ids: &[], auth_changed: false, thread_store: Some(thread_store), thread_init: Some(thread_init), @@ -87,6 +91,17 @@ impl<'a, C> McpServerContributionContext<'a, C> { } } + /// Applies thread denials before resolving ownership of unreadable plugin manifests. + pub fn with_disabled_plugin_ids(mut self, ids: &'a [String]) -> Self { + self.disabled_plugin_ids = ids; + self + } + + /// Plugin identities explicitly disabled for the active thread. + pub fn disabled_plugin_ids(&self) -> &'a [String] { + self.disabled_plugin_ids + } + /// Attaches the same environment snapshot used to project MCP authority. /// /// Preserve pending and failed selections: an unavailable primary executor diff --git a/codex-rs/ext/mcp/src/plugin.rs b/codex-rs/ext/mcp/src/plugin.rs index e2095bdfbb..1c75869648 100644 --- a/codex-rs/ext/mcp/src/plugin.rs +++ b/codex-rs/ext/mcp/src/plugin.rs @@ -343,7 +343,10 @@ impl McpServerContributor for PluginContributor { &root.selected_root.location; // Ownership survives capability parse failures; an unresolved root cannot // bypass a denying policy. - if !plugins_config.allows_plugin(&root.selected_root.id) + if (!plugins_config.allows_plugin(&root.selected_root.id) + || context + .disabled_plugin_ids() + .contains(&root.selected_root.id)) && discovery.error.is_none() && !matches!( self.ownership_for_root( @@ -389,7 +392,9 @@ impl McpServerContributor for PluginContributor { } } else { for selected_root in selected_roots { - if !plugins_config.allows_plugin(&selected_root.id) { + if !plugins_config.allows_plugin(&selected_root.id) + || context.disabled_plugin_ids().contains(&selected_root.id) + { if !matches!( self.ownership_for_root( &state, diff --git a/codex-rs/ext/mcp/tests/plugin_mcp.rs b/codex-rs/ext/mcp/tests/plugin_mcp.rs index 0f6167099d..adf42e03f9 100644 --- a/codex-rs/ext/mcp/tests/plugin_mcp.rs +++ b/codex-rs/ext/mcp/tests/plugin_mcp.rs @@ -303,7 +303,7 @@ async fn selected_root_ownership_retries_errors_and_caches_success() -> TestResu for (phase, enabled_root, expected) in [ (0, None, vec![("standalone", vec![])]), - (1, None, vec![]), + (1, None, vec![("standalone", vec![])]), (2, Some("owned-subfolder"), vec![("selected-root", vec![])]), ( 3, @@ -350,8 +350,8 @@ async fn selected_root_ownership_retries_errors_and_caches_success() -> TestResu .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() .await?; - // A warning requires metadata probing, which the test filesystem cannot sandbox. - // Complete discovery retries ownership under the same sandbox, root, and policy. + // Neither incomplete nor legacy complete discovery proves that a marker is absent. + // The scoped metadata probe fails here; later readable roots must retry ownership. let discovery = (phase < 2).then(|| { ExecutorCapabilityDiscoverySnapshot::new( std::slice::from_ref(standalone),