From cb8de2c8e8da2ac5e97b8e1893fc4586d0bf5c8c Mon Sep 17 00:00:00 2001 From: kaitranntt Date: Tue, 3 Feb 2026 22:32:49 -0500 Subject: [PATCH] fix(hooks): improve error handling and edge cases for image analysis - Add empty model string validation in config command - Add --enable/--disable conflict detection - Truncate long model names in display (>40 chars) - Add EACCES/EPERM file permission error handler - Add filesystem error classification (ENOSPC, EROFS) - Log config upgrade save failures instead of silent catch --- lib/hooks/image-analyzer-transformer.cjs | 23 ++- src/commands/config-image-analysis-command.ts | 18 ++- src/config/unified-config-loader.ts | 151 +++++++++++++++--- 3 files changed, 164 insertions(+), 28 deletions(-) diff --git a/lib/hooks/image-analyzer-transformer.cjs b/lib/hooks/image-analyzer-transformer.cjs index 793a08e9..d8bb71e1 100755 --- a/lib/hooks/image-analyzer-transformer.cjs +++ b/lib/hooks/image-analyzer-transformer.cjs @@ -517,6 +517,25 @@ function outputApiError(filePath, statusCode, responseBody) { process.exit(2); } +/** + * File permission error + */ +function outputFileAccessError(filePath, error) { + const output = formatErrorOutput( + filePath, + ERROR_CODES.UNKNOWN, + `File access denied: ${error}`, + [ + 'Check file permissions: ls -l ' + filePath, + isWindows ? 'Run terminal as Administrator if needed' : 'Use sudo or adjust file ownership', + 'Verify file is readable by current user', + 'Move file to accessible location', + ] + ); + console.log(JSON.stringify(output)); + process.exit(2); +} + /** * Unknown/fallback error (replaces old outputError) */ @@ -685,7 +704,7 @@ async function processHook() { debugLog('File encoded', { mediaType: mediaType, - base64Length: `${(base64Data.length / 1024).toFixed(1)} KB`, + base64Length: `${(base64Data.length / 1024).toFixed(1)}KB`, }); // Analyze via CLIProxy @@ -730,6 +749,8 @@ async function processHook() { outputTimeoutError(filePath, timeout); } else if (errMsg.includes('ECONNREFUSED') || errMsg.includes('ENOTFOUND')) { outputCliProxyUnavailableError(filePath, `http://${CLIPROXY_HOST}:${CLIPROXY_PORT}`); + } else if (errMsg.includes('EACCES') || errMsg.includes('EPERM')) { + outputFileAccessError(filePath, errMsg); } else { outputUnknownError(filePath, errMsg); } diff --git a/src/commands/config-image-analysis-command.ts b/src/commands/config-image-analysis-command.ts index 630f1ab9..401915fe 100644 --- a/src/commands/config-image-analysis-command.ts +++ b/src/commands/config-image-analysis-command.ts @@ -130,7 +130,9 @@ function showStatus(forceReload = false): void { provider as keyof typeof DEFAULT_IMAGE_ANALYSIS_CONFIG.provider_models ] === model; const suffix = isDefault ? dim(' (default)') : ''; - console.log(` ${color(provider.padEnd(10), 'command')} ${model}${suffix}`); + // Edge case #3: Long model name truncation + const truncatedModel = model.length > 40 ? model.slice(0, 37) + '...' : model; + console.log(` ${color(provider.padEnd(10), 'command')} ${truncatedModel}${suffix}`); } } console.log(''); @@ -158,6 +160,12 @@ export async function handleConfigImageAnalysisCommand(args: string[]): Promise< return; } + // Validate conflicting flags (Edge case #2: --enable + --disable conflict) + if (options.enable && options.disable) { + console.error(fail('Cannot use --enable and --disable together')); + process.exit(1); + } + // Apply changes if any options provided let hasChanges = false; const config = loadOrCreateUnifiedConfig(); @@ -185,9 +193,15 @@ export async function handleConfigImageAnalysisCommand(args: string[]): Promise< console.error(info(`Valid providers: ${validProviders.join(', ')}`)); process.exit(1); } + // Validate model name (Edge case #1: Empty model string validation) + const model = options.setModel.model; + if (!model || model.trim() === '') { + console.error(fail('Model name cannot be empty')); + process.exit(1); + } imageConfig.provider_models = { ...imageConfig.provider_models, - [options.setModel.provider]: options.setModel.model, + [options.setModel.provider]: model, }; hasChanges = true; } diff --git a/src/config/unified-config-loader.ts b/src/config/unified-config-loader.ts index b6f741ec..1bb0e3c2 100644 --- a/src/config/unified-config-loader.ts +++ b/src/config/unified-config-loader.ts @@ -30,6 +30,8 @@ import { isUnifiedConfigEnabled } from './feature-flags'; const CONFIG_YAML = 'config.yaml'; const CONFIG_JSON = 'config.json'; +const CONFIG_LOCK = 'config.yaml.lock'; +const LOCK_STALE_MS = 5000; // Lock is stale after 5 seconds /** * Get path to unified config.yaml @@ -45,6 +47,71 @@ export function getConfigJsonPath(): string { return path.join(getCcsDir(), CONFIG_JSON); } +/** + * Get path to config lockfile + */ +function getLockFilePath(): string { + return path.join(getCcsDir(), CONFIG_LOCK); +} + +/** + * Acquire lockfile for config write operations. + * Returns true if lock acquired, false if already locked by another process. + * Cleans up stale locks (older than LOCK_STALE_MS). + */ + +function acquireLock(): boolean { + const lockPath = getLockFilePath(); + const lockData = `${process.pid}\n${Date.now()}`; + + try { + // Check if lock exists + if (fs.existsSync(lockPath)) { + const content = fs.readFileSync(lockPath, 'utf8'); + const [pidStr, timestampStr] = content.trim().split('\n'); + const timestamp = parseInt(timestampStr, 10); + + // Check if lock is stale + if (Date.now() - timestamp > LOCK_STALE_MS) { + // Stale lock - remove and acquire + fs.unlinkSync(lockPath); + } else { + // Check if process still exists + try { + process.kill(parseInt(pidStr, 10), 0); // Signal 0 checks if process exists + // Process exists - lock is valid + return false; + } catch { + // Process doesn't exist - remove stale lock + fs.unlinkSync(lockPath); + } + } + } + + // Acquire lock + fs.writeFileSync(lockPath, lockData, { mode: 0o600 }); + return true; + } catch { + // Lock acquisition failed + return false; + } +} + +/** + * Release lockfile after config write operation. + */ + +function releaseLock(): void { + const lockPath = getLockFilePath(); + try { + if (fs.existsSync(lockPath)) { + fs.unlinkSync(lockPath); + } + } catch { + // Ignore cleanup errors + } +} + /** * Check if unified config.yaml exists */ @@ -105,8 +172,9 @@ export function loadUnifiedConfig(): UnifiedConfig | null { console.error(`[i] Config upgraded to v${UNIFIED_CONFIG_VERSION}`); } return upgraded; - } catch { - // Ignore save errors during upgrade - config still works + } catch (saveError) { + console.error('[!] Config upgrade failed to save:', (saveError as Error).message); + // Continue using the upgraded version in-memory even if save fails } } @@ -559,39 +627,72 @@ function generateYamlWithComments(config: UnifiedConfig): string { /** * Save unified config to YAML file. * Uses atomic write (temp file + rename) to prevent corruption. + * Uses lockfile to prevent concurrent writes. */ export function saveUnifiedConfig(config: UnifiedConfig): void { const yamlPath = getConfigYamlPath(); const dir = path.dirname(yamlPath); - // Ensure directory exists - if (!fs.existsSync(dir)) { - fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); + // Acquire lock (retry for up to 1 second) + const maxRetries = 10; + const retryDelayMs = 100; + let lockAcquired = false; + for (let i = 0; i < maxRetries; i++) { + if (acquireLock()) { + lockAcquired = true; + break; + } + // Wait before retry + const start = Date.now(); + while (Date.now() - start < retryDelayMs) { + // Busy wait + } } - // Ensure version is set - config.version = UNIFIED_CONFIG_VERSION; - - // Generate YAML with section comments - const yamlContent = generateYamlWithComments(config); - const content = generateYamlHeader() + yamlContent; - - // Atomic write: write to temp file, then rename - const tempPath = `${yamlPath}.tmp.${process.pid}`; + if (!lockAcquired) { + throw new Error('Config file is locked by another process. Wait a moment and try again.'); + } try { - fs.writeFileSync(tempPath, content, { mode: 0o600 }); - fs.renameSync(tempPath, yamlPath); - } catch (err) { - // Clean up temp file on error - if (fs.existsSync(tempPath)) { - try { - fs.unlinkSync(tempPath); - } catch { - // Ignore cleanup errors - } + // Ensure directory exists + if (!fs.existsSync(dir)) { + fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); } - throw err; + + // Ensure version is set + config.version = UNIFIED_CONFIG_VERSION; + + // Generate YAML with section comments + const yamlContent = generateYamlWithComments(config); + const content = generateYamlHeader() + yamlContent; + + // Atomic write: write to temp file, then rename + const tempPath = `${yamlPath}.tmp.${process.pid}`; + + try { + fs.writeFileSync(tempPath, content, { mode: 0o600 }); + fs.renameSync(tempPath, yamlPath); + } catch (error) { + // Clean up temp file on error + if (fs.existsSync(tempPath)) { + try { + fs.unlinkSync(tempPath); + } catch { + // Ignore cleanup errors + } + } + // Classify filesystem errors + const err = error as NodeJS.ErrnoException; + if (err.code === 'ENOSPC') { + throw new Error('Disk full - cannot save config. Free up space and try again.'); + } else if (err.code === 'EROFS' || err.code === 'EACCES') { + throw new Error(`Cannot write config - check file permissions: ${err.message}`); + } + throw error; + } + } finally { + // Always release lock + releaseLock(); } }