diff --git a/src/core/assistant-message/NativeToolCallParser.ts b/src/core/assistant-message/NativeToolCallParser.ts index bda7c71eb8..9639ae1baa 100644 --- a/src/core/assistant-message/NativeToolCallParser.ts +++ b/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, diff --git a/src/core/tools/AskFollowupQuestionTool.ts b/src/core/tools/AskFollowupQuestionTool.ts index 857ee7c152..3baea7ed52 100644 --- a/src/core/tools/AskFollowupQuestionTool.ts +++ b/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 => { + 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, diff --git a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts index 9bac90067b..4a035447b1 100644 --- a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts +++ b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts @@ -83,7 +83,7 @@ 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) @@ -91,7 +91,35 @@ describe("AskFollowupQuestionTool", () => { 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" }, + }) + } + }) }) })