From 76eb14c115b39bcb0b3c50f91c67d33632deaa45 Mon Sep 17 00:00:00 2001 From: stefanstokic-oai Date: Thu, 4 Jun 2026 13:36:21 -0400 Subject: [PATCH] external-agent-migration: avoid mixed MCP transport configs --- .../src/config/external_agent_config.rs | 48 ++++++- .../src/config/external_agent_config_tests.rs | 118 ++++++++++++++++++ codex-rs/external-agent-migration/src/lib.rs | 43 +++++++ 3 files changed, 203 insertions(+), 6 deletions(-) diff --git a/codex-rs/app-server/src/config/external_agent_config.rs b/codex-rs/app-server/src/config/external_agent_config.rs index 18c276f07d7..eca927fc23c 100644 --- a/codex-rs/app-server/src/config/external_agent_config.rs +++ b/codex-rs/app-server/src/config/external_agent_config.rs @@ -316,9 +316,8 @@ impl ExternalAgentConfigService { Some(self.external_agent_home.as_path()), mcp_settings.as_ref(), )?; - let mcp_server_names = migrated_mcp_server_names(&migrated_mcp); + let mut mcp_server_names = migrated_mcp_server_names(&migrated_mcp); if !is_empty_toml_table(&migrated_mcp) { - let mut should_include = true; if target_config.exists() { let existing_raw = fs::read_to_string(&target_config)?; let mut existing = if existing_raw.trim().is_empty() { @@ -328,10 +327,10 @@ impl ExternalAgentConfigService { invalid_data_error(format!("invalid existing config.toml: {err}")) })? }; - should_include = merge_missing_toml_values(&mut existing, &migrated_mcp)?; + mcp_server_names = merge_missing_mcp_servers(&mut existing, &migrated_mcp)?; } - if should_include { + if !mcp_server_names.is_empty() { items.push(ExternalAgentConfigMigrationItem { item_type: ExternalAgentConfigMigrationItemType::McpServerConfig, description: format!( @@ -341,7 +340,7 @@ impl ExternalAgentConfigService { ), cwd: cwd.clone(), details: Some(MigrationDetails { - mcp_servers: named_migrations(mcp_server_names.clone()), + mcp_servers: named_migrations(mcp_server_names), ..Default::default() }), }); @@ -863,7 +862,7 @@ impl ExternalAgentConfigService { toml::from_str::(&existing_raw) .map_err(|err| invalid_data_error(format!("invalid existing config.toml: {err}")))? }; - if merge_missing_toml_values(&mut existing, &migrated)? { + if !merge_missing_mcp_servers(&mut existing, &migrated)?.is_empty() { write_toml_file(&target_config, &existing)?; } Ok(()) @@ -1538,6 +1537,43 @@ fn merge_missing_toml_values(existing: &mut TomlValue, incoming: &TomlValue) -> } } +fn merge_missing_mcp_servers( + existing: &mut TomlValue, + incoming: &TomlValue, +) -> io::Result> { + let existing_root = existing + .as_table_mut() + .ok_or_else(|| invalid_data_error("expected existing config to be a TOML table"))?; + let incoming_root = incoming + .as_table() + .ok_or_else(|| invalid_data_error("expected migrated MCP config to be a TOML table"))?; + let Some(incoming_servers) = incoming_root.get("mcp_servers") else { + return Ok(Vec::new()); + }; + let incoming_servers = incoming_servers + .as_table() + .ok_or_else(|| invalid_data_error("expected migrated MCP servers to be a TOML table"))?; + let Some(existing_servers) = existing_root.get_mut("mcp_servers") else { + existing_root.insert( + "mcp_servers".to_string(), + TomlValue::Table(incoming_servers.clone()), + ); + return Ok(incoming_servers.keys().cloned().collect()); + }; + let Some(existing_servers) = existing_servers.as_table_mut() else { + return Ok(Vec::new()); + }; + + let mut merged_server_names = Vec::new(); + for (server_name, incoming_server) in incoming_servers { + if !existing_servers.contains_key(server_name) { + existing_servers.insert(server_name.clone(), incoming_server.clone()); + merged_server_names.push(server_name.clone()); + } + } + Ok(merged_server_names) +} + fn write_toml_file(path: &Path, value: &TomlValue) -> io::Result<()> { let serialized = toml::to_string_pretty(value) .map_err(|err| invalid_data_error(format!("failed to serialize config.toml: {err}")))?; diff --git a/codex-rs/app-server/src/config/external_agent_config_tests.rs b/codex-rs/app-server/src/config/external_agent_config_tests.rs index 47f93a5f8be..8c120fb51ba 100644 --- a/codex-rs/app-server/src/config/external_agent_config_tests.rs +++ b/codex-rs/app-server/src/config/external_agent_config_tests.rs @@ -638,6 +638,124 @@ Research with Codex carefully.""" assert_eq!(agent, expected_agent); } +#[tokio::test] +async fn import_repo_mcp_preserves_existing_same_named_server() { + let root = TempDir::new().expect("create tempdir"); + let repo_root = root.path().join("repo"); + fs::create_dir_all(repo_root.join(".git")).expect("create git dir"); + fs::write( + repo_root.join(".mcp.json"), + r#"{ + "mcpServers": { + "mixedTransport": { + "command": "mcp-remote-proxy", + "args": [ + "https://example.com/mixed-transport", + "--transport", + "http" + ], + "url": "https://example.com/mixed-transport" + } + } + }"#, + ) + .expect("write mcp"); + fs::create_dir_all(repo_root.join(".codex")).expect("create codex dir"); + let existing_config = r#"[mcp_servers.mixedTransport] +url = "https://example.com/mixed-transport" +"#; + fs::write( + repo_root.join(".codex").join("config.toml"), + existing_config, + ) + .expect("write config"); + + let service = service_for_paths( + root.path().join(EXTERNAL_AGENT_DIR), + root.path().join(".codex"), + ); + assert_eq!( + service + .detect(ExternalAgentConfigDetectOptions { + include_home: false, + cwds: Some(vec![repo_root.clone()]), + }) + .await + .expect("detect"), + Vec::::new() + ); + + service + .import(vec![ExternalAgentConfigMigrationItem { + item_type: ExternalAgentConfigMigrationItemType::McpServerConfig, + description: String::new(), + cwd: Some(repo_root.clone()), + details: None, + }]) + .await + .expect("import"); + + assert_eq!( + fs::read_to_string(repo_root.join(".codex").join("config.toml")).expect("read config"), + existing_config + ); +} + +#[tokio::test] +async fn detect_repo_mcp_lists_only_missing_servers() { + let root = TempDir::new().expect("create tempdir"); + let repo_root = root.path().join("repo"); + fs::create_dir_all(repo_root.join(".git")).expect("create git dir"); + fs::write( + repo_root.join(".mcp.json"), + r#"{ + "mcpServers": { + "docs": {"command": "docs-server"}, + "mixedTransport": {"command": "mcp-remote-proxy"} + } + }"#, + ) + .expect("write mcp"); + fs::create_dir_all(repo_root.join(".codex")).expect("create codex dir"); + fs::write( + repo_root.join(".codex").join("config.toml"), + r#"[mcp_servers.mixedTransport] +url = "https://example.com/mixed-transport" +"#, + ) + .expect("write config"); + + let items = service_for_paths( + root.path().join(EXTERNAL_AGENT_DIR), + root.path().join(".codex"), + ) + .detect(ExternalAgentConfigDetectOptions { + include_home: false, + cwds: Some(vec![repo_root.clone()]), + }) + .await + .expect("detect"); + + assert_eq!( + items, + vec![ExternalAgentConfigMigrationItem { + item_type: ExternalAgentConfigMigrationItemType::McpServerConfig, + description: format!( + "Migrate MCP servers from {} into {}", + repo_root.display(), + repo_root.join(".codex").join("config.toml").display() + ), + cwd: Some(repo_root), + details: Some(MigrationDetails { + mcp_servers: vec![NamedMigration { + name: "docs".to_string(), + }], + ..Default::default() + }), + }] + ); +} + #[tokio::test] async fn import_home_migrates_supported_config_fields_skills_and_agents_md() { let (_root, external_agent_home, codex_home) = fixture_paths(); diff --git a/codex-rs/external-agent-migration/src/lib.rs b/codex-rs/external-agent-migration/src/lib.rs index d7b8801796b..6ee0b38a24d 100644 --- a/codex-rs/external-agent-migration/src/lib.rs +++ b/codex-rs/external-agent-migration/src/lib.rs @@ -1451,6 +1451,49 @@ mod tests { ); } + #[test] + fn mcp_migration_prefers_command_transport_for_mixed_server_config() { + let root = tempfile::TempDir::new().expect("tempdir"); + fs::write( + root.path().join(".mcp.json"), + r#"{ + "mcpServers": { + "mixedTransport": { + "command": "mcp-remote-proxy", + "args": [ + "https://example.com/mixed-transport", + "--transport", + "http" + ], + "url": "https://example.com/mixed-transport" + } + } + }"#, + ) + .expect("write mcp"); + + assert_eq!( + build_mcp_config_from_external( + root.path(), + /*external_agent_home*/ None, + /*settings*/ None, + ) + .unwrap(), + toml::from_str( + r#" +[mcp_servers.mixedTransport] +command = "mcp-remote-proxy" +args = [ + "https://example.com/mixed-transport", + "--transport", + "http", +] +"# + ) + .unwrap() + ); + } + #[test] fn mcp_migration_skips_unsupported_transports() { let root = tempfile::TempDir::new().expect("tempdir");