openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-18 17:18:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

Classify executor capability root ownership independently of parsing (#51491)

Browse Source 
## Why

A malformed plugin manifest still marks a capability root as plugin-owned. Discovery or metadata failures must not be treated as evidence that a root is standalone.

## What changed

- Add `ExecutorPluginProvider::root_ownership` and export `PluginRootOwnership` to distinguish plugin-owned roots from standalone roots before capability parsing.
- Use discovery results when conclusive; otherwise probe manifest files at the selected root and its ancestors using the supplied filesystem sandbox. Nested plugin manifests do not establish ownership of their parent root.
- Share manifest lookup with plugin resolution and pipeline up to 256 metadata probes while preserving ancestor and manifest priority, including error ordering.

## Testing

Add tests for malformed ancestor manifests, discovery failures, ancestor versus nested ownership, concurrent probe ordering, and sandboxed metadata checks without reading manifest contents.

GitOrigin-RevId: 3512452cf1df22f52c2a2b422f96c760d98c24ac
This commit is contained in:
stevenlee-oai
2026-10-06 22:36:22 +00:00
committed by copyberry
parent 0b47040dc9
commit b3b83ace17
3 changed files with 371 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core-plugins/src/executor_provider.rs
+131 -34
View File
@@ -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<EnvironmentManager>,
}
/// 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<PluginRootOwnership, ExecutorPluginProviderError> {
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<Item = PathUri> + Send,
file_system: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
) -> Result<Option<PathUri>, 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;
codex-rs/core-plugins/src/executor_provider_tests.rs
+239 -2
View File
@@ -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<FileSystemSandboxContext>,
},
Read(PathUri),
}
@@ -55,6 +66,9 @@ struct SyntheticPluginFileSystem {
plugin_root: PathUri,
manifest_path: PathUri,
calls: Mutex<Vec<FileSystemCall>>,
metadata_gate: Option<Arc<Semaphore>>,
metadata_delay: Option<(PathUri, Arc<Semaphore>)>,
metadata_error: Option<PathUri>,
}
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::<HashSet<_>>();
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::<HashSet<_>>();
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)
codex-rs/core-plugins/src/lib.rs
+1
View File
@@ -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;