From 2e2509add32b5b3068cfafaac1a189dbe0e62bdc Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Sun, 6 Sep 2026 12:19:08 -0700 Subject: [PATCH 1/9] 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 4a55a8d5939ec951ad6a2e11029db7c3aef5e8dc Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Sun, 6 Sep 2026 14:35:00 -0700 Subject: [PATCH 2/9] 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 b0e8ffa2691754d86132b179281fd9646aa7455d Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Mon, 7 Sep 2026 07:22:25 -0700 Subject: [PATCH 3/9] 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 c18d1cfe7bf20e38801ce78ddcee2852b06814c9 Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Mon, 7 Sep 2026 09:23:36 -0700 Subject: [PATCH 4/9] 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 28eeb265ef..0ca77f59a4 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1015,6 +1015,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 999dec0f9636d6afc3fb9fea777dfa6f5d556803 Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Mon, 7 Sep 2026 11:30:08 -0700 Subject: [PATCH 5/9] 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 0ca77f59a4..35f454a201 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1041,7 +1041,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 592c2a0750938608627cd3e35e85b99ca49b05a7 Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Mon, 7 Sep 2026 13:37:30 -0700 Subject: [PATCH 6/9] 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 35f454a201..3c2bf9bc6d 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1034,28 +1034,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 9820c3fa1e9b8d2da46544f59f58caa9d8f50896 Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Mon, 7 Sep 2026 15:48:00 -0700 Subject: [PATCH 7/9] 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 3c2bf9bc6d..abfe0a4e5e 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1036,17 +1036,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 8049c24122bc904eece572a1490f01dcb9b4c9e3 Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Tue, 8 Sep 2026 10:28:07 -0700 Subject: [PATCH 8/9] 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 abfe0a4e5e..88a5ecd03b 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1034,7 +1034,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 bd47f6405c8f184e39ab5cb9168d398adbb12f2e Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Tue, 8 Sep 2026 14:01:59 -0700 Subject: [PATCH 9/9] 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 88a5ecd03b..890299a30d 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1035,6 +1035,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)