From b3615dc11186c05f0062573d3e25e2cdb1b66f10 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 6 Jan 2026 16:35:35 +0000 Subject: [PATCH] fix: simplify safeWriteJson to always pretty-print - Remove optional indent parameter from safeWriteJson - always use 2-space indent - Remove streaming approach for compact output (no longer needed) - Update McpHub.ts to remove explicit indent=2 parameters - Update tests to reflect the simplified behavior - Add test for un-compacting previously compacted JSON files --- src/services/mcp/McpHub.ts | 8 +-- src/services/mcp/__tests__/McpHub.spec.ts | 18 ++--- src/utils/__tests__/safeWriteJson.test.ts | 68 +++++++++---------- src/utils/safeWriteJson.ts | 81 +++++------------------ 4 files changed, 55 insertions(+), 120 deletions(-) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index d0040a762e..b5b6cbe30a 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -479,7 +479,7 @@ export class McpHub { ) const fileExists = await fileExistsAtPath(mcpSettingsFilePath) if (!fileExists) { - await safeWriteJson(mcpSettingsFilePath, { mcpServers: {} }, 2) + await safeWriteJson(mcpSettingsFilePath, { mcpServers: {} }) } return mcpSettingsFilePath } @@ -1568,7 +1568,7 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(configPath, updatedConfig, 2) + await safeWriteJson(configPath, updatedConfig) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { @@ -1653,7 +1653,7 @@ export class McpHub { mcpServers: config.mcpServers, } - await safeWriteJson(configPath, updatedConfig, 2) + await safeWriteJson(configPath, updatedConfig) // Update server connections with the correct source await this.updateServerConnections(config.mcpServers, serverSource) @@ -1804,7 +1804,7 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(normalizedPath, config, 2) + await safeWriteJson(normalizedPath, config) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index dd8592c0b2..6092df6c9b 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -41,12 +41,12 @@ import { safeWriteJson } from "../../../utils/safeWriteJson" // Mock safeWriteJson vi.mock("../../../utils/safeWriteJson", () => ({ - safeWriteJson: vi.fn(async (filePath, data, indent) => { + safeWriteJson: vi.fn(async (filePath, data) => { // Instead of trying to write to the file system, just call fs.writeFile mock // This avoids the complex file locking and temp file operations const fs = await import("fs/promises") - // Support indent parameter for formatting - const jsonString = indent !== undefined ? JSON.stringify(data, null, indent) : JSON.stringify(data) + // Always pretty-print with 2-space indentation (matching the real implementation) + const jsonString = JSON.stringify(data, null, 2) return fs.writeFile(filePath, jsonString, "utf8") }), })) @@ -914,7 +914,7 @@ describe("McpHub", () => { expect(writtenConfig.mcpServers["test-server"].alwaysAllow).toContain("new-tool") }) - it("should preserve JSON pretty-print formatting when toggling tool always allow", async () => { + it("should always write pretty-printed JSON when toggling tool always allow", async () => { const mockConfig = { mcpServers: { "test-server": { @@ -947,17 +947,11 @@ describe("McpHub", () => { await mcpHub.toggleToolAlwaysAllow("test-server", "global", "new-tool", true) - // Verify safeWriteJson was called with indent parameter + // Verify safeWriteJson was called (it always pretty-prints now) const safeWriteJsonMock = vi.mocked(safeWriteJson) expect(safeWriteJsonMock).toHaveBeenCalled() - // Get the last call to safeWriteJson (most recent write) - const lastCall = safeWriteJsonMock.mock.calls[safeWriteJsonMock.mock.calls.length - 1] - - // Verify indent=2 was passed (third parameter) - expect(lastCall[2]).toBe(2) - - // Verify the written content would be pretty-printed + // Verify the written content is pretty-printed const writeCalls = vi.mocked(fs.writeFile).mock.calls const lastWriteCall = writeCalls[writeCalls.length - 1] if (lastWriteCall) { diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 689c14a156..1e24d4f3ea 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -478,10 +478,10 @@ describe("safeWriteJson", () => { consoleErrorSpy.mockRestore() }) - // Tests for pretty-printing functionality - test("should write pretty-printed JSON with numeric indent parameter", async () => { + // Tests for pretty-printing functionality (always enabled) + test("should always write pretty-printed JSON with 2-space indentation", async () => { const data = { mcpServers: { test: { command: "node", args: ["test.js"] } } } - await safeWriteJson(currentTestFilePath, data, 2) + await safeWriteJson(currentTestFilePath, data) const content = await fs.readFile(currentTestFilePath, "utf-8") const parsed = JSON.parse(content) @@ -495,38 +495,7 @@ describe("safeWriteJson", () => { expect(content).toMatch(/\s+"test":\s+\{/) }) - test("should write pretty-printed JSON with string indent parameter", async () => { - const data = { test: "value", nested: { key: "data" } } - await safeWriteJson(currentTestFilePath, data, "\t") - - const content = await fs.readFile(currentTestFilePath, "utf-8") - const parsed = JSON.parse(content) - - // Verify data is correct - expect(parsed).toEqual(data) - - // Verify it uses tab indentation - expect(content).toContain("\t") - expect(content).toContain("\n") - }) - - test("should maintain compact format when no indent parameter is specified", async () => { - const data = { test: "value", nested: { key: "data" } } - await safeWriteJson(currentTestFilePath, data) - - const content = await fs.readFile(currentTestFilePath, "utf-8") - const parsed = JSON.parse(content) - - // Verify data is correct - expect(parsed).toEqual(data) - - // Verify it's compact (no newlines except possibly at the very end) - // Compact JSON from stream-json should be all on one line - const lines = content.split("\n").filter((line) => line.trim().length > 0) - expect(lines.length).toBe(1) - }) - - test("should preserve formatting for complex nested structures with indent", async () => { + test("should preserve formatting for complex nested structures", async () => { const data = { mcpServers: { server1: { @@ -542,7 +511,7 @@ describe("safeWriteJson", () => { }, }, } - await safeWriteJson(currentTestFilePath, data, 2) + await safeWriteJson(currentTestFilePath, data) const content = await fs.readFile(currentTestFilePath, "utf-8") const parsed = JSON.parse(content) @@ -557,8 +526,8 @@ describe("safeWriteJson", () => { expect(content).toMatch(/"alwaysAllow":\s*\[/) }) - test("should handle undefined data with indent parameter", async () => { - await safeWriteJson(currentTestFilePath, undefined, 2) + test("should handle undefined data by converting to null", async () => { + await safeWriteJson(currentTestFilePath, undefined) const content = await fs.readFile(currentTestFilePath, "utf-8") const parsed = JSON.parse(content) @@ -567,4 +536,27 @@ describe("safeWriteJson", () => { expect(parsed).toBeNull() expect(content.trim()).toBe("null") }) + + test("should un-compact previously compacted JSON files on save", async () => { + // Simulate a previously compacted JSON file + const compactJson = '{"mcpServers":{"test":{"command":"node","args":["test.js"],"alwaysAllow":["tool1"]}}}' + await fs.writeFile(currentTestFilePath, compactJson) + + // Read the data and write it back using safeWriteJson + const data = JSON.parse(compactJson) + await safeWriteJson(currentTestFilePath, data) + + const content = await fs.readFile(currentTestFilePath, "utf-8") + const parsed = JSON.parse(content) + + // Verify data is correct + expect(parsed).toEqual(data) + + // Verify the output is now pretty-printed (un-compacted) + expect(content).toContain("\n") + expect(content).toMatch(/\{\s+"mcpServers"/) + // Should have multiple lines now + const lines = content.split("\n").filter((line) => line.trim().length > 0) + expect(lines.length).toBeGreaterThan(1) + }) }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index e7ada4e8e6..209fbab773 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -2,24 +2,22 @@ import * as fs from "fs/promises" import * as fsSync from "fs" import * as path from "path" import * as lockfile from "proper-lockfile" -import Disassembler from "stream-json/Disassembler" -import Stringer from "stream-json/Stringer" /** - * Safely writes JSON data to a file. + * Safely writes JSON data to a file with pretty-printing (2-space indentation). * - Creates parent directories if they don't exist * - Uses 'proper-lockfile' for inter-process advisory locking to prevent concurrent writes to the same path. * - Writes to a temporary file first. * - If the target file exists, it's backed up before being replaced. * - Attempts to roll back and clean up in case of errors. + * - Always outputs pretty-printed JSON for readability and manual editing. * * @param {string} filePath - The absolute path to the target file. * @param {any} data - The data to serialize to JSON and write. - * @param {number | string} indent - Optional indentation for pretty-printing. Use a number for spaces or a string (e.g., '\t') for custom indentation. * @returns {Promise} */ -async function safeWriteJson(filePath: string, data: any, indent?: number | string): Promise { +async function safeWriteJson(filePath: string, data: any): Promise { const absoluteFilePath = path.resolve(filePath) let releaseLock = async () => {} // Initialized to a no-op @@ -76,7 +74,7 @@ async function safeWriteJson(filePath: string, data: any, indent?: number | stri `.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, ) - await _streamDataToFile(actualTempNewFilePath, data, indent) + await _streamDataToFile(actualTempNewFilePath, data) // Step 2: Check if the target file exists. If so, rename it to a backup path. try { @@ -180,71 +178,22 @@ async function safeWriteJson(filePath: string, data: any, indent?: number | stri } /** - * Helper function to stream JSON data to a file. - * @param targetPath The path to write the stream to. - * @param data The data to stream. - * @param indent Optional indentation for pretty-printing. + * Helper function to write JSON data to a file with pretty-printing. + * @param targetPath The path to write to. + * @param data The data to serialize as JSON. * @returns Promise */ -async function _streamDataToFile(targetPath: string, data: any, indent?: number | string): Promise { - // If indent is specified, use JSON.stringify for pretty-printing - // This is suitable for small config files where readability is more important than memory efficiency - if (indent !== undefined) { - const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" }) - return new Promise((resolve, reject) => { - fileWriteStream.on("error", reject) - fileWriteStream.on("finish", resolve) - - // Handle undefined data by converting to null for valid JSON - const jsonString = JSON.stringify(data === undefined ? null : data, null, indent) - fileWriteStream.write(jsonString) - fileWriteStream.end() - }) - } - - // For compact output, use streaming to avoid high memory usage for large JSON objects +async function _streamDataToFile(targetPath: string, data: any): Promise { const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" }) - const disassembler = Disassembler.disassembler() - const stringer = Stringer.stringer() - return new Promise((resolve, reject) => { - let errorOccurred = false - const handleError = (_streamName: string) => (err: Error) => { - if (!errorOccurred) { - errorOccurred = true - if (!fileWriteStream.destroyed) { - fileWriteStream.destroy(err) - } - reject(err) - } - } + fileWriteStream.on("error", reject) + fileWriteStream.on("finish", resolve) - disassembler.on("error", handleError("Disassembler")) - stringer.on("error", handleError("Stringer")) - fileWriteStream.on("error", (err: Error) => { - if (!errorOccurred) { - errorOccurred = true - reject(err) - } - }) - - fileWriteStream.on("finish", () => { - if (!errorOccurred) { - resolve() - } - }) - - disassembler.pipe(stringer).pipe(fileWriteStream) - - // stream-json's Disassembler might error if `data` is undefined. - // JSON.stringify(undefined) would produce the string "undefined" if it's the root value. - // Writing 'null' is a safer JSON representation for a root undefined value. - if (data === undefined) { - disassembler.write(null) - } else { - disassembler.write(data) - } - disassembler.end() + // Handle undefined data by converting to null for valid JSON + // Always use 2-space indentation for readability + const jsonString = JSON.stringify(data === undefined ? null : data, null, 2) + fileWriteStream.write(jsonString) + fileWriteStream.end() }) }