mirror of
https://github.com/openai/codex.git
synced 2026-11-13 06:45:07 +00:00
Support package-style MCP server names (#41700)
## What changed - Allow MCP server names to contain `:`, `@`, `/`, and `.`, enabling names such as `npm:@modelcontextprotocol/server-sequential.thinking`. - Preserve these names across `mcp add`, `get`, `list`, and `remove`, runtime tool namespaces, and OAuth credential lookup. - Quote non-bare server names in generated `config.toml` recovery hints and keep similarly escaped OAuth credential names isolated. ## Testing - Add CLI and runtime round-trip coverage for npm-style names. - Add snapshot coverage for quoted recovery hints and a regression test for OAuth credential-name collisions. GitOrigin-RevId: 97e9353cdbc544db4eb2377ddbfa1d2d539b1e04
This commit is contained in:
1 parent
88f776588f
commit
94cbbddafc
12 files changed
+159
-13
No files matched your search
Generated
+1
@@ -3623,6 +3623,7 @@ dependencies = [
|
|||||||
"codex-utils-path-uri",
|
"codex-utils-path-uri",
|
||||||
"codex-utils-plugins",
|
"codex-utils-plugins",
|
||||||
"futures",
|
"futures",
|
||||||
|
"insta",
|
||||||
"lru",
|
"lru",
|
||||||
"pretty_assertions",
|
"pretty_assertions",
|
||||||
"regex-lite",
|
"regex-lite",
|
||||||
|
|||||||
@@ -474,6 +474,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re
|
|||||||
let mut servers = load_global_mcp_servers(&codex_home)
|
let mut servers = load_global_mcp_servers(&codex_home)
|
||||||
.await
|
.await
|
||||||
.with_context(|| format!("failed to load MCP servers from {}", codex_home.display()))?;
|
.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);
|
servers.insert(name.clone(), new_entry);
|
||||||
|
|
||||||
ConfigEditsBuilder::new(&codex_home)
|
ConfigEditsBuilder::new(&codex_home)
|
||||||
@@ -495,7 +496,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re
|
|||||||
oauth_config.discovered_scopes.clone(),
|
oauth_config.discovered_scopes.clone(),
|
||||||
);
|
);
|
||||||
perform_oauth_login_retry_without_scopes(
|
perform_oauth_login_retry_without_scopes(
|
||||||
&name,
|
credential_name.as_ref(),
|
||||||
&oauth_config.url,
|
&oauth_config.url,
|
||||||
config.mcp_oauth_credentials_store_mode,
|
config.mcp_oauth_credentials_store_mode,
|
||||||
config.auth_keyring_backend_kind(),
|
config.auth_keyring_backend_kind(),
|
||||||
@@ -1123,12 +1124,12 @@ fn validate_server_name(name: &str) -> Result<()> {
|
|||||||
let is_valid = !name.is_empty()
|
let is_valid = !name.is_empty()
|
||||||
&& name
|
&& name
|
||||||
.chars()
|
.chars()
|
||||||
.all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_');
|
.all(|c| c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | ':' | '@' | '/' | '.'));
|
||||||
|
|
||||||
if is_valid {
|
if is_valid {
|
||||||
Ok(())
|
Ok(())
|
||||||
} else {
|
} else {
|
||||||
bail!("invalid server name '{name}' (use letters, numbers, '-', '_')");
|
bail!("invalid server name '{name}' (use letters, numbers, '-', '_', ':', '@', '/', '.')");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -73,6 +73,53 @@ async fn add_and_remove_server_updates_global_config() -> Result<()> {
|
|||||||
Ok(())
|
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<_>>(),
|
||||||
|
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::Value> = 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]
|
#[tokio::test]
|
||||||
async fn add_and_login_discover_oauth_through_configured_http_proxy() -> Result<()> {
|
async fn add_and_login_discover_oauth_through_configured_http_proxy() -> Result<()> {
|
||||||
let codex_home = TempDir::new()?;
|
let codex_home = TempDir::new()?;
|
||||||
|
|||||||
@@ -3,4 +3,5 @@ load("//:defs.bzl", "codex_rust_crate")
|
|||||||
codex_rust_crate(
|
codex_rust_crate(
|
||||||
name = "codex-mcp",
|
name = "codex-mcp",
|
||||||
crate_name = "codex_mcp",
|
crate_name = "codex_mcp",
|
||||||
|
test_data_extra = glob(["src/**/snapshots/**"]),
|
||||||
)
|
)
|
||||||
@@ -46,6 +46,7 @@ url = { workspace = true }
|
|||||||
assert_matches = { workspace = true }
|
assert_matches = { workspace = true }
|
||||||
codex-exec-server-test-support = { workspace = true }
|
codex-exec-server-test-support = { workspace = true }
|
||||||
codex-plugin = { workspace = true }
|
codex-plugin = { workspace = true }
|
||||||
|
insta = { workspace = true }
|
||||||
pretty_assertions = { workspace = true }
|
pretty_assertions = { workspace = true }
|
||||||
rmcp = { workspace = true, default-features = false, features = ["base64", "macros", "schemars", "server"] }
|
rmcp = { workspace = true, default-features = false, features = ["base64", "macros", "schemars", "server"] }
|
||||||
tempfile = { workspace = true }
|
tempfile = { workspace = true }
|
||||||
@@ -78,6 +78,15 @@ pub(super) fn mcp_init_error_display(
|
|||||||
error: &StartupOutcomeError,
|
error: &StartupOutcomeError,
|
||||||
reason: Option<McpStartupFailureReason>,
|
reason: Option<McpStartupFailureReason>,
|
||||||
) -> String {
|
) -> 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 {
|
if let Some(McpServerTransportConfig::StreamableHttp {
|
||||||
url,
|
url,
|
||||||
bearer_token_env_var,
|
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)
|
&& http_headers.as_ref().map(HashMap::is_empty).unwrap_or(true)
|
||||||
{
|
{
|
||||||
format!(
|
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() {
|
} else if error.is_authentication_required() {
|
||||||
let recovery_hint = if config.is_some_and(|config| !config.is_local_environment()) {
|
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)
|
.unwrap_or(DEFAULT_STARTUP_TIMEOUT)
|
||||||
.as_secs();
|
.as_secs();
|
||||||
format!(
|
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 {
|
} else {
|
||||||
format!("MCP client for `{server_name}` failed to start: {error:#}")
|
format!("MCP client for `{server_name}` failed to start: {error:#}")
|
||||||
|
|||||||
@@ -1806,25 +1806,35 @@ fn test_normalize_tools_disambiguates_sanitized_namespace_collisions() {
|
|||||||
let tools = vec![
|
let tools = vec![
|
||||||
create_test_tool("basic-server", "lookup"),
|
create_test_tool("basic-server", "lookup"),
|
||||||
create_test_tool("basic_server", "query"),
|
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 =
|
let model_tools =
|
||||||
normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true, &[]);
|
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
|
let mut namespaces = model_tools
|
||||||
.iter()
|
.iter()
|
||||||
.map(|tool| tool.callable_namespace.as_str())
|
.map(|tool| tool.callable_namespace.as_str())
|
||||||
.collect::<Vec<_>>();
|
.collect::<Vec<_>>();
|
||||||
namespaces.sort();
|
namespaces.sort();
|
||||||
namespaces.dedup();
|
namespaces.dedup();
|
||||||
assert_eq!(namespaces.len(), 2);
|
assert_eq!(namespaces.len(), 4);
|
||||||
|
|
||||||
let raw_servers = model_tools
|
let raw_servers = model_tools
|
||||||
.iter()
|
.iter()
|
||||||
.map(|tool| tool.server_name.as_str())
|
.map(|tool| tool.server_name.as_str())
|
||||||
.collect::<HashSet<_>>();
|
.collect::<HashSet<_>>();
|
||||||
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);
|
let model_names = model_tool_names(&model_tools);
|
||||||
assert!(
|
assert!(
|
||||||
model_names.iter().all(is_code_mode_compatible_tool_name),
|
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);
|
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]
|
#[test]
|
||||||
fn mcp_init_error_display_includes_startup_timeout_hint() {
|
fn mcp_init_error_display_includes_startup_timeout_hint() {
|
||||||
let server_name = "slow";
|
let server_name = "slow";
|
||||||
|
|||||||
@@ -860,7 +860,7 @@ fn resolve_bearer_token(
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn validate_mcp_server_name(server_name: &str) -> Result<()> {
|
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) {
|
if !re.is_match(server_name) {
|
||||||
return Err(anyhow!(
|
return Err(anyhow!(
|
||||||
"Invalid MCP server name '{server_name}': must match pattern {pattern}",
|
"Invalid MCP server name '{server_name}': must match pattern {pattern}",
|
||||||
|
|||||||
+19
@@ -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
|
||||||
@@ -864,9 +864,11 @@ async fn environment_mcp_policy_filters_runtime_config_and_model_tools(
|
|||||||
Ok(())
|
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)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
|
||||||
#[serial(mcp_test_value)]
|
#[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.
|
// TODO(anp): Remove after packaging a Windows stdio test server for Wine exec.
|
||||||
skip_if_wine_exec!(
|
skip_if_wine_exec!(
|
||||||
Ok(()),
|
Ok(()),
|
||||||
@@ -878,8 +880,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> {
|
|||||||
|
|
||||||
let call_id = "call-123";
|
let call_id = "call-123";
|
||||||
let search_call_id = "search-rmcp-echo";
|
let search_call_id = "search-rmcp-echo";
|
||||||
let server_name = "rmcp";
|
let namespace = namespace.to_string();
|
||||||
let namespace = format!("mcp__{server_name}");
|
|
||||||
|
|
||||||
let search_mock = mount_sse_once(
|
let search_mock = mount_sse_once(
|
||||||
&server,
|
&server,
|
||||||
@@ -1013,7 +1014,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> {
|
|||||||
search_description.len() < 513 * 1024,
|
search_description.len() < 513 * 1024,
|
||||||
"the complete tool search description must remain bounded"
|
"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"));
|
assert!(search_description.contains("🦀keep the complete MCP metadata"));
|
||||||
|
|
||||||
let search_output = call_mock
|
let search_output = call_mock
|
||||||
|
|||||||
@@ -831,6 +831,9 @@ fn load_oauth_tokens_from_file_with_lock_held(
|
|||||||
false
|
false
|
||||||
} else {
|
} else {
|
||||||
entry.server_url == url
|
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
|
&& (entry.server_name == local_server_name
|
||||||
|| (stored_key == &key && entry.server_name == server_name))
|
|| (stored_key == &key && entry.server_name == server_name))
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -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]
|
#[test]
|
||||||
fn legacy_rmcp_oauth_keyring_credentials_remain_readable() -> Result<()> {
|
fn legacy_rmcp_oauth_keyring_credentials_remain_readable() -> Result<()> {
|
||||||
let _env = TempCodexHome::new();
|
let _env = TempCodexHome::new();
|
||||||
|
|||||||
Reference in new issue
Block a user