mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
exec-server: canonicalize bound filesystem paths
This commit is contained in:
@@ -41,6 +41,7 @@ use crate::protocol::ExecExitedNotification;
|
||||
use crate::protocol::ExecOutputDeltaNotification;
|
||||
use crate::protocol::ExecParams;
|
||||
use crate::protocol::ExecResponse;
|
||||
use crate::protocol::FS_CANONICALIZE_METHOD;
|
||||
use crate::protocol::FS_COPY_METHOD;
|
||||
use crate::protocol::FS_CREATE_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_GET_METADATA_METHOD;
|
||||
@@ -48,6 +49,8 @@ use crate::protocol::FS_READ_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_READ_FILE_METHOD;
|
||||
use crate::protocol::FS_REMOVE_METHOD;
|
||||
use crate::protocol::FS_WRITE_FILE_METHOD;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCanonicalizeResponse;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCopyResponse;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
@@ -414,6 +417,13 @@ impl ExecServerClient {
|
||||
self.call(FS_GET_METADATA_METHOD, ¶ms).await
|
||||
}
|
||||
|
||||
pub async fn fs_canonicalize(
|
||||
&self,
|
||||
params: FsCanonicalizeParams,
|
||||
) -> Result<FsCanonicalizeResponse, ExecServerError> {
|
||||
self.call(FS_CANONICALIZE_METHOD, ¶ms).await
|
||||
}
|
||||
|
||||
pub async fn fs_read_directory(
|
||||
&self,
|
||||
params: FsReadDirectoryParams,
|
||||
|
||||
@@ -2,6 +2,8 @@ use std::fmt;
|
||||
use std::hash::Hash;
|
||||
use std::hash::Hasher;
|
||||
use std::io;
|
||||
use std::path::Component;
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -60,6 +62,13 @@ impl EnvironmentPathRef {
|
||||
pub fn with_path(&self, path: AbsolutePathBuf) -> Self {
|
||||
Self::new(Arc::clone(&self.file_system), path)
|
||||
}
|
||||
|
||||
/// Best-effort resolves this path through its bound filesystem.
|
||||
pub fn canonicalize_if_exists(&self) -> Self {
|
||||
self.file_system
|
||||
.canonicalize(&self.path)
|
||||
.map_or_else(|_| self.clone(), |path| self.with_path(path))
|
||||
}
|
||||
}
|
||||
|
||||
impl PartialEq for EnvironmentPathRef {
|
||||
@@ -100,6 +109,7 @@ mod tests {
|
||||
|
||||
#[derive(Clone, Debug, Eq, PartialEq)]
|
||||
enum RecordedMethod {
|
||||
Canonicalize,
|
||||
ReadFileText,
|
||||
Metadata,
|
||||
ReadDirectory,
|
||||
@@ -114,17 +124,26 @@ mod tests {
|
||||
|
||||
struct RecordingFileSystem {
|
||||
calls: Mutex<Vec<RecordedCall>>,
|
||||
canonicalize_supported: bool,
|
||||
}
|
||||
|
||||
impl Default for RecordingFileSystem {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
calls: Mutex::new(Vec::new()),
|
||||
canonicalize_supported: true,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl RecordingFileSystem {
|
||||
fn without_canonicalize() -> Self {
|
||||
Self {
|
||||
calls: Mutex::new(Vec::new()),
|
||||
canonicalize_supported: false,
|
||||
}
|
||||
}
|
||||
|
||||
fn recorded_calls(&self) -> Vec<RecordedCall> {
|
||||
match self.calls.lock() {
|
||||
Ok(calls) => calls.clone(),
|
||||
@@ -142,6 +161,21 @@ mod tests {
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for RecordingFileSystem {
|
||||
fn canonicalize(&self, path: &AbsolutePathBuf) -> io::Result<AbsolutePathBuf> {
|
||||
self.push_call(RecordedCall {
|
||||
method: RecordedMethod::Canonicalize,
|
||||
path: path.clone(),
|
||||
sandbox: None,
|
||||
});
|
||||
if !self.canonicalize_supported {
|
||||
return Err(io::Error::new(
|
||||
io::ErrorKind::Unsupported,
|
||||
"filesystem does not support canonicalization",
|
||||
));
|
||||
}
|
||||
Ok(path.parent().unwrap())
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
@@ -318,4 +352,63 @@ mod tests {
|
||||
let set = HashSet::from([left, same, different_path, different_fs]);
|
||||
assert_eq!(set.len(), 3);
|
||||
}
|
||||
#[test]
|
||||
fn canonicalize_if_exists_keeps_bound_file_system_identity() {
|
||||
let path = std::env::temp_dir().join("skills/demo").abs();
|
||||
let file_system = Arc::new(RecordingFileSystem::default());
|
||||
let path_ref = EnvironmentPathRef::new(file_system.clone(), path.clone());
|
||||
|
||||
let canonicalized = path_ref.canonicalize_if_exists();
|
||||
|
||||
assert_eq!(canonicalized.path(), &path.parent().unwrap());
|
||||
assert_eq!(
|
||||
canonicalized,
|
||||
EnvironmentPathRef::new(file_system.clone(), path.parent().unwrap())
|
||||
);
|
||||
assert_eq!(
|
||||
file_system.recorded_calls(),
|
||||
vec![RecordedCall {
|
||||
method: RecordedMethod::Canonicalize,
|
||||
path,
|
||||
sandbox: None,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn canonicalize_if_exists_keeps_unsupported_path() {
|
||||
let path = std::env::temp_dir().join("skills/demo").abs();
|
||||
let path_ref =
|
||||
EnvironmentPathRef::new(Arc::new(RecordingFileSystem::without_canonicalize()), path);
|
||||
|
||||
assert_eq!(path_ref.canonicalize_if_exists(), path_ref);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn join_relative_keeps_literal_tilde_under_bound_path() {
|
||||
let path_ref = EnvironmentPathRef::local(std::env::temp_dir().join("skills").abs());
|
||||
|
||||
assert_eq!(
|
||||
path_ref
|
||||
.join_relative(Path::new("~"))
|
||||
.map(|path_ref| path_ref.path().clone()),
|
||||
Some(std::env::temp_dir().join("skills/~").abs())
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn join_relative_rejects_parent_dirs() {
|
||||
let path_ref = EnvironmentPathRef::local(std::env::temp_dir().join("skills").abs());
|
||||
|
||||
assert_eq!(path_ref.join_relative(Path::new("../outside")), None);
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn join_relative_rejects_windows_prefixed_and_rooted_paths() {
|
||||
let path_ref = EnvironmentPathRef::local(std::env::temp_dir().join("skills").abs());
|
||||
|
||||
assert_eq!(path_ref.join_relative(Path::new(r"C:temp")), None);
|
||||
assert_eq!(path_ref.join_relative(Path::new(r"\temp")), None);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -79,6 +79,10 @@ impl LocalFileSystem {
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for LocalFileSystem {
|
||||
fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult<AbsolutePathBuf> {
|
||||
self.unsandboxed.canonicalize(path)
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
@@ -152,6 +156,10 @@ impl ExecutorFileSystem for LocalFileSystem {
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for UnsandboxedFileSystem {
|
||||
fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult<AbsolutePathBuf> {
|
||||
self.file_system.canonicalize(path)
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
@@ -238,6 +246,10 @@ impl ExecutorFileSystem for UnsandboxedFileSystem {
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for DirectFileSystem {
|
||||
fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult<AbsolutePathBuf> {
|
||||
path.canonicalize()
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
|
||||
@@ -23,6 +23,7 @@ pub const FS_READ_FILE_METHOD: &str = "fs/readFile";
|
||||
pub const FS_WRITE_FILE_METHOD: &str = "fs/writeFile";
|
||||
pub const FS_CREATE_DIRECTORY_METHOD: &str = "fs/createDirectory";
|
||||
pub const FS_GET_METADATA_METHOD: &str = "fs/getMetadata";
|
||||
pub const FS_CANONICALIZE_METHOD: &str = "fs/canonicalize";
|
||||
pub const FS_READ_DIRECTORY_METHOD: &str = "fs/readDirectory";
|
||||
pub const FS_REMOVE_METHOD: &str = "fs/remove";
|
||||
pub const FS_COPY_METHOD: &str = "fs/copy";
|
||||
@@ -211,6 +212,18 @@ pub struct FsGetMetadataResponse {
|
||||
pub modified_at_ms: i64,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsCanonicalizeParams {
|
||||
pub path: AbsolutePathBuf,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsCanonicalizeResponse {
|
||||
pub path: AbsolutePathBuf,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsReadDirectoryParams {
|
||||
|
||||
@@ -2,6 +2,7 @@ use async_trait::async_trait;
|
||||
use base64::Engine as _;
|
||||
use base64::engine::general_purpose::STANDARD;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use std::thread;
|
||||
use tokio::io;
|
||||
use tracing::trace;
|
||||
|
||||
@@ -15,6 +16,7 @@ use crate::FileSystemSandboxContext;
|
||||
use crate::ReadDirectoryEntry;
|
||||
use crate::RemoveOptions;
|
||||
use crate::client::LazyRemoteExecServerClient;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
@@ -40,6 +42,29 @@ impl RemoteFileSystem {
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for RemoteFileSystem {
|
||||
fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult<AbsolutePathBuf> {
|
||||
trace!("remote fs canonicalize");
|
||||
let runtime = tokio::runtime::Handle::try_current().map_err(|err| {
|
||||
io::Error::other(format!("remote fs canonicalize requires runtime: {err}"))
|
||||
})?;
|
||||
let client = self.client.clone();
|
||||
let path = path.clone();
|
||||
// ExecutorFileSystem::canonicalize is synchronous, so remote callers have to hop through
|
||||
// the active runtime from a helper thread to reach the JSON-RPC filesystem surface.
|
||||
thread::spawn(move || {
|
||||
runtime.block_on(async move {
|
||||
let client = client.get().await.map_err(map_remote_error)?;
|
||||
let response = client
|
||||
.fs_canonicalize(FsCanonicalizeParams { path })
|
||||
.await
|
||||
.map_err(map_remote_error)?;
|
||||
Ok(response.path)
|
||||
})
|
||||
})
|
||||
.join()
|
||||
.map_err(|_| io::Error::other("remote fs canonicalize thread panicked"))?
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
|
||||
@@ -11,6 +11,8 @@ use crate::ExecutorFileSystem;
|
||||
use crate::RemoveOptions;
|
||||
use crate::local_file_system::LocalFileSystem;
|
||||
use crate::protocol::FS_WRITE_FILE_METHOD;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCanonicalizeResponse;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCopyResponse;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
@@ -106,6 +108,17 @@ impl FileSystemHandler {
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) async fn canonicalize(
|
||||
&self,
|
||||
params: FsCanonicalizeParams,
|
||||
) -> Result<FsCanonicalizeResponse, JSONRPCErrorError> {
|
||||
let path = self
|
||||
.file_system
|
||||
.canonicalize(¶ms.path)
|
||||
.map_err(map_fs_error)?;
|
||||
Ok(FsCanonicalizeResponse { path })
|
||||
}
|
||||
|
||||
pub(crate) async fn read_directory(
|
||||
&self,
|
||||
params: FsReadDirectoryParams,
|
||||
|
||||
@@ -16,6 +16,8 @@ use crate::client::http_client::PendingReqwestHttpBodyStream;
|
||||
use crate::client::http_client::ReqwestHttpRequestRunner;
|
||||
use crate::protocol::ExecParams;
|
||||
use crate::protocol::ExecResponse;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCanonicalizeResponse;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCopyResponse;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
@@ -240,6 +242,14 @@ impl ExecServerHandler {
|
||||
self.file_system.get_metadata(params).await
|
||||
}
|
||||
|
||||
pub(crate) async fn fs_canonicalize(
|
||||
&self,
|
||||
params: FsCanonicalizeParams,
|
||||
) -> Result<FsCanonicalizeResponse, JSONRPCErrorError> {
|
||||
self.require_initialized_for("filesystem")?;
|
||||
self.file_system.canonicalize(params).await
|
||||
}
|
||||
|
||||
pub(crate) async fn fs_read_directory(
|
||||
&self,
|
||||
params: FsReadDirectoryParams,
|
||||
|
||||
@@ -5,6 +5,7 @@ use crate::protocol::EXEC_READ_METHOD;
|
||||
use crate::protocol::EXEC_TERMINATE_METHOD;
|
||||
use crate::protocol::EXEC_WRITE_METHOD;
|
||||
use crate::protocol::ExecParams;
|
||||
use crate::protocol::FS_CANONICALIZE_METHOD;
|
||||
use crate::protocol::FS_COPY_METHOD;
|
||||
use crate::protocol::FS_CREATE_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_GET_METADATA_METHOD;
|
||||
@@ -12,6 +13,7 @@ use crate::protocol::FS_READ_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_READ_FILE_METHOD;
|
||||
use crate::protocol::FS_REMOVE_METHOD;
|
||||
use crate::protocol::FS_WRITE_FILE_METHOD;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
@@ -96,6 +98,12 @@ pub(crate) fn build_router() -> RpcRouter<ExecServerHandler> {
|
||||
handler.fs_get_metadata(params).await
|
||||
},
|
||||
);
|
||||
router.request(
|
||||
FS_CANONICALIZE_METHOD,
|
||||
|handler: Arc<ExecServerHandler>, params: FsCanonicalizeParams| async move {
|
||||
handler.fs_canonicalize(params).await
|
||||
},
|
||||
);
|
||||
router.request(
|
||||
FS_READ_DIRECTORY_METHOD,
|
||||
|handler: Arc<ExecServerHandler>, params: FsReadDirectoryParams| async move {
|
||||
|
||||
@@ -365,6 +365,18 @@ async fn file_system_methods_cover_surface_area(use_remote: bool) -> Result<()>
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
|
||||
let source_link = tmp.path().join("source-link");
|
||||
symlink(&source_dir, &source_link)?;
|
||||
let canonical_nested = file_system
|
||||
.canonicalize(&absolute_path(source_link.join("nested").join("note.txt")))
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert_eq!(
|
||||
canonical_nested,
|
||||
absolute_path(std::fs::canonicalize(
|
||||
source_dir.join("nested").join("note.txt")
|
||||
)?)
|
||||
);
|
||||
|
||||
let nested_file_contents = file_system
|
||||
.read_file(&absolute_path(nested_file.clone()), /*sandbox*/ None)
|
||||
.await
|
||||
|
||||
@@ -133,6 +133,17 @@ pub type FileSystemResult<T> = io::Result<T>;
|
||||
/// a remote environment.
|
||||
#[async_trait]
|
||||
pub trait ExecutorFileSystem: Send + Sync {
|
||||
/// Resolves a path within this filesystem when supported.
|
||||
///
|
||||
/// Implementations that cannot resolve bound paths may return
|
||||
/// [`io::ErrorKind::Unsupported`].
|
||||
fn canonicalize(&self, _path: &AbsolutePathBuf) -> FileSystemResult<AbsolutePathBuf> {
|
||||
Err(io::Error::new(
|
||||
io::ErrorKind::Unsupported,
|
||||
"filesystem does not support canonicalization",
|
||||
))
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
|
||||
Reference in New Issue
Block a user