diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 99a63e3ace..a780c6180d 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -397,7 +397,7 @@ async fn apply_hunks_to_files( read_optional_file_text_for_delta(&path_abs, fs, sandbox, &mut delta.exact) .await; try_write!( - write_file_with_missing_parent_retry( + replace_file_with_missing_parent_retry( fs, &path_abs, contents.clone().into_bytes(), @@ -466,7 +466,7 @@ async fn apply_hunks_to_files( read_optional_file_text_for_delta(&dest_abs, fs, sandbox, &mut delta.exact) .await; try_write!( - write_file_with_missing_parent_retry( + replace_file_with_missing_parent_retry( fs, &dest_abs, new_contents.clone().into_bytes(), @@ -522,12 +522,13 @@ async fn apply_hunks_to_files( modified.push(affected_path); } else { try_write!( - fs.write_file(&path_abs, new_contents.clone().into_bytes(), sandbox) - .await - .with_context(|| format!( - "Failed to write file {}", - path_abs.display() - )) + replace_file_with_missing_parent_retry( + fs, + &path_abs, + new_contents.clone().into_bytes(), + sandbox, + ) + .await ); delta.changes.push(AppliedPatchChange { path: path_abs.into_path_buf(), @@ -645,6 +646,45 @@ async fn write_file_with_missing_parent_retry( } } +async fn replace_file_with_missing_parent_retry( + fs: &dyn ExecutorFileSystem, + path_abs: &AbsolutePathBuf, + contents: Vec, + sandbox: Option<&FileSystemSandboxContext>, +) -> anyhow::Result<()> { + match fs.get_metadata(path_abs, sandbox).await { + Ok(metadata) => { + if metadata.is_directory { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "path is a directory", + )) + .with_context(|| format!("Failed to write file {}", path_abs.display())); + } + if metadata.is_symlink { + return write_file_with_missing_parent_retry(fs, path_abs, contents, sandbox).await; + } + fs.remove( + path_abs, + RemoveOptions { + recursive: false, + force: false, + }, + sandbox, + ) + .await + .with_context(|| format!("Failed to replace file {}", path_abs.display()))?; + } + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => { + return Err(error) + .with_context(|| format!("Failed to inspect file {}", path_abs.display())); + } + } + + write_file_with_missing_parent_retry(fs, path_abs, contents, sandbox).await +} + struct AppliedPatch { original_contents: String, new_contents: String, diff --git a/codex-rs/apply-patch/tests/suite/tool.rs b/codex-rs/apply-patch/tests/suite/tool.rs index 8499d0fb90..f224539682 100644 --- a/codex-rs/apply-patch/tests/suite/tool.rs +++ b/codex-rs/apply-patch/tests/suite/tool.rs @@ -205,6 +205,52 @@ fn test_apply_patch_cli_add_overwrites_existing_file() -> anyhow::Result<()> { Ok(()) } +#[test] +fn test_apply_patch_cli_add_replaces_existing_hard_link() -> anyhow::Result<()> { + let tmp = tempdir()?; + let workspace = tmp.path().join("workspace"); + let outside = tmp.path().join("outside.txt"); + let link = workspace.join("hard-link.txt"); + fs::create_dir_all(&workspace)?; + fs::write(&outside, "outside\n")?; + fs::hard_link(&outside, &link)?; + + run_apply_patch_in_dir( + &workspace, + "*** Begin Patch\n*** Add File: hard-link.txt\n+workspace replacement\n*** End Patch", + )? + .success() + .stdout("Success. Updated the following files:\nA hard-link.txt\n"); + + assert_eq!(fs::read_to_string(&outside)?, "outside\n"); + assert_eq!(fs::read_to_string(&link)?, "workspace replacement\n"); + + Ok(()) +} + +#[test] +fn test_apply_patch_cli_update_replaces_existing_hard_link() -> anyhow::Result<()> { + let tmp = tempdir()?; + let workspace = tmp.path().join("workspace"); + let outside = tmp.path().join("outside.txt"); + let link = workspace.join("hard-link.txt"); + fs::create_dir_all(&workspace)?; + fs::write(&outside, "outside\n")?; + fs::hard_link(&outside, &link)?; + + run_apply_patch_in_dir( + &workspace, + "*** Begin Patch\n*** Update File: hard-link.txt\n@@\n-outside\n+workspace replacement\n*** End Patch", + )? + .success() + .stdout("Success. Updated the following files:\nM hard-link.txt\n"); + + assert_eq!(fs::read_to_string(&outside)?, "outside\n"); + assert_eq!(fs::read_to_string(&link)?, "workspace replacement\n"); + + Ok(()) +} + #[test] fn test_apply_patch_cli_delete_directory_fails() -> anyhow::Result<()> { let tmp = tempdir()?; diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index f95fd1efef..10134d72a3 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -796,7 +796,7 @@ async fn apply_patch_cli_does_not_write_through_symlink_escape_outside_workspace #[test_case(ApplyPatchModelOutput::Shell ; "shell")] #[test_case(ApplyPatchModelOutput::ShellViaHeredoc ; "shell_heredoc")] #[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc ; "shell_command_heredoc")] -async fn apply_patch_cli_preserves_existing_hard_link_outside_workspace( +async fn apply_patch_cli_replaces_existing_hard_link_outside_workspace( model_output: ApplyPatchModelOutput, ) -> Result<()> { skip_if_no_network!(Ok(())); @@ -872,24 +872,24 @@ async fn apply_patch_cli_preserves_existing_hard_link_outside_workspace( assert!( out.contains("Success. Updated the following files:"), - "apply_patch should intentionally allow updates through existing hard links; tool output: {out}" + "apply_patch should update the workspace path without mutating the outside hard-link target; tool output: {out}" ); assert_eq!( std::fs::read_to_string(&outside_file)?, - "updated through existing hard link\n", - "apply_patch intentionally preserves existing hard-link semantics; the outside path observes the shared inode update" + "original outside content\n", + "apply_patch must not mutate the outside hard-link target" ); assert_eq!( std::fs::read_to_string(&link_path)?, "updated through existing hard link\n", - "apply_patch intentionally preserves existing hard-link semantics; the workspace path observes the same update" + "apply_patch should still update the workspace path" ); std::fs::write(&outside_file, "post-apply outside write\n")?; assert_eq!( std::fs::read_to_string(&link_path)?, - "post-apply outside write\n", - "apply_patch must not unlink or replace an existing hard link; later writes through either path should still be visible" + "updated through existing hard link\n", + "apply_patch should replace the workspace hard-link entry instead of preserving the shared inode" ); Ok(())