codex: address plugin refactor review feedback

This commit is contained in:
xli-oai
2026-04-30 01:48:25 -07:00
parent 80ce900035
commit 818838178d
14 changed files with 31 additions and 41 deletions
@@ -6395,7 +6395,7 @@ impl CodexMessageProcessor {
.plugins_for_layer_stack(&config.config_layer_stack, plugin_feature_flags)
.await
} else {
codex_core::plugins::PluginLoadOutcome::default()
codex_core_plugins::PluginLoadOutcome::default()
};
let hooks = codex_hooks::list_hooks(codex_hooks::HooksConfig {
feature_enabled: config.features.enabled(Feature::CodexHooks),
@@ -4,7 +4,7 @@ use codex_app_server_protocol::AppInfo;
use codex_app_server_protocol::AppSummary;
use codex_chatgpt::connectors;
use codex_core::config::Config;
use codex_core::plugins::AppConnectorId;
use codex_core_plugins::AppConnectorId;
use codex_exec_server::EnvironmentManager;
use tracing::warn;
@@ -113,7 +113,7 @@ pub(super) fn plugin_apps_needing_auth(
#[cfg(test)]
mod tests {
use codex_app_server_protocol::AppInfo;
use codex_core::plugins::AppConnectorId;
use codex_core_plugins::AppConnectorId;
use pretty_assertions::assert_eq;
use super::plugin_apps_needing_auth;
@@ -293,7 +293,7 @@ impl CodexMessageProcessor {
.app_ids
.iter()
.cloned()
.map(codex_core::plugins::AppConnectorId)
.map(codex_core_plugins::AppConnectorId)
.collect::<Vec<_>>();
let environment_manager = self.thread_manager.environment_manager();
let app_summaries = plugin_app_helpers::load_plugin_app_summaries(
@@ -618,7 +618,7 @@ impl CodexMessageProcessor {
config: &Config,
is_chatgpt_auth: bool,
plugin_id: &str,
plugin_apps: &[codex_core::plugins::AppConnectorId],
plugin_apps: &[codex_core_plugins::AppConnectorId],
) -> Vec<AppSummary> {
if plugin_apps.is_empty() || !config.features.apps_enabled_for_auth(is_chatgpt_auth) {
return Vec::new();
@@ -691,14 +691,14 @@ impl CodexMessageProcessor {
params: PluginUninstallParams,
) -> Result<PluginUninstallResponse, JSONRPCErrorError> {
let PluginUninstallParams { plugin_id } = params;
if codex_core::plugins::PluginId::parse(&plugin_id).is_err()
&& (plugin_id.is_empty() || !is_valid_remote_plugin_id(&plugin_id))
if codex_core_plugins::PluginId::parse(&plugin_id).is_err()
&& !is_valid_remote_uninstall_plugin_id(&plugin_id)
{
return Err(invalid_request(
"invalid plugin id: expected a local plugin id or remote plugin id",
"invalid plugin id: expected a local plugin id in the form `plugin@marketplace` or a remote plugin id starting with `plugins~`, `app_`, `asdk_app_`, or `connector_`",
));
}
if !plugin_id.is_empty() && is_valid_remote_plugin_id(&plugin_id) {
if is_valid_remote_uninstall_plugin_id(&plugin_id) {
return self.remote_plugin_uninstall_response(plugin_id).await;
}
let plugins_manager = self.thread_manager.plugins_manager();
@@ -835,6 +835,15 @@ fn is_valid_remote_plugin_id(plugin_name: &str) -> bool {
.all(|ch| ch.is_ascii_alphanumeric() || ch == '-' || ch == '_' || ch == '~')
}
fn is_valid_remote_uninstall_plugin_id(plugin_name: &str) -> bool {
!plugin_name.is_empty()
&& is_valid_remote_plugin_id(plugin_name)
&& (plugin_name.starts_with("plugins~")
|| plugin_name.starts_with("app_")
|| plugin_name.starts_with("asdk_app_")
|| plugin_name.starts_with("connector_"))
}
fn remote_marketplace_to_info(marketplace: RemoteMarketplace) -> PluginMarketplaceEntry {
PluginMarketplaceEntry {
name: marketplace.name,
@@ -1,7 +1,7 @@
use codex_config::types::PluginConfig;
use codex_core::config::Config;
use codex_core::config::ConfigBuilder;
use codex_core::plugins::PluginId;
use codex_core_plugins::PluginId;
use codex_core_plugins::PluginInstallRequest;
use codex_core_plugins::PluginsManager;
use codex_core_plugins::marketplace::MarketplacePluginInstallPolicy;
+1 -1
View File
@@ -32,7 +32,7 @@ use codex_config::ResidencyRequirement as CoreResidencyRequirement;
use codex_config::SandboxModeRequirement as CoreSandboxModeRequirement;
use codex_core::ThreadManager;
use codex_core::config::Config;
use codex_core::plugins::PluginId;
use codex_core_plugins::PluginId;
use codex_core_plugins::loader::installed_plugin_telemetry_metadata;
use codex_core_plugins::toggles::collect_plugin_enabled_candidates;
use codex_features::canonical_feature_for_key;
@@ -133,13 +133,6 @@ impl McpProcess {
Self::new_with_env_and_args(codex_home, &[], &[]).await
}
pub async fn new_with_env_and_plugin_startup_tasks(
codex_home: &Path,
env_overrides: &[(&str, Option<&str>)],
) -> anyhow::Result<Self> {
Self::new_with_env_and_args(codex_home, env_overrides, &[]).await
}
pub async fn new_with_args(codex_home: &Path, args: &[&str]) -> anyhow::Result<Self> {
let mut all_args = vec![DISABLE_PLUGIN_STARTUP_TASKS_ARG];
all_args.extend_from_slice(args);
+2 -3
View File
@@ -16,7 +16,7 @@ pub use codex_core::connectors::list_accessible_connectors_from_mcp_tools_with_o
pub use codex_core::connectors::list_accessible_connectors_from_mcp_tools_with_options_and_status;
pub use codex_core::connectors::list_cached_accessible_connectors_from_mcp_tools;
pub use codex_core::connectors::with_app_enabled_state;
use codex_core::plugins::AppConnectorId;
use codex_core_plugins::AppConnectorId;
use codex_core_plugins::PluginsManager;
use codex_login::AuthManager;
use codex_login::CodexAuth;
@@ -135,7 +135,7 @@ fn all_connectors_cache_key(config: &Config, auth: &CodexAuth) -> AllConnectorsC
)
}
async fn plugin_apps_for_config(config: &Config) -> Vec<codex_core::plugins::AppConnectorId> {
async fn plugin_apps_for_config(config: &Config) -> Vec<AppConnectorId> {
PluginsManager::new(config.codex_home.to_path_buf())
.plugins_for_config(&config.config_layer_stack, config.plugin_feature_flags())
.await
@@ -188,7 +188,6 @@ pub fn merge_connectors_with_accessible(
mod tests {
use super::*;
use codex_connectors::metadata::connector_install_url;
use codex_core::plugins::AppConnectorId;
use pretty_assertions::assert_eq;
fn app(id: &str) -> AppInfo {
+3 -1
View File
@@ -39,6 +39,8 @@ pub const TOOL_SUGGEST_DISCOVERABLE_PLUGIN_ALLOWLIST: &[&str] = &[
"computer-use@openai-bundled",
];
pub use codex_plugin::AppConnectorId;
pub use codex_plugin::PluginId;
pub use discoverable::list_tool_suggest_discoverable_plugins;
pub use manager::ConfiguredMarketplace;
pub use manager::ConfiguredMarketplaceListOutcome;
@@ -58,4 +60,4 @@ pub use manager::RemotePluginSyncResult;
#[cfg(test)]
pub(crate) type LoadedPlugin = codex_plugin::LoadedPlugin<McpServerConfig>;
pub(crate) type PluginLoadOutcome = codex_plugin::PluginLoadOutcome<McpServerConfig>;
pub type PluginLoadOutcome = codex_plugin::PluginLoadOutcome<McpServerConfig>;
+1 -1
View File
@@ -7,10 +7,10 @@ use codex_protocol::models::ResponseItem;
use crate::connectors;
use crate::context::ContextualUserFragment;
use crate::context::PluginInstructions;
use crate::plugins::PluginCapabilitySummary;
use crate::plugins::render_explicit_plugin_instructions;
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
use codex_mcp::ToolInfo;
use codex_plugin::PluginCapabilitySummary;
pub(crate) fn build_plugin_injections(
mentioned_plugins: &[PluginCapabilitySummary],
+1 -2
View File
@@ -12,8 +12,7 @@ use crate::injection::plugin_config_name_from_path;
use crate::injection::tool_kind_for_path;
use crate::mention_syntax::PLUGIN_TEXT_MENTION_SIGIL;
use crate::mention_syntax::TOOL_MENTION_SIGIL;
use super::PluginCapabilitySummary;
use codex_plugin::PluginCapabilitySummary;
pub(crate) struct CollectedToolMentions {
pub(crate) plain_names: HashSet<String>,
+1 -1
View File
@@ -1,11 +1,11 @@
use std::collections::HashSet;
use codex_plugin::PluginCapabilitySummary;
use codex_protocol::user_input::UserInput;
use pretty_assertions::assert_eq;
use super::collect_explicit_app_ids;
use super::collect_explicit_plugin_mentions;
use crate::plugins::PluginCapabilitySummary;
fn text_input(text: &str) -> UserInput {
UserInput::Text {
-13
View File
@@ -4,19 +4,6 @@ mod render;
#[cfg(test)]
pub(crate) mod test_support;
use codex_config::types::McpServerConfig;
pub use codex_plugin::AppConnectorId;
pub use codex_plugin::EffectiveSkillRoots;
pub use codex_plugin::PluginCapabilitySummary;
pub use codex_plugin::PluginId;
pub use codex_plugin::PluginIdError;
pub use codex_plugin::PluginTelemetryMetadata;
pub use codex_plugin::validate_plugin_segment;
pub type LoadedPlugin = codex_plugin::LoadedPlugin<McpServerConfig>;
pub type PluginLoadOutcome = codex_plugin::PluginLoadOutcome<McpServerConfig>;
pub(crate) use injection::build_plugin_injections;
pub(crate) use render::render_explicit_plugin_instructions;
+1 -1
View File
@@ -2,7 +2,7 @@
use crate::context::AvailablePluginsInstructions;
#[cfg(test)]
use crate::context::ContextualUserFragment;
use crate::plugins::PluginCapabilitySummary;
use codex_plugin::PluginCapabilitySummary;
#[cfg(test)]
pub(crate) fn render_plugins_section(plugins: &[PluginCapabilitySummary]) -> Option<String> {
+2 -1
View File
@@ -71,6 +71,7 @@ use codex_hooks::HookEvent;
use codex_hooks::HookEventAfterAgent;
use codex_hooks::HookPayload;
use codex_hooks::HookResult;
use codex_plugin::PluginCapabilitySummary;
use codex_protocol::config_types::ModeKind;
use codex_protocol::error::CodexErr;
use codex_protocol::error::Result as CodexResult;
@@ -275,7 +276,7 @@ pub(crate) async fn run_turn(
build_plugin_injections(&mentioned_plugins, &mcp_tools, &available_connectors);
let mentioned_plugin_metadata = mentioned_plugins
.iter()
.filter_map(crate::plugins::PluginCapabilitySummary::telemetry_metadata)
.filter_map(PluginCapabilitySummary::telemetry_metadata)
.collect::<Vec<_>>();
let mut explicitly_enabled_connectors = collect_explicit_app_ids(&input);