diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 620c466331..48236896ac 100644 --- a/.coderabbit.yaml +++ b/.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: >- diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts index 70da666bc2..74949a8742 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts +++ b/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("../../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"]) + }) + }) }) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 3202358c09..dd86cc7d44 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -578,6 +578,13 @@ export class Task extends EventEmitter 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 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 } diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index bbf77a85e9..f52f29174c 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/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]) }) }) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 8a9e1fa4df..00a3db4ee1 100644 --- a/src/eslint-suppressions.json +++ b/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": {