centralize file filtering in `FileDiscoveryService` (#1039)

6 files changed, 67 insertions, 68 deletions

diff --git a/packages/core/src/utils/bfsFileSearch.test.ts b/packages/core/src/utils/bfsFileSearch.test.ts
index f313c427..83e9b0b9 100644
--- a/packages/core/src/utils/bfsFileSearch.test.ts
+++ b/packages/core/src/utils/bfsFileSearch.test.ts
@@ -4,18 +4,19 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { Dirent, PathLike } from 'fs';
+import * as fs from 'fs';
 import { vi, describe, it, expect, beforeEach } from 'vitest';
-import * as fs from 'fs/promises';
+import * as fsPromises from 'fs/promises';
 import * as gitUtils from './gitUtils.js';
 import { bfsFileSearch } from './bfsFileSearch.js';
 import { FileDiscoveryService } from '../services/fileDiscoveryService.js';
 
+vi.mock('fs');
 vi.mock('fs/promises');
 vi.mock('./gitUtils.js');
 
-const createMockDirent = (name: string, isFile: boolean): Dirent => {
-  const dirent = new Dirent();
+const createMockDirent = (name: string, isFile: boolean): fs.Dirent => {
+  const dirent = new fs.Dirent();
   dirent.name = name;
   dirent.isFile = () => isFile;
   dirent.isDirectory = () => !isFile;
@@ -24,9 +25,9 @@ const createMockDirent = (name: string, isFile: boolean): Dirent => {
 
 // Type for the specific overload we're using
 type ReaddirWithFileTypes = (
-  path: PathLike,
+  path: fs.PathLike,
   options: { withFileTypes: true },
-) => Promise<Dirent[]>;
+) => Promise<fs.Dirent[]>;
 
 describe('bfsFileSearch', () => {
   beforeEach(() => {
@@ -34,7 +35,7 @@ describe('bfsFileSearch', () => {
   });
 
   it('should find a file in the root directory', async () => {
-    const mockFs = vi.mocked(fs);
+    const mockFs = vi.mocked(fsPromises);
     const mockReaddir = mockFs.readdir as unknown as ReaddirWithFileTypes;
     vi.mocked(mockReaddir).mockResolvedValue([
       createMockDirent('file1.txt', true),
@@ -46,7 +47,7 @@ describe('bfsFileSearch', () => {
   });
 
   it('should find a file in a subdirectory', async () => {
-    const mockFs = vi.mocked(fs);
+    const mockFs = vi.mocked(fsPromises);
     const mockReaddir = mockFs.readdir as unknown as ReaddirWithFileTypes;
     vi.mocked(mockReaddir).mockImplementation(async (dir) => {
       if (dir === '/test') {
@@ -63,7 +64,7 @@ describe('bfsFileSearch', () => {
   });
 
   it('should ignore specified directories', async () => {
-    const mockFs = vi.mocked(fs);
+    const mockFs = vi.mocked(fsPromises);
     const mockReaddir = mockFs.readdir as unknown as ReaddirWithFileTypes;
     vi.mocked(mockReaddir).mockImplementation(async (dir) => {
       if (dir === '/test') {
@@ -89,7 +90,7 @@ describe('bfsFileSearch', () => {
   });
 
   it('should respect maxDirs limit', async () => {
-    const mockFs = vi.mocked(fs);
+    const mockFs = vi.mocked(fsPromises);
     const mockReaddir = mockFs.readdir as unknown as ReaddirWithFileTypes;
     vi.mocked(mockReaddir).mockImplementation(async (dir) => {
       if (dir === '/test') {
@@ -115,7 +116,7 @@ describe('bfsFileSearch', () => {
   });
 
   it('should respect .gitignore files', async () => {
-    const mockFs = vi.mocked(fs);
+    const mockFs = vi.mocked(fsPromises);
     const mockGitUtils = vi.mocked(gitUtils);
     mockGitUtils.isGitRepository.mockReturnValue(true);
     const mockReaddir = mockFs.readdir as unknown as ReaddirWithFileTypes;
@@ -135,10 +136,9 @@ describe('bfsFileSearch', () => {
       }
       return [];
     });
-    mockFs.readFile.mockResolvedValue('subdir2');
+    vi.mocked(fs).readFileSync.mockReturnValue('subdir2');
 
     const fileService = new FileDiscoveryService('/test');
-    await fileService.initialize();
     const result = await bfsFileSearch('/test', {
       fileName: 'file1.txt',
       fileService,
diff --git a/packages/core/src/utils/bfsFileSearch.ts b/packages/core/src/utils/bfsFileSearch.ts
index 7cd33c03..e552f520 100644
--- a/packages/core/src/utils/bfsFileSearch.ts
+++ b/packages/core/src/utils/bfsFileSearch.ts
@@ -69,7 +69,7 @@ export async function bfsFileSearch(
 
     for (const entry of entries) {
       const fullPath = path.join(currentDir, entry.name);
-      if (fileService?.shouldIgnoreFile(fullPath)) {
+      if (fileService?.shouldGitIgnoreFile(fullPath)) {
         continue;
       }
 
diff --git a/packages/core/src/utils/getFolderStructure.test.ts b/packages/core/src/utils/getFolderStructure.test.ts
index 63724ba8..83d83624 100644
--- a/packages/core/src/utils/getFolderStructure.test.ts
+++ b/packages/core/src/utils/getFolderStructure.test.ts
@@ -7,6 +7,7 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 import { describe, it, expect, vi, beforeEach, afterEach, Mock } from 'vitest';
 import fsPromises from 'fs/promises';
+import * as fs from 'fs';
 import { Dirent as FSDirent } from 'fs';
 import * as nodePath from 'path';
 import { getFolderStructure } from './getFolderStructure.js';
@@ -23,6 +24,7 @@ vi.mock('path', async (importOriginal) => {
 });
 
 vi.mock('fs/promises');
+vi.mock('fs');
 vi.mock('./gitUtils.js');
 
 // Import 'path' again here, it will be the mocked version
@@ -308,7 +310,7 @@ describe('getFolderStructure gitignore', () => {
       return [];
     });
 
-    (fsPromises.readFile as Mock).mockImplementation(async (p) => {
+    (fs.readFileSync as Mock).mockImplementation((p) => {
       const path = p.toString();
       if (path === '/test/project/.gitignore') {
         return 'ignored.txt\nnode_modules/\n.gemini/\n!/.gemini/config.yaml';
@@ -321,7 +323,6 @@ describe('getFolderStructure gitignore', () => {
 
   it('should ignore files and folders specified in .gitignore', async () => {
     const fileService = new FileDiscoveryService('/test/project');
-    await fileService.initialize();
     const structure = await getFolderStructure('/test/project', {
       fileService,
     });
@@ -332,9 +333,9 @@ describe('getFolderStructure gitignore', () => {
 
   it('should not ignore files if respectGitIgnore is false', async () => {
     const fileService = new FileDiscoveryService('/test/project');
-    await fileService.initialize({ respectGitIgnore: false });
     const structure = await getFolderStructure('/test/project', {
       fileService,
+      respectGitIgnore: false,
     });
     expect(structure).toContain('ignored.txt');
     // node_modules is still ignored by default
diff --git a/packages/core/src/utils/getFolderStructure.ts b/packages/core/src/utils/getFolderStructure.ts
index 7aee377b..6798a147 100644
--- a/packages/core/src/utils/getFolderStructure.ts
+++ b/packages/core/src/utils/getFolderStructure.ts
@@ -26,6 +26,8 @@ interface FolderStructureOptions {
   fileIncludePattern?: RegExp;
   /** For filtering files. */
   fileService?: FileDiscoveryService;
+  /** Whether to use .gitignore patterns. */
+  respectGitIgnore?: boolean;
 }
 
 // Define a type for the merged options where fileIncludePattern remains optional
@@ -124,8 +126,8 @@ async function readFullStructure(
         }
         const fileName = entry.name;
         const filePath = path.join(currentPath, fileName);
-        if (options.fileService) {
-          if (options.fileService.shouldIgnoreFile(filePath)) {
+        if (options.respectGitIgnore && options.fileService) {
+          if (options.fileService.shouldGitIgnoreFile(filePath)) {
             continue;
           }
         }
@@ -159,8 +161,8 @@ async function readFullStructure(
         const subFolderPath = path.join(currentPath, subFolderName);
 
         let isIgnoredByGit = false;
-        if (options?.fileService) {
-          if (options.fileService.shouldIgnoreFile(subFolderPath)) {
+        if (options.respectGitIgnore && options.fileService) {
+          if (options.fileService.shouldGitIgnoreFile(subFolderPath)) {
             isIgnoredByGit = true;
           }
         }
@@ -293,6 +295,7 @@ export async function getFolderStructure(
     ignoredFolders: options?.ignoredFolders ?? DEFAULT_IGNORED_FOLDERS,
     fileIncludePattern: options?.fileIncludePattern,
     fileService: options?.fileService,
+    respectGitIgnore: options?.respectGitIgnore ?? true,
   };
 
   try {
diff --git a/packages/core/src/utils/gitIgnoreParser.test.ts b/packages/core/src/utils/gitIgnoreParser.test.ts
index 12084266..f58d50be 100644
--- a/packages/core/src/utils/gitIgnoreParser.test.ts
+++ b/packages/core/src/utils/gitIgnoreParser.test.ts
@@ -6,12 +6,12 @@
 
 import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest';
 import { GitIgnoreParser } from './gitIgnoreParser.js';
-import * as fs from 'fs/promises';
+import * as fs from 'fs';
 import * as path from 'path';
 import { isGitRepository } from './gitUtils.js';
 
 // Mock fs module
-vi.mock('fs/promises');
+vi.mock('fs');
 
 // Mock gitUtils module
 vi.mock('./gitUtils.js');
@@ -23,8 +23,7 @@ describe('GitIgnoreParser', () => {
   beforeEach(() => {
     parser = new GitIgnoreParser(mockProjectRoot);
     // Reset mocks before each test
-    vi.mocked(fs.readFile).mockClear();
-    vi.mocked(fs.readFile).mockRejectedValue(new Error('ENOENT')); // Default to no file
+    vi.mocked(fs.readFileSync).mockClear();
     vi.mocked(isGitRepository).mockReturnValue(true);
   });
 
@@ -33,11 +32,11 @@ describe('GitIgnoreParser', () => {
   });
 
   describe('initialization', () => {
-    it('should initialize without errors when no .gitignore exists', async () => {
-      await expect(parser.initialize()).resolves.not.toThrow();
+    it('should initialize without errors when no .gitignore exists', () => {
+      expect(() => parser.loadGitRepoPatterns()).not.toThrow();
     });
 
-    it('should load .gitignore patterns when file exists', async () => {
+    it('should load .gitignore patterns when file exists', () => {
       const gitignoreContent = `
 # Comment
 node_modules/
@@ -45,11 +44,9 @@ node_modules/
 /dist
 .env
 `;
-      vi.mocked(fs.readFile)
-        .mockResolvedValueOnce(gitignoreContent)
-        .mockRejectedValue(new Error('ENOENT'));
+      vi.mocked(fs.readFileSync).mockReturnValueOnce(gitignoreContent);
 
-      await parser.initialize();
+      parser.loadGitRepoPatterns();
 
       expect(parser.getPatterns()).toEqual([
         '.git',
@@ -64,8 +61,8 @@ node_modules/
       expect(parser.isIgnored('.env')).toBe(true);
     });
 
-    it('should handle git exclude file', async () => {
-      vi.mocked(fs.readFile).mockImplementation(async (filePath) => {
+    it('should handle git exclude file', () => {
+      vi.mocked(fs.readFileSync).mockImplementation((filePath) => {
         if (
           filePath === path.join(mockProjectRoot, '.git', 'info', 'exclude')
         ) {
@@ -74,30 +71,34 @@ node_modules/
         throw new Error('ENOENT');
       });
 
-      await parser.initialize();
+      parser.loadGitRepoPatterns();
       expect(parser.getPatterns()).toEqual(['.git', 'temp/', '*.tmp']);
       expect(parser.isIgnored('temp/file.txt')).toBe(true);
       expect(parser.isIgnored('src/file.tmp')).toBe(true);
     });
 
-    it('should handle custom patterns file name', async () => {
+    it('should handle custom patterns file name', () => {
       vi.mocked(isGitRepository).mockReturnValue(false);
-      vi.mocked(fs.readFile).mockImplementation(async (filePath) => {
+      vi.mocked(fs.readFileSync).mockImplementation((filePath) => {
         if (filePath === path.join(mockProjectRoot, '.geminiignore')) {
           return 'temp/\n*.tmp';
         }
         throw new Error('ENOENT');
       });
 
-      await parser.initialize('.geminiignore');
+      parser.loadPatterns('.geminiignore');
       expect(parser.getPatterns()).toEqual(['temp/', '*.tmp']);
       expect(parser.isIgnored('temp/file.txt')).toBe(true);
       expect(parser.isIgnored('src/file.tmp')).toBe(true);
     });
+
+    it('should initialize without errors when no .geminiignore exists', () => {
+      expect(() => parser.loadPatterns('.geminiignore')).not.toThrow();
+    });
   });
 
   describe('isIgnored', () => {
-    beforeEach(async () => {
+    beforeEach(() => {
       const gitignoreContent = `
 node_modules/
 *.log
@@ -106,10 +107,8 @@ node_modules/
 src/*.tmp
 !src/important.tmp
 `;
-      vi.mocked(fs.readFile)
-        .mockResolvedValueOnce(gitignoreContent)
-        .mockRejectedValue(new Error('ENOENT'));
-      await parser.initialize();
+      vi.mocked(fs.readFileSync).mockReturnValueOnce(gitignoreContent);
+      parser.loadGitRepoPatterns();
     });
 
     it('should always ignore .git directory', () => {
@@ -165,11 +164,12 @@ src/*.tmp
   });
 
   describe('getIgnoredPatterns', () => {
-    it('should return the raw patterns added', async () => {
+    it('should return the raw patterns added', () => {
       const gitignoreContent = '*.log\n!important.log';
-      vi.mocked(fs.readFile).mockResolvedValueOnce(gitignoreContent);
+      vi.mocked(fs.readFileSync).mockReturnValueOnce(gitignoreContent);
 
-      await parser.initialize();
+      parser.loadGitRepoPatterns();
+      expect(parser.getPatterns()).toEqual(['.git', '*.log', '!important.log']);
     });
   });
 });
diff --git a/packages/core/src/utils/gitIgnoreParser.ts b/packages/core/src/utils/gitIgnoreParser.ts
index 16367a4a..69797282 100644
--- a/packages/core/src/utils/gitIgnoreParser.ts
+++ b/packages/core/src/utils/gitIgnoreParser.ts
@@ -4,18 +4,18 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import * as fs from 'fs/promises';
+import * as fs from 'fs';
 import * as path from 'path';
 import ignore, { type Ignore } from 'ignore';
 import { isGitRepository } from './gitUtils.js';
 
 export interface GitIgnoreFilter {
   isIgnored(filePath: string): boolean;
+  getPatterns(): string[];
 }
 
 export class GitIgnoreParser implements GitIgnoreFilter {
   private projectRoot: string;
-  private isGitRepo: boolean = false;
   private ig: Ignore = ignore();
   private patterns: string[] = [];
 
@@ -23,33 +23,28 @@ export class GitIgnoreParser implements GitIgnoreFilter {
     this.projectRoot = path.resolve(projectRoot);
   }
 
-  async initialize(patternsFileName?: string): Promise<void> {
-    const patternFiles = [];
-    if (patternsFileName && patternsFileName !== '') {
-      patternFiles.push(patternsFileName);
-    }
+  loadGitRepoPatterns(): void {
+    if (!isGitRepository(this.projectRoot)) return;
 
-    this.isGitRepo = isGitRepository(this.projectRoot);
-    if (this.isGitRepo) {
-      patternFiles.push('.gitignore');
-      patternFiles.push(path.join('.git', 'info', 'exclude'));
+    // Always ignore .git directory regardless of .gitignore content
+    this.addPatterns(['.git']);
 
-      // Always ignore .git directory regardless of .gitignore content
-      this.addPatterns(['.git']);
-    }
+    const patternFiles = ['.gitignore', path.join('.git', 'info', 'exclude')];
     for (const pf of patternFiles) {
-      try {
-        await this.loadPatterns(pf);
-      } catch (_error) {
-        // File doesn't exist or can't be read, continue silently
-      }
+      this.loadPatterns(pf);
     }
   }
 
-  async loadPatterns(patternsFileName: string): Promise<void> {
+  loadPatterns(patternsFileName: string): void {
     const patternsFilePath = path.join(this.projectRoot, patternsFileName);
-    const content = await fs.readFile(patternsFilePath, 'utf-8');
-    const patterns = content
+    let content: string;
+    try {
+      content = fs.readFileSync(patternsFilePath, 'utf-8');
+    } catch (_error) {
+      // ignore file not found
+      return;
+    }
+    const patterns = (content ?? '')
       .split('\n')
       .map((p) => p.trim())
       .filter((p) => p !== '' && !p.startsWith('#'));