Merge pull request #3226 from ACMYuechen/fix/writefile-overwrite-guidance

fix(tools): stop write_file from coaching destructive overwrite (#3150)

pkg/agent/instance.go

@@ -103,8 +103,25 @@ func NewAgentInstance(
 			toolsRegistry.Register(tools.NewReadFileBytesTool(workspace, readRestrict, maxReadFileSize, allowReadPaths))
 		}
 	}
+	if cfg.Tools.IsToolEnabled("edit_file") {
+		toolsRegistry.Register(tools.NewEditFileTool(workspace, restrict, allowWritePaths))
+	}
+	if cfg.Tools.IsToolEnabled("append_file") {
+		toolsRegistry.Register(tools.NewAppendFileTool(workspace, restrict, allowWritePaths))
+	}
+	// Build write_file's copy from the registered editors so it steers the agent
+	// to edit_file/append_file only when those tools are actually available.
 	if cfg.Tools.IsToolEnabled("write_file") {
-		toolsRegistry.Register(tools.NewWriteFileTool(workspace, restrict, allowWritePaths))
+		writeTool := tools.NewWriteFileTool(workspace, restrict, allowWritePaths)
+		var altTools []string
+		if toolsRegistry.HasRegistered("append_file") {
+			altTools = append(altTools, "append_file")
+		}
+		if toolsRegistry.HasRegistered("edit_file") {
+			altTools = append(altTools, "edit_file")
+		}
+		writeTool.SetAlternativeTools(altTools)
+		toolsRegistry.Register(writeTool)
 	}
 	if cfg.Tools.IsToolEnabled("list_dir") {
 		toolsRegistry.Register(tools.NewListDirTool(workspace, readRestrict, allowReadPaths))
@@ -119,13 +136,6 @@ func NewAgentInstance(
 		}
 	}
 
-	if cfg.Tools.IsToolEnabled("edit_file") {
-		toolsRegistry.Register(tools.NewEditFileTool(workspace, restrict, allowWritePaths))
-	}
-	if cfg.Tools.IsToolEnabled("append_file") {
-		toolsRegistry.Register(tools.NewAppendFileTool(workspace, restrict, allowWritePaths))
-	}
-
 	sessionsDir := filepath.Join(workspace, "sessions")
 	sessions := initSessionStore(sessionsDir)
 

pkg/agent/instance_test.go

@@ -584,6 +584,102 @@ func TestNewAgentInstance_ReadFileModeSelectsSchema(t *testing.T) {
 	}
 }
 
