Remove undo_edit command from file_editor

[enyst]

Summary

This PR removes the undo_edit command from the file_editor tool as requested in issue #2063. The undo_edit command was used by some older LLMs but is no longer needed as the project is moving towards higher granularity undo mechanisms like checkpointing.

Changes:

• Remove undo_edit from CommandLiteral type and related descriptions in definition.py
• Remove undo_edit method and history tracking from FileEditor class in editor.py
• Remove FileHistoryManager and history.py utility file (only used for undo functionality)
• Update planning_file_editor/definition.py to remove undo_edit reference
• Update tests to remove undo_edit test cases
• Add test to verify undo_edit is no longer available as a command

Fixes #2063

Checklist

• If the PR is changing/adding functionality, are there tests to reflect this?
• If there is an example, have you run the example to make sure that it works?
• If there are instructions on how to run the code, have you followed the instructions and made sure that it works?
• If the feature is significant enough to require documentation, is there a PR open on the OpenHands/docs repository with the same branch name?
• Is the github CI passing?

@enyst can click here to continue refining the PR

---

Agent Server images for this PR

• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server

Variants & Base Images

Variant	Architectures	Base Image	Docs / Tags
java	amd64, arm64	eclipse-temurin:17-jdk	Link
python	amd64, arm64	nikolaik/python-nodejs:python3.12-nodejs22	Link
golang	amd64, arm64	golang:1.21-bookworm	Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:1cdb1c4-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-1cdb1c4-python \
  ghcr.io/openhands/agent-server:1cdb1c4-python

All tags pushed for this build

ghcr.io/openhands/agent-server:1cdb1c4-golang-amd64
ghcr.io/openhands/agent-server:1cdb1c4-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:1cdb1c4-golang-arm64
ghcr.io/openhands/agent-server:1cdb1c4-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:1cdb1c4-java-amd64
ghcr.io/openhands/agent-server:1cdb1c4-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:1cdb1c4-java-arm64
ghcr.io/openhands/agent-server:1cdb1c4-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:1cdb1c4-python-amd64
ghcr.io/openhands/agent-server:1cdb1c4-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-amd64
ghcr.io/openhands/agent-server:1cdb1c4-python-arm64
ghcr.io/openhands/agent-server:1cdb1c4-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-arm64
ghcr.io/openhands/agent-server:1cdb1c4-golang
ghcr.io/openhands/agent-server:1cdb1c4-java
ghcr.io/openhands/agent-server:1cdb1c4-python

About Multi-Architecture Support

• Each variant tag (e.g., 1cdb1c4-python) is a multi-arch manifest supporting both amd64 and arm64
• Docker automatically pulls the correct architecture for your platform
• Individual architecture tags (e.g., 1cdb1c4-python-amd64) are also available if needed

[all-hands-bot]

🟢 Good taste - Clean removal that simplifies the codebase

Analysis:

• Eliminates entire history management subsystem (FileHistoryManager + disk storage)
• Removes stateful tracking from FileEditor - simpler data flow
• The insert method actually gets cleaner (reads file once instead of accumulating lines while iterating)
• Thorough removal: all references gone, tests properly updated

Verdict: ✅ Worth merging - This is dead code removal done right. Less state, less complexity, cleaner abstractions.

Key Insight: Good code removal is as valuable as good code addition - this eliminates ~150 LOC of state management that was serving an unused feature.

[enyst]

@OpenHands Do a /codereview-roasted on this PR. Anything realistically missing?

[github-actions]

Coverage Report •

File	Stmts	Miss	Cover	Missing
openhands-tools/openhands/tools/file_editor
definition.py	83	12	85%	48, 100, 127–128, 131, 153, 156, 159, 166, 168, 170, 172
editor.py	242	176	27%	88, 91, 108, 111, 120–126, 132–138, 140, 158–159, 179–180, 184, 188–189, 198, 202–205, 213–214, 218–220, 226, 229, 234, 237–238, 241, 244–245, 249, 253, 268–270, 279, 282, 288, 293–294, 302–306, 308, 310, 314–315, 319–320, 328–329, 331–334, 336–339, 342–343, 352–353, 356–360, 365–368, 370, 377–378, 384–386, 394–398, 402, 404–405, 412, 415, 420–421, 423, 447–448, 470–471, 473–474, 481, 483, 486, 490–494, 497–498, 501–505, 508, 511–512, 516, 519, 521–522, 528, 532, 551, 556–559, 561, 569, 576, 583, 605–608, 617–619, 648–650, 652–661, 666–669, 682–683, 688, 693, 699, 705
openhands-tools/openhands/tools/planning_file_editor
definition.py	37	21	43%	84, 88, 91–92, 95–96, 99–100, 102, 105, 110, 113, 116–117, 119, 122–124, 127, 133, 140
TOTAL	18247	9252	49%

[enyst]

@OpenHands Do a /codereview-roasted on this PR.

Note: tools are

• serialized to be sent over the wire (verify in agent-server package)
• serialized to be saved in system prompt event

[enyst]

🔴 Needs improvement — Breaking change to persisted event format with no backward compatibility handling

---

[CRITICAL ISSUES]

1. [definition.py] Breaking Change: Old conversations with undo_edit events will fail to load

This is the big one, and it's the kind of thing that will blow up in production at 2 AM.

FileEditorAction and FileEditorObservation both inherit from Schema, which has model_config = ConfigDict(extra="forbid", frozen=True). The command field uses CommandLiteral = Literal["view", "create", "str_replace", "insert"] — undo_edit is gone.

When a user resumes a conversation that was created before this change, and that conversation contains an ActionEvent with action: FileEditorAction(command="undo_edit") or an ObservationEvent with observation: FileEditorObservation(command="undo_edit"), Pydantic will blow up:

pydantic_core.ValidationError: 1 validation error for FileEditorObservation
command
  Input should be 'view', 'create', 'str_replace' or 'insert' [type=literal_error]

This repo has an explicit iron law about this exact situation — see handle_deprecated_model_fields in TextContent and Message. This PR needs the same treatment, but for a Literal value narrowing rather than a removed field.

The pattern here is different from the _DEPRECATED_FIELDS approach because you're narrowing a Literal, not removing a field. The fix should be a @model_validator(mode="before") on both FileEditorAction and FileEditorObservation that intercepts command="undo_edit" and either:

• Converts it to a no-op (e.g., command="view") so the event loads gracefully, or
• Keeps "undo_edit" in the CommandLiteral type but only for deserialization tolerance — the tool schema sent to LLMs (to_mcp_schema()) can still exclude it

The second approach is simpler and more honest. You could also have a separate "storage Literal" vs "active commands Literal" if you want to be clean about it.

This is a "never break userspace" violation. Must fix.

2. [file_cache.py] Dead code left behind

FileCache (openhands-tools/openhands/tools/file_editor/utils/file_cache.py) is now dead code in production. Its only consumer was FileHistoryManager in history.py, which this PR deleted. The file cache module and its test file (tests/tools/file_editor/utils/test_file_cache.py) are dangling corpses.

If you're removing the undo feature and its history, remove ALL of its infrastructure. Don't leave half-rotted dependencies lying around for the next person to trip over.

---

[IMPROVEMENT OPPORTUNITIES]

3. [editor.py] Extra file read in insert — acceptable, good simplification

The old code avoided an extra read_file() call by collecting history_lines during the file-copy loop. The new code does old_file_text = self.read_file(path) up front, then proceeds to copy line-by-line via tempfile as before. The new approach is cleaner and more correct (the old history_lines approach was fragile). The extra read is negligible for any reasonable file size. 👍

4. [test_early_stopper.py] Minor: test now uses insert which is already a default forbidden command

The test_custom_forbidden_commands test was renamed from undo_edit to insert, but insert is already in the default forbidden commands list. Using a non-default command (e.g., "view") would more clearly test the "custom" aspect. Not blocking.

---

[TESTING GAPS]

5. No backward compatibility test for loading old events with command="undo_edit"

Per this repo's own conventions (see the test_v1_10_0_text_content_with_enable_truncation pattern), there should be a test that verifies:

• An old FileEditorObservation dict with command="undo_edit" loads successfully
• An old FileEditorAction dict with command="undo_edit" loads successfully
• Current format without undo_edit still works

This is not optional — it's the regression proof that your backward compat handler actually works.

---

VERDICT

❌ Needs rework — The Literal narrowing breaks deserialization of persisted events. This is the exact production failure mode the repo's deprecation policy was written to prevent.

KEY INSIGHT

Removing a value from a Literal type on a serialized Pydantic model is semantically identical to removing a field from a model with extra="forbid" — both break deserialization of old data. The same backward-compat discipline applies.

[enyst]

Code Review — PR #2064: Remove undo_edit command from file_editor

Thorough review of the full diff (15 files, -818/+190 lines) after the backward compatibility and cleanup commits.

Assessment

The core removal is clean and well-scoped. The backward compat handling follows the established repo pattern correctly. A couple of minor observations below.

---

Backward Compatibility ✅

Verified the full deserialization chain works:

• FileEditorObservation.model_validate({"command": "undo_edit", ...}) → migrates to "view" ✅
• Observation.model_validate({"kind": "FileEditorObservation", "command": "undo_edit", ...}) → routes through DiscriminatedUnionMixin, then hits _handle_deprecated_commands → "view" ✅
• Same for FileEditorAction through Action base ✅
• FileEditorAction.to_mcp_schema() → enum is ["view", "create", "str_replace", "insert"], no undo_edit leaked to LLMs ✅

The model_validator(mode="before") runs before the _validate_subtype(mode="wrap") in the MRO, so the deprecated value is migrated before Literal validation. Correct.

editor.py — insert method change ✅

The old code built old_content by collecting history_lines during the tempfile-copy loop (two separate open() + enumerate passes). The new code reads the file once upfront with self.read_file(path). Cleaner, correct, and the extra read is negligible.

Dead code cleanup ✅

FileCache (only consumer was FileHistoryManager) and its test file removed. Good.

Minor observations (non-blocking)

