Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 122 additions & 0 deletions src/core/ignore/BaseIgnoreController.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import path from "path"
import fsSync from "fs"
import ignore, { Ignore } from "ignore"
import * as vscode from "vscode"

/**
* Base class for ignore controllers that provides common functionality
* for handling ignore patterns and file validation.
*/
export abstract class BaseIgnoreController {
protected cwd: string
protected ignoreInstance: Ignore
protected disposables: vscode.Disposable[] = []

constructor(cwd: string) {
this.cwd = cwd
this.ignoreInstance = ignore()
}

/**
* Initialize the controller - must be implemented by subclasses
*/
abstract initialize(): Promise<void>

/**
* Check if a file should be accessible (not ignored by patterns)
* Automatically resolves symlinks
* @param filePath - Path to check (relative to cwd)
* @returns true if file is accessible, false if ignored
*/
validateAccess(filePath: string): boolean {
// Allow subclasses to override the "no patterns" check
if (!this.hasPatterns()) {
return true
}

try {
const absolutePath = path.resolve(this.cwd, filePath)

// Follow symlinks to get the real path
let realPath: string
try {
realPath = fsSync.realpathSync(absolutePath)
} catch {
// If realpath fails (file doesn't exist, broken symlink, etc.),
// use the original path
realPath = absolutePath
}

// Convert real path to relative for ignore checking
const relativePath = path.relative(this.cwd, realPath).toPosix()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expression path.relative(this.cwd, realPath).toPosix() uses a non-standard method. Consider using a standard alternative (e.g. path.posix.relative(this.cwd, realPath) or replacing backslashes) to ensure cross-platform consistency.

Suggested change
const relativePath = path.relative(this.cwd, realPath).toPosix()
const relativePath = path.posix.relative(this.cwd, realPath)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P0] String.prototype.toPosix() does not exist and will throw at runtime. Normalize separators explicitly before passing to the ignore matcher.

Suggested change
const relativePath = path.relative(this.cwd, realPath).toPosix()
const relativePath = path.relative(this.cwd, realPath).split(path.sep).join("/")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we tested this and it's already in prod in the rooIgnoreController line 108
const relativePath = path.relative(this.cwd, realPath).toPosix()


// Check if the real path is ignored
return !this.ignoreInstance.ignores(relativePath)
} catch (error) {
// Allow access to files outside cwd or on errors (backward compatibility)
return true
}
}

/**
* Filter an array of paths, removing those that should be ignored
* @param paths - Array of paths to filter (relative to cwd)
* @returns Array of allowed paths
*/
filterPaths(paths: string[]): string[] {
try {
return paths
.map((p) => ({
path: p,
allowed: this.validateAccess(p),
}))
.filter((x) => x.allowed)
.map((x) => x.path)
} catch (error) {
console.error("Error filtering paths:", error)
return [] // Fail closed for security
}
}

/**
* Clean up resources when the controller is no longer needed
*/
dispose(): void {
this.disposables.forEach((d) => d.dispose())
this.disposables = []
}

/**
* Check if the controller has any patterns loaded
* Must be implemented by subclasses
*/
protected abstract hasPatterns(): boolean

/**
* Set up file watchers with debouncing to avoid rapid reloads
* @param pattern - VSCode RelativePattern for the files to watch
* @param reloadCallback - Function to call when files change
*/
protected setupFileWatcher(pattern: vscode.RelativePattern, reloadCallback: () => void): void {
const fileWatcher = vscode.workspace.createFileSystemWatcher(pattern)

// Debounce rapid changes
let reloadTimeout: NodeJS.Timeout | undefined
const debouncedReload = () => {
if (reloadTimeout) {
clearTimeout(reloadTimeout)
}
reloadTimeout = setTimeout(reloadCallback, 100)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Debounced reload can fire after dispose, leading to callbacks running on disposed state. Consider storing the timeout on the instance and clearing it in dispose(), or guarding reloadCallback when disposed.

}

// Watch for changes, creation, and deletion
this.disposables.push(
fileWatcher.onDidChange(debouncedReload),
fileWatcher.onDidCreate(debouncedReload),
fileWatcher.onDidDelete(debouncedReload),
)

// Add fileWatcher itself to disposables
this.disposables.push(fileWatcher)
}
}
203 changes: 203 additions & 0 deletions src/core/ignore/GitIgnoreController.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
import path from "path"
import { fileExistsAtPath } from "../../utils/fs"
import fs from "fs/promises"
import ignore from "ignore"
import * as vscode from "vscode"
import { BaseIgnoreController } from "./BaseIgnoreController"

