mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Add selected-plugin precedence and attribution to the MCP catalog (#27884)
## Why **In short:** this PR resolves already-discovered MCP registrations. It does not read selected plugins or discover their MCP servers. The resolved MCP catalog currently builds config and auto-discovered plugin registrations before runtime contributors are applied. A thread-selected plugin needs a distinct precedence tier in that same initial resolution pass: otherwise a disabled lower-precedence winner can leave stale name-level state behind, and the winning MCP tools cannot be attributed to the selected package reliably. This PR adds that catalog boundary before executor discovery is connected. ## What changed - Added an explicit selected-plugin registration tier between auto-discovered plugins and explicit config. - Collected selected-plugin contributions before the initial catalog build, while leaving compatibility and generic extension overlays in their existing runtime phase. - Retained the winning plugin ID and display name directly on plugin-owned catalog registrations. - Derived MCP tool provenance from the winning catalog entry instead of joining against local-only plugin summaries. - Retained the winning selected server's tool approval policy in the running connection manager, so a selected registration cannot inherit approval behavior from a losing local plugin. - Kept remembered approval session-scoped for selected plugins until there is an authority-aware persistence contract; Codex will not write approval back to an unrelated local plugin. - Preserved existing name-level disabled vetoes for discovered plugins and config, while keeping a selected package's own disabled registration scoped to that registration. - Preserved deterministic selection order and existing config, compatibility, and extension precedence. The resulting order is: ```text auto-discovered plugin < selected plugin < explicit config < compatibility registration < extension overlay ``` ## Behavior and scope This is a catalog and provenance change only. No production host contributes selected-plugin MCP registrations yet, so existing local MCP behavior remains unchanged. The stacked follow-up, #27870, installs the executor plugin provider that produces these registrations. App-server activation remains a separate final step. ## Verification Focused tests cover precedence, deterministic selected-plugin conflicts, disabled-veto behavior across catalog phases, managed requirements before selected-plugin resolution, winning-server approval policy, and attribution when local and selected packages share an ID or server name. CI owns execution of the test suite.
This commit is contained in:
@@ -4357,8 +4357,13 @@ async fn rebuild_preserving_session_layers_refreshes_plugin_derived_mcp_config()
|
||||
Some(&http_mcp("https://sample.example/mcp"))
|
||||
);
|
||||
assert_eq!(
|
||||
mcp_config.mcp_server_catalog.plugin_ids_by_server_name(),
|
||||
HashMap::from([("sample".to_string(), "sample@test".to_string())])
|
||||
mcp_config
|
||||
.mcp_server_catalog
|
||||
.plugin_attributions_by_server_name(),
|
||||
HashMap::from([(
|
||||
"sample".to_string(),
|
||||
McpPluginAttribution::new("sample@test".to_string(), "sample".to_string()),
|
||||
)])
|
||||
);
|
||||
|
||||
Ok(())
|
||||
@@ -4416,7 +4421,7 @@ enabled = true
|
||||
assert!(
|
||||
mcp_config
|
||||
.mcp_server_catalog
|
||||
.plugin_ids_by_server_name()
|
||||
.plugin_attributions_by_server_name()
|
||||
.is_empty()
|
||||
);
|
||||
|
||||
@@ -4424,7 +4429,7 @@ enabled = true
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn to_mcp_config_applies_plugin_mcp_cloud_config_bundle() -> anyhow::Result<()> {
|
||||
async fn selected_plugin_wins_after_discovered_plugin_requirements() -> anyhow::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let plugin_root = codex_home
|
||||
.path()
|
||||
@@ -4497,6 +4502,36 @@ url = "https://sample.example/mcp"
|
||||
})
|
||||
))
|
||||
);
|
||||
|
||||
let selected = http_mcp("https://selected.example/mcp");
|
||||
let mcp_config = config
|
||||
.to_mcp_config_with_plugin_registrations(
|
||||
&plugins_manager,
|
||||
[McpServerRegistration::from_selected_plugin(
|
||||
"unlisted".to_string(),
|
||||
McpPluginAttribution::new(
|
||||
"selected-root".to_string(),
|
||||
"Selected Plugin".to_string(),
|
||||
),
|
||||
/*selection_order*/ 0,
|
||||
selected.clone(),
|
||||
)],
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
mcp_config
|
||||
.mcp_server_catalog
|
||||
.server("unlisted")
|
||||
.map(|server| (server.source().clone(), server.config().clone())),
|
||||
Some((
|
||||
codex_mcp::McpServerSource::SelectedPlugin(McpPluginAttribution::new(
|
||||
"selected-root".to_string(),
|
||||
"Selected Plugin".to_string(),
|
||||
)),
|
||||
selected,
|
||||
))
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -70,6 +70,7 @@ 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::McpPluginAttribution;
|
||||
use codex_mcp::McpServerRegistration;
|
||||
use codex_mcp::ResolvedMcpCatalog;
|
||||
use codex_memories_read::memory_root;
|
||||
@@ -1396,19 +1397,45 @@ impl Config {
|
||||
)
|
||||
}
|
||||
|
||||
pub async fn to_mcp_config(
|
||||
/// Applies managed MCP requirements to servers supplied by one plugin.
|
||||
pub fn apply_plugin_mcp_server_requirements(
|
||||
&self,
|
||||
plugins_manager: &codex_core_plugins::PluginsManager,
|
||||
) -> McpConfig {
|
||||
let plugins_input = self.plugins_config_input();
|
||||
let loaded_plugins = plugins_manager.plugins_for_config(&plugins_input).await;
|
||||
let mut catalog = ResolvedMcpCatalog::builder();
|
||||
plugin_id: &str,
|
||||
mcp_servers: &mut HashMap<String, McpServerConfig>,
|
||||
) {
|
||||
filter_plugin_mcp_servers_by_requirements(
|
||||
plugin_id,
|
||||
mcp_servers,
|
||||
self.config_layer_stack.requirements().plugins.as_ref(),
|
||||
);
|
||||
let empty_mcp_allowlist = self
|
||||
.config_layer_stack
|
||||
.requirements()
|
||||
.mcp_servers
|
||||
.as_ref()
|
||||
.filter(|requirements| requirements.value.is_empty());
|
||||
filter_mcp_servers_by_requirements(mcp_servers, empty_mcp_allowlist);
|
||||
}
|
||||
|
||||
pub async fn to_mcp_config(
|
||||
&self,
|
||||
plugins_manager: &codex_core_plugins::PluginsManager,
|
||||
) -> McpConfig {
|
||||
self.to_mcp_config_with_plugin_registrations(
|
||||
plugins_manager,
|
||||
std::iter::empty::<McpServerRegistration>(),
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
pub(crate) async fn to_mcp_config_with_plugin_registrations(
|
||||
&self,
|
||||
plugins_manager: &codex_core_plugins::PluginsManager,
|
||||
additional_plugin_registrations: impl IntoIterator<Item = McpServerRegistration>,
|
||||
) -> McpConfig {
|
||||
let plugins_input = self.plugins_config_input();
|
||||
let loaded_plugins = plugins_manager.plugins_for_config(&plugins_input).await;
|
||||
let mut catalog = ResolvedMcpCatalog::builder();
|
||||
for (plugin_order, plugin) in loaded_plugins
|
||||
.plugins()
|
||||
.iter()
|
||||
@@ -1416,21 +1443,23 @@ impl Config {
|
||||
.enumerate()
|
||||
{
|
||||
let mut plugin_mcp_servers = plugin.mcp_servers.clone();
|
||||
filter_plugin_mcp_servers_by_requirements(
|
||||
&plugin.config_name,
|
||||
&mut plugin_mcp_servers,
|
||||
self.config_layer_stack.requirements().plugins.as_ref(),
|
||||
self.apply_plugin_mcp_server_requirements(&plugin.config_name, &mut plugin_mcp_servers);
|
||||
let attribution = McpPluginAttribution::new(
|
||||
plugin.config_name.clone(),
|
||||
plugin.display_name().to_string(),
|
||||
);
|
||||
filter_mcp_servers_by_requirements(&mut plugin_mcp_servers, empty_mcp_allowlist);
|
||||
for (name, plugin_server) in plugin_mcp_servers {
|
||||
catalog.register(McpServerRegistration::from_plugin(
|
||||
name,
|
||||
plugin.config_name.clone(),
|
||||
attribution.clone(),
|
||||
plugin_order,
|
||||
plugin_server,
|
||||
));
|
||||
}
|
||||
}
|
||||
for registration in additional_plugin_registrations {
|
||||
catalog.register(registration);
|
||||
}
|
||||
for (name, server) in self.mcp_servers.get() {
|
||||
catalog.register(McpServerRegistration::from_config(
|
||||
name.clone(),
|
||||
|
||||
+85
-23
@@ -12,6 +12,7 @@ use codex_login::CodexAuth;
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use codex_mcp::EffectiveMcpServer;
|
||||
use codex_mcp::McpConfig;
|
||||
use codex_mcp::McpPluginAttribution;
|
||||
use codex_mcp::McpServerRegistration;
|
||||
use codex_mcp::codex_apps_mcp_server_config;
|
||||
use codex_mcp::configured_mcp_servers;
|
||||
@@ -19,6 +20,20 @@ use codex_mcp::effective_mcp_servers;
|
||||
|
||||
const LEGACY_CODEX_APPS_REGISTRATION_ID: &str = "legacy_codex_apps";
|
||||
|
||||
enum OrderedMcpOverlay {
|
||||
Set {
|
||||
contributor_id: &'static str,
|
||||
contribution_order: usize,
|
||||
name: String,
|
||||
config: Box<McpServerConfig>,
|
||||
},
|
||||
Remove {
|
||||
contributor_id: &'static str,
|
||||
contribution_order: usize,
|
||||
name: String,
|
||||
},
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct McpManager {
|
||||
plugins_manager: Arc<PluginsManager>,
|
||||
@@ -65,7 +80,58 @@ impl McpManager {
|
||||
config: &Config,
|
||||
thread_init: Option<&ExtensionDataInit>,
|
||||
) -> McpConfig {
|
||||
let mut mcp_config = config.to_mcp_config(self.plugins_manager.as_ref()).await;
|
||||
let context = match thread_init {
|
||||
Some(thread_init) => McpServerContributionContext::for_thread(config, thread_init),
|
||||
None => McpServerContributionContext::global(config),
|
||||
};
|
||||
let mut selected_plugin_registrations = Vec::new();
|
||||
let mut overlays = Vec::new();
|
||||
// A contributor can emit multiple ordered actions, so order each action globally rather
|
||||
// than enumerating contributors.
|
||||
let mut contribution_order = 0;
|
||||
for contributor in self.extensions.mcp_server_contributors() {
|
||||
for contribution in contributor.contribute(context).await {
|
||||
match contribution {
|
||||
McpServerContribution::Set { name, config } => {
|
||||
overlays.push(OrderedMcpOverlay::Set {
|
||||
contributor_id: contributor.id(),
|
||||
contribution_order,
|
||||
name,
|
||||
config,
|
||||
});
|
||||
}
|
||||
McpServerContribution::SelectedPlugin {
|
||||
name,
|
||||
plugin_id,
|
||||
plugin_display_name,
|
||||
selection_order,
|
||||
config,
|
||||
} => selected_plugin_registrations.push(
|
||||
McpServerRegistration::from_selected_plugin(
|
||||
name,
|
||||
McpPluginAttribution::new(plugin_id, plugin_display_name),
|
||||
selection_order,
|
||||
*config,
|
||||
),
|
||||
),
|
||||
McpServerContribution::Remove { name } => {
|
||||
overlays.push(OrderedMcpOverlay::Remove {
|
||||
contributor_id: contributor.id(),
|
||||
contribution_order,
|
||||
name,
|
||||
});
|
||||
}
|
||||
}
|
||||
contribution_order += 1;
|
||||
}
|
||||
}
|
||||
|
||||
let mut mcp_config = config
|
||||
.to_mcp_config_with_plugin_registrations(
|
||||
self.plugins_manager.as_ref(),
|
||||
selected_plugin_registrations,
|
||||
)
|
||||
.await;
|
||||
let mut catalog = mcp_config.mcp_server_catalog.to_builder();
|
||||
if mcp_config.apps_enabled {
|
||||
catalog.register(McpServerRegistration::from_compatibility(
|
||||
@@ -83,28 +149,24 @@ impl McpManager {
|
||||
);
|
||||
}
|
||||
|
||||
let context = match thread_init {
|
||||
Some(thread_init) => McpServerContributionContext::for_thread(config, thread_init),
|
||||
None => McpServerContributionContext::global(config),
|
||||
};
|
||||
let mut contribution_order = 0;
|
||||
for contributor in self.extensions.mcp_server_contributors() {
|
||||
for contribution in contributor.contribute(context).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;
|
||||
for overlay in overlays {
|
||||
match overlay {
|
||||
OrderedMcpOverlay::Set {
|
||||
contributor_id,
|
||||
contribution_order,
|
||||
name,
|
||||
config,
|
||||
} => catalog.register(McpServerRegistration::from_extension(
|
||||
name,
|
||||
contributor_id,
|
||||
contribution_order,
|
||||
*config,
|
||||
)),
|
||||
OrderedMcpOverlay::Remove {
|
||||
contributor_id,
|
||||
contribution_order,
|
||||
name,
|
||||
} => catalog.remove_extension(name, contributor_id, contribution_order),
|
||||
}
|
||||
}
|
||||
let catalog = catalog.build();
|
||||
|
||||
@@ -166,6 +166,15 @@ pub(crate) async fn handle_mcp_tool_call(
|
||||
};
|
||||
let approval_mode = if server == CODEX_APPS_MCP_SERVER_NAME {
|
||||
app_tool_policy.approval
|
||||
} else if let Some(approval_mode) = {
|
||||
// Selected-plugin registrations are absent from config.toml and the legacy plugin manager,
|
||||
// so their resolved catalog entry is the authoritative source for tool approval policy.
|
||||
let manager = sess.services.mcp_connection_manager.load();
|
||||
manager
|
||||
.is_selected_plugin_mcp_server(&server)
|
||||
.then(|| manager.tool_approval_mode(&server, &tool_name))
|
||||
} {
|
||||
approval_mode
|
||||
} else {
|
||||
custom_mcp_tool_approval_mode(sess.as_ref(), turn_context.as_ref(), &server, &tool_name)
|
||||
.await
|
||||
@@ -1168,8 +1177,16 @@ async fn maybe_request_mcp_tool_approval(
|
||||
}
|
||||
|
||||
let session_approval_key = session_mcp_tool_approval_key(invocation, metadata, approval_mode);
|
||||
let persistent_approval_key =
|
||||
persistent_mcp_tool_approval_key(invocation, metadata, approval_mode);
|
||||
let persistent_approval_key = if sess
|
||||
.services
|
||||
.mcp_connection_manager
|
||||
.load()
|
||||
.is_selected_plugin_mcp_server(&invocation.server)
|
||||
{
|
||||
None
|
||||
} else {
|
||||
persistent_mcp_tool_approval_key(invocation, metadata, approval_mode)
|
||||
};
|
||||
if let Some(key) = session_approval_key.as_ref()
|
||||
&& mcp_tool_approval_is_remembered(sess, key).await
|
||||
{
|
||||
|
||||
@@ -448,8 +448,12 @@ async fn start_thread_seeds_extension_data_for_mcp_and_lifecycle_contributors()
|
||||
&selected_root.location;
|
||||
server.environment_id = environment_id.clone();
|
||||
server.enabled = false;
|
||||
vec![codex_extension_api::McpServerContribution::Set {
|
||||
name: selected_root.id,
|
||||
let plugin_id = selected_root.id;
|
||||
vec![codex_extension_api::McpServerContribution::SelectedPlugin {
|
||||
name: plugin_id.clone(),
|
||||
plugin_display_name: plugin_id.clone(),
|
||||
plugin_id,
|
||||
selection_order: 0,
|
||||
config: Box::new(server),
|
||||
}]
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user