From 2d0fa89ec361f9a2c33ef4b2e83176b5c573a57b Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Thu, 12 Feb 2026 22:45:14 -0700 Subject: [PATCH] fix: prevent false 'tool not used' errors when parseToolCall returns null When NativeToolCallParser.parseToolCall() returns null in the 'tool_call' handler, push a fallback ToolUse block to assistantMessageContent instead of silently breaking. This ensures the didToolUse check at the end of the request sees a tool_use block, preventing the false '[ERROR] You did not use a tool' injection. The fallback block flows into presentAssistantMessage()'s existing unknown-tool handler which reports the actual error to the model. Add 4 tests covering the fallback behavior. --- src/core/task/Task.ts | 11 +- src/core/task/__tests__/Task.spec.ts | 145 +++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 2 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 10e7da857c..2747ad4016 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -3082,7 +3082,7 @@ export class Task extends EventEmitter implements TaskLike { case "tool_call": { // Legacy: Handle complete tool calls (for backward compatibility) // Convert native tool call to ToolUse format - const toolUse = NativeToolCallParser.parseToolCall({ + let toolUse = NativeToolCallParser.parseToolCall({ id: chunk.id, name: chunk.name as ToolName, arguments: chunk.arguments, @@ -3090,7 +3090,14 @@ export class Task extends EventEmitter implements TaskLike { if (!toolUse) { console.error(`Failed to parse tool call for task ${this.taskId}:`, chunk) - break + // Still push a tool_use block so the didToolUse check passes + // and presentAssistantMessage's unknown-tool handler can report the error + toolUse = { + type: "tool_use" as const, + name: (chunk.name ?? "unknown_tool") as ToolName, + params: {}, + partial: false, + } } // Store the tool call ID on the ToolUse object for later reference diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 135d691de9..8df49b4e28 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -15,6 +15,7 @@ import { ApiStreamChunk } from "../../../api/transform/stream" import { ContextProxy } from "../../config/ContextProxy" import { processUserContentMentions } from "../../mentions/processUserContentMentions" import { MultiSearchReplaceDiffStrategy } from "../../diff/strategies/multi-search-replace" +import { NativeToolCallParser } from "../../assistant-message/NativeToolCallParser" // Mock delay before any imports that might use it vi.mock("delay", () => ({ @@ -2259,4 +2260,148 @@ describe("pushToolResultToUserContent", () => { expect(task.pendingToolResults).toHaveLength(1) expect(task.pendingToolResults[0]).toEqual(toolResult) }) + + describe("case tool_call handler - fallback when parseToolCall returns null", () => { + it("should push a fallback tool_use block when parseToolCall returns null", () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + // Verify that parseToolCall returns null for an unrecognized tool name + const result = NativeToolCallParser.parseToolCall({ + id: "call_fallback_123", + name: "completely_nonexistent_tool" as any, + arguments: "{}", + }) + expect(result).toBeNull() + + // Simulate the fixed case "tool_call" handler: + // When parseToolCall returns null, the handler now creates a fallback ToolUse block + const chunk = { + type: "tool_call" as const, + id: "call_fallback_123", + name: "completely_nonexistent_tool", + arguments: "{}", + } + + let toolUse = NativeToolCallParser.parseToolCall({ + id: chunk.id, + name: chunk.name as any, + arguments: chunk.arguments, + }) + + if (!toolUse) { + toolUse = { + type: "tool_use" as const, + name: (chunk.name ?? "unknown_tool") as any, + params: {}, + partial: false, + } + } + + toolUse.id = chunk.id + task.assistantMessageContent.push(toolUse) + + // Verify the fallback block was pushed + expect(task.assistantMessageContent).toHaveLength(1) + const block = task.assistantMessageContent[0] as any + expect(block.type).toBe("tool_use") + expect(block.name).toBe("completely_nonexistent_tool") + expect(block.id).toBe("call_fallback_123") + expect(block.partial).toBe(false) + }) + + it("should ensure didToolUse check passes when fallback tool_use block is pushed", () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + // Simulate the fallback block being pushed (as the fix does) + task.assistantMessageContent.push({ + type: "tool_use" as const, + name: "unknown_tool" as any, + params: {}, + partial: false, + }) + + // This is the exact check from Task.ts ~line 3751 + const didToolUse = task.assistantMessageContent.some( + (block) => block.type === "tool_use" || block.type === "mcp_tool_use", + ) + + expect(didToolUse).toBe(true) + }) + + it("should use chunk.name when available in fallback block", () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const chunk = { + type: "tool_call" as const, + id: "call_with_name", + name: "some_bad_tool", + arguments: "{invalid json", + } + + // parseToolCall returns null for invalid JSON or unrecognized tool + const toolUse = NativeToolCallParser.parseToolCall({ + id: chunk.id, + name: chunk.name as any, + arguments: chunk.arguments, + }) + expect(toolUse).toBeNull() + + // The fallback should use chunk.name + const fallback: any = { + type: "tool_use" as const, + name: (chunk.name ?? "unknown_tool") as any, + params: {}, + partial: false, + } + fallback.id = chunk.id + task.assistantMessageContent.push(fallback) + + expect((task.assistantMessageContent[0] as any).name).toBe("some_bad_tool") + }) + + it("should not affect the streaming path (tool_call_start pushes block independently)", () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + // The streaming path pushes a block at tool_call_start time, + // so even before parsing completes, the block exists. + // Simulate the streaming path pushing a partial tool_use block: + const partialToolUse: any = { + type: "tool_use" as const, + name: "read_file" as any, + params: {}, + partial: true, + id: "call_streaming_123", + } + task.assistantMessageContent.push(partialToolUse) + + // The didToolUse check should find it + const didToolUse = task.assistantMessageContent.some( + (block) => block.type === "tool_use" || block.type === "mcp_tool_use", + ) + expect(didToolUse).toBe(true) + + // Verify the streaming path block is intact + expect(task.assistantMessageContent[0]).toEqual(partialToolUse) + }) + }) })