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

.github/workflows/tests.yml

@@ -361,7 +361,7 @@ jobs:
 
   tests-windows:
     runs-on: windows-latest
-    timeout-minutes: 10
+    timeout-minutes: 15
     strategy:
       fail-fast: false
       matrix:

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}"
             )
@@ -231,6 +235,65 @@ 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:
+        """Stage filesystem aliases; retain ordinary destination write semantics otherwise.
+
+        An alias needs a staged replacement because writing and then removing the source
+        would delete the updated file. Distinct files retain the released write/remove path,
+        including an existing destination's metadata and file-level write permissions.
+        If the paths stop identifying the same file after an alias replacement, leave both
+        paths alone and report the incomplete move. This includes distinct hardlink aliases.
+        These operations do not provide a transaction against concurrent workspace writers.
+        """
+        if source.as_posix() == moved_destination.as_posix():
+            # 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.
+            #
+            # The comparison is on the spelling rather than on `Path` equality, which folds case
+            # on a Windows host. Whether two sandbox paths are one file is the sandbox's answer,
+            # not the host's: a Windows host talking to a case-sensitive sandbox would otherwise
+            # take this branch for a case-only rename and never create the new name. Paths that
+            # differ only in case go down the staging path, where `same_file` asks the sandbox.
+            await self._write_text(source, text)
+            return
+
+        if not await self._session.same_file(
+            source, moved_destination, follow_symlinks=False, user=self._user
+        ):
+            await self._write_text(moved_destination, text)
+            await self._session.rm(source, user=self._user)
+            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
+        same_entry = await self._session.same_file(
+            source, moved_destination, follow_symlinks=False, user=self._user
+        )
+        if same_entry and source.name != moved_destination.name:
+            # On case-folding APFS, replacing an existing entry through a case-variant path
+            # updates its content but keeps its old spelling. Moving that same entry performs
+            # the requested case-only rename without touching the committed content.
+            await self._session.mv(source, moved_destination, user=self._user)
+        elif not same_entry:
+            raise ApplyPatchDiffError(
+                message=(
+                    "Move destination was updated, but source and destination no longer identify "
+                    "the same file; source was left untouched"
+                ),
+            )
+
     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/sandboxes/_unix_local_file_ops.py

@@ -191,6 +191,52 @@ def listing(self, path: Path) -> list[dict[str, str | int]]:
         finally:
             os.close(fd)
 
+    def rename(self, source: Path, destination: Path) -> None:
+        """Rename a regular file or symlink, replacing the destination entry.
+
+        This is a rename of the directory entry itself: a symlink leaf is moved, not its
+        target, and a destination that is an existing directory is an error rather than a
+        place to put the source. On a filesystem that folds case, renaming an entry to another
+        spelling of its own name changes the stored spelling.
+        """
+        with (
+            self.parent(source, for_write=True) as (source_fd, source_name),
+            self.parent(destination, for_write=True) as (destination_fd, destination_name),
+        ):
+            mode = os.stat(source_name, dir_fd=source_fd, follow_symlinks=False).st_mode
+            if not (stat.S_ISREG(mode) or stat.S_ISLNK(mode)):
+                raise OSError(
+                    errno.EINVAL, "Move source must be a regular file or symlink", str(source)
+                )
+            os.rename(
+                source_name,
+                destination_name,
+                src_dir_fd=source_fd,
+                dst_dir_fd=destination_fd,
+            )
+
+    def same_file(self, left: Path, right: Path, *, follow_symlinks: bool = True) -> bool:
+        """Return whether two paths name one file, by device and inode.
+
+        The filesystem answers this, not a string comparison: on a volume that folds case, or
+        Unicode normalization, two spellings can be one entry. A path that does not exist is
+        not the same file as anything. With no-follow enabled, symlink leaves return false.
+        """
+        try:
+            with (
+                self.parent(left) as (left_fd, left_name),
+                self.parent(right) as (right_fd, right_name),
+            ):
+                left_stat = os.stat(left_name, dir_fd=left_fd, follow_symlinks=follow_symlinks)
+                right_stat = os.stat(right_name, dir_fd=right_fd, follow_symlinks=follow_symlinks)
+        except FileNotFoundError:
+            return False
+        if not follow_symlinks and (
+            stat.S_ISLNK(left_stat.st_mode) or stat.S_ISLNK(right_stat.st_mode)
+        ):
+            return False
+        return os.path.samestat(left_stat, right_stat)
+
 
 def _entry(path: Path, entry: os.stat_result) -> dict[str, str | int]:
     try:
@@ -240,12 +286,14 @@ def _remove_at(parent_fd: int, name: str, *, recursive: bool) -> None:
 
 def _main() -> None:
     # The application supplies this code and an authorized path directly, never via the workspace.
-    operation, raw_path = sys.argv[1:]
+    operation, *raw_paths = sys.argv[1:]
     files = _FileOps()
-    path = Path(raw_path)
+    paths = [Path(raw_path) for raw_path in raw_paths]
     if operation == "write":
+        (path,) = paths
         files.write(path, cast(io.IOBase, sys.stdin.buffer))
     elif operation == "write_new":
+        (path,) = paths
         try:
             files.write_new(path, cast(io.IOBase, sys.stdin.buffer))
         except FileExistsError:
