fix(ask_followup_question): report non-array follow_up as a type error. Fixes #511 (#662)

A native tool call for `ask_followup_question` with a present-but-non-array
`follow_up` (e.g. a keyed object produced by incremental JSON parsing)
was forwarded unchecked and then rejected by the tool via
`recordMissingParamError`, which always reports the misleading message

    Missing value for required parameter follow_up

The value was the wrong type, not missing, which sent the model
into a retry loop with the same payload.

Fix:

AskFollowupQuestionTool.execute now distinguishes the cases:
`null`/`undefined` `follow_up` still reports a missing parameter,
while a present-but-non-array value reports a clear type/shape error
(surfaced to both the user via `say(error)` and the model via `toolError`)
instructing it to retry with a JSON array.

`NativeToolCallParser` keeps forwarding the raw follow_up value on the finalize path
(with an explanatory comment) rather than guarding with `Array.isArray`,
so the tool can emit the precise error instead of the call being silently dropped
to null by the generic parser guard.

Adds test coverage for string and keyed-object non-array follow_up values,
plus a parser finalize test confirming the value is forwarded rather than dropped.

Assisted-By: Claude Opus 4.8 in Zoo Code.

Co-authored-by: Naved  Merchant <naved.merchant@gmail.com>

Author: nh2

src/core/assistant-message/NativeToolCallParser.ts

@@ -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,

src/core/tools/AskFollowupQuestionTool.ts

@@ -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[]
 }
 
@@ -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,

src/core/tools/__tests__/askFollowupQuestionTool.spec.ts

@@ -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 =====
@@ -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" },
+				})
+			}
+		})
 	})
 })