From 4b774b9961a35f78e93a3bfd3d332c99edddc5c9 Mon Sep 17 00:00:00 2001 From: Eric Wheeler Date: Tue, 20 May 2025 20:06:30 -0700 Subject: [PATCH] fix: use safeWriteJson for all JSON file writes This change refactors all direct JSON file writes to use the safeWriteJson utility, which implements atomic file writes to prevent data corruption during write operations. - Modified safeWriteJson to accept optional replacer and space arguments - Updated tests to verify correct behavior with the new implementation Fixes: #722 Signed-off-by: Eric Wheeler --- .../config/__tests__/importExport.spec.ts | 30 +++++++++++-------- src/core/config/importExport.ts | 3 +- .../context-tracking/FileContextTracker.ts | 3 +- src/core/task-persistence/apiMessages.ts | 3 +- src/core/task-persistence/taskMessages.ts | 3 +- .../webview/__tests__/ClineProvider.spec.ts | 10 +++---- src/core/webview/webviewMessageHandler.ts | 3 +- src/services/mcp/McpHub.ts | 7 +++-- src/utils/safeWriteJson.ts | 9 ++++-- 9 files changed, 43 insertions(+), 28 deletions(-) diff --git a/src/core/config/__tests__/importExport.spec.ts b/src/core/config/__tests__/importExport.spec.ts index 4ba43f475e..4a525c3e63 100644 --- a/src/core/config/__tests__/importExport.spec.ts +++ b/src/core/config/__tests__/importExport.spec.ts @@ -12,6 +12,7 @@ import { importSettings, exportSettings } from "../importExport" import { ProviderSettingsManager } from "../ProviderSettingsManager" import { ContextProxy } from "../ContextProxy" import { CustomModesManager } from "../CustomModesManager" +import { safeWriteJson } from "../../../utils/safeWriteJson" import type { Mock } from "vitest" @@ -43,6 +44,8 @@ vi.mock("os", () => ({ homedir: vi.fn(() => "/mock/home"), })) +vi.mock("../../../utils/safeWriteJson") + describe("importExport", () => { let mockProviderSettingsManager: ReturnType> let mockContextProxy: ReturnType> @@ -384,11 +387,10 @@ describe("importExport", () => { expect(mockContextProxy.export).toHaveBeenCalled() expect(fs.mkdir).toHaveBeenCalledWith("/mock/path", { recursive: true }) - expect(fs.writeFile).toHaveBeenCalledWith( - "/mock/path/roo-code-settings.json", - JSON.stringify({ providerProfiles: mockProviderProfiles, globalSettings: mockGlobalSettings }, null, 2), - "utf-8", - ) + expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/roo-code-settings.json", { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, + }) }) it("should include globalSettings when allowedMaxRequests is null", async () => { @@ -417,11 +419,10 @@ describe("importExport", () => { contextProxy: mockContextProxy, }) - expect(fs.writeFile).toHaveBeenCalledWith( - "/mock/path/roo-code-settings.json", - JSON.stringify({ providerProfiles: mockProviderProfiles, globalSettings: mockGlobalSettings }, null, 2), - "utf-8", - ) + expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/roo-code-settings.json", { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, + }) }) it("should handle errors during the export process", async () => { @@ -436,7 +437,8 @@ describe("importExport", () => { }) mockContextProxy.export.mockResolvedValue({ mode: "code" }) - ;(fs.writeFile as Mock).mockRejectedValue(new Error("Write error")) + // Simulate an error during the safeWriteJson operation + ;(safeWriteJson as Mock).mockRejectedValueOnce(new Error("Safe write error")) await exportSettings({ providerSettingsManager: mockProviderSettingsManager, @@ -447,8 +449,10 @@ describe("importExport", () => { expect(mockProviderSettingsManager.export).toHaveBeenCalled() expect(mockContextProxy.export).toHaveBeenCalled() expect(fs.mkdir).toHaveBeenCalledWith("/mock/path", { recursive: true }) - expect(fs.writeFile).toHaveBeenCalled() + expect(safeWriteJson).toHaveBeenCalled() // safeWriteJson is called, but it will throw // The error is caught and the function exits silently. + // Optionally, ensure no error message was shown if that's part of "silent" + // expect(vscode.window.showErrorMessage).not.toHaveBeenCalled(); }) it("should handle errors during directory creation", async () => { @@ -474,7 +478,7 @@ describe("importExport", () => { expect(mockProviderSettingsManager.export).toHaveBeenCalled() expect(mockContextProxy.export).toHaveBeenCalled() expect(fs.mkdir).toHaveBeenCalled() - expect(fs.writeFile).not.toHaveBeenCalled() // Should not be called since mkdir failed. + expect(safeWriteJson).not.toHaveBeenCalled() // Should not be called since mkdir failed. }) it("should use the correct default save location", async () => { diff --git a/src/core/config/importExport.ts b/src/core/config/importExport.ts index 4830a5f987..cd92a81ff6 100644 --- a/src/core/config/importExport.ts +++ b/src/core/config/importExport.ts @@ -1,3 +1,4 @@ +import { safeWriteJson } from "../../utils/safeWriteJson" import os from "os" import * as path from "path" import fs from "fs/promises" @@ -116,6 +117,6 @@ export const exportSettings = async ({ providerSettingsManager, contextProxy }: const dirname = path.dirname(uri.fsPath) await fs.mkdir(dirname, { recursive: true }) - await fs.writeFile(uri.fsPath, JSON.stringify({ providerProfiles, globalSettings }, null, 2), "utf-8") + await safeWriteJson(uri.fsPath, { providerProfiles, globalSettings }) } catch (e) {} } diff --git a/src/core/context-tracking/FileContextTracker.ts b/src/core/context-tracking/FileContextTracker.ts index 323bb4122f..5741b62cfc 100644 --- a/src/core/context-tracking/FileContextTracker.ts +++ b/src/core/context-tracking/FileContextTracker.ts @@ -1,3 +1,4 @@ +import { safeWriteJson } from "../../utils/safeWriteJson" import * as path from "path" import * as vscode from "vscode" import { getTaskDirectoryPath } from "../../utils/storage" @@ -130,7 +131,7 @@ export class FileContextTracker { const globalStoragePath = this.getContextProxy()!.globalStorageUri.fsPath const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) const filePath = path.join(taskDir, GlobalFileNames.taskMetadata) - await fs.writeFile(filePath, JSON.stringify(metadata, null, 2)) + await safeWriteJson(filePath, metadata) } catch (error) { console.error("Failed to save task metadata:", error) } diff --git a/src/core/task-persistence/apiMessages.ts b/src/core/task-persistence/apiMessages.ts index d6c17bd9b3..f846aaf13f 100644 --- a/src/core/task-persistence/apiMessages.ts +++ b/src/core/task-persistence/apiMessages.ts @@ -1,3 +1,4 @@ +import { safeWriteJson } from "../../utils/safeWriteJson" import * as path from "path" import * as fs from "fs/promises" @@ -78,5 +79,5 @@ export async function saveApiMessages({ }) { const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) const filePath = path.join(taskDir, GlobalFileNames.apiConversationHistory) - await fs.writeFile(filePath, JSON.stringify(messages)) + await safeWriteJson(filePath, messages) } diff --git a/src/core/task-persistence/taskMessages.ts b/src/core/task-persistence/taskMessages.ts index 3ed5c5099e..63a2eefbaa 100644 --- a/src/core/task-persistence/taskMessages.ts +++ b/src/core/task-persistence/taskMessages.ts @@ -1,3 +1,4 @@ +import { safeWriteJson } from "../../utils/safeWriteJson" import * as path from "path" import * as fs from "fs/promises" @@ -37,5 +38,5 @@ export type SaveTaskMessagesOptions = { export async function saveTaskMessages({ messages, taskId, globalStoragePath }: SaveTaskMessagesOptions) { const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) const filePath = path.join(taskDir, GlobalFileNames.uiMessages) - await fs.writeFile(filePath, JSON.stringify(messages)) + await safeWriteJson(filePath, messages) } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index fabb0aae60..6e3ef13dd8 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -13,6 +13,7 @@ import { experimentDefault } from "../../../shared/experiments" import { setTtsEnabled } from "../../../utils/tts" import { ContextProxy } from "../../config/ContextProxy" import { Task, TaskOptions } from "../../task/Task" +import { safeWriteJson } from "../../../utils/safeWriteJson" import { ClineProvider } from "../ClineProvider" @@ -43,6 +44,8 @@ vi.mock("axios", () => ({ post: vi.fn(), })) +vi.mock("../../../utils/safeWriteJson") + vi.mock("@modelcontextprotocol/sdk/types.js", () => ({ CallToolResultSchema: {}, ListResourcesResultSchema: {}, @@ -1976,11 +1979,8 @@ describe("Project MCP Settings", () => { // Check that fs.mkdir was called with the correct path expect(mockedFs.mkdir).toHaveBeenCalledWith("/test/workspace/.roo", { recursive: true }) - // Check that fs.writeFile was called with default content - expect(mockedFs.writeFile).toHaveBeenCalledWith( - "/test/workspace/.roo/mcp.json", - JSON.stringify({ mcpServers: {} }, null, 2), - ) + // Verify file was created with default content + expect(safeWriteJson).toHaveBeenCalledWith("/test/workspace/.roo/mcp.json", { mcpServers: {} }) // Check that openFile was called expect(openFileSpy).toHaveBeenCalledWith("/test/workspace/.roo/mcp.json") diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index f689196d79..4c7e8351c7 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -1,3 +1,4 @@ +import { safeWriteJson } from "../../utils/safeWriteJson" import * as path from "path" import fs from "fs/promises" import pWaitFor from "p-wait-for" @@ -594,7 +595,7 @@ export const webviewMessageHandler = async ( const exists = await fileExistsAtPath(mcpPath) if (!exists) { - await fs.writeFile(mcpPath, JSON.stringify({ mcpServers: {} }, null, 2)) + await safeWriteJson(mcpPath, { mcpServers: {} }) } await openFile(mcpPath) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 10a74712ef..feb87fd956 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -1,3 +1,4 @@ +import { safeWriteJson } from "../../utils/safeWriteJson" import { Client } from "@modelcontextprotocol/sdk/client/index.js" import { StdioClientTransport, getDefaultEnvironment } from "@modelcontextprotocol/sdk/client/stdio.js" import { SSEClientTransport } from "@modelcontextprotocol/sdk/client/sse.js" @@ -1344,7 +1345,7 @@ export class McpHub { mcpServers: config.mcpServers, } - await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2)) + await safeWriteJson(configPath, updatedConfig) } public async updateServerTimeout( @@ -1422,7 +1423,7 @@ export class McpHub { mcpServers: config.mcpServers, } - await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2)) + await safeWriteJson(configPath, updatedConfig) // Update server connections with the correct source await this.updateServerConnections(config.mcpServers, serverSource) @@ -1567,7 +1568,7 @@ export class McpHub { targetList.splice(toolIndex, 1) } - await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2)) + await safeWriteJson(normalizedPath, config) if (connection) { connection.server.tools = await this.fetchToolsList(serverName, source) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 2038b2457b..02412f21bb 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -14,7 +14,12 @@ const activeLocks = new Set() * @param {any} data - The data to serialize to JSON and write. * @returns {Promise} */ -async function safeWriteJson(filePath: string, data: any): Promise { +async function safeWriteJson( + filePath: string, + data: any, + replacer?: (key: string, value: any) => any, + space: string | number = 2, +): Promise { const absoluteFilePath = path.resolve(filePath) if (activeLocks.has(absoluteFilePath)) { @@ -33,7 +38,7 @@ async function safeWriteJson(filePath: string, data: any): Promise { path.dirname(absoluteFilePath), `.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, ) - const jsonData = JSON.stringify(data, null, 2) + const jsonData = JSON.stringify(data, replacer, space) await fs.writeFile(actualTempNewFilePath, jsonData, "utf8") // Step 2: Check if the target file exists. If so, rename it to a backup path.