From 007f668c34cca4b9c36ca7ddc08066897f2bbabc Mon Sep 17 00:00:00 2001 From: Dmitriy Kovalenko Date: Wed, 4 Nov 2026 01:47:39 +0000 Subject: [PATCH] Respect turn filesystem permissions during executor skill discovery (#59291) ## Why Direct executor skill discovery used unrestricted filesystem access, bypassing the turn's permission context. Cached catalogs also needed to account for changes in filesystem access within a turn. ## What changed - Bind selected capability roots to callback-scoped filesystem access through `AccessedSelectedCapabilityRoot`. - Pass accessors built with the turn's granted permissions into skill preparation and discovery, removing the executor skill provider's environment lookup and unrestricted accessor. - Include filesystem access keys in the direct discovery cache so permission changes trigger rediscovery. ## Testing Extend tests to verify that restricted discovery returns no skills when sandbox access is unavailable, restoring access restores discovery, and changing or restoring access invalidates the catalog cache. GitOrigin-RevId: 0f7aa7646f65923f65f0cfc0b387d94bd7f2db05 --- codex-rs/app-server/src/mcp_refresh.rs | 1 - codex-rs/app-server/src/message_processor.rs | 1 - codex-rs/core/src/session/mod.rs | 21 ++- .../core/tests/suite/executor_discovery_v2.rs | 5 +- codex-rs/core/tests/suite/scenarios.rs | 17 +-- .../suite/scenarios_frozen_cloud_skills.rs | 1 - codex-rs/core/tests/suite/skills_extension.rs | 41 ++--- .../skills_extension/cloud_lifecycle_tests.rs | 2 +- .../skills_extension/cloud_skill_tests.rs | 2 +- .../ext/extension-api/src/contributors.rs | 9 +- codex-rs/ext/extension-api/src/lib.rs | 1 + codex-rs/ext/skills/src/catalog_state.rs | 30 ++-- codex-rs/ext/skills/src/cloud_cache_tests.rs | 5 +- codex-rs/ext/skills/src/cloud_skill.rs | 1 - codex-rs/ext/skills/src/extension.rs | 2 - codex-rs/ext/skills/src/preparation.rs | 29 +--- codex-rs/ext/skills/src/provider.rs | 13 +- codex-rs/ext/skills/src/provider/executor.rs | 42 +----- .../ext/skills/src/provider/frozen_cloud.rs | 2 +- codex-rs/ext/skills/src/provider/host.rs | 2 +- codex-rs/ext/skills/src/render_tests.rs | 2 - codex-rs/ext/skills/src/sources.rs | 12 +- codex-rs/ext/skills/src/state.rs | 25 ++-- .../tests/executor_file_system_authority.rs | 140 +++++++++++------- codex-rs/ext/skills/tests/skills_extension.rs | 102 +++++++++++-- .../skills_extension/discovery_v2_tests.rs | 29 ++-- .../skills_extension/merge_catalog_tests.rs | 7 +- .../skills_extension/preparation_tests.rs | 2 +- .../shadow_task_context_tests.rs | 2 +- 29 files changed, 294 insertions(+), 254 deletions(-) diff --git a/codex-rs/app-server/src/mcp_refresh.rs b/codex-rs/app-server/src/mcp_refresh.rs index 4c4573a472..4a6e7daae2 100644 --- a/codex-rs/app-server/src/mcp_refresh.rs +++ b/codex-rs/app-server/src/mcp_refresh.rs @@ -528,7 +528,6 @@ enabled = false let environment_manager = Arc::new(EnvironmentManager::default_for_tests()); let executor_skill_provider: Arc = Arc::new( codex_skills_extension::ExecutorSkillProvider::new_with_restriction_product( - Arc::clone(&environment_manager), SessionSource::Exec.restriction_product(), ), ); diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index f221d8be26..024dca4205 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -354,7 +354,6 @@ impl MessageProcessor { let restriction_product = session_source.restriction_product(); let executor_skill_provider: Arc = Arc::new( codex_skills_extension::ExecutorSkillProvider::new_with_restriction_product( - Arc::clone(&environment_manager_for_extensions), restriction_product, ), ); diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 7f106ca1d7..729db3180d 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -4413,11 +4413,30 @@ impl Session { &turn_context.disabled_plugin_ids, ) .await; + let mut filesystem_access = HashMap::new(); + for environment in environments.turn_environments() { + let environment_id = &environment.selection.environment_id; + let grants = turn_context.granted_permissions(environment_id); + filesystem_access.insert(environment_id, environment.fs_accessor(grants)); + } + let accessed_roots = ready_selected_capability_roots + .iter() + .filter_map(|root| { + let codex_protocol::capabilities::CapabilityRootLocation::Environment { + environment_id, + .. + } = &root.location; + Some(codex_extension_api::AccessedSelectedCapabilityRoot { + root, + fs: filesystem_access.get(environment_id)?, + }) + }) + .collect::>(); for contributor in self.services.extensions.tool_contributors() { contributor .prepare_for_step(codex_extension_api::StepPreparationInput { turn_id: &turn_context.sub_id, - ready_selected_capability_roots: &ready_selected_capability_roots, + ready_selected_capability_roots: &accessed_roots, executor_capability_discovery: executor_capability_discovery.as_deref(), executor_capability_discovery_v2: executor_capability_discovery_v2 .as_deref(), diff --git a/codex-rs/core/tests/suite/executor_discovery_v2.rs b/codex-rs/core/tests/suite/executor_discovery_v2.rs index 6cc0b3ee2b..e443ed1cd3 100644 --- a/codex-rs/core/tests/suite/executor_discovery_v2.rs +++ b/codex-rs/core/tests/suite/executor_discovery_v2.rs @@ -579,7 +579,6 @@ async fn check_acquisition(enabled: bool, version: &'static str) -> Result<()> { &mut extensions, codex_skills_extension::SkillProviders::new().with_executor_provider(Arc::new( codex_skills_extension::ExecutorSkillProvider::new_with_restriction_product( - Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()), /*restriction_product*/ None, ), )), @@ -1069,7 +1068,7 @@ async fn auth_rotation_refreshes_plugin_acceptance_before_merged_skills() -> Res let manager = Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()); install_plugin_providers( &mut extensions, - PluginProviders::new(Arc::new(ExecutorPluginProvider::new(manager.clone()))) + PluginProviders::new(Arc::new(ExecutorPluginProvider::new(manager))) .with_cloud_provider(cloud.clone()), ); install_with_providers( @@ -1093,7 +1092,7 @@ async fn auth_rotation_refreshes_plugin_acceptance_before_merged_skills() -> Res })) .with_executor_provider(Arc::new( ExecutorSkillProvider::new_with_restriction_product( - manager, /*restriction_product*/ None, + /*restriction_product*/ None, ), )), |_config: &Config| SkillsExtensionConfig { diff --git a/codex-rs/core/tests/suite/scenarios.rs b/codex-rs/core/tests/suite/scenarios.rs index ed1f070259..930a06b27d 100644 --- a/codex-rs/core/tests/suite/scenarios.rs +++ b/codex-rs/core/tests/suite/scenarios.rs @@ -1500,14 +1500,12 @@ async fn astra_omits_disabled_executor_and_plugin_skills_from_model_context( "Follow the disabled plugin workflow.", )?; let environment_manager = Arc::new(EnvironmentManager::default_for_tests()); - let provider = ExecutorSkillProvider::new_with_restriction_product( - Arc::clone(&environment_manager), - /*restriction_product*/ None, - ) - .with_disabled_skill_paths(HashMap::from([( - LOCAL_ENVIRONMENT_ID.to_string(), - HashSet::from([PathUri::from_host_native_path(disabled)?]), - )])); + let provider = + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None) + .with_disabled_skill_paths(HashMap::from([( + LOCAL_ENVIRONMENT_ID.to_string(), + HashSet::from([PathUri::from_host_native_path(disabled)?]), + )])); let mut extensions = ExtensionRegistryBuilder::::new(); codex_mcp_extension::install_plugins(&mut extensions, environment_manager); let (started, captured) = oneshot::channel(); @@ -1917,7 +1915,7 @@ async fn skill_read_recovers_omitted_installation_and_reads_selected_environment // Supply already-reconciled catalog fixtures; exercise production preparation and reads. struct InstallationSkills(SkillCatalog); impl SkillProvider for InstallationSkills { - fn list(&self, _query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, _query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { Box::pin(async { Ok(self.0.clone()) }) } @@ -2006,7 +2004,6 @@ async fn skill_read_recovers_omitted_installation_and_reads_selected_environment .with_cloud_provider(Arc::new(InstallationSkills(catalog))) .with_executor_provider(Arc::new( ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), /*restriction_product*/ None, ), )), diff --git a/codex-rs/core/tests/suite/scenarios_frozen_cloud_skills.rs b/codex-rs/core/tests/suite/scenarios_frozen_cloud_skills.rs index 25d094c767..7a758f60b8 100644 --- a/codex-rs/core/tests/suite/scenarios_frozen_cloud_skills.rs +++ b/codex-rs/core/tests/suite/scenarios_frozen_cloud_skills.rs @@ -29,7 +29,6 @@ async fn frozen_cloud_skills_with_named_executor_request_history() -> Result<()> .with_cloud_provider(Arc::new(provider)) .with_executor_provider(Arc::new( ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), /*restriction_product*/ None, ), )), diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index 6f391c5163..eb5b6812b4 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -1,7 +1,3 @@ -use codex_exec_server::FileSystemEnvironmentAccessor; -use codex_exec_server::FileSystemSandboxContext; -use codex_skills_extension::HostSkillsLoadInput; -use core_test_support::test_codex::TestCodexHarness; use std::sync::Arc; use std::sync::Mutex; use std::sync::atomic::AtomicBool; @@ -19,9 +15,10 @@ use codex_core::WaitForEnvironmentToolConfig; use codex_core::config::Config; use codex_core::config::Constrained; use codex_exec_server::CreateDirectoryOptions; -use codex_exec_server::EnvironmentManager; use codex_exec_server::ExecParams; use codex_exec_server::ExecProcessEvent; +use codex_exec_server::FileSystemEnvironmentAccessor; +use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_exec_server::ProcessId; use codex_exec_server::RemoveOptions; @@ -73,6 +70,7 @@ use codex_protocol::request_permissions::RequestPermissionsResponse; use codex_protocol::user_input::UserInput; use codex_skills_extension::ExecutorSkillProvider; use codex_skills_extension::HostSkillProvider; +use codex_skills_extension::HostSkillsLoadInput; use codex_skills_extension::SkillProvider; use codex_skills_extension::SkillProviderSource; use codex_skills_extension::SkillProviders; @@ -110,6 +108,7 @@ use core_test_support::skip_if_no_network; use core_test_support::skip_if_remote; use core_test_support::skip_if_target_windows; use core_test_support::skip_if_wine_exec; +use core_test_support::test_codex::TestCodexHarness; use core_test_support::test_codex::environment_config_for_selection; use core_test_support::test_codex::test_codex; use core_test_support::test_codex::test_env; @@ -155,7 +154,7 @@ struct PausedCatalogSkillProvider { } impl SkillProvider for PausedCatalogSkillProvider { - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { Box::pin(async move { if query.host_snapshot.is_none() { return Ok(SkillCatalog::default()); @@ -214,7 +213,7 @@ impl ExtensionEventSink for ChannelEventSink { } impl SkillProvider for StaticSkillProvider { - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { // Keep thread context empty so the catalog is exercised through the // production turn-input path, where the host snapshot is available. let catalog = if query.host_snapshot.is_some() { @@ -248,7 +247,7 @@ impl SkillProvider for StaticSkillProvider { } impl SkillProvider for CatalogSkillProvider { - fn list(&self, _query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, _query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { Box::pin(async { Ok(self.catalog.clone()) }) } @@ -1600,10 +1599,7 @@ async fn opted_in_executor_provider_skips_host_discovery_but_injects_discovered_ install_with_providers( &mut extensions, SkillProviders::new().with_executor_provider(Arc::new( - ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), - /*restriction_product*/ None, - ), + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None), )), |config: &Config| SkillsExtensionConfig { include_instructions: config.include_skill_instructions, @@ -1831,10 +1827,7 @@ async fn executor_only_provider_preserves_structured_repo_skill_without_discover install_with_providers( &mut extensions, SkillProviders::new().with_executor_provider(Arc::new( - ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), - /*restriction_product*/ None, - ), + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None), )), |config: &Config| SkillsExtensionConfig { include_instructions: config.include_skill_instructions, @@ -1982,10 +1975,7 @@ async fn executor_skill_tool_reads_references_under_current_permissions( install_with_providers( &mut extensions, SkillProviders::new().with_executor_provider(Arc::new( - ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), - /*restriction_product*/ None, - ), + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None), )), |config: &Config| SkillsExtensionConfig { include_instructions: config.include_skill_instructions, @@ -2407,16 +2397,6 @@ async fn explicit_executor_skill_prompt_rejects_oversized_resource() -> Result<( )], warnings: Vec::new(), }; - let environment_manager = match environment.exec_server_url() { - Some(url) => { - EnvironmentManager::create_for_tests( - Some(url.to_string()), - /*local_runtime_paths*/ None, - ) - .await - } - None => EnvironmentManager::default_for_tests(), - }; let (event_tx, event_rx) = std::sync::mpsc::channel(); let mut extensions = ExtensionRegistryBuilder::::with_event_sink(Arc::new(ChannelEventSink(event_tx))); @@ -2426,7 +2406,6 @@ async fn explicit_executor_skill_prompt_rejects_oversized_resource() -> Result<( .with_executor_provider(Arc::new(CatalogSkillProvider { catalog })) .with_executor_provider(Arc::new( ExecutorSkillProvider::new_with_restriction_product( - Arc::new(environment_manager), /*restriction_product*/ None, ), )), diff --git a/codex-rs/core/tests/suite/skills_extension/cloud_lifecycle_tests.rs b/codex-rs/core/tests/suite/skills_extension/cloud_lifecycle_tests.rs index aba192e5b0..c31a8debbf 100644 --- a/codex-rs/core/tests/suite/skills_extension/cloud_lifecycle_tests.rs +++ b/codex-rs/core/tests/suite/skills_extension/cloud_lifecycle_tests.rs @@ -15,7 +15,7 @@ struct CloudTurnProvider { } impl SkillProvider for CloudTurnProvider { - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { Box::pin(async move { self.lists.fetch_add(1, Ordering::SeqCst); *self.resource_key.lock().expect("resource key lock") = query diff --git a/codex-rs/core/tests/suite/skills_extension/cloud_skill_tests.rs b/codex-rs/core/tests/suite/skills_extension/cloud_skill_tests.rs index bf56badb52..33e2e95e5e 100644 --- a/codex-rs/core/tests/suite/skills_extension/cloud_skill_tests.rs +++ b/codex-rs/core/tests/suite/skills_extension/cloud_skill_tests.rs @@ -15,7 +15,7 @@ struct FakeCloudSkillProvider { } impl SkillProvider for FakeCloudSkillProvider { - fn list(&self, _query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, _query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { Box::pin(async { Ok(self.catalog.clone()) }) } diff --git a/codex-rs/ext/extension-api/src/contributors.rs b/codex-rs/ext/extension-api/src/contributors.rs index dc95f991a4..80fb7cb193 100644 --- a/codex-rs/ext/extension-api/src/contributors.rs +++ b/codex-rs/ext/extension-api/src/contributors.rs @@ -362,11 +362,18 @@ pub trait SkillInvocationContributor: Send + Sync { } } +/// A selected capability root bound to filesystem access for the current callback. +#[derive(Clone, Copy)] +pub struct AccessedSelectedCapabilityRoot<'a> { + pub root: &'a SelectedCapabilityRoot, + pub fs: &'a dyn codex_file_system::EnvironmentAccess, +} + /// Runtime inputs shared by preparation, tool construction, and later context consumers. #[derive(Clone, Copy)] pub struct StepPreparationInput<'a> { pub turn_id: &'a str, - pub ready_selected_capability_roots: &'a [SelectedCapabilityRoot], + pub ready_selected_capability_roots: &'a [AccessedSelectedCapabilityRoot<'a>], pub executor_capability_discovery: Option<&'a ExecutorCapabilityDiscoverySnapshot>, pub executor_capability_discovery_v2: Option<&'a [ExecutorCapabilityDiscoveryV2Snapshot]>, /// Captured plugin policy; an empty snapshot represents no selected plugins. diff --git a/codex-rs/ext/extension-api/src/lib.rs b/codex-rs/ext/extension-api/src/lib.rs index 147b52ff1b..d78b0a8472 100644 --- a/codex-rs/ext/extension-api/src/lib.rs +++ b/codex-rs/ext/extension-api/src/lib.rs @@ -52,6 +52,7 @@ pub use codex_tools::TurnItemEmissionFuture; pub use codex_tools::TurnItemEmitter; pub use codex_tools::parse_tool_input_schema; pub use codex_tools::parse_tool_input_schema_without_compaction; +pub use contributors::AccessedSelectedCapabilityRoot; pub use contributors::ApprovalDecision; pub use contributors::ApprovalDecisionInput; pub use contributors::ApprovalReviewContributor; diff --git a/codex-rs/ext/skills/src/catalog_state.rs b/codex-rs/ext/skills/src/catalog_state.rs index efa4b7442f..e3fc3fedbf 100644 --- a/codex-rs/ext/skills/src/catalog_state.rs +++ b/codex-rs/ext/skills/src/catalog_state.rs @@ -5,8 +5,8 @@ use std::collections::HashMap; use std::sync::Arc; +use codex_exec_server::EnvironmentAccessKey; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; -use codex_exec_server::ResolvedSelectedCapabilityRoot; use codex_extension_api::CatalogRefreshBackoff; use codex_mcp::McpResourceClient; use codex_mcp::McpResourceClientAuthKey; @@ -124,25 +124,22 @@ impl CachedExecutorV2Catalog { #[derive(Clone, Debug)] pub(crate) struct ExecutorCatalogCacheKey { pub(crate) turn_id: String, - pub(crate) resolved_roots: Vec, + pub(crate) roots: Vec<(SelectedCapabilityRoot, EnvironmentAccessKey)>, pub(crate) include_bundled_skills: bool, } impl ExecutorCatalogCacheKey { - pub(crate) fn matches(&self, query: &crate::provider::SkillListQuery) -> bool { + pub(crate) fn matches(&self, query: &crate::provider::SkillListQuery<'_>) -> bool { self.turn_id == query.turn_id - && self - .resolved_roots - .iter() - .map(ResolvedSelectedCapabilityRoot::selected_root) - .eq(query.executor_roots.iter()) && self.include_bundled_skills == query.include_bundled_skills - // Unbound callers cannot establish identity for a cached filesystem catalog. - && query.resolved_executor_roots.len() == query.executor_roots.len() - && self.resolved_roots.iter().zip(&query.resolved_executor_roots).all(|(previous, current)| { - previous.selected_root() == current.selected_root() - && Arc::ptr_eq(previous.environment(), current.environment()) - }) + && self.roots.len() == query.executor_roots.len() + && self + .roots + .iter() + .zip(&query.executor_roots) + .all(|((root, access), current)| { + root == current.root && *access == current.fs.cache_key() + }) } } @@ -155,7 +152,6 @@ pub(crate) struct CachedExecutorCatalog { /// Keeps the latest result; only successful results for unchanged inputs can be reused. #[derive(Clone, Debug)] pub(crate) struct CachedExecutorDiscoveryCatalog { - pub(crate) roots: Vec, pub(crate) discovery: ExecutorCapabilityDiscoverySnapshot, pub(crate) include_bundled_skills: bool, pub(crate) catalog: SkillCatalog, @@ -164,13 +160,11 @@ pub(crate) struct CachedExecutorDiscoveryCatalog { impl CachedExecutorDiscoveryCatalog { pub(crate) fn matches( &self, - roots: &[SelectedCapabilityRoot], discovery: Option<&ExecutorCapabilityDiscoverySnapshot>, include_bundled_skills: bool, ) -> bool { discovery.is_some_and(|discovery| { - self.roots == roots - && self.include_bundled_skills == include_bundled_skills + self.include_bundled_skills == include_bundled_skills && self.discovery.sandbox_contexts() == discovery.sandbox_contexts() && self.discovery.roots().len() == discovery.roots().len() && self.discovery.roots().iter().zip(discovery.roots()).all( diff --git a/codex-rs/ext/skills/src/cloud_cache_tests.rs b/codex-rs/ext/skills/src/cloud_cache_tests.rs index 8639255126..a9c0ccdf06 100644 --- a/codex-rs/ext/skills/src/cloud_cache_tests.rs +++ b/codex-rs/ext/skills/src/cloud_cache_tests.rs @@ -36,7 +36,7 @@ struct Provider { } impl SkillProvider for Provider { - fn list(&self, _query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, _query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { Box::pin(async move { self.lists.fetch_add(1, Ordering::SeqCst); let fail = self.fail_list.swap(false, Ordering::SeqCst); @@ -112,11 +112,10 @@ fn client() -> Arc { )))) } -fn query(client: Option>) -> SkillListQuery { +fn query(client: Option>) -> SkillListQuery<'static> { SkillListQuery { turn_id: "turn".into(), executor_roots: Vec::new(), - resolved_executor_roots: Vec::new(), host_snapshot: None, include_host_skills: false, include_bundled_skills: false, diff --git a/codex-rs/ext/skills/src/cloud_skill.rs b/codex-rs/ext/skills/src/cloud_skill.rs index 06fd23db45..12a363fd15 100644 --- a/codex-rs/ext/skills/src/cloud_skill.rs +++ b/codex-rs/ext/skills/src/cloud_skill.rs @@ -27,7 +27,6 @@ impl SkillsExtension { let query = SkillListQuery { turn_id: input.turn_id.to_string(), executor_roots: Vec::new(), - resolved_executor_roots: Vec::new(), host_snapshot: None, include_host_skills: false, include_bundled_skills: config.bundled_skills_enabled, diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index 5c20226441..4c1c7191e0 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -213,7 +213,6 @@ where .list_for_turn(SkillListQuery { turn_id: thread_store.level_id().to_string(), executor_roots: Vec::new(), - resolved_executor_roots: Vec::new(), host_snapshot: None, include_host_skills: false, include_bundled_skills: config.bundled_skills_enabled, @@ -358,7 +357,6 @@ where let query = SkillListQuery { turn_id: input.turn_id.clone(), executor_roots: Vec::new(), - resolved_executor_roots: Vec::new(), host_snapshot: host_snapshot.clone(), include_host_skills: host_skills.is_none() && !host_catalog_in_world_state, include_bundled_skills: config.bundled_skills_enabled, diff --git a/codex-rs/ext/skills/src/preparation.rs b/codex-rs/ext/skills/src/preparation.rs index fb4d40d29d..81e911fc0a 100644 --- a/codex-rs/ext/skills/src/preparation.rs +++ b/codex-rs/ext/skills/src/preparation.rs @@ -12,7 +12,6 @@ use crate::state::SkillsSessionState; use crate::state::SkillsThreadState; use crate::step_state::CatalogStatus; use crate::step_state::SkillsStepState; -use codex_exec_server::ResolvedSelectedCapabilityRoot; use codex_extension_api::StepPreparationInput; pub(crate) async fn prepare_catalogs( @@ -48,13 +47,7 @@ pub(crate) async fn prepare_catalogs( SkillsStepState { discovery_v2, tools_enabled: cloud_enabled - || input - .step_store - .get::>() - .map_or_else( - || !input.ready_selected_capability_roots.is_empty(), - |roots| !roots.is_empty(), - ) + || !input.ready_selected_capability_roots.is_empty() || discovery_v2, catalog: thread_state.prepare_step_catalog(cloud, executor, discovery_v2), host_catalog, @@ -70,26 +63,9 @@ async fn discover_executor_catalog( config: &SkillsExtensionConfig, cloud_enabled: bool, ) -> SkillCatalog { - // Core supplies captured roots even when none are ready. Descriptor-only callers - // retain their unbound discovery path. - let captured_roots = input - .step_store - .get::>(); - let executor_roots = captured_roots - .as_ref() - .map(|roots| { - roots - .iter() - .map(|root| root.selected_root().clone()) - .collect() - }) - .unwrap_or_else(|| input.ready_selected_capability_roots.to_vec()); let query = SkillListQuery { turn_id: input.turn_id.to_string(), - executor_roots, - resolved_executor_roots: captured_roots - .map(|roots| roots.as_ref().clone()) - .unwrap_or_default(), + executor_roots: input.ready_selected_capability_roots.to_vec(), host_snapshot: None, include_host_skills: false, include_bundled_skills: config.bundled_skills_enabled, @@ -121,7 +97,6 @@ async fn discover_host_catalog( .list_host_for_turn(SkillListQuery { turn_id: input.turn_id.to_string(), executor_roots: Vec::new(), - resolved_executor_roots: Vec::new(), host_snapshot: Some(host_snapshot), include_host_skills: true, include_bundled_skills: false, diff --git a/codex-rs/ext/skills/src/provider.rs b/codex-rs/ext/skills/src/provider.rs index 09e6419ec7..361f1f228a 100644 --- a/codex-rs/ext/skills/src/provider.rs +++ b/codex-rs/ext/skills/src/provider.rs @@ -10,9 +10,8 @@ use crate::HostSkillsSnapshot; use codex_exec_server::EnvironmentAccess; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; use codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot; -use codex_exec_server::ResolvedSelectedCapabilityRoot; +use codex_extension_api::AccessedSelectedCapabilityRoot; use codex_mcp::McpResourceClient; -use codex_protocol::capabilities::SelectedCapabilityRoot; use crate::catalog::SkillAuthority; use crate::catalog::SkillCatalog; @@ -29,11 +28,11 @@ pub use host::HostSkillProvider; pub(crate) const MAX_SKILL_RESOURCE_CONTENT_BYTES: usize = 1024 * 1024; -#[derive(Clone, Debug)] -pub struct SkillListQuery { +/// Catalog inputs with executor roots bound to filesystem access for this callback. +#[derive(Clone)] +pub struct SkillListQuery<'a> { pub turn_id: String, - pub executor_roots: Vec, - pub resolved_executor_roots: Vec, + pub executor_roots: Vec>, pub host_snapshot: Option>, pub include_host_skills: bool, pub include_bundled_skills: bool, @@ -95,7 +94,7 @@ pub trait SkillProvider: Send + Sync { } /// Providers set `is_partial` when listing omits entries; warnings are diagnostic only. - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog>; + fn list<'a>(&'a self, query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog>; fn read<'a>( &'a self, diff --git a/codex-rs/ext/skills/src/provider/executor.rs b/codex-rs/ext/skills/src/provider/executor.rs index bb16a39380..01292d9cbb 100644 --- a/codex-rs/ext/skills/src/provider/executor.rs +++ b/codex-rs/ext/skills/src/provider/executor.rs @@ -1,12 +1,9 @@ use std::collections::HashMap; use std::collections::HashSet; -use std::sync::Arc; use codex_exec_server::EnvironmentAccess; mod discovery_v2; -use codex_exec_server::EnvironmentManager; -use codex_exec_server::FileSystemEnvironmentAccessor; use codex_extension_api::SelectedPluginSnapshot; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::protocol::Product; @@ -38,18 +35,13 @@ use crate::provider::SkillSearchRequest; /// Discovers and reads skills through the filesystem owned by an execution environment. #[derive(Clone, Debug)] pub struct ExecutorSkillProvider { - environment_manager: Arc, restriction_product: Option, disabled_skill_paths: HashMap>, } impl ExecutorSkillProvider { - pub fn new_with_restriction_product( - environment_manager: Arc, - restriction_product: Option, - ) -> Self { + pub fn new_with_restriction_product(restriction_product: Option) -> Self { Self { - environment_manager, restriction_product, disabled_skill_paths: HashMap::new(), } @@ -87,7 +79,7 @@ pub(crate) fn attribute_executor_plugins( } impl SkillProvider for ExecutorSkillProvider { - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { Box::pin(async move { if let Some(snapshots) = &query.executor_capability_discovery_v2 { return Ok(self.list_from_discovery_v2(snapshots)); @@ -96,7 +88,9 @@ impl SkillProvider for ExecutorSkillProvider { return Ok(self.list_from_discovery(&discovery)); } let mut catalog = SkillCatalog::default(); - for selected_root in query.executor_roots { + for bound in query.executor_roots { + let fs = bound.fs; + let selected_root = bound.root; let selected_root_id = &selected_root.id; let CapabilityRootLocation::Environment { environment_id, @@ -104,30 +98,8 @@ impl SkillProvider for ExecutorSkillProvider { } = &selected_root.location; let authority = SkillAuthority::new(SkillSourceKind::Executor, selected_root_id.clone()); - let file_system = query - .resolved_executor_roots - .iter() - .find(|root| root.selected_root() == &selected_root) - .map(|root| root.environment().get_filesystem()) - .or_else(|| { - self.environment_manager - .get_environment(environment_id) - .map(|environment| environment.get_filesystem()) - }); - let Some(file_system) = file_system else { - catalog.warnings.push(format!( - "Selected capability root `{selected_root_id}` references unavailable environment `{environment_id}`." - )); - continue; - }; - // TODO(anp): Take this accessor from the selected turn root when discovery receives - // turn permissions; until then, preserve direct access through its existing filesystem. - let outcome = load_environment_skills_from_root( - &FileSystemEnvironmentAccessor::unrestricted(&file_system), - path, - self.restriction_product, - ) - .await; + let outcome = + load_environment_skills_from_root(fs, path, self.restriction_product).await; catalog.warnings.extend(outcome.warnings); for skill in outcome.skills { catalog.push_entry(catalog_entry_from_skill( diff --git a/codex-rs/ext/skills/src/provider/frozen_cloud.rs b/codex-rs/ext/skills/src/provider/frozen_cloud.rs index e162c821f6..7e2ac80f23 100644 --- a/codex-rs/ext/skills/src/provider/frozen_cloud.rs +++ b/codex-rs/ext/skills/src/provider/frozen_cloud.rs @@ -118,7 +118,7 @@ impl SkillProvider for FrozenCloudSkillProvider { false } - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { Box::pin(async move { Ok(if query.include_cloud_skills { self.catalog.clone() diff --git a/codex-rs/ext/skills/src/provider/host.rs b/codex-rs/ext/skills/src/provider/host.rs index d37a4803be..54a56c396d 100644 --- a/codex-rs/ext/skills/src/provider/host.rs +++ b/codex-rs/ext/skills/src/provider/host.rs @@ -36,7 +36,7 @@ impl HostSkillProvider { } impl SkillProvider for HostSkillProvider { - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { Box::pin(async move { let Some(host_snapshot) = query.host_snapshot else { return Err(SkillProviderError::new( diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index b01411694f..22e4c17541 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -640,7 +640,6 @@ async fn host_alias_roots_follow_core_discovery_order() -> Result<(), Box bool { + fn should_list(&self, query: &SkillListQuery<'_>) -> bool { match &self.kind { SkillSourceKind::Host => query.include_host_skills, SkillSourceKind::Executor => { @@ -126,14 +126,14 @@ impl SkillProviders { .any(|source| source.kind == SkillSourceKind::Host) } - pub(crate) async fn list_for_turn(&self, query: SkillListQuery) -> SkillCatalog { + pub(crate) async fn list_for_turn(&self, query: SkillListQuery<'_>) -> SkillCatalog { self.list_matching(&query, |source| source.should_list(&query)) .await } pub(crate) async fn list_cloud_for_turn( &self, - query: SkillListQuery, + query: SkillListQuery<'_>, ) -> SkillProviderResult { let mut catalog = SkillCatalog::default(); @@ -154,19 +154,19 @@ impl SkillProviders { Ok(catalog) } - pub(crate) async fn list_executor_for_turn(&self, query: SkillListQuery) -> SkillCatalog { + pub(crate) async fn list_executor_for_turn(&self, query: SkillListQuery<'_>) -> SkillCatalog { self.list_matching(&query, |source| source.kind == SkillSourceKind::Executor) .await } - pub(crate) async fn list_host_for_turn(&self, query: SkillListQuery) -> SkillCatalog { + pub(crate) async fn list_host_for_turn(&self, query: SkillListQuery<'_>) -> SkillCatalog { self.list_matching(&query, |source| source.kind == SkillSourceKind::Host) .await } async fn list_matching( &self, - query: &SkillListQuery, + query: &SkillListQuery<'_>, should_list: impl Fn(&SkillProviderSource) -> bool, ) -> SkillCatalog { let mut catalog = SkillCatalog::default(); diff --git a/codex-rs/ext/skills/src/state.rs b/codex-rs/ext/skills/src/state.rs index 1f7eb84722..4105e7c815 100644 --- a/codex-rs/ext/skills/src/state.rs +++ b/codex-rs/ext/skills/src/state.rs @@ -147,7 +147,7 @@ impl SkillsThreadState { pub(crate) async fn refresh_executor_catalog( &self, providers: &SkillProviders, - query: SkillListQuery, + query: SkillListQuery<'_>, ) -> SkillCatalog { if let Some(snapshots) = &query.executor_capability_discovery_v2 { let snapshots = snapshots.clone(); @@ -204,7 +204,11 @@ impl SkillsThreadState { let key = ExecutorCatalogCacheKey { turn_id: query.turn_id.clone(), - resolved_roots: query.resolved_executor_roots.clone(), + roots: query + .executor_roots + .iter() + .map(|root| (root.root.clone(), root.fs.cache_key())) + .collect(), include_bundled_skills: query.include_bundled_skills, }; let catalog = if query.executor_roots.is_empty() { @@ -228,7 +232,7 @@ impl SkillsThreadState { async fn executor_discovery_catalog_snapshot( &self, providers: &SkillProviders, - query: SkillListQuery, + query: SkillListQuery<'_>, ) -> SkillCatalog { let discovery = query.executor_capability_discovery.clone(); let discovery_failed = discovery.as_ref().is_some_and(|discovery| { @@ -244,20 +248,16 @@ impl SkillsThreadState { .catalog_state .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - let cached = state.executor_discovery_cache.as_ref().filter(|cached| { - cached.matches( - &query.executor_roots, - discovery.as_ref(), - query.include_bundled_skills, - ) - }); + let cached = state + .executor_discovery_cache + .as_ref() + .filter(|cached| cached.matches(discovery.as_ref(), query.include_bundled_skills)); if !discovery_failed && let Some(cached) = cached { return cached.catalog.clone(); } cached.is_some() }; - let roots = query.executor_roots.clone(); let include_bundled_skills = query.include_bundled_skills; let discovered = providers.list_executor_for_turn(query).await; if !already_observed { @@ -270,7 +270,6 @@ impl SkillsThreadState { // Failed discovery is returned to this caller but is never reused by another request. state.executor_discovery_cache = discovery.map(|discovery| CachedExecutorDiscoveryCatalog { - roots, discovery, include_bundled_skills, catalog: discovered.clone(), @@ -299,7 +298,7 @@ impl SkillsThreadState { pub(crate) async fn refresh_cloud_catalog( &self, providers: &SkillProviders, - query: SkillListQuery, + query: SkillListQuery<'_>, turn_store: &ExtensionData, ) -> SkillProviderResult<()> { // Wait without holding config/catalog state or the MCP runtime permit. diff --git a/codex-rs/ext/skills/tests/executor_file_system_authority.rs b/codex-rs/ext/skills/tests/executor_file_system_authority.rs index 70ba0221ab..73a93dfd2b 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -1,4 +1,3 @@ -use codex_skills::ImplicitSkillLookup; use std::collections::HashMap; use std::io; use std::sync::Arc; @@ -39,14 +38,14 @@ use codex_exec_server::WalkOutcome; #[cfg(windows)] use codex_exec_server::WindowsSandboxSelection; use codex_exec_server::WriteFileOptions; +use codex_extension_api::AccessedSelectedCapabilityRoot; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_protocol::models::PermissionProfile; -#[cfg(windows)] use codex_protocol::permissions::FileSystemSandboxPolicy; -#[cfg(windows)] use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::Product; +use codex_skills::ImplicitSkillLookup; use codex_skills_extension::ExecutorSkillProvider; use codex_skills_extension::HostSkillsLoadInput; use codex_skills_extension::HostSkillsService; @@ -366,10 +365,9 @@ async fn reads_use_callback_access_without_capability_roots() { let environment = manager.get_environment("local").expect("local environment"); let fs = environment.get_filesystem(); let access = FileSystemEnvironmentAccessor::unrestricted(&fs); - let provider = ExecutorSkillProvider::new_with_restriction_product( - manager, /*restriction_product*/ None, - ); - // This ID is absent from the provider's registry; the callback owns its filesystem access. + let provider = + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None); + // The callback owns filesystem access for this environment ID. let resource = SkillResourceId::environment( "skill://root/skill/SKILL.md", "captured", @@ -396,10 +394,8 @@ async fn reads_use_callback_access_without_capability_roots() { #[tokio::test] async fn executor_skill_reads_reject_invalid_resource_handles() { let access = FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS); - let provider = ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), - /*restriction_product*/ None, - ); + let provider = + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None); for (resource, message) in [ ( "skill://root/package/../secret", @@ -442,10 +438,8 @@ async fn windows_executor_skill_read_requires_a_requested_sandbox() -> io::Resul )?; let file_system: Arc = Arc::new(LocalFileSystem::with_runtime_paths(runtime_paths)); - let provider = ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), - /*restriction_product*/ None, - ); + let provider = + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None); let resource = SkillResourceId::environment( "skill://windows-root/skill/SKILL.md", "local", @@ -511,24 +505,29 @@ async fn selected_root_id_distinguishes_identical_executor_paths() { } else { selected_root }; - let provider = ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), - /*restriction_product*/ None, + let provider = + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None); + let access = FileSystemEnvironmentAccessor::new( + &LOCAL_FS, + FileSystemSandboxContext::from_permission_profile( + PermissionProfile::Disabled, + PathUri::from_host_native_path(&test_root).expect("skill root URI"), + ), ); + let selected_roots = ["root-a", "root-b"].map(|id| SelectedCapabilityRoot { + id: id.to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "local".to_string(), + path: PathUri::from_host_native_path(&test_root).expect("skill root URI"), + }, + }); let catalog = provider .list(SkillListQuery { turn_id: "turn-1".to_string(), - executor_roots: ["root-a", "root-b"] - .into_iter() - .map(|id| SelectedCapabilityRoot { - id: id.to_string(), - location: CapabilityRootLocation::Environment { - environment_id: "local".to_string(), - path: PathUri::from_host_native_path(&test_root).expect("skill root URI"), - }, - }) + executor_roots: selected_roots + .iter() + .map(|root| AccessedSelectedCapabilityRoot { root, fs: &access }) .collect(), - resolved_executor_roots: Vec::new(), host_snapshot: None, include_host_skills: false, include_bundled_skills: true, @@ -572,10 +571,8 @@ async fn selected_root_id_distinguishes_identical_executor_paths() { #[tokio::test] async fn executor_discovery_preserves_posix_and_windows_locator_alias_roots() { - let provider = ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), - /*restriction_product*/ None, - ); + let provider = + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None); for (root_uri, alias_root) in [ ( @@ -618,8 +615,7 @@ async fn executor_discovery_preserves_posix_and_windows_locator_alias_roots() { let catalog = provider .list(SkillListQuery { turn_id: "turn-1".to_string(), - executor_roots: vec![selected_root], - resolved_executor_roots: Vec::new(), + executor_roots: Vec::new(), host_snapshot: None, include_host_skills: false, include_bundled_skills: true, @@ -724,10 +720,7 @@ async fn executor_discovery_routes_produce_equivalent_catalog_metadata() { .expect("write invalid metadata"); let manager = Arc::new(EnvironmentManager::default_for_tests()); - let provider = ExecutorSkillProvider::new_with_restriction_product( - Arc::clone(&manager), - Some(Product::Codex), - ); + let provider = ExecutorSkillProvider::new_with_restriction_product(Some(Product::Codex)); let executor_roots = vec![SelectedCapabilityRoot { id: "parity-root".to_string(), location: CapabilityRootLocation::Environment { @@ -735,10 +728,19 @@ async fn executor_discovery_routes_produce_equivalent_catalog_metadata() { path: PathUri::from_host_native_path(&test_root).expect("skill root URI"), }, }]; + let access = FileSystemEnvironmentAccessor::new( + &LOCAL_FS, + FileSystemSandboxContext::from_permission_profile( + PermissionProfile::Disabled, + PathUri::from_host_native_path(&test_root).expect("skill root URI"), + ), + ); let query = |executor_capability_discovery| SkillListQuery { turn_id: "turn-1".to_string(), - executor_roots: executor_roots.clone(), - resolved_executor_roots: Vec::new(), + executor_roots: executor_roots + .iter() + .map(|root| AccessedSelectedCapabilityRoot { root, fs: &access }) + .collect(), host_snapshot: None, include_host_skills: false, include_bundled_skills: true, @@ -787,6 +789,32 @@ async fn executor_discovery_routes_produce_equivalent_catalog_metadata() { .collect::>() }; assert_eq!(comparable_entries(&bundled), comparable_entries(&direct)); + // This filesystem has no sandbox helper: restricted discovery must fail closed instead + // of falling back to its ambient access, and restoring access must discover the same skills. + let restricted = FileSystemEnvironmentAccessor::new( + &LOCAL_FS, + FileSystemSandboxContext::from_permission_profile( + PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(Vec::new()), + NetworkSandboxPolicy::Restricted, + ), + PathUri::from_host_native_path(&test_root).expect("skill root URI"), + ), + ); + let mut restricted_query = query(None); + restricted_query.executor_roots[0].fs = &restricted; + assert!( + provider + .list(restricted_query) + .await + .unwrap() + .entries + .is_empty() + ); + assert_eq!( + comparable_entries(&provider.list(query(None)).await.unwrap()), + comparable_entries(&direct), + ); assert_eq!(direct.warnings.len(), 1); assert!(direct.warnings[0].contains("missing field `description`")); let entry = direct @@ -879,10 +907,8 @@ async fn pre_discovered_executor_catalog_snapshot() { .expect("write skill metadata"); let manager = Arc::new(EnvironmentManager::default_for_tests()); - let provider = ExecutorSkillProvider::new_with_restriction_product( - Arc::clone(&manager), - /*restriction_product*/ None, - ); + let provider = + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None); let executor_roots = vec![SelectedCapabilityRoot { id: "snapshot-root".to_string(), location: CapabilityRootLocation::Environment { @@ -902,8 +928,7 @@ async fn pre_discovered_executor_catalog_snapshot() { let catalog = provider .list(SkillListQuery { turn_id: "turn-1".to_string(), - executor_roots, - resolved_executor_roots: Vec::new(), + executor_roots: Vec::new(), host_snapshot: None, include_host_skills: false, include_bundled_skills: true, @@ -1013,15 +1038,19 @@ async fn direct_executor_discovery_preserves_hidden_nested_and_probed_metadata() path: root_uri.clone(), }, }; - let manager = Arc::new(EnvironmentManager::default_for_tests()); - let provider = ExecutorSkillProvider::new_with_restriction_product( - manager, /*restriction_product*/ None, + let provider = + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None); + let access = FileSystemEnvironmentAccessor::new( + &LOCAL_FS, + FileSystemSandboxContext::from_permission_profile(PermissionProfile::Disabled, root_uri), ); let direct = provider .list(SkillListQuery { turn_id: "turn-1".to_string(), - executor_roots: vec![selected_root], - resolved_executor_roots: Vec::new(), + executor_roots: vec![AccessedSelectedCapabilityRoot { + root: &selected_root, + fs: &access, + }], host_snapshot: None, include_host_skills: false, include_bundled_skills: true, @@ -1064,10 +1093,8 @@ async fn direct_executor_discovery_preserves_hidden_nested_and_probed_metadata() async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() { let test_root = create_local_skill_root("materialized").expect("create local skill root"); let manager = Arc::new(EnvironmentManager::default_for_tests()); - let provider = ExecutorSkillProvider::new_with_restriction_product( - Arc::clone(&manager), - /*restriction_product*/ None, - ); + let provider = + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None); let executor_roots = vec![SelectedCapabilityRoot { id: "materialized-root".to_string(), location: CapabilityRootLocation::Environment { @@ -1087,8 +1114,7 @@ async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() { let catalog = provider .list(SkillListQuery { turn_id: "turn-1".to_string(), - executor_roots, - resolved_executor_roots: Vec::new(), + executor_roots: Vec::new(), host_snapshot: None, include_host_skills: false, include_bundled_skills: true, diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index 25090eaef2..24d4b9b94f 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -21,6 +21,7 @@ use codex_exec_server::ExecutorSkill; use codex_exec_server::FileSystemEnvironmentAccessor; use codex_exec_server::LOCAL_FS; use codex_exec_server::SkillDiscoveryRoot; +use codex_extension_api::AccessedSelectedCapabilityRoot; use codex_extension_api::ConversationHistory; use codex_extension_api::ExtensionData; use codex_extension_api::ExtensionEventSink; @@ -154,11 +155,40 @@ async fn prepared_world_state( .step_store .get::() .unwrap_or_default(); + let access = FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS); + let captured_roots = input + .step_store + .get::>(); + let captured_access = captured_roots + .as_ref() + .map(|roots| { + roots + .iter() + .map(|root| { + FileSystemEnvironmentAccessor::unrestricted(root.environment().filesystem_ref()) + }) + .collect::>() + }) + .unwrap_or_default(); + let accessed_roots = match &captured_roots { + Some(roots) => roots + .iter() + .zip(&captured_access) + .map(|(root, fs)| AccessedSelectedCapabilityRoot { + root: root.selected_root(), + fs, + }) + .collect::>(), + None => ready_selected_capability_roots + .iter() + .map(|root| AccessedSelectedCapabilityRoot { root, fs: &access }) + .collect(), + }; prepare_step( registry, codex_extension_api::StepPreparationInput { turn_id: input.turn_id, - ready_selected_capability_roots, + ready_selected_capability_roots: &accessed_roots, executor_capability_discovery: discovery.as_deref(), executor_capability_discovery_v2: snapshots.as_deref().map(Vec::as_slice), selected_plugins: &selected_plugins, @@ -207,7 +237,18 @@ async fn prepared_turn_input( codex_extension_api::StepPreparationInput { selected_plugins: &selected_plugins, turn_id: &input.turn_id, - ready_selected_capability_roots: &roots, + ready_selected_capability_roots: &roots + .iter() + .filter_map(|root| { + let CapabilityRootLocation::Environment { environment_id, .. } = &root.location; + let fs = input + .environments + .iter() + .find(|environment| environment.environment_id == *environment_id)? + .fs; + Some(AccessedSelectedCapabilityRoot { root, fs }) + }) + .collect::>(), executor_capability_discovery: discovery.as_deref(), executor_capability_discovery_v2: snapshots.as_deref().map(Vec::as_slice), session_store, @@ -829,7 +870,7 @@ async fn nonempty_executor_empty_host_records_catalog_metrics() -> TestResult { }) .await; - let selected_roots = vec![SelectedCapabilityRoot { + let selected_roots = [SelectedCapabilityRoot { id: "lint-fix".to_string(), location: CapabilityRootLocation::Environment { environment_id: "env-1".to_string(), @@ -1597,7 +1638,7 @@ async fn selected_executor_catalog_caches_root_selections_per_turn_and_reuses_di ("cache-turn-1", vec![first_root.clone()], true, 4), ("cache-turn-2", vec![first_root.clone()], true, 5), ("cache-turn-2", vec![first_root.clone()], false, 6), - ("cache-turn-2", vec![first_root], false, 6), + ("cache-turn-2", vec![first_root.clone()], false, 6), ] { let mut next_config = current_config.clone(); next_config.bundled_skills_enabled = include_bundled_skills; @@ -1638,6 +1679,46 @@ async fn selected_executor_catalog_caches_root_selections_per_turn_and_reuses_di ); } + // Keep the turn, root, and filesystem fixed while access changes and is restored. + let open = FileSystemEnvironmentAccessor::unrestricted(cache_environment.filesystem_ref()); + let changed = FileSystemEnvironmentAccessor::new( + cache_environment.filesystem_ref(), + codex_exec_server::FileSystemSandboxContext::from_permission_profile( + codex_protocol::models::PermissionProfile::read_only(), + PathUri::parse("file:///skills")?, + ), + ); + let turn_store = ExtensionData::new("cache-turn-2"); + for (access, expected_new_calls) in [ + (&open as &dyn codex_exec_server::EnvironmentAccess, 6), + (&changed, 7), + (&changed, 7), + (&open, 8), + ] { + prepare_step( + ®istry, + codex_extension_api::StepPreparationInput { + turn_id: "cache-turn-2", + ready_selected_capability_roots: &[AccessedSelectedCapabilityRoot { + root: &first_root, + fs: access, + }], + executor_capability_discovery: None, + executor_capability_discovery_v2: None, + selected_plugins: &Default::default(), + session_store: &session_store, + thread_store: &thread_store, + turn_store: &turn_store, + step_store: &ExtensionData::new("permission-step"), + }, + ) + .await; + assert_eq!( + previous_list_calls + expected_new_calls, + list_calls.load(Ordering::Relaxed), + ); + } + Ok(()) } @@ -1694,7 +1775,6 @@ async fn overlapping_executor_catalogs_render_their_own_roots() -> TestResult { let root_b = root("skill-b")?; let mut builder = ExtensionRegistryBuilder::new(); let provider = codex_skills_extension::ExecutorSkillProvider::new_with_restriction_product( - Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()), /*restriction_product*/ None, ); install_with_providers( @@ -2668,9 +2748,7 @@ async fn v2_root_aliases_and_read_cache_use_the_captured_environment() -> TestRe .get_environment("local") .ok_or("replacement environment")?; let providers = SkillProviders::new().with_executor_provider(Arc::new( - ExecutorSkillProvider::new_with_restriction_product( - manager, /*restriction_product*/ None, - ), + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None), )); let mut builder = ExtensionRegistryBuilder::new(); install_with_providers(&mut builder, providers, skills_extension_config); @@ -3040,7 +3118,7 @@ async fn model_context_window_scales_executor_and_cloud_catalogs() -> TestResult .await; assert!(thread_fragments.is_empty()); - let selected_roots = vec![SelectedCapabilityRoot { + let selected_roots = [SelectedCapabilityRoot { id: "skills".to_string(), location: CapabilityRootLocation::Environment { environment_id: "env-1".to_string(), @@ -3172,7 +3250,7 @@ async fn executor_catalog_emits_at_most_four_warnings() -> TestResult { thread_store: &thread_store, }) .await; - let selected_roots = vec![SelectedCapabilityRoot { + let selected_roots = [SelectedCapabilityRoot { id: "skills".to_string(), location: CapabilityRootLocation::Environment { environment_id: "env-1".to_string(), @@ -3551,7 +3629,7 @@ fn expected_catalog_metric_samples(catalog_surface: &str, count: i64) -> Vec SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, _query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { let list_call = self .list_calls .as_ref() @@ -3705,7 +3783,7 @@ async fn shared_catalog_keeps_captured_executor_and_separates_bindings_in_one_tu Some(std::time::Duration::from_millis(100)), )?; let provider = Arc::new(ExecutorSkillProvider::new_with_restriction_product( - manager, /*restriction_product*/ None, + /*restriction_product*/ None, )); let mut builder = ExtensionRegistryBuilder::new(); install_with_providers( diff --git a/codex-rs/ext/skills/tests/skills_extension/discovery_v2_tests.rs b/codex-rs/ext/skills/tests/skills_extension/discovery_v2_tests.rs index b7c610f779..6a9ff3619a 100644 --- a/codex-rs/ext/skills/tests/skills_extension/discovery_v2_tests.rs +++ b/codex-rs/ext/skills/tests/skills_extension/discovery_v2_tests.rs @@ -3,7 +3,6 @@ use super::*; use codex_exec_server::DiscoverV2CapabilitiesResponse; use codex_exec_server::DiscoveredPluginFiles; -use codex_exec_server::EnvironmentManager; use codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot; use codex_exec_server::ExecutorPlugin; use codex_exec_server::ExecutorSkill; @@ -56,7 +55,6 @@ fn installation( #[tokio::test] async fn v2_steps_refresh_metadata_and_preserve_separate_environments() -> TestResult { let provider = Arc::new(ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), /*restriction_product*/ None, )); let dependencies = Some(SkillDependencies { @@ -94,7 +92,6 @@ async fn v2_steps_refresh_metadata_and_preserve_separate_environments() -> TestR let query = SkillListQuery { turn_id: "turn-1".to_string(), executor_roots: Vec::new(), - resolved_executor_roots: Vec::new(), host_snapshot: None, include_host_skills: false, include_bundled_skills: false, @@ -284,26 +281,28 @@ async fn v2_steps_refresh_metadata_and_preserve_separate_environments() -> TestR #[tokio::test] async fn v2_empty_failed_and_partial_results_never_load_legacy_roots() -> TestResult { - let provider = ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), - /*restriction_product*/ None, - ); + let provider = + ExecutorSkillProvider::new_with_restriction_product(/*restriction_product*/ None); let successful = installation("executor-a")?; let failed = ExecutorCapabilityDiscoveryV2Snapshot { environment_id: "executor-b".to_string(), result: Err("discovery unavailable".to_string()), }; + let root = SelectedCapabilityRoot { + id: "legacy".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "unavailable-legacy-executor".to_string(), + path: PathUri::parse("file:///legacy").unwrap(), + }, + }; + let access = FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS); let query = |snapshots| SkillListQuery { turn_id: "turn-1".to_string(), - // A legacy lookup would report an unavailable environment. V2 must ignore this root. - executor_roots: vec![SelectedCapabilityRoot { - id: "legacy".to_string(), - location: CapabilityRootLocation::Environment { - environment_id: "unavailable-legacy-executor".to_string(), - path: PathUri::parse("file:///legacy").unwrap(), - }, + // V2 must ignore this unavailable legacy root. + executor_roots: vec![AccessedSelectedCapabilityRoot { + root: &root, + fs: &access, }], - resolved_executor_roots: Vec::new(), host_snapshot: None, include_host_skills: false, include_bundled_skills: false, diff --git a/codex-rs/ext/skills/tests/skills_extension/merge_catalog_tests.rs b/codex-rs/ext/skills/tests/skills_extension/merge_catalog_tests.rs index 403dbe1033..1997379e3b 100644 --- a/codex-rs/ext/skills/tests/skills_extension/merge_catalog_tests.rs +++ b/codex-rs/ext/skills/tests/skills_extension/merge_catalog_tests.rs @@ -155,12 +155,17 @@ impl MergeHarness { step.insert(plugins.as_ref().clone()); } let selected_plugins = step.get::().unwrap_or_default(); + let access = FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS); prepare_step( &self.registry, codex_extension_api::StepPreparationInput { selected_plugins: &selected_plugins, turn_id: self.turn_store.level_id(), - ready_selected_capability_roots: &self.roots, + ready_selected_capability_roots: &self + .roots + .iter() + .map(|root| AccessedSelectedCapabilityRoot { root, fs: &access }) + .collect::>(), executor_capability_discovery: self.discovery.as_ref(), executor_capability_discovery_v2: self.snapshots.as_deref(), session_store: &self.session_store, diff --git a/codex-rs/ext/skills/tests/skills_extension/preparation_tests.rs b/codex-rs/ext/skills/tests/skills_extension/preparation_tests.rs index eff8de50b6..f937ce84e0 100644 --- a/codex-rs/ext/skills/tests/skills_extension/preparation_tests.rs +++ b/codex-rs/ext/skills/tests/skills_extension/preparation_tests.rs @@ -11,7 +11,7 @@ struct PausedExecutor { } impl SkillProvider for PausedExecutor { - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { Box::pin(async move { self.queries .lock() diff --git a/codex-rs/ext/skills/tests/skills_extension/shadow_task_context_tests.rs b/codex-rs/ext/skills/tests/skills_extension/shadow_task_context_tests.rs index fc56eda6cf..8ee745de8a 100644 --- a/codex-rs/ext/skills/tests/skills_extension/shadow_task_context_tests.rs +++ b/codex-rs/ext/skills/tests/skills_extension/shadow_task_context_tests.rs @@ -51,7 +51,7 @@ pub(super) async fn collect_shadow_observations( struct FailedReads(StaticSkillProvider); impl SkillProvider for FailedReads { - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { + fn list<'a>(&'a self, query: SkillListQuery<'a>) -> SkillProviderFuture<'a, SkillCatalog> { self.0.list(query) }