Skip to content

Commit 100462a

Browse files
committed
fix: improve error handling in roo-config service to only catch expected filesystem errors
- Replace broad catch-all error handling with specific error code checking - Only catch expected errors (ENOENT, ENOTDIR, EISDIR) in filesystem operations - Allow unexpected errors (EACCES, EIO, etc.) to propagate for proper diagnosis - Update directoryExists(), fileExists(), and readFileIfExists() functions - Add comprehensive test coverage for both expected and unexpected error scenarios - Verify loadConfiguration() properly propagates errors from file operations This prevents masking of critical system errors like permission issues, I/O failures, and disk problems while maintaining graceful handling of expected "file not found" cases. Improves debugging capabilities and follows error handling best practices. Fixes potential issues where permission denied or I/O errors were silently treated as "file doesn't exist", making troubleshooting difficult in production environments.
1 parent 0dbf807 commit 100462a

File tree

2 files changed

+122
-12
lines changed

2 files changed

+122
-12
lines changed

src/services/roo-config/__tests__/index.spec.ts

Lines changed: 101 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -73,13 +73,33 @@ describe("RooConfigService", () => {
7373
})
7474

7575
it("should return false for non-existing path", async () => {
76-
mockStat.mockRejectedValue(new Error("ENOENT"))
76+
const error = new Error("ENOENT") as any
77+
error.code = "ENOENT"
78+
mockStat.mockRejectedValue(error)
7779

7880
const result = await directoryExists("/non/existing/path")
7981

8082
expect(result).toBe(false)
8183
})
8284

