diff --git a/pkg/tools/builtin/todo.go b/pkg/tools/builtin/todo.go index 380435d7f..0cf1b55fb 100644 --- a/pkg/tools/builtin/todo.go +++ b/pkg/tools/builtin/todo.go @@ -2,8 +2,8 @@ package builtin import ( "context" + "encoding/json" "fmt" - "strings" "sync" "sync/atomic" @@ -31,7 +31,7 @@ var ( type Todo struct { ID string `json:"id" jsonschema:"ID of the todo item"` Description string `json:"description" jsonschema:"Description of the todo item"` - Status string `json:"status" jsonschema:"New status (pending, in-progress,completed)"` + Status string `json:"status" jsonschema:"Status of the todo item (pending, in-progress, completed)"` } type CreateTodoArgs struct { @@ -44,13 +44,28 @@ type CreateTodosArgs struct { type TodoUpdate struct { ID string `json:"id" jsonschema:"ID of the todo item"` - Status string `json:"status" jsonschema:"New status (pending, in-progress,completed)"` + Status string `json:"status" jsonschema:"New status (pending, in-progress, completed)"` } type UpdateTodosArgs struct { Updates []TodoUpdate `json:"updates" jsonschema:"List of todo updates"` } +// Output types for JSON-structured responses. + +type CreateTodosOutput struct { + Created []Todo `json:"created" jsonschema:"List of created todo items"` +} + +type UpdateTodosOutput struct { + Updated []TodoUpdate `json:"updated,omitempty" jsonschema:"List of successfully updated todos"` + NotFound []string `json:"not_found,omitempty" jsonschema:"IDs of todos that were not found"` +} + +type ListTodosOutput struct { + Todos []Todo `json:"todos" jsonschema:"List of all current todo items"` +} + // TodoStorage defines the storage layer for todo items. type TodoStorage interface { // Add appends a new todo item. @@ -157,55 +172,48 @@ IMPORTANT: You MUST use these tools to track the progress of your tasks: This toolset is REQUIRED for maintaining task state and ensuring all steps are completed.` } -func (h *todoHandler) createTodo(_ context.Context, params CreateTodoArgs) (*tools.ToolCallResult, error) { - id := fmt.Sprintf("todo_%d", h.nextID.Add(1)) +// addTodo creates a new todo and adds it to storage. +func (h *todoHandler) addTodo(description string) Todo { todo := Todo{ - ID: id, - Description: params.Description, + ID: fmt.Sprintf("todo_%d", h.nextID.Add(1)), + Description: description, Status: "pending", } h.storage.Add(todo) + return todo +} +// jsonResult builds a ToolCallResult with a JSON-serialized output and current storage as Meta. +func (h *todoHandler) jsonResult(v any) (*tools.ToolCallResult, error) { + out, err := json.Marshal(v) + if err != nil { + return nil, fmt.Errorf("marshaling todo output: %w", err) + } return &tools.ToolCallResult{ - Output: fmt.Sprintf("Created todo [%s]: %s", id, params.Description), + Output: string(out), Meta: h.storage.All(), }, nil } -func (h *todoHandler) createTodos(_ context.Context, params CreateTodosArgs) (*tools.ToolCallResult, error) { - ids := make([]int64, len(params.Descriptions)) - for i, desc := range params.Descriptions { - ids[i] = h.nextID.Add(1) - h.storage.Add(Todo{ - ID: fmt.Sprintf("todo_%d", ids[i]), - Description: desc, - Status: "pending", - }) - } +func (h *todoHandler) createTodo(_ context.Context, params CreateTodoArgs) (*tools.ToolCallResult, error) { + return h.jsonResult(h.addTodo(params.Description)) +} - var output strings.Builder - fmt.Fprintf(&output, "Created %d todos: ", len(params.Descriptions)) - for i := range params.Descriptions { - if i > 0 { - output.WriteString(", ") - } - fmt.Fprintf(&output, "[todo_%d]", ids[i]) +func (h *todoHandler) createTodos(_ context.Context, params CreateTodosArgs) (*tools.ToolCallResult, error) { + created := make([]Todo, 0, len(params.Descriptions)) + for _, desc := range params.Descriptions { + created = append(created, h.addTodo(desc)) } - - return &tools.ToolCallResult{ - Output: output.String(), - Meta: h.storage.All(), - }, nil + return h.jsonResult(CreateTodosOutput{Created: created}) } func (h *todoHandler) updateTodos(_ context.Context, params UpdateTodosArgs) (*tools.ToolCallResult, error) { - var notFound []string - var updated []string + result := UpdateTodosOutput{} for _, update := range params.Updates { idx := h.storage.FindByID(update.ID) if idx == -1 { - notFound = append(notFound, update.ID) + result.NotFound = append(result.NotFound, update.ID) continue } @@ -213,32 +221,23 @@ func (h *todoHandler) updateTodos(_ context.Context, params UpdateTodosArgs) (*t t.Status = update.Status return t }) - updated = append(updated, fmt.Sprintf("%s -> %s", update.ID, update.Status)) + result.Updated = append(result.Updated, update) } - var output strings.Builder - if len(updated) > 0 { - fmt.Fprintf(&output, "Updated %d todos: %s", len(updated), strings.Join(updated, ", ")) - } - if len(notFound) > 0 { - if output.Len() > 0 { - output.WriteString("; ") + if len(result.NotFound) > 0 && len(result.Updated) == 0 { + res, err := h.jsonResult(result) + if err != nil { + return nil, err } - fmt.Fprintf(&output, "Not found: %s", strings.Join(notFound, ", ")) - } - - if len(notFound) > 0 && len(updated) == 0 { - return tools.ResultError(output.String()), nil + res.IsError = true + return res, nil } if h.allCompleted() { h.storage.Clear() } - return &tools.ToolCallResult{ - Output: output.String(), - Meta: h.storage.All(), - }, nil + return h.jsonResult(result) } func (h *todoHandler) allCompleted() bool { @@ -255,17 +254,11 @@ func (h *todoHandler) allCompleted() bool { } func (h *todoHandler) listTodos(_ context.Context, _ tools.ToolCall) (*tools.ToolCallResult, error) { - var output strings.Builder - output.WriteString("Current todos:\n") - - for _, todo := range h.storage.All() { - fmt.Fprintf(&output, "- [%s] %s (Status: %s)\n", todo.ID, todo.Description, todo.Status) + todos := h.storage.All() + if todos == nil { + todos = []Todo{} } - - return &tools.ToolCallResult{ - Output: output.String(), - Meta: h.storage.All(), - }, nil + return h.jsonResult(ListTodosOutput{Todos: todos}) } func (t *TodoTool) Tools(context.Context) ([]tools.Tool, error) { @@ -275,7 +268,7 @@ func (t *TodoTool) Tools(context.Context) ([]tools.Tool, error) { Category: "todo", Description: "Create a new todo item with a description", Parameters: tools.MustSchemaFor[CreateTodoArgs](), - OutputSchema: tools.MustSchemaFor[string](), + OutputSchema: tools.MustSchemaFor[Todo](), Handler: tools.NewHandler(t.handler.createTodo), Annotations: tools.ToolAnnotations{ Title: "Create TODO", @@ -287,7 +280,7 @@ func (t *TodoTool) Tools(context.Context) ([]tools.Tool, error) { Category: "todo", Description: "Create a list of new todo items with descriptions", Parameters: tools.MustSchemaFor[CreateTodosArgs](), - OutputSchema: tools.MustSchemaFor[string](), + OutputSchema: tools.MustSchemaFor[CreateTodosOutput](), Handler: tools.NewHandler(t.handler.createTodos), Annotations: tools.ToolAnnotations{ Title: "Create TODOs", @@ -299,7 +292,7 @@ func (t *TodoTool) Tools(context.Context) ([]tools.Tool, error) { Category: "todo", Description: "Update the status of one or more todo item(s)", Parameters: tools.MustSchemaFor[UpdateTodosArgs](), - OutputSchema: tools.MustSchemaFor[string](), + OutputSchema: tools.MustSchemaFor[UpdateTodosOutput](), Handler: tools.NewHandler(t.handler.updateTodos), Annotations: tools.ToolAnnotations{ Title: "Update TODOs", @@ -310,7 +303,7 @@ func (t *TodoTool) Tools(context.Context) ([]tools.Tool, error) { Name: ToolNameListTodos, Category: "todo", Description: "List all current todos with their status", - OutputSchema: tools.MustSchemaFor[string](), + OutputSchema: tools.MustSchemaFor[ListTodosOutput](), Handler: t.handler.listTodos, Annotations: tools.ToolAnnotations{ Title: "List TODOs", diff --git a/pkg/tools/builtin/todo_test.go b/pkg/tools/builtin/todo_test.go index 6f2c55c38..6b72f7cf5 100644 --- a/pkg/tools/builtin/todo_test.go +++ b/pkg/tools/builtin/todo_test.go @@ -1,6 +1,7 @@ package builtin import ( + "encoding/json" "testing" "github.com/stretchr/testify/assert" @@ -28,24 +29,16 @@ func TestTodoTool_CreateTodo(t *testing.T) { result, err := tool.handler.createTodo(t.Context(), CreateTodoArgs{ Description: "Test todo item", }) - require.NoError(t, err) - assert.Contains(t, result.Output, "Created todo [todo_1]: Test todo item") - assert.Equal(t, 1, storage.Len()) - todos := storage.All() - require.Len(t, todos, 1) - assert.Equal(t, "todo_1", todos[0].ID) - assert.Equal(t, "Test todo item", todos[0].Description) - assert.Equal(t, "pending", todos[0].Status) + var output Todo + require.NoError(t, json.Unmarshal([]byte(result.Output), &output)) + assert.Equal(t, "todo_1", output.ID) + assert.Equal(t, "Test todo item", output.Description) + assert.Equal(t, "pending", output.Status) - // Verify Meta contains the created todo - metaTodos, ok := result.Meta.([]Todo) - require.True(t, ok, "Meta should be []Todo") - require.Len(t, metaTodos, 1) - assert.Equal(t, "todo_1", metaTodos[0].ID) - assert.Equal(t, "Test todo item", metaTodos[0].Description) - assert.Equal(t, "pending", metaTodos[0].Status) + require.Equal(t, 1, storage.Len()) + requireMeta(t, result, 1) } func TestTodoTool_CreateTodos(t *testing.T) { @@ -53,95 +46,67 @@ func TestTodoTool_CreateTodos(t *testing.T) { tool := NewTodoTool(WithStorage(storage)) result, err := tool.handler.createTodos(t.Context(), CreateTodosArgs{ - Descriptions: []string{ - "First todo item", - "Second todo item", - "Third todo item", - }, + Descriptions: []string{"First", "Second", "Third"}, }) - require.NoError(t, err) - assert.Contains(t, result.Output, "Created 3 todos:") - assert.Contains(t, result.Output, "todo_1") - assert.Contains(t, result.Output, "todo_2") - assert.Contains(t, result.Output, "todo_3") - assert.Equal(t, 3, storage.Len()) + var output CreateTodosOutput + require.NoError(t, json.Unmarshal([]byte(result.Output), &output)) + require.Len(t, output.Created, 3) + assert.Equal(t, "todo_1", output.Created[0].ID) + assert.Equal(t, "First", output.Created[0].Description) + assert.Equal(t, "pending", output.Created[0].Status) + assert.Equal(t, "todo_2", output.Created[1].ID) + assert.Equal(t, "todo_3", output.Created[2].ID) - // Verify Meta contains all todos (order not guaranteed from map) - metaTodos, ok := result.Meta.([]Todo) - require.True(t, ok, "Meta should be []Todo") - require.Len(t, metaTodos, 3) - - // Verify order is preserved - assert.Equal(t, "First todo item", metaTodos[0].Description) - assert.Equal(t, "Second todo item", metaTodos[1].Description) - assert.Equal(t, "Third todo item", metaTodos[2].Description) + assert.Equal(t, 3, storage.Len()) + requireMeta(t, result, 3) + // A second call continues the ID sequence result, err = tool.handler.createTodos(t.Context(), CreateTodosArgs{ - Descriptions: []string{ - "Last todo item", - }, + Descriptions: []string{"Last"}, }) - require.NoError(t, err) - assert.Contains(t, result.Output, "Created 1 todos:") - assert.Contains(t, result.Output, "todo_4") - assert.Equal(t, 4, storage.Len()) - // Verify Meta for second call contains all 4 todos - metaTodos, ok = result.Meta.([]Todo) - require.True(t, ok, "Meta should be []Todo") - require.Len(t, metaTodos, 4) + require.NoError(t, json.Unmarshal([]byte(result.Output), &output)) + require.Len(t, output.Created, 1) + assert.Equal(t, "todo_4", output.Created[0].ID) + assert.Equal(t, 4, storage.Len()) + requireMeta(t, result, 4) } func TestTodoTool_ListTodos(t *testing.T) { tool := NewTodoTool() - todos := []string{ - "First todo item", - "Second todo item", - "Third todo item", - } - - for _, todoDesc := range todos { - _, err := tool.handler.createTodo(t.Context(), CreateTodoArgs{ - Description: todoDesc, - }) - + descs := []string{"First", "Second", "Third"} + for _, d := range descs { + _, err := tool.handler.createTodo(t.Context(), CreateTodoArgs{Description: d}) require.NoError(t, err) } result, err := tool.handler.listTodos(t.Context(), tools.ToolCall{}) - require.NoError(t, err) - assert.Contains(t, result.Output, "Current todos:") - for _, todoDesc := range todos { - assert.Contains(t, result.Output, todoDesc) - assert.Contains(t, result.Output, "Status: pending") + + var output ListTodosOutput + require.NoError(t, json.Unmarshal([]byte(result.Output), &output)) + require.Len(t, output.Todos, 3) + for i, d := range descs { + assert.Equal(t, d, output.Todos[i].Description) + assert.Equal(t, "pending", output.Todos[i].Status) } - // Verify Meta contains all todos - metaTodos, ok := result.Meta.([]Todo) - require.True(t, ok, "Meta should be []Todo") - require.Len(t, metaTodos, 3) + requireMeta(t, result, 3) } func TestTodoTool_UpdateTodos(t *testing.T) { storage := NewMemoryTodoStorage() tool := NewTodoTool(WithStorage(storage)) - // Create multiple todos first _, err := tool.handler.createTodos(t.Context(), CreateTodosArgs{ - Descriptions: []string{ - "First todo item", - "Second todo item", - "Third todo item", - }, + Descriptions: []string{"First", "Second", "Third"}, }) require.NoError(t, err) - // Update multiple todos in one call result, err := tool.handler.updateTodos(t.Context(), UpdateTodosArgs{ Updates: []TodoUpdate{ {ID: "todo_1", Status: "completed"}, @@ -150,34 +115,34 @@ func TestTodoTool_UpdateTodos(t *testing.T) { }) require.NoError(t, err) assert.False(t, result.IsError) - assert.Contains(t, result.Output, "Updated 2 todos") - assert.Contains(t, result.Output, "todo_1 -> completed") - assert.Contains(t, result.Output, "todo_3 -> in-progress") - // Verify the todos were updated + var output UpdateTodosOutput + require.NoError(t, json.Unmarshal([]byte(result.Output), &output)) + require.Len(t, output.Updated, 2) + assert.Equal(t, "todo_1", output.Updated[0].ID) + assert.Equal(t, "completed", output.Updated[0].Status) + assert.Equal(t, "todo_3", output.Updated[1].ID) + assert.Equal(t, "in-progress", output.Updated[1].Status) + assert.Empty(t, output.NotFound) + todos := storage.All() require.Len(t, todos, 3) assert.Equal(t, "completed", todos[0].Status) assert.Equal(t, "pending", todos[1].Status) assert.Equal(t, "in-progress", todos[2].Status) - // Verify Meta contains all todos with updated status - metaTodos, ok := result.Meta.([]Todo) - require.True(t, ok, "Meta should be []Todo") - require.Len(t, metaTodos, 3) + requireMeta(t, result, 3) } func TestTodoTool_UpdateTodos_PartialFailure(t *testing.T) { storage := NewMemoryTodoStorage() tool := NewTodoTool(WithStorage(storage)) - // Create two todos so we can complete one without clearing the list _, err := tool.handler.createTodos(t.Context(), CreateTodosArgs{ - Descriptions: []string{"First todo item", "Second todo item"}, + Descriptions: []string{"First", "Second"}, }) require.NoError(t, err) - // Try to update one existing and one non-existing todo result, err := tool.handler.updateTodos(t.Context(), UpdateTodosArgs{ Updates: []TodoUpdate{ {ID: "todo_1", Status: "completed"}, @@ -185,11 +150,15 @@ func TestTodoTool_UpdateTodos_PartialFailure(t *testing.T) { }, }) require.NoError(t, err) - assert.False(t, result.IsError) // Not an error because at least one succeeded - assert.Contains(t, result.Output, "Updated 1 todos") - assert.Contains(t, result.Output, "Not found: nonexistent") + assert.False(t, result.IsError) + + var output UpdateTodosOutput + require.NoError(t, json.Unmarshal([]byte(result.Output), &output)) + require.Len(t, output.Updated, 1) + assert.Equal(t, "todo_1", output.Updated[0].ID) + require.Len(t, output.NotFound, 1) + assert.Equal(t, "nonexistent", output.NotFound[0]) - // Verify the existing todo was updated (list not cleared because todo_2 still pending) todos := storage.All() require.Len(t, todos, 2) assert.Equal(t, "completed", todos[0].Status) @@ -199,7 +168,6 @@ func TestTodoTool_UpdateTodos_PartialFailure(t *testing.T) { func TestTodoTool_UpdateTodos_AllNotFound(t *testing.T) { tool := NewTodoTool() - // Try to update non-existing todos result, err := tool.handler.updateTodos(t.Context(), UpdateTodosArgs{ Updates: []TodoUpdate{ {ID: "nonexistent1", Status: "completed"}, @@ -207,21 +175,25 @@ func TestTodoTool_UpdateTodos_AllNotFound(t *testing.T) { }, }) require.NoError(t, err) - assert.True(t, result.IsError) // Error because all failed - assert.Contains(t, result.Output, "Not found: nonexistent1, nonexistent2") + assert.True(t, result.IsError) + + var output UpdateTodosOutput + require.NoError(t, json.Unmarshal([]byte(result.Output), &output)) + assert.Empty(t, output.Updated) + require.Len(t, output.NotFound, 2) + assert.Equal(t, "nonexistent1", output.NotFound[0]) + assert.Equal(t, "nonexistent2", output.NotFound[1]) } func TestTodoTool_UpdateTodos_ClearsWhenAllCompleted(t *testing.T) { storage := NewMemoryTodoStorage() tool := NewTodoTool(WithStorage(storage)) - // Create multiple todos _, err := tool.handler.createTodos(t.Context(), CreateTodosArgs{ - Descriptions: []string{"First todo item", "Second todo item"}, + Descriptions: []string{"First", "Second"}, }) require.NoError(t, err) - // Complete all todos result, err := tool.handler.updateTodos(t.Context(), UpdateTodosArgs{ Updates: []TodoUpdate{ {ID: "todo_1", Status: "completed"}, @@ -229,28 +201,22 @@ func TestTodoTool_UpdateTodos_ClearsWhenAllCompleted(t *testing.T) { }, }) require.NoError(t, err) - assert.Contains(t, result.Output, "Updated 2 todos") - // Verify all todos were cleared - todos := storage.All() - assert.Empty(t, todos) + var output UpdateTodosOutput + require.NoError(t, json.Unmarshal([]byte(result.Output), &output)) + require.Len(t, output.Updated, 2) - // Verify Meta is also empty - metaTodos, ok := result.Meta.([]Todo) - require.True(t, ok, "Meta should be []Todo") - assert.Empty(t, metaTodos) + assert.Empty(t, storage.All()) + requireMeta(t, result, 0) } func TestTodoTool_WithStorage(t *testing.T) { storage := NewMemoryTodoStorage() tool := NewTodoTool(WithStorage(storage)) - _, err := tool.handler.createTodo(t.Context(), CreateTodoArgs{ - Description: "Test item", - }) + _, err := tool.handler.createTodo(t.Context(), CreateTodoArgs{Description: "Test item"}) require.NoError(t, err) - // Verify the custom storage received the item assert.Equal(t, 1, storage.Len()) assert.Equal(t, "Test item", storage.All()[0].Description) } @@ -287,3 +253,11 @@ func TestTodoTool_ParametersAreObjects(t *testing.T) { assert.Equal(t, "object", m["type"]) } } + +// requireMeta asserts that result.Meta is a []Todo of the expected length. +func requireMeta(t *testing.T, result *tools.ToolCallResult, expectedLen int) { + t.Helper() + metaTodos, ok := result.Meta.([]Todo) + require.True(t, ok, "Meta should be []Todo") + require.Len(t, metaTodos, expectedLen) +}