Skip to content

Commit 97987f3

Browse files
roomoteroomote
committed
feat: add todo validation for attempt completion
- Add validateTodosForCompletion function to check for incomplete todos - Modify attemptCompletionTool to validate todos before allowing completion - Block completion when todos have 'pending' or 'in_progress' status - Provide detailed error messages showing which todos are incomplete - Add comprehensive test coverage for validation logic - Maintain existing functionality when no todos exist Resolves Slack request to prevent completion with incomplete todos
1 parent d4abe73 commit 97987f3

File tree

4 files changed

+333
-0
lines changed

4 files changed

+333
-0
lines changed

‎src/core/tools/__tests__/attemptCompletionTool.spec.ts‎

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
import { describe, test, expect, vi } from "vitest"
2+
import { attemptCompletionTool } from "../attemptCompletionTool"
3+
import { Task } from "../../task/Task"
4+
import { TodoItem } from "@roo-code/types"
5+
import { ToolUse } from "../../../shared/tools"
6+
7+
// Mock dependencies
8+
vi.mock("@roo-code/telemetry", () => ({
9+
TelemetryService: {
10+
instance: {
11+
captureTaskCompleted: vi.fn(),
12+
},
13+
},
14+
}))
15+
16+
vi.mock("../prompts/responses", () => ({
17+
formatResponse: {
18+
toolError: vi.fn((message) => message),
19+
},
20+
}))
21+
22+
describe("attemptCompletionTool", () => {
23+
const createMockTask = (todoList?: TodoItem[]): Task => {
24+
const task = {
25+
todoList,
26+
consecutiveMistakeCount: 0,
27+
clineMessages: [],
28+
recordToolError: vi.fn(),
29+
sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"),
30+
say: vi.fn(),
31+
emit: vi.fn(),
32+
getTokenUsage: vi.fn().mockReturnValue({}),
33+
toolUsage: {},
34+
parentTask: null,
35+
ask: vi.fn(),
36+
userMessageContent: [],
37+
} as unknown as Task
38+
return task
39+
}
40+
41+
const createMockToolUse = (result?: string, partial = false): ToolUse => ({
42+
type: "tool_use",
43+
name: "attempt_completion",
44+
params: { result },
45+
partial,
46+
})
47+
48+
const mockFunctions = {
49+
askApproval: vi.fn(),
50+
handleError: vi.fn(),
51+
pushToolResult: vi.fn(),
52+
removeClosingTag: vi.fn((tag, text) => text || ""),
53+
toolDescription: vi.fn(() => "[attempt_completion]"),
54+
askFinishSubTaskApproval: vi.fn(),
55+
}
56+
57+
test("should block completion when there are pending todos", async () => {
58+
const todoList: TodoItem[] = [
59+
{ id: "1", content: "Complete task 1", status: "completed" },
60+
{ id: "2", content: "Complete task 2", status: "pending" },
61+
]
62+
const task = createMockTask(todoList)
63+
const toolUse = createMockToolUse("Task completed successfully")
64+
65+
await attemptCompletionTool(
66+
task,
67+
toolUse,
68+
mockFunctions.askApproval,
69+
mockFunctions.handleError,
70+
mockFunctions.pushToolResult,
71+
mockFunctions.removeClosingTag,
72+
mockFunctions.toolDescription,
73+
mockFunctions.askFinishSubTaskApproval,
74+
)
75+
76+
expect(task.consecutiveMistakeCount).toBe(1)
77+
expect(task.recordToolError).toHaveBeenCalledWith("attempt_completion")
78+
expect(mockFunctions.pushToolResult).toHaveBeenCalledWith(
79+
expect.stringContaining("Cannot attempt completion while there are incomplete todos"),
80+
)
81+
expect(mockFunctions.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("Pending todos:"))
82+
expect(mockFunctions.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("- [ ] Complete task 2"))
83+
})
84+
85+
test("should block completion when there are in_progress todos", async () => {
86+
const todoList: TodoItem[] = [
87+
{ id: "1", content: "Complete task 1", status: "completed" },
88+
{ id: "2", content: "Complete task 2", status: "in_progress" },
89+
]
90+
const task = createMockTask(todoList)
91+
const toolUse = createMockToolUse("Task completed successfully")
92+
93+
await attemptCompletionTool(
94+
task,
95+
toolUse,
96+
mockFunctions.askApproval,
97+
mockFunctions.handleError,
98+
mockFunctions.pushToolResult,
99+
mockFunctions.removeClosingTag,
100+
mockFunctions.toolDescription,
101+
mockFunctions.askFinishSubTaskApproval,
102+
)
103+
104+
expect(task.consecutiveMistakeCount).toBe(1)
105+
expect(task.recordToolError).toHaveBeenCalledWith("attempt_completion")
106+
expect(mockFunctions.pushToolResult).toHaveBeenCalledWith(
107+
expect.stringContaining("Cannot attempt completion while there are incomplete todos"),
108+
)
109+
expect(mockFunctions.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("In Progress todos:"))
110+
expect(mockFunctions.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("- [-] Complete task 2"))
111+
})
112+
113+
test("should allow completion when all todos are completed", async () => {
114+
const todoList: TodoItem[] = [
115+
{ id: "1", content: "Complete task 1", status: "completed" },
116+
{ id: "2", content: "Complete task 2", status: "completed" },
117+
]
118+
const task = createMockTask(todoList)
119+
const toolUse = createMockToolUse("Task completed successfully")
120+
121+
// Mock the ask method to return yesButtonClicked for completion
122+
task.ask = vi.fn().mockResolvedValue({ response: "yesButtonClicked" })
123+
124+
await attemptCompletionTool(
125+
task,
126+
toolUse,
127+
mockFunctions.askApproval,
128+
mockFunctions.handleError,
129+
mockFunctions.pushToolResult,
130+
mockFunctions.removeClosingTag,
131+
mockFunctions.toolDescription,
132+
mockFunctions.askFinishSubTaskApproval,
133+
)
134+
135+
expect(task.consecutiveMistakeCount).toBe(0)
136+
expect(task.recordToolError).not.toHaveBeenCalled()
137+
expect(task.say).toHaveBeenCalledWith("completion_result", "Task completed successfully", undefined, false)
138+
expect(mockFunctions.pushToolResult).toHaveBeenCalledWith("")
139+
})
140+
141+
test("should allow completion when no todos exist", async () => {
142+
const task = createMockTask() // No todos
143+
const toolUse = createMockToolUse("Task completed successfully")
144+
145+
// Mock the ask method to return yesButtonClicked for completion
146+
task.ask = vi.fn().mockResolvedValue({ response: "yesButtonClicked" })
147+
148+
await attemptCompletionTool(
149+
task,
150+
toolUse,
151+
mockFunctions.askApproval,
152+
mockFunctions.handleError,
153+
mockFunctions.pushToolResult,
154+
mockFunctions.removeClosingTag,
155+
mockFunctions.toolDescription,
156+
mockFunctions.askFinishSubTaskApproval,
157+
)
158+
159+
expect(task.consecutiveMistakeCount).toBe(0)
160+
expect(task.recordToolError).not.toHaveBeenCalled()
161+
expect(task.say).toHaveBeenCalledWith("completion_result", "Task completed successfully", undefined, false)
162+
expect(mockFunctions.pushToolResult).toHaveBeenCalledWith("")
163+
})
164+
165+
test("should still block completion for missing result parameter", async () => {
166+
const task = createMockTask()
167+
const toolUse = createMockToolUse() // No result parameter
168+
169+
await attemptCompletionTool(
170+
task,
171+
toolUse,
172+
mockFunctions.askApproval,
173+
mockFunctions.handleError,
174+
mockFunctions.pushToolResult,
175+
mockFunctions.removeClosingTag,
176+
mockFunctions.toolDescription,
177+
mockFunctions.askFinishSubTaskApproval,
178+
)
179+
180+
expect(task.consecutiveMistakeCount).toBe(1)
181+
expect(task.recordToolError).toHaveBeenCalledWith("attempt_completion")
182+
expect(task.sayAndCreateMissingParamError).toHaveBeenCalledWith("attempt_completion", "result")
183+
})
184+
})

