Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions codex-rs/app-server/src/skills_watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ impl SkillsWatcher {
.skill_roots_for_config(&skills_input, Some(environment.get_filesystem()))
.await
.into_iter()
// Plugin roots are invalidated by plugin lifecycle operations.
.filter(|root| root.plugin_id.is_none())
.map(|root| WatchPath {
path: root.path.into_path_buf(),
recursive: true,
Expand Down
22 changes: 18 additions & 4 deletions codex-rs/core-plugins/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use codex_config::HooksFile;
use codex_config::types::McpServerConfig;
use codex_config::types::PluginConfig;
use codex_config::types::PluginMcpServerConfig;
use codex_core_skills::PluginSkillSnapshots;
use codex_core_skills::SkillMetadata;
use codex_core_skills::config_rules::SkillConfigRules;
use codex_core_skills::config_rules::resolve_disabled_skill_paths;
Expand Down Expand Up @@ -72,6 +73,7 @@ enum PluginLoadScope<'a> {
AllCapabilities {
restriction_product: Option<Product>,
skill_config_rules: &'a SkillConfigRules,
plugin_skill_snapshots: Option<&'a PluginSkillSnapshots>,
},
HooksOnly,
}
Expand Down Expand Up @@ -113,6 +115,7 @@ pub(crate) async fn load_plugins_from_layer_stack(
config_layer_stack: &ConfigLayerStack,
extra_plugins: HashMap<String, PluginConfig>,
store: &PluginStore,
plugin_skill_snapshots: Option<&PluginSkillSnapshots>,
restriction_product: Option<Product>,
prefer_remote_curated_conflicts: bool,
) -> Vec<LoadedPlugin<McpServerConfig>> {
Expand All @@ -125,6 +128,7 @@ pub(crate) async fn load_plugins_from_layer_stack(
PluginLoadScope::AllCapabilities {
restriction_product,
skill_config_rules: &skill_config_rules,
plugin_skill_snapshots,
},
)
.await
Expand Down Expand Up @@ -713,6 +717,7 @@ async fn load_plugin(
PluginLoadScope::AllCapabilities {
restriction_product,
skill_config_rules,
plugin_skill_snapshots,
} => {
loaded_plugin.manifest_name = Some(manifest.display_name().to_string());
loaded_plugin.manifest_description = manifest.description.clone();
Expand All @@ -723,6 +728,7 @@ async fn load_plugin(
&manifest,
*restriction_product,
skill_config_rules,
*plugin_skill_snapshots,
)
.await;
let has_enabled_skills = resolved_skills.has_enabled_skills();
Expand Down Expand Up @@ -819,17 +825,25 @@ pub async fn load_plugin_skills(
manifest: &PluginManifest,
restriction_product: Option<Product>,
skill_config_rules: &SkillConfigRules,
plugin_skill_snapshots: Option<&PluginSkillSnapshots>,
) -> ResolvedPluginSkills {
load_plugin_skill_inventory(plugin_root, plugin_id, manifest, restriction_product)
.await
.resolve(skill_config_rules)
load_plugin_skill_inventory(
plugin_root,
plugin_id,
manifest,
restriction_product,
plugin_skill_snapshots,
)
.await
.resolve(skill_config_rules)
}

pub(crate) async fn load_plugin_skill_inventory(
plugin_root: &AbsolutePathBuf,
plugin_id: &PluginId,
manifest: &PluginManifest,
restriction_product: Option<Product>,
plugin_skill_snapshots: Option<&PluginSkillSnapshots>,
) -> PluginSkillInventory {
let roots = plugin_skill_roots(plugin_root, &manifest.paths)
.into_iter()
Expand All @@ -842,7 +856,7 @@ pub(crate) async fn load_plugin_skill_inventory(
plugin_root: Some(plugin_root.clone()),
})
.collect::<Vec<_>>();
let outcome = load_skills_from_roots(roots).await;
let outcome = load_skills_from_roots(roots, plugin_skill_snapshots).await;
let had_errors = !outcome.errors.is_empty();
let skills = outcome
.skills
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core-plugins/src/loader_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ enabled = true
&stack,
HashMap::new(),
&store,
/*plugin_skill_snapshots*/ None,
Some(Product::Codex),
/*prefer_remote_curated_conflicts*/ false,
)
Expand Down
74 changes: 54 additions & 20 deletions codex-rs/core-plugins/src/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ 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::PluginSkillSnapshots;
use codex_core_skills::SkillMetadata;
use codex_core_skills::config_rules::SkillConfigRules;
use codex_core_skills::config_rules::skill_config_rules_from_stack;
Expand Down Expand Up @@ -368,6 +369,7 @@ pub struct PluginsManager {
struct LoadedPluginsCacheEntry {
key: PluginLoadCacheKey,
plugins: Vec<LoadedPlugin>,
plugin_skill_snapshots: PluginSkillSnapshots,
}

#[derive(Default)]
Expand All @@ -383,6 +385,16 @@ struct PluginLoadCacheKey {
remote_plugin_enabled: bool,
}

impl PluginLoadCacheKey {
fn from_config(config: &PluginsConfigInput) -> Self {
Self {
configured_plugins: configured_plugins_from_stack(&config.config_layer_stack),
skill_config_rules: skill_config_rules_from_stack(&config.config_layer_stack),
remote_plugin_enabled: config.remote_plugin_enabled,
}
}
}

impl PluginsManager {
pub fn new(codex_home: PathBuf) -> Self {
Self::new_with_options(codex_home, Some(Product::Codex), /*auth_mode*/ None)
Expand Down Expand Up @@ -468,6 +480,24 @@ impl PluginsManager {
.await
}

/// Returns skill snapshots parsed while loading the matching plugin cache entry.
pub fn plugin_skill_snapshots_for_config(
&self,
config: &PluginsConfigInput,
) -> Option<PluginSkillSnapshots> {
if !config.plugins_enabled {
return None;
}
let key = PluginLoadCacheKey::from_config(config);
self.loaded_plugins_cache
.read()
.unwrap_or_else(std::sync::PoisonError::into_inner)
.entry
.as_ref()
.filter(|cached| cached.key == key)
.map(|cached| cached.plugin_skill_snapshots.clone())
}

#[instrument(
name = "plugins_for_config",
level = "info",
Expand All @@ -487,11 +517,7 @@ impl PluginsManager {
return PluginLoadOutcome::default();
}

let cache_key = PluginLoadCacheKey {
configured_plugins: configured_plugins_from_stack(&config.config_layer_stack),
skill_config_rules: skill_config_rules_from_stack(&config.config_layer_stack),
remote_plugin_enabled: config.remote_plugin_enabled,
};
let cache_key = PluginLoadCacheKey::from_config(config);
if !force_reload && let Some(plugins) = self.cached_loaded_plugins(&cache_key) {
return self.resolve_loaded_plugins_for_auth(plugins);
}
Expand All @@ -504,16 +530,23 @@ impl PluginsManager {
return self.resolve_loaded_plugins_for_auth(plugins);
}
let cache_generation = self.loaded_plugins_cache_generation();
let plugin_skill_snapshots = PluginSkillSnapshots::for_plugin_load();
let plugins = load_plugins_from_layer_stack(
&config.config_layer_stack,
self.remote_installed_plugin_configs(),
&self.store,
Some(&plugin_skill_snapshots),
self.restriction_product,
config.remote_plugin_enabled,
)
.await;
log_plugin_load_errors(&plugins);
self.cache_loaded_plugins_if_current(cache_generation, cache_key, plugins.clone());
self.cache_loaded_plugins_if_current(
cache_generation,
cache_key,
plugins.clone(),
plugin_skill_snapshots,
);
self.resolve_loaded_plugins_for_auth(plugins)
}

Expand Down Expand Up @@ -587,6 +620,7 @@ impl PluginsManager {
config_layer_stack,
self.remote_installed_plugin_configs(),
&self.store,
/*plugin_skill_snapshots*/ None,
self.restriction_product,
config.remote_plugin_enabled,
)
Expand Down Expand Up @@ -624,19 +658,13 @@ impl PluginsManager {
}

fn cached_loaded_plugins(&self, key: &PluginLoadCacheKey) -> Option<Vec<LoadedPlugin>> {
match self.loaded_plugins_cache.read() {
Ok(cache) => cache
.entry
.as_ref()
.filter(|cached| cached.key == *key)
.map(|cached| cached.plugins.clone()),
Err(err) => err
.into_inner()
.entry
.as_ref()
.filter(|cached| cached.key == *key)
.map(|cached| cached.plugins.clone()),
}
self.loaded_plugins_cache
.read()
.unwrap_or_else(std::sync::PoisonError::into_inner)
.entry
.as_ref()
.filter(|cached| cached.key == *key)
.map(|cached| cached.plugins.clone())
}

fn loaded_plugins_cache_generation(&self) -> u64 {
Expand All @@ -651,13 +679,18 @@ impl PluginsManager {
generation: u64,
key: PluginLoadCacheKey,
plugins: Vec<LoadedPlugin>,
plugin_skill_snapshots: PluginSkillSnapshots,
) {
let mut cache = match self.loaded_plugins_cache.write() {
Ok(cache) => cache,
Err(err) => err.into_inner(),
};
if cache.generation == generation {
cache.entry = Some(LoadedPluginsCacheEntry { key, plugins });
cache.entry = Some(LoadedPluginsCacheEntry {
key,
plugins,
plugin_skill_snapshots,
});
}
}

Expand Down Expand Up @@ -1624,6 +1657,7 @@ impl PluginsManager {
&codex_core_skills::config_rules::skill_config_rules_from_stack(
&config.config_layer_stack,
),
/*plugin_skill_snapshots*/ None,
)
.await;
let plugin_data_root = self.store.plugin_data_root(&plugin_id);
Expand Down
61 changes: 60 additions & 1 deletion codex-rs/core-plugins/src/manager_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ use codex_config::McpServerConfig;
use codex_config::McpServerOAuthConfig;
use codex_config::McpServerToolConfig;
use codex_config::types::McpServerTransportConfig;
use codex_core_skills::PluginSkillSnapshots;
use codex_core_skills::SkillsLoadInput;
use codex_core_skills::SkillsService;
use codex_core_skills::config_rules::SkillConfigRules;
use codex_login::CodexAuth;
use codex_plugin::AppDeclaration;
Expand Down Expand Up @@ -1476,6 +1479,7 @@ async fn load_plugin_skills_dedupes_overlapping_manifest_roots() {
&manifest,
/*restriction_product*/ None,
&SkillConfigRules::default(),
/*plugin_skill_snapshots*/ None,
)
.await;

Expand Down Expand Up @@ -2039,6 +2043,55 @@ async fn plugin_cache_ignores_unrelated_session_overrides() {
assert_eq!(second.plugins()[0].mcp_servers.len(), 1);
}

#[tokio::test]
async fn skills_service_reuses_skills_parsed_during_plugin_load() {
let codex_home = TempDir::new().unwrap();
let codex_home_abs = codex_home.path().to_path_buf().abs();
let plugin_root = codex_home
.path()
.join("plugins/cache")
.join("test/sample/local");
write_plugin(
codex_home.path().join("plugins/cache/test").as_path(),
"sample/local",
"sample",
);
let skill_path = plugin_root.join("skills/SKILL.md");
write_file(&skill_path, "---\nname: search\ndescription: first\n---\n");
write_file(
&codex_home.path().join(CONFIG_TOML_FILE),
&plugin_config_toml(/*enabled*/ true, /*plugins_feature_enabled*/ true),
);

let config = load_config(codex_home.path(), codex_home.path()).await;
let manager = PluginsManager::new(codex_home.path().to_path_buf());
let plugin_outcome = manager.plugins_for_config(&config).await;
let plugin_skill_snapshots = manager.plugin_skill_snapshots_for_config(&config);
write_file(&skill_path, "---\nname: search\ndescription: second\n---\n");

let skills_input = SkillsLoadInput::new(
codex_home_abs.clone(),
plugin_outcome.effective_plugin_skill_roots(),
config.config_layer_stack.clone(),
/*bundled_skills_enabled*/ false,
)
.with_plugin_skill_snapshots(plugin_skill_snapshots);
let skills_service = SkillsService::new(codex_home_abs, /*bundled_skills_enabled*/ false);
let cached = skills_service
.snapshot_for_config(&skills_input, /*fs*/ None)
.await;

assert_eq!(
cached
.outcome()
.skills
.iter()
.map(|skill| skill.description.as_str())
.collect::<Vec<_>>(),
vec!["first"]
);
}

#[test]
fn loaded_plugins_cache_invalidation_rejects_stale_load_completion() {
let codex_home = TempDir::new().unwrap();
Expand All @@ -2051,7 +2104,12 @@ fn loaded_plugins_cache_invalidation_rejects_stale_load_completion() {
let stale_generation = manager.loaded_plugins_cache_generation();

manager.clear_loaded_plugins_cache();
manager.cache_loaded_plugins_if_current(stale_generation, cache_key.clone(), Vec::new());
manager.cache_loaded_plugins_if_current(
stale_generation,
cache_key.clone(),
Vec::new(),
PluginSkillSnapshots::for_plugin_load(),
);

assert_eq!(manager.cached_loaded_plugins(&cache_key), None);
}
Expand Down Expand Up @@ -4924,6 +4982,7 @@ async fn load_plugins_ignores_project_config_files() {
&stack,
std::collections::HashMap::new(),
&PluginStore::new(codex_home.path().to_path_buf()),
/*plugin_skill_snapshots*/ None,
Some(Product::Codex),
/*prefer_remote_curated_conflicts*/ false,
)
Expand Down
10 changes: 8 additions & 2 deletions codex-rs/core-plugins/src/tool_suggest_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,14 @@ async fn load_plugin_metadata(
}
let manifest = load_plugin_manifest(plugin_root.as_path())
.ok_or_else(|| "missing or invalid plugin.json".to_string())?;
let skill_inventory =
load_plugin_skill_inventory(plugin_root, &plugin_id, &manifest, restriction_product).await;
let skill_inventory = load_plugin_skill_inventory(
plugin_root,
&plugin_id,
&manifest,
restriction_product,
/*plugin_skill_snapshots*/ None,
)
.await;
let mut mcp_server_names =
load_plugin_mcp_servers(plugin_root.as_path(), /*auth_mode*/ None)
.await
Expand Down
2 changes: 2 additions & 0 deletions codex-rs/core-skills/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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::PluginSkillSnapshots;
pub use service::SkillsLoadInput;
pub use service::SkillsService;
pub use skill_instructions::SkillInstructions;
Loading
Loading