diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 30623fdf82..af9a60680a 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -1,8 +1,10 @@ from __future__ import annotations +import contextlib import io from pathlib import Path from typing import TYPE_CHECKING, Any, Literal, Protocol, cast, runtime_checkable +from uuid import uuid4 from ..apply_diff import ApplyDiffMode, apply_diff from ..editor import ApplyPatchOperation, ApplyPatchOperationType, ApplyPatchResult @@ -111,9 +113,11 @@ async def apply_operation( moved_relative_path, moved_display_path = self._resolve_path(operation.move_to) moved_destination = self._session.normalize_path(moved_relative_path) - await self._write_text(moved_destination, updated_text) - if moved_destination != destination: - await self._session.rm(destination, user=self._user) + await self._move_updated_text( + source=destination, + moved_destination=moved_destination, + text=updated_text, + ) return ApplyPatchResult( output=f"Updated {display_path}\nMoved {display_path} to {moved_display_path}" ) @@ -209,6 +213,65 @@ async def _read_text(self, destination: Path, *, op_path: str, decode_path: Path path=op_path, ) + async def _move_updated_text( + self, + *, + source: Path, + moved_destination: Path, + text: str, + ) -> None: + """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 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. + + 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 + reading and reapplying them per backend; committing the content in a single `mv` is + worth more than the mode bits. + + The staging name is a fixed length rather than a decoration of the destination name, + because a destination basename near the filesystem's 255-byte limit would make the + decorated name exceed it and the write would fail with ENAMETOOLONG. + """ + if source == moved_destination: + # Not a rename, so nothing needs committing elsewhere. Writing in place is what an + # update without `move_to` does, and it keeps the inode, the mode and the xattrs. + await self._write_text(source, text) + return + + staging = moved_destination.with_name(f".apply_patch-{uuid4().hex}.tmp") + try: + await self._write_text(staging, text) + await self._session.mv(staging, moved_destination, user=self._user) + except BaseException: + with contextlib.suppress(Exception): + await self._session.rm(staging, user=self._user) + raise + 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: await self._session.mkdir(destination.parent, parents=True, user=self._user) await self._session.write( diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index d377bea9ef..3d8ffff1ad 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -1146,6 +1146,99 @@ 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, + *, + follow_symlinks: bool = True, + user: str | User | None = None, + ) -> bool: + """Return whether two paths name the same file on the sandbox filesystem. + + This asks the filesystem, through `test -ef`, which compares device and inode. Two + paths that differ as strings can be one file: a filesystem that folds case stores + `notes.txt` and `Notes.txt` as a single entry, and APFS folds Unicode normalization + as well, so the NFC and NFD spellings of one accented name are also a single entry. + No string comparison can answer this, and neither can the host that is driving the + session, which may not be the kind of system the sandbox is running on. + + `test -ef` resolves symlinks, so a symlink and the file it points at are the same + file by this test while being two directory entries. 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 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. + """ + left = await self._validate_path_access(left) + right = await self._validate_path_access(right) + + left_arg = sandbox_path_str(left) + right_arg = sandbox_path_str(right) + test = '[ "$1" -ef "$2" ]' + if not follow_symlinks: + test = '[ ! -L "$1" ] && [ ! -L "$2" ] && ' + test + cmd = ("sh", "-lc", test, "sh", left_arg, right_arg) + result = await self.exec(*cmd, shell=False, user=user) + if result.exit_code == 0: + return True + # `[` answers "different file" with 1 and reports its own failures with 2, and a + # missing shell exits 127. Only 1 is an answer; anything else is the session + # failing to tell us, and a caller about to delete a file on the strength of this + # must not read that as "different". + if result.exit_code == 1: + return False + raise ExecNonZeroError( + result, command=("sh", "-lc", "", 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..c6d5a1df6b 100644 --- a/src/agents/sandbox/session/sandbox_session.py +++ b/src/agents/sandbox/session/sandbox_session.py @@ -641,6 +641,25 @@ async def rm( ) -> None: await self._inner.rm(path, recursive=recursive, user=user) + async def mv( + self, + source: Path | str, + destination: Path | str, + *, + user: str | User | None = None, + ) -> None: + await self._inner.mv(source, destination, user=user) + + async def same_file( + self, + left: Path | str, + right: Path | str, + *, + follow_symlinks: bool = True, + user: str | User | None = None, + ) -> bool: + return await self._inner.same_file(left, right, follow_symlinks=follow_symlinks, user=user) + async def mkdir( self, path: Path | str, diff --git a/src/agents/testing/sandbox.py b/src/agents/testing/sandbox.py index d086ae7c9d..919f0c88ce 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,32 @@ async def mkdir( ) -> None: await self._invoke("mkdir", (path,), {"parents": parents, "user": user}) + async def mv( + self, + source: Path | str, + destination: Path | str, + *, + user: str | User | None = None, + ) -> None: + await self._invoke("mv", (source, destination), {"user": user}) + + async def same_file( + self, + left: Path | str, + right: Path | str, + *, + follow_symlinks: bool = True, + user: str | User | None = None, + ) -> bool: + return cast( + bool, + await self._invoke( + "same_file", + (left, right), + {"follow_symlinks": follow_symlinks, "user": user}, + ), + ) + async def apply_patch( self, operations: ApplyPatchOperation diff --git a/tests/sandbox/_apply_patch_test_session.py b/tests/sandbox/_apply_patch_test_session.py index 24ce567011..849043ebb1 100644 --- a/tests/sandbox/_apply_patch_test_session.py +++ b/tests/sandbox/_apply_patch_test_session.py @@ -1,8 +1,10 @@ from __future__ import annotations import io +import unicodedata 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 @@ -21,6 +23,18 @@ 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]] = [] + # 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: + """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 @@ -93,6 +107,194 @@ 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) + stored_destination = self._stored_path(destination) + destination_exists = stored_destination in self.files + payload = self.files.pop(stored_source) + normalized_destination = self.normalize_path(destination) + 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( + self, + left: Path | str, + right: Path | str, + *, + follow_symlinks: bool = True, + user: str | User | None = None, + ) -> bool: + _ = user + 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: + 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): + """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 store that models case-folding APFS. + + 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: + 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 NormalizationFoldingApplyPatchSession(PosixHostApplyPatchSession): + """A case-sensitive host over a filesystem that folds case and Unicode normalization. + + 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 + + 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 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: @@ -112,6 +314,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: @@ -150,3 +354,24 @@ 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, + *, + 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, follow_symlinks=follow_symlinks) diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index c4cd676fec..4a6c083822 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -1,6 +1,8 @@ from __future__ import annotations -from pathlib import Path +from pathlib import Path, PurePosixPath +from typing import cast +from unittest.mock import AsyncMock, MagicMock import pytest @@ -12,9 +14,15 @@ ApplyPatchFileNotFoundError, ApplyPatchPathError, ) +from agents.sandbox.session.sandbox_session import SandboxSession from tests.sandbox._apply_patch_test_session import ( ApplyPatchSession, + CaseFoldingApplyPatchSession, + ConcurrentWriterApplyPatchSession, + NormalizationFoldingApplyPatchSession, + PosixHostApplyPatchSession, ProviderNotFoundApplyPatchSession, + WriteFailureApplyPatchSession, ) @@ -247,6 +255,256 @@ 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"} + 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 +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_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" + + 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"} + # The file surviving is not enough. The refused implementation removed the source and then + # 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 +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 +async def test_apply_patch_move_to_removes_a_source_symlink_pointing_at_the_destination() -> None: + """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")) + 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"} + 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 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) + + 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 fb13be3c51..ee5f356fe0 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 ( @@ -513,3 +514,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