fix(core): address all code review issues from PR #199

Critical fixes:
- CRIT-1: Fix placeholder detection case sensitivity in api-key-validator
- CRIT-2: Add TOCTOU protection via loadMigrationCheckData() for atomic config reads
- CRIT-3: Restore fs.existsSync mock in profile-detector tests

High priority fixes:
- H1-H3: API validator HTTP/HTTPS support, timeout abort, debug log
- H4-H6: OAuth explicit flow type, signal handling, TTY detection
- H7-H8: Profile collision warning, config-first save order
- H9-H11: expandPath consistency, preset sync CLI/UI

src/cliproxy/auth/oauth-process.ts

@@ -23,6 +23,7 @@ import { ProviderOAuthConfig } from './auth-types';
 import { getTimeoutTroubleshooting, showStep } from './environment-detector';
 import { isAuthenticated, registerAccountFromToken } from './token-manager';
 import { deviceCodeEvents, type DeviceCodePrompt } from '../device-code-handler';
+import { OAUTH_FLOW_TYPES } from '../../management';
 
 /** Options for OAuth process execution */
 export interface OAuthProcessOptions {
@@ -104,7 +105,9 @@ async function handleStdout(
   log(`stdout: ${output.trim()}`);
   state.accumulatedOutput += output;
 
-  const isDeviceCodeFlow = options.callbackPort === null;
+  // H4: Use explicit flow type from OAUTH_FLOW_TYPES instead of null port check
+  const flowType = OAUTH_FLOW_TYPES[options.provider] || 'authorization_code';
+  const isDeviceCodeFlow = flowType === 'device_code';
 
   // Parse project list when available
   if (isProjectList(state.accumulatedOutput) && state.parsedProjects.length === 0) {
@@ -258,16 +261,29 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
   };
 
   return new Promise<AccountInfo | null>((resolve) => {
-    // Device Code flows (Qwen, GHCP) may need interactive stdin for email/prompts
+    // H4: Use explicit flow type from OAUTH_FLOW_TYPES instead of null port check
+    const flowType = OAUTH_FLOW_TYPES[provider] || 'authorization_code';
+    const isDeviceCodeFlow = flowType === 'device_code';
+
+    // H6: TTY detection - only inherit stdin if TTY available (prevents issues in CI/piped scripts)
+    // Device Code flows may need interactive stdin for email/prompts
     // Authorization Code flows need piped stdin for project selection
-    const isDeviceCodeFlow = callbackPort === null;
-    const stdinMode = isDeviceCodeFlow ? 'inherit' : 'pipe';
+    const stdinMode = isDeviceCodeFlow && process.stdin.isTTY ? 'inherit' : 'pipe';
 
     const authProcess = spawn(binaryPath, args, {
       stdio: [stdinMode, 'pipe', 'pipe'],
       env: { ...process.env, CLI_PROXY_AUTH_DIR: tokenDir },
     });
 
+    // H5: Signal handling - properly kill child process on SIGINT/SIGTERM
+    const cleanup = () => {
+      if (authProcess && !authProcess.killed) {
+        authProcess.kill('SIGTERM');
+      }
+    };
+    process.on('SIGINT', cleanup);
+    process.on('SIGTERM', cleanup);
+
     const state: ProcessState = {
       stderrData: '',
       urlDisplayed: false,
@@ -328,6 +344,9 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
     // Timeout handling
     const timeoutMs = headless ? 300000 : 120000;
     const timeout = setTimeout(() => {
+      // H5: Remove signal handlers before killing process
+      process.removeListener('SIGINT', cleanup);
+      process.removeListener('SIGTERM', cleanup);
       authProcess.kill();
       console.log('');
       console.log(fail(`OAuth timed out after ${headless ? 5 : 2} minutes`));
@@ -339,6 +358,9 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
 
     authProcess.on('exit', (code) => {
       clearTimeout(timeout);
+      // H5: Remove signal handlers to prevent memory leaks
+      process.removeListener('SIGINT', cleanup);
+      process.removeListener('SIGTERM', cleanup);
       const elapsed = ((Date.now() - startTime) / 1000).toFixed(1);
 
       if (code === 0) {
@@ -380,6 +402,9 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
 
     authProcess.on('error', (error) => {
       clearTimeout(timeout);
+      // H5: Remove signal handlers to prevent memory leaks
+      process.removeListener('SIGINT', cleanup);
+      process.removeListener('SIGTERM', cleanup);
       console.log('');
       console.log(fail(`Failed to start auth process: ${error.message}`));
       resolve(null);

src/cliproxy/config-generator.ts

@@ -11,6 +11,7 @@
 import * as fs from 'fs';
 import * as path from 'path';
 import { getCcsDir } from '../utils/config-manager';
+import { expandPath } from '../utils/helpers';
 import { warn } from '../utils/ui';
 import { CLIProxyProvider, ProviderConfig, ProviderModelMapping } from './types';
 import { getModelMappingFromConfig, getEnvVarsFromConfig } from './base-config-loader';
@@ -463,7 +464,7 @@ export function getEffectiveEnvVars(
 
   // Priority 1: Custom settings path (for user-defined variants)
   if (customSettingsPath) {
-    const expandedPath = customSettingsPath.replace(/^~/, require('os').homedir());
+    const expandedPath = expandPath(customSettingsPath);
     if (fs.existsSync(expandedPath)) {
       try {
         const content = fs.readFileSync(expandedPath, 'utf-8');
@@ -569,7 +570,7 @@ export function getRemoteEnvVars(
 
   // Priority 1: Custom settings path (for user-defined variants)
   if (customSettingsPath) {
-    const expandedPath = customSettingsPath.replace(/^~/, require('os').homedir());
+    const expandedPath = expandPath(customSettingsPath);
     if (fs.existsSync(expandedPath)) {
       try {
         const content = fs.readFileSync(expandedPath, 'utf-8');

src/config/migration-manager.ts

@@ -45,6 +45,43 @@ export function needsMigration(): boolean {
   return hasOldConfig && !hasNewConfig;
 }
 
+/**
+ * Pre-loaded config data for TOCTOU-safe migration checks.
+ * Read once, use for both check and migration.
+ */
+export interface MigrationCheckData {
+  legacyConfig: Record<string, unknown> | null;
+  unifiedConfig: ReturnType<typeof loadUnifiedConfig>;
+  needsMigration: boolean;
+}
+
+/**
+ * Load config files once for TOCTOU-safe migration operations.
+ * Returns data that can be passed to migration functions.
+ */
+export function loadMigrationCheckData(): MigrationCheckData {
+  const ccsDir = getCcsDir();
+  const configJsonPath = path.join(ccsDir, 'config.json');
+
+  const legacyConfig = fs.existsSync(configJsonPath) ? readJsonSafe(configJsonPath) : null;
+  const unifiedConfig = loadUnifiedConfig();
+
+  // Determine if migration is needed
+  let needsMigration = false;
+
+  if (legacyConfig?.profiles && typeof legacyConfig.profiles === 'object' && unifiedConfig) {
+    const legacyProfiles = legacyConfig.profiles as Record<string, unknown>;
+    for (const profileName of Object.keys(legacyProfiles)) {
+      if (!unifiedConfig.profiles[profileName]) {
+        needsMigration = true;
+        break;
+      }
+    }
+  }
+
+  return { legacyConfig, unifiedConfig, needsMigration };
+}
+
 /**
  * Check if there are legacy profiles that haven't been migrated to config.yaml.
  * This catches the case where config.yaml exists but is empty/missing profiles
@@ -52,38 +89,12 @@ export function needsMigration(): boolean {
  *
  * Used by autoMigrate() to trigger inline migration when needed.
  * (Fix for issue #195 - GLM auth persistence regression)
+ *
+ * Note: For TOCTOU-safe operations, use loadMigrationCheckData() instead.
  */
 export function needsProfileMigration(): boolean {
-  const ccsDir = getCcsDir();
-  const configJsonPath = path.join(ccsDir, 'config.json');
-
-  // No legacy config → nothing to migrate
-  if (!fs.existsSync(configJsonPath)) {
-    return false;
-  }
-
-  const legacyConfig = readJsonSafe(configJsonPath);
-  const unifiedConfig = loadUnifiedConfig();
-
-  // No legacy profiles → nothing to migrate
-  if (!legacyConfig?.profiles || typeof legacyConfig.profiles !== 'object') {
-    return false;
-  }
-
-  // No unified config → needs full migration (handled by needsMigration)
-  if (!unifiedConfig) {
-    return false;
-  }
-
-  // Check if any legacy profile is missing from unified config
-  const legacyProfiles = legacyConfig.profiles as Record<string, unknown>;
-  for (const profileName of Object.keys(legacyProfiles)) {
-    if (!unifiedConfig.profiles[profileName]) {
-      return true; // Found unmigrated profile
-    }
-  }
-
-  return false;
+  const data = loadMigrationCheckData();
+  return data.needsMigration;
 }
 
 /**
@@ -209,7 +220,13 @@ export async function migrate(dryRun = false): Promise<MigrationResult> {
       }
     }
 
-    // 7. Migrate cache files
+    // 7. Write new config FIRST (before moving files - H8 fix)
+    // Note: Settings remain in *.settings.json files, config.yaml only stores references
+    if (!dryRun) {
+      saveUnifiedConfig(unifiedConfig);
+    }
+
+    // 8. Migrate cache files AFTER config is saved (H8: prevents inconsistent state)
     if (!dryRun) {
       const cacheDir = path.join(ccsDir, 'cache');
       if (!fs.existsSync(cacheDir)) {
@@ -230,12 +247,6 @@ export async function migrate(dryRun = false): Promise<MigrationResult> {
       }
     }
 
-    // 8. Write new config (unless dry run)
-    // Note: Settings remain in *.settings.json files, config.yaml only stores references
-    if (!dryRun) {
-      saveUnifiedConfig(unifiedConfig);
-    }
-
     return {
       success: true,
       backupPath: dryRun ? undefined : backupDir,
@@ -398,12 +409,20 @@ export async function autoMigrate(): Promise<void> {
   }
 
   // Check if inline profile migration is needed (config.yaml exists but missing profiles)
-  if (needsProfileMigration()) {
-    const result = await migrateProfilesToUnified();
+  // CRIT-2: Load data once and pass to migrateProfilesToUnified to avoid TOCTOU race
+  const migrationData = loadMigrationCheckData();
+  if (migrationData.needsMigration) {
+    const result = await migrateProfilesToUnified(migrationData);
     if (result.success && result.migratedFiles.length > 0) {
       console.log('');
       console.log(infoBox('Migrated legacy profiles to config.yaml', 'SUCCESS'));
       console.log(`  Profiles: ${result.migratedFiles.join(', ')}`);
+      // H7: Show collision warnings if any
+      if (result.warnings.length > 0) {
+        for (const warning of result.warnings) {
+          console.log(warn(warning));
+        }
+      }
       console.log('');
     }
   }
@@ -412,15 +431,21 @@ export async function autoMigrate(): Promise<void> {
 /**
  * Migrate only profiles from config.json to existing config.yaml.
  * Used when config.yaml exists but is missing profiles.
+ *
+ * @param preloadedData - Optional pre-loaded config data from loadMigrationCheckData()
+ *                        for TOCTOU-safe operations. If not provided, reads configs fresh.
  */
-async function migrateProfilesToUnified(): Promise<MigrationResult> {
+async function migrateProfilesToUnified(
+  preloadedData?: MigrationCheckData
+): Promise<MigrationResult> {
   const ccsDir = getCcsDir();
   const migratedFiles: string[] = [];
   const warnings: string[] = [];
 
   try {
-    const oldConfig = readJsonSafe(path.join(ccsDir, 'config.json'));
-    const unifiedConfig = loadUnifiedConfig();
+    // Use preloaded data if available (CRIT-2: TOCTOU fix)
+    const oldConfig = preloadedData?.legacyConfig ?? readJsonSafe(path.join(ccsDir, 'config.json'));
+    const unifiedConfig = preloadedData?.unifiedConfig ?? loadUnifiedConfig();
 
     if (!oldConfig?.profiles || !unifiedConfig) {
       return { success: true, migratedFiles, warnings };
@@ -430,12 +455,20 @@ async function migrateProfilesToUnified(): Promise<MigrationResult> {
 
     // Migrate API profiles from config.json
     for (const [name, settingsPath] of Object.entries(oldConfig.profiles)) {
-      // Skip if already in unified config
+      const pathStr = settingsPath as string;
+
+      // H7: Detect collision - profile exists in both configs
       if (unifiedConfig.profiles[name]) {
+        // Check if settings differ (potential data loss)
+        const existingSettings = unifiedConfig.profiles[name].settings;
+        if (existingSettings && existingSettings !== pathStr) {
+          warnings.push(
+            `Profile "${name}" exists in both configs with different settings - keeping existing (${existingSettings}), skipping legacy (${pathStr})`
+          );
+        }
         continue;
       }
 
-      const pathStr = settingsPath as string;
       const expandedPath = expandPath(pathStr);
 
       // Verify settings file exists

src/utils/api-key-validator.ts

@@ -5,6 +5,7 @@
  * Catches expired keys early with actionable error messages.
  */
 
+import * as http from 'http';
 import * as https from 'https';
 import { URL } from 'url';
 
@@ -19,7 +20,7 @@ const DEFAULT_PLACEHOLDERS = [
   'YOUR_GLM_API_KEY_HERE',
   'YOUR_KIMI_API_KEY_HERE',
   'YOUR_API_KEY_HERE',
-  'your-api-key-here',
+  'YOUR-API-KEY-HERE',
   'PLACEHOLDER',
   '',
 ];
@@ -64,14 +65,14 @@ export async function validateGlmKey(
   }
 
   return new Promise((resolve) => {
-    const timeout = setTimeout(() => {
-      // Fail-open on timeout - let Claude CLI handle it
-      resolve({ valid: true });
-    }, timeoutMs);
+    // Determine protocol - use http module for http:// URLs
+    const isHttps = url.protocol === 'https:';
+    const httpModule = isHttps ? https : http;
+    const defaultPort = isHttps ? 443 : 80;
 
     const options: https.RequestOptions = {
       hostname: url.hostname,
-      port: url.port || 443,
+      port: url.port || defaultPort,
       path: url.pathname,
       method: 'GET',
       headers: {
@@ -80,8 +81,8 @@ export async function validateGlmKey(
       },
     };
 
-    const req = https.request(options, (res) => {
-      clearTimeout(timeout);
+    const req = httpModule.request(options, (res) => {
+      clearTimeout(timeoutId);
 
       if (res.statusCode === 200) {
         resolve({ valid: true });
@@ -97,6 +98,12 @@ export async function validateGlmKey(
         });
       } else {
         // Other errors (404, 500, etc.) - fail-open, let Claude CLI handle
+        // Debug log for diagnostics when CCS_DEBUG is set
+        if (process.env.CCS_DEBUG === '1') {
+          console.error(
+            `[CCS-Preflight] Unexpected status ${res.statusCode} from ${url.href} - fail-open`
+          );
+        }
         resolve({ valid: true });
       }
 
@@ -105,11 +112,19 @@ export async function validateGlmKey(
     });
 
     req.on('error', () => {
-      clearTimeout(timeout);
+      clearTimeout(timeoutId);
       // Network error - fail-open
       resolve({ valid: true });
     });
 
+    // Set timeout after request is created so we can destroy it on timeout
+    const timeoutId = setTimeout(() => {
+      // Abort request to prevent TCP connection leak
+      req.destroy();
+      // Fail-open on timeout - let Claude CLI handle it
+      resolve({ valid: true });
+    }, timeoutMs);
+
     req.end();
   });
 }

tests/unit/auth/profile-detector.test.ts

@@ -144,12 +144,13 @@ describe('ProfileDetector', () => {
     it('should return null for unknown profile (throws error)', () => {
       const isUnifiedModeSpy = spyOn(unifiedConfigLoader, 'isUnifiedMode').mockReturnValue(false);
       // Mock readConfig/readProfiles to return empty
-      spyOn(fs, 'existsSync').mockReturnValue(false);
+      const existsSyncSpy = spyOn(fs, 'existsSync').mockReturnValue(false);
 
       try {
         expect(() => detector.detectProfileType('unknown')).toThrow(/Profile not found/);
       } finally {
         isUnifiedModeSpy.mockRestore();
+        existsSyncSpy.mockRestore();
       }
     });
   });

tests/unit/utils/expand-path.test.ts

@@ -66,4 +66,39 @@ describe("expandPath", () => {
     expect(expandPath("${UNDEFINED_VAR}/file.txt")).toBe(path.normalize("/file.txt"));
     expect(expandPath("$UNDEFINED_VAR/file.txt")).toBe(path.normalize("/file.txt"));
   });
+
+  test("11. Windows drive letters stay intact", () => {
+    // Windows drive letter paths should be preserved
+    const result = expandPath("C:\\Users\\test\\file.txt");
+    expect(result).toContain("Users");
+    expect(result).toContain("test");
+  });
+
+  test("12. Windows UNC paths handled", () => {
+    // UNC paths start with \\
+    const uncPath = "\\\\server\\share\\folder";
+    const result = expandPath(uncPath);
+    // Should normalize but preserve the structure
+    expect(result).toContain("server");
+    expect(result).toContain("share");
+  });
+
+  test("13. Null-like input throws TypeError", () => {
+    // Function requires string input - documents current behavior
+    // @ts-ignore - testing runtime edge case
+    expect(() => expandPath(undefined as unknown as string)).toThrow(TypeError);
+    // @ts-ignore - testing runtime edge case
+    expect(() => expandPath(null as unknown as string)).toThrow(TypeError);
+  });
+
+  test("14. Path with spaces preserved", () => {
+    const pathWithSpaces = "~/My Documents/file.txt";
+    const result = expandPath(pathWithSpaces);
+    expect(result).toContain("My Documents");
+  });
+
+  test("15. Multiple consecutive slashes normalized", () => {
+    const result = expandPath("path//to///file.txt");
+    expect(result).toBe(path.normalize("path/to/file.txt"));
+  });
 });

ui/src/lib/provider-presets.ts

@@ -19,6 +19,10 @@ export interface ProviderPreset {
   apiKeyPlaceholder: string;
   apiKeyHint?: string;
   category: PresetCategory;
+  /** Additional env vars for thinking mode, etc. */
+  extraEnv?: Record<string, string>;
+  /** Enable always thinking mode */
+  alwaysThinkingEnabled?: boolean;
 }
 
 export const OPENROUTER_BASE_URL = 'https://openrouter.ai/api';
@@ -141,7 +145,7 @@ export function getPresetsByCategory(category: PresetCategory): ProviderPreset[]
 
 /** Get preset by ID */
 export function getPresetById(id: string): ProviderPreset | undefined {
-  return PROVIDER_PRESETS.find((p) => p.id === id);
+  return PROVIDER_PRESETS.find((p) => p.id.toLowerCase() === id.toLowerCase());
 }
 
 /** Check if a URL matches a known preset */