From ea0c12e33d89f9bd7cec51fb6943c25db6be3809 Mon Sep 17 00:00:00 2001 From: breken-ai <312387581+breken-ai@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:23:25 -0700 Subject: [PATCH] Refuse a hunk that matches more than one place, whatever its length locate only treated a repeated block as ambiguous when it was shorter than three lines. A three-line block that appears twice was placed at the first match and reported as applied. Go's `if err != nil { return err }` is three lines and sits in most functions, so a hunk built from it edited whichever function came first, while the patch instructions tell the model that a hunk whose lines appear more than once is rejected. Any repeated block is now refused with the same disambiguation report short ones already get. Co-Authored-By: Claude Opus 5.5 (1M context) --- internal/repo/patch.go | 8 ++++---- internal/repo/patch_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/internal/repo/patch.go b/internal/repo/patch.go index 32e324b..6f2e3c6 100644 --- a/internal/repo/patch.go +++ b/internal/repo/patch.go @@ -665,9 +665,9 @@ func splitByAnchor(lines []string, current hunk) []hunk { return parts } -// locate finds the one place block occurs in lines. A short block that -// appears more than once is ambiguous, and picking one would risk editing -// the wrong part of the file, so it is refused instead. +// locate finds the one place block occurs in lines. A block that appears +// more than once is ambiguous, however long it is, and picking one would +// risk editing the wrong part of the file, so it is refused instead. func locate(lines, block []string) (int, *HunkError) { matches := findAll(lines, block, func(a, b string) bool { return a == b }) if len(matches) == 0 { @@ -720,7 +720,7 @@ func locate(lines, block []string) (int, *HunkError) { detail += fmt.Sprintf("\n\nthe closest match is at line %d, where it differs:\n expected: %q\n in file: %q", at, expected, found) } return 0, &HunkError{Reason: reason, Expected: expected, Found: found, Detail: detail, Block: block} - case len(matches) > 1 && len(block) < 3: + case len(matches) > 1: reason := fmt.Sprintf("this hunk's lines appear %d times, too ambiguous to place", len(matches)) return 0, &HunkError{Reason: reason, Detail: fmt.Sprintf("%s:\n%s%s", reason, preview(block), disambiguate(lines, matches, block))} default: diff --git a/internal/repo/patch_test.go b/internal/repo/patch_test.go index c080b66..da08c5d 100644 --- a/internal/repo/patch_test.go +++ b/internal/repo/patch_test.go @@ -147,6 +147,34 @@ func TestAmbiguousHunkErrorPointsAtEachMatch(t *testing.T) { } } +func TestApplyIsRefusedWhenALongerHunkIsAmbiguous(t *testing.T) { + // Length does not make a block unique. Go's three-line error check + // appears in almost every function, and a hunk made of exactly that + // could mean any of them: placing it at the first one edits a function + // nobody asked about and reports the patch as applied. + root := t.TempDir() + original := "func a() error {\n\tif err != nil {\n\t\treturn err\n\t}\n\treturn nil\n}\n\n" + + "func b() error {\n\tif err != nil {\n\t\treturn err\n\t}\n\treturn nil\n}\n" + writeFile(t, root, "a.go", original) + repository := &Repository{Root: root} + patch := "@a.go\n" + + " \tif err != nil {\n" + + "-\t\treturn err\n" + + "+\t\treturn fmt.Errorf(\"b: %w\", err)\n" + + " \t}\n" + err := repository.Apply(patch) + if err == nil { + t.Fatal("expected a hunk that matches two places to be refused") + } + if !strings.Contains(err.Error(), "ambiguous") { + t.Fatalf("err = %v, want it to explain the ambiguity", err) + } + content, _ := os.ReadFile(filepath.Join(root, "a.go")) + if string(content) != original { + t.Fatalf("file was modified despite the refusal: %q", content) + } +} + func TestApplyLeavesFilesAloneWhenAHunkCannotBePlaced(t *testing.T) { patch := "--- a/index.html\n+++ b/index.html\n@@ -1,1 +1,1 @@\n" + "- Something Else Entirely\n" +