Skip to content

Commit 02627aa

Browse files
committed
refactor: use ignore package and RooIgnoreController for gitignore handling
- Removed custom gitignore parsing logic from list-files.ts - Added gitignore support to RooIgnoreController using the ignore package - Moved helper functions to ignore-utils.ts for better code organization - Added comprehensive tests for gitignore and whitelist functionality - Maintains backward compatibility while aligning with established patterns This addresses the review feedback about using the centralized RooIgnoreController and the standard ignore package instead of manual parsing.
1 parent 4636605 commit 02627aa

File tree

4 files changed

+415
-133
lines changed

4 files changed

+415
-133
lines changed

src/core/ignore/RooIgnoreController.ts

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { fileExistsAtPath } from "../../utils/fs"
33
import fs from "fs/promises"
44
import ignore, { Ignore } from "ignore"
55
import * as vscode from "vscode"
6+
import { GITIGNORE_WHITELIST } from "../../services/glob/constants"
67

78
export const LOCK_TEXT_SYMBOL = "\u{1F512}"
89

@@ -14,7 +15,9 @@ export const LOCK_TEXT_SYMBOL = "\u{1F512}"
1415
export class RooIgnoreController {
1516
private cwd: string
1617
private ignoreInstance: Ignore
18+
private gitignoreInstance: Ignore | null = null
1719
private disposables: vscode.Disposable[] = []
20+
private whitelist = GITIGNORE_WHITELIST
1821
rooIgnoreContent: string | undefined
1922

2023
constructor(cwd: string) {
@@ -31,6 +34,7 @@ export class RooIgnoreController {
3134
*/
3235
async initialize(): Promise<void> {
3336
await this.loadRooIgnore()
37+
await this.loadGitignore()
3438
}
3539

3640
/**
@@ -79,6 +83,75 @@ export class RooIgnoreController {
7983
}
8084
}
8185

86+
/**
87+
* Load patterns from .gitignore if it exists
88+
*/
89+
async loadGitignore(): Promise<void> {
90+
try {
91+
this.gitignoreInstance = ignore()
92+
const gitignorePath = path.join(this.cwd, ".gitignore")
93+
if (await fileExistsAtPath(gitignorePath)) {
94+
const content = await fs.readFile(gitignorePath, "utf8")
95+
this.gitignoreInstance.add(content)
96+
}
97+
} catch (error) {
98+
console.error("Error loading .gitignore:", error)
99+
this.gitignoreInstance = null
100+
}
101+
}
102+
103+
/**
104+
* Check if a path is whitelisted
105+
* @param filePath - Path to check (relative to cwd)
106+
* @returns true if path is whitelisted
107+
*/
108+
isWhitelisted(filePath: string): boolean {
109+
// Convert to relative path if absolute
110+
let relativePath: string
111+
if (path.isAbsolute(filePath)) {
112+
relativePath = path.relative(this.cwd, filePath)
113+
} else {
114+
relativePath = filePath
115+
}
116+
117+
// Normalize the path
118+
const normalizedPath = path.normalize(relativePath).replace(/\\/g, "/")
119+
120+
return this.whitelist.some((pattern) => {
121+
const normalizedPattern = path.normalize(pattern).replace(/\\/g, "/")
122+
// Check if this is the whitelisted path or under it
123+
return (
124+
normalizedPath === normalizedPattern ||
125+
normalizedPath.startsWith(normalizedPattern + "/") ||
126+
// Check if this is a parent of the whitelisted path
127+
normalizedPattern.startsWith(normalizedPath + "/")
128+
)
129+
})
130+
}
131+
132+
/**
133+
* Check if a path is ignored by gitignore (considering whitelist)
134+
* @param filePath - Path to check (relative to cwd)
135+
* @returns true if path is ignored by gitignore and not whitelisted
136+
*/
137+
isGitignored(filePath: string): boolean {
138+
if (!this.gitignoreInstance) {
139+
return false
140+
}
141+
142+
// Check whitelist first
143+
if (this.isWhitelisted(filePath)) {
144+
return false
145+
}
146+
147+
try {
148+
const relativePath = path.relative(this.cwd, path.resolve(this.cwd, filePath)).replace(/\\/g, "/")
149+
return this.gitignoreInstance.ignores(relativePath)
150+
} catch (error) {
151+
return false
152+
}
153+
}
154+
82155
/**
83156
* Check if a file should be accessible to the LLM
84157
* @param filePath - Path to check (relative to cwd)
Lines changed: 245 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,245 @@
1+
// npx vitest core/ignore/__tests__/RooIgnoreController.gitignore.spec.ts
2+
3+
import type { Mock } from "vitest"
4+
import { RooIgnoreController } from "../RooIgnoreController"
5+
import * as path from "path"
6+
import * as fs from "fs/promises"
7+
import { fileExistsAtPath } from "../../../utils/fs"
8+
import { GITIGNORE_WHITELIST } from "../../../services/glob/constants"
9+
10+
// Mock dependencies
11+
vi.mock("fs/promises")
12+
vi.mock("../../../utils/fs")
13+
14+
// Mock vscode
15+
vi.mock("vscode", () => {
16+
const mockDisposable = { dispose: vi.fn() }
17+
const mockEventEmitter = {
18+
event: vi.fn(),
19+
fire: vi.fn(),
20+
}
21+
22+
return {
23+
workspace: {
24+
createFileSystemWatcher: vi.fn(() => ({
25+
onDidCreate: vi.fn(() => mockDisposable),
26+
onDidChange: vi.fn(() => mockDisposable),
27+
onDidDelete: vi.fn(() => mockDisposable),
28+
dispose: vi.fn(),
29+
})),
30+
},
31+
RelativePattern: vi.fn().mockImplementation((base, pattern) => ({
32+
base,
33+
pattern,
34+
})),
35+
EventEmitter: vi.fn().mockImplementation(() => mockEventEmitter),
36+
Disposable: {
37+
from: vi.fn(),
38+
},
39+
}
40+
})
41+
42+
describe("RooIgnoreController - Gitignore Support", () => {
43+
const TEST_CWD = "/test/path"
44+
let controller: RooIgnoreController
45+
let mockFileExists: Mock<typeof fileExistsAtPath>
46+
let mockReadFile: Mock<typeof fs.readFile>
47+
48+
beforeEach(() => {
49+
// Reset mocks
50+
vi.clearAllMocks()
51+
52+
// Setup fs mocks
53+
mockFileExists = fileExistsAtPath as Mock<typeof fileExistsAtPath>
54+
mockReadFile = fs.readFile as Mock<typeof fs.readFile>
55+
56+
// Create controller
57+
controller = new RooIgnoreController(TEST_CWD)
58+
})
59+
60+
describe("loadGitignore", () => {
61+
it("should load .gitignore patterns on initialization when file exists", async () => {
62+
// Setup mocks to simulate existing .gitignore file
63+
mockFileExists.mockImplementation(async (filePath) => {
64+
const pathStr = filePath.toString()
65+
if (pathStr.endsWith(".gitignore")) return true
66+
return false
67+
})
68+
mockReadFile.mockResolvedValue("node_modules/\n*.log\nbuild/")
69+
70+
// Initialize controller
71+
await controller.initialize()
72+
73+
// Verify gitignore was loaded
74+
expect(mockFileExists).toHaveBeenCalledWith(path.join(TEST_CWD, ".gitignore"))
75+
expect(mockReadFile).toHaveBeenCalledWith(path.join(TEST_CWD, ".gitignore"), "utf8")
76+
77+
// Test that gitignore patterns are applied
78+
expect(controller.isGitignored("node_modules/package.json")).toBe(true)
79+
expect(controller.isGitignored("app.log")).toBe(true)
80+
expect(controller.isGitignored("build/output.js")).toBe(true)
81+
expect(controller.isGitignored("src/app.ts")).toBe(false)
82+
})
83+
84+
it("should handle missing .gitignore gracefully", async () => {
85+
// Setup mocks to simulate missing .gitignore file
86+
mockFileExists.mockResolvedValue(false)
87+
88+
// Initialize controller
89+
await controller.initialize()
90+
91+
// Verify no gitignore patterns are applied
92+
expect(controller.isGitignored("node_modules/package.json")).toBe(false)
93+
expect(controller.isGitignored("build/output.js")).toBe(false)
94+
})
95+
96+
it("should handle errors when loading .gitignore", async () => {
97+
// Setup mocks to simulate error
98+
mockFileExists.mockImplementation(async (filePath) => {
99+
const pathStr = filePath.toString()
100+
if (pathStr.endsWith(".gitignore")) return true
101+
return false
102+
})
103+
mockReadFile.mockRejectedValue(new Error("Test file read error"))
104+
105+
// Spy on console.error
106+
const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {})
107+
108+
// Initialize controller - shouldn't throw
109+
await controller.initialize()
110+
111+
// Verify error was logged
112+
expect(consoleSpy).toHaveBeenCalledWith("Error loading .gitignore:", expect.any(Error))
113+
114+
// Cleanup
115+
consoleSpy.mockRestore()
116+
})
117+
})
118+
119+
describe("isWhitelisted", () => {
120+
beforeEach(async () => {
121+
await controller.initialize()
122+
})
123+
124+
it("should correctly identify whitelisted paths", () => {
125+
// Test exact matches
126+
expect(controller.isWhitelisted(".roo/temp/file.txt")).toBe(true)
127+
expect(controller.isWhitelisted(".roo/temp/")).toBe(true)
128+
expect(controller.isWhitelisted(".roo/temp")).toBe(true)
129+
130+
// Test subdirectories
131+
expect(controller.isWhitelisted(".roo/temp/subdir/file.txt")).toBe(true)
132+
expect(controller.isWhitelisted(".roo/temp/deep/nested/path/file.txt")).toBe(true)
133+
134+
// Test non-whitelisted paths
135+
expect(controller.isWhitelisted(".roo/other/file.txt")).toBe(false)
136+
expect(controller.isWhitelisted("src/.roo/temp/file.txt")).toBe(false)
137+
expect(controller.isWhitelisted("temp/file.txt")).toBe(false)
138+
})
139+
140+
it("should handle absolute paths correctly", () => {
141+
const absolutePath = path.join(TEST_CWD, ".roo/temp/file.txt")
142+
expect(controller.isWhitelisted(absolutePath)).toBe(true)
143+
144+
const nonWhitelistedAbsolute = path.join(TEST_CWD, "node_modules/file.txt")
145+
expect(controller.isWhitelisted(nonWhitelistedAbsolute)).toBe(false)
146+
})
147+
148+
it("should handle path normalization", () => {
149+
// Test with different path separators and formats
150+
expect(controller.isWhitelisted(".roo\\temp\\file.txt")).toBe(true)
151+
expect(controller.isWhitelisted("./.roo/temp/file.txt")).toBe(true)
152+
expect(controller.isWhitelisted(".roo/./temp/file.txt")).toBe(true)
153+
})
154+
})
155+
156+
describe("isGitignored with whitelist", () => {
157+
beforeEach(async () => {
158+
// Setup both .gitignore and .rooignore
159+
mockFileExists.mockImplementation(async (filePath) => {
160+
const pathStr = filePath.toString()
161+
if (pathStr.endsWith(".gitignore")) return true
162+
if (pathStr.endsWith(".rooignore")) return true
163+
return false
164+
})
165+
mockReadFile.mockImplementation(async (filePath) => {
166+
const pathStr = filePath.toString()
167+
if (pathStr.endsWith(".gitignore")) {
168+
return ".roo/\nnode_modules/\n*.log"
169+
}
170+
if (pathStr.endsWith(".rooignore")) {
171+
return "secrets/"
172+
}
173+
throw new Error("Unexpected file")
174+
})
175+
await controller.initialize()
176+
})
177+
178+
it("should respect whitelist even if path is in .gitignore", () => {
179+
// .roo/temp/ is whitelisted even though .roo/ is in .gitignore
180+
expect(controller.isGitignored(".roo/temp/file.txt")).toBe(false)
181+
182+
// .roo/other/ is NOT whitelisted, so it should be gitignored
183+
expect(controller.isGitignored(".roo/other/file.txt")).toBe(true)
184+
185+
// Other gitignored paths should still be ignored
186+
expect(controller.isGitignored("node_modules/package.json")).toBe(true)
187+
expect(controller.isGitignored("app.log")).toBe(true)
188+
})
189+
190+
it("should handle nested whitelisted paths in gitignored directories", () => {
191+
// Even though .roo/ is gitignored, .roo/temp/ is whitelisted
192+
expect(controller.isGitignored(".roo/temp/nested/deep/file.txt")).toBe(false)
193+
expect(controller.isGitignored(".roo/temp/")).toBe(false)
194+
})
195+
196+
it("should return false for non-gitignored paths", () => {
197+
expect(controller.isGitignored("src/app.ts")).toBe(false)
198+
expect(controller.isGitignored("README.md")).toBe(false)
199+
})
200+
})
201+
202+
describe("integration with validateAccess", () => {
203+
beforeEach(async () => {
204+
// Setup both .gitignore and .rooignore
205+
mockFileExists.mockImplementation(async (filePath) => {
206+
const pathStr = filePath.toString()
207+
if (pathStr.endsWith(".gitignore")) return true
208+
if (pathStr.endsWith(".rooignore")) return true
209+
return false
210+
})
211+
mockReadFile.mockImplementation(async (filePath) => {
212+
const pathStr = filePath.toString()
213+
if (pathStr.endsWith(".gitignore")) {
214+
return ".roo/\nnode_modules/"
215+
}
216+
if (pathStr.endsWith(".rooignore")) {
217+
return "secrets/"
218+
}
219+
throw new Error("Unexpected file")
220+
})
221+
await controller.initialize()
222+
})
223+
224+
it("should allow access to whitelisted paths even if gitignored", () => {
225+
// Whitelisted paths should be accessible
226+
expect(controller.validateAccess(".roo/temp/file.txt")).toBe(true)
227+
expect(controller.validateAccess(".roo/temp/subdir/data.json")).toBe(true)
228+
229+
// Rooignored paths should be blocked
230+
expect(controller.validateAccess("secrets/api-key.txt")).toBe(false)
231+
232+
// Regular paths should be accessible
233+
expect(controller.validateAccess("src/app.ts")).toBe(true)
234+
})
235+
})
236+
237+
describe("whitelist constants", () => {
238+
it("should have the correct whitelist entries", () => {
239+
// Verify the whitelist contains expected entries
240+
expect(GITIGNORE_WHITELIST).toContain(".roo/temp")
241+
expect(GITIGNORE_WHITELIST).toBeInstanceOf(Array)
242+
expect(GITIGNORE_WHITELIST.length).toBeGreaterThan(0)
243+
})
244+
})
245+
})

0 commit comments

Comments
 (0)