From e12dd73b7d5a2aa2b8d0933a2053e7eb5eba6fbb Mon Sep 17 00:00:00 2001 From: charlesgong-openai Date: Wed, 17 Jun 2026 21:33:53 -0700 Subject: [PATCH] [codex] Support plugin manifest path lists (#28790) ## Summary Allow plugin manifests to declare `skills` as either a single path string or an array of path strings in the core plugin loader. ## Why Some plugin packages need to expose skills from more than one directory. Before this change, `plugin.json` only accepted a single string for `skills`, so manifests like this were ignored as an invalid `skills` shape: ```json { "skills": ["./skills/abc", "./skills/edk"] } ``` This keeps the existing single-string form working while adding support for the list form. The final scope is intentionally limited to the core plugin manifest/load path for `skills`; `apps`, file-backed `mcpServers`, and the bundled plugin-creator assets are unchanged in this PR. ## What changed - Parse `skills` as either a string or an array of strings in `plugin.json`. - Store resolved skill paths as a list in `PluginManifestPaths`. - Load manifest-declared skill roots in addition to the default `./skills` root. - Deduplicate exact duplicate skill roots before loading. - Rely on existing skill-loader dedupe by canonical `SKILL.md` path for overlapping roots such as `./skills` plus `./skills/abc`. - Update plugin manifest tests to cover: - single string `skills` - list of string `skills` - duplicate skill roots - `./skills` as a manifest path - explicit child roots like `./skills/abc` and `./skills/edk` - overlapping-root dedupe ## Validation - `just test -p codex-plugin` - `just test -p codex-core-plugins` - `just test -p codex-mcp-extension` - `git diff --check` --- codex-rs/core-plugins/src/loader.rs | 9 +- codex-rs/core-plugins/src/manager_tests.rs | 242 ++++++++++++------ codex-rs/core-plugins/src/manifest.rs | 32 ++- .../mcp/src/executor_plugin/provider_tests.rs | 2 +- codex-rs/plugin/src/manifest.rs | 7 +- codex-rs/plugin/src/provider_tests.rs | 6 +- 6 files changed, 202 insertions(+), 96 deletions(-) diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index fb542f045..846487aba 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -819,10 +819,11 @@ fn plugin_skill_roots( plugin_root: &AbsolutePathBuf, manifest_paths: &PluginManifestPaths, ) -> Vec { - let mut paths = default_skill_roots(plugin_root); - if let Some(path) = &manifest_paths.skills { - paths.push(path.clone()); - } + let mut paths = if manifest_paths.skills.is_empty() { + default_skill_roots(plugin_root) + } else { + manifest_paths.skills.clone() + }; paths.sort_unstable(); paths.dedup(); paths diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index c2917211d..b5d22911f 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -4,6 +4,7 @@ use crate::OPENAI_API_CURATED_MARKETPLACE_NAME; use crate::OPENAI_CURATED_MARKETPLACE_NAME; use crate::PluginLoadOutcome; use crate::installed_marketplaces::marketplace_install_root; +use crate::loader::load_plugin_skills; use crate::loader::load_plugins_from_layer_stack; use crate::loader::refresh_non_curated_plugin_cache; use crate::loader::refresh_non_curated_plugin_cache_force_reinstall; @@ -33,10 +34,13 @@ use codex_config::McpServerConfig; use codex_config::McpServerOAuthConfig; use codex_config::McpServerToolConfig; use codex_config::types::McpServerTransportConfig; +use codex_core_skills::config_rules::SkillConfigRules; use codex_login::CodexAuth; use codex_plugin::AppDeclaration; +use codex_plugin::PluginId; use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::Product; +use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; use pretty_assertions::assert_eq; use std::fs; @@ -1282,32 +1286,62 @@ async fn capability_summary_truncates_overlong_plugin_descriptions() { #[tokio::test] async fn load_plugins_uses_manifest_configured_component_paths() { - let codex_home = TempDir::new().unwrap(); - let plugin_root = codex_home - .path() - .join("plugins/cache") - .join("test/sample/local"); + for (skills_json, expected_skill_dirs) in [ + (r#""./custom-skills/""#, &["custom-skills"][..]), + ( + r#"["./custom-skills/", "./extra-skills/"]"#, + &["custom-skills", "extra-skills"][..], + ), + ( + r#"["./custom-skills/", "./custom-skills/"]"#, + &["custom-skills"][..], + ), + (r#""./skills/""#, &["skills"][..]), + ( + r#"["./skills/abc/", "./skills/edk/"]"#, + &["skills/abc", "skills/edk"][..], + ), + ] { + let codex_home = TempDir::new().unwrap(); + let plugin_root = codex_home + .path() + .join("plugins/cache") + .join("test/sample/local"); - write_file( - &plugin_root.join(".codex-plugin/plugin.json"), - r#"{ + write_file( + &plugin_root.join(".codex-plugin/plugin.json"), + &format!( + r#"{{ "name": "sample", - "skills": "./custom-skills/", + "skills": {skills_json}, "mcpServers": "./config/custom.mcp.json", "apps": "./config/custom.app.json" -}"#, - ); - write_file( - &plugin_root.join("skills/default-skill/SKILL.md"), - "---\nname: default-skill\ndescription: default skill\n---\n", - ); - write_file( - &plugin_root.join("custom-skills/custom-skill/SKILL.md"), - "---\nname: custom-skill\ndescription: custom skill\n---\n", - ); - write_file( - &plugin_root.join(".mcp.json"), - r#"{ +}}"# + ), + ); + write_file( + &plugin_root.join("skills/default-skill/SKILL.md"), + "---\nname: default-skill\ndescription: default skill\n---\n", + ); + write_file( + &plugin_root.join("skills/abc/SKILL.md"), + "---\nname: abc\ndescription: abc skill\n---\n", + ); + write_file( + &plugin_root.join("skills/edk/SKILL.md"), + "---\nname: edk\ndescription: edk skill\n---\n", + ); + write_file( + &plugin_root.join("custom-skills/custom-skill/SKILL.md"), + "---\nname: custom-skill\ndescription: custom skill\n---\n", + ); + write_file( + &plugin_root.join("extra-skills/extra-skill/SKILL.md"), + "---\nname: extra-skill\ndescription: extra skill\n---\n", + ); + write_file( + &plugin_root.join(".mcp.json"), + r#"{ "mcpServers": { "default": { "type": "http", @@ -1315,10 +1349,10 @@ async fn load_plugins_uses_manifest_configured_component_paths() { } } }"#, - ); - write_file( - &plugin_root.join("config/custom.mcp.json"), - r#"{ + ); + write_file( + &plugin_root.join("config/custom.mcp.json"), + r#"{ "mcpServers": { "custom": { "type": "http", @@ -1326,74 +1360,132 @@ async fn load_plugins_uses_manifest_configured_component_paths() { } } }"#, - ); - write_file( - &plugin_root.join(".app.json"), - r#"{ + ); + write_file( + &plugin_root.join(".app.json"), + r#"{ "apps": { "default-app": { "id": "connector_default" } } }"#, - ); - write_file( - &plugin_root.join("config/custom.app.json"), - r#"{ + ); + write_file( + &plugin_root.join("config/custom.app.json"), + r#"{ "apps": { "custom-app": { "id": "connector_custom" } } }"#, - ); + ); + let outcome = load_plugins_from_config( + &plugin_config_toml(/*enabled*/ true, /*plugins_feature_enabled*/ true), + codex_home.path(), + Some(AuthMode::Chatgpt), + ) + .await; + let mut expected_skill_roots = expected_skill_dirs + .iter() + .map(|dir| plugin_root.join(dir).abs()) + .collect::>(); + expected_skill_roots.sort_unstable(); + expected_skill_roots.dedup(); - let outcome = load_plugins_from_config( - &plugin_config_toml(/*enabled*/ true, /*plugins_feature_enabled*/ true), - codex_home.path(), - Some(AuthMode::Chatgpt), + assert_eq!(outcome.plugins()[0].skill_roots, expected_skill_roots); + assert_eq!( + outcome.plugins()[0].mcp_servers, + HashMap::from([( + "custom".to_string(), + McpServerConfig { + transport: McpServerTransportConfig::StreamableHttp { + url: "https://custom.example/mcp".to_string(), + bearer_token_env_var: None, + http_headers: None, + env_http_headers: None, + }, + environment_id: "local".to_string(), + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::new(), + }, + )]) + ); + assert_eq!( + outcome.plugins()[0].apps, + vec![app_declaration("custom-app", "connector_custom")] + ); + } +} + +#[tokio::test] +async fn load_plugin_skills_dedupes_overlapping_manifest_roots() { + let codex_home = TempDir::new().unwrap(); + let plugin_root = codex_home + .path() + .join("plugins/cache") + .join("test/sample/local") + .abs(); + write_file( + &plugin_root.join("skills/abc/SKILL.md"), + "---\nname: abc\ndescription: abc skill\n---\n", + ); + write_file( + &plugin_root.join("skills/edk/SKILL.md"), + "---\nname: edk\ndescription: edk skill\n---\n", + ); + let manifest_paths = crate::manifest::PluginManifestPaths { + skills: vec![ + plugin_root.join("skills"), + plugin_root.join("skills/abc"), + plugin_root.join("skills/edk"), + plugin_root.join("skills/abc"), + ], + mcp_servers: None, + apps: None, + hooks: None, + }; + let plugin_id = PluginId::parse("sample@test").expect("plugin id should parse"); + + let resolved = load_plugin_skills( + &plugin_root, + &plugin_id, + &manifest_paths, + /*restriction_product*/ None, + &SkillConfigRules::default(), ) .await; + let skill_paths = resolved + .skills + .iter() + .map(|skill| skill.path_to_skills_md.clone()) + .collect::>(); + let canonical_skill_path = |path| { + AbsolutePathBuf::from_absolute_path_checked( + fs::canonicalize(plugin_root.join(path)).expect("canonical skill path"), + ) + .expect("absolute skill path") + }; assert_eq!( - outcome.plugins()[0].skill_roots, + skill_paths, vec![ - plugin_root.join("custom-skills").abs(), - plugin_root.join("skills").abs() + canonical_skill_path("skills/abc/SKILL.md"), + canonical_skill_path("skills/edk/SKILL.md") ] ); - assert_eq!( - outcome.plugins()[0].mcp_servers, - HashMap::from([( - "custom".to_string(), - McpServerConfig { - transport: McpServerTransportConfig::StreamableHttp { - url: "https://custom.example/mcp".to_string(), - bearer_token_env_var: None, - http_headers: None, - env_http_headers: None, - }, - environment_id: "local".to_string(), - enabled: true, - required: false, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth: None, - oauth_resource: None, - tools: HashMap::new(), - }, - )]) - ); - assert_eq!( - outcome.plugins()[0].apps, - vec![app_declaration("custom-app", "connector_custom")] - ); } #[tokio::test] @@ -1521,7 +1613,7 @@ async fn load_plugins_ignores_invalid_manifest_skills_shape() { &plugin_root.join(".codex-plugin/plugin.json"), r#"{ "name": "sample", - "skills": ["./custom-skills/"] + "skills": { "path": "./custom-skills/" } }"#, ); write_file( diff --git a/codex-rs/core-plugins/src/manifest.rs b/codex-rs/core-plugins/src/manifest.rs index fe0bb2564..37630a843 100644 --- a/codex-rs/core-plugins/src/manifest.rs +++ b/codex-rs/core-plugins/src/manifest.rs @@ -30,7 +30,7 @@ struct RawPluginManifest { // Keep manifest paths as raw strings so we can validate the required `./...` syntax before // resolving them under the plugin root. #[serde(default)] - skills: Option, + skills: Option, #[serde(default)] mcp_servers: Option, #[serde(default)] @@ -94,8 +94,9 @@ enum RawPluginManifestDefaultPromptEntry { #[derive(Debug, Deserialize)] #[serde(untagged)] -enum RawPluginManifestPath { +enum RawPluginManifestPaths { Path(String), + Paths(Vec), Invalid(JsonValue), } @@ -230,7 +231,7 @@ pub(crate) fn parse_plugin_manifest( description, keywords, paths: PluginManifestPaths { - skills: resolve_manifest_path_value(plugin_root, "skills", skills.as_ref()), + skills: resolve_manifest_paths(plugin_root, "skills", skills.as_ref()), mcp_servers: resolve_manifest_mcp_servers(plugin_root, mcp_servers), apps: resolve_manifest_path(plugin_root, "apps", apps.as_deref()), hooks: resolve_manifest_hooks(plugin_root, hooks), @@ -395,20 +396,29 @@ fn json_value_type(value: &JsonValue) -> &'static str { } } -fn resolve_manifest_path_value( +fn resolve_manifest_paths( plugin_root: &Path, field: &'static str, - path: Option<&RawPluginManifestPath>, -) -> Option { - match path? { - RawPluginManifestPath::Path(path) => resolve_manifest_path(plugin_root, field, Some(path)), - RawPluginManifestPath::Invalid(value) => { + paths: Option<&RawPluginManifestPaths>, +) -> Vec { + match paths { + Some(RawPluginManifestPaths::Path(path)) => { + resolve_manifest_path(plugin_root, field, Some(path)) + .map(|path| vec![path]) + .unwrap_or_default() + } + Some(RawPluginManifestPaths::Paths(paths)) => paths + .iter() + .filter_map(|path| resolve_manifest_path(plugin_root, field, Some(path))) + .collect(), + Some(RawPluginManifestPaths::Invalid(value)) => { tracing::warn!( - "ignoring {field}: expected a string; found {}", + "ignoring {field}: expected a string or string array; found {}", json_value_type(value) ); - None + Vec::new() } + None => Vec::new(), } } diff --git a/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs b/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs index dc6a0403d..b154b060b 100644 --- a/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs +++ b/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs @@ -316,7 +316,7 @@ fn resolved_plugin( description: None, keywords: Vec::new(), paths: PluginManifestPaths { - skills: None, + skills: Vec::new(), mcp_servers, apps: None, hooks: None, diff --git a/codex-rs/plugin/src/manifest.rs b/codex-rs/plugin/src/manifest.rs index 4aeec49be..599f4ee40 100644 --- a/codex-rs/plugin/src/manifest.rs +++ b/codex-rs/plugin/src/manifest.rs @@ -17,7 +17,7 @@ pub struct PluginManifest { /// Component resources declared by a plugin manifest. #[derive(Debug, Clone, PartialEq, Eq)] pub struct PluginManifestPaths { - pub skills: Option, + pub skills: Vec, pub mcp_servers: Option>, pub apps: Option, pub hooks: Option>, @@ -172,7 +172,10 @@ impl PluginManifest { description, keywords, paths: PluginManifestPaths { - skills: skills.map(&mut map).transpose()?, + skills: skills + .into_iter() + .map(&mut map) + .collect::, _>>()?, mcp_servers, apps: apps.map(&mut map).transpose()?, hooks, diff --git a/codex-rs/plugin/src/provider_tests.rs b/codex-rs/plugin/src/provider_tests.rs index 776e63b1c..99bc3c208 100644 --- a/codex-rs/plugin/src/provider_tests.rs +++ b/codex-rs/plugin/src/provider_tests.rs @@ -37,7 +37,7 @@ fn environment_descriptor_binds_every_manifest_resource() { description: None, keywords: Vec::new(), paths: PluginManifestPaths { - skills: Some(skills.clone()), + skills: vec![skills.clone()], mcp_servers: Some(PluginManifestMcpServers::Path(mcp_servers.clone())), apps: Some(apps.clone()), hooks: Some(PluginManifestHooks::Paths(vec![hooks.clone()])), @@ -71,7 +71,7 @@ fn environment_descriptor_binds_every_manifest_resource() { description: None, keywords: Vec::new(), paths: PluginManifestPaths { - skills: Some(resource("executor-1", skills)), + skills: vec![resource("executor-1", skills)], mcp_servers: Some(PluginManifestMcpServers::Path(resource( "executor-1", mcp_servers, @@ -103,7 +103,7 @@ fn environment_descriptor_rejects_resources_outside_package_root() { description: None, keywords: Vec::new(), paths: PluginManifestPaths { - skills: None, + skills: Vec::new(), mcp_servers: Some(PluginManifestMcpServers::Path(outside.clone())), apps: None, hooks: None,