Skip to content

Commit 2df9a7a

Browse files
mrubensmrubens
committed
Prevent completion with open todos
1 parent d4abe73 commit 2df9a7a

File tree

2 files changed

+242
-0
lines changed

2 files changed

+242
-0
lines changed

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

Lines changed: 228 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,228 @@
1+
import { describe, it, expect, vi, beforeEach } from "vitest"
2+
import { TodoItem } from "@roo-code/types"
3+
import { AttemptCompletionToolUse } from "../../../shared/tools"
4+
5+
// Mock the formatResponse module before importing the tool
6+
vi.mock("../../prompts/responses", () => ({
7+
formatResponse: {
8+
toolError: vi.fn((msg: string) => `Error: ${msg}`),
9+
},
10+
}))
11+
12+
import { attemptCompletionTool } from "../attemptCompletionTool"
13+
import { Task } from "../../task/Task"
14+
15+
describe("attemptCompletionTool", () => {
16+
let mockTask: Partial<Task>
17+
let mockPushToolResult: ReturnType<typeof vi.fn>
18+
let mockAskApproval: ReturnType<typeof vi.fn>
19+
let mockHandleError: ReturnType<typeof vi.fn>
20+
let mockRemoveClosingTag: ReturnType<typeof vi.fn>
21+
let mockToolDescription: ReturnType<typeof vi.fn>
22+
let mockAskFinishSubTaskApproval: ReturnType<typeof vi.fn>
23+
24+
beforeEach(() => {
25+
mockPushToolResult = vi.fn()
26+
mockAskApproval = vi.fn()
27+
mockHandleError = vi.fn()
28+
mockRemoveClosingTag = vi.fn()
29+
mockToolDescription = vi.fn()
30+
mockAskFinishSubTaskApproval = vi.fn()
31+
32+
mockTask = {
33+
consecutiveMistakeCount: 0,
34+
recordToolError: vi.fn(),
35+
todoList: undefined,
36+
}
37+
})
38+
39+
describe("todo list validation", () => {
40+
it("should allow completion when there is no todo list", async () => {
41+
const block: AttemptCompletionToolUse = {
42+
type: "tool_use",
43+
name: "attempt_completion",
44+
params: { result: "Task completed successfully" },
45+
partial: false,
46+
}
47+
48+
mockTask.todoList = undefined
49+
50+
// Mock the formatResponse to avoid import issues
51+
vi.doMock("../../prompts/responses", () => ({
52+
formatResponse: {
53+
toolError: vi.fn((msg) => `Error: ${msg}`),
54+
},
55+
}))
56+
57+
await attemptCompletionTool(
58+
mockTask as Task,
59+
block,
60+
mockAskApproval,
61+
mockHandleError,
62+
mockPushToolResult,
63+
mockRemoveClosingTag,
64+
mockToolDescription,
65+
mockAskFinishSubTaskApproval,
66+
)
67+
68+
// Should not call pushToolResult with an error for empty todo list
69+
expect(mockTask.consecutiveMistakeCount).toBe(0)
70+
expect(mockTask.recordToolError).not.toHaveBeenCalled()
71+
})
72+
73+
it("should allow completion when todo list is empty", async () => {
74+
const block: AttemptCompletionToolUse = {
75+
type: "tool_use",
76+
name: "attempt_completion",
77+
params: { result: "Task completed successfully" },
78+
partial: false,
79+
}
80+
81+
mockTask.todoList = []
82+
83+
await attemptCompletionTool(
84+
mockTask as Task,
85+
block,
86+
mockAskApproval,
87+
mockHandleError,
88+
mockPushToolResult,
89+
mockRemoveClosingTag,
90+
mockToolDescription,
91+
mockAskFinishSubTaskApproval,
92+
)
93+
94+
expect(mockTask.consecutiveMistakeCount).toBe(0)
95+
expect(mockTask.recordToolError).not.toHaveBeenCalled()
96+
})
97+
98+
it("should allow completion when all todos are completed", async () => {
99+
const block: AttemptCompletionToolUse = {
100+
type: "tool_use",
101+
name: "attempt_completion",
102+
params: { result: "Task completed successfully" },
103+
partial: false,
104+
}
105+
106+
const completedTodos: TodoItem[] = [
107+
{ id: "1", content: "First task", status: "completed" },
108+
{ id: "2", content: "Second task", status: "completed" },
109+
]
110+
111+
mockTask.todoList = completedTodos
112+
113+
await attemptCompletionTool(
114+
mockTask as Task,
115+
block,
116+
mockAskApproval,
117+
mockHandleError,
118+
mockPushToolResult,
119+
mockRemoveClosingTag,
120+
mockToolDescription,
121+
mockAskFinishSubTaskApproval,
122+
)
123+
124+
expect(mockTask.consecutiveMistakeCount).toBe(0)
125+
expect(mockTask.recordToolError).not.toHaveBeenCalled()
126+
})
127+
128+
it("should prevent completion when there are pending todos", async () => {
129+
const block: AttemptCompletionToolUse = {
130+
type: "tool_use",
131+
name: "attempt_completion",
132+
params: { result: "Task completed successfully" },
133+
partial: false,
134+
}
135+
136+
const todosWithPending: TodoItem[] = [
137+
{ id: "1", content: "First task", status: "completed" },
138+
{ id: "2", content: "Second task", status: "pending" },
139+
]
140+
141+
mockTask.todoList = todosWithPending
142+
143+
await attemptCompletionTool(
144+
mockTask as Task,
145+
block,
146+
mockAskApproval,
147+
mockHandleError,
148+
mockPushToolResult,
149+
mockRemoveClosingTag,
150+
mockToolDescription,
151+
mockAskFinishSubTaskApproval,
152+
)
153+
154+
expect(mockTask.consecutiveMistakeCount).toBe(1)
155+
expect(mockTask.recordToolError).toHaveBeenCalledWith("attempt_completion")
156+
expect(mockPushToolResult).toHaveBeenCalledWith(
157+
expect.stringContaining("Cannot complete task while there are incomplete todos"),
158+
)
159+
})
160+
161+
it("should prevent completion when there are in-progress todos", async () => {
162+
const block: AttemptCompletionToolUse = {
163+
type: "tool_use",
164+
name: "attempt_completion",
165+
params: { result: "Task completed successfully" },
166+
partial: false,
167+
}
168+
169+
const todosWithInProgress: TodoItem[] = [
170+
{ id: "1", content: "First task", status: "completed" },
171+
{ id: "2", content: "Second task", status: "in_progress" },
172+
]
173+
174+
mockTask.todoList = todosWithInProgress
175+
176+
await attemptCompletionTool(
177+
mockTask as Task,
178+
block,
179+
mockAskApproval,
180+
mockHandleError,
181+
mockPushToolResult,
182+
mockRemoveClosingTag,
183+
mockToolDescription,
184+
mockAskFinishSubTaskApproval,
185+
)
186+
187+
expect(mockTask.consecutiveMistakeCount).toBe(1)
188+
expect(mockTask.recordToolError).toHaveBeenCalledWith("attempt_completion")
189+
expect(mockPushToolResult).toHaveBeenCalledWith(
190+
expect.stringContaining("Cannot complete task while there are incomplete todos"),
191+
)
192+
})
193+
194+
it("should prevent completion when there are mixed incomplete todos", async () => {
195+
const block: AttemptCompletionToolUse = {
196+
type: "tool_use",
197+
name: "attempt_completion",
198+
params: { result: "Task completed successfully" },
199+
partial: false,
200+
}
201+
202+
const mixedTodos: TodoItem[] = [
203+
{ id: "1", content: "First task", status: "completed" },
204+
{ id: "2", content: "Second task", status: "pending" },
205+
{ id: "3", content: "Third task", status: "in_progress" },
206+
]
207+
208+
mockTask.todoList = mixedTodos
209+
210+
await attemptCompletionTool(
211+
mockTask as Task,
212+
block,
213+
mockAskApproval,
214+
mockHandleError,
215+
mockPushToolResult,
216+
mockRemoveClosingTag,
217+
mockToolDescription,
218+
mockAskFinishSubTaskApproval,
219+
)
220+
221+
expect(mockTask.consecutiveMistakeCount).toBe(1)
222+
expect(mockTask.recordToolError).toHaveBeenCalledWith("attempt_completion")
223+
expect(mockPushToolResult).toHaveBeenCalledWith(
224+
expect.stringContaining("Cannot complete task while there are incomplete todos"),
225+
)
226+
})
227+
})
228+
})

‎src/core/tools/attemptCompletionTool.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,20 @@ export async function attemptCompletionTool(
2828
const result: string | undefined = block.params.result
2929
const command: string | undefined = block.params.command
3030

31+
// Check if there are incomplete todos
32+
const hasIncompleteTodos = cline.todoList && cline.todoList.some((todo) => todo.status !== "completed")
33+
34+
if (hasIncompleteTodos) {
35+
cline.consecutiveMistakeCount++
36+
cline.recordToolError("attempt_completion")
37+
pushToolResult(
38+
formatResponse.toolError(
39+
"Cannot complete task while there are incomplete todos. Please finish all todos before attempting completion.",
40+
),
41+
)
42+
return
43+
}
44+
3145
try {
3246
const lastMessage = cline.clineMessages.at(-1)
3347

0 commit comments

Comments
 (0)