From a7e069dc3a88b5bfaba6e6cc57fa0100ef5135b2 Mon Sep 17 00:00:00 2001 From: Anthony Tafoya Date: Fri, 18 Sep 2026 23:32:50 +0000 Subject: [PATCH] Separate plugin catalog listing from package resolution (#46567) ## What changed - Move catalog types and the `PluginProvider` listing interface into `codex-core-plugins`, with a concrete `PluginListQuery` carrying thread, turn, and MCP resource context. - Remove root resolution from `PluginProvider`; keep executor package access in `ExecutorPluginProvider::resolve_bound`, which retains the filesystem used to resolve the package. - Add shared provider result and error types, and re-export the listing API from `codex-mcp-extension`. ## Testing Update executor and manifest tests to use `resolve_bound`, and assert that malformed manifest errors preserve the underlying `serde_json::Error` as their source. GitOrigin-RevId: c6cd5a0269ed3b8ac9b93f306a70821e63e3671e --- .../{plugin => core-plugins}/src/catalog.rs | 0 .../core-plugins/src/executor_provider.rs | 245 ++++++++++++++++ ...er_tests.rs => executor_provider_tests.rs} | 19 +- codex-rs/core-plugins/src/lib.rs | 17 +- codex-rs/core-plugins/src/manifest.rs | 5 +- codex-rs/core-plugins/src/provider.rs | 273 ++---------------- codex-rs/ext/mcp/src/lib.rs | 10 +- codex-rs/ext/mcp/src/provider.rs | 16 - codex-rs/plugin/src/lib.rs | 10 +- codex-rs/plugin/src/provider.rs | 37 +-- 10 files changed, 311 insertions(+), 321 deletions(-) rename codex-rs/{plugin => core-plugins}/src/catalog.rs (100%) create mode 100644 codex-rs/core-plugins/src/executor_provider.rs rename codex-rs/core-plugins/src/{provider_tests.rs => executor_provider_tests.rs} (96%) delete mode 100644 codex-rs/ext/mcp/src/provider.rs diff --git a/codex-rs/plugin/src/catalog.rs b/codex-rs/core-plugins/src/catalog.rs similarity index 100% rename from codex-rs/plugin/src/catalog.rs rename to codex-rs/core-plugins/src/catalog.rs diff --git a/codex-rs/core-plugins/src/executor_provider.rs b/codex-rs/core-plugins/src/executor_provider.rs new file mode 100644 index 0000000000..20399d8ac9 --- /dev/null +++ b/codex-rs/core-plugins/src/executor_provider.rs @@ -0,0 +1,245 @@ +//! Resolves selected plugin roots through their owning executor's filesystem. +//! Resource locations retain that environment's authority. + +use crate::PluginProvider; +use crate::manifest::parse_plugin_manifest_uri; +use codex_exec_server::EnvironmentManager; +use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::GetMetadataOptions; +use codex_exec_server::ReadFileOptions; +use codex_plugin::ResolvedPlugin; +use codex_plugin::ResolvedPluginError; +use codex_protocol::capabilities::CapabilityRootLocation; +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 std::io; +use std::sync::Arc; +use thiserror::Error; + +/// Failure to resolve an environment-owned capability root as a plugin package. +#[derive(Debug, Error)] +pub enum ExecutorPluginProviderError { + #[error( + "selected capability root `{root_id}` references unavailable environment `{environment_id}`" + )] + UnavailableEnvironment { + root_id: String, + environment_id: String, + }, + #[error("failed to inspect selected capability root `{root_id}` at {path}: {source}")] + InspectRoot { + root_id: String, + path: PathUri, + #[source] + source: io::Error, + }, + #[error("selected capability root `{root_id}` path {path} is not a directory")] + RootNotDirectory { root_id: String, path: PathUri }, + #[error( + "failed to resolve plugin manifest path `{relative_path}` below selected capability root `{root_id}` at {root}: {source}" + )] + InvalidManifestPath { + root_id: String, + root: PathUri, + relative_path: &'static str, + #[source] + source: PathUriParseError, + }, + #[error("failed to inspect plugin manifest for `{root_id}` at {path}: {source}")] + InspectManifest { + root_id: String, + path: PathUri, + #[source] + source: io::Error, + }, + #[error("failed to read plugin manifest for `{root_id}` at {path}: {source}")] + ReadManifest { + root_id: String, + path: PathUri, + #[source] + source: io::Error, + }, + #[error("failed to parse plugin manifest for `{root_id}` at {path}: {source}")] + ParseManifest { + root_id: String, + path: PathUri, + #[source] + source: serde_json::Error, + }, + #[error("failed to construct plugin descriptor for `{root_id}`: {source}")] + ConstructDescriptor { + root_id: String, + #[source] + source: ResolvedPluginError, + }, +} + +/// Resolves plugin packages through the filesystem owned by an execution environment. +#[derive(Clone, Debug)] +pub struct ExecutorPluginProvider { + environment_manager: Arc, +} + +/// A resolved plugin paired with the concrete filesystem used to read it. +#[derive(Clone)] +pub struct ResolvedExecutorPlugin { + plugin: ResolvedPlugin, + file_system: Arc, +} + +impl ResolvedExecutorPlugin { + /// Returns the source-neutral plugin descriptor. + pub fn plugin(&self) -> &ResolvedPlugin { + &self.plugin + } + + /// Returns the concrete filesystem that resolved the descriptor. + pub fn file_system(&self) -> &dyn ExecutorFileSystem { + self.file_system.as_ref() + } +} + +impl ExecutorPluginProvider { + /// Creates a provider backed by the active execution environments. + pub fn new(environment_manager: Arc) -> Self { + Self { + environment_manager, + } + } + + /// 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( + &self, + selected_root: &SelectedCapabilityRoot, + ) -> Result, ExecutorPluginProviderError> { + let root_id = &selected_root.id; + let plugin_root = selected_plugin_root(selected_root); + let CapabilityRootLocation::Environment { environment_id, .. } = &selected_root.location; + let environment = self + .environment_manager + .get_environment(environment_id) + .ok_or_else(|| ExecutorPluginProviderError::UnavailableEnvironment { + root_id: root_id.clone(), + environment_id: environment_id.clone(), + })?; + let file_system = environment.get_filesystem(); + let plugin = resolve_plugin_root(selected_root, plugin_root, file_system.as_ref()).await?; + + Ok(plugin.map(|plugin| ResolvedExecutorPlugin { + plugin, + file_system, + })) + } +} + +impl PluginProvider for ExecutorPluginProvider {} + +fn selected_plugin_root(selected_root: &SelectedCapabilityRoot) -> PathUri { + let CapabilityRootLocation::Environment { path, .. } = &selected_root.location; + path.clone() +} + +async fn resolve_plugin_root( + selected_root: &SelectedCapabilityRoot, + plugin_root: PathUri, + file_system: &dyn ExecutorFileSystem, +) -> Result, ExecutorPluginProviderError> { + let root_id = &selected_root.id; + let CapabilityRootLocation::Environment { environment_id, .. } = &selected_root.location; + let root_metadata = file_system + .get_metadata( + &plugin_root, + GetMetadataOptions::default(), + /*sandbox*/ None, + ) + .await + .map_err(|source| ExecutorPluginProviderError::InspectRoot { + root_id: root_id.clone(), + path: plugin_root.clone(), + source, + })?; + if !root_metadata.is_directory { + return Err(ExecutorPluginProviderError::RootNotDirectory { + root_id: root_id.clone(), + path: 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 { + return Ok(None); + }; + let contents = file_system + .read_file_text( + &manifest_uri, + ReadFileOptions::default(), + /*sandbox*/ None, + ) + .await + .map_err(|source| ExecutorPluginProviderError::ReadManifest { + root_id: root_id.clone(), + path: manifest_uri.clone(), + source, + })?; + let manifest = + parse_plugin_manifest_uri(&plugin_root, &manifest_uri, &contents).map_err(|source| { + ExecutorPluginProviderError::ParseManifest { + root_id: root_id.clone(), + path: manifest_uri.clone(), + source, + } + })?; + + let plugin = ResolvedPlugin::from_environment( + root_id.clone(), + environment_id.clone(), + plugin_root, + manifest_uri, + manifest, + ) + .map_err(|source| ExecutorPluginProviderError::ConstructDescriptor { + root_id: root_id.clone(), + source, + })?; + + Ok(Some(plugin)) +} + +#[cfg(test)] +#[path = "executor_provider_tests.rs"] +mod tests; diff --git a/codex-rs/core-plugins/src/provider_tests.rs b/codex-rs/core-plugins/src/executor_provider_tests.rs similarity index 96% rename from codex-rs/core-plugins/src/provider_tests.rs rename to codex-rs/core-plugins/src/executor_provider_tests.rs index 612f22c8e9..738c298e65 100644 --- a/codex-rs/core-plugins/src/provider_tests.rs +++ b/codex-rs/core-plugins/src/executor_provider_tests.rs @@ -20,7 +20,6 @@ use codex_exec_server::WalkOptions; use codex_exec_server::WalkOutcome; use codex_exec_server::WriteFileOptions; use codex_exec_server_test_support::environment_manager_without_environments; -use codex_plugin::PluginProvider; use codex_plugin::ResolvedPlugin; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; @@ -319,7 +318,7 @@ async fn standalone_capability_root_is_not_a_plugin() { let provider = ExecutorPluginProvider::new(Arc::new(EnvironmentManager::default_for_tests())); let resolved = provider - .resolve(&selected_root( + .resolve_bound(&selected_root( "standalone", LOCAL_ENVIRONMENT_ID, &standalone_root, @@ -327,7 +326,7 @@ async fn standalone_capability_root_is_not_a_plugin() { .await .expect("resolve standalone root"); - assert_eq!(resolved, None); + assert!(resolved.is_none()); } #[tokio::test] @@ -346,7 +345,7 @@ async fn root_agent_plugin_manifest_is_not_an_executor_plugin() { let provider = ExecutorPluginProvider::new(Arc::new(EnvironmentManager::default_for_tests())); let resolved = provider - .resolve(&selected_root( + .resolve_bound(&selected_root( "agent-plugin", LOCAL_ENVIRONMENT_ID, &plugin_root, @@ -354,7 +353,7 @@ async fn root_agent_plugin_manifest_is_not_an_executor_plugin() { .await .expect("resolve selected root"); - assert_eq!(resolved, None); + assert!(resolved.is_none()); } #[tokio::test] @@ -366,8 +365,9 @@ async fn unavailable_environment_does_not_fall_back_to_host_filesystem() { ExecutorPluginProvider::new(Arc::new(environment_manager_without_environments())); let err = provider - .resolve(&selected_root("host-plugin", "missing", &plugin_root)) + .resolve_bound(&selected_root("host-plugin", "missing", &plugin_root)) .await + .map(|_| ()) .expect_err("missing environment should fail"); assert_eq!( @@ -392,14 +392,19 @@ async fn malformed_preferred_manifest_does_not_fall_through_to_alternate() { let provider = ExecutorPluginProvider::new(Arc::new(EnvironmentManager::default_for_tests())); let err = provider - .resolve(&selected_root( + .resolve_bound(&selected_root( "selected-demo", LOCAL_ENVIRONMENT_ID, &plugin_root, )) .await + .map(|_| ()) .expect_err("malformed preferred manifest should fail"); + assert!( + std::error::Error::source(&err) + .is_some_and(::is::) + ); let ExecutorPluginProviderError::ParseManifest { root_id, path, diff --git a/codex-rs/core-plugins/src/lib.rs b/codex-rs/core-plugins/src/lib.rs index 04ebe593ba..513a020e53 100644 --- a/codex-rs/core-plugins/src/lib.rs +++ b/codex-rs/core-plugins/src/lib.rs @@ -1,9 +1,11 @@ mod app_mcp_routing; mod artifact_operation; +mod catalog; mod command_migration; mod discoverable; mod error_subtype; mod executor_hooks; +mod executor_provider; mod git_policy; mod http_client_selector; pub mod installed_marketplaces; @@ -57,6 +59,10 @@ pub type PluginLoadOutcome = codex_plugin::PluginLoadOutcome, +pub struct PluginListQuery { + pub thread_id: String, + pub turn_id: String, + pub mcp_resources: Option>, } -/// A resolved plugin paired with the concrete filesystem used to read it. -#[derive(Clone)] -pub struct ResolvedExecutorPlugin { - plugin: ResolvedPlugin, - file_system: Arc, +/// Failure to list plugin metadata. +#[derive(Debug, Error)] +pub enum PluginProviderError { + #[error(transparent)] + Executor(#[from] ExecutorPluginProviderError), + #[error("{0}")] + Message(String), } -impl ResolvedExecutorPlugin { - /// Returns the source-neutral plugin descriptor. - pub fn plugin(&self) -> &ResolvedPlugin { - &self.plugin - } +pub type PluginProviderResult = Result; - /// Returns the concrete filesystem that resolved the descriptor. - pub fn file_system(&self) -> &dyn ExecutorFileSystem { - self.file_system.as_ref() +pub type PluginProviderFuture<'a, T> = + Pin> + Send + 'a>>; + +/// Lists plugin metadata without resolving packages or binding filesystems. +/// +/// Cloud providers implement `list`; executor listing is not yet supported. +/// Package access uses [`crate::ExecutorPluginProvider::resolve_bound`] directly. +pub trait PluginProvider: Send + Sync { + /// Returns a complete snapshot; on error, callers retain the previous snapshot. + fn list(&self, _query: PluginListQuery) -> PluginProviderFuture<'_, PluginCatalog> { + Box::pin(async { Ok(PluginCatalog::default()) }) } } - -impl ExecutorPluginProvider { - /// Creates a provider backed by the active execution environments. - pub fn new(environment_manager: Arc) -> Self { - Self { - environment_manager, - } - } - - /// 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( - &self, - selected_root: &SelectedCapabilityRoot, - ) -> Result, ExecutorPluginProviderError> { - let root_id = &selected_root.id; - let plugin_root = selected_plugin_root(selected_root); - let CapabilityRootLocation::Environment { environment_id, .. } = &selected_root.location; - let environment = self - .environment_manager - .get_environment(environment_id) - .ok_or_else(|| ExecutorPluginProviderError::UnavailableEnvironment { - root_id: root_id.clone(), - environment_id: environment_id.clone(), - })?; - let file_system = environment.get_filesystem(); - let plugin = resolve_plugin_root(selected_root, plugin_root, file_system.as_ref()).await?; - - Ok(plugin.map(|plugin| ResolvedExecutorPlugin { - plugin, - file_system, - })) - } -} - -impl PluginProvider for ExecutorPluginProvider { - type Error = ExecutorPluginProviderError; - - async fn resolve( - &self, - selected_root: &SelectedCapabilityRoot, - ) -> Result, Self::Error> { - self.resolve_bound(selected_root) - .await - .map(|plugin| plugin.map(|plugin| plugin.plugin)) - } -} - -fn selected_plugin_root(selected_root: &SelectedCapabilityRoot) -> PathUri { - let CapabilityRootLocation::Environment { path, .. } = &selected_root.location; - path.clone() -} - -async fn resolve_plugin_root( - selected_root: &SelectedCapabilityRoot, - plugin_root: PathUri, - file_system: &dyn ExecutorFileSystem, -) -> Result, ExecutorPluginProviderError> { - let root_id = &selected_root.id; - let CapabilityRootLocation::Environment { environment_id, .. } = &selected_root.location; - let root_metadata = file_system - .get_metadata( - &plugin_root, - GetMetadataOptions::default(), - /*sandbox*/ None, - ) - .await - .map_err(|source| ExecutorPluginProviderError::InspectRoot { - root_id: root_id.clone(), - path: plugin_root.clone(), - source, - })?; - if !root_metadata.is_directory { - return Err(ExecutorPluginProviderError::RootNotDirectory { - root_id: root_id.clone(), - path: 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 { - return Ok(None); - }; - let contents = file_system - .read_file_text( - &manifest_uri, - ReadFileOptions::default(), - /*sandbox*/ None, - ) - .await - .map_err(|source| ExecutorPluginProviderError::ReadManifest { - root_id: root_id.clone(), - path: manifest_uri.clone(), - source, - })?; - let manifest = - parse_plugin_manifest_uri(&plugin_root, &manifest_uri, &contents).map_err(|source| { - ExecutorPluginProviderError::ParseManifest { - root_id: root_id.clone(), - path: manifest_uri.clone(), - source, - } - })?; - - let plugin = ResolvedPlugin::from_environment( - root_id.clone(), - environment_id.clone(), - plugin_root, - manifest_uri, - manifest, - ) - .map_err(|source| ExecutorPluginProviderError::ConstructDescriptor { - root_id: root_id.clone(), - source, - })?; - - Ok(Some(plugin)) -} - -#[cfg(test)] -#[path = "provider_tests.rs"] -mod tests; diff --git a/codex-rs/ext/mcp/src/lib.rs b/codex-rs/ext/mcp/src/lib.rs index ecb9a20a36..62689947e9 100644 --- a/codex-rs/ext/mcp/src/lib.rs +++ b/codex-rs/ext/mcp/src/lib.rs @@ -11,13 +11,13 @@ use codex_mcp::hosted_plugin_runtime_mcp_server_config; #[path = "event_stream_tests.rs"] mod event_stream_tests; mod plugin; -mod provider; mod stream_manager; -pub use provider::PluginListQuery; -pub use provider::PluginProvider; -pub use provider::PluginProviderError; -pub use provider::PluginProviderFuture; +pub use codex_core_plugins::PluginListQuery; +pub use codex_core_plugins::PluginProvider; +pub use codex_core_plugins::PluginProviderError; +pub use codex_core_plugins::PluginProviderFuture; +pub use codex_core_plugins::PluginProviderResult; pub use stream_manager::McpEventStreamManager; pub use stream_manager::McpEventStreamUpdate; diff --git a/codex-rs/ext/mcp/src/provider.rs b/codex-rs/ext/mcp/src/provider.rs deleted file mode 100644 index 1fbe5d8915..0000000000 --- a/codex-rs/ext/mcp/src/provider.rs +++ /dev/null @@ -1,16 +0,0 @@ -//! MCP context passed to plugin catalog providers. - -use codex_mcp::McpResourceClient; -use std::sync::Arc; - -pub use codex_plugin::PluginProvider; -pub use codex_plugin::PluginProviderError; -pub use codex_plugin::PluginProviderFuture; - -/// Turn identifiers and MCP client used to request one catalog snapshot. -#[derive(Clone, Debug)] -pub struct PluginListQuery { - pub thread_id: String, - pub turn_id: String, - pub mcp_resources: Option>, -} diff --git a/codex-rs/plugin/src/lib.rs b/codex-rs/plugin/src/lib.rs index d981aef2b0..ed81bb5e92 100644 --- a/codex-rs/plugin/src/lib.rs +++ b/codex-rs/plugin/src/lib.rs @@ -1,21 +1,16 @@ -//! Shared plugin package models, source providers, identifiers, and telemetry summaries. +//! Shared plugin package models, identifiers, and telemetry summaries. use std::collections::HashSet; pub use codex_utils_plugins::mention_syntax; mod bundled_hooks; -mod catalog; mod load_outcome; pub mod manifest; mod plugin_id; mod provider; pub use bundled_hooks::is_allowlisted_bundled_cleanup_hook; -pub use catalog::PluginCatalog; -pub use catalog::PluginCatalogEntry; -pub use catalog::PluginIdentity; -pub use catalog::PluginSourceLocation; use codex_config::HookEventsToml; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; @@ -25,9 +20,6 @@ pub use load_outcome::prompt_safe_plugin_description; pub use plugin_id::PluginId; pub use plugin_id::PluginIdError; pub use plugin_id::validate_plugin_segment; -pub use provider::PluginProvider; -pub use provider::PluginProviderError; -pub use provider::PluginProviderFuture; pub use provider::PluginResourceLocator; pub use provider::ResolvedPlugin; pub use provider::ResolvedPluginError; diff --git a/codex-rs/plugin/src/provider.rs b/codex-rs/plugin/src/provider.rs index 2cb325dec6..82d1bb8a3a 100644 --- a/codex-rs/plugin/src/provider.rs +++ b/codex-rs/plugin/src/provider.rs @@ -1,12 +1,7 @@ -//! Shared APIs for catalog discovery and root-at-a-time plugin resolution. +//! Authority-bound plugin descriptors and resource locations. -use crate::PluginCatalog; use crate::manifest::PluginManifest; -use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_utils_path_uri::PathUri; -use std::error::Error as StdError; -use std::future::Future; -use std::pin::Pin; use thiserror::Error; /// A plugin resource paired with the environment that owns its filesystem. @@ -108,36 +103,6 @@ fn environment_resource( }) } -/// Error returned when a provider cannot produce a complete catalog snapshot. -#[derive(Clone, Debug, Error, PartialEq, Eq)] -#[error("{0}")] -pub struct PluginProviderError(pub String); - -pub type PluginProviderFuture<'a, T> = - Pin> + Send + 'a>>; - -/// Discovers plugin metadata through batch listing or single-root resolution. -/// -/// Cloud providers implement `list`; executor providers continue implementing `resolve`. -/// Each contributor invokes its provider's supported operation; the other is a no-op. -pub trait PluginProvider: Send + Sync { - /// Error returned by root-at-a-time resolution. - type Error: StdError + Send + Sync + 'static; - - /// Returns a complete snapshot. - fn list(&self, _query: ListQuery) -> PluginProviderFuture<'_, PluginCatalog> { - Box::pin(async { Ok(PluginCatalog::default()) }) - } - - /// Resolves one selected root using the filesystem authority named by that root. - fn resolve( - &self, - _root: &SelectedCapabilityRoot, - ) -> impl Future, Self::Error>> + Send { - async { Ok(None) } - } -} - #[cfg(test)] #[path = "provider_tests.rs"] mod tests;