From 9689bd16f7e6e3ceae90ef73abf066d4806271a4 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Mon, 18 May 2026 00:35:18 -0700 Subject: [PATCH] codex: address PR review feedback (#23230) --- codex-rs/app-server/src/extensions.rs | 7 +-- codex-rs/app-server/src/mcp_refresh.rs | 6 +-- codex-rs/app-server/src/message_processor.rs | 6 +-- codex-rs/core/src/tools/spec_plan.rs | 24 +++++++-- codex-rs/core/src/tools/spec_plan_tests.rs | 55 ++++++++++++++++++-- codex-rs/core/tests/common/test_codex.rs | 2 +- codex-rs/ext/plugins/src/lib.rs | 9 +--- codex-rs/tools/src/tool_config.rs | 4 ++ 8 files changed, 83 insertions(+), 30 deletions(-) diff --git a/codex-rs/app-server/src/extensions.rs b/codex-rs/app-server/src/extensions.rs index 5bb8d414f4..ea33737571 100644 --- a/codex-rs/app-server/src/extensions.rs +++ b/codex-rs/app-server/src/extensions.rs @@ -12,17 +12,14 @@ use codex_extension_api::ExtensionRegistryBuilder; use codex_protocol::ThreadId; use codex_protocol::error::CodexErr; -pub(crate) fn thread_extensions( - guardian_agent_spawner: S, - plugin_install_list_tool_enabled: bool, -) -> Arc> +pub(crate) fn thread_extensions(guardian_agent_spawner: S) -> Arc> where S: AgentSpawner + 'static, { let mut builder = ExtensionRegistryBuilder::::new(); codex_guardian::install(&mut builder, guardian_agent_spawner); codex_memories_extension::install(&mut builder); - codex_plugins_extension::install(&mut builder, plugin_install_list_tool_enabled); + codex_plugins_extension::install(&mut builder); Arc::new(builder.build()) } diff --git a/codex-rs/app-server/src/mcp_refresh.rs b/codex-rs/app-server/src/mcp_refresh.rs index 0ea6c8769e..f7d32b2ea8 100644 --- a/codex-rs/app-server/src/mcp_refresh.rs +++ b/codex-rs/app-server/src/mcp_refresh.rs @@ -2,7 +2,6 @@ use crate::config_manager::ConfigManager; use codex_core::CodexThread; use codex_core::ThreadManager; use codex_core::config::Config; -use codex_features::Feature; use codex_protocol::ThreadId; use codex_protocol::protocol::McpServerRefreshConfig; use codex_protocol::protocol::Op; @@ -187,10 +186,7 @@ mod tests { auth_manager, SessionSource::Exec, Arc::new(EnvironmentManager::default_for_tests()), - thread_extensions( - guardian_agent_spawner(thread_manager.clone()), - good_config.features.enabled(Feature::PluginInstallListTool), - ), + thread_extensions(guardian_agent_spawner(thread_manager.clone())), /*analytics_events_client*/ None, thread_store, Some(state_db.clone()), diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index e2a95d8cec..fc835b82ec 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -68,7 +68,6 @@ use codex_chatgpt::workspace_settings; use codex_core::ThreadManager; use codex_core::config::Config; use codex_exec_server::EnvironmentManager; -use codex_features::Feature; use codex_feedback::CodexFeedback; use codex_login::AuthManager; use codex_login::auth::ExternalAuth; @@ -308,10 +307,7 @@ impl MessageProcessor { auth_manager.clone(), session_source, environment_manager, - thread_extensions( - guardian_agent_spawner(thread_manager.clone()), - config.features.enabled(Feature::PluginInstallListTool), - ), + thread_extensions(guardian_agent_spawner(thread_manager.clone())), Some(analytics_events_client.clone()), Arc::clone(&thread_store), state_db.clone(), diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index fd704a26e7..da1b772a7e 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -85,7 +85,7 @@ struct ToolRegistryBuildParams<'a> { deferred_mcp_tools: Option<&'a [ToolInfo]>, discoverable_tools: Option<&'a [DiscoverableTool]>, extension_tool_executors: &'a [Arc>], - list_installable_plugins_registered: bool, + plugin_install_list_tool_enabled: bool, dynamic_tools: &'a [DynamicToolSpec], default_agent_type_description: &'a str, wait_agent_timeouts: WaitAgentTimeoutOptions, @@ -110,6 +110,11 @@ fn build_tool_specs_and_registry( let list_installable_plugins_registered = extension_tool_executors.iter().any(|executor| { executor.tool_name() == ToolName::plain(LIST_INSTALLABLE_PLUGINS_TOOL_NAME) }); + let plugin_install_list_tool_enabled = config.plugin_install_list_tool + && discoverable_tools + .as_ref() + .is_some_and(|tools| !tools.is_empty()) + && list_installable_plugins_registered; let default_agent_type_description = crate::agent::role::spawn_tool_spec::build(&std::collections::BTreeMap::new()); let mut executors = collect_tool_executors( @@ -119,7 +124,7 @@ fn build_tool_specs_and_registry( deferred_mcp_tools: deferred_mcp_tools.as_deref(), discoverable_tools: discoverable_tools.as_deref(), extension_tool_executors: &extension_tool_executors, - list_installable_plugins_registered, + plugin_install_list_tool_enabled, dynamic_tools, default_agent_type_description: &default_agent_type_description, wait_agent_timeouts: wait_agent_timeout_options(config), @@ -424,7 +429,7 @@ fn collect_tool_executors( { executors.push(Arc::new(RequestPluginInstallHandler::new( discoverable_tools, - params.list_installable_plugins_registered, + params.plugin_install_list_tool_enabled, ))); } @@ -553,7 +558,12 @@ fn collect_tool_executors( executors.push(handler); } - append_extension_tool_executors(config, params.extension_tool_executors, &mut executors); + append_extension_tool_executors( + config, + params.extension_tool_executors, + params.plugin_install_list_tool_enabled, + &mut executors, + ); executors } @@ -594,6 +604,7 @@ fn prepend_code_mode_executors( fn append_extension_tool_executors( config: &ToolsConfig, executors: &[Arc>], + plugin_install_list_tool_enabled: bool, registered_executors: &mut Vec>, ) { if executors.is_empty() { @@ -619,6 +630,11 @@ fn append_extension_tool_executors( for executor in executors.iter().cloned() { let tool_name = executor.tool_name(); + if tool_name == ToolName::plain(LIST_INSTALLABLE_PLUGINS_TOOL_NAME) + && !plugin_install_list_tool_enabled + { + continue; + } if !reserved_tool_names.insert(tool_name.clone()) { warn!("Skipping extension tool `{tool_name}`: tool already registered"); continue; diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index f53279b755..6d017dcad6 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -1982,6 +1982,7 @@ fn request_plugin_install_is_not_registered_without_feature_flag() { .iter() .any(|tool| tool.name() == REQUEST_PLUGIN_INSTALL_TOOL_NAME) ); + assert_lacks_tool_name(&tools, LIST_INSTALLABLE_PLUGINS_TOOL_NAME); } #[test] @@ -2044,6 +2045,50 @@ fn request_plugin_install_can_be_registered_without_search_tool() { ); } +#[test] +fn request_plugin_install_description_lists_discoverable_tools_when_list_feature_disabled() { + let model_info = search_capable_model_info(); + let mut features = Features::with_defaults(); + features.enable(Feature::Apps); + features.enable(Feature::Plugins); + features.enable(Feature::ToolSuggest); + features.disable(Feature::PluginInstallListTool); + let available_models = Vec::new(); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + available_models: &available_models, + features: &features, + image_generation_tool_auth_allowed: true, + web_search_mode: Some(WebSearchMode::Cached), + session_source: SessionSource::Cli, + permission_profile: &PermissionProfile::Disabled, + windows_sandbox_level: WindowsSandboxLevel::Disabled, + }); + let (tools, _) = build_specs_with_inputs_for_test( + &tools_config, + /*mcp_tools*/ None, + /*deferred_mcp_tools*/ None, + Some(vec![discoverable_connector( + "connector_2128aebfecb84f64a069897515042a44", + "Google Calendar", + "Plan events and schedules.", + )]), + &[extension_tool_executor( + LIST_INSTALLABLE_PLUGINS_TOOL_NAME, + "List installable plugins.", + )], + &[], + ); + + assert_lacks_tool_name(&tools, LIST_INSTALLABLE_PLUGINS_TOOL_NAME); + let request_plugin_install = find_tool(&tools, REQUEST_PLUGIN_INSTALL_TOOL_NAME); + let ToolSpec::Function(ResponsesApiTool { description, .. }) = request_plugin_install else { + panic!("expected function tool"); + }; + assert!(description.contains("Known plugins/connectors available to install:")); + assert!(!description.contains("Only call this tool with a result from")); +} + #[test] fn request_plugin_install_description_lists_discoverable_tools_without_list_tool() { let model_info = search_capable_model_info(); @@ -2514,9 +2559,13 @@ fn build_specs_with_inputs_for_test( deferred_mcp_tools: deferred_mcp_tools.as_deref(), discoverable_tools: discoverable_tools.as_deref(), extension_tool_executors, - list_installable_plugins_registered: extension_tool_executors.iter().any(|executor| { - executor.tool_name() == ToolName::plain(LIST_INSTALLABLE_PLUGINS_TOOL_NAME) - }), + plugin_install_list_tool_enabled: config.plugin_install_list_tool + && discoverable_tools + .as_ref() + .is_some_and(|tools| !tools.is_empty()) + && extension_tool_executors.iter().any(|executor| { + executor.tool_name() == ToolName::plain(LIST_INSTALLABLE_PLUGINS_TOOL_NAME) + }), dynamic_tools, default_agent_type_description: DEFAULT_AGENT_TYPE_DESCRIPTION, wait_agent_timeouts: wait_agent_timeout_options(), diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index 97b77e143f..5c3727b551 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -477,7 +477,7 @@ impl TestCodexBuilder { let installation_id = resolve_installation_id(&config.codex_home).await?; let extensions = if self.plugins_extension_enabled { let mut builder = ExtensionRegistryBuilder::new(); - codex_plugins_extension::install(&mut builder, true); + codex_plugins_extension::install(&mut builder); Arc::new(builder.build()) } else { empty_extension_registry() diff --git a/codex-rs/ext/plugins/src/lib.rs b/codex-rs/ext/plugins/src/lib.rs index 85f168c5b6..ede31f2940 100644 --- a/codex-rs/ext/plugins/src/lib.rs +++ b/codex-rs/ext/plugins/src/lib.rs @@ -101,13 +101,8 @@ impl ToolExecutor for ListInstallablePluginsTool { } /// Installs plugins extension contributors into the supplied extension registry. -pub fn install( - registry: &mut ExtensionRegistryBuilder, - list_installable_plugins_enabled: bool, -) { - if list_installable_plugins_enabled { - registry.tool_contributor(Arc::new(PluginsExtension)); - } +pub fn install(registry: &mut ExtensionRegistryBuilder) { + registry.tool_contributor(Arc::new(PluginsExtension)); } #[cfg(test)] diff --git a/codex-rs/tools/src/tool_config.rs b/codex-rs/tools/src/tool_config.rs index 5c1681d164..c978cf8a73 100644 --- a/codex-rs/tools/src/tool_config.rs +++ b/codex-rs/tools/src/tool_config.rs @@ -109,6 +109,7 @@ pub struct ToolsConfig { pub search_tool: bool, pub namespace_tools: bool, pub tool_suggest: bool, + pub plugin_install_list_tool: bool, pub exec_permission_approvals_enabled: bool, pub request_permissions_tool_enabled: bool, pub code_mode_enabled: bool, @@ -187,6 +188,8 @@ impl ToolsConfig { let include_tool_suggest = features.enabled(Feature::ToolSuggest) && features.enabled(Feature::Apps) && features.enabled(Feature::Plugins); + let include_plugin_install_list_tool = + include_tool_suggest && features.enabled(Feature::PluginInstallListTool); let include_original_image_detail = can_request_original_image_detail(model_info); // API-key auth bypasses Codex backend entitlement/tool normalization, so // callers must confirm ChatGPT auth before exposing the built-in tool. @@ -249,6 +252,7 @@ impl ToolsConfig { search_tool: include_search_tool, namespace_tools: true, tool_suggest: include_tool_suggest, + plugin_install_list_tool: include_plugin_install_list_tool, exec_permission_approvals_enabled, request_permissions_tool_enabled, code_mode_enabled: include_code_mode,