‎src/core/tools/__tests__/updateTodoListTool.spec.ts‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
import { describe, test, expect } from "vitest"
2+
import { validateTodosForCompletion } from "../updateTodoListTool"
3+
import { TodoItem } from "@roo-code/types"
4+
5+
describe("validateTodosForCompletion", () => {
6+
test("should allow completion when no todos exist", () => {
7+
const result = validateTodosForCompletion(undefined)
8+
expect(result.valid).toBe(true)
9+
expect(result.error).toBeUndefined()
10+
})
11+
12+
test("should allow completion when empty todo list", () => {
13+
const result = validateTodosForCompletion([])
14+
expect(result.valid).toBe(true)
15+
expect(result.error).toBeUndefined()
16+
})
17+
18+
test("should allow completion when all todos are completed", () => {
19+
const todos: TodoItem[] = [
20+
{ id: "1", content: "Task 1", status: "completed" },
21+
{ id: "2", content: "Task 2", status: "completed" },
22+
]
23+
const result = validateTodosForCompletion(todos)
24+
expect(result.valid).toBe(true)
25+
expect(result.error).toBeUndefined()
26+
})
27+
28+
test("should block completion when there are pending todos", () => {
29+
const todos: TodoItem[] = [
30+
{ id: "1", content: "Task 1", status: "completed" },
31+
{ id: "2", content: "Task 2", status: "pending" },
32+
]
33+
const result = validateTodosForCompletion(todos)
34+
expect(result.valid).toBe(false)
35+
expect(result.error).toContain("Cannot attempt completion while there are incomplete todos")
36+
expect(result.error).toContain("Pending todos:")
37+
expect(result.error).toContain("- [ ] Task 2")
38+
expect(result.incompleteTodos?.pending).toHaveLength(1)
39+
expect(result.incompleteTodos?.pending[0].content).toBe("Task 2")
40+
})
41+
42+
test("should block completion when there are in_progress todos", () => {
43+
const todos: TodoItem[] = [
44+
{ id: "1", content: "Task 1", status: "completed" },
45+
{ id: "2", content: "Task 2", status: "in_progress" },
46+
]
47+
const result = validateTodosForCompletion(todos)
48+
expect(result.valid).toBe(false)
49+
expect(result.error).toContain("Cannot attempt completion while there are incomplete todos")
50+
expect(result.error).toContain("In Progress todos:")
51+
expect(result.error).toContain("- [-] Task 2")
52+
expect(result.incompleteTodos?.inProgress).toHaveLength(1)
53+
expect(result.incompleteTodos?.inProgress[0].content).toBe("Task 2")
54+
})
55+
56+
test("should block completion when there are both pending and in_progress todos", () => {
57+
const todos: TodoItem[] = [
58+
{ id: "1", content: "Task 1", status: "completed" },
59+
{ id: "2", content: "Task 2", status: "pending" },
60+
{ id: "3", content: "Task 3", status: "in_progress" },
61+
{ id: "4", content: "Task 4", status: "pending" },
62+
]
63+
const result = validateTodosForCompletion(todos)
64+
expect(result.valid).toBe(false)
65+
expect(result.error).toContain("Cannot attempt completion while there are incomplete todos")
66+
expect(result.error).toContain("Pending todos:")
67+
expect(result.error).toContain("- [ ] Task 2")
68+
expect(result.error).toContain("- [ ] Task 4")
69+
expect(result.error).toContain("In Progress todos:")
70+
expect(result.error).toContain("- [-] Task 3")
71+
expect(result.error).toContain(
72+
"Please complete all todos using the update_todo_list tool before attempting completion",
73+
)
74+
expect(result.incompleteTodos?.pending).toHaveLength(2)
75+
expect(result.incompleteTodos?.inProgress).toHaveLength(1)
76+
})
77+
78+
test("should provide helpful error message with update_todo_list tool reference", () => {
79+
const todos: TodoItem[] = [{ id: "1", content: "Incomplete task", status: "pending" }]
80+
const result = validateTodosForCompletion(todos)
81+
expect(result.valid).toBe(false)
82+
expect(result.error).toContain(
83+
"Please complete all todos using the update_todo_list tool before attempting completion",
84+
)
85+
})
86+
})

