Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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)
}
}
230 changes: 230 additions & 0 deletions src/core/ignore/GitIgnoreController.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,230 @@
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) {
const exists = await fileExistsAtPath(gitignorePath)

if (exists) {
this.gitignoreFiles.push(gitignorePath)
await this.loadGitignoreFile(gitignorePath)
}
}

// 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) {
Copy link

Choose a reason for hiding this comment

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

[P2] Potential duplicates in gitignoreFiles if a path is discovered via both the predefined list and recursion. De-duplicate before loading to avoid redundant add() calls and reduce churn.

Suggested change
for (const p of this.gitignoreFiles) {
// Load any files discovered by recursion that weren't loaded yet
// De-duplicate discovered paths to avoid redundant loads
this.gitignoreFiles = Array.from(new Set(this.gitignoreFiles))
for (const p of this.gitignoreFiles) {
if (!this.gitignoreContents.has(p)) {
await this.loadGitignoreFile(p)
}
}

if (!this.gitignoreContents.has(p)) {
await this.loadGitignoreFile(p)
}
}

// 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, we need to create patterns that match files within that directory
const lines = content.split(/\r?\n/).filter((line) => line.trim() && !line.startsWith("#"))
Copy link

Choose a reason for hiding this comment

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

[P1] Escaped literals are misparsed. Lines beginning with an escaped '#' (e.g., '#foo') are treated as comments and dropped; and lines beginning with an escaped '!' (e.g., '!keep.txt') are treated as negations. Gitignore semantics: '#' and '!' at the start denote literal '#' and '!' patterns. Parse comments only when the first non-whitespace char is an unescaped '#', and unescape a leading '#' or '!' before further processing.

Copy link

Choose a reason for hiding this comment

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

P2: Comment detection trims content but checks startsWith on the untrimmed line. Lines like ' # comment' will be treated as patterns and then fed to ignore(), which is not intended and can create bogus entries. Use the trimmed string for the comment check (escaped # still passes through as intended).

Suggested change
const lines = content.split(/\r?\n/).filter((line) => line.trim() && !line.startsWith("#"))
const lines = content
.split(/\r?\n/)
.filter((line) => {
const t = line.trim();
return t.length > 0 && !t.startsWith("#");
});

Copy link
Author

Choose a reason for hiding this comment

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

@roomote-agent we need test coverage for this at this point. Can you confirm that the tests don't cover this?

Copy link

Choose a reason for hiding this comment

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

P3: Comment detection uses !line.startsWith('#') on the untrimmed line. Lines with leading spaces before # become patterns, and escaped \# should be treated as a literal #. Trim leading whitespace and only treat as a comment if the first non-space char is an unescaped #.

Suggested change
const lines = content.split(/\r?\n/).filter((line) => line.trim() && !line.startsWith("#"))
const lines = content.split(/\r?\n/).filter((line) => {
const s = line.trimStart();
return s && !(s.startsWith('#') && !s.startsWith('\\#'));
});

Copy link

Choose a reason for hiding this comment

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

[P1] Escaped '#' patterns (\#) are valid gitignore entries (literal '#') and should not be dropped as comments. The current filter removes them.

Suggested change
const lines = content.split(/\r?\n/).filter((line) => line.trim() && !line.startsWith("#"))
const lines = content
.split(/\r?\n/)
.map((l) => l.trim())
.filter((l) => l && !(l.startsWith("#") && !l.startsWith("\\#")))

Copy link
Author

Choose a reason for hiding this comment

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

there is test coverage for this and they all pass.

// Convert Windows paths to POSIX for consistent pattern matching
const normalizedRelativeDir = relativeDir.split(path.sep).join("/")

const adjustedPatterns = lines.flatMap((pattern) => {
const trimmed = pattern.trim()

if (trimmed.startsWith("/")) {
// Absolute patterns (starting with /) are relative to the .gitignore location
return [normalizedRelativeDir + trimmed]
} 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 ["!" + normalizedRelativeDir + negatedPattern]
} else {
// For relative negation patterns, match in the directory and subdirectories
return [
"!" + normalizedRelativeDir + "/" + negatedPattern,
"!" + normalizedRelativeDir + "/**/" + negatedPattern,
]
}
} else {
// Relative patterns - match files in the directory and all subdirectories
// For "*.tmp" in src/.gitignore, we need TWO patterns:
// - src/*.tmp (matches direct children like src/temp.tmp)
// - src/**/*.tmp (matches descendants like src/subdir/temp.tmp)
const patterns = [
normalizedRelativeDir + "/" + trimmed,
normalizedRelativeDir + "/**/" + trimmed,
]
return patterns
}
})

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