fix(config): recover a config.json that stays damaged (#692)

src/config-recovery.ts

@@ -0,0 +1,83 @@
+import fs from 'fs/promises';
+import path from 'path';
+import { existsSync } from 'fs';
+import type { ServerConfig } from './config-manager.js';
+
+/** Where a corrupt config.json was found: at start, by a write, or by the file watcher */
+export type RecoveryPhase = 'startup' | 'mutation' | 'watcher';
+
+/** The config_parse_error_recovered event */
+export interface RecoveryEvent {
+  phase: RecoveryPhase;
+  config_bytes: number | null;
+  backup_created: boolean;
+  recovered_by_other_process: boolean;
+}
+
+/**
+ * The settings still readable in a corrupt config.json: the longest beginning
+ * of it that is a JSON object once closed, so every complete top-level setting
+ * before the first error. {} when there is none.
+ */
+export function salvageSettings(text: string): ServerConfig {
+  const body = text.charCodeAt(0) === 0xfeff ? text.slice(1) : text;
+  // A readable beginning can end at each comma between top-level settings, or at
+  // the brace that closes the object (anything after it is ignored)
+  const ends: number[] = [];
+  let depth = 0;
+  let inString = false;
+  let escaped = false;
+  for (let i = 0; i < body.length; i++) {
+    const char = body[i];
+    if (inString) {
+      if (escaped) escaped = false;
+      else if (char === '\\') escaped = true;
+      else if (char === '"') inString = false;
+    } else if (char === '"') {
+      inString = true;
+    } else if (char === '{' || char === '[') {
+      depth++;
+    } else if (char === '}' || char === ']') {
+      if (--depth === 0) { ends.push(i); break; }
+    } else if (char === ',' && depth === 1) {
+      ends.push(i);
+    }
+  }
+  for (const end of ends.reverse()) {
+    try {
+      return JSON.parse(body.slice(0, end) + '}');
+    } catch {
+      // Not valid JSON up to here either, so try a shorter beginning
+    }
+  }
+  return {};
+}
+
+/**
+ * What a corrupt config.json is recovered as: the defaults, then the settings
+ * last read (while running), then the settings salvaged from it. The welcome
+ * page stays off, as for any existing install.
+ */
+export function buildRecoveredConfig(defaults: ServerConfig, lastRead: ServerConfig | null, text: string): ServerConfig {
+  const { version: _version, ...settings } = lastRead ?? {};
+  return { ...defaults, ...settings, ...salvageSettings(text), welcomeOnboardingEligible: false, pendingWelcomeOnboarding: false };
+}
+
+/**
+ * Backs up a corrupt config.json as config.json.corrupt.<ms>.<pid>, unless the
+ * newest backup already holds `bytes` (a recovery tried again after its write
+ * failed). Returns the backup's name, or null when there is no config.json.
+ */
+export async function backupCorruptConfig(configPath: string, bytes: Buffer): Promise<string | null> {
+  if (!existsSync(configPath)) return null;
+  const folder = path.dirname(configPath);
+  const prefix = `${path.basename(configPath)}.corrupt.`;
+  const newest = (await fs.readdir(folder).catch(() => [] as string[]))
+    .filter((name) => name.startsWith(prefix))
+    .sort((a, b) => parseInt(a.slice(prefix.length), 10) - parseInt(b.slice(prefix.length), 10))
+    .pop();
+  if (newest && (await fs.readFile(path.join(folder, newest)).catch(() => null))?.equals(bytes)) return newest;
+  const name = `${prefix}${Date.now()}.${process.pid}`;
+  await fs.copyFile(configPath, path.join(folder, name));
+  return name;
+}

src/tools/config.ts

@@ -188,7 +188,8 @@ export async function setConfigValue(args: unknown) {
         valueToStore = numeric;
       }
 
-      await configManager.setValue(parsed.data.key, valueToStore);
+      // Not saved: in effect all the same, as the answer below says, and saved with the held changes
+      await configManager.setValue(parsed.data.key, valueToStore, { holdIfNotSaved: true });
       // Get the updated configuration to show the user
       const updatedConfig = await configManager.getConfig();
       console.error(`setConfigValue: Successfully set ${parsed.data.key} to ${JSON.stringify(valueToStore)}`);

src/utils/atomic-write.ts

@@ -24,6 +24,11 @@ const pendingWrites = new Map<string, Promise<void>>();
 export interface AtomicWriteOptions {
   encoding?: BufferEncoding;
   mode?: number;
+  /**
+   * A last check once the new content is on disk, before it replaces the file:
+   * if it throws, nothing is replaced and the write rejects with its error
+   */
+  beforeCommit?: () => void | Promise<void>;
 }
 
 async function writeThroughTempFile(filePath: string, data: string | Uint8Array, options: AtomicWriteOptions): Promise<void> {
@@ -36,6 +41,7 @@ async function writeThroughTempFile(filePath: string, data: string | Uint8Array,
     } finally {
       await handle.close();
     }
+    await options.beforeCommit?.();
     await renameWithRetry(tempPath, filePath);
   } finally {
     // After a successful rename the temp file is gone (ENOENT); after a failed

test/helpers/config-child.js

@@ -0,0 +1,35 @@
+import path from 'path';
+import { spawnSync } from 'child_process';
+import { fileURLToPath, pathToFileURL } from 'url';
+
+const DIST = pathToFileURL(path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..', 'dist')).href;
+
+/**
+ * Loads the built config manager (dist/config-manager.js) in a child process
+ * of its own, with `env` (a test home), so a test can make the file system
+ * fail the way it needs before the config manager uses it.
+ *
+ * The script sees `fs` (fs/promises, the object the config manager calls:
+ * patch its methods in `prelude`), `fsSync` (fs), `DIST` (the dist/ folder's
+ * URL, for more imports) and, in `body`, `configManager`. `body` prints its
+ * result as the last stdout line, as JSON.
+ * Returns { status, result (that JSON, or undefined), stdout, stderr }.
+ */
+export function runConfigManagerChild(env, { prelude = '', body }) {
+  const script = `
+    import fs from 'fs/promises';
+    import fsSync from 'fs';
+    const DIST = ${JSON.stringify(DIST)};
+    ${prelude}
+    const { configManager } = await import(DIST + '/config-manager.js');
+    ${body}`;
+  const child = spawnSync(process.execPath, ['--input-type=module', '-e', script], { env, encoding: 'utf8', timeout: 60_000 });
+  const last = (child.stdout ?? '').trim().split('\n').pop();
+  let result;
+  try {
+    result = JSON.parse(last);
+  } catch {
+    // No JSON result line: the caller reports status, stdout and stderr instead
+  }
+  return { status: child.status, result, stdout: child.stdout ?? '', stderr: child.stderr ?? '' };
+}

test/helpers/mcp-server.js

@@ -0,0 +1,44 @@
+import path from 'path';
+import { fileURLToPath } from 'url';
+import { Client } from '@modelcontextprotocol/sdk/client/index.js';
+import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js';
+import { LoggingMessageNotificationSchema } from '@modelcontextprotocol/sdk/types.js';
+import { closeClient } from './close-client.js';
+
+const SERVER = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..', 'dist', 'index.js');
+
+/**
+ * Starts the built server (dist/index.js) the way `desktop-commander remote`
+ * starts its local MCP: client "desktop-commander-client", DC_REMOTE_DEVICE=true.
+ * Resolves once `initialize` has succeeded; rejects with the client's error
+ * otherwise (e.g. `MCP error -32603: …`, which `remote` reports as
+ * "Device startup failed").
+ *
+ * The result collects what the server reports: `logs` (its console output,
+ * which reaches the client as log notifications: `{ level, data }`) and
+ * `stderr`. Call `close()` when done.
+ */
+export async function startServerLikeRemote(env, { timeout = 30_000 } = {}) {
+  const transport = new StdioClientTransport({
+    command: process.execPath,
+    args: [SERVER],
+    env: { ...env, DC_REMOTE_DEVICE: 'true' },
+    stderr: 'pipe',
+  });
+  const client = new Client({ name: 'desktop-commander-client', version: '1.0.0' }, { capabilities: {} });
+  const server = {
+    client,
+    logs: [],
+    stderr: '',
+    close: () => closeClient(client),
+  };
+  transport.stderr?.on('data', (chunk) => { server.stderr += chunk; });
+  client.setNotificationHandler(LoggingMessageNotificationSchema, (notification) => { server.logs.push(notification.params); });
+  try {
+    await client.connect(transport, { timeout });
+  } catch (error) {
+    await server.close();
+    throw error;
+  }
+  return server;
+}

test/repro/test-config-damaged-start.js

@@ -0,0 +1,109 @@
+// Repro (#692): with a damaged ~/.claude-server-commander/config.json,
+// `desktop-commander remote` fails every start with `MCP error -32603`, and
+// keeps failing until the user moves the file aside.
+//
+// Reports: macOS 0.2.50, a truncated file: "Unexpected end of JSON input".
+// Windows 0.2.51 (#697 comments), no other instance running:
+// "Unexpected token '', ""... is not valid JSON". Node quotes the file's first
+// 10 characters there, all invisible: a file of NUL bytes, as a crash
+// mid-write leaves it (a BOM would show the `{` after it).
+//
+// Mechanism: init() cannot parse the file (after retrying it for 1 s), logs it
+// and carries on with in-memory defaults; the file stays as it is. Those
+// defaults say "new install, welcome page pending", so `initialize` clears the
+// flag with setValue, whose mutation reads the same file again and throws: the
+// client gets -32603 and `remote` shuts the device down. Every client fails the
+// same way, not only `remote`.
+//
+// Measured on 4715bd4: 10 of 10 starts failed on Windows 11 and 10 of 10 on
+// macOS 26, each after ~3.4-3.8 s (the 1 s read wait, twice), for all five
+// kinds of damage below; config.json was never changed.
+//
+// Here each kind of damage is written to config.json in the temporary home,
+// and the current build is started the way `remote` starts its local MCP
+// (client "desktop-commander-client", DC_REMOTE_DEVICE=true), then asked for
+// get_config. After each start the file is compared with what was written.
+//
+// Run: node test/repro/run-repro.js test-config-damaged-start.js
+//      (REPRO_RUNS=2 starts per kind of damage by default)
+// Exit code: 1 if any start failed.
+import fs from 'fs';
+import os from 'os';
+import path from 'path';
+import { isTestHome } from '../helpers/test-env.js';
+import { startServerLikeRemote } from '../helpers/mcp-server.js';
+import { exitProcess } from '../../dist/utils/exit-process.js';
+
+const RUNS = Number(process.env.REPRO_RUNS || 2);
+
+// This replaces config.json, so never in a real home
+if (!isTestHome()) {
+  console.error('Run it through the repro runner: node test/repro/run-repro.js test-config-damaged-start.js');
+  exitProcess(2);
+}
+
+const configDir = path.join(os.homedir(), '.claude-server-commander');
+const configPath = path.join(configDir, 'config.json');
+
+const saved = JSON.stringify({
+  blockedCommands: ['rm'],
+  allowedDirectories: [os.tmpdir()],
+  telemetryEnabled: false,
+  pendingWelcomeOnboarding: false,
+  welcomeOnboardingEligible: false,
+}, null, 2);
+
+const DAMAGE = {
+  // The issue's own minimal reproduction
+  truncated: Buffer.from('{"defaultShell":'),
+  empty: Buffer.alloc(0),
+  // A crash mid-write can leave the file's length with no data behind it
+  nul: Buffer.alloc(saved.length),
+  // An editor saving it as "UTF-8 with BOM" (Notepad, PowerShell 5 Set-Content -Encoding UTF8)
+  bom: Buffer.concat([Buffer.from([0xef, 0xbb, 0xbf]), Buffer.from(saved)]),
+  // A hand edit adding a Windows folder without doubling its backslashes
+  handEdit: Buffer.from(saved.replace(JSON.stringify(os.tmpdir()), '"C:\\Users\\me\\projects"')),
+};
+
+async function start() {
+  const started = Date.now();
+  let server;
+  let error = '';
+  try {
+    server = await startServerLikeRemote(process.env);
+    const result = await server.client.callTool({ name: 'get_config', arguments: {} });
+    if (result.isError) error = `get_config: ${result.content?.[0]?.text}`;
+  } catch (e) {
+    error = e?.message ?? String(e);
+  } finally {
+    await server?.close();
+  }
+  return { error, ms: Date.now() - started };
+}
+
+async function runRepro() {
+  let failed = 0;
+  let total = 0;
+  for (const [kind, bytes] of Object.entries(DAMAGE)) {
+    fs.rmSync(configDir, { recursive: true, force: true });
+    fs.mkdirSync(configDir, { recursive: true });
+    fs.writeFileSync(configPath, bytes);
+    for (let run = 1; run <= RUNS; run++) {
+      total++;
+      const { error, ms } = await start();
+      if (error) failed++;
+      const after = fs.existsSync(configPath) ? fs.readFileSync(configPath) : null;
+      const file = after === null ? 'gone' : after.equals(bytes) ? 'unchanged' : `rewritten (${after.length} bytes)`;
+      const others = fs.readdirSync(configDir).filter((name) => name !== 'config.json');
+      console.log(`${kind} start ${run}: ${error ? `FAILED after ${ms} ms: ${JSON.stringify(error)}` : `ok in ${ms} ms`}; config.json ${file}${others.length ? `; also in the folder: ${others.join(', ')}` : ''}`);
+    }
+  }
+
+  console.log(failed > 0
+    ? `REPRODUCED: ${failed} of ${total} starts with a damaged config.json failed`
+    : `NOT REPRODUCED: ${total} starts with a damaged config.json, none failed`);
+  exitProcess(failed > 0 ? 1 : 0);
+}
+
+// Only in a test home: outside one, the check above refused to run
+if (isTestHome()) await runRepro();