external-agent-migration: avoid mixed MCP transport configs (#26435)

## Why MCP migration could recursively merge an imported server into an existing same-named Codex server. When one definition used stdio and the other used HTTP, this produced an invalid mixed configuration containing both `command` and `url`. ## What changed - Merge MCP configuration at the server level instead of field by field. - Preserve an existing same-named Codex MCP server unchanged. - Report only MCP servers that would actually be added during detection. - Add regression coverage for mixed command/HTTP source configurations. - Use neutral fixture names and reserved `example.com` URLs. ## Test plan - `just test -p codex-app-server repo_mcp` - 5 tests passed. - `just test -p codex-external-agent-migration mcp_migration_prefers_command_transport_for_mixed_server_config` - 1 test passed.
2026-07-01 00:31:56 +08:00 · 2026-06-04 14:16:03 -04:00
parent 881cf191d7
commit c8fdc74b42
3 changed files with 203 additions and 6 deletions
@@ -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::<TomlValue>(&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<Vec<String>> {
+    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}")))?;
@@ -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::<ExternalAgentConfigMigrationItem>::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();
@@ -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");