1. Command type alias (line 174-179) is now identical to CommandLiteral and has zero importers outside this file. Dead weight — could be removed or consolidated. Not blocking since it's pre-existing.

2. test_custom_forbidden_commands in test_early_stopper.py now uses insert as the custom command, but insert is already in the default forbidden list. Using "view" would better test the "custom" aspect. Minor.

LGTM — would approve if I weren't a contributor. 👍

[enyst]

@OpenHands Do a /codereview-roasted on this PR

[openhands-ai]

I'm on it! enyst can track my progress at all-hands.dev

[openhands-ai]

I did a /codereview-roasted of PR #2064 (“Remove undo_edit command from file_editor”) and found the main problem isn’t the deletion itself — it’s the “backward compat” approach.

What I reviewed / concluded

• The PR removes undo_edit from the tool schema and implementation (reasonable).
• But it then adds a Pydantic model_validator that silently rewrites legacy {"command": "undo_edit"} into {"command": "view"} when loading old events.
  • That avoids crashes, but it corrupts intent: an old event that used to mutate state (undo) now becomes a read-only view and may appear to succeed while doing nothing.
  • This is the worst kind of compatibility: silent behavior change.
• I also flagged a smaller issue in insert() where the new approach can cause extra file reads (not catastrophic, but sloppy).

Checklist against your request

• Provided a brutally honest, technically rigorous review (“roasted”)
• Focused on realistic missing pieces, especially serialization/event replay concerns
• Did not modify code, commit, or push anything (review-only request)

Repo state / pushing

No code changes were made by me, so there is nothing to double-check in git diff and nothing to push.

View full conversation

[enyst]

I’m OpenHands (automated reviewer bot).

🔴 Taste Rating: Needs improvement
You mostly did the obvious mechanical removal correctly, but then you tried to be “helpful” in a way that creates silent behavior changes and future confusion. This is exactly how tool protocols rot.

[CRITICAL ISSUES] (must fix)

• [openhands-tools/openhands/tools/file_editor/definition.py] Breaking semantics via silent command rewrite

  • You removed undo_edit (fine), but then you added a Pydantic model_validator that silently migrates {"command": "undo_edit"} to "view".
  • That’s not “backward compatibility”, that’s lying. Any old event that expected state mutation (“undo last edit”) now turns into a read-only view and will “succeed” while doing nothing. That’s the worst kind of compat: failures that look like success.
  • If you need compat for event replay, do it explicitly:
    • either map to a hard failure with a clear error (“undo_edit deprecated”) so replays surface the problem,
    • or preserve the original command in the observation and make the runner handle it,
    • or add a “deprecated_command” field and record the migration.
  • Silent rewrite is a debugging time bomb.

• Protocol/tooling compatibility: you didn’t prove serialization consumers won’t break

  • The reviewer note explicitly mentions tools are serialized “over the wire” and “saved in system prompt event”.
  • You changed schema + removed command + removed files. If any downstream consumer validates enums strictly (agent-server, event replayer), it might choke on historical "undo_edit" unless you handle it correctly.
  • Right now you “handle” it by pretending it was "view". That avoids crashing but corrupts meaning.

[IMPROVEMENT OPPORTUNITIES] (should fix)

• [definition.py] You removed Command alias but left a compatibility hack instead

  • Removing a redundant alias is fine, but the new migration code + dict mapping is extra machinery that didn’t exist. The PR’s stated goal was simplification. This is the opposite.

• [editor.py] Insert path now reads full file twice

  • You added old_file_text = self.read_file(path) before insert so you can populate old_content. That’s correct for diff reporting, but it means for insert you:
    • read file for old_file_text
    • then stream it twice into the temp file (open path twice)
    • then read file again for snippet / new content
  • It’s not catastrophic, but it’s sloppy for large files. If you care about performance, avoid redundant reads (or accept that removing undo history means you no longer keep “history_lines” around and that’s fine, but don’t re-read gratuitously).

• Tests: you added backward-compat tests but compat behavior is questionable

  • I see tests/tools/file_editor/test_backward_compat.py added. The real question: are you testing “old events replay without crashing” or “old behavior preserved”?
  • If the test asserts that undo_edit becomes view, you’re encoding the lie into permanence.

[STYLE / CLARITY NOTES] (minor)

• Comment says “silently migrated… permanent mappings”
  • Permanent? Great, now you’ve committed to silently rewriting meaning forever. That’s not a win.

VERDICT

❌ Needs rework before merge: removing undo_edit is fine, but silently rewriting historical "undo_edit" commands into "view" is a semantic foot-gun. If you want compat for stored events, handle it in a way that preserves truth (explicit deprecation error or explicit “deprecated command encountered” observation), not by pretending nothing happened.

KEY INSIGHT: Backward compatibility isn’t “make it not crash”; it’s “don’t corrupt intent.” Silent migration here corrupts intent.

[openhands-ai]

Looks like there are a few issues preventing this PR from being merged!

• GitHub Actions are failing:
  • Review Thread Gate

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #2064 at branch `openhands/remove-undo-edit`

Feel free to include any additional details that might help me get this PR into a better state.

_{^{You can manage your notification settings}}