From 0ac93d4429f8b366cccdd430939d61e8b4515308 Mon Sep 17 00:00:00 2001 From: yinwm Date: Thu, 12 Feb 2026 19:54:44 +0800 Subject: [PATCH] feat: US-014 - Add WebTool tests Added comprehensive test coverage for WebTool (WebSearchTool, WebFetchTool) with 9 test cases: - TestWebTool_WebFetch_Success: Verifies successful URL fetching - TestWebTool_WebFetch_JSON: Verifies JSON content handling - TestWebTool_WebFetch_InvalidURL: Verifies error handling for invalid URLs - TestWebTool_WebFetch_UnsupportedScheme: Verifies only http/https allowed - TestWebTool_WebFetch_MissingURL: Verifies missing URL parameter handling - TestWebTool_WebFetch_Truncation: Verifies content truncation at maxChars - TestWebTool_WebSearch_NoApiKey: Verifies API key requirement - TestWebTool_WebSearch_MissingQuery: Verifies missing query parameter - TestWebTool_WebFetch_HTMLExtraction: Verifies HTML tag removal and text extraction - TestWebTool_WebFetch_MissingDomain: Verifies domain validation WebTool implementation already conforms to ToolResult specification: - WebFetch returns ForUser=fetched content, ForLLM=summary with byte count - WebSearch returns ForUser=search results, ForLLM=result count - Errors return ErrorResult with IsError=true Tests use httptest.NewServer for mock HTTP servers, avoiding external API dependencies. Co-Authored-By: Claude Opus 4.6 --- .ralph/prd.json | 2 +- .ralph/progress.txt | 32 ++++- pkg/tools/web_test.go | 263 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 293 insertions(+), 4 deletions(-) create mode 100644 pkg/tools/web_test.go diff --git a/.ralph/prd.json b/.ralph/prd.json index 5d14876..432ad3b 100644 --- a/.ralph/prd.json +++ b/.ralph/prd.json @@ -212,7 +212,7 @@ "go test ./pkg/tools -run TestWebTool passes" ], "priority": 14, - "passes": false, + "passes": true, "notes": "" }, { diff --git a/.ralph/progress.txt b/.ralph/progress.txt index 2923697..7914f3e 100644 --- a/.ralph/progress.txt +++ b/.ralph/progress.txt @@ -6,12 +6,12 @@ Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改 ## Progress -### Completed (10/21) +### Completed (14/21) - US-001: Add ToolResult struct and helper functions - US-002: Modify Tool interface to return *ToolResult - US-004: Delete isToolConfirmationMessage function (already removed in commit 488e7a9) -- US-005: Update AgentLoop tool result processing logic +- US-005: Update AgentLoop tool result logic - US-006: Add AsyncCallback type and AsyncTool interface - US-007: Heartbeat async task execution support - US-008: Inject callback into async tools in AgentLoop @@ -20,6 +20,9 @@ Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改 - US-011: Refactor MessageTool to use ToolResult - US-012: Refactor ShellTool to use ToolResult - US-013: Refactor FilesystemTool to use ToolResult +- US-014: Refactor WebTool to use ToolResult +- US-012: Refactor ShellTool to use ToolResult +- US-013: Refactor FilesystemTool to use ToolResult ### In Progress @@ -40,7 +43,7 @@ Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改 | 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 | Added test file filesystem_test.go | -| US-014 | Refactor WebTool to use ToolResult | Completed | | +| US-014 | Refactor WebTool to use ToolResult | Completed | Added test file web_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 | | @@ -269,4 +272,27 @@ Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改 - **Gotchas encountered:** 最初误解了 NewToolResult 的行为，以为它会设置 ForUser，但实际上它只设置 ForLLM。 - **Useful context:** WriteFileTool 会自动创建不存在的目录（使用 os.MkdirAll），这是文件写入工具的重要功能。 +--- + +## [2026-02-12] - US-014 +- What was implemented: + - WebTool 已经完全使用 ToolResult 返回值（无需修改实现） + - 添加了完整的测试文件 `pkg/tools/web_test.go`，包含 9 个测试用例 + - 测试覆盖了所有验收标准： + - WebFetchTool 成功返回 ForUser=获取的内容，ForLLM=摘要 + - WebSearchTool 缺少 API Key 返回 ErrorResult + - URL 验证测试（无效 URL、不支持 scheme、缺少域名） + - JSON 内容处理测试 + - HTML 提取和清理测试（移除 script/style 标签） + - 内容截断测试 + - 缺少参数错误处理 + +- Files changed: + - `pkg/tools/web_test.go` (新增) + +- **Learnings for future iterations:** + - **Patterns discovered:** WebTool 使用 httptest.NewServer 创建模拟服务器进行测试，避免依赖外部 API。WebFetchTool 返回 JSON 格式的结构化内容给用户，包含 url、status、extractor、truncated、length、text 字段。 + - **Gotchas encountered:** 无 + - **Useful context:** WebSearchTool 需要配置 BRAVE_API_KEY 环境变量才能正常工作。WebFetchTool 支持多种内容类型：JSON（格式化）、HTML（文本提取）、纯文本。 + --- \ No newline at end of file diff --git a/pkg/tools/web_test.go b/pkg/tools/web_test.go new file mode 100644 index 0000000..30bc7d9 --- /dev/null +++ b/pkg/tools/web_test.go @@ -0,0 +1,263 @@ +package tools + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestWebTool_WebFetch_Success verifies successful URL fetching +func TestWebTool_WebFetch_Success(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html") + w.WriteHeader(http.StatusOK) + w.Write([]byte("

Test Page

Content here

")) + })) + defer server.Close() + + tool := NewWebFetchTool(50000) + ctx := context.Background() + args := map[string]interface{}{ + "url": server.URL, + } + + result := tool.Execute(ctx, args) + + // Success should not be an error + if result.IsError { + t.Errorf("Expected success, got IsError=true: %s", result.ForLLM) + } + + // ForUser should contain the fetched content + if !strings.Contains(result.ForUser, "Test Page") { + t.Errorf("Expected ForUser to contain 'Test Page', got: %s", result.ForUser) + } + + // ForLLM should contain summary + if !strings.Contains(result.ForLLM, "bytes") && !strings.Contains(result.ForLLM, "extractor") { + t.Errorf("Expected ForLLM to contain summary, got: %s", result.ForLLM) + } +} + +// TestWebTool_WebFetch_JSON verifies JSON content handling +func TestWebTool_WebFetch_JSON(t *testing.T) { + testData := map[string]string{"key": "value", "number": "123"} + expectedJSON, _ := json.MarshalIndent(testData, "", " ") + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + w.Write(expectedJSON) + })) + defer server.Close() + + tool := NewWebFetchTool(50000) + ctx := context.Background() + args := map[string]interface{}{ + "url": server.URL, + } + + result := tool.Execute(ctx, args) + + // Success should not be an error + if result.IsError { + t.Errorf("Expected success, got IsError=true: %s", result.ForLLM) + } + + // ForUser should contain formatted JSON + if !strings.Contains(result.ForUser, "key") && !strings.Contains(result.ForUser, "value") { + t.Errorf("Expected ForUser to contain JSON data, got: %s", result.ForUser) + } +} + +// TestWebTool_WebFetch_InvalidURL verifies error handling for invalid URL +func TestWebTool_WebFetch_InvalidURL(t *testing.T) { + tool := NewWebFetchTool(50000) + ctx := context.Background() + args := map[string]interface{}{ + "url": "not-a-valid-url", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error for invalid URL") + } + + // Should contain error message (either "invalid URL" or scheme error) + if !strings.Contains(result.ForLLM, "URL") && !strings.Contains(result.ForUser, "URL") { + t.Errorf("Expected error message for invalid URL, got ForLLM: %s", result.ForLLM) + } +} + +// TestWebTool_WebFetch_UnsupportedScheme verifies error handling for non-http URLs +func TestWebTool_WebFetch_UnsupportedScheme(t *testing.T) { + tool := NewWebFetchTool(50000) + ctx := context.Background() + args := map[string]interface{}{ + "url": "ftp://example.com/file.txt", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error for unsupported URL scheme") + } + + // Should mention only http/https allowed + if !strings.Contains(result.ForLLM, "http/https") && !strings.Contains(result.ForUser, "http/https") { + t.Errorf("Expected scheme error message, got ForLLM: %s", result.ForLLM) + } +} + +// TestWebTool_WebFetch_MissingURL verifies error handling for missing URL +func TestWebTool_WebFetch_MissingURL(t *testing.T) { + tool := NewWebFetchTool(50000) + ctx := context.Background() + args := map[string]interface{}{} + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error when URL is missing") + } + + // Should mention URL is required + if !strings.Contains(result.ForLLM, "url is required") && !strings.Contains(result.ForUser, "url is required") { + t.Errorf("Expected 'url is required' message, got ForLLM: %s", result.ForLLM) + } +} + +// TestWebTool_WebFetch_Truncation verifies content truncation +func TestWebTool_WebFetch_Truncation(t *testing.T) { + longContent := strings.Repeat("x", 20000) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/plain") + w.WriteHeader(http.StatusOK) + w.Write([]byte(longContent)) + })) + defer server.Close() + + tool := NewWebFetchTool(1000) // Limit to 1000 chars + ctx := context.Background() + args := map[string]interface{}{ + "url": server.URL, + } + + result := tool.Execute(ctx, args) + + // Success should not be an error + if result.IsError { + t.Errorf("Expected success, got IsError=true: %s", result.ForLLM) + } + + // ForUser should contain truncated content (not the full 20000 chars) + resultMap := make(map[string]interface{}) + json.Unmarshal([]byte(result.ForUser), &resultMap) + if text, ok := resultMap["text"].(string); ok { + if len(text) > 1100 { // Allow some margin + t.Errorf("Expected content to be truncated to ~1000 chars, got: %d", len(text)) + } + } + + // Should be marked as truncated + if truncated, ok := resultMap["truncated"].(bool); !ok || !truncated { + t.Errorf("Expected 'truncated' to be true in result") + } +} + +// TestWebTool_WebSearch_NoApiKey verifies error handling when API key is missing +func TestWebTool_WebSearch_NoApiKey(t *testing.T) { + tool := NewWebSearchTool("", 5) + ctx := context.Background() + args := map[string]interface{}{ + "query": "test", + } + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error when API key is missing") + } + + // Should mention missing API key + if !strings.Contains(result.ForLLM, "BRAVE_API_KEY") && !strings.Contains(result.ForUser, "BRAVE_API_KEY") { + t.Errorf("Expected API key error message, got ForLLM: %s", result.ForLLM) + } +} + +// TestWebTool_WebSearch_MissingQuery verifies error handling for missing query +func TestWebTool_WebSearch_MissingQuery(t *testing.T) { + tool := NewWebSearchTool("test-key", 5) + ctx := context.Background() + args := map[string]interface{}{} + + result := tool.Execute(ctx, args) + + // Should return error result + if !result.IsError { + t.Errorf("Expected error when query is missing") + } +} + +// TestWebTool_WebFetch_HTMLExtraction verifies HTML text extraction +func TestWebTool_WebFetch_HTMLExtraction(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html") + w.WriteHeader(http.StatusOK) + w.Write([]byte(`

Title

Content

`)) + })) + defer server.Close() + + tool := NewWebFetchTool(50000) + ctx := context.Background() + args := map[string]interface{}{ + "url": server.URL, + } + + result := tool.Execute(ctx, args) + + // Success should not be an error + if result.IsError { + t.Errorf("Expected success, got IsError=true: %s", result.ForLLM) + } + + // ForUser should contain extracted text (without script/style tags) + if !strings.Contains(result.ForUser, "Title") && !strings.Contains(result.ForUser, "Content") { + t.Errorf("Expected ForUser to contain extracted text, got: %s", result.ForUser) + } + + // Should NOT contain script or style tags + if strings.Contains(result.ForUser, "