fix(sandbox): keep move_to when coercing apply_patch operation mappings - #4242
Merged
Merged
Conversation
seratch
reviewed
Aug 6, 2026
seratch
left a comment
Member
There was a problem hiding this comment.
Confirmed this works well as stated.
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
SandboxSession.apply_patch()accepts operations either asApplyPatchOperationdataclasses or as plain mappings (dict[str, object]). The mapping form is coerced by_coerce_operation_mapping()insrc/agents/sandbox/apply_patch.py, which readstype,path,diff, andctx_wrapper— but notmove_to.ApplyPatchOperation.move_tois a public field, andWorkspaceEditor.apply_operation()implements it: forupdate_fileit writes the patched content to the new path and removes the source. Because the mapping coercion drops the field, a mapping operation that requests a rename is applied in place instead. No error is raised andapply_patch()still returns"Done!", so the caller has no signal that the rename did not happen — the content lands under the old path and the requested destination never appears.The two sibling coercions for the same dataclass already read
move_to:_parse_apply_patch_operation_json()insrc/agents/sandbox/capabilities/tools/apply_patch_tool.py_coerce_apply_patch_move_to()insrc/agents/run_internal/tool_execution.pyso this is a gap in one of three coercion sites, not a change to the supported contract. The model-driven
apply_patchtool paths are unaffected; only the programmatic mapping API is.Repro (before this change):
/workspace/renamed/new.txtb"beta\n"/workspace/old.txtb"beta\n"(updated in place)"Done!""Done!"The identical operation built as
ApplyPatchOperation(..., move_to="renamed/new.txt")already moves the file correctly, so the two accepted input forms disagree.Root cause
_coerce_operation_mapping()enumerates the mapping keys it forwards toApplyPatchOperation.move_towas never added to that list, so it is silently discarded, andapply_operation()then takes itsoperation.move_to is Nonein-place branch.Fix and why it is minimal
Read
move_tofrom the mapping, validate it the same wayapply_patch_tool.pydoes (reject a non-None, non-strvalue), and pass it through. No new abstraction, no new source of truth, no public API change, and no change to which statements run on the already-correct paths — only the previously dropped field is forwarded.An empty
move_tostring is deliberately not given a new error here: it falls through to_validate_path(), which raisesApplyPatchPathError(reason="empty")— exactly what the dataclass form already does. Keeping that shared makes both input forms behave identically.Non-goals
WorkspaceEditor.apply_operation()move semantics (added in Sandbox Agents #2889, and the bound-user behavior fixed in fix(sandbox): remove apply_patch move source as the bound user #4100).apply_patchtool parsers, which already handlemove_to.create_file/delete_filehandling.Test plan
Added to
tests/sandbox/test_apply_patch.py:test_apply_patch_mapping_operation_moves_file— the bug: a mapping operation withmove_towrites to the new path (including a nested destination directory) and removes the source.test_apply_patch_mapping_operation_without_move_to_updates_in_place— happy path unchanged: nomove_tostill updates in place and issues norm.test_apply_patch_mapping_operation_rejects_non_string_move_to— boundary: a non-stringmove_toraisesApplyPatchDiffErrorand leaves the file untouched.The first and third fail on clean
main(upstream/main@b47a0e4b); the second passes before and after, confirming the in-place path is not altered.Before the source change:
After:
Full verification stack:
git diff --checkis clean. No network, model, or API key is needed for any of the added tests.Compatibility
No public API or schema change. Callers that pass a mapping without
move_toare unaffected. Callers that pass a mapping withmove_towere silently getting an in-place update; they now get the move the field asks for, which matches both the dataclass form and the tool parsers.Limits
Verification ran on macOS with Python 3.13. The Python 3.10 matrix run and the integration-test profiles were not run locally; this change touches no version-specific or provider-specific behavior.
Issue number
N/A
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR