Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

114 changes: 98 additions & 16 deletions codex-rs/cli/src/doctor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1520,19 +1520,35 @@ async fn mcp_check_from_servers(servers: &HashMap<String, McpServerConfig>) -> D
if disabled_server {
continue;
}
if let Some(cwd) = cwd
&& !cwd.exists()
{
missing_env.push(format!("{name}: cwd does not exist ({})", cwd.display()));
}
if command.trim().is_empty() {
let command_is_empty = command.trim().is_empty();
if command_is_empty {
missing_env.push(format!("{name}: stdio command is empty"));
} else if let Err(err) =
stdio_command_resolves(command, cwd.as_deref(), env.as_ref())
{
missing_env.push(format!(
"{name}: stdio command {command:?} is not resolvable ({err})"
));
}
if server.is_local_environment() {
Comment thread
anp-oai marked this conversation as resolved.
let host_native_cwd = cwd.as_ref().map(|cwd| Path::new(cwd.as_str()));
if let Some(cwd) = host_native_cwd
&& !cwd.exists()
{
missing_env.push(format!("{name}: cwd does not exist ({})", cwd.display()));
}
if !command_is_empty
&& let Err(err) =
stdio_command_resolves(command, host_native_cwd, env.as_ref())
{
missing_env.push(format!(
"{name}: stdio command {command:?} is not resolvable ({err})"
));
}
} else {
match cwd {
Some(cwd) if cwd.to_inferred_path_uri().is_none() => {
missing_env
.push(format!("{name}: remote stdio cwd is not absolute ({cwd})"));
}
None => missing_env
.push(format!("{name}: remote stdio requires an explicit cwd")),
Some(_) => {}
Comment thread
anp-oai marked this conversation as resolved.
}
}
if let Some(env) = env {
for key in env.keys().filter(|key| key.trim().is_empty()) {
Expand All @@ -1541,10 +1557,12 @@ async fn mcp_check_from_servers(servers: &HashMap<String, McpServerConfig>) -> D
}
for env_var in env_vars {
if env_var.is_remote_source() {
missing_env.push(format!(
"{name}: env_vars entry `{}` uses source `remote`, which requires remote MCP stdio",
env_var.name()
));
if server.is_local_environment() {
missing_env.push(format!(
"{name}: env_vars entry `{}` uses source `remote`, which requires remote MCP stdio",
env_var.name()
));
}
} else if !env_var_present(env_var.name()) {
missing_env.push(format!("{name}: env var {} is not set", env_var.name()));
}
Expand Down Expand Up @@ -3863,6 +3881,70 @@ mod tests {
}));
}

#[tokio::test]
async fn mcp_check_skips_host_path_checks_for_remote_stdio() {
#[cfg(not(windows))]
let cwd = r"C:\Users\openai\share";
#[cfg(windows)]
let cwd = "/home/openai/share";
let cwd = toml::Value::String(cwd.to_string());
let remote_server: McpServerConfig = toml::from_str(&format!(
r#"
command = "definitely-missing-codex-doctor-mcp"
environment_id = "remote"
cwd = {cwd}
required = true
env_vars = [{{ name = "REMOTE_ONLY_TOKEN", source = "remote" }}]
"#,
))
.expect("should deserialize remote MCP config");
let servers = HashMap::from([("remote".to_string(), remote_server)]);

let check = mcp_check_from_servers(&servers).await;

assert_eq!(check.status, CheckStatus::Ok);
assert_eq!(check.summary, "MCP configuration is locally consistent");
}

#[tokio::test]
async fn mcp_check_validates_remote_stdio_cwd() {
let missing_cwd: McpServerConfig = toml::from_str(
r#"
command = "echo"
environment_id = "remote"
required = true
"#,
)
.expect("should deserialize remote MCP config without cwd");
let relative_cwd: McpServerConfig = toml::from_str(
r#"
command = "echo"
environment_id = "remote"
cwd = "relative"
required = true
"#,
)
.expect("should deserialize remote MCP config with relative cwd");
let servers = HashMap::from([
("missing".to_string(), missing_cwd),
("relative".to_string(), relative_cwd),
]);

let check = mcp_check_from_servers(&servers).await;

assert_eq!(check.status, CheckStatus::Fail);
assert!(
check
.details
.contains(&"missing: remote stdio requires an explicit cwd".to_string())
);
assert!(
check
.details
.contains(&"relative: remote stdio cwd is not absolute (relative)".to_string())
);
}

