test: add AgentReporter interface and event-ordering tests

- pkg/orch/reporter_test.go: Noop panic-safety + Broadcaster Report*
  method field-mapping and snapshot lifecycle (5 tests)
- pkg/tools/toolloop_reporter_test.go: nil-reporter fallback and
  waiting→toolcall(echo_tool)→waiting ordering (4 tests)
- pkg/tools/subagent_reporter_test.go: Spawn full lifecycle events
  via real Broadcaster (agent_spawn→conversation→state→gc) and
  snapshot liveness check (2 tests)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

pkg/orch/reporter_test.go

@@ -0,0 +1,102 @@
+package orch
+
+import "testing"
+
+// Compile-time: Broadcaster must satisfy AgentReporter.
+var _ AgentReporter = (*Broadcaster)(nil)
+
+// TestNoop_AllMethods_NoPanic verifies that orch.Noop can be called for all
+// four methods without panic. This is the nil-safe baseline for disabled
+// orchestration mode.
+func TestNoop_AllMethods_NoPanic(t *testing.T) {
+	Noop.ReportSpawn("id", "label", "task")
+	Noop.ReportStateChange("id", "waiting", "")
+	Noop.ReportStateChange("id", "toolcall", "bash")
+	Noop.ReportConversation("conductor", "sub-1", "do something")
+	Noop.ReportGC("id", "completed")
+}
+
+// TestBroadcaster_ReportSpawn_MapsToAgentSpawnEvent verifies that ReportSpawn
+// publishes an Event with Type="agent_spawn" and the correct ID/Label/Task
+// fields, and that the agent appears in the Snapshot immediately.
+func TestBroadcaster_ReportSpawn_MapsToAgentSpawnEvent(t *testing.T) {
+	b := NewBroadcaster()
+	sub := b.Subscribe()
+	defer b.Unsubscribe(sub)
+
+	b.ReportSpawn("agent-1", "scout", "find all TODOs")
+
+	ev := <-sub.Ch
+	if ev.Type != "agent_spawn" {
+		t.Fatalf("want agent_spawn, got %q", ev.Type)
+	}
+	if ev.ID != "agent-1" || ev.Label != "scout" || ev.Task != "find all TODOs" {
+		t.Fatalf("field mismatch: %+v", ev)
+	}
+	snap := b.Snapshot()
+	if len(snap) != 1 || snap[0].ID != "agent-1" || snap[0].Label != "scout" {
+		t.Fatalf("snapshot not updated correctly: %v", snap)
+	}
+}
+
+// TestBroadcaster_ReportStateChange_MapsToAgentStateEvent verifies that
+// ReportStateChange publishes agent_state and updates the live snapshot.
+func TestBroadcaster_ReportStateChange_MapsToAgentStateEvent(t *testing.T) {
+	b := NewBroadcaster()
+	sub := b.Subscribe()
+	defer b.Unsubscribe(sub)
+
+	b.ReportSpawn("agent-1", "coder", "implement it")
+	<-sub.Ch // consume spawn
+
+	b.ReportStateChange("agent-1", "toolcall", "bash")
+	ev := <-sub.Ch
+	if ev.Type != "agent_state" || ev.State != "toolcall" || ev.Tool != "bash" {
+		t.Fatalf("unexpected event: %+v", ev)
+	}
+	snap := b.Snapshot()
+	if snap[0].State != "toolcall" || snap[0].Tool != "bash" {
+		t.Fatalf("snapshot state not updated: %v", snap)
+	}
+}
+
+// TestBroadcaster_ReportConversation_MapsToConversationEvent verifies that
+// ReportConversation publishes a conversation event with correct From/To/Text
+// fields and does NOT modify the agent snapshot (conversation is not a state
+// change of any agent).
+func TestBroadcaster_ReportConversation_MapsToConversationEvent(t *testing.T) {
+	b := NewBroadcaster()
+	sub := b.Subscribe()
+	defer b.Unsubscribe(sub)
+
+	b.ReportConversation("conductor", "sub-1", "please do the task")
+
+	ev := <-sub.Ch
+	if ev.Type != "conversation" || ev.From != "conductor" || ev.To != "sub-1" || ev.Text != "please do the task" {
+		t.Fatalf("unexpected event: %+v", ev)
+	}
+	if len(b.Snapshot()) != 0 {
+		t.Fatal("conversation event must not modify agent snapshot")
+	}
+}
+
+// TestBroadcaster_ReportGC_RemovesAgentFromSnapshot verifies that ReportGC
+// publishes agent_gc with the correct Reason and removes the agent from the
+// live snapshot so new WS connections no longer see it.
+func TestBroadcaster_ReportGC_RemovesAgentFromSnapshot(t *testing.T) {
+	b := NewBroadcaster()
+	sub := b.Subscribe()
+	defer b.Unsubscribe(sub)
+
+	b.ReportSpawn("agent-1", "scout", "task")
+	<-sub.Ch // consume spawn
+
+	b.ReportGC("agent-1", "completed")
+	ev := <-sub.Ch
+	if ev.Type != "agent_gc" || ev.ID != "agent-1" || ev.Reason != "completed" {
+		t.Fatalf("unexpected event: %+v", ev)
+	}
+	if len(b.Snapshot()) != 0 {
+		t.Fatal("agent must be removed from snapshot after ReportGC")
+	}
+}

