From 3a3d1281eb938ce897e466cecb03973c497e08f0 Mon Sep 17 00:00:00 2001 From: emozilla Date: Mon, 12 Oct 2026 13:29:23 -0400 Subject: [PATCH] fix(tools): reject quote escapes that only the replacement carries Quote-drift correction stripped every escaped quote kind in new_string once old_string verified against the file. Only the kinds old_string escapes are proven drift; a kind escaped only in new_string may be an escape the model meant. For a file holding message = "it's fine", the call old_string=it\'s fine, new_string=it\'s \"great\" wrote message = "it's "great"" and reported success. Raise AmbiguousEscapeDriftError for that case, the same rejection the backslash path uses for unverifiable replacement escapes, so the file stays unchanged and the model resends with its intended escaping. Reported by Greptile on #137288. --- tests/tools/test_escape_drift_autocorrect.py | 28 +++++++++++++++++++- tools/escape_drift_autocorrect.py | 16 +++++++++-- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/tests/tools/test_escape_drift_autocorrect.py b/tests/tools/test_escape_drift_autocorrect.py index ad89f3e222..267acdd236 100644 --- a/tests/tools/test_escape_drift_autocorrect.py +++ b/tests/tools/test_escape_drift_autocorrect.py @@ -9,7 +9,7 @@ import json import pytest -from tools.escape_drift_autocorrect import maybe_correct_escape_drift +from tools.escape_drift_autocorrect import AmbiguousEscapeDriftError, maybe_correct_escape_drift @pytest.fixture @@ -38,6 +38,24 @@ def test_mixed_quote_escaping_is_not_corrected(): assert maybe_correct_escape_drift(old, new, content) == (old, new, None) +# The anchor proves only apostrophe drift; the replacement's \" has no evidence either way. +_UNPROVEN_QUOTE_KIND = ( + 'message = "it' + "'" + 's fine"\n', + r"it\'s fine", + r"it\'s \"great\"", +) + + +def test_quote_kind_escaped_only_in_replacement_is_rejected(): + content, old, new = _UNPROVEN_QUOTE_KIND + with pytest.raises(AmbiguousEscapeDriftError): + maybe_correct_escape_drift(old, new, content) + from tools.fuzzy_match import fuzzy_find_and_replace + out, count, _strategy, err = fuzzy_find_and_replace(content, old, new) + assert count == 0 and out == content + assert "intended escaping" in err + + _SHARED_CASES = [ ('pat = r"\\d+"\n', r'pat = r"\\d+"', 'pat = None', 'pat = None\n'), ("x = 'it'\n", r"x = \'it\'", r"x = \'its\'", "x = 'its'\n"), @@ -131,6 +149,14 @@ class TestPatchEndToEnd: assert r.get("success") is not True assert f.read_text() == original + def test_quote_kind_escaped_only_in_replacement_is_rejected(self, workdir): + content, old, new = _UNPROVEN_QUOTE_KIND + f = workdir / "e.py" + f.write_text(content) + r = _patch_tool(path=str(f), old_string=old, new_string=new, task_id="t-drift") + assert r.get("success") is not True + assert f.read_text() == content + def test_quote_mixed_call_rejected_not_mangled(self, workdir): content, old, new = _MIXED_CALLS["quote_new_line_really_escaped"] f = workdir / "b.py" diff --git a/tools/escape_drift_autocorrect.py b/tools/escape_drift_autocorrect.py index 9d2d627409..4159d72697 100644 --- a/tools/escape_drift_autocorrect.py +++ b/tools/escape_drift_autocorrect.py @@ -94,7 +94,8 @@ def maybe_correct_quote_escaping(old_string: str, new_string: str, content: str) appear verbatim in content, which verifies old_string (and rules out a file that really contains ``\\'``/``\\"``). new_string's added code is not in the file, so it is only stripped when ``_quote_escapes_all_spurious`` holds for the whole call; otherwise the call - is left unchanged for the existing reject-and-ask guard. + is left unchanged for the existing reject-and-ask guard. Raises ``AmbiguousEscapeDriftError`` + when new_string escapes a quote kind that old_string does not. """ has_quote_suspect = ( ("\\'" in old_string and "\\'" in new_string) @@ -108,6 +109,16 @@ def maybe_correct_quote_escaping(old_string: str, new_string: str, content: str) stripped_old = _strip_spurious_quote_escapes(old_string) if stripped_old not in content: return old_string, new_string + # Only old_string is checked against the file, so only the quote kinds it escapes are + # proven drift. A kind escaped only in new_string may be an escape the model meant. + unproven = [q for q in ("'", '"') if "\\" + q in new_string and "\\" + q not in old_string] + if unproven: + shown = " and ".join("\\" + q for q in unproven) + raise AmbiguousEscapeDriftError( + "Escape-drift detected: removing the spurious quote escapes in old_string matches " + f"the file, but new_string also contains {shown}, which old_string gives no evidence " + "for. Re-read the file and resend old_string/new_string with their intended escaping." + ) return stripped_old, _strip_spurious_quote_escapes(new_string) @@ -118,7 +129,8 @@ def maybe_correct_escape_drift(old_string: str, new_string: str, content: str) - Tries quote escaping, then backslash correction of the anchor only. Raises ``AmbiguousEscapeDriftError`` for a verified doubled anchor whose replacement - contains backslashes; the caller must return an error without attempting a match. + contains backslashes, or a verified quote correction whose replacement escapes a + quote kind the anchor does not; the caller must return an error without matching. Returns ``(corrected_old, corrected_new, note)``: ``note`` is a human-readable description of the correction that was applied, or ``None`` if neither fired. The caller attaches