mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: prevent duplicate tool_use IDs causing API 400 errors (#10760)
This commit is contained in:
parent
df42655fc9
commit
bbb6a6e4b4
2 changed files with 298 additions and 2 deletions
|
|
@ -2833,6 +2833,18 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
|
||||
for (const event of events) {
|
||||
if (event.type === "tool_call_start") {
|
||||
// Guard against duplicate tool_call_start events for the same tool ID.
|
||||
// This can occur due to stream retry, reconnection, or API quirks.
|
||||
// Without this check, duplicate tool_use blocks with the same ID would
|
||||
// be added to assistantMessageContent, causing API 400 errors:
|
||||
// "tool_use ids must be unique"
|
||||
if (this.streamingToolCallIndices.has(event.id)) {
|
||||
console.warn(
|
||||
`[Task#${this.taskId}] Ignoring duplicate tool_call_start for ID: ${event.id} (tool: ${event.name})`,
|
||||
)
|
||||
continue
|
||||
}
|
||||
|
||||
// Initialize streaming in NativeToolCallParser
|
||||
NativeToolCallParser.startStreamingToolCall(event.id, event.name as ToolName)
|
||||
|
||||
|
|
@ -3426,6 +3438,10 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
|
||||
// Add tool_use blocks with their IDs for native protocol
|
||||
// This handles both regular ToolUse and McpToolUse types
|
||||
// IMPORTANT: Track seen IDs to prevent duplicates in the API request.
|
||||
// Duplicate tool_use IDs cause Anthropic API 400 errors:
|
||||
// "tool_use ids must be unique"
|
||||
const seenToolUseIds = new Set<string>()
|
||||
const toolUseBlocks = this.assistantMessageContent.filter(
|
||||
(block) => block.type === "tool_use" || block.type === "mcp_tool_use",
|
||||
)
|
||||
|
|
@ -3435,9 +3451,18 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
// The arguments are the raw tool arguments (matching the simplified schema)
|
||||
const mcpBlock = block as import("../../shared/tools").McpToolUse
|
||||
if (mcpBlock.id) {
|
||||
const sanitizedId = sanitizeToolUseId(mcpBlock.id)
|
||||
// Pre-flight deduplication: Skip if we've already added this ID
|
||||
if (seenToolUseIds.has(sanitizedId)) {
|
||||
console.warn(
|
||||
`[Task#${this.taskId}] Pre-flight deduplication: Skipping duplicate MCP tool_use ID: ${sanitizedId} (tool: ${mcpBlock.name})`,
|
||||
)
|
||||
continue
|
||||
}
|
||||
seenToolUseIds.add(sanitizedId)
|
||||
assistantContent.push({
|
||||
type: "tool_use" as const,
|
||||
id: sanitizeToolUseId(mcpBlock.id),
|
||||
id: sanitizedId,
|
||||
name: mcpBlock.name, // Original dynamic name
|
||||
input: mcpBlock.arguments, // Direct tool arguments
|
||||
})
|
||||
|
|
@ -3447,6 +3472,15 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
const toolUse = block as import("../../shared/tools").ToolUse
|
||||
const toolCallId = toolUse.id
|
||||
if (toolCallId) {
|
||||
const sanitizedId = sanitizeToolUseId(toolCallId)
|
||||
// Pre-flight deduplication: Skip if we've already added this ID
|
||||
if (seenToolUseIds.has(sanitizedId)) {
|
||||
console.warn(
|
||||
`[Task#${this.taskId}] Pre-flight deduplication: Skipping duplicate tool_use ID: ${sanitizedId} (tool: ${toolUse.name})`,
|
||||
)
|
||||
continue
|
||||
}
|
||||
seenToolUseIds.add(sanitizedId)
|
||||
// nativeArgs is already in the correct API format for all tools
|
||||
const input = toolUse.nativeArgs || toolUse.params
|
||||
|
||||
|
|
@ -3458,7 +3492,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
|
||||
assistantContent.push({
|
||||
type: "tool_use" as const,
|
||||
id: sanitizeToolUseId(toolCallId),
|
||||
id: sanitizedId,
|
||||
name: toolNameForHistory,
|
||||
input,
|
||||
})
|
||||
|
|
|
|||
262
src/core/task/__tests__/duplicate-tool-use-ids.spec.ts
Normal file
262
src/core/task/__tests__/duplicate-tool-use-ids.spec.ts
Normal file
|
|
@ -0,0 +1,262 @@
|
|||
/**
|
||||
* Tests for duplicate tool_use ID prevention.
|
||||
*
|
||||
* These tests verify the fix for API 400 error "tool_use ids must be unique"
|
||||
* that can occur when:
|
||||
* 1. Stream retries/reconnections cause duplicate tool_call_start events
|
||||
* 2. Multiple tool_use blocks with the same ID accumulate in assistantMessageContent
|
||||
*
|
||||
* The fix implements two layers of protection:
|
||||
* - Layer 1: Guard in streaming handler (streamingToolCallIndices check)
|
||||
* - Layer 2: Pre-flight deduplication when building API request content
|
||||
*/
|
||||
|
||||
import { sanitizeToolUseId } from "../../../utils/tool-id"
|
||||
import type { ToolUse, McpToolUse } from "../../../shared/tools"
|
||||
|
||||
describe("Duplicate tool_use ID Prevention", () => {
|
||||
describe("Pre-flight deduplication logic", () => {
|
||||
/**
|
||||
* Simulates the pre-flight deduplication logic from Task.ts lines 3444-3518.
|
||||
* This tests the Set-based deduplication that happens when building assistant
|
||||
* message content for the API.
|
||||
*/
|
||||
const deduplicateToolUseBlocks = (
|
||||
assistantMessageContent: Array<{ type: string; name?: string; id?: string }>,
|
||||
): Array<{ type: string; name: string; id: string }> => {
|
||||
const seenToolUseIds = new Set<string>()
|
||||
const result: Array<{ type: string; name: string; id: string }> = []
|
||||
|
||||
const toolUseBlocks = assistantMessageContent.filter(
|
||||
(block) => block.type === "tool_use" || block.type === "mcp_tool_use",
|
||||
)
|
||||
|
||||
for (const block of toolUseBlocks) {
|
||||
const id = block.id
|
||||
if (id) {
|
||||
const sanitizedId = sanitizeToolUseId(id)
|
||||
if (seenToolUseIds.has(sanitizedId)) {
|
||||
// Skip duplicate - this is what the fix does
|
||||
continue
|
||||
}
|
||||
seenToolUseIds.add(sanitizedId)
|
||||
result.push({
|
||||
type: "tool_use",
|
||||
name: block.name || "unknown",
|
||||
id: sanitizedId,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
return result
|
||||
}
|
||||
|
||||
it("should skip duplicate tool_use blocks with identical IDs", () => {
|
||||
const assistantMessageContent = [
|
||||
{ type: "tool_use", name: "read_file", id: "toolu_abc123" },
|
||||
{ type: "tool_use", name: "read_file", id: "toolu_abc123" }, // Duplicate
|
||||
]
|
||||
|
||||
const result = deduplicateToolUseBlocks(assistantMessageContent)
|
||||
|
||||
expect(result).toHaveLength(1)
|
||||
expect(result[0].id).toBe("toolu_abc123")
|
||||
})
|
||||
|
||||
it("should preserve unique tool_use blocks", () => {
|
||||
const assistantMessageContent = [
|
||||
{ type: "tool_use", name: "read_file", id: "toolu_abc123" },
|
||||
{ type: "tool_use", name: "write_to_file", id: "toolu_def456" },
|
||||
]
|
||||
|
||||
const result = deduplicateToolUseBlocks(assistantMessageContent)
|
||||
|
||||
expect(result).toHaveLength(2)
|
||||
expect(result[0].id).toBe("toolu_abc123")
|
||||
expect(result[1].id).toBe("toolu_def456")
|
||||
})
|
||||
|
||||
it("should handle multiple duplicates", () => {
|
||||
const assistantMessageContent = [
|
||||
{ type: "tool_use", name: "read_file", id: "toolu_1" },
|
||||
{ type: "tool_use", name: "read_file", id: "toolu_1" }, // Dup of toolu_1
|
||||
{ type: "tool_use", name: "write_to_file", id: "toolu_2" },
|
||||
{ type: "tool_use", name: "write_to_file", id: "toolu_2" }, // Dup of toolu_2
|
||||
{ type: "tool_use", name: "read_file", id: "toolu_1" }, // Another dup of toolu_1
|
||||
]
|
||||
|
||||
const result = deduplicateToolUseBlocks(assistantMessageContent)
|
||||
|
||||
expect(result).toHaveLength(2)
|
||||
expect(result[0].id).toBe("toolu_1")
|
||||
expect(result[1].id).toBe("toolu_2")
|
||||
})
|
||||
|
||||
it("should handle mcp_tool_use blocks", () => {
|
||||
const assistantMessageContent = [
|
||||
{ type: "mcp_tool_use", name: "mcp__server__tool", id: "mcp_123" },
|
||||
{ type: "mcp_tool_use", name: "mcp__server__tool", id: "mcp_123" }, // Duplicate
|
||||
{ type: "tool_use", name: "read_file", id: "toolu_456" },
|
||||
]
|
||||
|
||||
const result = deduplicateToolUseBlocks(assistantMessageContent)
|
||||
|
||||
expect(result).toHaveLength(2)
|
||||
expect(result[0].id).toBe("mcp_123")
|
||||
expect(result[1].id).toBe("toolu_456")
|
||||
})
|
||||
|
||||
it("should sanitize IDs before deduplication", () => {
|
||||
// IDs with special characters that need sanitization
|
||||
const assistantMessageContent = [
|
||||
{ type: "tool_use", name: "read_file", id: "toolu_abc#123" },
|
||||
{ type: "tool_use", name: "read_file", id: "toolu_abc#123" }, // Same after sanitization
|
||||
]
|
||||
|
||||
const result = deduplicateToolUseBlocks(assistantMessageContent)
|
||||
|
||||
// Both should be deduplicated since they sanitize to the same value
|
||||
expect(result).toHaveLength(1)
|
||||
})
|
||||
|
||||
it("should skip blocks without IDs", () => {
|
||||
const assistantMessageContent = [
|
||||
{ type: "tool_use", name: "read_file", id: "toolu_123" },
|
||||
{ type: "tool_use", name: "write_to_file" }, // No ID
|
||||
{ type: "text" }, // Not a tool_use
|
||||
]
|
||||
|
||||
const result = deduplicateToolUseBlocks(assistantMessageContent)
|
||||
|
||||
expect(result).toHaveLength(1)
|
||||
expect(result[0].id).toBe("toolu_123")
|
||||
})
|
||||
})
|
||||
|
||||
describe("Streaming duplicate guard logic", () => {
|
||||
/**
|
||||
* Simulates the streaming duplicate guard from Task.ts lines 2835-2847.
|
||||
* The streamingToolCallIndices Map tracks which tool IDs have already been
|
||||
* added during streaming to prevent duplicate tool_call_start events.
|
||||
*/
|
||||
it("should prevent duplicate tool_call_start events", () => {
|
||||
const streamingToolCallIndices = new Map<string, number>()
|
||||
const processedEvents: string[] = []
|
||||
|
||||
const processToolCallStart = (id: string, name: string): boolean => {
|
||||
// Guard against duplicate tool_call_start events
|
||||
if (streamingToolCallIndices.has(id)) {
|
||||
// Would log: console.warn(`Ignoring duplicate tool_call_start for ID: ${id}`)
|
||||
return false // Skipped
|
||||
}
|
||||
|
||||
// Track the index (simulate adding to assistantMessageContent)
|
||||
streamingToolCallIndices.set(id, processedEvents.length)
|
||||
processedEvents.push(id)
|
||||
return true // Processed
|
||||
}
|
||||
|
||||
// First event for toolu_123 should be processed
|
||||
expect(processToolCallStart("toolu_123", "read_file")).toBe(true)
|
||||
expect(processedEvents).toEqual(["toolu_123"])
|
||||
|
||||
// Duplicate event for toolu_123 should be skipped
|
||||
expect(processToolCallStart("toolu_123", "read_file")).toBe(false)
|
||||
expect(processedEvents).toEqual(["toolu_123"]) // Still only one
|
||||
|
||||
// Different ID should be processed
|
||||
expect(processToolCallStart("toolu_456", "write_to_file")).toBe(true)
|
||||
expect(processedEvents).toEqual(["toolu_123", "toolu_456"])
|
||||
|
||||
// Another duplicate for toolu_456
|
||||
expect(processToolCallStart("toolu_456", "write_to_file")).toBe(false)
|
||||
expect(processedEvents).toEqual(["toolu_123", "toolu_456"]) // No change
|
||||
})
|
||||
|
||||
it("should track indices correctly for multiple tools", () => {
|
||||
const streamingToolCallIndices = new Map<string, number>()
|
||||
let currentIndex = 0
|
||||
|
||||
const processToolCallStart = (id: string): number | null => {
|
||||
if (streamingToolCallIndices.has(id)) {
|
||||
return null // Duplicate
|
||||
}
|
||||
|
||||
const index = currentIndex
|
||||
streamingToolCallIndices.set(id, index)
|
||||
currentIndex++
|
||||
return index
|
||||
}
|
||||
|
||||
expect(processToolCallStart("toolu_1")).toBe(0)
|
||||
expect(processToolCallStart("toolu_2")).toBe(1)
|
||||
expect(processToolCallStart("toolu_3")).toBe(2)
|
||||
|
||||
// Duplicates return null
|
||||
expect(processToolCallStart("toolu_1")).toBeNull()
|
||||
expect(processToolCallStart("toolu_2")).toBeNull()
|
||||
|
||||
// Verify the indices stored
|
||||
expect(streamingToolCallIndices.get("toolu_1")).toBe(0)
|
||||
expect(streamingToolCallIndices.get("toolu_2")).toBe(1)
|
||||
expect(streamingToolCallIndices.get("toolu_3")).toBe(2)
|
||||
})
|
||||
|
||||
it("should clear tracking between API requests", () => {
|
||||
const streamingToolCallIndices = new Map<string, number>()
|
||||
|
||||
// First API request
|
||||
streamingToolCallIndices.set("toolu_123", 0)
|
||||
expect(streamingToolCallIndices.has("toolu_123")).toBe(true)
|
||||
|
||||
// Clear between requests (simulates this.streamingToolCallIndices.clear())
|
||||
streamingToolCallIndices.clear()
|
||||
expect(streamingToolCallIndices.has("toolu_123")).toBe(false)
|
||||
|
||||
// New request can use the same ID
|
||||
streamingToolCallIndices.set("toolu_123", 0)
|
||||
expect(streamingToolCallIndices.has("toolu_123")).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe("Integration scenario: Stream retry causing duplicates", () => {
|
||||
/**
|
||||
* This simulates the exact scenario that causes the API 400 error:
|
||||
* A stream retry or reconnection causes the same tool_call_start event
|
||||
* to be received twice for the same tool ID.
|
||||
*/
|
||||
it("should handle stream retry scenario without duplicate tool_use blocks", () => {
|
||||
// Simulate the state tracking in Task.ts
|
||||
const streamingToolCallIndices = new Map<string, number>()
|
||||
const assistantMessageContent: Array<{ type: string; id: string; name: string }> = []
|
||||
|
||||
const handleToolCallStart = (id: string, name: string) => {
|
||||
// Layer 1: Streaming guard
|
||||
if (streamingToolCallIndices.has(id)) {
|
||||
return // Skip duplicate
|
||||
}
|
||||
|
||||
const toolUseIndex = assistantMessageContent.length
|
||||
streamingToolCallIndices.set(id, toolUseIndex)
|
||||
assistantMessageContent.push({ type: "tool_use", id, name })
|
||||
}
|
||||
|
||||
// Initial tool call
|
||||
handleToolCallStart("toolu_abc123", "read_file")
|
||||
expect(assistantMessageContent).toHaveLength(1)
|
||||
|
||||
// Stream retry causes duplicate tool_call_start
|
||||
handleToolCallStart("toolu_abc123", "read_file")
|
||||
expect(assistantMessageContent).toHaveLength(1) // Still 1, not 2
|
||||
|
||||
// Another tool call
|
||||
handleToolCallStart("toolu_def456", "write_to_file")
|
||||
expect(assistantMessageContent).toHaveLength(2)
|
||||
|
||||
// Final content should have unique IDs
|
||||
const ids = assistantMessageContent.map((block) => block.id)
|
||||
const uniqueIds = [...new Set(ids)]
|
||||
expect(ids).toEqual(uniqueIds) // All IDs are unique
|
||||
})
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Reference in a new issue