‎src/core/tools/attemptCompletionTool.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
AskFinishSubTaskApproval,
1515
} from "../../shared/tools"
1616
import { formatResponse } from "../prompts/responses"
17+
import { validateTodosForCompletion } from "./updateTodoListTool"
1718

1819
export async function attemptCompletionTool(
1920
cline: Task,
@@ -63,6 +64,17 @@ export async function attemptCompletionTool(
6364
return
6465
}
6566

67+
// Validate that all todos are completed before allowing completion
68+
const todoValidation = validateTodosForCompletion(cline.todoList)
69+
if (!todoValidation.valid) {
70+
cline.consecutiveMistakeCount++
71+
cline.recordToolError("attempt_completion")
72+
pushToolResult(
73+
formatResponse.toolError(todoValidation.error || "Cannot complete task with incomplete todos"),
74+
)
75+
return
76+
}
77+
6678
cline.consecutiveMistakeCount = 0
6779

6880
// Command execution is permanently disabled in attempt_completion

‎src/core/tools/updateTodoListTool.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,57 @@ function validateTodos(todos: any[]): { valid: boolean; error?: string } {
143143
return { valid: true }
144144
}
145145

146+
/**
147+
* Validate that all todos are completed before allowing task completion.
148+
* @param todoList Array of TodoItem objects to validate
149+
* @returns Validation result with error details if incomplete todos exist
150+
*/
151+
export function validateTodosForCompletion(todoList?: TodoItem[]): {
152+
valid: boolean
153+
error?: string
154+
incompleteTodos?: { pending: TodoItem[]; inProgress: TodoItem[] }
155+
} {
156+
// If no todos exist, completion is allowed
157+
if (!todoList || todoList.length === 0) {
158+
return { valid: true }
159+
}
160+
161+
const pendingTodos = todoList.filter((todo) => todo.status === "pending")
162+
const inProgressTodos = todoList.filter((todo) => todo.status === "in_progress")
163+
164+
// If there are any incomplete todos, block completion
165+
if (pendingTodos.length > 0 || inProgressTodos.length > 0) {
166+
let errorMessage = "Cannot attempt completion while there are incomplete todos:\n\n"
167+
168+
if (pendingTodos.length > 0) {
169+
errorMessage += "Pending todos:\n"
170+
pendingTodos.forEach((todo) => {
171+
errorMessage += `- [ ] ${todo.content}\n`
172+
})
173+
errorMessage += "\n"
174+
}
175+
176+
if (inProgressTodos.length > 0) {
177+
errorMessage += "In Progress todos:\n"
178+
inProgressTodos.forEach((todo) => {
179+
errorMessage += `- [-] ${todo.content}\n`
180+
})
181+
errorMessage += "\n"
182+
}
183+
184+
errorMessage += "Please complete all todos using the update_todo_list tool before attempting completion."
185+
186+
return {
187+
valid: false,
188+
error: errorMessage,
189+
incompleteTodos: { pending: pendingTodos, inProgress: inProgressTodos },
190+
}
191+
}
192+
193+
// All todos are completed
194+
return { valid: true }
195+
}
196+
146197
/**
147198
* Update the todo list for a task.
148199
* @param cline Task instance

0 commit comments

Comments
 (0)