fix(anthropic): keep tool_result blocks contiguous by grouping image blocks separately

.coderabbit.yaml

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

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

@@ -1,11 +1,14 @@
 // npx vitest src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts
 
 import { DispatchState } from "../../task/dispatchState"
-import { describe, it, expect, beforeEach, vi } from "vitest"
+import { describe, it, expect, beforeEach, beforeAll, vi } from "vitest"
+import type { Anthropic } from "@anthropic-ai/sdk"
 import { presentAssistantMessage } from "../presentAssistantMessage"
 import { validateToolUse } from "../../tools/validateToolUse"
 import type { RequestPolicySnapshot } from "../../prompts/tools/effective-tool-policy"
 
+type UserContentBlock = Anthropic.TextBlockParam | Anthropic.ImageBlockParam | Anthropic.ToolResultBlockParam
+
 // Mock dependencies
 vi.mock("../../task/Task")
 vi.mock("../../tools/validateToolUse", () => ({
@@ -32,6 +35,21 @@ const baseSnapshot: RequestPolicySnapshot = {
 	customModes: [],
 }
 
+// The real Task.prototype.pushToolResultToUserContent, imported past the
+// module-level automock. Binding the real implementation (rather than a copy of
+// its partition logic) is what makes the #1307 tests non-tautological: deleting
+// the grouping from Task.ts makes these tests fail.
+type RealPushToolResult = (
+	this: { userMessageContent: UserContentBlock[] },
+	toolResult: Anthropic.ToolResultBlockParam,
+) => boolean
+let realPushToolResultToUserContent: RealPushToolResult
+
+beforeAll(async () => {
+	const actual = await vi.importActual<typeof import("../../task/Task")>("../../task/Task")
+	realPushToolResultToUserContent = actual.Task.prototype.pushToolResultToUserContent
+})
+
 describe("presentAssistantMessage - Image Handling in Native Tool Calling", () => {
 	let mockTask: any
 
@@ -75,17 +93,11 @@ describe("presentAssistantMessage - Image Handling in Native Tool Calling", () =
 			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
-		})
+		// Bind the REAL Task.prototype method to this structural double: it only
+		// reads and rewrites `userMessageContent`, which the double provides.
+		mockTask.pushToolResultToUserContent = vi.fn((toolResult: Anthropic.ToolResultBlockParam) =>
+			realPushToolResultToUserContent.call(mockTask, toolResult),
+		)
 	})
 
 	it("should preserve images in tool_result for native tool calling", async () => {
@@ -330,4 +342,127 @@ describe("presentAssistantMessage - Image Handling in Native Tool Calling", () =
 			expect(toolResult.content).toContain("was interrupted and not executed")
 		})
 	})
+
+	describe("tool_result contiguity (GitHub #1307)", () => {
+		it("groups images after all tool_results when parallel tools return images", async () => {
+			const toolCallId1 = "tool_call_img_1"
+			const toolCallId2 = "tool_call_img_2"
+
+			mockTask.assistantMessageContent = [
+				{
+					type: "tool_use",
+					id: toolCallId1,
+					name: "ask_followup_question",
+					params: { question: "Q1?", follow_up: [] },
+					nativeArgs: { question: "Q1?", follow_up: [] },
+				},
+				{
+					type: "tool_use",
+					id: toolCallId2,
+					name: "ask_followup_question",
+					params: { question: "Q2?", follow_up: [] },
+					nativeArgs: { question: "Q2?", follow_up: [] },
+				},
+			]
+
+			// The tool asks the followup question via task.ask("followup", ...);
+			// distinct image data per question so ordering is observable.
+			mockTask.ask = vi.fn().mockImplementation((type: string, message: string) => {
+				if (type !== "followup") {
+					return Promise.resolve({ response: "yesButtonClicked", text: "approved", images: undefined })
+				}
+				const prefix = message.includes("Q1") ? "a" : "b"
+				return Promise.resolve({
+					response: "yesButtonClicked",
+					text: `answer ${prefix}`,
+					images: [`data:image/png;base64,${prefix}-result`],
+				})
+			})
+
+			await presentAssistantMessage(mockTask, baseSnapshot)
+
+			// Anthropic rejects user messages where image blocks sit between
+			// tool_result blocks, so images from both tools must be grouped
+			// after the last tool_result instead of interleaved.
+			expect(mockTask.userMessageContent.map((block: UserContentBlock) => block.type)).toEqual([
+				"tool_result",
+				"tool_result",
+				"image",
+				"image",
+			])
+			expect(
+				mockTask.userMessageContent
+					.filter(
+						(block: UserContentBlock): block is Anthropic.ToolResultBlockParam =>
+							block.type === "tool_result",
+					)
+					.map((block: Anthropic.ToolResultBlockParam) => block.tool_use_id),
+			).toEqual([toolCallId1, toolCallId2])
+			expect(
+				mockTask.userMessageContent
+					.filter((block: UserContentBlock): block is Anthropic.ImageBlockParam => block.type === "image")
+					.map(
+						(block: Anthropic.ImageBlockParam) =>
+							(block.source as { type: "base64"; media_type: string; data: string }).data,
+					),
+			).toEqual(["a-result", "b-result"])
+		})
+
+		it("relocates an earlier tool's images after a later tool_result with no images", async () => {
+			const toolCallId1 = "tool_call_img_3"
+			const toolCallId2 = "tool_call_img_4"
+
+			mockTask.assistantMessageContent = [
+				{
+					type: "tool_use",
+					id: toolCallId1,
+					name: "ask_followup_question",
+					params: { question: "Q1?", follow_up: [] },
+					nativeArgs: { question: "Q1?", follow_up: [] },
+				},
+				{
+					type: "tool_use",
+					id: toolCallId2,
+					name: "ask_followup_question",
+					params: { question: "Q2?", follow_up: [] },
+					nativeArgs: { question: "Q2?", follow_up: [] },
+				},
+			]
+
+			mockTask.ask = vi.fn().mockImplementation((type: string, message: string) => {
+				if (type === "followup" && message.includes("Q1")) {
+					return Promise.resolve({
+						response: "yesButtonClicked",
+						text: "answer a",
+						images: ["data:image/png;base64,a-result"],
+					})
+				}
+				return Promise.resolve({ response: "yesButtonClicked", text: "answer b", images: undefined })
+			})
+
+			await presentAssistantMessage(mockTask, baseSnapshot)
+
+			expect(mockTask.userMessageContent.map((block: UserContentBlock) => block.type)).toEqual([
+				"tool_result",
+				"tool_result",
+				"image",
+			])
+			expect(
+				mockTask.userMessageContent
+					.filter(
+						(block: UserContentBlock): block is Anthropic.ToolResultBlockParam =>
+							block.type === "tool_result",
+					)
+					.map((block: Anthropic.ToolResultBlockParam) => block.tool_use_id),
+			).toEqual([toolCallId1, toolCallId2])
+			expect(
+				mockTask.userMessageContent
+					.filter((block: UserContentBlock): block is Anthropic.ImageBlockParam => block.type === "image")
+					.map(
+						(block: Anthropic.ImageBlockParam) =>
+							(block.source as { type: "base64"; media_type: string; data: string }).data,
+					),
+			).toEqual(["a-result"])
+		})
+	})
 })

