diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 1c35c8b89f2..b5b6cbe30a5 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -479,14 +479,7 @@ export class McpHub { ) const fileExists = await fileExistsAtPath(mcpSettingsFilePath) if (!fileExists) { - await fs.writeFile( - mcpSettingsFilePath, - `{ - "mcpServers": { - - } -}`, - ) + await safeWriteJson(mcpSettingsFilePath, { mcpServers: {} }) } return mcpSettingsFilePath } diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index 2d895fdbca5..6092df6c9bf 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -45,7 +45,9 @@ vi.mock("../../../utils/safeWriteJson", () => ({ // 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") - return fs.writeFile(filePath, JSON.stringify(data), "utf8") + // Always pretty-print with 2-space indentation (matching the real implementation) + const jsonString = JSON.stringify(data, null, 2) + return fs.writeFile(filePath, jsonString, "utf8") }), })) @@ -911,6 +913,55 @@ describe("McpHub", () => { expect(writtenConfig.mcpServers["test-server"].alwaysAllow).toBeDefined() expect(writtenConfig.mcpServers["test-server"].alwaysAllow).toContain("new-tool") }) + + it("should always write pretty-printed JSON when toggling tool always allow", async () => { + const mockConfig = { + mcpServers: { + "test-server": { + type: "stdio", + command: "node", + args: ["test.js"], + alwaysAllow: [], + }, + }, + } + + // Mock reading initial config + vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify(mockConfig)) + + // Set up mock connection + const mockConnection: ConnectedMcpConnection = { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + alwaysAllow: [], + source: "global", + } as any, + client: {} as any, + transport: {} as any, + } + mcpHub.connections = [mockConnection] + + await mcpHub.toggleToolAlwaysAllow("test-server", "global", "new-tool", true) + + // Verify safeWriteJson was called (it always pretty-prints now) + const safeWriteJsonMock = vi.mocked(safeWriteJson) + expect(safeWriteJsonMock).toHaveBeenCalled() + + // Verify the written content is pretty-printed + const writeCalls = vi.mocked(fs.writeFile).mock.calls + const lastWriteCall = writeCalls[writeCalls.length - 1] + if (lastWriteCall) { + const writtenContent = lastWriteCall[1] as string + // Verify it contains newlines (not compact) + expect(writtenContent).toContain("\n") + // Verify proper indentation + expect(writtenContent).toMatch(/\{\s+"mcpServers":\s+\{/) + } + }) }) describe("toggleToolEnabledForPrompt", () => { diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index e18b86a02d8..1e24d4f3ea4 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -477,4 +477,86 @@ describe("safeWriteJson", () => { consoleErrorSpy.mockRestore() }) + + // 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) + + const content = await fs.readFile(currentTestFilePath, "utf-8") + const parsed = JSON.parse(content) + + // Verify data is correct + expect(parsed).toEqual(data) + + // Verify it's pretty-printed (contains newlines and proper indentation) + expect(content).toContain("\n") + expect(content).toMatch(/\{\s+"mcpServers":\s+\{/) + expect(content).toMatch(/\s+"test":\s+\{/) + }) + + test("should preserve formatting for complex nested structures", async () => { + const data = { + mcpServers: { + server1: { + command: "node", + args: ["index.js", "--flag"], + env: { VAR: "value" }, + alwaysAllow: ["tool1", "tool2"], + }, + server2: { + command: "python", + args: ["script.py"], + disabled: true, + }, + }, + } + 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 proper formatting with multiple levels of nesting + expect(content).toContain("\n") + expect(content).toMatch(/"mcpServers":\s*{/) + expect(content).toMatch(/"server1":\s*{/) + expect(content).toMatch(/"alwaysAllow":\s*\[/) + }) + + 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) + + // undefined should be converted to null + 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 719bbd72167..209fbab773a 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -2,16 +2,15 @@ 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. @@ -179,56 +178,22 @@ async function safeWriteJson(filePath: string, data: any): Promise { } /** - * Helper function to stream JSON data to a file. - * @param targetPath The path to write the stream to. - * @param data The data to stream. + * 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): Promise { - // Stream data to avoid high memory usage for large JSON objects. const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" }) - const disassembler = Disassembler.disassembler() - // Output will be compact JSON as standard Stringer is used. - 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) - } - } - - 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() + fileWriteStream.on("error", reject) + fileWriteStream.on("finish", resolve) + + // 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() }) }