From 502cf4701aa593414881a709b66a1b5cfb10bfd3 Mon Sep 17 00:00:00 2001 From: Eric Wheeler Date: Tue, 1 Jul 2025 21:35:48 -0700 Subject: [PATCH] refactor: replace fs.readFile + JSON.parse with safeReadJson Replace manual file reading and JSON parsing with the safer safeReadJson utility across multiple files in the codebase. This change: - Provides atomic file access with proper locking to prevent race conditions - Streams file contents efficiently for better memory usage - Improves error handling consistency - Reduces code duplication Fixes: #5331 Signed-off-by: Eric Wheeler --- src/api/providers/fetchers/modelCache.ts | 14 +++-- .../providers/fetchers/modelEndpointCache.ts | 9 ++- src/core/config/importExport.ts | 3 +- .../context-tracking/FileContextTracker.ts | 13 +++-- src/core/task-persistence/apiMessages.ts | 57 ++++++++++--------- src/core/task-persistence/taskMessages.ts | 16 +++--- src/core/webview/ClineProvider.ts | 10 +++- src/integrations/misc/extract-text.ts | 4 +- src/services/code-index/cache-manager.ts | 4 +- .../marketplace/MarketplaceManager.ts | 7 +-- src/services/marketplace/SimpleInstaller.ts | 7 +-- src/services/mcp/McpHub.ts | 46 +++++++-------- src/services/mdm/MdmService.ts | 17 +++--- 13 files changed, 105 insertions(+), 102 deletions(-) diff --git a/src/api/providers/fetchers/modelCache.ts b/src/api/providers/fetchers/modelCache.ts index 2ccb73a455..e1e88bd7e5 100644 --- a/src/api/providers/fetchers/modelCache.ts +++ b/src/api/providers/fetchers/modelCache.ts @@ -2,15 +2,13 @@ import * as path from "path" import fs from "fs/promises" import NodeCache from "node-cache" - import type { ProviderName } from "@roo-code/types" - +import { safeReadJson } from "../../../utils/safeReadJson" import { safeWriteJson } from "../../../utils/safeWriteJson" import { ContextProxy } from "../../../core/config/ContextProxy" import { getCacheDirectoryPath } from "../../../utils/storage" import type { RouterName, ModelRecord } from "../../../shared/api" -import { fileExistsAtPath } from "../../../utils/fs" import { getOpenRouterModels } from "./openrouter" import { getVercelAiGatewayModels } from "./vercel-ai-gateway" @@ -37,8 +35,14 @@ async function readModels(router: RouterName): Promise const filename = `${router}_models.json` const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath) const filePath = path.join(cacheDir, filename) - const exists = await fileExistsAtPath(filePath) - return exists ? JSON.parse(await fs.readFile(filePath, "utf8")) : undefined + try { + return await safeReadJson(filePath) + } catch (error: any) { + if (error.code === "ENOENT") { + return undefined + } + throw error + } } /** diff --git a/src/api/providers/fetchers/modelEndpointCache.ts b/src/api/providers/fetchers/modelEndpointCache.ts index 256ae84048..e149d558bd 100644 --- a/src/api/providers/fetchers/modelEndpointCache.ts +++ b/src/api/providers/fetchers/modelEndpointCache.ts @@ -2,13 +2,13 @@ import * as path from "path" import fs from "fs/promises" import NodeCache from "node-cache" +import { safeReadJson } from "../../../utils/safeReadJson" import { safeWriteJson } from "../../../utils/safeWriteJson" import sanitize from "sanitize-filename" import { ContextProxy } from "../../../core/config/ContextProxy" import { getCacheDirectoryPath } from "../../../utils/storage" import { RouterName, ModelRecord } from "../../../shared/api" -import { fileExistsAtPath } from "../../../utils/fs" import { getOpenRouterModelEndpoints } from "./openrouter" @@ -26,8 +26,11 @@ async function readModelEndpoints(key: string): Promise const filename = `${key}_endpoints.json` const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath) const filePath = path.join(cacheDir, filename) - const exists = await fileExistsAtPath(filePath) - return exists ? JSON.parse(await fs.readFile(filePath, "utf8")) : undefined + try { + return await safeReadJson(filePath) + } catch (error) { + return undefined + } } export const getModelEndpoints = async ({ diff --git a/src/core/config/importExport.ts b/src/core/config/importExport.ts index c3d6f9c215..c19ea4998b 100644 --- a/src/core/config/importExport.ts +++ b/src/core/config/importExport.ts @@ -1,3 +1,4 @@ +import { safeReadJson } from "../../utils/safeReadJson" import { safeWriteJson } from "../../utils/safeWriteJson" import os from "os" import * as path from "path" @@ -49,7 +50,7 @@ export async function importSettingsFromPath( const previousProviderProfiles = await providerSettingsManager.export() const { providerProfiles: newProviderProfiles, globalSettings = {} } = schema.parse( - JSON.parse(await fs.readFile(filePath, "utf-8")), + await safeReadJson(filePath), ) const providerProfiles = { diff --git a/src/core/context-tracking/FileContextTracker.ts b/src/core/context-tracking/FileContextTracker.ts index 5741b62cfc..45d15c2ce2 100644 --- a/src/core/context-tracking/FileContextTracker.ts +++ b/src/core/context-tracking/FileContextTracker.ts @@ -1,10 +1,9 @@ +import { safeReadJson } from "../../utils/safeReadJson" import { safeWriteJson } from "../../utils/safeWriteJson" import * as path from "path" import * as vscode from "vscode" import { getTaskDirectoryPath } from "../../utils/storage" import { GlobalFileNames } from "../../shared/globalFileNames" -import { fileExistsAtPath } from "../../utils/fs" -import fs from "fs/promises" import { ContextProxy } from "../config/ContextProxy" import type { FileMetadataEntry, RecordSource, TaskMetadata } from "./FileContextTrackerTypes" import { ClineProvider } from "../webview/ClineProvider" @@ -116,12 +115,14 @@ export class FileContextTracker { const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) const filePath = path.join(taskDir, GlobalFileNames.taskMetadata) try { - if (await fileExistsAtPath(filePath)) { - return JSON.parse(await fs.readFile(filePath, "utf8")) - } + return await safeReadJson(filePath) } catch (error) { - console.error("Failed to read task metadata:", error) + if (error.code !== "ENOENT") { + console.error("Failed to read task metadata:", error) + } } + + // On error, return default empty metadata return { files_in_context: [] } } diff --git a/src/core/task-persistence/apiMessages.ts b/src/core/task-persistence/apiMessages.ts index f846aaf13f..f4b8c4046c 100644 --- a/src/core/task-persistence/apiMessages.ts +++ b/src/core/task-persistence/apiMessages.ts @@ -1,3 +1,4 @@ +import { safeReadJson } from "../../utils/safeReadJson" import { safeWriteJson } from "../../utils/safeWriteJson" import * as path from "path" import * as fs from "fs/promises" @@ -21,29 +22,21 @@ export async function readApiMessages({ const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) const filePath = path.join(taskDir, GlobalFileNames.apiConversationHistory) - if (await fileExistsAtPath(filePath)) { - const fileContent = await fs.readFile(filePath, "utf8") - try { - const parsedData = JSON.parse(fileContent) - if (Array.isArray(parsedData) && parsedData.length === 0) { - console.error( - `[Roo-Debug] readApiMessages: Found API conversation history file, but it's empty (parsed as []). TaskId: ${taskId}, Path: ${filePath}`, - ) - } - return parsedData - } catch (error) { + try { + const parsedData = await safeReadJson(filePath) + if (Array.isArray(parsedData) && parsedData.length === 0) { console.error( - `[Roo-Debug] readApiMessages: Error parsing API conversation history file. TaskId: ${taskId}, Path: ${filePath}, Error: ${error}`, + `[Roo-Debug] readApiMessages: Found API conversation history file, but it's empty (parsed as []). TaskId: ${taskId}, Path: ${filePath}`, ) - throw error } - } else { - const oldPath = path.join(taskDir, "claude_messages.json") + return parsedData + } catch (error: any) { + if (error.code === "ENOENT") { + // File doesn't exist, try the old path + const oldPath = path.join(taskDir, "claude_messages.json") - if (await fileExistsAtPath(oldPath)) { - const fileContent = await fs.readFile(oldPath, "utf8") try { - const parsedData = JSON.parse(fileContent) + const parsedData = await safeReadJson(oldPath) if (Array.isArray(parsedData) && parsedData.length === 0) { console.error( `[Roo-Debug] readApiMessages: Found OLD API conversation history file (claude_messages.json), but it's empty (parsed as []). TaskId: ${taskId}, Path: ${oldPath}`, @@ -51,21 +44,29 @@ export async function readApiMessages({ } await fs.unlink(oldPath) return parsedData - } catch (error) { + } catch (oldError: any) { + if (oldError.code === "ENOENT") { + // If we reach here, neither the new nor the old history file was found. + console.error( + `[Roo-Debug] readApiMessages: API conversation history file not found for taskId: ${taskId}. Expected at: ${filePath}`, + ) + return [] + } + + // For any other error with the old file, log and rethrow console.error( - `[Roo-Debug] readApiMessages: Error parsing OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${error}`, + `[Roo-Debug] readApiMessages: Error reading OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${oldError}`, ) - // DO NOT unlink oldPath if parsing failed, throw error instead. - throw error + throw oldError } + } else { + // For any other error with the main file, log and rethrow + console.error( + `[Roo-Debug] readApiMessages: Error reading API conversation history file. TaskId: ${taskId}, Path: ${filePath}, Error: ${error}`, + ) + throw error } } - - // If we reach here, neither the new nor the old history file was found. - console.error( - `[Roo-Debug] readApiMessages: API conversation history file not found for taskId: ${taskId}. Expected at: ${filePath}`, - ) - return [] } export async function saveApiMessages({ diff --git a/src/core/task-persistence/taskMessages.ts b/src/core/task-persistence/taskMessages.ts index 63a2eefbaa..cc86ab0aaa 100644 --- a/src/core/task-persistence/taskMessages.ts +++ b/src/core/task-persistence/taskMessages.ts @@ -1,11 +1,9 @@ +import { safeReadJson } from "../../utils/safeReadJson" import { safeWriteJson } from "../../utils/safeWriteJson" import * as path from "path" -import * as fs from "fs/promises" import type { ClineMessage } from "@roo-code/types" -import { fileExistsAtPath } from "../../utils/fs" - import { GlobalFileNames } from "../../shared/globalFileNames" import { getTaskDirectoryPath } from "../../utils/storage" @@ -20,13 +18,15 @@ export async function readTaskMessages({ }: ReadTaskMessagesOptions): Promise { const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) const filePath = path.join(taskDir, GlobalFileNames.uiMessages) - const fileExists = await fileExistsAtPath(filePath) - if (fileExists) { - return JSON.parse(await fs.readFile(filePath, "utf8")) + try { + return await safeReadJson(filePath) + } catch (error) { + if (error.code !== "ENOENT") { + console.error("Failed to read task messages:", error) + } + return [] } - - return [] } export type SaveTaskMessagesOptions = { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 9abddc6d96..db66e126eb 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -82,6 +82,7 @@ import { t } from "../../i18n" import { buildApiHandler } from "../../api" import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../api/providers/fetchers/lmstudio" +import { safeReadJson } from "../../utils/safeReadJson" import { ContextProxy } from "../config/ContextProxy" import { ProviderSettingsManager } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" @@ -1457,10 +1458,9 @@ export class ClineProvider const taskDirPath = await getTaskDirectoryPath(globalStoragePath, id) const apiConversationHistoryFilePath = path.join(taskDirPath, GlobalFileNames.apiConversationHistory) const uiMessagesFilePath = path.join(taskDirPath, GlobalFileNames.uiMessages) - const fileExists = await fileExistsAtPath(apiConversationHistoryFilePath) - if (fileExists) { - const apiConversationHistory = JSON.parse(await fs.readFile(apiConversationHistoryFilePath, "utf8")) + try { + const apiConversationHistory = await safeReadJson(apiConversationHistoryFilePath) return { historyItem, @@ -1469,6 +1469,10 @@ export class ClineProvider uiMessagesFilePath, apiConversationHistory, } + } catch (error) { + if (error.code !== "ENOENT") { + console.error(`Failed to read API conversation history for task ${id}:`, error) + } } } diff --git a/src/integrations/misc/extract-text.ts b/src/integrations/misc/extract-text.ts index 8231c609be..cf82f68239 100644 --- a/src/integrations/misc/extract-text.ts +++ b/src/integrations/misc/extract-text.ts @@ -7,6 +7,7 @@ import { isBinaryFile } from "isbinaryfile" import { extractTextFromXLSX } from "./extract-text-from-xlsx" import { countFileLines } from "./line-counter" import { readLines } from "./read-lines" +import { safeReadJson } from "../../utils/safeReadJson" async function extractTextFromPDF(filePath: string): Promise { const dataBuffer = await fs.readFile(filePath) @@ -20,8 +21,7 @@ async function extractTextFromDOCX(filePath: string): Promise { } async function extractTextFromIPYNB(filePath: string): Promise { - const data = await fs.readFile(filePath, "utf8") - const notebook = JSON.parse(data) + const notebook = await safeReadJson(filePath) let extractedText = "" for (const cell of notebook.cells) { diff --git a/src/services/code-index/cache-manager.ts b/src/services/code-index/cache-manager.ts index a9a4f0ac47..ff7ae8d8f9 100644 --- a/src/services/code-index/cache-manager.ts +++ b/src/services/code-index/cache-manager.ts @@ -2,6 +2,7 @@ import * as vscode from "vscode" import { createHash } from "crypto" import { ICacheManager } from "./interfaces/cache" import debounce from "lodash.debounce" +import { safeReadJson } from "../../utils/safeReadJson" import { safeWriteJson } from "../../utils/safeWriteJson" import { TelemetryService } from "@roo-code/telemetry" import { TelemetryEventName } from "@roo-code/types" @@ -37,8 +38,7 @@ export class CacheManager implements ICacheManager { */ async initialize(): Promise { try { - const cacheData = await vscode.workspace.fs.readFile(this.cachePath) - this.fileHashes = JSON.parse(cacheData.toString()) + this.fileHashes = await safeReadJson(this.cachePath.fsPath) } catch (error) { this.fileHashes = {} TelemetryService.instance.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { diff --git a/src/services/marketplace/MarketplaceManager.ts b/src/services/marketplace/MarketplaceManager.ts index dfde5600b9..364c9950e7 100644 --- a/src/services/marketplace/MarketplaceManager.ts +++ b/src/services/marketplace/MarketplaceManager.ts @@ -15,6 +15,7 @@ import type { CustomModesManager } from "../../core/config/CustomModesManager" import { RemoteConfigLoader } from "./RemoteConfigLoader" import { SimpleInstaller } from "./SimpleInstaller" +import { safeReadJson } from "../../utils/safeReadJson" export interface MarketplaceItemsResponse { organizationMcps: MarketplaceItem[] @@ -272,8 +273,7 @@ export class MarketplaceManager { // Check MCPs in .roo/mcp.json const projectMcpPath = path.join(workspaceFolder.uri.fsPath, ".roo", "mcp.json") try { - const content = await fs.readFile(projectMcpPath, "utf-8") - const data = JSON.parse(content) + const data = await safeReadJson(projectMcpPath) if (data?.mcpServers && typeof data.mcpServers === "object") { for (const serverName of Object.keys(data.mcpServers)) { metadata[serverName] = { @@ -317,8 +317,7 @@ export class MarketplaceManager { // Check global MCPs const globalMcpPath = path.join(globalSettingsPath, GlobalFileNames.mcpSettings) try { - const content = await fs.readFile(globalMcpPath, "utf-8") - const data = JSON.parse(content) + const data = await safeReadJson(globalMcpPath) if (data?.mcpServers && typeof data.mcpServers === "object") { for (const serverName of Object.keys(data.mcpServers)) { metadata[serverName] = { diff --git a/src/services/marketplace/SimpleInstaller.ts b/src/services/marketplace/SimpleInstaller.ts index be002e2f1d..3c2f6a2fee 100644 --- a/src/services/marketplace/SimpleInstaller.ts +++ b/src/services/marketplace/SimpleInstaller.ts @@ -6,6 +6,7 @@ import type { MarketplaceItem, MarketplaceItemType, InstallMarketplaceItemOption import { GlobalFileNames } from "../../shared/globalFileNames" import { ensureSettingsDirectoryExists } from "../../utils/globalContext" import type { CustomModesManager } from "../../core/config/CustomModesManager" +import { safeReadJson } from "../../utils/safeReadJson" export interface InstallOptions extends InstallMarketplaceItemOptions { target: "project" | "global" @@ -229,8 +230,7 @@ export class SimpleInstaller { // Read existing file or create new structure let existingData: any = { mcpServers: {} } try { - const existing = await fs.readFile(filePath, "utf-8") - existingData = JSON.parse(existing) || { mcpServers: {} } + existingData = (await safeReadJson(filePath)) || { mcpServers: {} } } catch (error: any) { if (error.code === "ENOENT") { // File doesn't exist, use default structure @@ -332,8 +332,7 @@ export class SimpleInstaller { const filePath = await this.getMcpFilePath(target) try { - const existing = await fs.readFile(filePath, "utf-8") - const existingData = JSON.parse(existing) + const existingData = await safeReadJson(filePath) if (existingData?.mcpServers) { // Parse the item content to get server names diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index caca5ddb39..485dc03a69 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -18,6 +18,8 @@ import * as path from "path" import * as vscode from "vscode" import { z } from "zod" import { t } from "../../i18n" +import { safeReadJson } from "../../utils/safeReadJson" +import { safeWriteJson } from "../../utils/safeWriteJson" import { ClineProvider } from "../../core/webview/ClineProvider" import { GlobalFileNames } from "../../shared/globalFileNames" @@ -297,11 +299,9 @@ export class McpHub { private async handleConfigFileChange(filePath: string, source: "global" | "project"): Promise { try { - const content = await fs.readFile(filePath, "utf-8") let config: any - try { - config = JSON.parse(content) + config = await safeReadJson(filePath) } catch (parseError) { const errorMessage = t("mcp:errors.invalid_settings_syntax") console.error(errorMessage, parseError) @@ -383,11 +383,9 @@ export class McpHub { const projectMcpPath = await this.getProjectMcpPath() if (!projectMcpPath) return - const content = await fs.readFile(projectMcpPath, "utf-8") let config: any - try { - config = JSON.parse(content) + config = await safeReadJson(projectMcpPath) } catch (parseError) { const errorMessage = t("mcp:errors.invalid_settings_syntax") console.error(errorMessage, parseError) @@ -511,8 +509,7 @@ export class McpHub { return } - const content = await fs.readFile(configPath, "utf-8") - const config = JSON.parse(content) + const config = await safeReadJson(configPath) const result = McpSettingsSchema.safeParse(config) if (result.success) { @@ -927,14 +924,12 @@ export class McpHub { const projectMcpPath = await this.getProjectMcpPath() if (projectMcpPath) { configPath = projectMcpPath - const content = await fs.readFile(configPath, "utf-8") - serverConfigData = JSON.parse(content) + serverConfigData = await safeReadJson(configPath) } } else { // Get global MCP settings path configPath = await this.getMcpSettingsFilePath() - const content = await fs.readFile(configPath, "utf-8") - serverConfigData = JSON.parse(content) + serverConfigData = await safeReadJson(configPath) } if (serverConfigData) { alwaysAllowConfig = serverConfigData.mcpServers?.[serverName]?.alwaysAllow || [] @@ -1236,8 +1231,7 @@ export class McpHub { const globalPath = await this.getMcpSettingsFilePath() let globalServers: Record = {} try { - const globalContent = await fs.readFile(globalPath, "utf-8") - const globalConfig = JSON.parse(globalContent) + const globalConfig = await safeReadJson(globalPath) globalServers = globalConfig.mcpServers || {} const globalServerNames = Object.keys(globalServers) } catch (error) { @@ -1248,8 +1242,7 @@ export class McpHub { let projectServers: Record = {} if (projectPath) { try { - const projectContent = await fs.readFile(projectPath, "utf-8") - const projectConfig = JSON.parse(projectContent) + const projectConfig = await safeReadJson(projectPath) projectServers = projectConfig.mcpServers || {} const projectServerNames = Object.keys(projectServers) } catch (error) { @@ -1281,8 +1274,7 @@ export class McpHub { private async notifyWebviewOfServerChanges(): Promise { // 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 config = await safeReadJson(settingsPath) const globalServerOrder = Object.keys(config.mcpServers || {}) // Get project server order if available @@ -1290,8 +1282,7 @@ export class McpHub { let projectServerOrder: string[] = [] if (projectMcpPath) { try { - const projectContent = await fs.readFile(projectMcpPath, "utf-8") - const projectConfig = JSON.parse(projectContent) + const projectConfig = await safeReadJson(projectMcpPath) projectServerOrder = Object.keys(projectConfig.mcpServers || {}) } catch (error) { // Silently continue with empty project server order @@ -1429,8 +1420,9 @@ export class McpHub { } // Read and parse the config file - const content = await fs.readFile(configPath, "utf-8") - const config = JSON.parse(content) + // This is a read-modify-write-operation, but we cannot + // use safeWriteJson because it does not (yet) support pretty printing. + const config = await safeReadJson(configPath) // Validate the config structure if (!config || typeof config !== "object") { @@ -1520,8 +1512,9 @@ export class McpHub { throw new Error("Settings file not accessible") } - const content = await fs.readFile(configPath, "utf-8") - const config = JSON.parse(content) + // This is a read-modify-write-operation, but we cannot + // use safeWriteJson because it does not (yet) support pretty printing. + const config = await safeReadJson(configPath) // Validate the config structure if (!config || typeof config !== "object") { @@ -1658,8 +1651,9 @@ 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 config = JSON.parse(content) + // This is a read-modify-write-operation, but we cannot + // use safeWriteJson because it does not (yet) support pretty printing. + const config = await safeReadJson(configPath) if (!config.mcpServers) { config.mcpServers = {} diff --git a/src/services/mdm/MdmService.ts b/src/services/mdm/MdmService.ts index 63bdbe29fc..c06dec442e 100644 --- a/src/services/mdm/MdmService.ts +++ b/src/services/mdm/MdmService.ts @@ -4,7 +4,7 @@ import * as os from "os" import { z } from "zod" import { CloudService, getClerkBaseUrl, PRODUCTION_CLERK_BASE_URL } from "@roo-code/cloud" - +import { safeReadJson } from "../../utils/safeReadJson" import { t } from "../../i18n" // MDM Configuration Schema @@ -120,19 +120,16 @@ export class MdmService { const configPath = this.getMdmConfigPath() try { - // Check if file exists - if (!fs.existsSync(configPath)) { - return null - } - - // Read and parse the configuration file - const configContent = fs.readFileSync(configPath, "utf-8") - const parsedConfig = JSON.parse(configContent) + // Read and parse the configuration file using safeReadJson + const parsedConfig = await safeReadJson(configPath) // Validate against schema return mdmConfigSchema.parse(parsedConfig) } catch (error) { - this.log(`[MDM] Error reading MDM config from ${configPath}:`, error) + // If file doesn't exist, return null + if ((error as any)?.code !== "ENOENT") { + this.log(`[MDM] Error reading MDM config from ${configPath}:`, error) + } return null } }