src/core/task/Task.ts

@@ -578,6 +578,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 	 * Push a tool_result block to userMessageContent, preventing duplicates.
 	 * Duplicate tool_use_ids cause API errors.
 	 *
+	 * Keeps all tool_result blocks first and contiguous, as the Anthropic API
+	 * requires: a user message responding to tool_use blocks must begin with
+	 * the tool_result blocks, and any other content (images, text) between or
+	 * ahead of them is rejected ("tool_use ids were found without tool_result
+	 * blocks immediately after"). All tool_result blocks are therefore kept
+	 * ahead of other content in this array.
+	 *
 	 * @param toolResult - The tool_result block to add
 	 * @returns true if added, false if duplicate was skipped
 	 */
@@ -593,6 +600,16 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 			return false
 		}
 		this.userMessageContent.push(toolResult)
+
+		// Partition into [tool_results, other blocks], preserving order within
+		// each group, so results stay first and contiguous regardless of what
+		// other content (images, text) arrived between pushes.
+		const results = this.userMessageContent.filter(
+			(block): block is Anthropic.ToolResultBlockParam => block.type === "tool_result",
+		)
+		const others = this.userMessageContent.filter((block) => block.type !== "tool_result")
+		this.userMessageContent.splice(0, this.userMessageContent.length, ...results, ...others)
+
 		return true
 	}
 

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