pkg/tools/subagent_reporter_test.go

@@ -0,0 +1,153 @@
+package tools
+
+import (
+	"context"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/sipeed/picoclaw/pkg/orch"
+)
+
+// TestSubagentManager_Spawn_EmitsLifecycleEvents verifies that Spawn() fires
+// the correct sequence of orchestration events through a real Broadcaster:
+//
+//	agent_spawn → conversation(conductor→sub) → agent_state(waiting) →
+//	conversation(sub→conductor) → agent_gc(completed)
+//
+// It also verifies that the snapshot is empty after ReportGC and that the
+// completion callback is invoked.
+func TestSubagentManager_Spawn_EmitsLifecycleEvents(t *testing.T) {
+	b := orch.NewBroadcaster()
+	sub := b.Subscribe()
+	defer b.Unsubscribe(sub)
+
+	provider := &MockLLMProvider{}
+	mgr := NewSubagentManager(provider, "test-model", "/tmp/test", nil, b)
+
+	var callbackCalled int32
+	cb := AsyncCallback(func(_ context.Context, _ *ToolResult) {
+		atomic.StoreInt32(&callbackCalled, 1)
+	})
+
+	_, err := mgr.Spawn(
+		context.Background(),
+		"say hello", "hello-task", "", "cli", "direct",
+		cb,
+	)
+	if err != nil {
+		t.Fatalf("Spawn() error: %v", err)
+	}
+
+	// Collect events until agent_gc or timeout.
+	var events []orch.Event
+	deadline := time.After(3 * time.Second)
+loop:
+	for {
+		select {
+		case ev := <-sub.Ch:
+			events = append(events, ev)
+			if ev.Type == "agent_gc" {
+				break loop
+			}
+		case <-deadline:
+			t.Fatalf("timed out waiting for agent_gc; events so far: %+v", events)
+		}
+	}
+
+	// 1. First event must be agent_spawn with the correct label.
+	if len(events) == 0 || events[0].Type != "agent_spawn" {
+		t.Fatalf("first event must be agent_spawn, got: %+v", events)
+	}
+	if events[0].Label != "hello-task" {
+		t.Errorf("agent_spawn label = %q, want %q", events[0].Label, "hello-task")
+	}
+	spawnedID := events[0].ID
+
+	// 2. There must be a conversation from conductor → subagent.
+	var hasConvToSub bool
+	for _, ev := range events {
+		if ev.Type == "conversation" && ev.From == "conductor" && ev.To == spawnedID {
+			hasConvToSub = true
+			break
+		}
+	}
+	if !hasConvToSub {
+		t.Errorf("missing conversation(conductor → %s); events: %+v", spawnedID, events)
+	}
+
+	// 3. There must be at least one agent_state(waiting) for the subagent.
+	var hasWaiting bool
+	for _, ev := range events {
+		if ev.Type == "agent_state" && ev.ID == spawnedID && ev.State == "waiting" {
+			hasWaiting = true
+			break
+		}
+	}
+	if !hasWaiting {
+		t.Errorf("missing agent_state(waiting) for %s; events: %+v", spawnedID, events)
+	}
+
+	// 4. Last event must be agent_gc with reason "completed".
+	last := events[len(events)-1]
+	if last.Type != "agent_gc" || last.ID != spawnedID || last.Reason != "completed" {
+		t.Errorf("last event must be agent_gc(completed), got: %+v", last)
+	}
+
+	// 5. Snapshot must be empty after GC (agent removed from live map).
+	if snap := b.Snapshot(); len(snap) != 0 {
+		t.Errorf("snapshot must be empty after agent_gc, got: %v", snap)
+	}
+
+	// 6. Callback must be called. The callback fires in the same goroutine
+	// as ReportGC (after the deferred unlock), so we poll briefly.
+	for i := 0; i < 100; i++ {
+		if atomic.LoadInt32(&callbackCalled) == 1 {
+			break
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+	if atomic.LoadInt32(&callbackCalled) != 1 {
+		t.Error("completion callback was not called after agent_gc")
+	}
+}
+
+// TestSubagentManager_Spawn_SnapshotLiveDuringExecution verifies that the
+// Broadcaster snapshot contains the agent between agent_spawn and agent_gc.
+// Because Publish() updates the agent map before dispatching to subscribers,
+// the snapshot is guaranteed to be non-empty as soon as agent_spawn is
+// received on the channel.
+func TestSubagentManager_Spawn_SnapshotLiveDuringExecution(t *testing.T) {
+	b := orch.NewBroadcaster()
+	sub := b.Subscribe()
+	defer b.Unsubscribe(sub)
+
+	provider := &MockLLMProvider{}
+	mgr := NewSubagentManager(provider, "test-model", "/tmp/test", nil, b)
+
+	_, err := mgr.Spawn(
+		context.Background(),
+		"any task", "live-test", "", "cli", "direct",
+		nil,
+	)
+	if err != nil {
+		t.Fatalf("Spawn() error: %v", err)
+	}
+
+	// Wait for agent_spawn, then immediately check snapshot.
+	deadline := time.After(2 * time.Second)
+	for {
+		select {
+		case ev := <-sub.Ch:
+			if ev.Type == "agent_spawn" {
+				snap := b.Snapshot()
+				if len(snap) == 0 {
+					t.Error("snapshot must contain the spawned agent after agent_spawn event")
+				}
+				return // test complete; background goroutine drains safely
+			}
+		case <-deadline:
+			t.Fatal("timed out waiting for agent_spawn event")
+		}
+	}
+}

pkg/tools/toolloop_reporter_test.go

@@ -0,0 +1,170 @@
+package tools
+
+import (
+	"context"
+	"sync"
+	"testing"
+
+	"github.com/sipeed/picoclaw/pkg/orch"
+	"github.com/sipeed/picoclaw/pkg/providers"
+)
+
+// reporterSpy records every ReportStateChange call in order.
+// Spawn/Conversation/GC are not needed for toolloop tests.
+type reporterSpy struct {
+	mu    sync.Mutex
+	calls []spyCall
+}
+
+type spyCall struct {
+	state string
+	tool  string
+}
+
+func (r *reporterSpy) ReportSpawn(id, label, task string)       {}
+func (r *reporterSpy) ReportConversation(from, to, text string) {}
+func (r *reporterSpy) ReportGC(id, reason string)               {}
+func (r *reporterSpy) ReportStateChange(id, state, tool string) {
+	r.mu.Lock()
+	r.calls = append(r.calls, spyCall{state, tool})
+	r.mu.Unlock()
+}
+func (r *reporterSpy) snapshot() []spyCall {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	out := make([]spyCall, len(r.calls))
+	copy(out, r.calls)
+	return out
+}
+
+// sequenceMockProvider returns a tool call on the first Chat() call and a
+// plain text response on all subsequent calls. Used to exercise the
+// waiting → toolcall → waiting event sequence in RunToolLoop.
+type sequenceMockProvider struct {
+	mu        sync.Mutex
+	callCount int
+}
+
+func (m *sequenceMockProvider) Chat(
+	_ context.Context,
+	_ []providers.Message,
+	_ []providers.ToolDefinition,
+	_ string,
+	_ map[string]any,
+) (*providers.LLMResponse, error) {
+	m.mu.Lock()
+	m.callCount++
+	n := m.callCount
+	m.mu.Unlock()
+	if n == 1 {
+		return &providers.LLMResponse{
+			ToolCalls: []providers.ToolCall{
+				{ID: "tc-1", Name: "echo_tool", Arguments: map[string]any{"msg": "hi"}},
+			},
+		}, nil
+	}
+	return &providers.LLMResponse{Content: "done"}, nil
+}
+func (m *sequenceMockProvider) GetDefaultModel() string { return "test" }
+func (m *sequenceMockProvider) SupportsTools() bool     { return true }
+func (m *sequenceMockProvider) GetContextWindow() int   { return 4096 }
+
+// echoTool is a minimal Tool stub registered as "echo_tool".
+type echoTool struct{}
+
+func (t *echoTool) Name() string        { return "echo_tool" }
+func (t *echoTool) Description() string { return "echo" }
+func (t *echoTool) Parameters() map[string]any {
+	return map[string]any{"type": "object", "properties": map[string]any{}}
+}
+func (t *echoTool) Execute(_ context.Context, _ map[string]any) *ToolResult {
+	return &ToolResult{ForLLM: "echoed"}
+}
+
+// TestToolLoop_NilReporter_FallsBackToNoop ensures that passing nil as
+// Reporter does not panic — the loop must substitute orch.Noop internally.
+func TestToolLoop_NilReporter_FallsBackToNoop(t *testing.T) {
+	_, err := RunToolLoop(context.Background(), ToolLoopConfig{
+		Provider:      &MockLLMProvider{},
+		Model:         "test",
+		MaxIterations: 1,
+		Reporter:      nil, // must not panic
+	}, []providers.Message{{Role: "user", Content: "hi"}}, "cli", "direct")
+	if err != nil {
+		t.Fatalf("unexpected error with nil reporter: %v", err)
+	}
+}
+
+// TestToolLoop_Reporter_WaitingBeforeLLM verifies that ReportStateChange is
+// called with state="waiting" before the first LLM call. The mock provider
+// returns a direct text answer (no tool calls), so exactly one waiting event
+// is expected.
+func TestToolLoop_Reporter_WaitingBeforeLLM(t *testing.T) {
+	rep := &reporterSpy{}
+	_, err := RunToolLoop(context.Background(), ToolLoopConfig{
+		Provider:      &MockLLMProvider{},
+		Model:         "test",
+		MaxIterations: 1,
+		Reporter:      rep,
+		AgentID:       "sess-1",
+	}, []providers.Message{{Role: "user", Content: "hi"}}, "cli", "direct")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	calls := rep.snapshot()
+	if len(calls) == 0 {
+		t.Fatal("expected at least one ReportStateChange call")
+	}
+	if calls[0].state != "waiting" {
+		t.Fatalf("first call must be state=waiting, got %+v", calls[0])
+	}
+}
+
+// TestToolLoop_Reporter_ToolcallOrderedAfterWaiting verifies the canonical
+// two-iteration sequence:
+//
+//	waiting  (before 1st LLM call)
+//	toolcall(echo_tool)  (before tool execution)
+//	waiting  (before 2nd LLM call)
+//
+// The sequenceMockProvider returns a tool call on iteration 1 and a text
+// response on iteration 2, driving exactly this path.
+func TestToolLoop_Reporter_ToolcallOrderedAfterWaiting(t *testing.T) {
+	rep := &reporterSpy{}
+	reg := NewToolRegistry()
+	reg.Register(&echoTool{})
+
+	_, err := RunToolLoop(context.Background(), ToolLoopConfig{
+		Provider:      &sequenceMockProvider{},
+		Model:         "test",
+		Tools:         reg,
+		MaxIterations: 5,
+		Reporter:      rep,
+		AgentID:       "sess-1",
+	}, []providers.Message{{Role: "user", Content: "do it"}}, "cli", "direct")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	calls := rep.snapshot()
+	if len(calls) < 3 {
+		t.Fatalf("expected at least 3 calls, got %d: %+v", len(calls), calls)
+	}
+	if calls[0].state != "waiting" {
+		t.Fatalf("calls[0] must be waiting, got %+v", calls[0])
+	}
+	if calls[1].state != "toolcall" || calls[1].tool != "echo_tool" {
+		t.Fatalf("calls[1] must be toolcall(echo_tool), got %+v", calls[1])
+	}
+	if calls[2].state != "waiting" {
+		t.Fatalf("calls[2] must be waiting (2nd LLM iteration), got %+v", calls[2])
+	}
+}
+
+// TestToolLoop_Reporter_NoopImplementsInterface is a compile-time check that
+// orch.Noop satisfies the orch.AgentReporter interface accepted by
+// ToolLoopConfig.Reporter. If Noop ever stops implementing the interface the
+// build will fail here before any test runs.
+func TestToolLoop_Reporter_NoopImplementsInterface(t *testing.T) {
+	var _ orch.AgentReporter = orch.Noop
+}