Skip to content

Commit 70eba96

Browse files
myk1ytZoo (VP)edelauna
authored
fix(anthropic): keep tool_result blocks contiguous by grouping image blocks separately (#2243)
* fix(anthropic): keep tool_result blocks contiguous by grouping image blocks separately When a turn issued multiple tool_use blocks and tool results carried image blocks (read_file on images, screenshots, feedback images), presentAssistantMessage pushed each tool_result followed immediately by its images, yielding tool_result/image interleaving in userMessageContent. Task.ts then consumed the array verbatim, and the Anthropic API rejected the request with a 400: "tool_use ids were found without tool_result blocks immediately after" (#1307). pushToolResultToUserContent is the single choke point every tool_result push flows through, so it now maintains the invariant: when a new tool_result is appended, image blocks already accumulated are moved after it. The resulting user message is tool_results first (in tool_use order), then images, then other blocks — matching the shape the issue expects. A single tool result with images and image-free messages are unchanged. * fix(anthropic): keep all tool_result blocks ahead of other user-message content * test(anthropic): bind the real tool_result grouping and cover mixed interleave Addresses edelauna's two review asks on #1705. 1. presentAssistantMessage-images.spec.ts bound a hand-copied partition of Task.pushToolResultToUserContent, so the #1307 tests were tautological: deleting the grouping from Task.ts still passed them. The mock now binds the real Task.prototype.pushToolResultToUserContent via vi.importActual. Verified: removing the partition from Task.ts fails both #1307 tests. 2. Task.spec.ts only covered "images after the first result". Added the mixed interleave [image, text, r1, text, r2] -> [r1, r2, image, text, text2], which fails under an image-only grouping and only passes with the full tool_result-first partition. Also carries main's new policySnapshot argument at the 2 integration call sites, and drops this spec's no-explicit-any suppressions from 16 to 14. * refactor(task): drop redundant guard in pushToolResultToUserContent --------- Co-authored-by: Zoo (VP) <zoo@emebala.dev> Co-authored-by: Elliott de Launay <edelauna@gmail.com>
1 parent 997f5d8 commit 70eba96

5 files changed

Lines changed: 271 additions & 21 deletions

File tree

‎.coderabbit.yaml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,19 @@ reviews:
9696
a call-count check. For code with fallback behavior, verify both the cold-start case
9797
(no prior state) and the warm case (prior state exists) are covered, as they exercise
9898
different branches.
99+
A test double must not re-implement production logic. If a mock, stub, or inline helper
100+
copies a predicate, filter, sort, or transform that also exists in production code, a
101+
change to the production copy cannot fail the test. Bind the real function instead, for
102+
example with `vi.importActual` or `vi.spyOn`, and replace only the external boundary
103+
(network, filesystem, VS Code API).
104+
For code that filters, groups, reorders, or deduplicates a collection, require a test
105+
with mixed input, such as interleaved item types and repeated items, that asserts the
106+
full output array. A test that uses only one item type cannot fail when the production
107+
predicate becomes narrower. Ask for the case that fails when the predicate covers a
108+
subset of the intended items.
109+
When Stryker reports a surviving mutant on changed code, prefer a test that kills it.
110+
If the mutant is equivalent, prefer to delete the redundant branch before adding a
111+
`Stryker disable` comment.
99112
100113
- path: "apps/vscode-e2e/**"
101114
instructions: >-

‎src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts‎

Lines changed: 147 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
// npx vitest src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts
22

33
import { DispatchState } from "../../task/dispatchState"
4-
import { describe, it, expect, beforeEach, vi } from "vitest"
4+
import { describe, it, expect, beforeEach, beforeAll, vi } from "vitest"
5+
import type { Anthropic } from "@anthropic-ai/sdk"
56
import { presentAssistantMessage } from "../presentAssistantMessage"
67
import { validateToolUse } from "../../tools/validateToolUse"
78
import type { RequestPolicySnapshot } from "../../prompts/tools/effective-tool-policy"
89

10+
type UserContentBlock = Anthropic.TextBlockParam | Anthropic.ImageBlockParam | Anthropic.ToolResultBlockParam
11+
912
// Mock dependencies
1013
vi.mock("../../task/Task")
1114
vi.mock("../../tools/validateToolUse", () => ({
@@ -32,6 +35,21 @@ const baseSnapshot: RequestPolicySnapshot = {
3235
customModes: [],
3336
}
3437

38+
// The real Task.prototype.pushToolResultToUserContent, imported past the
39+
// module-level automock. Binding the real implementation (rather than a copy of
40+
// its partition logic) is what makes the #1307 tests non-tautological: deleting
41+
// the grouping from Task.ts makes these tests fail.
42+
type RealPushToolResult = (
43+
this: { userMessageContent: UserContentBlock[] },
44+
toolResult: Anthropic.ToolResultBlockParam,
45+
) => boolean
46+
let realPushToolResultToUserContent: RealPushToolResult
47+
48+
beforeAll(async () => {
49+
const actual = await vi.importActual<typeof import("../../task/Task")>("../../task/Task")
50+
realPushToolResultToUserContent = actual.Task.prototype.pushToolResultToUserContent
51+
})
52+
3553
describe("presentAssistantMessage - Image Handling in Native Tool Calling", () => {
3654
let mockTask: any
3755

@@ -75,17 +93,11 @@ describe("presentAssistantMessage - Image Handling in Native Tool Calling", () =
7593
ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }),
7694
}
7795

78-
// Add pushToolResultToUserContent method after mockTask is created so it can reference mockTask
79-
mockTask.pushToolResultToUserContent = vi.fn().mockImplementation((toolResult: any) => {
80-
const existingResult = mockTask.userMessageContent.find(
81-
(block: any) => block.type === "tool_result" && block.tool_use_id === toolResult.tool_use_id,
82-
)
83-
if (existingResult) {
84-
return false
85-
}
86-
mockTask.userMessageContent.push(toolResult)
87-
return true
88-
})
96+
// Bind the REAL Task.prototype method to this structural double: it only
97+
// reads and rewrites `userMessageContent`, which the double provides.
98+
mockTask.pushToolResultToUserContent = vi.fn((toolResult: Anthropic.ToolResultBlockParam) =>
99+
realPushToolResultToUserContent.call(mockTask, toolResult),
100+
)
89101
})
90102

91103
it("should preserve images in tool_result for native tool calling", async () => {
@@ -330,4 +342,127 @@ describe("presentAssistantMessage - Image Handling in Native Tool Calling", () =
330342
expect(toolResult.content).toContain("was interrupted and not executed")
331343
})
332344
})
345+
346+
describe("tool_result contiguity (GitHub #1307)", () => {
347+
it("groups images after all tool_results when parallel tools return images", async () => {
348+
const toolCallId1 = "tool_call_img_1"
349+
const toolCallId2 = "tool_call_img_2"
350+
351+
mockTask.assistantMessageContent = [
352+
{
353+
type: "tool_use",
354+
id: toolCallId1,
355+
name: "ask_followup_question",
356+
params: { question: "Q1?", follow_up: [] },
357+
nativeArgs: { question: "Q1?", follow_up: [] },
358+
},
359+
{
360+
type: "tool_use",
361+
id: toolCallId2,
362+
name: "ask_followup_question",
363+
params: { question: "Q2?", follow_up: [] },
364+
nativeArgs: { question: "Q2?", follow_up: [] },
365+
},
366+
]
367+
368+
// The tool asks the followup question via task.ask("followup", ...);
369+
// distinct image data per question so ordering is observable.
370+
mockTask.ask = vi.fn().mockImplementation((type: string, message: string) => {
371+
if (type !== "followup") {
372+
return Promise.resolve({ response: "yesButtonClicked", text: "approved", images: undefined })
373+
}
374+
const prefix = message.includes("Q1") ? "a" : "b"
375+
return Promise.resolve({
376+
response: "yesButtonClicked",
377+
text: `answer ${prefix}`,
378+
images: [`data:image/png;base64,${prefix}-result`],
379+
})
380+
})
381+
382+
await presentAssistantMessage(mockTask, baseSnapshot)
383+
384+
// Anthropic rejects user messages where image blocks sit between
385+
// tool_result blocks, so images from both tools must be grouped
386+
// after the last tool_result instead of interleaved.
387+
expect(mockTask.userMessageContent.map((block: UserContentBlock) => block.type)).toEqual([
388+
"tool_result",
389+
"tool_result",
390+
"image",
391+
"image",
392+
])
393+
expect(
394+
mockTask.userMessageContent
395+
.filter(
396+
(block: UserContentBlock): block is Anthropic.ToolResultBlockParam =>
397+
block.type === "tool_result",
398+
)
399+
.map((block: Anthropic.ToolResultBlockParam) => block.tool_use_id),
400+
).toEqual([toolCallId1, toolCallId2])
401+
expect(
402+
mockTask.userMessageContent
403+
.filter((block: UserContentBlock): block is Anthropic.ImageBlockParam => block.type === "image")
404+
.map(
405+
(block: Anthropic.ImageBlockParam) =>
406+
(block.source as { type: "base64"; media_type: string; data: string }).data,
407+
),
408+
).toEqual(["a-result", "b-result"])
409+
})
410+
411+
it("relocates an earlier tool's images after a later tool_result with no images", async () => {
412+
const toolCallId1 = "tool_call_img_3"
413+
const toolCallId2 = "tool_call_img_4"
414+
415+
mockTask.assistantMessageContent = [
416+
{
417+
type: "tool_use",
418+
id: toolCallId1,
419+
name: "ask_followup_question",
420+
params: { question: "Q1?", follow_up: [] },
421+
nativeArgs: { question: "Q1?", follow_up: [] },
422+
},
423+
{
424+
type: "tool_use",
425+
id: toolCallId2,
426+
name: "ask_followup_question",
427+
params: { question: "Q2?", follow_up: [] },
428+
nativeArgs: { question: "Q2?", follow_up: [] },
429+
},
430+
]
431+
432+
mockTask.ask = vi.fn().mockImplementation((type: string, message: string) => {
433+
if (type === "followup" && message.includes("Q1")) {
434+
return Promise.resolve({
435+
response: "yesButtonClicked",
436+
text: "answer a",
437+
images: ["data:image/png;base64,a-result"],
438+
})
439+
}
440+
return Promise.resolve({ response: "yesButtonClicked", text: "answer b", images: undefined })
441+
})
442+
443+
await presentAssistantMessage(mockTask, baseSnapshot)
444+
445+
expect(mockTask.userMessageContent.map((block: UserContentBlock) => block.type)).toEqual([
446+
"tool_result",
447+
"tool_result",
448+
"image",
449+
])
450+
expect(
451+
mockTask.userMessageContent
452+
.filter(
453+
(block: UserContentBlock): block is Anthropic.ToolResultBlockParam =>
454+
block.type === "tool_result",
455+
)
456+
.map((block: Anthropic.ToolResultBlockParam) => block.tool_use_id),
457+
).toEqual([toolCallId1, toolCallId2])
458+
expect(
459+
mockTask.userMessageContent
460+
.filter((block: UserContentBlock): block is Anthropic.ImageBlockParam => block.type === "image")
461+
.map(
462+
(block: Anthropic.ImageBlockParam) =>
463+
(block.source as { type: "base64"; media_type: string; data: string }).data,
464+
),
465+
).toEqual(["a-result"])
466+
})
467+
})
333468
})

‎src/core/task/Task.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -578,6 +578,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
578578
* Push a tool_result block to userMessageContent, preventing duplicates.
579579
* Duplicate tool_use_ids cause API errors.
580580
*
581+
* Keeps all tool_result blocks first and contiguous, as the Anthropic API
582+
* requires: a user message responding to tool_use blocks must begin with
583+
* the tool_result blocks, and any other content (images, text) between or
584+
* ahead of them is rejected ("tool_use ids were found without tool_result
585+
* blocks immediately after"). All tool_result blocks are therefore kept
586+
* ahead of other content in this array.
587+
*
581588
* @param toolResult - The tool_result block to add
582589
* @returns true if added, false if duplicate was skipped
583590
*/
@@ -593,6 +600,16 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
593600
return false
594601
}
595602
this.userMessageContent.push(toolResult)
603+
604+
// Partition into [tool_results, other blocks], preserving order within
605+
// each group, so results stay first and contiguous regardless of what
606+
// other content (images, text) arrived between pushes.
607+
const results = this.userMessageContent.filter(
608+
(block): block is Anthropic.ToolResultBlockParam => block.type === "tool_result",
609+
)
610+
const others = this.userMessageContent.filter((block) => block.type !== "tool_result")
611+
this.userMessageContent.splice(0, this.userMessageContent.length, ...results, ...others)
612+
596613
return true
597614
}
598615

