mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
feat: make built-in MCPs first-class runtime servers (#21356)
## DISCLAIMER This is experimental and no production service must rely on this ## Why Built-in MCPs are product-owned runtime capabilities, but they were previously flattened into the same config-backed stdio path as user-configured servers. That made them depend on a hidden `codex builtin-mcp` re-exec path, exposed them through config-oriented CLI flows, and erased distinctions the runtime needs to preserve—most notably whether an MCP call should count as external context for memory-mode pollution. ## What changed - Model product-owned built-ins separately from config-backed MCP servers via `BuiltinMcpServer` and `EffectiveMcpServer`. - Launch built-ins in process through a reusable async transport instead of the hidden `builtin-mcp` stdio subcommand. - Keep config-oriented CLI operations such as `codex mcp list/get/login/logout` scoped to configured servers, while merging built-ins only into the effective runtime server set. - Retain server metadata after launch so parallel-tool support and context classification come from the live server set; built-in `memories` is now classified as local Codex state rather than external context. ## Test plan - `cargo test -p codex-mcp` - `cargo test -p codex-core --test suite builtin_memories_mcp_call_does_not_mark_thread_memory_mode_polluted_when_configured` --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
1 parent
40e282849c
commit
b2268999fe
35 files changed
+668
-345
No files matched your search
@@ -12,6 +12,8 @@ use codex_rmcp_client::discover_streamable_http_oauth;
|
||||
use futures::future::join_all;
|
||||
use tracing::warn;
|
||||
|
||||
use crate::server::EffectiveMcpServer;
|
||||
|
||||
use super::CODEX_APPS_MCP_SERVER_NAME;
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
@@ -45,7 +47,7 @@ pub struct ResolvedMcpOAuthScopes {
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct McpAuthStatusEntry {
|
||||
pub config: McpServerConfig,
|
||||
pub config: Option<McpServerConfig>,
|
||||
pub auth_status: McpAuthStatus,
|
||||
}
|
||||
|
||||
@@ -131,29 +133,37 @@ pub async fn compute_auth_statuses<'a, I>(
|
||||
auth: Option<&CodexAuth>,
|
||||
) -> HashMap<String, McpAuthStatusEntry>
|
||||
where
|
||||
I: IntoIterator<Item = (&'a String, &'a McpServerConfig)>,
|
||||
I: IntoIterator<Item = (&'a String, &'a EffectiveMcpServer)>,
|
||||
{
|
||||
let futures = servers.into_iter().map(|(name, config)| {
|
||||
let futures = servers.into_iter().map(|(name, server)| {
|
||||
let name = name.clone();
|
||||
let config = config.clone();
|
||||
let config = server.configured_config().cloned();
|
||||
let has_runtime_auth = name == CODEX_APPS_MCP_SERVER_NAME
|
||||
&& auth.is_some_and(CodexAuth::uses_codex_backend)
|
||||
&& matches!(
|
||||
&config.transport,
|
||||
McpServerTransportConfig::StreamableHttp {
|
||||
bearer_token_env_var: None,
|
||||
..
|
||||
}
|
||||
);
|
||||
async move {
|
||||
let auth_status =
|
||||
match compute_auth_status(&name, &config, store_mode, has_runtime_auth).await {
|
||||
Ok(status) => status,
|
||||
Err(error) => {
|
||||
warn!("failed to determine auth status for MCP server `{name}`: {error:?}");
|
||||
McpAuthStatus::Unsupported
|
||||
&& config.as_ref().is_some_and(|config| {
|
||||
matches!(
|
||||
&config.transport,
|
||||
McpServerTransportConfig::StreamableHttp {
|
||||
bearer_token_env_var: None,
|
||||
..
|
||||
}
|
||||
};
|
||||
)
|
||||
});
|
||||
async move {
|
||||
let auth_status = match config.as_ref() {
|
||||
Some(config) => {
|
||||
match compute_auth_status(&name, config, store_mode, has_runtime_auth).await {
|
||||
Ok(status) => status,
|
||||
Err(error) => {
|
||||
warn!(
|
||||
"failed to determine auth status for MCP server `{name}`: {error:?}"
|
||||
);
|
||||
McpAuthStatus::Unsupported
|
||||
}
|
||||
}
|
||||
}
|
||||
None => McpAuthStatus::Unsupported,
|
||||
};
|
||||
let entry = McpAuthStatusEntry {
|
||||
config,
|
||||
auth_status,
|
||||
|
||||
@@ -38,6 +38,7 @@ use serde_json::Value;
|
||||
use crate::codex_apps::codex_apps_tools_cache_key;
|
||||
use crate::connection_manager::McpConnectionManager;
|
||||
use crate::runtime::McpRuntimeEnvironment;
|
||||
use crate::server::EffectiveMcpServer;
|
||||
|
||||
pub const CODEX_APPS_MCP_SERVER_NAME: &str = "codex_apps";
|
||||
const MCP_TOOL_NAME_PREFIX: &str = "mcp";
|
||||
@@ -99,8 +100,8 @@ pub struct McpPermissionPromptAutoApproveContext {
|
||||
/// approval/sandbox policy, locate OAuth state, and merge plugin-provided MCP
|
||||
/// servers. Request-scoped or auth-scoped state should not be stored here;
|
||||
/// thread those values explicitly into runtime entry points such as
|
||||
/// [`with_codex_apps_mcp`] and snapshot collection helpers so config objects do
|
||||
/// not go stale when auth changes.
|
||||
/// [`effective_mcp_servers`] and snapshot collection helpers so config objects
|
||||
/// do not go stale when auth changes.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct McpConfig {
|
||||
/// Base URL for ChatGPT-hosted app MCP servers, copied from the root config.
|
||||
@@ -128,12 +129,13 @@ pub struct McpConfig {
|
||||
/// ChatGPT auth is checked separately at runtime before the built-in apps
|
||||
/// MCP server is added.
|
||||
pub apps_enabled: bool,
|
||||
/// Configured MCP servers keyed by server name.
|
||||
/// Config-backed MCP servers keyed by server name.
|
||||
///
|
||||
/// This includes product-owned built-ins, user-configured servers, and
|
||||
/// plugin-provided servers. Runtime-only additions belong in
|
||||
/// Product-owned built-ins and runtime-only additions are merged later by
|
||||
/// [`effective_mcp_servers`].
|
||||
pub configured_mcp_servers: HashMap<String, McpServerConfig>,
|
||||
/// Product-owned built-ins enabled for this runtime config.
|
||||
pub builtin_mcp_servers: Vec<codex_builtin_mcps::BuiltinMcpServer>,
|
||||
/// Plugin metadata used to attribute MCP tools/connectors to plugin display names.
|
||||
pub plugin_capability_summaries: Vec<PluginCapabilitySummary>,
|
||||
}
|
||||
@@ -197,14 +199,14 @@ impl ToolPluginProvenance {
|
||||
}
|
||||
|
||||
pub fn with_codex_apps_mcp(
|
||||
mut servers: HashMap<String, McpServerConfig>,
|
||||
mut servers: HashMap<String, EffectiveMcpServer>,
|
||||
auth: Option<&CodexAuth>,
|
||||
config: &McpConfig,
|
||||
) -> HashMap<String, McpServerConfig> {
|
||||
) -> HashMap<String, EffectiveMcpServer> {
|
||||
if host_owned_codex_apps_enabled(config, auth) {
|
||||
servers.insert(
|
||||
CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
codex_apps_mcp_server_config(config),
|
||||
EffectiveMcpServer::configured(codex_apps_mcp_server_config(config)),
|
||||
);
|
||||
} else {
|
||||
servers.remove(CODEX_APPS_MCP_SERVER_NAME);
|
||||
@@ -223,8 +225,25 @@ pub fn configured_mcp_servers(config: &McpConfig) -> HashMap<String, McpServerCo
|
||||
pub fn effective_mcp_servers(
|
||||
config: &McpConfig,
|
||||
auth: Option<&CodexAuth>,
|
||||
) -> HashMap<String, McpServerConfig> {
|
||||
let servers = configured_mcp_servers(config);
|
||||
) -> HashMap<String, EffectiveMcpServer> {
|
||||
effective_mcp_servers_from_configured(configured_mcp_servers(config), config, auth)
|
||||
}
|
||||
|
||||
pub fn effective_mcp_servers_from_configured(
|
||||
configured_servers: HashMap<String, McpServerConfig>,
|
||||
config: &McpConfig,
|
||||
auth: Option<&CodexAuth>,
|
||||
) -> HashMap<String, EffectiveMcpServer> {
|
||||
let mut servers = configured_servers
|
||||
.into_iter()
|
||||
.map(|(name, server)| (name, EffectiveMcpServer::configured(server)))
|
||||
.collect::<HashMap<_, _>>();
|
||||
for builtin_server in &config.builtin_mcp_servers {
|
||||
servers.insert(
|
||||
builtin_server.name().to_string(),
|
||||
EffectiveMcpServer::builtin(*builtin_server),
|
||||
);
|
||||
}
|
||||
with_codex_apps_mcp(servers, auth, config)
|
||||
}
|
||||
|
||||
|
||||
@@ -13,6 +13,7 @@ use codex_protocol::protocol::GranularApprovalConfig;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
|
||||
fn test_mcp_config(codex_home: PathBuf) -> McpConfig {
|
||||
McpConfig {
|
||||
@@ -28,6 +29,7 @@ fn test_mcp_config(codex_home: PathBuf) -> McpConfig {
|
||||
use_legacy_landlock: false,
|
||||
apps_enabled: false,
|
||||
configured_mcp_servers: HashMap::new(),
|
||||
builtin_mcp_servers: Vec::new(),
|
||||
plugin_capability_summaries: Vec::new(),
|
||||
}
|
||||
}
|
||||
@@ -217,7 +219,10 @@ fn codex_apps_server_config_uses_legacy_codex_apps_path() {
|
||||
let server = servers
|
||||
.get(CODEX_APPS_MCP_SERVER_NAME)
|
||||
.expect("codex apps should be present when apps is enabled");
|
||||
let url = match &server.transport {
|
||||
let config = server
|
||||
.configured_config()
|
||||
.expect("codex apps should use configured transport");
|
||||
let url = match &config.transport {
|
||||
McpServerTransportConfig::StreamableHttp { url, .. } => url,
|
||||
_ => panic!("expected streamable http transport for codex apps"),
|
||||
};
|
||||
@@ -236,7 +241,10 @@ fn codex_apps_server_config_uses_configured_apps_mcp_path_override() {
|
||||
let server = servers
|
||||
.get(CODEX_APPS_MCP_SERVER_NAME)
|
||||
.expect("codex apps should be present when apps is enabled");
|
||||
let url = match &server.transport {
|
||||
let config = server
|
||||
.configured_config()
|
||||
.expect("codex apps should use configured transport");
|
||||
let url = match &config.transport {
|
||||
McpServerTransportConfig::StreamableHttp { url, .. } => url,
|
||||
_ => panic!("expected streamable http transport for codex apps"),
|
||||
};
|
||||
@@ -310,6 +318,16 @@ async fn effective_mcp_servers_preserve_user_servers_and_add_codex_apps() {
|
||||
.get(CODEX_APPS_MCP_SERVER_NAME)
|
||||
.expect("codex apps server should exist");
|
||||
|
||||
let sample = sample
|
||||
.configured_config()
|
||||
.expect("configured server should retain transport");
|
||||
let docs = docs
|
||||
.configured_config()
|
||||
.expect("configured server should retain transport");
|
||||
let codex_apps = codex_apps
|
||||
.configured_config()
|
||||
.expect("codex apps should use configured transport");
|
||||
|
||||
match &sample.transport {
|
||||
McpServerTransportConfig::StreamableHttp { url, .. } => {
|
||||
assert_eq!(url, "https://user.example/mcp");
|
||||
@@ -329,3 +347,53 @@ async fn effective_mcp_servers_preserve_user_servers_and_add_codex_apps() {
|
||||
other => panic!("expected streamable http transport, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn effective_mcp_servers_preserve_builtin_runtime_shape() {
|
||||
let mut config = test_mcp_config(PathBuf::from("/tmp"));
|
||||
config.builtin_mcp_servers = vec![codex_builtin_mcps::BuiltinMcpServer::Memories];
|
||||
|
||||
let effective = effective_mcp_servers(&config, /*auth*/ None);
|
||||
let memories = effective
|
||||
.get(codex_builtin_mcps::MEMORIES_MCP_SERVER_NAME)
|
||||
.expect("memories server should exist");
|
||||
|
||||
assert!(!crate::server::McpServerMetadata::from(memories).pollutes_memory);
|
||||
assert!(matches!(
|
||||
memories.launch(),
|
||||
crate::server::McpServerLaunch::Builtin(codex_builtin_mcps::BuiltinMcpServer::Memories)
|
||||
));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn builtin_memories_server_runs_in_process() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let mut config = test_mcp_config(codex_home.path().to_path_buf());
|
||||
config.builtin_mcp_servers = vec![codex_builtin_mcps::BuiltinMcpServer::Memories];
|
||||
|
||||
let snapshot = collect_mcp_server_status_snapshot_with_detail(
|
||||
&config,
|
||||
/*auth*/ None,
|
||||
"builtin-memories-test".to_string(),
|
||||
McpRuntimeEnvironment::new(
|
||||
Arc::new(codex_exec_server::Environment::default_for_tests()),
|
||||
codex_home.path().to_path_buf(),
|
||||
),
|
||||
McpSnapshotDetail::ToolsAndAuthOnly,
|
||||
)
|
||||
.await;
|
||||
|
||||
let tools = snapshot
|
||||
.tools_by_server
|
||||
.get(codex_builtin_mcps::MEMORIES_MCP_SERVER_NAME)
|
||||
.expect("memories tools should be listed");
|
||||
assert_eq!(
|
||||
tools
|
||||
.keys()
|
||||
.cloned()
|
||||
.collect::<std::collections::BTreeSet<_>>(),
|
||||
["list".to_string(), "read".to_string(), "search".to_string()]
|
||||
.into_iter()
|
||||
.collect()
|
||||
);
|
||||
}
|
||||
Reference in new issue
Block a user