From 9b004e2db126995307bc32735ad3ee20e6fe5ced Mon Sep 17 00:00:00 2001 From: xl-openai Date: Tue, 3 Mar 2026 15:00:18 -0800 Subject: [PATCH] Refactor plugin config and cache path (#13333) Update config.toml plugin entries to use @ as the key. Plugin now stays in [plugins/cache/marketplace-name/plugin-name/$version/] Clean up the plugin code structure. Add plugin install functionality (not used yet). --- codex-rs/core/config.schema.json | 6 - codex-rs/core/src/config/types.rs | 1 - codex-rs/core/src/mcp/mod.rs | 15 +- .../src/{plugins.rs => plugins/manager.rs} | 253 ++++++++---- codex-rs/core/src/plugins/manifest.rs | 37 ++ codex-rs/core/src/plugins/mod.rs | 14 + codex-rs/core/src/plugins/store.rs | 374 ++++++++++++++++++ codex-rs/core/tests/suite/plugins.rs | 14 +- 8 files changed, 622 insertions(+), 92 deletions(-) rename codex-rs/core/src/{plugins.rs => plugins/manager.rs} (72%) create mode 100644 codex-rs/core/src/plugins/manifest.rs create mode 100644 codex-rs/core/src/plugins/mod.rs create mode 100644 codex-rs/core/src/plugins/store.rs diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 3cd700091..74af6d50c 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -1107,14 +1107,8 @@ "enabled": { "default": true, "type": "boolean" - }, - "path": { - "$ref": "#/definitions/AbsolutePathBuf" } }, - "required": [ - "path" - ], "type": "object" }, "ProjectConfig": { diff --git a/codex-rs/core/src/config/types.rs b/codex-rs/core/src/config/types.rs index 8fd5b109d..6f8c34915 100644 --- a/codex-rs/core/src/config/types.rs +++ b/codex-rs/core/src/config/types.rs @@ -778,7 +778,6 @@ pub struct SkillConfig { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] #[schemars(deny_unknown_fields)] pub struct PluginConfig { - pub path: AbsolutePathBuf, #[serde(default = "default_enabled")] pub enabled: bool, } diff --git a/codex-rs/core/src/mcp/mod.rs b/codex-rs/core/src/mcp/mod.rs index 4edb45c46..91a7f74db 100644 --- a/codex-rs/core/src/mcp/mod.rs +++ b/codex-rs/core/src/mcp/mod.rs @@ -417,7 +417,7 @@ mod tests { fs::write(path, contents).unwrap(); } - fn plugin_config_toml(plugin_root: &Path) -> String { + fn plugin_config_toml() -> String { let mut root = toml::map::Map::new(); let mut features = toml::map::Map::new(); @@ -425,14 +425,10 @@ mod tests { root.insert("features".to_string(), Value::Table(features)); let mut plugin = toml::map::Map::new(); - plugin.insert( - "path".to_string(), - Value::String(plugin_root.display().to_string()), - ); plugin.insert("enabled".to_string(), Value::Boolean(true)); let mut plugins = toml::map::Map::new(); - plugins.insert("sample".to_string(), Value::Table(plugin)); + plugins.insert("sample@test".to_string(), Value::Table(plugin)); root.insert("plugins".to_string(), Value::Table(plugins)); toml::to_string(&Value::Table(root)).expect("plugin test config should serialize") @@ -616,7 +612,10 @@ mod tests { #[tokio::test] async fn effective_mcp_servers_include_plugins_without_overriding_user_config() { let codex_home = tempfile::tempdir().expect("tempdir"); - let plugin_root = codex_home.path().join("plugin-sample"); + let plugin_root = codex_home + .path() + .join("plugins/cache") + .join("test/sample/local"); write_file( &plugin_root.join(".codex-plugin/plugin.json"), r#"{"name":"sample"}"#, @@ -638,7 +637,7 @@ mod tests { ); write_file( &codex_home.path().join(CONFIG_TOML_FILE), - &plugin_config_toml(&plugin_root), + &plugin_config_toml(), ); let mut config = ConfigBuilder::default() diff --git a/codex-rs/core/src/plugins.rs b/codex-rs/core/src/plugins/manager.rs similarity index 72% rename from codex-rs/core/src/plugins.rs rename to codex-rs/core/src/plugins/manager.rs index 890d15a32..d98c4379f 100644 --- a/codex-rs/core/src/plugins.rs +++ b/codex-rs/core/src/plugins/manager.rs @@ -1,4 +1,14 @@ +use super::load_plugin_manifest; +use super::plugin_manifest_name; +use super::store::DEFAULT_PLUGIN_VERSION; +use super::store::PluginId; +use super::store::PluginInstallRequest; +use super::store::PluginInstallResult; +use super::store::PluginStore; +use super::store::PluginStoreError; use crate::config::Config; +use crate::config::ConfigService; +use crate::config::ConfigServiceError; use crate::config::ConfigToml; use crate::config::profile::ConfigProfile; use crate::config::types::McpServerConfig; @@ -7,10 +17,13 @@ use crate::config_loader::ConfigLayerStack; use crate::features::Feature; use crate::features::FeatureOverrides; use crate::features::Features; +use codex_app_server_protocol::ConfigValueWriteParams; +use codex_app_server_protocol::MergeStrategy; use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use serde_json::Map as JsonMap; use serde_json::Value as JsonValue; +use serde_json::json; use std::collections::HashMap; use std::fs; use std::path::Path; @@ -18,7 +31,6 @@ use std::path::PathBuf; use std::sync::RwLock; use tracing::warn; -const PLUGIN_MANIFEST_PATH: &str = ".codex-plugin/plugin.json"; const DEFAULT_SKILLS_DIR_NAME: &str = "skills"; const DEFAULT_MCP_CONFIG_FILE: &str = ".mcp.json"; @@ -71,12 +83,16 @@ impl PluginLoadOutcome { } pub struct PluginsManager { + codex_home: PathBuf, + store: PluginStore, cache_by_cwd: RwLock>, } impl PluginsManager { - pub fn new(_codex_home: PathBuf) -> Self { + pub fn new(codex_home: PathBuf) -> Self { Self { + codex_home: codex_home.clone(), + store: PluginStore::new(codex_home), cache_by_cwd: RwLock::new(HashMap::new()), } } @@ -104,7 +120,7 @@ impl PluginsManager { return outcome; } - let outcome = load_plugins_from_layer_stack(config_layer_stack); + let outcome = load_plugins_from_layer_stack(config_layer_stack, &self.store); log_plugin_load_errors(&outcome); let mut cache = match self.cache_by_cwd.write() { Ok(cache) => cache, @@ -128,6 +144,50 @@ impl PluginsManager { Err(err) => err.into_inner().get(cwd).cloned(), } } + + pub async fn install_plugin( + &self, + request: PluginInstallRequest, + ) -> Result { + let store = self.store.clone(); + let result = tokio::task::spawn_blocking(move || store.install(request)) + .await + .map_err(PluginInstallError::join)??; + + ConfigService::new_with_defaults(self.codex_home.clone()) + .write_value(ConfigValueWriteParams { + key_path: format!("plugins.{}", result.plugin_id.as_key()), + value: json!({ + "enabled": true, + }), + merge_strategy: MergeStrategy::Replace, + file_path: None, + expected_version: None, + }) + .await + .map(|_| ()) + .map_err(PluginInstallError::from)?; + + Ok(result) + } +} + +#[derive(Debug, thiserror::Error)] +pub enum PluginInstallError { + #[error("{0}")] + Store(#[from] PluginStoreError), + + #[error("{0}")] + Config(#[from] ConfigServiceError), + + #[error("failed to join plugin install task: {0}")] + Join(#[from] tokio::task::JoinError), +} + +impl PluginInstallError { + fn join(source: tokio::task::JoinError) -> Self { + Self::Join(source) + } } fn plugins_feature_enabled_from_stack(config_layer_stack: &ConfigLayerStack) -> bool { @@ -160,11 +220,6 @@ fn log_plugin_load_errors(outcome: &PluginLoadOutcome) { } } -#[derive(Debug, Default, Deserialize)] -struct PluginManifest { - name: String, -} - #[derive(Debug, Default, Deserialize)] #[serde(rename_all = "camelCase")] struct PluginMcpFile { @@ -172,7 +227,10 @@ struct PluginMcpFile { mcp_servers: HashMap, } -pub fn load_plugins_from_layer_stack(config_layer_stack: &ConfigLayerStack) -> PluginLoadOutcome { +pub(crate) fn load_plugins_from_layer_stack( + config_layer_stack: &ConfigLayerStack, + store: &PluginStore, +) -> PluginLoadOutcome { let mut configured_plugins: Vec<_> = configured_plugins_from_stack(config_layer_stack) .into_iter() .collect(); @@ -181,7 +239,7 @@ pub fn load_plugins_from_layer_stack(config_layer_stack: &ConfigLayerStack) -> P let mut plugins = Vec::with_capacity(configured_plugins.len()); let mut seen_mcp_server_names = HashMap::::new(); for (configured_name, plugin) in configured_plugins { - let loaded_plugin = load_plugin(configured_name.clone(), &plugin); + let loaded_plugin = load_plugin(configured_name.clone(), &plugin, store); for name in loaded_plugin.mcp_servers.keys() { if let Some(previous_plugin) = seen_mcp_server_names.insert(name.clone(), configured_name.clone()) @@ -226,12 +284,18 @@ fn configured_plugins_from_stack( } } -fn load_plugin(config_name: String, plugin: &PluginConfig) -> LoadedPlugin { - let plugin_root = plugin.path.clone(); +fn load_plugin(config_name: String, plugin: &PluginConfig, store: &PluginStore) -> LoadedPlugin { + let plugin_version = DEFAULT_PLUGIN_VERSION.to_string(); + let plugin_root = PluginId::parse(&config_name) + .map(|plugin_id| store.plugin_root(&plugin_id, &plugin_version)); + let root = match &plugin_root { + Ok(plugin_root) => plugin_root.clone(), + Err(_) => store.root().clone(), + }; let mut loaded_plugin = LoadedPlugin { config_name, manifest_name: None, - root: plugin_root.clone(), + root, enabled: plugin.enabled, skill_roots: Vec::new(), mcp_servers: HashMap::new(), @@ -242,6 +306,14 @@ fn load_plugin(config_name: String, plugin: &PluginConfig) -> LoadedPlugin { return loaded_plugin; } + let plugin_root = match plugin_root { + Ok(plugin_root) => plugin_root, + Err(err) => { + loaded_plugin.error = Some(err.to_string()); + return loaded_plugin; + } + }; + if !plugin_root.as_path().is_dir() { loaded_plugin.error = Some("path does not exist or is not a directory".to_string()); return loaded_plugin; @@ -272,33 +344,6 @@ fn load_plugin(config_name: String, plugin: &PluginConfig) -> LoadedPlugin { loaded_plugin } -fn load_plugin_manifest(plugin_root: &Path) -> Option { - let manifest_path = plugin_root.join(PLUGIN_MANIFEST_PATH); - if !manifest_path.is_file() { - return None; - } - let contents = fs::read_to_string(&manifest_path).ok()?; - match serde_json::from_str(&contents) { - Ok(manifest) => Some(manifest), - Err(err) => { - warn!( - path = %manifest_path.display(), - "failed to parse plugin manifest: {err}" - ); - None - } - } -} - -fn plugin_manifest_name(manifest: &PluginManifest, plugin_root: &Path) -> String { - plugin_root - .file_name() - .and_then(|name| name.to_str()) - .filter(|_| manifest.name.trim().is_empty()) - .unwrap_or(&manifest.name) - .to_string() -} - fn default_skill_roots(plugin_root: &Path) -> Vec { let skills_dir = plugin_root.join(DEFAULT_SKILLS_DIR_NAME); if skills_dir.is_dir() { @@ -422,6 +467,7 @@ mod tests { use crate::config::ConfigBuilder; use crate::config::types::McpServerTransportConfig; use pretty_assertions::assert_eq; + use std::fs; use tempfile::TempDir; use toml::Value; @@ -430,11 +476,20 @@ mod tests { fs::write(path, contents).unwrap(); } - fn plugin_config_toml( - plugin_root: &Path, - enabled: bool, - plugins_feature_enabled: bool, - ) -> String { + fn write_plugin(root: &Path, dir_name: &str, manifest_name: &str) { + let plugin_root = root.join(dir_name); + fs::create_dir_all(plugin_root.join(".codex-plugin")).unwrap(); + fs::create_dir_all(plugin_root.join("skills")).unwrap(); + fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + format!(r#"{{"name":"{manifest_name}"}}"#), + ) + .unwrap(); + fs::write(plugin_root.join("skills/SKILL.md"), "skill").unwrap(); + fs::write(plugin_root.join(".mcp.json"), r#"{"mcpServers":{}}"#).unwrap(); + } + + fn plugin_config_toml(enabled: bool, plugins_feature_enabled: bool) -> String { let mut root = toml::map::Map::new(); let mut features = toml::map::Map::new(); @@ -445,14 +500,10 @@ mod tests { root.insert("features".to_string(), Value::Table(features)); let mut plugin = toml::map::Map::new(); - plugin.insert( - "path".to_string(), - Value::String(plugin_root.display().to_string()), - ); plugin.insert("enabled".to_string(), Value::Boolean(enabled)); let mut plugins = toml::map::Map::new(); - plugins.insert("sample".to_string(), Value::Table(plugin)); + plugins.insert("sample@test".to_string(), Value::Table(plugin)); root.insert("plugins".to_string(), Value::Table(plugins)); toml::to_string(&Value::Table(root)).expect("plugin test config should serialize") @@ -471,7 +522,10 @@ mod tests { #[tokio::test] async fn load_plugins_loads_default_skills_and_mcp_servers() { let codex_home = TempDir::new().unwrap(); - let plugin_root = codex_home.path().join("plugin-sample"); + let plugin_root = codex_home + .path() + .join("plugins/cache") + .join("test/sample/local"); write_file( &plugin_root.join(".codex-plugin/plugin.json"), @@ -497,16 +551,13 @@ mod tests { }"#, ); - let outcome = load_plugins_from_config( - &plugin_config_toml(&plugin_root, true, true), - codex_home.path(), - ) - .await; + let outcome = + load_plugins_from_config(&plugin_config_toml(true, true), codex_home.path()).await; assert_eq!( outcome.plugins, vec![LoadedPlugin { - config_name: "sample".to_string(), + config_name: "sample@test".to_string(), manifest_name: Some("sample".to_string()), root: AbsolutePathBuf::try_from(plugin_root.clone()).unwrap(), enabled: true, @@ -544,7 +595,10 @@ mod tests { #[tokio::test] async fn load_plugins_preserves_disabled_plugins_without_effective_contributions() { let codex_home = TempDir::new().unwrap(); - let plugin_root = codex_home.path().join("plugin-sample"); + let plugin_root = codex_home + .path() + .join("plugins/cache") + .join("test/sample/local"); write_file( &plugin_root.join(".codex-plugin/plugin.json"), @@ -562,16 +616,13 @@ mod tests { }"#, ); - let outcome = load_plugins_from_config( - &plugin_config_toml(&plugin_root, false, true), - codex_home.path(), - ) - .await; + let outcome = + load_plugins_from_config(&plugin_config_toml(false, true), codex_home.path()).await; assert_eq!( outcome.plugins, vec![LoadedPlugin { - config_name: "sample".to_string(), + config_name: "sample@test".to_string(), manifest_name: None, root: AbsolutePathBuf::try_from(plugin_root).unwrap(), enabled: false, @@ -605,7 +656,10 @@ mod tests { #[tokio::test] async fn load_plugins_returns_empty_when_feature_disabled() { let codex_home = TempDir::new().unwrap(); - let plugin_root = codex_home.path().join("plugin-sample"); + let plugin_root = codex_home + .path() + .join("plugins/cache") + .join("test/sample/local"); write_file( &plugin_root.join(".codex-plugin/plugin.json"), @@ -616,12 +670,77 @@ mod tests { "---\nname: sample-search\ndescription: search sample data\n---\n", ); + let outcome = + load_plugins_from_config(&plugin_config_toml(true, false), codex_home.path()).await; + + assert_eq!(outcome, PluginLoadOutcome::default()); + } + + #[tokio::test] + async fn load_plugins_rejects_invalid_plugin_keys() { + 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#"{"name":"sample"}"#, + ); + + let mut root = toml::map::Map::new(); + let mut features = toml::map::Map::new(); + features.insert("plugins".to_string(), Value::Boolean(true)); + root.insert("features".to_string(), Value::Table(features)); + + let mut plugin = toml::map::Map::new(); + plugin.insert("enabled".to_string(), Value::Boolean(true)); + + let mut plugins = toml::map::Map::new(); + plugins.insert("sample".to_string(), Value::Table(plugin)); + root.insert("plugins".to_string(), Value::Table(plugins)); + let outcome = load_plugins_from_config( - &plugin_config_toml(&plugin_root, true, false), + &toml::to_string(&Value::Table(root)).expect("plugin test config should serialize"), codex_home.path(), ) .await; - assert_eq!(outcome, PluginLoadOutcome::default()); + assert_eq!(outcome.plugins.len(), 1); + assert_eq!( + outcome.plugins[0].error.as_deref(), + Some("invalid plugin key `sample`; expected @") + ); + assert!(outcome.effective_skill_roots().is_empty()); + assert!(outcome.effective_mcp_servers().is_empty()); + } + + #[tokio::test] + async fn install_plugin_updates_config_with_relative_path_and_plugin_key() { + let tmp = tempfile::tempdir().unwrap(); + write_plugin(tmp.path(), "sample-plugin", "sample-plugin"); + + let result = PluginsManager::new(tmp.path().to_path_buf()) + .install_plugin(PluginInstallRequest { + source_path: tmp.path().join("sample-plugin"), + marketplace_name: None, + }) + .await + .unwrap(); + + let installed_path = tmp.path().join("plugins/cache/debug/sample-plugin/local"); + assert_eq!( + result, + PluginInstallResult { + plugin_id: PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap(), + plugin_version: "local".to_string(), + installed_path, + } + ); + + let config = fs::read_to_string(tmp.path().join("config.toml")).unwrap(); + assert!(config.contains(r#"[plugins."sample-plugin@debug"]"#)); + assert!(config.contains("enabled = true")); } } diff --git a/codex-rs/core/src/plugins/manifest.rs b/codex-rs/core/src/plugins/manifest.rs new file mode 100644 index 000000000..ad677de81 --- /dev/null +++ b/codex-rs/core/src/plugins/manifest.rs @@ -0,0 +1,37 @@ +use serde::Deserialize; +use std::fs; +use std::path::Path; + +pub(crate) const PLUGIN_MANIFEST_PATH: &str = ".codex-plugin/plugin.json"; + +#[derive(Debug, Default, Deserialize)] +pub(crate) struct PluginManifest { + name: String, +} + +pub(crate) fn load_plugin_manifest(plugin_root: &Path) -> Option { + let manifest_path = plugin_root.join(PLUGIN_MANIFEST_PATH); + if !manifest_path.is_file() { + return None; + } + let contents = fs::read_to_string(&manifest_path).ok()?; + match serde_json::from_str(&contents) { + Ok(manifest) => Some(manifest), + Err(err) => { + tracing::warn!( + path = %manifest_path.display(), + "failed to parse plugin manifest: {err}" + ); + None + } + } +} + +pub(crate) fn plugin_manifest_name(manifest: &PluginManifest, plugin_root: &Path) -> String { + plugin_root + .file_name() + .and_then(|name| name.to_str()) + .filter(|_| manifest.name.trim().is_empty()) + .unwrap_or(&manifest.name) + .to_string() +} diff --git a/codex-rs/core/src/plugins/mod.rs b/codex-rs/core/src/plugins/mod.rs new file mode 100644 index 000000000..faf0a8dd0 --- /dev/null +++ b/codex-rs/core/src/plugins/mod.rs @@ -0,0 +1,14 @@ +mod manager; +mod manifest; +mod store; + +pub use manager::LoadedPlugin; +pub use manager::PluginInstallError; +pub use manager::PluginLoadOutcome; +pub use manager::PluginsManager; +pub(crate) use manager::plugin_namespace_for_skill_path; +pub(crate) use manifest::load_plugin_manifest; +pub(crate) use manifest::plugin_manifest_name; +pub use store::PluginId; +pub use store::PluginInstallRequest; +pub use store::PluginInstallResult; diff --git a/codex-rs/core/src/plugins/store.rs b/codex-rs/core/src/plugins/store.rs new file mode 100644 index 000000000..96c355805 --- /dev/null +++ b/codex-rs/core/src/plugins/store.rs @@ -0,0 +1,374 @@ +use super::load_plugin_manifest; +use super::manifest::PLUGIN_MANIFEST_PATH; +use super::plugin_manifest_name; +use codex_utils_absolute_path::AbsolutePathBuf; +use std::fs; +use std::io; +use std::path::Path; +use std::path::PathBuf; + +const DEFAULT_MARKETPLACE_NAME: &str = "debug"; +pub(crate) const DEFAULT_PLUGIN_VERSION: &str = "local"; +pub(crate) const PLUGINS_CACHE_DIR: &str = "plugins/cache"; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PluginInstallRequest { + pub source_path: PathBuf, + pub marketplace_name: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PluginId { + pub plugin_name: String, + pub marketplace_name: String, +} + +impl PluginId { + pub fn new(plugin_name: String, marketplace_name: String) -> Result { + validate_plugin_segment(&plugin_name, "plugin name") + .map_err(PluginStoreError::InvalidPluginKey)?; + validate_plugin_segment(&marketplace_name, "marketplace name") + .map_err(PluginStoreError::InvalidPluginKey)?; + Ok(Self { + plugin_name, + marketplace_name, + }) + } + + pub fn parse(plugin_key: &str) -> Result { + let Some((plugin_name, marketplace_name)) = plugin_key.rsplit_once('@') else { + return Err(PluginStoreError::InvalidPluginKey(format!( + "invalid plugin key `{plugin_key}`; expected @" + ))); + }; + if plugin_name.is_empty() || marketplace_name.is_empty() { + return Err(PluginStoreError::InvalidPluginKey(format!( + "invalid plugin key `{plugin_key}`; expected @" + ))); + } + + Self::new(plugin_name.to_string(), marketplace_name.to_string()).map_err(|err| match err { + PluginStoreError::InvalidPluginKey(message) => { + PluginStoreError::InvalidPluginKey(format!("{message} in `{plugin_key}`")) + } + other => other, + }) + } + + pub fn as_key(&self) -> String { + format!("{}@{}", self.plugin_name, self.marketplace_name) + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PluginInstallResult { + pub plugin_id: PluginId, + pub plugin_version: String, + pub installed_path: PathBuf, +} + +#[derive(Debug, Clone)] +pub struct PluginStore { + root: AbsolutePathBuf, +} + +impl PluginStore { + pub fn new(codex_home: PathBuf) -> Self { + Self { + root: AbsolutePathBuf::try_from(codex_home.join(PLUGINS_CACHE_DIR)) + .unwrap_or_else(|err| panic!("plugin cache root should be absolute: {err}")), + } + } + + pub fn root(&self) -> &AbsolutePathBuf { + &self.root + } + + pub fn plugin_root(&self, plugin_id: &PluginId, plugin_version: &str) -> AbsolutePathBuf { + AbsolutePathBuf::try_from( + self.root + .as_path() + .join(&plugin_id.marketplace_name) + .join(&plugin_id.plugin_name) + .join(plugin_version), + ) + .unwrap_or_else(|err| panic!("plugin cache path should resolve to an absolute path: {err}")) + } + + pub fn install( + &self, + request: PluginInstallRequest, + ) -> Result { + let source_path = request.source_path; + if !source_path.is_dir() { + return Err(PluginStoreError::InvalidPlugin(format!( + "plugin source path is not a directory: {}", + source_path.display() + ))); + } + + let plugin_name = plugin_name_for_source(&source_path)?; + let marketplace_name = request + .marketplace_name + .filter(|name| !name.trim().is_empty()) + .unwrap_or_else(|| DEFAULT_MARKETPLACE_NAME.to_string()); + let plugin_version = DEFAULT_PLUGIN_VERSION.to_string(); + let plugin_id = match PluginId::new(plugin_name, marketplace_name) { + Ok(plugin_id) => plugin_id, + Err(PluginStoreError::InvalidPluginKey(message)) => { + return Err(PluginStoreError::InvalidPlugin(message)); + } + Err(err) => return Err(err), + }; + let installed_path = self + .plugin_root(&plugin_id, &plugin_version) + .into_path_buf(); + + if let Some(parent) = installed_path.parent() { + fs::create_dir_all(parent).map_err(|err| { + PluginStoreError::io("failed to create plugin cache directory", err) + })?; + } + + remove_existing_target(&installed_path)?; + copy_dir_recursive(&source_path, &installed_path)?; + + Ok(PluginInstallResult { + plugin_id, + plugin_version, + installed_path, + }) + } +} + +#[derive(Debug, thiserror::Error)] +pub enum PluginStoreError { + #[error("{context}: {source}")] + Io { + context: &'static str, + #[source] + source: io::Error, + }, + + #[error("{0}")] + InvalidPlugin(String), + + #[error("{0}")] + InvalidPluginKey(String), +} + +impl PluginStoreError { + fn io(context: &'static str, source: io::Error) -> Self { + Self::Io { context, source } + } +} + +fn plugin_name_for_source(source_path: &Path) -> Result { + let manifest_path = source_path.join(PLUGIN_MANIFEST_PATH); + if !manifest_path.is_file() { + return Err(PluginStoreError::InvalidPlugin(format!( + "missing plugin manifest: {}", + manifest_path.display() + ))); + } + + let manifest = load_plugin_manifest(source_path).ok_or_else(|| { + PluginStoreError::InvalidPlugin(format!( + "missing or invalid plugin manifest: {}", + manifest_path.display() + )) + })?; + + let plugin_name = plugin_manifest_name(&manifest, source_path); + validate_plugin_segment(&plugin_name, "plugin name") + .map_err(PluginStoreError::InvalidPlugin) + .map(|_| plugin_name) +} + +fn validate_plugin_segment(segment: &str, kind: &str) -> Result<(), String> { + if segment.is_empty() { + return Err(format!("invalid {kind}: must not be empty")); + } + if !segment + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || ch == '-' || ch == '_') + { + return Err(format!( + "invalid {kind}: only ASCII letters, digits, `_`, and `-` are allowed" + )); + } + Ok(()) +} + +fn remove_existing_target(path: &Path) -> Result<(), PluginStoreError> { + if !path.exists() { + return Ok(()); + } + + if path.is_dir() { + fs::remove_dir_all(path).map_err(|err| { + PluginStoreError::io("failed to remove existing plugin cache entry", err) + }) + } else { + fs::remove_file(path).map_err(|err| { + PluginStoreError::io("failed to remove existing plugin cache entry", err) + }) + } +} + +fn copy_dir_recursive(source: &Path, target: &Path) -> Result<(), PluginStoreError> { + fs::create_dir_all(target) + .map_err(|err| PluginStoreError::io("failed to create plugin target directory", err))?; + + for entry in fs::read_dir(source) + .map_err(|err| PluginStoreError::io("failed to read plugin source directory", err))? + { + let entry = + entry.map_err(|err| PluginStoreError::io("failed to enumerate plugin source", err))?; + let source_path = entry.path(); + let target_path = target.join(entry.file_name()); + let file_type = entry + .file_type() + .map_err(|err| PluginStoreError::io("failed to inspect plugin source entry", err))?; + + if file_type.is_dir() { + copy_dir_recursive(&source_path, &target_path)?; + } else if file_type.is_file() { + fs::copy(&source_path, &target_path) + .map_err(|err| PluginStoreError::io("failed to copy plugin file", err))?; + } + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use tempfile::tempdir; + + fn write_plugin(root: &Path, dir_name: &str, manifest_name: &str) { + let plugin_root = root.join(dir_name); + fs::create_dir_all(plugin_root.join(".codex-plugin")).unwrap(); + fs::create_dir_all(plugin_root.join("skills")).unwrap(); + fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + format!(r#"{{"name":"{manifest_name}"}}"#), + ) + .unwrap(); + fs::write(plugin_root.join("skills/SKILL.md"), "skill").unwrap(); + fs::write(plugin_root.join(".mcp.json"), r#"{"mcpServers":{}}"#).unwrap(); + } + + #[test] + fn install_copies_plugin_into_default_marketplace() { + let tmp = tempdir().unwrap(); + write_plugin(tmp.path(), "sample-plugin", "sample-plugin"); + + let result = PluginStore::new(tmp.path().to_path_buf()) + .install(PluginInstallRequest { + source_path: tmp.path().join("sample-plugin"), + marketplace_name: None, + }) + .unwrap(); + + let installed_path = tmp.path().join("plugins/cache/debug/sample-plugin/local"); + assert_eq!( + result, + PluginInstallResult { + plugin_id: PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap(), + plugin_version: "local".to_string(), + installed_path: installed_path.clone(), + } + ); + assert!(installed_path.join(".codex-plugin/plugin.json").is_file()); + assert!(installed_path.join("skills/SKILL.md").is_file()); + } + + #[test] + fn install_uses_manifest_name_for_destination_and_key() { + let tmp = tempdir().unwrap(); + write_plugin(tmp.path(), "source-dir", "manifest-name"); + + let result = PluginStore::new(tmp.path().to_path_buf()) + .install(PluginInstallRequest { + source_path: tmp.path().join("source-dir"), + marketplace_name: Some("market".to_string()), + }) + .unwrap(); + + assert_eq!( + result, + PluginInstallResult { + plugin_id: PluginId::new("manifest-name".to_string(), "market".to_string()) + .unwrap(), + plugin_version: "local".to_string(), + installed_path: tmp.path().join("plugins/cache/market/manifest-name/local"), + } + ); + } + + #[test] + fn plugin_root_derives_path_from_key_and_version() { + let tmp = tempdir().unwrap(); + let store = PluginStore::new(tmp.path().to_path_buf()); + let plugin_id = PluginId::new("sample".to_string(), "debug".to_string()).unwrap(); + + assert_eq!( + store.plugin_root(&plugin_id, "local").as_path(), + tmp.path().join("plugins/cache/debug/sample/local") + ); + } + + #[test] + fn plugin_root_rejects_path_separators_in_key_segments() { + let err = PluginId::parse("../../etc@debug").unwrap_err(); + assert_eq!( + err.to_string(), + "invalid plugin name: only ASCII letters, digits, `_`, and `-` are allowed in `../../etc@debug`" + ); + + let err = PluginId::parse("sample@../../etc").unwrap_err(); + assert_eq!( + err.to_string(), + "invalid marketplace name: only ASCII letters, digits, `_`, and `-` are allowed in `sample@../../etc`" + ); + } + + #[test] + fn install_rejects_manifest_names_with_path_separators() { + let tmp = tempdir().unwrap(); + write_plugin(tmp.path(), "source-dir", "../../etc"); + + let err = PluginStore::new(tmp.path().to_path_buf()) + .install(PluginInstallRequest { + source_path: tmp.path().join("source-dir"), + marketplace_name: None, + }) + .unwrap_err(); + + assert_eq!( + err.to_string(), + "invalid plugin name: only ASCII letters, digits, `_`, and `-` are allowed" + ); + } + + #[test] + fn install_rejects_marketplace_names_with_path_separators() { + let tmp = tempdir().unwrap(); + write_plugin(tmp.path(), "source-dir", "sample-plugin"); + + let err = PluginStore::new(tmp.path().to_path_buf()) + .install(PluginInstallRequest { + source_path: tmp.path().join("source-dir"), + marketplace_name: Some("../../etc".to_string()), + }) + .unwrap_err(); + + assert_eq!( + err.to_string(), + "invalid marketplace name: only ASCII letters, digits, `_`, and `-` are allowed" + ); + } +} diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index ed2ba54a3..3e641d010 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -24,7 +24,7 @@ use tempfile::TempDir; use wiremock::MockServer; fn write_plugin_skill_plugin(home: &TempDir) -> std::path::PathBuf { - let plugin_root = home.path().join("plugins/sample"); + let plugin_root = home.path().join("plugins/cache/test/sample/local"); let skill_dir = plugin_root.join("skills/sample-search"); std::fs::create_dir_all(skill_dir.as_path()).expect("create plugin skill dir"); std::fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("create plugin manifest dir"); @@ -40,17 +40,14 @@ fn write_plugin_skill_plugin(home: &TempDir) -> std::path::PathBuf { .expect("write plugin skill"); std::fs::write( home.path().join("config.toml"), - format!( - "[features]\nplugins = true\n\n[plugins.sample]\nenabled = true\npath = \"{}\"\n", - plugin_root.display() - ), + "[features]\nplugins = true\n\n[plugins.\"sample@test\"]\nenabled = true\n", ) .expect("write config"); skill_dir.join("SKILL.md") } fn write_plugin_mcp_plugin(home: &TempDir, command: &str) { - let plugin_root = home.path().join("plugins/sample"); + let plugin_root = home.path().join("plugins/cache/test/sample/local"); std::fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("create plugin manifest dir"); std::fs::write( plugin_root.join(".codex-plugin/plugin.json"), @@ -72,10 +69,7 @@ fn write_plugin_mcp_plugin(home: &TempDir, command: &str) { .expect("write plugin mcp config"); std::fs::write( home.path().join("config.toml"), - format!( - "[features]\nplugins = true\n\n[plugins.sample]\nenabled = true\npath = \"{}\"\n", - plugin_root.display() - ), + "[features]\nplugins = true\n\n[plugins.\"sample@test\"]\nenabled = true\n", ) .expect("write config"); }