Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions src/agents/sandbox/apply_patch.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,27 @@ async def apply_operation(
path=operation.path,
)

async def _move_file(self, operation: ApplyPatchOperation) -> ApplyPatchResult:
if operation.move_to is None:
raise ApplyPatchDiffError(
message=f"Missing move destination for path {operation.path}",
path=operation.path,
)

relative_path, display_path = self._resolve_path(operation.path)
destination = self._session.normalize_path(relative_path)
moved_relative_path, moved_display_path = self._resolve_path(operation.move_to)
moved_destination = self._session.normalize_path(moved_relative_path)
payload = await self._read_payload(destination, op_path=operation.path)

if moved_destination != destination:
await self._session.mkdir(moved_destination.parent, parents=True, user=self._user)
data = io.StringIO(payload) if isinstance(payload, str) else io.BytesIO(payload)
await self._session.write(moved_destination, data, user=self._user)
await self._session.rm(destination, user=self._user)
Comment on lines +152 to +155

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve filesystem metadata for move-only patches

When the source is executable and the destination does not already exist, implementing the advertised rename as read/write/remove copies only the payload and drops file metadata. For example, UnixLocalSandbox.write() creates the destination with open("wb") (src/agents/sandbox/sandboxes/unix_local.py:974-977), so moving a 0755 script typically produces a non-executable 0644 file and later sandbox commands can fail even though no content hunk was requested. Use a rename primitive or explicitly preserve the source metadata before removing it.

Useful? React with 👍 / 👎.


return ApplyPatchResult(output=f"Moved {display_path} to {moved_display_path}")

def normalize_operation(self, operation: ApplyPatchOperation) -> ApplyPatchOperation:
"""Return an operation whose paths use the workspace policy's canonical form."""
normalized_path = self._validate_path(operation.path).as_posix()
Expand Down Expand Up @@ -187,6 +208,16 @@ async def _ensure_exists(self, destination: Path, *, display_path: str) -> None:
handle.close()

async def _read_text(self, destination: Path, *, op_path: str, decode_path: Path) -> str:
payload = await self._read_payload(destination, op_path=op_path)

if isinstance(payload, str):
return payload
try:
return payload.decode("utf-8")
except UnicodeDecodeError as exc:
raise ApplyPatchDecodeError(path=decode_path, cause=exc) from exc