/**
* Controls file access by enforcing nested .gitignore patterns.
* Handles multiple .gitignore files throughout the directory tree, unlike ripgrep which only honors top-level .gitignore.
* Designed to be instantiated once and passed to file manipulation services.
* Uses the 'ignore' library to support standard .gitignore syntax.
*/
export class GitIgnoreController extends BaseIgnoreController {
private gitignoreFiles: string[] = []
private gitignoreContents: Map<string, string> = new Map()

constructor(cwd: string) {
super(cwd)
this.gitignoreFiles = []
this.gitignoreContents = new Map()
}

/**
* Initialize the controller by discovering and loading all .gitignore files
* Must be called after construction and before using the controller
*/
async initialize(): Promise<void> {
await this.discoverAndLoadGitignoreFiles()
this.setupGitIgnoreWatchers()
}

/**
* Discover and load .gitignore files (root + common subdirectories)
*/
private async discoverAndLoadGitignoreFiles(): Promise<void> {
try {
// Reset state
this.ignoreInstance = ignore()
this.gitignoreFiles = []
this.gitignoreContents.clear()

// Check for common .gitignore file locations (manually defined for simplicity)
const commonGitignorePaths = [
path.join(this.cwd, ".gitignore"), // Root
path.join(this.cwd, "src", ".gitignore"), // src/
path.join(this.cwd, "lib", ".gitignore"), // lib/
path.join(this.cwd, "test", ".gitignore"), // test/
path.join(this.cwd, "tests", ".gitignore"), // tests/
]

// Check each location and load if it exists
for (const gitignorePath of commonGitignorePaths) {
if (await fileExistsAtPath(gitignorePath)) {
this.gitignoreFiles.push(gitignorePath)
await this.loadGitignoreFile(gitignorePath)
}
}

// Always ignore .gitignore files themselves
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Recursive discovery is never used, so nested .gitignore files outside the hardcoded locations won't be honored. findGitignoreFilesRecursively() is defined but never invoked, and discoverAndLoadGitignoreFiles() only checks a fixed set (root/src/lib/test/tests). This undermines the stated behavior and the issue’s requirement to respect nested .gitignore across the tree. Consider invoking recursive discovery and loading newly found files before finalizing patterns.

Suggested change
// Always ignore .gitignore files themselves
// Also discover arbitrary nested .gitignore files across the workspace
await this.findGitignoreFilesRecursively(this.cwd)
// Load any files discovered by recursion that weren't loaded yet
for (const p of this.gitignoreFiles) {
if (!this.gitignoreContents.has(p)) {
await this.loadGitignoreFile(p)
}
}
// Always ignore .gitignore files themselves
this.ignoreInstance.add(".gitignore")

this.ignoreInstance.add(".gitignore")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Only the root-level .gitignore is ignored; nested .gitignore files are not. Use a glob to ignore at all depths so .gitignore files themselves are consistently excluded throughout the tree.

Suggested change
this.ignoreInstance.add(".gitignore")
// Always ignore .gitignore files at any depth
this.ignoreInstance.add("**/.gitignore")

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Only the root .gitignore is excluded from results with this pattern. Nested .gitignore files will still be considered unless we also ignore '**/.gitignore'. This can surface .gitignore files from subdirectories in search results.

Suggested change
this.ignoreInstance.add(".gitignore")
this.ignoreInstance.add([".gitignore", "**/.gitignore"])

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this important? idk if this matters. Sometimes I want roo to edit .gitignore files

} catch (error) {
console.error("Error discovering .gitignore files:", error)
}
}

/**
* Recursively find all .gitignore files in the directory tree
*/
private async findGitignoreFilesRecursively(dirPath: string): Promise<void> {
try {
// Skip the root directory since we already checked it in discoverAndLoadGitignoreFiles
if (dirPath === this.cwd) {
// Get all subdirectories
const entries = await fs.readdir(dirPath, { withFileTypes: true })
const subdirs = entries
.filter((entry) => entry.isDirectory() && !entry.name.startsWith("."))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Hidden directories are skipped during discovery. This prevents picking up .gitignore files in hidden folders (e.g. .github/.gitignore, .vscode/.gitignore). Include hidden directories and let ignore patterns control exclusions.

Suggested change
.filter((entry) => entry.isDirectory() && !entry.name.startsWith("."))
const subdirs = entries
.filter((entry) => entry.isDirectory())
.map((entry) => path.join(dirPath, entry.name))

.map((entry) => path.join(dirPath, entry.name))
Comment on lines +87 to +90
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Recursive discovery traverses large directories like node_modules, dist, build, etc., which can severely impact performance on large workspaces. Consider skipping common heavy directories during traversal.

Suggested change
const entries = await fs.readdir(dirPath, { withFileTypes: true })
const subdirs = entries
.filter((entry) => entry.isDirectory() && !entry.name.startsWith("."))
.map((entry) => path.join(dirPath, entry.name))
const entries = await fs.readdir(dirPath, { withFileTypes: true })
const SKIP_DIRS = new Set(["node_modules", "dist", "build", "out", "coverage", ".git"])
const subdirs = entries
.filter((entry) => entry.isDirectory() && !entry.name.startsWith(".") && !SKIP_DIRS.has(entry.name))
.map((entry) => path.join(dirPath, entry.name))


// Recursively search subdirectories
for (const subdir of subdirs) {
await this.findGitignoreFilesRecursively(subdir)
}
} else {
// For subdirectories, check for .gitignore and continue recursively
const gitignorePath = path.join(dirPath, ".gitignore")

// Check if .gitignore exists in current directory
if (await fileExistsAtPath(gitignorePath)) {
this.gitignoreFiles.push(gitignorePath)
}

// Get all subdirectories
const entries = await fs.readdir(dirPath, { withFileTypes: true })
const subdirs = entries
.filter((entry) => entry.isDirectory() && !entry.name.startsWith("."))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Same discovery issue in the recursive branch: skipping dot-directories can miss nested .gitignore files under hidden folders.

Suggested change
.filter((entry) => entry.isDirectory() && !entry.name.startsWith("."))
const subdirs = entries
.filter((entry) => entry.isDirectory())
.map((entry) => path.join(dirPath, entry.name))

.map((entry) => path.join(dirPath, entry.name))

// Recursively search subdirectories
for (const subdir of subdirs) {
await this.findGitignoreFilesRecursively(subdir)
}
}
} catch (error) {
// Skip directories we can't read
console.debug(`Could not read directory ${dirPath}:`, error)
}
}

/**
* Load content from a specific .gitignore file
*/
private async loadGitignoreFile(gitignoreFile: string): Promise<void> {
try {
const content = await fs.readFile(gitignoreFile, "utf8")
this.gitignoreContents.set(gitignoreFile, content)

// Add patterns to ignore instance with proper context
// For nested .gitignore files, we need to adjust patterns relative to the workspace root
const relativeDir = path.relative(this.cwd, path.dirname(gitignoreFile))

if (relativeDir) {
// For nested .gitignore files, prefix patterns with the relative directory
const lines = content.split("\n").filter((line) => line.trim() && !line.startsWith("#"))
const adjustedPatterns = lines.map((pattern) => {
const trimmed = pattern.trim()
if (trimmed.startsWith("/")) {
// Absolute patterns (starting with /) are relative to the .gitignore location
return path.posix.join(relativeDir, trimmed.slice(1))
} else if (trimmed.startsWith("!")) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Negation handling doesn't account for escaped '!'. A pattern like '!keep.txt' should be treated as a literal starting with '!', not a re-include. Add a guard before the negation branch to detect and strip a leading backslash before '!' so it is treated as a literal pattern.

// Negation patterns
const negatedPattern = trimmed.slice(1)
if (negatedPattern.startsWith("/")) {
return "!" + path.posix.join(relativeDir, negatedPattern.slice(1))
} else {
return "!" + path.posix.join(relativeDir, "**", negatedPattern)
}
} else {
// Relative patterns apply to the directory and all subdirectories
return path.posix.join(relativeDir, "**", trimmed)
}
})

this.ignoreInstance.add(adjustedPatterns)
} else {
// Root .gitignore file - add patterns as-is (like RooIgnoreController)
this.ignoreInstance.add(content)
}
} catch (error) {
console.warn(`Could not read .gitignore at ${gitignoreFile}:`, error)
}
}

