mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
fix: prevent duplicate tool_result in subtask delegation (EXT-665)
- Check ALL user messages in parentApiMessages for existing tool_result with the same tool_use_id before appending a new one - Previously only checked the last message, missing duplicates in earlier messages - Log warning when skipping duplicate tool_result for debugging - Add tests for duplicate detection and normal case scenarios
This commit is contained in:
Roo Code
parent
a44842f16f
commit
42f2b21113
2 changed files with 177 additions and 11 deletions
|
|
@ -541,4 +541,167 @@ describe("History resume delegation - parent metadata transitions", () => {
|
|||
}),
|
||||
)
|
||||
})
|
||||
|
||||
it("reopenParentFromDelegation skips duplicate tool_result when one already exists in history (EXT-665)", async () => {
|
||||
const logSpy = vi.fn()
|
||||
const provider = {
|
||||
contextProxy: { globalStorageUri: { fsPath: "/storage" } },
|
||||
log: logSpy,
|
||||
getTaskWithId: vi.fn().mockResolvedValue({
|
||||
historyItem: {
|
||||
id: "p-dup",
|
||||
status: "delegated",
|
||||
awaitingChildId: "c-dup",
|
||||
childIds: [],
|
||||
ts: 100,
|
||||
task: "Parent with existing tool_result",
|
||||
tokensIn: 0,
|
||||
tokensOut: 0,
|
||||
totalCost: 0,
|
||||
},
|
||||
}),
|
||||
emit: vi.fn(),
|
||||
getCurrentTask: vi.fn(() => ({ taskId: "c-dup" })),
|
||||
removeClineFromStack: vi.fn().mockResolvedValue(undefined),
|
||||
createTaskWithHistoryItem: vi.fn().mockResolvedValue({
|
||||
taskId: "p-dup",
|
||||
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 bug scenario: API history already has a tool_result for the same tool_use_id
|
||||
// This can happen if the tool was interrupted and the result was already added
|
||||
const existingToolUseId = "toolu_01SnH3c7xgVdfLc2md4Fk6yB"
|
||||
const existingUiMessages = [{ type: "ask", ask: "tool", text: "new_task request", ts: 50 }]
|
||||
const existingApiMessages = [
|
||||
{ role: "user", content: [{ type: "text", text: "Create a subtask" }], ts: 40 },
|
||||
{
|
||||
role: "assistant",
|
||||
content: [
|
||||
{
|
||||
type: "tool_use",
|
||||
name: "new_task",
|
||||
id: existingToolUseId,
|
||||
input: { mode: "code", message: "Do something" },
|
||||
},
|
||||
],
|
||||
ts: 50,
|
||||
},
|
||||
// This tool_result already exists from a previous operation (e.g., interruption)
|
||||
{
|
||||
role: "user",
|
||||
content: [
|
||||
{
|
||||
type: "tool_result",
|
||||
tool_use_id: existingToolUseId,
|
||||
content: "Tool execution was interrupted before completion.",
|
||||
},
|
||||
],
|
||||
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-dup",
|
||||
childTaskId: "c-dup",
|
||||
completionResultSummary: "Subtask completed successfully",
|
||||
})
|
||||
|
||||
// Verify that we logged the skip message
|
||||
expect(logSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining(`Skipping duplicate tool_result for tool_use_id: ${existingToolUseId}`),
|
||||
)
|
||||
|
||||
// Verify API history was saved WITHOUT a new tool_result (should still have exactly 3 messages)
|
||||
const apiCall = vi.mocked(saveApiMessages).mock.calls[0][0]
|
||||
expect(apiCall.messages).toHaveLength(3) // Original 3 messages, no new one added
|
||||
|
||||
// Count tool_result blocks - should only be 1 (the existing one)
|
||||
const toolResultBlocks = apiCall.messages.flatMap((msg: any) =>
|
||||
msg.role === "user" && Array.isArray(msg.content)
|
||||
? msg.content.filter((block: any) => block.type === "tool_result")
|
||||
: [],
|
||||
)
|
||||
expect(toolResultBlocks).toHaveLength(1)
|
||||
expect(toolResultBlocks[0].tool_use_id).toBe(existingToolUseId)
|
||||
})
|
||||
|
||||
it("reopenParentFromDelegation adds tool_result when none exists for the tool_use_id (normal case)", async () => {
|
||||
const logSpy = vi.fn()
|
||||
const provider = {
|
||||
contextProxy: { globalStorageUri: { fsPath: "/storage" } },
|
||||
log: logSpy,
|
||||
getTaskWithId: vi.fn().mockResolvedValue({
|
||||
historyItem: {
|
||||
id: "p-new",
|
||||
status: "delegated",
|
||||
awaitingChildId: "c-new",
|
||||
childIds: [],
|
||||
ts: 100,
|
||||
task: "Parent without tool_result yet",
|
||||
tokensIn: 0,
|
||||
tokensOut: 0,
|
||||
totalCost: 0,
|
||||
},
|
||||
}),
|
||||
emit: vi.fn(),
|
||||
getCurrentTask: vi.fn(() => ({ taskId: "c-new" })),
|
||||
removeClineFromStack: vi.fn().mockResolvedValue(undefined),
|
||||
createTaskWithHistoryItem: vi.fn().mockResolvedValue({
|
||||
taskId: "p-new",
|
||||
resumeAfterDelegation: vi.fn().mockResolvedValue(undefined),
|
||||
overwriteClineMessages: vi.fn().mockResolvedValue(undefined),
|
||||
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
|
||||
}),
|
||||
updateTaskHistory: vi.fn().mockResolvedValue([]),
|
||||
} as unknown as ClineProvider
|
||||
|
||||
// Normal case: API history has tool_use but no tool_result yet
|
||||
const toolUseId = "toolu_normal123"
|
||||
const existingUiMessages = [{ type: "ask", ask: "tool", text: "new_task request", ts: 50 }]
|
||||
const existingApiMessages = [
|
||||
{ role: "user", content: [{ type: "text", text: "Create a subtask" }], ts: 40 },
|
||||
{
|
||||
role: "assistant",
|
||||
content: [
|
||||
{
|
||||
type: "tool_use",
|
||||
name: "new_task",
|
||||
id: toolUseId,
|
||||
input: { mode: "code", message: "Do something" },
|
||||
},
|
||||
],
|
||||
ts: 50,
|
||||
},
|
||||
]
|
||||
|
||||
vi.mocked(readTaskMessages).mockResolvedValue(existingUiMessages as any)
|
||||
vi.mocked(readApiMessages).mockResolvedValue(existingApiMessages as any)
|
||||
|
||||
await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, {
|
||||
parentTaskId: "p-new",
|
||||
childTaskId: "c-new",
|
||||
completionResultSummary: "Subtask completed successfully",
|
||||
})
|
||||
|
||||
// Verify that we did NOT log a skip message
|
||||
expect(logSpy).not.toHaveBeenCalledWith(expect.stringContaining("Skipping duplicate tool_result"))
|
||||
|
||||
// Verify API history was saved WITH a new tool_result (should have 3 messages now)
|
||||
const apiCall = vi.mocked(saveApiMessages).mock.calls[0][0]
|
||||
expect(apiCall.messages).toHaveLength(3)
|
||||
|
||||
// The last message should be the new tool_result
|
||||
const lastMsg = apiCall.messages[2]
|
||||
expect(lastMsg.role).toBe("user")
|
||||
expect((lastMsg.content[0] as any).type).toBe("tool_result")
|
||||
expect((lastMsg.content[0] as any).tool_use_id).toBe(toolUseId)
|
||||
expect((lastMsg.content[0] as any).content).toContain("Subtask c-new completed")
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -3344,22 +3344,25 @@ export class ClineProvider
|
|||
// inject a matching tool_result for the Anthropic message contract:
|
||||
// user → assistant (tool_use) → user (tool_result)
|
||||
if (toolUseId) {
|
||||
// Check if the last message is already a user message with a tool_result for this tool_use_id
|
||||
// (in case this is a retry or the history was already updated)
|
||||
const lastMsg = parentApiMessages[parentApiMessages.length - 1]
|
||||
// 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
|
||||
if (lastMsg?.role === "user" && Array.isArray(lastMsg.content)) {
|
||||
for (const block of lastMsg.content) {
|
||||
if (block.type === "tool_result" && block.tool_use_id === toolUseId) {
|
||||
// Update the existing tool_result content
|
||||
block.content = `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`
|
||||
alreadyHasToolResult = true
|
||||
break
|
||||
for (const msg of parentApiMessages) {
|
||||
if (msg.role === "user" && Array.isArray(msg.content)) {
|
||||
for (const block of msg.content) {
|
||||
if (block.type === "tool_result" && block.tool_use_id === toolUseId) {
|
||||
alreadyHasToolResult = true
|
||||
this.log(
|
||||
`[reopenParentFromDelegation] Skipping duplicate tool_result for tool_use_id: ${toolUseId}`,
|
||||
)
|
||||
break
|
||||
}
|
||||
}
|
||||
if (alreadyHasToolResult) break
|
||||
}
|
||||
}
|
||||
|
||||
// If no existing tool_result found, create a NEW user message with the tool_result
|
||||
// Only create a NEW user message with the tool_result if none exists
|
||||
if (!alreadyHasToolResult) {
|
||||
parentApiMessages.push({
|
||||
role: "user",
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue