mirror of
https://github.com/openai/codex.git
synced 2026-10-27 08:01:52 +00:00
Enforce Agent Plugin runtime boundaries (#37027)
## What changed - Track Agent Plugin manifests through plugin, skill, and MCP loading so their capabilities use format-specific behavior without changing legacy plugins. - Discover only direct-child skills, exclude app and hook capabilities, isolate MCP data, and reject MCP configuration files that are non-regular or resolve outside the plugin root. - Bound model-visible skill instructions, plugin instructions, MCP descriptions, schemas, individual tools, and the aggregate Agent Plugin MCP tool set. - Stop MCP and OAuth redirects when Agent Plugins send configured or authorization headers, while retaining existing redirect behavior for legacy MCP servers. ## Testing - Add coverage for capability filtering, skill discovery, isolated MCP data and reserved-path expansion, unsafe MCP configuration files, context limits, and redirect handling. GitOrigin-RevId: c9af66b051269f3226628ca280a58d32c808c38f
This commit is contained in:
@@ -1700,10 +1700,17 @@ impl Config {
|
||||
{
|
||||
let mut plugin_mcp_servers = plugin.mcp_servers.clone();
|
||||
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(),
|
||||
);
|
||||
let attribution = if plugin.is_agent_plugin() {
|
||||
McpPluginAttribution::agent_plugin(
|
||||
plugin.config_name.clone(),
|
||||
plugin.display_name().to_string(),
|
||||
)
|
||||
} else {
|
||||
McpPluginAttribution::new(
|
||||
plugin.config_name.clone(),
|
||||
plugin.display_name().to_string(),
|
||||
)
|
||||
};
|
||||
for (name, plugin_server) in plugin_mcp_servers {
|
||||
catalog.register(McpServerRegistration::from_plugin(
|
||||
name,
|
||||
|
||||
@@ -12,6 +12,7 @@ use codex_protocol::request_user_input::RequestUserInputQuestion;
|
||||
use codex_protocol::request_user_input::RequestUserInputQuestionOption;
|
||||
use codex_protocol::request_user_input::RequestUserInputResponse;
|
||||
use codex_rmcp_client::OAuthDiscoveryTimeout;
|
||||
use codex_rmcp_client::StreamableHttpRedirectMode;
|
||||
use codex_rmcp_client::perform_oauth_login;
|
||||
use tokio_util::sync::CancellationToken;
|
||||
use tracing::warn;
|
||||
@@ -152,6 +153,7 @@ pub(crate) async fn maybe_install_mcp_dependencies(
|
||||
&server_config.transport,
|
||||
Arc::clone(&http_client),
|
||||
discovery_timeout,
|
||||
StreamableHttpRedirectMode::Legacy,
|
||||
)
|
||||
.await;
|
||||
let oauth_config = match login_support {
|
||||
|
||||
@@ -11,6 +11,9 @@ use codex_tools::ToolName;
|
||||
use tracing::instrument;
|
||||
use tracing::warn;
|
||||
|
||||
const MAX_AGENT_PLUGIN_MCP_SPEC_BYTES: usize = 8_000;
|
||||
const MAX_AGENT_PLUGIN_MCP_TOTAL_BYTES: usize = 64_000;
|
||||
|
||||
use crate::config::Config;
|
||||
use crate::tools::handlers::McpHandler;
|
||||
use crate::tools::registry::ToolRegistry;
|
||||
@@ -20,6 +23,7 @@ pub(crate) fn append_mcp_tools(
|
||||
all_mcp_tools: &[McpToolInfo],
|
||||
config: &Config,
|
||||
apps_enabled: bool,
|
||||
mcp_server_catalog: &codex_mcp::ResolvedMcpCatalog,
|
||||
search_tool_enabled: bool,
|
||||
registry: &mut ToolRegistry,
|
||||
) -> HashSet<ToolName> {
|
||||
@@ -35,11 +39,43 @@ pub(crate) fn append_mcp_tools(
|
||||
ToolExposure::Direct
|
||||
};
|
||||
let mut registered_tools = HashSet::new();
|
||||
let mut agent_plugin_bytes = 0usize;
|
||||
for tool in non_app_tools.chain(app_tools) {
|
||||
let tool_name = tool.canonical_tool_name();
|
||||
match McpHandler::new(tool.clone()) {
|
||||
let agent_plugin = mcp_server_catalog
|
||||
.server(&tool.server_name)
|
||||
.is_some_and(|server| server.source().is_agent_plugin());
|
||||
let handler = if agent_plugin {
|
||||
McpHandler::new_agent_plugin(tool.clone())
|
||||
} else {
|
||||
McpHandler::new(tool.clone())
|
||||
};
|
||||
match handler {
|
||||
Ok(handler) => {
|
||||
if registry.register_external_with_exposure(Arc::new(handler), exposure) {
|
||||
let fits_agent_budget = if agent_plugin {
|
||||
handler.model_spec_bytes().is_ok_and(|bytes| {
|
||||
if bytes > MAX_AGENT_PLUGIN_MCP_SPEC_BYTES {
|
||||
return false;
|
||||
}
|
||||
let next = agent_plugin_bytes.saturating_add(bytes);
|
||||
if next <= MAX_AGENT_PLUGIN_MCP_TOTAL_BYTES {
|
||||
agent_plugin_bytes = next;
|
||||
true
|
||||
} else {
|
||||
false
|
||||
}
|
||||
})
|
||||
} else {
|
||||
true
|
||||
};
|
||||
let tool_exposure = if fits_agent_budget {
|
||||
exposure
|
||||
} else {
|
||||
ToolExposure::Hidden
|
||||
};
|
||||
if registry.register_external_with_exposure(Arc::new(handler), tool_exposure)
|
||||
&& fits_agent_budget
|
||||
{
|
||||
registered_tools.insert(tool_name);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2,6 +2,9 @@ use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use codex_mcp::McpPluginAttribution;
|
||||
use codex_mcp::McpServerRegistration;
|
||||
use codex_mcp::ResolvedMcpCatalog;
|
||||
use codex_mcp::ToolInfo;
|
||||
use codex_tools::ToolExposure;
|
||||
use codex_tools::ToolName;
|
||||
@@ -75,12 +78,29 @@ fn runtimes_by_name(
|
||||
config: &Config,
|
||||
apps_enabled: bool,
|
||||
search_tool_enabled: bool,
|
||||
) -> HashMap<ToolName, ToolExposure> {
|
||||
runtimes_by_name_with_catalog(
|
||||
tools,
|
||||
config,
|
||||
apps_enabled,
|
||||
&ResolvedMcpCatalog::default(),
|
||||
search_tool_enabled,
|
||||
)
|
||||
}
|
||||
|
||||
fn runtimes_by_name_with_catalog(
|
||||
tools: &[ToolInfo],
|
||||
config: &Config,
|
||||
apps_enabled: bool,
|
||||
mcp_server_catalog: &ResolvedMcpCatalog,
|
||||
search_tool_enabled: bool,
|
||||
) -> HashMap<ToolName, ToolExposure> {
|
||||
let mut registry = ToolRegistry::default();
|
||||
append_mcp_tools(
|
||||
tools,
|
||||
config,
|
||||
apps_enabled,
|
||||
mcp_server_catalog,
|
||||
search_tool_enabled,
|
||||
&mut registry,
|
||||
);
|
||||
@@ -90,6 +110,91 @@ fn runtimes_by_name(
|
||||
.collect()
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn agent_plugin_budget_hides_only_overflow_agent_tools() {
|
||||
let codex_home = tempdir().expect("tempdir should succeed");
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
"[mcp_servers.agent]\ncommand = \"echo\"\n",
|
||||
)
|
||||
.expect("write config");
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.build()
|
||||
.await
|
||||
.expect("config should build");
|
||||
let agent_config = config.mcp_servers.get()["agent"].clone();
|
||||
let legacy_config = agent_config.clone();
|
||||
let mut catalog = ResolvedMcpCatalog::builder();
|
||||
catalog.register(McpServerRegistration::from_plugin(
|
||||
"agent".to_string(),
|
||||
McpPluginAttribution::agent_plugin("agent@test".to_string(), "Agent".to_string()),
|
||||
/*plugin_order*/ 0,
|
||||
agent_config,
|
||||
));
|
||||
catalog.register(McpServerRegistration::from_plugin(
|
||||
"legacy".to_string(),
|
||||
McpPluginAttribution::new("legacy@test".to_string(), "Legacy".to_string()),
|
||||
/*plugin_order*/ 1,
|
||||
legacy_config,
|
||||
));
|
||||
let catalog = catalog.build();
|
||||
let mut tools = (0..40)
|
||||
.map(|index| {
|
||||
let name = format!("tool_{index}");
|
||||
let mut tool = make_mcp_tool(
|
||||
"agent",
|
||||
&name,
|
||||
"mcp__agent",
|
||||
&name,
|
||||
/*connector_id*/ None,
|
||||
/*connector_name*/ None,
|
||||
);
|
||||
tool.namespace_description = Some("n".repeat(1_000));
|
||||
tool.tool.description = Some("d".repeat(1_000).into());
|
||||
tool
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
let oversized_name = "x".repeat(MAX_AGENT_PLUGIN_MCP_SPEC_BYTES);
|
||||
let oversized_agent_tool = make_mcp_tool(
|
||||
"agent",
|
||||
"oversized_agent_tool",
|
||||
"mcp__agent",
|
||||
&oversized_name,
|
||||
/*connector_id*/ None,
|
||||
/*connector_name*/ None,
|
||||
);
|
||||
tools.push(oversized_agent_tool.clone());
|
||||
let legacy_tool = make_mcp_tool(
|
||||
"legacy",
|
||||
"legacy_tool",
|
||||
"mcp__legacy",
|
||||
&oversized_name,
|
||||
/*connector_id*/ None,
|
||||
/*connector_name*/ None,
|
||||
);
|
||||
tools.push(legacy_tool.clone());
|
||||
|
||||
let runtimes = runtimes_by_name_with_catalog(
|
||||
&tools, &config, /*apps_enabled*/ false, &catalog, /*search_tool_enabled*/ false,
|
||||
);
|
||||
let agent_exposures = tools[..40]
|
||||
.iter()
|
||||
.map(|tool| runtimes[&tool.canonical_tool_name()])
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
assert!(agent_exposures.contains(&ToolExposure::Direct));
|
||||
assert!(agent_exposures.contains(&ToolExposure::Hidden));
|
||||
assert_eq!(
|
||||
runtimes[&oversized_agent_tool.canonical_tool_name()],
|
||||
ToolExposure::Hidden
|
||||
);
|
||||
assert_eq!(
|
||||
runtimes[&legacy_tool.canonical_tool_name()],
|
||||
ToolExposure::Direct
|
||||
);
|
||||
}
|
||||
|
||||
fn with_visibility(mut tool: ToolInfo, visibility: &[&str]) -> ToolInfo {
|
||||
tool.tool.meta = Some(MetaObject(
|
||||
serde_json::json!({ "ui": { "visibility": visibility } })
|
||||
@@ -240,6 +345,7 @@ async fn app_tool_registration_uses_trusted_catalog_metadata_and_preserves_sourc
|
||||
&mcp_tools,
|
||||
&config,
|
||||
/*apps_enabled*/ true,
|
||||
&ResolvedMcpCatalog::default(),
|
||||
/*search_tool_enabled*/ false,
|
||||
&mut registry,
|
||||
);
|
||||
|
||||
@@ -18,6 +18,7 @@ fn plugin(config_name: &str, display_name: &str) -> PluginCapabilitySummary {
|
||||
PluginCapabilitySummary {
|
||||
config_name: config_name.to_string(),
|
||||
display_name: display_name.to_string(),
|
||||
plugin_namespace: None,
|
||||
description: None,
|
||||
has_skills: true,
|
||||
mcp_server_names: Vec::new(),
|
||||
|
||||
@@ -3,6 +3,11 @@ use crate::context::AvailablePluginsInstructions;
|
||||
#[cfg(test)]
|
||||
use crate::context::ContextualUserFragment;
|
||||
use crate::plugins::PluginCapabilitySummary;
|
||||
use codex_utils_string::take_bytes_at_char_boundary;
|
||||
|
||||
const MAX_EXPLICIT_PLUGIN_INSTRUCTIONS_BYTES: usize = 4 * 1024;
|
||||
const TRUNCATED_PLUGIN_INSTRUCTIONS_SUFFIX: &str =
|
||||
"\n- Additional plugin capabilities omitted to fit the context limit.";
|
||||
|
||||
#[cfg(test)]
|
||||
pub(crate) fn render_plugins_section(plugins: &[PluginCapabilitySummary]) -> Option<String> {
|
||||
@@ -20,9 +25,12 @@ pub(crate) fn render_explicit_plugin_instructions(
|
||||
)];
|
||||
|
||||
if plugin.has_skills {
|
||||
let skill_namespace = plugin
|
||||
.plugin_namespace
|
||||
.as_deref()
|
||||
.unwrap_or(plugin.display_name.as_str());
|
||||
lines.push(format!(
|
||||
"- Skills from this plugin are prefixed with `{}:`.",
|
||||
plugin.display_name
|
||||
"- Skills from this plugin are prefixed with `{skill_namespace}:`."
|
||||
));
|
||||
}
|
||||
|
||||
@@ -54,7 +62,18 @@ pub(crate) fn render_explicit_plugin_instructions(
|
||||
|
||||
lines.push("Use these plugin-associated capabilities to help solve the task.".to_string());
|
||||
|
||||
Some(lines.join("\n"))
|
||||
Some(bound_explicit_plugin_instructions(lines.join("\n")))
|
||||
}
|
||||
|
||||
fn bound_explicit_plugin_instructions(rendered: String) -> String {
|
||||
if rendered.len() <= MAX_EXPLICIT_PLUGIN_INSTRUCTIONS_BYTES {
|
||||
return rendered;
|
||||
}
|
||||
|
||||
let max_prefix_bytes = MAX_EXPLICIT_PLUGIN_INSTRUCTIONS_BYTES
|
||||
.saturating_sub(TRUNCATED_PLUGIN_INSTRUCTIONS_SUFFIX.len());
|
||||
let prefix = take_bytes_at_char_boundary(&rendered, max_prefix_bytes);
|
||||
format!("{prefix}{TRUNCATED_PLUGIN_INSTRUCTIONS_SUFFIX}")
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -11,6 +11,7 @@ fn render_plugins_section_keeps_plugin_usage_guidance_without_listing_plugins()
|
||||
let rendered = render_plugins_section(&[PluginCapabilitySummary {
|
||||
config_name: "sample@test".to_string(),
|
||||
display_name: "sample".to_string(),
|
||||
plugin_namespace: None,
|
||||
description: Some("inspect sample data".to_string()),
|
||||
has_skills: true,
|
||||
..PluginCapabilitySummary::default()
|
||||
@@ -21,3 +22,43 @@ fn render_plugins_section_keeps_plugin_usage_guidance_without_listing_plugins()
|
||||
|
||||
assert_eq!(rendered, expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn explicit_plugin_instructions_use_manifest_namespace_for_skills() {
|
||||
let rendered = render_explicit_plugin_instructions(
|
||||
&PluginCapabilitySummary {
|
||||
config_name: "acme.tools@test".to_string(),
|
||||
display_name: "Acme Developer Tools".to_string(),
|
||||
plugin_namespace: Some("acme.tools".to_string()),
|
||||
has_skills: true,
|
||||
..PluginCapabilitySummary::default()
|
||||
},
|
||||
&[],
|
||||
&[],
|
||||
)
|
||||
.expect("skill capability should render");
|
||||
|
||||
assert!(rendered.contains("`acme.tools:`"));
|
||||
assert!(!rendered.contains("`Acme Developer Tools:`"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn explicit_plugin_instructions_are_bounded() {
|
||||
let servers = (0..1_024)
|
||||
.map(|index| format!("server-{index}"))
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
let rendered = render_explicit_plugin_instructions(
|
||||
&PluginCapabilitySummary {
|
||||
config_name: "sample@test".to_string(),
|
||||
display_name: "sample".to_string(),
|
||||
..PluginCapabilitySummary::default()
|
||||
},
|
||||
&servers,
|
||||
&[],
|
||||
)
|
||||
.expect("MCP capability should render");
|
||||
|
||||
assert!(rendered.len() <= MAX_EXPLICIT_PLUGIN_INSTRUCTIONS_BYTES);
|
||||
assert!(rendered.ends_with(TRUNCATED_PLUGIN_INSTRUCTIONS_SUFFIX));
|
||||
}
|
||||
|
||||
@@ -23,6 +23,7 @@ use codex_tools::ToolName;
|
||||
use codex_tools::ToolSearchInfo;
|
||||
use codex_tools::ToolSearchSourceInfo;
|
||||
use codex_tools::ToolSpec;
|
||||
use codex_tools::agent_plugin_mcp_tool_to_responses_api_tool;
|
||||
use codex_tools::mcp_tool_to_responses_api_tool;
|
||||
use codex_utils_string::take_bytes_at_char_boundary;
|
||||
use futures::future::BoxFuture;
|
||||
@@ -31,6 +32,7 @@ use serde_json::Value;
|
||||
|
||||
const LEGACY_MCP_TOOL_NAME_PREFIX: &str = "mcp__";
|
||||
const MCP_TOOL_NAME_DELIMITER: &str = "__";
|
||||
const MAX_AGENT_PLUGIN_MCP_NAMESPACE_DESCRIPTION_BYTES: usize = 1_000;
|
||||
const MAX_MCP_NAMESPACE_DESCRIPTION_BYTES: usize = 512 * 1024;
|
||||
|
||||
pub struct McpHandler {
|
||||
@@ -40,10 +42,38 @@ pub struct McpHandler {
|
||||
|
||||
impl McpHandler {
|
||||
pub fn new(tool_info: ToolInfo) -> Result<Self, serde_json::Error> {
|
||||
let spec = create_tool_spec(&tool_info)?;
|
||||
Self::with_agent_plugin(tool_info, /*agent_plugin*/ false)
|
||||
}
|
||||
|
||||
pub fn new_agent_plugin(tool_info: ToolInfo) -> Result<Self, serde_json::Error> {
|
||||
Self::with_agent_plugin(tool_info, /*agent_plugin*/ true)
|
||||
}
|
||||
|
||||
fn with_agent_plugin(
|
||||
mut tool_info: ToolInfo,
|
||||
agent_plugin: bool,
|
||||
) -> Result<Self, serde_json::Error> {
|
||||
if agent_plugin {
|
||||
tool_info.namespace_description =
|
||||
tool_info
|
||||
.namespace_description
|
||||
.as_deref()
|
||||
.map(|description| {
|
||||
take_bytes_at_char_boundary(
|
||||
description,
|
||||
MAX_AGENT_PLUGIN_MCP_NAMESPACE_DESCRIPTION_BYTES,
|
||||
)
|
||||
.to_string()
|
||||
});
|
||||
}
|
||||
let spec = create_tool_spec(&tool_info, agent_plugin)?;
|
||||
Ok(Self { tool_info, spec })
|
||||
}
|
||||
|
||||
pub(crate) fn model_spec_bytes(&self) -> Result<usize, serde_json::Error> {
|
||||
serde_json::to_vec(&self.spec).map(|spec| spec.len())
|
||||
}
|
||||
|
||||
fn hook_tool_name(&self) -> HookToolName {
|
||||
HookToolName::new(ensure_mcp_prefix(&join_tool_name(&self.tool_name())))
|
||||
}
|
||||
@@ -236,9 +266,16 @@ impl CoreToolRuntime for McpHandler {
|
||||
}
|
||||
}
|
||||
|
||||
fn create_tool_spec(tool_info: &ToolInfo) -> Result<ToolSpec, serde_json::Error> {
|
||||
fn create_tool_spec(
|
||||
tool_info: &ToolInfo,
|
||||
agent_plugin: bool,
|
||||
) -> Result<ToolSpec, serde_json::Error> {
|
||||
let tool_name = tool_info.canonical_tool_name();
|
||||
let tool = mcp_tool_to_responses_api_tool(&tool_name, &tool_info.tool)?;
|
||||
let tool = if agent_plugin {
|
||||
agent_plugin_mcp_tool_to_responses_api_tool(&tool_name, &tool_info.tool)?
|
||||
} else {
|
||||
mcp_tool_to_responses_api_tool(&tool_name, &tool_info.tool)?
|
||||
};
|
||||
let description = tool_info
|
||||
.namespace_description
|
||||
.as_deref()
|
||||
|
||||
@@ -89,6 +89,27 @@ fn mcp_namespace_descriptions_are_bounded_at_512_kib() {
|
||||
assert_eq!(namespace.description, expected_description);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn agent_plugin_namespace_descriptions_use_the_stricter_bound() {
|
||||
let expected_description = "é".repeat(MAX_AGENT_PLUGIN_MCP_NAMESPACE_DESCRIPTION_BYTES / 2);
|
||||
let mut info = tool_info();
|
||||
info.namespace_description = Some(format!("{expected_description}overflow"));
|
||||
let handler = McpHandler::new_agent_plugin(info).expect("MCP tool spec should build");
|
||||
let search_info = handler.search_info().expect("MCP search info");
|
||||
|
||||
assert_eq!(
|
||||
search_info.source_info,
|
||||
Some(ToolSearchSourceInfo {
|
||||
name: "Calendar".to_string(),
|
||||
description: Some(expected_description.clone()),
|
||||
})
|
||||
);
|
||||
let LoadableToolSpec::Namespace(namespace) = search_info.entry.output else {
|
||||
panic!("expected namespace search output");
|
||||
};
|
||||
assert_eq!(namespace.description, expected_description);
|
||||
}
|
||||
|
||||
fn tool_info() -> ToolInfo {
|
||||
ToolInfo {
|
||||
server_name: "codex-apps".to_string(),
|
||||
|
||||
@@ -150,6 +150,7 @@ pub(crate) fn build_tool_router(
|
||||
mcp.tools(),
|
||||
&turn_context.config,
|
||||
apps_enabled,
|
||||
&mcp.config().mcp_server_catalog,
|
||||
search_tool_enabled(turn_context),
|
||||
&mut registry,
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user