diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 30623fdf82..5e6f8d98fc 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -127,7 +127,11 @@ async def apply_operation( path=operation.path, cause=exc, ) from exc - await self._write_text(destination, created_text) + # Hand over the unresolved path. destination has already been through + # normalize_path(), which resolves leaf symlinks on some backends, so passing + # it would ask the backend to create the link target instead of the requested + # name and a dangling link would be reported as a successful create. + await self._write_new_text(relative_path, created_text, display_path=display_path) return ApplyPatchResult(output=f"Created {display_path}") raise ApplyPatchDiffError( @@ -186,6 +190,25 @@ async def _ensure_exists(self, destination: Path, *, display_path: str) -> None: else: handle.close() + async def _write_new_text(self, destination: Path, text: str, *, display_path: str) -> None: + # Add File is documented as creating a new file, so the name is claimed + # exclusively by the backend rather than checked and then overwritten. + try: + await self._session.write_new_file( + destination, + io.BytesIO(text.encode("utf-8")), + user=self._user, + ) + except FileExistsError as exc: + raise ApplyPatchDiffError( + message=( + f"apply_patch cannot create {display_path} because it already exists. " + "Use an update_file operation to change an existing file." + ), + path=display_path, + cause=exc, + ) from exc + async def _read_text(self, destination: Path, *, op_path: str, decode_path: Path) -> str: try: handle = await self._session.read(destination, user=self._user) diff --git a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py index 90d228b4ac..cde4832acb 100644 --- a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py +++ b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py @@ -25,6 +25,9 @@ ) +_EXISTING_TARGET_EXIT_CODE = 13 + + class _FileOps: """Operate on canonical absolute paths already authorized by the owning session.""" @@ -76,6 +79,28 @@ def write(self, path: Path, stream: io.IOBase) -> None: with out: shutil.copyfileobj(stream, out) + def write_new(self, path: Path, stream: io.IOBase) -> None: + """Create a file that must not already exist. + + O_EXCL fails with EEXIST when the name is taken by anything, including a directory + or a dangling symlink, and it claims the name in the same syscall that creates the + file, so a concurrent creator either loses the race or keeps its own content. + """ + with self.parent(path, for_write=True, create_parents=True) as (parent_fd, name): + fd = os.open( + name, + os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW, + 0o666, + dir_fd=parent_fd, + ) + try: + out = os.fdopen(fd, "wb") + except BaseException: + os.close(fd) + raise + with out: + shutil.copyfileobj(stream, out) + def mkdir(self, path: Path, *, parents: bool) -> None: with self.parent(path, for_write=True, create_parents=parents) as (parent_fd, name): try: @@ -167,6 +192,12 @@ def _main() -> None: path = Path(raw_path) if operation == "write": files.write(path, cast(io.IOBase, sys.stdin.buffer)) + elif operation == "write_new": + try: + files.write_new(path, cast(io.IOBase, sys.stdin.buffer)) + except FileExistsError: + # A distinct status keeps "already exists" separable from a real write failure. + sys.exit(_EXISTING_TARGET_EXIT_CODE) elif operation == "ls": print(json.dumps(files.listing(path), ensure_ascii=True)) else: diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index ad25e630ef..5013bf0269 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1054,6 +1054,60 @@ async def write( except OSError as e: raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e + async def write_new_file( + self, + path: Path, + data: io.IOBase, + *, + user: str | User | None = None, + ) -> None: + payload = coerce_write_payload(path=path, data=data) + # Resolve the parent the way the ordinary write path does, so a supported internal + # symlink such as "internal -> real" still works, then keep the leaf name + # unresolved so the file ops open it with O_NOFOLLOW and a symlink at the target + # name is rejected rather than followed. + requested = Path(path) + target = self.normalize_path(requested.parent, for_write=True) / requested.name + if user is not None: + await self._write_new_stream_with_exec(target, payload.stream, user=user) + return + + try: + self._files.write_new(target, payload.stream) + except FileExistsError: + raise + except OSError as e: + raise WorkspaceArchiveWriteError(path=target, cause=e) from e + + async def _write_new_stream_with_exec( + self, + path: Path, + stream: io.IOBase, + *, + user: str | User, + ) -> None: + payload = stream.read() + if isinstance(payload, str): + payload = payload.encode("utf-8") + elif not isinstance(payload, bytes): + payload = bytes(payload) + try: + result = await self._run_file_operation_as_user( + "write_new", path, user=user, payload=payload + ) + except OSError as e: + raise WorkspaceArchiveWriteError(path=path, cause=e) from e + if result.returncode == _unix_local_file_ops._EXISTING_TARGET_EXIT_CODE: + raise FileExistsError(str(path)) + if result.returncode: + raise WorkspaceArchiveWriteError( + path=path, + context={ + "stderr": result.stderr.decode("utf-8", errors="replace"), + "operation": "write_new", + }, + ) + async def _write_stream_with_exec( self, path: Path, @@ -1083,14 +1137,14 @@ async def _write_stream_with_exec( async def _run_file_operation_as_user( self, - operation: Literal["ls", "write"], + operation: Literal["ls", "write", "write_new"], path: Path, *, user: str | User, payload: bytes = b"", ) -> subprocess.CompletedProcess[bytes]: # Authorization is synchronous and captured for this operation before dispatch. - path = self._files.authorize(path, for_write=operation == "write") + path = self._files.authorize(path, for_write=operation != "ls") command = self._prepare_exec_command( "python3", "-I", diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index d377bea9ef..d6079f2a97 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -149,6 +149,12 @@ fi done """.strip() +_EXISTING_TARGET_EXIT_CODE = 13 +# A bare existence test. It needs only execute permission on the parent, never reads the +# target, and reports absent when the parent itself is missing, so a nested create still +# reaches write() and lets the backend create the parents. +_TARGET_EXISTS_SCRIPT = 'if [ -e "$1" ] || [ -L "$1" ]; then exit 13; fi\n' + _WRITE_ACCESS_CHECK_SCRIPT = ( 'target="$1"\n' 'if [ -e "$target" ]; then\n' @@ -945,6 +951,55 @@ async def write( :param user: Optional sandbox user to perform the write as. """ + async def write_new_file( + self, + path: Path, + data: io.IOBase, + *, + user: str | User | None = None, + ) -> None: + """Write a file that must not already exist. + + This default checks the target and then writes, so it rejects the ordinary case of + a create aimed at a path that is already occupied and leaves the existing content + alone. It is not atomic: a creator that arrives between the check and the write is + overwritten. A backend that can express an exclusive create should override this + and claim the name in one step; ``UnixLocalSandboxSession`` does. + + :param path: Absolute path in the container or path relative to the + workspace root. + :param data: A file-like object positioned at the start of the payload. + :param user: Optional sandbox user to perform the write as. + :raises FileExistsError: If the path already exists. + """ + workspace_path = await self._validate_path_access(path, for_write=True) + path_arg = sandbox_path_str(workspace_path) + # The probe reports absent as 0, including when the parent does not exist, so 0 is + # the only status that may proceed. Any other status means the probe itself did not + # run, and write() can still succeed through a separate upload API on provider + # sessions, which would overwrite an existing target exactly when the precondition + # could not be checked. + probe = await self.exec( + "sh", "-c", _TARGET_EXISTS_SCRIPT, "sh", path_arg, shell=False, user=user + ) + if probe.exit_code == _EXISTING_TARGET_EXIT_CODE: + raise FileExistsError(path_arg) + if probe.exit_code != 0: + raise WorkspaceArchiveWriteError( + path=workspace_path, + context={ + "command": ["sh", "-c", "", path_arg], + "exit_code": probe.exit_code, + "stdout": probe.stdout.decode("utf-8", errors="replace"), + "stderr": probe.stderr.decode("utf-8", errors="replace"), + }, + ) + # Create the parents explicitly, the way the previous create path did, so the call + # sequence a caller can observe is unchanged and backends that do not create them + # during write() still work. + await self.mkdir(workspace_path.parent, parents=True, user=user) + await self.write(workspace_path, data, user=user) + async def _check_read_with_exec( self, path: Path | str, *, user: str | User | None = None ) -> Path: diff --git a/src/agents/sandbox/session/sandbox_session.py b/src/agents/sandbox/session/sandbox_session.py index 923f025857..593ff1054a 100644 --- a/src/agents/sandbox/session/sandbox_session.py +++ b/src/agents/sandbox/session/sandbox_session.py @@ -677,6 +677,19 @@ async def write( ) -> None: await self._inner.write(path, data, user=user) + @instrumented_op("write", data=_write_start_data) + async def write_new_file( + self, + path: Path, + data: io.IOBase, + *, + user: str | User | None = None, + ) -> None: + # Forwarded so a backend with a native exclusive-create primitive is actually + # used. Without this the wrapper would fall back to the shared implementation and + # bypass the inner session's override. + await self._inner.write_new_file(path, data, user=user) + @instrumented_op( "running", finish_data=_running_finish_data, diff --git a/src/agents/testing/sandbox.py b/src/agents/testing/sandbox.py index d086ae7c9d..dcf7dde587 100644 --- a/src/agents/testing/sandbox.py +++ b/src/agents/testing/sandbox.py @@ -331,6 +331,23 @@ def __init__(self, steps: Sequence[_SandboxStep], *, manifest: Manifest | None) self._calls: list[SandboxCall] = [] self._running = False + async def write_new_file( + self, + path: Path, + data: io.IOBase, + *, + user: str | User | None = None, + ) -> None: + # A scripted session has no filesystem to hold a colliding entry, so an exclusive + # create is scripted as the same mkdir and write pair the ordinary create path uses. + # Delegating here also keeps the inherited default from reaching for `exec`, which a + # script that only configures file steps hides. Normalize first: the create path + # hands over an unresolved path so a symlinked leaf is not followed, but a script + # matching on arguments expects the workspace paths these calls have always carried. + target = self.normalize_path(path) + await self.mkdir(target.parent, parents=True, user=user) + await self.write(target, data, user=user) + def __getattribute__(self, name: str) -> Any: if name in _SCRIPTABLE_METHODS: configured = object.__getattribute__(self, "_configured_methods") diff --git a/tests/sandbox/_apply_patch_test_session.py b/tests/sandbox/_apply_patch_test_session.py index 24ce567011..911581b707 100644 --- a/tests/sandbox/_apply_patch_test_session.py +++ b/tests/sandbox/_apply_patch_test_session.py @@ -56,6 +56,20 @@ async def write( else: self.files[normalized] = bytes(payload) + async def write_new_file( + self, + path: Path, + data: io.IOBase, + *, + user: str | User | None = None, + ) -> None: + normalized = self.normalize_path(path) + if normalized in self.files: + raise FileExistsError(str(normalized)) + # Real backends create the parents inside the primitive, so record that here too. + await self.mkdir(normalized.parent, parents=True, user=user) + await self.write(path, data, user=user) + async def _exec_internal( self, *command: str | Path, diff --git a/tests/sandbox/_filesystem_test_session.py b/tests/sandbox/_filesystem_test_session.py index 83f1d4d400..ffe3f65a4a 100644 --- a/tests/sandbox/_filesystem_test_session.py +++ b/tests/sandbox/_filesystem_test_session.py @@ -21,7 +21,11 @@ UnixLocalSandboxSessionState, ) from agents.sandbox.session import SandboxSession -from agents.sandbox.session.base_sandbox_session import BaseSandboxSession +from agents.sandbox.session.base_sandbox_session import ( + _EXISTING_TARGET_EXIT_CODE, + _TARGET_EXISTS_SCRIPT, + BaseSandboxSession, +) from agents.sandbox.session.sandbox_session_state import SandboxSessionState from agents.sandbox.snapshot import NoopSnapshot, SnapshotBase, SnapshotSpec from agents.sandbox.types import ExecResult, Permissions, User @@ -56,6 +60,19 @@ async def _exec_internal( path = Path(command_parts[2]) exists = path.is_dir() if command_parts[1] == "-d" else path.is_file() return ExecResult(stdout=b"", stderr=b"", exit_code=0 if exists else 1) + # The exclusive-create probe is a bare existence test dispatched as `sh -c`. It has + # to see a dangling symlink as present, so it uses lexists rather than exists. + if ( + len(command_parts) == 5 + and command_parts[:2] == ("sh", "-c") + and command_parts[2] == _TARGET_EXISTS_SCRIPT + ): + present = os.path.lexists(command_parts[4]) + return ExecResult( + stdout=b"", + stderr=b"", + exit_code=_EXISTING_TARGET_EXIT_CODE if present else 0, + ) raise AssertionError(f"Unexpected filesystem test command: {command_parts!r}") @staticmethod diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index c4cd676fec..0cfc69b3cf 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -1,5 +1,6 @@ from __future__ import annotations +import io from pathlib import Path import pytest @@ -11,7 +12,9 @@ ApplyPatchDiffError, ApplyPatchFileNotFoundError, ApplyPatchPathError, + WorkspaceReadNotFoundError, ) +from agents.sandbox.types import User from tests.sandbox._apply_patch_test_session import ( ApplyPatchSession, ProviderNotFoundApplyPatchSession, @@ -411,3 +414,50 @@ async def test_apply_patch_mapping_operation_rejects_non_string_move_to() -> Non ) assert session.files[Path("/workspace/old.txt")] == b"alpha\n" + + +@pytest.mark.asyncio +async def test_apply_patch_create_rejects_an_existing_file() -> None: + session = ApplyPatchSession() + session.files[Path("/workspace/notes.txt")] = b"alpha\n" + + with pytest.raises(ApplyPatchDiffError): + await session.apply_patch( + ApplyPatchOperation( + type="create_file", + path="notes.txt", + diff="+beta\n", + ) + ) + + assert session.files[Path("/workspace/notes.txt")] == b"alpha\n" + + +class _AlwaysMissingReadApplyPatchSession(ApplyPatchSession): + """Reports every path as missing while still holding the file. + + This stands in for a backend that provides an exclusive create. A create that only + probed with read() would be told the path is free and would overwrite the stored + content, so this pins the rejection to the backend primitive rather than to a probe. + """ + + async def read(self, path: Path, *, user: str | User | None = None) -> io.BytesIO: + _ = (path, user) + raise WorkspaceReadNotFoundError(path=path) + + +@pytest.mark.asyncio +async def test_apply_patch_create_rejects_an_existing_file_without_reading_it() -> None: + session = _AlwaysMissingReadApplyPatchSession() + session.files[Path("/workspace/notes.txt")] = b"alpha\n" + + with pytest.raises(ApplyPatchDiffError): + await session.apply_patch( + ApplyPatchOperation( + type="create_file", + path="notes.txt", + diff="+beta\n", + ) + ) + + assert 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 9188b1fc33..696446c90c 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -2,7 +2,9 @@ import asyncio import io +import os import signal +import subprocess import tarfile import threading import time @@ -12,10 +14,15 @@ import pytest +from agents.editor import ApplyPatchOperation from agents.sandbox import SandboxPathGrant -from agents.sandbox.errors import PtySessionNotFoundError +from agents.sandbox.errors import ( + ApplyPatchDiffError, + PtySessionNotFoundError, + WorkspaceArchiveWriteError, +) from agents.sandbox.manifest import Environment, Manifest -from agents.sandbox.sandboxes import unix_local as unix_local_module +from agents.sandbox.sandboxes import _unix_local_file_ops, unix_local as unix_local_module from agents.sandbox.sandboxes.unix_local import ( UnixLocalSandboxClient, UnixLocalSandboxSession, @@ -24,6 +31,7 @@ ) from agents.sandbox.snapshot import NoopSnapshot from agents.sandbox.types import ExecResult, User +from tests.sandbox._filesystem_test_session import FilesystemTestSandboxSession class _RecordingUnixLocalSession(UnixLocalSandboxSession): @@ -610,3 +618,319 @@ 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 + + +def _exclusive_write_session(root: Path) -> UnixLocalSandboxSession: + return UnixLocalSandboxSession( + state=UnixLocalSandboxSessionState( + manifest=Manifest(root=str(root)), + snapshot=NoopSnapshot(id="noop"), + ) + ) + + +@pytest.mark.asyncio +async def test_write_new_file_keeps_an_intervening_creator_content(tmp_path: Path) -> None: + """The name is claimed by the write itself, so a creator that got there first wins.""" + session = _exclusive_write_session(tmp_path) + target = tmp_path / "notes.txt" + target.write_bytes(b"written by someone else\n") + + with pytest.raises(FileExistsError): + await session.write_new_file(Path("notes.txt"), io.BytesIO(b"clobbered")) + + assert target.read_bytes() == b"written by someone else\n" + + +@pytest.mark.asyncio +async def test_apply_patch_create_through_the_session_rejects_a_dangling_symlink( + tmp_path: Path, +) -> None: + """Drive the real caller path. + + WorkspaceEditor normalizes the destination before dispatching, and this backend + resolves leaf symlinks, so a create aimed at a dangling link used to land on the + link's absent target and report success. + """ + session = _exclusive_write_session(tmp_path) + (tmp_path / "link.txt").symlink_to(tmp_path / "missing.txt") + + with pytest.raises(ApplyPatchDiffError): + await session.apply_patch( + ApplyPatchOperation(type="create_file", path="link.txt", diff="+clobbered\n") + ) + + assert not (tmp_path / "missing.txt").exists() + assert (tmp_path / "link.txt").is_symlink() + + +@pytest.mark.asyncio +async def test_apply_patch_create_through_the_session_rejects_a_directory( + tmp_path: Path, +) -> None: + session = _exclusive_write_session(tmp_path) + (tmp_path / "adir").mkdir() + + with pytest.raises(ApplyPatchDiffError): + await session.apply_patch( + ApplyPatchOperation(type="create_file", path="adir", diff="+clobbered\n") + ) + + assert list((tmp_path / "adir").iterdir()) == [] + + +@pytest.mark.asyncio +async def test_apply_patch_create_through_the_session_keeps_existing_content( + tmp_path: Path, +) -> None: + session = _exclusive_write_session(tmp_path) + (tmp_path / "notes.txt").write_bytes(b"important\n") + + with pytest.raises(ApplyPatchDiffError): + await session.apply_patch( + ApplyPatchOperation(type="create_file", path="notes.txt", diff="+clobbered\n") + ) + + assert (tmp_path / "notes.txt").read_bytes() == b"important\n" + + +@pytest.mark.asyncio +async def test_apply_patch_create_through_the_session_writes_a_new_nested_file( + tmp_path: Path, +) -> None: + session = _exclusive_write_session(tmp_path) + + await session.apply_patch( + ApplyPatchOperation(type="create_file", path="nested/dir/new.txt", diff="+hello\n") + ) + + assert (tmp_path / "nested" / "dir" / "new.txt").read_text() == "hello" + assert not any(p.name.startswith(".") for p in (tmp_path / "nested" / "dir").iterdir()) + + +@pytest.mark.asyncio +async def test_apply_patch_create_through_the_session_reports_a_file_parent_as_a_write_error( + tmp_path: Path, +) -> None: + """A parent that is a regular file is not a collision on the requested name. + + Reporting it as one would tell the model to use update_file for a target that does + not exist and cannot be updated. + """ + session = _exclusive_write_session(tmp_path) + (tmp_path / "parent").write_bytes(b"i am a file\n") + + with pytest.raises(WorkspaceArchiveWriteError): + await session.apply_patch( + ApplyPatchOperation(type="create_file", path="parent/child.txt", diff="+hi\n") + ) + + +@pytest.mark.asyncio +async def test_apply_patch_create_accepts_a_destination_at_the_component_limit( + tmp_path: Path, +) -> None: + """Staging must not push a valid destination name past the filesystem's limit. + + Deriving the staging basename from the destination made it longer than the + destination itself, so a name the ordinary write path accepts failed to create. + """ + session = _exclusive_write_session(tmp_path) + long_name = "a" * 250 + ".txt" + # Confirm the platform really does accept this name, so the test fails for the + # right reason rather than because the limit is lower here. + probe = tmp_path / long_name + probe.write_text("probe") + probe.unlink() + + await session.apply_patch( + ApplyPatchOperation(type="create_file", path=long_name, diff="+hello\n") + ) + + assert (tmp_path / long_name).read_text() == "hello" + + +@pytest.mark.skipif(os.geteuid() == 0, reason="root bypasses directory write permissions") +@pytest.mark.asyncio +async def test_apply_patch_create_reports_collision_inside_a_read_only_parent( + tmp_path: Path, +) -> None: + """A visible collision must classify as a collision, not as a permission failure. + + Staging before classifying meant a target inside an executable but non-writable + parent failed on the staging write, so the caller was told the write failed instead + of being told to use update_file. + """ + session = _exclusive_write_session(tmp_path) + parent = tmp_path / "locked" + parent.mkdir() + target = parent / "notes.txt" + target.write_bytes(b"important\n") + parent.chmod(0o555) + try: + with pytest.raises(ApplyPatchDiffError): + await session.apply_patch( + ApplyPatchOperation( + type="create_file", path="locked/notes.txt", diff="+clobbered\n" + ) + ) + assert target.read_bytes() == b"important\n" + finally: + parent.chmod(0o755) + + +@pytest.mark.asyncio +async def test_write_new_file_maps_the_worker_exists_status(tmp_path: Path) -> None: + """The bound-user path runs the same exclusive create in a worker process. + + A real sandbox user is not available here, so this pins the status mapping: the + worker exits with a distinct code for an existing target, and the caller has to turn + that into FileExistsError rather than a generic write error. + """ + session = _exclusive_write_session(tmp_path) + recorded: list[str] = [] + + async def fake_worker( + operation: str, path: Path, *, user: object, payload: bytes = b"" + ) -> subprocess.CompletedProcess[bytes]: + _ = (path, user, payload) + recorded.append(operation) + return subprocess.CompletedProcess( + args=[], returncode=_unix_local_file_ops._EXISTING_TARGET_EXIT_CODE + ) + + session._run_file_operation_as_user = fake_worker # type: ignore[assignment,method-assign] + + with pytest.raises(FileExistsError): + await session.write_new_file( + Path("notes.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") + ) + + assert recorded == ["write_new"] + + +@pytest.mark.asyncio +async def test_base_default_create_probe_does_not_fetch_the_payload(tmp_path: Path) -> None: + """The inherited default must not read the target to decide a collision. + + read() eagerly fetches the whole payload on the remote backends that inherit the + default, so probing an existing large file would download it, and an existing file the + bound user cannot read would report a read failure instead of the collision. This uses + a filesystem double that does not override write_new_file, so it exercises the base. + """ + workspace = tmp_path / "workspace" + workspace.mkdir() + session = FilesystemTestSandboxSession( + state=UnixLocalSandboxSessionState( + manifest=Manifest(root=str(workspace)), + snapshot=NoopSnapshot(id="noop"), + ) + ) + reads: list[Path] = [] + original_read = session.read + + async def counting_read(path: Path, *, user: object = None) -> io.IOBase: + reads.append(Path(path)) + return await original_read(path, user=user) + + session.read = counting_read # type: ignore[assignment,method-assign] + (workspace / "notes.txt").write_bytes(b"important\n") + + with pytest.raises(ApplyPatchDiffError): + await session.apply_patch( + ApplyPatchOperation(type="create_file", path="notes.txt", diff="+clobbered\n") + ) + + assert reads == [] + assert (workspace / "notes.txt").read_bytes() == b"important\n" + + +@pytest.mark.asyncio +async def test_apply_patch_create_supports_a_symlinked_parent(tmp_path: Path) -> None: + """A supported internal symlink parent must still work. + + The ordinary write path resolves these safe aliases, so the exclusive create has to + resolve the parent too and keep only the leaf name unresolved. Passing the whole path + through unresolved made the file ops open the parent with O_NOFOLLOW and fail. + """ + session = _exclusive_write_session(tmp_path) + (tmp_path / "real").mkdir() + (tmp_path / "internal").symlink_to(tmp_path / "real", target_is_directory=True) + + await session.apply_patch( + ApplyPatchOperation(type="create_file", path="internal/new.txt", diff="+hello\n") + ) + + assert (tmp_path / "real" / "new.txt").read_text() == "hello" + + # The leaf is still unresolved, so a dangling link at the target name is rejected. + (tmp_path / "real" / "dangling.txt").symlink_to(tmp_path / "real" / "missing.txt") + with pytest.raises(ApplyPatchDiffError): + await session.apply_patch( + ApplyPatchOperation(type="create_file", path="internal/dangling.txt", diff="+x\n") + ) + assert not (tmp_path / "real" / "missing.txt").exists() + + +@pytest.mark.asyncio +async def test_base_default_create_allows_a_missing_parent(tmp_path: Path) -> None: + """A nested create must still reach write() when the parent does not exist yet. + + The probe reports absent for a missing parent instead of failing, so backends whose + write path creates parents keep working. Probing by listing the parent broke this + because the listing itself fails when the directory is not there. + """ + workspace = tmp_path / "workspace" + workspace.mkdir() + session = FilesystemTestSandboxSession( + state=UnixLocalSandboxSessionState( + manifest=Manifest(root=str(workspace)), + snapshot=NoopSnapshot(id="noop"), + ) + ) + + await session.apply_patch( + ApplyPatchOperation(type="create_file", path="newdir/file.txt", diff="+hello\n") + ) + + assert (workspace / "newdir" / "file.txt").read_text() == "hello" + + +class _ProbeFailureSession(FilesystemTestSandboxSession): + """Reports an unexpected status from the existence probe.""" + + async def _exec_internal( + self, + *command: str | Path, + timeout: float | None = None, + ) -> ExecResult: + _ = (command, timeout) + return ExecResult(stdout=b"", stderr=b"sh: not found", exit_code=127) + + +@pytest.mark.asyncio +async def test_base_default_create_fails_closed_when_the_probe_cannot_run( + tmp_path: Path, +) -> None: + """A probe that did not run must not be read as "absent". + + write() can still succeed through a separate upload API on provider sessions, so + treating an unexpected status as absent would overwrite an existing target exactly + when the precondition could not be checked. + """ + workspace = tmp_path / "workspace" + workspace.mkdir() + session = _ProbeFailureSession( + state=UnixLocalSandboxSessionState( + manifest=Manifest(root=str(workspace)), + snapshot=NoopSnapshot(id="noop"), + ) + ) + (workspace / "notes.txt").write_bytes(b"important\n") + + with pytest.raises(WorkspaceArchiveWriteError): + await session.apply_patch( + ApplyPatchOperation(type="create_file", path="notes.txt", diff="+clobbered\n") + ) + + assert (workspace / "notes.txt").read_bytes() == b"important\n" diff --git a/tests/sandbox/test_unix_local_file_io.py b/tests/sandbox/test_unix_local_file_io.py index 4f7095af29..cc9923ec47 100644 --- a/tests/sandbox/test_unix_local_file_io.py +++ b/tests/sandbox/test_unix_local_file_io.py @@ -93,6 +93,22 @@ def swap(path: Path | str, *, for_write: bool = False) -> Path: # Suspend at the check/use boundary without replacing the actual OS file operations. monkeypatch.setattr(session, "normalize_path", 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. + authorize = session._files.authorize + + def swap_authorize(path: Path, *, for_write: bool = False) -> Path: + nonlocal swapped + result = authorize(path, for_write=for_write) + if not swapped and for_write and result.name == "target": + swapped = True + parent.rename(workspace / "original") + parent.symlink_to(outside, target_is_directory=True) + return result + + if operation == "patch": + monkeypatch.setattr(session._files, "authorize", swap_authorize) with pytest.raises( ( OSError, diff --git a/tests/test_scripted_sandbox.py b/tests/test_scripted_sandbox.py index 247e0022fc..36216aff1b 100644 --- a/tests/test_scripted_sandbox.py +++ b/tests/test_scripted_sandbox.py @@ -10,7 +10,9 @@ from typing_extensions import assert_type from agents import RunConfig, Runner +from agents.editor import ApplyPatchOperation from agents.sandbox import ExecResult, Manifest, SandboxAgent +from agents.sandbox.apply_patch import WorkspaceEditor from agents.sandbox.capabilities import Shell from agents.sandbox.files import FileEntry from agents.sandbox.session.base_sandbox_session import BaseSandboxSession @@ -573,3 +575,30 @@ async def test_scripted_sandbox_drives_black_box_sandbox_agent_workflow() -> Non assert len(model.calls) == 2 session.assert_complete() model.assert_complete() + + +@pytest.mark.asyncio +async def test_scripted_sandbox_supports_apply_patch_create() -> None: + """An exclusive create has to stay scriptable with ordinary file steps. + + The inherited default probes with `exec`, and a script that configures only file + steps hides `exec`, so without a scripted implementation a previously valid + mkdir-and-write script fails with AttributeError. + """ + session = scripted_sandbox_session( + [{"method": "mkdir", "result": None}, {"method": "write", "result": None}] + ) + + result = await WorkspaceEditor(session).apply_operation( + ApplyPatchOperation(type="create_file", path="notes.txt", diff="+hello\n") + ) + + assert result.output == "Created notes.txt" + assert session.remaining_steps == 0 + # The recorded paths matter as much as the methods. The create path hands over an + # unresolved path so a symlinked leaf is not followed, and a script matching on + # arguments still expects the workspace paths these calls have always carried. + assert [(call.method, call.args[0]) for call in session.calls] == [ + ("mkdir", Path("/workspace")), + ("write", Path("/workspace/notes.txt")), + ]