From 35fa64cde83de37489c475fd49e06d49b50d8c88 Mon Sep 17 00:00:00 2001 From: yinwm Date: Thu, 12 Feb 2026 19:55:57 +0800 Subject: [PATCH] feat: US-015 - Add EditTool tests Added comprehensive test coverage for EditTool (EditFileTool, AppendFileTool) with 10 test cases: - TestEditTool_EditFile_Success: Verifies successful file editing - TestEditTool_EditFile_NotFound: Verifies error handling for non-existent files - TestEditTool_EditFile_OldTextNotFound: Verifies error when old_text not found - TestEditTool_EditFile_MultipleMatches: Verifies error for multiple occurrences - TestEditTool_EditFile_OutsideAllowedDir: Verifies directory restriction - TestEditTool_EditFile_MissingPath: Verifies missing path parameter - TestEditTool_EditFile_MissingOldText: Verifies missing old_text parameter - TestEditTool_EditFile_MissingNewText: Verifies missing new_text parameter - TestEditTool_AppendFile_Success: Verifies successful file appending - TestEditTool_AppendFile_MissingPath: Verifies missing path for append - TestEditTool_AppendFile_MissingContent: Verifies missing content for append EditTool implementation already conforms to ToolResult specification: - EditFile returns SilentResult('File edited: ...') - AppendFile returns SilentResult('Appended to ...') - Errors return ErrorResult with IsError=true EditFileTool includes security feature: optional directory restriction to prevent editing files outside allowed paths. Co-Authored-By: Claude Opus 4.6 --- .ralph/prd.json | 2 +- .ralph/progress.txt | 27 +++- pkg/tools/edit_test.go | 289 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 316 insertions(+), 2 deletions(-) create mode 100644 pkg/tools/edit_test.go diff --git a/.ralph/prd.json b/.ralph/prd.json index 432ad3b..83ba072 100644 --- a/.ralph/prd.json +++ b/.ralph/prd.json @@ -226,7 +226,7 @@ "go test ./pkg/tools -run TestEditTool passes" ], "priority": 15, - "passes": false, + "passes": true, "notes": "" }, { diff --git a/.ralph/progress.txt b/.ralph/progress.txt index 7914f3e..5549ea0 100644 --- a/.ralph/progress.txt +++ b/.ralph/progress.txt @@ -6,7 +6,7 @@ Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改 ## Progress -### Completed (14/21) +### Completed (15/21) - US-001: Add ToolResult struct and helper functions - US-002: Modify Tool interface to return *ToolResult @@ -21,6 +21,7 @@ Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改 - US-012: Refactor ShellTool to use ToolResult - US-013: Refactor FilesystemTool to use ToolResult - US-014: Refactor WebTool to use ToolResult +- US-015: Refactor EditTool to use ToolResult - US-012: Refactor ShellTool to use ToolResult - US-013: Refactor FilesystemTool to use ToolResult @@ -44,6 +45,7 @@ Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改 | US-012 | Refactor ShellTool to use ToolResult | Completed | | | US-013 | Refactor FilesystemTool to use ToolResult | Completed | Added test file filesystem_test.go | | US-014 | Refactor WebTool to use ToolResult | Completed | Added test file web_test.go | +| US-015 | Refactor EditTool to use ToolResult | Completed | Added test file edit_test.go | | US-015 | Refactor EditTool to use ToolResult | Completed | | | US-016 | Refactor CronTool to use ToolResult | Pending | | | US-017 | Refactor SpawnTool to use AsyncTool and callbacks | Pending | | @@ -295,4 +297,27 @@ Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改 - **Gotchas encountered:** 无 - **Useful context:** WebSearchTool 需要配置 BRAVE_API_KEY 环境变量才能正常工作。WebFetchTool 支持多种内容类型：JSON（格式化）、HTML（文本提取）、纯文本。 +--- + +## [2026-02-12] - US-015 +- What was implemented: + - EditTool 已经完全使用 ToolResult 返回值（无需修改实现） + - 添加了完整的测试文件 `pkg/tools/edit_test.go`，包含 10 个测试用例 + - 测试覆盖了所有验收标准： + - EditFileTool 成功返回 SilentResult（Silent=true，ForUser=空） + - AppendFileTool 成功返回 SilentResult + - 文件不存在错误处理 + - old_text 不存在错误处理 + - 多次匹配错误处理 + - 目录限制测试 + - 缺少参数错误处理 + +- Files changed: + - `pkg/tools/edit_test.go` (新增) + +- **Learnings for future iterations:** + - **Patterns discovered:** EditTool 包含两个子工具：EditFileTool（替换文本）和 AppendFileTool（追加内容）。都返回 SilentResult 避免重复发送内容给用户。 + - **Gotchas encountered:** 无 + - **Useful context:** EditFileTool 支持可选的目录限制，用于安全控制，防止编辑允许目录外的文件。 + --- \ No newline at end of file diff --git a/pkg/tools/edit_test.go b/pkg/tools/edit_test.go new file mode 100644 index 0000000..aeda005 --- /dev/null +++ b/pkg/tools/edit_test.go @@ -0,0 +1,289 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestEditTool_EditFile_Success verifies successful file editing +func TestEditTool_EditFile_Success(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "test.txt") + os.WriteFile(testFile, []byte("Hello World\nThis is a test"), 0644) + + tool := NewEditFileTool(tmpDir) + ctx := context.Background() + args := map[string]interface{}{ + "path": testFile, + "old_text": "World", + "new_text": "Universe", + } + + result := tool.Execute(ctx, args) + + // Success should not be an error + if result.IsError { + t.Errorf("Expected success, got IsError=true: %s", result.ForLLM) + } + + // Should return SilentResult + if !result.Silent { + t.Errorf("Expected Silent=true for EditFile, got false") + } + + // ForUser should be empty (silent result) + if result.ForUser != "" { + t.Errorf("Expected ForUser to be empty for SilentResult, got: %s", result.ForUser) + } + + // Verify file was actually edited + content, err := os.ReadFile(testFile) + if err != nil { + t.Fatalf("Failed to read edited file: %v", err) + } + contentStr := string(content) + if !strings.Contains(contentStr, "Hello Universe") { + t.Errorf("Expected file to contain 'Hello Universe', got: %s", contentStr) + } + if strings.Contains(contentStr, "Hello World") { + t.Errorf("Expected 'Hello World' to be replaced, got: %s", contentStr) + } +} + +// TestEditTool_EditFile_NotFound verifies error handling for non-existent file +func TestEditTool_EditFile_NotFound(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "nonexistent.txt") + + tool := NewEditFileTool(tmpDir) + ctx := context.Background() + args := map[string]interface{}{ + "path": testFile, + "old_text": "old", + "new_text": "new", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error for non-existent file") + } + + // Should mention file not found + if !strings.Contains(result.ForLLM, "not found") && !strings.Contains(result.ForUser, "not found") { + t.Errorf("Expected 'file not found' message, got ForLLM: %s", result.ForLLM) + } +} + +// TestEditTool_EditFile_OldTextNotFound verifies error when old_text doesn't exist +func TestEditTool_EditFile_OldTextNotFound(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "test.txt") + os.WriteFile(testFile, []byte("Hello World"), 0644) + + tool := NewEditFileTool(tmpDir) + ctx := context.Background() + args := map[string]interface{}{ + "path": testFile, + "old_text": "Goodbye", + "new_text": "Hello", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error when old_text not found") + } + + // Should mention old_text not found + if !strings.Contains(result.ForLLM, "not found") && !strings.Contains(result.ForUser, "not found") { + t.Errorf("Expected 'not found' message, got ForLLM: %s", result.ForLLM) + } +} + +// TestEditTool_EditFile_MultipleMatches verifies error when old_text appears multiple times +func TestEditTool_EditFile_MultipleMatches(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "test.txt") + os.WriteFile(testFile, []byte("test test test"), 0644) + + tool := NewEditFileTool(tmpDir) + ctx := context.Background() + args := map[string]interface{}{ + "path": testFile, + "old_text": "test", + "new_text": "done", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error when old_text appears multiple times") + } + + // Should mention multiple occurrences + if !strings.Contains(result.ForLLM, "times") && !strings.Contains(result.ForUser, "times") { + t.Errorf("Expected 'multiple times' message, got ForLLM: %s", result.ForLLM) + } +} + +// TestEditTool_EditFile_OutsideAllowedDir verifies error when path is outside allowed directory +func TestEditTool_EditFile_OutsideAllowedDir(t *testing.T) { + tmpDir := t.TempDir() + otherDir := t.TempDir() + testFile := filepath.Join(otherDir, "test.txt") + os.WriteFile(testFile, []byte("content"), 0644) + + tool := NewEditFileTool(tmpDir) // Restrict to tmpDir + ctx := context.Background() + args := map[string]interface{}{ + "path": testFile, + "old_text": "content", + "new_text": "new", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error when path is outside allowed directory") + } + + // Should mention outside allowed directory + if !strings.Contains(result.ForLLM, "outside") && !strings.Contains(result.ForUser, "outside") { + t.Errorf("Expected 'outside allowed' message, got ForLLM: %s", result.ForLLM) + } +} + +// TestEditTool_EditFile_MissingPath verifies error handling for missing path +func TestEditTool_EditFile_MissingPath(t *testing.T) { + tool := NewEditFileTool("") + ctx := context.Background() + args := map[string]interface{}{ + "old_text": "old", + "new_text": "new", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error when path is missing") + } +} + +// TestEditTool_EditFile_MissingOldText verifies error handling for missing old_text +func TestEditTool_EditFile_MissingOldText(t *testing.T) { + tool := NewEditFileTool("") + ctx := context.Background() + args := map[string]interface{}{ + "path": "/tmp/test.txt", + "new_text": "new", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error when old_text is missing") + } +} + +// TestEditTool_EditFile_MissingNewText verifies error handling for missing new_text +func TestEditTool_EditFile_MissingNewText(t *testing.T) { + tool := NewEditFileTool("") + ctx := context.Background() + args := map[string]interface{}{ + "path": "/tmp/test.txt", + "old_text": "old", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error when new_text is missing") + } +} + +// TestEditTool_AppendFile_Success verifies successful file appending +func TestEditTool_AppendFile_Success(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "test.txt") + os.WriteFile(testFile, []byte("Initial content"), 0644) + + tool := NewAppendFileTool() + ctx := context.Background() + args := map[string]interface{}{ + "path": testFile, + "content": "\nAppended content", + } + + result := tool.Execute(ctx, args) + + // Success should not be an error + if result.IsError { + t.Errorf("Expected success, got IsError=true: %s", result.ForLLM) + } + + // Should return SilentResult + if !result.Silent { + t.Errorf("Expected Silent=true for AppendFile, got false") + } + + // ForUser should be empty (silent result) + if result.ForUser != "" { + t.Errorf("Expected ForUser to be empty for SilentResult, got: %s", result.ForUser) + } + + // Verify content was actually appended + content, err := os.ReadFile(testFile) + if err != nil { + t.Fatalf("Failed to read file: %v", err) + } + contentStr := string(content) + if !strings.Contains(contentStr, "Initial content") { + t.Errorf("Expected original content to remain, got: %s", contentStr) + } + if !strings.Contains(contentStr, "Appended content") { + t.Errorf("Expected appended content, got: %s", contentStr) + } +} + +// TestEditTool_AppendFile_MissingPath verifies error handling for missing path +func TestEditTool_AppendFile_MissingPath(t *testing.T) { + tool := NewAppendFileTool() + ctx := context.Background() + args := map[string]interface{}{ + "content": "test", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error when path is missing") + } +} + +// TestEditTool_AppendFile_MissingContent verifies error handling for missing content +func TestEditTool_AppendFile_MissingContent(t *testing.T) { + tool := NewAppendFileTool() + ctx := context.Background() + args := map[string]interface{}{ + "path": "/tmp/test.txt", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error when content is missing") + } +}