Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions src/core/assistant-message/NativeToolCallParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<TName>
}
break
Expand Down
51 changes: 49 additions & 2 deletions src/core/tools/AskFollowupQuestionTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,66 @@ import { BaseTool, ToolCallbacks } from "./BaseTool"

interface Suggestion {
text: string
mode?: string
mode?: string | null
}

interface AskFollowupQuestionParams {
question: string
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<void> {
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<void> => {
Expand Down
166 changes: 159 additions & 7 deletions src/core/tools/__tests__/askFollowupQuestionTool.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { askFollowupQuestionTool } from "../AskFollowupQuestionTool"
import { askFollowupQuestionTool, coerceFollowUp } from "../AskFollowupQuestionTool"
import { ToolUse } from "../../../shared/tools"
import { NativeToolCallParser } from "../../assistant-message/NativeToolCallParser"

Expand Down Expand Up @@ -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,
)
})
Expand Down Expand Up @@ -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,
)
Expand Down Expand Up @@ -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",
Expand All @@ -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"> = {
Expand Down Expand Up @@ -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 },
])
}
})
})
})
Loading