mirror of
https://github.com/tiennm99/ccs.git
synced 2026-07-17 02:16:43 +00:00
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
This commit is contained in:
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user