[Sandbox] apply_patch move removes the source file as the default user instead of the bound sandbox user #4099

Please read this first

• Have you read the docs? Yes — sandbox docs and the SandboxApplyPatchTool / WorkspaceEditor sources.
• Have you searched for related issues? Yes. I searched open and closed issues and PRs for apply_patch, sandbox, WorkspaceEditor, move_to, and run_as and found no existing report.

Describe the bug

WorkspaceEditor (src/agents/sandbox/apply_patch.py) is constructed with a bound sandbox user and forwards user=self._user to every sandbox filesystem call it makes — read, mkdir, write, and the rm in the delete_file branch. The rm that removes the source path of an update_file + move_to operation is the one exception: it is called without user, so it runs as the sandbox's default user instead of the bound one.

This is reachable through the normal tool path: SandboxCapabilities builds SandboxApplyPatchTool(session=self.session, user=self.run_as) (src/agents/sandbox/capabilities/filesystem.py), which forwards run_as to SandboxApplyPatchEditor and then to WorkspaceEditor. BaseSandboxSession._prepare_exec_command turns user into a sudo -u <user> -- prefix, so the missing argument means the source file is removed as the default sandbox user (commonly root) while the rest of the very same operation ran as run_as.

Two concrete consequences when run_as is configured:

1. The removal escapes the configured identity: an apply_patch move can delete a source path that the bound user is not permitted to delete.
2. If the default user cannot delete the path (for example a workspace owned by run_as), rm raises ExecNonZeroError after the destination has already been written, so the move half-applies and leaves both the source and destination files behind.

Debug information

• Agents SDK version: 0.19.2 (also reproduces on main @ a134f3a29869f3aa0e10e37447897046d18c2d23)
• Python version: 3.12.13
• Operating system: macOS 15 (the affected code is platform independent)
• Model and model provider: not applicable — no model call is involved
• Does the issue reproduce with the latest Agents SDK release? Yes
• Does the issue occur consistently or intermittently? Consistently

Repro steps

No API key, network access, or real sandbox provider is needed. The script below uses an in-memory BaseSandboxSession that records the user each filesystem call receives.

import asyncio
import io
import uuid
from pathlib import Path

from agents.editor import ApplyPatchOperation
from agents.sandbox import Manifest
from agents.sandbox.capabilities.tools import SandboxApplyPatchTool
from agents.sandbox.session.base_sandbox_session import BaseSandboxSession
from agents.sandbox.snapshot import NoopSnapshot
from agents.sandbox.types import ExecResult, User


class RecordingSession(BaseSandboxSession):
    """In-memory sandbox session that records the user each file call runs as."""

    def __init__(self) -> None:
        self.state = type(
            "S",
            (),
            {
                "manifest": Manifest(root="/workspace"),
                "snapshot": NoopSnapshot(id=str(uuid.uuid4())),
            },
        )()
        self.files: dict[Path, bytes] = {}
        self.calls: list[tuple[str, str | None]] = []

    @staticmethod
    def _name(user):
        return user.name if isinstance(user, User) else user

    async def start(self): ...
    async def stop(self): ...
    async def shutdown(self): ...
    async def running(self) -> bool:
        return True

    async def persist_workspace(self):
        return io.BytesIO()

    async def hydrate_workspace(self, data): ...

    async def _exec_internal(self, *command, timeout=None) -> ExecResult:
        raise AssertionError("unused")

    async def read(self, path, *, user=None):
        self.calls.append(("read", self._name(user)))
        return io.BytesIO(self.files[self.normalize_path(path)])

    async def write(self, path, data, *, user=None):
        self.calls.append(("write", self._name(user)))
        self.files[self.normalize_path(path)] = bytes(data.read())

    async def mkdir(self, path, *, parents=False, user=None):
        self.calls.append(("mkdir", self._name(user)))

    async def rm(self, path, *, recursive=False, user=None):
        self.calls.append(("rm", self._name(user)))
        self.files.pop(self.normalize_path(path), None)


async def main() -> None:
    session = RecordingSession()
    session.files[Path("/workspace/existing.txt")] = b"old\n"
    tool = SandboxApplyPatchTool(session=session, user=User(name="sandbox-user"))

    await tool.editor.update_file(
        ApplyPatchOperation(
            type="update_file",
            path="existing.txt",
            diff="@@\n-old\n+new\n",
            move_to="moved.txt",
        )
    )

    for call, user in session.calls:
        print(f"{call:6} -> user={user!r}")


asyncio.run(main())

Actual output:

read   -> user='sandbox-user'
mkdir  -> user='sandbox-user'
write  -> user='sandbox-user'
rm     -> user=None

Expected behavior

Every filesystem call a WorkspaceEditor makes should run as the user it was constructed with, including the removal of the source path during a move:

read   -> user='sandbox-user'
mkdir  -> user='sandbox-user'
write  -> user='sandbox-user'
rm     -> user='sandbox-user'

This is the contract the surrounding code already establishes: delete_file calls self._session.rm(destination, user=self._user), and tests/sandbox/capabilities/test_apply_patch_tool.py::test_editor_runs_file_operations_as_bound_user already asserts session.rm_users == ["sandbox-user"] for create/update/delete. That test simply never exercises move_to, so the gap went unnoticed.

Root-cause hypothesis (hypothesis, not confirmed by a maintainer): the user=self._user keyword was applied to each session call when WorkspaceEditor gained user binding, and the move branch's rm was missed. The line has been unchanged since the original Sandbox Agents commit (2d665c9a, #2889).

Proposed scope

One-line fix: pass user=self._user to the rm call in the update_file + move_to branch, plus regression tests covering the move path and the move_to == path boundary (where no rm should happen at all). No public API change, no behavior change when no user is bound.

I'd like to work on this and can open a PR.