diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts new file mode 100644 index 0000000000..5b43cf9022 --- /dev/null +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -0,0 +1,184 @@ +import { describe, test, expect, vi } from "vitest" +import { attemptCompletionTool } from "../attemptCompletionTool" +import { Task } from "../../task/Task" +import { TodoItem } from "@roo-code/types" +import { ToolUse } from "../../../shared/tools" + +// Mock dependencies +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureTaskCompleted: vi.fn(), + }, + }, +})) + +vi.mock("../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((message) => message), + }, +})) + +describe("attemptCompletionTool", () => { + const createMockTask = (todoList?: TodoItem[]): Task => { + const task = { + todoList, + consecutiveMistakeCount: 0, + clineMessages: [], + recordToolError: vi.fn(), + sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"), + say: vi.fn(), + emit: vi.fn(), + getTokenUsage: vi.fn().mockReturnValue({}), + toolUsage: {}, + parentTask: null, + ask: vi.fn(), + userMessageContent: [], + } as unknown as Task + return task + } + + const createMockToolUse = (result?: string, partial = false): ToolUse => ({ + type: "tool_use", + name: "attempt_completion", + params: { result }, + partial, + }) + + const mockFunctions = { + askApproval: vi.fn(), + handleError: vi.fn(), + pushToolResult: vi.fn(), + removeClosingTag: vi.fn((tag, text) => text || ""), + toolDescription: vi.fn(() => "[attempt_completion]"), + askFinishSubTaskApproval: vi.fn(), + } + + test("should block completion when there are pending todos", async () => { + const todoList: TodoItem[] = [ + { id: "1", content: "Complete task 1", status: "completed" }, + { id: "2", content: "Complete task 2", status: "pending" }, + ] + const task = createMockTask(todoList) + const toolUse = createMockToolUse("Task completed successfully") + + await attemptCompletionTool( + task, + toolUse, + mockFunctions.askApproval, + mockFunctions.handleError, + mockFunctions.pushToolResult, + mockFunctions.removeClosingTag, + mockFunctions.toolDescription, + mockFunctions.askFinishSubTaskApproval, + ) + + expect(task.consecutiveMistakeCount).toBe(1) + expect(task.recordToolError).toHaveBeenCalledWith("attempt_completion") + expect(mockFunctions.pushToolResult).toHaveBeenCalledWith( + expect.stringContaining("Cannot attempt completion while there are incomplete todos"), + ) + expect(mockFunctions.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("Pending todos:")) + expect(mockFunctions.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("- [ ] Complete task 2")) + }) + + test("should block completion when there are in_progress todos", async () => { + const todoList: TodoItem[] = [ + { id: "1", content: "Complete task 1", status: "completed" }, + { id: "2", content: "Complete task 2", status: "in_progress" }, + ] + const task = createMockTask(todoList) + const toolUse = createMockToolUse("Task completed successfully") + + await attemptCompletionTool( + task, + toolUse, + mockFunctions.askApproval, + mockFunctions.handleError, + mockFunctions.pushToolResult, + mockFunctions.removeClosingTag, + mockFunctions.toolDescription, + mockFunctions.askFinishSubTaskApproval, + ) + + expect(task.consecutiveMistakeCount).toBe(1) + expect(task.recordToolError).toHaveBeenCalledWith("attempt_completion") + expect(mockFunctions.pushToolResult).toHaveBeenCalledWith( + expect.stringContaining("Cannot attempt completion while there are incomplete todos"), + ) + expect(mockFunctions.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("In Progress todos:")) + expect(mockFunctions.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("- [-] Complete task 2")) + }) + + test("should allow completion when all todos are completed", async () => { + const todoList: TodoItem[] = [ + { id: "1", content: "Complete task 1", status: "completed" }, + { id: "2", content: "Complete task 2", status: "completed" }, + ] + const task = createMockTask(todoList) + const toolUse = createMockToolUse("Task completed successfully") + + // Mock the ask method to return yesButtonClicked for completion + task.ask = vi.fn().mockResolvedValue({ response: "yesButtonClicked" }) + + await attemptCompletionTool( + task, + toolUse, + mockFunctions.askApproval, + mockFunctions.handleError, + mockFunctions.pushToolResult, + mockFunctions.removeClosingTag, + mockFunctions.toolDescription, + mockFunctions.askFinishSubTaskApproval, + ) + + expect(task.consecutiveMistakeCount).toBe(0) + expect(task.recordToolError).not.toHaveBeenCalled() + expect(task.say).toHaveBeenCalledWith("completion_result", "Task completed successfully", undefined, false) + expect(mockFunctions.pushToolResult).toHaveBeenCalledWith("") + }) + + test("should allow completion when no todos exist", async () => { + const task = createMockTask() // No todos + const toolUse = createMockToolUse("Task completed successfully") + + // Mock the ask method to return yesButtonClicked for completion + task.ask = vi.fn().mockResolvedValue({ response: "yesButtonClicked" }) + + await attemptCompletionTool( + task, + toolUse, + mockFunctions.askApproval, + mockFunctions.handleError, + mockFunctions.pushToolResult, + mockFunctions.removeClosingTag, + mockFunctions.toolDescription, + mockFunctions.askFinishSubTaskApproval, + ) + + expect(task.consecutiveMistakeCount).toBe(0) + expect(task.recordToolError).not.toHaveBeenCalled() + expect(task.say).toHaveBeenCalledWith("completion_result", "Task completed successfully", undefined, false) + expect(mockFunctions.pushToolResult).toHaveBeenCalledWith("") + }) + + test("should still block completion for missing result parameter", async () => { + const task = createMockTask() + const toolUse = createMockToolUse() // No result parameter + + await attemptCompletionTool( + task, + toolUse, + mockFunctions.askApproval, + mockFunctions.handleError, + mockFunctions.pushToolResult, + mockFunctions.removeClosingTag, + mockFunctions.toolDescription, + mockFunctions.askFinishSubTaskApproval, + ) + + expect(task.consecutiveMistakeCount).toBe(1) + expect(task.recordToolError).toHaveBeenCalledWith("attempt_completion") + expect(task.sayAndCreateMissingParamError).toHaveBeenCalledWith("attempt_completion", "result") + }) +}) diff --git a/src/core/tools/__tests__/updateTodoListTool.spec.ts b/src/core/tools/__tests__/updateTodoListTool.spec.ts new file mode 100644 index 0000000000..cd74e078bf --- /dev/null +++ b/src/core/tools/__tests__/updateTodoListTool.spec.ts @@ -0,0 +1,86 @@ +import { describe, test, expect } from "vitest" +import { validateTodosForCompletion } from "../updateTodoListTool" +import { TodoItem } from "@roo-code/types" + +describe("validateTodosForCompletion", () => { + test("should allow completion when no todos exist", () => { + const result = validateTodosForCompletion(undefined) + expect(result.valid).toBe(true) + expect(result.error).toBeUndefined() + }) + + test("should allow completion when empty todo list", () => { + const result = validateTodosForCompletion([]) + expect(result.valid).toBe(true) + expect(result.error).toBeUndefined() + }) + + test("should allow completion when all todos are completed", () => { + const todos: TodoItem[] = [ + { id: "1", content: "Task 1", status: "completed" }, + { id: "2", content: "Task 2", status: "completed" }, + ] + const result = validateTodosForCompletion(todos) + expect(result.valid).toBe(true) + expect(result.error).toBeUndefined() + }) + + test("should block completion when there are pending todos", () => { + const todos: TodoItem[] = [ + { id: "1", content: "Task 1", status: "completed" }, + { id: "2", content: "Task 2", status: "pending" }, + ] + const result = validateTodosForCompletion(todos) + expect(result.valid).toBe(false) + expect(result.error).toContain("Cannot attempt completion while there are incomplete todos") + expect(result.error).toContain("Pending todos:") + expect(result.error).toContain("- [ ] Task 2") + expect(result.incompleteTodos?.pending).toHaveLength(1) + expect(result.incompleteTodos?.pending[0].content).toBe("Task 2") + }) + + test("should block completion when there are in_progress todos", () => { + const todos: TodoItem[] = [ + { id: "1", content: "Task 1", status: "completed" }, + { id: "2", content: "Task 2", status: "in_progress" }, + ] + const result = validateTodosForCompletion(todos) + expect(result.valid).toBe(false) + expect(result.error).toContain("Cannot attempt completion while there are incomplete todos") + expect(result.error).toContain("In Progress todos:") + expect(result.error).toContain("- [-] Task 2") + expect(result.incompleteTodos?.inProgress).toHaveLength(1) + expect(result.incompleteTodos?.inProgress[0].content).toBe("Task 2") + }) + + test("should block completion when there are both pending and in_progress todos", () => { + const todos: TodoItem[] = [ + { id: "1", content: "Task 1", status: "completed" }, + { id: "2", content: "Task 2", status: "pending" }, + { id: "3", content: "Task 3", status: "in_progress" }, + { id: "4", content: "Task 4", status: "pending" }, + ] + const result = validateTodosForCompletion(todos) + expect(result.valid).toBe(false) + expect(result.error).toContain("Cannot attempt completion while there are incomplete todos") + expect(result.error).toContain("Pending todos:") + expect(result.error).toContain("- [ ] Task 2") + expect(result.error).toContain("- [ ] Task 4") + expect(result.error).toContain("In Progress todos:") + expect(result.error).toContain("- [-] Task 3") + expect(result.error).toContain( + "Please complete all todos using the update_todo_list tool before attempting completion", + ) + expect(result.incompleteTodos?.pending).toHaveLength(2) + expect(result.incompleteTodos?.inProgress).toHaveLength(1) + }) + + test("should provide helpful error message with update_todo_list tool reference", () => { + const todos: TodoItem[] = [{ id: "1", content: "Incomplete task", status: "pending" }] + const result = validateTodosForCompletion(todos) + expect(result.valid).toBe(false) + expect(result.error).toContain( + "Please complete all todos using the update_todo_list tool before attempting completion", + ) + }) +}) diff --git a/src/core/tools/attemptCompletionTool.ts b/src/core/tools/attemptCompletionTool.ts index 57f5870022..0898912e6d 100644 --- a/src/core/tools/attemptCompletionTool.ts +++ b/src/core/tools/attemptCompletionTool.ts @@ -14,6 +14,7 @@ import { AskFinishSubTaskApproval, } from "../../shared/tools" import { formatResponse } from "../prompts/responses" +import { validateTodosForCompletion } from "./updateTodoListTool" export async function attemptCompletionTool( cline: Task, @@ -63,6 +64,17 @@ export async function attemptCompletionTool( return } + // Validate that all todos are completed before allowing completion + const todoValidation = validateTodosForCompletion(cline.todoList) + if (!todoValidation.valid) { + cline.consecutiveMistakeCount++ + cline.recordToolError("attempt_completion") + pushToolResult( + formatResponse.toolError(todoValidation.error || "Cannot complete task with incomplete todos"), + ) + return + } + cline.consecutiveMistakeCount = 0 // Command execution is permanently disabled in attempt_completion diff --git a/src/core/tools/updateTodoListTool.ts b/src/core/tools/updateTodoListTool.ts index cbb90338d3..dc3b05b591 100644 --- a/src/core/tools/updateTodoListTool.ts +++ b/src/core/tools/updateTodoListTool.ts @@ -143,6 +143,57 @@ function validateTodos(todos: any[]): { valid: boolean; error?: string } { return { valid: true } } +/** + * Validate that all todos are completed before allowing task completion. + * @param todoList Array of TodoItem objects to validate + * @returns Validation result with error details if incomplete todos exist + */ +export function validateTodosForCompletion(todoList?: TodoItem[]): { + valid: boolean + error?: string + incompleteTodos?: { pending: TodoItem[]; inProgress: TodoItem[] } +} { + // If no todos exist, completion is allowed + if (!todoList || todoList.length === 0) { + return { valid: true } + } + + const pendingTodos = todoList.filter((todo) => todo.status === "pending") + const inProgressTodos = todoList.filter((todo) => todo.status === "in_progress") + + // If there are any incomplete todos, block completion + if (pendingTodos.length > 0 || inProgressTodos.length > 0) { + let errorMessage = "Cannot attempt completion while there are incomplete todos:\n\n" + + if (pendingTodos.length > 0) { + errorMessage += "Pending todos:\n" + pendingTodos.forEach((todo) => { + errorMessage += `- [ ] ${todo.content}\n` + }) + errorMessage += "\n" + } + + if (inProgressTodos.length > 0) { + errorMessage += "In Progress todos:\n" + inProgressTodos.forEach((todo) => { + errorMessage += `- [-] ${todo.content}\n` + }) + errorMessage += "\n" + } + + errorMessage += "Please complete all todos using the update_todo_list tool before attempting completion." + + return { + valid: false, + error: errorMessage, + incompleteTodos: { pending: pendingTodos, inProgress: inProgressTodos }, + } + } + + // All todos are completed + return { valid: true } +} + /** * Update the todo list for a task. * @param cline Task instance