From 32a894d4a78cfe85d954529381c6681d04b5666e Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Mon, 16 Mar 2026 09:17:33 -0700 Subject: [PATCH] fix: error on ambiguous matches in edit_files tool (#23125) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem The `edit_files` tool used `strings.ReplaceAll` for exact substring matches, silently replacing **every** occurrence. When an LLM's search string wasn't unique in the file, this caused unintended edits. Fuzzy matches (passes 2 and 3) only replaced the first occurrence, creating inconsistent behavior. Zero matches were also silently ignored. ## Investigation Investigated how **coder/mux** and **openai/codex** handle this: | Tool | Multiple matches | No match | Flag | |---|---|---|---| | **coder/mux** `file_edit_replace_string` | Error (default `replace_count=1`) | Error | `replace_count` (int, default 1, -1=all) | | **openai/codex** `apply_patch` | Uses first match after cursor (structural disambiguation via context lines + `@@` markers) | Error | None (different paradigm) | | **coder/coder** `edit_files` (before) | Exact: replaces all. Fuzzy: replaces first. | Silent success | None | ## Solution Adopted the mux approach (error on ambiguity) with a simpler `replace_all: bool` instead of `replace_count: int`: - **Default (`replace_all: false`)**: search string must match exactly once. Multiple matches → error with guidance: *"search string matches N occurrences. Include more surrounding context to make the match unique, or set replace_all to true"* - **`replace_all: true`**: replaces all occurrences (opt-in for intentional bulk operations like variable renames) - **Zero matches**: now returns an error instead of silently succeeding Chose `bool` over `int` count because: 1. LLMs are bad at counting occurrences 2. The real intent is binary (one specific spot vs. all occurrences) 3. Simpler error recovery loop for the LLM ## Changes | File | Change | |---|---| | `codersdk/workspacesdk/agentconn.go` | Add `ReplaceAll bool` to `FileEdit` struct | | `agent/agentfiles/files.go` | Count matches before replacing; error if >1 and not opted in; error on zero matches; add `countLineMatches` helper | | `codersdk/toolsdk/toolsdk.go` | Expose `replace_all` in tool schema with description | | `agent/agentfiles/files_test.go` | Update existing tests, add `EditEditAmbiguous`, `EditEditReplaceAll`, `NoMatchErrors`, `AmbiguousExactMatch`, `ReplaceAllExact` | --- agent/agentfiles/files.go | 127 ++++++++++++++++++++--------- agent/agentfiles/files_test.go | 71 +++++++++++++++- codersdk/toolsdk/toolsdk.go | 6 +- codersdk/workspacesdk/agentconn.go | 5 +- 4 files changed, 165 insertions(+), 44 deletions(-) diff --git a/agent/agentfiles/files.go b/agent/agentfiles/files.go index a541165abe..75c2c73c68 100644 --- a/agent/agentfiles/files.go +++ b/agent/agentfiles/files.go @@ -447,13 +447,10 @@ func (api *API) editFile(ctx context.Context, path string, edits []workspacesdk. content := string(data) for _, edit := range edits { - var ok bool - content, ok = fuzzyReplace(content, edit.Search, edit.Replace) - if !ok { - api.logger.Warn(ctx, "edit search string not found, skipping", - slog.F("path", path), - slog.F("search_preview", truncate(edit.Search, 64)), - ) + var err error + content, err = fuzzyReplace(content, edit) + if err != nil { + return http.StatusBadRequest, xerrors.Errorf("edit %s: %w", path, err) } } @@ -480,51 +477,92 @@ func (api *API) editFile(ctx context.Context, path string, edits []workspacesdk. return 0, nil } -// fuzzyReplace attempts to find `search` inside `content` and replace its first -// occurrence with `replace`. It uses a cascading match strategy inspired by +// fuzzyReplace attempts to find `search` inside `content` and replace it +// with `replace`. It uses a cascading match strategy inspired by // openai/codex's apply_patch: // // 1. Exact substring match (byte-for-byte). // 2. Line-by-line match ignoring trailing whitespace on each line. -// 3. Line-by-line match ignoring all leading/trailing whitespace (indentation-tolerant). +// 3. Line-by-line match ignoring all leading/trailing whitespace +// (indentation-tolerant). // -// When a fuzzy match is found (passes 2 or 3), the replacement is still applied -// at the byte offsets of the original content so that surrounding text (including -// indentation of untouched lines) is preserved. +// When edit.ReplaceAll is false (the default), the search string must +// match exactly one location. If multiple matches are found, an error +// is returned asking the caller to include more context or set +// replace_all. // -// Returns the (possibly modified) content and a bool indicating whether a match -// was found. -func fuzzyReplace(content, search, replace string) (string, bool) { - // Pass 1 – exact substring (replace all occurrences). +// When a fuzzy match is found (passes 2 or 3), the replacement is still +// applied at the byte offsets of the original content so that surrounding +// text (including indentation of untouched lines) is preserved. +func fuzzyReplace(content string, edit workspacesdk.FileEdit) (string, error) { + search := edit.Search + replace := edit.Replace + + // Pass 1 – exact substring match. if strings.Contains(content, search) { - return strings.ReplaceAll(content, search, replace), true + if edit.ReplaceAll { + return strings.ReplaceAll(content, search, replace), nil + } + count := strings.Count(content, search) + if count > 1 { + return "", xerrors.Errorf("search string matches %d occurrences "+ + "(expected exactly 1). Include more surrounding "+ + "context to make the match unique, or set "+ + "replace_all to true", count) + } + // Exactly one match. + return strings.Replace(content, search, replace, 1), nil } - // For line-level fuzzy matching we split both content and search into lines. + // For line-level fuzzy matching we split both content and search + // into lines. contentLines := strings.SplitAfter(content, "\n") searchLines := strings.SplitAfter(search, "\n") - // A trailing newline in the search produces an empty final element from - // SplitAfter. Drop it so it doesn't interfere with line matching. + // A trailing newline in the search produces an empty final element + // from SplitAfter. Drop it so it doesn't interfere with line + // matching. if len(searchLines) > 0 && searchLines[len(searchLines)-1] == "" { searchLines = searchLines[:len(searchLines)-1] } - // Pass 2 – trim trailing whitespace on each line. - if start, end, ok := seekLines(contentLines, searchLines, func(a, b string) bool { + trimRight := func(a, b string) bool { return strings.TrimRight(a, " \t\r\n") == strings.TrimRight(b, " \t\r\n") - }); ok { - return spliceLines(contentLines, start, end, replace), true } - - // Pass 3 – trim all leading and trailing whitespace (indentation-tolerant). - if start, end, ok := seekLines(contentLines, searchLines, func(a, b string) bool { + trimAll := func(a, b string) bool { return strings.TrimSpace(a) == strings.TrimSpace(b) - }); ok { - return spliceLines(contentLines, start, end, replace), true } - return content, false + // Pass 2 – trim trailing whitespace on each line. + if start, end, ok := seekLines(contentLines, searchLines, trimRight); ok { + if !edit.ReplaceAll { + if count := countLineMatches(contentLines, searchLines, trimRight); count > 1 { + return "", xerrors.Errorf("search string matches %d occurrences "+ + "(expected exactly 1). Include more surrounding "+ + "context to make the match unique, or set "+ + "replace_all to true", count) + } + } + return spliceLines(contentLines, start, end, replace), nil + } + + // Pass 3 – trim all leading and trailing whitespace + // (indentation-tolerant). + if start, end, ok := seekLines(contentLines, searchLines, trimAll); ok { + if !edit.ReplaceAll { + if count := countLineMatches(contentLines, searchLines, trimAll); count > 1 { + return "", xerrors.Errorf("search string matches %d occurrences "+ + "(expected exactly 1). Include more surrounding "+ + "context to make the match unique, or set "+ + "replace_all to true", count) + } + } + return spliceLines(contentLines, start, end, replace), nil + } + + return "", xerrors.New("search string not found in file. Verify the search " + + "string matches the file content exactly, including whitespace " + + "and indentation") } // seekLines scans contentLines looking for a contiguous subsequence that matches @@ -549,6 +587,26 @@ outer: return 0, 0, false } +// countLineMatches counts how many non-overlapping contiguous +// subsequences of contentLines match searchLines according to eq. +func countLineMatches(contentLines, searchLines []string, eq func(a, b string) bool) int { + count := 0 + if len(searchLines) == 0 || len(searchLines) > len(contentLines) { + return count + } +outer: + for i := 0; i <= len(contentLines)-len(searchLines); i++ { + for j, sLine := range searchLines { + if !eq(contentLines[i+j], sLine) { + continue outer + } + } + count++ + i += len(searchLines) - 1 // skip past this match + } + return count +} + // spliceLines replaces contentLines[start:end] with replacement text, returning // the full content as a single string. func spliceLines(contentLines []string, start, end int, replacement string) string { @@ -562,10 +620,3 @@ func spliceLines(contentLines []string, start, end int, replacement string) stri } return b.String() } - -func truncate(s string, n int) string { - if len(s) <= n { - return s - } - return s[:n] + "..." -} diff --git a/agent/agentfiles/files_test.go b/agent/agentfiles/files_test.go index 1428bb9c28..6290de25e7 100644 --- a/agent/agentfiles/files_test.go +++ b/agent/agentfiles/files_test.go @@ -576,7 +576,9 @@ func TestEditFiles(t *testing.T) { expected: map[string]string{filepath.Join(tmpdir, "edit1"): "bar bar"}, }, { - name: "EditEdit", // Edits affect previous edits. + // When the second edit creates ambiguity (two "bar" + // occurrences), it should fail. + name: "EditEditAmbiguous", contents: map[string]string{filepath.Join(tmpdir, "edit-edit"): "foo bar"}, edits: []workspacesdk.FileEdits{ { @@ -593,7 +595,33 @@ func TestEditFiles(t *testing.T) { }, }, }, - expected: map[string]string{filepath.Join(tmpdir, "edit-edit"): "qux qux"}, + errCode: http.StatusBadRequest, + errors: []string{"matches 2 occurrences"}, + // File should not be modified on error. + expected: map[string]string{filepath.Join(tmpdir, "edit-edit"): "foo bar"}, + }, + { + // With replace_all the cascading edit replaces + // both occurrences. + name: "EditEditReplaceAll", + contents: map[string]string{filepath.Join(tmpdir, "edit-edit-ra"): "foo bar"}, + edits: []workspacesdk.FileEdits{ + { + Path: filepath.Join(tmpdir, "edit-edit-ra"), + Edits: []workspacesdk.FileEdit{ + { + Search: "foo", + Replace: "bar", + }, + { + Search: "bar", + Replace: "qux", + ReplaceAll: true, + }, + }, + }, + }, + expected: map[string]string{filepath.Join(tmpdir, "edit-edit-ra"): "qux qux"}, }, { name: "Multiline", @@ -720,7 +748,7 @@ func TestEditFiles(t *testing.T) { expected: map[string]string{filepath.Join(tmpdir, "exact-preferred"): "goodbye world"}, }, { - name: "NoMatchStillSucceeds", + name: "NoMatchErrors", contents: map[string]string{filepath.Join(tmpdir, "no-match"): "original content"}, edits: []workspacesdk.FileEdits{ { @@ -733,9 +761,46 @@ func TestEditFiles(t *testing.T) { }, }, }, + errCode: http.StatusBadRequest, + errors: []string{"search string not found in file"}, // File should remain unchanged. expected: map[string]string{filepath.Join(tmpdir, "no-match"): "original content"}, }, + { + name: "AmbiguousExactMatch", + contents: map[string]string{filepath.Join(tmpdir, "ambig-exact"): "foo bar foo baz foo"}, + edits: []workspacesdk.FileEdits{ + { + Path: filepath.Join(tmpdir, "ambig-exact"), + Edits: []workspacesdk.FileEdit{ + { + Search: "foo", + Replace: "qux", + }, + }, + }, + }, + errCode: http.StatusBadRequest, + errors: []string{"matches 3 occurrences"}, + expected: map[string]string{filepath.Join(tmpdir, "ambig-exact"): "foo bar foo baz foo"}, + }, + { + name: "ReplaceAllExact", + contents: map[string]string{filepath.Join(tmpdir, "ra-exact"): "foo bar foo baz foo"}, + edits: []workspacesdk.FileEdits{ + { + Path: filepath.Join(tmpdir, "ra-exact"), + Edits: []workspacesdk.FileEdit{ + { + Search: "foo", + Replace: "qux", + ReplaceAll: true, + }, + }, + }, + }, + expected: map[string]string{filepath.Join(tmpdir, "ra-exact"): "qux bar qux baz qux"}, + }, { name: "MixedWhitespaceMultiline", contents: map[string]string{filepath.Join(tmpdir, "mixed-ws"): "func main() {\n\tresult := compute()\n\tfmt.Println(result)\n}"}, diff --git a/codersdk/toolsdk/toolsdk.go b/codersdk/toolsdk/toolsdk.go index eab0e774f8..9d78d45fa5 100644 --- a/codersdk/toolsdk/toolsdk.go +++ b/codersdk/toolsdk/toolsdk.go @@ -1702,12 +1702,16 @@ var WorkspaceEditFiles = Tool[WorkspaceEditFilesArgs, codersdk.Response]{ "properties": map[string]any{ "search": map[string]any{ "type": "string", - "description": "The old string to replace.", + "description": "The old string to replace. Must uniquely match exactly one location in the file unless replace_all is true. Include enough surrounding context to make the match unique.", }, "replace": map[string]any{ "type": "string", "description": "The new string that replaces the old string.", }, + "replace_all": map[string]any{ + "type": "boolean", + "description": "When true, replaces all occurrences of the search string. Defaults to false, which requires the search string to match exactly once.", + }, }, "required": []string{"search", "replace"}, }, diff --git a/codersdk/workspacesdk/agentconn.go b/codersdk/workspacesdk/agentconn.go index 276770da5d..c12d528b12 100644 --- a/codersdk/workspacesdk/agentconn.go +++ b/codersdk/workspacesdk/agentconn.go @@ -899,8 +899,9 @@ func DefaultReadFileLinesLimits() ReadFileLinesLimits { } type FileEdit struct { - Search string `json:"search"` - Replace string `json:"replace"` + Search string `json:"search"` + Replace string `json:"replace"` + ReplaceAll bool `json:"replace_all,omitempty"` } type FileEdits struct {