diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index ba42adc6c2..2b0bda2a7e 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3623,6 +3623,7 @@ dependencies = [ "codex-utils-path-uri", "codex-utils-plugins", "futures", + "insta", "lru", "pretty_assertions", "regex-lite", diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index acc695eaf5..38a7f323a8 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -474,6 +474,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re let mut servers = load_global_mcp_servers(&codex_home) .await .with_context(|| format!("failed to load MCP servers from {}", codex_home.display()))?; + let credential_name = new_entry.oauth_credential_name(&name); servers.insert(name.clone(), new_entry); ConfigEditsBuilder::new(&codex_home) @@ -495,7 +496,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re oauth_config.discovered_scopes.clone(), ); perform_oauth_login_retry_without_scopes( - &name, + credential_name.as_ref(), &oauth_config.url, config.mcp_oauth_credentials_store_mode, config.auth_keyring_backend_kind(), @@ -1123,12 +1124,12 @@ fn validate_server_name(name: &str) -> Result<()> { let is_valid = !name.is_empty() && name .chars() - .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_'); + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | ':' | '@' | '/' | '.')); if is_valid { Ok(()) } else { - bail!("invalid server name '{name}' (use letters, numbers, '-', '_')"); + bail!("invalid server name '{name}' (use letters, numbers, '-', '_', ':', '@', '/', '.')"); } } diff --git a/codex-rs/cli/tests/mcp_add_remove.rs b/codex-rs/cli/tests/mcp_add_remove.rs index c5c65b9d9f..ca68faa061 100644 --- a/codex-rs/cli/tests/mcp_add_remove.rs +++ b/codex-rs/cli/tests/mcp_add_remove.rs @@ -73,6 +73,53 @@ async fn add_and_remove_server_updates_global_config() -> Result<()> { Ok(()) } +#[tokio::test] +async fn npm_server_names_round_trip_through_cli() -> Result<()> { + let codex_home = TempDir::new()?; + let name = "npm:@modelcontextprotocol/server-sequential.thinking"; + + codex_command(codex_home.path())? + .args(["mcp", "add", name, "--", "echo", "hello"]) + .assert() + .success(); + + let config = std::fs::read_to_string(codex_home.path().join("config.toml"))?; + assert!(config.contains(&format!("[mcp_servers.\"{name}\"]"))); + let servers = load_global_mcp_servers(codex_home.path()).await?; + assert_eq!( + servers.keys().map(String::as_str).collect::>(), + vec![name] + ); + + let output = codex_command(codex_home.path())? + .args(["mcp", "get", name, "--json"]) + .assert() + .success() + .get_output() + .stdout + .clone(); + let server: serde_json::Value = serde_json::from_slice(&output)?; + assert_eq!(server["name"], name); + + let output = codex_command(codex_home.path())? + .args(["mcp", "list", "--json"]) + .assert() + .success() + .get_output() + .stdout + .clone(); + let listed: Vec = serde_json::from_slice(&output)?; + assert_eq!(listed.len(), 1); + assert_eq!(listed[0]["name"], name); + + codex_command(codex_home.path())? + .args(["mcp", "remove", name]) + .assert() + .success(); + assert!(load_global_mcp_servers(codex_home.path()).await?.is_empty()); + Ok(()) +} + #[tokio::test] async fn add_and_login_discover_oauth_through_configured_http_proxy() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/codex-mcp/BUILD.bazel b/codex-rs/codex-mcp/BUILD.bazel index fbae63201a..9b1b77ac8e 100644 --- a/codex-rs/codex-mcp/BUILD.bazel +++ b/codex-rs/codex-mcp/BUILD.bazel @@ -3,4 +3,5 @@ load("//:defs.bzl", "codex_rust_crate") codex_rust_crate( name = "codex-mcp", crate_name = "codex_mcp", + test_data_extra = glob(["src/**/snapshots/**"]), ) diff --git a/codex-rs/codex-mcp/Cargo.toml b/codex-rs/codex-mcp/Cargo.toml index 3c6e7f0499..d989cf8b9c 100644 --- a/codex-rs/codex-mcp/Cargo.toml +++ b/codex-rs/codex-mcp/Cargo.toml @@ -46,6 +46,7 @@ url = { workspace = true } assert_matches = { workspace = true } codex-exec-server-test-support = { workspace = true } codex-plugin = { workspace = true } +insta = { workspace = true } pretty_assertions = { workspace = true } rmcp = { workspace = true, default-features = false, features = ["base64", "macros", "schemars", "server"] } tempfile = { workspace = true } diff --git a/codex-rs/codex-mcp/src/connection_manager/startup.rs b/codex-rs/codex-mcp/src/connection_manager/startup.rs index 3052e2e965..5d4c1fdd53 100644 --- a/codex-rs/codex-mcp/src/connection_manager/startup.rs +++ b/codex-rs/codex-mcp/src/connection_manager/startup.rs @@ -78,6 +78,15 @@ pub(super) fn mcp_init_error_display( error: &StartupOutcomeError, reason: Option, ) -> String { + let server_key = if !server_name.is_empty() + && server_name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') + { + server_name.to_string() + } else { + serde_json::Value::String(server_name.to_string()).to_string() + }; if let Some(McpServerTransportConfig::StreamableHttp { url, bearer_token_env_var, @@ -89,7 +98,7 @@ pub(super) fn mcp_init_error_display( && http_headers.as_ref().map(HashMap::is_empty).unwrap_or(true) { format!( - "GitHub MCP does not support OAuth. Log in by adding a personal access token (https://github.com/settings/personal-access-tokens) to your environment and config.toml:\n[mcp_servers.{server_name}]\nbearer_token_env_var = CODEX_GITHUB_PERSONAL_ACCESS_TOKEN" + "GitHub MCP does not support OAuth. Log in by adding a personal access token (https://github.com/settings/personal-access-tokens) to your environment and config.toml:\n[mcp_servers.{server_key}]\nbearer_token_env_var = CODEX_GITHUB_PERSONAL_ACCESS_TOKEN" ) } else if error.is_authentication_required() { let recovery_hint = if config.is_some_and(|config| !config.is_local_environment()) { @@ -116,7 +125,7 @@ pub(super) fn mcp_init_error_display( .unwrap_or(DEFAULT_STARTUP_TIMEOUT) .as_secs(); format!( - "MCP client for `{server_name}` timed out after {startup_timeout_secs} seconds. Add or adjust `startup_timeout_sec` in your config.toml:\n[mcp_servers.{server_name}]\nstartup_timeout_sec = XX" + "MCP client for `{server_name}` timed out after {startup_timeout_secs} seconds. Add or adjust `startup_timeout_sec` in your config.toml:\n[mcp_servers.{server_key}]\nstartup_timeout_sec = XX" ) } else { format!("MCP client for `{server_name}` failed to start: {error:#}") diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 8388d49c73..47aa905f6b 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1806,25 +1806,35 @@ fn test_normalize_tools_disambiguates_sanitized_namespace_collisions() { let tools = vec![ create_test_tool("basic-server", "lookup"), create_test_tool("basic_server", "query"), + create_test_tool("npm:@scope/package.name", "lookup"), + create_test_tool("npm__scope_package_name", "lookup"), ]; let model_tools = normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true, &[]); - assert_eq!(model_tools.len(), 2); + assert_eq!(model_tools.len(), 4); let mut namespaces = model_tools .iter() .map(|tool| tool.callable_namespace.as_str()) .collect::>(); namespaces.sort(); namespaces.dedup(); - assert_eq!(namespaces.len(), 2); + assert_eq!(namespaces.len(), 4); let raw_servers = model_tools .iter() .map(|tool| tool.server_name.as_str()) .collect::>(); - assert_eq!(raw_servers, HashSet::from(["basic-server", "basic_server"])); + assert_eq!( + raw_servers, + HashSet::from([ + "basic-server", + "basic_server", + "npm:@scope/package.name", + "npm__scope_package_name", + ]) + ); let model_names = model_tool_names(&model_tools); assert!( model_names.iter().all(is_code_mode_compatible_tool_name), @@ -4573,6 +4583,27 @@ fn mcp_init_error_display_reports_generic_errors() { assert_eq!(expected, display); } +#[test] +fn mcp_init_error_display_quotes_server_names() { + let github_config: McpServerConfig = serde_json::from_value(serde_json::json!({ + "url": "https://api.githubcopilot.com/mcp/", + })) + .expect("GitHub MCP configuration should deserialize"); + let error: StartupOutcomeError = anyhow::anyhow!("request timed out").into(); + let mut displays = Vec::new(); + for server_name in ["npm:@scope/package.name", "server.name"] { + for config in [None, Some(&github_config)] { + displays.push(mcp_init_error_display( + server_name, + config, + &error, + /*reason*/ None, + )); + } + } + insta::assert_snapshot!(displays.join("\n\n")); +} + #[test] fn mcp_init_error_display_includes_startup_timeout_hint() { let server_name = "slow"; diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index 563d6bedf1..1bf0cfc5b7 100644 --- a/codex-rs/codex-mcp/src/rmcp_client.rs +++ b/codex-rs/codex-mcp/src/rmcp_client.rs @@ -860,7 +860,7 @@ fn resolve_bearer_token( } fn validate_mcp_server_name(server_name: &str) -> Result<()> { - let re = regex_lite::Regex::new(r"^[a-zA-Z0-9_-]+$")?; + let re = regex_lite::Regex::new(r"^[a-zA-Z0-9_:@/.-]+$")?; if !re.is_match(server_name) { return Err(anyhow!( "Invalid MCP server name '{server_name}': must match pattern {pattern}", diff --git a/codex-rs/codex-mcp/src/snapshots/codex_mcp__connection_manager__tests__mcp_init_error_display_quotes_server_names.snap b/codex-rs/codex-mcp/src/snapshots/codex_mcp__connection_manager__tests__mcp_init_error_display_quotes_server_names.snap new file mode 100644 index 0000000000..555a69efc4 --- /dev/null +++ b/codex-rs/codex-mcp/src/snapshots/codex_mcp__connection_manager__tests__mcp_init_error_display_quotes_server_names.snap @@ -0,0 +1,19 @@ +--- +source: codex-mcp/src/connection_manager_tests.rs +expression: "displays.join(\"\\n\\n\")" +--- +MCP client for `npm:@scope/package.name` timed out after 30 seconds. Add or adjust `startup_timeout_sec` in your config.toml: +[mcp_servers."npm:@scope/package.name"] +startup_timeout_sec = XX + +GitHub MCP does not support OAuth. Log in by adding a personal access token (https://github.com/settings/personal-access-tokens) to your environment and config.toml: +[mcp_servers."npm:@scope/package.name"] +bearer_token_env_var = CODEX_GITHUB_PERSONAL_ACCESS_TOKEN + +MCP client for `server.name` timed out after 30 seconds. Add or adjust `startup_timeout_sec` in your config.toml: +[mcp_servers."server.name"] +startup_timeout_sec = XX + +GitHub MCP does not support OAuth. Log in by adding a personal access token (https://github.com/settings/personal-access-tokens) to your environment and config.toml: +[mcp_servers."server.name"] +bearer_token_env_var = CODEX_GITHUB_PERSONAL_ACCESS_TOKEN diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 6f1b29b0ff..76a92f8ec6 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -864,9 +864,11 @@ async fn environment_mcp_policy_filters_runtime_config_and_model_tools( Ok(()) } +#[test_case("rmcp", "mcp__rmcp"; "simple name")] +#[test_case("npm:@scope/package.name", "mcp__npm__scope_package_name"; "npm name")] #[tokio::test(flavor = "multi_thread", worker_threads = 1)] #[serial(mcp_test_value)] -async fn stdio_server_round_trip() -> anyhow::Result<()> { +async fn stdio_server_round_trip(server_name: &'static str, namespace: &str) -> anyhow::Result<()> { // TODO(anp): Remove after packaging a Windows stdio test server for Wine exec. skip_if_wine_exec!( Ok(()), @@ -878,8 +880,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { let call_id = "call-123"; let search_call_id = "search-rmcp-echo"; - let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}"); + let namespace = namespace.to_string(); let search_mock = mount_sse_once( &server, @@ -1013,7 +1014,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { search_description.len() < 513 * 1024, "the complete tool search description must remain bounded" ); - assert!(search_description.contains(&format!("- rmcp: {expected_description}"))); + assert!(search_description.contains(&format!("- {server_name}: {expected_description}"))); assert!(search_description.contains("🦀keep the complete MCP metadata")); let search_output = call_mock diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 6b7eaae501..2880d8cba1 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -831,6 +831,9 @@ fn load_oauth_tokens_from_file_with_lock_held( false } else { entry.server_url == url + // Escaped names may also match another server's stored, escaped name. + // Only accept a legacy unescaped entry under this identity's own key. + && (!server_name.starts_with("local:") || stored_key == &key) && (entry.server_name == local_server_name || (stored_key == &key && entry.server_name == server_name)) }; diff --git a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs index 37d82397d4..b8428f5e5e 100644 --- a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs +++ b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs @@ -110,6 +110,38 @@ fn sample_tokens() -> StoredOAuthTokens { } } +#[test] +fn file_credentials_keep_repeated_local_prefixes_isolated() -> Result<()> { + let _env = TempCodexHome::new(); + let config: codex_config::McpServerConfig = serde_json::from_value(serde_json::json!({ + "url": "https://example.test", + }))?; + let mut first = sample_tokens(); + first.server_name = config.oauth_credential_name("local:foo").into_owned(); + first.expires_at = None; + first.token_response.0.set_expires_in(None); + save_oauth_tokens_to_file(&first)?; + + let second_name = config.oauth_credential_name("local:local:foo"); + assert_eq!(load_oauth_tokens_from_file(&second_name, &first.url)?, None); + + let mut second = first.clone(); + second.server_name = second_name.into_owned(); + second + .token_response + .0 + .set_access_token(AccessToken::new("second-token".to_string())); + save_oauth_tokens_to_file(&second)?; + + for expected in [first, second] { + assert_eq!( + load_oauth_tokens_from_file(&expected.server_name, &expected.url)?, + Some(expected) + ); + } + Ok(()) +} + #[test] fn legacy_rmcp_oauth_keyring_credentials_remain_readable() -> Result<()> { let _env = TempCodexHome::new();