Skip to content

fix(sandbox): keep move_to when coercing apply_patch operation mappings - #4242

Merged
seratch merged 1 commit into
openai:mainfrom
hsusul:fix/apply-patch-mapping-move-to
Aug 6, 2026
Merged

fix(sandbox): keep move_to when coercing apply_patch operation mappings#4242
seratch merged 1 commit into
openai:mainfrom
hsusul:fix/apply-patch-mapping-move-to

Conversation

@hsusul

@hsusul hsusul commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

SandboxSession.apply_patch() accepts operations either as ApplyPatchOperation dataclasses or as plain mappings (dict[str, object]). The mapping form is coerced by _coerce_operation_mapping() in src/agents/sandbox/apply_patch.py, which reads type, path, diff, and ctx_wrapper — but not move_to.

ApplyPatchOperation.move_to is a public field, and WorkspaceEditor.apply_operation() implements it: for update_file it 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 and apply_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() in src/agents/sandbox/capabilities/tools/apply_patch_tool.py
  • _coerce_apply_patch_move_to() in src/agents/run_internal/tool_execution.py

so this is a gap in one of three coercion sites, not a change to the supported contract. The model-driven apply_patch tool paths are unaffected; only the programmatic mapping API is.

Repro (before this change):

session.files[Path("/workspace/old.txt")] = b"alpha\n"

await session.apply_patch(
    {
        "type": "update_file",
        "path": "old.txt",
        "diff": "@@\n-alpha\n+beta\n",
        "move_to": "renamed/new.txt",
    }
)
Before After
/workspace/renamed/new.txt missing b"beta\n"
/workspace/old.txt b"beta\n" (updated in place) removed
return value "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 to ApplyPatchOperation. move_to was never added to that list, so it is silently discarded, and apply_operation() then takes its operation.move_to is None in-place branch.

Fix and why it is minimal

Read move_to from the mapping, validate it the same way apply_patch_tool.py does (reject a non-None, non-str value), 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_to string is deliberately not given a new error here: it falls through to _validate_path(), which raises ApplyPatchPathError(reason="empty") — exactly what the dataclass form already does. Keeping that shared makes both input forms behave identically.

Non-goals

Test plan

Added to tests/sandbox/test_apply_patch.py:

  • test_apply_patch_mapping_operation_moves_file — the bug: a mapping operation with move_to writes 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: no move_to still updates in place and issues no rm.
  • test_apply_patch_mapping_operation_rejects_non_string_move_to — boundary: a non-string move_to raises ApplyPatchDiffError and 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:

FAILED tests/sandbox/test_apply_patch.py::test_apply_patch_mapping_operation_moves_file
FAILED tests/sandbox/test_apply_patch.py::test_apply_patch_mapping_operation_rejects_non_string_move_to
2 failed, 1 passed, 16 deselected

After:

uv run pytest tests/sandbox/test_apply_patch.py -q
19 passed

Full verification stack:

env UV_DEFAULT_INDEX=https://pypi.org/simple bash .agents/skills/code-change-verification/scripts/run.sh
make format   -> 862 files left unchanged; ruff check --fix: All checks passed!
make lint     -> passed
make tests    -> passed
make typecheck-> passed
code-change-verification: all commands passed.

git diff --check is 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_to are unaffected. Callers that pass a mapping with move_to were 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

  • 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.20.x milestone Aug 6, 2026

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Confirmed this works well as stated.

@seratch
seratch merged commit f3b6c61 into openai:main Aug 6, 2026
10 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.

2 participants