diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index ed2dd2a0..a2798fd4 100644 --- a/pkg/agent/instance.go +++ b/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) diff --git a/pkg/agent/instance_test.go b/pkg/agent/instance_test.go index dff2c0f2..21d65526 100644 --- a/pkg/agent/instance_test.go +++ b/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() diff --git a/pkg/tools/fs/filesystem.go b/pkg/tools/fs/filesystem.go index adc7fa99..a23cc9b2 100644 --- a/pkg/tools/fs/filesystem.go +++ b/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, + ), ) } } diff --git a/pkg/tools/fs/filesystem_test.go b/pkg/tools/fs/filesystem_test.go index 4387332b..356e4e55 100644 --- a/pkg/tools/fs/filesystem_test.go +++ b/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) {