From 967fa19890c79c1fa5f263b1a70cc3e1625e2547 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Thu, 30 Apr 2026 03:18:01 +0000 Subject: [PATCH 1/2] fix: coerce string follow_up to array in ask_followup_question tool Some models (e.g. Qwen3.6 35B-A3B) output the follow_up parameter as a string instead of the required array format. This adds a coerceFollowUp helper that normalizes: - JSON strings that parse to arrays - Plain strings (wrapped as single suggestion) - Existing arrays (passed through unchanged) Applied in AskFollowupQuestionTool.execute() and both partial/finalize handlers in NativeToolCallParser. Closes #12233 --- .../assistant-message/NativeToolCallParser.ts | 27 +++- src/core/tools/AskFollowupQuestionTool.ts | 37 ++++- .../__tests__/askFollowupQuestionTool.spec.ts | 143 +++++++++++++++++- 3 files changed, 199 insertions(+), 8 deletions(-) diff --git a/src/core/assistant-message/NativeToolCallParser.ts b/src/core/assistant-message/NativeToolCallParser.ts index bda7c71eb8d..df6619f405f 100644 --- a/src/core/assistant-message/NativeToolCallParser.ts +++ b/src/core/assistant-message/NativeToolCallParser.ts @@ -475,9 +475,20 @@ 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 : undefined + } catch { + coercedPartialFollowUp = undefined + } + } else if (!Array.isArray(coercedPartialFollowUp)) { + coercedPartialFollowUp = undefined + } nativeArgs = { question: partialArgs.question, - follow_up: Array.isArray(partialArgs.follow_up) ? partialArgs.follow_up : undefined, + follow_up: coercedPartialFollowUp, } } break @@ -820,9 +831,21 @@ 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 : [{ text: trimmed }] + } catch { + coercedFinalFollowUp = [{ text: trimmed }] + } + } + } 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..93a539ae93f 100644 --- a/src/core/tools/AskFollowupQuestionTool.ts +++ b/src/core/tools/AskFollowupQuestionTool.ts @@ -14,11 +14,46 @@ 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 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 parsed + } + } catch { + // Not valid JSON -- fall through to plain-string wrapping + } + + // Wrap plain string as a single suggestion + return [{ text: value }] + } + + return undefined +} + 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..fcdaaf64284 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" @@ -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,104 @@ 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"}]'), + 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"},{"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 return arrays as-is", () => { + const input = [{ text: "Option 1" }, { text: "Option 2" }] + expect(coerceFollowUp(input)).toEqual(input) + }) + + it("should parse a JSON string containing an array", () => { + const input = '[{"text":"A"},{"text":"B","mode":"code"}]' + expect(coerceFollowUp(input)).toEqual([{ text: "A" }, { text: "B", mode: "code" }]) + }) + + it("should wrap a plain string as a single suggestion", () => { + expect(coerceFollowUp("some option")).toEqual([{ text: "some option" }]) + }) + + 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", () => { + // A JSON string like '{"text":"hello"}' is valid JSON but not an array + expect(coerceFollowUp('{"text":"hello"}')).toEqual([{ text: '{"text":"hello"}' }]) + }) + }) + 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 +382,48 @@ 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" }]) + } + }) + + 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" }, { text: "B" }]) + } + }) }) }) From d012af9a0eed560268ed59fa07d3555295ffd4be Mon Sep 17 00:00:00 2001 From: Roo Code Date: Thu, 30 Apr 2026 11:15:10 +0000 Subject: [PATCH 2/2] fix: ensure coerced follow_up suggestions always include mode field The tool schema requires mode on each suggestion item (required: ["text", "mode"]). When follow_up was coerced from a string, the mode field was missing entirely. Some model Jinja templates (e.g. Qwen via llama.cpp) fail with "Cannot convert value of type Optional to Jinja Value" when a required field is absent from tool call arguments in conversation history. Now all coercion paths (coerceFollowUp helper + NativeToolCallParser partial and finalization handlers) normalize every suggestion to include mode: null when not explicitly set. --- .../assistant-message/NativeToolCallParser.ts | 22 +++++++-- src/core/tools/AskFollowupQuestionTool.ts | 20 +++++++-- .../__tests__/askFollowupQuestionTool.spec.ts | 45 +++++++++++++------ 3 files changed, 66 insertions(+), 21 deletions(-) diff --git a/src/core/assistant-message/NativeToolCallParser.ts b/src/core/assistant-message/NativeToolCallParser.ts index df6619f405f..5d156ac94d4 100644 --- a/src/core/assistant-message/NativeToolCallParser.ts +++ b/src/core/assistant-message/NativeToolCallParser.ts @@ -479,11 +479,18 @@ export class NativeToolCallParser { if (!Array.isArray(coercedPartialFollowUp) && typeof coercedPartialFollowUp === "string") { try { const parsed = JSON.parse(coercedPartialFollowUp) - coercedPartialFollowUp = Array.isArray(parsed) ? parsed : undefined + coercedPartialFollowUp = Array.isArray(parsed) + ? parsed.map((s: any) => ({ text: s.text, mode: s.mode ?? null })) + : undefined } catch { coercedPartialFollowUp = undefined } - } else if (!Array.isArray(coercedPartialFollowUp)) { + } else if (Array.isArray(coercedPartialFollowUp)) { + coercedPartialFollowUp = coercedPartialFollowUp.map((s: any) => ({ + text: s.text, + mode: s.mode ?? null, + })) + } else { coercedPartialFollowUp = undefined } nativeArgs = { @@ -837,11 +844,18 @@ export class NativeToolCallParser { if (trimmed.length > 0) { try { const parsed = JSON.parse(trimmed) - coercedFinalFollowUp = Array.isArray(parsed) ? parsed : [{ text: trimmed }] + coercedFinalFollowUp = Array.isArray(parsed) + ? parsed.map((s: any) => ({ text: s.text, mode: s.mode ?? null })) + : [{ text: trimmed, mode: null }] } catch { - coercedFinalFollowUp = [{ text: trimmed }] + 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, diff --git a/src/core/tools/AskFollowupQuestionTool.ts b/src/core/tools/AskFollowupQuestionTool.ts index 93a539ae93f..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 { @@ -27,7 +27,7 @@ interface AskFollowupQuestionParams { */ export function coerceFollowUp(value: unknown): Suggestion[] | undefined { if (Array.isArray(value)) { - return value + return normalizeSuggestions(value) } if (typeof value === "string" && value.trim().length > 0) { @@ -35,19 +35,31 @@ export function coerceFollowUp(value: unknown): Suggestion[] | undefined { try { const parsed = JSON.parse(value) if (Array.isArray(parsed)) { - return parsed + return normalizeSuggestions(parsed) } } catch { // Not valid JSON -- fall through to plain-string wrapping } // Wrap plain string as a single suggestion - return [{ text: value }] + 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 diff --git a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts index fcdaaf64284..6be52ea6a4c 100644 --- a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts +++ b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts @@ -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, ) @@ -189,7 +189,7 @@ describe("askFollowupQuestionTool", () => { // Plain string should be coerced to [{ text: "not an array" }] expect(mockCline.ask).toHaveBeenCalledWith( "followup", - expect.stringContaining('"suggest":[{"answer":"not an array"}]'), + expect.stringContaining('"suggest":[{"answer":"not an array","mode":null}]'), false, ) }) @@ -217,7 +217,9 @@ describe("askFollowupQuestionTool", () => { // JSON string should be parsed into a proper array expect(mockCline.ask).toHaveBeenCalledWith( "followup", - expect.stringContaining('"suggest":[{"answer":"Option A"},{"answer":"Option B","mode":"code"}]'), + expect.stringContaining( + '"suggest":[{"answer":"Option A","mode":null},{"answer":"Option B","mode":"code"}]', + ), false, ) }) @@ -248,18 +250,32 @@ describe("askFollowupQuestionTool", () => { }) describe("coerceFollowUp helper", () => { - it("should return arrays as-is", () => { + it("should normalize arrays to include mode: null when missing", () => { const input = [{ text: "Option 1" }, { text: "Option 2" }] - expect(coerceFollowUp(input)).toEqual(input) + 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" }, { 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", () => { - expect(coerceFollowUp("some option")).toEqual([{ text: "some option" }]) + 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", () => { @@ -278,9 +294,9 @@ describe("askFollowupQuestionTool", () => { expect(coerceFollowUp(" ")).toBeUndefined() }) - it("should wrap a JSON string that parses to a non-array as a suggestion", () => { + 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"}' }]) + expect(coerceFollowUp('{"text":"hello"}')).toEqual([{ text: '{"text":"hello"}', mode: null }]) }) }) @@ -400,7 +416,7 @@ describe("askFollowupQuestionTool", () => { follow_up: Array<{ text: string; mode?: string }> } expect(nativeArgs.question).toBe("Pick one") - expect(nativeArgs.follow_up).toEqual([{ text: "Option A" }]) + expect(nativeArgs.follow_up).toEqual([{ text: "Option A", mode: null }]) } }) @@ -422,7 +438,10 @@ describe("askFollowupQuestionTool", () => { follow_up: Array<{ text: string; mode?: string }> } expect(nativeArgs.question).toBe("Pick one") - expect(nativeArgs.follow_up).toEqual([{ text: "A" }, { text: "B" }]) + expect(nativeArgs.follow_up).toEqual([ + { text: "A", mode: null }, + { text: "B", mode: null }, + ]) } }) })