mirror of
https://github.com/openai/codex.git
synced 2026-04-26 15:45:02 +00:00
Refactor ExecServer filesystem split between local and remote (#15232)
For each feature we have: 1. Trait exposed on environment 2. **Local Implementation** of the trait 3. Remote implementation that uses the client to proxy via network 4. Handler implementation that handles PRC requests and calls into **Local Implementation**
This commit is contained in:
pakrym-oai
@@ -11,6 +11,8 @@ use codex_app_server_protocol::RequestId;
|
||||
use codex_utils_cargo_bin::cargo_bin;
|
||||
use futures::SinkExt;
|
||||
use futures::StreamExt;
|
||||
use tokio::io::AsyncBufReadExt;
|
||||
use tokio::io::BufReader;
|
||||
use tokio::process::Child;
|
||||
use tokio::process::Command;
|
||||
use tokio::time::Instant;
|
||||
@@ -25,6 +27,7 @@ const EVENT_TIMEOUT: Duration = Duration::from_secs(5);
|
||||
|
||||
pub(crate) struct ExecServerHarness {
|
||||
child: Child,
|
||||
websocket_url: String,
|
||||
websocket: tokio_tungstenite::WebSocketStream<
|
||||
tokio_tungstenite::MaybeTlsStream<tokio::net::TcpStream>,
|
||||
>,
|
||||
@@ -39,23 +42,28 @@ impl Drop for ExecServerHarness {
|
||||
|
||||
pub(crate) async fn exec_server() -> anyhow::Result<ExecServerHarness> {
|
||||
let binary = cargo_bin("codex-exec-server")?;
|
||||
let websocket_url = reserve_websocket_url()?;
|
||||
let mut child = Command::new(binary);
|
||||
child.args(["--listen", &websocket_url]);
|
||||
child.args(["--listen", "ws://127.0.0.1:0"]);
|
||||
child.stdin(Stdio::null());
|
||||
child.stdout(Stdio::null());
|
||||
child.stdout(Stdio::piped());
|
||||
child.stderr(Stdio::inherit());
|
||||
let child = child.spawn()?;
|
||||
let mut child = child.spawn()?;
|
||||
|
||||
let websocket_url = read_listen_url_from_stdout(&mut child).await?;
|
||||
let (websocket, _) = connect_websocket_when_ready(&websocket_url).await?;
|
||||
Ok(ExecServerHarness {
|
||||
child,
|
||||
websocket_url,
|
||||
websocket,
|
||||
next_request_id: 1,
|
||||
})
|
||||
}
|
||||
|
||||
impl ExecServerHarness {
|
||||
pub(crate) fn websocket_url(&self) -> &str {
|
||||
&self.websocket_url
|
||||
}
|
||||
|
||||
pub(crate) async fn send_request(
|
||||
&mut self,
|
||||
method: &str,
|
||||
@@ -155,13 +163,6 @@ impl ExecServerHarness {
|
||||
}
|
||||
}
|
||||
|
||||
fn reserve_websocket_url() -> anyhow::Result<String> {
|
||||
let listener = std::net::TcpListener::bind("127.0.0.1:0")?;
|
||||
let addr = listener.local_addr()?;
|
||||
drop(listener);
|
||||
Ok(format!("ws://{addr}"))
|
||||
}
|
||||
|
||||
async fn connect_websocket_when_ready(
|
||||
websocket_url: &str,
|
||||
) -> anyhow::Result<(
|
||||
@@ -186,3 +187,30 @@ async fn connect_websocket_when_ready(
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async fn read_listen_url_from_stdout(child: &mut Child) -> anyhow::Result<String> {
|
||||
let stdout = child
|
||||
.stdout
|
||||
.take()
|
||||
.ok_or_else(|| anyhow!("failed to capture exec-server stdout"))?;
|
||||
let mut lines = BufReader::new(stdout).lines();
|
||||
let deadline = Instant::now() + CONNECT_TIMEOUT;
|
||||
|
||||
loop {
|
||||
let now = Instant::now();
|
||||
if now >= deadline {
|
||||
return Err(anyhow!(
|
||||
"timed out waiting for exec-server listen URL on stdout after {CONNECT_TIMEOUT:?}"
|
||||
));
|
||||
}
|
||||
let remaining = deadline.duration_since(now);
|
||||
let line = timeout(remaining, lines.next_line())
|
||||
.await
|
||||
.map_err(|_| anyhow!("timed out waiting for exec-server stdout"))??
|
||||
.ok_or_else(|| anyhow!("exec-server stdout closed before emitting listen URL"))?;
|
||||
let listen_url = line.trim();
|
||||
if listen_url.starts_with("ws://") {
|
||||
return Ok(listen_url.to_string());
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
361
codex-rs/exec-server/tests/file_system.rs
Normal file
361
codex-rs/exec-server/tests/file_system.rs
Normal file
@@ -0,0 +1,361 @@
|
||||
#![cfg(unix)]
|
||||
|
||||
mod common;
|
||||
|
||||
use std::os::unix::fs::symlink;
|
||||
use std::process::Command;
|
||||
use std::sync::Arc;
|
||||
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use codex_exec_server::CopyOptions;
|
||||
use codex_exec_server::CreateDirectoryOptions;
|
||||
use codex_exec_server::Environment;
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::ReadDirectoryEntry;
|
||||
use codex_exec_server::RemoveOptions;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::TempDir;
|
||||
use test_case::test_case;
|
||||
|
||||
use common::exec_server::ExecServerHarness;
|
||||
use common::exec_server::exec_server;
|
||||
|
||||
struct FileSystemContext {
|
||||
file_system: Arc<dyn ExecutorFileSystem>,
|
||||
_server: Option<ExecServerHarness>,
|
||||
}
|
||||
|
||||
async fn create_file_system_context(use_remote: bool) -> Result<FileSystemContext> {
|
||||
if use_remote {
|
||||
let server = exec_server().await?;
|
||||
let environment = Environment::create(Some(server.websocket_url().to_string())).await?;
|
||||
Ok(FileSystemContext {
|
||||
file_system: environment.get_filesystem(),
|
||||
_server: Some(server),
|
||||
})
|
||||
} else {
|
||||
let environment = Environment::create(None).await?;
|
||||
Ok(FileSystemContext {
|
||||
file_system: environment.get_filesystem(),
|
||||
_server: None,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
fn absolute_path(path: std::path::PathBuf) -> AbsolutePathBuf {
|
||||
assert!(
|
||||
path.is_absolute(),
|
||||
"path must be absolute: {}",
|
||||
path.display()
|
||||
);
|
||||
match AbsolutePathBuf::try_from(path) {
|
||||
Ok(path) => path,
|
||||
Err(err) => panic!("path should be absolute: {err}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn file_system_get_metadata_returns_expected_fields(use_remote: bool) -> Result<()> {
|
||||
let context = create_file_system_context(use_remote).await?;
|
||||
let file_system = context.file_system;
|
||||
|
||||
let tmp = TempDir::new()?;
|
||||
let file_path = tmp.path().join("note.txt");
|
||||
std::fs::write(&file_path, "hello")?;
|
||||
|
||||
let metadata = file_system
|
||||
.get_metadata(&absolute_path(file_path))
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert_eq!(metadata.is_directory, false);
|
||||
assert_eq!(metadata.is_file, true);
|
||||
assert!(metadata.modified_at_ms > 0);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn file_system_methods_cover_surface_area(use_remote: bool) -> Result<()> {
|
||||
let context = create_file_system_context(use_remote).await?;
|
||||
let file_system = context.file_system;
|
||||
|
||||
let tmp = TempDir::new()?;
|
||||
let source_dir = tmp.path().join("source");
|
||||
let nested_dir = source_dir.join("nested");
|
||||
let source_file = source_dir.join("root.txt");
|
||||
let nested_file = nested_dir.join("note.txt");
|
||||
let copied_dir = tmp.path().join("copied");
|
||||
let copied_file = tmp.path().join("copy.txt");
|
||||
|
||||
file_system
|
||||
.create_directory(
|
||||
&absolute_path(nested_dir.clone()),
|
||||
CreateDirectoryOptions { recursive: true },
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
|
||||
file_system
|
||||
.write_file(
|
||||
&absolute_path(nested_file.clone()),
|
||||
b"hello from trait".to_vec(),
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
file_system
|
||||
.write_file(
|
||||
&absolute_path(source_file.clone()),
|
||||
b"hello from source root".to_vec(),
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
|
||||
let nested_file_contents = file_system
|
||||
.read_file(&absolute_path(nested_file.clone()))
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert_eq!(nested_file_contents, b"hello from trait");
|
||||
|
||||
file_system
|
||||
.copy(
|
||||
&absolute_path(nested_file),
|
||||
&absolute_path(copied_file.clone()),
|
||||
CopyOptions { recursive: false },
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert_eq!(std::fs::read_to_string(copied_file)?, "hello from trait");
|
||||
|
||||
file_system
|
||||
.copy(
|
||||
&absolute_path(source_dir.clone()),
|
||||
&absolute_path(copied_dir.clone()),
|
||||
CopyOptions { recursive: true },
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(copied_dir.join("nested").join("note.txt"))?,
|
||||
"hello from trait"
|
||||
);
|
||||
|
||||
let mut entries = file_system
|
||||
.read_directory(&absolute_path(source_dir))
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
entries.sort_by(|left, right| left.file_name.cmp(&right.file_name));
|
||||
assert_eq!(
|
||||
entries,
|
||||
vec![
|
||||
ReadDirectoryEntry {
|
||||
file_name: "nested".to_string(),
|
||||
is_directory: true,
|
||||
is_file: false,
|
||||
},
|
||||
ReadDirectoryEntry {
|
||||
file_name: "root.txt".to_string(),
|
||||
is_directory: false,
|
||||
is_file: true,
|
||||
},
|
||||
]
|
||||
);
|
||||
|
||||
file_system
|
||||
.remove(
|
||||
&absolute_path(copied_dir.clone()),
|
||||
RemoveOptions {
|
||||
recursive: true,
|
||||
force: true,
|
||||
},
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert!(!copied_dir.exists());
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn file_system_copy_rejects_directory_without_recursive(use_remote: bool) -> Result<()> {
|
||||
let context = create_file_system_context(use_remote).await?;
|
||||
let file_system = context.file_system;
|
||||
|
||||
let tmp = TempDir::new()?;
|
||||
let source_dir = tmp.path().join("source");
|
||||
std::fs::create_dir_all(&source_dir)?;
|
||||
|
||||
let error = file_system
|
||||
.copy(
|
||||
&absolute_path(source_dir),
|
||||
&absolute_path(tmp.path().join("dest")),
|
||||
CopyOptions { recursive: false },
|
||||
)
|
||||
.await;
|
||||
let error = match error {
|
||||
Ok(()) => panic!("copy should fail"),
|
||||
Err(error) => error,
|
||||
};
|
||||
assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput);
|
||||
assert_eq!(
|
||||
error.to_string(),
|
||||
"fs/copy requires recursive: true when sourcePath is a directory"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn file_system_copy_rejects_copying_directory_into_descendant(
|
||||
use_remote: bool,
|
||||
) -> Result<()> {
|
||||
let context = create_file_system_context(use_remote).await?;
|
||||
let file_system = context.file_system;
|
||||
|
||||
let tmp = TempDir::new()?;
|
||||
let source_dir = tmp.path().join("source");
|
||||
std::fs::create_dir_all(source_dir.join("nested"))?;
|
||||
|
||||
let error = file_system
|
||||
.copy(
|
||||
&absolute_path(source_dir.clone()),
|
||||
&absolute_path(source_dir.join("nested").join("copy")),
|
||||
CopyOptions { recursive: true },
|
||||
)
|
||||
.await;
|
||||
let error = match error {
|
||||
Ok(()) => panic!("copy should fail"),
|
||||
Err(error) => error,
|
||||
};
|
||||
assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput);
|
||||
assert_eq!(
|
||||
error.to_string(),
|
||||
"fs/copy cannot copy a directory to itself or one of its descendants"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn file_system_copy_preserves_symlinks_in_recursive_copy(use_remote: bool) -> Result<()> {
|
||||
let context = create_file_system_context(use_remote).await?;
|
||||
let file_system = context.file_system;
|
||||
|
||||
let tmp = TempDir::new()?;
|
||||
let source_dir = tmp.path().join("source");
|
||||
let nested_dir = source_dir.join("nested");
|
||||
let copied_dir = tmp.path().join("copied");
|
||||
std::fs::create_dir_all(&nested_dir)?;
|
||||
symlink("nested", source_dir.join("nested-link"))?;
|
||||
|
||||
file_system
|
||||
.copy(
|
||||
&absolute_path(source_dir),
|
||||
&absolute_path(copied_dir.clone()),
|
||||
CopyOptions { recursive: true },
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
|
||||
let copied_link = copied_dir.join("nested-link");
|
||||
let metadata = std::fs::symlink_metadata(&copied_link)?;
|
||||
assert!(metadata.file_type().is_symlink());
|
||||
assert_eq!(
|
||||
std::fs::read_link(copied_link)?,
|
||||
std::path::PathBuf::from("nested")
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn file_system_copy_ignores_unknown_special_files_in_recursive_copy(
|
||||
use_remote: bool,
|
||||
) -> Result<()> {
|
||||
let context = create_file_system_context(use_remote).await?;
|
||||
let file_system = context.file_system;
|
||||
|
||||
let tmp = TempDir::new()?;
|
||||
let source_dir = tmp.path().join("source");
|
||||
let copied_dir = tmp.path().join("copied");
|
||||
std::fs::create_dir_all(&source_dir)?;
|
||||
std::fs::write(source_dir.join("note.txt"), "hello")?;
|
||||
|
||||
let fifo_path = source_dir.join("named-pipe");
|
||||
let output = Command::new("mkfifo").arg(&fifo_path).output()?;
|
||||
if !output.status.success() {
|
||||
anyhow::bail!(
|
||||
"mkfifo failed: stdout={} stderr={}",
|
||||
String::from_utf8_lossy(&output.stdout).trim(),
|
||||
String::from_utf8_lossy(&output.stderr).trim()
|
||||
);
|
||||
}
|
||||
|
||||
file_system
|
||||
.copy(
|
||||
&absolute_path(source_dir),
|
||||
&absolute_path(copied_dir.clone()),
|
||||
CopyOptions { recursive: true },
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(copied_dir.join("note.txt"))?,
|
||||
"hello"
|
||||
);
|
||||
assert!(!copied_dir.join("named-pipe").exists());
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn file_system_copy_rejects_standalone_fifo_source(use_remote: bool) -> Result<()> {
|
||||
let context = create_file_system_context(use_remote).await?;
|
||||
let file_system = context.file_system;
|
||||
|
||||
let tmp = TempDir::new()?;
|
||||
let fifo_path = tmp.path().join("named-pipe");
|
||||
let output = Command::new("mkfifo").arg(&fifo_path).output()?;
|
||||
if !output.status.success() {
|
||||
anyhow::bail!(
|
||||
"mkfifo failed: stdout={} stderr={}",
|
||||
String::from_utf8_lossy(&output.stdout).trim(),
|
||||
String::from_utf8_lossy(&output.stderr).trim()
|
||||
);
|
||||
}
|
||||
|
||||
let error = file_system
|
||||
.copy(
|
||||
&absolute_path(fifo_path),
|
||||
&absolute_path(tmp.path().join("copied")),
|
||||
CopyOptions { recursive: false },
|
||||
)
|
||||
.await;
|
||||
let error = match error {
|
||||
Ok(()) => panic!("copy should fail"),
|
||||
Err(error) => error,
|
||||
};
|
||||
assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput);
|
||||
assert_eq!(
|
||||
error.to_string(),
|
||||
"fs/copy only supports regular files, directories, and symlinks"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
Reference in New Issue
Block a user