diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index dc70b01da..02fe33b2d 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -102,10 +102,10 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any } // Fuzzy fallback: when the exact string (and its CRLF translation) is absent, - // run a cascade of tolerant matchers (trimmed lines, block anchors, collapsed - // whitespace, indentation drift, escape normalization) to locate the span the - // model intended. Only a span that occurs literally in the file is accepted, - // so the replacement applied below is still exact. + // run a cascade of tolerant matchers (trimmed lines, collapsed whitespace, + // indentation drift, escape normalization) to locate the span the model + // intended. These transformations must preserve non-whitespace content; a + // merely similar interior is not safe to replace. if occurrences == 0 { findOld, findNew := oldString, newString if strings.Contains(content, "\r\n") && !strings.Contains(findOld, "\r\n") { diff --git a/internal/tools/edit_replacers.go b/internal/tools/edit_replacers.go index 8f23c2bb5..3a3604693 100644 --- a/internal/tools/edit_replacers.go +++ b/internal/tools/edit_replacers.go @@ -2,10 +2,9 @@ package tools // Fuzzy fallback matching for edit_file. When the model's old_string fails to // match byte-for-byte (drifted indentation, collapsed whitespace, escaped -// characters, a slightly-misremembered middle line), these replacers propose -// candidate spans that plausibly correspond to what the model intended. Only a -// candidate that occurs literally in the file is accepted, so the replacement -// itself is always exact even when the match was tolerant. +// characters), these replacers propose candidate spans that differ only by the +// normalization each strategy explicitly handles. They never infer a match +// from similar but different content. // // Strategy cascade ported from opencode's edit tool, whose replacers were in // turn distilled from Cline's diff-apply evals and gemini-cli's edit corrector. @@ -13,7 +12,6 @@ package tools import ( "errors" "regexp" - "sort" "strings" ) @@ -26,9 +24,6 @@ var ( errEditFuzzyAmbiguous = errors.New("fuzzy match for old_string is ambiguous") ) -// Minimum average middle-line similarity for block-anchor matches. -const editAnchorSimilarityThreshold = 0.65 - // fuzzyEditMatch runs the replacer cascade and returns the exact span of // content to replace. When replaceAll is false the span must be unique in // content; an ambiguous candidate is skipped in favor of later candidates and @@ -37,12 +32,10 @@ const editAnchorSimilarityThreshold = 0.65 func fuzzyEditMatch(content, find string, replaceAll bool) (string, error) { replacers := []editReplacer{ lineTrimmedReplacer, - blockAnchorReplacer, whitespaceNormalizedReplacer, indentationFlexibleReplacer, escapeNormalizedReplacer, trimmedBoundaryReplacer, - contextAwareReplacer, } found := false for _, replacer := range replacers { @@ -89,9 +82,8 @@ func fuzzyEditMatch(content, find string, replaceAll bool) (string, error) { return "", errEditFuzzyAmbiguous } -// isDisproportionateEditMatch guards against anchor-style replacers matching a -// span far larger than the text the model asked to replace (e.g. first/last -// line anchors bridging hundreds of unrelated lines). +// isDisproportionateEditMatch guards against a fuzzy replacer matching a span +// far larger than the text the model asked to replace. func isDisproportionateEditMatch(search, find string) bool { findLines := strings.Count(find, "\n") + 1 searchLines := strings.Count(search, "\n") + 1 @@ -148,106 +140,6 @@ func lineTrimmedReplacer(content, find string) []string { return candidates } -// blockAnchorReplacer anchors on the first and last lines (trimmed) and -// accepts the block when the middle lines average >= the similarity threshold -// (Levenshtein-based), tolerating a slightly misremembered interior. -func blockAnchorReplacer(content, find string) []string { - findLines := splitFindLines(find) - if len(findLines) < 3 { - return nil - } - contentLines := strings.Split(content, "\n") - firstAnchor := strings.TrimSpace(findLines[0]) - lastAnchor := strings.TrimSpace(findLines[len(findLines)-1]) - searchBlockSize := len(findLines) - maxLineDelta := searchBlockSize / 4 - if maxLineDelta < 1 { - maxLineDelta = 1 - } - - type span struct{ start, end int } - var candidates []span - for i := 0; i < len(contentLines); i++ { - if strings.TrimSpace(contentLines[i]) != firstAnchor { - continue - } - // Only the first occurrence of the last anchor after this start counts, - // mirroring the reference implementation: a farther-away closing line is - // assumed to close a different block. - for j := i + 2; j < len(contentLines); j++ { - if strings.TrimSpace(contentLines[j]) != lastAnchor { - continue - } - actualBlockSize := j - i + 1 - delta := actualBlockSize - searchBlockSize - if delta < 0 { - delta = -delta - } - if delta <= maxLineDelta { - candidates = append(candidates, span{start: i, end: j}) - } - break - } - } - if len(candidates) == 0 { - return nil - } - - middleSimilarity := func(candidate span) float64 { - actualBlockSize := candidate.end - candidate.start + 1 - linesToCheck := searchBlockSize - 2 - if actualBlockSize-2 < linesToCheck { - linesToCheck = actualBlockSize - 2 - } - if linesToCheck <= 0 { - return 1.0 - } - similarity := 0.0 - for j := 1; j < searchBlockSize-1 && j < actualBlockSize-1; j++ { - contentLine := strings.TrimSpace(contentLines[candidate.start+j]) - findLine := strings.TrimSpace(findLines[j]) - maxLen := len(contentLine) - if len(findLine) > maxLen { - maxLen = len(findLine) - } - if maxLen == 0 { - continue - } - distance := levenshtein(contentLine, findLine) - similarity += 1 - float64(distance)/float64(maxLen) - } - return similarity / float64(linesToCheck) - } - - // Return EVERY candidate that clears the similarity threshold, best first. - // Picking only the best would hide competing blocks from fuzzyEditMatch's - // distinct-candidate ambiguity check and could silently edit the wrong - // block; with all qualifying spans surfaced, one clear winner still - // resolves (single candidate) while two plausible blocks are rejected as - // ambiguous. replaceAll consumers take the first (most similar) span. - type scoredSpan struct { - span span - similarity float64 - } - var qualifying []scoredSpan - for _, candidate := range candidates { - if s := middleSimilarity(candidate); s >= editAnchorSimilarityThreshold { - qualifying = append(qualifying, scoredSpan{span: candidate, similarity: s}) - } - } - if len(qualifying) == 0 { - return nil - } - sort.SliceStable(qualifying, func(i, j int) bool { - return qualifying[i].similarity > qualifying[j].similarity - }) - spans := make([]string, 0, len(qualifying)) - for _, scored := range qualifying { - spans = append(spans, strings.Join(contentLines[scored.span.start:scored.span.end+1], "\n")) - } - return spans -} - var editWhitespaceRun = regexp.MustCompile(`\s+`) func normalizeEditWhitespace(text string) string { @@ -407,49 +299,6 @@ func trimmedBoundaryReplacer(content, find string) []string { return candidates } -// contextAwareReplacer anchors on the first and last lines and accepts an -// equal-length block when at least half of its non-empty middle lines match -// after trimming — a cheaper, stricter cousin of blockAnchorReplacer. -func contextAwareReplacer(content, find string) []string { - findLines := splitFindLines(find) - if len(findLines) < 3 { - return nil - } - contentLines := strings.Split(content, "\n") - firstAnchor := strings.TrimSpace(findLines[0]) - lastAnchor := strings.TrimSpace(findLines[len(findLines)-1]) - var candidates []string - for i := 0; i < len(contentLines); i++ { - if strings.TrimSpace(contentLines[i]) != firstAnchor { - continue - } - for j := i + 2; j < len(contentLines); j++ { - if strings.TrimSpace(contentLines[j]) != lastAnchor { - continue - } - if j-i+1 == len(findLines) { - matching, nonEmpty := 0, 0 - for k := 1; k < len(findLines)-1; k++ { - blockLine := strings.TrimSpace(contentLines[i+k]) - findLine := strings.TrimSpace(findLines[k]) - if blockLine == "" && findLine == "" { - continue - } - nonEmpty++ - if blockLine == findLine { - matching++ - } - } - if nonEmpty == 0 || float64(matching)/float64(nonEmpty) >= 0.5 { - candidates = append(candidates, strings.Join(contentLines[i:j+1], "\n")) - } - } - break - } - } - return candidates -} - // adaptReplacementToSpan re-shapes the model's replacement to the span a // tolerant matcher resolved. When old_string only matched after normalization, // new_string was written at old_string's (wrong) shape, so applying it raw @@ -458,8 +307,7 @@ func contextAwareReplacer(content, find string) []string { // 1. Uniform re-indent: when every span line equals delta + the corresponding // find line's indentation (the line-trimmed / indentation-flexible shapes), // the same delta is prepended to every non-blank replacement line. Any -// line that breaks the uniform-delta relationship disables the shift — -// block-anchor matches with a drifted interior are left untouched. +// line that breaks the uniform-delta relationship disables the shift. // 2. Trailing CR: a span from a CRLF file ends mid-line at "\r" (candidates // are built by joining lines split on "\n"); the replacement gets the same // trailing "\r" so the file's CRLF pairs stay intact. @@ -518,37 +366,3 @@ func uniformIndentDelta(span, find string) (string, bool) { } return delta, haveDelta } - -// levenshtein computes edit distance with a two-row rolling matrix. -func levenshtein(a, b string) int { - if a == "" { - return len(b) - } - if b == "" { - return len(a) - } - previous := make([]int, len(b)+1) - current := make([]int, len(b)+1) - for j := range previous { - previous[j] = j - } - for i := 1; i <= len(a); i++ { - current[0] = i - for j := 1; j <= len(b); j++ { - cost := 1 - if a[i-1] == b[j-1] { - cost = 0 - } - minimum := previous[j] + 1 - if current[j-1]+1 < minimum { - minimum = current[j-1] + 1 - } - if previous[j-1]+cost < minimum { - minimum = previous[j-1] + cost - } - current[j] = minimum - } - previous, current = current, previous - } - return previous[len(b)] -} diff --git a/internal/tools/edit_replacers_test.go b/internal/tools/edit_replacers_test.go index 918e7e251..4fe016d6c 100644 --- a/internal/tools/edit_replacers_test.go +++ b/internal/tools/edit_replacers_test.go @@ -78,33 +78,18 @@ func TestEditFuzzyWhitespaceNormalizedSingleLine(t *testing.T) { } } -func TestEditFuzzyBlockAnchorToleratesMiddleDrift(t *testing.T) { - // First and last lines anchor the block; one interior line differs slightly - // (comment text drifted). Levenshtein similarity keeps it above 0.65. - initial := strings.Join([]string{ - "func handler(w http.ResponseWriter, r *http.Request) {", - "\t// write the response body to the client", - "\tw.WriteHeader(http.StatusOK)", - "\tfmt.Fprint(w, \"done\")", - "}", - "", - }, "\n") - find := strings.Join([]string{ - "func handler(w http.ResponseWriter, r *http.Request) {", - "\t// write the response body to client", - "\tw.WriteHeader(http.StatusOK)", - "\tfmt.Fprint(w, \"done\")", - "}", - }, "\n") +func TestEditFuzzyRejectsInteriorDrift(t *testing.T) { + initial := "func a() {\n\tx := 1\n\ty := 2\n\tz := 3\n\treturn\n}\n" + find := "func a() {\n\tx := 1\n\ty := 99\n\tz := 3\n\treturn\n}" result, after := runEdit(t, t.TempDir(), initial, map[string]any{ "old_string": find, - "new_string": "func handler(w http.ResponseWriter, r *http.Request) {\n\tw.WriteHeader(http.StatusNoContent)\n}", + "new_string": "func a() {}", }) - if result.Status != StatusOK { - t.Fatalf("expected ok, got %q", result.Output) + if result.Status != StatusError || !strings.Contains(result.Output, "Could not find the exact string") { + t.Fatalf("expected exact-match error, got %q", result.Output) } - if !strings.Contains(after, "StatusNoContent") || strings.Contains(after, "StatusOK") { - t.Fatalf("unexpected content: %q", after) + if after != initial { + t.Fatalf("file must be unchanged, got %q", after) } } @@ -168,8 +153,8 @@ func TestUniformIndentDelta(t *testing.T) { } func TestAdaptReplacementLeavesNonUniformSpansAlone(t *testing.T) { - // Block-anchor style match with a drifted interior: no uniform delta, so - // the replacement must pass through untouched. + // A non-uniform span has no safe indentation delta, so the replacement must + // pass through untouched. span := "\tfunc h() {\n\t\t// drifted comment\n\t}" find := "func h() {\n// different comment\n}" if got := adaptReplacementToSpan(span, find, "replacement()"); got != "replacement()" { @@ -283,8 +268,7 @@ func TestEditFuzzyCRLFFile(t *testing.T) { } func TestIsDisproportionateEditMatch(t *testing.T) { - // A candidate span that dwarfs old_string must be refused: anchors bridging - // unrelated code would otherwise delete it all. + // A candidate span that dwarfs old_string must be refused. big := strings.Repeat("line\n", 10) if !isDisproportionateEditMatch(big, "a\nb\nc") { t.Fatal("10-line span for 3-line find must be disproportionate") @@ -300,24 +284,6 @@ func TestIsDisproportionateEditMatch(t *testing.T) { } } -func TestLevenshtein(t *testing.T) { - cases := []struct { - a, b string - want int - }{ - {"", "", 0}, - {"abc", "", 3}, - {"", "abc", 3}, - {"kitten", "sitting", 3}, - {"same", "same", 0}, - } - for _, c := range cases { - if got := levenshtein(c.a, c.b); got != c.want { - t.Fatalf("levenshtein(%q,%q) = %d, want %d", c.a, c.b, got, c.want) - } - } -} - func TestEditFuzzyDistinctCandidatesAreAmbiguous(t *testing.T) { // Two same-content blocks at DIFFERENT indentation: each resolved span is // distinct and occurs exactly once, so the old literal-uniqueness check @@ -336,11 +302,9 @@ func TestEditFuzzyDistinctCandidatesAreAmbiguous(t *testing.T) { } } -func TestEditFuzzyBlockAnchorTwoPlausibleBlocksAmbiguous(t *testing.T) { - // Two blocks share the same first/last anchor lines and BOTH interiors sit - // above the similarity threshold. Picking the "best" one would silently - // edit a block the model may not have meant; both must surface so the - // cascade reports ambiguity and the file stays untouched. +func TestEditFuzzyDoesNotChooseBetweenDriftedBlocks(t *testing.T) { + // Shared first and last lines do not make either drifted interior safe to + // replace. Neither block may be selected. initial := strings.Join([]string{ "func setup(cfg Config) {", "\tvalue := compute(alpha, beta, gamma)", @@ -360,8 +324,8 @@ func TestEditFuzzyBlockAnchorTwoPlausibleBlocksAmbiguous(t *testing.T) { "old_string": find, "new_string": "func setup(cfg Config) {}", }) - if result.Status != StatusError || !strings.Contains(result.Output, "multiple locations") { - t.Fatalf("two plausible anchor blocks must be ambiguous, got %q", result.Output) + if result.Status != StatusError || !strings.Contains(result.Output, "Could not find the exact string") { + t.Fatalf("expected exact-match error, got %q", result.Output) } if after != initial { t.Fatalf("file must be unchanged, got %q", after)