From 36864ea11a2815ca53999159bf41f233d3c6d920 Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Thu, 12 Feb 2026 13:40:14 -0800 Subject: [PATCH] fix: harden error handling, fix TOCTOU races, and improve test accuracy Core library fixes: - session-manager.js: wrap all statSync calls in try-catch to prevent TOCTOU crashes when files are deleted between readdir and stat - session-manager.js: use birthtime||ctime fallback for Linux compat - session-manager.js: remove redundant existsSync before readFile - utils.js: fix findFiles TOCTOU race on statSync inside readdir loop Hook improvements: - Add 1MB stdin buffer limits to all PostToolUse hooks to prevent unbounded memory growth from large payloads - suggest-compact.js: use fd-based atomic read+write for counter file to reduce race window between concurrent invocations - session-end.js: log when transcript file is missing, check replaceInFile return value for failed timestamp updates - start-observer.sh: log claude CLI failures instead of silently swallowing them, check observations file exists before analysis Test fixes: - Fix blocking hook tests to send matching input (dev server command) and expect correct exit code 2 instead of 1 --- .../agents/start-observer.sh | 15 ++++++-- scripts/hooks/check-console-log.js | 5 ++- scripts/hooks/post-edit-console-warn.js | 5 ++- scripts/hooks/post-edit-format.js | 5 ++- scripts/hooks/post-edit-typecheck.js | 5 ++- scripts/hooks/session-end.js | 13 +++++-- scripts/hooks/suggest-compact.js | 30 ++++++++++----- scripts/lib/session-manager.js | 38 ++++++++++++------- scripts/lib/utils.js | 9 ++++- .../agents/start-observer.sh | 15 ++++++-- tests/integration/hooks.test.js | 17 ++++++--- 11 files changed, 113 insertions(+), 44 deletions(-) diff --git a/.cursor/skills/continuous-learning-v2/agents/start-observer.sh b/.cursor/skills/continuous-learning-v2/agents/start-observer.sh index 085af88b..42a5f1b3 100755 --- a/.cursor/skills/continuous-learning-v2/agents/start-observer.sh +++ b/.cursor/skills/continuous-learning-v2/agents/start-observer.sh @@ -74,7 +74,10 @@ case "${1:-start}" in trap 'rm -f "$PID_FILE"; exit 0' TERM INT analyze_observations() { - # Only analyze if we have enough observations + # Only analyze if observations file exists and has enough entries + if [ ! -f "$OBSERVATIONS_FILE" ]; then + return + fi obs_count=$(wc -l < "$OBSERVATIONS_FILE" 2>/dev/null || echo 0) if [ "$obs_count" -lt 10 ]; then return @@ -85,16 +88,20 @@ case "${1:-start}" in # Use Claude Code with Haiku to analyze observations # This spawns a quick analysis session if command -v claude &> /dev/null; then - claude --model haiku --max-turns 3 --print \ + if ! claude --model haiku --max-turns 3 --print \ "Read $OBSERVATIONS_FILE and identify patterns. If you find 3+ occurrences of the same pattern, create an instinct file in $CONFIG_DIR/instincts/personal/ following the format in the observer agent spec. Be conservative - only create instincts for clear patterns." \ - >> "$LOG_FILE" 2>&1 || true + >> "$LOG_FILE" 2>&1; then + echo "[$(date)] Claude analysis failed (exit $?)" >> "$LOG_FILE" + fi + else + echo "[$(date)] claude CLI not found, skipping analysis" >> "$LOG_FILE" fi # Archive processed observations if [ -f "$OBSERVATIONS_FILE" ]; then archive_dir="${CONFIG_DIR}/observations.archive" mkdir -p "$archive_dir" - mv "$OBSERVATIONS_FILE" "$archive_dir/processed-$(date +%Y%m%d-%H%M%S).jsonl" + mv "$OBSERVATIONS_FILE" "$archive_dir/processed-$(date +%Y%m%d-%H%M%S).jsonl" 2>/dev/null || true touch "$OBSERVATIONS_FILE" fi } diff --git a/scripts/hooks/check-console-log.js b/scripts/hooks/check-console-log.js index 924af5c2..be290534 100755 --- a/scripts/hooks/check-console-log.js +++ b/scripts/hooks/check-console-log.js @@ -26,10 +26,13 @@ const EXCLUDED_PATTERNS = [ /__mocks__\//, ]; +const MAX_STDIN = 1024 * 1024; // 1MB limit let data = ''; process.stdin.on('data', chunk => { - data += chunk; + if (data.length < MAX_STDIN) { + data += chunk; + } }); process.stdin.on('end', () => { diff --git a/scripts/hooks/post-edit-console-warn.js b/scripts/hooks/post-edit-console-warn.js index e4f5c3c5..a7e5d92a 100644 --- a/scripts/hooks/post-edit-console-warn.js +++ b/scripts/hooks/post-edit-console-warn.js @@ -11,10 +11,13 @@ const fs = require('fs'); +const MAX_STDIN = 1024 * 1024; // 1MB limit let data = ''; process.stdin.on('data', chunk => { - data += chunk; + if (data.length < MAX_STDIN) { + data += chunk; + } }); process.stdin.on('end', () => { diff --git a/scripts/hooks/post-edit-format.js b/scripts/hooks/post-edit-format.js index 8769cd62..adcf2b1f 100644 --- a/scripts/hooks/post-edit-format.js +++ b/scripts/hooks/post-edit-format.js @@ -11,10 +11,13 @@ const { execFileSync } = require('child_process'); const fs = require('fs'); +const MAX_STDIN = 1024 * 1024; // 1MB limit let data = ''; process.stdin.on('data', chunk => { - data += chunk; + if (data.length < MAX_STDIN) { + data += chunk; + } }); process.stdin.on('end', () => { diff --git a/scripts/hooks/post-edit-typecheck.js b/scripts/hooks/post-edit-typecheck.js index 51c013ea..03746c39 100644 --- a/scripts/hooks/post-edit-typecheck.js +++ b/scripts/hooks/post-edit-typecheck.js @@ -13,10 +13,13 @@ const { execFileSync } = require('child_process'); const fs = require('fs'); const path = require('path'); +const MAX_STDIN = 1024 * 1024; // 1MB limit let data = ''; process.stdin.on('data', chunk => { - data += chunk; + if (data.length < MAX_STDIN) { + data += chunk; + } }); process.stdin.on('end', () => { diff --git a/scripts/hooks/session-end.js b/scripts/hooks/session-end.js index df802f78..de7c8afd 100644 --- a/scripts/hooks/session-end.js +++ b/scripts/hooks/session-end.js @@ -99,17 +99,24 @@ async function main() { const transcriptPath = process.env.CLAUDE_TRANSCRIPT_PATH; let summary = null; - if (transcriptPath && fs.existsSync(transcriptPath)) { - summary = extractSessionSummary(transcriptPath); + if (transcriptPath) { + if (fs.existsSync(transcriptPath)) { + summary = extractSessionSummary(transcriptPath); + } else { + log(`[SessionEnd] Transcript not found: ${transcriptPath}`); + } } if (fs.existsSync(sessionFile)) { // Update existing session file - replaceInFile( + const updated = replaceInFile( sessionFile, /\*\*Last Updated:\*\*.*/, `**Last Updated:** ${currentTime}` ); + if (!updated) { + log(`[SessionEnd] Failed to update timestamp in ${sessionFile}`); + } // If we have a new summary and the file still has the blank template, replace it if (summary) { diff --git a/scripts/hooks/suggest-compact.js b/scripts/hooks/suggest-compact.js index 7426c773..87a33fe4 100644 --- a/scripts/hooks/suggest-compact.js +++ b/scripts/hooks/suggest-compact.js @@ -13,10 +13,10 @@ * - Compact after completing a milestone, before starting next */ +const fs = require('fs'); const path = require('path'); const { getTempDir, - readFile, writeFile, log } = require('../lib/utils'); @@ -32,16 +32,28 @@ async function main() { let count = 1; // Read existing count or start at 1 - const existing = readFile(counterFile); - if (existing) { - const parsed = parseInt(existing.trim(), 10); - // Guard against NaN from corrupted counter file - count = Number.isFinite(parsed) ? parsed + 1 : 1; + // Use fd-based read+write to reduce (but not eliminate) race window + // between concurrent hook invocations + try { + const fd = fs.openSync(counterFile, 'a+'); + try { + const buf = Buffer.alloc(64); + const bytesRead = fs.readSync(fd, buf, 0, 64, 0); + if (bytesRead > 0) { + const parsed = parseInt(buf.toString('utf8', 0, bytesRead).trim(), 10); + count = Number.isFinite(parsed) ? parsed + 1 : 1; + } + // Truncate and write new value + fs.ftruncateSync(fd, 0); + fs.writeSync(fd, String(count), 0); + } finally { + fs.closeSync(fd); + } + } catch { + // Fallback: just use writeFile if fd operations fail + writeFile(counterFile, String(count)); } - // Save updated count - writeFile(counterFile, String(count)); - // Suggest compact after threshold tool calls if (count === threshold) { log(`[StrategicCompact] ${threshold} tool calls reached - consider /compact if transitioning phases`); diff --git a/scripts/lib/session-manager.js b/scripts/lib/session-manager.js index d19f8a78..deeae2c5 100644 --- a/scripts/lib/session-manager.js +++ b/scripts/lib/session-manager.js @@ -58,10 +58,6 @@ function getSessionPath(filename) { * @returns {string|null} Session content or null if not found */ function getSessionContent(sessionPath) { - if (!fs.existsSync(sessionPath)) { - return null; - } - return readFile(sessionPath); } @@ -217,8 +213,14 @@ function getAllSessions(options = {}) { const sessionPath = path.join(sessionsDir, filename); - // Get file stats - const stats = fs.statSync(sessionPath); + // Get file stats (wrapped in try-catch to handle TOCTOU race where + // file is deleted between readdirSync and statSync) + let stats; + try { + stats = fs.statSync(sessionPath); + } catch { + continue; // File was deleted between readdir and stat + } sessions.push({ ...metadata, @@ -226,7 +228,7 @@ function getAllSessions(options = {}) { hasContent: stats.size > 0, size: stats.size, modifiedTime: stats.mtime, - createdTime: stats.birthtime + createdTime: stats.birthtime || stats.ctime }); } @@ -278,14 +280,19 @@ function getSessionById(sessionId, includeContent = false) { } const sessionPath = path.join(sessionsDir, filename); - const stats = fs.statSync(sessionPath); + let stats; + try { + stats = fs.statSync(sessionPath); + } catch { + return null; // File was deleted between readdir and stat + } const session = { ...metadata, sessionPath, size: stats.size, modifiedTime: stats.mtime, - createdTime: stats.birthtime + createdTime: stats.birthtime || stats.ctime }; if (includeContent) { @@ -319,11 +326,12 @@ function getSessionTitle(sessionPath) { * @returns {string} Formatted size (e.g., "1.2 KB") */ function getSessionSize(sessionPath) { - if (!fs.existsSync(sessionPath)) { + let stats; + try { + stats = fs.statSync(sessionPath); + } catch { return '0 B'; } - - const stats = fs.statSync(sessionPath); const size = stats.size; if (size < 1024) return `${size} B`; @@ -387,7 +395,11 @@ function deleteSession(sessionPath) { * @returns {boolean} True if session exists */ function sessionExists(sessionPath) { - return fs.existsSync(sessionPath) && fs.statSync(sessionPath).isFile(); + try { + return fs.statSync(sessionPath).isFile(); + } catch { + return false; + } } module.exports = { diff --git a/scripts/lib/utils.js b/scripts/lib/utils.js index 69f9b65d..3cc9dc47 100644 --- a/scripts/lib/utils.js +++ b/scripts/lib/utils.js @@ -168,14 +168,19 @@ function findFiles(dir, pattern, options = {}) { const fullPath = path.join(currentDir, entry.name); if (entry.isFile() && regex.test(entry.name)) { + let stats; + try { + stats = fs.statSync(fullPath); + } catch { + continue; // File deleted between readdir and stat + } + if (maxAge !== null) { - const stats = fs.statSync(fullPath); const ageInDays = (Date.now() - stats.mtimeMs) / (1000 * 60 * 60 * 24); if (ageInDays <= maxAge) { results.push({ path: fullPath, mtime: stats.mtimeMs }); } } else { - const stats = fs.statSync(fullPath); results.push({ path: fullPath, mtime: stats.mtimeMs }); } } else if (entry.isDirectory() && recursive) { diff --git a/skills/continuous-learning-v2/agents/start-observer.sh b/skills/continuous-learning-v2/agents/start-observer.sh index 085af88b..42a5f1b3 100755 --- a/skills/continuous-learning-v2/agents/start-observer.sh +++ b/skills/continuous-learning-v2/agents/start-observer.sh @@ -74,7 +74,10 @@ case "${1:-start}" in trap 'rm -f "$PID_FILE"; exit 0' TERM INT analyze_observations() { - # Only analyze if we have enough observations + # Only analyze if observations file exists and has enough entries + if [ ! -f "$OBSERVATIONS_FILE" ]; then + return + fi obs_count=$(wc -l < "$OBSERVATIONS_FILE" 2>/dev/null || echo 0) if [ "$obs_count" -lt 10 ]; then return @@ -85,16 +88,20 @@ case "${1:-start}" in # Use Claude Code with Haiku to analyze observations # This spawns a quick analysis session if command -v claude &> /dev/null; then - claude --model haiku --max-turns 3 --print \ + if ! claude --model haiku --max-turns 3 --print \ "Read $OBSERVATIONS_FILE and identify patterns. If you find 3+ occurrences of the same pattern, create an instinct file in $CONFIG_DIR/instincts/personal/ following the format in the observer agent spec. Be conservative - only create instincts for clear patterns." \ - >> "$LOG_FILE" 2>&1 || true + >> "$LOG_FILE" 2>&1; then + echo "[$(date)] Claude analysis failed (exit $?)" >> "$LOG_FILE" + fi + else + echo "[$(date)] claude CLI not found, skipping analysis" >> "$LOG_FILE" fi # Archive processed observations if [ -f "$OBSERVATIONS_FILE" ]; then archive_dir="${CONFIG_DIR}/observations.archive" mkdir -p "$archive_dir" - mv "$OBSERVATIONS_FILE" "$archive_dir/processed-$(date +%Y%m%d-%H%M%S).jsonl" + mv "$OBSERVATIONS_FILE" "$archive_dir/processed-$(date +%Y%m%d-%H%M%S).jsonl" 2>/dev/null || true touch "$OBSERVATIONS_FILE" fi } diff --git a/tests/integration/hooks.test.js b/tests/integration/hooks.test.js index a2aee0a9..a9e93551 100644 --- a/tests/integration/hooks.test.js +++ b/tests/integration/hooks.test.js @@ -236,7 +236,7 @@ async function runTests() { })) passed++; else failed++; if (await asyncTest('blocking hooks output BLOCKED message', async () => { - // Test the dev server blocking hook + // Test the dev server blocking hook — must send a matching command const blockingCommand = hooks.hooks.PreToolUse[0].hooks[0].command; const match = blockingCommand.match(/^node -e "(.+)"$/s); @@ -248,6 +248,10 @@ async function runTests() { let code = null; proc.stderr.on('data', data => stderr += data); + // Send a dev server command so the hook triggers the block + proc.stdin.write(JSON.stringify({ + tool_input: { command: 'npm run dev' } + })); proc.stdin.end(); await new Promise(resolve => { @@ -258,7 +262,7 @@ async function runTests() { }); assert.ok(stderr.includes('BLOCKED'), 'Blocking hook should output BLOCKED'); - assert.strictEqual(code, 1, 'Blocking hook should exit with code 1'); + assert.strictEqual(code, 2, 'Blocking hook should exit with code 2'); })) passed++; else failed++; // ========================================== @@ -271,8 +275,8 @@ async function runTests() { assert.strictEqual(result.code, 0, 'Non-blocking hook should exit 0'); })) passed++; else failed++; - if (await asyncTest('blocking hooks exit with code 1', async () => { - // The dev server blocker always blocks + if (await asyncTest('blocking hooks exit with code 2', async () => { + // The dev server blocker blocks when a dev server command is detected const blockingCommand = hooks.hooks.PreToolUse[0].hooks[0].command; const match = blockingCommand.match(/^node -e "(.+)"$/s); @@ -281,6 +285,9 @@ async function runTests() { }); let code = null; + proc.stdin.write(JSON.stringify({ + tool_input: { command: 'yarn dev' } + })); proc.stdin.end(); await new Promise(resolve => { @@ -290,7 +297,7 @@ async function runTests() { }); }); - assert.strictEqual(code, 1, 'Blocking hook should exit 1'); + assert.strictEqual(code, 2, 'Blocking hook should exit 2'); })) passed++; else failed++; if (await asyncTest('hooks handle missing files gracefully', async () => {