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.