From e26a35e25379db5f4d72c351c5ca205c90f3c85e Mon Sep 17 00:00:00 2001 From: jif-oai Date: Thu, 21 May 2026 17:27:23 +0100 Subject: [PATCH 1/4] feat: codex mcp on new profile --- codex-rs/cli/src/main.rs | 13 ++- codex-rs/cli/src/mcp_cmd.rs | 113 +++++++++++++-------------- codex-rs/cli/tests/mcp_add_remove.rs | 59 ++++++++++++++ codex-rs/core/src/config/mod.rs | 21 ++++- 4 files changed, 144 insertions(+), 62 deletions(-) diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 4382dee29224..929d1b86542f 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -918,7 +918,9 @@ async fn cli_main(arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> { )?; // Propagate any root-level config overrides (e.g. `-c key=value`). prepend_config_flags(&mut mcp_cli.config_overrides, root_config_overrides.clone()); - mcp_cli.run().await?; + let loader_overrides = + loader_overrides_for_profile(interactive.config_profile_v2.as_ref())?; + mcp_cli.run(loader_overrides).await?; } Some(Subcommand::Plugin(plugin_cli)) => { reject_remote_mode_for_subcommand( @@ -1459,11 +1461,12 @@ fn profile_v2_for_subcommand<'a>( | Subcommand::Review(_) | Subcommand::Resume(_) | Subcommand::Fork(_) + | Subcommand::Mcp(_) | Subcommand::Debug(DebugCommand { subcommand: DebugSubcommand::PromptInput(_), }) => Ok(Some(profile_v2)), _ => anyhow::bail!( - "--profile only applies to runtime commands: `codex`, `codex exec`, `codex review`, `codex resume`, `codex fork`, and `codex debug prompt-input`." + "--profile only applies to runtime commands and `codex mcp`: `codex`, `codex exec`, `codex review`, `codex resume`, `codex fork`, `codex mcp`, and `codex debug prompt-input`." ), } } @@ -2264,6 +2267,12 @@ mod tests { .as_deref(), Some("work") ); + assert_eq!( + profile_v2_for_args(&["codex", "--profile", "work", "mcp", "list"]) + .expect("mcp supports profile-v2") + .as_deref(), + Some("work") + ); } #[test] diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index f52d24160ba9..fd93c2155018 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -11,9 +11,10 @@ use codex_config::types::McpServerConfig; use codex_config::types::McpServerTransportConfig; use codex_core::McpManager; use codex_core::config::Config; +use codex_core::config::ConfigBuilder; +use codex_core::config::LoaderOverrides; use codex_core::config::edit::ConfigEditsBuilder; -use codex_core::config::find_codex_home; -use codex_core::config::load_global_mcp_servers; +use codex_core::config::selected_user_mcp_servers; use codex_core_plugins::PluginsManager; use codex_mcp::McpOAuthLoginSupport; use codex_mcp::ResolvedMcpOAuthScopes; @@ -157,30 +158,32 @@ pub struct LogoutArgs { } impl McpCli { - pub async fn run(self) -> Result<()> { + pub async fn run(self, loader_overrides: LoaderOverrides) -> Result<()> { let McpCli { config_overrides, subcommand, } = self; + let config = load_config(&config_overrides, loader_overrides).await?; + match subcommand { McpSubcommand::List(args) => { - run_list(&config_overrides, args).await?; + run_list(&config, args).await?; } McpSubcommand::Get(args) => { - run_get(&config_overrides, args).await?; + run_get(&config, args).await?; } McpSubcommand::Add(args) => { - run_add(&config_overrides, args).await?; + run_add(&config, args).await?; } McpSubcommand::Remove(args) => { - run_remove(&config_overrides, args).await?; + run_remove(&config, args).await?; } McpSubcommand::Login(args) => { - run_login(&config_overrides, args).await?; + run_login(&config, args).await?; } McpSubcommand::Logout(args) => { - run_logout(&config_overrides, args).await?; + run_logout(&config, args).await?; } } @@ -239,15 +242,22 @@ async fn perform_oauth_login_retry_without_scopes( } } -async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Result<()> { - // Validate any provided overrides even though they are not currently applied. +async fn load_config( + config_overrides: &CliConfigOverrides, + loader_overrides: LoaderOverrides, +) -> Result { let overrides = config_overrides .parse_overrides() .map_err(anyhow::Error::msg)?; - let config = Config::load_with_cli_overrides(overrides) + ConfigBuilder::default() + .cli_overrides(overrides) + .loader_overrides(loader_overrides) + .build() .await - .context("failed to load configuration")?; + .context("failed to load configuration") +} +async fn run_add(config: &Config, add_args: AddArgs) -> Result<()> { let AddArgs { name, transport_args, @@ -255,10 +265,12 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re validate_server_name(&name)?; - let codex_home = find_codex_home().context("failed to resolve CODEX_HOME")?; - let mut servers = load_global_mcp_servers(&codex_home) - .await - .with_context(|| format!("failed to load MCP servers from {}", codex_home.display()))?; + let mut servers = selected_user_mcp_servers(config).with_context(|| { + format!( + "failed to load MCP servers from {}", + config.codex_home.display() + ) + })?; let transport = match transport_args { AddMcpTransportArgs { @@ -319,11 +331,16 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re servers.insert(name.clone(), new_entry); - ConfigEditsBuilder::new(&codex_home) + ConfigEditsBuilder::for_config(config) .replace_mcp_servers(&servers) .apply() .await - .with_context(|| format!("failed to write MCP servers to {}", codex_home.display()))?; + .with_context(|| { + format!( + "failed to write MCP servers to {}", + config.codex_home.display() + ) + })?; println!("Added global MCP server '{name}'."); @@ -359,28 +376,30 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re Ok(()) } -async fn run_remove(config_overrides: &CliConfigOverrides, remove_args: RemoveArgs) -> Result<()> { - config_overrides - .parse_overrides() - .map_err(anyhow::Error::msg)?; - +async fn run_remove(config: &Config, remove_args: RemoveArgs) -> Result<()> { let RemoveArgs { name } = remove_args; validate_server_name(&name)?; - let codex_home = find_codex_home().context("failed to resolve CODEX_HOME")?; - let mut servers = load_global_mcp_servers(&codex_home) - .await - .with_context(|| format!("failed to load MCP servers from {}", codex_home.display()))?; - + let mut servers = selected_user_mcp_servers(config).with_context(|| { + format!( + "failed to load MCP servers from {}", + config.codex_home.display() + ) + })?; let removed = servers.remove(&name).is_some(); if removed { - ConfigEditsBuilder::new(&codex_home) + ConfigEditsBuilder::for_config(config) .replace_mcp_servers(&servers) .apply() .await - .with_context(|| format!("failed to write MCP servers to {}", codex_home.display()))?; + .with_context(|| { + format!( + "failed to write MCP servers to {}", + config.codex_home.display() + ) + })?; } if removed { @@ -392,13 +411,7 @@ async fn run_remove(config_overrides: &CliConfigOverrides, remove_args: RemoveAr Ok(()) } -async fn run_login(config_overrides: &CliConfigOverrides, login_args: LoginArgs) -> Result<()> { - let overrides = config_overrides - .parse_overrides() - .map_err(anyhow::Error::msg)?; - let config = Config::load_with_cli_overrides(overrides) - .await - .context("failed to load configuration")?; +async fn run_login(config: &Config, login_args: LoginArgs) -> Result<()> { let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); @@ -446,13 +459,7 @@ async fn run_login(config_overrides: &CliConfigOverrides, login_args: LoginArgs) Ok(()) } -async fn run_logout(config_overrides: &CliConfigOverrides, logout_args: LogoutArgs) -> Result<()> { - let overrides = config_overrides - .parse_overrides() - .map_err(anyhow::Error::msg)?; - let config = Config::load_with_cli_overrides(overrides) - .await - .context("failed to load configuration")?; +async fn run_logout(config: &Config, logout_args: LogoutArgs) -> Result<()> { let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); @@ -478,13 +485,7 @@ async fn run_logout(config_overrides: &CliConfigOverrides, logout_args: LogoutAr Ok(()) } -async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) -> Result<()> { - let overrides = config_overrides - .parse_overrides() - .map_err(anyhow::Error::msg)?; - let config = Config::load_with_cli_overrides(overrides) - .await - .context("failed to load configuration")?; +async fn run_list(config: &Config, list_args: ListArgs) -> Result<()> { let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); @@ -734,13 +735,7 @@ async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) -> Ok(()) } -async fn run_get(config_overrides: &CliConfigOverrides, get_args: GetArgs) -> Result<()> { - let overrides = config_overrides - .parse_overrides() - .map_err(anyhow::Error::msg)?; - let config = Config::load_with_cli_overrides(overrides) - .await - .context("failed to load configuration")?; +async fn run_get(config: &Config, get_args: GetArgs) -> Result<()> { let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); diff --git a/codex-rs/cli/tests/mcp_add_remove.rs b/codex-rs/cli/tests/mcp_add_remove.rs index 15afaf0828f4..05ca541c4a32 100644 --- a/codex-rs/cli/tests/mcp_add_remove.rs +++ b/codex-rs/cli/tests/mcp_add_remove.rs @@ -68,6 +68,65 @@ async fn add_and_remove_server_updates_global_config() -> Result<()> { Ok(()) } +#[tokio::test] +async fn profile_add_list_and_remove_only_mutate_profile_config() -> Result<()> { + let codex_home = TempDir::new()?; + + let mut add_base_cmd = codex_command(codex_home.path())?; + add_base_cmd + .args(["mcp", "add", "base", "--url", "https://base.example/mcp"]) + .assert() + .success(); + + let mut add_profile_cmd = codex_command(codex_home.path())?; + add_profile_cmd + .args([ + "--profile", + "work", + "mcp", + "add", + "docs", + "--", + "docs-server", + ]) + .assert() + .success(); + + let profile_path = codex_home.path().join("work.config.toml"); + let profile_contents = std::fs::read_to_string(&profile_path)?; + assert!(profile_contents.contains("[mcp_servers.docs]")); + assert!(!profile_contents.contains("[mcp_servers.base]")); + + let base_servers = load_global_mcp_servers(codex_home.path()).await?; + assert!(base_servers.contains_key("base")); + assert!(!base_servers.contains_key("docs")); + + let mut list_profile_cmd = codex_command(codex_home.path())?; + let list_profile_output = list_profile_cmd + .args(["--profile", "work", "mcp", "list", "--json"]) + .output()?; + assert!(list_profile_output.status.success()); + let stdout = String::from_utf8(list_profile_output.stdout)?; + assert!(stdout.contains("\"name\": \"base\"")); + assert!(stdout.contains("\"name\": \"docs\"")); + + let mut remove_profile_cmd = codex_command(codex_home.path())?; + remove_profile_cmd + .args(["--profile", "work", "mcp", "remove", "docs"]) + .assert() + .success() + .stdout(contains("Removed global MCP server 'docs'.")); + + let profile_contents = std::fs::read_to_string(profile_path)?; + assert!(!profile_contents.contains("[mcp_servers.docs]")); + + let base_servers = load_global_mcp_servers(codex_home.path()).await?; + assert!(base_servers.contains_key("base")); + assert!(!base_servers.contains_key("docs")); + + Ok(()) +} + #[tokio::test] async fn add_with_env_preserves_key_order_and_values() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 75bcf5aa3cb7..5dfa3d38cd09 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1751,10 +1751,29 @@ pub async fn load_global_mcp_servers( ) .await?; let merged_toml = config_layer_stack.effective_config(); - let Some(servers_value) = merged_toml.get("mcp_servers") else { + mcp_servers_from_toml(&merged_toml) +} + +/// Returns MCP servers from the active user config file only. +/// +/// Under profile config this is the selected profile overlay rather than the +/// inherited base user config, so mutations can target the selected file +/// without copying inherited entries into it. +pub fn selected_user_mcp_servers( + config: &Config, +) -> std::io::Result> { + let Some(user_layer) = config.config_layer_stack.get_active_user_layer() else { return Ok(BTreeMap::new()); }; + mcp_servers_from_toml(&user_layer.config) +} +fn mcp_servers_from_toml( + config_toml: &TomlValue, +) -> std::io::Result> { + let Some(servers_value) = config_toml.get("mcp_servers") else { + return Ok(BTreeMap::new()); + }; ensure_no_inline_bearer_tokens(servers_value)?; servers_value From d4e523a42d9a73c5e81e636bc0bc8494a6120edd Mon Sep 17 00:00:00 2001 From: jif-oai Date: Thu, 21 May 2026 17:28:28 +0100 Subject: [PATCH 2/4] nit --- codex-rs/cli/src/mcp_cmd.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index fd93c2155018..66a047a557a2 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -415,7 +415,7 @@ async fn run_login(config: &Config, login_args: LoginArgs) -> Result<()> { let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); - let mcp_servers = mcp_manager.configured_servers(&config).await; + let mcp_servers = mcp_manager.configured_servers(config).await; let LoginArgs { name, scopes } = login_args; @@ -463,7 +463,7 @@ async fn run_logout(config: &Config, logout_args: LogoutArgs) -> Result<()> { let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); - let mcp_servers = mcp_manager.configured_servers(&config).await; + let mcp_servers = mcp_manager.configured_servers(config).await; let LogoutArgs { name } = logout_args; @@ -489,8 +489,8 @@ async fn run_list(config: &Config, list_args: ListArgs) -> Result<()> { let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); - let mcp_servers = mcp_manager.configured_servers(&config).await; - let effective_mcp_servers = mcp_manager.effective_servers(&config, /*auth*/ None).await; + let mcp_servers = mcp_manager.configured_servers(config).await; + let effective_mcp_servers = mcp_manager.effective_servers(config, /*auth*/ None).await; let mut entries: Vec<_> = mcp_servers.iter().collect(); entries.sort_by(|(a, _), (b, _)| a.cmp(b)); @@ -739,7 +739,7 @@ async fn run_get(config: &Config, get_args: GetArgs) -> Result<()> { let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); - let mcp_servers = mcp_manager.configured_servers(&config).await; + let mcp_servers = mcp_manager.configured_servers(config).await; let Some(server) = mcp_servers.get(&get_args.name) else { bail!("No MCP server named '{name}' found.", name = get_args.name); From 4f0bc7f963d92a820346d3d951d98beacef5e9c7 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Thu, 21 May 2026 18:08:57 +0100 Subject: [PATCH 3/4] Fix profile MCP server edits --- codex-rs/cli/src/mcp_cmd.rs | 78 +++++++++++----- codex-rs/cli/tests/mcp_add_remove.rs | 120 +++++++++++++++++++++++++ codex-rs/core/src/config/edit.rs | 120 +++++++++++++++++++------ codex-rs/core/src/config/edit_tests.rs | 54 +++++++++++ codex-rs/core/src/config/mod.rs | 22 +---- 5 files changed, 321 insertions(+), 73 deletions(-) diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index 66a047a557a2..4fae5920250f 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -6,6 +6,7 @@ use anyhow::Result; use anyhow::anyhow; use anyhow::bail; use clap::ArgGroup; +use codex_config::ConfigLayerStackOrdering; use codex_config::types::AppToolApproval; use codex_config::types::McpServerConfig; use codex_config::types::McpServerTransportConfig; @@ -14,7 +15,6 @@ use codex_core::config::Config; use codex_core::config::ConfigBuilder; use codex_core::config::LoaderOverrides; use codex_core::config::edit::ConfigEditsBuilder; -use codex_core::config::selected_user_mcp_servers; use codex_core_plugins::PluginsManager; use codex_mcp::McpOAuthLoginSupport; use codex_mcp::ResolvedMcpOAuthScopes; @@ -28,6 +28,7 @@ use codex_rmcp_client::delete_oauth_tokens; use codex_rmcp_client::perform_oauth_login; use codex_utils_cli::CliConfigOverrides; use codex_utils_cli::format_env_display; +use toml::Value as TomlValue; /// Subcommands: /// - `list` — list configured servers (with `--json`) @@ -265,12 +266,12 @@ async fn run_add(config: &Config, add_args: AddArgs) -> Result<()> { validate_server_name(&name)?; - let mut servers = selected_user_mcp_servers(config).with_context(|| { - format!( - "failed to load MCP servers from {}", - config.codex_home.display() - ) - })?; + let server_layers = user_mcp_server_layers(config, &name); + if server_layers.inherited { + bail!( + "MCP server '{name}' is inherited from the base user config; choose a different name for the profile server" + ); + } let transport = match transport_args { AddMcpTransportArgs { @@ -329,10 +330,8 @@ async fn run_add(config: &Config, add_args: AddArgs) -> Result<()> { tools: HashMap::new(), }; - servers.insert(name.clone(), new_entry); - ConfigEditsBuilder::for_config(config) - .replace_mcp_servers(&servers) + .set_mcp_server(&name, &new_entry) .apply() .await .with_context(|| { @@ -381,17 +380,22 @@ async fn run_remove(config: &Config, remove_args: RemoveArgs) -> Result<()> { validate_server_name(&name)?; - let mut servers = selected_user_mcp_servers(config).with_context(|| { - format!( - "failed to load MCP servers from {}", - config.codex_home.display() - ) - })?; - let removed = servers.remove(&name).is_some(); - - if removed { + let server_layers = user_mcp_server_layers(config, &name); + if server_layers.inherited { ConfigEditsBuilder::for_config(config) - .replace_mcp_servers(&servers) + .set_mcp_server_disabled_override(&name) + .apply() + .await + .with_context(|| { + format!( + "failed to write MCP servers to {}", + config.codex_home.display() + ) + })?; + println!("Disabled inherited MCP server '{name}'."); + } else if server_layers.selected { + ConfigEditsBuilder::for_config(config) + .remove_mcp_server(&name) .apply() .await .with_context(|| { @@ -400,9 +404,6 @@ async fn run_remove(config: &Config, remove_args: RemoveArgs) -> Result<()> { config.codex_home.display() ) })?; - } - - if removed { println!("Removed global MCP server '{name}'."); } else { println!("No MCP server named '{name}' found."); @@ -411,6 +412,37 @@ async fn run_remove(config: &Config, remove_args: RemoveArgs) -> Result<()> { Ok(()) } +#[derive(Debug, Default)] +struct UserMcpServerLayers { + selected: bool, + inherited: bool, +} + +fn user_mcp_server_layers(config: &Config, name: &str) -> UserMcpServerLayers { + let user_layers = config.config_layer_stack.get_user_layers( + ConfigLayerStackOrdering::HighestPrecedenceFirst, + /*include_disabled*/ false, + ); + let selected = user_layers + .first() + .is_some_and(|layer| config_has_mcp_server(&layer.config, name)); + let inherited = user_layers + .iter() + .skip(1) + .any(|layer| config_has_mcp_server(&layer.config, name)); + UserMcpServerLayers { + selected, + inherited, + } +} + +fn config_has_mcp_server(config: &TomlValue, name: &str) -> bool { + config + .get("mcp_servers") + .and_then(TomlValue::as_table) + .is_some_and(|servers| servers.contains_key(name)) +} + async fn run_login(config: &Config, login_args: LoginArgs) -> Result<()> { let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), diff --git a/codex-rs/cli/tests/mcp_add_remove.rs b/codex-rs/cli/tests/mcp_add_remove.rs index 05ca541c4a32..b82d01ef306f 100644 --- a/codex-rs/cli/tests/mcp_add_remove.rs +++ b/codex-rs/cli/tests/mcp_add_remove.rs @@ -127,6 +127,126 @@ async fn profile_add_list_and_remove_only_mutate_profile_config() -> Result<()> Ok(()) } +#[tokio::test] +async fn profile_add_and_remove_preserve_partial_inherited_server_override() -> Result<()> { + let codex_home = TempDir::new()?; + + let mut add_base_cmd = codex_command(codex_home.path())?; + add_base_cmd + .args(["mcp", "add", "base", "--", "base-server"]) + .assert() + .success(); + + let profile_path = codex_home.path().join("work.config.toml"); + std::fs::write( + &profile_path, + r#"[mcp_servers.base] +enabled = false +"#, + )?; + + let mut add_profile_cmd = codex_command(codex_home.path())?; + add_profile_cmd + .args([ + "--profile", + "work", + "mcp", + "add", + "docs", + "--", + "docs-server", + ]) + .assert() + .success(); + + let profile_contents = std::fs::read_to_string(&profile_path)?; + assert!(profile_contents.contains("[mcp_servers.base]")); + assert!(profile_contents.contains("enabled = false")); + assert!(profile_contents.contains("[mcp_servers.docs]")); + + let mut remove_profile_cmd = codex_command(codex_home.path())?; + remove_profile_cmd + .args(["--profile", "work", "mcp", "remove", "docs"]) + .assert() + .success(); + + let profile_contents = std::fs::read_to_string(profile_path)?; + assert!(profile_contents.contains("[mcp_servers.base]")); + assert!(profile_contents.contains("enabled = false")); + assert!(!profile_contents.contains("[mcp_servers.docs]")); + + Ok(()) +} + +#[tokio::test] +async fn profile_remove_disables_inherited_server() -> Result<()> { + let codex_home = TempDir::new()?; + + let mut add_base_cmd = codex_command(codex_home.path())?; + add_base_cmd + .args(["mcp", "add", "base", "--", "base-server"]) + .assert() + .success(); + + let mut remove_profile_cmd = codex_command(codex_home.path())?; + remove_profile_cmd + .args(["--profile", "work", "mcp", "remove", "base"]) + .assert() + .success() + .stdout(contains("Disabled inherited MCP server 'base'.")); + + let profile_contents = std::fs::read_to_string(codex_home.path().join("work.config.toml"))?; + assert!(profile_contents.contains("[mcp_servers.base]")); + assert!(profile_contents.contains("enabled = false")); + + let mut get_profile_cmd = codex_command(codex_home.path())?; + let get_profile_output = get_profile_cmd + .args(["--profile", "work", "mcp", "get", "base", "--json"]) + .output()?; + assert!(get_profile_output.status.success()); + let stdout = String::from_utf8(get_profile_output.stdout)?; + assert!(stdout.contains("\"enabled\": false")); + + Ok(()) +} + +#[tokio::test] +async fn profile_add_rejects_inherited_server_name() -> Result<()> { + let codex_home = TempDir::new()?; + + let mut add_base_cmd = codex_command(codex_home.path())?; + add_base_cmd + .args(["mcp", "add", "docs", "--", "base-docs-server"]) + .assert() + .success(); + + let mut add_profile_cmd = codex_command(codex_home.path())?; + add_profile_cmd + .args([ + "--profile", + "work", + "mcp", + "add", + "docs", + "--url", + "https://profile.example/mcp", + ]) + .assert() + .failure() + .stderr(contains( + "MCP server 'docs' is inherited from the base user config", + )); + + let mut list_profile_cmd = codex_command(codex_home.path())?; + list_profile_cmd + .args(["--profile", "work", "mcp", "list"]) + .assert() + .success() + .stdout(contains("base-docs-server")); + + Ok(()) +} + #[tokio::test] async fn add_with_env_preserves_key_order_and_values() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index 1b5cd879c45c..5685bb3c135d 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -56,6 +56,15 @@ pub enum ConfigEdit { RecordModelMigrationSeen { from: String, to: String }, /// Replace the entire `[mcp_servers]` table. ReplaceMcpServers(BTreeMap), + /// Set one entry under `[mcp_servers]` without changing sibling entries. + SetMcpServer { + name: String, + config: Box, + }, + /// Remove one entry under `[mcp_servers]`. + RemoveMcpServer { name: String }, + /// Replace one `[mcp_servers]` entry with a disabled profile override. + SetMcpServerDisabledOverride { name: String }, /// Add a disabled tool suggestion under `[tool_suggest].disabled_tools`. AddToolSuggestDisabledTool(ToolSuggestDisabledTool), /// Set or clear a skill config entry under `[[skills.config]]` by path. @@ -639,6 +648,13 @@ impl ConfigDocument { value(to.clone()), )), ConfigEdit::ReplaceMcpServers(servers) => Ok(self.replace_mcp_servers(servers)), + ConfigEdit::SetMcpServer { name, config } => Ok(self.set_mcp_server(name, config)), + ConfigEdit::RemoveMcpServer { name } => { + Ok(self.clear(Scope::Global, &["mcp_servers", name.as_str()])) + } + ConfigEdit::SetMcpServerDisabledOverride { name } => { + Ok(self.set_mcp_server_disabled_override(name)) + } ConfigEdit::AddToolSuggestDisabledTool(disabled_tool) => { Ok(self.add_tool_suggest_disabled_tool(disabled_tool)) } @@ -724,23 +740,7 @@ impl ConfigDocument { return self.clear(Scope::Global, &["mcp_servers"]); } - let root = self.doc.as_table_mut(); - if !root.contains_key("mcp_servers") { - root.insert( - "mcp_servers", - TomlItem::Table(document_helpers::new_implicit_table()), - ); - } - - let Some(item) = root.get_mut("mcp_servers") else { - return false; - }; - - if document_helpers::ensure_table_for_write(item).is_none() { - *item = TomlItem::Table(document_helpers::new_implicit_table()); - } - - let Some(table) = item.as_table_mut() else { + let Some(table) = self.mcp_servers_table_for_write() else { return false; }; @@ -755,23 +755,63 @@ impl ConfigDocument { } for (name, config) in servers { - if let Some(existing) = table.get_mut(name.as_str()) { - if let TomlItem::Value(value) = existing - && let Some(inline) = value.as_inline_table_mut() - { - let replacement = document_helpers::serialize_mcp_server_inline(config); - document_helpers::merge_inline_table(inline, replacement); - } else { - *existing = document_helpers::serialize_mcp_server(config); - } - } else { - table.insert(name, document_helpers::serialize_mcp_server(config)); - } + Self::write_mcp_server(table, name, config); } true } + fn set_mcp_server(&mut self, name: &str, config: &McpServerConfig) -> bool { + let Some(table) = self.mcp_servers_table_for_write() else { + return false; + }; + Self::write_mcp_server(table, name, config); + true + } + + fn set_mcp_server_disabled_override(&mut self, name: &str) -> bool { + let mut override_table = TomlTable::new(); + override_table.set_implicit(false); + override_table["enabled"] = value(false); + self.insert( + &["mcp_servers".to_string(), name.to_string()], + TomlItem::Table(override_table), + ) + } + + fn mcp_servers_table_for_write(&mut self) -> Option<&mut TomlTable> { + let root = self.doc.as_table_mut(); + if !root.contains_key("mcp_servers") { + root.insert( + "mcp_servers", + TomlItem::Table(document_helpers::new_implicit_table()), + ); + } + + let item = root.get_mut("mcp_servers")?; + + if document_helpers::ensure_table_for_write(item).is_none() { + *item = TomlItem::Table(document_helpers::new_implicit_table()); + } + + item.as_table_mut() + } + + fn write_mcp_server(table: &mut TomlTable, name: &str, config: &McpServerConfig) { + if let Some(existing) = table.get_mut(name) { + if let TomlItem::Value(value) = existing + && let Some(inline) = value.as_inline_table_mut() + { + let replacement = document_helpers::serialize_mcp_server_inline(config); + document_helpers::merge_inline_table(inline, replacement); + } else { + *existing = document_helpers::serialize_mcp_server(config); + } + } else { + table.insert(name, document_helpers::serialize_mcp_server(config)); + } + } + fn set_skill_config(&mut self, selector: SkillConfigSelector, enabled: bool) -> bool { let selector = match selector { SkillConfigSelector::Name(name) => SkillConfigSelector::Name(name.trim().to_string()), @@ -1234,6 +1274,28 @@ impl ConfigEditsBuilder { self } + pub fn set_mcp_server(mut self, name: &str, config: &McpServerConfig) -> Self { + self.edits.push(ConfigEdit::SetMcpServer { + name: name.to_string(), + config: Box::new(config.clone()), + }); + self + } + + pub fn remove_mcp_server(mut self, name: &str) -> Self { + self.edits.push(ConfigEdit::RemoveMcpServer { + name: name.to_string(), + }); + self + } + + pub fn set_mcp_server_disabled_override(mut self, name: &str) -> Self { + self.edits.push(ConfigEdit::SetMcpServerDisabledOverride { + name: name.to_string(), + }); + self + } + pub fn set_project_trust_level>( mut self, project_path: P, diff --git a/codex-rs/core/src/config/edit_tests.rs b/codex-rs/core/src/config/edit_tests.rs index 740d819aed2d..2450ee2e0b59 100644 --- a/codex-rs/core/src/config/edit_tests.rs +++ b/codex-rs/core/src/config/edit_tests.rs @@ -1031,6 +1031,60 @@ B = \"2\" assert_eq!(raw, expected); } +#[test] +fn blocking_point_mcp_server_edits_preserve_sibling_overrides() { + let tmp = tempdir().expect("tmpdir"); + let codex_home = tmp.path(); + std::fs::write( + codex_home.join(CONFIG_TOML_FILE), + r#"[mcp_servers.base] +enabled = false +"#, + ) + .expect("seed"); + + let docs = McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: "docs-server".to_string(), + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::new(), + }; + ConfigEditsBuilder::new(codex_home) + .set_mcp_server("docs", &docs) + .apply_blocking() + .expect("set docs"); + ConfigEditsBuilder::new(codex_home) + .set_mcp_server_disabled_override("docs") + .apply_blocking() + .expect("disable docs"); + + let contents = std::fs::read_to_string(codex_home.join(CONFIG_TOML_FILE)).expect("read config"); + let expected = r#"[mcp_servers.base] +enabled = false + +[mcp_servers.docs] +enabled = false +"#; + assert_eq!(contents, expected); +} + #[test] fn blocking_replace_mcp_servers_serializes_tool_approval_overrides() { let tmp = tempdir().expect("tmpdir"); diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 5dfa3d38cd09..63ee84296692 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1751,27 +1751,7 @@ pub async fn load_global_mcp_servers( ) .await?; let merged_toml = config_layer_stack.effective_config(); - mcp_servers_from_toml(&merged_toml) -} - -/// Returns MCP servers from the active user config file only. -/// -/// Under profile config this is the selected profile overlay rather than the -/// inherited base user config, so mutations can target the selected file -/// without copying inherited entries into it. -pub fn selected_user_mcp_servers( - config: &Config, -) -> std::io::Result> { - let Some(user_layer) = config.config_layer_stack.get_active_user_layer() else { - return Ok(BTreeMap::new()); - }; - mcp_servers_from_toml(&user_layer.config) -} - -fn mcp_servers_from_toml( - config_toml: &TomlValue, -) -> std::io::Result> { - let Some(servers_value) = config_toml.get("mcp_servers") else { + let Some(servers_value) = merged_toml.get("mcp_servers") else { return Ok(BTreeMap::new()); }; ensure_no_inline_bearer_tokens(servers_value)?; From d9f1e9811b160a8a56f7fba276a7acc240916367 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Thu, 21 May 2026 18:38:33 +0100 Subject: [PATCH 4/4] Limit MCP profiles to migration guard --- codex-rs/cli/src/mcp_cmd.rs | 175 ++++++++++++------------ codex-rs/cli/tests/mcp_add_remove.rs | 177 ++----------------------- codex-rs/core/src/config/edit.rs | 120 ++++------------- codex-rs/core/src/config/edit_tests.rs | 54 -------- codex-rs/core/src/config/mod.rs | 1 + 5 files changed, 125 insertions(+), 402 deletions(-) diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index 4fae5920250f..3104262c23fd 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -6,7 +6,6 @@ use anyhow::Result; use anyhow::anyhow; use anyhow::bail; use clap::ArgGroup; -use codex_config::ConfigLayerStackOrdering; use codex_config::types::AppToolApproval; use codex_config::types::McpServerConfig; use codex_config::types::McpServerTransportConfig; @@ -15,6 +14,8 @@ use codex_core::config::Config; use codex_core::config::ConfigBuilder; use codex_core::config::LoaderOverrides; use codex_core::config::edit::ConfigEditsBuilder; +use codex_core::config::find_codex_home; +use codex_core::config::load_global_mcp_servers; use codex_core_plugins::PluginsManager; use codex_mcp::McpOAuthLoginSupport; use codex_mcp::ResolvedMcpOAuthScopes; @@ -28,7 +29,6 @@ use codex_rmcp_client::delete_oauth_tokens; use codex_rmcp_client::perform_oauth_login; use codex_utils_cli::CliConfigOverrides; use codex_utils_cli::format_env_display; -use toml::Value as TomlValue; /// Subcommands: /// - `list` — list configured servers (with `--json`) @@ -165,26 +165,28 @@ impl McpCli { subcommand, } = self; - let config = load_config(&config_overrides, loader_overrides).await?; + if loader_overrides.user_config_profile.is_some() { + validate_profile_v2_migration(&config_overrides, loader_overrides).await?; + } match subcommand { McpSubcommand::List(args) => { - run_list(&config, args).await?; + run_list(&config_overrides, args).await?; } McpSubcommand::Get(args) => { - run_get(&config, args).await?; + run_get(&config_overrides, args).await?; } McpSubcommand::Add(args) => { - run_add(&config, args).await?; + run_add(&config_overrides, args).await?; } McpSubcommand::Remove(args) => { - run_remove(&config, args).await?; + run_remove(&config_overrides, args).await?; } McpSubcommand::Login(args) => { - run_login(&config, args).await?; + run_login(&config_overrides, args).await?; } McpSubcommand::Logout(args) => { - run_logout(&config, args).await?; + run_logout(&config_overrides, args).await?; } } @@ -243,10 +245,10 @@ async fn perform_oauth_login_retry_without_scopes( } } -async fn load_config( +async fn validate_profile_v2_migration( config_overrides: &CliConfigOverrides, loader_overrides: LoaderOverrides, -) -> Result { +) -> Result<()> { let overrides = config_overrides .parse_overrides() .map_err(anyhow::Error::msg)?; @@ -255,10 +257,19 @@ async fn load_config( .loader_overrides(loader_overrides) .build() .await - .context("failed to load configuration") + .context("failed to load configuration")?; + Ok(()) } -async fn run_add(config: &Config, add_args: AddArgs) -> Result<()> { +async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Result<()> { + // Validate any provided overrides even though they are not currently applied. + let overrides = config_overrides + .parse_overrides() + .map_err(anyhow::Error::msg)?; + let config = Config::load_with_cli_overrides(overrides) + .await + .context("failed to load configuration")?; + let AddArgs { name, transport_args, @@ -266,12 +277,10 @@ async fn run_add(config: &Config, add_args: AddArgs) -> Result<()> { validate_server_name(&name)?; - let server_layers = user_mcp_server_layers(config, &name); - if server_layers.inherited { - bail!( - "MCP server '{name}' is inherited from the base user config; choose a different name for the profile server" - ); - } + let codex_home = find_codex_home().context("failed to resolve CODEX_HOME")?; + let mut servers = load_global_mcp_servers(&codex_home) + .await + .with_context(|| format!("failed to load MCP servers from {}", codex_home.display()))?; let transport = match transport_args { AddMcpTransportArgs { @@ -330,16 +339,13 @@ async fn run_add(config: &Config, add_args: AddArgs) -> Result<()> { tools: HashMap::new(), }; - ConfigEditsBuilder::for_config(config) - .set_mcp_server(&name, &new_entry) + servers.insert(name.clone(), new_entry); + + ConfigEditsBuilder::new(&codex_home) + .replace_mcp_servers(&servers) .apply() .await - .with_context(|| { - format!( - "failed to write MCP servers to {}", - config.codex_home.display() - ) - })?; + .with_context(|| format!("failed to write MCP servers to {}", codex_home.display()))?; println!("Added global MCP server '{name}'."); @@ -375,35 +381,31 @@ async fn run_add(config: &Config, add_args: AddArgs) -> Result<()> { Ok(()) } -async fn run_remove(config: &Config, remove_args: RemoveArgs) -> Result<()> { +async fn run_remove(config_overrides: &CliConfigOverrides, remove_args: RemoveArgs) -> Result<()> { + config_overrides + .parse_overrides() + .map_err(anyhow::Error::msg)?; + let RemoveArgs { name } = remove_args; validate_server_name(&name)?; - let server_layers = user_mcp_server_layers(config, &name); - if server_layers.inherited { - ConfigEditsBuilder::for_config(config) - .set_mcp_server_disabled_override(&name) - .apply() - .await - .with_context(|| { - format!( - "failed to write MCP servers to {}", - config.codex_home.display() - ) - })?; - println!("Disabled inherited MCP server '{name}'."); - } else if server_layers.selected { - ConfigEditsBuilder::for_config(config) - .remove_mcp_server(&name) + let codex_home = find_codex_home().context("failed to resolve CODEX_HOME")?; + let mut servers = load_global_mcp_servers(&codex_home) + .await + .with_context(|| format!("failed to load MCP servers from {}", codex_home.display()))?; + + let removed = servers.remove(&name).is_some(); + + if removed { + ConfigEditsBuilder::new(&codex_home) + .replace_mcp_servers(&servers) .apply() .await - .with_context(|| { - format!( - "failed to write MCP servers to {}", - config.codex_home.display() - ) - })?; + .with_context(|| format!("failed to write MCP servers to {}", codex_home.display()))?; + } + + if removed { println!("Removed global MCP server '{name}'."); } else { println!("No MCP server named '{name}' found."); @@ -412,42 +414,17 @@ async fn run_remove(config: &Config, remove_args: RemoveArgs) -> Result<()> { Ok(()) } -#[derive(Debug, Default)] -struct UserMcpServerLayers { - selected: bool, - inherited: bool, -} - -fn user_mcp_server_layers(config: &Config, name: &str) -> UserMcpServerLayers { - let user_layers = config.config_layer_stack.get_user_layers( - ConfigLayerStackOrdering::HighestPrecedenceFirst, - /*include_disabled*/ false, - ); - let selected = user_layers - .first() - .is_some_and(|layer| config_has_mcp_server(&layer.config, name)); - let inherited = user_layers - .iter() - .skip(1) - .any(|layer| config_has_mcp_server(&layer.config, name)); - UserMcpServerLayers { - selected, - inherited, - } -} - -fn config_has_mcp_server(config: &TomlValue, name: &str) -> bool { - config - .get("mcp_servers") - .and_then(TomlValue::as_table) - .is_some_and(|servers| servers.contains_key(name)) -} - -async fn run_login(config: &Config, login_args: LoginArgs) -> Result<()> { +async fn run_login(config_overrides: &CliConfigOverrides, login_args: LoginArgs) -> Result<()> { + let overrides = config_overrides + .parse_overrides() + .map_err(anyhow::Error::msg)?; + let config = Config::load_with_cli_overrides(overrides) + .await + .context("failed to load configuration")?; let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); - let mcp_servers = mcp_manager.configured_servers(config).await; + let mcp_servers = mcp_manager.configured_servers(&config).await; let LoginArgs { name, scopes } = login_args; @@ -491,11 +468,17 @@ async fn run_login(config: &Config, login_args: LoginArgs) -> Result<()> { Ok(()) } -async fn run_logout(config: &Config, logout_args: LogoutArgs) -> Result<()> { +async fn run_logout(config_overrides: &CliConfigOverrides, logout_args: LogoutArgs) -> Result<()> { + let overrides = config_overrides + .parse_overrides() + .map_err(anyhow::Error::msg)?; + let config = Config::load_with_cli_overrides(overrides) + .await + .context("failed to load configuration")?; let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); - let mcp_servers = mcp_manager.configured_servers(config).await; + let mcp_servers = mcp_manager.configured_servers(&config).await; let LogoutArgs { name } = logout_args; @@ -517,12 +500,18 @@ async fn run_logout(config: &Config, logout_args: LogoutArgs) -> Result<()> { Ok(()) } -async fn run_list(config: &Config, list_args: ListArgs) -> Result<()> { +async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) -> Result<()> { + let overrides = config_overrides + .parse_overrides() + .map_err(anyhow::Error::msg)?; + let config = Config::load_with_cli_overrides(overrides) + .await + .context("failed to load configuration")?; let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); - let mcp_servers = mcp_manager.configured_servers(config).await; - let effective_mcp_servers = mcp_manager.effective_servers(config, /*auth*/ None).await; + let mcp_servers = mcp_manager.configured_servers(&config).await; + let effective_mcp_servers = mcp_manager.effective_servers(&config, /*auth*/ None).await; let mut entries: Vec<_> = mcp_servers.iter().collect(); entries.sort_by(|(a, _), (b, _)| a.cmp(b)); @@ -767,11 +756,17 @@ async fn run_list(config: &Config, list_args: ListArgs) -> Result<()> { Ok(()) } -async fn run_get(config: &Config, get_args: GetArgs) -> Result<()> { +async fn run_get(config_overrides: &CliConfigOverrides, get_args: GetArgs) -> Result<()> { + let overrides = config_overrides + .parse_overrides() + .map_err(anyhow::Error::msg)?; + let config = Config::load_with_cli_overrides(overrides) + .await + .context("failed to load configuration")?; let mcp_manager = McpManager::new(Arc::new(PluginsManager::new( config.codex_home.to_path_buf(), ))); - let mcp_servers = mcp_manager.configured_servers(config).await; + let mcp_servers = mcp_manager.configured_servers(&config).await; let Some(server) = mcp_servers.get(&get_args.name) else { bail!("No MCP server named '{name}' found.", name = get_args.name); diff --git a/codex-rs/cli/tests/mcp_add_remove.rs b/codex-rs/cli/tests/mcp_add_remove.rs index b82d01ef306f..ecfbed1264d5 100644 --- a/codex-rs/cli/tests/mcp_add_remove.rs +++ b/codex-rs/cli/tests/mcp_add_remove.rs @@ -69,180 +69,23 @@ async fn add_and_remove_server_updates_global_config() -> Result<()> { } #[tokio::test] -async fn profile_add_list_and_remove_only_mutate_profile_config() -> Result<()> { +async fn profile_mcp_reports_legacy_profile_migration() -> Result<()> { let codex_home = TempDir::new()?; - - let mut add_base_cmd = codex_command(codex_home.path())?; - add_base_cmd - .args(["mcp", "add", "base", "--url", "https://base.example/mcp"]) - .assert() - .success(); - - let mut add_profile_cmd = codex_command(codex_home.path())?; - add_profile_cmd - .args([ - "--profile", - "work", - "mcp", - "add", - "docs", - "--", - "docs-server", - ]) - .assert() - .success(); - - let profile_path = codex_home.path().join("work.config.toml"); - let profile_contents = std::fs::read_to_string(&profile_path)?; - assert!(profile_contents.contains("[mcp_servers.docs]")); - assert!(!profile_contents.contains("[mcp_servers.base]")); - - let base_servers = load_global_mcp_servers(codex_home.path()).await?; - assert!(base_servers.contains_key("base")); - assert!(!base_servers.contains_key("docs")); - - let mut list_profile_cmd = codex_command(codex_home.path())?; - let list_profile_output = list_profile_cmd - .args(["--profile", "work", "mcp", "list", "--json"]) - .output()?; - assert!(list_profile_output.status.success()); - let stdout = String::from_utf8(list_profile_output.stdout)?; - assert!(stdout.contains("\"name\": \"base\"")); - assert!(stdout.contains("\"name\": \"docs\"")); - - let mut remove_profile_cmd = codex_command(codex_home.path())?; - remove_profile_cmd - .args(["--profile", "work", "mcp", "remove", "docs"]) - .assert() - .success() - .stdout(contains("Removed global MCP server 'docs'.")); - - let profile_contents = std::fs::read_to_string(profile_path)?; - assert!(!profile_contents.contains("[mcp_servers.docs]")); - - let base_servers = load_global_mcp_servers(codex_home.path()).await?; - assert!(base_servers.contains_key("base")); - assert!(!base_servers.contains_key("docs")); - - Ok(()) -} - -#[tokio::test] -async fn profile_add_and_remove_preserve_partial_inherited_server_override() -> Result<()> { - let codex_home = TempDir::new()?; - - let mut add_base_cmd = codex_command(codex_home.path())?; - add_base_cmd - .args(["mcp", "add", "base", "--", "base-server"]) - .assert() - .success(); - - let profile_path = codex_home.path().join("work.config.toml"); std::fs::write( - &profile_path, - r#"[mcp_servers.base] -enabled = false + codex_home.path().join("config.toml"), + r#"[profiles.work] +model = "gpt-5" "#, )?; - let mut add_profile_cmd = codex_command(codex_home.path())?; - add_profile_cmd - .args([ - "--profile", - "work", - "mcp", - "add", - "docs", - "--", - "docs-server", - ]) - .assert() - .success(); - - let profile_contents = std::fs::read_to_string(&profile_path)?; - assert!(profile_contents.contains("[mcp_servers.base]")); - assert!(profile_contents.contains("enabled = false")); - assert!(profile_contents.contains("[mcp_servers.docs]")); - - let mut remove_profile_cmd = codex_command(codex_home.path())?; - remove_profile_cmd - .args(["--profile", "work", "mcp", "remove", "docs"]) - .assert() - .success(); - - let profile_contents = std::fs::read_to_string(profile_path)?; - assert!(profile_contents.contains("[mcp_servers.base]")); - assert!(profile_contents.contains("enabled = false")); - assert!(!profile_contents.contains("[mcp_servers.docs]")); - - Ok(()) -} - -#[tokio::test] -async fn profile_remove_disables_inherited_server() -> Result<()> { - let codex_home = TempDir::new()?; - - let mut add_base_cmd = codex_command(codex_home.path())?; - add_base_cmd - .args(["mcp", "add", "base", "--", "base-server"]) - .assert() - .success(); - - let mut remove_profile_cmd = codex_command(codex_home.path())?; - remove_profile_cmd - .args(["--profile", "work", "mcp", "remove", "base"]) - .assert() - .success() - .stdout(contains("Disabled inherited MCP server 'base'.")); - - let profile_contents = std::fs::read_to_string(codex_home.path().join("work.config.toml"))?; - assert!(profile_contents.contains("[mcp_servers.base]")); - assert!(profile_contents.contains("enabled = false")); - - let mut get_profile_cmd = codex_command(codex_home.path())?; - let get_profile_output = get_profile_cmd - .args(["--profile", "work", "mcp", "get", "base", "--json"]) - .output()?; - assert!(get_profile_output.status.success()); - let stdout = String::from_utf8(get_profile_output.stdout)?; - assert!(stdout.contains("\"enabled\": false")); - - Ok(()) -} - -#[tokio::test] -async fn profile_add_rejects_inherited_server_name() -> Result<()> { - let codex_home = TempDir::new()?; - - let mut add_base_cmd = codex_command(codex_home.path())?; - add_base_cmd - .args(["mcp", "add", "docs", "--", "base-docs-server"]) - .assert() - .success(); - - let mut add_profile_cmd = codex_command(codex_home.path())?; - add_profile_cmd - .args([ - "--profile", - "work", - "mcp", - "add", - "docs", - "--url", - "https://profile.example/mcp", - ]) - .assert() - .failure() - .stderr(contains( - "MCP server 'docs' is inherited from the base user config", - )); - - let mut list_profile_cmd = codex_command(codex_home.path())?; - list_profile_cmd + let mut list_cmd = codex_command(codex_home.path())?; + list_cmd .args(["--profile", "work", "mcp", "list"]) .assert() - .success() - .stdout(contains("base-docs-server")); + .failure() + .stderr(contains("--profile `work` cannot be used")) + .stderr(contains("[profiles.work]")) + .stderr(contains("work.config.toml")); Ok(()) } diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index 5685bb3c135d..1b5cd879c45c 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -56,15 +56,6 @@ pub enum ConfigEdit { RecordModelMigrationSeen { from: String, to: String }, /// Replace the entire `[mcp_servers]` table. ReplaceMcpServers(BTreeMap), - /// Set one entry under `[mcp_servers]` without changing sibling entries. - SetMcpServer { - name: String, - config: Box, - }, - /// Remove one entry under `[mcp_servers]`. - RemoveMcpServer { name: String }, - /// Replace one `[mcp_servers]` entry with a disabled profile override. - SetMcpServerDisabledOverride { name: String }, /// Add a disabled tool suggestion under `[tool_suggest].disabled_tools`. AddToolSuggestDisabledTool(ToolSuggestDisabledTool), /// Set or clear a skill config entry under `[[skills.config]]` by path. @@ -648,13 +639,6 @@ impl ConfigDocument { value(to.clone()), )), ConfigEdit::ReplaceMcpServers(servers) => Ok(self.replace_mcp_servers(servers)), - ConfigEdit::SetMcpServer { name, config } => Ok(self.set_mcp_server(name, config)), - ConfigEdit::RemoveMcpServer { name } => { - Ok(self.clear(Scope::Global, &["mcp_servers", name.as_str()])) - } - ConfigEdit::SetMcpServerDisabledOverride { name } => { - Ok(self.set_mcp_server_disabled_override(name)) - } ConfigEdit::AddToolSuggestDisabledTool(disabled_tool) => { Ok(self.add_tool_suggest_disabled_tool(disabled_tool)) } @@ -740,46 +724,6 @@ impl ConfigDocument { return self.clear(Scope::Global, &["mcp_servers"]); } - let Some(table) = self.mcp_servers_table_for_write() else { - return false; - }; - - let keys_to_remove: Vec = table - .iter() - .map(|(key, _)| key.to_string()) - .filter(|key| !servers.contains_key(key.as_str())) - .collect(); - - for key in keys_to_remove { - table.remove(&key); - } - - for (name, config) in servers { - Self::write_mcp_server(table, name, config); - } - - true - } - - fn set_mcp_server(&mut self, name: &str, config: &McpServerConfig) -> bool { - let Some(table) = self.mcp_servers_table_for_write() else { - return false; - }; - Self::write_mcp_server(table, name, config); - true - } - - fn set_mcp_server_disabled_override(&mut self, name: &str) -> bool { - let mut override_table = TomlTable::new(); - override_table.set_implicit(false); - override_table["enabled"] = value(false); - self.insert( - &["mcp_servers".to_string(), name.to_string()], - TomlItem::Table(override_table), - ) - } - - fn mcp_servers_table_for_write(&mut self) -> Option<&mut TomlTable> { let root = self.doc.as_table_mut(); if !root.contains_key("mcp_servers") { root.insert( @@ -788,28 +732,44 @@ impl ConfigDocument { ); } - let item = root.get_mut("mcp_servers")?; + let Some(item) = root.get_mut("mcp_servers") else { + return false; + }; if document_helpers::ensure_table_for_write(item).is_none() { *item = TomlItem::Table(document_helpers::new_implicit_table()); } - item.as_table_mut() - } + let Some(table) = item.as_table_mut() else { + return false; + }; + + let keys_to_remove: Vec = table + .iter() + .map(|(key, _)| key.to_string()) + .filter(|key| !servers.contains_key(key.as_str())) + .collect(); + + for key in keys_to_remove { + table.remove(&key); + } - fn write_mcp_server(table: &mut TomlTable, name: &str, config: &McpServerConfig) { - if let Some(existing) = table.get_mut(name) { - if let TomlItem::Value(value) = existing - && let Some(inline) = value.as_inline_table_mut() - { - let replacement = document_helpers::serialize_mcp_server_inline(config); - document_helpers::merge_inline_table(inline, replacement); + for (name, config) in servers { + if let Some(existing) = table.get_mut(name.as_str()) { + if let TomlItem::Value(value) = existing + && let Some(inline) = value.as_inline_table_mut() + { + let replacement = document_helpers::serialize_mcp_server_inline(config); + document_helpers::merge_inline_table(inline, replacement); + } else { + *existing = document_helpers::serialize_mcp_server(config); + } } else { - *existing = document_helpers::serialize_mcp_server(config); + table.insert(name, document_helpers::serialize_mcp_server(config)); } - } else { - table.insert(name, document_helpers::serialize_mcp_server(config)); } + + true } fn set_skill_config(&mut self, selector: SkillConfigSelector, enabled: bool) -> bool { @@ -1274,28 +1234,6 @@ impl ConfigEditsBuilder { self } - pub fn set_mcp_server(mut self, name: &str, config: &McpServerConfig) -> Self { - self.edits.push(ConfigEdit::SetMcpServer { - name: name.to_string(), - config: Box::new(config.clone()), - }); - self - } - - pub fn remove_mcp_server(mut self, name: &str) -> Self { - self.edits.push(ConfigEdit::RemoveMcpServer { - name: name.to_string(), - }); - self - } - - pub fn set_mcp_server_disabled_override(mut self, name: &str) -> Self { - self.edits.push(ConfigEdit::SetMcpServerDisabledOverride { - name: name.to_string(), - }); - self - } - pub fn set_project_trust_level>( mut self, project_path: P, diff --git a/codex-rs/core/src/config/edit_tests.rs b/codex-rs/core/src/config/edit_tests.rs index 2450ee2e0b59..740d819aed2d 100644 --- a/codex-rs/core/src/config/edit_tests.rs +++ b/codex-rs/core/src/config/edit_tests.rs @@ -1031,60 +1031,6 @@ B = \"2\" assert_eq!(raw, expected); } -#[test] -fn blocking_point_mcp_server_edits_preserve_sibling_overrides() { - let tmp = tempdir().expect("tmpdir"); - let codex_home = tmp.path(); - std::fs::write( - codex_home.join(CONFIG_TOML_FILE), - r#"[mcp_servers.base] -enabled = false -"#, - ) - .expect("seed"); - - let docs = McpServerConfig { - transport: McpServerTransportConfig::Stdio { - command: "docs-server".to_string(), - args: Vec::new(), - env: None, - env_vars: Vec::new(), - cwd: None, - }, - environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), - enabled: true, - required: false, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth: None, - oauth_resource: None, - tools: HashMap::new(), - }; - ConfigEditsBuilder::new(codex_home) - .set_mcp_server("docs", &docs) - .apply_blocking() - .expect("set docs"); - ConfigEditsBuilder::new(codex_home) - .set_mcp_server_disabled_override("docs") - .apply_blocking() - .expect("disable docs"); - - let contents = std::fs::read_to_string(codex_home.join(CONFIG_TOML_FILE)).expect("read config"); - let expected = r#"[mcp_servers.base] -enabled = false - -[mcp_servers.docs] -enabled = false -"#; - assert_eq!(contents, expected); -} - #[test] fn blocking_replace_mcp_servers_serializes_tool_approval_overrides() { let tmp = tempdir().expect("tmpdir"); diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 63ee84296692..75bcf5aa3cb7 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1754,6 +1754,7 @@ pub async fn load_global_mcp_servers( let Some(servers_value) = merged_toml.get("mcp_servers") else { return Ok(BTreeMap::new()); }; + ensure_no_inline_bearer_tokens(servers_value)?; servers_value