fix(sandbox): make Docker persist_workspace archives restorable by hydrate_workspace

src/agents/sandbox/sandboxes/docker.py

@@ -1371,7 +1371,11 @@ async def persist_workspace(self) -> io.IOBase:
                 staging_workspace,
                 cleanup_path=staging_parent,
             )
-            return strip_tar_member_prefix(root_prefixed_archive, prefix=staging_workspace.name)
+            return strip_tar_member_prefix(
+                root_prefixed_archive,
+                prefix=staging_workspace.name,
+                relativize_symlinks_under=root,
+            )
         except docker.errors.NotFound as e:
             raise WorkspaceArchiveReadError(path=error_root, cause=e, retryable=False) from e
         except docker.errors.APIError as e:

src/agents/sandbox/util/tar_utils.py

@@ -3,12 +3,13 @@
 import copy
 import io
 import os
+import posixpath
 import shutil
 import tarfile
 import tempfile
 from collections.abc import Iterable
-from pathlib import Path, PurePosixPath, PureWindowsPath
-from typing import cast
+from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath
+from typing import IO, cast
 
 
 class UnsafeTarMemberError(ValueError):
@@ -100,60 +101,112 @@ def safe_tar_member_rel_path(
     return Path(*rel.parts)
 
 
-def strip_tar_member_prefix(data: io.IOBase, *, prefix: str | Path) -> io.IOBase:
+def strip_tar_member_prefix(
+    data: io.IOBase,
+    *,
+    prefix: str | Path,
+    relativize_symlinks_under: str | PurePath | None = None,
+) -> io.IOBase:
     """Return a seekable tar stream after replacing a leading member prefix with `.`.
 
     For example, Docker archives a workspace copied to `/tmp/stage/workspace`
     as `workspace/...`; portable workspace snapshots should store the same
     files as `.` and `...`, independent of the source backend's root name.
+
+    The rewritten archive only contains members that the strict hydrate extractor
+    accepts. Archivers such as Docker's represent a second hardlinked path as a
+    hardlink member and keep FIFOs and device nodes, and ordinary workspaces contain
+    them (``uv`` and ``pnpm`` hardlink installed packages, dev servers leave FIFOs
+    behind). Hardlink members are stored as regular files with the target's payload (read
+    back from the rewritten archive, so the source is still streamed once),
+    FIFOs and device nodes are dropped, and when `relativize_symlinks_under` names
+    the workspace root, an absolute symlink target under that root becomes relative
+    to the link's own directory so it restores under any root.
     """
 
     prefix_rel = _normalize_rel(prefix)
     if prefix_rel == Path():
         raise ValueError("tar member prefix must not be empty")
+    symlink_root: PurePosixPath | None = None
+    if relativize_symlinks_under is not None:
+        symlink_root = PurePosixPath(
+            relativize_symlinks_under.as_posix()
+            if isinstance(relativize_symlinks_under, PurePath)
+            else relativize_symlinks_under
+        )
 
     out = tempfile.TemporaryFile()
     try:
-        with data:
-            with tarfile.open(fileobj=data, mode="r|*") as src:
-                with tarfile.open(fileobj=out, mode="w|") as dst:
-                    for member in src:
-                        rel_path = safe_tar_member_rel_path(
-                            member,
-                            allow_symlinks=True,
+        # Stream the source once. A hardlink member carries no payload of its own, so its
+        # target's bytes are read back from the rewritten archive being written (recorded by
+        # original member name), which keeps temp usage at one archive instead of two.
+        written_payloads: dict[str, tuple[int, int]] = {}
+        with data, tarfile.open(fileobj=data, mode="r|*") as src:
+            with tarfile.open(fileobj=out, mode="w") as dst:
+                for member in src:
+                    if member.isfifo() or member.ischr() or member.isblk():
+                        continue
+                    payload: tuple[int, int] | None = None
+                    if member.islnk():
+                        payload = written_payloads.get(member.linkname)
+                        if payload is None:
+                            reason = (
+                                f"hardlink target is not a file in the archive: {member.linkname}"
+                            )
+                            raise UnsafeTarMemberError(member=member.name, reason=reason)
+                        member = copy.copy(member)
+                        member.type = tarfile.REGTYPE
+                        member.linkname = ""
+                        member.size = payload[1]
+                    rel_path = safe_tar_member_rel_path(
+                        member,
+                        allow_symlinks=True,
+                    )
+                    if rel_path is None:
+                        stripped_name = "."
+                    elif rel_path == prefix_rel:
+                        stripped_name = "."
+                    elif rel_path.parts[: len(prefix_rel.parts)] == prefix_rel.parts:
+                        stripped_name = Path(*rel_path.parts[len(prefix_rel.parts) :]).as_posix()
+                    else:
+                        reason = f"member does not start with prefix: {prefix_rel.as_posix()}"
+                        raise UnsafeTarMemberError(
+                            member=member.name,
+                            reason=reason,
+                        )
+
+                    rewritten = copy.copy(member)
+                    rewritten.name = stripped_name
+                    rewritten.pax_headers = dict(member.pax_headers)
+                    rewritten.pax_headers.pop("path", None)
+                    if rewritten.issym() and symlink_root is not None:
+                        rewritten.linkname = _relative_symlink_target(
+                            rewritten.linkname,
+                            link_name=stripped_name,
+                            root=symlink_root,
                         )
-                        if rel_path is None:
-                            stripped_name = "."
-                        elif rel_path == prefix_rel:
-                            stripped_name = "."
-                        elif rel_path.parts[: len(prefix_rel.parts)] == prefix_rel.parts:
-                            stripped_name = Path(
-                                *rel_path.parts[len(prefix_rel.parts) :]
-                            ).as_posix()
-                        else:
-                            reason = f"member does not start with prefix: {prefix_rel.as_posix()}"
+                        # A long source target lives in a PAX "linkpath" record that would
+                        # otherwise override the rewritten linkname; tobuf() re-derives it.
+                        rewritten.pax_headers.pop("linkpath", None)
+                    if not rewritten.isreg():
+                        dst.addfile(rewritten)
+                        continue
+                    if payload is not None:
+                        fileobj: IO[bytes] = cast(IO[bytes], _ArchivePayloadReader(out, *payload))
+                    else:
+                        extracted = src.extractfile(member)
+                        if extracted is None:
                             raise UnsafeTarMemberError(
                                 member=member.name,
-                                reason=reason,
+                                reason="missing file payload",
                             )
-
-                        rewritten = copy.copy(member)
-                        rewritten.name = stripped_name
-                        rewritten.pax_headers = dict(member.pax_headers)
-                        rewritten.pax_headers.pop("path", None)
-                        if member.isreg():
-                            fileobj = src.extractfile(member)
-                            if fileobj is None:
-                                raise UnsafeTarMemberError(
-                                    member=member.name,
-                                    reason="missing file payload",
-                                )
-                            try:
-                                dst.addfile(rewritten, fileobj)
-                            finally:
-                                fileobj.close()
-                        else:
-                            dst.addfile(rewritten)
+                        fileobj = extracted
+                    try:
+                        dst.addfile(rewritten, fileobj)
+                    finally:
+                        fileobj.close()
+                    padded = -(-rewritten.size // tarfile.BLOCKSIZE) * tarfile.BLOCKSIZE
+                    written_payloads[member.name] = (dst.offset - padded, rewritten.size)
 
         out.seek(0)
         with tarfile.open(fileobj=out, mode="r:*") as tar:
@@ -165,6 +218,59 @@ def strip_tar_member_prefix(data: io.IOBase, *, prefix: str | Path) -> io.IOBase
         raise
 
 
+class _ArchivePayloadReader(io.RawIOBase):
+    """Read a member payload back from the archive file that is still being written.
+
+    Every read seeks to the payload and then restores the writer's position, so the reader
+    can be interleaved with `TarFile.addfile()` writing to the same file object.
+    """
+
+    def __init__(self, archive: IO[bytes], start: int, size: int) -> None:
+        super().__init__()
+        self._archive = archive
+        self._position = start
+        self._end = start + size
+
+    def readable(self) -> bool:
+        return True
+
+    def read(self, size: int = -1) -> bytes:
+        remaining = self._end - self._position
+        if size is None or size < 0 or size > remaining:
+            size = remaining
+        if size <= 0:
+            return b""
+        write_position = self._archive.tell()
+        try:
+            self._archive.seek(self._position)
+            data = self._archive.read(size)
+        finally:
+            self._archive.seek(write_position)
+        self._position += len(data)
+        return data
+
+    def close(self) -> None:
+        # The archive stays open for the writer; only this view closes.
+        io.RawIOBase.close(self)
+
+
+def _relative_symlink_target(linkname: str, *, link_name: str, root: PurePosixPath) -> str:
+    """Make an absolute symlink target under `root` relative to the link's directory."""
+
+    target = PurePosixPath(linkname)
+    if not target.is_absolute():
+        return linkname
+    # normpath keeps exactly two leading slashes (POSIX leaves "//" implementation-defined);
+    # Linux resolves them as "/", so collapse them before the containment check.
+    normalized = PurePosixPath("/" + posixpath.normpath(linkname).lstrip("/"))
+    try:
+        target_rel = normalized.relative_to(root)
+    except ValueError:
+        return linkname
+    link_dir = PurePosixPath(link_name).parent
+    return posixpath.relpath(target_rel.as_posix() or ".", start=link_dir.as_posix())
+
+
 def _normalize_rel(prefix: str | Path) -> Path:
     rel = prefix if isinstance(prefix, Path) else Path(prefix)
     posix = rel.as_posix()

tests/sandbox/test_tar_utils.py

@@ -6,7 +6,7 @@
 import sys
 import tarfile
 from dataclasses import dataclass
-from pathlib import Path
+from pathlib import Path, PurePosixPath
 
 import pytest
 
@@ -16,6 +16,7 @@
     safe_tar_member_rel_path,
     strip_tar_member_prefix,
     validate_tar_bytes,
+    validate_tarfile,
 )
 
 
@@ -179,6 +180,109 @@ def test_strip_tar_member_prefix_returns_workspace_relative_archive() -> None:
         assert tar.getnames() == [".", "pkg", "pkg/main.py", "pkg/python"]
 
 
+def _prefixed_workspace_archive(*, external_symlink: bool) -> io.BytesIO:
+    """A `workspace/...` archive shaped like Docker's, with members hydrate refuses as-is."""
+
+    buf = io.BytesIO()
+    with tarfile.open(fileobj=buf, mode="w") as tar:
+        root = tarfile.TarInfo("workspace")
+        root.type = tarfile.DIRTYPE
+        tar.addfile(root)
+        sub = tarfile.TarInfo("workspace/sub")
+        sub.type = tarfile.DIRTYPE
+        tar.addfile(sub)
+        payload = b"shared"
+        regular = tarfile.TarInfo("workspace/a.txt")
+        regular.size = len(payload)
+        tar.addfile(regular, io.BytesIO(payload))
+        hardlink = tarfile.TarInfo("workspace/sub/hardlink.txt")
+        hardlink.type = tarfile.LNKTYPE
+        hardlink.linkname = "workspace/a.txt"
+        tar.addfile(hardlink)
+        fifo = tarfile.TarInfo("workspace/dev.fifo")
+        fifo.type = tarfile.FIFOTYPE
+        tar.addfile(fifo)
+        abs_inside = tarfile.TarInfo("workspace/sub/abs_up")
+        abs_inside.type = tarfile.SYMTYPE
+        abs_inside.linkname = "/workspace/a.txt"
+        tar.addfile(abs_inside)
+        rel = tarfile.TarInfo("workspace/rel")
+        rel.type = tarfile.SYMTYPE
+        rel.linkname = "a.txt"
+        tar.addfile(rel)
+        double_slash = tarfile.TarInfo("workspace/double_slash")
+        double_slash.type = tarfile.SYMTYPE
+        double_slash.linkname = "//workspace/a.txt"
+        tar.addfile(double_slash)
+        # Longer than the 100-byte ustar field, so tarfile records it in a PAX linkpath.
+        long_target = "/workspace/" + "/".join(["deeply-nested-directory"] * 5) + "/target.txt"
+        long_link = tarfile.TarInfo("workspace/long_link")
+        long_link.type = tarfile.SYMTYPE
+        long_link.linkname = long_target
+        tar.addfile(long_link)
+        if external_symlink:
+            outside = tarfile.TarInfo("workspace/outside")
+            outside.type = tarfile.SYMTYPE
+            outside.linkname = "/usr/bin/python3"
+            tar.addfile(outside)
+    buf.seek(0)
+    return buf
+
+
+def test_strip_tar_member_prefix_rewrites_members_hydrate_refuses() -> None:
+    stripped = strip_tar_member_prefix(
+        _prefixed_workspace_archive(external_symlink=True),
+        prefix="workspace",
+        relativize_symlinks_under="/workspace",
+    )
+
+    with tarfile.open(fileobj=stripped, mode="r:*") as tar:
+        members = {member.name: 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["sub/abs_up"].issym()
+        assert members["sub/abs_up"].linkname == "../a.txt"
+        assert members["rel"].linkname == "a.txt"
+        assert members["double_slash"].linkname == "a.txt"
+        long_link = members["long_link"]
+        assert long_link.linkname == "/".join(["deeply-nested-directory"] * 5) + "/target.txt"
+        assert "linkpath" not in long_link.pax_headers or (
+            long_link.pax_headers["linkpath"] == long_link.linkname
+        )
+        # External absolute targets are left for hydrate's policy to decide.
+        assert members["outside"].linkname == "/usr/bin/python3"
+
+
+def test_strip_tar_member_prefix_output_passes_strict_hydrate_validation(
+    tmp_path: Path,
+) -> None:
+    stripped = strip_tar_member_prefix(
+        _prefixed_workspace_archive(external_symlink=False),
+        prefix="workspace",
+        relativize_symlinks_under=PurePosixPath("/workspace"),
+    )
+
+    with tarfile.open(fileobj=stripped, mode="r:*") as tar:
+        validate_tarfile(tar, allow_external_symlink_targets=False)
+        safe_extract_tarfile(tar, root=tmp_path, allow_external_symlink_targets=False)
+
+    assert (tmp_path / "sub" / "hardlink.txt").read_bytes() == b"shared"
+    assert (tmp_path / "sub" / "abs_up").read_bytes() == b"shared"
+    assert not (tmp_path / "dev.fifo").exists()
+
+
+def test_strip_tar_member_prefix_keeps_absolute_symlinks_without_a_root() -> None:
+    stripped = strip_tar_member_prefix(
+        _prefixed_workspace_archive(external_symlink=False), prefix="workspace"
+    )
+
+    with tarfile.open(fileobj=stripped, mode="r:*") as tar:
+        assert tar.getmember("sub/abs_up").linkname == "/workspace/a.txt"
+
+
 def test_strip_tar_member_prefix_rewrites_pax_path_headers() -> None:
     long_name = "workspace/" + ("a" * 120) + ".txt"
     payload = b"payload"