diff --git a/scripts/hooks/suggest-compact.js b/scripts/hooks/suggest-compact.js index 0e9d5e66..0638ae2b 100644 --- a/scripts/hooks/suggest-compact.js +++ b/scripts/hooks/suggest-compact.js @@ -23,6 +23,72 @@ const { output } = require('../lib/utils'); +const COUNTER_FILE_PREFIX = 'claude-tool-count-'; +const DEFAULT_COMPACT_STATE_TTL_DAYS = 14; + +function getCounterRetentionDays() { + const raw = process.env.COMPACT_STATE_TTL_DAYS; + if (!raw) return DEFAULT_COMPACT_STATE_TTL_DAYS; + const parsed = Number.parseInt(raw, 10); + return Number.isInteger(parsed) && parsed > 0 ? parsed : DEFAULT_COMPACT_STATE_TTL_DAYS; +} + +/** + * Sweep stale counter files from the temp dir. + * + * Each session writes `claude-tool-count-` into the OS temp + * dir; nothing else removes them. Without a sweep these files accumulate + * one-per-session forever. This helper removes counters whose mtime is + * older than `retentionDays`, while preserving the active session's + * counter (which is about to be re-written by the caller). + * + * The helper never throws; per the always-exit-0 hook contract any + * filesystem failure is swallowed and logged to stderr. + * + * @param {string} tempDir - The temp directory to sweep. + * @param {number} retentionDays - Files older than this many days are removed. + * @param {string} currentCounterFile - Absolute path of the active session's + * counter file; preserved unconditionally. + */ +function cleanupOldCounters(tempDir, retentionDays, currentCounterFile) { + let entries; + try { + entries = fs.readdirSync(tempDir, { withFileTypes: true }); + } catch (err) { + log(`[StrategicCompact] Skipping counter sweep; readdir failed: ${err.message}`); + return; + } + + const cutoffMs = Date.now() - retentionDays * 24 * 60 * 60 * 1000; + const currentBasename = path.basename(currentCounterFile); + + for (const entry of entries) { + if (!entry.isFile()) continue; + if (!entry.name.startsWith(COUNTER_FILE_PREFIX)) continue; + if (entry.name === currentBasename) continue; + + const fullPath = path.join(tempDir, entry.name); + let stats; + try { + stats = fs.statSync(fullPath); + } catch { + continue; + } + + // Strict "older than" semantics per the docstring: a file whose mtime + // sits exactly on the cutoff boundary has age == retentionDays, which + // is not *older than* retentionDays, so preserve it. Use >= so only + // strictly older files (mtimeMs < cutoffMs) fall through to deletion. + if (stats.mtimeMs >= cutoffMs) continue; + + try { + fs.rmSync(fullPath, { force: true }); + } catch (err) { + log(`[StrategicCompact] Warning: failed to prune stale counter ${fullPath}: ${err.message}`); + } + } +} + async function resolveSessionId() { // Claude Code passes hook input via stdin JSON; session_id is the // canonical field. Fall back to the legacy env var, then 'default'. @@ -43,7 +109,13 @@ async function main() { // legacy env var, or 'default' as fallback. const rawSessionId = await resolveSessionId(); const sessionId = rawSessionId.replace(/[^a-zA-Z0-9_-]/g, '') || 'default'; - const counterFile = path.join(getTempDir(), `claude-tool-count-${sessionId}`); + const tempDir = getTempDir(); + const counterFile = path.join(tempDir, `${COUNTER_FILE_PREFIX}${sessionId}`); + + // Sweep stale counter files (concern 1 of #2156). Cheap, swallows errors, + // skips the active session's file. See cleanupOldCounters for details. + cleanupOldCounters(tempDir, getCounterRetentionDays(), counterFile); + const rawThreshold = parseInt(process.env.COMPACT_THRESHOLD || '50', 10); const threshold = Number.isFinite(rawThreshold) && rawThreshold > 0 && rawThreshold <= 10000 ? rawThreshold diff --git a/tests/hooks/suggest-compact.test.js b/tests/hooks/suggest-compact.test.js index dc3c5c23..2ad564a4 100644 --- a/tests/hooks/suggest-compact.test.js +++ b/tests/hooks/suggest-compact.test.js @@ -451,6 +451,192 @@ function runTests() { })) passed++; else failed++; + // ── Counter file cleanup (#2156) ── + // claude-tool-count- files were never removed. The hook now + // sweeps stale counters older than COMPACT_STATE_TTL_DAYS (default 14) + // before opening the active counter. These tests pin the contract. + console.log('\nCounter file cleanup (#2156):'); + + /** + * Set a file's mtime/atime to N days ago. + */ + function setMtimeDaysAgo(filePath, daysAgo) { + const seconds = Math.floor(Date.now() / 1000) - daysAgo * 24 * 60 * 60; + fs.utimesSync(filePath, seconds, seconds); + } + + if (test('removes counter files older than retention window', () => { + const { sessionId, cleanup } = createCounterContext(); + const stale = getCounterFilePath(`stale-${Date.now()}`); + fs.writeFileSync(stale, '1'); + setMtimeDaysAgo(stale, 30); + try { + const result = runCompact({ CLAUDE_SESSION_ID: sessionId }); + assert.strictEqual(result.code, 0, 'Should exit 0'); + assert.ok(!fs.existsSync(stale), + `Stale counter file should have been swept. Path: ${stale}`); + } finally { + try { fs.unlinkSync(stale); } catch (_err) { /* ignore */ } + cleanup(); + } + })) passed++; + else failed++; + + if (test('preserves counter files within retention window', () => { + const { sessionId, cleanup } = createCounterContext(); + const fresh = getCounterFilePath(`fresh-${Date.now()}`); + fs.writeFileSync(fresh, '1'); + setMtimeDaysAgo(fresh, 5); + try { + runCompact({ CLAUDE_SESSION_ID: sessionId }); + assert.ok(fs.existsSync(fresh), + `Fresh counter file should be preserved. Path: ${fresh}`); + } finally { + try { fs.unlinkSync(fresh); } catch (_err) { /* ignore */ } + cleanup(); + } + })) passed++; + else failed++; + + if (test('preserves the active session\'s counter file even if old', () => { + const { sessionId, counterFile, cleanup } = createCounterContext(); + cleanup(); + fs.writeFileSync(counterFile, '7'); + setMtimeDaysAgo(counterFile, 30); + try { + const result = runCompact({ CLAUDE_SESSION_ID: sessionId }); + assert.strictEqual(result.code, 0, 'Should exit 0'); + // Active counter survives the sweep AND is incremented by the hook (7 -> 8). + assert.ok(fs.existsSync(counterFile), + 'Active session counter must survive the sweep'); + const count = parseInt(fs.readFileSync(counterFile, 'utf8').trim(), 10); + assert.strictEqual(count, 8, + `Active counter should be incremented by the hook. Got ${count}`); + } finally { + cleanup(); + } + })) passed++; + else failed++; + + if (test('honours COMPACT_STATE_TTL_DAYS env var', () => { + const { sessionId, cleanup } = createCounterContext(); + const target = getCounterFilePath(`ttl-${Date.now()}`); + fs.writeFileSync(target, '1'); + setMtimeDaysAgo(target, 5); // Within default 14d window, but outside TTL=3 + try { + const result = runCompact({ + CLAUDE_SESSION_ID: sessionId, + COMPACT_STATE_TTL_DAYS: '3' + }); + assert.strictEqual(result.code, 0); + assert.ok(!fs.existsSync(target), + `TTL=3 should sweep a 5-day-old file. Path: ${target}`); + } finally { + try { fs.unlinkSync(target); } catch (_err) { /* ignore */ } + cleanup(); + } + })) passed++; + else failed++; + + if (test('falls back to default for invalid COMPACT_STATE_TTL_DAYS', () => { + const { sessionId, cleanup } = createCounterContext(); + const target = getCounterFilePath(`fallback-${Date.now()}`); + fs.writeFileSync(target, '1'); + setMtimeDaysAgo(target, 5); // Within default 14d window, would survive a fallback + try { + // Each invalid form: zero, negative, non-numeric — should fall back to 14d default. + for (const bad of ['0', '-5', 'abc']) { + // Reset mtime each iteration so the file remains 5 days old. + setMtimeDaysAgo(target, 5); + const result = runCompact({ + CLAUDE_SESSION_ID: sessionId, + COMPACT_STATE_TTL_DAYS: bad + }); + assert.strictEqual(result.code, 0); + assert.ok(fs.existsSync(target), + `Invalid TTL '${bad}' should fall back to default (14d) and preserve a 5-day-old file`); + } + } finally { + try { fs.unlinkSync(target); } catch (_err) { /* ignore */ } + cleanup(); + } + })) passed++; + else failed++; + + if (test('does not touch unrelated temp files', () => { + const { sessionId, cleanup } = createCounterContext(); + const unrelated = path.join(os.tmpdir(), `unrelated-${Date.now()}.tmp`); + fs.writeFileSync(unrelated, 'do not touch'); + setMtimeDaysAgo(unrelated, 60); + try { + runCompact({ CLAUDE_SESSION_ID: sessionId }); + assert.ok(fs.existsSync(unrelated), + `Unrelated temp file should not be swept. Path: ${unrelated}`); + } finally { + try { fs.unlinkSync(unrelated); } catch (_err) { /* ignore */ } + cleanup(); + } + })) passed++; + else failed++; + + if (test('preserves files whose mtime sits at or after the TTL cutoff', () => { + // Contract: docstring says files "older than" retentionDays are removed. + // A file at the exact boundary (age == retentionDays) is NOT older than + // retentionDays, so it must survive the sweep. Pins the >= comparison + // in cleanupOldCounters: anything with mtimeMs >= cutoffMs is skipped. + // + // We can't pin the boundary by clock — the sweep computes its own + // Date.now() after this test runs, so `setMtimeDaysAgo(file, 14)` is + // effectively "14d + handful of ms", placing the file just past the + // cutoff. To exercise the boundary deterministically, set the file's + // mtime two seconds *newer* than the projected cutoff: with `>` the + // file would be deleted (mtimeMs > cutoffMs is false at the cutoff + // edge); with `>=` it survives. + const { sessionId, cleanup } = createCounterContext(); + const boundary = getCounterFilePath(`boundary-${Date.now()}`); + fs.writeFileSync(boundary, '1'); + const retentionDays = 14; + const boundaryMs = Date.now() - retentionDays * 24 * 60 * 60 * 1000 + 2000; + const sec = Math.floor(boundaryMs / 1000); + fs.utimesSync(boundary, sec, sec); + try { + const result = runCompact({ CLAUDE_SESSION_ID: sessionId }); + assert.strictEqual(result.code, 0); + assert.ok(fs.existsSync(boundary), + `Boundary-aged counter file should be preserved. Path: ${boundary}`); + } finally { + try { fs.unlinkSync(boundary); } catch (_err) { /* ignore */ } + cleanup(); + } + })) passed++; + else failed++; + + if (test('exit 0 holds when sweep encounters a populated temp dir', () => { + // Functional smoke: with a mix of stale, fresh, and unrelated files + // present, the hook must still exit 0 — the always-exit-0 contract + // takes precedence over sweep failures. + const { sessionId, cleanup } = createCounterContext(); + const stale = getCounterFilePath(`mix-stale-${Date.now()}`); + const fresh = getCounterFilePath(`mix-fresh-${Date.now()}`); + const unrelated = path.join(os.tmpdir(), `mix-unrelated-${Date.now()}.tmp`); + fs.writeFileSync(stale, '1'); + fs.writeFileSync(fresh, '1'); + fs.writeFileSync(unrelated, '1'); + setMtimeDaysAgo(stale, 30); + setMtimeDaysAgo(fresh, 1); + setMtimeDaysAgo(unrelated, 30); + try { + const result = runCompact({ CLAUDE_SESSION_ID: sessionId }); + assert.strictEqual(result.code, 0, 'Hook must exit 0 even with files in temp dir'); + } finally { + for (const p of [stale, fresh, unrelated]) { + try { fs.unlinkSync(p); } catch (_err) { /* ignore */ } + } + cleanup(); + } + })) passed++; + else failed++; + // Summary console.log(` Results: Passed: ${passed}, Failed: ${failed}`);