Skip to content
Merged
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
6 changes: 6 additions & 0 deletions src/core/assistant-message/NativeToolCallParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -819,6 +819,12 @@ export class NativeToolCallParser {
break

case "ask_followup_question":
// Require a question and a present follow_up. When follow_up is
// present-but-not-an-array (e.g. an object/string/number produced by
// incremental JSON parsing), we still construct nativeArgs and forward
// the raw value so the tool can emit a precise "must be an array" error
// instead of the generic parser failure, which would surface as a
// misleading "Missing value for required parameter 'follow_up'" error.
if (args.question !== undefined && args.follow_up !== undefined) {
nativeArgs = {
question: args.question,
Expand Down
25 changes: 24 additions & 1 deletion src/core/tools/AskFollowupQuestionTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ interface Suggestion {

interface AskFollowupQuestionParams {
question: string
// follow_up is typed as an array, but at runtime the value may arrive as a
// non-array (object/string/number) due to incremental JSON parsing, so the
// runtime validation in execute() guards against that explicitly.
follow_up: Suggestion[]
}

Expand All @@ -29,17 +32,37 @@ export class AskFollowupQuestionTool extends BaseTool<"ask_followup_question"> {
pushToolResult(await task.sayAndCreateMissingParamError("ask_followup_question", paramName))
}

const recordValidationError = async (message: string): Promise<void> => {
task.consecutiveMistakeCount++
task.recordToolError("ask_followup_question")
task.didToolFailInCurrentTurn = true
await task.say("error", message)
pushToolResult(formatResponse.toolError(message))
}

try {
if (!question) {
await recordMissingParamError("question")
return
}

if (!follow_up || !Array.isArray(follow_up)) {
// Truly missing follow_up (null/undefined) -> report as a missing parameter.
if (follow_up === undefined || follow_up === null) {
await recordMissingParamError("follow_up")
return
}

// Present-but-wrong-type follow_up (object/string/number) -> report a clear
// type/shape error rather than the misleading "Missing value" message, so the
// model can correct it instead of looping with the same payload.
if (!Array.isArray(follow_up)) {
await recordValidationError(
"The 'follow_up' parameter must be an array of suggestion objects, each shaped like { text: string, mode?: string }. " +
"Retry with 'follow_up' as a JSON array.",
)
return
}

// Transform follow_up suggestions to the format expected by task.ask
const follow_up_json = {
question,
Expand Down
58 changes: 56 additions & 2 deletions src/core/tools/__tests__/askFollowupQuestionTool.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,43 @@ describe("AskFollowupQuestionTool", () => {
expect(mockTask.sayAndCreateMissingParamError).toHaveBeenCalledWith("ask_followup_question", "follow_up")
})

it("should handle follow_up that is not an array", async () => {
it("should report a type error when follow_up is a string", async () => {
const params = { question: "What?", follow_up: "not-an-array" as any }

await tool.execute(params, mockTask, mockCallbacks)

expect(mockTask.consecutiveMistakeCount).toBe(1)
expect(mockTask.recordToolError).toHaveBeenCalledWith("ask_followup_question")
expect(mockTask.didToolFailInCurrentTurn).toBe(true)
expect(mockTask.sayAndCreateMissingParamError).toHaveBeenCalledWith("ask_followup_question", "follow_up")
// A present-but-non-array value is a type error.
expect(mockTask.sayAndCreateMissingParamError).not.toHaveBeenCalled()
const pushed = (mockCallbacks.pushToolResult as any).mock.calls[0][0]
expect(pushed).toContain("must be an array")
expect(pushed).not.toContain("Missing value")
})

it("should report a type error when follow_up is an object", async () => {
// Reproduces the issue: follow_up arrives as a keyed object instead of an array.
const params = {
question: "How should I proceed?",
follow_up: {
"0": { mode: null, text: "Keep the guard" },
"1": { mode: null, text: "Remove the guard" },
} as any,
}

await tool.execute(params, mockTask, mockCallbacks)

expect(mockTask.consecutiveMistakeCount).toBe(1)
expect(mockTask.recordToolError).toHaveBeenCalledWith("ask_followup_question")
expect(mockTask.didToolFailInCurrentTurn).toBe(true)
expect(mockTask.sayAndCreateMissingParamError).not.toHaveBeenCalled()
expect(mockTask.say).toHaveBeenCalledWith("error", expect.stringContaining("must be an array"))
const pushed = (mockCallbacks.pushToolResult as any).mock.calls[0][0]
expect(pushed).toContain("must be an array")
expect(pushed).not.toContain("Missing value")
// The tool must not proceed to ask the user with an invalid payload.
expect(mockTask.ask).not.toHaveBeenCalled()
})

// ===== Happy path tests =====
Expand Down Expand Up @@ -513,5 +541,31 @@ describe("AskFollowupQuestionTool", () => {
})
}
})

it("should finalize and forward a non-array follow_up so the tool can report it", () => {
NativeToolCallParser.startStreamingToolCall("call_789", "ask_followup_question")

// follow_up arrives as a keyed object instead of an array (the bug repro).
const completeJson =
'{"question":"How should I proceed?","follow_up":{"0":{"mode":null,"text":"Keep"},"1":{"mode":null,"text":"Remove"}}}'
NativeToolCallParser.processStreamingChunk("call_789", completeJson)

const result = NativeToolCallParser.finalizeStreamingToolCall("call_789")

// The call must NOT be dropped (null) - it should reach the tool with the raw
// value so the tool can emit a precise "must be an array" error.
expect(result).not.toBeNull()
expect(result?.type).toBe("tool_use")
expect(result?.name).toBe("ask_followup_question")
if (result?.type === "tool_use") {
const nativeArgs = result.nativeArgs as { question: string; follow_up: unknown }
expect(nativeArgs.question).toBe("How should I proceed?")
expect(Array.isArray(nativeArgs.follow_up)).toBe(false)
expect(nativeArgs.follow_up).toEqual({
"0": { mode: null, text: "Keep" },
"1": { mode: null, text: "Remove" },
})
}
})
})
})
Loading