mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Load plugin hooks without other plugin capabilities (#26272)
## Summary `hooks/list` only consumes plugin hook declarations, but previously loaded every enabled plugin's skills, MCP configuration, apps, and capability summary before discarding them. In a local benchmark, this reduced `hooks/list` latency by over 100ms (e.g., from 594 to 467ms on startup, and 168 to 16ms when making a `hooks/list` call later in the same TUI session). This is on the critical path to rendering the TUI, so every 10s of ms should be eyed skeptically (IMO). This change adds a hook-specific plugin loading path that preserves plugin enablement, remote/local conflict resolution, deterministic ordering, manifest resolution, and hook-loading warnings while skipping unrelated capabilities. (I think there's room for a more general design here that allows you to project the capabilities you need at load-time, but that seems unnecessary right now.)
This commit is contained in:
committed by
GitHub
Unverified
parent
d81fcdf8ef
commit
4ae7930f58
@@ -303,7 +303,6 @@ use codex_core::windows_sandbox::WindowsSandboxSetupRequest;
|
||||
use codex_core::windows_sandbox::sandbox_setup_is_complete;
|
||||
use codex_core_plugins::PluginInstallError as CorePluginInstallError;
|
||||
use codex_core_plugins::PluginInstallRequest;
|
||||
use codex_core_plugins::PluginLoadOutcome;
|
||||
use codex_core_plugins::PluginReadRequest;
|
||||
use codex_core_plugins::PluginUninstallError as CorePluginUninstallError;
|
||||
use codex_core_plugins::loader::load_plugin_apps;
|
||||
|
||||
@@ -645,20 +645,20 @@ impl CatalogRequestProcessor {
|
||||
.await;
|
||||
let plugins_enabled =
|
||||
config.features.enabled(Feature::Plugins) && workspace_codex_plugins_enabled;
|
||||
let plugin_outcome = if plugins_enabled {
|
||||
let plugin_hooks = if plugins_enabled {
|
||||
let plugins_input = config.plugins_config_input();
|
||||
plugins_manager
|
||||
.plugins_for_layer_stack(&config.config_layer_stack, &plugins_input)
|
||||
.plugin_hooks_for_layer_stack(&config.config_layer_stack, &plugins_input)
|
||||
.await
|
||||
} else {
|
||||
PluginLoadOutcome::default()
|
||||
codex_core_plugins::PluginHookLoadOutcome::default()
|
||||
};
|
||||
let hooks = codex_hooks::list_hooks(codex_hooks::HooksConfig {
|
||||
feature_enabled: config.features.enabled(Feature::CodexHooks),
|
||||
bypass_hook_trust: config.bypass_hook_trust,
|
||||
config_layer_stack: Some(config.config_layer_stack),
|
||||
plugin_hook_sources: plugin_outcome.effective_plugin_hook_sources(),
|
||||
plugin_hook_load_warnings: plugin_outcome.effective_plugin_hook_warnings(),
|
||||
plugin_hook_sources: plugin_hooks.hook_sources,
|
||||
plugin_hook_load_warnings: plugin_hooks.hook_load_warnings,
|
||||
..Default::default()
|
||||
});
|
||||
data.push(codex_app_server_protocol::HooksListEntry {
|
||||
|
||||
@@ -26,6 +26,7 @@ pub type PluginLoadOutcome = codex_plugin::PluginLoadOutcome<codex_config::McpSe
|
||||
|
||||
pub use discoverable::ToolSuggestDiscoverablePlugin;
|
||||
pub use discoverable::ToolSuggestPluginDiscoveryInput;
|
||||
pub use loader::PluginHookLoadOutcome;
|
||||
pub use manager::ConfiguredMarketplace;
|
||||
pub use manager::ConfiguredMarketplaceListOutcome;
|
||||
pub use manager::ConfiguredMarketplacePlugin;
|
||||
|
||||
@@ -53,6 +53,21 @@ const DEFAULT_APP_CONFIG_FILE: &str = ".app.json";
|
||||
const CONFIG_TOML_FILE: &str = "config.toml";
|
||||
const CURATED_PLUGIN_CACHE_VERSION_SHA_PREFIX_LEN: usize = 8;
|
||||
|
||||
/// Hook declarations and warnings resolved without loading other plugin capabilities.
|
||||
#[derive(Debug, Clone, Default, PartialEq, Eq)]
|
||||
pub struct PluginHookLoadOutcome {
|
||||
pub hook_sources: Vec<PluginHookSource>,
|
||||
pub hook_load_warnings: Vec<String>,
|
||||
}
|
||||
|
||||
enum PluginLoadScope<'a> {
|
||||
AllCapabilities {
|
||||
restriction_product: Option<Product>,
|
||||
skill_config_rules: &'a SkillConfigRules,
|
||||
},
|
||||
HooksOnly,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, PartialEq, Eq)]
|
||||
enum NonCuratedCacheRefreshMode {
|
||||
IfVersionChanged,
|
||||
@@ -117,6 +132,26 @@ pub async fn load_plugins_from_layer_stack(
|
||||
prefer_remote_curated_conflicts: bool,
|
||||
) -> PluginLoadOutcome<McpServerConfig> {
|
||||
let skill_config_rules = skill_config_rules_from_stack(config_layer_stack);
|
||||
load_plugins_from_layer_stack_with_scope(
|
||||
config_layer_stack,
|
||||
extra_plugins,
|
||||
store,
|
||||
prefer_remote_curated_conflicts,
|
||||
PluginLoadScope::AllCapabilities {
|
||||
restriction_product,
|
||||
skill_config_rules: &skill_config_rules,
|
||||
},
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn load_plugins_from_layer_stack_with_scope(
|
||||
config_layer_stack: &ConfigLayerStack,
|
||||
extra_plugins: HashMap<String, PluginConfig>,
|
||||
store: &PluginStore,
|
||||
prefer_remote_curated_conflicts: bool,
|
||||
scope: PluginLoadScope<'_>,
|
||||
) -> PluginLoadOutcome<McpServerConfig> {
|
||||
let configured_plugins = merge_configured_plugins_with_remote_installed(
|
||||
configured_plugins_from_stack(config_layer_stack),
|
||||
extra_plugins,
|
||||
@@ -129,14 +164,7 @@ pub async fn load_plugins_from_layer_stack(
|
||||
let mut plugins = Vec::with_capacity(configured_plugins.len());
|
||||
let mut seen_mcp_server_names = HashMap::<String, String>::new();
|
||||
for (configured_name, plugin) in configured_plugins {
|
||||
let loaded_plugin = load_plugin(
|
||||
configured_name.clone(),
|
||||
&plugin,
|
||||
store,
|
||||
restriction_product,
|
||||
&skill_config_rules,
|
||||
)
|
||||
.await;
|
||||
let loaded_plugin = load_plugin(configured_name.clone(), &plugin, store, &scope).await;
|
||||
for name in loaded_plugin.mcp_servers.keys() {
|
||||
if let Some(previous_plugin) =
|
||||
seen_mcp_server_names.insert(name.clone(), configured_name.clone())
|
||||
@@ -155,6 +183,27 @@ pub async fn load_plugins_from_layer_stack(
|
||||
PluginLoadOutcome::from_plugins(plugins)
|
||||
}
|
||||
|
||||
/// Load hooks from enabled plugins without loading their skills, MCP servers, or apps.
|
||||
pub async fn load_plugin_hooks_from_layer_stack(
|
||||
config_layer_stack: &ConfigLayerStack,
|
||||
extra_plugins: HashMap<String, PluginConfig>,
|
||||
store: &PluginStore,
|
||||
prefer_remote_curated_conflicts: bool,
|
||||
) -> PluginHookLoadOutcome {
|
||||
let outcome = load_plugins_from_layer_stack_with_scope(
|
||||
config_layer_stack,
|
||||
extra_plugins,
|
||||
store,
|
||||
prefer_remote_curated_conflicts,
|
||||
PluginLoadScope::HooksOnly,
|
||||
)
|
||||
.await;
|
||||
PluginHookLoadOutcome {
|
||||
hook_sources: outcome.effective_plugin_hook_sources(),
|
||||
hook_load_warnings: outcome.effective_plugin_hook_warnings(),
|
||||
}
|
||||
}
|
||||
|
||||
fn merge_configured_plugins_with_remote_installed(
|
||||
mut configured_plugins: HashMap<String, PluginConfig>,
|
||||
extra_plugins: HashMap<String, PluginConfig>,
|
||||
@@ -557,8 +606,7 @@ async fn load_plugin(
|
||||
config_name: String,
|
||||
plugin: &PluginConfig,
|
||||
store: &PluginStore,
|
||||
restriction_product: Option<Product>,
|
||||
skill_config_rules: &SkillConfigRules,
|
||||
scope: &PluginLoadScope<'_>,
|
||||
) -> LoadedPlugin<McpServerConfig> {
|
||||
let plugin_id = PluginId::parse(&config_name);
|
||||
let active_plugin_root = plugin_id
|
||||
@@ -616,46 +664,55 @@ async fn load_plugin(
|
||||
};
|
||||
|
||||
let manifest_paths = &manifest.paths;
|
||||
loaded_plugin.manifest_name = manifest
|
||||
.interface
|
||||
.as_ref()
|
||||
.and_then(|interface| interface.display_name.as_deref())
|
||||
.map(str::trim)
|
||||
.filter(|display_name| !display_name.is_empty())
|
||||
.map(str::to_string)
|
||||
.or_else(|| Some(manifest.name.clone()));
|
||||
loaded_plugin.manifest_description = manifest.description.clone();
|
||||
loaded_plugin.skill_roots = plugin_skill_roots(&plugin_root, manifest_paths);
|
||||
let resolved_skills = load_plugin_skills(
|
||||
&plugin_root,
|
||||
&loaded_plugin_id,
|
||||
manifest_paths,
|
||||
restriction_product,
|
||||
skill_config_rules,
|
||||
)
|
||||
.await;
|
||||
let has_enabled_skills = resolved_skills.has_enabled_skills();
|
||||
loaded_plugin.disabled_skill_paths = resolved_skills.disabled_skill_paths;
|
||||
loaded_plugin.has_enabled_skills = has_enabled_skills;
|
||||
let mut mcp_servers = HashMap::new();
|
||||
for mcp_config_path in plugin_mcp_config_paths(plugin_root.as_path(), manifest_paths) {
|
||||
let plugin_mcp = load_mcp_servers_from_file(plugin_root.as_path(), &mcp_config_path).await;
|
||||
for (name, mut config) in plugin_mcp.mcp_servers {
|
||||
if let Some(policy) = plugin.mcp_servers.get(&name) {
|
||||
apply_plugin_mcp_server_policy(&mut config, policy);
|
||||
}
|
||||
if mcp_servers.insert(name.clone(), config).is_some() {
|
||||
warn!(
|
||||
plugin = %plugin_root.display(),
|
||||
path = %mcp_config_path.display(),
|
||||
server = name,
|
||||
"plugin MCP file overwrote an earlier server definition"
|
||||
);
|
||||
match scope {
|
||||
PluginLoadScope::AllCapabilities {
|
||||
restriction_product,
|
||||
skill_config_rules,
|
||||
} => {
|
||||
loaded_plugin.manifest_name = manifest
|
||||
.interface
|
||||
.as_ref()
|
||||
.and_then(|interface| interface.display_name.as_deref())
|
||||
.map(str::trim)
|
||||
.filter(|display_name| !display_name.is_empty())
|
||||
.map(str::to_string)
|
||||
.or_else(|| Some(manifest.name.clone()));
|
||||
loaded_plugin.manifest_description = manifest.description.clone();
|
||||
loaded_plugin.skill_roots = plugin_skill_roots(&plugin_root, manifest_paths);
|
||||
let resolved_skills = load_plugin_skills(
|
||||
&plugin_root,
|
||||
&loaded_plugin_id,
|
||||
manifest_paths,
|
||||
*restriction_product,
|
||||
skill_config_rules,
|
||||
)
|
||||
.await;
|
||||
let has_enabled_skills = resolved_skills.has_enabled_skills();
|
||||
loaded_plugin.disabled_skill_paths = resolved_skills.disabled_skill_paths;
|
||||
loaded_plugin.has_enabled_skills = has_enabled_skills;
|
||||
let mut mcp_servers = HashMap::new();
|
||||
for mcp_config_path in plugin_mcp_config_paths(plugin_root.as_path(), manifest_paths) {
|
||||
let plugin_mcp =
|
||||
load_mcp_servers_from_file(plugin_root.as_path(), &mcp_config_path).await;
|
||||
for (name, mut config) in plugin_mcp.mcp_servers {
|
||||
if let Some(policy) = plugin.mcp_servers.get(&name) {
|
||||
apply_plugin_mcp_server_policy(&mut config, policy);
|
||||
}
|
||||
if mcp_servers.insert(name.clone(), config).is_some() {
|
||||
warn!(
|
||||
plugin = %plugin_root.display(),
|
||||
path = %mcp_config_path.display(),
|
||||
server = name,
|
||||
"plugin MCP file overwrote an earlier server definition"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
loaded_plugin.mcp_servers = mcp_servers;
|
||||
loaded_plugin.apps = load_plugin_apps(plugin_root.as_path()).await;
|
||||
}
|
||||
PluginLoadScope::HooksOnly => {}
|
||||
}
|
||||
loaded_plugin.mcp_servers = mcp_servers;
|
||||
loaded_plugin.apps = load_plugin_apps(plugin_root.as_path()).await;
|
||||
let (hook_sources, hook_load_warnings) = load_plugin_hooks(
|
||||
&plugin_root,
|
||||
&loaded_plugin_id,
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use super::*;
|
||||
use crate::manifest::load_plugin_manifest;
|
||||
use crate::test_support::write_file;
|
||||
use codex_config::ConfigLayerEntry;
|
||||
use codex_config::ConfigLayerSource;
|
||||
use codex_config::ConfigRequirements;
|
||||
@@ -65,6 +66,158 @@ fn configured_plugins_from_stack_merges_user_layers() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn hooks_only_scope_shares_plugin_resolution_without_loading_other_capabilities() {
|
||||
let temp_dir = TempDir::new().expect("tempdir");
|
||||
let plugin_root = temp_dir.path().join("plugins/cache/test/valid/local");
|
||||
write_file(
|
||||
&plugin_root.join(".codex-plugin/plugin.json"),
|
||||
r#"{"name":"valid"}"#,
|
||||
);
|
||||
write_file(
|
||||
&plugin_root.join("skills/example/SKILL.md"),
|
||||
"---\nname: example\ndescription: example skill\n---\n",
|
||||
);
|
||||
write_file(
|
||||
&plugin_root.join(".mcp.json"),
|
||||
r#"{"mcpServers":{"example":{"command":"echo"}}}"#,
|
||||
);
|
||||
write_file(
|
||||
&plugin_root.join(".app.json"),
|
||||
r#"{"apps":{"example":{"id":"connector_example"}}}"#,
|
||||
);
|
||||
write_file(
|
||||
&plugin_root.join("hooks/hooks.json"),
|
||||
r#"{
|
||||
"hooks": {
|
||||
"SessionStart": [
|
||||
{
|
||||
"hooks": [
|
||||
{
|
||||
"type": "command",
|
||||
"command": "echo startup"
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
}"#,
|
||||
);
|
||||
|
||||
let disabled_root = temp_dir.path().join("plugins/cache/test/disabled/local");
|
||||
write_file(
|
||||
&disabled_root.join(".codex-plugin/plugin.json"),
|
||||
r#"{"name":"disabled"}"#,
|
||||
);
|
||||
write_file(
|
||||
&disabled_root.join("hooks/hooks.json"),
|
||||
r#"{"hooks":{"SessionStart":[{"hooks":[{"type":"command","command":"echo disabled"}]}]}}"#,
|
||||
);
|
||||
|
||||
let malformed_root = temp_dir.path().join("plugins/cache/test/malformed/local");
|
||||
write_file(
|
||||
&malformed_root.join(".codex-plugin/plugin.json"),
|
||||
"not valid json",
|
||||
);
|
||||
|
||||
let warning_root = temp_dir.path().join("plugins/cache/test/warning/local");
|
||||
write_file(
|
||||
&warning_root.join(".codex-plugin/plugin.json"),
|
||||
r#"{"name":"warning"}"#,
|
||||
);
|
||||
write_file(&warning_root.join("hooks/hooks.json"), "not valid json");
|
||||
|
||||
let stack = ConfigLayerStack::new(
|
||||
vec![user_layer(
|
||||
user_config_path(&temp_dir, "config.toml"),
|
||||
r#"
|
||||
[plugins."valid@test"]
|
||||
enabled = true
|
||||
|
||||
[plugins."disabled@test"]
|
||||
enabled = false
|
||||
|
||||
[plugins.invalid]
|
||||
enabled = true
|
||||
|
||||
[plugins."malformed@test"]
|
||||
enabled = true
|
||||
|
||||
[plugins."missing@test"]
|
||||
enabled = true
|
||||
|
||||
[plugins."warning@test"]
|
||||
enabled = true
|
||||
"#,
|
||||
)],
|
||||
ConfigRequirements::default(),
|
||||
ConfigRequirementsToml::default(),
|
||||
)
|
||||
.expect("valid config layer stack");
|
||||
let store = PluginStore::new(temp_dir.path().to_path_buf());
|
||||
|
||||
let full = load_plugins_from_layer_stack(
|
||||
&stack,
|
||||
HashMap::new(),
|
||||
&store,
|
||||
Some(Product::Codex),
|
||||
/*prefer_remote_curated_conflicts*/ false,
|
||||
)
|
||||
.await;
|
||||
let hooks_only = load_plugins_from_layer_stack_with_scope(
|
||||
&stack,
|
||||
HashMap::new(),
|
||||
&store,
|
||||
/*prefer_remote_curated_conflicts*/ false,
|
||||
PluginLoadScope::HooksOnly,
|
||||
)
|
||||
.await;
|
||||
|
||||
let validation_state = |outcome: &PluginLoadOutcome<McpServerConfig>| {
|
||||
outcome
|
||||
.plugins()
|
||||
.iter()
|
||||
.map(|plugin| {
|
||||
(
|
||||
plugin.config_name.clone(),
|
||||
plugin.enabled,
|
||||
plugin.root.clone(),
|
||||
plugin.error.clone(),
|
||||
)
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
};
|
||||
assert_eq!(validation_state(&hooks_only), validation_state(&full));
|
||||
assert_eq!(
|
||||
hooks_only.effective_plugin_hook_sources(),
|
||||
full.effective_plugin_hook_sources()
|
||||
);
|
||||
assert_eq!(
|
||||
hooks_only.effective_plugin_hook_warnings(),
|
||||
full.effective_plugin_hook_warnings()
|
||||
);
|
||||
|
||||
let full_valid = full
|
||||
.plugins()
|
||||
.iter()
|
||||
.find(|plugin| plugin.config_name == "valid@test")
|
||||
.expect("full load should include valid plugin");
|
||||
assert!(full_valid.manifest_name.is_some());
|
||||
assert!(!full_valid.skill_roots.is_empty());
|
||||
assert!(!full_valid.mcp_servers.is_empty());
|
||||
assert!(!full_valid.apps.is_empty());
|
||||
|
||||
let hooks_only_valid = hooks_only
|
||||
.plugins()
|
||||
.iter()
|
||||
.find(|plugin| plugin.config_name == "valid@test")
|
||||
.expect("hooks-only load should include valid plugin");
|
||||
assert_eq!(hooks_only_valid.manifest_name, None);
|
||||
assert!(hooks_only_valid.skill_roots.is_empty());
|
||||
assert!(hooks_only_valid.mcp_servers.is_empty());
|
||||
assert!(hooks_only_valid.apps.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn plugin_mcp_file_supports_mcp_servers_object_format() {
|
||||
let parsed = serde_json::from_str::<PluginMcpFile>(
|
||||
|
||||
@@ -2,11 +2,13 @@ use super::PluginLoadOutcome;
|
||||
use super::startup_remote_sync::start_startup_remote_plugin_sync_once;
|
||||
use crate::OPENAI_CURATED_MARKETPLACE_NAME;
|
||||
use crate::installed_marketplaces::installed_marketplace_roots_from_layer_stack;
|
||||
use crate::loader::PluginHookLoadOutcome;
|
||||
use crate::loader::configured_curated_plugin_ids_from_codex_home;
|
||||
use crate::loader::curated_plugin_cache_version;
|
||||
use crate::loader::installed_plugin_telemetry_metadata;
|
||||
use crate::loader::load_plugin_apps;
|
||||
use crate::loader::load_plugin_hooks;
|
||||
use crate::loader::load_plugin_hooks_from_layer_stack;
|
||||
use crate::loader::load_plugin_mcp_servers;
|
||||
use crate::loader::load_plugin_skills;
|
||||
use crate::loader::load_plugins_from_layer_stack;
|
||||
@@ -543,6 +545,24 @@ impl PluginsManager {
|
||||
.await
|
||||
}
|
||||
|
||||
/// Resolve plugin hooks for a config layer stack without loading other plugin capabilities.
|
||||
pub async fn plugin_hooks_for_layer_stack(
|
||||
&self,
|
||||
config_layer_stack: &ConfigLayerStack,
|
||||
config: &PluginsConfigInput,
|
||||
) -> PluginHookLoadOutcome {
|
||||
if !config.plugins_enabled {
|
||||
return PluginHookLoadOutcome::default();
|
||||
}
|
||||
load_plugin_hooks_from_layer_stack(
|
||||
config_layer_stack,
|
||||
self.remote_installed_plugin_configs(),
|
||||
&self.store,
|
||||
config.remote_plugin_enabled,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
/// Resolve plugin skill roots for a config layer stack without touching the plugins cache.
|
||||
pub async fn effective_skill_roots_for_layer_stack(
|
||||
&self,
|
||||
|
||||
@@ -3858,3 +3858,50 @@ async fn load_plugins_ignores_project_config_files() {
|
||||
|
||||
assert_eq!(outcome, PluginLoadOutcome::default());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn plugin_hooks_for_layer_stack_loads_configured_plugin_hooks() {
|
||||
let codex_home = TempDir::new().unwrap();
|
||||
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",
|
||||
);
|
||||
write_file(
|
||||
&plugin_root.join("hooks/hooks.json"),
|
||||
r#"{
|
||||
"hooks": {
|
||||
"SessionStart": [
|
||||
{
|
||||
"hooks": [
|
||||
{
|
||||
"type": "command",
|
||||
"command": "echo startup"
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
}"#,
|
||||
);
|
||||
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 outcome = PluginsManager::new(codex_home.path().to_path_buf())
|
||||
.plugin_hooks_for_layer_stack(&config.config_layer_stack, &config)
|
||||
.await;
|
||||
|
||||
assert_eq!(outcome.hook_sources.len(), 1);
|
||||
assert_eq!(
|
||||
outcome.hook_sources[0].source_relative_path,
|
||||
"hooks/hooks.json"
|
||||
);
|
||||
assert_eq!(outcome.hook_load_warnings, Vec::<String>::new());
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user