fix: Allow plugin skills to share plugin-level icon assets (#23776)

Thread the plugin root through plugin skill loading so skill interface
icons can reference shared plugin assets, such as ../../assets/logo.svg.

codex-rs/core-plugins/src/loader.rs

@@ -655,6 +655,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::<Vec<_>>();
     let outcome = load_skills_from_roots(roots).await;

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<dyn ExecutorFileSystem>,
     pub plugin_id: Option<String>,
+    pub plugin_root: Option<AbsolutePathBuf>,
 }
 
 pub async fn load_skills_from_roots<I>(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<SkillMetadata, SkillParseError> {
     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<Interface>,
     skill_dir: &AbsolutePathBuf,
+    plugin_root: Option<&AbsolutePathBuf>,
 ) -> Option<SkillInterface> {
     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<SkillToolDependency>
 
 fn resolve_asset_path(
     skill_dir: &AbsolutePathBuf,
+    plugin_root: Option<&AbsolutePathBuf>,
     field: &'static str,
     path: Option<PathBuf>,
 ) -> Option<AbsolutePathBuf> {
-    // 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<AbsolutePathBuf> {
+    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::<Vec<_>>().join(" ")
 }

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"), "<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;

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,

codex-rs/plugin/src/load_outcome.rs

@@ -126,6 +126,7 @@ impl<M: Clone> PluginLoadOutcome<M> {
                     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"),
             }]
         );
     }

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,
 }