diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index ea7f83e9d8..cb79d58184 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -25,7 +25,7 @@ from contextlib import suppress from dataclasses import dataclass, field from functools import partial -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import Literal, cast from ...logger import log_tool_action_warning @@ -112,6 +112,65 @@ def _close_fd_quietly(fd: int) -> None: os.close(fd) +def _restorable_tar_member(ti: tarfile.TarInfo, *, root: Path) -> tarfile.TarInfo | None: + """Rewrite one ``persist_workspace`` member so ``hydrate_workspace`` can restore it. + + The strict extractor used for hydrate refuses hardlink members, special files, and + absolute symlink targets. A local workspace legitimately contains all three (``uv`` and + ``pnpm`` hardlink installed packages, dev servers leave FIFOs behind, ``ln -s "$PWD/x"`` + makes an absolute link), and archiving them as-is produced a snapshot that could never be + restored. Store hardlinks as regular files, drop FIFOs and device nodes, and make an + absolute symlink target that stays under the workspace root relative so it survives the + root moving between sessions. Absolute targets outside the workspace are kept unchanged. + """ + + if ti.isfifo() or ti.ischr() or ti.isblk(): + return None + if ti.islnk(): + # tarfile turns the second occurrence of an inode into a hardlink member with no + # payload; ``TarFile.add`` reads the file contents for a regular member instead. + ti.type = tarfile.REGTYPE + ti.linkname = "" + ti.size = os.stat(root / ti.name).st_size + return ti + if ti.issym() and ti.linkname.startswith("/"): + ti.linkname = _rebase_symlink_target( + ti.linkname, link_name=ti.name, roots=(root, root.resolve(strict=False)) + ) + return ti + + +def _rebase_symlink_target(linkname: str, *, link_name: str, roots: tuple[Path, ...]) -> str: + """Rewrite an absolute symlink target under the workspace root as a link-relative one. + + Only the root prefix is replaced; the remaining components are kept verbatim (no + normalization), because ``..`` after a symlink component is resolved by the kernel + against the link target, so ``/current/../config`` with ``current -> releases/v1`` + names ``releases/config`` and must stay ``current/../config``. Absolute targets outside + the workspace are returned unchanged. A leading ``//`` is collapsed to ``/`` (Linux + treats them alike). + """ + + target = "/" + linkname.lstrip("/") + for candidate_root in roots: + prefix = candidate_root.as_posix().rstrip("/") + if target == prefix: + rest = "" + elif target.startswith(prefix + "/"): + # Consume the whole separator run at the boundary (`//a.txt`), keeping + # every later component, including `..`, untouched. + rest = target[len(prefix) :].lstrip("/") + else: + continue + # The link's own directory inside the archive holds no symlink components (the + # archive validator rejects members beneath a symlink), so climbing it is exact. + climb = "/".join([".."] * len(PurePosixPath(link_name).parent.parts)) + if rest and climb: + return f"{climb}/{rest}" + return rest or climb or "." + return linkname + + def _restore_pty_child_signal_defaults() -> None: for signum in _PTY_CHILD_SIGNAL_DEFAULTS: signal.signal(signum, signal.SIG_DFL) @@ -1097,7 +1156,7 @@ def _archive_workspace() -> None: skip_rel_paths=skip, root_name=None, ) - else ti + else _restorable_tar_member(ti, root=root) ), ) diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index fb13be3c51..6741ff5191 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 signal import tarfile import threading @@ -470,6 +471,91 @@ async def test_rm_as_user_checks_permissions_then_uses_local_fs( assert not any(part.startswith("rm ") for part in session.exec_commands[0]) +class TestUnixLocalPersistWorkspaceRestorable: + """persist_workspace must only emit members that the strict hydrate extractor accepts.""" + + @staticmethod + def _workspace(tmp_path: Path) -> Path: + workspace = tmp_path / "workspace" + (workspace / "sub").mkdir(parents=True) + (workspace / "a.txt").write_text("shared", encoding="utf-8") + os.link(workspace / "a.txt", workspace / "sub" / "hardlink.txt") + os.mkfifo(workspace / "dev.fifo") + (workspace / "abs_inside").symlink_to(workspace / "a.txt") + (workspace / "sub" / "abs_up").symlink_to(workspace / "a.txt") + (workspace / "rel").symlink_to("a.txt") + (workspace / "double_slash").symlink_to("/" + str(workspace / "a.txt")) + (workspace / "double_sep").symlink_to(str(workspace) + "//a.txt") + (workspace / "outside").symlink_to(tmp_path / "elsewhere.txt") + return workspace + + @pytest.mark.asyncio + async def test_persist_emits_restorable_members(self, tmp_path: Path) -> None: + workspace = self._workspace(tmp_path) + session = _RecordingUnixLocalSession(workspace) + + blob = await session.persist_workspace() + + with tarfile.open(fileobj=cast(io.BytesIO, blob), mode="r:*") as tar: + members = {member.name.removeprefix("./"): member for member in tar.getmembers()} + assert "dev.fifo" not in members + hardlink = members["sub/hardlink.txt"] + assert hardlink.isreg() and hardlink.size == len("shared") + extracted = tar.extractfile(hardlink) + assert extracted is not None and extracted.read() == b"shared" + assert members["abs_inside"].linkname == "a.txt" + assert members["sub/abs_up"].linkname == "../a.txt" + assert members["rel"].linkname == "a.txt" + assert members["double_slash"].linkname == "a.txt" + assert members["double_sep"].linkname == "a.txt" + assert members["outside"].linkname == str(tmp_path / "elsewhere.txt") + + @pytest.mark.asyncio + async def test_rebased_symlink_keeps_parent_steps_after_symlink_components( + self, + tmp_path: Path, + ) -> None: + """`/current/../config` with `current -> releases/v1` names `releases/config`; + collapsing the `..` lexically would silently retarget the restored link.""" + workspace = tmp_path / "workspace" + (workspace / "releases" / "v1").mkdir(parents=True) + (workspace / "releases" / "config").write_text("right", encoding="utf-8") + (workspace / "config").write_text("wrong", encoding="utf-8") + (workspace / "current").symlink_to("releases/v1") + (workspace / "abs_config").symlink_to(workspace / "current" / ".." / "config") + (workspace / "releases" / "v1" / "abs_up").symlink_to( + workspace / "current" / ".." / "config" + ) + assert (workspace / "abs_config").read_text(encoding="utf-8") == "right" + + blob = await _RecordingUnixLocalSession(workspace).persist_workspace() + restored_root = tmp_path / "restored" + await _RecordingUnixLocalSession(restored_root).hydrate_workspace(blob) + + assert os.readlink(restored_root / "abs_config") == "current/../config" + assert ( + os.readlink(restored_root / "releases" / "v1" / "abs_up") == "../../current/../config" + ) + assert (restored_root / "abs_config").read_text(encoding="utf-8") == "right" + assert (restored_root / "releases" / "v1" / "abs_up").read_text(encoding="utf-8") == "right" + + @pytest.mark.asyncio + async def test_persisted_workspace_hydrates_into_a_new_root(self, tmp_path: Path) -> None: + workspace = self._workspace(tmp_path) + (workspace / "outside").unlink() # hydrate rejects external targets by design + blob = await _RecordingUnixLocalSession(workspace).persist_workspace() + + restored_root = tmp_path / "restored" + restored = _RecordingUnixLocalSession(restored_root) + await restored.hydrate_workspace(blob) + + assert (restored_root / "sub" / "hardlink.txt").read_text(encoding="utf-8") == "shared" + assert not (restored_root / "dev.fifo").exists() + assert os.readlink(restored_root / "abs_inside") == "a.txt" + assert (restored_root / "abs_inside").read_text(encoding="utf-8") == "shared" + assert (restored_root / "sub" / "abs_up").read_text(encoding="utf-8") == "shared" + + @pytest.mark.asyncio async def test_hydrate_workspace_cancellation_waits_for_the_extracting_worker( tmp_path: Path,