fix: keep the file when apply_patch does a case-only rename

src/agents/sandbox/apply_patch.py

@@ -1,8 +1,10 @@
 from __future__ import annotations
 
+import contextlib
 import io
 from pathlib import Path
 from typing import TYPE_CHECKING, Any, Literal, Protocol, cast, runtime_checkable
+from uuid import uuid4
 
 from ..apply_diff import ApplyDiffMode, apply_diff
 from ..editor import ApplyPatchOperation, ApplyPatchOperationType, ApplyPatchResult
@@ -111,9 +113,11 @@ async def apply_operation(
 
             moved_relative_path, moved_display_path = self._resolve_path(operation.move_to)
             moved_destination = self._session.normalize_path(moved_relative_path)
-            await self._write_text(moved_destination, updated_text)
-            if moved_destination != destination:
-                await self._session.rm(destination, user=self._user)
+            await self._move_updated_text(
+                source=destination,
+                moved_destination=moved_destination,
+                text=updated_text,
+            )
             return ApplyPatchResult(
                 output=f"Updated {display_path}\nMoved {display_path} to {moved_display_path}"
             )
@@ -209,6 +213,61 @@ async def _read_text(self, destination: Path, *, op_path: str, decode_path: Path
             path=op_path,
         )
 
+    async def _move_updated_text(
+        self,
+        *,
+        source: Path,
+        moved_destination: Path,
+        text: str,
+    ) -> None:
+        """Apply an update that renames the file, without a window in which it does not exist.
+
+        Writing the destination and then removing the source destroys the file whenever the two
+        paths are one file on disk, which is what a case-only rename is on a filesystem that
+        folds case. Removing the source first destroys it whenever the replacement write fails.
+
+        So neither path is written or removed until the new content is committed somewhere else:
+        the text goes to a staging file, a single `mv` puts it at the destination, and only then
+        is the source removed, and only if the filesystem says it is a different entry. Before
+        that `mv` the original is untouched; after it the new content exists. There is no moment
+        where the only copy is in memory, and nothing is restored after the fact, so a file that
+        another writer creates at the source path while this runs is never overwritten.
+
+        It can still be removed. The identity answer is read before the removal, and the removal
+        names a path rather than the entry that answer was about, so a writer that replaces the
+        source between the two loses the file it just wrote. Closing that needs a removal that
+        can be told which entry it is allowed to remove, which no backend here offers.
+
+        The staging file is a new inode, so a rename the filesystem folds onto the source path
+        replaces the original's mode and extended attributes. Carrying those across would mean
+        reading and reapplying them per backend; committing the content in a single `mv` is
+        worth more than the mode bits.
+
+        The staging name is a fixed length rather than a decoration of the destination name,
+        because a destination basename near the filesystem's 255-byte limit would make the
+        decorated name exceed it and the write would fail with ENAMETOOLONG.
+        """
+        if source == moved_destination:
+            # Not a rename, so nothing needs committing elsewhere. Writing in place is what an
+            # update without `move_to` does, and it keeps the inode, the mode and the xattrs.
+            await self._write_text(source, text)
+            return
+
+        staging = moved_destination.with_name(f".apply_patch-{uuid4().hex}.tmp")
+        try:
+            await self._write_text(staging, text)
+            await self._session.mv(staging, moved_destination, user=self._user)
+        except BaseException:
+            with contextlib.suppress(Exception):
+                await self._session.rm(staging, user=self._user)
+            raise
+        # A symlink is its own directory entry: removing it leaves the file it points at, so
+        # the source still has to go. `-ef` follows symlinks, so ask without following.
+        if not await self._session.same_file(
+            source, moved_destination, follow_symlinks=False, user=self._user
+        ):
+            await self._session.rm(source, user=self._user)
+
     async def _write_text(self, destination: Path, text: str) -> None:
         await self._session.mkdir(destination.parent, parents=True, user=self._user)
         await self._session.write(

src/agents/sandbox/session/base_sandbox_session.py

@@ -1146,6 +1146,100 @@ async def rm(
         if not result.ok():
             raise ExecNonZeroError(result, command=cmd)
 
+    async def mv(
+        self,
+        source: Path | str,
+        destination: Path | str,
+        *,
+        user: str | User | None = None,
+    ) -> None:
+        """Rename a path, replacing the destination if it exists.
+
+        This is a rename, not `mv`'s other behavior. Given an existing directory as the
+        destination, `mv` puts the source inside it and reports success, which for a caller
+        that then removes the source is a way to delete a file while believing it moved. The
+        destination is checked in the same shell invocation as the move, which keeps the
+        check and the move in one round trip. It does not make them one syscall.
+
+        `mv -T` would say this directly and is GNU-only, so it is unavailable on the BSD
+        userland this also has to run against.
+
+        :param source: Path to move.
+        :param destination: Path to move it to.
+        :param user: Optional sandbox user to move as.
+        :raises ExecNonZeroError: If the destination is an existing directory, or the move
+                fails.
+        """
+        source = await self._validate_path_access(source, for_write=True)
+        destination = await self._validate_path_access(destination, for_write=True)
+
+        source_arg = sandbox_path_str(source)
+        destination_arg = sandbox_path_str(destination)
+        cmd = (
+            "sh",
+            "-lc",
+            'if [ -d "$2" ]; then exit 3; fi\nmv -f -- "$1" "$2"',
+            "sh",
+            source_arg,
+            destination_arg,
+        )
+        result = await self.exec(*cmd, shell=False, user=user)
+        if not result.ok():
+            raise ExecNonZeroError(
+                result, command=("sh", "-lc", "<mv>", source_arg, destination_arg)
+            )
+
+    async def same_file(
+        self,
+        left: Path | str,
+        right: Path | str,
+        *,
+        follow_symlinks: bool = True,
+        user: str | User | None = None,
+    ) -> bool:
+        """Return whether two paths name the same file on the sandbox filesystem.
+
+        This asks the filesystem, through `test -ef`, which compares device and inode. Two
+        paths that differ as strings can be one file: a filesystem that folds case stores
+        `notes.txt` and `Notes.txt` as a single entry, and APFS folds Unicode normalization
+        as well, so the NFC and NFD spellings of one accented name are also a single entry.
+        No string comparison can answer this, and neither can the host that is driving the
+        session, which may not be the kind of system the sandbox is running on.
+
+        `test -ef` resolves symlinks, so a symlink and the file it points at are the same
+        file by this test while being two directory entries: removing the symlink leaves the
+        file alone. Pass ``follow_symlinks=False`` when the answer is going to decide whether
+        removing one path destroys the other, which makes a symlink on either side answer no.
+
+        :param left: First path to compare.
+        :param right: Second path to compare.
+        :param follow_symlinks: If false, a symlink on either side is not the same file as
+                its target.
+        :param user: Optional sandbox user to compare as.
+        :returns: True when both paths resolve to the same file.
+        """
+        left = await self._validate_path_access(left)
+        right = await self._validate_path_access(right)
+
+        left_arg = sandbox_path_str(left)
+        right_arg = sandbox_path_str(right)
+        test = '[ "$1" -ef "$2" ]'
+        if not follow_symlinks:
+            test = '[ ! -L "$1" ] && [ ! -L "$2" ] && ' + test
+        cmd = ("sh", "-lc", test, "sh", left_arg, right_arg)
+        result = await self.exec(*cmd, shell=False, user=user)
+        if result.exit_code == 0:
+            return True
+        # `[` answers "different file" with 1 and reports its own failures with 2, and a
+        # missing shell exits 127. Only 1 is an answer; anything else is the session
+        # failing to tell us, and a caller about to delete a file on the strength of this
+        # must not read that as "different".
+        if result.exit_code == 1:
+            return False
+        raise ExecNonZeroError(
+            result, command=("sh", "-lc", "<same_file_check>", left_arg, right_arg)
+        )
+
     async def mkdir(
         self,
         path: Path | str,

src/agents/sandbox/session/sandbox_session.py

@@ -641,6 +641,25 @@ async def rm(
     ) -> None:
         await self._inner.rm(path, recursive=recursive, user=user)
 
+    async def mv(
+        self,
+        source: Path | str,
+        destination: Path | str,
+        *,
+        user: str | User | None = None,
+    ) -> None:
+        await self._inner.mv(source, destination, user=user)
+
+    async def same_file(
+        self,
+        left: Path | str,
+        right: Path | str,
+        *,
+        follow_symlinks: bool = True,
+        user: str | User | None = None,
+    ) -> bool:
+        return await self._inner.same_file(left, right, follow_symlinks=follow_symlinks, user=user)
+
     async def mkdir(
         self,
         path: Path | str,

src/agents/testing/sandbox.py

@@ -27,10 +27,12 @@
     "exec",
     "ls",
     "mkdir",
+    "mv",
     "pty_exec_start",
     "pty_write_stdin",
     "read",
     "rm",
+    "same_file",
     "write",
 ]
 SandboxStepReason = Literal["invalid_input", "unknown_method", "invalid_matcher", "invalid_outcome"]
@@ -489,6 +491,32 @@ async def mkdir(
     ) -> None:
         await self._invoke("mkdir", (path,), {"parents": parents, "user": user})
 
+    async def mv(
+        self,
+        source: Path | str,
+        destination: Path | str,
+        *,
+        user: str | User | None = None,
+    ) -> None:
+        await self._invoke("mv", (source, destination), {"user": user})
+
+    async def same_file(
+        self,
+        left: Path | str,
+        right: Path | str,
+        *,
+        follow_symlinks: bool = True,
+        user: str | User | None = None,
+    ) -> bool:
+        return cast(
+            bool,
+            await self._invoke(
+                "same_file",
+                (left, right),
+                {"follow_symlinks": follow_symlinks, "user": user},
+            ),
+        )
+
     async def apply_patch(
         self,
         operations: ApplyPatchOperation