diff --git a/src/agents/sandbox/remote_mount_policy.py b/src/agents/sandbox/remote_mount_policy.py index 7a7687b812..f183a5f051 100644 --- a/src/agents/sandbox/remote_mount_policy.py +++ b/src/agents/sandbox/remote_mount_policy.py @@ -38,11 +38,7 @@ def build_remote_mount_policy_instructions(manifest: Manifest) -> str | None: allowlist_text = ", ".join( f"`{command}`" for command in manifest.remote_mount_command_allowlist ) - edit_instructions = ( - "Use `apply_patch` directly for text edits. " - "For shell-based edits, first `cp` the mounted file to a normal local workspace path, " - "edit the local copy there, then `cp` it back. " - ) + edit_instructions = _remote_mount_edit_instructions(remote_mounts) return REMOTE_MOUNT_POLICY.format( path_lines=path_lines, REMOTE_MOUNT_COMMAND_ALLOWLIST_TEXT=allowlist_text, @@ -50,6 +46,27 @@ def build_remote_mount_policy_instructions(manifest: Manifest) -> str | None: ) +def _remote_mount_edit_instructions(remote_mounts: list[tuple[Path, bool]]) -> str: + has_read_write = any(not read_only for _, read_only in remote_mounts) + has_read_only = any(read_only for _, read_only in remote_mounts) + + instructions: list[str] = [] + if has_read_write: + instructions.append( + "Use `apply_patch` directly for text edits on read+write mounts. " + "For shell-based edits on read+write mounts, first `cp` the mounted file " + "to a normal local workspace path, edit the local copy there, then copy " + "it back." + ) + if has_read_only: + instructions.append( + "Do not edit paths marked read-only in place, including with `apply_patch`, " + "and do not write edited files back to them. Copy read-only files to a " + "normal local workspace path only if you need an editable scratch copy." + ) + return " ".join(instructions) + + def _format_remote_mount_line(path: Path, read_only: bool) -> str: if read_only: return f"- {path.as_posix()} (mounted in read-only mode)" diff --git a/tests/sandbox/test_remote_mount_policy.py b/tests/sandbox/test_remote_mount_policy.py new file mode 100644 index 0000000000..396e726b41 --- /dev/null +++ b/tests/sandbox/test_remote_mount_policy.py @@ -0,0 +1,57 @@ +from pathlib import Path + +from agents.sandbox.entries import BaseEntry, DockerVolumeMountStrategy, S3Mount +from agents.sandbox.manifest import Manifest +from agents.sandbox.remote_mount_policy import build_remote_mount_policy_instructions + + +def _s3_mount(*, read_only: bool) -> S3Mount: + return S3Mount( + bucket="example-bucket", + mount_strategy=DockerVolumeMountStrategy(driver="rclone"), + read_only=read_only, + ) + + +def _policy_for(entries: dict[str | Path, BaseEntry]) -> str: + policy = build_remote_mount_policy_instructions(Manifest(entries=entries)) + assert policy is not None + return policy + + +def test_remote_mount_policy_does_not_suggest_direct_edits_for_read_only_mounts() -> None: + policy = _policy_for({"data": _s3_mount(read_only=True)}) + + assert "/workspace/data (mounted in read-only mode)" in policy + assert "`apply_patch` directly" not in policy + assert "copy it back" not in policy + assert "Do not edit paths marked read-only in place" in policy + assert "including with `apply_patch`" in policy + assert "do not write edited files back" in policy + + +def test_remote_mount_policy_keeps_direct_and_copy_back_guidance_for_read_write_mounts() -> None: + policy = _policy_for({"data": _s3_mount(read_only=False)}) + + assert "/workspace/data (mounted in read+write mode)" in policy + assert "Use `apply_patch` directly for text edits on read+write mounts." in policy + assert "For shell-based edits on read+write mounts" in policy + assert "copy it back" in policy + assert "Do not edit paths marked read-only" not in policy + + +def test_remote_mount_policy_handles_mixed_read_only_and_read_write_mounts() -> None: + policy = _policy_for( + { + "input": _s3_mount(read_only=True), + "output": _s3_mount(read_only=False), + } + ) + + assert "/workspace/input (mounted in read-only mode)" in policy + assert "/workspace/output (mounted in read+write mode)" in policy + assert "Use `apply_patch` directly for text edits on read+write mounts." in policy + assert "For shell-based edits on read+write mounts" in policy + assert "Do not edit paths marked read-only in place" in policy + assert "including with `apply_patch`" in policy + assert "do not write edited files back" in policy diff --git a/tests/sandbox/test_runtime.py b/tests/sandbox/test_runtime.py index b0b9f7b687..f455f26635 100644 --- a/tests/sandbox/test_runtime.py +++ b/tests/sandbox/test_runtime.py @@ -1220,9 +1220,9 @@ async def test_runner_adds_remote_mount_policy_instructions() -> None: expected_policy_pattern = expected_policy_pattern.replace( re.escape("{edit_instructions}"), re.escape( - "Use `apply_patch` directly for text edits. " - "For shell-based edits, first `cp` the mounted file to a normal local workspace " - "path, edit the local copy there, then `cp` it back. " + "Do not edit paths marked read-only in place, including with `apply_patch`, " + "and do not write edited files back to them. Copy read-only files to a normal " + "local workspace path only if you need an editable scratch copy." ), ) assert isinstance(re.search(expected_policy_pattern, system_instructions), re.Match) @@ -1338,10 +1338,10 @@ async def test_runner_marks_writable_remote_mounts_in_policy() -> None: system_instructions = model.first_turn_args["system_instructions"] assert isinstance(system_instructions, str) assert "- /workspace/remote (mounted in read+write mode)" in system_instructions - assert "Use `apply_patch` directly for text edits." in system_instructions + assert "Use `apply_patch` directly for text edits on read+write mounts." in system_instructions assert ( - "For shell-based edits, first `cp` the mounted file to a normal local workspace path, " - "edit the local copy there, then `cp` it back." in system_instructions + "For shell-based edits on read+write mounts, first `cp` the mounted file to a normal " + "local workspace path, edit the local copy there, then copy it back." in system_instructions )