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 <noreply@anthropic.com>
2026-02-12 19:55:57 +08:00
parent 0ac93d4429
commit 35fa64cde8
3 changed files with 316 additions and 2 deletions
--- 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": ""
    },
    {
--- 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 支持可选的目录限制，用于安全控制，防止编辑允许目录外的文件。
+
 ---
--- a/pkg/tools/edit_test.go
+++ 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")
+	}
+}