Merge pull request #2037 from QwenLM/mingholy/refactor/settings-migration

Refactor settings migration to sequential framework with atomic file writes

packages/cli/src/utils/settingsUtils.ts

@@ -129,6 +129,13 @@ export function getNestedValue(
   return undefined;
 }
 
+export function getNestedProperty(
+  obj: Record<string, unknown>,
+  path: string,
+): unknown {
+  return getNestedValue(obj, path.split('.'));
+}
+
 /**
  * Get the effective value for a setting, considering inheritance from higher scopes
  * Always returns a value (never undefined) - falls back to default if not set anywhere
@@ -382,30 +389,69 @@ export function settingExistsInScope(
   return value !== undefined;
 }
 
-/**
- * Recursively sets a value in a nested object using a key path array.
- */
-function setNestedValue(
+export function setNestedPropertyForce(
   obj: Record<string, unknown>,
-  path: string[],
+  path: string,
   value: unknown,
-): Record<string, unknown> {
-  const [first, ...rest] = path;
-  if (!first) {
-    return obj;
+): void {
+  const keys = path.split('.');
+  const lastKey = keys.pop();
+  if (!lastKey) return;
+
+  let current: Record<string, unknown> = obj;
+  for (const key of keys) {
+    if (!current[key] || typeof current[key] !== 'object') {
+      current[key] = {};
+    }
+    current = current[key] as Record<string, unknown>;
   }
 
-  if (rest.length === 0) {
-    obj[first] = value;
-    return obj;
+  current[lastKey] = value;
+}
+
+export function setNestedPropertySafe(
+  obj: Record<string, unknown>,
+  path: string,
+  value: unknown,
+): void {
+  const keys = path.split('.');
+  const lastKey = keys.pop();
+  if (!lastKey) return;
+
+  let current: Record<string, unknown> = obj;
+  for (const key of keys) {
+    if (current[key] === undefined) {
+      current[key] = {};
+    }
+    const next = current[key];
+    if (typeof next === 'object' && next !== null) {
+      current = next as Record<string, unknown>;
+    } else {
+      return;
+    }
   }
 
-  if (!obj[first] || typeof obj[first] !== 'object') {
-    obj[first] = {};
+  current[lastKey] = value;
+}
+
+export function deleteNestedPropertySafe(
+  obj: Record<string, unknown>,
+  path: string,
+): void {
+  const keys = path.split('.');
+  const lastKey = keys.pop();
+  if (!lastKey) return;
+
+  let current: Record<string, unknown> = obj;
+  for (const key of keys) {
+    const next = current[key];
+    if (typeof next !== 'object' || next === null) {
+      return;
+    }
+    current = next as Record<string, unknown>;
   }
 
-  setNestedValue(obj[first] as Record<string, unknown>, rest, value);
-  return obj;
+  delete current[lastKey];
 }
 
 /**
@@ -416,9 +462,8 @@ export function setPendingSettingValue(
   value: boolean,
   pendingSettings: Settings,
 ): Settings {
-  const path = key.split('.');
   const newSettings = JSON.parse(JSON.stringify(pendingSettings));
-  setNestedValue(newSettings, path, value);
+  setNestedPropertyForce(newSettings, key, value);
   return newSettings;
 }
 
@@ -430,9 +475,8 @@ export function setPendingSettingValueAny(
   value: SettingsValue,
   pendingSettings: Settings,
 ): Settings {
-  const path = key.split('.');
   const newSettings = structuredClone(pendingSettings);
-  setNestedValue(newSettings, path, value);
+  setNestedPropertyForce(newSettings, key, value);
   return newSettings;
 }
 

packages/cli/src/utils/writeWithBackup.test.ts

@@ -0,0 +1,232 @@
+/**
+ * @license
+ * Copyright 2025 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import * as fs from 'node:fs';
+import * as path from 'node:path';
+import * as os from 'node:os';
+import { writeWithBackup, writeWithBackupSync } from './writeWithBackup.js';
+
+describe('writeWithBackup', () => {
+  let tempDir: string;
+
+  beforeEach(() => {
+    tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'writeWithBackup-test-'));
+  });
+
+  afterEach(() => {
+    // Clean up temp directory
+    try {
+      fs.rmSync(tempDir, { recursive: true, force: true });
+    } catch (_e) {
+      // Ignore cleanup errors
+    }
+  });
+
+  describe('writeWithBackupSync', () => {
+    it('should write content to a new file', () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+      const content = 'Hello, World!';
+
+      writeWithBackupSync(targetPath, content);
+
+      expect(fs.existsSync(targetPath)).toBe(true);
+      expect(fs.readFileSync(targetPath, 'utf-8')).toBe(content);
+    });
+
+    it('should backup existing file before writing', () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+      const originalContent = 'Original content';
+      const newContent = 'New content';
+
+      fs.writeFileSync(targetPath, originalContent);
+      writeWithBackupSync(targetPath, newContent);
+
+      expect(fs.readFileSync(targetPath, 'utf-8')).toBe(newContent);
+      expect(fs.existsSync(`${targetPath}.orig`)).toBe(true);
+      expect(fs.readFileSync(`${targetPath}.orig`, 'utf-8')).toBe(
+        originalContent,
+      );
+    });
+
+    it('should use custom backup suffix', () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+      const originalContent = 'Original';
+
+      fs.writeFileSync(targetPath, originalContent);
+      writeWithBackupSync(targetPath, 'New', { backupSuffix: '.bak' });
+
+      expect(fs.existsSync(`${targetPath}.bak`)).toBe(true);
+      expect(fs.existsSync(`${targetPath}.orig`)).toBe(false);
+    });
+
+    it('should clean up temp file on failure', () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+      const tempPath = `${targetPath}.tmp`;
+
+      // Create a situation where rename will fail (e.g., by creating a directory at target)
+      fs.mkdirSync(targetPath);
+
+      expect(() => writeWithBackupSync(targetPath, 'content')).toThrow();
+      expect(fs.existsSync(tempPath)).toBe(false);
+    });
+
+    it('should preserve original file content when write fails after backup', () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+      const originalContent = 'Original content that must be preserved';
+
+      // Create original file
+      fs.writeFileSync(targetPath, originalContent);
+
+      // Create a situation where rename will fail (by creating a directory at temp path)
+      const tempPath = `${targetPath}.tmp`;
+      fs.mkdirSync(tempPath);
+
+      // The write should fail
+      expect(() => writeWithBackupSync(targetPath, 'New content')).toThrow();
+
+      // Original file should still exist with original content
+      expect(fs.existsSync(targetPath)).toBe(true);
+      expect(fs.statSync(targetPath).isFile()).toBe(true);
+      expect(fs.readFileSync(targetPath, 'utf-8')).toBe(originalContent);
+
+      // Cleanup
+      fs.rmdirSync(tempPath);
+    });
+
+    it('should restore original file from backup when rename fails', () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+      const backupPath = `${targetPath}.orig`;
+      const originalContent = 'Original content';
+      const newContent = 'New content';
+
+      // Create original file
+      fs.writeFileSync(targetPath, originalContent);
+
+      // Write new content successfully first
+      writeWithBackupSync(targetPath, newContent);
+
+      // Verify backup exists with original content
+      expect(fs.existsSync(backupPath)).toBe(true);
+      expect(fs.readFileSync(backupPath, 'utf-8')).toBe(originalContent);
+
+      // Verify target has new content
+      expect(fs.readFileSync(targetPath, 'utf-8')).toBe(newContent);
+
+      // Now simulate a failure scenario: delete target and try to restore from backup
+      fs.unlinkSync(targetPath);
+
+      // Restore from backup manually to verify backup integrity
+      fs.copyFileSync(backupPath, targetPath);
+      expect(fs.readFileSync(targetPath, 'utf-8')).toBe(originalContent);
+    });
+
+    it('should include recovery information in error message', () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+
+      // Create a situation where rename will fail (directory at target)
+      fs.mkdirSync(targetPath);
+
+      let errorMessage = '';
+      try {
+        writeWithBackupSync(targetPath, 'content');
+      } catch (error) {
+        errorMessage = error instanceof Error ? error.message : String(error);
+      }
+
+      // Error message should be descriptive
+      expect(errorMessage).toContain('directory');
+      expect(errorMessage.length).toBeGreaterThan(10);
+    });
+
+    it('should handle backup failure with descriptive error', () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+      const backupPath = `${targetPath}.orig`;
+      const originalContent = 'Original content';
+
+      // Create original file
+      fs.writeFileSync(targetPath, originalContent);
+
+      // Create a directory at backup path to cause backup to fail
+      fs.mkdirSync(backupPath);
+
+      let errorMessage = '';
+      try {
+        writeWithBackupSync(targetPath, 'New content');
+      } catch (error) {
+        errorMessage = error instanceof Error ? error.message : String(error);
+      }
+
+      // Error message should mention backup failure
+      expect(errorMessage).toContain('backup');
+
+      // Original file should still exist
+      expect(fs.existsSync(targetPath)).toBe(true);
+      expect(fs.readFileSync(targetPath, 'utf-8')).toBe(originalContent);
+
+      // Cleanup
+      fs.rmdirSync(backupPath);
+    });
+
+    it('should clean up temp file when backup creation fails', () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+      const tempPath = `${targetPath}.tmp`;
+      const backupPath = `${targetPath}.orig`;
+      const originalContent = 'Original content';
+
+      // Create original file
+      fs.writeFileSync(targetPath, originalContent);
+
+      // Create a directory at backup path to cause backup to fail
+      fs.mkdirSync(backupPath);
+
+      // The write should fail
+      expect(() => writeWithBackupSync(targetPath, 'New content')).toThrow();
+
+      // Temp file should be cleaned up
+      expect(fs.existsSync(tempPath)).toBe(false);
+
+      // Cleanup
+      fs.rmdirSync(backupPath);
+    });
+  });
+
+  describe('writeWithBackup (async)', () => {
+    it('should write content to a new file', async () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+      const content = 'Hello, World!';
+
+      await writeWithBackup(targetPath, content);
+
+      expect(fs.existsSync(targetPath)).toBe(true);
+      expect(fs.readFileSync(targetPath, 'utf-8')).toBe(content);
+    });
+
+    it('should backup existing file before writing', async () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+      const originalContent = 'Original content';
+      const newContent = 'New content';
+
+      fs.writeFileSync(targetPath, originalContent);
+      await writeWithBackup(targetPath, newContent);
+
+      expect(fs.readFileSync(targetPath, 'utf-8')).toBe(newContent);
+      expect(fs.existsSync(`${targetPath}.orig`)).toBe(true);
+      expect(fs.readFileSync(`${targetPath}.orig`, 'utf-8')).toBe(
+        originalContent,
+      );
+    });
+
+    it('should use custom encoding', async () => {
+      const targetPath = path.join(tempDir, 'test-file.txt');
+      const content = 'Hello, World!';
+
+      await writeWithBackup(targetPath, content, { encoding: 'utf8' });
+
+      expect(fs.readFileSync(targetPath, 'utf-8')).toBe(content);
+    });
+  });
+});

packages/cli/src/utils/writeWithBackup.ts

@@ -0,0 +1,169 @@
+/**
+ * @license
+ * Copyright 2025 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import * as fs from 'node:fs';
+
+/**
+ * Options for writeWithBackup function.
+ */
+export interface WriteWithBackupOptions {
+  /** Suffix for backup file (default: '.orig') */
+  backupSuffix?: string;
+  /** File encoding (default: 'utf-8') */
+  encoding?: BufferEncoding;
+}
+
+/**
+ * Safely writes content to a file with backup protection.
+ *
+ * This function ensures data safety by:
+ * 1. Writing content to a temporary file first
+ * 2. Backing up the existing target file (if any)
+ * 3. Renaming the temporary file to the target path
+ *
+ * If any step fails, an error is thrown and no partial changes are left on disk.
+ * The backup file (if created) can be used for manual recovery.
+ *
+ * Note: This is not 100% atomic but provides good protection. In the worst case,
+ * a .orig backup file remains that can be manually restored.
+ *
+ * @param targetPath - The path to write to
+ * @param content - The content to write
+ * @param options - Optional configuration
+ * @throws Error if any step of the write process fails
+ *
+ * @example
+ * ```typescript
+ * await writeWithBackup('/path/to/settings.json', JSON.stringify(settings, null, 2));
+ * // If /path/to/settings.json existed, it's now backed up to /path/to/settings.json.orig
+ * ```
+ */
+export async function writeWithBackup(
+  targetPath: string,
+  content: string,
+  options: WriteWithBackupOptions = {},
+): Promise<void> {
+  // Async version delegates to sync version since file operations are synchronous
+  writeWithBackupSync(targetPath, content, options);
+}
+
+/**
+ * Synchronous version of writeWithBackup.
+ *
+ * @param targetPath - The path to write to
+ * @param content - The content to write
+ * @param options - Optional configuration
+ * @throws Error if any step of the write process fails
+ */
+export function writeWithBackupSync(
+  targetPath: string,
+  content: string,
+  options: WriteWithBackupOptions = {},
+): void {
+  const { backupSuffix = '.orig', encoding = 'utf-8' } = options;
+  const tempPath = `${targetPath}.tmp`;
+  const backupPath = `${targetPath}${backupSuffix}`;
+
+  // Clean up any existing temp file from previous failed attempts
+  try {
+    if (fs.existsSync(tempPath)) {
+      fs.unlinkSync(tempPath);
+    }
+  } catch (_e) {
+    // Ignore cleanup errors
+  }
+
+  try {
+    // Step 1: Write to temporary file
+    fs.writeFileSync(tempPath, content, { encoding });
+
+    // Step 2: If target exists, back it up
+    if (fs.existsSync(targetPath)) {
+      // Check if target is a directory - we can't write to a directory
+      const targetStat = fs.statSync(targetPath);
+      if (targetStat.isDirectory()) {
+        // Clean up temp file before throwing
+        try {
+          fs.unlinkSync(tempPath);
+        } catch (_e) {
+          // Ignore cleanup error
+        }
+        throw new Error(
+          `Cannot write to '${targetPath}' because it is a directory`,
+        );
+      }
+
+      try {
+        fs.renameSync(targetPath, backupPath);
+      } catch (backupError) {
+        // Clean up temp file before throwing
+        try {
+          fs.unlinkSync(tempPath);
+        } catch (_e) {
+          // Ignore cleanup error
+        }
+        throw new Error(
+          `Failed to backup existing file: ${backupError instanceof Error ? backupError.message : String(backupError)}`,
+        );
+      }
+    }
+
+    // Step 3: Rename temp file to target
+    try {
+      fs.renameSync(tempPath, targetPath);
+    } catch (renameError) {
+      let restoreFailedMessage: string | undefined;
+      let backupExisted = false;
+
+      // Attempt to restore backup if rename failed
+      if (fs.existsSync(backupPath)) {
+        backupExisted = true;
+        try {
+          fs.renameSync(backupPath, targetPath);
+        } catch (restoreError) {
+          restoreFailedMessage =
+            restoreError instanceof Error
+              ? restoreError.message
+              : String(restoreError);
+        }
+      }
+
+      const writeFailureMessage =
+        renameError instanceof Error
+          ? renameError.message
+          : String(renameError);
+
+      if (restoreFailedMessage) {
+        throw new Error(
+          `Failed to write file: ${writeFailureMessage}. ` +
+            `Automatic restore failed: ${restoreFailedMessage}. ` +
+            `Manual recovery may be required using backup file '${backupPath}'.`,
+        );
+      }
+
+      if (backupExisted) {
+        throw new Error(
+          `Failed to write file: ${writeFailureMessage}. ` +
+            `Target was automatically restored from backup '${backupPath}'.`,
+        );
+      }
+
+      throw new Error(
+        `Failed to write file: ${writeFailureMessage}. No backup file was available for restoration.`,
+      );
+    }
+  } catch (error) {
+    // Ensure temp file is cleaned up on any error
+    try {
+      if (fs.existsSync(tempPath)) {
+        fs.unlinkSync(tempPath);
+      }
+    } catch (_e) {
+      // Ignore cleanup error
+    }
+    throw error;
+  }
+}