From 99d6f55e385bf8d6c4028bc081cd2ba6ee478270 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 2 Aug 2026 11:13:38 -0700 Subject: [PATCH] feat(patch): detect already-applied edits and return success no-op MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #1 patch failure class in production (state.db mining, 250k-window) is a re-send of an edit that already landed: 'old_string and new_string are identical' (299 occurrences) plus a share of hunk-not-found errors where the new text is already in the file. These errored, sending models into re-read/re-patch loops. New tools/fuzzy_match.is_already_applied(content, old, new) — a conservative check requiring (1) non-trivial new_string (>=8 chars), (2) EXACT presence of new_string, (3) old_string gone (unless identical). Wired into three sites: - patch_replace (replace mode): returns success + no_change: true + an explicit note instead of the identical-strings / no-match error. - V4A validation phase: an already-applied hunk validates as a no-op so multi-hunk patches no longer fail wholesale when one hunk landed in a prior call. - V4A apply phase: mirrors the same skip so the two phases agree. Genuine no-matches (new text absent) and half-applied renames (old text still present) keep their error behavior — covered by tests. --- tests/tools/test_patch_already_applied.py | 142 ++++++++++++++++++++++ tools/file_operations.py | 30 ++++- tools/fuzzy_match.py | 27 ++++ tools/patch_parser.py | 16 +++ 4 files changed, 213 insertions(+), 2 deletions(-) create mode 100644 tests/tools/test_patch_already_applied.py diff --git a/tests/tools/test_patch_already_applied.py b/tests/tools/test_patch_already_applied.py new file mode 100644 index 0000000000000..16588d0f2d96f --- /dev/null +++ b/tests/tools/test_patch_already_applied.py @@ -0,0 +1,142 @@ +"""Tests for already-applied patch detection (success-shaped no-op). + +Production mining showed the #1 patch failure class is a re-send of an +edit that already landed: `old_string and new_string are identical` (299 +occurrences in a 250k-window) plus a share of hunk-not-found errors where +new text is already present. These previously errored, sending the model +into re-read/re-patch loops; they now return success with no_change=True. +""" + +import json +import os +import tempfile + +import pytest + +from tools.fuzzy_match import is_already_applied + + +class TestIsAlreadyApplied: + def test_identical_strings_present_in_content(self): + assert is_already_applied("x = compute_value(1)\n", "compute_value(1)", "compute_value(1)") + + def test_identical_strings_absent_from_content(self): + assert not is_already_applied("y = 2\n", "compute_value(1)", "compute_value(1)") + + def test_old_gone_new_present(self): + content = "def new_name(x):\n return x\n" + assert is_already_applied(content, "def old_name(x):", "def new_name(x):") + + def test_old_still_present_means_half_applied(self): + content = "def old_name(x):\n pass\n\ndef new_name(x):\n pass\n" + assert not is_already_applied(content, "def old_name(x):", "def new_name(x):") + + def test_new_absent_not_applied(self): + assert not is_already_applied("def old_name(x):\n", "def old_name(x):", "def new_name(x):") + + def test_trivial_new_string_never_matches(self): + # A short target ("x = 1") appearing by coincidence must not mask a + # genuinely broken edit. + assert not is_already_applied("x = 1\n", "y = 2", "x = 1") + + def test_exact_presence_required(self): + content = "def new_name( x ):\n" # whitespace differs + assert not is_already_applied(content, "def old_name(x):", "def new_name(x):") + + +@pytest.fixture +def workdir(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + return tmp_path + + +def _patch_tool(**kwargs): + from tools.file_tools import patch_tool + return json.loads(patch_tool(**kwargs)) + + +class TestPatchReplaceAlreadyApplied: + def test_identical_old_new_present_is_success_noop(self, workdir): + f = workdir / "a.py" + f.write_text("value = compute_total(items)\n") + r = _patch_tool(path=str(f), old_string="value = compute_total(items)", + new_string="value = compute_total(items)", task_id="t-applied") + assert r["success"] is True + assert r.get("no_change") is True + assert "already" in r["note"] + assert f.read_text() == "value = compute_total(items)\n" + + def test_replay_of_landed_edit_is_success_noop(self, workdir): + # old_string is entirely gone (no approximate remnant for the fuzzy + # chain to latch onto) while new_string is present verbatim. + f = workdir / "b.py" + f.write_text("import os\n\nRETRY_LIMIT_SECONDS = 30\n") + r = _patch_tool(path=str(f), old_string="TIMEOUT_WINDOW_MS = 9000", + new_string="RETRY_LIMIT_SECONDS = 30", task_id="t-applied") + assert r["success"] is True + assert r.get("no_change") is True + + def test_genuine_no_match_still_errors(self, workdir): + f = workdir / "c.py" + f.write_text("something_else = 1\n") + r = _patch_tool(path=str(f), old_string="def missing_function():", + new_string="def replacement_function():", task_id="t-applied") + assert "error" in r + + def test_identical_but_absent_still_errors(self, workdir): + f = workdir / "d.py" + f.write_text("unrelated = True\n") + r = _patch_tool(path=str(f), old_string="def not_here_function():", + new_string="def not_here_function():", task_id="t-applied") + assert "error" in r + + def test_half_applied_rename_still_errors(self, workdir): + # Both old and new text present: NOT already-applied. The identical + # old/new strings short-circuit before any fuzzy matching, and the + # old text still being present must block the no-op path. + f = workdir / "e.py" + f.write_text("def old_fn_name():\n pass\n\ndef new_fn_variant():\n pass\n") + from tools.fuzzy_match import is_already_applied + assert not is_already_applied(f.read_text(), "def old_fn_name():", "def new_fn_variant():") + + +class TestV4AAlreadyApplied: + def test_already_applied_hunk_skipped_in_multi_hunk_patch(self, workdir): + f = workdir / "mod.py" + f.write_text( + "def already_renamed_helper(x):\n" + " return x * 2\n" + "\n" + "def second_helper(y):\n" + " return y + 1\n" + ) + patch_content = ( + "*** Begin Patch\n" + f"*** Update File: {f}\n" + "@@ def already_renamed_helper @@\n" + "-def old_helper_name(x):\n" + "+def already_renamed_helper(x):\n" + "@@ def second_helper @@\n" + "- return y + 1\n" + "+ return y + 2\n" + "*** End Patch\n" + ) + r = _patch_tool(mode="patch", patch=patch_content, task_id="t-v4a") + assert r["success"] is True, r + text = f.read_text() + assert "return y + 2" in text # live hunk applied + assert "already_renamed_helper" in text # no-op hunk left intact + + def test_fully_applied_patch_is_noop_success(self, workdir): + f = workdir / "done.py" + f.write_text("STATUS = 'migrated_to_v2_schema'\n") + patch_content = ( + "*** Begin Patch\n" + f"*** Update File: {f}\n" + "-STATUS = 'legacy_v1_schema'\n" + "+STATUS = 'migrated_to_v2_schema'\n" + "*** End Patch\n" + ) + r = _patch_tool(mode="patch", patch=patch_content, task_id="t-v4a") + assert r["success"] is True, r + assert f.read_text() == "STATUS = 'migrated_to_v2_schema'\n" diff --git a/tools/file_operations.py b/tools/file_operations.py index 0796ec913f68b..2f2edfd4ef4e9 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -205,9 +205,18 @@ class PatchResult: # See :class:`WriteResult.lsp_diagnostics`. lsp_diagnostics: Optional[str] = None error: Optional[str] = None - + # Set on success-shaped no-ops: the requested edit was already present + # in the file, so nothing was written. Carries a short note for the + # model explaining why no diff is included. + no_change: bool = False + note: Optional[str] = None + def to_dict(self) -> dict: - result = {"success": self.success} + result: Dict[str, Any] = {"success": self.success} + if self.no_change: + result["no_change"] = True + if self.note: + result["note"] = self.note if self.diff: result["diff"] = self.diff if self.files_modified: @@ -1631,6 +1640,23 @@ class ShellFileOperations(FileOperations): ) if error or match_count == 0: + # Already-applied detection: the most common patch failure in + # production is a re-send of an edit that has already landed + # (identical old/new strings, or old_string gone while + # new_string is present verbatim). Surface that as an explicit + # success-shaped no-op so the model moves on instead of + # burning turns on re-reads and re-patches. + from tools.fuzzy_match import is_already_applied + if is_already_applied(content, old_string, new_string): + return PatchResult( + success=True, + no_change=True, + note=( + f"File already contains the target text — the edit " + f"appears to be already applied to {path}. No write " + "performed; do not re-send this patch." + ), + ) err_msg = error or f"Could not find match for old_string in {path}" try: from tools.fuzzy_match import format_no_match_hint diff --git a/tools/fuzzy_match.py b/tools/fuzzy_match.py index 44a7885855a39..d0353dd4b6d7a 100644 --- a/tools/fuzzy_match.py +++ b/tools/fuzzy_match.py @@ -64,6 +64,33 @@ def _unicode_normalize(text: str) -> str: return text +def is_already_applied(content: str, old_string: str, new_string: str) -> bool: + """Return True when the requested edit is already present in the file. + + Production trajectory mining shows the most common patch failure is a + re-send of an edit that already landed (old_string == new_string, or + old_string gone while new_string is present) — the model's intent is + "make the file contain this text", and it already does. Callers use + this to convert those errors into an explicit success-shaped no-op so + the model moves on instead of re-reading and re-patching. + + Deliberately conservative: + - new_string must be non-trivial (>= 8 chars stripped) — a tiny target + matching by coincidence must not mask a genuine typo'd edit; + - new_string must appear EXACTLY in the content (no fuzzy matching — + approximate presence is not proof the edit landed); + - when old_string differs from new_string, old_string must be GONE + (still-present old text means the edit is at best half-applied). + """ + if not new_string or len(new_string.strip()) < 8: + return False + if new_string not in content: + return False + if old_string == new_string: + return True + return old_string not in content + + def fuzzy_find_and_replace(content: str, old_string: str, new_string: str, replace_all: bool = False) -> Tuple[str, int, Optional[str], Optional[str]]: """ diff --git a/tools/patch_parser.py b/tools/patch_parser.py index b2c7a701f64f4..271538904ec00 100644 --- a/tools/patch_parser.py +++ b/tools/patch_parser.py @@ -326,6 +326,15 @@ def _validate_operations( simulated, search_pattern, replacement, replace_all=False ) if count == 0: + # Already-applied hunk: validate as a no-op when the + # replacement text is already present (and the search + # text gone) — the edit landed earlier. Keeps multi-hunk + # patches from failing wholesale because one hunk was + # already applied in a prior call. The apply phase + # performs the same skip. + from tools.fuzzy_match import is_already_applied + if is_already_applied(simulated or "", search_pattern, replacement): + continue label = f"'{hunk.context_hint}'" if hunk.context_hint else "(no hint)" msg = ( f"{op.file_path}: hunk {hunk_index} {label} not found" @@ -628,6 +637,13 @@ def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str, Optiona error = None if error: + # Already-applied hunk: skip it, mirroring the + # validation-phase check (validation may also have + # passed via this path, so apply MUST skip too or the + # two phases disagree and the whole patch fails here). + from tools.fuzzy_match import is_already_applied + if is_already_applied(new_content, search_pattern, replacement): + continue err_msg = f"Could not apply hunk: {error}" try: from tools.fuzzy_match import format_no_match_hint