mirror of
https://github.com/openai/codex.git
synced 2026-04-29 17:06:51 +00:00
4a210faf33
## Why This is a follow-up to #15360. That change fixed the `arg0` helper setup, but `rmcp-client` still coerced stdio transport environment values into UTF-8 `String`s before program resolution and process spawn. If `PATH` or another inherited environment value contains non-UTF-8 bytes, that loses fidelity before it reaches `which` and `Command`. ## What changed - change `create_env_for_mcp_server()` to return `HashMap<OsString, OsString>` and read inherited values with `std::env::var_os()` - change `TransportRecipe::Stdio.env`, `RmcpClient::new_stdio_client()`, and `program_resolver::resolve()` to keep stdio transport env values in `OsString` form within `rmcp-client` - keep the `codex-core` config boundary stringly, but convert configured stdio env values to `OsString` once when constructing the transport - update the rmcp-client stdio test fixtures and callers to use `OsString` env maps - add a Unix regression test that verifies `create_env_for_mcp_server()` preserves a non-UTF-8 `PATH` ## How to verify - `cargo test -p codex-rmcp-client` - `cargo test -p codex-core mcp_connection_manager` - `just argument-comment-lint` Targeted coverage in this change includes `utils::tests::create_env_preserves_path_when_it_is_not_utf8`, while the updated stdio transport path is exercised by the existing rmcp-client tests that construct `RmcpClient::new_stdio_client()`.
94 lines
2.7 KiB
Rust
94 lines
2.7 KiB
Rust
#![cfg(unix)]
|
|
|
|
use std::collections::HashMap;
|
|
use std::ffi::OsString;
|
|
use std::fs;
|
|
use std::path::Path;
|
|
use std::time::Duration;
|
|
|
|
use anyhow::Context;
|
|
use anyhow::Result;
|
|
use codex_rmcp_client::RmcpClient;
|
|
|
|
fn process_exists(pid: u32) -> bool {
|
|
std::process::Command::new("kill")
|
|
.arg("-0")
|
|
.arg(pid.to_string())
|
|
.stderr(std::process::Stdio::null())
|
|
.status()
|
|
.map(|status| status.success())
|
|
.unwrap_or(false)
|
|
}
|
|
|
|
async fn wait_for_pid_file(path: &Path) -> Result<u32> {
|
|
for _ in 0..50 {
|
|
match fs::read_to_string(path) {
|
|
Ok(content) => {
|
|
let trimmed = content.trim();
|
|
if trimmed.is_empty() {
|
|
tokio::time::sleep(Duration::from_millis(100)).await;
|
|
continue;
|
|
}
|
|
|
|
let pid = trimmed
|
|
.parse::<u32>()
|
|
.with_context(|| format!("failed to parse pid from {}", path.display()))?;
|
|
return Ok(pid);
|
|
}
|
|
Err(error) if error.kind() == std::io::ErrorKind::NotFound => {
|
|
tokio::time::sleep(Duration::from_millis(100)).await;
|
|
}
|
|
Err(error) => {
|
|
return Err(error).with_context(|| format!("failed to read {}", path.display()));
|
|
}
|
|
}
|
|
}
|
|
|
|
anyhow::bail!("timed out waiting for child pid file at {}", path.display());
|
|
}
|
|
|
|
async fn wait_for_process_exit(pid: u32) -> Result<()> {
|
|
for _ in 0..50 {
|
|
if !process_exists(pid) {
|
|
return Ok(());
|
|
}
|
|
tokio::time::sleep(Duration::from_millis(100)).await;
|
|
}
|
|
|
|
anyhow::bail!("process {pid} still running after timeout");
|
|
}
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
|
|
async fn drop_kills_wrapper_process_group() -> Result<()> {
|
|
let temp_dir = tempfile::tempdir()?;
|
|
let child_pid_file = temp_dir.path().join("child.pid");
|
|
let child_pid_file_str = child_pid_file.to_string_lossy().into_owned();
|
|
|
|
let client = RmcpClient::new_stdio_client(
|
|
OsString::from("/bin/sh"),
|
|
vec![
|
|
OsString::from("-c"),
|
|
OsString::from(
|
|
"sleep 300 & child_pid=$!; echo \"$child_pid\" > \"$CHILD_PID_FILE\"; cat >/dev/null",
|
|
),
|
|
],
|
|
Some(HashMap::from([(
|
|
OsString::from("CHILD_PID_FILE"),
|
|
OsString::from(child_pid_file_str),
|
|
)])),
|
|
&[],
|
|
None,
|
|
)
|
|
.await?;
|
|
|
|
let grandchild_pid = wait_for_pid_file(&child_pid_file).await?;
|
|
assert!(
|
|
process_exists(grandchild_pid),
|
|
"expected grandchild process {grandchild_pid} to be running before dropping client"
|
|
);
|
|
|
|
drop(client);
|
|
|
|
wait_for_process_exit(grandchild_pid).await
|
|
}
|