feat: update read_file tool tests to handle user feedback and approval states

samhvw8 · samhvw8 · commit 4a676fbd2c6b · 2025-05-07T20:18:53.000+07:00
diff --git a/src/core/__tests__/read-file-maxReadFileLine.test.ts b/src/core/__tests__/read-file-maxReadFileLine.test.ts
@@ -115,13 +115,14 @@ describe("read_file tool with maxReadFileLine setting", () => {
 			validateAccess: jest.fn().mockReturnValue(true),
 		}
 		mockCline.say = jest.fn().mockResolvedValue(undefined)
-		mockCline.ask = jest.fn().mockResolvedValue(true)
+		mockCline.ask = jest.fn().mockResolvedValue({ response: "yesButtonClicked" })
 		mockCline.presentAssistantMessage = jest.fn()
 		mockCline.getFileContextTracker = jest.fn().mockReturnValue({
 			trackFileContext: jest.fn().mockResolvedValue(undefined),
 		})
 		mockCline.recordToolUsage = jest.fn().mockReturnValue(undefined)
 		mockCline.recordToolError = jest.fn().mockReturnValue(undefined)
+		mockCline.didRejectTool = false
 		// Reset tool result
 		toolResult = undefined
 	})
@@ -257,6 +258,7 @@ describe("read_file tool with maxReadFileLine setting", () => {
 		it("should return an empty content with source code definitions", async () => {
 			// Setup - for maxReadFileLine = 0, the implementation won't call readLines
 			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef)
+			mockedCountFileLines.mockResolvedValue(5)
 
 			// Execute - skip addLineNumbers check as it's not called for maxReadFileLine=0
 			const result = await executeReadFileTool(
@@ -286,6 +288,7 @@ describe("read_file tool with maxReadFileLine setting", () => {
 			// Setup
 			mockedIsBinaryFile.mockResolvedValue(true)
 			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue("")
+			mockedCountFileLines.mockResolvedValue(0)
 
 			// Execute
 			const result = await executeReadFileTool({}, { maxReadFileLine: 0 })
diff --git a/src/core/__tests__/read-file-tool.test.ts b/src/core/__tests__/read-file-tool.test.ts
@@ -43,7 +43,7 @@ describe("read_file tool functionality", () => {
 			validateAccess: jest.fn().mockReturnValue(true),
 		},
 		say: jest.fn().mockResolvedValue(undefined),
-		ask: jest.fn().mockResolvedValue(true),
+		ask: jest.fn().mockResolvedValue({ response: "yesButtonClicked" }),
 		presentAssistantMessage: jest.fn(),
 		getFileContextTracker: jest.fn().mockReturnValue({
 			trackFileContext: jest.fn().mockResolvedValue(undefined),
@@ -52,21 +52,36 @@ describe("read_file tool functionality", () => {
 		recordToolError: jest.fn(),
 		sayAndCreateMissingParamError: jest.fn().mockResolvedValue("Missing required parameter"),
 		consecutiveMistakeCount: 0,
+		didRejectTool: false,
 	}
 
 	beforeEach(() => {
 		jest.clearAllMocks()
 		mockCline.consecutiveMistakeCount = 0
+		mockCline.didRejectTool = false
+
+		// Setup default mocks
 		;(extractTextFromFile as jest.Mock).mockImplementation(() => Promise.resolve("Test content"))
 		;(readLines as jest.Mock).mockImplementation(() => Promise.resolve("Test content"))
-		;(addLineNumbers as jest.Mock).mockImplementation((text, startLine = 1) => {
+		;(addLineNumbers as jest.Mock).mockImplementation((text: string) => {
+			if (!text) return ""
 			return text
 				.split("\n")
-				.map((line: string, i: number) => `${startLine + i} | ${line}`)
+				.map((line: string, i: number) => `${i + 1} | ${line}`)
 				.join("\n")
 		})
 		;(isBinaryFile as jest.Mock).mockResolvedValue(false)
 		;(parseSourceCodeDefinitionsForFile as jest.Mock).mockResolvedValue("")
+		mockCline.ask.mockResolvedValue({
+			response: "yesButtonClicked",
+			text: "Looks good!",
+			images: ["image1.png"],
+		})
+
+		// Reset mock implementations
+		;(extractTextFromFile as jest.Mock).mockClear()
+		;(readLines as jest.Mock).mockClear()
+		;(addLineNumbers as jest.Mock).mockClear()
 	})
 
 	describe("Args Parameter Format", () => {
@@ -473,6 +488,203 @@ describe("read_file tool functionality", () => {
 				`<files>\n<file><path>src/code.ts</path><error>Error reading file: Parser error</error></file>\n</files>`,
 			)
 		})
+
+		describe("User Feedback Handling", () => {
+			it("should handle approval with feedback", async () => {
+				const toolUse: ReadFileToolUse = {
+					type: "tool_use",
+					name: "read_file",
+					params: {
+						args: `<file><path>src/app.ts</path></file>`,
+					},
+					partial: false,
+				}
+
+				const { readFileTool } = require("../tools/readFileTool")
+
+				;(countFileLines as jest.Mock).mockResolvedValue(10)
+				;(readLines as jest.Mock).mockResolvedValue("Test content")
+				;(addLineNumbers as jest.Mock).mockReturnValue("1 | Test content")
+				mockCline.ask.mockResolvedValueOnce({
+					response: "yesButtonClicked",
+					text: "Looks good!",
+					images: ["image1.png"],
+				})
+
+				let result: string | undefined
+				await readFileTool(
+					mockCline,
+					toolUse,
+					mockCline.ask,
+					jest.fn(),
+					(r: string) => {
+						result = r
+					},
+					(param: string, value: string) => value,
+				)
+
+				const expectedXml = `<files>\n<file><path>src/app.ts</path>\n<content lines="1-10">\n1 | Test content</content>\n</file>\n</files>`
+				expect(result).toBe(
+					`The tool was approved with feedback:\n<feedback>\nLooks good!\n</feedback>\n${expectedXml}`,
+				)
+				expect(mockCline.say).toHaveBeenCalledWith("user_feedback", "Looks good!", ["image1.png"])
+			})
+
+			it("should handle denial with feedback", async () => {
+				const toolUse: ReadFileToolUse = {
+					type: "tool_use",
+					name: "read_file",
+					params: {
+						args: `<file><path>src/app.ts</path></file>`,
+					},
+					partial: false,
+				}
+
+				const { readFileTool } = require("../tools/readFileTool")
+
+				mockCline.ask.mockResolvedValueOnce({
+					response: "noButtonClicked",
+					text: "Not allowed",
+					images: ["image2.png"],
+				})
+
+				let result: string | undefined
+				await readFileTool(
+					mockCline,
+					toolUse,
+					mockCline.ask,
+					jest.fn(),
+					(r: string) => {
+						result = r
+					},
+					(param: string, value: string) => value,
+				)
+
+				const expectedXml = `<files>\n<file><path>src/app.ts</path><status>Denied by user</status></file>\n</files>`
+				expect(result).toBe(
+					`The user denied this operation and provided the following feedback:\n<feedback>\nNot allowed\n</feedback>\n${expectedXml}`,
+				)
+				expect(mockCline.say).toHaveBeenCalledWith("user_feedback", "Not allowed", ["image2.png"])
+				expect(mockCline.didRejectTool).toBe(true)
+			})
+		})
+
+		describe("Invalid Line Range Validation", () => {
+			it("should handle end line less than start line", async () => {
+				const toolUse: ReadFileToolUse = {
+					type: "tool_use",
+					name: "read_file",
+					params: {
+						args: `<file><path>src/app.ts</path><line_range>10-5</line_range></file>`,
+					},
+					partial: false,
+				}
+
+				const { readFileTool } = require("../tools/readFileTool")
+
+				;(countFileLines as jest.Mock).mockResolvedValue(10)
+				;(readLines as jest.Mock).mockRejectedValue(
+					new Error("Invalid line range: end line cannot be less than start line"),
+				)
+				mockCline.ask.mockResolvedValueOnce({ response: "yesButtonClicked" })
+
+				let result: string | undefined
+				await readFileTool(
+					mockCline,
+					toolUse,
+					mockCline.ask,
+					jest.fn(),
+					(r: string) => {
+						result = r
+					},
+					(param: string, value: string) => value,
+				)
+
+				expect(result).toBe(
+					`<files>\n<file><path>src/app.ts</path><error>Error reading file: Invalid line range: end line cannot be less than start line</error></file>\n</files>`,
+				)
+			})
+
+			it("should handle NaN line values", async () => {
+				const toolUse: ReadFileToolUse = {
+					type: "tool_use",
+					name: "read_file",
+					params: {
+						args: `<file><path>src/app.ts</path><line_range>abc-def</line_range></file>`,
+					},
+					partial: false,
+				}
+
+				const { readFileTool } = require("../tools/readFileTool")
+
+				;(countFileLines as jest.Mock).mockResolvedValue(10)
+				;(readLines as jest.Mock).mockRejectedValue(new Error("Invalid line range values"))
+				mockCline.ask.mockResolvedValueOnce({ response: "yesButtonClicked" })
+
+				let result: string | undefined
+				await readFileTool(
+					mockCline,
+					toolUse,
+					mockCline.ask,
+					jest.fn(),
+					(r: string) => {
+						result = r
+					},
+					(param: string, value: string) => value,
+				)
+
+				expect(result).toBe(
+					`<files>\n<file><path>src/app.ts</path><error>Error reading file: Invalid line range values</error></file>\n</files>`,
+				)
+			})
+		})
+
+		describe("Multiple File States", () => {
+			it("should handle mixed approval states", async () => {
+				const toolUse: ReadFileToolUse = {
+					type: "tool_use",
+					name: "read_file",
+					params: {
+						args: `<file><path>src/app.ts</path></file><file><path>src/blocked.ts</path></file>`,
+					},
+					partial: false,
+				}
+
+				const { readFileTool } = require("../tools/readFileTool")
+
+				;(countFileLines as jest.Mock).mockResolvedValue(10)
+
+				// First file approved
+				mockCline.ask.mockResolvedValueOnce({
+					response: "yesButtonClicked",
+					text: "First approved",
+				})
+
+				// Second file denied
+				mockCline.ask.mockResolvedValueOnce({
+					response: "noButtonClicked",
+					text: "Second denied",
+				})
+
+				let result: string | undefined
+				await readFileTool(
+					mockCline,
+					toolUse,
+					mockCline.ask,
+					jest.fn(),
+					(r: string) => {
+						result = r
+					},
+					(param: string, value: string) => value,
+				)
+
+				const expectedXml = `<files>\n<file><path>src/app.ts</path>\n<content lines="1-10">\n1 | Test content</content>\n</file>\n<file><path>src/blocked.ts</path><status>Denied by user</status></file>\n</files>`
+				expect(result).toBe(
+					`The user denied this operation and provided the following feedback:\n<feedback>\nSecond denied\n</feedback>\n${expectedXml}`,
+				)
+				expect(mockCline.didRejectTool).toBe(true)
+			})
+		})
 	})
 
 	describe("Performance Edge Cases", () => {
@@ -490,6 +702,12 @@ describe("read_file tool functionality", () => {
 
 			;(countFileLines as jest.Mock).mockResolvedValue(1000000)
 			;(readLines as jest.Mock).mockResolvedValue("First 500 lines")
+			;(addLineNumbers as jest.Mock).mockReturnValue("1 | First 500 lines")
+			mockCline.ask.mockResolvedValueOnce({
+				response: "yesButtonClicked",
+				text: "Looks good!",
+				images: ["image1.png"],
+			})
 
 			let result: string | undefined
 			await readFileTool(
@@ -503,7 +721,10 @@ describe("read_file tool functionality", () => {
 				(param: string, value: string) => value,
 			)
 
-			expect(result).toContain("<notice>Showing only 500 of 1000000 total lines")
+			const expectedXml = `<files>\n<file><path>huge.log</path>\n<content lines="1-500">\n1 | First 500 lines</content>\n<notice>Showing only 500 of 1000000 total lines. Use line_range if you need to read more lines</notice>\n</file>\n</files>`
+			expect(result).toBe(
+				`The tool was approved with feedback:\n<feedback>\nLooks good!\n</feedback>\n${expectedXml}`,
+			)
 			expect(extractTextFromFile).not.toHaveBeenCalled()
 			expect(readLines).toHaveBeenCalled()
 		})
diff --git a/src/core/__tests__/read-file-xml.test.ts b/src/core/__tests__/read-file-xml.test.ts
@@ -111,7 +111,7 @@ describe("read_file tool XML output structure", () => {
 			validateAccess: jest.fn().mockReturnValue(true),
 		}
 		mockCline.say = jest.fn().mockResolvedValue(undefined)
-		mockCline.ask = jest.fn().mockResolvedValue(true)
+		mockCline.ask = jest.fn().mockResolvedValue({ response: "yesButtonClicked" })
 		mockCline.presentAssistantMessage = jest.fn()
 		mockCline.sayAndCreateMissingParamError = jest.fn().mockResolvedValue("Missing required parameter")
 		// Add mock for getFileContextTracker method
@@ -120,6 +120,7 @@ describe("read_file tool XML output structure", () => {
 		})
 		mockCline.recordToolUsage = jest.fn().mockReturnValue(undefined)
 		mockCline.recordToolError = jest.fn().mockReturnValue(undefined)
+		mockCline.didRejectTool = false
 
 		// Reset tool result
 		toolResult = undefined
@@ -611,8 +612,11 @@ describe("read_file tool XML output structure", () => {
 				return Promise.reject(new Error("File not found"))
 			})
 
-			// Mock approval to always succeed since RooIgnore handles access
-			mockCline.ask = jest.fn().mockResolvedValue(true)
+			// Mock approval for both files
+			mockCline.ask = jest
+				.fn()
+				.mockResolvedValueOnce({ response: "yesButtonClicked" }) // First file approved
+				.mockResolvedValueOnce({ response: "noButtonClicked" }) // Second file denied
 
 			// Execute - Skip the default validateAccess mock
 			const { readFileTool } = require("../tools/readFileTool")
@@ -654,7 +658,7 @@ describe("read_file tool XML output structure", () => {
 
 			// Verify result
 			expect(result).toBe(
-				`<files>\n<file><path>${invalidPath}</path><error>${formatResponse.rooIgnoreError(invalidPath)}</error></file>\n<file><path>${validPath}</path>\n<content lines="1-1">\n${numberedContent}</content>\n</file>\n</files>`,
+				`<files>\n<file><path>${validPath}</path>\n<content lines="1-1">\n${numberedContent}</content>\n</file>\n<file><path>${invalidPath}</path><error>${formatResponse.rooIgnoreError(invalidPath)}</error></file>\n</files>`,
 			)
 		})
 