#[cfg(unix)]
#[test]
fn read_probe_file_rejects_unreadable_file() {
Expand Down
4 changes: 2 additions & 2 deletions codex-rs/cli/src/mcp_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) ->
let env_display = format_env_display(env.as_ref(), env_vars);
let cwd_display = cwd
.as_ref()
.map(|path| path.display().to_string())
.map(ToString::to_string)
.filter(|value| !value.is_empty())
.unwrap_or_else(|| "-".to_string());
let status = format_mcp_status(cfg);
Expand Down Expand Up @@ -897,7 +897,7 @@ async fn run_get(config_overrides: &CliConfigOverrides, get_args: GetArgs) -> Re
println!(" args: {args_display}");
let cwd_display = cwd
.as_ref()
.map(|path| path.display().to_string())
.map(ToString::to_string)
.filter(|value| !value.is_empty())
.unwrap_or_else(|| "-".to_string());
println!(" cwd: {cwd_display}");
Expand Down
3 changes: 2 additions & 1 deletion codex-rs/codex-mcp/src/plugin_config_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use codex_config::McpServerConfig;
use codex_config::McpServerEnvVar;
use codex_config::McpServerOAuthConfig;
use codex_config::McpServerTransportConfig;
use codex_utils_path_uri::LegacyAppPathString;
use pretty_assertions::assert_eq;
use std::collections::BTreeMap;
use std::collections::HashMap;
Expand All @@ -31,7 +32,7 @@ fn stdio_server(
args: Vec::new(),
env: None,
env_vars,
cwd: Some(cwd.to_path_buf()),
cwd: Some(LegacyAppPathString::from_path(cwd)),
},
environment_id: environment_id.to_string(),
enabled: true,
Expand Down
1 change: 1 addition & 0 deletions codex-rs/codex-mcp/src/rmcp_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,7 @@ async fn make_rmcp_client(
)) as Arc<dyn StdioServerLauncher>
};

let cwd = cwd.map(codex_utils_path_uri::LegacyAppPathString::into_string);
RmcpClient::new_stdio_client(command_os, args_os, env_os, &env_vars, cwd, launcher)
.await
.map_err(|err| StartupOutcomeError::from(anyhow!(err)))
Expand Down
55 changes: 2 additions & 53 deletions codex-rs/codex-mcp/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,6 @@ impl McpRuntimeContext {
.environment_manager
.get_environment(&config.environment_id)
{
if !config.is_local_environment() {
ensure_remote_stdio_cwd(server_name, config)?;
}
return Ok(Some(environment));
}

Expand All @@ -88,27 +85,6 @@ impl McpRuntimeContext {
}
}

fn ensure_remote_stdio_cwd(
Comment thread
anp-oai marked this conversation as resolved.
server_name: &str,
config: &codex_config::McpServerConfig,
) -> Result<(), String> {
let codex_config::McpServerTransportConfig::Stdio { cwd, .. } = &config.transport else {
return Ok(());
};
let Some(cwd) = cwd else {
return Err(format!(
"remote stdio MCP server `{server_name}` requires an absolute cwd"
));
};
if cwd.is_absolute() {
return Ok(());
}
Err(format!(
"remote stdio MCP server `{server_name}` requires an absolute cwd, got `{}`",
cwd.display()
))
}

