From 0e3a0d7dc5c2a62fb3dd6124e30be407692f98f9 Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Wed, 10 Sep 2025 09:54:50 -0700 Subject: [PATCH] Add .geminiignore support to the glob tool. (#8086) --- .../core/src/services/fileDiscoveryService.ts | 42 ++ packages/core/src/tools/glob.test.ts | 87 ++++ packages/core/src/tools/glob.ts | 70 ++- packages/core/src/tools/ls.test.ts | 484 ++++++------------ packages/core/src/tools/ls.ts | 77 +-- packages/core/src/tools/read-many-files.ts | 98 +--- 6 files changed, 388 insertions(+), 470 deletions(-) diff --git a/packages/core/src/services/fileDiscoveryService.ts b/packages/core/src/services/fileDiscoveryService.ts index 0a309f8faa..1689d7ecce 100644 --- a/packages/core/src/services/fileDiscoveryService.ts +++ b/packages/core/src/services/fileDiscoveryService.ts @@ -16,6 +16,12 @@ export interface FilterFilesOptions { respectGeminiIgnore?: boolean; } +export interface FilterReport { + filteredPaths: string[]; + gitIgnoredCount: number; + geminiIgnoredCount: number; +} + export class FileDiscoveryService { private gitIgnoreFilter: GitIgnoreFilter | null = null; private geminiIgnoreFilter: GitIgnoreFilter | null = null; @@ -65,6 +71,42 @@ export class FileDiscoveryService { }); } + /** + * Filters a list of file paths based on git ignore rules and returns a report + * with counts of ignored files. + */ + filterFilesWithReport( + filePaths: string[], + opts: FilterFilesOptions = { + respectGitIgnore: true, + respectGeminiIgnore: true, + }, + ): FilterReport { + const filteredPaths: string[] = []; + let gitIgnoredCount = 0; + let geminiIgnoredCount = 0; + + for (const filePath of filePaths) { + if (opts.respectGitIgnore && this.shouldGitIgnoreFile(filePath)) { + gitIgnoredCount++; + continue; + } + + if (opts.respectGeminiIgnore && this.shouldGeminiIgnoreFile(filePath)) { + geminiIgnoredCount++; + continue; + } + + filteredPaths.push(filePath); + } + + return { + filteredPaths, + gitIgnoredCount, + geminiIgnoredCount, + }; + } + /** * Checks if a single file should be git-ignored */ diff --git a/packages/core/src/tools/glob.test.ts b/packages/core/src/tools/glob.test.ts index 3a911a5711..b965ce9036 100644 --- a/packages/core/src/tools/glob.test.ts +++ b/packages/core/src/tools/glob.test.ts @@ -28,6 +28,10 @@ describe('GlobTool', () => { const mockConfig = { getFileService: () => new FileDiscoveryService(tempRootDir), getFileFilteringRespectGitIgnore: () => true, + getFileFilteringOptions: () => ({ + respectGitIgnore: true, + respectGeminiIgnore: true, + }), getTargetDir: () => tempRootDir, getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), getFileExclusions: () => ({ @@ -38,6 +42,7 @@ describe('GlobTool', () => { beforeEach(async () => { // Create a unique root directory for each test run tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'glob-tool-root-')); + await fs.writeFile(path.join(tempRootDir, '.git'), ''); // Fake git repo globTool = new GlobTool(mockConfig); // Create some test files and directories within this root @@ -366,6 +371,88 @@ describe('GlobTool', () => { expect(result.llmContent).toContain('FileD.MD'); }); }); + + describe('ignore file handling', () => { + it('should respect .gitignore files by default', async () => { + await fs.writeFile(path.join(tempRootDir, '.gitignore'), '*.ignored.txt'); + await fs.writeFile( + path.join(tempRootDir, 'a.ignored.txt'), + 'ignored content', + ); + await fs.writeFile( + path.join(tempRootDir, 'b.notignored.txt'), + 'not ignored content', + ); + + const params: GlobToolParams = { pattern: '*.txt' }; + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('Found 3 file(s)'); // fileA.txt, FileB.TXT, b.notignored.txt + expect(result.llmContent).not.toContain('a.ignored.txt'); + }); + + it('should respect .geminiignore files by default', async () => { + await fs.writeFile( + path.join(tempRootDir, '.geminiignore'), + '*.geminiignored.txt', + ); + await fs.writeFile( + path.join(tempRootDir, 'a.geminiignored.txt'), + 'ignored content', + ); + await fs.writeFile( + path.join(tempRootDir, 'b.notignored.txt'), + 'not ignored content', + ); + + const params: GlobToolParams = { pattern: '*.txt' }; + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('Found 3 file(s)'); // fileA.txt, FileB.TXT, b.notignored.txt + expect(result.llmContent).not.toContain('a.geminiignored.txt'); + }); + + it('should not respect .gitignore when respect_git_ignore is false', async () => { + await fs.writeFile(path.join(tempRootDir, '.gitignore'), '*.ignored.txt'); + await fs.writeFile( + path.join(tempRootDir, 'a.ignored.txt'), + 'ignored content', + ); + + const params: GlobToolParams = { + pattern: '*.txt', + respect_git_ignore: false, + }; + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('Found 3 file(s)'); // fileA.txt, FileB.TXT, a.ignored.txt + expect(result.llmContent).toContain('a.ignored.txt'); + }); + + it('should not respect .geminiignore when respect_gemini_ignore is false', async () => { + await fs.writeFile( + path.join(tempRootDir, '.geminiignore'), + '*.geminiignored.txt', + ); + await fs.writeFile( + path.join(tempRootDir, 'a.geminiignored.txt'), + 'ignored content', + ); + + const params: GlobToolParams = { + pattern: '*.txt', + respect_gemini_ignore: false, + }; + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('Found 3 file(s)'); // fileA.txt, FileB.TXT, a.geminiignored.txt + expect(result.llmContent).toContain('a.geminiignored.txt'); + }); + }); }); describe('sortFileEntries', () => { diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index 7efae2baf1..895b6c9e8a 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -10,7 +10,10 @@ import { glob, escape } from 'glob'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; import { shortenPath, makeRelative } from '../utils/paths.js'; -import type { Config } from '../config/config.js'; +import { + type Config, + DEFAULT_FILE_FILTERING_OPTIONS, +} from '../config/config.js'; import { ToolErrorType } from './tool-error.js'; // Subset of 'Path' interface provided by 'glob' that we can implement for testing @@ -72,6 +75,11 @@ export interface GlobToolParams { * Whether to respect .gitignore patterns (optional, defaults to true) */ respect_git_ignore?: boolean; + + /** + * Whether to respect .geminiignore patterns (optional, defaults to true) + */ + respect_gemini_ignore?: boolean; } class GlobToolInvocation extends BaseToolInvocation< @@ -128,14 +136,10 @@ class GlobToolInvocation extends BaseToolInvocation< } // Get centralized file discovery service - const respectGitIgnore = - this.params.respect_git_ignore ?? - this.config.getFileFilteringRespectGitIgnore(); const fileDiscovery = this.config.getFileService(); // Collect entries from all search directories - let allEntries: GlobPath[] = []; - + const allEntries: GlobPath[] = []; for (const searchDir of searchDirectories) { let pattern = this.params.pattern; const fullPath = path.join(searchDir, pattern); @@ -155,33 +159,32 @@ class GlobToolInvocation extends BaseToolInvocation< signal, })) as GlobPath[]; - allEntries = allEntries.concat(entries); + allEntries.push(...entries); } - const entries = allEntries; + const relativePaths = allEntries.map((p) => + path.relative(this.config.getTargetDir(), p.fullpath()), + ); - // Apply git-aware filtering if enabled and in git repository - let filteredEntries = entries; - let gitIgnoredCount = 0; - - if (respectGitIgnore) { - const relativePaths = entries.map((p) => - path.relative(this.config.getTargetDir(), p.fullpath()), - ); - const filteredRelativePaths = fileDiscovery.filterFiles(relativePaths, { - respectGitIgnore, + const { filteredPaths, gitIgnoredCount, geminiIgnoredCount } = + fileDiscovery.filterFilesWithReport(relativePaths, { + respectGitIgnore: + this.params?.respect_git_ignore ?? + this.config.getFileFilteringOptions().respectGitIgnore ?? + DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore, + respectGeminiIgnore: + this.params?.respect_gemini_ignore ?? + this.config.getFileFilteringOptions().respectGeminiIgnore ?? + DEFAULT_FILE_FILTERING_OPTIONS.respectGeminiIgnore, }); - const filteredAbsolutePaths = new Set( - filteredRelativePaths.map((p) => - path.resolve(this.config.getTargetDir(), p), - ), - ); - filteredEntries = entries.filter((entry) => - filteredAbsolutePaths.has(entry.fullpath()), - ); - gitIgnoredCount = entries.length - filteredEntries.length; - } + const filteredAbsolutePaths = new Set( + filteredPaths.map((p) => path.resolve(this.config.getTargetDir(), p)), + ); + + const filteredEntries = allEntries.filter((entry) => + filteredAbsolutePaths.has(entry.fullpath()), + ); if (!filteredEntries || filteredEntries.length === 0) { let message = `No files found matching pattern "${this.params.pattern}"`; @@ -193,6 +196,9 @@ class GlobToolInvocation extends BaseToolInvocation< if (gitIgnoredCount > 0) { message += ` (${gitIgnoredCount} files were git-ignored)`; } + if (geminiIgnoredCount > 0) { + message += ` (${geminiIgnoredCount} files were gemini-ignored)`; + } return { llmContent: message, returnDisplay: `No files found`, @@ -225,6 +231,9 @@ class GlobToolInvocation extends BaseToolInvocation< if (gitIgnoredCount > 0) { resultMessage += ` (${gitIgnoredCount} additional files were git-ignored)`; } + if (geminiIgnoredCount > 0) { + resultMessage += ` (${geminiIgnoredCount} additional files were gemini-ignored)`; + } resultMessage += `, sorted by modification time (newest first):\n${fileListDescription}`; return { @@ -282,6 +291,11 @@ export class GlobTool extends BaseDeclarativeTool { 'Optional: Whether to respect .gitignore patterns when finding files. Only available in git repositories. Defaults to true.', type: 'boolean', }, + respect_gemini_ignore: { + description: + 'Optional: Whether to respect .geminiignore patterns when finding files. Defaults to true.', + type: 'boolean', + }, }, required: ['pattern'], type: 'object', diff --git a/packages/core/src/tools/ls.test.ts b/packages/core/src/tools/ls.test.ts index 9b2797b548..f48f8cde23 100644 --- a/packages/core/src/tools/ls.test.ts +++ b/packages/core/src/tools/ls.test.ts @@ -4,65 +4,38 @@ * SPDX-License-Identifier: Apache-2.0 */ -/* eslint-disable @typescript-eslint/no-explicit-any */ - -import { describe, it, expect, beforeEach, vi } from 'vitest'; -import fs from 'node:fs'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import fs from 'node:fs/promises'; import path from 'node:path'; - -vi.mock('fs', () => ({ - default: { - statSync: vi.fn(), - readdirSync: vi.fn(), - }, - statSync: vi.fn(), - readdirSync: vi.fn(), - mkdirSync: vi.fn(), -})); +import os from 'node:os'; import { LSTool } from './ls.js'; import type { Config } from '../config/config.js'; -import type { WorkspaceContext } from '../utils/workspaceContext.js'; -import type { FileDiscoveryService } from '../services/fileDiscoveryService.js'; +import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; import { ToolErrorType } from './tool-error.js'; +import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; describe('LSTool', () => { let lsTool: LSTool; + let tempRootDir: string; + let tempSecondaryDir: string; let mockConfig: Config; - let mockWorkspaceContext: WorkspaceContext; - let mockFileService: FileDiscoveryService; - const mockPrimaryDir = '/home/user/project'; - const mockSecondaryDir = '/home/user/other-project'; + const abortSignal = new AbortController().signal; - beforeEach(() => { - vi.resetAllMocks(); + beforeEach(async () => { + tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'ls-tool-root-')); + tempSecondaryDir = await fs.mkdtemp( + path.join(os.tmpdir(), 'ls-tool-secondary-'), + ); - // Mock WorkspaceContext - mockWorkspaceContext = { - getDirectories: vi - .fn() - .mockReturnValue([mockPrimaryDir, mockSecondaryDir]), - isPathWithinWorkspace: vi - .fn() - .mockImplementation( - (path) => - path.startsWith(mockPrimaryDir) || - path.startsWith(mockSecondaryDir), - ), - addDirectory: vi.fn(), - } as unknown as WorkspaceContext; + const mockWorkspaceContext = createMockWorkspaceContext(tempRootDir, [ + tempSecondaryDir, + ]); - // Mock FileService - mockFileService = { - shouldGitIgnoreFile: vi.fn().mockReturnValue(false), - shouldGeminiIgnoreFile: vi.fn().mockReturnValue(false), - } as unknown as FileDiscoveryService; - - // Mock Config mockConfig = { - getTargetDir: vi.fn().mockReturnValue(mockPrimaryDir), - getWorkspaceContext: vi.fn().mockReturnValue(mockWorkspaceContext), - getFileService: vi.fn().mockReturnValue(mockFileService), - getFileFilteringOptions: vi.fn().mockReturnValue({ + getTargetDir: () => tempRootDir, + getWorkspaceContext: () => mockWorkspaceContext, + getFileService: () => new FileDiscoveryService(tempRootDir), + getFileFilteringOptions: () => ({ respectGitIgnore: true, respectGeminiIgnore: true, }), @@ -71,221 +44,132 @@ describe('LSTool', () => { lsTool = new LSTool(mockConfig); }); + afterEach(async () => { + await fs.rm(tempRootDir, { recursive: true, force: true }); + await fs.rm(tempSecondaryDir, { recursive: true, force: true }); + }); + describe('parameter validation', () => { - it('should accept valid absolute paths within workspace', () => { - const params = { - path: '/home/user/project/src', - }; - vi.mocked(fs.statSync).mockReturnValue({ - isDirectory: () => true, - } as fs.Stats); - const invocation = lsTool.build(params); + it('should accept valid absolute paths within workspace', async () => { + const testPath = path.join(tempRootDir, 'src'); + await fs.mkdir(testPath); + + const invocation = lsTool.build({ path: testPath }); + expect(invocation).toBeDefined(); }); it('should reject relative paths', () => { - const params = { - path: './src', - }; - - expect(() => lsTool.build(params)).toThrow( + expect(() => lsTool.build({ path: './src' })).toThrow( 'Path must be absolute: ./src', ); }); it('should reject paths outside workspace with clear error message', () => { - const params = { - path: '/etc/passwd', - }; - - expect(() => lsTool.build(params)).toThrow( - 'Path must be within one of the workspace directories: /home/user/project, /home/user/other-project', + expect(() => lsTool.build({ path: '/etc/passwd' })).toThrow( + `Path must be within one of the workspace directories: ${tempRootDir}, ${tempSecondaryDir}`, ); }); - it('should accept paths in secondary workspace directory', () => { - const params = { - path: '/home/user/other-project/lib', - }; - vi.mocked(fs.statSync).mockReturnValue({ - isDirectory: () => true, - } as fs.Stats); - const invocation = lsTool.build(params); + it('should accept paths in secondary workspace directory', async () => { + const testPath = path.join(tempSecondaryDir, 'lib'); + await fs.mkdir(testPath); + + const invocation = lsTool.build({ path: testPath }); + expect(invocation).toBeDefined(); }); }); describe('execute', () => { it('should list files in a directory', async () => { - const testPath = '/home/user/project/src'; - const mockFiles = ['file1.ts', 'file2.ts', 'subdir']; - const mockStats = { - isDirectory: vi.fn(), - mtime: new Date(), - size: 1024, - }; + await fs.writeFile(path.join(tempRootDir, 'file1.txt'), 'content1'); + await fs.mkdir(path.join(tempRootDir, 'subdir')); + await fs.writeFile( + path.join(tempSecondaryDir, 'secondary-file.txt'), + 'secondary', + ); - vi.mocked(fs.statSync).mockImplementation((path: any) => { - const pathStr = path.toString(); - if (pathStr === testPath) { - return { isDirectory: () => true } as fs.Stats; - } - // For individual files - if (pathStr.toString().endsWith('subdir')) { - return { ...mockStats, isDirectory: () => true, size: 0 } as fs.Stats; - } - return { ...mockStats, isDirectory: () => false } as fs.Stats; - }); - - vi.mocked(fs.readdirSync).mockReturnValue(mockFiles as any); - - const invocation = lsTool.build({ path: testPath }); - const result = await invocation.execute(new AbortController().signal); + const invocation = lsTool.build({ path: tempRootDir }); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('[DIR] subdir'); - expect(result.llmContent).toContain('file1.ts'); - expect(result.llmContent).toContain('file2.ts'); - expect(result.returnDisplay).toBe('Listed 3 item(s).'); - }); - - it('should list files from secondary workspace directory', async () => { - const testPath = '/home/user/other-project/lib'; - const mockFiles = ['module1.js', 'module2.js']; - - vi.mocked(fs.statSync).mockImplementation((path: any) => { - if (path.toString() === testPath) { - return { isDirectory: () => true } as fs.Stats; - } - return { - isDirectory: () => false, - mtime: new Date(), - size: 2048, - } as fs.Stats; - }); - - vi.mocked(fs.readdirSync).mockReturnValue(mockFiles as any); - - const invocation = lsTool.build({ path: testPath }); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).toContain('module1.js'); - expect(result.llmContent).toContain('module2.js'); + expect(result.llmContent).toContain('file1.txt'); expect(result.returnDisplay).toBe('Listed 2 item(s).'); }); - it('should handle empty directories', async () => { - const testPath = '/home/user/project/empty'; - - vi.mocked(fs.statSync).mockReturnValue({ - isDirectory: () => true, - } as fs.Stats); - vi.mocked(fs.readdirSync).mockReturnValue([]); - - const invocation = lsTool.build({ path: testPath }); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).toBe( - 'Directory /home/user/project/empty is empty.', + it('should list files from secondary workspace directory', async () => { + await fs.writeFile(path.join(tempRootDir, 'file1.txt'), 'content1'); + await fs.mkdir(path.join(tempRootDir, 'subdir')); + await fs.writeFile( + path.join(tempSecondaryDir, 'secondary-file.txt'), + 'secondary', ); + + const invocation = lsTool.build({ path: tempSecondaryDir }); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('secondary-file.txt'); + expect(result.returnDisplay).toBe('Listed 1 item(s).'); + }); + + it('should handle empty directories', async () => { + const emptyDir = path.join(tempRootDir, 'empty'); + await fs.mkdir(emptyDir); + const invocation = lsTool.build({ path: emptyDir }); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toBe(`Directory ${emptyDir} is empty.`); expect(result.returnDisplay).toBe('Directory is empty.'); }); it('should respect ignore patterns', async () => { - const testPath = '/home/user/project/src'; - const mockFiles = ['test.js', 'test.spec.js', 'index.js']; - - vi.mocked(fs.statSync).mockImplementation((path: any) => { - const pathStr = path.toString(); - if (pathStr === testPath) { - return { isDirectory: () => true } as fs.Stats; - } - return { - isDirectory: () => false, - mtime: new Date(), - size: 1024, - } as fs.Stats; - }); - vi.mocked(fs.readdirSync).mockReturnValue(mockFiles as any); + await fs.writeFile(path.join(tempRootDir, 'file1.txt'), 'content1'); + await fs.writeFile(path.join(tempRootDir, 'file2.log'), 'content1'); const invocation = lsTool.build({ - path: testPath, - ignore: ['*.spec.js'], + path: tempRootDir, + ignore: ['*.log'], }); - const result = await invocation.execute(new AbortController().signal); + const result = await invocation.execute(abortSignal); - expect(result.llmContent).toContain('test.js'); - expect(result.llmContent).toContain('index.js'); - expect(result.llmContent).not.toContain('test.spec.js'); - expect(result.returnDisplay).toBe('Listed 2 item(s).'); + expect(result.llmContent).toContain('file1.txt'); + expect(result.llmContent).not.toContain('file2.log'); + expect(result.returnDisplay).toBe('Listed 1 item(s).'); }); it('should respect gitignore patterns', async () => { - const testPath = '/home/user/project/src'; - const mockFiles = ['file1.js', 'file2.js', 'ignored.js']; + await fs.writeFile(path.join(tempRootDir, 'file1.txt'), 'content1'); + await fs.writeFile(path.join(tempRootDir, 'file2.log'), 'content1'); + await fs.writeFile(path.join(tempRootDir, '.git'), ''); + await fs.writeFile(path.join(tempRootDir, '.gitignore'), '*.log'); + const invocation = lsTool.build({ path: tempRootDir }); + const result = await invocation.execute(abortSignal); - vi.mocked(fs.statSync).mockImplementation((path: any) => { - const pathStr = path.toString(); - if (pathStr === testPath) { - return { isDirectory: () => true } as fs.Stats; - } - return { - isDirectory: () => false, - mtime: new Date(), - size: 1024, - } as fs.Stats; - }); - vi.mocked(fs.readdirSync).mockReturnValue(mockFiles as any); - (mockFileService.shouldGitIgnoreFile as any).mockImplementation( - (path: string) => path.includes('ignored.js'), - ); - - const invocation = lsTool.build({ path: testPath }); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).toContain('file1.js'); - expect(result.llmContent).toContain('file2.js'); - expect(result.llmContent).not.toContain('ignored.js'); - expect(result.returnDisplay).toBe('Listed 2 item(s). (1 git-ignored)'); + expect(result.llmContent).toContain('file1.txt'); + expect(result.llmContent).not.toContain('file2.log'); + // .git is always ignored by default. + expect(result.returnDisplay).toBe('Listed 2 item(s). (2 git-ignored)'); }); it('should respect geminiignore patterns', async () => { - const testPath = '/home/user/project/src'; - const mockFiles = ['file1.js', 'file2.js', 'private.js']; + await fs.writeFile(path.join(tempRootDir, 'file1.txt'), 'content1'); + await fs.writeFile(path.join(tempRootDir, 'file2.log'), 'content1'); + await fs.writeFile(path.join(tempRootDir, '.geminiignore'), '*.log'); + const invocation = lsTool.build({ path: tempRootDir }); + const result = await invocation.execute(abortSignal); - vi.mocked(fs.statSync).mockImplementation((path: any) => { - const pathStr = path.toString(); - if (pathStr === testPath) { - return { isDirectory: () => true } as fs.Stats; - } - return { - isDirectory: () => false, - mtime: new Date(), - size: 1024, - } as fs.Stats; - }); - vi.mocked(fs.readdirSync).mockReturnValue(mockFiles as any); - (mockFileService.shouldGeminiIgnoreFile as any).mockImplementation( - (path: string) => path.includes('private.js'), - ); - - const invocation = lsTool.build({ path: testPath }); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).toContain('file1.js'); - expect(result.llmContent).toContain('file2.js'); - expect(result.llmContent).not.toContain('private.js'); + expect(result.llmContent).toContain('file1.txt'); + expect(result.llmContent).not.toContain('file2.log'); expect(result.returnDisplay).toBe('Listed 2 item(s). (1 gemini-ignored)'); }); it('should handle non-directory paths', async () => { - const testPath = '/home/user/project/file.txt'; - - vi.mocked(fs.statSync).mockReturnValue({ - isDirectory: () => false, - } as fs.Stats); + const testPath = path.join(tempRootDir, 'file1.txt'); + await fs.writeFile(testPath, 'content1'); const invocation = lsTool.build({ path: testPath }); - const result = await invocation.execute(new AbortController().signal); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Path is not a directory'); expect(result.returnDisplay).toBe('Error: Path is not a directory.'); @@ -293,14 +177,9 @@ describe('LSTool', () => { }); it('should handle non-existent paths', async () => { - const testPath = '/home/user/project/does-not-exist'; - - vi.mocked(fs.statSync).mockImplementation(() => { - throw new Error('ENOENT: no such file or directory'); - }); - + const testPath = path.join(tempRootDir, 'does-not-exist'); const invocation = lsTool.build({ path: testPath }); - const result = await invocation.execute(new AbortController().signal); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Error listing directory'); expect(result.returnDisplay).toBe('Error: Failed to list directory.'); @@ -308,54 +187,38 @@ describe('LSTool', () => { }); it('should sort directories first, then files alphabetically', async () => { - const testPath = '/home/user/project/src'; - const mockFiles = ['z-file.ts', 'a-dir', 'b-file.ts', 'c-dir']; + await fs.writeFile(path.join(tempRootDir, 'a-file.txt'), 'content1'); + await fs.writeFile(path.join(tempRootDir, 'b-file.txt'), 'content1'); + await fs.mkdir(path.join(tempRootDir, 'x-dir')); + await fs.mkdir(path.join(tempRootDir, 'y-dir')); - vi.mocked(fs.statSync).mockImplementation((path: any) => { - if (path.toString() === testPath) { - return { isDirectory: () => true } as fs.Stats; - } - if (path.toString().endsWith('-dir')) { - return { - isDirectory: () => true, - mtime: new Date(), - size: 0, - } as fs.Stats; - } - return { - isDirectory: () => false, - mtime: new Date(), - size: 1024, - } as fs.Stats; - }); - - vi.mocked(fs.readdirSync).mockReturnValue(mockFiles as any); - - const invocation = lsTool.build({ path: testPath }); - const result = await invocation.execute(new AbortController().signal); + const invocation = lsTool.build({ path: tempRootDir }); + const result = await invocation.execute(abortSignal); const lines = ( typeof result.llmContent === 'string' ? result.llmContent : '' - ).split('\n'); - const entries = lines.slice(1).filter((line: string) => line.trim()); // Skip header - expect(entries[0]).toBe('[DIR] a-dir'); - expect(entries[1]).toBe('[DIR] c-dir'); - expect(entries[2]).toBe('b-file.ts'); - expect(entries[3]).toBe('z-file.ts'); + ) + .split('\n') + .filter(Boolean); + const entries = lines.slice(1); // Skip header + + expect(entries[0]).toBe('[DIR] x-dir'); + expect(entries[1]).toBe('[DIR] y-dir'); + expect(entries[2]).toBe('a-file.txt'); + expect(entries[3]).toBe('b-file.txt'); }); it('should handle permission errors gracefully', async () => { - const testPath = '/home/user/project/restricted'; + const restrictedDir = path.join(tempRootDir, 'restricted'); + await fs.mkdir(restrictedDir); - vi.mocked(fs.statSync).mockReturnValue({ - isDirectory: () => true, - } as fs.Stats); - vi.mocked(fs.readdirSync).mockImplementation(() => { - throw new Error('EACCES: permission denied'); - }); + // To simulate a permission error in a cross-platform way, + // we mock fs.readdir to throw an error. + const error = new Error('EACCES: permission denied'); + vi.spyOn(fs, 'readdir').mockRejectedValueOnce(error); - const invocation = lsTool.build({ path: testPath }); - const result = await invocation.execute(new AbortController().signal); + const invocation = lsTool.build({ path: restrictedDir }); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Error listing directory'); expect(result.llmContent).toContain('permission denied'); @@ -363,62 +226,57 @@ describe('LSTool', () => { expect(result.error?.type).toBe(ToolErrorType.LS_EXECUTION_ERROR); }); - it('should throw for invalid params at build time', async () => { + it('should throw for invalid params at build time', () => { expect(() => lsTool.build({ path: '../outside' })).toThrow( 'Path must be absolute: ../outside', ); }); it('should handle errors accessing individual files during listing', async () => { - const testPath = '/home/user/project/src'; - const mockFiles = ['accessible.ts', 'inaccessible.ts']; + await fs.writeFile(path.join(tempRootDir, 'file1.txt'), 'content1'); + const problematicFile = path.join(tempRootDir, 'problematic.txt'); + await fs.writeFile(problematicFile, 'content2'); - vi.mocked(fs.statSync).mockImplementation((path: any) => { - if (path.toString() === testPath) { - return { isDirectory: () => true } as fs.Stats; + // To simulate an error on a single file in a cross-platform way, + // we mock fs.stat to throw for a specific file. This avoids + // platform-specific behavior with things like dangling symlinks. + const originalStat = fs.stat; + const statSpy = vi.spyOn(fs, 'stat').mockImplementation(async (p) => { + if (p.toString() === problematicFile) { + throw new Error('Simulated stat error'); } - if (path.toString().endsWith('inaccessible.ts')) { - throw new Error('EACCES: permission denied'); - } - return { - isDirectory: () => false, - mtime: new Date(), - size: 1024, - } as fs.Stats; + return originalStat(p); }); - vi.mocked(fs.readdirSync).mockReturnValue(mockFiles as any); - // Spy on console.error to verify it's called const consoleErrorSpy = vi .spyOn(console, 'error') .mockImplementation(() => {}); - const invocation = lsTool.build({ path: testPath }); - const result = await invocation.execute(new AbortController().signal); + const invocation = lsTool.build({ path: tempRootDir }); + const result = await invocation.execute(abortSignal); - // Should still list the accessible file - expect(result.llmContent).toContain('accessible.ts'); - expect(result.llmContent).not.toContain('inaccessible.ts'); + // Should still list the other files + expect(result.llmContent).toContain('file1.txt'); + expect(result.llmContent).not.toContain('problematic.txt'); expect(result.returnDisplay).toBe('Listed 1 item(s).'); // Verify error was logged expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining('Error accessing'), + expect.stringMatching(/Error accessing.*problematic\.txt/s), ); + statSpy.mockRestore(); consoleErrorSpy.mockRestore(); }); }); describe('getDescription', () => { it('should return shortened relative path', () => { + const deeplyNestedDir = path.join(tempRootDir, 'deeply', 'nested'); const params = { - path: `${mockPrimaryDir}/deeply/nested/directory`, + path: path.join(deeplyNestedDir, 'directory'), }; - vi.mocked(fs.statSync).mockReturnValue({ - isDirectory: () => true, - } as fs.Stats); const invocation = lsTool.build(params); const description = invocation.getDescription(); expect(description).toBe(path.join('deeply', 'nested', 'directory')); @@ -426,31 +284,27 @@ describe('LSTool', () => { it('should handle paths in secondary workspace', () => { const params = { - path: `${mockSecondaryDir}/lib`, + path: path.join(tempSecondaryDir, 'lib'), }; - vi.mocked(fs.statSync).mockReturnValue({ - isDirectory: () => true, - } as fs.Stats); const invocation = lsTool.build(params); const description = invocation.getDescription(); - expect(description).toBe(path.join('..', 'other-project', 'lib')); + const expected = path.relative(tempRootDir, params.path); + expect(description).toBe(expected); }); }); describe('workspace boundary validation', () => { - it('should accept paths in primary workspace directory', () => { - const params = { path: `${mockPrimaryDir}/src` }; - vi.mocked(fs.statSync).mockReturnValue({ - isDirectory: () => true, - } as fs.Stats); + it('should accept paths in primary workspace directory', async () => { + const testPath = path.join(tempRootDir, 'src'); + await fs.mkdir(testPath); + const params = { path: testPath }; expect(lsTool.build(params)).toBeDefined(); }); - it('should accept paths in secondary workspace directory', () => { - const params = { path: `${mockSecondaryDir}/lib` }; - vi.mocked(fs.statSync).mockReturnValue({ - isDirectory: () => true, - } as fs.Stats); + it('should accept paths in secondary workspace directory', async () => { + const testPath = path.join(tempSecondaryDir, 'lib'); + await fs.mkdir(testPath); + const params = { path: testPath }; expect(lsTool.build(params)).toBeDefined(); }); @@ -462,28 +316,16 @@ describe('LSTool', () => { }); it('should list files from secondary workspace directory', async () => { - const testPath = `${mockSecondaryDir}/tests`; - const mockFiles = ['test1.spec.ts', 'test2.spec.ts']; + await fs.writeFile( + path.join(tempSecondaryDir, 'secondary-file.txt'), + 'secondary', + ); - vi.mocked(fs.statSync).mockImplementation((path: any) => { - if (path.toString() === testPath) { - return { isDirectory: () => true } as fs.Stats; - } - return { - isDirectory: () => false, - mtime: new Date(), - size: 512, - } as fs.Stats; - }); + const invocation = lsTool.build({ path: tempSecondaryDir }); + const result = await invocation.execute(abortSignal); - vi.mocked(fs.readdirSync).mockReturnValue(mockFiles as any); - - const invocation = lsTool.build({ path: testPath }); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).toContain('test1.spec.ts'); - expect(result.llmContent).toContain('test2.spec.ts'); - expect(result.returnDisplay).toBe('Listed 2 item(s).'); + expect(result.llmContent).toContain('secondary-file.txt'); + expect(result.returnDisplay).toBe('Listed 1 item(s).'); }); }); }); diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 4a59730612..09c26c796e 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import fs from 'node:fs'; +import fs from 'node:fs/promises'; import path from 'node:path'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; @@ -133,7 +133,7 @@ class LSToolInvocation extends BaseToolInvocation { */ async execute(_signal: AbortSignal): Promise { try { - const stats = fs.statSync(this.params.path); + const stats = await fs.stat(this.params.path); if (!stats) { // fs.statSync throws on non-existence, so this check might be redundant // but keeping for clarity. Error message adjusted. @@ -151,28 +151,7 @@ class LSToolInvocation extends BaseToolInvocation { ); } - const files = fs.readdirSync(this.params.path); - - const defaultFileIgnores = - this.config.getFileFilteringOptions() ?? DEFAULT_FILE_FILTERING_OPTIONS; - - const fileFilteringOptions = { - respectGitIgnore: - this.params.file_filtering_options?.respect_git_ignore ?? - defaultFileIgnores.respectGitIgnore, - respectGeminiIgnore: - this.params.file_filtering_options?.respect_gemini_ignore ?? - defaultFileIgnores.respectGeminiIgnore, - }; - - // Get centralized file discovery service - - const fileDiscovery = this.config.getFileService(); - - const entries: FileEntry[] = []; - let gitIgnoredCount = 0; - let geminiIgnoredCount = 0; - + const files = await fs.readdir(this.params.path); if (files.length === 0) { // Changed error message to be more neutral for LLM return { @@ -181,38 +160,39 @@ class LSToolInvocation extends BaseToolInvocation { }; } - for (const file of files) { - if (this.shouldIgnore(file, this.params.ignore)) { - continue; - } - - const fullPath = path.join(this.params.path, file); - const relativePath = path.relative( + const relativePaths = files.map((file) => + path.relative( this.config.getTargetDir(), - fullPath, - ); + path.join(this.params.path, file), + ), + ); - // Check if this file should be ignored based on git or gemini ignore rules - if ( - fileFilteringOptions.respectGitIgnore && - fileDiscovery.shouldGitIgnoreFile(relativePath) - ) { - gitIgnoredCount++; - continue; - } - if ( - fileFilteringOptions.respectGeminiIgnore && - fileDiscovery.shouldGeminiIgnoreFile(relativePath) - ) { - geminiIgnoredCount++; + const fileDiscovery = this.config.getFileService(); + const { filteredPaths, gitIgnoredCount, geminiIgnoredCount } = + fileDiscovery.filterFilesWithReport(relativePaths, { + respectGitIgnore: + this.params.file_filtering_options?.respect_git_ignore ?? + this.config.getFileFilteringOptions().respectGitIgnore ?? + DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore, + respectGeminiIgnore: + this.params.file_filtering_options?.respect_gemini_ignore ?? + this.config.getFileFilteringOptions().respectGeminiIgnore ?? + DEFAULT_FILE_FILTERING_OPTIONS.respectGeminiIgnore, + }); + + const entries = []; + for (const relativePath of filteredPaths) { + const fullPath = path.resolve(this.config.getTargetDir(), relativePath); + + if (this.shouldIgnore(path.basename(fullPath), this.params.ignore)) { continue; } try { - const stats = fs.statSync(fullPath); + const stats = await fs.stat(fullPath); const isDir = stats.isDirectory(); entries.push({ - name: file, + name: path.basename(fullPath), path: fullPath, isDirectory: isDir, size: isDir ? 0 : stats.size, @@ -244,7 +224,6 @@ class LSToolInvocation extends BaseToolInvocation { if (geminiIgnoredCount > 0) { ignoredMessages.push(`${geminiIgnoredCount} gemini-ignored`); } - if (ignoredMessages.length > 0) { resultMessage += `\n\n(${ignoredMessages.join(', ')})`; } diff --git a/packages/core/src/tools/read-many-files.ts b/packages/core/src/tools/read-many-files.ts index 538e0d9056..0694413762 100644 --- a/packages/core/src/tools/read-many-files.ts +++ b/packages/core/src/tools/read-many-files.ts @@ -18,8 +18,10 @@ import { getSpecificMimeType, } from '../utils/fileUtils.js'; import type { PartListUnion } from '@google/genai'; -import type { Config } from '../config/config.js'; -import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/config.js'; +import { + type Config, + DEFAULT_FILE_FILTERING_OPTIONS, +} from '../config/config.js'; import { FileOperation } from '../telemetry/metrics.js'; import { getProgrammingLanguage } from '../telemetry/telemetry-utils.js'; import { logFileOperation } from '../telemetry/loggers.js'; @@ -173,20 +175,6 @@ ${finalExclusionPatternsForDescription useDefaultExcludes = true, } = this.params; - const defaultFileIgnores = - this.config.getFileFilteringOptions() ?? DEFAULT_FILE_FILTERING_OPTIONS; - - const fileFilteringOptions = { - respectGitIgnore: - this.params.file_filtering_options?.respect_git_ignore ?? - defaultFileIgnores.respectGitIgnore, // Use the property from the returned object - respectGeminiIgnore: - this.params.file_filtering_options?.respect_gemini_ignore ?? - defaultFileIgnores.respectGeminiIgnore, // Use the property from the returned object - }; - // Get centralized file discovery service - const fileDiscovery = this.config.getFileService(); - const filesToConsider = new Set(); const skippedFiles: Array<{ path: string; reason: string }> = []; const processedFilesRelativePaths: string[] = []; @@ -227,71 +215,37 @@ ${finalExclusionPatternsForDescription allEntries.add(entry); } } - const entries = Array.from(allEntries); + const relativeEntries = Array.from(allEntries).map((p) => + path.relative(this.config.getTargetDir(), p), + ); - const gitFilteredEntries = fileFilteringOptions.respectGitIgnore - ? fileDiscovery - .filterFiles( - entries.map((p) => path.relative(this.config.getTargetDir(), p)), - { - respectGitIgnore: true, - respectGeminiIgnore: false, - }, - ) - .map((p) => path.resolve(this.config.getTargetDir(), p)) - : entries; + const fileDiscovery = this.config.getFileService(); + const { filteredPaths, gitIgnoredCount, geminiIgnoredCount } = + fileDiscovery.filterFilesWithReport(relativeEntries, { + respectGitIgnore: + this.params.file_filtering_options?.respect_git_ignore ?? + this.config.getFileFilteringOptions().respectGitIgnore ?? + DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore, + respectGeminiIgnore: + this.params.file_filtering_options?.respect_gemini_ignore ?? + this.config.getFileFilteringOptions().respectGeminiIgnore ?? + DEFAULT_FILE_FILTERING_OPTIONS.respectGeminiIgnore, + }); - // Apply gemini ignore filtering if enabled - const finalFilteredEntries = fileFilteringOptions.respectGeminiIgnore - ? fileDiscovery - .filterFiles( - gitFilteredEntries.map((p) => - path.relative(this.config.getTargetDir(), p), - ), - { - respectGitIgnore: false, - respectGeminiIgnore: true, - }, - ) - .map((p) => path.resolve(this.config.getTargetDir(), p)) - : gitFilteredEntries; - - let gitIgnoredCount = 0; - let geminiIgnoredCount = 0; - - for (const absoluteFilePath of entries) { + for (const relativePath of filteredPaths) { // Security check: ensure the glob library didn't return something outside the workspace. + + const fullPath = path.resolve(this.config.getTargetDir(), relativePath); if ( - !this.config - .getWorkspaceContext() - .isPathWithinWorkspace(absoluteFilePath) + !this.config.getWorkspaceContext().isPathWithinWorkspace(fullPath) ) { skippedFiles.push({ - path: absoluteFilePath, - reason: `Security: Glob library returned path outside workspace. Path: ${absoluteFilePath}`, + path: fullPath, + reason: `Security: Glob library returned path outside workspace. Path: ${fullPath}`, }); continue; } - - // Check if this file was filtered out by git ignore - if ( - fileFilteringOptions.respectGitIgnore && - !gitFilteredEntries.includes(absoluteFilePath) - ) { - gitIgnoredCount++; - continue; - } - - // Check if this file was filtered out by gemini ignore - if ( - fileFilteringOptions.respectGeminiIgnore && - !finalFilteredEntries.includes(absoluteFilePath) - ) { - geminiIgnoredCount++; - continue; - } - - filesToConsider.add(absoluteFilePath); + filesToConsider.add(fullPath); } // Add info about git-ignored files if any were filtered