diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 275349fbef..07961e4361 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4092,6 +4092,7 @@ dependencies = [ "serde", "serde_yaml", "thiserror 2.0.18", + "tracing", ] [[package]] diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 2651c6445a..ac5d090e67 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -24,8 +24,11 @@ use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_skills::ParsedSkillFrontmatter; +use codex_skills::SkillInterfaceAssetPolicy; +use codex_skills::SkillInterfaceFile; use codex_skills::SkillParseError; use codex_skills::parse_skill_frontmatter_metadata; +use codex_skills::resolve_skill_interface; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use codex_utils_path_uri::PathUri; @@ -49,9 +52,6 @@ use std::collections::HashSet; use std::error::Error; use std::fmt; use std::io; -use std::path::Component; -use std::path::Path; -use std::path::PathBuf; use std::sync::Arc; use tokio::sync::Semaphore; use toml::Value as TomlValue; @@ -63,7 +63,7 @@ pub const MAX_CONCURRENT_ROOT_SCANS: usize = 8; #[derive(Debug, Default, Deserialize)] struct SkillMetadataFile { #[serde(default)] - interface: Option, + interface: Option, #[serde(default)] dependencies: Option, #[serde(default)] @@ -77,16 +77,6 @@ struct LoadedSkillMetadata { policy: Option, } -#[derive(Debug, Default, Deserialize)] -struct Interface { - display_name: Option, - short_description: Option, - icon_small: Option, - icon_large: Option, - brand_color: Option, - default_prompt: Option, -} - #[derive(Debug, Default, Deserialize)] struct Dependencies { #[serde(default)] @@ -120,8 +110,6 @@ const SKILLS_DIR_NAME: &str = "skills"; const MAX_NAME_LEN: usize = 64; const MAX_QUALIFIED_NAME_LEN: usize = 128; const MAX_DESCRIPTION_LEN: usize = 1024; -const MAX_SHORT_DESCRIPTION_LEN: usize = MAX_DESCRIPTION_LEN; -const MAX_DEFAULT_PROMPT_LEN: usize = MAX_DESCRIPTION_LEN; const MAX_DEPENDENCY_TYPE_LEN: usize = MAX_NAME_LEN; const MAX_DEPENDENCY_TRANSPORT_LEN: usize = MAX_NAME_LEN; const MAX_DEPENDENCY_VALUE_LEN: usize = MAX_DESCRIPTION_LEN; @@ -784,58 +772,17 @@ async fn load_skill_metadata( dependencies, policy, } = parsed; + let asset_policy = match plugin_root { + Some(plugin_root) => SkillInterfaceAssetPolicy::PluginShared { plugin_root }, + None => SkillInterfaceAssetPolicy::LocalOnly, + }; LoadedSkillMetadata { - interface: resolve_interface(interface, &skill_dir, plugin_root), + interface: resolve_skill_interface(interface, &skill_dir, asset_policy), dependencies: resolve_dependencies(dependencies), policy: resolve_policy(policy), } } -fn resolve_interface( - interface: Option, - skill_dir: &AbsolutePathBuf, - plugin_root: Option<&AbsolutePathBuf>, -) -> Option { - let interface = interface?; - let interface = SkillInterface { - display_name: resolve_str( - interface.display_name, - MAX_NAME_LEN, - "interface.display_name", - ), - short_description: resolve_str( - interface.short_description, - MAX_SHORT_DESCRIPTION_LEN, - "interface.short_description", - ), - icon_small: resolve_asset_path( - skill_dir, - plugin_root, - "interface.icon_small", - interface.icon_small, - ), - icon_large: resolve_asset_path( - skill_dir, - plugin_root, - "interface.icon_large", - interface.icon_large, - ), - brand_color: resolve_color_str(interface.brand_color, "interface.brand_color"), - default_prompt: resolve_str( - interface.default_prompt, - MAX_DEFAULT_PROMPT_LEN, - "interface.default_prompt", - ), - }; - let has_fields = interface.display_name.is_some() - || interface.short_description.is_some() - || interface.icon_small.is_some() - || interface.icon_large.is_some() - || interface.brand_color.is_some() - || interface.default_prompt.is_some(); - if has_fields { Some(interface) } else { None } -} - fn resolve_dependencies(dependencies: Option) -> Option { let dependencies = dependencies?; let tools: Vec = dependencies @@ -895,97 +842,6 @@ fn resolve_dependency_tool(tool: DependencyTool) -> Option }) } -fn resolve_asset_path( - skill_dir: &AbsolutePathBuf, - plugin_root: Option<&AbsolutePathBuf>, - field: &'static str, - path: Option, -) -> Option { - // Icons must stay under the skill's assets directory. Plugin skills may - // also share icons from the plugin-level assets directory. - let path = path?; - if path.as_os_str().is_empty() { - return None; - } - - let assets_dir = skill_dir.join("assets"); - if path.is_absolute() { - tracing::warn!( - "ignoring {field}: icon must be a relative assets path (not {})", - assets_dir.display() - ); - return None; - } - - let mut normalized = PathBuf::new(); - for component in path.components() { - match component { - Component::CurDir => {} - Component::Normal(component) => normalized.push(component), - Component::ParentDir => { - return resolve_plugin_shared_asset_path(skill_dir, plugin_root, field, &path); - } - _ => { - tracing::warn!("ignoring {field}: icon path must be under assets/"); - return None; - } - } - } - - let mut components = normalized.components(); - match components.next() { - Some(Component::Normal(component)) if component == "assets" => {} - _ => { - tracing::warn!("ignoring {field}: icon path must be under assets/"); - return None; - } - } - - Some(skill_dir.join(normalized)) -} - -fn resolve_plugin_shared_asset_path( - skill_dir: &AbsolutePathBuf, - plugin_root: Option<&AbsolutePathBuf>, - field: &'static str, - path: &Path, -) -> Option { - let Some(plugin_root) = plugin_root else { - tracing::warn!("ignoring {field}: icon path must not contain '..'"); - return None; - }; - - let plugin_assets_dir = lexically_normalize(plugin_root.join("assets").as_path()); - let resolved = lexically_normalize(skill_dir.join(path).as_path()); - if !resolved.starts_with(&plugin_assets_dir) { - tracing::warn!("ignoring {field}: icon path with '..' must resolve under plugin assets/"); - return None; - } - - AbsolutePathBuf::try_from(resolved) - .map_err(|err| { - tracing::warn!("ignoring {field}: icon path must resolve to an absolute path: {err}"); - err - }) - .ok() -} - -fn lexically_normalize(path: &Path) -> PathBuf { - let mut normalized = PathBuf::new(); - for component in path.components() { - match component { - Component::CurDir => {} - Component::ParentDir => { - normalized.pop(); - } - Component::Prefix(_) | Component::RootDir | Component::Normal(_) => { - normalized.push(component.as_os_str()); - } - } - } - normalized -} - fn sanitize_single_line(raw: &str) -> String { raw.split_whitespace().collect::>().join(" ") } @@ -1033,22 +889,6 @@ fn resolve_required_str( resolve_str(Some(value), max_len, field) } -fn resolve_color_str(value: Option, field: &'static str) -> Option { - let value = value?; - let value = value.trim(); - if value.is_empty() { - tracing::warn!("ignoring {field}: value is empty"); - return None; - } - let mut chars = value.chars(); - if value.len() == 7 && chars.next() == Some('#') && chars.all(|c| c.is_ascii_hexdigit()) { - Some(value.to_string()) - } else { - tracing::warn!("ignoring {field}: expected #RRGGBB, got {value}"); - None - } -} - #[cfg(test)] pub(crate) async fn skill_roots_from_layer_stack( fs: Arc, diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index 61d5e62cdb..51185a7c42 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -847,198 +847,6 @@ policy: ); } -#[tokio::test] -async fn accepts_icon_paths_under_assets_dir() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); - let skill_dir = skill_path.parent().expect("skill dir"); - let normalized_skill_dir = normalized(skill_dir); - - write_skill_interface_at( - skill_dir, - r#" -{ - "interface": { - "display_name": "UI Skill", - "icon_small": "assets/icon.png", - "icon_large": "./assets/logo.svg" - } -} -"#, - ); - - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "ui-skill".to_string(), - description: "from json".to_string(), - short_description: None, - interface: Some(SkillInterface { - display_name: Some("UI Skill".to_string()), - short_description: None, - icon_small: Some(normalized_skill_dir.join("assets/icon.png")), - icon_large: Some(normalized_skill_dir.join("assets/logo.svg")), - brand_color: None, - default_prompt: None, - }), - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn ignores_invalid_brand_color() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); - let skill_dir = skill_path.parent().expect("skill dir"); - - write_skill_interface_at( - skill_dir, - r#" -{ - "interface": { - "brand_color": "blue" - } -} -"#, - ); - - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "ui-skill".to_string(), - description: "from json".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn ignores_default_prompt_over_max_length() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); - let skill_dir = skill_path.parent().expect("skill dir"); - let normalized_skill_dir = normalized(skill_dir); - let too_long = "x".repeat(MAX_DEFAULT_PROMPT_LEN + 1); - - write_skill_interface_at( - skill_dir, - &format!( - r##" -{{ - "interface": {{ - "display_name": "UI Skill", - "icon_small": "./assets/small-400px.png", - "default_prompt": "{too_long}" - }} -}} -"## - ), - ); - - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "ui-skill".to_string(), - description: "from json".to_string(), - short_description: None, - interface: Some(SkillInterface { - display_name: Some("UI Skill".to_string()), - short_description: None, - icon_small: Some(normalized_skill_dir.join("assets/small-400px.png")), - icon_large: None, - brand_color: None, - default_prompt: None, - }), - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn drops_interface_when_icons_are_invalid() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); - let skill_dir = skill_path.parent().expect("skill dir"); - - write_skill_interface_at( - skill_dir, - r#" -{ - "interface": { - "icon_small": "icon.png", - "icon_large": "./assets/../logo.svg" - } -} -"#, - ); - - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "ui-skill".to_string(), - description: "from json".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - #[tokio::test] async fn loads_plugin_skill_interface_icons_from_shared_plugin_assets() { let root = tempfile::tempdir().expect("tempdir"); @@ -1110,65 +918,6 @@ interface: ); } -#[tokio::test] -async fn drops_plugin_skill_interface_icons_that_escape_shared_plugin_assets() { - let root = tempfile::tempdir().expect("tempdir"); - let plugin_root = root.path().join("plugins/twilio-developer-kit"); - let skill_path = write_skill_at( - &plugin_root.join("skills"), - "twilio-send-message", - "send-message", - "send messages", - ); - let skill_dir = skill_path.parent().expect("skill dir"); - write_skill_interface_at( - skill_dir, - r##" -interface: - icon_small: "../../other/logo.svg" -"##, - ); - - let outcome = load_skills_from_roots( - [SkillRoot { - path: plugin_root.join("skills").abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: Some(PluginIdentity { - plugin_id: "twilio-developer-kit@test".to_string(), - remote_plugin_id: None, - }), - plugin_namespace: None, - plugin_root: Some(plugin_root.abs()), - discovery_mode: SkillDiscoveryMode::Recursive, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "send-message".to_string(), - description: "send messages".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: Some("twilio-developer-kit@test".to_string()), - remote_plugin_id: None, - }] - ); -} - #[cfg(unix)] fn symlink_dir(target: &Path, link: &Path) { std::os::unix::fs::symlink(target, link).unwrap(); @@ -2141,7 +1890,7 @@ async fn preserves_overlong_short_descriptions() { let codex_home = tempfile::tempdir().expect("tempdir"); let skill_dir = codex_home.path().join("skills/demo"); fs::create_dir_all(&skill_dir).unwrap(); - let too_long = "x".repeat(MAX_SHORT_DESCRIPTION_LEN + 1); + let too_long = "x".repeat(MAX_DESCRIPTION_LEN + 1); let contents = format!( "---\nname: demo-skill\ndescription: long description\nmetadata:\n short-description: {too_long}\n---\n\n# Body\n" ); diff --git a/codex-rs/ext/skills/src/loader/host.rs b/codex-rs/ext/skills/src/loader/host.rs index c76c3a9ecf..9500413ff2 100644 --- a/codex-rs/ext/skills/src/loader/host.rs +++ b/codex-rs/ext/skills/src/loader/host.rs @@ -32,6 +32,7 @@ pub struct HostSkillRoot { pub path: AbsolutePathBuf, pub scope: SkillScope, pub file_system: Arc, + pub plugin_root: Option, } #[derive(Clone, Debug, PartialEq, Eq)] @@ -72,6 +73,10 @@ async fn load_skills_under_root( root: &AbsolutePathBuf, ) -> (Vec, Vec) { let file_system = skill_root.file_system.as_ref(); + let plugin_root = match skill_root.plugin_root.as_ref() { + Some(plugin_root) => Some(canonicalize_for_skill_identity(file_system, plugin_root).await), + None => None, + }; let directory_symlinks = match skill_root.scope { SkillScope::User | SkillScope::Repo | SkillScope::Admin => DirectorySymlinkPolicy::Follow, SkillScope::System => DirectorySymlinkPolicy::Ignore, @@ -138,16 +143,20 @@ async fn load_skills_under_root( namespace_roots, ); let skill_results = futures::stream::iter(resolved_skills) - .map(|skill| async move { - let result = parse_skill_file( - file_system, - &skill.skill, - &skill.path, - &skill.path_uri, - skill_root.scope, - ) - .await; - (skill.path, skill.path_uri, result) + .map(|skill| { + let plugin_root = plugin_root.as_ref(); + async move { + let result = parse_skill_file( + file_system, + &skill.skill, + &skill.path, + &skill.path_uri, + skill_root.scope, + plugin_root, + ) + .await; + (skill.path, skill.path_uri, result) + } }) .buffered(MAX_CONCURRENT_SKILL_LOADS) .collect::>(); @@ -181,6 +190,7 @@ async fn parse_skill_file( path: &AbsolutePathBuf, path_uri: &PathUri, scope: SkillScope, + plugin_root: Option<&AbsolutePathBuf>, ) -> Result { let metadata_path = path_uri .parent() @@ -194,7 +204,7 @@ async fn parse_skill_file( .unwrap_or(SkillMetadataDiscovery::Absent); let (contents, loaded_metadata) = tokio::join!( file_system.read_file_text(path_uri, /*sandbox*/ None), - load_host_skill_metadata(file_system, &metadata), + load_host_skill_metadata(file_system, path, &metadata, plugin_root), ); let contents = contents.map_err(|error| format!("failed to read file: {error}"))?; let ParsedSkillFrontmatter { @@ -204,6 +214,7 @@ async fn parse_skill_file( } = parse_skill_frontmatter_metadata(&contents, || default_skill_name(path)) .map_err(|error| error.to_string())?; let LoadedSkillMetadata { + interface, dependencies, policy, } = loaded_metadata; @@ -212,7 +223,7 @@ async fn parse_skill_file( name, description, short_description, - interface: None, + interface, dependencies, policy, path_to_skills_md: path.clone(), diff --git a/codex-rs/ext/skills/src/loader/host_tests.rs b/codex-rs/ext/skills/src/loader/host_tests.rs index 047a149fc1..8e5fd3be37 100644 --- a/codex-rs/ext/skills/src/loader/host_tests.rs +++ b/codex-rs/ext/skills/src/loader/host_tests.rs @@ -5,6 +5,7 @@ use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_skills::SkillDependencies; +use codex_skills::SkillInterface; use codex_skills::SkillMetadata; use codex_skills::SkillPolicy; use codex_skills::SkillToolDependency; @@ -39,6 +40,53 @@ fn root_for(temp_dir: &TempDir, scope: SkillScope) -> HostSkillRoot { path: AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute root"), scope, file_system: Arc::clone(&LOCAL_FS), + plugin_root: None, + } +} + +struct PluginSkillFixture { + root: TempDir, + plugin_root: AbsolutePathBuf, + skill_path: AbsolutePathBuf, +} + +impl PluginSkillFixture { + fn new() -> Self { + let root = TempDir::new().expect("temp dir"); + let plugin_root = AbsolutePathBuf::from_absolute_path( + fs::canonicalize(root.path()).expect("canonical plugin root"), + ) + .expect("absolute plugin root"); + let skill_path = write_skill( + &root, + "skills/send-message", + "name: send-message\ndescription: Send messages", + ); + Self { + root, + plugin_root, + skill_path, + } + } + + fn write_asset(&self, relative_path: &str) { + let asset_path = self.root.path().join(relative_path); + fs::create_dir_all(asset_path.parent().expect("asset parent")) + .expect("create asset directory"); + fs::write(asset_path, "").expect("write asset"); + } + + fn write_metadata(&self, contents: &str) { + write_metadata(&self.root, "skills/send-message", contents); + } + + fn host_root(&self) -> HostSkillRoot { + HostSkillRoot { + path: self.plugin_root.join("skills"), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_root: Some(self.plugin_root.clone()), + } } } @@ -122,6 +170,163 @@ async fn invalid_optional_metadata_fails_open() { ); } +#[tokio::test] +async fn loads_host_interface_metadata_and_local_asset_paths() { + let root = TempDir::new().expect("temp dir"); + let skill_path = write_skill(&root, "demo", "name: demo\ndescription: Demo skill"); + fs::create_dir_all(root.path().join("demo/assets")).expect("create assets"); + write_metadata( + &root, + "demo", + r##"interface: + display_name: Demo + short_description: Interface summary + icon_small: assets/icon.svg + icon_large: assets/icon-large.png + brand_color: "#123ABC" + default_prompt: Run the demo +"##, + ); + + let snapshot = load_host_skill_root(root_for(&root, SkillScope::User)).await; + let skill_dir = skill_path.parent().expect("skill parent"); + + assert_eq!(snapshot.errors, Vec::new()); + assert_eq!( + snapshot.skills, + vec![SkillMetadata { + name: "demo".to_string(), + description: "Demo skill".to_string(), + short_description: None, + interface: Some(SkillInterface { + display_name: Some("Demo".to_string()), + short_description: Some("Interface summary".to_string()), + icon_small: Some(skill_dir.join("assets/icon.svg")), + icon_large: Some(skill_dir.join("assets/icon-large.png")), + brand_color: Some("#123ABC".to_string()), + default_prompt: Some("Run the demo".to_string()), + }), + dependencies: None, + policy: None, + path_to_skills_md: skill_path, + scope: SkillScope::User, + plugin_id: None, + remote_plugin_id: None, + }] + ); +} + +#[tokio::test] +async fn loads_plugin_skill_interface_icons_from_local_and_shared_assets() { + let fixture = PluginSkillFixture::new(); + fixture.write_asset("skills/send-message/assets/icon.svg"); + fixture.write_asset("assets/logo.svg"); + fixture.write_metadata( + r#"interface: + icon_small: "assets/icon.svg" + icon_large: "../../assets/logo.svg" +"#, + ); + + let snapshot = load_host_skill_root(fixture.host_root()).await; + + assert_eq!(snapshot.errors, Vec::new()); + assert_eq!( + snapshot.skills, + vec![SkillMetadata { + name: "send-message".to_string(), + description: "Send messages".to_string(), + short_description: None, + interface: Some(SkillInterface { + display_name: None, + short_description: None, + icon_small: Some( + fixture + .plugin_root + .join("skills/send-message/assets/icon.svg"), + ), + icon_large: Some(fixture.plugin_root.join("assets/logo.svg")), + brand_color: None, + default_prompt: None, + }), + dependencies: None, + policy: None, + path_to_skills_md: fixture.skill_path, + scope: SkillScope::User, + plugin_id: None, + remote_plugin_id: None, + }] + ); +} + +#[tokio::test] +async fn rejects_plugin_skill_interface_icons_outside_shared_assets() { + let fixture = PluginSkillFixture::new(); + fixture.write_asset("other/icon.svg"); + fixture.write_metadata( + r#"interface: + display_name: Send Message + icon_small: "../../other/icon.svg" +"#, + ); + + let snapshot = load_host_skill_root(fixture.host_root()).await; + + assert_eq!(snapshot.errors, Vec::new()); + assert_eq!( + snapshot.skills, + vec![SkillMetadata { + name: "send-message".to_string(), + description: "Send messages".to_string(), + short_description: None, + interface: Some(SkillInterface { + display_name: Some("Send Message".to_string()), + short_description: None, + icon_small: None, + icon_large: None, + brand_color: None, + default_prompt: None, + }), + dependencies: None, + policy: None, + path_to_skills_md: fixture.skill_path, + scope: SkillScope::User, + plugin_id: None, + remote_plugin_id: None, + }] + ); +} + +#[tokio::test] +async fn rejects_interface_fields_that_escape_or_fail_validation() { + let root = TempDir::new().expect("temp dir"); + let skill_path = write_skill(&root, "demo", "name: demo\ndescription: Demo skill"); + write_metadata( + &root, + "demo", + "interface:\n icon_small: ../outside.svg\n brand_color: blue\n", + ); + + let snapshot = load_host_skill_root(root_for(&root, SkillScope::User)).await; + + assert_eq!(snapshot.errors, Vec::new()); + assert_eq!( + snapshot.skills, + vec![SkillMetadata { + name: "demo".to_string(), + description: "Demo skill".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: skill_path, + scope: SkillScope::User, + plugin_id: None, + remote_plugin_id: None, + }] + ); +} + #[tokio::test] async fn skips_hidden_host_skills() { let root = TempDir::new().expect("temp dir"); diff --git a/codex-rs/ext/skills/src/loader/metadata.rs b/codex-rs/ext/skills/src/loader/metadata.rs index 5c9c15f12f..bb6c17c61f 100644 --- a/codex-rs/ext/skills/src/loader/metadata.rs +++ b/codex-rs/ext/skills/src/loader/metadata.rs @@ -1,12 +1,16 @@ use std::io; -use std::path::PathBuf; use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_skills::SkillDependencies; +use codex_skills::SkillInterface; +use codex_skills::SkillInterfaceAssetPolicy; +use codex_skills::SkillInterfaceFile; use codex_skills::SkillParseError; use codex_skills::SkillPolicy; use codex_skills::SkillToolDependency; +use codex_skills::resolve_skill_interface; +use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use super::MAX_DEPENDENCY_COMMAND_LEN; @@ -20,34 +24,17 @@ use super::discovery::SkillMetadataDiscovery; #[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)] + 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(Default)] pub(super) struct LoadedSkillMetadata { + pub(super) interface: Option, pub(super) dependencies: Option, pub(super) policy: Option, } @@ -79,9 +66,14 @@ struct DependencyTool { pub(super) async fn load_host_skill_metadata( file_system: &dyn ExecutorFileSystem, + skill_path: &AbsolutePathBuf, metadata: &SkillMetadataDiscovery, + plugin_root: Option<&AbsolutePathBuf>, ) -> LoadedSkillMetadata { // Fail open: optional metadata should not block loading SKILL.md. + let Some(skill_dir) = skill_path.parent() else { + return LoadedSkillMetadata::default(); + }; let metadata_path = match metadata { SkillMetadataDiscovery::Present(path) => path, SkillMetadataDiscovery::Absent => return LoadedSkillMetadata::default(), @@ -130,11 +122,16 @@ pub(super) async fn load_host_skill_metadata( }; let SkillMetadataFile { - _interface: _, + interface, dependencies, policy, } = parsed; + let asset_policy = match plugin_root { + Some(plugin_root) => SkillInterfaceAssetPolicy::PluginShared { plugin_root }, + None => SkillInterfaceAssetPolicy::LocalOnly, + }; LoadedSkillMetadata { + interface: resolve_skill_interface(interface, &skill_dir, asset_policy), dependencies: resolve_dependencies(dependencies), policy: resolve_policy(policy), } @@ -221,7 +218,11 @@ pub(super) fn validate_len( Ok(()) } -fn resolve_str(value: Option, max_len: usize, field: &'static str) -> Option { +pub(super) 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() { diff --git a/codex-rs/skills/Cargo.toml b/codex-rs/skills/Cargo.toml index f1c91c43a6..15080dad7d 100644 --- a/codex-rs/skills/Cargo.toml +++ b/codex-rs/skills/Cargo.toml @@ -21,6 +21,7 @@ include_dir = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_yaml = { workspace = true } thiserror = { workspace = true } +tracing = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } diff --git a/codex-rs/skills/src/interface.rs b/codex-rs/skills/src/interface.rs new file mode 100644 index 0000000000..25fb474b2b --- /dev/null +++ b/codex-rs/skills/src/interface.rs @@ -0,0 +1,201 @@ +use std::path::Component; +use std::path::Path; +use std::path::PathBuf; + +use codex_utils_absolute_path::AbsolutePathBuf; +use serde::Deserialize; + +use crate::SkillInterface; + +const MAX_NAME_LEN: usize = 64; +const MAX_DESCRIPTION_LEN: usize = 1024; + +/// Interface metadata deserialized from a skill's `agents/openai.yaml` file. +#[derive(Debug, Default, Deserialize)] +pub struct SkillInterfaceFile { + display_name: Option, + short_description: Option, + icon_small: Option, + icon_large: Option, + brand_color: Option, + default_prompt: Option, +} + +/// Controls whether interface icons may resolve through a plugin's shared assets directory. +#[derive(Debug, Clone, Copy)] +pub enum SkillInterfaceAssetPolicy<'a> { + LocalOnly, + PluginShared { plugin_root: &'a AbsolutePathBuf }, +} + +/// Validates skill interface metadata and resolves its asset paths. +pub fn resolve_skill_interface( + interface: Option, + skill_dir: &AbsolutePathBuf, + asset_policy: SkillInterfaceAssetPolicy<'_>, +) -> Option { + let interface = interface?; + let interface = SkillInterface { + display_name: resolve_str( + interface.display_name, + MAX_NAME_LEN, + "interface.display_name", + ), + short_description: resolve_str( + interface.short_description, + MAX_DESCRIPTION_LEN, + "interface.short_description", + ), + icon_small: resolve_asset_path( + skill_dir, + asset_policy, + "interface.icon_small", + interface.icon_small, + ), + icon_large: resolve_asset_path( + skill_dir, + asset_policy, + "interface.icon_large", + interface.icon_large, + ), + brand_color: resolve_color_str(interface.brand_color, "interface.brand_color"), + default_prompt: resolve_str( + interface.default_prompt, + MAX_DESCRIPTION_LEN, + "interface.default_prompt", + ), + }; + let has_fields = interface.display_name.is_some() + || interface.short_description.is_some() + || interface.icon_small.is_some() + || interface.icon_large.is_some() + || interface.brand_color.is_some() + || interface.default_prompt.is_some(); + has_fields.then_some(interface) +} + +fn resolve_asset_path( + skill_dir: &AbsolutePathBuf, + asset_policy: SkillInterfaceAssetPolicy<'_>, + field: &'static str, + path: Option, +) -> Option { + let path = path?; + if path.as_os_str().is_empty() { + return None; + } + + let assets_dir = skill_dir.join("assets"); + if path.is_absolute() { + tracing::warn!( + "ignoring {field}: icon must be a relative assets path (not {})", + assets_dir.display() + ); + return None; + } + + let mut normalized = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::Normal(component) => normalized.push(component), + Component::ParentDir => { + return resolve_plugin_shared_asset_path(skill_dir, asset_policy, field, &path); + } + Component::Prefix(_) | Component::RootDir => { + tracing::warn!("ignoring {field}: icon path must be under assets/"); + return None; + } + } + } + + let mut components = normalized.components(); + match components.next() { + Some(Component::Normal(component)) if component == "assets" => {} + _ => { + tracing::warn!("ignoring {field}: icon path must be under assets/"); + return None; + } + } + + Some(skill_dir.join(normalized)) +} + +fn resolve_plugin_shared_asset_path( + skill_dir: &AbsolutePathBuf, + asset_policy: SkillInterfaceAssetPolicy<'_>, + field: &'static str, + path: &Path, +) -> Option { + let SkillInterfaceAssetPolicy::PluginShared { plugin_root } = asset_policy else { + tracing::warn!("ignoring {field}: icon path must not contain '..'"); + return None; + }; + + let plugin_assets_dir = lexically_normalize(plugin_root.join("assets").as_path()); + let resolved = lexically_normalize(skill_dir.join(path).as_path()); + if !resolved.starts_with(&plugin_assets_dir) { + tracing::warn!("ignoring {field}: icon path with '..' must resolve under plugin assets/"); + return None; + } + + AbsolutePathBuf::try_from(resolved) + .map_err(|error| { + tracing::warn!("ignoring {field}: icon path must resolve to an absolute path: {error}"); + error + }) + .ok() +} + +fn lexically_normalize(path: &Path) -> PathBuf { + let mut normalized = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::ParentDir => { + normalized.pop(); + } + Component::Prefix(_) | Component::RootDir | Component::Normal(_) => { + normalized.push(component.as_os_str()); + } + } + } + normalized +} + +fn resolve_str(value: Option, max_len: usize, field: &'static str) -> Option { + let value = value?; + let value = value.split_whitespace().collect::>().join(" "); + 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_color_str(value: Option, field: &'static str) -> Option { + let value = value?; + let value = value.trim(); + if value.is_empty() { + tracing::warn!("ignoring {field}: value is empty"); + return None; + } + let mut chars = value.chars(); + if value.len() == 7 + && chars.next() == Some('#') + && chars.all(|character| character.is_ascii_hexdigit()) + { + Some(value.to_string()) + } else { + tracing::warn!("ignoring {field}: expected #RRGGBB, got {value}"); + None + } +} + +#[cfg(test)] +#[path = "interface_tests.rs"] +mod tests; diff --git a/codex-rs/skills/src/interface_tests.rs b/codex-rs/skills/src/interface_tests.rs new file mode 100644 index 0000000000..872c977f4f --- /dev/null +++ b/codex-rs/skills/src/interface_tests.rs @@ -0,0 +1,165 @@ +use std::path::PathBuf; + +use codex_utils_absolute_path::AbsolutePathBuf; +use pretty_assertions::assert_eq; + +use super::SkillInterfaceAssetPolicy; +use super::SkillInterfaceFile; +use super::resolve_skill_interface; +use crate::SkillInterface; + +fn absolute_path(path: &str) -> AbsolutePathBuf { + AbsolutePathBuf::try_from( + std::env::current_dir() + .expect("current directory") + .join(path), + ) + .expect("absolute path") +} + +#[test] +fn resolves_local_interface_fields_and_assets() { + let skill_dir = absolute_path("skill"); + let interface = SkillInterfaceFile { + display_name: Some(" Demo skill ".to_string()), + short_description: Some(" Short description ".to_string()), + icon_small: Some(PathBuf::from("./assets/icon.svg")), + icon_large: Some(PathBuf::from("assets/icon-large.png")), + brand_color: Some(" #123ABC ".to_string()), + default_prompt: Some(" Run the demo ".to_string()), + }; + + assert_eq!( + resolve_skill_interface( + Some(interface), + &skill_dir, + SkillInterfaceAssetPolicy::LocalOnly, + ), + Some(SkillInterface { + display_name: Some("Demo skill".to_string()), + short_description: Some("Short description".to_string()), + icon_small: Some(skill_dir.join("assets/icon.svg")), + icon_large: Some(skill_dir.join("assets/icon-large.png")), + brand_color: Some("#123ABC".to_string()), + default_prompt: Some("Run the demo".to_string()), + }) + ); +} + +#[test] +fn rejects_invalid_local_interface_fields() { + let skill_dir = absolute_path("skill"); + let interface = SkillInterfaceFile { + display_name: None, + short_description: None, + icon_small: Some(PathBuf::from("../outside.svg")), + icon_large: Some(PathBuf::from("icon.png")), + brand_color: Some("blue".to_string()), + default_prompt: Some("x".repeat(1025)), + }; + + assert_eq!( + resolve_skill_interface( + Some(interface), + &skill_dir, + SkillInterfaceAssetPolicy::LocalOnly, + ), + None + ); +} + +#[test] +fn rejects_absolute_asset_paths() { + let skill_dir = absolute_path("skill"); + let interface = SkillInterfaceFile { + icon_small: Some(absolute_path("outside.svg").as_path().to_path_buf()), + ..Default::default() + }; + + assert_eq!( + resolve_skill_interface( + Some(interface), + &skill_dir, + SkillInterfaceAssetPolicy::LocalOnly, + ), + None + ); +} + +#[test] +fn drops_invalid_fields_without_discarding_valid_interface_fields() { + let skill_dir = absolute_path("skill"); + let interface = SkillInterfaceFile { + display_name: Some("Demo skill".to_string()), + short_description: None, + icon_small: Some(PathBuf::from("assets/icon.svg")), + icon_large: Some(PathBuf::from("icon.png")), + brand_color: Some("blue".to_string()), + default_prompt: Some("x".repeat(1025)), + }; + + assert_eq!( + resolve_skill_interface( + Some(interface), + &skill_dir, + SkillInterfaceAssetPolicy::LocalOnly, + ), + Some(SkillInterface { + display_name: Some("Demo skill".to_string()), + short_description: None, + icon_small: Some(skill_dir.join("assets/icon.svg")), + icon_large: None, + brand_color: None, + default_prompt: None, + }) + ); +} + +#[test] +fn resolves_plugin_shared_assets() { + let plugin_root = absolute_path("plugin"); + let skill_dir = plugin_root.join("skills/demo"); + let interface = SkillInterfaceFile { + icon_small: Some(PathBuf::from("../../assets/icon.svg")), + ..Default::default() + }; + + assert_eq!( + resolve_skill_interface( + Some(interface), + &skill_dir, + SkillInterfaceAssetPolicy::PluginShared { + plugin_root: &plugin_root, + }, + ), + Some(SkillInterface { + display_name: None, + short_description: None, + icon_small: Some(plugin_root.join("assets/icon.svg")), + icon_large: None, + brand_color: None, + default_prompt: None, + }) + ); +} + +#[test] +fn rejects_plugin_assets_outside_shared_assets_root() { + let plugin_root = absolute_path("plugin"); + let skill_dir = plugin_root.join("skills/demo"); + let interface = SkillInterfaceFile { + icon_small: Some(PathBuf::from("../../other/icon.svg")), + ..Default::default() + }; + + assert_eq!( + resolve_skill_interface( + Some(interface), + &skill_dir, + SkillInterfaceAssetPolicy::PluginShared { + plugin_root: &plugin_root, + }, + ), + None + ); +} diff --git a/codex-rs/skills/src/lib.rs b/codex-rs/skills/src/lib.rs index c6e88f01bc..cc3cc948de 100644 --- a/codex-rs/skills/src/lib.rs +++ b/codex-rs/skills/src/lib.rs @@ -1,6 +1,10 @@ +mod interface; mod model; mod parser; +pub use interface::SkillInterfaceAssetPolicy; +pub use interface::SkillInterfaceFile; +pub use interface::resolve_skill_interface; pub use model::EnvironmentSkillMetadata; pub use model::SkillConfigRule; pub use model::SkillConfigRuleSelector;