pub(crate) fn emit_duration(metric: &str, duration: Duration, tags: &[(&str, &str)]) {
if let Some(metrics) = codex_otel::global() {
let _ = metrics.record_duration(metric, duration, tags);
Expand All @@ -123,6 +99,7 @@ mod tests {
use codex_config::McpServerConfig;
use codex_config::McpServerTransportConfig;
use codex_exec_server::EnvironmentManager;
use codex_utils_path_uri::LegacyAppPathString;
use pretty_assertions::assert_eq;

use super::*;
Expand Down Expand Up @@ -236,7 +213,7 @@ mod tests {
let McpServerTransportConfig::Stdio { cwd, .. } = &mut remote_stdio.transport else {
unreachable!("stdio helper should build stdio transport");
};
*cwd = Some(std::env::temp_dir());
*cwd = Some(LegacyAppPathString::from_path(&std::env::temp_dir()));
for resolved_runtime in [
runtime_context.resolve_server_environment("stdio", &remote_stdio),
runtime_context.resolve_server_environment("http", &http_server("remote")),
Expand Down Expand Up @@ -264,32 +241,4 @@ mod tests {
};
assert!(resolved_runtime.is_some());
}

#[tokio::test]
async fn remote_stdio_requires_absolute_cwd() {
let runtime_context = McpRuntimeContext::new(
Arc::new(
EnvironmentManager::create_for_tests(
Some("ws://127.0.0.1:8765".to_string()),
/*local_runtime_paths*/ None,
)
.await,
),
PathBuf::from("/tmp"),
);
let mut remote_stdio = stdio_server("remote");
let McpServerTransportConfig::Stdio { cwd, .. } = &mut remote_stdio.transport else {
unreachable!("stdio helper should build stdio transport");
};
*cwd = Some(PathBuf::from("relative"));

let error = match runtime_context.resolve_server_environment("stdio", &remote_stdio) {
Ok(_) => panic!("remote stdio MCP should require absolute cwd"),
Err(error) => error,
};
assert_eq!(
error,
"remote stdio MCP server `stdio` requires an absolute cwd, got `relative`"
);
}
}
2 changes: 1 addition & 1 deletion codex-rs/config/src/mcp_edit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ fn serialize_mcp_server(config: &McpServerConfig) -> TomlItem {
entry["env_vars"] = array_from_env_vars(env_vars);
}
if let Some(cwd) = cwd {
entry["cwd"] = value(cwd.to_string_lossy().to_string());
entry["cwd"] = value(cwd.as_str());
}
}
McpServerTransportConfig::StreamableHttp {
Expand Down
31 changes: 3 additions & 28 deletions codex-rs/config/src/mcp_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

use std::collections::HashMap;
use std::fmt;
use std::path::PathBuf;
use std::time::Duration;

use codex_utils_path_uri::LegacyAppPathString;
use schemars::JsonSchema;
use serde::Deserialize;
use serde::Deserializer;
Expand Down Expand Up @@ -224,7 +224,7 @@ pub struct RawMcpServerConfig {
#[serde(default)]
pub env_vars: Option<Vec<McpServerEnvVar>>,
#[serde(default)]
pub cwd: Option<PathBuf>,
pub cwd: Option<LegacyAppPathString>,
pub http_headers: Option<HashMap<String, String>>,
#[serde(default)]
pub env_http_headers: Option<HashMap<String, String>>,
Expand Down Expand Up @@ -358,7 +358,6 @@ impl TryFrom<RawMcpServerConfig> for McpServerConfig {

let environment_id =
environment_id.unwrap_or_else(|| DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string());
validate_remote_stdio_cwd(&transport, &environment_id)?;

Ok(Self {
transport,
Expand Down Expand Up @@ -395,30 +394,6 @@ const fn default_enabled() -> bool {
true
}

fn validate_remote_stdio_cwd(
transport: &McpServerTransportConfig,
environment_id: &str,
) -> Result<(), String> {
if environment_id == DEFAULT_MCP_SERVER_ENVIRONMENT_ID {
return Ok(());
}
let McpServerTransportConfig::Stdio { cwd, .. } = transport else {
return Ok(());
};
let Some(cwd) = cwd else {
return Err(format!(
"remote stdio MCP servers require an absolute cwd when environment_id is `{environment_id}`"
));
};
if cwd.is_absolute() {
return Ok(());
}
Err(format!(
"remote stdio MCP servers require an absolute cwd when environment_id is `{environment_id}`, got `{}`",
cwd.display()
))
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema)]
#[serde(untagged, deny_unknown_fields, rename_all = "snake_case")]
pub enum McpServerTransportConfig {
Expand All @@ -432,7 +407,7 @@ pub enum McpServerTransportConfig {
#[serde(default, skip_serializing_if = "Vec::is_empty")]
env_vars: Vec<McpServerEnvVar>,
#[serde(default, skip_serializing_if = "Option::is_none")]
cwd: Option<PathBuf>,
cwd: Option<LegacyAppPathString>,
},
/// https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#streamable-http
StreamableHttp {
Expand Down
Loading
Loading