diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 30623fdf82..24e10362e0 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,24 @@ 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: + # Backends with a native no-clobber primitive can reject occupied names. + 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/errors.py b/src/agents/sandbox/errors.py index 252b2a6f28..7fe331d83b 100644 --- a/src/agents/sandbox/errors.py +++ b/src/agents/sandbox/errors.py @@ -506,9 +506,10 @@ def __init__( context: Mapping[str, object] | None = None, cause: BaseException | None = None, retryable: bool | None = None, + message: str | None = None, ) -> None: super().__init__( - message=f"failed to write archive for path: {path}", + message=message if message is not None else f"failed to write archive for path: {path}", error_code=ErrorCode.WORKSPACE_ARCHIVE_WRITE_ERROR, op="write", context={"path": str(path), **_as_context(context)}, diff --git a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py index ffbc063c1d..01f3900160 100644 --- a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py +++ b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py @@ -26,6 +26,14 @@ ) +_EXISTING_TARGET_EXIT_CODE = 13 +_INCOMPLETE_CREATE_EXIT_CODE = 14 + + +class _IncompleteCreateError(OSError): + """The destination was claimed, but writing its contents did not complete.""" + + class _FileOps: """Operate on canonical absolute paths already authorized by the owning session.""" @@ -118,6 +126,34 @@ 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: + try: + out = os.fdopen(fd, "wb") + except BaseException: + os.close(fd) + raise + with out: + shutil.copyfileobj(stream, out) + except OSError as exc: + # Do not unlink the name: another workspace operation may have replaced it. + # Keep filesystem compatibility (no hard-link staging requirement) and let + # the caller inspect and recover the partial result explicitly. + raise _IncompleteCreateError("File creation did not complete") from exc + def mkdir(self, path: Path, *, parents: bool) -> None: with self.parent(path, for_write=True, create_parents=parents) as (parent_fd, name): try: @@ -209,6 +245,14 @@ 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) + except _IncompleteCreateError: + sys.exit(_INCOMPLETE_CREATE_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 928f255b1f..a722da1bba 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -110,6 +110,12 @@ logger = logging.getLogger(__name__) +_INCOMPLETE_CREATE_MESSAGE = ( + "File creation failed after claiming the destination. The file may contain incomplete " + "contents. Inspect the destination before using update_file or removing it to retry; " + "retrying Add File without inspection is unsafe." +) + def _mount_path_diagnostic_extra(mount_path: Path) -> dict[str, object]: return {"mount_path": str(mount_path)} @@ -1065,6 +1071,73 @@ 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 _unix_local_file_ops._IncompleteCreateError as e: + raise WorkspaceArchiveWriteError( + path=target, + cause=e, + retryable=False, + message=_INCOMPLETE_CREATE_MESSAGE, + ) from e + 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 == _unix_local_file_ops._INCOMPLETE_CREATE_EXIT_CODE: + raise WorkspaceArchiveWriteError( + path=path, + retryable=False, + message=_INCOMPLETE_CREATE_MESSAGE, + ) + 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, @@ -1094,14 +1167,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 1b7abb69cc..affccf0039 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -995,6 +995,23 @@ 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: + """Backend hook for apply_patch creation. + + The default retains the provider's existing mkdir/write semantics. UnixLocal + overrides this hook to claim the leaf exclusively; other backends need a native + primitive before they can offer the same guarantee. + """ + target = self.normalize_path(path) + await self.mkdir(target.parent, parents=True, user=user) + await self.write(target, 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 bea749e41c..90f5a59975 100644 --- a/src/agents/sandbox/session/sandbox_session.py +++ b/src/agents/sandbox/session/sandbox_session.py @@ -684,6 +684,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/tests/sandbox/_apply_patch_test_session.py b/tests/sandbox/_apply_patch_test_session.py index 24ce567011..16ec0f41b9 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/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 de24f3f3bf..b06430d169 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -15,8 +15,13 @@ import pytest +from agents.editor import ApplyPatchOperation from agents.sandbox import LocalSnapshotSpec, SandboxPathGrant -from agents.sandbox.errors import PtySessionNotFoundError, WorkspaceArchiveWriteError +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.unix_local import ( @@ -27,6 +32,7 @@ ) from agents.sandbox.snapshot import LocalSnapshot, NoopSnapshot from agents.sandbox.types import ExecResult, User +from tests.sandbox._filesystem_test_session import FilesystemTestSandboxSession class _RecordingUnixLocalSession(UnixLocalSandboxSession): @@ -764,6 +770,211 @@ def _slow_extract(tar: object, **kwargs: object) -> None: 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_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: + """A filename accepted by ordinary writes must still support Add File.""" + 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 reports the supported update alternative.""" + 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_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: + """Provider defaults retain the released mkdir/write behavior for nested creates.""" + 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" + + +@pytest.mark.asyncio +async def test_base_default_create_preserves_provider_write_semantics( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + session = FilesystemTestSandboxSession( + state=UnixLocalSandboxSessionState( + manifest=Manifest(root=str(tmp_path)), snapshot=NoopSnapshot(id="noop") + ) + ) + target = tmp_path / "existing.txt" + target.write_bytes(b"previous") + + async def no_new_probe(*args: object, **kwargs: object) -> ExecResult: + raise AssertionError("Creation must not add an exec requirement to providers") + + monkeypatch.setattr(session, "_exec_internal", no_new_probe) + await session.apply_patch( + ApplyPatchOperation(type="create_file", path="existing.txt", diff="+replacement\n") + ) + assert target.read_bytes() == b"replacement" + + @pytest.mark.asyncio async def test_client_delete_keeps_workspace_removal_off_the_event_loop( tmp_path: Path, monkeypatch: pytest.MonkeyPatch diff --git a/tests/sandbox/test_unix_local_create.py b/tests/sandbox/test_unix_local_create.py new file mode 100644 index 0000000000..3487e6d49d --- /dev/null +++ b/tests/sandbox/test_unix_local_create.py @@ -0,0 +1,185 @@ +"""Caller-visible no-clobber and partial-write recovery for UnixLocal Add File.""" + +from __future__ import annotations + +import errno +import io +import os +import shutil +import subprocess +import sys +from pathlib import Path +from types import SimpleNamespace +from typing import TYPE_CHECKING, Any + +import pytest + +from agents.editor import ApplyPatchOperation +from agents.sandbox.apply_patch import WorkspaceEditor +from agents.sandbox.errors import ApplyPatchDiffError, WorkspaceArchiveWriteError +from agents.sandbox.manifest import Manifest +from agents.sandbox.session import SandboxSession +from agents.sandbox.snapshot import NoopSnapshot + +if TYPE_CHECKING or sys.platform != "win32": + from agents.sandbox.sandboxes.unix_local import ( + UnixLocalSandboxSession, + UnixLocalSandboxSessionState, + ) + +pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="Unix only") + + +@pytest.fixture(params=["direct", "wrapped", "bound-user"]) +def editor( + request: pytest.FixtureRequest, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> WorkspaceEditor: + session = UnixLocalSandboxSession( + state=UnixLocalSandboxSessionState( + manifest=Manifest(root=str(tmp_path)), snapshot=NoopSnapshot(id="create-test") + ) + ) + if request.param == "direct": + return WorkspaceEditor(session) + if request.param == "wrapped": + return WorkspaceEditor(SandboxSession(session)) + + # Execute the exact shipped worker body under the current test identity. Only the + # sudo/process boundary is replaced; traversal, exclusive open and copy are real. + def worker(command: list[str], **kwargs: Any) -> subprocess.CompletedProcess[bytes]: + assert command[:4] == ["/usr/bin/sudo", "-u", "example-user", "--"] + assert command[4:8] == ["python3", "-I", "-S", "-c"] + assert command[9] == "write_new" + assert kwargs["env"] == {"PATH": os.defpath} + assert kwargs["cwd"] == "/" + namespace: dict[str, Any] = {"__name__": "test_worker"} + exec(compile(command[8], "", "exec"), namespace) + with pytest.MonkeyPatch.context() as patch: + patch.setattr(sys, "argv", ["-c", *command[9:]]) + patch.setattr(sys, "stdin", SimpleNamespace(buffer=io.BytesIO(kwargs["input"]))) + try: + namespace["_main"]() + except SystemExit as exc: + return subprocess.CompletedProcess(command, int(exc.code), b"", b"") + except OSError as exc: + return subprocess.CompletedProcess(command, 1, b"", str(exc).encode()) + return subprocess.CompletedProcess(command, 0, b"", b"") + + monkeypatch.setattr(shutil, "which", lambda _: "/usr/bin/sudo") + monkeypatch.setattr(subprocess, "run", worker) + return WorkspaceEditor(SandboxSession(session), user="example-user") + + +@pytest.mark.asyncio +async def test_create_preserves_a_creator_between_validation_and_open( + editor: WorkspaceEditor, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + target = tmp_path / "notes.txt" + original_open = os.open + intervened = False + + def intervening_open(path: Any, flags: int, *args: Any, **kwargs: Any) -> int: + nonlocal intervened + if path == "notes.txt" and flags & os.O_CREAT: + assert not target.exists() + intervened = True + target.write_bytes(b"other creator\n") + return original_open(path, flags, *args, **kwargs) + + monkeypatch.setattr(os, "open", intervening_open) + with pytest.raises(ApplyPatchDiffError, match="already exists"): + await editor.apply_operation( + ApplyPatchOperation(type="create_file", path="notes.txt", diff="+replacement\n") + ) + assert intervened + assert target.read_bytes() == b"other creator\n" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("replace_target", [False, True]) +async def test_failed_create_requires_inspection_and_never_deletes_a_replacement( + editor: WorkspaceEditor, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + replace_target: bool, +) -> None: + target = tmp_path / "notes.txt" + + def fail_copy(source: Any, destination: Any) -> None: + destination.write(b"partial") + destination.flush() + if replace_target: + target.unlink() + target.write_bytes(b"new owner\n") + raise OSError(errno.ENOSPC, "synthetic full filesystem") + + with monkeypatch.context() as patch: + patch.setattr(shutil, "copyfileobj", fail_copy) + with pytest.raises(WorkspaceArchiveWriteError, match="Inspect the destination") as failure: + await editor.apply_operation( + ApplyPatchOperation(type="create_file", path="notes.txt", diff="+complete\n") + ) + assert failure.value.retryable is False + assert target.read_bytes() == (b"new owner\n" if replace_target else b"partial") + with pytest.raises(ApplyPatchDiffError, match="already exists"): + await editor.apply_operation( + ApplyPatchOperation(type="create_file", path="notes.txt", diff="+complete\n") + ) + assert target.read_bytes() == (b"new owner\n" if replace_target else b"partial") + if not replace_target: + # The caller has inspected and deliberately removed its incomplete result. + target.unlink() + result = await editor.apply_operation( + ApplyPatchOperation(type="create_file", path="notes.txt", diff="+complete\n") + ) + assert result.output == "Created notes.txt" + assert target.read_bytes() == b"complete" + + +@pytest.mark.asyncio +async def test_create_writes_nested_file_and_rejects_dangling_leaf( + editor: WorkspaceEditor, tmp_path: Path +) -> None: + (tmp_path / "real").mkdir() + (tmp_path / "alias").symlink_to(tmp_path / "real", target_is_directory=True) + result = await editor.apply_operation( + ApplyPatchOperation(type="create_file", path="alias/nested/new.txt", diff="+contents\n") + ) + assert result.output == "Created alias/nested/new.txt" + assert (tmp_path / "real/nested/new.txt").read_bytes() == b"contents" + (tmp_path / "alias/link.txt").symlink_to(tmp_path / "missing.txt") + with pytest.raises(ApplyPatchDiffError, match="already exists"): + await editor.apply_operation( + ApplyPatchOperation(type="create_file", path="alias/link.txt", diff="+contents\n") + ) + assert not (tmp_path / "missing.txt").exists() + assert (tmp_path / "alias/link.txt").is_symlink() + + +@pytest.mark.asyncio +async def test_create_close_failure_reports_incomplete_contents( + editor: WorkspaceEditor, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + original_fdopen = os.fdopen + + class FailingClose: + def __init__(self, stream: Any): + self.stream = stream + + def __enter__(self) -> Any: + return self.stream + + def write(self, data: bytes) -> int: + return self.stream.write(data) + + def __exit__(self, *args: Any) -> None: + self.stream.close() + raise OSError(errno.ENOSPC, "synthetic flush failure") + + monkeypatch.setattr(os, "fdopen", lambda *a, **kw: FailingClose(original_fdopen(*a, **kw))) + with pytest.raises(WorkspaceArchiveWriteError, match="Inspect the destination") as failure: + await editor.apply_operation( + ApplyPatchOperation(type="create_file", path="notes.txt", diff="+payload\n") + ) + assert failure.value.retryable is False + assert (tmp_path / "notes.txt").read_bytes() == b"payload" 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..b8561ec6d1 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,25 @@ 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: + """Creation keeps the existing file steps without requiring a scripted exec call.""" + 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")), + ]