feat(patch): detect already-applied edits and return success no-op
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.
This commit is contained in:
Teknium
parent
af27e60603
commit
99d6f55e38
|
|
@ -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"
|
||||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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]]:
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue