From 6d822b39373f1877577830aad9db6e035b1337bd Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Sun, 6 Sep 2026 21:22:06 +0800 Subject: [PATCH 01/12] fix: keep the file when apply_patch does a case-only rename On a filesystem that folds case, `notes.txt` and `Notes.txt` are the same file. `_apply_update` wrote the new text to the destination and then removed the source, and because the path comparison is case-sensitive the removal deleted the file that had just been written. The user's edit was lost. For a case-only rename, remove the source before writing instead. That end state is correct on a folding filesystem and on a case-sensitive one, so the SDK does not have to know which kind it is talking to. The write is wrapped so a failure restores the original text at the source path. Co-Authored-By: Claude Opus 5 --- src/agents/sandbox/apply_patch.py | 51 +++++++++++++- tests/sandbox/_apply_patch_test_session.py | 80 +++++++++++++++++++++- tests/sandbox/test_apply_patch.py | 61 ++++++++++++++++- 3 files changed, 188 insertions(+), 4 deletions(-) diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 30623fdf82..a5c1c62ef8 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -1,5 +1,6 @@ from __future__ import annotations +import contextlib import io from pathlib import Path from typing import TYPE_CHECKING, Any, Literal, Protocol, cast, runtime_checkable @@ -111,9 +112,23 @@ 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: + # A sandbox filesystem that folds case stores notes.txt and Notes.txt as one file, so + # removing the source after the write would delete the text that was just written. + # Removing the source first renames the file on a case-folding filesystem and on a + # case-sensitive one. Every other rename keeps the write-then-remove order so a failed + # write leaves the source intact. + if _is_case_only_rename(destination, moved_destination): await self._session.rm(destination, user=self._user) + await self._write_moved_text( + moved_destination, + updated_text, + restore_destination=destination, + restore_text=original_text, + ) + else: + await self._write_text(moved_destination, updated_text) + if moved_destination != destination: + await self._session.rm(destination, user=self._user) return ApplyPatchResult( output=f"Updated {display_path}\nMoved {display_path} to {moved_display_path}" ) @@ -209,6 +224,28 @@ async def _read_text(self, destination: Path, *, op_path: str, decode_path: Path path=op_path, ) + async def _write_moved_text( + self, + destination: Path, + text: str, + *, + restore_destination: Path, + restore_text: str, + ) -> None: + """Write a rename destination whose source was already removed, restoring it on failure. + + The source is gone while this write is in flight, so a sandbox write that fails partway + through, for example on a dropped connection to a remote session, would otherwise leave + the file in neither place. The original text is still in memory, so put it back before + the failure propagates. + """ + try: + await self._write_text(destination, text) + except BaseException: + with contextlib.suppress(Exception): + await self._write_text(restore_destination, restore_text) + raise + 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( @@ -218,6 +255,16 @@ async def _write_text(self, destination: Path, text: str) -> None: ) +def _is_case_only_rename(source: Path, destination: Path) -> bool: + """Return whether two sandbox paths are distinct and differ only in character case.""" + + source_posix = source.as_posix() + destination_posix = destination.as_posix() + if source_posix == destination_posix: + return False + return source_posix.casefold() == destination_posix.casefold() + + def _coerce_operations( operations: ApplyPatchOperation | dict[str, object] diff --git a/tests/sandbox/_apply_patch_test_session.py b/tests/sandbox/_apply_patch_test_session.py index 24ce567011..737b1f9204 100644 --- a/tests/sandbox/_apply_patch_test_session.py +++ b/tests/sandbox/_apply_patch_test_session.py @@ -2,7 +2,8 @@ import io import uuid -from pathlib import Path +from pathlib import Path, PurePosixPath +from typing import cast from agents.sandbox import Manifest from agents.sandbox.errors import WorkspaceReadNotFoundError @@ -94,6 +95,83 @@ async def rm( self.files.pop(normalized, None) +class PosixHostApplyPatchSession(ApplyPatchSession): + """An apply_patch session whose workspace paths compare case-sensitively on every host. + + Linux and macOS hosts compare sandbox paths case-sensitively, while a Windows host folds + case in `Path` comparisons. `PurePosixPath` keeps the host comparison case-sensitive + everywhere so case-only rename coverage does not depend on the operating system that runs + the tests. + """ + + def normalize_path(self, path: Path | str, *, for_write: bool = False) -> Path: + normalized = super().normalize_path(path, for_write=for_write) + return cast(Path, PurePosixPath(normalized.as_posix())) + + +class CaseFoldingApplyPatchSession(PosixHostApplyPatchSession): + """A case-sensitive host over a sandbox filesystem that folds path case. + + APFS, NTFS, and Docker bind mounts backed by either store `notes.txt` and `Notes.txt` as + one file, and they preserve the case of the name that created the file. Lookups here fold + case so an existing entry keeps its stored name when it is written again. + """ + + def _stored_path(self, path: Path | str) -> Path: + normalized = self.normalize_path(path) + folded = normalized.as_posix().casefold() + for stored in self.files: + if stored.as_posix().casefold() == folded: + return stored + return normalized + + async def read(self, path: Path, *, user: str | User | None = None) -> io.BytesIO: + return await super().read(self._stored_path(path), user=user) + + async def write( + self, + path: Path, + data: io.IOBase, + *, + user: str | User | None = None, + ) -> None: + await super().write(self._stored_path(path), data, user=user) + + async def rm( + self, + path: Path | str, + *, + recursive: bool = False, + user: str | User | None = None, + ) -> None: + await super().rm(self._stored_path(path), recursive=recursive, user=user) + + +class WriteFailureApplyPatchSession(CaseFoldingApplyPatchSession): + """A case-folding session whose first write fails, as a dropped sandbox connection would. + + A case-only rename removes the source before it writes the destination, so the file exists + in neither place while that write is in flight. Failing only the first write leaves the + restoring write able to succeed. + """ + + def __init__(self, manifest: Manifest | None = None) -> None: + super().__init__(manifest) + self.fail_next_write = True + + async def write( + self, + path: Path, + data: io.IOBase, + *, + user: str | User | None = None, + ) -> None: + if self.fail_next_write: + self.fail_next_write = False + raise ConnectionError("sandbox write failed") + await super().write(path, data, user=user) + + class ProviderNotFoundApplyPatchSession(ApplyPatchSession): async def read(self, path: Path, *, user: str | User | None = None) -> io.BytesIO: try: diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index c4cd676fec..8106bf5127 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -1,6 +1,7 @@ from __future__ import annotations -from pathlib import Path +from pathlib import Path, PurePosixPath +from typing import cast import pytest @@ -14,7 +15,10 @@ ) from tests.sandbox._apply_patch_test_session import ( ApplyPatchSession, + CaseFoldingApplyPatchSession, + PosixHostApplyPatchSession, ProviderNotFoundApplyPatchSession, + WriteFailureApplyPatchSession, ) @@ -247,6 +251,61 @@ async def test_apply_patch_normalizes_backslashes_in_move_to() -> None: assert Path("/workspace/source.txt") not in session.files +@pytest.mark.asyncio +async def test_apply_patch_case_only_move_to_keeps_file_on_case_folding_filesystem() -> None: + """A case-folding filesystem stores both names as one file, which the removal must keep.""" + session = CaseFoldingApplyPatchSession() + session.files[cast(Path, PurePosixPath("/workspace/notes.txt"))] = b"alpha\nbeta\n" + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="Notes.txt", + ) + ) + + assert session.files == {PurePosixPath("/workspace/Notes.txt"): b"alpha\ngamma\n"} + + +@pytest.mark.asyncio +async def test_apply_patch_case_only_move_to_moves_file_on_case_sensitive_filesystem() -> None: + """A case-sensitive filesystem keeps the names apart, so the source must still be removed.""" + session = PosixHostApplyPatchSession() + session.files[cast(Path, PurePosixPath("/workspace/notes.txt"))] = b"alpha\nbeta\n" + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="Notes.txt", + ) + ) + + assert session.files == {PurePosixPath("/workspace/Notes.txt"): b"alpha\ngamma\n"} + + +@pytest.mark.asyncio +async def test_apply_patch_case_only_move_to_restores_source_when_write_fails() -> None: + """The source is removed before the write, so a failed write must put the file back.""" + session = WriteFailureApplyPatchSession() + session.files[cast(Path, PurePosixPath("/workspace/notes.txt"))] = b"alpha\nbeta\n" + + with pytest.raises(ConnectionError): + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="Notes.txt", + ) + ) + + assert session.files == {PurePosixPath("/workspace/notes.txt"): b"alpha\nbeta\n"} + + @pytest.mark.asyncio async def test_apply_patch_allows_absolute_path_within_root() -> None: session = ApplyPatchSession() From 43e24638b61f0831ff9fd0ae1d9bb8ce35e4a8c6 Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:23:56 +0800 Subject: [PATCH 02/12] fix: commit the rename before removing the source The previous commit removed the source first on a case-only rename and restored the original text from memory if the write then failed. That trades one way to lose the file for another: if the write and the restore both fail the file is gone, and a restore that lands late overwrites whatever another writer put at the source path in the meantime. Ask the filesystem instead of comparing strings. `same_file` runs `[ "$1" -ef "$2" ]`, which compares device and inode, so it answers for a filesystem that folds case and for one that folds Unicode normalisation, which `str.casefold` cannot. `mv` renames a path and refuses a directory destination, because `mv` given a directory moves the source inside it and exits 0, which for a caller that then removes the source is a way to delete a file while believing it moved. The rename now writes a staging file, commits it onto the destination with one move, and removes the source only when the filesystem says it is a different file. Nothing is removed before the new content is on disk, and nothing is written back after a failure. Three of the new tests run on the macOS runner against a real case-folding volume, so the behaviour is no longer only modelled. Co-Authored-By: Claude Opus 5 --- docs/testing.md | 2 +- src/agents/sandbox/apply_patch.py | 68 ++++----- .../sandbox/session/base_sandbox_session.py | 83 +++++++++++ src/agents/sandbox/session/sandbox_session.py | 18 +++ src/agents/testing/sandbox.py | 20 +++ tests/sandbox/_apply_patch_test_session.py | 137 +++++++++++++++++- tests/sandbox/test_apply_patch.py | 110 +++++++++++++- tests/sandbox/test_unix_local.py | 84 ++++++++++- 8 files changed, 473 insertions(+), 49 deletions(-) diff --git a/docs/testing.md b/docs/testing.md index 57e5064615..d7cab034af 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -286,7 +286,7 @@ Each matching Sandbox call consumes the next step in one global FIFO sequence. A | `error` | The method should raise a specific exception | | `match` | The call should be rejected before producing its outcome unless the matcher returns a value other than `False` | -The supported scripted method names are `apply_patch`, `exec`, `ls`, `mkdir`, `pty_exec_start`, `pty_write_stdin`, `read`, `rm`, and `write`. Only configured model-facing capabilities are exposed. The two PTY methods are exposed together when either PTY method is configured because they form one interactive-shell capability, but calls still consume the global FIFO script. +The supported scripted method names are `apply_patch`, `exec`, `ls`, `mkdir`, `mv`, `pty_exec_start`, `pty_write_stdin`, `read`, `rm`, `same_file`, and `write`. Only configured model-facing capabilities are exposed. The two PTY methods are exposed together when either PTY method is configured because they form one interactive-shell capability, but calls still consume the global FIFO script. `sandbox.calls` contains detached `SandboxCall` snapshots with zero-based `call_index`, `method`, positional `args`, and read-only `kwargs`. Static results are also snapshotted when the script is created. `io.BytesIO` and `io.StringIO` values are supported; use a custom Sandbox session for other live stream objects or lifecycle behavior. diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index a5c1c62ef8..a6ade970fb 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -4,6 +4,7 @@ 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 @@ -112,23 +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) - # A sandbox filesystem that folds case stores notes.txt and Notes.txt as one file, so - # removing the source after the write would delete the text that was just written. - # Removing the source first renames the file on a case-folding filesystem and on a - # case-sensitive one. Every other rename keeps the write-then-remove order so a failed - # write leaves the source intact. - if _is_case_only_rename(destination, moved_destination): - await self._session.rm(destination, user=self._user) - await self._write_moved_text( - moved_destination, - updated_text, - restore_destination=destination, - restore_text=original_text, - ) - else: - 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}" ) @@ -224,27 +213,36 @@ async def _read_text(self, destination: Path, *, op_path: str, decode_path: Path path=op_path, ) - async def _write_moved_text( + async def _move_updated_text( self, - destination: Path, - text: str, *, - restore_destination: Path, - restore_text: str, + source: Path, + moved_destination: Path, + text: str, ) -> None: - """Write a rename destination whose source was already removed, restoring it on failure. - - The source is gone while this write is in flight, so a sandbox write that fails partway - through, for example on a dropped connection to a remote session, would otherwise leave - the file in neither place. The original text is still in memory, so put it back before - the failure propagates. + """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 file. 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. """ + staging = moved_destination.with_name(f".{moved_destination.name}.{uuid4().hex[:8]}.tmp") + await self._write_text(staging, text) try: - await self._write_text(destination, text) + await self._session.mv(staging, moved_destination, user=self._user) except BaseException: with contextlib.suppress(Exception): - await self._write_text(restore_destination, restore_text) + await self._session.rm(staging, user=self._user) raise + if not await self._session.same_file(source, moved_destination, 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) @@ -255,16 +253,6 @@ async def _write_text(self, destination: Path, text: str) -> None: ) -def _is_case_only_rename(source: Path, destination: Path) -> bool: - """Return whether two sandbox paths are distinct and differ only in character case.""" - - source_posix = source.as_posix() - destination_posix = destination.as_posix() - if source_posix == destination_posix: - return False - return source_posix.casefold() == destination_posix.casefold() - - def _coerce_operations( operations: ApplyPatchOperation | dict[str, object] diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index d377bea9ef..19b16ce79f 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -1146,6 +1146,89 @@ 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", "", source_arg, destination_arg) + ) + + async def same_file( + self, + left: Path | str, + right: Path | str, + *, + 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. + + :param left: First path to compare. + :param right: Second path to compare. + :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) + cmd = ("sh", "-lc", '[ "$1" -ef "$2" ]', "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", "", left_arg, right_arg) + ) + async def mkdir( self, path: Path | str, diff --git a/src/agents/sandbox/session/sandbox_session.py b/src/agents/sandbox/session/sandbox_session.py index 923f025857..5d78fa67cc 100644 --- a/src/agents/sandbox/session/sandbox_session.py +++ b/src/agents/sandbox/session/sandbox_session.py @@ -641,6 +641,24 @@ 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, + *, + user: str | User | None = None, + ) -> bool: + return await self._inner.same_file(left, right, user=user) + async def mkdir( self, path: Path | str, diff --git a/src/agents/testing/sandbox.py b/src/agents/testing/sandbox.py index d086ae7c9d..dfd674181c 100644 --- a/src/agents/testing/sandbox.py +++ b/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,24 @@ 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, + *, + user: str | User | None = None, + ) -> bool: + return cast(bool, await self._invoke("same_file", (left, right), {"user": user})) + async def apply_patch( self, operations: ApplyPatchOperation diff --git a/tests/sandbox/_apply_patch_test_session.py b/tests/sandbox/_apply_patch_test_session.py index 737b1f9204..9ddf096f1a 100644 --- a/tests/sandbox/_apply_patch_test_session.py +++ b/tests/sandbox/_apply_patch_test_session.py @@ -1,6 +1,7 @@ from __future__ import annotations import io +import unicodedata import uuid from pathlib import Path, PurePosixPath from typing import cast @@ -22,6 +23,16 @@ def __init__(self, manifest: Manifest | None = None) -> None: self.files: dict[Path, bytes] = {} self.mkdir_calls: list[tuple[Path, bool]] = [] self.rm_calls: list[tuple[Path, bool]] = [] + self.mv_calls: list[tuple[Path, Path]] = [] + self.directories: set[Path] = set() + + def _stored_path(self, path: Path | str) -> Path: + """Return the key this store holds `path` under. + + A store that folds names overrides this. Here every distinct spelling is a distinct + file, which is what a case-sensitive filesystem does. + """ + return self.normalize_path(path) async def start(self) -> None: return None @@ -94,6 +105,39 @@ async def rm( self.rm_calls.append((normalized, recursive)) self.files.pop(normalized, None) + async def mv( + self, + source: Path | str, + destination: Path | str, + *, + user: str | User | None = None, + ) -> None: + _ = user + if self.normalize_path(destination) in self.directories: + # A real `mv` would move the source inside this directory and report success. + raise IsADirectoryError(self.normalize_path(destination)) + stored_source = self._stored_path(source) + if stored_source not in self.files: + raise FileNotFoundError(stored_source) + payload = self.files.pop(stored_source) + # Look the destination up after removing the source, so a case-only rename does not + # find the entry it is renaming. A real `mv` replaces whatever is at the destination + # and stores the name it was given, which is how a case-only rename changes the case. + self.files.pop(self._stored_path(destination), None) + normalized_destination = self.normalize_path(destination) + self.files[normalized_destination] = payload + self.mv_calls.append((stored_source, normalized_destination)) + + async def same_file( + self, + left: Path | str, + right: Path | str, + *, + user: str | User | None = None, + ) -> bool: + _ = user + return self._stored_path(left) == self._stored_path(right) + class PosixHostApplyPatchSession(ApplyPatchSession): """An apply_patch session whose workspace paths compare case-sensitively on every host. @@ -147,14 +191,47 @@ async def rm( await super().rm(self._stored_path(path), recursive=recursive, user=user) -class WriteFailureApplyPatchSession(CaseFoldingApplyPatchSession): - """A case-folding session whose first write fails, as a dropped sandbox connection would. +class NormalizationFoldingApplyPatchSession(PosixHostApplyPatchSession): + """A case-sensitive host over a filesystem that folds case and Unicode normalization. - A case-only rename removes the source before it writes the destination, so the file exists - in neither place while that write is in flight. Failing only the first write leaves the - restoring write able to succeed. + This is APFS. It stores one entry for the decomposed and the composed spelling of the same + accented name, as well as for `notes.txt` and `Notes.txt`. `str.casefold` answers the first + pair wrong, which is one reason the fix may not ask a string whether two paths are one file. """ + def _stored_path(self, path: Path | str) -> Path: + normalized = self.normalize_path(path) + folded = unicodedata.normalize("NFC", normalized.as_posix()).casefold() + for stored in self.files: + if unicodedata.normalize("NFC", stored.as_posix()).casefold() == folded: + return stored + return normalized + + async def read(self, path: Path, *, user: str | User | None = None) -> io.BytesIO: + return await ApplyPatchSession.read(self, self._stored_path(path), user=user) + + async def write( + self, + path: Path, + data: io.IOBase, + *, + user: str | User | None = None, + ) -> None: + await ApplyPatchSession.write(self, self._stored_path(path), data, user=user) + + async def rm( + self, + path: Path | str, + *, + recursive: bool = False, + user: str | User | None = None, + ) -> None: + await ApplyPatchSession.rm(self, self._stored_path(path), recursive=recursive, user=user) + + +class WriteFailureApplyPatchSession(CaseFoldingApplyPatchSession): + """A case-folding session whose first write fails, as a dropped sandbox connection would.""" + def __init__(self, manifest: Manifest | None = None) -> None: super().__init__(manifest) self.fail_next_write = True @@ -172,6 +249,34 @@ async def write( await super().write(path, data, user=user) +class ConcurrentWriterApplyPatchSession(PosixHostApplyPatchSession): + """A case-sensitive session where another writer takes the source path and the move fails. + + The rename is committed by a move. This session lets the staging write land, then has a + second writer put its own file at the source path, then fails the move. The operation + cannot succeed from here. What it must not do is put the original text back over the file + that other writer just created. + """ + + def __init__(self, manifest: Manifest | None = None) -> None: + super().__init__(manifest) + self.concurrent_source: Path | None = None + self.concurrent_text = "written by someone else\n" + + async def mv( + self, + source: Path | str, + destination: Path | str, + *, + user: str | User | None = None, + ) -> None: + if self.concurrent_source is not None: + self.files[self.normalize_path(self.concurrent_source)] = self.concurrent_text.encode( + "utf-8" + ) + raise ConnectionError("sandbox move failed") + + class ProviderNotFoundApplyPatchSession(ApplyPatchSession): async def read(self, path: Path, *, user: str | User | None = None) -> io.BytesIO: try: @@ -190,6 +295,8 @@ def __init__(self, manifest: Manifest | None = None) -> None: self.write_users: list[str | None] = [] self.mkdir_users: list[str | None] = [] self.rm_users: list[str | None] = [] + self.mv_users: list[str | None] = [] + self.same_file_users: list[str | None] = [] @staticmethod def _user_name(user: str | User | None) -> str | None: @@ -228,3 +335,23 @@ async def rm( ) -> None: self.rm_users.append(self._user_name(user)) await super().rm(path, recursive=recursive) + + async def mv( + self, + source: Path | str, + destination: Path | str, + *, + user: str | User | None = None, + ) -> None: + self.mv_users.append(self._user_name(user)) + await super().mv(source, destination) + + async def same_file( + self, + left: Path | str, + right: Path | str, + *, + user: str | User | None = None, + ) -> bool: + self.same_file_users.append(self._user_name(user)) + return await super().same_file(left, right) diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index 8106bf5127..886515163a 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -16,6 +16,8 @@ from tests.sandbox._apply_patch_test_session import ( ApplyPatchSession, CaseFoldingApplyPatchSession, + ConcurrentWriterApplyPatchSession, + NormalizationFoldingApplyPatchSession, PosixHostApplyPatchSession, ProviderNotFoundApplyPatchSession, WriteFailureApplyPatchSession, @@ -288,8 +290,8 @@ async def test_apply_patch_case_only_move_to_moves_file_on_case_sensitive_filesy @pytest.mark.asyncio -async def test_apply_patch_case_only_move_to_restores_source_when_write_fails() -> None: - """The source is removed before the write, so a failed write must put the file back.""" +async def test_apply_patch_move_to_leaves_the_source_alone_when_the_write_fails() -> None: + """Nothing is removed until the replacement is committed, so a failed write changes nothing.""" session = WriteFailureApplyPatchSession() session.files[cast(Path, PurePosixPath("/workspace/notes.txt"))] = b"alpha\nbeta\n" @@ -304,6 +306,110 @@ async def test_apply_patch_case_only_move_to_restores_source_when_write_fails() ) assert session.files == {PurePosixPath("/workspace/notes.txt"): b"alpha\nbeta\n"} + # The file surviving is not enough. The refused implementation removed the source and then + # wrote it back, which also ends here. Nothing may be removed at all. + assert session.rm_calls == [] + + +@pytest.mark.asyncio +async def test_apply_patch_move_to_does_not_overwrite_a_concurrent_writer_after_a_failure() -> None: + """A failed move must not restore the original over a file another writer just created. + + The operation cannot finish once the move fails. The question is what it leaves behind. An + implementation that kept the original text in memory and wrote it back at the source path + would destroy whatever arrived there in the meantime. + """ + session = ConcurrentWriterApplyPatchSession() + source = cast(Path, PurePosixPath("/workspace/notes.txt")) + session.files[source] = b"alpha\nbeta\n" + session.concurrent_source = source + + with pytest.raises(ConnectionError): + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="Notes.txt", + ) + ) + + assert session.files[source] == b"written by someone else\n" + assert PurePosixPath("/workspace/Notes.txt") not in session.files + assert not [path for path in session.files if path.name.endswith(".tmp")] + + +@pytest.mark.asyncio +async def test_apply_patch_move_to_keeps_the_file_when_only_unicode_normalization_changes() -> None: + """APFS folds NFC against NFD, so the two spellings of one accented name are one file. + + `str.casefold` does not normalize, so any fix that compares folded strings sends this pair + down the path that destroys it. Asking the filesystem covers it without naming the case. + """ + session = NormalizationFoldingApplyPatchSession() + decomposed = "/workspace/cafe\u0301.txt" + composed = "/workspace/caf\u00e9.txt" + session.files[cast(Path, PurePosixPath(decomposed))] = b"alpha\nbeta\n" + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path=decomposed, + diff="@@\n alpha\n-beta\n+gamma\n", + move_to=composed, + ) + ) + + assert session.files == {PurePosixPath(composed): b"alpha\ngamma\n"} + + +@pytest.mark.asyncio +async def test_apply_patch_move_to_an_existing_directory_keeps_the_source() -> None: + """`mv` moves a file into a directory destination and calls that success. + + The source would then be removed on the strength of that success, and the operation would + report a move that did not happen. `move_to` comes from the model, so this is reachable. + """ + session = PosixHostApplyPatchSession() + source = cast(Path, PurePosixPath("/workspace/notes.txt")) + session.files[source] = b"alpha\nbeta\n" + session.directories.add(cast(Path, PurePosixPath("/workspace/docs"))) + + with pytest.raises(IsADirectoryError): + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="docs", + ) + ) + + assert session.files[source] == b"alpha\nbeta\n" + assert not [path for path in session.files if path.name.endswith(".tmp")] + + +@pytest.mark.asyncio +async def test_apply_patch_move_to_commits_the_destination_before_removing_the_source() -> None: + """The order is the fix. Assert it directly, so a future reordering fails here.""" + session = PosixHostApplyPatchSession() + session.files[cast(Path, PurePosixPath("/workspace/notes.txt"))] = b"alpha\nbeta\n" + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="Notes.txt", + ) + ) + + assert len(session.mv_calls) == 1 + staging, moved_to = session.mv_calls[0] + assert staging.parent == PurePosixPath("/workspace") + assert staging.name.endswith(".tmp") + assert moved_to == PurePosixPath("/workspace/Notes.txt") + assert session.rm_calls == [(cast(Path, PurePosixPath("/workspace/notes.txt")), False)] @pytest.mark.asyncio diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index 9188b1fc33..55f3848b62 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -12,8 +12,9 @@ import pytest +from agents.editor import ApplyPatchOperation from agents.sandbox import SandboxPathGrant -from agents.sandbox.errors import PtySessionNotFoundError +from agents.sandbox.errors import ExecNonZeroError, PtySessionNotFoundError from agents.sandbox.manifest import Environment, Manifest from agents.sandbox.sandboxes import unix_local as unix_local_module from agents.sandbox.sandboxes.unix_local import ( @@ -610,3 +611,84 @@ def _slow_extract(tar: object, **kwargs: object) -> None: # the workspace root are only released once nothing is still writing to them. assert events == ["extract-start", "extract-end"] assert not buf.closed + + +class TestUnixLocalApplyPatchRename: + """apply_patch renames against a real filesystem, not a model of one. + + Every other test of this behaviour drives a session double. A double can only be wrong in + the same direction as the code it was written beside. The default macOS volume folds case, + so on the macOS runner these exercise the case that loses the file. + """ + + @pytest.mark.asyncio + @pytest.mark.requires_native_macos_sandbox + async def test_case_only_move_to_keeps_the_file(self, tmp_path: Path) -> None: + workspace = tmp_path / "workspace" + client = UnixLocalSandboxClient() + manifest = Manifest(root=str(workspace)) + + async with await client.create(manifest=manifest, snapshot=None, options=None) as session: + await session.write(Path("notes.txt"), io.BytesIO(b"alpha\nbeta\n")) + + source = workspace / "notes.txt" + destination = workspace / "Notes.txt" + if not await session.same_file(source, destination): + pytest.skip("this volume does not fold case, so it cannot exercise the bug") + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="Notes.txt", + ) + ) + + names = sorted(entry.name for entry in workspace.iterdir()) + assert names == ["Notes.txt"] + assert destination.read_bytes() == b"alpha\ngamma\n" + + @pytest.mark.asyncio + @pytest.mark.requires_native_macos_sandbox + async def test_move_to_an_existing_directory_keeps_the_source(self, tmp_path: Path) -> None: + """The guard against a directory destination is a shell test, so run a real shell. + + `mv` given an existing directory moves the source inside it and exits 0. A caller that + removes the source on that exit code deletes the file. The unit tests model this; this + one runs it. + """ + workspace = tmp_path / "workspace" + client = UnixLocalSandboxClient() + manifest = Manifest(root=str(workspace)) + + async with await client.create(manifest=manifest, snapshot=None, options=None) as session: + await session.write(Path("notes.txt"), io.BytesIO(b"alpha\nbeta\n")) + await session.mkdir(Path("docs")) + + with pytest.raises(ExecNonZeroError): + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="docs", + ) + ) + + assert (workspace / "notes.txt").read_bytes() == b"alpha\nbeta\n" + assert sorted(entry.name for entry in (workspace / "docs").iterdir()) == [] + + @pytest.mark.asyncio + @pytest.mark.requires_native_macos_sandbox + async def test_same_file_answers_for_real_paths(self, tmp_path: Path) -> None: + workspace = tmp_path / "workspace" + client = UnixLocalSandboxClient() + manifest = Manifest(root=str(workspace)) + + async with await client.create(manifest=manifest, snapshot=None, options=None) as session: + await session.write(Path("one.txt"), io.BytesIO(b"one\n")) + await session.write(Path("two.txt"), io.BytesIO(b"two\n")) + + assert await session.same_file(workspace / "one.txt", workspace / "one.txt") is True + assert await session.same_file(workspace / "one.txt", workspace / "two.txt") is False From 5f9dc66cdbe9c77c0719015b0195cea950fea563 Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Tue, 8 Sep 2026 02:51:31 +0800 Subject: [PATCH 03/12] fix: answer identity without following symlinks, and stage safely Acts on five of the six findings the Codex reviewer raised on 5b3650ca. `test -ef` follows symlinks, so a source symlink pointing at the destination answered "same file" and the removal was skipped, leaving the old name pointing at the new one. `same_file` takes `follow_symlinks` now, and the editor asks with it off, because the answer decides whether removing one path destroys the other. The staging write moves inside the `try`, so a write that fails after creating the file no longer orphans it. The staging basename is a fixed length, because decorating a destination basename near the 255-byte limit overflowed it. A `move_to` naming the path the file already has short-circuits to an in-place write, which is what an update without `move_to` does and what this code did before the rename was staged. `docs/testing.md` is reverted: AGENTS.md keeps documentation for unreleased behaviour out of the pull request that introduces it. The directory check is still not atomic with the rename. Closing that needs a per-backend rename primitive, which is a question for the maintainer. Co-Authored-By: Claude Opus 5 --- docs/testing.md | 2 +- src/agents/sandbox/apply_patch.py | 27 ++++- .../sandbox/session/base_sandbox_session.py | 13 ++- src/agents/sandbox/session/sandbox_session.py | 3 +- src/agents/testing/sandbox.py | 10 +- tests/sandbox/_apply_patch_test_session.py | 22 +++- tests/sandbox/test_apply_patch.py | 102 +++++++++++++++++- tests/sandbox/test_unix_local.py | 25 +++++ 8 files changed, 192 insertions(+), 12 deletions(-) diff --git a/docs/testing.md b/docs/testing.md index d7cab034af..57e5064615 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -286,7 +286,7 @@ Each matching Sandbox call consumes the next step in one global FIFO sequence. A | `error` | The method should raise a specific exception | | `match` | The call should be rejected before producing its outcome unless the matcher returns a value other than `False` | -The supported scripted method names are `apply_patch`, `exec`, `ls`, `mkdir`, `mv`, `pty_exec_start`, `pty_write_stdin`, `read`, `rm`, `same_file`, and `write`. Only configured model-facing capabilities are exposed. The two PTY methods are exposed together when either PTY method is configured because they form one interactive-shell capability, but calls still consume the global FIFO script. +The supported scripted method names are `apply_patch`, `exec`, `ls`, `mkdir`, `pty_exec_start`, `pty_write_stdin`, `read`, `rm`, and `write`. Only configured model-facing capabilities are exposed. The two PTY methods are exposed together when either PTY method is configured because they form one interactive-shell capability, but calls still consume the global FIFO script. `sandbox.calls` contains detached `SandboxCall` snapshots with zero-based `call_index`, `method`, positional `args`, and read-only `kwargs`. Static results are also snapshotted when the script is created. `io.BytesIO` and `io.StringIO` values are supported; use a custom Sandbox session for other live stream objects or lifecycle behavior. diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index a6ade970fb..0d8c5d2253 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -228,20 +228,39 @@ async def _move_updated_text( 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 file. Before + 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. + + 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. """ - staging = moved_destination.with_name(f".{moved_destination.name}.{uuid4().hex[:8]}.tmp") - await self._write_text(staging, text) + 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 - if not await self._session.same_file(source, moved_destination, user=self._user): + # 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: diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index 19b16ce79f..d0aa5e63c9 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -1194,6 +1194,7 @@ async def same_file( 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. @@ -1205,8 +1206,15 @@ async def same_file( 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. """ @@ -1215,7 +1223,10 @@ async def same_file( left_arg = sandbox_path_str(left) right_arg = sandbox_path_str(right) - cmd = ("sh", "-lc", '[ "$1" -ef "$2" ]', "sh", left_arg, right_arg) + 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 diff --git a/src/agents/sandbox/session/sandbox_session.py b/src/agents/sandbox/session/sandbox_session.py index 5d78fa67cc..c6d5a1df6b 100644 --- a/src/agents/sandbox/session/sandbox_session.py +++ b/src/agents/sandbox/session/sandbox_session.py @@ -655,9 +655,10 @@ async def same_file( left: Path | str, right: Path | str, *, + follow_symlinks: bool = True, user: str | User | None = None, ) -> bool: - return await self._inner.same_file(left, right, user=user) + return await self._inner.same_file(left, right, follow_symlinks=follow_symlinks, user=user) async def mkdir( self, diff --git a/src/agents/testing/sandbox.py b/src/agents/testing/sandbox.py index dfd674181c..919f0c88ce 100644 --- a/src/agents/testing/sandbox.py +++ b/src/agents/testing/sandbox.py @@ -505,9 +505,17 @@ async def same_file( 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), {"user": user})) + return cast( + bool, + await self._invoke( + "same_file", + (left, right), + {"follow_symlinks": follow_symlinks, "user": user}, + ), + ) async def apply_patch( self, diff --git a/tests/sandbox/_apply_patch_test_session.py b/tests/sandbox/_apply_patch_test_session.py index 9ddf096f1a..6e660b716f 100644 --- a/tests/sandbox/_apply_patch_test_session.py +++ b/tests/sandbox/_apply_patch_test_session.py @@ -24,6 +24,8 @@ def __init__(self, manifest: Manifest | None = None) -> None: self.mkdir_calls: list[tuple[Path, bool]] = [] self.rm_calls: list[tuple[Path, bool]] = [] self.mv_calls: list[tuple[Path, Path]] = [] + # Link path -> target path, for the paths a test declares to be symlinks. + self.symlinks: dict[Path, Path] = {} self.directories: set[Path] = set() def _stored_path(self, path: Path | str) -> Path: @@ -133,10 +135,25 @@ async def same_file( left: Path | str, right: Path | str, *, + follow_symlinks: bool = True, user: str | User | None = None, ) -> bool: _ = user - return self._stored_path(left) == self._stored_path(right) + if not follow_symlinks and ( + self._stored_path(left) in self.symlinks or self._stored_path(right) in self.symlinks + ): + return False + return self._resolved_path(left) == self._resolved_path(right) + + def _resolved_path(self, path: Path | str) -> Path: + # `-ef` resolves the whole chain, so a fake that follows one hop would answer False + # where a real filesystem answers True. The seen set stops a cycle. + stored = self._stored_path(path) + seen: set[Path] = set() + while stored in self.symlinks and stored not in seen: + seen.add(stored) + stored = self._stored_path(self.symlinks[stored]) + return stored class PosixHostApplyPatchSession(ApplyPatchSession): @@ -351,7 +368,8 @@ async def same_file( left: Path | str, right: Path | str, *, + follow_symlinks: bool = True, user: str | User | None = None, ) -> bool: self.same_file_users.append(self._user_name(user)) - return await super().same_file(left, right) + return await super().same_file(left, right, follow_symlinks=follow_symlinks) diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index 886515163a..c589a6baa4 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -2,6 +2,7 @@ from pathlib import Path, PurePosixPath from typing import cast +from unittest.mock import AsyncMock, MagicMock import pytest @@ -13,6 +14,7 @@ ApplyPatchFileNotFoundError, ApplyPatchPathError, ) +from agents.sandbox.session.sandbox_session import SandboxSession from tests.sandbox._apply_patch_test_session import ( ApplyPatchSession, CaseFoldingApplyPatchSession, @@ -307,8 +309,10 @@ async def test_apply_patch_move_to_leaves_the_source_alone_when_the_write_fails( assert session.files == {PurePosixPath("/workspace/notes.txt"): b"alpha\nbeta\n"} # The file surviving is not enough. The refused implementation removed the source and then - # wrote it back, which also ends here. Nothing may be removed at all. - assert session.rm_calls == [] + # wrote it back, which also ends here. The source may not be removed at all, and the only + # path this is allowed to remove is the staging file it was in the middle of writing. + assert PurePosixPath("/workspace/notes.txt") not in [path for path, _ in session.rm_calls] + assert all(path.name.startswith(".apply_patch-") for path, _ in session.rm_calls) @pytest.mark.asyncio @@ -412,6 +416,100 @@ async def test_apply_patch_move_to_commits_the_destination_before_removing_the_s assert session.rm_calls == [(cast(Path, PurePosixPath("/workspace/notes.txt")), False)] +@pytest.mark.asyncio +async def test_apply_patch_move_to_removes_a_source_symlink_pointing_at_the_destination() -> None: + """`test -ef` follows symlinks, and the removal decision must not. + + A symlink and its target are one file by device and inode, and two directory entries. + Removing the symlink leaves the target alone, so a rename that reads them as the same file + leaves the old name behind pointing at the new one. + """ + session = PosixHostApplyPatchSession() + link = cast(Path, PurePosixPath("/workspace/notes.txt")) + target = cast(Path, PurePosixPath("/workspace/Notes.txt")) + session.files[link] = b"alpha\nbeta\n" + session.files[target] = b"alpha\nbeta\n" + session.symlinks[link] = target + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="Notes.txt", + ) + ) + + assert session.files == {target: b"alpha\ngamma\n"} + + +@pytest.mark.asyncio +async def test_sandbox_session_forwards_follow_symlinks_to_the_inner_session() -> None: + """The editor always talks to the instrumented wrapper, never to the session underneath. + + `BaseSandboxSession.apply_patch` builds the editor around `self`, and every client hands + out a `SandboxSession`. A wrapper that accepts `follow_symlinks` and drops it leaves the + inner session running the plain `-ef` test, and every test above uses a session double that + never crosses the wrapper, so nothing else here would notice. + """ + inner = MagicMock() + inner.same_file = AsyncMock(return_value=True) + session = SandboxSession(inner) + + await session.same_file("/workspace/link.txt", "/workspace/target.txt", follow_symlinks=False) + + # .get, not [], so a wrapper that drops the argument fails on the value rather than + # raising KeyError from the assertion itself. + assert inner.same_file.await_args.kwargs.get("follow_symlinks") is False + + +@pytest.mark.asyncio +async def test_apply_patch_move_to_the_same_path_writes_in_place() -> None: + """A `move_to` that names the path it already has is an update, not a rename. + + Committing it through a staging file would replace the inode, and with it the mode and the + extended attributes, for an operation that moves nothing. + """ + session = PosixHostApplyPatchSession() + source = cast(Path, PurePosixPath("/workspace/notes.txt")) + session.files[source] = b"alpha\nbeta\n" + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="notes.txt", + ) + ) + + assert session.files == {source: b"alpha\ngamma\n"} + assert session.mv_calls == [] + assert session.rm_calls == [] + + +@pytest.mark.asyncio +async def test_apply_patch_move_to_a_long_name_keeps_the_staging_name_within_the_limit() -> None: + """A staging name built from the destination name overflows the 255-byte basename limit.""" + session = PosixHostApplyPatchSession() + session.files[cast(Path, PurePosixPath("/workspace/notes.txt"))] = b"alpha\nbeta\n" + long_name = "n" * 250 + ".txt" + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to=long_name, + ) + ) + + assert len(session.mv_calls) == 1 + staging, _ = session.mv_calls[0] + assert len(staging.name.encode("utf-8")) <= 255 + assert session.files == {PurePosixPath(f"/workspace/{long_name}"): b"alpha\ngamma\n"} + + @pytest.mark.asyncio async def test_apply_patch_allows_absolute_path_within_root() -> None: session = ApplyPatchSession() diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index 55f3848b62..5c467c392c 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -692,3 +692,28 @@ async def test_same_file_answers_for_real_paths(self, tmp_path: Path) -> None: assert await session.same_file(workspace / "one.txt", workspace / "one.txt") is True assert await session.same_file(workspace / "one.txt", workspace / "two.txt") is False + + @pytest.mark.asyncio + @pytest.mark.requires_native_macos_sandbox + async def test_same_file_does_not_follow_symlinks_when_asked_not_to( + self, tmp_path: Path + ) -> None: + """A symlink and its target are one file and two directory entries. + + `test -ef` follows the link, so it calls them the same file. A caller deciding whether + removing one destroys the other needs the other answer, and this is shell code, so run + a real shell against a real symlink. + """ + workspace = tmp_path / "workspace" + client = UnixLocalSandboxClient() + manifest = Manifest(root=str(workspace)) + + async with await client.create(manifest=manifest, snapshot=None, options=None) as session: + await session.write(Path("target.txt"), io.BytesIO(b"alpha\n")) + (workspace / "link.txt").symlink_to(workspace / "target.txt") + + link = workspace / "link.txt" + target = workspace / "target.txt" + assert await session.same_file(link, target) is True + assert await session.same_file(link, target, follow_symlinks=False) is False + assert await session.same_file(target, target, follow_symlinks=False) is True From 0d69c23118ee6260412857171898c628bb904c29 Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Tue, 8 Sep 2026 03:48:51 +0800 Subject: [PATCH 04/12] docs: say what the removal does not guarantee The docstring said a file another writer creates at the source path is never overwritten. That is true and was standing in for a guarantee it does not make: the identity answer is read before the removal, and the removal names a path rather than the entry that answer was about, so such a file can still be removed. No behaviour change. Closing the window needs a removal that can be told which entry it may remove, which is the per-backend question already open with the maintainer. Co-Authored-By: Claude Opus 5 --- src/agents/sandbox/apply_patch.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 0d8c5d2253..3d2ddc72bb 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -233,6 +233,11 @@ async def _move_updated_text( 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 From 5a3a50ca3f6b110446fc474d4784942e68ce5376 Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:12:07 +0800 Subject: [PATCH 05/12] fix: complete case-only renames on folding filesystems --- src/agents/sandbox/apply_patch.py | 26 +++++++++++-------- .../sandbox/session/base_sandbox_session.py | 7 +++-- tests/sandbox/_apply_patch_test_session.py | 24 +++++++++-------- tests/sandbox/test_apply_patch.py | 25 +++++++----------- tests/sandbox/test_unix_local.py | 25 ------------------ 5 files changed, 41 insertions(+), 66 deletions(-) diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 3d2ddc72bb..af9a60680a 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -228,15 +228,16 @@ async def _move_updated_text( 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. + is the source removed when the filesystem says it is a different entry. When both names + are one entry, a second move of that entry changes its stored spelling. Before the first + move 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. - 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 identity answer can still go stale. On the different-entry branch, a writer that + replaces the source before the removal loses its file. On the same-entry branch, a writer + that replaces the source before the second move has its content moved onto the destination + and reported as the patched file. Closing either race needs an operation tied to the entry + whose identity was checked, 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 @@ -261,11 +262,14 @@ async def _move_updated_text( 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( + if await self._session.same_file( source, moved_destination, follow_symlinks=False, user=self._user ): + # 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) + else: await self._session.rm(source, user=self._user) async def _write_text(self, destination: Path, text: str) -> None: diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index d0aa5e63c9..3d8ffff1ad 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -1207,14 +1207,13 @@ async def same_file( 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. + file by this test while being two directory entries. Pass ``follow_symlinks=False`` + when the caller needs to distinguish those entries. :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. + its target when the backend preserves the requested leaf path. :param user: Optional sandbox user to compare as. :returns: True when both paths resolve to the same file. """ diff --git a/tests/sandbox/_apply_patch_test_session.py b/tests/sandbox/_apply_patch_test_session.py index 6e660b716f..849043ebb1 100644 --- a/tests/sandbox/_apply_patch_test_session.py +++ b/tests/sandbox/_apply_patch_test_session.py @@ -121,13 +121,17 @@ async def mv( stored_source = self._stored_path(source) if stored_source not in self.files: raise FileNotFoundError(stored_source) + stored_destination = self._stored_path(destination) + destination_exists = stored_destination in self.files payload = self.files.pop(stored_source) - # Look the destination up after removing the source, so a case-only rename does not - # find the entry it is renaming. A real `mv` replaces whatever is at the destination - # and stores the name it was given, which is how a case-only rename changes the case. - self.files.pop(self._stored_path(destination), None) normalized_destination = self.normalize_path(destination) - self.files[normalized_destination] = payload + if destination_exists and stored_destination != stored_source: + # APFS keeps an existing entry's spelling when a different inode replaces it + # through a case-variant path. A later rename of that same entry changes the case. + self.files[stored_destination] = payload + else: + self.files.pop(stored_destination, None) + self.files[normalized_destination] = payload self.mv_calls.append((stored_source, normalized_destination)) async def same_file( @@ -146,8 +150,6 @@ async def same_file( return self._resolved_path(left) == self._resolved_path(right) def _resolved_path(self, path: Path | str) -> Path: - # `-ef` resolves the whole chain, so a fake that follows one hop would answer False - # where a real filesystem answers True. The seen set stops a cycle. stored = self._stored_path(path) seen: set[Path] = set() while stored in self.symlinks and stored not in seen: @@ -171,11 +173,11 @@ def normalize_path(self, path: Path | str, *, for_write: bool = False) -> Path: class CaseFoldingApplyPatchSession(PosixHostApplyPatchSession): - """A case-sensitive host over a sandbox filesystem that folds path case. + """A case-sensitive host over a store that models case-folding APFS. - APFS, NTFS, and Docker bind mounts backed by either store `notes.txt` and `Notes.txt` as - one file, and they preserve the case of the name that created the file. Lookups here fold - case so an existing entry keeps its stored name when it is written again. + Case-folding APFS stores `notes.txt` and `Notes.txt` as one file. Replacing that entry + through a case-variant path keeps its stored name, while moving the entry itself changes + the spelling. Lookups here fold case so the double follows that measured behavior. """ def _stored_path(self, path: Path | str) -> Path: diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index c589a6baa4..4a6c083822 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -271,6 +271,12 @@ async def test_apply_patch_case_only_move_to_keeps_file_on_case_folding_filesyst ) assert session.files == {PurePosixPath("/workspace/Notes.txt"): b"alpha\ngamma\n"} + assert len(session.mv_calls) == 2 + assert session.mv_calls[1] == ( + PurePosixPath("/workspace/notes.txt"), + PurePosixPath("/workspace/Notes.txt"), + ) + assert session.rm_calls == [] @pytest.mark.asyncio @@ -418,12 +424,7 @@ async def test_apply_patch_move_to_commits_the_destination_before_removing_the_s @pytest.mark.asyncio async def test_apply_patch_move_to_removes_a_source_symlink_pointing_at_the_destination() -> None: - """`test -ef` follows symlinks, and the removal decision must not. - - A symlink and its target are one file by device and inode, and two directory entries. - Removing the symlink leaves the target alone, so a rename that reads them as the same file - leaves the old name behind pointing at the new one. - """ + """A source symlink is a separate entry even when `test -ef` follows it to the destination.""" session = PosixHostApplyPatchSession() link = cast(Path, PurePosixPath("/workspace/notes.txt")) target = cast(Path, PurePosixPath("/workspace/Notes.txt")) @@ -441,25 +442,19 @@ async def test_apply_patch_move_to_removes_a_source_symlink_pointing_at_the_dest ) assert session.files == {target: b"alpha\ngamma\n"} + assert session.mv_calls[-1][1] == target + assert session.rm_calls == [(link, False)] @pytest.mark.asyncio async def test_sandbox_session_forwards_follow_symlinks_to_the_inner_session() -> None: - """The editor always talks to the instrumented wrapper, never to the session underneath. - - `BaseSandboxSession.apply_patch` builds the editor around `self`, and every client hands - out a `SandboxSession`. A wrapper that accepts `follow_symlinks` and drops it leaves the - inner session running the plain `-ef` test, and every test above uses a session double that - never crosses the wrapper, so nothing else here would notice. - """ + """The wrapper every client receives must preserve the destructive check's argument.""" inner = MagicMock() inner.same_file = AsyncMock(return_value=True) session = SandboxSession(inner) await session.same_file("/workspace/link.txt", "/workspace/target.txt", follow_symlinks=False) - # .get, not [], so a wrapper that drops the argument fails on the value rather than - # raising KeyError from the assertion itself. assert inner.same_file.await_args.kwargs.get("follow_symlinks") is False diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index 5c467c392c..55f3848b62 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -692,28 +692,3 @@ async def test_same_file_answers_for_real_paths(self, tmp_path: Path) -> None: assert await session.same_file(workspace / "one.txt", workspace / "one.txt") is True assert await session.same_file(workspace / "one.txt", workspace / "two.txt") is False - - @pytest.mark.asyncio - @pytest.mark.requires_native_macos_sandbox - async def test_same_file_does_not_follow_symlinks_when_asked_not_to( - self, tmp_path: Path - ) -> None: - """A symlink and its target are one file and two directory entries. - - `test -ef` follows the link, so it calls them the same file. A caller deciding whether - removing one destroys the other needs the other answer, and this is shell code, so run - a real shell against a real symlink. - """ - workspace = tmp_path / "workspace" - client = UnixLocalSandboxClient() - manifest = Manifest(root=str(workspace)) - - async with await client.create(manifest=manifest, snapshot=None, options=None) as session: - await session.write(Path("target.txt"), io.BytesIO(b"alpha\n")) - (workspace / "link.txt").symlink_to(workspace / "target.txt") - - link = workspace / "link.txt" - target = workspace / "target.txt" - assert await session.same_file(link, target) is True - assert await session.same_file(link, target, follow_symlinks=False) is False - assert await session.same_file(target, target, follow_symlinks=False) is True From bd8abb8f2553c944c46bd346e382b4d4547a1b17 Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Tue, 8 Sep 2026 23:00:06 +0800 Subject: [PATCH 06/12] fix: skip redundant moves through path aliases --- src/agents/sandbox/apply_patch.py | 13 +++--- tests/sandbox/_apply_patch_test_session.py | 51 ++++++++++++++++++++++ tests/sandbox/test_apply_patch.py | 22 ++++++++++ 3 files changed, 80 insertions(+), 6 deletions(-) diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index af9a60680a..dde8693010 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -229,9 +229,9 @@ async def _move_updated_text( 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 when the filesystem says it is a different entry. When both names - are one entry, a second move of that entry changes its stored spelling. Before the first - move 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. + are one entry and the leaf spellings differ, a second move changes the stored spelling. + Before the first move 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. The identity answer can still go stale. On the different-entry branch, a writer that replaces the source before the removal loses its file. On the same-entry branch, a writer @@ -262,14 +262,15 @@ async def _move_updated_text( with contextlib.suppress(Exception): await self._session.rm(staging, user=self._user) raise - if await self._session.same_file( + 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) - else: + elif not same_entry: await self._session.rm(source, user=self._user) async def _write_text(self, destination: Path, text: str) -> None: diff --git a/tests/sandbox/_apply_patch_test_session.py b/tests/sandbox/_apply_patch_test_session.py index 849043ebb1..8da57855ac 100644 --- a/tests/sandbox/_apply_patch_test_session.py +++ b/tests/sandbox/_apply_patch_test_session.py @@ -210,6 +210,57 @@ async def rm( await super().rm(self._stored_path(path), recursive=recursive, user=user) +class ParentAliasApplyPatchSession(PosixHostApplyPatchSession): + """A store where `/workspace/alias` resolves to `/workspace/real`.""" + + def _stored_path(self, path: Path | str) -> Path: + normalized = cast(PurePosixPath, self.normalize_path(path)) + alias = PurePosixPath("/workspace/alias") + try: + relative = normalized.relative_to(alias) + except ValueError: + return cast(Path, normalized) + return cast(Path, PurePosixPath("/workspace/real") / relative) + + async def read(self, path: Path, *, user: str | User | None = None) -> io.BytesIO: + return await ApplyPatchSession.read(self, self._stored_path(path), user=user) + + async def write( + self, + path: Path, + data: io.IOBase, + *, + user: str | User | None = None, + ) -> None: + await ApplyPatchSession.write(self, self._stored_path(path), data, user=user) + + async def rm( + self, + path: Path | str, + *, + recursive: bool = False, + user: str | User | None = None, + ) -> None: + await super().rm(self._stored_path(path), recursive=recursive, user=user) + + async def mv( + self, + source: Path | str, + destination: Path | str, + *, + user: str | User | None = None, + ) -> None: + normalized_source = self.normalize_path(source) + normalized_destination = self.normalize_path(destination) + if ( + normalized_source != normalized_destination + and normalized_source.name == normalized_destination.name + and self._stored_path(source) == self._stored_path(destination) + ): + raise RuntimeError("mv: source and destination are the same file") + await super().mv(source, destination, user=user) + + class NormalizationFoldingApplyPatchSession(PosixHostApplyPatchSession): """A case-sensitive host over a filesystem that folds case and Unicode normalization. diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index 4a6c083822..3ab368ed9a 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -20,6 +20,7 @@ CaseFoldingApplyPatchSession, ConcurrentWriterApplyPatchSession, NormalizationFoldingApplyPatchSession, + ParentAliasApplyPatchSession, PosixHostApplyPatchSession, ProviderNotFoundApplyPatchSession, WriteFailureApplyPatchSession, @@ -297,6 +298,27 @@ async def test_apply_patch_case_only_move_to_moves_file_on_case_sensitive_filesy assert session.files == {PurePosixPath("/workspace/Notes.txt"): b"alpha\ngamma\n"} +@pytest.mark.asyncio +async def test_apply_patch_same_leaf_through_parent_alias_does_not_move_twice() -> None: + """A parent symlink alias does not require renaming the file's directory entry.""" + session = ParentAliasApplyPatchSession() + source = cast(Path, PurePosixPath("/workspace/real/notes.txt")) + session.files[source] = b"alpha\nbeta\n" + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="real/notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="alias/notes.txt", + ) + ) + + assert session.files == {source: b"alpha\ngamma\n"} + assert len(session.mv_calls) == 1 + assert session.rm_calls == [] + + @pytest.mark.asyncio async def test_apply_patch_move_to_leaves_the_source_alone_when_the_write_fails() -> None: """Nothing is removed until the replacement is committed, so a failed write changes nothing.""" From 67ca872513d93490f389c1f22b33eb7cf41b47ad Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Wed, 9 Sep 2026 03:22:51 +0800 Subject: [PATCH 07/12] fix: ask the sandbox, not the host, whether two paths are one file `Path` equality folds case on a Windows host, so a case-only `move_to` took the same-path fast path there. The update was written in place, the operation reported success, and the requested name was never created in a case-sensitive sandbox. The host's filesystem semantics say nothing about the sandbox's. Comparing the canonical spellings keeps the fast path for a genuine no-op and sends a case-only rename down the staging-and-move path, where `same_file` asks the sandbox whether the two paths are one entry. Co-Authored-By: Claude Opus 5 --- src/agents/sandbox/apply_patch.py | 8 +- tests/sandbox/_apply_patch_test_session.py | 85 +++++++++++++++++++++- tests/sandbox/test_apply_patch.py | 24 ++++++ 3 files changed, 115 insertions(+), 2 deletions(-) diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index dde8693010..6d08c7d6c1 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -248,9 +248,15 @@ async def _move_updated_text( 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: + 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 diff --git a/tests/sandbox/_apply_patch_test_session.py b/tests/sandbox/_apply_patch_test_session.py index 8da57855ac..8c2497401c 100644 --- a/tests/sandbox/_apply_patch_test_session.py +++ b/tests/sandbox/_apply_patch_test_session.py @@ -3,7 +3,7 @@ import io import unicodedata import uuid -from pathlib import Path, PurePosixPath +from pathlib import Path, PurePath, PurePosixPath from typing import cast from agents.sandbox import Manifest @@ -172,6 +172,89 @@ def normalize_path(self, path: Path | str, *, for_write: bool = False) -> Path: return cast(Path, PurePosixPath(normalized.as_posix())) +class _CaseFoldingHostPath(PurePosixPath): + """A pure path that compares case-insensitively, as `WindowsPath` does on a Windows host. + + `Path("/workspace/notes.txt") == Path("/workspace/Notes.txt")` is `True` on Windows and + `False` everywhere else. Modelling that here rather than reading `sys.platform` keeps the + coverage on every host that runs the suite. + """ + + def __eq__(self, other: object) -> bool: + if isinstance(other, PurePath): + return self.as_posix().casefold() == other.as_posix().casefold() + return NotImplemented + + def __hash__(self) -> int: + return hash(self.as_posix().casefold()) + + +class CaseFoldingHostApplyPatchSession(PosixHostApplyPatchSession): + """A host that folds case in path comparisons, over a sandbox store that does not. + + This is the SDK running on Windows against a Linux container. The host's own filesystem + says nothing about the sandbox's, so a case-only `move_to` is a real rename that the + sandbox must perform. The store below keeps every spelling apart; only `normalize_path` + hands back paths that fold. + """ + + def normalize_path(self, path: Path | str, *, for_write: bool = False) -> Path: + normalized = super().normalize_path(path, for_write=for_write) + return cast(Path, _CaseFoldingHostPath(normalized.as_posix())) + + def _stored_path(self, path: Path | str) -> Path: + return cast(Path, PurePosixPath(self.normalize_path(path).as_posix())) + + async def read(self, path: Path, *, user: str | User | None = None) -> io.BytesIO: + _ = user + stored = self._stored_path(path) + if stored not in self.files: + raise FileNotFoundError(stored) + return io.BytesIO(self.files[stored]) + + async def write( + self, + path: Path, + data: io.IOBase, + *, + user: str | User | None = None, + ) -> None: + _ = user + payload = data.read() + stored = self._stored_path(path) + if isinstance(payload, str): + self.files[stored] = payload.encode("utf-8") + else: + self.files[stored] = bytes(payload) + + async def rm( + self, + path: Path | str, + *, + recursive: bool = False, + user: str | User | None = None, + ) -> None: + _ = user + stored = self._stored_path(path) + self.rm_calls.append((stored, recursive)) + self.files.pop(stored, None) + + async def mv( + self, + source: Path | str, + destination: Path | str, + *, + user: str | User | None = None, + ) -> None: + _ = user + stored_source = self._stored_path(source) + if stored_source not in self.files: + raise FileNotFoundError(stored_source) + stored_destination = self._stored_path(destination) + self.files[stored_destination] = self.files.pop(stored_source) + self.mv_calls.append((stored_source, stored_destination)) + + class CaseFoldingApplyPatchSession(PosixHostApplyPatchSession): """A case-sensitive host over a store that models case-folding APFS. diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index 3ab368ed9a..96ae01c573 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -18,6 +18,7 @@ from tests.sandbox._apply_patch_test_session import ( ApplyPatchSession, CaseFoldingApplyPatchSession, + CaseFoldingHostApplyPatchSession, ConcurrentWriterApplyPatchSession, NormalizationFoldingApplyPatchSession, ParentAliasApplyPatchSession, @@ -298,6 +299,29 @@ async def test_apply_patch_case_only_move_to_moves_file_on_case_sensitive_filesy assert session.files == {PurePosixPath("/workspace/Notes.txt"): b"alpha\ngamma\n"} +@pytest.mark.asyncio +async def test_apply_patch_case_only_move_to_renames_from_a_case_folding_host() -> None: + """The host's path comparison must not decide whether two sandbox paths are one file. + + On a Windows host `Path` equality folds case, so a case-only `move_to` looked like a + `move_to` that names the path it already has. The update was written in place and reported + as a success while the requested name was never created in the case-sensitive sandbox. + """ + session = CaseFoldingHostApplyPatchSession() + session.files[cast(Path, PurePosixPath("/workspace/notes.txt"))] = b"alpha\nbeta\n" + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="notes.txt", + diff="@@\n alpha\n-beta\n+gamma\n", + move_to="Notes.txt", + ) + ) + + assert session.files == {PurePosixPath("/workspace/Notes.txt"): b"alpha\ngamma\n"} + + @pytest.mark.asyncio async def test_apply_patch_same_leaf_through_parent_alias_does_not_move_twice() -> None: """A parent symlink alias does not require renaming the file's directory entry.""" From 8837bebeabbe8965a44ef229388a6131d1a50aef Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Fri, 11 Sep 2026 14:09:56 +0800 Subject: [PATCH 08/12] docs: say what committing a new inode costs an existing move target A staged commit replaces the destination inode, so a rename that lands on an existing distinct file replaces that file's mode, ownership and extended attributes, where writing into it in place kept them. The docstring covered only the case-folding entry. Preserving them would cost an existence probe and the single-`mv` commit for that case; the content is worth more than the bits. Co-Authored-By: Claude Opus 5 --- src/agents/sandbox/apply_patch.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 6d08c7d6c1..6a9c82c64a 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -239,10 +239,14 @@ async def _move_updated_text( and reported as the patched file. Closing either race needs an operation tied to the entry whose identity was checked, 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 file is a new inode. Committing it replaces the mode, ownership and + extended attributes of whatever entry was at the destination: the original, when the + filesystem folds the two names onto one entry, and an existing distinct file, when the + rename lands on one. An update without `move_to` keeps them, because it writes into the + existing inode. Carrying them across would mean reading and reapplying them per backend, + or asking the sandbox whether the destination exists and writing in place when it does, + which gives up the single-`mv` commit for that case. The committed content 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 From 5b3b0b9753d53cb8662783e3078717823bd87a0d Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Sun, 13 Sep 2026 04:02:34 +0800 Subject: [PATCH 09/12] fix: rename and compare entries descriptor-relative on UnixLocal Since #4931, every UnixLocal file operation runs against a directory descriptor so the path that was validated is the entry acted on. The rename and identity check this branch added still went through `sh -lc`, which re-resolves the path and reopens the window #4931 closed. `_FileOps` gains `rename`, an `os.rename` with `src_dir_fd` and `dst_dir_fd`, and `same_file`, a descriptor-relative `stat` compared by device and inode. UnixLocal overrides `mv` and `same_file` with them, directly for the host user and through the file worker for a bound user. `os.rename` also refuses an existing directory as the destination, where `mv` would have moved the source inside it. The shell versions stay on `BaseSandboxSession` for backends that only offer `exec`. Co-Authored-By: Claude Opus 5 --- .../sandbox/sandboxes/_unix_local_file_ops.py | 50 +++++++++- src/agents/sandbox/sandboxes/unix_local.py | 95 ++++++++++++++++++- .../sandbox/session/base_sandbox_session.py | 7 ++ tests/sandbox/test_unix_local.py | 8 +- tests/sandbox/test_unix_local_file_io.py | 89 ++++++++++++++++- tests/sandbox/test_unix_local_user_file_io.py | 89 ++++++++++++++++- 6 files changed, 325 insertions(+), 13 deletions(-) diff --git a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py index 90d228b4ac..0eb72eed49 100644 --- a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py +++ b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py @@ -113,6 +113,43 @@ def listing(self, path: Path) -> list[dict[str, str | int]]: finally: os.close(fd) + def rename(self, source: Path, destination: Path) -> None: + """Rename an entry, replacing whatever entry is at the destination. + + 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), + ): + 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 entry, 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. + """ + 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 + return os.path.samestat(left_stat, right_stat) + def _entry(path: Path, entry: os.stat_result) -> dict[str, str | int]: try: @@ -162,13 +199,22 @@ 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 == "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") diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index ad25e630ef..95858a6389 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1054,6 +1054,92 @@ 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_path(source, for_write=True) + normalized_destination = self.normalize_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: + normalized_left = self.normalize_path(left) + normalized_right = self.normalize_path(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 + async def _write_stream_with_exec( self, path: Path, @@ -1083,14 +1169,15 @@ async def _write_stream_with_exec( async def _run_file_operation_as_user( self, - operation: Literal["ls", "write"], + operation: Literal["ls", "write", "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 == "write") + for_write = operation in ("write", "rename") + paths = [self._files.authorize(each, for_write=for_write) for each in (path, *more_paths)] command = self._prepare_exec_command( "python3", "-I", @@ -1098,7 +1185,7 @@ async def _run_file_operation_as_user( "-c", _USER_FILE_WORKER_SOURCE, operation, - str(path), + *(str(each) for each in paths), shell=False, user=user, ) diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index 3d8ffff1ad..eef35aee49 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -1164,6 +1164,10 @@ async def mv( `mv -T` would say this directly and is GNU-only, so it is unavailable on the BSD userland this also has to run against. + This is the shell fallback for backends that only offer `exec`. A backend with + direct filesystem access, such as UnixLocal, overrides it with a descriptor-relative + `os.rename`, so the path it validated is the entry it renames. + :param source: Path to move. :param destination: Path to move it to. :param user: Optional sandbox user to move as. @@ -1210,6 +1214,9 @@ async def same_file( file by this test while being two directory entries. Pass ``follow_symlinks=False`` when the caller needs to distinguish those entries. + This is the shell fallback for backends that only offer `exec`. UnixLocal overrides + it with a descriptor-relative `stat` on both entries. + :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 diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index 55f3848b62..6ba95909a9 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -652,11 +652,11 @@ async def test_case_only_move_to_keeps_the_file(self, tmp_path: Path) -> None: @pytest.mark.asyncio @pytest.mark.requires_native_macos_sandbox async def test_move_to_an_existing_directory_keeps_the_source(self, tmp_path: Path) -> None: - """The guard against a directory destination is a shell test, so run a real shell. + """A directory destination is refused, and the source is where it was. - `mv` given an existing directory moves the source inside it and exits 0. A caller that - removes the source on that exit code deletes the file. The unit tests model this; this - one runs it. + `mv` given an existing directory moves the source inside it and exits 0, and a caller + that removes the source on that exit code deletes the file. The rename here goes + through `os.rename`, which fails instead. The unit tests model this; this one runs it. """ workspace = tmp_path / "workspace" client = UnixLocalSandboxClient() diff --git a/tests/sandbox/test_unix_local_file_io.py b/tests/sandbox/test_unix_local_file_io.py index 4f7095af29..92fef12aad 100644 --- a/tests/sandbox/test_unix_local_file_io.py +++ b/tests/sandbox/test_unix_local_file_io.py @@ -26,6 +26,7 @@ from agents.sandbox.snapshot import NoopSnapshot if TYPE_CHECKING or sys.platform != "win32": + from agents.sandbox.sandboxes._unix_local_file_ops import _FileOps from agents.sandbox.sandboxes.unix_local import ( UnixLocalSandboxSession, UnixLocalSandboxSessionState, @@ -59,10 +60,16 @@ async def _operate(session: UnixLocalSandboxSession, operation: str, path: Path) ) elif operation in {"rm", "rmtree"}: await session.rm(path, recursive=operation == "rmtree") + elif operation == "mv": + await session.mv(path, path.with_name("moved")) + elif operation == "same_file": + return await session.same_file(path, path, follow_symlinks=False) return None -@pytest.mark.parametrize("operation", ["read", "write", "mkdir", "ls", "rm", "rmtree", "patch"]) +@pytest.mark.parametrize( + "operation", ["read", "write", "mkdir", "ls", "rm", "rmtree", "patch", "mv", "same_file"] +) async def test_parent_swap_after_validation_cannot_access_outside( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, operation: str ) -> None: @@ -133,7 +140,9 @@ def swap(path: Path | str, *, for_write: bool = False) -> Path: return result monkeypatch.setattr(session, "normalize_path", swap) - if operation in {"rm", "rmtree"}: + if operation in {"rm", "rmtree", "mv", "same_file"}: + # These act on the entry itself, so a swapped-in symlink is removed, moved or + # compared as a link, never followed. await _operate(session, operation, Path("target")) else: with pytest.raises( @@ -417,3 +426,79 @@ def paused_scandir(path: int): for fd in opened: with pytest.raises(OSError): os.fstat(fd) + + +async def test_rename_replaces_the_destination_entry(tmp_path: Path) -> None: + workspace = tmp_path / "workspace" + workspace.mkdir() + (workspace / "a.txt").write_bytes(b"from a") + (workspace / "b.txt").write_bytes(b"from b") + session = _session(workspace) + + await session.mv(Path("a.txt"), Path("b.txt")) + + assert sorted(entry.name for entry in workspace.iterdir()) == ["b.txt"] + assert (workspace / "b.txt").read_bytes() == b"from a" + + +async def test_rename_onto_a_directory_fails_and_keeps_the_source(tmp_path: Path) -> None: + # `mv` would put the source inside the directory and exit 0; a caller that then removes + # the source would delete the file. `os.rename` refuses instead. + workspace = tmp_path / "workspace" + workspace.mkdir() + (workspace / "notes.txt").write_bytes(b"kept") + (workspace / "docs").mkdir() + session = _session(workspace) + + with pytest.raises(ExecNonZeroError): + await session.mv(Path("notes.txt"), Path("docs")) + + assert (workspace / "notes.txt").read_bytes() == b"kept" + assert list((workspace / "docs").iterdir()) == [] + + +async def test_rename_moves_a_symlink_entry_rather_than_its_target(tmp_path: Path) -> None: + # Below the session, the operation is on the entry named, so a link moves as a link. + workspace = tmp_path / "workspace" + workspace.mkdir() + target = workspace / "target.txt" + target.write_bytes(b"target") + link = workspace / "link.txt" + link.symlink_to(target) + + _FileOps().rename(link, workspace / "moved.txt") + + assert target.read_bytes() == b"target" + assert (workspace / "moved.txt").is_symlink() + assert not link.exists() + + +async def test_same_file_answers_by_entry_identity(tmp_path: Path) -> None: + workspace = tmp_path / "workspace" + workspace.mkdir() + one = workspace / "one.txt" + one.write_bytes(b"one") + (workspace / "two.txt").write_bytes(b"two") + (workspace / "hard.txt").hardlink_to(one) + session = _session(workspace) + + assert await session.same_file(Path("one.txt"), Path("one.txt")) is True + assert await session.same_file(Path("one.txt"), Path("hard.txt")) is True + assert await session.same_file(Path("one.txt"), Path("two.txt")) is False + assert await session.same_file(Path("one.txt"), Path("missing.txt")) is False + + +async def test_same_file_distinguishes_a_link_from_its_target_when_asked( + tmp_path: Path, +) -> None: + # The session resolves a leaf symlink before the check, so this is the module's answer. + workspace = tmp_path / "workspace" + workspace.mkdir() + target = workspace / "target.txt" + target.write_bytes(b"target") + link = workspace / "link.txt" + link.symlink_to(target) + + files = _FileOps() + assert files.same_file(link, target) is True + assert files.same_file(link, target, follow_symlinks=False) is False diff --git a/tests/sandbox/test_unix_local_user_file_io.py b/tests/sandbox/test_unix_local_user_file_io.py index 927fb1e821..c3747fb403 100644 --- a/tests/sandbox/test_unix_local_user_file_io.py +++ b/tests/sandbox/test_unix_local_user_file_io.py @@ -111,6 +111,89 @@ def close(self) -> None: assert output.closed +@pytest.mark.asyncio +async def test_user_rename_runs_in_the_worker_relative_to_both_parents( + session: unix_local.UnixLocalSandboxSession, monkeypatch: pytest.MonkeyPatch +) -> None: + opened = Mock(side_effect=[10, 11, 12, 13, 14, 15]) + closed = Mock() + renamed = Mock() + monkeypatch.setattr(os, "open", opened) + monkeypatch.setattr(os, "close", closed) + monkeypatch.setattr(os, "rename", renamed) + dispatch = Mock(side_effect=_worker) + monkeypatch.setattr(subprocess, "run", dispatch) + await session.mv(Path("old/file"), Path("new/File"), user=User(name="example-user")) + assert dispatch.call_count == 1 + assert dispatch.call_args.args[0][9:] == [ + "rename", + "/workspace/old/file", + "/workspace/new/File", + ] + assert [call.args[0] for call in opened.call_args_list] == [ + "/", + "workspace", + "old", + "/", + "workspace", + "new", + ] + assert all(call.args[1] & os.O_NOFOLLOW for call in opened.call_args_list) + renamed.assert_called_once_with("file", "File", src_dir_fd=12, dst_dir_fd=15) + assert sorted(call.args[0] for call in closed.call_args_list) == [10, 11, 12, 13, 14, 15] + + +@pytest.mark.asyncio +async def test_user_rename_failure_keeps_the_error( + session: unix_local.UnixLocalSandboxSession, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(os, "open", Mock(side_effect=[10, 11, 12, 13])) + monkeypatch.setattr(os, "close", Mock()) + monkeypatch.setattr(os, "rename", Mock(side_effect=IsADirectoryError("Is a directory"))) + dispatch = Mock(side_effect=_worker) + monkeypatch.setattr(subprocess, "run", dispatch) + with pytest.raises(ExecNonZeroError) as error: + await session.mv(Path("file"), Path("docs"), user="example-user") + assert dispatch.call_count == 1 + assert b"Is a directory" in error.value.result.stderr + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("follow_symlinks", "inodes", "expected"), + [ + pytest.param(True, (7, 7), True, id="same-entry"), + pytest.param(True, (7, 8), False, id="different-entries"), + pytest.param(False, (7, 9), False, id="link-not-followed"), + ], +) +async def test_user_same_file_asks_the_worker( + session: unix_local.UnixLocalSandboxSession, + monkeypatch: pytest.MonkeyPatch, + follow_symlinks: bool, + inodes: tuple[int, int], + expected: bool, +) -> None: + monkeypatch.setattr(os, "open", Mock(side_effect=[10, 11, 12, 13])) + monkeypatch.setattr(os, "close", Mock()) + stats = [os.stat_result((stat.S_IFREG, ino, 1, 1, 0, 0, 0, 0, 0, 0)) for ino in inodes] + stat_mock = Mock(side_effect=stats) + monkeypatch.setattr(os, "stat", stat_mock) + dispatch = Mock(side_effect=_worker) + monkeypatch.setattr(subprocess, "run", dispatch) + answer = await session.same_file( + Path("left"), Path("right"), follow_symlinks=follow_symlinks, user="example-user" + ) + assert answer is expected + assert dispatch.call_count == 1 + assert dispatch.call_args.args[0][9:] == ["same_file", "/workspace/left", "/workspace/right"] + assert dispatch.call_args.kwargs["input"] == (b"1" if follow_symlinks else b"0") + assert [call.kwargs["follow_symlinks"] for call in stat_mock.call_args_list] == [ + follow_symlinks, + follow_symlinks, + ] + + @pytest.mark.asyncio @pytest.mark.parametrize("failure", [errno.EACCES, errno.ELOOP]) @pytest.mark.parametrize("operation", ["ls", "write"]) @@ -209,13 +292,17 @@ async def test_user_listing_preserves_metadata_and_names( @pytest.mark.asyncio -@pytest.mark.parametrize("operation", ["ls", "write"]) +@pytest.mark.parametrize("operation", ["ls", "write", "mv", "same_file"]) async def test_ungranted_path_never_dispatches( session: unix_local.UnixLocalSandboxSession, operation: str ) -> None: with pytest.raises(InvalidManifestPathError): if operation == "ls": await session.ls("/ungranted/file", user="example-user") + elif operation == "mv": + await session.mv(Path("file"), Path("/ungranted/file"), user="example-user") + elif operation == "same_file": + await session.same_file(Path("file"), Path("/ungranted/file"), user="example-user") else: await session.write(Path("/ungranted/file"), io.BytesIO(b"value"), user="example-user") subprocess.run.assert_not_called() # type: ignore[attr-defined] From 6edb43b85b3df1bab9bc4b54e1ddae8e5541aab2 Mon Sep 17 00:00:00 2001 From: Justin Beckwith Date: Sun, 27 Sep 2026 12:33:26 -0700 Subject: [PATCH 10/12] Restrict sandbox moves to files and symlinks --- .../sandbox/sandboxes/_unix_local_file_ops.py | 7 +- .../sandbox/session/base_sandbox_session.py | 19 ++++-- tests/sandbox/test_apply_patch.py | 66 ++++++++++++++++++- tests/sandbox/test_unix_local_file_io.py | 34 ++++++++++ tests/sandbox/test_unix_local_user_file_io.py | 20 ++++++ 5 files changed, 137 insertions(+), 9 deletions(-) diff --git a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py index c0de43203b..67e0fbbabe 100644 --- a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py +++ b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py @@ -156,7 +156,7 @@ def listing(self, path: Path) -> list[dict[str, str | int]]: os.close(fd) def rename(self, source: Path, destination: Path) -> None: - """Rename an entry, replacing whatever entry is at the destination. + """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 @@ -167,6 +167,11 @@ def rename(self, source: Path, destination: Path) -> None: 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, diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index 320ee3294b..be3308b01e 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -1203,17 +1203,20 @@ async def mv( *, user: str | User | None = None, ) -> None: - """Rename a path, replacing the destination if it exists. + """Rename a regular file or symlink, replacing the destination entry if it exists. + + Directory and special-file sources are unsupported and rejected before the move. + As with other workspace file APIs, callers must serialize conflicting mutations. The exec fallback requires ``mv -T`` (GNU or compatible). A tool without that option - fails without moving the source. Do not replace it with a directory precheck: a + fails without moving the source. Do not replace it with a destination precheck: a concurrently created directory could otherwise receive the source as a child. This is the shell fallback for backends that only offer `exec`. A backend with direct filesystem access, such as UnixLocal, overrides it with a descriptor-relative `os.rename`, so the path it validated is the entry it renames. - :param source: Path to move. + :param source: Regular file or symlink 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 @@ -1221,14 +1224,18 @@ async def mv( """ 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 = ("mv", "-fT", "--", source_arg, destination_arg) + script = ( + 'if [ ! -L "$1" ] && [ ! -f "$1" ]; then ' + 'printf "%s\\n" "Move source must be a regular file or symlink" >&2; exit 1; fi; ' + 'exec mv -fT -- "$1" "$2"' + ) + cmd = ("sh", "-c", script, "sh", source_arg, destination_arg) result = await self.exec(*cmd, shell=False, user=user) if not result.ok(): raise ExecNonZeroError( - result, command=("sh", "-lc", "", source_arg, destination_arg) + result, command=("sh", "-c", "", source_arg, destination_arg) ) async def same_file( diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index a323eeb6c1..33d68c42da 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -721,6 +721,7 @@ async def test_apply_patch_mapping_operation_rejects_non_string_move_to() -> Non @pytest.mark.asyncio async def test_fallback_move_cannot_put_source_inside_a_new_directory(tmp_path: Path) -> None: + import shlex import shutil import subprocess import sys @@ -744,7 +745,14 @@ async def exec(self, *command, **kwargs): # The directory arrives after validation but before the actual move. destination.mkdir() result = subprocess.run( - [executable, *map(str, command[1:])], capture_output=True, check=False + [ + "sh", + "-c", + str(command[2]).replace("exec mv ", f"exec {shlex.quote(executable)} "), + *map(str, command[3:]), + ], + capture_output=True, + check=False, ) return ExecResult( stdout=result.stdout, stderr=result.stderr, exit_code=result.returncode @@ -758,6 +766,7 @@ async def exec(self, *command, **kwargs): @pytest.mark.asyncio async def test_fallback_move_replaces_a_directory_symlink(tmp_path: Path) -> None: + import shlex import shutil import subprocess import sys @@ -781,7 +790,14 @@ class ShellSession(ApplyPatchSession): async def exec(self, *command, **kwargs): result = subprocess.run( - [executable, *map(str, command[1:])], capture_output=True, check=False + [ + "sh", + "-c", + str(command[2]).replace("exec mv ", f"exec {shlex.quote(executable)} "), + *map(str, command[3:]), + ], + capture_output=True, + check=False, ) return ExecResult( stdout=result.stdout, stderr=result.stderr, exit_code=result.returncode @@ -792,3 +808,49 @@ async def exec(self, *command, **kwargs): assert link.read_bytes() == b"replacement" assert directory.is_dir() assert not source.exists() + + +@pytest.mark.asyncio +async def test_fallback_move_rejects_directory_source_through_parent_alias(tmp_path: Path) -> None: + import subprocess + import sys + + from agents.sandbox.errors import ExecNonZeroError + from agents.sandbox.manifest import Manifest, SandboxPathGrant + from agents.sandbox.session.base_sandbox_session import BaseSandboxSession + from agents.sandbox.types import ExecResult + + if sys.platform == "win32": + pytest.skip("requires a POSIX shell and symlinks") + workspace = tmp_path / "workspace" + workspace.mkdir() + shared = tmp_path / "shared" + protected = shared / "tree" / "private" + protected.mkdir(parents=True) + payload = protected / "file" + payload.write_bytes(b"protected") + alias = workspace / "alias" + alias.symlink_to(shared, target_is_directory=True) + + class ShellSession(ApplyPatchSession): + mv = BaseSandboxSession.mv + + async def exec(self, *command, **kwargs): + result = subprocess.run(list(map(str, command)), capture_output=True, check=False) + return ExecResult( + stdout=result.stdout, stderr=result.stderr, exit_code=result.returncode + ) + + session = ShellSession( + Manifest( + root=str(workspace), + extra_path_grants=( + SandboxPathGrant(path=str(shared)), + SandboxPathGrant(path=str(protected), read_only=True), + ), + ) + ) + with pytest.raises(ExecNonZeroError, match="regular file or symlink"): + await session.mv(alias / "tree", workspace / "moved") + assert payload.read_bytes() == b"protected" + assert not (workspace / "moved").exists() diff --git a/tests/sandbox/test_unix_local_file_io.py b/tests/sandbox/test_unix_local_file_io.py index 6158da6497..9c268da831 100644 --- a/tests/sandbox/test_unix_local_file_io.py +++ b/tests/sandbox/test_unix_local_file_io.py @@ -665,3 +665,37 @@ async def test_entry_operations_honor_exact_file_grants(tmp_path: Path) -> None: assert destination.read_bytes() == b"granted" assert readonly.read_bytes() == b"read only" assert not source.exists() + + +@pytest.mark.parametrize("user", [None, "example-user"]) +async def test_move_rejects_directory_sources_through_parent_aliases( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, user: str | None +) -> None: + from agents.sandbox.sandboxes import unix_local + from tests.sandbox.test_unix_local_user_file_io import _worker + + workspace = tmp_path / "workspace" + workspace.mkdir() + shared = tmp_path / "shared" + protected = shared / "tree" / "private" + protected.mkdir(parents=True) + payload = protected / "file" + payload.write_bytes(b"protected") + alias = workspace / "alias" + alias.symlink_to(shared, target_is_directory=True) + session = _session( + workspace, + grants=( + SandboxPathGrant(path=str(shared)), + SandboxPathGrant(path=str(protected), read_only=True), + ), + ) + if user is not None: + monkeypatch.setattr(unix_local.shutil, "which", lambda command: "/usr/bin/sudo") + monkeypatch.setattr(unix_local.subprocess, "run", _worker) + + with pytest.raises(ExecNonZeroError, match="regular file or symlink"): + await session.mv(alias / "tree", workspace / "moved", user=user) + + assert payload.read_bytes() == b"protected" + assert not (workspace / "moved").exists() diff --git a/tests/sandbox/test_unix_local_user_file_io.py b/tests/sandbox/test_unix_local_user_file_io.py index 31a42c97af..8825ee0e7e 100644 --- a/tests/sandbox/test_unix_local_user_file_io.py +++ b/tests/sandbox/test_unix_local_user_file_io.py @@ -128,6 +128,16 @@ async def test_user_rename_runs_in_the_worker_relative_to_both_parents( monkeypatch.setattr(os, "open", opened) monkeypatch.setattr(os, "close", closed) monkeypatch.setattr(os, "rename", renamed) + original_stat = os.stat + monkeypatch.setattr( + os, + "stat", + lambda *args, **kwargs: ( + os.stat_result((stat.S_IFREG, 1, 1, 1, 0, 0, 0, 0, 0, 0)) + if "dir_fd" in kwargs + else original_stat(*args, **kwargs) + ), + ) dispatch = Mock(side_effect=_worker) monkeypatch.setattr(subprocess, "run", dispatch) await session.mv(Path("old/file"), Path("new/File"), user=User(name="example-user")) @@ -157,6 +167,16 @@ async def test_user_rename_failure_keeps_the_error( monkeypatch.setattr(os, "open", Mock(side_effect=[10, 11, 12, 13])) monkeypatch.setattr(os, "close", Mock()) monkeypatch.setattr(os, "rename", Mock(side_effect=IsADirectoryError("Is a directory"))) + original_stat = os.stat + monkeypatch.setattr( + os, + "stat", + lambda *args, **kwargs: ( + os.stat_result((stat.S_IFREG, 1, 1, 1, 0, 0, 0, 0, 0, 0)) + if "dir_fd" in kwargs + else original_stat(*args, **kwargs) + ), + ) dispatch = Mock(side_effect=_worker) monkeypatch.setattr(subprocess, "run", dispatch) with pytest.raises(ExecNonZeroError) as error: From bca5250ee50f5daad67fd7811804903cd6fee211 Mon Sep 17 00:00:00 2001 From: Justin Beckwith Date: Sun, 27 Sep 2026 13:29:21 -0700 Subject: [PATCH 11/12] fix: preserve diverged source after alias patch commit --- src/agents/sandbox/apply_patch.py | 9 ++++- tests/sandbox/test_unix_local_file_io.py | 50 ++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 7ff40dc16b..97329237df 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -225,6 +225,8 @@ async def _move_updated_text( 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(): @@ -263,7 +265,12 @@ async def _move_updated_text( # the requested case-only rename without touching the committed content. await self._session.mv(source, moved_destination, user=self._user) elif not same_entry: - await self._session.rm(source, user=self._user) + 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) diff --git a/tests/sandbox/test_unix_local_file_io.py b/tests/sandbox/test_unix_local_file_io.py index 9c268da831..38286fcc3c 100644 --- a/tests/sandbox/test_unix_local_file_io.py +++ b/tests/sandbox/test_unix_local_file_io.py @@ -15,6 +15,7 @@ from agents.sandbox.apply_patch import WorkspaceEditor from agents.sandbox.capabilities import Capability from agents.sandbox.errors import ( + ApplyPatchDiffError, ExecNonZeroError, InvalidManifestPathError, WorkspaceArchiveReadError, @@ -44,6 +45,55 @@ def _session(root: Path, *, grants: tuple[SandboxPathGrant, ...] = ()) -> UnixLo ) +@pytest.mark.parametrize("replace_source", [False, True]) +async def test_apply_patch_preserves_diverged_alias_source( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, replace_source: bool +) -> None: + source = tmp_path / "source.txt" + destination = tmp_path / "destination.txt" + source.write_bytes(b"original\n") + destination.hardlink_to(source) + session = _session(tmp_path) + committed = asyncio.Event() + resume = asyncio.Event() + move = session.mv + + async def pause_after_commit(*args, **kwargs): + await move(*args, **kwargs) + committed.set() + await resume.wait() + + monkeypatch.setattr(session, "mv", pause_after_commit) + operation = asyncio.create_task( + session.apply_patch( + ApplyPatchOperation( + type="update_file", + path="source.txt", + move_to="destination.txt", + diff="@@\n-original\n+updated\n", + ) + ) + ) + try: + await asyncio.wait_for(committed.wait(), timeout=5) + assert destination.read_bytes() == b"updated\n" + if replace_source: + source.unlink() + source.write_bytes(b"other writer\n") + resume.set() + with pytest.raises(ApplyPatchDiffError, match="source was left untouched"): + await operation + assert source.read_bytes() == (b"other writer\n" if replace_source else b"original\n") + assert destination.read_bytes() == b"updated\n" + assert sorted(path.name for path in tmp_path.iterdir()) == [ + "destination.txt", + "source.txt", + ] + finally: + operation.cancel() + await asyncio.gather(operation, return_exceptions=True) + + async def _operate(session: UnixLocalSandboxSession, operation: str, path: Path) -> object: if operation == "read": with await session.read(path) as stream: From 88e45725ba76d90a1b0b7577cc6fec20ee4a4794 Mon Sep 17 00:00:00 2001 From: Justin Beckwith Date: Sun, 27 Sep 2026 18:00:47 -0700 Subject: [PATCH 12/12] ci: allow Windows tests time to finish cleanup --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index f9077289a5..aa87fe63a6 100644 --- a/.github/workflows/tests.yml +++ b/.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: