mirror of
https://github.com/openai/codex.git
synced 2026-05-24 04:54:52 +00:00
## Why Windows Bazel runs in the permissions stack exposed that app-server integration tests were launching normal plugin startup warmups in every subprocess. Those warmups can call `https://chatgpt.com/backend-api/plugins/featured` when a test is not specifically exercising plugin startup, which adds slow background work, noisy stderr, and dependence on external network state. The relevant startup/featured-plugin behavior was introduced across #15042 and #15264. A few app-server tests also had long optional waits or unbounded cleanup paths, making failures expensive to diagnose and contributing to slow Windows shards. One external-agent config test from #18246 used a GitHub-style marketplace source, which was enough to exercise the pending remote-import path but also meant the background completion task could attempt a real clone. ## What Changed - Adds explicit `AppServerRuntimeOptions` / `PluginStartupTasks` plumbing and a hidden debug-only `--disable-plugin-startup-tasks-for-tests` app-server flag, so integration tests can suppress startup plugin warmups without adding a production env-var gate. - Has the app-server test harness pass that hidden flag by default, while opting plugin-startup coverage back in for tests that intentionally exercise startup sync and featured-plugin warmup behavior. - Lowers normal app-server subprocess logging from `info`/`debug` to `warn` to avoid multi-megabyte stderr output in Bazel logs. - Prevents the external-agent config test from attempting a real marketplace clone by using an invalid non-local source while still exercising the pending-import completion path. - Bounds optional filesystem/realtime waits and fake WebSocket test-server shutdown so failures produce targeted timeouts instead of hanging a shard. - Fixes the Unix script-resolution test in `rmcp-client` to exercise PATH resolution directly and include the actual spawn error in failures. ## Verification - `cargo check -p codex-app-server` - `cargo clippy -p codex-app-server --tests -- -D warnings` - `cargo test -p codex-rmcp-client program_resolver::tests::test_unix_executes_script_without_extension` - `cargo test -p codex-app-server --test all external_agent_config_import_sends_completion_notification_after_pending_plugins_finish -- --nocapture` - `cargo test -p codex-app-server --test all plugin_list_uses_warmed_featured_plugin_ids_cache_on_first_request -- --nocapture` - Windows Local Bazel passed with this test-hardening bundle before it was extracted from #19606. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19683). * #19395 * #19394 * #19393 * #19392 * #19606 * __->__ #19683
233 lines
7.8 KiB
Rust
233 lines
7.8 KiB
Rust
//! Platform-specific program resolution for MCP server execution.
|
|
//!
|
|
//! This module provides a unified interface for resolving executable paths
|
|
//! across different operating systems. The key challenge it addresses is that
|
|
//! Windows cannot execute script files (e.g., `.cmd`, `.bat`) directly through
|
|
//! `Command::new()` without their file extensions, while Unix systems handle
|
|
//! scripts natively through shebangs.
|
|
//!
|
|
//! The `resolve` function abstracts these platform differences:
|
|
//! - On Unix: Returns the program unchanged (OS handles script execution)
|
|
//! - On Windows: Uses the `which` crate to resolve full paths including extensions
|
|
|
|
use std::collections::HashMap;
|
|
use std::ffi::OsString;
|
|
use std::path::Path;
|
|
|
|
/// Resolves a program to its executable path on Unix systems.
|
|
///
|
|
/// Unix systems handle PATH resolution and script execution natively through
|
|
/// 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>,
|
|
_cwd: &Path,
|
|
) -> std::io::Result<OsString> {
|
|
Ok(program)
|
|
}
|
|
|
|
/// Resolves a program to its executable path on Windows systems.
|
|
///
|
|
/// Windows requires explicit file extensions for script execution. This function
|
|
/// uses the `which` crate to search the `PATH` environment variable and find
|
|
/// the full path to the executable, including necessary script extensions
|
|
/// (`.cmd`, `.bat`, etc.) defined in `PATHEXT`.
|
|
///
|
|
/// 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>,
|
|
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) {
|
|
Ok(resolved) => {
|
|
tracing::debug!("Resolved {program:?} to {resolved:?}");
|
|
Ok(resolved.into_os_string())
|
|
}
|
|
Err(e) => {
|
|
tracing::debug!("Failed to resolve {program:?}: {e}. Using original path");
|
|
// Fallback to original program - let Command::new() handle the error
|
|
Ok(program)
|
|
}
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::*;
|
|
use crate::utils::create_env_for_mcp_server;
|
|
use anyhow::Result;
|
|
use std::fs;
|
|
use std::path::Path;
|
|
use tempfile::TempDir;
|
|
use tokio::process::Command;
|
|
|
|
/// Unix: Verifies the OS handles script execution without file extensions.
|
|
#[cfg(unix)]
|
|
#[tokio::test]
|
|
async fn test_unix_executes_script_without_extension() -> Result<()> {
|
|
let env = TestExecutableEnv::new()?;
|
|
let mut cmd = Command::new(&env.program_name);
|
|
cmd.envs(&env.mcp_env);
|
|
|
|
let output = cmd.output().await;
|
|
assert!(
|
|
output.is_ok(),
|
|
"Unix should execute PATH-resolved scripts directly: {output:?}"
|
|
);
|
|
Ok(())
|
|
}
|
|
|
|
/// Windows: Verifies scripts fail to execute without the proper extension.
|
|
#[cfg(windows)]
|
|
#[tokio::test]
|
|
async fn test_windows_fails_without_extension() -> Result<()> {
|
|
let env = TestExecutableEnv::new()?;
|
|
let mut cmd = Command::new(&env.program_name);
|
|
cmd.envs(&env.mcp_env);
|
|
|
|
let output = cmd.output().await;
|
|
assert!(
|
|
output.is_err(),
|
|
"Windows requires .cmd/.bat extension for direct execution"
|
|
);
|
|
Ok(())
|
|
}
|
|
|
|
/// Windows: Verifies scripts with an explicit extension execute correctly.
|
|
#[cfg(windows)]
|
|
#[tokio::test]
|
|
async fn test_windows_succeeds_with_extension() -> Result<()> {
|
|
let env = TestExecutableEnv::new()?;
|
|
// Append the `.cmd` extension to the program name
|
|
let program_with_ext = format!("{}.cmd", env.program_name);
|
|
let mut cmd = Command::new(&program_with_ext);
|
|
cmd.envs(&env.mcp_env);
|
|
|
|
let output = cmd.output().await;
|
|
assert!(
|
|
output.is_ok(),
|
|
"Windows should execute scripts when the extension is provided"
|
|
);
|
|
Ok(())
|
|
}
|
|
|
|
/// Verifies program resolution enables successful execution on all platforms.
|
|
#[tokio::test]
|
|
async fn test_resolved_program_executes_successfully() -> Result<()> {
|
|
let env = TestExecutableEnv::new()?;
|
|
let program = OsString::from(&env.program_name);
|
|
|
|
// Apply platform-specific resolution
|
|
let resolved = resolve(program, &env.mcp_env, std::env::current_dir()?.as_path())?;
|
|
|
|
// Verify resolved path executes successfully
|
|
let mut cmd = Command::new(resolved);
|
|
cmd.envs(&env.mcp_env);
|
|
let output = cmd.output().await;
|
|
|
|
assert!(
|
|
output.is_ok(),
|
|
"Resolved program should execute successfully"
|
|
);
|
|
Ok(())
|
|
}
|
|
|
|
// Test fixture for creating temporary executables in a controlled environment.
|
|
struct TestExecutableEnv {
|
|
// Held to prevent the temporary directory from being deleted.
|
|
_temp_dir: TempDir,
|
|
program_name: String,
|
|
mcp_env: HashMap<OsString, OsString>,
|
|
}
|
|
|
|
impl TestExecutableEnv {
|
|
const TEST_PROGRAM: &'static str = "test_mcp_server";
|
|
|
|
fn new() -> Result<Self> {
|
|
let temp_dir = TempDir::new()?;
|
|
let dir_path = temp_dir.path();
|
|
|
|
Self::create_executable(dir_path)?;
|
|
|
|
// Build a clean environment with the temp dir in the PATH.
|
|
let mut extra_env = HashMap::new();
|
|
extra_env.insert(OsString::from("PATH"), Self::build_path_env_var(dir_path));
|
|
|
|
#[cfg(windows)]
|
|
extra_env.insert(OsString::from("PATHEXT"), Self::ensure_cmd_extension());
|
|
|
|
let mcp_env = create_env_for_mcp_server(Some(extra_env), &[])?;
|
|
|
|
Ok(Self {
|
|
_temp_dir: temp_dir,
|
|
program_name: Self::TEST_PROGRAM.to_string(),
|
|
mcp_env,
|
|
})
|
|
}
|
|
|
|
/// Creates a simple, platform-specific executable script.
|
|
fn create_executable(dir: &Path) -> Result<()> {
|
|
#[cfg(windows)]
|
|
{
|
|
let file = dir.join(format!("{}.cmd", Self::TEST_PROGRAM));
|
|
fs::write(&file, "@echo off\nexit 0")?;
|
|
}
|
|
|
|
#[cfg(unix)]
|
|
{
|
|
let file = dir.join(Self::TEST_PROGRAM);
|
|
fs::write(&file, "#!/bin/sh\nexit 0")?;
|
|
Self::set_executable(&file)?;
|
|
}
|
|
|
|
Ok(())
|
|
}
|
|
|
|
#[cfg(unix)]
|
|
fn set_executable(path: &Path) -> Result<()> {
|
|
use std::os::unix::fs::PermissionsExt;
|
|
let mut perms = fs::metadata(path)?.permissions();
|
|
perms.set_mode(0o755);
|
|
fs::set_permissions(path, perms)?;
|
|
Ok(())
|
|
}
|
|
|
|
/// Prepends the given directory to the system's PATH variable.
|
|
fn build_path_env_var(dir: &Path) -> OsString {
|
|
let mut path = OsString::from(dir.as_os_str());
|
|
if let Some(current) = std::env::var_os("PATH") {
|
|
let sep = if cfg!(windows) { ";" } else { ":" };
|
|
path.push(sep);
|
|
path.push(current);
|
|
}
|
|
path
|
|
}
|
|
|
|
/// Ensures `.CMD` is in the `PATHEXT` variable on Windows for script discovery.
|
|
#[cfg(windows)]
|
|
fn ensure_cmd_extension() -> OsString {
|
|
let current = std::env::var_os("PATHEXT").unwrap_or_default();
|
|
if current
|
|
.to_string_lossy()
|
|
.to_ascii_uppercase()
|
|
.contains(".CMD")
|
|
{
|
|
current
|
|
} else {
|
|
let mut path_ext = OsString::from(".CMD;");
|
|
path_ext.push(current);
|
|
path_ext
|
|
}
|
|
}
|
|
}
|
|
}
|