diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index c3ed4aa02fa0..99a63e3ace5e 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -192,13 +192,33 @@ impl AppliedPatchDelta { Self { changes, exact } } + fn empty() -> Self { + Self::new(Vec::new(), /*exact*/ true) + } + pub fn changes(&self) -> &[AppliedPatchChange] { &self.changes } + pub fn is_empty(&self) -> bool { + self.changes.is_empty() + } + pub fn is_exact(&self) -> bool { self.exact } + + /// Appends a later committed prefix while preserving the aggregate exactness. + pub fn append(&mut self, other: Self) { + self.changes.extend(other.changes); + self.exact &= other.exact; + } +} + +impl Default for AppliedPatchDelta { + fn default() -> Self { + Self::empty() + } } /// A committed file change, preserved in the order it was applied. @@ -225,6 +245,34 @@ pub enum AppliedPatchFileChange { }, } +/// A failed patch application together with the textual mutations that were +/// definitely committed before the failure was observed. +#[derive(Debug, Error)] +#[error("{error}")] +pub struct ApplyPatchFailure { + #[source] + error: ApplyPatchError, + delta: AppliedPatchDelta, +} + +impl ApplyPatchFailure { + fn new(error: ApplyPatchError, delta: AppliedPatchDelta) -> Self { + Self { error, delta } + } + + fn without_delta(error: ApplyPatchError) -> Self { + Self::new(error, AppliedPatchDelta::empty()) + } + + pub fn delta(&self) -> &AppliedPatchDelta { + &self.delta + } + + pub fn into_parts(self) -> (ApplyPatchError, AppliedPatchDelta) { + (self.error, self.delta) + } +} + /// Applies the patch and prints the result to stdout/stderr. pub async fn apply_patch( patch: &str, @@ -233,13 +281,15 @@ pub async fn apply_patch( stderr: &mut impl std::io::Write, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, -) -> Result { +) -> Result { let hunks = match parse_patch(patch) { Ok(source) => source.hunks, Err(e) => { match &e { InvalidPatchError(message) => { - writeln!(stderr, "Invalid patch: {message}").map_err(ApplyPatchError::from)?; + writeln!(stderr, "Invalid patch: {message}") + .map_err(ApplyPatchError::from) + .map_err(ApplyPatchFailure::without_delta)?; } InvalidHunkError { message, @@ -249,10 +299,13 @@ pub async fn apply_patch( stderr, "Invalid patch hunk on line {line_number}: {message}" ) - .map_err(ApplyPatchError::from)?; + .map_err(ApplyPatchError::from) + .map_err(ApplyPatchFailure::without_delta)?; } } - return Err(ApplyPatchError::ParseError(e)); + return Err(ApplyPatchFailure::without_delta( + ApplyPatchError::ParseError(e), + )); } }; @@ -267,24 +320,29 @@ pub async fn apply_hunks( stderr: &mut impl std::io::Write, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, -) -> Result { - // Delegate to a helper that applies each hunk to the filesystem. - match apply_hunks_to_files(hunks, cwd, fs, sandbox).await { - Ok(applied) => { - print_summary(&applied.affected_paths, stdout).map_err(ApplyPatchError::from)?; - Ok(applied.delta) +) -> Result { + let mut delta = AppliedPatchDelta::empty(); + match apply_hunks_to_files(hunks, cwd, fs, sandbox, &mut delta).await { + Ok(affected_paths) => { + print_summary(&affected_paths, stdout).map_err(|error| { + ApplyPatchFailure::new(ApplyPatchError::from(error), delta.clone()) + })?; + Ok(delta) } - Err(err) => { - let msg = err.to_string(); - writeln!(stderr, "{msg}").map_err(ApplyPatchError::from)?; - if let Some(io) = err.downcast_ref::() { - Err(ApplyPatchError::from(io)) + Err(error) => { + let msg = error.to_string(); + writeln!(stderr, "{msg}").map_err(|error| { + ApplyPatchFailure::new(ApplyPatchError::from(error), delta.clone()) + })?; + let error = if let Some(io) = error.downcast_ref::() { + ApplyPatchError::from(io) } else { - Err(ApplyPatchError::IoError(IoError { + ApplyPatchError::IoError(IoError { context: msg, - source: std::io::Error::other(err), - })) - } + source: std::io::Error::other(error), + }) + }; + Err(ApplyPatchFailure::new(error, delta)) } } } @@ -299,11 +357,6 @@ pub struct AffectedPaths { pub deleted: Vec, } -struct AppliedHunks { - affected_paths: AffectedPaths, - delta: AppliedPatchDelta, -} - /// Apply the hunks to the filesystem, returning which files were added, modified, or deleted. /// Returns an error if the patch could not be applied. async fn apply_hunks_to_files( @@ -311,7 +364,8 @@ async fn apply_hunks_to_files( cwd: &AbsolutePathBuf, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, -) -> anyhow::Result { + delta: &mut AppliedPatchDelta, +) -> anyhow::Result { if hunks.is_empty() { anyhow::bail!("No files were modified."); } @@ -319,24 +373,39 @@ async fn apply_hunks_to_files( let mut added: Vec = Vec::new(); let mut modified: Vec = Vec::new(); let mut deleted: Vec = Vec::new(); - let mut delta_changes = Vec::new(); - let mut delta_exact = true; + // A failed write can still have modified the target before surfacing an + // error (for example by truncating before ENOSPC), so the accumulated + // delta is no longer exact when a write fails. + macro_rules! try_write { + ($result:expr) => { + match $result { + Ok(value) => value, + Err(error) => { + delta.exact = false; + return Err(anyhow::Error::from(error)); + } + } + }; + } + for hunk in hunks { let affected_path = hunk.path().to_path_buf(); let path_abs = hunk.resolve_path(cwd); match hunk { Hunk::AddFile { contents, .. } => { let overwritten_content = - read_optional_file_text_for_delta(&path_abs, fs, sandbox, &mut delta_exact) + read_optional_file_text_for_delta(&path_abs, fs, sandbox, &mut delta.exact) .await; - write_file_with_missing_parent_retry( - fs, - &path_abs, - contents.clone().into_bytes(), - sandbox, - ) - .await?; - delta_changes.push(AppliedPatchChange { + try_write!( + write_file_with_missing_parent_retry( + fs, + &path_abs, + contents.clone().into_bytes(), + sandbox, + ) + .await + ); + delta.changes.push(AppliedPatchChange { path: path_abs.into_path_buf(), change: AppliedPatchFileChange::Add { content: contents.clone(), @@ -346,20 +415,16 @@ async fn apply_hunks_to_files( added.push(affected_path); } Hunk::DeleteFile { .. } => { - note_existing_path_delta_support(&path_abs, fs, sandbox, &mut delta_exact).await; + note_existing_path_delta_support(&path_abs, fs, sandbox, &mut delta.exact).await; let deleted_content = fs.read_file_text(&path_abs, sandbox).await.ok(); if deleted_content.is_none() { - delta_exact = false; + delta.exact = false; } - let result: io::Result<()> = async { - let metadata = fs.get_metadata(&path_abs, sandbox).await?; - if metadata.is_directory { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - "path is a directory", - )); - } - fs.remove( + ensure_not_directory(&path_abs, fs, sandbox) + .await + .with_context(|| format!("Failed to delete file {}", path_abs.display()))?; + if let Err(error) = fs + .remove( &path_abs, RemoveOptions { recursive: false, @@ -368,11 +433,19 @@ async fn apply_hunks_to_files( sandbox, ) .await + .with_context(|| format!("Failed to delete file {}", path_abs.display())) + { + delta.exact &= remove_failure_was_side_effect_free( + &path_abs, + deleted_content.as_deref(), + fs, + sandbox, + ) + .await; + return Err(error); } - .await; - result.with_context(|| format!("Failed to delete file {}", path_abs.display()))?; if let Some(content) = deleted_content { - delta_changes.push(AppliedPatchChange { + delta.changes.push(AppliedPatchChange { path: path_abs.into_path_buf(), change: AppliedPatchFileChange::Delete { content }, }); @@ -382,7 +455,7 @@ async fn apply_hunks_to_files( Hunk::UpdateFile { move_path, chunks, .. } => { - note_existing_path_delta_support(&path_abs, fs, sandbox, &mut delta_exact).await; + note_existing_path_delta_support(&path_abs, fs, sandbox, &mut delta.exact).await; let AppliedPatch { original_contents, new_contents, @@ -390,24 +463,32 @@ async fn apply_hunks_to_files( if let Some(dest) = move_path { let dest_abs = AbsolutePathBuf::resolve_path_against_base(dest, cwd); let overwritten_move_content = - read_optional_file_text_for_delta(&dest_abs, fs, sandbox, &mut delta_exact) + read_optional_file_text_for_delta(&dest_abs, fs, sandbox, &mut delta.exact) .await; - write_file_with_missing_parent_retry( - fs, - &dest_abs, - new_contents.clone().into_bytes(), - sandbox, - ) - .await?; - let result: io::Result<()> = async { - let metadata = fs.get_metadata(&path_abs, sandbox).await?; - if metadata.is_directory { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - "path is a directory", - )); - } - fs.remove( + try_write!( + write_file_with_missing_parent_retry( + fs, + &dest_abs, + new_contents.clone().into_bytes(), + sandbox, + ) + .await + ); + let dest_write_change_index = delta.changes.len(); + delta.changes.push(AppliedPatchChange { + path: dest_abs.to_path_buf(), + change: AppliedPatchFileChange::Add { + content: new_contents.clone(), + overwritten_content: overwritten_move_content.clone(), + }, + }); + ensure_not_directory(&path_abs, fs, sandbox) + .await + .with_context(|| { + format!("Failed to remove original {}", path_abs.display()) + })?; + if let Err(error) = fs + .remove( &path_abs, RemoveOptions { recursive: false, @@ -416,12 +497,20 @@ async fn apply_hunks_to_files( sandbox, ) .await + .with_context(|| { + format!("Failed to remove original {}", path_abs.display()) + }) + { + delta.exact &= remove_failure_was_side_effect_free( + &path_abs, + Some(&original_contents), + fs, + sandbox, + ) + .await; + return Err(error); } - .await; - result.with_context(|| { - format!("Failed to remove original {}", path_abs.display()) - })?; - delta_changes.push(AppliedPatchChange { + delta.changes[dest_write_change_index] = AppliedPatchChange { path: path_abs.into_path_buf(), change: AppliedPatchFileChange::Update { move_path: Some(dest_abs.into_path_buf()), @@ -429,13 +518,18 @@ async fn apply_hunks_to_files( overwritten_move_content, new_content: new_contents, }, - }); + }; modified.push(affected_path); } else { - fs.write_file(&path_abs, new_contents.clone().into_bytes(), sandbox) - .await - .with_context(|| format!("Failed to write file {}", path_abs.display()))?; - delta_changes.push(AppliedPatchChange { + try_write!( + fs.write_file(&path_abs, new_contents.clone().into_bytes(), sandbox) + .await + .with_context(|| format!( + "Failed to write file {}", + path_abs.display() + )) + ); + delta.changes.push(AppliedPatchChange { path: path_abs.into_path_buf(), change: AppliedPatchFileChange::Update { move_path: None, @@ -449,16 +543,43 @@ async fn apply_hunks_to_files( } } } - Ok(AppliedHunks { - affected_paths: AffectedPaths { - added, - modified, - deleted, - }, - delta: AppliedPatchDelta::new(delta_changes, delta_exact), + Ok(AffectedPaths { + added, + modified, + deleted, }) } +async fn ensure_not_directory( + path: &AbsolutePathBuf, + fs: &dyn ExecutorFileSystem, + sandbox: Option<&FileSystemSandboxContext>, +) -> io::Result<()> { + let metadata = fs.get_metadata(path, sandbox).await?; + if metadata.is_directory { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "path is a directory", + )); + } + Ok(()) +} + +async fn remove_failure_was_side_effect_free( + path: &AbsolutePathBuf, + expected_content: Option<&str>, + fs: &dyn ExecutorFileSystem, + sandbox: Option<&FileSystemSandboxContext>, +) -> bool { + match expected_content { + Some(expected_content) => fs + .read_file_text(path, sandbox) + .await + .is_ok_and(|content| content == expected_content), + None => false, + } +} + async fn read_optional_file_text_for_delta( path: &AbsolutePathBuf, fs: &dyn ExecutorFileSystem, @@ -972,6 +1093,61 @@ mod tests { assert_eq!(contents, "line2\n"); } + #[cfg(unix)] + #[tokio::test] + async fn test_failed_move_returns_committed_destination_delta() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempdir().unwrap(); + let source_dir = dir.path().join("locked"); + let dest_dir = dir.path().join("out"); + fs::create_dir(&source_dir).unwrap(); + fs::create_dir(&dest_dir).unwrap(); + let src = source_dir.join("src.txt"); + let dest = dest_dir.join("dst.txt"); + fs::write(&src, "line\n").unwrap(); + fs::set_permissions(&source_dir, fs::Permissions::from_mode(0o555)).unwrap(); + + let patch = wrap_patch( + "*** Update File: locked/src.txt\n*** Move to: out/dst.txt\n@@\n-line\n+line2", + ); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let failure = apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + /*sandbox*/ None, + ) + .await + .expect_err("source removal should fail after destination write"); + + fs::set_permissions(&source_dir, fs::Permissions::from_mode(0o755)).unwrap(); + + assert!( + String::from_utf8(stderr) + .unwrap() + .contains(&format!("Failed to remove original {}", src.display())) + ); + assert_eq!( + failure.delta(), + &AppliedPatchDelta::new( + vec![AppliedPatchChange { + path: dest.clone(), + change: AppliedPatchFileChange::Add { + content: "line2\n".to_string(), + overwritten_content: None, + }, + }], + /*exact*/ true, + ) + ); + assert_eq!(fs::read_to_string(src).unwrap(), "line\n"); + assert_eq!(fs::read_to_string(dest).unwrap(), "line2\n"); + } + /// 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. #[tokio::test] @@ -1427,19 +1603,17 @@ g ); } + #[cfg(unix)] #[tokio::test] async fn test_apply_patch_fails_on_write_error() { + use std::os::unix::fs::PermissionsExt; + let dir = tempdir().unwrap(); - let path = dir.path().join("readonly.txt"); - fs::write(&path, "before\n").unwrap(); - let mut perms = fs::metadata(&path).unwrap().permissions(); - perms.set_readonly(true); - fs::set_permissions(&path, perms).unwrap(); + let locked_dir = dir.path().join("locked"); + fs::create_dir(&locked_dir).unwrap(); + fs::set_permissions(&locked_dir, fs::Permissions::from_mode(0o555)).unwrap(); - let patch = wrap_patch(&format!( - "*** Update File: {}\n@@\n-before\n+after\n*** End Patch", - path.display() - )); + let patch = wrap_patch("*** Add File: locked/new.txt\n+after"); let mut stdout = Vec::new(); let mut stderr = Vec::new(); @@ -1452,7 +1626,11 @@ g /*sandbox*/ None, ) .await; - assert!(result.is_err()); + let failure = result.expect_err("write should fail"); + + fs::set_permissions(&locked_dir, fs::Permissions::from_mode(0o755)).unwrap(); + + assert!(!failure.delta().is_exact()); } #[tokio::test] diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index f3a9513ae9e2..51f1833f074b 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -57,13 +57,16 @@ pub(crate) enum ToolEventStage<'a> { output: ExecToolCallOutput, applied_patch_delta: Option<&'a AppliedPatchDelta>, }, - Failure(ToolEventFailure), + Failure(ToolEventFailure<'a>), } -pub(crate) enum ToolEventFailure { +pub(crate) enum ToolEventFailure<'a> { Output(ExecToolCallOutput), Message(String), - Rejected(String), + Rejected { + message: String, + applied_patch_delta: Option<&'a AppliedPatchDelta>, + }, } enum TurnDiffTrackerUpdate<'a> { @@ -72,6 +75,14 @@ enum TurnDiffTrackerUpdate<'a> { None, } +fn tracker_update_for_known_delta(delta: &AppliedPatchDelta) -> TurnDiffTrackerUpdate<'_> { + if delta.is_exact() && delta.is_empty() { + TurnDiffTrackerUpdate::None + } else { + TurnDiffTrackerUpdate::Track(delta) + } +} + pub(crate) async fn emit_exec_command_begin( ctx: ToolEventCtx<'_>, command: &[String], @@ -217,15 +228,9 @@ impl ToolEmitter { } else { PatchApplyStatus::Failed }; - let tracker_update = if output.exit_code == 0 { - if let Some(delta) = applied_patch_delta { - TurnDiffTrackerUpdate::Track(delta) - } else { - TurnDiffTrackerUpdate::Invalidate - } - } else { - TurnDiffTrackerUpdate::Invalidate - }; + let tracker_update = applied_patch_delta + .map(tracker_update_for_known_delta) + .unwrap_or(TurnDiffTrackerUpdate::Invalidate); emit_patch_end( ctx, changes.clone(), @@ -270,7 +275,10 @@ impl ToolEmitter { } ( Self::ApplyPatch { changes, .. }, - ToolEventStage::Failure(ToolEventFailure::Rejected(message)), + ToolEventStage::Failure(ToolEventFailure::Rejected { + message, + applied_patch_delta, + }), ) => { emit_patch_end( ctx, @@ -278,7 +286,9 @@ impl ToolEmitter { String::new(), (*message).to_string(), PatchApplyStatus::Declined, - TurnDiffTrackerUpdate::None, + applied_patch_delta + .map(tracker_update_for_known_delta) + .unwrap_or(TurnDiffTrackerUpdate::None), ) .await; } @@ -347,13 +357,27 @@ impl ToolEmitter { }; (event, result) } - Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Timeout { output }))) - | Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output, .. }))) => { + Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Timeout { output }))) => { let response = self.format_exec_output_for_model(&output, ctx); let event = ToolEventStage::Failure(ToolEventFailure::Output(*output)); let result = Err(FunctionCallError::RespondToModel(response)); (event, result) } + Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output, .. }))) => { + let response = self.format_exec_output_for_model(&output, ctx); + // apply_patch can be denied after it has already committed a + // known prefix. Reuse the output-bearing path so the visible + // item still fails while the turn diff consumes that prefix. + let event = match (self, applied_patch_delta) { + (Self::ApplyPatch { .. }, Some(delta)) => ToolEventStage::Success { + output: *output, + applied_patch_delta: Some(delta), + }, + _ => ToolEventStage::Failure(ToolEventFailure::Output(*output)), + }; + let result = Err(FunctionCallError::RespondToModel(response)); + (event, result) + } Err(ToolError::Codex(err)) => { let message = format!("execution error: {err:?}"); let event = ToolEventStage::Failure(ToolEventFailure::Message(message.clone())); @@ -380,7 +404,10 @@ impl ToolEmitter { } else { msg }; - let event = ToolEventStage::Failure(ToolEventFailure::Rejected(normalized.clone())); + let event = ToolEventStage::Failure(ToolEventFailure::Rejected { + message: normalized.clone(), + applied_patch_delta, + }); let result = Err(FunctionCallError::RespondToModel(normalized)); (event, result) } @@ -477,7 +504,7 @@ async fn emit_exec_stage( }; emit_exec_end(ctx, exec_input, exec_result).await; } - ToolEventStage::Failure(ToolEventFailure::Rejected(message)) => { + ToolEventStage::Failure(ToolEventFailure::Rejected { message, .. }) => { let text = message.to_string(); let exec_result = ExecCommandResult { stdout: String::new(), @@ -550,8 +577,8 @@ async fn emit_patch_end( let mut guard = tracker.lock().await; let previous_diff = guard.get_unified_diff(); let tracker_changed = match tracker_update { - TurnDiffTrackerUpdate::Track(action) => { - guard.track_successful_patch(action); + TurnDiffTrackerUpdate::Track(delta) => { + guard.track_delta(delta); true } TurnDiffTrackerUpdate::Invalidate => { @@ -573,3 +600,102 @@ async fn emit_patch_end( } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::session::tests::make_session_and_context_with_dynamic_tools_and_rx; + use crate::turn_diff_tracker::TurnDiffTracker; + use codex_exec_server::LOCAL_FS; + use codex_protocol::error::CodexErr; + use codex_protocol::error::SandboxErr; + use codex_protocol::exec_output::ExecToolCallOutput; + use codex_protocol::items::TurnItem; + use codex_protocol::protocol::PatchApplyStatus; + use codex_utils_absolute_path::AbsolutePathBuf; + use std::sync::Arc; + use tempfile::tempdir; + use tokio::sync::Mutex; + + async fn assert_failed_apply_patch_tracks_committed_delta( + out: Result, + expected_status: PatchApplyStatus, + ) { + let (session, turn, rx_event) = + make_session_and_context_with_dynamic_tools_and_rx(Vec::new()).await; + let tracker = Arc::new(Mutex::new(TurnDiffTracker::new())); + let dir = tempdir().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(dir.path()).expect("absolute cwd"); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let delta = codex_apply_patch::apply_patch( + "*** Begin Patch\n*** Add File: out/dest.txt\n+after\n*** End Patch", + &cwd, + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + /*sandbox*/ None, + ) + .await + .expect("apply patch"); + + ToolEmitter::apply_patch(HashMap::new(), /*auto_approved*/ false) + .finish( + ToolEventCtx::new(session.as_ref(), turn.as_ref(), "call-id", Some(&tracker)), + out, + Some(&delta), + ) + .await + .expect_err("failed patch"); + + let completed = rx_event.recv().await.expect("item completed event"); + assert!(matches!( + completed.msg, + EventMsg::ItemCompleted(event) + if matches!( + &event.item, + TurnItem::FileChange(FileChangeItem { + status: Some(status), + .. + }) if status == &expected_status + ) + )); + + let unified_diff = loop { + let event = tokio::time::timeout(Duration::from_secs(1), rx_event.recv()) + .await + .expect("turn diff event") + .expect("channel open"); + if let EventMsg::TurnDiff(TurnDiffEvent { unified_diff }) = event.msg { + break unified_diff; + } + }; + assert!(unified_diff.contains("out/dest.txt")); + assert!(unified_diff.contains("+after")); + } + + #[tokio::test] + async fn denied_apply_patch_tracks_committed_delta() { + let output = ExecToolCallOutput { + exit_code: 1, + ..Default::default() + }; + assert_failed_apply_patch_tracks_committed_delta( + Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { + output: Box::new(output), + network_policy_decision: None, + }))), + PatchApplyStatus::Failed, + ) + .await; + } + + #[tokio::test] + async fn rejected_apply_patch_tracks_committed_delta() { + assert_failed_apply_patch_tracks_committed_delta( + Err(ToolError::Rejected("rejected by user".to_string())), + PatchApplyStatus::Declined, + ) + .await; + } +} diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 9e31eb9915ed..104ab9ffb6c3 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -438,8 +438,8 @@ impl ToolHandler for ApplyPatchHandler { .await .map(|result| result.output); let (out, delta) = match out { - Ok(output) => (Ok(output.exec_output), output.delta), - Err(error) => (Err(error), None), + Ok(output) => (Ok(output.exec_output), Some(output.delta)), + Err(error) => (Err(error), Some(runtime.committed_delta().clone())), }; let event_ctx = ToolEventCtx::new( session.as_ref(), @@ -550,8 +550,8 @@ pub(crate) async fn intercept_apply_patch( .await .map(|result| result.output); let (out, delta) = match out { - Ok(output) => (Ok(output.exec_output), output.delta), - Err(error) => (Err(error), None), + Ok(output) => (Ok(output.exec_output), Some(output.delta)), + Err(error) => (Err(error), Some(runtime.committed_delta().clone())), }; let event_ctx = ToolEventCtx::new( session.as_ref(), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 69110a250142..1ab249fb0aed 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -47,17 +47,23 @@ pub struct ApplyPatchRequest { } #[derive(Default)] -pub struct ApplyPatchRuntime; +pub struct ApplyPatchRuntime { + committed_delta: AppliedPatchDelta, +} #[derive(Debug)] pub struct ApplyPatchRuntimeOutput { pub exec_output: ExecToolCallOutput, - pub delta: Option, + pub delta: AppliedPatchDelta, } impl ApplyPatchRuntime { pub fn new() -> Self { - Self + Self::default() + } + + pub fn committed_delta(&self) -> &AppliedPatchDelta { + &self.committed_delta } fn build_guardian_review_request( @@ -217,7 +223,13 @@ impl ToolRuntime for ApplyPatchRunti .await; let stdout = String::from_utf8_lossy(&stdout).into_owned(); let stderr = String::from_utf8_lossy(&stderr).into_owned(); - let exit_code = if result.is_ok() { 0 } else { 1 }; + let failed = result.is_err(); + let exit_code = if failed { 1 } else { 0 }; + let delta = match result { + Ok(delta) => delta, + Err(failure) => failure.into_parts().1, + }; + self.committed_delta.append(delta); let output = ExecToolCallOutput { exit_code, stdout: StreamOutput::new(stdout.clone()), @@ -226,7 +238,7 @@ impl ToolRuntime for ApplyPatchRunti duration: started_at.elapsed(), timed_out: false, }; - if result.is_err() && is_likely_sandbox_denied(attempt.sandbox, &output) { + if failed && is_likely_sandbox_denied(attempt.sandbox, &output) { return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output: Box::new(output), network_policy_decision: None, @@ -234,7 +246,7 @@ impl ToolRuntime for ApplyPatchRunti } Ok(ApplyPatchRuntimeOutput { exec_output: output, - delta: result.ok(), + delta: self.committed_delta.clone(), }) } } diff --git a/codex-rs/core/src/turn_diff_tracker.rs b/codex-rs/core/src/turn_diff_tracker.rs index b7b75bbca7d2..3835ae234593 100644 --- a/codex-rs/core/src/turn_diff_tracker.rs +++ b/codex-rs/core/src/turn_diff_tracker.rs @@ -13,8 +13,8 @@ const ZERO_OID: &str = "0000000000000000000000000000000000000000"; const DEV_NULL: &str = "/dev/null"; const REGULAR_FILE_MODE: &str = "100644"; -/// Tracks the net text diff for the current turn from successful apply_patch -/// operations, without rereading the workspace filesystem. +/// Tracks the net text diff for the current turn from committed apply_patch +/// mutations, without rereading the workspace filesystem. pub struct TurnDiffTracker { valid: bool, display_root: Option, @@ -46,7 +46,7 @@ impl TurnDiffTracker { tracker } - pub fn track_successful_patch(&mut self, delta: &AppliedPatchDelta) { + pub fn track_delta(&mut self, delta: &AppliedPatchDelta) { if !delta.is_exact() { self.invalidate(); return; diff --git a/codex-rs/core/src/turn_diff_tracker_tests.rs b/codex-rs/core/src/turn_diff_tracker_tests.rs index ed5da7063479..d25fec2aadd7 100644 --- a/codex-rs/core/src/turn_diff_tracker_tests.rs +++ b/codex-rs/core/src/turn_diff_tracker_tests.rs @@ -51,14 +51,14 @@ async fn accumulates_add_then_update_as_single_add() { "*** Begin Patch\n*** Add File: a.txt\n+foo\n*** End Patch", ) .await; - tracker.track_successful_patch(&add); + tracker.track_delta(&add); let update = apply_verified_patch( dir.path(), "*** Begin Patch\n*** Update File: a.txt\n@@\n foo\n+bar\n*** End Patch", ) .await; - tracker.track_successful_patch(&update); + tracker.track_delta(&update); let right_oid = git_blob_sha1_hex("foo\nbar\n"); let expected = format!( @@ -85,7 +85,7 @@ async fn invalidated_tracker_suppresses_existing_diff() { "*** Begin Patch\n*** Add File: a.txt\n+foo\n*** End Patch", ) .await; - tracker.track_successful_patch(&add); + tracker.track_delta(&add); tracker.invalidate(); @@ -103,7 +103,7 @@ async fn accumulates_delete() { "*** Begin Patch\n*** Delete File: b.txt\n*** End Patch", ) .await; - tracker.track_successful_patch(&delete); + tracker.track_delta(&delete); let left_oid = git_blob_sha1_hex("x\n"); let expected = format!( @@ -130,7 +130,7 @@ async fn accumulates_move_and_update() { "*** Begin Patch\n*** Update File: src.txt\n*** Move to: dst.txt\n@@\n-line\n+line2\n*** End Patch", ) .await; - tracker.track_successful_patch(&update); + tracker.track_delta(&update); let left_oid = git_blob_sha1_hex("line\n"); let right_oid = git_blob_sha1_hex("line2\n"); @@ -158,7 +158,7 @@ async fn pure_rename_yields_no_diff() { "*** Begin Patch\n*** Update File: old.txt\n*** Move to: new.txt\n@@\n same\n*** End Patch", ) .await; - tracker.track_successful_patch(&rename); + tracker.track_delta(&rename); assert_eq!(tracker.get_unified_diff(), None); } @@ -174,7 +174,7 @@ async fn add_over_existing_file_becomes_update() { "*** Begin Patch\n*** Add File: dup.txt\n+after\n*** End Patch", ) .await; - tracker.track_successful_patch(&add); + tracker.track_delta(&add); let left_oid = git_blob_sha1_hex("before\n"); let right_oid = git_blob_sha1_hex("after\n"); @@ -202,14 +202,14 @@ async fn delete_then_readd_same_path_becomes_update() { "*** Begin Patch\n*** Delete File: cycle.txt\n*** End Patch", ) .await; - tracker.track_successful_patch(&delete); + tracker.track_delta(&delete); let add = apply_verified_patch( dir.path(), "*** Begin Patch\n*** Add File: cycle.txt\n+after\n*** End Patch", ) .await; - tracker.track_successful_patch(&add); + tracker.track_delta(&add); let left_oid = git_blob_sha1_hex("before\n"); let right_oid = git_blob_sha1_hex("after\n"); @@ -238,7 +238,7 @@ async fn move_over_existing_destination_without_content_change_deletes_source_on "*** Begin Patch\n*** Update File: a.txt\n*** Move to: b.txt\n@@\n same\n*** End Patch", ) .await; - tracker.track_successful_patch(&move_overwrite); + tracker.track_delta(&move_overwrite); let left_oid = git_blob_sha1_hex("same\n"); let expected = format!( @@ -267,7 +267,7 @@ async fn move_over_existing_destination_with_content_change_deletes_source_and_u "*** Begin Patch\n*** Update File: a.txt\n*** Move to: b.txt\n@@\n-from\n+new\n*** End Patch", ) .await; - tracker.track_successful_patch(&move_overwrite); + tracker.track_delta(&move_overwrite); let left_oid_a = git_blob_sha1_hex("from\n"); let left_oid_b = git_blob_sha1_hex("existing\n"); @@ -304,7 +304,7 @@ async fn preserves_committed_change_order_with_delete_then_move_overwrite() { "*** Begin Patch\n*** Delete File: b.txt\n*** Update File: a.txt\n*** Move to: b.txt\n@@\n-from\n+new\n*** End Patch", ) .await; - tracker.track_successful_patch(&ordered_patch); + tracker.track_delta(&ordered_patch); let left_oid_a = git_blob_sha1_hex("from\n"); let left_oid_b = git_blob_sha1_hex("existing\n"); diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index e52bca8fcaa1..bc51fdd460dd 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -63,6 +63,21 @@ async fn apply_patch_harness_with( } async fn submit_without_wait(harness: &TestCodexHarness, prompt: &str) -> Result<()> { + submit_without_wait_with_turn_permissions( + harness, + prompt, + SandboxPolicy::DangerFullAccess, + /*permission_profile*/ None, + ) + .await +} + +async fn submit_without_wait_with_turn_permissions( + harness: &TestCodexHarness, + prompt: &str, + sandbox_policy: SandboxPolicy, + permission_profile: Option, +) -> Result<()> { let test = harness.test(); let session_model = test.session_configured.model.clone(); test.codex @@ -76,8 +91,8 @@ async fn submit_without_wait(harness: &TestCodexHarness, prompt: &str) -> Result cwd: harness.cwd().to_path_buf(), approval_policy: AskForApproval::Never, approvals_reviewer: None, - sandbox_policy: SandboxPolicy::DangerFullAccess, - permission_profile: None, + sandbox_policy, + permission_profile, model: session_model, effort: None, summary: None,