+// write_file copy names append_file/edit_file only when they are registered.
+func TestNewAgentInstance_WriteFileCopyReflectsAvailableAltTools(t *testing.T) {
+	newCfg := func(editEnabled, appendEnabled bool) *config.Config {
+		return &config.Config{
+			Agents: config.AgentsConfig{
+				Defaults: config.AgentDefaults{
+					Workspace: t.TempDir(),
+					ModelName: "test-model",
+				},
+			},
+			Tools: config.ToolsConfig{
+				WriteFile:  config.ToolConfig{Enabled: true},
+				EditFile:   config.ToolConfig{Enabled: editEnabled},
+				AppendFile: config.ToolConfig{Enabled: appendEnabled},
+			},
+		}
+	}
+
+	writeToolDesc := func(cfg *config.Config) string {
+		agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, &mockProvider{})
+		writeTool, ok := agent.Tools.Get("write_file")
+		if !ok {
+			t.Fatal("write_file tool not registered")
+		}
+		return writeTool.Description()
+	}
+
+	t.Run("only write_file exposed", func(t *testing.T) {
+		desc := writeToolDesc(newCfg(false, false))
+		if strings.Contains(desc, "append_file") || strings.Contains(desc, "edit_file") {
+			t.Fatalf("write_file must not reference unavailable tools, got: %q", desc)
+		}
+	})
+
+	t.Run("only append_file exposed", func(t *testing.T) {
+		desc := writeToolDesc(newCfg(false, true))
+		if !strings.Contains(desc, "append_file") {
+			t.Fatalf("expected write_file to reference append_file, got: %q", desc)
+		}
+		if strings.Contains(desc, "edit_file") {
+			t.Fatalf("write_file must not reference disabled edit_file, got: %q", desc)
+		}
+	})
+
+	t.Run("both exposed", func(t *testing.T) {
+		desc := writeToolDesc(newCfg(true, true))
+		if !strings.Contains(desc, "append_file") || !strings.Contains(desc, "edit_file") {
+			t.Fatalf("expected write_file to reference both alternatives, got: %q", desc)
+		}
+	})
+}
+
+// Availability follows the per-agent allowlist, not just the enable flag:
+// editors enabled globally but hidden by frontmatter must not be named.
+func TestNewAgentInstance_WriteFileCopyExcludesAllowlistHiddenAltTools(t *testing.T) {
+	workspace := setupWorkspace(t, map[string]string{
+		"AGENT.md": "---\ntools: [write_file]\n---\n# Agent\n",
+	})
+	defer cleanupWorkspace(t, workspace)
+
+	cfg := &config.Config{
+		Agents: config.AgentsConfig{
+			Defaults: config.AgentDefaults{
+				Workspace: workspace,
+				ModelName: "test-model",
+			},
+		},
+		Tools: config.ToolsConfig{
+			WriteFile:  config.ToolConfig{Enabled: true},
+			EditFile:   config.ToolConfig{Enabled: true},
+			AppendFile: config.ToolConfig{Enabled: true},
+		},
+	}
+
+	agent := NewAgentInstance(&config.AgentConfig{
+		ID:        "restricted",
+		Workspace: workspace,
+	}, &cfg.Agents.Defaults, cfg, &mockProvider{})
+
+	if _, ok := agent.Tools.Get("edit_file"); ok {
+		t.Fatal("edit_file should be blocked by the allowlist")
+	}
+	if _, ok := agent.Tools.Get("append_file"); ok {
+		t.Fatal("append_file should be blocked by the allowlist")
+	}
+
+	writeTool, ok := agent.Tools.Get("write_file")
+	if !ok {
+		t.Fatal("write_file tool not registered")
+	}
+	if desc := writeTool.Description(); strings.Contains(desc, "append_file") ||
+		strings.Contains(desc, "edit_file") {
+		t.Fatalf("write_file must not name allowlist-hidden tools, got: %q", desc)
+	}
+}
+
 func TestNewAgentInstance_InvalidExecConfigDoesNotExit(t *testing.T) {
 	workspace := t.TempDir()
 

pkg/tools/fs/filesystem.go

@@ -862,7 +862,8 @@ func getInt64Arg(args map[string]any, key string, defaultVal int64) (int64, erro
 }
 
 type WriteFileTool struct {
-	fs fileSystem
+	fs       fileSystem
+	altTools []string
 }
 
 func NewWriteFileTool(
@@ -874,7 +875,32 @@ func NewWriteFileTool(
 	if len(allowPaths) > 0 {
 		patterns = allowPaths[0]
 	}
-	return &WriteFileTool{fs: buildFs(workspace, restrict, patterns)}
+	// Default to both alternatives so standalone callers keep the full guidance;
+	// the agent wiring narrows this to the tools actually registered.
+	return &WriteFileTool{
+		fs:       buildFs(workspace, restrict, patterns),
+		altTools: []string{"append_file", "edit_file"},
+	}
+}
+
+// SetAlternativeTools limits which alternatives the copy names, so it never
+// directs the model to tools that are not available.
+func (t *WriteFileTool) SetAlternativeTools(names []string) {
+	present := make(map[string]bool, len(names))
+	for _, name := range names {
+		present[name] = true
+	}
+	ordered := make([]string, 0, 2)
+	for _, name := range []string{"append_file", "edit_file"} {
+		if present[name] {
+			ordered = append(ordered, name)
+		}
+	}
+	t.altTools = ordered
+}
+
+func (t *WriteFileTool) altToolsPhrase() string {
+	return strings.Join(t.altTools, " or ")
 }
 
 func (t *WriteFileTool) Name() string {
@@ -882,10 +908,24 @@ func (t *WriteFileTool) Name() string {
 }
 
 func (t *WriteFileTool) Description() string {
-	return "Write content to a file. Content is written byte-for-byte after argument decoding. Standard JSON escaping applies: \\n for newline and \\\\n for a literal backslash-n sequence. If the file already exists, you must set overwrite=true to replace it."
+	desc := "Write content to a file, replacing any existing content. Content is written byte-for-byte after argument decoding. Standard JSON escaping applies: \\n for newline and \\\\n for a literal backslash-n sequence. If the file already exists you must set overwrite=true, which replaces the ENTIRE file."
+	if phrase := t.altToolsPhrase(); phrase != "" {
+		desc += fmt.Sprintf(
+			" To add to or change part of an existing file without losing its current contents, use %s instead.",
+			phrase,
+		)
+	}
+	return desc
 }
 
 func (t *WriteFileTool) Parameters() map[string]any {
+	overwriteDesc := "Set to true to replace an existing file in full. This discards the file's current contents."
+	if phrase := t.altToolsPhrase(); phrase != "" {
+		overwriteDesc = fmt.Sprintf(
+			"Set to true to replace an existing file in full. This discards the file's current contents — to preserve them, use %s instead of write_file.",
+			phrase,
+		)
+	}
 	return map[string]any{
 		"type": "object",
 		"properties": map[string]any{
@@ -899,7 +939,7 @@ func (t *WriteFileTool) Parameters() map[string]any {
 			},
 			"overwrite": map[string]any{
 				"type":        "boolean",
-				"description": "Must be set to true to overwrite an existing file.",
+				"description": overwriteDesc,
 				"default":     false,
 			},
 		},
@@ -922,8 +962,20 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *ToolR
 
 	if !overwrite {
 		if _, err := t.fs.Open(path); err == nil {
+			if phrase := t.altToolsPhrase(); phrase != "" {
+				return ErrorResult(
+					fmt.Sprintf(
+						"file: %s already exists. To add to it or change part of it without losing the current contents, use %s. Only set overwrite=true if you intend to replace the entire file.",
+						path,
+						phrase,
+					),
+				)
+			}
 			return ErrorResult(
-				fmt.Sprintf("file: %s already exists. Set overwrite=true to replace.", path),
+				fmt.Sprintf(
+					"file: %s already exists. Set overwrite=true only if you intend to replace the entire file, which discards its current contents.",
+					path,
+				),
 			)
 		}
 	}

pkg/tools/fs/filesystem_test.go

@@ -250,6 +250,9 @@ func TestFilesystemTool_WriteFile_OverwriteDefaultBlocked(t *testing.T) {
 	assert.True(t, result.IsError, "expected error when overwriting without overwrite=true")
 	assert.Contains(t, result.ForLLM, "already exists")
 	assert.Contains(t, result.ForLLM, "overwrite=true")
+	// The guard must steer toward non-destructive tools rather than only coaching overwrite.
+	assert.Contains(t, result.ForLLM, "append_file")
+	assert.Contains(t, result.ForLLM, "edit_file")
 
 	// Original content must be untouched
 	data, err := os.ReadFile(testFile)
@@ -257,6 +260,76 @@ func TestFilesystemTool_WriteFile_OverwriteDefaultBlocked(t *testing.T) {
 	assert.Equal(t, "original", string(data))
 }
 
+// Copy (description, overwrite param, guard) only names available alternatives.
+func TestFilesystemTool_WriteFile_AltToolsConditionalCopy(t *testing.T) {
+	tmpDir := t.TempDir()
+	testFile := filepath.Join(tmpDir, "existing.txt")
+	os.WriteFile(testFile, []byte("original"), 0o644)
+
+	overwriteParamDesc := func(tool *WriteFileTool) string {
+		props := tool.Parameters()["properties"].(map[string]any)
+		return props["overwrite"].(map[string]any)["description"].(string)
+	}
+
+	t.Run("no alternatives available", func(t *testing.T) {
+		tool := NewWriteFileTool("", false)
+		tool.SetAlternativeTools(nil)
+
+		assert.NotContains(t, tool.Description(), "append_file")
+		assert.NotContains(t, tool.Description(), "edit_file")
+		assert.NotContains(t, overwriteParamDesc(tool), "append_file")
+		assert.NotContains(t, overwriteParamDesc(tool), "edit_file")
+
+		result := tool.Execute(context.Background(), map[string]any{
+			"path":    testFile,
+			"content": "new content",
+		})
+		assert.True(t, result.IsError, "expected overwrite guard to still block")
+		assert.Contains(t, result.ForLLM, "already exists")
+		assert.Contains(t, result.ForLLM, "overwrite=true")
+		assert.NotContains(t, result.ForLLM, "append_file")
+		assert.NotContains(t, result.ForLLM, "edit_file")
+	})
+
+	t.Run("only append_file available", func(t *testing.T) {
+		tool := NewWriteFileTool("", false)
+		tool.SetAlternativeTools([]string{"append_file"})
+
+		assert.Contains(t, tool.Description(), "append_file")
+		assert.NotContains(t, tool.Description(), "edit_file")
+		assert.Contains(t, overwriteParamDesc(tool), "append_file")
+		assert.NotContains(t, overwriteParamDesc(tool), "edit_file")
+
+		result := tool.Execute(context.Background(), map[string]any{
+			"path":    testFile,
+			"content": "new content",
+		})
+		assert.True(t, result.IsError)
+		assert.Contains(t, result.ForLLM, "append_file")
+		assert.NotContains(t, result.ForLLM, "edit_file")
+	})
+
+	t.Run("both available uses canonical order", func(t *testing.T) {
+		tool := NewWriteFileTool("", false)
+		// Reversed input to confirm the order is normalized.
+		tool.SetAlternativeTools([]string{"edit_file", "append_file"})
+
+		assert.Contains(t, tool.Description(), "append_file or edit_file")
+
+		result := tool.Execute(context.Background(), map[string]any{
+			"path":    testFile,
+			"content": "new content",
+		})
+		assert.True(t, result.IsError)
+		assert.Contains(t, result.ForLLM, "append_file or edit_file")
+	})
+
+	// Blocked writes must leave the original untouched.
+	data, err := os.ReadFile(testFile)
+	assert.NoError(t, err)
+	assert.Equal(t, "original", string(data))
+}
+
 // TestFilesystemTool_WriteFile_OverwriteExplicitAllowed verifies that setting
 // overwrite=true replaces the existing file.
 func TestFilesystemTool_WriteFile_OverwriteExplicitAllowed(t *testing.T) {