diff --git a/codex-rs/core-plugins/src/executor_provider.rs b/codex-rs/core-plugins/src/executor_provider.rs index 20399d8ac9..78a66b1ad6 100644 --- a/codex-rs/core-plugins/src/executor_provider.rs +++ b/codex-rs/core-plugins/src/executor_provider.rs @@ -3,8 +3,10 @@ use crate::PluginProvider; use crate::manifest::parse_plugin_manifest_uri; +use codex_exec_server::CapabilityRootDiscovery; use codex_exec_server::EnvironmentManager; use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::GetMetadataOptions; use codex_exec_server::ReadFileOptions; use codex_plugin::ResolvedPlugin; @@ -14,13 +16,18 @@ use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_utils_path_uri::PathUri; use codex_utils_path_uri::PathUriParseError; use codex_utils_plugins::DISCOVERABLE_PLUGIN_MANIFEST_PATHS; +use futures::StreamExt; use std::io; use std::sync::Arc; use thiserror::Error; +const MAX_CONCURRENT_MANIFEST_PROBES: usize = 256; + /// Failure to resolve an environment-owned capability root as a plugin package. #[derive(Debug, Error)] pub enum ExecutorPluginProviderError { + #[error("discovery failed for selected capability root `{root_id}`")] + DiscoveryFailed { root_id: String }, #[error( "selected capability root `{root_id}` references unavailable environment `{environment_id}`" )] @@ -82,6 +89,13 @@ pub struct ExecutorPluginProvider { environment_manager: Arc, } +/// Ownership of a selected root, independently of whether its manifest can be parsed. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum PluginRootOwnership { + Plugin, + Standalone, +} + /// A resolved plugin paired with the concrete filesystem used to read it. #[derive(Clone)] pub struct ResolvedExecutorPlugin { @@ -109,6 +123,61 @@ impl ExecutorPluginProvider { } } + /// Classifies ownership before capability parsing. Failed discovery or metadata + /// access returns an error, not evidence that a root is standalone. + pub async fn root_ownership( + &self, + selected_root: &SelectedCapabilityRoot, + discovery: Option<&CapabilityRootDiscovery>, + sandbox: Option<&FileSystemSandboxContext>, + ) -> Result { + let CapabilityRootLocation::Environment { + environment_id, + path, + } = &selected_root.location; + if let Some(discovery) = discovery { + if discovery.error.is_some() { + return Err(ExecutorPluginProviderError::DiscoveryFailed { + root_id: selected_root.id.clone(), + }); + } + if discovery.plugin.is_some() + || discovery.namespace_manifests.iter().any(|manifest| { + manifest + .path + .parent() + .and_then(|directory| directory.parent()) + .is_some_and(|plugin_root| path.starts_with(&plugin_root)) + }) + { + return Ok(PluginRootOwnership::Plugin); + } + if discovery.warnings.is_empty() { + return Ok(PluginRootOwnership::Standalone); + } + } + let environment = self + .environment_manager + .get_environment(environment_id) + .ok_or_else(|| ExecutorPluginProviderError::UnavailableEnvironment { + root_id: selected_root.id.clone(), + environment_id: environment_id.clone(), + })?; + let file_system = environment.get_filesystem(); + let manifest = find_manifest( + selected_root, + std::iter::successors(Some(path.clone()), PathUri::parent), + file_system.as_ref(), + sandbox, + ) + .await?; + Ok(if manifest.is_some() { + PluginRootOwnership::Plugin + } else { + PluginRootOwnership::Standalone + }) + } + /// Resolves a plugin and retains the exact filesystem used for package access. #[tracing::instrument(name = "plugins.executor.package.resolve", skip_all)] pub async fn resolve_bound( @@ -168,40 +237,14 @@ async fn resolve_plugin_root( }); } - let mut manifest_path = None; - for relative_path in DISCOVERABLE_PLUGIN_MANIFEST_PATHS { - let candidate_uri = plugin_root.join(relative_path).map_err(|source| { - ExecutorPluginProviderError::InvalidManifestPath { - root_id: root_id.clone(), - root: plugin_root.clone(), - relative_path, - source, - } - })?; - match file_system - .get_metadata( - &candidate_uri, - GetMetadataOptions::default(), - /*sandbox*/ None, - ) - .await - { - Ok(metadata) if metadata.is_file => { - manifest_path = Some(candidate_uri); - break; - } - Ok(_) => {} - Err(err) if err.kind() == io::ErrorKind::NotFound => {} - Err(source) => { - return Err(ExecutorPluginProviderError::InspectManifest { - root_id: root_id.clone(), - path: candidate_uri, - source, - }); - } - } - } - let Some(manifest_uri) = manifest_path else { + let Some(manifest_uri) = find_manifest( + selected_root, + std::iter::once(plugin_root.clone()), + file_system, + /*sandbox*/ None, + ) + .await? + else { return Ok(None); }; let contents = file_system @@ -240,6 +283,60 @@ async fn resolve_plugin_root( Ok(Some(plugin)) } +async fn find_manifest( + selected_root: &SelectedCapabilityRoot, + mut plugin_roots: impl Iterator + Send, + file_system: &dyn ExecutorFileSystem, + sandbox: Option<&FileSystemSandboxContext>, +) -> Result, ExecutorPluginProviderError> { + let root_id = &selected_root.id; + let mut plugin_root = plugin_roots.next(); + let mut marker_index = 0; + let probes = std::iter::from_fn(move || { + let root = plugin_root.clone()?; + let relative_path = *DISCOVERABLE_PLUGIN_MANIFEST_PATHS.get(marker_index)?; + let candidate_uri = root.join(relative_path).map_err(|source| { + ExecutorPluginProviderError::InvalidManifestPath { + root_id: root_id.clone(), + root, + relative_path, + source, + } + }); + marker_index += 1; + if marker_index == DISCOVERABLE_PLUGIN_MANIFEST_PATHS.len() { + marker_index = 0; + plugin_root = plugin_roots.next(); + } + Some(candidate_uri) + }); + // Pipeline remote metadata calls while consuming results in ancestor/manifest priority order. + let mut results = futures::stream::iter(probes) + .map(|candidate_uri| async move { + let candidate_uri = candidate_uri?; + match file_system + .get_metadata(&candidate_uri, GetMetadataOptions::default(), sandbox) + .await + { + Ok(metadata) if metadata.is_file => Ok(Some(candidate_uri)), + Ok(_) => Ok(None), + Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(None), + Err(source) => Err(ExecutorPluginProviderError::InspectManifest { + root_id: root_id.clone(), + path: candidate_uri, + source, + }), + } + }) + .buffered(MAX_CONCURRENT_MANIFEST_PROBES); + while let Some(result) = results.next().await { + if let Some(manifest) = result? { + return Ok(Some(manifest)); + } + } + Ok(None) +} + #[cfg(test)] #[path = "executor_provider_tests.rs"] mod tests; diff --git a/codex-rs/core-plugins/src/executor_provider_tests.rs b/codex-rs/core-plugins/src/executor_provider_tests.rs index 738c298e65..a6609ab727 100644 --- a/codex-rs/core-plugins/src/executor_provider_tests.rs +++ b/codex-rs/core-plugins/src/executor_provider_tests.rs @@ -1,7 +1,11 @@ use super::ExecutorPluginProvider; use super::ExecutorPluginProviderError; +use super::PluginRootOwnership; +use super::find_manifest; use super::resolve_plugin_root; use crate::manifest::parse_plugin_manifest_uri; +use codex_exec_server::CapabilityRootDiscovery; +use codex_exec_server::CapabilityTextFile; use codex_exec_server::CopyOptions; use codex_exec_server::CreateDirectoryOptions; use codex_exec_server::EnvironmentManager; @@ -24,13 +28,16 @@ use codex_plugin::ResolvedPlugin; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_utils_path_uri::PathUri; +use codex_utils_plugins::DISCOVERABLE_PLUGIN_MANIFEST_PATHS; use pretty_assertions::assert_eq; +use std::collections::HashSet; use std::fs; use std::io; use std::path::Path; use std::sync::Arc; use std::sync::Mutex; use tempfile::tempdir; +use tokio::sync::Semaphore; const MANIFEST_CONTENTS: &str = r#"{ "name": "demo-plugin", @@ -48,6 +55,10 @@ const MANIFEST_CONTENTS: &str = r#"{ #[derive(Debug, PartialEq, Eq)] enum FileSystemCall { Metadata(PathUri), + SandboxedMetadata { + path: PathUri, + sandbox: Box, + }, Read(PathUri), } @@ -55,6 +66,9 @@ struct SyntheticPluginFileSystem { plugin_root: PathUri, manifest_path: PathUri, calls: Mutex>, + metadata_gate: Option>, + metadata_delay: Option<(PathUri, Arc)>, + metadata_error: Option, } impl SyntheticPluginFileSystem { @@ -125,13 +139,30 @@ impl ExecutorFileSystem for SyntheticPluginFileSystem { &'a self, path: &'a PathUri, _options: GetMetadataOptions, - _sandbox: Option<&'a FileSystemSandboxContext>, + sandbox: Option<&'a FileSystemSandboxContext>, ) -> ExecutorFileSystemFuture<'a, FileMetadata> { Box::pin(async move { self.calls .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) - .push(FileSystemCall::Metadata(path.clone())); + .push(match sandbox { + Some(sandbox) => FileSystemCall::SandboxedMetadata { + path: path.clone(), + sandbox: Box::new(sandbox.clone()), + }, + None => FileSystemCall::Metadata(path.clone()), + }); + if let Some(gate) = &self.metadata_gate { + gate.acquire().await.expect("open metadata gate").forget(); + } + if let Some((delayed_path, gate)) = &self.metadata_delay + && delayed_path == path + { + gate.acquire().await.expect("open delayed gate").forget(); + } + if self.metadata_error.as_ref() == Some(path) { + return Err(io::Error::new(io::ErrorKind::PermissionDenied, "denied")); + } let (is_directory, is_file) = if path == &self.plugin_root { (true, false) } else if path == &self.manifest_path { @@ -214,6 +245,206 @@ fn selected_root_uri(id: &str, environment_id: &str, path: PathUri) -> SelectedC } } +#[tokio::test] +async fn root_ownership_survives_malformed_ancestor_manifests() { + let directory = tempdir().expect("tempdir"); + let root = directory.path().join("package/skills"); + fs::create_dir_all(&root).expect("create skills root"); + let provider = ExecutorPluginProvider::new(Arc::new(EnvironmentManager::default_for_tests())); + let selected = selected_root("example@marketplace", LOCAL_ENVIRONMENT_ID, &root); + assert_eq!( + provider + .root_ownership(&selected, /*discovery*/ None, /*sandbox*/ None) + .await + .unwrap(), + PluginRootOwnership::Standalone + ); + write_manifest( + root.parent().unwrap(), + ".codex-plugin/plugin.json", + "{broken", + ); + assert_eq!( + provider + .root_ownership(&selected, /*discovery*/ None, /*sandbox*/ None) + .await + .unwrap(), + PluginRootOwnership::Plugin + ); + let mut discovery = CapabilityRootDiscovery { + id: selected.id.clone(), + path: PathUri::from_host_native_path(&root).unwrap(), + plugin: None, + skills: Vec::new(), + namespace_manifests: Vec::new(), + warnings: vec!["unreadable manifest".into()], + error: None, + }; + assert_eq!( + provider + .root_ownership(&selected, Some(&discovery), /*sandbox*/ None) + .await + .unwrap(), + PluginRootOwnership::Plugin + ); + discovery.error = Some("discovery failed".into()); + assert!(matches!( + provider + .root_ownership(&selected, Some(&discovery), /*sandbox*/ None) + .await, + Err(ExecutorPluginProviderError::DiscoveryFailed { .. }) + )); +} + +#[tokio::test] +async fn discovery_ownership_uses_ancestors_not_nested_plugins() { + let directory = tempdir().expect("tempdir"); + let path = PathUri::from_host_native_path(directory.path()).unwrap(); + let selected = selected_root_uri("example@marketplace", "executor", path.clone()); + let provider = + ExecutorPluginProvider::new(Arc::new(environment_manager_without_environments())); + let mut discovery = CapabilityRootDiscovery { + id: selected.id.clone(), + path: path.clone(), + plugin: None, + skills: Vec::new(), + namespace_manifests: vec![CapabilityTextFile { + path: path.join("nested/.codex-plugin/plugin.json").unwrap(), + contents: "{broken".into(), + }], + warnings: Vec::new(), + error: None, + }; + assert_eq!( + provider + .root_ownership(&selected, Some(&discovery), /*sandbox*/ None) + .await + .unwrap(), + PluginRootOwnership::Standalone + ); + discovery.namespace_manifests[0].path = path.join(".codex-plugin/plugin.json").unwrap(); + assert_eq!( + provider + .root_ownership(&selected, Some(&discovery), /*sandbox*/ None) + .await + .unwrap(), + PluginRootOwnership::Plugin + ); + assert!(matches!( + provider + .root_ownership(&selected, /*discovery*/ None, /*sandbox*/ None) + .await, + Err(ExecutorPluginProviderError::UnavailableEnvironment { .. }) + )); +} + +#[tokio::test] +async fn ancestor_manifest_probes_pipeline_and_preserve_error_order() { + let root = PathUri::parse("file:///a/b/c/d/e/f").expect("selected root URI"); + let owner = PathUri::parse("file:///a/b/c").expect("owner URI"); + let manifest = owner.join(".codex-plugin/plugin.json").unwrap(); + let earlier_error = root.join(".codex-plugin/plugin.json").unwrap(); + let later_error = owner.join(".claude-plugin/plugin.json").unwrap(); + let expected_probes = std::iter::successors(Some(root.clone()), PathUri::parent) + .flat_map(|ancestor| { + DISCOVERABLE_PLUGIN_MANIFEST_PATHS + .iter() + .map(move |marker| ancestor.join(marker).unwrap()) + }) + .collect::>(); + + for (metadata_error, expected) in [ + (None, Ok(Some(manifest.clone()))), + ( + Some(earlier_error.clone()), + Err((earlier_error.clone(), io::ErrorKind::PermissionDenied)), + ), + (Some(later_error), Ok(Some(manifest.clone()))), + ] { + let gate = Arc::new(Semaphore::new(/*permits*/ 0)); + let earliest_gate = Arc::new(Semaphore::new(/*permits*/ 0)); + let file_system = SyntheticPluginFileSystem { + // A directory named like a manifest must not count as plugin ownership. + plugin_root: earlier_error.clone(), + manifest_path: manifest.clone(), + calls: Mutex::new(Vec::new()), + metadata_gate: Some(Arc::clone(&gate)), + metadata_delay: Some((earlier_error.clone(), Arc::clone(&earliest_gate))), + metadata_error, + }; + let selected = selected_root_uri("example@marketplace", "executor", root.clone()); + let mut resolution = Box::pin(find_manifest( + &selected, + std::iter::successors(Some(root.clone()), PathUri::parent), + &file_system, + /*sandbox*/ None, + )); + assert!(futures::poll!(resolution.as_mut()).is_pending()); + let probes = file_system + .calls + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .iter() + .map(|call| match call { + FileSystemCall::Metadata(path) => path.clone(), + call => panic!("unexpected filesystem call: {call:?}"), + }) + .collect::>(); + assert_eq!(probes, expected_probes); + gate.add_permits(probes.len()); + // Later successes/errors cannot outrank the earliest still-pending candidate. + assert!(futures::poll!(resolution.as_mut()).is_pending()); + earliest_gate.add_permits(/*n*/ 1); + let result = resolution.await.map_err(|error| { + let ExecutorPluginProviderError::InspectManifest { path, source, .. } = error else { + panic!("unexpected manifest resolution error: {error}"); + }; + (path, source.kind()) + }); + assert_eq!(result, expected); + } +} + +#[tokio::test] +async fn manifest_presence_uses_sandboxed_metadata_without_reading_contents() { + let directory = tempdir().expect("tempdir"); + let plugin_root = PathUri::from_host_native_path(directory.path()).expect("root URI"); + let manifest_path = plugin_root + .join(".codex-plugin/plugin.json") + .expect("manifest URI"); + let file_system = SyntheticPluginFileSystem { + plugin_root: plugin_root.clone(), + manifest_path: manifest_path.clone(), + calls: Mutex::new(Vec::new()), + metadata_gate: None, + metadata_delay: None, + metadata_error: None, + }; + let sandbox = + FileSystemSandboxContext::from_permission_profile(Default::default(), plugin_root.clone()); + assert_eq!( + find_manifest( + &selected_root_uri("selected-demo", "executor-test", plugin_root.clone()), + std::iter::once(plugin_root.clone()), + &file_system, + Some(&sandbox), + ) + .await + .expect("probe manifest"), + Some(manifest_path.clone()), + ); + assert_eq!( + *file_system + .calls + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner), + vec![FileSystemCall::SandboxedMetadata { + path: manifest_path, + sandbox: Box::new(sandbox) + }], + ); +} + #[tokio::test] async fn plugin_root_resolution_uses_supplied_executor_file_system() { let temp_dir = tempdir().expect("tempdir"); @@ -230,6 +461,9 @@ async fn plugin_root_resolution_uses_supplied_executor_file_system() { plugin_root: plugin_root.clone(), manifest_path: manifest_path.clone(), calls: Mutex::new(Vec::new()), + metadata_gate: None, + metadata_delay: None, + metadata_error: None, }; let resolved = resolve_plugin_root( &selected_root_uri("selected-demo", "executor-test", plugin_root.clone()), @@ -278,6 +512,9 @@ async fn plugin_root_resolution_accepts_foreign_executor_file_uri() { plugin_root: plugin_root.clone(), manifest_path: manifest_path.clone(), calls: Mutex::new(Vec::new()), + metadata_gate: None, + metadata_delay: None, + metadata_error: None, }; let selected_root = selected_root_uri("selected-demo", "executor-test", plugin_root.clone()); let resolved = resolve_plugin_root(&selected_root, plugin_root.clone(), &file_system) diff --git a/codex-rs/core-plugins/src/lib.rs b/codex-rs/core-plugins/src/lib.rs index 513a020e53..b542f867c0 100644 --- a/codex-rs/core-plugins/src/lib.rs +++ b/codex-rs/core-plugins/src/lib.rs @@ -74,6 +74,7 @@ pub use discoverable::ToolSuggestPluginDiscoveryInput; pub use executor_hooks::executor_plugin_hook_sources; pub use executor_provider::ExecutorPluginProvider; pub use executor_provider::ExecutorPluginProviderError; +pub use executor_provider::PluginRootOwnership; pub use executor_provider::ResolvedExecutorPlugin; pub use loader::PluginHookLoadOutcome; pub use manager::ConfiguredMarketplace;