mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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`
This commit is contained in:
committed by
GitHub
Unverified
parent
ac0bff27e7
commit
24faf49b2a
@@ -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()
|
||||
|
||||
@@ -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"]
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -112,7 +112,6 @@ pub async fn load_plugins_from_layer_stack(
|
||||
extra_plugins: HashMap<String, PluginConfig>,
|
||||
store: &PluginStore,
|
||||
restriction_product: Option<Product>,
|
||||
plugin_hooks_enabled: bool,
|
||||
) -> PluginLoadOutcome<McpServerConfig> {
|
||||
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<Product>,
|
||||
skill_config_rules: &SkillConfigRules,
|
||||
plugin_hooks_enabled: bool,
|
||||
) -> LoadedPlugin<McpServerConfig> {
|
||||
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
|
||||
}
|
||||
|
||||
|
||||
@@ -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<PluginSkillRoot> {
|
||||
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<PluginLoadOutcome> {
|
||||
fn cached_enabled_outcome(&self, config_version: &str) -> Option<PluginLoadOutcome> {
|
||||
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
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -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(),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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(),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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?;
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user