feat: US-013 - Add FilesystemTool tests

Added comprehensive test coverage for FilesystemTool (ReadFileTool, WriteFileTool, ListDirTool) with 10 test cases: - TestFilesystemTool_ReadFile_Success: Verifies file content goes to ForLLM - TestFilesystemTool_ReadFile_NotFound: Verifies error handling for missing files - TestFilesystemTool_ReadFile_MissingPath: Verifies missing parameter handling - TestFilesystemTool_WriteFile_Success: Verifies SilentResult behavior - TestFilesystemTool_WriteFile_CreateDir: Verifies automatic directory creation - TestFilesystemTool_WriteFile_MissingPath: Verifies missing path parameter handling - TestFilesystemTool_WriteFile_MissingContent: Verifies missing content parameter handling - TestFilesystemTool_ListDir_Success: Verifies directory listing functionality - TestFilesystemTool_ListDir_NotFound: Verifies error handling for invalid paths - TestFilesystemTool_ListDir_DefaultPath: Verifies default to current directory FilesystemTool implementation already conforms to ToolResult specification: - ReadFile/ListDir return NewToolResult (ForLLM only, ForUser empty) - WriteFile returns SilentResult (Silent=true, ForUser empty) - Errors return ErrorResult with IsError=true Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-02-12 19:53:00 +08:00
parent e7e3f95ebe
commit 88014ecaff
3 changed files with 275 additions and 2 deletions
--- a/.ralph/prd.json
+++ b/.ralph/prd.json
@@ -197,7 +197,7 @@
        "go test ./pkg/tools -run TestFilesystemTool passes"
      ],
      "priority": 13,
