mirror of
https://github.com/openai/codex.git
synced 2026-09-20 12:47:38 +00:00
Route skill discovery and loading through EnvironmentAccess (#46293)
## What changed Use `EnvironmentAccess` for skill discovery, environment skill loading, and plugin namespace resolution, replacing direct `ExecutorFileSystem` calls with explicit `None` sandbox arguments. Wrap existing host and executor filesystems with `FileSystemEnvironmentAccessor::unrestricted` to preserve their current access behavior. Update discovery, loading, and namespace tests to use the same adapter. GitOrigin-RevId: d68168cc0ee19dc303ef4ee726cf1de1a8db81e0
This commit is contained in:
committed by
copyberry
parent
93321c88d8
commit
47fc8d661e
@@ -1,7 +1,7 @@
|
||||
use std::collections::HashSet;
|
||||
use std::io;
|
||||
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::EnvironmentAccess;
|
||||
use codex_exec_server::WalkEntryKind;
|
||||
use codex_exec_server::WalkOptions;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
@@ -52,7 +52,7 @@ pub(super) enum SkillMetadataDiscovery {
|
||||
}
|
||||
|
||||
pub(super) async fn discover_skills(
|
||||
file_system: &dyn ExecutorFileSystem,
|
||||
file_system: &dyn EnvironmentAccess,
|
||||
root: &PathUri,
|
||||
options: SkillDiscoveryOptions,
|
||||
) -> SkillDiscovery {
|
||||
@@ -81,7 +81,6 @@ pub(super) async fn discover_skills(
|
||||
HiddenDirectoryPolicy::Skip
|
||||
),
|
||||
},
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.await
|
||||
{
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
|
||||
use codex_exec_server::FileSystemEnvironmentAccessor;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use codex_utils_plugins::SkillDiscoveryMode;
|
||||
@@ -27,7 +28,7 @@ fn canonical_uri(path: &Path) -> PathUri {
|
||||
|
||||
async fn discover(root: &Path) -> SkillDiscovery {
|
||||
discover_skills(
|
||||
LOCAL_FS.as_ref(),
|
||||
&FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS),
|
||||
&canonical_uri(root),
|
||||
SkillDiscoveryOptions {
|
||||
directory_symlinks: DirectorySymlinkPolicy::Follow,
|
||||
|
||||
@@ -2,7 +2,8 @@ use std::collections::HashMap;
|
||||
use std::io;
|
||||
|
||||
use codex_exec_server::CapabilityRootDiscovery;
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::EnvironmentAccess;
|
||||
use codex_exec_server::EnvironmentAccessExt;
|
||||
use codex_exec_server::GetMetadataOptions;
|
||||
use codex_exec_server::ReadFileOptions;
|
||||
use codex_protocol::protocol::Product;
|
||||
@@ -54,7 +55,7 @@ pub struct EnvironmentSkillSnapshotOutcome {
|
||||
|
||||
impl ParsedEnvironmentSkill {
|
||||
async fn load(
|
||||
file_system: &dyn ExecutorFileSystem,
|
||||
file_system: &dyn EnvironmentAccess,
|
||||
skill: &DiscoveredSkill,
|
||||
) -> Result<Self, String> {
|
||||
let (contents, discovered_metadata) = match &skill.metadata {
|
||||
@@ -110,7 +111,7 @@ pub struct EnvironmentSkillLoadOutcome {
|
||||
fields(skill_count = tracing::field::Empty)
|
||||
)]
|
||||
pub async fn load_environment_skills_from_root(
|
||||
file_system: &dyn ExecutorFileSystem,
|
||||
file_system: &dyn EnvironmentAccess,
|
||||
root: &PathUri,
|
||||
restriction_product: Option<Product>,
|
||||
) -> EnvironmentSkillLoadOutcome {
|
||||
@@ -324,29 +325,21 @@ fn nearest_plugin_namespace<'a>(
|
||||
None
|
||||
}
|
||||
async fn read_skill_contents(
|
||||
file_system: &dyn ExecutorFileSystem,
|
||||
file_system: &dyn EnvironmentAccess,
|
||||
skill_path: &PathUri,
|
||||
) -> Result<String, String> {
|
||||
file_system
|
||||
.read_file_text(
|
||||
skill_path,
|
||||
ReadFileOptions::default(),
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.read_file_text(skill_path, ReadFileOptions::default())
|
||||
.await
|
||||
.map_err(|err| format!("failed to read file: {err}"))
|
||||
}
|
||||
|
||||
async fn probe_skill_metadata(
|
||||
file_system: &dyn ExecutorFileSystem,
|
||||
file_system: &dyn EnvironmentAccess,
|
||||
metadata_path: &PathUri,
|
||||
) -> (Option<SkillDependencies>, Option<SkillPolicy>) {
|
||||
match file_system
|
||||
.get_metadata(
|
||||
metadata_path,
|
||||
GetMetadataOptions::default(),
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.get_metadata(metadata_path, GetMetadataOptions::default())
|
||||
.await
|
||||
{
|
||||
Ok(metadata) if metadata.is_file => {}
|
||||
@@ -361,15 +354,11 @@ async fn probe_skill_metadata(
|
||||
}
|
||||
|
||||
async fn read_skill_metadata(
|
||||
file_system: &dyn ExecutorFileSystem,
|
||||
file_system: &dyn EnvironmentAccess,
|
||||
metadata_path: &PathUri,
|
||||
) -> (Option<SkillDependencies>, Option<SkillPolicy>) {
|
||||
let contents = match file_system
|
||||
.read_file_text(
|
||||
metadata_path,
|
||||
ReadFileOptions::default(),
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.read_file_text(metadata_path, ReadFileOptions::default())
|
||||
.await
|
||||
{
|
||||
Ok(contents) => contents,
|
||||
|
||||
@@ -1,6 +1,9 @@
|
||||
use std::fs;
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::FileSystemEnvironmentAccessor;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_skills::EnvironmentSkillMetadata;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
@@ -50,11 +53,14 @@ async fn loads_nearest_plugin_namespaces_without_reading_unused_sibling_manifest
|
||||
.expect("skill");
|
||||
}
|
||||
|
||||
let file_system =
|
||||
RecordingFileSystem::new(LOCAL_FS.as_ref(), ManifestMetadataBehavior::Immediate);
|
||||
let file_system = Arc::new(RecordingFileSystem::new(
|
||||
LOCAL_FS.as_ref(),
|
||||
ManifestMetadataBehavior::Immediate,
|
||||
));
|
||||
let executor: Arc<dyn ExecutorFileSystem> = file_system.clone();
|
||||
let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI");
|
||||
let outcome = load_environment_skills_from_root(
|
||||
&file_system,
|
||||
&FileSystemEnvironmentAccessor::unrestricted(&executor),
|
||||
&root_uri,
|
||||
/*restriction_product*/ None,
|
||||
)
|
||||
@@ -130,11 +136,14 @@ async fn reuses_walk_inventory_for_missing_skill_metadata() {
|
||||
skill_paths.push(skill_path);
|
||||
}
|
||||
|
||||
let file_system =
|
||||
RecordingFileSystem::new(LOCAL_FS.as_ref(), ManifestMetadataBehavior::Immediate);
|
||||
let file_system = Arc::new(RecordingFileSystem::new(
|
||||
LOCAL_FS.as_ref(),
|
||||
ManifestMetadataBehavior::Immediate,
|
||||
));
|
||||
let executor: Arc<dyn ExecutorFileSystem> = file_system.clone();
|
||||
let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI");
|
||||
let outcome = load_environment_skills_from_root(
|
||||
&file_system,
|
||||
&FileSystemEnvironmentAccessor::unrestricted(&executor),
|
||||
&root_uri,
|
||||
/*restriction_product*/ None,
|
||||
)
|
||||
@@ -193,15 +202,15 @@ async fn reads_skill_files_while_resolving_plugin_namespaces() {
|
||||
)
|
||||
.expect("skill");
|
||||
|
||||
let file_system = RecordingFileSystem::new(
|
||||
let file_system: Arc<dyn ExecutorFileSystem> = Arc::new(RecordingFileSystem::new(
|
||||
LOCAL_FS.as_ref(),
|
||||
ManifestMetadataBehavior::WaitForSkillRead,
|
||||
);
|
||||
));
|
||||
let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI");
|
||||
let outcome = tokio::time::timeout(
|
||||
Duration::from_secs(5),
|
||||
load_environment_skills_from_root(
|
||||
&file_system,
|
||||
&FileSystemEnvironmentAccessor::unrestricted(&file_system),
|
||||
&root_uri,
|
||||
/*restriction_product*/ None,
|
||||
),
|
||||
|
||||
@@ -2,6 +2,7 @@ use std::fs;
|
||||
|
||||
use codex_exec_server::CapabilityRootDiscoverRequest;
|
||||
use codex_exec_server::CapabilityRootsDiscoverParams;
|
||||
use codex_exec_server::FileSystemEnvironmentAccessor;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_exec_server::discover_capability_roots;
|
||||
use codex_protocol::protocol::Product;
|
||||
@@ -50,8 +51,9 @@ policy:
|
||||
.expect("metadata");
|
||||
|
||||
let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI");
|
||||
let file_system = FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS);
|
||||
let outcome =
|
||||
load_environment_skills_from_root(LOCAL_FS.as_ref(), &root_uri, Some(Product::Codex)).await;
|
||||
load_environment_skills_from_root(&file_system, &root_uri, Some(Product::Codex)).await;
|
||||
|
||||
assert_eq!(
|
||||
outcome.skills,
|
||||
@@ -78,11 +80,10 @@ policy:
|
||||
}]
|
||||
);
|
||||
let atlas =
|
||||
load_environment_skills_from_root(LOCAL_FS.as_ref(), &root_uri, Some(Product::Atlas)).await;
|
||||
load_environment_skills_from_root(&file_system, &root_uri, Some(Product::Atlas)).await;
|
||||
assert_eq!(atlas.skills, outcome.skills);
|
||||
let filtered =
|
||||
load_environment_skills_from_root(LOCAL_FS.as_ref(), &root_uri, Some(Product::Chatgpt))
|
||||
.await;
|
||||
load_environment_skills_from_root(&file_system, &root_uri, Some(Product::Chatgpt)).await;
|
||||
assert!(filtered.skills.is_empty());
|
||||
}
|
||||
|
||||
@@ -116,7 +117,7 @@ async fn executor_bundle_parser_matches_direct_environment_loader() {
|
||||
|
||||
let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI");
|
||||
let existing = load_environment_skills_from_root(
|
||||
LOCAL_FS.as_ref(),
|
||||
&FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS),
|
||||
&root_uri,
|
||||
/*restriction_product*/ None,
|
||||
)
|
||||
@@ -184,7 +185,7 @@ async fn executor_bundle_preserves_parent_namespace_and_manifest_precedence() {
|
||||
|
||||
let root_uri = PathUri::from_host_native_path(&skills_root).expect("skills root URI");
|
||||
let existing = load_environment_skills_from_root(
|
||||
LOCAL_FS.as_ref(),
|
||||
&FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS),
|
||||
&root_uri,
|
||||
/*restriction_product*/ None,
|
||||
)
|
||||
|
||||
@@ -2,6 +2,7 @@ use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::FileSystemEnvironmentAccessor;
|
||||
use codex_exec_server::GetMetadataOptions;
|
||||
use codex_exec_server::ReadFileOptions;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
@@ -151,6 +152,9 @@ async fn load_skills_under_root(
|
||||
Vec<SkillError>,
|
||||
) {
|
||||
let file_system = skill_root.file_system.as_ref();
|
||||
// TODO(anp): Bind discovery to turn permissions when host skill roots accept an accessor;
|
||||
// until then, keep using the same unrestricted filesystem that supplied the root.
|
||||
let discovery_access = FileSystemEnvironmentAccessor::unrestricted(&skill_root.file_system);
|
||||
let plugin_identity = skill_root.plugin_identity();
|
||||
let plugin_root = match skill_root.plugin_root() {
|
||||
Some(plugin_root) => Some(canonicalize_for_skill_identity(file_system, plugin_root).await),
|
||||
@@ -167,7 +171,7 @@ async fn load_skills_under_root(
|
||||
mut namespace_roots,
|
||||
warnings,
|
||||
} = discover_skills(
|
||||
file_system,
|
||||
&discovery_access,
|
||||
&PathUri::from_abs_path(root),
|
||||
SkillDiscoveryOptions {
|
||||
directory_symlinks,
|
||||
@@ -270,7 +274,7 @@ async fn load_skills_under_root(
|
||||
Some(namespace) => SkillNamespaceResolver::with_provided_namespace(namespace),
|
||||
None => {
|
||||
SkillNamespaceResolver::discover(
|
||||
file_system,
|
||||
&discovery_access,
|
||||
&root_uri,
|
||||
&skill_paths,
|
||||
plugin_roots,
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::EnvironmentAccess;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use codex_utils_plugins::plugin_namespace_for_root_uri;
|
||||
use futures::StreamExt;
|
||||
@@ -37,7 +37,7 @@ impl SkillNamespaceResolver {
|
||||
}
|
||||
|
||||
pub(crate) async fn discover(
|
||||
fs: &dyn ExecutorFileSystem,
|
||||
fs: &dyn EnvironmentAccess,
|
||||
root: &PathUri,
|
||||
skill_paths: &[PathUri],
|
||||
plugin_roots: HashSet<PathUri>,
|
||||
|
||||
@@ -4,6 +4,7 @@ use std::path::Path;
|
||||
#[cfg(unix)]
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_exec_server::FileSystemEnvironmentAccessor;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
#[cfg(unix)]
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
@@ -50,7 +51,7 @@ async fn namespaces_for(
|
||||
let root = canonical_uri(root);
|
||||
namespace_roots.insert(root.clone());
|
||||
let resolver = SkillNamespaceResolver::discover(
|
||||
LOCAL_FS.as_ref(),
|
||||
&FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS),
|
||||
&root,
|
||||
skill_paths,
|
||||
plugin_roots,
|
||||
|
||||
@@ -3,6 +3,7 @@ use std::collections::HashSet;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_exec_server::EnvironmentManager;
|
||||
use codex_exec_server::FileSystemEnvironmentAccessor;
|
||||
use codex_exec_server::FileSystemSandboxContext;
|
||||
use codex_extension_api::SelectedPluginSnapshot;
|
||||
use codex_protocol::capabilities::CapabilityRootLocation;
|
||||
@@ -113,8 +114,10 @@ impl SkillProvider for ExecutorSkillProvider {
|
||||
));
|
||||
continue;
|
||||
};
|
||||
// TODO(anp): Take this accessor from the selected turn root when discovery receives
|
||||
// turn permissions; until then, preserve direct access through its existing filesystem.
|
||||
let outcome = load_environment_skills_from_root(
|
||||
file_system.as_ref(),
|
||||
&FileSystemEnvironmentAccessor::unrestricted(&file_system),
|
||||
path,
|
||||
self.restriction_product,
|
||||
)
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
//! Resolve plugin namespace from skill file paths by walking ancestors for `plugin.json`.
|
||||
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::EnvironmentAccess;
|
||||
use codex_exec_server::EnvironmentAccessExt;
|
||||
use codex_exec_server::GetMetadataOptions;
|
||||
use codex_exec_server::ReadFileOptions;
|
||||
use codex_exec_server_protocol::DISCOVERABLE_PLUGIN_MANIFEST_PATHS;
|
||||
@@ -87,18 +88,14 @@ struct RawPluginManifestName {
|
||||
|
||||
/// Returns the plugin manifest `name` defined directly below `plugin_root`.
|
||||
pub async fn plugin_namespace_for_root_uri(
|
||||
fs: &dyn ExecutorFileSystem,
|
||||
fs: &dyn EnvironmentAccess,
|
||||
plugin_root: &PathUri,
|
||||
) -> Option<String> {
|
||||
let mut manifest_path = None;
|
||||
for relative_path in DISCOVERABLE_PLUGIN_MANIFEST_PATHS {
|
||||
let candidate = plugin_root.join(relative_path).ok()?;
|
||||
match fs
|
||||
.get_metadata(
|
||||
&candidate,
|
||||
GetMetadataOptions::default(),
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.get_metadata(&candidate, GetMetadataOptions::default())
|
||||
.await
|
||||
{
|
||||
Ok(metadata) if metadata.is_file => {
|
||||
@@ -109,11 +106,7 @@ pub async fn plugin_namespace_for_root_uri(
|
||||
}
|
||||
}
|
||||
let contents = fs
|
||||
.read_file_text(
|
||||
&manifest_path?,
|
||||
ReadFileOptions::default(),
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.read_file_text(&manifest_path?, ReadFileOptions::default())
|
||||
.await
|
||||
.ok()?;
|
||||
let RawPluginManifestName { name: raw_name } = serde_json::from_str(&contents).ok()?;
|
||||
@@ -131,6 +124,7 @@ mod tests {
|
||||
use super::AGENT_PLUGIN_SCHEMA_URI;
|
||||
use super::find_plugin_manifest_path;
|
||||
use super::plugin_namespace_for_root_uri;
|
||||
use codex_exec_server::FileSystemEnvironmentAccessor;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_utils_absolute_path::test_support::PathBufExt;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
@@ -157,7 +151,7 @@ mod tests {
|
||||
|
||||
assert_eq!(
|
||||
plugin_namespace_for_root_uri(
|
||||
LOCAL_FS.as_ref(),
|
||||
&FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS),
|
||||
&PathUri::from_abs_path(&plugin_root.abs()),
|
||||
)
|
||||
.await,
|
||||
@@ -180,7 +174,7 @@ mod tests {
|
||||
|
||||
assert_eq!(
|
||||
plugin_namespace_for_root_uri(
|
||||
LOCAL_FS.as_ref(),
|
||||
&FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS),
|
||||
&PathUri::from_abs_path(&plugin_root.abs()),
|
||||
)
|
||||
.await,
|
||||
@@ -204,7 +198,7 @@ mod tests {
|
||||
|
||||
assert_eq!(
|
||||
plugin_namespace_for_root_uri(
|
||||
LOCAL_FS.as_ref(),
|
||||
&FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS),
|
||||
&PathUri::from_abs_path(&plugin_root.abs()),
|
||||
)
|
||||
.await,
|
||||
|
||||
Reference in New Issue
Block a user