From 4873fce7919b4d74cee183a91fa8a3af58aef993 Mon Sep 17 00:00:00 2001 From: "Anas H. Sulaiman" Date: Sat, 14 Jun 2025 10:25:34 -0400 Subject: centralize file filtering in `FileDiscoveryService` (#1039) --- packages/core/src/tools/glob.test.ts | 6 +--- packages/core/src/tools/glob.ts | 4 +-- packages/core/src/tools/ls.ts | 5 ++- packages/core/src/tools/read-file.test.ts | 11 +++++-- packages/core/src/tools/read-file.ts | 17 +++------- packages/core/src/tools/read-many-files.test.ts | 24 ++++++--------- packages/core/src/tools/read-many-files.ts | 41 ++++++++++--------------- 7 files changed, 44 insertions(+), 64 deletions(-) (limited to 'packages/core/src/tools') diff --git a/packages/core/src/tools/glob.test.ts b/packages/core/src/tools/glob.test.ts index 21f7f0fe..a4bc99a9 100644 --- a/packages/core/src/tools/glob.test.ts +++ b/packages/core/src/tools/glob.test.ts @@ -20,11 +20,7 @@ describe('GlobTool', () => { // Mock config for testing const mockConfig = { - getFileService: async () => { - const service = new FileDiscoveryService(tempRootDir); - await service.initialize({ respectGitIgnore: true }); - return service; - }, + getFileService: () => new FileDiscoveryService(tempRootDir), getFileFilteringRespectGitIgnore: () => true, } as Partial as Config; diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index d94a380a..0dd2f411 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -221,7 +221,7 @@ export class GlobTool extends BaseTool { const respectGitIgnore = params.respect_git_ignore ?? this.config.getFileFilteringRespectGitIgnore(); - const fileDiscovery = await this.config.getFileService(); + const fileDiscovery = this.config.getFileService(); const entries = (await glob(params.pattern, { cwd: searchDirAbsolute, @@ -239,7 +239,7 @@ export class GlobTool extends BaseTool { let filteredEntries = entries; let gitIgnoredCount = 0; - if (respectGitIgnore && fileDiscovery.isGitRepository()) { + if (respectGitIgnore) { const relativePaths = entries.map((p) => path.relative(this.rootDirectory, p.fullpath()), ); diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 56a016aa..6e3869be 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -233,7 +233,7 @@ export class LSTool extends BaseTool { const respectGitIgnore = params.respect_git_ignore ?? this.config.getFileFilteringRespectGitIgnore(); - const fileDiscovery = await this.config.getFileService(); + const fileDiscovery = this.config.getFileService(); const entries: FileEntry[] = []; let gitIgnoredCount = 0; @@ -257,8 +257,7 @@ export class LSTool extends BaseTool { // Check if this file should be git-ignored (only in git repositories) if ( respectGitIgnore && - fileDiscovery.isGitRepository() && - fileDiscovery.shouldIgnoreFile(relativePath) + fileDiscovery.shouldGitIgnoreFile(relativePath) ) { gitIgnoredCount++; continue; diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index 39c22d06..987b451c 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -11,6 +11,7 @@ import path from 'path'; import os from 'os'; import fs from 'fs'; // For actual fs operations in setup import { Config } from '../config/config.js'; +import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; // Mock fileUtils.processSingleFileContent vi.mock('../utils/fileUtils', async () => { @@ -34,9 +35,14 @@ describe('ReadFileTool', () => { tempRootDir = fs.mkdtempSync( path.join(os.tmpdir(), 'read-file-tool-root-'), ); + fs.writeFileSync( + path.join(tempRootDir, '.geminiignore'), + ['foo.*'].join('\n'), + ); + const fileService = new FileDiscoveryService(tempRootDir); const mockConfigInstance = { - getGeminiIgnorePatterns: () => ['**/foo.bar', 'foo.baz', 'foo.*'], - } as Config; + getFileService: () => fileService, + } as unknown as Config; tool = new ReadFileTool(tempRootDir, mockConfigInstance); mockProcessSingleFileContent.mockReset(); }); @@ -235,7 +241,6 @@ describe('ReadFileTool', () => { }; const result = await tool.execute(params, abortSignal); expect(result.returnDisplay).toContain('foo.bar'); - expect(result.returnDisplay).toContain('foo.*'); expect(result.returnDisplay).not.toContain('foo.baz'); }); }); diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index 5c5994d7..bed95746 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -5,7 +5,6 @@ */ import path from 'path'; -import micromatch from 'micromatch'; import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { BaseTool, ToolResult } from './tools.js'; @@ -37,11 +36,10 @@ export interface ReadFileToolParams { */ export class ReadFileTool extends BaseTool { static readonly Name: string = 'read_file'; - private readonly geminiIgnorePatterns: string[]; constructor( private rootDirectory: string, - config: Config, + private config: Config, ) { super( ReadFileTool.Name, @@ -70,7 +68,6 @@ export class ReadFileTool extends BaseTool { }, ); this.rootDirectory = path.resolve(rootDirectory); - this.geminiIgnorePatterns = config.getGeminiIgnorePatterns() || []; } validateToolParams(params: ReadFileToolParams): string | null { @@ -97,16 +94,10 @@ export class ReadFileTool extends BaseTool { return 'Limit must be a positive number'; } - // Check against .geminiignore patterns - if (this.geminiIgnorePatterns.length > 0) { + const fileService = this.config.getFileService(); + if (fileService.shouldGeminiIgnoreFile(params.path)) { const relativePath = makeRelative(params.path, this.rootDirectory); - if (micromatch.isMatch(relativePath, this.geminiIgnorePatterns)) { - // Get patterns that matched to show in the error message - const matchingPatterns = this.geminiIgnorePatterns.filter((p) => - micromatch.isMatch(relativePath, p), - ); - return `File path '${shortenPath(relativePath)}' is ignored by the following .geminiignore pattern(s):\n\n${matchingPatterns.join('\n')}`; - } + return `File path '${shortenPath(relativePath)}' is ignored by .geminiignore pattern(s).`; } return null; diff --git a/packages/core/src/tools/read-many-files.test.ts b/packages/core/src/tools/read-many-files.test.ts index 90a5a312..63cc7bb2 100644 --- a/packages/core/src/tools/read-many-files.test.ts +++ b/packages/core/src/tools/read-many-files.test.ts @@ -21,17 +21,6 @@ describe('ReadManyFilesTool', () => { let tempDirOutsideRoot: string; let mockReadFileFn: Mock; - // Mock config for testing - const mockConfig = { - getFileService: async () => { - const service = new FileDiscoveryService(tempRootDir); - await service.initialize({ respectGitIgnore: true }); - return service; - }, - getFileFilteringRespectGitIgnore: () => true, - getGeminiIgnorePatterns: () => ['**/foo.bar', 'foo.baz', 'foo.*'], - } as Partial as Config; - beforeEach(async () => { tempRootDir = fs.mkdtempSync( path.join(os.tmpdir(), 'read-many-files-root-'), @@ -39,6 +28,13 @@ describe('ReadManyFilesTool', () => { tempDirOutsideRoot = fs.mkdtempSync( path.join(os.tmpdir(), 'read-many-files-external-'), ); + fs.writeFileSync(path.join(tempRootDir, '.geminiignore'), 'foo.*'); + const fileService = new FileDiscoveryService(tempRootDir); + const mockConfig = { + getFileService: () => fileService, + getFileFilteringRespectGitIgnore: () => true, + } as Partial as Config; + tool = new ReadManyFilesTool(tempRootDir, mockConfig); mockReadFileFn = mockControl.mockReadFile; @@ -369,13 +365,13 @@ describe('ReadManyFilesTool', () => { it('should return error if path is ignored by a .geminiignore pattern', async () => { createFile('foo.bar', ''); - createFile('qux/foo.baz', ''); + createFile('bar.ts', ''); createFile('foo.quux', ''); - const params = { paths: ['foo.bar', 'qux/foo.baz', 'foo.quux'] }; + const params = { paths: ['foo.bar', 'bar.ts', 'foo.quux'] }; const result = await tool.execute(params, new AbortController().signal); expect(result.returnDisplay).not.toContain('foo.bar'); - expect(result.returnDisplay).toContain('qux/foo.baz'); expect(result.returnDisplay).not.toContain('foo.quux'); + expect(result.returnDisplay).toContain('bar.ts'); }); }); }); diff --git a/packages/core/src/tools/read-many-files.ts b/packages/core/src/tools/read-many-files.ts index daf9bc04..107e16b3 100644 --- a/packages/core/src/tools/read-many-files.ts +++ b/packages/core/src/tools/read-many-files.ts @@ -119,7 +119,7 @@ export class ReadManyFilesTool extends BaseTool< ToolResult > { static readonly Name: string = 'read_many_files'; - private readonly geminiIgnorePatterns: string[]; + private readonly geminiIgnorePatterns: string[] = []; /** * Creates an instance of ReadManyFilesTool. @@ -191,7 +191,9 @@ Use this tool when the user's query implies needing the content of several files parameterSchema, ); this.targetDir = path.resolve(targetDir); - this.geminiIgnorePatterns = config.getGeminiIgnorePatterns() || []; + this.geminiIgnorePatterns = config + .getFileService() + .getGeminiIgnorePatterns(); } validateParams(params: ReadManyFilesParams): string | null { @@ -292,7 +294,7 @@ Use this tool when the user's query implies needing the content of several files respect_git_ignore ?? this.config.getFileFilteringRespectGitIgnore(); // Get centralized file discovery service - const fileDiscovery = await this.config.getFileService(); + const fileDiscovery = this.config.getFileService(); const toolBaseDir = this.targetDir; const filesToConsider = new Set(); @@ -323,18 +325,16 @@ Use this tool when the user's query implies needing the content of several files signal, }); - // Apply git-aware filtering if enabled and in git repository - const filteredEntries = - respectGitIgnore && fileDiscovery.isGitRepository() - ? fileDiscovery - .filterFiles( - entries.map((p) => path.relative(toolBaseDir, p)), - { - respectGitIgnore, - }, - ) - .map((p) => path.resolve(toolBaseDir, p)) - : entries; + const filteredEntries = respectGitIgnore + ? fileDiscovery + .filterFiles( + entries.map((p) => path.relative(toolBaseDir, p)), + { + respectGitIgnore, + }, + ) + .map((p) => path.resolve(toolBaseDir, p)) + : entries; let gitIgnoredCount = 0; for (const absoluteFilePath of entries) { @@ -348,11 +348,7 @@ Use this tool when the user's query implies needing the content of several files } // Check if this file was filtered out by git ignore - if ( - respectGitIgnore && - fileDiscovery.isGitRepository() && - !filteredEntries.includes(absoluteFilePath) - ) { + if (respectGitIgnore && !filteredEntries.includes(absoluteFilePath)) { gitIgnoredCount++; continue; } @@ -362,12 +358,9 @@ Use this tool when the user's query implies needing the content of several files // Add info about git-ignored files if any were filtered if (gitIgnoredCount > 0) { - const reason = respectGitIgnore - ? 'git-ignored' - : 'filtered by custom ignore patterns'; skippedFiles.push({ path: `${gitIgnoredCount} file(s)`, - reason, + reason: 'ignored', }); } } catch (error) { -- cgit v1.2.3