Repository navigation
fix(sandbox): remove apply_patch move source as the bound user - #4100
Merged
Merged
Conversation
seratch
approved these changes
Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Component:
src/agents/sandbox/apply_patch.py(WorkspaceEditor), reached throughSandboxApplyPatchTool/SandboxCapabilities(run_as=...).Problem.
WorkspaceEditoris constructed with a bound sandbox user and forwardsuser=self._userto every sandbox filesystem call it makes —read,mkdir,write, and thermin thedelete_filebranch. Thermthat removes the source path of anupdate_file+move_tooperation was the one exception: it was called withoutuser, so it ran as the sandbox's default user instead of the bound one.BaseSandboxSession._prepare_exec_commandturnsuserinto asudo -u <user> --prefix, so withrun_asconfigured the removal escapes the configured identity (commonly running asroot), and if the default user cannot delete the path thermraisesExecNonZeroErrorafter 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):
Corrected behavior:
Root cause. The
user=self._userkeyword is applied to each session call inWorkspaceEditor, but the move branch'srmwas 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 siblingdelete_filebranch already passes it — no new helper, parameter, or code path is introduced.Regression tests (
tests/sandbox/capabilities/test_apply_patch_tool.py, using the existingUserRecordingApplyPatchSessionhelper):test_editor_removes_moved_source_as_bound_user— the demonstrated failure: a move runsread/mkdir/write/rmall as the bound user, produces the documented output string, and leaves only the destination file. Fails onmainwithassert [None] == ['sandbox-user'].test_editor_move_to_same_path_does_not_remove_the_file— nearest boundary: whenmove_toequalspath, normhappens 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_usercontinues to cover the create/update/delete paths unchanged.Compatibility. No public API change. Behavior is identical when no user is bound (
user=Nonewas, 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_diffparsing, 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 inWorkspaceEditormissing 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.
Pre-fix reproduction (test added first, production code untouched):
→
1 failed, 2 passed, 7 deselected— failing onassert [None] == ['sandbox-user'], i.e. for the suspected reason.Same command after the fix:
→
10 passed. Repeated 3× with-p no:randomly:10 passedeach time.Adjacent modules:
→
991 passed, 2 skipped.Full repository verification stack:
→
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.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 theintegration-tests-*targets (they require live provider credentials, which this change does not touch).Issue number
Closes #4099
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR