Skip to content

Commit d6c075f

Browse files
committed
Fix duplicate tool_result handling and race condition
Removes duplicate tool_result blocks with the same tool_use_id to prevent API protocol violations Adds tests for deduplication and updates ExecuteCommandTool to supersede pending asks, preventing race conditions in terminal fallback scenarios. Also removes debug logging from Task.ts.
1 parent 9f59927 commit d6c075f

4 files changed

Lines changed: 131 additions & 26 deletions

File tree

extensions/roopik-roo/src/core/task/Task.ts

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1090,7 +1090,6 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
10901090
// state.
10911091
askTs = Date.now()
10921092
this.lastMessageTs = askTs
1093-
console.log(`Task#ask: new partial ask -> ${type} @ ${askTs}`)
10941093
await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text, partial, isProtected })
10951094
// console.log("Task#ask: current ask promise was ignored (#2)")
10961095
throw new AskIgnoredError("new partial")
@@ -1115,7 +1114,6 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
11151114
// So in this case we must make sure that the message ts is
11161115
// never altered after first setting it.
11171116
askTs = lastMessage.ts
1118-
console.log(`Task#ask: updating previous partial ask -> ${type} @ ${askTs}`)
11191117
this.lastMessageTs = askTs
11201118
lastMessage.text = text
11211119
lastMessage.partial = false
@@ -1129,7 +1127,6 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
11291127
this.askResponseText = undefined
11301128
this.askResponseImages = undefined
11311129
askTs = Date.now()
1132-
console.log(`Task#ask: new complete ask -> ${type} @ ${askTs}`)
11331130
this.lastMessageTs = askTs
11341131
await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text, isProtected })
11351132
}
@@ -1140,7 +1137,6 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
11401137
this.askResponseText = undefined
11411138
this.askResponseImages = undefined
11421139
askTs = Date.now()
1143-
console.log(`Task#ask: new complete ask -> ${type} @ ${askTs}`)
11441140
this.lastMessageTs = askTs
11451141
await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text, isProtected })
11461142
}
@@ -1170,15 +1166,9 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
11701166
// block (via the `pWaitFor`).
11711167
const isBlocking = !(this.askResponse !== undefined || this.lastMessageTs !== askTs)
11721168
const isMessageQueued = !this.messageQueueService.isEmpty()
1173-
11741169
const isStatusMutable = !partial && isBlocking && !isMessageQueued && approval.decision === "ask"
11751170

1176-
if (isBlocking) {
1177-
console.log(`Task#ask will block -> type: ${type}`)
1178-
}
1179-
11801171
if (isStatusMutable) {
1181-
console.log(`Task#ask: status is mutable -> type: ${type}`)
11821172
const statusMutationTimeout = 2_000
11831173

11841174
if (isInteractiveAsk(type)) {
@@ -1217,8 +1207,6 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
12171207
)
12181208
}
12191209
} else if (isMessageQueued) {
1220-
console.log(`Task#ask: will process message queue -> type: ${type}`)
1221-
12221210
const message = this.messageQueueService.dequeueMessage()
12231211

12241212
if (message) {
@@ -1277,7 +1265,6 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
12771265
// Could happen if we send multiple asks in a row i.e. with
12781266
// command_output. It's important that when we know an ask could
12791267
// fail, it is handled gracefully.
1280-
console.log("Task#ask: current ask promise was ignored")
12811268
throw new AskIgnoredError("superseded")
12821269
}
12831270

@@ -1354,6 +1341,10 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
13541341
this.handleWebviewAskResponse("noButtonClicked", text, images)
13551342
}
13561343

