mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: prevent duplicate tool_result blocks causing API errors (#10497)
This commit is contained in:
parent
503f40241d
commit
741b2680b5
6 changed files with 276 additions and 16 deletions
|
|
@ -77,6 +77,18 @@ describe("presentAssistantMessage - Custom Tool Recording", () => {
|
|||
say: vi.fn().mockResolvedValue(undefined),
|
||||
ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }),
|
||||
}
|
||||
|
||||
// Add pushToolResultToUserContent method after mockTask is created so it can reference mockTask
|
||||
mockTask.pushToolResultToUserContent = vi.fn().mockImplementation((toolResult: any) => {
|
||||
const existingResult = mockTask.userMessageContent.find(
|
||||
(block: any) => block.type === "tool_result" && block.tool_use_id === toolResult.tool_use_id,
|
||||
)
|
||||
if (existingResult) {
|
||||
return false
|
||||
}
|
||||
mockTask.userMessageContent.push(toolResult)
|
||||
return true
|
||||
})
|
||||
})
|
||||
|
||||
describe("Custom tool usage recording", () => {
|
||||
|
|
|
|||
|
|
@ -60,6 +60,18 @@ describe("presentAssistantMessage - Image Handling in Native Tool Calls", () =>
|
|||
say: vi.fn().mockResolvedValue(undefined),
|
||||
ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }),
|
||||
}
|
||||
|
||||
// Add pushToolResultToUserContent method after mockTask is created so it can reference mockTask
|
||||
mockTask.pushToolResultToUserContent = vi.fn().mockImplementation((toolResult: any) => {
|
||||
const existingResult = mockTask.userMessageContent.find(
|
||||
(block: any) => block.type === "tool_result" && block.tool_use_id === toolResult.tool_use_id,
|
||||
)
|
||||
if (existingResult) {
|
||||
return false
|
||||
}
|
||||
mockTask.userMessageContent.push(toolResult)
|
||||
return true
|
||||
})
|
||||
})
|
||||
|
||||
it("should preserve images in tool_result for native protocol", async () => {
|
||||
|
|
|
|||
|
|
@ -59,6 +59,18 @@ describe("presentAssistantMessage - Unknown Tool Handling", () => {
|
|||
say: vi.fn().mockResolvedValue(undefined),
|
||||
ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }),
|
||||
}
|
||||
|
||||
// Add pushToolResultToUserContent method after mockTask is created so 'this' binds correctly
|
||||
mockTask.pushToolResultToUserContent = vi.fn().mockImplementation((toolResult: any) => {
|
||||
const existingResult = mockTask.userMessageContent.find(
|
||||
(block: any) => block.type === "tool_result" && block.tool_use_id === toolResult.tool_use_id,
|
||||
)
|
||||
if (existingResult) {
|
||||
return false
|
||||
}
|
||||
mockTask.userMessageContent.push(toolResult)
|
||||
return true
|
||||
})
|
||||
})
|
||||
|
||||
it("should return error for unknown tool in native protocol", async () => {
|
||||
|
|
|
|||
|
|
@ -115,12 +115,12 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
: `MCP tool ${mcpBlock.name} was interrupted and not executed due to user rejecting a previous tool.`
|
||||
|
||||
if (toolCallId) {
|
||||
cline.userMessageContent.push({
|
||||
cline.pushToolResultToUserContent({
|
||||
type: "tool_result",
|
||||
tool_use_id: toolCallId,
|
||||
content: errorMessage,
|
||||
is_error: true,
|
||||
} as Anthropic.ToolResultBlockParam)
|
||||
})
|
||||
}
|
||||
break
|
||||
}
|
||||
|
|
@ -130,12 +130,12 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
const errorMessage = `MCP tool [${mcpBlock.name}] was not executed because a tool has already been used in this message. Only one tool may be used per message.`
|
||||
|
||||
if (toolCallId) {
|
||||
cline.userMessageContent.push({
|
||||
cline.pushToolResultToUserContent({
|
||||
type: "tool_result",
|
||||
tool_use_id: toolCallId,
|
||||
content: errorMessage,
|
||||
is_error: true,
|
||||
} as Anthropic.ToolResultBlockParam)
|
||||
})
|
||||
}
|
||||
break
|
||||
}
|
||||
|
|
@ -167,11 +167,11 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
}
|
||||
|
||||
if (toolCallId) {
|
||||
cline.userMessageContent.push({
|
||||
cline.pushToolResultToUserContent({
|
||||
type: "tool_result",
|
||||
tool_use_id: toolCallId,
|
||||
content: resultContent,
|
||||
} as Anthropic.ToolResultBlockParam)
|
||||
})
|
||||
|
||||
if (imageBlocks.length > 0) {
|
||||
cline.userMessageContent.push(...imageBlocks)
|
||||
|
|
@ -446,12 +446,12 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
|
||||
if (toolCallId) {
|
||||
// Native protocol: MUST send tool_result for every tool_use
|
||||
cline.userMessageContent.push({
|
||||
cline.pushToolResultToUserContent({
|
||||
type: "tool_result",
|
||||
tool_use_id: toolCallId,
|
||||
content: errorMessage,
|
||||
is_error: true,
|
||||
} as Anthropic.ToolResultBlockParam)
|
||||
})
|
||||
} else {
|
||||
// XML protocol: send as text
|
||||
cline.userMessageContent.push({
|
||||
|
|
@ -471,12 +471,12 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
|
||||
if (toolCallId) {
|
||||
// Native protocol: MUST send tool_result for every tool_use
|
||||
cline.userMessageContent.push({
|
||||
cline.pushToolResultToUserContent({
|
||||
type: "tool_result",
|
||||
tool_use_id: toolCallId,
|
||||
content: errorMessage,
|
||||
is_error: true,
|
||||
} as Anthropic.ToolResultBlockParam)
|
||||
})
|
||||
} else {
|
||||
// XML protocol: send as text
|
||||
cline.userMessageContent.push({
|
||||
|
|
@ -530,11 +530,11 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
}
|
||||
|
||||
// Add tool_result with text content only
|
||||
cline.userMessageContent.push({
|
||||
cline.pushToolResultToUserContent({
|
||||
type: "tool_result",
|
||||
tool_use_id: toolCallId,
|
||||
content: resultContent,
|
||||
} as Anthropic.ToolResultBlockParam)
|
||||
})
|
||||
|
||||
// Add image blocks separately after tool_result
|
||||
if (imageBlocks.length > 0) {
|
||||
|
|
@ -735,12 +735,12 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
|
||||
if (toolProtocol === TOOL_PROTOCOL.NATIVE && toolCallId) {
|
||||
// For native protocol, push tool_result directly without setting didAlreadyUseTool
|
||||
cline.userMessageContent.push({
|
||||
cline.pushToolResultToUserContent({
|
||||
type: "tool_result",
|
||||
tool_use_id: toolCallId,
|
||||
content: typeof errorContent === "string" ? errorContent : "(validation error)",
|
||||
is_error: true,
|
||||
} as Anthropic.ToolResultBlockParam)
|
||||
})
|
||||
} else {
|
||||
// For XML protocol, use the standard pushToolResult
|
||||
pushToolResult(errorContent)
|
||||
|
|
@ -1110,12 +1110,12 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
// Push tool_result directly for native protocol WITHOUT setting didAlreadyUseTool
|
||||
// This prevents the stream from being interrupted with "Response interrupted by tool use result"
|
||||
if (toolProtocol === TOOL_PROTOCOL.NATIVE && toolCallId) {
|
||||
cline.userMessageContent.push({
|
||||
cline.pushToolResultToUserContent({
|
||||
type: "tool_result",
|
||||
tool_use_id: toolCallId,
|
||||
content: formatResponse.toolError(errorMessage, toolProtocol),
|
||||
is_error: true,
|
||||
} as Anthropic.ToolResultBlockParam)
|
||||
})
|
||||
} else {
|
||||
pushToolResult(formatResponse.toolError(errorMessage, toolProtocol))
|
||||
}
|
||||
|
|
|
|||
|
|
@ -337,6 +337,28 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
presentAssistantMessageHasPendingUpdates = false
|
||||
userMessageContent: (Anthropic.TextBlockParam | Anthropic.ImageBlockParam | Anthropic.ToolResultBlockParam)[] = []
|
||||
userMessageContentReady = false
|
||||
|
||||
/**
|
||||
* Push a tool_result block to userMessageContent, preventing duplicates.
|
||||
* This is critical for native tool protocol where duplicate tool_use_ids cause API errors.
|
||||
*
|
||||
* @param toolResult - The tool_result block to add
|
||||
* @returns true if added, false if duplicate was skipped
|
||||
*/
|
||||
public pushToolResultToUserContent(toolResult: Anthropic.ToolResultBlockParam): boolean {
|
||||
const existingResult = this.userMessageContent.find(
|
||||
(block): block is Anthropic.ToolResultBlockParam =>
|
||||
block.type === "tool_result" && block.tool_use_id === toolResult.tool_use_id,
|
||||
)
|
||||
if (existingResult) {
|
||||
console.warn(
|
||||
`[Task#pushToolResultToUserContent] Skipping duplicate tool_result for tool_use_id: ${toolResult.tool_use_id}`,
|
||||
)
|
||||
return false
|
||||
}
|
||||
this.userMessageContent.push(toolResult)
|
||||
return true
|
||||
}
|
||||
didRejectTool = false
|
||||
didAlreadyUseTool = false
|
||||
didToolFailInCurrentTurn = false
|
||||
|
|
|
|||
|
|
@ -1977,3 +1977,205 @@ describe("Queued message processing after condense", () => {
|
|||
expect(taskB.messageQueueService.isEmpty()).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe("pushToolResultToUserContent", () => {
|
||||
let mockProvider: any
|
||||
let mockApiConfig: ProviderSettings
|
||||
|
||||
beforeEach(() => {
|
||||
mockApiConfig = {
|
||||
apiProvider: "anthropic",
|
||||
apiModelId: "claude-3-5-sonnet-20241022",
|
||||
apiKey: "test-api-key",
|
||||
}
|
||||
|
||||
const storageUri = { fsPath: path.join(os.tmpdir(), "test-storage") }
|
||||
const mockExtensionContext = {
|
||||
globalState: {
|
||||
get: vi.fn().mockImplementation((_key: keyof GlobalState) => undefined),
|
||||
update: vi.fn().mockResolvedValue(undefined),
|
||||
keys: vi.fn().mockReturnValue([]),
|
||||
},
|
||||
globalStorageUri: storageUri,
|
||||
workspaceState: {
|
||||
get: vi.fn().mockImplementation((_key) => undefined),
|
||||
update: vi.fn().mockResolvedValue(undefined),
|
||||
keys: vi.fn().mockReturnValue([]),
|
||||
},
|
||||
secrets: {
|
||||
get: vi.fn().mockResolvedValue(undefined),
|
||||
store: vi.fn().mockResolvedValue(undefined),
|
||||
delete: vi.fn().mockResolvedValue(undefined),
|
||||
},
|
||||
extensionUri: { fsPath: "/mock/extension/path" },
|
||||
extension: { packageJSON: { version: "1.0.0" } },
|
||||
} as unknown as vscode.ExtensionContext
|
||||
|
||||
const mockOutputChannel = {
|
||||
name: "test-output",
|
||||
appendLine: vi.fn(),
|
||||
append: vi.fn(),
|
||||
replace: vi.fn(),
|
||||
clear: vi.fn(),
|
||||
show: vi.fn(),
|
||||
hide: vi.fn(),
|
||||
dispose: vi.fn(),
|
||||
}
|
||||
|
||||
mockProvider = new ClineProvider(
|
||||
mockExtensionContext,
|
||||
mockOutputChannel,
|
||||
"sidebar",
|
||||
new ContextProxy(mockExtensionContext),
|
||||
) as any
|
||||
|
||||
mockProvider.postMessageToWebview = vi.fn().mockResolvedValue(undefined)
|
||||
mockProvider.postStateToWebview = vi.fn().mockResolvedValue(undefined)
|
||||
})
|
||||
|
||||
it("should add tool_result when not a duplicate", () => {
|
||||
const task = new Task({
|
||||
provider: mockProvider,
|
||||
apiConfiguration: mockApiConfig,
|
||||
task: "test task",
|
||||
startTask: false,
|
||||
})
|
||||
|
||||
const toolResult: Anthropic.ToolResultBlockParam = {
|
||||
type: "tool_result",
|
||||
tool_use_id: "test-id-1",
|
||||
content: "Test result",
|
||||
}
|
||||
|
||||
const added = task.pushToolResultToUserContent(toolResult)
|
||||
|
||||
expect(added).toBe(true)
|
||||
expect(task.userMessageContent).toHaveLength(1)
|
||||
expect(task.userMessageContent[0]).toEqual(toolResult)
|
||||
})
|
||||
|
||||
it("should prevent duplicate tool_result with same tool_use_id", () => {
|
||||
const task = new Task({
|
||||
provider: mockProvider,
|
||||
apiConfiguration: mockApiConfig,
|
||||
task: "test task",
|
||||
startTask: false,
|
||||
})
|
||||
|
||||
const toolResult1: Anthropic.ToolResultBlockParam = {
|
||||
type: "tool_result",
|
||||
tool_use_id: "duplicate-id",
|
||||
content: "First result",
|
||||
}
|
||||
|
||||
const toolResult2: Anthropic.ToolResultBlockParam = {
|
||||
type: "tool_result",
|
||||
tool_use_id: "duplicate-id",
|
||||
content: "Second result (should be skipped)",
|
||||
}
|
||||
|
||||
// Spy on console.warn to verify warning is logged
|
||||
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {})
|
||||
|
||||
// Add first result - should succeed
|
||||
const added1 = task.pushToolResultToUserContent(toolResult1)
|
||||
expect(added1).toBe(true)
|
||||
expect(task.userMessageContent).toHaveLength(1)
|
||||
|
||||
// Add second result with same ID - should be skipped
|
||||
const added2 = task.pushToolResultToUserContent(toolResult2)
|
||||
expect(added2).toBe(false)
|
||||
expect(task.userMessageContent).toHaveLength(1)
|
||||
|
||||
// Verify only the first result is in the array
|
||||
expect(task.userMessageContent[0]).toEqual(toolResult1)
|
||||
|
||||
// Verify warning was logged
|
||||
expect(warnSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining("Skipping duplicate tool_result for tool_use_id: duplicate-id"),
|
||||
)
|
||||
|
||||
warnSpy.mockRestore()
|
||||
})
|
||||
|
||||
it("should allow different tool_use_ids to be added", () => {
|
||||
const task = new Task({
|
||||
provider: mockProvider,
|
||||
apiConfiguration: mockApiConfig,
|
||||
task: "test task",
|
||||
startTask: false,
|
||||
})
|
||||
|
||||
const toolResult1: Anthropic.ToolResultBlockParam = {
|
||||
type: "tool_result",
|
||||
tool_use_id: "id-1",
|
||||
content: "Result 1",
|
||||
}
|
||||
|
||||
const toolResult2: Anthropic.ToolResultBlockParam = {
|
||||
type: "tool_result",
|
||||
tool_use_id: "id-2",
|
||||
content: "Result 2",
|
||||
}
|
||||
|
||||
const added1 = task.pushToolResultToUserContent(toolResult1)
|
||||
const added2 = task.pushToolResultToUserContent(toolResult2)
|
||||
|
||||
expect(added1).toBe(true)
|
||||
expect(added2).toBe(true)
|
||||
expect(task.userMessageContent).toHaveLength(2)
|
||||
expect(task.userMessageContent[0]).toEqual(toolResult1)
|
||||
expect(task.userMessageContent[1]).toEqual(toolResult2)
|
||||
})
|
||||
|
||||
it("should handle tool_result with is_error flag", () => {
|
||||
const task = new Task({
|
||||
provider: mockProvider,
|
||||
apiConfiguration: mockApiConfig,
|
||||
task: "test task",
|
||||
startTask: false,
|
||||
})
|
||||
|
||||
const errorResult: Anthropic.ToolResultBlockParam = {
|
||||
type: "tool_result",
|
||||
tool_use_id: "error-id",
|
||||
content: "Error message",
|
||||
is_error: true,
|
||||
}
|
||||
|
||||
const added = task.pushToolResultToUserContent(errorResult)
|
||||
|
||||
expect(added).toBe(true)
|
||||
expect(task.userMessageContent).toHaveLength(1)
|
||||
expect(task.userMessageContent[0]).toEqual(errorResult)
|
||||
})
|
||||
|
||||
it("should not interfere with other content types in userMessageContent", () => {
|
||||
const task = new Task({
|
||||
provider: mockProvider,
|
||||
apiConfiguration: mockApiConfig,
|
||||
task: "test task",
|
||||
startTask: false,
|
||||
})
|
||||
|
||||
// Add text and image blocks manually
|
||||
task.userMessageContent.push(
|
||||
{ type: "text", text: "Some text" },
|
||||
{ type: "image", source: { type: "base64", media_type: "image/png", data: "base64data" } },
|
||||
)
|
||||
|
||||
const toolResult: Anthropic.ToolResultBlockParam = {
|
||||
type: "tool_result",
|
||||
tool_use_id: "test-id",
|
||||
content: "Result",
|
||||
}
|
||||
|
||||
const added = task.pushToolResultToUserContent(toolResult)
|
||||
|
||||
expect(added).toBe(true)
|
||||
expect(task.userMessageContent).toHaveLength(3)
|
||||
expect(task.userMessageContent[0].type).toBe("text")
|
||||
expect(task.userMessageContent[1].type).toBe("image")
|
||||
expect(task.userMessageContent[2]).toEqual(toolResult)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue