From ee46ee6772908794704f8e05be1fdda72263e1e6 Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Sun, 6 Sep 2026 12:19:08 -0700 Subject: [PATCH 01/15] fix(sandbox): reject apply_patch create_file on an existing file Add File is documented to the model as creating a new file, and the delete and update operations both enforce their existing-file precondition. create_file enforced nothing, so an Add File operation aimed at a path that already existed overwrote it and reported "Created ", losing the previous contents with no error. Check that the destination is absent before writing, mirroring the existing _ensure_exists precondition used by delete_file. --- src/agents/sandbox/apply_patch.py | 15 +++++++++++++++ .../capabilities/test_apply_patch_tool.py | 4 +++- tests/sandbox/test_apply_patch.py | 17 +++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 30623fdf82..2be3e8edb8 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -119,6 +119,7 @@ async def apply_operation( ) if operation.type == "create_file": + await self._ensure_absent(destination, display_path=display_path) try: created_text = format_impl.apply_diff("", operation.diff, mode="create") except ValueError as exc: @@ -186,6 +187,20 @@ async def _ensure_exists(self, destination: Path, *, display_path: str) -> None: else: handle.close() + async def _ensure_absent(self, destination: Path, *, display_path: str) -> None: + try: + handle = await self._session.read(destination, user=self._user) + except (FileNotFoundError, WorkspaceReadNotFoundError): + return + handle.close() + 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, + ) + 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/tests/sandbox/capabilities/test_apply_patch_tool.py b/tests/sandbox/capabilities/test_apply_patch_tool.py index c0f5f46ad8..a54ce76af3 100644 --- a/tests/sandbox/capabilities/test_apply_patch_tool.py +++ b/tests/sandbox/capabilities/test_apply_patch_tool.py @@ -542,7 +542,9 @@ async def test_editor_runs_file_operations_as_bound_user(self) -> None: ), ) - assert session.read_users == ["sandbox-user", "sandbox-user"] + # Three reads: the update_file read, the create_file existence probe, and the + # delete_file existence check. Each must run as the bound user. + assert session.read_users == ["sandbox-user", "sandbox-user", "sandbox-user"] assert session.mkdir_users == ["sandbox-user", "sandbox-user"] assert session.write_users == ["sandbox-user", "sandbox-user"] assert session.rm_users == ["sandbox-user"] diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index c4cd676fec..e9839e463c 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -411,3 +411,20 @@ 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" From a68a49c555b0896e4ce4479073a830a19c5ab36c Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Sun, 6 Sep 2026 14:35:00 -0700 Subject: [PATCH 02/15] fix(sandbox): keep the create precondition probe out of failed read spans The absence check reached SandboxSession.read(), which records a failed sandbox.read child span when the file is missing. Missing is the success case for a create, so every successful Add File looked like it contained a failed sandbox operation. Use the existing _read_with_expected_span_errors helper, the same path the skills capability already uses for an existence probe. --- src/agents/sandbox/apply_patch.py | 12 +++++++++- tests/sandbox/test_apply_patch.py | 40 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 2be3e8edb8..4da9709e8d 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -188,8 +188,18 @@ async def _ensure_exists(self, destination: Path, *, display_path: str) -> None: handle.close() async def _ensure_absent(self, destination: Path, *, display_path: str) -> None: + # A missing destination is the success case here, so the probe must not mark the + # child sandbox.read span as failed on every successful create. Imported locally + # because the session package imports this module. + from .session.sandbox_session import _read_with_expected_span_errors + try: - handle = await self._session.read(destination, user=self._user) + handle = await _read_with_expected_span_errors( + self._session, + destination, + user=self._user, + expected_span_errors=(FileNotFoundError, WorkspaceReadNotFoundError), + ) except (FileNotFoundError, WorkspaceReadNotFoundError): return handle.close() diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index e9839e463c..1a6697818c 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -428,3 +428,43 @@ async def test_apply_patch_create_rejects_an_existing_file() -> None: ) assert session.files[Path("/workspace/notes.txt")] == b"alpha\n" + + +@pytest.mark.asyncio +async def test_apply_patch_create_does_not_record_a_failed_read_span(tmp_path: Path) -> None: + """The absence probe must not make every successful create look like a failed read.""" + import uuid + + from agents.sandbox.sandboxes.unix_local import UnixLocalSandboxSessionState + from agents.sandbox.session import SandboxSession + from agents.sandbox.snapshot import LocalSnapshot + from agents.tracing import trace + from tests.sandbox._filesystem_test_session import FilesystemTestSandboxSession + from tests.testing_processor import fetch_ordered_spans + + workspace = tmp_path / "workspace" + inner = FilesystemTestSandboxSession( + state=UnixLocalSandboxSessionState( + manifest=Manifest(root=str(workspace)), + snapshot=LocalSnapshot(id=str(uuid.uuid4()), base_path=tmp_path), + ) + ) + + with trace("apply_patch_create_span_test"): + async with SandboxSession(inner) as session: + await session.apply_patch( + ApplyPatchOperation( + type="create_file", + path="brand-new.txt", + diff="+hello\n", + ) + ) + + assert (workspace / "brand-new.txt").read_text() == "hello" + + read_span_errors = [ + span.error + for span in fetch_ordered_spans() + if span.span_data.export().get("name") == "sandbox.read" + ] + assert read_span_errors and all(error is None for error in read_span_errors) From 6e3d6c2f9c9730a954c9a3ad38796cd1b51c07de Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Mon, 7 Sep 2026 07:22:25 -0700 Subject: [PATCH 03/15] test(sandbox): skip the create span test on Windows The span assertion needs FilesystemTestSandboxSession, which is typed to UnixLocalSandboxSessionState, and importing agents.sandbox.sandboxes.unix_local raises ImportError on Windows by design. tests/conftest.py already collect-ignores the other files that depend on it, but test_apply_patch.py must stay collectible because its remaining tests are platform independent. The unix-only imports stay inside the test body so collection does not touch unix_local on Windows. --- tests/sandbox/test_apply_patch.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index 1a6697818c..55c88e7417 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -1,5 +1,6 @@ from __future__ import annotations +import sys from pathlib import Path import pytest @@ -430,6 +431,7 @@ async def test_apply_patch_create_rejects_an_existing_file() -> None: assert session.files[Path("/workspace/notes.txt")] == b"alpha\n" +@pytest.mark.skipif(sys.platform == "win32", reason="UnixLocalSandbox is Unix-only") @pytest.mark.asyncio async def test_apply_patch_create_does_not_record_a_failed_read_span(tmp_path: Path) -> None: """The absence probe must not make every successful create look like a failed read.""" From 05935329638e04b067983e147da8b5276218763f Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Mon, 7 Sep 2026 09:23:36 -0700 Subject: [PATCH 04/15] fix(sandbox): claim the create_file name at the backend write boundary The previous read-then-write check left a window: a concurrent sandbox command could create the file after the probe, and the unconditional write then destroyed that content. Reading also did not establish that a dangling symlink entry was absent, because the unix_local path policy resolves symlinks and the write landed on the link target. Add BaseSandboxSession.write_new_file(), which claims the target name before the payload is written and raises FileExistsError when the name is already taken. The shared implementation uses a shell noclobber redirection, so the redirect itself is the O_EXCL attempt; UnixLocal overrides it with os.open(O_CREAT|O_EXCL) for its direct path. Both validate the parent through the normal policy, preserving grants and the bound user, and leave the final component unresolved so a symlink at that name is rejected rather than followed. apply_patch create_file now uses it and drops the probe, so the tracing workaround for the probe read is no longer needed. Existing files still have to go through update_file. --- src/agents/sandbox/apply_patch.py | 36 +++--- src/agents/sandbox/sandboxes/unix_local.py | 40 ++++++ .../sandbox/session/base_sandbox_session.py | 57 +++++++++ tests/sandbox/_apply_patch_test_session.py | 14 +++ .../capabilities/test_apply_patch_tool.py | 4 +- tests/sandbox/test_apply_patch.py | 62 ++++----- tests/sandbox/test_unix_local.py | 118 ++++++++++++++++++ 7 files changed, 271 insertions(+), 60 deletions(-) diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 4da9709e8d..e41a5b97a0 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -119,7 +119,6 @@ async def apply_operation( ) if operation.type == "create_file": - await self._ensure_absent(destination, display_path=display_path) try: created_text = format_impl.apply_diff("", operation.diff, mode="create") except ValueError as exc: @@ -128,7 +127,7 @@ async def apply_operation( path=operation.path, cause=exc, ) from exc - await self._write_text(destination, created_text) + await self._write_new_text(destination, created_text, display_path=display_path) return ApplyPatchResult(output=f"Created {display_path}") raise ApplyPatchDiffError( @@ -187,29 +186,24 @@ async def _ensure_exists(self, destination: Path, *, display_path: str) -> None: else: handle.close() - async def _ensure_absent(self, destination: Path, *, display_path: str) -> None: - # A missing destination is the success case here, so the probe must not mark the - # child sandbox.read span as failed on every successful create. Imported locally - # because the session package imports this module. - from .session.sandbox_session import _read_with_expected_span_errors - + 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: - handle = await _read_with_expected_span_errors( - self._session, + await self._session.write_new_file( destination, + io.BytesIO(text.encode("utf-8")), user=self._user, - expected_span_errors=(FileNotFoundError, WorkspaceReadNotFoundError), ) - except (FileNotFoundError, WorkspaceReadNotFoundError): - return - handle.close() - 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, - ) + 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: diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index ad25e630ef..dd2865dace 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1054,6 +1054,46 @@ 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: + if user is not None: + await super().write_new_file(path, data, user=user) + return + + payload = coerce_write_payload(path=path, data=data) + # Validate the parent with the normal policy so grants and symlinked parents are + # still enforced, then keep the final component unresolved. normalize_path() + # resolves symlinks, which would turn a dangling link at the target name into its + # absent target and let the write land there instead of being rejected. + requested = Path(path) + parent_path = self.normalize_path(requested.parent, for_write=True) + workspace_path = parent_path / requested.name + # O_EXCL fails with EEXIST when the name is already taken, including by a symlink, + # so the name is claimed in the same syscall that creates the file. + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL + if hasattr(os, "O_NOFOLLOW"): + flags |= os.O_NOFOLLOW + try: + parent_path.mkdir(parents=True, exist_ok=True) + descriptor = os.open(workspace_path, flags, 0o644) + except FileExistsError: + raise + except OSError as e: + if e.errno in {errno.ELOOP, errno.EMLINK}: + raise FileExistsError(str(workspace_path)) from e + raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e + + try: + with os.fdopen(descriptor, "wb") as f: + shutil.copyfileobj(payload.stream, f) + except OSError as e: + raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e + async def _write_stream_with_exec( self, path: Path, diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index d377bea9ef..f03474890e 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -149,6 +149,15 @@ fi done """.strip() +_EXCLUSIVE_CREATE_EXISTS_CODE = 13 +# ``set -C`` makes the redirection use O_EXCL, so it fails when the target name already +# exists, including a dangling symlink, and the existing content is left untouched. The +# distinct exit codes keep "already exists" separable from any other failure without +# parsing shell-specific stderr text. +_EXCLUSIVE_CREATE_SCRIPT = ( + 'target="$1"\nmkdir -p "$(dirname "$target")" || exit 12\nset -C\n: > "$target" || exit 13\n' +) + _WRITE_ACCESS_CHECK_SCRIPT = ( 'target="$1"\n' 'if [ -e "$target" ]; then\n' @@ -945,6 +954,54 @@ 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. + + The target name is claimed atomically before the payload is written, so a + concurrent creator either loses the race or keeps its content. + + :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, including a dangling symlink. + """ + # Validate the parent so grants and symlinked parents are still enforced, then + # keep the final component unresolved. A path policy that resolves symlinks would + # otherwise turn a dangling link at the target name into its absent target and let + # the create land there instead of being rejected. + requested = Path(path) + parent_path = await self._validate_path_access(requested.parent, for_write=True) + workspace_path = parent_path / requested.name + path_arg = sandbox_path_str(workspace_path) + result = await self.exec( + "sh", + "-lc", + _EXCLUSIVE_CREATE_SCRIPT, + "sh", + path_arg, + shell=False, + user=user, + ) + if result.exit_code == _EXCLUSIVE_CREATE_EXISTS_CODE: + raise FileExistsError(path_arg) + if not result.ok(): + raise WorkspaceArchiveWriteError( + path=workspace_path, + context={ + "command": ["sh", "-lc", "", path_arg], + "stdout": result.stdout.decode("utf-8", errors="replace"), + "stderr": result.stderr.decode("utf-8", errors="replace"), + }, + ) + 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/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/capabilities/test_apply_patch_tool.py b/tests/sandbox/capabilities/test_apply_patch_tool.py index a54ce76af3..c0f5f46ad8 100644 --- a/tests/sandbox/capabilities/test_apply_patch_tool.py +++ b/tests/sandbox/capabilities/test_apply_patch_tool.py @@ -542,9 +542,7 @@ async def test_editor_runs_file_operations_as_bound_user(self) -> None: ), ) - # Three reads: the update_file read, the create_file existence probe, and the - # delete_file existence check. Each must run as the bound user. - assert session.read_users == ["sandbox-user", "sandbox-user", "sandbox-user"] + assert session.read_users == ["sandbox-user", "sandbox-user"] assert session.mkdir_users == ["sandbox-user", "sandbox-user"] assert session.write_users == ["sandbox-user", "sandbox-user"] assert session.rm_users == ["sandbox-user"] diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index 55c88e7417..0fda2bf2c7 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -1,6 +1,6 @@ from __future__ import annotations -import sys +import io from pathlib import Path import pytest @@ -12,7 +12,9 @@ ApplyPatchDiffError, ApplyPatchFileNotFoundError, ApplyPatchPathError, + WorkspaceReadNotFoundError, ) +from agents.sandbox.types import User from tests.sandbox._apply_patch_test_session import ( ApplyPatchSession, ProviderNotFoundApplyPatchSession, @@ -431,42 +433,30 @@ async def test_apply_patch_create_rejects_an_existing_file() -> None: assert session.files[Path("/workspace/notes.txt")] == b"alpha\n" -@pytest.mark.skipif(sys.platform == "win32", reason="UnixLocalSandbox is Unix-only") +class _AlwaysMissingReadApplyPatchSession(ApplyPatchSession): + """Reports every path as missing while still holding the file. + + 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 write boundary. + """ + + 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_does_not_record_a_failed_read_span(tmp_path: Path) -> None: - """The absence probe must not make every successful create look like a failed read.""" - import uuid - - from agents.sandbox.sandboxes.unix_local import UnixLocalSandboxSessionState - from agents.sandbox.session import SandboxSession - from agents.sandbox.snapshot import LocalSnapshot - from agents.tracing import trace - from tests.sandbox._filesystem_test_session import FilesystemTestSandboxSession - from tests.testing_processor import fetch_ordered_spans - - workspace = tmp_path / "workspace" - inner = FilesystemTestSandboxSession( - state=UnixLocalSandboxSessionState( - manifest=Manifest(root=str(workspace)), - snapshot=LocalSnapshot(id=str(uuid.uuid4()), base_path=tmp_path), - ) - ) +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 trace("apply_patch_create_span_test"): - async with SandboxSession(inner) as session: - await session.apply_patch( - ApplyPatchOperation( - type="create_file", - path="brand-new.txt", - diff="+hello\n", - ) + with pytest.raises(ApplyPatchDiffError): + await session.apply_patch( + ApplyPatchOperation( + type="create_file", + path="notes.txt", + diff="+beta\n", ) + ) - assert (workspace / "brand-new.txt").read_text() == "hello" - - read_span_errors = [ - span.error - for span in fetch_ordered_spans() - if span.span_data.export().get("name") == "sandbox.read" - ] - assert read_span_errors and all(error is None for error in read_span_errors) + 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..cd3f082dd2 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -610,3 +610,121 @@ 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_write_new_file_rejects_a_dangling_symlink(tmp_path: Path) -> None: + """A symlink entry is not absent, and the write must not follow it to its target.""" + session = _exclusive_write_session(tmp_path) + link = tmp_path / "link.txt" + link.symlink_to(tmp_path / "missing.txt") + + with pytest.raises(FileExistsError): + await session.write_new_file(Path("link.txt"), io.BytesIO(b"clobbered")) + + assert link.is_symlink() + assert not (tmp_path / "missing.txt").exists() + + +@pytest.mark.asyncio +async def test_write_new_file_creates_a_file_and_its_parents(tmp_path: Path) -> None: + session = _exclusive_write_session(tmp_path) + + await session.write_new_file(Path("nested/dir/new.txt"), io.BytesIO(b"payload")) + + assert (tmp_path / "nested" / "dir" / "new.txt").read_bytes() == b"payload" + + +class _ExitCodeUnixLocalSession(UnixLocalSandboxSession): + """Drives the shared exec-based exclusive create with a chosen exit code.""" + + def __init__(self, root: Path, exit_code: int) -> None: + super().__init__( + state=UnixLocalSandboxSessionState( + manifest=Manifest(root=str(root)), + snapshot=NoopSnapshot(id="noop"), + ) + ) + self._exit_code = exit_code + self.exec_commands: list[tuple[str, ...]] = [] + self.writes: list[Path] = [] + + async def _exec_internal( + self, + *command: str | Path, + timeout: float | None = None, + ) -> ExecResult: + _ = timeout + self.exec_commands.append(tuple(str(part) for part in command)) + return ExecResult(stdout=b"", stderr=b"", exit_code=self._exit_code) + + async def write(self, path: Path, data: io.IOBase, *, user: object = None) -> None: + _ = (data, user) + self.writes.append(path) + + +@pytest.mark.asyncio +async def test_write_new_file_with_a_bound_user_reports_an_existing_name(tmp_path: Path) -> None: + """Exit 13 from the exclusive-create script means the name was already taken.""" + session = _ExitCodeUnixLocalSession(tmp_path, exit_code=13) + + with pytest.raises(FileExistsError): + await session.write_new_file( + Path("notes.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") + ) + + assert session.writes == [] + + +@pytest.mark.asyncio +async def test_write_new_file_with_a_bound_user_writes_after_claiming_the_name( + tmp_path: Path, +) -> None: + session = _ExitCodeUnixLocalSession(tmp_path, exit_code=0) + + await session.write_new_file( + Path("notes.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") + ) + + assert [path.name for path in session.writes] == ["notes.txt"] + assert any("set -C" in part for cmd in session.exec_commands for part in cmd) + + +@pytest.mark.asyncio +async def test_write_new_file_with_a_bound_user_keeps_a_symlink_name_unresolved( + tmp_path: Path, +) -> None: + """The exclusive create must act on the link name, not on the target it points at.""" + session = _ExitCodeUnixLocalSession(tmp_path, exit_code=13) + (tmp_path / "link.txt").symlink_to(tmp_path / "missing.txt") + + with pytest.raises(FileExistsError): + await session.write_new_file( + Path("link.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") + ) + + dispatched = [part for cmd in session.exec_commands for part in cmd] + assert any(part.endswith("link.txt") for part in dispatched) + assert not any(part.endswith("missing.txt") for part in dispatched) From d7321b2d9f72a5d255bec1b78aa6e24d9573397b Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Mon, 7 Sep 2026 11:30:08 -0700 Subject: [PATCH 05/15] fix(sandbox): link a completed payload into place for create_file Three problems with the previous commit. SandboxSession did not forward write_new_file, so a session built by SandboxClient fell back to the shared implementation and never reached the UnixLocal os.open override. Forward it. The shared implementation created an empty file and then wrote the payload in a separate step, so a concurrent writer could be overwritten and a failed upload left an empty file holding the name. Write the payload under a staging name first, then claim the target with ln, which fails when the name is taken. The content is complete before the name exists, and a failed create leaves only the staging entry, which is removed. The script used ':' for the noclobber redirection. ':' is a POSIX special builtin, so on dash a redirection failure ended the shell before the exit mapping ran and a collision surfaced as a generic write error instead of FileExistsError. ln is a regular command, and a test now runs the script through sh, dash and bash so this cannot regress silently. Also create local files with 0o666 so the process umask decides the final mode, matching Path.open("wb") on the ordinary write path. --- src/agents/sandbox/sandboxes/unix_local.py | 4 +- .../sandbox/session/base_sandbox_session.py | 75 ++++++++++++------- src/agents/sandbox/session/sandbox_session.py | 13 ++++ tests/sandbox/test_unix_local.py | 75 ++++++++++++++++++- 4 files changed, 135 insertions(+), 32 deletions(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index dd2865dace..e26aed1bb9 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1080,7 +1080,9 @@ async def write_new_file( flags |= os.O_NOFOLLOW try: parent_path.mkdir(parents=True, exist_ok=True) - descriptor = os.open(workspace_path, flags, 0o644) + # 0o666 lets the process umask decide the final mode, matching what + # Path.open("wb") does on the ordinary write path. + descriptor = os.open(workspace_path, flags, 0o666) except FileExistsError: raise except OSError as e: diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index f03474890e..471afb441d 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -2,7 +2,9 @@ import asyncio import io import shlex +import uuid from collections.abc import Awaitable, Callable, Mapping, Sequence +from contextlib import suppress from pathlib import Path, PurePath from typing import Literal, NoReturn, TypeVar @@ -150,12 +152,20 @@ done """.strip() _EXCLUSIVE_CREATE_EXISTS_CODE = 13 -# ``set -C`` makes the redirection use O_EXCL, so it fails when the target name already -# exists, including a dangling symlink, and the existing content is left untouched. The -# distinct exit codes keep "already exists" separable from any other failure without -# parsing shell-specific stderr text. +# ``ln`` is the only step that claims the target name, and it fails when that name is +# already taken, including by a dangling symlink. Linking a fully written staging file +# means the content is complete before the name exists, so a failed or cancelled upload +# cannot leave an empty file behind that would block a retry. The trailing test only +# classifies a failure, so "already exists" stays separable from any other error without +# parsing shell-specific stderr text. ``ln`` is a regular command, unlike ``:``, so a +# failure still reaches the explicit exit mapping on shells where ``:`` is special. _EXCLUSIVE_CREATE_SCRIPT = ( - 'target="$1"\nmkdir -p "$(dirname "$target")" || exit 12\nset -C\n: > "$target" || exit 13\n' + 'target="$1"\n' + 'source="$2"\n' + 'mkdir -p "$(dirname "$target")" || exit 12\n' + 'ln "$source" "$target" 2>/dev/null && exit 0\n' + 'if [ -e "$target" ] || [ -L "$target" ]; then exit 13; fi\n' + "exit 14\n" ) _WRITE_ACCESS_CHECK_SCRIPT = ( @@ -963,8 +973,9 @@ async def write_new_file( ) -> None: """Write a file that must not already exist. - The target name is claimed atomically before the payload is written, so a - concurrent creator either loses the race or keeps its content. + The target name is claimed in a single atomic step once the payload is complete, + so a concurrent creator either loses the race or keeps its own content, and a + failed write does not leave a partial file holding the name. :param path: Absolute path in the container or path relative to the workspace root. @@ -980,27 +991,37 @@ async def write_new_file( parent_path = await self._validate_path_access(requested.parent, for_write=True) workspace_path = parent_path / requested.name path_arg = sandbox_path_str(workspace_path) - result = await self.exec( - "sh", - "-lc", - _EXCLUSIVE_CREATE_SCRIPT, - "sh", - path_arg, - shell=False, - user=user, - ) - if result.exit_code == _EXCLUSIVE_CREATE_EXISTS_CODE: - raise FileExistsError(path_arg) - if not result.ok(): - raise WorkspaceArchiveWriteError( - path=workspace_path, - context={ - "command": ["sh", "-lc", "", path_arg], - "stdout": result.stdout.decode("utf-8", errors="replace"), - "stderr": result.stderr.decode("utf-8", errors="replace"), - }, + staging_path = parent_path / f".{requested.name}.create-{uuid.uuid4().hex}" + staging_arg = sandbox_path_str(staging_path) + + await self.write(staging_path, data, user=user) + try: + result = await self.exec( + "sh", + "-lc", + _EXCLUSIVE_CREATE_SCRIPT, + "sh", + path_arg, + staging_arg, + shell=False, + user=user, ) - await self.write(workspace_path, data, user=user) + if result.exit_code == _EXCLUSIVE_CREATE_EXISTS_CODE: + raise FileExistsError(path_arg) + if not result.ok(): + raise WorkspaceArchiveWriteError( + path=workspace_path, + context={ + "command": ["sh", "-lc", "", path_arg, staging_arg], + "stdout": result.stdout.decode("utf-8", errors="replace"), + "stderr": result.stderr.decode("utf-8", errors="replace"), + }, + ) + finally: + # The staging entry is an implementation detail, and removing it must not + # replace the outcome of the create. + with suppress(Exception): + await self.rm(staging_path, user=user) async def _check_read_with_exec( self, path: Path | str, *, user: str | User | None = None 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/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index cd3f082dd2..5a4175868e 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -2,7 +2,9 @@ import asyncio import io +import shutil import signal +import subprocess import tarfile import threading import time @@ -22,6 +24,10 @@ UnixLocalSandboxSessionState, _UnixPtyProcessEntry, ) +from agents.sandbox.session.base_sandbox_session import ( + _EXCLUSIVE_CREATE_EXISTS_CODE, + _EXCLUSIVE_CREATE_SCRIPT, +) from agents.sandbox.snapshot import NoopSnapshot from agents.sandbox.types import ExecResult, User @@ -670,6 +676,7 @@ def __init__(self, root: Path, exit_code: int) -> None: self._exit_code = exit_code self.exec_commands: list[tuple[str, ...]] = [] self.writes: list[Path] = [] + self.removed: list[Path] = [] async def _exec_internal( self, @@ -684,6 +691,16 @@ async def write(self, path: Path, data: io.IOBase, *, user: object = None) -> No _ = (data, user) self.writes.append(path) + async def rm( + self, + path: Path | str, + *, + recursive: bool = False, + user: object = None, + ) -> None: + _ = (recursive, user) + self.removed.append(Path(path)) + @pytest.mark.asyncio async def test_write_new_file_with_a_bound_user_reports_an_existing_name(tmp_path: Path) -> None: @@ -695,21 +712,31 @@ async def test_write_new_file_with_a_bound_user_reports_an_existing_name(tmp_pat Path("notes.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") ) - assert session.writes == [] + # The payload only ever reached a staging name, and that staging entry is cleaned up, + # so a rejected create leaves nothing behind at the requested name. + assert [path.name for path in session.writes] != ["notes.txt"] + assert all(path.name.startswith(".notes.txt.create-") for path in session.writes) + assert session.removed == session.writes @pytest.mark.asyncio -async def test_write_new_file_with_a_bound_user_writes_after_claiming_the_name( +async def test_write_new_file_with_a_bound_user_links_the_completed_payload( tmp_path: Path, ) -> None: + """The payload is written first, then the target name is claimed by linking it.""" session = _ExitCodeUnixLocalSession(tmp_path, exit_code=0) await session.write_new_file( Path("notes.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") ) - assert [path.name for path in session.writes] == ["notes.txt"] - assert any("set -C" in part for cmd in session.exec_commands for part in cmd) + staged = session.writes[0] + assert staged.name.startswith(".notes.txt.create-") + dispatched = [part for cmd in session.exec_commands for part in cmd] + assert any("ln " in part for part in dispatched) + assert any(part.endswith("notes.txt") for part in dispatched) + assert str(staged) in dispatched + assert session.removed == [staged] @pytest.mark.asyncio @@ -728,3 +755,43 @@ async def test_write_new_file_with_a_bound_user_keeps_a_symlink_name_unresolved( dispatched = [part for cmd in session.exec_commands for part in cmd] assert any(part.endswith("link.txt") for part in dispatched) assert not any(part.endswith("missing.txt") for part in dispatched) + + +@pytest.mark.parametrize("shell", ["sh", "dash", "bash"]) +def test_exclusive_create_script_reports_a_taken_name_on_each_shell( + shell: str, tmp_path: Path +) -> None: + """Run the shipped script through real shells. + + The script is dispatched as ``sh -lc``, so whichever shell provides ``/bin/sh`` + decides how a failing command is handled. An earlier version used ``:``, which is a + POSIX special builtin, so a redirection failure terminated dash before the explicit + exit mapping ran and the collision surfaced as a generic write error. This lives with + the Unix-local tests because tests/conftest.py already skips them on Windows. + """ + executable = shutil.which(shell) + if executable is None: + pytest.skip(f"{shell} is not available") + + staging = tmp_path / "staging" + staging.write_bytes(b"payload") + taken = tmp_path / "taken.txt" + taken.write_bytes(b"existing\n") + dangling = tmp_path / "dangling.txt" + dangling.symlink_to(tmp_path / "missing.txt") + + def run(target: Path) -> int: + return subprocess.run( + [executable, "-c", _EXCLUSIVE_CREATE_SCRIPT, shell, str(target), str(staging)], + capture_output=True, + ).returncode + + assert run(taken) == _EXCLUSIVE_CREATE_EXISTS_CODE + assert taken.read_bytes() == b"existing\n" + + assert run(dangling) == _EXCLUSIVE_CREATE_EXISTS_CODE + assert not (tmp_path / "missing.txt").exists() + + fresh = tmp_path / "nested" / "fresh.txt" + assert run(fresh) == 0 + assert fresh.read_bytes() == b"payload" From 49bdc8aedce640f369e454bff4281ee320d5a7db Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Mon, 7 Sep 2026 13:37:30 -0700 Subject: [PATCH 06/15] fix(sandbox): claim the requested name, not its resolved target WorkspaceEditor normalizes the destination before dispatching, and UnixLocal resolves leaf symlinks, so create_file handed the primitive the link target. Add File on a dangling link.txt created missing.txt and reported success. Pass the unresolved path for create. Stage the payload locally too, then os.link it into place. os.link fails with EEXIST for a file, a directory or a dangling symlink, and a write that fails partway now leaves only the staging entry instead of a file holding the name. Reject a name held by a directory in the shared script. Bare ln treats an existing directory as a target directory and would have linked the staging file inside it while reporting the directory as created. Move the staging write inside the cleanup scope so a failed upload cannot leak the staging entry, and create the parent as the bound user so a fresh nested path is owned the way the previous write path owned it. The new tests drive session.apply_patch() rather than the primitive, which is the path that was actually broken. --- src/agents/sandbox/apply_patch.py | 6 +- src/agents/sandbox/sandboxes/unix_local.py | 26 +++--- .../sandbox/session/base_sandbox_session.py | 23 +++-- tests/sandbox/test_unix_local.py | 88 ++++++++++++++++++- 4 files changed, 116 insertions(+), 27 deletions(-) diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index e41a5b97a0..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_new_text(destination, created_text, display_path=display_path) + # 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( diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index e26aed1bb9..12f0ab2356 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1073,28 +1073,22 @@ async def write_new_file( requested = Path(path) parent_path = self.normalize_path(requested.parent, for_write=True) workspace_path = parent_path / requested.name - # O_EXCL fails with EEXIST when the name is already taken, including by a symlink, - # so the name is claimed in the same syscall that creates the file. - flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL - if hasattr(os, "O_NOFOLLOW"): - flags |= os.O_NOFOLLOW + staging_path = parent_path / f".{requested.name}.create-{uuid.uuid4().hex}" try: parent_path.mkdir(parents=True, exist_ok=True) - # 0o666 lets the process umask decide the final mode, matching what - # Path.open("wb") does on the ordinary write path. - descriptor = os.open(workspace_path, flags, 0o666) + with staging_path.open("wb") as staged: + shutil.copyfileobj(payload.stream, staged) + # os.link claims the name in one step and fails with EEXIST when it is taken + # by anything, including a directory or a dangling symlink. Linking a complete + # payload means a failed write never leaves a file holding the name. + os.link(staging_path, workspace_path) except FileExistsError: raise - except OSError as e: - if e.errno in {errno.ELOOP, errno.EMLINK}: - raise FileExistsError(str(workspace_path)) from e - raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e - - try: - with os.fdopen(descriptor, "wb") as f: - shutil.copyfileobj(payload.stream, f) except OSError as e: raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e + finally: + with suppress(OSError): + staging_path.unlink() async def _write_stream_with_exec( self, diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index 471afb441d..9b09b597ee 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -152,17 +152,19 @@ done """.strip() _EXCLUSIVE_CREATE_EXISTS_CODE = 13 -# ``ln`` is the only step that claims the target name, and it fails when that name is -# already taken, including by a dangling symlink. Linking a fully written staging file -# means the content is complete before the name exists, so a failed or cancelled upload -# cannot leave an empty file behind that would block a retry. The trailing test only -# classifies a failure, so "already exists" stays separable from any other error without -# parsing shell-specific stderr text. ``ln`` is a regular command, unlike ``:``, so a -# failure still reaches the explicit exit mapping on shells where ``:`` is special. +# ``ln`` claims the target name and fails when that name is already taken. Linking a +# fully written staging file means the content is complete before the name exists, so a +# failed or cancelled upload cannot leave a file behind that holds the name. The leading +# test rejects a name held by a directory, which ``ln`` would otherwise treat as a target +# directory and populate; the trailing test only classifies a failure, so "already +# exists" stays separable from any other error without parsing shell-specific stderr. +# ``ln`` is a regular command, unlike ``:``, so its failure still reaches the explicit +# exit mapping on shells where ``:`` is a special builtin. The caller creates the parent, +# so this script never has to create one as a different identity. _EXCLUSIVE_CREATE_SCRIPT = ( 'target="$1"\n' 'source="$2"\n' - 'mkdir -p "$(dirname "$target")" || exit 12\n' + 'if [ -e "$target" ] || [ -L "$target" ]; then exit 13; fi\n' 'ln "$source" "$target" 2>/dev/null && exit 0\n' 'if [ -e "$target" ] || [ -L "$target" ]; then exit 13; fi\n' "exit 14\n" @@ -994,8 +996,11 @@ async def write_new_file( staging_path = parent_path / f".{requested.name}.create-{uuid.uuid4().hex}" staging_arg = sandbox_path_str(staging_path) - await self.write(staging_path, data, user=user) try: + # Create the parent as the bound user so a fresh nested path is owned the same + # way the ordinary write path owned it. + await self.mkdir(parent_path, parents=True, user=user) + await self.write(staging_path, data, user=user) result = await self.exec( "sh", "-lc", diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index 5a4175868e..d1ea56927d 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -14,8 +14,9 @@ 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 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 ( @@ -677,6 +678,7 @@ def __init__(self, root: Path, exit_code: int) -> None: self.exec_commands: list[tuple[str, ...]] = [] self.writes: list[Path] = [] self.removed: list[Path] = [] + self.made_dirs: list[Path] = [] async def _exec_internal( self, @@ -701,6 +703,16 @@ async def rm( _ = (recursive, user) self.removed.append(Path(path)) + async def mkdir( + self, + path: Path | str, + *, + parents: bool = False, + user: object = None, + ) -> None: + _ = (parents, user) + self.made_dirs.append(Path(path)) + @pytest.mark.asyncio async def test_write_new_file_with_a_bound_user_reports_an_existing_name(tmp_path: Path) -> None: @@ -717,6 +729,7 @@ async def test_write_new_file_with_a_bound_user_reports_an_existing_name(tmp_pat assert [path.name for path in session.writes] != ["notes.txt"] assert all(path.name.startswith(".notes.txt.create-") for path in session.writes) assert session.removed == session.writes + assert session.made_dirs != [] @pytest.mark.asyncio @@ -792,6 +805,79 @@ def run(target: Path) -> int: assert run(dangling) == _EXCLUSIVE_CREATE_EXISTS_CODE assert not (tmp_path / "missing.txt").exists() + # The caller creates the parent, so the script only has to claim the name. fresh = tmp_path / "nested" / "fresh.txt" + fresh.parent.mkdir() assert run(fresh) == 0 assert fresh.read_bytes() == b"payload" + + existing_directory = tmp_path / "adir" + existing_directory.mkdir() + assert run(existing_directory) == _EXCLUSIVE_CREATE_EXISTS_CODE + assert list(existing_directory.iterdir()) == [] + + +@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()) From 37c15d73820d6d52570041dd0a5789e0b10f6f46 Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Mon, 7 Sep 2026 15:48:00 -0700 Subject: [PATCH 07/15] fix(sandbox): drop the login shell and narrow the create collision Use sh -c instead of sh -lc for the exclusive create. This path runs for a filesystem-only capability set, so it must not source shell startup files that live in the workspace it is editing. A parent that is a regular file makes mkdir raise FileExistsError, and the broad handler reported that as a collision on the requested name, telling the model to use update_file for a target that does not exist. Only the os.link call can report a collision now; parent and staging failures are wrapped as write errors. --- src/agents/sandbox/sandboxes/unix_local.py | 24 ++++++++++++------- .../sandbox/session/base_sandbox_session.py | 6 +++-- tests/sandbox/test_unix_local.py | 24 ++++++++++++++++++- 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index 12f0ab2356..5b2b047ccd 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1075,17 +1075,25 @@ async def write_new_file( workspace_path = parent_path / requested.name staging_path = parent_path / f".{requested.name}.create-{uuid.uuid4().hex}" try: - parent_path.mkdir(parents=True, exist_ok=True) - with staging_path.open("wb") as staged: - shutil.copyfileobj(payload.stream, staged) + # Only the link may report a collision. A parent that is a regular file also + # raises FileExistsError from mkdir, and reporting that as "the target already + # exists" would send the model to update_file for a target that is absent. + try: + parent_path.mkdir(parents=True, exist_ok=True) + with staging_path.open("wb") as staged: + shutil.copyfileobj(payload.stream, staged) + except OSError as e: + raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e + # os.link claims the name in one step and fails with EEXIST when it is taken # by anything, including a directory or a dangling symlink. Linking a complete # payload means a failed write never leaves a file holding the name. - os.link(staging_path, workspace_path) - except FileExistsError: - raise - except OSError as e: - raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e + try: + os.link(staging_path, workspace_path) + except FileExistsError: + raise + except OSError as e: + raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e finally: with suppress(OSError): staging_path.unlink() diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index 9b09b597ee..672ee2b09b 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -1001,9 +1001,11 @@ async def write_new_file( # way the ordinary write path owned it. await self.mkdir(parent_path, parents=True, user=user) await self.write(staging_path, data, user=user) + # -c rather than -lc: this runs on a filesystem-only capability set, so it + # must not source workspace-writable shell startup files. result = await self.exec( "sh", - "-lc", + "-c", _EXCLUSIVE_CREATE_SCRIPT, "sh", path_arg, @@ -1017,7 +1019,7 @@ async def write_new_file( raise WorkspaceArchiveWriteError( path=workspace_path, context={ - "command": ["sh", "-lc", "", path_arg, staging_arg], + "command": ["sh", "-c", "", path_arg, staging_arg], "stdout": result.stdout.decode("utf-8", errors="replace"), "stderr": result.stderr.decode("utf-8", errors="replace"), }, diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index d1ea56927d..8b32e8c340 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -16,7 +16,11 @@ from agents.editor import ApplyPatchOperation from agents.sandbox import SandboxPathGrant -from agents.sandbox.errors import ApplyPatchDiffError, 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.unix_local import ( @@ -881,3 +885,21 @@ async def test_apply_patch_create_through_the_session_writes_a_new_nested_file( 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") + ) From 1995aed003f0c730a26679d2a894e8da16c0778d Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Tue, 8 Sep 2026 10:28:07 -0700 Subject: [PATCH 08/15] fix(sandbox): use a fixed-length staging basename The staging name was derived from the destination, so it was always longer than the destination itself. A filename that fits the filesystem's component limit, and that the ordinary write path accepts, could then fail to stage: a 254 character name raised WorkspaceArchiveWriteError where a plain write succeeded before this branch. The staging basename is now constant at 52 characters regardless of the destination. Reported by fscfede-beep in #4930. --- src/agents/sandbox/sandboxes/unix_local.py | 2 +- .../sandbox/session/base_sandbox_session.py | 5 +++- tests/sandbox/test_unix_local.py | 28 +++++++++++++++++-- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index 5b2b047ccd..b1b39a26b5 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1073,7 +1073,7 @@ async def write_new_file( requested = Path(path) parent_path = self.normalize_path(requested.parent, for_write=True) workspace_path = parent_path / requested.name - staging_path = parent_path / f".{requested.name}.create-{uuid.uuid4().hex}" + staging_path = parent_path / f".apply-patch-create-{uuid.uuid4().hex}" try: # Only the link may report a collision. A parent that is a regular file also # raises FileExistsError from mkdir, and reporting that as "the target already diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index 672ee2b09b..1aefdde1f5 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -993,7 +993,10 @@ async def write_new_file( parent_path = await self._validate_path_access(requested.parent, for_write=True) workspace_path = parent_path / requested.name path_arg = sandbox_path_str(workspace_path) - staging_path = parent_path / f".{requested.name}.create-{uuid.uuid4().hex}" + # A fixed-length staging basename. Deriving it from the destination made the + # staging name longer than the destination, so a name that fits the filesystem's + # component limit could still fail to stage. + staging_path = parent_path / f".apply-patch-create-{uuid.uuid4().hex}" staging_arg = sandbox_path_str(staging_path) try: diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index 8b32e8c340..6a86264ce3 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -731,7 +731,7 @@ async def test_write_new_file_with_a_bound_user_reports_an_existing_name(tmp_pat # The payload only ever reached a staging name, and that staging entry is cleaned up, # so a rejected create leaves nothing behind at the requested name. assert [path.name for path in session.writes] != ["notes.txt"] - assert all(path.name.startswith(".notes.txt.create-") for path in session.writes) + assert all(path.name.startswith(".apply-patch-create-") for path in session.writes) assert session.removed == session.writes assert session.made_dirs != [] @@ -748,7 +748,7 @@ async def test_write_new_file_with_a_bound_user_links_the_completed_payload( ) staged = session.writes[0] - assert staged.name.startswith(".notes.txt.create-") + assert staged.name.startswith(".apply-patch-create-") dispatched = [part for cmd in session.exec_commands for part in cmd] assert any("ln " in part for part in dispatched) assert any(part.endswith("notes.txt") for part in dispatched) @@ -903,3 +903,27 @@ async def test_apply_patch_create_through_the_session_reports_a_file_parent_as_a 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" From e444c8543a16d3713ada5f534944936a38a08c02 Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Tue, 8 Sep 2026 14:01:59 -0700 Subject: [PATCH 09/15] fix(sandbox): classify a visible collision before staging the payload The staging write ran before the link script could classify the target, so a target inside an executable but non-writable parent failed on the staging write and the caller saw WorkspaceArchiveWriteError instead of the ApplyPatchDiffError that points it at update_file. It also meant an Add File onto an occupied name uploaded a payload that was then discarded. Probe the target first in both paths, then keep the atomic claim to decide real races. A creator that wins between the probe and the link still loses the name, and its staging entry is still cleaned up. Reported by Codex and by fscfede-beep in #4930. --- src/agents/sandbox/sandboxes/unix_local.py | 6 ++ .../sandbox/session/base_sandbox_session.py | 12 ++++ tests/sandbox/test_unix_local.py | 66 ++++++++++++++++--- 3 files changed, 76 insertions(+), 8 deletions(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index b1b39a26b5..3885a4d301 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1074,6 +1074,12 @@ async def write_new_file( parent_path = self.normalize_path(requested.parent, for_write=True) workspace_path = parent_path / requested.name staging_path = parent_path / f".apply-patch-create-{uuid.uuid4().hex}" + # Classify a visible collision before staging, so a target inside a non-writable + # parent reports the collision rather than a permission failure from the staging + # write. os.path.lexists does not follow a symlink at the target name. + if os.path.lexists(workspace_path): + raise FileExistsError(str(workspace_path)) + try: # Only the link may report a collision. A parent that is a regular file also # raises FileExistsError from mkdir, and reporting that as "the target already diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index 1aefdde1f5..784f30c7a3 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -152,6 +152,12 @@ done """.strip() _EXCLUSIVE_CREATE_EXISTS_CODE = 13 +# Classify an already-visible target before any payload is staged. Without this the +# staging write runs first, so a target inside an executable but non-writable parent +# fails on permissions and the caller sees a write error instead of the collision error +# that tells it to use update_file. The atomic claim below still decides real races. +_TARGET_EXISTS_SCRIPT = 'target="$1"\nif [ -e "$target" ] || [ -L "$target" ]; then exit 13; fi\n' + # ``ln`` claims the target name and fails when that name is already taken. Linking a # fully written staging file means the content is complete before the name exists, so a # failed or cancelled upload cannot leave a file behind that holds the name. The leading @@ -999,6 +1005,12 @@ async def write_new_file( staging_path = parent_path / f".apply-patch-create-{uuid.uuid4().hex}" staging_arg = sandbox_path_str(staging_path) + preflight = await self.exec( + "sh", "-c", _TARGET_EXISTS_SCRIPT, "sh", path_arg, shell=False, user=user + ) + if preflight.exit_code == _EXCLUSIVE_CREATE_EXISTS_CODE: + raise FileExistsError(path_arg) + try: # Create the parent as the bound user so a fresh nested path is owned the same # way the ordinary write path owned it. diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index 6a86264ce3..2413bf9452 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -2,6 +2,7 @@ import asyncio import io +import os import shutil import signal import subprocess @@ -671,7 +672,7 @@ async def test_write_new_file_creates_a_file_and_its_parents(tmp_path: Path) -> class _ExitCodeUnixLocalSession(UnixLocalSandboxSession): """Drives the shared exec-based exclusive create with a chosen exit code.""" - def __init__(self, root: Path, exit_code: int) -> None: + def __init__(self, root: Path, exit_code: int, *, preflight_exit_code: int = 0) -> None: super().__init__( state=UnixLocalSandboxSessionState( manifest=Manifest(root=str(root)), @@ -679,6 +680,7 @@ def __init__(self, root: Path, exit_code: int) -> None: ) ) self._exit_code = exit_code + self._preflight_exit_code = preflight_exit_code self.exec_commands: list[tuple[str, ...]] = [] self.writes: list[Path] = [] self.removed: list[Path] = [] @@ -690,8 +692,12 @@ async def _exec_internal( timeout: float | None = None, ) -> ExecResult: _ = timeout - self.exec_commands.append(tuple(str(part) for part in command)) - return ExecResult(stdout=b"", stderr=b"", exit_code=self._exit_code) + parts = tuple(str(part) for part in command) + self.exec_commands.append(parts) + # The collision preflight is the invocation that receives only the target. + is_preflight = not any("ln " in part for part in parts) + code = self._preflight_exit_code if is_preflight else self._exit_code + return ExecResult(stdout=b"", stderr=b"", exit_code=code) async def write(self, path: Path, data: io.IOBase, *, user: object = None) -> None: _ = (data, user) @@ -720,17 +726,32 @@ async def mkdir( @pytest.mark.asyncio async def test_write_new_file_with_a_bound_user_reports_an_existing_name(tmp_path: Path) -> None: - """Exit 13 from the exclusive-create script means the name was already taken.""" - session = _ExitCodeUnixLocalSession(tmp_path, exit_code=13) + """A target that is already visible is rejected before any payload is staged.""" + session = _ExitCodeUnixLocalSession(tmp_path, exit_code=0, preflight_exit_code=13) + + with pytest.raises(FileExistsError): + await session.write_new_file( + Path("notes.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") + ) + + # No payload bytes were uploaded, so a create onto an occupied name costs one probe + # rather than a full staged write that is then discarded. + assert session.writes == [] + assert session.removed == [] + + +@pytest.mark.asyncio +async def test_write_new_file_with_a_bound_user_reports_a_racing_creator(tmp_path: Path) -> None: + """A creator that wins between the preflight and the link still loses the name.""" + session = _ExitCodeUnixLocalSession(tmp_path, exit_code=13, preflight_exit_code=0) with pytest.raises(FileExistsError): await session.write_new_file( Path("notes.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") ) - # The payload only ever reached a staging name, and that staging entry is cleaned up, - # so a rejected create leaves nothing behind at the requested name. - assert [path.name for path in session.writes] != ["notes.txt"] + # Here the payload was staged before the race was detected, and the staging entry is + # still cleaned up rather than left in the workspace. assert all(path.name.startswith(".apply-patch-create-") for path in session.writes) assert session.removed == session.writes assert session.made_dirs != [] @@ -927,3 +948,32 @@ async def test_apply_patch_create_accepts_a_destination_at_the_component_limit( ) 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) From 59ea737eae4946dc26d5a163138ceb638e24bebe Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Wed, 9 Sep 2026 18:42:38 -0700 Subject: [PATCH 10/15] fix(sandbox): build exclusive create on the descriptor-relative file ops #4931 gave UnixLocal a dir_fd plus O_NOFOLLOW file-ops layer and routed the bound-user path through a Python worker running that same module. That is the primitive this change was approximating, so use it instead. write_new is the existing write with O_TRUNC replaced by O_EXCL, which claims the name in the same syscall that creates the file and fails with EEXIST when the name is taken by a file, a directory, or a dangling symlink. The worker exits with a distinct status for an existing target so the caller can report FileExistsError rather than a generic write error. This deletes the machinery the previous approach needed: the shell script, the staging file, the hard link, and the staging cleanup. With no hard link there is nothing to fail on writable object-storage mounts or across owners under fs.protected_hardlinks, and with no staging entry there is nothing for concurrent work to replace. The shared base now documents what it actually guarantees: it rejects an occupied target but is not atomic, and a backend that can claim a name should override it. test_parent_swap_after_validation_cannot_access_outside[patch] injected at session.normalize_path, which the create path no longer calls, so its swap stopped firing. Re-pointed the patch case at the file-ops authorize boundary the create path does traverse, keeping the coverage rather than letting it pass vacuously. Verified separately that a parent symlinked outside the workspace is still refused, with the outside file untouched. Co-Authored-By: Claude Opus 5 --- .../sandbox/sandboxes/_unix_local_file_ops.py | 31 +++ src/agents/sandbox/sandboxes/unix_local.py | 79 +++--- .../sandbox/session/base_sandbox_session.py | 97 ++------ tests/sandbox/test_apply_patch.py | 5 +- tests/sandbox/test_unix_local.py | 233 +++--------------- tests/sandbox/test_unix_local_file_io.py | 16 ++ 6 files changed, 135 insertions(+), 326 deletions(-) diff --git a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py index 90d228b4ac..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 3885a4d301..8011be33dc 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1061,48 +1061,49 @@ async def write_new_file( *, user: str | User | None = None, ) -> None: + payload = coerce_write_payload(path=path, data=data) + # The path is handed over unresolved. The descriptor-relative file ops authorize it + # without following symlinks and then open the leaf with O_NOFOLLOW, so a symlink at + # the target name is rejected rather than followed to its target. if user is not None: - await super().write_new_file(path, data, user=user) + await self._write_new_stream_with_exec(Path(path), payload.stream, user=user) return - payload = coerce_write_payload(path=path, data=data) - # Validate the parent with the normal policy so grants and symlinked parents are - # still enforced, then keep the final component unresolved. normalize_path() - # resolves symlinks, which would turn a dangling link at the target name into its - # absent target and let the write land there instead of being rejected. - requested = Path(path) - parent_path = self.normalize_path(requested.parent, for_write=True) - workspace_path = parent_path / requested.name - staging_path = parent_path / f".apply-patch-create-{uuid.uuid4().hex}" - # Classify a visible collision before staging, so a target inside a non-writable - # parent reports the collision rather than a permission failure from the staging - # write. os.path.lexists does not follow a symlink at the target name. - if os.path.lexists(workspace_path): - raise FileExistsError(str(workspace_path)) - try: - # Only the link may report a collision. A parent that is a regular file also - # raises FileExistsError from mkdir, and reporting that as "the target already - # exists" would send the model to update_file for a target that is absent. - try: - parent_path.mkdir(parents=True, exist_ok=True) - with staging_path.open("wb") as staged: - shutil.copyfileobj(payload.stream, staged) - except OSError as e: - raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e + self._files.write_new(Path(path), payload.stream) + except FileExistsError: + raise + except OSError as e: + raise WorkspaceArchiveWriteError(path=Path(path), cause=e) from e - # os.link claims the name in one step and fails with EEXIST when it is taken - # by anything, including a directory or a dangling symlink. Linking a complete - # payload means a failed write never leaves a file holding the name. - try: - os.link(staging_path, workspace_path) - except FileExistsError: - raise - except OSError as e: - raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e - finally: - with suppress(OSError): - staging_path.unlink() + 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, @@ -1133,14 +1134,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 784f30c7a3..d61fddc560 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -2,9 +2,7 @@ import asyncio import io import shlex -import uuid from collections.abc import Awaitable, Callable, Mapping, Sequence -from contextlib import suppress from pathlib import Path, PurePath from typing import Literal, NoReturn, TypeVar @@ -151,31 +149,6 @@ fi done """.strip() -_EXCLUSIVE_CREATE_EXISTS_CODE = 13 -# Classify an already-visible target before any payload is staged. Without this the -# staging write runs first, so a target inside an executable but non-writable parent -# fails on permissions and the caller sees a write error instead of the collision error -# that tells it to use update_file. The atomic claim below still decides real races. -_TARGET_EXISTS_SCRIPT = 'target="$1"\nif [ -e "$target" ] || [ -L "$target" ]; then exit 13; fi\n' - -# ``ln`` claims the target name and fails when that name is already taken. Linking a -# fully written staging file means the content is complete before the name exists, so a -# failed or cancelled upload cannot leave a file behind that holds the name. The leading -# test rejects a name held by a directory, which ``ln`` would otherwise treat as a target -# directory and populate; the trailing test only classifies a failure, so "already -# exists" stays separable from any other error without parsing shell-specific stderr. -# ``ln`` is a regular command, unlike ``:``, so its failure still reaches the explicit -# exit mapping on shells where ``:`` is a special builtin. The caller creates the parent, -# so this script never has to create one as a different identity. -_EXCLUSIVE_CREATE_SCRIPT = ( - 'target="$1"\n' - 'source="$2"\n' - 'if [ -e "$target" ] || [ -L "$target" ]; then exit 13; fi\n' - 'ln "$source" "$target" 2>/dev/null && exit 0\n' - 'if [ -e "$target" ] || [ -L "$target" ]; then exit 13; fi\n' - "exit 14\n" -) - _WRITE_ACCESS_CHECK_SCRIPT = ( 'target="$1"\n' 'if [ -e "$target" ]; then\n' @@ -981,69 +954,27 @@ async def write_new_file( ) -> None: """Write a file that must not already exist. - The target name is claimed in a single atomic step once the payload is complete, - so a concurrent creator either loses the race or keeps its own content, and a - failed write does not leave a partial file holding the name. + 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, including a dangling symlink. + :raises FileExistsError: If the path already exists. """ - # Validate the parent so grants and symlinked parents are still enforced, then - # keep the final component unresolved. A path policy that resolves symlinks would - # otherwise turn a dangling link at the target name into its absent target and let - # the create land there instead of being rejected. - requested = Path(path) - parent_path = await self._validate_path_access(requested.parent, for_write=True) - workspace_path = parent_path / requested.name - path_arg = sandbox_path_str(workspace_path) - # A fixed-length staging basename. Deriving it from the destination made the - # staging name longer than the destination, so a name that fits the filesystem's - # component limit could still fail to stage. - staging_path = parent_path / f".apply-patch-create-{uuid.uuid4().hex}" - staging_arg = sandbox_path_str(staging_path) - - preflight = await self.exec( - "sh", "-c", _TARGET_EXISTS_SCRIPT, "sh", path_arg, shell=False, user=user - ) - if preflight.exit_code == _EXCLUSIVE_CREATE_EXISTS_CODE: - raise FileExistsError(path_arg) - + workspace_path = await self._validate_path_access(path, for_write=True) try: - # Create the parent as the bound user so a fresh nested path is owned the same - # way the ordinary write path owned it. - await self.mkdir(parent_path, parents=True, user=user) - await self.write(staging_path, data, user=user) - # -c rather than -lc: this runs on a filesystem-only capability set, so it - # must not source workspace-writable shell startup files. - result = await self.exec( - "sh", - "-c", - _EXCLUSIVE_CREATE_SCRIPT, - "sh", - path_arg, - staging_arg, - shell=False, - user=user, - ) - if result.exit_code == _EXCLUSIVE_CREATE_EXISTS_CODE: - raise FileExistsError(path_arg) - if not result.ok(): - raise WorkspaceArchiveWriteError( - path=workspace_path, - context={ - "command": ["sh", "-c", "", path_arg, staging_arg], - "stdout": result.stdout.decode("utf-8", errors="replace"), - "stderr": result.stderr.decode("utf-8", errors="replace"), - }, - ) - finally: - # The staging entry is an implementation detail, and removing it must not - # replace the outcome of the create. - with suppress(Exception): - await self.rm(staging_path, user=user) + handle = await self.read(workspace_path, user=user) + except (FileNotFoundError, WorkspaceReadNotFoundError): + pass + else: + handle.close() + raise FileExistsError(sandbox_path_str(workspace_path)) + await self.write(workspace_path, data, user=user) async def _check_read_with_exec( self, path: Path | str, *, user: str | User | None = None diff --git a/tests/sandbox/test_apply_patch.py b/tests/sandbox/test_apply_patch.py index 0fda2bf2c7..0cfc69b3cf 100644 --- a/tests/sandbox/test_apply_patch.py +++ b/tests/sandbox/test_apply_patch.py @@ -436,8 +436,9 @@ async def test_apply_patch_create_rejects_an_existing_file() -> None: class _AlwaysMissingReadApplyPatchSession(ApplyPatchSession): """Reports every path as missing while still holding the file. - 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 write boundary. + 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: diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index 2413bf9452..f160da4c63 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -3,7 +3,6 @@ import asyncio import io import os -import shutil import signal import subprocess import tarfile @@ -23,17 +22,13 @@ 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, UnixLocalSandboxSessionState, _UnixPtyProcessEntry, ) -from agents.sandbox.session.base_sandbox_session import ( - _EXCLUSIVE_CREATE_EXISTS_CODE, - _EXCLUSIVE_CREATE_SCRIPT, -) from agents.sandbox.snapshot import NoopSnapshot from agents.sandbox.types import ExecResult, User @@ -646,202 +641,6 @@ async def test_write_new_file_keeps_an_intervening_creator_content(tmp_path: Pat assert target.read_bytes() == b"written by someone else\n" -@pytest.mark.asyncio -async def test_write_new_file_rejects_a_dangling_symlink(tmp_path: Path) -> None: - """A symlink entry is not absent, and the write must not follow it to its target.""" - session = _exclusive_write_session(tmp_path) - link = tmp_path / "link.txt" - link.symlink_to(tmp_path / "missing.txt") - - with pytest.raises(FileExistsError): - await session.write_new_file(Path("link.txt"), io.BytesIO(b"clobbered")) - - assert link.is_symlink() - assert not (tmp_path / "missing.txt").exists() - - -@pytest.mark.asyncio -async def test_write_new_file_creates_a_file_and_its_parents(tmp_path: Path) -> None: - session = _exclusive_write_session(tmp_path) - - await session.write_new_file(Path("nested/dir/new.txt"), io.BytesIO(b"payload")) - - assert (tmp_path / "nested" / "dir" / "new.txt").read_bytes() == b"payload" - - -class _ExitCodeUnixLocalSession(UnixLocalSandboxSession): - """Drives the shared exec-based exclusive create with a chosen exit code.""" - - def __init__(self, root: Path, exit_code: int, *, preflight_exit_code: int = 0) -> None: - super().__init__( - state=UnixLocalSandboxSessionState( - manifest=Manifest(root=str(root)), - snapshot=NoopSnapshot(id="noop"), - ) - ) - self._exit_code = exit_code - self._preflight_exit_code = preflight_exit_code - self.exec_commands: list[tuple[str, ...]] = [] - self.writes: list[Path] = [] - self.removed: list[Path] = [] - self.made_dirs: list[Path] = [] - - async def _exec_internal( - self, - *command: str | Path, - timeout: float | None = None, - ) -> ExecResult: - _ = timeout - parts = tuple(str(part) for part in command) - self.exec_commands.append(parts) - # The collision preflight is the invocation that receives only the target. - is_preflight = not any("ln " in part for part in parts) - code = self._preflight_exit_code if is_preflight else self._exit_code - return ExecResult(stdout=b"", stderr=b"", exit_code=code) - - async def write(self, path: Path, data: io.IOBase, *, user: object = None) -> None: - _ = (data, user) - self.writes.append(path) - - async def rm( - self, - path: Path | str, - *, - recursive: bool = False, - user: object = None, - ) -> None: - _ = (recursive, user) - self.removed.append(Path(path)) - - async def mkdir( - self, - path: Path | str, - *, - parents: bool = False, - user: object = None, - ) -> None: - _ = (parents, user) - self.made_dirs.append(Path(path)) - - -@pytest.mark.asyncio -async def test_write_new_file_with_a_bound_user_reports_an_existing_name(tmp_path: Path) -> None: - """A target that is already visible is rejected before any payload is staged.""" - session = _ExitCodeUnixLocalSession(tmp_path, exit_code=0, preflight_exit_code=13) - - with pytest.raises(FileExistsError): - await session.write_new_file( - Path("notes.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") - ) - - # No payload bytes were uploaded, so a create onto an occupied name costs one probe - # rather than a full staged write that is then discarded. - assert session.writes == [] - assert session.removed == [] - - -@pytest.mark.asyncio -async def test_write_new_file_with_a_bound_user_reports_a_racing_creator(tmp_path: Path) -> None: - """A creator that wins between the preflight and the link still loses the name.""" - session = _ExitCodeUnixLocalSession(tmp_path, exit_code=13, preflight_exit_code=0) - - with pytest.raises(FileExistsError): - await session.write_new_file( - Path("notes.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") - ) - - # Here the payload was staged before the race was detected, and the staging entry is - # still cleaned up rather than left in the workspace. - assert all(path.name.startswith(".apply-patch-create-") for path in session.writes) - assert session.removed == session.writes - assert session.made_dirs != [] - - -@pytest.mark.asyncio -async def test_write_new_file_with_a_bound_user_links_the_completed_payload( - tmp_path: Path, -) -> None: - """The payload is written first, then the target name is claimed by linking it.""" - session = _ExitCodeUnixLocalSession(tmp_path, exit_code=0) - - await session.write_new_file( - Path("notes.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") - ) - - staged = session.writes[0] - assert staged.name.startswith(".apply-patch-create-") - dispatched = [part for cmd in session.exec_commands for part in cmd] - assert any("ln " in part for part in dispatched) - assert any(part.endswith("notes.txt") for part in dispatched) - assert str(staged) in dispatched - assert session.removed == [staged] - - -@pytest.mark.asyncio -async def test_write_new_file_with_a_bound_user_keeps_a_symlink_name_unresolved( - tmp_path: Path, -) -> None: - """The exclusive create must act on the link name, not on the target it points at.""" - session = _ExitCodeUnixLocalSession(tmp_path, exit_code=13) - (tmp_path / "link.txt").symlink_to(tmp_path / "missing.txt") - - with pytest.raises(FileExistsError): - await session.write_new_file( - Path("link.txt"), io.BytesIO(b"payload"), user=User(name="sandbox-user") - ) - - dispatched = [part for cmd in session.exec_commands for part in cmd] - assert any(part.endswith("link.txt") for part in dispatched) - assert not any(part.endswith("missing.txt") for part in dispatched) - - -@pytest.mark.parametrize("shell", ["sh", "dash", "bash"]) -def test_exclusive_create_script_reports_a_taken_name_on_each_shell( - shell: str, tmp_path: Path -) -> None: - """Run the shipped script through real shells. - - The script is dispatched as ``sh -lc``, so whichever shell provides ``/bin/sh`` - decides how a failing command is handled. An earlier version used ``:``, which is a - POSIX special builtin, so a redirection failure terminated dash before the explicit - exit mapping ran and the collision surfaced as a generic write error. This lives with - the Unix-local tests because tests/conftest.py already skips them on Windows. - """ - executable = shutil.which(shell) - if executable is None: - pytest.skip(f"{shell} is not available") - - staging = tmp_path / "staging" - staging.write_bytes(b"payload") - taken = tmp_path / "taken.txt" - taken.write_bytes(b"existing\n") - dangling = tmp_path / "dangling.txt" - dangling.symlink_to(tmp_path / "missing.txt") - - def run(target: Path) -> int: - return subprocess.run( - [executable, "-c", _EXCLUSIVE_CREATE_SCRIPT, shell, str(target), str(staging)], - capture_output=True, - ).returncode - - assert run(taken) == _EXCLUSIVE_CREATE_EXISTS_CODE - assert taken.read_bytes() == b"existing\n" - - assert run(dangling) == _EXCLUSIVE_CREATE_EXISTS_CODE - assert not (tmp_path / "missing.txt").exists() - - # The caller creates the parent, so the script only has to claim the name. - fresh = tmp_path / "nested" / "fresh.txt" - fresh.parent.mkdir() - assert run(fresh) == 0 - assert fresh.read_bytes() == b"payload" - - existing_directory = tmp_path / "adir" - existing_directory.mkdir() - assert run(existing_directory) == _EXCLUSIVE_CREATE_EXISTS_CODE - assert list(existing_directory.iterdir()) == [] - - @pytest.mark.asyncio async def test_apply_patch_create_through_the_session_rejects_a_dangling_symlink( tmp_path: Path, @@ -977,3 +776,33 @@ async def test_apply_patch_create_reports_collision_inside_a_read_only_parent( 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"] 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, From a2c5e05c84c7bea0931abc8b8757b887d7e77242 Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Wed, 9 Sep 2026 23:02:42 -0700 Subject: [PATCH 11/15] fix(sandbox): keep supported parent symlinks and stop reading the target Two problems with the previous commit. Passing the whole path into the file ops unresolved made a supported internal symlink parent fail. The ordinary write path resolves those safe aliases, and test_safe_symlinks_grants_and_listing_paths_remain_supported establishes the support, but a create through "internal -> real" opened "internal" with O_NOFOLLOW | O_DIRECTORY and failed. Resolve the parent the way write does and keep only the leaf name unresolved, so a dangling symlink at the target name is still rejected without its target being created. The shared default probed the target with read(), which eagerly fetches the whole payload on the remote backends that inherit it. An Add File aimed at an existing large file would download it before reporting the collision, and an existing file the bound user cannot read reported a read failure instead of the collision. List the parent instead, which needs no payload and no read permission on the target. Both regressions have a test that fails on the unfixed source, including one asserting the rejected create issues no read at all. Co-Authored-By: Claude Opus 5 --- src/agents/sandbox/sandboxes/unix_local.py | 15 +++-- .../sandbox/session/base_sandbox_session.py | 11 ++-- tests/sandbox/test_unix_local.py | 64 +++++++++++++++++++ 3 files changed, 80 insertions(+), 10 deletions(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index 8011be33dc..5013bf0269 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1062,19 +1062,22 @@ async def write_new_file( user: str | User | None = None, ) -> None: payload = coerce_write_payload(path=path, data=data) - # The path is handed over unresolved. The descriptor-relative file ops authorize it - # without following symlinks and then open the leaf with O_NOFOLLOW, so a symlink at - # the target name is rejected rather than followed to its target. + # 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(Path(path), payload.stream, user=user) + await self._write_new_stream_with_exec(target, payload.stream, user=user) return try: - self._files.write_new(Path(path), payload.stream) + self._files.write_new(target, payload.stream) except FileExistsError: raise except OSError as e: - raise WorkspaceArchiveWriteError(path=Path(path), cause=e) from e + raise WorkspaceArchiveWriteError(path=target, cause=e) from e async def _write_new_stream_with_exec( self, diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index d61fddc560..b86b47c34f 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -967,12 +967,15 @@ async def write_new_file( :raises FileExistsError: If the path already exists. """ workspace_path = await self._validate_path_access(path, for_write=True) + # List the parent rather than reading the target. read() eagerly fetches the whole + # payload on the remote backends that inherit this 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. try: - handle = await self.read(workspace_path, user=user) + entries = await self.ls(workspace_path.parent, user=user) except (FileNotFoundError, WorkspaceReadNotFoundError): - pass - else: - handle.close() + entries = [] + if any(Path(entry.path).name == workspace_path.name for entry in entries): raise FileExistsError(sandbox_path_str(workspace_path)) await self.write(workspace_path, data, user=user) diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index f160da4c63..a5fc9a4821 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -31,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): @@ -806,3 +807,66 @@ async def fake_worker( ) 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() From 9bc4eb874c18063f576c42b81299769afcaf980c Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Thu, 10 Sep 2026 09:16:17 -0700 Subject: [PATCH 12/15] fix(sandbox): probe the target directly instead of listing its parent Listing the parent to decide a collision failed when the parent did not exist. BaseSandboxSession.ls() raises ExecNonZeroError in that case, which the probe did not catch, so a nested create such as newdir/file.txt aborted before write() on every backend inheriting the default, while succeeding on UnixLocal. Those backends' write paths create missing parents. Use a bare existence test instead. It needs only execute permission on the parent, never reads the target, and reports absent when the parent is missing so the create still reaches write(). Only a positive result rejects; any other probe outcome falls through rather than blocking a valid create. The test is also more accurate than the listing: it sees a file inside an executable but unreadable parent, which listing could not, so an existing file there is no longer silently overwritten. Validated the exact shipped script under sh, bash and dash for an existing file, a dangling symlink, an absent name, a missing parent, and a file in a 0311 parent. Co-Authored-By: Claude Opus 5 --- .../sandbox/session/base_sandbox_session.py | 25 +++++++++++-------- tests/sandbox/_filesystem_test_session.py | 19 +++++++++++++- tests/sandbox/test_unix_local.py | 24 ++++++++++++++++++ 3 files changed, 57 insertions(+), 11 deletions(-) diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index b86b47c34f..1ad96f0e38 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' @@ -967,16 +973,15 @@ async def write_new_file( :raises FileExistsError: If the path already exists. """ workspace_path = await self._validate_path_access(path, for_write=True) - # List the parent rather than reading the target. read() eagerly fetches the whole - # payload on the remote backends that inherit this 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. - try: - entries = await self.ls(workspace_path.parent, user=user) - except (FileNotFoundError, WorkspaceReadNotFoundError): - entries = [] - if any(Path(entry.path).name == workspace_path.name for entry in entries): - raise FileExistsError(sandbox_path_str(workspace_path)) + path_arg = sandbox_path_str(workspace_path) + # Only a positive result rejects the create. Anything else, including a missing + # parent or an unexpected probe failure, falls through to write(), which keeps a + # nested create working on backends whose write path creates parents. + 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) await self.write(workspace_path, data, user=user) async def _check_read_with_exec( 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_unix_local.py b/tests/sandbox/test_unix_local.py index a5fc9a4821..86f2f14219 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -870,3 +870,27 @@ async def test_apply_patch_create_supports_a_symlinked_parent(tmp_path: Path) -> 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" From 554580577a64d44d765bd18df559c6e02b4329a8 Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Fri, 11 Sep 2026 12:21:57 -0700 Subject: [PATCH 13/15] fix(sandbox): fail closed on a failed probe and keep scripted creates working Two problems with the previous commit. The probe treated any unexpected status as "absent" so a valid create would not be blocked. That was wrong: exit 0 already means absent, including when the parent does not exist, so the permissive fallback only covered the case where the probe itself did not run. On provider sessions whose write() uses a separate upload API, the write then succeeds and overwrites an existing target precisely when the precondition could not be checked. Only 0 may proceed now; 13 stays the collision and every other status is a write error. scripted_sandbox_session() broke. The inherited default reaches for exec, and a script that configures only file steps hides exec, so a previously valid mkdir-and-write script failed with AttributeError. Give the scripted session an implementation that consumes the same mkdir and write pair, and restore the explicit parent creation in the default so the observable call sequence matches what the create path did before this branch. Both have a test that fails on the unfixed source. Co-Authored-By: Claude Opus 5 --- .../sandbox/session/base_sandbox_session.py | 22 ++++++++-- src/agents/testing/sandbox.py | 14 +++++++ tests/sandbox/test_unix_local.py | 40 +++++++++++++++++++ tests/test_scripted_sandbox.py | 23 +++++++++++ 4 files changed, 96 insertions(+), 3 deletions(-) diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index 1ad96f0e38..d6079f2a97 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -974,14 +974,30 @@ async def write_new_file( """ workspace_path = await self._validate_path_access(path, for_write=True) path_arg = sandbox_path_str(workspace_path) - # Only a positive result rejects the create. Anything else, including a missing - # parent or an unexpected probe failure, falls through to write(), which keeps a - # nested create working on backends whose write path creates parents. + # 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( diff --git a/src/agents/testing/sandbox.py b/src/agents/testing/sandbox.py index d086ae7c9d..321f0a50c0 100644 --- a/src/agents/testing/sandbox.py +++ b/src/agents/testing/sandbox.py @@ -331,6 +331,20 @@ 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. + await self.mkdir(path.parent, parents=True, user=user) + await self.write(path, 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/test_unix_local.py b/tests/sandbox/test_unix_local.py index 86f2f14219..696446c90c 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -894,3 +894,43 @@ async def test_base_default_create_allows_a_missing_parent(tmp_path: Path) -> No ) 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/test_scripted_sandbox.py b/tests/test_scripted_sandbox.py index 247e0022fc..fe31455a5e 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,24 @@ 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 + assert [call.method for call in session.calls] == ["mkdir", "write"] From c9a7a6a21948ef050c8ae3f217b85435658ac6ec Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Sat, 12 Sep 2026 03:35:35 -0700 Subject: [PATCH 14/15] fix(sandbox): keep scripted create calls on their normalized paths The create path hands write_new_file an unresolved path on purpose, so a symlink at the target name is rejected rather than followed. The scripted session forwarded that path straight into its compatibility mkdir and write pair, so a script using the documented match callback saw "." and "notes.txt" where it previously saw the workspace paths, and failed with SandboxCallMatcherError. Normalize before emitting the pair. Verified against the previous commit: with a custom manifest root the calls were "sub" and "sub/notes.txt" and are now "/custom/root/sub" and "/custom/root/sub/notes.txt", matching what this surface recorded before this branch. The earlier test only asserted method names, which is why it could not catch this. It now asserts the recorded paths too and fails on the unfixed source. --- src/agents/testing/sandbox.py | 9 ++++++--- tests/test_scripted_sandbox.py | 8 +++++++- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/agents/testing/sandbox.py b/src/agents/testing/sandbox.py index 321f0a50c0..dcf7dde587 100644 --- a/src/agents/testing/sandbox.py +++ b/src/agents/testing/sandbox.py @@ -341,9 +341,12 @@ async def write_new_file( # 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. - await self.mkdir(path.parent, parents=True, user=user) - await self.write(path, data, user=user) + # 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: diff --git a/tests/test_scripted_sandbox.py b/tests/test_scripted_sandbox.py index fe31455a5e..36216aff1b 100644 --- a/tests/test_scripted_sandbox.py +++ b/tests/test_scripted_sandbox.py @@ -595,4 +595,10 @@ async def test_scripted_sandbox_supports_apply_patch_create() -> None: assert result.output == "Created notes.txt" assert session.remaining_steps == 0 - assert [call.method for call in session.calls] == ["mkdir", "write"] + # 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")), + ] From f788c42d33a1edaa866450a9338f5c44f8870800 Mon Sep 17 00:00:00 2001 From: Justin Beckwith Date: Sun, 27 Sep 2026 10:03:34 -0700 Subject: [PATCH 15/15] fix(sandbox): narrow exclusive creates and clarify partial-write recovery --- src/agents/sandbox/apply_patch.py | 5 +- src/agents/sandbox/errors.py | 3 +- .../sandbox/sandboxes/_unix_local_file_ops.py | 25 ++- src/agents/sandbox/sandboxes/unix_local.py | 21 +- .../sandbox/session/base_sandbox_session.py | 54 +---- src/agents/sandbox/session/sandbox_session.py | 4 +- src/agents/testing/sandbox.py | 17 -- tests/sandbox/_apply_patch_test_session.py | 2 +- tests/sandbox/_filesystem_test_session.py | 19 +- tests/sandbox/test_unix_local.py | 146 ++------------ tests/sandbox/test_unix_local_create.py | 185 ++++++++++++++++++ tests/test_scripted_sandbox.py | 7 +- 12 files changed, 258 insertions(+), 230 deletions(-) create mode 100644 tests/sandbox/test_unix_local_create.py diff --git a/src/agents/sandbox/apply_patch.py b/src/agents/sandbox/apply_patch.py index 5e6f8d98fc..24e10362e0 100644 --- a/src/agents/sandbox/apply_patch.py +++ b/src/agents/sandbox/apply_patch.py @@ -191,10 +191,9 @@ async def _ensure_exists(self, destination: Path, *, display_path: str) -> None: 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. + # Backends with a native no-clobber primitive can reject occupied names. try: - await self._session.write_new_file( + await self._session._write_new_file( destination, io.BytesIO(text.encode("utf-8")), 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 a895244551..01f3900160 100644 --- a/src/agents/sandbox/sandboxes/_unix_local_file_ops.py +++ b/src/agents/sandbox/sandboxes/_unix_local_file_ops.py @@ -27,6 +27,11 @@ _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: @@ -136,12 +141,18 @@ def write_new(self, path: Path, stream: io.IOBase) -> None: dir_fd=parent_fd, ) try: - out = os.fdopen(fd, "wb") - except BaseException: - os.close(fd) - raise - with out: - shutil.copyfileobj(stream, out) + 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): @@ -240,6 +251,8 @@ def _main() -> None: 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 c01d86542a..8f9f81b261 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -109,6 +109,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)} @@ -1064,7 +1070,7 @@ async def write( except OSError as e: raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e - async def write_new_file( + async def _write_new_file( self, path: Path, data: io.IOBase, @@ -1084,6 +1090,13 @@ async def write_new_file( 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: @@ -1109,6 +1122,12 @@ async def _write_new_stream_with_exec( 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, diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index 14d113deb5..2f3ffcc732 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -150,12 +150,6 @@ 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' @@ -1001,54 +995,22 @@ async def write( :param user: Optional sandbox user to perform the write as. """ - async def write_new_file( + 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. + """Backend hook for apply_patch creation. - :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. + 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. """ - 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) + 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 diff --git a/src/agents/sandbox/session/sandbox_session.py b/src/agents/sandbox/session/sandbox_session.py index 89552b9bbf..90f5a59975 100644 --- a/src/agents/sandbox/session/sandbox_session.py +++ b/src/agents/sandbox/session/sandbox_session.py @@ -685,7 +685,7 @@ async def write( await self._inner.write(path, data, user=user) @instrumented_op("write", data=_write_start_data) - async def write_new_file( + async def _write_new_file( self, path: Path, data: io.IOBase, @@ -695,7 +695,7 @@ async def write_new_file( # 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) + await self._inner._write_new_file(path, data, user=user) @instrumented_op( "running", diff --git a/src/agents/testing/sandbox.py b/src/agents/testing/sandbox.py index dcf7dde587..d086ae7c9d 100644 --- a/src/agents/testing/sandbox.py +++ b/src/agents/testing/sandbox.py @@ -331,23 +331,6 @@ 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 911581b707..16ec0f41b9 100644 --- a/tests/sandbox/_apply_patch_test_session.py +++ b/tests/sandbox/_apply_patch_test_session.py @@ -56,7 +56,7 @@ async def write( else: self.files[normalized] = bytes(payload) - async def write_new_file( + async def _write_new_file( self, path: Path, data: io.IOBase, diff --git a/tests/sandbox/_filesystem_test_session.py b/tests/sandbox/_filesystem_test_session.py index 1471118944..54f34d4035 100644 --- a/tests/sandbox/_filesystem_test_session.py +++ b/tests/sandbox/_filesystem_test_session.py @@ -22,11 +22,7 @@ UnixLocalSandboxSessionState, ) from agents.sandbox.session import SandboxSession -from agents.sandbox.session.base_sandbox_session import ( - _EXISTING_TARGET_EXIT_CODE, - _TARGET_EXISTS_SCRIPT, - BaseSandboxSession, -) +from agents.sandbox.session.base_sandbox_session import BaseSandboxSession from agents.sandbox.session.sandbox_session_state import SandboxSessionState from agents.sandbox.snapshot import LocalSnapshot, NoopSnapshot, SnapshotBase, SnapshotSpec from agents.sandbox.types import ExecResult, Permissions, User @@ -61,19 +57,6 @@ 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_unix_local.py b/tests/sandbox/test_unix_local.py index 696446c90c..0f7b646c2b 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -4,7 +4,6 @@ import io import os import signal -import subprocess import tarfile import threading import time @@ -22,7 +21,7 @@ WorkspaceArchiveWriteError, ) from agents.sandbox.manifest import Environment, Manifest -from agents.sandbox.sandboxes import _unix_local_file_ops, unix_local as unix_local_module +from agents.sandbox.sandboxes import unix_local as unix_local_module from agents.sandbox.sandboxes.unix_local import ( UnixLocalSandboxClient, UnixLocalSandboxSession, @@ -629,19 +628,6 @@ def _exclusive_write_session(root: Path) -> UnixLocalSandboxSession: ) -@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, @@ -730,11 +716,7 @@ async def test_apply_patch_create_through_the_session_reports_a_file_parent_as_a 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. - """ + """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 @@ -755,12 +737,7 @@ async def test_apply_patch_create_accepts_a_destination_at_the_component_limit( 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. - """ + """A visible collision reports the supported update alternative.""" session = _exclusive_write_session(tmp_path) parent = tmp_path / "locked" parent.mkdir() @@ -779,72 +756,6 @@ async def test_apply_patch_create_reports_collision_inside_a_read_only_parent( 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. @@ -874,12 +785,7 @@ async def test_apply_patch_create_supports_a_symlinked_parent(tmp_path: Path) -> @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. - """ + """Provider defaults retain the released mkdir/write behavior for nested creates.""" workspace = tmp_path / "workspace" workspace.mkdir() session = FilesystemTestSandboxSession( @@ -896,41 +802,23 @@ async def test_base_default_create_allows_a_missing_parent(tmp_path: Path) -> No 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, +async def test_base_default_create_preserves_provider_write_semantics( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> 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( + session = FilesystemTestSandboxSession( state=UnixLocalSandboxSessionState( - manifest=Manifest(root=str(workspace)), - snapshot=NoopSnapshot(id="noop"), + manifest=Manifest(root=str(tmp_path)), snapshot=NoopSnapshot(id="noop") ) ) - (workspace / "notes.txt").write_bytes(b"important\n") + target = tmp_path / "existing.txt" + target.write_bytes(b"previous") - with pytest.raises(WorkspaceArchiveWriteError): - await session.apply_patch( - ApplyPatchOperation(type="create_file", path="notes.txt", diff="+clobbered\n") - ) + async def no_new_probe(*args: object, **kwargs: object) -> ExecResult: + raise AssertionError("Creation must not add an exec requirement to providers") - assert (workspace / "notes.txt").read_bytes() == b"important\n" + 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" 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/test_scripted_sandbox.py b/tests/test_scripted_sandbox.py index 36216aff1b..b8561ec6d1 100644 --- a/tests/test_scripted_sandbox.py +++ b/tests/test_scripted_sandbox.py @@ -579,12 +579,7 @@ async def test_scripted_sandbox_drives_black_box_sandbox_agent_workflow() -> Non @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. - """ + """Creation keeps the existing file steps without requiring a scripted exec call.""" session = scripted_sandbox_session( [{"method": "mkdir", "result": None}, {"method": "write", "result": None}] )