diff --git a/src/agents/extensions/sandbox/modal/sandbox.py b/src/agents/extensions/sandbox/modal/sandbox.py index 848b8e12a3..565daadff6 100644 --- a/src/agents/extensions/sandbox/modal/sandbox.py +++ b/src/agents/extensions/sandbox/modal/sandbox.py @@ -88,6 +88,7 @@ from ....sandbox.util.tar_utils import UnsafeTarMemberError, validate_tar_bytes from ....sandbox.workspace_paths import ( coerce_posix_path, + normalize_posix_path, posix_path_as_path, posix_path_for_error, sandbox_path_str, @@ -1421,7 +1422,7 @@ async def _persist_workspace_via_snapshot_directory(self) -> io.IOBase: assert self._sandbox is not None if not hasattr(self._sandbox, "snapshot_directory"): return await self._persist_workspace_via_tar() - if self._native_snapshot_requires_tar_fallback(): + if self._native_snapshot_requires_tar_fallback(snapshot_root=root): return await self._persist_workspace_via_tar() plain_skip = self._modal_snapshot_plain_skip_relpaths(root) skip_abs = [root / rel for rel in sorted(plain_skip, key=lambda p: p.as_posix())] @@ -1509,7 +1510,9 @@ async def restore_ephemeral_paths_or_raise() -> None: }, ) - for mount_entry, mount_path in self._snapshot_directory_mount_targets_to_restore(root): + for mount_entry, mount_path in self._snapshot_directory_mount_targets_to_restore( + root, include_ephemeral=True + ): transition_error, transition_cancelled = await _settle_mount_transition( self, mount_entry.mount_strategy.teardown_for_snapshot( @@ -1938,14 +1941,19 @@ async def _run_restore() -> None: cause=e, ) from e - def _snapshot_directory_mount_targets_to_restore(self, root: Path) -> list[tuple[Mount, Path]]: + def _snapshot_directory_mount_targets_to_restore( + self, + root: Path, + *, + include_ephemeral: bool = False, + ) -> list[tuple[Mount, Path]]: mount_targets: list[tuple[Mount, Path]] = [] for mount_entry, mount_path in self.state.manifest.mount_targets(): - if mount_entry.ephemeral: + if mount_entry.ephemeral and not include_ephemeral: continue if isinstance(mount_entry.mount_strategy, ModalCloudBucketMountStrategy): continue - if mount_path != root and root not in mount_path.parents: + if not self._sandbox_paths_overlap(root, mount_path): continue mount_targets.append((mount_entry, mount_path)) return mount_targets @@ -2063,10 +2071,11 @@ def _validate_manifest_for_workspace_persistence( if workspace_persistence != _WORKSPACE_PERSISTENCE_SNAPSHOT_DIRECTORY: return - root = posix_path_as_path(coerce_posix_path(manifest.root)) + root = posix_path_as_path(normalize_posix_path(manifest.root)) for mount_entry, mount_path in manifest.mount_targets(): if not isinstance(mount_entry.mount_strategy, ModalCloudBucketMountStrategy): continue + mount_path = posix_path_as_path(normalize_posix_path(mount_path)) if mount_path == root or root in mount_path.parents: raise MountConfigError( message=( @@ -2079,6 +2088,18 @@ def _validate_manifest_for_workspace_persistence( "workspace_persistence": workspace_persistence, }, ) + if mount_path in root.parents: + raise MountConfigError( + message=( + "snapshot_directory is not supported when the workspace root " + "lives inside a Modal cloud bucket mount" + ), + context={ + "workspace_root": root.as_posix(), + "mount_path": mount_path.as_posix(), + "workspace_persistence": workspace_persistence, + }, + ) @redact_mount_error_data async def create( @@ -2302,6 +2323,10 @@ async def resume( ) _mark_mount_validation_error(error) raise error + self._validate_manifest_for_workspace_persistence( + manifest=state.manifest, + workspace_persistence=state.workspace_persistence, + ) if state.mount_authority_rebound: state.sandbox_id = None state.session_id = uuid.uuid4() diff --git a/src/agents/sandbox/session/base_sandbox_session.py b/src/agents/sandbox/session/base_sandbox_session.py index d377bea9ef..ff54a883f5 100644 --- a/src/agents/sandbox/session/base_sandbox_session.py +++ b/src/agents/sandbox/session/base_sandbox_session.py @@ -37,6 +37,7 @@ from ..workspace_paths import ( WorkspacePathPolicy, coerce_posix_path, + normalize_posix_path, posix_path_as_path, posix_path_for_error, sandbox_path_str, @@ -578,8 +579,14 @@ async def _aclose_dependencies(self) -> None: await dependencies.aclose() @staticmethod - def _workspace_relpaths_overlap(lhs: Path, rhs: Path) -> bool: - return lhs == rhs or lhs in rhs.parents or rhs in lhs.parents + def _sandbox_paths_overlap(lhs: PurePath, rhs: PurePath) -> bool: + lhs_posix = normalize_posix_path(lhs) + rhs_posix = normalize_posix_path(rhs) + return ( + lhs_posix == rhs_posix + or lhs_posix in rhs_posix.parents + or rhs_posix in lhs_posix.parents + ) def _mount_relpaths_within_workspace(self) -> set[Path]: root = self._workspace_root_path() @@ -595,11 +602,21 @@ def _overlapping_mount_relpaths(self, rel_path: Path) -> set[Path]: return { mount_relpath for mount_relpath in self._mount_relpaths_within_workspace() - if self._workspace_relpaths_overlap(rel_path, mount_relpath) + if self._sandbox_paths_overlap(rel_path, mount_relpath) } - def _native_snapshot_requires_tar_fallback(self) -> bool: - for mount_entry, _mount_path in self.state.manifest.mount_targets(): + def _native_snapshot_requires_tar_fallback(self, *, snapshot_root: Path | None = None) -> bool: + """Return whether mounts prevent a safe native snapshot. + + Filesystem-wide native snapshots pass no root and conservatively inspect every mount. + Directory snapshots may ignore mounts that cannot overlap the resolved snapshot root. + """ + + for mount_entry, mount_path in self.state.manifest.mount_targets(): + if snapshot_root is not None and not self._sandbox_paths_overlap( + snapshot_root, mount_path + ): + continue if not mount_entry.mount_strategy.supports_native_snapshot_detach(mount_entry): return True return False diff --git a/src/agents/sandbox/workspace_paths.py b/src/agents/sandbox/workspace_paths.py index 2a5b28a606..7b200641e4 100644 --- a/src/agents/sandbox/workspace_paths.py +++ b/src/agents/sandbox/workspace_paths.py @@ -37,6 +37,15 @@ def coerce_posix_path(path: str | PurePath) -> PurePosixPath: return PurePosixPath(path) +def normalize_posix_path(path: str | PurePath) -> PurePosixPath: + """Return a normalized POSIX path for sandbox filesystem comparisons.""" + + normalized = posixpath.normpath(coerce_posix_path(path).as_posix()) + if normalized.startswith("//"): + normalized = f"/{normalized.lstrip('/')}" + return PurePosixPath(normalized) + + def windows_absolute_path(path: str | PurePath) -> PureWindowsPath | None: """Return a Windows absolute path when the input uses Windows absolute syntax.""" @@ -498,10 +507,10 @@ def _absolute_workspace_posix_path(self, path: PurePosixPath) -> PurePosixPath: def _absolute_posix_path(self, path: PurePosixPath) -> PurePosixPath: root = self._normalized_root() raw_candidate = path.as_posix() if path.is_absolute() else str(root / path.as_posix()) - return PurePosixPath(posixpath.normpath(str(raw_candidate))) + return normalize_posix_path(str(raw_candidate)) def _normalized_root(self) -> PurePosixPath: - return PurePosixPath(posixpath.normpath(self._sandbox_root.as_posix())) + return normalize_posix_path(self._sandbox_root) @staticmethod def _path_exists(path: Path) -> bool: diff --git a/tests/extensions/sandbox/test_modal.py b/tests/extensions/sandbox/test_modal.py index 44a3fa5c72..a9f20875df 100644 --- a/tests/extensions/sandbox/test_modal.py +++ b/tests/extensions/sandbox/test_modal.py @@ -1429,6 +1429,58 @@ async def test_modal_resume_creates_fresh_sandbox_for_rebound_mount_authority( assert mount.secret.name == "current-secret" +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("workspace_root", "mount_path"), + [ + pytest.param("/workspace", "/workspace", id="equal"), + pytest.param("/workspace", "/workspace/remote", id="descendant"), + pytest.param("/workspace/project", "/workspace", id="ancestor"), + ], +) +async def test_modal_resume_rejects_snapshot_directory_with_rebound_overlapping_cloud_mount( + monkeypatch: pytest.MonkeyPatch, + workspace_root: str, + mount_path: str, +) -> None: + modal_module, create_calls, _registry_tags = _load_modal_module(monkeypatch) + trusted_manifest = Manifest( + root=workspace_root, + entries={ + "remote": S3Mount( + bucket="bucket", + mount_path=Path(mount_path), + mount_strategy=modal_module.ModalCloudBucketMountStrategy( + secret_name="current-secret" + ), + ) + }, + ) + state = modal_module.ModalSandboxSessionState( + manifest=trusted_manifest, + snapshot=modal_module.resolve_snapshot(None, "snapshot"), + app_name="sandbox-tests", + sandbox_id="sb-existing", + workspace_persistence="snapshot_directory", + ) + client = modal_module.ModalSandboxClient() + restored = client.deserialize_session_state(client.serialize_session_state(state)) + rebound = restored.rebind_persisted_mount_authority( + trusted_manifest, + provider_backend_id="modal", + ) + original_session_id = rebound.session_id + original_sandbox_id = rebound.sandbox_id + + with pytest.raises(MountConfigError, match="sandbox mount configuration is invalid"): + await client.resume(rebound) + + assert rebound.session_id == original_session_id + assert rebound.sandbox_id == original_sandbox_id + assert create_calls == [] + assert sys.modules["modal"].Sandbox.from_id_calls == [] + + @pytest.mark.asyncio async def test_modal_resume_marks_reconnected_sandbox_preserved_before_snapshot_reuse( monkeypatch: pytest.MonkeyPatch, @@ -3561,6 +3613,42 @@ async def test_modal_snapshot_directory_restore_preserves_exposed_ports( assert sys.modules["modal"].Image.from_id_calls == ["snap-dir-123"] +@pytest.mark.asyncio +async def test_modal_snapshot_directory_restore_recreates_disjoint_cloud_bucket_mount( + monkeypatch: pytest.MonkeyPatch, +) -> None: + modal_module, create_calls, _registry_tags = _load_modal_module(monkeypatch) + + state = modal_module.ModalSandboxSessionState( + manifest=Manifest( + root="/workspace", + entries={ + "remote": S3Mount( + bucket="bucket", + mount_path=Path("/mnt/remote"), + mount_strategy=modal_module.ModalCloudBucketMountStrategy(), + ) + }, + ), + snapshot=modal_module.resolve_snapshot(None, "snapshot"), + app_name="sandbox-tests", + workspace_persistence="snapshot_directory", + ) + session = modal_module.ModalSandboxSession.from_state(state) + + await session.hydrate_workspace( + io.BytesIO(modal_module._encode_snapshot_directory_ref(snapshot_id="snap-dir-123")) + ) + + assert create_calls + volumes = cast(dict[str, object], create_calls[0]["volumes"]) + assert volumes.keys() == {"/mnt/remote"} + assert session._sandbox is not None # noqa: SLF001 + assert session._sandbox.mount_image_calls == [ # noqa: SLF001 + ("/workspace", "snap-dir-123") + ] + + @pytest.mark.asyncio async def test_modal_snapshot_directory_restore_reactivates_durable_workspace_mounts( monkeypatch: pytest.MonkeyPatch, @@ -3596,8 +3684,21 @@ async def test_modal_snapshot_directory_restore_reactivates_durable_workspace_mo @pytest.mark.asyncio +@pytest.mark.parametrize( + ("mount_path", "expected_mount_path"), + [ + pytest.param(Path("actual"), "/workspace/actual", id="canonical"), + pytest.param( + Path("/mnt/../workspace/actual"), + "/mnt/../workspace/actual", + id="normalized", + ), + ], +) async def test_modal_snapshot_directory_persist_only_detaches_durable_workspace_mounts( monkeypatch: pytest.MonkeyPatch, + mount_path: Path, + expected_mount_path: str, ) -> None: modal_module, create_calls, _registry_tags = _load_modal_module(monkeypatch) events: list[tuple[str, str]] = [] @@ -3607,7 +3708,7 @@ async def test_modal_snapshot_directory_persist_only_detaches_durable_workspace_ root="/workspace", entries={ "inside": _RecordingMount( - mount_path=Path("actual"), + mount_path=mount_path, ephemeral=False, ).bind_events(events), "outside": _RecordingMount( @@ -3628,7 +3729,65 @@ async def test_modal_snapshot_directory_persist_only_detaches_durable_workspace_ assert create_calls assert session._sandbox is not None # noqa: SLF001 assert archive.read() == modal_module._encode_snapshot_directory_ref(snapshot_id="im-123") - assert events == [("unmount", "/workspace/actual"), ("mount", "/workspace/actual")] + assert events == [("unmount", expected_mount_path), ("mount", expected_mount_path)] + + +@pytest.mark.asyncio +async def test_modal_snapshot_directory_persist_detaches_durable_ancestor_mount( + monkeypatch: pytest.MonkeyPatch, +) -> None: + modal_module, _create_calls, _registry_tags = _load_modal_module(monkeypatch) + events: list[tuple[str, str]] = [] + + state = modal_module.ModalSandboxSessionState( + manifest=Manifest( + root="/workspace/project", + entries={ + "remote": _RecordingMount( + mount_path=Path("/workspace"), + ephemeral=False, + ).bind_events(events) + }, + ), + snapshot=modal_module.resolve_snapshot(None, "snapshot"), + app_name="sandbox-tests", + workspace_persistence="snapshot_directory", + ) + session = modal_module.ModalSandboxSession.from_state(state) + + archive = await session.persist_workspace() + + assert archive.read() == modal_module._encode_snapshot_directory_ref(snapshot_id="im-123") + assert events == [("unmount", "/workspace"), ("mount", "/workspace")] + + +@pytest.mark.asyncio +async def test_modal_snapshot_directory_persist_detaches_ephemeral_overlapping_mount( + monkeypatch: pytest.MonkeyPatch, +) -> None: + modal_module, _create_calls, _registry_tags = _load_modal_module(monkeypatch) + events: list[tuple[str, str]] = [] + + state = modal_module.ModalSandboxSessionState( + manifest=Manifest( + root="/workspace", + entries={ + "remote": _RecordingMount( + mount_path=Path("remote"), + ephemeral=True, + ).bind_events(events) + }, + ), + snapshot=modal_module.resolve_snapshot(None, "snapshot"), + app_name="sandbox-tests", + workspace_persistence="snapshot_directory", + ) + session = modal_module.ModalSandboxSession.from_state(state) + + archive = await session.persist_workspace() + + assert archive.read() == modal_module._encode_snapshot_directory_ref(snapshot_id="im-123") + assert events == [("unmount", "/workspace/remote"), ("mount", "/workspace/remote")] @pytest.mark.asyncio @@ -4234,8 +4393,14 @@ async def _fake_tar_persist() -> io.BytesIO: @pytest.mark.asyncio +@pytest.mark.parametrize( + "mount_path", + [Path("remote"), Path("/mnt/../workspace/remote"), Path("//workspace/remote")], + ids=["canonical", "normalized", "double-slash"], +) async def test_modal_create_rejects_snapshot_directory_with_cloud_bucket_mount_under_workspace( monkeypatch: pytest.MonkeyPatch, + mount_path: Path, ) -> None: modal_module, create_calls, _registry_tags = _load_modal_module(monkeypatch) @@ -4252,6 +4417,7 @@ async def test_modal_create_rejects_snapshot_directory_with_cloud_bucket_mount_u entries={ "remote": S3Mount( bucket="bucket", + mount_path=mount_path, mount_strategy=modal_module.ModalCloudBucketMountStrategy(), ) } @@ -4265,6 +4431,46 @@ async def test_modal_create_rejects_snapshot_directory_with_cloud_bucket_mount_u assert create_calls == [] +@pytest.mark.asyncio +@pytest.mark.parametrize( + "workspace_root", + ["/workspace/project", "/x/../workspace/project", "//workspace/project"], + ids=["canonical", "normalized", "double-slash"], +) +async def test_modal_create_rejects_snapshot_directory_inside_cloud_bucket_mount( + monkeypatch: pytest.MonkeyPatch, + workspace_root: str, +) -> None: + modal_module, create_calls, _registry_tags = _load_modal_module(monkeypatch) + + client = modal_module.ModalSandboxClient() + with pytest.raises( + MountConfigError, + match=( + "snapshot_directory is not supported when the workspace root " + "lives inside a Modal cloud bucket mount" + ), + ): + await client.create( + manifest=Manifest( + root=workspace_root, + entries={ + "remote": S3Mount( + bucket="bucket", + mount_path=Path("/workspace"), + mount_strategy=modal_module.ModalCloudBucketMountStrategy(), + ) + }, + ), + options=modal_module.ModalSandboxClientOptions( + app_name="sandbox-tests", + workspace_persistence="snapshot_directory", + ), + ) + + assert create_calls == [] + + @pytest.mark.asyncio async def test_modal_create_allows_snapshot_directory_with_cloud_bucket_mount_outside_workspace( monkeypatch: pytest.MonkeyPatch, @@ -4293,6 +4499,39 @@ async def test_modal_create_allows_snapshot_directory_with_cloud_bucket_mount_ou assert volumes.keys() == {"/mnt/remote"} +@pytest.mark.asyncio +async def test_modal_snapshot_directory_uses_native_with_disjoint_cloud_bucket_mount( + monkeypatch: pytest.MonkeyPatch, +) -> None: + modal_module, _create_calls, _registry_tags = _load_modal_module(monkeypatch) + + client = modal_module.ModalSandboxClient() + session = await client.create( + manifest=Manifest( + entries={ + "remote": S3Mount( + bucket="bucket", + mount_path=Path("/mnt/remote"), + mount_strategy=modal_module.ModalCloudBucketMountStrategy(), + ) + } + ), + options=modal_module.ModalSandboxClientOptions( + app_name="sandbox-tests", + workspace_persistence="snapshot_directory", + ), + ) + + async def _unexpected_tar_persist() -> io.BytesIO: + return io.BytesIO(b"tar-fallback") + + monkeypatch.setattr(session._inner, "_persist_workspace_via_tar", _unexpected_tar_persist) + + archive = await session.persist_workspace() + + assert archive.read() == modal_module._encode_snapshot_directory_ref(snapshot_id="im-123") + + @pytest.mark.asyncio async def test_modal_clear_workspace_root_on_resume_preserves_nested_cloud_bucket_mounts( monkeypatch: pytest.MonkeyPatch, diff --git a/tests/sandbox/test_session_utils.py b/tests/sandbox/test_session_utils.py index d1ddc828ef..2ec260e22a 100644 --- a/tests/sandbox/test_session_utils.py +++ b/tests/sandbox/test_session_utils.py @@ -7,11 +7,13 @@ import sys import uuid from concurrent.futures import ThreadPoolExecutor -from pathlib import Path +from pathlib import Path, PureWindowsPath +from typing import Literal import pytest from agents.sandbox.entries import GCSMount, InContainerMountStrategy, MountpointMountPattern +from agents.sandbox.entries.mounts.base import Mount from agents.sandbox.errors import ( MountConfigError, WorkspaceArchiveReadError, @@ -83,6 +85,14 @@ def __init__(self, manifest: Manifest) -> None: ) +class _NonDetachableMountStrategy(InContainerMountStrategy): + type: Literal["test_non_detachable"] = "test_non_detachable" + + def supports_native_snapshot_detach(self, mount: Mount) -> bool: + _ = mount + return False + + class _QueuedExecSession(_CaptureExecSession): def __init__(self, results: list[ExecResult]) -> None: super().__init__() @@ -445,3 +455,83 @@ def test_register_persist_workspace_skip_path_allows_non_overlapping_path() -> N registered = session.register_persist_workspace_skip_path("logs/events.jsonl") assert registered == Path("logs/events.jsonl") + + +@pytest.mark.parametrize( + ("snapshot_root", "mount_path", "requires_fallback"), + [ + pytest.param("/workspace", "/mnt/remote", False, id="disjoint"), + pytest.param("/workspace", "/workspace/remote", True, id="descendant"), + pytest.param("/workspace", "/workspace", True, id="equal"), + pytest.param("/workspace/project", "/workspace", True, id="ancestor"), + pytest.param("/workspace", "/mnt/../workspace/remote", True, id="normalized-descendant"), + pytest.param("/x/../workspace/project", "/workspace", True, id="normalized-ancestor"), + pytest.param("/workspace", "//workspace/remote", True, id="double-slash-mount"), + pytest.param("//workspace/project", "/workspace", True, id="double-slash-root"), + ], +) +def test_native_directory_snapshot_fallback_only_considers_overlapping_non_detachable_mounts( + snapshot_root: str, + mount_path: str, + requires_fallback: bool, +) -> None: + resolved_snapshot_root = Path(snapshot_root) + session = _ManifestSession( + Manifest( + root=resolved_snapshot_root.as_posix(), + entries={ + "remote": GCSMount( + bucket="bucket", + mount_path=Path(mount_path), + mount_strategy=_NonDetachableMountStrategy(pattern=MountpointMountPattern()), + ) + }, + ) + ) + + assert ( + session._native_snapshot_requires_tar_fallback(snapshot_root=resolved_snapshot_root) + is requires_fallback + ) + + +def test_native_directory_snapshot_keeps_detachable_overlapping_mounts_native() -> None: + snapshot_root = Path("/workspace") + session = _ManifestSession( + Manifest( + root=snapshot_root.as_posix(), + entries={ + "remote": GCSMount( + bucket="bucket", + mount_path=Path("/workspace/remote"), + mount_strategy=InContainerMountStrategy(pattern=MountpointMountPattern()), + ) + }, + ) + ) + + assert not session._native_snapshot_requires_tar_fallback(snapshot_root=snapshot_root) + + +def test_native_directory_snapshot_overlap_is_independent_of_host_path_flavor() -> None: + assert BaseSandboxSession._sandbox_paths_overlap( + PureWindowsPath("/workspace"), + PureWindowsPath("/workspace/remote"), + ) + + +def test_filesystem_snapshot_still_considers_disjoint_non_detachable_mounts() -> None: + session = _ManifestSession( + Manifest( + root="/workspace", + entries={ + "remote": GCSMount( + bucket="bucket", + mount_path=Path("/mnt/remote"), + mount_strategy=_NonDetachableMountStrategy(pattern=MountpointMountPattern()), + ) + }, + ) + ) + + assert session._native_snapshot_requires_tar_fallback()