Skip to content

Commit 0500894

Browse files
roomote[bot]roomote
roomote[bot]
and
roomote
authored
fix: sort symlinked rules files by symlink names, not target names (#5903)
* fix: sort symlinked rules files alphabetically - Add alphabetical sorting to readTextFilesFromDirectory function - Sort by basename of filename (case-insensitive) for consistent order - Fixes issue where symlinked rules were read in random order - Add test case to verify alphabetical sorting behavior Fixes #4131 * chore: remove solution-indicating comment per PR feedback * fix: sort symlinks by their symlink names, not target names - Modified readTextFilesFromDirectory to store both original symlink path and resolved target path - Updated resolveDirectoryEntry and resolveSymLink to track both paths - Sort files by original path (symlink name) but read content from resolved path - Added test to verify symlinks are sorted by their names, not their target names - This ensures consistent alphabetical ordering when using symlinks in rules directories --------- Co-authored-by: Roo Code <[email protected]>
1 parent 2eb586b commit 0500894

File tree

2 files changed

+188
-18
lines changed

2 files changed

+188
-18
lines changed

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

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1033,6 +1033,157 @@ describe("Rules directory reading", () => {
10331033
expect(result).toContain("content of file3")
10341034
})
10351035

1036+
it("should return files in alphabetical order by filename", async () => {
1037+
// Simulate .roo/rules directory exists
1038+
statMock.mockResolvedValueOnce({
1039+
isDirectory: vi.fn().mockReturnValue(true),
1040+
} as any)
1041+
1042+
// Simulate listing files in non-alphabetical order to test sorting
1043+
readdirMock.mockResolvedValueOnce([
1044+
{ name: "zebra.txt", isFile: () => true, parentPath: "/fake/path/.roo/rules" },
1045+
{ name: "alpha.txt", isFile: () => true, parentPath: "/fake/path/.roo/rules" },
1046+
{ name: "Beta.txt", isFile: () => true, parentPath: "/fake/path/.roo/rules" }, // Test case-insensitive sorting
1047+
] as any)
1048+
1049+
statMock.mockImplementation((path) => {
1050+
return Promise.resolve({
1051+
isFile: vi.fn().mockReturnValue(true),
1052+
}) as any
1053+
})
1054+
1055+
readFileMock.mockImplementation((filePath: PathLike) => {
1056+
const pathStr = filePath.toString()
1057+
const normalizedPath = pathStr.replace(/\\/g, "/")
1058+
if (normalizedPath === "/fake/path/.roo/rules/zebra.txt") {
1059+
return Promise.resolve("zebra content")
1060+
}
1061+
if (normalizedPath === "/fake/path/.roo/rules/alpha.txt") {
1062+
return Promise.resolve("alpha content")
1063+
}
1064+
if (normalizedPath === "/fake/path/.roo/rules/Beta.txt") {
1065+
return Promise.resolve("beta content")
1066+
}
1067+
return Promise.reject({ code: "ENOENT" })
1068+
})
1069+
1070+
const result = await loadRuleFiles("/fake/path")
1071+
1072+
// Files should appear in alphabetical order: alpha.txt, Beta.txt, zebra.txt
1073+
const alphaIndex = result.indexOf("alpha content")
1074+
const betaIndex = result.indexOf("beta content")
1075+
const zebraIndex = result.indexOf("zebra content")
1076+
1077+
expect(alphaIndex).toBeLessThan(betaIndex)
1078+
expect(betaIndex).toBeLessThan(zebraIndex)
1079+
1080+
// Verify the expected file paths are in the result
1081+
const expectedAlphaPath =
1082+
process.platform === "win32" ? "\\fake\\path\\.roo\\rules\\alpha.txt" : "/fake/path/.roo/rules/alpha.txt"
1083+
const expectedBetaPath =
1084+
process.platform === "win32" ? "\\fake\\path\\.roo\\rules\\Beta.txt" : "/fake/path/.roo/rules/Beta.txt"
1085+
const expectedZebraPath =
1086+
process.platform === "win32" ? "\\fake\\path\\.roo\\rules\\zebra.txt" : "/fake/path/.roo/rules/zebra.txt"
1087+
1088+
expect(result).toContain(`# Rules from ${expectedAlphaPath}:`)
1089+
expect(result).toContain(`# Rules from ${expectedBetaPath}:`)
1090+
expect(result).toContain(`# Rules from ${expectedZebraPath}:`)
1091+
})
1092+
1093+
it("should sort symlinks by their symlink names, not target names", async () => {
1094+
// Reset mocks
1095+
statMock.mockReset()
1096+
readdirMock.mockReset()
1097+
readlinkMock.mockReset()
1098+
readFileMock.mockReset()
1099+
1100+
// First call: check if .roo/rules directory exists
1101+
statMock.mockResolvedValueOnce({
1102+
isDirectory: vi.fn().mockReturnValue(true),
1103+
} as any)
1104+
1105+
// Simulate listing files with symlinks that point to files with different names
1106+
readdirMock.mockResolvedValueOnce([
1107+
{
1108+
name: "01-first.link",
1109+
isFile: () => false,
1110+
isSymbolicLink: () => true,
1111+
parentPath: "/fake/path/.roo/rules",
1112+
},
1113+
{
1114+
name: "02-second.link",
1115+
isFile: () => false,
1116+
isSymbolicLink: () => true,
1117+
parentPath: "/fake/path/.roo/rules",
1118+
},
1119+
{
1120+
name: "03-third.link",
1121+
isFile: () => false,
1122+
isSymbolicLink: () => true,
1123+
parentPath: "/fake/path/.roo/rules",
1124+
},
1125+
] as any)
1126+
1127+
// Mock readlink to return target paths that would sort differently than symlink names
1128+
readlinkMock
1129+
.mockResolvedValueOnce("../../targets/zzz-last.txt") // 01-first.link -> zzz-last.txt
1130+
.mockResolvedValueOnce("../../targets/aaa-first.txt") // 02-second.link -> aaa-first.txt
1131+
.mockResolvedValueOnce("../../targets/mmm-middle.txt") // 03-third.link -> mmm-middle.txt
1132+
1133+
// Set up stat mock for the remaining calls
1134+
statMock.mockImplementation((path) => {
1135+
const normalizedPath = path.toString().replace(/\\/g, "/")
1136+
// Target files exist and are files
1137+
if (normalizedPath.endsWith(".txt")) {
1138+
return Promise.resolve({
1139+
isFile: vi.fn().mockReturnValue(true),
1140+
isDirectory: vi.fn().mockReturnValue(false),
1141+
} as any)
1142+
}
1143+
return Promise.resolve({
1144+
isFile: vi.fn().mockReturnValue(false),
1145+
isDirectory: vi.fn().mockReturnValue(false),
1146+
} as any)
1147+
})
1148+
1149+
readFileMock.mockImplementation((filePath: PathLike) => {
1150+
const pathStr = filePath.toString()
1151+
const normalizedPath = pathStr.replace(/\\/g, "/")
1152+
if (normalizedPath.endsWith("zzz-last.txt")) {
1153+
return Promise.resolve("content from zzz-last.txt")
1154+
}
1155+
if (normalizedPath.endsWith("aaa-first.txt")) {
1156+
return Promise.resolve("content from aaa-first.txt")
1157+
}
1158+
if (normalizedPath.endsWith("mmm-middle.txt")) {
1159+
return Promise.resolve("content from mmm-middle.txt")
1160+
}
1161+
return Promise.reject({ code: "ENOENT" })
1162+
})
1163+
1164+
const result = await loadRuleFiles("/fake/path")
1165+
1166+
// Content should appear in order of symlink names (01-first, 02-second, 03-third)
1167+
// NOT in order of target names (aaa-first, mmm-middle, zzz-last)
1168+
const firstIndex = result.indexOf("content from zzz-last.txt") // from 01-first.link
1169+
const secondIndex = result.indexOf("content from aaa-first.txt") // from 02-second.link
1170+
const thirdIndex = result.indexOf("content from mmm-middle.txt") // from 03-third.link
1171+
1172+
// All content should be found
1173+
expect(firstIndex).toBeGreaterThan(-1)
1174+
expect(secondIndex).toBeGreaterThan(-1)
1175+
expect(thirdIndex).toBeGreaterThan(-1)
1176+
1177+
// And they should be in the order of symlink names, not target names
1178+
expect(firstIndex).toBeLessThan(secondIndex)
1179+
expect(secondIndex).toBeLessThan(thirdIndex)
1180+
1181+
// Verify the target paths are shown (not symlink paths)
1182+
expect(result).toContain("zzz-last.txt")
1183+
expect(result).toContain("aaa-first.txt")
1184+
expect(result).toContain("mmm-middle.txt")
1185+
})
1186+
10361187
it("should handle empty file list gracefully", async () => {
10371188
// Simulate .roo/rules directory exists
10381189
statMock.mockResolvedValueOnce({

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

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ const MAX_DEPTH = 5
4444
async function resolveDirectoryEntry(
4545
entry: Dirent,
4646
dirPath: string,
47-
filePaths: string[],
47+
fileInfo: Array<{ originalPath: string; resolvedPath: string }>,
4848
depth: number,
4949
): Promise<void> {
5050
// Avoid cyclic symlinks
@@ -54,44 +54,49 @@ async function resolveDirectoryEntry(
5454

5555
const fullPath = path.resolve(entry.parentPath || dirPath, entry.name)
5656
if (entry.isFile()) {
57-
// Regular file
58-
filePaths.push(fullPath)
57+
// Regular file - both original and resolved paths are the same
58+
fileInfo.push({ originalPath: fullPath, resolvedPath: fullPath })
5959
} else if (entry.isSymbolicLink()) {
6060
// Await the resolution of the symbolic link
61-
await resolveSymLink(fullPath, filePaths, depth + 1)
61+
await resolveSymLink(fullPath, fileInfo, depth + 1)
6262
}
6363
}
6464

6565
/**
6666
* Recursively resolve a symbolic link and collect file paths
6767
*/
68-
async function resolveSymLink(fullPath: string, filePaths: string[], depth: number): Promise<void> {
68+
async function resolveSymLink(
69+
symlinkPath: string,
70+
fileInfo: Array<{ originalPath: string; resolvedPath: string }>,
71+
depth: number,
72+
): Promise<void> {
6973
// Avoid cyclic symlinks
7074
if (depth > MAX_DEPTH) {
7175
return
7276
}
7377
try {
7478
// Get the symlink target
75-
const linkTarget = await fs.readlink(fullPath)
79+
const linkTarget = await fs.readlink(symlinkPath)
7680
// Resolve the target path (relative to the symlink location)
77-
const resolvedTarget = path.resolve(path.dirname(fullPath), linkTarget)
81+
const resolvedTarget = path.resolve(path.dirname(symlinkPath), linkTarget)
7882

7983
// Check if the target is a file
8084
const stats = await fs.stat(resolvedTarget)
8185
if (stats.isFile()) {
82-
filePaths.push(resolvedTarget)
86+
// For symlinks to files, store the symlink path as original and target as resolved
87+
fileInfo.push({ originalPath: symlinkPath, resolvedPath: resolvedTarget })
8388
} else if (stats.isDirectory()) {
8489
const anotherEntries = await fs.readdir(resolvedTarget, { withFileTypes: true, recursive: true })
8590
// Collect promises for recursive calls within the directory
8691
const directoryPromises: Promise<void>[] = []
8792
for (const anotherEntry of anotherEntries) {
88-
directoryPromises.push(resolveDirectoryEntry(anotherEntry, resolvedTarget, filePaths, depth + 1))
93+
directoryPromises.push(resolveDirectoryEntry(anotherEntry, resolvedTarget, fileInfo, depth + 1))
8994
}
9095
// Wait for all entries in the resolved directory to be processed
9196
await Promise.all(directoryPromises)
9297
} else if (stats.isSymbolicLink()) {
9398
// Handle nested symlinks by awaiting the recursive call
94-
await resolveSymLink(resolvedTarget, filePaths, depth + 1)
99+
await resolveSymLink(resolvedTarget, fileInfo, depth + 1)
95100
}
96101
} catch (err) {
97102
// Skip invalid symlinks
@@ -106,29 +111,31 @@ async function readTextFilesFromDirectory(dirPath: string): Promise<Array<{ file
106111
const entries = await fs.readdir(dirPath, { withFileTypes: true, recursive: true })
107112

108113
// Process all entries - regular files and symlinks that might point to files
109-
const filePaths: string[] = []
114+
// Store both original path (for sorting) and resolved path (for reading)
115+
const fileInfo: Array<{ originalPath: string; resolvedPath: string }> = []
110116
// Collect promises for the initial resolution calls
111117
const initialPromises: Promise<void>[] = []
112118

113119
for (const entry of entries) {
114-
initialPromises.push(resolveDirectoryEntry(entry, dirPath, filePaths, 0))
120+
initialPromises.push(resolveDirectoryEntry(entry, dirPath, fileInfo, 0))
115121
}
116122

117123
// Wait for all asynchronous operations (including recursive ones) to complete
118124
await Promise.all(initialPromises)
119125

120126
const fileContents = await Promise.all(
121-
filePaths.map(async (file) => {
127+
fileInfo.map(async ({ originalPath, resolvedPath }) => {
122128
try {
123129
// Check if it's a file (not a directory)
124-
const stats = await fs.stat(file)
130+
const stats = await fs.stat(resolvedPath)
125131
if (stats.isFile()) {
126132
// Filter out cache files and system files that shouldn't be in rules
127-
if (!shouldIncludeRuleFile(file)) {
133+
if (!shouldIncludeRuleFile(resolvedPath)) {
128134
return null
129135
}
130-
const content = await safeReadFile(file)
131-
return { filename: file, content }
136+
const content = await safeReadFile(resolvedPath)
137+
// Use resolvedPath for display to maintain existing behavior
138+
return { filename: resolvedPath, content, sortKey: originalPath }
132139
}
133140
return null
134141
} catch (err) {
@@ -138,7 +145,19 @@ async function readTextFilesFromDirectory(dirPath: string): Promise<Array<{ file
138145
)
139146

140147
// Filter out null values (directories, failed reads, or excluded files)
141-
return fileContents.filter((item): item is { filename: string; content: string } => item !== null)
148+
const filteredFiles = fileContents.filter(
149+
(item): item is { filename: string; content: string; sortKey: string } => item !== null,
150+
)
151+
152+
// Sort files alphabetically by the original filename (case-insensitive) to ensure consistent order
153+
// For symlinks, this will use the symlink name, not the target name
154+
return filteredFiles
155+
.sort((a, b) => {
156+
const filenameA = path.basename(a.sortKey).toLowerCase()
157+
const filenameB = path.basename(b.sortKey).toLowerCase()
158+
return filenameA.localeCompare(filenameB)
159+
})
160+
.map(({ filename, content }) => ({ filename, content }))
142161
} catch (err) {
143162
return []
144163
}

0 commit comments

Comments
 (0)