fix(e2e): address readonly tool review feedback

Author: roomote

apps/vscode-e2e/src/fixtures/list-files.ts

@@ -24,11 +24,11 @@ export function addListFilesResultFixtures(mock: InstanceType<typeof LLMock>) {
 			id: "call_list_files_non_recursive_002",
 		},
 		{
-			userMessagePattern: "LIST_FILES_RECURSIVE_SMOKE",
+			userMessagePattern: "deep-nested-file.ts is included",
 			toolName: "list_files",
 			arguments: '{"path":"list-files-tool-fixture","recursive":true}',
 			toolCallId: "call_list_files_recursive_001",
-			expected: ["nested/", "nested/deep/"],
+			expected: ["nested/", "nested/deep/", "deep-nested-file.ts"],
 			result: "The recursive listing for `list-files-tool-fixture` reached the nested structure and includes `nested/`, `deep/`, and `deep-nested-file.ts`.",
 			id: "call_list_files_recursive_002",
 		},
@@ -42,7 +42,7 @@ export function addListFilesResultFixtures(mock: InstanceType<typeof LLMock>) {
 			id: "call_list_files_symlink_002",
 		},
 		{
-			userMessagePattern: "LIST_FILES_WORKSPACE_ROOT_SMOKE",
+			userMessagePattern: "confirm whether list-files-tool-fixture or list-files-symlink-fixture is present",
 			toolName: "list_files",
 			arguments: '{"path":".","recursive":false}',
 			toolCallId: "call_list_files_workspace_root_001",

apps/vscode-e2e/src/fixtures/search-files.ts

@@ -15,7 +15,7 @@ type SearchFilesFixture = {
 export function addSearchFilesResultFixtures(mock: InstanceType<typeof LLMock>) {
 	const fixtures: SearchFilesFixture[] = [
 		{
-			userMessagePattern: "SEARCH_FILES_FUNCTIONS_SMOKE",
+			userMessagePattern: "JavaScript function declarations",
 			toolName: "search_files",
 			arguments: '{"path":"search-files-tool-fixture","regex":"function\\\\s+\\\\w+"}',
 			toolCallId: "call_search_files_functions_001",
@@ -28,7 +28,7 @@ export function addSearchFilesResultFixtures(mock: InstanceType<typeof LLMock>)
 			id: "call_search_files_functions_002",
 		},
 		{
-			userMessagePattern: "SEARCH_FILES_TODO_SMOKE",
+			userMessagePattern: "matching TODO entries",
 			toolName: "search_files",
 			arguments: '{"path":"search-files-tool-fixture","regex":"TODO.*"}',
 			toolCallId: "call_search_files_todo_001",
@@ -85,7 +85,7 @@ export function addSearchFilesResultFixtures(mock: InstanceType<typeof LLMock>)
 			id: "call_search_files_complex_regex_002",
 		},
 		{
-			userMessagePattern: "SEARCH_FILES_NO_MATCH_SMOKE",
+			userMessagePattern: "nonExistentPattern12345 and report that there are no matches",
 			toolName: "search_files",
 			arguments: '{"path":"search-files-tool-fixture","regex":"nonExistentPattern12345"}',
 			toolCallId: "call_search_files_no_match_001",
@@ -94,7 +94,7 @@ export function addSearchFilesResultFixtures(mock: InstanceType<typeof LLMock>)
 			id: "call_search_files_no_match_002",
 		},
 		{
-			userMessagePattern: "SEARCH_FILES_CLASS_METHOD_SMOKE",
+			userMessagePattern: "TypeScript class definitions and async methods",
 			toolName: "search_files",
 			arguments:
 				'{"path":"search-files-tool-fixture","regex":"(class\\\\s+\\\\w+|async\\\\s+\\\\w+)","file_pattern":"*.ts"}',

apps/vscode-e2e/src/suite/tools/list-files.test.ts

@@ -262,7 +262,7 @@ This directory contains various files and subdirectories for testing the list_fi
 					alwaysAllowReadOnly: true,
 					alwaysAllowReadOnlyOutsideWorkspace: true,
 				},
-				text: "LIST_FILES_RECURSIVE_SMOKE",
+				text: "List every file in the list-files-tool-fixture directory recursively and confirm that the nested path for deep-nested-file.ts is included.",
 			})
 
 			console.log("Task ID:", taskId)
@@ -275,7 +275,8 @@ This directory contains various files and subdirectories for testing the list_fi
 					m.type === "say" &&
 					(m.say === "completion_result" || m.say === "text") &&
 					m.text?.includes("nested/") &&
-					m.text?.includes("deep/"),
+					m.text?.includes("deep/") &&
+					m.text?.includes("deep-nested-file.ts"),
 			)
 			assert.ok(completionMessage, "AI should have summarized the recursive directory contents")
 
@@ -403,7 +404,7 @@ This directory contains various files and subdirectories for testing the list_fi
 					alwaysAllowReadOnly: true,
 					alwaysAllowReadOnlyOutsideWorkspace: true,
 				},
