From 7748b1ce30dc2b4b596940c37cf5b8aa98bdf0b7 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Wed, 28 Jan 2026 05:17:35 +0000 Subject: [PATCH] fix(EXT-665): append tool_result to existing user message instead of creating new one True root cause: When reopenParentFromDelegation created a NEW user message with only the new_task tool_result, validateAndFixToolResultIds would see the assistant message expecting BOTH tool_results (e.g., update_todo_list + new_task) but the new message only had new_task, causing a placeholder to be generated for update_todo_list. Fix: Find the existing user message that was created by flushPendingToolResultsToHistory (which already has other tool_results like update_todo_list) and APPEND the new_task tool_result to it instead of creating a new message. This ensures all tool_results for a turn are in the same user message, matching what the Anthropic API expects. --- .../history-resume-delegation.spec.ts | 107 ++++++++++++++++++ src/core/webview/ClineProvider.ts | 84 ++++++++++---- 2 files changed, 170 insertions(+), 21 deletions(-) diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index 6b4b1719a9..833af2338f 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -205,6 +205,7 @@ describe("History resume delegation - parent metadata transitions", () => { it("reopenParentFromDelegation injects tool_result when new_task tool_use exists in API history", async () => { const provider = { contextProxy: { globalStorageUri: { fsPath: "/storage" } }, + log: vi.fn(), getTaskWithId: vi.fn().mockResolvedValue({ historyItem: { id: "p-tool", @@ -704,4 +705,110 @@ describe("History resume delegation - parent metadata transitions", () => { expect((lastMsg.content[0] as any).tool_use_id).toBe(toolUseId) expect((lastMsg.content[0] as any).content).toContain("Subtask c-new completed") }) + + it("reopenParentFromDelegation appends to existing user message when multiple tools in one turn (EXT-665 root cause)", async () => { + // This tests the actual root cause of EXT-665: + // When assistant calls multiple tools (e.g., update_todo_list + new_task) in one turn, + // flushPendingToolResultsToHistory saves a user message with update_todo_list tool_result. + // When child completes, we must APPEND new_task tool_result to that existing message, + // NOT create a new message (which would cause validateAndFixToolResultIds to generate + // a placeholder for update_todo_list since the new message only has new_task). + const logSpy = vi.fn() + const provider = { + contextProxy: { globalStorageUri: { fsPath: "/storage" } }, + log: logSpy, + getTaskWithId: vi.fn().mockResolvedValue({ + historyItem: { + id: "p-multi", + status: "delegated", + awaitingChildId: "c-multi", + childIds: [], + ts: 100, + task: "Parent with multiple tools in one turn", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + }, + }), + emit: vi.fn(), + getCurrentTask: vi.fn(() => ({ taskId: "c-multi" })), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTaskWithHistoryItem: vi.fn().mockResolvedValue({ + taskId: "p-multi", + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + overwriteClineMessages: vi.fn().mockResolvedValue(undefined), + overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), + }), + updateTaskHistory: vi.fn().mockResolvedValue([]), + } as unknown as ClineProvider + + // Simulate the EXT-665 scenario: + // - Assistant calls update_todo_list (id=A) AND new_task (id=B) in one turn + // - flushPendingToolResultsToHistory saved tool_result for update_todo_list + const updateTodoListToolId = "toolu_update123" + const newTaskToolId = "toolu_newtask456" + const existingUiMessages = [{ type: "ask", ask: "tool", text: "tools", ts: 50 }] + const existingApiMessages = [ + { role: "user", content: [{ type: "text", text: "Do the work" }], ts: 40 }, + { + role: "assistant", + content: [ + { + type: "tool_use", + name: "update_todo_list", + id: updateTodoListToolId, + input: { todos: "[ ] Task 1" }, + }, + { + type: "tool_use", + name: "new_task", + id: newTaskToolId, + input: { mode: "code", message: "Do subtask" }, + }, + ], + ts: 50, + }, + // This user message was saved by flushPendingToolResultsToHistory during delegation + // It has tool_result for update_todo_list, but NOT for new_task yet + { + role: "user", + content: [ + { + type: "tool_result", + tool_use_id: updateTodoListToolId, + content: "Delegating to subtask...", + }, + ], + ts: 60, + }, + ] + + vi.mocked(readTaskMessages).mockResolvedValue(existingUiMessages as any) + vi.mocked(readApiMessages).mockResolvedValue(existingApiMessages as any) + + await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "p-multi", + childTaskId: "c-multi", + completionResultSummary: "Subtask completed", + }) + + // Verify that we logged APPEND (not CREATE) + expect(logSpy).toHaveBeenCalledWith( + expect.stringContaining("Appended new_task tool_result to existing user message"), + ) + + // Verify API history still has exactly 3 messages (no new message created) + const apiCall = vi.mocked(saveApiMessages).mock.calls[0][0] + expect(apiCall.messages).toHaveLength(3) + + // Verify the last user message now has BOTH tool_results + const lastUserMsg = apiCall.messages[2] + expect(lastUserMsg.role).toBe("user") + expect(lastUserMsg.content).toHaveLength(2) // update_todo_list + new_task + + // Verify both tool_results are present + const toolResults = (lastUserMsg.content as any[]).filter((b: any) => b.type === "tool_result") + expect(toolResults).toHaveLength(2) + expect(toolResults.map((t: any) => t.tool_use_id).sort()).toEqual([updateTodoListToolId, newTaskToolId].sort()) + }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 1f8e80341e..788ff76423 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3347,7 +3347,9 @@ export class ClineProvider // Check ALL user messages for an existing tool_result with this tool_use_id // (not just the last message, to prevent duplicate tool_results - EXT-665) let alreadyHasToolResult = false - for (const msg of parentApiMessages) { + let existingUserMsgIndex = -1 + for (let i = 0; i < parentApiMessages.length; i++) { + const msg = parentApiMessages[i] if (msg.role === "user" && Array.isArray(msg.content)) { for (const block of msg.content) { if (block.type === "tool_result" && block.tool_use_id === toolUseId) { @@ -3359,31 +3361,71 @@ export class ClineProvider } } if (alreadyHasToolResult) break + // Track the last user message index for potential appending + existingUserMsgIndex = i } } - // Only create a NEW user message with the tool_result if none exists + // If no tool_result exists for new_task, we need to add one. + // IMPORTANT: If there's already a user message with tool_results AFTER the assistant + // message (from flushPendingToolResultsToHistory), append to it instead of creating a new one. + // Creating a new user message causes validateAndFixToolResultIds to generate + // placeholder tool_results for OTHER tool_uses that were already satisfied in + // the previous user message (EXT-665 root cause). if (!alreadyHasToolResult) { - parentApiMessages.push({ - role: "user", - content: [ - { - type: "tool_result" as const, - tool_use_id: toolUseId, - content: `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`, - }, - ], - ts, - }) - } + const newToolResult: Anthropic.ToolResultBlockParam = { + type: "tool_result" as const, + tool_use_id: toolUseId, + content: `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`, + } - // Validate the newly injected tool_result against the preceding assistant message. - // This ensures the tool_result's tool_use_id matches a tool_use in the immediately - // preceding assistant message (Anthropic API requirement). - const lastMessage = parentApiMessages[parentApiMessages.length - 1] - if (lastMessage?.role === "user") { - const validatedMessage = validateAndFixToolResultIds(lastMessage, parentApiMessages.slice(0, -1)) - parentApiMessages[parentApiMessages.length - 1] = validatedMessage + // Find the assistant message with the new_task tool_use + let assistantMsgIdx = -1 + for (let i = parentApiMessages.length - 1; i >= 0; i--) { + const msg = parentApiMessages[i] + if (msg.role === "assistant" && Array.isArray(msg.content)) { + for (const block of msg.content) { + if (block.type === "tool_use" && block.name === "new_task" && block.id === toolUseId) { + assistantMsgIdx = i + break + } + } + if (assistantMsgIdx !== -1) break + } + } + + // Find a user message with tool_results that comes AFTER the assistant message + // This would be from flushPendingToolResultsToHistory during delegation + let targetUserMsgIdx = -1 + if (assistantMsgIdx !== -1) { + for (let i = assistantMsgIdx + 1; i < parentApiMessages.length; i++) { + const msg = parentApiMessages[i] + if (msg.role === "user" && Array.isArray(msg.content)) { + // Check if this user message has any tool_results + const hasToolResults = msg.content.some((block: any) => block.type === "tool_result") + if (hasToolResults) { + targetUserMsgIdx = i + break + } + } + } + } + + if (targetUserMsgIdx !== -1 && Array.isArray(parentApiMessages[targetUserMsgIdx].content)) { + // Append to existing user message that has tool_results + ;(parentApiMessages[targetUserMsgIdx].content as Anthropic.ContentBlockParam[]).push(newToolResult) + this.log( + `[reopenParentFromDelegation] Appended new_task tool_result to existing user message at index ${targetUserMsgIdx}`, + ) + } else { + // Create new user message if no suitable existing message found + parentApiMessages.push({ + role: "user", + content: [newToolResult], + ts, + }) + this.log(`[reopenParentFromDelegation] Created new user message for new_task tool_result`) + } } } else { // If there is no corresponding tool_use in the parent API history, we cannot emit a