Skip to content

Commit e453690

Browse files
taisukeoetaisukeoe
authored
Fix bug not to respect symbolic linked rules, if target is a directory or another symbolic link (#2513)
* read symbolic linked dir and files recursively * add symlinked dir and nested symlink test case for custom-instructions * enhance comments * add changeset
1 parent 2eba534 commit e453690

File tree

3 files changed

+132
-31
lines changed

3 files changed

+132
-31
lines changed

‎.changeset/metal-papayas-think.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"roo-cline": patch
3+
---
4+
5+
Fix bug not to respect symbolic linked rules, if target is a directory or another symbolic link

‎src/core/prompts/sections/__tests__/custom-instructions.test.ts‎

Lines changed: 58 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -615,30 +615,64 @@ describe("Rules directory reading", () => {
615615
} as any)
616616

617617
// Simulate listing files including a symlink
618-
readdirMock.mockResolvedValueOnce([
619-
{
620-
name: "regular.txt",
621-
isFile: () => true,
622-
isSymbolicLink: () => false,
623-
parentPath: "/fake/path/.roo/rules",
624-
},
625-
{ name: "link.txt", isFile: () => false, isSymbolicLink: () => true, parentPath: "/fake/path/.roo/rules" },
626-
] as any)
618+
readdirMock
619+
.mockResolvedValueOnce([
620+
{
621+
name: "regular.txt",
622+
isFile: () => true,
623+
isSymbolicLink: () => false,
624+
parentPath: "/fake/path/.roo/rules",
625+
},
626+
{
627+
name: "link.txt",
628+
isFile: () => false,
629+
isSymbolicLink: () => true,
630+
parentPath: "/fake/path/.roo/rules",
631+
},
632+
{
633+
name: "link_dir",
634+
isFile: () => false,
635+
isSymbolicLink: () => true,
636+
parentPath: "/fake/path/.roo/rules",
637+
},
638+
{
639+
name: "nested_link.txt",
640+
isFile: () => false,
641+
isSymbolicLink: () => true,
642+
parentPath: "/fake/path/.roo/rules",
643+
},
644+
] as any)
645+
.mockResolvedValueOnce([
646+
{ name: "subdir_link.txt", isFile: () => true, parentPath: "/fake/path/.roo/rules/symlink-target-dir" },
647+
] as any)
627648

628649
// Simulate readlink response
629-
readlinkMock.mockResolvedValueOnce("../symlink-target.txt")
650+
readlinkMock
651+
.mockResolvedValueOnce("../symlink-target.txt")
652+
.mockResolvedValueOnce("../symlink-target-dir")
653+
.mockResolvedValueOnce("../nested-symlink")
654+
.mockResolvedValueOnce("nested-symlink-target.txt")
630655

631656
// Reset and set up the stat mock with more granular control
632657
statMock.mockReset()
633658
statMock.mockImplementation((path: string) => {
634659
// For directory check
635-
if (path === "/fake/path/.roo/rules") {
660+
if (path === "/fake/path/.roo/rules" || path.endsWith("dir")) {
636661
return Promise.resolve({
637662
isDirectory: jest.fn().mockReturnValue(true),
638663
isFile: jest.fn().mockReturnValue(false),
639664
} as any)
640665
}
641666

667+
// For symlink check
668+
if (path.endsWith("symlink")) {
669+
return Promise.resolve({
670+
isDirectory: jest.fn().mockReturnValue(false),
671+
isFile: jest.fn().mockReturnValue(false),
672+
isSymbolicLink: jest.fn().mockReturnValue(true),
673+
} as any)
674+
}
675+
642676
// For all files
643677
return Promise.resolve({
644678
isFile: jest.fn().mockReturnValue(true),
@@ -654,6 +688,12 @@ describe("Rules directory reading", () => {
654688
if (filePath.toString() === "/fake/path/.roo/rules/../symlink-target.txt") {
655689
return Promise.resolve("symlink target content")
656690
}
691+
if (filePath.toString() === "/fake/path/.roo/rules/symlink-target-dir/subdir_link.txt") {
692+
return Promise.resolve("regular file content under symlink target dir")
693+
}
694+
if (filePath.toString() === "/fake/path/.roo/rules/../nested-symlink-target.txt") {
695+
return Promise.resolve("nested symlink target content")
696+
}
657697
return Promise.reject({ code: "ENOENT" })
658698
})
659699

@@ -664,13 +704,20 @@ describe("Rules directory reading", () => {
664704
expect(result).toContain("regular file content")
665705
expect(result).toContain("# Rules from /fake/path/.roo/rules/../symlink-target.txt:")
666706
expect(result).toContain("symlink target content")
707+
expect(result).toContain("# Rules from /fake/path/.roo/rules/symlink-target-dir/subdir_link.txt:")
708+
expect(result).toContain("regular file content under symlink target dir")
709+
expect(result).toContain("# Rules from /fake/path/.roo/rules/../nested-symlink-target.txt:")
710+
expect(result).toContain("nested symlink target content")
667711

668712
// Verify readlink was called with the symlink path
669713
expect(readlinkMock).toHaveBeenCalledWith("/fake/path/.roo/rules/link.txt")
714+
expect(readlinkMock).toHaveBeenCalledWith("/fake/path/.roo/rules/link_dir")
670715

671716
// Verify both files were read
672717
expect(readFileMock).toHaveBeenCalledWith("/fake/path/.roo/rules/regular.txt", "utf-8")
673718
expect(readFileMock).toHaveBeenCalledWith("/fake/path/.roo/rules/../symlink-target.txt", "utf-8")
719+
expect(readFileMock).toHaveBeenCalledWith("/fake/path/.roo/rules/symlink-target-dir/subdir_link.txt", "utf-8")
720+
expect(readFileMock).toHaveBeenCalledWith("/fake/path/.roo/rules/../nested-symlink-target.txt", "utf-8")
674721
})
675722
beforeEach(() => {
676723
jest.clearAllMocks()

‎src/core/prompts/sections/custom-instructions.ts‎

Lines changed: 69 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import fs from "fs/promises"
22
import path from "path"
33

44
import { LANGUAGES, isLanguage } from "../../../shared/language"
5+
import { Dirent } from "fs"
56

67
/**
78
* Safely read a file and return its trimmed content
@@ -31,6 +32,68 @@ async function directoryExists(dirPath: string): Promise<boolean> {
3132
}
3233
}
3334

35+
const MAX_DEPTH = 5
36+
37+
/**
38+
* Recursively resolve directory entries and collect file paths
39+
*/
40+
async function resolveDirectoryEntry(
41+
entry: Dirent,
42+
dirPath: string,
43+
filePaths: string[],
44+
depth: number,
45+
): Promise<void> {
46+
// Avoid cyclic symlinks
47+
if (depth > MAX_DEPTH) {
48+
return
49+
}
50+
51+
const fullPath = path.resolve(entry.parentPath || dirPath, entry.name)
52+
if (entry.isFile()) {
53+
// Regular file
54+
filePaths.push(fullPath)
55+
} else if (entry.isSymbolicLink()) {
56+
// Await the resolution of the symbolic link
57+
await resolveSymLink(fullPath, filePaths, depth + 1)
58+
}
59+
}
60+
61+
/**
62+
* Recursively resolve a symbolic link and collect file paths
63+
*/
64+
async function resolveSymLink(fullPath: string, filePaths: string[], depth: number): Promise<void> {
65+
// Avoid cyclic symlinks
66+
if (depth > MAX_DEPTH) {
67+
return
68+
}
69+
try {
70+
// Get the symlink target
71+
const linkTarget = await fs.readlink(fullPath)
72+
// Resolve the target path (relative to the symlink location)
73+
const resolvedTarget = path.resolve(path.dirname(fullPath), linkTarget)
74+
75+
// Check if the target is a file
76+
const stats = await fs.stat(resolvedTarget)
77+
if (stats.isFile()) {
78+
filePaths.push(resolvedTarget)
79+
} else if (stats.isDirectory()) {
80+
const anotherEntries = await fs.readdir(resolvedTarget, { withFileTypes: true, recursive: true })
81+
// Collect promises for recursive calls within the directory
82+
const directoryPromises: Promise<void>[] = []
83+
for (const anotherEntry of anotherEntries) {
84+
directoryPromises.push(resolveDirectoryEntry(anotherEntry, resolvedTarget, filePaths, depth + 1))
85+
}
86+
// Wait for all entries in the resolved directory to be processed
87+
await Promise.all(directoryPromises)
88+
} else if (stats.isSymbolicLink()) {
89+
// Handle nested symlinks by awaiting the recursive call
90+
await resolveSymLink(resolvedTarget, filePaths, depth + 1)
91+
}
92+
} catch (err) {
93+
// Skip invalid symlinks
94+
}
95+
}
96+
3497
/**
3598
* Read all text files from a directory in alphabetical order
3699
*/
@@ -40,30 +103,16 @@ async function readTextFilesFromDirectory(dirPath: string): Promise<Array<{ file
40103

41104
// Process all entries - regular files and symlinks that might point to files
42105
const filePaths: string[] = []
106+
// Collect promises for the initial resolution calls
107+
const initialPromises: Promise<void>[] = []
43108

44109
for (const entry of entries) {
45-
const fullPath = path.resolve(entry.parentPath || dirPath, entry.name)
46-
if (entry.isFile()) {
47-
// Regular file
48-
filePaths.push(fullPath)
49-
} else if (entry.isSymbolicLink()) {
50-
try {
51-
// Get the symlink target
52-
const linkTarget = await fs.readlink(fullPath)
53-
// Resolve the target path (relative to the symlink location)
54-
const resolvedTarget = path.resolve(path.dirname(fullPath), linkTarget)
55-
56-
// Check if the target is a file
57-
const stats = await fs.stat(resolvedTarget)
58-
if (stats.isFile()) {
59-
filePaths.push(resolvedTarget)
60-
}
61-
} catch (err) {
62-
// Skip invalid symlinks
63-
}
64-
}
110+
initialPromises.push(resolveDirectoryEntry(entry, dirPath, filePaths, 0))
65111
}
66112

113+
// Wait for all asynchronous operations (including recursive ones) to complete
114+
await Promise.all(initialPromises)
115+
67116
const fileContents = await Promise.all(
68117
filePaths.map(async (file) => {
69118
try {

0 commit comments

Comments
 (0)