/**
* Set up file watchers for all .gitignore files in the workspace
*/
private setupGitIgnoreWatchers(): void {
// Create a watcher for .gitignore files throughout the workspace
const gitignorePattern = new vscode.RelativePattern(this.cwd, "**/.gitignore")
this.setupFileWatcher(gitignorePattern, () => this.discoverAndLoadGitignoreFiles())
}

/**
* Check if the controller has any patterns loaded
*/
protected hasPatterns(): boolean {
return this.gitignoreFiles.length > 0
}

/**
* Get all discovered .gitignore file paths
* @returns Array of absolute paths to .gitignore files
*/
getGitignoreFiles(): string[] {
return [...this.gitignoreFiles]
}

/**
* Get the content of a specific .gitignore file
* @param gitignoreFile - Absolute path to the .gitignore file
* @returns Content of the file or undefined if not found
*/
getGitignoreContent(gitignoreFile: string): string | undefined {
return this.gitignoreContents.get(gitignoreFile)
}

/**
* Check if any .gitignore files exist in the workspace
* @returns true if at least one .gitignore file exists
*/
hasGitignoreFiles(): boolean {
return this.gitignoreFiles.length > 0
}

/**
* Clean up resources when the controller is no longer needed
*/
override dispose(): void {
super.dispose()
this.gitignoreContents.clear()
this.gitignoreFiles = []
}
}
Loading
Loading