From b611fd31013e76937af32b51ee5feeba63397ad0 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Wed, 3 Jun 2026 00:23:43 -0700 Subject: [PATCH] refactor plugin data store seam --- codex-rs/core-plugins/src/data_store.rs | 43 +++++++++++ codex-rs/core-plugins/src/data_store_tests.rs | 16 ++++ codex-rs/core-plugins/src/lib.rs | 3 + codex-rs/core-plugins/src/loader.rs | 6 +- codex-rs/core-plugins/src/manager.rs | 33 +++++++- codex-rs/core-plugins/src/manager_tests.rs | 75 +++++++++++++++++++ codex-rs/core-plugins/src/store.rs | 15 +--- codex-rs/core-plugins/src/store_tests.rs | 12 --- 8 files changed, 175 insertions(+), 28 deletions(-) create mode 100644 codex-rs/core-plugins/src/data_store.rs create mode 100644 codex-rs/core-plugins/src/data_store_tests.rs diff --git a/codex-rs/core-plugins/src/data_store.rs b/codex-rs/core-plugins/src/data_store.rs new file mode 100644 index 0000000000..cade963efb --- /dev/null +++ b/codex-rs/core-plugins/src/data_store.rs @@ -0,0 +1,43 @@ +use crate::store::PluginStoreError; +use codex_plugin::PluginId; +use codex_utils_absolute_path::AbsolutePathBuf; +use std::path::PathBuf; + +const PLUGINS_DATA_DIR: &str = "plugins/data"; + +/// Resolves executor-readable writable roots for mutable plugin data. +pub trait PluginDataStore: Send + Sync + 'static { + /// Returns the writable local root exposed to hooks for one plugin. + fn plugin_data_root(&self, plugin_id: &PluginId) -> AbsolutePathBuf; +} + +/// Stores mutable plugin data under the local Codex Home layout. +#[derive(Debug, Clone)] +pub struct LocalPluginDataStore { + root: AbsolutePathBuf, +} + +impl LocalPluginDataStore { + /// Creates the local mutable plugin data store for one Codex Home. + pub fn from_codex_home(codex_home: PathBuf) -> Result { + let root = AbsolutePathBuf::from_absolute_path_checked(codex_home.join(PLUGINS_DATA_DIR)) + .map_err(|source| PluginStoreError::Io { + context: "failed to resolve plugin data root", + source, + })?; + Ok(Self { root }) + } +} + +impl PluginDataStore for LocalPluginDataStore { + fn plugin_data_root(&self, plugin_id: &PluginId) -> AbsolutePathBuf { + self.root.join(format!( + "{}-{}", + plugin_id.plugin_name, plugin_id.marketplace_name + )) + } +} + +#[cfg(test)] +#[path = "data_store_tests.rs"] +mod tests; diff --git a/codex-rs/core-plugins/src/data_store_tests.rs b/codex-rs/core-plugins/src/data_store_tests.rs new file mode 100644 index 0000000000..e8e60759c1 --- /dev/null +++ b/codex-rs/core-plugins/src/data_store_tests.rs @@ -0,0 +1,16 @@ +use super::*; +use codex_plugin::PluginId; +use pretty_assertions::assert_eq; +use tempfile::tempdir; + +#[test] +fn local_plugin_data_root_derives_path_from_key() { + let tmp = tempdir().unwrap(); + let store = LocalPluginDataStore::from_codex_home(tmp.path().to_path_buf()).unwrap(); + let plugin_id = PluginId::new("sample".to_string(), "debug".to_string()).unwrap(); + + assert_eq!( + store.plugin_data_root(&plugin_id).as_path(), + tmp.path().join("plugins/data/sample-debug") + ); +} diff --git a/codex-rs/core-plugins/src/lib.rs b/codex-rs/core-plugins/src/lib.rs index 4728a8c6f8..c27619b269 100644 --- a/codex-rs/core-plugins/src/lib.rs +++ b/codex-rs/core-plugins/src/lib.rs @@ -1,3 +1,4 @@ +pub mod data_store; mod discoverable; pub mod installed_marketplaces; pub mod loader; @@ -24,6 +25,8 @@ pub const OPENAI_BUNDLED_MARKETPLACE_NAME: &str = "openai-bundled"; pub type LoadedPlugin = codex_plugin::LoadedPlugin; pub type PluginLoadOutcome = codex_plugin::PluginLoadOutcome; +pub use data_store::LocalPluginDataStore; +pub use data_store::PluginDataStore; pub use discoverable::ToolSuggestDiscoverablePlugin; pub use discoverable::ToolSuggestPluginDiscoveryInput; pub use manager::ConfiguredMarketplace; diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index d51771085d..46a155ca99 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -1,4 +1,5 @@ use crate::OPENAI_CURATED_MARKETPLACE_NAME; +use crate::data_store::PluginDataStore; use crate::manifest::PluginManifestHooks; use crate::manifest::PluginManifestPaths; use crate::manifest::load_plugin_manifest; @@ -113,6 +114,7 @@ pub async fn load_plugins_from_layer_stack( config_layer_stack: &ConfigLayerStack, extra_plugins: HashMap, store: &PluginStore, + data_store: &dyn PluginDataStore, restriction_product: Option, prefer_remote_curated_conflicts: bool, ) -> PluginLoadOutcome { @@ -133,6 +135,7 @@ pub async fn load_plugins_from_layer_stack( configured_name.clone(), &plugin, store, + data_store, restriction_product, &skill_config_rules, ) @@ -557,6 +560,7 @@ async fn load_plugin( config_name: String, plugin: &PluginConfig, store: &PluginStore, + data_store: &dyn PluginDataStore, restriction_product: Option, skill_config_rules: &SkillConfigRules, ) -> LoadedPlugin { @@ -659,7 +663,7 @@ async fn load_plugin( let (hook_sources, hook_load_warnings) = load_plugin_hooks( &plugin_root, &loaded_plugin_id, - &store.plugin_data_root(&loaded_plugin_id), + &data_store.plugin_data_root(&loaded_plugin_id), manifest_paths, ); loaded_plugin.hook_sources = hook_sources; diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index 8022c6c86d..5c9af9adff 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -1,6 +1,8 @@ use super::PluginLoadOutcome; use super::startup_remote_sync::start_startup_remote_plugin_sync_once; use crate::OPENAI_CURATED_MARKETPLACE_NAME; +use crate::data_store::LocalPluginDataStore; +use crate::data_store::PluginDataStore; use crate::installed_marketplaces::installed_marketplace_roots_from_layer_stack; use crate::loader::configured_curated_plugin_ids_from_codex_home; use crate::loader::curated_plugin_cache_version; @@ -398,6 +400,7 @@ impl From for PluginRemoteSyncError { pub struct PluginsManager { codex_home: PathBuf, store: PluginStore, + data_store: Arc, featured_plugin_ids_cache: RwLock>, configured_marketplace_upgrade_state: RwLock, non_curated_cache_refresh_state: RwLock, @@ -420,9 +423,34 @@ impl PluginsManager { Self::new_with_restriction_product(codex_home, Some(Product::Codex)) } + /// Creates a plugins manager with an alternative mutable plugin data backend. + pub fn new_with_data_store(codex_home: PathBuf, data_store: Arc) -> Self { + Self::new_with_restriction_product_and_data_store( + codex_home, + Some(Product::Codex), + data_store, + ) + } + pub fn new_with_restriction_product( codex_home: PathBuf, restriction_product: Option, + ) -> Self { + let data_store = Arc::new( + LocalPluginDataStore::from_codex_home(codex_home.clone()) + .unwrap_or_else(|err| panic!("plugin data root should be absolute: {err}")), + ); + Self::new_with_restriction_product_and_data_store( + codex_home, + restriction_product, + data_store, + ) + } + + fn new_with_restriction_product_and_data_store( + codex_home: PathBuf, + restriction_product: Option, + data_store: Arc, ) -> Self { // Product restrictions are enforced at marketplace admission time for a given CODEX_HOME: // listing, install, and curated refresh all consult this restriction context before new @@ -434,6 +462,7 @@ impl PluginsManager { Self { codex_home: codex_home.clone(), store: PluginStore::new(codex_home), + data_store, featured_plugin_ids_cache: RwLock::new(None), configured_marketplace_upgrade_state: RwLock::new( ConfiguredMarketplaceUpgradeState::default(), @@ -491,6 +520,7 @@ impl PluginsManager { &config.config_layer_stack, self.remote_installed_plugin_configs(), &self.store, + self.data_store.as_ref(), self.restriction_product, config.remote_plugin_enabled, ) @@ -537,6 +567,7 @@ impl PluginsManager { config_layer_stack, self.remote_installed_plugin_configs(), &self.store, + self.data_store.as_ref(), self.restriction_product, config.remote_plugin_enabled, ) @@ -1456,7 +1487,7 @@ impl PluginsManager { ), ) .await; - let plugin_data_root = self.store.plugin_data_root(&plugin_id); + let plugin_data_root = self.data_store.plugin_data_root(&plugin_id); let (hook_sources, _hook_load_warnings) = load_plugin_hooks(&source_path, &plugin_id, &plugin_data_root, &manifest.paths); let hooks = plugin_hook_declarations(&hook_sources) diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 905d120a8e..6ce6652dc8 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -1,6 +1,8 @@ use super::*; use crate::LoadedPlugin; use crate::PluginLoadOutcome; +use crate::data_store::LocalPluginDataStore; +use crate::data_store::PluginDataStore; use crate::installed_marketplaces::marketplace_install_root; use crate::loader::load_plugins_from_layer_stack; use crate::loader::refresh_non_curated_plugin_cache; @@ -29,10 +31,12 @@ use codex_config::types::McpServerTransportConfig; use codex_login::CodexAuth; 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; use std::path::Path; +use std::sync::Arc; use tempfile::TempDir; use toml::Value; use wiremock::Mock; @@ -45,6 +49,17 @@ use wiremock::matchers::query_param; const MAX_CAPABILITY_SUMMARY_DESCRIPTION_LEN: usize = 1024; +#[derive(Debug)] +struct FakePluginDataStore { + root: AbsolutePathBuf, +} + +impl PluginDataStore for FakePluginDataStore { + fn plugin_data_root(&self, _plugin_id: &PluginId) -> AbsolutePathBuf { + self.root.clone() + } +} + fn write_plugin_with_version( root: &Path, dir_name: &str, @@ -3851,6 +3866,7 @@ async fn load_plugins_ignores_project_config_files() { &stack, std::collections::HashMap::new(), &PluginStore::new(codex_home.path().to_path_buf()), + &LocalPluginDataStore::from_codex_home(codex_home.path().to_path_buf()).unwrap(), Some(Product::Codex), /*prefer_remote_curated_conflicts*/ false, ) @@ -3858,3 +3874,62 @@ async fn load_plugins_ignores_project_config_files() { assert_eq!(outcome, PluginLoadOutcome::default()); } + +#[tokio::test] +async fn plugins_for_layer_stack_uses_injected_plugin_data_store() { + let codex_home = TempDir::new().unwrap(); + let plugin_root = codex_home + .path() + .join("plugins/cache") + .join("test/sample/local"); + let config_path = AbsolutePathBuf::try_from(codex_home.path().join(CONFIG_TOML_FILE)).unwrap(); + let data_root = + AbsolutePathBuf::try_from(codex_home.path().join("injected-plugin-data")).unwrap(); + + write_file( + &plugin_root.join(".codex-plugin/plugin.json"), + r#"{"name":"sample"}"#, + ); + write_file( + &plugin_root.join("hooks/hooks.json"), + r#"{"hooks":{"SessionStart":[{"hooks":[{"type":"command","command":"echo startup"}]}]}}"#, + ); + let stack = ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: config_path, + profile: None, + }, + toml::from_str(&plugin_config_toml( + /*enabled*/ true, /*plugins_feature_enabled*/ true, + )) + .expect("user config should parse"), + )], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("config layer stack should build"); + let config = PluginsConfigInput::new( + stack.clone(), + /*plugins_enabled*/ true, + /*remote_plugin_enabled*/ false, + "https://chatgpt.com/backend-api/".to_string(), + ); + let outcome = PluginsManager::new_with_data_store( + codex_home.path().to_path_buf(), + Arc::new(FakePluginDataStore { + root: data_root.clone(), + }), + ) + .plugins_for_layer_stack(&stack, &config) + .await; + + assert_eq!( + outcome + .effective_plugin_hook_sources() + .into_iter() + .map(|source| source.plugin_data_root) + .collect::>(), + vec![data_root] + ); +} diff --git a/codex-rs/core-plugins/src/store.rs b/codex-rs/core-plugins/src/store.rs index 19c3b0116f..a6690bb158 100644 --- a/codex-rs/core-plugins/src/store.rs +++ b/codex-rs/core-plugins/src/store.rs @@ -15,7 +15,6 @@ use std::path::PathBuf; pub const DEFAULT_PLUGIN_VERSION: &str = "local"; pub const PLUGINS_CACHE_DIR: &str = "plugins/cache"; -pub const PLUGINS_DATA_DIR: &str = "plugins/data"; #[derive(Debug, Clone, PartialEq, Eq)] pub struct PluginInstallResult { @@ -27,7 +26,6 @@ pub struct PluginInstallResult { #[derive(Debug, Clone)] pub struct PluginStore { root: AbsolutePathBuf, - data_root: AbsolutePathBuf, } impl PluginStore { @@ -39,11 +37,7 @@ impl PluginStore { pub fn try_new(codex_home: PathBuf) -> Result { let root = AbsolutePathBuf::from_absolute_path_checked(codex_home.join(PLUGINS_CACHE_DIR)) .map_err(|err| PluginStoreError::io("failed to resolve plugin cache root", err))?; - let data_root = - AbsolutePathBuf::from_absolute_path_checked(codex_home.join(PLUGINS_DATA_DIR)) - .map_err(|err| PluginStoreError::io("failed to resolve plugin data root", err))?; - - Ok(Self { root, data_root }) + Ok(Self { root }) } pub fn root(&self) -> &AbsolutePathBuf { @@ -60,13 +54,6 @@ impl PluginStore { self.plugin_base_root(plugin_id).join(plugin_version) } - pub fn plugin_data_root(&self, plugin_id: &PluginId) -> AbsolutePathBuf { - self.data_root.join(format!( - "{}-{}", - plugin_id.plugin_name, plugin_id.marketplace_name - )) - } - pub fn active_plugin_version(&self, plugin_id: &PluginId) -> Option { let mut discovered_versions = fs::read_dir(self.plugin_base_root(plugin_id).as_path()) .ok()? diff --git a/codex-rs/core-plugins/src/store_tests.rs b/codex-rs/core-plugins/src/store_tests.rs index 200055fe6c..6c64130a1b 100644 --- a/codex-rs/core-plugins/src/store_tests.rs +++ b/codex-rs/core-plugins/src/store_tests.rs @@ -109,18 +109,6 @@ fn plugin_root_derives_path_from_key_and_version() { ); } -#[test] -fn plugin_data_root_derives_path_from_key() { - 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_data_root(&plugin_id).as_path(), - tmp.path().join("plugins/data/sample-debug") - ); -} - #[test] fn install_with_version_uses_requested_cache_version() { let tmp = tempdir().unwrap();