From 790c0fbe702bcb28cf9123fdcf56c8db85657835 Mon Sep 17 00:00:00 2001 From: charlesgong-openai Date: Wed, 17 Jun 2026 14:16:24 -0700 Subject: [PATCH 1/7] Support plugin manifest path lists --- codex-rs/core-plugins/src/loader.rs | 14 +- codex-rs/core-plugins/src/manager_tests.rs | 99 +++++++++---- codex-rs/core-plugins/src/manifest.rs | 49 +++++-- codex-rs/core-plugins/src/remote_bundle.rs | 9 +- .../ext/mcp/src/executor_plugin/provider.rs | 136 +++++++++--------- .../mcp/src/executor_plugin/provider_tests.rs | 124 +++++++++++++--- codex-rs/plugin/src/manifest.rs | 25 ++-- codex-rs/plugin/src/provider_tests.rs | 20 +-- .../references/plugin-json-spec.md | 10 +- 9 files changed, 325 insertions(+), 161 deletions(-) diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index fb542f045fda..f76a74560f2c 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -820,9 +820,7 @@ fn plugin_skill_roots( manifest_paths: &PluginManifestPaths, ) -> Vec { let mut paths = default_skill_roots(plugin_root); - if let Some(path) = &manifest_paths.skills { - paths.push(path.clone()); - } + paths.extend(manifest_paths.skills.iter().cloned()); paths.sort_unstable(); paths.dedup(); paths @@ -841,8 +839,8 @@ fn plugin_mcp_config_paths( plugin_root: &Path, manifest_paths: &PluginManifestPaths, ) -> Vec { - if let Some(PluginManifestMcpServers::Path(path)) = &manifest_paths.mcp_servers { - return vec![path.clone()]; + if let Some(PluginManifestMcpServers::Paths(paths)) = &manifest_paths.mcp_servers { + return paths.clone(); } default_mcp_config_paths(plugin_root) } @@ -885,8 +883,8 @@ fn plugin_app_config_paths( plugin_root: &Path, manifest_paths: &PluginManifestPaths, ) -> Vec { - if let Some(path) = &manifest_paths.apps { - return vec![path.clone()]; + if !manifest_paths.apps.is_empty() { + return manifest_paths.apps.clone(); } default_app_config_paths(plugin_root) } @@ -1164,7 +1162,7 @@ async fn load_plugin_mcp_servers_from_manifest( } } } - Some(PluginManifestMcpServers::Path(_)) | None => { + Some(PluginManifestMcpServers::Paths(_)) | None => { for mcp_config_path in plugin_mcp_config_paths(plugin_root, manifest_paths) { let plugin_mcp = load_mcp_servers_from_file(plugin_root, &mcp_config_path).await; for (name, mut config) in plugin_mcp.mcp_servers { diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index c2917211d26c..09a33439563f 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -124,6 +124,31 @@ fn app_declaration(name: &str, connector_id: &str) -> AppDeclaration { } } +fn streamable_http_server(url: &str) -> McpServerConfig { + McpServerConfig { + transport: McpServerTransportConfig::StreamableHttp { + url: url.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(), + } +} + async fn auth_projection_config(codex_home: &Path) -> PluginsConfigInput { let config_toml = r#"[features] plugins = true @@ -1292,9 +1317,9 @@ async fn load_plugins_uses_manifest_configured_component_paths() { &plugin_root.join(".codex-plugin/plugin.json"), r#"{ "name": "sample", - "skills": "./custom-skills/", - "mcpServers": "./config/custom.mcp.json", - "apps": "./config/custom.app.json" + "skills": ["./custom-skills/", "./extra-skills/"], + "mcpServers": ["./config/custom.mcp.json", "./config/extra.mcp.json"], + "apps": ["./config/custom.app.json", "./config/extra.app.json"] }"#, ); write_file( @@ -1305,6 +1330,10 @@ async fn load_plugins_uses_manifest_configured_component_paths() { &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#"{ @@ -1325,6 +1354,17 @@ async fn load_plugins_uses_manifest_configured_component_paths() { "url": "https://custom.example/mcp" } } +}"#, + ); + write_file( + &plugin_root.join("config/extra.mcp.json"), + r#"{ + "mcpServers": { + "extra": { + "type": "http", + "url": "https://extra.example/mcp" + } + } }"#, ); write_file( @@ -1347,6 +1387,16 @@ async fn load_plugins_uses_manifest_configured_component_paths() { } }"#, ); + write_file( + &plugin_root.join("config/extra.app.json"), + r#"{ + "apps": { + "extra-app": { + "id": "connector_extra" + } + } +}"#, + ); let outcome = load_plugins_from_config( &plugin_config_toml(/*enabled*/ true, /*plugins_feature_enabled*/ true), @@ -1359,40 +1409,29 @@ async fn load_plugins_uses_manifest_configured_component_paths() { outcome.plugins()[0].skill_roots, vec![ plugin_root.join("custom-skills").abs(), + plugin_root.join("extra-skills").abs(), plugin_root.join("skills").abs() ] ); 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(), - }, - )]) + HashMap::from([ + ( + "custom".to_string(), + streamable_http_server("https://custom.example/mcp"), + ), + ( + "extra".to_string(), + streamable_http_server("https://extra.example/mcp"), + ), + ]) ); assert_eq!( outcome.plugins()[0].apps, - vec![app_declaration("custom-app", "connector_custom")] + vec![ + app_declaration("custom-app", "connector_custom"), + app_declaration("extra-app", "connector_extra") + ] ); } @@ -1521,7 +1560,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 fe0bb25644aa..e83af48b4380 100644 --- a/codex-rs/core-plugins/src/manifest.rs +++ b/codex-rs/core-plugins/src/manifest.rs @@ -30,11 +30,11 @@ 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)] - apps: Option, + apps: Option, #[serde(default)] hooks: Option, #[serde(default)] @@ -94,8 +94,9 @@ enum RawPluginManifestDefaultPromptEntry { #[derive(Debug, Deserialize)] #[serde(untagged)] -enum RawPluginManifestPath { +enum RawPluginManifestPaths { Path(String), + Paths(Vec), Invalid(JsonValue), } @@ -103,6 +104,7 @@ enum RawPluginManifestPath { #[serde(untagged)] enum RawPluginManifestMcpServers { Path(String), + Paths(Vec), Object(std::collections::BTreeMap), Invalid(JsonValue), } @@ -230,9 +232,9 @@ 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()), + apps: resolve_manifest_paths(plugin_root, "apps", apps.as_ref()), hooks: resolve_manifest_hooks(plugin_root, hooks), }, interface, @@ -276,7 +278,15 @@ fn resolve_manifest_mcp_servers( match mcp_servers? { RawPluginManifestMcpServers::Path(path) => { resolve_manifest_path(plugin_root, "mcpServers", Some(&path)) - .map(PluginManifestMcpServers::Path) + .map(|path| PluginManifestMcpServers::Paths(vec![path])) + } + RawPluginManifestMcpServers::Paths(paths) => { + let mcp_server_paths = paths + .iter() + .filter_map(|path| resolve_manifest_path(plugin_root, "mcpServers", Some(path))) + .collect::>(); + (!mcp_server_paths.is_empty()) + .then_some(PluginManifestMcpServers::Paths(mcp_server_paths)) } RawPluginManifestMcpServers::Object(servers) => match serde_json::to_string(&servers) { Ok(servers) => Some(PluginManifestMcpServers::Object(servers)), @@ -287,7 +297,7 @@ fn resolve_manifest_mcp_servers( }, RawPluginManifestMcpServers::Invalid(value) => { tracing::warn!( - "ignoring mcpServers: expected a string or object; found {}", + "ignoring mcpServers: expected a string, string array, or object; found {}", json_value_type(&value) ); None @@ -395,20 +405,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/core-plugins/src/remote_bundle.rs b/codex-rs/core-plugins/src/remote_bundle.rs index cb60d60b77ee..68ccff1db7f4 100644 --- a/codex-rs/core-plugins/src/remote_bundle.rs +++ b/codex-rs/core-plugins/src/remote_bundle.rs @@ -494,7 +494,14 @@ fn overwrite_plugin_app_manifest( app_manifest: &JsonValue, ) -> Result<(), RemotePluginBundleInstallError> { let app_manifest_path = crate::manifest::load_plugin_manifest(plugin_root) - .and_then(|manifest| manifest.paths.apps.map(|path| path.to_path_buf())) + .and_then(|manifest| { + manifest + .paths + .apps + .into_iter() + .next() + .map(|path| path.to_path_buf()) + }) .unwrap_or_else(|| plugin_root.join(".app.json")); write_json_file( &app_manifest_path, diff --git a/codex-rs/ext/mcp/src/executor_plugin/provider.rs b/codex-rs/ext/mcp/src/executor_plugin/provider.rs index e9d5f803db58..4965425d466e 100644 --- a/codex-rs/ext/mcp/src/executor_plugin/provider.rs +++ b/codex-rs/ext/mcp/src/executor_plugin/provider.rs @@ -57,84 +57,90 @@ async fn load_from_file_system( ) -> Result, ExecutorPluginMcpProviderError> { let ResolvedPluginLocation::Environment { environment_id, .. } = plugin.location(); let plugin_id = plugin.selected_root_id(); - let (contents, config_path) = match plugin.manifest().paths.mcp_servers.as_ref() { - Some(PluginManifestMcpServers::Path(PluginResourceLocator::Environment { - path, .. - })) => { - let config_uri = PathUri::from_abs_path(path); - ( - file_system - .read_file_text(&config_uri, /*sandbox*/ None) - .await - .map_err(|source| ExecutorPluginMcpProviderError::ReadConfig { - plugin_id: plugin_id.to_string(), - path: path.clone(), - source, - })?, - path.clone(), - ) + let config_sources = match plugin.manifest().paths.mcp_servers.as_ref() { + Some(PluginManifestMcpServers::Paths(paths)) => { + let mut config_sources = Vec::new(); + for PluginResourceLocator::Environment { path, .. } in paths { + let contents = read_config_file(plugin_id, path, file_system).await?; + config_sources.push((contents, path.clone())); + } + config_sources + } + Some(PluginManifestMcpServers::Object(object_config)) => { + vec![( + object_config.clone(), + plugin_root.join(".codex-plugin/plugin.json"), + )] } - Some(PluginManifestMcpServers::Object(object_config)) => ( - object_config.clone(), - plugin_root.join(".codex-plugin/plugin.json"), - ), None => { let config_path = plugin_root.join(DEFAULT_MCP_CONFIG_FILE); - let config_uri = PathUri::from_abs_path(&config_path); - let contents = match file_system - .read_file_text(&config_uri, /*sandbox*/ None) - .await - { + let contents = match read_config_file(plugin_id, &config_path, file_system).await { Ok(contents) => contents, - Err(source) if source.kind() == io::ErrorKind::NotFound => { + Err(ExecutorPluginMcpProviderError::ReadConfig { source, .. }) + if source.kind() == io::ErrorKind::NotFound => + { return Ok(Vec::new()); } - Err(source) => { - return Err(ExecutorPluginMcpProviderError::ReadConfig { - plugin_id: plugin_id.to_string(), - path: config_path.clone(), - source, - }); - } + Err(err) => return Err(err), }; - (contents, config_path) + vec![(contents, config_path)] } }; - let parsed = parse_plugin_mcp_config( - plugin_root.as_path(), - &contents, - PluginMcpServerPlacement::Environment { environment_id }, - ) - .map_err(|source| ExecutorPluginMcpProviderError::ParseConfig { - plugin_id: plugin_id.to_string(), - path: config_path, - source, - })?; - for error in parsed.errors { - tracing::warn!( - plugin = plugin_id, - server = error.name, - error = error.message, - "ignoring invalid executor plugin MCP server" - ); - } + let mut servers = Vec::new(); + for (contents, config_path) in config_sources { + let parsed = parse_plugin_mcp_config( + plugin_root.as_path(), + &contents, + PluginMcpServerPlacement::Environment { environment_id }, + ) + .map_err(|source| ExecutorPluginMcpProviderError::ParseConfig { + plugin_id: plugin_id.to_string(), + path: config_path, + source, + })?; + + for error in parsed.errors { + tracing::warn!( + plugin = plugin_id, + server = error.name, + error = error.message, + "ignoring invalid executor plugin MCP server" + ); + } - Ok(parsed - .servers - .into_iter() - .filter_map(|(name, config)| match &config.transport { - McpServerTransportConfig::Stdio { .. } => Some((name, config)), - McpServerTransportConfig::StreamableHttp { .. } => { - tracing::warn!( - plugin = plugin_id, - server = name, - "ignoring HTTP MCP server from executor plugin" - ); - None + servers.extend(parsed.servers.into_iter().filter_map(|(name, config)| { + match &config.transport { + McpServerTransportConfig::Stdio { .. } => Some((name, config)), + McpServerTransportConfig::StreamableHttp { .. } => { + tracing::warn!( + plugin = plugin_id, + server = name, + "ignoring HTTP MCP server from executor plugin" + ); + None + } } + })); + } + + Ok(servers) +} + +async fn read_config_file( + plugin_id: &str, + config_path: &AbsolutePathBuf, + file_system: &dyn ExecutorFileSystem, +) -> Result { + let config_uri = PathUri::from_abs_path(config_path); + file_system + .read_file_text(&config_uri, /*sandbox*/ None) + .await + .map_err(|source| ExecutorPluginMcpProviderError::ReadConfig { + plugin_id: plugin_id.to_string(), + path: config_path.clone(), + source, }) - .collect()) } #[cfg(test)] 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 dc6a0403d03d..391feec333c8 100644 --- a/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs +++ b/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs @@ -32,8 +32,7 @@ const MCP_CONFIG_CONTENTS: &str = r#"{ }"#; struct SyntheticExecutorFileSystem { - config_path: AbsolutePathBuf, - config_contents: Option<&'static str>, + config_contents_by_path: HashMap, reads: Mutex>, } @@ -66,10 +65,8 @@ impl ExecutorFileSystem for SyntheticExecutorFileSystem { .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) .push(path.clone()); - if path != self.config_path { - return Err(io::Error::new(io::ErrorKind::NotFound, "not found")); - } - self.config_contents + self.config_contents_by_path + .get(&path) .map(|contents| contents.as_bytes().to_vec()) .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "not found")) }) @@ -147,11 +144,10 @@ async fn reads_declared_config_only_through_executor_file_system() { let config_path = plugin_root.join("config/mcp.json"); let plugin = resolved_plugin( &plugin_root, - Some(PluginManifestMcpServers::Path(config_path.clone())), + Some(PluginManifestMcpServers::Paths(vec![config_path.clone()])), ); let file_system = SyntheticExecutorFileSystem { - config_path: config_path.clone(), - config_contents: Some(MCP_CONFIG_CONTENTS), + config_contents_by_path: HashMap::from([(config_path.clone(), MCP_CONFIG_CONTENTS)]), reads: Mutex::new(Vec::new()), }; @@ -191,12 +187,105 @@ async fn reads_declared_config_only_through_executor_file_system() { assert_eq!(reads(&file_system), vec![config_path]); } +#[tokio::test] +async fn reads_multiple_declared_configs_through_executor_file_system() { + let temp_dir = tempfile::tempdir().expect("tempdir"); + let plugin_root = + AbsolutePathBuf::from_absolute_path_checked(temp_dir.path().join("executor-only-plugin")) + .expect("absolute plugin root"); + assert!(!plugin_root.as_path().exists()); + let first_config_path = plugin_root.join("config/first.mcp.json"); + let second_config_path = plugin_root.join("config/second.mcp.json"); + let plugin = resolved_plugin( + &plugin_root, + Some(PluginManifestMcpServers::Paths(vec![ + first_config_path.clone(), + second_config_path.clone(), + ])), + ); + let file_system = SyntheticExecutorFileSystem { + config_contents_by_path: HashMap::from([ + (first_config_path.clone(), MCP_CONFIG_CONTENTS), + ( + second_config_path.clone(), + r#"{"mcpServers":{"other":{"command":"other-mcp","args":["--fast"]}}}"#, + ), + ]), + reads: Mutex::new(Vec::new()), + }; + + let servers = load_from_file_system(&plugin, &plugin_root, &file_system) + .await + .expect("load executor MCP configs"); + + assert_eq!( + servers, + vec![ + ( + "demo".to_string(), + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: "demo-mcp".to_string(), + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: Some(plugin_root.to_path_buf()), + }, + environment_id: "executor-test".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(), + }, + ), + ( + "other".to_string(), + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: "other-mcp".to_string(), + args: vec!["--fast".to_string()], + env: None, + env_vars: Vec::new(), + cwd: Some(plugin_root.to_path_buf()), + }, + environment_id: "executor-test".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!( + reads(&file_system), + vec![first_config_path, second_config_path] + ); +} + #[tokio::test] async fn reads_manifest_object_config_without_executor_file_system_access() { let temp_dir = tempfile::tempdir().expect("tempdir"); let plugin_root = AbsolutePathBuf::from_absolute_path_checked(temp_dir.path().join("plugin")) .expect("absolute plugin root"); - let config_path = plugin_root.join(DEFAULT_MCP_CONFIG_FILE); let plugin = resolved_plugin( &plugin_root, Some(PluginManifestMcpServers::Object( @@ -204,8 +293,7 @@ async fn reads_manifest_object_config_without_executor_file_system_access() { )), ); let file_system = SyntheticExecutorFileSystem { - config_path, - config_contents: None, + config_contents_by_path: HashMap::new(), reads: Mutex::new(Vec::new()), }; @@ -253,8 +341,7 @@ async fn missing_default_config_is_empty() { let config_path = plugin_root.join(DEFAULT_MCP_CONFIG_FILE); let plugin = resolved_plugin(&plugin_root, /*mcp_servers*/ None); let file_system = SyntheticExecutorFileSystem { - config_path: config_path.clone(), - config_contents: None, + config_contents_by_path: HashMap::new(), reads: Mutex::new(Vec::new()), }; @@ -274,11 +361,10 @@ async fn malformed_declared_config_is_an_error() { let config_path = plugin_root.join("mcp.json"); let plugin = resolved_plugin( &plugin_root, - Some(PluginManifestMcpServers::Path(config_path.clone())), + Some(PluginManifestMcpServers::Paths(vec![config_path.clone()])), ); let file_system = SyntheticExecutorFileSystem { - config_path: config_path.clone(), - config_contents: Some("{not-json"), + config_contents_by_path: HashMap::from([(config_path.clone(), "{not-json")]), reads: Mutex::new(Vec::new()), }; @@ -316,9 +402,9 @@ fn resolved_plugin( description: None, keywords: Vec::new(), paths: PluginManifestPaths { - skills: None, + skills: Vec::new(), mcp_servers, - apps: None, + apps: Vec::new(), hooks: None, }, interface: None, diff --git a/codex-rs/plugin/src/manifest.rs b/codex-rs/plugin/src/manifest.rs index 4aeec49be8dc..98c93e996b03 100644 --- a/codex-rs/plugin/src/manifest.rs +++ b/codex-rs/plugin/src/manifest.rs @@ -17,16 +17,16 @@ 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 apps: Vec, pub hooks: Option>, } /// MCP server declarations embedded in or referenced by a plugin manifest. #[derive(Debug, Clone, PartialEq, Eq)] pub enum PluginManifestMcpServers { - Path(Resource), + Paths(Vec), Object(String), } @@ -117,9 +117,12 @@ impl PluginManifest { None => None, }; let mcp_servers = match mcp_servers { - Some(PluginManifestMcpServers::Path(path)) => { - Some(PluginManifestMcpServers::Path(map(path)?)) - } + Some(PluginManifestMcpServers::Paths(paths)) => Some(PluginManifestMcpServers::Paths( + paths + .into_iter() + .map(&mut map) + .collect::, _>>()?, + )), Some(PluginManifestMcpServers::Object(servers)) => { Some(PluginManifestMcpServers::Object(servers)) } @@ -172,9 +175,15 @@ 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()?, + apps: apps + .into_iter() + .map(&mut map) + .collect::, _>>()?, hooks, }, interface, diff --git a/codex-rs/plugin/src/provider_tests.rs b/codex-rs/plugin/src/provider_tests.rs index 776e63b1c3d2..80618ff94f63 100644 --- a/codex-rs/plugin/src/provider_tests.rs +++ b/codex-rs/plugin/src/provider_tests.rs @@ -37,9 +37,9 @@ fn environment_descriptor_binds_every_manifest_resource() { description: None, keywords: Vec::new(), paths: PluginManifestPaths { - skills: Some(skills.clone()), - mcp_servers: Some(PluginManifestMcpServers::Path(mcp_servers.clone())), - apps: Some(apps.clone()), + skills: vec![skills.clone()], + mcp_servers: Some(PluginManifestMcpServers::Paths(vec![mcp_servers.clone()])), + apps: vec![apps.clone()], hooks: Some(PluginManifestHooks::Paths(vec![hooks.clone()])), }, interface: Some(PluginManifestInterface { @@ -71,12 +71,12 @@ fn environment_descriptor_binds_every_manifest_resource() { description: None, keywords: Vec::new(), paths: PluginManifestPaths { - skills: Some(resource("executor-1", skills)), - mcp_servers: Some(PluginManifestMcpServers::Path(resource( + skills: vec![resource("executor-1", skills)], + mcp_servers: Some(PluginManifestMcpServers::Paths(vec![resource( "executor-1", mcp_servers, - ))), - apps: Some(resource("executor-1", apps)), + )])), + apps: vec![resource("executor-1", apps)], hooks: Some(PluginManifestHooks::Paths(vec![resource( "executor-1", hooks, @@ -103,9 +103,9 @@ fn environment_descriptor_rejects_resources_outside_package_root() { description: None, keywords: Vec::new(), paths: PluginManifestPaths { - skills: None, - mcp_servers: Some(PluginManifestMcpServers::Path(outside.clone())), - apps: None, + skills: Vec::new(), + mcp_servers: Some(PluginManifestMcpServers::Paths(vec![outside.clone()])), + apps: Vec::new(), hooks: None, }, interface: None, diff --git a/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md b/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md index ec5476ae14e2..0e834759a221 100644 --- a/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md +++ b/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md @@ -60,10 +60,10 @@ - `repository` (`string`): Source code URL. - `license` (`string`): License identifier (for example `MIT`, `Apache-2.0`). - `keywords` (`array` of `string`): Search/discovery tags. -- `skills` (`string`): Relative path to skill directories/files. -- `hooks` (`string`): Hook config path. -- `mcpServers` (`string` or `object`): MCP config path, or an object whose keys are MCP server names and whose values are MCP server config objects. -- `apps` (`string`): App manifest path for plugin integrations. +- `skills` (`string` or array of `string`): Relative path(s) to skill directories/files. +- `hooks` (`string` or array of `string`): Hook config path(s). +- `mcpServers` (`string`, array of `string`, or `object`): MCP config path(s), or an object whose keys are MCP server names and whose values are MCP server config objects. +- `apps` (`string` or array of `string`): App manifest path(s) for plugin integrations. - `interface` (`object`): Interface/UX metadata block for plugin presentation. `mcpServers` may be declared as a companion file path: @@ -112,7 +112,7 @@ Or as an object directly in `plugin.json`: ### Path conventions and defaults - Path values should be relative and begin with `./`. -- `skills`, `hooks`, and string-valued `mcpServers` are supplemented on top of default component discovery; they do not replace defaults. +- `skills`, `hooks`, `apps`, and file-backed `mcpServers` can be declared as either a single path string or an array of path strings. - Custom path values must follow the plugin root convention and naming/namespacing rules. - This repo’s scaffold writes `.codex-plugin/plugin.json`; treat that as the manifest location this skill generates. From 258b4eb2993486744a26fec7e900fc1abb069bae Mon Sep 17 00:00:00 2001 From: charlesgong-openai Date: Wed, 17 Jun 2026 14:47:23 -0700 Subject: [PATCH 2/7] Limit plugin manifest path lists to skills --- codex-rs/core-plugins/src/loader.rs | 10 +- codex-rs/core-plugins/src/manager_tests.rs | 91 ++++-------- codex-rs/core-plugins/src/manifest.rs | 17 +-- codex-rs/core-plugins/src/remote_bundle.rs | 9 +- .../ext/mcp/src/executor_plugin/provider.rs | 136 +++++++++--------- .../mcp/src/executor_plugin/provider_tests.rs | 122 +++------------- codex-rs/plugin/src/manifest.rs | 18 +-- codex-rs/plugin/src/provider_tests.rs | 14 +- .../references/plugin-json-spec.md | 9 +- 9 files changed, 139 insertions(+), 287 deletions(-) diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index f76a74560f2c..02f3b4d330f4 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -839,8 +839,8 @@ fn plugin_mcp_config_paths( plugin_root: &Path, manifest_paths: &PluginManifestPaths, ) -> Vec { - if let Some(PluginManifestMcpServers::Paths(paths)) = &manifest_paths.mcp_servers { - return paths.clone(); + if let Some(PluginManifestMcpServers::Path(path)) = &manifest_paths.mcp_servers { + return vec![path.clone()]; } default_mcp_config_paths(plugin_root) } @@ -883,8 +883,8 @@ fn plugin_app_config_paths( plugin_root: &Path, manifest_paths: &PluginManifestPaths, ) -> Vec { - if !manifest_paths.apps.is_empty() { - return manifest_paths.apps.clone(); + if let Some(path) = &manifest_paths.apps { + return vec![path.clone()]; } default_app_config_paths(plugin_root) } @@ -1162,7 +1162,7 @@ async fn load_plugin_mcp_servers_from_manifest( } } } - Some(PluginManifestMcpServers::Paths(_)) | None => { + Some(PluginManifestMcpServers::Path(_)) | None => { for mcp_config_path in plugin_mcp_config_paths(plugin_root, manifest_paths) { let plugin_mcp = load_mcp_servers_from_file(plugin_root, &mcp_config_path).await; for (name, mut config) in plugin_mcp.mcp_servers { diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 09a33439563f..929de6f96571 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -124,31 +124,6 @@ fn app_declaration(name: &str, connector_id: &str) -> AppDeclaration { } } -fn streamable_http_server(url: &str) -> McpServerConfig { - McpServerConfig { - transport: McpServerTransportConfig::StreamableHttp { - url: url.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(), - } -} - async fn auth_projection_config(codex_home: &Path) -> PluginsConfigInput { let config_toml = r#"[features] plugins = true @@ -1318,8 +1293,8 @@ async fn load_plugins_uses_manifest_configured_component_paths() { r#"{ "name": "sample", "skills": ["./custom-skills/", "./extra-skills/"], - "mcpServers": ["./config/custom.mcp.json", "./config/extra.mcp.json"], - "apps": ["./config/custom.app.json", "./config/extra.app.json"] + "mcpServers": "./config/custom.mcp.json", + "apps": "./config/custom.app.json" }"#, ); write_file( @@ -1354,17 +1329,6 @@ async fn load_plugins_uses_manifest_configured_component_paths() { "url": "https://custom.example/mcp" } } -}"#, - ); - write_file( - &plugin_root.join("config/extra.mcp.json"), - r#"{ - "mcpServers": { - "extra": { - "type": "http", - "url": "https://extra.example/mcp" - } - } }"#, ); write_file( @@ -1387,17 +1351,6 @@ async fn load_plugins_uses_manifest_configured_component_paths() { } }"#, ); - write_file( - &plugin_root.join("config/extra.app.json"), - r#"{ - "apps": { - "extra-app": { - "id": "connector_extra" - } - } -}"#, - ); - let outcome = load_plugins_from_config( &plugin_config_toml(/*enabled*/ true, /*plugins_feature_enabled*/ true), codex_home.path(), @@ -1415,23 +1368,35 @@ async fn load_plugins_uses_manifest_configured_component_paths() { ); assert_eq!( outcome.plugins()[0].mcp_servers, - HashMap::from([ - ( - "custom".to_string(), - streamable_http_server("https://custom.example/mcp"), - ), - ( - "extra".to_string(), - streamable_http_server("https://extra.example/mcp"), - ), - ]) + 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"), - app_declaration("extra-app", "connector_extra") - ] + vec![app_declaration("custom-app", "connector_custom")] ); } diff --git a/codex-rs/core-plugins/src/manifest.rs b/codex-rs/core-plugins/src/manifest.rs index e83af48b4380..37630a8436c5 100644 --- a/codex-rs/core-plugins/src/manifest.rs +++ b/codex-rs/core-plugins/src/manifest.rs @@ -34,7 +34,7 @@ struct RawPluginManifest { #[serde(default)] mcp_servers: Option, #[serde(default)] - apps: Option, + apps: Option, #[serde(default)] hooks: Option, #[serde(default)] @@ -104,7 +104,6 @@ enum RawPluginManifestPaths { #[serde(untagged)] enum RawPluginManifestMcpServers { Path(String), - Paths(Vec), Object(std::collections::BTreeMap), Invalid(JsonValue), } @@ -234,7 +233,7 @@ pub(crate) fn parse_plugin_manifest( paths: PluginManifestPaths { skills: resolve_manifest_paths(plugin_root, "skills", skills.as_ref()), mcp_servers: resolve_manifest_mcp_servers(plugin_root, mcp_servers), - apps: resolve_manifest_paths(plugin_root, "apps", apps.as_ref()), + apps: resolve_manifest_path(plugin_root, "apps", apps.as_deref()), hooks: resolve_manifest_hooks(plugin_root, hooks), }, interface, @@ -278,15 +277,7 @@ fn resolve_manifest_mcp_servers( match mcp_servers? { RawPluginManifestMcpServers::Path(path) => { resolve_manifest_path(plugin_root, "mcpServers", Some(&path)) - .map(|path| PluginManifestMcpServers::Paths(vec![path])) - } - RawPluginManifestMcpServers::Paths(paths) => { - let mcp_server_paths = paths - .iter() - .filter_map(|path| resolve_manifest_path(plugin_root, "mcpServers", Some(path))) - .collect::>(); - (!mcp_server_paths.is_empty()) - .then_some(PluginManifestMcpServers::Paths(mcp_server_paths)) + .map(PluginManifestMcpServers::Path) } RawPluginManifestMcpServers::Object(servers) => match serde_json::to_string(&servers) { Ok(servers) => Some(PluginManifestMcpServers::Object(servers)), @@ -297,7 +288,7 @@ fn resolve_manifest_mcp_servers( }, RawPluginManifestMcpServers::Invalid(value) => { tracing::warn!( - "ignoring mcpServers: expected a string, string array, or object; found {}", + "ignoring mcpServers: expected a string or object; found {}", json_value_type(&value) ); None diff --git a/codex-rs/core-plugins/src/remote_bundle.rs b/codex-rs/core-plugins/src/remote_bundle.rs index 68ccff1db7f4..cb60d60b77ee 100644 --- a/codex-rs/core-plugins/src/remote_bundle.rs +++ b/codex-rs/core-plugins/src/remote_bundle.rs @@ -494,14 +494,7 @@ fn overwrite_plugin_app_manifest( app_manifest: &JsonValue, ) -> Result<(), RemotePluginBundleInstallError> { let app_manifest_path = crate::manifest::load_plugin_manifest(plugin_root) - .and_then(|manifest| { - manifest - .paths - .apps - .into_iter() - .next() - .map(|path| path.to_path_buf()) - }) + .and_then(|manifest| manifest.paths.apps.map(|path| path.to_path_buf())) .unwrap_or_else(|| plugin_root.join(".app.json")); write_json_file( &app_manifest_path, diff --git a/codex-rs/ext/mcp/src/executor_plugin/provider.rs b/codex-rs/ext/mcp/src/executor_plugin/provider.rs index 4965425d466e..e9d5f803db58 100644 --- a/codex-rs/ext/mcp/src/executor_plugin/provider.rs +++ b/codex-rs/ext/mcp/src/executor_plugin/provider.rs @@ -57,90 +57,84 @@ async fn load_from_file_system( ) -> Result, ExecutorPluginMcpProviderError> { let ResolvedPluginLocation::Environment { environment_id, .. } = plugin.location(); let plugin_id = plugin.selected_root_id(); - let config_sources = match plugin.manifest().paths.mcp_servers.as_ref() { - Some(PluginManifestMcpServers::Paths(paths)) => { - let mut config_sources = Vec::new(); - for PluginResourceLocator::Environment { path, .. } in paths { - let contents = read_config_file(plugin_id, path, file_system).await?; - config_sources.push((contents, path.clone())); - } - config_sources - } - Some(PluginManifestMcpServers::Object(object_config)) => { - vec![( - object_config.clone(), - plugin_root.join(".codex-plugin/plugin.json"), - )] + let (contents, config_path) = match plugin.manifest().paths.mcp_servers.as_ref() { + Some(PluginManifestMcpServers::Path(PluginResourceLocator::Environment { + path, .. + })) => { + let config_uri = PathUri::from_abs_path(path); + ( + file_system + .read_file_text(&config_uri, /*sandbox*/ None) + .await + .map_err(|source| ExecutorPluginMcpProviderError::ReadConfig { + plugin_id: plugin_id.to_string(), + path: path.clone(), + source, + })?, + path.clone(), + ) } + Some(PluginManifestMcpServers::Object(object_config)) => ( + object_config.clone(), + plugin_root.join(".codex-plugin/plugin.json"), + ), None => { let config_path = plugin_root.join(DEFAULT_MCP_CONFIG_FILE); - let contents = match read_config_file(plugin_id, &config_path, file_system).await { + let config_uri = PathUri::from_abs_path(&config_path); + let contents = match file_system + .read_file_text(&config_uri, /*sandbox*/ None) + .await + { Ok(contents) => contents, - Err(ExecutorPluginMcpProviderError::ReadConfig { source, .. }) - if source.kind() == io::ErrorKind::NotFound => - { + Err(source) if source.kind() == io::ErrorKind::NotFound => { return Ok(Vec::new()); } - Err(err) => return Err(err), + Err(source) => { + return Err(ExecutorPluginMcpProviderError::ReadConfig { + plugin_id: plugin_id.to_string(), + path: config_path.clone(), + source, + }); + } }; - vec![(contents, config_path)] + (contents, config_path) } }; + let parsed = parse_plugin_mcp_config( + plugin_root.as_path(), + &contents, + PluginMcpServerPlacement::Environment { environment_id }, + ) + .map_err(|source| ExecutorPluginMcpProviderError::ParseConfig { + plugin_id: plugin_id.to_string(), + path: config_path, + source, + })?; - let mut servers = Vec::new(); - for (contents, config_path) in config_sources { - let parsed = parse_plugin_mcp_config( - plugin_root.as_path(), - &contents, - PluginMcpServerPlacement::Environment { environment_id }, - ) - .map_err(|source| ExecutorPluginMcpProviderError::ParseConfig { - plugin_id: plugin_id.to_string(), - path: config_path, - source, - })?; - - for error in parsed.errors { - tracing::warn!( - plugin = plugin_id, - server = error.name, - error = error.message, - "ignoring invalid executor plugin MCP server" - ); - } - - servers.extend(parsed.servers.into_iter().filter_map(|(name, config)| { - match &config.transport { - McpServerTransportConfig::Stdio { .. } => Some((name, config)), - McpServerTransportConfig::StreamableHttp { .. } => { - tracing::warn!( - plugin = plugin_id, - server = name, - "ignoring HTTP MCP server from executor plugin" - ); - None - } - } - })); + for error in parsed.errors { + tracing::warn!( + plugin = plugin_id, + server = error.name, + error = error.message, + "ignoring invalid executor plugin MCP server" + ); } - Ok(servers) -} - -async fn read_config_file( - plugin_id: &str, - config_path: &AbsolutePathBuf, - file_system: &dyn ExecutorFileSystem, -) -> Result { - let config_uri = PathUri::from_abs_path(config_path); - file_system - .read_file_text(&config_uri, /*sandbox*/ None) - .await - .map_err(|source| ExecutorPluginMcpProviderError::ReadConfig { - plugin_id: plugin_id.to_string(), - path: config_path.clone(), - source, + Ok(parsed + .servers + .into_iter() + .filter_map(|(name, config)| match &config.transport { + McpServerTransportConfig::Stdio { .. } => Some((name, config)), + McpServerTransportConfig::StreamableHttp { .. } => { + tracing::warn!( + plugin = plugin_id, + server = name, + "ignoring HTTP MCP server from executor plugin" + ); + None + } }) + .collect()) } #[cfg(test)] 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 391feec333c8..b154b060b2b1 100644 --- a/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs +++ b/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs @@ -32,7 +32,8 @@ const MCP_CONFIG_CONTENTS: &str = r#"{ }"#; struct SyntheticExecutorFileSystem { - config_contents_by_path: HashMap, + config_path: AbsolutePathBuf, + config_contents: Option<&'static str>, reads: Mutex>, } @@ -65,8 +66,10 @@ impl ExecutorFileSystem for SyntheticExecutorFileSystem { .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) .push(path.clone()); - self.config_contents_by_path - .get(&path) + if path != self.config_path { + return Err(io::Error::new(io::ErrorKind::NotFound, "not found")); + } + self.config_contents .map(|contents| contents.as_bytes().to_vec()) .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "not found")) }) @@ -144,10 +147,11 @@ async fn reads_declared_config_only_through_executor_file_system() { let config_path = plugin_root.join("config/mcp.json"); let plugin = resolved_plugin( &plugin_root, - Some(PluginManifestMcpServers::Paths(vec![config_path.clone()])), + Some(PluginManifestMcpServers::Path(config_path.clone())), ); let file_system = SyntheticExecutorFileSystem { - config_contents_by_path: HashMap::from([(config_path.clone(), MCP_CONFIG_CONTENTS)]), + config_path: config_path.clone(), + config_contents: Some(MCP_CONFIG_CONTENTS), reads: Mutex::new(Vec::new()), }; @@ -187,105 +191,12 @@ async fn reads_declared_config_only_through_executor_file_system() { assert_eq!(reads(&file_system), vec![config_path]); } -#[tokio::test] -async fn reads_multiple_declared_configs_through_executor_file_system() { - let temp_dir = tempfile::tempdir().expect("tempdir"); - let plugin_root = - AbsolutePathBuf::from_absolute_path_checked(temp_dir.path().join("executor-only-plugin")) - .expect("absolute plugin root"); - assert!(!plugin_root.as_path().exists()); - let first_config_path = plugin_root.join("config/first.mcp.json"); - let second_config_path = plugin_root.join("config/second.mcp.json"); - let plugin = resolved_plugin( - &plugin_root, - Some(PluginManifestMcpServers::Paths(vec![ - first_config_path.clone(), - second_config_path.clone(), - ])), - ); - let file_system = SyntheticExecutorFileSystem { - config_contents_by_path: HashMap::from([ - (first_config_path.clone(), MCP_CONFIG_CONTENTS), - ( - second_config_path.clone(), - r#"{"mcpServers":{"other":{"command":"other-mcp","args":["--fast"]}}}"#, - ), - ]), - reads: Mutex::new(Vec::new()), - }; - - let servers = load_from_file_system(&plugin, &plugin_root, &file_system) - .await - .expect("load executor MCP configs"); - - assert_eq!( - servers, - vec![ - ( - "demo".to_string(), - McpServerConfig { - transport: McpServerTransportConfig::Stdio { - command: "demo-mcp".to_string(), - args: Vec::new(), - env: None, - env_vars: Vec::new(), - cwd: Some(plugin_root.to_path_buf()), - }, - environment_id: "executor-test".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(), - }, - ), - ( - "other".to_string(), - McpServerConfig { - transport: McpServerTransportConfig::Stdio { - command: "other-mcp".to_string(), - args: vec!["--fast".to_string()], - env: None, - env_vars: Vec::new(), - cwd: Some(plugin_root.to_path_buf()), - }, - environment_id: "executor-test".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!( - reads(&file_system), - vec![first_config_path, second_config_path] - ); -} - #[tokio::test] async fn reads_manifest_object_config_without_executor_file_system_access() { let temp_dir = tempfile::tempdir().expect("tempdir"); let plugin_root = AbsolutePathBuf::from_absolute_path_checked(temp_dir.path().join("plugin")) .expect("absolute plugin root"); + let config_path = plugin_root.join(DEFAULT_MCP_CONFIG_FILE); let plugin = resolved_plugin( &plugin_root, Some(PluginManifestMcpServers::Object( @@ -293,7 +204,8 @@ async fn reads_manifest_object_config_without_executor_file_system_access() { )), ); let file_system = SyntheticExecutorFileSystem { - config_contents_by_path: HashMap::new(), + config_path, + config_contents: None, reads: Mutex::new(Vec::new()), }; @@ -341,7 +253,8 @@ async fn missing_default_config_is_empty() { let config_path = plugin_root.join(DEFAULT_MCP_CONFIG_FILE); let plugin = resolved_plugin(&plugin_root, /*mcp_servers*/ None); let file_system = SyntheticExecutorFileSystem { - config_contents_by_path: HashMap::new(), + config_path: config_path.clone(), + config_contents: None, reads: Mutex::new(Vec::new()), }; @@ -361,10 +274,11 @@ async fn malformed_declared_config_is_an_error() { let config_path = plugin_root.join("mcp.json"); let plugin = resolved_plugin( &plugin_root, - Some(PluginManifestMcpServers::Paths(vec![config_path.clone()])), + Some(PluginManifestMcpServers::Path(config_path.clone())), ); let file_system = SyntheticExecutorFileSystem { - config_contents_by_path: HashMap::from([(config_path.clone(), "{not-json")]), + config_path: config_path.clone(), + config_contents: Some("{not-json"), reads: Mutex::new(Vec::new()), }; @@ -404,7 +318,7 @@ fn resolved_plugin( paths: PluginManifestPaths { skills: Vec::new(), mcp_servers, - apps: Vec::new(), + apps: None, hooks: None, }, interface: None, diff --git a/codex-rs/plugin/src/manifest.rs b/codex-rs/plugin/src/manifest.rs index 98c93e996b03..599f4ee40720 100644 --- a/codex-rs/plugin/src/manifest.rs +++ b/codex-rs/plugin/src/manifest.rs @@ -19,14 +19,14 @@ pub struct PluginManifest { pub struct PluginManifestPaths { pub skills: Vec, pub mcp_servers: Option>, - pub apps: Vec, + pub apps: Option, pub hooks: Option>, } /// MCP server declarations embedded in or referenced by a plugin manifest. #[derive(Debug, Clone, PartialEq, Eq)] pub enum PluginManifestMcpServers { - Paths(Vec), + Path(Resource), Object(String), } @@ -117,12 +117,9 @@ impl PluginManifest { None => None, }; let mcp_servers = match mcp_servers { - Some(PluginManifestMcpServers::Paths(paths)) => Some(PluginManifestMcpServers::Paths( - paths - .into_iter() - .map(&mut map) - .collect::, _>>()?, - )), + Some(PluginManifestMcpServers::Path(path)) => { + Some(PluginManifestMcpServers::Path(map(path)?)) + } Some(PluginManifestMcpServers::Object(servers)) => { Some(PluginManifestMcpServers::Object(servers)) } @@ -180,10 +177,7 @@ impl PluginManifest { .map(&mut map) .collect::, _>>()?, mcp_servers, - apps: apps - .into_iter() - .map(&mut map) - .collect::, _>>()?, + apps: apps.map(&mut map).transpose()?, hooks, }, interface, diff --git a/codex-rs/plugin/src/provider_tests.rs b/codex-rs/plugin/src/provider_tests.rs index 80618ff94f63..99bc3c20891f 100644 --- a/codex-rs/plugin/src/provider_tests.rs +++ b/codex-rs/plugin/src/provider_tests.rs @@ -38,8 +38,8 @@ fn environment_descriptor_binds_every_manifest_resource() { keywords: Vec::new(), paths: PluginManifestPaths { skills: vec![skills.clone()], - mcp_servers: Some(PluginManifestMcpServers::Paths(vec![mcp_servers.clone()])), - apps: vec![apps.clone()], + mcp_servers: Some(PluginManifestMcpServers::Path(mcp_servers.clone())), + apps: Some(apps.clone()), hooks: Some(PluginManifestHooks::Paths(vec![hooks.clone()])), }, interface: Some(PluginManifestInterface { @@ -72,11 +72,11 @@ fn environment_descriptor_binds_every_manifest_resource() { keywords: Vec::new(), paths: PluginManifestPaths { skills: vec![resource("executor-1", skills)], - mcp_servers: Some(PluginManifestMcpServers::Paths(vec![resource( + mcp_servers: Some(PluginManifestMcpServers::Path(resource( "executor-1", mcp_servers, - )])), - apps: vec![resource("executor-1", apps)], + ))), + apps: Some(resource("executor-1", apps)), hooks: Some(PluginManifestHooks::Paths(vec![resource( "executor-1", hooks, @@ -104,8 +104,8 @@ fn environment_descriptor_rejects_resources_outside_package_root() { keywords: Vec::new(), paths: PluginManifestPaths { skills: Vec::new(), - mcp_servers: Some(PluginManifestMcpServers::Paths(vec![outside.clone()])), - apps: Vec::new(), + mcp_servers: Some(PluginManifestMcpServers::Path(outside.clone())), + apps: None, hooks: None, }, interface: None, diff --git a/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md b/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md index 0e834759a221..c1cb83541f94 100644 --- a/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md +++ b/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md @@ -61,9 +61,9 @@ - `license` (`string`): License identifier (for example `MIT`, `Apache-2.0`). - `keywords` (`array` of `string`): Search/discovery tags. - `skills` (`string` or array of `string`): Relative path(s) to skill directories/files. -- `hooks` (`string` or array of `string`): Hook config path(s). -- `mcpServers` (`string`, array of `string`, or `object`): MCP config path(s), or an object whose keys are MCP server names and whose values are MCP server config objects. -- `apps` (`string` or array of `string`): App manifest path(s) for plugin integrations. +- `hooks` (`string`): Hook config path. +- `mcpServers` (`string` or `object`): MCP config path, or an object whose keys are MCP server names and whose values are MCP server config objects. +- `apps` (`string`): App manifest path for plugin integrations. - `interface` (`object`): Interface/UX metadata block for plugin presentation. `mcpServers` may be declared as a companion file path: @@ -112,7 +112,8 @@ Or as an object directly in `plugin.json`: ### Path conventions and defaults - Path values should be relative and begin with `./`. -- `skills`, `hooks`, `apps`, and file-backed `mcpServers` can be declared as either a single path string or an array of path strings. +- `skills` can be declared as either a single path string or an array of path strings. +- `skills`, `hooks`, and string-valued `mcpServers` are supplemented on top of default component discovery; they do not replace defaults. - Custom path values must follow the plugin root convention and naming/namespacing rules. - This repo’s scaffold writes `.codex-plugin/plugin.json`; treat that as the manifest location this skill generates. From debdd4be25190b7bcb9fff8f15e42f2723c16a99 Mon Sep 17 00:00:00 2001 From: charlesgong-openai Date: Wed, 17 Jun 2026 15:10:05 -0700 Subject: [PATCH 3/7] Test plugin manifest skill path shapes --- codex-rs/core-plugins/src/manager_tests.rs | 178 +++++++++++---------- 1 file changed, 93 insertions(+), 85 deletions(-) diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 929de6f96571..77d0b43e0b0e 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -1282,36 +1282,45 @@ 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"][..], + ), + ] { + 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/", "./extra-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("extra-skills/extra-skill/SKILL.md"), - "---\nname: extra-skill\ndescription: extra 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("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", @@ -1319,10 +1328,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", @@ -1330,74 +1339,73 @@ 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; - - assert_eq!( - outcome.plugins()[0].skill_roots, - vec![ - plugin_root.join("custom-skills").abs(), - plugin_root.join("extra-skills").abs(), - plugin_root.join("skills").abs() - ] - ); - 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, + ); + let outcome = load_plugins_from_config( + &plugin_config_toml(/*enabled*/ true, /*plugins_feature_enabled*/ true), + codex_home.path(), + Some(AuthMode::Chatgpt), + ) + .await; + let expected_skill_roots = expected_skill_dirs + .iter() + .map(|dir| plugin_root.join(dir).abs()) + .chain(std::iter::once(plugin_root.join("skills").abs())) + .collect::>(); + + 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(), }, - 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")] - ); + )]) + ); + assert_eq!( + outcome.plugins()[0].apps, + vec![app_declaration("custom-app", "connector_custom")] + ); + } } #[tokio::test] From 9b7811e610d78e9b36c56e596807c6e372776c6b Mon Sep 17 00:00:00 2001 From: charlesgong-openai Date: Wed, 17 Jun 2026 15:20:31 -0700 Subject: [PATCH 4/7] Cover duplicate plugin skill roots --- codex-rs/core-plugins/src/manager_tests.rs | 83 +++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 77d0b43e0b0e..22d717688234 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; @@ -1288,6 +1292,15 @@ async fn load_plugins_uses_manifest_configured_component_paths() { 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", "skills/abc", "skills/edk"][..], + ), ] { let codex_home = TempDir::new().unwrap(); let plugin_root = codex_home @@ -1310,6 +1323,14 @@ async fn load_plugins_uses_manifest_configured_component_paths() { &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", @@ -1366,11 +1387,13 @@ async fn load_plugins_uses_manifest_configured_component_paths() { Some(AuthMode::Chatgpt), ) .await; - let expected_skill_roots = expected_skill_dirs + let mut expected_skill_roots = expected_skill_dirs .iter() .map(|dir| plugin_root.join(dir).abs()) .chain(std::iter::once(plugin_root.join("skills").abs())) .collect::>(); + expected_skill_roots.sort_unstable(); + expected_skill_roots.dedup(); assert_eq!(outcome.plugins()[0].skill_roots, expected_skill_roots); assert_eq!( @@ -1408,6 +1431,64 @@ async fn load_plugins_uses_manifest_configured_component_paths() { } } +#[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!( + skill_paths, + vec![ + canonical_skill_path("skills/abc/SKILL.md"), + canonical_skill_path("skills/edk/SKILL.md") + ] + ); +} + #[tokio::test] async fn load_plugins_ignores_manifest_component_paths_without_dot_slash() { let codex_home = TempDir::new().unwrap(); From 63a9075758ee01aadcb7639c40e943a96235c7a2 Mon Sep 17 00:00:00 2001 From: charlesgong-openai Date: Wed, 17 Jun 2026 15:37:13 -0700 Subject: [PATCH 5/7] Allow skills arrays in plugin validator --- .../plugin-creator/scripts/validate_plugin.py | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/codex-rs/skills/src/assets/samples/plugin-creator/scripts/validate_plugin.py b/codex-rs/skills/src/assets/samples/plugin-creator/scripts/validate_plugin.py index 5c6fae8e6332..bf2f119b8bd4 100644 --- a/codex-rs/skills/src/assets/samples/plugin-creator/scripts/validate_plugin.py +++ b/codex-rs/skills/src/assets/samples/plugin-creator/scripts/validate_plugin.py @@ -124,7 +124,7 @@ def validate_manifest_shape( validate_optional_non_empty_string(author, "email", errors, prefix="author") validate_optional_https_url(author, "url", errors, prefix="author") - validate_optional_contract_path(manifest, "skills", "skills", errors) + validate_optional_skills_paths(manifest, errors) validate_optional_contract_path(manifest, "apps", ".app.json", errors) validate_manifest_mcp_servers(plugin_root, manifest, errors) @@ -281,6 +281,37 @@ def validate_optional_contract_path( errors.append(f"plugin.json field `{key}` must resolve to `{expected}`") +def validate_optional_skills_paths(payload: dict[str, Any], errors: list[str]) -> None: + value = payload.get("skills") + if value is None: + return + if isinstance(value, str): + validate_manifest_relative_path(value, "skills", errors) + return + if isinstance(value, list): + for index, path in enumerate(value): + validate_manifest_relative_path(path, f"skills[{index}]", errors) + return + errors.append("plugin.json field `skills` must be a string path or array of string paths") + + +def validate_manifest_relative_path(raw_path: Any, field: str, errors: list[str]) -> None: + label = f"plugin.json field `{field}`" + if not isinstance(raw_path, str) or not raw_path.strip(): + errors.append(f"{label} must be a non-empty path string") + return + if not raw_path.startswith("./"): + errors.append(f"{label} must start with `./` relative to plugin root") + return + relative_path = raw_path[2:] + if not relative_path: + errors.append(f"{label} must not be `./`") + return + candidate = PurePosixPath(relative_path.replace("\\", "/")) + if candidate.is_absolute() or any(part in {"", ".", ".."} for part in candidate.parts): + errors.append(f"{label} must stay inside the plugin archive") + + def validate_manifest_mcp_servers( plugin_root: Path, manifest: dict[str, Any], From bd8259cc5418034c35e2ed04b441e5e4b3fedc76 Mon Sep 17 00:00:00 2001 From: charlesgong-openai Date: Wed, 17 Jun 2026 15:44:36 -0700 Subject: [PATCH 6/7] Remove plugin creator changes from skills path PR --- .../references/plugin-json-spec.md | 3 +- .../plugin-creator/scripts/validate_plugin.py | 33 +------------------ 2 files changed, 2 insertions(+), 34 deletions(-) diff --git a/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md b/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md index c1cb83541f94..ec5476ae14e2 100644 --- a/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md +++ b/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md @@ -60,7 +60,7 @@ - `repository` (`string`): Source code URL. - `license` (`string`): License identifier (for example `MIT`, `Apache-2.0`). - `keywords` (`array` of `string`): Search/discovery tags. -- `skills` (`string` or array of `string`): Relative path(s) to skill directories/files. +- `skills` (`string`): Relative path to skill directories/files. - `hooks` (`string`): Hook config path. - `mcpServers` (`string` or `object`): MCP config path, or an object whose keys are MCP server names and whose values are MCP server config objects. - `apps` (`string`): App manifest path for plugin integrations. @@ -112,7 +112,6 @@ Or as an object directly in `plugin.json`: ### Path conventions and defaults - Path values should be relative and begin with `./`. -- `skills` can be declared as either a single path string or an array of path strings. - `skills`, `hooks`, and string-valued `mcpServers` are supplemented on top of default component discovery; they do not replace defaults. - Custom path values must follow the plugin root convention and naming/namespacing rules. - This repo’s scaffold writes `.codex-plugin/plugin.json`; treat that as the manifest location this skill generates. diff --git a/codex-rs/skills/src/assets/samples/plugin-creator/scripts/validate_plugin.py b/codex-rs/skills/src/assets/samples/plugin-creator/scripts/validate_plugin.py index bf2f119b8bd4..5c6fae8e6332 100644 --- a/codex-rs/skills/src/assets/samples/plugin-creator/scripts/validate_plugin.py +++ b/codex-rs/skills/src/assets/samples/plugin-creator/scripts/validate_plugin.py @@ -124,7 +124,7 @@ def validate_manifest_shape( validate_optional_non_empty_string(author, "email", errors, prefix="author") validate_optional_https_url(author, "url", errors, prefix="author") - validate_optional_skills_paths(manifest, errors) + validate_optional_contract_path(manifest, "skills", "skills", errors) validate_optional_contract_path(manifest, "apps", ".app.json", errors) validate_manifest_mcp_servers(plugin_root, manifest, errors) @@ -281,37 +281,6 @@ def validate_optional_contract_path( errors.append(f"plugin.json field `{key}` must resolve to `{expected}`") -def validate_optional_skills_paths(payload: dict[str, Any], errors: list[str]) -> None: - value = payload.get("skills") - if value is None: - return - if isinstance(value, str): - validate_manifest_relative_path(value, "skills", errors) - return - if isinstance(value, list): - for index, path in enumerate(value): - validate_manifest_relative_path(path, f"skills[{index}]", errors) - return - errors.append("plugin.json field `skills` must be a string path or array of string paths") - - -def validate_manifest_relative_path(raw_path: Any, field: str, errors: list[str]) -> None: - label = f"plugin.json field `{field}`" - if not isinstance(raw_path, str) or not raw_path.strip(): - errors.append(f"{label} must be a non-empty path string") - return - if not raw_path.startswith("./"): - errors.append(f"{label} must start with `./` relative to plugin root") - return - relative_path = raw_path[2:] - if not relative_path: - errors.append(f"{label} must not be `./`") - return - candidate = PurePosixPath(relative_path.replace("\\", "/")) - if candidate.is_absolute() or any(part in {"", ".", ".."} for part in candidate.parts): - errors.append(f"{label} must stay inside the plugin archive") - - def validate_manifest_mcp_servers( plugin_root: Path, manifest: dict[str, Any], From 0a885c2763d255a8af6adc4b4f678899d34fea4d Mon Sep 17 00:00:00 2001 From: charlesgong-openai Date: Wed, 17 Jun 2026 21:10:34 -0700 Subject: [PATCH 7/7] Respect plugin manifest skill path overrides --- codex-rs/core-plugins/src/loader.rs | 7 +++++-- codex-rs/core-plugins/src/manager_tests.rs | 3 +-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 02f3b4d330f4..846487aba6f8 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -819,8 +819,11 @@ fn plugin_skill_roots( plugin_root: &AbsolutePathBuf, manifest_paths: &PluginManifestPaths, ) -> Vec { - let mut paths = default_skill_roots(plugin_root); - paths.extend(manifest_paths.skills.iter().cloned()); + 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 22d717688234..b5d22911f9db 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -1299,7 +1299,7 @@ async fn load_plugins_uses_manifest_configured_component_paths() { (r#""./skills/""#, &["skills"][..]), ( r#"["./skills/abc/", "./skills/edk/"]"#, - &["skills", "skills/abc", "skills/edk"][..], + &["skills/abc", "skills/edk"][..], ), ] { let codex_home = TempDir::new().unwrap(); @@ -1390,7 +1390,6 @@ async fn load_plugins_uses_manifest_configured_component_paths() { let mut expected_skill_roots = expected_skill_dirs .iter() .map(|dir| plugin_root.join(dir).abs()) - .chain(std::iter::once(plugin_root.join("skills").abs())) .collect::>(); expected_skill_roots.sort_unstable(); expected_skill_roots.dedup();