diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index a05a5af36..b109d6443 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1543,11 +1543,14 @@ dependencies = [ "anyhow", "assert_cmd", "assert_matches", + "codex-exec-server", + "codex-utils-absolute-path", "codex-utils-cargo-bin", "pretty_assertions", "similar", "tempfile", "thiserror 2.0.18", + "tokio", "tree-sitter", "tree-sitter-bash", ] @@ -1558,9 +1561,11 @@ version = "0.0.0" dependencies = [ "anyhow", "codex-apply-patch", + "codex-exec-server", "codex-linux-sandbox", "codex-sandboxing", "codex-shell-escalation", + "codex-utils-absolute-path", "codex-utils-home-dir", "dotenvy", "tempfile", diff --git a/codex-rs/apply-patch/Cargo.toml b/codex-rs/apply-patch/Cargo.toml index 2fad46c2a..ba4fa5e48 100644 --- a/codex-rs/apply-patch/Cargo.toml +++ b/codex-rs/apply-patch/Cargo.toml @@ -17,8 +17,11 @@ workspace = true [dependencies] anyhow = { workspace = true } +codex-exec-server = { workspace = true } +codex-utils-absolute-path = { workspace = true } similar = { workspace = true } thiserror = { workspace = true } +tokio = { workspace = true, features = ["macros", "rt"] } tree-sitter = { workspace = true } tree-sitter-bash = { workspace = true } diff --git a/codex-rs/apply-patch/src/invocation.rs b/codex-rs/apply-patch/src/invocation.rs index 519671798..3b0db2fa9 100644 --- a/codex-rs/apply-patch/src/invocation.rs +++ b/codex-rs/apply-patch/src/invocation.rs @@ -2,6 +2,8 @@ use std::collections::HashMap; use std::path::Path; use std::sync::LazyLock; +use codex_exec_server::ExecutorFileSystem; +use codex_utils_absolute_path::AbsolutePathBuf; use tree_sitter::Parser; use tree_sitter::Query; use tree_sitter::QueryCursor; @@ -129,7 +131,11 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { /// cwd must be an absolute path so that we can resolve relative paths in the /// patch. -pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApplyPatchVerified { +pub async fn maybe_parse_apply_patch_verified( + argv: &[String], + cwd: &AbsolutePathBuf, + fs: &dyn ExecutorFileSystem, +) -> MaybeApplyPatchVerified { // Detect a raw patch body passed directly as the command or as the body of a shell // script. In these cases, report an explicit error rather than applying the patch. if let [body] = argv @@ -151,24 +157,20 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp }) => { let effective_cwd = workdir .as_ref() - .map(|dir| { - let path = Path::new(dir); - if path.is_absolute() { - path.to_path_buf() - } else { - cwd.join(path) - } - }) - .unwrap_or_else(|| cwd.to_path_buf()); + .map(|dir| cwd.join(Path::new(dir))) + .unwrap_or_else(|| cwd.clone()); let mut changes = HashMap::new(); for hunk in hunks { let path = hunk.resolve_path(&effective_cwd); match hunk { Hunk::AddFile { contents, .. } => { - changes.insert(path, ApplyPatchFileChange::Add { content: contents }); + changes.insert( + path.into_path_buf(), + ApplyPatchFileChange::Add { content: contents }, + ); } Hunk::DeleteFile { .. } => { - let content = match std::fs::read_to_string(&path) { + let content = match fs.read_file_text(&path).await { Ok(content) => content, Err(e) => { return MaybeApplyPatchVerified::CorrectnessError( @@ -179,7 +181,10 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp ); } }; - changes.insert(path, ApplyPatchFileChange::Delete { content }); + changes.insert( + path.into_path_buf(), + ApplyPatchFileChange::Delete { content }, + ); } Hunk::UpdateFile { move_path, chunks, .. @@ -187,17 +192,17 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp let ApplyPatchFileUpdate { unified_diff, content: contents, - } = match unified_diff_from_chunks(&path, &chunks) { + } = match unified_diff_from_chunks(&path, &chunks, fs).await { Ok(diff) => diff, Err(e) => { return MaybeApplyPatchVerified::CorrectnessError(e); } }; changes.insert( - path, + path.into_path_buf(), ApplyPatchFileChange::Update { unified_diff, - move_path: move_path.map(|p| effective_cwd.join(p)), + move_path: move_path.map(|p| effective_cwd.join(p).into_path_buf()), new_content: contents, }, ); @@ -371,7 +376,10 @@ fn extract_apply_patch_from_bash( #[cfg(test)] mod tests { use super::*; + use crate::unified_diff_from_chunks; use assert_matches::assert_matches; + use codex_exec_server::LOCAL_FS; + use codex_utils_absolute_path::test_support::PathExt; use pretty_assertions::assert_eq; use std::fs; use std::path::PathBuf; @@ -450,30 +458,40 @@ mod tests { ); } - #[test] - fn test_implicit_patch_single_arg_is_error() { + #[tokio::test] + async fn test_implicit_patch_single_arg_is_error() { let patch = "*** Begin Patch\n*** Add File: foo\n+hi\n*** End Patch".to_string(); let args = vec![patch]; let dir = tempdir().unwrap(); assert_matches!( - maybe_parse_apply_patch_verified(&args, dir.path()), + maybe_parse_apply_patch_verified( + &args, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + LOCAL_FS.as_ref() + ) + .await, MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation) ); } - #[test] - fn test_implicit_patch_bash_script_is_error() { + #[tokio::test] + async fn test_implicit_patch_bash_script_is_error() { let script = "*** Begin Patch\n*** Add File: foo\n+hi\n*** End Patch"; let args = args_bash(script); let dir = tempdir().unwrap(); assert_matches!( - maybe_parse_apply_patch_verified(&args, dir.path()), + maybe_parse_apply_patch_verified( + &args, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + LOCAL_FS.as_ref() + ) + .await, MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation) ); } - #[test] - fn test_literal() { + #[tokio::test] + async fn test_literal() { let args = strs_to_strings(&[ "apply_patch", r#"*** Begin Patch @@ -497,8 +515,8 @@ mod tests { } } - #[test] - fn test_literal_applypatch() { + #[tokio::test] + async fn test_literal_applypatch() { let args = strs_to_strings(&[ "applypatch", r#"*** Begin Patch @@ -522,20 +540,20 @@ mod tests { } } - #[test] - fn test_heredoc() { + #[tokio::test] + async fn test_heredoc() { assert_match(&heredoc_script(""), /*expected_workdir*/ None); } - #[test] - fn test_heredoc_non_login_shell() { + #[tokio::test] + async fn test_heredoc_non_login_shell() { let script = heredoc_script(""); let args = strs_to_strings(&["bash", "-c", &script]); assert_match_args(args, /*expected_workdir*/ None); } - #[test] - fn test_heredoc_applypatch() { + #[tokio::test] + async fn test_heredoc_applypatch() { let args = strs_to_strings(&[ "bash", "-lc", @@ -562,96 +580,96 @@ PATCH"#, } } - #[test] - fn test_powershell_heredoc() { + #[tokio::test] + async fn test_powershell_heredoc() { let script = heredoc_script(""); assert_match_args(args_powershell(&script), /*expected_workdir*/ None); } - #[test] - fn test_powershell_heredoc_no_profile() { + #[tokio::test] + async fn test_powershell_heredoc_no_profile() { let script = heredoc_script(""); assert_match_args( args_powershell_no_profile(&script), /*expected_workdir*/ None, ); } - #[test] - fn test_pwsh_heredoc() { + #[tokio::test] + async fn test_pwsh_heredoc() { let script = heredoc_script(""); assert_match_args(args_pwsh(&script), /*expected_workdir*/ None); } - #[test] - fn test_cmd_heredoc_with_cd() { + #[tokio::test] + async fn test_cmd_heredoc_with_cd() { let script = heredoc_script("cd foo && "); assert_match_args(args_cmd(&script), Some("foo")); } - #[test] - fn test_heredoc_with_leading_cd() { + #[tokio::test] + async fn test_heredoc_with_leading_cd() { assert_match(&heredoc_script("cd foo && "), Some("foo")); } - #[test] - fn test_cd_with_semicolon_is_ignored() { + #[tokio::test] + async fn test_cd_with_semicolon_is_ignored() { assert_not_match(&heredoc_script("cd foo; ")); } - #[test] - fn test_cd_or_apply_patch_is_ignored() { + #[tokio::test] + async fn test_cd_or_apply_patch_is_ignored() { assert_not_match(&heredoc_script("cd bar || ")); } - #[test] - fn test_cd_pipe_apply_patch_is_ignored() { + #[tokio::test] + async fn test_cd_pipe_apply_patch_is_ignored() { assert_not_match(&heredoc_script("cd bar | ")); } - #[test] - fn test_cd_single_quoted_path_with_spaces() { + #[tokio::test] + async fn test_cd_single_quoted_path_with_spaces() { assert_match(&heredoc_script("cd 'foo bar' && "), Some("foo bar")); } - #[test] - fn test_cd_double_quoted_path_with_spaces() { + #[tokio::test] + async fn test_cd_double_quoted_path_with_spaces() { assert_match(&heredoc_script("cd \"foo bar\" && "), Some("foo bar")); } - #[test] - fn test_echo_and_apply_patch_is_ignored() { + #[tokio::test] + async fn test_echo_and_apply_patch_is_ignored() { assert_not_match(&heredoc_script("echo foo && ")); } - #[test] - fn test_apply_patch_with_arg_is_ignored() { + #[tokio::test] + async fn test_apply_patch_with_arg_is_ignored() { let script = "apply_patch foo <<'PATCH'\n*** Begin Patch\n*** Add File: foo\n+hi\n*** End Patch\nPATCH"; assert_not_match(script); } - #[test] - fn test_double_cd_then_apply_patch_is_ignored() { + #[tokio::test] + async fn test_double_cd_then_apply_patch_is_ignored() { assert_not_match(&heredoc_script("cd foo && cd bar && ")); } - #[test] - fn test_cd_two_args_is_ignored() { + #[tokio::test] + async fn test_cd_two_args_is_ignored() { assert_not_match(&heredoc_script("cd foo bar && ")); } - #[test] - fn test_cd_then_apply_patch_then_extra_is_ignored() { + #[tokio::test] + async fn test_cd_then_apply_patch_then_extra_is_ignored() { let script = heredoc_script_ps("cd bar && ", " && echo done"); assert_not_match(&script); } - #[test] - fn test_echo_then_cd_and_apply_patch_is_ignored() { + #[tokio::test] + async fn test_echo_then_cd_and_apply_patch_is_ignored() { // Ensure preceding commands before the `cd && apply_patch <<...` sequence do not match. assert_not_match(&heredoc_script("echo foo; cd bar && ")); } - #[test] - fn test_unified_diff_last_line_replacement() { + #[tokio::test] + async fn test_unified_diff_last_line_replacement() { // Replace the very last line of the file. let dir = tempdir().unwrap(); let path = dir.path().join("last.txt"); @@ -674,7 +692,10 @@ PATCH"#, _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -2,2 +2,2 @@ bar -baz @@ -687,8 +708,8 @@ PATCH"#, assert_eq!(expected, diff); } - #[test] - fn test_unified_diff_insert_at_eof() { + #[tokio::test] + async fn test_unified_diff_insert_at_eof() { // Insert a new line at end‑of‑file. let dir = tempdir().unwrap(); let path = dir.path().join("insert.txt"); @@ -709,7 +730,10 @@ PATCH"#, _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -3 +3,2 @@ baz +quux @@ -721,8 +745,8 @@ PATCH"#, assert_eq!(expected, diff); } - #[test] - fn test_apply_patch_should_resolve_absolute_paths_in_cwd() { + #[tokio::test] + async fn test_apply_patch_should_resolve_absolute_paths_in_cwd() { let session_dir = tempdir().unwrap(); let relative_path = "source.txt"; @@ -742,7 +766,12 @@ PATCH"#, .to_string(), ]; - let result = maybe_parse_apply_patch_verified(&argv, session_dir.path()); + let result = maybe_parse_apply_patch_verified( + &argv, + &AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(), + LOCAL_FS.as_ref(), + ) + .await; // Verify the patch contents - as otherwise we may have pulled contents // from the wrong file (as we're using relative paths) @@ -762,13 +791,13 @@ PATCH"#, }, )]), patch: argv[1].clone(), - cwd: session_dir.path().to_path_buf(), + cwd: AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(), }) ); } - #[test] - fn test_apply_patch_resolves_move_path_with_effective_cwd() { + #[tokio::test] + async fn test_apply_patch_resolves_move_path_with_effective_cwd() { let session_dir = tempdir().unwrap(); let worktree_rel = "alt"; let worktree_dir = session_dir.path().join(worktree_rel); @@ -790,13 +819,18 @@ PATCH"#, let shell_script = format!("cd {worktree_rel} && apply_patch <<'PATCH'\n{patch}\nPATCH"); let argv = vec!["bash".into(), "-lc".into(), shell_script]; - let result = maybe_parse_apply_patch_verified(&argv, session_dir.path()); + let result = maybe_parse_apply_patch_verified( + &argv, + &AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(), + LOCAL_FS.as_ref(), + ) + .await; let action = match result { MaybeApplyPatchVerified::Body(action) => action, other => panic!("expected verified body, got {other:?}"), }; - assert_eq!(action.cwd, worktree_dir); + assert_eq!(action.cwd.as_path(), worktree_dir.as_path()); let change = action .changes() diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 3041c4b2c..6b6aba2df 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -4,11 +4,16 @@ mod seek_sequence; mod standalone_executable; use std::collections::HashMap; +use std::io; use std::path::Path; use std::path::PathBuf; use anyhow::Context; use anyhow::Result; +use codex_exec_server::CreateDirectoryOptions; +use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::RemoveOptions; +use codex_utils_absolute_path::AbsolutePathBuf; pub use parser::Hunk; pub use parser::ParseError; use parser::ParseError::*; @@ -134,7 +139,7 @@ pub struct ApplyPatchAction { pub patch: String, /// The working directory that was used to resolve relative paths in the patch. - pub cwd: PathBuf, + pub cwd: AbsolutePathBuf, } impl ApplyPatchAction { @@ -149,11 +154,7 @@ impl ApplyPatchAction { /// Should be used exclusively for testing. (Not worth the overhead of /// creating a feature flag for this.) - pub fn new_add_for_test(path: &Path, content: String) -> Self { - if !path.is_absolute() { - panic!("path must be absolute"); - } - + pub fn new_add_for_test(path: &AbsolutePathBuf, content: String) -> Self { #[expect(clippy::expect_used)] let filename = path .file_name() @@ -170,20 +171,19 @@ impl ApplyPatchAction { #[expect(clippy::expect_used)] Self { changes, - cwd: path - .parent() - .expect("path should have parent") - .to_path_buf(), + cwd: path.parent().expect("path should have parent"), patch, } } } /// Applies the patch and prints the result to stdout/stderr. -pub fn apply_patch( +pub async fn apply_patch( patch: &str, + cwd: &AbsolutePathBuf, stdout: &mut impl std::io::Write, stderr: &mut impl std::io::Write, + fs: &dyn ExecutorFileSystem, ) -> Result<(), ApplyPatchError> { let hunks = match parse_patch(patch) { Ok(source) => source.hunks, @@ -207,45 +207,21 @@ pub fn apply_patch( } }; - apply_hunks(&hunks, stdout, stderr)?; + apply_hunks(&hunks, cwd, stdout, stderr, fs).await?; Ok(()) } /// Applies hunks and continues to update stdout/stderr -pub fn apply_hunks( +pub async fn apply_hunks( hunks: &[Hunk], + cwd: &AbsolutePathBuf, stdout: &mut impl std::io::Write, stderr: &mut impl std::io::Write, + fs: &dyn ExecutorFileSystem, ) -> Result<(), ApplyPatchError> { - let _existing_paths: Vec<&Path> = hunks - .iter() - .filter_map(|hunk| match hunk { - Hunk::AddFile { .. } => { - // The file is being added, so it doesn't exist yet. - None - } - Hunk::DeleteFile { path } => Some(path.as_path()), - Hunk::UpdateFile { - path, move_path, .. - } => match move_path { - Some(move_path) => { - if std::fs::metadata(move_path) - .map(|m| m.is_file()) - .unwrap_or(false) - { - Some(move_path.as_path()) - } else { - None - } - } - None => Some(path.as_path()), - }, - }) - .collect::>(); - // Delegate to a helper that applies each hunk to the filesystem. - match apply_hunks_to_files(hunks) { + match apply_hunks_to_files(hunks, cwd, fs).await { Ok(affected) => { print_summary(&affected, stdout).map_err(ApplyPatchError::from)?; Ok(()) @@ -267,7 +243,8 @@ pub fn apply_hunks( /// Applies each parsed patch hunk to the filesystem. /// Returns an error if any of the changes could not be applied. -/// Tracks file paths affected by applying a patch. +/// Tracks file paths affected by applying a patch, preserving the path spelling +/// from the patch for user-facing summaries. pub struct AffectedPaths { pub added: Vec, pub modified: Vec, @@ -276,7 +253,11 @@ pub struct AffectedPaths { /// Apply the hunks to the filesystem, returning which files were added, modified, or deleted. /// Returns an error if the patch could not be applied. -fn apply_hunks_to_files(hunks: &[Hunk]) -> anyhow::Result { +async fn apply_hunks_to_files( + hunks: &[Hunk], + cwd: &AbsolutePathBuf, + fs: &dyn ExecutorFileSystem, +) -> anyhow::Result { if hunks.is_empty() { anyhow::bail!("No files were modified."); } @@ -285,48 +266,97 @@ fn apply_hunks_to_files(hunks: &[Hunk]) -> anyhow::Result { let mut modified: Vec = Vec::new(); let mut deleted: Vec = Vec::new(); for hunk in hunks { + let affected_path = hunk.path().to_path_buf(); + let path_abs = hunk.resolve_path(cwd); match hunk { - Hunk::AddFile { path, contents } => { - if let Some(parent) = path.parent() - && !parent.as_os_str().is_empty() - { - std::fs::create_dir_all(parent).with_context(|| { - format!("Failed to create parent directories for {}", path.display()) - })?; + Hunk::AddFile { contents, .. } => { + if let Some(parent_abs) = path_abs.parent() { + fs.create_directory(&parent_abs, CreateDirectoryOptions { recursive: true }) + .await + .with_context(|| { + format!( + "Failed to create parent directories for {}", + path_abs.display() + ) + })?; } - std::fs::write(path, contents) - .with_context(|| format!("Failed to write file {}", path.display()))?; - added.push(path.clone()); + fs.write_file(&path_abs, contents.clone().into_bytes()) + .await + .with_context(|| format!("Failed to write file {}", path_abs.display()))?; + added.push(affected_path); } - Hunk::DeleteFile { path } => { - std::fs::remove_file(path) - .with_context(|| format!("Failed to delete file {}", path.display()))?; - deleted.push(path.clone()); + Hunk::DeleteFile { .. } => { + let result: io::Result<()> = async { + let metadata = fs.get_metadata(&path_abs).await?; + if metadata.is_directory { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "path is a directory", + )); + } + fs.remove( + &path_abs, + RemoveOptions { + recursive: false, + force: false, + }, + ) + .await + } + .await; + result.with_context(|| format!("Failed to delete file {}", path_abs.display()))?; + deleted.push(affected_path); } Hunk::UpdateFile { - path, - move_path, - chunks, + move_path, chunks, .. } => { let AppliedPatch { new_contents, .. } = - derive_new_contents_from_chunks(path, chunks)?; + derive_new_contents_from_chunks(&path_abs, chunks, fs).await?; if let Some(dest) = move_path { - if let Some(parent) = dest.parent() - && !parent.as_os_str().is_empty() - { - std::fs::create_dir_all(parent).with_context(|| { - format!("Failed to create parent directories for {}", dest.display()) + let dest_abs = AbsolutePathBuf::resolve_path_against_base(dest, cwd); + if let Some(parent_abs) = dest_abs.parent() { + fs.create_directory( + &parent_abs, + CreateDirectoryOptions { recursive: true }, + ) + .await + .with_context(|| { + format!( + "Failed to create parent directories for {}", + dest_abs.display() + ) })?; } - std::fs::write(dest, new_contents) - .with_context(|| format!("Failed to write file {}", dest.display()))?; - std::fs::remove_file(path) - .with_context(|| format!("Failed to remove original {}", path.display()))?; - modified.push(dest.clone()); + fs.write_file(&dest_abs, new_contents.into_bytes()) + .await + .with_context(|| format!("Failed to write file {}", dest_abs.display()))?; + let result: io::Result<()> = async { + let metadata = fs.get_metadata(&path_abs).await?; + if metadata.is_directory { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "path is a directory", + )); + } + fs.remove( + &path_abs, + RemoveOptions { + recursive: false, + force: false, + }, + ) + .await + } + .await; + result.with_context(|| { + format!("Failed to remove original {}", path_abs.display()) + })?; + modified.push(affected_path); } else { - std::fs::write(path, new_contents) - .with_context(|| format!("Failed to write file {}", path.display()))?; - modified.push(path.clone()); + fs.write_file(&path_abs, new_contents.into_bytes()) + .await + .with_context(|| format!("Failed to write file {}", path_abs.display()))?; + modified.push(affected_path); } } } @@ -345,19 +375,17 @@ struct AppliedPatch { /// Return *only* the new file contents (joined into a single `String`) after /// applying the chunks to the file at `path`. -fn derive_new_contents_from_chunks( - path: &Path, +async fn derive_new_contents_from_chunks( + path_abs: &AbsolutePathBuf, chunks: &[UpdateFileChunk], + fs: &dyn ExecutorFileSystem, ) -> std::result::Result { - let original_contents = match std::fs::read_to_string(path) { - Ok(contents) => contents, - Err(err) => { - return Err(ApplyPatchError::IoError(IoError { - context: format!("Failed to read file to update {}", path.display()), - source: err, - })); - } - }; + let original_contents = fs.read_file_text(path_abs).await.map_err(|err| { + ApplyPatchError::IoError(IoError { + context: format!("Failed to read file to update {}", path_abs.display()), + source: err, + }) + })?; let mut original_lines: Vec = original_contents.split('\n').map(String::from).collect(); @@ -367,7 +395,7 @@ fn derive_new_contents_from_chunks( original_lines.pop(); } - let replacements = compute_replacements(&original_lines, path, chunks)?; + let replacements = compute_replacements(&original_lines, path_abs.as_path(), chunks)?; let new_lines = apply_replacements(original_lines, &replacements); let mut new_lines = new_lines; if !new_lines.last().is_some_and(String::is_empty) { @@ -508,22 +536,24 @@ pub struct ApplyPatchFileUpdate { content: String, } -pub fn unified_diff_from_chunks( - path: &Path, +pub async fn unified_diff_from_chunks( + path_abs: &AbsolutePathBuf, chunks: &[UpdateFileChunk], + fs: &dyn ExecutorFileSystem, ) -> std::result::Result { - unified_diff_from_chunks_with_context(path, chunks, /*context*/ 1) + unified_diff_from_chunks_with_context(path_abs, chunks, /*context*/ 1, fs).await } -pub fn unified_diff_from_chunks_with_context( - path: &Path, +pub async fn unified_diff_from_chunks_with_context( + path_abs: &AbsolutePathBuf, chunks: &[UpdateFileChunk], context: usize, + fs: &dyn ExecutorFileSystem, ) -> std::result::Result { let AppliedPatch { original_contents, new_contents, - } = derive_new_contents_from_chunks(path, chunks)?; + } = derive_new_contents_from_chunks(path_abs, chunks, fs).await?; let text_diff = TextDiff::from_lines(&original_contents, &new_contents); let unified_diff = text_diff.unified_diff().context_radius(context).to_string(); Ok(ApplyPatchFileUpdate { @@ -554,6 +584,8 @@ pub fn print_summary( #[cfg(test)] mod tests { use super::*; + use codex_exec_server::LOCAL_FS; + use codex_utils_absolute_path::test_support::PathExt; use pretty_assertions::assert_eq; use std::fs; use std::string::ToString; @@ -564,8 +596,8 @@ mod tests { format!("*** Begin Patch\n{body}\n*** End Patch") } - #[test] - fn test_add_file_hunk_creates_file_with_contents() { + #[tokio::test] + async fn test_add_file_hunk_creates_file_with_contents() { let dir = tempdir().unwrap(); let path = dir.path().join("add.txt"); let patch = wrap_patch(&format!( @@ -576,7 +608,15 @@ mod tests { )); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); // Verify expected stdout and stderr outputs. let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); @@ -590,15 +630,88 @@ mod tests { assert_eq!(contents, "ab\ncd\n"); } - #[test] - fn test_delete_file_hunk_removes_file() { + #[tokio::test] + async fn test_apply_patch_hunks_accept_relative_and_absolute_paths() { + let dir = tempdir().unwrap(); + let cwd = dir.path().abs(); + let relative_add = dir.path().join("relative-add.txt"); + let absolute_add = dir.path().join("absolute-add.txt"); + let relative_delete = dir.path().join("relative-delete.txt"); + let absolute_delete = dir.path().join("absolute-delete.txt"); + let relative_update = dir.path().join("relative-update.txt"); + let absolute_update = dir.path().join("absolute-update.txt"); + fs::write(&relative_delete, "delete relative\n").unwrap(); + fs::write(&absolute_delete, "delete absolute\n").unwrap(); + fs::write(&relative_update, "relative old\n").unwrap(); + fs::write(&absolute_update, "absolute old\n").unwrap(); + + let patch = wrap_patch(&format!( + r#"*** Add File: relative-add.txt ++relative add +*** Add File: {} ++absolute add +*** Delete File: relative-delete.txt +*** Delete File: {} +*** Update File: relative-update.txt +@@ +-relative old ++relative new +*** Update File: {} +@@ +-absolute old ++absolute new"#, + absolute_add.display(), + absolute_delete.display(), + absolute_update.display(), + )); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + apply_patch(&patch, &cwd, &mut stdout, &mut stderr, LOCAL_FS.as_ref()) + .await + .unwrap(); + + assert_eq!(fs::read_to_string(&relative_add).unwrap(), "relative add\n"); + assert_eq!(fs::read_to_string(&absolute_add).unwrap(), "absolute add\n"); + assert!(!relative_delete.exists()); + assert!(!absolute_delete.exists()); + assert_eq!( + fs::read_to_string(&relative_update).unwrap(), + "relative new\n" + ); + assert_eq!( + fs::read_to_string(&absolute_update).unwrap(), + "absolute new\n" + ); + assert_eq!(String::from_utf8(stderr).unwrap(), ""); + assert_eq!( + String::from_utf8(stdout).unwrap(), + format!( + "Success. Updated the following files:\nA relative-add.txt\nA {}\nM relative-update.txt\nM {}\nD relative-delete.txt\nD {}\n", + absolute_add.display(), + absolute_update.display(), + absolute_delete.display(), + ) + ); + } + + #[tokio::test] + async fn test_delete_file_hunk_removes_file() { let dir = tempdir().unwrap(); let path = dir.path().join("del.txt"); fs::write(&path, "x").unwrap(); let patch = wrap_patch(&format!("*** Delete File: {}", path.display())); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); let expected_out = format!( @@ -610,8 +723,8 @@ mod tests { assert!(!path.exists()); } - #[test] - fn test_update_file_hunk_modifies_content() { + #[tokio::test] + async fn test_update_file_hunk_modifies_content() { let dir = tempdir().unwrap(); let path = dir.path().join("update.txt"); fs::write(&path, "foo\nbar\n").unwrap(); @@ -625,7 +738,15 @@ mod tests { )); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); // Validate modified file contents and expected stdout/stderr. let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); @@ -639,8 +760,8 @@ mod tests { assert_eq!(contents, "foo\nbaz\n"); } - #[test] - fn test_update_file_hunk_can_move_file() { + #[tokio::test] + async fn test_update_file_hunk_can_move_file() { let dir = tempdir().unwrap(); let src = dir.path().join("src.txt"); let dest = dir.path().join("dst.txt"); @@ -656,7 +777,15 @@ mod tests { )); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); // Validate move semantics and expected stdout/stderr. let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); @@ -673,8 +802,8 @@ mod tests { /// Verify that a single `Update File` hunk with multiple change chunks can update different /// parts of a file and that the file is listed only once in the summary. - #[test] - fn test_multiple_update_chunks_apply_to_single_file() { + #[tokio::test] + async fn test_multiple_update_chunks_apply_to_single_file() { // Start with a file containing four lines. let dir = tempdir().unwrap(); let path = dir.path().join("multi.txt"); @@ -696,7 +825,15 @@ mod tests { )); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); let expected_out = format!( @@ -713,8 +850,8 @@ mod tests { /// replacements in separate chunks that appear in non‑adjacent parts of the /// file. Verifies that all edits are applied and that the summary lists the /// file only once. - #[test] - fn test_update_file_hunk_interleaved_changes() { + #[tokio::test] + async fn test_update_file_hunk_interleaved_changes() { let dir = tempdir().unwrap(); let path = dir.path().join("interleaved.txt"); @@ -745,7 +882,15 @@ mod tests { let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); @@ -761,8 +906,8 @@ mod tests { assert_eq!(contents, "a\nB\nc\nd\nE\nf\ng\n"); } - #[test] - fn test_pure_addition_chunk_followed_by_removal() { + #[tokio::test] + async fn test_pure_addition_chunk_followed_by_removal() { let dir = tempdir().unwrap(); let path = dir.path().join("panic.txt"); fs::write(&path, "line1\nline2\nline3\n").unwrap(); @@ -780,7 +925,15 @@ mod tests { )); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); let contents = fs::read_to_string(path).unwrap(); assert_eq!( contents, @@ -794,8 +947,8 @@ mod tests { /// internal matcher failed requiring an exact byte-for-byte match. The /// fuzzy-matching pass that normalises common punctuation should now bridge /// the gap. - #[test] - fn test_update_line_with_unicode_dash() { + #[tokio::test] + async fn test_update_line_with_unicode_dash() { let dir = tempdir().unwrap(); let path = dir.path().join("unicode.py"); @@ -814,7 +967,15 @@ mod tests { let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); // File should now contain the replaced comment. let expected = "import asyncio # HELLO\n"; @@ -833,8 +994,8 @@ mod tests { assert_eq!(String::from_utf8(stderr).unwrap(), ""); } - #[test] - fn test_unified_diff() { + #[tokio::test] + async fn test_unified_diff() { // Start with a file containing four lines. let dir = tempdir().unwrap(); let path = dir.path().join("multi.txt"); @@ -857,7 +1018,10 @@ mod tests { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, update_file_chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, update_file_chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -1,4 +1,4 @@ foo -bar @@ -873,8 +1037,8 @@ mod tests { assert_eq!(expected, diff); } - #[test] - fn test_unified_diff_first_line_replacement() { + #[tokio::test] + async fn test_unified_diff_first_line_replacement() { // Replace the very first line of the file. let dir = tempdir().unwrap(); let path = dir.path().join("first.txt"); @@ -896,7 +1060,10 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -1,2 +1,2 @@ -foo +FOO @@ -909,8 +1076,8 @@ mod tests { assert_eq!(expected, diff); } - #[test] - fn test_unified_diff_last_line_replacement() { + #[tokio::test] + async fn test_unified_diff_last_line_replacement() { // Replace the very last line of the file. let dir = tempdir().unwrap(); let path = dir.path().join("last.txt"); @@ -933,7 +1100,10 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -2,2 +2,2 @@ bar -baz @@ -946,8 +1116,8 @@ mod tests { assert_eq!(expected, diff); } - #[test] - fn test_unified_diff_insert_at_eof() { + #[tokio::test] + async fn test_unified_diff_insert_at_eof() { // Insert a new line at end‑of‑file. let dir = tempdir().unwrap(); let path = dir.path().join("insert.txt"); @@ -968,7 +1138,10 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -3 +3,2 @@ baz +quux @@ -980,8 +1153,8 @@ mod tests { assert_eq!(expected, diff); } - #[test] - fn test_unified_diff_interleaved_changes() { + #[tokio::test] + async fn test_unified_diff_interleaved_changes() { // Original file with six lines. let dir = tempdir().unwrap(); let path = dir.path().join("interleaved.txt"); @@ -1014,7 +1187,10 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -1,6 +1,7 @@ a @@ -1037,7 +1213,15 @@ mod tests { let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); let contents = fs::read_to_string(path).unwrap(); assert_eq!( contents, @@ -1052,8 +1236,8 @@ g ); } - #[test] - fn test_apply_patch_fails_on_write_error() { + #[tokio::test] + async fn test_apply_patch_fails_on_write_error() { let dir = tempdir().unwrap(); let path = dir.path().join("readonly.txt"); fs::write(&path, "before\n").unwrap(); @@ -1068,7 +1252,14 @@ g let mut stdout = Vec::new(); let mut stderr = Vec::new(); - let result = apply_patch(&patch, &mut stdout, &mut stderr); + let result = apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await; assert!(result.is_err()); } } diff --git a/codex-rs/apply-patch/src/parser.rs b/codex-rs/apply-patch/src/parser.rs index 274a45497..24ebcb146 100644 --- a/codex-rs/apply-patch/src/parser.rs +++ b/codex-rs/apply-patch/src/parser.rs @@ -23,6 +23,9 @@ //! The parser below is a little more lenient than the explicit spec and allows for //! leading/trailing whitespace around patch markers. use crate::ApplyPatchArgs; +use codex_utils_absolute_path::AbsolutePathBuf; +#[cfg(test)] +use codex_utils_absolute_path::test_support::PathBufExt; use std::path::Path; use std::path::PathBuf; @@ -76,11 +79,28 @@ pub enum Hunk { } impl Hunk { - pub fn resolve_path(&self, cwd: &Path) -> PathBuf { + pub fn resolve_path(&self, cwd: &AbsolutePathBuf) -> AbsolutePathBuf { + let path = match self { + Hunk::UpdateFile { path, .. } => path, + Hunk::AddFile { .. } | Hunk::DeleteFile { .. } => self.path(), + }; + AbsolutePathBuf::resolve_path_against_base(path, cwd) + } + + /// Returns the path affected by this hunk, using the move destination for rename hunks. + pub fn path(&self) -> &Path { match self { - Hunk::AddFile { path, .. } => cwd.join(path), - Hunk::DeleteFile { path } => cwd.join(path), - Hunk::UpdateFile { path, .. } => cwd.join(path), + Hunk::AddFile { path, .. } => path, + Hunk::DeleteFile { path } => path, + Hunk::UpdateFile { + move_path: Some(path), + .. + } => path, + Hunk::UpdateFile { + path, + move_path: None, + .. + } => path, } } } @@ -583,6 +603,108 @@ fn test_parse_patch() { ); } +#[test] +fn test_parse_patch_accepts_relative_and_absolute_hunk_paths() { + let dir = tempfile::tempdir().unwrap(); + let absolute_delete = dir.path().join("absolute-delete.py").abs(); + let absolute_update = dir.path().join("absolute-update.py").abs(); + let patch_text = format!( + r#"*** Begin Patch +*** Add File: relative-add.py ++content +*** Delete File: {} +*** Update File: {} +@@ +-old ++new +*** End Patch"#, + absolute_delete.display(), + absolute_update.display() + ); + + assert_eq!( + parse_patch_text(&patch_text, ParseMode::Strict) + .unwrap() + .hunks, + vec![ + AddFile { + path: PathBuf::from("relative-add.py"), + contents: "content\n".to_string() + }, + DeleteFile { + path: absolute_delete.to_path_buf() + }, + UpdateFile { + path: absolute_update.to_path_buf(), + move_path: None, + chunks: vec![UpdateFileChunk { + change_context: None, + old_lines: vec!["old".to_string()], + new_lines: vec!["new".to_string()], + is_end_of_file: false + }] + }, + ] + ); +} + +#[test] +fn test_hunk_resolve_path_accepts_relative_and_absolute_paths() { + let cwd_dir = tempfile::tempdir().unwrap(); + let cwd = cwd_dir.path().to_path_buf().abs(); + let absolute_dir = tempfile::tempdir().unwrap(); + let absolute_add = absolute_dir.path().join("absolute-add.py").abs(); + let absolute_delete = absolute_dir.path().join("absolute-delete.py").abs(); + let absolute_update = absolute_dir.path().join("absolute-update.py").abs(); + + for (hunk, expected_path) in [ + ( + AddFile { + path: PathBuf::from("relative-add.py"), + contents: String::new(), + }, + cwd.join("relative-add.py"), + ), + ( + DeleteFile { + path: PathBuf::from("relative-delete.py"), + }, + cwd.join("relative-delete.py"), + ), + ( + UpdateFile { + path: PathBuf::from("relative-update.py"), + move_path: None, + chunks: Vec::new(), + }, + cwd.join("relative-update.py"), + ), + ( + AddFile { + path: absolute_add.to_path_buf(), + contents: String::new(), + }, + absolute_add, + ), + ( + DeleteFile { + path: absolute_delete.to_path_buf(), + }, + absolute_delete, + ), + ( + UpdateFile { + path: absolute_update.to_path_buf(), + move_path: None, + chunks: Vec::new(), + }, + absolute_update, + ), + ] { + assert_eq!(hunk.resolve_path(&cwd), expected_path); + } +} + #[test] fn test_parse_patch_lenient() { let patch_text = r#"*** Begin Patch diff --git a/codex-rs/apply-patch/src/standalone_executable.rs b/codex-rs/apply-patch/src/standalone_executable.rs index d77a82fa9..149bfd338 100644 --- a/codex-rs/apply-patch/src/standalone_executable.rs +++ b/codex-rs/apply-patch/src/standalone_executable.rs @@ -48,7 +48,30 @@ pub fn run_main() -> i32 { let mut stdout = std::io::stdout(); let mut stderr = std::io::stderr(); - match crate::apply_patch(&patch_arg, &mut stdout, &mut stderr) { + let cwd = match codex_utils_absolute_path::AbsolutePathBuf::current_dir() { + Ok(cwd) => cwd, + Err(err) => { + eprintln!("Error: Failed to determine current directory.\n{err}"); + return 1; + } + }; + let runtime = match tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + { + Ok(runtime) => runtime, + Err(err) => { + eprintln!("Error: Failed to initialize runtime.\n{err}"); + return 1; + } + }; + match runtime.block_on(crate::apply_patch( + &patch_arg, + &cwd, + &mut stdout, + &mut stderr, + codex_exec_server::LOCAL_FS.as_ref(), + )) { Ok(()) => { // Flush to ensure output ordering when used in pipelines. let _ = stdout.flush(); diff --git a/codex-rs/apply-patch/tests/suite/tool.rs b/codex-rs/apply-patch/tests/suite/tool.rs index 56cd6e57a..8499d0fb9 100644 --- a/codex-rs/apply-patch/tests/suite/tool.rs +++ b/codex-rs/apply-patch/tests/suite/tool.rs @@ -2,6 +2,7 @@ use assert_cmd::Command; use pretty_assertions::assert_eq; use std::fs; use std::path::Path; +use std::path::PathBuf; use tempfile::tempdir; fn run_apply_patch_in_dir(dir: &Path, patch: &str) -> anyhow::Result { @@ -16,9 +17,14 @@ fn apply_patch_command(dir: &Path) -> anyhow::Result { Ok(cmd) } +fn resolved_under(root: &Path, path: &str) -> anyhow::Result { + Ok(root.canonicalize()?.join(path)) +} + #[test] fn test_apply_patch_cli_applies_multiple_operations() -> anyhow::Result<()> { let tmp = tempdir()?; + let add_path = tmp.path().join("nested/new.txt"); let modify_path = tmp.path().join("modify.txt"); let delete_path = tmp.path().join("delete.txt"); @@ -31,10 +37,7 @@ fn test_apply_patch_cli_applies_multiple_operations() -> anyhow::Result<()> { "Success. Updated the following files:\nA nested/new.txt\nM modify.txt\nD delete.txt\n", ); - assert_eq!( - fs::read_to_string(tmp.path().join("nested/new.txt"))?, - "created\n" - ); + assert_eq!(fs::read_to_string(add_path)?, "created\n"); assert_eq!(fs::read_to_string(&modify_path)?, "line1\nchanged\n"); assert!(!delete_path.exists()); @@ -98,13 +101,17 @@ fn test_apply_patch_cli_rejects_empty_patch() -> anyhow::Result<()> { fn test_apply_patch_cli_reports_missing_context() -> anyhow::Result<()> { let tmp = tempdir()?; let target_path = tmp.path().join("modify.txt"); + let expected_target_path = resolved_under(tmp.path(), "modify.txt")?; fs::write(&target_path, "line1\nline2\n")?; apply_patch_command(tmp.path())? .arg("*** Begin Patch\n*** Update File: modify.txt\n@@\n-missing\n+changed\n*** End Patch") .assert() .failure() - .stderr("Failed to find expected lines in modify.txt:\nmissing\n"); + .stderr(format!( + "Failed to find expected lines in {}:\nmissing\n", + expected_target_path.display() + )); assert_eq!(fs::read_to_string(&target_path)?, "line1\nline2\n"); Ok(()) @@ -113,12 +120,16 @@ fn test_apply_patch_cli_reports_missing_context() -> anyhow::Result<()> { #[test] fn test_apply_patch_cli_rejects_missing_file_delete() -> anyhow::Result<()> { let tmp = tempdir()?; + let missing_path = resolved_under(tmp.path(), "missing.txt")?; apply_patch_command(tmp.path())? .arg("*** Begin Patch\n*** Delete File: missing.txt\n*** End Patch") .assert() .failure() - .stderr("Failed to delete file missing.txt\n"); + .stderr(format!( + "Failed to delete file {}\n", + missing_path.display() + )); Ok(()) } @@ -139,14 +150,16 @@ fn test_apply_patch_cli_rejects_empty_update_hunk() -> anyhow::Result<()> { #[test] fn test_apply_patch_cli_requires_existing_file_for_update() -> anyhow::Result<()> { let tmp = tempdir()?; + let missing_path = resolved_under(tmp.path(), "missing.txt")?; apply_patch_command(tmp.path())? .arg("*** Begin Patch\n*** Update File: missing.txt\n@@\n-old\n+new\n*** End Patch") .assert() .failure() - .stderr( - "Failed to read file to update missing.txt: No such file or directory (os error 2)\n", - ); + .stderr(format!( + "Failed to read file to update {}: No such file or directory (os error 2)\n", + missing_path.display() + )); Ok(()) } @@ -195,13 +208,18 @@ fn test_apply_patch_cli_add_overwrites_existing_file() -> anyhow::Result<()> { #[test] fn test_apply_patch_cli_delete_directory_fails() -> anyhow::Result<()> { let tmp = tempdir()?; - fs::create_dir(tmp.path().join("dir"))?; + let dir = tmp.path().join("dir"); + let expected_dir = resolved_under(tmp.path(), "dir")?; + fs::create_dir(&dir)?; apply_patch_command(tmp.path())? .arg("*** Begin Patch\n*** Delete File: dir\n*** End Patch") .assert() .failure() - .stderr("Failed to delete file dir\n"); + .stderr(format!( + "Failed to delete file {}\n", + expected_dir.display() + )); Ok(()) } @@ -243,13 +261,17 @@ fn test_apply_patch_cli_updates_file_appends_trailing_newline() -> anyhow::Resul fn test_apply_patch_cli_failure_after_partial_success_leaves_changes() -> anyhow::Result<()> { let tmp = tempdir()?; let new_file = tmp.path().join("created.txt"); + let missing_file = resolved_under(tmp.path(), "missing.txt")?; apply_patch_command(tmp.path())? .arg("*** Begin Patch\n*** Add File: created.txt\n+hello\n*** Update File: missing.txt\n@@\n-old\n+new\n*** End Patch") .assert() .failure() .stdout("") - .stderr("Failed to read file to update missing.txt: No such file or directory (os error 2)\n"); + .stderr(format!( + "Failed to read file to update {}: No such file or directory (os error 2)\n", + missing_file.display() + )); assert_eq!(fs::read_to_string(&new_file)?, "hello\n"); diff --git a/codex-rs/arg0/Cargo.toml b/codex-rs/arg0/Cargo.toml index cd409fedd..8da0fcbd0 100644 --- a/codex-rs/arg0/Cargo.toml +++ b/codex-rs/arg0/Cargo.toml @@ -14,9 +14,11 @@ workspace = true [dependencies] anyhow = { workspace = true } codex-apply-patch = { workspace = true } +codex-exec-server = { workspace = true } codex-linux-sandbox = { workspace = true } codex-sandboxing = { workspace = true } codex-shell-escalation = { workspace = true } +codex-utils-absolute-path = { workspace = true } codex-utils-home-dir = { workspace = true } dotenvy = { workspace = true } tempfile = { workspace = true } diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index efad6c248..f8b61796b 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -99,7 +99,24 @@ pub fn arg0_dispatch() -> Option { Some(patch_arg) => { let mut stdout = std::io::stdout(); let mut stderr = std::io::stderr(); - match codex_apply_patch::apply_patch(&patch_arg, &mut stdout, &mut stderr) { + let cwd = match codex_utils_absolute_path::AbsolutePathBuf::current_dir() { + Ok(cwd) => cwd, + Err(_) => std::process::exit(1), + }; + let runtime = match tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + { + Ok(runtime) => runtime, + Err(_) => std::process::exit(1), + }; + match runtime.block_on(codex_apply_patch::apply_patch( + &patch_arg, + &cwd, + &mut stdout, + &mut stderr, + codex_exec_server::LOCAL_FS.as_ref(), + )) { Ok(()) => 0, Err(_) => 1, } diff --git a/codex-rs/core/src/apply_patch_tests.rs b/codex-rs/core/src/apply_patch_tests.rs index 1b9e722d5..c0190c370 100644 --- a/codex-rs/core/src/apply_patch_tests.rs +++ b/codex-rs/core/src/apply_patch_tests.rs @@ -1,4 +1,5 @@ use super::*; +use core_test_support::PathBufExt; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -6,14 +7,14 @@ use tempfile::tempdir; #[test] fn convert_apply_patch_maps_add_variant() { let tmp = tempdir().expect("tmp"); - let p = tmp.path().join("a.txt"); + let p = tmp.path().join("a.txt").abs(); // Create an action with a single Add change let action = ApplyPatchAction::new_add_for_test(&p, "hello".to_string()); let got = convert_apply_patch_to_protocol(&action); assert_eq!( - got.get(&p), + got.get(p.as_path()), Some(&FileChange::Add { content: "hello".to_string() }) diff --git a/codex-rs/core/src/safety_tests.rs b/codex-rs/core/src/safety_tests.rs index 6e5795e59..9ea59efd3 100644 --- a/codex-rs/core/src/safety_tests.rs +++ b/codex-rs/core/src/safety_tests.rs @@ -5,6 +5,7 @@ use codex_protocol::protocol::FileSystemSandboxEntry; use codex_protocol::protocol::FileSystemSpecialPath; use codex_protocol::protocol::GranularApprovalConfig; use codex_utils_absolute_path::AbsolutePathBuf; +use core_test_support::PathBufExt; use pretty_assertions::assert_eq; use tempfile::TempDir; @@ -17,7 +18,10 @@ fn test_writable_roots_constraint() { let parent = cwd.parent().unwrap().to_path_buf(); // Helper to build a single‑entry patch that adds a file at `p`. - let make_add_change = |p: PathBuf| ApplyPatchAction::new_add_for_test(&p, "".to_string()); + let make_add_change = |p: PathBuf| { + let p = p.abs(); + ApplyPatchAction::new_add_for_test(&p, "".to_string()) + }; let add_inside = make_add_change(cwd.join("inner.txt")); let add_outside = make_add_change(parent.join("outside.txt")); @@ -64,7 +68,8 @@ fn test_writable_roots_constraint() { fn external_sandbox_auto_approves_in_on_request() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); - let add_inside = ApplyPatchAction::new_add_for_test(&cwd.join("inner.txt"), "".to_string()); + let add_inside_path = cwd.join("inner.txt").abs(); + let add_inside = ApplyPatchAction::new_add_for_test(&add_inside_path, "".to_string()); let policy = SandboxPolicy::ExternalSandbox { network_access: codex_protocol::protocol::NetworkAccess::Enabled, @@ -91,8 +96,8 @@ fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); let parent = cwd.parent().unwrap().to_path_buf(); - let add_outside = - ApplyPatchAction::new_add_for_test(&parent.join("outside.txt"), "".to_string()); + let outside_path = parent.join("outside.txt").abs(); + let add_outside = ApplyPatchAction::new_add_for_test(&outside_path, "".to_string()); let policy_workspace_only = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: Default::default(), @@ -136,8 +141,8 @@ fn granular_sandbox_approval_false_rejects_out_of_root_patch() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); let parent = cwd.parent().unwrap().to_path_buf(); - let add_outside = - ApplyPatchAction::new_add_for_test(&parent.join("outside.txt"), "".to_string()); + let outside_path = parent.join("outside.txt").abs(); + let add_outside = ApplyPatchAction::new_add_for_test(&outside_path, "".to_string()); let policy_workspace_only = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: Default::default(), @@ -171,7 +176,8 @@ fn granular_sandbox_approval_false_rejects_out_of_root_patch() { fn read_only_policy_rejects_patch_with_read_only_reason() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); - let action = ApplyPatchAction::new_add_for_test(&cwd.join("inside.txt"), "".to_string()); + let inside_path = cwd.join("inside.txt").abs(); + let action = ApplyPatchAction::new_add_for_test(&inside_path, "".to_string()); let sandbox_policy = SandboxPolicy::new_read_only_policy(); let file_system_sandbox_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, &cwd); @@ -200,8 +206,8 @@ fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); let blocked_path = cwd.join("blocked.txt"); - let blocked_absolute = AbsolutePathBuf::from_absolute_path(blocked_path.clone()).unwrap(); - let action = ApplyPatchAction::new_add_for_test(&blocked_path, "".to_string()); + let blocked_absolute = blocked_path.abs(); + let action = ApplyPatchAction::new_add_for_test(&blocked_absolute, "".to_string()); let sandbox_policy = SandboxPolicy::ExternalSandbox { network_access: codex_protocol::protocol::NetworkAccess::Restricted, }; @@ -243,8 +249,9 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); let blocked_path = cwd.join("docs").join("blocked.txt"); + let blocked_absolute = blocked_path.abs(); let docs_absolute = AbsolutePathBuf::resolve_path_against_base("docs", &cwd); - let action = ApplyPatchAction::new_add_for_test(&blocked_path, "".to_string()); + let action = ApplyPatchAction::new_add_for_test(&blocked_absolute, "".to_string()); let sandbox_policy = SandboxPolicy::ExternalSandbox { network_access: codex_protocol::protocol::NetworkAccess::Restricted, }; @@ -285,8 +292,8 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { fn missing_project_dot_codex_config_requires_approval() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); - let action = - ApplyPatchAction::new_add_for_test(&cwd.join(".codex").join("config.toml"), "".to_string()); + let config_path = cwd.join(".codex").join("config.toml").abs(); + let action = ApplyPatchAction::new_add_for_test(&config_path, "".to_string()); let sandbox_policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: Default::default(), diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index a972566b9..0be920feb 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -23,6 +23,7 @@ use crate::tools::runtimes::apply_patch::ApplyPatchRuntime; use crate::tools::sandboxing::ToolCtx; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; +use codex_exec_server::ExecutorFileSystem; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; @@ -37,7 +38,7 @@ pub struct ApplyPatchHandler; fn file_paths_for_action(action: &ApplyPatchAction) -> Vec { let mut keys = Vec::new(); - let cwd = action.cwd.as_path(); + let cwd = &action.cwd; for (path, change) in action.changes() { if let Some(key) = to_abs_path(cwd, path) { @@ -55,14 +56,14 @@ fn file_paths_for_action(action: &ApplyPatchAction) -> Vec { keys } -fn to_abs_path(cwd: &Path, path: &Path) -> Option { +fn to_abs_path(cwd: &AbsolutePathBuf, path: &Path) -> Option { Some(AbsolutePathBuf::resolve_path_against_base(path, cwd)) } fn write_permissions_for_paths( file_paths: &[AbsolutePathBuf], file_system_sandbox_policy: &codex_protocol::permissions::FileSystemSandboxPolicy, - cwd: &Path, + cwd: &AbsolutePathBuf, ) -> Option { let write_paths = file_paths .iter() @@ -71,7 +72,9 @@ fn write_permissions_for_paths( .unwrap_or_else(|| path.clone()) .into_path_buf() }) - .filter(|path| !file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd)) + .filter(|path| { + !file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd.as_path()) + }) .collect::>() .into_iter() .map(AbsolutePathBuf::from_absolute_path) @@ -110,7 +113,7 @@ async fn effective_patch_permissions( let effective_additional_permissions = apply_granted_turn_permissions( session, crate::sandboxing::SandboxPermissions::UseDefault, - write_permissions_for_paths(&file_paths, &file_system_sandbox_policy, turn.cwd.as_path()), + write_permissions_for_paths(&file_paths, &file_system_sandbox_policy, &turn.cwd), ) .await; @@ -167,7 +170,14 @@ impl ToolHandler for ApplyPatchHandler { // Avoid building temporary ExecParams/command vectors; derive directly from inputs. let cwd = turn.cwd.clone(); let command = vec!["apply_patch".to_string(), patch_input.clone()]; - match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd) { + let Some(environment) = turn.environment.as_ref() else { + return Err(FunctionCallError::RespondToModel( + "apply_patch is unavailable in this session".to_string(), + )); + }; + let fs = environment.get_filesystem(); + match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd, fs.as_ref()).await + { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { let (file_paths, effective_additional_permissions, file_system_sandbox_policy) = effective_patch_permissions(session.as_ref(), turn.as_ref(), &changes).await; @@ -254,7 +264,8 @@ impl ToolHandler for ApplyPatchHandler { #[allow(clippy::too_many_arguments)] pub(crate) async fn intercept_apply_patch( command: &[String], - cwd: &Path, + cwd: &AbsolutePathBuf, + fs: &dyn ExecutorFileSystem, timeout_ms: Option, session: Arc, turn: Arc, @@ -262,7 +273,7 @@ pub(crate) async fn intercept_apply_patch( call_id: &str, tool_name: &str, ) -> Result, FunctionCallError> { - match codex_apply_patch::maybe_parse_apply_patch_verified(command, cwd) { + match codex_apply_patch::maybe_parse_apply_patch_verified(command, cwd, fs).await { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { session .record_model_warning( diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 86ae58692..afe2e09db 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -1,17 +1,21 @@ use super::*; use codex_apply_patch::MaybeApplyPatchVerified; +use codex_exec_server::LOCAL_FS; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::SandboxPolicy; +use core_test_support::PathBufExt; +use core_test_support::PathExt; use pretty_assertions::assert_eq; use tempfile::TempDir; -#[test] -fn approval_keys_include_move_destination() { +#[tokio::test] +async fn approval_keys_include_move_destination() { let tmp = TempDir::new().expect("tmp"); - let cwd = tmp.path(); - std::fs::create_dir_all(cwd.join("old")).expect("create old dir"); - std::fs::create_dir_all(cwd.join("renamed/dir")).expect("create dest dir"); - std::fs::write(cwd.join("old/name.txt"), "old content\n").expect("write old file"); + let cwd_path = tmp.path(); + let cwd = cwd_path.abs(); + std::fs::create_dir_all(cwd_path.join("old")).expect("create old dir"); + std::fs::create_dir_all(cwd_path.join("renamed/dir")).expect("create dest dir"); + std::fs::write(cwd_path.join("old/name.txt"), "old content\n").expect("write old file"); let patch = r#"*** Begin Patch *** Update File: old/name.txt *** Move to: renamed/dir/name.txt @@ -20,10 +24,13 @@ fn approval_keys_include_move_destination() { +new content *** End Patch"#; let argv = vec!["apply_patch".to_string(), patch.to_string()]; - let action = match codex_apply_patch::maybe_parse_apply_patch_verified(&argv, cwd) { - MaybeApplyPatchVerified::Body(action) => action, - other => panic!("expected patch body, got: {other:?}"), - }; + let action = + match codex_apply_patch::maybe_parse_apply_patch_verified(&argv, &cwd, LOCAL_FS.as_ref()) + .await + { + MaybeApplyPatchVerified::Body(action) => action, + other => panic!("expected patch body, got: {other:?}"), + }; let keys = file_paths_for_action(&action); assert_eq!(keys.len(), 2); @@ -32,8 +39,9 @@ fn approval_keys_include_move_destination() { #[test] fn write_permissions_for_paths_skip_dirs_already_writable_under_workspace_root() { let tmp = TempDir::new().expect("tmp"); - let cwd = tmp.path(); - let nested = cwd.join("nested"); + let cwd_path = tmp.path(); + let cwd = cwd_path.abs(); + let nested = cwd_path.join("nested"); std::fs::create_dir_all(&nested).expect("create nested dir"); let file_path = AbsolutePathBuf::try_from(nested.join("file.txt")) .expect("nested file path should be absolute"); @@ -45,7 +53,7 @@ fn write_permissions_for_paths_skip_dirs_already_writable_under_workspace_root() exclude_slash_tmp: false, }); - let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, cwd); + let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd); assert_eq!(permissions, None); } @@ -59,6 +67,7 @@ fn write_permissions_for_paths_keep_dirs_outside_workspace_root() { std::fs::create_dir_all(&outside).expect("create outside dir"); let file_path = AbsolutePathBuf::try_from(outside.join("file.txt")) .expect("outside file path should be absolute"); + let cwd_abs = cwd.abs(); let sandbox_policy = FileSystemSandboxPolicy::from(&SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: Default::default(), @@ -67,11 +76,9 @@ fn write_permissions_for_paths_keep_dirs_outside_workspace_root() { exclude_slash_tmp: true, }); - let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd); - let expected_outside = AbsolutePathBuf::from_absolute_path(dunce::simplified( - &outside.canonicalize().expect("canonicalize outside dir"), - )) - .expect("outside dir should be absolute"); + let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd_abs); + let expected_outside = + dunce::simplified(&outside.canonicalize().expect("canonicalize outside dir")).abs(); assert_eq!( permissions.and_then(|profile| profile.file_system.and_then(|fs| fs.write)), diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 6f662ff49..e83556624 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -39,6 +39,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::ExecCommandSource; use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::ShellCommandBackendConfig; +use codex_utils_absolute_path::AbsolutePathBuf; pub struct ShellHandler; @@ -395,6 +396,13 @@ impl ShellHandler { } = args; let mut exec_params = exec_params; + let Some(environment) = turn.environment.as_ref() else { + return Err(FunctionCallError::RespondToModel( + "shell is unavailable in this session".to_string(), + )); + }; + let fs = environment.get_filesystem(); + let dependency_env = session.dependency_env().await; if !dependency_env.is_empty() { exec_params.env.extend(dependency_env.clone()); @@ -458,9 +466,16 @@ impl ShellHandler { } // Intercept apply_patch if present. + let apply_patch_cwd = + AbsolutePathBuf::from_absolute_path(&exec_params.cwd).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "apply_patch verification failed: failed to resolve cwd: {err}" + )) + })?; if let Some(output) = intercept_apply_patch( &exec_params.command, - &exec_params.cwd, + &apply_patch_cwd, + fs.as_ref(), exec_params.expiration.timeout_ms(), session.clone(), turn.clone(), diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 4486cd791..b83068c66 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -30,6 +30,7 @@ use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::TerminalInteractionEvent; use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::UnifiedExecShellMode; +use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use std::path::PathBuf; use std::sync::Arc; @@ -176,6 +177,13 @@ impl ToolHandler for UnifiedExecHandler { } }; + let Some(environment) = turn.environment.as_ref() else { + return Err(FunctionCallError::RespondToModel( + "unified exec is unavailable in this session".to_string(), + )); + }; + let fs = environment.get_filesystem(); + let manager: &UnifiedExecProcessManager = &session.services.unified_exec_manager; let context = UnifiedExecContext::new(session.clone(), turn.clone(), call_id.clone()); @@ -274,9 +282,19 @@ impl ToolHandler for UnifiedExecHandler { } }; + let apply_patch_cwd = match AbsolutePathBuf::from_absolute_path(&cwd) { + Ok(cwd) => cwd, + Err(err) => { + manager.release_process_id(process_id).await; + return Err(FunctionCallError::RespondToModel(format!( + "apply_patch verification failed: failed to resolve cwd: {err}" + ))); + } + }; if let Some(output) = intercept_apply_patch( &command, - &cwd, + &apply_patch_cwd, + fs.as_ref(), Some(yield_time_ms), context.session.clone(), context.turn.clone(), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index a6dc8885a..efb9a8a8f 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -58,7 +58,7 @@ impl ApplyPatchRuntime { ) -> GuardianApprovalRequest { GuardianApprovalRequest::ApplyPatch { id: call_id.to_string(), - cwd: req.action.cwd.clone(), + cwd: req.action.cwd.to_path_buf(), files: req.file_paths.clone(), patch: req.action.patch.clone(), } @@ -101,7 +101,7 @@ impl ApplyPatchRuntime { CODEX_CORE_APPLY_PATCH_ARG1.to_string(), req.action.patch.clone(), ], - cwd: req.action.cwd.clone(), + cwd: req.action.cwd.to_path_buf(), // Run apply_patch with a minimal environment for determinism and to avoid leaks. env: HashMap::new(), additional_permissions: req.additional_permissions.clone(), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs index 3d5ac27cb..e5831414c 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -1,5 +1,6 @@ use super::*; use codex_protocol::protocol::GranularApprovalConfig; +use core_test_support::PathBufExt; use pretty_assertions::assert_eq; use std::collections::HashMap; #[cfg(not(target_os = "windows"))] @@ -31,17 +32,17 @@ fn wants_no_sandbox_approval_granular_respects_sandbox_flag() { #[test] fn guardian_review_request_includes_patch_context() { - let path = std::env::temp_dir().join("guardian-apply-patch-test.txt"); + let path = std::env::temp_dir() + .join("guardian-apply-patch-test.txt") + .abs(); let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); - let expected_cwd = action.cwd.clone(); + let expected_cwd = action.cwd.to_path_buf(); let expected_patch = action.patch.clone(); let request = ApplyPatchRequest { action, - file_paths: vec![ - AbsolutePathBuf::from_absolute_path(&path).expect("temp path should be absolute"), - ], + file_paths: vec![path.clone()], changes: HashMap::from([( - path, + path.to_path_buf(), FileChange::Add { content: "hello".to_string(), }, @@ -71,15 +72,15 @@ fn guardian_review_request_includes_patch_context() { #[cfg(not(target_os = "windows"))] #[test] fn build_sandbox_command_prefers_configured_codex_self_exe_for_apply_patch() { - let path = std::env::temp_dir().join("apply-patch-current-exe-test.txt"); + let path = std::env::temp_dir() + .join("apply-patch-current-exe-test.txt") + .abs(); let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); let request = ApplyPatchRequest { action, - file_paths: vec![ - AbsolutePathBuf::from_absolute_path(&path).expect("temp path should be absolute"), - ], + file_paths: vec![path.clone()], changes: HashMap::from([( - path, + path.to_path_buf(), FileChange::Add { content: "hello".to_string(), }, @@ -103,15 +104,15 @@ fn build_sandbox_command_prefers_configured_codex_self_exe_for_apply_patch() { #[cfg(not(target_os = "windows"))] #[test] fn build_sandbox_command_falls_back_to_current_exe_for_apply_patch() { - let path = std::env::temp_dir().join("apply-patch-current-exe-test.txt"); + let path = std::env::temp_dir() + .join("apply-patch-current-exe-test.txt") + .abs(); let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); let request = ApplyPatchRequest { action, - file_paths: vec![ - AbsolutePathBuf::from_absolute_path(&path).expect("temp path should be absolute"), - ], + file_paths: vec![path.clone()], changes: HashMap::from([( - path, + path.to_path_buf(), FileChange::Add { content: "hello".to_string(), }, diff --git a/codex-rs/core/tests/common/lib.rs b/codex-rs/core/tests/common/lib.rs index 8aff42e96..d16054e6d 100644 --- a/codex-rs/core/tests/common/lib.rs +++ b/codex-rs/core/tests/common/lib.rs @@ -13,8 +13,9 @@ use codex_core::config::Config; use codex_core::config::ConfigBuilder; use codex_core::config::ConfigOverrides; use codex_utils_absolute_path::AbsolutePathBuf; +pub use codex_utils_absolute_path::test_support::PathBufExt; +pub use codex_utils_absolute_path::test_support::PathExt; use regex_lite::Regex; -use std::path::Path; use std::path::PathBuf; pub mod apps_test_server; @@ -105,26 +106,6 @@ pub fn test_absolute_path(unix_path: &str) -> AbsolutePathBuf { test_absolute_path_with_windows(unix_path, /*windows_path*/ None) } -pub trait PathExt { - fn abs(&self) -> AbsolutePathBuf; -} - -impl PathExt for Path { - fn abs(&self) -> AbsolutePathBuf { - AbsolutePathBuf::try_from(self.to_path_buf()).expect("path should already be absolute") - } -} - -pub trait PathBufExt { - fn abs(&self) -> AbsolutePathBuf; -} - -impl PathBufExt for PathBuf { - fn abs(&self) -> AbsolutePathBuf { - self.as_path().abs() - } -} - pub trait TempDirExt { fn abs(&self) -> AbsolutePathBuf; } diff --git a/codex-rs/exec-server/src/file_system.rs b/codex-rs/exec-server/src/file_system.rs index 35c2243f8..b04b48089 100644 --- a/codex-rs/exec-server/src/file_system.rs +++ b/codex-rs/exec-server/src/file_system.rs @@ -39,6 +39,12 @@ pub type FileSystemResult = io::Result; pub trait ExecutorFileSystem: Send + Sync { async fn read_file(&self, path: &AbsolutePathBuf) -> FileSystemResult>; + /// Reads a file and decodes it as UTF-8 text. + async fn read_file_text(&self, path: &AbsolutePathBuf) -> FileSystemResult { + let bytes = self.read_file(path).await?; + String::from_utf8(bytes).map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err)) + } + async fn write_file(&self, path: &AbsolutePathBuf, contents: Vec) -> FileSystemResult<()>; async fn create_directory( diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index dea47e8fc..ca55587f5 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -122,6 +122,12 @@ async fn file_system_methods_cover_surface_area(use_remote: bool) -> Result<()> .with_context(|| format!("mode={use_remote}"))?; assert_eq!(nested_file_contents, b"hello from trait"); + let nested_file_text = file_system + .read_file_text(&absolute_path(nested_file.clone())) + .await + .with_context(|| format!("mode={use_remote}"))?; + assert_eq!(nested_file_text, "hello from trait"); + file_system .copy( &absolute_path(nested_file), diff --git a/codex-rs/tui/src/test_support.rs b/codex-rs/tui/src/test_support.rs index 790fc805e..eb284a599 100644 --- a/codex-rs/tui/src/test_support.rs +++ b/codex-rs/tui/src/test_support.rs @@ -1,27 +1,6 @@ -use codex_utils_absolute_path::AbsolutePathBuf; +pub(crate) use codex_utils_absolute_path::test_support::PathBufExt; +pub(crate) use codex_utils_absolute_path::test_support::PathExt; use std::path::Path; -use std::path::PathBuf; - -pub(crate) trait PathExt { - fn abs(&self) -> AbsolutePathBuf; -} - -impl PathExt for Path { - fn abs(&self) -> AbsolutePathBuf { - AbsolutePathBuf::try_from(self.to_path_buf()) - .unwrap_or_else(|_| panic!("path should already be absolute")) - } -} - -pub(crate) trait PathBufExt { - fn abs(&self) -> AbsolutePathBuf; -} - -impl PathBufExt for PathBuf { - fn abs(&self) -> AbsolutePathBuf { - self.as_path().abs() - } -} pub(crate) fn test_path_display(path: &str) -> String { Path::new(path).abs().display().to_string() diff --git a/codex-rs/utils/absolute-path/src/lib.rs b/codex-rs/utils/absolute-path/src/lib.rs index 2b7ca4c30..a09ce8f0c 100644 --- a/codex-rs/utils/absolute-path/src/lib.rs +++ b/codex-rs/utils/absolute-path/src/lib.rs @@ -126,6 +126,38 @@ impl From for PathBuf { } } +/// Helpers for constructing absolute paths in tests. +pub mod test_support { + use super::AbsolutePathBuf; + use std::path::Path; + use std::path::PathBuf; + + /// Extension methods for converting paths into [`AbsolutePathBuf`] values in tests. + pub trait PathExt { + /// Converts an already absolute path into an [`AbsolutePathBuf`]. + fn abs(&self) -> AbsolutePathBuf; + } + + impl PathExt for Path { + #[expect(clippy::expect_used)] + fn abs(&self) -> AbsolutePathBuf { + AbsolutePathBuf::try_from(self).expect("path should already be absolute") + } + } + + /// Extension methods for converting path buffers into [`AbsolutePathBuf`] values in tests. + pub trait PathBufExt { + /// Converts an already absolute path buffer into an [`AbsolutePathBuf`]. + fn abs(&self) -> AbsolutePathBuf; + } + + impl PathBufExt for PathBuf { + fn abs(&self) -> AbsolutePathBuf { + self.as_path().abs() + } + } +} + impl TryFrom<&Path> for AbsolutePathBuf { type Error = std::io::Error;