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: diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 24e10362e0..5cb43b7f8a 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}" ) @@ -231,6 +235,65 @@ async def _read_text(self, destination: Path, *, op_path: str, decode_path: Path path=op_path, ) + async def _move_updated_text( + self, + *, + source: Path, + moved_destination: Path, + text: str, + ) -> None: + """Stage filesystem aliases; retain ordinary destination write semantics otherwise. + + An alias needs a staged replacement because writing and then removing the source + would delete the updated file. Distinct files retain the released write/remove path, + including an existing destination's metadata and file-level write permissions. + If the paths stop identifying the same file after an alias replacement, leave both + paths alone and report the incomplete move. This includes distinct hardlink aliases. + These operations do not provide a transaction against concurrent workspace writers. + """ + if source.as_posix() == moved_destination.as_posix(): + # Not a rename, so nothing needs committing elsewhere. Writing in place is what an + # update without `move_to` does, and it keeps the inode, the mode and the xattrs. + # + # The comparison is on the spelling rather than on `Path` equality, which folds case + # on a Windows host. Whether two sandbox paths are one file is the sandbox's answer, + # not the host's: a Windows host talking to a case-sensitive sandbox would otherwise + # take this branch for a case-only rename and never create the new name. Paths that + # differ only in case go down the staging path, where `same_file` asks the sandbox. + await self._write_text(source, text) + return + + if not await self._session.same_file( + source, moved_destination, follow_symlinks=False, user=self._user + ): + await self._write_text(moved_destination, text) + await self._session.rm(source, user=self._user) + return + + staging = moved_destination.with_name(f".apply_patch-{uuid4().hex}.tmp") + try: + await self._write_text(staging, text) + await self._session.mv(staging, moved_destination, user=self._user) + except BaseException: + with contextlib.suppress(Exception): + await self._session.rm(staging, user=self._user) + raise + same_entry = await self._session.same_file( + source, moved_destination, follow_symlinks=False, user=self._user + ) + if same_entry and source.name != moved_destination.name: + # On case-folding APFS, replacing an existing entry through a case-variant path + # updates its content but keeps its old spelling. Moving that same entry performs + # the requested case-only rename without touching the committed content. + await self._session.mv(source, moved_destination, user=self._user) + elif not same_entry: + raise ApplyPatchDiffError( + message=( + "Move destination was updated, but source and destination no longer identify " + "the same file; source was left untouched" + ), + ) + async def _write_text(self, destination: Path, text: str) -> None: await self._session.mkdir(destination.parent, parents=True, user=self._user) await self._session.write( diff --git a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py index 01f3900160..fff616da45 100644 --- a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py +++ b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py @@ -191,6 +191,52 @@ def listing(self, path: Path) -> list[dict[str, str | int]]: finally: os.close(fd) + def rename(self, source: Path, destination: Path) -> None: + """Rename a regular file or symlink, replacing the destination entry. + + This is a rename of the directory entry itself: a symlink leaf is moved, not its + target, and a destination that is an existing directory is an error rather than a + place to put the source. On a filesystem that folds case, renaming an entry to another + spelling of its own name changes the stored spelling. + """ + with ( + self.parent(source, for_write=True) as (source_fd, source_name), + self.parent(destination, for_write=True) as (destination_fd, destination_name), + ): + mode = os.stat(source_name, dir_fd=source_fd, follow_symlinks=False).st_mode + if not (stat.S_ISREG(mode) or stat.S_ISLNK(mode)): + raise OSError( + errno.EINVAL, "Move source must be a regular file or symlink", str(source) + ) + os.rename( + source_name, + destination_name, + src_dir_fd=source_fd, + dst_dir_fd=destination_fd, + ) + + def same_file(self, left: Path, right: Path, *, follow_symlinks: bool = True) -> bool: + """Return whether two paths name one file, by device and inode. + + The filesystem answers this, not a string comparison: on a volume that folds case, or + Unicode normalization, two spellings can be one entry. A path that does not exist is + not the same file as anything. With no-follow enabled, symlink leaves return false. + """ + try: + with ( + self.parent(left) as (left_fd, left_name), + self.parent(right) as (right_fd, right_name), + ): + left_stat = os.stat(left_name, dir_fd=left_fd, follow_symlinks=follow_symlinks) + right_stat = os.stat(right_name, dir_fd=right_fd, follow_symlinks=follow_symlinks) + except FileNotFoundError: + return False + if not follow_symlinks and ( + stat.S_ISLNK(left_stat.st_mode) or stat.S_ISLNK(right_stat.st_mode) + ): + return False + return os.path.samestat(left_stat, right_stat) + def _entry(path: Path, entry: os.stat_result) -> dict[str, str | int]: try: @@ -240,12 +286,14 @@ def _remove_at(parent_fd: int, name: str, *, recursive: bool) -> None: def _main() -> None: # The application supplies this code and an authorized path directly, never via the workspace. - operation, raw_path = sys.argv[1:] + operation, *raw_paths = sys.argv[1:] files = _FileOps() - path = Path(raw_path) + paths = [Path(raw_path) for raw_path in raw_paths] if operation == "write": + (path,) = paths files.write(path, cast(io.IOBase, sys.stdin.buffer)) elif operation == "write_new": + (path,) = paths try: files.write_new(path, cast(io.IOBase, sys.stdin.buffer)) except FileExistsError: @@ -254,7 +302,15 @@ def _main() -> None: except _IncompleteCreateError: sys.exit(_INCOMPLETE_CREATE_EXIT_CODE) elif operation == "ls": + (path,) = paths print(json.dumps(files.listing(path), ensure_ascii=True)) + elif operation == "rename": + source, destination = paths + files.rename(source, destination) + elif operation == "same_file": + left, right = paths + follow_symlinks = sys.stdin.buffer.read() != b"0" + print(json.dumps(files.same_file(left, right, follow_symlinks=follow_symlinks))) else: raise ValueError("Unsupported UnixLocal file operation") diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index a722da1bba..9c67741374 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1071,6 +1071,102 @@ async def write( except OSError as e: raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e + async def mv( + self, + source: Path | str, + destination: Path | str, + *, + user: str | User | None = None, + ) -> None: + # A rename of the entry, descriptor-relative like every other file operation here, + # so the paths validated above are the ones acted on. `os.rename` never puts the + # source inside an existing directory the way `mv` does; it fails instead. + normalized_source = self._normalize_entry_path(source, for_write=True) + normalized_destination = self._normalize_entry_path(destination, for_write=True) + command = ("mv", "-f", "--", str(normalized_source), str(normalized_destination)) + if user is not None: + try: + result = await self._run_file_operation_as_user( + "rename", normalized_source, normalized_destination, user=user + ) + except OSError as e: + raise ExecNonZeroError( + ExecResult(stdout=b"", stderr=str(e).encode("utf-8"), exit_code=1), + command=command, + cause=e, + ) from e + if result.returncode: + raise ExecNonZeroError( + ExecResult( + stdout=result.stdout, stderr=result.stderr, exit_code=result.returncode + ), + command=command, + ) + return + try: + self._files.rename(normalized_source, normalized_destination) + except OSError as e: + raise ExecNonZeroError( + ExecResult(stdout=b"", stderr=str(e).encode("utf-8"), exit_code=1), + command=command, + cause=e, + ) from e + + async def same_file( + self, + left: Path | str, + right: Path | str, + *, + follow_symlinks: bool = True, + user: str | User | None = None, + ) -> bool: + normalize = self.normalize_path if follow_symlinks else self._normalize_entry_path + normalized_left = normalize(left) + normalized_right = normalize(right) + command = ("test", str(normalized_left), "-ef", str(normalized_right)) + if user is not None: + try: + result = await self._run_file_operation_as_user( + "same_file", + normalized_left, + normalized_right, + user=user, + payload=b"1" if follow_symlinks else b"0", + ) + except OSError as e: + raise ExecNonZeroError( + ExecResult(stdout=b"", stderr=str(e).encode("utf-8"), exit_code=1), + command=command, + cause=e, + ) from e + if result.returncode: + raise ExecNonZeroError( + ExecResult( + stdout=result.stdout, stderr=result.stderr, exit_code=result.returncode + ), + command=command, + ) + return bool(json.loads(result.stdout)) + try: + return self._files.same_file( + normalized_left, normalized_right, follow_symlinks=follow_symlinks + ) + except OSError as e: + raise ExecNonZeroError( + ExecResult(stdout=b"", stderr=str(e).encode("utf-8"), exit_code=1), + command=command, + cause=e, + ) from e + + def _normalize_entry_path(self, path: Path | str, *, for_write: bool = False) -> Path: + # Resolve and authorize the parent without following the leaf that rename/stat owns. + self._files.configure(self.state.manifest) + lexical = Path(path) + if not lexical.is_absolute(): + lexical = self._workspace_path_policy().absolute_workspace_path(path) + resolved = lexical.parent.resolve(strict=False) / lexical.name + return self._files.authorize(resolved, for_write=for_write) + async def _write_new_file( self, path: Path, @@ -1167,14 +1263,15 @@ async def _write_stream_with_exec( async def _run_file_operation_as_user( self, - operation: Literal["ls", "write", "write_new"], + operation: Literal["ls", "write", "write_new", "rename", "same_file"], path: Path, - *, + *more_paths: Path, user: str | User, payload: bytes = b"", ) -> subprocess.CompletedProcess[bytes]: # Authorization is synchronous and captured for this operation before dispatch. - path = self._files.authorize(path, for_write=operation != "ls") + for_write = operation in ("write", "write_new", "rename") + paths = [self._files.authorize(each, for_write=for_write) for each in (path, *more_paths)] command = self._prepare_exec_command( "python3", "-I", @@ -1182,7 +1279,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 ea54de925e..115172eb21 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -1217,6 +1217,100 @@ async def rm( if not result.ok(): raise ExecNonZeroError(result, command=cmd) + async def mv( + self, + source: Path | str, + destination: Path | str, + *, + user: str | User | None = None, + ) -> None: + """Rename a 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 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: 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 + 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) + 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", "-c", "", 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. With ``follow_symlinks=False``, return false if either + leaf is a symlink, including two paths naming the same symlink. This mode compares + only non-symlink entries and is used before removing an apply-patch source. + + 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, return false when either leaf is a symlink. + :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 90f5a59975..468afacd9d 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 16ec0f41b9..cc70de8f29 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, PurePath, 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 @@ -50,6 +64,8 @@ async def write( ) -> None: _ = user normalized = self.normalize_path(path) + if normalized in self.directories: + raise IsADirectoryError(normalized) payload = data.read() if isinstance(payload, str): self.files[normalized] = payload.encode("utf-8") @@ -107,6 +123,328 @@ 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 _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. + + 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 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. + + 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(CaseFoldingApplyPatchSession): + """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: @@ -126,6 +464,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: @@ -164,3 +504,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 0cfc69b3cf..7a7d32ed43 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -1,7 +1,9 @@ from __future__ import annotations import io -from pathlib import Path +from pathlib import Path, PurePosixPath +from typing import cast +from unittest.mock import AsyncMock, MagicMock import pytest @@ -14,10 +16,18 @@ ApplyPatchPathError, WorkspaceReadNotFoundError, ) +from agents.sandbox.session.sandbox_session import SandboxSession from agents.sandbox.types import User from tests.sandbox._apply_patch_test_session import ( ApplyPatchSession, + CaseFoldingApplyPatchSession, + CaseFoldingHostApplyPatchSession, + ConcurrentWriterApplyPatchSession, + NormalizationFoldingApplyPatchSession, + ParentAliasApplyPatchSession, + PosixHostApplyPatchSession, ProviderNotFoundApplyPatchSession, + WriteFailureApplyPatchSession, ) @@ -250,6 +260,302 @@ 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_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.""" + 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.""" + 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: + """Ordinary moves write the destination before removing the source, without staging.""" + + class OrderedSession(PosixHostApplyPatchSession): + async def rm(self, path, *, recursive=False, user=None): + assert self.files[PurePosixPath("/workspace/Notes.txt")] == b"alpha\ngamma\n" + await super().rm(path, recursive=recursive, user=user) + + session = OrderedSession() + 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.mv_calls == [] + 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 == [] + 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 = CaseFoldingApplyPatchSession() + long_name = "n" * 250 + ".txt" + session.files[cast(Path, PurePosixPath(f"/workspace/{long_name}"))] = b"alpha\nbeta\n" + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", + path=long_name, + diff="@@\n alpha\n-beta\n+gamma\n", + move_to=long_name.upper(), + ) + ) + + assert len(session.mv_calls) == 2 + staging, _ = session.mv_calls[0] + assert len(staging.name.encode("utf-8")) <= 255 + assert session.files == {PurePosixPath(f"/workspace/{long_name.upper()}"): b"alpha\ngamma\n"} + + @pytest.mark.asyncio async def test_apply_patch_allows_absolute_path_within_root() -> None: session = ApplyPatchSession() @@ -417,6 +723,142 @@ 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 + + from agents.sandbox.errors import ExecNonZeroError + from agents.sandbox.manifest import Manifest + from agents.sandbox.session.base_sandbox_session import BaseSandboxSession + from agents.sandbox.types import ExecResult + + executable = shutil.which("gmv") or (shutil.which("mv") if sys.platform == "linux" else None) + if executable is None: + pytest.skip("requires a GNU-compatible mv") + source = tmp_path / "source" + source.write_bytes(b"original") + destination = tmp_path / "destination" + + class ShellSession(ApplyPatchSession): + mv = BaseSandboxSession.mv + + async def exec(self, *command, **kwargs): + # The directory arrives after validation but before the actual move. + destination.mkdir() + result = subprocess.run( + [ + "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 + ) + + with pytest.raises(ExecNonZeroError): + await ShellSession(Manifest(root=str(tmp_path))).mv(source, destination) + assert source.read_bytes() == b"original" + assert list(destination.iterdir()) == [] + + +@pytest.mark.asyncio +async def test_fallback_move_replaces_a_directory_symlink(tmp_path: Path) -> None: + import shlex + import shutil + import subprocess + import sys + + from agents.sandbox.manifest import Manifest + from agents.sandbox.session.base_sandbox_session import BaseSandboxSession + from agents.sandbox.types import ExecResult + + executable = shutil.which("gmv") or (shutil.which("mv") if sys.platform == "linux" else None) + if executable is None: + pytest.skip("requires a GNU-compatible mv") + source = tmp_path / "source" + source.write_bytes(b"replacement") + directory = tmp_path / "directory" + directory.mkdir() + link = tmp_path / "link" + link.symlink_to(directory, target_is_directory=True) + + class ShellSession(ApplyPatchSession): + mv = BaseSandboxSession.mv + + async def exec(self, *command, **kwargs): + result = subprocess.run( + [ + "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 + ) + + await ShellSession(Manifest(root=str(tmp_path))).mv(source, link) + assert not link.is_symlink() + 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() + + async def test_apply_patch_create_rejects_an_existing_file() -> None: session = ApplyPatchSession() session.files[Path("/workspace/notes.txt")] = b"alpha\n" diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index b06430d169..44e166debd 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -770,6 +770,85 @@ def _slow_extract(tar: object, **kwargs: object) -> None: 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: + """A directory destination is refused, and the source is where it was. + + The destination write must fail before the editor removes the source. + """ + 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(WorkspaceArchiveWriteError): + 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 + + def _exclusive_write_session(root: Path) -> UnixLocalSandboxSession: return UnixLocalSandboxSession( state=UnixLocalSandboxSessionState( diff --git a/tests/sandbox/test_unix_local_file_io.py b/tests/sandbox/test_unix_local_file_io.py index cc9923ec47..67c450db7f 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, @@ -26,6 +27,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, @@ -43,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: @@ -59,10 +110,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: @@ -79,7 +136,8 @@ async def test_parent_swap_after_validation_cannot_access_outside( else: (root / "target").write_bytes(b"original content") session = _session(workspace) - normalize = session.normalize_path + boundary = "_normalize_entry_path" if operation in {"mv", "same_file"} else "normalize_path" + normalize = getattr(session, boundary) swapped = False def swap(path: Path | str, *, for_write: bool = False) -> Path: @@ -92,7 +150,7 @@ def swap(path: Path | str, *, for_write: bool = False) -> Path: return result # Suspend at the check/use boundary without replacing the actual OS file operations. - monkeypatch.setattr(session, "normalize_path", swap) + monkeypatch.setattr(session, boundary, swap) # The exclusive create authorizes through the descriptor-relative file ops rather than # session.normalize_path, so the patch case injects at that boundary instead. @@ -124,7 +182,9 @@ def swap_authorize(path: Path, *, for_write: bool = False) -> Path: assert sentinel.read_bytes() == b"original content" -@pytest.mark.parametrize("operation", ["read", "write", "mkdir", "ls", "rm", "rmtree"]) +@pytest.mark.parametrize( + "operation", ["read", "write", "mkdir", "ls", "rm", "rmtree", "mv", "same_file"] +) async def test_leaf_swap_does_not_follow_new_symlink( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, operation: str ) -> None: @@ -140,16 +200,23 @@ async def test_leaf_swap_does_not_follow_new_symlink( else: path.write_bytes(b"original content") session = _session(workspace) - normalize = session.normalize_path + boundary = "_normalize_entry_path" if operation in {"mv", "same_file"} else "normalize_path" + normalize = getattr(session, boundary) + swapped = False def swap(path: Path | str, *, for_write: bool = False) -> Path: + nonlocal swapped result = normalize(path, for_write=for_write) - target.rename(workspace / "original") - target.symlink_to(outside, target_is_directory=directory) + if not swapped: + swapped = True + target.rename(workspace / "original") + target.symlink_to(outside, target_is_directory=directory) return result - monkeypatch.setattr(session, "normalize_path", swap) - if operation in {"rm", "rmtree"}: + monkeypatch.setattr(session, boundary, swap) + 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( @@ -433,3 +500,268 @@ 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 + + +@pytest.mark.parametrize("user", [None, "example-user"]) +async def test_public_entry_operations_preserve_leaf_symlinks( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, user: str | None +) -> None: + from agents.sandbox.sandboxes import unix_local + from agents.sandbox.session.sandbox_session import SandboxSession + from tests.sandbox.test_unix_local_user_file_io import _worker + + workspace = tmp_path / "workspace" + workspace.mkdir() + target = workspace / "target" + target.write_bytes(b"untouched") + link = workspace / "link" + link.symlink_to("target") + parent_alias = workspace / "alias" + parent_alias.symlink_to(".", target_is_directory=True) + session = SandboxSession(_session(workspace)) + if user is not None: + monkeypatch.setattr(unix_local.shutil, "which", lambda command: "/usr/bin/sudo") + monkeypatch.setattr(unix_local.subprocess, "run", _worker) + + assert await session.same_file("link", "target", user=user) + assert not await session.same_file("link", "target", follow_symlinks=False, user=user) + assert not await session.same_file("link", "link", follow_symlinks=False, user=user) + await session.mv("alias/link", "moved", user=user) + assert not link.is_symlink() + assert (workspace / "moved").is_symlink() + assert target.read_bytes() == b"untouched" + (workspace / "replacement").write_bytes(b"replacement") + await session.mv("replacement", "moved", user=user) + assert not (workspace / "moved").is_symlink() + assert (workspace / "moved").read_bytes() == b"replacement" + assert target.read_bytes() == b"untouched" + + +@pytest.mark.parametrize("mode", [0o755, 0o600]) +async def test_apply_patch_preserves_existing_move_destination(tmp_path: Path, mode: int) -> None: + workspace = tmp_path / "workspace" + workspace.mkdir() + source = workspace / "source" + source.write_text("before\n") + destination = workspace / "destination" + destination.write_text("old destination\n") + destination.chmod(mode) + before = destination.stat() + + await _session(workspace).apply_patch( + ApplyPatchOperation( + type="update_file", path="source", move_to="destination", diff="@@\n-before\n+after\n" + ) + ) + + assert destination.read_text() == "after\n" + assert destination.stat().st_ino == before.st_ino + assert destination.stat().st_mode == before.st_mode + assert not source.exists() + + +async def test_apply_patch_can_write_existing_destination_in_protected_directory( + tmp_path: Path, +) -> None: + if os.geteuid() == 0: + pytest.skip("root bypasses directory permissions") + workspace = tmp_path / "workspace" + workspace.mkdir() + source = workspace / "source" + source.write_text("before\n") + protected = workspace / "protected" + protected.mkdir() + destination = protected / "destination" + destination.write_text("old\n") + protected.chmod(0o555) + try: + await _session(workspace).apply_patch( + ApplyPatchOperation( + type="update_file", + path="source", + move_to="protected/destination", + diff="@@\n-before\n+after\n", + ) + ) + assert destination.read_text() == "after\n" + assert not source.exists() + finally: + protected.chmod(0o755) + + +async def test_apply_patch_move_under_aliased_workspace_root(tmp_path: Path) -> None: + workspace = tmp_path / "workspace" + workspace.mkdir() + alias = tmp_path / "alias" + alias.symlink_to(workspace, target_is_directory=True) + source = workspace / "source" + source.write_text("before\n") + session = _session(alias) + + await session.apply_patch( + ApplyPatchOperation( + type="update_file", path="source", move_to="destination", diff="@@\n-before\n+after\n" + ) + ) + + destination = workspace / "destination" + assert destination.read_text() == "after\n" + assert not source.exists() + assert await session.same_file(destination.resolve(), "destination", follow_symlinks=False) + await session.mv(destination.resolve(), "renamed") + assert (workspace / "renamed").read_text() == "after\n" + + +async def test_apply_patch_case_alias_on_real_filesystem(tmp_path: Path) -> None: + workspace = tmp_path / "workspace" + workspace.mkdir() + source = workspace / "notes.txt" + source.write_text("before\n") + if not (workspace / "Notes.txt").exists(): + pytest.skip("volume is case-sensitive") + await _session(workspace).apply_patch( + ApplyPatchOperation( + type="update_file", path="notes.txt", move_to="Notes.txt", diff="@@\n-before\n+after\n" + ) + ) + assert [entry.name for entry in workspace.iterdir()] == ["Notes.txt"] + assert (workspace / "Notes.txt").read_text() == "after\n" + + +async def test_entry_operations_honor_exact_file_grants(tmp_path: Path) -> None: + workspace = tmp_path / "workspace" + workspace.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + source = outside / "source" + destination = outside / "destination" + source.write_bytes(b"granted") + readonly = outside / "readonly" + readonly.write_bytes(b"read only") + session = _session( + workspace, + grants=( + SandboxPathGrant(path=str(source)), + SandboxPathGrant(path=str(destination)), + SandboxPathGrant(path=str(readonly), read_only=True), + ), + ) + assert await session.same_file(source, source, follow_symlinks=False) + with pytest.raises(WorkspaceArchiveWriteError): + await session.mv(source, readonly) + with pytest.raises(InvalidManifestPathError): + await session.mv(source, outside / "ungranted") + await session.mv(source, destination) + 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 053bf1e5e2..8825ee0e7e 100644 --- a/tests/sandbox/test_unix_local_user_file_io.py +++ b/tests/sandbox/test_unix_local_user_file_io.py @@ -118,6 +118,119 @@ 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) + 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")) + 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"))) + 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: + await session.mv(Path("file"), Path("docs"), user="example-user") + assert dispatch.call_count == 1 + assert b"Is a directory" in error.value.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] + original_stat = os.stat + entries = iter(stats) + stat_mock = Mock( + side_effect=lambda *args, **kwargs: ( + next(entries) if "dir_fd" in kwargs else original_stat(*args, **kwargs) + ) + ) + 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 + if "dir_fd" in call.kwargs + ] == [ + follow_symlinks, + follow_symlinks, + ] + + @pytest.mark.asyncio @pytest.mark.parametrize("failure", [errno.EACCES, errno.ELOOP]) @pytest.mark.parametrize("operation", ["ls", "write"]) @@ -218,13 +331,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]