openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-07 20:30:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

Respect turn filesystem permissions during executor skill discovery (#59291)

Browse Source 
## 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
This commit is contained in:
Dmitriy Kovalenko
2026-11-04 02:00:01 +00:00
committed by copyberry
parent db3c44414c
commit 007f668c34
29 changed files with 294 additions and 254 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/session/mod.rs
+20 -1
View File
@@ -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::<Vec<_>>();
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(),
codex-rs/core/tests/suite/executor_discovery_v2.rs
+2 -3
View File
@@ -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 {
codex-rs/core/tests/suite/scenarios.rs
+7 -10
View File
@@ -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::<Config>::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,
),
)),
codex-rs/core/tests/suite/scenarios_frozen_cloud_skills.rs
-1
View File
@@ -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,
),
)),
codex-rs/core/tests/suite/skills_extension.rs
+10 -31
View File
@@ -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::<Config>::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,
),
)),
codex-rs/core/tests/suite/skills_extension/cloud_lifecycle_tests.rs
+1 -1
View File
@@ -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
codex-rs/core/tests/suite/skills_extension/cloud_skill_tests.rs
+1 -1
View File
@@ -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()) })
}