Skip to content

Commit 1c84dd2

Browse files
committed
feat: optimize todo_tool with multi-tool execution and diff UI
- Enable update_todo_list to execute alongside other tools in the same LLM response - Replace full todo list output with collapsible diff-style UI feedback - Add TodoListDiff component for compact change visualization - Implement diff generation algorithm to track added/removed/modified todos - Add comprehensive tests for multi-tool execution and diff generation - Reduce token usage by showing only changes instead of full list Fixes #7412
1 parent 8684877 commit 1c84dd2

File tree

7 files changed

+888
-10
lines changed

7 files changed

+888
-10
lines changed
Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
import { describe, it, expect, vi, beforeEach } from "vitest"
2+
import { presentAssistantMessage } from "../presentAssistantMessage"
3+
import { Task } from "../../task/Task"
4+
import { updateTodoListTool } from "../../tools/updateTodoListTool"
5+
import { readFileTool } from "../../tools/readFileTool"
6+
7+
vi.mock("../../tools/updateTodoListTool")
8+
vi.mock("../../tools/readFileTool")
9+
vi.mock("@roo-code/telemetry", () => ({
10+
TelemetryService: {
11+
instance: {
12+
captureToolUsage: vi.fn(),
13+
captureConsecutiveMistakeError: vi.fn(),
14+
},
15+
hasInstance: () => true,
16+
},
17+
}))
18+
19+
describe("presentAssistantMessage - Multi-tool execution", () => {
20+
let mockTask: Task
21+
let mockAskApproval: any
22+
let mockHandleError: any
23+
let mockPushToolResult: any
24+
25+
beforeEach(() => {
26+
vi.clearAllMocks()
27+
28+
// Create a minimal mock task
29+
mockTask = {
30+
taskId: "test-task",
31+
instanceId: "test-instance",
32+
abort: false,
33+
presentAssistantMessageLocked: false,
34+
presentAssistantMessageHasPendingUpdates: false,
35+
currentStreamingContentIndex: 0,
36+
assistantMessageContent: [],
37+
didCompleteReadingStream: false,
38+
userMessageContentReady: false,
39+
didRejectTool: false,
40+
didAlreadyUseTool: false,
41+
userMessageContent: [],
42+
say: vi.fn(),
43+
ask: vi.fn(),
44+
recordToolUsage: vi.fn(),
45+
recordToolError: vi.fn(),
46+
consecutiveMistakeCount: 0,
47+
clineMessages: [],
48+
apiConversationHistory: [],
49+
todoList: [],
50+
checkpointSave: vi.fn(),
51+
currentStreamingDidCheckpoint: false,
52+
browserSession: { closeBrowser: vi.fn() },
53+
toolRepetitionDetector: { check: vi.fn(() => ({ allowExecution: true })) },
54+
providerRef: { deref: vi.fn(() => ({ getState: vi.fn(() => ({ mode: "code" })) })) },
55+
api: { getModel: vi.fn(() => ({ id: "test-model" })) },
56+
} as any
57+
58+
mockAskApproval = vi.fn(() => Promise.resolve(true))
59+
mockHandleError = vi.fn()
60+
mockPushToolResult = vi.fn()
61+
})
62+
63+
it("should allow update_todo_list to execute alongside other tools", async () => {
64+
// Set up assistant message content with two tools
65+
mockTask.assistantMessageContent = [
66+
{
67+
type: "tool_use",
68+
name: "read_file",
69+
params: { path: "test.txt" },
70+
partial: false,
71+
},
72+
{
73+
type: "tool_use",
74+
name: "update_todo_list",
75+
params: { todos: "[ ] Test todo" },
76+
partial: false,
77+
},
78+
]
79+
80+
// Mock the tool implementations
81+
vi.mocked(readFileTool).mockImplementation(async (cline, block, askApproval, handleError, pushToolResult) => {
82+
pushToolResult("File content")
83+
})
84+
85+
vi.mocked(updateTodoListTool).mockImplementation(
86+
async (cline, block, askApproval, handleError, pushToolResult) => {
87+
pushToolResult("Todo list updated")
88+
},
89+
)
90+
91+
// Process first tool
92+
mockTask.currentStreamingContentIndex = 0
93+
await presentAssistantMessage(mockTask)
94+
95+
// After first tool, didAlreadyUseTool should be true
96+
expect(mockTask.didAlreadyUseTool).toBe(true)
97+
98+
// Process second tool (update_todo_list)
99+
mockTask.currentStreamingContentIndex = 1
100+
await presentAssistantMessage(mockTask)
101+
102+
// Both tools should have been executed
103+
expect(readFileTool).toHaveBeenCalledTimes(1)
104+
expect(updateTodoListTool).toHaveBeenCalledTimes(1)
105+
106+
// Check that both tool results were pushed
107+
// The first two entries should be for read_file
108+
expect(mockTask.userMessageContent[0]).toEqual({
109+
type: "text",
110+
text: expect.stringContaining("Result:"),
111+
})
112+
expect(mockTask.userMessageContent[1]).toEqual({
113+
type: "text",
114+
text: "File content",
115+
})
116+
// The next two entries should be for update_todo_list
117+
expect(mockTask.userMessageContent[2]).toEqual({
118+
type: "text",
119+
text: "[update_todo_list] Result:",
120+
})
121+
expect(mockTask.userMessageContent[3]).toEqual({
122+
type: "text",
123+
text: "Todo list updated",
124+
})
125+
})
126+
127+
it("should block non-update_todo_list tools after a tool has been used", async () => {
128+
// Set up assistant message content with two non-update_todo_list tools
129+
mockTask.assistantMessageContent = [
130+
{
131+
type: "tool_use",
132+
name: "read_file",
133+
params: { path: "test.txt" },
134+
partial: false,
135+
},
136+
{
137+
type: "tool_use",
138+
name: "write_to_file",
139+
params: { path: "test.txt", content: "new content" },
140+
partial: false,
141+
},
142+
]
143+
144+
// Mock the read_file tool
145+
vi.mocked(readFileTool).mockImplementation(async (cline, block, askApproval, handleError, pushToolResult) => {
146+
pushToolResult("File content")
147+
})
148+
149+
// Process first tool
150+
mockTask.currentStreamingContentIndex = 0
151+
await presentAssistantMessage(mockTask)
152+
153+
// After first tool, didAlreadyUseTool should be true
154+
expect(mockTask.didAlreadyUseTool).toBe(true)
155+
156+
// Process second tool (should be blocked)
157+
mockTask.currentStreamingContentIndex = 1
158+
await presentAssistantMessage(mockTask)
159+
160+
// Only the first tool should have been executed
161+
expect(readFileTool).toHaveBeenCalledTimes(1)
162+
163+
// Check that the second tool was blocked
164+
expect(mockTask.userMessageContent).toContainEqual(
165+
expect.objectContaining({
166+
type: "text",
167+
text: expect.stringContaining(
168+
"Tool [write_to_file] was not executed because a tool has already been used",
169+
),
170+
}),
171+
)
172+
})
173+
174+
it("should not set didAlreadyUseTool when update_todo_list is executed", async () => {
175+
// Set up assistant message content with update_todo_list first
176+
mockTask.assistantMessageContent = [
177+
{
178+
type: "tool_use",
179+
name: "update_todo_list",
180+
params: { todos: "[ ] Test todo" },
181+
partial: false,
182+
},
183+
]
184+
185+
// Mock the tool implementation
186+
vi.mocked(updateTodoListTool).mockImplementation(
187+
async (cline, block, askApproval, handleError, pushToolResult) => {
188+
pushToolResult("Todo list updated")
189+
},
190+
)
191+
192+
// Process the update_todo_list tool
193+
await presentAssistantMessage(mockTask)
194+
195+
// didAlreadyUseTool should remain false for update_todo_list
196+
expect(mockTask.didAlreadyUseTool).toBe(false)
197+
expect(updateTodoListTool).toHaveBeenCalledTimes(1)
198+
})
199+
})

src/core/assistant-message/presentAssistantMessage.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -242,8 +242,11 @@ export async function presentAssistantMessage(cline: Task) {
242242
break
243243
}
244244

245-
if (cline.didAlreadyUseTool) {
246-
// Ignore any content after a tool has already been used.
245+
// Special handling for update_todo_list - it can be used alongside other tools
246+
const isUpdateTodoList = block.name === "update_todo_list"
247+
248+
if (cline.didAlreadyUseTool && !isUpdateTodoList) {
249+
// Ignore any content after a tool has already been used (except update_todo_list).
247250
cline.userMessageContent.push({
248251
type: "text",
249252
text: `Tool [${block.name}] was not executed because a tool has already been used in this message. Only one tool may be used per message. You must assess the first tool's result before proceeding to use the next tool.`,
@@ -263,8 +266,10 @@ export async function presentAssistantMessage(cline: Task) {
263266

264267
// Once a tool result has been collected, ignore all other tool
265268
// uses since we should only ever present one tool result per
266-
// message.
267-
cline.didAlreadyUseTool = true
269+
// message. Exception: update_todo_list can be used alongside other tools.
270+
if (!isUpdateTodoList) {
271+
cline.didAlreadyUseTool = true
272+
}
268273
}
269274

270275
const askApproval = async (

0 commit comments

Comments
 (0)