fix(filesystem): refuse an edit_file edit whose oldText matches more than once

docs/tools/filesystem/index.md

@@ -44,7 +44,7 @@ This helps agents distinguish between an empty directory and a tool failure, avo
 | `read_file`            | Read the contents of a file (whole file, or a line range of a text file)  |
 | `read_multiple_files`  | Read several files in one call (more efficient than multiple `read_file`) |
 | `write_file`           | Create or overwrite a file with new content                               |
-| `edit_file`            | Make line-based edits (find-and-replace) in an existing file. Each edit must specify a non-empty `oldText` to match and replace; empty `oldText` values are rejected with an error. |
+| `edit_file`            | Make line-based edits (find-and-replace) in an existing file. Each edit must specify a non-empty `oldText` that matches exactly once in the file; empty `oldText` values and ambiguous matches are rejected with an error naming the occurrence count. Add surrounding context to disambiguate, or send one edit per occurrence to change several. |
 | `list_directory`       | List files and directories at a given path (explicitly reports empty directories) |
 | `directory_tree`       | Recursive tree view of a directory                                        |
 | `create_directory`     | Create a new directory (creates parent directories as needed)             |

pkg/acp/filesystem.go

@@ -282,15 +282,11 @@ func (t *FilesystemToolset) handleEditFile(ctx context.Context, toolCall tools.T
 	modifiedContent := resp.Content
 
 	for i, edit := range args.Edits {
-		// strings.Contains always matches "" and strings.Replace would insert
-		// newText at offset 0, silently prepending to the file. Mirrors the
-		// guard in the built-in filesystem toolset, which serves the same
-		// edit_file tool name and schema over a different transport.
-		if edit.OldText == "" {
-			return tools.ResultError(fmt.Sprintf("Edit %d failed: oldText must not be empty", i+1)), nil
-		}
-		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)
 	}

pkg/tools/builtin/filesystem/filesystem.go

@@ -337,7 +337,7 @@ type ReadFileMeta struct {
 }
 
 type Edit struct {
-	OldText string `json:"oldText" jsonschema:"Exact text to replace"`
+	OldText string `json:"oldText" jsonschema:"Exact text to replace. Must be non-empty and must match exactly once in the file: include surrounding context to disambiguate, or send one edit per occurrence to change several."`
 	NewText string `json:"newText" jsonschema:"Replacement text"`
 }
 
@@ -530,7 +530,7 @@ func (t *ToolSet) Tools(context.Context) ([]tools.Tool, error) {
 		{
 			Name:         ToolNameEditFile,
 			Category:     "filesystem",
-			Description:  "Make line-based edits to a text file. Each edit replaces exact line sequences with new content.",
+			Description:  "Make line-based edits to a text file. Each edit replaces exact line sequences with new content. Every oldText must match exactly once.",
 			Parameters:   tools.MustSchemaFor[EditFileArgs](),
 			OutputSchema: tools.MustSchemaFor[string](),
 			Handler:      t.editFileHandler(),
@@ -1010,6 +1010,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)
@@ -1030,13 +1063,10 @@ func (t *ToolSet) handleEditFile(ctx context.Context, args EditFileArgs) (*tools
 
 	var changes []string
 	for i, edit := range args.Edits {
-		// strings.Contains always matches "" and strings.Replace would insert
-		// newText at offset 0, silently prepending to the file.
-		if edit.OldText == "" {
-			return tools.ResultError(fmt.Sprintf("Edit %d failed: oldText must not be empty", i+1)), nil
-		}
-		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)))

pkg/tools/builtin/filesystem/filesystem_test.go

@@ -584,6 +584,103 @@ func TestFilesystemTool_EditFileRejectsEmptyOldText(t *testing.T) {
 	})
 }
 
+// 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()
 
@@ -1647,3 +1744,86 @@ func TestFilesystemTool_EditFileHandlerRefusesWellFormedEmptyOldText(t *testing.
 	require.NoError(t, err)
 	assert.Equal(t, original, string(after))
 }
+
+// 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))
+}

pkg/tui/components/tool/editfile/render.go

@@ -17,6 +17,7 @@ import (
 	"github.com/docker/docker-agent/pkg/concurrent"
 	"github.com/docker/docker-agent/pkg/lrucache"
 	"github.com/docker/docker-agent/pkg/tools"
+	"github.com/docker/docker-agent/pkg/tools/builtin/filesystem"
 	"github.com/docker/docker-agent/pkg/tui/styles"
 	"github.com/docker/docker-agent/pkg/tui/types"
 )
@@ -127,6 +128,10 @@ func renderEditFileUncached(toolCall tools.ToolCall, width int, splitView bool,
 		return ""
 	}
 
+	if notice := refusalNotice(args, toolStatus); notice != "" {
+		return notice
+	}
+
 	var output strings.Builder
 	for i, edit := range args.Edits {
 		if i > 0 {
@@ -148,6 +153,43 @@ func renderEditFileUncached(toolCall tools.ToolCall, width int, splitView bool,
 	return output.String()
 }
 
+// refusalNotice returns the message to show in place of a diff when the edit
+// the user is being asked to approve will be refused, or an empty string when
+// the call is applicable.
+//
+// Without this, an ambiguous or empty oldText is previewed as a
+// first-occurrence diff — strings.Replace(..., 1) always produces one — so the
+// user approves a change the tool then declines, and the diff they saw was
+// never the change that would have been made.
+//
+// Only meaningful before execution: once the tool has run, the file on disk is
+// the outcome and there is nothing left to predict.
+func refusalNotice(args filesystem.EditFileArgs, toolStatus types.ToolStatus) string {
+	if toolStatus != types.ToolStatusConfirmation {
+		return ""
+	}
+
+	content, err := os.ReadFile(args.Path)
+	if err != nil {
+		// The tool reports unreadable paths itself; a preview must not
+		// second-guess it.
+		return ""
+	}
+
+	// Evaluated against the running content exactly as handleEditFile does, so
+	// a preview never refuses a call the tool would accept: an earlier edit may
+	// legitimately remove a duplicate that made a later one ambiguous.
+	running := string(content)
+	for i, edit := range args.Edits {
+		if reason := filesystem.EditFailureReason(running, edit); reason != "" {
+			return styles.ErrorStyle.Render(
+				fmt.Sprintf("This call will be refused — Edit %d: %s", i+1, reason))
+		}
+		running = strings.Replace(running, edit.OldText, edit.NewText, 1)
+	}
+	return ""
+}
+
 // countDiffLines returns the number of added and removed lines for the edit.
 // Results are cached per tool call since arguments are immutable.
 func countDiffLines(toolCall tools.ToolCall, _ types.ToolStatus) (added, removed int) {