mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-05 08:10:14 +00:00
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
This commit is contained in:
parent
ad25634905
commit
967fa19890
3 changed files with 199 additions and 8 deletions
|
|
@ -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<TName>
|
||||
}
|
||||
break
|
||||
|
|
|
|||
|
|
@ -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<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> => {
|
||||
|
|
|
|||
|
|
@ -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" }])
|
||||
}
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue