diff --git a/codex-rs/apply-patch/src/invocation.rs b/codex-rs/apply-patch/src/invocation.rs index 519671798d..a7109c6ec9 100644 --- a/codex-rs/apply-patch/src/invocation.rs +++ b/codex-rs/apply-patch/src/invocation.rs @@ -1,7 +1,9 @@ use std::collections::HashMap; use std::path::Path; +use std::path::PathBuf; use std::sync::LazyLock; +use similar::TextDiff; use tree_sitter::Parser; use tree_sitter::Query; use tree_sitter::QueryCursor; @@ -12,9 +14,11 @@ use crate::ApplyPatchAction; use crate::ApplyPatchArgs; use crate::ApplyPatchError; use crate::ApplyPatchFileChange; +use crate::ApplyPatchFileSystem; use crate::ApplyPatchFileUpdate; use crate::IoError; use crate::MaybeApplyPatchVerified; +use crate::derive_new_contents_from_chunks_with_fs; use crate::parser::Hunk; use crate::parser::ParseError; use crate::parser::parse_patch; @@ -149,17 +153,7 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp hunks, workdir, }) => { - 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()); + let effective_cwd = resolve_effective_cwd(cwd, workdir.as_deref()); let mut changes = HashMap::new(); for hunk in hunks { let path = hunk.resolve_path(&effective_cwd); @@ -216,6 +210,101 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp } } +/// Async variant of [`maybe_parse_apply_patch_verified`] that reads file +/// contents through an abstract filesystem instead of directly from `std::fs`. +pub async fn maybe_parse_apply_patch_verified_with_fs( + argv: &[String], + cwd: &Path, + fs: &dyn ApplyPatchFileSystem, +) -> MaybeApplyPatchVerified { + if let [body] = argv + && parse_patch(body).is_ok() + { + return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation); + } + if let Some((_, script)) = parse_shell_script(argv) + && parse_patch(script).is_ok() + { + return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation); + } + + match maybe_parse_apply_patch(argv) { + MaybeApplyPatch::Body(ApplyPatchArgs { + patch, + hunks, + workdir, + }) => { + let effective_cwd = resolve_effective_cwd(cwd, workdir.as_deref()); + 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 }); + } + Hunk::DeleteFile { .. } => { + let content = match fs.read_text(&path).await { + Ok(content) => content, + Err(err) => { + return MaybeApplyPatchVerified::CorrectnessError(err); + } + }; + changes.insert(path, ApplyPatchFileChange::Delete { content }); + } + Hunk::UpdateFile { + move_path, chunks, .. + } => { + let applied = + match derive_new_contents_from_chunks_with_fs(&path, &chunks, fs).await + { + Ok(applied) => applied, + Err(err) => { + return MaybeApplyPatchVerified::CorrectnessError(err); + } + }; + let unified_diff = + TextDiff::from_lines(&applied.original_contents, &applied.new_contents) + .unified_diff() + .context_radius(1) + .to_string(); + changes.insert( + path, + ApplyPatchFileChange::Update { + unified_diff, + move_path: move_path.map(|p| effective_cwd.join(p)), + new_content: applied.new_contents, + }, + ); + } + } + } + MaybeApplyPatchVerified::Body(ApplyPatchAction { + changes, + patch, + cwd: effective_cwd, + }) + } + MaybeApplyPatch::ShellParseError(err) => MaybeApplyPatchVerified::ShellParseError(err), + MaybeApplyPatch::PatchParseError(err) => { + MaybeApplyPatchVerified::CorrectnessError(err.into()) + } + MaybeApplyPatch::NotApplyPatch => MaybeApplyPatchVerified::NotApplyPatch, + } +} + +fn resolve_effective_cwd(cwd: &Path, workdir: Option<&str>) -> PathBuf { + workdir + .map(Path::new) + .map(|path| { + if path.is_absolute() { + path.to_path_buf() + } else { + cwd.join(path) + } + }) + .unwrap_or_else(|| cwd.to_path_buf()) +} + /// Extract the heredoc body (and optional `cd` workdir) from a `bash -lc` script /// that invokes the apply_patch tool using a heredoc. /// diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 3041c4b2cd..b854f5fffe 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -4,8 +4,10 @@ mod seek_sequence; mod standalone_executable; use std::collections::HashMap; +use std::future::Future; use std::path::Path; use std::path::PathBuf; +use std::pin::Pin; use anyhow::Context; use anyhow::Result; @@ -18,6 +20,7 @@ use similar::TextDiff; use thiserror::Error; pub use invocation::maybe_parse_apply_patch_verified; +pub use invocation::maybe_parse_apply_patch_verified_with_fs; pub use standalone_executable::main; use crate::invocation::ExtractHeredocError; @@ -50,6 +53,15 @@ pub enum ApplyPatchError { ImplicitInvocation, } +impl ApplyPatchError { + pub fn io_error(context: impl Into, source: std::io::Error) -> Self { + Self::IoError(IoError { + context: context.into(), + source, + }) + } +} + impl From for ApplyPatchError { fn from(err: std::io::Error) -> Self { ApplyPatchError::IoError(IoError { @@ -179,6 +191,33 @@ impl ApplyPatchAction { } } +/// Filesystem operations required to verify and apply a patch. +/// +/// This keeps the patch parser/diff logic independent from the host filesystem +/// so callers can provide sandboxed or remote-backed implementations. +pub trait ApplyPatchFileSystem: Send + Sync { + fn read_text<'a>( + &'a self, + path: &'a Path, + ) -> Pin> + Send + 'a>>; + + fn write_text<'a>( + &'a self, + path: &'a Path, + contents: String, + ) -> Pin> + Send + 'a>>; + + fn create_dir_all<'a>( + &'a self, + path: &'a Path, + ) -> Pin> + Send + 'a>>; + + fn remove_file<'a>( + &'a self, + path: &'a Path, + ) -> Pin> + Send + 'a>>; +} + /// Applies the patch and prints the result to stdout/stderr. pub fn apply_patch( patch: &str, @@ -359,6 +398,14 @@ fn derive_new_contents_from_chunks( } }; + derive_new_contents_from_text(&original_contents, path, chunks) +} + +pub(crate) fn derive_new_contents_from_text( + original_contents: &str, + path: &Path, + chunks: &[UpdateFileChunk], +) -> std::result::Result { let mut original_lines: Vec = original_contents.split('\n').map(String::from).collect(); // Drop the trailing empty element that results from the final newline so @@ -375,11 +422,88 @@ fn derive_new_contents_from_chunks( } let new_contents = new_lines.join("\n"); Ok(AppliedPatch { - original_contents, + original_contents: original_contents.to_string(), new_contents, }) } +pub(crate) async fn derive_new_contents_from_chunks_with_fs( + path: &Path, + chunks: &[UpdateFileChunk], + fs: &dyn ApplyPatchFileSystem, +) -> std::result::Result { + let original_contents = match fs.read_text(path).await { + Ok(contents) => contents, + Err(ApplyPatchError::IoError(IoError { source, .. })) => { + return Err(ApplyPatchError::IoError(IoError { + context: format!("Failed to read file to update {}", path.display()), + source, + })); + } + Err(err) => return Err(err), + }; + + derive_new_contents_from_text(&original_contents, path, chunks) +} + +/// Apply a verified patch action through an abstract filesystem. +pub async fn apply_action_with_fs( + action: &ApplyPatchAction, + fs: &dyn ApplyPatchFileSystem, +) -> std::result::Result { + if action.is_empty() { + return Err(ApplyPatchError::IoError(IoError { + context: "No files were modified.".to_string(), + source: std::io::Error::other("empty patch"), + })); + } + + let mut added = Vec::new(); + let mut modified = Vec::new(); + let mut deleted = Vec::new(); + for (path, change) in action.changes() { + match change { + ApplyPatchFileChange::Add { content } => { + if let Some(parent) = path.parent() + && !parent.as_os_str().is_empty() + { + fs.create_dir_all(parent).await?; + } + fs.write_text(path, content.clone()).await?; + added.push(path.clone()); + } + ApplyPatchFileChange::Delete { .. } => { + fs.remove_file(path).await?; + deleted.push(path.clone()); + } + ApplyPatchFileChange::Update { + new_content, + move_path, + .. + } => { + if let Some(dest) = move_path { + if let Some(parent) = dest.parent() + && !parent.as_os_str().is_empty() + { + fs.create_dir_all(parent).await?; + } + fs.write_text(dest, new_content.clone()).await?; + fs.remove_file(path).await?; + modified.push(dest.clone()); + } else { + fs.write_text(path, new_content.clone()).await?; + modified.push(path.clone()); + } + } + } + } + Ok(AffectedPaths { + added, + modified, + deleted, + }) +} + /// Compute a list of replacements needed to transform `original_lines` into the /// new lines, given the patch `chunks`. Each replacement is returned as /// `(start_index, old_len, new_lines)`. @@ -556,9 +680,120 @@ mod tests { use super::*; use pretty_assertions::assert_eq; use std::fs; + use std::future::Future; + use std::pin::Pin; use std::string::ToString; + use std::sync::Arc; + use std::task::Context; + use std::task::Poll; + use std::task::Wake; + use std::task::Waker; use tempfile::tempdir; + #[derive(Default)] + struct NoopWake; + + impl Wake for NoopWake { + fn wake(self: Arc) {} + } + + fn block_on(future: F) -> F::Output { + let waker = Waker::from(Arc::new(NoopWake)); + let mut context = Context::from_waker(&waker); + let mut future = Pin::from(Box::new(future)); + loop { + match future.as_mut().poll(&mut context) { + Poll::Ready(output) => return output, + Poll::Pending => std::thread::yield_now(), + } + } + } + + #[derive(Clone, Default)] + struct TestApplyPatchFileSystem { + files: std::sync::Arc>>, + removed: std::sync::Arc>>, + directories: std::sync::Arc>>, + } + + impl TestApplyPatchFileSystem { + fn with_file(path: &Path, content: &str) -> Self { + let file_system = Self::default(); + file_system + .files + .lock() + .expect("lock files") + .insert(path.to_path_buf(), content.to_string()); + file_system + } + } + + impl ApplyPatchFileSystem for TestApplyPatchFileSystem { + fn read_text<'a>( + &'a self, + path: &'a Path, + ) -> Pin> + Send + 'a>> + { + Box::pin(async move { + self.files + .lock() + .expect("lock files") + .get(path) + .cloned() + .ok_or_else(|| { + ApplyPatchError::io_error( + format!("missing test file {}", path.display()), + std::io::Error::new(std::io::ErrorKind::NotFound, "missing"), + ) + }) + }) + } + + fn write_text<'a>( + &'a self, + path: &'a Path, + contents: String, + ) -> Pin> + Send + 'a>> + { + Box::pin(async move { + self.files + .lock() + .expect("lock files") + .insert(path.to_path_buf(), contents); + Ok(()) + }) + } + + fn create_dir_all<'a>( + &'a self, + path: &'a Path, + ) -> Pin> + Send + 'a>> + { + Box::pin(async move { + self.directories + .lock() + .expect("lock dirs") + .push(path.to_path_buf()); + Ok(()) + }) + } + + fn remove_file<'a>( + &'a self, + path: &'a Path, + ) -> Pin> + Send + 'a>> + { + Box::pin(async move { + self.files.lock().expect("lock files").remove(path); + self.removed + .lock() + .expect("lock removed") + .push(path.to_path_buf()); + Ok(()) + }) + } + } + /// Helper to construct a patch with the given body. fn wrap_patch(body: &str) -> String { format!("*** Begin Patch\n{body}\n*** End Patch") @@ -1071,4 +1306,83 @@ g let result = apply_patch(&patch, &mut stdout, &mut stderr); assert!(result.is_err()); } + + #[test] + fn apply_action_with_fs_updates_file_without_touching_host_fs() { + let path = PathBuf::from("/virtual/update.txt"); + let patch = wrap_patch(&format!( + r#"*** Update File: {} +@@ +-before ++after"#, + path.display() + )); + let argv = vec!["apply_patch".to_string(), patch]; + let fs = TestApplyPatchFileSystem::with_file(&path, "before\n"); + + let action = match block_on(maybe_parse_apply_patch_verified_with_fs( + &argv, + Path::new("/"), + &fs, + )) { + MaybeApplyPatchVerified::Body(action) => action, + other => panic!("expected patch body, got {other:?}"), + }; + let affected = block_on(apply_action_with_fs(&action, &fs)).expect("apply action"); + + assert_eq!(affected.modified, vec![path.clone()]); + assert_eq!( + fs.files.lock().expect("lock files").get(&path).cloned(), + Some("after\n".to_string()) + ); + } + + #[test] + fn maybe_parse_apply_patch_verified_with_fs_uses_abstract_fs_for_update_and_delete() { + let update_path = PathBuf::from("/virtual/update.txt"); + let delete_path = PathBuf::from("/virtual/delete.txt"); + let patch = wrap_patch(&format!( + r#"*** Update File: {} +@@ +-before ++after +*** Delete File: {}"#, + update_path.display(), + delete_path.display() + )); + let argv = vec!["apply_patch".to_string(), patch]; + let fs = TestApplyPatchFileSystem::default(); + fs.files + .lock() + .expect("lock files") + .insert(update_path.clone(), "before\n".to_string()); + fs.files + .lock() + .expect("lock files") + .insert(delete_path.clone(), "gone\n".to_string()); + + let action = match block_on(maybe_parse_apply_patch_verified_with_fs( + &argv, + Path::new("/"), + &fs, + )) { + MaybeApplyPatchVerified::Body(action) => action, + other => panic!("expected patch body, got {other:?}"), + }; + + assert_eq!( + action.changes().get(&delete_path), + Some(&ApplyPatchFileChange::Delete { + content: "gone\n".to_string() + }) + ); + assert_eq!( + action.changes().get(&update_path), + Some(&ApplyPatchFileChange::Update { + unified_diff: "@@ -1 +1 @@\n-before\n+after\n".to_string(), + move_path: None, + new_content: "after\n".to_string(), + }) + ); + } } diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 0810d319b8..0cd946fc59 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -4,11 +4,21 @@ use crate::safety::SafetyCheck; use crate::safety::assess_patch_safety; use crate::tools::sandboxing::ExecApprovalRequirement; use codex_apply_patch::ApplyPatchAction; +use codex_apply_patch::ApplyPatchError; use codex_apply_patch::ApplyPatchFileChange; +use codex_apply_patch::ApplyPatchFileSystem; +use codex_exec_server::CreateDirectoryOptions; +use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::RemoveOptions; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::FileSystemSandboxPolicy; +use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; +use std::future::Future; +use std::io; use std::path::PathBuf; +use std::pin::Pin; +use std::sync::Arc; pub(crate) enum InternalApplyPatchInvocation { /// The `apply_patch` call was handled programmatically, without any sort @@ -33,6 +43,107 @@ pub(crate) struct ApplyPatchExec { pub(crate) exec_approval_requirement: ExecApprovalRequirement, } +pub(crate) struct EnvironmentApplyPatchFileSystem { + file_system: Arc, +} + +impl EnvironmentApplyPatchFileSystem { + pub(crate) fn new(file_system: Arc) -> Self { + Self { file_system } + } +} + +impl ApplyPatchFileSystem for EnvironmentApplyPatchFileSystem { + fn read_text<'a>( + &'a self, + path: &'a std::path::Path, + ) -> Pin> + Send + 'a>> + { + Box::pin(async move { + let path = absolute_path(path)?; + let bytes = self.file_system.read_file(&path).await.map_err(|source| { + ApplyPatchError::io_error(format!("Failed to read {}", path.display()), source) + })?; + String::from_utf8(bytes).map_err(|source| { + ApplyPatchError::io_error( + format!("Failed to decode UTF-8 for {}", path.display()), + io::Error::new(io::ErrorKind::InvalidData, source.to_string()), + ) + }) + }) + } + + fn write_text<'a>( + &'a self, + path: &'a std::path::Path, + contents: String, + ) -> Pin> + Send + 'a>> { + Box::pin(async move { + let path = absolute_path(path)?; + self.file_system + .write_file(&path, contents.into_bytes()) + .await + .map_err(|source| { + ApplyPatchError::io_error( + format!("Failed to write file {}", path.display()), + source, + ) + }) + }) + } + + fn create_dir_all<'a>( + &'a self, + path: &'a std::path::Path, + ) -> Pin> + Send + 'a>> { + Box::pin(async move { + let path = absolute_path(path)?; + self.file_system + .create_directory(&path, CreateDirectoryOptions { recursive: true }) + .await + .map_err(|source| { + ApplyPatchError::io_error( + format!("Failed to create parent directories for {}", path.display()), + source, + ) + }) + }) + } + + fn remove_file<'a>( + &'a self, + path: &'a std::path::Path, + ) -> Pin> + Send + 'a>> { + Box::pin(async move { + let path = absolute_path(path)?; + self.file_system + .remove( + &path, + RemoveOptions { + recursive: false, + force: false, + }, + ) + .await + .map_err(|source| { + ApplyPatchError::io_error( + format!("Failed to delete file {}", path.display()), + source, + ) + }) + }) + } +} + +fn absolute_path(path: &std::path::Path) -> std::result::Result { + AbsolutePathBuf::from_absolute_path(path).map_err(|error| { + ApplyPatchError::io_error( + format!("Expected absolute path for apply_patch: {}", path.display()), + io::Error::new(io::ErrorKind::InvalidInput, error.to_string()), + ) + }) +} + pub(crate) async fn apply_patch( turn_context: &TurnContext, file_system_sandbox_policy: &FileSystemSandboxPolicy, diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 00b1173aa4..beb2794e6f 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -1,6 +1,7 @@ use std::path::Path; use crate::apply_patch; +use crate::apply_patch::EnvironmentApplyPatchFileSystem; use crate::apply_patch::InternalApplyPatchInvocation; use crate::apply_patch::convert_apply_patch_to_protocol; use crate::codex::Session; @@ -167,7 +168,7 @@ 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) { + match parse_apply_patch_verified(turn.as_ref(), &command, &cwd).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; @@ -262,7 +263,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 parse_apply_patch_verified(turn.as_ref(), command, cwd).await { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { session .record_model_warning( @@ -346,6 +347,15 @@ pub(crate) async fn intercept_apply_patch( } } +async fn parse_apply_patch_verified( + turn: &TurnContext, + command: &[String], + cwd: &Path, +) -> codex_apply_patch::MaybeApplyPatchVerified { + let fs = EnvironmentApplyPatchFileSystem::new(turn.environment.get_filesystem()); + codex_apply_patch::maybe_parse_apply_patch_verified_with_fs(command, cwd, &fs).await +} + #[cfg(test)] #[path = "apply_patch_tests.rs"] mod tests; diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index a6dc8885a4..8e85e8694d 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -4,6 +4,7 @@ //! decision to avoid re-prompting, builds the self-invocation command for //! `codex --codex-run-as-apply-patch`, and runs under the current //! `SandboxAttempt` with a minimal environment. +use crate::apply_patch::EnvironmentApplyPatchFileSystem; use crate::exec::ExecCapturePolicy; use crate::guardian::GuardianApprovalRequest; use crate::guardian::review_approval_request; @@ -22,6 +23,7 @@ use crate::tools::sandboxing::with_cached_approval; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::CODEX_CORE_APPLY_PATCH_ARG1; use codex_protocol::exec_output::ExecToolCallOutput; +use codex_protocol::exec_output::StreamOutput; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::FileChange; @@ -32,6 +34,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; use futures::future::BoxFuture; use std::collections::HashMap; use std::path::PathBuf; +use std::time::Duration; #[derive(Debug)] pub struct ApplyPatchRequest { @@ -115,6 +118,29 @@ impl ApplyPatchRuntime { tx_event: ctx.session.get_tx_event(), }) } + + async fn run_with_environment_fs( + req: &ApplyPatchRequest, + fs: EnvironmentApplyPatchFileSystem, + ) -> Result { + let affected = codex_apply_patch::apply_action_with_fs(&req.action, &fs) + .await + .map_err(|err| ToolError::Rejected(err.to_string()))?; + let mut stdout = Vec::new(); + codex_apply_patch::print_summary(&affected, &mut stdout) + .map_err(|err| ToolError::Rejected(err.to_string()))?; + let stdout = String::from_utf8(stdout).map_err(|err| { + ToolError::Rejected(format!("apply_patch wrote non-UTF-8 output: {err}")) + })?; + Ok(ExecToolCallOutput { + exit_code: 0, + stdout: StreamOutput::new(stdout.clone()), + stderr: StreamOutput::new(String::new()), + aggregated_output: StreamOutput::new(stdout), + duration: Duration::ZERO, + timed_out: false, + }) + } } impl Sandboxable for ApplyPatchRuntime { @@ -211,6 +237,11 @@ impl ToolRuntime for ApplyPatchRuntime { attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, ) -> Result { + if ctx.turn.environment.exec_server_url().is_some() { + let fs = EnvironmentApplyPatchFileSystem::new(ctx.turn.environment.get_filesystem()); + return Self::run_with_environment_fs(req, fs).await; + } + #[cfg(target_os = "windows")] let command = Self::build_sandbox_command(req, &ctx.turn.config.codex_home)?; #[cfg(not(target_os = "windows"))]