From 24faf49b2a70f8813e522afb0e701add9b15b0bd Mon Sep 17 00:00:00 2001 From: Abhinav Date: Thu, 21 May 2026 12:15:18 -0700 Subject: [PATCH] Remove plugin hooks feature flag (#22552) # Why This is a follow-up stacked on top of the `plugin_hooks` default-on change. Once we are comfortable making plugin hooks part of the normal plugin behavior, the separate feature flag stops buying us much and leaves extra branching/cache state behind. # What - remove the `PluginHooks` feature and generated config-schema entries - make plugin hook loading/listing follow plugin enablement directly - drop plugin-manager cache/state that only existed to distinguish hook-flag toggles - remove tests and fixtures that modeled `plugin_hooks = true/false` --- .../request_processors/catalog_processor.rs | 9 +-- .../app-server/tests/suite/v2/hooks_list.rs | 1 - .../app-server/tests/suite/v2/plugin_read.rs | 1 - codex-rs/core-plugins/src/loader.rs | 21 ++---- codex-rs/core-plugins/src/manager.rs | 56 ++++---------- codex-rs/core-plugins/src/manager_tests.rs | 73 ------------------- codex-rs/core-plugins/src/test_support.rs | 5 -- codex-rs/core/src/config/mod.rs | 1 - codex-rs/core/src/session/mod.rs | 15 +--- codex-rs/core/tests/suite/hooks.rs | 4 - codex-rs/features/src/lib.rs | 9 ++- codex-rs/features/src/tests.rs | 29 ++++++-- 12 files changed, 58 insertions(+), 166 deletions(-) diff --git a/codex-rs/app-server/src/request_processors/catalog_processor.rs b/codex-rs/app-server/src/request_processors/catalog_processor.rs index d2845d7d6..ba6c53566 100644 --- a/codex-rs/app-server/src/request_processors/catalog_processor.rs +++ b/codex-rs/app-server/src/request_processors/catalog_processor.rs @@ -612,15 +612,10 @@ impl CatalogRequestProcessor { .await; let plugins_enabled = config.features.enabled(Feature::Plugins) && workspace_codex_plugins_enabled; - let plugin_outcome = if plugins_enabled && config.features.enabled(Feature::PluginHooks) - { + let plugin_outcome = if plugins_enabled { let plugins_input = config.plugins_config_input(); plugins_manager - .plugins_for_layer_stack( - &config.config_layer_stack, - &plugins_input, - /*plugin_hooks_feature_enabled*/ true, - ) + .plugins_for_layer_stack(&config.config_layer_stack, &plugins_input) .await } else { PluginLoadOutcome::default() diff --git a/codex-rs/app-server/tests/suite/v2/hooks_list.rs b/codex-rs/app-server/tests/suite/v2/hooks_list.rs index 15675257f..1c8f76fe5 100644 --- a/codex-rs/app-server/tests/suite/v2/hooks_list.rs +++ b/codex-rs/app-server/tests/suite/v2/hooks_list.rs @@ -97,7 +97,6 @@ fn write_plugin_hook_config(codex_home: &std::path::Path, hooks_json: &str) -> R codex_home.join("config.toml"), r#"[features] plugins = true -plugin_hooks = true hooks = true [plugins."demo@test"] diff --git a/codex-rs/app-server/tests/suite/v2/plugin_read.rs b/codex-rs/app-server/tests/suite/v2/plugin_read.rs index 3ec2366f5..2a5d3d887 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_read.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_read.rs @@ -1273,7 +1273,6 @@ description: Visible only for ChatGPT codex_home.path().join("config.toml"), r#"[features] plugins = true -plugin_hooks = true [[skills.config]] name = "demo-plugin:thread-summarizer" diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index fc4193aa6..be6c8aca6 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -112,7 +112,6 @@ pub async fn load_plugins_from_layer_stack( extra_plugins: HashMap, store: &PluginStore, restriction_product: Option, - plugin_hooks_enabled: bool, ) -> PluginLoadOutcome { let skill_config_rules = skill_config_rules_from_stack(config_layer_stack); let mut configured_plugins = configured_plugins_from_stack(config_layer_stack); @@ -129,7 +128,6 @@ pub async fn load_plugins_from_layer_stack( store, restriction_product, &skill_config_rules, - plugin_hooks_enabled, ) .await; for name in loaded_plugin.mcp_servers.keys() { @@ -499,7 +497,6 @@ async fn load_plugin( store: &PluginStore, restriction_product: Option, skill_config_rules: &SkillConfigRules, - plugin_hooks_enabled: bool, ) -> LoadedPlugin { let plugin_id = PluginId::parse(&config_name); let active_plugin_root = plugin_id @@ -597,16 +594,14 @@ async fn load_plugin( } loaded_plugin.mcp_servers = mcp_servers; loaded_plugin.apps = load_plugin_apps(plugin_root.as_path()).await; - if plugin_hooks_enabled { - let (hook_sources, hook_load_warnings) = load_plugin_hooks( - &plugin_root, - &loaded_plugin_id, - &store.plugin_data_root(&loaded_plugin_id), - manifest_paths, - ); - loaded_plugin.hook_sources = hook_sources; - loaded_plugin.hook_load_warnings = hook_load_warnings; - } + let (hook_sources, hook_load_warnings) = load_plugin_hooks( + &plugin_root, + &loaded_plugin_id, + &store.plugin_data_root(&loaded_plugin_id), + manifest_paths, + ); + loaded_plugin.hook_sources = hook_sources; + loaded_plugin.hook_load_warnings = hook_load_warnings; loaded_plugin } diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index b2495a12c..ec3e74dd1 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -90,7 +90,6 @@ pub struct PluginsConfigInput { pub config_layer_stack: ConfigLayerStack, pub plugins_enabled: bool, pub remote_plugin_enabled: bool, - pub plugin_hooks_enabled: bool, pub chatgpt_base_url: String, } @@ -99,14 +98,12 @@ impl PluginsConfigInput { config_layer_stack: ConfigLayerStack, plugins_enabled: bool, remote_plugin_enabled: bool, - plugin_hooks_enabled: bool, chatgpt_base_url: String, ) -> Self { Self { config_layer_stack, plugins_enabled, remote_plugin_enabled, - plugin_hooks_enabled, chatgpt_base_url, } } @@ -415,7 +412,6 @@ pub struct PluginsManager { #[derive(Clone)] struct CachedPluginLoadOutcome { config_version: String, - plugin_hooks_enabled: bool, outcome: PluginLoadOutcome, } @@ -486,12 +482,8 @@ impl PluginsManager { return PluginLoadOutcome::default(); } - let plugin_hooks_enabled = config.plugin_hooks_enabled; let config_version = version_for_toml(&config.config_layer_stack.effective_config()); - if !force_reload - && let Some(outcome) = - self.cached_enabled_outcome(&config_version, plugin_hooks_enabled) - { + if !force_reload && let Some(outcome) = self.cached_enabled_outcome(&config_version) { return outcome; } @@ -500,7 +492,6 @@ impl PluginsManager { self.remote_installed_plugin_configs(), &self.store, self.restriction_product, - plugin_hooks_enabled, ) .await; log_plugin_load_errors(&outcome); @@ -510,7 +501,6 @@ impl PluginsManager { }; *cache = Some(CachedPluginLoadOutcome { config_version, - plugin_hooks_enabled, outcome: outcome.clone(), }); outcome @@ -538,7 +528,6 @@ impl PluginsManager { &self, config_layer_stack: &ConfigLayerStack, config: &PluginsConfigInput, - plugin_hooks_feature_enabled: bool, ) -> PluginLoadOutcome { if !config.plugins_enabled { return PluginLoadOutcome::default(); @@ -548,7 +537,6 @@ impl PluginsManager { self.remote_installed_plugin_configs(), &self.store, self.restriction_product, - plugin_hooks_feature_enabled, ) .await } @@ -559,31 +547,21 @@ impl PluginsManager { config_layer_stack: &ConfigLayerStack, config: &PluginsConfigInput, ) -> Vec { - self.plugins_for_layer_stack(config_layer_stack, config, config.plugin_hooks_enabled) + self.plugins_for_layer_stack(config_layer_stack, config) .await .effective_plugin_skill_roots() } - fn cached_enabled_outcome( - &self, - config_version: &str, - plugin_hooks_enabled: bool, - ) -> Option { + fn cached_enabled_outcome(&self, config_version: &str) -> Option { match self.cached_enabled_outcome.read() { Ok(cache) => cache .as_ref() - .filter(|cached| { - cached.config_version == config_version - && cached.plugin_hooks_enabled == plugin_hooks_enabled - }) + .filter(|cached| cached.config_version == config_version) .map(|cached| cached.outcome.clone()), Err(err) => err .into_inner() .as_ref() - .filter(|cached| { - cached.config_version == config_version - && cached.plugin_hooks_enabled == plugin_hooks_enabled - }) + .filter(|cached| cached.config_version == config_version) .map(|cached| cached.outcome.clone()), } } @@ -1451,20 +1429,16 @@ impl PluginsManager { ), ) .await; - let hooks = if config.plugin_hooks_enabled { - let plugin_data_root = self.store.plugin_data_root(&plugin_id); - let (hook_sources, _hook_load_warnings) = - load_plugin_hooks(&source_path, &plugin_id, &plugin_data_root, &manifest.paths); - plugin_hook_declarations(&hook_sources) - .into_iter() - .map(|hook| PluginHookSummary { - key: hook.key, - event_name: hook.event_name, - }) - .collect() - } else { - Vec::new() - }; + let plugin_data_root = self.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) + .into_iter() + .map(|hook| PluginHookSummary { + key: hook.key, + event_name: hook.event_name, + }) + .collect(); let apps = load_plugin_apps(source_path.as_path()).await; let mut mcp_server_names = load_plugin_mcp_servers(source_path.as_path()) .await diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 71495d1e4..0131ef22e 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -101,18 +101,6 @@ fn run_git(repo: &Path, args: &[&str]) { } fn plugin_config_toml(enabled: bool, plugins_feature_enabled: bool) -> String { - plugin_config_toml_with_plugin_hooks( - enabled, - plugins_feature_enabled, - /*plugin_hooks_feature_enabled*/ false, - ) -} - -fn plugin_config_toml_with_plugin_hooks( - enabled: bool, - plugins_feature_enabled: bool, - plugin_hooks_feature_enabled: bool, -) -> String { let mut root = toml::map::Map::new(); let mut features = toml::map::Map::new(); @@ -120,10 +108,6 @@ fn plugin_config_toml_with_plugin_hooks( "plugins".to_string(), Value::Boolean(plugins_feature_enabled), ); - features.insert( - "plugin_hooks".to_string(), - Value::Boolean(plugin_hooks_feature_enabled), - ); root.insert("features".to_string(), Value::Table(features)); let mut plugin = toml::map::Map::new(); @@ -1133,61 +1117,6 @@ async fn load_plugins_returns_empty_when_feature_disabled() { assert_eq!(outcome, PluginLoadOutcome::default()); } -#[tokio::test] -async fn plugins_for_config_reloads_when_plugin_hooks_enablement_changes() { - let codex_home = TempDir::new().unwrap(); - let plugin_root = codex_home - .path() - .join("plugins/cache") - .join("test/sample/local"); - - write_file( - &plugin_root.join(".codex-plugin/plugin.json"), - r#"{"name":"sample"}"#, - ); - write_file( - &plugin_root.join("hooks/hooks.json"), - r#"{ - "hooks": { - "PreToolUse": [ - { - "hooks": [{ "type": "command", "command": "echo plugin hook" }] - } - ] - } -}"#, - ); - - let manager = PluginsManager::new(codex_home.path().to_path_buf()); - write_file( - &codex_home.path().join(CONFIG_TOML_FILE), - &plugin_config_toml_with_plugin_hooks( - /*enabled*/ true, /*plugins_feature_enabled*/ true, - /*plugin_hooks_feature_enabled*/ false, - ), - ); - let config_without_plugin_hooks = load_config(codex_home.path(), codex_home.path()).await; - let without_plugin_hooks = manager - .plugins_for_config(&config_without_plugin_hooks) - .await; - assert!( - without_plugin_hooks - .effective_plugin_hook_sources() - .is_empty() - ); - - write_file( - &codex_home.path().join(CONFIG_TOML_FILE), - &plugin_config_toml_with_plugin_hooks( - /*enabled*/ true, /*plugins_feature_enabled*/ true, - /*plugin_hooks_feature_enabled*/ true, - ), - ); - let config_with_plugin_hooks = load_config(codex_home.path(), codex_home.path()).await; - let with_plugin_hooks = manager.plugins_for_config(&config_with_plugin_hooks).await; - assert_eq!(with_plugin_hooks.effective_plugin_hook_sources().len(), 1); -} - #[tokio::test] async fn load_plugins_rejects_invalid_plugin_keys() { let codex_home = TempDir::new().unwrap(); @@ -2032,7 +1961,6 @@ async fn read_plugin_for_config_installed_git_source_reads_from_cache_without_cl &tmp.path().join(CONFIG_TOML_FILE), r#"[features] plugins = true -plugin_hooks = true [plugins."toolkit@debug"] enabled = true @@ -3741,7 +3669,6 @@ async fn load_plugins_ignores_project_config_files() { std::collections::HashMap::new(), &PluginStore::new(codex_home.path().to_path_buf()), Some(Product::Codex), - /*plugin_hooks_enabled*/ false, ) .await; diff --git a/codex-rs/core-plugins/src/test_support.rs b/codex-rs/core-plugins/src/test_support.rs index 6be2fbf0d..9e37a1431 100644 --- a/codex-rs/core-plugins/src/test_support.rs +++ b/codex-rs/core-plugins/src/test_support.rs @@ -120,11 +120,6 @@ pub(crate) async fn load_plugins_config(codex_home: &Path, cwd: &Path) -> Plugin "remote_plugin", /*default_enabled*/ false, ), - feature_enabled( - &effective_config, - "plugin_hooks", - /*default_enabled*/ false, - ), "https://chatgpt.com/backend-api/".to_string(), ) } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 75bcf5aa3..2392af0e5 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1282,7 +1282,6 @@ impl Config { self.config_layer_stack.clone(), self.features.enabled(Feature::Plugins), self.features.enabled(Feature::RemotePlugin), - self.features.enabled(Feature::PluginHooks), self.chatgpt_base_url.clone(), ) } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index c73b43a83..675e794ae 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -3319,17 +3319,10 @@ async fn build_hooks_for_config( let mut hook_shell_argv = user_shell.derive_exec_args("", /*use_login_shell*/ false); let hook_shell_program = hook_shell_argv.remove(0); let _ = hook_shell_argv.pop(); - let plugin_hooks_enabled = config.features.enabled(Feature::PluginHooks); - let (plugin_hook_sources, plugin_hook_load_warnings) = if plugin_hooks_enabled { - let plugins_input = config.plugins_config_input(); - let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await; - ( - plugin_outcome.effective_plugin_hook_sources(), - plugin_outcome.effective_plugin_hook_warnings(), - ) - } else { - (Vec::new(), Vec::new()) - }; + let plugins_input = config.plugins_config_input(); + let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await; + let plugin_hook_sources = plugin_outcome.effective_plugin_hook_sources(); + let plugin_hook_load_warnings = plugin_outcome.effective_plugin_hook_warnings(); Hooks::new(HooksConfig { legacy_notify_argv: config.notify.clone(), feature_enabled: config.features.enabled(Feature::CodexHooks), diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index e9067b8a4..ee1826ceb 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -2934,10 +2934,6 @@ print(json.dumps({{ .enable(Feature::Plugins) .expect("test config should allow feature update"); trust_plugin_hooks(config, plugin_hook_sources); - config - .features - .enable(Feature::PluginHooks) - .expect("test config should allow feature update"); }); let test = builder.build(&server).await?; diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 662d01fe2..4db7b56d2 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -140,7 +140,7 @@ pub enum Feature { ToolSuggest, /// Enable plugins. Plugins, - /// Enable plugin-bundled lifecycle hooks. + /// Removed compatibility flag for plugin-bundled lifecycle hooks. PluginHooks, /// Allow the in-app browser pane in desktop apps. /// @@ -434,6 +434,9 @@ impl Features { "image_detail_original" => { continue; } + "plugin_hooks" => { + continue; + } "skill_env_var_dependency_prompt" => { continue; } @@ -979,8 +982,8 @@ pub const FEATURES: &[FeatureSpec] = &[ FeatureSpec { id: Feature::PluginHooks, key: "plugin_hooks", - stage: Stage::Stable, - default_enabled: true, + stage: Stage::Removed, + default_enabled: false, }, FeatureSpec { id: Feature::InAppBrowser, diff --git a/codex-rs/features/src/tests.rs b/codex-rs/features/src/tests.rs index e2a456f1b..e1f48745f 100644 --- a/codex-rs/features/src/tests.rs +++ b/codex-rs/features/src/tests.rs @@ -76,6 +76,13 @@ fn apply_patch_freeform_is_removed_and_disabled_by_default() { ); } +#[test] +fn plugin_hooks_is_removed_and_disabled_by_default() { + assert_eq!(Feature::PluginHooks.stage(), Stage::Removed); + assert_eq!(Feature::PluginHooks.default_enabled(), false); + assert_eq!(feature_for_key("plugin_hooks"), Some(Feature::PluginHooks)); +} + #[test] fn code_mode_only_requires_code_mode() { let mut features = Features::with_defaults(); @@ -194,12 +201,6 @@ fn tool_search_is_removed_and_disabled_by_default() { assert_eq!(feature_for_key("tool_search"), Some(Feature::ToolSearch)); } -#[test] -fn plugin_hooks_are_stable_and_enabled_by_default() { - assert_eq!(Feature::PluginHooks.stage(), Stage::Stable); - assert_eq!(Feature::PluginHooks.default_enabled(), true); -} - #[test] fn browser_controls_are_stable_and_enabled_by_default() { assert_eq!(Feature::InAppBrowser.stage(), Stage::Stable); @@ -499,6 +500,22 @@ fn from_sources_ignores_removed_apply_patch_freeform_feature_key() { assert_eq!(features, Features::with_defaults()); } +#[test] +fn from_sources_ignores_removed_plugin_hooks_feature_key() { + let features_toml = FeaturesToml::from(BTreeMap::from([("plugin_hooks".to_string(), true)])); + + let features = Features::from_sources( + FeatureConfigSource { + features: Some(&features_toml), + ..Default::default() + }, + FeatureConfigSource::default(), + FeatureOverrides::default(), + ); + + assert_eq!(features, Features::with_defaults()); +} + #[test] fn multi_agent_v2_feature_config_deserializes_boolean_toggle() { let features: FeaturesToml = toml::from_str(