mirror of
https://github.com/openai/codex.git
synced 2026-10-28 18:58:46 +00:00
Reuse parsed plugin skill roots
This commit is contained in:
@@ -20,11 +20,11 @@ use codex_config::types::McpServerConfig;
|
||||
use codex_config::types::PluginConfig;
|
||||
use codex_config::types::PluginMcpServerConfig;
|
||||
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;
|
||||
use codex_core_skills::loader::SkillRoot;
|
||||
use codex_core_skills::loader::load_skills_from_roots;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_mcp::PluginMcpServerPlacement;
|
||||
use codex_mcp::parse_plugin_mcp_config;
|
||||
@@ -72,6 +72,7 @@ enum PluginLoadScope<'a> {
|
||||
AllCapabilities {
|
||||
restriction_product: Option<Product>,
|
||||
skill_config_rules: &'a SkillConfigRules,
|
||||
skill_root_loader: &'a SkillRootLoader,
|
||||
},
|
||||
HooksOnly,
|
||||
}
|
||||
@@ -113,6 +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,
|
||||
restriction_product: Option<Product>,
|
||||
prefer_remote_curated_conflicts: bool,
|
||||
) -> Vec<LoadedPlugin<McpServerConfig>> {
|
||||
@@ -125,6 +127,7 @@ pub(crate) async fn load_plugins_from_layer_stack(
|
||||
PluginLoadScope::AllCapabilities {
|
||||
restriction_product,
|
||||
skill_config_rules: &skill_config_rules,
|
||||
skill_root_loader,
|
||||
},
|
||||
)
|
||||
.await
|
||||
@@ -713,6 +716,7 @@ async fn load_plugin(
|
||||
PluginLoadScope::AllCapabilities {
|
||||
restriction_product,
|
||||
skill_config_rules,
|
||||
skill_root_loader,
|
||||
} => {
|
||||
loaded_plugin.manifest_name = Some(manifest.display_name().to_string());
|
||||
loaded_plugin.manifest_description = manifest.description.clone();
|
||||
@@ -723,6 +727,7 @@ async fn load_plugin(
|
||||
&manifest,
|
||||
*restriction_product,
|
||||
skill_config_rules,
|
||||
skill_root_loader,
|
||||
)
|
||||
.await;
|
||||
let has_enabled_skills = resolved_skills.has_enabled_skills();
|
||||
@@ -791,6 +796,7 @@ pub async fn load_plugin_skills(
|
||||
manifest: &PluginManifest,
|
||||
restriction_product: Option<Product>,
|
||||
skill_config_rules: &SkillConfigRules,
|
||||
skill_root_loader: &SkillRootLoader,
|
||||
) -> ResolvedPluginSkills {
|
||||
let roots = plugin_skill_roots(plugin_root, &manifest.paths)
|
||||
.into_iter()
|
||||
@@ -803,7 +809,7 @@ pub async fn load_plugin_skills(
|
||||
plugin_root: Some(plugin_root.clone()),
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
let outcome = load_skills_from_roots(roots).await;
|
||||
let outcome = skill_root_loader.load_skills_from_roots(roots).await;
|
||||
let had_errors = !outcome.errors.is_empty();
|
||||
let skills = outcome
|
||||
.skills
|
||||
|
||||
@@ -5,6 +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_plugin::PluginId;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::TempDir;
|
||||
@@ -155,11 +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 full = load_plugins_from_layer_stack(
|
||||
&stack,
|
||||
HashMap::new(),
|
||||
&store,
|
||||
&skill_root_loader,
|
||||
Some(Product::Codex),
|
||||
/*prefer_remote_curated_conflicts*/ false,
|
||||
)
|
||||
|
||||
@@ -60,6 +60,7 @@ use codex_config::types::PluginConfig;
|
||||
use codex_config::types::ToolSuggestDisabledTool;
|
||||
use codex_config::types::ToolSuggestDiscoverableType;
|
||||
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;
|
||||
@@ -354,6 +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>,
|
||||
remote_installed_plugins_cache: RwLock<Option<Vec<RemoteInstalledPlugin>>>,
|
||||
remote_installed_plugins_cache_refresh_state: RwLock<RemoteInstalledPluginsCacheRefreshState>,
|
||||
global_remote_catalog_cache_refresh_state: RwLock<GlobalRemoteCatalogCacheRefreshState>,
|
||||
@@ -410,6 +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()),
|
||||
remote_installed_plugins_cache: RwLock::new(None),
|
||||
remote_installed_plugins_cache_refresh_state: RwLock::new(
|
||||
RemoteInstalledPluginsCacheRefreshState::default(),
|
||||
@@ -435,6 +438,11 @@ 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)
|
||||
}
|
||||
|
||||
pub fn auth_mode(&self) -> Option<AuthMode> {
|
||||
match self.auth_mode.read() {
|
||||
Ok(auth_mode_guard) => *auth_mode_guard,
|
||||
@@ -505,6 +513,7 @@ impl PluginsManager {
|
||||
&config.config_layer_stack,
|
||||
self.remote_installed_plugin_configs(),
|
||||
&self.store,
|
||||
self.skill_root_loader.as_ref(),
|
||||
self.restriction_product,
|
||||
config.remote_plugin_enabled,
|
||||
)
|
||||
@@ -551,6 +560,7 @@ impl PluginsManager {
|
||||
}
|
||||
|
||||
fn clear_loaded_plugins_cache(&self) {
|
||||
self.skill_root_loader.clear_cache();
|
||||
let mut cache = match self.loaded_plugins_cache.write() {
|
||||
Ok(cache) => cache,
|
||||
Err(err) => err.into_inner(),
|
||||
@@ -572,6 +582,7 @@ impl PluginsManager {
|
||||
config_layer_stack,
|
||||
self.remote_installed_plugin_configs(),
|
||||
&self.store,
|
||||
self.skill_root_loader.as_ref(),
|
||||
self.restriction_product,
|
||||
config.remote_plugin_enabled,
|
||||
)
|
||||
@@ -1596,6 +1607,7 @@ impl PluginsManager {
|
||||
&codex_core_skills::config_rules::skill_config_rules_from_stack(
|
||||
&config.config_layer_stack,
|
||||
),
|
||||
self.skill_root_loader.as_ref(),
|
||||
)
|
||||
.await;
|
||||
let plugin_data_root = self.store.plugin_data_root(&plugin_id);
|
||||
|
||||
@@ -34,6 +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::config_rules::SkillConfigRules;
|
||||
use codex_login::CodexAuth;
|
||||
use codex_plugin::AppDeclaration;
|
||||
@@ -1466,6 +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 resolved = load_plugin_skills(
|
||||
&plugin_root,
|
||||
@@ -1473,6 +1475,7 @@ async fn load_plugin_skills_dedupes_overlapping_manifest_roots() {
|
||||
&manifest,
|
||||
/*restriction_product*/ None,
|
||||
&SkillConfigRules::default(),
|
||||
&skill_root_loader,
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -4820,10 +4823,12 @@ async fn load_plugins_ignores_project_config_files() {
|
||||
)
|
||||
.expect("config layer stack should build");
|
||||
|
||||
let skill_root_loader = SkillRootLoader::default();
|
||||
let plugins = load_plugins_from_layer_stack(
|
||||
&stack,
|
||||
std::collections::HashMap::new(),
|
||||
&PluginStore::new(codex_home.path().to_path_buf()),
|
||||
&skill_root_loader,
|
||||
Some(Product::Codex),
|
||||
/*prefer_remote_curated_conflicts*/ false,
|
||||
)
|
||||
|
||||
@@ -6,6 +6,7 @@ mod mention_counts;
|
||||
pub mod model;
|
||||
pub mod remote;
|
||||
pub mod render;
|
||||
mod root_loader;
|
||||
pub mod service;
|
||||
mod skill_instructions;
|
||||
pub mod system;
|
||||
@@ -29,6 +30,7 @@ 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;
|
||||
|
||||
@@ -1,11 +1,11 @@
|
||||
use crate::model::SkillDependencies;
|
||||
use crate::model::SkillError;
|
||||
use crate::model::SkillFileSystemsByPath;
|
||||
use crate::model::SkillInterface;
|
||||
use crate::model::SkillLoadOutcome;
|
||||
use crate::model::SkillMetadata;
|
||||
use crate::model::SkillPolicy;
|
||||
use crate::model::SkillToolDependency;
|
||||
use crate::root_loader::SkillRootLoader;
|
||||
use crate::system::system_cache_root_dir;
|
||||
use codex_app_server_protocol::ConfigLayerSource;
|
||||
use codex_config::ConfigLayerStack;
|
||||
@@ -24,7 +24,6 @@ use codex_utils_plugins::PluginSkillRoot;
|
||||
use codex_utils_plugins::plugin_namespace_for_skill_path;
|
||||
use dirs::home_dir;
|
||||
use serde::Deserialize;
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::collections::VecDeque;
|
||||
use std::error::Error;
|
||||
@@ -165,73 +164,46 @@ pub async fn load_skills_from_roots<I>(roots: I) -> SkillLoadOutcome
|
||||
where
|
||||
I: IntoIterator<Item = SkillRoot>,
|
||||
{
|
||||
SkillRootLoader::default()
|
||||
.load_skills_from_roots(roots)
|
||||
.await
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct SkillRootSnapshot {
|
||||
pub(crate) root: AbsolutePathBuf,
|
||||
pub(crate) skills: Vec<SkillMetadata>,
|
||||
pub(crate) errors: Vec<SkillError>,
|
||||
pub(crate) file_system: Arc<dyn ExecutorFileSystem>,
|
||||
}
|
||||
|
||||
pub(crate) async fn load_skill_root(root: SkillRoot) -> SkillRootSnapshot {
|
||||
let SkillRoot {
|
||||
path,
|
||||
scope,
|
||||
file_system,
|
||||
plugin_id,
|
||||
plugin_namespace,
|
||||
plugin_root,
|
||||
} = root;
|
||||
let root = canonicalize_for_skill_identity(file_system.as_ref(), &path).await;
|
||||
let mut outcome = SkillLoadOutcome::default();
|
||||
let mut skill_roots: Vec<AbsolutePathBuf> = Vec::new();
|
||||
let mut skill_root_by_path: HashMap<AbsolutePathBuf, AbsolutePathBuf> = HashMap::new();
|
||||
let mut file_systems_by_skill_path: HashMap<AbsolutePathBuf, Arc<dyn ExecutorFileSystem>> =
|
||||
HashMap::new();
|
||||
for root in roots {
|
||||
let fs = root.file_system;
|
||||
let root_path = canonicalize_for_skill_identity(fs.as_ref(), &root.path).await;
|
||||
let skills_before_root = outcome.skills.len();
|
||||
discover_skills_under_root(
|
||||
fs.as_ref(),
|
||||
&root_path,
|
||||
root.scope,
|
||||
root.plugin_id.as_deref(),
|
||||
root.plugin_namespace.as_deref(),
|
||||
root.plugin_root.as_ref(),
|
||||
&mut outcome,
|
||||
)
|
||||
.await;
|
||||
for skill in &outcome.skills[skills_before_root..] {
|
||||
if !skill_roots.contains(&root_path) {
|
||||
skill_roots.push(root_path.clone());
|
||||
}
|
||||
skill_root_by_path
|
||||
.entry(skill.path_to_skills_md.clone())
|
||||
.or_insert_with(|| root_path.clone());
|
||||
file_systems_by_skill_path
|
||||
.entry(skill.path_to_skills_md.clone())
|
||||
.or_insert_with(|| Arc::clone(&fs));
|
||||
}
|
||||
discover_skills_under_root(
|
||||
file_system.as_ref(),
|
||||
&root,
|
||||
scope,
|
||||
plugin_id.as_deref(),
|
||||
plugin_namespace.as_deref(),
|
||||
plugin_root.as_ref(),
|
||||
&mut outcome,
|
||||
)
|
||||
.await;
|
||||
SkillRootSnapshot {
|
||||
root,
|
||||
skills: outcome.skills,
|
||||
errors: outcome.errors,
|
||||
file_system,
|
||||
}
|
||||
|
||||
let mut seen: HashSet<AbsolutePathBuf> = HashSet::new();
|
||||
outcome
|
||||
.skills
|
||||
.retain(|skill| seen.insert(skill.path_to_skills_md.clone()));
|
||||
let retained_skill_paths: HashSet<AbsolutePathBuf> = outcome
|
||||
.skills
|
||||
.iter()
|
||||
.map(|skill| skill.path_to_skills_md.clone())
|
||||
.collect();
|
||||
skill_root_by_path.retain(|path, _| retained_skill_paths.contains(path));
|
||||
let used_roots: HashSet<AbsolutePathBuf> = skill_root_by_path.values().cloned().collect();
|
||||
skill_roots.retain(|root| used_roots.contains(root));
|
||||
file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path));
|
||||
outcome.skill_roots = skill_roots;
|
||||
outcome.skill_root_by_path = Arc::new(skill_root_by_path);
|
||||
outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path);
|
||||
|
||||
fn scope_rank(scope: SkillScope) -> u8 {
|
||||
// Higher-priority scopes first (matches root scan order for dedupe).
|
||||
match scope {
|
||||
SkillScope::Repo => 0,
|
||||
SkillScope::User => 1,
|
||||
SkillScope::System => 2,
|
||||
SkillScope::Admin => 3,
|
||||
}
|
||||
}
|
||||
|
||||
outcome.skills.sort_by(|a, b| {
|
||||
scope_rank(a.scope)
|
||||
.cmp(&scope_rank(b.scope))
|
||||
.then_with(|| a.name.cmp(&b.name))
|
||||
.then_with(|| a.path_to_skills_md.cmp(&b.path_to_skills_md))
|
||||
});
|
||||
|
||||
outcome
|
||||
}
|
||||
|
||||
pub(crate) async fn skill_roots(
|
||||
|
||||
@@ -0,0 +1,188 @@
|
||||
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;
|
||||
use crate::loader::SkillRoot;
|
||||
use crate::loader::SkillRootSnapshot;
|
||||
use crate::loader::load_skill_root;
|
||||
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.
|
||||
///
|
||||
/// Non-plugin roots are always loaded directly because their filesystem lifecycle is owned by the
|
||||
/// environment or config layer that supplied them.
|
||||
#[derive(Default)]
|
||||
pub struct SkillRootLoader {
|
||||
plugin_root_cache: RwLock<HashMap<SkillRootCacheKey, SkillRootSnapshot>>,
|
||||
}
|
||||
|
||||
impl SkillRootLoader {
|
||||
/// 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
|
||||
I: IntoIterator<Item = SkillRoot>,
|
||||
{
|
||||
let mut snapshots = Vec::new();
|
||||
for root in roots {
|
||||
let cache_key = SkillRootCacheKey::from_root(&root);
|
||||
let snapshot = match cache_key.as_ref().and_then(|key| self.cached_snapshot(key)) {
|
||||
Some(snapshot) => snapshot,
|
||||
None => {
|
||||
let snapshot = load_skill_root(root).await;
|
||||
if let Some(cache_key) = cache_key {
|
||||
self.cache_snapshot(cache_key, snapshot.clone());
|
||||
}
|
||||
snapshot
|
||||
}
|
||||
};
|
||||
snapshots.push(snapshot);
|
||||
}
|
||||
|
||||
merge_skill_root_snapshots(snapshots)
|
||||
}
|
||||
|
||||
/// Invalidates every cached plugin-root snapshot.
|
||||
pub fn clear_cache(&self) {
|
||||
let mut cache = self
|
||||
.plugin_root_cache
|
||||
.write()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
cache.clear();
|
||||
}
|
||||
|
||||
fn cached_snapshot(&self, key: &SkillRootCacheKey) -> Option<SkillRootSnapshot> {
|
||||
match self.plugin_root_cache.read() {
|
||||
Ok(cache) => cache.get(key).cloned(),
|
||||
Err(err) => err.into_inner().get(key).cloned(),
|
||||
}
|
||||
}
|
||||
|
||||
fn cache_snapshot(&self, key: SkillRootCacheKey, snapshot: SkillRootSnapshot) {
|
||||
let mut cache = self
|
||||
.plugin_root_cache
|
||||
.write()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
if cache.len() < MAX_CACHED_PLUGIN_SKILL_ROOTS || cache.contains_key(&key) {
|
||||
cache.insert(key, snapshot);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn merge_skill_root_snapshots(snapshots: Vec<SkillRootSnapshot>) -> SkillLoadOutcome {
|
||||
let mut outcome = SkillLoadOutcome::default();
|
||||
let mut skill_roots = Vec::new();
|
||||
let mut skill_root_by_path = HashMap::new();
|
||||
let mut file_systems_by_skill_path = HashMap::new();
|
||||
|
||||
for snapshot in snapshots {
|
||||
let SkillRootSnapshot {
|
||||
root,
|
||||
skills,
|
||||
errors,
|
||||
file_system,
|
||||
} = snapshot;
|
||||
if !skills.is_empty() && !skill_roots.contains(&root) {
|
||||
skill_roots.push(root.clone());
|
||||
}
|
||||
for skill in &skills {
|
||||
skill_root_by_path
|
||||
.entry(skill.path_to_skills_md.clone())
|
||||
.or_insert_with(|| root.clone());
|
||||
file_systems_by_skill_path
|
||||
.entry(skill.path_to_skills_md.clone())
|
||||
.or_insert_with(|| Arc::clone(&file_system));
|
||||
}
|
||||
outcome.skills.extend(skills);
|
||||
outcome.errors.extend(errors);
|
||||
}
|
||||
|
||||
let mut seen = HashSet::new();
|
||||
outcome
|
||||
.skills
|
||||
.retain(|skill| seen.insert(skill.path_to_skills_md.clone()));
|
||||
let retained_skill_paths = outcome
|
||||
.skills
|
||||
.iter()
|
||||
.map(|skill| skill.path_to_skills_md.clone())
|
||||
.collect::<HashSet<_>>();
|
||||
skill_root_by_path.retain(|path, _| retained_skill_paths.contains(path));
|
||||
let used_roots = skill_root_by_path.values().cloned().collect::<HashSet<_>>();
|
||||
skill_roots.retain(|root| used_roots.contains(root));
|
||||
file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path));
|
||||
outcome.skill_roots = skill_roots;
|
||||
outcome.skill_root_by_path = Arc::new(skill_root_by_path);
|
||||
outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path);
|
||||
|
||||
outcome.skills.sort_by(|a, b| {
|
||||
scope_rank(a.scope)
|
||||
.cmp(&scope_rank(b.scope))
|
||||
.then_with(|| a.name.cmp(&b.name))
|
||||
.then_with(|| a.path_to_skills_md.cmp(&b.path_to_skills_md))
|
||||
});
|
||||
|
||||
outcome
|
||||
}
|
||||
|
||||
fn scope_rank(scope: codex_protocol::protocol::SkillScope) -> u8 {
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
|
||||
match scope {
|
||||
SkillScope::Repo => 0,
|
||||
SkillScope::User => 1,
|
||||
SkillScope::System => 2,
|
||||
SkillScope::Admin => 3,
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "root_loader_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,55 @@
|
||||
use std::fs;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_utils_absolute_path::test_support::PathExt;
|
||||
|
||||
use super::SkillRootLoader;
|
||||
use crate::loader::SkillRoot;
|
||||
|
||||
fn plugin_root(path: &std::path::Path) -> SkillRoot {
|
||||
SkillRoot {
|
||||
path: path.join("skills").abs(),
|
||||
scope: SkillScope::User,
|
||||
file_system: Arc::clone(&LOCAL_FS),
|
||||
plugin_id: Some("sample@test".to_string()),
|
||||
plugin_namespace: Some("sample".to_string()),
|
||||
plugin_root: Some(path.abs()),
|
||||
}
|
||||
}
|
||||
|
||||
fn write_skill(path: &std::path::Path, description: &str) {
|
||||
let skill_dir = path.join("skills/search");
|
||||
fs::create_dir_all(&skill_dir).expect("create skill dir");
|
||||
fs::write(
|
||||
skill_dir.join("SKILL.md"),
|
||||
format!("---\nname: search\ndescription: {description}\n---\n"),
|
||||
)
|
||||
.expect("write skill");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn reuses_plugin_root_snapshot_until_cache_is_cleared() {
|
||||
let tempdir = tempfile::tempdir().expect("tempdir");
|
||||
let loader = SkillRootLoader::default();
|
||||
|
||||
write_skill(tempdir.path(), "first");
|
||||
let first = loader
|
||||
.load_skills_from_roots([plugin_root(tempdir.path())])
|
||||
.await;
|
||||
|
||||
write_skill(tempdir.path(), "second");
|
||||
let cached = loader
|
||||
.load_skills_from_roots([plugin_root(tempdir.path())])
|
||||
.await;
|
||||
assert_eq!(cached.skills, first.skills);
|
||||
|
||||
loader.clear_cache();
|
||||
let refreshed = loader
|
||||
.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);
|
||||
}
|
||||
@@ -15,12 +15,12 @@ use tracing::warn;
|
||||
|
||||
use crate::HostSkillsSnapshot;
|
||||
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;
|
||||
use crate::config_rules::skill_config_rules_from_stack;
|
||||
use crate::loader::SkillRoot;
|
||||
use crate::loader::load_skills_from_roots;
|
||||
use crate::loader::skill_roots;
|
||||
use crate::system::install_system_skills;
|
||||
use crate::system::uninstall_system_skills;
|
||||
@@ -56,6 +56,7 @@ impl SkillsLoadInput {
|
||||
pub struct SkillsService {
|
||||
codex_home: AbsolutePathBuf,
|
||||
restriction_product: Option<Product>,
|
||||
skill_root_loader: Arc<SkillRootLoader>,
|
||||
extra_roots: RwLock<Vec<AbsolutePathBuf>>,
|
||||
cache_by_cwd: RwLock<HashMap<AbsolutePathBuf, HostSkillsSnapshot>>,
|
||||
cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, HostSkillsSnapshot>>,
|
||||
@@ -74,6 +75,7 @@ impl SkillsService {
|
||||
let service = Self {
|
||||
codex_home,
|
||||
restriction_product,
|
||||
skill_root_loader: Arc::new(SkillRootLoader::default()),
|
||||
extra_roots: RwLock::new(Vec::new()),
|
||||
cache_by_cwd: RwLock::new(HashMap::new()),
|
||||
cache_by_config: RwLock::new(HashMap::new()),
|
||||
@@ -88,6 +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;
|
||||
self
|
||||
}
|
||||
|
||||
pub fn set_extra_roots(&self, extra_roots: Vec<AbsolutePathBuf>) {
|
||||
{
|
||||
let mut roots = self
|
||||
@@ -199,7 +207,7 @@ impl SkillsService {
|
||||
skill_config_rules: &SkillConfigRules,
|
||||
) -> SkillLoadOutcome {
|
||||
let outcome = crate::filter_skill_load_outcome_for_product(
|
||||
load_skills_from_roots(roots).await,
|
||||
self.skill_root_loader.load_skills_from_roots(roots).await,
|
||||
self.restriction_product,
|
||||
);
|
||||
let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules);
|
||||
@@ -207,6 +215,7 @@ impl SkillsService {
|
||||
}
|
||||
|
||||
pub fn clear_cache(&self) {
|
||||
self.skill_root_loader.clear_cache();
|
||||
let cleared_cwd = {
|
||||
let mut cache = self
|
||||
.cache_by_cwd
|
||||
|
||||
@@ -281,11 +281,14 @@ impl ThreadManager {
|
||||
Arc::clone(&plugins_manager),
|
||||
Arc::clone(&extensions),
|
||||
));
|
||||
let skills_service = Arc::new(SkillsService::new_with_restriction_product(
|
||||
codex_home,
|
||||
config.bundled_skills_enabled(),
|
||||
restriction_product,
|
||||
));
|
||||
let skills_service = Arc::new(
|
||||
SkillsService::new_with_restriction_product(
|
||||
codex_home,
|
||||
config.bundled_skills_enabled(),
|
||||
restriction_product,
|
||||
)
|
||||
.with_skill_root_loader(plugins_manager.skill_root_loader()),
|
||||
);
|
||||
Self {
|
||||
state: Arc::new(ThreadManagerState {
|
||||
threads: Arc::new(RwLock::new(HashMap::new())),
|
||||
@@ -373,11 +376,14 @@ impl ThreadManager {
|
||||
auth_manager.get_api_auth_mode(),
|
||||
));
|
||||
let mcp_manager = Arc::new(McpManager::new(Arc::clone(&plugins_manager)));
|
||||
let skills_service = Arc::new(SkillsService::new_with_restriction_product(
|
||||
skills_codex_home,
|
||||
/*bundled_skills_enabled*/ true,
|
||||
restriction_product,
|
||||
));
|
||||
let skills_service = Arc::new(
|
||||
SkillsService::new_with_restriction_product(
|
||||
skills_codex_home,
|
||||
/*bundled_skills_enabled*/ true,
|
||||
restriction_product,
|
||||
)
|
||||
.with_skill_root_loader(plugins_manager.skill_root_loader()),
|
||||
);
|
||||
// This test constructor has no Config input. Tests that need a non-local
|
||||
// process store should construct ThreadManager::new with an explicit store.
|
||||
let thread_store: Arc<dyn ThreadStore> = Arc::new(LocalThreadStore::new(
|
||||
|
||||
Reference in New Issue
Block a user