85+
it("should return false for ENOTDIR error", async () => {
86+
const error = new Error("ENOTDIR") as any
87+
error.code = "ENOTDIR"
88+
mockStat.mockRejectedValue(error)
89+
90+
const result = await directoryExists("/not/a/directory")
91+
92+
expect(result).toBe(false)
93+
})
94+
95+
it("should throw unexpected errors", async () => {
96+
const error = new Error("Permission denied") as any
97+
error.code = "EACCES"
98+
mockStat.mockRejectedValue(error)
99+
100+
await expect(directoryExists("/permission/denied")).rejects.toThrow("Permission denied")
101+
})
102+
83103
it("should return false for files", async () => {
84104
mockStat.mockResolvedValue({ isDirectory: () => false } as any)
85105

@@ -100,13 +120,33 @@ describe("RooConfigService", () => {
100120
})
101121

102122
it("should return false for non-existing file", async () => {
103-
mockStat.mockRejectedValue(new Error("ENOENT"))
123+
const error = new Error("ENOENT") as any
124+
error.code = "ENOENT"
125+
mockStat.mockRejectedValue(error)
104126

105127
const result = await fileExists("/non/existing/file.txt")
106128

107129
expect(result).toBe(false)
108130
})
109131

132+
it("should return false for ENOTDIR error", async () => {
133+
const error = new Error("ENOTDIR") as any
134+
error.code = "ENOTDIR"
135+
mockStat.mockRejectedValue(error)
136+
137+
const result = await fileExists("/not/a/directory/file.txt")
138+
139+
expect(result).toBe(false)
140+
})
141+
142+
it("should throw unexpected errors", async () => {
143+
const error = new Error("Permission denied") as any
144+
error.code = "EACCES"
145+
mockStat.mockRejectedValue(error)
146+
147+
await expect(fileExists("/permission/denied/file.txt")).rejects.toThrow("Permission denied")
148+
})
149+
110150
it("should return false for directories", async () => {
111151
mockStat.mockResolvedValue({ isFile: () => false } as any)
112152

@@ -127,12 +167,42 @@ describe("RooConfigService", () => {
127167
})
128168

129169
it("should return null for non-existing file", async () => {
130-
mockReadFile.mockRejectedValue(new Error("ENOENT"))
170+
const error = new Error("ENOENT") as any
171+
error.code = "ENOENT"
172+
mockReadFile.mockRejectedValue(error)
131173

132174
const result = await readFileIfExists("/non/existing/file.txt")
133175

134176
expect(result).toBe(null)
135177
})
178+
179+
it("should return null for ENOTDIR error", async () => {
180+
const error = new Error("ENOTDIR") as any
181+
error.code = "ENOTDIR"
182+
mockReadFile.mockRejectedValue(error)
183+
184+
const result = await readFileIfExists("/not/a/directory/file.txt")
185+
186+
expect(result).toBe(null)
187+
})
188+
189+
it("should return null for EISDIR error", async () => {
190+
const error = new Error("EISDIR") as any
191+
error.code = "EISDIR"
192+
mockReadFile.mockRejectedValue(error)
193+
194+
const result = await readFileIfExists("/is/a/directory")
195+
196+
expect(result).toBe(null)
197+
})
198+
199+
it("should throw unexpected errors", async () => {
200+
const error = new Error("Permission denied") as any
201+
error.code = "EACCES"
202+
mockReadFile.mockRejectedValue(error)
203+
204+
await expect(readFileIfExists("/permission/denied/file.txt")).rejects.toThrow("Permission denied")
205+
})
136206
})
137207

138208
describe("getRooDirectoriesForCwd", () => {
@@ -147,7 +217,9 @@ describe("RooConfigService", () => {
147217

148218
describe("loadConfiguration", () => {
149219
it("should load global configuration only when project does not exist", async () => {
150-
mockReadFile.mockResolvedValueOnce("global content").mockRejectedValueOnce(new Error("ENOENT"))
220+
const error = new Error("ENOENT") as any
221+
error.code = "ENOENT"
222+
mockReadFile.mockResolvedValueOnce("global content").mockRejectedValueOnce(error)
151223

152224
const result = await loadConfiguration("rules/rules.md", "/project/path")
153225

@@ -159,7 +231,9 @@ describe("RooConfigService", () => {
159231
})
160232

161233
it("should load project configuration only when global does not exist", async () => {
162-
mockReadFile.mockRejectedValueOnce(new Error("ENOENT")).mockResolvedValueOnce("project content")
234+
const error = new Error("ENOENT") as any
235+
error.code = "ENOENT"
236+
mockReadFile.mockRejectedValueOnce(error).mockResolvedValueOnce("project content")
163237

164238
const result = await loadConfiguration("rules/rules.md", "/project/path")
165239

@@ -183,7 +257,9 @@ describe("RooConfigService", () => {
183257
})
184258

185259
it("should return empty merged content when neither exists", async () => {
186-
mockReadFile.mockRejectedValueOnce(new Error("ENOENT")).mockRejectedValueOnce(new Error("ENOENT"))
260+
const error = new Error("ENOENT") as any
261+
error.code = "ENOENT"
262+
mockReadFile.mockRejectedValueOnce(error).mockRejectedValueOnce(error)
187263

188264
const result = await loadConfiguration("rules/rules.md", "/project/path")
189265

@@ -194,6 +270,25 @@ describe("RooConfigService", () => {
194270
})
195271
})
196272

273+
it("should propagate unexpected errors from global file read", async () => {
274+
const error = new Error("Permission denied") as any
275+
error.code = "EACCES"
276+
mockReadFile.mockRejectedValueOnce(error)
277+
278+
await expect(loadConfiguration("rules/rules.md", "/project/path")).rejects.toThrow("Permission denied")
279+
})
280+
281+
it("should propagate unexpected errors from project file read", async () => {
282+
const globalError = new Error("ENOENT") as any
283+
globalError.code = "ENOENT"
284+
const projectError = new Error("Permission denied") as any
285+
projectError.code = "EACCES"
286+
287+
mockReadFile.mockRejectedValueOnce(globalError).mockRejectedValueOnce(projectError)
288+
289+
await expect(loadConfiguration("rules/rules.md", "/project/path")).rejects.toThrow("Permission denied")
290+
})
291+
197292
it("should use correct file paths", async () => {
198293
mockReadFile.mockResolvedValue("content")
199294

src/services/roo-config/index.ts

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,13 @@ export async function directoryExists(dirPath: string): Promise<boolean> {
2626
try {
2727
const stat = await fs.stat(dirPath)
2828
return stat.isDirectory()
29-
} catch {
30-
return false
29+
} catch (error: any) {
30+
// Only catch expected "not found" errors
31+
if (error.code === "ENOENT" || error.code === "ENOTDIR") {
32+
return false
33+
}
34+
// Re-throw unexpected errors (permission, I/O, etc.)
35+
throw error
3136
}
3237
}
3338

@@ -38,8 +43,13 @@ export async function fileExists(filePath: string): Promise<boolean> {
3843
try {
3944
const stat = await fs.stat(filePath)
4045
return stat.isFile()
41-
} catch {
42-
return false
46+
} catch (error: any) {
47+
// Only catch expected "not found" errors
48+
if (error.code === "ENOENT" || error.code === "ENOTDIR") {
49+
return false
50+
}
51+
// Re-throw unexpected errors (permission, I/O, etc.)
52+
throw error
4353
}
4454
}
4555

@@ -49,8 +59,13 @@ export async function fileExists(filePath: string): Promise<boolean> {
4959
export async function readFileIfExists(filePath: string): Promise<string | null> {
5060
try {
5161
return await fs.readFile(filePath, "utf-8")
52-
} catch {
53-
return null
62+
} catch (error: any) {
63+
// Only catch expected "not found" errors
64+
if (error.code === "ENOENT" || error.code === "ENOTDIR" || error.code === "EISDIR") {
65+
return null
66+
}
67+
// Re-throw unexpected errors (permission, I/O, etc.)
68+
throw error
5469
}
5570
}
5671

0 commit comments

Comments
 (0)