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

- Add optional indent parameter to safeWriteJson for pretty-printing
- Update all mcp-settings.json write operations to use indent=2
- Replace fs.writeFile with safeWriteJson for initial file creation
- Add comprehensive unit and integration tests
- Maintain backward compatibility with default compact output
This commit is contained in:
Roo Code 2025-12-31 21:25:44 +00:00
parent 2068531801
commit 1b465be350
4 changed files with 174 additions and 18 deletions

View file

@ -479,14 +479,7 @@ export class McpHub {
)
const fileExists = await fileExistsAtPath(mcpSettingsFilePath)
if (!fileExists) {
await fs.writeFile(
mcpSettingsFilePath,
`{
"mcpServers": {
}
}`,
)
await safeWriteJson(mcpSettingsFilePath, { mcpServers: {} }, 2)
}
return mcpSettingsFilePath
}
@ -1575,7 +1568,7 @@ export class McpHub {
}
this.isProgrammaticUpdate = true
try {
await safeWriteJson(configPath, updatedConfig)
await safeWriteJson(configPath, updatedConfig, 2)
} finally {
// Reset flag after watcher debounce period (non-blocking)
this.flagResetTimer = setTimeout(() => {
@ -1660,7 +1653,7 @@ export class McpHub {
mcpServers: config.mcpServers,
}
await safeWriteJson(configPath, updatedConfig)
await safeWriteJson(configPath, updatedConfig, 2)
// Update server connections with the correct source
await this.updateServerConnections(config.mcpServers, serverSource)
@ -1811,7 +1804,7 @@ export class McpHub {
}
this.isProgrammaticUpdate = true
try {
await safeWriteJson(normalizedPath, config)
await safeWriteJson(normalizedPath, config, 2)
} finally {
// Reset flag after watcher debounce period (non-blocking)
this.flagResetTimer = setTimeout(() => {

View file

@ -41,11 +41,13 @@ import { safeWriteJson } from "../../../utils/safeWriteJson"
// Mock safeWriteJson
vi.mock("../../../utils/safeWriteJson", () => ({
safeWriteJson: vi.fn(async (filePath, data) => {
safeWriteJson: vi.fn(async (filePath, data, indent) => {
// 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")
// Support indent parameter for formatting
const jsonString = indent !== undefined ? JSON.stringify(data, null, indent) : JSON.stringify(data)
return fs.writeFile(filePath, jsonString, "utf8")
}),
}))
@ -911,6 +913,61 @@ describe("McpHub", () => {
expect(writtenConfig.mcpServers["test-server"].alwaysAllow).toBeDefined()
expect(writtenConfig.mcpServers["test-server"].alwaysAllow).toContain("new-tool")
})
it("should preserve JSON pretty-print formatting 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 with indent parameter
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
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", () => {

View file

@ -477,4 +477,94 @@ describe("safeWriteJson", () => {
consoleErrorSpy.mockRestore()
})
// Tests for pretty-printing functionality
test("should write pretty-printed JSON with numeric indent parameter", async () => {
const data = { mcpServers: { test: { command: "node", args: ["test.js"] } } }
await safeWriteJson(currentTestFilePath, data, 2)
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 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 () => {
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, 2)
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 with indent parameter", async () => {
await safeWriteJson(currentTestFilePath, undefined, 2)
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")
})
})

View file

@ -15,10 +15,11 @@ import Stringer from "stream-json/Stringer"
*
* @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<void>}
*/
async function safeWriteJson(filePath: string, data: any): Promise<void> {
async function safeWriteJson(filePath: string, data: any, indent?: number | string): Promise<void> {
const absoluteFilePath = path.resolve(filePath)
let releaseLock = async () => {} // Initialized to a no-op
@ -75,7 +76,7 @@ async function safeWriteJson(filePath: string, data: any): Promise<void> {
`.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`,
)
await _streamDataToFile(actualTempNewFilePath, data)
await _streamDataToFile(actualTempNewFilePath, data, indent)
// Step 2: Check if the target file exists. If so, rename it to a backup path.
try {
@ -182,13 +183,28 @@ 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.
* @param indent Optional indentation for pretty-printing.
* @returns Promise<void>
*/
async function _streamDataToFile(targetPath: string, data: any): Promise<void> {
// Stream data to avoid high memory usage for large JSON objects.
async function _streamDataToFile(targetPath: string, data: any, indent?: number | string): Promise<void> {
// 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<void>((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
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) => {