Skip to content

fix(sandbox): remove apply_patch move source as the bound user - #4100

Merged
seratch merged 1 commit into
openai:mainfrom
hsusul:fix/sandbox-apply-patch-move-user
Aug 2, 2026
Merged

seratch merged 1 commit into
openai:mainfrom
hsusul:fix/sandbox-apply-patch-move-user

Conversation

@hsusul

@hsusul hsusul commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Component: src/agents/sandbox/apply_patch.py (WorkspaceEditor), reached through SandboxApplyPatchTool / SandboxCapabilities(run_as=...).

Problem. WorkspaceEditor 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 was the one exception: it was called without user, so it ran as the sandbox's default user instead of the bound one. BaseSandboxSession._prepare_exec_command turns user into a sudo -u <user> -- prefix, so with run_as configured the removal escapes the configured identity (commonly running as root), and if the default user cannot delete the path the rm raises ExecNonZeroError after the destination has already been written — leaving the move half-applied with both files present.

Current behavior (in-memory session recording the user of each call; full script in #4099):

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

Corrected behavior:

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

Root cause. The user=self._user keyword is applied to each session call in WorkspaceEditor, but the move branch's rm was missed. The line has been unchanged since the original Sandbox Agents commit (2d665c9a, #2889).

Implementation. One keyword argument: await self._session.rm(destination, user=self._user). This is the minimal change because the expected value already exists on the instance and the sibling delete_file branch already passes it — no new helper, parameter, or code path is introduced.

Regression tests (tests/sandbox/capabilities/test_apply_patch_tool.py, using the existing UserRecordingApplyPatchSession helper):

  • test_editor_removes_moved_source_as_bound_user — the demonstrated failure: a move runs read/mkdir/write/rm all as the bound user, produces the documented output string, and leaves only the destination file. Fails on main with assert [None] == ['sandbox-user'].
  • test_editor_move_to_same_path_does_not_remove_the_file — nearest boundary: when move_to equals path, no rm happens at all and the updated content is kept. Passes before and after, guarding against the fix introducing a spurious removal.

The pre-existing test_editor_runs_file_operations_as_bound_user continues to cover the create/update/delete paths unchanged.

Compatibility. No public API change. Behavior is identical when no user is bound (user=None was, and still is, what the session receives). Only the move branch is touched; create, update-in-place, and delete are unaffected.

Non-goals. No changes to apply_diff parsing, path validation, approval handling, or any other sandbox capability. I did not audit user propagation across other sandbox tools beyond confirming that this is the only session call in WorkspaceEditor missing the argument.

Test plan

All commands run from the repository root on macOS 15 / Python 3.12.13, with no API key and no network access.

  1. Pre-fix reproduction (test added first, production code untouched):

    uv run pytest tests/sandbox/capabilities/test_apply_patch_tool.py -q -k "bound_user or same_path"
    

    → 1 failed, 2 passed, 7 deselected — failing on assert [None] == ['sandbox-user'], i.e. for the suspected reason.

  2. Same command after the fix:

    uv run pytest tests/sandbox/capabilities/test_apply_patch_tool.py -q
    

    → 10 passed. Repeated 3× with -p no:randomly: 10 passed each time.

  3. Adjacent modules:

    uv run pytest tests/sandbox tests/test_apply_patch_tool.py tests/test_apply_diff.py tests/test_apply_diff_helpers.py -q
    

    → 991 passed, 2 skipped.

  4. Full repository verification stack:

    bash .agents/skills/code-change-verification/scripts/run.sh
    

    → make format (844 files unchanged), make lint (all checks passed), make tests (passed in 71s), make typecheck (mypy + pyright, passed in 201s) — code-change-verification: all commands passed.

  5. git diff --check → clean.

Baseline before any edits, on upstream/main @ a134f3a29869f3aa0e10e37447897046d18c2d23: uv run coverage run -m pytest → 6060 passed, 7 skipped, so there are no pre-existing failures to disentangle from this change.

Not run: make build-docs (no documentation changed) and the integration-tests-* targets (they require live provider credentials, which this change does not touch).

Issue number

Closes #4099

Checks

  • I've added new tests, if relevant
  • I've run .agents/skills/code-change-verification/scripts/run.sh
  • I've confirmed all verification steps pass
  • If using Codex, I've run /review before submitting this PR

@seratch seratch added this to the 0.19.x milestone Aug 2, 2026
@seratch
seratch enabled auto-merge (squash) August 2, 2026 00:58
@seratch
seratch merged commit 98df4ea into openai:main Aug 2, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants