mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-10 22:41:14 +00:00
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 <roomote@roocode.com>
This commit is contained in:
roomote[bot]
GitHub
parent
2eb586b422
commit
0500894b34
2 changed files with 188 additions and 18 deletions
|
|
@ -1033,6 +1033,157 @@ describe("Rules directory reading", () => {
|
|||
expect(result).toContain("content of file3")
|
||||
})
|
||||
|
||||
it("should return files in alphabetical order by filename", async () => {
|
||||
// Simulate .roo/rules directory exists
|
||||
statMock.mockResolvedValueOnce({
|
||||
isDirectory: vi.fn().mockReturnValue(true),
|
||||
} as any)
|
||||
|
||||
// Simulate listing files in non-alphabetical order to test sorting
|
||||
readdirMock.mockResolvedValueOnce([
|
||||
{ name: "zebra.txt", isFile: () => true, parentPath: "/fake/path/.roo/rules" },
|
||||
{ name: "alpha.txt", isFile: () => true, parentPath: "/fake/path/.roo/rules" },
|
||||
{ name: "Beta.txt", isFile: () => true, parentPath: "/fake/path/.roo/rules" }, // Test case-insensitive sorting
|
||||
] as any)
|
||||
|
||||
statMock.mockImplementation((path) => {
|
||||
return Promise.resolve({
|
||||
isFile: vi.fn().mockReturnValue(true),
|
||||
}) as any
|
||||
})
|
||||
|
||||
readFileMock.mockImplementation((filePath: PathLike) => {
|
||||
const pathStr = filePath.toString()
|
||||
const normalizedPath = pathStr.replace(/\\/g, "/")
|
||||
if (normalizedPath === "/fake/path/.roo/rules/zebra.txt") {
|
||||
return Promise.resolve("zebra content")
|
||||
}
|
||||
if (normalizedPath === "/fake/path/.roo/rules/alpha.txt") {
|
||||
return Promise.resolve("alpha content")
|
||||
}
|
||||
if (normalizedPath === "/fake/path/.roo/rules/Beta.txt") {
|
||||
return Promise.resolve("beta content")
|
||||
}
|
||||
return Promise.reject({ code: "ENOENT" })
|
||||
})
|
||||
|
||||
const result = await loadRuleFiles("/fake/path")
|
||||
|
||||
// Files should appear in alphabetical order: alpha.txt, Beta.txt, zebra.txt
|
||||
const alphaIndex = result.indexOf("alpha content")
|
||||
const betaIndex = result.indexOf("beta content")
|
||||
const zebraIndex = result.indexOf("zebra content")
|
||||
|
||||
expect(alphaIndex).toBeLessThan(betaIndex)
|
||||
expect(betaIndex).toBeLessThan(zebraIndex)
|
||||
|
||||
// Verify the expected file paths are in the result
|
||||
const expectedAlphaPath =
|
||||
process.platform === "win32" ? "\\fake\\path\\.roo\\rules\\alpha.txt" : "/fake/path/.roo/rules/alpha.txt"
|
||||
const expectedBetaPath =
|
||||
process.platform === "win32" ? "\\fake\\path\\.roo\\rules\\Beta.txt" : "/fake/path/.roo/rules/Beta.txt"
|
||||
const expectedZebraPath =
|
||||
process.platform === "win32" ? "\\fake\\path\\.roo\\rules\\zebra.txt" : "/fake/path/.roo/rules/zebra.txt"
|
||||
|
||||
expect(result).toContain(`# Rules from ${expectedAlphaPath}:`)
|
||||
expect(result).toContain(`# Rules from ${expectedBetaPath}:`)
|
||||
expect(result).toContain(`# Rules from ${expectedZebraPath}:`)
|
||||
})
|
||||
|
||||
it("should sort symlinks by their symlink names, not target names", async () => {
|
||||
// Reset mocks
|
||||
statMock.mockReset()
|
||||
readdirMock.mockReset()
|
||||
readlinkMock.mockReset()
|
||||
readFileMock.mockReset()
|
||||
|
||||
// First call: check if .roo/rules directory exists
|
||||
statMock.mockResolvedValueOnce({
|
||||
isDirectory: vi.fn().mockReturnValue(true),
|
||||
} as any)
|
||||
|
||||
// Simulate listing files with symlinks that point to files with different names
|
||||
readdirMock.mockResolvedValueOnce([
|
||||
{
|
||||
name: "01-first.link",
|
||||
isFile: () => false,
|
||||
isSymbolicLink: () => true,
|
||||
parentPath: "/fake/path/.roo/rules",
|
||||
},
|
||||
{
|
||||
name: "02-second.link",
|
||||
isFile: () => false,
|
||||
isSymbolicLink: () => true,
|
||||
parentPath: "/fake/path/.roo/rules",
|
||||
},
|
||||
{
|
||||
name: "03-third.link",
|
||||
isFile: () => false,
|
||||
isSymbolicLink: () => true,
|
||||
parentPath: "/fake/path/.roo/rules",
|
||||
},
|
||||
] as any)
|
||||
|
||||
// Mock readlink to return target paths that would sort differently than symlink names
|
||||
readlinkMock
|
||||
.mockResolvedValueOnce("../../targets/zzz-last.txt") // 01-first.link -> zzz-last.txt
|
||||
.mockResolvedValueOnce("../../targets/aaa-first.txt") // 02-second.link -> aaa-first.txt
|
||||
.mockResolvedValueOnce("../../targets/mmm-middle.txt") // 03-third.link -> mmm-middle.txt
|
||||
|
||||
// Set up stat mock for the remaining calls
|
||||
statMock.mockImplementation((path) => {
|
||||
const normalizedPath = path.toString().replace(/\\/g, "/")
|
||||
// Target files exist and are files
|
||||
if (normalizedPath.endsWith(".txt")) {
|
||||
return Promise.resolve({
|
||||
isFile: vi.fn().mockReturnValue(true),
|
||||
isDirectory: vi.fn().mockReturnValue(false),
|
||||
} as any)
|
||||
}
|
||||
return Promise.resolve({
|
||||
isFile: vi.fn().mockReturnValue(false),
|
||||
isDirectory: vi.fn().mockReturnValue(false),
|
||||
} as any)
|
||||
})
|
||||
|
||||
readFileMock.mockImplementation((filePath: PathLike) => {
|
||||
const pathStr = filePath.toString()
|
||||
const normalizedPath = pathStr.replace(/\\/g, "/")
|
||||
if (normalizedPath.endsWith("zzz-last.txt")) {
|
||||
return Promise.resolve("content from zzz-last.txt")
|
||||
}
|
||||
if (normalizedPath.endsWith("aaa-first.txt")) {
|
||||
return Promise.resolve("content from aaa-first.txt")
|
||||
}
|
||||
if (normalizedPath.endsWith("mmm-middle.txt")) {
|
||||
return Promise.resolve("content from mmm-middle.txt")
|
||||
}
|
||||
return Promise.reject({ code: "ENOENT" })
|
||||
})
|
||||
|
||||
const result = await loadRuleFiles("/fake/path")
|
||||
|
||||
// Content should appear in order of symlink names (01-first, 02-second, 03-third)
|
||||
// NOT in order of target names (aaa-first, mmm-middle, zzz-last)
|
||||
const firstIndex = result.indexOf("content from zzz-last.txt") // from 01-first.link
|
||||
const secondIndex = result.indexOf("content from aaa-first.txt") // from 02-second.link
|
||||
const thirdIndex = result.indexOf("content from mmm-middle.txt") // from 03-third.link
|
||||
|
||||
// All content should be found
|
||||
expect(firstIndex).toBeGreaterThan(-1)
|
||||
expect(secondIndex).toBeGreaterThan(-1)
|
||||
expect(thirdIndex).toBeGreaterThan(-1)
|
||||
|
||||
// And they should be in the order of symlink names, not target names
|
||||
expect(firstIndex).toBeLessThan(secondIndex)
|
||||
expect(secondIndex).toBeLessThan(thirdIndex)
|
||||
|
||||
// Verify the target paths are shown (not symlink paths)
|
||||
expect(result).toContain("zzz-last.txt")
|
||||
expect(result).toContain("aaa-first.txt")
|
||||
expect(result).toContain("mmm-middle.txt")
|
||||
})
|
||||
|
||||
it("should handle empty file list gracefully", async () => {
|
||||
// Simulate .roo/rules directory exists
|
||||
statMock.mockResolvedValueOnce({
|
||||
|
|
|
|||
|
|
@ -44,7 +44,7 @@ const MAX_DEPTH = 5
|
|||
async function resolveDirectoryEntry(
|
||||
entry: Dirent,
|
||||
dirPath: string,
|
||||
filePaths: string[],
|
||||
fileInfo: Array<{ originalPath: string; resolvedPath: string }>,
|
||||
depth: number,
|
||||
): Promise<void> {
|
||||
// Avoid cyclic symlinks
|
||||
|
|
@ -54,44 +54,49 @@ async function resolveDirectoryEntry(
|
|||
|
||||
const fullPath = path.resolve(entry.parentPath || dirPath, entry.name)
|
||||
if (entry.isFile()) {
|
||||
// Regular file
|
||||
filePaths.push(fullPath)
|
||||
// Regular file - both original and resolved paths are the same
|
||||
fileInfo.push({ originalPath: fullPath, resolvedPath: fullPath })
|
||||
} else if (entry.isSymbolicLink()) {
|
||||
// Await the resolution of the symbolic link
|
||||
await resolveSymLink(fullPath, filePaths, depth + 1)
|
||||
await resolveSymLink(fullPath, fileInfo, depth + 1)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Recursively resolve a symbolic link and collect file paths
|
||||
*/
|
||||
async function resolveSymLink(fullPath: string, filePaths: string[], depth: number): Promise<void> {
|
||||
async function resolveSymLink(
|
||||
symlinkPath: string,
|
||||
fileInfo: Array<{ originalPath: string; resolvedPath: string }>,
|
||||
depth: number,
|
||||
): Promise<void> {
|
||||
// Avoid cyclic symlinks
|
||||
if (depth > MAX_DEPTH) {
|
||||
return
|
||||
}
|
||||
try {
|
||||
// Get the symlink target
|
||||
const linkTarget = await fs.readlink(fullPath)
|
||||
const linkTarget = await fs.readlink(symlinkPath)
|
||||
// Resolve the target path (relative to the symlink location)
|
||||
const resolvedTarget = path.resolve(path.dirname(fullPath), linkTarget)
|
||||
const resolvedTarget = path.resolve(path.dirname(symlinkPath), linkTarget)
|
||||
|
||||
// Check if the target is a file
|
||||
const stats = await fs.stat(resolvedTarget)
|
||||
if (stats.isFile()) {
|
||||
filePaths.push(resolvedTarget)
|
||||
// For symlinks to files, store the symlink path as original and target as resolved
|
||||
fileInfo.push({ originalPath: symlinkPath, resolvedPath: resolvedTarget })
|
||||
} else if (stats.isDirectory()) {
|
||||
const anotherEntries = await fs.readdir(resolvedTarget, { withFileTypes: true, recursive: true })
|
||||
// Collect promises for recursive calls within the directory
|
||||
const directoryPromises: Promise<void>[] = []
|
||||
for (const anotherEntry of anotherEntries) {
|
||||
directoryPromises.push(resolveDirectoryEntry(anotherEntry, resolvedTarget, filePaths, depth + 1))
|
||||
directoryPromises.push(resolveDirectoryEntry(anotherEntry, resolvedTarget, fileInfo, depth + 1))
|
||||
}
|
||||
// Wait for all entries in the resolved directory to be processed
|
||||
await Promise.all(directoryPromises)
|
||||
} else if (stats.isSymbolicLink()) {
|
||||
// Handle nested symlinks by awaiting the recursive call
|
||||
await resolveSymLink(resolvedTarget, filePaths, depth + 1)
|
||||
await resolveSymLink(resolvedTarget, fileInfo, depth + 1)
|
||||
}
|
||||
} catch (err) {
|
||||
// Skip invalid symlinks
|
||||
|
|
@ -106,29 +111,31 @@ async function readTextFilesFromDirectory(dirPath: string): Promise<Array<{ file
|
|||
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[] = []
|
||||
// Store both original path (for sorting) and resolved path (for reading)
|
||||
const fileInfo: Array<{ originalPath: string; resolvedPath: string }> = []
|
||||
// Collect promises for the initial resolution calls
|
||||
const initialPromises: Promise<void>[] = []
|
||||
|
||||
for (const entry of entries) {
|
||||
initialPromises.push(resolveDirectoryEntry(entry, dirPath, filePaths, 0))
|
||||
initialPromises.push(resolveDirectoryEntry(entry, dirPath, fileInfo, 0))
|
||||
}
|
||||
|
||||
// Wait for all asynchronous operations (including recursive ones) to complete
|
||||
await Promise.all(initialPromises)
|
||||
|
||||
const fileContents = await Promise.all(
|
||||
filePaths.map(async (file) => {
|
||||
fileInfo.map(async ({ originalPath, resolvedPath }) => {
|
||||
try {
|
||||
// Check if it's a file (not a directory)
|
||||
const stats = await fs.stat(file)
|
||||
const stats = await fs.stat(resolvedPath)
|
||||
if (stats.isFile()) {
|
||||
// Filter out cache files and system files that shouldn't be in rules
|
||||
if (!shouldIncludeRuleFile(file)) {
|
||||
if (!shouldIncludeRuleFile(resolvedPath)) {
|
||||
return null
|
||||
}
|
||||
const content = await safeReadFile(file)
|
||||
return { filename: file, content }
|
||||
const content = await safeReadFile(resolvedPath)
|
||||
// Use resolvedPath for display to maintain existing behavior
|
||||
return { filename: resolvedPath, content, sortKey: originalPath }
|
||||
}
|
||||
return null
|
||||
} catch (err) {
|
||||
|
|
@ -138,7 +145,19 @@ async function readTextFilesFromDirectory(dirPath: string): Promise<Array<{ file
|
|||
)
|
||||
|
||||
// Filter out null values (directories, failed reads, or excluded files)
|
||||
return fileContents.filter((item): item is { filename: string; content: string } => item !== null)
|
||||
const filteredFiles = fileContents.filter(
|
||||
(item): item is { filename: string; content: string; sortKey: string } => item !== null,
|
||||
)
|
||||
|
||||
// Sort files alphabetically by the original filename (case-insensitive) to ensure consistent order
|
||||
// For symlinks, this will use the symlink name, not the target name
|
||||
return filteredFiles
|
||||
.sort((a, b) => {
|
||||
const filenameA = path.basename(a.sortKey).toLowerCase()
|
||||
const filenameB = path.basename(b.sortKey).toLowerCase()
|
||||
return filenameA.localeCompare(filenameB)
|
||||
})
|
||||
.map(({ filename, content }) => ({ filename, content }))
|
||||
} catch (err) {
|
||||
return []
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue