diff --git a/pkg/acp/filesystem.go b/pkg/acp/filesystem.go index de9878427c..f6d9158f10 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 5b8d1856dc..f68b434939 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,8 +1050,10 @@ 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) { - return tools.ResultError(fmt.Sprintf("Edit %d failed: old text not found", i+1)), 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))) diff --git a/pkg/tools/builtin/filesystem/filesystem_test.go b/pkg/tools/builtin/filesystem/filesystem_test.go index c09e76ef1f..056042f41e 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() @@ -1536,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)) +}