async def _read_payload(self, destination: Path, *, op_path: str) -> str | bytes:
try:
handle = await self._session.read(destination, user=self._user)
except (FileNotFoundError, WorkspaceReadNotFoundError) as exc:
Expand All @@ -200,10 +231,7 @@ async def _read_text(self, destination: Path, *, op_path: str, decode_path: Path
if isinstance(payload, str):
return payload
if isinstance(payload, bytes | bytearray):
try:
return bytes(payload).decode("utf-8")
except UnicodeDecodeError as exc:
raise ApplyPatchDecodeError(path=decode_path, cause=exc) from exc
return bytes(payload)
raise ApplyPatchDiffError(
message=f"apply_patch read() returned non-text content: {type(payload).__name__}",
path=op_path,
Expand Down
20 changes: 14 additions & 6 deletions src/agents/sandbox/capabilities/tools/apply_patch_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
hunk: add_hunk | delete_hunk | update_hunk
add_hunk: "*** Add File: " filename LF add_line+
delete_hunk: "*** Delete File: " filename LF
update_hunk: "*** Update File: " filename LF change_move? change?
update_hunk: "*** Update File: " filename LF (change_move change? | change)

filename: /(.+)/
add_line: "+" /(.*)/ LF -> line
Expand Down Expand Up @@ -60,7 +60,8 @@
*** Update File: <path> - patch an existing file in place (optionally with a rename).

May be immediately followed by *** Move to: <new path> if you want to rename the file.
Then one or more hunks, each introduced by @@ (optionally followed by a hunk header).
Content hunks are optional for a rename. Otherwise, include one or more hunks, each
introduced by @@ (optionally followed by a hunk header).
Within a hunk, each line starts with a space, -, or +.

For context lines:
Expand Down Expand Up @@ -94,7 +95,7 @@
FileOp := AddFile | DeleteFile | UpdateFile
AddFile := "*** Add File: " path NEWLINE { "+" line NEWLINE }
DeleteFile := "*** Delete File: " path NEWLINE
UpdateFile := "*** Update File: " path NEWLINE [ MoveTo ] { Hunk }
UpdateFile := "*** Update File: " path NEWLINE (MoveTo { Hunk } | Hunk { Hunk })
MoveTo := "*** Move to: " newPath NEWLINE
Hunk := "@@" [ header ] NEWLINE { HunkLine } [ "*** End of File" NEWLINE ]
HunkLine := (" " | "-" | "+") text NEWLINE
Expand Down Expand Up @@ -265,7 +266,14 @@ async def _on_invoke_tool(self, ctx: ToolContext[Any], raw_input: str) -> str:
if operation.type == "create_file":
result = await self.editor.create_file(operation)
elif operation.type == "update_file":
result = await self.editor.update_file(operation)
if operation.diff is None and operation.move_to is not None:
result = await WorkspaceEditor(
self.session,
user=self.editor.user,
workspace_scope=self.workspace_scope,
)._move_file(operation)
else:
result = await self.editor.update_file(operation)
elif operation.type == "delete_file":
result = await self.editor.delete_file(operation)
else:
Expand Down Expand Up @@ -389,13 +397,13 @@ def _parse_update_file(lines: list[str], index: int) -> tuple[ApplyPatchOperatio
while index < len(lines) - 1 and not _is_file_operation_header(lines[index]):
diff_lines.append(lines[index])
index += 1
if not diff_lines:
if not diff_lines and move_to is None:
raise ValueError(f"Update File patch for {path} must include a hunk")
return (
ApplyPatchOperation(
type="update_file",
path=path,
diff=_join_diff(diff_lines),
diff=_join_diff(diff_lines) if diff_lines else None,
move_to=move_to,
),
index,
Expand Down
69 changes: 68 additions & 1 deletion tests/sandbox/capabilities/test_apply_patch_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@
from agents.run_internal.tool_actions import CustomToolAction
from agents.sandbox import SandboxWorkspaceScope
from agents.sandbox.capabilities.tools import SandboxApplyPatchTool
from agents.sandbox.errors import ApplyPatchDecodeError, ApplyPatchFileNotFoundError
from agents.sandbox.errors import (
ApplyPatchDecodeError,
ApplyPatchDiffError,
ApplyPatchFileNotFoundError,
)
from agents.sandbox.types import User
from agents.testing import scripted_sandbox_session
from tests.sandbox._apply_patch_test_session import (
Expand All @@ -40,6 +44,10 @@ def test_exposes_custom_apply_patch_tool(self) -> None:
assert tool.tool_config["name"] == "apply_patch"
assert tool.tool_config["format"]["type"] == "grammar"
assert tool.tool_config["format"]["syntax"] == "lark"
assert (
'update_hunk: "*** Update File: " filename LF (change_move change? | change)'
in tool.tool_config["format"]["definition"]
)

def test_converter_uses_sandbox_custom_apply_patch_tool_config(self) -> None:
tool = SandboxApplyPatchTool(session=scripted_sandbox_session())
Expand Down Expand Up @@ -221,6 +229,22 @@ async def test_invalid_patch_input_surfaces_tool_error_after_approval_precheck(s
assert isinstance(result, ToolCallOutputItem)
assert "apply_patch input must start with '*** Begin Patch'" in result.output

@pytest.mark.asyncio
async def test_empty_update_surfaces_tool_error_without_modifying_file(self) -> None:
session = ApplyPatchSession()
session.files[Path("/workspace/notes.txt")] = b"hello\n"
tool = SandboxApplyPatchTool(session=session)

result = await _execute_custom_tool_call(
tool,
context_wrapper=make_context_wrapper(),
raw_input=("*** Begin Patch\n*** Update File: notes.txt\n*** End Patch\n"),
)

assert isinstance(result, ToolCallOutputItem)
assert "Update File patch for notes.txt must include a hunk" in result.output
assert session.files[Path("/workspace/notes.txt")] == b"hello\n"

@pytest.mark.asyncio
async def test_editor_create_update_delete_round_trip(self) -> None:
session = ApplyPatchSession()
Expand Down Expand Up @@ -558,6 +582,27 @@ async def test_editor_move_to_same_path_does_not_remove_the_file(self) -> None:
assert session.rm_users == []
assert session.files[Path("/workspace/existing.txt")] == b"new\n"

@pytest.mark.asyncio
async def test_editor_rejects_move_without_diff_before_filesystem_access(self) -> None:
session = ApplyPatchSession()
session.files[Path("/workspace/existing.txt")] = b"old\n"
tool = SandboxApplyPatchTool(session=session)

with pytest.raises(ApplyPatchDiffError, match="Missing diff"):
await cast(
Awaitable[ApplyPatchResult],
tool.editor.update_file(
ApplyPatchOperation(
type="update_file",
path="existing.txt",
diff=None,
move_to="moved.txt",
)
),
)

assert session.files == {Path("/workspace/existing.txt"): b"old\n"}

@pytest.mark.asyncio
async def test_custom_tool_input_create_update_move_delete(self) -> None:
session = ApplyPatchSession()
Expand Down Expand Up @@ -600,6 +645,28 @@ async def test_custom_tool_input_create_update_move_delete(self) -> None:
)
assert Path("/workspace/moved.txt") not in session.files

@pytest.mark.asyncio
async def test_custom_tool_input_moves_file_without_content_hunk(self) -> None:
session = ApplyPatchSession()
original = b"\xffhello\r\nworld\n"
session.files[Path("/workspace/notes.txt")] = original
tool = SandboxApplyPatchTool(session=session)

result = await _execute_custom_tool_call(
tool,
context_wrapper=make_context_wrapper(),
raw_input=(
"*** Begin Patch\n"
"*** Update File: notes.txt\n"
"*** Move to: moved.txt\n"
"*** End Patch\n"
),
)

assert result.output == "Moved notes.txt to moved.txt"
assert Path("/workspace/notes.txt") not in session.files
assert session.files[Path("/workspace/moved.txt")] == original


async def _execute_custom_tool_call(
tool: SandboxApplyPatchTool,
Expand Down
Loading