‎src/core/task/__tests__/Task.spec.ts‎

Lines changed: 93 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8255,7 +8255,7 @@ describe("pushToolResultToUserContent", () => {
82558255
expect(task.userMessageContent[0]).toEqual(errorResult)
82568256
})
82578257

8258-
it("should not interfere with other content types in userMessageContent", () => {
8258+
it("should preserve other content types while keeping tool_results contiguous", () => {
82598259
const task = new Task({
82608260
provider: mockProvider,
82618261
apiConfiguration: mockApiConfig,
@@ -8264,10 +8264,11 @@ describe("pushToolResultToUserContent", () => {
82648264
})
82658265

82668266
// Add text and image blocks manually
8267-
task.userMessageContent.push(
8268-
{ type: "text", text: "Some text" },
8269-
{ type: "image", source: { type: "base64", media_type: "image/png", data: "base64data" } },
8270-
)
8267+
const image: Anthropic.ImageBlockParam = {
8268+
type: "image",
8269+
source: { type: "base64", media_type: "image/png", data: "base64data" },
8270+
}
8271+
task.userMessageContent.push({ type: "text", text: "Some text" }, image)
82718272

82728273
const toolResult: Anthropic.ToolResultBlockParam = {
82738274
type: "tool_result",
@@ -8279,8 +8280,92 @@ describe("pushToolResultToUserContent", () => {
82798280

82808281
expect(added).toBe(true)
82818282
expect(task.userMessageContent).toHaveLength(3)
8282-
expect(task.userMessageContent[0].type).toBe("text")
8283-
expect(task.userMessageContent[1].type).toBe("image")
8284-
expect(task.userMessageContent[2]).toEqual(toolResult)
8283+
// Results must lead the message (Anthropic rejects other content ahead
8284+
// of or between tool_results), so the pre-existing text and image move
8285+
// after the result; blocks keep their relative order within each group.
8286+
expect(task.userMessageContent[0]).toEqual(toolResult)
8287+
expect(task.userMessageContent[1].type).toBe("text")
8288+
expect(task.userMessageContent[2]).toEqual(image)
8289+
})
8290+
8291+
it("keeps tool_result blocks contiguous when a later result has no images (GitHub #1307)", () => {
8292+
const task = new Task({
8293+
provider: mockProvider,
8294+
apiConfiguration: mockApiConfig,
8295+
task: "test task",
8296+
startTask: false,
8297+
})
8298+
8299+
const toolResult1: Anthropic.ToolResultBlockParam = {
8300+
type: "tool_result",
8301+
tool_use_id: "id-1",
8302+
content: "Result 1",
8303+
}
8304+
const toolResult2: Anthropic.ToolResultBlockParam = {
8305+
type: "tool_result",
8306+
tool_use_id: "id-2",
8307+
content: "Result 2",
8308+
}
8309+
const image1: Anthropic.ImageBlockParam = {
8310+
type: "image",
8311+
source: { type: "base64", media_type: "image/png", data: "img1" },
8312+
}
8313+
const image2: Anthropic.ImageBlockParam = {
8314+
type: "image",
8315+
source: { type: "base64", media_type: "image/png", data: "img2" },
8316+
}
8317+
8318+
// First tool result arrives with its images appended right after it (as
8319+
// presentAssistantMessage does); a second result without images follows.
8320+
task.pushToolResultToUserContent(toolResult1)
8321+
task.userMessageContent.push(image1, image2)
8322+
task.pushToolResultToUserContent(toolResult2)
8323+
8324+
expect(task.userMessageContent.map((block) => block.type)).toEqual([
8325+
"tool_result",
8326+
"tool_result",
8327+
"image",
8328+
"image",
8329+
])
8330+
expect(task.userMessageContent[0]).toEqual(toolResult1)
8331+
expect(task.userMessageContent[1]).toEqual(toolResult2)
8332+
expect(task.userMessageContent[2]).toEqual(image1)
8333+
expect(task.userMessageContent[3]).toEqual(image2)
8334+
})
8335+
8336+
it("moves text as well as images behind the tool_results, preserving order within each group", () => {
8337+
const task = new Task({
8338+
provider: mockProvider,
8339+
apiConfiguration: mockApiConfig,
8340+
task: "test task",
8341+
startTask: false,
8342+
})
8343+
8344+
const toolResult1: Anthropic.ToolResultBlockParam = {
8345+
type: "tool_result",
8346+
tool_use_id: "id-1",
8347+
content: "Result 1",
8348+
}
8349+
const toolResult2: Anthropic.ToolResultBlockParam = {
8350+
type: "tool_result",
8351+
tool_use_id: "id-2",
8352+
content: "Result 2",
8353+
}
8354+
const image: Anthropic.ImageBlockParam = {
8355+
type: "image",
8356+
source: { type: "base64", media_type: "image/png", data: "img1" },
8357+
}
8358+
const text1: Anthropic.TextBlockParam = { type: "text", text: "text one" }
8359+
const text2: Anthropic.TextBlockParam = { type: "text", text: "text two" }
8360+
8361+
// Interleaved arrival: image, text, r1, more text, r2. Grouping only
8362+
// images would leave text1 ahead of r1, which Anthropic rejects; every
8363+
// non-tool_result block must move behind the contiguous result run.
8364+
task.userMessageContent.push(image, text1)
8365+
task.pushToolResultToUserContent(toolResult1)
8366+
task.userMessageContent.push(text2)
8367+
task.pushToolResultToUserContent(toolResult2)
8368+
8369+
expect(task.userMessageContent).toEqual([toolResult1, toolResult2, image, text1, text2])
82858370
})
82868371
})

‎src/eslint-suppressions.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,7 @@
571571
},
572572
"core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts": {
573573
"@typescript-eslint/no-explicit-any": {
574-
"count": 16
574+
"count": 14
575575
}
576576
},
577577
"core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts": {

0 commit comments

Comments
 (0)