-      "passes": false,
+      "passes": true,
      "notes": ""
    },
    {
--- a/.ralph/progress.txt
+++ b/.ralph/progress.txt
@@ -19,6 +19,7 @@ Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改
 - US-010: Update RecordLastChannel to use atomic save
 - US-011: Refactor MessageTool to use ToolResult
 - US-012: Refactor ShellTool to use ToolResult
+- US-013: Refactor FilesystemTool to use ToolResult

 ### In Progress

@@ -38,7 +39,7 @@ Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改
 | US-010 | Update RecordLastChannel to use atomic save | Completed | |
 | US-011 | Refactor MessageTool to use ToolResult | Completed | Added test file message_test.go |
 | US-012 | Refactor ShellTool to use ToolResult | Completed | |
-| US-013 | Refactor FilesystemTool 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 | |
 | US-015 | Refactor EditTool to use ToolResult | Completed | |
 | US-016 | Refactor CronTool to use ToolResult | Pending | |
@@ -245,4 +246,27 @@ Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改
  - **Gotchas encountered:** 无
  - **Useful context:** ShellTool 的安全防护机制包括：危险命令检测（如 `rm -rf`）、工作空间路径遍历检测、命令超时控制。

+---
+
+## [2026-02-12] - US-013
+- What was implemented:
+  - FilesystemTool 已经完全使用 ToolResult 返回值（无需修改实现）
+  - 添加了完整的测试文件 `pkg/tools/filesystem_test.go`，包含 10 个测试用例
+  - 测试覆盖了所有验收标准：
+    - ReadFileTool 成功返回 NewToolResult（ForLLM=内容，ForUser=空）
+    - WriteFileTool 成功返回 SilentResult（Silent=true，ForUser=空）
+    - ListDirTool 成功返回 NewToolResult（列出文件和目录）
+    - 错误场景返回 ErrorResult（IsError=true）
+    - 缺少参数的错误处理
+    - 目录自动创建功能测试
+    - 默认路径处理测试
+
+- Files changed:
+  - `pkg/tools/filesystem_test.go` (新增)
+
+- **Learnings for future iterations:**
+  - **Patterns discovered:** FilesystemTool 中不同操作使用不同的返回值模式：ReadFile/ListDir 使用 NewToolResult（仅设置 ForLLM），WriteFile 使用 SilentResult（设置 Silent=true）。
+  - **Gotchas encountered:** 最初误解了 NewToolResult 的行为，以为它会设置 ForUser，但实际上它只设置 ForLLM。
+  - **Useful context:** WriteFileTool 会自动创建不存在的目录（使用 os.MkdirAll），这是文件写入工具的重要功能。
+
 ---
--- a/pkg/tools/filesystem_test.go
+++ b/pkg/tools/filesystem_test.go
@@ -0,0 +1,249 @@
+package tools
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestFilesystemTool_ReadFile_Success verifies successful file reading
+func TestFilesystemTool_ReadFile_Success(t *testing.T) {
+	tmpDir := t.TempDir()
+	testFile := filepath.Join(tmpDir, "test.txt")
+	os.WriteFile(testFile, []byte("test content"), 0644)
+
+	tool := &ReadFileTool{}
+	ctx := context.Background()
+	args := map[string]interface{}{
+		"path": testFile,
+	}
+
+	result := tool.Execute(ctx, args)
+
+	// Success should not be an error
+	if result.IsError {
+		t.Errorf("Expected success, got IsError=true: %s", result.ForLLM)
+	}
+
+	// ForLLM should contain file content
+	if !strings.Contains(result.ForLLM, "test content") {
+		t.Errorf("Expected ForLLM to contain 'test content', got: %s", result.ForLLM)
+	}
+
+	// ReadFile returns NewToolResult which only sets ForLLM, not ForUser
+	// This is the expected behavior - file content goes to LLM, not directly to user
+	if result.ForUser != "" {
+		t.Errorf("Expected ForUser to be empty for NewToolResult, got: %s", result.ForUser)
+	}
+}
+
+// TestFilesystemTool_ReadFile_NotFound verifies error handling for missing file
+func TestFilesystemTool_ReadFile_NotFound(t *testing.T) {
+	tool := &ReadFileTool{}
+	ctx := context.Background()
+	args := map[string]interface{}{
+		"path": "/nonexistent_file_12345.txt",
+	}
+
+	result := tool.Execute(ctx, args)
+
+	// Failure should be marked as error
+	if !result.IsError {
+		t.Errorf("Expected error for missing file, got IsError=false")
+	}
+
+	// Should contain error message
+	if !strings.Contains(result.ForLLM, "failed to read") && !strings.Contains(result.ForUser, "failed to read") {
+		t.Errorf("Expected error message, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser)
+	}
+}
+
+// TestFilesystemTool_ReadFile_MissingPath verifies error handling for missing path
+func TestFilesystemTool_ReadFile_MissingPath(t *testing.T) {
+	tool := &ReadFileTool{}
+	ctx := context.Background()
+	args := map[string]interface{}{}
+
+	result := tool.Execute(ctx, args)
+
+	// Should return error result
+	if !result.IsError {
+		t.Errorf("Expected error when path is missing")
+	}
+
+	// Should mention required parameter
+	if !strings.Contains(result.ForLLM, "path is required") && !strings.Contains(result.ForUser, "path is required") {
+		t.Errorf("Expected 'path is required' message, got ForLLM: %s", result.ForLLM)
+	}
+}
+
+// TestFilesystemTool_WriteFile_Success verifies successful file writing
+func TestFilesystemTool_WriteFile_Success(t *testing.T) {
+	tmpDir := t.TempDir()
+	testFile := filepath.Join(tmpDir, "newfile.txt")
+
+	tool := &WriteFileTool{}
+	ctx := context.Background()
+	args := map[string]interface{}{
+		"path":    testFile,
+		"content": "hello world",
+	}
+
+	result := tool.Execute(ctx, args)
+
+	// Success should not be an error
+	if result.IsError {
+		t.Errorf("Expected success, got IsError=true: %s", result.ForLLM)
+	}
+
+	// WriteFile returns SilentResult
+	if !result.Silent {
+		t.Errorf("Expected Silent=true for WriteFile, 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 written
+	content, err := os.ReadFile(testFile)
+	if err != nil {
+		t.Fatalf("Failed to read written file: %v", err)
+	}
+	if string(content) != "hello world" {
+		t.Errorf("Expected file content 'hello world', got: %s", string(content))
+	}
+}
+
+// TestFilesystemTool_WriteFile_CreateDir verifies directory creation
+func TestFilesystemTool_WriteFile_CreateDir(t *testing.T) {
+	tmpDir := t.TempDir()
+	testFile := filepath.Join(tmpDir, "subdir", "newfile.txt")
+
+	tool := &WriteFileTool{}
+	ctx := context.Background()
+	args := map[string]interface{}{
+		"path":    testFile,
+		"content": "test",
+	}
+
+	result := tool.Execute(ctx, args)
+
+	// Success should not be an error
+	if result.IsError {
+		t.Errorf("Expected success with directory creation, got IsError=true: %s", result.ForLLM)
+	}
+
+	// Verify directory was created and file written
+	content, err := os.ReadFile(testFile)
+	if err != nil {
+		t.Fatalf("Failed to read written file: %v", err)
+	}
+	if string(content) != "test" {
+		t.Errorf("Expected file content 'test', got: %s", string(content))
+	}
+}
+
+// TestFilesystemTool_WriteFile_MissingPath verifies error handling for missing path
+func TestFilesystemTool_WriteFile_MissingPath(t *testing.T) {
+	tool := &WriteFileTool{}
+	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")
+	}
+}
+
+// TestFilesystemTool_WriteFile_MissingContent verifies error handling for missing content
+func TestFilesystemTool_WriteFile_MissingContent(t *testing.T) {
+	tool := &WriteFileTool{}
+	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")
+	}
+
+	// Should mention required parameter
+	if !strings.Contains(result.ForLLM, "content is required") && !strings.Contains(result.ForUser, "content is required") {
+		t.Errorf("Expected 'content is required' message, got ForLLM: %s", result.ForLLM)
+	}
+}
+
+// TestFilesystemTool_ListDir_Success verifies successful directory listing
+func TestFilesystemTool_ListDir_Success(t *testing.T) {
+	tmpDir := t.TempDir()
+	os.WriteFile(filepath.Join(tmpDir, "file1.txt"), []byte("content"), 0644)
+	os.WriteFile(filepath.Join(tmpDir, "file2.txt"), []byte("content"), 0644)
+	os.Mkdir(filepath.Join(tmpDir, "subdir"), 0755)
+
+	tool := &ListDirTool{}
+	ctx := context.Background()
+	args := map[string]interface{}{
+		"path": tmpDir,
+	}
+
+	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 list files and directories
+	if !strings.Contains(result.ForLLM, "file1.txt") || !strings.Contains(result.ForLLM, "file2.txt") {
+		t.Errorf("Expected files in listing, got: %s", result.ForLLM)
+	}
+	if !strings.Contains(result.ForLLM, "subdir") {
+		t.Errorf("Expected subdir in listing, got: %s", result.ForLLM)
+	}
+}
+
+// TestFilesystemTool_ListDir_NotFound verifies error handling for non-existent directory
+func TestFilesystemTool_ListDir_NotFound(t *testing.T) {
+	tool := &ListDirTool{}
+	ctx := context.Background()
+	args := map[string]interface{}{
+		"path": "/nonexistent_directory_12345",
+	}
+
+	result := tool.Execute(ctx, args)
+
+	// Failure should be marked as error
+	if !result.IsError {
+		t.Errorf("Expected error for non-existent directory, got IsError=false")
+	}
+
+	// Should contain error message
+	if !strings.Contains(result.ForLLM, "failed to read") && !strings.Contains(result.ForUser, "failed to read") {
+		t.Errorf("Expected error message, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser)
+	}
+}
+
+// TestFilesystemTool_ListDir_DefaultPath verifies default to current directory
+func TestFilesystemTool_ListDir_DefaultPath(t *testing.T) {
+	tool := &ListDirTool{}
+	ctx := context.Background()
+	args := map[string]interface{}{}
+
+	result := tool.Execute(ctx, args)
+
+	// Should use "." as default path
+	if result.IsError {
+		t.Errorf("Expected success with default path '.', got IsError=true: %s", result.ForLLM)
+	}
+}