mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: add file size validation to prevent memory exhaustion
- Add MAX_FILE_SIZE_BYTES constants for different file types - Add size validation to readFileIfExists() in roo-config service - Add size validation to .gitignore reading in list-files service - Add size validation to checkpoint file reading in ShadowCheckpointService - Add size validation to MCP config reading in McpHub - Add comprehensive tests for file size validation - Add FileSizeLimitError class for better error handling This prevents memory exhaustion and performance issues when reading large files across various Roo-Code services. Fixes #6155
This commit is contained in:
parent
1e17b3b3bb
commit
40a52aecee
7 changed files with 404 additions and 48 deletions
|
|
@ -12,6 +12,7 @@ import { executeRipgrep } from "../../services/search/file-search"
|
|||
|
||||
import { CheckpointDiff, CheckpointResult, CheckpointEventMap } from "./types"
|
||||
import { getExcludePatterns } from "./excludes"
|
||||
import { MAX_CHECKPOINT_FILE_SIZE_BYTES } from "../constants/file-limits"
|
||||
|
||||
export abstract class ShadowCheckpointService extends EventEmitter {
|
||||
public readonly taskId: string
|
||||
|
|
@ -292,9 +293,25 @@ export abstract class ShadowCheckpointService extends EventEmitter {
|
|||
const absPath = path.join(cwdPath, relPath)
|
||||
const before = await this.git.show([`${from}:${relPath}`]).catch(() => "")
|
||||
|
||||
const after = to
|
||||
? await this.git.show([`${to}:${relPath}`]).catch(() => "")
|
||||
: await fs.readFile(absPath, "utf8").catch(() => "")
|
||||
let after = ""
|
||||
if (to) {
|
||||
after = await this.git.show([`${to}:${relPath}`]).catch(() => "")
|
||||
} else {
|
||||
// Check file size before reading from filesystem
|
||||
try {
|
||||
const stats = await fs.stat(absPath)
|
||||
if (stats.size > MAX_CHECKPOINT_FILE_SIZE_BYTES) {
|
||||
console.warn(
|
||||
`Checkpoint file ${absPath} exceeds size limit (${stats.size} bytes > ${MAX_CHECKPOINT_FILE_SIZE_BYTES} bytes)`,
|
||||
)
|
||||
after = `[File too large to display: ${stats.size} bytes]`
|
||||
} else {
|
||||
after = await fs.readFile(absPath, "utf8")
|
||||
}
|
||||
} catch (err) {
|
||||
after = ""
|
||||
}
|
||||
}
|
||||
|
||||
result.push({ paths: { relative: relPath, absolute: absPath }, content: { before, after } })
|
||||
}
|
||||
|
|
|
|||
28
src/services/constants/file-limits.ts
Normal file
28
src/services/constants/file-limits.ts
Normal file
|
|
@ -0,0 +1,28 @@
|
|||
/**
|
||||
* File size limits for various file types across Roo-Code services
|
||||
* These constants help prevent memory exhaustion and performance issues
|
||||
*/
|
||||
|
||||
// General file size limit (1MB) - used for code files and general file operations
|
||||
export const MAX_FILE_SIZE_BYTES = 1 * 1024 * 1024 // 1MB
|
||||
|
||||
// Configuration files should be smaller (100KB)
|
||||
export const MAX_CONFIG_FILE_SIZE_BYTES = 100 * 1024 // 100KB
|
||||
|
||||
// .gitignore files should be even smaller (50KB)
|
||||
export const MAX_GITIGNORE_FILE_SIZE_BYTES = 50 * 1024 // 50KB
|
||||
|
||||
// Checkpoint files can be larger but still need a reasonable limit (5MB)
|
||||
export const MAX_CHECKPOINT_FILE_SIZE_BYTES = 5 * 1024 * 1024 // 5MB
|
||||
|
||||
/**
|
||||
* Error class for file size limit violations
|
||||
*/
|
||||
export class FileSizeLimitError extends Error {
|
||||
constructor(filePath: string, size: number, limit: number) {
|
||||
const sizeInKB = Math.round(size / 1024)
|
||||
const limitInKB = Math.round(limit / 1024)
|
||||
super(`File ${filePath} exceeds size limit (${sizeInKB}KB > ${limitInKB}KB)`)
|
||||
this.name = "FileSizeLimitError"
|
||||
}
|
||||
}
|
||||
175
src/services/glob/__tests__/list-files-gitignore-size.spec.ts
Normal file
175
src/services/glob/__tests__/list-files-gitignore-size.spec.ts
Normal file
|
|
@ -0,0 +1,175 @@
|
|||
import { vi, describe, it, expect, beforeEach, afterEach } from "vitest"
|
||||
import * as path from "path"
|
||||
import { MAX_GITIGNORE_FILE_SIZE_BYTES } from "../../constants/file-limits"
|
||||
|
||||
// Use vi.hoisted to ensure mocks are available during hoisting
|
||||
const { mockConsoleWarn, mockStat, mockReadFile, mockAccess } = vi.hoisted(() => ({
|
||||
mockConsoleWarn: vi.fn(),
|
||||
mockStat: vi.fn(),
|
||||
mockReadFile: vi.fn(),
|
||||
mockAccess: vi.fn(),
|
||||
}))
|
||||
|
||||
vi.mock("fs", () => ({
|
||||
promises: {
|
||||
stat: mockStat,
|
||||
readFile: mockReadFile,
|
||||
access: mockAccess,
|
||||
readdir: vi.fn().mockResolvedValue([]),
|
||||
},
|
||||
}))
|
||||
|
||||
// Mock other dependencies
|
||||
vi.mock("../../ripgrep", () => ({
|
||||
getBinPath: vi.fn(() => Promise.resolve("/mock/path/to/rg")),
|
||||
}))
|
||||
|
||||
vi.mock("vscode", () => ({
|
||||
env: {
|
||||
appRoot: "/mock/app/root",
|
||||
},
|
||||
}))
|
||||
|
||||
vi.mock("child_process", () => ({
|
||||
spawn: vi.fn(() => {
|
||||
const mockProcess = {
|
||||
stdout: {
|
||||
on: vi.fn((event, callback) => {
|
||||
if (event === "data") {
|
||||
setTimeout(() => callback("test-file.txt\n"), 10)
|
||||
}
|
||||
}),
|
||||
},
|
||||
stderr: { on: vi.fn() },
|
||||
on: vi.fn((event, callback) => {
|
||||
if (event === "close") {
|
||||
setTimeout(() => callback(0), 20)
|
||||
}
|
||||
}),
|
||||
kill: vi.fn(),
|
||||
}
|
||||
return mockProcess
|
||||
}),
|
||||
}))
|
||||
|
||||
vi.mock("../../path", () => ({
|
||||
arePathsEqual: vi.fn().mockReturnValue(false),
|
||||
}))
|
||||
|
||||
// Import the function to test after mocks are set up
|
||||
import { listFiles } from "../list-files"
|
||||
|
||||
describe("list-files .gitignore size validation", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
console.warn = mockConsoleWarn
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
it("should skip .gitignore files that exceed size limit", async () => {
|
||||
// Mock finding a .gitignore file
|
||||
mockAccess.mockResolvedValue(undefined) // File exists
|
||||
|
||||
// Mock .gitignore file that exceeds size limit
|
||||
const largeSize = MAX_GITIGNORE_FILE_SIZE_BYTES + 1000
|
||||
mockStat.mockResolvedValue({ size: largeSize })
|
||||
|
||||
// Call listFiles
|
||||
await listFiles("/test/dir", false, 100)
|
||||
|
||||
// Verify that console.warn was called with the appropriate message
|
||||
expect(mockConsoleWarn).toHaveBeenCalledWith(expect.stringContaining("Skipping large .gitignore file"))
|
||||
expect(mockConsoleWarn).toHaveBeenCalledWith(
|
||||
expect.stringContaining(`${largeSize} bytes > ${MAX_GITIGNORE_FILE_SIZE_BYTES} bytes`),
|
||||
)
|
||||
|
||||
// Verify that readFile was NOT called for the large .gitignore
|
||||
expect(mockReadFile).not.toHaveBeenCalledWith(expect.stringContaining(".gitignore"), "utf8")
|
||||
})
|
||||
|
||||
it("should read .gitignore files within size limit", async () => {
|
||||
// Mock finding a .gitignore file
|
||||
mockAccess.mockResolvedValue(undefined) // File exists
|
||||
|
||||
// Mock .gitignore file within size limit
|
||||
const normalSize = 1024 // 1KB, well under limit
|
||||
mockStat.mockResolvedValue({ size: normalSize })
|
||||
mockReadFile.mockResolvedValue("*.log\nnode_modules/\n")
|
||||
|
||||
// Call listFiles
|
||||
await listFiles("/test/dir", false, 100)
|
||||
|
||||
// Verify that readFile was called for the .gitignore
|
||||
expect(mockReadFile).toHaveBeenCalledWith(expect.stringContaining(".gitignore"), "utf8")
|
||||
|
||||
// Verify no warning was logged
|
||||
expect(mockConsoleWarn).not.toHaveBeenCalledWith(expect.stringContaining("Skipping large .gitignore file"))
|
||||
})
|
||||
|
||||
it("should handle multiple .gitignore files with mixed sizes", async () => {
|
||||
// Mock finding .gitignore files at different levels
|
||||
mockAccess.mockResolvedValue(undefined) // All files exist
|
||||
|
||||
// Mock different .gitignore files with different sizes
|
||||
// First call is for the .gitignore in the current directory
|
||||
mockStat
|
||||
.mockResolvedValueOnce({ size: 1024 }) // First .gitignore - OK
|
||||
.mockResolvedValueOnce({ size: MAX_GITIGNORE_FILE_SIZE_BYTES + 1000 }) // Second - too large
|
||||
|
||||
mockReadFile.mockResolvedValueOnce("*.log\n")
|
||||
|
||||
// Call listFiles with recursive to trigger .gitignore checks
|
||||
await listFiles("/test/dir", true, 100)
|
||||
|
||||
// Verify that readFile was called only for files within limit
|
||||
expect(mockReadFile).toHaveBeenCalled()
|
||||
|
||||
// Verify warning was logged for the large file if multiple .gitignore files were found
|
||||
// The test might only find one .gitignore depending on the mock setup
|
||||
const warnCalls = mockConsoleWarn.mock.calls
|
||||
const hasLargeFileWarning = warnCalls.some((call) => call[0].includes("Skipping large .gitignore file"))
|
||||
|
||||
// If we had multiple stat calls, we should have seen the warning
|
||||
if (mockStat.mock.calls.length > 1) {
|
||||
expect(hasLargeFileWarning).toBe(true)
|
||||
}
|
||||
})
|
||||
|
||||
it("should continue processing when .gitignore stat fails", async () => {
|
||||
// Mock finding a .gitignore file
|
||||
mockAccess.mockResolvedValue(undefined) // File exists
|
||||
|
||||
// Mock stat failure
|
||||
mockStat.mockRejectedValue(new Error("Permission denied"))
|
||||
|
||||
// Call listFiles
|
||||
const [results] = await listFiles("/test/dir", false, 100)
|
||||
|
||||
// Should still return results despite .gitignore stat failure
|
||||
expect(results.length).toBeGreaterThan(0)
|
||||
|
||||
// Verify warning was logged
|
||||
expect(mockConsoleWarn).toHaveBeenCalledWith(expect.stringContaining("Error reading .gitignore"))
|
||||
})
|
||||
|
||||
it("should handle .gitignore at exactly the size limit", async () => {
|
||||
// Mock finding a .gitignore file
|
||||
mockAccess.mockResolvedValue(undefined) // File exists
|
||||
|
||||
// Mock .gitignore file at exactly the size limit
|
||||
mockStat.mockResolvedValue({ size: MAX_GITIGNORE_FILE_SIZE_BYTES })
|
||||
mockReadFile.mockResolvedValue("*.tmp\n")
|
||||
|
||||
// Call listFiles
|
||||
await listFiles("/test/dir", false, 100)
|
||||
|
||||
// Verify that readFile was called (file at limit should be read)
|
||||
expect(mockReadFile).toHaveBeenCalledWith(expect.stringContaining(".gitignore"), "utf8")
|
||||
|
||||
// Verify no warning was logged
|
||||
expect(mockConsoleWarn).not.toHaveBeenCalledWith(expect.stringContaining("Skipping large .gitignore file"))
|
||||
})
|
||||
})
|
||||
|
|
@ -7,6 +7,7 @@ import ignore from "ignore"
|
|||
import { arePathsEqual } from "../../utils/path"
|
||||
import { getBinPath } from "../../services/ripgrep"
|
||||
import { DIRS_TO_IGNORE } from "./constants"
|
||||
import { MAX_GITIGNORE_FILE_SIZE_BYTES } from "../constants/file-limits"
|
||||
|
||||
/**
|
||||
* List files in a directory, with optional recursive traversal
|
||||
|
|
@ -257,6 +258,15 @@ async function createIgnoreInstance(dirPath: string): Promise<ReturnType<typeof
|
|||
// Add patterns from all .gitignore files
|
||||
for (const gitignoreFile of gitignoreFiles) {
|
||||
try {
|
||||
// Check file size before reading
|
||||
const stats = await fs.promises.stat(gitignoreFile)
|
||||
if (stats.size > MAX_GITIGNORE_FILE_SIZE_BYTES) {
|
||||
console.warn(
|
||||
`Skipping large .gitignore file: ${gitignoreFile} (${stats.size} bytes > ${MAX_GITIGNORE_FILE_SIZE_BYTES} bytes)`,
|
||||
)
|
||||
continue
|
||||
}
|
||||
|
||||
const content = await fs.promises.readFile(gitignoreFile, "utf8")
|
||||
ignoreInstance.add(content)
|
||||
} catch (err) {
|
||||
|
|
|
|||
|
|
@ -32,6 +32,7 @@ import {
|
|||
import { fileExistsAtPath } from "../../utils/fs"
|
||||
import { arePathsEqual } from "../../utils/path"
|
||||
import { injectVariables } from "../../utils/config"
|
||||
import { MAX_CONFIG_FILE_SIZE_BYTES } from "../constants/file-limits"
|
||||
|
||||
export type McpConnection = {
|
||||
server: McpServer
|
||||
|
|
@ -276,9 +277,29 @@ export class McpHub {
|
|||
this.configChangeDebounceTimers.set(key, timer)
|
||||
}
|
||||
|
||||
/**
|
||||
* Safely reads a config file with size validation
|
||||
* @param filePath Path to the config file
|
||||
* @returns The file content or null if the file is too large
|
||||
* @throws Error if file cannot be read (other than size limit)
|
||||
*/
|
||||
private async readConfigFile(filePath: string): Promise<string | null> {
|
||||
const stats = await fs.stat(filePath)
|
||||
if (stats.size > MAX_CONFIG_FILE_SIZE_BYTES) {
|
||||
const errorMessage = `Config file ${filePath} exceeds size limit (${stats.size} bytes > ${MAX_CONFIG_FILE_SIZE_BYTES} bytes)`
|
||||
console.error(errorMessage)
|
||||
vscode.window.showErrorMessage(errorMessage)
|
||||
return null
|
||||
}
|
||||
return await fs.readFile(filePath, "utf-8")
|
||||
}
|
||||
|
||||
private async handleConfigFileChange(filePath: string, source: "global" | "project"): Promise<void> {
|
||||
try {
|
||||
const content = await fs.readFile(filePath, "utf-8")
|
||||
const content = await this.readConfigFile(filePath)
|
||||
if (!content) {
|
||||
return // File too large, error already shown
|
||||
}
|
||||
let config: any
|
||||
|
||||
try {
|
||||
|
|
@ -364,7 +385,10 @@ export class McpHub {
|
|||
const projectMcpPath = await this.getProjectMcpPath()
|
||||
if (!projectMcpPath) return
|
||||
|
||||
const content = await fs.readFile(projectMcpPath, "utf-8")
|
||||
const content = await this.readConfigFile(projectMcpPath)
|
||||
if (!content) {
|
||||
return // File too large, error already shown
|
||||
}
|
||||
let config: any
|
||||
|
||||
try {
|
||||
|
|
@ -492,7 +516,10 @@ export class McpHub {
|
|||
return
|
||||
}
|
||||
|
||||
const content = await fs.readFile(configPath, "utf-8")
|
||||
const content = await this.readConfigFile(configPath)
|
||||
if (!content) {
|
||||
return // File too large, error already shown
|
||||
}
|
||||
const config = JSON.parse(content)
|
||||
const result = McpSettingsSchema.safeParse(config)
|
||||
|
||||
|
|
@ -846,14 +873,18 @@ export class McpHub {
|
|||
const projectMcpPath = await this.getProjectMcpPath()
|
||||
if (projectMcpPath) {
|
||||
configPath = projectMcpPath
|
||||
const content = await fs.readFile(configPath, "utf-8")
|
||||
serverConfigData = JSON.parse(content)
|
||||
const content = await this.readConfigFile(configPath)
|
||||
if (content) {
|
||||
serverConfigData = JSON.parse(content)
|
||||
}
|
||||
}
|
||||
} else {
|
||||
// Get global MCP settings path
|
||||
configPath = await this.getMcpSettingsFilePath()
|
||||
const content = await fs.readFile(configPath, "utf-8")
|
||||
serverConfigData = JSON.parse(content)
|
||||
const content = await this.readConfigFile(configPath)
|
||||
if (content) {
|
||||
serverConfigData = JSON.parse(content)
|
||||
}
|
||||
}
|
||||
if (serverConfigData) {
|
||||
alwaysAllowConfig = serverConfigData.mcpServers?.[serverName]?.alwaysAllow || []
|
||||
|
|
@ -1118,15 +1149,17 @@ export class McpHub {
|
|||
const globalPath = await this.getMcpSettingsFilePath()
|
||||
let globalServers: Record<string, any> = {}
|
||||
try {
|
||||
const globalContent = await fs.readFile(globalPath, "utf-8")
|
||||
const globalConfig = JSON.parse(globalContent)
|
||||
globalServers = globalConfig.mcpServers || {}
|
||||
const globalServerNames = Object.keys(globalServers)
|
||||
vscode.window.showInformationMessage(
|
||||
t("mcp:info.global_servers_active", {
|
||||
mcpServers: `${globalServerNames.join(", ") || "none"}`,
|
||||
}),
|
||||
)
|
||||
const globalContent = await this.readConfigFile(globalPath)
|
||||
if (globalContent) {
|
||||
const globalConfig = JSON.parse(globalContent)
|
||||
globalServers = globalConfig.mcpServers || {}
|
||||
const globalServerNames = Object.keys(globalServers)
|
||||
vscode.window.showInformationMessage(
|
||||
t("mcp:info.global_servers_active", {
|
||||
mcpServers: `${globalServerNames.join(", ") || "none"}`,
|
||||
}),
|
||||
)
|
||||
}
|
||||
} catch (error) {
|
||||
console.log("Error reading global MCP config:", error)
|
||||
}
|
||||
|
|
@ -1135,15 +1168,17 @@ export class McpHub {
|
|||
let projectServers: Record<string, any> = {}
|
||||
if (projectPath) {
|
||||
try {
|
||||
const projectContent = await fs.readFile(projectPath, "utf-8")
|
||||
const projectConfig = JSON.parse(projectContent)
|
||||
projectServers = projectConfig.mcpServers || {}
|
||||
const projectServerNames = Object.keys(projectServers)
|
||||
vscode.window.showInformationMessage(
|
||||
t("mcp:info.project_servers_active", {
|
||||
mcpServers: `${projectServerNames.join(", ") || "none"}`,
|
||||
}),
|
||||
)
|
||||
const projectContent = await this.readConfigFile(projectPath)
|
||||
if (projectContent) {
|
||||
const projectConfig = JSON.parse(projectContent)
|
||||
projectServers = projectConfig.mcpServers || {}
|
||||
const projectServerNames = Object.keys(projectServers)
|
||||
vscode.window.showInformationMessage(
|
||||
t("mcp:info.project_servers_active", {
|
||||
mcpServers: `${projectServerNames.join(", ") || "none"}`,
|
||||
}),
|
||||
)
|
||||
}
|
||||
} catch (error) {
|
||||
console.log("Error reading project MCP config:", error)
|
||||
}
|
||||
|
|
@ -1175,8 +1210,8 @@ export class McpHub {
|
|||
private async notifyWebviewOfServerChanges(): Promise<void> {
|
||||
// Get global server order from settings file
|
||||
const settingsPath = await this.getMcpSettingsFilePath()
|
||||
const content = await fs.readFile(settingsPath, "utf-8")
|
||||
const config = JSON.parse(content)
|
||||
const content = await this.readConfigFile(settingsPath)
|
||||
const config = content ? JSON.parse(content) : { mcpServers: {} }
|
||||
const globalServerOrder = Object.keys(config.mcpServers || {})
|
||||
|
||||
// Get project server order if available
|
||||
|
|
@ -1184,9 +1219,11 @@ export class McpHub {
|
|||
let projectServerOrder: string[] = []
|
||||
if (projectMcpPath) {
|
||||
try {
|
||||
const projectContent = await fs.readFile(projectMcpPath, "utf-8")
|
||||
const projectConfig = JSON.parse(projectContent)
|
||||
projectServerOrder = Object.keys(projectConfig.mcpServers || {})
|
||||
const projectContent = await this.readConfigFile(projectMcpPath)
|
||||
if (projectContent) {
|
||||
const projectConfig = JSON.parse(projectContent)
|
||||
projectServerOrder = Object.keys(projectConfig.mcpServers || {})
|
||||
}
|
||||
} catch (error) {
|
||||
// Silently continue with empty project server order
|
||||
}
|
||||
|
|
@ -1310,7 +1347,10 @@ export class McpHub {
|
|||
}
|
||||
|
||||
// Read and parse the config file
|
||||
const content = await fs.readFile(configPath, "utf-8")
|
||||
const content = await this.readConfigFile(configPath)
|
||||
if (!content) {
|
||||
throw new Error("Config file too large or unreadable")
|
||||
}
|
||||
const config = JSON.parse(content)
|
||||
|
||||
// Validate the config structure
|
||||
|
|
@ -1401,7 +1441,10 @@ export class McpHub {
|
|||
throw new Error("Settings file not accessible")
|
||||
}
|
||||
|
||||
const content = await fs.readFile(configPath, "utf-8")
|
||||
const content = await this.readConfigFile(configPath)
|
||||
if (!content) {
|
||||
throw new Error("Config file too large or unreadable")
|
||||
}
|
||||
const config = JSON.parse(content)
|
||||
|
||||
// Validate the config structure
|
||||
|
|
@ -1539,7 +1582,10 @@ export class McpHub {
|
|||
const normalizedPath = process.platform === "win32" ? configPath.replace(/\\/g, "/") : configPath
|
||||
|
||||
// Read the appropriate config file
|
||||
const content = await fs.readFile(normalizedPath, "utf-8")
|
||||
const content = await this.readConfigFile(normalizedPath)
|
||||
if (!content) {
|
||||
throw new Error("Config file too large or unreadable")
|
||||
}
|
||||
const config = JSON.parse(content)
|
||||
|
||||
if (!config.mcpServers) {
|
||||
|
|
|
|||
|
|
@ -2,10 +2,11 @@ import * as path from "path"
|
|||
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"
|
||||
|
||||
// Use vi.hoisted to ensure mocks are available during hoisting
|
||||
const { mockStat, mockReadFile, mockHomedir } = vi.hoisted(() => ({
|
||||
const { mockStat, mockReadFile, mockHomedir, mockConsoleWarn } = vi.hoisted(() => ({
|
||||
mockStat: vi.fn(),
|
||||
mockReadFile: vi.fn(),
|
||||
mockHomedir: vi.fn(),
|
||||
mockConsoleWarn: vi.fn(),
|
||||
}))
|
||||
|
||||
// Mock fs/promises module
|
||||
|
|
@ -30,11 +31,14 @@ import {
|
|||
getRooDirectoriesForCwd,
|
||||
loadConfiguration,
|
||||
} from "../index"
|
||||
import { MAX_CONFIG_FILE_SIZE_BYTES } from "../../constants/file-limits"
|
||||
|
||||
describe("RooConfigService", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
mockHomedir.mockReturnValue("/mock/home")
|
||||
// Mock console.warn
|
||||
console.warn = mockConsoleWarn
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
|
|
@ -157,51 +161,91 @@ describe("RooConfigService", () => {
|
|||
})
|
||||
|
||||
describe("readFileIfExists", () => {
|
||||
it("should return file content for existing file", async () => {
|
||||
it("should return file content for existing file within size limit", async () => {
|
||||
mockStat.mockResolvedValue({ size: 1024 }) // 1KB, well under limit
|
||||
mockReadFile.mockResolvedValue("file content")
|
||||
|
||||
const result = await readFileIfExists("/some/file.txt")
|
||||
|
||||
expect(result).toBe("file content")
|
||||
expect(mockStat).toHaveBeenCalledWith("/some/file.txt")
|
||||
expect(mockReadFile).toHaveBeenCalledWith("/some/file.txt", "utf-8")
|
||||
})
|
||||
|
||||
it("should return null for file exceeding size limit", async () => {
|
||||
const largeSize = MAX_CONFIG_FILE_SIZE_BYTES + 1
|
||||
mockStat.mockResolvedValue({ size: largeSize })
|
||||
|
||||
const result = await readFileIfExists("/large/file.txt")
|
||||
|
||||
expect(result).toBe(null)
|
||||
expect(mockStat).toHaveBeenCalledWith("/large/file.txt")
|
||||
expect(mockReadFile).not.toHaveBeenCalled()
|
||||
expect(mockConsoleWarn).toHaveBeenCalledWith(
|
||||
`File /large/file.txt exceeds size limit (${largeSize} bytes > ${MAX_CONFIG_FILE_SIZE_BYTES} bytes)`,
|
||||
)
|
||||
})
|
||||
|
||||
it("should return file content for file exactly at size limit", async () => {
|
||||
mockStat.mockResolvedValue({ size: MAX_CONFIG_FILE_SIZE_BYTES })
|
||||
mockReadFile.mockResolvedValue("file content at limit")
|
||||
|
||||
const result = await readFileIfExists("/exact/limit/file.txt")
|
||||
|
||||
expect(result).toBe("file content at limit")
|
||||
expect(mockStat).toHaveBeenCalledWith("/exact/limit/file.txt")
|
||||
expect(mockReadFile).toHaveBeenCalledWith("/exact/limit/file.txt", "utf-8")
|
||||
})
|
||||
|
||||
it("should return null for non-existing file", async () => {
|
||||
const error = new Error("ENOENT") as any
|
||||
error.code = "ENOENT"
|
||||
mockReadFile.mockRejectedValue(error)
|
||||
mockStat.mockRejectedValue(error)
|
||||
|
||||
const result = await readFileIfExists("/non/existing/file.txt")
|
||||
|
||||
expect(result).toBe(null)
|
||||
expect(mockReadFile).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("should return null for ENOTDIR error", async () => {
|
||||
const error = new Error("ENOTDIR") as any
|
||||
error.code = "ENOTDIR"
|
||||
mockReadFile.mockRejectedValue(error)
|
||||
mockStat.mockRejectedValue(error)
|
||||
|
||||
const result = await readFileIfExists("/not/a/directory/file.txt")
|
||||
|
||||
expect(result).toBe(null)
|
||||
expect(mockReadFile).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("should return null for EISDIR error", async () => {
|
||||
const error = new Error("EISDIR") as any
|
||||
error.code = "EISDIR"
|
||||
mockReadFile.mockRejectedValue(error)
|
||||
mockStat.mockRejectedValue(error)
|
||||
|
||||
const result = await readFileIfExists("/is/a/directory")
|
||||
|
||||
expect(result).toBe(null)
|
||||
expect(mockReadFile).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("should throw unexpected errors", async () => {
|
||||
it("should throw unexpected errors from stat", async () => {
|
||||
const error = new Error("Permission denied") as any
|
||||
error.code = "EACCES"
|
||||
mockReadFile.mockRejectedValue(error)
|
||||
mockStat.mockRejectedValue(error)
|
||||
|
||||
await expect(readFileIfExists("/permission/denied/file.txt")).rejects.toThrow("Permission denied")
|
||||
expect(mockReadFile).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("should throw unexpected errors from readFile", async () => {
|
||||
mockStat.mockResolvedValue({ size: 1024 }) // Within limit
|
||||
const error = new Error("Read error") as any
|
||||
error.code = "EIO"
|
||||
mockReadFile.mockRejectedValue(error)
|
||||
|
||||
await expect(readFileIfExists("/io/error/file.txt")).rejects.toThrow("Read error")
|
||||
})
|
||||
})
|
||||
|
||||
|
|
@ -219,7 +263,9 @@ describe("RooConfigService", () => {
|
|||
it("should load global configuration only when project does not exist", async () => {
|
||||
const error = new Error("ENOENT") as any
|
||||
error.code = "ENOENT"
|
||||
mockReadFile.mockResolvedValueOnce("global content").mockRejectedValueOnce(error)
|
||||
// Mock stat calls for readFileIfExists
|
||||
mockStat.mockResolvedValueOnce({ size: 100 }).mockRejectedValueOnce(error)
|
||||
mockReadFile.mockResolvedValueOnce("global content")
|
||||
|
||||
const result = await loadConfiguration("rules/rules.md", "/project/path")
|
||||
|
||||
|
|
@ -233,7 +279,9 @@ describe("RooConfigService", () => {
|
|||
it("should load project configuration only when global does not exist", async () => {
|
||||
const error = new Error("ENOENT") as any
|
||||
error.code = "ENOENT"
|
||||
mockReadFile.mockRejectedValueOnce(error).mockResolvedValueOnce("project content")
|
||||
// Mock stat calls for readFileIfExists
|
||||
mockStat.mockRejectedValueOnce(error).mockResolvedValueOnce({ size: 100 })
|
||||
mockReadFile.mockResolvedValueOnce("project content")
|
||||
|
||||
const result = await loadConfiguration("rules/rules.md", "/project/path")
|
||||
|
||||
|
|
@ -245,6 +293,8 @@ describe("RooConfigService", () => {
|
|||
})
|
||||
|
||||
it("should merge global and project configurations with project overriding global", async () => {
|
||||
// Mock stat calls for readFileIfExists
|
||||
mockStat.mockResolvedValueOnce({ size: 100 }).mockResolvedValueOnce({ size: 100 })
|
||||
mockReadFile.mockResolvedValueOnce("global content").mockResolvedValueOnce("project content")
|
||||
|
||||
const result = await loadConfiguration("rules/rules.md", "/project/path")
|
||||
|
|
@ -259,7 +309,8 @@ describe("RooConfigService", () => {
|
|||
it("should return empty merged content when neither exists", async () => {
|
||||
const error = new Error("ENOENT") as any
|
||||
error.code = "ENOENT"
|
||||
mockReadFile.mockRejectedValueOnce(error).mockRejectedValueOnce(error)
|
||||
// Mock stat calls for readFileIfExists
|
||||
mockStat.mockRejectedValueOnce(error).mockRejectedValueOnce(error)
|
||||
|
||||
const result = await loadConfiguration("rules/rules.md", "/project/path")
|
||||
|
||||
|
|
@ -273,7 +324,7 @@ describe("RooConfigService", () => {
|
|||
it("should propagate unexpected errors from global file read", async () => {
|
||||
const error = new Error("Permission denied") as any
|
||||
error.code = "EACCES"
|
||||
mockReadFile.mockRejectedValueOnce(error)
|
||||
mockStat.mockRejectedValueOnce(error)
|
||||
|
||||
await expect(loadConfiguration("rules/rules.md", "/project/path")).rejects.toThrow("Permission denied")
|
||||
})
|
||||
|
|
@ -284,18 +335,37 @@ describe("RooConfigService", () => {
|
|||
const projectError = new Error("Permission denied") as any
|
||||
projectError.code = "EACCES"
|
||||
|
||||
mockReadFile.mockRejectedValueOnce(globalError).mockRejectedValueOnce(projectError)
|
||||
mockStat.mockRejectedValueOnce(globalError).mockRejectedValueOnce(projectError)
|
||||
|
||||
await expect(loadConfiguration("rules/rules.md", "/project/path")).rejects.toThrow("Permission denied")
|
||||
})
|
||||
|
||||
it("should use correct file paths", async () => {
|
||||
mockStat.mockResolvedValue({ size: 100 })
|
||||
mockReadFile.mockResolvedValue("content")
|
||||
|
||||
await loadConfiguration("rules/rules.md", "/project/path")
|
||||
|
||||
expect(mockStat).toHaveBeenCalledWith(path.join("/mock/home", ".roo", "rules/rules.md"))
|
||||
expect(mockStat).toHaveBeenCalledWith(path.join("/project/path", ".roo", "rules/rules.md"))
|
||||
expect(mockReadFile).toHaveBeenCalledWith(path.join("/mock/home", ".roo", "rules/rules.md"), "utf-8")
|
||||
expect(mockReadFile).toHaveBeenCalledWith(path.join("/project/path", ".roo", "rules/rules.md"), "utf-8")
|
||||
})
|
||||
|
||||
it("should handle large config files gracefully", async () => {
|
||||
const largeSize = MAX_CONFIG_FILE_SIZE_BYTES + 1
|
||||
// Global file is too large, project file is normal
|
||||
mockStat.mockResolvedValueOnce({ size: largeSize }).mockResolvedValueOnce({ size: 100 })
|
||||
mockReadFile.mockResolvedValueOnce("project content")
|
||||
|
||||
const result = await loadConfiguration("rules/rules.md", "/project/path")
|
||||
|
||||
expect(result).toEqual({
|
||||
global: null,
|
||||
project: "project content",
|
||||
merged: "project content",
|
||||
})
|
||||
expect(mockConsoleWarn).toHaveBeenCalledWith(expect.stringContaining("exceeds size limit"))
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import * as path from "path"
|
||||
import * as os from "os"
|
||||
import fs from "fs/promises"
|
||||
import { MAX_CONFIG_FILE_SIZE_BYTES } from "../constants/file-limits"
|
||||
|
||||
/**
|
||||
* Gets the global .roo directory path based on the current platform
|
||||
|
|
@ -100,6 +101,15 @@ export async function fileExists(filePath: string): Promise<boolean> {
|
|||
*/
|
||||
export async function readFileIfExists(filePath: string): Promise<string | null> {
|
||||
try {
|
||||
// Check file size before reading
|
||||
const stats = await fs.stat(filePath)
|
||||
if (stats.size > MAX_CONFIG_FILE_SIZE_BYTES) {
|
||||
console.warn(
|
||||
`File ${filePath} exceeds size limit (${stats.size} bytes > ${MAX_CONFIG_FILE_SIZE_BYTES} bytes)`,
|
||||
)
|
||||
return null
|
||||
}
|
||||
|
||||
return await fs.readFile(filePath, "utf-8")
|
||||
} catch (error: any) {
|
||||
// Only catch expected "not found" errors
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue