feat: Add todo validation for attempt completion

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")
+	})
+})

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",
+		)
+	})
+})

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

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