diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 6475531c0..8d9dbe05e 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -479,19 +479,6 @@ impl CodexMessageProcessor { self.thread_manager.skills_manager().clear_cache(); } - pub(crate) async fn maybe_start_plugin_startup_tasks_for_latest_config(&self) { - match self.load_latest_config(/*fallback_cwd*/ None).await { - Ok(config) => self - .thread_manager - .plugins_manager() - .maybe_start_plugin_startup_tasks_for_config( - &config, - self.thread_manager.auth_manager(), - ), - Err(err) => warn!("failed to load latest config for plugin startup tasks: {err:?}"), - } - } - fn current_account_updated_notification(&self) -> AccountUpdatedNotification { let auth = self.auth_manager.auth_cached(); AccountUpdatedNotification { @@ -5964,6 +5951,7 @@ impl CodexMessageProcessor { force_remote_sync, } = params; let roots = cwds.unwrap_or_default(); + plugins_manager.maybe_start_non_curated_plugin_cache_refresh_for_roots(&roots); let mut config = match self.load_latest_config(/*fallback_cwd*/ None).await { Ok(config) => config, diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 8e3c5be81..4bcce5c7c 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -868,9 +868,6 @@ impl MessageProcessor { match self.config_api.write_value(params).await { Ok(response) => { self.codex_message_processor.clear_plugin_related_caches(); - self.codex_message_processor - .maybe_start_plugin_startup_tasks_for_latest_config() - .await; self.outgoing.send_response(request_id, response).await; } Err(error) => self.outgoing.send_error(request_id, error).await, @@ -899,9 +896,6 @@ impl MessageProcessor { { Ok(response) => { self.codex_message_processor.clear_plugin_related_caches(); - self.codex_message_processor - .maybe_start_plugin_startup_tasks_for_latest_config() - .await; self.outgoing.send_response(request_id, response).await; if should_refresh_apps_list { self.refresh_apps_list_after_experimental_feature_enablement_set() @@ -982,9 +976,6 @@ impl MessageProcessor { match result { Ok(response) => { self.codex_message_processor.clear_plugin_related_caches(); - self.codex_message_processor - .maybe_start_plugin_startup_tasks_for_latest_config() - .await; self.outgoing.send_response(request_id, response).await; } Err(error) => self.outgoing.send_error(request_id, error).await, diff --git a/codex-rs/core/src/plugins/manager.rs b/codex-rs/core/src/plugins/manager.rs index 388c677e1..1da98b85c 100644 --- a/codex-rs/core/src/plugins/manager.rs +++ b/codex-rs/core/src/plugins/manager.rs @@ -25,6 +25,7 @@ use super::startup_sync::start_startup_remote_plugin_sync_once; use super::store::PluginInstallResult as StorePluginInstallResult; use super::store::PluginStore; use super::store::PluginStoreError; +use super::store::plugin_version_for_source; use super::sync_openai_plugins_repo; use crate::SkillMetadata; use crate::config::CONFIG_TOML_FILE; @@ -99,6 +100,13 @@ struct CachedFeaturedPluginIds { featured_plugin_ids: Vec, } +#[derive(Default)] +struct NonCuratedCacheRefreshState { + requested_roots: Option>, + last_refreshed_roots: Option>, + in_flight: bool, +} + fn featured_plugin_ids_cache_key( config: &Config, auth: Option<&CodexAuth>, @@ -312,6 +320,7 @@ pub struct PluginsManager { codex_home: PathBuf, store: PluginStore, featured_plugin_ids_cache: RwLock>, + non_curated_cache_refresh_state: RwLock, cached_enabled_outcome: RwLock>, remote_sync_lock: Mutex<()>, restriction_product: Option, @@ -338,6 +347,7 @@ impl PluginsManager { codex_home: codex_home.clone(), store: PluginStore::new(codex_home), featured_plugin_ids_cache: RwLock::new(None), + non_curated_cache_refresh_state: RwLock::new(NonCuratedCacheRefreshState::default()), cached_enabled_outcome: RwLock::new(None), remote_sync_lock: Mutex::new(()), restriction_product, @@ -1044,6 +1054,56 @@ impl PluginsManager { } } + pub fn maybe_start_non_curated_plugin_cache_refresh_for_roots( + self: &Arc, + roots: &[AbsolutePathBuf], + ) { + let mut roots = roots.to_vec(); + roots.sort_unstable_by(|left, right| left.as_path().cmp(right.as_path())); + roots.dedup(); + if roots.is_empty() { + return; + } + + let should_spawn = { + let mut state = match self.non_curated_cache_refresh_state.write() { + Ok(state) => state, + Err(err) => err.into_inner(), + }; + // Collapse repeated plugin/list requests onto one worker and only queue another pass + // when the requested roots set actually changes. + if state.requested_roots.as_ref() == Some(&roots) + || (!state.in_flight && state.last_refreshed_roots.as_ref() == Some(&roots)) + { + return; + } + state.requested_roots = Some(roots); + if state.in_flight { + false + } else { + state.in_flight = true; + true + } + }; + if !should_spawn { + return; + } + + let manager = Arc::clone(self); + if let Err(err) = std::thread::Builder::new() + .name("plugins-non-curated-cache-refresh".to_string()) + .spawn(move || manager.run_non_curated_plugin_cache_refresh_loop()) + { + let mut state = match self.non_curated_cache_refresh_state.write() { + Ok(state) => state, + Err(err) => err.into_inner(), + }; + state.in_flight = false; + state.requested_roots = None; + warn!("failed to start non-curated plugin cache refresh task: {err}"); + } + } + fn start_curated_repo_sync(self: &Arc) { if CURATED_REPO_SYNC_STARTED.swap(true, Ordering::SeqCst) { return; @@ -1055,8 +1115,13 @@ impl PluginsManager { .spawn( move || match sync_openai_plugins_repo(codex_home.as_path()) { Ok(curated_plugin_version) => { - let configured_curated_plugin_ids = - configured_curated_plugin_ids_from_codex_home(codex_home.as_path()); + let configured_curated_plugin_ids = curated_plugin_ids_from_config_keys( + configured_plugins_from_codex_home( + codex_home.as_path(), + "failed to read user config while refreshing curated plugin cache", + "failed to parse user config while refreshing curated plugin cache", + ), + ); match refresh_curated_plugin_cache( codex_home.as_path(), &curated_plugin_version, @@ -1086,6 +1151,55 @@ impl PluginsManager { } } + fn run_non_curated_plugin_cache_refresh_loop(self: Arc) { + loop { + let roots = { + let state = match self.non_curated_cache_refresh_state.read() { + Ok(state) => state, + Err(err) => err.into_inner(), + }; + state.requested_roots.clone() + }; + + let Some(roots) = roots else { + let mut state = match self.non_curated_cache_refresh_state.write() { + Ok(state) => state, + Err(err) => err.into_inner(), + }; + state.in_flight = false; + return; + }; + + let refreshed = + match refresh_non_curated_plugin_cache(self.codex_home.as_path(), &roots) { + Ok(cache_refreshed) => { + if cache_refreshed { + self.clear_cache(); + } + true + } + Err(err) => { + self.clear_cache(); + warn!("failed to refresh non-curated plugin cache: {err}"); + false + } + }; + + let mut state = match self.non_curated_cache_refresh_state.write() { + Ok(state) => state, + Err(err) => err.into_inner(), + }; + if refreshed { + state.last_refreshed_roots = Some(roots.clone()); + } + if state.requested_roots.as_ref() == Some(&roots) { + state.requested_roots = None; + state.in_flight = false; + return; + } + } + } + fn configured_plugin_states(&self, config: &Config) -> (HashSet, HashSet) { let configured_plugins = configured_plugins_from_stack(&config.config_layer_stack); let installed_plugins = configured_plugins @@ -1318,6 +1432,90 @@ fn refresh_curated_plugin_cache( Ok(cache_refreshed) } +fn refresh_non_curated_plugin_cache( + codex_home: &Path, + additional_roots: &[AbsolutePathBuf], +) -> Result { + let configured_non_curated_plugin_ids = + non_curated_plugin_ids_from_config_keys(configured_plugins_from_codex_home( + codex_home, + "failed to read user config while refreshing non-curated plugin cache", + "failed to parse user config while refreshing non-curated plugin cache", + )); + if configured_non_curated_plugin_ids.is_empty() { + return Ok(false); + } + let configured_non_curated_plugin_keys = configured_non_curated_plugin_ids + .iter() + .map(PluginId::as_key) + .collect::>(); + + let store = PluginStore::new(codex_home.to_path_buf()); + let marketplace_outcome = list_marketplaces(additional_roots) + .map_err(|err| format!("failed to discover marketplaces for cache refresh: {err}"))?; + let mut plugin_sources = HashMap::::new(); + + for marketplace in marketplace_outcome.marketplaces { + if marketplace.name == OPENAI_CURATED_MARKETPLACE_NAME { + continue; + } + + for plugin in marketplace.plugins { + let plugin_id = + PluginId::new(plugin.name.clone(), marketplace.name.clone()).map_err(|err| { + match err { + PluginIdError::Invalid(message) => { + format!("failed to prepare non-curated plugin cache refresh: {message}") + } + } + })?; + let plugin_key = plugin_id.as_key(); + if !configured_non_curated_plugin_keys.contains(&plugin_key) { + continue; + } + if plugin_sources.contains_key(&plugin_key) { + warn!( + plugin = plugin.name, + marketplace = marketplace.name, + "ignoring duplicate non-curated plugin entry during cache refresh" + ); + continue; + } + + let source_path = match plugin.source { + MarketplacePluginSource::Local { path } => path, + }; + let plugin_version = plugin_version_for_source(source_path.as_path()) + .map_err(|err| format!("failed to read plugin version for {plugin_key}: {err}"))?; + plugin_sources.insert(plugin_key, (source_path, plugin_version)); + } + } + + let mut cache_refreshed = false; + for plugin_id in configured_non_curated_plugin_ids { + let plugin_key = plugin_id.as_key(); + let Some((source_path, plugin_version)) = plugin_sources.get(&plugin_key).cloned() else { + warn!( + plugin = plugin_id.plugin_name, + marketplace = plugin_id.marketplace_name, + "configured non-curated plugin no longer exists in discovered marketplaces during cache refresh" + ); + continue; + }; + + if store.active_plugin_version(&plugin_id).as_deref() == Some(plugin_version.as_str()) { + continue; + } + + store + .install_with_version(source_path, plugin_id.clone(), plugin_version) + .map_err(|err| format!("failed to refresh plugin cache for {plugin_key}: {err}"))?; + cache_refreshed = true; + } + + Ok(cache_refreshed) +} + fn configured_plugins_from_stack( config_layer_stack: &ConfigLayerStack, ) -> HashMap { @@ -1343,42 +1541,22 @@ fn configured_plugins_from_user_config_value( } } -fn configured_curated_plugin_ids( - configured_plugins: HashMap, -) -> Vec { - let mut configured_curated_plugin_ids = configured_plugins - .into_keys() - .filter_map(|plugin_key| match PluginId::parse(&plugin_key) { - Ok(plugin_id) if plugin_id.marketplace_name == OPENAI_CURATED_MARKETPLACE_NAME => { - Some(plugin_id) - } - Ok(_) => None, - Err(err) => { - warn!( - plugin_key, - error = %err, - "ignoring invalid configured plugin key during curated sync setup" - ); - None - } - }) - .collect::>(); - configured_curated_plugin_ids.sort_unstable_by_key(PluginId::as_key); - configured_curated_plugin_ids -} - -fn configured_curated_plugin_ids_from_codex_home(codex_home: &Path) -> Vec { +fn configured_plugins_from_codex_home( + codex_home: &Path, + read_error_message: &str, + parse_error_message: &str, +) -> HashMap { let config_path = codex_home.join(CONFIG_TOML_FILE); let user_config = match fs::read_to_string(&config_path) { Ok(user_config) => user_config, - Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Vec::new(), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return HashMap::new(), Err(err) => { warn!( path = %config_path.display(), error = %err, - "failed to read user config while refreshing curated plugin cache" + "{read_error_message}" ); - return Vec::new(); + return HashMap::new(); } }; @@ -1388,13 +1566,61 @@ fn configured_curated_plugin_ids_from_codex_home(codex_home: &Path) -> Vec, + invalid_plugin_key_message: &str, +) -> Vec { + configured_plugins + .into_keys() + .filter_map(|plugin_key| match PluginId::parse(&plugin_key) { + Ok(plugin_id) => Some(plugin_id), + Err(err) => { + warn!( + plugin_key, + error = %err, + "{invalid_plugin_key_message}" + ); + None + } + }) + .collect() +} + +fn curated_plugin_ids_from_config_keys( + configured_plugins: HashMap, +) -> Vec { + let mut configured_curated_plugin_ids = configured_plugin_ids( + configured_plugins, + "ignoring invalid configured plugin key during curated sync setup", + ) + .into_iter() + .filter(|plugin_id| plugin_id.marketplace_name == OPENAI_CURATED_MARKETPLACE_NAME) + .collect::>(); + configured_curated_plugin_ids.sort_unstable_by_key(PluginId::as_key); + configured_curated_plugin_ids +} + +fn non_curated_plugin_ids_from_config_keys( + configured_plugins: HashMap, +) -> Vec { + let mut configured_non_curated_plugin_ids = configured_plugin_ids( + configured_plugins, + "ignoring invalid plugin key during non-curated cache refresh setup", + ) + .into_iter() + .filter(|plugin_id| plugin_id.marketplace_name != OPENAI_CURATED_MARKETPLACE_NAME) + .collect::>(); + configured_non_curated_plugin_ids.sort_unstable_by_key(PluginId::as_key); + configured_non_curated_plugin_ids } fn load_plugin( diff --git a/codex-rs/core/src/plugins/manager_tests.rs b/codex-rs/core/src/plugins/manager_tests.rs index a9f455bbc..2f1d3ae9c 100644 --- a/codex-rs/core/src/plugins/manager_tests.rs +++ b/codex-rs/core/src/plugins/manager_tests.rs @@ -30,19 +30,36 @@ use wiremock::matchers::query_param; const MAX_CAPABILITY_SUMMARY_DESCRIPTION_LEN: usize = 1024; -fn write_plugin(root: &Path, dir_name: &str, manifest_name: &str) { +fn write_plugin_with_version( + root: &Path, + dir_name: &str, + manifest_name: &str, + manifest_version: Option<&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(); + let version = manifest_version + .map(|manifest_version| format!(r#","version":"{manifest_version}""#)) + .unwrap_or_default(); fs::write( plugin_root.join(".codex-plugin/plugin.json"), - format!(r#"{{"name":"{manifest_name}"}}"#), + format!(r#"{{"name":"{manifest_name}"{version}}}"#), ) .unwrap(); fs::write(plugin_root.join("skills/SKILL.md"), "skill").unwrap(); fs::write(plugin_root.join(".mcp.json"), r#"{"mcpServers":{}}"#).unwrap(); } +fn write_plugin(root: &Path, dir_name: &str, manifest_name: &str) { + write_plugin_with_version( + root, + dir_name, + manifest_name, + /*manifest_version*/ None, + ); +} + fn plugin_config_toml(enabled: bool, plugins_feature_enabled: bool) -> String { let mut root = toml::map::Map::new(); @@ -955,6 +972,60 @@ async fn install_plugin_updates_config_with_relative_path_and_plugin_key() { assert!(config.contains("enabled = true")); } +#[tokio::test] +async fn install_plugin_uses_manifest_version_for_non_curated_plugins() { + let tmp = tempfile::tempdir().unwrap(); + let repo_root = tmp.path().join("repo"); + fs::create_dir_all(repo_root.join(".git")).unwrap(); + fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap(); + write_plugin_with_version( + &repo_root, + "sample-plugin", + "sample-plugin", + Some("1.2.3-beta+7"), + ); + fs::write( + repo_root.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "debug", + "plugins": [ + { + "name": "sample-plugin", + "source": { + "source": "local", + "path": "./sample-plugin" + } + } + ] +}"#, + ) + .unwrap(); + + let result = PluginsManager::new(tmp.path().to_path_buf()) + .install_plugin(PluginInstallRequest { + plugin_name: "sample-plugin".to_string(), + marketplace_path: AbsolutePathBuf::try_from( + repo_root.join(".agents/plugins/marketplace.json"), + ) + .unwrap(), + }) + .await + .unwrap(); + + let installed_path = tmp + .path() + .join("plugins/cache/debug/sample-plugin/1.2.3-beta+7"); + assert_eq!( + result, + PluginInstallOutcome { + plugin_id: PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap(), + plugin_version: "1.2.3-beta+7".to_string(), + installed_path: AbsolutePathBuf::try_from(installed_path).unwrap(), + auth_policy: MarketplacePluginAuthPolicy::OnInstall, + } + ); +} + #[tokio::test] async fn uninstall_plugin_removes_cache_and_config_entry() { let tmp = tempfile::tempdir().unwrap(); @@ -2208,7 +2279,7 @@ fn refresh_curated_plugin_cache_reinstalls_missing_configured_plugin_with_curren } #[test] -fn configured_curated_plugin_ids_from_codex_home_reads_latest_user_config() { +fn curated_plugin_ids_from_config_keys_reads_latest_codex_home_user_config() { let tmp = tempfile::tempdir().unwrap(); write_file( &tmp.path().join(CONFIG_TOML_FILE), @@ -2224,10 +2295,14 @@ enabled = true ); assert_eq!( - configured_curated_plugin_ids_from_codex_home(tmp.path()) - .into_iter() - .map(|plugin_id| plugin_id.as_key()) - .collect::>(), + curated_plugin_ids_from_config_keys(configured_plugins_from_codex_home( + tmp.path(), + "failed to read user config while refreshing curated plugin cache", + "failed to parse user config while refreshing curated plugin cache", + )) + .into_iter() + .map(|plugin_id| plugin_id.as_key()) + .collect::>(), vec!["slack@openai-curated".to_string()] ); @@ -2239,7 +2314,11 @@ plugins = true ); assert_eq!( - configured_curated_plugin_ids_from_codex_home(tmp.path()), + curated_plugin_ids_from_config_keys(configured_plugins_from_codex_home( + tmp.path(), + "failed to read user config while refreshing curated plugin cache", + "failed to parse user config while refreshing curated plugin cache", + )), Vec::::new() ); } @@ -2266,6 +2345,212 @@ fn refresh_curated_plugin_cache_returns_false_when_configured_plugins_are_curren ); } +#[test] +fn refresh_non_curated_plugin_cache_replaces_existing_local_version_with_manifest_version() { + let tmp = tempfile::tempdir().unwrap(); + let repo_root = tmp.path().join("repo"); + fs::create_dir_all(repo_root.join(".git")).unwrap(); + fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap(); + write_plugin_with_version(&repo_root, "sample-plugin", "sample-plugin", Some("1.2.3")); + write_file( + &repo_root.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "debug", + "plugins": [ + { + "name": "sample-plugin", + "source": { + "source": "local", + "path": "./sample-plugin" + } + } + ] +}"#, + ); + write_plugin( + &tmp.path().join("plugins/cache/debug"), + "sample-plugin/local", + "sample-plugin", + ); + write_file( + &tmp.path().join(CONFIG_TOML_FILE), + r#"[features] +plugins = true + +[plugins."sample-plugin@debug"] +enabled = true +"#, + ); + + assert!( + refresh_non_curated_plugin_cache( + tmp.path(), + &[AbsolutePathBuf::try_from(repo_root).unwrap()], + ) + .expect("cache refresh should succeed") + ); + + assert!( + !tmp.path() + .join("plugins/cache/debug/sample-plugin/local") + .exists() + ); + assert!( + tmp.path() + .join("plugins/cache/debug/sample-plugin/1.2.3") + .is_dir() + ); +} + +#[test] +fn refresh_non_curated_plugin_cache_reinstalls_missing_configured_plugin_with_manifest_version() { + let tmp = tempfile::tempdir().unwrap(); + let repo_root = tmp.path().join("repo"); + fs::create_dir_all(repo_root.join(".git")).unwrap(); + fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap(); + write_plugin_with_version(&repo_root, "sample-plugin", "sample-plugin", Some("1.2.3")); + write_file( + &repo_root.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "debug", + "plugins": [ + { + "name": "sample-plugin", + "source": { + "source": "local", + "path": "./sample-plugin" + } + } + ] +}"#, + ); + write_file( + &tmp.path().join(CONFIG_TOML_FILE), + r#"[features] +plugins = true + +[plugins."sample-plugin@debug"] +enabled = true +"#, + ); + + assert!( + refresh_non_curated_plugin_cache( + tmp.path(), + &[AbsolutePathBuf::try_from(repo_root).unwrap()], + ) + .expect("cache refresh should reinstall missing configured plugin") + ); + + assert!( + tmp.path() + .join("plugins/cache/debug/sample-plugin/1.2.3") + .is_dir() + ); +} + +#[test] +fn refresh_non_curated_plugin_cache_returns_false_when_configured_plugins_are_current() { + let tmp = tempfile::tempdir().unwrap(); + let repo_root = tmp.path().join("repo"); + fs::create_dir_all(repo_root.join(".git")).unwrap(); + fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap(); + write_plugin_with_version(&repo_root, "sample-plugin", "sample-plugin", Some("1.2.3")); + write_file( + &repo_root.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "debug", + "plugins": [ + { + "name": "sample-plugin", + "source": { + "source": "local", + "path": "./sample-plugin" + } + } + ] +}"#, + ); + write_plugin_with_version( + &tmp.path().join("plugins/cache/debug"), + "sample-plugin/1.2.3", + "sample-plugin", + Some("1.2.3"), + ); + write_file( + &tmp.path().join(CONFIG_TOML_FILE), + r#"[features] +plugins = true + +[plugins."sample-plugin@debug"] +enabled = true +"#, + ); + + assert!( + !refresh_non_curated_plugin_cache( + tmp.path(), + &[AbsolutePathBuf::try_from(repo_root).unwrap()], + ) + .expect("cache refresh should be a no-op when configured plugins are current") + ); +} + +#[test] +fn refresh_non_curated_plugin_cache_ignores_invalid_unconfigured_plugin_versions() { + let tmp = tempfile::tempdir().unwrap(); + let repo_root = tmp.path().join("repo"); + fs::create_dir_all(repo_root.join(".git")).unwrap(); + fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap(); + write_plugin_with_version(&repo_root, "sample-plugin", "sample-plugin", Some("1.2.3")); + write_plugin_with_version(&repo_root, "broken-plugin", "broken-plugin", Some(" ")); + write_file( + &repo_root.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "debug", + "plugins": [ + { + "name": "sample-plugin", + "source": { + "source": "local", + "path": "./sample-plugin" + } + }, + { + "name": "broken-plugin", + "source": { + "source": "local", + "path": "./broken-plugin" + } + } + ] +}"#, + ); + write_file( + &tmp.path().join(CONFIG_TOML_FILE), + r#"[features] +plugins = true + +[plugins."sample-plugin@debug"] +enabled = true +"#, + ); + + assert!( + refresh_non_curated_plugin_cache( + tmp.path(), + &[AbsolutePathBuf::try_from(repo_root).unwrap()], + ) + .expect("cache refresh should ignore unrelated invalid plugin manifests") + ); + + assert!( + tmp.path() + .join("plugins/cache/debug/sample-plugin/1.2.3") + .is_dir() + ); +} + #[test] fn load_plugins_ignores_project_config_files() { let codex_home = TempDir::new().unwrap(); diff --git a/codex-rs/core/src/plugins/manifest.rs b/codex-rs/core/src/plugins/manifest.rs index 8fbacb456..f1253441b 100644 --- a/codex-rs/core/src/plugins/manifest.rs +++ b/codex-rs/core/src/plugins/manifest.rs @@ -14,6 +14,8 @@ struct RawPluginManifest { #[serde(default)] name: String, #[serde(default)] + version: Option, + #[serde(default)] description: Option, // Keep manifest paths as raw strings so we can validate the required `./...` syntax before // resolving them under the plugin root. @@ -30,6 +32,7 @@ struct RawPluginManifest { #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct PluginManifest { pub(crate) name: String, + pub(crate) version: Option, pub(crate) description: Option, pub(crate) paths: PluginManifestPaths, pub(crate) interface: Option, @@ -121,6 +124,7 @@ pub(crate) fn load_plugin_manifest(plugin_root: &Path) -> Option Ok(manifest) => { let RawPluginManifest { name: raw_name, + version, description, skills, mcp_servers, @@ -133,6 +137,10 @@ pub(crate) fn load_plugin_manifest(plugin_root: &Path) -> Option .filter(|_| raw_name.trim().is_empty()) .unwrap_or(&raw_name) .to_string(); + let version = version.and_then(|version| { + let version = version.trim(); + (!version.is_empty()).then(|| version.to_string()) + }); let interface = interface.and_then(|interface| { let RawPluginManifestInterface { display_name, @@ -204,6 +212,7 @@ pub(crate) fn load_plugin_manifest(plugin_root: &Path) -> Option }); Some(PluginManifest { name, + version, description, paths: PluginManifestPaths { skills: resolve_manifest_path(plugin_root, "skills", skills.as_deref()), @@ -381,13 +390,17 @@ mod tests { use std::path::Path; use tempfile::tempdir; - fn write_manifest(plugin_root: &Path, interface: &str) { + fn write_manifest(plugin_root: &Path, version: Option<&str>, interface: &str) { fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("create manifest dir"); + let version = version + .map(|version| format!(" \"version\": \"{version}\",\n")) + .unwrap_or_default(); fs::write( plugin_root.join(".codex-plugin/plugin.json"), format!( r#"{{ "name": "demo-plugin", +{version} "interface": {interface} }}"# ), @@ -405,6 +418,7 @@ mod tests { let plugin_root = tmp.path().join("demo-plugin"); write_manifest( &plugin_root, + /*version*/ None, r#"{ "displayName": "Demo Plugin", "defaultPrompt": " Summarize my inbox " @@ -427,6 +441,7 @@ mod tests { let too_long = "x".repeat(MAX_DEFAULT_PROMPT_LEN + 1); write_manifest( &plugin_root, + /*version*/ None, &format!( r#"{{ "displayName": "Demo Plugin", @@ -462,6 +477,7 @@ mod tests { let plugin_root = tmp.path().join("demo-plugin"); write_manifest( &plugin_root, + /*version*/ None, r#"{ "displayName": "Demo Plugin", "defaultPrompt": { "text": "Summarize my inbox" } @@ -473,4 +489,21 @@ mod tests { assert_eq!(interface.default_prompt, None); } + + #[test] + fn plugin_manifest_reads_trimmed_version() { + let tmp = tempdir().expect("tempdir"); + let plugin_root = tmp.path().join("demo-plugin"); + write_manifest( + &plugin_root, + Some(" 1.2.3-beta+7 "), + r#"{ + "displayName": "Demo Plugin" + }"#, + ); + + let manifest = load_manifest(&plugin_root); + + assert_eq!(manifest.version, Some("1.2.3-beta+7".to_string())); + } } diff --git a/codex-rs/core/src/plugins/store.rs b/codex-rs/core/src/plugins/store.rs index faa7fc081..4316f1800 100644 --- a/codex-rs/core/src/plugins/store.rs +++ b/codex-rs/core/src/plugins/store.rs @@ -1,8 +1,11 @@ use super::load_plugin_manifest; +use super::manifest::PluginManifest; use codex_plugin::PluginId; use codex_plugin::validate_plugin_segment; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_plugins::PLUGIN_MANIFEST_PATH; +use serde::Deserialize; +use serde_json::Value as JsonValue; use std::fs; use std::io; use std::path::Path; @@ -62,7 +65,7 @@ impl PluginStore { entry.file_type().ok().filter(std::fs::FileType::is_dir)?; entry.file_name().into_string().ok() }) - .filter(|version| validate_plugin_segment(version, "plugin version").is_ok()) + .filter(|version| validate_plugin_version_segment(version).is_ok()) .collect::>(); discovered_versions.sort_unstable(); if discovered_versions.is_empty() { @@ -91,7 +94,8 @@ impl PluginStore { source_path: AbsolutePathBuf, plugin_id: PluginId, ) -> Result { - self.install_with_version(source_path, plugin_id, DEFAULT_PLUGIN_VERSION.to_string()) + let plugin_version = plugin_version_for_source(source_path.as_path())?; + self.install_with_version(source_path, plugin_id, plugin_version) } pub fn install_with_version( @@ -114,8 +118,7 @@ impl PluginStore { plugin_id.plugin_name ))); } - validate_plugin_segment(&plugin_version, "plugin version") - .map_err(PluginStoreError::Invalid)?; + validate_plugin_version_segment(&plugin_version).map_err(PluginStoreError::Invalid)?; let installed_path = self.plugin_root(&plugin_id, &plugin_version); replace_plugin_root_atomically( source_path.as_path(), @@ -154,7 +157,33 @@ impl PluginStoreError { } } -fn plugin_name_for_source(source_path: &Path) -> Result { +pub(crate) fn plugin_version_for_source(source_path: &Path) -> Result { + let plugin_version = plugin_manifest_version_for_source(source_path)? + .unwrap_or_else(|| DEFAULT_PLUGIN_VERSION.to_string()); + validate_plugin_version_segment(&plugin_version).map_err(PluginStoreError::Invalid)?; + Ok(plugin_version) +} + +fn validate_plugin_version_segment(plugin_version: &str) -> Result<(), String> { + if plugin_version.is_empty() { + return Err("invalid plugin version: must not be empty".to_string()); + } + if matches!(plugin_version, "." | "..") { + return Err("invalid plugin version: path traversal is not allowed".to_string()); + } + if !plugin_version + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || matches!(ch, '-' | '_' | '.' | '+')) + { + return Err( + "invalid plugin version: only ASCII letters, digits, `.`, `+`, `_`, and `-` are allowed" + .to_string(), + ); + } + Ok(()) +} + +fn plugin_manifest_for_source(source_path: &Path) -> Result { let manifest_path = source_path.join(PLUGIN_MANIFEST_PATH); if !manifest_path.is_file() { return Err(PluginStoreError::Invalid(format!( @@ -163,12 +192,61 @@ fn plugin_name_for_source(source_path: &Path) -> Result, +} + +fn plugin_manifest_version_for_source( + source_path: &Path, +) -> Result, PluginStoreError> { + let manifest_path = source_path.join(PLUGIN_MANIFEST_PATH); + if !manifest_path.is_file() { + return Err(PluginStoreError::Invalid(format!( + "missing plugin manifest: {}", + manifest_path.display() + ))); + } + + let contents = fs::read_to_string(&manifest_path) + .map_err(|err| PluginStoreError::io("failed to read plugin manifest", err))?; + let manifest: RawPluginManifestVersion = serde_json::from_str(&contents).map_err(|err| { + PluginStoreError::Invalid(format!( + "failed to parse plugin manifest {}: {err}", + manifest_path.display() + )) })?; + let Some(version) = manifest.version else { + return Ok(None); + }; + let Some(version) = version.as_str() else { + return Err(PluginStoreError::Invalid(format!( + "invalid plugin version in manifest {}: expected string", + manifest_path.display() + ))); + }; + let version = version.trim(); + if version.is_empty() { + return Err(PluginStoreError::Invalid(format!( + "invalid plugin version in manifest {}: must not be blank", + manifest_path.display() + ))); + } + Ok(Some(version.to_string())) +} + +fn plugin_name_for_source(source_path: &Path) -> Result { + let manifest = plugin_manifest_for_source(source_path)?; let plugin_name = manifest.name; validate_plugin_segment(&plugin_name, "plugin name") diff --git a/codex-rs/core/src/plugins/store_tests.rs b/codex-rs/core/src/plugins/store_tests.rs index e75e71c56..cb23443e8 100644 --- a/codex-rs/core/src/plugins/store_tests.rs +++ b/codex-rs/core/src/plugins/store_tests.rs @@ -3,19 +3,36 @@ use codex_plugin::PluginId; use pretty_assertions::assert_eq; use tempfile::tempdir; -fn write_plugin(root: &Path, dir_name: &str, manifest_name: &str) { +fn write_plugin_with_version( + root: &Path, + dir_name: &str, + manifest_name: &str, + manifest_version: Option<&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(); + let version = manifest_version + .map(|manifest_version| format!(r#","version":"{manifest_version}""#)) + .unwrap_or_default(); fs::write( plugin_root.join(".codex-plugin/plugin.json"), - format!(r#"{{"name":"{manifest_name}"}}"#), + format!(r#"{{"name":"{manifest_name}"{version}}}"#), ) .unwrap(); fs::write(plugin_root.join("skills/SKILL.md"), "skill").unwrap(); fs::write(plugin_root.join(".mcp.json"), r#"{"mcpServers":{}}"#).unwrap(); } +fn write_plugin(root: &Path, dir_name: &str, manifest_name: &str) { + write_plugin_with_version( + root, + dir_name, + manifest_name, + /*manifest_version*/ None, + ); +} + #[test] fn install_copies_plugin_into_default_marketplace() { let tmp = tempdir().unwrap(); @@ -110,6 +127,62 @@ fn install_with_version_uses_requested_cache_version() { assert!(installed_path.join(".codex-plugin/plugin.json").is_file()); } +#[test] +fn install_uses_manifest_version_when_present() { + let tmp = tempdir().unwrap(); + write_plugin_with_version( + tmp.path(), + "sample-plugin", + "sample-plugin", + Some("1.2.3-beta+7"), + ); + let plugin_id = PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap(); + + let result = PluginStore::new(tmp.path().to_path_buf()) + .install( + AbsolutePathBuf::try_from(tmp.path().join("sample-plugin")).unwrap(), + plugin_id.clone(), + ) + .unwrap(); + + let installed_path = tmp + .path() + .join("plugins/cache/debug/sample-plugin/1.2.3-beta+7"); + assert_eq!( + result, + PluginInstallResult { + plugin_id, + plugin_version: "1.2.3-beta+7".to_string(), + installed_path: AbsolutePathBuf::try_from(installed_path.clone()).unwrap(), + } + ); + assert!(installed_path.join(".codex-plugin/plugin.json").is_file()); +} + +#[test] +fn install_rejects_blank_manifest_version() { + let tmp = tempdir().unwrap(); + write_plugin_with_version(tmp.path(), "sample-plugin", "sample-plugin", Some(" ")); + let plugin_id = PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap(); + + let err = PluginStore::new(tmp.path().to_path_buf()) + .install( + AbsolutePathBuf::try_from(tmp.path().join("sample-plugin")).unwrap(), + plugin_id, + ) + .expect_err("blank manifest version should be rejected"); + let err = err.to_string().replace('\\', "/"); + + assert!( + err.starts_with("invalid plugin version in manifest "), + "unexpected error: {err}" + ); + assert!( + err.ends_with("sample-plugin/.codex-plugin/plugin.json: must not be blank"), + "unexpected error: {err}" + ); +} + #[test] fn active_plugin_version_reads_version_directory_name() { let tmp = tempdir().unwrap();