From 77ed60a1731e3317474e45b069e743d646e30980 Mon Sep 17 00:00:00 2001 From: rossdonald <49425722+rossdonald@users.noreply.github.com> Date: Tue, 17 Feb 2026 04:14:37 +1100 Subject: [PATCH] fix: add follow_up param validation in AskFollowupQuestionTool (#11484) Added strict validation for the `follow_up` parameter to check for presence and type (Array). Added test cases covering missing, null, and invalid type scenarios. Refactored parameter error handling into a helper method to reduce duplication. --- src/core/tools/AskFollowupQuestionTool.ts | 17 +++- .../__tests__/askFollowupQuestionTool.spec.ts | 85 +++++++++++++++++++ 2 files changed, 98 insertions(+), 4 deletions(-) diff --git a/src/core/tools/AskFollowupQuestionTool.ts b/src/core/tools/AskFollowupQuestionTool.ts index 010a6240f1..22cdbcf5de 100644 --- a/src/core/tools/AskFollowupQuestionTool.ts +++ b/src/core/tools/AskFollowupQuestionTool.ts @@ -21,12 +21,21 @@ export class AskFollowupQuestionTool extends BaseTool<"ask_followup_question"> { const { question, follow_up } = params const { handleError, pushToolResult } = callbacks + const recordMissingParamError = async (paramName: string): Promise => { + task.consecutiveMistakeCount++ + task.recordToolError("ask_followup_question") + task.didToolFailInCurrentTurn = true + pushToolResult(await task.sayAndCreateMissingParamError("ask_followup_question", paramName)) + } + try { if (!question) { - task.consecutiveMistakeCount++ - task.recordToolError("ask_followup_question") - task.didToolFailInCurrentTurn = true - pushToolResult(await task.sayAndCreateMissingParamError("ask_followup_question", "question")) + await recordMissingParamError("question") + return + } + + if (!follow_up || !Array.isArray(follow_up)) { + await recordMissingParamError("follow_up") return } diff --git a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts index e13f639ba0..63bfad8a3d 100644 --- a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts +++ b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts @@ -13,7 +13,9 @@ describe("askFollowupQuestionTool", () => { mockCline = { ask: vi.fn().mockResolvedValue({ text: "Test response" }), say: vi.fn().mockResolvedValue(undefined), + sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"), consecutiveMistakeCount: 0, + recordToolError: vi.fn(), } mockPushToolResult = vi.fn((result) => { @@ -109,6 +111,89 @@ describe("askFollowupQuestionTool", () => { ) }) + describe("parameter validation", () => { + it("should handle missing follow_up parameter", async () => { + const block: ToolUse = { + type: "tool_use", + name: "ask_followup_question", + params: { + question: "What would you like to do?", + }, + nativeArgs: { + question: "What would you like to do?", + follow_up: undefined as any, + }, + partial: false, + } + + await askFollowupQuestionTool.handle(mockCline, block as ToolUse<"ask_followup_question">, { + askApproval: vi.fn(), + handleError: vi.fn(), + pushToolResult: mockPushToolResult, + }) + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("ask_followup_question", "follow_up") + expect(mockCline.recordToolError).toHaveBeenCalledWith("ask_followup_question") + expect(mockCline.didToolFailInCurrentTurn).toBe(true) + expect(mockCline.consecutiveMistakeCount).toBe(1) + expect(mockCline.ask).not.toHaveBeenCalled() + }) + + it("should handle null follow_up parameter", async () => { + const block: ToolUse = { + type: "tool_use", + name: "ask_followup_question", + params: { + question: "What would you like to do?", + }, + nativeArgs: { + question: "What would you like to do?", + follow_up: null as any, + }, + partial: false, + } + + await askFollowupQuestionTool.handle(mockCline, block as ToolUse<"ask_followup_question">, { + askApproval: vi.fn(), + handleError: vi.fn(), + pushToolResult: mockPushToolResult, + }) + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("ask_followup_question", "follow_up") + expect(mockCline.recordToolError).toHaveBeenCalledWith("ask_followup_question") + expect(mockCline.didToolFailInCurrentTurn).toBe(true) + expect(mockCline.consecutiveMistakeCount).toBe(1) + expect(mockCline.ask).not.toHaveBeenCalled() + }) + + it("should handle non-array follow_up parameter", async () => { + const block: ToolUse = { + type: "tool_use", + name: "ask_followup_question", + params: { + question: "What would you like to do?", + }, + nativeArgs: { + question: "What would you like to do?", + follow_up: "not an array" as any, + } as any, + partial: false, + } + + await askFollowupQuestionTool.handle(mockCline, block as ToolUse<"ask_followup_question">, { + askApproval: vi.fn(), + handleError: vi.fn(), + pushToolResult: mockPushToolResult, + }) + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("ask_followup_question", "follow_up") + expect(mockCline.recordToolError).toHaveBeenCalledWith("ask_followup_question") + expect(mockCline.didToolFailInCurrentTurn).toBe(true) + expect(mockCline.consecutiveMistakeCount).toBe(1) + expect(mockCline.ask).not.toHaveBeenCalled() + }) + }) + describe("handlePartial with native protocol", () => { it("should only send question during partial streaming to avoid raw JSON display", async () => { const block: ToolUse<"ask_followup_question"> = {