mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Fix relative stdio MCP cwd fallback (#19031)
This commit is contained in:
@@ -5802,8 +5802,8 @@ impl CodexMessageProcessor {
|
||||
let environment_manager = self.thread_manager.environment_manager();
|
||||
let runtime_environment = match environment_manager.default_environment() {
|
||||
Some(environment) => {
|
||||
// Status listing has no turn cwd. This fallback is used only
|
||||
// by executor-backed stdio MCPs whose config omits `cwd`.
|
||||
// Status listing has no turn cwd. This fallback is used by
|
||||
// stdio MCPs whose config omits `cwd`.
|
||||
McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf())
|
||||
}
|
||||
None => McpRuntimeEnvironment::new(
|
||||
|
||||
@@ -666,8 +666,8 @@ pub struct McpConnectionManager {
|
||||
/// servers should run for the current caller. Keep it explicit at manager
|
||||
/// construction time so status/snapshot paths and real sessions make the same
|
||||
/// local-vs-remote decision. `fallback_cwd` is not a per-server override; it is
|
||||
/// used only when an executor-backed stdio server omits `cwd` and the executor
|
||||
/// API still needs a concrete process working directory.
|
||||
/// used when a stdio server omits `cwd` and the launcher needs a concrete
|
||||
/// process working directory.
|
||||
#[derive(Clone)]
|
||||
pub struct McpRuntimeEnvironment {
|
||||
environment: Arc<Environment>,
|
||||
@@ -1573,7 +1573,9 @@ async fn make_rmcp_client(
|
||||
runtime_environment.fallback_cwd(),
|
||||
))
|
||||
} else {
|
||||
Arc::new(LocalStdioServerLauncher) as Arc<dyn StdioServerLauncher>
|
||||
Arc::new(LocalStdioServerLauncher::new(
|
||||
runtime_environment.fallback_cwd(),
|
||||
)) as Arc<dyn StdioServerLauncher>
|
||||
};
|
||||
|
||||
// `RmcpClient` always sees a launched MCP stdio server. The
|
||||
|
||||
@@ -618,6 +618,69 @@ async fn remote_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() -
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
|
||||
#[serial(mcp_cwd)]
|
||||
async fn local_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() -> anyhow::Result<()>
|
||||
{
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = responses::start_mock_server().await;
|
||||
let server_name = "rmcp_local_fallback_cwd";
|
||||
let expected_cwd = Arc::new(Mutex::new(None::<PathBuf>));
|
||||
let expected_cwd_for_config = Arc::clone(&expected_cwd);
|
||||
let rmcp_test_server_bin = cargo_bin("test_stdio_server")?;
|
||||
let relative_server_path = PathBuf::from("mcp-bin").join(
|
||||
rmcp_test_server_bin
|
||||
.file_name()
|
||||
.expect("test stdio server binary should have a file name"),
|
||||
);
|
||||
let relative_command = relative_server_path.to_string_lossy().into_owned();
|
||||
|
||||
let fixture = test_codex()
|
||||
.with_config(move |config| {
|
||||
*expected_cwd_for_config
|
||||
.lock()
|
||||
.expect("expected cwd lock should not be poisoned") =
|
||||
Some(config.cwd.to_path_buf());
|
||||
|
||||
let target_bin = config.cwd.join(&relative_server_path).into_path_buf();
|
||||
let target_dir = target_bin
|
||||
.parent()
|
||||
.expect("relative test server path should include a parent");
|
||||
fs::create_dir_all(target_dir).expect("create relative MCP bin directory");
|
||||
fs::copy(&rmcp_test_server_bin, &target_bin).expect("copy test stdio server");
|
||||
|
||||
insert_mcp_server(
|
||||
config,
|
||||
server_name,
|
||||
stdio_transport(
|
||||
relative_command,
|
||||
Some(HashMap::from([(
|
||||
"MCP_TEST_VALUE".to_string(),
|
||||
"local-fallback-cwd".to_string(),
|
||||
)])),
|
||||
Vec::new(),
|
||||
),
|
||||
TestMcpServerOptions::default(),
|
||||
);
|
||||
})
|
||||
.build(&server)
|
||||
.await?;
|
||||
|
||||
let expected_cwd = expected_cwd
|
||||
.lock()
|
||||
.expect("expected cwd lock should not be poisoned")
|
||||
.clone()
|
||||
.expect("test config should record runtime fallback cwd");
|
||||
let structured =
|
||||
call_cwd_tool(&server, &fixture, server_name, "call-local-fallback-cwd").await?;
|
||||
|
||||
assert_cwd_tool_output(&structured, &expected_cwd);
|
||||
server.verify().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
|
||||
async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
@@ -12,6 +12,7 @@
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::ffi::OsString;
|
||||
use std::path::Path;
|
||||
|
||||
/// Resolves a program to its executable path on Unix systems.
|
||||
///
|
||||
@@ -19,7 +20,11 @@ use std::ffi::OsString;
|
||||
/// the kernel's shebang (`#!`) mechanism, so this function simply returns
|
||||
/// the program name unchanged.
|
||||
#[cfg(unix)]
|
||||
pub fn resolve(program: OsString, _env: &HashMap<OsString, OsString>) -> std::io::Result<OsString> {
|
||||
pub fn resolve(
|
||||
program: OsString,
|
||||
_env: &HashMap<OsString, OsString>,
|
||||
_cwd: &Path,
|
||||
) -> std::io::Result<OsString> {
|
||||
Ok(program)
|
||||
}
|
||||
|
||||
@@ -33,16 +38,16 @@ pub fn resolve(program: OsString, _env: &HashMap<OsString, OsString>) -> std::io
|
||||
/// This enables tools like `npx`, `pnpm`, and `yarn` to work correctly on Windows
|
||||
/// without requiring users to specify full paths or extensions in their configuration.
|
||||
#[cfg(windows)]
|
||||
pub fn resolve(program: OsString, env: &HashMap<OsString, OsString>) -> std::io::Result<OsString> {
|
||||
// Get current directory for relative path resolution
|
||||
let cwd = std::env::current_dir()
|
||||
.map_err(|e| std::io::Error::other(format!("Failed to get current directory: {e}")))?;
|
||||
|
||||
pub fn resolve(
|
||||
program: OsString,
|
||||
env: &HashMap<OsString, OsString>,
|
||||
cwd: &Path,
|
||||
) -> std::io::Result<OsString> {
|
||||
// Extract PATH from environment for search locations
|
||||
let search_path = env.get(std::ffi::OsStr::new("PATH"));
|
||||
|
||||
// Attempt resolution via which crate
|
||||
match which::which_in(&program, search_path, &cwd) {
|
||||
match which::which_in(&program, search_path, cwd) {
|
||||
Ok(resolved) => {
|
||||
tracing::debug!("Resolved {program:?} to {resolved:?}");
|
||||
Ok(resolved.into_os_string())
|
||||
@@ -119,7 +124,7 @@ mod tests {
|
||||
let program = OsString::from(&env.program_name);
|
||||
|
||||
// Apply platform-specific resolution
|
||||
let resolved = resolve(program, &env.mcp_env)?;
|
||||
let resolved = resolve(program, &env.mcp_env, std::env::current_dir()?.as_path())?;
|
||||
|
||||
// Verify resolved path executes successfully
|
||||
let mut cmd = Command::new(resolved);
|
||||
|
||||
@@ -162,14 +162,27 @@ impl StdioServerCommand {
|
||||
/// process spawns the configured command and rmcp talks to the child's local
|
||||
/// stdin/stdout pipes directly.
|
||||
#[derive(Clone)]
|
||||
pub struct LocalStdioServerLauncher;
|
||||
pub struct LocalStdioServerLauncher {
|
||||
fallback_cwd: PathBuf,
|
||||
}
|
||||
|
||||
impl LocalStdioServerLauncher {
|
||||
/// Creates a local stdio launcher.
|
||||
///
|
||||
/// `fallback_cwd` is used when the MCP server config omits `cwd`, so
|
||||
/// relative commands resolve from the caller's runtime working directory.
|
||||
pub fn new(fallback_cwd: PathBuf) -> Self {
|
||||
Self { fallback_cwd }
|
||||
}
|
||||
}
|
||||
|
||||
impl StdioServerLauncher for LocalStdioServerLauncher {
|
||||
fn launch(
|
||||
&self,
|
||||
command: StdioServerCommand,
|
||||
) -> BoxFuture<'static, io::Result<StdioServerTransport>> {
|
||||
async move { Self::launch_server(command) }.boxed()
|
||||
let fallback_cwd = self.fallback_cwd.clone();
|
||||
async move { Self::launch_server(command, fallback_cwd) }.boxed()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -193,7 +206,10 @@ mod private {
|
||||
impl private::Sealed for LocalStdioServerLauncher {}
|
||||
|
||||
impl LocalStdioServerLauncher {
|
||||
fn launch_server(command: StdioServerCommand) -> io::Result<StdioServerTransport> {
|
||||
fn launch_server(
|
||||
command: StdioServerCommand,
|
||||
fallback_cwd: PathBuf,
|
||||
) -> io::Result<StdioServerTransport> {
|
||||
let StdioServerCommand {
|
||||
program,
|
||||
args,
|
||||
@@ -203,22 +219,21 @@ impl LocalStdioServerLauncher {
|
||||
} = command;
|
||||
let program_name = program.to_string_lossy().into_owned();
|
||||
let envs = create_env_for_mcp_server(env, &env_vars).map_err(io::Error::other)?;
|
||||
let cwd = cwd.unwrap_or(fallback_cwd);
|
||||
let resolved_program =
|
||||
program_resolver::resolve(program, &envs).map_err(io::Error::other)?;
|
||||
program_resolver::resolve(program, &envs, &cwd).map_err(io::Error::other)?;
|
||||
|
||||
let mut command = Command::new(resolved_program);
|
||||
command
|
||||
.kill_on_drop(true)
|
||||
.stdin(Stdio::piped())
|
||||
.stdout(Stdio::piped())
|
||||
.current_dir(cwd)
|
||||
.env_clear()
|
||||
.envs(envs)
|
||||
.args(args);
|
||||
#[cfg(unix)]
|
||||
command.process_group(0);
|
||||
if let Some(cwd) = cwd {
|
||||
command.current_dir(cwd);
|
||||
}
|
||||
|
||||
let (transport, stderr) = TokioChildProcess::builder(command)
|
||||
.stderr(Stdio::piped())
|
||||
|
||||
@@ -80,7 +80,7 @@ async fn drop_kills_wrapper_process_group() -> Result<()> {
|
||||
)])),
|
||||
&[],
|
||||
/*cwd*/ None,
|
||||
Arc::new(LocalStdioServerLauncher),
|
||||
Arc::new(LocalStdioServerLauncher::new(std::env::current_dir()?)),
|
||||
)
|
||||
.await?;
|
||||
|
||||
|
||||
@@ -63,7 +63,7 @@ async fn rmcp_client_can_list_and_read_resources() -> anyhow::Result<()> {
|
||||
/*env*/ None,
|
||||
&[],
|
||||
/*cwd*/ None,
|
||||
Arc::new(LocalStdioServerLauncher),
|
||||
Arc::new(LocalStdioServerLauncher::new(std::env::current_dir()?)),
|
||||
)
|
||||
.await?;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user