mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Resolve MCP server registrations through a catalog (#27634)
## Why MCP servers currently come from user config, local plugins, compatibility Apps synthesis, and host extensions. Those sources were composed by mutating a shared map, leaving registration identity, precedence, removal, and provenance implicit in assembly order. Before adding executor-owned MCPs, Codex needs one durable resolution boundary above `McpConnectionManager`. This PR introduces that boundary while preserving current server configuration, policy, and runtime behavior. Executor-scoped registrations and explicit policy layers remain follow-ups. ## What changed - Add typed `McpServerRegistration` inputs and an immutable `ResolvedMcpCatalog` in `codex-mcp`. - Retain each registration's complete `McpServerConfig`, including its environment binding, while recording its source and provenance. - Preserve the existing structural precedence between plugin, config, compatibility, and ordered extension sources. - Resolve equal-precedence actions by contribution order; provenance IDs are used only for diagnostics and cannot affect the winner. - Preserve extension removals and the existing name-scoped `enabled = false` veto. - Report same-tier conflicts with every contender and the final catalog outcome, including whether the winning action registers or removes the server. - Require MCP contributors to provide a stable diagnostic identity. - Derive materialized server maps and plugin ownership from the resolved catalog. `McpConnectionManager`, transport startup, tool calls, and resource routing continue to consume the same effective `McpServerConfig` values. ## Scope This PR does not add new MCP capabilities or change user-visible behavior. It does not add executor plugin discovery, thread-scoped registrations, dynamic refresh generations, or new user/managed policy semantics. ## Verification - Added focused catalog coverage for source precedence, complete configuration preservation, disabled vetoes, plugin ownership, contribution-order tie breaking, removal outcomes, and conflict diagnostics. - Extended hosted Apps coverage for ordered extension removal and Apps-disabled hosts with and without the hosted extension installed. - `cargo check -p codex-mcp --tests -p codex-extension-api -p codex-core`
This commit is contained in:
@@ -4347,13 +4347,14 @@ async fn rebuild_preserving_session_layers_refreshes_plugin_derived_mcp_config()
|
||||
.await?;
|
||||
let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf());
|
||||
let mcp_config = config.to_mcp_config(&plugins_manager).await;
|
||||
let configured_servers = mcp_config.mcp_server_catalog.configured_servers();
|
||||
|
||||
assert_eq!(
|
||||
mcp_config.configured_mcp_servers.get("sample"),
|
||||
configured_servers.get("sample"),
|
||||
Some(&http_mcp("https://sample.example/mcp"))
|
||||
);
|
||||
assert_eq!(
|
||||
mcp_config.plugin_ids_by_mcp_server_name,
|
||||
mcp_config.mcp_server_catalog.plugin_ids_by_server_name(),
|
||||
HashMap::from([("sample".to_string(), "sample@test".to_string())])
|
||||
);
|
||||
|
||||
@@ -4403,12 +4404,18 @@ enabled = true
|
||||
.await?;
|
||||
let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf());
|
||||
let mcp_config = config.to_mcp_config(&plugins_manager).await;
|
||||
let configured_servers = mcp_config.mcp_server_catalog.configured_servers();
|
||||
|
||||
assert_eq!(
|
||||
mcp_config.configured_mcp_servers.get("sample"),
|
||||
configured_servers.get("sample"),
|
||||
Some(&http_mcp("https://user.example/mcp"))
|
||||
);
|
||||
assert!(mcp_config.plugin_ids_by_mcp_server_name.is_empty());
|
||||
assert!(
|
||||
mcp_config
|
||||
.mcp_server_catalog
|
||||
.plugin_ids_by_server_name()
|
||||
.is_empty()
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
@@ -4465,17 +4472,16 @@ url = "https://sample.example/mcp"
|
||||
.await?;
|
||||
let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf());
|
||||
let mcp_config = config.to_mcp_config(&plugins_manager).await;
|
||||
let configured_servers = mcp_config.mcp_server_catalog.configured_servers();
|
||||
|
||||
assert_eq!(
|
||||
mcp_config
|
||||
.configured_mcp_servers
|
||||
configured_servers
|
||||
.get("sample")
|
||||
.map(|server| (server.enabled, server.disabled_reason.clone())),
|
||||
Some((true, None))
|
||||
);
|
||||
assert_eq!(
|
||||
mcp_config
|
||||
.configured_mcp_servers
|
||||
configured_servers
|
||||
.get("unlisted")
|
||||
.map(|server| (server.enabled, server.disabled_reason.clone())),
|
||||
Some((
|
||||
@@ -4538,10 +4544,10 @@ enabled = true
|
||||
.await?;
|
||||
let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf());
|
||||
let mcp_config = config.to_mcp_config(&plugins_manager).await;
|
||||
let configured_servers = mcp_config.mcp_server_catalog.configured_servers();
|
||||
|
||||
assert_eq!(
|
||||
mcp_config
|
||||
.configured_mcp_servers
|
||||
configured_servers
|
||||
.get("sample")
|
||||
.map(|server| (server.enabled, server.disabled_reason.clone())),
|
||||
Some((
|
||||
|
||||
@@ -69,6 +69,8 @@ use codex_git_utils::resolve_root_git_project_for_trust;
|
||||
use codex_install_context::InstallContext;
|
||||
use codex_login::AuthManagerConfig;
|
||||
use codex_mcp::McpConfig;
|
||||
use codex_mcp::McpServerRegistration;
|
||||
use codex_mcp::ResolvedMcpCatalog;
|
||||
use codex_memories_read::memory_root;
|
||||
use codex_model_provider_info::LEGACY_OLLAMA_CHAT_PROVIDER_ID;
|
||||
use codex_model_provider_info::ModelProviderInfo;
|
||||
@@ -112,7 +114,6 @@ use serde::Serialize;
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::collections::hash_map::Entry;
|
||||
use std::io::ErrorKind;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
@@ -1391,12 +1392,18 @@ impl Config {
|
||||
) -> McpConfig {
|
||||
let plugins_input = self.plugins_config_input();
|
||||
let loaded_plugins = plugins_manager.plugins_for_config(&plugins_input).await;
|
||||
let mut configured_mcp_servers = self.mcp_servers.get().clone();
|
||||
let mut plugin_ids_by_mcp_server_name = HashMap::new();
|
||||
for plugin in loaded_plugins
|
||||
let mut catalog = ResolvedMcpCatalog::builder();
|
||||
let empty_mcp_allowlist = self
|
||||
.config_layer_stack
|
||||
.requirements()
|
||||
.mcp_servers
|
||||
.as_ref()
|
||||
.filter(|requirements| requirements.value.is_empty());
|
||||
for (plugin_order, plugin) in loaded_plugins
|
||||
.plugins()
|
||||
.iter()
|
||||
.filter(|plugin| plugin.is_active())
|
||||
.enumerate()
|
||||
{
|
||||
let mut plugin_mcp_servers = plugin.mcp_servers.clone();
|
||||
filter_plugin_mcp_servers_by_requirements(
|
||||
@@ -1404,22 +1411,22 @@ impl Config {
|
||||
&mut plugin_mcp_servers,
|
||||
self.config_layer_stack.requirements().plugins.as_ref(),
|
||||
);
|
||||
filter_mcp_servers_by_requirements(&mut plugin_mcp_servers, empty_mcp_allowlist);
|
||||
for (name, plugin_server) in plugin_mcp_servers {
|
||||
if let Entry::Vacant(entry) = configured_mcp_servers.entry(name.clone()) {
|
||||
entry.insert(plugin_server);
|
||||
plugin_ids_by_mcp_server_name.insert(name, plugin.config_name.clone());
|
||||
}
|
||||
catalog.register(McpServerRegistration::from_plugin(
|
||||
name,
|
||||
plugin.config_name.clone(),
|
||||
plugin_order,
|
||||
plugin_server,
|
||||
));
|
||||
}
|
||||
}
|
||||
if let Some(mcp_requirements) = self.config_layer_stack.requirements().mcp_servers.as_ref()
|
||||
&& mcp_requirements.value.is_empty()
|
||||
{
|
||||
// A present empty allowlist bans configurable MCPs, including plugin MCPs merged
|
||||
// above.
|
||||
filter_mcp_servers_by_requirements(&mut configured_mcp_servers, Some(mcp_requirements));
|
||||
for (name, server) in self.mcp_servers.get() {
|
||||
catalog.register(McpServerRegistration::from_config(
|
||||
name.clone(),
|
||||
server.clone(),
|
||||
));
|
||||
}
|
||||
plugin_ids_by_mcp_server_name
|
||||
.retain(|server_name, _| configured_mcp_servers.contains_key(server_name));
|
||||
|
||||
McpConfig {
|
||||
chatgpt_base_url: self.chatgpt_base_url.clone(),
|
||||
@@ -1446,8 +1453,7 @@ impl Config {
|
||||
// indicates this should be an empty object.
|
||||
ElicitationCapability::default()
|
||||
},
|
||||
configured_mcp_servers,
|
||||
plugin_ids_by_mcp_server_name,
|
||||
mcp_server_catalog: catalog.build(),
|
||||
plugin_capability_summaries: loaded_plugins.capability_summaries().to_vec(),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -43,6 +43,7 @@ use codex_mcp::codex_apps_tools_cache_key;
|
||||
use codex_mcp::compute_auth_statuses;
|
||||
use codex_mcp::effective_mcp_servers;
|
||||
use codex_mcp::host_owned_codex_apps_enabled;
|
||||
use codex_mcp::tool_plugin_provenance;
|
||||
|
||||
const CONNECTORS_READY_TIMEOUT_ON_EMPTY_TOOLS: Duration = Duration::from_secs(30);
|
||||
|
||||
@@ -251,7 +252,8 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_mcp_manager(
|
||||
});
|
||||
}
|
||||
let cache_key = accessible_connectors_cache_key(config, auth.as_ref());
|
||||
let tool_plugin_provenance = mcp_manager.tool_plugin_provenance(config).await;
|
||||
let mcp_config = mcp_manager.runtime_config(config).await;
|
||||
let tool_plugin_provenance = tool_plugin_provenance(&mcp_config);
|
||||
if !force_refetch && let Some(cached_connectors) = read_cached_accessible_connectors(&cache_key)
|
||||
{
|
||||
let cached_connectors = codex_connectors::filter::filter_disallowed_connectors(
|
||||
@@ -265,7 +267,6 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_mcp_manager(
|
||||
});
|
||||
}
|
||||
|
||||
let mcp_config = mcp_manager.runtime_config(config).await;
|
||||
let mut mcp_servers = effective_mcp_servers(&mcp_config, auth.as_ref());
|
||||
mcp_servers.retain(|name, _| name == CODEX_APPS_MCP_SERVER_NAME);
|
||||
let host_owned_codex_apps_enabled = host_owned_codex_apps_enabled(&mcp_config, auth.as_ref());
|
||||
|
||||
+40
-48
@@ -10,11 +10,12 @@ use codex_login::CodexAuth;
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use codex_mcp::EffectiveMcpServer;
|
||||
use codex_mcp::McpConfig;
|
||||
use codex_mcp::ToolPluginProvenance;
|
||||
use codex_mcp::McpServerRegistration;
|
||||
use codex_mcp::codex_apps_mcp_server_config;
|
||||
use codex_mcp::configured_mcp_servers;
|
||||
use codex_mcp::effective_mcp_servers;
|
||||
use codex_mcp::tool_plugin_provenance as collect_tool_plugin_provenance;
|
||||
|
||||
const LEGACY_CODEX_APPS_REGISTRATION_ID: &str = "legacy_codex_apps";
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct McpManager {
|
||||
@@ -45,32 +46,53 @@ impl McpManager {
|
||||
/// runtime-only extension overlays.
|
||||
pub async fn runtime_config(&self, config: &Config) -> McpConfig {
|
||||
let mut mcp_config = config.to_mcp_config(self.plugins_manager.as_ref()).await;
|
||||
let disabled_server_names = mcp_config
|
||||
.configured_mcp_servers
|
||||
.iter()
|
||||
.filter(|(_, server)| !server.enabled)
|
||||
.map(|(name, _)| name.clone())
|
||||
.collect::<Vec<_>>();
|
||||
let mut catalog = mcp_config.mcp_server_catalog.to_builder();
|
||||
if mcp_config.apps_enabled {
|
||||
mcp_config.configured_mcp_servers.insert(
|
||||
catalog.register(McpServerRegistration::from_compatibility(
|
||||
CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
LEGACY_CODEX_APPS_REGISTRATION_ID,
|
||||
codex_apps_mcp_server_config(
|
||||
&mcp_config.chatgpt_base_url,
|
||||
mcp_config.apps_mcp_product_sku.as_deref(),
|
||||
),
|
||||
);
|
||||
));
|
||||
} else {
|
||||
mcp_config
|
||||
.configured_mcp_servers
|
||||
.remove(CODEX_APPS_MCP_SERVER_NAME);
|
||||
catalog.remove_compatibility(
|
||||
CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
LEGACY_CODEX_APPS_REGISTRATION_ID,
|
||||
);
|
||||
}
|
||||
let contributions = self.contributions(config).await;
|
||||
Self::apply_to_configured_servers(&contributions, &mut mcp_config.configured_mcp_servers);
|
||||
for name in disabled_server_names {
|
||||
if let Some(server) = mcp_config.configured_mcp_servers.get_mut(&name) {
|
||||
server.enabled = false;
|
||||
|
||||
let mut contribution_order = 0;
|
||||
for contributor in self.extensions.mcp_server_contributors() {
|
||||
for contribution in contributor.contribute(config).await {
|
||||
match contribution {
|
||||
McpServerContribution::Set {
|
||||
name,
|
||||
config: server_config,
|
||||
} => catalog.register(McpServerRegistration::from_extension(
|
||||
name,
|
||||
contributor.id(),
|
||||
contribution_order,
|
||||
*server_config,
|
||||
)),
|
||||
McpServerContribution::Remove { name } => {
|
||||
catalog.remove_extension(name, contributor.id(), contribution_order)
|
||||
}
|
||||
}
|
||||
contribution_order += 1;
|
||||
}
|
||||
}
|
||||
let catalog = catalog.build();
|
||||
for conflict in catalog.conflicts() {
|
||||
tracing::warn!(
|
||||
server = conflict.name,
|
||||
outcome = ?conflict.outcome,
|
||||
contenders = ?conflict.contenders,
|
||||
"conflicting MCP server actions; using resolved catalog outcome"
|
||||
);
|
||||
}
|
||||
mcp_config.mcp_server_catalog = catalog;
|
||||
mcp_config
|
||||
}
|
||||
|
||||
@@ -95,34 +117,4 @@ impl McpManager {
|
||||
let mcp_config = self.runtime_config(config).await;
|
||||
effective_mcp_servers(&mcp_config, auth)
|
||||
}
|
||||
|
||||
/// Returns provenance for plugin-owned servers in the configured view.
|
||||
pub async fn tool_plugin_provenance(&self, config: &Config) -> ToolPluginProvenance {
|
||||
let mcp_config = config.to_mcp_config(self.plugins_manager.as_ref()).await;
|
||||
collect_tool_plugin_provenance(&mcp_config)
|
||||
}
|
||||
|
||||
async fn contributions(&self, config: &Config) -> Vec<McpServerContribution> {
|
||||
let mut contributions = Vec::new();
|
||||
for contributor in self.extensions.mcp_server_contributors() {
|
||||
contributions.extend(contributor.contribute(config).await);
|
||||
}
|
||||
contributions
|
||||
}
|
||||
|
||||
fn apply_to_configured_servers(
|
||||
contributions: &[McpServerContribution],
|
||||
servers: &mut HashMap<String, McpServerConfig>,
|
||||
) {
|
||||
for contribution in contributions {
|
||||
match contribution {
|
||||
McpServerContribution::Set { name, config } => {
|
||||
servers.insert(name.clone(), config.as_ref().clone());
|
||||
}
|
||||
McpServerContribution::Remove { name } => {
|
||||
servers.remove(name);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -294,11 +294,7 @@ impl Session {
|
||||
.mcp_manager
|
||||
.runtime_config(config.as_ref())
|
||||
.await;
|
||||
let tool_plugin_provenance = self
|
||||
.services
|
||||
.mcp_manager
|
||||
.tool_plugin_provenance(config.as_ref())
|
||||
.await;
|
||||
let tool_plugin_provenance = codex_mcp::tool_plugin_provenance(&mcp_config);
|
||||
let mcp_servers =
|
||||
effective_mcp_servers_from_configured(mcp_servers, &mcp_config, auth.as_ref());
|
||||
let host_owned_codex_apps_enabled =
|
||||
|
||||
@@ -605,16 +605,16 @@ impl Session {
|
||||
let mcp_manager_for_mcp = Arc::clone(&mcp_manager);
|
||||
let auth_and_mcp_fut = async move {
|
||||
let auth = auth_manager_clone.auth().await;
|
||||
let mcp_servers = mcp_manager_for_mcp
|
||||
.effective_servers(&config_for_mcp, auth.as_ref())
|
||||
.await;
|
||||
let mcp_config = mcp_manager_for_mcp.runtime_config(&config_for_mcp).await;
|
||||
let mcp_servers = codex_mcp::effective_mcp_servers(&mcp_config, auth.as_ref());
|
||||
let tool_plugin_provenance = codex_mcp::tool_plugin_provenance(&mcp_config);
|
||||
let auth_statuses = compute_auth_statuses(
|
||||
mcp_servers.iter(),
|
||||
config_for_mcp.mcp_oauth_credentials_store_mode,
|
||||
auth.as_ref(),
|
||||
)
|
||||
.await;
|
||||
(auth, mcp_servers, auth_statuses)
|
||||
(auth, mcp_servers, auth_statuses, tool_plugin_provenance)
|
||||
}
|
||||
.instrument(info_span!(
|
||||
"session_init.auth_mcp",
|
||||
@@ -637,7 +637,7 @@ impl Session {
|
||||
let (
|
||||
thread_persistence_result,
|
||||
state_db_ctx,
|
||||
(auth, mcp_servers, auth_statuses),
|
||||
(auth, mcp_servers, auth_statuses, tool_plugin_provenance),
|
||||
plugin_skill_errors,
|
||||
) = tokio::join!(
|
||||
thread_persistence_fut,
|
||||
@@ -1104,7 +1104,6 @@ impl Session {
|
||||
sess.send_event_raw(event).await;
|
||||
}
|
||||
|
||||
let tool_plugin_provenance = mcp_manager.tool_plugin_provenance(config.as_ref()).await;
|
||||
let host_owned_codex_apps_enabled = config
|
||||
.features
|
||||
.apps_enabled_for_auth(auth.as_ref().is_some_and(|auth| auth.uses_codex_backend()));
|
||||
|
||||
Reference in New Issue
Block a user