clone/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-05-29 15:30:22 +00:00
Code Issues Packages Projects Releases Wiki Activity

codex: address PR review feedback (#23230)

Browse Source
This commit is contained in:
Dylan Hurd
2026-05-18 00:35:18 -07:00
parent c7aaadc76f
commit 9689bd16f7
8 changed files with 83 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
codex-rs/app-server/src/extensions.rs
View File

@@ -12,17 +12,14 @@ use codex_extension_api::ExtensionRegistryBuilder;
use codex_protocol::ThreadId;
use codex_protocol::error::CodexErr;
pub(crate) fn thread_extensions<S>(
guardian_agent_spawner: S,
plugin_install_list_tool_enabled: bool,
) -> Arc<ExtensionRegistry<Config>>
pub(crate) fn thread_extensions<S>(guardian_agent_spawner: S) -> Arc<ExtensionRegistry<Config>>
where
S: AgentSpawner<StartThreadOptions, Spawned = NewThread, Error = CodexErr> + 'static,
{
let mut builder = ExtensionRegistryBuilder::<Config>::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())
}

6
codex-rs/app-server/src/mcp_refresh.rs
View File

@@ -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()),

6
codex-rs/app-server/src/message_processor.rs
View File

@@ -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(),

24
codex-rs/core/src/tools/spec_plan.rs
View File

@@ -85,7 +85,7 @@ struct ToolRegistryBuildParams<'a> {
deferred_mcp_tools: Option<&'a [ToolInfo]>,
discoverable_tools: Option<&'a [DiscoverableTool]>,
extension_tool_executors: &'a [Arc<dyn ToolExecutor<ExtensionToolCall>>],
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<dyn ToolExecutor<ExtensionToolCall>>],
plugin_install_list_tool_enabled: bool,
registered_executors: &mut Vec<Arc<dyn CoreToolRuntime>>,
) {
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;

55
codex-rs/core/src/tools/spec_plan_tests.rs
View File

@@ -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(),

2
codex-rs/core/tests/common/test_codex.rs
View File

@@ -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()

9
codex-rs/ext/plugins/src/lib.rs
View File

@@ -101,13 +101,8 @@ impl ToolExecutor<ToolCall> for ListInstallablePluginsTool {
}
/// Installs plugins extension contributors into the supplied extension registry.
pub fn install<C>(
registry: &mut ExtensionRegistryBuilder<C>,
list_installable_plugins_enabled: bool,
) {
if list_installable_plugins_enabled {
registry.tool_contributor(Arc::new(PluginsExtension));
}
pub fn install<C>(registry: &mut ExtensionRegistryBuilder<C>) {
registry.tool_contributor(Arc::new(PluginsExtension));
}
#[cfg(test)]

4
codex-rs/tools/src/tool_config.rs
View File

@@ -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,