@@ -8255,7 +8255,7 @@ describe("pushToolResultToUserContent", () => {
 		expect(task.userMessageContent[0]).toEqual(errorResult)
 	})
 
-	it("should not interfere with other content types in userMessageContent", () => {
+	it("should preserve other content types while keeping tool_results contiguous", () => {
 		const task = new Task({
 			provider: mockProvider,
 			apiConfiguration: mockApiConfig,
@@ -8264,10 +8264,11 @@ describe("pushToolResultToUserContent", () => {
 		})
 
 		// 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 image: Anthropic.ImageBlockParam = {
+			type: "image",
+			source: { type: "base64", media_type: "image/png", data: "base64data" },
+		}
+		task.userMessageContent.push({ type: "text", text: "Some text" }, image)
 
 		const toolResult: Anthropic.ToolResultBlockParam = {
 			type: "tool_result",
@@ -8279,8 +8280,92 @@ describe("pushToolResultToUserContent", () => {
 
 		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)
+		// Results must lead the message (Anthropic rejects other content ahead
+		// of or between tool_results), so the pre-existing text and image move
+		// after the result; blocks keep their relative order within each group.
+		expect(task.userMessageContent[0]).toEqual(toolResult)
+		expect(task.userMessageContent[1].type).toBe("text")
+		expect(task.userMessageContent[2]).toEqual(image)
+	})
+
+	it("keeps tool_result blocks contiguous when a later result has no images (GitHub #1307)", () => {
+		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 image1: Anthropic.ImageBlockParam = {
+			type: "image",
+			source: { type: "base64", media_type: "image/png", data: "img1" },
+		}
+		const image2: Anthropic.ImageBlockParam = {
+			type: "image",
+			source: { type: "base64", media_type: "image/png", data: "img2" },
+		}
+
+		// First tool result arrives with its images appended right after it (as
+		// presentAssistantMessage does); a second result without images follows.
+		task.pushToolResultToUserContent(toolResult1)
+		task.userMessageContent.push(image1, image2)
+		task.pushToolResultToUserContent(toolResult2)
+
+		expect(task.userMessageContent.map((block) => block.type)).toEqual([
+			"tool_result",
+			"tool_result",
+			"image",
+			"image",
+		])
+		expect(task.userMessageContent[0]).toEqual(toolResult1)
+		expect(task.userMessageContent[1]).toEqual(toolResult2)
+		expect(task.userMessageContent[2]).toEqual(image1)
+		expect(task.userMessageContent[3]).toEqual(image2)
+	})
+
+	it("moves text as well as images behind the tool_results, preserving order within each group", () => {
+		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 image: Anthropic.ImageBlockParam = {
+			type: "image",
+			source: { type: "base64", media_type: "image/png", data: "img1" },
+		}
+		const text1: Anthropic.TextBlockParam = { type: "text", text: "text one" }
+		const text2: Anthropic.TextBlockParam = { type: "text", text: "text two" }
+
+		// Interleaved arrival: image, text, r1, more text, r2. Grouping only
+		// images would leave text1 ahead of r1, which Anthropic rejects; every
+		// non-tool_result block must move behind the contiguous result run.
+		task.userMessageContent.push(image, text1)
+		task.pushToolResultToUserContent(toolResult1)
+		task.userMessageContent.push(text2)
+		task.pushToolResultToUserContent(toolResult2)
+
+		expect(task.userMessageContent).toEqual([toolResult1, toolResult2, image, text1, text2])
 	})
 })

src/eslint-suppressions.json

@@ -571,7 +571,7 @@
 	},
 	"core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts": {
 		"@typescript-eslint/no-explicit-any": {
-			"count": 16
+			"count": 14
 		}
 	},
 	"core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts": {