diff --git a/src/core/assistant-message/NativeToolCallParser.ts b/src/core/assistant-message/NativeToolCallParser.ts index bda7c71eb8d..5d156ac94d4 100644 --- a/src/core/assistant-message/NativeToolCallParser.ts +++ b/src/core/assistant-message/NativeToolCallParser.ts @@ -475,9 +475,27 @@ export class NativeToolCallParser { case "ask_followup_question": if (partialArgs.question !== undefined || partialArgs.follow_up !== undefined) { + let coercedPartialFollowUp = partialArgs.follow_up + if (!Array.isArray(coercedPartialFollowUp) && typeof coercedPartialFollowUp === "string") { + try { + const parsed = JSON.parse(coercedPartialFollowUp) + coercedPartialFollowUp = Array.isArray(parsed) + ? parsed.map((s: any) => ({ text: s.text, mode: s.mode ?? null })) + : undefined + } catch { + coercedPartialFollowUp = undefined + } + } else if (Array.isArray(coercedPartialFollowUp)) { + coercedPartialFollowUp = coercedPartialFollowUp.map((s: any) => ({ + text: s.text, + mode: s.mode ?? null, + })) + } else { + coercedPartialFollowUp = undefined + } nativeArgs = { question: partialArgs.question, - follow_up: Array.isArray(partialArgs.follow_up) ? partialArgs.follow_up : undefined, + follow_up: coercedPartialFollowUp, } } break @@ -820,9 +838,28 @@ export class NativeToolCallParser { case "ask_followup_question": if (args.question !== undefined && args.follow_up !== undefined) { + let coercedFinalFollowUp = args.follow_up + if (!Array.isArray(coercedFinalFollowUp) && typeof coercedFinalFollowUp === "string") { + const trimmed = (coercedFinalFollowUp as string).trim() + if (trimmed.length > 0) { + try { + const parsed = JSON.parse(trimmed) + coercedFinalFollowUp = Array.isArray(parsed) + ? parsed.map((s: any) => ({ text: s.text, mode: s.mode ?? null })) + : [{ text: trimmed, mode: null }] + } catch { + coercedFinalFollowUp = [{ text: trimmed, mode: null }] + } + } + } else if (Array.isArray(coercedFinalFollowUp)) { + coercedFinalFollowUp = coercedFinalFollowUp.map((s: any) => ({ + text: s.text, + mode: s.mode ?? null, + })) + } nativeArgs = { question: args.question, - follow_up: args.follow_up, + follow_up: coercedFinalFollowUp, } as NativeArgsFor } break diff --git a/src/core/tools/AskFollowupQuestionTool.ts b/src/core/tools/AskFollowupQuestionTool.ts index 22cdbcf5de2..388b15a4b3c 100644 --- a/src/core/tools/AskFollowupQuestionTool.ts +++ b/src/core/tools/AskFollowupQuestionTool.ts @@ -6,7 +6,7 @@ import { BaseTool, ToolCallbacks } from "./BaseTool" interface Suggestion { text: string - mode?: string + mode?: string | null } interface AskFollowupQuestionParams { @@ -14,11 +14,58 @@ interface AskFollowupQuestionParams { follow_up: Suggestion[] } +/** + * Coerce a follow_up value from various formats into the expected Suggestion array. + * Some models (e.g. smaller Qwen models) output follow_up as a string instead of an array. + * This helper normalizes the value so the tool works regardless of the model's output format. + * + * Supported coercions: + * - Already an array: returned as-is + * - A JSON string that parses to an array: parsed and returned + * - A plain string: wrapped as a single suggestion `[{ text: value }]` + * - Anything else (null, undefined, number, etc.): returns undefined so callers can error + */ +export function coerceFollowUp(value: unknown): Suggestion[] | undefined { + if (Array.isArray(value)) { + return normalizeSuggestions(value) + } + + if (typeof value === "string" && value.trim().length > 0) { + // Try parsing as JSON first (model may have serialized the array as a string) + try { + const parsed = JSON.parse(value) + if (Array.isArray(parsed)) { + return normalizeSuggestions(parsed) + } + } catch { + // Not valid JSON -- fall through to plain-string wrapping + } + + // Wrap plain string as a single suggestion + return [{ text: value, mode: null }] + } + + return undefined +} + +/** + * Ensure every suggestion has an explicit `mode` field (defaulting to `null`). + * The tool schema requires `mode` on each item (`required: ["text", "mode"]`), + * and some model Jinja templates fail if a required field is absent. + */ +function normalizeSuggestions(items: Suggestion[]): Suggestion[] { + return items.map((s) => ({ + text: s.text, + mode: s.mode ?? null, + })) +} + export class AskFollowupQuestionTool extends BaseTool<"ask_followup_question"> { readonly name = "ask_followup_question" as const async execute(params: AskFollowupQuestionParams, task: Task, callbacks: ToolCallbacks): Promise { - const { question, follow_up } = params + const { question } = params + const follow_up = coerceFollowUp(params.follow_up) const { handleError, pushToolResult } = callbacks const recordMissingParamError = async (paramName: string): Promise => { diff --git a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts index 63bfad8a3d0..6be52ea6a4c 100644 --- a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts +++ b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts @@ -1,4 +1,4 @@ -import { askFollowupQuestionTool } from "../AskFollowupQuestionTool" +import { askFollowupQuestionTool, coerceFollowUp } from "../AskFollowupQuestionTool" import { ToolUse } from "../../../shared/tools" import { NativeToolCallParser } from "../../assistant-message/NativeToolCallParser" @@ -45,7 +45,7 @@ describe("askFollowupQuestionTool", () => { expect(mockCline.ask).toHaveBeenCalledWith( "followup", - expect.stringContaining('"suggest":[{"answer":"Option 1"},{"answer":"Option 2"}]'), + expect.stringContaining('"suggest":[{"answer":"Option 1","mode":null},{"answer":"Option 2","mode":null}]'), false, ) }) @@ -105,7 +105,7 @@ describe("askFollowupQuestionTool", () => { expect(mockCline.ask).toHaveBeenCalledWith( "followup", expect.stringContaining( - '"suggest":[{"answer":"Regular option"},{"answer":"Plan architecture","mode":"architect"}]', + '"suggest":[{"answer":"Regular option","mode":null},{"answer":"Plan architecture","mode":"architect"}]', ), false, ) @@ -166,7 +166,7 @@ describe("askFollowupQuestionTool", () => { expect(mockCline.ask).not.toHaveBeenCalled() }) - it("should handle non-array follow_up parameter", async () => { + it("should coerce a plain string follow_up into a single-item array", async () => { const block: ToolUse = { type: "tool_use", name: "ask_followup_question", @@ -186,14 +186,120 @@ describe("askFollowupQuestionTool", () => { pushToolResult: mockPushToolResult, }) + // Plain string should be coerced to [{ text: "not an array" }] + expect(mockCline.ask).toHaveBeenCalledWith( + "followup", + expect.stringContaining('"suggest":[{"answer":"not an array","mode":null}]'), + false, + ) + }) + + it("should coerce a JSON string array follow_up into a proper array", 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: '[{"text":"Option A"},{"text":"Option B","mode":"code"}]' as any, + } as any, + partial: false, + } + + await askFollowupQuestionTool.handle(mockCline, block as ToolUse<"ask_followup_question">, { + askApproval: vi.fn(), + handleError: vi.fn(), + pushToolResult: mockPushToolResult, + }) + + // JSON string should be parsed into a proper array + expect(mockCline.ask).toHaveBeenCalledWith( + "followup", + expect.stringContaining( + '"suggest":[{"answer":"Option A","mode":null},{"answer":"Option B","mode":"code"}]', + ), + false, + ) + }) + + it("should handle number follow_up parameter as missing", 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: 42 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("coerceFollowUp helper", () => { + it("should normalize arrays to include mode: null when missing", () => { + const input = [{ text: "Option 1" }, { text: "Option 2" }] + expect(coerceFollowUp(input)).toEqual([ + { text: "Option 1", mode: null }, + { text: "Option 2", mode: null }, + ]) + }) + + it("should preserve existing mode values in arrays", () => { + const input = [{ text: "Option 1", mode: "code" }, { text: "Option 2" }] + expect(coerceFollowUp(input)).toEqual([ + { text: "Option 1", mode: "code" }, + { text: "Option 2", mode: null }, + ]) + }) + + it("should parse a JSON string containing an array", () => { + const input = '[{"text":"A"},{"text":"B","mode":"code"}]' + expect(coerceFollowUp(input)).toEqual([ + { text: "A", mode: null }, + { text: "B", mode: "code" }, + ]) + }) + + it("should wrap a plain string as a single suggestion with mode: null", () => { + expect(coerceFollowUp("some option")).toEqual([{ text: "some option", mode: null }]) + }) + + it("should return undefined for null", () => { + expect(coerceFollowUp(null)).toBeUndefined() + }) + + it("should return undefined for undefined", () => { + expect(coerceFollowUp(undefined)).toBeUndefined() + }) + + it("should return undefined for empty string", () => { + expect(coerceFollowUp("")).toBeUndefined() + }) + + it("should return undefined for whitespace-only string", () => { + expect(coerceFollowUp(" ")).toBeUndefined() + }) + + it("should wrap a JSON string that parses to a non-array as a suggestion with mode: null", () => { + // A JSON string like '{"text":"hello"}' is valid JSON but not an array + expect(coerceFollowUp('{"text":"hello"}')).toEqual([{ text: '{"text":"hello"}', mode: null }]) + }) + }) + 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"> = { @@ -292,5 +398,51 @@ describe("askFollowupQuestionTool", () => { }) } }) + + it("should coerce string follow_up to array during finalization", () => { + NativeToolCallParser.startStreamingToolCall("call_789", "ask_followup_question") + + // Simulate a model that outputs follow_up as a plain string + const jsonWithStringFollowUp = '{"question":"Pick one","follow_up":"Option A"}' + NativeToolCallParser.processStreamingChunk("call_789", jsonWithStringFollowUp) + + const result = NativeToolCallParser.finalizeStreamingToolCall("call_789") + + expect(result).not.toBeNull() + expect(result?.type).toBe("tool_use") + if (result?.type === "tool_use") { + const nativeArgs = result.nativeArgs as { + question: string + follow_up: Array<{ text: string; mode?: string }> + } + expect(nativeArgs.question).toBe("Pick one") + expect(nativeArgs.follow_up).toEqual([{ text: "Option A", mode: null }]) + } + }) + + it("should coerce JSON-string follow_up to array during finalization", () => { + NativeToolCallParser.startStreamingToolCall("call_101", "ask_followup_question") + + // Simulate a model that outputs follow_up as a JSON string of an array + const jsonWithJsonStringFollowUp = + '{"question":"Pick one","follow_up":"[{\\"text\\":\\"A\\"},{\\"text\\":\\"B\\"}]"}' + NativeToolCallParser.processStreamingChunk("call_101", jsonWithJsonStringFollowUp) + + const result = NativeToolCallParser.finalizeStreamingToolCall("call_101") + + expect(result).not.toBeNull() + expect(result?.type).toBe("tool_use") + if (result?.type === "tool_use") { + const nativeArgs = result.nativeArgs as { + question: string + follow_up: Array<{ text: string; mode?: string }> + } + expect(nativeArgs.question).toBe("Pick one") + expect(nativeArgs.follow_up).toEqual([ + { text: "A", mode: null }, + { text: "B", mode: null }, + ]) + } + }) }) })