[fix] Harden managed configuration writes

src/config-writer.spec.ts

@@ -1,4 +1,4 @@
-import { lstatSync, mkdtempSync, readFileSync, readlinkSync, symlinkSync, writeFileSync } from 'node:fs'
+import { existsSync, lstatSync, mkdtempSync, readFileSync, readlinkSync, symlinkSync, writeFileSync } from 'node:fs'
 import { join } from 'node:path'
 import { tmpdir } from 'node:os'
 import { describe, it, expect, beforeEach } from 'vitest'
@@ -333,4 +333,48 @@ describe('writeConfig', () => {
     expect(mcp).toHaveLength(3)
     expect(mcp.map((e: { serverName: string }) => e.serverName)).toEqual(['server_a', 'server_b', 'server_c'])
   })
+
+  it('preserves malformed mcp-config.json and does not write mdm-config.json', () => {
+    const mcpPath = join(outputDir, 'mcp-config.json')
+    const original = '{"serverName":'
+    writeFileSync(mcpPath, original)
+
+    expect(() =>
+      writeConfig({
+        serverName: 'glean_default',
+        serverUrl: 'https://example.com/mcp/default',
+        autoUpdate: false,
+        binaryUrlPrefix: 'https://example.com/binaries',
+        outputDir,
+      }),
+    ).toThrow()
+
+    expect(readFileSync(mcpPath, 'utf-8')).toBe(original)
+    expect(existsSync(join(outputDir, 'mdm-config.json'))).toBe(false)
+  })
+
+  it('preserves unknown fields when appending an MCP server', () => {
+    const mcpPath = join(outputDir, 'mcp-config.json')
+    writeFileSync(
+      mcpPath,
+      JSON.stringify([
+        {
+          serverName: 'existing',
+          url: 'https://existing.example.com/mcp',
+          futureField: { enabled: true },
+        },
+      ]),
+    )
+
+    writeConfig({
+      serverName: 'glean_default',
+      serverUrl: 'https://example.com/mcp/default',
+      autoUpdate: false,
+      binaryUrlPrefix: 'https://example.com/binaries',
+      outputDir,
+    })
+
+    const mcp = JSON.parse(readFileSync(mcpPath, 'utf-8'))
+    expect(mcp[0].futureField).toEqual({ enabled: true })
+  })
 })

src/config-writer.ts

@@ -1,8 +1,9 @@
-import { mkdirSync, readFileSync, realpathSync, renameSync, writeFileSync } from 'node:fs'
+import { mkdirSync } from 'node:fs'
 import { join } from 'node:path'
 
 import { McpConfigSchema, type McpServerEntry, MdmConfigSchema } from './config.js'
 import { log } from './logger.js'
+import { atomicWriteFile, readTextFileIfExists, resolveWritePath, withFileLock } from './managed-file.js'
 import { getDefaultConfigDir } from './platform.js'
 
 export interface WriteConfigOptions {
@@ -15,24 +16,9 @@ export interface WriteConfigOptions {
   outputDir?: string
 }
 
-function resolveWritePath(filePath: string): string {
-  try {
-    return realpathSync(filePath)
-  } catch {
-    return filePath
-  }
-}
-
 function readExistingMcpEntries(filePath: string): McpServerEntry[] {
-  let raw: string
-  try {
-    raw = readFileSync(filePath, 'utf-8')
-  } catch (err: any) {
-    if (err.code === 'ENOENT') {
-      return []
-    }
-    throw err
-  }
+  const raw = readTextFileIfExists(filePath)
+  if (raw === undefined) return []
   const json = JSON.parse(raw)
   return McpConfigSchema.parse(json)
 }
@@ -53,28 +39,28 @@ export function writeConfig(options: WriteConfigOptions): void {
   const parsedMdm = MdmConfigSchema.parse(mdmData)
 
   const mcpPath = join(outputDir, 'mcp-config.json')
-  const existingEntries = readExistingMcpEntries(mcpPath)
-  const nameMatch = existingEntries.find((e) => e.serverName === newEntry.serverName)
-  const urlMatch = existingEntries.find((e) => e.url === newEntry.url)
+  const mcpWritePath = resolveWritePath(mcpPath)
+  withFileLock(mcpWritePath, () => {
+    const existingEntries = readExistingMcpEntries(mcpWritePath)
+    const nameMatch = existingEntries.find((e) => e.serverName === newEntry.serverName)
+    const urlMatch = existingEntries.find((e) => e.url === newEntry.url)
 
-  if (nameMatch) {
-    log.info(`Skipped ${mcpPath} (entry "${newEntry.serverName}" already exists)`)
-  } else if (urlMatch) {
-    log.info(`Skipped ${mcpPath} (URL "${newEntry.url}" already configured under "${urlMatch.serverName}")`)
-  } else {
-    const merged = [...existingEntries, newEntry]
-    const mcpWritePath = resolveWritePath(mcpPath)
-    const mcpTmp = `${mcpWritePath}.tmp`
-    writeFileSync(mcpTmp, JSON.stringify(merged, null, 2) + '\n')
-    renameSync(mcpTmp, mcpWritePath)
-    log.info(`Added entry "${newEntry.serverName}" to ${mcpPath}`)
-  }
+    if (nameMatch) {
+      log.info(`Skipped ${mcpPath} (entry "${newEntry.serverName}" already exists)`)
+    } else if (urlMatch) {
+      log.info(`Skipped ${mcpPath} (URL "${newEntry.url}" already configured under "${urlMatch.serverName}")`)
+    } else {
+      const merged = [...existingEntries, newEntry]
+      atomicWriteFile(mcpWritePath, JSON.stringify(merged, null, 2) + '\n')
+      log.info(`Added entry "${newEntry.serverName}" to ${mcpPath}`)
+    }
+  })
 
   const mdmPath = join(outputDir, 'mdm-config.json')
   const mdmWritePath = resolveWritePath(mdmPath)
-  const mdmTmp = `${mdmWritePath}.tmp`
-  writeFileSync(mdmTmp, JSON.stringify(parsedMdm, null, 2) + '\n')
-  renameSync(mdmTmp, mdmWritePath)
+  withFileLock(mdmWritePath, () => {
+    atomicWriteFile(mdmWritePath, JSON.stringify(parsedMdm, null, 2) + '\n')
+  })
 
   log.info(`Wrote ${mdmPath}`)
 }

src/config.spec.ts

@@ -53,6 +53,7 @@ describe('McpConfigSchema', () => {
     })
 
     expect(result.success).toBe(true)
+    if (result.success) expect(result.data[0].someNewField).toBe('future-value')
   })
 })
 