@@ -254,7 +302,15 @@ def _main() -> None:
         except _IncompleteCreateError:
             sys.exit(_INCOMPLETE_CREATE_EXIT_CODE)
     elif operation == "ls":
+        (path,) = paths
         print(json.dumps(files.listing(path), ensure_ascii=True))
+    elif operation == "rename":
+        source, destination = paths
+        files.rename(source, destination)
+    elif operation == "same_file":
+        left, right = paths
+        follow_symlinks = sys.stdin.buffer.read() != b"0"
+        print(json.dumps(files.same_file(left, right, follow_symlinks=follow_symlinks)))
     else:
         raise ValueError("Unsupported UnixLocal file operation")
 

src/agents/sandbox/sandboxes/unix_local.py

@@ -1071,6 +1071,102 @@ async def write(
         except OSError as e:
             raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e
 
+    async def mv(
+        self,
+        source: Path | str,
+        destination: Path | str,
+        *,
+        user: str | User | None = None,
+    ) -> None:
+        # A rename of the entry, descriptor-relative like every other file operation here,
+        # so the paths validated above are the ones acted on. `os.rename` never puts the
+        # source inside an existing directory the way `mv` does; it fails instead.
+        normalized_source = self._normalize_entry_path(source, for_write=True)
+        normalized_destination = self._normalize_entry_path(destination, for_write=True)
+        command = ("mv", "-f", "--", str(normalized_source), str(normalized_destination))
+        if user is not None:
+            try:
+                result = await self._run_file_operation_as_user(
+                    "rename", normalized_source, normalized_destination, user=user
+                )
+            except OSError as e:
+                raise ExecNonZeroError(
+                    ExecResult(stdout=b"", stderr=str(e).encode("utf-8"), exit_code=1),
+                    command=command,
+                    cause=e,
+                ) from e
+            if result.returncode:
+                raise ExecNonZeroError(
+                    ExecResult(
+                        stdout=result.stdout, stderr=result.stderr, exit_code=result.returncode
+                    ),
+                    command=command,
+                )
+            return
+        try:
+            self._files.rename(normalized_source, normalized_destination)
+        except OSError as e:
+            raise ExecNonZeroError(
+                ExecResult(stdout=b"", stderr=str(e).encode("utf-8"), exit_code=1),
+                command=command,
+                cause=e,
+            ) from e
+
+    async def same_file(
+        self,
+        left: Path | str,
+        right: Path | str,
+        *,
+        follow_symlinks: bool = True,
+        user: str | User | None = None,
+    ) -> bool:
+        normalize = self.normalize_path if follow_symlinks else self._normalize_entry_path
+        normalized_left = normalize(left)
+        normalized_right = normalize(right)
+        command = ("test", str(normalized_left), "-ef", str(normalized_right))
+        if user is not None:
+            try:
+                result = await self._run_file_operation_as_user(
+                    "same_file",
+                    normalized_left,
+                    normalized_right,
+                    user=user,
+                    payload=b"1" if follow_symlinks else b"0",
+                )
+            except OSError as e:
+                raise ExecNonZeroError(
+                    ExecResult(stdout=b"", stderr=str(e).encode("utf-8"), exit_code=1),
+                    command=command,
+                    cause=e,
+                ) from e
+            if result.returncode:
+                raise ExecNonZeroError(
+                    ExecResult(
+                        stdout=result.stdout, stderr=result.stderr, exit_code=result.returncode
+                    ),
+                    command=command,
+                )
+            return bool(json.loads(result.stdout))
+        try:
+            return self._files.same_file(
+                normalized_left, normalized_right, follow_symlinks=follow_symlinks
+            )
+        except OSError as e:
+            raise ExecNonZeroError(
+                ExecResult(stdout=b"", stderr=str(e).encode("utf-8"), exit_code=1),
+                command=command,
+                cause=e,
+            ) from e
+
+    def _normalize_entry_path(self, path: Path | str, *, for_write: bool = False) -> Path:
+        # Resolve and authorize the parent without following the leaf that rename/stat owns.
+        self._files.configure(self.state.manifest)
+        lexical = Path(path)
+        if not lexical.is_absolute():
+            lexical = self._workspace_path_policy().absolute_workspace_path(path)
+        resolved = lexical.parent.resolve(strict=False) / lexical.name
+        return self._files.authorize(resolved, for_write=for_write)
+
     async def _write_new_file(
         self,
         path: Path,
@@ -1167,22 +1263,23 @@ async def _write_stream_with_exec(
 
     async def _run_file_operation_as_user(
         self,
-        operation: Literal["ls", "write", "write_new"],
+        operation: Literal["ls", "write", "write_new", "rename", "same_file"],
         path: Path,
-        *,
+        *more_paths: Path,
         user: str | User,
         payload: bytes = b"",
     ) -> subprocess.CompletedProcess[bytes]:
         # Authorization is synchronous and captured for this operation before dispatch.
-        path = self._files.authorize(path, for_write=operation != "ls")
+        for_write = operation in ("write", "write_new", "rename")
+        paths = [self._files.authorize(each, for_write=for_write) for each in (path, *more_paths)]
         command = self._prepare_exec_command(
             "python3",
             "-I",
             "-S",
             "-c",
             _USER_FILE_WORKER_SOURCE,
             operation,
-            str(path),
+            *(str(each) for each in paths),
             shell=False,
             user=user,
         )