diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index b377e909e..deab6f395 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -1,3 +1,9 @@ +mod environment; + +pub use environment::EnvironmentSkillLoadOutcome; +pub use environment::EnvironmentSkillMetadata; +pub use environment::load_environment_skills_from_root; + use crate::model::SkillDependencies; use crate::model::SkillError; use crate::model::SkillInterface; @@ -104,6 +110,13 @@ struct DependencyTool { url: Option, } +#[derive(Debug, Clone, PartialEq, Eq)] +struct ParsedSkillFrontmatter { + name: String, + description: String, + short_description: Option, +} + const SKILLS_FILENAME: &str = "SKILL.md"; const AGENTS_DIR_NAME: &str = ".agents"; const SKILLS_METADATA_DIR: &str = "agents"; @@ -124,6 +137,17 @@ const MAX_DEPENDENCY_URL_LEN: usize = MAX_DESCRIPTION_LEN; const MAX_SCAN_DEPTH: usize = 6; const MAX_SKILLS_DIRS_PER_ROOT: usize = 2000; +#[derive(Clone, Copy)] +enum SymlinkPolicy { + FollowDirectories, + Ignore, +} + +struct SkillFileDiscovery { + skill_files: Vec, + warnings: Vec, +} + #[derive(Debug)] enum SkillParseError { Read(std::io::Error), @@ -189,7 +213,7 @@ pub(crate) async fn load_skill_root(root: SkillRoot) -> SkillRootSnapshot { } = root; let root = canonicalize_for_skill_identity(file_system.as_ref(), &path).await; let mut outcome = SkillLoadOutcome::default(); - discover_skills_under_root( + load_skills_under_root( file_system.as_ref(), &root, scope, @@ -470,35 +494,31 @@ async fn canonicalize_for_skill_identity( async fn discover_skills_under_root( fs: &dyn ExecutorFileSystem, - root: &AbsolutePathBuf, - scope: SkillScope, - plugin_id: Option<&str>, - plugin_namespace: Option<&str>, - plugin_root: Option<&AbsolutePathBuf>, - outcome: &mut SkillLoadOutcome, -) { + root: &PathUri, + symlink_policy: SymlinkPolicy, +) -> SkillFileDiscovery { let root = root.clone(); - let plugin_root = match plugin_root { - Some(plugin_root) => Some(canonicalize_for_skill_identity(fs, plugin_root).await), - None => None, + let mut discovery = SkillFileDiscovery { + skill_files: Vec::new(), + warnings: Vec::new(), }; - - let root_uri = PathUri::from_abs_path(&root); - match fs.get_metadata(&root_uri, /*sandbox*/ None).await { + match fs.get_metadata(&root, /*sandbox*/ None).await { Ok(metadata) if metadata.is_directory => {} - Ok(_) => return, - Err(err) if err.kind() == io::ErrorKind::NotFound => return, + Ok(_) => return discovery, + Err(err) if err.kind() == io::ErrorKind::NotFound => return discovery, Err(err) => { - error!("failed to stat skills root {}: {err:#}", root.display()); - return; + discovery + .warnings + .push(format!("failed to stat skills root {root}: {err:#}")); + return discovery; } } fn enqueue_dir( - queue: &mut VecDeque<(AbsolutePathBuf, usize)>, - visited_dirs: &mut HashSet, + queue: &mut VecDeque<(PathUri, usize)>, + visited_dirs: &mut HashSet, truncated_by_dir_limit: &mut bool, - path: AbsolutePathBuf, + path: PathUri, depth: usize, ) { if depth > MAX_SCAN_DEPTH { @@ -513,24 +533,18 @@ async fn discover_skills_under_root( } } - // Follow symlinked directories for user, admin, and repo skills. System skills are written by Codex itself. - let follow_symlinks = matches!( - scope, - SkillScope::Repo | SkillScope::User | SkillScope::Admin - ); - - let mut visited_dirs: HashSet = HashSet::new(); - visited_dirs.insert(root.clone()); - - let mut queue: VecDeque<(AbsolutePathBuf, usize)> = VecDeque::from([(root.clone(), 0)]); + let follow_symlinks = matches!(symlink_policy, SymlinkPolicy::FollowDirectories); + let mut visited_dirs: HashSet = HashSet::from([root.clone()]); + let mut queue: VecDeque<(PathUri, usize)> = VecDeque::from([(root.clone(), 0)]); let mut truncated_by_dir_limit = false; while let Some((dir, depth)) = queue.pop_front() { - let dir_uri = PathUri::from_abs_path(&dir); - let entries = match fs.read_directory(&dir_uri, /*sandbox*/ None).await { + let entries = match fs.read_directory(&dir, /*sandbox*/ None).await { Ok(entries) => entries, - Err(e) => { - error!("failed to read skills dir {}: {e:#}", dir.display()); + Err(err) => { + discovery + .warnings + .push(format!("failed to read skills directory {dir}: {err:#}")); continue; } }; @@ -542,25 +556,31 @@ async fn discover_skills_under_root( if file_name.starts_with('.') { return None; } - let path = dir.join(&file_name); - let path_uri = PathUri::from_abs_path(&path); - Some((file_name, path, path_uri)) + match dir.join(&file_name) { + Ok(path) => Some((file_name, path)), + Err(err) => { + discovery.warnings.push(format!( + "failed to resolve skill path {dir}/{file_name}: {err}" + )); + None + } + } }) .collect::>(); let metadata_results = join_all( paths .iter() - .map(|(_, _, path_uri)| fs.get_metadata(path_uri, /*sandbox*/ None)), + .map(|(_, path)| fs.get_metadata(path, /*sandbox*/ None)), ) .await; - for ((file_name, path, path_uri), metadata_result) in - paths.into_iter().zip(metadata_results) - { + for ((file_name, path), metadata_result) in paths.into_iter().zip(metadata_results) { let metadata = match metadata_result { Ok(metadata) => metadata, - Err(e) => { - error!("failed to stat skills path {}: {e:#}", path.display()); + Err(err) => { + discovery + .warnings + .push(format!("failed to stat skill path {path}: {err:#}")); continue; } }; @@ -569,9 +589,9 @@ async fn discover_skills_under_root( if !follow_symlinks { continue; } - match fs.read_directory(&path_uri, /*sandbox*/ None).await { + match fs.read_directory(&path, /*sandbox*/ None).await { Ok(_) => { - let resolved_dir = canonicalize_for_skill_identity(fs, &path).await; + let resolved_dir = canonicalize_uri_for_skill_identity(fs, &path).await; enqueue_dir( &mut queue, &mut visited_dirs, @@ -585,51 +605,26 @@ async fn discover_skills_under_root( err.kind(), io::ErrorKind::NotADirectory | io::ErrorKind::NotFound ) => {} - Err(err) => { - error!( - "failed to read skills symlink dir {}: {err:#}", - path.display() - ); - } + Err(err) => discovery.warnings.push(format!( + "failed to read skills symlink directory {path}: {err:#}" + )), } continue; } if metadata.is_directory { - let resolved_dir = canonicalize_for_skill_identity(fs, &path).await; enqueue_dir( &mut queue, &mut visited_dirs, &mut truncated_by_dir_limit, - resolved_dir, + path, depth + 1, ); continue; } if metadata.is_file && file_name == SKILLS_FILENAME { - match parse_skill_file( - fs, - &path, - scope, - plugin_id, - plugin_namespace, - plugin_root.as_ref(), - ) - .await - { - Ok(skill) => { - outcome.skills.push(skill); - } - Err(err) => { - if scope != SkillScope::System { - outcome.errors.push(SkillError { - path: path.clone(), - message: err.to_string(), - }); - } - } - } + discovery.skill_files.push(path); } } } @@ -638,9 +633,72 @@ async fn discover_skills_under_root( tracing::warn!( "skills scan truncated after {} directories (root: {})", MAX_SKILLS_DIRS_PER_ROOT, - root.display() + root ); } + discovery +} + +async fn canonicalize_uri_for_skill_identity( + file_system: &dyn ExecutorFileSystem, + path: &PathUri, +) -> PathUri { + file_system + .canonicalize(path, /*sandbox*/ None) + .await + .unwrap_or_else(|_| path.clone()) +} + +async fn load_skills_under_root( + fs: &dyn ExecutorFileSystem, + root: &AbsolutePathBuf, + scope: SkillScope, + plugin_id: Option<&str>, + plugin_namespace: Option<&str>, + plugin_root: Option<&AbsolutePathBuf>, + outcome: &mut SkillLoadOutcome, +) { + let plugin_root = match plugin_root { + Some(plugin_root) => Some(canonicalize_for_skill_identity(fs, plugin_root).await), + None => None, + }; + let symlink_policy = match scope { + SkillScope::User | SkillScope::Repo | SkillScope::Admin => SymlinkPolicy::FollowDirectories, + SkillScope::System => SymlinkPolicy::Ignore, + }; + let SkillFileDiscovery { + skill_files, + warnings, + } = discover_skills_under_root(fs, &PathUri::from_abs_path(root), symlink_policy).await; + for warning in warnings { + error!("{warning}"); + } + for path_uri in skill_files { + let path = match path_uri.to_abs_path() { + Ok(path) => path, + Err(err) => { + error!("failed to convert discovered skill path {path_uri}: {err}"); + continue; + } + }; + match parse_skill_file( + fs, + &path, + scope, + plugin_id, + plugin_namespace, + plugin_root.as_ref(), + ) + .await + { + Ok(skill) => outcome.skills.push(skill), + Err(err) if scope != SkillScope::System => outcome.errors.push(SkillError { + path, + message: err.to_string(), + }), + Err(_) => {} + } + } } async fn parse_skill_file( @@ -656,8 +714,40 @@ async fn parse_skill_file( .read_file_text(&path_uri, /*sandbox*/ None) .await .map_err(SkillParseError::Read)?; + let ParsedSkillFrontmatter { + name: base_name, + description, + short_description, + } = parse_skill_frontmatter_metadata_inner(&contents, || default_skill_name(path))?; + let name = namespaced_skill_name(fs, path, &base_name, plugin_namespace).await; + let LoadedSkillMetadata { + interface, + dependencies, + policy, + } = load_skill_metadata(fs, path, plugin_root).await; - let frontmatter = extract_frontmatter(&contents).ok_or(SkillParseError::MissingFrontmatter)?; + validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name")?; + + let resolved_path = canonicalize_for_skill_identity(fs, path).await; + + Ok(SkillMetadata { + name, + description, + short_description, + interface, + dependencies, + policy, + path_to_skills_md: resolved_path, + scope, + plugin_id: plugin_id.map(str::to_string), + }) +} + +fn parse_skill_frontmatter_metadata_inner( + contents: &str, + default_name: impl FnOnce() -> String, +) -> Result { + let frontmatter = extract_frontmatter(contents).ok_or(SkillParseError::MissingFrontmatter)?; let parsed: SkillFrontmatter = match serde_yaml::from_str(&frontmatter) { Ok(parsed) => Ok(parsed), @@ -673,13 +763,12 @@ async fn parse_skill_file( } .map_err(SkillParseError::InvalidYaml)?; - let base_name = parsed + let name = parsed .name .as_deref() .map(sanitize_single_line) .filter(|value| !value.is_empty()) - .unwrap_or_else(|| default_skill_name(path)); - let name = namespaced_skill_name(fs, path, &base_name, plugin_namespace).await; + .unwrap_or_else(default_name); let description = parsed .description .as_deref() @@ -691,30 +780,16 @@ async fn parse_skill_file( .as_deref() .map(sanitize_single_line) .filter(|value| !value.is_empty()); - let LoadedSkillMetadata { - interface, - dependencies, - policy, - } = load_skill_metadata(fs, path, plugin_root).await; - validate_len(&base_name, MAX_NAME_LEN, "name")?; - validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name")?; + validate_len(&name, MAX_NAME_LEN, "name")?; if description.is_empty() { return Err(SkillParseError::MissingField("description")); } - let resolved_path = canonicalize_for_skill_identity(fs, path).await; - - Ok(SkillMetadata { + Ok(ParsedSkillFrontmatter { name, description, short_description, - interface, - dependencies, - policy, - path_to_skills_md: resolved_path, - scope, - plugin_id: plugin_id.map(str::to_string), }) } diff --git a/codex-rs/core-skills/src/loader/environment.rs b/codex-rs/core-skills/src/loader/environment.rs new file mode 100644 index 000000000..55fe6e29f --- /dev/null +++ b/codex-rs/core-skills/src/loader/environment.rs @@ -0,0 +1,180 @@ +use std::io; + +use codex_exec_server::ExecutorFileSystem; +use codex_protocol::protocol::Product; +use codex_utils_path_uri::PathUri; +use codex_utils_plugins::plugin_namespace_for_skill_uri; + +use crate::model::SkillDependencies; +use crate::model::SkillPolicy; + +use super::MAX_QUALIFIED_NAME_LEN; +use super::ParsedSkillFrontmatter; +use super::SKILLS_METADATA_DIR; +use super::SKILLS_METADATA_FILENAME; +use super::SkillMetadataFile; +use super::SymlinkPolicy; +use super::discover_skills_under_root; +use super::parse_skill_frontmatter_metadata_inner; +use super::resolve_dependencies; +use super::resolve_policy; +use super::sanitize_single_line; +use super::validate_len; + +/// URI-native metadata for one skill owned by an execution environment. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct EnvironmentSkillMetadata { + pub path_to_skills_md: PathUri, + pub name: String, + pub description: String, + pub short_description: Option, + pub dependencies: Option, + pub policy: Option, +} + +impl EnvironmentSkillMetadata { + pub fn allows_implicit_invocation(&self) -> bool { + self.policy + .as_ref() + .and_then(|policy| policy.allow_implicit_invocation) + .unwrap_or(true) + } + + fn matches_product_restriction(&self, restriction_product: Option) -> bool { + match &self.policy { + Some(policy) => { + policy.products.is_empty() + || restriction_product.is_some_and(|product| { + product.matches_product_restriction(&policy.products) + }) + } + None => true, + } + } + + async fn parse(file_system: &dyn ExecutorFileSystem, path: &PathUri) -> Result { + let contents = file_system + .read_file_text(path, /*sandbox*/ None) + .await + .map_err(|err| format!("failed to read file: {err}"))?; + let ParsedSkillFrontmatter { + name: base_name, + description, + short_description, + } = parse_skill_frontmatter_metadata_inner(&contents, || default_skill_name(path)) + .map_err(|err| err.to_string())?; + let name = plugin_namespace_for_skill_uri(file_system, path) + .await + .map(|namespace| format!("{namespace}:{base_name}")) + .unwrap_or(base_name); + validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name") + .map_err(|err| err.to_string())?; + let (dependencies, policy) = load_skill_metadata(file_system, path).await; + + Ok(Self { + path_to_skills_md: path.clone(), + name, + description, + short_description, + dependencies, + policy, + }) + } +} + +#[derive(Debug, Default)] +pub struct EnvironmentSkillLoadOutcome { + pub skills: Vec, + pub warnings: Vec, +} + +/// Discovers skills without converting environment-owned paths to host paths. +pub async fn load_environment_skills_from_root( + file_system: &dyn ExecutorFileSystem, + root: &PathUri, + restriction_product: Option, +) -> EnvironmentSkillLoadOutcome { + let mut outcome = EnvironmentSkillLoadOutcome::default(); + let discovery = + discover_skills_under_root(file_system, root, SymlinkPolicy::FollowDirectories).await; + outcome.warnings.extend(discovery.warnings); + for path in discovery.skill_files { + match EnvironmentSkillMetadata::parse(file_system, &path).await { + Ok(skill) if skill.matches_product_restriction(restriction_product) => { + outcome.skills.push(skill); + } + Ok(_) => {} + Err(message) => outcome.warnings.push(format!( + "Failed to load environment skill at {path}: {message}" + )), + } + } + outcome.skills.sort_by(|left, right| { + left.name.cmp(&right.name).then_with(|| { + left.path_to_skills_md + .to_string() + .cmp(&right.path_to_skills_md.to_string()) + }) + }); + outcome +} + +async fn load_skill_metadata( + file_system: &dyn ExecutorFileSystem, + skill_path: &PathUri, +) -> (Option, Option) { + let Some(skill_dir) = skill_path.parent() else { + return (None, None); + }; + let Ok(metadata_path) = + skill_dir.join(&format!("{SKILLS_METADATA_DIR}/{SKILLS_METADATA_FILENAME}")) + else { + return (None, None); + }; + match file_system + .get_metadata(&metadata_path, /*sandbox*/ None) + .await + { + Ok(metadata) if metadata.is_file => {} + Ok(_) => return (None, None), + Err(error) if error.kind() == io::ErrorKind::NotFound => return (None, None), + Err(error) => { + tracing::warn!("ignoring {metadata_path}: failed to stat metadata: {error}"); + return (None, None); + } + } + let contents = match file_system + .read_file_text(&metadata_path, /*sandbox*/ None) + .await + { + Ok(contents) => contents, + Err(error) => { + tracing::warn!("ignoring {metadata_path}: failed to read metadata: {error}"); + return (None, None); + } + }; + let parsed: SkillMetadataFile = match serde_yaml::from_str(&contents) { + Ok(parsed) => parsed, + Err(error) => { + tracing::warn!("ignoring {metadata_path}: invalid metadata: {error}"); + return (None, None); + } + }; + + ( + resolve_dependencies(parsed.dependencies), + resolve_policy(parsed.policy), + ) +} + +fn default_skill_name(path: &PathUri) -> String { + path.parent() + .and_then(|parent| parent.basename()) + .map(|name| sanitize_single_line(&name)) + .filter(|name| !name.is_empty()) + .unwrap_or_else(|| "skill".to_string()) +} + +#[cfg(test)] +#[path = "environment_tests.rs"] +mod tests; diff --git a/codex-rs/core-skills/src/loader/environment_tests.rs b/codex-rs/core-skills/src/loader/environment_tests.rs new file mode 100644 index 000000000..a817830b3 --- /dev/null +++ b/codex-rs/core-skills/src/loader/environment_tests.rs @@ -0,0 +1,78 @@ +use std::fs; + +use codex_exec_server::LOCAL_FS; +use codex_protocol::protocol::Product; +use codex_utils_path_uri::PathUri; +use pretty_assertions::assert_eq; +use tempfile::tempdir; + +use crate::model::SkillDependencies; +use crate::model::SkillPolicy; +use crate::model::SkillToolDependency; + +use super::EnvironmentSkillMetadata; +use super::load_environment_skills_from_root; + +#[tokio::test] +async fn loads_plugin_namespace_dependencies_and_policy() { + let root = tempdir().expect("tempdir"); + let skill_dir = root.path().join("skills/deploy"); + fs::create_dir_all(root.path().join(".codex-plugin")).expect("manifest dir"); + fs::create_dir_all(skill_dir.join("agents")).expect("metadata dir"); + fs::write( + root.path().join(".codex-plugin/plugin.json"), + r#"{"name":"demo-plugin"}"#, + ) + .expect("manifest"); + fs::write( + skill_dir.join("SKILL.md"), + "---\nname: deploy\ndescription: Deploy the service.\n---\n", + ) + .expect("skill"); + fs::write( + skill_dir.join("agents/openai.yaml"), + r#" +dependencies: + tools: + - type: mcp + value: deploy-server + description: Deploy MCP +policy: + allow_implicit_invocation: false + products: [codex] +"#, + ) + .expect("metadata"); + + let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI"); + let outcome = + load_environment_skills_from_root(LOCAL_FS.as_ref(), &root_uri, Some(Product::Codex)).await; + + assert_eq!( + outcome.skills, + vec![EnvironmentSkillMetadata { + path_to_skills_md: PathUri::from_host_native_path(skill_dir.join("SKILL.md"),).unwrap(), + name: "demo-plugin:deploy".to_string(), + description: "Deploy the service.".to_string(), + short_description: None, + dependencies: Some(SkillDependencies { + tools: vec![SkillToolDependency { + r#type: "mcp".to_string(), + value: "deploy-server".to_string(), + description: Some("Deploy MCP".to_string()), + transport: None, + command: None, + url: None, + }], + }), + policy: Some(SkillPolicy { + allow_implicit_invocation: Some(false), + products: vec![Product::Codex], + }), + }] + ); + let filtered = + load_environment_skills_from_root(LOCAL_FS.as_ref(), &root_uri, Some(Product::Chatgpt)) + .await; + assert!(filtered.skills.is_empty()); +} diff --git a/codex-rs/ext/goal/tests/goal_extension_backend.rs b/codex-rs/ext/goal/tests/goal_extension_backend.rs index 32043c980..b38bd6393 100644 --- a/codex-rs/ext/goal/tests/goal_extension_backend.rs +++ b/codex-rs/ext/goal/tests/goal_extension_backend.rs @@ -1,3 +1,4 @@ +#![recursion_limit = "256"] #![allow(clippy::expect_used)] use std::sync::Arc; diff --git a/codex-rs/ext/skills/Cargo.toml b/codex-rs/ext/skills/Cargo.toml index afd5111e9..35827ac57 100644 --- a/codex-rs/ext/skills/Cargo.toml +++ b/codex-rs/ext/skills/Cargo.toml @@ -20,7 +20,6 @@ codex-extension-api = { workspace = true } codex-mcp = { workspace = true } codex-protocol = { workspace = true } codex-tools = { workspace = true } -codex-utils-absolute-path = { workspace = true } codex-utils-path-uri = { workspace = true } codex-utils-string = { workspace = true } schemars = { workspace = true } @@ -31,5 +30,6 @@ tracing = { workspace = true } url = { workspace = true } [dev-dependencies] +codex-utils-absolute-path = { workspace = true } pretty_assertions = { workspace = true } tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } diff --git a/codex-rs/ext/skills/src/catalog.rs b/codex-rs/ext/skills/src/catalog.rs index 96b8fec10..59977a47c 100644 --- a/codex-rs/ext/skills/src/catalog.rs +++ b/codex-rs/ext/skills/src/catalog.rs @@ -1,5 +1,5 @@ use codex_core_skills::model::SkillDependencies; -use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; /// Source authority that owns a skill package and must be used to read it. #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -76,7 +76,7 @@ impl SkillResourceId { pub fn environment( id: impl Into, environment_id: impl Into, - path: AbsolutePathBuf, + path: PathUri, ) -> Self { Self { id: id.into(), @@ -91,7 +91,7 @@ impl SkillResourceId { &self.id } - pub(crate) fn environment_path(&self) -> Option<(&str, &AbsolutePathBuf)> { + pub(crate) fn environment_path(&self) -> Option<(&str, &PathUri)> { self.environment_path .as_ref() .map(|resource| (resource.environment_id.as_str(), &resource.path)) @@ -101,7 +101,7 @@ impl SkillResourceId { #[derive(Clone, Debug, PartialEq, Eq, Hash)] struct EnvironmentSkillResource { environment_id: String, - path: AbsolutePathBuf, + path: PathUri, } /// Metadata shown in the always-visible skills catalog. diff --git a/codex-rs/ext/skills/src/provider/executor.rs b/codex-rs/ext/skills/src/provider/executor.rs index 273a1614b..9938035e9 100644 --- a/codex-rs/ext/skills/src/provider/executor.rs +++ b/codex-rs/ext/skills/src/provider/executor.rs @@ -1,15 +1,11 @@ use std::sync::Arc; -use codex_core_skills::SkillMetadata; -use codex_core_skills::filter_skill_load_outcome_for_product; -use codex_core_skills::loader::SkillRoot; -use codex_core_skills::loader::load_skills_from_roots; +use codex_core_skills::loader::EnvironmentSkillMetadata; +use codex_core_skills::loader::load_environment_skills_from_root; use codex_exec_server::EnvironmentManager; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::protocol::Product; -use codex_protocol::protocol::SkillScope; -use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_path_uri::PathUri; +use codex_utils_path_uri::PathConvention; use crate::catalog::SkillAuthority; use crate::catalog::SkillCatalog; @@ -64,42 +60,17 @@ impl SkillProvider for ExecutorSkillProvider { )); continue; }; - let root_path = match executor_absolute_path(&path) { - Ok(root_path) => root_path, - Err(err) => { - catalog.warnings.push(format!( - "Selected capability root `{selected_root_id}` has invalid path `{path}`: {err}" - )); - continue; - } - }; let file_system = environment.get_filesystem(); - let outcome = filter_skill_load_outcome_for_product( - load_skills_from_roots( - [SkillRoot { - path: root_path.clone(), - scope: SkillScope::User, - file_system: Arc::clone(&file_system), - plugin_id: None, - plugin_namespace: None, - plugin_root: None, - }], - /*plugin_skill_snapshots*/ None, - ) - .await, + let outcome = load_environment_skills_from_root( + file_system.as_ref(), + &path, self.restriction_product, - ); - catalog.warnings.extend(outcome.errors.iter().map(|err| { - format!( - "Failed to load executor skill at {}: {}", - err.path.display(), - err.message - ) - })); - for (skill, enabled) in outcome.skills_with_enabled() { + ) + .await; + catalog.warnings.extend(outcome.warnings); + for skill in outcome.skills { catalog.push_entry(catalog_entry_from_skill( - skill, - enabled, + &skill, authority.clone(), &selected_root_id, &environment_id, @@ -134,10 +105,9 @@ impl SkillProvider for ExecutorSkillProvider { "executor skill resource references unavailable environment `{environment_id}`" ))); }; - let resource_path = PathUri::from_abs_path(resource_path); let contents = environment .get_filesystem() - .read_file_text(&resource_path, /*sandbox*/ None) + .read_file_text(resource_path, /*sandbox*/ None) .await .map_err(|err| { SkillProviderError::new(format!( @@ -159,19 +129,21 @@ impl SkillProvider for ExecutorSkillProvider { } fn catalog_entry_from_skill( - skill: &SkillMetadata, - enabled: bool, + skill: &EnvironmentSkillMetadata, authority: SkillAuthority, selected_root_id: &str, environment_id: &str, ) -> SkillCatalogEntry { - let skill_path = skill.path_to_skills_md.to_string_lossy().into_owned(); - let normalized_path = skill_path.replace('\\', "/"); + let skill_path = skill.path_to_skills_md.inferred_native_path_string(); + let normalized_path = match skill.path_to_skills_md.infer_path_convention() { + Some(PathConvention::Windows) => skill_path.replace('\\', "/"), + Some(PathConvention::Posix) | None => skill_path, + }; let display_path = format!( "skill://{selected_root_id}/{}", normalized_path.trim_start_matches('/') ); - let mut entry = SkillCatalogEntry::new( + let entry = SkillCatalogEntry::new( SkillPackageId(display_path.clone()), authority, skill.name.clone(), @@ -186,16 +158,9 @@ fn catalog_entry_from_skill( .with_display_path(display_path) .with_dependencies(skill.dependencies.clone()); - if !enabled { - entry = entry.disabled(); + if skill.allows_implicit_invocation() { + entry + } else { + entry.hidden_from_prompt() } - if !skill.allows_implicit_invocation() { - entry = entry.hidden_from_prompt(); - } - - entry -} - -fn executor_absolute_path(path: &PathUri) -> std::io::Result { - path.to_abs_path() } diff --git a/codex-rs/ext/skills/tests/executor_file_system_authority.rs b/codex-rs/ext/skills/tests/executor_file_system_authority.rs index 8c5160e2c..06bf062ea 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -28,40 +28,48 @@ use pretty_assertions::assert_eq; const SKILL_CONTENTS: &str = "---\nname: synthetic\ndescription: Synthetic executor skill.\n---\n\nEXECUTOR_ONLY_BODY\n"; +const PLUGIN_MANIFEST: &str = r#"{"name":"synthetic-plugin"}"#; static NEXT_TEST_ROOT_ID: AtomicUsize = AtomicUsize::new(0); struct SyntheticFileSystem { - alias_root: AbsolutePathBuf, - canonical_root: AbsolutePathBuf, + alias_root: PathUri, + canonical_root: PathUri, + has_plugin_manifest: bool, } impl SyntheticFileSystem { + fn path(&self, relative_path: &str) -> io::Result { + self.canonical_root + .join(relative_path) + .map_err(io::Error::other) + } + async fn canonicalize(&self, path: &PathUri) -> io::Result { - let path = path.to_abs_path()?; - if path == self.alias_root { - return Ok(PathUri::from_abs_path(&self.canonical_root)); + if path == &self.alias_root { + return Ok(self.canonical_root.clone()); } - self.metadata(&path)?; - Ok(PathUri::from_abs_path(&path)) + self.metadata(path)?; + Ok(path.clone()) } async fn read_file(&self, path: &PathUri) -> io::Result> { - if path.to_abs_path()? == self.canonical_root.join("skill/SKILL.md") { + if path == &self.path("skill/SKILL.md")? { Ok(SKILL_CONTENTS.as_bytes().to_vec()) + } else if self.has_plugin_manifest && path == &self.path(".claude-plugin/plugin.json")? { + Ok(PLUGIN_MANIFEST.as_bytes().to_vec()) } else { Err(io::Error::new(io::ErrorKind::NotFound, "not found")) } } async fn read_directory(&self, path: &PathUri) -> io::Result> { - let path = path.to_abs_path()?; - if path == self.canonical_root { + if path == &self.canonical_root { Ok(vec![ReadDirectoryEntry { file_name: "skill".to_string(), is_directory: true, is_file: false, }]) - } else if path == self.canonical_root.join("skill") { + } else if path == &self.path("skill")? { Ok(vec![ReadDirectoryEntry { file_name: "SKILL.md".to_string(), is_directory: false, @@ -72,12 +80,13 @@ impl SyntheticFileSystem { } } - fn metadata(&self, path: &AbsolutePathBuf) -> io::Result { - let skill_dir = self.canonical_root.join("skill"); - let skill_path = skill_dir.join("SKILL.md"); + fn metadata(&self, path: &PathUri) -> io::Result { + let skill_dir = self.path("skill")?; + let skill_path = self.path("skill/SKILL.md")?; + let manifest_path = self.path(".claude-plugin/plugin.json")?; let (is_directory, is_file) = if path == &self.canonical_root || path == &skill_dir { (true, false) - } else if path == &skill_path { + } else if path == &skill_path || self.has_plugin_manifest && path == &manifest_path { (false, true) } else { return Err(io::Error::new(io::ErrorKind::NotFound, "not found")); @@ -146,7 +155,7 @@ impl ExecutorFileSystem for SyntheticFileSystem { path: &'a PathUri, _sandbox: Option<&'a FileSystemSandboxContext>, ) -> ExecutorFileSystemFuture<'a, FileMetadata> { - Box::pin(async move { self.metadata(&path.to_abs_path()?) }) + Box::pin(async move { self.metadata(path) }) } fn read_directory<'a>( @@ -193,8 +202,9 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() { path: alias_root.clone(), scope: SkillScope::User, file_system: Arc::new(SyntheticFileSystem { - alias_root, - canonical_root: canonical_root.clone(), + alias_root: PathUri::from_abs_path(&alias_root), + canonical_root: PathUri::from_abs_path(&canonical_root), + has_plugin_manifest: false, }), plugin_id: None, plugin_namespace: None, @@ -221,13 +231,18 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() { #[tokio::test] async fn selected_root_id_distinguishes_identical_executor_paths() { - let test_root = create_local_skill_root("root-identity").expect("create local skill root"); - let canonical_root = AbsolutePathBuf::from_absolute_path_checked(&test_root) - .expect("absolute skill root") - .canonicalize() - .expect("canonicalize skill root") - .to_string_lossy() - .replace('\\', "/"); + let root_label = if cfg!(unix) { + r"root\identity" + } else { + "root-identity" + }; + let test_root = create_local_skill_root(root_label).expect("create local skill root"); + let selected_root = test_root.to_string_lossy().into_owned(); + let selected_root = if cfg!(windows) { + selected_root.replace('\\', "/") + } else { + selected_root + }; let provider = ExecutorSkillProvider::new_with_restriction_product( Arc::new(EnvironmentManager::default_for_tests()), /*restriction_product*/ None, @@ -268,14 +283,14 @@ async fn selected_root_id_distinguishes_identical_executor_paths() { "root-a".to_string(), format!( "skill://root-a/{}/skill/SKILL.md", - canonical_root.trim_start_matches('/') + selected_root.trim_start_matches('/') ), ), ( "root-b".to_string(), format!( "skill://root-b/{}/skill/SKILL.md", - canonical_root.trim_start_matches('/') + selected_root.trim_start_matches('/') ), ), ] diff --git a/codex-rs/utils/plugins/src/lib.rs b/codex-rs/utils/plugins/src/lib.rs index 203b1f22e..70e4dcdb8 100644 --- a/codex-rs/utils/plugins/src/lib.rs +++ b/codex-rs/utils/plugins/src/lib.rs @@ -10,6 +10,7 @@ pub mod plugin_namespace; pub use plugin_namespace::DISCOVERABLE_PLUGIN_MANIFEST_PATHS; pub use plugin_namespace::find_plugin_manifest_path; pub use plugin_namespace::plugin_namespace_for_skill_path; +pub use plugin_namespace::plugin_namespace_for_skill_uri; #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct PluginSkillRoot { diff --git a/codex-rs/utils/plugins/src/plugin_namespace.rs b/codex-rs/utils/plugins/src/plugin_namespace.rs index b29ff5c5d..085c4e1db 100644 --- a/codex-rs/utils/plugins/src/plugin_namespace.rs +++ b/codex-rs/utils/plugins/src/plugin_namespace.rs @@ -26,13 +26,12 @@ struct RawPluginManifestName { async fn plugin_manifest_name( fs: &dyn ExecutorFileSystem, - plugin_root: &AbsolutePathBuf, + plugin_root: &PathUri, ) -> Option { let mut manifest_path = None; for relative_path in DISCOVERABLE_PLUGIN_MANIFEST_PATHS { - let candidate = plugin_root.join(relative_path); - let candidate_uri = PathUri::from_abs_path(&candidate); - match fs.get_metadata(&candidate_uri, /*sandbox*/ None).await { + let candidate = plugin_root.join(relative_path).ok()?; + match fs.get_metadata(&candidate, /*sandbox*/ None).await { Ok(metadata) if metadata.is_file => { manifest_path = Some(candidate); break; @@ -40,20 +39,16 @@ async fn plugin_manifest_name( Ok(_) | Err(_) => {} } } - let manifest_path = manifest_path?; - let manifest_path_uri = PathUri::from_abs_path(&manifest_path); let contents = fs - .read_file_text(&manifest_path_uri, /*sandbox*/ None) + .read_file_text(&manifest_path?, /*sandbox*/ None) .await .ok()?; let RawPluginManifestName { name: raw_name } = serde_json::from_str(&contents).ok()?; Some( plugin_root - .file_name() - .and_then(|entry| entry.to_str()) + .basename() .filter(|_| raw_name.trim().is_empty()) - .unwrap_or(raw_name.as_str()) - .to_string(), + .unwrap_or(raw_name), ) } @@ -63,10 +58,20 @@ pub async fn plugin_namespace_for_skill_path( fs: &dyn ExecutorFileSystem, path: &AbsolutePathBuf, ) -> Option { - for ancestor in path.ancestors() { - if let Some(name) = plugin_manifest_name(fs, &ancestor).await { + plugin_namespace_for_skill_uri(fs, &PathUri::from_abs_path(path)).await +} + +/// Returns the plugin manifest `name` for the nearest URI ancestor of `path`. +pub async fn plugin_namespace_for_skill_uri( + fs: &dyn ExecutorFileSystem, + path: &PathUri, +) -> Option { + let mut ancestor = Some(path.clone()); + while let Some(path) = ancestor { + if let Some(name) = plugin_manifest_name(fs, &path).await { return Some(name); } + ancestor = path.parent(); } None }