diff --git a/pkg/multiagent/blackboard_test.go b/pkg/multiagent/blackboard_test.go index fe204fb7d..89ff0aefb 100644 --- a/pkg/multiagent/blackboard_test.go +++ b/pkg/multiagent/blackboard_test.go @@ -328,6 +328,55 @@ func TestBlackboardTool_SetBoard(t *testing.T) { } } +// TestBlackboard_UnmarshalJSON_InvalidData verifies that UnmarshalJSON returns an error +// for malformed input instead of silently producing a broken blackboard. +func TestBlackboard_UnmarshalJSON_InvalidData(t *testing.T) { + bb := NewBlackboard() + err := bb.UnmarshalJSON([]byte("not valid json")) + if err == nil { + t.Error("expected error for invalid JSON input") + } + // Board should remain empty after a failed unmarshal + if bb.Size() != 0 { + t.Errorf("Size() = %d after failed unmarshal, want 0", bb.Size()) + } +} + +// TestBlackboardTool_ListEmpty verifies the "Blackboard is empty" message path when +// the board has no entries. +func TestBlackboardTool_ListEmpty(t *testing.T) { + bb := NewBlackboard() + tool := NewBlackboardTool(bb, "lister") + + result := tool.Execute(context.Background(), map[string]any{ + "action": "list", + }) + if result.IsError { + t.Fatalf("list on empty board should not error: %s", result.ForLLM) + } + if !contains(result.ForLLM, "empty") { + t.Errorf("expected 'empty' in result for empty board, got %q", result.ForLLM) + } +} + +// TestBlackboardTool_DeleteMissing verifies the "not found" path for delete on a +// key that does not exist. +func TestBlackboardTool_DeleteMissing(t *testing.T) { + bb := NewBlackboard() + tool := NewBlackboardTool(bb, "deleter") + + result := tool.Execute(context.Background(), map[string]any{ + "action": "delete", + "key": "nonexistent_key", + }) + if result.IsError { + t.Fatalf("delete of missing key should not be an error: %s", result.ForLLM) + } + if !contains(result.ForLLM, "not found") { + t.Errorf("expected 'not found' in result, got %q", result.ForLLM) + } +} + func TestBoardAware_Interface(t *testing.T) { // Verify both tools implement BoardAware bb := NewBlackboard() diff --git a/pkg/multiagent/handoff_test.go b/pkg/multiagent/handoff_test.go index 8cae66738..eb3527a28 100644 --- a/pkg/multiagent/handoff_test.go +++ b/pkg/multiagent/handoff_test.go @@ -2,6 +2,7 @@ package multiagent import ( "context" + "fmt" "strings" "testing" @@ -600,6 +601,142 @@ func TestAllowlistCheckerFunc(t *testing.T) { } } +// TestExecuteHandoff_DepthBoundary verifies that depth == maxDepth - 1 (one below limit) succeeds, +// while depth == maxDepth fails. This is the exact boundary behaviour of the recursion guard. +func TestExecuteHandoff_DepthBoundary(t *testing.T) { + provider := &mockProvider{response: "done"} + resolver := newMockResolver(&AgentInfo{ + ID: "target", Name: "Target", Model: "test", + Provider: provider, Tools: tools.NewToolRegistry(), MaxIter: 5, + }) + bb := NewBlackboard() + + // depth == maxDepth - 1 (2 < 3): must succeed + result := ExecuteHandoff(context.Background(), resolver, bb, HandoffRequest{ + FromAgentID: "main", + ToAgentID: "target", + Task: "do something", + Depth: 2, + MaxDepth: 3, + Visited: []string{"main", "middle"}, + }, "cli", "direct") + if !result.Success { + t.Errorf("depth == maxDepth-1 should succeed, got error: %s", result.Error) + } + + // depth == maxDepth (3 >= 3): must fail + result = ExecuteHandoff(context.Background(), resolver, bb, HandoffRequest{ + FromAgentID: "main", + ToAgentID: "target", + Task: "do something", + Depth: 3, + MaxDepth: 3, + Visited: []string{"main", "a", "b"}, + }, "cli", "direct") + if result.Success { + t.Error("depth == maxDepth should fail") + } + if !strings.Contains(result.Error, "depth limit") { + t.Errorf("Error = %q, expected 'depth limit'", result.Error) + } +} + +// TestExecuteHandoff_ProviderError verifies that a provider error during RunToolLoop +// is surfaced as a failed HandoffResult with an error message. +func TestExecuteHandoff_ProviderError(t *testing.T) { + provider := &mockProvider{err: fmt.Errorf("LLM provider unavailable")} + resolver := newMockResolver(&AgentInfo{ + ID: "target", Name: "Target", Model: "test", + Provider: provider, Tools: tools.NewToolRegistry(), MaxIter: 5, + }) + + bb := NewBlackboard() + result := ExecuteHandoff(context.Background(), resolver, bb, HandoffRequest{ + FromAgentID: "main", + ToAgentID: "target", + Task: "failing task", + }, "cli", "direct") + + if result.Success { + t.Error("expected failure when provider returns error") + } + if !strings.Contains(result.Error, "provider unavailable") { + t.Errorf("Error = %q, expected provider error message", result.Error) + } + if result.AgentID != "target" { + t.Errorf("AgentID = %q, want 'target'", result.AgentID) + } +} + +// TestExecuteHandoff_MaxIterDefault verifies that MaxIter == 0 on the target agent +// is defaulted to 10 inside ExecuteHandoff (not left as 0 which would mean no iterations). +func TestExecuteHandoff_MaxIterDefault(t *testing.T) { + provider := &mockProvider{response: "ran with default iter"} + resolver := newMockResolver(&AgentInfo{ + ID: "target", + Name: "Target", + Model: "test", + Provider: provider, + Tools: tools.NewToolRegistry(), + MaxIter: 0, // explicitly zero, should default to 10 + }) + + bb := NewBlackboard() + result := ExecuteHandoff(context.Background(), resolver, bb, HandoffRequest{ + FromAgentID: "main", + ToAgentID: "target", + Task: "task with default iter", + }, "cli", "direct") + + if !result.Success { + t.Errorf("expected success with default MaxIter, got: %s", result.Error) + } +} + +// TestExecuteHandoff_CycleDetectionSingleHop verifies A->A (self-handoff) is caught. +func TestExecuteHandoff_CycleDetectionSingleHop(t *testing.T) { + provider := &mockProvider{response: "done"} + resolver := newMockResolver(&AgentInfo{ + ID: "main", Name: "Main", Model: "test", + Provider: provider, Tools: tools.NewToolRegistry(), MaxIter: 5, + }) + + bb := NewBlackboard() + // "main" handing off to itself, already in visited + result := ExecuteHandoff(context.Background(), resolver, bb, HandoffRequest{ + FromAgentID: "main", + ToAgentID: "main", + Task: "self task", + Depth: 0, + Visited: []string{"main"}, + }, "cli", "direct") + + if result.Success { + t.Error("expected failure for self-handoff cycle") + } + if !strings.Contains(result.Error, "cycle detected") { + t.Errorf("Error = %q, expected 'cycle detected'", result.Error) + } +} + +// TestHandoffTool_SetContext verifies SetContext updates origin channel and chatID. +func TestHandoffTool_SetContext(t *testing.T) { + resolver := newMockResolver() + bb := NewBlackboard() + tool := NewHandoffTool(resolver, bb, "main") + + tool.SetContext("telegram", "chat-123") + + // Verify fields are updated (access via the exported setter, values verified by ensuring + // no panic and the defaults were overwritten — integration confirmed via Execute routing). + if tool.originChannel != "telegram" { + t.Errorf("originChannel = %q, want %q", tool.originChannel, "telegram") + } + if tool.originChatID != "chat-123" { + t.Errorf("originChatID = %q, want %q", tool.originChatID, "chat-123") + } +} + func TestBuildHandoffSystemPrompt(t *testing.T) { agent := &AgentInfo{ Name: "Code Agent", diff --git a/pkg/tools/hooks_test.go b/pkg/tools/hooks_test.go index d984aa878..13670cbe0 100644 --- a/pkg/tools/hooks_test.go +++ b/pkg/tools/hooks_test.go @@ -111,3 +111,72 @@ func TestToolHook_FirstBlockStopsChain(t *testing.T) { t.Error("hook2 before should NOT have been called (chain stopped)") } } + +// TestToolHook_AfterExecuteRunsForAllHooksOnBlock verifies that when a BeforeExecute +// hook blocks execution, AfterExecute is still invoked on ALL registered hooks +// (not just the blocking one) for observability purposes. +func TestToolHook_AfterExecuteRunsForAllHooksOnBlock(t *testing.T) { + reg := NewToolRegistry() + reg.Register(&dummyTool{name: "observed_tool"}) + + hook1 := &testHook{blockTool: "observed_tool"} + hook2 := &testHook{} // does not block, but should still get AfterExecute + reg.AddHook(hook1) + reg.AddHook(hook2) + + result := reg.Execute(context.Background(), "observed_tool", nil) + + if !result.IsError { + t.Error("expected error result when hook1 blocks") + } + // BeforeExecute: hook1 called, hook2 NOT called (chain stopped) + if len(hook1.beforeCalls) != 1 { + t.Errorf("hook1.beforeCalls = %d, want 1", len(hook1.beforeCalls)) + } + if len(hook2.beforeCalls) != 0 { + t.Errorf("hook2.beforeCalls = %d, want 0 (chain stopped)", len(hook2.beforeCalls)) + } + // AfterExecute: BOTH hooks called (inner loop over all hooks for observability) + if len(hook1.afterCalls) != 1 { + t.Errorf("hook1.afterCalls = %d, want 1", len(hook1.afterCalls)) + } + if len(hook2.afterCalls) != 1 { + t.Errorf("hook2.afterCalls = %d, want 1 (AfterExecute runs for all hooks even on block)", len(hook2.afterCalls)) + } +} + +// TestToolHook_NotFoundToolSkipsHooks verifies that hooks are not called when +// a tool does not exist in the registry. +func TestToolHook_NotFoundToolSkipsHooks(t *testing.T) { + reg := NewToolRegistry() + // Do NOT register the tool + + hook := &testHook{} + reg.AddHook(hook) + + result := reg.Execute(context.Background(), "ghost_tool", nil) + + if !result.IsError { + t.Error("expected error for unknown tool") + } + // Hooks should not be called when the tool doesn't exist (early return before hook loop) + if len(hook.beforeCalls) != 0 { + t.Errorf("hook.beforeCalls = %d, want 0 for missing tool", len(hook.beforeCalls)) + } + if len(hook.afterCalls) != 0 { + t.Errorf("hook.afterCalls = %d, want 0 for missing tool", len(hook.afterCalls)) + } +} + +// TestToolHook_NoHooksSucceeds verifies that a tool executes normally with no hooks registered. +func TestToolHook_NoHooksSucceeds(t *testing.T) { + reg := NewToolRegistry() + reg.Register(&dummyTool{name: "plain_tool"}) + // No hooks added + + result := reg.Execute(context.Background(), "plain_tool", nil) + + if result.IsError { + t.Errorf("expected success with no hooks, got error: %s", result.ForLLM) + } +}