diff --git a/src/core/prompts/sections/__tests__/custom-instructions.spec.ts b/src/core/prompts/sections/__tests__/custom-instructions.spec.ts index 9c8e0031432..f6fb8ea1f4a 100644 --- a/src/core/prompts/sections/__tests__/custom-instructions.spec.ts +++ b/src/core/prompts/sections/__tests__/custom-instructions.spec.ts @@ -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({ diff --git a/src/core/prompts/sections/custom-instructions.ts b/src/core/prompts/sections/custom-instructions.ts index 3c8558a57f4..71613ff94a1 100644 --- a/src/core/prompts/sections/custom-instructions.ts +++ b/src/core/prompts/sections/custom-instructions.ts @@ -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 { // 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 { +async function resolveSymLink( + symlinkPath: string, + fileInfo: Array<{ originalPath: string; resolvedPath: string }>, + depth: number, +): Promise { // 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[] = [] 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 = [] // Collect promises for the initial resolution calls const initialPromises: Promise[] = [] 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 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 [] }