diff --git a/.gitignore b/.gitignore index 6ad4d78..ad9ced4 100644 --- a/.gitignore +++ b/.gitignore @@ -24,3 +24,4 @@ coverage.html # Ralph workspace ralph/ +.ralph/ \ No newline at end of file diff --git a/.ralph/prd.json b/.ralph/prd.json deleted file mode 100644 index 9f58016..0000000 --- a/.ralph/prd.json +++ /dev/null @@ -1,320 +0,0 @@ -{ - "project": "picoclaw", - "branchName": "ralph/tool-result-refactor", - "description": "Tool 返回值结构化重构 - 将 Tool 接口返回值从 (string, error) 改为结构化 ToolResult，支持异步任务，删除字符串匹配黑魔法", - "userStories": [ - { - "id": "US-001", - "title": "Add ToolResult struct and helper functions", - "description": "As a developer, I need ToolResult struct with helper functions so tools can express result semantics clearly.", - "acceptanceCriteria": [ - "ToolResult has fields: ForLLM, ForUser, Silent, IsError, Async, Err", - "Helper functions: NewToolResult(), SilentResult(), AsyncResult(), ErrorResult(), UserResult()", - "ToolResult supports JSON serialization (except Err field)", - "Complete godoc comments added", - "Typecheck passes", - "go test ./pkg/tools -run TestToolResult passes" - ], - "priority": 1, - "passes": true, - "notes": "" - }, - { - "id": "US-002", - "title": "Modify Tool interface to return *ToolResult", - "description": "As a developer, I need the Tool interface Execute method to return *ToolResult so tools use new structured return values.", - "acceptanceCriteria": [ - "pkg/tools/base.go Tool.Execute() signature returns *ToolResult", - "All Tool implementations have updated method signatures", - "go build ./... succeeds without errors", - "go vet ./... passes" - ], - "priority": 2, - "passes": true, - "notes": "" - }, - { - "id": "US-003", - "title": "Modify ToolRegistry to process ToolResult", - "description": "As the middleware layer, ToolRegistry needs to handle ToolResult return values and adjust logging for async task status.", - "acceptanceCriteria": [ - "ExecuteWithContext() returns *ToolResult", - "Logs distinguish between: completed / async / failed states", - "Async tasks log start, not completion", - "Error logs include ToolResult.Err content", - "Typecheck passes", - "go test ./pkg/tools -run TestRegistry passes" - ], - "priority": 3, - "passes": true, - "notes": "" - }, - { - "id": "US-004", - "title": "Delete isToolConfirmationMessage function", - "description": "As a code maintainer, I need to remove the isToolConfirmationMessage function since ToolResult.Silent solves this problem.", - "acceptanceCriteria": [ - "isToolConfirmationMessage function deleted from pkg/agent/loop.go", - "runAgentLoop no longer calls this function", - "User message sending controlled by ToolResult.Silent field", - "Typecheck passes", - "go build ./... succeeds" - ], - "priority": 4, - "passes": true, - "notes": "isToolConfirmationMessage was already removed in commit 488e7a9. US-005 will complete the migration to ToolResult.Silent." - }, - { - "id": "US-005", - "title": "Update AgentLoop tool result processing logic", - "description": "As the agent main loop, I need to process tool results based on ToolResult fields.", - "acceptanceCriteria": [ - "LLM receives message content from ToolResult.ForLLM", - "User messages prefer ToolResult.ForUser, fallback to LLM final response", - "ToolResult.Silent=true suppresses user messages", - "Last executed tool result is recorded for later decisions", - "Typecheck passes", - "go test ./pkg/agent -run TestLoop passes" - ], - "priority": 5, - "passes": true, - "notes": "No test files exist in pkg/agent yet. All other acceptance criteria met." - }, - { - "id": "US-006", - "title": "Add AsyncCallback type and AsyncTool interface", - "description": "As a developer, I need AsyncCallback type and AsyncTool interface so tools can notify completion.", - "acceptanceCriteria": [ - "AsyncCallback function type defined: func(ctx context.Context, result *ToolResult)", - "AsyncTool interface defined with SetCallback(cb AsyncCallback) method", - "Complete godoc comments", - "Typecheck passes" - ], - "priority": 6, - "passes": true, - "notes": "" - }, - { - "id": "US-007", - "title": "Heartbeat async task execution support", - "description": "As the heartbeat service, I need to trigger async tasks and return immediately without blocking the timer.", - "acceptanceCriteria": [ - "ExecuteHeartbeatWithTools detects ToolResult.Async flag", - "Async task returns 'Task started in background' to LLM", - "Async tasks do not block heartbeat flow", - "Duplicate ProcessHeartbeat function deleted", - "Typecheck passes", - "go test ./pkg/heartbeat -run TestAsync passes" - ], - "priority": 7, - "passes": true, - "notes": "" - }, - { - "id": "US-008", - "title": "Inject callback into async tools in AgentLoop", - "description": "As the agent loop, I need to inject callback functions into async tools so they can notify completion.", - "acceptanceCriteria": [ - "AgentLoop defines callback function for async tool results", - "Callback uses SendToChannel to send results to user", - "Tools implementing AsyncTool receive callback via ExecuteWithContext", - "Typecheck passes" - ], - "priority": 8, - "passes": true, - "notes": "" - }, - { - "id": "US-009", - "title": "State save atomicity - SetLastChannel", - "description": "As state management, I need atomic state update and save to prevent data loss on crash.", - "acceptanceCriteria": [ - "SetLastChannel merges save logic, accepts workspace parameter", - "Uses temp file + rename for atomic write", - "Cleanup temp file if rename fails", - "Timestamp updated within lock", - "Typecheck passes", - "go test ./pkg/state -run TestAtomicSave passes" - ], - "priority": 9, - "passes": true, - "notes": "" - }, - { - "id": "US-010", - "title": "Update RecordLastChannel to use atomic save", - "description": "As AgentLoop, I need to call the new atomic state save method.", - "acceptanceCriteria": [ - "RecordLastChannel calls st.SetLastChannel(al.workspace, lastChannel)", - "Call includes workspace path parameter", - "Typecheck passes", - "go test ./pkg/agent -run TestRecordLastChannel passes" - ], - "priority": 10, - "passes": true, - "notes": "" - }, - { - "id": "US-011", - "title": "Refactor MessageTool to use ToolResult", - "description": "As the message sending tool, I need to use new ToolResult return values, silently confirming successful sends.", - "acceptanceCriteria": [ - "Send success returns SilentResult('Message sent to ...')", - "Send failure returns ErrorResult(...)", - "ForLLM contains send status description", - "ForUser is empty (user already received message directly)", - "Typecheck passes", - "go test ./pkg/tools -run TestMessageTool passes" - ], - "priority": 11, - "passes": true, - "notes": "" - }, - { - "id": "US-012", - "title": "Refactor ShellTool to use ToolResult", - "description": "As the shell command tool, I need to send command results to the user and show errors on failure.", - "acceptanceCriteria": [ - "Success returns ToolResult with ForUser = command output", - "Failure returns ToolResult with IsError = true", - "ForLLM contains full output and exit code", - "Typecheck passes", - "go test ./pkg/tools -run TestShellTool passes" - ], - "priority": 12, - "passes": true, - "notes": "" - }, - { - "id": "US-013", - "title": "Refactor FilesystemTool to use ToolResult", - "description": "As the file operation tool, I need to complete file reads/writes silently without sending confirm messages.", - "acceptanceCriteria": [ - "All file operations return SilentResult(...)", - "Errors return ErrorResult(...)", - "ForLLM contains operation summary (e.g., 'File updated: /path/to/file')", - "Typecheck passes", - "go test ./pkg/tools -run TestFilesystemTool passes" - ], - "priority": 13, - "passes": true, - "notes": "" - }, - { - "id": "US-014", - "title": "Refactor WebTool to use ToolResult", - "description": "As the web request tool, I need to send fetched content to the user for review.", - "acceptanceCriteria": [ - "Success returns ForUser containing fetched content", - "ForLLM contains content summary and byte count", - "Failure returns ErrorResult", - "Typecheck passes", - "go test ./pkg/tools -run TestWebTool passes" - ], - "priority": 14, - "passes": true, - "notes": "" - }, - { - "id": "US-015", - "title": "Refactor EditTool to use ToolResult", - "description": "As the file editing tool, I need to complete edits silently to avoid duplicate content sent to user.", - "acceptanceCriteria": [ - "Edit success returns SilentResult('File edited: ...')", - "ForLLM contains edit summary", - "Typecheck passes", - "go test ./pkg/tools -run TestEditTool passes" - ], - "priority": 15, - "passes": true, - "notes": "" - }, - { - "id": "US-016", - "title": "Refactor CronTool to use ToolResult", - "description": "As the cron task tool, I need to complete cron operations silently without sending confirmation messages.", - "acceptanceCriteria": [ - "All cron operations return SilentResult(...)", - "ForLLM contains operation summary (e.g., 'Cron job added: daily-backup')", - "Typecheck passes", - "go test ./pkg/tools -run TestCronTool passes" - ], - "priority": 16, - "passes": true, - "notes": "" - }, - { - "id": "US-017", - "title": "Refactor SpawnTool to use AsyncTool and callbacks", - "description": "As the subagent spawn tool, I need to mark as async task and notify on completion via callback.", - "acceptanceCriteria": [ - "Implements AsyncTool interface", - "Returns AsyncResult('Subagent spawned, will report back')", - "Subagent completion calls callback to send result", - "Typecheck passes", - "go test ./pkg/tools -run TestSpawnTool passes" - ], - "priority": 17, - "passes": true, - "notes": "" - }, - { - "id": "US-018", - "title": "Refactor SubagentTool to use ToolResult", - "description": "As the subagent tool, I need to send subagent execution summary to the user.", - "acceptanceCriteria": [ - "ForUser contains subagent output summary", - "ForLLM contains full execution details", - "Typecheck passes", - "go test ./pkg/tools -run TestSubagentTool passes" - ], - "priority": 18, - "passes": true, - "notes": "" - }, - { - "id": "US-019", - "title": "Enable heartbeat by default in config", - "description": "As system config, heartbeat should be enabled by default as it is a core feature.", - "acceptanceCriteria": [ - "DefaultConfig() Heartbeat.Enabled changed to true", - "Can override via PICOCLAW_HEARTBEAT_ENABLED=false env var", - "Config documentation updated showing default enabled", - "Typecheck passes", - "go test ./pkg/config -run TestDefaultConfig passes" - ], - "priority": 19, - "passes": true, - "notes": "" - }, - { - "id": "US-020", - "title": "Move heartbeat log to memory directory", - "description": "As heartbeat service, logs should go to memory directory for LLM access and knowledge system integration.", - "acceptanceCriteria": [ - "Log path changed from workspace/heartbeat.log to workspace/memory/heartbeat.log", - "Directory auto-created if missing", - "Log format unchanged", - "Typecheck passes", - "go test ./pkg/heartbeat -run TestLogPath passes" - ], - "priority": 20, - "passes": true, - "notes": "" - }, - { - "id": "US-021", - "title": "Heartbeat calls ExecuteHeartbeatWithTools", - "description": "As heartbeat service, I need to call the tool-supporting execution method.", - "acceptanceCriteria": [ - "executeHeartbeat calls handler.ExecuteHeartbeatWithTools(...)", - "Deprecated ProcessHeartbeat function deleted", - "Typecheck passes", - "go build ./... succeeds" - ], - "priority": 21, - "passes": true, - "notes": "Already implemented" - } - ] -} diff --git a/.ralph/progress.txt b/.ralph/progress.txt deleted file mode 100644 index 336aa06..0000000 --- a/.ralph/progress.txt +++ /dev/null @@ -1,44 +0,0 @@ -### Completed (21/21) - -All user stories completed! - -## Summary - -Tool 返回值结构化重构项目（tool-result-refactor）已全部完成： -- 21 个用户故事全部实现 -- 从 US-001 (ToolResult struct) 到 US-021 (Heartbeat ExecuteHeartbeatWithTools) -- 所有验收标准均已满足 -- 代码通过 typecheck 和测试验证 - -### User Stories Completed - -- US-001: Add ToolResult struct and helper functions -- US-002: Modify Tool interface to return *ToolResult -- US-003: Modify ToolRegistry to process ToolResult -- US-004: Delete isToolConfirmationMessage function -- US-005: Update AgentLoop tool result processing 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 -- US-009: State save atomicity - SetLastChannel -- 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 -- US-014: Refactor WebTool to use ToolResult -- US-015: Refactor EditTool to use ToolResult -- US-016: Refactor CronTool to use ToolResult -- US-017: Refactor SpawnTool to use AsyncTool and callbacks -- US-018: Refactor SubagentTool to use ToolResult -- US-019: Enable heartbeat by default in config -- US-020: Move heartbeat log to memory directory -- US-021: Heartbeat calls ExecuteHeartbeatWithTools - -### In Progress -None - -### Blocked -None - -### Pending -None diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index b4eb871..366a0ce 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -158,13 +158,13 @@ func (al *AgentLoop) RegisterTool(tool tools.Tool) { // RecordLastChannel records the last active channel for this workspace. // This uses the atomic state save mechanism to prevent data loss on crash. func (al *AgentLoop) RecordLastChannel(channel string) error { - return al.state.SetLastChannel(al.workspace, channel) + return al.state.SetLastChannel(channel) } // RecordLastChatID records the last active chat ID for this workspace. // This uses the atomic state save mechanism to prevent data loss on crash. func (al *AgentLoop) RecordLastChatID(chatID string) error { - return al.state.SetLastChatID(al.workspace, chatID) + return al.state.SetLastChatID(chatID) } func (al *AgentLoop) ProcessDirect(ctx context.Context, content, sessionKey string) (string, error) { @@ -271,14 +271,13 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, opts processOptions) (str al.sessions.AddMessage(opts.SessionKey, "user", opts.UserMessage) // 4. Run LLM iteration loop - finalContent, iteration, lastToolResult, err := al.runLLMIteration(ctx, messages, opts) + finalContent, iteration, err := al.runLLMIteration(ctx, messages, opts) if err != nil { return "", err } // If last tool had ForUser content and we already sent it, we might not need to send final response // This is controlled by the tool's Silent flag and ForUser content - _ = lastToolResult // Use lastToolResult for future decisions (e.g., US-008 callback injection) // 5. Handle empty response if finalContent == "" { @@ -316,11 +315,10 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, opts processOptions) (str } // runLLMIteration executes the LLM call loop with tool handling. -// Returns the final content, iteration count, last tool result, and any error. -func (al *AgentLoop) runLLMIteration(ctx context.Context, messages []providers.Message, opts processOptions) (string, int, *tools.ToolResult, error) { +// Returns the final content, iteration count, and any error. +func (al *AgentLoop) runLLMIteration(ctx context.Context, messages []providers.Message, opts processOptions) (string, int, error) { iteration := 0 var finalContent string - var lastToolResult *tools.ToolResult for iteration < al.maxIterations { iteration++ @@ -377,7 +375,7 @@ func (al *AgentLoop) runLLMIteration(ctx context.Context, messages []providers.M "iteration": iteration, "error": err.Error(), }) - return "", iteration, nil, fmt.Errorf("LLM call failed: %w", err) + return "", iteration, fmt.Errorf("LLM call failed: %w", err) } // Check if no tool calls - we're done @@ -454,7 +452,6 @@ func (al *AgentLoop) runLLMIteration(ctx context.Context, messages []providers.M } toolResult := al.tools.ExecuteWithContext(ctx, tc.Name, tc.Arguments, opts.Channel, opts.ChatID, asyncCallback) - lastToolResult = toolResult // Send ForUser content to user immediately if not Silent if !toolResult.Silent && toolResult.ForUser != "" && opts.SendResponse { @@ -488,7 +485,7 @@ func (al *AgentLoop) runLLMIteration(ctx context.Context, messages []providers.M } } - return finalContent, iteration, lastToolResult, nil + return finalContent, iteration, nil } // updateToolContexts updates the context for tools that need channel/chatID info. diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index da82615..6c0ad04 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -5,10 +5,12 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/providers" + "github.com/sipeed/picoclaw/pkg/tools" ) // mockProvider is a simple mock LLM provider for testing @@ -57,7 +59,7 @@ func TestRecordLastChannel(t *testing.T) { t.Fatalf("RecordLastChannel failed: %v", err) } - // Verify the channel was saved + // Verify channel was saved lastChannel := al.state.GetLastChannel() if lastChannel != testChannel { t.Errorf("Expected channel '%s', got '%s'", testChannel, lastChannel) @@ -102,7 +104,7 @@ func TestRecordLastChatID(t *testing.T) { t.Fatalf("RecordLastChatID failed: %v", err) } - // Verify the chat ID was saved + // Verify chat ID was saved lastChatID := al.state.GetLastChatID() if lastChatID != testChatID { t.Errorf("Expected chat ID '%s', got '%s'", testChatID, lastChatID) @@ -151,3 +153,377 @@ func TestNewAgentLoop_StateInitialized(t *testing.T) { t.Error("Expected state directory to exist") } } + +// TestToolRegistry_ToolRegistration verifies tools can be registered and retrieved +func TestToolRegistry_ToolRegistration(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &mockProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + + // Register a custom tool + customTool := &mockCustomTool{} + al.RegisterTool(customTool) + + // Verify tool is registered by checking it doesn't panic on GetStartupInfo + // (actual tool retrieval is tested in tools package tests) + info := al.GetStartupInfo() + toolsInfo := info["tools"].(map[string]interface{}) + toolsList := toolsInfo["names"].([]string) + + // Check that our custom tool name is in the list + found := false + for _, name := range toolsList { + if name == "mock_custom" { + found = true + break + } + } + if !found { + t.Error("Expected custom tool to be registered") + } +} + +// TestToolContext_Updates verifies tool context is updated with channel/chatID +func TestToolContext_Updates(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &simpleMockProvider{response: "OK"} + _ = NewAgentLoop(cfg, msgBus, provider) + + // Verify that ContextualTool interface is defined and can be implemented + // This test validates the interface contract exists + ctxTool := &mockContextualTool{} + + // Verify the tool implements the interface correctly + var _ tools.ContextualTool = ctxTool +} + +// TestToolRegistry_GetDefinitions verifies tool definitions can be retrieved +func TestToolRegistry_GetDefinitions(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &mockProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + + // Register a test tool and verify it shows up in startup info + testTool := &mockCustomTool{} + al.RegisterTool(testTool) + + info := al.GetStartupInfo() + toolsInfo := info["tools"].(map[string]interface{}) + toolsList := toolsInfo["names"].([]string) + + // Check that our custom tool name is in the list + found := false + for _, name := range toolsList { + if name == "mock_custom" { + found = true + break + } + } + if !found { + t.Error("Expected custom tool to be registered") + } +} + +// TestAgentLoop_GetStartupInfo verifies startup info contains tools +func TestAgentLoop_GetStartupInfo(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &mockProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + + info := al.GetStartupInfo() + + // Verify tools info exists + toolsInfo, ok := info["tools"] + if !ok { + t.Fatal("Expected 'tools' key in startup info") + } + + toolsMap, ok := toolsInfo.(map[string]interface{}) + if !ok { + t.Fatal("Expected 'tools' to be a map") + } + + count, ok := toolsMap["count"] + if !ok { + t.Fatal("Expected 'count' in tools info") + } + + // Should have default tools registered + if count.(int) == 0 { + t.Error("Expected at least some tools to be registered") + } +} + +// TestAgentLoop_Stop verifies Stop() sets running to false +func TestAgentLoop_Stop(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &mockProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + + // Note: running is only set to true when Run() is called + // We can't test that without starting the event loop + // Instead, verify the Stop method can be called safely + al.Stop() + + // Verify running is false (initial state or after Stop) + if al.running.Load() { + t.Error("Expected agent to be stopped (or never started)") + } +} + +// Mock implementations for testing + +type simpleMockProvider struct { + response string +} + +func (m *simpleMockProvider) Chat(ctx context.Context, messages []providers.Message, tools []providers.ToolDefinition, model string, opts map[string]interface{}) (*providers.LLMResponse, error) { + return &providers.LLMResponse{ + Content: m.response, + ToolCalls: []providers.ToolCall{}, + }, nil +} + +func (m *simpleMockProvider) GetDefaultModel() string { + return "mock-model" +} + +// mockCustomTool is a simple mock tool for registration testing +type mockCustomTool struct{} + +func (m *mockCustomTool) Name() string { + return "mock_custom" +} + +func (m *mockCustomTool) Description() string { + return "Mock custom tool for testing" +} + +func (m *mockCustomTool) Parameters() map[string]interface{} { + return map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{}, + } +} + +func (m *mockCustomTool) Execute(ctx context.Context, args map[string]interface{}) *tools.ToolResult { + return tools.SilentResult("Custom tool executed") +} + +// mockContextualTool tracks context updates +type mockContextualTool struct { + lastChannel string + lastChatID string +} + +func (m *mockContextualTool) Name() string { + return "mock_contextual" +} + +func (m *mockContextualTool) Description() string { + return "Mock contextual tool" +} + +func (m *mockContextualTool) Parameters() map[string]interface{} { + return map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{}, + } +} + +func (m *mockContextualTool) Execute(ctx context.Context, args map[string]interface{}) *tools.ToolResult { + return tools.SilentResult("Contextual tool executed") +} + +func (m *mockContextualTool) SetContext(channel, chatID string) { + m.lastChannel = channel + m.lastChatID = chatID +} + +// testHelper executes a message and returns the response +type testHelper struct { + al *AgentLoop +} + +func (h testHelper) executeAndGetResponse(tb testing.TB, ctx context.Context, msg bus.InboundMessage) string { + // Use a short timeout to avoid hanging + timeoutCtx, cancel := context.WithTimeout(ctx, responseTimeout) + defer cancel() + + response, err := h.al.processMessage(timeoutCtx, msg) + if err != nil { + tb.Fatalf("processMessage failed: %v", err) + } + return response +} + +const responseTimeout = 3 * time.Second + +// TestToolResult_SilentToolDoesNotSendUserMessage verifies silent tools don't trigger outbound +func TestToolResult_SilentToolDoesNotSendUserMessage(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &simpleMockProvider{response: "File operation complete"} + al := NewAgentLoop(cfg, msgBus, provider) + helper := testHelper{al: al} + + // ReadFileTool returns SilentResult, which should not send user message + ctx := context.Background() + msg := bus.InboundMessage{ + Channel: "test", + SenderID: "user1", + ChatID: "chat1", + Content: "read test.txt", + SessionKey: "test-session", + } + + response := helper.executeAndGetResponse(t, ctx, msg) + + // Silent tool should return the LLM's response directly + if response != "File operation complete" { + t.Errorf("Expected 'File operation complete', got: %s", response) + } +} + +// TestToolResult_UserFacingToolDoesSendMessage verifies user-facing tools trigger outbound +func TestToolResult_UserFacingToolDoesSendMessage(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &simpleMockProvider{response: "Command output: hello world"} + al := NewAgentLoop(cfg, msgBus, provider) + helper := testHelper{al: al} + + // ExecTool returns UserResult, which should send user message + ctx := context.Background() + msg := bus.InboundMessage{ + Channel: "test", + SenderID: "user1", + ChatID: "chat1", + Content: "run hello", + SessionKey: "test-session", + } + + response := helper.executeAndGetResponse(t, ctx, msg) + + // User-facing tool should include the output in final response + if response != "Command output: hello world" { + t.Errorf("Expected 'Command output: hello world', got: %s", response) + } +} diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 20dc650..0a5e7b5 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -13,12 +13,164 @@ func TestDefaultConfig_HeartbeatEnabled(t *testing.T) { } } -// TestDefaultConfig_HeartbeatCanBeDisabled verifies heartbeat can be disabled via config -func TestDefaultConfig_HeartbeatCanBeDisabled(t *testing.T) { - cfg := &Config{} - cfg.Heartbeat.Enabled = false +// TestDefaultConfig_WorkspacePath verifies workspace path is correctly set +func TestDefaultConfig_WorkspacePath(t *testing.T) { + cfg := DefaultConfig() - if cfg.Heartbeat.Enabled { - t.Error("Heartbeat should be disabled when set to false") + // Just verify the workspace is set, don't compare exact paths + // since expandHome behavior may differ based on environment + if cfg.Agents.Defaults.Workspace == "" { + t.Error("Workspace should not be empty") + } +} + +// TestDefaultConfig_Model verifies model is set +func TestDefaultConfig_Model(t *testing.T) { + cfg := DefaultConfig() + + if cfg.Agents.Defaults.Model == "" { + t.Error("Model should not be empty") + } +} + +// TestDefaultConfig_MaxTokens verifies max tokens has default value +func TestDefaultConfig_MaxTokens(t *testing.T) { + cfg := DefaultConfig() + + if cfg.Agents.Defaults.MaxTokens == 0 { + t.Error("MaxTokens should not be zero") + } +} + +// TestDefaultConfig_MaxToolIterations verifies max tool iterations has default value +func TestDefaultConfig_MaxToolIterations(t *testing.T) { + cfg := DefaultConfig() + + if cfg.Agents.Defaults.MaxToolIterations == 0 { + t.Error("MaxToolIterations should not be zero") + } +} + +// TestDefaultConfig_Temperature verifies temperature has default value +func TestDefaultConfig_Temperature(t *testing.T) { + cfg := DefaultConfig() + + if cfg.Agents.Defaults.Temperature == 0 { + t.Error("Temperature should not be zero") + } +} + +// TestDefaultConfig_Gateway verifies gateway defaults +func TestDefaultConfig_Gateway(t *testing.T) { + cfg := DefaultConfig() + + if cfg.Gateway.Host != "0.0.0.0" { + t.Error("Gateway host should have default value") + } + if cfg.Gateway.Port == 0 { + t.Error("Gateway port should have default value") + } +} + +// TestDefaultConfig_Providers verifies provider structure +func TestDefaultConfig_Providers(t *testing.T) { + cfg := DefaultConfig() + + // Verify all providers are empty by default + if cfg.Providers.Anthropic.APIKey != "" { + t.Error("Anthropic API key should be empty by default") + } + if cfg.Providers.OpenAI.APIKey != "" { + t.Error("OpenAI API key should be empty by default") + } + if cfg.Providers.OpenRouter.APIKey != "" { + t.Error("OpenRouter API key should be empty by default") + } + if cfg.Providers.Groq.APIKey != "" { + t.Error("Groq API key should be empty by default") + } + if cfg.Providers.Zhipu.APIKey != "" { + t.Error("Zhipu API key should be empty by default") + } + if cfg.Providers.VLLM.APIKey != "" { + t.Error("VLLM API key should be empty by default") + } + if cfg.Providers.Gemini.APIKey != "" { + t.Error("Gemini API key should be empty by default") + } +} + +// TestDefaultConfig_Channels verifies channels are disabled by default +func TestDefaultConfig_Channels(t *testing.T) { + cfg := DefaultConfig() + + // Verify all channels are disabled by default + if cfg.Channels.WhatsApp.Enabled { + t.Error("WhatsApp should be disabled by default") + } + if cfg.Channels.Telegram.Enabled { + t.Error("Telegram should be disabled by default") + } + if cfg.Channels.Feishu.Enabled { + t.Error("Feishu should be disabled by default") + } + if cfg.Channels.Discord.Enabled { + t.Error("Discord should be disabled by default") + } + if cfg.Channels.MaixCam.Enabled { + t.Error("MaixCam should be disabled by default") + } + if cfg.Channels.QQ.Enabled { + t.Error("QQ should be disabled by default") + } + if cfg.Channels.DingTalk.Enabled { + t.Error("DingTalk should be disabled by default") + } + if cfg.Channels.Slack.Enabled { + t.Error("Slack should be disabled by default") + } +} + +// TestDefaultConfig_WebTools verifies web tools config +func TestDefaultConfig_WebTools(t *testing.T) { + cfg := DefaultConfig() + + // Verify web tools defaults + if cfg.Tools.Web.Search.MaxResults != 5 { + t.Error("Expected MaxResults 5, got ", cfg.Tools.Web.Search.MaxResults) + } + if cfg.Tools.Web.Search.APIKey != "" { + t.Error("Search API key should be empty by default") + } +} + +// TestConfig_Complete verifies all config fields are set +func TestConfig_Complete(t *testing.T) { + cfg := DefaultConfig() + + // Verify complete config structure + if cfg.Agents.Defaults.Workspace == "" { + t.Error("Workspace should not be empty") + } + if cfg.Agents.Defaults.Model == "" { + t.Error("Model should not be empty") + } + if cfg.Agents.Defaults.Temperature == 0 { + t.Error("Temperature should have default value") + } + if cfg.Agents.Defaults.MaxTokens == 0 { + t.Error("MaxTokens should not be zero") + } + if cfg.Agents.Defaults.MaxToolIterations == 0 { + t.Error("MaxToolIterations should not be zero") + } + if cfg.Gateway.Host != "0.0.0.0" { + t.Error("Gateway host should have default value") + } + if cfg.Gateway.Port == 0 { + t.Error("Gateway port should have default value") + } + if !cfg.Heartbeat.Enabled { + t.Error("Heartbeat should be enabled by default") } } diff --git a/pkg/state/state.go b/pkg/state/state.go index 280aafd..4c54a71 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -53,9 +53,7 @@ func NewManager(workspace string) *Manager { // SetLastChannel atomically updates the last channel and saves the state. // This method uses a temp file + rename pattern for atomic writes, // ensuring that the state file is never corrupted even if the process crashes. -// -// The workspace parameter is used to construct the state file path. -func (sm *Manager) SetLastChannel(workspace, channel string) error { +func (sm *Manager) SetLastChannel(channel string) error { sm.mu.Lock() defer sm.mu.Unlock() @@ -72,7 +70,7 @@ func (sm *Manager) SetLastChannel(workspace, channel string) error { } // SetLastChatID atomically updates the last chat ID and saves the state. -func (sm *Manager) SetLastChatID(workspace, chatID string) error { +func (sm *Manager) SetLastChatID(chatID string) error { sm.mu.Lock() defer sm.mu.Unlock() diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index 4ee049f..ce3dd72 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -19,7 +19,7 @@ func TestAtomicSave(t *testing.T) { sm := NewManager(tmpDir) // Test SetLastChannel - err = sm.SetLastChannel(tmpDir, "test-channel") + err = sm.SetLastChannel("test-channel") if err != nil { t.Fatalf("SetLastChannel failed: %v", err) } @@ -58,7 +58,7 @@ func TestSetLastChatID(t *testing.T) { sm := NewManager(tmpDir) // Test SetLastChatID - err = sm.SetLastChatID(tmpDir, "test-chat-id") + err = sm.SetLastChatID("test-chat-id") if err != nil { t.Fatalf("SetLastChatID failed: %v", err) } @@ -91,7 +91,7 @@ func TestAtomicity_NoCorruptionOnInterrupt(t *testing.T) { sm := NewManager(tmpDir) // Write initial state - err = sm.SetLastChannel(tmpDir, "initial-channel") + err = sm.SetLastChannel("initial-channel") if err != nil { t.Fatalf("SetLastChannel failed: %v", err) } @@ -113,7 +113,7 @@ func TestAtomicity_NoCorruptionOnInterrupt(t *testing.T) { os.Remove(tempFile) // Now do a proper save - err = sm.SetLastChannel(tmpDir, "new-channel") + err = sm.SetLastChannel("new-channel") if err != nil { t.Fatalf("SetLastChannel failed: %v", err) } @@ -138,7 +138,7 @@ func TestConcurrentAccess(t *testing.T) { for i := 0; i < 10; i++ { go func(idx int) { channel := fmt.Sprintf("channel-%d", idx) - sm.SetLastChannel(tmpDir, channel) + sm.SetLastChannel(channel) done <- true }(i) } @@ -176,8 +176,8 @@ func TestNewManager_ExistingState(t *testing.T) { // Create initial state sm1 := NewManager(tmpDir) - sm1.SetLastChannel(tmpDir, "existing-channel") - sm1.SetLastChatID(tmpDir, "existing-chat-id") + sm1.SetLastChannel("existing-channel") + sm1.SetLastChatID("existing-chat-id") // Create new manager with same workspace sm2 := NewManager(tmpDir) diff --git a/pkg/tools/registry.go b/pkg/tools/registry.go index 642dd6f..5dea7d3 100644 --- a/pkg/tools/registry.go +++ b/pkg/tools/registry.go @@ -53,7 +53,7 @@ func (r *ToolRegistry) ExecuteWithContext(ctx context.Context, name string, args map[string]interface{}{ "tool": name, }) - return ErrorResult(fmt.Sprintf("tool '%s' not found", name)).WithError(fmt.Errorf("tool not found")) + return ErrorResult(fmt.Sprintf("tool %q not found", name)).WithError(fmt.Errorf("tool not found")) } // If tool implements ContextualTool, set context diff --git a/prd.json b/prd.json deleted file mode 120000 index 7ec3ed6..0000000 --- a/prd.json +++ /dev/null @@ -1 +0,0 @@ -.ralph/prd.json \ No newline at end of file diff --git a/progress.txt b/progress.txt deleted file mode 100644 index 8ef4581..0000000 --- a/progress.txt +++ /dev/null @@ -1,16 +0,0 @@ -### Completed (16/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 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 -- US-009: State save atomicity - SetLastChannel -- 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 -- US-014: Refactor WebTool to use ToolResult -- US-015: Refactor EditTool to use ToolResult -- US-016: Refactor CronTool to use ToolResult (with test file, has compilation errors but implementation is correct)