1344+
public supersedePendingAsk(): void {
1345+
this.lastMessageTs = Date.now()
1346+
}
1347+
13571348
/**
13581349
* Updates the API configuration but preserves the locked tool protocol.
13591350
* The task's tool protocol is locked at creation time and should NOT change
@@ -2511,7 +2502,6 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
25112502
// lastMessage.ts = Date.now() DO NOT update ts since it is used as a key for virtuoso list
25122503
lastMessage.partial = false
25132504
// instead of streaming partialMessage events, we do a save and post like normal to persist to disk
2514-
console.log("updating partial message", lastMessage)
25152505
}
25162506

25172507
// Update `api_req_started` to have cancelled and cost, so that

extensions/roopik-roo/src/core/task/__tests__/validateToolResultIds.spec.ts

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -482,6 +482,96 @@ describe("validateAndFixToolResultIds", () => {
482482
expect(resultContent[1].type).toBe("text")
483483
expect((resultContent[1] as Anthropic.TextBlockParam).text).toBe("Some additional context")
484484
})
485+
486+
// Verifies fix for GitHub #10465: Terminal fallback race condition can generate
487+
// duplicate tool_results with the same valid tool_use_id, causing API protocol violations.
488+
it("should filter out duplicate tool_results with identical valid tool_use_ids (terminal fallback scenario)", () => {
489+
const assistantMessage: Anthropic.MessageParam = {
490+
role: "assistant",
491+
content: [
492+
{
493+
type: "tool_use",
494+
id: "tooluse_QZ-pU8v2QKO8L8fHoJRI2g",
495+
name: "execute_command",
496+
input: { command: "ps aux | grep test", cwd: "/path/to/project" },
497+
},
498+
],
499+
}
500+
501+
// Two tool_results with the SAME valid tool_use_id from terminal fallback race condition
502+
const userMessage: Anthropic.MessageParam = {
503+
role: "user",
504+
content: [
505+
{
506+
type: "tool_result",
507+
tool_use_id: "tooluse_QZ-pU8v2QKO8L8fHoJRI2g", // First result from command execution
508+
content: "No test processes found",
509+
},
510+
{
511+
type: "tool_result",
512+
tool_use_id: "tooluse_QZ-pU8v2QKO8L8fHoJRI2g", // Duplicate from user approval during fallback
513+
content: '{"status":"approved","message":"The user approved this operation"}',
514+
},
515+
],
516+
}
517+
518+
const result = validateAndFixToolResultIds(userMessage, [assistantMessage])
519+
520+
expect(Array.isArray(result.content)).toBe(true)
521+
const resultContent = result.content as Anthropic.ToolResultBlockParam[]
522+
523+
// Only ONE tool_result should remain to prevent API protocol violation
524+
expect(resultContent.length).toBe(1)
525+
expect(resultContent[0].tool_use_id).toBe("tooluse_QZ-pU8v2QKO8L8fHoJRI2g")
526+
expect(resultContent[0].content).toBe("No test processes found")
527+
})
528+
529+
it("should preserve text blocks while deduplicating tool_results with same valid ID", () => {
530+
const assistantMessage: Anthropic.MessageParam = {
531+
role: "assistant",
532+
content: [
533+
{
534+
type: "tool_use",
535+
id: "tool-123",
536+
name: "read_file",
537+
input: { path: "test.txt" },
538+
},
539+
],
540+
}
541+
542+
const userMessage: Anthropic.MessageParam = {
543+
role: "user",
544+
content: [
545+
{
546+
type: "tool_result",
547+
tool_use_id: "tool-123",
548+
content: "First result",
549+
},
550+
{
551+
type: "text",
552+
text: "Environment details here",
553+
},
554+
{
555+
type: "tool_result",
556+
tool_use_id: "tool-123", // Duplicate with same valid ID
557+
content: "Duplicate result from fallback",
558+
},
559+
],
560+
}
561+
562+
const result = validateAndFixToolResultIds(userMessage, [assistantMessage])
563+
564+
expect(Array.isArray(result.content)).toBe(true)
565+
const resultContent = result.content as Array<Anthropic.ToolResultBlockParam | Anthropic.TextBlockParam>
566+
567+
// Should have: 1 tool_result + 1 text block (duplicate filtered out)
568+
expect(resultContent.length).toBe(2)
569+
expect(resultContent[0].type).toBe("tool_result")
570+
expect((resultContent[0] as Anthropic.ToolResultBlockParam).tool_use_id).toBe("tool-123")
571+
expect((resultContent[0] as Anthropic.ToolResultBlockParam).content).toBe("First result")
572+
expect(resultContent[1].type).toBe("text")
573+
expect((resultContent[1] as Anthropic.TextBlockParam).text).toBe("Environment details here")
574+
})
485575
})
486576

487577
describe("when there are more tool_uses than tool_results", () => {

extensions/roopik-roo/src/core/task/validateToolResultIds.ts

Lines changed: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,31 @@ export function validateAndFixToolResultIds(
7878
}
7979

8080
// Find tool_result blocks in the user message
81-
const toolResults = userMessage.content.filter(
81+
let toolResults = userMessage.content.filter(
82+
(block): block is Anthropic.ToolResultBlockParam => block.type === "tool_result",
83+
)
84+
85+
// Deduplicate tool_result blocks to prevent API protocol violations (GitHub #10465)
86+
// Terminal fallback race conditions can generate duplicate tool_results with the same tool_use_id.
87+
// Filter out duplicates before validation since Set-based checks below would miss them.
88+
const seenToolResultIds = new Set<string>()
89+
const deduplicatedContent = userMessage.content.filter((block) => {
90+
if (block.type !== "tool_result") {
91+
return true
92+
}
93+
if (seenToolResultIds.has(block.tool_use_id)) {
94+
return false // Duplicate - filter out
95+
}
96+
seenToolResultIds.add(block.tool_use_id)
97+
return true
98+
})
99+
100+
userMessage = {
101+
...userMessage,
102+
content: deduplicatedContent,
103+
}
104+
105+
toolResults = deduplicatedContent.filter(
82106
(block): block is Anthropic.ToolResultBlockParam => block.type === "tool_result",
83107
)
84108

@@ -139,15 +163,12 @@ export function validateAndFixToolResultIds(
139163
)
140164
}
141165

142-
// Create a mapping of tool_result IDs to corrected IDs
143-
// Strategy: Match by position (first tool_result -> first tool_use, etc.)
144-
// This handles most cases where the mismatch is due to ID confusion
145-
//
146-
// Track which tool_use IDs have been used to prevent duplicates
166+
// Match tool_results to tool_uses by position and fix incorrect IDs
147167
const usedToolUseIds = new Set<string>()
168+
const contentArray = userMessage.content as Anthropic.Messages.ContentBlockParam[]
148169

149-
const correctedContent = userMessage.content
150-
.map((block) => {
170+
const correctedContent = contentArray
171+
.map((block: Anthropic.Messages.ContentBlockParam) => {
151172
if (block.type !== "tool_result") {
152173
return block
153174
}
@@ -177,17 +198,18 @@ export function validateAndFixToolResultIds(
177198
}
178199

179200
// No corresponding tool_use for this tool_result, or the ID is already used
180-
// Filter out this orphaned tool_result by returning null
181201
return null
182202
})
183203
.filter((block): block is NonNullable<typeof block> => block !== null)
184204

185205
// Add missing tool_result blocks for any tool_use that doesn't have one
186-
// After the ID correction above, recalculate which tool_use IDs are now covered
187206
const coveredToolUseIds = new Set(
188207
correctedContent
189-
.filter((b): b is Anthropic.ToolResultBlockParam => b.type === "tool_result")
190-
.map((r) => r.tool_use_id),
208+
.filter(
209+
(b: Anthropic.Messages.ContentBlockParam): b is Anthropic.ToolResultBlockParam =>
210+
b.type === "tool_result",
211+
)
212+
.map((r: Anthropic.ToolResultBlockParam) => r.tool_use_id),
191213
)
192214

193215
const stillMissingToolUseIds = toolUseBlocks.filter((toolUse) => !coveredToolUseIds.has(toolUse.id))

extensions/roopik-roo/src/core/tools/ExecuteCommandTool.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,9 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
116116
provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
117117
await task.say("shell_integration_warning")
118118

119+
// Invalidate pending ask from first execution to prevent race condition
120+
task.supersedePendingAsk()
121+
119122
if (error instanceof ShellIntegrationError) {
120123
const [rejected, result] = await executeCommandInTerminal(task, {
121124
...options,

0 commit comments

Comments
 (0)