@@ -76,6 +77,7 @@ describe('MdmConfigSchema', () => {
     })
 
     expect(result.success).toBe(true)
+    if (result.success) expect(result.data.someNewField).toBe('future-value')
   })
 
   describe('autoUpdate', () => {

src/config.ts

@@ -9,7 +9,7 @@ const SEMVER_PATTERN = /^v?\d+\.\d+\.\d+$/
 const McpServerEntrySchema = z.object({
   serverName: z.string().min(1),
   url: z.string().min(1),
-})
+}).passthrough()
 
 export const McpConfigSchema = z
   .union([z.array(McpServerEntrySchema).min(1), McpServerEntrySchema])
@@ -28,6 +28,7 @@ export const MdmConfigSchema = z
       z.string().url(),
     ),
   })
+  .passthrough()
   .refine((data) => !data.autoUpdate || data.versionUrl !== undefined, {
     message: 'versionUrl is required when autoUpdate is true',
     path: ['versionUrl'],

src/hosts/json-configurator.ts

@@ -1,40 +1,9 @@
-import { mkdirSync, readFileSync, renameSync, writeFileSync } from 'node:fs'
-import { dirname } from 'node:path'
-
-import { log } from '../logger.js'
-
-import { isPlainObject, resolveWritePath, withoutDuplicateUrls } from './utils.js'
-
-export interface ConfigureFileOptions {
-  configToMerge: Record<string, unknown>
-  filePath: string
-}
+import { type ConfigureFileOptions, configureManagedFile } from './managed-config.js'
 
 export function configureJsonFile(options: ConfigureFileOptions): void {
-  const { configToMerge, filePath } = options
-
-  let existing: Record<string, unknown> = {}
-  try {
-    existing = JSON.parse(readFileSync(filePath, 'utf-8'))
-  } catch {
-    // File doesn't exist or isn't valid JSON — start fresh
-  }
-
-  for (const [key, value] of Object.entries(configToMerge)) {
-    if (isPlainObject(value)) {
-      const existingSection = isPlainObject(existing[key]) ? existing[key] : {}
-      const filtered = withoutDuplicateUrls(existingSection, value)
-      existing[key] = { ...existingSection, ...filtered }
-    } else {
-      existing[key] = value
-    }
-  }
-
-  const writePath = resolveWritePath(filePath)
-  mkdirSync(dirname(writePath), { recursive: true })
-  const tmpPath = `${writePath}.tmp`
-  writeFileSync(tmpPath, JSON.stringify(existing, null, 2) + '\n')
-  renameSync(tmpPath, writePath)
-
-  log.info(`Configured JSON: ${filePath}`)
+  configureManagedFile(options, {
+    format: 'JSON',
+    parse: JSON.parse,
+    serialize: (config) => JSON.stringify(config, null, 2) + '\n',
+  })
 }

src/hosts/managed-config.spec.ts

@@ -0,0 +1,78 @@
+import { mkdirSync, mkdtempSync, readFileSync, readdirSync, writeFileSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+
+import { beforeEach, describe, expect, it } from 'vitest'
+
+import { configureJsonFile } from './json-configurator'
+import { configureTomlFile } from './toml-configurator'
+import { configureYamlFile } from './yaml-configurator'
+
+const configToMerge = {
+  mcpServers: {
+    glean_default: {
+      type: 'http',
+      url: 'https://example-be.glean.com/mcp/default',
+    },
+  },
+}
+
+const formats = [
+  {
+    configure: configureJsonFile,
+    extension: 'json',
+    format: 'JSON',
+    invalidContent: '{"mcpServers":',
+    nonObjectContent: '[]',
+  },
+  {
+    configure: configureTomlFile,
+    extension: 'toml',
+    format: 'TOML',
+    invalidContent: 'mcp_servers = [',
+  },
+  {
+    configure: configureYamlFile,
+    extension: 'yaml',
+    format: 'YAML',
+    invalidContent: 'mcpServers: [unterminated',
+    nonObjectContent: '- item\n',
+  },
+]
+
+describe.each(formats)('$format managed configuration', (formatCase) => {
+  let tempDir: string
+  let filePath: string
+
+  beforeEach(() => {
+    tempDir = mkdtempSync(join(tmpdir(), `managed-${formatCase.extension}-test-`))
+    filePath = join(tempDir, `config.${formatCase.extension}`)
+  })
+
+  it('preserves malformed files byte-for-byte', () => {
+    writeFileSync(filePath, formatCase.invalidContent)
+
+    expect(() => formatCase.configure({ configToMerge, filePath })).toThrow(
+      `Cannot update invalid ${formatCase.format} configuration`,
+    )
+
+    expect(readFileSync(filePath, 'utf-8')).toBe(formatCase.invalidContent)
+    expect(readdirSync(tempDir)).toEqual([`config.${formatCase.extension}`])
+  })
+
+  it('does not treat read errors as missing files', () => {
+    mkdirSync(filePath)
+
+    expect(() => formatCase.configure({ configToMerge, filePath })).toThrow()
+    expect(readdirSync(tempDir)).toEqual([`config.${formatCase.extension}`])
+  })
+
+  if (formatCase.nonObjectContent) {
+    it('preserves files whose root value is not an object', () => {
+      writeFileSync(filePath, formatCase.nonObjectContent)
+
+      expect(() => formatCase.configure({ configToMerge, filePath })).toThrow(/root value must be an object/)
+      expect(readFileSync(filePath, 'utf-8')).toBe(formatCase.nonObjectContent)
+    })
+  }
+})

src/hosts/managed-config.ts

@@ -0,0 +1,69 @@
+import { log } from '../logger.js'
+import { atomicWriteFile, readTextFileIfExists, resolveWritePath, withFileLock } from '../managed-file.js'
+
+import { isPlainObject, withoutDuplicateUrls } from './utils.js'
+
+export interface ConfigureFileOptions {
+  configToMerge: Record<string, unknown>
+  filePath: string
+}
+
+export interface ManagedConfigCodec {
+  format: string
+  parse: (content: string) => unknown
+  serialize: (config: Record<string, unknown>) => string
+}
+
+function describeError(error: unknown): string {
+  return error instanceof Error ? error.message : String(error)
+}
+
+function mergeManagedConfig(
+  existing: Record<string, unknown>,
+  configToMerge: Record<string, unknown>,
+): Record<string, unknown> {
+  const merged = { ...existing }
+
+  for (const [key, value] of Object.entries(configToMerge)) {
+    if (isPlainObject(value)) {
+      const existingSection = isPlainObject(merged[key]) ? merged[key] : {}
+      const filtered = withoutDuplicateUrls(existingSection, value)
+      merged[key] = { ...existingSection, ...filtered }
+    } else {
+      merged[key] = value
+    }
+  }
+
+  return merged
+}
+
+export function configureManagedFile(options: ConfigureFileOptions, codec: ManagedConfigCodec): void {
+  const { configToMerge, filePath } = options
+  const writePath = resolveWritePath(filePath)
+
+  withFileLock(writePath, () => {
+    const content = readTextFileIfExists(writePath)
+    let existing: Record<string, unknown> = {}
+
+    if (content !== undefined) {
+      let parsed: unknown
+      try {
+        parsed = codec.parse(content)
+      } catch (error) {
+        throw new Error(`Cannot update invalid ${codec.format} configuration at ${filePath}: ${describeError(error)}`, {
+          cause: error,
+        })
+      }
+
+      if (!isPlainObject(parsed)) {
+        throw new Error(`Cannot update ${codec.format} configuration at ${filePath}: root value must be an object`)
+      }
+      existing = parsed
+    }
+
+    const merged = mergeManagedConfig(existing, configToMerge)
+    atomicWriteFile(writePath, codec.serialize(merged))
+  })
+
+  log.info(`Configured ${codec.format}: ${filePath}`)
+}