mirror of
https://github.com/openai/codex.git
synced 2026-10-24 18:18:41 +00:00
Reconcile V2 executor plugins with cloud catalogs (#55301)
## What changed - Use V2 discovery inventories for plugin MCP contributions when supplied, including empty or failed inventories; retain legacy discovery when no V2 snapshot is supplied. - Merge plugins by canonical identity, using cloud metadata and enablement for remote plugins and attaching executor installations only when versions match. Preserve configured executor-local plugins and executor enablement when cloud data is unavailable. - Keep installation order stable across refreshes and run merged MCP declarations on the first retained executor installation while preserving attribution for the others. - Carry local and remote identities through selected-plugin snapshots, skill attribution, and active-plugin telemetry. Track disabled plugin IDs alongside disabled roots. - Preserve a complete cloud catalog when a partial refresh arrives, and cap tool-description plugin attribution at 1,024 bytes while retaining all plugin associations. ## Testing Add coverage for catalog parsing and reconciliation, enablement and discovery-mode selection, installation refresh ordering, executor-specific MCP normalization, selected-plugin snapshots, and bounded UTF-8 attribution text. GitOrigin-RevId: a8ad352de6a2b8dc4bfd1ff6a5c62ee8ed1d1664
This commit is contained in:
committed by
copyberry
parent
1277bcfddc
commit
f7a135ec61
@@ -78,9 +78,11 @@ pub use mcp::hosted_plugin_runtime_mcp_server_config;
|
||||
pub use mcp::tool_plugin_context;
|
||||
pub use plugin_config::PluginMcpConfigParseOutcome;
|
||||
pub use plugin_config::PluginMcpServerParseError;
|
||||
pub use plugin_config::normalize_executor_plugin_mcp_declarations;
|
||||
pub use plugin_config::parse_agent_plugin_mcp_config;
|
||||
pub use plugin_config::parse_executor_plugin_mcp_config;
|
||||
pub use plugin_config::parse_plugin_mcp_config;
|
||||
pub use plugin_config::parse_plugin_mcp_declarations;
|
||||
|
||||
pub use mcp::McpServerStatusSnapshot;
|
||||
pub use mcp::McpSnapshotDetail;
|
||||
|
||||
@@ -63,6 +63,13 @@ impl PluginMcpFile {
|
||||
}
|
||||
}
|
||||
|
||||
/// Parses wrapped or plain plugin MCP declarations without runtime normalization.
|
||||
pub fn parse_plugin_mcp_declarations(
|
||||
contents: &str,
|
||||
) -> Result<BTreeMap<String, JsonValue>, serde_json::Error> {
|
||||
serde_json::from_str::<PluginMcpFile>(contents).map(PluginMcpFile::into_mcp_servers)
|
||||
}
|
||||
|
||||
/// Parses the two supported plugin MCP file shapes and normalizes each server.
|
||||
///
|
||||
/// Native plugin HTTP servers share the regular MCP transport configuration;
|
||||
@@ -93,6 +100,21 @@ pub fn parse_executor_plugin_mcp_config(
|
||||
)
|
||||
}
|
||||
|
||||
/// Normalizes already-parsed declarations for one executor installation.
|
||||
pub fn normalize_executor_plugin_mcp_declarations(
|
||||
plugin_root: &PathUri,
|
||||
declarations: BTreeMap<String, JsonValue>,
|
||||
environment_id: &str,
|
||||
) -> PluginMcpConfigParseOutcome {
|
||||
normalize_plugin_mcp_declarations(
|
||||
declarations,
|
||||
PluginMcpSource::Environment {
|
||||
root: plugin_root,
|
||||
environment_id,
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
impl PluginMcpSource<'_> {
|
||||
fn display(self) -> String {
|
||||
match self {
|
||||
@@ -106,10 +128,17 @@ fn parse_plugin_mcp_config_from(
|
||||
contents: &str,
|
||||
source: PluginMcpSource<'_>,
|
||||
) -> Result<PluginMcpConfigParseOutcome, serde_json::Error> {
|
||||
let parsed = serde_json::from_str::<PluginMcpFile>(contents)?;
|
||||
let declarations = parse_plugin_mcp_declarations(contents)?;
|
||||
Ok(normalize_plugin_mcp_declarations(declarations, source))
|
||||
}
|
||||
|
||||
fn normalize_plugin_mcp_declarations(
|
||||
declarations: BTreeMap<String, JsonValue>,
|
||||
source: PluginMcpSource<'_>,
|
||||
) -> PluginMcpConfigParseOutcome {
|
||||
let mut outcome = PluginMcpConfigParseOutcome::default();
|
||||
|
||||
for (name, config_value) in parsed.into_mcp_servers() {
|
||||
for (name, config_value) in declarations {
|
||||
match normalize_plugin_mcp_server(config_value, source) {
|
||||
Ok(config) => {
|
||||
outcome.servers.insert(name, config);
|
||||
@@ -120,7 +149,7 @@ fn parse_plugin_mcp_config_from(
|
||||
}
|
||||
}
|
||||
|
||||
Ok(outcome)
|
||||
outcome
|
||||
}
|
||||
|
||||
fn normalize_plugin_mcp_server(
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use super::PluginMcpConfigParseOutcome;
|
||||
use super::PluginMcpServerParseError;
|
||||
use super::normalize_executor_plugin_mcp_declarations;
|
||||
use super::parse_agent_plugin_mcp_config;
|
||||
use super::parse_executor_plugin_mcp_config;
|
||||
use super::parse_plugin_mcp_config;
|
||||
@@ -751,6 +752,18 @@ fn executor_environment_placement_resolves_foreign_uri_cwd() {
|
||||
)
|
||||
.expect("parse plugin MCP config");
|
||||
|
||||
assert_eq!(
|
||||
normalize_executor_plugin_mcp_declarations(
|
||||
&plugin_root,
|
||||
BTreeMap::from([(
|
||||
"demo".to_string(),
|
||||
serde_json::json!({"command": "demo-mcp", "cwd": "scripts"}),
|
||||
)]),
|
||||
"executor-1",
|
||||
),
|
||||
outcome
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
outcome,
|
||||
PluginMcpConfigParseOutcome {
|
||||
|
||||
@@ -12,6 +12,10 @@
|
||||
#[path = "rmcp_client/status.rs"]
|
||||
mod status;
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "rmcp_client/provenance_tests.rs"]
|
||||
mod provenance_tests;
|
||||
|
||||
use std::borrow::Cow;
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::HashMap;
|
||||
@@ -97,6 +101,8 @@ use tracing::warn;
|
||||
/// not use it. Its `cacheable: false` property disables sharing tool definitions across connections.
|
||||
const MCP_TOOL_CATALOG_CACHE_CAPABILITY: &str = "codex/tool-catalog-cache";
|
||||
const MCP_TOOL_CATALOG_CACHEABLE_PROPERTY: &str = "cacheable";
|
||||
// Bound only the model-visible attribution note, not plugin membership or tool availability.
|
||||
const MAX_PLUGIN_PROVENANCE_BYTES: usize = 1_024;
|
||||
pub(crate) const MCP_TOOLS_LIST_DURATION_METRIC: &str = "codex.mcp.tools.list.duration_ms";
|
||||
pub(crate) const MCP_TOOLS_FETCH_UNCACHED_DURATION_METRIC: &str =
|
||||
"codex.mcp.tools.fetch_uncached.duration_ms";
|
||||
@@ -766,18 +772,39 @@ fn add_plugin_provenance_to_tool(tool: &mut ToolInfo, plugin_names: &[String]) {
|
||||
return;
|
||||
}
|
||||
|
||||
let plugin_source_note = if plugin_names.len() == 1 {
|
||||
format!("This tool is part of plugin `{}`.", plugin_names[0])
|
||||
let plural = if plugin_names.len() == 1 { "" } else { "s" };
|
||||
let mut plugin_source_note = format!("This tool is part of plugin{plural} ");
|
||||
let mut shown = 0;
|
||||
for (index, name) in plugin_names.iter().enumerate() {
|
||||
let separator = if index == 0 { "" } else { ", " };
|
||||
let remaining = plugin_names.len() - index - 1;
|
||||
let ending = if remaining == 0 {
|
||||
".".to_string()
|
||||
} else {
|
||||
format!(", and {remaining} more.")
|
||||
};
|
||||
// Reserve the suffix and keep whole names, including multibyte UTF-8 characters.
|
||||
if plugin_source_note.len() + separator.len() + name.len() + 2 + ending.len()
|
||||
> MAX_PLUGIN_PROVENANCE_BYTES
|
||||
{
|
||||
break;
|
||||
}
|
||||
plugin_source_note.push_str(separator);
|
||||
plugin_source_note.push('`');
|
||||
plugin_source_note.push_str(name);
|
||||
plugin_source_note.push('`');
|
||||
shown += 1;
|
||||
}
|
||||
if shown == 0 {
|
||||
plugin_source_note = format!(
|
||||
"This tool is part of {} plugin{plural}.",
|
||||
plugin_names.len()
|
||||
);
|
||||
} else if shown < plugin_names.len() {
|
||||
plugin_source_note.push_str(&format!(", and {} more.", plugin_names.len() - shown));
|
||||
} else {
|
||||
format!(
|
||||
"This tool is part of plugins {}.",
|
||||
plugin_names
|
||||
.iter()
|
||||
.map(|plugin_name| format!("`{plugin_name}`"))
|
||||
.collect::<Vec<_>>()
|
||||
.join(", ")
|
||||
)
|
||||
};
|
||||
plugin_source_note.push('.');
|
||||
}
|
||||
let description = tool
|
||||
.tool
|
||||
.description
|
||||
|
||||
@@ -0,0 +1,84 @@
|
||||
//! Plugin attribution stays bounded without changing tool definitions or membership.
|
||||
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
fn app_tool(description: &str) -> ToolInfo {
|
||||
tool_info_from_listed_tool(
|
||||
CODEX_APPS_MCP_SERVER_NAME,
|
||||
/*is_codex_apps_mcp_server*/ true,
|
||||
/*server_instructions*/ None,
|
||||
ToolWithConnectorId {
|
||||
tool: RmcpTool::new("search", description.to_string(), Arc::default()),
|
||||
connector_id: Some("shared-connector".to_string()),
|
||||
connector_name: None,
|
||||
connector_description: None,
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn provenance_preserves_short_descriptions_and_membership() {
|
||||
for (description, names, expected_description) in [
|
||||
("Search", vec![], "Search"),
|
||||
("", vec!["Alpha"], "This tool is part of plugin `Alpha`."),
|
||||
(
|
||||
"Search",
|
||||
vec!["Alpha"],
|
||||
"Search. This tool is part of plugin `Alpha`.",
|
||||
),
|
||||
(
|
||||
"Search!",
|
||||
vec!["Alpha", "Beta"],
|
||||
"Search! This tool is part of plugins `Alpha`, `Beta`.",
|
||||
),
|
||||
] {
|
||||
let names = names.into_iter().map(str::to_string).collect::<Vec<_>>();
|
||||
let mut tool = app_tool(description);
|
||||
let mut expected = tool.clone();
|
||||
expected.tool.description = Some(expected_description.to_string().into());
|
||||
expected.plugin_display_names = names.clone();
|
||||
add_plugin_provenance_to_tool(&mut tool, &names);
|
||||
assert_eq!(tool, expected);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn provenance_bounds_aggregate_names_and_retains_all_associations() {
|
||||
let exact_name =
|
||||
"a".repeat(MAX_PLUGIN_PROVENANCE_BYTES - "This tool is part of plugin ``.".len());
|
||||
for names in [
|
||||
vec![exact_name.clone()],
|
||||
vec![format!("{exact_name}a")],
|
||||
vec!["界".repeat(MAX_PLUGIN_PROVENANCE_BYTES)],
|
||||
(0..512)
|
||||
.map(|index| format!("{index:03}{}", "界".repeat(61)))
|
||||
.collect(),
|
||||
] {
|
||||
let mut tool = app_tool("Search.");
|
||||
let original = tool.clone();
|
||||
add_plugin_provenance_to_tool(&mut tool, &names);
|
||||
let note = tool
|
||||
.tool
|
||||
.description
|
||||
.as_deref()
|
||||
.unwrap()
|
||||
.strip_prefix("Search. ")
|
||||
.unwrap();
|
||||
assert!(note.len() <= MAX_PLUGIN_PROVENANCE_BYTES);
|
||||
if names[0] == exact_name {
|
||||
assert_eq!(note.len(), MAX_PLUGIN_PROVENANCE_BYTES);
|
||||
} else if names.len() == 1 {
|
||||
assert_eq!(note, "This tool is part of 1 plugin.");
|
||||
} else {
|
||||
let shown = note.matches('`').count() / 2;
|
||||
assert!(shown > 0 && shown < names.len());
|
||||
assert!(note.starts_with(&format!("This tool is part of plugins `{}`", names[0])));
|
||||
assert!(note.ends_with(&format!(", and {} more.", names.len() - shown)));
|
||||
}
|
||||
let mut expected = original;
|
||||
expected.tool.description = tool.tool.description.clone();
|
||||
expected.plugin_display_names = names;
|
||||
assert_eq!(tool, expected);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user