Skip to content

Follow symlinks for rules files #2421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mrubens merged 1 commit into from
Apr 9, 2025
Merged

Follow symlinks for rules files #2421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
105 changes: 99 additions & 6 deletions src/core/prompts/sections/__tests__/custom-instructions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@ jest.mock("fs/promises")
const readFileMock = jest.fn()
const statMock = jest.fn()
const readdirMock = jest.fn()
const readlinkMock = jest.fn()

// Replace fs functions with our mocks
fs.readFile = readFileMock as any
fs.stat = statMock as any
fs.readdir = readdirMock as any
fs.readlink = readlinkMock as any

// Mock path.resolve and path.join to be predictable in tests
jest.mock("path", () => ({
Expand Down Expand Up @@ -127,8 +129,8 @@ describe("loadRuleFiles", () => {

// Simulate listing files
readdirMock.mockResolvedValueOnce([
{ name: "file1.txt", isFile: () => true, parentPath: "/fake/path/.roo/rules" },
{ name: "file2.txt", isFile: () => true, parentPath: "/fake/path/.roo/rules" },
{ name: "file1.txt", isFile: () => true, isSymbolicLink: () => false, parentPath: "/fake/path/.roo/rules" },
{ name: "file2.txt", isFile: () => true, isSymbolicLink: () => false, parentPath: "/fake/path/.roo/rules" },
] as any)

statMock.mockImplementation(
Expand All @@ -154,6 +156,8 @@ describe("loadRuleFiles", () => {
expect(result).toContain("# Rules from /fake/path/.roo/rules/file2.txt:")
expect(result).toContain("content of file2")

// We expect both checks because our new implementation checks the files again for validation
expect(statMock).toHaveBeenCalledWith("/fake/path/.roo/rules")
expect(statMock).toHaveBeenCalledWith("/fake/path/.roo/rules/file1.txt")
expect(statMock).toHaveBeenCalledWith("/fake/path/.roo/rules/file2.txt")
expect(readFileMock).toHaveBeenCalledWith("/fake/path/.roo/rules/file1.txt", "utf-8")
Expand Down Expand Up @@ -210,17 +214,31 @@ describe("loadRuleFiles", () => {

// Simulate listing files including subdirectories
readdirMock.mockResolvedValueOnce([
{ name: "subdir", isFile: () => false, isDirectory: () => true, parentPath: "/fake/path/.roo/rules" },
{ name: "root.txt", isFile: () => true, isDirectory: () => false, parentPath: "/fake/path/.roo/rules" },
{
name: "subdir",
isFile: () => false,
isSymbolicLink: () => false,
isDirectory: () => true,
parentPath: "/fake/path/.roo/rules",
},
{
name: "root.txt",
isFile: () => true,
isSymbolicLink: () => false,
isDirectory: () => false,
parentPath: "/fake/path/.roo/rules",
},
{
name: "nested1.txt",
isFile: () => true,
isSymbolicLink: () => false,
isDirectory: () => false,
parentPath: "/fake/path/.roo/rules/subdir",
},
{
name: "nested2.txt",
isFile: () => true,
isSymbolicLink: () => false,
isDirectory: () => false,
parentPath: "/fake/path/.roo/rules/subdir/subdir2",
},
Expand Down Expand Up @@ -395,8 +413,18 @@ describe("addCustomInstructions", () => {

// Simulate listing files
readdirMock.mockResolvedValueOnce([
{ name: "rule1.txt", isFile: () => true, parentPath: "/fake/path/.roo/rules-test-mode" },
{ name: "rule2.txt", isFile: () => true, parentPath: "/fake/path/.roo/rules-test-mode" },
{
name: "rule1.txt",
isFile: () => true,
isSymbolicLink: () => false,
parentPath: "/fake/path/.roo/rules-test-mode",
},
{
name: "rule2.txt",
isFile: () => true,
isSymbolicLink: () => false,
parentPath: "/fake/path/.roo/rules-test-mode",
},
] as any)

statMock.mockImplementation(
Expand Down Expand Up @@ -430,6 +458,7 @@ describe("addCustomInstructions", () => {
expect(result).toContain("# Rules from /fake/path/.roo/rules-test-mode/rule2.txt:")
expect(result).toContain("mode specific rule 2")

expect(statMock).toHaveBeenCalledWith("/fake/path/.roo/rules-test-mode")
expect(statMock).toHaveBeenCalledWith("/fake/path/.roo/rules-test-mode/rule1.txt")
expect(statMock).toHaveBeenCalledWith("/fake/path/.roo/rules-test-mode/rule2.txt")
expect(readFileMock).toHaveBeenCalledWith("/fake/path/.roo/rules-test-mode/rule1.txt", "utf-8")
Expand Down Expand Up @@ -579,6 +608,70 @@ describe("Directory existence checks", () => {

// Indirectly test readTextFilesFromDirectory and formatDirectoryContent through loadRuleFiles
describe("Rules directory reading", () => {
it("should follow symbolic links in the rules directory", async () => {
// Simulate .roo/rules directory exists
statMock.mockResolvedValueOnce({
isDirectory: jest.fn().mockReturnValue(true),
} as any)

// Simulate listing files including a symlink
readdirMock.mockResolvedValueOnce([
{
name: "regular.txt",
isFile: () => true,
isSymbolicLink: () => false,
parentPath: "/fake/path/.roo/rules",
},
{ name: "link.txt", isFile: () => false, isSymbolicLink: () => true, parentPath: "/fake/path/.roo/rules" },
] as any)

// Simulate readlink response
readlinkMock.mockResolvedValueOnce("../symlink-target.txt")

// Reset and set up the stat mock with more granular control
statMock.mockReset()
statMock.mockImplementation((path: string) => {
// For directory check
if (path === "/fake/path/.roo/rules") {
return Promise.resolve({
isDirectory: jest.fn().mockReturnValue(true),
isFile: jest.fn().mockReturnValue(false),
} as any)
}

// For all files
return Promise.resolve({
isFile: jest.fn().mockReturnValue(true),
isDirectory: jest.fn().mockReturnValue(false),
} as any)
})

// Simulate file content reading
readFileMock.mockImplementation((filePath: PathLike) => {
if (filePath.toString() === "/fake/path/.roo/rules/regular.txt") {
return Promise.resolve("regular file content")
}
if (filePath.toString() === "/fake/path/.roo/rules/../symlink-target.txt") {
return Promise.resolve("symlink target content")
}
return Promise.reject({ code: "ENOENT" })
})

const result = await loadRuleFiles("/fake/path")

// Verify both regular file and symlink target content are included
expect(result).toContain("# Rules from /fake/path/.roo/rules/regular.txt:")
expect(result).toContain("regular file content")
expect(result).toContain("# Rules from /fake/path/.roo/rules/../symlink-target.txt:")
expect(result).toContain("symlink target content")

// Verify readlink was called with the symlink path
expect(readlinkMock).toHaveBeenCalledWith("/fake/path/.roo/rules/link.txt")

// Verify both files were read
expect(readFileMock).toHaveBeenCalledWith("/fake/path/.roo/rules/regular.txt", "utf-8")
expect(readFileMock).toHaveBeenCalledWith("/fake/path/.roo/rules/../symlink-target.txt", "utf-8")
})
beforeEach(() => {
jest.clearAllMocks()
})
Expand Down
33 changes: 28 additions & 5 deletions src/core/prompts/sections/custom-instructions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,36 @@ async function directoryExists(dirPath: string): Promise<boolean> {
*/
async function readTextFilesFromDirectory(dirPath: string): Promise<Array<{ filename: string; content: string }>> {
try {
const files = await fs
.readdir(dirPath, { withFileTypes: true, recursive: true })
.then((files) => files.filter((file) => file.isFile()))
.then((files) => files.map((file) => path.resolve(file.parentPath, file.name)))
const entries = await fs.readdir(dirPath, { withFileTypes: true, recursive: true })

// Process all entries - regular files and symlinks that might point to files
const filePaths: string[] = []

for (const entry of entries) {
const fullPath = path.resolve(entry.parentPath || dirPath, entry.name)
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Apr 9, 2025

Choose a reason for hiding this comment

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

Non-standard use of entry.parentPath in readTextFilesFromDirectory. Dirent objects from fs.readdir do not include this property by default. Consider documenting the custom use or calculating the full path via dirPath.

if (entry.isFile()) {
// Regular file
filePaths.push(fullPath)
} else if (entry.isSymbolicLink()) {
try {
// Get the symlink target
const linkTarget = await fs.readlink(fullPath)
// Resolve the target path (relative to the symlink location)
const resolvedTarget = path.resolve(path.dirname(fullPath), linkTarget)

// Check if the target is a file
const stats = await fs.stat(resolvedTarget)
if (stats.isFile()) {
filePaths.push(resolvedTarget)
}
} catch (err) {
// Skip invalid symlinks
}
}
}

const fileContents = await Promise.all(
files.map(async (file) => {
filePaths.map(async (file) => {
try {
// Check if it's a file (not a directory)
const stats = await fs.stat(file)
Expand Down