-				text: "LIST_FILES_WORKSPACE_ROOT_SMOKE",
+				text: "List the files in the workspace root directory without recursing and confirm whether list-files-tool-fixture or list-files-symlink-fixture is present.",
 			})
 
 			console.log("Task ID:", taskId)
@@ -416,7 +417,7 @@ This directory contains various files and subdirectories for testing the list_fi
 				(m) =>
 					m.type === "say" &&
 					(m.say === "completion_result" || m.say === "text") &&
-					(m.text?.includes("apps") || m.text?.includes("packages") || m.text?.includes("workspace")),
+					(m.text?.includes("list-files-tool-fixture") || m.text?.includes("list-files-symlink-fixture")),
 			)
 			assert.ok(completionMessage, "AI should have mentioned workspace contents")
 

apps/vscode-e2e/src/suite/tools/search-files.test.ts

@@ -323,7 +323,7 @@ The search should find matches across different file types and provide context f
 					alwaysAllowReadOnly: true,
 					alwaysAllowReadOnlyOutsideWorkspace: true,
 				},
-				text: "SEARCH_FILES_FUNCTIONS_SMOKE",
+				text: "Search the search-files-tool-fixture directory for JavaScript function declarations using the regex function\\s+\\w+ and report the function names you find.",
 			})
 
 			console.log("Task ID:", taskId)
@@ -379,7 +379,7 @@ The search should find matches across different file types and provide context f
 					alwaysAllowReadOnly: true,
 					alwaysAllowReadOnlyOutsideWorkspace: true,
 				},
-				text: "SEARCH_FILES_TODO_SMOKE",
+				text: "Search the search-files-tool-fixture directory for TODO comments using the regex TODO.* and report the matching TODO entries.",
 			})
 
 			// Wait for task completion
@@ -649,7 +649,7 @@ The search should find matches across different file types and provide context f
 					alwaysAllowReadOnly: true,
 					alwaysAllowReadOnlyOutsideWorkspace: true,
 				},
-				text: "SEARCH_FILES_NO_MATCH_SMOKE",
+				text: "Search the search-files-tool-fixture directory for nonExistentPattern12345 and report that there are no matches if the regex finds nothing.",
 			})
 
 			// Wait for task completion
@@ -724,7 +724,7 @@ The search should find matches across different file types and provide context f
 					alwaysAllowReadOnly: true,
 					alwaysAllowReadOnlyOutsideWorkspace: true,
 				},
-				text: "SEARCH_FILES_CLASS_METHOD_SMOKE",
+				text: "Search the search-files-tool-fixture directory for TypeScript class definitions and async methods using the regex (class\\s+\\w+|async\\s+\\w+) with file_pattern *.ts, then report what you find.",
 			})
 
 			// Wait for task completion

src/services/glob/__tests__/list-files.spec.ts

