Respect filesystem permissions during capability discovery (#36124)

## Why

Executor capability discovery can traverse plugin and skill roots, including
symlinks. Under restricted filesystem permissions, discovery must not expose
files outside the permitted paths.

## What changed

- Pass each environment's filesystem sandbox context through capability root
  discovery and apply it to metadata, directory walks, and file reads.
- Enable discovery for restricted sessions so permitted executor skills remain
  available while inaccessible roots and symlink targets are omitted.
- Key discovery caches by sandbox context and reject sandboxed discovery on
  executors that do not advertise support for it.
- Split requests with more than 128 roots into supported-size batches.

## Testing

- Cover permitted and denied external symlink targets.
- Verify restricted skill listing excludes inaccessible skills.
- Verify cache separation across permission contexts and discovery of 129 roots.

GitOrigin-RevId: 44d16468ca003403bdb8b71a04ae8c9ff94ed494
This commit is contained in:
jif authored and copyberry committed 2026-07-30 10:01:06 +00:00
1 parent 3d805abdf0
commit 5decb399ae
14 files changed
+510 -70

No files matched your search

@@ -36,6 +36,7 @@ const SKILL_NAME: &str = "demo-plugin:deploy";
const SKILL_MARKER: &str = "EXECUTOR_SKILL_BODY_MARKER"; const SKILL_MARKER: &str = "EXECUTOR_SKILL_BODY_MARKER";
const LOCAL_SKILL_MARKER: &str = "LOCAL_SKILL_BODY_MARKER"; const LOCAL_SKILL_MARKER: &str = "LOCAL_SKILL_BODY_MARKER";
const REFERENCE_MARKER: &str = "EXECUTOR_SKILL_REFERENCE_MARKER"; const REFERENCE_MARKER: &str = "EXECUTOR_SKILL_REFERENCE_MARKER";
const DENIED_SKILL_NAME: &str = "demo-plugin:denied";
#[derive(Clone, Copy, Debug, Eq, PartialEq)] #[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum ExecutorSkillScenario { enum ExecutorSkillScenario {
@@ -43,6 +44,7 @@ enum ExecutorSkillScenario {
ExplicitOnly, ExplicitOnly,
RestrictedPermittedReference, RestrictedPermittedReference,
RestrictedDeniedReference, RestrictedDeniedReference,
RestrictedVisible,
} }
#[tokio::test] #[tokio::test]
@@ -66,11 +68,17 @@ async fn restricted_executor_skill_rejects_reference_until_permission_approved()
exercise_executor_skill(ExecutorSkillScenario::RestrictedDeniedReference).await exercise_executor_skill(ExecutorSkillScenario::RestrictedDeniedReference).await
} }
#[tokio::test]
async fn restricted_executor_skill_is_listed_only_when_permitted() -> Result<()> {
exercise_executor_skill(ExecutorSkillScenario::RestrictedVisible).await
}
async fn exercise_executor_skill(scenario: ExecutorSkillScenario) -> Result<()> { async fn exercise_executor_skill(scenario: ExecutorSkillScenario) -> Result<()> {
let restricted = matches!( let restricted = matches!(
scenario, scenario,
ExecutorSkillScenario::RestrictedPermittedReference ExecutorSkillScenario::RestrictedPermittedReference
| ExecutorSkillScenario::RestrictedDeniedReference | ExecutorSkillScenario::RestrictedDeniedReference
| ExecutorSkillScenario::RestrictedVisible
); );
if restricted { if restricted {
skip_if_target_windows!( skip_if_target_windows!(
@@ -162,9 +170,13 @@ stream_max_retries = 0
ExecutorSkillScenario::VisibleWithBudgetWarning => 600 * 1024, ExecutorSkillScenario::VisibleWithBudgetWarning => 600 * 1024,
ExecutorSkillScenario::ExplicitOnly ExecutorSkillScenario::ExplicitOnly
| ExecutorSkillScenario::RestrictedPermittedReference | ExecutorSkillScenario::RestrictedPermittedReference
| ExecutorSkillScenario::RestrictedDeniedReference => 40 * 1024, | ExecutorSkillScenario::RestrictedDeniedReference
| ExecutorSkillScenario::RestrictedVisible => 40 * 1024,
}; };
let allow_implicit_invocation = scenario == ExecutorSkillScenario::VisibleWithBudgetWarning; let allow_implicit_invocation = matches!(
scenario,
ExecutorSkillScenario::VisibleWithBudgetWarning | ExecutorSkillScenario::RestrictedVisible
);
let reference_contents = format!("{REFERENCE_MARKER}\n{}", "x".repeat(reference_size)); let reference_contents = format!("{REFERENCE_MARKER}\n{}", "x".repeat(reference_size));
tokio::try_join!( tokio::try_join!(
file_system.write_file( file_system.write_file(
@@ -207,6 +219,19 @@ stream_max_retries = 0
std::fs::remove_file(reference_native_path.as_path())?; std::fs::remove_file(reference_native_path.as_path())?;
std::os::unix::fs::symlink(external_reference, reference_native_path.as_path())?; std::os::unix::fs::symlink(external_reference, reference_native_path.as_path())?;
} }
#[cfg(unix)]
if scenario == ExecutorSkillScenario::RestrictedVisible && !auto_env.environment().is_remote() {
let denied_skill_dir = codex_home.path().join("denied-skill");
std::fs::create_dir_all(&denied_skill_dir)?;
std::fs::write(
denied_skill_dir.join("SKILL.md"),
"---\nname: denied\ndescription: Skill outside the permitted workspace.\n---\n",
)?;
std::os::unix::fs::symlink(
denied_skill_dir,
plugin_dir.to_abs_path()?.join("skills/denied"),
)?;
}
if scenario == ExecutorSkillScenario::VisibleWithBudgetWarning { if scenario == ExecutorSkillScenario::VisibleWithBudgetWarning {
futures::stream::iter(0..200) futures::stream::iter(0..200)
.map(|index| { .map(|index| {
@@ -431,7 +456,8 @@ stream_max_retries = 0
assert!(skill_fragment.contains(SKILL_MARKER)); assert!(skill_fragment.contains(SKILL_MARKER));
assert!(!skill_fragment.contains(LOCAL_SKILL_MARKER)); assert!(!skill_fragment.contains(LOCAL_SKILL_MARKER));
match scenario { match scenario {
ExecutorSkillScenario::VisibleWithBudgetWarning => { ExecutorSkillScenario::VisibleWithBudgetWarning
| ExecutorSkillScenario::RestrictedVisible => {
assert!(!skill_fragment.contains("<resource_access>")); assert!(!skill_fragment.contains("<resource_access>"));
} }
ExecutorSkillScenario::ExplicitOnly ExecutorSkillScenario::ExplicitOnly
@@ -459,7 +485,8 @@ stream_max_retries = 0
.expect("skills.list output"), .expect("skills.list output"),
)?; )?;
match scenario { match scenario {
ExecutorSkillScenario::VisibleWithBudgetWarning => { ExecutorSkillScenario::VisibleWithBudgetWarning
| ExecutorSkillScenario::RestrictedVisible => {
let deploy_skill = list_output["skills"] let deploy_skill = list_output["skills"]
.as_array() .as_array()
.and_then(|skills| skills.iter().find(|skill| skill["name"] == SKILL_NAME)) .and_then(|skills| skills.iter().find(|skill| skill["name"] == SKILL_NAME))
@@ -474,7 +501,16 @@ stream_max_retries = 0
"main_resource": main_resource, "main_resource": main_resource,
}) })
); );
assert!(list_output["next_cursor"].is_string()); assert!(list_output["skills"].as_array().is_none_or(|skills| {
skills
.iter()
.all(|skill| skill["name"] != DENIED_SKILL_NAME)
}));
if scenario == ExecutorSkillScenario::VisibleWithBudgetWarning {
assert!(list_output["next_cursor"].is_string());
} else {
assert!(list_output["next_cursor"].is_null());
}
} }
ExecutorSkillScenario::ExplicitOnly ExecutorSkillScenario::ExplicitOnly
| ExecutorSkillScenario::RestrictedPermittedReference | ExecutorSkillScenario::RestrictedPermittedReference
@@ -512,7 +548,8 @@ stream_max_retries = 0
} }
ExecutorSkillScenario::ExplicitOnly ExecutorSkillScenario::ExplicitOnly
| ExecutorSkillScenario::RestrictedPermittedReference | ExecutorSkillScenario::RestrictedPermittedReference
| ExecutorSkillScenario::RestrictedDeniedReference => { | ExecutorSkillScenario::RestrictedDeniedReference
| ExecutorSkillScenario::RestrictedVisible => {
assert!(reference_output["next_cursor"].is_null()); assert!(reference_output["next_cursor"].is_null());
} }
} }
@@ -589,6 +589,7 @@ async fn executor_bundle_parser_matches_the_existing_environment_loader() {
roots: vec![CapabilityRootDiscoverRequest { roots: vec![CapabilityRootDiscoverRequest {
id: "demo@1".to_string(), id: "demo@1".to_string(),
path: root_uri, path: root_uri,
sandbox: None,
}], }],
}, },
) )
@@ -656,6 +657,7 @@ async fn executor_bundle_preserves_parent_namespace_and_manifest_precedence() {
roots: vec![CapabilityRootDiscoverRequest { roots: vec![CapabilityRootDiscoverRequest {
id: "skills-only".to_string(), id: "skills-only".to_string(),
path: root_uri, path: root_uri,
sandbox: None,
}], }],
}, },
) )
+72 -5
View File
@@ -2,12 +2,14 @@ use super::mcp_refresh::McpRefreshInvalidationGuard;
use super::*; use super::*;
use codex_exec_server::ExecutorCapabilityDiscoveryCache; use codex_exec_server::ExecutorCapabilityDiscoveryCache;
use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot;
use codex_exec_server::FileSystemSandboxContext;
use codex_exec_server::MAX_SELECTED_CAPABILITY_ROOTS; use codex_exec_server::MAX_SELECTED_CAPABILITY_ROOTS;
use codex_exec_server::ResolvedSelectedCapabilityRoot; use codex_exec_server::ResolvedSelectedCapabilityRoot;
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
use codex_mcp::ElicitationReviewRequest; use codex_mcp::ElicitationReviewRequest;
use codex_mcp::ElicitationReviewer; use codex_mcp::ElicitationReviewer;
use codex_mcp::ElicitationReviewerHandle; use codex_mcp::ElicitationReviewerHandle;
use codex_protocol::capabilities::CapabilityRootLocation;
use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_protocol::capabilities::SelectedCapabilityRoot;
use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::mcp_approval_meta::APPROVAL_KIND_KEY as MCP_ELICITATION_APPROVAL_KIND_KEY; use codex_protocol::mcp_approval_meta::APPROVAL_KIND_KEY as MCP_ELICITATION_APPROVAL_KIND_KEY;
@@ -90,6 +92,12 @@ impl Session {
config: &Config, config: &Config,
) -> (McpConfig, McpRuntimeContext) { ) -> (McpConfig, McpRuntimeContext) {
let originator = self.originator().await; let originator = self.originator().await;
let windows_sandbox_level = self
.state
.lock()
.await
.session_configuration
.windows_sandbox_level;
let environments = self.services.turn_environments.snapshot().await; let environments = self.services.turn_environments.snapshot().await;
let selected_capability_roots = self let selected_capability_roots = self
.resolve_selected_capability_roots_for_step(&environments) .resolve_selected_capability_roots_for_step(&environments)
@@ -97,7 +105,12 @@ impl Session {
let ready_selected_capability_roots = let ready_selected_capability_roots =
Self::ready_selected_capability_roots(&selected_capability_roots); Self::ready_selected_capability_roots(&selected_capability_roots);
let executor_capability_discovery = self let executor_capability_discovery = self
.executor_capability_discovery_for_step(config, &ready_selected_capability_roots) .executor_capability_discovery_for_step(
config,
&ready_selected_capability_roots,
&environments,
windows_sandbox_level,
)
.await; .await;
let mcp_config = self let mcp_config = self
.services .services
@@ -172,6 +185,8 @@ impl Session {
.executor_capability_discovery_for_step( .executor_capability_discovery_for_step(
&desired.config, &desired.config,
&ready_selected_capability_roots, &ready_selected_capability_roots,
&desired.environments,
desired.windows_sandbox_level,
) )
.await; .await;
let mcp_projection = self let mcp_projection = self
@@ -224,6 +239,8 @@ impl Session {
.executor_capability_discovery_for_step( .executor_capability_discovery_for_step(
&desired.config, &desired.config,
&ready_selected_capability_roots, &ready_selected_capability_roots,
&desired.environments,
desired.windows_sandbox_level,
) )
.await; .await;
let mcp_projection = self let mcp_projection = self
@@ -294,20 +311,67 @@ impl Session {
&self, &self,
config: &Config, config: &Config,
ready_selected_capability_roots: &[SelectedCapabilityRoot], ready_selected_capability_roots: &[SelectedCapabilityRoot],
environments: &TurnEnvironmentSnapshot,
windows_sandbox_level: WindowsSandboxLevel,
) -> Option<Arc<ExecutorCapabilityDiscoverySnapshot>> { ) -> Option<Arc<ExecutorCapabilityDiscoverySnapshot>> {
if !config let restricted_file_system = !config
.features .permissions
.enabled(Feature::ExecutorCapabilityDiscovery) .file_system_sandbox_policy()
.has_full_disk_read_access();
if !restricted_file_system
&& !config
.features
.enabled(Feature::ExecutorCapabilityDiscovery)
{ {
return None; return None;
} }
let sandbox_contexts = if restricted_file_system {
environments
.turn_environments()
.map(|environment| {
let mut sandbox = FileSystemSandboxContext::from_permission_profile_with_cwd(
config.permissions.permission_profile().clone(),
environment.cwd().clone(),
);
sandbox.workspace_roots = environment.workspace_roots().to_vec();
sandbox.windows_sandbox_level = windows_sandbox_level;
sandbox.windows_sandbox_private_desktop =
config.permissions.windows_sandbox_private_desktop;
sandbox.use_legacy_landlock = config.features.use_legacy_landlock();
(environment.environment_id.clone(), sandbox)
})
.collect::<HashMap<_, _>>()
} else {
HashMap::new()
};
let environment_manager = self.services.turn_environments.environment_manager(); let environment_manager = self.services.turn_environments.environment_manager();
let cache = self let cache = self
.services .services
.thread_extension_data .thread_extension_data
.get_or_init(|| ExecutorCapabilityDiscoveryCache::new(environment_manager)); .get_or_init(|| ExecutorCapabilityDiscoveryCache::new(environment_manager));
let selected_capability_roots = ready_selected_capability_roots
.iter()
.filter(|selected_root| {
if !restricted_file_system {
return true;
}
let CapabilityRootLocation::Environment { environment_id, .. } =
&selected_root.location;
if sandbox_contexts.contains_key(environment_id) {
return true;
}
warn!(
selected_root = selected_root.id,
environment_id, "skipping capability root without a filesystem sandbox context"
);
false
})
.cloned()
.collect::<Vec<_>>();
Some(Arc::new( Some(Arc::new(
cache.snapshot(ready_selected_capability_roots).await, cache
.snapshot(&selected_capability_roots, &sandbox_contexts)
.await,
)) ))
} }
@@ -535,10 +599,13 @@ impl Session {
.services .services
.mcp_runtime .mcp_runtime
.current_ready_selected_capability_roots(); .current_ready_selected_capability_roots();
let environments = self.services.turn_environments.snapshot().await;
let executor_capability_discovery = self let executor_capability_discovery = self
.executor_capability_discovery_for_step( .executor_capability_discovery_for_step(
refresh_config, refresh_config,
&ready_selected_capability_roots, &ready_selected_capability_roots,
&environments,
turn_context.windows_sandbox_level,
) )
.await; .await;
let mcp_projection = self let mcp_projection = self
+3
View File
@@ -16,6 +16,7 @@ pub(super) struct McpDesiredState {
pub(super) submit_id: String, pub(super) submit_id: String,
pub(super) originator: String, pub(super) originator: String,
pub(super) environments: TurnEnvironmentSnapshot, pub(super) environments: TurnEnvironmentSnapshot,
pub(super) windows_sandbox_level: WindowsSandboxLevel,
} }
impl McpDesiredState { impl McpDesiredState {
@@ -51,6 +52,7 @@ impl Session {
submit_id: self.next_internal_sub_id(), submit_id: self.next_internal_sub_id(),
originator: session_configuration.originator.clone(), originator: session_configuration.originator.clone(),
environments, environments,
windows_sandbox_level: session_configuration.windows_sandbox_level,
} }
} }
@@ -72,6 +74,7 @@ impl Session {
submit_id: INITIAL_SUBMIT_ID.to_owned(), submit_id: INITIAL_SUBMIT_ID.to_owned(),
originator: session_configuration.originator.clone(), originator: session_configuration.originator.clone(),
environments: resolved_environments.clone(), environments: resolved_environments.clone(),
windows_sandbox_level: session_configuration.windows_sandbox_level,
}; };
self.publish_mcp_runtime( self.publish_mcp_runtime(
&desired, &desired,
+8 -1
View File
@@ -3064,11 +3064,18 @@ impl Session {
.executor_capability_discovery_for_step( .executor_capability_discovery_for_step(
&turn_context.config, &turn_context.config,
&ready_selected_capability_roots, &ready_selected_capability_roots,
&environments,
turn_context.windows_sandbox_level,
) )
.await; .await;
let extension_data = codex_extension_api::ExtensionData::new(turn_context.sub_id.clone()); let extension_data = codex_extension_api::ExtensionData::new(turn_context.sub_id.clone());
extension_data.insert(selected_capability_roots.clone()); extension_data.insert(selected_capability_roots.clone());
if !turn_context if let Some(discovery) = &executor_capability_discovery {
extension_data.insert(discovery.as_ref().clone());
if !discovery.sandbox_contexts().is_empty() {
extension_data.insert(discovery.sandbox_contexts().clone());
}
} else if !turn_context
.config .config
.permissions .permissions
.file_system_sandbox_policy() .file_system_sandbox_policy()
@@ -105,6 +105,9 @@ pub struct EnvironmentCapabilities {
/// Whether `exec` accepts instructions for launching an executor-local network proxy. /// Whether `exec` accepts instructions for launching an executor-local network proxy.
#[serde(default)] #[serde(default)]
pub network_proxy_launch: bool, pub network_proxy_launch: bool,
/// Whether capability discovery applies the filesystem sandbox sent with each root.
#[serde(default)]
pub capability_discovery_sandbox: bool,
} }
/// Status returned by an initialized exec-server connection. /// Status returned by an initialized exec-server connection.
@@ -136,6 +139,7 @@ impl EnvironmentInfo {
.and_then(|cwd| PathUri::from_host_native_path(cwd).ok()), .and_then(|cwd| PathUri::from_host_native_path(cwd).ok()),
capabilities: EnvironmentCapabilities { capabilities: EnvironmentCapabilities {
network_proxy_launch: true, network_proxy_launch: true,
capability_discovery_sandbox: true,
}, },
} }
} }
@@ -475,6 +479,9 @@ pub struct CapabilityRootDiscoverRequest {
pub id: String, pub id: String,
/// Absolute root URI interpreted using the exec-server host's path rules. /// Absolute root URI interpreted using the exec-server host's path rules.
pub path: PathUri, pub path: PathUri,
/// Filesystem permissions for this root and its symlink targets.
#[serde(default)]
pub sandbox: Option<FileSystemSandboxContext>,
} }
/// Executor-local discovery results in request order. /// Executor-local discovery results in request order.
@@ -539,6 +546,7 @@ pub struct CapabilityRootDiscovery {
#[derive(Clone, Debug)] #[derive(Clone, Debug)]
pub struct ExecutorCapabilityDiscoverySnapshot { pub struct ExecutorCapabilityDiscoverySnapshot {
roots: Arc<[ExecutorCapabilityDiscoverySnapshotEntry]>, roots: Arc<[ExecutorCapabilityDiscoverySnapshotEntry]>,
sandbox_contexts: Arc<HashMap<String, FileSystemSandboxContext>>,
} }
#[derive(Clone, Debug)] #[derive(Clone, Debug)]
@@ -551,6 +559,7 @@ impl ExecutorCapabilityDiscoverySnapshot {
pub fn new( pub fn new(
selected_roots: &[SelectedCapabilityRoot], selected_roots: &[SelectedCapabilityRoot],
discoveries: Vec<Result<Arc<CapabilityRootDiscovery>, String>>, discoveries: Vec<Result<Arc<CapabilityRootDiscovery>, String>>,
sandbox_contexts: HashMap<String, FileSystemSandboxContext>,
) -> Self { ) -> Self {
debug_assert_eq!(selected_roots.len(), discoveries.len()); debug_assert_eq!(selected_roots.len(), discoveries.len());
Self { Self {
@@ -565,12 +574,17 @@ impl ExecutorCapabilityDiscoverySnapshot {
}, },
) )
.collect(), .collect(),
sandbox_contexts: Arc::new(sandbox_contexts),
} }
} }
pub fn roots(&self) -> &[ExecutorCapabilityDiscoverySnapshotEntry] { pub fn roots(&self) -> &[ExecutorCapabilityDiscoverySnapshotEntry] {
&self.roots &self.roots
} }
pub fn sandbox_contexts(&self) -> &HashMap<String, FileSystemSandboxContext> {
self.sandbox_contexts.as_ref()
}
} }
/// HTTP header represented in the executor protocol. /// HTTP header represented in the executor protocol.
@@ -10,6 +10,7 @@ use codex_exec_server_protocol::DISCOVERABLE_PLUGIN_MANIFEST_PATHS;
use codex_exec_server_protocol::DiscoveredPluginFiles; use codex_exec_server_protocol::DiscoveredPluginFiles;
use codex_exec_server_protocol::DiscoveredSkillFiles; use codex_exec_server_protocol::DiscoveredSkillFiles;
use codex_file_system::ExecutorFileSystem; use codex_file_system::ExecutorFileSystem;
use codex_file_system::FileSystemSandboxContext;
use codex_file_system::WalkEntryKind; use codex_file_system::WalkEntryKind;
use codex_file_system::WalkOptions; use codex_file_system::WalkOptions;
use codex_utils_path_uri::PathUri; use codex_utils_path_uri::PathUri;
@@ -17,7 +18,7 @@ use futures::StreamExt;
use serde::Deserialize; use serde::Deserialize;
use serde_json::Value; use serde_json::Value;
const MAX_ROOTS_PER_REQUEST: usize = 128; pub(crate) const MAX_ROOTS_PER_REQUEST: usize = 128;
const MAX_SCAN_DEPTH: usize = 6; const MAX_SCAN_DEPTH: usize = 6;
const MAX_DIRECTORIES_PER_ROOT: usize = 2_000; const MAX_DIRECTORIES_PER_ROOT: usize = 2_000;
const MAX_ENTRIES_PER_ROOT: usize = 20_000; const MAX_ENTRIES_PER_ROOT: usize = 20_000;
@@ -63,7 +64,8 @@ async fn discover_root(
file_system: &dyn ExecutorFileSystem, file_system: &dyn ExecutorFileSystem,
request: CapabilityRootDiscoverRequest, request: CapabilityRootDiscoverRequest,
) -> CapabilityRootDiscovery { ) -> CapabilityRootDiscovery {
let CapabilityRootDiscoverRequest { id, path } = request; let CapabilityRootDiscoverRequest { id, path, sandbox } = request;
let sandbox = sandbox.as_ref();
let mut discovery = CapabilityRootDiscovery { let mut discovery = CapabilityRootDiscovery {
id, id,
path: path.clone(), path: path.clone(),
@@ -74,7 +76,17 @@ async fn discover_root(
error: None, error: None,
}; };
match file_system.get_metadata(&path, /*sandbox*/ None).await { #[cfg(target_os = "windows")]
if sandbox.is_some_and(|context| {
context.should_run_in_sandbox()
&& context.windows_sandbox_level
== codex_protocol::config_types::WindowsSandboxLevel::Disabled
}) {
discovery.error = Some("filesystem sandbox is unavailable on this executor".to_string());
return discovery;
}
match file_system.get_metadata(&path, sandbox).await {
Ok(metadata) if metadata.is_directory => {} Ok(metadata) if metadata.is_directory => {}
Ok(_) => { Ok(_) => {
discovery.error = Some(format!("capability root {path} is not a directory")); discovery.error = Some(format!("capability root {path} is not a directory"));
@@ -96,7 +108,7 @@ async fn discover_root(
follow_directory_symlinks: true, follow_directory_symlinks: true,
prune_hidden_directories: false, prune_hidden_directories: false,
}, },
/*sandbox*/ None, sandbox,
) )
.await .await
{ {
@@ -143,14 +155,26 @@ async fn discover_root(
}); });
let mut budget = BundleBudget::default(); let mut budget = BundleBudget::default();
let root_manifest = let root_manifest = read_first_plugin_manifest(
read_first_plugin_manifest(file_system, &path, &mut budget, &mut discovery.warnings).await; file_system,
&path,
sandbox,
&mut budget,
&mut discovery.warnings,
)
.await;
let inherited_manifest = match root_manifest.as_ref() { let inherited_manifest = match root_manifest.as_ref() {
Some(manifest) => Some(manifest.clone()), Some(manifest) => Some(manifest.clone()),
None => { None => {
read_nearest_ancestor_manifest(file_system, &path, &mut budget, &mut discovery.warnings) read_nearest_ancestor_manifest(
.await file_system,
&path,
sandbox,
&mut budget,
&mut discovery.warnings,
)
.await
} }
}; };
let mut seen_namespace_roots = HashSet::new(); let mut seen_namespace_roots = HashSet::new();
@@ -170,6 +194,7 @@ async fn discover_root(
if let Some(manifest) = read_optional_text_file( if let Some(manifest) = read_optional_text_file(
file_system, file_system,
manifest_path, manifest_path,
sandbox,
&mut budget, &mut budget,
&mut discovery.warnings, &mut discovery.warnings,
) )
@@ -190,15 +215,27 @@ async fn discover_root(
}; };
let mcp_config = match mcp_path { let mcp_config = match mcp_path {
Some(path) => { Some(path) => {
read_optional_text_file(file_system, path, &mut budget, &mut discovery.warnings) read_optional_text_file(
.await file_system,
path,
sandbox,
&mut budget,
&mut discovery.warnings,
)
.await
} }
None => None, None => None,
}; };
let apps_config = match declarations.apps_config { let apps_config = match declarations.apps_config {
Some(path) => { Some(path) => {
read_optional_text_file(file_system, path, &mut budget, &mut discovery.warnings) read_optional_text_file(
.await file_system,
path,
sandbox,
&mut budget,
&mut discovery.warnings,
)
.await
} }
None => None, None => None,
}; };
@@ -213,6 +250,7 @@ async fn discover_root(
let Some(instructions) = read_optional_text_file( let Some(instructions) = read_optional_text_file(
file_system, file_system,
skill_path.clone(), skill_path.clone(),
sandbox,
&mut budget, &mut budget,
&mut discovery.warnings, &mut discovery.warnings,
) )
@@ -228,6 +266,7 @@ async fn discover_root(
read_optional_text_file( read_optional_text_file(
file_system, file_system,
metadata_path, metadata_path,
sandbox,
&mut budget, &mut budget,
&mut discovery.warnings, &mut discovery.warnings,
) )
@@ -247,6 +286,7 @@ async fn discover_root(
async fn read_first_plugin_manifest( async fn read_first_plugin_manifest(
file_system: &dyn ExecutorFileSystem, file_system: &dyn ExecutorFileSystem,
root: &PathUri, root: &PathUri,
sandbox: Option<&FileSystemSandboxContext>,
budget: &mut BundleBudget, budget: &mut BundleBudget,
warnings: &mut Vec<String>, warnings: &mut Vec<String>,
) -> Option<CapabilityTextFile> { ) -> Option<CapabilityTextFile> {
@@ -254,7 +294,9 @@ async fn read_first_plugin_manifest(
let Ok(path) = root.join(relative_path) else { let Ok(path) = root.join(relative_path) else {
continue; continue;
}; };
if let Some(manifest) = read_optional_text_file(file_system, path, budget, warnings).await { if let Some(manifest) =
read_optional_text_file(file_system, path, sandbox, budget, warnings).await
{
return Some(manifest); return Some(manifest);
} }
} }
@@ -264,13 +306,14 @@ async fn read_first_plugin_manifest(
async fn read_nearest_ancestor_manifest( async fn read_nearest_ancestor_manifest(
file_system: &dyn ExecutorFileSystem, file_system: &dyn ExecutorFileSystem,
root: &PathUri, root: &PathUri,
sandbox: Option<&FileSystemSandboxContext>,
budget: &mut BundleBudget, budget: &mut BundleBudget,
warnings: &mut Vec<String>, warnings: &mut Vec<String>,
) -> Option<CapabilityTextFile> { ) -> Option<CapabilityTextFile> {
let mut ancestor = root.parent(); let mut ancestor = root.parent();
while let Some(path) = ancestor { while let Some(path) = ancestor {
if let Some(manifest) = if let Some(manifest) =
read_first_plugin_manifest(file_system, &path, budget, warnings).await read_first_plugin_manifest(file_system, &path, sandbox, budget, warnings).await
{ {
return Some(manifest); return Some(manifest);
} }
@@ -282,10 +325,11 @@ async fn read_nearest_ancestor_manifest(
async fn read_optional_text_file( async fn read_optional_text_file(
file_system: &dyn ExecutorFileSystem, file_system: &dyn ExecutorFileSystem,
path: PathUri, path: PathUri,
sandbox: Option<&FileSystemSandboxContext>,
budget: &mut BundleBudget, budget: &mut BundleBudget,
warnings: &mut Vec<String>, warnings: &mut Vec<String>,
) -> Option<CapabilityTextFile> { ) -> Option<CapabilityTextFile> {
let metadata = match file_system.get_metadata(&path, /*sandbox*/ None).await { let metadata = match file_system.get_metadata(&path, sandbox).await {
Ok(metadata) if metadata.is_file => metadata, Ok(metadata) if metadata.is_file => metadata,
Ok(_) => return None, Ok(_) => return None,
Err(error) if error.kind() == io::ErrorKind::NotFound => return None, Err(error) if error.kind() == io::ErrorKind::NotFound => return None,
@@ -310,32 +354,49 @@ async fn read_optional_text_file(
)); ));
return None; return None;
} }
let mut stream = match file_system.read_file_stream(&path, /*sandbox*/ None).await { let contents = if sandbox.is_some_and(FileSystemSandboxContext::should_run_in_sandbox) {
Ok(stream) => stream, match file_system.read_file(&path, sandbox).await {
Err(error) => { Ok(contents) if contents.len() <= MAX_FILE_BYTES && budget.can_add(contents.len()) => {
warnings.push(format!("failed to read capability file {path}: {error}")); contents
return None; }
Ok(_) => {
warnings.push(format!("capability file {path} exceeded its read limit"));
return None;
}
Err(error) => {
warnings.push(format!("failed to read capability file {path}: {error}"));
return None;
}
} }
}; } else {
let mut contents = Vec::with_capacity(size); let mut stream = match file_system.read_file_stream(&path, sandbox).await {
while let Some(chunk) = stream.next().await { Ok(stream) => stream,
let chunk = match chunk {
Ok(chunk) => chunk,
Err(error) => { Err(error) => {
warnings.push(format!("failed to read capability file {path}: {error}")); warnings.push(format!("failed to read capability file {path}: {error}"));
return None; return None;
} }
}; };
let Some(new_len) = contents.len().checked_add(chunk.len()) else { let mut contents = Vec::with_capacity(size);
warnings.push(format!("capability file {path} exceeded its read limit")); while let Some(chunk) = stream.next().await {
return None; let chunk = match chunk {
}; Ok(chunk) => chunk,
if new_len > MAX_FILE_BYTES || !budget.can_add(new_len) { Err(error) => {
warnings.push(format!("capability file {path} exceeded its read limit")); warnings.push(format!("failed to read capability file {path}: {error}"));
return None; return None;
}
};
let Some(new_len) = contents.len().checked_add(chunk.len()) else {
warnings.push(format!("capability file {path} exceeded its read limit"));
return None;
};
if new_len > MAX_FILE_BYTES || !budget.can_add(new_len) {
warnings.push(format!("capability file {path} exceeded its read limit"));
return None;
}
contents.extend_from_slice(&chunk);
} }
contents.extend_from_slice(&chunk); contents
} };
let contents = match String::from_utf8(contents) { let contents = match String::from_utf8(contents) {
Ok(contents) => contents, Ok(contents) => contents,
Err(error) => { Err(error) => {
@@ -1,4 +1,5 @@
use std::collections::BTreeMap; use std::collections::BTreeMap;
use std::collections::HashMap;
use std::sync::Arc; use std::sync::Arc;
use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::CapabilityRootLocation;
@@ -10,6 +11,7 @@ use crate::CapabilityRootDiscovery;
use crate::CapabilityRootsDiscoverParams; use crate::CapabilityRootsDiscoverParams;
use crate::EnvironmentManager; use crate::EnvironmentManager;
use crate::ExecutorCapabilityDiscoverySnapshot; use crate::ExecutorCapabilityDiscoverySnapshot;
use crate::FileSystemSandboxContext;
/// Thread-scoped cache shared by capability consumers using the high-level executor API. /// Thread-scoped cache shared by capability consumers using the high-level executor API.
/// ///
@@ -31,6 +33,7 @@ impl std::fmt::Debug for ExecutorCapabilityDiscoveryCache {
struct CachedRoot { struct CachedRoot {
selected_root: SelectedCapabilityRoot, selected_root: SelectedCapabilityRoot,
sandbox: Option<FileSystemSandboxContext>,
// Both successes and failures are memoized for the thread. Retrying a transient failure for // Both successes and failures are memoized for the thread. Retrying a transient failure for
// the same stable selected root requires explicit invalidation or a new thread. // the same stable selected root requires explicit invalidation or a new thread.
result: Result<Arc<CapabilityRootDiscovery>, String>, result: Result<Arc<CapabilityRootDiscovery>, String>,
@@ -53,36 +56,47 @@ impl ExecutorCapabilityDiscoveryCache {
pub async fn discover( pub async fn discover(
&self, &self,
selected_roots: &[SelectedCapabilityRoot], selected_roots: &[SelectedCapabilityRoot],
sandbox_contexts: &HashMap<String, FileSystemSandboxContext>,
) -> Vec<Result<Arc<CapabilityRootDiscovery>, String>> { ) -> Vec<Result<Arc<CapabilityRootDiscovery>, String>> {
let missing = { let missing = {
let entries = self.entries.lock().await; let entries = self.entries.lock().await;
selected_roots selected_roots
.iter() .iter()
.filter(|selected_root| { .filter(|selected_root| {
!entries let CapabilityRootLocation::Environment { environment_id, .. } =
.iter() &selected_root.location;
.any(|cached| cached.selected_root == **selected_root) let sandbox = sandbox_contexts.get(environment_id);
!entries.iter().any(|cached| {
cached.selected_root == **selected_root
&& cached.sandbox.as_ref() == sandbox
})
}) })
.cloned() .cloned()
.collect::<Vec<_>>() .collect::<Vec<_>>()
}; };
let discovered = self.discover_missing(missing).await; let discovered = self.discover_missing(missing, sandbox_contexts).await;
let mut entries = self.entries.lock().await; let mut entries = self.entries.lock().await;
for discovered_root in discovered { for discovered_root in discovered {
if !entries if let Some(cached) = entries
.iter() .iter_mut()
.any(|cached| cached.selected_root == discovered_root.selected_root) .find(|cached| cached.selected_root == discovered_root.selected_root)
{ {
if cached.sandbox != discovered_root.sandbox {
*cached = discovered_root;
}
} else {
entries.push(discovered_root); entries.push(discovered_root);
} }
} }
selected_roots selected_roots
.iter() .iter()
.map(|selected_root| { .map(|selected_root| {
match entries let CapabilityRootLocation::Environment { environment_id, .. } =
.iter() &selected_root.location;
.find(|cached| cached.selected_root == *selected_root) let sandbox = sandbox_contexts.get(environment_id);
{ match entries.iter().find(|cached| {
cached.selected_root == *selected_root && cached.sandbox.as_ref() == sandbox
}) {
Some(cached) => cached.result.clone(), Some(cached) => cached.result.clone(),
None => Err(format!( None => Err(format!(
"selected capability root `{}` was not discovered", "selected capability root `{}` was not discovered",
@@ -97,14 +111,20 @@ impl ExecutorCapabilityDiscoveryCache {
pub async fn snapshot( pub async fn snapshot(
&self, &self,
selected_roots: &[SelectedCapabilityRoot], selected_roots: &[SelectedCapabilityRoot],
sandbox_contexts: &HashMap<String, FileSystemSandboxContext>,
) -> ExecutorCapabilityDiscoverySnapshot { ) -> ExecutorCapabilityDiscoverySnapshot {
ExecutorCapabilityDiscoverySnapshot::new( ExecutorCapabilityDiscoverySnapshot::new(
selected_roots, selected_roots,
self.discover(selected_roots).await, self.discover(selected_roots, sandbox_contexts).await,
sandbox_contexts.clone(),
) )
} }
async fn discover_missing(&self, missing: Vec<SelectedCapabilityRoot>) -> Vec<CachedRoot> { async fn discover_missing(
&self,
missing: Vec<SelectedCapabilityRoot>,
sandbox_contexts: &HashMap<String, FileSystemSandboxContext>,
) -> Vec<CachedRoot> {
let mut grouped = BTreeMap::<String, Vec<SelectedCapabilityRoot>>::new(); let mut grouped = BTreeMap::<String, Vec<SelectedCapabilityRoot>>::new();
for selected_root in missing { for selected_root in missing {
let CapabilityRootLocation::Environment { environment_id, .. } = let CapabilityRootLocation::Environment { environment_id, .. } =
@@ -115,8 +135,15 @@ impl ExecutorCapabilityDiscoveryCache {
.push(selected_root); .push(selected_root);
} }
let discoveries = futures::future::join_all(grouped.into_iter().map( let batches = grouped.into_iter().flat_map(|(environment_id, roots)| {
|(environment_id, selected_roots)| async move { roots
.chunks(crate::capability_discovery::MAX_ROOTS_PER_REQUEST)
.map(|batch| (environment_id.clone(), batch.to_vec()))
.collect::<Vec<_>>()
});
let discoveries =
futures::future::join_all(batches.map(|(environment_id, selected_roots)| async move {
let sandbox = sandbox_contexts.get(&environment_id).cloned();
let Some(environment) = self.environment_manager.get_environment(&environment_id) let Some(environment) = self.environment_manager.get_environment(&environment_id)
else { else {
let error = format!("environment `{environment_id}` is unavailable"); let error = format!("environment `{environment_id}` is unavailable");
@@ -124,6 +151,7 @@ impl ExecutorCapabilityDiscoveryCache {
.into_iter() .into_iter()
.map(|selected_root| CachedRoot { .map(|selected_root| CachedRoot {
selected_root, selected_root,
sandbox: sandbox.clone(),
result: Err(error.clone()), result: Err(error.clone()),
}) })
.collect::<Vec<_>>(); .collect::<Vec<_>>();
@@ -137,6 +165,7 @@ impl ExecutorCapabilityDiscoveryCache {
CapabilityRootDiscoverRequest { CapabilityRootDiscoverRequest {
id: selected_root.id.clone(), id: selected_root.id.clone(),
path: path.clone(), path: path.clone(),
sandbox: sandbox.clone(),
} }
}) })
.collect(), .collect(),
@@ -149,6 +178,7 @@ impl ExecutorCapabilityDiscoveryCache {
.into_iter() .into_iter()
.map(|selected_root| CachedRoot { .map(|selected_root| CachedRoot {
selected_root, selected_root,
sandbox: sandbox.clone(),
result: Err(error.clone()), result: Err(error.clone()),
}) })
.collect(); .collect();
@@ -164,6 +194,7 @@ impl ExecutorCapabilityDiscoveryCache {
.into_iter() .into_iter()
.map(|selected_root| CachedRoot { .map(|selected_root| CachedRoot {
selected_root, selected_root,
sandbox: sandbox.clone(),
result: Err(error.clone()), result: Err(error.clone()),
}) })
.collect(); .collect();
@@ -185,13 +216,13 @@ impl ExecutorCapabilityDiscoveryCache {
}; };
CachedRoot { CachedRoot {
selected_root, selected_root,
sandbox: sandbox.clone(),
result, result,
} }
}) })
.collect() .collect()
}, }))
)) .await;
.await;
discoveries.into_iter().flatten().collect() discoveries.into_iter().flatten().collect()
} }
} }
+18 -1
View File
@@ -814,7 +814,24 @@ impl Environment {
params: CapabilityRootsDiscoverParams, params: CapabilityRootsDiscoverParams,
) -> Result<CapabilityRootsDiscoverResponse, ExecServerError> { ) -> Result<CapabilityRootsDiscoverResponse, ExecServerError> {
match &self.remote_client { match &self.remote_client {
Some(client) => client.get().await?.discover_capability_roots(params).await, Some(client) => {
let client = client.get().await?;
if params.roots.iter().any(|root| {
root.sandbox
.as_ref()
.is_some_and(crate::FileSystemSandboxContext::should_run_in_sandbox)
}) && !client
.environment_info()
.await?
.capabilities
.capability_discovery_sandbox
{
return Err(ExecServerError::Protocol(
"exec-server does not support sandboxed capability discovery".to_string(),
));
}
client.discover_capability_roots(params).await
}
None => crate::discover_capability_roots(self.filesystem.as_ref(), params) None => crate::discover_capability_roots(self.filesystem.as_ref(), params)
.await .await
.map_err(|error| ExecServerError::Protocol(error.to_string())), .map_err(|error| ExecServerError::Protocol(error.to_string())),
@@ -4,11 +4,26 @@ use codex_exec_server::CAPABILITY_ROOTS_DISCOVER_METHOD;
use codex_exec_server::CapabilityRootDiscovery; use codex_exec_server::CapabilityRootDiscovery;
use codex_exec_server::CapabilityRootsDiscoverParams; use codex_exec_server::CapabilityRootsDiscoverParams;
use codex_exec_server::CapabilityRootsDiscoverResponse; use codex_exec_server::CapabilityRootsDiscoverResponse;
use codex_exec_server::FileSystemSandboxContext;
use codex_exec_server::InitializeParams; use codex_exec_server::InitializeParams;
use codex_exec_server::InitializeResponse; use codex_exec_server::InitializeResponse;
use codex_exec_server_protocol::CapabilityRootDiscoverRequest; use codex_exec_server_protocol::CapabilityRootDiscoverRequest;
use codex_exec_server_protocol::JSONRPCMessage; use codex_exec_server_protocol::JSONRPCMessage;
use codex_exec_server_protocol::JSONRPCResponse; use codex_exec_server_protocol::JSONRPCResponse;
#[cfg(unix)]
use codex_protocol::models::PermissionProfile;
#[cfg(unix)]
use codex_protocol::permissions::FileSystemAccessMode;
#[cfg(unix)]
use codex_protocol::permissions::FileSystemPath;
#[cfg(unix)]
use codex_protocol::permissions::FileSystemSandboxEntry;
#[cfg(unix)]
use codex_protocol::permissions::FileSystemSandboxPolicy;
#[cfg(unix)]
use codex_protocol::permissions::NetworkSandboxPolicy;
#[cfg(unix)]
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri; use codex_utils_path_uri::PathUri;
use common::exec_server::exec_server; use common::exec_server::exec_server;
use pretty_assertions::assert_eq; use pretty_assertions::assert_eq;
@@ -166,10 +181,103 @@ async fn discovers_cursor_plugin_without_reading_default_mcp_for_inline_servers(
Ok(()) Ok(())
} }
#[cfg(unix)]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn sandboxed_discovery_follows_only_permitted_external_symlinks() -> anyhow::Result<()> {
let root = tempfile::tempdir()?;
let external = tempfile::tempdir()?;
write_file(
&root.path().join(".codex-plugin/plugin.json"),
r#"{"name":"linked-plugin","mcpServers":"./external-mcp.json"}"#,
)?;
write_file(
&external.path().join("mcp.json"),
r#"{"mcpServers":{"linked":{"command":"linked-server"}}}"#,
)?;
write_file(
&external.path().join("skill/SKILL.md"),
"---\nname: linked\ndescription: Linked external skill.\n---\n",
)?;
std::fs::create_dir_all(root.path().join("skills"))?;
std::os::unix::fs::symlink(
external.path().join("skill"),
root.path().join("skills/linked"),
)?;
std::os::unix::fs::symlink(
external.path().join("mcp.json"),
root.path().join("external-mcp.json"),
)?;
let mut server = exec_server().await?;
initialize(&mut server).await?;
let root_uri = PathUri::from_host_native_path(root.path())?;
let root_path = AbsolutePathBuf::from_absolute_path(root.path())?;
let external_root = AbsolutePathBuf::from_absolute_path(external.path())?;
let path_entry =
|path, access| FileSystemSandboxEntry::new(FileSystemPath::Path { path }, access);
let read_root = path_entry(root_path, FileSystemAccessMode::Read);
let read_external = path_entry(external_root.clone(), FileSystemAccessMode::Read);
let deny_external_skill = path_entry(external_root.join("skill"), FileSystemAccessMode::Deny);
let cases = [
(
"permitted symlinks",
vec![read_root.clone(), read_external.clone()],
true,
true,
),
(
"denied external root",
vec![read_root.clone()],
false,
false,
),
(
"denied external skill",
vec![read_root, read_external, deny_external_skill],
false,
true,
),
];
for (scenario, entries, has_skill, has_mcp) in cases {
let policy = FileSystemSandboxPolicy::restricted(entries);
let sandbox = FileSystemSandboxContext::from_permission_profile_with_cwd(
PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted),
root_uri.clone(),
);
let discovery =
discover_root_with_sandbox(&mut server, "linked", root_uri.clone(), Some(sandbox))
.await?;
assert_eq!(discovery.error, None, "{scenario}");
assert_eq!(discovery.skills.len(), usize::from(has_skill), "{scenario}");
assert_eq!(
discovery
.plugin
.and_then(|plugin| plugin.mcp_config)
.is_some_and(|config| config.contents.contains("linked-server")),
has_mcp,
"{scenario}"
);
}
server.shutdown().await?;
Ok(())
}
async fn discover_root( async fn discover_root(
server: &mut common::exec_server::ExecServerHarness, server: &mut common::exec_server::ExecServerHarness,
id: &str, id: &str,
path: PathUri, path: PathUri,
) -> anyhow::Result<CapabilityRootDiscovery> {
discover_root_with_sandbox(server, id, path, /*sandbox*/ None).await
}
async fn discover_root_with_sandbox(
server: &mut common::exec_server::ExecServerHarness,
id: &str,
path: PathUri,
sandbox: Option<FileSystemSandboxContext>,
) -> anyhow::Result<CapabilityRootDiscovery> { ) -> anyhow::Result<CapabilityRootDiscovery> {
let request_id = server let request_id = server
.send_request( .send_request(
@@ -178,6 +286,7 @@ async fn discover_root(
roots: vec![CapabilityRootDiscoverRequest { roots: vec![CapabilityRootDiscoverRequest {
id: id.to_string(), id: id.to_string(),
path, path,
sandbox,
}], }],
})?, })?,
) )
@@ -230,7 +230,7 @@ async fn raw_selected_plugin_contributions(
{ {
Some( Some(
ExecutorCapabilityDiscoveryCache::new(environment_manager) ExecutorCapabilityDiscoveryCache::new(environment_manager)
.snapshot(&selected_capability_roots) .snapshot(&selected_capability_roots, &Default::default())
.await, .await,
) )
} else { } else {
+4 -1
View File
@@ -4,6 +4,7 @@ use std::sync::Arc;
use codex_core_skills::HostSkillsSnapshot; use codex_core_skills::HostSkillsSnapshot;
use codex_core_skills::injection::HostSkillsCatalogInWorldState; use codex_core_skills::injection::HostSkillsCatalogInWorldState;
use codex_core_skills::injection::InjectedHostSkillPrompts; use codex_core_skills::injection::InjectedHostSkillPrompts;
use codex_exec_server::ExecutorCapabilityDiscoverySnapshot;
use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::FileSystemSandboxContext;
use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_exec_server::LOCAL_ENVIRONMENT_ID;
use codex_exec_server::ResolvedSelectedCapabilityRoot; use codex_exec_server::ResolvedSelectedCapabilityRoot;
@@ -457,7 +458,9 @@ where
include_bundled_skills: false, include_bundled_skills: false,
include_orchestrator_skills: false, include_orchestrator_skills: false,
mcp_resources: None, mcp_resources: None,
executor_capability_discovery: None, executor_capability_discovery: step_store
.get::<ExecutorCapabilityDiscoverySnapshot>()
.map(|discovery| discovery.as_ref().clone()),
}); });
self.build_skill_tools( self.build_skill_tools(
session_store, session_store,
+15 -2
View File
@@ -4,6 +4,7 @@ use std::future::Future;
use std::sync::Arc; use std::sync::Arc;
use std::sync::Mutex; use std::sync::Mutex;
use codex_exec_server::FileSystemSandboxContext;
use codex_extension_api::ExtensionMetrics; use codex_extension_api::ExtensionMetrics;
use codex_mcp::McpResourceClient; use codex_mcp::McpResourceClient;
use codex_mcp::McpResourceClientCacheKey; use codex_mcp::McpResourceClientCacheKey;
@@ -138,12 +139,19 @@ impl SkillsThreadState {
providers: &SkillProviders, providers: &SkillProviders,
query: SkillListQuery, query: SkillListQuery,
) -> SkillCatalog { ) -> SkillCatalog {
let sandbox_contexts = query
.executor_capability_discovery
.as_ref()
.map(|discovery| discovery.sandbox_contexts().clone())
.unwrap_or_default();
if let Some(cached) = self if let Some(cached) = self
.executor_discovery_cache .executor_discovery_cache
.lock() .lock()
.unwrap_or_else(std::sync::PoisonError::into_inner) .unwrap_or_else(std::sync::PoisonError::into_inner)
.as_ref() .as_ref()
.filter(|cached| cached.roots == query.executor_roots) .filter(|cached| {
cached.roots == query.executor_roots && cached.sandbox_contexts == sandbox_contexts
})
{ {
return cached.catalog.clone(); return cached.catalog.clone();
} }
@@ -153,11 +161,15 @@ impl SkillsThreadState {
.executor_discovery_cache .executor_discovery_cache
.lock() .lock()
.unwrap_or_else(std::sync::PoisonError::into_inner); .unwrap_or_else(std::sync::PoisonError::into_inner);
if let Some(cached) = cache.as_ref().filter(|cached| cached.roots == roots) { if let Some(cached) = cache
.as_ref()
.filter(|cached| cached.roots == roots && cached.sandbox_contexts == sandbox_contexts)
{
return cached.catalog.clone(); return cached.catalog.clone();
} }
*cache = Some(CachedExecutorDiscoveryCatalog { *cache = Some(CachedExecutorDiscoveryCatalog {
roots, roots,
sandbox_contexts,
catalog: discovered.clone(), catalog: discovered.clone(),
}); });
discovered discovered
@@ -282,6 +294,7 @@ struct CachedExecutorCatalog {
struct CachedExecutorDiscoveryCatalog { struct CachedExecutorDiscoveryCatalog {
roots: Vec<SelectedCapabilityRoot>, roots: Vec<SelectedCapabilityRoot>,
sandbox_contexts: HashMap<String, FileSystemSandboxContext>,
catalog: SkillCatalog, catalog: SkillCatalog,
} }
@@ -1,3 +1,4 @@
use std::collections::HashMap;
use std::io; use std::io;
use std::sync::Arc; use std::sync::Arc;
use std::sync::atomic::AtomicUsize; use std::sync::atomic::AtomicUsize;
@@ -365,7 +366,7 @@ async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() {
}, },
}]; }];
let executor_capability_discovery = ExecutorCapabilityDiscoveryCache::new(manager) let executor_capability_discovery = ExecutorCapabilityDiscoveryCache::new(manager)
.snapshot(&executor_roots) .snapshot(&executor_roots, &Default::default())
.await; .await;
let catalog = provider let catalog = provider
.list(SkillListQuery { .list(SkillListQuery {
@@ -403,6 +404,81 @@ async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() {
assert_eq!(read.contents, SKILL_CONTENTS); assert_eq!(read.contents, SKILL_CONTENTS);
} }
#[tokio::test]
async fn high_level_discovery_cache_separates_filesystem_permission_contexts() {
let test_root = create_local_skill_root("permission-context").expect("create local skill root");
let manager = Arc::new(EnvironmentManager::default_for_tests());
let cache = ExecutorCapabilityDiscoveryCache::new(manager);
let executor_roots = vec![SelectedCapabilityRoot {
id: "permission-root".to_string(),
location: CapabilityRootLocation::Environment {
environment_id: "local".to_string(),
path: PathUri::from_host_native_path(&test_root).expect("skill root URI"),
},
}];
cache.snapshot(&executor_roots, &HashMap::new()).await;
let updated_contents =
"---\nname: synthetic\ndescription: Updated executor skill.\n---\n\nUPDATED_BODY\n";
std::fs::write(test_root.join("skill/SKILL.md"), updated_contents)
.expect("update executor skill");
let sandbox_contexts = HashMap::from([(
"local".to_string(),
FileSystemSandboxContext::from_permission_profile(PermissionProfile::Disabled),
)]);
let second_snapshot = cache.snapshot(&executor_roots, &sandbox_contexts).await;
let second_discovery = second_snapshot.roots()[0]
.result
.as_ref()
.expect("second discovery");
assert_eq!(
second_discovery.skills[0].instructions.contents,
updated_contents
);
assert_eq!(second_snapshot.sandbox_contexts(), &sandbox_contexts);
let restored_contents =
"---\nname: synthetic\ndescription: Restored executor skill.\n---\n\nRESTORED_BODY\n";
std::fs::write(test_root.join("skill/SKILL.md"), restored_contents)
.expect("restore executor skill");
let restored_snapshot = cache.snapshot(&executor_roots, &HashMap::new()).await;
let restored_discovery = restored_snapshot.roots()[0]
.result
.as_ref()
.expect("restored discovery");
assert_eq!(
restored_discovery.skills[0].instructions.contents,
restored_contents
);
std::fs::remove_dir_all(test_root).expect("remove skill directory");
}
#[tokio::test]
async fn high_level_discovery_batches_more_than_128_roots() {
let test_root = create_local_skill_root("many-roots").expect("create local skill root");
let manager = Arc::new(EnvironmentManager::default_for_tests());
let cache = ExecutorCapabilityDiscoveryCache::new(manager);
let executor_roots = (0..129)
.map(|index| SelectedCapabilityRoot {
id: format!("root-{index}"),
location: CapabilityRootLocation::Environment {
environment_id: "local".to_string(),
path: PathUri::from_host_native_path(&test_root).expect("skill root URI"),
},
})
.collect::<Vec<_>>();
let snapshot = cache.snapshot(&executor_roots, &HashMap::new()).await;
assert_eq!(snapshot.roots().len(), executor_roots.len());
assert!(snapshot.roots().iter().all(|root| root.result.is_ok()));
std::fs::remove_dir_all(test_root).expect("remove skill directory");
}
fn create_local_skill_root(label: &str) -> io::Result<std::path::PathBuf> { fn create_local_skill_root(label: &str) -> io::Result<std::path::PathBuf> {
let id = NEXT_TEST_ROOT_ID.fetch_add(1, Ordering::Relaxed); let id = NEXT_TEST_ROOT_ID.fetch_add(1, Ordering::Relaxed);
let test_root = std::env::temp_dir().join(format!( let test_root = std::env::temp_dir().join(format!(