Skip to content

Fix a bug not to read rule files properly, under nested .roo/rules directories #2405

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 2 commits into from
Apr 8, 2025
Merged

Fix a bug not to read rule files properly, under nested .roo/rules directories #2405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8ef297
fix .roo/rules/subdir/file path calculation
taisukeoe Apr 8, 2025
e9c7bed
add changeset
taisukeoe Apr 8, 2025
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
5 changes: 5 additions & 0 deletions .changeset/poor-dolphins-brush.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"roo-cline": patch
---

Fix bug where nested .roo/rules directories are not respected properly
92 changes: 84 additions & 8 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 @@ -127,8 +127,8 @@ describe("loadRuleFiles", () => {

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

statMock.mockImplementation(
Expand Down Expand Up @@ -201,6 +201,80 @@ describe("loadRuleFiles", () => {
const result = await loadRuleFiles("/fake/path")
expect(result).toBe("\n# Rules from .roorules:\nroo rules content\n")
})

it("should read files from nested subdirectories in .roo/rules/", async () => {
// Simulate .roo/rules directory exists
statMock.mockResolvedValueOnce({
isDirectory: jest.fn().mockReturnValue(true),
} as any)

// 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: "nested1.txt",
isFile: () => true,
isDirectory: () => false,
parentPath: "/fake/path/.roo/rules/subdir",
},
{
name: "nested2.txt",
isFile: () => true,
isDirectory: () => false,
parentPath: "/fake/path/.roo/rules/subdir/subdir2",
},
] as any)

statMock.mockImplementation((path: string) => {
if (path.endsWith("txt")) {
return Promise.resolve({
isFile: jest.fn().mockReturnValue(true),
isDirectory: jest.fn().mockReturnValue(false),
} as any)
}
return Promise.resolve({
isFile: jest.fn().mockReturnValue(false),
isDirectory: jest.fn().mockReturnValue(true),
} as any)
})

readFileMock.mockImplementation((filePath: PathLike) => {
const path = filePath.toString()
if (path === "/fake/path/.roo/rules/root.txt") {
return Promise.resolve("root file content")
}
if (path === "/fake/path/.roo/rules/subdir/nested1.txt") {
return Promise.resolve("nested file 1 content")
}
if (path === "/fake/path/.roo/rules/subdir/subdir2/nested2.txt") {
return Promise.resolve("nested file 2 content")
}
return Promise.reject({ code: "ENOENT" })
})

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

// Check root file content
expect(result).toContain("# Rules from /fake/path/.roo/rules/root.txt:")
expect(result).toContain("root file content")

// Check nested files content
expect(result).toContain("# Rules from /fake/path/.roo/rules/subdir/nested1.txt:")
expect(result).toContain("nested file 1 content")
expect(result).toContain("# Rules from /fake/path/.roo/rules/subdir/subdir2/nested2.txt:")
expect(result).toContain("nested file 2 content")

// Verify correct paths were checked
expect(statMock).toHaveBeenCalledWith("/fake/path/.roo/rules/root.txt")
expect(statMock).toHaveBeenCalledWith("/fake/path/.roo/rules/subdir/nested1.txt")
expect(statMock).toHaveBeenCalledWith("/fake/path/.roo/rules/subdir/subdir2/nested2.txt")

// Verify files were read with correct paths
expect(readFileMock).toHaveBeenCalledWith("/fake/path/.roo/rules/root.txt", "utf-8")
expect(readFileMock).toHaveBeenCalledWith("/fake/path/.roo/rules/subdir/nested1.txt", "utf-8")
expect(readFileMock).toHaveBeenCalledWith("/fake/path/.roo/rules/subdir/subdir2/nested2.txt", "utf-8")
})
})

describe("addCustomInstructions", () => {
Expand Down Expand Up @@ -321,8 +395,8 @@ describe("addCustomInstructions", () => {

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

statMock.mockImplementation(
Expand Down Expand Up @@ -422,7 +496,9 @@ describe("addCustomInstructions", () => {
)

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

// Set up stat mock for checking files
Expand Down Expand Up @@ -515,9 +591,9 @@ describe("Rules directory reading", () => {

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

statMock.mockImplementation((path) => {
Expand Down
2 changes: 1 addition & 1 deletion src/core/prompts/sections/custom-instructions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ async function readTextFilesFromDirectory(dirPath: string): Promise<Array<{ file
const files = await fs
.readdir(dirPath, { withFileTypes: true, recursive: true })
.then((files) => files.filter((file) => file.isFile()))
.then((files) => files.map((file) => path.resolve(dirPath, file.name)))
.then((files) => files.map((file) => path.resolve(file.parentPath, file.name)))
Copy link
Contributor

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

Choose a reason for hiding this comment

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

Using file.parentPath assumes that the Dirent includes a parentPath property, which is non-standard. Consider defaulting to the original dirPath if parentPath is undefined.

Suggested change
.then((files) => files.map((file) => path.resolve(file.parentPath, file.name)))
.then((files) => files.map((file) => path.resolve(file.parentPath || dirPath, file.name)))

Copy link
Contributor Author

@taisukeoe taisukeoe Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed Dirent#parentPath is marked as Stability:1.

https://nodejs.org/docs/latest-v20.x/api/fs.html#direntparentpath

Stability: 1 - Experimental. The feature is not subject to semantic versioning rules. Non-backward compatible changes or removal may occur in any future release. Use of the feature is not recommended in production environments.

Considering Dirent#path was the original function name but deprecated now, the new function name parentPath seems to be still the first option to use.
It might be arguable that we may want to use just parentPath, or prepare some fail-overs.

  • stay file.parentPath
  • file.parentPath || dirPath
  • file.parentPath || file.path
  • or calculate fullpath in a recursive concatenation manner (subdir/ + subdir2/ + file1.txt)

Which pattern do you prefer?

Copy link
Collaborator

@mrubens mrubens Apr 8, 2025

Choose a reason for hiding this comment

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

Oh nice, I'm not sure I have any strong opinions. What do you think is best? cc: @upanume who wrote this originally.

Copy link
Contributor

@upamune upamune Apr 8, 2025

Choose a reason for hiding this comment

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

I implemented this! Thank you for the bug fix 😭

IMO: While there are certainly points of discussion, I think either "stay file.parentPath" or "file.parentPath || dirPath" would be good. If the behavior changes due to Node.js modifications, the tests would fail, so we would be able to notice, and I don't think we need to make things unnecessarily complicated.

Copy link
Collaborator

@mrubens mrubens Apr 8, 2025

Choose a reason for hiding this comment

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

Sounds good to me!


const fileContents = await Promise.all(
files.map(async (file) => {
Expand Down
Loading