@@ -112,18 +112,15 @@ describe("list-files symlink support", () => {
 		await listFiles(testDir, false, 100)
 
 		// Verify that spawn was called with --follow flag (the critical fix)
-		const [rgPath, args] = mockSpawn.mock.calls[0]
+		const [rgPath, args, options] = mockSpawn.mock.calls[0]
 		expect(rgPath).toBe("/mock/path/to/rg")
 		expect(args).toContain("--files")
 		expect(args).toContain("--hidden")
 		expect(args).toContain("--follow") // This is the critical assertion - the fix should add this flag
 
-		// Platform-agnostic path check - verify the last argument ends with the expected path
 		const lastArg = args[args.length - 1]
-		// On Windows, the path might be resolved to something like D:\test\dir
-		// On Unix, it would be /test/dir
-		// So we just check that it ends with the expected segments
-		expect(lastArg).toMatch(/[/\\]test[/\\]dir$/)
+		expect(lastArg).toBe(".")
+		expect(options).toMatchObject({ cwd: testDir })
 	})
 
 	it("should include --follow flag for recursive listings too", async () => {
@@ -137,18 +134,30 @@ describe("list-files symlink support", () => {
 		await listFiles(testDir, true, 100)
 
 		// Verify that spawn was called with --follow flag (the critical fix)
-		const [rgPath, args] = mockSpawn.mock.calls[0]
+		const [rgPath, args, options] = mockSpawn.mock.calls[0]
 		expect(rgPath).toBe("/mock/path/to/rg")
 		expect(args).toContain("--files")
 		expect(args).toContain("--hidden")
 		expect(args).toContain("--follow") // This should be present in recursive mode too
 
-		// Platform-agnostic path check - verify the last argument ends with the expected path
 		const lastArg = args[args.length - 1]
-		// On Windows, the path might be resolved to something like D:\test\dir
-		// On Unix, it would be /test/dir
-		// So we just check that it ends with the expected segments
-		expect(lastArg).toMatch(/[/\\]test[/\\]dir$/)
+		expect(lastArg).toBe(".")
+		expect(options).toMatchObject({ cwd: testDir })
+	})
+
+	it("should keep ignored ancestor directories like /tmp from excluding recursive file results", async () => {
+		const mockSpawn = vi.mocked(childProcess.spawn)
+		mockSpawn.mockReturnValue(createMockRipgrepProcess(["nested/deep/deep-nested-file.ts\n"]) as any)
+
+		const testDir = "/tmp/roo-test-workspace/list-files-tool-fixture"
+
+		const [files] = await listFiles(testDir, true, 100)
+
+		const [, args, options] = mockSpawn.mock.calls[0]
+		expect(args).toContain("!**/tmp/**")
+		expect(args[args.length - 1]).toBe(".")
+		expect(options).toMatchObject({ cwd: testDir })
+		expect(files).toContain(path.join(testDir, "nested", "deep", "deep-nested-file.ts"))
 	})
 
 	it("should ensure first-level directories are included when limit is reached", async () => {

src/services/glob/list-files.ts

@@ -203,27 +203,28 @@ async function listFilesWithRipgrep(
 	recursive: boolean,
 	limit: number,
 ): Promise<string[]> {
-	const rgArgs = buildRipgrepArgs(dirPath, recursive)
+	const absolutePath = path.resolve(dirPath)
+	const rgArgs = buildRipgrepArgs(".", recursive, dirPath)
 
-	const relativePaths = await execRipgrep(rgPath, rgArgs, limit)
+	const relativePaths = await execRipgrep(rgPath, rgArgs, limit, absolutePath)
 
-	// Convert relative paths from ripgrep to absolute paths
-	// Resolve dirPath once here for the mapping operation
-	const absolutePath = path.resolve(dirPath)
+	// Convert relative paths from ripgrep to absolute paths.
+	// Ripgrep now runs from the target directory so glob exclusions apply within that root
+	// instead of accidentally matching ignored ancestor path segments like /tmp.
 	return relativePaths.map((relativePath) => path.resolve(absolutePath, relativePath))
 }
 
 /**
  * Build appropriate ripgrep arguments based on whether we're doing a recursive search
  */
-function buildRipgrepArgs(dirPath: string, recursive: boolean): string[] {
+function buildRipgrepArgs(searchPath: string, recursive: boolean, targetDirPath: string): string[] {
 	// Base arguments to list files
 	const args = ["--files", "--hidden", "--follow"]
 
 	if (recursive) {
-		return [...args, ...buildRecursiveArgs(dirPath), dirPath]
+		return [...args, ...buildRecursiveArgs(targetDirPath), searchPath]
 	} else {
-		return [...args, ...buildNonRecursiveArgs(), dirPath]
+		return [...args, ...buildNonRecursiveArgs(), searchPath]
 	}
 }
 
@@ -646,12 +647,9 @@ function formatAndCombineResults(files: string[], directories: string[], limit:
 /**
  * Execute ripgrep command and return list of files
  */
-async function execRipgrep(rgPath: string, args: string[], limit: number): Promise<string[]> {
+async function execRipgrep(rgPath: string, args: string[], limit: number, cwd?: string): Promise<string[]> {
 	return new Promise((resolve, reject) => {
-		// Extract the directory path from args (it's the last argument)
-		const searchDir = args[args.length - 1]
-
-		const rgProcess = childProcess.spawn(rgPath, args)
+		const rgProcess = childProcess.spawn(rgPath, args, cwd ? { cwd } : undefined)
 		let output = ""
 		let results: string[] = []