From 18c5a76a96901e169027a762186121f96694ae0e Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Thu, 12 Feb 2026 07:06:53 -0800 Subject: [PATCH] fix: improve error handling, fix bugs, and optimize core libraries utils.js: - Fix countInFile: enforce global flag on regex to prevent silent under-counting (match() without /g returns only first match) - Add 5s timeout to readStdinJson to prevent hooks hanging forever - Handle EEXIST race condition in ensureDir - Pre-compile regex patterns in getGitModifiedFiles to avoid N*M compilations and catch invalid patterns before filtering - Add JSDoc documentation to all improved functions session-manager.js: - Fix getSessionById triple file read: pass pre-read content to getSessionStats instead of re-reading from disk - Allow getSessionStats to accept content string directly session-aliases.js: - Wrap temp file cleanup in try/catch to prevent cascading errors check-console-log.js: - Refactor to use shared utils (isGitRepo, getGitModifiedFiles, log) instead of raw execSync calls - Add exclusion patterns for test files, config files, and scripts/ where console.log is intentional session-end.js: - Log count of skipped unparseable transcript lines for diagnostics suggest-compact.js: - Guard against NaN from corrupted counter files package-manager.js: - Remove dead fallbackOrder parameter (unused after #162 fix) --- scripts/hooks/check-console-log.js | 50 +++++++++-------- scripts/hooks/session-end.js | 7 ++- scripts/hooks/suggest-compact.js | 10 ++-- scripts/lib/package-manager.js | 5 +- scripts/lib/session-aliases.js | 10 ++-- scripts/lib/session-manager.js | 19 +++++-- scripts/lib/utils.js | 87 ++++++++++++++++++++++++------ 7 files changed, 134 insertions(+), 54 deletions(-) diff --git a/scripts/hooks/check-console-log.js b/scripts/hooks/check-console-log.js index 9b25fc4e..924af5c2 100755 --- a/scripts/hooks/check-console-log.js +++ b/scripts/hooks/check-console-log.js @@ -2,57 +2,61 @@ /** * Stop Hook: Check for console.log statements in modified files - * - * This hook runs after each response and checks if any modified - * JavaScript/TypeScript files contain console.log statements. - * It provides warnings to help developers remember to remove - * debug statements before committing. + * + * Cross-platform (Windows, macOS, Linux) + * + * Runs after each response and checks if any modified JavaScript/TypeScript + * files contain console.log statements. Provides warnings to help developers + * remember to remove debug statements before committing. + * + * Exclusions: test files, config files, and scripts/ directory (where + * console.log is often intentional). */ -const { execSync } = require('child_process'); const fs = require('fs'); +const { isGitRepo, getGitModifiedFiles, log } = require('../lib/utils'); + +// Files where console.log is expected and should not trigger warnings +const EXCLUDED_PATTERNS = [ + /\.test\.[jt]sx?$/, + /\.spec\.[jt]sx?$/, + /\.config\.[jt]s$/, + /scripts\//, + /__tests__\//, + /__mocks__\//, +]; let data = ''; -// Read stdin process.stdin.on('data', chunk => { data += chunk; }); process.stdin.on('end', () => { try { - // Check if we're in a git repository - try { - execSync('git rev-parse --git-dir', { stdio: 'pipe' }); - } catch { - // Not in a git repo, just pass through the data + if (!isGitRepo()) { console.log(data); process.exit(0); } - // Get list of modified files - const files = execSync('git diff --name-only HEAD', { - encoding: 'utf8', - stdio: ['pipe', 'pipe', 'pipe'] - }) - .split('\n') - .filter(f => /\.(ts|tsx|js|jsx)$/.test(f) && fs.existsSync(f)); + const files = getGitModifiedFiles(['\\.tsx?$', '\\.jsx?$']) + .filter(f => fs.existsSync(f)) + .filter(f => !EXCLUDED_PATTERNS.some(pattern => pattern.test(f))); let hasConsole = false; - // Check each file for console.log for (const file of files) { const content = fs.readFileSync(file, 'utf8'); if (content.includes('console.log')) { - console.error(`[Hook] WARNING: console.log found in ${file}`); + log(`[Hook] WARNING: console.log found in ${file}`); hasConsole = true; } } if (hasConsole) { - console.error('[Hook] Remove console.log statements before committing'); + log('[Hook] Remove console.log statements before committing'); } - } catch (_error) { + } catch { // Silently ignore errors (git might not be available, etc.) } diff --git a/scripts/hooks/session-end.js b/scripts/hooks/session-end.js index 6c1fbed9..df802f78 100644 --- a/scripts/hooks/session-end.js +++ b/scripts/hooks/session-end.js @@ -38,6 +38,7 @@ function extractSessionSummary(transcriptPath) { const userMessages = []; const toolsUsed = new Set(); const filesModified = new Set(); + let parseErrors = 0; for (const line of lines) { try { @@ -66,10 +67,14 @@ function extractSessionSummary(transcriptPath) { } } } catch { - // Skip unparseable lines + parseErrors++; } } + if (parseErrors > 0) { + log(`[SessionEnd] Skipped ${parseErrors}/${lines.length} unparseable transcript lines`); + } + if (userMessages.length === 0) return null; return { diff --git a/scripts/hooks/suggest-compact.js b/scripts/hooks/suggest-compact.js index 323e34c9..7426c773 100644 --- a/scripts/hooks/suggest-compact.js +++ b/scripts/hooks/suggest-compact.js @@ -23,9 +23,9 @@ const { async function main() { // Track tool call count (increment in a temp file) - // Use a session-specific counter file based on PID from parent process - // or session ID from environment - const sessionId = process.env.CLAUDE_SESSION_ID || process.ppid || 'default'; + // Use a session-specific counter file based on session ID from environment + // or parent PID as fallback + const sessionId = process.env.CLAUDE_SESSION_ID || String(process.ppid) || 'default'; const counterFile = path.join(getTempDir(), `claude-tool-count-${sessionId}`); const threshold = parseInt(process.env.COMPACT_THRESHOLD || '50', 10); @@ -34,7 +34,9 @@ async function main() { // Read existing count or start at 1 const existing = readFile(counterFile); if (existing) { - count = parseInt(existing.trim(), 10) + 1; + const parsed = parseInt(existing.trim(), 10); + // Guard against NaN from corrupted counter file + count = Number.isFinite(parsed) ? parsed + 1 : 1; } // Save updated count diff --git a/scripts/lib/package-manager.js b/scripts/lib/package-manager.js index 440cb0b3..4e8b60a0 100644 --- a/scripts/lib/package-manager.js +++ b/scripts/lib/package-manager.js @@ -156,11 +156,12 @@ function getAvailablePackageManagers() { * 5. Global user preference (in ~/.claude/package-manager.json) * 6. Default to npm (no child processes spawned) * - * @param {object} options - { projectDir, fallbackOrder } + * @param {object} options - Options + * @param {string} options.projectDir - Project directory to detect from (default: cwd) * @returns {object} - { name, config, source } */ function getPackageManager(options = {}) { - const { projectDir = process.cwd(), fallbackOrder = DETECTION_PRIORITY } = options; + const { projectDir = process.cwd() } = options; // 1. Check environment variable const envPm = process.env.CLAUDE_PACKAGE_MANAGER; diff --git a/scripts/lib/session-aliases.js b/scripts/lib/session-aliases.js index 2635b41c..f6172e24 100644 --- a/scripts/lib/session-aliases.js +++ b/scripts/lib/session-aliases.js @@ -135,9 +135,13 @@ function saveAliases(aliases) { } } - // Clean up temp file - if (fs.existsSync(tempPath)) { - fs.unlinkSync(tempPath); + // Clean up temp file (best-effort) + try { + if (fs.existsSync(tempPath)) { + fs.unlinkSync(tempPath); + } + } catch { + // Non-critical: temp file will be overwritten on next save } return false; diff --git a/scripts/lib/session-manager.js b/scripts/lib/session-manager.js index cc9c3c60..d19f8a78 100644 --- a/scripts/lib/session-manager.js +++ b/scripts/lib/session-manager.js @@ -143,11 +143,21 @@ function parseSessionMetadata(content) { /** * Calculate statistics for a session - * @param {string} sessionPath - Full path to session file + * @param {string} sessionPathOrContent - Full path to session file, OR + * the pre-read content string (to avoid redundant disk reads when + * the caller already has the content loaded). * @returns {object} Statistics object */ -function getSessionStats(sessionPath) { - const content = getSessionContent(sessionPath); +function getSessionStats(sessionPathOrContent) { + // Accept pre-read content string to avoid redundant file reads. + // If the argument looks like a file path (no newlines, ends with .tmp), + // read from disk. Otherwise treat it as content. + const content = (typeof sessionPathOrContent === 'string' && + !sessionPathOrContent.includes('\n') && + sessionPathOrContent.endsWith('.tmp')) + ? getSessionContent(sessionPathOrContent) + : sessionPathOrContent; + const metadata = parseSessionMetadata(content); return { @@ -281,7 +291,8 @@ function getSessionById(sessionId, includeContent = false) { if (includeContent) { session.content = getSessionContent(sessionPath); session.metadata = parseSessionMetadata(session.content); - session.stats = getSessionStats(sessionPath); + // Pass pre-read content to avoid a redundant disk read + session.stats = getSessionStats(session.content || ''); } return session; diff --git a/scripts/lib/utils.js b/scripts/lib/utils.js index 5369ba00..69f9b65d 100644 --- a/scripts/lib/utils.js +++ b/scripts/lib/utils.js @@ -57,10 +57,20 @@ function getTempDir() { /** * Ensure a directory exists (create if not) + * @param {string} dirPath - Directory path to create + * @returns {string} The directory path + * @throws {Error} If directory cannot be created (e.g., permission denied) */ function ensureDir(dirPath) { - if (!fs.existsSync(dirPath)) { - fs.mkdirSync(dirPath, { recursive: true }); + try { + if (!fs.existsSync(dirPath)) { + fs.mkdirSync(dirPath, { recursive: true }); + } + } catch (err) { + // EEXIST is fine (race condition with another process creating it) + if (err.code !== 'EEXIST') { + throw err; + } } return dirPath; } @@ -187,10 +197,29 @@ function findFiles(dir, pattern, options = {}) { /** * Read JSON from stdin (for hook input) + * @param {object} options - Options + * @param {number} options.timeoutMs - Timeout in milliseconds (default: 5000). + * Prevents hooks from hanging indefinitely if stdin never closes. + * @returns {Promise} Parsed JSON object, or empty object if stdin is empty */ -async function readStdinJson() { +async function readStdinJson(options = {}) { + const { timeoutMs = 5000 } = options; + return new Promise((resolve, reject) => { let data = ''; + let settled = false; + + const timer = setTimeout(() => { + if (!settled) { + settled = true; + // Resolve with whatever we have so far rather than hanging + try { + resolve(data.trim() ? JSON.parse(data) : {}); + } catch { + resolve({}); + } + } + }, timeoutMs); process.stdin.setEncoding('utf8'); process.stdin.on('data', chunk => { @@ -198,18 +227,22 @@ async function readStdinJson() { }); process.stdin.on('end', () => { + if (settled) return; + settled = true; + clearTimeout(timer); try { - if (data.trim()) { - resolve(JSON.parse(data)); - } else { - resolve({}); - } + resolve(data.trim() ? JSON.parse(data) : {}); } catch (err) { reject(err); } }); - process.stdin.on('error', reject); + process.stdin.on('error', err => { + if (settled) return; + settled = true; + clearTimeout(timer); + reject(err); + }); }); } @@ -313,7 +346,10 @@ function isGitRepo() { } /** - * Get git modified files + * Get git modified files, optionally filtered by regex patterns + * @param {string[]} patterns - Array of regex pattern strings to filter files. + * Invalid patterns are silently skipped. + * @returns {string[]} Array of modified file paths */ function getGitModifiedFiles(patterns = []) { if (!isGitRepo()) return []; @@ -324,12 +360,18 @@ function getGitModifiedFiles(patterns = []) { let files = result.output.split('\n').filter(Boolean); if (patterns.length > 0) { - files = files.filter(file => { - return patterns.some(pattern => { - const regex = new RegExp(pattern); - return regex.test(file); - }); - }); + // Pre-compile patterns, skipping invalid ones + const compiled = []; + for (const pattern of patterns) { + try { + compiled.push(new RegExp(pattern)); + } catch { + // Skip invalid regex patterns + } + } + if (compiled.length > 0) { + files = files.filter(file => compiled.some(regex => regex.test(file))); + } } return files; @@ -349,12 +391,23 @@ function replaceInFile(filePath, search, replace) { /** * Count occurrences of a pattern in a file + * @param {string} filePath - Path to the file + * @param {string|RegExp} pattern - Pattern to count. Strings are treated as + * global regex patterns. RegExp instances are used as-is but the global + * flag is enforced to ensure correct counting. + * @returns {number} Number of matches found */ function countInFile(filePath, pattern) { const content = readFile(filePath); if (content === null) return 0; - const regex = pattern instanceof RegExp ? pattern : new RegExp(pattern, 'g'); + let regex; + if (pattern instanceof RegExp) { + // Ensure global flag is set for correct counting + regex = pattern.global ? pattern : new RegExp(pattern.source, pattern.flags + 'g'); + } else { + regex = new RegExp(pattern, 'g'); + } const matches = content.match(regex); return matches ? matches.length : 0; }