From 39fc83dcf9f44712579fa303fb4d229a9421973e Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Thu, 6 Aug 2026 17:32:40 +0330 Subject: [PATCH 1/4] fix(pkg/tools/builtin/filesystem/filesystem.go): fixing problem of only single first match for edit file in filesystem --- pkg/tools/builtin/filesystem/filesystem.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/pkg/tools/builtin/filesystem/filesystem.go b/pkg/tools/builtin/filesystem/filesystem.go index 5b8d1856d..7eb8f2a01 100644 --- a/pkg/tools/builtin/filesystem/filesystem.go +++ b/pkg/tools/builtin/filesystem/filesystem.go @@ -1017,8 +1017,17 @@ func (t *ToolSet) handleEditFile(ctx context.Context, args EditFileArgs) (*tools var changes []string for i, edit := range args.Edits { - if !strings.Contains(modifiedContent, edit.OldText) { + // Counted against the running content, not the original: an earlier edit + // may legitimately have removed a duplicate. Replacing an ambiguous match + // would silently pick the first occurrence, which the caller cannot tell + // apart from the site they meant. + switch n := strings.Count(modifiedContent, edit.OldText); { + case n == 0: return tools.ResultError(fmt.Sprintf("Edit %d failed: old text not found", i+1)), nil + case n > 1: + return tools.ResultError(fmt.Sprintf( + "Edit %d failed: old text appears %d times; include more surrounding context so it matches exactly once", + i+1, n)), nil } modifiedContent = strings.Replace(modifiedContent, edit.OldText, edit.NewText, 1) changes = append(changes, fmt.Sprintf("Edit %d: Replaced %d characters", i+1, len(edit.OldText))) From c3742a9054577a5b69d38cc0921bad7d722234d4 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Thu, 6 Aug 2026 17:33:08 +0330 Subject: [PATCH 2/4] test(pkg/tools/builtin/filesystem/filesystem_test.go): adding edge case tests for single first match edit bug --- .../builtin/filesystem/filesystem_test.go | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/pkg/tools/builtin/filesystem/filesystem_test.go b/pkg/tools/builtin/filesystem/filesystem_test.go index c09e76ef1..97f3c8718 100644 --- a/pkg/tools/builtin/filesystem/filesystem_test.go +++ b/pkg/tools/builtin/filesystem/filesystem_test.go @@ -532,6 +532,103 @@ func TestFilesystemTool_EditFile(t *testing.T) { assert.Contains(t, result.Output, "old text not found") } +// An oldText that matches more than once is ambiguous: strings.Replace(..., 1) +// would rewrite the first occurrence and report a plain success, so the model +// cannot tell whether it edited the site it meant. The caller has to +// disambiguate with more surrounding context instead. +func TestFilesystemTool_EditFileRejectsAmbiguousMatch(t *testing.T) { + t.Parallel() + + // The same assignment in two different blocks — a realistic shape. + const original = "def dev():\n debug = True\n\ndef prod():\n debug = True\n" + + t.Run("two occurrences are refused", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "conf.py"), []byte(original), 0o644)) + + result, err := tool.handleEditFile(t.Context(), EditFileArgs{ + Path: "conf.py", + Edits: []Edit{{OldText: " debug = True", NewText: " debug = False"}}, + }) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.Contains(t, result.Output, "appears 2 times") + + after, err := os.ReadFile(filepath.Join(tmpDir, "conf.py")) + require.NoError(t, err) + assert.Equal(t, original, string(after), "an ambiguous edit must not modify the file") + }) + + t.Run("a uniquely matching edit still applies", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "conf.py"), []byte(original), 0o644)) + + // Enough surrounding context to match exactly once. + result, err := tool.handleEditFile(t.Context(), EditFileArgs{ + Path: "conf.py", + Edits: []Edit{{OldText: "def prod():\n debug = True", NewText: "def prod():\n debug = False"}}, + }) + require.NoError(t, err) + assert.False(t, result.IsError) + + after, err := os.ReadFile(filepath.Join(tmpDir, "conf.py")) + require.NoError(t, err) + assert.Equal(t, "def dev():\n debug = True\n\ndef prod():\n debug = False\n", string(after)) + }) + + // Occurrences must be counted against the running content, not the original: + // an earlier edit can legitimately remove a duplicate and leave the later + // edit unambiguous. + t.Run("an earlier edit may resolve a later edit's ambiguity", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "conf.py"), []byte(original), 0o644)) + + result, err := tool.handleEditFile(t.Context(), EditFileArgs{ + Path: "conf.py", + Edits: []Edit{ + // Removes the first duplicate, using surrounding context. + {OldText: "def dev():\n debug = True", NewText: "def dev():\n debug = None"}, + // Now matches exactly once. + {OldText: " debug = True", NewText: " debug = False"}, + }, + }) + require.NoError(t, err) + assert.False(t, result.IsError, "got: %s", result.Output) + + after, err := os.ReadFile(filepath.Join(tmpDir, "conf.py")) + require.NoError(t, err) + assert.Equal(t, "def dev():\n debug = None\n\ndef prod():\n debug = False\n", string(after)) + }) + + t.Run("an ambiguous later edit discards the earlier one", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "conf.py"), []byte(original), 0o644)) + + result, err := tool.handleEditFile(t.Context(), EditFileArgs{ + Path: "conf.py", + Edits: []Edit{ + {OldText: "def dev():", NewText: "def development():"}, + {OldText: " debug = True", NewText: " debug = False"}, + }, + }) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.Contains(t, result.Output, "Edit 2") + + after, err := os.ReadFile(filepath.Join(tmpDir, "conf.py")) + require.NoError(t, err) + assert.Equal(t, original, string(after), "no edit may be persisted when a later one is rejected") + }) +} + func TestParseEditFileArgs(t *testing.T) { t.Parallel() From e1926afb800158436b398bf228e1735d13ebd519 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Fri, 7 Aug 2026 11:08:38 +0330 Subject: [PATCH 3/4] refactor(filesystem): share the edit validity rule with the ACP toolset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ACP toolset overrides edit_file with its own client-backed handler while serving the same tool name and schema, but carried a copy of the pre-fix loop — so an ambiguous edit was refused over the built-in transport and silently applied to the first match over ACP. The same tool call meant two different things. Extracts EditFailureReason as the single rule both loops apply, so these semantics cannot drift apart again. The rule also gains an explicit empty-oldText branch, checked before the occurrence count: strings.Count(s, "") returns the rune count plus one, so an empty oldText previously fell into the n > 1 arm and reported a meaningless "appears 19 times" with advice that could not be satisfied. The ambiguous-match message now names both remedies. Repeating an identical edit used to be how this schema expressed "change every occurrence", and refusing it is a deliberate behaviour change — but "include more surrounding context" alone reads as "your text is wrong" to a model whose intent was every site, sending it into a useless retry. --- pkg/acp/filesystem.go | 7 +++- pkg/tools/builtin/filesystem/filesystem.go | 48 +++++++++++++++++----- 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/pkg/acp/filesystem.go b/pkg/acp/filesystem.go index de9878427..f6d9158f1 100644 --- a/pkg/acp/filesystem.go +++ b/pkg/acp/filesystem.go @@ -282,8 +282,11 @@ func (t *FilesystemToolset) handleEditFile(ctx context.Context, toolCall tools.T modifiedContent := resp.Content for i, edit := range args.Edits { - if !strings.Contains(modifiedContent, edit.OldText) { - return tools.ResultError(fmt.Sprintf("Edit %d failed: old text not found", i+1)), nil + // Shared with the built-in filesystem toolset: this handler overrides the + // same edit_file tool name and schema, so both must agree on what a valid + // edit is or the call means different things depending on transport. + if reason := filesystem.EditFailureReason(modifiedContent, edit); reason != "" { + return tools.ResultError(fmt.Sprintf("Edit %d failed: %s", i+1, reason)), nil } modifiedContent = strings.Replace(modifiedContent, edit.OldText, edit.NewText, 1) } diff --git a/pkg/tools/builtin/filesystem/filesystem.go b/pkg/tools/builtin/filesystem/filesystem.go index 7eb8f2a01..f68b43493 100644 --- a/pkg/tools/builtin/filesystem/filesystem.go +++ b/pkg/tools/builtin/filesystem/filesystem.go @@ -997,6 +997,39 @@ func (t *ToolSet) editFileHandler() tools.ToolHandler { } } +// EditFailureReason reports why edit cannot be applied to content, or an empty +// string when it can be applied to exactly one site. +// +// It is exported because the ACP toolset overrides edit_file with its own +// client-backed handler while serving the same tool name and schema +// (pkg/acp/filesystem.go). Both loops must agree on what a valid edit is, or the +// same tool call means different things depending on transport — so the rule +// lives here once rather than being duplicated per handler. +// +// Callers supply their own "Edit N failed: " prefix. +func EditFailureReason(content string, edit Edit) string { + // strings.Contains always matches "" and strings.Replace would insert + // newText at offset 0, silently prepending to the file. Checked before the + // occurrence count because strings.Count(s, "") returns the rune count plus + // one, which would otherwise report a meaningless "appears 42 times". + if edit.OldText == "" { + return "oldText must not be empty" + } + + switch n := strings.Count(content, edit.OldText); { + case n == 0: + return "old text not found" + case n > 1: + // Naming the count and both remedies matters: the model's intent may + // have been a single site (needs more context) or every site (needs one + // edit per occurrence, which is how this schema expresses replace-all). + // "Your text is wrong" alone would send it into a useless retry. + return fmt.Sprintf("old text appears %d times; include more surrounding context so it "+ + "matches exactly once, or send one edit per occurrence to change several", n) + } + return "" +} + func (t *ToolSet) handleEditFile(ctx context.Context, args EditFileArgs) (*tools.ToolCallResult, error) { annotateFilesystemSpan(ctx, "edit_file", args.Path) resolvedPath, err := t.resolveAndCheckPath(args.Path) @@ -1017,17 +1050,10 @@ func (t *ToolSet) handleEditFile(ctx context.Context, args EditFileArgs) (*tools var changes []string for i, edit := range args.Edits { - // Counted against the running content, not the original: an earlier edit - // may legitimately have removed a duplicate. Replacing an ambiguous match - // would silently pick the first occurrence, which the caller cannot tell - // apart from the site they meant. - switch n := strings.Count(modifiedContent, edit.OldText); { - case n == 0: - return tools.ResultError(fmt.Sprintf("Edit %d failed: old text not found", i+1)), nil - case n > 1: - return tools.ResultError(fmt.Sprintf( - "Edit %d failed: old text appears %d times; include more surrounding context so it matches exactly once", - i+1, n)), nil + // Checked against the running content, not the original: an earlier edit + // may legitimately have removed a duplicate. + if reason := EditFailureReason(modifiedContent, edit); reason != "" { + return tools.ResultError(fmt.Sprintf("Edit %d failed: %s", i+1, reason)), nil } modifiedContent = strings.Replace(modifiedContent, edit.OldText, edit.NewText, 1) changes = append(changes, fmt.Sprintf("Edit %d: Replaced %d characters", i+1, len(edit.OldText))) From 661616f63c8ea32690e61efe37d3261fb596b12b Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Fri, 7 Aug 2026 11:08:38 +0330 Subject: [PATCH 4/4] test(filesystem): cover the shared edit rule and replace-all intent Tests EditFailureReason directly since it is now the rule both the built-in and ACP handlers apply, including that an empty oldText reports its own message rather than an occurrence count. Also pins that multi-occurrence intent stays expressible: the old spelling (the same edit twice) is refused, and one context-extended edit per occurrence achieves what the model meant. --- .../builtin/filesystem/filesystem_test.go | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/pkg/tools/builtin/filesystem/filesystem_test.go b/pkg/tools/builtin/filesystem/filesystem_test.go index 97f3c8718..056042f41 100644 --- a/pkg/tools/builtin/filesystem/filesystem_test.go +++ b/pkg/tools/builtin/filesystem/filesystem_test.go @@ -1633,3 +1633,86 @@ func TestFilesystemTool_RootedListDirRefusesSymlinkSwap(t *testing.T) { require.Error(t, err, "rooted readDir must refuse a directory symlink that escapes the allow-list") } + +// EditFailureReason is the single rule both the built-in handler and the ACP +// override apply, so it is tested directly rather than only through them. +func TestEditFailureReason(t *testing.T) { + t.Parallel() + + const content = "a = 1\nb = 2\na = 1\n" + + t.Run("applicable edit has no reason", func(t *testing.T) { + t.Parallel() + assert.Empty(t, EditFailureReason(content, Edit{OldText: "b = 2", NewText: "b = 9"})) + }) + + t.Run("missing text", func(t *testing.T) { + t.Parallel() + assert.Equal(t, "old text not found", + EditFailureReason(content, Edit{OldText: "nope", NewText: "x"})) + }) + + // strings.Count(s, "") returns the rune count plus one, so an empty oldText + // would otherwise fall into the n > 1 arm and report a meaningless + // "appears 19 times" with advice that cannot be satisfied. + t.Run("empty oldText gets its own message, not an occurrence count", func(t *testing.T) { + t.Parallel() + reason := EditFailureReason(content, Edit{OldText: "", NewText: "x"}) + assert.Equal(t, "oldText must not be empty", reason) + assert.NotContains(t, reason, "appears") + }) + + t.Run("ambiguous match names the count and both remedies", func(t *testing.T) { + t.Parallel() + reason := EditFailureReason(content, Edit{OldText: "a = 1", NewText: "a = 9"}) + assert.Contains(t, reason, "appears 2 times") + assert.Contains(t, reason, "more surrounding context") + // A model whose intent was *every* occurrence needs to be told how this + // schema expresses that, or it retries the same payload. + assert.Contains(t, reason, "one edit per occurrence") + }) +} + +// Repeating an identical edit used to be the way to express "change every +// occurrence", and refusing it is a deliberate behaviour change. The intent must +// still be expressible, which is what the error message now points at. +func TestFilesystemTool_EditFileMultiOccurrenceIntentRemainsExpressible(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + tool := New(tmpDir) + const original = "a = 1\nb = 2\na = 1\n" + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "conf.py"), []byte(original), 0o644)) + + // The old spelling — the same edit twice — is now refused. + result, err := tool.handleEditFile(t.Context(), EditFileArgs{ + Path: "conf.py", + Edits: []Edit{ + {OldText: "a = 1", NewText: "a = 9"}, + {OldText: "a = 1", NewText: "a = 9"}, + }, + }) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, result.Output, "one edit per occurrence") + + after, err := os.ReadFile(filepath.Join(tmpDir, "conf.py")) + require.NoError(t, err) + require.Equal(t, original, string(after), "the refused batch must not have written") + + // One edit per occurrence, each carrying enough context to be unique, does + // what the model meant. + result, err = tool.handleEditFile(t.Context(), EditFileArgs{ + Path: "conf.py", + Edits: []Edit{ + {OldText: "a = 1\nb = 2", NewText: "a = 9\nb = 2"}, + {OldText: "b = 2\na = 1", NewText: "b = 2\na = 9"}, + }, + }) + require.NoError(t, err) + require.False(t, result.IsError, result.Output) + + after, err = os.ReadFile(filepath.Join(tmpDir, "conf.py")) + require.NoError(t, err) + assert.Equal(t, "a = 9\nb = 2\na = 9\n", string(after)) +}