openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-21 17:48:44 +00:00
Code Issues Packages Projects Releases Wiki Activity

Simplify plugin skill root snapshot cache

Browse Source
This commit is contained in:
Matthew Zeng
2026-06-18 00:18:52 -07:00
parent 132972507a
commit 31491e461b
10 changed files with 80 additions and 156 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core-plugins/src/loader.rs
+8 -8
View File
@@ -19,8 +19,8 @@ use codex_config::HooksFile;
use codex_config::types::McpServerConfig;
use codex_config::types::PluginConfig;
use codex_config::types::PluginMcpServerConfig;
use codex_core_skills::PluginSkillRootCache;
use codex_core_skills::SkillMetadata;
use codex_core_skills::SkillRootLoader;
use codex_core_skills::config_rules::SkillConfigRules;
use codex_core_skills::config_rules::resolve_disabled_skill_paths;
use codex_core_skills::config_rules::skill_config_rules_from_stack;
@@ -72,7 +72,7 @@ enum PluginLoadScope<'a> {
AllCapabilities {
restriction_product: Option<Product>,
skill_config_rules: &'a SkillConfigRules,
skill_root_loader: &'a SkillRootLoader,
plugin_skill_root_cache: &'a PluginSkillRootCache,
},
HooksOnly,
}
@@ -114,7 +114,7 @@ pub(crate) async fn load_plugins_from_layer_stack(
config_layer_stack: &ConfigLayerStack,
extra_plugins: HashMap<String, PluginConfig>,
store: &PluginStore,
skill_root_loader: &SkillRootLoader,
plugin_skill_root_cache: &PluginSkillRootCache,
restriction_product: Option<Product>,
prefer_remote_curated_conflicts: bool,
) -> Vec<LoadedPlugin<McpServerConfig>> {
@@ -127,7 +127,7 @@ pub(crate) async fn load_plugins_from_layer_stack(
PluginLoadScope::AllCapabilities {
restriction_product,
skill_config_rules: &skill_config_rules,
skill_root_loader,
plugin_skill_root_cache,
},
)
.await
@@ -716,7 +716,7 @@ async fn load_plugin(
PluginLoadScope::AllCapabilities {
restriction_product,
skill_config_rules,
skill_root_loader,
plugin_skill_root_cache,
} => {
loaded_plugin.manifest_name = Some(manifest.display_name().to_string());
loaded_plugin.manifest_description = manifest.description.clone();
@@ -727,7 +727,7 @@ async fn load_plugin(
&manifest,
*restriction_product,
skill_config_rules,
skill_root_loader,
plugin_skill_root_cache,
)
.await;
let has_enabled_skills = resolved_skills.has_enabled_skills();
@@ -796,7 +796,7 @@ pub async fn load_plugin_skills(
manifest: &PluginManifest,
restriction_product: Option<Product>,
skill_config_rules: &SkillConfigRules,
skill_root_loader: &SkillRootLoader,
plugin_skill_root_cache: &PluginSkillRootCache,
) -> ResolvedPluginSkills {
let roots = plugin_skill_roots(plugin_root, &manifest.paths)
.into_iter()
@@ -809,7 +809,7 @@ pub async fn load_plugin_skills(
plugin_root: Some(plugin_root.clone()),
})
.collect::<Vec<_>>();
let outcome = skill_root_loader.load_skills_from_roots(roots).await;
let outcome = plugin_skill_root_cache.load_skills_from_roots(roots).await;
let had_errors = !outcome.errors.is_empty();
let skills = outcome
.skills
codex-rs/core-plugins/src/loader_tests.rs
+3 -3
View File
@@ -5,7 +5,7 @@ use codex_config::ConfigLayerEntry;
use codex_config::ConfigLayerSource;
use codex_config::ConfigRequirements;
use codex_config::ConfigRequirementsToml;
use codex_core_skills::SkillRootLoader;
use codex_core_skills::PluginSkillRootCache;
use codex_plugin::PluginId;
use pretty_assertions::assert_eq;
use tempfile::TempDir;
@@ -156,13 +156,13 @@ enabled = true
)
.expect("valid config layer stack");
let store = PluginStore::new(temp_dir.path().to_path_buf());
let skill_root_loader = SkillRootLoader::default();
let plugin_skill_root_cache = PluginSkillRootCache::default();
let full = load_plugins_from_layer_stack(
&stack,
HashMap::new(),
&store,
&skill_root_loader,
&plugin_skill_root_cache,
Some(Product::Codex),
/*prefer_remote_curated_conflicts*/ false,
)
codex-rs/core-plugins/src/manager.rs
+10 -10
View File
@@ -59,8 +59,8 @@ use codex_config::set_user_plugin_enabled;
use codex_config::types::PluginConfig;
use codex_config::types::ToolSuggestDisabledTool;
use codex_config::types::ToolSuggestDiscoverableType;
use codex_core_skills::PluginSkillRootCache;
use codex_core_skills::SkillMetadata;
use codex_core_skills::SkillRootLoader;
use codex_core_skills::config_rules::SkillConfigRules;
use codex_core_skills::config_rules::skill_config_rules_from_stack;
use codex_hooks::plugin_hook_declarations;
@@ -355,7 +355,7 @@ pub struct PluginsManager {
// Keep the cache auth-independent so auth changes only need to resolve capabilities again.
loaded_plugins_cache: RwLock<LoadedPluginsCache>,
loaded_plugins_load_semaphore: Semaphore,
skill_root_loader: Arc<SkillRootLoader>,
plugin_skill_root_cache: Arc<PluginSkillRootCache>,
remote_installed_plugins_cache: RwLock<Option<Vec<RemoteInstalledPlugin>>>,
remote_installed_plugins_cache_refresh_state: RwLock<RemoteInstalledPluginsCacheRefreshState>,
global_remote_catalog_cache_refresh_state: RwLock<GlobalRemoteCatalogCacheRefreshState>,
@@ -412,7 +412,7 @@ impl PluginsManager {
non_curated_cache_refresh_state: RwLock::new(NonCuratedCacheRefreshState::default()),
loaded_plugins_cache: RwLock::new(LoadedPluginsCache::default()),
loaded_plugins_load_semaphore: Semaphore::new(/*permits*/ 1),
skill_root_loader: Arc::new(SkillRootLoader::default()),
plugin_skill_root_cache: Arc::new(PluginSkillRootCache::default()),
remote_installed_plugins_cache: RwLock::new(None),
remote_installed_plugins_cache_refresh_state: RwLock::new(
RemoteInstalledPluginsCacheRefreshState::default(),
@@ -438,9 +438,9 @@ impl PluginsManager {
true
}
/// Returns the loader whose plugin-root snapshots are populated while loading plugins.
pub fn skill_root_loader(&self) -> Arc<SkillRootLoader> {
Arc::clone(&self.skill_root_loader)
/// Returns the shared cache populated while loading plugin skill roots.
pub fn plugin_skill_root_cache(&self) -> Arc<PluginSkillRootCache> {
Arc::clone(&self.plugin_skill_root_cache)
}
pub fn auth_mode(&self) -> Option<AuthMode> {
@@ -513,7 +513,7 @@ impl PluginsManager {
&config.config_layer_stack,
self.remote_installed_plugin_configs(),
&self.store,
self.skill_root_loader.as_ref(),
self.plugin_skill_root_cache.as_ref(),
self.restriction_product,
config.remote_plugin_enabled,
)
@@ -560,7 +560,7 @@ impl PluginsManager {
}
fn clear_loaded_plugins_cache(&self) {
self.skill_root_loader.clear_cache();
self.plugin_skill_root_cache.clear_cache();
let mut cache = match self.loaded_plugins_cache.write() {
Ok(cache) => cache,
Err(err) => err.into_inner(),
@@ -582,7 +582,7 @@ impl PluginsManager {
config_layer_stack,
self.remote_installed_plugin_configs(),
&self.store,
self.skill_root_loader.as_ref(),
self.plugin_skill_root_cache.as_ref(),
self.restriction_product,
config.remote_plugin_enabled,
)
@@ -1607,7 +1607,7 @@ impl PluginsManager {
&codex_core_skills::config_rules::skill_config_rules_from_stack(
&config.config_layer_stack,
),
self.skill_root_loader.as_ref(),
self.plugin_skill_root_cache.as_ref(),
)
.await;
let plugin_data_root = self.store.plugin_data_root(&plugin_id);
codex-rs/core-plugins/src/manager_tests.rs
+5 -5
View File
@@ -34,7 +34,7 @@ use codex_config::McpServerConfig;
use codex_config::McpServerOAuthConfig;
use codex_config::McpServerToolConfig;
use codex_config::types::McpServerTransportConfig;
use codex_core_skills::SkillRootLoader;
use codex_core_skills::PluginSkillRootCache;
use codex_core_skills::config_rules::SkillConfigRules;
use codex_login::CodexAuth;
use codex_plugin::AppDeclaration;
@@ -1467,7 +1467,7 @@ async fn load_plugin_skills_dedupes_overlapping_manifest_roots() {
interface: None,
};
let plugin_id = PluginId::parse("sample@test").expect("plugin id should parse");
let skill_root_loader = SkillRootLoader::default();
let plugin_skill_root_cache = PluginSkillRootCache::default();
let resolved = load_plugin_skills(
&plugin_root,
@@ -1475,7 +1475,7 @@ async fn load_plugin_skills_dedupes_overlapping_manifest_roots() {
&manifest,
/*restriction_product*/ None,
&SkillConfigRules::default(),
&skill_root_loader,
&plugin_skill_root_cache,
)
.await;
@@ -4823,12 +4823,12 @@ async fn load_plugins_ignores_project_config_files() {
)
.expect("config layer stack should build");
let skill_root_loader = SkillRootLoader::default();
let plugin_skill_root_cache = PluginSkillRootCache::default();
let plugins = load_plugins_from_layer_stack(
&stack,
std::collections::HashMap::new(),
&PluginStore::new(codex_home.path().to_path_buf()),
&skill_root_loader,
&plugin_skill_root_cache,
Some(Product::Codex),
/*prefer_remote_curated_conflicts*/ false,
)
codex-rs/core-skills/src/lib.rs
+2 -2
View File
@@ -4,9 +4,9 @@ pub(crate) mod invocation_utils;
pub mod loader;
mod mention_counts;
pub mod model;
mod plugin_skill_root_cache;
pub mod remote;
pub mod render;
mod root_loader;
pub mod service;
mod skill_instructions;
pub mod system;
@@ -20,6 +20,7 @@ pub use model::SkillLoadOutcome;
pub use model::SkillMetadata;
pub use model::SkillPolicy;
pub use model::filter_skill_load_outcome_for_product;
pub use plugin_skill_root_cache::PluginSkillRootCache;
pub use render::AvailableSkills;
pub use render::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS;
pub use render::SKILLS_HOW_TO_USE_WITH_ALIASES;
@@ -30,7 +31,6 @@ pub use render::SkillRenderReport;
pub use render::build_available_skills;
pub use render::default_skill_metadata_budget;
pub use render::render_available_skills_body;
pub use root_loader::SkillRootLoader;
pub use service::SkillsLoadInput;
pub use service::SkillsService;
pub use skill_instructions::SkillInstructions;
codex-rs/core-skills/src/loader.rs
+2 -2
View File
@@ -5,7 +5,7 @@ use crate::model::SkillLoadOutcome;
use crate::model::SkillMetadata;
use crate::model::SkillPolicy;
use crate::model::SkillToolDependency;
use crate::root_loader::SkillRootLoader;
use crate::plugin_skill_root_cache::PluginSkillRootCache;
use crate::system::system_cache_root_dir;
use codex_app_server_protocol::ConfigLayerSource;
use codex_config::ConfigLayerStack;
@@ -164,7 +164,7 @@ pub async fn load_skills_from_roots<I>(roots: I) -> SkillLoadOutcome
where
I: IntoIterator<Item = SkillRoot>,
{
SkillRootLoader::default()
PluginSkillRootCache::default()
.load_skills_from_roots(roots)
.await
}
codex-rs/core-skills/src/root_loader.rs → codex-rs/core-skills/src/plugin_skill_root_cache.rs
+28 -88
View File
@@ -1,11 +1,8 @@
use std::collections::HashMap;
use std::collections::HashSet;
use std::hash::Hash;
use std::hash::Hasher;
use std::sync::Arc;
use std::sync::RwLock;
use codex_exec_server::ExecutorFileSystem;
use codex_utils_absolute_path::AbsolutePathBuf;
use crate::SkillLoadOutcome;
@@ -16,62 +13,16 @@ use crate::model::SkillFileSystemsByPath;
const MAX_CACHED_PLUGIN_SKILL_ROOTS: usize = 256;
#[derive(Clone)]
struct FileSystemIdentity(Arc<dyn ExecutorFileSystem>);
impl PartialEq for FileSystemIdentity {
fn eq(&self, other: &Self) -> bool {
Arc::ptr_eq(&self.0, &other.0)
}
}
impl Eq for FileSystemIdentity {}
impl Hash for FileSystemIdentity {
fn hash<H: Hasher>(&self, state: &mut H) {
(Arc::as_ptr(&self.0) as *const ()).hash(state);
}
}
#[derive(Clone, PartialEq, Eq, Hash)]
struct SkillRootCacheKey {
path: AbsolutePathBuf,
scope_rank: u8,
file_system: FileSystemIdentity,
plugin_id: String,
plugin_namespace: String,
plugin_root: AbsolutePathBuf,
}
impl SkillRootCacheKey {
fn from_root(root: &SkillRoot) -> Option<Self> {
Some(Self {
path: root.path.clone(),
scope_rank: scope_rank(root.scope),
file_system: FileSystemIdentity(Arc::clone(&root.file_system)),
plugin_id: root.plugin_id.clone()?,
plugin_namespace: root.plugin_namespace.clone()?,
plugin_root: root.plugin_root.clone()?,
})
}
}
/// Loads skill roots and reuses parsed plugin-root snapshots until explicitly invalidated.
/// Shares parsed plugin skill-root snapshots between plugin and skill loading.
///
/// Non-plugin roots are always loaded directly because their filesystem lifecycle is owned by the
/// environment or config layer that supplied them.
#[derive(Default)]
struct PluginRootCache {
generation: u64,
snapshots: HashMap<SkillRootCacheKey, SkillRootSnapshot>,
pub struct PluginSkillRootCache {
snapshots: RwLock<HashMap<AbsolutePathBuf, SkillRootSnapshot>>,
}
#[derive(Default)]
pub struct SkillRootLoader {
plugin_root_cache: RwLock<PluginRootCache>,
}
impl SkillRootLoader {
impl PluginSkillRootCache {
/// Loads and merges roots, reusing snapshots for roots owned by a plugin.
pub async fn load_skills_from_roots<I>(&self, roots: I) -> SkillLoadOutcome
where
@@ -79,16 +30,28 @@ impl SkillRootLoader {
{
let mut snapshots = Vec::new();
for root in roots {
let cache_key = SkillRootCacheKey::from_root(&root);
let (cache_generation, cached_snapshot) = cache_key
// Plugin skill roots always use local filesystem and User scope, so the absolute skill
// root path is sufficient to share their snapshot between plugin and skill loading.
let cache_key = root.plugin_root.as_ref().map(|_| root.path.clone());
let cached_snapshot = cache_key
.as_ref()
.map_or((0, None), |key| self.cached_snapshot(key));
.and_then(|root| match self.snapshots.read() {
Ok(cache) => cache.get(root).cloned(),
Err(err) => err.into_inner().get(root).cloned(),
});
let snapshot = match cached_snapshot {
Some(snapshot) => snapshot,
None => {
let snapshot = load_skill_root(root).await;
if let Some(cache_key) = cache_key {
self.cache_snapshot(cache_generation, cache_key, snapshot.clone());
if let Some(root) = cache_key {
let mut cache = self
.snapshots
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
if cache.len() < MAX_CACHED_PLUGIN_SKILL_ROOTS || cache.contains_key(&root)
{
cache.insert(root, snapshot.clone());
}
}
snapshot
}
@@ -102,34 +65,10 @@ impl SkillRootLoader {
/// Invalidates every cached plugin-root snapshot.
pub fn clear_cache(&self) {
let mut cache = self
.plugin_root_cache
.snapshots
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
cache.generation = cache.generation.wrapping_add(1);
cache.snapshots.clear();
}
fn cached_snapshot(&self, key: &SkillRootCacheKey) -> (u64, Option<SkillRootSnapshot>) {
match self.plugin_root_cache.read() {
Ok(cache) => (cache.generation, cache.snapshots.get(key).cloned()),
Err(err) => {
let cache = err.into_inner();
(cache.generation, cache.snapshots.get(key).cloned())
}
}
}
fn cache_snapshot(&self, generation: u64, key: SkillRootCacheKey, snapshot: SkillRootSnapshot) {
let mut cache = self
.plugin_root_cache
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
if cache.generation == generation
&& (cache.snapshots.len() < MAX_CACHED_PLUGIN_SKILL_ROOTS
|| cache.snapshots.contains_key(&key))
{
cache.snapshots.insert(key, snapshot);
}
cache.clear();
}
}
@@ -178,9 +117,10 @@ fn merge_skill_root_snapshots(snapshots: Vec<SkillRootSnapshot>) -> SkillLoadOut
outcome.skill_root_by_path = Arc::new(skill_root_by_path);
outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path);
// The merged outcome includes non-plugin roots, so preserve the global scope ordering.
outcome.skills.sort_by(|a, b| {
scope_rank(a.scope)
.cmp(&scope_rank(b.scope))
merged_skill_scope_rank(a.scope)
.cmp(&merged_skill_scope_rank(b.scope))
.then_with(|| a.name.cmp(&b.name))
.then_with(|| a.path_to_skills_md.cmp(&b.path_to_skills_md))
});
@@ -188,7 +128,7 @@ fn merge_skill_root_snapshots(snapshots: Vec<SkillRootSnapshot>) -> SkillLoadOut
outcome
}
fn scope_rank(scope: codex_protocol::protocol::SkillScope) -> u8 {
fn merged_skill_scope_rank(scope: codex_protocol::protocol::SkillScope) -> u8 {
use codex_protocol::protocol::SkillScope;
match scope {
@@ -200,5 +140,5 @@ fn scope_rank(scope: codex_protocol::protocol::SkillScope) -> u8 {
}
#[cfg(test)]
#[path = "root_loader_tests.rs"]
#[path = "plugin_skill_root_cache_tests.rs"]
mod tests;
codex-rs/core-skills/src/root_loader_tests.rs → codex-rs/core-skills/src/plugin_skill_root_cache_tests.rs
+7 -28
View File
@@ -5,10 +5,8 @@ use codex_exec_server::LOCAL_FS;
use codex_protocol::protocol::SkillScope;
use codex_utils_absolute_path::test_support::PathExt;
use super::SkillRootCacheKey;
use super::SkillRootLoader;
use super::PluginSkillRootCache;
use crate::loader::SkillRoot;
use crate::loader::load_skill_root;
fn plugin_root(path: &std::path::Path) -> SkillRoot {
SkillRoot {
@@ -32,45 +30,26 @@ fn write_skill(path: &std::path::Path, description: &str) {
}
#[tokio::test]
async fn reuses_plugin_root_snapshot_until_cache_is_cleared() {
async fn reuses_plugin_skill_root_snapshot_until_cache_is_cleared() {
let tempdir = tempfile::tempdir().expect("tempdir");
let loader = SkillRootLoader::default();
let cache = PluginSkillRootCache::default();
write_skill(tempdir.path(), "first");
let first = loader
let first = cache
.load_skills_from_roots([plugin_root(tempdir.path())])
.await;
write_skill(tempdir.path(), "second");
let cached = loader
let cached = cache
.load_skills_from_roots([plugin_root(tempdir.path())])
.await;
assert_eq!(cached.skills, first.skills);
loader.clear_cache();
let refreshed = loader
cache.clear_cache();
let refreshed = cache
.load_skills_from_roots([plugin_root(tempdir.path())])
.await;
let mut expected = first.skills;
expected[0].description = "second".to_string();
assert_eq!(refreshed.skills, expected);
}
#[tokio::test]
async fn clear_cache_rejects_an_in_flight_snapshot() {
let tempdir = tempfile::tempdir().expect("tempdir");
let loader = SkillRootLoader::default();
write_skill(tempdir.path(), "first");
let root = plugin_root(tempdir.path());
let cache_key = SkillRootCacheKey::from_root(&root).expect("plugin root cache key");
let (generation, cached_snapshot) = loader.cached_snapshot(&cache_key);
assert!(cached_snapshot.is_none());
let snapshot = load_skill_root(root).await;
loader.clear_cache();
loader.cache_snapshot(generation, cache_key.clone(), snapshot);
let (_, cached_snapshot) = loader.cached_snapshot(&cache_key);
assert!(cached_snapshot.is_none());
}
codex-rs/core-skills/src/service.rs
+13 -8
View File
@@ -14,8 +14,8 @@ use tracing::instrument;
use tracing::warn;
use crate::HostSkillsSnapshot;
use crate::PluginSkillRootCache;
use crate::SkillLoadOutcome;
use crate::SkillRootLoader;
use crate::build_implicit_skill_path_indexes;
use crate::config_rules::SkillConfigRules;
use crate::config_rules::resolve_disabled_skill_paths;
@@ -56,7 +56,7 @@ impl SkillsLoadInput {
pub struct SkillsService {
codex_home: AbsolutePathBuf,
restriction_product: Option<Product>,
skill_root_loader: Arc<SkillRootLoader>,
plugin_skill_root_cache: Arc<PluginSkillRootCache>,
extra_roots: RwLock<Vec<AbsolutePathBuf>>,
cache_by_cwd: RwLock<HashMap<AbsolutePathBuf, HostSkillsSnapshot>>,
cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, HostSkillsSnapshot>>,
@@ -75,7 +75,7 @@ impl SkillsService {
let service = Self {
codex_home,
restriction_product,
skill_root_loader: Arc::new(SkillRootLoader::default()),
plugin_skill_root_cache: Arc::new(PluginSkillRootCache::default()),
extra_roots: RwLock::new(Vec::new()),
cache_by_cwd: RwLock::new(HashMap::new()),
cache_by_config: RwLock::new(HashMap::new()),
@@ -90,9 +90,12 @@ impl SkillsService {
service
}
/// Reuses plugin-root snapshots populated by another owner of the loader.
pub fn with_skill_root_loader(mut self, skill_root_loader: Arc<SkillRootLoader>) -> Self {
self.skill_root_loader = skill_root_loader;
/// Reuses plugin-root snapshots populated by another owner of the cache.
pub fn with_plugin_skill_root_cache(
mut self,
plugin_skill_root_cache: Arc<PluginSkillRootCache>,
) -> Self {
self.plugin_skill_root_cache = plugin_skill_root_cache;
self
}
@@ -207,7 +210,9 @@ impl SkillsService {
skill_config_rules: &SkillConfigRules,
) -> SkillLoadOutcome {
let outcome = crate::filter_skill_load_outcome_for_product(
self.skill_root_loader.load_skills_from_roots(roots).await,
self.plugin_skill_root_cache
.load_skills_from_roots(roots)
.await,
self.restriction_product,
);
let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules);
@@ -215,7 +220,7 @@ impl SkillsService {
}
pub fn clear_cache(&self) {
self.skill_root_loader.clear_cache();
self.plugin_skill_root_cache.clear_cache();
let cleared_cwd = {
let mut cache = self
.cache_by_cwd
codex-rs/core/src/thread_manager.rs
+2 -2
View File
@@ -287,7 +287,7 @@ impl ThreadManager {
config.bundled_skills_enabled(),
restriction_product,
)
.with_skill_root_loader(plugins_manager.skill_root_loader()),
.with_plugin_skill_root_cache(plugins_manager.plugin_skill_root_cache()),
);
Self {
state: Arc::new(ThreadManagerState {
@@ -382,7 +382,7 @@ impl ThreadManager {
/*bundled_skills_enabled*/ true,
restriction_product,
)
.with_skill_root_loader(plugins_manager.skill_root_loader()),
.with_plugin_skill_root_cache(plugins_manager.plugin_skill_root_cache()),
);
// This test constructor has no Config input. Tests that need a non-local
// process store should construct ThreadManager::new with an explicit store.