diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index cfad1bd94a..c0457af4ac 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4087,6 +4087,9 @@ dependencies = [ "codex-utils-absolute-path", "codex-utils-path-uri", "include_dir", + "pretty_assertions", + "serde", + "serde_yaml", "thiserror 2.0.18", ] @@ -4104,14 +4107,18 @@ dependencies = [ "codex-skills", "codex-tools", "codex-utils-absolute-path", + "codex-utils-cargo-bin", "codex-utils-path-uri", "codex-utils-string", "futures", + "insta", "opentelemetry_sdk", "pretty_assertions", "schemars 0.8.22", "serde", "serde_json", + "serde_yaml", + "tempfile", "tokio", "tracing", "url", diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 18a80f7d7b..2651c6445a 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -4,9 +4,6 @@ mod namespace; pub use environment::EnvironmentSkillLoadOutcome; pub use environment::EnvironmentSkillMetadata; -pub use environment::EnvironmentSkillSnapshot; -pub use environment::EnvironmentSkillSnapshotOutcome; -pub use environment::load_environment_skills_from_discovery; pub use environment::load_environment_skills_from_root; use crate::model::SkillDependencies; @@ -26,6 +23,9 @@ use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; +use codex_skills::ParsedSkillFrontmatter; +use codex_skills::SkillParseError; +use codex_skills::parse_skill_frontmatter_metadata; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use codex_utils_path_uri::PathUri; @@ -60,22 +60,6 @@ use tracing::error; // TODO(anp): Tune this eight-scan limit after revisiting byte-based backpressure. pub const MAX_CONCURRENT_ROOT_SCANS: usize = 8; -#[derive(Debug, Deserialize)] -struct SkillFrontmatter { - #[serde(default)] - name: Option, - #[serde(default)] - description: Option, - #[serde(default)] - metadata: SkillFrontmatterMetadata, -} - -#[derive(Debug, Default, Deserialize)] -struct SkillFrontmatterMetadata { - #[serde(default, rename = "short-description")] - short_description: Option, -} - #[derive(Debug, Default, Deserialize)] struct SkillMetadataFile { #[serde(default)] @@ -128,13 +112,6 @@ 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"; @@ -165,31 +142,27 @@ struct ResolvedDiscoveredSkill { } #[derive(Debug)] -enum SkillParseError { +enum SkillLoadError { Read(std::io::Error), - MissingFrontmatter, - InvalidYaml(serde_yaml::Error), - MissingField(&'static str), - InvalidField { field: &'static str, reason: String }, + Parse(SkillParseError), } -impl fmt::Display for SkillParseError { +impl fmt::Display for SkillLoadError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - SkillParseError::Read(e) => write!(f, "failed to read file: {e}"), - SkillParseError::MissingFrontmatter => { - write!(f, "missing YAML frontmatter delimited by ---") - } - SkillParseError::InvalidYaml(e) => write!(f, "invalid YAML: {e}"), - SkillParseError::MissingField(field) => write!(f, "missing field `{field}`"), - SkillParseError::InvalidField { field, reason } => { - write!(f, "invalid {field}: {reason}") - } + Self::Read(error) => write!(f, "failed to read file: {error}"), + Self::Parse(error) => write!(f, "{error}"), } } } -impl Error for SkillParseError {} +impl Error for SkillLoadError {} + +impl From for SkillLoadError { + fn from(error: SkillParseError) -> Self { + Self::Parse(error) + } +} pub struct SkillRoot { pub path: AbsolutePathBuf, @@ -693,7 +666,7 @@ async fn parse_skill_file( scope: SkillScope, plugin_identity: Option<&PluginIdentity>, plugin_root: Option<&AbsolutePathBuf>, -) -> Result { +) -> Result { let metadata_path = path_uri .parent() .and_then(|parent| parent.join(SKILLS_METADATA_DIR).ok()) @@ -708,12 +681,12 @@ async fn parse_skill_file( fs.read_file_text(path_uri, /*sandbox*/ None), load_skill_metadata(fs, path, &metadata, plugin_root), ); - let contents = contents.map_err(SkillParseError::Read)?; + let contents = contents.map_err(SkillLoadError::Read)?; let ParsedSkillFrontmatter { name: base_name, description, short_description, - } = parse_skill_frontmatter_metadata_inner(&contents, || default_skill_name(path))?; + } = parse_skill_frontmatter_metadata(&contents, || default_skill_name(path))?; let LoadedSkillMetadata { interface, dependencies, @@ -734,56 +707,6 @@ async fn parse_skill_file( }) } -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), - Err(original_error) => match repair_frontmatter_scalar_fields(&frontmatter) { - // Some third-party skills use prose like `description: Build for AWS: ECS` - // or `argument-hint: `. Keep the repair line-oriented - // so unrelated invalid YAML still surfaces. - Some(repaired_frontmatter) => { - serde_yaml::from_str(&repaired_frontmatter).map_err(|_| original_error) - } - None => Err(original_error), - }, - } - .map_err(SkillParseError::InvalidYaml)?; - - let name = parsed - .name - .as_deref() - .map(sanitize_single_line) - .filter(|value| !value.is_empty()) - .unwrap_or_else(default_name); - let description = parsed - .description - .as_deref() - .map(sanitize_single_line) - .unwrap_or_default(); - let short_description = parsed - .metadata - .short_description - .as_deref() - .map(sanitize_single_line) - .filter(|value| !value.is_empty()); - - validate_len(&name, MAX_NAME_LEN, "name")?; - if description.is_empty() { - return Err(SkillParseError::MissingField("description")); - } - - Ok(ParsedSkillFrontmatter { - name, - description, - short_description, - }) -} - fn default_skill_name(path: &AbsolutePathBuf) -> String { path.parent() .and_then(|parent| { @@ -1067,91 +990,6 @@ fn sanitize_single_line(raw: &str) -> String { raw.split_whitespace().collect::>().join(" ") } -fn repair_frontmatter_scalar_fields(frontmatter: &str) -> Option { - let mut changed = false; - let mut block_scalar_indent: Option = None; - let mut repaired_lines: Vec = Vec::new(); - for line in frontmatter.lines() { - let indent = line - .chars() - .take_while(|character| *character == ' ') - .count(); - if let Some(block_indent) = block_scalar_indent { - if line.trim().is_empty() || indent > block_indent { - repaired_lines.push(line.to_string()); - continue; - } - block_scalar_indent = None; - } - - let Some((key, value)) = line.split_once(':') else { - repaired_lines.push(line.to_string()); - continue; - }; - if key.trim().is_empty() || !value.chars().next().is_none_or(char::is_whitespace) { - repaired_lines.push(line.to_string()); - continue; - } - - let trimmed_start = value.trim_start(); - let leading_whitespace = &value[..value.len() - trimmed_start.len()]; - let mut scalar = trimmed_start; - let mut comment = ""; - for (index, character) in trimmed_start.char_indices() { - if character == '#' - && (index == 0 - || trimmed_start[..index] - .chars() - .next_back() - .is_some_and(char::is_whitespace)) - { - let comment_start = trimmed_start[..index].trim_end().len(); - scalar = &trimmed_start[..comment_start]; - comment = &trimmed_start[comment_start..]; - break; - } - } - - let scalar = scalar.trim_end(); - let Some(first_char) = scalar.chars().next() else { - repaired_lines.push(line.to_string()); - continue; - }; - if matches!(first_char, '|' | '>') { - block_scalar_indent = Some(indent); - repaired_lines.push(line.to_string()); - continue; - } - if matches!(first_char, '\'' | '"') { - repaired_lines.push(line.to_string()); - continue; - } - let mut has_colon_separator = false; - let mut chars = scalar.chars().peekable(); - while let Some(character) = chars.next() { - if character == ':' - && matches!(chars.peek(), Some(next_character) if next_character.is_whitespace()) - { - has_colon_separator = true; - break; - } - } - let invalid_flow_like_scalar = matches!(first_char, '[' | '{' | '@' | '`') - && serde_yaml::from_str::(scalar).is_err(); - if !has_colon_separator && !invalid_flow_like_scalar { - repaired_lines.push(line.to_string()); - continue; - } - - let quoted_scalar = format!("'{}'", scalar.replace('\'', "''")); - repaired_lines.push(format!( - "{key}:{leading_whitespace}{quoted_scalar}{comment}" - )); - changed = true; - } - changed.then(|| repaired_lines.join("\n")) -} - fn validate_len( value: &str, max_len: usize, @@ -1211,28 +1049,6 @@ fn resolve_color_str(value: Option, field: &'static str) -> Option Option { - let mut lines = contents.lines(); - if !matches!(lines.next(), Some(line) if line.trim() == "---") { - return None; - } - - let mut frontmatter_lines: Vec<&str> = Vec::new(); - let mut found_closing = false; - for line in lines.by_ref() { - if line.trim() == "---" { - found_closing = true; - break; - } - frontmatter_lines.push(line); - } - - if frontmatter_lines.is_empty() || !found_closing { - return None; - } - - Some(frontmatter_lines.join("\n")) -} #[cfg(test)] pub(crate) async fn skill_roots_from_layer_stack( fs: Arc, diff --git a/codex-rs/core-skills/src/loader/environment.rs b/codex-rs/core-skills/src/loader/environment.rs index 0e881b6c24..fa950ec7b1 100644 --- a/codex-rs/core-skills/src/loader/environment.rs +++ b/codex-rs/core-skills/src/loader/environment.rs @@ -1,10 +1,10 @@ -use std::collections::HashMap; use std::io; -use codex_exec_server::CapabilityRootDiscovery; use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; pub use codex_skills::EnvironmentSkillMetadata; +use codex_skills::ParsedSkillFrontmatter; +use codex_skills::parse_skill_frontmatter_metadata; use codex_utils_path_uri::PathUri; use futures::StreamExt; @@ -12,7 +12,6 @@ use crate::model::SkillDependencies; use crate::model::SkillPolicy; use super::MAX_QUALIFIED_NAME_LEN; -use super::ParsedSkillFrontmatter; use super::SkillMetadataFile; use super::discovery::DirectorySymlinkPolicy; use super::discovery::DiscoveredSkill; @@ -22,7 +21,6 @@ use super::discovery::SkillDiscoveryOptions; use super::discovery::SkillMetadataDiscovery; use super::discovery::discover_skills; use super::namespace::SkillNamespaceResolver; -use super::parse_skill_frontmatter_metadata_inner; use super::resolve_dependencies; use super::resolve_policy; use super::sanitize_single_line; @@ -37,19 +35,6 @@ struct ParsedEnvironmentSkill { policy: Option, } -/// Parsed executor skill plus the instructions already materialized by capability discovery. -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct EnvironmentSkillSnapshot { - pub metadata: EnvironmentSkillMetadata, - pub instructions: String, -} - -#[derive(Debug, Default)] -pub struct EnvironmentSkillSnapshotOutcome { - pub skills: Vec, - pub warnings: Vec, -} - impl ParsedEnvironmentSkill { async fn load( file_system: &dyn ExecutorFileSystem, @@ -72,7 +57,7 @@ impl ParsedEnvironmentSkill { name: base_name, description, short_description, - } = parse_skill_frontmatter_metadata_inner(&contents, || default_skill_name(&skill.path)) + } = parse_skill_frontmatter_metadata(&contents, || default_skill_name(&skill.path)) .map_err(|err| err.to_string())?; let (dependencies, policy) = match &skill.metadata { SkillMetadataDiscovery::Present(_) | SkillMetadataDiscovery::Absent => { @@ -197,130 +182,6 @@ pub async fn load_environment_skills_from_root( outcome } -/// Parses an executor-produced manifest bundle without issuing additional filesystem requests. -pub fn load_environment_skills_from_discovery( - discovery: &CapabilityRootDiscovery, - restriction_product: Option, -) -> EnvironmentSkillSnapshotOutcome { - let mut outcome = EnvironmentSkillSnapshotOutcome { - warnings: discovery.warnings.clone(), - ..Default::default() - }; - if let Some(error) = &discovery.error { - outcome.warnings.push(error.clone()); - return outcome; - } - - let mut plugin_namespaces = HashMap::new(); - for (plugin_root, name) in discovery.namespace_manifests.iter().filter_map(|manifest| { - #[derive(serde::Deserialize)] - struct ManifestName { - #[serde(default)] - name: String, - } - - let plugin_root = manifest.path.parent()?.parent()?; - let parsed = serde_json::from_str::(&manifest.contents).ok()?; - let name = if parsed.name.trim().is_empty() { - plugin_root.basename()? - } else { - parsed.name - }; - Some((plugin_root, name)) - }) { - // Exec-server orders manifests by the same precedence as local discovery. Preserve the - // first manifest if an older or alternate server returns duplicates for one plugin root. - plugin_namespaces.entry(plugin_root).or_insert(name); - } - - for skill in &discovery.skills { - let plugin_namespace = - nearest_plugin_namespace(&skill.instructions.path, &plugin_namespaces); - let ParsedSkillFrontmatter { - name: base_name, - description, - short_description, - } = match parse_skill_frontmatter_metadata_inner(&skill.instructions.contents, || { - default_skill_name(&skill.instructions.path) - }) { - Ok(frontmatter) => frontmatter, - Err(error) => { - outcome.warnings.push(format!( - "Failed to load environment skill at {}: {error}", - skill.instructions.path - )); - continue; - } - }; - let name = plugin_namespace - .map(|namespace| format!("{namespace}:{base_name}")) - .unwrap_or(base_name); - if let Err(error) = validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name") { - outcome.warnings.push(format!( - "Failed to load environment skill at {}: {error}", - skill.instructions.path - )); - continue; - } - let (dependencies, policy) = skill - .metadata - .as_ref() - .and_then(|metadata| { - serde_yaml::from_str::(&metadata.contents) - .map_err(|error| { - tracing::warn!( - path = %metadata.path, - "ignoring invalid discovered skill metadata: {error}" - ); - }) - .ok() - }) - .map(|metadata| { - ( - resolve_dependencies(metadata.dependencies), - resolve_policy(metadata.policy), - ) - }) - .unwrap_or((None, None)); - let metadata = EnvironmentSkillMetadata { - path_to_skills_md: skill.instructions.path.clone(), - name, - description, - short_description, - dependencies, - policy, - }; - if metadata.matches_product_restriction(restriction_product) { - outcome.skills.push(EnvironmentSkillSnapshot { - metadata, - instructions: skill.instructions.contents.clone(), - }); - } - } - outcome.skills.sort_by(|left, right| { - left.metadata.name.cmp(&right.metadata.name).then_with(|| { - left.metadata - .path_to_skills_md - .to_string() - .cmp(&right.metadata.path_to_skills_md.to_string()) - }) - }); - outcome -} - -fn nearest_plugin_namespace<'a>( - skill_path: &PathUri, - plugin_namespaces: &'a HashMap, -) -> Option<&'a str> { - let mut ancestor = skill_path.parent(); - while let Some(path) = ancestor { - if let Some(namespace) = plugin_namespaces.get(&path) { - return Some(namespace); - } - ancestor = path.parent(); - } - None -} async fn read_skill_contents( file_system: &dyn ExecutorFileSystem, skill_path: &PathUri, diff --git a/codex-rs/core-skills/tests/environment_loader.rs b/codex-rs/core-skills/tests/environment_loader.rs index 88b26f2349..0150322703 100644 --- a/codex-rs/core-skills/tests/environment_loader.rs +++ b/codex-rs/core-skills/tests/environment_loader.rs @@ -6,10 +6,7 @@ use std::sync::atomic::Ordering; use std::time::Duration; use codex_core_skills::loader::EnvironmentSkillMetadata; -use codex_core_skills::loader::load_environment_skills_from_discovery; use codex_core_skills::loader::load_environment_skills_from_root; -use codex_exec_server::CapabilityRootDiscoverRequest; -use codex_exec_server::CapabilityRootsDiscoverParams; use codex_exec_server::CopyOptions; use codex_exec_server::CreateDirectoryOptions; use codex_exec_server::ExecutorFileSystem; @@ -22,7 +19,6 @@ use codex_exec_server::ReadDirectoryEntry; use codex_exec_server::RemoveOptions; use codex_exec_server::WalkOptions; use codex_exec_server::WalkOutcome; -use codex_exec_server::discover_capability_roots; use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -547,142 +543,3 @@ async fn host_loading_reuses_walk_inventory_for_symlinked_skill_pack() { 1 ); } - -#[tokio::test] -async fn executor_bundle_parser_matches_the_existing_environment_loader() { - let root = tempdir().expect("tempdir"); - let plugin_manifest = root.path().join(".codex-plugin/plugin.json"); - let nested_manifest = root.path().join("nested/.claude-plugin/plugin.json"); - let deploy_skill = root.path().join("skills/deploy/SKILL.md"); - let deploy_metadata = root.path().join("skills/deploy/agents/openai.yaml"); - let audit_skill = root.path().join("nested/skills/audit/SKILL.md"); - for (path, contents) in [ - (&plugin_manifest, r#"{"name":"demo"}"#), - (&nested_manifest, r#"{"name":"nested"}"#), - ( - &deploy_skill, - "---\nname: deploy\ndescription: Deploy the service.\n---\n\nDeploy.\n", - ), - ( - &deploy_metadata, - "policy:\n allow_implicit_invocation: false\n", - ), - ( - &audit_skill, - "---\nname: audit\ndescription: Audit the service.\n---\n\nAudit.\n", - ), - ] { - fs::create_dir_all(path.parent().expect("test file parent")).expect("test directory"); - fs::write(path, contents).expect("test file"); - } - - let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI"); - let existing = load_environment_skills_from_root( - LOCAL_FS.as_ref(), - &root_uri, - /*restriction_product*/ None, - ) - .await; - let response = discover_capability_roots( - LOCAL_FS.as_ref(), - CapabilityRootsDiscoverParams { - roots: vec![CapabilityRootDiscoverRequest { - id: "demo@1".to_string(), - path: root_uri, - sandbox: None, - }], - }, - ) - .await - .expect("capability discovery"); - let bundled = load_environment_skills_from_discovery( - response.roots.first().expect("discovered root"), - /*restriction_product*/ None, - ); - - assert_eq!(bundled.warnings, existing.warnings); - assert_eq!( - bundled - .skills - .iter() - .map(|skill| skill.metadata.clone()) - .collect::>(), - existing.skills - ); - assert_eq!( - bundled - .skills - .iter() - .map(|skill| skill.instructions.as_str()) - .collect::>(), - vec![ - "---\nname: deploy\ndescription: Deploy the service.\n---\n\nDeploy.\n", - "---\nname: audit\ndescription: Audit the service.\n---\n\nAudit.\n", - ] - ); -} - -#[tokio::test] -async fn executor_bundle_preserves_parent_namespace_and_manifest_precedence() { - let plugin = tempdir().expect("tempdir"); - for (relative_path, name) in [ - (".codex-plugin/plugin.json", "codex-name"), - (".claude-plugin/plugin.json", "claude-name"), - (".cursor-plugin/plugin.json", "cursor-name"), - ] { - let manifest = plugin.path().join(relative_path); - fs::create_dir_all(manifest.parent().expect("manifest parent")) - .expect("manifest directory"); - fs::write(&manifest, format!(r#"{{"name":"{name}"}}"#)).expect("manifest"); - } - let skills_root = plugin.path().join("skills"); - let skill_path = skills_root.join("search/SKILL.md"); - fs::create_dir_all(skill_path.parent().expect("skill parent")).expect("skill directory"); - fs::write( - &skill_path, - "---\nname: search\ndescription: Search the project.\n---\n\nSearch.\n", - ) - .expect("skill"); - - 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(), - &root_uri, - /*restriction_product*/ None, - ) - .await; - let response = discover_capability_roots( - LOCAL_FS.as_ref(), - CapabilityRootsDiscoverParams { - roots: vec![CapabilityRootDiscoverRequest { - id: "skills-only".to_string(), - path: root_uri, - sandbox: None, - }], - }, - ) - .await - .expect("capability discovery"); - let discovery = response.roots.first().expect("discovered root"); - let bundled = - load_environment_skills_from_discovery(discovery, /*restriction_product*/ None); - - assert_eq!(discovery.plugin, None); - assert_eq!(discovery.namespace_manifests.len(), 1); - assert!( - discovery.namespace_manifests[0] - .path - .to_string() - .ends_with("/.codex-plugin/plugin.json") - ); - assert_eq!(bundled.warnings, existing.warnings); - assert_eq!( - bundled - .skills - .iter() - .map(|skill| skill.metadata.clone()) - .collect::>(), - existing.skills - ); - assert_eq!(bundled.skills[0].metadata.name, "codex-name:search"); -} diff --git a/codex-rs/ext/skills/Cargo.toml b/codex-rs/ext/skills/Cargo.toml index 43518bd826..692f6ba4e2 100644 --- a/codex-rs/ext/skills/Cargo.toml +++ b/codex-rs/ext/skills/Cargo.toml @@ -27,6 +27,7 @@ futures = { workspace = true } schemars = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +serde_yaml = { workspace = true } tokio = { workspace = true, features = ["sync", "time"] } tracing = { workspace = true } url = { workspace = true } @@ -34,6 +35,9 @@ url = { workspace = true } [dev-dependencies] codex-models-manager = { workspace = true } codex-utils-absolute-path = { workspace = true } +codex-utils-cargo-bin = { workspace = true } +insta = { workspace = true } opentelemetry_sdk = { workspace = true } pretty_assertions = { workspace = true } +tempfile = { workspace = true } tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index 3893fd03ed..4b4c4e57ce 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -4,6 +4,7 @@ mod config; mod dynamic_skill_selector; mod extension; mod fragments; +mod loader; pub mod provider; mod render; mod render_observability; diff --git a/codex-rs/ext/skills/src/loader/environment.rs b/codex-rs/ext/skills/src/loader/environment.rs new file mode 100644 index 0000000000..ce7624bdd5 --- /dev/null +++ b/codex-rs/ext/skills/src/loader/environment.rs @@ -0,0 +1,165 @@ +use std::collections::HashMap; + +use codex_exec_server::CapabilityRootDiscovery; +use codex_protocol::protocol::Product; +use codex_skills::EnvironmentSkillMetadata; +use codex_skills::ParsedSkillFrontmatter; +use codex_skills::parse_skill_frontmatter_metadata; +use codex_utils_path_uri::PathUri; + +use super::MAX_QUALIFIED_NAME_LEN; +use super::metadata::SkillMetadataFile; +use super::metadata::resolve_dependencies; +use super::metadata::resolve_policy; +use super::metadata::sanitize_single_line; +use super::metadata::validate_len; + +/// Parsed executor skill plus the instructions already materialized by capability discovery. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct EnvironmentSkillSnapshot { + pub metadata: EnvironmentSkillMetadata, + pub instructions: String, +} + +#[derive(Debug, Default)] +pub struct EnvironmentSkillSnapshotOutcome { + pub skills: Vec, + pub warnings: Vec, +} + +/// Parses an executor-produced manifest bundle without issuing additional filesystem requests. +pub fn load_environment_skills_from_discovery( + discovery: &CapabilityRootDiscovery, + restriction_product: Option, +) -> EnvironmentSkillSnapshotOutcome { + let mut outcome = EnvironmentSkillSnapshotOutcome { + warnings: discovery.warnings.clone(), + ..Default::default() + }; + if let Some(error) = &discovery.error { + outcome.warnings.push(error.clone()); + return outcome; + } + + let mut plugin_namespaces = HashMap::new(); + for (plugin_root, name) in discovery.namespace_manifests.iter().filter_map(|manifest| { + #[derive(serde::Deserialize)] + struct ManifestName { + #[serde(default)] + name: String, + } + + let plugin_root = manifest.path.parent()?.parent()?; + let parsed = serde_json::from_str::(&manifest.contents).ok()?; + let name = if parsed.name.trim().is_empty() { + plugin_root.basename()? + } else { + parsed.name + }; + Some((plugin_root, name)) + }) { + // Exec-server orders manifests by the same precedence as local discovery. Preserve the + // first manifest if an older or alternate server returns duplicates for one plugin root. + plugin_namespaces.entry(plugin_root).or_insert(name); + } + + for skill in &discovery.skills { + let plugin_namespace = + nearest_plugin_namespace(&skill.instructions.path, &plugin_namespaces); + let ParsedSkillFrontmatter { + name: base_name, + description, + short_description, + } = match parse_skill_frontmatter_metadata(&skill.instructions.contents, || { + default_skill_name(&skill.instructions.path) + }) { + Ok(frontmatter) => frontmatter, + Err(error) => { + outcome.warnings.push(format!( + "Failed to load environment skill at {}: {error}", + skill.instructions.path + )); + continue; + } + }; + let name = plugin_namespace + .map(|namespace| format!("{namespace}:{base_name}")) + .unwrap_or(base_name); + if let Err(error) = validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name") { + outcome.warnings.push(format!( + "Failed to load environment skill at {}: {error}", + skill.instructions.path + )); + continue; + } + let (dependencies, policy) = skill + .metadata + .as_ref() + .and_then(|metadata| { + serde_yaml::from_str::(&metadata.contents) + .map_err(|error| { + tracing::warn!( + path = %metadata.path, + "ignoring invalid discovered skill metadata: {error}" + ); + }) + .ok() + }) + .map(|metadata| { + ( + resolve_dependencies(metadata.dependencies), + resolve_policy(metadata.policy), + ) + }) + .unwrap_or((None, None)); + let metadata = EnvironmentSkillMetadata { + path_to_skills_md: skill.instructions.path.clone(), + name, + description, + short_description, + dependencies, + policy, + }; + if metadata.matches_product_restriction(restriction_product) { + outcome.skills.push(EnvironmentSkillSnapshot { + metadata, + instructions: skill.instructions.contents.clone(), + }); + } + } + outcome.skills.sort_by(|left, right| { + left.metadata.name.cmp(&right.metadata.name).then_with(|| { + left.metadata + .path_to_skills_md + .to_string() + .cmp(&right.metadata.path_to_skills_md.to_string()) + }) + }); + outcome +} + +fn nearest_plugin_namespace<'a>( + skill_path: &PathUri, + plugin_namespaces: &'a HashMap, +) -> Option<&'a str> { + let mut ancestor = skill_path.parent(); + while let Some(path) = ancestor { + if let Some(namespace) = plugin_namespaces.get(&path) { + return Some(namespace); + } + ancestor = path.parent(); + } + None +} + +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/ext/skills/src/loader/environment_tests.rs b/codex-rs/ext/skills/src/loader/environment_tests.rs new file mode 100644 index 0000000000..884b53aee5 --- /dev/null +++ b/codex-rs/ext/skills/src/loader/environment_tests.rs @@ -0,0 +1,151 @@ +use std::fs; + +use codex_core_skills::loader::load_environment_skills_from_root; +use codex_exec_server::CapabilityRootDiscoverRequest; +use codex_exec_server::CapabilityRootsDiscoverParams; +use codex_exec_server::LOCAL_FS; +use codex_exec_server::discover_capability_roots; +use codex_utils_path_uri::PathUri; +use pretty_assertions::assert_eq; +use tempfile::tempdir; + +use super::load_environment_skills_from_discovery; + +#[tokio::test] +async fn executor_bundle_parser_matches_the_existing_environment_loader() { + let root = tempdir().expect("tempdir"); + let plugin_manifest = root.path().join(".codex-plugin/plugin.json"); + let nested_manifest = root.path().join("nested/.claude-plugin/plugin.json"); + let deploy_skill = root.path().join("skills/deploy/SKILL.md"); + let deploy_metadata = root.path().join("skills/deploy/agents/openai.yaml"); + let audit_skill = root.path().join("nested/skills/audit/SKILL.md"); + for (path, contents) in [ + (&plugin_manifest, r#"{"name":"demo"}"#), + (&nested_manifest, r#"{"name":"nested"}"#), + ( + &deploy_skill, + "---\nname: deploy\ndescription: Deploy the service.\n---\n\nDeploy.\n", + ), + ( + &deploy_metadata, + "policy:\n allow_implicit_invocation: false\n", + ), + ( + &audit_skill, + "---\nname: audit\ndescription: Audit the service.\n---\n\nAudit.\n", + ), + ] { + fs::create_dir_all(path.parent().expect("test file parent")).expect("test directory"); + fs::write(path, contents).expect("test file"); + } + + let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI"); + let existing = load_environment_skills_from_root( + LOCAL_FS.as_ref(), + &root_uri, + /*restriction_product*/ None, + ) + .await; + let response = discover_capability_roots( + LOCAL_FS.as_ref(), + CapabilityRootsDiscoverParams { + roots: vec![CapabilityRootDiscoverRequest { + id: "demo@1".to_string(), + path: root_uri, + sandbox: None, + }], + }, + ) + .await + .expect("capability discovery"); + let bundled = load_environment_skills_from_discovery( + response.roots.first().expect("discovered root"), + /*restriction_product*/ None, + ); + + assert_eq!(bundled.warnings, existing.warnings); + assert_eq!( + bundled + .skills + .iter() + .map(|skill| skill.metadata.clone()) + .collect::>(), + existing.skills + ); + assert_eq!( + bundled + .skills + .iter() + .map(|skill| skill.instructions.as_str()) + .collect::>(), + vec![ + "---\nname: deploy\ndescription: Deploy the service.\n---\n\nDeploy.\n", + "---\nname: audit\ndescription: Audit the service.\n---\n\nAudit.\n", + ] + ); +} + +#[tokio::test] +async fn executor_bundle_preserves_parent_namespace_and_manifest_precedence() { + let plugin = tempdir().expect("tempdir"); + for (relative_path, name) in [ + (".codex-plugin/plugin.json", "codex-name"), + (".claude-plugin/plugin.json", "claude-name"), + (".cursor-plugin/plugin.json", "cursor-name"), + ] { + let manifest = plugin.path().join(relative_path); + fs::create_dir_all(manifest.parent().expect("manifest parent")) + .expect("manifest directory"); + fs::write(&manifest, format!(r#"{{"name":"{name}"}}"#)).expect("manifest"); + } + let skills_root = plugin.path().join("skills"); + let skill_path = skills_root.join("search/SKILL.md"); + fs::create_dir_all(skill_path.parent().expect("skill parent")).expect("skill directory"); + fs::write( + &skill_path, + "---\nname: search\ndescription: Search the project.\n---\n\nSearch.\n", + ) + .expect("skill"); + + 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(), + &root_uri, + /*restriction_product*/ None, + ) + .await; + let response = discover_capability_roots( + LOCAL_FS.as_ref(), + CapabilityRootsDiscoverParams { + roots: vec![CapabilityRootDiscoverRequest { + id: "skills-only".to_string(), + path: root_uri, + sandbox: None, + }], + }, + ) + .await + .expect("capability discovery"); + let discovery = response.roots.first().expect("discovered root"); + let bundled = + load_environment_skills_from_discovery(discovery, /*restriction_product*/ None); + + assert_eq!(discovery.plugin, None); + assert_eq!(discovery.namespace_manifests.len(), 1); + assert!( + discovery.namespace_manifests[0] + .path + .to_string() + .ends_with("/.codex-plugin/plugin.json") + ); + assert_eq!(bundled.warnings, existing.warnings); + assert_eq!( + bundled + .skills + .iter() + .map(|skill| skill.metadata.clone()) + .collect::>(), + existing.skills + ); + assert_eq!(bundled.skills[0].metadata.name, "codex-name:search"); +} diff --git a/codex-rs/ext/skills/src/loader/metadata.rs b/codex-rs/ext/skills/src/loader/metadata.rs new file mode 100644 index 0000000000..e3537cfdd1 --- /dev/null +++ b/codex-rs/ext/skills/src/loader/metadata.rs @@ -0,0 +1,176 @@ +use std::path::PathBuf; + +use codex_protocol::protocol::Product; +use codex_skills::SkillDependencies; +use codex_skills::SkillParseError; +use codex_skills::SkillPolicy; +use codex_skills::SkillToolDependency; +use serde::Deserialize; + +use super::MAX_DEPENDENCY_COMMAND_LEN; +use super::MAX_DEPENDENCY_DESCRIPTION_LEN; +use super::MAX_DEPENDENCY_TRANSPORT_LEN; +use super::MAX_DEPENDENCY_TYPE_LEN; +use super::MAX_DEPENDENCY_URL_LEN; +use super::MAX_DEPENDENCY_VALUE_LEN; + +#[derive(Debug, Default, Deserialize)] +pub(super) struct SkillMetadataFile { + // Keep parsing the host-only interface fields so malformed interface metadata + // invalidates the file exactly as it does in the legacy loader. + #[serde(default, rename = "interface")] + _interface: Option, + #[serde(default)] + pub(super) dependencies: Option, + #[serde(default)] + pub(super) policy: Option, +} + +#[derive(Debug, Default, Deserialize)] +struct Interface { + #[serde(rename = "display_name")] + _display_name: Option, + #[serde(rename = "short_description")] + _short_description: Option, + #[serde(rename = "icon_small")] + _icon_small: Option, + #[serde(rename = "icon_large")] + _icon_large: Option, + #[serde(rename = "brand_color")] + _brand_color: Option, + #[serde(rename = "default_prompt")] + _default_prompt: Option, +} + +#[derive(Debug, Default, Deserialize)] +pub(super) struct Dependencies { + #[serde(default)] + tools: Vec, +} + +#[derive(Debug, Deserialize)] +pub(super) struct Policy { + #[serde(default)] + allow_implicit_invocation: Option, + #[serde(default)] + products: Vec, +} + +#[derive(Debug, Default, Deserialize)] +struct DependencyTool { + #[serde(rename = "type")] + kind: Option, + value: Option, + description: Option, + transport: Option, + command: Option, + url: Option, +} + +pub(super) fn resolve_dependencies( + dependencies: Option, +) -> Option { + let dependencies = dependencies?; + let tools = dependencies + .tools + .into_iter() + .filter_map(resolve_dependency_tool) + .collect::>(); + if tools.is_empty() { + None + } else { + Some(SkillDependencies { tools }) + } +} + +pub(super) fn resolve_policy(policy: Option) -> Option { + policy.map(|policy| SkillPolicy { + allow_implicit_invocation: policy.allow_implicit_invocation, + products: policy.products, + }) +} + +fn resolve_dependency_tool(tool: DependencyTool) -> Option { + let r#type = resolve_required_str( + tool.kind, + MAX_DEPENDENCY_TYPE_LEN, + "dependencies.tools.type", + )?; + let value = resolve_required_str( + tool.value, + MAX_DEPENDENCY_VALUE_LEN, + "dependencies.tools.value", + )?; + let description = resolve_str( + tool.description, + MAX_DEPENDENCY_DESCRIPTION_LEN, + "dependencies.tools.description", + ); + let transport = resolve_str( + tool.transport, + MAX_DEPENDENCY_TRANSPORT_LEN, + "dependencies.tools.transport", + ); + let command = resolve_str( + tool.command, + MAX_DEPENDENCY_COMMAND_LEN, + "dependencies.tools.command", + ); + let url = resolve_str(tool.url, MAX_DEPENDENCY_URL_LEN, "dependencies.tools.url"); + + Some(SkillToolDependency { + r#type, + value, + description, + transport, + command, + url, + }) +} + +pub(super) fn sanitize_single_line(raw: &str) -> String { + raw.split_whitespace().collect::>().join(" ") +} + +pub(super) fn validate_len( + value: &str, + max_len: usize, + field_name: &'static str, +) -> Result<(), SkillParseError> { + if value.is_empty() { + return Err(SkillParseError::MissingField(field_name)); + } + if value.chars().count() > max_len { + return Err(SkillParseError::InvalidField { + field: field_name, + reason: format!("exceeds maximum length of {max_len} characters"), + }); + } + Ok(()) +} + +fn resolve_str(value: Option, max_len: usize, field: &'static str) -> Option { + let value = value?; + let value = sanitize_single_line(&value); + if value.is_empty() { + tracing::warn!("ignoring {field}: value is empty"); + return None; + } + if value.chars().count() > max_len { + tracing::warn!("ignoring {field}: exceeds maximum length of {max_len} characters"); + return None; + } + Some(value) +} + +fn resolve_required_str( + value: Option, + max_len: usize, + field: &'static str, +) -> Option { + let Some(value) = value else { + tracing::warn!("ignoring {field}: value is missing"); + return None; + }; + resolve_str(Some(value), max_len, field) +} diff --git a/codex-rs/ext/skills/src/loader/mod.rs b/codex-rs/ext/skills/src/loader/mod.rs new file mode 100644 index 0000000000..ac5ed067ff --- /dev/null +++ b/codex-rs/ext/skills/src/loader/mod.rs @@ -0,0 +1,14 @@ +mod environment; +mod metadata; + +pub(crate) use environment::load_environment_skills_from_discovery; + +pub(super) const MAX_NAME_LEN: usize = 64; +pub(super) const MAX_QUALIFIED_NAME_LEN: usize = 128; +pub(super) const MAX_DESCRIPTION_LEN: usize = 1024; +pub(super) const MAX_DEPENDENCY_TYPE_LEN: usize = MAX_NAME_LEN; +pub(super) const MAX_DEPENDENCY_TRANSPORT_LEN: usize = MAX_NAME_LEN; +pub(super) const MAX_DEPENDENCY_VALUE_LEN: usize = MAX_DESCRIPTION_LEN; +pub(super) const MAX_DEPENDENCY_DESCRIPTION_LEN: usize = MAX_DESCRIPTION_LEN; +pub(super) const MAX_DEPENDENCY_COMMAND_LEN: usize = MAX_DESCRIPTION_LEN; +pub(super) const MAX_DEPENDENCY_URL_LEN: usize = MAX_DESCRIPTION_LEN; diff --git a/codex-rs/ext/skills/src/provider/executor.rs b/codex-rs/ext/skills/src/provider/executor.rs index 3c9132d619..dde6473a89 100644 --- a/codex-rs/ext/skills/src/provider/executor.rs +++ b/codex-rs/ext/skills/src/provider/executor.rs @@ -1,6 +1,5 @@ use std::sync::Arc; -use codex_core_skills::loader::load_environment_skills_from_discovery; use codex_core_skills::loader::load_environment_skills_from_root; use codex_exec_server::EnvironmentManager; use codex_exec_server::FileSystemSandboxContext; @@ -20,6 +19,7 @@ use crate::catalog::SkillReadResult; use crate::catalog::SkillResourceId; use crate::catalog::SkillSearchResult; use crate::catalog::SkillSourceKind; +use crate::loader::load_environment_skills_from_discovery; use crate::provider::MAX_SKILL_RESOURCE_CONTENT_BYTES; use crate::provider::SkillListQuery; use crate::provider::SkillProvider; 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 1fa0a20989..a74e9b4875 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -7,6 +7,7 @@ use std::sync::atomic::Ordering; use codex_core_skills::HostSkillsSnapshot; use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; use codex_core_skills::loader::SkillRoot; +use codex_core_skills::loader::load_environment_skills_from_root as load_legacy_environment_skills_from_root; use codex_core_skills::loader::load_skills_from_roots; use codex_exec_server::CopyOptions; use codex_exec_server::CreateDirectoryOptions; @@ -24,6 +25,7 @@ use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_skills_extension::ExecutorSkillProvider; use codex_skills_extension::catalog::SkillAuthority; @@ -350,6 +352,295 @@ async fn selected_root_id_distinguishes_identical_executor_paths() { std::fs::remove_dir_all(test_root).expect("remove skill directory"); } +#[tokio::test] +async fn executor_discovery_routes_produce_equivalent_catalog_metadata() { + let id = NEXT_TEST_ROOT_ID.fetch_add(1, Ordering::Relaxed); + let test_root = std::env::temp_dir().join(format!( + "codex-executor-skill-parity-{}-{id}", + std::process::id() + )); + let plugin_manifest = test_root.join(".codex-plugin/plugin.json"); + let deploy_skill = test_root.join("skills/deploy/SKILL.md"); + let deploy_metadata = test_root.join("skills/deploy/agents/openai.yaml"); + let excluded_skill = test_root.join("skills/excluded/SKILL.md"); + let excluded_metadata = test_root.join("skills/excluded/agents/openai.yaml"); + let repaired_skill = test_root.join("skills/repaired/SKILL.md"); + let invalid_skill = test_root.join("skills/invalid/SKILL.md"); + let invalid_metadata_skill = test_root.join("skills/invalid-metadata/SKILL.md"); + let invalid_metadata = test_root.join("skills/invalid-metadata/agents/openai.yaml"); + for path in [ + &plugin_manifest, + &deploy_skill, + &deploy_metadata, + &excluded_skill, + &excluded_metadata, + &repaired_skill, + &invalid_skill, + &invalid_metadata_skill, + &invalid_metadata, + ] { + std::fs::create_dir_all(path.parent().expect("test file parent")) + .expect("create test directory"); + } + std::fs::write(&plugin_manifest, r#"{"name":"catalog"}"#).expect("write plugin manifest"); + std::fs::write( + &deploy_skill, + "---\nname: deploy\ndescription: Deploy the service.\nmetadata:\n short-description: Deploy safely.\n---\n\nDeploy.\n", + ) + .expect("write deploy skill"); + std::fs::write( + &deploy_metadata, + "dependencies:\n tools:\n - type: mcp\n value: deployer\n description: Deployment server.\npolicy:\n allow_implicit_invocation: false\n products:\n - codex\n", + ) + .expect("write deploy metadata"); + std::fs::write( + &excluded_skill, + "---\nname: excluded\ndescription: Excluded skill.\n---\n", + ) + .expect("write excluded skill"); + std::fs::write(&excluded_metadata, "policy:\n products:\n - chatgpt\n") + .expect("write excluded metadata"); + std::fs::write( + &repaired_skill, + "---\ndescription: Build for AWS: ECS\n---\n", + ) + .expect("write repaired skill"); + std::fs::write(&invalid_skill, "---\nname: invalid\n---\n").expect("write invalid skill"); + std::fs::write( + &invalid_metadata_skill, + "---\nname: invalid-metadata\ndescription: Invalid optional metadata.\n---\n", + ) + .expect("write invalid metadata skill"); + std::fs::write( + &invalid_metadata, + "interface: []\ndependencies:\n tools:\n - type: mcp\n value: must-be-ignored\n", + ) + .expect("write invalid metadata"); + + let manager = Arc::new(EnvironmentManager::default_for_tests()); + let provider = ExecutorSkillProvider::new_with_restriction_product( + Arc::clone(&manager), + Some(Product::Codex), + ); + let executor_roots = vec![SelectedCapabilityRoot { + id: "parity-root".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "local".to_string(), + path: PathUri::from_host_native_path(&test_root).expect("skill root URI"), + }, + }]; + let query = |executor_capability_discovery| SkillListQuery { + turn_id: "turn-1".to_string(), + executor_roots: executor_roots.clone(), + resolved_executor_roots: Vec::new(), + host_snapshot: None, + include_host_skills: false, + include_bundled_skills: true, + include_orchestrator_skills: false, + mcp_resources: None, + executor_capability_discovery, + }; + + let legacy_file_system = manager + .get_environment("local") + .expect("local environment") + .get_filesystem(); + let legacy = load_legacy_environment_skills_from_root( + legacy_file_system.as_ref(), + &PathUri::from_host_native_path(&test_root).expect("skill root URI"), + Some(Product::Codex), + ) + .await; + let direct = provider + .list(query(None)) + .await + .expect("list directly discovered executor skills"); + let discovery = ExecutorCapabilityDiscoveryCache::new(manager) + .snapshot(&executor_roots, &Default::default()) + .await; + let bundled = provider + .list(query(Some(discovery))) + .await + .expect("list bundled executor skills"); + + assert_eq!(bundled.warnings, direct.warnings); + let comparable_entries = |catalog: &codex_skills_extension::catalog::SkillCatalog| { + catalog + .entries + .iter() + .map(|entry| { + ( + entry.id.clone(), + entry.authority.clone(), + entry.name.clone(), + entry.description.clone(), + entry.short_description.clone(), + entry.display_path.clone(), + entry.dependencies.clone(), + entry.enabled, + entry.prompt_visible, + ) + }) + .collect::>() + }; + assert_eq!(comparable_entries(&bundled), comparable_entries(&direct)); + assert_eq!(legacy.warnings, direct.warnings); + assert_eq!(direct.warnings.len(), 1); + assert!(direct.warnings[0].contains("missing field `description`")); + let legacy_metadata = legacy + .skills + .iter() + .map(|skill| { + ( + skill.name.clone(), + skill.description.clone(), + skill.short_description.clone(), + skill.dependencies.clone(), + skill.allows_implicit_invocation(), + ) + }) + .collect::>(); + let catalog_metadata = direct + .entries + .iter() + .map(|entry| { + ( + entry.name.clone(), + entry.description.clone(), + entry.short_description.clone(), + entry.dependencies.clone(), + entry.prompt_visible, + ) + }) + .collect::>(); + assert_eq!(catalog_metadata, legacy_metadata); + let entry = direct + .entries + .iter() + .find(|entry| entry.name == "catalog:deploy") + .expect("deploy skill"); + assert_eq!( + entry + .dependencies + .as_ref() + .expect("skill dependencies") + .tools[0] + .value, + "deployer" + ); + assert!(!entry.prompt_visible); + let repaired = direct + .entries + .iter() + .find(|entry| entry.name == "catalog:repaired") + .expect("repaired skill"); + assert_eq!(repaired.description, "Build for AWS: ECS"); + let invalid_metadata = direct + .entries + .iter() + .find(|entry| entry.name == "catalog:invalid-metadata") + .expect("invalid metadata skill"); + assert_eq!(invalid_metadata.dependencies, None); + + std::fs::remove_dir_all(test_root).expect("remove skill directory"); +} + +#[tokio::test] +async fn pre_discovered_executor_catalog_snapshot() { + let test_root = create_local_skill_root("snapshot").expect("create local skill root"); + let manifest_dir = test_root.join(".codex-plugin"); + let metadata_dir = test_root.join("skill/agents"); + std::fs::create_dir_all(&manifest_dir).expect("create plugin manifest directory"); + std::fs::create_dir_all(&metadata_dir).expect("create skill metadata directory"); + std::fs::write( + manifest_dir.join("plugin.json"), + r#"{"name":"snapshot-plugin"}"#, + ) + .expect("write plugin manifest"); + std::fs::write( + metadata_dir.join("openai.yaml"), + "dependencies:\n tools:\n - type: mcp\n value: deployer\n description: Deployment server.\npolicy:\n allow_implicit_invocation: false\n", + ) + .expect("write skill metadata"); + + let manager = Arc::new(EnvironmentManager::default_for_tests()); + let provider = ExecutorSkillProvider::new_with_restriction_product( + Arc::clone(&manager), + /*restriction_product*/ None, + ); + let executor_roots = vec![SelectedCapabilityRoot { + id: "snapshot-root".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "local".to_string(), + path: PathUri::from_host_native_path(&test_root).expect("skill root URI"), + }, + }]; + let executor_capability_discovery = ExecutorCapabilityDiscoveryCache::new(manager) + .snapshot(&executor_roots, &Default::default()) + .await; + let catalog = provider + .list(SkillListQuery { + turn_id: "turn-1".to_string(), + executor_roots, + resolved_executor_roots: Vec::new(), + host_snapshot: None, + include_host_skills: false, + include_bundled_skills: true, + include_orchestrator_skills: false, + mcp_resources: None, + executor_capability_discovery: Some(executor_capability_discovery), + }) + .await + .expect("list pre-discovered executor skills"); + + let mut snapshot = serde_json::json!({ + "warnings": &catalog.warnings, + "entries": catalog + .entries + .iter() + .map(|entry| serde_json::json!({ + "authority": { + "kind": entry.authority.kind.to_string(), + "id": &entry.authority.id, + }, + "name": &entry.name, + "description": &entry.description, + "short_description": entry.short_description.as_deref(), + "dependencies": entry.dependencies.as_ref().map(|dependencies| { + dependencies.tools.iter().map(|tool| serde_json::json!({ + "type": &tool.r#type, + "value": &tool.value, + "description": tool.description.as_deref(), + "transport": tool.transport.as_deref(), + "command": tool.command.as_deref(), + "url": tool.url.as_deref(), + })).collect::>() + }), + "enabled": entry.enabled, + "prompt_visible": entry.prompt_visible, + })) + .collect::>(), + }); + snapshot.sort_all_objects(); + let snapshot_file = codex_utils_cargo_bin::find_resource!( + "tests/snapshots/executor_file_system_authority__pre_discovered_executor_catalog.snap" + ) + .expect("resolve catalog snapshot"); + let snapshot_dir = snapshot_file + .parent() + .unwrap_or_else(|| panic!("snapshot file has no parent: {}", snapshot_file.display())); + let mut settings = insta::Settings::clone_current(); + settings.set_snapshot_path(snapshot_dir); + settings.bind(|| { + insta::assert_snapshot!( + "pre_discovered_executor_catalog", + serde_json::to_string_pretty(&snapshot).expect("serialize catalog snapshot") + ); + }); + + std::fs::remove_dir_all(test_root).expect("remove skill directory"); +} + #[tokio::test] async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() { let test_root = create_local_skill_root("materialized").expect("create local skill root"); diff --git a/codex-rs/ext/skills/tests/snapshots/executor_file_system_authority__pre_discovered_executor_catalog.snap b/codex-rs/ext/skills/tests/snapshots/executor_file_system_authority__pre_discovered_executor_catalog.snap new file mode 100644 index 0000000000..0b59ec0031 --- /dev/null +++ b/codex-rs/ext/skills/tests/snapshots/executor_file_system_authority__pre_discovered_executor_catalog.snap @@ -0,0 +1,30 @@ +--- +source: ext/skills/tests/executor_file_system_authority.rs +expression: "serde_json::to_string_pretty(&snapshot).expect(\"serialize catalog snapshot\")" +--- +{ + "entries": [ + { + "authority": { + "id": "snapshot-root", + "kind": "executor" + }, + "dependencies": [ + { + "command": null, + "description": "Deployment server.", + "transport": null, + "type": "mcp", + "url": null, + "value": "deployer" + } + ], + "description": "Synthetic executor skill.", + "enabled": true, + "name": "snapshot-plugin:synthetic", + "prompt_visible": false, + "short_description": null + } + ], + "warnings": [] +} diff --git a/codex-rs/skills/Cargo.toml b/codex-rs/skills/Cargo.toml index 010e675186..f1c91c43a6 100644 --- a/codex-rs/skills/Cargo.toml +++ b/codex-rs/skills/Cargo.toml @@ -18,4 +18,9 @@ codex-protocol = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-path-uri = { workspace = true } include_dir = { workspace = true } +serde = { workspace = true, features = ["derive"] } +serde_yaml = { workspace = true } thiserror = { workspace = true } + +[dev-dependencies] +pretty_assertions = { workspace = true } diff --git a/codex-rs/skills/src/lib.rs b/codex-rs/skills/src/lib.rs index 76ce83b6c7..c6e88f01bc 100644 --- a/codex-rs/skills/src/lib.rs +++ b/codex-rs/skills/src/lib.rs @@ -1,4 +1,5 @@ mod model; +mod parser; pub use model::EnvironmentSkillMetadata; pub use model::SkillConfigRule; @@ -9,6 +10,9 @@ pub use model::SkillInterface; pub use model::SkillMetadata; pub use model::SkillPolicy; pub use model::SkillToolDependency; +pub use parser::ParsedSkillFrontmatter; +pub use parser::SkillParseError; +pub use parser::parse_skill_frontmatter_metadata; use codex_utils_absolute_path::AbsolutePathBuf; use include_dir::Dir; diff --git a/codex-rs/skills/src/parser.rs b/codex-rs/skills/src/parser.rs new file mode 100644 index 0000000000..aef295cd03 --- /dev/null +++ b/codex-rs/skills/src/parser.rs @@ -0,0 +1,225 @@ +use serde::Deserialize; +use thiserror::Error; + +const MAX_NAME_LEN: usize = 64; + +#[derive(Debug, Deserialize)] +struct SkillFrontmatter { + #[serde(default)] + name: Option, + #[serde(default)] + description: Option, + #[serde(default)] + metadata: SkillFrontmatterMetadata, +} + +#[derive(Debug, Default, Deserialize)] +struct SkillFrontmatterMetadata { + #[serde(default, rename = "short-description")] + short_description: Option, +} + +/// Validated metadata parsed from a `SKILL.md` frontmatter block. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ParsedSkillFrontmatter { + pub name: String, + pub description: String, + pub short_description: Option, +} + +/// Error produced while parsing or validating `SKILL.md` metadata. +#[derive(Debug, Error)] +pub enum SkillParseError { + #[error("missing YAML frontmatter delimited by ---")] + MissingFrontmatter, + #[error("invalid YAML: {0}")] + InvalidYaml(#[source] serde_yaml::Error), + #[error("missing field `{0}`")] + MissingField(&'static str), + #[error("invalid {field}: {reason}")] + InvalidField { field: &'static str, reason: String }, +} + +/// Parses and validates the metadata frontmatter from `SKILL.md` contents. +pub fn parse_skill_frontmatter_metadata( + 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), + Err(original_error) => match repair_frontmatter_scalar_fields(&frontmatter) { + // Some third-party skills use prose like `description: Build for AWS: ECS` + // or `argument-hint: `. Keep the repair line-oriented + // so unrelated invalid YAML still surfaces. + Some(repaired_frontmatter) => { + serde_yaml::from_str(&repaired_frontmatter).map_err(|_| original_error) + } + None => Err(original_error), + }, + } + .map_err(SkillParseError::InvalidYaml)?; + + let name = parsed + .name + .as_deref() + .map(sanitize_single_line) + .filter(|value| !value.is_empty()) + .unwrap_or_else(default_name); + let description = parsed + .description + .as_deref() + .map(sanitize_single_line) + .unwrap_or_default(); + let short_description = parsed + .metadata + .short_description + .as_deref() + .map(sanitize_single_line) + .filter(|value| !value.is_empty()); + + validate_len(&name, MAX_NAME_LEN, "name")?; + if description.is_empty() { + return Err(SkillParseError::MissingField("description")); + } + + Ok(ParsedSkillFrontmatter { + name, + description, + short_description, + }) +} + +fn sanitize_single_line(raw: &str) -> String { + raw.split_whitespace().collect::>().join(" ") +} + +fn repair_frontmatter_scalar_fields(frontmatter: &str) -> Option { + let mut changed = false; + let mut block_scalar_indent: Option = None; + let mut repaired_lines = Vec::new(); + for line in frontmatter.lines() { + let indent = line + .chars() + .take_while(|character| *character == ' ') + .count(); + if let Some(block_indent) = block_scalar_indent { + if line.trim().is_empty() || indent > block_indent { + repaired_lines.push(line.to_string()); + continue; + } + block_scalar_indent = None; + } + + let Some((key, value)) = line.split_once(':') else { + repaired_lines.push(line.to_string()); + continue; + }; + if key.trim().is_empty() || !value.chars().next().is_none_or(char::is_whitespace) { + repaired_lines.push(line.to_string()); + continue; + } + + let trimmed_start = value.trim_start(); + let leading_whitespace = &value[..value.len() - trimmed_start.len()]; + let mut scalar = trimmed_start; + let mut comment = ""; + for (index, character) in trimmed_start.char_indices() { + if character == '#' + && (index == 0 + || trimmed_start[..index] + .chars() + .next_back() + .is_some_and(char::is_whitespace)) + { + let comment_start = trimmed_start[..index].trim_end().len(); + scalar = &trimmed_start[..comment_start]; + comment = &trimmed_start[comment_start..]; + break; + } + } + + let scalar = scalar.trim_end(); + let Some(first_char) = scalar.chars().next() else { + repaired_lines.push(line.to_string()); + continue; + }; + if matches!(first_char, '|' | '>') { + block_scalar_indent = Some(indent); + repaired_lines.push(line.to_string()); + continue; + } + if matches!(first_char, '\'' | '"') { + repaired_lines.push(line.to_string()); + continue; + } + let mut has_colon_separator = false; + let mut chars = scalar.chars().peekable(); + while let Some(character) = chars.next() { + if character == ':' + && matches!(chars.peek(), Some(next_character) if next_character.is_whitespace()) + { + has_colon_separator = true; + break; + } + } + let invalid_flow_like_scalar = matches!(first_char, '[' | '{' | '@' | '`') + && serde_yaml::from_str::(scalar).is_err(); + if !has_colon_separator && !invalid_flow_like_scalar { + repaired_lines.push(line.to_string()); + continue; + } + + let quoted_scalar = format!("'{}'", scalar.replace('\'', "''")); + repaired_lines.push(format!( + "{key}:{leading_whitespace}{quoted_scalar}{comment}" + )); + changed = true; + } + changed.then(|| repaired_lines.join("\n")) +} + +fn validate_len( + value: &str, + max_len: usize, + field_name: &'static str, +) -> Result<(), SkillParseError> { + if value.is_empty() { + return Err(SkillParseError::MissingField(field_name)); + } + if value.chars().count() > max_len { + return Err(SkillParseError::InvalidField { + field: field_name, + reason: format!("exceeds maximum length of {max_len} characters"), + }); + } + Ok(()) +} + +fn extract_frontmatter(contents: &str) -> Option { + let mut lines = contents.lines(); + if !matches!(lines.next(), Some(line) if line.trim() == "---") { + return None; + } + + let mut frontmatter_lines = Vec::new(); + let mut found_closing = false; + for line in lines.by_ref() { + if line.trim() == "---" { + found_closing = true; + break; + } + frontmatter_lines.push(line); + } + + if frontmatter_lines.is_empty() || !found_closing { + return None; + } + + Some(frontmatter_lines.join("\n")) +} + +#[cfg(test)] +#[path = "parser_tests.rs"] +mod tests; diff --git a/codex-rs/skills/src/parser_tests.rs b/codex-rs/skills/src/parser_tests.rs new file mode 100644 index 0000000000..2bbad1e2c0 --- /dev/null +++ b/codex-rs/skills/src/parser_tests.rs @@ -0,0 +1,43 @@ +use pretty_assertions::assert_eq; + +use super::ParsedSkillFrontmatter; +use super::parse_skill_frontmatter_metadata; + +#[test] +fn parses_repairs_and_sanitizes_frontmatter() { + let parsed = parse_skill_frontmatter_metadata( + "---\nname: deploy service\ndescription: Build for AWS: ECS\nmetadata:\n short-description: Deploy safely\n---\n", + || "fallback".to_string(), + ) + .expect("valid frontmatter"); + + assert_eq!( + parsed, + ParsedSkillFrontmatter { + name: "deploy service".to_string(), + description: "Build for AWS: ECS".to_string(), + short_description: Some("Deploy safely".to_string()), + } + ); +} + +#[test] +fn uses_default_name_and_requires_description() { + let parsed = parse_skill_frontmatter_metadata("---\ndescription: Demo skill\n---\n", || { + "demo".to_string() + }) + .expect("valid frontmatter"); + assert_eq!( + parsed, + ParsedSkillFrontmatter { + name: "demo".to_string(), + description: "Demo skill".to_string(), + short_description: None, + } + ); + + let error = + parse_skill_frontmatter_metadata("---\nname: demo\n---\n", || "fallback".to_string()) + .expect_err("description should be required"); + assert_eq!(error.to_string(), "missing field `description`"); +}