feat: ensure mcp-settings.json is always pretty-printed

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
 	}

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", () => {

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)
+	})
 })

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<void> {
 }
 
 /**
- * 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<void>
  */
 async function _streamDataToFile(targetPath: string, data: any): Promise<void> {
-	// 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<void>((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()
 	})
 }