From 1e6b77fde4cd491c8755b7ad3c69f89f000abd45 Mon Sep 17 00:00:00 2001 From: Xin Lin Date: Wed, 20 May 2026 15:45:12 -0700 Subject: [PATCH] fix: Allow plugin skills to share plugin-level icon assets --- codex-rs/core-plugins/src/loader.rs | 1 + codex-rs/core-skills/src/loader.rs | 84 ++++++++++++++-- codex-rs/core-skills/src/loader_tests.rs | 116 ++++++++++++++++++++++ codex-rs/core-skills/src/manager_tests.rs | 33 +++--- codex-rs/plugin/src/load_outcome.rs | 2 + codex-rs/utils/plugins/src/lib.rs | 1 + 6 files changed, 215 insertions(+), 22 deletions(-) diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index fc4193aa67bf..cfd2e67aa64c 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -660,6 +660,7 @@ pub async fn load_plugin_skills( scope: SkillScope::User, file_system: Arc::clone(&LOCAL_FS), plugin_id: Some(plugin_id.as_key()), + plugin_root: Some(plugin_root.clone()), }) .collect::>(); let outcome = load_skills_from_roots(roots).await; diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 2473f7108cf9..00d1cbba1421 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -30,6 +30,7 @@ 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 toml::Value as TomlValue; @@ -154,6 +155,7 @@ pub struct SkillRoot { pub scope: SkillScope, pub file_system: Arc, pub plugin_id: Option, + pub plugin_root: Option, } pub async fn load_skills_from_roots(roots: I) -> SkillLoadOutcome @@ -174,6 +176,7 @@ where &root_path, root.scope, root.plugin_id.as_deref(), + root.plugin_root.as_ref(), &mut outcome, ) .await; @@ -258,6 +261,7 @@ async fn skill_roots_with_home_dir( scope: SkillScope::User, file_system: Arc::clone(&LOCAL_FS), plugin_id: Some(root.plugin_id), + plugin_root: Some(root.plugin_root), })); roots.extend(repo_agents_skill_roots(fs, config_layer_stack, cwd).await); dedupe_skill_roots_by_path(&mut roots); @@ -287,6 +291,7 @@ fn skill_roots_from_layer_stack_inner( scope: SkillScope::Repo, file_system: Arc::clone(repo_fs), plugin_id: None, + plugin_root: None, }); } } @@ -298,6 +303,7 @@ fn skill_roots_from_layer_stack_inner( scope: SkillScope::User, file_system: Arc::clone(&LOCAL_FS), plugin_id: None, + plugin_root: None, }); // `$HOME/.agents/skills` (user-installed skills). @@ -307,6 +313,7 @@ fn skill_roots_from_layer_stack_inner( scope: SkillScope::User, file_system: Arc::clone(&LOCAL_FS), plugin_id: None, + plugin_root: None, }); } @@ -317,6 +324,7 @@ fn skill_roots_from_layer_stack_inner( scope: SkillScope::System, file_system: Arc::clone(&LOCAL_FS), plugin_id: None, + plugin_root: None, }); } ConfigLayerSource::System { .. } => { @@ -327,6 +335,7 @@ fn skill_roots_from_layer_stack_inner( scope: SkillScope::Admin, file_system: Arc::clone(&LOCAL_FS), plugin_id: None, + plugin_root: None, }); } ConfigLayerSource::Mdm { .. } @@ -359,6 +368,7 @@ async fn repo_agents_skill_roots( scope: SkillScope::Repo, file_system: Arc::clone(&fs), plugin_id: None, + plugin_root: None, }), Ok(_) => {} Err(err) if err.kind() == io::ErrorKind::NotFound => {} @@ -458,9 +468,11 @@ async fn discover_skills_under_root( root: &AbsolutePathBuf, scope: SkillScope, plugin_id: Option<&str>, + plugin_root: Option<&AbsolutePathBuf>, outcome: &mut SkillLoadOutcome, ) { let root = canonicalize_for_skill_identity(root); + let plugin_root = plugin_root.map(canonicalize_for_skill_identity); match fs.get_metadata(&root, /*sandbox*/ None).await { Ok(metadata) if metadata.is_directory => {} @@ -570,7 +582,7 @@ async fn discover_skills_under_root( } if metadata.is_file && file_name == SKILLS_FILENAME { - match parse_skill_file(fs, &path, scope, plugin_id).await { + match parse_skill_file(fs, &path, scope, plugin_id, plugin_root.as_ref()).await { Ok(skill) => { outcome.skills.push(skill); } @@ -601,6 +613,7 @@ async fn parse_skill_file( path: &AbsolutePathBuf, scope: SkillScope, plugin_id: Option<&str>, + plugin_root: Option<&AbsolutePathBuf>, ) -> Result { let contents = fs .read_file_text(path, /*sandbox*/ None) @@ -634,7 +647,7 @@ async fn parse_skill_file( interface, dependencies, policy, - } = load_skill_metadata(fs, path).await; + } = load_skill_metadata(fs, path, plugin_root).await; validate_len(&name, MAX_NAME_LEN, "name")?; validate_len(&description, MAX_DESCRIPTION_LEN, "description")?; @@ -687,6 +700,7 @@ async fn namespaced_skill_name( async fn load_skill_metadata( fs: &dyn ExecutorFileSystem, skill_path: &AbsolutePathBuf, + plugin_root: Option<&AbsolutePathBuf>, ) -> LoadedSkillMetadata { // Fail open: optional metadata should not block loading SKILL.md. let Some(skill_dir) = skill_path.parent() else { @@ -744,7 +758,7 @@ async fn load_skill_metadata( policy, } = parsed; LoadedSkillMetadata { - interface: resolve_interface(interface, &skill_dir), + interface: resolve_interface(interface, &skill_dir, plugin_root), dependencies: resolve_dependencies(dependencies), policy: resolve_policy(policy), } @@ -753,6 +767,7 @@ async fn load_skill_metadata( fn resolve_interface( interface: Option, skill_dir: &AbsolutePathBuf, + plugin_root: Option<&AbsolutePathBuf>, ) -> Option { let interface = interface?; let interface = SkillInterface { @@ -766,8 +781,18 @@ fn resolve_interface( MAX_SHORT_DESCRIPTION_LEN, "interface.short_description", ), - icon_small: resolve_asset_path(skill_dir, "interface.icon_small", interface.icon_small), - icon_large: resolve_asset_path(skill_dir, "interface.icon_large", interface.icon_large), + 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, @@ -845,10 +870,12 @@ 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 be relative paths under the skill's assets/ directory; otherwise return None. + // 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; @@ -869,8 +896,7 @@ fn resolve_asset_path( Component::CurDir => {} Component::Normal(component) => normalized.push(component), Component::ParentDir => { - tracing::warn!("ignoring {field}: icon path must not contain '..'"); - return None; + return resolve_plugin_shared_asset_path(skill_dir, plugin_root, field, &path); } _ => { tracing::warn!("ignoring {field}: icon path must be under assets/"); @@ -891,6 +917,48 @@ fn resolve_asset_path( 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(" ") } diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index a1d03dead2e7..84867ca66bfd 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -822,6 +822,116 @@ async fn drops_interface_when_icons_are_invalid() { ); } +#[tokio::test] +async fn loads_plugin_skill_interface_icons_from_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"); + fs::create_dir_all(plugin_root.join("assets")).unwrap(); + fs::write(plugin_root.join("assets/logo.svg"), "").unwrap(); + write_skill_interface_at( + skill_dir, + r##" +interface: + icon_small: "../../assets/logo.svg" + icon_large: "../../assets/logo.svg" +"##, + ); + + let plugin_root_abs = plugin_root.abs(); + let outcome = load_skills_from_roots([SkillRoot { + path: plugin_root.join("skills").abs(), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: Some("twilio-developer-kit@test".to_string()), + plugin_root: Some(plugin_root_abs.clone()), + }]) + .await; + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + let expected_icon_path = normalized(&plugin_root.join("assets/logo.svg")); + assert_eq!( + outcome.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(expected_icon_path.clone()), + icon_large: Some(expected_icon_path), + brand_color: None, + default_prompt: None, + }), + dependencies: None, + policy: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + plugin_id: Some("twilio-developer-kit@test".to_string()), + }] + ); +} + +#[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_id: Some("twilio-developer-kit@test".to_string()), + plugin_root: Some(plugin_root.abs()), + }]) + .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()), + }] + ); +} + #[cfg(unix)] fn symlink_dir(target: &Path, link: &Path) { std::os::unix::fs::symlink(target, link).unwrap(); @@ -943,6 +1053,7 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() { scope: SkillScope::Admin, file_system: Arc::clone(&LOCAL_FS), plugin_id: None, + plugin_root: None, }]) .await; @@ -1024,6 +1135,7 @@ async fn system_scope_ignores_symlinked_subdir() { scope: SkillScope::System, file_system: Arc::clone(&LOCAL_FS), plugin_id: None, + plugin_root: None, }]) .await; assert!( @@ -1057,6 +1169,7 @@ async fn respects_max_scan_depth_for_user_scope() { scope: SkillScope::User, file_system: Arc::clone(&LOCAL_FS), plugin_id: None, + plugin_root: None, }]) .await; @@ -1163,6 +1276,7 @@ async fn namespaces_plugin_skills_using_plugin_name() { scope: SkillScope::User, file_system: Arc::clone(&LOCAL_FS), plugin_id: Some("sample@test".to_string()), + plugin_root: Some(plugin_root.abs()), }]) .await; @@ -1485,12 +1599,14 @@ async fn deduplicates_by_path_preferring_first_root() { scope: SkillScope::Repo, file_system: Arc::clone(&LOCAL_FS), plugin_id: None, + plugin_root: None, }, SkillRoot { path: root.path().abs(), scope: SkillScope::User, file_system: Arc::clone(&LOCAL_FS), plugin_id: None, + plugin_root: None, }, ]) .await; diff --git a/codex-rs/core-skills/src/manager_tests.rs b/codex-rs/core-skills/src/manager_tests.rs index 15b06b393a48..4afa83e21ef7 100644 --- a/codex-rs/core-skills/src/manager_tests.rs +++ b/codex-rs/core-skills/src/manager_tests.rs @@ -15,6 +15,7 @@ use codex_utils_plugins::PluginSkillRoot; use pretty_assertions::assert_eq; use std::collections::HashSet; use std::fs; +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use tempfile::TempDir; @@ -54,6 +55,21 @@ fn write_plugin_skill( skill_path } +fn plugin_skill_root_for_skill_path(skill_path: &Path, plugin_id: &str) -> PluginSkillRoot { + let skills_root = skill_path + .parent() + .and_then(Path::parent) + .expect("plugin skill should live under a skills root"); + let plugin_root = skills_root + .parent() + .expect("plugin skills root should live under a plugin root"); + PluginSkillRoot { + path: skills_root.abs(), + plugin_id: plugin_id.to_string(), + plugin_root: plugin_root.abs(), + } +} + fn test_skill(name: &str, path: PathBuf) -> SkillMetadata { SkillMetadata { name: name.to_string(), @@ -146,18 +162,11 @@ async fn skills_for_config_with_stack( skills_manager: &SkillsManager, cwd: &TempDir, config_layer_stack: &ConfigLayerStack, - effective_skill_roots: &[AbsolutePathBuf], + effective_skill_roots: &[PluginSkillRoot], ) -> SkillLoadOutcome { let skills_input = SkillsLoadInput::new( cwd.path().abs(), - effective_skill_roots - .iter() - .cloned() - .map(|path| PluginSkillRoot { - path, - plugin_id: "test-plugin@test".to_string(), - }) - .collect(), + effective_skill_roots.to_vec(), config_layer_stack.clone(), bundled_skills_enabled_from_stack(config_layer_stack), ); @@ -228,11 +237,7 @@ async fn skills_for_config_disables_plugin_skills_by_name() { &codex_home, &name_toggle_config("sample:sample-search", /*enabled*/ false), ); - let plugin_skill_root = skill_path - .parent() - .and_then(std::path::Path::parent) - .expect("plugin skill should live under a skills root") - .abs(); + let plugin_skill_root = plugin_skill_root_for_skill_path(&skill_path, "test-plugin@test"); let skills_manager = SkillsManager::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, diff --git a/codex-rs/plugin/src/load_outcome.rs b/codex-rs/plugin/src/load_outcome.rs index c76697366f01..2588ee0a7f94 100644 --- a/codex-rs/plugin/src/load_outcome.rs +++ b/codex-rs/plugin/src/load_outcome.rs @@ -126,6 +126,7 @@ impl PluginLoadOutcome { skill_roots.push(PluginSkillRoot { path: path.clone(), plugin_id: plugin.config_name.clone(), + plugin_root: plugin.root.clone(), }); } } @@ -245,6 +246,7 @@ mod tests { vec![PluginSkillRoot { path: shared_root, plugin_id: "zeta@test".to_string(), + plugin_root: test_path("zeta@test"), }] ); } diff --git a/codex-rs/utils/plugins/src/lib.rs b/codex-rs/utils/plugins/src/lib.rs index dec24d99d856..38bf68040344 100644 --- a/codex-rs/utils/plugins/src/lib.rs +++ b/codex-rs/utils/plugins/src/lib.rs @@ -14,4 +14,5 @@ pub use plugin_namespace::plugin_namespace_for_skill_path; pub struct PluginSkillRoot { pub path: AbsolutePathBuf, pub plugin_id: String, + pub plugin_root: AbsolutePathBuf, }