Skip to content

Commit 77ed60a

Browse files
authored
fix: add follow_up param validation in AskFollowupQuestionTool (#11484)
Added strict validation for the `follow_up` parameter to check for presence and type (Array). Added test cases covering missing, null, and invalid type scenarios. Refactored parameter error handling into a helper method to reduce duplication.
1 parent 6ef149d commit 77ed60a

2 files changed

Lines changed: 98 additions & 4 deletions

File tree

src/core/tools/AskFollowupQuestionTool.ts

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,21 @@ export class AskFollowupQuestionTool extends BaseTool<"ask_followup_question"> {
2121
const { question, follow_up } = params
2222
const { handleError, pushToolResult } = callbacks
2323

24+
const recordMissingParamError = async (paramName: string): Promise<void> => {
25+
task.consecutiveMistakeCount++
26+
task.recordToolError("ask_followup_question")
27+
task.didToolFailInCurrentTurn = true
28+
pushToolResult(await task.sayAndCreateMissingParamError("ask_followup_question", paramName))
29+
}
30+
2431
try {
2532
if (!question) {
26-
task.consecutiveMistakeCount++
27-
task.recordToolError("ask_followup_question")
28-
task.didToolFailInCurrentTurn = true
29-
pushToolResult(await task.sayAndCreateMissingParamError("ask_followup_question", "question"))
33+
await recordMissingParamError("question")
34+
return
35+
}
36+
37+
if (!follow_up || !Array.isArray(follow_up)) {
38+
await recordMissingParamError("follow_up")
3039
return
3140
}
3241

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

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ describe("askFollowupQuestionTool", () => {
1313
mockCline = {
1414
ask: vi.fn().mockResolvedValue({ text: "Test response" }),
1515
say: vi.fn().mockResolvedValue(undefined),
16+
sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"),
1617
consecutiveMistakeCount: 0,
18+
recordToolError: vi.fn(),
1719
}
1820

1921
mockPushToolResult = vi.fn((result) => {
@@ -109,6 +111,89 @@ describe("askFollowupQuestionTool", () => {
109111
)
110112
})
111113

114+
describe("parameter validation", () => {
115+
it("should handle missing follow_up parameter", async () => {
116+
const block: ToolUse = {
117+
type: "tool_use",
118+
name: "ask_followup_question",
119+
params: {
120+
question: "What would you like to do?",
121+
},
122+
nativeArgs: {
123+
question: "What would you like to do?",
124+
follow_up: undefined as any,
125+
},
126+
partial: false,
127+
}
128+
129+
await askFollowupQuestionTool.handle(mockCline, block as ToolUse<"ask_followup_question">, {
130+
askApproval: vi.fn(),
131+
handleError: vi.fn(),
132+
pushToolResult: mockPushToolResult,
133+
})
134+
135+
expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("ask_followup_question", "follow_up")
136+
expect(mockCline.recordToolError).toHaveBeenCalledWith("ask_followup_question")
137+
expect(mockCline.didToolFailInCurrentTurn).toBe(true)
138+
expect(mockCline.consecutiveMistakeCount).toBe(1)
139+
expect(mockCline.ask).not.toHaveBeenCalled()
140+
})
141+
142+
it("should handle null follow_up parameter", async () => {
143+
const block: ToolUse = {
144+
type: "tool_use",
145+
name: "ask_followup_question",
146+
params: {
147+
question: "What would you like to do?",
148+
},
149+
nativeArgs: {
150+
question: "What would you like to do?",
151+
follow_up: null as any,
152+
},
153+
partial: false,
154+
}
155+
156+
await askFollowupQuestionTool.handle(mockCline, block as ToolUse<"ask_followup_question">, {
157+
askApproval: vi.fn(),
158+
handleError: vi.fn(),
159+
pushToolResult: mockPushToolResult,
160+
})
161+
162+
expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("ask_followup_question", "follow_up")
163+
expect(mockCline.recordToolError).toHaveBeenCalledWith("ask_followup_question")
164+
expect(mockCline.didToolFailInCurrentTurn).toBe(true)
165+
expect(mockCline.consecutiveMistakeCount).toBe(1)
166+
expect(mockCline.ask).not.toHaveBeenCalled()
167+
})
168+
169+
it("should handle non-array follow_up parameter", async () => {
170+
const block: ToolUse = {
171+
type: "tool_use",
172+
name: "ask_followup_question",
173+
params: {
174+
question: "What would you like to do?",
175+
},
176+
nativeArgs: {
177+
question: "What would you like to do?",
178+
follow_up: "not an array" as any,
179+
} as any,
180+
partial: false,
181+
}
182+
183+
await askFollowupQuestionTool.handle(mockCline, block as ToolUse<"ask_followup_question">, {
184+
askApproval: vi.fn(),
185+
handleError: vi.fn(),
186+
pushToolResult: mockPushToolResult,
187+
})
188+
189+
expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("ask_followup_question", "follow_up")
190+
expect(mockCline.recordToolError).toHaveBeenCalledWith("ask_followup_question")
191+
expect(mockCline.didToolFailInCurrentTurn).toBe(true)
192+
expect(mockCline.consecutiveMistakeCount).toBe(1)
193+
expect(mockCline.ask).not.toHaveBeenCalled()
194+
})
195+
})
196+
112197
describe("handlePartial with native protocol", () => {
113198
it("should only send question during partial streaming to avoid raw JSON display", async () => {
114199
const block: ToolUse<"ask_followup_question"> = {

0 commit comments

Comments
 (0)