fix: map acp todo tool results to session todos (#1376)

2026-06-14 21:59:33 +01:00
parent a140c5817b
commit d8b8d2b7c6
3 changed files with 214 additions and 0 deletions
@@ -270,6 +270,22 @@ func (a *Adapter) convertToolCallResultUpdate(sessionID string, tcu *acp.Session
 	// are known, schedule the synthetic prompt; on terminal status, clean up.
 	a.handleWakeupEvent(sessionID, toolCallID, tcu.Meta, tcu.RawInput, isTerminal)

+	// Todo tools from MCP-style runtimes report the final list in the completion
+	// output rather than through ACP's native plan notification. Feed those
+	// entries into the same plan stream Claude/Codex already use so the existing
+	// above-input todo indicator updates without a separate UI path. Emitted
+	// after the adapter lock is released because sendUpdate takes a read lock
+	// and sync.RWMutex is not reentrant.
+	if tcu.RawOutput != nil {
+		if entries, ok := planEntriesFromTodosResult(tcu.RawOutput); ok {
+			a.sendUpdate(AgentEvent{
+				Type:        streams.EventTypePlan,
+				SessionID:   sessionID,
+				PlanEntries: entries,
+			})
+		}
+	}
+
 	// When a switch_mode tool carries a plan (e.g. ExitPlanMode), emit it
 	// as an agent_plan event so the orchestrator creates a visible plan message.
 	if tcu.RawInput != nil {
@@ -0,0 +1,92 @@
+package acp
+
+import (
+	"github.com/kandev/kandev/internal/agentctl/types/streams"
+)
+
+func planEntriesFromTodosResult(rawOutput any) ([]streams.PlanEntry, bool) {
+	items, ok := extractTodoItems(rawOutput)
+	if !ok {
+		return nil, false
+	}
+	entries := make([]streams.PlanEntry, len(items))
+	for i, item := range items {
+		entries[i] = streams.PlanEntry{
+			Description: item.Description,
+			Status:      item.Status,
+			Priority:    item.Priority,
+		}
+	}
+	return entries, true
+}
+
+type acpTodoItem struct {
+	Description string
+	Status      string
+	Priority    string
+}
+
+func extractTodoItems(raw any) ([]acpTodoItem, bool) {
+	todosSlice, ok := findTodosSlice(raw)
+	if !ok {
+		return nil, false
+	}
+	items := make([]acpTodoItem, 0, len(todosSlice))
+	for _, rawItem := range todosSlice {
+		item, ok := todoItemFromRaw(rawItem)
+		if !ok {
+			continue
+		}
+		items = append(items, item)
+	}
+	// Require at least one recognisable item so an empty or fully-malformed
+	// todos array doesn't emit a no-op plan event that wipes the indicator.
+	if len(items) == 0 {
+		return nil, false
+	}
+	return items, true
+}
+
+func findTodosSlice(raw any) ([]any, bool) {
+	m, ok := raw.(map[string]any)
+	if !ok {
+		return nil, false
+	}
+	if todos, ok := m["todos"].([]any); ok {
+		return todos, true
+	}
+	if meta, ok := m["metadata"].(map[string]any); ok {
+		if todos, ok := meta["todos"].([]any); ok {
+			return todos, true
+		}
+	}
+	if rawOutput, ok := m["rawOutput"].(map[string]any); ok {
+		return findTodosSlice(rawOutput)
+	}
+	return nil, false
+}
+
+func todoItemFromRaw(raw any) (acpTodoItem, bool) {
+	m, ok := raw.(map[string]any)
+	if !ok {
+		return acpTodoItem{}, false
+	}
+	desc := stringFromTodoMap(m, "content", "text", "description")
+	if desc == "" {
+		return acpTodoItem{}, false
+	}
+	return acpTodoItem{
+		Description: desc,
+		Status:      stringFromTodoMap(m, "status"),
+		Priority:    stringFromTodoMap(m, "priority"),
+	}, true
+}
+
+func stringFromTodoMap(m map[string]any, keys ...string) string {
+	for _, key := range keys {
+		if value, ok := m[key].(string); ok && value != "" {
+			return value
+		}
+	}
+	return ""
+}
@@ -0,0 +1,106 @@
+package acp
+
+import "testing"
+
+func TestPlanEntriesFromTodosResult_MetadataTodos(t *testing.T) {
+	entries, ok := planEntriesFromTodosResult(map[string]any{
+		"metadata": map[string]any{
+			"todos": []any{
+				map[string]any{"content": "Gather PR state", "status": "in_progress", "priority": "high"},
+				map[string]any{"content": "Fix CI", "status": "pending", "priority": "high"},
+			},
+			"truncated": false,
+		},
+		"output": "[]",
+	})
+	if !ok {
+		t.Fatal("planEntriesFromTodosResult did not recognize metadata.todos")
+	}
+	if len(entries) != 2 {
+		t.Fatalf("len(entries) = %d, want 2", len(entries))
+	}
+	if entries[0].Description != "Gather PR state" ||
+		entries[0].Status != "in_progress" ||
+		entries[0].Priority != "high" {
+		t.Fatalf("unexpected first entry: %+v", entries[0])
+	}
+	if entries[1].Description != "Fix CI" || entries[1].Status != "pending" {
+		t.Fatalf("unexpected second entry: %+v", entries[1])
+	}
+}
+
+func TestPlanEntriesFromTodosResult_TopLevelTodos(t *testing.T) {
+	entries, ok := planEntriesFromTodosResult(map[string]any{
+		"todos": []any{
+			map[string]any{"text": "Read README", "status": "pending"},
+		},
+	})
+	if !ok || len(entries) != 1 {
+		t.Fatalf("planEntriesFromTodosResult = (%+v, %v), want one entry ok=true", entries, ok)
+	}
+	if entries[0].Description != "Read README" || entries[0].Status != "pending" {
+		t.Fatalf("unexpected entry: %+v", entries[0])
+	}
+}
+
+func TestPlanEntriesFromTodosResult_RawOutputWrapped(t *testing.T) {
+	entries, ok := planEntriesFromTodosResult(map[string]any{
+		"rawOutput": map[string]any{
+			"todos": []any{
+				map[string]any{"description": "Investigate", "status": "in_progress"},
+			},
+		},
+	})
+	if !ok || len(entries) != 1 {
+		t.Fatalf("planEntriesFromTodosResult = (%+v, %v), want one entry ok=true", entries, ok)
+	}
+	if entries[0].Description != "Investigate" || entries[0].Status != "in_progress" {
+		t.Fatalf("unexpected entry: %+v", entries[0])
+	}
+}
+
+func TestPlanEntriesFromTodosResult_SkipsItemsWithoutContent(t *testing.T) {
+	entries, ok := planEntriesFromTodosResult(map[string]any{
+		"todos": []any{
+			map[string]any{"status": "pending"},
+			map[string]any{"text": "Real item", "status": "in_progress"},
+		},
+	})
+	if !ok || len(entries) != 1 {
+		t.Fatalf("planEntriesFromTodosResult = (%+v, %v), want one valid entry", entries, ok)
+	}
+	if entries[0].Description != "Real item" {
+		t.Fatalf("unexpected description: %q", entries[0].Description)
+	}
+}
+
+func TestPlanEntriesFromTodosResult_EmptyArrayIsNotMatched(t *testing.T) {
+	if entries, ok := planEntriesFromTodosResult(map[string]any{"todos": []any{}}); ok || entries != nil {
+		t.Fatalf("planEntriesFromTodosResult = (%+v, %v), want nil false (empty array must not wipe indicator)", entries, ok)
+	}
+}
+
+func TestPlanEntriesFromTodosResult_AllMalformedIsNotMatched(t *testing.T) {
+	if entries, ok := planEntriesFromTodosResult(map[string]any{
+		"todos": []any{
+			map[string]any{"status": "pending"},
+			"not-an-object",
+		},
+	}); ok || entries != nil {
+		t.Fatalf("planEntriesFromTodosResult = (%+v, %v), want nil false (no recognisable items)", entries, ok)
+	}
+}
+
+func TestPlanEntriesFromTodosResult_IgnoresUnrelatedOutput(t *testing.T) {
+	if entries, ok := planEntriesFromTodosResult(map[string]any{"output": "hello"}); ok || entries != nil {
+		t.Fatalf("planEntriesFromTodosResult = (%+v, %v), want nil false", entries, ok)
+	}
+}
+
+func TestPlanEntriesFromTodosResult_IgnoresNonArrayTodos(t *testing.T) {
+	if entries, ok := planEntriesFromTodosResult(map[string]any{
+		"todos": map[string]any{"status": "pending"},
+	}); ok || entries != nil {
+		t.Fatalf("planEntriesFromTodosResult = (%+v, %v), want nil false (non-array todos should be ignored)", entries, ok)
+	}
+}